Merge pull request #721 from mhenke/fix/720-session-status-fallback

fix(foreground-fallback): check props.error in session.status handler

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

@@ -126,8 +126,14 @@ describe('isRateLimitError', () => {
     expect(isRateLimitError(null)).toBe(false);
   });
 
+  test('returns true for string error with rate-limit message', () => {
+    expect(isRateLimitError('Usage exceeded')).toBe(true);
+    expect(isRateLimitError('rate limit exceeded')).toBe(true);
+    expect(isRateLimitError('quota exceeded')).toBe(true);
+  });
+
   test('returns false for non-object', () => {
-    expect(isRateLimitError('string error')).toBe(false);
+    expect(isRateLimitError(42)).toBe(false);
   });
 });
 
@@ -449,11 +455,10 @@ describe('ForegroundFallbackManager session.status', () => {
     expect(mocks.promptAsync).not.toHaveBeenCalled();
   });
 
-  test('tracks retries and only intervenes after maxRetries', async () => {
+  test('triggers fallback on first session.status retry (uses shouldIntervene)', async () => {
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
 
-    // Pre-seed model
     await mgr.handleEvent({
       type: 'message.updated',
       properties: {
@@ -465,11 +470,41 @@ describe('ForegroundFallbackManager session.status', () => {
       },
     });
 
-    // First two retries should be absorbed (maxRetries - 1 = 2)
+    // First retry triggers fallback immediately — no budget absorption
     await mgr.handleEvent({
       type: 'session.status',
       properties: {
         sessionID: 'sess-retry',
+        status: {
+          type: 'retry',
+          attempt: 1,
+          message: 'Free usage exceeded, subscribe to Go',
+        },
+      },
+    });
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+  });
+
+  test('second session.status retry goes through budget after first triggered fallback', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-retry2',
+          providerID: 'anthropic',
+          modelID: 'claude-opus-4-5',
+        },
+      },
+    });
+
+    // First retry triggers immediately
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-retry2',
         status: {
           type: 'retry',
           attempt: 1,
@@ -477,10 +512,13 @@ describe('ForegroundFallbackManager session.status', () => {
         },
       },
     });
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+
+    // Second retry absorbed by budget (tried > 0 → checkRetryBudget)
     await mgr.handleEvent({
       type: 'session.status',
       properties: {
-        sessionID: 'sess-retry',
+        sessionID: 'sess-retry2',
         status: {
           type: 'retry',
           attempt: 2,
@@ -488,13 +526,10 @@ describe('ForegroundFallbackManager session.status', () => {
         },
       },
     });
-    expect(mocks.promptAsync).not.toHaveBeenCalled();
-
-    // Third retry exhausts the budget → tryFallback intervenes
     await mgr.handleEvent({
       type: 'session.status',
       properties: {
-        sessionID: 'sess-retry',
+        sessionID: 'sess-retry2',
         status: {
           type: 'retry',
           attempt: 3,
@@ -502,7 +537,131 @@ describe('ForegroundFallbackManager session.status', () => {
         },
       },
     });
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
+  });
+
+  test('triggers fallback when rate-limit text is in props.error instead of status.message', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-error-field',
+          providerID: 'anthropic',
+          modelID: 'claude-opus-4-5',
+        },
+      },
+    });
+
+    // status.message is benign but props.error carries the rate-limit signal
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-error-field',
+        status: { type: 'retry', attempt: 1, message: 'retrying...' },
+        error: { message: 'Usage exceeded for this billing period' },
+      },
+    });
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+  });
+
+  test('triggers fallback when props.error is a plain string', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-str-error',
+          providerID: 'anthropic',
+          modelID: 'claude-opus-4-5',
+        },
+      },
+    });
+
+    // props.error is a plain string — no object wrapper
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-str-error',
+        status: { type: 'retry', attempt: 1, message: 'retrying...' },
+        error: 'Usage exceeded for this billing period',
+      },
+    });
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+  });
+
+  test('non-rate-limit status does not clear retries (no infinite loop from abort side effects)', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-nonrl',
+          providerID: 'anthropic',
+          modelID: 'claude-opus-4-5',
+        },
+      },
+    });
+
+    // First rate-limit: triggers fallback, sessionRetries set to 1
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-nonrl',
+        status: {
+          type: 'retry',
+          attempt: 1,
+          message: 'rate limit, retrying...',
+        },
+      },
+    });
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+
+    // Non-rate-limit status (e.g. abort side effect): must NOT reset retries.
+    // If it did, the next rate-limit would see tried=0 and trigger immediate
+    // fallback again — the infinite loop.
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-nonrl',
+        status: { type: 'retry', attempt: 1, message: 'aborted' },
+      },
+    });
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+
+    // Second rate-limit: absorbed by budget (tried=1, not 0 from reset)
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-nonrl',
+        status: {
+          type: 'retry',
+          attempt: 2,
+          message: 'rate limit, retrying...',
+        },
+      },
+    });
     expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+
