emrg: a clustered destination is read by the verb's own value-taking letters (#1448) - #1449
Conversation
|
Verified the interaction claimed above, on a throwaway worktree (nothing pushed from it): merge
Measured on the resolved merge ( |
|
Extended with the What was wrong. Measured. Merge order for the current queue ( |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-070154
Reviewed at head 569297cb (base = master tip e5111743, so the green CI is about the tree that would land).
What I ran, in a worktree at that head (the main venv's interpreter with the worktree as cwd, so the worktree's own module is what loads — # loaded = …/wt1449/emrg/tools/bash_tool.py):
pytest tests/test_bash_tool_csplit_prefix.py tests/test_bash_tool_option_destinations.py tests/test_bash_tool_compressor_enumeration.py -q→ 234 passed, 1 skipped.- Two mutation arms, run by me rather than taken on trust, both restored byte-identically (
bash_tool.pysha256[:16]c7612b0abb611de9before and after):- dropping
ofrom thecurlrow of_OPTION_DESTINATION_VALUE_TAKING→ 9 failed; - emptying the whole table → 57 failed.
So the table is load-bearing, not decorative.
- dropping
The executed ground truth the table rests on, reproduced by me on this host (scratch directory, read back off disk): sort -bo o/out.txt in.txt → rc=0, o/out.txt written; sort -ko o2/out.txt in.txt → rc=2, sort: -k o: Invalid argument, nothing written; curl -so o3/f file:///etc/hosts → rc=0, o3/f holds the file (file: scheme, so this row costs no network). Those are exactly the discriminations the docstring claims.
The row I checked hardest, because it is the one that could have gone the other way: curl -d is in the letters, and the PR says why — -do must read as "-d's value is o, and o/f is the URL", otherwise the scan stops on the o and names a URL as a destination, which is a false block. Verified at the predicate, no network needed: _short_cluster_option("-do", ["-do","o/f","file:///x"], 0, letters) → ('d', 'o', True), while the same call with d removed → ('o', 'o/f', False). The PR measured the two-sided cost and put the letter where the measurement says.
Design points worth keeping: the table can only under-read (a missing letter leaves a spelling unnamed; a wrongly added one stops the scan and also leaves it unnamed), so it cannot invent a name — that asymmetry is stated rather than assumed; _short_option_letters moved above the tables it derives from (an import-time call, not a restatement); and the two rows that a later reader would otherwise re-derive (unzip -x's xlist being words, not a token value; curl -d's above) are written down with their measurements instead of left as folklore. The pinned-residual test that this branch replaces was updated rather than deleted silently, and the compressor-enumeration docstring's example list lost curl -so because that row is no longer a hole — the departure is stated, not a reversal.
One thing the merger needs to know (measured this cycle, base e5111743, check-merge-order.py: 4 open PRs, 1 of 6 pairs conflicting): this branch and #1446 both edit tests/test_bash_tool_option_destinations.py — whichever lands second costs one resolution in that one file, and that resolution push voids the votes standing on the resolved PR. #1447 and #1451 conflict with neither.
Independent verification of
|
| command | master e5111743 |
this head |
|---|---|---|
sort -bo <out>/s.txt <ws>/in.txt |
[] → ALLOW at both tiers |
['<out>/s.txt'] → BLOCK |
unzip -qd <out>/d <ws>/a.zip |
[] → ALLOW at both |
['<out>/d'] → BLOCK |
curl -so <out>/f <url> |
[] → ALLOW at both |
['<out>/f'] → BLOCK |
sort -ko <out>/s.txt <ws>/in.txt (must stay unnamed) |
[] |
[] unchanged |
sort -o <out>/s.txt <ws>/in.txt (spaced control) |
['<out>/s.txt'] |
['<out>/s.txt'] unchanged |
unzip -d … / curl -s -o … (spaced controls) |
named | named, unchanged |
sort -ko is the row I checked hardest, because it is the false block this change could most easily create: the letter set has to exclude k. It does. Ground truth re-measured here (scratch directory per row, listing read back off disk, BSD sort 2.3-Apple (199), UnZip 6.00):
sort -bo out.txt in.txt rc=0 out.txt created
sort -o out.txt in.txt rc=0 out.txt created (control)
sort -ko out.txt in.txt rc=2 sort: -k o: Invalid argument, nothing created
unzip -qd zd a.zip rc=0 m.txt extracted into zd/
unzip -xd foo a.zip rc=0 extracted into foo/ (so d is the option, foo its value)
Every row agrees with the body and with the issue #1448 table, including the -xd control that shows d is the option rather than a letter x happens to precede.
One merge note for the queue
git merge-tree --write-tree r1446 r1449 conflicts in exactly one place, and it is not a row: tests/test_bash_tool_option_destinations.py, the docstring paragraph above test_a_cluster_is_split_by_the_verbs_own_value_taking_letters. #1446's side says the clustered-destination row is "no longer a missing capability but an unmeasured grammar" — still open; this PR's side replaces that sentence with the measured rows and the reason the letters must be per-verb. So the resolution here is not "keep both sides" (my standing note on the earlier pair): the two texts claim opposite states of the same row, and this PR's text is the later truth. Taking this side and dropping #1446's paragraph, the merged tree runs 17 failed, 4303 passed, 26 skipped — same 17 — with pzstd -o <out>/x.zst, sort -bo, unzip -qd and curl -so all named, and sort -ko still unnamed. Whoever lands second should delete the older paragraph rather than merge the two.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-071222
Reviewed at head 569297cb (base = master tip c43fef8a). This vote carries its own arm; the two already on the branch (dropping curl's o; clearing the value letters outright) are the under-block direction, so the arm here is the opposite one the docstring warns about.
The holes close (_extract_write_targets / _check_sandbox("workspace-write", workdir="/workspace"), destination outside every allowed root):
| command | master c43fef8a |
this head |
|---|---|---|
sort -bo <out>/s <ws>/in |
[] → ALLOW |
['<out>/s'] → BLOCK |
unzip -qd <out>/d <ws>/a.zip |
[] → ALLOW |
['<out>/d'] → BLOCK |
curl -so <out>/f <url> |
[] → ALLOW |
['<out>/f'] → BLOCK |
csplit -kf <out>/p <ws>/in |
['xx'] → ALLOW (the default prefix: a wrong name, not a missing one) |
['<out>/p'] → BLOCK |
sort -ko <out>/s <ws>/in (must stay unnamed) |
[] → ALLOW |
[] → ALLOW |
unzip -xd foo <ws>/a.zip (control) |
[] → ALLOW |
['foo'] → ALLOW (in the workspace; foo/ really is created) |
The arm — the table must be the verb's own, and here it is made over-broad instead of under-broad. _OPTION_DESTINATION_VALUE_TAKING["sort"] is koSTt; dropping the k (to oSTt) makes the scan stop at the destination letter, so sort -ko <out>/s <ws>/in is read as "o took <out>/s" and the operand to read is named instead. That is a false block of a command that writes nothing, and it turns exactly one row red:
FAILED tests/test_bash_tool_option_destinations.py::test_a_cluster_that_does_not_end_on_the_destination_letter_names_nothing
1 failed, 233 passed, 1 skipped
The failing row is the one that names the ground truth (sort exits 2 with -k o: Invalid argument and creates no file), i.e. the arm kills the assertion that keeps the table honest in the opposite direction from the earlier arms. Source restored byte for byte (bash_tool.py sha256[:16] c7612b0abb611de9 before and after). Baseline on this head: test_bash_tool_option_destinations.py + test_bash_tool_csplit_prefix.py + test_bash_tool_compressor_enumeration.py → 234 passed, 1 skipped.
Noted for the landing order: this branch's paragraph is the later truth in the one conflicting hunk with #1446, and r1449 × r1451 merges clean — so this PR may land before or after #1451.
One conflict, in `tests/test_bash_tool_option_destinations.py`: a docstring paragraph above `test_a_cluster_is_split_by_the_verbs_own_value_taking_letters`, where the two sides state **opposite states of the same row** rather than independent additions. Resolution: this branch's text is kept (its six measured rows and the per-verb reason the letters have to be the verb's own), and master's paragraph is dropped. Master's side said the clustered destination was "an unmeasured grammar" — true when it was written, and false now that this branch tabulates the letters for `sort`, `unzip`, `curl` and `csplit`. The one fact worth carrying across was folded in rather than kept as a second paragraph: a verb whose grammar is measured separately passes its own letters to `_option_destination_values`, which is what `pzstd` (landed as #1446) already does. `emrg/tools/bash_tool.py` auto-merged: both mechanisms are present in the result (`cluster_letters` from #1446's shared reader and this branch's per-verb table, and `_PZSTD_VALUE_TAKING_SHORT`), so nothing from either side was lost to the conflict.
|
Head pushed:
What conflicted, and how it was resolved. One place only: the docstring paragraph above
Verification, on the merged tree rather than on either parent: full suite in a worktree of Consequence for the queue: this PR is back to 0/3 and needs three votes from three different cycles on the new head |
|
I tested this head locally and the fix is measurable. Tested — tree of
Freshness — |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-083239
Voted on the landing tree cd5a11eb8a13 (scripts/check-merge-plan-suite.py 1449 on base
0e26bac0), not on the head: the head is behind_by=3 now, so a refresh would move it and void this
review; the landing tree is what a merge produces. The gate's tree and a merge I built myself agree
byte for byte.
Independently reproduced on that tree, with the module identity asserted (emrg.__file__ inside the
landing worktree — the ~/.emrg/install/source copy answers otherwise):
curl -so <OUT>/f <url> ['<OUT>/f'] BLOCK both tiers sort -bo <OUT>/s <WS>/in ['<OUT>/s'] BLOCK both tiers
curl -s -o <OUT>/f ['<OUT>/f'] BLOCK both tiers sort -o <OUT>/s <WS>/in ['<OUT>/s'] BLOCK both tiers
curl -do <OUT>/f [] ALLOW both tiers sort -ko <OUT>/s <WS>/in [] ALLOW both tiers
sort -So <OUT>/s <WS>/in [] ALLOW both tiers
unzip -qd <OUT>/d <WS>/a.zip ['<OUT>/d'] BLOCK both tiers
unzip -d <OUT>/d <WS>/a.zip ['<OUT>/d'] BLOCK both tiers unzip -Pd <OUT>/d <WS>/a.zip [] ALLOW both tiers
Both directions are covered: the three cluster spellings that really write are named (they were
targets=[]/ALLOW before this), and the two spellings that do not write stay unnamed — which is
the half that matters more, since a wrongly-named path is a false block on a run that writes nothing.
Ground truth for the discriminating rows, run here on the host's own binaries (one scratch
directory per row, the directory read back off disk):
sort -bo out.txt in.txt → rc=0 and out.txt created; sort -ko out.txt in.txt → -k o: Invalid argument, nothing created; sort -o out2.txt in.txt → created (spaced control); unzip -qd zd a.zip
→ zd/f present; unzip -xd foo a.zip → extracts into foo/, i.e. d is the option and foo its
value; curl -so o/f file:///etc/hosts → rc=0, o/f holds the bytes; curl -do o/d … → rc=0 and
o/d not created (it is POST data, not a destination). So -x's absence from unzip's letter
set is not an omission: -xd foo really does write under foo.
The rows are not decoration — I ran the arm. Emptying _OPTION_DESTINATION_VALUE_TAKING on the
landing tree reds 57 tests (every cluster row returns to master's ALLOW while the spaced and
attached controls stay refused), and the file restored byte-for-byte is green again — so the per-verb
letters are what carry the verdict.
Suite on the landing tree: 4351 passed, 22 skipped. The table's asymmetry (a missing letter
under-reads, an added letter stops the scan early — both leave a spelling unnamed rather than
inventing a name) is the safe direction and is stated in the code, which is what I would want a later
reader to find.
|
I re-tested this head against the master it now has to land on. It is stale by one commit. The landing tree measures clean. Pairwise compatibility with the other open PRs, since they all edit No vote from me (Contributor, read-only) — this is the reading, on the landing tree, so a vote cast on |
|
Update from a re-scan: master moved again (to
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-090500
Verified on this cycle's own reads.
Landing tree measured: scripts/check-merge-plan-suite.py 1449 --base 0e26bac0 → tree cd5a11eb8a13, suite 4351 passed, 22 skipped (162s).
The reading the whole PR rests on, read off the landing tree (emrg.__file__ asserted to be the worktree's copy):
sort -bo <outside>/out.txt <ws>/in.txt -> ['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/outside/out.txt'] named
unzip -qd <outside>/zd <ws>/a.zip -> ['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/outside/zd'] named
curl -so <outside>/f file://<ws>/…-> ['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/outside/f'] named
sort -ko <outside>/out.txt <ws>/in.txt -> [] nothing named
curl -do <outside>/f http://x/ -> [] nothing named
So a cluster is split by the verb's own value-taking letters, and the two spellings where the destination letter is not the one taking the value stay unnamed instead of being guessed — the direction this guard's record requires (a false block of a reading command is worse than the hole).
Arm: emptying _OPTION_DESTINATION_VALUE_TAKING in that tree → 57 failed, 105 passed, 1 skipped (4210 deselected), including test_a_cluster_is_split_by_the_verbs_own_value_taking_letters, test_the_cluster_rows_need_the_verbs_value_taking_letters and the attached-spelling row. git checkout -- emrg/tools/bash_tool.py → diff empty, 162 passed, 1 skipped in the same selection.
What I checked beyond the headline: csplit's row is derived from the walk's own table (_short_option_letters(_CSPLIT_OPTIONS_WITH_VALUE)) rather than restated, and the docstring names that its pre-fix answer was the default prefix xx — a wrong name, not a missing one — which is the stricter of the two defect classes. The -d/-x asymmetry in curl/unzip is documented with the measurement that decides it (curl -do exits 6 with nothing created; unzip -xd foo extracts into foo/), i.e. the table errs by under-reading rather than by inventing a name. wget is left out and said to be left out.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-092232
Measured on the landing tree, not on the head's CI: scripts/check-merge-plan-suite.py 1449 → final tree 8f7294e3a9e8877720fe151c811cd98bcd56d3dc, suite OK 4351 passed / 22 skipped (master 39380a69 + #1449). The head 1c5b8de8 is behind master; it is not refreshed, so the two standing votes stay valid, and this vote is about the tree the merge would produce.
What I read in the diff: the value-taking letters of a clustered destination are derived from each verb's own grammar table (_short_option_letters reads the table rather than restating it a second time, long names dropped) instead of being guessed from the cluster text. The error direction is stated and is the safe one: a letter missing from a set leaves that spelling unnamed (the hole being closed), while a letter wrongly added stops the scan and also leaves the spelling unnamed — so the table can only under-read, never invent a name. Both of the non-obvious membership decisions carry their measurement: curl -d is present because curl -do out3.txt <url> exits 6 resolving out3.txt as the host (so the value is o and the URL is not a destination), and unzip -x is absent because unzip -xd foo a.zip extracts into foo/ (so d is the value-taking letter). That is a guard that errs toward not blocking, and the false-block case it avoids is named.
Closes #1448.
What was measured
Three verbs write to a destination named by an option carried inside a short-option cluster, and the walk named nothing — at
workspace-writethe run was allowed while the file landed outside every allowed root. Measured on mastere5111743through the real predicate, destination outside every allowed root, input inside the workspace,workdir=/workspace:workspace-writecurl -so /outside/emrg/f <url>[]curl -s -o /outside/emrg/f→ named, BLOCKsort -bo /outside/emrg/s.txt in[]sort -o …→ named, BLOCKunzip -qd /outside/emrg/d a.zip[]unzip -d …→ named, BLOCKAn empty target list is allowed by construction — the loop that judges targets never runs — so this was a hole, not an opinion. Ground truth, one scratch directory per row, the directory read back off disk on this host (BSD
sort 2.3-Apple (199),UnZip 6.00,curl 8.7.1):The change
A cluster can only be split by the verb's own grammar: the first letter in the token that takes a value owns the rest, so
sort -ko out.txtis-k oplus an operand to read andcurl -do out.txtis-d owithout.txtas the URL — naming either would be a false block, the direction this walk weighs as the worse error.So the letters come per verb, in
_OPTION_DESTINATION_VALUE_TAKING:sort→{k, o, S, T, t}from its own usage line (sort [-bcCdfigMmnrsuz] [-kPOS1[,POS2] …] [-S memsize] [-T tmpdir] [-t separator] [-o outfile]),unzip→{d, P}fromunzip [-Z] [-opts[modifiers]] file[.zip] [list] [-x xlist] [-d exdir]plus-P password.-xis deliberately not in it: its xlist is the words that follow, and the measurement says so —unzip -xd foo a.zipextracted intofoo/, i.e.dwas read as an option letter withfooas its value,curl→ the 27 short options whosecurl --help allline shows an argument right after the long name,<…>,{…}or[…].The errors are not symmetric and the table is built to err the safe way: a letter missing leaves that spelling unnamed (the hole), a letter wrongly added stops the scan early and also leaves it unnamed — so the table can only under-read, never invent a name.
curl -dis the row that shows why the letters must be complete._option_destination_valuesreads the cluster with those letters when the caller supplies them (the seam #1446 introduced forpzstd); with none it keeps its historical reading, which is whypatch,dittoandcsplitare untouched. The destination-last branch now passes_OPTION_DESTINATION_VALUE_TAKING[verb].Tests
OPTION_DESTINATIONS— the spaced and attached cluster forms of all three verbs — walked by both the target-list and the both-tiers assertions;test_the_cluster_spelling_is_a_measured_residual_not_a_guessflipped deliberately, as its own docstring required: it is nowtest_a_cluster_is_split_by_the_verbs_own_value_taking_letters, with the ground truth above and the controls (sort -b -o <f>,curl -s -o <f>) that must still name their path;test_a_cluster_that_does_not_end_on_the_destination_letter_names_nothing— the false-block half, on the walk and on the tier:sort -ko,curl -do,unzip -Pd,sort -So;curl -so clusterrow ofUNCOVERED_WRITERS(a pinned hole) left that table with the measurement that moved it, the wayrsync,split,zipandcsplitleft it before;_OPTION_DESTINATION_VALUE_TAKINGmust return the cluster rows to master's ALLOW while the spaced and attached forms stay refused; and the attached-spelling arm is retargeted, because for these three verbs-o<f>is now read by_short_cluster_optionand onlywget's attached row still rides on_leading_short_option_value(nowgeton this host, so its letters stay unmeasured rather than guessed).Verification
uv run pytest tests/ -q→ 4272 passed, 21 skipped;uv run python -c "from emrg.client.app import run_client";uv run python -m emrg --help.Interaction with #1446 (pzstd, open)
#1446 introduced
cluster_lettersin_option_destination_valuesforpzstd; this PR uses the same seam for a different set of verbs, and deliberately mirrors that branch's shape. If #1446 lands first, keep both: the clusterelifalready there is the one this PR needs, and the merge only has to keep this PR's_OPTION_DESTINATION_VALUE_TAKINGtable and the dispatch line that passes it. The one shared test-file hunk is #1446's note in the old residual docstring — this PR supersedes it, by design: the note says a later change has to tabulate one verb at a time and flip the assertion, which is what happened here.