diff --git a/src/commands/autofix-pr/__tests__/launchAutofixPr.test.ts b/src/commands/autofix-pr/__tests__/launchAutofixPr.test.ts index c6df04ff9..7fbd34a54 100644 --- a/src/commands/autofix-pr/__tests__/launchAutofixPr.test.ts +++ b/src/commands/autofix-pr/__tests__/launchAutofixPr.test.ts @@ -46,7 +46,7 @@ mock.module('src/utils/teleport.js', () => ({ })) const registerMock = mock(() => ({ - taskId: 'task-abc', + taskId: 'framework-task-id', sessionId: 'session-123', cleanup: () => {}, })) @@ -56,10 +56,14 @@ const checkEligibilityMock = mock(() => const getSessionUrlMock = mock( (id: string) => `https://claude.ai/session/${id}`, ) +const registerCompletionHookMock = mock< + (taskType: string, hook: (taskId: string, metadata?: unknown) => void) => void +>(() => {}) mock.module('src/tasks/RemoteAgentTask/RemoteAgentTask.js', () => ({ checkRemoteAgentEligibility: checkEligibilityMock, registerRemoteAgentTask: registerMock, + registerCompletionHook: registerCompletionHookMock, getRemoteTaskSessionUrl: getSessionUrlMock, formatPreconditionError: (e: { type: string }) => e.type, })) @@ -375,6 +379,63 @@ describe('callAutofixPr', () => { }) }) +// Regression suite for the taskId-mismatch latent bug + completion hook wiring. +// Before this fix, createAutofixTeammate generated a teammate UUID, that UUID +// was used to acquire the singleton monitor lock, and registerRemoteAgentTask +// generated a *different* framework taskId. When the framework eventually +// called clearActiveMonitor(frameworkTaskId) on natural completion, the guard +// failed (active.taskId !== frameworkTaskId) and the lock stayed acquired, +// blocking any subsequent /autofix-pr invocations in the same process. +describe('callAutofixPr · completion hook wiring (taskId mismatch regression)', () => { + test('updateActiveMonitor swaps lock taskId to framework-assigned id after register', async () => { + await callAutofixPr(onDone, makeContext(), '42') + const monitor = getActiveMonitor() as { taskId: string } | null + expect(monitor).not.toBeNull() + // registerMock returns 'framework-task-id'; before the fix this would be + // a teammate-generated random UUID instead. + expect(monitor?.taskId).toBe('framework-task-id') + }) + + test('framework hook → clearActiveMonitor releases lock on natural completion', async () => { + await callAutofixPr(onDone, makeContext(), '42') + expect(getActiveMonitor()).not.toBeNull() + + // Find the hook the module registered at import time. We grab the last + // call so re-imports across tests don't break this — only the most recent + // registration is what the framework would invoke now. + const calls = registerCompletionHookMock.mock.calls + expect(calls.length).toBeGreaterThan(0) + const lastCall = calls[calls.length - 1] + expect(lastCall?.[0]).toBe('autofix-pr') + const hook = lastCall?.[1] as (id: string, metadata?: unknown) => void + expect(typeof hook).toBe('function') + + // Simulate the framework invoking the hook with the framework taskId + // after a terminal transition. Before the fix this would no-op against + // a lock keyed by the teammate UUID. + hook('framework-task-id', { owner: 'acme', repo: 'myrepo', prNumber: 42 }) + expect(getActiveMonitor()).toBeNull() + }) + + test('subsequent /autofix-pr succeeds after framework hook clears the lock', async () => { + await callAutofixPr(onDone, makeContext(), '42') + // Simulate natural completion via the registered hook + const calls = registerCompletionHookMock.mock.calls + const hook = calls[calls.length - 1]?.[1] as ( + id: string, + metadata?: unknown, + ) => void + hook('framework-task-id', { owner: 'acme', repo: 'myrepo', prNumber: 42 }) + + onDone.mockClear() + await callAutofixPr(onDone, makeContext(), '99') + const firstArg = onDone.mock.calls[0]?.[0] as string + // Should be the success path, not "already monitoring" + expect(firstArg).not.toMatch(/already monitoring/i) + expect(firstArg).toMatch(/Autofix launched/) + }) +}) + // Cover ../index.ts load() — placed in this test file so all the heavy mocks // (teleport / detectRepository / RemoteAgentTask / bootstrap-state / analytics / // skillDetect) are already registered when load() dynamically imports diff --git a/src/commands/autofix-pr/__tests__/monitorState.test.ts b/src/commands/autofix-pr/__tests__/monitorState.test.ts index 43ce2f091..f97fcd972 100644 --- a/src/commands/autofix-pr/__tests__/monitorState.test.ts +++ b/src/commands/autofix-pr/__tests__/monitorState.test.ts @@ -5,6 +5,7 @@ import { isMonitoring, setActiveMonitor, trySetActiveMonitor, + updateActiveMonitor, } from '../monitorState.js' function makeState( @@ -76,4 +77,41 @@ describe('monitorState', () => { // First state remains expect(getActiveMonitor()?.prNumber).toBe(1) }) + + test('updateActiveMonitor returns false when no active monitor', () => { + expect(updateActiveMonitor({ taskId: 'task-x' })).toBe(false) + expect(getActiveMonitor()).toBeNull() + }) + + test('updateActiveMonitor merges partial fields into the active monitor', () => { + setActiveMonitor(makeState({ taskId: 'tentative-uuid' })) + expect(updateActiveMonitor({ taskId: 'framework-task-id' })).toBe(true) + const after = getActiveMonitor() + expect(after?.taskId).toBe('framework-task-id') + // Other fields untouched + expect(after?.owner).toBe('acme') + expect(after?.repo).toBe('myrepo') + expect(after?.prNumber).toBe(42) + }) + + test('updateActiveMonitor with new taskId makes clearActiveMonitor recognise framework taskId', () => { + // Reproduce the latent bug scenario: lock acquired with one taskId, + // framework assigns a different one. Before the fix, the framework's + // clearActiveMonitor(frameworkTaskId) would no-op because guard fails. + setActiveMonitor(makeState({ taskId: 'teammate-uuid' })) + // Framework cleanup using its own taskId — would fail guard before the fix + clearActiveMonitor('framework-uuid') + expect(getActiveMonitor()).not.toBeNull() + // After updateActiveMonitor swaps the taskId, framework cleanup works + updateActiveMonitor({ taskId: 'framework-uuid' }) + clearActiveMonitor('framework-uuid') + expect(getActiveMonitor()).toBeNull() + }) + + test('updateActiveMonitor does not change abortController identity', () => { + const ac = new AbortController() + setActiveMonitor(makeState({ abortController: ac, taskId: 'tentative' })) + updateActiveMonitor({ taskId: 'updated' }) + expect(getActiveMonitor()?.abortController).toBe(ac) + }) }) diff --git a/src/commands/autofix-pr/launchAutofixPr.ts b/src/commands/autofix-pr/launchAutofixPr.ts index cb4eb87f8..892448d75 100644 --- a/src/commands/autofix-pr/launchAutofixPr.ts +++ b/src/commands/autofix-pr/launchAutofixPr.ts @@ -13,6 +13,7 @@ import { checkRemoteAgentEligibility, formatPreconditionError, getRemoteTaskSessionUrl, + registerCompletionHook, registerRemoteAgentTask, type BackgroundRemoteSessionPrecondition, } from '../../tasks/RemoteAgentTask/RemoteAgentTask.js' @@ -26,10 +27,21 @@ import { getActiveMonitor, isMonitoring, trySetActiveMonitor, + updateActiveMonitor, } from './monitorState.js' import { parseAutofixArgs } from './parseArgs.js' import { detectAutofixSkills, formatSkillsHint } from './skillDetect.js' +// Release the singleton monitor lock when the framework transitions the +// autofix task to a terminal state. Without this, the lock — keyed by the +// framework-assigned taskId (after callAutofixPr's updateActiveMonitor swap) +// — would dangle past natural completion, blocking subsequent /autofix-pr +// invocations until the process restarts. Registered at module load; the +// framework's runCompletionHook invokes it once per terminal transition. +registerCompletionHook('autofix-pr', taskId => { + clearActiveMonitor(taskId) +}) + function makeErrorText(message: string, code: string): string { logEvent('tengu_autofix_pr_result', { result: @@ -277,8 +289,15 @@ export const callAutofixPr: LocalJSXCommandCall = async ( // 4.9 register task. If this throws, release the lock so the user can // retry — the remote CCR session is already created so we surface a // dedicated error code. + // + // After registration succeeds, swap the lock's taskId from the tentative + // teammate UUID (used to acquire the lock atomically before teleport) to + // the framework-assigned taskId. Without this swap, the framework's own + // cleanup path (clearActiveMonitor(frameworkTaskId) on natural completion) + // would no-op against a lock keyed by teammate.taskId, leaving the + // singleton lock dangling and blocking future /autofix-pr invocations. try { - registerRemoteAgentTask({ + const { taskId: frameworkTaskId } = registerRemoteAgentTask({ remoteTaskType: 'autofix-pr', session, command: `/autofix-pr ${prNumber}`, @@ -286,6 +305,7 @@ export const callAutofixPr: LocalJSXCommandCall = async ( isLongRunning: true, remoteTaskMetadata: { owner, repo, prNumber }, }) + updateActiveMonitor({ taskId: frameworkTaskId }) } catch (regErr: unknown) { clearActiveMonitor(teammate.taskId) const regMsg = regErr instanceof Error ? regErr.message : String(regErr) diff --git a/src/commands/autofix-pr/monitorState.ts b/src/commands/autofix-pr/monitorState.ts index df74292f1..273f0a90c 100644 --- a/src/commands/autofix-pr/monitorState.ts +++ b/src/commands/autofix-pr/monitorState.ts @@ -46,6 +46,20 @@ export function clearActiveMonitor(taskId?: string): void { active = null } +/** + * Atomically merges partial updates into the active monitor. Returns true if + * applied, false if no active monitor. Used when the caller needs to swap the + * lock's taskId after the framework assigns a different one than the + * tentative one used to acquire the lock — without this the framework's + * cleanup (clearActiveMonitor with the framework taskId) would no-op against + * a lock keyed by the caller's tentative id. + */ +export function updateActiveMonitor(partial: Partial): boolean { + if (!active) return false + active = { ...active, ...partial } + return true +} + export function isMonitoring( owner: string, repo: string, diff --git a/src/tasks/RemoteAgentTask/RemoteAgentTask.tsx b/src/tasks/RemoteAgentTask/RemoteAgentTask.tsx index 641c1c461..881046322 100644 --- a/src/tasks/RemoteAgentTask/RemoteAgentTask.tsx +++ b/src/tasks/RemoteAgentTask/RemoteAgentTask.tsx @@ -114,6 +114,38 @@ export function registerCompletionChecker(remoteTaskType: RemoteTaskType, checke completionCheckers.set(remoteTaskType, checker); } +/** + * Called after the task transitions to a terminal state and the notification + * has been enqueued. Used by command modules to release singleton locks, + * clear cached state, or perform other cleanup the framework cannot see. + * Hooks must be synchronous and best-effort — errors are logged but never + * propagate. + */ +export type RemoteTaskCompletionHook = (taskId: string, remoteTaskMetadata: RemoteTaskMetadata | undefined) => void; + +const completionHooks = new Map(); + +/** + * Register a completion hook for a remote task type. Invoked once after the + * task reaches a terminal state in any of the framework's completion branches + * (archived session, completionChecker, stableIdle, result). Use this to + * release command-module state (e.g. singleton locks) without forcing the + * framework to reverse-import from the command package. + */ +export function registerCompletionHook(remoteTaskType: RemoteTaskType, hook: RemoteTaskCompletionHook): void { + completionHooks.set(remoteTaskType, hook); +} + +function runCompletionHook(taskId: string, task: RemoteAgentTaskState): void { + const hook = completionHooks.get(task.remoteTaskType); + if (!hook) return; + try { + hook(taskId, task.remoteTaskMetadata); + } catch (e) { + logError(e); + } +} + /** * Persist a remote-agent metadata entry to the session sidecar. * Fire-and-forget — persistence failures must not block task registration. @@ -681,6 +713,7 @@ function startRemoteSessionPolling(taskId: string, context: TaskContext): () => enqueueRemoteNotification(taskId, task.title, 'completed', context.setAppState, task.toolUseId); void evictTaskOutput(taskId); void removeRemoteAgentMetadata(taskId); + runCompletionHook(taskId, task); return; } @@ -694,6 +727,7 @@ function startRemoteSessionPolling(taskId: string, context: TaskContext): () => enqueueRemoteNotification(taskId, completionResult, 'completed', context.setAppState, task.toolUseId); void evictTaskOutput(taskId); void removeRemoteAgentMetadata(taskId); + runCompletionHook(taskId, task); return; } } @@ -853,6 +887,7 @@ function startRemoteSessionPolling(taskId: string, context: TaskContext): () => enqueueRemoteReviewNotification(taskId, reviewContent, context.setAppState); void evictTaskOutput(taskId); void removeRemoteAgentMetadata(taskId); + runCompletionHook(taskId, task); return; // Stop polling } @@ -870,12 +905,14 @@ function startRemoteSessionPolling(taskId: string, context: TaskContext): () => enqueueRemoteReviewFailureNotification(taskId, reason, context.setAppState); void evictTaskOutput(taskId); void removeRemoteAgentMetadata(taskId); + runCompletionHook(taskId, task); return; // Stop polling } enqueueRemoteNotification(taskId, task.title, finalStatus, context.setAppState, task.toolUseId); void evictTaskOutput(taskId); void removeRemoteAgentMetadata(taskId); + runCompletionHook(taskId, task); return; // Stop polling } } catch (error) {