fix(task-board): fix session-task linkage and stuck workflow advancement - #436
Conversation
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s).
server/index.ts
Correct fix for a real bug — tasks stored clientIds but orphan detection compared against SDK sessionIds, so spawned tasks were never matched. The .finally() cleanup for NullTransport sessions is the right approach. Two minor style nits, one suggestion for wiring-level test coverage.
- 🔵 style (L275): The
if (registry.get(clientId))guard beforeregistry.remove(clientId)is redundant —remove()already handles missing entries gracefully (checksif (session)internally). The guard adds visual noise without preventing anything. Consider just callingregistry.remove(clientId)directly.[fixable] - 🔵 style (L239): The loop
for (const [clientId] of registry.entries()) { ids.add(clientId); }destructures tuples just to get the key. If SessionRegistry exposes akeys()iterator or asize/clientIdsaccessor, that would be cleaner. Minor — current form is clear enough.[fixable]
server/__tests__/task-orchestrator.test.ts
Correct fix for a real bug — tasks stored clientIds but orphan detection compared against SDK sessionIds, so spawned tasks were never matched. The .finally() cleanup for NullTransport sessions is the right approach. Two minor style nits, one suggestion for wiring-level test coverage.
- 🔵 missing_tests: The
.finally()cleanup in spawnSession (registry removal + tick trigger) is critical to fixing stuck workflows but is only covered by integration-level wiring in index.ts, not by a unit test. The orchestrator tests validate orphan detection logic itself, which is the important part, but a test confirming that a completed spawn removes its registry entry and triggers tick would guard against regressions in the wiring.
| // Session ended (success or failure) — clean up registry entry | ||
| // (no WS close to trigger normal removal) and advance the | ||
| // orchestrator so orphan reclaim picks up unfinished tasks. | ||
| if (registry.get(clientId)) { |
There was a problem hiding this comment.
🔵 style: The if (registry.get(clientId)) guard before registry.remove(clientId) is redundant — remove() already handles missing entries gracefully (checks if (session) internally). The guard adds visual noise without preventing anything. Consider just calling registry.remove(clientId) directly. [fixable]
| }, | ||
| getActiveSessionIds: () => { | ||
| // Return clientIds — task.session_id stores clientId, not SDK sessionId. | ||
| // Using sessionId here caused orphan detection to never match spawned tasks. |
There was a problem hiding this comment.
🔵 style: The loop for (const [clientId] of registry.entries()) { ids.add(clientId); } destructures tuples just to get the key. If SessionRegistry exposes a keys() iterator or a size/clientIds accessor, that would be cleaner. Minor — current form is clear enough. [fixable]
Centaur ReviewFound 3 issue(s) (1 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s) (1 warning).
server/__tests__/task-orchestrator.test.ts
Solid bug fix — the clientId/sessionId mismatch in orphan detection was a clear correctness issue, and the .finally() wiring properly handles spawned session lifecycle. Minor gaps in test coverage for the failure path and a fragile synchrony assumption on taskContext assignment.
- 🔵 missing_tests (L869): The new test 'tick after spawn session ends reclaims orphaned task and advances' covers the happy path (task completes, then session ends). There's no test for the failure path: a spawned session dies without completing the task. In production,
.finally()fires →registry.remove()→tick()→ orphan reclaim resets the still-active task to pending. This is the primary scenario the.finally()wiring exists to handle and should be tested explicitly.[fixable] - 🔵 style (L900): The test comment says 'Simulate .finally() wiring' but calls
orch.onTaskCompleted(t1.id), while the actual.finally()block callsorchestratorRef?.tick(). These are different entry points:onTaskCompletedhas astate !== 'running'guard and resetsspawnDepthbefore callingtick(). The test works because tick is idempotent, but it doesn't precisely model the production code path. Usingorch.tick()directly (after manually clearing the active set) would be a more faithful simulation.[fixable]
server/index.ts
Solid bug fix — the clientId/sessionId mismatch in orphan detection was a clear correctness issue, and the .finally() wiring properly handles spawned session lifecycle. Minor gaps in test coverage for the failure path and a fragile synchrony assumption on taskContext assignment.
- 🟡 unsafe_assumptions (L277): The comment says 'register() is synchronous inside startChat, so the session exists here', but
startChatis fire-and-forget (not awaited). This depends onstartChatsynchronously callingregistry.register()before its firstawait. IfstartChatis ever refactored to defer registration (e.g. behind an async worktree check),registry.get(clientId)will silently return undefined, theif (session)guard will skip silently, andtaskContextwon't be set — breaking session overview visibility with no error logged. Consider logging whensessionis null, or moving this assignment intostartChatitself via the options bag.[fixable]
|
|
||
| expect(spawnSession).toHaveBeenCalledWith(task.id, expect.any(String), goal.id); | ||
| expect(store.get(task.id)?.status).toBe('active'); | ||
| expect(deps.setTaskContext).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
🔵 missing_tests: The new test 'tick after spawn session ends reclaims orphaned task and advances' covers the happy path (task completes, then session ends). There's no test for the failure path: a spawned session dies without completing the task. In production, .finally() fires → registry.remove() → tick() → orphan reclaim resets the still-active task to pending. This is the primary scenario the .finally() wiring exists to handle and should be tested explicitly. [fixable]
|
|
||
| // Simulate .finally() wiring: session removed from registry, tick called | ||
| activeClients = new Set(); | ||
| orch.onTaskCompleted(t1.id); |
There was a problem hiding this comment.
🔵 style: The test comment says 'Simulate .finally() wiring' but calls orch.onTaskCompleted(t1.id), while the actual .finally() block calls orchestratorRef?.tick(). These are different entry points: onTaskCompleted has a state !== 'running' guard and resets spawnDepth before calling tick(). The test works because tick is idempotent, but it doesn't precisely model the production code path. Using orch.tick() directly (after manually clearing the active set) would be a more faithful simulation. [fixable]
|
|
||
| // Link session to task for session overview visibility. | ||
| // register() is synchronous inside startChat, so the session exists here. | ||
| const session = registry.get(clientId); |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The comment says 'register() is synchronous inside startChat, so the session exists here', but startChat is fire-and-forget (not awaited). This depends on startChat synchronously calling registry.register() before its first await. If startChat is ever refactored to defer registration (e.g. behind an async worktree check), registry.get(clientId) will silently return undefined, the if (session) guard will skip silently, and taskContext won't be set — breaking session overview visibility with no error logged. Consider logging when session is null, or moving this assignment into startChat itself via the options bag. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 4 issue(s) (2 warning).
server/__tests__/task-orchestrator.test.ts
Correct fix for a real bug (clientId/sessionId mismatch broke orphan detection for spawned tasks), with good test coverage of the happy path, but missing test coverage for the failure-path orphan reclaim scenario that motivated the fix.
- 🟡 missing_tests (L869): The new 'tick after spawn session ends' test simulates the .finally() wiring using orch.onTaskCompleted(t1.id), but the actual .finally() in index.ts calls orchestratorRef?.tick() directly. onTaskCompleted resets spawnDepth to 0 (line 197 of task-orchestrator.ts) while tick() does not, making the test more permissive than reality. The test should call orch.tick() directly (after manually updating store state) to match the real code path.
[fixable] - 🟡 missing_tests: No test covers the failure path — the primary scenario this fix addresses. When a spawned session fails (.catch branch) without calling TaskComplete, the .finally() fires, registry.remove() makes the task orphaned, and tick() should reclaim it back to 'pending'. This scenario (task still 'active' when session ends) is distinct from the tested happy path (task already 'done' when session ends) and exercises the orphan detection that was previously broken.
[fixable]
server/index.ts
Correct fix for a real bug (clientId/sessionId mismatch broke orphan detection for spawned tasks), with good test coverage of the happy path, but missing test coverage for the failure-path orphan reclaim scenario that motivated the fix.
- 🔵 unsafe_assumptions (L274): The comment 'register() is synchronous inside startChat, so the session exists here' is correct today (registry.register at chat.ts:848 runs before any await in _startChatInner), but this is a fragile implicit contract. If a future refactor adds an await before register() (e.g. async validation), the registry.get(clientId) here would silently return undefined and skip the taskContext assignment. Consider moving the taskContext assignment into the spawnSession callback after setSessionId in task-orchestrator.ts, or documenting the contract at the register() call site.
[fixable] - 🔵 style (L241): Array.from(registry.entries(), ([clientId]) => clientId) allocates an intermediate array just to feed it into a Set. A clearer alternative: new Set(registry.keys()) if SessionRegistry exposes a keys() method, or the existing loop pattern (for...of with ids.add) which avoids the allocation.
[fixable]
|
|
||
| expect(spawnSession).toHaveBeenCalledWith(task.id, expect.any(String), goal.id); | ||
| expect(store.get(task.id)?.status).toBe('active'); | ||
| expect(deps.setTaskContext).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
🟡 missing_tests: The new 'tick after spawn session ends' test simulates the .finally() wiring using orch.onTaskCompleted(t1.id), but the actual .finally() in index.ts calls orchestratorRef?.tick() directly. onTaskCompleted resets spawnDepth to 0 (line 197 of task-orchestrator.ts) while tick() does not, making the test more permissive than reality. The test should call orch.tick() directly (after manually updating store state) to match the real code path. [fixable]
| orchestratorRef?.tick(); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🔵 unsafe_assumptions: The comment 'register() is synchronous inside startChat, so the session exists here' is correct today (registry.register at chat.ts:848 runs before any await in _startChatInner), but this is a fragile implicit contract. If a future refactor adds an await before register() (e.g. async validation), the registry.get(clientId) here would silently return undefined and skip the taskContext assignment. Consider moving the taskContext assignment into the spawnSession callback after setSessionId in task-orchestrator.ts, or documenting the contract at the register() call site. [fixable]
| // Return clientIds — task.session_id stores clientId, not SDK sessionId. | ||
| // Using sessionId here caused orphan detection to never match spawned tasks. | ||
| return new Set(Array.from(registry.entries(), ([clientId]) => clientId)); | ||
| }, |
There was a problem hiding this comment.
🔵 style: Array.from(registry.entries(), ([clientId]) => clientId) allocates an intermediate array just to feed it into a Set. A clearer alternative: new Set(registry.keys()) if SessionRegistry exposes a keys() method, or the existing loop pattern (for...of with ids.add) which avoids the allocation. [fixable]
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 4 issue(s).
server/__tests__/task-orchestrator.test.ts
Correct fix for a real bug — getActiveSessionIds was returning SDK sessionIds but setSessionId stores clientIds, causing orphan detection to silently skip all spawned tasks. The .finally() wiring and taskContext linkage are well-structured. All findings are suggestions; no blocking issues.
- 🔵 missing_tests (L895): The 'tick after spawn session completes advances workflow' test doesn't assert that
spawnSessionwas called exactly once. Addingexpect(spawnSession).toHaveBeenCalledTimes(1)after verifying t2 is active would prove t2 used the reuse path (not spawn), which is the key workflow-advancement contract being tested.[fixable]
server/index.ts
Correct fix for a real bug — getActiveSessionIds was returning SDK sessionIds but setSessionId stores clientIds, causing orphan detection to silently skip all spawned tasks. The .finally() wiring and taskContext linkage are well-structured. All findings are suggestions; no blocking issues.
- 🔵 style (L240): Minor:
Array.from(registry.entries(), ([clientId]) => clientId)iterates key-value pairs but only uses keys. Consider adding akeys()method toSessionRegistry(it wraps a Map) for cleaner intent, or at minimum a comment explaining whyentries()is used (because nokeys()exists).[fixable] - 🔵 unsafe_assumptions (L289): The
taskContextlinkage assumesregistry.get(clientId)succeeds becauseregister()runs synchronously beforestartChat's firstawait. The assumption is currently valid (verified: noawaitbeforeregister()in_startChatInner), and thelog.warnfallback is good defensive coding. However, this is a fragile coupling — if someone adds anawaitbeforeregister()inchat.ts, this breaks silently. Consider adding a brief code comment inchat.tsnearregister()noting that callers depend on its synchronous execution relative to the function's return.[fixable] - 🔵 style (L243): The
task:prefix for spawned sessions is a good improvement overheadless:, butapp.ts:501still usesheadless:for externally-created headless sessions (POST /api/sessions). The inconsistency is intentional (different use cases), but worth noting that both code paths create NullTransport sessions — only the task path gets.finally()cleanup. If external headless sessions also need orphan-aware cleanup in the future, the same pattern would be needed there.
|
|
||
| // Agent completed the task during the session | ||
| store.update(t1.id, { status: 'done' }); | ||
| store.cascadeStatus(t1.id); |
There was a problem hiding this comment.
🔵 missing_tests: The 'tick after spawn session completes advances workflow' test doesn't assert that spawnSession was called exactly once. Adding expect(spawnSession).toHaveBeenCalledTimes(1) after verifying t2 is active would prove t2 used the reuse path (not spawn), which is the key workflow-advancement contract being tested. [fixable]
| return ids; | ||
| // Return clientIds — task.session_id stores clientId, not SDK sessionId. | ||
| // Using sessionId here caused orphan detection to never match spawned tasks. | ||
| return new Set(Array.from(registry.entries(), ([clientId]) => clientId)); |
There was a problem hiding this comment.
🔵 style: Minor: Array.from(registry.entries(), ([clientId]) => clientId) iterates key-value pairs but only uses keys. Consider adding a keys() method to SessionRegistry (it wraps a Map) for cleaner intent, or at minimum a comment explaining why entries() is used (because no keys() exists). [fixable]
| // so the session exists here. Log if that assumption ever breaks. | ||
| const session = registry.get(clientId); | ||
| if (session) { | ||
| session.taskContext = { currentTaskId: taskId, goalId }; |
There was a problem hiding this comment.
🔵 unsafe_assumptions: The taskContext linkage assumes registry.get(clientId) succeeds because register() runs synchronously before startChat's first await. The assumption is currently valid (verified: no await before register() in _startChatInner), and the log.warn fallback is good defensive coding. However, this is a fragile coupling — if someone adds an await before register() in chat.ts, this breaks silently. Consider adding a brief code comment in chat.ts near register() noting that callers depend on its synchronous execution relative to the function's return. [fixable]
| }, | ||
| spawnSession: async (taskId: string, prompt: string, goalId: string) => { | ||
| const clientId = `headless:${generateWtId()}`; | ||
| const clientId = `task:${generateWtId()}`; |
There was a problem hiding this comment.
🔵 style: The task: prefix for spawned sessions is a good improvement over headless:, but app.ts:501 still uses headless: for externally-created headless sessions (POST /api/sessions). The inconsistency is intentional (different use cases), but worth noting that both code paths create NullTransport sessions — only the task path gets .finally() cleanup. If external headless sessions also need orphan-aware cleanup in the future, the same pattern would be needed there.
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s) (1 warning).
server/index.ts
Solid fix for the clientId-vs-sessionId mismatch in orphan detection and the missing spawn lifecycle cleanup. The .finally() handler should guard against stale goals to avoid overriding manual pauses.
- 🟡 unsafe_assumptions (L275): The
.finally()handler resumes a paused orchestrator without checking if it's still on the same goal. If a user manually pauses the orchestrator (/api/loop/pause) while a spawned session is still in-flight, the.finally()will override the manual pause by callingresume(). ThespawnSessionclosure already hasgoalIdin scope — add a guard likeorchestratorRef.getStatus().goalId === goalIdto only advance the same goal's workflow, matching the existing guard pattern in the orchestrator's spawn.then()callback (task-orchestrator.ts:364).[fixable] - 🔵 style (L240):
new Set(Array.from(registry.entries(), ([clientId]) => clientId))allocates an intermediate array. The registry is a Map wrapper, so this could be simplified tonew Set(registry.sessions.keys())— though that would require exposingkeys()on SessionRegistry. Consider adding akeys()method toSessionRegistryfor a cleaner API. Minor — current approach is correct, just slightly more allocation than needed.[fixable]
server/__tests__/task-orchestrator.test.ts
Solid fix for the clientId-vs-sessionId mismatch in orphan detection and the missing spawn lifecycle cleanup. The .finally() handler should guard against stale goals to avoid overriding manual pauses.
- 🔵 missing_tests: No test covers the
.finally()path when the orchestrator has been stopped (state='idle') before the spawned session finishes. In production,tick()returns immediately (guarded bystate !== 'running'), so it's safe — but a test would document this behavior and prevent regressions if the guard changes.[fixable]
| // After all tasks are spawned, the orchestrator pauses (no pending | ||
| // tasks left). tick() is a no-op when paused, so resume() is needed | ||
| // to re-enter the running state and trigger orphan reclaim. | ||
| if (orchestratorRef) { |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The .finally() handler resumes a paused orchestrator without checking if it's still on the same goal. If a user manually pauses the orchestrator (/api/loop/pause) while a spawned session is still in-flight, the .finally() will override the manual pause by calling resume(). The spawnSession closure already has goalId in scope — add a guard like orchestratorRef.getStatus().goalId === goalId to only advance the same goal's workflow, matching the existing guard pattern in the orchestrator's spawn .then() callback (task-orchestrator.ts:364). [fixable]
| return ids; | ||
| // Return clientIds — task.session_id stores clientId, not SDK sessionId. | ||
| // Using sessionId here caused orphan detection to never match spawned tasks. | ||
| return new Set(Array.from(registry.entries(), ([clientId]) => clientId)); |
There was a problem hiding this comment.
🔵 style: new Set(Array.from(registry.entries(), ([clientId]) => clientId)) allocates an intermediate array. The registry is a Map wrapper, so this could be simplified to new Set(registry.sessions.keys()) — though that would require exposing keys() on SessionRegistry. Consider adding a keys() method to SessionRegistry for a cleaner API. Minor — current approach is correct, just slightly more allocation than needed. [fixable]
Centaur ReviewFound 3 issue(s).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s).
server/index.ts
Solid bug fix — the root cause (clientId vs sessionId mismatch in orphan detection) is correctly identified and fixed, with good test coverage for the new .finally() lifecycle wiring. No correctness issues found.
- 🔵 unsafe_assumptions (L275): The
.finally()handler readsorchestratorRef.getStatus().stateand branches onpausedvs other. If the orchestrator wasstop()ped between spawn and session completion (user stopped the loop), callingtick()is harmless (returns early due tostate !== 'running'), andresume()won't fire since state isidlenotpaused. This is safe — but a brief log line in theelsebranch of theorchestratorRefcheck (or when state isidle) would help debug "session ended after loop stopped" situations in production.[fixable] - 🔵 style (L240):
new Set(Array.from(registry.entries(), ([clientId]) => clientId))can be simplified tonew Set(registry.keys())ifSessionRegistryexposes akeys()method, ornew Set(Array.from(registry.entries()).map(([k]) => k))isn't more readable. Current form works butArray.from(iterable, mapFn)with destructuring is slightly unusual. Consider adding akeys()method toSessionRegistry— it would make this intent obvious.[fixable]
server/__tests__/task-orchestrator.test.ts
Solid bug fix — the root cause (clientId vs sessionId mismatch in orphan detection) is correctly identified and fixed, with good test coverage for the new .finally() lifecycle wiring. No correctness issues found.
- 🔵 missing_tests (L906): The "resume after spawn session dies" test covers the crash → reclaim → re-dispatch path well, but doesn't verify the reclaimed task's status transitions (active → pending → active). Adding
expect(store.get(task.id)!.status).toBe('pending')before theresume()call would document the pre-condition more clearly and guard against store-level regressions.[fixable]
| // After all tasks are spawned, the orchestrator pauses (no pending | ||
| // tasks left). tick() is a no-op when paused, so resume() is needed | ||
| // to re-enter the running state and trigger orphan reclaim. | ||
| if (orchestratorRef) { |
There was a problem hiding this comment.
🔵 unsafe_assumptions: The .finally() handler reads orchestratorRef.getStatus().state and branches on paused vs other. If the orchestrator was stop()ped between spawn and session completion (user stopped the loop), calling tick() is harmless (returns early due to state !== 'running'), and resume() won't fire since state is idle not paused. This is safe — but a brief log line in the else branch of the orchestratorRef check (or when state is idle) would help debug "session ended after loop stopped" situations in production. [fixable]
| return ids; | ||
| // Return clientIds — task.session_id stores clientId, not SDK sessionId. | ||
| // Using sessionId here caused orphan detection to never match spawned tasks. | ||
| return new Set(Array.from(registry.entries(), ([clientId]) => clientId)); |
There was a problem hiding this comment.
🔵 style: new Set(Array.from(registry.entries(), ([clientId]) => clientId)) can be simplified to new Set(registry.keys()) if SessionRegistry exposes a keys() method, or new Set(Array.from(registry.entries()).map(([k]) => k)) isn't more readable. Current form works but Array.from(iterable, mapFn) with destructuring is slightly unusual. Consider adding a keys() method to SessionRegistry — it would make this intent obvious. [fixable]
| expect(orch.getStatus().activeTaskId).toBe(t2.id); | ||
| }); | ||
|
|
||
| it('resume after spawn session dies reclaims unfinished task', async () => { |
There was a problem hiding this comment.
🔵 missing_tests: The "resume after spawn session dies" test covers the crash → reclaim → re-dispatch path well, but doesn't verify the reclaimed task's status transitions (active → pending → active). Adding expect(store.get(task.id)!.status).toBe('pending') before the resume() call would document the pre-condition more clearly and guard against store-level regressions. [fixable]
Centaur ReviewFound 4 issue(s) (1 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 5 issue(s) (2 warning).
server/index.ts
Solid fix — correctly aligns orphan detection to use clientIds instead of SDK sessionIds, adds proper cleanup via .finally(), and includes good test coverage for the main scenarios. The few findings are low-severity: a missing edge-case test for goal-change, and minor style/coupling observations.
- 🟡 unsafe_assumptions (L275): The
.finally()callback checksorchestratorRef.getStatus().goalId === goalIdto decide whether to advance. However, when the orchestrator is inrunningstate with the same goalId, callingtick()is safe but could double-dispatch if another source (e.g.,SignalProcessorat line 183, oronTaskCompleted) triggerstick()in the same microtask queue. Since Node.js is single-threaded andtick()is synchronous this is safe today, but the lack of a re-entrancy guard makes it fragile iftick()ever becomes async. Low risk given current design, noting for awareness. - 🟡 unsafe_assumptions (L294): The comment says
register() runs synchronously before startChat's first await— this assumesstartChatcallsregistry.register(clientId, ...)synchronously before its firstawait. IfstartChatever changes to do an async operation before registering (e.g., async worktree creation),registry.get(clientId)would return undefined, andtaskContextwould silently not be set. Thelog.warnfallback correctly detects this, but the session would run without taskContext, which could cause the session overview to miss it. The defensive logging is good; just flagging the coupling. - 🔵 style (L267): The
.finally()block is 22 lines with branching logic (paused vs running, goal match vs mismatch). Consider extracting this into a named function (e.g.,onTaskSessionEnded(clientId, taskId, goalId)) to improve readability and make the intent self-documenting. The current inline approach works but is dense for a fire-and-forget chain.[fixable] - 🔵 style (L243): The prefix was renamed from
headless:totask:which is clearer. However,log.infoon line 256 still says'task session resolved'and line 261 says'task session failed'— these are fine. But the catch block at line 306 still says'failed to spawn session for task', which could be confused with line 261's'task session failed'during log searching. Consider differentiating: one is the async session failure (agent error) and the other is the synchronous spawn failure (e.g., transport creation). Minor log clarity issue.[fixable]
server/__tests__/task-orchestrator.test.ts
Solid fix — correctly aligns orphan detection to use clientIds instead of SDK sessionIds, adds proper cleanup via .finally(), and includes good test coverage for the main scenarios. The few findings are low-severity: a missing edge-case test for goal-change, and minor style/coupling observations.
- 🔵 missing_tests: The new tests cover the happy paths (tick advances, resume reclaims, idle no-op), but there's no test for the goal-changed guard: what happens when
.finally()fires but the orchestrator has moved to a different goalId (viastop()+start(newGoal))? The production code handles it (line 275 checksgoalId === goalId), but there's no test asserting that the old goal's tasks are left untouched.[fixable]
| // Only advance if the orchestrator is still on the same goal. | ||
| // A user may have paused manually or started a new goal while | ||
| // this session was in-flight — don't override that. | ||
| if (orchestratorRef && orchestratorRef.getStatus().goalId === goalId) { |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The .finally() callback checks orchestratorRef.getStatus().goalId === goalId to decide whether to advance. However, when the orchestrator is in running state with the same goalId, calling tick() is safe but could double-dispatch if another source (e.g., SignalProcessor at line 183, or onTaskCompleted) triggers tick() in the same microtask queue. Since Node.js is single-threaded and tick() is synchronous this is safe today, but the lack of a re-entrancy guard makes it fragile if tick() ever becomes async. Low risk given current design, noting for awareness.
| // Link session to task for session overview visibility. | ||
| // register() runs synchronously before startChat's first await, | ||
| // so the session exists here. Log if that assumption ever breaks. | ||
| const session = registry.get(clientId); |
There was a problem hiding this comment.
🟡 unsafe_assumptions: The comment says register() runs synchronously before startChat's first await — this assumes startChat calls registry.register(clientId, ...) synchronously before its first await. If startChat ever changes to do an async operation before registering (e.g., async worktree creation), registry.get(clientId) would return undefined, and taskContext would silently not be set. The log.warn fallback correctly detects this, but the session would run without taskContext, which could cause the session overview to miss it. The defensive logging is good; just flagging the coupling.
| error: (err as Error).message, | ||
| }); | ||
| }) | ||
| .finally(() => { |
There was a problem hiding this comment.
🔵 style: The .finally() block is 22 lines with branching logic (paused vs running, goal match vs mismatch). Consider extracting this into a named function (e.g., onTaskSessionEnded(clientId, taskId, goalId)) to improve readability and make the intent self-documenting. The current inline approach works but is dense for a fire-and-forget chain. [fixable]
| }, | ||
| spawnSession: async (taskId: string, prompt: string, goalId: string) => { | ||
| const clientId = `headless:${generateWtId()}`; | ||
| const clientId = `task:${generateWtId()}`; |
There was a problem hiding this comment.
🔵 style: The prefix was renamed from headless: to task: which is clearer. However, log.info on line 256 still says 'task session resolved' and line 261 says 'task session failed' — these are fine. But the catch block at line 306 still says 'failed to spawn session for task', which could be confused with line 261's 'task session failed' during log searching. Consider differentiating: one is the async session failure (agent error) and the other is the synchronous spawn failure (e.g., transport creation). Minor log clarity issue. [fixable]
Three bugs fixed: 1. Orphan detection used SDK sessionIds but task.session_id stores clientIds — IDs never matched, so spawned tasks were never reclaimed. Changed getActiveSessionIds to return clientIds from the registry. 2. Spawned task sessions never got taskContext set, making them invisible in the session overview (no link back to the task they're working on). Now sets taskContext immediately after spawn. 3. When a spawned session ended (success or crash), nothing triggered orchestrator.tick() — the workflow stayed stuck on the completed task forever. Added .finally() cleanup that removes the registry entry and advances the orchestrator. Also renamed clientId prefix from 'headless:' to 'task:' — these are task-spawned sessions, not headless. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove redundant registry.get() guard before remove() (style) - Simplify getActiveSessionIds loop with Array.from (style) - Add test for spawn-end → reclaim → advance workflow (missing_tests) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…trator Centaur review surfaced that tick() is a no-op when the orchestrator is paused. After spawning all tasks, the orchestrator pauses (no pending tasks left). When a spawned session ends, .finally() must call resume() (not tick()) to re-enter the running state and trigger orphan reclaim. Also: - Add failure-path test: session dies without completing → resume → orphan reclaim → task re-dispatched (spawnSession called twice) - Fix happy-path test to use tick() directly instead of onTaskCompleted - Add log warning when taskContext can't be set (sync assumption guard) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Guard .finally() handler with goalId check to prevent overriding manual pauses or new goals started while session was in-flight - Avoid intermediate array allocation in getActiveSessionIds - Add test: tick is a no-op when orchestrator stopped before session ends Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…og, test assertion - Add SessionRegistry.keys() method and use it in getActiveSessionIds - Log when .finally() fires after orchestrator loop has already stopped - Add status pre-condition assertion in resume-after-crash test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2a1b80f to
a3fabae
Compare
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s).
server/__tests__/task-orchestrator.test.ts
Solid fix for a real bug — orphan detection was comparing clientIds against SDK sessionIds, so it never matched spawned tasks. The `.finally() handler for registry cleanup and orchestrator advancement is well-guarded against all edge cases (stopped, goal changed, paused). Tests cover the critical paths. Only minor suggestions.
- 🔵 missing_tests: The
.finally()handler inserver/index.tshas an else branch for when the goal has changed or the orchestrator was stopped (logs 'task session ended after loop stopped or goal changed'). The 'stopped' case is tested ('tick is a no-op when orchestrator is stopped'), but the 'goal changed while session in-flight' scenario is not — e.g., user starts a new goal while a spawned session from the old goal is still running. Consider adding a test wheregoalIdno longer matches.[fixable]
packages/harness/src/session-registry.ts
Solid fix for a real bug — orphan detection was comparing clientIds against SDK sessionIds, so it never matched spawned tasks. The `.finally() handler for registry cleanup and orchestrator advancement is well-guarded against all edge cases (stopped, goal changed, paused). Tests cover the critical paths. Only minor suggestions.
- 🔵 missing_tests: No unit test file exists for
SessionRegistry. The newkeys()method is a trivial one-liner, but there are no tests for any of the registry's methods (entries, get, remove, etc.). Low priority since these are thin wrappers over Map, but worth noting for coverage.[fixable]
server/index.ts
Solid fix for a real bug — orphan detection was comparing clientIds against SDK sessionIds, so it never matched spawned tasks. The `.finally() handler for registry cleanup and orchestrator advancement is well-guarded against all edge cases (stopped, goal changed, paused). Tests cover the critical paths. Only minor suggestions.
- 🔵 style: Minor:
orchestratorRef.getStatus()is called twice in the.finally()handler (once to checkgoalId, once to checkstate). Since this is synchronous single-threaded code there's no correctness issue, but a singleconst status = orchestratorRef.getStatus()would be marginally cleaner.[fixable]
Add test for goal-changed-while-session-in-flight scenario. Extract double getStatus() call to single variable in .finally() handler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Early return when orchestratorRef is null so TypeScript can narrow the type for subsequent method calls. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s) (1 warning).
server/index.ts
Solid fix for a real bug where orphan detection never matched spawned tasks due to clientId vs sessionId mismatch. The .finally() handler for session lifecycle advancement is well-guarded with goal comparison. Tests are comprehensive. Minor concern: wrap orchestrator calls in .finally() with try-catch to avoid unhandled rejections on store errors.
- 🟡 bugs (L268): The .finally() handler calls orchestratorRef.tick() or .resume() without a try-catch. These methods call into TaskStore (SQLite queries) which can throw on database errors. Since .finally() is on a promise chain, an exception here becomes an unhandled promise rejection — the session cleanup (registry.remove) has already happened, but the error is silently swallowed in production or crashes the process depending on Node.js --unhandled-rejections mode. Wrap the orchestrator calls in try-catch with a log.error fallback.
[fixable] - 🔵 style (L243): The prefix changed from 'headless:' to 'task:' which is a clearer name. However, no migration handling exists for tasks in SQLite that may still reference 'headless:' session_ids from before this change. On server restart, such tasks would be orphaned and reclaimed (correct behavior), but worth noting in the commit message that this is intentional — the prefix change applies only to new sessions.
server/__tests__/task-orchestrator.test.ts
Solid fix for a real bug where orphan detection never matched spawned tasks due to clientId vs sessionId mismatch. The .finally() handler for session lifecycle advancement is well-guarded with goal comparison. Tests are comprehensive. Minor concern: wrap orchestrator calls in .finally() with try-catch to avoid unhandled rejections on store errors.
- 🔵 missing_tests (L946): The 'resume after spawn session dies' test relies on the .then() callback from the mock spawnSession setting sessionId on the task (via queueMicrotask flushing during vi.waitFor). This implicit dependency makes the test fragile — if vi.waitFor timing changes, the sessionId might not be set when resume() runs, causing orphan detection to miss the task (no session_id = no orphan). Consider adding an explicit assertion that task.sessionId is set before simulating the crash, e.g.
expect(store.get(task.id)!.sessionId).toBe('task:spawned-1')before clearing activeClients.[fixable]
| error: (err as Error).message, | ||
| }); | ||
| }) | ||
| .finally(() => { |
There was a problem hiding this comment.
🟡 bugs: The .finally() handler calls orchestratorRef.tick() or .resume() without a try-catch. These methods call into TaskStore (SQLite queries) which can throw on database errors. Since .finally() is on a promise chain, an exception here becomes an unhandled promise rejection — the session cleanup (registry.remove) has already happened, but the error is silently swallowed in production or crashes the process depending on Node.js --unhandled-rejections mode. Wrap the orchestrator calls in try-catch with a log.error fallback. [fixable]
| }, | ||
| spawnSession: async (taskId: string, prompt: string, goalId: string) => { | ||
| const clientId = `headless:${generateWtId()}`; | ||
| const clientId = `task:${generateWtId()}`; |
There was a problem hiding this comment.
🔵 style: The prefix changed from 'headless:' to 'task:' which is a clearer name. However, no migration handling exists for tasks in SQLite that may still reference 'headless:' session_ids from before this change. On server restart, such tasks would be orphaned and reclaimed (correct behavior), but worth noting in the commit message that this is intentional — the prefix change applies only to new sessions.
| expect(orch.getStatus().activeTaskId).toBe(t2.id); | ||
| }); | ||
|
|
||
| it('resume after spawn session dies reclaims unfinished task', async () => { |
There was a problem hiding this comment.
🔵 missing_tests: The 'resume after spawn session dies' test relies on the .then() callback from the mock spawnSession setting sessionId on the task (via queueMicrotask flushing during vi.waitFor). This implicit dependency makes the test fragile — if vi.waitFor timing changes, the sessionId might not be set when resume() runs, causing orphan detection to miss the task (no session_id = no orphan). Consider adding an explicit assertion that task.sessionId is set before simulating the crash, e.g. expect(store.get(task.id)!.sessionId).toBe('task:spawned-1') before clearing activeClients. [fixable]
…ardening Wrap orchestrator calls in .finally() handler with try-catch to prevent unhandled rejections on store errors. Add explicit sessionId assertion in crash-reclaim test to guard against timing fragility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centaur ReviewFound 5 issue(s) (1 warning).
|
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s).
server/__tests__/task-orchestrator.test.ts
Solid bug fix — the core issue (sessionId vs clientId mismatch in orphan detection) is correctly identified and fixed, the .finally() handler has proper guards against goal drift and state transitions, and the new tests cover the key lifecycle scenarios (success advancement, crash recovery, idle no-op, goal change).
- 🔵 missing_tests: The
.finally()handler in index.ts has aresume()path (whenstatus.state === 'paused') and atick()path (otherwise). The 'resume after spawn session dies' test at line 946 exercises the resume path by callingorch.resume()directly, but doesn't test the goal-change guard on that path. Consider adding a test where the orchestrator is paused AND the goal has changed, verifying that.resume()on the new goal doesn't cause unexpected behavior from the old session's finally handler. This is low-risk since the.finally()guard in index.ts checksstatus.goalId === goalIdbefore either branch.[fixable]
server/index.ts
Solid bug fix — the core issue (sessionId vs clientId mismatch in orphan detection) is correctly identified and fixed, the .finally() handler has proper guards against goal drift and state transitions, and the new tests cover the key lifecycle scenarios (success advancement, crash recovery, idle no-op, goal change).
- 🔵 style (L243): Prefix changed from
headless:totask:— this is a good semantic improvement, but any external logging/monitoring/alerting that greps forheadless:will silently stop matching. If there are log queries or dashboards filtering on the old prefix, they should be updated. Not a code issue, but worth noting in the PR description. - 🔵 unsafe_assumptions (L240): The new
getActiveSessionIdsreturns ALL clientIds from the registry viaregistry.keys(), including regular browser-connected sessions. Previously it only returned SDK sessionIds from active sessions. This means orphan detection now compares tasksession_id(which stores clientIds liketask:abc123) against ALL registry keys (which also include browser client IDs). This works correctly because browser clientIds will never match atask:*sessionId stored on a task, but it's a broader set than strictly necessary. Not a bug — just a noted semantic change.
| }, | ||
| spawnSession: async (taskId: string, prompt: string, goalId: string) => { | ||
| const clientId = `headless:${generateWtId()}`; | ||
| const clientId = `task:${generateWtId()}`; |
There was a problem hiding this comment.
🔵 style: Prefix changed from headless: to task: — this is a good semantic improvement, but any external logging/monitoring/alerting that greps for headless: will silently stop matching. If there are log queries or dashboards filtering on the old prefix, they should be updated. Not a code issue, but worth noting in the PR description.
| return ids; | ||
| // Return clientIds — task.session_id stores clientId, not SDK sessionId. | ||
| // Using sessionId here caused orphan detection to never match spawned tasks. | ||
| return new Set(registry.keys()); |
There was a problem hiding this comment.
🔵 unsafe_assumptions: The new getActiveSessionIds returns ALL clientIds from the registry via registry.keys(), including regular browser-connected sessions. Previously it only returned SDK sessionIds from active sessions. This means orphan detection now compares task session_id (which stores clientIds like task:abc123) against ALL registry keys (which also include browser client IDs). This works correctly because browser clientIds will never match a task:* sessionId stored on a task, but it's a broader set than strictly necessary. Not a bug — just a noted semantic change.
dimakis
left a comment
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 2 issue(s) (2 warning).
server/index.ts
The core fix (clientId-based orphan detection replacing broken sessionId matching) is correct and well-tested with comprehensive edge-case coverage; the one gap is that .finally() cannot distinguish user-initiated pause from auto-pause, which could override a manual pause.
- 🟡 bugs (L286): The
.finally()handler callsresume()wheneverstate === 'paused', but cannot distinguish between auto-pause (all tasks dispatched — correct to resume) and user-initiatedpause()(should stay paused). If a user manually pauses the orchestrator while spawned sessions are in-flight, the next session to end will callresume()and override the user's intent. Consider tracking amanuallyPausedflag on the orchestrator, or changing the.finally()to always calltick()— and havingtick()internally handle orphan reclaim even in paused state.[fixable]
server/__tests__/task-orchestrator.test.ts
The core fix (clientId-based orphan detection replacing broken sessionId matching) is correct and well-tested with comprehensive edge-case coverage; the one gap is that .finally() cannot distinguish user-initiated pause from auto-pause, which could override a manual pause.
- 🟡 missing_tests: No test for 'user manually pauses while a spawned session is in-flight; session ends; orchestrator should remain paused'. The existing 'does not resume old goal when paused orchestrator moved to a new goal' test covers the goal-change guard but not the same-goal manual-pause scenario. This is the complement of the auto-pause/resume test at line 939 and would expose the warning above.
[fixable]
| orchestratorRef.tick(); | ||
| } | ||
| } else { | ||
| log.info('task session ended after loop stopped or goal changed', { |
There was a problem hiding this comment.
🟡 bugs: The .finally() handler calls resume() whenever state === 'paused', but cannot distinguish between auto-pause (all tasks dispatched — correct to resume) and user-initiated pause() (should stay paused). If a user manually pauses the orchestrator while spawned sessions are in-flight, the next session to end will call resume() and override the user's intent. Consider tracking a manuallyPaused flag on the orchestrator, or changing the .finally() to always call tick() — and having tick() internally handle orphan reclaim even in paused state. [fixable]
1 participant
Summary
getActiveSessionIds()returned SDKsessionIdUUIDs buttask.session_idstoresclientId— they never matched, so spawned tasks were never reclaimed when their session diedtaskContextset, so the session overview couldn't show which task a session was working onorchestrator.tick()— the workflow stayed frozen on the completed task foreverTest plan
reclaims spawned task sessions using clientId matching— verifiestask:prefixed clientIds are correctly matched in orphan detection🤖 Generated with Claude Code