fix(task-board): set task context on spawned sessions

[dimakis]

Summary

• Spawned task sessions never had taskContext set on their registry entry, so TaskComplete calls from the agent hit a 400 error and tasks stayed stuck on "running" forever
• Added setTaskContextForClient to OrchestratorDeps — sets task context on a specific clientId (not just the pinned client)
• Called it in the spawn success path right after setSessionId, so spawned agents can complete tasks and trigger the orchestrator to advance

Test plan

• New test: sets task context on spawned session via setTaskContextForClient
• New test: does not set task context when spawn returns null (fallback path)
• All 48 orchestrator tests pass
• No regressions in full suite (pre-existing failures unchanged)
• Deploy and test with a real PR shepherd workflow — verify tasks advance past "running"

🤖 Generated with Claude Code

[dimakis]

Centaur Review

LGTM — no issues found.

[dimakis]

Centaur Review

Found 1 issue(s).

server/task-orchestrator.ts

Clean, well-tested fix that correctly sets task context on spawned sessions. The optional dependency is backward-compatible, the implementation mirrors the existing setTaskContext pattern, and both success and fallback paths are covered by tests. One pre-existing timing gap noted (MCP tools not wired up for spawned sessions) but not introduced by this PR.

• 🔵 unsafe_assumptions (L370): Pre-existing timing gap (not introduced by this PR): setTaskContextForClient runs in the .then() after spawnSession resolves, but startChat (fire-and-forget inside spawnSession) reads taskContext synchronously in buildMcpAllowedTools (chat.ts:838) and buildTaskMcpServer (chat.ts:918) — both before the first await. So spawned sessions get the task prompt in the system prompt (chat.ts:962, after await — works correctly), but the task-board MCP server and its tool allowlist are not wired up. The agent is told about its task but can't call TaskComplete/TaskStatus tools. A future fix could pass taskContext as a startChat option so it's available during synchronous setup. [fixable]

[dimakis]

🔵 unsafe_assumptions: Pre-existing timing gap (not introduced by this PR): setTaskContextForClient runs in the .then() after spawnSession resolves, but startChat (fire-and-forget inside spawnSession) reads taskContext synchronously in buildMcpAllowedTools (chat.ts:838) and buildTaskMcpServer (chat.ts:918) — both before the first await. So spawned sessions get the task prompt in the system prompt (chat.ts:962, after await — works correctly), but the task-board MCP server and its tool allowlist are not wired up. The agent is told about its task but can't call TaskComplete/TaskStatus tools. A future fix could pass taskContext as a startChat option so it's available during synchronous setup. [fixable]

[dimakis]

Centaur Review

Found 2 issue(s) (1 warning).

server/chat.ts

Correct fix for missing task context on spawned sessions, but buildMcpAllowedTools ordering means mcp__task-board__* won't be in allowedTools — the MCP server starts but its tools may be inaccessible to the agent.

• 🟡 bugs (L838): buildMcpAllowedTools(clientId) is called at line 838, before the session is registered (line 848) and before taskContext is set (~line 865 via options.taskContext). So registry.get(clientId) returns null and mcp__task-board__* is never added to mcpAllowed. At line 1018 the computed allowedTools array won't include the task-board tool pattern. Meanwhile buildTaskMcpServer(clientId) at line 918 correctly sees taskContext and configures the MCP server. Result: the task-board MCP server starts but its tools may be filtered out by the SDK because they're not in allowedTools. Fix: move buildMcpAllowedTools to after taskContext is set, or pass options.taskContext as a parameter so it doesn't need the registry lookup. [fixable]

server/index.ts

Correct fix for missing task context on spawned sessions, but buildMcpAllowedTools ordering means mcp__task-board__* won't be in allowedTools — the MCP server starts but its tools may be inaccessible to the agent.

