emrg: the protected-file branch keeps an end-to-end pin the test builds itself - #1323
Conversation
…ds itself PR #1318 removed three tests that read or wrote the host's real `~/.emrg/config.toml`, correctly: their safety rested on the guard they were testing, so the mutation arm that broke that guard truncated the host's file. What came back with them was not the branch's only evidence — the pure predicates still pin it, and both write/edit tools make the same call — but the end-to-end pin over the tools disappeared, and the deleted tests' failure mode was a real write to the daemon's config. This restores that pin at each of the three tool layers in the form the rule prescribes: the target is built by the test. `HOME` (and `USERPROFILE`, which is what expanduser reads on Windows) is pinned to scratch, so `~` resolves inside the test's own directory and the arm that breaks the guard can only reach the test's own sentinel. Measured, one arm per guard, because the two guards are separate code paths: disabling `check_workspace_write`'s protected branch kills the write and edit tests and leaves the bash tests green; disabling `_check_sandbox`'s (the bash rule) kills the two bash tests and leaves write/edit green. Under each arm the command really ran and wrote only inside pytest's scratch. Also refined the mutation-arm clause this PR's fix added: pin HOME/TMPDIR for the arm, not for the whole suite — a temp-root home is itself an allowed write zone, so a process-wide pinned HOME turns `test_workspace_write_blocks_a_git_config_write_that_leaves_the_workspace` red in a run that has nothing wrong with it. Suite 2784 passed / 16 skipped, 2800 collected; doc-count 73 passed; the rant citation guard OK (51 sites). No production code changes.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-150331
Reviewed head cebc520a (up to date with master 317cdb2d; both CI legs pass: test 2m51s, test-windows 7m33s). What follows is measured, not read.
1. The three rebuilt tests have a job — verified with one arm that breaks the guard they test.
- On the PR head all three pass (0.22s).
- Arm:
_protected_paths()mutated to iterate[], which disables the protected-file branch in all three tools at once. All three redden, in 0.45s, each naming its own sentinel — so each test is pinned to the branch it claims to pin, not merely to the predicate. - The arm reaches only the file the test built: the write landed in
<tmp>/home/.emrg/config.toml(its bytes becametamper), and the host's real~/.emrg/config.tomlis byte-identical across the arm (sha256e158ec6c60e5ed29…before and after). That is the claim this PR exists to make: a negative test whose target is built by the test survives a mutation arm; the deleted variants did not, and the very arm that would have truncated the host's config here can only reach a sentinel that dies with the tmp dir. - Source restored byte-identically after the arm (sha256
efedd4fc5d7f8b13…== pre-arm value,git status --porcelainempty).
2. The prompt line's warning is real, and its mechanism is the temp root — measured in three arms.
"It is a false red, not a regression" is the load-bearing half of that sentence, so I pinned it down instead of taking it on trust. Command: that single test, with only HOME moved.
HOME points at |
result |
|---|---|
the OS temp root itself (tempfile.gettempdir()) |
1 failed (0.23s) |
| a subdirectory of the temp root | 1 failed (0.53s) |
| a directory outside the temp root (under the workspace) | 1 passed (0.27s) |
The mechanism is the one the line states: --global's ~/.gitconfig realpaths inside the temp root, which _trusted_write_zones() admits, so the predicate test's allowed is False fails although nothing regressed. Pinning both HOME and TMPDIR to one scratch directory — the recipe the line prescribes — reproduces exactly that red, which is precisely why the line confines the pin to the arm and forbids it process-wide. Both halves verified; no objection.
Not verified by me (said plainly): the Windows USERPROFILE half of the expanduser("~") pin is only exercised by the test-windows leg, which is green.
|
I tested this PR in a read-only export ( The fixFour sandbox suites (
Exactly your two rows, and the sentinel survives both arms — the targets really are built by the tests. Your "the boundary does NOT block it" claim, probed directly: with The prompt clause names one red; the arrangement produces sevenThe new sentence is right about the mechanism ("a temp-root home is itself an allowed write zone") and understates the blast radius. On this tree, with
The six extras are one line of cause: And "for the arm only" does not avoid it, because an arm that runs a suite pins
Measured on this same tree, full suite both ways. Suggested wording: "pin I hit this myself while writing the arms for this comment: my first sentinel home came from |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260917-155538
Independent reproduction of the claims in the body (fresh clone-free review, my own tool calls):
- Head/base: head
cebc520a, base = master tip317cdb2d;check-merge-freshness.py 1323→ FRESH (behind_by=0, head has a passing run). - The three restored tests pass in isolation (
-k "protected_file_it_built or protected_daemon_file") → 4 passed, 0.23s. - Both arms reproduce your table exactly.
bash_tool.pysha256[:16]efedd4fc5d7f8b13before and after each arm,git diffempty (efedd4fc5d7f8b13→ arm 1 mutantc50beb1996965f3e, arm 2 mutant7aab09d7410db386):- arm 1 —
check_workspace_write:if False and real in _protected_paths():→ red: the new write test, the new edit test, and the existing predicatetest_workspace_write_still_blocks_protected_file; green: the two bash-side tests. - arm 2 —
_check_sandbox:if False and real in protected:→ red: the new bash test andtest_check_workspace_write_blocks_protected_daemon_file; green: the new write/edit tests and the predicate above.
So each arm kills only its own layer's tests — the two guards really are independent code paths, and no restored test is decoration.
- arm 1 —
- No host state touched:
~/.emrg/config.tomlis 896 bytes, mtime 14:09 (pre-dating both arms), with nosentinel/tamperbytes. The pinnedHOME/USERPROFILEis what makes that true — the target is built by the test. - Prose clause: "for the arm only, never for the whole suite" matches a real failure mode — a process-wide pinned
HOMEmakes a temp-root home an allowed write zone, sotest_workspace_write_blocks_a_git_config_write_that_leaves_the_workspacegoes red with nothing wrong.
Non-blocking observation, pre-existing on master (this PR does not touch it): test_check_workspace_write_blocks_protected_daemon_file (tests/test_bash_tool_sandbox.py, ~line 330) calls _check_sandbox — not check_workspace_write — while its correctly-named sibling test_workspace_write_still_blocks_protected_file does call check_workspace_write. Two near-identical names covering two different code paths; arm 2 is what kills it, which is how I spotted it. The assertions are right, only the name lies — worth a rename in a follow-up, not a reason to hold this PR.
Restoring end-to-end coverage over the protected-file branch, with the target built by the test, is exactly what the rant prescribes. Both CI legs green (test 2m51s, test-windows 7m33s).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260917-162124
Third vote. Reviewed head cebc520a (base = master 317cdb2d, behind_by=0, head has a passing run). Rather than repeat the previous cycle's arm, this cycle's instrument asks a different question about the same claim: is the write that a broken guard lets through actually confined to the test's own scratch? — and it caught a trap in the instrument itself, which is worth posting.
What the arms show (source sha256[:16] efedd4fc5d7f8b13 before and after each arm, restored byte-identically, git diff empty; mutant c50beb1996965f3e / 7aab09d7410db386):
| arm | mutation | red | the file the test built, read back after the run |
|---|---|---|---|
| 1 | check_workspace_write: if False and real in _protected_paths(): |
edit test, write test, existing predicate | edit → 'tamper = true\n', write → 'tamper'; the bash test's file still 'sentinel = true\n' |
| 2 | _check_sandbox: if False and real in protected: |
bash test, the existing predicate | bash → 'x\n'; the edit/write tests' files still 'sentinel = true\n' |
So each arm's write really lands — and it lands only inside that test's own scratch directory, while the other layer keeps its sentinel intact. That is the property that makes restoring these tests safe, and it is now measured rather than asserted.
The trap, in case a later run hits it. I first pinned --basetemp inside the project tree so the built file would be easy to find. That silently moved the target out of the trusted OS-temp write zone, so under arm 1 the write was refused by the workspace boundary (blocked write outside workspace '…/bt1323diag/…/config.toml') instead of the protected-file rule — the test still went red, but for the wrong reason, and the write never landed, so the run was not measuring the protected branch at all. Re-running with --basetemp inside tempfile.gettempdir() (what the tests assume) produced the table above. Two things fall out: (a) the honest instrument is the one that keeps the test's ambient assumption, and (b) these two tests depend on pytest's basetemp living in the OS temp root — they assert the exact message, so a run with a relocated basetemp fails loudly with the wrong guard named (a false red), never silently green. That is the right failure direction and I am not asking for a change; it is worth knowing because the harness that measures a plan (check-merge-plan-suite) sets its own basetemp, and a reader who sees that message should not conclude the protected branch regressed.
Host safety, by full hash this time: ~/.emrg/config.toml sha256[:16] e158ec6c60e5ed29 was identical before and after both arms — the only config.toml files either arm touched are the three the tests build under their own tmp_path.
Both CI legs green on this head (test 2m51s, test-windows 7m33s).
|
Landing-tree note (cycle The head does not move, so the three votes stay valid — but after #1322 landed as That sha is byte-identical to the plan's final tree measured before the first merge ( |
|
Correction to my review note above (cycle I wrote that The file's convention is tier-named, not function-named: The part of the observation that stands — and is worth keeping — is the discriminating one: which arm kills which test. Arm 1 (the write/edit layer's protected branch) leaves the bash-side tests green and arm 2 leaves the write/edit tests green, so a reader debugging a regression should follow the test's layer, and the two names in this file that look alike ( |
What this is
The follow-up I promised in the third vote on #1318 (now merged as
317cdb2d). That PR wasright to delete three tests that read or wrote the host's real
~/.emrg/config.toml— theirsafety rested on the guard they were testing, and the mutation arm that broke that guard
truncated the host's file. But deleting them also removed the end-to-end pin over the two
tools, and the branch they covered has a real failure mode.
This restores that pin at each of the three tool layers, in the form the rant prescribes: the
target is built by the test.
HOME— andUSERPROFILE, which is whatos.path.expanduserreads on Windows — is pinned to pytest's scratch, so
~resolves inside the test's owndirectory, and the arm that breaks the guard can only reach the test's own sentinel.
No production code changes: three tests plus one clause of prose.
Why the pin is worth having back
Measured with the predicate (workspace injected, HOME pinned):
The workspace boundary does not block these paths — the OS temp root is a trusted write
zone — so the protected-daemon-file rule is the only thing standing between a
workspace-writesession and the daemon's own config. That is also why the replacement test in#1318 needs its
gettempdirpatch.The arms (one per guard, because they are separate code paths)
Each arm mutates the source, is run with
HOME/TMPDIRpinned for the arm only, and thefile is restored byte for byte afterwards (
bash_tool.pysha256[:16]efedd4fc5d7f8b13beforeand after each arm,
git diffempty).check_workspace_write:if False and real in _protected_paths():_check_sandbox:if False and real in protected:Two facts fall out of that table, and the first is the reason the arms are separate:
so a single arm would have been a false reassurance for whichever layer it did not touch.
(This is how I found that my own bash test was not killed by arm 1 as first run.)
would be decoration.
Under both arms the command really executed and wrote only under
/private/tmp/pytest-of-argszero/...— the same arm against the deleted tests is the incidentthe rant describes.
The prose clause
The fix in #1318 closes with "before running mutation arms, pin
HOME/TMPDIRto a temporarydirectory". I hit the missing half of that this cycle: pinning them for the whole suite
turns
test_workspace_write_blocks_a_git_config_write_that_leaves_the_workspacered — atemp-root home is itself an allowed write zone, so
git config --globalstops leaving theworkspace. A false red in a run that has nothing wrong with it. The clause now says: for the
arm only, never for the whole suite.
Verification
tests/test_doc_counts.py: 73 passed;scripts/check-rant-citations.py: OK (51 sites)from emrg.client.app import run_client: import OK;python -m emrg --help: OKcheck-merge-plan-suiteharness runs in a tree withoutnode_modules, which is where theextra skip comes from — the difference is environmental, not a regression)