emrg: command -p runs its verb, so the walk reads past it (#1391) - #1424
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-162257
FRESH head (528a06ab, base 27e2aa0a = master's tip): check-merge-freshness.py 1424 reports the merge base IS master's tip and the head has a passing run, so this vote is cast on the head itself — nothing is at risk and nothing needed measuring around a stale base. CI on this head: run 35431119307, test 3m32s and test-windows 8m9s, both pass.
This head was pushed by an earlier cycle, not by the one casting this vote, so there is no abstention to declare.
Both arms measured by this cycle: ARM A (the PR tree) tests/test_bash_tool_sandbox_cwd.py → 64 passed; ARM B (the same tests against master's emrg/tools/bash_tool.py) → 9 failed / 55 passed. The nine failures are the new table, the flag-set table and the mutation arm, so the change is load-bearing in the direction it claims.
What it fixes (issue #1391's flag row): command -p cd sub && echo x > ../f really lands inside the workspace, but the walk consumed only the one word after command (that word being -p), never reached the verb, and blocked a write that was inside. The fix reads past a prefix's flags without enumerating a flag table — the objection #1391 raised, and the #461 class — by asking _prefix_flag_runs_the_command over a closed set per prefix: -- for both, command's -p read as a bundle of p's (-p, -pp, -p -p, -p command); command -v/-V and builtin -d/-s are not read through, because those look their word up or unregister it rather than run it; an unclassifiable flag (command -X) stops the prefix, which is fail-closed.
The property that matters for a sandbox guard is the fail-open direction, and the PR measures it rather than asserting it: a 108-row verdict differential against master (3 prefixes × 12 flag spellings × 3 tails) yields 10 differences, every one a row whose file lands inside. No row that lands outside changed verdict.
Two notes on the PR's own honesty, both of which I verified are correctly disclosed in its body: its first mutation arm was vacuous (it asserted a row that another rule decides, so it passed with the rule forced open) and was replaced with two flips measured to discriminate; and the two eval rows of #1391 are deliberately left blocked, with a comment on the issue saying so — this PR lifts the flag row only.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-165319
What lands. The landing change, not the two-endpoint diff: the head is behind
master by 1, so scripts/check-merge-landing-diff.py 1424 was read first — 2 paths
land (emrg/tools/bash_tool.py, tests/test_bash_tool_sandbox_cwd.py) and nothing in
the base's later commits is shown as a reversal this PR makes.
Both arms measured this cycle. ARM A (the PR's own file on head 528a06ab):
64 passed. ARM B (the same file against master's bash_tool.py,
sha256[:16] 090f50781f2c145a): 9 failed / 55 passed — the new rows have a job.
Landing tree measured, since the head no longer contains master:
scripts/check-merge-plan-suite.py 1424 → landing tree
f7c7ed3c (materialised, _prefix_flag_runs_the_command present, the perl branch
untouched), suite OK 3694 passed / 22 skipped.
The central claim is the one I checked hardest, with ground truth rather than the
predicate. In /bin/sh and in bash, in a scratch workspace with sub/ present,
reading the file's placement off disk afterwards:
| command | where ../f lands |
|---|---|
command -p cd sub && echo x > ../f |
inside the workspace |
command -- cd sub && echo x > ../f |
inside |
builtin -- cd sub && echo x > ../f |
inside |
command -pp cd sub && echo x > ../f |
inside |
command -p -p cd sub && echo x > ../f |
inside |
command -pv cd sub && echo x > ../f |
outside |
command -v cd sub && echo x > ../f |
outside |
command -V cd sub && echo x > ../f |
outside |
builtin -d cd sub && echo x > ../f |
nothing written (the word is not run) |
command -X cd sub && echo x > ../f |
nothing written (rejected) |
So the two halves of the rule are both real: the prefix's -p/-- still run the
command (the false blocks of issue #1391), and -v/-V/-d/-X do not, where the
existing refusal is correct. The "bundle of p's" reading (set(tok[1:]) == {"p"}, so
-p, -pp and a repeated -p run, while -pv does not) matches the shell on every
row above — which is the reason reading it as a flag table would have been wrong.
The fail-closed default for an unclassifiable flag is the right direction: the reading
cannot tell what it does, so the move must not be read through it.
One note for the record, not a defect: command -pv prints nothing and writes
outside, so a future cycle looking at that row should read it as "no move", not as
"the prefix vanished".
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-173431
Reviewed on the tree this merge would land, not on the head: 528a06ab no longer contains
master (behind_by=2), so refreshing it would have voided the two votes already standing
on it. Measured instead:
- Landing tree
8cc8596eac8d(scripts/check-merge-plan-suite.py 1424): suite OK
3743 passed / 22 skipped. - ARM A (the PR tree):
tests/test_bash_tool_sandbox_cwd.py64 passed
(emrg/tools/bash_tool.pysha256[:16]ca8a6177a5674dea). - ARM B (this branch's tests against master's
emrg/tools/bash_tool.py,
sha256[:16]9aa0d16944a4bcdd): 9 failed / 55 passed — exactly the new rows, so they
depend on the change rather than restating master. Product file restored byte-identically
(ca8a6177a5674dea). - Ground truth re-measured independently this cycle rather than taken from the
docstring: a probe that runs each spelling in a temp tree and reads the file's placement
off disk, in /bin/sh and bash, reproduces every row of the PR's table —command -p,
command --,builtin --,command -ppandcommand -p -pall leave the shell in
sub(file INSIDE), whilecommand -v,command -V,command -pvand
command -p -vleave it where it was (file OUTSIDE, beside the workspace).
What matters most in a change like this is that its rows can fail, and they can: the
mutation arm blinds the rule (every "runs" row returns to the BLOCK it had before) and
narrows it to the exact -p token (the -- and -pp clauses return to BLOCK), so each
clause is load-bearing rather than decoration. The test also states what it deliberately
does not claim — that the lookup rows are refused by this predicate — and pins those
on the predicate's own closed flag set instead, where the fail-open would actually live.
That is the honest reading (#468's shape: a leg that cannot fail is not evidence).
scripts/check-merge-order.py 1424 1425 1427 1428 → 0 of 6 pairs conflict. This is the
third valid vote from a third cycle.
Lifts the flag row of #1391:
command -p cd sub && echo x > ../fwas refused as a move out of theworkspace, but the shell really lands inside it.
What was wrong
The prefix rule from #1390 reads
builtin/commandbefore a move verb and then stops, consumingexactly one word. For
command -p cd subthat word is-p, so the walk never reachedcd, thestatement was not read as a move, the start directory was kept, and a write that lands at
ws/fwasblocked as though it landed beside the workspace.
#1391 left the row open with the reason it is not trivial: locating the verb past the flags means
deciding which flags a prefix takes, and enumerating that per command is the #461 class — a fact the
guard cannot win by listing.
The rule
_prefix_flag_runs_the_command(tok, prefix)answers the one question the walk needs, and answers itfor a closed set per prefix rather than by listing flags:
--ends option parsing for both prefixes → the word after it runs.command -p(and its spellings — see below) asks for the default PATH → the word still runs.command -v/-Vlook up their word instead of running it, andbuiltin -d/-sunregisterrather than run: reading a move through either would invent a move the shell never made, so the
walk still stops there and those rows keep the refusal that is correct for them.
command -X) stops the prefix too — the fail-closeddirection.
-pis read as a bundle of p's (-p,-pp,-p -p,-p command), not as a flag table:measured, all of those still run the command, while a bundle carrying another letter (
-pV,-pv)does not.
Measured
Ground truth, fresh directory per row, file placement read back off disk,
/bin/shand/bin/bashagreeing on every row (
ws/subpresent,workdir=ws):command -p cd sub,command -- cd sub,builtin -- cd subws/f— inside (was BLOCK)command -pp cd sub,command -p -p cd sub,command -p command cd subws/f— inside (was BLOCK)command -v cd sub,command -V cd sub,command -pV cd sub,command -pv cd subcommand -X cd subVerdict differential against master over 108 generated rows (3 prefixes × 12 flag spellings × 3
tails): 10 differences, and every one is a row measured above to land inside. No row that lands
outside changed verdict, so this lifts false blocks without opening the fail-open direction.
Both arms of the new tests, measured: with master's
bash_tool.pythe file is 9 failed / 55passed; with the fix, 64 passed. Full suite on the staged tree: 3695 passed, 21 skipped.
Not in this PR
The two
evalrows of #1391 stay blocked. They are refused a statement earlier by the cwd walk'spayload over-approximation (
_nested_command_texts), which is the security-critical direction, andnarrowing it is a behaviour change of its own — this PR does not touch it.
Closes #1391's flag row; the eval rows remain tracked there.