diff --git a/__tests__/unit/cat/forget-failure-is-not-no-match.test.ts b/__tests__/unit/cat/forget-failure-is-not-no-match.test.ts new file mode 100644 index 000000000..ceac62a5a --- /dev/null +++ b/__tests__/unit/cat/forget-failure-is-not-no-match.test.ts @@ -0,0 +1,105 @@ +/** + * "We could not look" is not "you have no such memory". + * + * Both forget stores used to swallow database errors into + * `{ deleted: [], notFound: wanted }` — the exact shape they return when they + * looked properly and found nothing. So: + * + * * a failed SELECT told the user no such memory existed; + * * a failed DELETE told the user nothing matched, while the memory they had + * just disowned was still sitting there. + * + * Telling someone their data is gone when it is not is the worst thing a + * forget feature can do, and it did it silently. bitbaum/orangecat#563 + * finding 8. + */ + +import { forgetMemoriesMatching } from '@/services/cat/memory'; + +jest.mock('@/utils/logger', () => ({ + logger: { error: jest.fn(), warn: jest.fn(), info: jest.fn(), debug: jest.fn() }, +})); + +jest.mock('@/services/ai/embeddings', () => ({ + embeddingsEnabled: () => false, + embedText: jest.fn(), + embedTexts: jest.fn(), +})); + +const FACTS = ['photography skills']; +const STORED = { id: 'm1', content: 'Has photography skills from years of work' }; + +/** + * Minimal Supabase stand-in: `select().eq()` resolves the load, `delete()…in()` + * resolves the delete. Either can be told to fail. + */ +function makeClient(opts: { loadError?: unknown; deleteError?: unknown } = {}) { + return { + from: () => ({ + select: () => ({ + eq: () => + Promise.resolve( + opts.loadError + ? { data: null, error: opts.loadError } + : { data: [STORED], error: null } + ), + }), + delete: () => ({ + eq: () => ({ + in: () => Promise.resolve({ error: opts.deleteError ?? null }), + }), + }), + upsert: () => Promise.resolve({ error: null }), + insert: () => Promise.resolve({ error: null }), + }), + rpc: () => Promise.resolve({ data: [], error: null }), + } as never; +} + +describe('forgetMemoriesMatching — failure is its own channel', () => { + beforeEach(() => jest.clearAllMocks()); + + it('deletes and reports it, when the store works', async () => { + const result = await forgetMemoriesMatching(makeClient(), 'u1', FACTS); + + expect(result.deleted).toEqual([STORED.content]); + expect(result.failed).toEqual([]); + expect(result.notFound).toEqual([]); + }); + + it('does NOT claim the memory is absent when the read fails', async () => { + const result = await forgetMemoriesMatching( + makeClient({ loadError: { message: 'connection refused' } }), + 'u1', + FACTS + ); + + // The bug: this used to be notFound, i.e. "you have no such memory". + expect(result.failed).toEqual(FACTS); + expect(result.notFound).toEqual([]); + expect(result.deleted).toEqual([]); + }); + + it('does NOT claim nothing matched when the DELETE fails', async () => { + const result = await forgetMemoriesMatching( + makeClient({ deleteError: { message: 'deadlock detected' } }), + 'u1', + FACTS + ); + + // We matched this memory and failed to remove it — it is still there. + // Reporting notFound would be the exact opposite of the truth. + expect(result.failed).toEqual([STORED.content]); + expect(result.deleted).toEqual([]); + expect(result.notFound).not.toContain(FACTS[0]); + }); + + it('reports a genuine miss as notFound, not as a failure', async () => { + // The distinction has to cut both ways, or callers learn to ignore it. + const result = await forgetMemoriesMatching(makeClient(), 'u1', ['unicycle repair']); + + expect(result.notFound).toEqual(['unicycle repair']); + expect(result.failed).toEqual([]); + expect(result.deleted).toEqual([]); + }); +}); diff --git a/__tests__/unit/cat/memory-forget.test.ts b/__tests__/unit/cat/memory-forget.test.ts index c83cddd8d..67aeb6fb0 100644 --- a/__tests__/unit/cat/memory-forget.test.ts +++ b/__tests__/unit/cat/memory-forget.test.ts @@ -72,11 +72,16 @@ describe('forgetMemoriesMatching', () => { expect(result.notFound).toEqual(['plays the tuba']); }); - it('a failed delete reports nothing as deleted', async () => { + // Was: `expect(result.notFound).toEqual(['photography'])`. That assertion + // pinned the bug — a failed DELETE reported the fact as "not found", so Cat + // told the user no such memory existed while it was still stored. The store + // matched it; the removal is what failed. bitbaum/orangecat#563 finding 8. + it('a failed delete reports the memory as FAILED, never as absent', async () => { const { client } = supabaseWithCorpus(CORPUS, { deleteError: { message: 'boom' } }); const result = await forgetMemoriesMatching(client, 'u1', ['photography']); expect(result.deleted).toEqual([]); - expect(result.notFound).toEqual(['photography']); + expect(result.notFound).not.toContain('photography'); + expect(result.failed.length).toBeGreaterThan(0); }); it('ignores degenerate fragments that would match everything', async () => { diff --git a/__tests__/unit/cat/tool-executor-forget.test.ts b/__tests__/unit/cat/tool-executor-forget.test.ts index fa00a853e..b3cef16ff 100644 --- a/__tests__/unit/cat/tool-executor-forget.test.ts +++ b/__tests__/unit/cat/tool-executor-forget.test.ts @@ -43,10 +43,12 @@ describe('executeToolCall forget_memories', () => { mockForgetMemories.mockResolvedValue({ deleted: ['Does photography'], notFound: ['speaks French'], + failed: [], }); mockRemoveProfile.mockResolvedValue({ removed: ['skill: photography'], notFound: ['speaks French'], + failed: [], }); const events: Array> = []; @@ -80,8 +82,8 @@ describe('executeToolCall forget_memories', () => { }); it('counts a profile-only removal as a success, not no_results', async () => { - mockForgetMemories.mockResolvedValue({ deleted: [], notFound: ['welding'] }); - mockRemoveProfile.mockResolvedValue({ removed: ['skill: welding'], notFound: [] }); + mockForgetMemories.mockResolvedValue({ deleted: [], notFound: ['welding'], failed: [] }); + mockRemoveProfile.mockResolvedValue({ removed: ['skill: welding'], notFound: [], failed: [] }); const events: Array> = []; await executeToolCall(supabase, USER_ID, forgetCall(['welding']), 'forget welding', e => @@ -93,8 +95,8 @@ describe('executeToolCall forget_memories', () => { }); it('reports no_results only when BOTH stores found nothing', async () => { - mockForgetMemories.mockResolvedValue({ deleted: [], notFound: ['x'] }); - mockRemoveProfile.mockResolvedValue({ removed: [], notFound: ['x'] }); + mockForgetMemories.mockResolvedValue({ deleted: [], notFound: ['x'], failed: [] }); + mockRemoveProfile.mockResolvedValue({ removed: [], notFound: ['x'], failed: [] }); const events: Array> = []; await executeToolCall(supabase, USER_ID, forgetCall(['x']), 'forget x', e => diff --git a/src/services/cat/economic-profile.ts b/src/services/cat/economic-profile.ts index b8d56a48a..7247b7e96 100644 --- a/src/services/cat/economic-profile.ts +++ b/src/services/cat/economic-profile.ts @@ -268,6 +268,15 @@ export interface ProfileRemovalResult { removed: string[]; /** Terms that matched nothing in the profile. */ notFound: string[]; + /** + * Terms we could not answer for, because the write did not land. + * + * Same distinction as ForgetResult.failed: "your profile has no such entry" + * and "we could not save the removal" used to be one value, so a failed + * upsert reported notFound and the entry stayed in the profile while the user + * was told it was gone. bitbaum/orangecat#563 finding 8. + */ + failed: string[]; } function entryText(it: unknown): string { @@ -302,7 +311,7 @@ export async function removeFromEconomicProfile( terms: string[] ): Promise { const wanted = terms.map(t => t.trim()).filter(t => t.length >= 4); - const result: ProfileRemovalResult = { removed: [], notFound: [] }; + const result: ProfileRemovalResult = { removed: [], notFound: [], failed: [] }; if (wanted.length === 0) { return result; } @@ -361,12 +370,14 @@ export async function removeFromEconomicProfile( { onConflict: 'user_id' } ); if (error) { + // The entries matched; saving the profile without them is what failed, so + // they are still there. Reporting notFound would invert the truth. logger.warn('removeFromEconomicProfile upsert failed', { error }, 'EconomicProfile'); - return { removed: [], notFound: wanted }; + return { removed: [], notFound: result.notFound, failed: [...matchedTerms] }; } } catch (err) { logger.warn('removeFromEconomicProfile failed', { err: String(err) }, 'EconomicProfile'); - return { removed: [], notFound: wanted }; + return { removed: [], notFound: result.notFound, failed: [...matchedTerms] }; } return result; } diff --git a/src/services/cat/handlers/context.ts b/src/services/cat/handlers/context.ts index 63aec906f..b76d001d4 100644 --- a/src/services/cat/handlers/context.ts +++ b/src/services/cat/handlers/context.ts @@ -62,6 +62,24 @@ export const contextHandlers: Record = { const stillUnknown = facts.filter( f => mem.notFound.includes(f) && profile.notFound.includes(f) ); + + // A store that could not be reached is NOT a store with nothing in it. + // Checked before the no-match branch, because that branch's wording — "no + // stored memory matched, nothing was removed" — is a factual claim about + // the user's data that we are in no position to make when the query or the + // delete failed. Telling someone a memory is gone while it is still there + // is the worst outcome this feature has. + const failed = [...mem.failed, ...profile.failed]; + if (failed.length > 0) { + return { + success: false, + error: + 'Could not reach your memories just now, so nothing was removed — ' + + 'please try again in a moment. Nothing has been deleted, and you can ' + + 'check what is stored at Settings → AI → What Cat remembers.', + }; + } + if (removedCount === 0) { return { success: false, diff --git a/src/services/cat/memory.ts b/src/services/cat/memory.ts index c808fcec0..003699933 100644 --- a/src/services/cat/memory.ts +++ b/src/services/cat/memory.ts @@ -98,6 +98,17 @@ export interface ForgetResult { deleted: string[]; /** Requested facts for which no stored memory matched. */ notFound: string[]; + /** + * Facts we could not answer for, because the database did not respond. + * + * Distinct from `notFound`, and the distinction is the whole point: "we + * looked and you have no such memory" and "we could not look" used to be the + * same value. A failed delete reported `notFound`, so Cat told the user + * nothing matched — while the memory they had just disowned was still there. + * Telling someone their data is gone when it is not is the worst thing this + * feature can do. bitbaum/orangecat#563 finding 8. + */ + failed: string[]; } /** @@ -214,7 +225,7 @@ export async function forgetMemoriesMatching( .map(f => f.trim()) .filter(f => f.length >= MIN_FORGET_FRAGMENT_CHARS) .slice(0, MAX_FORGET_FACTS); - const result: ForgetResult = { deleted: [], notFound: [] }; + const result: ForgetResult = { deleted: [], notFound: [], failed: [] }; if (wanted.length === 0) { return result; } @@ -224,8 +235,10 @@ export async function forgetMemoriesMatching( .select('id, content') .eq('user_id', userId); if (loadError) { + // Could not read the corpus, so we cannot say anything about what matched. + // Reporting notFound here would tell the user their memories are absent. logger.warn('forgetMemoriesMatching load failed', { error: loadError }, 'CatMemory'); - return { deleted: [], notFound: wanted }; + return { deleted: [], notFound: [], failed: wanted }; } const corpus = (rows ?? []) as Array<{ id: string; content: string }>; @@ -283,8 +296,10 @@ export async function forgetMemoriesMatching( .eq('user_id', userId) .in('id', [...doomed.keys()]); if (error) { + // We DID match these — the delete is what failed, so the memories are + // still there. Saying notFound would be the opposite of the truth. logger.warn('forgetMemoriesMatching delete failed', { error }, 'CatMemory'); - return { deleted: [], notFound: wanted }; + return { deleted: [], notFound: result.notFound, failed: [...doomed.values()] }; } result.deleted.push(...doomed.values()); // Remember WHAT was forgotten (suppression list) so passive extraction diff --git a/src/services/cat/tool-executor.ts b/src/services/cat/tool-executor.ts index fbf79f2da..2975af7f9 100644 --- a/src/services/cat/tool-executor.ts +++ b/src/services/cat/tool-executor.ts @@ -257,13 +257,20 @@ Explain this to the user in plain language: which provider is healthy, degraded, deleted: mem.deleted, removedProfileEntries: profile.removed, notFound: facts.filter(f => mem.notFound.includes(f) && profile.notFound.includes(f)), + // A store we could not reach is not a store with nothing in it. Without + // this the model was handed the same shape for both and told the user + // "no matching memory was found" while the memory was still there. + failedToRemove: [...mem.failed, ...profile.failed], }; content = JSON.stringify(outcome) + '\n\nReport ONLY what "deleted" and "removedProfileEntries" confirm was removed, ' + 'quoting the deleted items. For anything in "notFound", say plainly that no matching ' + 'stored memory or profile entry was found and that the full list is at ' + - 'Settings → AI → What Cat remembers. Never claim other changes.'; + 'Settings → AI → What Cat remembers. ' + + 'For anything in "failedToRemove", say the removal did NOT happen and could not be ' + + 'completed right now, and that they should try again — never describe it as absent ' + + 'or as removed. Never claim other changes.'; const removedCount = outcome.deleted.length + outcome.removedProfileEntries.length; if (removedCount > 0) { onToolCall?.({