fix(space): attach space-agent-tools to post-approval merger session [#852] - #2379
Conversation
…852] post-approval spawns (e.g. the built-in `merger`) built init.mcpServers with only node-agent + agent-memory. Such a session carries no NodeExecution row and its id has no `:exec:` segment, so resolveSpaceMcpSessionPolicy classifies it as an ad_hoc_member — and QueryRunner.ensureMemberSpaceMcpInvariant therefore REQUIRES `space-agent-tools` on it. The missing server made the merger's first turn throw the MCP-invariant error, which approvePendingCompletion misrecorded as "Interrupted by user". #850/#2374 fixed a separate reuse-path issue and never touched this omission, so the operator saw the same failure after merging it. Attach `space-agent-tools` in spawnPostApprovalSubSession via the SAME builder attachSpaceToolsToMemberSession uses (extracted to SpaceRuntimeService.buildMemberSpaceToolsMcpServer — no hand-rolled server). init.mcpServers is merged into the session's runtime MCP map inside createSubSession BEFORE startStreamingQuery, so the server is present when runQuery runs the invariant at first turn (race-free, unlike a post-create merge which can lose to runQuery's check). Also wire spawned.onMissingMemberSpaceMcpServers -> reattachMemberSpaceTools. Sub-sessions are created via AgentSession.fromInit, which — unlike SessionManager.createAgentSessionFromSession — does not wire this callback, so attach it explicitly so a future regression (cache eviction / DB reload) self -heals instead of throwing. Normal node-agent workflow-worker sessions are untouched: they remain workflow_worker (require only node-agent) and never hit this spawn path.
] - space-mcp-session-policy: pin that a post-approval session id (no `:exec:` segment, no NodeExecution row) resolves to ad_hoc_member requiring space-agent-tools — the invariant the spawn must satisfy. Documents why the bug occurred. - task-agent-manager-post-approval-fresh-session: drive the CREATE branch of spawnPostApprovalSubSession and assert `space-agent-tools` reaches the session's runtime MCP map (the map ensureMemberSpaceMcpInvariant reads) via the shared buildMemberSpaceToolsMcpServer, and that the self-heal callback is wired to reattachMemberSpaceTools. Verified to fail without the fix.
lsm
left a comment
There was a problem hiding this comment.
🤖 Review by glm-5.1 (GLM)
Model: glm-5.1 | Client: HyperNeo | Provider: GLM (Zhipu AI)
Recommendation: APPROVE — zero findings. (Posting as COMMENT because the workflow bot and this review share a GitHub author; GitHub rejects self-APPROVE.)
Verified the root cause and the fix against the actual code paths (not just the diff), with three independent traces plus empirical test runs.
Root cause confirmed. A post-approval session id (space:S:task:T:post-approval:agent) has no NodeExecution row and no :exec: segment, so resolveSpaceMcpSessionPolicy classifies it ad_hoc_member → requiredServers = ['space-agent-tools'], which ensureMemberSpaceMcpInvariant enforces. The old spawnPostApprovalSubSession attached only node-agent + agent-memory, so the first turn threw — misrecorded as "Interrupted by user" by approvePendingCompletion. Distinct from the reuse-path bug #850/#2374 fixed.
Fix is correct and race-free. space-agent-tools is added to init.mcpServers via the shared buildMemberSpaceToolsMcpServer (extracted from attachSpaceToolsToMemberSession; extraction is faithful — only mySessionId: session.id → sessionId). Traced the ordering: createSubSession merges init.mcpServers into the runtime MCP map synchronously at task-agent-manager.ts:1460, before startStreamingQuery at :1523; the invariant at query-runner.ts:1552 reads that same live map. Server is present at first turn.
Self-heal genuinely wired. onMissingMemberSpaceMcpServers → reattachMemberSpaceTools. The callback exists on AgentSession, is invoked+awaited by the invariant before throwing, and reattachMemberSpaceTools re-resolves policy (still ad_hoc_member for post-approval) and re-attaches. The long-horizon guard in attachSpaceToolsToMemberSession cannot skip post-approval (different id format).
Normal workers untouched. Worker spawns keep their :exec: id → workflow_worker → require only node-agent; post-approval path is disjoint, requiredWorkflowSubSessionMcpServers unaffected. No double-attach/overwrite risk (mergeRuntimeMcpServers is per-key; nothing else attaches space-agent-tools here).
Empirical:
- New policy test + CREATE-branch regression test pass (15/15).
- Reverted just the source fix and re-ran the regression test → it fails (
space-agent-toolsundefined in the merged map), confirming it pins the real bug. - lint + typecheck + knip + session-guards clean; CI fully green (all daemon unit + online shards).
One unrelated note (not a finding against this PR): the local bun run check fails its trailing check:test-quality step on provider-registry.test.ts:748 — pre-existing on origin/dev (last touched by #2264), in a file this PR doesn't touch, and not part of the PR CI gate. Worth a separate cleanup.
At this space's autonomy level the merge still needs human sign-off to finalize.
…kspace import, dead config P1 (round-5): tests/online/space/task-agent-skills.test.ts Test 1 asserted the old behavior (registry server copied into config.mcpServers). Updated to assert only genuine runtime servers (node-agent, agent-memory) are present and the registry server is NOT copied (it flows via the builder at query time now). P2 (startup race): QueryRunner.start() is per-session (guarded by messageQueue.isRunning), so options are built once; an mcp.registry.changed / skills.changed arriving between that build and queryObject assignment was dropped and never re-applied. QueryRunner now re-pushes the effective MCP set right after assigning queryObject (ACP skipped). Closes the per-session reconcile gap. P2 (workspace.add import): workspace.add calls refreshFromFile but didn't emit mcp.registry.changed; now emits on added/updated/removed > 0, mirroring settings.mcp.refreshImports. P3: dropped the now-dead appMcpManager field from TaskAgentManagerConfig (and its construction + now-unused import). Also merges dev (incl. #2379 space-agent-tools post-approval).
Post-approval spawns (e.g. the built-in
merger) builtinit.mcpServerswith onlynode-agent+agent-memory. A post-approval session carries no NodeExecution row and its id has no:exec:segment, soresolveSpaceMcpSessionPolicyclassifies it asad_hoc_member— andQueryRunner.ensureMemberSpaceMcpInvarianttherefore requiresspace-agent-toolson it. The missing server made the merger's first turn throw the MCP-invariant error, whichapprovePendingCompletionmisrecorded as "Interrupted by user". #850/#2374 fixed a separate reuse-path issue and never touched this omission, which is why the same failure recurred after merging it.This attaches
space-agent-toolsinspawnPostApprovalSubSessionvia the same builderattachSpaceToolsToMemberSessionuses (extracted toSpaceRuntimeService.buildMemberSpaceToolsMcpServer— no hand-rolled server). Becauseinit.mcpServersis merged into the session's runtime MCP map insidecreateSubSessionbeforestartStreamingQuery, the server is present whenrunQueryruns the invariant at first turn — race-free. It also wiresonMissingMemberSpaceMcpServers → reattachMemberSpaceToolsso a future regression self-heals instead of throwing. Normal workflow-worker node-agent sessions are untouched (they remainworkflow_worker, require onlynode-agent, and never hit this spawn path).Recovery for already-parked tasks (#785, #816, #848, …): their dispatch already failed, so they still need a manual Mark done or a send-back → re-approve to exercise the now-working path.