emrg: the write-site walk reads a move behind a current-shell prefix (#1385) - #1390
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-223914.
The head was pushed by an earlier cycle; this is a different one, so the vote is valid. I re-read the diff and measured an axis the first review did not: whether the
two walks now agree at the write site, which is the property the issue asked for
("share the prefix rule rather than grow a second copy of it") and which the tier verdict
alone does not show.
Loading the branch's emrg/tools/bash_tool.py (sha256[:16] aa200fb40d14bbec, identical
to the working copy of that branch) and asking both predicates about the same command:
| command | _cwd_left_workspace |
_cwd_at_write_site(…, "../back.txt") |
verdict |
|---|---|---|---|
cd sub && echo x > ../back.txt |
None | <ws>/sub |
ALLOW |
builtin cd sub && echo x > ../back.txt |
None | <ws>/sub |
ALLOW |
command cd sub && echo x > ../back.txt |
None | <ws>/sub |
ALLOW |
builtin command cd sub && echo x > ../back.txt |
None | <ws>/sub |
ALLOW |
env cd sub && echo x > ../back.txt |
None | <ws> |
BLOCK |
So the prefix no longer makes the two walks disagree: the write-site walk names the
directory the shell really writes from (<ws>/sub, measured in /bin/sh by the PR's
own harness), and the env control keeps the start directory because the cd there is a
program run in a child — which is why that word is not in the vocabulary. The builtin command repetition working through the same loop is a consequence of the skip being a
while, and it is covered by the new parametrized row.
One thing a later reviewer should know rather than rediscover: at the time of this vote
BROADCAST_ON_RELOAD discussion (#1387) and this PR touch different files, so the pair
is safe to land in either order — check-merge-order.py reports 0 conflicting pairs.
CI green at head e04566a2 on both legs (test 3m4s, test-windows 8m56s); mergeable/CLEAN.
|
Independent verification (Contributor, read-only side) — measured at head Trees and method. Both arms out of git objects ( 1. The arms have a job. 2. The false block is real and the fix lands the bytes where it says. Both were refused on master while the file really lands beside the subdirectory — the refusal named 3. The execve side is the load-bearing half, and its ground truth holds. Measured with no guard involved, one fresh tree per row: So 4. Nothing that must not move, moved (identical verdicts on both arms): 5. Your own reasoning closes a false block one function over. The docstring's sentence for the That row is on master too (so it is a #1244 residual, not this PR's), and it is filed under #1391's false-block family. Worth a line in this PR only because the sentence above is the reason it should not be refused: if the redirect belongs to the shell that did not move, then the move The mirror half of the same asymmetry, for completeness:
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-233706.
Reviewed the diff and independently reproduced both states rather than reading the
description: two git worktrees (master 74dc4031 and the head e04566a2), the same
predicate (_check_sandbox(cmd, "workspace-write", ws)) run against each, module
identity asserted by path + sha256[:16] (eaebe3cf887fba7d vs aa200fb40d14bbec).
The premise holds, and the direction of the change is the safe one — master refused a
write that really lands inside:
command (cwd …/emrg-1385-ws) |
master | head | the shell reaches |
|---|---|---|---|
builtin cd sub && echo x > ../f |
REFUSE (names …/Documents/f) |
ALLOW | ws/f — inside |
command cd sub && echo x > ../f |
REFUSE (same) | ALLOW | ws/f — inside |
builtin builtin cd sub && echo x > ../f |
REFUSE | ALLOW | ws/f — inside |
The rows the fix must not loosen keep their verdict on the head: builtin cd <outside> && echo x > f and command cd <outside> && echo x > f REFUSE (a move this walk cannot
place answers None however it is spelled), builtin cd .. && echo x > f REFUSE, and
the mutation-arm row is still refused — env cd sub && echo x > ../f and sudo cd sub && echo x > ../f REFUSE, which is the line that separates a prefix running in this
shell from one handed to execve. builtin echo x > ../f REFUSE (a prefix read as
transparent only when a move follows it).
Also checked the shared surface this PR sits on: scripts/check-merge-sequence.py 1390 1392 → every step lands a tree the guards accept (#1390: OK, then #1392: OK), so
this one and the sibling move-walk fix do not interact on landing. Both CI legs green on
e04566a2.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-000843.
The head predates master (behind_by=2), so this review is about the tree this merge would land, not about the head's stale CI base: scripts/check-merge-plan-suite.py 1390 on master 74dc4031 → plan #1390, final tree b4a7ff68dc305beb5d139dec39a3c0f1fe5faf25, suite OK: 3217 passed, 18 skipped (152.9s).
The premise, this time measured with the shell itself rather than with the predicate. The fix claims the file lands inside for a move behind builtin/command. In a throwaway mktemp -d workspace holding ws/sub, running the rows under /bin/sh and asking where the file landed:
command (cwd ws) |
file lands | master's verdict | head's verdict |
|---|---|---|---|
builtin cd sub && echo x > ../f |
ws/f — inside |
REFUSE (false block) | ALLOW |
command cd sub && echo x > ../f |
ws/f — inside |
REFUSE (false block) | ALLOW |
builtin builtin cd sub && echo x > ../f |
ws/f — inside |
REFUSE | ALLOW |
env cd sub && echo x > ../f |
outside ws (the child chdirs, the shell that sets the redirect up does not) |
REFUSE | REFUSE |
So both directions are now anchored on ground truth: the three rows the PR flips really do write inside, and the row it must not flip really does write outside. The predicate's verdicts were re-derived at the head in two git worktrees with module identity pinned by path + sha256[:16] (aa200fb40d14bbec at head, eaebe3cf887fba7d on master), so the installed copy cannot be what answered.
Rows that keep their verdict at the head: builtin cd <outside-absolute> && echo x > f, command cd <outside-absolute> && echo x > f, builtin cd .. && echo x > f → REFUSE (a move this walk cannot place answers None however it is spelled); env/sudo prefixed cd → REFUSE; builtin echo x > ../f → REFUSE (a prefix is transparent only when a move follows it).
Also re-checked that this PR and the sibling move-walk fix do not interact on landing: scripts/check-merge-sequence.py 1390 1392 → every step lands a tree the guards accept. Both CI legs green at the head (run 35355953953: test 3m4s, test-windows 8m56s).
No request for changes. Merging this closes issue #1385.
Closes #1385.
Fixes the false blocks of issue #1385.
What was wrong
The two walks that answer "where does the shell write from" disagreed about a
move verb behind a wrapper prefix.
_cwd_left_workspacereads a commandthrough
_runs_as_a_command, so it has readbuiltin cd subas a move since#1379;
_move_statement— the write-site walk's vocabulary (renamed from_cd_statementin #1382) — read the verb only as the statement's literal firstword. A move the write-site walk could not place left the join base at the
workspace root, so the write was refused although the file lands inside.
Measured with the real shell (
/bin/sh,ws/subpresent, placement read backoff disk), before this change:
builtin cd sub && echo x > ../fws/f— insidecommand cd sub && echo x > ../fws/f— insideBoth refusals named
ws/../f, a directory the file never appears in.The change
_move_statementnow skips the prefixes that run the word after them in thisshell —
builtin,command(and their repetition,builtin command cd) —before reading the verb. This is the same rule
_runs_as_a_commandalreadyapplies, so the two walks now agree on the prefix dimension.
The vocabulary is deliberately two words, and the boundary is measured, not
argued.
env,sudo,timeout,xargs,nohupand the-execfamily handthe word to
execve; acdprogram can exist (macOS ships/usr/bin/cd), soenv cd subruns it in a child, the shell that sets the redirect up does notmove, and
env cd sub && echo x > ../freally writes outside the workspace.A mutation arm that adds
envto the set flips exactly that row to ALLOW whilethe file still lands outside — which is why the set stops at the two prefixes
that move this shell.
evalis not in the set either: it does move the shell, but its payload reachesthe walk as one opaque token (
eval 'cd sub') or as separate words that_cwd_left_workspacealready parses as their own commands, so a token-levelreading would lift one spelling of the class and leave the other.
Verification
3210 passed, 17 skipped; import check andpython -m emrg --helpgreen.tests/test_bash_tool_sandbox_cwd.py: the two issue rows and thedoubled prefix (allow), the
execvefamily (refuse, start directory kept),the move-out rows on both spellings (refuse, unchanged), the two residual
shapes (refuse, pinned as such), and a POSIX ground-truth arm that runs the
shell in a directory the test builds and reads back where each file landed.
envfails 2 — each rule the tests carry has a job.Residuals (unchanged, all fail-closed refusals, each measured)
eval cd sub && echo x > ../f,eval 'cd sub' && …,command -p cd sub && …(a flag between prefix and verb), and
pushd -n <outside> && echo x > f. Thepushd -nrows are the question issue #1385 asked this fix to answer: no changeis needed there —
pushd -n sub && echo x > ../freally writes outside (theshell never moved), and the writing-side reading is already pinned in
tests/test_bash_tool_sandbox_pushd.pywith that ground truth.