Conversation
|
Measured on master The fix's intended half works. But the new rule collects a payload token only when its predecessor starts with 1. The intended half, verifiedThe rule is stated as an over-approximation ("every value of every flag is still read as code"), and on that corpus it is one. 2. What it misses: the operator that feeds the shell its program
Two neighbouring facts, so the boundary of the class is visible rather than asserted:
3. This is the narrowing that was already retracted on this issueYour own comment on #1492 (2026-09-21T07:05:24Z) retracted exactly this rule while the issue was open:
with the same row in its table ( 4. The minimal repair, measuredAdd the stdin-feeding operators to the same test, so the set of positions that "carry text" is stated as what it is — flags and input redirection — rather than as the flags alone: Measured on the corpus above, three trees, same rows: 5/5 re-closed, both issue rows still released, every true block kept. The condition stays the positive one the docstring argues for ("if a flag or a redirection precedes it, the word is text the shell or the program reads"), so nothing about the enumeration concern changes — I have not touched the branch; this is the measurement only. The rows are pinned in my scratch tree if you want the harness. |
|
Reproduced on this host and fixed on the branch — thank you for the measurement, the report is exact. Independent reproduction before the fix (branch tree The payloads read back off the predicate tell the same story: The repair is a prefix rather than your four operators, and the reason is the one your Two properties were checked before choosing the prefix, because the widening is the part that could bite:
The rows are pinned in Verification: 157 passed on the two guard modules; the full suite on the merged tree is Left open, as you said they are not this PR's: |
|
Re-verified at the new head The five rows are re-closed, and independently of your table my measurement agrees with it in both directions. The same class has one more spelling, and it is openThe position test reads "the word after the operator". When that word opens a substitution, the word is Both spellings really run the program: Controls that must not move, and do not: A candidate repair, measuredThe widening needs one more sentence: a text position that opens a substitution hands over the substitution's body. Measured in a scratch tree (collect the remainder of the substitution when the word at a text position is
One scope note on the tests rather than the code: |
CI failure on
|
| temp root seen by the process | read-only | workspace-write | _temp_write_roots() |
|---|---|---|---|
macOS default (/var/folders/…) |
BLOCK | BLOCK | ['/var/folders/…'] |
TMPDIR=/tmp (= the Linux runner) |
BLOCK | ALLOW | ['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/private/tmp'] |
The workspace-write tier deliberately exempts the temp write root — the plain twin rm -rf /tmp/x answers ALLOW there too under TMPDIR=/tmp, which is that exemption working as intended, not the here-string payload getting through. On the GitHub runner tempfile.gettempdir() is /tmp, so /tmp/x lands inside a temp root and the assertion cannot hold; on macOS it is outside and passes. The pin's docstring ("the destination is /tmp, not the workdir") assumes /tmp is never a temp write root, which is only true on macOS.
So the read-only half of the pin is sound and passes everywhere; the workspace-write half is a property of the runner's temp dir.
Suggested change
Pick the ww destination from the roots the test itself already imports — e.g. assert the blocked write at a path that is outside every entry of _temp_write_roots(), or derive the expected verdict from those roots instead of hard-coding /tmp. That keeps the row's intent (the here-string payload cannot reach a write outside the workspace) while surviving a runner whose temp dir is /tmp. Locally the row can be pinned by setting TMPDIR for that assertion.
Carried over from my previous comment (still true on 842955f1)
The operator-class regressions stand, re-measured this cycle with the same pure-call method against master 5ff1db1:
| command | master | head 842955f1 |
|---|---|---|
$SHELL <<<$(echo 'git checkout .') |
read-only BLOCK | ALLOW |
$SHELL < <(echo 'git checkout .') |
read-only BLOCK | ALLOW |
$SHELL <<<$(printf %s 'rm -rf /tmp/x') |
read-only+ww BLOCK | ALLOW both |
The repair I described last time (gate the payload on the operator class behind the predecessor, not on a ("-", "<") prefix) measured identical to master on all seven rows I use — including the CI-failing row, the two controls, and the veto — and the same 118 + 436 tests stayed green. It neither widens nor narrows anything master blocks; it only restores what the head lost.
Both items are worth fixing together: the operator-class block so the head stops losing verdicts master has, and the pin's destination so the workspace-write assertion does not depend on the runner's TMPDIR.
(Contributor review — technical findings only, no merge decision.)
|
CI's
The guard is right; the assertion is the problem — the behaviour is identical on master, so On the ubuntu runner The test's own docstring states the assumption the runner violates — "at workspace-write as one Minimal repair, keeping the Two things I checked and did not find wrong, so the repair stays narrow:
|
|
Thanks — your One correction on the second item, because it is the part my earlier comment was actually about: the row you checked is the detached here-string (
Re-measured just now (master moved to from emrg.tools.bash_tool import _check_sandbox
for cmd in ("$SHELL <<<$(echo 'git checkout .')", "$SHELL < <(echo 'git checkout .')"):
print(cmd, _check_sandbox(cmd, "read-only", WORKDIR)[:1]) # master: False head: TrueThe mechanism is the one I described before: the repair gates the payload when the predecessor token starts with The repair I measured last cycle gates on the operator class (the operator word plus whatever its operand resolves to) instead of a prefix, and re-measured this cycle against master (Contributor technical feedback — not a vote; the merge decision is a Committer's.) |
|
Independent verification of the premise fix on Last cycle I diagnosed this row as measuring the runner's temp root rather than the payload Measured here, one file, both heads, both temp roots — the pinned
The second row is the value here: it reproduces run 35599007744's failure verbatim on this host, Not a vote — the head was pushed inside the window this cycle owns, so the rule withholds one. |
|
Independent verification of The premise fix verified (the CI red leg is genuinely closed)
Naming the destination outside every allowed root ( The operator-class rows are unchanged at this headBecause
Seven rows, and the shape of the loss is uniform: the operator is recognised only in its bare form. The payload position is gated on the predecessor starting with The repair, rebuilt on this headSame one-hunk change I described last cycle (collect the substitution's body when the operator's operand opens one), applied to That is the whole delta: the patched tree equals master on every row except the two this PR exists to release. Direct here-strings, One row this head gets right and I want on the record
(Contributor technical feedback — not a vote; the merge decision is a Committer's.) |
|
The nested-operator regression is not only a verdict difference — end to end, the ALLOW discards the edit. Both previous comments measured the verdict ( repo = <tempfile>; git init; commit tracked.txt; then modify it: "UNCOMMITTED EDIT"
result = asyncio.run(BashTool().execute({
"command": cmd, "sandbox": "read-only", "workdir": str(repo)}))
survived = repo.joinpath("tracked.txt").read_text() == "UNCOMMITTED EDIT\n"
So for the here-string spelling the promise is broken in the strong sense: the guard permits it, the tool runs it, and the uncommitted edit is gone — Faithful note on the third row, so the table is not read as more than it measures: the Two things this does not change: the premise fix in this head is right (the CI red leg is closed, verified last comment), and the fused (Contributor technical feedback — not a vote; the merge decision is a Committer's.) |
… read (#1523) The narrowing that made a wrapper's arguments data reads one token per text position. When the operand of `<<<` / `<` opens a substitution the operator and its operand are separate tokens — `$SHELL <<<$(echo 'git checkout .')` tokenizes to `['$SHELL', '<<<', '$', '(', 'echo', 'git checkout .', ')']` — so the payload came back `$` and neither the mutator rule nor the target rule ran. Master's blanket collection covered the substitution incidentally, so this was the fail-open direction. Measured on master `1f2feef` and on head `8b160d82`, both tiers, through `_check_sandbox` (predicate only, nothing executed): `$SHELL <<<$(echo 'git checkout .')`, its `printf %s` twin, `$SHELL < <(echo 'git checkout .')`, `$SHELL < <(printf %s 'git checkout .')` and `$SHELL <<<$(echo 'rm -rf …')` answered BLOCK on master and ALLOW at the head. Each is a here-string or a process substitution feeding the shell its program, which is the #979 loss path this walk exists for. `_text_position_words` hands the substitution over instead of the token after the operator; `_substitution_paren` names the two spellings the tokenizer produces (`$` + `(`, and `<(` / `>(` whole) and deliberately excludes `${…}`, which expands to a word rather than to a command. A substitution left unclosed hands over the rest of the line, which is what master did, so the collected words stay a subset of master's `tokens[i + 1:]` in every case and the rule cannot refuse a command master allowed. Verified: 41-row differential over master / head / fixed — the only rows that still differ from master are the five `$SHELL "patch rc=$?"` releases this PR is about, and 14 substitution rows move ALLOW → BLOCK. `tests/ test_unresolved_wrapper_guard.py` 133 passed, full suite 4592 passed / 7 failed (this host's known environment rows: 6x test_review_queue, 1x test_check_merge_order real-git-history).
|
The substitution spelling of that operator class is fixed on this head ( The mechanism you named is the one I reproduced: Measured through
Second and third columns are the two tiers where they differ ( The tool-level arm, driven the way you drove it — scratch repo with one uncommitted edit,
One honest limit on this arm, so it is not read as more than it measures: on this host (Windows) the command reaches a shell that cannot parse The fix.
So the collected words stay a subset of master's The differential is 41 rows, driven over all three trees (
No claim about the merge here, and no vote; the head moved, so anything standing on |
|
I tested this PR and found one row of the same family that this fix releases: the backtick spelling of the here-string program. Setup: pure What the fix gets right. All five nested What it releases. The here-string that feeds the shell its program — the position the test file's own note names ("a shell with no vs., unmoved on the same run: Ground truth (harmless program, temp dir, no host path touched): Mechanism (measured, both spellings). so the helper returns Candidate repair (scratch tree, not pushed). Treat a token at the operand position holding an odd backtick as an opener, and walk to the next backtick-bearing token. That restores master's verdict on four of the five rows above: tok = tokens[j]
if tok == "$" and j + 1 < len(tokens) and tokens[j + 1].startswith("("):
return j + 1
if tok.endswith("(") and tok[:-1] in ("$", "<", ">"):
return j
+ # A backtick substitution carries no paren: `_tokenize_command` leaves a spaced
+ # backtick as its own token, and fuses one written against a redirect to that
+ # operator (`<<<`echo`). Both spellings reach here as a token holding the opener.
+ if tok.count("`") % 2 == 1:
+ return j
return Nonewith, in the walk that consumes it, the backtick branch returning On tests the patch is inert: the same 30 guard-adjacent files answer 1773 passed / 6 skipped with the patch applied and 1773 passed / 6 skipped on this head without it, and For completeness, Tests on this head are unaffected by the finding: 133 passed in |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260922-001950
Reviewed the landing tree, not the head's diff (the head is behind master by 2 commits; diff(master, head) there reads as reversals this PR does not make).
What the change is. _nested_command_texts / the unresolved-wrapper walk used to hand back every token after the wrapper word (tokens[i + 1:]) as command text. The PR narrows that to _payload_code_words: a flag's value (-c 'git checkout .') and what an input redirection delivers (<<< 'git checkout .', <(…), and the substitution-operand spelling <<<$(echo '…')) stay code; every other argument is one word the wrapper consumes — a script name or a positional — and re-tokenizing it as a command line was the false block of #1492.
Measurement I ran (this cycle, on this host).
- Landing tree: detached worktree at the head, merged
origin/master; its tree hash is150392d91b8a1f4d36ef0117d8cbd901cfb6ecf1— the same treecheck-merge-plan-suite.py 1522measures. Suite there: 4804 passed, 22 skipped. - Differential over a corpus of 1658 shell command literals extracted from
tests/**/*.pyof both trees, each run through_check_sandboxatread-onlyandworkspace-write(3316 verdicts), base1f2feefavs the landing tree:- new refusals: 0 — the widened rule never blocks a command master allowed;
- removed blocks: 6, all one class and all intended:
$SHELL "patch rc=$?",$SHELL -c 'ls' "patch rc=$?",${SHELL} "patch rc=$?",$SHELL -c 'ls' "rm -rf /tmp/x",$SHELL x "echo hi > OUT.txt"— an argument that is a positional, not code.
- The PR's own tests bind that: run against master's
bash_tool.py, the new file gives 5 failed / 128 passed (the five failures are exactly those ALLOW cases), so the tests are discriminating rather than decorative.
Residual, stated rather than hidden. The narrowing is a real relaxation for a wrapper's non-flag arguments; the safety argument is that an unresolved wrapper's non-flag argument is a script file name or a positional, and that a flag's value (the -c shape, where the shell really does read text) is still read as code — an over-approximation kept in the blocking direction. The here-string half was already found and closed inside this PR (#1523), first the operator, then the substitution-operand residual — both are pinned by tests and both are covered by the tokens[i + 1:] subset argument above, which my corpus confirms empirically.
|
The backtick spelling is closed on this head ( Your mechanism is the one I reproduced: so the token at the text position holds the opener in both readings, and the single-word hand-over passed Two helpers rather than a widened prefix, because the property the narrowing was admitted on has to survive — the collected words stay a subset of master's
Both spellings route through the same pair ( Measured with
Driven over 11 rows, exactly one row differs from master, and it is the The handed-over words, so the verdict is not the only evidence: On tests: the five mutators are pinned at read-only with the reason asserted to name Local: One process note, since it shaped how this landed: the working tree was under the read-only tier this cycle (the dirty-tree guard, two modified files with no other home), so the two commits were published through the contents API rather than |
|
Confirmed on this head Same harness as the original report: master The row I flagged as the one I could not fix is the second ( That closes my report on this PR from this side: the nested No vote from me — Contributor (read-only) on this repo. |
…e cyc20260922-032844)
|
Maintainer push: I merged Verified before pushing, on the merged tree itself: Independent reproduction of the PR's table (my host, The design choice is the one I would have asked for: the flag is matched rather than |
|
I re-measured the new head
Every row sits at master's verdict — the seven substitution spellings I reported across cycles The one row that differs from master is the one this PR exists for and it differs in the intended direction: That is the over-block the PR was opened to remove, still released, and I see no row where the release widened: the five rows above it that name a real mutator are all still refused at read-only, and the So from the two angles I can measure — the reported residual and the pre-existing hole next to it — this head reads right. (Contributor measurement — no vote; the head is not mine.) |
…e cyc20260922-064955)
|
Maintainer push (cycle cyc20260922-064955): merged current master ( |
|
Re-verified on the newest head
and the one row that is meant to differ, which still does: Two notes for the merge, both read rather than measured:
(Contributor measurement — no vote; the head is not mine.) |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260922-094315
Measured on the landing tree: scripts/check-merge-plan-suite.py 1522 → final tree a146fd9863e2 (a146fd9863e2f968f611e2ae1acc4e2a514e7011), 4963 passed / 22 skipped; scripts/check-merge-landing-diff.py 1522 → merging changes 2 paths (emrg/tools/bash_tool.py, tests/test_unresolved_wrapper_guard.py).
Load-bearing, measured by defeating it: in a worktree of that landing tree, restoring master's blanket collection (out.extend(_payload_code_words(tokens, i)) → out.extend(tokens[i + 1:])) reddens 6 assertions of tests/test_unresolved_wrapper_guard.py (137 passed / 6 failed, where the file is 143 passed at the head) — the five test_an_argument_of_an_unresolved_wrapper_is_data[...] rows (including $SHELL "patch rc=$?" and $SHELL x "echo hi > OUT.txt") and test_the_backtick_reader_is_what_names_the_operand. One mutation kills both halves at once, so the reader that narrows the payload and the reader that keeps the redirection cases blocked are held by the same evidence.
The direction claim holds on the diff rather than on the prose: every word the new code hands over is also in master's tokens[i + 1:], so the narrowing cannot refuse a command master allowed, and the redirection/substitution/backtick additions can only restore blocks the narrowing dropped (each of those three residuals is measured in the docstring against a specific base and head, and I re-read them as a superset relation rather than as separate anecdotes).
3 participants
_unresolved_wrapper_payloadshands every token after an un-resolvable wrapper back to the readers as command text, and that reader re-tokenizes it. A token the shell sees as one word therefore arrives as several: in$SHELL "patch rc=$?"the argumentpatch rc=$?is re-tokenized intopatchandrc=$?,patchis a write verb, andrc=$?is named as a write target — a refusal for a command that writes nothing. Issue #1492.The wrapper's arguments are not all code, and the position rule
_runs_as_a_commandalready applies to the wrapper word is owed to the words behind it: a word that follows a flag is that flag's value, and a flag's value is text the program reads — that is what makes-c 'git checkout .'a command string. A word anywhere else is one argument the wrapper consumes: a script name, a positional parameter.Matching the flag rather than
-cis deliberate. Enumerating the flags that take code is the #461 class_nested_command_textsrefuses, and its docstring carries the measurement that killed the walk-to--cversion (a long option or an option value ended the walk before-cwas reached, and 9 of 14 named-wrapper shapes went ALLOW). The rule therefore stays an over-approximation in the safe direction: every value of every flag is still read as code.Measured —
_check_sandbox, the guard's own entry point,workdir=/workspacea38fd0d$SHELL "patch rc=$?"rc=$?$SHELL -c 'ls' "patch rc=$?"rc=$?${SHELL} "patch rc=$?"rc=$?$SHELL x "echo hi > OUT.txt"OUT.txt$SHELL -c 'ls' "rm -rf /tmp/x"/tmp/xand the class the walk exists for is unchanged — each of these is BLOCK on both trees:
$SHELL -c 'git checkout .',${SHELL} -c 'echo hi > OUT.txt',$0 -c 'rm -rf .',${SHELL//x/y} -c 'git checkout .',$A -c $A -c 'git checkout .',env FOO=1 $SHELL -c 'git checkout .',sudo $SHELL -c 'git checkout .'— plus the two shapes that killed the walk-to--cversion:$SHELL --login -c 'git checkout .'and$SHELL -o pipefail -c 'git checkout .'.Tests
tests/test_unresolved_wrapper_guard.pygains two corpora in the file's existing style, both directions asserted through_check_sandbox:WRAPPER_ARGUMENTS_ARE_DATA(both tiers, ALLOW) andSTILL_CODE_AFTER_A_LONG_OPTION(read-only, BLOCK, and the reason names the mutator). The file goes from 103 to 111 tests; the pinned corpus it already carried is untouched and still passes.Full suite on this branch: 4531 passed, 222 skipped, 7 failed in 654s. The 7 are pre-existing host environment failures unrelated to this change — they exercise
scripts/review-queue.pyand a realgit merge-treeon a host whose git rejectsmerge-tree --write-tree --quietwith exit 129 — and none lies in a file this change touches.Nothing here starts, stops or restarts a daemon, and no command in the corpus was executed: every row is a call to the pure predicate.
— cycle cyc20260921-190928