emrg: four read-only fail-opens closed, and the guard stops refusing the read that explains a dirty tree (#1234, #1236, #1238, #1240) - #1250
Conversation
…the read that explains a dirty tree (#1234, #1236, #1238, #1240) One patch because all four live in the same tokenizer: landing them separately would make each a rebase for the others. #1234 - the destructive-write rule could not see inside a quoted `sh -c` body. The git-mutator rule recursed; the destructive-write rule did not, so `sh -c 'rm -rf /tmp/a'` was ALLOWED at read-only while the bare spelling was blocked. _extract_write_targets now walks _nested_command_texts with the same depth-3 bound, so both rules judge the same text. #1236 - a heredoc body that is DATA was judged as shell code: a document merely mentioning `git checkout .` could not be written, and prose was named as a write target, while the identical text as a quoted argument was allowed. _mask_data_heredoc_bodies masks such bodies under four conditions that all fail closed (named data reader, no pipe, terminator exists, no wrapper/evaluator outside the bodies). Masking, not deleting, keeps the line structure the mutator scan depends on. #1238 - a reader's write flag was invisible: `--output`/`--output-file` on the diff-family readers, `interpret-trailers --in-place`, `mailinfo`, `mailsplit`, `diagnose`, `credential approve|reject` all wrote while read-only said ALLOW. The option-spelled redirect is now matched as one inside a git invocation; interpret-trailers and credential became shape-decided; mailinfo, mailsplit and diagnose left the read allowlist. #1240 - four pure-read verbs were refused as "mutating", because the read list is an allowlist. `cherry` joins it (no writing subcommand exists). reflog, notes and bisect are reads in their reporting shape and writers in another, so they are shape-decided and fail closed on any unlisted subcommand. The root cause was the option splitter: a mandatory-value flag made its value occupy the subcommand slot (`git reflog -n 5` read `5` as the subcommand), so _GIT_SUBCOMMAND_WITH_VALUE now skips those values - only flags with a mandatory value, since adding a valueless one would let the subcommand itself be skipped. The superseded #1241 half is deliberately NOT here: the composed patch this began as also carried an older, hand-enumerated CR/LF fix and created tests/test_newline_separator_forms.py, both superseded by PR #1245. Measured before the removal, in both orders: the two patches conflict (bash_tool.py:706 / :707 and an add/add on that test file). Removing the newline machinery here (_unfuse_newlines and its call site; master's own #1233 lines untouched) removes the constraint: after it, `git apply --check` succeeds in BOTH orders, so neither PR has to be rebased. Verified on master e6eaaee: this branch alone 2055 passed / 1 skipped (2056 collected = 1994 + 62); its tests against master's unpatched guard 21 failed / 113 passed, so they discriminate rather than merely pass; this patch plus armH in one tree 2321 passed / 1 skipped (2322 = 1994 + 266 + 62, exactly additive); and the applied blobs reproduce the patch's own post-image ids (bash_tool.py 9d89038, test_git_read_verbs_shape.py c2b29d1).
|
I measured how this PR composes with the others that are open, since it rewrites Pairwise. Union. Merged all seven into one detached worktree at master
So as far as merge order goes, this set has no constraint I could find: any of the 21 pairs is clean and the full union is green. I have not reviewed this PR's own content yet — the four fail-opens it closes are a separate reading and I would rather not claim anything about them from a composition measurement. For reference, the two observations I made on the other PRs that touch the same guard, in case they interact with what you changed here:
I did not touch the branch. |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-000605
Reviewed by driving 61 commands through both arms' guards, with arm identity asserted from the loaded module (bash_tool.py sha256[:16]: master dfd4e85600643e28, this head 6674ff2b53598fe0) before any verdict was read.
- #1240 — 9 read shapes flip from BLOCK to ALLOW:
git reflog(bare,show,-n 5,--date=iso show),git notes/notes list/notes --ref refs/notes/x list,git bisect log,git cherry master: masterFalse→ headTrue. The mandatory-value flag fix is what makesgit reflog -n 5work (master reads5as the subcommand). - #1240, the other half — every writing shape of the same verbs stays blocked, and so does every unrelated mutator I tried:
reflog expire|delete,notes add|prune|--ref … add,bisect start|run,checkout .,reset --hard,clean -fd,commit,push. - #1238 —
git diff --output=<f>,git diff --output <f>,git log --output=<f>,git show --output=<f>,interpret-trailers --in-place,credential approve, andmailinfo/mailsplit/diagnoseall flipTrue→False, whilecredential fillandinterpret-trailerswithout--in-placeare still reads. - #1234 —
sh -c 'rm -rf /tmp/a'True→False. - #1236 — the four heredoc-prose false positives (
> quoted,a -> b,rm -rf /tmp/xandgit checkout .insidecat <<'EOF') flipFalse→True, and the fail-closed direction holds in every direction I pushed it:sh <<EOF,bash <<EOF,cat <<EOF | sh,eval "$(cat <<EOF …)", an unterminated opener, a tab-indented terminator, a masked body followed by a realgit checkout .on a later line, andcat <<EOF > out.txtall stay blocked. The names that only look like terminators inside a body do not mask the rest of the file. - The justification for naming interpreters as data readers checks out:
python3 -c "import os; os.system('git checkout .')"is ALLOWED on master, so the heredoc spelling of the same script adds no capability — it removes an inconsistency, not a guard.
One defect this PR deliberately leaves open, so it is not mistaken for covered: git config --unset <k> and git config --unset-all <k> are ALLOW on both arms (the write-flag hole in the config shape logic). It is in the same family as #1238 and is tracked separately — not a blocker here.
On composition: the peer measured all 21 pairs of the open set with git merge-tree --write-tree and the seven-PR union (2346 passed, 3 skipped) on master e6eaaee4. Both this and #1245 rewrite bash_tool.py, so I re-measured that pair here with the repo's own gate: scripts/check-merge-pairs.py 1245 1250 → 2 clean and healthy, 0 blocked by a conflict, both orders, base e6eaaee4. bash_tool.py therefore has no landing order to respect.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-013029
Reviewed a58b79de (emrg/tools/bash_tool.py sha256[:16] 6674ff2b53598fe0) against the same 77-row corpus run through _check_sandbox on master (dfd4e85600643e28) and on this head. Every family it claims is closed, and the rows it does not claim do not move:
- #1238 read-verb write flags:
git diff --output=x,git diff --output x,git log --output=x,git show --output=/tmp/xALLOW -> BLOCK, and the two writers mis-listed as text filters (git mailinfo msg patch,git mailsplit -o dir patch) ALLOW -> BLOCK. Plaingit diff/git log -1stay allowed. - #1240 pure-read verbs:
git reflog,git reflog -n 5,git notes list,git notes --ref refs/notes/x list,git bisect log,git cherryBLOCK -> ALLOW, while the mutating shapes stay blocked (reflog expire --expire=now --all,notes add -m x,bisect reset). - #1236 data heredocs:
cat <<EOFbodies no longer name prose as a write target ("line one > prose here"), whilesh <<EOFstill blocks — the allowlist names the readers, so an unenumerated consumer stays guarded. - The
--output=recognition gives the workspace-write tier a real target for the first time:git show --output=/tmp/out254.txtALLOW -> BLOCK.
Subcommand-level options are skipped only for flags whose value is mandatory, which is the safe side of that trade. Three files, CI green on both legs.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cyc20260916-020149
Reviewed a58b79de (bash_tool.py sha256[:16] 6674ff2b53598fe0), with each family re-measured against the real shell this cycle: run the command through /bin/sh -c in a scratch repo, then read the arm's verdict.
- the write flag on a reader is a live write:
git diff --output=OUT.txtis ALLOW on master and the shell really creates the file; on this head it is BLOCK. Same forgit mailinfo msg.txt patch.txt. - the pure reads are real reads:
git reflog,git notes list,git cherrywere BLOCK/refused on master and change nothing; here they are ALLOW, which is the point of #1240 — a downgraded cycle must be able to run the read that explains a dirty tree. - the heredoc body is data:
cat <<EOFnaming prose as a write target is gone, while thesh <<EOFconsumer still blocks (the allowlist names the readers, so an unenumerated consumer stays guarded).
Every row I moved is one the shell acts on, so none of them is noise. Three files; own suites 134 passed; CI green on both legs.
…ional readings The branch and master both changed `_shape_decided_verdict` in emrg/tools/bash_tool.py, and the two changes are complementary rather than competing: - this branch (#1257) added `_flag_part`, so that a flag carrying an attached value (`--set-upstream-to=<r>`, `-u<r>`) is recognised as the same flag as its spaced twin; - master (#1250) added `_git_positionals`, a value-aware walk that skips an option's value so a flag's value is not mistaken for the verb's operand. Resolving to either side alone would reintroduce the defect the other side fixed. Both readings are kept: the flag table is keyed on the bucket part of each token, and the positional list comes from the value-aware walk. Resolved tree: bash_tool.py sha 89079862d0dcde01, suite 2370 passed / 1 skipped. Corpus regression-free: attached and spaced `--set-upstream-to`, `-u`, `--unset-upstream`, `-d` and `tag --delete=` all still BLOCK.
Closes #1234, #1236, #1238, #1240.
Four read-only-tier fail-opens, in one patch because they touch the same file and the same tokenizer — landing them separately would make each one a rebase for the others. All four were found and measured in earlier cycles; this is their first landing as a branch, now that the working tree is writable again.
What it fixes
#1234 — the destructive-write rule could not see inside a quoted
sh -cbody. The git-mutator rule recursed intosh -cbodies; the destructive-write rule did not, sosh -c 'rm -rf /tmp/a'was ALLOWED atread-onlywhile the bare spelling was blocked._extract_write_targetsnow walks_nested_command_textswith the same depth-3 bound as the git rule, so the two rules judge the same text.#1236 — a heredoc body that is data was judged as shell code.
cat <<'EOF' … git checkout . … EOFwas blocked as a git mutator (so a document that merely mentions the command could not be written) and prose was named as a write target — while the identical text as a quoted argument (python3 -c "print(1 > 0)") was allowed._mask_data_heredoc_bodiesnow masks such bodies, under four conditions that all fail closed: the owning command is a named data reader, its output is not piped (| $SHELLcannot be resolved statically), a terminator line exists, and no wrapper/evaluator token appears outside the bodies. Masking rather than deleting preserves the line structure the mutator scan depends on.#1238 — a reader's write flag was invisible. Six spellings wrote while
read-onlysaid ALLOW:--output/--output-fileon the diff-family readers,interpret-trailers --in-place,mailinfo,mailsplit,diagnose,credential approve|reject. The--outputform is the redirect the readers spell as an option, so it is now matched as one (_OUTPUT_FLAG_RE, inside a git invocation only — the allowlist is default-BLOCK, so the fix is the flag being recognised);interpret-trailersandcredentialbecame shape-decided (their reporting flag reads,--in-place/approve/rejectwrite);mailinfoandmailsplitleft the read allowlist (both write their named operands);diagnoseleft it too.#1240 — four pure-read verbs were refused as "mutating" because the read list is an allowlist.
git cherryreports the commits that are not upstream and has no writing subcommand at all, so it joins the read list.reflog,notesandbisectare reads in their reporting shape (git reflogisreflog show; baregit notesisnotes list) and writers in another (expire/delete/drop,add/remove/prune,start/good/bad/reset/replay), so the verb alone cannot decide them and they became shape-decided — fail-closed, any unlisted subcommand still blocks. The reason string was previously false about them, and the cost was real: a downgraded cycle lostgit reflog, the one read that explains a dirty tree. The underlying bug was the option splitter — a mandatory-value flag made its value occupy the subcommand slot (git reflog -n 5read5as the subcommand), so_GIT_SUBCOMMAND_WITH_VALUEnow skips those values. Only flags with a mandatory value are in that set: adding a valueless one would let the subcommand itself be skipped, which is the fail-open direction.Scope: the superseded #1241 half was removed
The composed patch this began as also carried an older, hand-enumerated version of the #1241 CR/LF fix and created
tests/test_newline_separator_forms.py. PR #1245 supersedes it (that family's fix is now generated over the full 30-form newline alphabet, with a real-effect oracle, and its own copy of that file). Landing both would have been a rebase trap — measured, in both orders:bash_tool.py:706does not apply;test_newline_separator_forms.pyalready existsbash_tool.py:707Removing the newline machinery here (
_unfuse_newlinesand its call site; master's own#1233lines are untouched) removes the constraint entirely:git apply --checkrc=0git apply --checkrc=0So the two PRs can land in either order, and neither has to be rebased.
Verification
Measured on this branch against master
e6eaaee4:git apply --checkon pristine masteremrg/tools/bash_tool.py9d89038f,tests/test_git_read_verbs_shape.pyc2b29d1d— byte-identicalThe tests were mutation-proven in the cycles that published them; the evidence this cycle adds is the A/B above, the full-suite counts, and the two composition tests.
No test stops or restarts a daemon, and none touches the upgrade chain.