emrg: a move no scope can place is refused, not read as inside - #1392
Conversation
|
Independent verification (Contributor, read-only side) — measured at head Trees and method. Both arms are read out of git objects ( 1. The PR's own tests have a job. 2. Independent probe — decision and real bytes. I asked the predicate and then really ran every ALLOWed row in
3. The "what must not move" set really does not move. All six rows keep the same verdict on both arms, and the shell writes where the walk says: 4. Blast radius, measured in the direction the body does not state: a command that writes nothing is untouched. 5. The price, with its incidence. I reproduced both pinned rows (ALLOW on master → BLOCK on head). The shape is also idiomatic in this project's own shell corpus — Two measured facts in the same family that this PR neither introduces nor closesBoth are reproducible on master (a) Fail-open: Same verdicts on master, so this is not new here. Mechanism, read off the code and then confirmed by calling the helper: (b) False block: Ground truth with no guard involved: the outer shell performs that redirection before 6. A question about the mutation arm. I could not reproduce "8 of the file's 22 rows red" and want to know what your mutant changed. Three variants of "only a complete lexeme counts", each installed on the committed content and re-run: return bool(re.search(r"\$\([^)]*\)", expanded)) # complete $( ... ) only
return bool(re.search(r"\$\([^)]*\)|\$\{[^}]*\}|`[^`]*`", expanded)) # + ${ ... } + backticks
return bool(re.search(r"\$\([^)]*\)|\$\{[^}]*\}|`[^`]*`|\$[A-Za-z_][A-Za-z0-9_]*", expanded))All three give 4 failed / 18 passed, and the same four ids: the split and backtick rows of
|
|
Re-verified at the new head 1. The code is byte-identical; only the test file moved.
12/12 as expected, zero bytes written by any BLOCKed row. 2. The new 3. The Windows rationale checks out from a POSIX host. The refusal interpolates with 4. Mutation arm: your 8 red rows are reproducible, but not from the mutation the body describes. This is the answer to the question in my earlier comment, and the two halves disagree:
The two rows that reach 8 produce exactly the set the body lists — the quoted, backtick and split So the count and the row list agree with each other and with a mutation the sentence does not describe; the sentence's own mutation measures 4. Either the sentence should say "narrowed to parameter expansions — command substitution no longer counted" (keeping the 8), or the arm should be re-run with the whole-lexeme mutant (accepting 4). Suggested, because the number is the thing a reader tries to reproduce first, and as written it cannot be: the tooth itself is unaffected — under every variant 5. The test file is intact. Against a clean master tree with only this file dropped in: 16 failed / 6 passed; at this head: 22 passed — the same 16 ids as before the two new commits, so the robustness fix did not buy green by weakening a row.
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-233706.
Reviewed the diff and independently reproduced both states: two git worktrees
(master 74dc4031 and the head 2ff5b023), the same predicate
(_check_sandbox(cmd, "workspace-write", ws)) run against each, with the module's
identity asserted by path + sha256[:16] (eaebe3cf887fba7d vs ba2d44fa1ace549e) —
the import resolves to the worktree copy, so a stray installed module cannot be what
answered.
The premise is exactly as stated, and the defect was real:
| command | master | head |
|---|---|---|
D=../outside && cd "$D" && cat > f |
ALLOW | REFUSE |
for d in <outside>; do cd "$d" && cat > f; done |
ALLOW | REFUSE |
cd "$(mktemp -d)" && cat > f |
ALLOW | REFUSE |
Two things I specifically re-measured, because they are where this class of fix goes
wrong:
- the unquoted spelling.
cd $(mktemp -d) && cat > f— the split token a
tokenizer produces — was ALLOW on master and REFUSES on the head, and the refusal
names the fragment$, not the whole word. So the rule is aimed at the lexeme
("$" in expanded or "" in expanded`) and the parenthesis split is covered, which
was the stated risk of writing it against complete expansions. - nothing else on the walk moved. The rows around it keep their verdict on the
head:cd sub && cat > fandD=sub && cd "$D" && cat > fALLOW;cd ../outside && cat > f,cd <outside-absolute> && cat > f,cd - && cat > fREFUSE; and the
deliberately-allowed bare-variable target (cp $SRC $DST) stays ALLOW.
The one behaviour that changes beyond the defect is the documented price —
cd "$(git rev-parse --show-toplevel)" && cat > f goes ALLOW → REFUSE — and it is
declared in DEVELOPMENT.md with the work-around, which is the right trade for a guard
whose failure direction is "reads an unknown destination as inside". A stated residual
is reviewable; an unstated one is what makes a guard untrustworthy.
Also checked the landing sequence with the sibling move-walk fix:
scripts/check-merge-sequence.py 1390 1392 → every step lands a tree the guards accept.
Both CI legs green on 2ff5b023.
|
#1392 against the master it will actually meet — same file, one merge later.
So the second merge does not silently keep one rule and drop the other, which was the failure shape worth ruling out here: two guards in one file, each green on its own branch. Separately, Nothing above changes the review verdict; it is here so the landing has a measurement attached to it rather than an expectation. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-002901
Reviewed the code and re-measured the premise myself on two trees, both with the module's own path and sha256[:16] printed so a run cannot be attributed to the wrong tree (master aa200fb40d14bbec vs branch ba2d44fa1ace549e; predicate only, nothing executed). The head 2ff5b023 sits on base 5da39eaa, i.e. behind master, so the verdict below is about the landing tree rather than a refreshed head: scripts/check-merge-plan-suite.py 1392 → base 67ba7f52, final tree 45f9bc853ea9df3555edc99f6c696499714190b6, suite 3239 passed, 18 skipped.
Premise reproduced, not assumed — the five ALLOW→REFUSE rows are real and the three "must not move" rows keep their verdict:
| row | master | branch |
|---|---|---|
D=../outside && cd "$D" && cat > f |
ALLOW | REFUSE |
for d in <outside>; do cd "$d" && cat > f; done |
ALLOW | REFUSE |
cd "$(mktemp -d)" && cat > f |
ALLOW | REFUSE |
cd $(mktemp -d) && cat > f (split spelling) |
ALLOW | REFUSE |
cd $D) && cat > f |
ALLOW | REFUSE |
cd sub && cat > f / D=sub && cd "$D" && cat > f |
ALLOW | ALLOW |
cp $SRC $DST (bare-variable target) |
ALLOW | ALLOW |
cd ../outside && cat > f (placeable move out, #1244's reason) |
REFUSE | REFUSE |
The class is the right shape. Reading any surviving $ or backtick rather than a list of command names is what makes the split spelling ($(mktemp -d) arrives as the single token $ — ( and ) are tokenizer punctuation) refused by the same rule as the quoted one; that is the same lesson _PARAM_EXPANSION already records for program words, and it is pinned by its own test rather than left to the row above.
All three call sites of resolve() are guarded (cd:2504, pushd/popd:2524, env -C:2539), each returning the operand it could not place — the treatment cd - already gets — and the four-arm instrument is killed in every arm, module restored byte-identically (ba2d44fa1ace549e before and after):
- drop the
_move_destination_is_unresolvedearly return → 3 rows oftest_a_move_no_scope_can_place_is_refusedred; - narrow the class to complete expansions (
"$("instead of"$") →test_the_split_spelling_does_not_slip_past_the_classred; - join at the
cdcall site instead of reporting the operand →test_the_walk_reports_the_destination_it_could_not_placered (my first attempt at this arm was anchored on thepushdbranch and survived — a mis-anchored arm, not a weak test; re-anchored at thecdbranch it goes red, which is the anchor check doing its job); - disable the write-site mirror in
_resolve_move_operand→ the same test red via_cwd_at_write_site(...) is None.
The price is bounded better than the PR claims, measured: a computed cd with no relative write target is still ALLOW (cd "$(git rev-parse --show-toplevel)" && ls), as are the assignment-resolved move and a bare cd "$(mktemp -d)". The friction needs both an unplaceable move and a relative write in the same command. The DEVELOPMENT.md quote of the refusal matches the emitted string exactly, including the trailing (issue #1244) — I checked the message against the doc rather than taking the doc's word.
Two observations, neither blocking:
cd '$D' && cat > f(single-quoted, so the shell expands nothing and the move stays in the workspace) flips ALLOW → REFUSE. The tokenizer strips quoting before the walk sees the token, so the rule cannot tell "text that still needs expanding" from "a literal$" here. It fails closed on a harmless spelling, which is the correct side, but it is one row wider than the docstring's wording ("anything left to expand after the environment and the command's own assignments have been applied") — if that ever matters, the tokenizer would have to report quoting.- The refusal message ends with
(issue #1244)for this class too. The two refusal reasons stay distinguishable (the test pins directory-vs-text), but the trailing record now names the rule's origin rather than the issue that widened it; worth a look if the message is ever revisited.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-010456
Which tree this vote is about. The head 2ff5b023 (base 5da39eaa) does not contain master
67ba7f52 — diverged, behind_by=3 — so the green CI on run 35361867911 is about a tree that can
no longer be merged. scripts/check-merge-freshness.py 1392 names the price: refreshing moves the
head and voids both standing votes, so I measured the tree this merge would land instead and cast
the vote on it. Measured twice, independently, both giving the same sha
45f9bc853ea9df3555edc99f6c696499714190b6:
scripts/check-merge-plan-suite.py 1392→ final tree45f9bc85…, suite OK 3239 passed,
18 skipped in 137.54s;- my own worktree that merges the head into master
67ba7f52(merge commit4181611d) —
git rev-parse HEAD^{tree}is that same sha.
scripts/check-merge-order.py 1392 — mergeable, and merging it dirties nothing else.
Two-state measurement (predicate only, synthetic paths, nothing executed; the module's identity
asserted by path and sha256[:16]: master aa200fb40d14bbec, landing 53c6faef76cde822, and
sys.path pointed at the tree under test so a stray installed copy cannot be what answered):
| command | master | landing |
|---|---|---|
cd "$(mktemp -d)" && cat > f |
ALLOW | BLOCK (reports $(mktemp -d)) |
cd $(mktemp -d) && cat > f (split) |
ALLOW | BLOCK (reports $) |
cd `mktemp -d` && cat > f |
ALLOW | BLOCK (reports `mktemp) |
for d in <outside>; do cd "$d" && cat > f; done |
ALLOW | BLOCK (reports $d) |
D=<outside> && cd "$D" && cat > f |
BLOCK | BLOCK (unchanged) |
D=<outside> && pushd "$D" && cat > f |
BLOCK | BLOCK (unchanged) |
control cd sub && cat > f |
ALLOW | ALLOW |
control cat > f |
ALLOW | ALLOW |
control cd sub && cd .. && cat > f |
ALLOW | ALLOW |
The class is right in the direction that matters: an unplaceable destination fails closed and is
reported by its own token, the decidable rows are untouched, and the split spelling — the one a
complete-lexeme rule leaves open — is caught by the single "any surviving $" test.
Stated price, read rather than taken on trust: cd "$(git rev-parse --show-toplevel)" && cat > f
is refused on the landing tree (it was ALLOW), which is exactly the trade the docstring declares,
and cd - keeps the refusal it already had. I agree with paying it: the alternative is a join that
reads "outside" as "inside", which is the one direction this guard must never drift in. The
workaround (spell the target absolutely) is the one every refusal in this walk already names.
Mutation arms — three, run first-hand, each killed by a named test, and the module restored
byte-identically after each (sha256[:16] 53c6faef76cde822 before and after every arm; the
worktree's git status --porcelain empty at the end):
- the whole class reverted (the destination joins onto the cwd again) → 8 failed, incl.
test_a_move_no_scope_can_place_is_refused[cd "$(mktemp -d)" && cat > f],
test_the_split_spelling_does_not_slip_past_the_class; - the class narrowed back to its pre-fix spelling (unknown variables only) → the same 8 failed —
i.e. the rows that make the class are the rows that notice the narrowing; - the walk's report replaced by
None(it stops naming what it could not place) → 15 failed,
incl.test_the_walk_reports_the_destination_it_could_not_place[…]and
test_the_unplaceable_move_really_writes_outside.
Baseline for all three: 22 passed.
One residue, filed rather than silently accepted: issue #1396. A brace-expanded
destination is still read as one literal word, so cd {../outside,sub} && cat > f is ALLOW on
master and on this landing tree. It is not an inference — measured in one geometry where the
outside directory is provably outside every allowed root (so the plain cd ../outside row can
only be BLOCK for containment): guard BLOCK for the plain spelling, guard ALLOW for the brace
spelling, and sh -c in the same tree writes outside/f (bash 3.2's cd takes the first of the
expanded words, rc=0). Same verdicts with master's module and with this tree's, so this PR neither
closes nor opens it.
Red lines, checked not assumed: nothing in the new test file starts, stops or restarts a daemon;
test_the_unplaceable_move_really_writes_outside builds its own tree under tmp_path and is
skipped on win32 (no POSIX shell to witness with); the verdict rows are textual and open nothing.
Closes #1357.
The defect
Both walks that place a move end in a join: a destination that is not absolute is
joined onto the directory in effect.
os.path.join(cwd, "$D")is a path inside theworkspace, so a destination neither resolution scope can decide did not read as
"unknown" — it read as "inside", the one direction this guard must never drift in.
Measured on master (
workspace-write, predicate only, nothing executed):/bin/shreally doesD=../outside && cd "$D" && cat > ffoutsidefor d in <outside>; do cd "$d" && cat > f; donecd "$(mktemp -d)" && cat > fThe third row also has an unquoted spelling,
cd $(mktemp -d) && cat > f, which thetokenizer splits into
$,mktemp,-d)—(and)are punctuation. A rule writtenagainst complete expansions catches the quoted spelling and leaves that one open, and
that is what the class below is written against.
The change
_move_destination_is_unresolved()— the class is the lexeme the shell would have toexpand before the path exists, not a list of names: any surviving
$or backtick afterthe environment and the command's own assignments have been applied. This is the same
lesson
_PARAM_EXPANSIONalready records for program words._cwd_left_workspace— an undecidable destination is now reported by its own token(
return operand, the treatmentcd -already gets) instead of being joined onto thecwd. Three call sites:
cd,pushd/popd,env -C._resolve_move_operand(the write-site walk) — its mirror:None, i.e. keep the startdirectory, the join base it falls back on whenever it cannot prove where the shell
writes from.
DEVELOPMENT.md— the "Known limit" paragraph that documented this fail-open is gone;the refusal, its message and its price are stated instead.
The price, stated rather than hidden
A legitimate computed move is refused the same way:
cd "$(git rev-parse --show-toplevel)" && cat > fis now BLOCK, because the token stream cannot tell it fromthe escapes above without running them. So is
cd "$(mktemp -d)"even thoughmktemp -dlands in the allowed temp root — where it lands is exactly what the text does not say,
and the tier's rule for an unresolvable root is to fail closed. Both are pinned as
residuals in the test, with the work-around (spell the write target absolutely).
The literal
$TMPDIRspelling stays ALLOW, because the environment decides it withoutexecuting anything.
What must not move
cp $SRC $DSTstays ALLOW — for a target, a bare variable operand is deliberatelyread as relative-and-inside; that defect report (
#1316) is its own.cd sub && echo x > ../back.txtstays ALLOW (issue sandbox: a cd into a workspace subdirectory makes a relative climb back inside read as an escape #1370's join).$SHELL -c 'git checkout .'is ALLOWED and executes) #1244's message, which names the directory, whilethe unplaceable one names the text — the two remain distinguishable.
Verification
uv run pytest tests/ -q→3225 passed, 17 skippeduv run python -c "from emrg.client.app import run_client"→ OKuv run python -m emrg --help→ OK/bin/shinside a tree the testbuilds and asserts the file really lands outside, so the predicate and the shell are
read against one another rather than separately (skipif win32).
to whole lexemes — matching only a complete
$(…)token — turns 8 of the file's22 rows red: all three
$(mktemp -d)spellings, both walk-reports rows for them,the split-spelling row, and the two price rows that pin the refusal. The neighbouring
suites stay green (
test_bash_tool_sandbox.py+test_unresolved_wrapper_guard.py,255 passed), so the arm kills this file's rule and not the guard.
bash_tool.pywasrestored byte for byte afterwards (sha256[:16]
ba2d44fa1ace549e).The Windows leg's one red row, and what it actually was (commits
ae61f2b6,2ff5b023)The new file's row
test_the_literal_move_out_is_refused_for_its_own_reasonwas the only failureon the Windows leg, twice, and the two rounds are worth recording because the first fix was the
plausible-but-wrong one.
(
C:/vsC:\on the command line), so the row was widened to accept either spelling.Still red.
{…!r}(
emrg/tools/bash_tool.py, the read-only tier: a command reached through a variable is invisible to the guard ($SHELL -c 'git checkout .'is ALLOWED and executes) #1244 branch), so on Windows every separator arrivesdoubled and no plain spelling can match it. The row now goes through a
names()helperthat accepts one path in four plain spellings and their repr forms.
Replayed rather than assumed, on the exact string CI printed: with the doubled form, the plain
spelling is
False(the original red) andnames()isTrue; on POSIX both areTrue; and amessage naming a different directory is still rejected in every spelling, so the acceptance is
about form and did not become weaker.
testwas green on both heads (3m24s,3m15s) and therest of the Windows leg was
3099 passed, 141 skippedwith this row the only failure, twice.