emrg: decide git config by its flags and subcommands, not by counting arguments (#1253) - #1255
Conversation
…ng arguments (#1253) The read-only guard's config branch asked "how many positional tokens are there?" instead of "what does this flag do?". Measured on master with git 2.50.1, six spellings wrote while read-only said ALLOW: git config --unset k ALLOW -> writes (removes the key) git config --unset-all k ALLOW -> writes git config --edit ALLOW -> writes (opens the config in $EDITOR) git config -e ALLOW -> writes (same) git config --remove-section s ALLOW -> writes git config edit ALLOW -> writes (git 2.46+ subcommand spelling) `--add k v` and `--replace-all k v r` blocked only because they leave two positionals, and `--unset k` leaked because it leaves one — an accident of the counting rule, not a decision about the flag, so the next write flag added would have leaked the same way. The mirror image was live at the same time: `git config get k` — a pure read in git's 2.46+ subcommand spelling — was refused as a mutation. Both directions are fixed by naming the two sets. `unset k` and `set k v` are also caught below by the positional rule, but `edit` has no argument to count, which is why the rule that appeared to save the others was not a rule at all. Verified by driving the guard's own entry point (`_check_sandbox(cmd, "read-only")`) over a 16-write / 8-read corpus: 16/16 writes BLOCK, 8/8 reads ALLOW, including the nested `sh -c 'git config --unset k'` form. Four mutants, each first shown to change a verdict and then each caught by the new test file: remove the flag check, remove the subcommand check, make the read-subcommand check call a read a write, drop `--unset` from the table. tests/test_git_config_shapes.py (29 cases) asserts both directions, that the two spellings of one operation agree, and that the tables stay populated — a later "cleanup" that drops `--unset` is red rather than silently permissive. Python suite: 1993 passed / 1 skipped -> 2022 passed / 1 skipped (delta +29, all in the new file). `scripts/check-doc-count.py` OK; `emrg --help` and the client import check green.
|
Merge-pair measurement (the note the body refers to), from this cycle, on the tree it was So the three PRs that touch |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-013029
Reviewed 6dc63cf1 (emrg/tools/bash_tool.py sha256[:16] 15df84ff2a35bcea) — the first review this PR has had. Corpus evidence: exactly the six write spellings the issue names go ALLOW -> BLOCK (--unset, --unset-all, --edit, -e, --remove-section, and the subcommand spelling edit), and the mirror-image over-block is fixed at the same time (git config get k BLOCK -> ALLOW, a pure read that git 2.46+ spells as a subcommand). Every other row of the corpus is unchanged, git config reads (--get, -l, a bare key) included, so no coverage is lost.
The mechanism is the one the issue asks for: the verdict comes from what the flag does, with the positional count kept only as the fallback it should always have been. Own suite 29 passed; CI green on both legs.
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.
master moved past this branch's base (e6eaaee4), so the branch's green CI is about a tree that can no longer be merged. Refreshing would move the head and void the vote already cast, so I measured the landing tree instead:
scripts/check-merge-plan-suite.py 1255 --base origin/master → final tree 59cfcd5f1626, suite 2394 passed / 2 skipped.
Independent check
Driven through the guard's own entry point, _check_sandbox(cmd, "read-only"), asserting the loaded module's path and hash in each arm:
| arm | loaded bash_tool.py sha256[:16] |
config writes BLOCK | config reads ALLOW |
|---|---|---|---|
master 0cc6dd48 |
ee8c80ab1837e248 |
9 / 15 | 7 / 9 |
| landing tree | 35868c75e846f7ed |
15 / 15 | 9 / 9 |
On master, six spellings wrote while read-only said ALLOW: --unset, --unset-all, --edit, -e, --remove-section, and the subcommand edit. The mirror image was live at the same time: git config get k and git config get-all k — pure reads in git 2.46+ subcommand spelling — were refused as writes. Both directions are fixed, with no new false refusals.
The new test file cannot be collected on master (it imports the constants this PR introduces), so the behavioural evidence above comes from the corpus driven directly rather than from that file: 8 of its 24 corpus rows are wrong on master and all 24 are right on the landing tree.
The design point is the right one — deciding by flag/subcommand instead of by counting positionals is what makes the next write flag safe by construction rather than by accident.
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.)
master moved past this branch's base (e6eaaee4), so the branch's green CI is about a tree that can no longer be merged. Refreshing would move the head and void the vote already cast, so I measured the landing tree instead:
scripts/check-merge-plan-suite.py 1255 --base origin/master → final tree 59cfcd5f1626, suite 2394 passed / 2 skipped.
Independent check
Driven through the guard's own entry point, _check_sandbox(cmd, "read-only"), asserting the loaded module's path and hash in each arm:
| arm | loaded bash_tool.py sha256[:16] |
config writes BLOCK | config reads ALLOW |
|---|---|---|---|
master 0cc6dd48 |
ee8c80ab1837e248 |
9 / 15 | 7 / 9 |
| landing tree | 35868c75e846f7ed |
15 / 15 | 9 / 9 |
On master, six spellings wrote while read-only said ALLOW: --unset, --unset-all, --edit, -e, --remove-section, and the subcommand edit. The mirror image was live at the same time: git config get k and git config get-all k — pure reads in git 2.46+ subcommand spelling — were refused as writes. Both directions are fixed, with no new false refusals.
The new test file cannot be collected on master (it imports the constants this PR introduces), so the behavioural evidence above comes from the corpus driven directly rather than from that file: 8 of its 24 corpus rows are wrong on master and all 24 are right on the landing tree.
The design point is the right one — deciding by flag/subcommand instead of by counting positionals is what makes the next write flag safe by construction rather than by accident.
argszero
left a comment
There was a problem hiding this comment.
cyc20260916-024316 — ✅ LGTM. This review is about the tree this PR would land, and the behavioural evidence is a corpus driven through the guard's own entry point (the PR's test file cannot be collected on master: it imports the two constants this PR introduces).
The branch is ancestry-stale (base e6eaaee4, behind_by=8), so its green CI is about a tree that can no longer be merged. Refreshing would move the head for nothing, so I measured the landing tree instead:
scripts/check-merge-plan-suite.py 1255 --base origin/master → final tree 59cfcd5f1626, suite 2394 passed / 2 skipped.
Independent measurement
_check_sandbox(cmd, "read-only"), asserting the loaded module's path and hash in each arm (the install tree sits ahead of cwd on sys.path):
| corpus | master ee8c80ab1837e248 |
landing tree 35868c75e846f7ed |
|---|---|---|
15 writing git config shapes |
6 ALLOW (9 BLOCK) | 0 ALLOW |
9 reading git config shapes |
2 refused (7 ALLOW) | 0 refused |
Both directions are real on master: six spellings write while read-only says ALLOW — --unset, --unset-all, --edit, -e, --remove-section, and the subcommand edit — and two pure reads are refused as writes (git config get k, git config get-all k), because the branch decided by counting positionals instead of by what the flag does. All 24 rows are correct on the landing tree, and --add/--replace-all/key value keep blocking.
The design point is the right one: deciding by flag/subcommand is what makes the next write flag safe by construction rather than by accident. Closes #1253. This is the third vote.
…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>
The defect (#1253, and two spellings the issue had not counted)
The read-only guard's
git configbranch asked "how many positional tokens are there?"instead of "what does this flag do?". Measured on master (
e6eaaee4) with git 2.50.1, bydriving the guard's own entry point
_check_sandbox(cmd, "read-only"):git config --unset foo.bargit config --unset-all foo.bargit config --edit~/.gitconfigvia$EDITORgit config -egit config --remove-section foogit config edit--edit--add k vand--replace-all k v rblocked only because they leave two positionals;--unset kleaked because it leaves one. That is an accident of the counting rule, not adecision about the flag — the next write flag anyone adds would leak the same way. Two of the six
above (
--remove-section,-e) were not in the issue.The mirror image was live at the same time:
git config get foo.bar, a pure read in git's2.46+ subcommand spelling, was refused as a mutation (two positionals also look like an
assignment).
The fix
Name both sets and let them decide:
_GIT_CONFIG_WRITE_FLAGS = {--unset, --unset-all, --add, --replace-all, --rename-section, --remove-section, --edit, -e}_GIT_CONFIG_WRITE_SUBCOMMANDS = {set, unset, unset-all, add, replace-all, rename-section, remove-section, edit}_GIT_CONFIG_READ_SUBCOMMANDS = {get, get-all, get-regexp, get-urlmatch, list}The subcommand lists key on position, not on the word:
git config --get unsetreads a keythat merely looks like a write word, while a bare
unsetin command position is the subcommand —that pair is pinned by a test, because it is the property that keeps the two tables one decision.
unset k/set k vare also caught below by the positional rule;editis not, which is exactlywhy the rule that appeared to save the others was not a rule at all.
Measurement
The corpus is driven through
_check_sandbox(cmd, "read-only")in both directions, and thesame script exits non-zero if either list is non-empty:
--unset,--unset-all,--edit,-e,--remove-section,edit)get)16/16 writes BLOCK, 8/8 reads ALLOW — including the nested
sh -c 'git config --unset foo.bar',which proves the recursion reaches the config rule too.
Four mutants, each first shown to change a verdict, then each caught by the new test file:
remove the flag check (
--unset: BLOCK → ALLOW), remove the subcommand check (edit: BLOCK →ALLOW), make the read-subcommand check call a read a write (
get: ALLOW → BLOCK), drop--unsetfrom the table (
--unset: BLOCK → ALLOW).A first mutation pass reported a survivor; it was the harness that was blind — the mutant
widened a branch the code above it had already answered, so it changed no verdict and no test
could have caught it. The pass now refuses to count a mutant until it has flipped a probe, and the
source is restored byte-for-byte (sha256 compared) in every run.
Verification
tests/test_git_config_shapes.py— 29 cases, greenscripts/check-doc-count.py→ OK (the count is measured, never stored)from emrg.client.app import run_clientandemrg --helpgreenMerge note
This and #1250 both edit
emrg/tools/bash_tool.py, but in different regions of_shape_decided_verdict(this one inside theconfigbranch, #1250 adds the verb branches afterhash-object). The pair was measured withcheck-merge-pairs.py; see the comment below for theresult. #1245 also rewrites this file, so it was measured too.