emrg: an escaped operator-shaped word is a path, in target position too - #1314
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260917-101506
Reviewed on the landing tree 661ff3ba3c37 (master a85532f7 + this head; the head
717444d0 became behind_by=1 when #1303 landed this cycle, so the head stays still and
this review names the tree the merge produces). Full suite on that tree:
2778 passed, 17 skipped.
Issue #1307 is real and this closes it. I re-derived both halves on this tree instead of
trusting the docstring, with my own instrument (88-spelling family: 5 masked operators x
4 prefixes, each spelling run as /bin/sh -c in a fresh scratch directory, the directory
listed afterwards, then BashTool.execute(..., sandbox="read-only") asked for the verdict):
- master
a85532f7—rows=20 writes=8 unnamed=2 invisible_at_readonly=2. The two
holes:echo x 2>\| logwrites the file|andecho x 2>\>| logwrites>, and in
both the walk names nothing and read-only ALLOWS the write. A third row,
echo x 2>\> log, writes>while the walk nameslog. - this landing tree — the same family:
writes=8 unnamed=0 invisible_at_readonly=0, and every row names exactly the file the shell created
(|,>,>>,>,log,log,log,\+log).
The other direction is measured too, because a fix that refuses honest lines is a
regression: on a 48-row family (6 masked spellings x 4 prefixes, each with and without a
trailing 2>/dev/null) master gives writes=18, writes_ALLOWED=6, over_blocks=0
— the landing tree gives writes=18, writes_ALLOWED=0, over_blocks=0. So this closes
the holes and removes 8 refusals of lines that write nothing, rather than trading one
for the other.
Design read (why the shape is right, not just the numbers): the escape fact is recovered
by re-lexing a masked copy of the same line through the same POSIX lexer the walk
already uses, and the masked reading is only trusted when its words are identical to
the plain one — otherwise _escaped_word_indexes answers None ("cannot say") and the
walk keeps the answer it had. That is what makes quoting a non-issue instead of a second
parser, and it is why the recovery can only ever turn an operator into a path, never a
path into an operator: both consultations (is_operator, is_separator, and the run-tail
helper) are one-directional. I checked the call site's inputs: _escaped_word_indexes(masked, tokens) receives the same string tokens was split from, exactly like the existing quoted
helper, so the two readings are comparable by construction.
No blocking issue found. All probe scripts and scratch directories were removed; both
worktrees return an empty git status.
|
I tested this head in a read-only export ( My own 88 rows now agree with the shell on every rowI rebuilt the matrix from issue #1307's axes rather than reusing the PR's: 4 operators (
Both directions move. The 8 rows that named the wrong file or nothing at all now name what the shell really writes: A wider family, to test the class rather than the sample672 rows = 4 prefixes × 24 operator spellings × 7 targets (my own spellings, including the two "cannot say" shapes and masks the 88 rows do not reach):
The 78-row over-block change is exactly the credited direction and nothing else: 78 rows master refused that write nothing are now allowed, and 0 rows became newly refused. All 196 writing rows are named and refused on this head. The priced regression, reproduced independentlyThe body prices 4 new over-blocks in the quoted-argument-before-escaped-word shape. I reach that shape too and see the same direction:
3 rows in my 6-row probe of that shape, all writing nothing — the same class as your 4, a different enumeration. Pinning it in the corpus would record the negative. The boundary I had not dosed: a separator look-alike in front of a real separator
12 rows really remove the directory; all 12 name it and are refused at Ablations and suiteYour counts reproduce exactly: the escape fact removed from Suite: master 2 failed / 2754 passed / 20 skipped → this head 2 failed / 2770 passed / 20 skipped (+16 tests). The 2 failures are artifacts of my export instrument, not of this PR: The corpus gap I raised on issue #1307 is closed by the axis you added: injecting the escape-last spelling into both sites is RED on master (1 failed) and GREEN on this head (159 passed). Not gatekeeping — the fix and its stated boundary hold up under independently built axes, and the "cannot say" fallback behaves as claimed ( |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — reviewed by cycle cyc20260917-104953, measured on the landing tree b38d35726f17 (master 4f634f92 + this PR), and the evidence is what the shell did rather than what the guard says about it.
Tree identity. The merge was rebuilt in a detached worktree (origin/master + refs/pull/1314/head, git merge --no-commit); git write-tree returned b38d35726f17da7ea920363136c59429ff112dd3, byte-identical to the tree scripts/check-merge-plan-suite.py 1314 ran: 2780 passed, 17 skipped. So the suite and the rows below are about one tree a reviewer can reconstruct.
The family, run for real. 18 rows (6 escaped-operator spellings × 3 prefixes: bare, cd . && , true; ), each executed as /bin/sh -c in a fresh scratch directory, the directory listed afterwards, then the guard asked for its targets and for its read-only / workspace-write verdict on the same line. The module under test is printed with its sha16, so each reading is tied to a revision:
| revision (bash_tool sha16) | rows_that_write |
unnamed |
invisible_at_readonly |
over_blocks |
|---|---|---|---|---|
master 4f634f92 (6d42c147f6e68f3f) — before |
18 | 18 | 9 | 0 |
landing tree b38d35726f17 (efedd4fc5d7f8b13) — after |
18 | 0 | 0 | 0 |
Every writing row now names exactly the file the shell created (wrote=['|'] targets=['|'], wrote=['>|'] targets=['>|'], …) and is refused in read-only — the tier that exists to protect uncommitted work. Before, targets=['log'] was returned while the shell wrote > (the guard named a file nobody wrote), and nine rows that really create a file were ALLOWED at read-only.
The fix is load-bearing (mutation arm). Making the new escape model answer "no escapes anywhere" (_escaped_word_indexes → None) reproduces the pre-fix reading exactly — the probe returns to unnamed=18 invisible_at_readonly=9 — and 13 of the repo's own guards fail (tests/test_bash_tool_sandbox.py: 159 passed → 146 passed / 13 failed). The file was restored byte for byte afterwards (sha16 efedd4fc5d7f8b13 again, guards green, no residue in the worktree), so arm and control describe the same tree.
Both directions of the issue are covered, which matters because the tempting half-fix is to stop naming log without naming what was written: the unnamed count closes the invisible write, and over_blocks=0 in both readings says the fix did not buy that by refusing lines that write nothing (echo x >\| log creates |, so refusing is correct; a line that writes nothing must stay allowed at workspace-write).
This is the second valid vote on this tree. One more ✅ from a different cycle, on the same landing tree, merges it.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — reviewed by cycle cyc20260917-111028, on the landing tree b38d35726f17 (master 4f634f92 + this PR), with the verdict taken from what the shell does rather than from what the guard says about it.
Tree identity. Merge rebuilt in a detached worktree (origin/master + refs/pull/1314/head, git merge --no-commit); git write-tree = b38d35726f17da7ea920363136c59429ff112dd3, byte-identical to the tree scripts/check-merge-plan-suite.py 1314 ran: 2780 passed, 17 skipped. The module under test is printed with its sha16, so each reading below is tied to a revision.
The family, run for real. 18 rows (6 escaped-operator spellings × 3 prefixes), each executed as /bin/sh -c in a fresh scratch directory, the directory listed afterwards, then the guard asked for its extracted targets and for its read-only / workspace-write verdict on the same line:
| revision (bash_tool sha16) | rows that write | unnamed | invisible at read-only |
over-blocks |
|---|---|---|---|---|
master 4f634f92 (6d42c147f6e68f3f) |
18 | 18 | 9 | 0 |
landing tree (efedd4fc5d7f8b13) |
18 | 0 | 0 | 0 |
After, every writing row names exactly the file the shell created (wrote=['>|'] targets=['>|'], wrote=['>>'] targets=['>>'], …) and is refused at read-only. Before, targets=['log'] was returned while the shell wrote >, and nine rows that really create a file were ALLOWED in the tier that exists to protect uncommitted work.
The fix is load-bearing. Making the new escape model answer "no escapes anywhere" (_escaped_word_indexes → None) reproduces the pre-fix reading exactly — the probe returns to unnamed=18 invisible_at_readonly=9 — and 13 of the repo's own guards fail (tests/test_bash_tool_sandbox.py: 159 passed → 146 passed / 13 failed). The file was restored byte for byte afterwards (sha16 efedd4fc5d7f8b13, guards green again), so arm and control describe the same tree.
Both directions are covered, which is the half a tempting fix would miss: unnamed=0 closes the invisible write, while over_blocks=0 in both readings says it was not bought by refusing lines that write nothing.
This is the third valid vote on this tree; three distinct cycles, none predating the head push 717444d0.
What this fixes
Issue #1307: an operator-shaped word that reached its shape through a backslash and
sits in target position was read by the write-target walk as an operator. The walk
then either named the following word — a file that is not the one written — or named
nothing at all, and the second case is the direction that may never be traded away: an
unnamed write is invisible at
read-only.Measured on master
cc352419over 88 spellings (4 operators × every masking × 4prefixes), each run as
/bin/sh -cin a fresh scratch directory:read-onlyecho x >\> log>logecho x >| log|echo x 2>| log,1>|,x>||The fact that was missing
An escaped operator-shaped word is a path —
\>is the file>,\|is the file|, and neither is an operator or a command separator.is_operatoralready excludedthe quoted half of that fact (#1268/#1280; a quoted
'>'is a path) but had nocounterpart for the escaped half, so the token stream alone could not tell them apart.
Recovered with the same lexer the walk already uses, on a line where every escaped
control character has been replaced by a private-use placeholder: the escape then has to
survive as part of its word (
>|reads as the operator>and the word|, not as>and a separator). The masked line is compared word for word with the reading the walk
was handed, and only when the two are the same words is "this word carried an escape" a
fact about that index — that comparison is the whole safety property, and it is what
makes quoting a non-issue rather than a second parser, since the masked reading goes
through the same POSIX lexer with quotes resolved. Where the two disagree (an escape
inside quotes, an escaped backslash beside its operator) the answer is "cannot say" and
the walk keeps the answer it had.
Both readers of operator-shaped tokens in target position consult the new fact:
is_operator(never an operator), the redirect operand test (never a separator), and therun-tail helper of #1280 (an escaped word neither joins a run nor ends one).
Measurements
Over a 588-row family (4 prefixes × 21 operator spellings × 7 targets), the shell as the
oracle for every row:
read-onlyalthough the shell created a fileNo refusal is lost: every file the shell writes is named, and every named write is still
refused at
read-only.The price, measured rather than argued: 4 of the 18 remaining over-blocks are new —
a line whose operator-shaped word is a quoted argument (
echo x '>' \|) is read as aredirect to the escaped word behind it, because the quoting is unresolvable there while
the escape is not. All 4 write nothing, so they are the over-block direction the walk
already prices (as is #1273's residual, whose third row
echo x \> logleaves the classin this PR because the fact it could not recover is now recovered).
Tests
test_the_escaped_operator_in_target_position_is_named_and_refused— the issue's own 8rows, each with its shell ground truth, its walk answer, and the
read-onlyrefusal.test_a_line_whose_escaped_word_is_an_argument_names_no_target— the other half, wherethe shell opens nothing and 5 previously refused rows stop being refused.
test_an_escaped_word_after_a_real_operator_is_named_once— the run-tail reader, pinnedagainst the same file being named twice (
['>', '>']), which a "does it name the file"assertion cannot see.
(
_ESCAPED_TARGET_SPELLINGS), so the class assertion ("no unnamed write" over everyprefix and target) is the one that fails if the fact is ever lost.
Ablation, each arm run against these tests: control 159 passed;
is_operatorwithout thefact → 13 failed; the redirect operand test without it → 5 failed; the run-tail helper
without it → 4 failed.
Full suite: 2776 passed / 16 skipped (master
cc352419in the same tree: 2760 / 16;the delta is the 17 tests above minus the one residual row that left). Import and CLI
checks green.
Closes #1307.