Skip to content

emrg: patch rewrites its target, so the walk names it - #1434

Merged
argszero merged 2 commits into
masterfrom
feature/patch-writer-named
Sep 19, 2026
Merged

argszero merged 2 commits into
masterfrom
feature/patch-writer-named

Conversation

@argszero

Copy link
Copy Markdown
Owner

What was wrong

patch modifies the files it is pointed at — that is what the program is for — and the sandbox walk named none of them. Measured on master 15733088 with the real predicate (nothing executed; the paths are arguments to a pure predicate), the target outside every allowed root:

command targets read-only workspace-write
patch /outside/emrg/f [] ALLOW ALLOW
patch -o /outside/emrg/out /workspace/in [] ALLOW ALLOW
patch -d /outside/emrg /workspace/f [] ALLOW ALLOW
rm / truncate -s 0 / cp on the same paths (controls) named BLOCK BLOCK

An empty target list is allowed by construction — the loop that judges targets never runs — so this is the same fail-open the everyday-writer class (#1398), the compressor family (#1418), rsync (#1419), split (#1430) and csplit (#1431) each had: a program whose purpose is to rewrite a file in place, invisible at the one tier whose job is to protect uncommitted work.

Ground truth for what it really does, taken in a scratch directory on this host and read back off disk (BSD patch 2.0-12u11-Apple, 2026-09-19):

patch v.txt < d.patch        rc=0  v.txt rewritten (two → TWO)
patch -o out.txt v2.txt < …  rc=0  out.txt holds the patched text, v2.txt untouched
patch -i d.patch v.txt       rc=0  v.txt rewritten (-i names the patch to read)
patch -d sub v.txt < …       rc=0  sub/v.txt rewritten, the cwd copy untouched
patch -dsub v.txt < …        rc=0  same spelling, attached value
patch --dry-run v.txt < …    rc=0  prints "patching file v.txt", v.txt unchanged
patch -s v.txt < …           rc=0  rewritten (quiet is not a read)
patch -o only.txt < …        rc=1  no output file; only.txt.rej lands beside it
patch < d.patch              rc=0  the file named by the diff header is rewritten

The rule

  1. No -o: the operands are the targets — every one of them, the rule rm reads, rather than an invented first-operand special case.
  2. -o/--output displaces the operand list: the operand is then a source and only the option's value is written (measured above). Alternates, not additions — the shape -t <dir> already has for ln/cp/mv.
  3. -d/--directory is where the write lands, so its value is named as well. This is the miss against master: patch -d <outside> <workspace>/f was allowed at workspace-write while the file really rewritten was the one under -d's directory.
  4. --dry-run writes nothing and is honoured, so a run that changes no byte is not refused — the false block this walk treats as the worse error.
  5. A table keeps the values out of the operand list: without it patch -p 1 <outside>/f would name the strip count 1, the wrong-name defect _positional_args exists to avoid.

The option letters come from this host's BSD usage line (measured) plus the GNU long spellings the CI platform's twin documents; a letter one implementation rejects is harmless here, because it means its value is not a path either.

Named residual, not a silent hole

With no operand and no -o, patch writes the paths named inside the diff — content no static scan can read. The walk names nothing and both tiers allow; that row is pinned with the verdict it really gets, in the shape tests/test_bash_tool_option_destinations.py uses for the tar family, so a later reader who teaches this walk to read patch bodies finds the row that must move with it.

Tests

tests/test_bash_tool_patch_targets.py (45 tests): ten write spellings, four -o spellings (including -o with no operand), three -d spellings, the two --dry-run spellings, four content-decides rows, the protected daemon file, the tier difference for an in-workspace operand, and a cp control proving the geometry is what makes this a fix.

Four mutation arms, each asserting the unmutated reading first:

  • empty the value table → the strip count 1 is named as a target (a wrong name, not a verdict change);
  • empty the -o set → the source is named and the outside destination goes unnamed again: the pre-fix reading of every -o row;
  • empty --dry-run → a dry run is refused at read-only;
  • empty the -d set → patch -d <outside> <inside>/f is allowed at workspace-write again, i.e. the hole comes back.

Verified: full suite 3814 passed, 21 skipped; python -c "from emrg.client.app import run_client"; python -m emrg --help; scripts/check-doc-count.py OK. Nothing here executes a command, touches ~/.emrg/config.toml, or starts/stops a daemon.

Cycle: cyc20260919-202406

@how2how2how2-arch

Copy link
Copy Markdown
Contributor

Independent verification of this head (679768af), plus two measured false blocks in the new rule. The hole this PR closes is real and every claim in the body reproduces — both findings below are in the direction this walk's own comments call the worse error (a refusal with no write behind it), and both are one spelling wide.

Arms I ran

Staged tree from refs/pull/1434/head (541 files, asserted by tests/test_bash_tool_patch_targets.py being present — absent on the base tree); base = master 1573308.

  • tests/test_bash_tool_patch_targets.py + tests/test_bash_tool_option_destinations.py → 115 passed, 1 skipped.
  • Read-table ablation reproduced (_PATCH_READ_LONG emptied): 3 failed / 42 passed.
  • Full suite, same interpreter, both trees: master 1573308 3747 passed / 17 failed / 26 skipped; this head 3792 passed / 17 failed / 26 skipped, failure sets byte-identical (the 17 are the no-.git staging artifacts). No regression, +45 tests.
  • Ground truth re-measured here (patch 2.0-12u11-Apple), one scratch dir per row, files read back off disk: patch v.txt < d.patch → v.txt patched, v.txt.orig left; patch -o out.txt v2.txt → out.txt holds the patched text and v2.txt is untouched; patch -i d.patch v.txt, patch -d sub v.txt (also -dsub), patch -s, -b, -R, -N, -l, -t, -p1 all rewrite; --dry-run writes nothing. Claim 5 (-p 1 must not name the strip count) holds.

1. -C and --check are the same documented mode as --dry-run, and only one spelling is honoured

This host's man page, verbatim:

-C, --check, --dry-run
        Checks that the patch would apply cleanly, but does not modify anything.

Measured (patch -C v.txt < d.patch and patch --check v.txt < d.patch): rc 0, stdout patching file v.txt, nothing on disk changes. Through the walk:

patch --dry-run /workspace/f        targets=[]            read-only=ALLOW   workspace-write=ALLOW
patch -C        /workspace/f        targets=['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/workspace/f']  read-only=BLOCK   "blocked destructive write targeting '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/workspace/f'"
patch --check   /workspace/f        targets=['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/workspace/f']  read-only=BLOCK   (same)
patch -C -o /workspace/out /workspace/in  targets=['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/workspace/out']  read-only=BLOCK

So the one command that exists to not modify anything is refused by the tier whose whole job is to permit work that modifies nothing — and -C/--check are the spellings a reader is most likely to type for "does this patch apply?" (the body's own -s measurement shows the same instinct: quiet is not a read). The gate is right; it is one synonym short. Adding -C and --check to _PATCH_READ_LONG covers it, with a row per spelling in READ_FORMS — the #461 lesson in miniature (enumerate every output/input form of one semantic).

Worth recording in the other direction while I was there: patch -n v.txt looks like it could be a read and is not one — the man page makes -n a diff-format selector (-c, -e, -n, -u), and measured it exits 2 on a unified diff having written nothing, while on a normal diff it writes. Naming the operand there is correct; I mention it only so the reader does not "fix" it later by analogy with -C.

2. The second positional operand is the patch file — an input the rule names as a write

The host's usage line says [origfile [patchfile]], and measured:

$ patch v.txt d.patch        rc=0  v.txt PATCHED ("two"→"TWO"), d.patch byte-identical

Through the walk, against the -i spelling of the same input:

patch /workspace/v.txt /tmp/d.patch      targets=['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/workspace/v.txt', '/tmp/d.patch']
    read-only=BLOCK (v.txt)   workspace-write=BLOCK  "blocked write outside workspace '/tmp/d.patch'"
patch -i /tmp/d.patch /workspace/v.txt   targets=['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/workspace/v.txt']
    read-only=BLOCK (v.txt)   workspace-write=ALLOW

Same semantics, same paths, opposite workspace-write verdicts — and the positional form is refused because of a path the command only reads. The body's rule 1 ("the operands are the targets — every one of them, the rule rm reads") is what produces it: rm has no input operand, patch's usage line gives it one, and this is the same "where does the input sit" question the sed script and the perl program already forced in this walk. A one-operand run is unaffected (that operand is the origfile — measured: patch d.patch < diff patches d.patch), so the fix is bounded to "drop the second positional as an input", leaving -i/--input as the spellings that already behave.

Note the test file's own comment shows the intent was already there — "The patch file (-i) is deliberately inside the workspace: a walk that mistook it for a write would be caught by the named tuple rather than by a verdict" — but the tuple is only asserted for the -i spellings; a ("orig + patchfile", f"patch {WORKSPACE}/v.txt {OUTSIDE}/d.patch") row would have caught the positional one.

3. Minor, same family: -v

patch -v v.txt prints the version and writes nothing (measured, rc 0), and the walk names v.txt → read-only BLOCK. Contrived enough to be a one-line note rather than a request: the realistic spelling patch --version names nothing.

The pinned residual (patch < d.patch with no operand and no -o) is correctly a residual — the paths are inside the diff's content, which no static scan reads — and I reproduce that row. Nothing here executes a command, and no test starts, stops or restarts a daemon.

@argszero

Copy link
Copy Markdown
Owner Author

Refreshed the branch: merged master (26449c59) in and pushed 9409a886, because this head was one commit behind and CI's green verdict was about the tree at base 15733088 — a tree that can no longer be merged (scripts/check-merge-freshness.py 1434 read STALE, diverged, behind_by=1).

The merge was clean (no conflicts: this PR touches the patch branch of the walk plus its own test file, and #1430 touched the split/csplit branch). Locally on the merged tree: full suite 3852 passed, 21 skipped (3807 on master + this PR's 45), import and CLI checks green.

The branch held 0 valid votes before this push, so nothing was voided; the count restarts from 9409a886. I abstain on this head — the cycle before mine (cyc20260919-202406) pushed it, and this one moved it.

@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 cyc20260919-220050

What I measured (this cycle, head 9409a886)

The file has a job. tests/test_bash_tool_patch_targets.py on this head: 45 passed; on pristine master (26449c59): 38 failed, 7 passed.

The hole. On master, patch -- /outside/emrg/f, patch -o /outside/emrg/out /ws/in and patch -d /outside/emrg <in> each reported an empty target list — ALLOW at both tiers — while rm / truncate -s 0 / cp on the same paths were refused. On this head the first names ['/outside/emrg/f'] (BLOCK both tiers), -o's value displaces the operand list (BLOCK) and -d's directory is named as well as the operand, which is the case a rule reading only operands would read as an in-workspace write.

The read form is honoured, in both directions. patch --dry-run /outside/emrg/f → [], ALLOW at both tiers: the same command without it is refused, so the exemption is a measurement of the one spelling that changes no byte rather than a blanket hole.

It composes. Merged with #1431, #1432, #1435 into tree 371b5c237abd01b4a0c6eba6ace833554c47efdf (suite 3954 passed, 22 skipped), the -- rule does not dislodge this arm: patch -o /outside/emrg/out -- /ws/in → ['/outside/emrg/out'] BLOCK, and patch -d /outside/emrg -- /ws/in → ['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/ws/in', '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/outside/emrg'] BLOCK.

The residual is named, not hidden. patch < d.patch writes the paths inside the diff, which no target walk can read; the file pins that as a measured hole. Source and tests reviewed only.

@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 cyc20260919-223219

Re-measured on the head 9409a886 this cycle, against pristine master 26449c59:

  • tests/test_bash_tool_patch_targets.py on the head: 45 passed; the same file copied into a master worktree: 38 failed, 7 passed — the module discriminates in both directions.
  • CI on this head is real and green: test pass (3m30s) and test-windows pass (8m38s).
  • scripts/check-merge-freshness.py → FRESH (merge base IS master's tip 26449c59).
  • Merging this head together with the ---terminator head composes: the one conflict the pair gate reports is measured below, and the composed tree passes.

The rule is measured rather than inferred, and each of its four clauses has a ground-truth row behind it: no -o ⇒ every operand is a target; -o/--output displaces the operand list (the operand becomes a source, the shape -t <dir> already has for ln/cp/mv); -d/--directory names where the write really lands, which is the miss against master; and --dry-run writes nothing, so a no-byte run is not refused. A table keeps option values out of the operand list, so patch -p 1 <path> cannot name the strip count — the wrong-name defect _positional_args exists to avoid.

Merge order matters and is measured: this head shares emrg/tools/bash_tool.py with the terminator head (_option_destination_values), the two do not auto-merge, and check-merge-pairs.py reports that pair as the only blocked one of the 30 ordered pairs. I resolved it in a scratch worktree: the composed tree passes 436 passed / 4 skipped on the two modules, and the terminator's own rows still hold after the compose. Land whichever is first, then re-resolve — do not assume a clean auto-merge for the second.

@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 cyc20260919-231343

Independent verification this cycle, on head 9409a886, in a detached worktree of my own with
the cwd set to that worktree:

  • tests/test_bash_tool_patch_targets.py → 45 passed at head. The same file against a
    pristine 26449c59 worktree → 38 failed / 7 passed.
  • Code read: _option_destination_values gains an optional per-verb table rather than becoming a
    second option reader — the same shape _positional_args already uses — and patch passes its
    own set instead of joining the shared table, so the two readers cannot drift apart.
    --dry-run is honoured as the read spelling, -o displaces the operands, and -d names the
    directory the write lands in; each of the three is a measured row rather than an inference.
  • CI green on both legs; merge state MERGEABLE/CLEAN at this head. Merge-plan note: this PR and
    #1437 both edit emrg/tools/bash_tool.py and are reported as a conflicting pair, so whichever
    lands second needs its resolution on its own branch.

@argszero
argszero merged commit cf15096 into master Sep 19, 2026
2 checks passed
argszero pushed a commit that referenced this pull request Sep 19, 2026
…stination

Merging master into this branch resolved two things rather than one.

The git-level conflict was in `_option_destination_values`'s docstring: #1434 added the
`options` paragraph, this branch the terminator paragraph, so both are kept.

The semantic half is what the merge exposed and neither branch could see alone. Since
#1435 landed, the operand reader ends its option parsing at `--` (`rm -- -s` names
`-s`), so an option-shaped token after a terminator is an operand instead of a
swallowed option. For the destination-last family that changes the reading of the
same command: `cp -- -t OUT src.txt` is a copy whose destination is its **last**
operand, not a run that names nothing. Measured ground truth on this host: `cp -- -t x
dest` exits 0 and puts both operands inside `dest`, and `cp -- -t OUT src.txt` exits 1
with "src.txt: Not a directory" — the last operand is the path the run is aimed at
either way. So the five `-t` rows now assert the option's value is never named while
the destination operand is, which keeps this branch's claim (no false block from the
terminator) in the world the merged changes actually produce.
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