• 🔵 style: Task context is set on spawned sessions via two independent mechanisms: (1) taskContext in startChat options (line 255, sets during session init before system prompt build) and (2) setTaskContextForClient called from the orchestrator's .then() callback (sets after spawn resolves). Due to async execution, _startChatInner runs synchronously through registry.register() and session.taskContext = options.taskContext before spawnSession returns clientId, so both paths set the same value. Path (1) alone is sufficient. Path (2) is harmless redundancy but adds interface surface (setTaskContextForClient on OrchestratorDeps) for no additional safety. Consider whether the belt-and-suspenders complexity is justified.

[dimakis]

Centaur Review

Found 2 issue(s).

server/index.ts

Clean, well-tested fix. The only concern is a redundant double-set of taskContext (once via startChat options, once via setTaskContextForClient) — harmless but worth consolidating for clarity.

• 🔵 style (L261): Redundant double-set: taskContext is passed to startChat (line 261), which sets session.taskContext synchronously during registration. Then setTaskContextForClient (called from the orchestrator's .then() callback) sets the same value again. Both paths always set identical values. Consider removing the taskContext option from the startChat call and relying solely on setTaskContextForClient, or vice versa, to make the ownership clear. [fixable]

server/__tests__/task-orchestrator.test.ts

Clean, well-tested fix. The only concern is a redundant double-set of taskContext (once via startChat options, once via setTaskContextForClient) — harmless but worth consolidating for clarity.

• 🔵 missing_tests (L883): The fallback test asserts setTaskContext was called but doesn't verify the arguments (task.id, goal.id). Adding toHaveBeenCalledWith(task.id, goal.id) would strengthen the assertion and guard against argument-order regressions. [fixable]

[dimakis]

🔵 style: Redundant double-set: taskContext is passed to startChat (line 261), which sets session.taskContext synchronously during registration. Then setTaskContextForClient (called from the orchestrator's .then() callback) sets the same value again. Both paths always set identical values. Consider removing the taskContext option from the startChat call and relying solely on setTaskContextForClient, or vice versa, to make the ownership clear. [fixable]

[dimakis]

🔵 missing_tests: The fallback test asserts setTaskContext was called but doesn't verify the arguments (task.id, goal.id). Adding toHaveBeenCalledWith(task.id, goal.id) would strengthen the assertion and guard against argument-order regressions. [fixable]

[dimakis]

Centaur Review

Found 1 issue(s).

server/task-orchestrator.ts

Clean, well-tested fix that correctly propagates task context to spawned sessions via startChat options. The setTaskContextForClient dep is redundant with the startChat path but harmless.

• 🔵 style (L370): Redundant task context assignment. spawnSession in index.ts already passes taskContext to startChat (line 261), which sets session.taskContext synchronously before _startChatInner's first await (line 1059). By the time this .then() runs, session.taskContext is already set to the identical value. The two mechanisms are defensible as belt-and-suspenders, but a comment explaining the intentional redundancy would help future readers understand which is the primary mechanism (the startChat option, since it's set before the system prompt is built at line 967).

[dimakis]

🔵 style: Redundant task context assignment. spawnSession in index.ts already passes taskContext to startChat (line 261), which sets session.taskContext synchronously before _startChatInner's first await (line 1059). By the time this .then() runs, session.taskContext is already set to the identical value. The two mechanisms are defensible as belt-and-suspenders, but a comment explaining the intentional redundancy would help future readers understand which is the primary mechanism (the startChat option, since it's set before the system prompt is built at line 967).

[dimakis]

Centaur Review

LGTM — no issues found.

[dimakis]

Centaur Review

LGTM — no issues found.

[dimakis]

Centaur Review

Found 3 issue(s) (1 warning).

server/chat.ts

Clean, well-scoped fix. The taskContext assignment ordering relative to _onSessionChange is worth checking; test coverage is good for the orchestrator but doesn't verify the chat.ts integration end of the change.

• 🟡 regressions (L867): The taskContext assignment happens after _onSessionChange?.(clientId, 'start') is emitted in the diff's line ordering. If any _onSessionChange listener reads session.taskContext (e.g. to broadcast session info), it will see null for spawned sessions. Consider moving the taskContext assignment before the _onSessionChange call to match the pattern of session.model and session.inputQueue which are also set before the notification. [fixable]

server/__tests__/task-orchestrator.test.ts

Clean, well-scoped fix. The taskContext assignment ordering relative to _onSessionChange is worth checking; test coverage is good for the orchestrator but doesn't verify the chat.ts integration end of the change.

• 🔵 missing_tests: No test verifies that startChat actually receives the taskContext option from the spawnSession callback. The orchestrator tests mock spawnSession entirely, and there's no integration test or unit test on chat.ts confirming that options.taskContext is written to session.taskContext. A unit test on _startChatInner (or at minimum a test in index.ts verifying the spawnSession closure passes taskContext) would close this gap. [fixable]
• 🔵 missing_tests: No test covers the spawn-throws path (.catch handler in orchestrator) to verify that setTaskContext is NOT called when spawnSession rejects. The existing tests cover success and null-return, but not the rejection case. [fixable]

[dimakis]

🟡 regressions: The taskContext assignment happens after _onSessionChange?.(clientId, 'start') is emitted in the diff's line ordering. If any _onSessionChange listener reads session.taskContext (e.g. to broadcast session info), it will see null for spawned sessions. Consider moving the taskContext assignment before the _onSessionChange call to match the pattern of session.model and session.inputQueue which are also set before the notification. [fixable]

[dimakis]

Centaur Review

LGTM — no issues found.

[dimakis]

Centaur Review

Found 2 issue(s) (1 warning).

server/__tests__/task-orchestrator.test.ts

Correct fix — spawned sessions will now get task context, MCP server, and system prompt. The two new orchestrator tests are near-duplicates of existing tests; the actual new behavior (startChat setting session.taskContext from options) lacks test coverage.

• 🟡 missing_tests: Both new orchestrator tests duplicate existing tests. 'does not call setTaskContext (pinned) for spawned sessions' duplicates 'auto policy (store default) spawns instead of reusing' (line 824) — both assert setTaskContext is not called on successful spawn. 'falls back to setTaskContext (pinned) when spawn returns null' duplicates 'falls back to pinned session when spawnSession returns null' (line 656) — both mock spawnSession returning null and assert setTaskContext is called. The actual new behavior — startChat propagating options.taskContext onto session.taskContext so that buildTaskMcpServer and buildTaskPromptForSession pick it up — has no test coverage anywhere. [fixable]

server/chat.ts

Correct fix — spawned sessions will now get task context, MCP server, and system prompt. The two new orchestrator tests are near-duplicates of existing tests; the actual new behavior (startChat setting session.taskContext from options) lacks test coverage.

• 🔵 style (L865): The placement between session.inputQueue and _onSessionChange is fine functionally but groups a task-orchestration concern with low-level session plumbing. Consider placing it near the session.model assignment (line 863) or after _onSessionChange with a brief comment linking it to the task-board MCP server and system prompt that read it downstream (lines 918, 962). Minor — the current placement works correctly. [fixable]

[dimakis]

🔵 style: The placement between session.inputQueue and _onSessionChange is fine functionally but groups a task-orchestration concern with low-level session plumbing. Consider placing it near the session.model assignment (line 863) or after _onSessionChange with a brief comment linking it to the task-board MCP server and system prompt that read it downstream (lines 918, 962). Minor — the current placement works correctly. [fixable]

[dimakis]

Centaur Review

Found 4 issue(s) (1 warning).

server/chat.ts

The wiring fix is correct and ordering in _startChatInner is sound, but the actual behavior change (startChat propagating taskContext to the session) lacks direct test coverage — the two new orchestrator tests duplicate existing mocked assertions rather than testing the real integration point.

• 🟡 missing_tests (L866): The core fix — startChat accepting taskContext and setting session.taskContext — has no test. The new orchestrator tests only verify mocked setTaskContext calls, but nothing validates that startChat actually propagates taskContext to the session object, or that buildTaskMcpServer/buildTaskPromptForSession see it. An integration test (or a focused unit test of _startChatInner with a fake registry) would cover the real behavior change. [fixable]

server/__tests__/task-orchestrator.test.ts

The wiring fix is correct and ordering in _startChatInner is sound, but the actual behavior change (startChat propagating taskContext to the session) lacks direct test coverage — the two new orchestrator tests duplicate existing mocked assertions rather than testing the real integration point.

• 🔵 missing_tests (L841): The new test 'does not call setTaskContext (pinned) for spawned sessions' duplicates existing coverage: the test at line 824 ('auto policy spawns instead of reusing') already asserts expect(deps.setTaskContext).not.toHaveBeenCalled() with an identical mock setup (spawnSession returns a clientId). The only difference is explicit sessionPolicy: 'spawn' vs. the default 'auto', but both resolve to the same code path (line 346: const policy = next.sessionPolicy === 'reuse' ? 'reuse' : 'spawn'). [fixable]
• 🔵 missing_tests (L864): The new test 'falls back to setTaskContext (pinned) when spawn returns null' duplicates the existing test at line 656 ('falls back to pinned session when spawnSession returns null'), which also mocks spawnSession returning null and asserts expect(deps.setTaskContext).toHaveBeenCalledWith(task.id, goal.id). The existing test additionally checks activeTaskId, making the new one strictly a subset. [fixable]

server/index.ts

The wiring fix is correct and ordering in _startChatInner is sound, but the actual behavior change (startChat propagating taskContext to the session) lacks direct test coverage — the two new orchestrator tests duplicate existing mocked assertions rather than testing the real integration point.

• 🔵 style (L258): Both telosTaskId: goalId (line 257) and taskContext: { currentTaskId: taskId, goalId } (line 258) now pass goalId to startChat via two separate paths. telosTaskId is used for event-store metadata while taskContext drives MCP tools and system prompt. The overlap is intentional but worth a brief inline note to prevent a future reader from consolidating them.

[dimakis]

🟡 missing_tests: The core fix — startChat accepting taskContext and setting session.taskContext — has no test. The new orchestrator tests only verify mocked setTaskContext calls, but nothing validates that startChat actually propagates taskContext to the session object, or that buildTaskMcpServer/buildTaskPromptForSession see it. An integration test (or a focused unit test of _startChatInner with a fake registry) would cover the real behavior change. [fixable]

[dimakis]

🔵 missing_tests: The new test 'does not call setTaskContext (pinned) for spawned sessions' duplicates existing coverage: the test at line 824 ('auto policy spawns instead of reusing') already asserts expect(deps.setTaskContext).not.toHaveBeenCalled() with an identical mock setup (spawnSession returns a clientId). The only difference is explicit sessionPolicy: 'spawn' vs. the default 'auto', but both resolve to the same code path (line 346: const policy = next.sessionPolicy === 'reuse' ? 'reuse' : 'spawn'). [fixable]

[dimakis]

🔵 missing_tests: The new test 'falls back to setTaskContext (pinned) when spawn returns null' duplicates the existing test at line 656 ('falls back to pinned session when spawnSession returns null'), which also mocks spawnSession returning null and asserts expect(deps.setTaskContext).toHaveBeenCalledWith(task.id, goal.id). The existing test additionally checks activeTaskId, making the new one strictly a subset. [fixable]

[dimakis]

🔵 style: Both telosTaskId: goalId (line 257) and taskContext: { currentTaskId: taskId, goalId } (line 258) now pass goalId to startChat via two separate paths. telosTaskId is used for event-store metadata while taskContext drives MCP tools and system prompt. The overlap is intentional but worth a brief inline note to prevent a future reader from consolidating them.