From 57693bb134ccd629cae861ce6e93907287d04ff0 Mon Sep 17 00:00:00 2001 From: Georgy Butaev <41178744+g-but@users.noreply.github.com> Date: Sat, 29 Aug 2026 07:07:35 +0200 Subject: [PATCH] fix(cat): "we could not look" was reported as "you have no such memory" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both forget stores swallowed database errors into `{ deleted: [], notFound: wanted }` — the exact shape they return when they looked properly and found nothing. So a user who asked Cat to forget something was told it was not there, in two different situations where it was: * the SELECT failed, so we never read the corpus at all; * the DELETE failed, so we HAD matched their memory and left it in place. The second is the bad one. Cat found the memory, could not remove it, and replied that nothing matched — while the fact the user had just disowned was still stored. Telling someone their data is gone when it is not is the worst thing a forget feature can do, and it did it silently. ForgetResult and ProfileRemovalResult now carry a `failed` channel, separate from `notFound`, at all three swallow sites (memory load, memory delete, profile upsert). Both consumers report it honestly: the exec_action handler returns success:false with "could not reach your memories … nothing has been deleted", checked BEFORE the no-match branch, because that branch's wording is a factual claim about the user's data we are in no position to make. The LLM tool path gets a `failedToRemove` list and an instruction never to describe those as absent or removed. One existing test asserted the old behaviour verbatim — `expect(result.notFound) .toEqual(['photography'])` on a failed delete. It pinned the bug, so it is corrected rather than worked around, and says why. Three tool-executor fixtures predated the new field and are extended. bitbaum/orangecat#563 finding 8. Mutation-proven: reverting either swallow site fails exactly one of the four new tests. 72 Cat suites (1025 tests) and type-check green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018waGt1ieA9TjpscqrbrnGb --- .../forget-failure-is-not-no-match.test.ts | 105 ++++++++++++++++++ __tests__/unit/cat/memory-forget.test.ts | 9 +- .../unit/cat/tool-executor-forget.test.ts | 10 +- src/services/cat/economic-profile.ts | 17 ++- src/services/cat/handlers/context.ts | 18 +++ src/services/cat/memory.ts | 21 +++- src/services/cat/tool-executor.ts | 9 +- 7 files changed, 176 insertions(+), 13 deletions(-) create mode 100644 __tests__/unit/cat/forget-failure-is-not-no-match.test.ts 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?.({