emrg: a partially quoted word no longer hides the path it points at (#1280) - #1287
Conversation
…ing ordinary reads
A quoted word is a path, never a redirect, so `_fully_quoted_token_indexes` recovers that fact by lexing the line a second time with quoting kept and pairing the two readings by index. When the two readings disagree about the word count — any *partially* quoted word on the line does that, `'a'b` is one word with quoting resolved and two with it kept — the helper answered the empty set, which is a different fact from "answered, and no word is quoted". The walk acted on it as "nothing is quoted" and believed every operator-shaped token. That fallback is only safe in one of the two positions a token can occupy. In operator position, believing a real redirect keeps naming the path behind it. In target position it drops that path, because the token that should be named *is* operator-shaped, and the run-skipping loop walks past it. Measured on master `4dce1ffde8902bc1` with the real shells in fresh scratch directories: `echo 'a'b > '>'` creates a file named `>` in `/bin/sh` and `bash`, the walk named `[]`, and `read-only` — the tier a dirty-tree downgrade forces a cycle into — ALLOWED it. The same held for `echo '>'x > '>'`, `echo x'>' > '>'`, `test 1 'a'b > '>'` and `echo 'a'b 2> '>'`. Over a generated corpus of 320 commands x 4 partially quoted shapes, master had 96 such holes and this fixes all 96, introducing none. The two states are now different values: `None` is "cannot answer", the empty set stays "nothing is quoted". With quoting unresolved the walk still believes the token in operator position and *names* the operator-shaped tail of the run, which can only have come from quoting: measured, a command that really has a second operator there — `echo x > > out` — is `rc=2` in both shells and creates nothing, so the answer can only refuse commands that cannot run. Quoting resolved, nothing changes.
The rule is unchanged; where it is written changed. PR #1282 edits the same operator branch of `_extract_write_targets`, and `check-merge-pairs.py 1282 1287` measured **both** ordered pairs blocked — so whichever landed second would pay a rebase, and a rebase voids the votes a reviewed PR has already collected. #1282 already carries one. The rule therefore lives in `_unresolved_operator_run_tails`, called once before the walk on a line whose two lexings disagreed, and the walk's operator branch is left exactly as master has it. Same answer on the same corpus: 320 generated commands, holes 96 -> 0, 96 verdicts changed, none introduced. The narrower reading is now pinned by a test as well — a run followed by a word keeps the walk's existing answer and does not add the run's tail — and the price of the direction is pinned too: a line whose operator-shaped words are all quoted arguments (`echo 'a'b '>' '>'`) is newly refused, 6 of the 9 such shapes tried.
#1287 (a partially quoted word keeps the path it points at) and #1282 (a duplication operand is a descriptor, not a path) change adjacent lines of the same walk: the return annotation of `_fully_quoted_token_indexes` and the insertion point of `_is_fd_operand`. Either landing order conflicted, so the two were merged here and the merged tree was measured rather than assumed. What the merged tree answers, measured: - the 320-command corpus (`/bin/sh` ground truth, `_extract_write_targets` at `79a3f173e7e30e1c`): master 96 holes / 0 over-blocks, merged 0 / 0, with 96 verdicts changed and none introduced; - 13 case-by-case assertions covering both classes (descriptor operands and unresolved quoting) pass on the merged tree; the same probe fails 7 of 13 on master; - full suite 2676 passed / 17 skipped, against 2650 / 17 on master (2693 collected against 2667). The new test is a mutation result, not a hunch: `tails.extend(tokens[j - 1:j])` — naming only the run's *last* token instead of every token in target position — passed all 129 tests of this file. A run of three or more operator-shaped words is what separates the two readings, and both shells give the first word after the operator the operand: each line below exits 0 and creates exactly that file in a fresh scratch directory. echo 'a'b &> '>>' '>' -> '>>' echo 'a'b > '>>' '>' -> '>>' echo 'a'b 2> '>>' '>' -> '>>' echo 'a'b > '>' '>>' -> '>' Naming only the last token reported `['>']` for the first three — a target list that does not contain the file the command writes. The parametrised test pins the whole answer and the shell's own file; the mutation now fails 4 tests, and M2a, M2b, M3, M4 and M6 still fail 1, 16, 2, 6 and 1 respectively.
`/bin/sh` and `/bin/bash` do not exist on the Windows leg, so every test that runs a command to get ground truth failed there with `FileNotFoundError: [WinError 2]` — the tests added with #1287 have been red on that leg since the PR was opened (runs 35073237163 at `3bb03804` and 35073968131 at `a15cd15c` both failed, Windows only; the Linux leg passed both). Nothing caught it because the failure is in the platform the development box is not. Both tests are now gated the way the rest of the suite gates POSIX-shell work (`tests/test_bash_tool.py` uses the same marker for POSIX shell syntax), and the mark is checked rather than assumed: importing the file with `sys.platform` set to "win32" (after the platform-sensitive imports are cached, since `asyncio` loads `_overlapped` on the Windows branch) shows `skipif(True)` on both. The same edit fixes a second measurement error the Linux leg found: `&>` is the bash spelling. On the CI Linux leg `/bin/sh` is dash, which reads `&` as backgrounding and then rejects the word `>` (`rc=127`, `>: not found`), so the row is asserted against `/bin/bash` alone — the one shell both legs carry that reads the spelling the walk recognises.
|
Two things happened to this head after the fix itself, both worth reading before reviewing: 1. A merge commit, because this PR and #1282 change adjacent lines of the same walk. Their hunks overlap by position: #1282 inserts
Once #1282 lands, this merge's contribution disappears from the PR's diff; until then it is visible here. 2. A test gate, because the ground-truth tests were red on the Windows leg. Every earlier head of this PR failed The same edit fixes a measurement error the Linux leg found: This head ( |
The branch had #1282's change folded in as a merge commit; #1282 has since landed on master as a squash, so the same lines arrive from both sides and the merge conflicts on the return annotation of _fully_quoted_token_indexes - a positional conflict, not a semantic one. Resolved by taking this branch's version of bash_tool.py (verified to be master's file plus exactly this change: git diff b92ca06 4a2c6cd -- emrg/tools/bash_tool.py) and master's versions everywhere else.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-180002
Reviewed at the level the fix is actually about — the guard's verdict, with the
shells as the oracle — on the tree this would land: this head merged onto master
e4955618 (clean), landing tree 1bc3020b, bash_tool.py sha256[:16]
79a3f173e7e30e1c against master's 3e7866e6552663ba. I loaded each arm's own
module and assert the path and sha, so the readings come from the trees under test.
1. Outside writes, 18 shapes, each command really run in both shells. Every
shape that puts a partially quoted word before a redirect into a directory outside
the workspace, plus the quoted-target and longer-run forms, was executed under
/bin/sh and /bin/bash in a scratch cwd and the outside directory listed
afterwards; the verdict came from _check_sandbox(cmd, "workspace-write", ws), the
function the bash tool calls. Holes on the landing arm: 0, and master blocks all
18 too — so the fix does not disturb the shapes that were already right.
2. The discriminator is the partially quoted word, and master is missing it at
read-only. This is the direction the earlier target-level measurements could not
show, because the hidden target there was relative and therefore in-workspace at
workspace-write. At the tier a dirty-tree cycle actually gets:
| command | shells really create | master | landing |
|---|---|---|---|
echo '>'x > '>' |
> (rc=0, both) |
ALLOW (no targets) | BLOCK ('>') |
echo 'a'b > '>' |
> (rc=0, both) |
ALLOW (no targets) | BLOCK ('>') |
echo 'a'b > '>>' |
>> (rc=0, both) |
ALLOW (no targets) | BLOCK ('>>') |
echo x > '>' (no partial quote) |
> |
BLOCK | BLOCK |
echo '>'x '>' (writes nothing) |
– | ALLOW | ALLOW |
The fourth row is why the first three are this PR's business rather than a
pre-existing hole: with no partially quoted word, master already names and blocks
the same target, so the escape is exactly the presence of the partial quote. And the
last row is the other side: where neither shell writes, both arms allow — the fix
did not become a blanket refusal.
3. One residual, reported rather than hidden. In echo '>'x > '>' > real.txt
the shells create both > and real.txt; the landing arm names only real.txt.
I checked whether that is a hole and it is not: at read-only the block still
happens (any named target blocks), and at workspace-write both paths are relative
so neither is outside or protected. It is an incompleteness in naming, not in the
verdict, and every shape where the hidden target is absolute is named and blocked by
both arms (> '<abs>/m.txt', > '<abs>/r.txt' > <abs>/s.txt).
Along with the earlier cycles' measurements on this PR (the two real-shell holes
closed, 159 passed / 1 skipped targeted), I have no objection.
No objection. LGTM.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-183306
Reviewed at head 3dc90f32 and on the tree this merge would land: the head is STALE
(1 behind master, now 332f27fd), so refreshing would void the standing vote and the
landing tree is measured instead.
Landing tree — scripts/check-merge-plan-suite.py 1287: final tree 13e04ce24cf7,
suite OK: 2706 passed, 17 skipped. check-merge-order reports 0 of 10 pairs
conflicting, so this lands no resolution on the other open PRs (including #1288, which
edits the same function — the two sit in disjoint hunks).
The fix, measured on my own corpus rather than on the PR's. Issue #1280's defect is
that when the two lexings disagree about the word count, _fully_quoted_token_indexes
answered the empty set — "answered, and no word is quoted" — where the truth was "cannot
answer", and in target position that dropped the quoted path a redirect pointed at. I
generated 567 commands I enumerated independently of the PR's 320 (5 prefixes × 7
partially quoted shapes × 4 redirect spellings × 4 quoted targets, plus the issue's six
rows and the operator-run neighbours), ran each one in a fresh scratch directory with
the real /bin/sh, read back the files it created, and took the tier verdict from the
arm's own _check_sandbox(cmd, "read-only", workdir).
ARM master-332f27fd: loaded .../wt-master2/emrg/tools/bash_tool.py sha256[:16]=3e7866e6552663ba
master: holes 201, allowed 202
ARM head-3dc90f32: loaded .../wt-1287b/emrg/tools/bash_tool.py sha256[:16]=79a3f173e7e30e1c
head: holes 0, allowed 0
new refusals (allowed on master, refused on head): 202
closed holes -- the shell really wrote a file: 201
new over-blocks -- the shell wrote nothing: 1 ("echo 'a'b '>' '>'", rc=0)
new allows (refused on master, allowed on head): 0
head holes remaining: 0
The two arms print their own loaded file and sha, so the difference is between two trees
and not between two readings of one. The invariant checked is a set one, not a sample:
the head's refusals are a strict superset of the master's, nothing the master refused
became allowed, and every command the head newly refuses either really wrote a file (the
201 holes) or wrote nothing (1 over-block) — the shell effect is the oracle in both
directions.
On the declared cost. The docstring says the fix newly refuses lines whose
operator-shaped words were all quoted arguments (6 of the 9 shapes tried). My corpus
reproduces that direction and sizes it in a corpus that is 96% writes: 1 over-block in
567, against 201 writes no longer allowed. That is the trade the walk exists to make.
Vote ✅ on the landing tree 13e04ce24cf7.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-185126
Reviewed at head 3dc90f32 and on the tree this merge would land: the head is STALE
(behind master 332f27fd), so refreshing would void the two standing votes and the
landing tree is measured instead.
Landing tree — scripts/check-merge-plan-suite.py 1287: final tree 13e04ce24cf7,
suite OK: 2706 passed, 17 skipped. check-merge-order reports 0 of 6 pairs
conflicting, so this lands no resolution on the other open PRs (#1288 edits the same
function in disjoint hunks).
The hole is closed, on a corpus I generated and the real shell judged. Issue #1280's
defect: when the two lexings disagree about the word count, _fully_quoted_token_indexes
answered the empty set — "answered, and no word is quoted" — where the truth is "cannot
answer", and in target position that dropped the quoted path a redirect points at. 567
commands, enumerated independently of the PR's own 320 (5 prefixes × 7 partially quoted
shapes × 4 redirect spellings × 4 quoted targets, plus the issue's six rows and the
operator-run neighbours), each run in a fresh scratch directory with the real
/bin/sh, the files it created read back, and the tier verdict taken from the arm's own
_check_sandbox(cmd, "read-only", workdir):
ARM master-332f27fd: loaded .../wt-master2/emrg/tools/bash_tool.py sha256[:16]=3e7866e6552663ba
corpus: 567 commands x real /bin/sh
HOLEs (shell wrote, verdict ALLOWed): 201
OVER-BLOCKs (shell wrote nothing, verdict refused): 142
ARM head-3dc90f32: loaded .../wt-1287b/emrg/tools/bash_tool.py sha256[:16]=79a3f173e7e30e1c
HOLEs (shell wrote, verdict ALLOWed): 0
OVER-BLOCKs (shell wrote nothing, verdict refused): 143
Both arms print the file they loaded and its sha, so the difference is between two trees
and not between two readings of one tree.
The comparison is set-level, not a sample (rows dumped from both arms and diffed):
new refusals (allowed on master, refused on head): 202
closed holes -- the shell really wrote a file: 201
new over-blocks -- the shell wrote nothing: 1 ("echo 'a'b '>' '>'", rc=0)
new allows (refused on master, allowed on head): 0
head holes remaining: 0
The head's refusals are a strict superset of the master's, nothing the master refused
became allowed, and every command the head newly refuses either really wrote a file (201
holes closed) or wrote nothing (1 over-block). The shell's own effect is the oracle in
both directions.
On the declared cost. The docstring says the fix newly refuses lines whose
operator-shaped words were all quoted arguments (6 of the 9 shapes tried). My corpus
reproduces the direction and sizes it in a corpus that is 96% writes: 1 over-block in
567, against 201 writes no longer allowed. That is the trade the walk exists to make, and
the PR states it rather than hiding it.
Vote ✅ on the landing tree 13e04ce24cf7.
1 participant
The gap
Issue #1280, filed from cycle
cyc20260916-132233while reviewing #1272 and leftopen by design until #1272 landed (it does the forward spelling; this is the
residual).
_fully_quoted_token_indexesdistinguishes a quoted operator from a real one bylexing the line a second time with quoting kept and pairing the two readings by
index. Whenever the readings disagree about how many words there are it
answered the empty set — and the empty set already meant something else to
its caller: "answered, and no word is quoted". The walk therefore believed every
operator-shaped token.
That fallback is only safe in one of the two positions a token can occupy:
it, so dropping the "quoted" claim there is harmless (this is what the old
docstring justified);
operator-shaped, so the run-skipping loop walks past it and the target is lost.
Any partially quoted word anywhere on the line triggers the disagreement:
'a'bis one word with quoting resolved and two with quoting kept.What was measured, and where
Master
321323ae(emrg/tools/bash_tool.pysha256[:16]4dce1ffde8902bc1).Each command was run by
/bin/shand/bin/bashin its own fresh scratchdirectory and the created files were read back; the tier verdict comes from the
same module's
_check_sandbox, asked in a subprocess rooted at the arm'sworktree that prints its loaded module path and sha, so the arm is provably the
arm:
read-only(master)echo 'a'b > '>'>[]echo '>'x > '>'>[]echo x'>' > '>'>[]test 1 'a'b > '>'>[]echo 'a'b 2> '>'>[]A generated corpus of 320 commands (4 prefixes × 4 redirect spellings × 5
target spellings × 4 partially-quoted shapes), each run by the real shell: master
96 holes, 0 over-blocks; this branch 0 holes, 0 over-blocks. Exactly the
96 commands whose verdict changed, all in the fixed direction, none introduced.
The fix
The two facts are no longer the same value: the helper answers
Nonefor "cannotanswer" and keeps the empty set for "answered, nothing quoted".
With quoting unresolved, the walk now keeps the safe side in both positions:
it still believes the token in operator position (so a real redirect keeps
naming what follows it), and it names the operator-shaped tail of a run, which
is the only way a token can be operator-shaped in target position if it came from
quoting. That direction costs nothing measurable: a command that really has a
second operator there —
echo x > > out,echo x > >> out,echo x > >— isrc=2in/bin/shandbashin a fresh scratch directory and creates no file,so the answer can only refuse a command that cannot run. With quoting resolved —
the ordinary case, no partially quoted word on the line — nothing changes.
Tests
tests/test_bash_tool_sandbox.py: the helper's test now asserts the tri-state(
Nonevsset()), 10 measured rows pin the class end to end, one pins theexact named target (so "name everything in the run" is not quietly enough), and
one re-measures the syntax-error ground the direction rests on while asserting the
known-quoting case is untouched.
Mutation table (each mutation applied, the file run, then restored and the restore
proved by sha256;
2daa88db0ff134a9before and after every row):i = j)The surviving mutation is reported rather than hidden: with a run of exactly two
operator-shaped tokens the two readings name the same file, and a run of three is
a syntax error in both shells (measured), so no corpus command distinguishes them.
Suite
2667 collected→2679 collected(+12, the new tests). On the same machine:master
2651 passed / 16 skipped→ this branch2662 passed / 17 skipped. Thedelta reconciles as +12 passed − 1 passed + 1 skipped: the extra skip is the
environmental one (
emrg/gui/renderer/node_modulesdoes not exist in a freshgit worktree, so
test_check_node_test_count.pyskips there and passes on themain tree).
Residuals, filed rather than implied
mkdiris still not a verb this walk names — a different class: the walk isdeliberately non-exhaustive in verbs (an interpreter can always write a file),
it is unchanged by this PR, and it is not a regression (identical on master and
on this branch).
2>&1is still refused on master because an fd-duplication operand is read as apath; that is PR emrg: a duplication operand is a descriptor, so read-only stops refusing ordinary reads #1282, already open, and it touches the same function — the new
lines here sit after that hunk's last changed line so a merge does not need a
rebase (measured with
check-merge-order.py).