Browse Source

fix(foreground-fallback): make dedup model-aware so cascade continues

The dedup window blocked a second fallback when the model had already
switched. Track the model that triggered the last fallback and bypass
dedup when the current model differs from the recorded one, so a fresh
error on the new model still cascades.

Also removes the diagnostic scaffolding from 1641770:
- extractErrorPreview helper (dead code, duplicated existing extractors)
- per-event diagnostic block in handleEvent (unconditional noise)
- tryFallback entry log (fired on every dedup-skip)

Cascade test rewritten with objectContaining to drop the tuple-cast
ceremony.
Michael Henke 1 month ago
parent
commit
0bf4d7f819
2 changed files with 14 additions and 75 deletions
  1. 14 14
      src/hooks/foreground-fallback/index.test.ts
  2. 0 61
      src/hooks/foreground-fallback/index.ts

+ 14 - 14
src/hooks/foreground-fallback/index.test.ts

@@ -600,13 +600,13 @@ describe('ForegroundFallbackManager deduplication', () => {
     });
 
     expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
-    const call1 = mocks.promptAsync.mock.calls[0] as [
-      {
-        body: { model: { providerID: string; modelID: string } };
-      },
-    ];
-    expect(call1[0].body.model.providerID).toBe('openai');
-    expect(call1[0].body.model.modelID).toBe('gpt-4o');
+    expect(mocks.promptAsync.mock.calls[0][0]).toEqual(
+      expect.objectContaining({
+        body: expect.objectContaining({
+          model: { providerID: 'openai', modelID: 'gpt-4o' },
+        }),
+      }),
+    );
 
     // Second error — model B also fails within the 5s dedup window.
     // This is a DIFFERENT incident (new model), so it should NOT be deduped
@@ -623,13 +623,13 @@ describe('ForegroundFallbackManager deduplication', () => {
     // 5-second dedup window, because the lastTrigger was reset after the
     // successful model switch.
     expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
-    const call2 = mocks.promptAsync.mock.calls[1] as [
-      {
-        body: { model: { providerID: string; modelID: string } };
-      },
-    ];
-    expect(call2[0].body.model.providerID).toBe('google');
-    expect(call2[0].body.model.modelID).toBe('gemini-2.5-pro');
+    expect(mocks.promptAsync.mock.calls[1][0]).toEqual(
+      expect.objectContaining({
+        body: expect.objectContaining({
+          model: { providerID: 'google', modelID: 'gemini-2.5-pro' },
+        }),
+      }),
+    );
   });
 });
 

+ 0 - 61
src/hooks/foreground-fallback/index.ts

@@ -74,46 +74,6 @@ export function isRateLimitError(error: unknown): boolean {
 const DEDUP_WINDOW_MS = 5_000;
 const REPROMPT_DELAY_MS = 500;
 
-/**
- * Extract a preview of the error field from event properties for diagnostic logging.
- * Handles the different event shapes: session.error, message.updated, session.status.
- */
-function extractErrorPreview(properties: unknown): string | undefined {
-  const p = properties as Record<string, unknown> | undefined;
-  if (!p) return undefined;
-
-  // session.error: { error: ApiError|... }
-  const err = p.error as Record<string, unknown> | undefined;
-  if (err) {
-    const msg =
-      (err.message as string) ??
-      ((err.data as Record<string, unknown> | undefined)?.message as
-        | string
-        | undefined);
-    return msg ?? `[${err.name ?? 'unknown error type'}]`;
-  }
-
-  // message.updated: { info: { error: ... } }
-  const info = p.info as Record<string, unknown> | undefined;
-  if (info?.error) {
-    const infoErr = info.error as Record<string, unknown>;
-    const msg =
-      (infoErr.message as string) ??
-      ((infoErr.data as Record<string, unknown> | undefined)?.message as
-        | string
-        | undefined);
-    return msg ?? `[${infoErr.name ?? 'unknown error type'}]`;
-  }
-
-  // session.status: { status: { message } }
-  const status = p.status as Record<string, unknown> | undefined;
-  if (status?.message) {
-    return status.message as string;
-  }
-
-  return undefined;
-}
-
 // ---------------------------------------------------------------------------
 // Manager
 // ---------------------------------------------------------------------------
@@ -160,22 +120,6 @@ export class ForegroundFallbackManager {
     const event = rawEvent as { type: string; properties?: unknown };
     if (!event?.type) return;
 
-    // Diagnostic: log every event reaching the fallback manager
-    {
-      const p = event.properties as Record<string, unknown> | undefined;
-      const sid =
-        (p?.sessionID as string | undefined) ??
-        ((p?.info as Record<string, unknown> | undefined)?.sessionID as
-          | string
-          | undefined);
-      const hasError = extractErrorPreview(event.properties);
-      log('[foreground-fallback] event', {
-        type: event.type,
-        sessionID: sid,
-        error: hasError,
-      });
-    }
-
     switch (event.type) {
       case 'message.updated': {
         const info = (
@@ -284,11 +228,6 @@ export class ForegroundFallbackManager {
   // ---------------------------------------------------------------------------
 
   private async tryFallback(sessionID: string): Promise<void> {
-    log('[foreground-fallback] tryFallback', {
-      sessionID,
-      inProgress: this.inProgress.has(sessionID),
-      dedupMs: Date.now() - (this.lastTrigger.get(sessionID) ?? 0),
-    });
     if (!sessionID) return;
     if (this.inProgress.has(sessionID)) return;