Browse Source

Merge pull request #737 from alvinunreal/fix/718-model-selection-stick

Alvin 2 weeks ago
parent
commit
0385ee4b8f

+ 1 - 2
oh-my-opencode-slim.schema.json

@@ -1095,8 +1095,7 @@
           "type": "boolean"
           "type": "boolean"
         },
         },
         "runtimeOverride": {
         "runtimeOverride": {
-          "default": true,
-          "description": "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.",
+          "description": "DEPRECATED: no longer used. Previously controlled whether out-of-chain runtime model picks triggered fallback. Fallback is now always disabled when a user explicitly selects a model via /model.",
           "type": "boolean"
           "type": "boolean"
         }
         }
       },
       },

+ 7 - 7
src/config/schema.ts

@@ -239,16 +239,16 @@ export const FailoverConfigSchema = z
         'When true (default), empty provider responses are treated as failures, ' +
         'When true (default), empty provider responses are treated as failures, ' +
           'triggering fallback/retry. Set to false to treat them as successes.',
           'triggering fallback/retry. Set to false to treat them as successes.',
       ),
       ),
+    // DEPRECATED: accepted for backward compatibility but no longer used.
+    // Fallback is now always disabled when a user explicitly selects a model
+    // via /model, so this flag has no effect.
     runtimeOverride: z
     runtimeOverride: z
       .boolean()
       .boolean()
-      .default(true)
+      .optional()
       .describe(
       .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.',
+        'DEPRECATED: no longer used. Previously controlled whether out-of-chain ' +
+          'runtime model picks triggered fallback. Fallback is now always ' +
+          'disabled when a user explicitly selects a model via /model.',
       ),
       ),
   })
   })
   .strict();
   .strict();

+ 24 - 205
src/hooks/foreground-fallback/index.test.ts

@@ -6,6 +6,8 @@ import {
   isRetryableError,
   isRetryableError,
 } from './index';
 } from './index';
 
 
+// ACCEPTANCE GAP: config() hook behaviour is not covered by CI — verify live.
+
 type ForegroundFallbackClient = ConstructorParameters<
 type ForegroundFallbackClient = ConstructorParameters<
   typeof ForegroundFallbackManager
   typeof ForegroundFallbackManager
 >[0];
 >[0];
@@ -1391,92 +1393,22 @@ describe('ForegroundFallbackManager resolveChain cross-agent isolation', () => {
 });
 });
 
 
 // ---------------------------------------------------------------------------
 // ---------------------------------------------------------------------------
-// runtimeOverride config
+// disableChain API
 // ---------------------------------------------------------------------------
 // ---------------------------------------------------------------------------
 
 
