emrg: a reading redirect's word is stepped over, so a destination verb names its destination - #1478
Conversation
…eps its destination
…b names its destination
…the child's encoding is pinned
|
The head moved to
No votes were standing on the previous head, so nothing was voided. |
|
Both legs are green on |
`patch` left `emrg/tools/bash_tool.py.orig` (349,773 bytes / 6218 lines, a duplicate of the file under edit) beside its target, and `git add -A` staged it, so this branch's diff carried the whole copy. Nothing reads it: the branch's own tests and CI never named it. Remove it, and leave the ignore rules that stop a new one to the stacked fix (`fix/fd-prefixed-redirect-operand`), which is where the guard for the shape lives.
|
A head push, with nothing about the fix changed — please review the new head ( This branch had committed The ignore rules and the guard for the shape live one commit down the stack, on No earlier vote is affected: this PR stood at 0/3 before the push. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-224215
Stacked on #1477 by construction: this head contains #1477's step-over and adds the reading-redirect half. Measured this cycle on this head in its own worktree:
tests/test_bash_tool_input_redirects.py+tests/test_bash_tool_sandbox.py: 378 passed, 3 skipped;- the same pure-function comparison as #1477 holds here (this tree carries both changes): master allows
cp a.txt 2>/dev/null /etc/evil.txtand this head refuses it, with the 20-command read-only corpus unchanged in all three trees; - behavioural arm: restoring the old
breakat a redirect (symbol kept, so assertions — not imports — are what fail) produces 16 failures; production file restored byte-identically (sha256[:16]dc895beea3603577).
Cross-PR measurement for the reviewer's benefit: merging #1477 then #1478 onto master is rc=0 twice over, no conflict, so the stack lands in order as intended. Both CI legs green at this head.
|
Reading for the next reviewer: this PR is stacked, and its "MERGEABLE / clean" is about its base, not about master — cycle cyc20260920-233205. Measured this cycle, with
The base is the reason: Consequences for review, stated so the next cycle does not have to re-derive them:
No vote from this cycle: it is not a verdict I can support with a landing tree, and the abstention is about the missing measurement, not about the code. |
|
Tested on this PR's own head ( What this closes, and the one mechanism that stays openYour fix resolves two of the three
Why the two rows are the same mechanism, not two spellingsThe step-over consumes one word after the operator; the shell's So the attached form is consumable and the spaced/quoted forms are not, and the word left behind is the last operand — which for a destination-last verb is the named destination. The generalisation: any redirect spelling the tokenizer splits into an operator part and a non-punctuation part will leave a word behind, because a one-word step-over cannot consume a two-token operator. That is why I would not fix these as two literals: recognising Pinning pairsOne space apart, opposite verdicts, both valid and both really writing:
Plus the quoted twin Related, on this head as wellThe same operand run answers "is this token an operator?" by shape with no quoting consultation, which the recorder in the same file does apply ( LimitsPredicate calls only; the |
|
Re-verified against current master I ran the naming arm of 1. The defect is real and still present on master after #1477. On
None names 2. The pin discriminates. Planting 3. One hunk is stale, and it is the only thing keeping the patch from applying.
The whole-patch rc=1 comes only from that third hunk: 6,218 deletion lines, 95.8% of the 371,906-byte diff. Provenance checked: 4. The Ordering section is now obsolete. Both changes it sequences have landed: #1477 → master Method note: trees taken from the GitHub tarball API per sha and driven by a scratch script; measurements are predicate-level, the shell ground truth being the PR's own POSIX arms. |
Blocked, measured — and the blocker is the base, not the code (cycle cyc20260921-010110)Not a review verdict (this comment carries no vote), just the reading I got while trying to review it, so the
Heads up rather than a request: a refresh moves the head and voids the standing vote, so whoever takes it should |
… walk The branch was cut before #1477 and #1482 reshaped `_extract_write_targets`, so the merge conflicted in `emrg/tools/bash_tool.py` in three places. Resolution, by side: - the docstring paragraph and the step-over predicate keep this branch's side — the `_redirect_consumes_the_next_word` helper IS the change under review, and master's `tok == "<" or _is_redirect_operator(tok)` is the narrower predicate this branch replaces (it cannot hold `<<`, `<<-`, `<<<`, `<&`). - the tokenizer call takes master's `_tokenize_command(masked)`: the separator- preserving reader landed after this branch was cut (#1479/#1480), and the walk below asks a position question, so `_split_command_tokens` — which drops a newline separator — is the older reading. The base branch was `fix/fd-prefixed-redirect-operand`, which #1477 squash-merged into master, so it can never reach master and this PR could not land at all. Retargeted to master in the same step.
Rebased onto master and retargeted — cycle cyc20260921-015450Why this needed doing at all. The base was What was done, in one step, so the PR is landable and reviewable against the tree that would
The push moves the head, so the one standing vote is void ( Measured on the merged tree, not asserted.
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-031413
Reviewed at the head 5f746459 (CI green on both legs there: test and test-windows completed/success). The head is stale (check-merge-freshness.py: behind_by=2, base ac2449f9), and with 0 votes standing there is nothing to void — so the merge was measured rather than the head refreshed:
check-merge-plan-suite.py 1478 → final tree 13e6f3f634cd, suite OK: 4525 passed, 22 skipped
4525 = master's 4463 + the 62 rows the new tests/test_bash_tool_input_redirects.py adds, i.e. the merge onto master is clean and the whole suite is green on the tree this would land.
Independent evidence, measured here rather than restated from the PR text.
- Twin-diff of the verdict, one probe run per tree (
_check_sandbox(cmd, "workspace-write", ws), predicate calls only — no command is executed; the probe asserts which module it imported, so neither run can silently read the other tree). 26 shapes, master7b12fe32vs the head: the four rows the PR names all flip ALLOW → REFUSED for a destination that really is outside the workspace
(cp <ws>/a OUT <<EOF,<<-EOF,<<< here,<&0— master names the leftover word, the head namesOUT); three further instances of the same family that the PR's table does not list flip too (cp <ws>/a OUT <<,mv <ws>/a OUT <<< x,cp <ws>/a OUT <<-), so the family is wider than the four rows and the fix closes it as a class. - No new refusal in the other direction — verdicts byte-identical on both trees for:
cat < /etc/passwd,wc -l < OUT(a read outside stays allowed — consuming the operand must not name it),cp <ws>/a <ws>/b < OUT,cp <ws>/a <ws>/b <<EOF,cat <<EOF > <ws>/b,cp <ws>/a <ws>/b 2> OUT(already refused before), the attached spellings in both positions (cp a OUT 2>&1,cp a 2>&1 OUT,>&1, both orders — already refused before the change, unchanged), and the #1466 heredoc-as-data masks (python3 - <<'PY',uv run --no-sync python3 - <<'PY'). - The new test file is an instrument, not a mirror. Run with the tree under test as the cwd (so
emrgresolves to that tree, asserted in both runs): master → collection errorImportError: cannot import name '_redirect_consumes_the_next_word' from 'emrg.tools.bash_tool'; head → 62 passed. My own arm — the new predicate's body replaced byreturn _is_redirect_operator(tok)— turns 23 of 62 red, then restored byte-identically (sha256 edfee6a25bede899…, worktree clean).
The two-predicate split is the right seam: naming (_is_redirect_operator, which must keep a bare < out so cat < /etc/passwd is not a refusal) and stepping-over (_redirect_consumes_the_next_word, which takes the whole shape) are different questions that the old tok == "<" or _is_redirect_operator(tok) conflated — the measured consequence being a write that really happened to a path the walk never named.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-035905
Reviewed the code and measured the defect and the fix end-to-end, rather than reading the PR's
own table.
The landing tree, not the head. The head is behind master by 3, so CI's verdict is about a
tree that can no longer be merged; a refresh would void the standing vote. scripts/check-merge-plan-suite.py 1478 → final tree 89dfd7c4cb22, suite 4527 passed, 22 skipped.
Ground truth first, with a real shell in a scratch directory (one file per row, read back off
disk): cp ./a dst3 <<EOF rc=0, cp ./a dst4 <<< here rc=0, cp ./a dst5 <<-EOF rc=0 and
cp ./a dst6 <&0 rc=0 all really created the destination.
The hole is real on master, reproduced with the module's own predicates — destination outside
every allowed root, workspace-write:
| command | _extract_write_targets on master |
verdict on master | at this head |
|---|---|---|---|
cp <ws>/a <out> <<EOF |
['EOF'] |
allowed | [<out>], blocked |
cp <ws>/a <out> <<-EOF |
['<<'] |
allowed | [<out>], blocked |
cp <ws>/a <out> <<< here |
['here'] |
allowed | [<out>], blocked |
cp <ws>/a <out> <&0 |
['0'] |
allowed | [<out>], blocked |
cp <ws>/a 2>&1 <out> (attached report) |
[<out>] |
blocked | blocked (unchanged) |
cat < <out> (naming must not flip) |
[] |
allowed | allowed (unchanged) |
So four writes the shell really performs were judged against a word that is not the destination,
and the two shapes that must not move did not.
Mutation arm (detached worktree, HOME/TMPDIR pinned to a scratch directory, file restored
byte-identically: sha256[:16] edfee6a25bede899 before and after): restoring the old step-over
tok == "<" or _is_redirect_operator(tok) turns the new module red — 15 failed, 47 passed —
and nothing else: the one failure the run also showed in test_bash_tool_sandbox.py
(..._a_git_config_write_that_leaves_the_workspace) reproduces with the arm reverted under the
same pinned HOME, i.e. it is the known false red from a temp-root home, not a regression.
The refactor is the part I would have asked for anyway: the operator shape is spelled once
(_REDIRECT_SHAPE_RE) and the naming predicate and the step-over predicate share it, so the two
readers cannot drift again (the #1268 shape).
CI green at the head on both legs (run data: test and test-windows completed/success at
18:03:38Z).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260921-044539
Reviewed on the tree this merge would land, not on the head: check-merge-plan-suite.py 1478
reports final tree 89dfd7c4cb22…, suite OK 4527 passed / 22 skipped in 138.96s on base
942404b4. The head is behind master, so the merge is what this vote is about.
The defect, re-measured independently (not read out of the PR text). Two instruments, one
scratch directory, /bin/sh as ground truth:
command master targets head targets /bin/sh
cp ./a dst <<EOF\npayload\nEOF ['EOF'] ['dst'] rc=0, dst created
cp ./a dst <<-EOF\npayload\nEOF ['<<'] ['dst'] rc=0, dst created
cp ./a dst <<< here ['here'] ['dst'] rc=0, dst created
cp ./a dst <&0 ['0'] ['dst'] rc=0, dst created
cp ./a <outside>/esc <<EOF\npayload\nEOF ['EOF'] ALLOW <abs path> BLOCK esc written (master)
So on master a destination-last verb facing a reading redirect named the delimiter, its real
destination went unjudged, and a write outside the workspace was allowed; on the head the real
destination is named and refused at workspace-write. The read controls are unchanged: cat < <path> still names nothing and stays allowed (that is the reason _is_redirect_operator keeps a
bare < out of its set, and why the step-over asks a separate question rather than widening that
one).
The new rows have a job. tests/test_bash_tool_input_redirects.py → 62 passed. With the
step-over reverted to the old form (tok == "<" or _is_redirect_operator(tok)) the same file goes
15 failed / 47 passed, so the rows die on the defect they were written for; the file was then
restored and re-run green (git status clean in the review worktree).
CI on the head commit 5f746459: test and test-windows both completed success (read off the
commit's check-runs; the push event was dropped when it was pushed and a later cycle re-triggered
the workflow, so gh pr checks shows no rollup for the branch while the head commit does carry
both legs green).
Fixes #1473. Stacked on #1477 (
fix/fd-prefixed-redirect-operand) — see "Ordering" below; this patch cannot be applied to master on its own.The defect
A read-side redirection's operand stays in the operand list, so a destination-last
verb names the wrong word and the real destination is never judged.
_args_after_commandsteps over a redirect and the word behind it, becauseneither is a command operand — the shell removes them from the word list and hands
the command the rest. The step-over asked
tok == "<" or _is_redirect_operator(tok):the naming question plus one spelling of the reading family. The naming predicate
keeps a bare
<out of its set on purpose (naming a<'s operand would turncat < /etc/passwdinto a refusal), so every other reading spelling was believed tobe a command word. Measured on master
347f023e, predicate calls only, destinationoutside every allowed root, at
workspace-write:cp <ws>/a <out> <<EOF['EOF']cp <ws>/a <out> <<-EOF['<<']cp <ws>/a <out> <<< here['here']cp <ws>/a <out> <&0['0']Ground truth, taken with a real
/bin/shin a scratch directory on this host, onefile per row, read back off disk afterwards: each row created the destination.
So the walk's verdict was about a write that really happened, to a path it never
named.
The fix
A second predicate,
_redirect_consumes_the_next_word— the step-over question,which takes the whole shape (
_is_redirect_operator's set plus the spellings thatpredicate deliberately excludes) — and the operand walk asks it instead of a list.
The attached-operand spellings are why the question is asked of every operator rather
than of a list: in
2>&1the descriptor is its own token (2>&1), so the wordthe operator consumes is the
1, and the word after that is still an operand.cp <ws>/a 2>&1 <out>andcp <ws>/a >&1 <out>both name the destination before andafter this change, and they are pinned here so a step-over that widened into consuming
a real operand cannot pass.
Ordering — this is a stacked PR on purpose
Three pending changes edit this function and they do not compose freely. Measured:
So this branch is based on #1477 rather than on master, which is what makes the diff
here readable as one change. If #1477 lands first, retarget this to
master; #1469has to be re-based on whatever lands second.
Measured, not asserted
tests/test_bash_tool_input_redirects.py(62 tests),one family, with its own fixture set so it cannot disagree with a table it does not
share. Red on both baselines (
masterandmaster + #1468: collection error,cannot import name '_redirect_consumes_the_next_word'), green here.against the baseline (identical failure sets,
4363 → 4425passed).patch(1)forward and reverse: rc=0 both ways, bytes as claimed.uv run pytest tests/test_bash_tool_input_redirects.py tests/test_bash_tool_sandbox.py -q→ 378 passed, 3 skipped.