Browse Source

fix(foreground-fallback): tighten lastTriggerModel type and fix test comments

- Map<string, string | undefined> → Map<string, string> with .has() guard
- Only store actual model strings, never undefined
- Fix test comments that incorrectly described dedup bypass as 'lastTrigger
  reset' when the real mechanism is modelChanged check
Michael Henke 1 month ago
parent
commit
8029dfbf48

+ 3 - 4
src/hooks/foreground-fallback/index.test.ts

@@ -609,8 +609,8 @@ describe('ForegroundFallbackManager deduplication', () => {
     );
 
     // Second error — model B also fails within the 5s dedup window.
-    // This is a DIFFERENT incident (new model), so it should NOT be deduped
-    // after the successful model switch cleared the lastTrigger timer.
+    // This is a DIFFERENT incident (new model), so dedup is bypassed
+    // because the current model differs from lastTriggerModel.
     await mgr.handleEvent({
       type: 'session.error',
       properties: {
@@ -620,8 +620,7 @@ describe('ForegroundFallbackManager deduplication', () => {
     });
 
     // Should trigger a second fallback despite being within the original
-    // 5-second dedup window, because the lastTrigger was reset after the
-    // successful model switch.
+    // 5-second dedup window, because the model changed (modelChanged bypass).
     expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
     expect(mocks.promptAsync.mock.calls[1][0]).toEqual(
       expect.objectContaining({

+ 7 - 4
src/hooks/foreground-fallback/index.ts

@@ -98,7 +98,7 @@ export class ForegroundFallbackManager {
   /** sessionID → model in use when lastTrigger was set; dedup is bypassed
    *  when the model has changed, allowing the cascade to continue when a
    *  new fallback model also fails within the dedup window. */
-  private readonly lastTriggerModel = new Map<string, string | undefined>();
+  private readonly lastTriggerModel = new Map<string, string>();
 
   constructor(
     private readonly client: OpencodeClient,
@@ -235,16 +235,19 @@ export class ForegroundFallbackManager {
     // Bypass dedup when the model changed since the last trigger — the new
     // model's failure is a separate incident and the cascade should continue.
     const now = Date.now();
-    const lastModel = this.lastTriggerModel.get(sessionID);
     const curModel = this.sessionModel.get(sessionID);
-    const modelChanged = lastModel !== undefined && lastModel !== curModel;
+    const modelChanged =
+      this.lastTriggerModel.has(sessionID) &&
+      this.lastTriggerModel.get(sessionID) !== curModel;
     if (
       !modelChanged &&
       now - (this.lastTrigger.get(sessionID) ?? 0) < DEDUP_WINDOW_MS
     )
       return;
     this.lastTrigger.set(sessionID, now);
-    this.lastTriggerModel.set(sessionID, curModel);
+    if (curModel !== undefined) {
+      this.lastTriggerModel.set(sessionID, curModel);
+    }
 
     this.inProgress.add(sessionID);
     try {