fix: stop the doctor's cwd cleanup from overwriting the handshake error - #79
Merged
Merged
Conversation
The handshake spawns the server with an empty tempdir as its cwd, the v1.9.3.4 cwd-shadow defense. On Windows that directory stays locked until the child has fully exited, so tearing the transport down after a failed handshake can lose the race and raise PermissionError [WinError 32] out of the tempdir's own __exit__ - which then replaces the very error the doctor exists to report. A Windows CI job hit this and showed a lock message where "synthetic client session entry failure" belonged. Cleanup of that isolation boundary is now best-effort. The directory is a boundary, not state the report depends on, and a leftover empty tempdir beats a masked cause. The shadow defense is unchanged. The regression test refuses to remove that directory rather than racing a real process, so it reproduces on every platform.
Both roadmaps report track 3 as "measurement not started" while the network-free, corpus-free half of the Phase 1 baseline is already frozen in validation/2026-08-05-quality-baseline-scorecard.md - a dated, revision-bound artifact this repo shipped a day earlier. Understating completed measurement is the same defect class as overstating it, and the scorecard paragraph sitting directly beneath the stale sentence made the section contradict itself. State what is actually done: Phase 1 part-run, with the corpus- and network-bound channels and the ablation still not started, and link the dated memo from the paragraph that describes the script producing it.
There was a problem hiding this comment.
Pull request overview
This PR prevents recon mcp doctor from masking handshake failures with TemporaryDirectory cleanup errors on Windows by making the isolation-cwd cleanup best-effort, and adds a deterministic regression test for the file-lock scenario. It also updates roadmap/status documentation to reflect that the Phase 1 baseline has been partially run and recorded.
Changes:
- Use
tempfile.TemporaryDirectory(..., ignore_cleanup_errors=True)in_run_handshakeso cleanup failures (e.g., WinError 32 directory locks) don’t overwrite the real handshake error. - Add a platform-independent regression test that simulates the locked isolation cwd by patching
os.rmdir. - Update roadmap and changelog documentation to reflect the Phase 1 baseline’s first dated run and corrected status wording.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_mcp_doctor.py | Adds a deterministic regression test ensuring isolation-cwd cleanup failures cannot mask handshake/session-entry failures. |
| src/recon_tool/mcp_client/doctor.py | Makes the isolation tempdir cleanup best-effort to preserve the original handshake error, especially on Windows. |
| ROADMAP.md | Updates track 3 status to “part-run” and links the first dated baseline run. |
| docs/roadmap.md | Aligns track 3 status wording and adds a concrete reference to the frozen network-free baseline artifact. |
| CHANGELOG.md | Documents the Windows cleanup-masking fix and references the new regression coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refusing the rmdir syscall drove tempfile's internal PermissionError retry ladder rather than the doctor. That ladder differs by version and platform: on 3.12+ it stops at the repeated-path guard, and on 3.11 there is no such guard, so unlink raising IsADirectoryError on POSIX recursed until the interpreter gave up. Ubuntu 3.14 and 3.14t reported RecursionError where the handshake error belonged - the test failing for its own reason and not the product's. Stand in for the lock at the context-manager boundary instead: delete the directory normally, then report it as still held. Same observable condition, no dependence on tempfile internals, and it also asserts the handshake asked for best-effort cleanup in the first place.
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
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.
Why
Last night's
mainCI was red for two unrelated reasons. One was a GitHub Actions incident (The job was not acquired by Runner of type hosted,Failed to resolve action download info) that cancelled most of the matrix. The other was real:_run_handshakespawns the server with an empty tempdir as its cwd - the v1.9.3.4 cwd-shadow defense. Windows keeps that directory locked until the child has fully exited, so tearing the transport down after a failed handshake can lose the race, andTemporaryDirectory.__exit__then raises over the top of the handshake error. The user-visible effect is worse than a flaky test:recon mcp doctorreports a file-lock message where the actual cause of the failure belongs.What changed
ignore_cleanup_errors=True). The directory is a boundary, not state the report depends on; a leftover empty tempdir beats a masked cause. The shadow defense itself is unchanged.validation/2026-08-05-quality-baseline-scorecard.md, directly contradicting the paragraph below it. Corrected to Phase 1 part-run, with the corpus- and network-bound channels and the ablation still open.Verification
uv run python scripts/check.py- all 27 stages green.