Browse Source

refactor: ponytail cleanup of background-job-board query methods

Deleted 6 YAGNI query methods (wasCancellationRequested, isReusable class,
getAlias, getTerminalState, isTimedOut, isStatusUncertain) that were only
used in logging where get()?.field was fine. Kept 7 methods with actual
decision logic or output construction usage.

Added field<K> generic helper, simplified getters, extracted locals,
reverted log blocks to cached properties, extracted backgroundJobState()
helper for 6 identical swaps in session-manager.

Net: -180 lines. All 1215 tests pass, typecheck clean.
Michael Henke 1 month ago
parent
commit
95ff938c8d

+ 19 - 19
src/hooks/task-session-manager/index.ts

@@ -260,13 +260,11 @@ export function createTaskSessionManagerHook(
 
     log('[task-session-manager] background job status updated', {
       taskID: updated.taskID,
-      alias: backgroundJobBoard.getAlias(updated.taskID),
-      parentSessionID: backgroundJobBoard.getParentSessionID(updated.taskID),
+      alias: updated.alias,
+      parentSessionID: updated.parentSessionID,
       state: updated.state,
-      terminalUnreconciled: backgroundJobBoard.isTerminalUnreconciled(
-        updated.taskID,
-      ),
-      timedOut: backgroundJobBoard.isTimedOut(updated.taskID),
+      terminalUnreconciled: updated.terminalUnreconciled,
+      timedOut: updated.timedOut,
     });
 
     if (backgroundJobBoard.isTerminalUnreconciled(updated.taskID)) {
@@ -343,8 +341,8 @@ export function createTaskSessionManagerHook(
 
     log('[task-session-manager] processed injected background completion', {
       taskID: updated.taskID,
-      alias: backgroundJobBoard.getAlias(updated.taskID),
-      parentSessionID: backgroundJobBoard.getParentSessionID(updated.taskID),
+      alias: updated.alias,
+      parentSessionID: updated.parentSessionID,
       state: updated.state,
       occurrenceId,
     });
@@ -802,14 +800,16 @@ export function createTaskSessionManagerHook(
         '[task-session-manager] session.deleted observed; clearing job state',
         {
           sessionID: sessionId,
-          deletedJob: backgroundJobBoard.get(sessionId)
-            ? {
-                state: backgroundJobBoard.getState(sessionId),
-                parentSessionID:
-                  backgroundJobBoard.getParentSessionID(sessionId),
-                alias: backgroundJobBoard.getAlias(sessionId),
-              }
-            : undefined,
+          deletedJob: (() => {
+            const record = backgroundJobBoard.get(sessionId);
+            return record
+              ? {
+                  state: record.state,
+                  parentSessionID: record.parentSessionID,
+                  alias: record.alias,
+                }
+              : undefined;
+          })(),
           childJobCount: backgroundJobBoard.list(sessionId).length,
           managesSession: options.shouldManageSession(sessionId),
         },
@@ -842,9 +842,9 @@ export function createTaskSessionManagerHook(
     if (!isLateCancelledTaskError(existing, status.state)) return;
     log('[task-session-manager] normalized late cancelled task output', {
       taskID: status.taskID,
-      alias: backgroundJobBoard.getAlias(status.taskID),
-      state: backgroundJobBoard.getState(status.taskID),
-      terminalState: backgroundJobBoard.getTerminalState(status.taskID),
+      alias: existing?.alias,
+      state: existing?.state,
+      terminalState: existing?.terminalState,
       result: status.result,
     });
     output.output = formatCancelledTaskStatusOutput(

+ 16 - 7
src/multiplexer/session-manager.ts

@@ -6,7 +6,10 @@ import {
   isServerRunning,
   type Multiplexer,
 } from '../multiplexer';
-import type { BackgroundJobBoard } from '../utils/background-job-board';
+import type {
+  BackgroundJobBoard,
+  BackgroundJobState,
+} from '../utils/background-job-board';
 import { log } from '../utils/logger';
 
 interface TrackedSession {
@@ -252,7 +255,7 @@ export class MultiplexerSessionManager {
         tracked: this.sessions.has(sessionId),
         known: this.knownSessions.has(sessionId),
         ownerInstanceId: this.sessions.get(sessionId)?.ownerInstanceId,
-        backgroundJobState: this.backgroundJobBoard?.getState(sessionId),
+        backgroundJobState: this.backgroundJobState(sessionId),
       });
 
       await this.closeSession(sessionId, 'idle');
@@ -273,7 +276,7 @@ export class MultiplexerSessionManager {
         tracked: this.sessions.has(sessionId),
         known: this.knownSessions.has(sessionId),
         ownerInstanceId: this.sessions.get(sessionId)?.ownerInstanceId,
-        backgroundJobState: this.backgroundJobBoard?.getState(sessionId),
+        backgroundJobState: this.backgroundJobState(sessionId),
       });
       await this.closeSession(sessionId, 'idle');
       return;
@@ -290,7 +293,7 @@ export class MultiplexerSessionManager {
         tracked: this.sessions.has(sessionId),
         known: this.knownSessions.has(sessionId),
         ownerInstanceId: this.sessions.get(sessionId)?.ownerInstanceId,
-        backgroundJobState: this.backgroundJobBoard?.getState(sessionId),
+        backgroundJobState: this.backgroundJobState(sessionId),
       });
       await this.respawnIfKnown(sessionId);
     }
@@ -309,7 +312,7 @@ export class MultiplexerSessionManager {
       tracked: this.sessions.has(sessionId),
       known: this.knownSessions.has(sessionId),
       ownerInstanceId: this.sessions.get(sessionId)?.ownerInstanceId,
-      backgroundJobState: this.backgroundJobBoard?.getState(sessionId),
+      backgroundJobState: this.backgroundJobState(sessionId),
     });
 
     this.deferredIdleCloses.delete(sessionId);
@@ -456,7 +459,7 @@ export class MultiplexerSessionManager {
           sessionId,
           paneId: tracked.paneId,
           reason,
-          backgroundJobState: this.backgroundJobBoard?.getState(sessionId),
+          backgroundJobState: this.backgroundJobState(sessionId),
         },
       );
       return;
@@ -470,7 +473,7 @@ export class MultiplexerSessionManager {
       sessionId,
       paneId: tracked.paneId,
       reason,
-      backgroundJobState: this.backgroundJobBoard?.getState(sessionId),
+      backgroundJobState: this.backgroundJobState(sessionId),
       parentId: tracked.parentId,
       title: tracked.title,
     });
