Repository navigation
test: isolate each test's working directory and guard the temp-root chain (see #235, #261) - #266
Merged
Merged
Conversation
…hain (see #235, #261) Implements slice A of the hermeticity plan. isolate_cwd (autouse) runs every test from a private directory under pytest's temp root, a SIBLING of tmp_path via mktemp -- never a child, which would make files a test writes into tmp_path ancestors of its own cwd. It requests isolate_trust_store explicitly, since pytest does not guarantee autouse order and the fake HOME must exist first. Two leaks close: * #261: manifest.loader._find_project_manifest walks up from Path.cwd() and stops at Path.home(); because isolate_trust_store redirects HOME to a fake dir, that stop never matched the REAL home, so from a checkout under $HOME the walk reached the developer's own ~/.pmcp/manifest.yaml and gated it as a *project* overlay -- a second consent refusal. The isolation fixture caused the failure; chdir-ing under the temp root makes the walk's existing temp_root stop fire. Verified: fails on main from a real-home cwd, passes here, and removing the chdir turns it red again (assert 2 == 1). * The npm local-prefix artifact: a checkout under a directory holding node_modules put that directory on every un-chdir'ed test's ancestor chain and inverted npm-identity assertions. On this /mnt worktree (volume-root node_modules present) the suite went from 107 failed/106 errors to 4011 passed. assert_clean_ancestor_chain (session) fails ONCE, naming the path, when the temp root's own ancestors carry a package.json/node_modules -- the one case isolate_cwd cannot escape, since tmp_path lives under gettempdir(). It walks only that chain, never Path.cwd(): a session fixture sees the invocation dir, so walking cwd would fail exactly the hosts isolate_cwd has already fixed. Verified both directions: planting /tmp/package.json raises the single named error; planting node_modules on a repo ancestor leaves 100 npm tests passing and the guard silent. npm_resolver._has_local_prefix is deliberately unchanged -- it replicates npm's real local-prefix rule, and weakening it to make tests pass would be the bug. Implementation surfaced a class the plan did not anticipate: ten tests read repo data files through cwd-relative paths. They are not cwd-subjects, so marking them real_cwd would misuse the marker; they now anchor to a repo root via Path(__file__), the idiom already used in tests/test_workflow_guards.py. Only test_the_allowlist_matches_the_committed_tree takes real_cwd -- its subject IS the committed tree at the invocation root, and scripts/check_workflows.py resolves .github/workflows relative to cwd by design. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H4VQypRNMRJ4caC64P7M3w
…is polluted (see #235) Panel CR (codex, blocking): assert_clean_ancestor_chain raised RuntimeError from a SESSION-scoped fixture. Pytest caches that setup failure and re-reports it against every dependent test, so the "one clear error" the guard exists to give became the very cascade it replaces. Measured on this branch: one planted /tmp/package.json produced 100 errors, not one -- my acceptance criterion said "ONE named guard error" and was simply false. My own verification missed it because I grepped the output for the expected message with head -6 instead of counting occurrences; the cascade was visible in that output all along. pytest.exit(msg, returncode=1) ends the session at first detection. Re-measured by counting: 0 errors, 1 guard message, "no tests ran". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H4VQypRNMRJ4caC64P7M3w
This was referenced Sep 21, 2026
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.
Implements slice A of the hermeticity plan (
.consiliency/plans/detailed-test-cwd-filesystem-isolation-20260920-0814.md, merged in #263). Tests only — nosrc/changes.The headline
On a
/mnt/<volume>/worktrees/...checkout whose volume root holdsnode_modules, the suite went from 107 failed / 106 errors → 4011 passed, 0 failed. That artifact has been a standing caveat on every verification run; it is now fixed rather than worked around.What it does
isolate_cwd(autouse) runs each test from a private directory under pytest's temp root — a sibling oftmp_pathviamktemp, never a child (a child would make files a test writes intotmp_pathancestors of its own cwd). It requestsisolate_trust_storeexplicitly, because pytest does not guarantee autouse ordering and the fakeHOMEmust exist first.assert_clean_ancestor_chain(session) fails once, naming the path, when the temp root's own ancestors carry apackage.json/node_modules— the one caseisolate_cwdcannot escape, sincetmp_pathlives undergettempdir(). It walks only that chain, neverPath.cwd(): a session fixture observes the invocation directory, so walking cwd would fail exactly the hostsisolate_cwdhas already fixed.real_cwdmarker registered, scoped narrowly to tests whose subject is the invocation directory.Why #261 was really failing
_find_project_manifestwalks up fromPath.cwd()and stops atPath.home()(the #243 guard).isolate_trust_storeredirectsHOMEto a fake dir, so that stop never matched the real home — from a checkout under$HOMEthe walk sailed past it, found the developer's own~/.pmcp/manifest.yaml, and gated it as a project overlay. The isolation fixture caused the failure. chdir-ing under the temp root makes the walk's existingtemp_rootstop fire.Verification (all run, not asserted)
mainfrom a real-home cwdchdirassert 2 == 1) — falsifiable/tmp/package.jsonplantednode_moduleson a repo ancestor (not a tempdir ancestor)/mnthostruff check/ruff formatScope note the plan did not anticipate
Implementation surfaced a class plan review could not: ten tests read repo data files through cwd-relative paths (
Path("src/pmcp/manifest/manifest.yaml")etc.), which break once cwd moves. They are not cwd-subjects, soreal_cwdwould misuse the marker against the plan's own definition — they now anchor to a repo root viaPath(__file__), the idiomtests/test_workflow_guards.pyalready used. Onlytest_the_allowlist_matches_the_committed_treetakes the marker: its subject genuinely is the committed tree at the invocation root, andscripts/check_workflows.pyresolves.github/workflowsrelative to cwd by design.npm_resolver._has_local_prefixis deliberately unchanged — it replicates npm's real local-prefix rule, and weakening it to make tests pass would be the actual bug.Closes nothing on merge by design; #261 is closed by this behaviour but left to a human to confirm. See #235, #261.
🤖 Generated with Claude Code
https://claude.ai/code/session_01H4VQypRNMRJ4caC64P7M3w
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.