Procházet zdrojové kódy

fix(task-session-manager): identity-verified takes for hosts without tool call IDs

Closes the known limitation flagged in the #1162 review thread: take()
without a call ID guessed by insertion order among parallel calls,
which could mis-attribute the description and even overwrite an
already-correct early-registered record (title-claimed label) with the
wrong pending's label.

- take() without a call ID now only proceeds when exactly one pending
  exists for the parent (sound: after-hooks fire once per call, so the
  sole survivor belongs to this call);
- new takeByTaskID(parent, taskID, ownerBoard?) removes and returns the
  pending whose early registration claimed that task ID — identity
  verified through the title+agent-matched claim chain — honoring the
  same board-generation fence as take();
- tool.execute.after falls back to takeByTaskID using the task ID
  parsed from its own output when no (known) call ID is available, so
  swapped after-hook ordering on legacy hosts can no longer corrupt
  descriptions.
GoldJohnKing před 5 dny
rodič
revize
67a581bb5d

+ 1 - 1
src/hooks/task-session-manager/codemap.md

@@ -63,7 +63,7 @@ All modules depend on `BackgroundJobBoard` from `src/utils/background-job-board.
     - The idle timer remains a backstop for when the model ends its turn without further requests; after reconciling injected terminal results, the opt-in continuation evaluator can run in the same idle cycle under its existing guards
 
 5. **Lifecycle Events (`event`)**
-    - `session.created`: Adds new task IDs to pending managed set. Early board registration claims a pending call only when it can be identified unambiguously: a unique child-session `title` match (the v2 host stamps `title = description` argument, additionally constrained to the child's agent) or a unique agent-type match among unmarked pendings **with no already-consumed same-agent call** — a no-title child arriving after a same-agent call's after-hook consumed its pending is treated as stale and never claims. Ambiguous, stale, or unattributable children get a placeholder `unattributed <agent> task` registration so task_status always resolves them; the owning `tool.execute.after` corrects the description. An already-registered child never fences a pending, and a pending's flags never cause `tool.execute.after` to drop the task ID parsed from its own output.
+    - `session.created`: Adds new task IDs to pending managed set. Early board registration claims a pending call only when it can be identified unambiguously: a unique child-session `title` match (the v2 host stamps `title = description` argument, additionally constrained to the child's agent) or a unique agent-type match among unmarked pendings **with no already-consumed same-agent call** — a no-title child arriving after a same-agent call's after-hook consumed its pending is treated as stale and never claims. Ambiguous, stale, or unattributable children get a placeholder `unattributed <agent> task` registration so task_status always resolves them; the owning `tool.execute.after` corrects the description. An already-registered child never fences a pending, and a pending's flags never cause `tool.execute.after` to drop the task ID parsed from its own output. On hosts that do not supply tool call IDs, `tool.execute.after` resolves identity via `takeByTaskID` — matching the task ID parsed from its own output against the pending the early registration claimed for that child — instead of guessing by insertion order among parallel calls; `take()` without a call ID only proceeds when exactly one pending exists for the parent.
     - `session.idle` / `session.status` (idle): Reconciles injected terminal jobs for the parent session (backstop path), then can run the opt-in continuation evaluator in the same idle cycle under its existing guards. Child idle is a stop candidate: the first observation stays provisional, and only a confirmed idle/absent after the 5s grace marks `stopped`
     - `session.status` (busy): Marks sessions as running from live session state and resets pending stop confirmation
     - `session.deleted`: Clears job state, child jobs, and pending call records for the session

+ 63 - 0
src/hooks/task-session-manager/parallel-same-agent-pairing.test.ts

@@ -196,4 +196,67 @@ describe('parallel same-agent pairing (incident 2026-09-12)', () => {
     // the stale child keeps the honest placeholder label, never B's
     expect(board.get(sX)?.description).toBe('unattributed oracle task');
   });
+
+  test('no-callID hosts: swapped after-hooks cannot corrupt descriptions', async () => {
+    const board = new BackgroundJobBoard();
+    const hook = createHook(board);
+    const sA = 'ses_aaaa1111';
+    const sB = 'ses_bbbb2222';
+    const v1Launch = (taskID: string) =>
+      [
+        `task_id: ${taskID}`,
+        'state: running',
+        '',
+        '<task_result>',
+        'Background task started.',
+        '</task_result>',
+      ].join('\n');
+
+    // two parallel calls WITHOUT callIDs (v1 legacy hosts): the before
+    // hook assigns anonymous pending IDs in insertion order
+    await hook['tool.execute.before'](
+      { tool: 'task', sessionID: PARENT },
+      {
+        args: {
+          subagent_type: 'oracle',
+          description: L_A,
+          prompt: 'do the review',
+          background: true,
+        },
+      },
+    );
+    await hook['tool.execute.before'](
+      { tool: 'task', sessionID: PARENT },
+      {
+        args: {
+          subagent_type: 'oracle',
+          description: L_B,
+          prompt: 'do the review',
+          background: true,
+        },
+      },
+    );
+
+    // created-first: titles claim the right pendings
+    await hook.event(created({ child: sA, title: L_A }));
+    await hook.event(created({ child: sB, title: L_B }));
+    expect(board.get(sA)?.description).toBe(L_A);
+    expect(board.get(sB)?.description).toBe(L_B);
+
+    // after-hooks fire in SWAPPED order with no callIDs: the oldest
+    // pending is A's, but this output belongs to call B — the oldest
+    // guess must neither drop sB nor overwrite its correct label
+    await hook['tool.execute.after'](
+      { tool: 'task', sessionID: PARENT },
+      { output: v1Launch(sB) },
+    );
+    await hook['tool.execute.after'](
+      { tool: 'task', sessionID: PARENT },
+      { output: v1Launch(sA) },
+    );
+
+    expect(board.taskIDs()).toEqual(new Set([sA, sB]));
+    expect(board.get(sA)?.description).toBe(L_A);
+    expect(board.get(sB)?.description).toBe(L_B);
+  });
 });

+ 43 - 0
src/hooks/task-session-manager/pending-call-tracker.test.ts

@@ -130,3 +130,46 @@ describe('peekByParentAndAgent', () => {
     expect(hit?.callId).toBe('b');
   });
 });
+
+describe('take', () => {
+  test('without callID, refuses when multiple pendings match the parent', () => {
+    const tracker = createPendingCallTracker();
+    tracker.add(pending({ callId: 'a' }));
+    tracker.add(pending({ callId: 'b' }));
+
+    expect(tracker.take(undefined, 'parent-1')).toBeUndefined();
+    // Nothing was consumed by the refused take.
+    expect(tracker.hasConsumedCall('parent-1')).toBe(false);
+  });
+
+  test('without callID, takes the sole pending for the parent', () => {
+    const tracker = createPendingCallTracker();
+    tracker.add(pending({ callId: 'a' }));
+
+    const taken = tracker.take(undefined, 'parent-1');
+
+    expect(taken?.callId).toBe('a');
+  });
+});
+
+describe('takeByTaskID', () => {
+  test('removes and returns the pending claimed for that task ID', () => {
+    const tracker = createPendingCallTracker();
+    tracker.add(pending({ callId: 'a', earlyRegisteredTaskID: 'ses_x' }));
+    tracker.add(pending({ callId: 'b' }));
+
+    const taken = tracker.takeByTaskID('parent-1', 'ses_x');
+
+    expect(taken?.callId).toBe('a');
+    // The other pending is untouched.
+    expect(tracker.take('b')?.callId).toBe('b');
+  });
+
+  test('returns undefined when no pending is claimed for the task ID', () => {
+    const tracker = createPendingCallTracker();
+    tracker.add(pending({ callId: 'a', earlyRegisteredTaskID: 'ses_x' }));
+
+    expect(tracker.takeByTaskID('parent-1', 'ses_y')).toBeUndefined();
+    expect(tracker.take('a')?.callId).toBe('a');
+  });
+});

+ 57 - 7
src/hooks/task-session-manager/pending-call-tracker.ts

@@ -41,6 +41,16 @@ export interface PendingCallTracker {
     ownerBoard?: BackgroundJobStore,
     options?: { recordConsumed?: boolean },
   ): PendingTaskCall | undefined;
+  /** Remove and return the pending call whose early registration claimed
+   * `taskID` for this parent — an identity-verified take for hosts that
+   * do not supply tool call IDs. When `ownerBoard` is given and the
+   * early registration was adopted by a different board generation, the
+   * pending is left for that generation (same fence as `take`). */
+  takeByTaskID(
+    parentSessionId: string,
+    taskID: string,
+    ownerBoard?: BackgroundJobStore,
+  ): PendingTaskCall | undefined;
   release(call: PendingTaskCall): void;
   peekByParent(parentSessionId: string): PendingTaskCall | undefined;
   peekByParentAndAgent(
@@ -103,6 +113,18 @@ export function createPendingCallTracker(
     return false;
   };
 
+  const solePendingIdForParent = (
+    parentSessionId: string,
+  ): string | undefined => {
+    let found: string | undefined;
+    for (const [callId, call] of pendingCalls.entries()) {
+      if (call.parentSessionId !== parentSessionId) continue;
+      if (found !== undefined) return undefined;
+      found = callId;
+    }
+    return found;
+  };
+
   const releaseCallLease = (call: PendingTaskCall): void => {
     if (call.relaunchLease) {
       (call.releaseLease ?? options.releaseLease)?.(call.relaunchLease);
@@ -132,13 +154,15 @@ export function createPendingCallTracker(
       takeOptions?: { recordConsumed?: boolean },
     ) {
       if (!callId && parentSessionId) {
-        for (const id of pendingCalls.keys()) {
-          const call = pendingCalls.get(id);
-          if (call && call.parentSessionId === parentSessionId) {
-            callId = id;
-            break;
-          }
-        }
+        // Without a tool call ID a take can only be sound when exactly
+        // one pending exists for the parent (after-hooks fire once per
+        // call, so a sole survivor belongs to this call). With several
+        // candidates, guessing by insertion order would mis-attribute
+        // the label and could overwrite an already-correct record —
+        // refuse and let the caller resolve identity via takeByTaskID.
+        const sole = solePendingIdForParent(parentSessionId);
+        if (!sole) return undefined;
+        callId = sole;
       }
       if (!callId) return undefined;
       const pending = pendingCalls.get(callId);
@@ -237,6 +261,32 @@ export function createPendingCallTracker(
       return hasConsumedFor(parentSessionId, agentType);
     },
 
+    takeByTaskID(
+      parentSessionId: string,
+      taskID: string,
+      ownerBoard?: BackgroundJobStore,
+    ) {
+      for (const [callId, call] of pendingCalls.entries()) {
+        if (
+          call.parentSessionId !== parentSessionId ||
+          call.earlyRegisteredTaskID !== taskID
+        ) {
+          continue;
+        }
+        if (
+          call.earlyRegistration &&
+          ownerBoard &&
+          call.earlyRegistration.backgroundJobBoard !== ownerBoard
+        ) {
+          return undefined;
+        }
+        pendingCalls.delete(callId);
+        recordConsumed(call);
+        return call;
+      }
+      return undefined;
+    },
+
     adoptEarlyRegistrations(
       backgroundJobBoard: BackgroundJobStore,
       backgroundJobSupervisor?: BackgroundJobSupervisor,

+ 26 - 1
src/hooks/task-session-manager/tool-execute-hooks.ts

@@ -288,6 +288,11 @@ export async function handleToolExecuteAfter(
         ownerBoard?: BackgroundJobStore,
         options?: { recordConsumed?: boolean },
       ): PendingTaskCall | undefined;
+      takeByTaskID(
+        sessionID: string,
+        taskID: string,
+        ownerBoard?: BackgroundJobStore,
+      ): PendingTaskCall | undefined;
       release?(call: PendingTaskCall): void;
     };
     taskContextTracker: {
@@ -330,13 +335,33 @@ export async function handleToolExecuteAfter(
     typeof input.callID === 'string' && input.callID.trim() !== ''
       ? input.callID
       : undefined;
-  const pending = deps.pendingCallTracker.take(
+  let pending = deps.pendingCallTracker.take(
     exactCallID,
     exactCallID ? undefined : input.sessionID,
     deps.backgroundJobBoard,
   );
   const exactCallConfirmed =
     exactCallID !== undefined && pending?.callId === exactCallID;
+  if (!pending && typeof output.output === 'string') {
+    // No tool call ID (or unknown one): resolve identity via the task
+    // ID parsed from this call's own output, matched against the
+    // pending the early registration claimed for that child. This
+    // avoids guessing by insertion order among parallel calls.
+    const identityTaskID = parseTaskIdFromTaskOutput(output.output);
+    if (identityTaskID && input.sessionID) {
+      pending = deps.pendingCallTracker.takeByTaskID(
+        input.sessionID,
+        identityTaskID,
+        deps.backgroundJobBoard,
+      );
+      if (pending) {
+        log(
+          '[task-session-manager] resolved task output identity via early-registered task ID',
+          { taskID: identityTaskID, callID: pending.callId },
+        );
+      }
+    }
+  }
   log('[task-session-manager] tool.execute.after task', {
     callID: input.callID,
     sessionID: input.sessionID,