Browse Source

fix(hooks): harden todo auto-continuation scheduling (#270)

* Stabilize todo auto-continuation scheduling

Auto-continuation depended on a single first-idle session heuristic and treated every orchestrator busy event as external activity. That made multi-session/subagent flows fragile and could let the countdown notification cancel its own pending continuation.

Constraint: Preserve the existing auto-continue prompt and thresholds

Constraint: Do not modify OpenCode core

Rejected: Rewrite the continuation prompt to force todo completion | changes model-facing behavior beyond the scheduling bug

Rejected: Treat autoEnableThreshold as a bug | it is configurable policy

Confidence: high

Scope-risk: narrow

Directive: Keep auto-continuation routing tied to resolved agent identity, not first idle order

Tested: bun test src/hooks/todo-continuation/index.test.ts

Tested: bun run typecheck

Tested: bun run check:ci

* Keep todo continuation timers session-scoped

The first scheduling fix tracked multiple orchestrator sessions but still used one shared timer without recording its owner. This keeps the shared single-timer behavior while preventing deletion of another orchestrator session from cancelling an unrelated pending continuation.

Constraint: Preserve existing auto-continue prompt and policy settings

Rejected: Introduce per-session continuation timers | larger behavior change and unnecessary for current single-active-timer design

Confidence: high

Scope-risk: narrow

Directive: If auto-continuation becomes truly parallel per session later, split counters/timers into per-session state deliberately

Tested: bun test src/hooks/todo-continuation/index.test.ts

Tested: bun run typecheck

Tested: bun run check:ci

* Preserve todo continuation limit during countdown

The countdown noReply prompt can emit a busy transition before the real continuation prompt fires. The previous guard prevented timer cancellation but still allowed that transition to reset the consecutive continuation counter, bypassing maxContinuations.

Constraint: Preserve existing prompt text and scheduling policy

Rejected: Remove the countdown notification | user-visible behavior change

Confidence: high

Scope-risk: narrow

Tested: bun test src/hooks/todo-continuation/index.test.ts

Tested: bun run typecheck

Tested: bun run check:ci

* Harden todo continuation notification guard

The boolean notification guard still depended on HTTP/SSE ordering and could be cleared by an unrelated notification completion. Track notification busy suppression per session with a short grace window so countdown busy events cannot cancel timers or reset max-continuation counters.

Constraint: Preserve existing prompt text and scheduling policy

Rejected: Remove the countdown notification | user-visible behavior change

Rejected: Use a long grace window | could hide real user activity for too long

Confidence: high

Scope-risk: narrow

Tested: bun test src/hooks/todo-continuation/index.test.ts

Tested: bun run typecheck

Tested: bun run check:ci
Raxxoor 2 days ago
parent
commit
dd42c26a22
3 changed files with 401 additions and 33 deletions
  1. 279 7
      src/hooks/todo-continuation/index.test.ts
  2. 111 23
      src/hooks/todo-continuation/index.ts
  3. 11 3
      src/index.ts

+ 279 - 7
src/hooks/todo-continuation/index.test.ts

@@ -397,7 +397,7 @@ describe('createTodoContinuationHook', () => {
       });
       const hook = createTodoContinuationHook(ctx, {
         maxContinuations: 5,
-        cooldownMs: 100,
+        cooldownMs: 500,
       });
 
       await hook.tool.auto_continue.execute({ enabled: true });
@@ -410,8 +410,8 @@ describe('createTodoContinuationHook', () => {
         },
       });
 
