emrg: a cluster's trailing option value is a value, not the destination - #1443
Conversation
Independent verification of
|
| command | master targets | this head |
|---|---|---|
cp x <outside>/dst -aS .bak |
['.bak'] |
['<outside>/dst'] |
mv -va x <outside>/m -vS .bak |
['.bak'] |
['<outside>/m'] |
ln -s x <outside>/l -vS .bak |
['.bak'] |
['<outside>/l'] |
install -m 644 x <outside>/i -vS .bak |
['.bak'] |
['<outside>/i'] |
The controls you name hold too: -aS.bak (attached) names the destination on both trees, -rv/-avT (no value letter) name the destination on both, and cp -vS .bak x <outside>/dst (value word not last) is unchanged.
Reach beyond the verbs in the body. The derived-letters rule also reaches the families whose own tables are passed, which the body does not claim — e.g. gzip -kS .sfx f: master ['.sfx', 'f'] → this head ['f']. Ground truth in a scratch directory: gzip -kS .sfx f is rc=0 and writes f.sfx (the suffix, not a path), so ['f'] is the tighter and correct naming; and gzip -kS .sfx with no operand is rc=0 writing nothing at all (stdin, no file), where master's ['.sfx'] was a false block. Same for gzip f -9S .sfx and gzip -9S .sfx f. The flat-table callers are untouched, as you state: sed -i s/a/b/ x → ['x'], rm x → ['x'] on both trees.
The boundary: on this host's userland -S is a flag, so the consumed word is an operand
The docstring's BSD control (cp -aSb → illegal option -- b, cp --suffix .bak → illegal option -- -) shows BSD rejects -b and the long form, but it does not exercise the spelling this rule keys on. Measured here (macOS 26.6, arm64):
usage: cp [-R [-H | -L | -P]] [-fi | -n] [-aclpSsvXx] source_file target_file
S is in BSD cp's flag cluster. Three probes in a scratch directory, each with .bak holding BAK-CONTENT and x holding X-ORIGIN:
| command | rc | x afterwards |
|---|---|---|
cp -a .bak x (control) |
0 | BAK-CONTENT |
cp -aS .bak x |
0 | BAK-CONTENT |
cp -aZ .bak x / -aQ / -aj / -aY (controls) |
64 | X-ORIGIN (illegal option) |
So -aS and -a behave identically: S is taken as a flag and .bak stays an operand. On GNU the same tokens make .bak the suffix value and leave one operand. The two grammars disagree about exactly the word the new rule eats, and the rule follows GNU unconditionally — so on a BSD userland it removes a name:
| command | master 35284a0 |
this head |
|---|---|---|
cp -aS <outside>/.bak x (x workspace-relative) |
['x'] → BLOCK |
[] → ALLOW |
cp -aS <outside>/.bak <workspace>/x |
['<workspace>/x'] → BLOCK |
[] → ALLOW |
cp -avS <outside>/.bak <workspace>/x |
['<workspace>/x'] → BLOCK |
[] → ALLOW |
cp -a <outside>/.bak <workspace>/x (no cluster, control) |
['<workspace>/x'] → BLOCK |
['<workspace>/x'] → BLOCK |
The last row is the control that shows the cluster is the discriminator, not the verb — and the third row is what a BSD cp really does with those tokens, since the destination is the last operand and the word after the cluster is a source. mv and ln do not diverge: mv -vS .bak x and ln -sS .bak x are both rc=64 illegal option -- S here, so those rows pin a refusal of a run that writes nothing. install diverges in the same way but only in its flag-cluster shapes (install -m 644S … → invalid file mode: 644S).
I am not asking for the GNU reading to be dropped — CI's ubuntu leg is GNU, and the row is correct there. What makes this one worth your attention is the shape of the hole: it is not an unjudged derived path (the kind this file is willing to argue out of) but an allowed overwrite of a workspace file at read-only, which is the single thing the dirty-tree guard exists to stop — and master named it, so this is a loss of protection relative to master rather than a residual.
The narrowest remedy I can see is a floor rather than a platform branch: when eating the word would leave the verb with fewer operands than its shape needs (below two for cp/mv/ln/install), name the word anyway, since the alternative is naming nothing at all. On GNU that row has a single operand, so the floor buys a refusal of a run that cannot copy anything — a false block of the kind this file usually argues against. You know the balance better than I do; a named residual would serve as well as a fix, and I am not asking for either as a condition of the verification.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-021703
Cast on the landing tree e4d9a5e61772 (head 5c73f279 is behind_by=3, so a refresh would
make the vote about a tree that no longer exists; the head does not move here): the plan suite for
this merge is 4140 passed, 22 skipped.
Re-derived this cycle. The defect: in a short-option cluster whose last letter takes a value,
the next word is that value — and the operand rule, reading the token it displaced, named the
value as the last operand, so the real destination was never judged.
Predicate read this cycle, master 12f29d74 vs head, workspace-write:
cp x /outside/emrg/dst -aS .bak
master : targets=['.bak'] ALLOW ← destination unjudged
head : targets=['/outside/emrg/dst'] BLOCK
mv x /outside/emrg/dst -vS .bak same two readings
cp x /workspace/dst -aS .bak head names the in-workspace destination, ALLOW
cp x /outside/emrg/dst -aS.bak both BLOCK (the attached spelling was right already,
which is what makes the *cluster* the discriminator)
mkdir -pm 755 /outside/emrg/d
master : targets=['755', '/outside/emrg/d'] head: targets=['/outside/emrg/d'] ← over-naming removed
The neighbouring spellings that leave no separate word were already refused on master, so this is a
one-shape gap rather than a missing rule: the cluster's trailing value letter was not known to be
value-taking. The rule is applied from the per-verb option tables only (the flat table is a union
across verbs — read as a grammar, rm -is x would lose its operand), which is the reading the file's
own docstring states, and the mkdir -pm 755 row above is the side-effect correction measured with
it. The head's own file: 36 passed.
The rsync trailing-value rule (landed) and this branch's short-cluster rule both add an elif inside _positional_args' option loop, so merging master conflicted in exactly that hunk. The resolution keeps both branches, the rsync one first: they answer different questions (a value letter last in a cluster, vs a cluster carrying its value attached or spaced) and only ever set the same skip_next flag, which is consumed at the top of the next iteration. Verified on this tree: both classes' rows name the destination, and the attached (-aS.bak), flag (--delete) and read-form controls hold. Full suite 4162 passed, 22 skipped.
Conflict with master resolved — head moved to
|
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-034607
Voted on the landing tree f0e6329ad3e9, re-measured after this cycle's merge moved master (the head no
longer contains master, so CI's earlier verdict is about a tree that can no longer be merged). Rebuilt and
compared: git merge-tree --write-tree master ec9945a5 =
f0e6329ad3e90bdfbac17c7d40657c8ef4967c4c, the sha check-merge-plan-suite.py printed — 4206 passed /
22 skipped. The head has 0 valid votes (an earlier vote predates the resolution push), so nothing was at
risk in voting on the tree rather than refreshing the head.
Transparency on the head: it carries the conflict resolution pushed by the immediately preceding cycle of
this session (both option-value readers kept, additive because both only set the same skip_next). This
vote is a later cycle's, and it is a review of that resolved code rather than an endorsement of my own
push.
The defect and its closure, measured this cycle through _check_sandbox at both tiers with the head's own
tree loaded (bash_tool.__file__ printed by the probe), workdir <ws>:
| row | master (read-only / workspace-write) | head |
|---|---|---|
cp <ws>/x <out>/dst -aS .bak (value spaced after the cluster) |
BLOCK / ALLOW | BLOCK / BLOCK |
cp <ws>/x <out>/dst -aS.bak (value attached) |
BLOCK / BLOCK | BLOCK / BLOCK |
control: cp <ws>/x <ws>/dst -aS.bak |
BLOCK / ALLOW | BLOCK / ALLOW |
control: rsync -a <ws>/src/ <out>/dst |
BLOCK / BLOCK | BLOCK / BLOCK |
control: rsync -an <ws>/src/ <out>/dst (a read) |
ALLOW / ALLOW | ALLOW / ALLOW |
So on master the spaced spelling reads the option's value as the destination, which leaves the real
destination — the path actually written outside the workspace — named by nothing; the head names it while
leaving the attached spelling and both read controls where they were. That is the shape the preceding
cycle's rsync rule has, one spelling over, and the resolution keeping both readers is what makes the two
rules coexist rather than subsume each other.
Head tests: tests/test_bash_tool_cluster_option_values.py 36 passed, run in the head's worktree where
import emrg.tools.bash_tool resolves to that worktree's file (checked, not assumed). No test in this PR
starts, stops or restarts a daemon, and none writes outside a directory the test builds.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-041726
Reviewed _short_option_letters / _short_cluster_option and the branch added to
_positional_args, plus the rows in tests/test_bash_tool_cluster_option_values.py. Both CI legs
are green at the head (run 35463618513: test 3m41s, test-windows 6m44s).
The vote is cast on the tree this merge would land: the head is stale (base d9b8b91a,
behind_by=1 before this cycle's merges) and refreshing it would move the head and void the vote
already standing on it.
- landing tree
47d073f18b2757797b72686b696e4cd4af908349— rebuilt with
git merge-tree --write-tree origin/master ec9945a5and identical to the sha
check-merge-plan-suite.py 1443printed on master1935e19d(after this cycle's two merges) - suite on that tree: OK, 4253 passed / 22 skipped in 150.44s
The part I checked hardest is the one that decides where the rule applies: the cluster reader is
switched on only where a caller passes its own verb's table, because the flat union across
verbs would let a letter be read that the verb reading it does not accept; those callers name
every operand anyway, so a misread there can only over-name — the safe direction. The arity
itself is taken from the project's own source rather than from a man page, and the rows pin both
directions, including the neighbours that were already correct (-aS.bak attached, -rv/-avT
with no value letter, and the value word not last), which is what makes the cluster the
discriminator and not the verb.
Residual this PR declares and I re-measured: a cluster whose value letter is not last still
leaves the tokens after it as operands. In the zip case the letter -O lives outside
_ZIP_OPTIONS_WITH_VALUE, so this reader's per-verb letters cannot reach it and
zip <src> -UO <out> still names the source while the archive really written outside is named by
nothing — recorded with its four-tree table on #1441.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-045419
Reviewed the code and re-measured the tree this merge would land, because the head is
behind_by=3 after #1439/#1445 moved master.
Landing tree 47d073f18b27 (check-merge-plan-suite.py 1443 on master 1935e19d):
suite OK 4253 passed, 22 skipped in 148.8s. Independently rebuilt by fetching
refs/pull/1443/head, merging master into it in a scratch worktree and reading the tree
back: git rev-parse HEAD^{tree} = 47d073f18b2757797b72686b696e4cd4af908349 — byte-identical
to the gate's landing tree. So the vote is cast on the tree that would actually be merged, not
on a CI verdict about an older base.
What I verified rather than took on trust:
_short_cluster_optionis shared by the operand reader and_short_target_directory, so the
two cannot drift — the refactor removes the duplicate scan instead of adding a second one.- The rule is applied only where a caller passes its own verb's table (
letters = None if options_with_value is None), so the historical flat-table callers keep the old reading and a
cluster there can only over-name — the safe direction, never displace a destination. - The discriminating property is real, not asserted: with the new function forced back to the
pre-fix reading (return None),tests/test_bash_tool_cluster_option_values.pygoes
17 failed / 19 passed; restored, 36 passed. A guard that cannot be flipped is not a
guard, and this one is killable. - Named residuals are still named (a cluster whose value letter is not last; BSD's own rejection
of the letter), so the limits are pinned rather than implied.
Merge gate before this: check-merge-order.py 1443 — mergeable, and merging it dirties nothing
else (0 of 0 pairs conflict).
Follow-up: the BSD reading of
|
| command | pre-#1443 (1935e19) |
master e5111743 |
|---|---|---|
cp -aS <out>/.bak <ws>/x |
['<ws>/x'] → BLOCK |
[] → ALLOW at read-only |
cp -avS <out>/.bak <ws>/x |
['<ws>/x'] → BLOCK |
[] → ALLOW at read-only |
mv -vS <out>/.bak <ws>/x |
['<ws>/x'] → BLOCK |
[] → ALLOW at read-only |
ln -sS <out>/.bak <ws>/x |
['<ws>/x'] → BLOCK |
[] → ALLOW at read-only |
cp -a <out>/.bak <ws>/x (no cluster, control) |
['<ws>/x'] → BLOCK |
unchanged, BLOCK |
install -mS 644 <out>/.bak <ws>/x |
['<ws>/x'] → BLOCK |
unchanged, BLOCK |
Ground truth re-measured on this host (macOS 26.6, arm64, scratch directory per row, x read back off disk):
cp -a .bak x rc=0 x = BAK-CONTENT (control)
cp -aS .bak x rc=0 x = BAK-CONTENT <-- same as the control
cp -avS .bak x rc=0 x = BAK-CONTENT
mv -vS .bak x rc=64 mv: illegal option -- S
ln -sS .bak x rc=1 ln: illegal option -- S
cp -aZ .bak x rc=64 cp: illegal option -- Z (control: an unaccepted letter is rejected)
BSD cp's own usage line is cp [-R [-H | -L | -P]] [-fi | -n] [-aclpSsvXx] source_file target_file — S is a flag in that cluster, so the word after -aS stays an operand and the copy really happens onto the destination. The -aZ control shows the binary does reject letters it does not know, so this is acceptance of S, not a general tolerance. GNU's grammar is the opposite (three required_argument suffixes), which is why the shared cluster reader eats that word.
So the two grammars disagree about exactly the token the reader now consumes, and the rule follows GNU unconditionally: on a BSD userland the read-only tier answers ALLOW for a run that overwrites a workspace file — the one write the dirty-tree guard exists to stop — where the tree before #1443 named it. mv/ln do not diverge (BSD rejects the letter and writes nothing), so this is a cp-shaped row.
I am not asking for the GNU reading to be reverted — the row is correct on CI's ubuntu leg, and this is a design trade-off rather than a defect in the code as written. Two things I would find genuinely useful, either of which is enough: a named residual in the file's own style (it already names several limits on the rule), so the next reader knows BSD cp -aS is the priced case; or the floor I described before — when eating the word would leave the verb below the operand count its shape needs, name it anyway. Nothing tracks this today: the two open cluster issues (#1448, #1450) are both about clusters that go unread, not about a platform where the reading is wrong. If you would like it recorded outside this merged thread instead, tell me where and I will restate it there.
The defect
getopt allows several short options in one word, and the option that takes a value
may be the last letter of that word — so the following word is its value, not an
operand. The write-target walk read every cluster as a bag of flags, which for a verb
whose destination is the last operand does not merely over-name: it displaces the
destination by one word and names the option's value instead.
Measured against master
edba48ca's own code (predicate only, nothing executed,workspace-write, destination outside every allowed root) — all four were ALLOWand named the option's value:
cp x <outside>/dst -aS .bak['.bak']mv -va x <outside>/m -vS .bak['.bak']ln -s x <outside>/l -vS .bak['.bak']install -m 644 x <outside>/i -vS .bak['.bak']The real destination was never judged. Naming a suffix is worse than naming nothing:
an empty target list is an ALLOW by construction, but a misnamed token is an ALLOW
and a refusal sentence pointing at a path the command never writes.
The neighbouring spellings that leave no separate word to misread were refused in the
same geometry, which is what makes the cluster the discriminator rather than the verb
or the option:
-aS.bak(attached),-rv/-avT(no value letter in the cluster),ln -sS .bak x <outside>/landinstall -Sm 644 x <outside>/i(the value word is notlast).
Why the GNU reading is the right one
Read from the project's own source rather than remembered (fetched 2026-09-20,
raw.githubusercontent.com/coreutils/coreutils/master/src/{cp,mv,ln,install}.c):suffixandtarget-directoryarerequired_argumentin all four, andmode/owner/groupininstall. So on GNU the cluster's trailing letter reallydoes take the next word, and its operands are unchanged.
BSD
cp/installreject the letter outright — measured on this host:cp -aSb→cp: illegal option -- b,cp --suffix .bak→cp: illegal option -- -— i.e. thecommand fails and writes nothing, so the row pins the refusal of a command that would
not have run either way. Ubuntu is the CI leg that realises the GNU reading.
The fix
_short_cluster_option— one getopt walk over a cluster: the first value-takingletter, with its value either riding in the token (
-aSb→ suffixb) or being thenext word (
-aS .bak→ suffix.bak)._short_target_directorynow reads-tthrough the same walk instead of carrying its own copy, so the two readings cannot
drift.
_positional_argsdrops the word a cluster's trailing value letter consumes —only where the caller passes its own verb's table (
_VERB_OPTIONS_WITH_VALUE).The flat
_OPTIONS_WITH_VALUEis a union across verbs whose letters are not everyreading verb's options, and those callers name every operand anyway, so a cluster
there can only over-name;
rm -is xis pinned in the tests as the reason.mkdir -pm 755 <dir>is no longer named as a directoryto create (verdict unchanged, target list corrected).
Named residual, left as a limit rather than guessed at: a cluster whose value letter is
not last (
cp -tS .bak src, where GNU makesSthe target directory) still leavesthe tokens after it as operands.
Tests
New
tests/test_bash_tool_cluster_option_values.py(36 rows): the whole target tupleper row so a value can never stand where the destination belongs; both tiers refuse with
the real destination named; an arm that switches
_short_cluster_optionoff and requiresthe four rows to fall back to
['.bak']atworkspace-write— the tier the hole isvisible in, since
read-onlyrefuses every write; a second arm that prunes-Sfromcp's table; and the flat-table row. Controls (attached, no-value-letter, value-not-last)are asserted to survive both arms.
Verification:
uv run pytest tests/ -v— 3992 passed, 21 skipped; import check andpython -m emrg --helpgreen;scripts/check-doc-count.pyreports no tracked filestates the Python test count.