@@ -606,6 +609,12 @@ export class MultiplexerSessionManager {
     return event.properties?.info?.id ?? event.properties?.sessionID;
   }
 
+  private backgroundJobState(
+    sessionId: string,
+  ): BackgroundJobState | undefined {
+    return this.backgroundJobBoard?.getState(sessionId);
+  }
+
   private isRunningBackgroundJob(sessionId: string): boolean {
     return this.backgroundJobBoard?.isRunning(sessionId) ?? false; // ponytail: intent-revealing query
   }

+ 16 - 22
src/tools/cancel-task.ts

@@ -61,18 +61,16 @@ Use only for obsolete, wrong, conflicting, or user-requested cancellation. Accep
         parentSessionID,
         requested,
         resolvedTaskID: job?.taskID,
-        alias: job?.taskID
-          ? options.backgroundJobBoard.getAlias(job.taskID)
+        alias: job
+          ? options.backgroundJobBoard.field(job.taskID, 'alias')
           : undefined,
-        state: job?.taskID
-          ? options.backgroundJobBoard.getState(job.taskID)
+        state: job
+          ? options.backgroundJobBoard.field(job.taskID, 'state')
           : undefined,
-        terminalState: job?.taskID
-          ? options.backgroundJobBoard.getTerminalState(job.taskID)
-          : undefined,
-        cancellationRequested: job?.taskID
-          ? options.backgroundJobBoard.wasCancellationRequested(job.taskID)
+        terminalState: job
+          ? options.backgroundJobBoard.field(job.taskID, 'terminalState')
           : undefined,