-      // Before cooldown expires, fire session.status with busy
-      await delay(50);
+      // After the notification grace but before cooldown expires, fire busy.
+      await delay(300);
       await hook.handleEvent({
         event: {
           type: 'session.status',
@@ -423,7 +423,7 @@ describe('createTodoContinuationHook', () => {
       });
 
       // Advance past original cooldown
-      await delay(60);
+      await delay(250);
 
       // Verify timer was cancelled and prompt NOT called
       expect(hasContinuation(ctx.client.session.prompt)).toBe(false);
@@ -473,7 +473,7 @@ describe('createTodoContinuationHook', () => {
       });
 
       // Advance past original cooldown
-      await delay(60);
+      await delay(250);
 
       // Orchestrator timer should still fire — prompt was called
       expect(hasContinuation(ctx.client.session.prompt)).toBe(true);
@@ -806,7 +806,7 @@ describe('createTodoContinuationHook', () => {
       });
 
       // Advance past original cooldown
-      await delay(60);
+      await delay(250);
 
       // Verify timer was cancelled and prompt NOT called
       expect(hasContinuation(ctx.client.session.prompt)).toBe(false);
@@ -855,7 +855,7 @@ describe('createTodoContinuationHook', () => {
       });
 
       // Advance past original cooldown
-      await delay(60);
+      await delay(250);
 
       // Orchestrator timer should still fire — prompt was called
       expect(hasContinuation(ctx.client.session.prompt)).toBe(true);
@@ -1897,6 +1897,278 @@ describe('createTodoContinuationHook', () => {
     });
   });
 
+  describe('session routing and notification cancellation', () => {
+    function createPendingCtx() {
+      return createMockContext({
+        todoResult: {
+          data: [
+            { id: '1', content: 'todo1', status: 'pending', priority: 'high' },
+          ],
+        },
+        messagesResult: {
+          data: [
+            {
+              info: { role: 'assistant' },
+              parts: [{ type: 'text', text: 'Work in progress' }],
+            },
+          ],
+        },
+      });
+    }
+
+    test('chat.message registers orchestrator sessions without first-idle lockout', async () => {
+      const ctx = createPendingCtx();
+      const hook = createTodoContinuationHook(ctx, {
+        cooldownMs: 50,
+      });
+      await hook.tool.auto_continue.execute({ enabled: true });
+
+      hook.handleChatMessage({ sessionID: 'sub1', agent: 'fixer' });
+      hook.handleChatMessage({ sessionID: 'main1', agent: 'orchestrator' });
+      hook.handleChatMessage({ sessionID: 'main2', agent: 'orchestrator' });
+
+      await hook.handleEvent({
+        event: {
+          type: 'session.idle',
+          properties: { sessionID: 'sub1' },
+        },
+      });
+      await delay(60);
+      expect(hasContinuation(ctx.client.session.prompt)).toBe(false);
+
+      await hook.handleEvent({
+        event: {
+          type: 'session.idle',
+          properties: { sessionID: 'main2' },
+        },
+      });
+      await delay(60);
+
+      expect(hasContinuation(ctx.client.session.prompt)).toBe(true);
+      expect(contCall(ctx.client.session.prompt)[0].path.id).toBe('main2');
+    });
+
+    test('chat.message without agent does not block legacy first-idle fallback', async () => {
+      const ctx = createPendingCtx();
+      const hook = createTodoContinuationHook(ctx, {
+        cooldownMs: 50,
+        autoEnable: true,
+        autoEnableThreshold: 1,
+      });
+
+      hook.handleChatMessage({ sessionID: 'main1' });
+      await hook.handleEvent({
+        event: {
+          type: 'session.idle',
+          properties: { sessionID: 'main1' },
+        },
+      });
+      await delay(60);
+
+      expect(hasContinuation(ctx.client.session.prompt)).toBe(true);
+    });
+
+    test('subagent chat.message prevents first-idle fallback registration', async () => {
+      const ctx = createPendingCtx();
+      const hook = createTodoContinuationHook(ctx, {
+        cooldownMs: 50,
+        autoEnable: true,
+        autoEnableThreshold: 1,
+      });
+
+      hook.handleChatMessage({ sessionID: 'sub1', agent: 'fixer' });
+      await hook.handleEvent({
+        event: {
+          type: 'session.idle',
+          properties: { sessionID: 'sub1' },
+        },
+      });
+      await delay(60);
+
+      expect(ctx.client.session.prompt).not.toHaveBeenCalled();
+    });
+
+    test('session.status idle triggers continuation like session.idle', async () => {
+      const ctx = createPendingCtx();
+      const hook = createTodoContinuationHook(ctx, { cooldownMs: 50 });
+      await hook.tool.auto_continue.execute({ enabled: true });
+      hook.handleChatMessage({ sessionID: 'main1', agent: 'orchestrator' });
+
+      await hook.handleEvent({
+        event: {
+          type: 'session.status',
+          properties: { sessionID: 'main1', status: { type: 'idle' } },
+        },
+      });
+      await delay(60);
+
+      expect(hasContinuation(ctx.client.session.prompt)).toBe(true);
+    });
+
+    test('deleting another orchestrator does not cancel the active session timer', async () => {
+      const ctx = createPendingCtx();
+      const hook = createTodoContinuationHook(ctx, { cooldownMs: 50 });
+      await hook.tool.auto_continue.execute({ enabled: true });
+      hook.handleChatMessage({ sessionID: 'main1', agent: 'orchestrator' });
+      hook.handleChatMessage({ sessionID: 'main2', agent: 'orchestrator' });
+
+      await hook.handleEvent({
+        event: {
+          type: 'session.idle',
+          properties: { sessionID: 'main1' },
+        },
+      });
+      await hook.handleEvent({
+        event: {
+          type: 'session.deleted',
+          properties: { sessionID: 'main2' },
+        },
+      });
+      await delay(60);
+
+      expect(hasContinuation(ctx.client.session.prompt)).toBe(true);
+      expect(contCall(ctx.client.session.prompt)[0].path.id).toBe('main1');
+    });
+
+    test('deleting all orchestrators restores legacy first-idle fallback', async () => {
+      const ctx = createPendingCtx();
+      const hook = createTodoContinuationHook(ctx, {
+        cooldownMs: 50,
+        autoEnable: true,
+        autoEnableThreshold: 1,
+      });
+      hook.handleChatMessage({ sessionID: 'main1', agent: 'orchestrator' });
+      hook.handleChatMessage({ sessionID: 'main2', agent: 'orchestrator' });
+
+      await hook.handleEvent({
+        event: {
+          type: 'session.deleted',
+          properties: { sessionID: 'main1' },
+        },
+      });
+      await hook.handleEvent({
+        event: {
+          type: 'session.deleted',
+          properties: { sessionID: 'main2' },
+        },
+      });
+
+      await hook.handleEvent({
+        event: {
+          type: 'session.idle',
+          properties: { sessionID: 'legacy-main' },
+        },
+      });
+      await delay(60);
+
+      expect(hasContinuation(ctx.client.session.prompt)).toBe(true);
+      expect(contCall(ctx.client.session.prompt)[0].path.id).toBe(
+        'legacy-main',
+      );
+    });
+
+    test('countdown notification busy status does not reset max-continuation counter', async () => {
+      const ctx = createPendingCtx();
+      const releaseNotifications: Array<() => void> = [];
+      ctx.client.session.prompt = mock(async (args: any) => {
+        if (args?.body?.noReply === true) {
+          await new Promise<void>((resolve) => {
+            releaseNotifications.push(resolve);
+          });
+        }
+        return {};
+      });
+      const hook = createTodoContinuationHook(ctx, {
+        cooldownMs: 50,
+        maxContinuations: 2,
+      });
+      await hook.tool.auto_continue.execute({ enabled: true });
+      hook.handleChatMessage({ sessionID: 'main1', agent: 'orchestrator' });
+
+      for (let i = 0; i < 2; i++) {
+        await hook.handleEvent({
+          event: {
+            type: 'session.idle',
+            properties: { sessionID: 'main1' },
+          },
+        });
+        await hook.handleEvent({
+          event: {
+            type: 'session.status',
+            properties: { sessionID: 'main1', status: { type: 'busy' } },
+          },
+        });
+        await delay(60);
+        releaseNotifications.shift()?.();
+        await delay(10);
+      }
+
+      ctx.client.session.prompt.mockClear();
+      await hook.handleEvent({
+        event: {
+          type: 'session.idle',
+          properties: { sessionID: 'main1' },
+        },
+      });
+      await delay(60);
+
+      expect(ctx.client.session.prompt).not.toHaveBeenCalled();
+    });
+
+    test('late countdown notification busy status does not cancel continuation timer', async () => {
+      const ctx = createPendingCtx();
+      const hook = createTodoContinuationHook(ctx, { cooldownMs: 50 });
+      await hook.tool.auto_continue.execute({ enabled: true });
+      hook.handleChatMessage({ sessionID: 'main1', agent: 'orchestrator' });
+
+      await hook.handleEvent({
+        event: {
+          type: 'session.idle',
+          properties: { sessionID: 'main1' },
+        },
+      });
+      await delay(10);
+      await hook.handleEvent({
+        event: {
+          type: 'session.status',
+          properties: { sessionID: 'main1', status: { type: 'busy' } },
+        },
+      });
+      await delay(60);
+
+      expect(hasContinuation(ctx.client.session.prompt)).toBe(true);
+    });
+
+    test('countdown notification busy status does not cancel continuation timer', async () => {
+      const ctx = createPendingCtx();
+      let callCount = 0;
+      ctx.client.session.prompt = mock(async () => {
+        callCount++;
+        return {};
+      });
+      const hook = createTodoContinuationHook(ctx, { cooldownMs: 50 });
+      await hook.tool.auto_continue.execute({ enabled: true });
+      hook.handleChatMessage({ sessionID: 'main1', agent: 'orchestrator' });
+
+      await hook.handleEvent({
+        event: {
+          type: 'session.idle',
+          properties: { sessionID: 'main1' },
+        },
+      });
+      await hook.handleEvent({
+        event: {
+          type: 'session.status',
+          properties: { sessionID: 'main1', status: { type: 'busy' } },
+        },
+      });
+      await delay(60);
+
+      expect(callCount).toBeGreaterThanOrEqual(2);
+      expect(hasContinuation(ctx.client.session.prompt)).toBe(true);
+    });
+  });
+
   describe('auto-enable on todo count', () => {
     function createAutoEnableCtx(
       todos: Array<{

+ 111 - 23
src/hooks/todo-continuation/index.ts

@@ -11,6 +11,7 @@ const CONTINUATION_PROMPT =
 // Suppress window after user abort (Esc/Ctrl+C) to avoid immediately
 // re-continuing something the user explicitly stopped
 const SUPPRESS_AFTER_ABORT_MS = 5_000;
+const NOTIFICATION_BUSY_GRACE_MS = 250;
 
 const QUESTION_PHRASES = [
   'would you like',
@@ -33,11 +34,18 @@ interface ContinuationState {
   enabled: boolean;
   consecutiveContinuations: number;
   pendingTimer: ReturnType<typeof setTimeout> | null;
+  pendingTimerSessionId: string | null;
   suppressUntil: number;
-  orchestratorSessionId: string | null;
+  orchestratorSessionIds: Set<string>;
+  sawChatMessage: boolean;
   // True while our auto-injection prompt is in flight — prevents counter reset
   // on session.status→busy and blocks duplicate injections
   isAutoInjecting: boolean;
+  // session IDs with an in-flight noReply countdown notification.
+  notifyingSessionIds: Set<string>;
+  // sessionID → timestamp until which just-completed noReply countdown
+  // notification busy transitions are ignored, covering HTTP/SSE reordering.
+  notificationBusyUntilBySession: Map<string, number>;
 }
 
 function isQuestion(text: string): boolean {
@@ -77,6 +85,7 @@ function cancelPendingTimer(state: ContinuationState): void {
     clearTimeout(state.pendingTimer);
     state.pendingTimer = null;
   }
+  state.pendingTimerSessionId = null;
 }
 
 function resetState(state: ContinuationState): void {
@@ -84,6 +93,8 @@ function resetState(state: ContinuationState): void {
   state.consecutiveContinuations = 0;
   state.suppressUntil = 0;
   state.isAutoInjecting = false;
+  state.notifyingSessionIds.clear();
+  state.notificationBusyUntilBySession.clear();
 }
 
 export function createTodoContinuationHook(
@@ -99,6 +110,7 @@ export function createTodoContinuationHook(
   handleEvent: (input: {
     event: { type: string; properties?: Record<string, unknown> };
   }) => Promise<void>;
+  handleChatMessage: (input: { sessionID: string; agent?: string }) => void;
   handleCommandExecuteBefore: (
     input: {
       command: string;
@@ -117,11 +129,67 @@ export function createTodoContinuationHook(
     enabled: false,
     consecutiveContinuations: 0,
     pendingTimer: null,
+    pendingTimerSessionId: null,
     suppressUntil: 0,
-    orchestratorSessionId: null,
+    orchestratorSessionIds: new Set<string>(),
+    sawChatMessage: false,
     isAutoInjecting: false,
+    notifyingSessionIds: new Set<string>(),
+    notificationBusyUntilBySession: new Map<string, number>(),
   };
 
+  function markNotificationStarted(sessionID: string): void {
+    state.notifyingSessionIds.add(sessionID);
+  }
+
+  function markNotificationFinished(sessionID: string): void {
+    state.notifyingSessionIds.delete(sessionID);
+    state.notificationBusyUntilBySession.set(
+      sessionID,
+      Date.now() + NOTIFICATION_BUSY_GRACE_MS,
+    );
+  }
+
+  function clearNotificationState(sessionID: string): void {
+    state.notifyingSessionIds.delete(sessionID);
+    state.notificationBusyUntilBySession.delete(sessionID);
+  }
+
+  function isNotificationBusy(sessionID: string): boolean {
+    if (state.notifyingSessionIds.has(sessionID)) {
+      return true;
+    }
+
+    const until = state.notificationBusyUntilBySession.get(sessionID) ?? 0;
+    if (until <= Date.now()) {
+      state.notificationBusyUntilBySession.delete(sessionID);
+      return false;
+    }
+    return true;
+  }
+
+  function isOrchestratorSession(sessionID: string): boolean {
+    return state.orchestratorSessionIds.has(sessionID);
+  }
+
+  function registerOrchestratorSession(sessionID: string): void {
+    state.orchestratorSessionIds.add(sessionID);
+  }
+
+  function handleChatMessage(input: {
+    sessionID: string;
+    agent?: string;
+  }): void {
+    if (!input.agent) {
+      return;
+    }
+
+    state.sawChatMessage = true;
+    if (input.agent === 'orchestrator') {
+      registerOrchestratorSession(input.sessionID);
+    }
+  }
+
   const autoContinue = tool({
     description:
       'Toggle auto-continuation for incomplete todos. When enabled, the orchestrator will automatically continue working through its todo list when it stops with incomplete items.',
@@ -150,7 +218,11 @@ export function createTodoContinuationHook(
     const { event } = input;
     const properties = event.properties ?? {};
 
-    if (event.type === 'session.idle') {
+    if (
+      event.type === 'session.idle' ||
+      (event.type === 'session.status' &&
+        (properties.status as { type?: string } | undefined)?.type === 'idle')
+    ) {
       const sessionID = properties.sessionID as string;
       if (!sessionID) {
         return;
@@ -158,17 +230,17 @@ export function createTodoContinuationHook(
 
       log(`[${HOOK_NAME}] Session idle`, { sessionID });
 
-      // Track orchestrator session (assumes orchestrator is the first
-      // session to go idle — correct for single-session main chat)
-      if (!state.orchestratorSessionId) {
-        state.orchestratorSessionId = sessionID;
+      // Backward compatibility: if no chat.message has identified the
+      // orchestrator yet, fall back to the first idle session.
+      if (!state.sawChatMessage && state.orchestratorSessionIds.size === 0) {
+        registerOrchestratorSession(sessionID);
         log(`[${HOOK_NAME}] Tracked orchestrator session`, {
           sessionID,
         });
       }
 
       // Gate: session is orchestrator (needed before auto-enable check)
-      if (state.orchestratorSessionId !== sessionID) {
+      if (!isOrchestratorSession(sessionID)) {
         log(`[${HOOK_NAME}] Skipped: not orchestrator session`, {
           sessionID,
         });
@@ -320,6 +392,7 @@ export function createTodoContinuationHook(
       });
 
       // Show countdown notification (noReply = agent doesn't respond)
+      markNotificationStarted(sessionID);
       ctx.client.session
         .prompt({
           path: { id: sessionID },
@@ -339,10 +412,16 @@ export function createTodoContinuationHook(
         })
         .catch(() => {
           /* best-effort notification */
+        })
+        .finally(() => {
+          markNotificationFinished(sessionID);
         });
 
+      state.pendingTimerSessionId = sessionID;
       state.pendingTimer = setTimeout(async () => {
         state.pendingTimer = null;
+        state.pendingTimerSessionId = null;
+        clearNotificationState(sessionID);
 
         // Guard: may have been disabled during cooldown
         if (!state.enabled) {
@@ -378,11 +457,16 @@ export function createTodoContinuationHook(
       const status = properties.status as { type: string };
       const sessionID = properties.sessionID as string;
       if (status?.type === 'busy') {
-        const isOrchestrator = sessionID === state.orchestratorSessionId;
+        const isOrchestrator = isOrchestratorSession(sessionID);
+        const isNotification = isNotificationBusy(sessionID);
 
         // Only cancel timer for orchestrator session — sub-agents going
         // busy must not silently kill the orchestrator's continuation.
-        if (isOrchestrator) {
+        if (
+          isOrchestrator &&
+          !isNotification &&
+          state.pendingTimerSessionId === sessionID
+        ) {
           cancelPendingTimer(state);
         }
 
@@ -390,6 +474,7 @@ export function createTodoContinuationHook(
         // not for our own auto-injection prompt. Scope to orchestrator only.
         if (
           !state.isAutoInjecting &&
+          !isNotification &&
           isOrchestrator &&
           state.consecutiveContinuations > 0
         ) {
@@ -403,7 +488,7 @@ export function createTodoContinuationHook(
       const error = properties.error as { name?: string };
       const sessionID = properties.sessionID as string;
       const errorName = error?.name;
-      const isOrchestrator = sessionID === state.orchestratorSessionId;
+      const isOrchestrator = isOrchestratorSession(sessionID);
       if (
         isOrchestrator &&
         (errorName === 'MessageAbortedError' || errorName === 'AbortError')
@@ -427,16 +512,20 @@ export function createTodoContinuationHook(
         (properties.info as { id?: string })?.id ??
         (properties.sessionID as string);
 
-      // Only cancel timer if the orchestrator session itself was deleted.
-      // Background sub-agent deletion must not kill the orchestrator's timer.
-      if (state.orchestratorSessionId === deletedSessionId) {
-        cancelPendingTimer(state);
-        log(`[${HOOK_NAME}] Cancelled pending timer on orchestrator delete`, {
-          sessionID: deletedSessionId,
-        });
+      if (deletedSessionId && isOrchestratorSession(deletedSessionId)) {
+        if (state.pendingTimerSessionId === deletedSessionId) {
+          cancelPendingTimer(state);
+          log(`[${HOOK_NAME}] Cancelled pending timer on orchestrator delete`, {
+            sessionID: deletedSessionId,
+          });
+        }
 
-        resetState(state);
-        state.orchestratorSessionId = null;
+        state.orchestratorSessionIds.delete(deletedSessionId);
+        clearNotificationState(deletedSessionId);
+        if (state.orchestratorSessionIds.size === 0) {
+          resetState(state);
+          state.sawChatMessage = false;
+        }
         log(`[${HOOK_NAME}] Reset orchestrator session on delete`, {
           sessionID: deletedSessionId,
         });
@@ -458,9 +547,7 @@ export function createTodoContinuationHook(
 
     // Seed orchestrator session from slash command (more reliable than
     // first-idle heuristic — slash commands only fire in main chat)
-    if (!state.orchestratorSessionId) {
-      state.orchestratorSessionId = input.sessionID;
-    }
+    registerOrchestratorSession(input.sessionID);
 
     // Clear template text — hook handles everything directly
     output.parts.length = 0;
@@ -533,6 +620,7 @@ export function createTodoContinuationHook(
   return {
     tool: { auto_continue: autoContinue },
     handleEvent,
+    handleChatMessage,
     handleCommandExecuteBefore,
   };
 }

+ 11 - 3
src/index.ts

@@ -502,10 +502,18 @@ const OhMyOpenCodeLite: Plugin = async (ctx) => {
     'chat.headers': chatHeadersHook['chat.headers'],
 
     // Track which agent each session uses (needed for serve-mode prompt injection)
-    'chat.message': async (input: { sessionID: string; agent?: string }) => {
-      if (input.agent) {
-        sessionAgentMap.set(input.sessionID, input.agent);
+    'chat.message': async (
+      input: { sessionID: string; agent?: string },
+      output?: { message?: { agent?: string } },
+    ) => {
+      const agent = input.agent ?? output?.message?.agent;
+      if (agent) {
+        sessionAgentMap.set(input.sessionID, agent);
       }
+      todoContinuationHook.handleChatMessage({
+        sessionID: input.sessionID,
+        agent,
+      });
     },
 
     // Inject orchestrator system prompt for serve-mode sessions.