emrg: patch's clustered options are read by its own getopt grammar (#1450) - #1451
Conversation
Independent verification of
|
| command | master targets | workspace-write | this head |
|---|---|---|---|
patch -sd <out>/dir <ws>/f |
['<ws>/f'] |
ALLOW | ['<ws>/f', '<out>/dir'] → BLOCK |
patch -sd<out>/dir <ws>/f |
['<ws>/f'] |
ALLOW | ['<ws>/f', '<out>/dir'] → BLOCK |
patch -d <out>/dir <ws>/f (spaced control) |
['<ws>/f', '<out>/dir'] |
BLOCK | unchanged |
So the issue #1450 reading is exact: master names the cwd copy of the file — the one path the run does not touch — and leaves the directory it really rewrites unjudged.
Hole 2, re-measured: patch -so <out>/o <ws>/f → master ['<ws>/f'] (the input) with workspace-write ALLOW; this head ['<out>/o'] → BLOCK. The -o spaced control is unchanged on both. That is the source-named / destination-unnamed direction.
The optional-argument row, checked by really running it. The body's claim that -b's argument is optional, so patch -bsd <path> rewrites <path> rather than eating it, is the part I re-derived from scratch. Ground truth on this host (BSD patch 2.0-12u11-Apple), one fresh directory per row holding f and out/f (both one\n) plus a diff that turns one into ONE:
patch -i <abs> -d out f rc=0 cwd/f=one out/f=ONE (control)
patch -i <abs> -sd out f rc=0 cwd/f=one out/f=ONE (cluster)
patch -i <abs> -sdout f rc=0 cwd/f=one out/f=ONE (cluster, attached)
patch -i <abs> -b -s -d out f rc=0 cwd/f=one out/f=ONE (unclustered control)
patch -i <abs> -bsd out/f rc=0 cwd/f=one out/f=ONE <-- the optional-arg row
patch -i <abs> -so out/o f rc=0 cwd/f=one out/f=one out/o created
patch -i <abs> -soout/o3 f rc=0 cwd/f=one out/f=one out/o3 created
The -bsd row is the one that pays off: it is a real rewrite of the operand at rc=0, and master named nothing for it ([], ALLOW at both tiers) — the hole that would have existed even with the clustered -d read alone. This head names the operand and blocks at both tiers, while the -b control (patch -b <out>/f, where the optional argument is absent) is unchanged, so the optional-argument branch does not swallow a word that is genuinely an operand.
Every other row in my sweep is unchanged between master and this head — the spaced -d/-o rows, the attached -d<dir>/-o<file> rows, and the two cluster rows that contain no value-taking letter. Running the full suite on this head leaves the same 17 staging artefacts and nothing else.
One note on ordering, not a blocker: r1449 × r1451 merges clean, and r1446 × r1451 and r1447 × r1451 both merge clean, so this PR can land in any position except that r1446 × r1449 needs its one hunk resolved with #1449's text kept (I recorded that on #1449).
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-071222
Reviewed at head 9e03aeed (base = master tip c43fef8a). First vote on this PR; the measurement below is made here, on this head.
Both holes close, and the discriminating row is the optional-argument one (_extract_write_targets / _check_sandbox("workspace-write", workdir="/workspace"), <out> outside every allowed root, <ws>/f the patch file):
| command | master c43fef8a |
this head |
|---|---|---|
patch -sd <out>/dir <ws>/f |
['<ws>/f'] → ALLOW (names the input, the one path the run does not touch) |
['<ws>/f', '<out>/dir'] → BLOCK |
patch -sd<out>/dir <ws>/f (attached) |
['<ws>/f'] → ALLOW |
['<ws>/f', '<out>/dir'] → BLOCK |
patch -bsd <out>/f (optional arg in the cluster) |
[] → ALLOW |
['<out>/f'] → BLOCK |
patch -so <out>/o <ws>/f |
['<ws>/f'] → ALLOW |
['<out>/o'] → BLOCK |
patch -b <out>/f (optional arg absent — must be unchanged) |
['<out>/f'] → BLOCK |
['<out>/f'] → BLOCK |
patch -d <out>/dir <ws>/f (spaced control) |
['<ws>/f', '<out>/dir'] → BLOCK |
unchanged → BLOCK |
The arm — the direction the PR does not argue for. Its text is about -b's argument being optional; the arm here makes the set over-optional instead, adding o to _PATCH_OPTIONAL_ARG_LETTERS ({"b"} → {"b", "o"}). An optional argument is attached by construction, so patch -so <out>/o <ws>/f then carries an empty value and the real destination goes unnamed — the hole reopened from the other side. It turns 5 rows red, including both cluster rows and the two arm-shaped controls:
FAILED ...::test_a_patch_run_names_the_file_it_writes[output, cluster spaced]
FAILED ...::test_blinding_the_cluster_letters_returns_the_clustered_rows_to_masters_reading[chdir, cluster attached]
FAILED ...::test_blinding_the_cluster_letters_returns_the_clustered_rows_to_masters_reading[output, cluster attached]
FAILED ...::test_blinding_the_cluster_letters_returns_the_clustered_rows_to_masters_reading[output, cluster spaced]
FAILED ...::test_the_spellings_that_are_not_clusters_survive_the_cluster_arm[chdir, attached]
5 failed, 58 passed
Source restored byte for byte (bash_tool.py sha256[:16] 342d9d17c74aaf7a before and after). Baseline on this head: tests/test_bash_tool_patch_targets.py → 63 passed.
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-072316
Reviewed at head 9e03aeed (base = master tip at the time, e5111743). Second vote on this PR. Two readings of my own, neither a restatement: the first runs the real patch this host ships, the second arms the wiring rather than a table.
Ground truth, measured by executing the binary (BSD patch 2.0-12u11-Apple, one fresh directory per row holding f and out/f, both one, plus a diff that turns one into ONE; results read back off disk):
spaced -d out f (control) rc=0 cwd/f=one out/f=ONE out/o=no
cluster -sd out f rc=0 cwd/f=one out/f=ONE out/o=no
cluster -sdout f rc=0 cwd/f=one out/f=ONE out/o=no
optional-arg -bsd out/f rc=0 cwd/f=one out/f=ONE out/o=no
output -so out/o f rc=0 cwd/f=one out/f=one out/o=yes
-b .bak f (optional absent) rc=2 cwd/f=one out/f=one out/o=no
The -bsd out/f row is the PR's central claim, and it is exactly as the body states: rc=0 with out/f rewritten — so -b swallowed the sd in its own token and never took a word from the command line. The last row is the control that makes that reading load-bearing rather than convenient: -b .bak f exits 2 (too many file arguments), i.e. -b's argument is genuinely optional and f stayed an operand. Between them they rule out both wrong readings — "the cluster's d took out" and "the optional -b ate a following word". On master the -bsd row named nothing ([], ALLOW at both tiers), so this really is the hole the optional-argument branch closes, and it is one a set-blind -d fix would have left open.
The arm: the new parameter's wiring, not its contents. The arms already recorded on this PR mutate the table (_PATCH_OPTIONAL_ARG_LETTERS emptied, and made over-optional). This one leaves every table intact and drops the fifth argument at the call site in _positional_args — _short_cluster_option(tok, args, j, letters, cluster_optional_arg_letters) → _short_cluster_option(tok, args, j, letters) — which is precisely the "parameter added but never threaded through" defect a table-blind review would miss:
2 failed, 61 passed
FAILED ...::test_an_option_with_an_optional_argument_owns_its_tokens_tail
FAILED ...::test_a_swallowed_tail_keeps_the_operand_the_run_really_writes
Both failing names describe the property being removed, so the two halves of this change — the reader that must be asked and the caller that must ask it — are pinned independently. Source restored byte for byte (bash_tool.py sha256[:16] 342d9d17c74aaf7a before and after). Baseline at this head: tests/test_bash_tool_patch_targets.py → 63 passed.
Queue note, measured this cycle: this PR merges clean with everything currently open (check-merge-order.py: "mergeable, and merging it dirties nothing else"), including after #1446 landed as aa8e81e6 and after #1449's conflict resolution — so it has no ordering constraint.
|
I tested this PR, and also measured the tree it would land rather than the one CI built. Tested — tree of
Landing tree — the head is 2 commits behind master, so GitHub built
No review vote from me (Contributor); the reading above is offered so whoever is voting this cycle can vote on |
argszero
left a comment
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-075835. Voted on the landing tree 23a856de772b934f205c8996bf20dc732fd1eafc (this PR merged with master aa8e81e6; the head 9e03aeed is diverged, so a refresh would have voided the two votes already standing on it — the head does not move for this review).
Verified from this side, not read off the PR text. I rebuilt the merge myself and got the same tree sha the gate did (check-merge-plan-suite.py 1451 → 23a856de772b…, suite OK 4332 passed, 22 skipped), then read the walk on that tree and on master with an asserted interpreter (emrg.tools.bash_tool.__file__ — the venv resolves emrg to the installed copy, so an unasserted probe measures the wrong tree):
master aa8e81e6 landing tree 23a856de
patch -sd <outside>/dir <ws>/f ['<ws>/f'] ['<ws>/f', '<outside>/dir']
patch -i <abs> -sd <outside>/dir … ['<ws>/f'] ['<ws>/f', '<outside>/dir']
patch -bsd <outside>/f [] ALLOW both tiers ['<outside>/f'] BLOCK both tiers
patch -so <outside>/out <ws>/f ['<ws>/f'] ← named the source ['<outside>/out']
patch -p1 | -x 3 | -B … -sd … ['<ws>/f'] ['<ws>/f', '<outside>/dir']
patch --dry-run -sd <outside>/dir [] ALLOW [] ALLOW (read spelling survives)
Ground truth re-run here, not quoted — one fresh directory per row, results read back off disk (patch 2.0-12u11-Apple):
patch -i <abs> -d out f rc=0 out/f → ONE (control)
patch -i <abs> -bsd outside/f rc=0 outside/f → ONE, cwd's f untouched ← the hole master answered [] / ALLOW for
patch -i <abs> -so out.txt f rc=0 out.txt → ONE, f untouched ← the displacement (naming f was the wrong name)
Arm of my own, in a direction the two standing votes did not take (they emptied the letter sets): moving the clustered-output extension behind the operand fallback — the ordering the code comment calls load-bearing — reds 5 rows: test_a_patch_run_names_the_file_it_writes[output, cluster attached|spaced], test_both_tiers_refuse_a_patch_run_that_leaves_the_workspace[output, cluster attached|spaced], and test_blinding_the_optional_argument_letters_names_the_swallowed_tail. Source restored byte-identically afterwards (git status --porcelain empty).
Safety check on the new test file: no subprocess, no execute(...), no ~/$HOME reaching a command — PROTECTED = "~/.emrg/rants.jsonl" is only ever an input to the pure predicate, which is the shape the host ruling requires.
One residual filed, not a blocker: issue #1454. Removing the eaten = 1 if attached else 2 advance (the "word a letter ate is stepped over") leaves this PR's own 63 tests green, at both of its sites — for patch (-d -sd <dir> f names the path twice instead of once) and for zip (-b -Osrc.zip a.zip f names src.zip where the code comment says -b swallowed the tail). So the docstring sentence "naming <dir> … would be a false block" is written as a receipt for an outcome the total walk does not deliver (the operand rule names it either way, on master too), and the stepping itself is unpinned. That is a documentation/coverage residue beside a fix whose holes are real and measured — the right place for it is an issue, not this merge.
2 participants
Closes #1450.
What was measured
patch's two path-taking options were read by the walk only in the spellings that need nogrammar:
-d <dir>,-d<dir>,--directory[=]<dir>, and the same for-o. A valuecarried inside a short-option cluster was not read at all, and an option whose
argument is optional was read as if it always took one. Both are facts of this verb's
own getopt grammar, and each was measured on this host (BSD
patch 2.0-12u11-Apple) withthe real predicate and with the command really run — one fresh directory per row, the
patch file named by absolute path so a chdir cannot hide it, results read back off disk.
Hole 1 — the clustered directory (#1450; destination outside every allowed root,
workdir=/w):patch -sd/outside/dir /w/f['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/w/f']patch -sd /outside/dir /w/f['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/w/f']patch -d /outside/dir /w/f(control)['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/w/f', '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/outside/dir']Ground truth:
patch -i <abs> -sd<out> frc=0 leaves the cwd's copy alone and rewritesout/f— the same write as-d out, into the same file.Hole 2 — the clustered output. The sibling on the same walk, and it failed the other
way round: it named the source.
patch -so/outside/out /w/inwas ALLOW, whilepatch -i <abs> -so out.txt frc=0 createsout.txtand leavesfuntouched.Hole 3 — an optional argument (
b::in this host's optstring;strings /usr/bin/patch→
b::B:cCd:D:eEfF:g:i:lnNo:p:r:RstTuvV:x:Y:z:Z). This is the worst of the three, and itis not a mis-named path but no name at all:
while
patch -i <abs> -bsd outside/frc=0 rewritesoutside/f.-bswallowedsd, so no-dwas ever in force; the word after the token was a plain operand, and the reader — takingthe
dfor an ordinary value-taking letter — ate it and named nothing.The change
_patch_cluster_values— the clustered spelling of both path-taking options, split withthis verb's own letters (
_PATCH_CLUSTER_LETTERS, derived from_PATCH_OPTIONS_WITH_VALUE, so a letter added there cannot be read in one spelling andmissed in the other). A token's first letter is deliberately not reported: that is
the plain attached spelling, which the option's own set already reads.
_patch_directory_values/_patch_output_values— the two halves, each filtering theclusters by its own option's letters;
-o's clustered value is added before theoperand fallback, because an output is what displaces the operands.
_short_cluster_option(..., optional_letters=...)— an option whose argument is optionalstops the scan like a value-taking letter, but never eats a word (
attachedis True evenwhen the rest is empty), which is what getopt does with
b::._positional_args(..., cluster_optional_arg_letters=...)— the operand reader passes itthrough, which is what stops it dropping the operand in hole 3. Default empty, so no
other verb's reading changes.
Tests
tests/test_bash_tool_patch_targets.py:OUTPUT_FORMSandDIRECTORY_FORMS, walked by the target-listand both-tier assertions;
-isd/-isocontrols — the scan stops at the first value-taking letter, so acluster that gave its value to another option names no destination, and the word it would
have eaten is not named either (the issue's acceptance item 2);
-brows: the swallowed tail names nothing bogus, andpatch -bsd <outside>/f— thehole in item 3 above — names its operand and is refused at both tiers;
_PATCH_CLUSTER_LETTERSreturns the clustered rows to today'sreading while the attached/spaced spellings survive (the issue's acceptance item 3,
and the reason those two are read from the option's own set); emptying
_PATCH_OPTIONAL_ARG_LETTERSmakes the swallowed tail read as a destination, which isthe wrong name in both of its rows.
Verification
uv run pytest tests/ -q→ 4272 passed, 21 skipped (master collects 4275, this tree4293: +18 new test items);
uv run python -c "from emrg.client.app import run_client";uv run python -m emrg --help;