Browse Source

fix: guard runtime model from silent overwrite on subagent dispatch

Three code paths unconditionally overwrite agent.config.model, clobbering
the user's runtime :model pick on every subagent dispatch:

1. applyOverrides (agents/index.ts:166): commit 9100e59 added
   agent.config.model = agent._modelArray[0].id for ALL agents including
   the orchestrator. This bypasses PR #639's guard because it runs BEFORE
   the config() hook. Fix: exclude orchestrator (set undefined so the
   config() hook's precedence-aware guard is the sole source of truth).

2. ForegroundFallbackManager: when a rate-limit error fires, the fallback
   manager switches to chain[0] even for models deliberately chosen outside
   the chain. Fix: add fallback.runtimeOverride config option (default true
   for backward compat). When false, out-of-chain models are respected and
   the error surfaces instead of silently swapping.

3. Schema: add runtimeOverride field to FailoverConfigSchema.

Closes #691 (paths 1 and 3). Path 2 (preset-override block) is a
separate PR.
dragon-Elec 1 month ago
parent
commit
bd702ce442

+ 5 - 1
src/agents/index.test.ts

@@ -236,7 +236,11 @@ describe('orchestrator agent', () => {
       { id: 'github-copilot/claude-3.5-haiku' },
       { id: 'openai/gpt-4' },
     ]);
-    expect(orchestrator?.config.model).toBe('google/gemini-3-pro');
+    // orchestrator is the long-lived foreground agent: config.model must
+    // stay undefined so a user's runtime /model selection (tracked via
+    // opencodeConfig.agent.orchestrator.model) is never overwritten by
+    // the config's static array default. See src/agents/index.ts:166.
+    expect(orchestrator?.config.model).toBeUndefined();
   });
 });
 

+ 18 - 4
src/agents/index.ts

@@ -160,10 +160,24 @@ function applyOverrides(
       agent._modelArray = override.model.map((m) =>
         typeof m === 'string' ? { id: m } : m,
       );
-      // Set config.model to the primary entry so the subagent has a valid
-      // model at launch time. ForegroundFallbackManager handles runtime
-      // failover to the remaining entries in _modelArray.
-      agent.config.model = agent._modelArray[0].id;
+      // Subagents are ephemeral, freshly-created sessions with no prior
+      // runtime state to preserve, so giving them a concrete config.model
+      // at launch time (the array's primary entry) is safe — see #9100e59.
+      // ForegroundFallbackManager handles runtime failover to the
+      // remaining entries in _modelArray.
+      //
+      // The orchestrator is different: it's a long-lived, foreground
+      // session where a user's runtime `/model` selection must survive
+      // across plugin re-inits (triggered by client.config.update() ->
+      // Instance.dispose(), e.g. on every subagent dispatch). Setting
+      // config.model here unconditionally would stomp that live
+      // selection every time this function re-runs, because it runs
+      // BEFORE the config() hook's merge with the live
+      // opencodeConfig.agent.orchestrator.model (see src/index.ts:524-528,
+      // added by #639). Leaving it undefined for the orchestrator lets
+      // that later, precedence-aware guard be the sole source of truth.
+      agent.config.model =
+        agent.name === 'orchestrator' ? undefined : agent._modelArray[0].id;
     } else {
       agent.config.model = override.model;
     }

+ 11 - 0
src/config/schema.ts

@@ -197,6 +197,17 @@ export const FailoverConfigSchema = z
         'When true (default), empty provider responses are treated as failures, ' +
           'triggering fallback/retry. Set to false to treat them as successes.',
       ),
+    runtimeOverride: z
+      .boolean()
+      .default(true)
+      .describe(
+        'When true (default), a runtime model selected via /model that is ' +
+          'outside the configured fallback chain will still trigger the chain ' +
+          'on rate-limit errors. When false, out-of-chain runtime picks are ' +
+          'respected and the error surfaces instead of silently falling back ' +
+          'to the chain. Models that are members of the chain always fall back ' +
+          'regardless of this setting.',
+      ),
   })
   .strict();
 

+ 166 - 0
src/hooks/foreground-fallback/index.test.ts

@@ -927,3 +927,169 @@ describe('ForegroundFallbackManager resolveChain cross-agent isolation', () => {
     expect(call[0].body.model.modelID).toBe('glm-5.2');
   });
 });
