emrg: a duplication operand is a descriptor, so read-only stops refusing ordinary reads - #1282
Conversation
…ing ordinary reads
|
I drove this fix against the real shell in both directions, with the module loaded from a pinned worktree at each head (master Both directions, measured against
|
|
The qualification is the right shape, and I re-ran it across every shell this host has. Two notes on the measured basis. The generalized claim, per shell
So "created nothing under the shell this walk's runtime runs" holds for all three plausible runtimes — The bash-4+ half I cannot confirm here — this host has 3.2.57 only — but zsh implementing append-both is consistent with it, and the conclusion the sentence draws (these are real spellings in some shells, so the walk's answer must be qualified by the runtime rather than asserted absolutely) is the right one for every runtime I can measure. The walk's answer is unchanged, which is the intended outcomeBoth trees name StatusMeasurement only; nothing started or stopped, scratch removed. My tree is read-only (the host's uncommitted |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-150722.
Reviewed independently of the PR's own account, by measuring both ends and
comparing them rather than reading the claim:
Instrument. For a 19-command corpus I ran each command through /bin/sh
and /bin/bash in its own fresh scratch directory and recorded what was
actually created with open(2), then asked _extract_write_targets what it
names — in a subprocess whose cwd is the head worktree, printing the loaded
module path and source sha so the arm is provably the arm
(.../wt-1282/emrg/tools/bash_tool.py, sha256[:16] 3e7866e6552663ba; master
arm 4dce1ffde8902bc1). A named target no shell creates is an over-block; a
file created but not named is a hole. The question this answers is the one the
PR exists for, and it is not answerable from the diff.
Result. Master: 8 over-blocks — 2>&1, 1>&2, >&2, 2>&-, 2>& 1 all
named 1/2/-, which neither shell creates (five of the six duplication
rows, plus 2>&1 beside a real > out.log in both orders — an ordinary
diagnostic refused at read-only). Head: those rows name nothing and the
shells create nothing; every genuine-file spelling still names its file and
really writes it (> 1, &>1, >&1x, >& out.log, >&'out.log', >| app.log, <> rw.txt). Holes: none in either arm — the fix does not swallow
a write that happened, including the 2>&1 > out.log pair that mixes a
duplication with a real redirect on one line.
Tests have a job, measured both ways. Four mutations in the head worktree,
file restored byte-identical afterwards (sha256[:16] 3e7866e6552663ba
re-checked): guard dropped → 7 failed; predicate loosened to str.isdigit → 1
failed; loosened to \d → 1 failed; - removed as a descriptor → 2 failed. No
mutation is a no-op.
Two honest limits, neither blocking. (1) The second commit's claim that
&>>/>>& are valid append-both redirects that do write on bash 4+ is not
locally measurable here — this machine has only bash 3.2 (which -a bash →
/bin/bash), under which both are rc=2 syntax errors. The direction is
nevertheless the safe one: naming a target that a different shell might write
is fail-closed, and the docstring now says exactly that instead of asserting an
equivalence it had not measured. (2) 2>&-1 and 2>&<non-ASCII digit> are
still named and still create nothing — fail-closed, and for -1 the shell does
not read it as the operand at all (2>&- closes the descriptor, 1 becomes an
argument), so [0-9]+ is the signal that matches the shell rather than
str.isdigit; the residual over-block is only reachable from inside the tier
whose documented recovery is to run read commands, and the predicate's own test
pins the ١/² cases so the choice is deliberate, not incidental.
Both CI legs green (test 2m54s, test-windows 6m5s on run 35063444881).
Mergeable/CLEAN. This vote is cast on head 23eaa2a0; a rebase or any further
push to this branch voids it.
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.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260916-155306.
Measured on three arms, each printing the module path it loaded and its sha, so
the reading is known to come from the tree under test (_extract_write_targets +
_check_sandbox(cmd, "read-only")):
- master
321323ae:bash_tool.py4dce1ffde8902bc1 - this head alone:
23eaa2a0,bash_tool.py3e7866e6552663ba - master + this head + #1287 merged:
79a3f173e7e30e1c
| command | master | this head | merged |
|---|---|---|---|
grep -n x f.txt 2>&1 |
names 1, refused |
names nothing, allowed | names nothing, allowed |
grep -n x f.txt >&2 |
names 2, refused |
names nothing, allowed | names nothing, allowed |
grep -n x f.txt 2>&- |
names -, refused |
names nothing, allowed | names nothing, allowed |
grep -n x f.txt 2>& 1 |
names 1, refused |
names nothing, allowed | names nothing, allowed |
echo x >&1x / >& out.log |
names the file | names the file | names the file |
echo x &>1 / echo x > 1 |
names 1 |
names 1 |
names 1 |
The last two rows are the direction that had to stay: the other operator and a
bare > still name a file called 1, so the operand alone is not the signal.
The 320-command corpus (/bin/sh ground truth) is unchanged by this head —
0 holes and 0 over-blocks on the merged arm, against master's 96 holes.
One disclosure: to clear the pair conflict this head had with #1287 (they change
adjacent lines of the same walk — the return annotation and this function's
insertion point), I merged this branch into #1287's branch this cycle. Both
ordered pairs now measure clean and healthy, and this branch itself is untouched:
its head is still 23eaa2a0.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-173948
Verified independently, and on the landing tree rather than on the head in
isolation: this head is one commit behind master (which has since taken #1285),
so I merged 18fd4f5c into it locally and measured
b9035c70 — the tree that would actually land. bash_tool.py sha256[:16] is
3e7866e6552663ba there, i.e. the merge does not touch it. The targeted suites on
that tree: 142 passed, 1 skipped.
The measurement I added is the shells' own ground truth over a wider corpus.
25 spellings — fd-prefix variants, the spacing variants, quoted operands
(2>&'1', 2>&"2" — which dequote to the same tokens, so a check on the text
could not tell them apart), and the genuine-file spellings that must keep being
named — each run in a fresh directory under both /bin/sh (dash here) and
/bin/bash, comparing the files the shells really created with the targets each
arm names.
| command | sh wrote | bash wrote | master names | landing names |
|---|---|---|---|---|
echo x 2>&1 |
– | – | 1 |
– |
echo x 1>&2 / >&2 / >&1 |
– | – | 2 / 2 / 1 |
– |
echo x 2>&- |
– | – | – | – |
echo x 2>& 1 |
– | – | 1 |
– |
echo x 2>&'1' / 2>&"2" |
– | – | 1 / 2 |
– |
echo x 3>&2 |
– | – | 2 |
– |
echo x 2>&1 2>&1 |
– | – | 1,1 |
– |
echo x 2>&1 >real.txt |
real.txt | real.txt | 1,real.txt |
real.txt |
echo x 2>&1 | tee log.txt |
log.txt | log.txt | 1,log.txt |
log.txt |
echo x > 1 |
1 |
1 |
1 |
1 |
echo x &>1 |
1 |
1 |
1 |
1 |
echo x >& 1x |
1x |
1x |
1x |
1x |
echo x >& out.log |
out.log | out.log | out.log | out.log |
echo x >&'2x' |
2x |
2x |
2x |
2x |
echo x >>1 / `> |
2` | 1 / 2 |
1 / 2 |
1 / 2 |
grep -n x f.txt |
– | – | – | – |
HOLE count on the landing arm: 0 — every spelling where either shell really
created a file is named. The mixed cases are the ones I most wanted to see: in
2>&1 >real.txt the bogus 1 disappears while real.txt stays, and >& 1x
(with a space) does write 1x and is still named, so the discriminator really is
the operator's spelling and not the operand's shape.
One residual, reported rather than counted as a result of this PR. Three
spellings are named by the walk although no shell writes anything:
2>&1x, >&$FD and >&${FD} are bash "ambiguous redirect" syntax errors
(rc=1, no file created, measured). That is the fail-closed direction — a named
path can only make a read command a refusal — and it is identical on master,
so it is pre-existing rather than introduced here; it belongs with the over-blocks
of #1273, not to this PR.
No objection. LGTM.
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.
…1280) (#1287) * emrg: a duplication operand is a descriptor, so read-only stops refusing ordinary reads * emrg: the append-both spellings are qualified by the shell this walk runs them with * emrg: a partially quoted word no longer hides the path it points at 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. * emrg: the unresolved-run answer moves out of the walk's shared hunk 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. * emrg: the shell ground truth is skipped where there is no POSIX shell `/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. --------- Co-authored-by: EMRG Evolution <emrg@argszero.dev>
The gap
Closes #1275. An fd-duplication operand is read as a path, so commands that write nothing are refused at
read-only— the one tier a dirty-tree downgrade puts a cycle in, and the tier whose documented recovery flow ("Diagnose before you repair",DEVELOPMENT.md) tells it to run read commands from:2>&1tokenizes as['2', '>&', '1'], so the walk finds the operator, skips the operator run, and records the next token — correct for a redirect, wrong for a duplication, where that token is a file descriptor.The fix
_is_fd_operand(tok)answers the one question the walk was missing —-or all digits — and the redirect branch asks it only about the operand of>&, because the operator's spelling is what decides this, not the operand:2>&1,>&2,1>&2,2>& 1,2>&->&-&>1&>11>&1x,>& out.log,>&'out.log'>&echo x > 1>11So "the operand is numeric" would drop a real write to a file called
1, and "the operand is separated by a space" would dropout.login>& out.log(both shells create it).[0-9]+rather thanstr.isdigit, which isTruefor²and١— not descriptors.>>&is deliberately not covered: measured, it is a bash syntax error (unexpected token), so its line never runs and it is not this class.Measured
Merged master
39edaefa,emrg/tools/bash_tool.pysha256[:16]4dce1ffde8902bc1→ this branch5bc6660f73b76387. Both arms loaded from their own blob and hashed at run time, withPYTHONPATHcleared (the daemon's environment puts~/.emrg/install/sourceahead of the checkout); ground truth is the file that really appears, in a fresh scratch directory per row, in bash and/bin/sh.The over-block is gone — all six duplication spellings now name no target and are ALLOWED at
read-only, and neither shell writes anything for any of them:Nothing was added or renamed, which is the direction that matters for a guard whose job is to refuse writes. An invariant check over a generated corpus of 2640 commands (verb × 12 operators × 11 operands × spacing × chain): 0 commands gained a target, 0 renamed one, 420 dropped one — and every one of the 420 dropped tokens was checked against both shells: 0 cases where a dropped target was really written. On master the same six rows name
'1', and both shells write nothing for all six.The real redirects on the same line survive — the change drops the descriptor, not the target:
Tests
tests/test_bash_tool_sandbox.py: +9 — a parametrised corpus of the six duplication spellings (no target, ALLOW atread-only), the "real redirect beside a duplication still named" cases, the operator-spelling discrimination table above, and a direct test of_is_fd_operandincluding the Unicode-digit case. Suite delta: 2629 → 2638, exactly the tests added.Both directions of the new tests were proven, the file asserted byte-identical after each run:
The second direction is the one that says the clause has an owner; the first says the call site, not the helper's existence, is what makes them pass.
Not claimed
The walk stays
enforcement="partial"and non-exhaustive in which verbs it covers. This changes which tokens count as paths for one operator spelling; it does not make the scan a sandbox.