Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dimakis merged 3 commits into from
Jul 4, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
84f1fa6
fix(task-board): set task context on spawned sessions
dimakis Jul 2, 2026
2da3ca3
fix(task-board): pass taskContext to startChat for spawned sessions
dimakis Jul 2, 2026
f11ab31
refactor(task-board): consolidate taskContext ownership in startChat
dimakis Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions server/__tests__/task-orchestrator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -839,6 +839,49 @@ describe('TaskOrchestrator', () => {
expect(deps.setTaskContext).not.toHaveBeenCalled();
});

@dimakis dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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]

it('does not call setTaskContext (pinned) for spawned sessions', async () => {
const spawnSession = vi.fn().mockResolvedValue('spawned-client-1');
const deps = createTestDeps(store);
deps.spawnSession = spawnSession;
const orch = new TaskOrchestrator(deps);

const goal = store.create({ title: 'Goal' });
store.create({
title: 'Spawn task',
parentId: goal.id,
sessionPolicy: 'spawn',
});

orch.start(goal.id);

await vi.waitFor(() => {
expect(spawnSession).toHaveBeenCalled();
});

// Spawned sessions get taskContext via startChat options, not setTaskContext
expect(deps.setTaskContext).not.toHaveBeenCalled();
});

@dimakis dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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]

it('falls back to setTaskContext (pinned) when spawn returns null', async () => {
const spawnSession = vi.fn().mockResolvedValue(null);
const deps = createTestDeps(store);
deps.spawnSession = spawnSession;
const orch = new TaskOrchestrator(deps);

const goal = store.create({ title: 'Goal' });
const task = store.create({
title: 'Spawn task',
parentId: goal.id,
sessionPolicy: 'spawn',
});

orch.start(goal.id);

await vi.waitFor(() => {

@dimakis dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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]

expect(deps.setTaskContext).toHaveBeenCalledWith(task.id, goal.id);
});
});

it('reuse policy tasks use pinned session as before', () => {
const spawnSession = vi.fn().mockResolvedValue('spawned-client-1');
const deps = createTestDeps(store);
Expand Down
5 changes: 5 additions & 0 deletions server/chat.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,7 @@ export async function startChat(
onSessionResolved?: (sessionId: string) => void;
telosTaskId?: string;
agentName?: string;
taskContext?: { currentTaskId: string; goalId: string };
},
) {
return withSpanAsync(
Expand Down Expand Up @@ -775,6 +776,7 @@ async function _startChatInner(
onSessionResolved?: (sessionId: string) => void;
telosTaskId?: string;
agentName?: string;
taskContext?: { currentTaskId: string; goalId: string };
},
) {
const abortController = new AbortController();
Expand Down Expand Up @@ -862,6 +864,9 @@ async function _startChatInner(
const session = registry.get(clientId)!;
session.model = options.model ?? session.model;

@dimakis dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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]

session.inputQueue = inputQueue as { push: (msg: unknown) => void; close: () => void };

@dimakis dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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]

if (options.taskContext) {

@dimakis dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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]

session.taskContext = options.taskContext;
}
_onSessionChange?.(clientId, 'start');

// Session state machine: mark CREATED (Phase 1 — write only, no behavior change)
Expand Down
1 change: 1 addition & 0 deletions server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ const orchestrator = new TaskOrchestrator({
mode: 'agent',
isolation: true,
telosTaskId: goalId,
taskContext: { currentTaskId: taskId, goalId },

@dimakis dimakis Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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 dimakis Jul 4, 2026

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 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.

onSessionResolved: (sessionId) => {
log.info('spawned headless session resolved', { taskId, sessionId, clientId });
sseRegistry.broadcast('sessions_changed', {});
Expand Down
Loading