-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,
-      undefined,
-      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 () => {
+describe('ForegroundFallbackManager disableChain', () => {
+  test('after disableChain, rate-limit error surfaces instead of falling back', async () => {
     const { client, mocks } = createMockClient();
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(
-      client,
-      makeChains(),
-      true,
-      3,
-      undefined,
-      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 → abort session, no fallback
-    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
-    expect(mocks.abort).toHaveBeenCalledTimes(1);
-  });
+    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
 
 
-  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,
-      undefined,
-      false, // runtimeOverride
-    );
+    mgr.disableChain('orchestrator');
 
 
-    // Simulate session using a model that IS in the chain
+    // Seed session with orchestrator model and trigger rate-limit
     await mgr.handleEvent({
     await mgr.handleEvent({
       type: 'message.updated',
       type: 'message.updated',
       properties: {
       properties: {
         info: {
         info: {
-          sessionID: 'sess-3',
+          sessionID: 'sess-disabled',
           agent: 'orchestrator',
           agent: 'orchestrator',
           providerID: 'anthropic',
           providerID: 'anthropic',
           modelID: 'claude-opus-4-5',
           modelID: 'claude-opus-4-5',
@@ -1485,151 +1417,38 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       },
       },
     });
     });
 
 
-    // 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');
+    // Chain disabled → no fallback, error surfaces
+    expect(mocks.promptAsync).not.toHaveBeenCalled();
+    expect(mocks.abort).not.toHaveBeenCalled();
   });
   });
 
 
-  test('falls back for in-chain secondary model when runtimeOverride=false', async () => {
+  test('other agents chains are unaffected by disableChain', async () => {
     const { client, mocks } = createMockClient();
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(
-      client,
-      makeChains(),
-      true,
-      3,
-      undefined,
-      false, // runtimeOverride
-    );
+    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
 
 
-    // Simulate session using chain[1] — still in chain
+    mgr.disableChain('orchestrator');
+
+    // Explorer session — should still fall back normally
     await mgr.handleEvent({
     await mgr.handleEvent({
       type: 'message.updated',
       type: 'message.updated',
       properties: {
       properties: {
         info: {
         info: {
-          sessionID: 'sess-4',
-          agent: 'orchestrator',
+          sessionID: 'sess-other',
+          agent: 'explorer',
           providerID: 'openai',
           providerID: 'openai',
-          modelID: 'gpt-4o',
-          error: { message: 'rate limit exceeded' },
+          modelID: 'gpt-4o-mini',
+          error: { message: 'quota exceeded' },
         },
         },
       },
       },
     });
     });
 
 
-    // Model IS in chain → should fall back
     expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
     expect(mocks.promptAsync).toHaveBeenCalledTimes(1);
     const call = mocks.promptAsync.mock.calls[0] as [
     const call = mocks.promptAsync.mock.calls[0] as [
       { body: { model: { providerID: string; modelID: string } } },
       { body: { model: { providerID: string; modelID: string } } },
     ];
     ];
-    // Falls back to chain[0] = anthropic/claude-opus-4-5 (chain[1] is tried)
+    // explorer chain: ['openai/gpt-4o-mini', 'anthropic/claude-haiku']
+    // current = gpt-4o-mini is tried → next = claude-haiku
     expect(call[0].body.model.providerID).toBe('anthropic');
     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,
-      undefined,
-      false, // runtimeOverride
-    );
-
-    // 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',
-          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);
-  });
-
-  test('session.error with runtimeOverride=false and out-of-chain model aborts session', async () => {
-    const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(
-      client,
-      makeChains(),
-      true,
-      3,
-      undefined,
-      false, // runtimeOverride
-    );
-
-    // Seed session with out-of-chain model
-    await mgr.handleEvent({
-      type: 'message.updated',
-      properties: {
-        info: {
-          sessionID: 'sess-err-override',
-          agent: 'orchestrator',
-          providerID: 'custom',
-          modelID: 'expensive-model',
-        },
-      },
-    });
-
-    // Trigger session.error with rate limit
-    await mgr.handleEvent({
-      type: 'session.error',
-      properties: {
-        sessionID: 'sess-err-override',
-        error: { message: 'Rate limit exceeded' },
-      },
-    });
-
-    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
-    expect(mocks.abort).toHaveBeenCalledTimes(1);
-  });
-
-  test('session.status with runtimeOverride=false aborts an out-of-chain model after retry exhaustion', async () => {
-    const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(
-      client,
-      makeChains(),
-      true,
-      1, // maxRetries
-      undefined,
-      false, // runtimeOverride
-    );
-
-    // Seed session with out-of-chain model
-    await mgr.handleEvent({
-      type: 'message.updated',
-      properties: {
-        info: {
-          sessionID: 'sess-status-override',
-          agent: 'orchestrator',
-          providerID: 'custom',
-          modelID: 'expensive-model',
-        },
-      },
-    });
-
-    // The first failover retry exhausts this one-attempt budget.
-    await mgr.handleEvent({
-      type: 'session.status',
-      properties: {
-        sessionID: 'sess-status-override',
-        status: { type: 'retry', message: 'rate limit, retrying...' },
-      },
-    });
-    expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
-    expect(mocks.abort).toHaveBeenCalled();
+    expect(call[0].body.model.modelID).toBe('claude-haiku');
   });
   });
 });
 });

+ 13 - 34
src/hooks/foreground-fallback/index.ts

@@ -219,6 +219,18 @@ export class ForegroundFallbackManager {
     return this.inProgress.has(sessionID);
     return this.inProgress.has(sessionID);
   }
   }
 
 
