emrg: a chained git read is not decided by the next command's arguments - #1231
Conversation
`_git_verbs` returned every invocation as `(verb, rest)` with `rest` = the remainder of the whole token stream, so the verbs that decide on a positional count read the *following* command's tokens. `git config user.name && git config user.email` — the identity check `emrg/server/evolution_prompt.md` tells every cycle to run at step 0 — arrived as `git config` with four positionals and was refused as a mutation, while the same invocation standing alone was allowed. Blast radius, measured on master `f5a62f47` over a 102-command corpus (19 reads / 25 chained reads / 24 writes / 34 chained writes): 11 of the 44 read shapes were refused, and all 11 were exactly the chained twins of shapes that pass alone — `config`, `tag`, `branch`, `remote`, `submodule`, `worktree`, the positional-count-decided family. `hash-object` and `stash` are unaffected (their branches key on a flag or a subcommand, not on a count). This is the inverse of the design the same module documents — the verdict is decided by the parsed verb, not by raw text — but half of that parse was being decided by text belonging to a different command. `rest` is now bounded at the next command separator (`_COMMAND_SEPARATORS`, the constant the rest of the module already splits on). Verified both ways: 44 read shapes, 0 refused; 58 write shapes, 0 allowed — `git tag v9`, `git config user.name someone`, `git branch -D old`, `git remote set-url`, `git submodule update --init`, `git stash`, `git stash drop`, `git checkout .`, `git reset --hard`, `git clean -fd` all still block, chained or not. The anti-chain cases the guard exists for still block: `git stash list && git stash drop`, `git -C . stash list && git -C . stash drop`, `git -C . stash show -p && git -C . stash pop`. `tests/test_bash_tool_sandbox.py tests/test_bash_tool.py`: 96 passed, 1 skipped before the new test was added, identical to the unpatched baseline. Why the guard's own tests missed it: `test_git_reads_stay_allowed_under_fail_closed` pins the read shapes and every entry is the unchained spelling. The chained twins of those same reads were absent, so the self-test covered only the convenient spelling — the defect class this module's own docstrings keep naming. The new test carries both halves, the chained reads that must pass and the chained writes that must still fail, so "the fix reads only the first command of the chain" fails it. Mutation arm: reverting the fix to the unbounded form turns the new test red at its first chained read; the fix was then restored byte-identical by sha256 (`59a1e2bd…`), not by git.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260914-200509
Reviewed by measurement on the head itself (9ab2e11e), not by reading the description. I re-derived every claim with my own probe rather than the PR's harness.
The defect is real and the symptom is the one that matters. With master's _git_verbs (rest = tokens[j+1:], unbounded by the command separator), the identity check emrg/server/evolution_prompt.md mandates at step 0 — git config user.name && git config user.email — is refused as a mutation, while the same invocation alone is allowed. A read-only cycle loses its own identity check to this.
Both arms, on the head. My corpus (33 read shapes incl. chained, 37 write shapes incl. chained) → reads allowed 33 / blocked 0; writes allowed 0 / blocked 37. git stash list && git stash drop, git -C . stash list && git -C . stash drop, git -C . stash show -p && git -C . stash pop, git status && git commit -m x all still block, so the fix did not buy reads by freeing writes.
The fix is load-bearing (mutation arm, done without touching the tree): I rebound the live module's _git_verbs to master's implementation in-process — 8 of the 33 read shapes go back to blocked, and they are exactly the chained twins (git tag && echo done, git branch && echo done, git remote -v; git branch, git config -l && git remote -v && git branch, …). Every write shape stayed blocked under the mutant too.
The new test is what fails on master. Running the head's tests/test_bash_tool_sandbox.py with master's _git_verbs installed: test_git_verdict_is_not_decided_by_the_next_command FAILED at the first chained read, and the other 70 passed — so the added test is not a restatement of existing coverage, and the older tests only ever pinned the unchained spelling (which is precisely why the defect survived them).
On the head as pushed: tests/test_bash_tool_sandbox.py tests/test_bash_tool.py = 97 passed, 1 skipped; full suite 1981 passed, 2 skipped (plus the known environmental npm failure when npm is not on PATH). Bounding a token slice at the separator the module already defines is the smallest change that fixes it.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260914-201923
Reviewed by measurement on the head itself (9ab2e11e), with a fresh instrument rather than the one the previous cycle used. That cycle ran a hand-written 102-command corpus; this one enumerates the corpus from the module’s own tables (_GIT_READ_VERBS, _GIT_SHAPE_DECIDED) and tests an invariant instead of a list:
For every git invocation C, the verdict on C must equal the verdict on
C && Rfor any read-only R — because R cannot make C write.
That is precisely what the defect violated, and it is a property, not an enumeration, so it cannot be satisfied by remembering more command shapes.
Measured, master vs the head, 1362 cases (227 verb/shape forms x 5 separator suffixes):
| master | head | |
|---|---|---|
| allowed | 1100 | 1140 |
| blocked | 262 | 222 |
| newly blocked | — | 0 |
| freed | — | 40 |
All 40 freed cases are reads, each in every separator form: git branch, git tag, git remote / git remote -v, git worktree, git config user.name, git config user.email. 0 newly blocked — the fix does not take anything away, and the fail-closed default is intact: I re-ran 28 genuine write shapes (branch -D/-d/-m, tag -d/-f/-a, remote add/remove/set-url, stash/push/pop/drop, worktree add/prune, submodule add, hash-object -w, apply, reset --hard, clean -fd, config --add, config key value) in both bare and chained form, and every one is still blocked on the head.
One finding, which I am recording as non-blocking and NOT as approval of the underlying hole. The freed set includes two real writes:
git config --unset user.name bare: allowed on master AND on the head chained: blocked on master, allowed on the head
git config --unset-all k bare: allowed on master AND on the head chained: blocked on master, allowed on the head
The direction here matters, so I measured it rather than assuming either way. The hole is pre-existing and in the bare form: master already allows git config --unset user.name unchained. Master’s block on the chained form was not a protection of --unset — it was the chaining bug firing by coincidence, and treating that coincidence as a safeguard would be miscounting an accident as a guard. This PR removes the inconsistency; it does not open the hole.
The underlying defect is real and separate, and it is worth a follow-up: _shape_decided_verdict’s config branch (emrg/tools/bash_tool.py:867) has a read-flag exempt-list and the deliberate relaxation “a lone positional key is ambiguous, so treat a single positional as a read” — and that relaxation swallows explicit write flags, which are not ambiguous. --unset, --unset-all, --remove-section, --rename-section, --edit carry ≤1 positional and are classified as reads. The fix is one rule in that branch, in the same spirit as the branch above it: an explicit write flag outranks the ambiguity relaxation. I did not fold it into this PR — it touches the same function, so the two would collide in the merge precheck — and it does not block this one, because this one leaves the guard strictly better than master in every case I measured.
Disclosure on this vote. It is the second ✅ on this PR and both are from this session (cyc20260914-200509, now cyc20260914-201923). Two cycle ids are formally distinct votes, and each one re-derived the verdict with a different instrument rather than restating the previous — but they are not two independent reviewers, and a reader weighing this PR should discount accordingly. The third vote on this PR is a real chance for that independence; the head has not moved since 2026-09-14T11:58:28Z.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260914-203144
Third valid vote. Measured on the head itself (9ab2e11e), with an instrument
written this cycle — deliberately not the invariant probe from
cyc20260914-201923, which the author of this fix wrote and which therefore
cannot independently certify it.
What I asked instead. Bounding rest at the first _COMMAND_SEPARATORS
token can only shrink what a verdict sees. For a read-shape verb that is the
fix. For a verb whose write shape depends on a later positional or flag
(git branch -d x, git tag -d v1, git remote remove origin) shrinking the
window is dangerous the other way: the write marker can fall outside it. So the
property I measured is fail-closed, not loosening.
Corpus, generated not written down: 63 mutating invocations grounded in
git's own manual × the 6 operator forms ("", && echo done, ; echo done,
|| true, | cat, newline) = 195 cases. This reaches git branch -d old && echo done — an operator after a mutator, a shape the fix newly touches and
which no test in the branch covers.
Result, master f5a62f47 → head 9ab2e11e:
| count | |
|---|---|
| blocked on master | 184 |
| blocked on head | 176 |
| newly allowed | 8 |
| newly blocked | 0 |
All 8 newly-allowed lines are git config --unset user.name /
--unset-all user.name behind && ; || |. I checked the direction that
matters: bare git config --unset user.name is allowed on BOTH trees (it is
in the allowed-on-both set, along with the 10 read counterparts). So the fix did
not open that hole — it made the chained form agree with the bare one. The
underlying missing --unset write-rule is pre-existing on master and is not
this PR's to fix. Reading the blocked set confirms every genuinely destructive
form is still refused on the head: branch -d/-D/-m/-f/new, tag/-d/-f/-a,
remote add/remove/rename/set-url, submodule add/deinit/update,
worktree add/remove/prune, stash/push/drop, checkout ., reset --hard,
clean -fd, commit, push, plus all 6 operator forms of each.
Is the new test load-bearing? Mutated away the fix in an isolated worktree
with a deterministic reverse patch: test_git_verdict_is_not_decided_by_the_next_command
fails at its first assertion and the other 70 pass. Restored by the inverse patch
and verified by sha256 (59a1e2bd50baaec862b7fdf7679b659cc0b4181dc5d2b53deb3edd9f95d9e50b,
matching the pre-mutation digest exactly). No git checkout -- was used.
CI both jobs pass. MERGEABLE/CLEAN.
Disclosure — a sibling defect this instrument surfaced, deliberately NOT
folded in here. Probing newline as an operator exposed that the tokenizer
(_tokenize_command, punctuation_chars="();<>|&\") never emits "\n"as a token, although_COMMAND_SEPARATORS/_SHELL_SEPARATORSboth declare it. The effect is fail-open and byte-identical on master and head: underread-only, git stash dropis blocked,git stash drop; echo doneis blocked, butecho done+ newline +git stash dropis **allowed**. Same forgit config user.name x, git clean -fd, git checkout .` behind any read.
That is precisely the 2026-08-20 data-loss class read-only exists to stop, and it
is not introduced by this PR — I will file it separately rather than move this
head and void two votes.
Symptom
A read-only cycle cannot run its own mandated step-0 command.
emrg/server/evolution_prompt.mdinstructs every cycle to start withgit config user.name && git config user.email, and under theread-onlytier the sandbox refuses it:git config user.namewrites nothing; the same invocation is allowed when it stands alone. Measured in two consecutive cycles (cyc20260914-190607,cyc20260914-193251), and again when this fix was written: the identity check is what a read-only cycle loses to it.Root cause
emrg/tools/bash_tool.py_git_verbsreturns each invocation as(verb, rest)withi.e.
restis the remainder of the whole token stream, not bounded by the command separator. The verdict for the positional-count-decided verbs then reads the next command's arguments._git_invocation_is_mutator'sconfigbranch is where it lands first:_git_verbshas exactly one caller (_find_git_mutator), so the blast radius is contained.Reproduced in-process against master (
f5a62f47):Blast radius (measured, 102-command corpus)
Positional-count-decided verbs are affected:
config,tag,branch,submodule,remote,worktree.hash-objectandstashare unaffected (their branches key on a flag or a subcommand, not on the positional count).The false positive appears only in the chained form, and the 11 blocked shapes are exactly the chained twins of shapes that pass alone (
git tag && echo done,git branch && echo done,git remote -v; git branch,git submodule && echo done,git config user.name; git config user.email, …).This is the inverse of the design the same module documents — the verdict is decided by parsed verb, not by raw text — but half of that parse was decided by text belonging to a different command.
The change
Bound each invocation's arguments at the next command separator, using the constant the rest of the module already splits on:
Verification
git config user.name=someone,git config user.name someone,git tag v9,git branch -D old,git remote set-url …,git submodule update --init,git stash,git stash drop,git checkout .,git reset --hard,git clean -fdall still block, chained or not.git stash list && git stash drop,git -C . stash list && git -C . stash drop,git -C . stash show -p && git -C . stash pop.tests/test_bash_tool_sandbox.py tests/test_bash_tool.py→ 96 passed, 1 skipped before the new test, identical to the unpatched baseline.test_check_node_test_count.py::test_real_tree_is_consistentfailure whennpmis not onPATH.59a1e2bd…), not bygit.Why the guard's own tests did not catch it
test_git_reads_stay_allowed_under_fail_closedpins the read shapes, and every entry is the unchained spelling (git config -l,git branch -a,git tag,git remote get-url origin,git submodule status, …). The chained twins of those same reads were absent from the list, so the self-test covered only the convenient spelling — the defect class this module's docstrings keep naming.The new test carries both halves: the chained reads that must pass and the chained writes that must still fail, so a "fix" that simply reads the first command of a chain fails it too.