+    // Third rate-limit: budget exhausted at maxRetries-1=2, re-triggers
+    await mgr.handleEvent({
+      type: 'session.status',
+      properties: {
+        sessionID: 'sess-nonrl',
+        status: {
+          type: 'retry',
+          attempt: 3,
+          message: 'rate limit, retrying...',
+        },
+      },
+    });
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(2);
   });
 });
 
@@ -899,11 +1058,10 @@ describe('ForegroundFallbackManager resolveChain cross-agent isolation', () => {
     expect(call[0].body.model.modelID).toBe('gpt-4o');
   });
 
-  test('falls through to model matching for non-omos agents (e.g. compaction)', async () => {
-    // compaction is an OpenCode built-in agent that is NOT an omos agent,
-    // so it has no chain configured. It should fall through to model
-    // matching and inherit a chain from a configured agent that shares
-    // its model, instead of being silently excluded from fallback.
+  test('does NOT bleed into other agent chains for non-omos agents without a chain', async () => {
+    // A user-defined agent (e.g. Build) shares its model with the orchestrator
+    // chain but has no chain of its own. It must NOT inherit the orchestrator
+    // chain — that would switch the session from Build to Orchestrator.
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(
       client,
@@ -915,8 +1073,8 @@ describe('ForegroundFallbackManager resolveChain cross-agent isolation', () => {
       type: 'message.updated',
       properties: {
         info: {
-          sessionID: 'compaction-sess',
-          agent: 'compaction', // NOT a known omos built-in agent
+          sessionID: 'build-sess',
+          agent: 'build',
           providerID: 'openai',
           modelID: 'gpt-5.6',
           error: { message: 'rate limit exceeded' },
@@ -924,14 +1082,8 @@ describe('ForegroundFallbackManager resolveChain cross-agent isolation', () => {
       },
     });
 
-    // compaction's model (openai/gpt-5.6) matches orchestrator's chain
-    // → should fall back to the next untried model in that chain
-    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
-    const call = mocks.promptAsync.mock.calls[0] as [
-      { body: { model: { providerID: string; modelID: string } } },
-    ];
-    expect(call[0].body.model.providerID).toBe('new-api');
-    expect(call[0].body.model.modelID).toBe('glm-5.2');
+    // build has no configured chain and must not inherit orchestrator's
+    expect(mocks.promptAsync).not.toHaveBeenCalled();
   });
 });
 
@@ -1086,14 +1238,14 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       false, // runtimeOverride
     );
 
-    // Simulate unknown agent (e.g. "compaction") using a model that IS in
-    // the orchestrator chain — resolveChain infers the chain from the model.
+    // Simulate an agent name OpenCode doesn't report (agent field absent).
+    // resolveChain falls through to model-matching, finds the chain that
+    // contains openai/gpt-4o (orchestrator's chain).
     await mgr.handleEvent({
       type: 'message.updated',
       properties: {
         info: {
           sessionID: 'sess-5',
-          agent: 'compaction',
           providerID: 'openai',
           modelID: 'gpt-4o',
           error: { message: 'rate limit exceeded' },
@@ -1142,7 +1294,7 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
     expect(mocks.abort).toHaveBeenCalledTimes(1);
   });
 
-  test('session.status with runtimeOverride=false and out-of-chain model aborts after retry budget exhausted', async () => {
+  test('session.status with runtimeOverride=false and out-of-chain model triggers immediate fallback which aborts', async () => {
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(
       client,
@@ -1166,27 +1318,7 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       },
     });
 
-    // First retry (attempt 1) — absorbed by retry budget
-    await mgr.handleEvent({
-      type: 'session.status',
-      properties: {
-        sessionID: 'sess-status-override',
-        status: { type: 'retry', message: 'rate limit, retrying...' },
-      },
-    });
-    expect(mocks.abort).toHaveBeenCalledTimes(0);
-
-    // Second retry (attempt 2) — absorbed
-    await mgr.handleEvent({
-      type: 'session.status',
-      properties: {
-        sessionID: 'sess-status-override',
-        status: { type: 'retry', message: 'rate limit, retrying...' },
-      },
-    });
-    expect(mocks.abort).toHaveBeenCalledTimes(0);
-
-    // Third retry (attempt 3) — budget exhausted, tryFallback runs, guard aborts
+    // First retry triggers shouldIntervene → immediate fallback → abort
     await mgr.handleEvent({
       type: 'session.status',
       properties: {
@@ -1195,6 +1327,6 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       },
     });
     expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
-    expect(mocks.abort).toHaveBeenCalledTimes(1);
+    expect(mocks.abort).toHaveBeenCalled();
   });
 });

src/hooks/foreground-fallback/index.ts

@@ -15,7 +15,6 @@
  */
 
 import type { PluginInput } from '@opencode-ai/plugin';
-import { ALL_AGENT_NAMES } from '../../config/constants';
 import { log } from '../../utils/logger';
 import {
   abortSessionWithTimeout,
@@ -53,7 +52,12 @@ const RATE_LIMIT_PATTERNS = [
 ];
 
 export function isRateLimitError(error: unknown): boolean {
-  if (!error || typeof error !== 'object') return false;
+  if (!error) return false;
+  // Handle string-typed errors (OpenCode may send a plain error string)
+  if (typeof error === 'string') {
+    return RATE_LIMIT_PATTERNS.some((p) => p.test(error));
+  }
+  if (typeof error !== 'object') return false;
   const err = error as {
     message?: string;
     data?: { statusCode?: number; message?: string; responseBody?: string };
@@ -206,30 +210,36 @@ export class ForegroundFallbackManager {
           | {
               sessionID?: string;
               status?: { type?: string; message?: string; attempt?: number };
+              error?: unknown;
             }
           | undefined;
-        if (!props?.sessionID || !props.status?.message) break;
-        const msg = props.status.message.toLowerCase();
-        if (
-          msg.includes('rate limit') ||
-          msg.includes('usage limit') ||
-          msg.includes('usage exceeded') ||
-          msg.includes('quota exceeded') ||
-          msg.includes('exceededbudget') ||
-          msg.includes('over budget') ||
-          msg.includes('insufficient') ||
-          msg.includes('high concurrency') ||
-          msg.includes('reduce concurrency')
-        ) {
-          // session.status retry path always counts toward the budget
-          // — even the first retry is absorbed before intervening.
-          if (this.checkRetryBudget(props.sessionID)) {
-            await this.tryFallback(props.sessionID);
+        if (!props?.sessionID) break;
+        const msg = props.status?.message?.toLowerCase() ?? '';
+        const isRateLimit =
+          (msg &&
+            (msg.includes('rate limit') ||
+              msg.includes('usage limit') ||
+              msg.includes('usage exceeded') ||
+              msg.includes('quota exceeded') ||
+              msg.includes('exceededbudget') ||
+              msg.includes('over budget') ||
+              msg.includes('insufficient') ||
+              msg.includes('high concurrency') ||
+              msg.includes('reduce concurrency'))) ||
+          isRateLimitError(props.error);
+        if (isRateLimit) {
+          if (this.shouldIntervene(props.sessionID)) {
+            await this.tryFallbackWithAbort(props.sessionID);
+            this.sessionRetries.set(props.sessionID, 1);
           }
-        } else {
-          // Non-rate-limit status: clear retry count (recovery).
-          this.sessionRetries.delete(props.sessionID);
         }
+        // Note: do NOT clear sessionRetries here on non-rate-limit statuses.
+        // Abort events triggered by our own fallback carry non-rate-limit
+        // messages and would reset the counter, creating an infinite loop:
+        // abort → fallback → set retries to 1 → abort event clears retries
+        // → next retry sees tried=0 → abort+fallback again → repeat.
+        // Retries are only cleared on successful response (message.updated
+        // without error) or session deletion.
         break;
       }
 
@@ -302,6 +312,38 @@ export class ForegroundFallbackManager {
     // Deduplicate: multiple events can fire for a single rate-limit event.
     // Bypass dedup when the model changed since the last trigger - the new
     // model's failure is a separate incident and the cascade should continue.
+    if (this.isDeduped(sessionID)) return;
+
+    this.inProgress.add(sessionID);
+    try {
+      await this.execFallback(sessionID);
+    } finally {
+      this.inProgress.delete(sessionID);
+    }
+  }
+
+  /**
+   * Fallback path for session.status retry events.  Aborts the retry loop
+   * before falling back because promptAsync alone is ignored while the
+   * session is in retry mode.  inProgress is set first so the
+   * task-session-manager sees isFallbackInProgress()=true during the
+   * abort idle window and does not cancel the pending task call.
+   */
+  private async tryFallbackWithAbort(sessionID: string): Promise<void> {
+    if (!sessionID) return;
+    if (this.inProgress.has(sessionID)) return;
+    if (this.isDeduped(sessionID)) return;
+
+    this.inProgress.add(sessionID);
+    try {
+      await abortSessionWithTimeout(this.client, sessionID);
+      await this.execFallback(sessionID);
+    } finally {
+      this.inProgress.delete(sessionID);
+    }
+  }
+
+  private isDeduped(sessionID: string): boolean {
     const now = Date.now();
     const curModel = this.sessionModel.get(sessionID);
     const modelChanged =
@@ -311,13 +353,15 @@ export class ForegroundFallbackManager {
       !modelChanged &&
       now - (this.lastTrigger.get(sessionID) ?? 0) < DEDUP_WINDOW_MS
     )
-      return;
+      return true;
     this.lastTrigger.set(sessionID, now);
     if (curModel !== undefined) {
       this.lastTriggerModel.set(sessionID, curModel);
     }
+    return false;
+  }
 
-    this.inProgress.add(sessionID);
+  private async execFallback(sessionID: string): Promise<void> {
     try {
       let currentModel = this.sessionModel.get(sessionID);
       const agentName = this.sessionAgent.get(sessionID);
@@ -481,8 +525,6 @@ export class ForegroundFallbackManager {
         sessionID,
         error: err instanceof Error ? err.message : String(err),
       });
-    } finally {
-      this.inProgress.delete(sessionID);
     }
   }
 
@@ -495,9 +537,8 @@ export class ForegroundFallbackManager {
    *
    * Priority:
    * 1. Agent name known AND has a configured chain → return it directly
-   * 2. Agent name known but NO chain configured → return [] (no fallback;
-   *    do NOT bleed into other agents' chains which would re-prompt the
-   *    session with a model belonging to a completely different agent)
+   * 2. Agent name known but NO chain → return [] (no fallback; never
+   *    bleed into other agents' chains)
    * 3. Agent name unknown, current model known → search all chains for
    *    the model to infer which chain to use
    * 4. Nothing matches → flatten all chains as a last resort (only
@@ -508,18 +549,13 @@ export class ForegroundFallbackManager {
     currentModel: string | undefined,
   ): string[] {
     if (agentName) {
-      // Agent is known: use its chain exactly if configured.
       const chain = this.chains[agentName];
       if (chain) return chain;
-      // Known omos built-in agent (oracle, librarian, …) without a
-      // configured chain: keep isolation - do NOT bleed into other
-      // agents' chains (preserves the cross-agent isolation contract
-      // from PR #199).
-      if ((ALL_AGENT_NAMES as readonly string[]).includes(agentName)) return [];
-      // Unknown agent (e.g. OpenCode built-in "compaction" or "title"
-      // that don't appear in the user preset): fall through to
-      // model-matching so they can inherit a chain from a configured
-      // agent that shares their model.
+      // Any known agent without a configured chain: no fallback.
+      // Don't bleed into other agents' chains via model-matching —
+      // that switches the session to the wrong agent (e.g. Build
+      // inherits Orchestrator's chain and becomes Orchestrator).
+      return [];
     }
 
     // Agent unknown: try to infer from the current model.