+        cancellationRequested: job?.cancellationRequested,
       });
       if (!job) {
         if (isSessionID(requested)) {
@@ -85,16 +83,13 @@ Use only for obsolete, wrong, conflicting, or user-requested cancellation. Accep
           }
 
           const knownJob = options.backgroundJobBoard.get(requested);
-          if (
-            knownJob &&
-            options.backgroundJobBoard.getParentSessionID(requested) !==
-              parentSessionID
-          ) {
+          const ownerParentSessionID =
+            options.backgroundJobBoard.getParentSessionID(requested);
+          if (knownJob && ownerParentSessionID !== parentSessionID) {
             log('[cancel-task] rejected unowned tracked raw session', {
               parentSessionID,
               taskID: requested,
-              ownerParentSessionID:
-                options.backgroundJobBoard.getParentSessionID(requested),
+              ownerParentSessionID,
             });
             return unknownTaskOutput(
               requested,
@@ -162,18 +157,17 @@ Use only for obsolete, wrong, conflicting, or user-requested cancellation. Accep
         Date.now(),
         { force: true },
       );
+      const state = options.backgroundJobBoard.getState(job.taskID);
       log('[cancel-task] marked job cancelled after verified abort', {
         taskID: job.taskID,
-        alias: options.backgroundJobBoard.getAlias(job.taskID),
-        previousState: options.backgroundJobBoard.getState(job.taskID),
-        state: options.backgroundJobBoard.getState(job.taskID),
-        cancellationRequested:
-          options.backgroundJobBoard.wasCancellationRequested(job.taskID),
+        alias: options.backgroundJobBoard.field(job.taskID, 'alias'),
+        state,
+        cancellationRequested: job.cancellationRequested,
       });
 
       return [
         `task_id: ${job.taskID}`,
-        `state: ${options.backgroundJobBoard.getState(job.taskID) ?? 'cancelled'}`,
+        `state: ${state ?? 'cancelled'}`,
         '',
         '<task_error>',
         options.backgroundJobBoard.getResultSummary(job.taskID) ?? 'cancelled',

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

@@ -720,55 +720,6 @@ describe('BackgroundJobBoard', () => {
       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({
@@ -786,19 +737,6 @@ describe('BackgroundJobBoard', () => {
       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({
@@ -845,60 +783,5 @@ describe('BackgroundJobBoard', () => {
       expect(board.getParentSessionID('job-1')).toBe('parent-1');
       expect(board.getParentSessionID('unknown-1')).toBeUndefined();
     });
-
-    test('getTerminalState: returns terminal state after updateStatus to terminal', () => {
-      const board = new BackgroundJobBoard();
-      board.registerLaunch({
-        taskID: 'job-1',
-        parentSessionID: 'parent-1',
-        agent: 'fixer',
-        now: 100,
-      });
-
-      expect(board.getTerminalState('job-1')).toBeUndefined();
-      board.updateStatus({ taskID: 'job-1', state: 'completed', now: 200 });
-      expect(board.getTerminalState('job-1')).toBe('completed');
-      expect(board.getTerminalState('unknown-1')).toBeUndefined();
-    });
-
-    test('isTimedOut: true after updateStatus with timedOut: true', () => {
-      const board = new BackgroundJobBoard();
-      board.registerLaunch({
-        taskID: 'job-1',
-        parentSessionID: 'parent-1',
-        agent: 'fixer',
-        now: 100,
-      });
-
-      expect(board.isTimedOut('job-1')).toBe(false);
-      board.updateStatus({
-        taskID: 'job-1',
-        state: 'running',
-        timedOut: true,
-        now: 200,
-      });
-      expect(board.isTimedOut('job-1')).toBe(true);
-      expect(board.isTimedOut('unknown-1')).toBe(false);
-    });
-
-    test('isStatusUncertain: true after updateStatus with statusUncertain: true', () => {
-      const board = new BackgroundJobBoard();
-      board.registerLaunch({
-        taskID: 'job-1',
-        parentSessionID: 'parent-1',
-        agent: 'fixer',
-        now: 100,
-      });
-
-      expect(board.isStatusUncertain('job-1')).toBe(false);
-      board.updateStatus({
-        taskID: 'job-1',
-        state: 'running',
-        statusUncertain: true,
-        now: 200,
-      });
-      expect(board.isStatusUncertain('job-1')).toBe(true);
-      expect(board.isStatusUncertain('unknown-1')).toBe(false);
-    });
   });
 });

+ 11 - 77
src/utils/background-job-board.ts

@@ -293,103 +293,37 @@ 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;
+  field<K extends keyof BackgroundJobRecord>(
+    taskID: string,
+    key: K,
+  ): BackgroundJobRecord[K] | undefined {
+    return this.get(taskID)?.[key];
   }
 
-  /**
-   * True if cancellation was requested for this job.
-   */
-  wasCancellationRequested(taskID: string): boolean {
+  isRunning(taskID: string): boolean {
     const job = this.get(taskID);
-    return !!job?.cancellationRequested;
+    return job?.state === 'running';
   }
 
-  /**
-   * 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;
+    return this.field(taskID, 'resultSummary');
   }
 
-  /**
-   * Get the last live busy timestamp, or undefined.
-   */
   getLastLiveBusyAt(taskID: string): number | undefined {
-    const job = this.get(taskID);
-    return job?.lastLiveBusyAt;
+    return this.field(taskID, 'lastLiveBusyAt');
   }
 
-  /**
-   * Get the parent session ID for a job, or undefined if not found.
-   */
   getParentSessionID(taskID: string): string | undefined {
-    const job = this.get(taskID);
-    return job?.parentSessionID;
+    return this.field(taskID, 'parentSessionID');
   }
 
-  /**
-   * Get the terminal state for a job, or undefined if not found or not terminal.
-   */
-  getTerminalState(taskID: string): TaskOutputState | undefined {
-    const job = this.get(taskID);
-    return job?.terminalState;
-  }
-
-  /**
-   * Get the timedOut flag for a job, or false if not found.
-   */
-  isTimedOut(taskID: string): boolean {
-    const job = this.get(taskID);
-    return !!job?.timedOut;
-  }
-
-  /**
-   * Get the statusUncertain flag for a job, or false if not found.
-   */
-  isStatusUncertain(taskID: string): boolean {
-    const job = this.get(taskID);
-    return !!job?.statusUncertain;
-  }
-
-  /**
-   * Get the current state of a job, or undefined if not found.
-   */
   getState(taskID: string): BackgroundJobState | undefined {
-    const job = this.get(taskID);
-    return job?.state;
+    return this.field(taskID, 'state');
   }
 
   resolve(