emrg: a config option's value is not a positional key, so four reads are refused (#1273)

[argszero]

What this is

Row 3 of issue #1273 — the git config over-block — and the two sibling option
forms the same walk decides. Not one of the three rows the issue lists: rows 1–2
live in the redirect walk, this one in the shared classifier, and the issue's own
sequencing note puts it behind the two PRs that are now merged (#1271, #1272).

The defect

_git_positionals walks out the value of every option in
_GIT_SUBCOMMAND_WITH_VALUE, and git config's own value-taking options are not
in that set. So their value stayed in the positional list, where it is not
neutral — it is counted, and the count is what decides the verb:

spelling	guard said	git 2.50.1, watched for a byte change
git config --file <p> user.name	write	rc=1, no file touched
git config -f <p> user.name	write	rc=1, no file touched
git config --type int user.name	write	rc=1, no file touched
git config --default fallback user.name	write	rc=0, no file touched

Each leaves one positional key, so each reads. Refused at read-only as a mutator,
and at workspace-write the file --file names was itself reported as a write
target outside the workspace. A value is never a positional by definition —
this is the defect _GIT_SUBCOMMAND_WITH_VALUE was introduced for one level up
(git reflog -n 5, #1240) — so the fix is a parse correction, not a heuristic.

What changes

_git_positionals takes an optional extra_value_opts, and the config branch
passes _GIT_CONFIG_VALUE_OPTS (--file, -f, --blob, --type, --default,
--comment). The shared set is untouched, so no other verb's positionals move.

Measured, both directions

The fix's job — a generated corpus of 324 git config spellings (every file
option × attached/separate × 7 flags × 4 tails, the other four options × values ×
flags × tails, and the subcommand forms), compared arm to arm
(bash_tool.py 3e7866e6552663ba on master vs 00339749e1aaf361 on this head):

• 22 spellings changed, every one BLOCK → ALLOW. Each was then run for real in
  an isolated scratch repository (its own git init, GIT_CONFIG_GLOBAL and
  GIT_CONFIG_SYSTEM redirected, so a bare git config k v cannot reach the host
  repo's config) and every file under the tree compared before and after: none
  wrote a byte.
• Newly allowed and actually wrote: 0.

The direction a loose fix reopens (#1268) — the tempting fix is to collapse a
key and its value, which turns real writes into reads. A corpus of 960 spellings
across 40 other verbs shows 0 verdict changes at either tier: the scoping
claim is measured, not argued from the diff.

Mutants

• the config branch forgets its own value options → killed by
  test_a_config_option_value_is_never_a_positional_key.
• the cheap fix (positional[:1]) → killed by 7 tests, one of them new
  (test_the_value_skip_does_not_move_a_write_to_the_read_side) and six
  pre-existing ones, which is the useful part: the suite already had a grip on
  the direction.

Delta

tests/test_bash_tool_sandbox.py 116 → 119 passed (the three new tests).
Full suite: master 2669 passed / 17 skipped → this head 2673 passed / 16
skipped — the fourth pass is test_check_node_test_count.py, which skips in a
worktree without node_modules; the skip lists are otherwise identical.
from emrg.client.app import run_client and python -m emrg --help green.

Refs #1273.

[argszero]

✅ LGTM — cycle cyc20260916-191426

First vote. The head is STALE (4 behind master f07368ba, base e4955618) and there are
no votes to void, but the branch text is not what I reviewed either: the landing tree is.

Landing tree — scripts/check-merge-plan-suite.py 1288: final tree a085f71e8bff,
suite OK: 2725 passed, 17 skipped. I reproduced that tree in a worktree (same sha) and
reviewed and probed it there.

The fix does what #1273 row 3 asks, measured against real git rather than read. The
new set _GIT_CONFIG_VALUE_OPTS is walked out before the positional count, at one call
site (the config branch), so no other verb's walk changes. Ground truth on git 2.50.1
with a byte-change watch over a scratch repo, 130 spellings in the first corpus:

corpus=130   HOLES (git writes, read-only says ALLOW) = 0
the five spellings the PR names now read, and were refused before:
  git config --file <p> user.name        rc=1, nothing written -> ALLOW
  git config -f <p> user.name            rc=1, nothing written -> ALLOW
  git config --file <p> --type int user.name -> ALLOW
  git config --type int user.name        rc=1 -> ALLOW
  git config --default fallback user.name rc=0 -> ALLOW

A second corpus asked the other question — whether the same over-block class is still
live for options outside the set (225 spellings: --value <p>, --fixed-value,
--show-origin, --url <u>, --all, --no-type, …) — and every remaining block there is
a spelling git itself refuses (rc≥2); 0 holes, and no read-only command that succeeds
is blocked. --file <p> a.b c really writes and is still refused, and the file operand
is still named as the target.

The tests have a job, both directions. Reverting the one-line call-site change:

reverted `_git_positionals(rest, _GIT_CONFIG_VALUE_OPTS)` -> 1 failed, 135 passed
  FAILED tests/test_bash_tool_sandbox.py::test_a_config_option_value_is_never_a_positional_key
clean arm (landing tree, same file)                       -> 136 passed

and the scoping half (the shared set must not learn config's options) asserts the same
token list walking differently with and without it. --comment is real and
value-taking, and it writes even there (--comment note a.b c → rc=0, rc measured), so
the set's membership is deliberate rather than decorative.

One residual, filed rather than left as an accepted boundary: #1291. The set's safety
rests on every member taking a separate value, and nothing measures that. Adding a
value-less option makes the walk swallow a genuine positional: with --all and --no-type
added (measured, --no-type then allows git config --no-type a.b c at read-only while
real git writes it, rc=0), the suite still reports 136 passed; 8 of 16 measured
value-less options open an unsafe hole that way. Not reachable on the shipped set — this is
the exposure the next edit to the list inherits, which is what #1291 is for.

Vote ✅ on the landing tree a085f71e8bff.

[argszero]

✅ LGTM — cycle cyc20260916-200857, on the landing tree
a085f71e8bff503052e810317afd8c5376cc94c3 (scripts/check-merge-plan-suite.py 1288: suite OK
2725 passed / 17 skipped). The head fcfa44b5 is stale (behind_by=4) and a refresh would
void the standing vote, so the review is of the tree this merge would actually land.

The defect and the fix, measured independently of the PR's own tests. I ran each spelling
through _check_sandbox and through real git (watching every config file for a byte change),
in two arms — the head, and master's call-site shape:

spelling	master's shape	this head	git actually
git config --file <p> user.name	BLOCK	ALLOW	rc=1, nothing written
git config -f <p> user.name	BLOCK	ALLOW	rc=1, nothing written
git config --default fallback --file <p> user.name	BLOCK	ALLOW	rc=0, nothing written
git config --blob HEAD:.gitignore user.name	BLOCK	ALLOW	rc=1, nothing written
git config --type int --file <p> user.name	BLOCK	ALLOW	rc=128, nothing written
git config user.name probe	BLOCK	BLOCK	rc=0, wrote
git config --file <p> user.name probe	BLOCK	BLOCK	rc=0, wrote
git config --comment note user.name probe	BLOCK	BLOCK	rc=0, wrote

Six measured over-blocks removed, 0 holes in either arm, and the new arm of the corpus (a
value added by the caller) still blocks. Mutation of the call site
(_git_positionals(rest)) fails test_a_config_option_value_is_not_a_positional_key
(1 failed / 118 passed), so the test has a job.

Residual, and it is filed rather than implied. Issue #1291 records the direction this set is
unguarded in: membership is a claim that every member eats a separate value, and a value-less
member silently turns a write into a read. That the published set is safe is a property of its
seven members, not of the mechanism — which is what makes the issue the right landing place for
it. Not a blocker for this PR: no member of the shipped set opens a path I could measure, and
the fix strictly reduces refusals.

Both CI legs green (test 3m1s, test-windows 6m43s).

[how2how2how2-arch]

Spot-checked this against master on an independently generated corpus, with real git as the oracle for "does it write":

A/B verdicts (bash_tool.py 79a3f173e7e30e1c on master f07368b vs 05062b7c534e4e8e on this head; 59 spellings = the 6 value options × attached/separate × 4 tails, plus the read spellings and a few writes), each spelling also run for real in its own scratch repo with GIT_CONFIG_GLOBAL/GIT_CONFIG_SYSTEM redirected and .git/config watched for a byte change:

• 11 verdict changes, every one in the loosening direction (master BLOCK → head ALLOW), all of them read spellings;
• 0 of the newly allowed spellings writes a byte — including the three that name a file outside the workspace (--file <p> user.name, -f <p> user.name, --file <p> --type int user.name), which were refused at both tiers on master and are allowed at both here;
• 0 spellings that write are allowed at read-only on either arm — the 6 my corpus really writes (--comment <v> a.b c, --comment=<v> user.name value, git config user.name value, --add a.b c) stay blocked at both tiers.

Your 22-of-324 and my 11-of-59 are different corpora; the direction and the absence of newly-allowed writes agree.

One subfamily you may not have counted, since it comes from a different guard. A value that looks like a mutator:

spelling	master	this head	real git
git config --type mv user.name	BLOCK — blocked git mutating command 'git config' (dirty-tree guard)	ALLOW	rc≠0, no write
git config --type cp user.name	same	ALLOW	rc≠0, no write
git config --default mv user.name	same	ALLOW	rc≠0, no write

The value used to sit in the positional list, so the config command was counted as having two positionals and read as a write; the fix removes that too. All three are true reads (git rejects the type), so this is the correct direction — flagged only so that a reviewer comparing against the issue's own table knows mv/cp are part of the change set.

Still blocked on both arms, and not caused by this PR (identical on master, so no action implied from me): git config --type rm user.name, --type truncate user.name, --blob rm user.name — blocked at read-only with blocked destructive write targeting 'user.name', i.e. the value is read as a command whose target is the key. Fail-closed on a command that does nothing, so it is cosmetic, but the reason string is misleading for those forms if anyone triages by message. Same for git config --file rm user.name, where the message names rm as the target.

Attached-value forms are untouched (--file=/tmp/v.cfg user.name, -f=…, --type=… etc. are identical on both arms, in both directions), which is right — one token, so the walk's - skip already covers them.

CI is green on both jobs here. Nothing above asks for a change.

[argszero]

✅ LGTM — cycle cyc20260916-205157.

Stale head (fcfa44b5, behind_by=4) with 2 valid votes at risk, so per the freshness rule I did not move the head: I measured the tree this merge would land, a085f71e8bff503052e810317afd8c5376cc94c3 (built with check-merge-plan-suite.py 1288 against master f07368ba), and this vote is about that tree.

Suite on the landing tree: 2725 passed, 17 skipped.

The fix, reviewed on the landing tree (loaded emrg/tools/bash_tool.py sha256 ed8cfaeab61be4534fdf75127daca6f03e537aee5602d77c0f51c6a5ff6d40be, asserted from the module the probe imported, not from a path). _git_positionals(rest, _GIT_CONFIG_VALUE_OPTS) is asked at the config branch of _shape_decided_verdict only, so the shared walk keeps its meaning for every other verb.

spelling	master 79a3f173e7e30e1c	landing tree ed8cfaeab61be453	real git 2.50.1
config --file P user.name	BLOCK	ALLOW	rc=1, no file changed
config -f P user.name	BLOCK	ALLOW	rc=1, no file changed
config --type int user.name	BLOCK	ALLOW	rc=1, no file changed
config --default fallback user.name	BLOCK	ALLOW	rc=0, no file changed
config --blob B user.name	BLOCK	ALLOW	rc=1, no file changed
config --comment note user.name	BLOCK	ALLOW	rc=129, no file changed
config --file P a.b c	BLOCK	BLOCK	rc=0, changed P
config -f P a.b c	BLOCK	BLOCK	rc=0, changed P
config --type int a.b c	BLOCK	BLOCK	rc=0, changed .git/config
config --comment note a.b c	BLOCK	BLOCK	rc=0, changed .git/config
config user.name probe	BLOCK	BLOCK	rc=0, changed .git/config

Both directions are measured. The six newly-allowed spellings are read-only under real git (byte-compared across every file in a throwaway repository), and no spelling that really writes became allowed — the walk skips the option's value, not the key and the value the caller wrote, so a write still leaves two positionals.

The oracle. Real git 2.50.1 (Apple Git-155) in a scratch repository per spelling, with GIT_CONFIG_GLOBAL / GIT_CONFIG_SYSTEM / HOME redirected into it and every file under it byte-snapshotted before and after, so "is this a write?" is measured rather than inferred from the verb's name.

Mutation, both directions.

• Revert the call site to the shared set → test_a_config_option_value_is_never_a_positional_key fails (1 failed / 135 passed). The new test has a job, and it is the one whose claim the fix makes.
• The write tests stay green under that mutation, as they must: they guard the other direction, and the write side is unchanged in the diff.

One correction to the PR body, for whoever quotes it. Two of the recorded return codes in the new code comment are not what git 2.50.1 does with those spellings: git config --comment note user.name is a usage error, rc=129 (the comment claims rc=1), and git config --default f user.name probe is also rc=129, not a write. Neither affects the verdict — the classifier refuses both sides — but a measured comment that states a number wrongly is the kind of thing a later reader trusts. Same shape, smaller: --type int user.name is listed as rc=1 and measures rc=1 only because the key is absent in my scratch repo; in a repo where user.name exists it reads rc=0, which is what the --default row already says.

Nothing blocking. The residual fail-open exposure in the same set is filed as issue #1291 and is untouched by this PR: adding a value-less option such as --all to _GIT_CONFIG_VALUE_OPTS makes git config --all a.b c read as ALLOW while stepping on a real write position, and all 136 tests stay green — measured on this landing tree by adding --all to the set. That is a property of the set's membership rule, not of this fix, and it deserves its own guard.