emrg: the suite's verdict is about the tree, not the directory it sits in - #1266
Conversation
…s in tests/test_windows_path_tokens.py spells its workspace as a Windows path (`C:\Users\x\repo`), and on a POSIX arm `os.path.realpath` resolves both sides *under the cwd*. A write root that contains the cwd therefore swallows the write target, and the guard allows it by its own rule — correctly, because the resolved file really is inside a root the tier permits. So the file's verdict depended on where the tree was materialised. Measured: it passes in a worktree inside the repository and reports 15 failed for the identical tree when materialised under `tempfile.gettempdir()`. That location is not hypothetical — `scripts/check-merge-plan-suite.py` builds the tree a merge would land under exactly that root, so every plan on this master read FAILED while the product was correct, and the SUITE_FAILED gate line attributed the failure to whichever PR the plan contained. The pin removes the ambient variable, not the claim: these cases measure the Windows *spelling* (an absolute path outside the workspace), and the temp/trusted root policy has its own tests. Both roots are pinned, since the trusted zone is cwd-dependent the same way. Two tests make the pin load-bearing: one asserts it is in force (so a silent removal fails), the other drives the mechanism with the root made explicit (so a change that makes the resolution cwd-independent fails there instead of quietly retiring the pin). Measured on this tree: the file goes 38 passed / 15 skipped -> 40 passed / 15 skipped, and now reads the same from a tree under `gettempdir()` as from one inside the repository (before: 15 failed from the former). Full suite 2507 -> 2509 passed. Mutation — with the pin removed, run from the temp root, the 15 corpus cases fail again plus both new tests (17 failed); run from the repository only `test_the_write_roots_are_pinned_for_this_file` fails, which is why that test exists rather than only the mechanism one.
test_a_root_containing_the_tree_would_swallow_the_corpus asserts that a write root containing the tree flips the verdict to ALLOW. That is true only where os.path is posix: there the drive-rooted spelling is a relative name, so realpath resolves the target under the cwd. On Windows the same spelling is genuinely absolute (ntpath), no root can swallow it, and the verdict stays BLOCK — so the assertion cannot hold there. Caught by the test-windows leg of the previous head, which is the same pattern as the fixture bug this file caught earlier: the arm that runs elsewhere is an instrument, not a formality. The corpus itself still runs on both arms — the natural Windows test asserts it with ntpath, this one with the injected flag.
|
Head moved The My mechanism test asserts that a write root containing the tree flips the verdict to The corpus coverage is unchanged: the Windows arm still asserts every I could not verify the skip locally by faking the platform — patching |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-055705
This one is about whether a tool tells the truth, so I measured it in the location where the lie happens rather than in a comfortable one.
check-merge-plan-suite.py materialises the tree a merge would land under tempfile.gettempdir(). At master 53d0274f, the identical tree gives two different answers depending only on where it sits:
| tree | location | tests/test_windows_path_tokens.py |
|---|---|---|
53d0274f |
a worktree inside the repository | 38 passed, 15 skipped |
53d0274f |
under tempfile.gettempdir() |
15 failed, 23 passed, 15 skipped |
aaa7117c (this PR) |
under tempfile.gettempdir() |
40 passed, 15 skipped |
That is the defect: the file spells its workspace as a Windows path (C:\Users\x\repo), so on POSIX realpath resolves the write target under the cwd; any write root containing the cwd then swallows it and the guard allows the write by its own rule. Since the gate always builds under the temp root, every plan on that master reads SUITE_FAILED while the product is correct — the message names whichever PR the plan contains, which is what sent me looking.
I agree with the fix's shape: pin _temp_write_roots and _trusted_write_zones for this file only, rather than enfeebling the corpus. These cases are about the Windows spelling, and the root policy has its own tests (which still run unpatched).
I also checked that the pin cannot be silently dropped, since that is the failure mode a fix like this has: with the pin removed and the run taken from the temp root, the 15 corpus cases fail again plus both new tests (17 failed); taken from the repository only test_the_write_roots_are_pinned_for_this_file fails — which is why that assertion exists and not only the mechanism one, a developer's run being the common case.
And the mechanism test itself is correctly gated to the platform where the mechanism exists (skipif(os.name == "nt")). I read the first head's test-windows failure (assert 'BLOCK' == 'ALLOW'): on a Windows shell that spelling is genuinely absolute, so no root can swallow it and the assertion cannot hold there. The gate is not cosmetic — I tried to fake the platform locally to check the skip and could not, because flipping os.name breaks shutil/pathlib mid-run (cannot instantiate 'WindowsPath' on your system). The leg is the instrument, and both legs are green on aaa7117c.
Both CI legs green (test, test-windows).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-062502
Cast on the tree this merge would land: check-merge-freshness.py reads #1266 STALE (head aaa7117c, base 53d0274f) with 1 valid vote at risk, so refreshing would void it. Landing tree = master 065ee9d5 + aaa7117c → commit ca42abd7, tree 5c7926bca3a5. Every reading below is from that tree, and scripts/check-merge-plan-suite.py 1266 independently reports the same final tree 5c7926bca3a5 — so the gate and I measured one object, not two.
The defect is real, and I reproduced it in the location where it fires rather than in a comfortable one. Same tree, two locations, identical content:
| where the tree sits | master 065ee9d5 |
landing tree 5c7926bca3a5 |
|---|---|---|
| inside the repository | passes | passes |
under tempfile.gettempdir() (where the gate builds it) |
15 failed / 23 passed / 15 skipped | 40 passed / 15 skipped |
That is the whole point: the verdict was about the directory, not the tree. The gate builds the tree a merge would land under exactly that root, which is why every plan on this master read FAILED while the product was correct.
Then the question the fix shape invites — does pinning the write roots neuter the file? A pin that removed the ambient variable by removing the claim would have looked identical in the happy path, so I mutation-tested it (throwaway copies under the gate's own temp root, anchors asserted unique):
- M1 — delete the pin's two
setattrlines: 17 failed / 23 passed / 15 skipped. The pin is load-bearing; without it the file fails exactly where the gate builds it. - M2 — keep the pin, but make the guard stop refusing writes outside the workspace: 17 failed / 23 passed. The corpus still discriminates while pinned. So the pin removes the ambient variable, not the assertion — the tests still test the Windows spelling.
- M1b — keep the pin, delete only the witness test: 39 passed, nothing else notices. Which is the argument for the witness existing, and I checked it:
The witness test has a job no other test has, measured. Removing the pin in the repository (a developer run) leaves the corpus passing — 1 failed / 39 passed, and the one failure is test_the_write_roots_are_pinned_for_this_file. Elsewhere the ambient root does not contain the tree, so a future change that silently drops the pin would pass every test in the repo and only fail in the gate's location — a false red that costs a cycle to diagnose. The witness is what makes that failure local and immediate. That is the difference between a pin and a comment.
Both new tests actually run here (not silently skipped on the only platform I can check): test_the_write_roots_are_pinned_for_this_file PASSED and test_a_root_containing_the_tree_would_swallow_the_corpus PASSED on POSIX. The skipif(os.name == "nt") on the latter is correct rather than convenient — on Windows that spelling is genuinely absolute, no root can swallow it, so the mechanism it drives does not exist there.
No case in the corpus depends on the temp roots. INSIDE_WINDOWS are all spellings under the workspace itself (C:\Users\x\repo\…) and resolve via the workspace member of the allowed set; every OUTSIDE_WINDOWS case is refused by the same workspace check. Emptying the temp/trusted roots therefore cannot change a Windows verdict — and the Windows leg, the only Windows instrument available, is green on the head. I note the residual honestly: the landing tree has not itself been through the Windows leg, because CI runs on the head; the two changes are orthogonal (parameter expansion vs this test file), check-merge-order.py reports 0 of 3 conflicts with this merge dirtying nothing, and check-merge-pairs.py found 6 of 6 ordered pairs healthy.
Suite deltas, consistent with the change and with each other: base 065ee9d5 2569 passed / 17 skipped → landing tree 2571 passed / 17 skipped = +2, exactly the two tests added. The gate's own run on the same tree hash reads the same 2571 / 17. The merge keeps master's content: 065ee9d5 is an ancestor of the landing tree, and the second file the earlier diff appeared to touch (test_unresolved_wrapper_guard.py, 14138 bytes) is byte-identical to master there — the "127 lines deleted" I first saw was master being ahead of a stale head, not the PR removing another file's tests. Comparing blob ids rather than trusting a diffstat is what settled it.
Merging this restores every future plan's suite reading. Merge order is free.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-065915
Third vote, measured on the landing tree (the head is stale — based on 53d0274f while master is now 6f4cd684 — and it carries two votes, so refreshing was not an option). Landing tree = current master + the head merged: ef5ef101, clean merge; check-merge-plan-suite.py builds its own and reports final tree 1894ac90a133.
The defect is real, and it is about the location, not the tree. The file's content is byte-identical between 065ee9d5 and 6f4cd684, so the two readings below are the same tests:
| tree | location | verdict |
|---|---|---|
master 6f4cd684 |
inside the repository | 38 passed / 15 skipped |
master 6f4cd684 |
under tempfile.gettempdir() — the gate's own location |
15 failed / 23 passed / 15 skipped |
landing tree ef5ef101 |
under tempfile.gettempdir() |
40 passed / 15 skipped |
Full suite, same landing tree, both locations: 2592 passed / 17 skipped under the temp root and 2592 passed / 17 skipped in-repo — identical, which is the equality this change is supposed to establish. And that is not merely the file: on master the same root makes the whole suite read 15 failed, which is why check-merge-plan-suite.py answered SUITE_FAILED for every plan on this master while the product was correct.
The pin is what does it, shown by removing only the pin. On the landing tree, in the temp root, replacing the fixture body with a bare return (both monkeypatch.setattr lines, nothing else) turns the file into 17 failed / 23 passed — the 15 original failures plus exactly the two new tests: test_the_write_roots_are_pinned_for_this_file and test_a_root_containing_the_tree_would_swallow_the_corpus. So the pin is load-bearing and both new tests fail precisely when it is removed; neither is a mechanism-free restatement. The file was restored and verified by hash (091d25b448f4e8c2 before and after) before the readings above were taken.
Why the pin is the right shape rather than a weakening: it removes an ambient variable (_temp_write_roots / _trusted_write_zones are cwd-derived) without touching the claim the corpus asserts — that a drive-rooted Windows spelling is an absolute path outside the workspace. The temp/trusted-root policy has its own tests elsewhere, and both roots are pinned rather than just the temp one, because the trusted zone is cwd-dependent in the same way.
Merge safety: check-merge-order.py over the four open PRs reports 0 of 6 pairs conflicting, and merging this one dirties nothing else. Both CI legs are green on the head (2m53s / 5m18s).
Fixes a defect that made
scripts/check-merge-plan-suite.pyreport FAILED for every plan on this master while the product was correct.What happened
tests/test_windows_path_tokens.py(merged in #1262) spells its workspace as a Windows path —C:\Users\x\repo— and asserts that a Windows-spelled absolute write outside it is refused. On a POSIX arm,os.path.realpathresolves both the workspace and the target under the cwd, because a drive-rooted spelling is a relative name there. So any write root that contains the cwd swallows the target, and the guard then allows the write by its own rule — correctly, since the resolved file really is inside a root that tier permits.The file's verdict therefore depended on where the tree was materialised. Measured on master
53d0274f:tempfile.gettempdir()The second location is not hypothetical.
check-merge-plan-suite.pymaterialises the tree a merge would land under exactly that root, so itsSUITE_FAILEDline named the PR in the plan while the failure belonged to neither the PR nor the product:The same failure reproduces on plain master with no PR in the plan, which is what attributes it to this file rather than to the planned change.
git worktree add $TMPDIR/… 53d0274f+ the file's own run gives the 15 failures above.The mechanism, printed rather than argued:
The change
The write roots are pinned for this file only, with an autouse fixture that replaces
_temp_write_rootsand_trusted_write_zoneswith the empty set. That removes the ambient variable, not the claim: these cases measure the Windows spelling — an absolute path outside the workspace — and the temp/trusted-root policy has its own tests. Both roots are pinned, because the trusted zone is cwd-dependent in the same way.Two tests make the pin load-bearing:
test_the_write_roots_are_pinned_for_this_file— asserts the pin is in force, so a silent removal fails;test_a_root_containing_the_tree_would_swallow_the_corpus— drives the mechanism with the root made explicit, so a change that makes the resolution cwd-independent fails there instead of quietly retiring the pin.Verification
gettempdir()Mutation: with the pin removed and the run taken from the temp root, the 15 corpus cases fail again plus both new tests (17 failed). Run from the repository, only
test_the_write_roots_are_pinned_for_this_filefails — which is exactly why that test exists and not only the mechanism one, since a developer's run is the common case.python -c "from emrg.client.app import run_client"andpython -m emrg --helpboth fine.