+
+// ---------------------------------------------------------------------------
+// runtimeOverride config
+// ---------------------------------------------------------------------------
+
+describe('ForegroundFallbackManager runtimeOverride', () => {
+  test('falls back for out-of-chain model when runtimeOverride=true (default)', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      client,
+      makeChains(),
+      true,
+      3,
+      true, // runtimeOverride
+    );
+
+    // Simulate session using a model NOT in any chain
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-1',
+          agent: 'orchestrator',
+          providerID: 'custom',
+          modelID: 'expensive-model',
+          error: { message: 'rate limit exceeded' },
+        },
+      },
+    });
+
+    // runtimeOverride=true → should fall back even for out-of-chain model
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+    const call = mocks.promptAsync.mock.calls[0] as [
+      { body: { model: { providerID: string; modelID: string } } },
+    ];
+    // Falls back to chain[0] = anthropic/claude-opus-4-5
+    expect(call[0].body.model.providerID).toBe('anthropic');
+    expect(call[0].body.model.modelID).toBe('claude-opus-4-5');
+  });
+
+  test('skips fallback for out-of-chain model when runtimeOverride=false', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      client,
+      makeChains(),
+      true,
+      3,
+      false, // runtimeOverride
+    );
+
+    // Simulate session using a model NOT in any chain
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-2',
+          agent: 'orchestrator',
+          providerID: 'custom',
+          modelID: 'expensive-model',
+          error: { message: 'rate limit exceeded' },
+        },
+      },
+    });
+
+    // runtimeOverride=false + model not in chain → should NOT fall back
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
+    expect(mocks.abort).toHaveBeenCalledTimes(0);
+  });
+
+  test('always falls back for in-chain model regardless of runtimeOverride=false', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      client,
+      makeChains(),
+      true,
+      3,
+      false, // runtimeOverride
+    );
+
+    // Simulate session using a model that IS in the chain
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-3',
+          agent: 'orchestrator',
+          providerID: 'anthropic',
+          modelID: 'claude-opus-4-5',
+          error: { message: 'rate limit exceeded' },
+        },
+      },
+    });
+
+    // Model IS in chain → should fall back regardless of runtimeOverride
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+    const call = mocks.promptAsync.mock.calls[0] as [
+      { body: { model: { providerID: string; modelID: string } } },
+    ];
+    // Falls back to chain[1] = openai/gpt-4o (chain[0] is the current model)
+    expect(call[0].body.model.providerID).toBe('openai');
+    expect(call[0].body.model.modelID).toBe('gpt-4o');
+  });
+
+  test('falls back for in-chain secondary model when runtimeOverride=false', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      client,
+      makeChains(),
+      true,
+      3,
+      false, // runtimeOverride
+    );
+
+    // Simulate session using chain[1] — still in chain
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-4',
+          agent: 'orchestrator',
+          providerID: 'openai',
+          modelID: 'gpt-4o',
+          error: { message: 'rate limit exceeded' },
+        },
+      },
+    });
+
+    // Model IS in chain → should fall back
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+    const call = mocks.promptAsync.mock.calls[0] as [
+      { body: { model: { providerID: string; modelID: string } } },
+    ];
+    // Falls back to chain[0] = anthropic/claude-opus-4-5 (chain[1] is tried)
+    expect(call[0].body.model.providerID).toBe('anthropic');
+    expect(call[0].body.model.modelID).toBe('claude-opus-4-5');
+  });
+
+  test('falls back for unknown agent with in-chain model when runtimeOverride=false', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      client,
+      makeChains(),
+      true,
+      3,
+      false, // runtimeOverride
+    );
+
+    // Simulate unknown agent (e.g. "compaction") using a model that IS in
+    // the orchestrator chain — resolveChain infers the chain from the model.
+    await mgr.handleEvent({
+      type: 'message.updated',
+      properties: {
+        info: {
+          sessionID: 'sess-5',
+          agent: 'compaction',
+          providerID: 'openai',
+          modelID: 'gpt-4o',
+          error: { message: 'rate limit exceeded' },
+        },
+      },
+    });
+
+    // Model IS in chain (resolved via model matching) → should fall back
+    expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
+  });
+});

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

@@ -120,6 +120,13 @@ export class ForegroundFallbackManager {
     private readonly enabled: boolean,
     /** Consecutive 429s tolerated on the same model before swap/abort. */
     private readonly maxRetries: number = 3,
+    /**
+     * When true (default), a runtime model outside the configured chain
+     * still triggers fallback on rate-limit errors. When false, out-of-chain
+     * runtime picks are respected and the error surfaces instead. Models
+     * that are members of the chain always fall back regardless.
+     */
+    private readonly runtimeOverride: boolean = true,
   ) {}
 
   /**
@@ -328,6 +335,26 @@ export class ForegroundFallbackManager {
         currentModel = chain[0];
       }
 
+      // Guard: when runtimeOverride is false, skip fallback for models
+      // that are not members of the configured chain. This respects a
+      // deliberate runtime `/model` pick (e.g. an expensive model outside
+      // the chain) and lets the error surface instead of silently swapping
+      // to the chain's default. Models that ARE in the chain always fall
+      // back normally regardless of this setting.
+      if (
+        !this.runtimeOverride &&
+        currentModel &&
+        !chain.includes(currentModel)
+      ) {
+        log('[foreground-fallback] current model not in chain, skipping fallback (runtimeOverride=false)', {
+          sessionID,
+          agentName,
+          currentModel,
+          chain,
+        });
+        return;
+      }
+
       if (!this.sessionTried.has(sessionID)) {
         this.sessionTried.set(sessionID, new Set());
       }

+ 1 - 0
src/index.ts

@@ -305,6 +305,7 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
       runtimeChains,
       config.fallback?.enabled !== false,
       config.fallback?.maxRetries ?? 3,
+      config.fallback?.runtimeOverride ?? true,
     );
 
     deepworkCommandHook = createDeepworkCommandHook();