+  /**
+   * Disable the fallback chain for a specific agent.
+   * After calling this, rate-limit errors for that agent surface instead of
+   * silently falling back through the chain.
+   */
+  disableChain(agentName: string): void {
+    // Keep the key present (known agent, no chain) rather than deleting it,
+    // so resolveChain's "known agent without a chain" path applies and the
+    // shared runtimeChains reference retains the agent entry.
+    this.chains[agentName] = [];
+  }
+
   registerSessionAgent(sessionID: string, agentName: string): void {
   registerSessionAgent(sessionID: string, agentName: string): void {
     const normalizedAgentName = agentName.trim();
     const normalizedAgentName = agentName.trim();
     if (
     if (
@@ -238,18 +250,11 @@ export class ForegroundFallbackManager {
      * e.g. { orchestrator: ['anthropic/claude-opus-4-5', 'openai/gpt-4o'] }
      * e.g. { orchestrator: ['anthropic/claude-opus-4-5', 'openai/gpt-4o'] }
      * The first model that hasn't been tried yet is selected on each fallback.
      * The first model that hasn't been tried yet is selected on each fallback.
      */
      */
-    private readonly chains: Record<string, string[]>,
+    private chains: Record<string, string[]>,
     private readonly enabled: boolean,
     private readonly enabled: boolean,
     /** Consecutive 429s tolerated on the same model before swap/abort. */
     /** Consecutive 429s tolerated on the same model before swap/abort. */
     private readonly maxRetries: number = 3,
     private readonly maxRetries: number = 3,
     coordinator?: SessionLifecycle,
     coordinator?: SessionLifecycle,
-    /**
-     * 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,
   ) {
   ) {
     if (coordinator) {
     if (coordinator) {
       coordinator.onSessionDeleted((id) => {
       coordinator.onSessionDeleted((id) => {
@@ -540,32 +545,6 @@ export class ForegroundFallbackManager {
         currentModel = chain[0];
         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,
-          },
-        );
-        // Abort the session so the rate-limit error surfaces to the user
-        // instead of leaving the session in a silent retry loop.
-        await abortSessionWithTimeout(this.client, sessionID);
-        return;
-      }
-
       if (!this.sessionTried.has(sessionID)) {
       if (!this.sessionTried.has(sessionID)) {
         this.sessionTried.set(sessionID, new Set());
         this.sessionTried.set(sessionID, new Set());
       }
       }

+ 14 - 1
src/index.ts

@@ -139,6 +139,7 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
   let agents: ReturnType<typeof getAgentConfigs>;
   let agents: ReturnType<typeof getAgentConfigs>;
   let mcps: ReturnType<typeof createBuiltinMcps>;
   let mcps: ReturnType<typeof createBuiltinMcps>;
   let modelArrayMap: Record<string, Array<{ id: string; variant?: string }>>;
   let modelArrayMap: Record<string, Array<{ id: string; variant?: string }>>;
+  let everModelSwitched: Set<string>;
   let runtimeChains: Record<string, string[]>;
   let runtimeChains: Record<string, string[]>;
   let multiplexerConfig: MultiplexerConfig;
   let multiplexerConfig: MultiplexerConfig;
   let multiplexerEnabled: boolean;
   let multiplexerEnabled: boolean;
@@ -216,6 +217,7 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
       string,
       string,
       Array<{ id: string; variant?: string }>
       Array<{ id: string; variant?: string }>
     >;
     >;
+    everModelSwitched = new Set<string>();
     runtimeChains = {} as Record<string, string[]>;
     runtimeChains = {} as Record<string, string[]>;
     for (const agentDef of agentDefs) {
     for (const agentDef of agentDefs) {
       if (agentDef._modelArray?.length) {
       if (agentDef._modelArray?.length) {
@@ -316,7 +318,6 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
       config.fallback?.enabled !== false,
       config.fallback?.enabled !== false,
       config.fallback?.maxRetries ?? 3,
       config.fallback?.maxRetries ?? 3,
       sessionLifecycle,
       sessionLifecycle,
-      config.fallback?.runtimeOverride ?? true,
     );
     );
 
 
     deepworkCommandHook = createDeepworkCommandHook();
     deepworkCommandHook = createDeepworkCommandHook();
@@ -565,6 +566,18 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
           const existing = (opencodeConfig.agent as Record<string, unknown>)[
           const existing = (opencodeConfig.agent as Record<string, unknown>)[
             name
             name
           ] as Record<string, unknown> | undefined;
           ] as Record<string, unknown> | undefined;
+          // User explicitly picked a model via /model → disable fallback.
+          // Only marks the agent if the model differs from the chain primary.
+          // Once marked, stays disabled even if user switches back to chain[0].
+          if (existing && typeof existing.model === 'string') {
+            const primary = modelArrayMap[name]?.[0]?.id;
+            if (primary && existing.model !== primary) {
+              everModelSwitched.add(name);
+            }
+            if (everModelSwitched.has(name)) {
+              foregroundFallback.disableChain(name);
+            }
+          }
           if (existing) {
           if (existing) {
             // Shallow merge: plugin defaults first, user overrides win
             // Shallow merge: plugin defaults first, user overrides win
             (opencodeConfig.agent as Record<string, unknown>)[name] = {
             (opencodeConfig.agent as Record<string, unknown>)[name] = {