Browse Source

refactor(foreground-fallback): remove dead _client param, make input required, clean up cancel-task

- Remove unused _client: OpencodeClient from ForegroundFallbackManager constructor
- Make input: PluginInput required (was optional with ! assertions)
- Delete canDeleteSession dead code in cancel-task.ts
- Update getClient JSDoc to describe current behavior (not v1 vs v2)
- Add ponytail comment explaining abort-verification loop removal
- Update all 41 test constructor calls to match new signature
- Remove ForegroundFallbackClient type alias (referenced deleted param)
Michael Henke 3 weeks ago
parent
commit
2b1790a22a

+ 42 - 46
src/hooks/foreground-fallback/index.test.ts

@@ -9,10 +9,6 @@ import {
 
 // ACCEPTANCE GAP: config() hook behaviour is not covered by CI — verify live.
 
-type ForegroundFallbackClient = ConstructorParameters<
-  typeof ForegroundFallbackManager
->[0];
-
 // Shared session reference so our mock.module for getClient returns the
 // current test's mock session without relying on this.input (which is
 // undefined in tests — always set in production).
@@ -74,7 +70,7 @@ function createMockClient(overrides?: {
   return {
     client: {
       session,
-    } as unknown as ForegroundFallbackClient,
+    } as never,
     mocks: { promptAsync, abort, messages },
   };
 }
@@ -207,7 +203,7 @@ describe('isFailoverError', () => {
 describe('ForegroundFallbackManager (disabled)', () => {
   test('does nothing when enabled=false', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), false);
+    const mgr = new ForegroundFallbackManager(makeChains(), false, { directory: '/test' } as any);
 
     await mgr.handleEvent({
       type: 'session.error',
@@ -232,7 +228,7 @@ describe('ForegroundFallbackManager session.error', () => {
 
   beforeEach(() => {
     ({ client, mocks } = createMockClient());
-    mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
   });
 
   test('triggers fallback on rate-limit session.error', async () => {
@@ -315,7 +311,7 @@ describe('ForegroundFallbackManager session.error', () => {
         },
       ],
     }));
-    mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -357,7 +353,7 @@ describe('ForegroundFallbackManager session.error', () => {
   });
 
   test('does nothing when no chain configured for session', async () => {
-    const emptyMgr = new ForegroundFallbackManager(client, {}, true);
+    const emptyMgr = new ForegroundFallbackManager({}, true, { directory: '/test' } as any);
     await emptyMgr.handleEvent({
       type: 'session.error',
       properties: {
@@ -372,7 +368,7 @@ describe('ForegroundFallbackManager session.error', () => {
 
   test('does not abort when promptAsync is unavailable', async () => {
     const { client, mocks } = createMockClient({ includePromptAsync: false });
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     await mgr.handleEvent({
       type: 'session.error',
@@ -395,7 +391,7 @@ describe('ForegroundFallbackManager session.error', () => {
         // abort succeeds on first call
       },
     });
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     await mgr.handleEvent({
       type: 'session.error',
@@ -418,7 +414,7 @@ describe('ForegroundFallbackManager session.error', () => {
 describe('ForegroundFallbackManager message.updated', () => {
   test('tracks model from message.updated and falls back on error', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -444,7 +440,7 @@ describe('ForegroundFallbackManager message.updated', () => {
 
   test('uses agent name from message.updated to select correct chain', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     // explorer message with its model
     await mgr.handleEvent({
@@ -489,7 +485,7 @@ describe('ForegroundFallbackManager session.status', () => {
         return {};
       },
     });
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 3);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -522,11 +518,11 @@ describe('ForegroundFallbackManager session.status', () => {
   test('keeps registered child agent identity sticky for retry fallback chain', async () => {
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(
-      client,
       makeChains({
         oracle: ['anthropic/claude-sonnet-4-5', 'openai/o3'],
       }),
       true,
+      { directory: '/test' } as any,
       1,
     );
 
@@ -561,11 +557,11 @@ describe('ForegroundFallbackManager session.status', () => {
   test('includes the sticky child agent in fallback promptAsync body', async () => {
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(
-      client,
       makeChains({
         oracle: ['anthropic/claude-sonnet-4-5', 'openai/o3'],
       }),
       true,
+      { directory: '/test' } as any,
       1,
     );
 
@@ -602,7 +598,7 @@ describe('ForegroundFallbackManager session.status', () => {
 
   test('triggers fallback on retry status with rate limit message', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 1);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 1);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -628,7 +624,7 @@ describe('ForegroundFallbackManager session.status', () => {
 
   test('triggers fallback on retry status with insufficient balance message', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 1);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 1);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -654,7 +650,7 @@ describe('ForegroundFallbackManager session.status', () => {
 
   test('ignores session.status with non-rate-limit retry message', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     await mgr.handleEvent({
       type: 'session.status',
@@ -669,7 +665,7 @@ describe('ForegroundFallbackManager session.status', () => {
 
   test('does not abort or switch after retries without a failover reason', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 3);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -698,7 +694,7 @@ describe('ForegroundFallbackManager session.status', () => {
 
   test('triggers immediate fallback on first failover retry', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 3);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -727,7 +723,7 @@ describe('ForegroundFallbackManager session.status', () => {
 
   test('switches to fallback model on first failover retry', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 3);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -756,7 +752,7 @@ describe('ForegroundFallbackManager session.status', () => {
 
   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, 1);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 1);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -783,7 +779,7 @@ describe('ForegroundFallbackManager session.status', () => {
 
   test('triggers fallback when props.error is a plain string', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 1);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 1);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -810,7 +806,7 @@ describe('ForegroundFallbackManager session.status', () => {
 
   test('non-rate-limit retry does not trigger fallback but rate-limit does', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 3);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -863,7 +859,7 @@ describe('ForegroundFallbackManager session.status', () => {
         return {};
       },
     });
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 3);
 
     // Seed session with model A (anthropic/claude-opus-4-5)
     await mgr.handleEvent({
@@ -934,7 +930,7 @@ describe('ForegroundFallbackManager session.status', () => {
         return {};
       },
     });
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 1); // maxRetries=1 for immediate fallback
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 1); // maxRetries=1 for immediate fallback
 
     // Seed session with model A
     await mgr.handleEvent({
@@ -1009,9 +1005,9 @@ describe('ForegroundFallbackManager chain exhaustion', () => {
     // tryFallback adds 'openai/gpt-b' to tried → chain.find() returns undefined → exhausted.
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(
-      client,
       { orchestrator: ['openai/gpt-b'] },
       true,
+      { directory: '/test' } as any,
     );
 
     // Seed current model as the only chain entry
@@ -1046,9 +1042,9 @@ describe('ForegroundFallbackManager chain exhaustion', () => {
     const { client, mocks } = createMockClient();
     const chain = ['openai/model-x', 'openai/model-y'];
     const mgr = new ForegroundFallbackManager(
-      client,
       { orchestrator: chain },
       true,
+      { directory: '/test' } as any,
     );
 
     // Session A: current model is model-x, which IS in the chain → picks model-y ✓
@@ -1071,9 +1067,9 @@ describe('ForegroundFallbackManager chain exhaustion', () => {
     // → abort called to stop the freeze
     const { client: client2, mocks: mocks2 } = createMockClient();
     const mgr2 = new ForegroundFallbackManager(
-      client2,
       { orchestrator: ['openai/model-y'] }, // single-entry chain already in use
       true,
+      { directory: '/test' } as any,
     );
     await mgr2.handleEvent({
       type: 'message.updated',
@@ -1099,7 +1095,7 @@ describe('ForegroundFallbackManager chain exhaustion', () => {
 describe('ForegroundFallbackManager deduplication', () => {
   test('ignores a second trigger within dedup window for same session', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     const event = {
       type: 'session.error',
@@ -1117,7 +1113,7 @@ describe('ForegroundFallbackManager deduplication', () => {
 
   test('different sessions are not deduplicated against each other', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     await mgr.handleEvent({
       type: 'session.error',
@@ -1133,7 +1129,7 @@ describe('ForegroundFallbackManager deduplication', () => {
 
   test('cascade continues when second error arrives within dedup window after model switch', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     // Seed session: current model is first entry in orchestrator chain
     await mgr.handleEvent({
@@ -1192,7 +1188,7 @@ describe('ForegroundFallbackManager deduplication', () => {
 describe('ForegroundFallbackManager subagent.session.created', () => {
   test('records agent name from subagent.session.created and falls back correctly', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     // Register the session as 'explorer' via subagent creation event
     await mgr.handleEvent({
@@ -1229,9 +1225,9 @@ describe('ForegroundFallbackManager session.deleted', () => {
     const coordinator = new SessionLifecycle(() => {});
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(
-      client,
       makeChains(),
       true,
+      { directory: '/test' } as any,
       3,
       coordinator,
     );
@@ -1276,7 +1272,7 @@ describe('ForegroundFallbackManager session.deleted', () => {
 
   test('ignores session.deleted with no sessionID', async () => {
     const { client } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
     // Should not throw
     await expect(
       mgr.handleEvent({ type: 'session.deleted', properties: {} }),
@@ -1287,9 +1283,9 @@ describe('ForegroundFallbackManager session.deleted', () => {
     const coordinator = new SessionLifecycle(() => {});
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(
-      client,
       makeChains(),
       true,
+      { directory: '/test' } as any,
       3,
       coordinator,
     );
@@ -1327,9 +1323,9 @@ describe('ForegroundFallbackManager session.deleted', () => {
     const coordinator = new SessionLifecycle(() => {});
     const { client } = createMockClient();
     const mgr = new ForegroundFallbackManager(
-      client,
       makeChains(),
       true,
+      { directory: '/test' } as any,
       3,
       coordinator,
     );
@@ -1378,12 +1374,12 @@ describe('ForegroundFallbackManager resolveChain cross-agent isolation', () => {
     // orchestrator's chain - re-prompting oracle with an orchestrator model.
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(
-      client,
       {
         // oracle intentionally absent - no chain configured
         orchestrator: ['openai/gpt-4o', 'google/gemini-2.5-pro'],
       },
       true,
+      { directory: '/test' } as any,
     );
 
     await mgr.handleEvent({
@@ -1408,9 +1404,9 @@ describe('ForegroundFallbackManager resolveChain cross-agent isolation', () => {
     // chain, the last-resort flattened chain is acceptable.
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(
-      client,
       { orchestrator: ['openai/gpt-4o'] },
       true,
+      { directory: '/test' } as any,
     );
 
     // No agent name tracked, no model tracked - triggers session.error
@@ -1437,9 +1433,9 @@ describe('ForegroundFallbackManager resolveChain cross-agent isolation', () => {
     // chain — that would switch the session from Build to Orchestrator.
     const { client, mocks } = createMockClient();
     const mgr = new ForegroundFallbackManager(
-      client,
       { orchestrator: ['openai/gpt-5.6', 'new-api/glm-5.2'] },
       true,
+      { directory: '/test' } as any,
     );
 
     await mgr.handleEvent({
@@ -1470,7 +1466,7 @@ describe('ForegroundFallbackManager no-chain sessions', () => {
     // FG must not abort or re-prompt — that races the council lifecycle and
     // previously produced "[foreground-fallback] no chain configured" noise.
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 3);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -1502,7 +1498,7 @@ describe('ForegroundFallbackManager no-chain sessions', () => {
 
   test('councillor session.error: no abort and no re-prompt', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     await mgr.handleEvent({
       type: 'message.updated',
@@ -1530,7 +1526,7 @@ describe('ForegroundFallbackManager no-chain sessions', () => {
 
   test('disableChain agent on session.status: no abort (not just no re-prompt)', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true, 3);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any, 3);
     mgr.disableChain('orchestrator');
 
     await mgr.handleEvent({
@@ -1569,7 +1565,7 @@ describe('ForegroundFallbackManager no-chain sessions', () => {
 describe('ForegroundFallbackManager disableChain', () => {
   test('after disableChain, rate-limit error surfaces instead of falling back', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     mgr.disableChain('orchestrator');
 
@@ -1594,7 +1590,7 @@ describe('ForegroundFallbackManager disableChain', () => {
 
   test('other agents chains are unaffected by disableChain', async () => {
     const { client, mocks } = createMockClient();
-    const mgr = new ForegroundFallbackManager(client, makeChains(), true);
+    const mgr = new ForegroundFallbackManager(makeChains(), true, { directory: '/test' } as any);
 
     mgr.disableChain('orchestrator');
 

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

@@ -28,8 +28,6 @@ import {
 import type { SessionLifecycle } from '../session-lifecycle';
 import { isUserMessageWithParts } from '../types';
 
-type OpencodeClient = PluginInput['client'];
-
 // ---------------------------------------------------------------------------
 // Retryable error detection
 // ---------------------------------------------------------------------------
@@ -257,7 +255,6 @@ export class ForegroundFallbackManager {
   }
 
   constructor(
-    _client: OpencodeClient,
     /**
      * Ordered fallback chains per agent.
      * e.g. { orchestrator: ['anthropic/claude-opus-4-5', 'openai/gpt-4o'] }
@@ -265,10 +262,10 @@ export class ForegroundFallbackManager {
      */
     private chains: Record<string, string[]>,
     private readonly enabled: boolean,
+    private readonly input: PluginInput,
     /** Consecutive 429s tolerated on the same model before swap/abort. */
     private readonly maxRetries: number = 3,
     coordinator?: SessionLifecycle,
-    private readonly input?: PluginInput,
   ) {
     if (coordinator) {
       coordinator.onSessionDeleted((id) => {

+ 1 - 2
src/index.ts

@@ -328,12 +328,11 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
     // Agents without a chain (e.g. councillor, owned by CouncilManager) are
     // left alone — FG only aborts/re-prompts when it has a model to switch to.
     foregroundFallback = new ForegroundFallbackManager(
-      ctx.client,
       runtimeChains,
       config.fallback?.enabled !== false,
+      ctx,
       config.fallback?.maxRetries ?? 3,
       sessionLifecycle,
-      ctx,
     );
 
     deepworkCommandHook = createDeepworkCommandHook();

+ 3 - 5
src/tools/cancel-task.ts

@@ -238,7 +238,9 @@ async function abortAndVerifySession(
     });
   }
 
-  // v2: delete is always available, skip polling fallback
+  // ponytail: v1 had a polling loop here that verified abort succeeded before
+  // proceeding to delete. v2 abort is server-side and synchronous — the delete
+  // verification loop below catches any remaining running state.
   await deleteAndVerifySession(options, taskID, 'cancel-task-after-abort');
 }
 
@@ -319,10 +321,6 @@ async function deleteAndVerifySession(
   );
 }
 
-function canDeleteSession(_input: PluginInput): boolean {
-  return true;
-}
-
 async function getSessionStatus(
   input: PluginInput,
   taskID: string,

+ 4 - 5
src/utils/opencode-client.ts

@@ -7,11 +7,10 @@ import { createOpencodeClient, type OpencodeClient } from '@opencode-ai/sdk/v2';
 const v2Clients = new Map<string, OpencodeClient>();
 
 /**
- * Returns a v2 OpenCode SDK client scoped to the same directory as the
- * plugin-provided v1 client. The v2 client exposes session methods
- * (switchModel, switchAgent) that are absent from the v1 client the
- * plugin hands us. Both clients target the same local server; session
- * state is server-side, so they observe identical sessions.
+ * Returns a memoized v2 OpenCode SDK client for the given plugin directory.
+ * The v2 client exposes session methods (switchModel, switchAgent) not
+ * available through PluginInput.client. Keyed by directory; the server
+ * holds session state so a cached client is valid for the process lifetime.
  */
 export function getClient(input: PluginInput): OpencodeClient {
   const cached = v2Clients.get(input.directory);