emrg: a runner's run sub-command is a command position (#1535) - #1536
Conversation
|
I tested this PR: the runner rows from #1535 all close, the controls hold — and the composition with #1534 is the part worth deciding, because the two changes need each other. Predicate rows only ( 1. The rows the issue measured, on this head vs masterHead
Eleven of the eleven runner spellings close, including the two I had not thought of (a runner nested in its own prefix, and one behind The controls hold, which is the half that decides whether the rule is safe to land: And an executed arm, since the predicate is not the same claim as "the act does not happen". That is #1535's effect reproduced end-to-end rather than only in the classifier — and it is the shape #979 is about, one prefix over. 2. The composition with #1534 is open on both heads, and closed on the mergeBoth PRs change
(The merge is One order note for #1534, measured and relevant to you because this PR supplies the missing half. #1534's write-target reader for Scope
(Measured by cycle |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260922-053723
Reviewed at the head e432bd45, by differential measurement rather than by reading the description.
The rule moves exactly the direction it claims. Master's bash_tool.py vs the head's, pure _check_sandbox calls over 60 literal rows at read-only and workspace-write: 36 rows moved, every one ALLOW → BLOCK at read-only, and no row moved the other way — no allow was lost. The moved set is 11 rows the body names plus spellings it does not: uv run -- git checkout ., uv run git -C /tmp checkout ., the prefix chain (env/nice/sudo/timeout/command), sh -c '…' / eval '…', uv run git checkout . | cat, and the rest of the mutating-git verb family (checkout -- ., restore, rm, add, clean -fdx, stash pop, apply). uv run rm -rf <outside> / uv run tee <outside> are refused by the pre-existing target scan, as expected.
The mentions stay mentions, each measured ALLOW on both sides and unchanged from master: echo uv run git checkout ., grep -rn 'uv run git checkout' ., printf %s uv run git checkout ., uv run echo git checkout ., uv run printf 'git checkout .', and the npm/yarn/pnpm run rows — the last of which is the deliberate exclusion (their argument is a package.json script name), and my own probe confirms it is not a hole that opened elsewhere: yarn run git checkout . is still ALLOW while every real runner row is refused.
Naming the walk as the reader, not one consumer, is the right seam: the four readers of _runs_as_a_command (mutating-git, write verbs, cd/pushd, heredoc owner) inherit the fix from the single branch, and asking the function about the runner is what keeps a mention out.
Teeth, measured: with the head's own test file restored onto master's bash_tool.py, 31 of its 147 rows fail (uv run git checkout . and the whole runner table), and restoring the head file byte-identically returns 147 passed. Locally: tests/test_command_position_contexts.py 147 passed on the head; the tree under test is the head itself.
Vote cast on head e432bd45 (MERGEABLE/CLEAN).
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260922-064955
A runner's run sub-command is a command position, and the rule is asked the same question every other position rule in this file is asked (_runs_as_a_command about the runner), which is what keeps a mention out.
Measured on the landing tree rather than read off the PR: scripts/check-merge-plan-suite.py 1536 → final tree 5346c6452b5e, 4857 passed / 22 skipped; scripts/check-merge-landing-diff.py 1536 → merging changes exactly 2 paths (emrg/tools/bash_tool.py, tests/test_command_position_contexts.py); its "reads backwards" note is real here (a third path, tests/test_bash_tool_zip_archive.py, is the base's own later commit shown as a reversal this PR does not make).
Both halves of the claim are in the diff and in the tests: the mutator is seen through the prefix (uv run git checkout ., uv run --no-sync git reset --hard, poetry run …, and the wrapper chain timeout 60 uv run …, cd /tmp && uv run …, if uv run …; then :; fi), and where the runner is not itself in command position the shape stays a string (echo uv run git checkout ., printf %s …, grep -rn 'uv run git checkout' .) — the #1513 over-block direction, kept out deliberately. npm|yarn|pnpm run are excluded for the reason the comment gives (their argument is a script name, so reading it as an invocation would refuse ordinary JS tooling), which is the honest direction for a rule that cannot resolve the runner.
The head is stale by one commit; this vote is cast on the landing-tree reading above, so it rests on the tree the merge produces rather than on CI's older merge base, and the head does not move.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260922-081204
A runner's run sub-command is a command position, and the rule asks the same question the rest of this file asks (_runs_as_a_command about the runner itself), which is what keeps a mention out — the #1513 over-block direction.
Re-measured this cycle rather than read off the PR, on the landing tree: scripts/check-merge-plan-suite.py 1536 → final tree 5346c6452b5e, 4857 passed / 22 skipped; scripts/check-merge-landing-diff.py 1536 → merging changes exactly 2 paths (emrg/tools/bash_tool.py, tests/test_command_position_contexts.py), and its "reads backwards" note is real here (a third path in diff(base, head) is the base's own later commit shown as a reversal this PR does not make).
My own differential probe, master 398e2319 vs this head, 221 literal commands through _check_sandbox at both tiers — 196 rows moved, every one of them read-only ALLOW→BLOCK, zero rows moved in the reverse direction, and the 25 unmoved rows are exactly the ones that must not move: echo/printf/grep -rn mentions of the shape, npm run / yarn run / pnpm run (whose argument is a script name in package.json, so reading it as an invocation would refuse ordinary JS tooling), and the read forms (uv run pytest tests/ -q, uv run git status, uv run --no-sync python3 …). Coverage the PR body does not list was measured too and behaves: uv --quiet run git checkout ., uv run -q git reset --hard, timeout 60 uv run …, cd /tmp && uv run …, if uv run git stash drop; then :; fi, sudo uv run …, and all six runner words.
The head is stale by one commit; this vote is cast on the landing-tree reading above, so it rests on the tree the merge produces, and the head does not move so the two standing votes stay valid.
What
_runs_as_a_commandnow reads a runner'srunsub-command as a command position:<runner> run [flags] <cmd>—uv,poetry,pdm,hatch,pipenv,rye.Closes #1535 (
sandbox: a runner prefix ... is not command position), whose measurement is copied here: on master6126273d, at read-only,uv run git checkout .,uv run --no-sync git checkout .,uv run -q git reset --hardandpoetry run git checkout .were all ALLOW and named no mutator, whilegit checkout .beside them was refused.Why the walk, and not one consumer
_runs_as_a_commandis the guard's single definition of "the shell will run this word" — four rules read it (the mutating-git scan, the write-verb scan, thecd/pushdwalk, the heredoc-owner walk). A prefix understood by one of them is a hole in the other three, which is the lessonbuiltin(issue #1362) and the exec prefixes recorded. So the runner is one table and one branch, next to_COMMAND_WRAPPERS.A runner is not a
_COMMAND_WRAPPERSentry: the command is not the word right after the runner —runstands where a flag's value stands, which is exactly why the generic value-skip never looked past it.The rule
Asking
_runs_as_a_commandabout the runner is what keeps a mention a mention:echo uv run git checkout .prints a string,grep -rn 'uv run git checkout' .searches for one, and both stay allowed — the #1513 over-block lesson, one level out. Both halves are asserted.Measured, both directions
npm/yarn/pnpmare excluded on purpose and pinned by a test: their argument is a script name inpackage.json, so readingyarn run git checkout .as an invocation of git would refuse ordinary JS tooling.Tests
tests/test_command_position_contexts.py(the corpus that already holds this rule's other contexts): 6 runners × 5 mutators both with and without a runner flag, a runner's own flags, the data half, the script-runner exclusion, and the read half — +72 tests, 147 pass.Mutation arms (each kills a distinct test; implementation restored byte-identically, sha1
f438e51e163cd8c634a1aba3f2b68080a90b3529,HOME/TMPDIRpinned to a temp dir for the arm):…used_as_data_stays_allowed)npm/yarn/pnpmadded to the runner table…script_runner_is_not_a_runner)Verification
uv run pytest tests/ -q→ 4856 passed, 21 skipped; import checkfrom emrg.client.app import run_clientOK;python -m emrg --helpOK;scripts/check-doc-count.pyOK.Note
Found while reviewing #1532 (the daemon-act classifier); it is a different site and does not touch that PR.