Browse Source

feat(background-job-board): add intent-revealing query methods

Add 7 query methods that hide field-reading patterns from callers:
- isRunning(taskID)
- isReusable(taskID)
- wasCancellationRequested(taskID)
- isTerminalUnreconciled(taskID)
- getAlias(taskID)
- getResultSummary(taskID)
- getLastLiveBusyAt(taskID)

Migrate 3 production consumers (task-session-manager, cancel-task,
multiplexer/session-manager) to use the new methods instead of reading
BackgroundJobRecord fields directly.

Reduces observation coupling — callers no longer depend on the 35-field
record layout for decision-making. Logging that reads multiple fields
for debugging is preserved.

Refs #648
Michael Henke 1 month ago
parent
commit
4d30fca52c

+ 28 - 12
src/hooks/task-session-manager/index.ts

@@ -236,7 +236,7 @@ export function createTaskSessionManagerHook(
       log('[task-session-manager] suppressed late cancelled task error', {
         taskID: status.taskID,
         alias: existing?.alias,
-        state: existing?.state,
+        state: status.state,
         terminalState: existing?.terminalState,
         result: status.result,
       });
@@ -311,12 +311,12 @@ export function createTaskSessionManagerHook(
     if (isFailed && isLateCancelledTaskError(existing, status.state)) {
       part.text = formatCancelledTaskStatusOutput(
         status.taskID,
-        existing?.resultSummary,
+        backgroundJobBoard.getResultSummary(status.taskID),
       );
       log('[task-session-manager] normalized late cancelled injected failure', {
         taskID: status.taskID,
         alias: existing?.alias,
-        state: existing?.state,
+        state: status.state,
         terminalState: existing?.terminalState,
         result: status.result,
       });
@@ -764,15 +764,20 @@ export function createTaskSessionManagerHook(
         const updated = sessionId
           ? backgroundJobBoard.markRunningFromLiveSession(sessionId)
           : undefined;
-        if (before?.cancellationRequested) {
+        if (
+          before &&
+          sessionId &&
+          backgroundJobBoard.wasCancellationRequested(sessionId)
+        ) {
           log('[task-session-manager] busy observed after cancel request', {
             sessionID: sessionId,
             previousState: before.state,
             previousTerminalState: before.terminalState,
-            terminalUnreconciled: before.terminalUnreconciled,
-            resultSummary: before.resultSummary,
-            updatedState: updated?.state,
-            updatedCancellationRequested: updated?.cancellationRequested,
+            terminalUnreconciled:
+              backgroundJobBoard.isTerminalUnreconciled(sessionId),
+            resultSummary:
+              backgroundJobBoard.getResultSummary(sessionId) ??
+              before.resultSummary,
           });
         }
         log('[task-session-manager] busy/status busy observed', {
@@ -782,11 +787,21 @@ export function createTaskSessionManagerHook(
             : false,
           previousState: before?.state,
           previousTerminalState: before?.terminalState,
-          previousCancellationRequested: before?.cancellationRequested,
-          previousLastLiveBusyAt: before?.lastLiveBusyAt,
+          previousCancellationRequested: sessionId
+            ? backgroundJobBoard.wasCancellationRequested(sessionId)
+            : false,
+          previousLastLiveBusyAt: sessionId
+            ? (backgroundJobBoard.getLastLiveBusyAt(sessionId) ??
+              before?.lastLiveBusyAt)
+            : undefined,
           updatedState: updated?.state,
-          updatedCancellationRequested: updated?.cancellationRequested,
-          updatedLastLiveBusyAt: updated?.lastLiveBusyAt,
+          updatedCancellationRequested: sessionId
+            ? backgroundJobBoard.wasCancellationRequested(sessionId)
+            : false,
+          updatedLastLiveBusyAt: sessionId
+            ? (backgroundJobBoard.getLastLiveBusyAt(sessionId) ??
+              updated?.lastLiveBusyAt)
+            : undefined,
         });
         return;
       }
@@ -859,6 +874,7 @@ function isLateCancelledTaskError(
   job: BackgroundJobRecord | undefined,
   state: string,
 ): boolean {
+  // ponytail: kept as-is per spec - uses multiple fields for complex check
   if (state !== 'error') return false;
   if (!job?.cancellationRequested) return false;
   return job.state === 'cancelled' || job.terminalState === 'cancelled';

+ 1 - 1
src/multiplexer/session-manager.ts

@@ -607,7 +607,7 @@ export class MultiplexerSessionManager {
   }
 
   private isRunningBackgroundJob(sessionId: string): boolean {
-    return this.backgroundJobBoard?.get(sessionId)?.state === 'running';
+    return this.backgroundJobBoard?.isRunning(sessionId) ?? false; // ponytail: intent-revealing query
   }
 
   async retryDeferredIdleClose(sessionId: string): Promise<void> {

+ 18 - 10
src/tools/cancel-task.ts

@@ -61,10 +61,14 @@ Use only for obsolete, wrong, conflicting, or user-requested cancellation. Accep
         parentSessionID,
         requested,
         resolvedTaskID: job?.taskID,
-        alias: job?.alias,
+        alias: job?.taskID
+          ? options.backgroundJobBoard.getAlias(job.taskID)
+          : undefined,
         state: job?.state,
         terminalState: job?.terminalState,
-        cancellationRequested: job?.cancellationRequested,
+        cancellationRequested: job?.taskID
+          ? options.backgroundJobBoard.wasCancellationRequested(job.taskID)
+          : undefined,
       });
       if (!job) {
         if (isSessionID(requested)) {
@@ -118,7 +122,9 @@ Use only for obsolete, wrong, conflicting, or user-requested cancellation. Accep
       try {
         await abortAndVerifySession(options, job.taskID);
       } catch (error) {
-        const stillRunning = error instanceof SessionStillRunningError;
+        const stillRunning =
+          error instanceof SessionStillRunningError ||
+          options.backgroundJobBoard.isRunning(job.taskID); // ponytail: intent-revealing query
         log('[cancel-task] abort failed', {
           taskID: job.taskID,
           stillRunning,
@@ -149,10 +155,11 @@ Use only for obsolete, wrong, conflicting, or user-requested cancellation. Accep
       );
       log('[cancel-task] marked job cancelled after verified abort', {
         taskID: job.taskID,
-        alias: job.alias,
+        alias: options.backgroundJobBoard.getAlias(job.taskID),
         previousState: job.state,
         state: cancelled?.state,
-        cancellationRequested: cancelled?.cancellationRequested,
+        cancellationRequested:
+          options.backgroundJobBoard.wasCancellationRequested(job.taskID),
       });
 
       return [
@@ -177,7 +184,9 @@ async function cancelSessionByID(
   try {
     await abortAndVerifySession(options, taskID);
   } catch (error) {
-    const stillRunning = error instanceof SessionStillRunningError;
+    const stillRunning =
+      error instanceof SessionStillRunningError ||
+      options.backgroundJobBoard.isRunning(taskID); // ponytail: intent-revealing query
     log('[cancel-task] raw session abort failed', {
       taskID,
       stillRunning,
@@ -257,12 +266,11 @@ async function abortAndVerifySession(
       stableStoppedForMs: stableStoppedSince
         ? Date.now() - stableStoppedSince
         : 0,
-      boardState: options.backgroundJobBoard.get(taskID)?.state,
-      boardLastLiveBusyAt:
-        options.backgroundJobBoard.get(taskID)?.lastLiveBusyAt,
+      boardState: options.backgroundJobBoard.isRunning(taskID), // ponytail: intent-revealing query
+      boardLastLiveBusyAt: options.backgroundJobBoard.getLastLiveBusyAt(taskID),
     });
     const boardLastLiveBusyAt =
-      options.backgroundJobBoard.get(taskID)?.lastLiveBusyAt;
+      options.backgroundJobBoard.getLastLiveBusyAt(taskID);
     if (boardLastLiveBusyAt && boardLastLiveBusyAt >= abortStartedAt) {
       log('[cancel-task] abort verification saw board busy after abort', {
         taskID,

+ 135 - 0
src/utils/background-job-board.test.ts

@@ -698,4 +698,139 @@ describe('BackgroundJobBoard', () => {
     const prompt = board.formatForPrompt('parent-1', 9_000);
     expect(prompt).toContain('running [resumed, 4s ago]');
   });
+
+  describe('intent-revealing query methods', () => {
+    test('isRunning: true for running jobs, false for terminal/reconciled/unknown', () => {
+      const board = new BackgroundJobBoard();
+      board.registerLaunch({
+        taskID: 'running-1',
+        parentSessionID: 'parent-1',
+        agent: 'fixer',
+        now: 100,
+      });
+      board.updateStatus({
+        taskID: 'terminal-1',
+        state: 'completed',
+        now: 200,
+      });
+      board.markReconciled('terminal-1', 300);
+
+      expect(board.isRunning('running-1')).toBe(true);
+      expect(board.isRunning('terminal-1')).toBe(false);
+      expect(board.isRunning('unknown-1')).toBe(false);
+    });
+
+    test('isReusable: true only for completed + reconciled', () => {
+      const board = new BackgroundJobBoard();
+      board.registerLaunch({
+        taskID: 'running-1',
+        parentSessionID: 'parent-1',
+        agent: 'fixer',
+        now: 100,
+      });
+      board.registerLaunch({
+        taskID: 'completed-1',
+        parentSessionID: 'parent-1',
+        agent: 'fixer',
+        now: 100,
+      });
+      board.updateStatus({
+        taskID: 'completed-1',
+        state: 'completed',
+        now: 200,
+      });
+
+      // After updateStatus to completed, job is terminalUnreconciled, so not reusable
+      expect(board.isReusable('running-1')).toBe(false);
+      expect(board.isReusable('completed-1')).toBe(false);
+
+      // markReconciled makes it reusable by clearing terminalUnreconciled
+      const reconciled = board.markReconciled('completed-1', 300);
+      expect(reconciled).toBeDefined();
+      expect(reconciled?.state).toBe('reconciled');
+      expect(reconciled?.terminalUnreconciled).toBe(false);
+      // After reconciliation, the job should be reusable (completed + reconciled)
+      expect(board.isReusable('completed-1')).toBe(true);
+      expect(board.isReusable('unknown-1')).toBe(false);
+    });
+
+    test('wasCancellationRequested: true after markCancelled', () => {
+      const board = new BackgroundJobBoard();
+      board.registerLaunch({
+        taskID: 'job-1',
+        parentSessionID: 'parent-1',
+        agent: 'fixer',
+        now: 100,
+      });
+
+      expect(board.wasCancellationRequested('job-1')).toBe(false);
+      board.markCancelled('job-1', 'user requested', 200);
+      expect(board.wasCancellationRequested('job-1')).toBe(true);
+      expect(board.wasCancellationRequested('unknown-1')).toBe(false);
+    });
+
+    test('isTerminalUnreconciled: true after updateStatus to terminal, false after markReconciled', () => {
+      const board = new BackgroundJobBoard();
+      board.registerLaunch({
+        taskID: 'job-1',
+        parentSessionID: 'parent-1',
+        agent: 'fixer',
+        now: 100,
+      });
+
+      expect(board.isTerminalUnreconciled('job-1')).toBe(false);
+      board.updateStatus({ taskID: 'job-1', state: 'completed', now: 200 });
+      expect(board.isTerminalUnreconciled('job-1')).toBe(true);
+      board.markReconciled('job-1', 300);
+      expect(board.isTerminalUnreconciled('job-1')).toBe(false);
+      expect(board.isTerminalUnreconciled('unknown-1')).toBe(false);
+    });
+
+    test('getAlias: returns alias after registerLaunch', () => {
+      const board = new BackgroundJobBoard();
+      board.registerLaunch({
+        taskID: 'job-1',
+        parentSessionID: 'parent-1',
+        agent: 'fixer',
+        now: 100,
+      });
+
+      expect(board.getAlias('job-1')).toBe('fix-1');
+      expect(board.getAlias('unknown-1')).toBeUndefined();
+    });
+
+    test('getResultSummary: returns summary after updateStatus with result', () => {
+      const board = new BackgroundJobBoard();
+      board.registerLaunch({
+        taskID: 'job-1',
+        parentSessionID: 'parent-1',
+        agent: 'fixer',
+        now: 100,
+      });
+      board.updateStatus({
+        taskID: 'job-1',
+        state: 'completed',
+        resultSummary: 'all good',
+        now: 200,
+      });
+
+      expect(board.getResultSummary('job-1')).toBe('all good');
+      expect(board.getResultSummary('unknown-1')).toBeUndefined();
+    });
+
+    test('getLastLiveBusyAt: returns timestamp after markRunningFromLiveSession', () => {
+      const board = new BackgroundJobBoard();
+      board.registerLaunch({
+        taskID: 'job-1',
+        parentSessionID: 'parent-1',
+        agent: 'fixer',
+        now: 100,
+      });
+
+      expect(board.getLastLiveBusyAt('job-1')).toBe(100);
+      board.markRunningFromLiveSession('job-1', 200);
+      expect(board.getLastLiveBusyAt('job-1')).toBe(200);
+      expect(board.getLastLiveBusyAt('unknown-1')).toBeUndefined();
+    });
+  });
 });

+ 59 - 0
src/utils/background-job-board.ts

@@ -293,6 +293,65 @@ export class BackgroundJobBoard {
     return this.jobs.get(taskID);
   }
 
+  /**
+   * True if the job exists and is in 'running' state.
+   */
+  isRunning(taskID: string): boolean {
+    const job = this.get(taskID);
+    return job?.state === 'running';
+  }
+
+  /**
+   * True if the job is terminal (completed/error/cancelled) and reconciled.
+   */
+  isReusable(taskID: string): boolean {
+    const job = this.get(taskID);
+    if (!job) return false;
+    // ponytail: inline the logic to avoid confusion with private trimReusable
+    const terminal = job.terminalState ?? terminalStateOf(job.state);
+    return terminal === 'completed' && !job.terminalUnreconciled;
+  }
+
+  /**
+   * True if cancellation was requested for this job.
+   */
+  wasCancellationRequested(taskID: string): boolean {
+    const job = this.get(taskID);
+    return !!job?.cancellationRequested;
+  }
+
+  /**
+   * True if the job is terminal but not yet reconciled.
+   */
+  isTerminalUnreconciled(taskID: string): boolean {
+    const job = this.get(taskID);
+    return !!job?.terminalUnreconciled;
+  }
+
+  /**
+   * Get the alias for a job, or undefined if not found.
+   */
+  getAlias(taskID: string): string | undefined {
+    const job = this.get(taskID);
+    return job?.alias;
+  }
+
+  /**
+   * Get the result summary for a terminal job, or undefined.
+   */
+  getResultSummary(taskID: string): string | undefined {
+    const job = this.get(taskID);
+    return job?.resultSummary;
+  }
+
+  /**
+   * Get the last live busy timestamp, or undefined.
+   */
+  getLastLiveBusyAt(taskID: string): number | undefined {
+    const job = this.get(taskID);
+    return job?.lastLiveBusyAt;
+  }
+
   resolve(
     parentSessionID: string,
     taskIDOrAlias: string,