fix: MCP integration robustness — server lifecycle, diagnostics, log hygiene, terminal startup hardening - #362
Merged
Conversation
reneleonhardt
force-pushed
the
fix/acp-mcp-infra
branch
from
August 8, 2026 08:45
538c708 to
4af6e4f
Compare
Contributor
|
The |
- Baseline mcp.json servers using url transport are logged as outside the stdio-only advertisement (informational only: the baseline is user-trusted; client-supplied http servers are rejected before connect). - Client-supplied MCP server spawns (arbitrary stdio processes with the user's privileges) are logged at connect time. - Server names and errors from clients/config are sanitized before logging, closing a log-injection vector via newline control characters. Test: TestSanitizeLogName. Co-authored-by: GPT-5.6 Sol <codex@openai.com>
The MCP SDK only wires the command's stdout and stdin, leaving stderr untouched. Capture it in the stdio transport so a fast server failure is diagnosed from the original spawn's output, instead of re-running the command (stdioCheck) — removing the extra spawn per failed server. Test: TestManagerCapturesStdioStderrOnFastFailure. Co-authored-by: GPT-5.6 Sol <codex@openai.com>
…efusals A server that fails to start (e.g. codemap refusing without --allow-spawn) was re-spawned on every session/new of a long-lived host. Cache failed server identities per process and skip them in later sessions, logging "previously failed, skipping". Define the standardized consent-refusal marker (MCP-CONSENT-REFUSED) that whale-ecosystem MCP servers emit when spawned without explicit consent; whale-acp matches it to log the refusal clearly. Tests: TestWireMCPServersNegativeCacheSkipsFailedServer. Co-authored-by: GPT-5.6 Sol <codex@openai.com>
…switches -race flagged App.loadPromotedToolState reading a.sessionID (MCP startup goroutine via InitializeMCP -> RestorePromotedTools) while ApplyResumeChoice writes it on the dispatch goroutine; session/new and fork write the same field unsynchronized. Add sessionMu guarding sessionID: all writers route through setSessionID (resume, session-new, fork), SessionID() reads under the lock, and the async MCP readers (writePromotedToolState, loadPromotedToolState) take a consistent sessionPath() snapshot instead of touching the fields raw. sessionsDir is immutable after construction and stays lock-free. Tests: new TestSessionIDConcurrentReadWrite (4x reader / 4x writer under -race); resume-vs-hydration service test now passes -race -count=2.
RestorePromotedTools populated the promotedTools map AFTER rebuildToolRegistriesLocked, so collectPromotedToolsLocked never saw the restored tools: the rebuild replaced the registry without them and the next rebuild pruned them as stale. Restore-on-resume was silently a no-op. promoteToolsLocked already tracks promoted tools before rebuild (its comment states the invariant); RestorePromotedTools now matches that order. Tests: new TestRestorePromotedToolsSuccess asserts the restored tool is in the registry and promotedTools map — failed before this fix. Also add unit coverage for loadPromotedToolState (missing file, stale hash, valid, malformed JSON, nil app), writePromotedToolState (nil app, empty path, round-trip), RestorePromotedTools no-state/nil paths, and the setSessionID/SessionID/sessionPath accessors.
isStarting() turns true as soon as the starting CAS lands in Start(), before the spawn block stored c.procDone. A Close racing that window could return with procDone == nil, so TestCloseWaitsForProcessExit flaked on CI with "no process was tracked" even though the process was spawning fine. Publish the procDone channel at CAS time and close the same channel from the reaper goroutine. Once a start attempt is in flight, Close can always observe a process-tracking channel; the existing bounded wait on it keeps the reap guarantee (an unreaped process holds workspace-directory handles on Windows). Test: TestCloseWaitsForProcessExit -count=30 green; full internal/lsp -count=3 green.
reneleonhardt
force-pushed
the
fix/acp-mcp-infra
branch
from
August 10, 2026 14:22
4af6e4f to
e77f2a5
Compare
Contributor
Author
|
The "Run tests" failure traces to a pre-existing issue on main, not to this branch's changes. The stderr-enrichment path in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A long-lived ACP host creates a session per conversation; each
session/newre-spawned the session's MCP servers — including ones that failed to start (e.g. consent-refusing codemap without--allow-spawn) — causing spawn storms and log spam. Failed stdio servers were diagnosed by re-running the command (an extra spawn per failure), and client-supplied names/errors went raw into logs (log-injection vector). This branch hardens MCP integration across the stack: server lifecycle (cmd/whale-acp+internal/mcp) and the terminal app's MCP startup path (internal/app).Changes (in logical, commit order)
url-transport servers logged as outside the stdio-only advertisement; client-supplied spawns logged at connect time; names/errors sanitized (sanitizeLogName, control chars stripped — closes log injection).boundedStderrcaptures server stderr, so fast failures are diagnosed without re-spawning (stdioCheckfallback stays for empty captures).MCP-CONSENT-REFUSEDmarker is classified in logs.-racefound MCP startup readingsessionIDwhile resume/new/fork write it; newsessionMuserializes writers (setSessionID), the accessor, and async readers (sessionPath()snapshot);sessionsDirstays lock-free.RestorePromotedToolspopulatedpromotedToolsafter the registry rebuild, so restored tools were dropped and pruned as stale — restore-on-resume was a silent no-op. Now tracks before rebuild, matchingpromoteToolsLocked.Why these packages
Robustness spans the MCP stack:
whale-acpwires client-supplied servers,internal/mcpowns spawn + diagnostics (shared with the terminal app),internal/appowns the terminal runtime's MCP tool restore. This branch covers only the server-robustness half; the permission-dialog UX is tracked separately.Validation
go test ./...suite green;-racegreen oncmd/whale-acp,internal/mcp,internal/app.-race), negative-cache (skip, keyed-by-command, consent-refused, concurrent), log hygiene (sanitize, sorted keys, session log lines), app startup (TestSessionIDConcurrentReadWrite,TestSessionIDAccessors,TestRestorePromotedToolsSuccess— failed before the ordering fix).Out of scope
The ACP permission-confirmation UX (dialog content, scoped always-allow, auto-allow read-only shell, v2 permission-response parsing) has been implemented separately; this branch covers only the MCP server-robustness half.
User-visible impact
whale: session resume re-promotes MCP tools (was silently dropped);SessionID()race-free under MCP startup.Breaking changes
None.
Notes for reviewers
boundedStderris concurrency-safe because os/exec's copy goroutine writes while the parent reads after a failed Connect; the cap keeps only the tail (the lines that explain a failure).sessionMuandtoolMuare never nested in both directions — no deadlock surface.Follow-up hardening (internal/lsp)
The CI run for this PR surfaced a one-off flake in
internal/lsp(TestCloseWaitsForProcessExit: "no process was tracked"). Root cause: a start attempt is visible toisStarting()the moment the starting CAS lands, before the spawn block publishedprocDone— aCloseracing that window could return withprocDone == nil.internal/lspnow publishes theprocDonechannel at CAS time and closes the same channel from the reaper goroutine, soClosecan always observe a process-tracking channel for an in-flight start while keeping the bounded reap guarantee. Verified withTestCloseWaitsForProcessExit -count=30and the fullinternal/lsppackage; no otherinternal/lspbehavior changed.Developed with carefully directed, manually reviewed AI assistance.