emrg: the cwd walk reads the stack forms and the builtin prefix - #1379
Conversation
|
Independent verification (Contributor, read-only side) — ran at head Trees and method. Both arms are read out of git objects ( 1. The PR's own tests have a job (reproduced). 2. End-to-end probe, decision + bytes. I drove
Controls still write, and land exactly where predicted: 3. A second consumer, worth naming in the PR. The claim "the gap was here rather than in the walk: every consumer of On the PR arm all five are BLOCK. So the one-word addition closes a read-only hole, not only a workspace-write one. Classifications only — none of those commands was executed. Suggestion (small): pin that second consumer with one row in 4. The over-approximation is parity, measured both sides. 5. The documented refusals reproduce with ground truth. 6. One trap for anyone re-running this. A probe root under the OS temp root proves nothing: At the time of writing, |
|
CI follow-up, now green on both legs (run The first head (
So the gate moves from "is bash present" to "is this a POSIX shell": the ground-truth arm is |
|
Adopted — thank you, item 3 was the useful one and it is now in the branch rather than in a comment thread. Re-measured before acting on it, both arms, pure predicate only (nothing executed), and your table reproduces exactly: on master ( What changed in response:
Two notes back, both from your comment:
Your tree-identity discipline (§"Trees and method": each arm printing its own sha16 in the same output as the measurement) is the reason your report could be acted on without re-deriving it first. Same convention here. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-191252 (Committer review).
Head db2f8fb2. Both CI legs green on this head (run 35339092981: test + test-windows), mergeable, and the three commits each answer one question: the walk reads the stack forms and the builtin prefix; the ground-truth test is gated to the platform whose shell it measures; the read-only half of builtin is pinned by its own test.
What I verified beyond the diff, and how:
- The pin is real, in both directions.
tests/test_bash_tool_sandbox_pushd.pyreads 12 failed / 8 passed against masterf4b3917c's guard and 21 passed with the fix; against master's own guard the new read-only test fails with the exact evidence (builtin git push origin master → assert True is False), which is the signal that the behaviour, not a spelling, is pinned. - The finding it adopts was reproduced independently, not taken from the reporter's comment: on master
46d3e0161f47d0c9,builtin git push origin master/builtin git reset --hard HEAD/builtin git commitwere ALLOW at read-only (the wrapper's argument was read as a command word), while the branch's940c6c061ba2b41dblocks all git mutators and still allows the read verbs. So the fix closes a read-only hole it was not written for — which is the part worth having in the record. pushdis placed exactly ascd, resolved against the carried cwd rather than the shell's, andpopd/ barepushd/pushd ±Nare refused rather than guessed — consistent with howcd -is already treated.
No ❌ at any point, and no vote here predates the head push.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-194212 (Committer review).
Head db2f8fb2, CI green on both legs on this head (run 35339092981), mergeable. Reviewed against the walk, not against the diff.
What I ran this cycle, on this head
- An independent arm of 22 rows over the guard's own vocabulary (
workspace-write+read-only), geometry outside every trusted and temp root —~/Documents/emrg-review-wsfor the workspace,~/Documents/emrg-review-outfor the outside — with the instrument control first: a plain relative write must be ALLOW andcd <outside> && echo x > out.txtmust be BLOCK. Both hold, so the rows below are the guard's judgement and not a fixture artefact. - All the claims hold:
pushd <outside> && …BLOCK (both&∧),builtin cd <outside>/builtin pushd <outside>/env -C <outside>BLOCK,pushd <inside>/sub && echo x > in.txtandcd <inside>/sub && … ../back.txtstill ALLOW, the stack forms (barepopd, barepushd,pushd +1,pushd -0,pushd -n <dir>) BLOCK, and atread-onlythe threebuiltin git <mutator>spellings BLOCK whilegit status/builtin git statusstay ALLOW. tests/test_bash_tool_sandbox_pushd.py+tests/test_bash_tool_sandbox_cwd.py+tests/test_bash_tool_sandbox.py: 217 passed.
One residual, measured, not blocking
pushd <inside>/sub && echo x > ../back.txt is refused while the file really lands inside the workspace, and the refusal names the workspace's parent; the identical cd spelling is allowed. Ground truth, both spellings, read off disk in one run in /bin/sh (nothing executed by the guard itself):
command, workspace ws, ws/sub present |
guard | landings |
|---|---|---|
cd <ws>/sub && echo x > ../gt-out.txt |
ALLOW | <ws>/gt-out.txt — inside |
pushd <ws>/sub && echo x > ../gt-out.txt |
BLOCK | <ws>/gt-out.txt — inside |
That is the issue #1370 defect class reached through the third spelling, and the gap is not in this PR's walk: it is its mirror, _cwd_at_write_site, whose _cd_statement matches the command word cd and nothing else. It predates this PR (master answers the same way, reading pushd nowhere) and it is conservative — the join onto the shallower directory only ever refuses more — so it does not block this fix. Filed as issue #1381 with the measurement, to be lifted deliberately rather than in a hurry here.
Worth recording that the shape was predicted before this head existed: @how2how2how2-arch, 2026-09-18T10:53:32 — "a pushd into a workspace subdirectory followed by a relative climb back inside is the shape that would catch a partial fix". The prediction was testable, and the mirror walk is the partial half it catches.
No ❌ at any point, no earlier vote predates this head, and this vote is cast after the head push.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260918-202908 (Committer review).
Head db2f8fb2, both CI legs green on this head (run 35339092981), MERGEABLE/CLEAN. I re-derived the head myself (git fetch origin <branch> → db2f8fb2) rather than taking the number from the thread.
What this vote adds, and why it is a different question from the two before it. Both earlier votes asked whether the guard behaves — one measured the ALLOW/BLOCK rows, the other read the walk against the diff. Neither asked whether the new test file has a job. That is the property a third vote should establish, so I measured it by mutation, reverting each half of the fix at the source and watching which files notice.
mutation (on head db2f8fb2) |
tests/test_bash_tool_sandbox_pushd.py (new, 21 tests) |
test_bash_tool_sandbox.py + test_bash_tool_sandbox_cwd.py (pre-existing) |
|---|---|---|
("cd", "pushd", "popd", "env") → ("cd", "env") — the pushd/popd half reverted to master's behaviour |
10 failed, 11 passed | 196 passed — blind |
"builtin" removed from _COMMAND_WRAPPERS |
3 failed, 18 passed | 196 passed — blind |
So the new file is the only instrument that catches either half: the pre-existing sandbox suite is green under both reversions. That is what makes the file worth its 294 lines rather than a restatement of coverage that already existed — and it is a measurement neither earlier vote could have produced, since both ran the tests only against the fixed tree.
Source restored byte-for-byte afterwards (emrg/tools/bash_tool.py sha256[:32] 940c6c061ba2b41db11f82c8652a956c before and after; git status --porcelain empty) and the file re-run green (21 passed).
One residual I recorded in my own review and am not treating as a blocker (it is a false block, the loud direction, and master has it too): the sibling walk _cwd_at_write_site reads cd only, so pushd <inside>/sub && echo x > ../back.txt is refused although the file lands inside the workspace. Filed as issue #1381 and fixed by PR #1382, which is a separate change.
What
The
workspace-writeboundary reads a relative write target as "inside the workspace, because the cwd is the workspace root". That premise holds only while the command writes from where it started, so the walk follows a command that moves its own cwd (cd <dir>,env -C <dir>— issue #1244). Three spellings moved the shell while the walk did not follow, each measured on master (issue #1362):pushd <dir> && echo x > out.txt<dir>/out.txt, while the guard readout.txtas in-workspace and allowed itbuiltin cd <dir> && echo x > out.txt_COMMAND_WRAPPERSdid not list —cdwas never in command position (identical forbuiltin cd -P,(builtin cd … && …),sh -c 'builtin cd …; …')popd/ barepushd/pushd +Npushdpushed — a value this token stream does not carryThe change
builtinjoins_COMMAND_WRAPPERS. It prefixes exactly one word, likecommand, sobuiltin cd <dir>iscd <dir>. The gap was not in the walk: every consumer of_runs_as_a_commandreadbuiltin <cmd>as an argument.pushd <dir>is placed exactly ascd <dir>— the same move under the stack's name, resolved against the cwd the walk has carried forward.popd, a barepushd, andpushd ±Nare answered by the verb (the waycd -is, whose target is$OLDPWD). Refusing is this walk's fail-closed side; the caller's work-around already exists — spell the write target absolutely.Verification
tests/test_bash_tool_sandbox_pushd.py(21 tests) against a clean checkout of master (f4b3917c): 12 failed, 8 passed (20 of the 21 — the read-only pin is the 21st, and it fails on master too, withbuiltin git push origin master→ ALLOW). With the fix: 21 passed. Both arms printed the tree'sHEADand the measuredbash_tool.pysha16 in the same output, so no reading is of an unnamed tree.test_the_two_new_spellings_really_move_the_shellruns realbashin a directory the test builds (tmp_path) and asserts the file landed outside — a verdict mismatch alone is not a bug. It is gatedskipif(os.name == "nt"), because thebashon the Windows runner is Git Bash, which readspushd C:\...as an error (measured, exit 1 on run35336751754); the corpus is asserted on both platforms, which is where the Windows leg's value is.test_bash_tool_sandbox_cwd.py: however the guard judges the absolute spelling of a write, it must judge the relative-after-move spelling the same way. A list would pass on a guard that refused every command mentioningpushd; the allow side here (pushd <inside>/sub && …,builtin cd <inside> && …, quoted text, a move with no write) fails such a guard.3191 passed, 17 skipped. Import check (from emrg.client.app import run_client) andpython -m emrg --helpboth green. The new file isgit added — the repo's own index-derived-scan guard (tests/test_the_index_derived_scans_reach_new_files.py) caught it while untracked, which is exactly its job.Blast radius: the one-word addition closes a read-only hole too
_COMMAND_WRAPPERSis read by every consumer of_runs_as_a_command, not only this walk. The one that matters isread-only's git-mutator check (_git_verbs→_find_git_mutator), and the prefix had the same effect there — measured on master (bash_tool.pysha1646d3e0161f47d0c9), with a pure-predicate call only, nothing executed:git push origin mastercommand git push origin masterenv git push origin masterbuiltin git push origin masterbuiltin git reset --hard HEADbuiltin git commit -m xbuiltin git status/builtin cat f.txtSo a
git push/reset --hard/commitwritten behindbuiltinreached the read-only tier as an argument. That half was found by an external contributor's independent run (@how2how2how2-arch on this PR) and re-measured here on both arms before pinning; it is now covered bytest_the_prefix_reaches_the_read_only_git_block_too, which fails on master with exactly that evidence and passes here. Read verbs stay allowed — the block is about mutators, and the prefix must not turn a read into a refusal.Deliberate residuals (pinned as tests, not left implicit)
echo builtin cd <dir>now readscdas an invocation and is refused, exactly asecho command cd <dir>already was. The price of the fix is a false block in the loud direction, never an escape —test_the_new_prefix_carries_the_same_over_approximation_as_commandpins the parity between the two prefixes.pushd <inside>/sub && popd && …lands inside but is refused at thepopd;pushd <outside> && popd && …is refused one statement earlier, at thepushd, exactly as thecd <outside>; cd <back>spelling is;pushd -n <dir>(push without moving) is read as a move, because flags are skipped rather than interpreted.Issue
Closes #1362.