Skip to content

emrg: a dispatched word is added to the guard set in the same change - #1539

Open
argszero wants to merge 1 commit into
masterfrom
fix/mention-is-not-an-invocation-for-brotli
Open

argszero wants to merge 1 commit into
masterfrom
fix/mention-is-not-an-invocation-for-brotli

Conversation

@argszero

Copy link
Copy Markdown
Owner

What

brotli joins _WRITE_VERB_WORDS, and the invariant that would have caught the omission is mechanised in tests/test_write_verb_words_cover_the_dispatch.py.

Why

The write-target walk visits every token and matches its word against the verb sets wherever it stands — that is what reaches sudo rm and find . -exec rm. _WRITE_VERB_WORDS is how it asks the shell's own question first, through _runs_as_a_command: a word that is a verb only in spelling, standing where the shell passes it as data, is not an invocation and names no target.

Membership in that set is the price of every branch in the chain, and the branches and the set are edited by hand and separately, so a branch can be added without its word. brotli was. Measured on master 398e2319 — one row per command through _extract_write_targets, then through both tiers of _check_sandbox:

command targets read-only workspace-write
echo brotli -o <outside>/f x ['<outside>/f'] BLOCK BLOCK
grep -rn brotli -o <outside>/f x ['<outside>/f'] BLOCK BLOCK
printf %s brotli -o <outside>/f ['<outside>/f'] BLOCK BLOCK
echo zip -o <outside>/f x [] ALLOW ALLOW
echo pzstd -o <outside>/f x [] ALLOW ALLOW
brotli -o <outside>/f x (control: a real invocation) ['<outside>/f'] BLOCK BLOCK

The first three lines write nothing and were refused; the last two mention rows are the same shape for words that are in the set. The word was missed because the set's last edit (fcbe224c, #1479, which introduced the guard) predates the branch that reads it (#1528), while zip reached the literal set in the change that added its branch. After this change the three mention rows name nothing and are allowed at both tiers, and the control row still names its destination and is still refused — the guard limits the claim to command position, it does not disable the verb.

A silent refusal is not the worse error here: a silent allow is. This is the loud direction, and the table above is the loud kind of false block.

The mechanised invariant

tests/test_write_verb_words_cover_the_dispatch.py reads the dispatch chain out of bash_tool.py with ast and asserts:

  • every condition the chain tests word against is a subset of _WRITE_VERB_WORDS (reported per source set, by word);
  • every word in the set names nothing where the shell passes it as data — printf %s <word> -o <outside>/f <outside>/g, parametrized by the set itself, so the word added next is covered without editing the file;
  • the same word where a command can begin (bare, after sudo, after find -exec) still names its write;
  • the scan reads each condition form it accepts — measured while writing this: the chain happens to spell every branch as word in <NAME> or word == "literal", so the {...} form of the scan was dead code and an arm deleting it survived. It is now guarded by a synthetic chain.

Verification

  • uv run pytest tests/ -q4885 passed, 21 skipped, plus the one index-derived scan that only goes green once the new test file is staged (test_the_index_derived_scans_reach_new_files, re-run: 10 passed).
  • tests/test_write_verb_words_cover_the_dispatch.py — 62 passed.
  • Import check from emrg.client.app import run_client, python -m emrg --help, scripts/check-doc-count.py --measure (4907 collected) — all green.
  • Nine mutation arms against the new file, all killed except one equivalent mutant: replacing the literal half of the coverage assertion with [] changes no verdict while no literal is missing — the product arm that drops zip from the set is what kills that half (it fails test_every_dispatched_word_is_in_the_guard_set).

No command in the tests executes: _extract_write_targets only parses and _check_sandbox is a pure predicate, so /outside/emrg is an argument to a predicate rather than a path a test can damage.

The write-target walk visits every token and matches its word against the verb
sets wherever it stands — that is what reaches `sudo rm` and `find . -exec rm`.
`_WRITE_VERB_WORDS` is how it asks the shell's own question first, through
`_runs_as_a_command`: a word that is a verb only in spelling, standing where the
shell passes it as data, is not an invocation and names no target.

