fix(cat): "we could not look" was reported as "you have no such memory"

__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([]);
+  });
+});

__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 () => {

__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<Record<string, unknown>> = [];
@@ -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<Record<string, unknown>> = [];
     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<Record<string, unknown>> = [];
     await executeToolCall(supabase, USER_ID, forgetCall(['x']), 'forget x', e =>

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<ProfileRemovalResult> {
   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;
 }

src/services/cat/handlers/context.ts

@@ -62,6 +62,24 @@ export const contextHandlers: Record<string, ActionHandler> = {
     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,

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

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?.({