Round parameters: Subject=Architect, Griller=Griller (both deepseek-v4-flash), Topic=full scope (diagnosis + plan, premise challenge allowed), Cap=7 questions (early conclusion at Q7), Closing=verdict + post-grill position.
The issue's current-state claims are accurate, with two line-level corrections and one omission noted below. Verified at the current checkout:
- Config —
internal/domain/config/mcp_config.go:MCPServerConfigis exactlyURL,Token,Username,Auth,RequiresConsent *bool,Timeout(struct ~L32-40); noCOMMAND/ARGS. Key regex^[a-z0-9-]{1,24}$at L15.validate()hard-requires a non-empty URL (URL must not be empty, ~L84), which is the rule that must be restructured.EffectiveAuth()maps""→"auto"(~L69-75);EffectiveTimeout()defaults 300s. Validation runs fromConfig.ValidateMCPServers()(internal/domain/config/config.go:270-276) — errors are hard startup failures. One useful detail the issue omits:expandEnvHook(internal/infrastructure/config/config.go:187-190) is a string-kind mapstructure decode hook, so${VAR}interpolation will apply toCOMMAND,ARGSelements,DIR, andENVvalues for free — but it usesos.ExpandEnv(process env), not viper env. - Transport —
internal/infrastructure/mcp/client.go:connect()hard-wires&sdkmcp.StreamableClientTransport{Endpoint:…, HTTPClient:…, DisableStandaloneSSE: true}(inconnect(), ~L170-185), with the "fresh transport per connect attempt" comment — correct per SDKTransportcontract ("used for at most one call to Connect", go-sdkmcp/transport.go:29-33).Close()idempotent + mutex-guarded (~L141-155);operationContextbounds each op (~L195-201); three-way error split inCallTool(~L119-138). - DI —
internal/infrastructure/di/mcp_factory.go: seamnewClient func(endpoint, username, token string, timeout time.Duration) (tools.MCPClient, error)(L31-34, default at L53-68);Buildwarn-and-skips (L79-105);resolveServerTokenreturns(username, token, ok)(L127-160). Catalog pins:resolveServerTokendefault case (L157-160) and theghtoken resolver (L60-65) are ACCEPTED entries indocs/architect/INTENTIONAL_NON_FIXES.md— both will drift when the seam changes and must be re-anchored in the same PR as the partition test (real_nonfix_catalog_test.go) per the coordination rule. - Domain port —
internal/domain/tools/mcp_client.go:MCPClient=ListTools/CallTool/Close, zero third-party deps — transport-agnostic, confirmed. - go-sdk v1.7.0 — verified in module cache:
StdioTransportis hard-wired toos.Stdin/os.Stdout(rwc{os.Stdin, nopCloserWriter{os.Stdout}},mcp/transport.go:114-119) — one per process, so the issue's rejection of it for "many local servers" is correct.IOTransport{Reader io.ReadCloser, Writer io.WriteCloser}(L130-138) — spawnable per server, correct.Client.Connect(ctx, t, opts)(client.go:307) runs the initialize/discover handshake synchronously within ctx, so a timeout context bounds the handshake — supports the "startup bound" requirement. Critical lifecycle fact the issue under-specifies:ClientSession.Close()→conn.Close()→rwc.Close()closes both pipes (transport.go:396-407,client.go:559-581), i.e.session.Close()sends EOF on the child's stdin. The SDK's own server examples useserver.Run(ctx, &mcp.StdioTransport{})(e.g.examples/server/hello/main.go:43) — the issue's integration-test pattern is real. - Plugin —
internal/tools/integrations/mcp/plugin.go: concurrent discovery (L61-89) withdefaultDiscoveryTimeout = 30s, per-server failure non-fatal;RequiresConsent→ToolDeclaration,Serial→ToolOptions(L110-140); namespacingmcp_<server>_<tool>per ADR-067 §5. Omission in the issue: the domain model —MCPServeris defined as "over Streamable HTTP" and invariantmcp-readonly-defaultsis URL-suffix-derived (docs/domain-model/tell-me-go.modelith.md).modelith-checkis a hard gate inmake check/check-full, so the model + rendered.mdmust change or CI fails — the issue's acceptance criteria miss this.
Consent/serial position — I take the issue's preferred option (a): stdio defaults to serial=true, consent=true, with REQUIRES_CONSENT: false opt-out. Rationale: (1) it is the only position consistent with the existing structure — isReadOnly() is strings.HasSuffix(strings.TrimRight(c.URL,"/"), "/readonly"); a COMMAND-based server has no URL, so EffectiveRequiresConsent() and EffectiveSerial() already fall through to !isReadOnly() = true. The "open decision" collapses to making the default explicit and documented rather than accidental; (2) a spawned child process with arbitrary privileges is the highest-consequence tool source in the system — defaulting it to auto-approvable while every other mutating source (non-readonly MCP, plugins) prompts would be the inconsistency, not the reverse; (3) the existing REQUIRES_CONSENT override wins unchanged. I would encode it as an explicit isStdio() helper making isReadOnly() return false for COMMAND servers, plus a new domain-model invariant mcp-stdio-mutating-defaults (or an amendment clause to mcp-readonly-defaults), flagged in the ADR.
Config (mcp_config.go): add Command string, Args []string, Dir string, Env map[string]string. Restructure validate(): exactly one of COMMAND/URL (hasCommand == hasURL → error, replacing the unconditional empty-URL check); ARGS/DIR/ENV without COMMAND → error; explicit AUTH other than none with COMMAND → error (note: EffectiveAuth() defaults ""→auto, so the check must read the raw field — empty tolerated, any explicit non-none value rejected). This is a negative rule, a deliberate deviation from ADR-067 §4's "positive-rule only" credential-normalization philosophy — that philosophy governs stray-TOKEN tolerances, not transport-mode conflicts; I'd document the deviation in the ADR. Stray TOKEN/USERNAME under stdio is tolerated/ignored (consistent with §4), since resolveServerToken short-circuits. Keep the key regex and TIMEOUT rules.
Transport — a separate StdioClient type in internal/infrastructure/mcp/stdio_client.go, same package (shares convertCallToolResult/convertContentItem/toolErrorSummary/marshalInputSchema with the HTTP client without widening ADR-067 confinement), not a mode on Client — the HTTP and exec lifecycles have nothing in common and a mode-on-Client would entangle two cleanup paths in one mutex'd struct. Construction is eager: NewStdioClient(cfg, logger) does exec.CommandContext (ctx owned by the client, cancelable from Close), cmd.Dir, cmd.Env = append(os.Environ(), sorted cfg.Env...), StdoutPipe/StdinPipe (the StdoutPipe-before-Start error is structurally unreachable — same class as the cataloged process_executor.go:143-145 entry), cmd.Stderr = slogWriter{…} (stderr logged, never parsed as JSON-RPC), cmd.Start(), then sdk.Connect(ctx, &sdkmcp.IOTransport{Reader: stdoutPipe, Writer: stdinPipe}, nil) bounded by EffectiveTimeout(). On handshake failure: close both pipes, cancel ctx (CommandContext kills), cmd.Wait() — no dangling child, no retry needed because the factory warn-and-skips. Eager construction (vs. the HTTP client's lazy connect) is deliberate: spawn failures surface at Build as the existing warn+skip, keeping the plugin untouched and making the factory test deterministic. ListTools/CallTool reuse the pre-connected session under operationContext; the three-way error split is unchanged.
Lifecycle/zombie guarantees — Close() idempotent + mutex-guarded: (1) session.Close() first — this closes both pipes via rwc.Close, sending stdin EOF so a well-behaved child exits gracefully (better than the issue's "cancel → close stdin → Wait" ordering, which SIGKILLs first); (2) cancel() as the kill backstop for uncooperative children (CommandContext kills on ctx cancel); (3) cmd.Wait() exactly once (guarded) to reap — this is the zombie-prevention guarantee. factory.Close() already iterates tracked clients, so session teardown kills every child; ADR-067 §10 (MCP_SERVERS read once, excluded from hot-reload) is unchanged — COMMAND/ARGS changes require a new session.
DI (mcp_factory.go): replace the HTTP-shaped seam with a single injectable dispatch newClientFor(serverCfg config.MCPServerConfig) (tools.MCPClient, error) (one testable branching point; the old parameter list is HTTP-shaped and cannot express stdio). Build branches on Command != ""; resolveServerToken returns ("", "", true) immediately for stdio (no credentials). Keep warn+skip. Re-anchor the two catalog pins (L60-65, L157-160) and update the partition test in the same PR.
Docs & model: README MCP section stdio example; ADR-067 amendment (same decision lineage — I'd extend it, not add a new ADR): second transport implementation, lifecycle/zombie guarantees, consent/serial defaults, the negative-rule validation deviation, the plugin-discovery nuance (30s defaultDiscoveryTimeout is the effective upper bound on first ListTools — a cold uvx download can be skipped at discovery even with TIMEOUT: 300; existing behavior, but document so operators don't misread TIMEOUT), and an extension of mcp-token-not-logged's intent: never log ARGS or ENV values (positional ARGS can embed secrets; the #1393 redaction is key-name-based and cannot see positional values). Domain model: MCPServer definition + command/args/dir/env attributes + the new stdio invariant; re-render the .md (hard modelith-check gate).
Tests: config-validation table tests (exclusivity both/neither, ARGS/DIR/ENV-without-COMMAND, AUTH≠none); factory tests (injected newClientFor fake asserts mode dispatch; resolver-not-called counter for stdio; warn+skip on constructor error); stdio integration in internal/infrastructure/mcp/ — build a minimal stdio MCP server from internal/infrastructure/mcp/testdata/ using the SDK's own server.Run(ctx, &StdioTransport{}) pattern, spawned via the go build-helper harness precedent (internal/infrastructure/exec/main_test.go:23-42), asserting ListTools/CallTool round-trip, stderr routed to slog (captured handler) not parsed, Close reaps the child (Wait returns; idempotent), handshake-timeout kills a never-initializing child (deterministic via the timeout itself — no time.Sleep, ADR-036), and two stdio clients in one process operate independently (no shared os.Stdin/Stdout). make check-full green incl. test-race.
- Close ordering — session.Close() first (graceful EOF), then cancel, then Wait; the issue's "cancel → close stdin → Wait" kills before the child can exit cleanly.
- Eager vs. lazy connect — the issue inherits the HTTP client's "fresh transport per connect attempt" framing; for stdio I'd make construction eager and the failed-constructor cleanup the only cleanup path — simpler and zombie-free by construction.
- Domain model + catalog drift missing from acceptance criteria —
modelith-checkfails CI without the model/.mdupdate; the twomcp_factory.gocatalog pins and the partition-test coordination rule are a same-PR requirement. - Discovery-timeout nuance — document that the plugin's 30s discovery bound, not
TIMEOUT, caps first tool discovery for slow-starting stdio servers. - ARGS/ENV secrecy — positional args are invisible to key-name redaction; the invariant's intent must cover them.
- Windows —
exec.CommandLookPath handles.cmdresolution via PATHEXT; CommandContext Kill works; DIR/ENV are portable — note in ADR, don't block.
Net: the issue's diagnosis is sound and its containment claim ("not an architectural change") holds — the port, plugin, and consent/serial plumbing survive untouched; the work is one new adapter, a validation restructure, a factory seam change, and the model/docs/catalog hygiene that the repo's gates enforce.
I've completed the bootstrap and verified the subject's claims against the code and the SDK module cache. My verification results:
Verified correct: config struct/validation (mcp_config.go), isReadOnly() URL-suffix fall-through → consent/serial=true for COMMAND servers, DI seam + warn/skip, both catalog pins (mcp_factory.go:60-65, 157-160) + coordination rule, plugin 30s defaultDiscoveryTimeout, domain model's "over Streamable HTTP" definition + mcp-readonly-defaults + modelith-check hard gate, expandEnvHook string-kind interpolation (applies to COMMAND/ARGS/DIR/ENV values; process env, not viper), go-sdk v1.7.0 internals — StdioTransport pinned to os.Stdin/os.Stdout, IOTransport{Reader,Writer}, Transport single-connect contract, ClientSession.Close() idempotent → ioConn.Close() → rwc.Close() closing both pipes, Connect handshake synchronous in ctx, hello-server test pattern, and the exec harness precedent.
One nuance the subject's Close-ordering rationale glosses: rwc.Close() closes the reader (child stdout) first, then stdin — so session.Close() doesn't purely deliver "graceful stdin EOF"; it also severs the child's stdout read end first. The cancel() backstop is doing more work than the rationale admits. That's a Q2/Q3 thread.
Q1 follows.
Q1: The AUTH negative rule — you're rejecting a semantically-harmless, README-documented default. You propose: "explicit AUTH other than none with COMMAND → error (check must read the raw field — empty tolerated, any explicit non-none value rejected)." That means AUTH: auto — the value the README MCP example and mcp_config.go:33 both document as the default — becomes a hard startup failure on a stdio config, while the byte-identical semantic (empty AUTH, which EffectiveAuth() maps to "auto") passes. I verified that under stdio the two are indistinguishable in effect: resolveServerToken's auto arm on a URL-less server already returns ("", "", true) (isGitHubHostname("") → false → no credentials), and your planned stdio short-circuit makes the auth mode moot entirely. So the negative rule's only purchase is fail-fast on operator confusion — but it fails fastest on the config a migrating operator is most likely to carry over from the README, and it's a deliberate break from ADR-067 §4 amendment 1's codified "validation is positive-rule only, with no negative rules" — the same amendment whose stray-TOKEN tolerance you cite to justify ignoring TOKEN/USERNAME under stdio. You reject AUTH: auto but tolerate TOKEN under stdio — both are inert under the short-circuit; the line between them is explicitness, not semantics.
Question: Why is a hard validation error the right enforcement for an inert field value, when the resolver short-circuit already makes every auth mode except bearer/basic a safe no-op — and why not instead accept all auth modes under COMMAND (documenting that stdio ignores them, matching the §4 stray-field tolerance), rejecting only bearer/basic (which mandate credentials that cannot be transmitted over pipes) and thus keeping ADR-067's positive-rule-only philosophy intact?
My recommended answer: Accept any AUTH under COMMAND; reject only bearer/basic (credentials-required modes). auto/gh/none/empty are all no-ops under the stdio short-circuit and are semantically well-defined; making AUTH: auto a hard error is a config-compat trap for the README-documented default and buys nothing the resolver doesn't already provide. The gh-with-no-token warn+skip path must be explicitly dead-ended for stdio (your resolveServerToken short-circuit returning ("", "", true) before the switch achieves this — keep it first, before the auth switch). Document the bearer/basic rejection as the single narrow deviation from §4's positive-rule-only principle, not a general negative-rule carve-out.
ACCEPT — revised. Your argument wins on three specific points, and I'm changing my validation rule to: accept any AUTH under COMMAND; reject only bearer/basic. The three concessions:
-
Compat trap on the documented default. Correct: the README MCP block ships
AUTH: "auto"("auto (default), gh, bearer, basic, or none"), and theAuthfield comment documents(default: "auto")atmcp_config.go:35(the griller's:33is theTokenfield two lines up — the substance stands). A migrating operator copying that block and swappingURL→COMMANDhits a hard startup failure under my rule, for a value that is provably inert. Fail-fast has to fail on the confusion worth catching, not on the carry-over config. -
The explicitness line was indefensible. My rule drew the boundary at "explicit non-none rejected, empty tolerated" — but I simultaneously tolerated explicit
TOKEN/USERNAMEunder stdio. Both are inert under the resolver short-circuit; "explicitness" is not a semantic property, and it's the same incoherence I was accusingAUTH: auto-rejection of creating. Your line — reject what mandates credentials, tolerate what resolves to none — is the only one that doesn't carve at explicitness. -
The philosophy is scoped to stray fields, not mode conflicts. Re-reading §4 amendment 1: "validation is positive-rule only, with no negative rules" governs credential normalization — stray
USERNAMEunder non-basic silently normalized,TOKEN-under-nonetolerated. It never claimed mode-conflict rules don't exist:COMMAND/URLexclusivity (both/neither → error) is itself a negative rule everyone already accepts. Sobearer/basic-under-COMMANDis not a deviation from §4 — it's the same axis as exclusivity: a transport-mode conflict, because bearer/basic are the only modes whose config asserts a property pipes cannot carry (an Authorization header).auto/gh/none/empty don't assert credentials; they resolve to "no credentials" —autoon a URL-less server deterministically (isGitHubHostname("")→ parse error → false →("", "", true), verified inmcp_factory.go's auto arm),ghonly reachesresolveTokenOrSkipif the switch runs — which it won't. I'd frame the ADR note as "bearer/basic under COMMAND is a mode-conflict rule, same family as COMMAND/URL exclusivity," not as a lone exception to §4 — that keeps the philosophy intact and avoids pretending the exclusivity rule doesn't already sit in that category.
The resulting rule set (in validate(), checked in this order so the most specific error wins):
hasCommand == hasURL→ error (exclusivity, unchanged from my plan).ARGS/DIR/ENVwithoutCOMMAND→ error (unchanged).hasCommand && EffectiveAuth() ∈ {bearer, basic}→ error: "AUTH bearer/basic requires an HTTP endpoint; stdio (COMMAND) servers transmit no credentials." This check must run before the existing credential-required positive rules (TOKEN must not be empty when AUTH is bearer), so the mode-conflict message wins and we never emit a credential-emptiness error for a config that's invalid for a different reason.hasCommand && EffectiveAuth() ∈ {auto, gh, none}→ pass.gh-with-no-token never warns (mcp_token_resolution_skipped) because the short-circuit precedes the switch.- Stray
TOKEN/USERNAMEunder stdio: tolerated (unchanged, §4).
Resolver placement — confirmed and made explicit: resolveServerToken gets if isStdio(serverCfg) { return "", "", true } as its first statement, before the auth switch. This dead-ends the gh warn+skip path for stdio exactly as you require, and makes every accepted mode (auto/gh/none) a safe no-op by construction, not by arm-specific luck.
Test-plan strengthening (this is where your rule is strictly better): add a factory test asserting that AUTH: gh + COMMAND (a) does not invoke the injected token resolver, (b) does not emit mcp_token_resolution_skipped, and (c) still yields a built client — i.e., the short-circuit precedes the switch. Under my original rule this test couldn't exist; under yours it pins the exact guarantee that makes gh acceptance safe. Config-validation table gains: AUTH: auto + COMMAND → valid (was my error case), AUTH: gh + COMMAND → valid, AUTH: bearer + COMMAND → error, AUTH: basic + COMMAND → error.
One residual I'll carry: the catalog re-anchor obligation is unchanged — resolveServerToken's default case (pin at mcp_factory.go:157-160) and the gh resolver pin (:60-65) still drift with the short-circuit insertion, same-PR partition-test coordination applies. The rule change doesn't alter that; it only shrinks the validation surface.
Verified two SDK facts that sharpen the lifecycle question: (1) on child death the ioConn read loop terminates with io.EOF → pending calls fail fast; (2) on a wedged child (alive, silent), the read loop blocks and calls abort only at the ctx timeout — and the SDK's own comment admits pipe-close may not unblock a blocked read, so the kill in the Close ordering is load-bearing, not merely a backstop (your ordering is right; the "graceful EOF first" framing is cosmetic).
Q2:
Q2: "Recoverable" is in your acceptance criteria, but only for spawn/connect/handshake failures — a child that wedges or dies after a successful handshake has no recovery story in your plan, and the difference between the two cases is visible in the SDK. I verified in go-sdk v1.7.0's newIOConn read loop: a dead child closes the stdout pipe → the read goroutine terminates with io.EOF → pending CallTools fail fast. A wedged child (alive, silent) leaves the read loop blocked on the pipe — CallTool aborts only when your operationContext TIMEOUT fires (and the SDK's own comment admits closing the pipe may not unblock that blocked read). Either way, your connect() caches c.session forever: there's no dead-child detection, no reconnect, no respawn. Once a stdio child dies or wedges, every tool call from that server fails (fast or after TIMEOUT) for the rest of the session; the child is only killed at factory.Close() — session teardown. The supergateway bridge you're replacing at least restarted nothing either, so you have an HTTP-parity argument — but the issue sells native spawn as removing "a supervision layer," and your own test strategy asserts "serial execution for mutating stdio servers" without any test for "child died mid-session" or "child hung mid-session" — which suggests the guarantee is narrower than the acceptance criteria read.
Question: What is the recovery story for a stdio child that dies or wedges after the handshake succeeds — is "tool unavailable until session restart, same as a dead remote server" the accepted, documented answer, and if so, what distinguishes the two failure modes to the operator (fast EOF error vs TIMEOUT) in the error surface? Or does the plan commit to detecting child death (e.g., on error, check cmd.ProcessState/pipe EOF and mark the client dead) — and should an operation-timeout on a stdio client additionally kill the wedged child so a hung server can't silently poison the remaining session?
My recommended answer: Accept parity for this PR — document, don't build supervision: a dead/wedged stdio child makes that server's tools unavailable until session restart, identical to a dead remote server (the cached-session design already has this property, and respawn is scope creep with real risk of runaway processes). But make the error text distinguish the cases — a fast "mcp: stdio child exited (signal/exit status N)" on EOF vs a TIMEOUT error on a wedged child — so the operator can tell "server crashed" from "server hung" without grepping logs, and add one integration test for each (child exits mid-session → subsequent CallTool fails fast with the child-exit error; child ignores stdin and never responds → CallTool returns TIMEOUT). Do not kill the child on operation-timeout — a slow legitimate call (e.g., a 200s filesystem op under TIMEOUT 300) is indistinguishable from a wedged child, and killing it would break that call; the wedge is a documented known limitation, with stderr routing as the operator's visibility window.
ACCEPT — with one implementation refinement and one contract sharpening. Parity is the right call: the HTTP Client already caches c.session forever with no reconnect/respawn, and supergateway restarted nothing either — building supervision into the stdio adapter would make it more resilient than the transport it replaces, which is exactly the scope creep the issue should avoid, and respawn carries real runaway-process risk (a crash-looping server would be re-spawned on every call). No-kill-on-operation-timeout is also correct: a 200s filesystem op under TIMEOUT 300 is indistinguishable from a wedge, killing conflates per-operation timeout with process health, the HTTP client kills nothing on timeout, and a timeout doesn't poison the session — a slow child can still serve the next call.
SDK verification confirms your two-mode claim, and it's worse than you said for the dead case — which is why I refine the implementation. I read the ioConn read loop (transport.go): on dec.Decode error it does incoming <- msgOrErr{msg: raw, err: err} then return — so a dead child (stdout write end closed) delivers raw io.EOF to the pending CallTool's read: fail-fast confirmed. A wedged child leaves that loop blocked on the pipe; the SDK's own comment (transport.go: "leaks a goroutine if rwc.Read does not unblock after it is closed… no portable way to guarantee reads are unblocked") confirms only your operationContext TIMEOUT aborts. But note the wrinkle: the read loop terminates on EOF — subsequent calls after death depend on the jsonrpc2 layer having torn the connection down; if that teardown races, a subsequent call could hang until TIMEOUT instead of failing fast. So "every tool call fails fast after death" is not guaranteed by the SDK alone — it needs our client to enforce it.
Refined implementation — a reaper goroutine owns cmd.Wait, and the client pre-checks death before every call:
- Reaper goroutine started right after
cmd.Start():go func() { c.waitDone <- cmd.Wait() }()withwaitDone chan error(buffered 1). Benefits: (a) zombie prevention is no longer contingent onClose()being reached — the child is reaped the moment it dies, which is a genuine upgrade over my opening plan (Wait-only-in-Close); (b) the Wait result is available for error annotation; (c)Close()simplifies tosession.Close()(graceful stdin EOF) →cancel()(CommandContext kill backstop) → join the reaper, treating the expectedsignal: killed/context.CanceledWait error as expected (logged at debug, not surfaced — onlysession.Close()errors are joined). - Fast-death pre-check at the top of
ListTools/CallTool, under the existing mutex: non-blockingselectonwaitDone— if the child has exited, return immediatelyfmt.Errorf("mcp: stdio child %q exited: %w", name, werr)without touching the SDK. This makes every subsequent call after death fail fast deterministically, independent of the SDK's teardown timing — closing the gap I identified above. - In-flight EOF annotation: for a call already in flight when the child dies, wrap the returned error — if
errors.Is(err, io.EOF) || errors.Is(err, sdkmcp.ErrConnectionClosed)and the reaper confirms exit, annotate "mcp: stdio child exited (exit status N / signal)"; otherwise leave the deadline error as-is. The wedge case therefore surfaces as the plaincontext.DeadlineExceededwrap fromoperationContext— the two failure modes are distinguishable by (a) error text and (b) latency (immediate vs TIMEOUT), exactly as you require.
Contract sharpening (one place I'd tighten your answer): "every tool call fails for the rest of the session" is true for a dead child (fast, definitive, annotated) but not quite for a wedged one — a wedge makes each call fail at TIMEOUT, but the session isn't poisoned: if the child un-wedges, the next call can succeed, and with Serial: true (stdio default) there's exactly one in-flight op per server, so no cross-talk between a stale response and a fresh request (JSON-RPC ID matching drops stale replies). The documented contract should say: dead = unavailable until session restart, fast child-exit error on every call; wedged = per-call TIMEOUT, transient, stderr as the visibility window, no kill. Both documented in the ADR amendment; "recoverable" in the acceptance criteria gets rescoped to spawn/connect/handshake failures (warn+skip, session continues) and per-call timeouts (call fails, session remains) — not mid-session death.
Tests — accepted, plus one third assertion. (1) Child exits mid-session: the fixture server exits (e.g., after a die tool call or on signal); the next CallTool must fail fast with the child-exit error — and a subsequent call must fail fast with the same error, not TIMEOUT (pins the pre-check, which the SDK alone doesn't guarantee). (2) Wedge: a fixture server that reads stdin but never responds; CallTool returns DeadlineExceeded at the configured short TIMEOUT — the timeout is the deterministic sync point, no time.Sleep (ADR-036). (3) Optional: a slow-but-alive server that responds after a delay longer than one call's timeout — next call succeeds, pinning the no-kill policy. All three run under the go build-helper harness (internal/infrastructure/exec/main_test.go:23-42 precedent), SDK confined to internal/infrastructure/mcp/.
The reaper refinement is sound — cmd.Wait owned by a goroutine with a buffered signal channel fixes the Wait-exactly-once constraint and makes reaping independent of Close being reached; the TOCTOU between pre-check and in-flight call is covered by the EOF annotation. I verified the SDK's ioConn read loop terminates on io.EOF (fail-fast on dead child) and blocks on a wedge, and the unbuffered incoming channel's closed-case select bounds the leak window. That thread is closed.
One verified gap remains in the plan's own risk #5. Q3:
Q3: The "never log ARGS or ENV" guarantee has no enforcement location — and the actual leak path is the config loader, not your adapter. You extended mcp-token-not-logged's intent to cover ARGS/ENV, but trace the exposure surface: the invariant's literal scope is "config validation, DI factory, client transport — verified clean" (ADR-067 §4 amendment 3), and your adapter can trivially honor it (don't put ARGS/ENV in error text — exec.Error only carries Path, so a spawn-failure wrap including cfg.Command but not cfg.Args is safe). But the feature introduces a new secret-bearing config surface that pre-dates your adapter in the pipeline: the config loader's debug dumps. The raw-content dump (redactRawContent, #1393) is line-oriented and key-anchored — ARGS and ENV are not on the isSecretKey deny-list (args/env match no family), and the ADR documents value-mode scanning as "gated on unquoted scalars so quoted prose/URLs pass byte-identically." So MCP_SERVERS.fs.ARGS: ["--token", "sk-1234"] passes through the raw dump verbatim — positional values have no key for a key-based redactor to see, exactly the residual class (i) the ADR admits ("innocuous-name scalars"). An operator running with debug logging — the precise scenario #1393 was filed for — leaks the positional secret, and the loader is outside both the invariant's literal scope and your adapter's control. Your plan says "extend the invariant's intent" but never names a mechanism or a file. Separately, your stderr→slog routing logs arbitrary server-authored output on every stdio server — npx prints install logs, some servers echo their argv/env on startup — which is a second, unbounded emission surface the invariant says nothing about.
Question: Where is the "never log ARGS or ENV" guarantee actually enforced — does the plan commit to extending the config-loader raw dump to suppress MCP_SERVERS.*.ARGS/ENV values (fail-closed, accepting over-redaction per the ADR's own documented cosmetic trade) in the same PR, plus an adapter-side contract that no error/warning text ever includes ARGS/ENV values, and a scoping decision on child stderr (always-logged-but-out-of-invariant-scope vs debug-gated)?
My recommended answer: Three-part commitment, all in this PR: (1) Config loader — extend the raw dump to suppress MCP_SERVERS ARGS/ENV values (fail-closed; over-redaction of innocuous args is the same accepted cosmetic trade the ADR already records), with a boundary test pinning ARGS: ["--token", "sk-1234"] redacted and ARGS: ["-y", "@modelcontextprotocol/server-filesystem"] redacted-or-suppressed; this is a config-loader change (one file, internal/infrastructure/config/config.go) — outside the adapter, which is exactly why it needs an explicit owner in the plan. (2) Adapter contract — errors/warnings carry cfg.Command at most, never ARGS/ENV values (pin with a factory test asserting the constructor-error text contains no ARGS element). (3) Stderr — keep always-logged (it is your wedge-visibility window from Q2, gating it would defeat the purpose) but document in the ADR that server-authored stderr is outside mcp-token-not-logged's scope — the invariant governs tell-me-go's own credential plumbing, not arbitrary child output — and note the residual in the invariant's ADR entry.
ACCEPT all three parts — with one mechanism correction, one scope extension (the question names only the raw dump), and one test-precision point.
Leak trace — confirmed and extended. I read internal/infrastructure/config/redact.go and the dump sites. The raw dump (config.go:147, redactRawContent) is line-oriented, key-anchored on the leaf, with the value-mode gate at isQuotedOrEmptyValue — so ARGS: ["-y", "@modelcontextprotocol/server-filesystem"]: key ARGS fails isSecretKey (regex redact.go:22: api_keys|auth_tokens|authorization|token|passwords|secret|credentials|username|keys — no args/env family), value starts with [ not a quote → value-mode scan → firstSecretTokenIndex finds no deny-listed word → passes verbatim. Confirmed. But two refinements to the griller's framing: (a) the parsed-key dump (config.go:157-163) leaks identically — v.AllKeys() yields mcp_servers.fs.args/mcp_servers.fs.env, isSecretKey misses both, and slog.Any("value", v.Get(key)) logs the full ARGS slice and ENV map; it's also the more reliable leak, because the raw dump truncates to the first 1000 bytes (config.go:147) while the parsed-key dump doesn't. (b) The proposed boundary test ARGS: ["--token", "sk-1234"] is already redacted today — the word token inside --token is a token-char-delimited name that firstSecretTokenIndex matches, so the brace-less branch redacts the whole value. The real leak class is positional secrets with no deny-listed word: ARGS: ["-p", "sk-1234"] or ARGS: ["npx", "-y", "server", "sk-1234"] passes verbatim. The test must pin that case.
Part 1 mechanism — one change to secretKeyPattern, not a parser extension. The raw parser is line-oriented and tracks indentation only during suppression — it has no key-path state, so a path-scoped "only under MCP_SERVERS" rule is not implementable without rewriting the parser. The only mechanism that works for both dumps is the global deny-list: add args?|env to secretKeyPattern (redact.go:22). That one regex change gives: parsed-key dump redacts mcp_servers.fs.args/.env (suffix-anchored match); raw dump hits rule-3 key-mode redaction for ARGS:/ENV: lines → ARGS: [REDACTED] with suppression of more-indented continuation lines (so a multi-line ENV: block map is dropped wholesale, and flow forms ENV: {…} collapse to ENV: [REDACTED]). I accept the consequence explicitly: over-redaction is repo-wide for any future key ending in args/env — the same accepted fail-closed trade the deny-list already records for keys? matching "monkey", so it's a conscious decision, not an accident. No existing test breaks (verified: neither the TestIsSecretKey negatives — max_tokens, timeout, etc. — nor any TestRedactRawContent case ends in args/env). New boundary tests in redact_test.go: isSecretKey("mcp_servers.fs.args")/.env true; raw ARGS: ["-p", "sk-1234"] → ARGS: [REDACTED] (the isolating case); ARGS: ["-y", "@modelcontextprotocol/server-filesystem"] → ARGS: [REDACTED] (fail-closed over-redaction, "redacted" not merely "suppressed" — the rule-3 form is deterministic); ENV: block map → first line ENV: [REDACTED], indented sub-keys dropped; and the max_tokens/max_history_tokens exclusion re-pinned so the args?|env addition doesn't disturb the token carve-out.
Part 2 — adapter contract: accept. NewStdioClient error wraps carry cfg.Command at most (fmt.Errorf("mcp stdio: command %q: %w", cfg.Command, err)) — exec.Error only carries Path/Name plus the wrapped error, and cmd.Start() failures don't embed our ARGS/ENV, so the contract is trivially honor-able. Factory test: inject a failing newClientFor (or construct with COMMAND: "definitely-missing", ARGS: ["--secret", "sk-1234"], ENV: {"TOKEN": "sk-5678"}) and assert the constructor error and the mcp_client_init_failed warn (captured via slog handler) contain neither ARGS element nor ENV value — Command yes, Args/Env never. mcp_token_resolution_skipped is unreachable for stdio (Q1 short-circuit), so no second factory log surface to audit.
Part 3 — stderr: accept, always-logged, out of scope. Gating it behind debug would defeat the Q2 wedge-visibility purpose. Two specifics: log at Info with a structured attribute (mcp_server=<name>) so child output is greppable without grepping raw logs, and document in the ADR amendment that server-authored stderr is outside mcp-token-not-logged — the invariant governs tell-me-go's own credential plumbing (config validation, DI factory, client transport), not arbitrary child output; a server echoing secrets to stderr is the server's behavior, the same exposure class as gh/npx writing to an operator's terminal today, and the residual is recorded in the ADR entry. The ADR amendment also records the deny-list extension (args?|env) as closing part of residual class (i) for the new stdio surface, accepting the over-redaction trade — so this PR has an explicit owner for every emission surface: config loader (redact.go + boundary tests), adapter (error-text contract + factory test), and child stderr (scoping decision + ADR note) — none left to "intent".
Verified the serial semantics — this contradicts an acceptance criterion. Q4:
Q4: Your stdio serial default makes acceptance criterion #2 ("Multiple stdio servers work concurrently in one session") unachievable as execution concurrency — and there is no override knob, so a read-only local server is permanently serial, unlike its HTTP /readonly counterpart. I verified the executor: buildExecutionBatches (executor.go:661-688) emits a single-task isSerial: true batch for every serial tool and flushes the parallel batch around it; ToolOptions.Serial is documented as "the agent waits for this tool to finish before running others" (types.go:120) and doc.go:25 says "Tools may be executed concurrently unless Serial is set." Consequence: two stdio servers' tools in the same LLM response execute strictly sequentially — there is no cross-server parallelism for stdio tools, ever. Your Q2 claim "with Serial: true there's exactly one in-flight op per server" is true, but only because the executor serializes every serial tool against every other, and you never stated that consequence. And it's a config-surface gap, not just a doc gap: MCPServerConfig has RequiresConsent *bool (override exists) but no serial override — EffectiveSerial() is purely !isReadOnly(), stdio has no URL so no /readonly suffix is possible, and no SERIAL/REQUIRES_SERIAL key exists. The issue's own example fetch (read-only, could safely run concurrent) is forced serial forever, where the same server over a /readonly-suffixed HTTP bridge would run concurrent. Criterion #2 as written — "work concurrently" — holds only at the process-plumbing level (independent children, no shared os.Stdin/Stdout), which the issue's test-strategy sentence does say, but the criterion's first clause overpromises. One further interaction: with serial batching, a dead stdio child (Q2) not only fails its own call — executeSerialBatch halts the remaining plan (executor.go:623-626: "Serial batch failed or interrupted, halting execution plan"), skipping the other tools in that response.
Question: Do you (a) rewrite criterion #2 to scoped wording ("multiple stdio servers run as independent child processes in one session; tool execution remains serialized per existing Serial semantics"), (b) add a serial override knob for stdio so read-only local servers can opt into concurrency (a READONLY: true key that also sets consent=false, or a SERIAL: false), or (c) accept always-serial with an ADR note — and whichever you choose, does the serial-halt-on-dead-child interaction belong in the documented recovery contract from Q2?
My recommended answer: (a) + (c) — rewrite the criterion to the process-plumbing scope and explicitly document always-serial for stdio (including cross-server serialization within a response batch and the halt-on-failure interaction) in the ADR amendment; do not add a serial knob in this PR. Serial is the correct safe default for arbitrary spawned local processes, the issue's own rationale ("they are mutating local processes") commits to it, and a SERIAL/READONLY key is config surface for marginal throughput on a minority of read-only local servers — plus it would entangle with the consent default (a READONLY: true flag that flips both consent and serial is a new semantic axis the issue never asked for). Fold the halt interaction into the Q2 contract: "a dead/wedged child fails its own call fast (or at TIMEOUT); as a serial tool it also halts the remaining execution plan for that response — the LLM sees the failure and re-plans, so the session continues" — that is the accurate recovery story, and it belongs in the ADR alongside the wedge/death distinction.
ACCEPT (a) + (c) — evidence verified, and the no-knob case is stronger than you made it. I confirmed every claim: buildExecutionBatches (internal/agent/executor/executor.go:661-688) emits each serial tool as a single-task isSerial: true batch and flushes the parallel batch around it — so two stdio servers' tools in one response run strictly sequentially, cross-server parallelism for stdio is structurally impossible; evaluateBatchOutcome (executor.go:623-636) halts on results[batch.tasks[0]].Error != nil || ctx.Err() != nil and failRemainingTasks skips the rest — confirmed; types.go:120 ("waits for this tool to finish before running others") and doc.go:25 ("unless ToolOptions.Serial is set") — verbatim; and MCPServerConfig has RequiresConsent *bool but no serial field, EffectiveSerial() is purely !isReadOnly(). Your consequence statement is exactly right and I accept the criterion overpromise.
Criterion #2 rewrite (a): "Multiple stdio servers run as independent child processes in one session — one child each, no shared os.Stdin/os.Stdout — with concurrent discovery and independent lifecycle; tool execution remains serialized per the existing ToolOptions.Serial semantics (a serial tool runs alone in its response and halts the remaining plan on failure)." Two clauses I'd keep explicit: discovery is genuinely concurrent (plugin.go:61-89 spawns one ListTools goroutine per server, bounded by defaultDiscoveryTimeout, non-fatal per server) — that's where "concurrent" truthfully holds, and it belongs in the rewritten criterion so we don't over-correct into "nothing is concurrent"; and the rewrite aligns the criterion with the issue's own test-strategy sentence, which already said "operate independently (no shared os.Stdin/Stdout)."
No-knob (c) — accept, with the strongest argument being transport symmetry, which you didn't fully deploy. There is no serial override for any transport today: EffectiveSerial() = !isReadOnly() is global — a mutating remote HTTP MCP server is always serial, period, and the only way to get concurrent HTTP MCP is a /readonly-suffixed URL. Adding SERIAL: false for stdio would make stdio more configurable than HTTP — a new asymmetry in the opposite direction. The principled options are "both transports get a knob" (scope creep this issue must not absorb) or "neither" (current state). And the READONLY: true variant is worse: consent is already overridable via RequiresConsent *bool (verified mcp_config.go), so the only real gap is serial — and a flag that flips consent+serial together invents a third semantic axis (transport-readonly-ness) the issue never asked for. I accept the deferral, with one addition: the ADR must record the follow-up explicitly — "a per-server serial override, if ever added, applies to both transports or carries a justification for asymmetry" — so this is a tracked decision, not a silent limitation, and the fetch-style read-only local server's forced serialization is on the record as accepted-for-now, with the note that the operator has no in-repo mitigation today (the HTTP bridge this issue removes was the only one).
Halt interaction — accept, folded into the Q2 contract, with one precision: the halt is not stdio-specific — it is the existing executor contract for every Serial tool; a failing mutating remote MCP tool halts its response plan identically today. What the stdio serial default introduces is the application of that contract to read-only local servers that would be serial: false over HTTP — and the asymmetry is real and verified: the parallel batch (evaluateBatchOutcome else-branch, executor.go:633-637) halts only on ctx.Err(), not on individual tool errors, so a failing /readonly HTTP tool flows to the LLM without halting its siblings, while a failing stdio tool halts all of them. The Q2 recovery contract therefore reads: "a dead/wedged stdio child fails its own call fast (child-exit error) or at TIMEOUT; being serial, that failure also halts the remaining execution plan for the response (existing Serial contract, executor.go:623-626) — the LLM receives the error plus 'skipped: execution halted due to previous serial tool error' results and re-plans; the session continues. Read-only HTTP MCP tools fail without halting; this asymmetry is a documented consequence of the stdio serial default." That goes in the ADR alongside the wedge/death distinction from Q2, as one coherent recovery story.
Full picture confirmed: wireToolRegistry (container.go:203-212) defers everything to first GetRegistry() — which calls BuildRegistry → mcpFactory.Build (sequential stdio spawns) → then plugin registration (concurrent discovery). The expensive phase is sequential, on the pre-first-turn path. Q5:
Q5: Your own A4 argument — "discovery is genuinely concurrent, that's where 'concurrent' truthfully holds" — covers the cheap phase, and the expensive phase for stdio is sequential in the very factory you're relying on. Verified chain: first GetRegistry() (container.go:203-212, "deferred until the first call to GetRegistry") → BuildRegistry → mcpFactory.Build runs a sequential for name, serverCfg := range servers loop (mcp_factory.go:79-108). Under your eager construction, each stdio iteration = exec.CommandContext spawn + sdk.Connect handshake, bounded by EffectiveTimeout() — 300s default each — and only after every spawn completes does the plugin's concurrent ListTools discovery run. For HTTP this was invisible: NewClient is O(1) allocation, connect is lazy. For stdio you've moved the cost into Build and serialized it: N cold npx/uvx servers (first-run downloads, the common local-server case) block the first turn for Σ of their spawn+handshake times, and one slow server (e.g., a 250s cold download) delays every server behind it — plus a handshake exceeding TIMEOUT warns-and-skips, leaving that server absent for the whole session with no retry (MCP_SERVERS read once, ADR-067 §10). This is a UX regression stdio introduces that the opening statement's "eager is deliberate" rationale (deterministic spawn failures at Build, factory-test determinism) never priced. The plugin's own concurrency pattern (plugin.go:61-89 — one goroutine per server, bounded, non-fatal per server) is the in-repo precedent for doing this right, and eager determinism survives it: each server's construction failure can still warn+skip independently. One implementation wrinkle if you go concurrent: resolveServerToken's cachedToken memoization (mcp_factory.go:172-185) is not mutex-guarded — it's safe today only because Build is single-threaded; concurrent construction must either guard it or resolve tokens sequentially before the parallel spawn phase (relevant only to HTTP gh/auto servers, but the race is real).
Question: Do you (a) make stdio construction concurrent in Build — an errgroup over servers, each spawn+handshake bounded by TIMEOUT, failures warn+skip per server, cachedToken mutex-guarded (or token resolution kept sequential) — mirroring the plugin's own discovery pattern and capping first-GetRegistry latency at max(T_i) instead of ΣT_i; or (b) accept sequential construction and put the cold-start latency math on the record in the ADR as an accepted cost?
My recommended answer: (a) — concurrent construction. It's the same pattern the plugin already uses, it preserves every property your eager choice was buying (spawn failures still surface at Build as warn+skip; factory test still deterministic — inject the fake newClientFor and assert per-server dispatch/resolution), and it directly serves the issue's adoption story ("many local MCP services"): first-run latency is the operator's first impression, and Σ of 3-6 minutes of serialized cold downloads before the first turn is the kind of cost that gets the feature reverted to supergateway. Keep token resolution sequential-first (or mutex the cachedToken write) so the gh memoization stays single-flight; document in the ADR that construction is concurrent, bounded per server by TIMEOUT, and that a server exceeding TIMEOUT at construction is skipped for the session.
ACCEPT (a) — concurrent construction, with two refinements: a sequential token pre-pass instead of mutex-guarding cachedToken, and the plugin's own sync.WaitGroup pattern instead of errgroup. The cost math is verified and damning for (b): Build's sequential loop (mcp_factory.go:79-108) under eager construction runs each spawn+handshake bounded by EffectiveTimeout() — 300s default — so N cold npx/uvx servers block the first GetRegistry() for worst-case N×300s before the plugin's concurrent discovery even starts; one 250s cold download delays every server behind it. That is exactly the first-impression regression that gets the feature reverted to supergateway; concurrent construction caps first-turn latency at max(Tᵢ) + 30s discovery instead of ΣTᵢ. Eager determinism survives untouched: spawn failures still warn+skip per server at Build, and the factory test stays deterministic with an injected newClientFor — the fake just needs a mutex'd call-recorder and order-independent assertions (ADR-021 hand-rolled mocks; the race detector in check-full will enforce it).
Refinement 1 — sequential token pre-pass, not a mutex. cachedToken (mcp_factory.go:172-185) is indeed unguarded — f.mu protects only clients, and today's single-threaded Build is the only thing making the memoization safe. Mutex-guarding works, but the cleaner fix is a two-phase Build: phase 1, sequential pass calling resolveServerToken per server (preserving the exact single-flight gh-memoization semantics of today with zero new lock machinery — the gh resolver is never spawned concurrently, so the memoized-token invariant holds by construction); phase 2, concurrent construction using the pre-resolved (username, token, ok) triples. Stdio servers short-circuit in phase 1 (Q1) and their spawn+handshake lives entirely in phase 2. This also keeps the ok=false skip-and-warn for HTTP gh/auto failures byte-identical to current behavior.
Refinement 2 — mirror the plugin, not errgroup. The cited precedent (plugin.go:61-89) is sync.WaitGroup + mutex'd result collection + per-server non-fatal warn — not errgroup. errgroup's Go+Wait returns the first error and its semantics are built for fail-fast fan-out; per-server warn+skip wants "run all, log each failure, return nil." Using errgroup here would be taking the recommendation's letter, not its intent — the in-repo pattern is the WaitGroup, and consistency with the plugin is the whole argument for (a). Per-server construction stays bounded by EffectiveTimeout; no overall Build context bound, matching the plugin's per-server bound.
Concurrency-safety audit (the rest is already safe): the clients slice append is already under f.mu (mcp_factory.go:96-99), so concurrent Build vs Close is race-free today; each NewStdioClient owns its own exec.Cmd + SDK client, so no shared state across construction goroutines. The only unsafe field is cachedToken, eliminated by the pre-pass.
ADR documentation (accept, with the exact contract): construction is concurrent, bounded per server by TIMEOUT; a server whose spawn+handshake exceeds TIMEOUT is warn-and-skipped for the whole session — no retry, consistent with MCP_SERVERS read-once (ADR-067 §10) and with the HTTP skip semantics; worst-case first-GetRegistry latency is max(Tᵢ) + plugin discovery, recorded so the cold-start trade is on the record, not an accident.
Tests to add: a factory concurrency test with a fake newClientFor whose per-server delay is controlled by channel-based gates (no time.Sleep, ADR-036), asserting (i) all N servers are constructed, (ii) total Build time ≈ max delay, not Σ, (iii) a failing server warns+skips while the others succeed, and (iv) the gh resolver is invoked at most once across a multi-server HTTP config (pinning the single-flight pre-pass); plus the existing dispatch-mode and token-skip assertions made order-independent. test-race via check-full is the enforcement gate.
A5's refinements are correct — sync.WaitGroup is the actual in-repo pattern (plugin.go:61-89, mutex'd result collection, per-server non-fatal), errgroup's fail-fast first-error semantics genuinely don't fit warn+skip; and the sequential token pre-pass avoids touching cachedToken's unguarded memoization while preserving single-flight gh. Verified.
One lifecycle guarantee remains untested against the issue's own flagship example. Q6:
Q6: "No zombie processes" (acceptance criterion) is only true for the direct child — your reaper's cmd.Wait() reaps exactly one process, and the issue's headline examples are process-tree spawners. npx -y @modelcontextprotocol/server-filesystem is node(npx) → node(server); uvx mcp-server-git is a Python launcher that spawns the tool as a subprocess — neither exec-replaces. Your Close() kills the direct child (cancel() → CommandContext) and reaps it; the grandchild is not killed and not reaped. The saving grace is structural and worth stating precisely: the server inherits the same stdin pipe (launchers pass stdio through, they don't proxy it), so when session.Close() closes the write end, the grandchild sees stdin EOF directly and self-terminates — and this is guaranteed by construction for any server built on the SDK's own server.Run(ctx, &StdioTransport{}) (the read loop exits on io.EOF → Run returns → process exits), which is exactly the fixture pattern your integration tests and the issue's examples use. So the honest guarantee is: direct child always reaped; process tree best-effort via shared-pipe stdin EOF, which SDK-built servers honor by construction and third-party servers honor by MCP stdio convention. The residual holes: (1) a server that ignores stdin EOF (wedged, or detached/daemonized stdio) survives Close as an orphan; (2) on Windows, the direct-child kill doesn't touch the .cmd/node grandchild chain and there's no SIGPIPE to backstop it — a non-EOF-honoring grandchild lingers indefinitely. None of this appears in the plan or the Q2 recovery contract, which speaks only of "the child" in the singular.
Question: Does the plan commit the guarantee as "direct-child reaping is deterministic; tree termination is best-effort via stdin EOF (SDK-built and convention-following servers), with the non-honoring-child orphan and the Windows grandchild chain documented as accepted residuals" — or does it add a POSIX process-group kill (SysProcAttr{Setpgid: true} + kill(-pgid, SIGKILL) in Close) to make tree termination deterministic on Unix, at the cost of a platform-split in the reaper?
My recommended answer: Commit the two-tier guarantee and do not add process-group kill in this PR — but make the tiering explicit and documented, not implied. The shared-pipe stdin-EOF mechanism already covers the issue's own examples deterministically (SDK-built servers), the process-group kill is a platform-split addition with its own footguns (a pgid-only kill is wrong for a child that legitimately reparents; Windows needs Job Objects, not a POSIX trick, so you'd carry two platform paths for a residual class that is "server ignores the MCP transport contract"), and the Q2 recovery contract already gives the operator the visibility window (fast child-exit error on the direct child's death; stderr logged). What must land in the ADR amendment: (1) the direct-child vs tree distinction, (2) the stdin-EOF self-termination mechanism and why it covers npx/uvx (shared pipe, not proxied), (3) the two residuals (EOF-ignoring child orphan; Windows grandchild chain) as accepted, with process-group kill / Job Objects recorded as the tracked future option, and (4) one integration test pinning the actual contract: fixture server exits on stdin EOF → Close() returns without the reaper's Wait error being surfaced as a failure (the graceful path), plus a second fixture that ignores stdin EOF → Close() still returns (cancel kills the direct child, Wait reaps, no hang) — proving the backstop works even when the graceful path doesn't.
ACCEPT — the two-tier guarantee, no process-group kill this PR — with three refinements: a third Unix mechanism made explicit, the zombie-vs-orphan distinction added to the contract, and a third integration test that pins the load-bearing tree claim your two tests leave implicit.
The mechanism chain is verified and the ordering matters. npx (node) and uvx (Python launcher) spawn, they don't exec-replace — direct child is the launcher, grandchild is the server. The saving grace is exactly as you state, and my Q2 Close() ordering makes tier 2 run before tier 1's kill: session.Close() → rwc.Close() closes the write end of the stdin pipe (transport.go:396-407) → the grandchild, holding a dup of the same read end through the launcher's pass-through, sees EOF directly → SDK-built servers exit (server.Run read loop gets io.EOF, returns, process exits — verified in the ioConn loop) → the launcher exits when its child does → then cancel() is the backstop for anything still alive → reaper reaps the direct child. So the graceful path is genuinely tried first, not an afterthought.
Refinement 1 — make the Unix backstop a named third mechanism, and sharpen the residual. Your framing implies Unix is covered by SIGPIPE where Windows isn't; that's only half true. The Unix backstop fires when a grandchild writes to its stdout (our closed read end) — SIGPIPE. A grandchild that ignores EOF and never writes is an orphan on both platforms; Windows additionally loses even the write-time backstop (broken-pipe error handling is process-dependent, not a guaranteed kill). So the ADR residual reads: "EOF-ignoring and silent grandchild = orphan (Unix); EOF-ignoring grandchild = orphan (Windows, no signal backstop)." Precise, not platform-flattering.
Refinement 2 — zombie vs orphan in the contract. The acceptance criterion "no zombie processes" is satisfiable and the distinction is load-bearing: a zombie is dead-and-unreaped — tier 1 guarantees none (reaper always reaps the direct child; tree descendants are reaped by their own parent chain or init). An orphan is alive-and-detached — an accepted residual, not a zombie, and not covered by the criterion. The ADR must say "no zombies, guaranteed; orphans, possible for transport-contract-violating servers" — otherwise the criterion and the residual look contradictory.
Refinement 3 — a third integration test for the tree, because your two tests don't pin the issue's headline claim. Your test 1 (SDK fixture exits on stdin EOF → Close() clean) pins the server side of tier 2; your test 2 (EOF-ignoring fixture → Close() still returns via cancel+reaper) pins the backstop. Neither pins the launcher pass-through — the claim that npx/uvx trees self-terminate, which is the entire adoption story. Add: a synthetic launcher fixture (a small Go program that execs the SDK server with Stdin: os.Stdin inherited — pass-through, not proxied) → assert on Close() that both the launcher (reaped by us, Wait returns) and the grandchild (liveness poll with deadline — deterministic poll loop, no time.Sleep, ADR-036) are gone. Real npx/uvx in CI is network-flaky; the synthetic launcher pins the mechanism without the network. Fixture design note for test 2: an EOF-ignoring server is achievable with the SDK — mcp.NewServer, srv.Connect(ctx, &mcp.StdioTransport{}) (handshake succeeds, tools work), then block on a channel instead of server.Run (which would exit on EOF) — so the fixture ignores EOF while remaining a real connected MCP server. And Close() must suppress the reaper's expected signal: killed on the cancel path (my Q2 design: surfaced only on spontaneous death, logged at debug on our own close) — test 2 asserts that suppression, i.e. Close() returns nil/no-failure with an EOF-ignoring child.
Why no process-group kill this PR — agree, with the caveat recorded. Setpgid+kill(-pgid) is a Unix-only platform split for a residual class that is "server ignores the MCP transport contract," the same class Q2 already answered with "document, don't build supervision"; pgid-kill is wrong for a child that legitimately reparents (a daemonizing server would either escape the pgid or, worse, take a killed grandchild that should have lived); and Windows needs Job Objects — two platform paths for a misbehaving-server edge. Tracked future option in the ADR, with the reparenting caveat and the Job-Objects note, so it's a decision with a paper trail, not a silent deferral.
ADR amendment content (the four items, plus my three additions): (1) direct-child vs tree distinction; (2) stdin-EOF self-termination mechanism and why it covers npx/uvx (shared pipe, not proxied); (3) Unix SIGPIPE write-backstop as a named third mechanism; (4) residuals — EOF-ignoring-and-silent orphan (Unix), EOF-ignoring orphan (Windows) — as accepted, with pgid-kill/Job Objects tracked; (5) zombie-vs-orphan wording so the acceptance criterion and the residual are consistent; (6) the three integration tests (graceful EOF exit, EOF-ignoring backstop, launcher-tree pass-through).
Verified — and this kills one of my candidate footguns while confirming a subtler one. exec.go:149-154: "If Path is relative, it is evaluated relative to Dir" — so COMMAND: "./mcp-server" + DIR: /srv/server resolves correctly; that concern is dead. But exec.go:387-389: a bare name (no separators) goes through LookPath, which runs in the parent process against the parent's PATH — cmd.Env is applied to the child only after exec. Q7:
Q7: The ENV field you're adding cannot resolve COMMAND — a bare-name COMMAND is looked up in tell-me-go's PATH, not the PATH you set in ENV, and the issue's own headline example makes the trap likely. Verified in the stdlib: exec.Command(name) with no separator → LookPath in the parent process (exec.go:387-389), using the parent's os.Getenv("PATH"); cmd.Env (your ENV: map[string]string merged into os.Environ()) is applied in the child after the exec, so it cannot influence the initial command resolution. An operator configuring the issue's own uvx example inside a venv — COMMAND: "uvx" + ENV: {PATH: "/venv/bin:..."} — gets "executable file not found in $PATH" at Build → warn+skip → the server silently absent for the session, with no retry (ADR-067 §10). The ENV.PATH is not useless — it does govern the child's own subprocess resolution (uvx finding mcp-server-git, node resolving toolchains) — which is precisely why the operator sets it and precisely why the failure is confusing: the PATH they set clearly "works" for the server's world, just not for the one lookup that matters first. The escape hatches exist but are undocumented: absolute COMMAND (/venv/bin/uvx), relative-with-DIR (./bin/uvx — now verified safe), or ${VAR} expansion via expandEnvHook (COMMAND: "${UVX_PATH}/uvx", process-env sourced). None of this is in the plan's config section, README example, or ADR amendment — the README stdio example you're adding (COMMAND: "npx", COMMAND: "uvx") demonstrates exactly the form that has this failure mode if the operator combines it with an ENV.PATH.
Question: Does the plan commit to documenting this resolution contract — bare COMMAND → parent process PATH (not ENV.PATH); separator-bearing relative COMMAND → resolved against DIR; ENV.PATH affects the child only post-exec; use absolute or ${VAR}-expanded COMMAND for venv/toolchain paths — in the README MCP section and the ADR amendment, and should the README stdio example pair each npx/uvx command with its required-absolute-path note?
My recommended answer: Document-only, in three places: one README paragraph in the new stdio MCP section (the resolution contract + the venv example with absolute path), one line in the ADR amendment (resolution semantics; deliberately standard os/exec, no custom lookup), and a sentence in the README example comments (COMMAND is resolved via the parent process PATH; use an absolute path or ${VAR} for toolchains). Do not add a Build-time heuristic warning when ENV contains a PATH override — ENV.PATH is legitimately set for the child's own needs even when COMMAND is absolute, so the warning would be noisy and wrong half the time. This is the last unexamined corner of the config surface; after this I have no further material — this is my final substantive question before the verdict.
ACCEPT the documentation plan in all three places — with one revision to the "do not add": reject the heuristic warning (agreed, it would be noisy and wrong half the time), but add a failure-point error annotation on exec.ErrNotFound, which is neither heuristic nor a warning — it fires only on a real LookPath failure and is never wrong. The technical claims are verified: exec.Command on a separator-free name runs LookPath in the parent (os/exec semantics; cmd.Env is applied to the child only after exec, so it cannot influence the initial resolution), separator-bearing COMMAND is used as-is with relative paths resolving against cmd.Dir, and expandEnvHook (internal/infrastructure/config/config.go:187-190, os.ExpandEnv) expands ${VAR} in COMMAND at config-load time from the process environment. So COMMAND: "uvx" + ENV: {PATH: "/venv/bin:…"} fails LookPath at Build → warn+skip → absent for the session, while the same PATH demonstrably "works" for the server's own subprocess resolution — precisely the confusing asymmetry you describe, and the issue's own uvx example is the trap's poster child.
Why I revise the "do not add": a heuristic warning keyed on "ENV contains PATH" is wrong because ENV.PATH is legitimately set for the child's world even with an absolute COMMAND — it would fire on configurations that work fine. But the failure point is knowable and narrow: NewStdioClient's exec.CommandContext returns an error satisfying errors.Is(err, exec.ErrNotFound) exactly when the bare-name lookup failed. Annotating that error — mcp stdio: command %q not found in tell-me-go's PATH (COMMAND resolves before ENV is applied; use an absolute path, a DIR-relative path, or ${VAR}-expanded COMMAND): %w — costs ~5 lines and one test, leaks nothing (Command only, per the Q3 adapter contract), and is the difference between "documented somewhere" and "the operator sees the answer at the exact moment of confusion." Without it, the operator who hits the failure sees only "executable file not found in $PATH" — correct, but silent about the ENV.PATH they set, which is the entire confusion. I'm distinguishing precisely: no preemptive warning; a fail-point annotation on the actual error. If you still prefer pure document-only, the annotation is droppable without architectural impact — but I'd keep it.
The three documentation placements, accepted with specifics:
-
README stdio MCP section — one paragraph stating the resolution contract: bare
COMMAND(no path separator) →LookPathin tell-me-go's own process PATH, notENV.PATHand notDIR; separator-bearingCOMMAND(/abs/path,./rel,dir/name) → used as-is, relative resolved againstDIR;ENV.PATHgoverns the child's environment only, post-exec (it does reach the server's own subprocess resolution — uvx findingmcp-server-git— which is why it's set and why the failure is confusing);${VAR}expands at config load from the process environment and works inCOMMAND,ARGS,DIR, andENVvalues. Follow with the venv example in absolute form:COMMAND: "/venv/bin/uvx"(orCOMMAND: "${UVX_PATH}/uvx"). -
ADR amendment — one line: resolution semantics are deliberately standard
os/exec(LookPathin the parent; no custom PATH resolution), with the parenthetical that Windows bare-name lookup honorsPATHEXT(npx→npx.cmd) via the same stdlib path;ENV.PATHis documented as post-exec-only to preempt the venv confusion class. -
README example comments — one sentence on each example: pair the issue's own
npx/uvxcommands with# COMMAND is resolved via tell-me-go's PATH (parent process); use an absolute path or ${VAR} for venv/toolchain locations— so the two headline examples, which are exactly the bare-name form with the failure mode, carry the caveat inline.
No new open threads: the resolution contract, the fail-point annotation, and the docs land together; the heuristic warning stays out.
VERDICT: The subject's diagnosis held up across every line-level claim I could verify — the issue's evidence, the SDK lifecycle facts, the catalog pins, and the domain-model gate — and its corrections to the issue (modelith-check, discovery-timeout vs TIMEOUT, ARGS/ENV secrecy, catalog coordination) were all real and adopted. Where the plan did not hold, the subject revised under pressure rather than defended: the indefensible AUTH negative rule (Q1), the overpromised "recoverable" contract for mid-session child death (Q2), the unachievable "concurrent" acceptance criterion under always-serial semantics (Q4), the unacknowledged serialized cold-start cost of eager construction (Q5), the two-tier process-tree guarantee that was implied but not committed (Q6), and the undocumented COMMAND-resolution contract (Q7) — with the final exec.ErrNotFound fail-point annotation a genuine improvement over my document-only position. Recommendation: proceed with changes — the integrated plan (validation as revised, reaper + fast-death pre-check, concurrent Build with sequential token pre-pass, two-tier lifecycle guarantee, args/env redaction, serial-scoped criterion, resolution contract, and the ADR/model/README/catalog hygiene) is coherent, verified against the code, and ready for implementation; no open questions remain.
POST-GRILL POSITION:
What I stand by, unchanged from the opening: the core design is right — a separate StdioClient in internal/infrastructure/mcp/ (never a mode on the HTTP Client), wired through sdkmcp.IOTransport{Reader: stdoutPipe, Writer: stdinPipe} (never the process-pinned StdioTransport), eager construction so spawn failures surface at Build as the existing warn+skip, stderr routed to slog, and the three-way error split preserved so non-terminal MCP errors stay recoverable in-turn. The diagnosis held: the issue's evidence was line-level accurate, the port/plugin/consent plumbing survives untouched, and this is a contained addition. The hygiene obligations were real from the start and remain non-negotiable: modelith-check (domain model + re-render), ADR-067 amendment, README, and the two mcp_factory.go catalog pins re-anchored in the same PR as the partition test per the coordination rule.
What the grill corrected — all seven, adopted: (1) the AUTH rule collapsed from "reject all non-none" to "reject only bearer/basic," on the mode-conflict-vs-stray-field axis that keeps ADR-067 §4's positive-rule philosophy intact and avoids the AUTH: auto compat trap; (2) the recovery contract is parity, not supervision — dead child fails fast with an annotated child-exit error (reaper + fast-death pre-check), wedged child fails at TIMEOUT, no kill on operation-timeout, with the serial halt-on-failure interaction folded in as existing ToolOptions.Serial semantics; (3) the ARGS/ENV secrecy guarantee found its real enforcement home in the config loader — args?|env added to secretKeyPattern covers both debug dumps, the adapter carries cfg.Command at most, and child stderr is documented as out of invariant scope; (4) acceptance criterion #2 is rewritten to the process-plumbing scope it always meant, with always-serial accepted and no knob (there is no serial override for any transport today — symmetry cuts both ways); (5) Build construction is concurrent (WaitGroup, mirroring the plugin) with a sequential token pre-pass preserving the cachedToken single-flight, capping first-GetRegistry latency at max(Tᵢ); (6) the process-tree guarantee is now explicit two-tier — direct-child reap deterministic, tree termination best-effort via shared-pipe stdin EOF plus the Unix SIGPIPE write-backstop, orphan residual and Windows chain documented, no pgid kill this PR; (7) the COMMAND-resolution contract (parent PATH, not ENV.PATH; separator-bearing relative against DIR; ${VAR} at load) is documented in README/ADR/example comments — plus the exec.ErrNotFound fail-point annotation I added over the griller's document-only position, which the verdict endorsed.
The integrated contract, as it now stands: a stdio server is a locally spawned child with a bounded handshake; construction is concurrent and per-server fail-fast with warn+skip; the child is reaped deterministically and its tree terminated best-effort; a dead child surfaces as a fast child-exit error and, being serial, halts its response's remaining plan (the LLM re-plans); a wedged child surfaces as per-call TIMEOUT with stderr as the visibility window; tool execution is serialized per existing semantics while discovery and lifecycle are genuinely independent per server; ARGS/ENV are redacted at the loader and never emitted by the adapter; and the config surface's one confusing corner — PATH resolution — is documented and annotated at the failure point.
Open questions: none blocking. The two items the grill surfaced as gaps are recorded ADR future options, not open decisions: a per-server serial override (which, if ever added, must apply to both transports or justify the asymmetry) and POSIX process-group kill / Windows Job Objects for transport-contract-violating servers. The verdict's "proceed with changes" is accepted; implementation follows the integrated plan as revised across these seven exchanges, with make check-full green as the gate.