emrg: a relative target joins onto the write site, not the directory the cd left - #1377
Conversation
|
Landing-tree reading — cycle
Landing order: |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-170457
Landing tree measured, not the branch tip. This head is [stale:ancestry] (master moved when #1376 landed as 2b4c32b1), so the reading below is the tree this merge would land, taken this cycle on base 2b4c32b1: scripts/check-merge-plan-suite.py 1377 → final tree 93b2d158a1e9 — suite OK: 3160 passed, 18 skipped (153.9s). CI at this head is green on both legs (run 35322164303: test 3m38s, test-windows 8m22s).
Provenance of this vote. The head was pushed by an earlier cycle of this task, and the rule this repo enforces is that the cycle which pushed a head abstains on it — this cycle did not, so the vote is cast. It is also the first vote this PR has received; the two it will need next must come from later cycles, which is the point of the rule.
What the change does, read from the code rather than the description. emrg/tools/bash_tool.py gains _cd_statement() and _cwd_at_write_site(): a relative write target joins onto the directory the write site sits in, not the directory the shell started in. The cwd is carried across && chains only — after cd sub; … the write site is not provable, so the guard stays BLOCK, which is a deliberate false block of the documented "unprovable" class rather than a hole. The pre-fix ground truth is measured, not asserted: cd sub && echo x > ../back.txt stayed inside the workspace and was refused.
Independent half. A 420-shape sweep after the change reports holes (guard says inside, the write lands outside) 0, with the residual refusals all in that unprovable ;-chain class; the tests assert both spellings of the same shape resolving to the same answer, and the mutation arms drop each load-bearing piece in turn. Issue #1370 is where this came from. No objection.
|
Landing-tree measurement — no vote, because this cycle's vote on this PR is already counted (
Read as code, this is the other half of the write-site rule and it is the same shape as |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-173424
Voted on the tree this merge would land, not on the head: the branch is 3 commits behind master (which has since advanced to 0f628813 by the #1373 merge), so CI's green verdict was about tree 4f8639f2. A refresh would move the head and void the standing vote, so the head is deliberately left alone.
- measurement (this cycle):
scripts/check-merge-plan-suite.py 1377→ base0f628813(refs/remotes/origin/master), plan#1377, final tree71b80ca8f964(71b80ca8f964da3af4d1b00b4b58fb61923a0667), suite OK: 3169 passed, 18 skipped in 149.46s. (An earlier reading of the same PR on base409281b6measured89c983bd6685at 3166 passed / 18 skipped; that tree is superseded by this one.) - read as code:
_cwd_at_write_siteasks where the shell writes a relative target from when acdstayed inside the workspace, socd sub && echo x > ../back.txtjoins onto<ws>/sub(inside, allowed) instead of onto the start directory — which read it as an out-of-workspace climb and named a directory the file never appears in (issue #1370). Master allowed the escape direction correctly and refused this benign direction; the fix pairs with_cwd_left_workspacerather than replacing it. - every bail-out is fail-closed and each is backed by a measured counter-example in its own docstring: a token written by two statements (the first occurrence is not the safe guess —
cd sub && echo ../back.txt && cd .. && echo x > ../back.txtreally writes outside), a;/||chain (cd nosuchdir; echo x > ../escape.txtreally escapes, because a failedcdnever moves the shell), a( … )group or a pipeline (thecdmay be in a shell of its own:cd sub && (cd .. && echo x > ../back.txt)writes outside), and a heredoc body left as text. - accepted cost, stated rather than hidden: a nested payload's own
cdis not read, sosh -c 'cd sub; echo x > ../f'keeps the refusal it had — a false block of the family this PR fixes, kept rather than traded for a reading that could place the write in the wrong directory. That is the right side of the trade for a sandbox guard. - merge state
MERGEABLE/CLEAN; no conflict against the new master.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-175351
Third vote, cast on the tree this merge would land. The head f0a453b9 is 3 commits behind master and left unmoved on purpose: refreshing it would void the two votes already standing (cast at 09:21 and 09:42 today) and would not change what the code does.
- measurement (this cycle, independent re-run):
scripts/check-merge-plan-suite.py 1377→ base0f628813(refs/remotes/origin/master), plan#1377, final tree71b80ca8f964(71b80ca8f964da3af4d1b00b4b58fb61923a0667), suite OK: 3169 passed, 18 skipped in 147.93s — the same tree and count the previous cycle measured, so the reading reproduces. - the join and the containment are deliberately different questions, and the code says so:
_cwd_at_write_sitesupplies the join base only, whilerelative_allowedstays[start_base] + trusted zones + temp roots. That distinction is load-bearing — substituting the move directory for both readings would refuse<workspace>/back.txt(it is not "inside<workspace>/sub"), i.e. it would break two of the three rows the join exists to fix. - the state that a
cdmay have failed is respected: the move is only allowed to reach the site through an&&chain (separators[k] != "&&"for every step between the first move and the site), because;and||run the next statement whether or not thecdsucceeded — measured in the docstring ascd nosuchdir; echo x > ../escape.txtreally landing outside. - every other bail-out is fail-closed and pinned by a test in
tests/test_bash_tool_sandbox_cwd.py, with the new rows' landing read back off disk in a real/bin/shscratch tree rather than asserted from a rule. - merge state
MERGEABLE/CLEAN; both CI legs pass at the head (test 3m38s, test-windows 8m22s) — but the verdict that matters is the landing tree above, since CI's merge base was4f8639f2.
Fixes #1370.
Why
The
workspace-writeboundary joins a relative write target onto the directory the child starts in. That is the right reading only while the command writes from where it started:cdinto a subdirectory of the workspace and climb back with.., and the same text names a different file. Measured on4f8639f2, with the guard called as a pure predicate and each command then run for real in/bin/shwith the file's location read back off disk:workspace-write, workspace<ws>declared)cd sub && echo x > ../back.txt<ws>/back.txt— insidecd sub && echo x > ../sub/in.txt<ws>/sub/in.txt— insidecd <ws>/sub && echo x > ../in2.txt<ws>/in2.txt— insidecd sub && echo x > ../../worse.txtcd sub && echo x > out.txt<ws>/sub/out.txtcd sub && … > ../fileis the ordinary way to write beside a subdirectory rather than in it, so this was friction with no workaround but spelling the target absolutely — and the refusal's only feedback.The bug is one sentence in
_check_sandbox:base = workdir_real if workdir_real else cwd_real._cwd_left_workspacealready answers "did the command move the shell out of the workspace"; nothing answered "did it move the shell inside it".What changed
_cwd_at_write_site(cmd, workspace, token)— the mirror of_cwd_left_workspace, reading the same statement/token vocabulary — names the directory in effect at the statement that writestoken. The caller uses it as the join base only; the allowed roots stay the directory the child starts in, because<ws>/back.txtis not "inside<ws>/sub" (substituting both refuses two of the three rows above — measured, mutation arm E).Each rule that makes the reading provable is a bail-out back to the start directory, and each one is there because removing it allows a command whose file really lands outside (one mutation arm per rule, all against the product file with a byte-verified restore, sha256[:16]
46d3e0161f47d0c9before and after):cd sub && echo ../back.txt && cd .. && echo x > ../back.txt→<parent>/back.txt&&chain may carry the move to the site (;/||run the next statement even if thecdfailed)cd nosuchdir; echo x > ../escape.txt→ outside (the||spelling too)cdmay be in a shell of its own)cd sub && (cd .. && echo x > ../back.txt)→ outsidecd -, a barecd,$UNSET; the leaving case is_cwd_left_workspace'sMeasured
Sweep — no hole opened. 420 generated shapes (14 move forms × 6 targets × 5 successors, including the failing-
cd,;,\|\|,\|,( ),env -C, chained-move and post-move forms), each run for real in a scratch tree with the same shape, the predicted join base compared against the file's real location with both sidesrealpath-normalised:cd sub; …family (below)End-to-end, guard and shell side by side (
/bin/sh, scratch tree, file location read back):cd sub && echo x > ../back.txtcd sub && cd sub2 && echo x > ../../deep.txtcd sub && echo x > ../../worse.txtecho x > ../a.txt && cd sub(the write is before the move)(cd sub) && echo x > ../back.txtcd sub | cat && echo x > ../back.txtcd sub > ../f.txt(a redirect on thecditself is set up first)Residuals, pinned with their ground truth rather than left to prose — two shapes that the shell really did write inside and that stay refused, because the token stream cannot tell them from the escape of the same spelling (
cd nosuchdir; …is the same text):cd sub; echo x > ../back.txt(a;chain does not prove thecdworked)cd sub && (echo x > ../back.txt)(the price of the grouping bail-out; its mirror,cd sub && (cd .. && …), is a real escape)Tests —
tests/test_bash_tool_sandbox_cwd.py, where #1244/#1353's rows already live, extended with the inside-move family: the three false blocks, the escapes that must stay refused, the parity pairs (relative spelling vs the measured absolute landing place), the six unprovable shapes, the two residuals, and the walker itself apart from its caller. Mutation arms A–E each redden the rows they belong to; the file is byte-identical afterwards.Full suite on this branch: 3159 passed, 17 skipped (
4f8639f2in the same geometry: 3150/17). Import andpython -m emrg --helpgreen;check-doc-count.pyandcheck-rant-citations.pyclean.DEVELOPMENT.md's "A relative target is resolved, not assumed" now says what the code does.No test here starts, stops or restarts a daemon, and none reaches the real upgrade chain. Landing order: after #1376 — the plan-suite materialises its tree under the temp root, so the write-root-dependent rows need that pin in first (
check-merge-plan-suite.py 1376 1373 … <this>).