🤖 fix: keep MCP out of an unsanitized replacement checkout when sanitize fails - #4673
Conversation
…iscovery before sanitize End-to-end Gate 4 test for #4576 on the real ServiceContainer (TaskService, WorkspaceService, AIService, MCPServerManager) with a real stdio MCP stub that records its start and every request; only the language model is substituted. The replacement's sanitize is held while a renderer-style prompt-catalog request (workspace.mcp.prompts.list) parks on the launch's running init. Nothing reaches the stub or the manager until sanitize returns; afterwards both the catalog request and the launch's first send (getToolsForWorkspace) reach the stub from the replacement's checkout (positive control). Negative controls (not committed): moving sanitize after the send fails the held-state ledger check (getToolsForWorkspace seen); moving init completion before sanitize fails the ordering check (getPromptsForWorkspace before sanitized). Refs #4576 --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs= -->
… reclaim On the reserved launch's failed-sanitize branch, init was completed before reclaimUnsanitizedTaskCheckout unpublished the row and deleted the checkout. Completing init releases requests parked in waitForInit, so a prompt-catalog request (workspace.mcp.prompts.list) could start MCP servers inside the unsanitized checkout while the row was still published. Init now ends only after the reclaim attempt (try/finally). A reclaimed task drops its init state like workspace removal does; completing it would recreate the deleted session dir to persist init-status.json. A retained checkout (row still published) completes init so waiters do not hang. New deterministic test in taskService.replacementMcpOrdering.test.ts holds the reclaim's unpublish and fails 3/3 before this change (init completed while the reclaim was held). Fixes #4576 --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$4.99`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=4.99 -->
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9330cd7c67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
1 participant
Summary
This adds a real-stack test proving that a workflow replacement launch starts no MCP server and runs no prompt discovery until its checkout is sanitized. The success path already behaves correctly. The test also found an ordering bug when sanitize fails: init was marked complete before the unsanitized checkout was reclaimed, which let a waiting
prompts.liststart MCP in that checkout. This PR fixes it.Fixes #4576.
Background
G2 (#4577, #4593) replaces a workflow child that ended without a report. The replacement launches through
createMany(..., { retires })and the reserved launch path. #4577 checked "sanitize before MCP" only at the TaskService/host seam. #4576 asked for proof against a real MCP server.Findings
MCP and prompt discovery start lazily. They start in exactly two places:
getToolsForWorkspace,listServers,getPrompt.workspace.mcp.prompts.listRPC.Workspace metadata listeners, AgentSession creation, and StreamManager do not start MCP.
The reserved path's send is awaited after sanitize. A client's
prompts.listblocks inwaitForInituntil init completes, and on the success path that happens only after sanitize.Bug on the failed-sanitize branch. It called
logComplete(-1)beforereclaimUnsanitizedTaskCheckout. That released waitingprompts.listcalls while the unsanitized checkout and its published row still existed. In a diagnostic run that held the row removal, the waiting request started the real stub server in the unsanitized checkout (2 of 2 runs).Implementation
reclaimUnsanitizedTaskCheckoutnow reports whether it reclaimed. Init ends in afinallyafter the reclaim attempt:initStateManager.clearInMemoryState(taskId), the same call workspace removal makes. Waiters are released and fail because the workspace is gone.logComplete(-1)later instead would have persistedinit-status.jsonand re-created the deleted session directory.logComplete(-1)runs after the attempt, so waiters do not hang.createMockInitStateManagergainsclearInMemoryState.Validation
taskService.replacementMcpOrdering.test.tsuses the real service container (TaskService, WorkspaceService, AIService, MCPServerManager) and a stdio recording MCP server (tests/fixtures/mcp/recording-server.ts) that is genuinely enabled for the project. Only the language model is stubbed.prompts.listcall is started and seen waiting inwaitForInit. There are no manager calls, the stub stays silent, andendInithas not run.Negative controls:
Local runs:
mcpIdentity.assembly, and the replacementRace/replacementRestart tests: 321.make static-check: green.Risks
Low. The change only affects the failed-sanitize branch of the reserved launch.
Still open and unchanged: when the reclaim itself fails, the unsanitized checkout stays published, and a later request can still start MCP there. That exposure existed before this PR. So did the case where checkout deletion fails after the unpublish. Both are tracked in #4674, which proposes fail-closed discovery and sends for a retained unsanitized checkout. The direct-create and deferred-checkout rollback paths still complete init after their rollback, which likely re-creates the removed session directory. That is not a sanitize-ordering issue, so it is left for a separate change.
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$253.94