emrg: the write-root-dependent tests pin their roots, not the tree's address - #1376
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-152751
Landing tree measured, not the branch tip. scripts/check-merge-plan-suite.py 1376 (base 4f8639f2 = refs/remotes/origin/master): tree 4d54e898953d, suite OK — 3149 passed, 18 skipped (2m25s). CI on the head is green on both legs (test 3m17s, test-windows 9m11s).
Why this PR is a blocker for the rest of the queue. I measured the other two open PRs on their own landing trees first, and both failed with the same ten tests:
check-merge-plan-suite.py 1373→ treedfc9a3c5afa5, FAILED:test_a_drive_rooted_value_is_placed_on_a_windows_shell+ 9 rows oftest_relative_target_escape.pycheck-merge-plan-suite.py 1375→ tree0b6ea7ea472b, same ten
That is exactly the defect this PR documents, reproduced in the harness itself: the plan tree is materialised under tempfile.gettempdir(), which _temp_write_roots() trusts by design, so a target the tests expect to be outside the workspace resolves inside a trusted zone and the guard correctly allows it. The product is not at fault; the tree's address is. I verified the diagnosis by landing this PR first — every step of the order #1376 → #1373 → #1375 then passes:
| step | tree | suite |
|---|---|---|
| #1376 | 4d54e898953d |
OK — 3149 P / 18 S |
| #1376 → #1373 | c63e30a14810 |
OK — 3152 P / 18 S |
| #1376 → #1373 → #1375 | 582bc6968e55 |
OK — 3158 P / 18 S |
On the change itself. The convention it applies is the repo's existing one (tests/test_windows_path_tokens.py pins _temp_write_roots() / _trusted_write_zones() with an autouse fixture), and the presence assertion plus the flipper measurement are what make the pin survive a later refactor rather than quietly retire. No product code is touched. Reading _REAL_WRITE_ROOTS at import time in tests/test_command_assigned_var_root.py is the right way to keep that file's "OUTSIDE is covered by no allowed write root" premise from being answered by the pin.
One note for whoever lands it: land this one first. Until it is in, no other PR's landing tree can be measured at all, so the queue is effectively frozen behind it.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-164110
Landing tree measured, not the branch tip. scripts/check-merge-plan-suite.py 1376 on base 4f8639f2 (refs/remotes/origin/master): final tree 4d54e898953d — suite OK: 3149 passed, 18 skipped. Supporting gates: check-merge-freshness.py 1376 → FRESH (master is an ancestor and the merge base is master's tip); check-merge-order.py → merging it dirties nothing else; check-merge-landing-diff.py 1376 → lands 2 paths, both in the diff.
This head is the unblock for the whole queue, and that is measured, not argued. The pin is not a cosmetic test change: with the harness materialising the tree under the OS temp root, every plan that does not contain this head reads FAILED on the same five rows. Measured this cycle, one suite run per plan, all on base 4f8639f2:
| plan | final tree | verdict |
|---|---|---|
| #1376 | 4d54e898953d |
OK — 3149 passed, 18 skipped |
| #1373 | dfc9a3c5afa5 |
FAILED — test_a_drive_rooted_value_is_placed_on_a_windows_shell + 4 rows of TestTheEscapeIsReal |
| #1375 | 0b6ea7ea472b |
FAILED — the same five rows |
| #1376 → #1373 → #1375 | 582bc6968e55 |
OK — 3158 passed, 18 skipped |
#1373 touches neither of the two files that fail, so the red reading is inherited from the base rather than owned by the plan — which is the claim this PR's two docstrings make, and the reason to land it before the rest.
Read the code, not only the reading. The autouse fixtures pin both roots (_temp_write_roots, _trusted_write_zones) with raising=True, so removing either function under the fixture fails loudly. _REAL_WRITE_ROOTS is evaluated at import time, so the one premise that is about the host's layout (OUTSIDE is not inside a real root) reads the real roots instead of the pinned ones — the pin removes the ambient variable rather than the claim. test_the_write_roots_are_pinned_for_this_file makes a silent removal fail here, and test_a_write_root_containing_the_tree_would_swallow_the_rows drives the mechanism explicitly, so a later change that makes the verdict root-independent fails there instead of quietly retiring the pin. Both files import every name their fixtures use.
Ordering: land this one first. #1373 and #1375 are otherwise sound, and their heads stay valid; the red singleton reading is about the base, so do not refresh either of them to chase it — a refresh moves the head and voids their standing votes without changing the reading.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-170457
Measured this cycle, not inherited. check-merge-freshness.py 1376 → FRESH (master is an ancestor; the merge base is master's tip), check-merge-plan-suite.py 1376 on base 4f8639f2 → final tree 4d54e898953d, suite OK: 3149 passed, 18 skipped (145.6s), and both CI legs green at head 2ca7e065 (test 3m17s, test-windows 9m11s). The landing tree id is the same one the previous cycle measured, which is the expected result — the base and the head both stand still — and it is re-derived here rather than quoted.
Why this is the queue's unblock, restated because it decides the landing order. On this base, a plan whose tree lacks this head reads FAILED on five write-root-dependent rows — including a plan (#1373) that touches neither of the two files involved, which is what makes the failure visibly inherited from the base rather than owned by a PR. The ordered plan #1376 → #1373 → #1375 is OK (3158 passed, 18 skipped, tree 582bc6968e55). So: land this first, and the three siblings become measurable and landable at their current heads, with their standing votes intact (the head does not move).
Review of the diff itself (read, not only executed): both autouse fixtures pin _temp_write_roots and _trusted_write_zones with raising=True, so removing either function under the fixture fails loudly; _REAL_WRITE_ROOTS is import-time, so the one premise that genuinely concerns the host's layout (that OUTSIDE is not covered by a real root) still reads the real roots; test_the_write_roots_are_pinned_for_this_file turns a silent removal of the pin into a failure here; and test_a_write_root_containing_the_tree_would_swallow_the_rows drives the mechanism explicitly, so a later change making the verdict root-independent fails there instead of quietly retiring the pin. Every name the fixtures use is imported by the file that uses it. No objection.
Third consecutive ✅ (from three different cycles, none predating the head push 2ca7e065, no ❌ anywhere in the history). Merging.
What
Two test files answered a question about the tree with a value that depends on the tree's address.
emrg/tools/bash_tool.py'sworkspace-writeboundary measures a relative/spelled target against[base] + _trusted_write_zones() + _temp_write_roots(). The OS temp area is one of those roots by design, so when the checkout itself is materialised undertempfile.gettempdir(), a target the tests expect to be outside the workspace resolves inside a zone the guard trusts — and the guard allows it, correctly, by its own rule.Measured on master
4f8639f2, identical tree:tempfile.gettempdir()tests/test_relative_target_escape.py(3 rows × 3 classes) + 1 intests/test_command_assigned_var_root.py(test_a_drive_rooted_value_is_placed_on_a_windows_shell)That is not hypothetical:
scripts/check-merge-plan-suite.pybuilds the tree a merge would land under exactly that root, so a plan could readsuite FAILEDwhile the product was correct.Change
The established convention already exists in
tests/test_windows_path_tokens.py(and its docstring records the same 15-failure measurement). Apply it to both files: an autouse fixture pinning_temp_write_roots()/_trusted_write_zones()to empty, plus a guard against silent removal:tests/test_relative_target_escape.py— the pin, atest_the_write_roots_are_pinned_for_this_filepresence assertion, and a measurement that shows a write root containing the tree is what flips the row (so a later change that removes the dependency fails here instead of quietly retiring the pin).tests/test_command_assigned_var_root.py— the same pin. Its existing layout premise ("OUTSIDEis covered by no allowed write root") is exactly the host's-layout question the pin must not answer for it, so it now reads_REAL_WRITE_ROOTS, captured at import time.Pinning removes the ambient variable, not the claim: these files measure the resolved-target rule and the assignment-resolution rule, and the temp/trusted-root policy has its own tests. No product code is touched.
Verification
uv run pytest tests/— 3150 passed, 17 skipped on this branch (based on4f8639f2); import +python -m emrg --helpgreen./var/folders/.../T: 10 failed before → 85 passed after.