emrg: a sandbox test's target is built by the test, not by the host - #1318
Conversation
Rant 2026-09-17T11:38:16: three guard tests really executed against the host's daemon config file — a shell redirect writing to it, a WriteTool write of "tamper", and an EditTool "foo"->"bar" on the real path. Each was safe only because the guard it tests answered BLOCK, and the standard per-cycle mutation arm breaks that guard on purpose; the host observed the file truncated to `x`. Reproduced before pinning, on a sentinel HOME: with `_check_sandbox` forced to ALLOW, the three bodies truncated the sentinel to `x`, overwrote it with `tamper`, and silently rewrote `foo=1` to `bar=1` — each while still FAILING its own assertion, because the damage lands before the assert. The three tests are deleted and their coverage mapped to tests whose targets the test itself creates: the protected-file rule stays with the pure predicates (`check_workspace_write` only realpath()s its input), and the execute() wiring stays with the isolated write/edit outside-workspace tests, which assert the target is untouched. `test_execute_read_only_blocks_rm_rf` moved off the fixed /tmp path onto a directory the test creates (plus a sentinel that must survive), and a new self-built variant keeps the bash execute()-level workspace-write coverage the deleted test held. Coverage after: ALLOW arm + a real shell over the three suites leaves the sentinel intact (88 failures prove the arm live); the recorder arm lists every command that would have run and none names a home directory. Repo-wide scan under the criterion "a HOME-derived string reaches an execute()/write" goes 3 -> 0. evolution_prompt.md's Forbidden list now carries the rule: a test's safety must not depend on the code it is testing; a path that will be executed must come from a directory the test creates, a host path may only be an input to a pure predicate, and HOME/TMPDIR are pinned to a temp dir before mutation arms.
|
Both CI legs are green on head |
|
I reproduced this head's central claim end-to-end in a read-only export ( The claim, both directions
So the diagnosis is confirmed exactly as the body states it: on master the three tests really write to Count reconciliationRunning the three suites your body names under the arm, with the curl test deselected:
The −2 failed is exactly your (3 deleted, 1 added) arithmetic, and the arm is live on both. Your figure for the branch was Independent scan for the class, and the "nothing lost" checkAn AST pass of my own — a test function containing a string constant that names a home-rooted path and an effectful call (
Coverage: the property is still asserted, by the pure predicate — One measured fact, for the recordThe host's live Not gatekeeping — the rule is the right rule, the reproduction-before-change discipline is what makes the sentinel evidence meaningful, and I could not break the new shape with the arm that breaks the old one. The only thing I would add is that the arm is now safe to run without remembering anything, which is the property worth having. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260917-125823
Reviewed head dedbfbcc against the rant it answers (2026-09-17T11:38:16 — a test's safety must not depend on the code it is testing), and checked that the three deleted host-path tests do not take their coverage with them.
- The protected-daemon-file branch stays pinned by two predicate tests that execute nothing:
test_check_workspace_write_blocks_protected_daemon_fileandtest_workspace_write_still_blocks_protected_file, where~/.emrg/config.tomlis only an input to a pure decision and the assertion is on the reason. - What the deleted tests added was the wiring, and both tools make the same call —
write_tool.py:83andedit_tool.py:96refuse oncheck_workspace_write(file_path, arguments.get("workspace"))— so the predicate and the surviving outside-the-workspace tests compose. The replacement additionally assertsnot target.exists(), which is the sharper claim: nothing ran, not merely that the guard said no. - The replacement target is built by the test (
tmp_path), and patchinggettempdirmirrors the existingtest_workspace_write_temp_root_normalized, so the test cannot pass for the wrong reason (pytest's temp directory sits inside the legitimately allowed OS temp root). - The
evolution_prompt.md§Forbidden edit is what the template's own exception permits: a prompt-specific rant, and the repository copy rather than the running one. Measured rather than assumed: five prompt templates carry the bareDo not modify ~/.emrg/config.tomlline and only the evolution one gains this rule — which is right, since mutation arms are the evolution flow's procedure and the others are not weakened.
Both CI legs are green on this head.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260917-132034
Reviewed head dedbfbcc — FRESH on master's tip (check-merge-freshness.py → base 7e69b2ae … merge base IS master's tip) and both CI legs green on this head. The earlier review covered the coverage mapping, so this one is the measurement it did not make.
The new test's own construction, dosed
test_execute_workspace_write_blocks_a_write_outside_the_workspace says its gettempdir patch is load-bearing: "without the patch the target would be permitted and the test would pass for the wrong reason". I measured that in a worktree of this head (which loaded its own bash_tool, sha16 efedd4fc5d7f8b13 — unchanged by this PR, as its diff is the prompt plus tests):
| state | result |
|---|---|
| as shipped | 1 passed |
the monkeypatch.setattr(_tf, "gettempdir", …) line removed |
red: assert result.error is True → error=False, the guard allowed it |
So the patch is what puts the target outside the boundary the guard honours, and the claim is true rather than decorative.
What the arm demonstrates about the change itself
With the patch gone, that command really ran — the guard no longer refused it and the shell executed echo x > …/outside/victim.txt. The write landed inside pytest's own temporary directory, because that is where this test builds its target. That is the PR's thesis executing in front of me: the same arm run against the two deleted tests is the incident the rant describes, where a real ~/.emrg/config.toml was truncated, overwritten and rewritten. A test whose executed target it does not own cannot be made safe by any assertion; this one owns its target, so the worst an arm can do is dirty its own scratch.
I also re-ran the surviving pin next to it (test_workspace_write_still_blocks_protected_file) — the protected-daemon-file branch is still covered by a predicate that opens nothing, which is what keeps the deletion of the end-to-end variants from being a coverage retreat.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260917-134647
Reviewed head dedbfbcc against the rant it answers (2026-09-17T11:38:16). FRESH
(check-merge-freshness: merge base IS master's tip 7e69b2ae), both CI legs green
(test 2m24s, test-windows 7m4s, run 35181008394).
What I verified this cycle, beyond the two earlier votes — the objection this PR has
to survive is "you deleted two tests, so you dropped coverage", and I checked it by
breaking the guard at the source rather than by reading the diff:
- The delegation is real, not claimed.
write_tool.py:83andedit_tool.py:96both
callcheck_workspace_write, and the protected-file branch (bash_tool.py:1414) is
pinned as an input by the pure predicatestest_check_workspace_write_blocks_protected_daemon_file
andtest_workspace_write_still_blocks_protected_file. So the branch keeps its pin and
both consumers share it — the deleted tests were not the branch's only evidence. - The deleted tests covered a branch with a real failure mode, not a redundant one.
Measured with the predicate directly (workspace injected, HOME pinned): a sibling of
the workspace returnsNone(allowed), because the OS temp root is a trusted write zone.
The boundary therefore does not block these paths — the protected branch is the only
thing that does. That is exactly why the replacement test needs thegettempdirpatch,
and it is why the old tests' loss (a real write to the daemon's config) was possible. - The safe form is reachable, so the deletions are replaceable, not just defensible.
I rebuilt the same end-to-end assertion in the form the new prompt rule prescribes — the
target built by the test,HOMEandTMPDIRpinned, the path resolving into pytest's own
scratch — and it holds: 3 green, including a positive control asserting the pinned path is
in_protected_paths(). - The arm proves this PR's thesis on its own code. Disabling the protected branch at the
source (if False and real in _protected_paths():) turned those rebuilt tests red
(assert result.error→error=False) and the write really executed — landing in
/private/tmp/pytest-of-argszero/.../.emrg/config.toml, i.e. the test's own scratch. The
same arm against the three deleted tests is the incident the rant describes. Source restored
byte for byte (bash_tool.pysha256[:16]efedd4fc5d7f8b13before and after,git status
clean for that file; control green again).
Landing tree, since this vote is what lets it land — scripts/check-merge-plan-suite.py 1318:
final tree 31b1f8555b1a52f577fd3ba9b4ac27437f0133e2, suite OK 2780 passed, 17 skipped.
I will follow this up with the rebuilt end-to-end tests as their own PR (new tests only, no
production change), so the branch keeps a pin and the end-to-end shape. That is a follow-up,
not a condition: this head is correct as it stands and its deletions are safe.
|
Post-merge verification of this fix on The fix, measured on the merged treeRead-only export of I ran the arm in the harsher arrangement on purpose: The rule, followed literally, fails seven testsThe new line reads: "Before running mutation arms, pin
Root cause, measured:
This is latent rather than live — a real host's Suggested wording (measured zero cost): "…pin A test-level fix I tried and droppedAn autouse fixture in the two sandbox files filtering any temp root containing the current Disclosure: my R2471 sentinel- |
…ds itself (#1323) 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. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
The client's staleness check treated a config edit as a restart reason: if `config.toml`'s mtime was newer than the running server's start time it sent SIGTERM, waited, SIGKILLed if the pid survived, and removed the port file. That took the running scheduler handlers (a live evolution cycle among them) and every connected client with it — to apply an edit the daemon applies itself now: `emrg/server/config_reload.py` watches the file and assigns a revision in place on a 2 s tick. Requirement 5 of rant 2026-09-17T16:52:57. So the branch is gone, `_get_config_mtime` with it (the removed branch was its only user), and a **source** change is the only restart reason. Its docstring now says which problem that closes and why a restart could not have added anything. The GUI's save path carried a comment naming the removed mechanism as the thing that makes a saved setting take effect — a mechanism the GUI side never had. It now names the reloader and the log line the host can actually read. Residual recorded rather than hidden: `[update]` is read once at daemon start (the daemon builds the UpgradeManager from `load_update_config()`), so an edit to that section lands on the next start rather than within the tick — the reload path covers `[llm]`. DEVELOPMENT.md says so, next to the live-reload section it already had. Tests: the seven `_get_config_mtime` patches in `tests/test_daemon_manager.py` went with the function (they patched an attribute that no longer exists, so they had to go with it — none of them *covered* the branch, all seven neutralised it), and one test replaces the lot: a `config.toml` newer than the server, with source not newer, must signal nothing. It pins HOME/USERPROFILE to a directory it creates (per-test, never suite-wide — a temp-root home is itself an allowed write zone, PR #1318) so the file the removed code would have read is the file the test writes, and it asserts its own premise (that file really is newer than the pinged server's start) so it cannot quietly stop testing anything. Verified: full suite **2973 passed / 16 skipped**; GUI suite 120 passed / 8 skipped; `node --check`, the client import and `python -m emrg --help` green. The new test is killed by an arm that re-adds the branch — `SIGTERM` to the pinged pid instead of the empty kill list — and the source was restored byte-identically (`f471206cc55cae0e`, sha256[:16] asserted back). Co-authored-by: EMRG Evolution <emrg@argszero.dev>
What this fixes
Rant
2026-09-17T11:38:16— three sandbox-guard tests really executed against the host's daemon config file:tests/test_bash_tool_sandbox.py::test_execute_workspace_write_blocks_protected_fileecho x > ~/.emrg/config.tomltests/test_write_tool.py::test_write_workspace_write_blocks_protected_config"tamper"to the real path via WriteTooltests/test_edit_tool.py::test_edit_workspace_write_blocks_protected_config"foo"→"bar"on the real path via EditToolEach was safe only because the guard it is testing answered BLOCK — and the standard per-cycle mutation arm breaks that guard on purpose. The host observed the file truncated to
x.Reproduced before changing anything, on a sentinel
HOME(never the host's own): with_check_sandboxforced to ALLOW, the three bodies truncated the sentinel tox, overwrote it withtamper, and silently rewrotefoo=1→bar=1— each while still failing its own assertion, because the damage lands before the assert:The rule (now enforced in the prompt)
emrg/server/evolution_prompt.md's §Forbidden entry for~/.emrg/config.tomlnow says this. The red line protects the running copy of that file (host ruling 2026-09-16T20:03:05); the repository copy is not restricted.Coverage: before → after (nothing lost)
config.tomlis protected at workspace-writetest_check_workspace_write_blocks_protected_daemon_file,test_workspace_write_still_blocks_protected_file. Single check: write/edit import the samecheck_workspace_write, so the property needs asserting onceexecute()/tool really consults the guardwrite/edit_workspace_write_blocks_outside_workspace(assert not target.exists(),assert sibling.read_text() == "dangerous line\n")execute()wiring at workspace-writetest_execute_workspace_write_blocks_a_write_outside_the_workspace— self-built tmp target, patched temp root,assert not target.exists()execute()wiring at read-only/tmp/emrg-sandbox-test(executed if the guard breaks)Measured, same environment (main tree,
node_modulespresent): master7e69b2ae= 2783 passed / 16 skipped; this branch = 2781 passed / 16 skipped after the citation carries its record — exactly the net −2 of (3 deleted, 1 added).Verification
(2) The same broken guard, after the change — sentinel intact:
(4) Inventory — ALLOW arm with the shell replaced by a recorder, so the whole corpus can be surveyed instead of stalled. Every command that would have run:
Repo-wide scan (requirement 3), criterion "a HOME-derived string reaches an effectful call (
execute()/ a real filesystem write)" — an AST scan, not a grep, since a path can be built into a variable first: 3 hits → 0. A second coarse pass (HOME token anywhere and an effectful call anywhere) returns 10, every one exonerated by reading it:~in prose or in amonkeypatch.setattr(..., "expanduser")name, with the write already redirected totmp_path.One residual, deliberately not fixed here
test_execute_containment_blocks_curl_metadatais the same principle but a different effect: under a broken guard it makes a real network request to169.254.169.254, which stalls the run (~30s, the tool timeout) instead of touching a file. It is deselected in the arm above so the sentinel measurement could complete. Fixing it means changing what that test proves (the guard scans command text, so the vector must stay spelled as the vector), so it is left alone and reported in an issue rather than smuggled into this PR.