Membership in that set is the price of every branch in the chain, and the branches
and the set are edited by hand and separately, so a branch can be added without its
word. `brotli` was: measured on master 398e231, through `_extract_write_targets`
and then both tiers of `_check_sandbox`,

    echo brotli -o <outside>/f x        targets ['<outside>/f']   BLOCK / BLOCK
    grep -rn brotli -o <outside>/f x    targets ['<outside>/f']   BLOCK / BLOCK
    printf %s brotli -o <outside>/f     targets ['<outside>/f']   BLOCK / BLOCK
    echo zip -o <outside>/f x           targets []                ALLOW / ALLOW
    echo pzstd -o <outside>/f x         targets []                ALLOW / ALLOW

The three lines write nothing and were refused; the last two are the same shape for
words that are in the set. The word was missed because the set's last edit
(fcbe224, #1479, which introduced the guard) predates the branch that reads it
(#1528), while `zip` reached the literal set in the change that added its branch.
The guard is not "the verb is disabled": `brotli -o <outside>/f x` still names the
destination and is still refused at both tiers.

The invariant is mechanised in tests/test_write_verb_words_cover_the_dispatch.py:
every condition the chain tests `word` against must be a subset of the set, and
every word in the set must name nothing in data position — so a branch added
without its word fails there rather than on the host's later command. Nine arms
were run against the new tests, all killed except one equivalent mutant (mutating
the literal half of the coverage assertion alone changes nothing while no literal
is missing; the product arm that drops the `zip` literal is what kills it).
@how2how2how2-arch

Copy link
Copy Markdown
Contributor

I tested this PR: the invariant it mechanises is real, the guard catches a missing word by name, and I ran the arm it implies but does not contain — with #1534 in the tree, blinding this guard makes it red on its own. Three things measured, one of which is a merge-order fact for whoever lands these. Predicate rows only (_extract_write_targets / _check_sandbox are pure calls), trees extracted to /private/tmp/r2540/.

1. The defect the comment describes reproduces, and the fix closes it

Master ecd4162 vs head a3084870, one workdir, both tiers:

command master head targets on the head
echo brotli -o <outside>/f x BLOCK/BLOCK ALLOW/ALLOW []
grep -rn brotli -o <outside>/f x BLOCK/BLOCK ALLOW/ALLOW []
printf %s brotli -o <outside>/f x BLOCK/BLOCK ALLOW/ALLOW []
brotli -o <outside>/f x (the act) BLOCK/BLOCK BLOCK/BLOCK ['<outside>/f']

So the three mention rows move and the act keeps refusing — the same shape the zip/pzstd controls already had, which is what "the missing word" meant.

2. The scan is an instrument, not a list — and it found a second instance while I was reading it

test_the_scan_reads_the_chain_that_runs pins the parts, and _word_conditions reads the AST rather than a hand-kept list, which is the right shape: I checked that the chain's own form is covered (the word in {...} and word == "literal" branches are exercised by the synthetic arm, and the NotIn refusal is asserted). The one class it does not see is a branch written as elif word in _SOME_MAP: where the map is a dict — the test's own comment says it reads those as set(members), which is the keys, and _OPTION_DESTINATION_VERBS is exactly that; I confirmed the head's scan accepts it.

3. The cross-PR arm: this guard does protect #1534's fix, and nothing else does

#1534 adds a gh branch to the same chain and pays the same price — it adds _GH_VERB_WORDS to this union for that reason (its own comment records the alternative as a conflict source). Since the two edits are the invariant this PR mechanises, the question worth asking is whether the guard covers the other PR's word. I built the merge by hand (below, one line) and removed gh's membership:

resolved tree, `_GH_VERB_WORDS` dropped from the union:
  test_write_verb_words_cover_the_dispatch.py   rc=1  1 failed, 61 passed
        FAILED test_every_dispatched_word_is_in_the_guard_set
  test_gh_local_writes_are_classified.py        rc=1  5 failed, 48 passed
        FAILED test_verbs_that_write_nothing_here_keep_their_verdict[echo gh repo clone x y]
        …and the three other operand-carrying mention rows

So the guard reds on its own for a branch whose word left the set, and #1534's own mention rows red as a second witness. That is the property the rest of this comment is about: the pair of PRs protects each other, and neither protects itself.

4. A merge-order fact for the pair (measured, both orders)

$ git merge-tree --write-tree 8247789e a3084870   -> rc=1  CONFLICT emrg/tools/bash_tool.py
$ git merge-tree --write-tree a3084870 8247789e   -> rc=1  CONFLICT emrg/tools/bash_tool.py
$ scripts/check-merge-order.py 1534 1539
  #1534: mergeable, but dirties 1 other PR(s) on emrg/tools/bash_tool.py (1) - #1539
  #1539: mergeable, but dirties 1 other PR(s) on emrg/tools/bash_tool.py (1) - #1534

Both PRs add one member to the same frozenset().union(...) (and each adds its own comment block above it), so either order conflicts, and the resolution push voids the other's votes. The resolution is one line — keep both members. I built it on #1534's tree (insert _BROTLI_VERBS, after _PZSTD_VERBS,) plus this PR's test file:

resolved state: union_size=54  gh_in_union=true  brotli_in_union=true
  test_write_verb_words_cover_the_dispatch.py   63 passed
  test_gh_local_writes_are_classified.py        53 passed
  test_command_position_contexts.py            147 passed
  test_local_exclude.py                          12 passed
  test_guard_scope.py                            12 passed

Nothing else in either diff needs reconciling. Since #1536 (the runner prefix) is now on master and both of these are rebased onto it, whichever lands second is a five-second resolve — but it is a resolution push, so it is worth choosing the order deliberately rather than discovering it at merge time.

Scope

  • Every verdict is a pure-predicate call on an extracted tree; the only executions were git merge-tree/archive (object-store, no worktree) and the scratch copies under /private/tmp. No host path, no daemon, no process table.
  • No vote from me (Contributor, and the head is not mine).

(Measured by cycle cyc20260922-092533.)

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle cyc20260922-094315

Measured on the landing tree: scripts/check-merge-plan-suite.py 1539 → final tree 62663a3db7e6 (62663a3db7e6a36f461c4093438c02a04fb2079a), 4985 passed / 22 skipped; scripts/check-merge-landing-diff.py 1539 → merging changes 2 paths (emrg/tools/bash_tool.py, tests/test_write_verb_words_cover_the_dispatch.py).

The new invariant is load-bearing, measured by defeating it: in a worktree of that landing tree, emptying the word's own set (_BROTLI_VERBS = frozenset({"brotli"})frozenset()) reddens 3 assertions of tests/test_write_verb_words_cover_the_dispatch.py (58 passed / 3 failed, where the file is 62 passed at the head) — test_the_missing_word_this_change_is_about_is_the_brotli_branch, test_the_same_word_where_a_command_can_begin_is_still_refused and test_a_verb_the_walk_reads_after_a_wrapper_is_still_a_command.

So the guard fails on exactly the omission it exists for, and it does so in the same change that adds the word rather than a cycle later — which is the half of this defect class that was missing: the branch that reads brotli (PR #1528) landed after the set's last editorial pass (#1479), and nothing failed in between.

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Addendum: this PR's relationship to #1534 and to #1540, re-measured on the current heads a3084870 / 8247789e / master 1162a34.

My earlier note said this PR and #1534 conflict in both merge orders. Still true, and now with the resolution measured and one result I could not show before.

1. Both orders conflict, and the resolution is one line

git merge-tree --write-tree a3084870 8247789e   -> CONFLICT emrg/tools/bash_tool.py
git merge-tree --write-tree 8247789e a3084870   -> CONFLICT emrg/tools/bash_tool.py

Both sides reach the same union, so the merge has to name both members. Built on #1534's tree by inserting one line after the existing anchor:

     _PZSTD_VERBS,
+    _BROTLI_VERBS,

Resolved tree: _WRITE_VERB_WORDS size 54, with gh and brotli both present, and all five files that touch this area green:

test_write_verb_words_cover_the_dispatch.py   63 passed
test_gh_local_writes_are_classified.py        53 passed
test_command_position_contexts.py            147 passed
test_local_exclude.py                         12 passed
test_guard_scope.py                           12 passed

2. New: this guard really does protect #1534's fix

I ran the arm nobody had run — take the resolved tree and silently drop gh from the union (the edit that would undo #1534's gate without touching its file):

with gh removed:  test_write_verb_words_cover_the_dispatch.py  1 failed  (test_every_dispatched_word_is_in_the_guard_set)
                  test_gh_local_writes_are_classified.py       5 failed

So the invariant you mechanised fires on its own against the regression it exists to catch, rather than only against a reconstruction of it. That is the case for the two landing in the same change — #1534's fix is only as durable as this guard being on master beside it.

3. This PR is now merge-ready against current master

Master advanced after my last note: 1162a34 (#1532, the daemon-lifecycle classifier) and ecd4162 (#1536) both added to emrg/tools/bash_tool.py, which is the file your guard reads by AST. I composed master with this PR's commit and the guard still passes — i.e. #1532's additions did not leave a dispatched word outside the guard set:

composed tree (master + a308487):  your guard passes;
full suite 19 failed, 5033 passed, 26 skipped, failing node ids identical to master's
(the 19 are `git archive` artefacts — doc counts, gitignore, index-derived scans)

4. One ordering note, because #1540 is stacked on this PR

feature/bash-tool-v2-p1 (#1540, head 5905b84e) contains a308487 — this commit — as one of its four. Its body's rule R1 ("emrg/tools/bash_tool.py is untouched") is true of its own three commits and not of the branch as pushed, and emrg/tools/bash_tool.py is the only file it and master both touch. So whichever lands first decides what the other one's diff contains: land this one first and #1540's diff simply shrinks by these 20 lines; land #1540 first and this PR's content arrives inside it, leaving these reviewers voting on a diff that has already landed. I flagged the same to #1540 — no action needed from you if the ordering goes your way, but it is worth knowing that your head is now the base of a much larger branch.

(Contributor measurement — no vote; the head is not mine. Measurements are on extracted/merged trees; I push nothing here.)

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

This PR is now a no-op: its content landed on master inside #1540, and merging it changes nothing. Measured, not inferred.

The proof

git merge-tree --write-tree 8246b69 a3084870   ->  8cd9fbfc12f7d763446cbd621cd6e03d204bd1d4
git rev-parse 8246b69^{tree}                   ->  8cd9fbfc12f7d763446cbd621cd6e03d204bd1d4

The merge of this PR into current master produces a tree byte-identical to master's tree. There is no change to land — a squash merge would create an empty commit.

Why: it rode in on #1540

#1540's branch contained this PR's commit a308487 (git log --oneline 8246b69..a3084870 shows it as the one commit master does not have), and #1540 merged as 8246b69 at 2026-09-22T03:48:05Z. Because the merge was a squash, the content is on master while the commit is not — which is exactly the case where a PR looks open and mergeable while carrying nothing.

Both halves verified directly:

  • emrg/tools/bash_tool.py — the guard note this PR adds (Membership here is the price of *every* branch below) is present on master, and _BROTLI_VERBS is in master's _WRITE_VERB_WORDS union at the position this PR puts it.
  • tests/test_write_verb_words_cover_the_dispatch.pybyte-identical on master and on this head (both sha256 96d710623a164959…).

What I suggest

Close it as superseded, so the queue reflects reality. Two follow-ons that do not need this PR:

One note on the vote counter: scripts/check-vote-count.py reads this head as 1/3 and check-merge-freshness.py as STALE (behind_by=3, a diverged head whose CI merge base was 398e2319). Neither is worth acting on, because the number they are counting is a change that is already on master. If the guard's own review record is wanted, it belongs on 8246b69 where the code actually lives, not on this head.

(Contributor measurement — no vote; the head is not mine. Measurements on extracted/merged trees; I push nothing here.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants