fix(orchestrator): default session_policy to spawn#415
Conversation
Tasks without an explicit sessionPolicy were defaulting to 'reuse', which injects task board instructions into the user's active interactive session. The spawn infrastructure (PR #377) was built to prevent this but was opt-in. Flip the default so tasks get dedicated headless sessions unless explicitly set to reuse. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 2 issue(s) (1 warning).
server/__tests__/task-orchestrator.test.ts
The change is logically correct in intent but effectively a no-op — TaskStore.create() defaults sessionPolicy to 'auto', which bypasses the ?? fallback. Either the store default or the 'auto' handling in the orchestrator also needs updating to achieve the desired spawn-by-default behavior.
- 🟡 missing_tests: No test verifies the new default. All spawn tests explicitly set
sessionPolicy: 'spawn', and all non-spawn tests either omitsessionPolicy(which the TaskStore defaults to'auto', bypassing the??fallback entirely) or explicitly set'reuse'. A test should cover the edge case wheresessionPolicyis null/undefined to confirm the new'spawn'default fires correctly.[fixable]
server/task-orchestrator.ts
The change is logically correct in intent but effectively a no-op — TaskStore.create() defaults sessionPolicy to 'auto', which bypasses the ?? fallback. Either the store default or the 'auto' handling in the orchestrator also needs updating to achieve the desired spawn-by-default behavior.
- 🔵 regressions (L345): The
??fallback only triggers whennext.sessionPolicyis null/undefined — butTaskStore.create()defaultssessionPolicyto'auto'(task-store.ts:269), and'auto'is neither'spawn'nor'reuse', so it falls through to the reuse (else) branch regardless of this change. In practice this makes the change a no-op for tasks created through the normal store path. If the intent is for tasks without an explicit policy to spawn by default, the store's default at line 269 (input.sessionPolicy ?? 'auto') may also need updating to'spawn', or the orchestrator should handle'auto'explicitly (e.g., treating it as'spawn').[fixable]
| case 'agent_work': | ||
| default: { | ||
| const policy = next.sessionPolicy ?? 'reuse'; | ||
| const policy = next.sessionPolicy ?? 'spawn'; |
There was a problem hiding this comment.
🔵 regressions: The ?? fallback only triggers when next.sessionPolicy is null/undefined — but TaskStore.create() defaults sessionPolicy to 'auto' (task-store.ts:269), and 'auto' is neither 'spawn' nor 'reuse', so it falls through to the reuse (else) branch regardless of this change. In practice this makes the change a no-op for tasks created through the normal store path. If the intent is for tasks without an explicit policy to spawn by default, the store's default at line 269 (input.sessionPolicy ?? 'auto') may also need updating to 'spawn', or the orchestrator should handle 'auto' explicitly (e.g., treating it as 'spawn'). [fixable]
The ?? fallback was a no-op because TaskStore.create() defaults sessionPolicy to 'auto', a truthy string. Now 'auto' explicitly resolves to 'spawn'. Added test covering the auto-to-spawn path and fixed an existing test that implicitly relied on the old default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 2 issue(s).
server/task-orchestrator.ts
Correct fix — resolves auto (the store default) to spawn instead of silently falling through to reuse. Minor style nit on dead-code fallback and one missing status assertion in the new test.
- 🔵 style (L347): The
?? 'spawn'fallback on line 347 is unreachable:TaskStore.create()always defaultssessionPolicyto'auto'(task-store.ts:269), so the field is neverundefinedon a stored task. The null-coalesce is dead code — only theraw === 'auto'branch matters. Not a bug, but it could mislead future readers into thinkingundefinedis a real case. Consider simplifying toconst policy = next.sessionPolicy === 'auto' ? 'spawn' : next.sessionPolicy;or just removing the fallback.[fixable]
server/__tests__/task-orchestrator.test.ts
Correct fix — resolves auto (the store default) to spawn instead of silently falling through to reuse. Minor style nit on dead-code fallback and one missing status assertion in the new test.
- 🔵 missing_tests (L838): The new test asserts
setTaskContextis not called (proving it spawns rather than reuses), but does not verify the task's status is set to'active'after spawn dispatch. Other spawn tests in this file also check status transitions. Consider addingexpect(store.get(task.id)?.status).toBe('active')for parity.[fixable]
| const policy = next.sessionPolicy ?? 'reuse'; | ||
| // Default to spawn: 'auto' and undefined both resolve to 'spawn' | ||
| // so tasks get dedicated sessions unless explicitly set to 'reuse'. | ||
| const raw = next.sessionPolicy ?? 'spawn'; |
There was a problem hiding this comment.
🔵 style: The ?? 'spawn' fallback on line 347 is unreachable: TaskStore.create() always defaults sessionPolicy to 'auto' (task-store.ts:269), so the field is never undefined on a stored task. The null-coalesce is dead code — only the raw === 'auto' branch matters. Not a bug, but it could mislead future readers into thinking undefined is a real case. Consider simplifying to const policy = next.sessionPolicy === 'auto' ? 'spawn' : next.sessionPolicy; or just removing the fallback. [fixable]
| await vi.waitFor(() => expect(spawnSession).toHaveBeenCalled()); | ||
|
|
||
| expect(spawnSession).toHaveBeenCalledWith(task.id, expect.any(String), goal.id); | ||
| expect(deps.setTaskContext).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
🔵 missing_tests: The new test asserts setTaskContext is not called (proving it spawns rather than reuses), but does not verify the task's status is set to 'active' after spawn dispatch. Other spawn tests in this file also check status transitions. Consider adding expect(store.get(task.id)?.status).toBe('active') for parity. [fixable]
…taur) Remove dead-code ?? fallback — sessionPolicy is always set by the store. Simplify to a direct 'reuse' check. Add status assertion to new test for parity with other spawn tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
LGTM — no issues found.
Summary
sessionPolicyfromreusetospawninTaskOrchestrator.tick()spawninfrastructure was already built in PR feat(orchestrator): multi-agent goal execution via session_policy spawn #377 but was opt-in — this makes it the defaultsessionPolicy: 'reuse'still works for tasks that need itTest plan
sessionPolicy: 'reuse'explicitly or don't providespawnSessionin deps, so they fall through correctly)🤖 Generated with Claude Code