Browse Source

Merge pull request #694 from dragon-Elec/fix/param-order-hotfix

fix: reorder constructor params (coordinator before runtimeOverride)
Alvin 1 month ago
parent
commit
a9f6508a8c
3 changed files with 105 additions and 4 deletions
  1. 100 2
      src/hooks/foreground-fallback/index.test.ts
  2. 4 1
      src/hooks/foreground-fallback/index.ts
  3. 1 1
      src/index.ts

+ 100 - 2
src/hooks/foreground-fallback/index.test.ts

@@ -947,6 +947,7 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       makeChains(),
       true,
       3,
+      undefined,
       true, // runtimeOverride
     );
 
@@ -981,6 +982,7 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       makeChains(),
       true,
       3,
+      undefined,
       false, // runtimeOverride
     );
 
@@ -998,9 +1000,9 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       },
     });
 
-    // runtimeOverride=false + model not in chain → should NOT fall back
+    // runtimeOverride=false + model not in chain → abort session, no fallback
     expect(mocks.promptAsync).toHaveBeenCalledTimes(0);
-    expect(mocks.abort).toHaveBeenCalledTimes(0);
+    expect(mocks.abort).toHaveBeenCalledTimes(1);
   });
 
   test('always falls back for in-chain model regardless of runtimeOverride=false', async () => {
@@ -1010,6 +1012,7 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       makeChains(),
       true,
       3,
+      undefined,
       false, // runtimeOverride
     );
 
@@ -1044,6 +1047,7 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       makeChains(),
       true,
       3,
+      undefined,
       false, // runtimeOverride
     );
 
@@ -1078,6 +1082,7 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
       makeChains(),
       true,
       3,
+      undefined,
       false, // runtimeOverride
     );
 
@@ -1099,4 +1104,97 @@ describe('ForegroundFallbackManager runtimeOverride', () => {
     // 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 and out-of-chain model aborts after retry budget exhausted', async () => {
+    const { client, mocks } = createMockClient();
+    const mgr = new ForegroundFallbackManager(
+      client,
+      makeChains(),
+      true,
+      3, // 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',
+        },
+      },
+    });
+
+    // 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
+    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).toHaveBeenCalledTimes(1);
+  });
 });

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

@@ -121,6 +121,7 @@ export class ForegroundFallbackManager {
     private readonly enabled: boolean,
     /** Consecutive 429s tolerated on the same model before swap/abort. */
     private readonly maxRetries: number = 3,
+    coordinator?: SessionLifecycle,
     /**
      * When true (default), a runtime model outside the configured chain
      * still triggers fallback on rate-limit errors. When false, out-of-chain
@@ -128,7 +129,6 @@ export class ForegroundFallbackManager {
      * that are members of the chain always fall back regardless.
      */
     private readonly runtimeOverride: boolean = true,
-    coordinator?: SessionLifecycle,
   ) {
     if (coordinator) {
       coordinator.onSessionDeleted((id) => {
@@ -356,6 +356,9 @@ export class ForegroundFallbackManager {
           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;
       }
 

+ 1 - 1
src/index.ts

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