fix(env): clean up overrides when setup fails

[oodadoudou]

When a server's later environment value fails validation or resolution, withEnvOverrides previously threw before entering its cleanup block. Earlier temporary writes remained in the parent process and could be inherited by the next MCP subprocess even though the failed connection callback never ran.

Move the setup loop inside the existing try/finally, retaining inherited-value precedence and cleanup ownership. Thanks @oodadoudou (JUSHUANGHUI LI) for the fix. The maintainer follow-up adds changelog credit, documents failed-setup cleanup, and checks that inherited values remain unchanged and invalid values are never applied.

On Node 24.20.0 / pnpm 10.34.5, pnpm check and pnpm test pass: 1,793 tests passed, 26 skipped. Independent proof uses built public createRuntime() on the original baseline ab0c27f03fd7d27aa5b48774ac749a4548836234 and the candidate. For both unsupported ${env:VAR} and missing $env:VAR forms, a failed connection is followed in the same process by a real local stdio MCP server. Initialization, tools/list, and tools/call complete successfully.

Base leaves the synthetic temporary value in the parent and the next MCP server; candidate leaves it absent in both. An inherited synthetic value stays unchanged in both revisions. The driver closes every runtime in finally. No transport/helper mocks, private configuration, or credentials are used.

Full-candidate isolated Codex review (original patch plus maintainer changes) found no actionable P0–P2 issues. The contributor's original commit is preserved; the maintainer follow-up carries a co-author trailer.

The existing contributor branch was reconciled with main after #343 and #341 by a normal merge commit. Both independent changelog entries and contributor history were preserved. Frozen install/build, all 19 focused environment/OAuth regressions, and the built public-runtime subprocess proof pass again on the combined candidate. Full combined P0–P2 reviews before the integration commit and landing found no actionable issues.

Final head: 8bc87249aa482e0860d7d04120d8fe4d18788838; base: 30880f8aa17ae4c247314e2a1a0ac7b18c190cf9. CI passed on Ubuntu, macOS 15, and Windows, including the full suites. Fork workflow approval was granted after reviewing the exact candidate and unchanged workflow. No CI test failures or reruns were needed.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed September 4, 2026, 5:04 AM ET / 09:04 UTC.

ClawSweeper review

What this changes

The PR cleans up temporary server environment values when setup fails, adds regression coverage, and documents the behavior.

Merge readiness

✅ Ready for maintainer review

This remains a necessary, well-supported fix: main and v0.13.8 still expose the reported failure. No actionable patch defects were found.

Priority: P2
Reviewed head: 3f636b39cc7fac7cdba7d9175651dd55d45584bd

Review scores

Measure	Result	What it means
Overall readiness	🦞 diamond lobster (5/6)	A minimal correction, focused regression assertions, and matching real-runtime before-and-after evidence support high confidence.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (live_output): Captured before-and-after runtime results exercise withEnvOverrides through public createRuntime and real stdio transport on Node 24.20.0. Both setup failures leave the candidate's parent and subsequent MCP server free of the temporary value, preserve inherited state, and allow successful MCP calls.
Patch quality	🦞 diamond lobster (5/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (live_output): Captured before-and-after runtime results exercise withEnvOverrides through public createRuntime and real stdio transport on Node 24.20.0. Both setup failures leave the candidate's parent and subsequent MCP server free of the temporary value, preserve inherited state, and allow successful MCP calls.
Evidence reviewed	8 items	Verified patch and policy scope: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-notes directory was found. The locally verified introduced diff matches the supplied four-file patch and changes no dependencies, workflows, or public APIs. Current main still contains the defect: The setup loop writes environment keys before entering try/finally. A later validation or resolution error therefore skips cleanup of earlier writes. Latest release remains affected: The supplied latest release, v0.13.8, has the same setup loop outside try/finally; this correction is not present in that release.
Findings	None	None.
Security	None	None.

How this fits together

MCPorter applies server-specific environment values while connecting MCP transports. Those values can reach spawned server processes, so cleanup determines what later connections inherit.

flowchart TD
  A[Server configuration] --> B[Apply temporary environment]
  B -->|Valid values| C[Connect MCP transport]
  B -->|Invalid value| D[Remove applied keys]
  C -->|Success or failure| D
  D --> E[Return result or error]
  E --> F[Later MCP connections]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Regression coverage	2 setup-failure cases; production net 0 lines, tests +23	The fix reuses existing cleanup and verifies both failure forms without adding production complexity.

Technical review

Best possible solution:

Failed connection setup should leave no temporary environment values for later servers while preserving existing value precedence.

Do we have a high-confidence way to reproduce the issue?

Yes. Main visibly leaves an earlier temporary value behind when a later override throws; supplied public-runtime before-and-after results confirm that path. This read-only review did not execute it.

Is this the best way to solve the issue?

Yes. Extending the existing try/finally to include setup is the narrowest correction and preserves validation order, inherited-value precedence, and callback cleanup.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against ab0c27f03fd7.

Labels

Label justifications:

• P2: This fixes environment contamination after failed MCP connection setup, with a narrow trigger and repair.
• rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Captured before-and-after runtime results exercise withEnvOverrides through public createRuntime and real stdio transport on Node 24.20.0. Both setup failures leave the candidate's parent and subsequent MCP server free of the temporary value, preserve inherited state, and allow successful MCP calls.
• proof: sufficient: Contributor real behavior proof is sufficient. Captured before-and-after runtime results exercise withEnvOverrides through public createRuntime and real stdio transport on Node 24.20.0. Both setup failures leave the candidate's parent and subsequent MCP server free of the temporary value, preserve inherited state, and allow successful MCP calls.

Evidence

What I checked:

• Verified patch and policy scope: Read the complete root AGENTS.md; no nested AGENTS.md or maintainer-notes directory was found. The locally verified introduced diff matches the supplied four-file patch and changes no dependencies, workflows, or public APIs. (AGENTS.md:1, 3f636b39cc7f)
• Current main still contains the defect: The setup loop writes environment keys before entering try/finally. A later validation or resolution error therefore skips cleanup of earlier writes. (src/env.ts:109, ab0c27f03fd7)
• Latest release remains affected: The supplied latest release, v0.13.8, has the same setup loop outside try/finally; this correction is not present in that release. (src/env.ts:109, 98e739a253fc)
• Production boundary: createRuntime feeds the connection cache, which calls createClientContext. That function wraps transport creation with withEnvOverrides; stdio setup copies the process environment and passes it to the real MCP transport. (src/runtime/transport.ts:233, 3f636b39cc7f)
• Cleanup and compatibility coverage: The candidate protects setup and callback execution with the same finally block. Two regression cases check unsupported placeholders and missing required values, including callback suppression, removal of temporary keys, and preservation of inherited values. (tests/env-and-daemon-utils.test.ts:80, 3f636b39cc7f)
• Captured real runtime results: The supplied PR body reports Node 24.20.0/pnpm 10.34.5 before-and-after runs using built public createRuntime and a real local stdio MCP server. For both failure forms, main leaked the synthetic value into the parent and subsequent server; the candidate left it absent in both while preserving an inherited value. Initialization, tools/list, and tools/call succeeded. The body also reports pnpm check and pnpm test passing with 1,793 tests passed and 26 skipped. These are supplied results, not reviewer executions; captured context revision is f16bce39cb206339b55100e01a67d5a7ad869ce3d4eeb8a80953eb2097cd92fd. (3f636b39cc7f)

Likely related people:

• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
• goutamadwant: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)

• reviewed 2026-09-03T19:55:25.929Z sha 7c499c5 :: needs real behavior proof before merge. :: none
• reviewed 2026-09-03T21:06:49.813Z sha 7c499c5 :: needs maintainer review before merge. :: none