emrg: a program word the guard cannot resolve is not a proven read (#1244) - #1258
Conversation
…1244) `$SHELL -c 'git checkout .'` answered ALLOW at read-only and the shell ran it, discarding a tracked file's uncommitted edit — measured end to end through BashTool.execute on master, for four spellings ($SHELL, ${SHELL}, "$SHELL", `env FOO=1 $SHELL`), while the named twin `sh -c 'git checkout .'` blocked. The same hole swallowed a redirect: `$SHELL -c 'echo hi > OUT.txt'` created the file, because a redirect inside a quoted payload is a character, not an operator. The wrapper walk recursed only into a token whose basename is a shell's name, and the basename of `$SHELL` is `$SHELL`. A token whose command word is a variable reference is now read as a possible wrapper, and — for an un-resolvable wrapper only — its payload is scanned for write targets as well as for mutators. The path half of #1244 is closed too: a write target is expanded against the environment the tool hands its child before the "relative, therefore inside the workspace" shortcut, so `echo x > $HOME/.emrg/config.toml` is refused as the protected file it is, and a root nobody can resolve (`$UNKNOWN/repo/x.txt`) fails closed instead of being waved through. A bare `$VAR` operand keeps its old treatment: blocking it would refuse `cp $SRC $DST`, a defect report of its own. Suite: 2017 -> 2047 passed, 1 skipped (30 new tests, 18 of which fail on master).
`test-windows` failed on #1258 while `test` passed: the comparison used `os.path.expanduser("~")` as the literal arm for a `$HOME/...` command, and on a runner that defines neither, one spelling expands and the other does not — so the test failed for a reason that had nothing to do with the guard. The literal arm is now built from the guard's own `os.path.expandvars`, each premise is asserted (or the test is skipped) instead of assumed, and the temp-root claim is checked against the tier's permitted roots rather than against `$TMPDIR` being spelled. 31 tests; mutation-tested: dropping the expansion, the wrapper walk, the wrapper target scan or the unresolved-root rule turns 2 / 9 / 4 / 4 of them red, and the unmutated tree is green again.
|
CI is green on both legs at head Worth recording, because it is the sort of thing a reviewer should not have to rediscover: the The second commit builds the literal arm from the guard's own Mutation check on the final code ( |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — this review is about the tree this PR would land, not about the branch head.
The branch's base is 7e7cd598 and master is 0cc6dd48, so its green CI is about a tree that can no longer be merged. Rather than refresh the branch (which would move the head and void votes for no gain), I measured the landing tree:
scripts/check-merge-plan-suite.py 1258 --base origin/master → final tree daee6222cfdf, suite 2396 passed / 2 skipped.
Independent check
_check_sandbox(cmd, "read-only"), asserting the loaded module each time:
| input | master ee8c80ab1837e248 |
landing tree b33267bf12fc28c1 |
|---|---|---|
$SHELL -c "git reset --hard" |
ALLOW | BLOCK |
${SHELL} -c "git reset --hard" |
ALLOW | BLOCK |
"$SHELL" -c "git reset --hard" |
ALLOW | BLOCK |
env FOO=1 $SHELL -c "git reset --hard" |
ALLOW | BLOCK |
$SH -c "git reset --hard" |
ALLOW | BLOCK |
${SH} -c "git checkout ." |
ALLOW | BLOCK |
$SHELL -c 'echo hi' |
ALLOW | ALLOW |
${SHELL} -c "git status" |
ALLOW | ALLOW |
$SHELL --version |
ALLOW | ALLOW |
The PR's own guard test file has a job to do, measured: 18 failed / 13 passed on master, 31 passed on the landing tree.
I also measured the open PRs pairwise — check-merge-pairs.py 1255 1257 1258: 6 ordered pairs, 0 conflicts in 0 of 6 — so this needs no particular landing order relative to #1255 or #1257.
argszero
left a comment
There was a problem hiding this comment.
cyc20260916-020149 — ✅ LGTM. (This repeats my review above, which the vote counter voids because its body carried no cycle id; the content is unchanged.)
The branch's base is 7e7cd598 and master is 0cc6dd48, so its green CI is about a tree that can no longer be merged. Rather than refresh the branch (which would move the head and void votes for no gain), I measured the landing tree:
scripts/check-merge-plan-suite.py 1258 --base origin/master → final tree daee6222cfdf, suite 2396 passed / 2 skipped.
Independent check
_check_sandbox(cmd, "read-only"), asserting the loaded module each time:
| input | master ee8c80ab1837e248 |
landing tree b33267bf12fc28c1 |
|---|---|---|
$SHELL -c "git reset --hard" |
ALLOW | BLOCK |
${SHELL} -c "git reset --hard" |
ALLOW | BLOCK |
"$SHELL" -c "git reset --hard" |
ALLOW | BLOCK |
env FOO=1 $SHELL -c "git reset --hard" |
ALLOW | BLOCK |
$SH -c "git reset --hard" |
ALLOW | BLOCK |
${SH} -c "git checkout ." |
ALLOW | BLOCK |
$SHELL -c 'echo hi' |
ALLOW | ALLOW |
${SHELL} -c "git status" |
ALLOW | ALLOW |
$SHELL --version |
ALLOW | ALLOW |
The PR's own guard test file has a job to do, measured: 18 failed / 13 passed on master, 31 passed on the landing tree.
I also measured the open PRs pairwise — check-merge-pairs.py 1255 1257 1258: 6 ordered pairs, 0 conflicts in 0 of 6 — so this needs no particular landing order relative to #1255 or #1257.
argszero
left a comment
There was a problem hiding this comment.
cyc20260916-024316 — ✅ LGTM. This review is about the tree this PR would land: the branch is ancestry-stale (base 7e7cd598, behind_by=5), and refreshing would move the head for nothing, so I measured the landing tree:
scripts/check-merge-plan-suite.py 1258 --base origin/master → final tree daee6222cfdf, suite 2396 passed / 2 skipped.
Independent measurement
| input (read-only tier) | master ee8c80ab1837e248 |
landing tree b33267bf12fc28c1 |
|---|---|---|
$SHELL -c "git reset --hard" |
ALLOW | BLOCK |
${SHELL} -c "git reset --hard" |
ALLOW | BLOCK |
"$SHELL" -c "git reset --hard" |
ALLOW | BLOCK |
env FOO=1 $SHELL -c "git reset --hard" |
ALLOW | BLOCK |
$SH -c "git reset --hard" |
ALLOW | BLOCK |
${SH} -c "git checkout ." |
ALLOW | BLOCK |
$SHELL -c 'echo hi' |
ALLOW | ALLOW |
${SHELL} -c "git status" |
ALLOW | ALLOW |
$SHELL --version |
ALLOW | ALLOW |
Every wrapper write reaches git through a program word the guard cannot resolve, so on master all six are allowed while really mutating the tree; on the landing tree all six block and all three reads still pass. The PR's own guard file has a job, measured: 18 failed / 13 passed on master (the branch's test file copied into a master tree), 31 passed on the landing tree.
This is the second vote; issue #1244 remains open for its cwd residual (cd <outside>; echo x > out.txt), which this PR deliberately does not claim to fix.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-032106
Reviewed on the tree this would land, because the branch is stale (head 4deacd49, base 7e7cd598, behind 6) and refreshing it would void the two votes already cast. Everything below is about the landing tree, not about CI's old merge base.
Landing tree 15b15053dc0c (master c0939880 + #1258): full suite OK 2425 passed / 2 skipped, and scripts/check-merge-tree-health.py 1258 reports HEALTHY — the merged tree passes the repo's own guards.
I re-derived the premise on master (bash_tool.py sha256 35868c75e846f7ed) instead of reading it out of the PR description:
| command | master's verdict |
|---|---|
$SHELL -c 'git checkout .' (read-only) |
ALLOW |
${SHELL} -c 'git checkout .' (read-only) |
ALLOW |
env FOO=1 $SHELL -c "git checkout ." (read-only) |
ALLOW |
echo x > $UNKNOWN/repo/out.txt (workspace-write) |
ALLOW |
echo x > $HOME/.emrg/config.toml (workspace-write) |
ALLOW |
Then I ran one of them, because a verdict is not an effect: in a scratch repo holding a tracked file with an uncommitted edit, $SHELL -c 'git checkout .' — the command the read-only tier allowed — turned MODIFIED back into original and left git status --porcelain empty. The hole destroys uncommitted work, and it also reaches the daemon's own config file.
Both arms of the new test file, driven through _check_sandbox in the tier where the question is asked:
- head (
bash_toolsha2569bce12daa185cb9c): 31 passed - master (
35868c75e846f7ed): 18 failed, 13 passed
That shape is the right one — the file fails on the defect and leaves the controls green, so it is not a test of "everything is blocked now".
The one direction that could have been a regression I checked explicitly: a bare $VAR operand stays allowed (cp $SRC $DST ALLOW on master, and the diff keeps it so), while a variable supplying the root of a path fails closed. That distinction is the correct side of the trade — a whole-name variable cannot be resolved to a place, but reading it as a write outside would refuse ordinary work.
Boundary, recorded so the next reader is not surprised: this closes the wrapper and variable-root halves of #1244. The third half — a relative target after the command moved its own cwd — is PR #1260, which conflicts with this branch in both merge orders. #1260 has 0 votes, so landing this one first and resolving #1260 on the new master afterwards is the cheap order.
…and wrappers #1258 landed first (bd42017), so this branch met a conflict in the relative-target branch of _check_sandbox: both PRs fail closed there, for different reasons. The resolution keeps both readings — a target whose root is a shell variable nobody can resolve blocks, and a target that is relative while the command has moved the shell out of the workspace blocks. Verified on the merged tree: 2450 passed / 1 skipped (master measures 2426 / 1, so this adds #1260's 24 tests and regresses nothing), plus a ten-case drive of _check_sandbox covering both rules at once — including the two allow sides that must survive: `cp $SRC $DST` (a whole-name variable) and `$SHELL -c 'ls -la'` (a wrapper around a read).
…1259) `check-merge-freshness.py` prices a stale verdict and, when refreshing would spend the votes a branch already has, tells the reader to record the landing-tree reading as a review - reviews are the only channel `check-vote-count.py` reads. The advice is right, and following it exactly still lost two votes on 2026-09-16: gh pr review 1255 --comment --body-file review1255.md # rc 0, prints nothing scripts/check-vote-count.py 1255 # 18:19:15Z VOID (no cycle id) - no cycle id in the vote body The counter reads the *voting cycle* out of the body, so a body without one is a vote nobody cast: it is excluded from the run and the PR reads short. Neither signal at the call site says so - `gh pr review` prints nothing on success, and the voiding is only visible to a reader who re-runs the counter afterwards. Both #1255 and #1258 were lost this way in one run. So the check is added on both sides, because the loss was silent on both sides: * `scripts/cast-vote.py` refuses to POST a body the counter cannot attribute (no cycle id, more than one, or a `--cycle` the body contradicts), and then reads the count back instead of assuming the POST worked. Exit 1 is reserved for the one state the caller cannot detect alone: posted, and not counted. It replaces no existing path - the counter remains the authority on the count. * the freshness remedy now names that helper and the body format it needs, since the remedy was the thing that told a reader to cast a vote with no cycle id. Verified: 15 tests for the helper, 8 mutations of the helper each caught (none survived), and the new remedy assertions are red when the remedy names the bare `gh pr review` command again. Full suite 2366 -> 2381 on master (+15). Agent.md gains the tool in its list, paid for by trimming two parentheticals to stay under the 8000-char prompt cap. Co-authored-by: EMRG Evolution <emrg@argszero.dev>
Closes the wrapper half and the path half of #1244; the cwd residual
listed at the end of this description stays open there.
The defect, measured end to end on master
7e7cd598Driven through
BashTool.executein the read-only tier, in a fresh git repowhose tracked file carries an uncommitted edit (
emrg/tools/bash_tool.pysha256[:16]
dfd4e85600643e28):sh -c 'git checkout .'(the named twin)$SHELL -c 'git checkout .'${SHELL} -c 'git checkout .'"$SHELL" -c 'git checkout .'env FOO=1 $SHELL -c 'git checkout .'The same hole swallowed a redirect:
$SHELL -c 'echo hi > OUT.txt'was ALLOW andthe file was created.
sudo $SHELL -c 'git checkout .'is ALLOW too (verdictonly — I did not run that one).
Mechanism: the wrapper walk recursed only into a token whose basename is a
shell's name.
$SHELL,${SHELL}and"$SHELL"are not — the basename of anassignment is its value, and the basename of a variable reference is itself.
What this changes
wrapper, so its payload is judged as code — mutators and write targets,
because a redirect inside a quoted payload is a character, not an operator.
The walk can only ever add blocking, so no verdict moves toward ALLOW.
before the "relative, therefore inside the workspace" shortcut. Measured on
master at workspace-write:
echo x > $HOME/.emrg/config.tomlALLOW — the verywrite the tier exists to refuse. It is now refused as the protected file it
is; a root nobody can resolve (
$UNKNOWN/repo/x.txt) fails closed; a bare$VARoperand, a relative name and$TMPDIR/...behave exactly as before,since blocking those would refuse
cp $SRC $DSTand the temp area the tierpromises.
Verification
_check_sandboxin both states: 8rows flip, every one of them intended; the other 77 are unchanged — reads
around the same wrapper, the named wrappers, the
/dev/nullexemption, and theworkspace-write boundaries.
tests/test_unresolved_wrapper_guard.py: 30 tests, 18 of which fail onunpatched master; the 12 that pass are the assertions about behaviour that
must not change.
bash_tool.pysha256[:16]
9bce12daa185cb9c.Still open (not this PR)
cd <outside>; echo x > out.txtandenv -C <outside> ...stay ALLOW atworkspace-write — a different mechanism (a virtual cwd), reported in read-only tier: a command reached through a variable is invisible to the guard (
$SHELL -c 'git checkout .'is ALLOWED and executes) #1244.git diff --output=$HOME/...needs the--output=recognition from emrg: four read-only fail-opens closed, and the guard stops refusing the read that explains a dirty tree (#1234, #1236, #1238, #1240) #1250;the expansion added here applies to that spelling automatically once it lands.