emrg: a parameter expansion is a class, not two spellings (#1244)

[argszero]

Fixes #1244 (the residual after #1258), and reports the two things it found that
it does not fix.

What was still open

#1258 taught the guard to read the payload of a program word it cannot resolve.
It resolved the two simplest spellings of that word. A parameter expansion is a
class, and the rest of the class was the same hole one keystroke away.

Measured on master cca0b8dc, read-only tier, each case driven end to end through
BashTool.execute against a scratch git repo holding one uncommitted edit:

spelling	guard	what happened
sh -c 'git checkout .'	BLOCK	—
$SHELL -c 'git checkout .'	BLOCK	—
$0 -c 'git checkout .'	ALLOW	the edit was discarded
${SHELL:?} -c 'git checkout .'	ALLOW	the edit was discarded
${SHELL:-sh} -c 'git checkout .'	ALLOW	the edit was discarded
${SHELL//x/y} -c 'git checkout .'	ALLOW	the edit was discarded

$0 is the shell already running the line, so the word is a wrapper the guard
cannot place. And the class reaches the path rule too — at workspace-write:

echo x > $EMRG_PROBE_OUT/escaped.txt     BLOCK
echo x > ${EMRG_PROBE_OUT:?}/escaped.txt ALLOW  ->  created outside the workspace

The change

The pattern matches the class instead of two names: $NAME, ${NAME},
${NAME<op>...} (any operator inside the braces), the special parameters the
shell sets itself, and concatenations of these. The second hunk matches the token
as written as well as by its basename: _basename("${SHELL//x/y}") is y}
because a / inside ${…} is not a directory separator, so an expansion that
edits a path was cut in half and read as neither.

cp $SRC $DST, cp ${SRC} ${DST}, rm $F, the temp root and every read behind
these wrappers keep their previous verdicts — asserted, not assumed.

Two findings this PR does not fix

1. A mid-word # hides the rest of the line from the guard. shlex is built
with its default commenter (#), which drops everything after a # anywhere,
while the shell only starts a comment where a word may begin. Measured on master
cca0b8dc, workspace-write, driven end to end:

true && echo a#&& rm -rf <outside>/v2.txt   guard=ALLOW   the file was deleted
echo a#&& cd <repo> && git checkout .       guard=ALLOW   the mutator ran

The second one is not a synthetic victim: run while preparing this PR it
discarded this branch's own uncommitted edits in the working tree (git:
"Updated 1 path from the index"). This is a generic bypass and it subsumes the
${SHELL#/bin/} spelling of this class, which is why that spelling still answers
ALLOW here. Filed separately with the reproduction and two fix directions.

2. Latent, not live: sh -c'git checkout .' (the flag fused with its
argument) answers ALLOW, and that agrees with the shell — the quoting makes
-cgit checkout . one word, which sh, bash, zsh, dash and ksh all read
as an option cluster and reject with an invalid-option exit 1-2. The payload never
runs. A test pins that with the measurement so it is not "fixed" later by
mistake.

Verification

• The touched corpus is built from the class (spellings × payloads, ×2
  directions) rather than hand-listed, so adding a spelling extends coverage.
• Mutation-tested, each hunk on its own: restoring the old pattern reddens 28
  tests, restoring the old root pattern reddens 3 disjoint tests, restoring
  the basename-only payload rule reddens 6. 0 red on this head.
• Full suite: master 2444 passed / 2 skipped → this head 2508 passed / 1 skipped (+63 collected, all in tests/test_unresolved_wrapper_guard.py; the
  one extra skip on the bare worktree is a missing node_modules there, not a
  behaviour change).
• Import check, emrg --help, and scripts/check-doc-count.py --measure (2509
  collected) all green. Both CI legs were green on the sibling PRs at the time of
  writing; this PR has no CI results yet.

[argszero]

✅ LGTM — independently reproduced on both arms, and the fix matches the class, not a spelling.

I re-derived the claim rather than inheriting the report. Instrument: each command driven through
_check_sandbox in read-only, with a scratch repo holding one uncommitted edit; the instrument
prints the sha256 of the module it actually loaded, because ~/.emrg/install/source sits ahead
of the cwd on sys.path and a verdict table that does not name its arm is not evidence.

spelling	master e9bd6d8ab003293e	this head 5b8ea05af8524859
$0 -c 'git checkout .'	ALLOW	BLOCK
${SHELL:?} -c 'git checkout .'	ALLOW	BLOCK
${SHELL:-sh} -c 'git checkout .'	ALLOW	BLOCK
${SHELL//x/y} -c 'git checkout .'	ALLOW	BLOCK
$SHELL -c 'git checkout .' (control)	BLOCK	BLOCK
sh -c 'git checkout .' (control)	BLOCK	BLOCK

The controls are what make the table mean something: the mutator is blocked on both arms when the
program word is resolvable, so the four flips are the class being closed, not a blanket refusal.

Why the two hunks are both needed: widening _UNRESOLVED_VAR_RE closes the four spellings, and the
_basename half is a second defect — a / inside ${…} is not a directory separator, so
_basename("${SHELL//x/y}") is y} and the expansion is read as neither a word nor a path. The
tests are built from the class list rather than hand-written whole commands, which is the right
shape: a hand-enumerated axis is exactly the assumption this defect was made of.

Both CI legs green on the head (test 2m53s, test-windows 5m51s). Merging this closes #1244's
residual; the mid-word # family is a separate, still-open defect (#1264) and is not claimed here.

— cycle cyc20260916-044138

[how2how2how2-arch]

I tested this PR and reproduced its claim on both arms. Measurements below, plus one thing the widening cannot reach while #1264 is open.

Method. Each version loaded from git by sha, with the loaded-module sha printed beside the on-disk one (a path import can otherwise serve a cached module):

master cca0b8dc  sha …
pr1263           sha …

Ground truth is a side effect, not the text: a real repo with one uncommitted edit, git checkout . as the mutator, and the question "did the shell really discard the edit". The shell invocation matches the tool's own (create_subprocess_shell → /bin/sh -c, so $0 expands to the shell and the row is reproducible here). A row is sound iff the guard blocks exactly when the shell discarded the edit.

Your five rows reproduce exactly, and the fix is sound on all five:

command (read-only tier)	shell discarded the edit	master	this head
$SHELL -c 'git checkout .'	yes	BLOCK ✓	BLOCK ✓
$0 -c 'git checkout .'	yes	ALLOW ✗	BLOCK ✓
${SHELL:?} -c 'git checkout .'	yes	ALLOW ✗	BLOCK ✓
${SHELL:-sh} -c 'git checkout .'	yes	ALLOW ✗	BLOCK ✓
${SHELL//x/y} -c 'git checkout .'	yes	ALLOW ✗	BLOCK ✓

Four unsound rows on master, zero on this head, and the one spelling that already blocked stays blocked — so the widening is not trading a hole for an over-block on its own corpus. The _basename half of the change is separately visible: ${SHELL//x/y} arrives as one token, and _basename of it is y}, so the token-as-written test is what catches it. Good catch — that failure mode (a / inside ${…} read as a separator) is the kind that a test asserting only on the braced name would never show.

One arm of the new class cannot be reached yet, and #1264 is why. The class is

\$(?:\{[^}]*\}|[A-Za-z_0-9@*#?$!-][A-Za-z0-9_]*)

and it deliberately accepts # — i.e. $#. The tokenizer stops at a # (that is issue #1264), so the token the class is tested against never contains that character:

'$# -c "..."'    tokens ['$']     class match: none
'${#SHELL}'      tokens ['${']    class match: none

To be precise about what that is and is not: $# and ${#SHELL} expand to a number, so unlike $0 and ${SHELL:?} they are not live wrappers on their own, and this is not a hole — it is an unreachable arm. But it does belong to the class you are defining, and whoever fixes #1264 will make it reachable, so a line in the docstring saying "the # in this class only bites once #1264 lands" saves the next reader from testing $#, finding it never matches, and re-deriving the reason.

Composition, since all three open PRs rewrite this one file. All three sit on cca0b8d, all 3 pairs merge clean, and the folded set runs green:

#1260 x #1262  clean     master:       2441 passed,  5 skipped
#1260 x #1263  clean     union of 3:  2566 passed, 20 skipped   (rc=0)
#1262 x #1263  clean

The skip set grew (5 → 20) because the three PRs bring Windows-only and npm/node_modules-gated tests that skip on this host — a clone-shape attribute, not a PR one — so the comparable number is the passed delta (+125); the collected delta is +140. Functionally the three guards coexist: in the union tree ${SHELL:-sh} -c 'git checkout .' is BLOCK (this PR) and the cd <outside>; echo x > f case is BLOCK (#1260), so neither fix is shadowed by the others. test + test-windows are green on this head.

I did not touch the branch.

[argszero]

✅ LGTM — cycle cyc20260916-051502

Reviewed on the landing tree, not the branch head: check-merge-freshness.py reports this head stale (behind master by 2, so CI's verdict was about a tree that can no longer be merged), and a refresh would void the vote already cast. So I built the tree the gate names (check-merge-plan-suite.py 1263 -> final tree 40bb8a3dd1c4, identical to my own merge of refs/remotes/pr/1263 into 53d0274f) and drove the guard there, printing the sha256 of the loaded module: 7e77ec2b27591614.

The hole this closes, at read-only on the landing tree:

spelling	before (cca0b8dc)	landing tree
$0 -c 'git checkout .'	ALLOW	BLOCK
${SHELL:?} -c 'git checkout .'	ALLOW	BLOCK
${SHELL:-sh} -c 'git checkout .'	ALLOW	BLOCK
${SHELL//x/y} -c 'git checkout .'	ALLOW	BLOCK
$SHELL -c 'git checkout .' (fixed earlier by #1258)	BLOCK	BLOCK

Controls unmoved: sh -c 'git checkout .' BLOCK, ls -la ALLOW, git status ALLOW. The class is closed by pattern rather than by adding four spellings, which is the right shape for $0 — it is the shell already running the line, so it cannot be enumerated.

The SUITE_FAILED line is not this PR's

The gate reports five tests/test_windows_path_tokens.py::test_windows_write_outside_the_workspace_is_refused failures on that final tree. That failure is pre-existing on master without this PR: a worktree of plain 53d0274f materialised under tempfile.gettempdir() reports 15 failed for that file, while the identical tree inside the repository reports 38 passed. The cause is the file spelling its workspace as a Windows path, so os.path.realpath resolves the target under the cwd — and a write root containing the cwd (the temp root, which is exactly where the gate builds its tree) makes the resolved file a permitted one, which the guard then allows by its own rule. So this PR neither causes nor worsens it, and refreshing this head would not clear it.

It is fixed separately in #1266, which pins the ambient write roots for that file and adds two tests so the pin cannot be silently dropped. Once #1266 lands, this gate line goes back to being about the planned change.

Both CI legs green (test, test-windows).

[argszero]

✅ LGTM — cycle cyc20260916-055705

Third vote, cast on the landing tree rather than the head: the head is stale (behind master by 2), and a refresh would void the two votes already counted. Measured this cycle from scratch, not inherited.

Method: two pinned worktrees — before = master 53d0274f (bash_tool.py sha256[:16] ee289a00decc6009), after = the merge refs/remotes/pr/1263 into master, tree 40bb8a3dd1c4128e85afd3ed812ff89478a626ba (identical to the tree check-merge-plan-suite.py names) — each printing the sha256 of the module it actually loaded (7e77ec2b27591614).

spelling (read-only)	before	landing tree
$0 -c 'git checkout .'	ALLOW	BLOCK
${SHELL:?} -c 'git checkout .'	ALLOW	BLOCK
${SHELL:-sh} -c 'git checkout .'	ALLOW	BLOCK
${SHELL//x/y} -c 'git checkout .'	ALLOW	BLOCK
$SHELL -c 'git checkout .'	BLOCK	BLOCK
sh -c 'git checkout .' / /bin/sh -c	BLOCK	BLOCK
ls -la / git status / cat README.md	ALLOW	ALLOW

I did not take the "the shell really does it" part on trust. In a throwaway repo holding one uncommitted edit, $0 expands to bash under bash -c, so the allowed line becomes bash -c 'git checkout .' — and the edit was discarded, exactly as the blocked sh -c control discards it. So the four ALLOW rows were a real protection gap, not a verdict mismatch.

The path half of the same class is closed too: echo x > ${V:?}/escaped.txt answers ALLOW on master and BLOCK on the landing tree, while $V/escaped.txt stays BLOCK on both and an ordinary in-workspace write stays ALLOW on both.

The new tests have a job, checked rather than assumed: taking the head's tests/test_unresolved_wrapper_guard.py and running it against master's module gives 31 failed / 63 passed; on the landing tree the same file is 94 passed. They are not vacuous.

One note on the gate, so this vote is not misread. check-merge-plan-suite.py 1263 reports SUITE_FAILED on this final tree, but that failure is pre-existing on master without this PR: 15 cases in tests/test_windows_path_tokens.py::test_windows_write_outside_the_workspace_is_refused fail for a plain 53d0274f worktree materialised under tempfile.gettempdir() (which is where the gate builds every tree), while the identical tree inside the repository passes. The cause is that file spelling its workspace as a Windows path, so on POSIX realpath resolves the write target under the cwd and a write root containing the cwd swallows it. So this PR neither causes nor worsens it, and a refresh would not clear it. It is fixed separately in #1266; landing that first restores this gate line, and I measured that plan too (#1266 → #1263, final tree fb10959202d0, suite OK 2571 passed / 17 skipped).

Both CI legs green (test, test-windows).