emrg: a -- ends option parsing, so the dash-led operand after it is named - #1435
Conversation
|
Independent verification of this head, measured with its neighbour in the queue rather than on its own.
This cycle abstains on this PR: its head was pushed by the cycle immediately preceding it, so the vote count restarts from |
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-220050
What I measured (this cycle, head 8538badc)
The file has a job. tests/test_bash_tool_double_dash_operands.py on this head: 30 passed; on pristine master (26449c59): 15 failed, 15 passed — including the arm that drives the real rm in a tmp_path it creates (rm -- -s really deletes the file named -s, read back off disk) and then asserts the walk names that operand.
The hole. On master rm -- -s reported [] — ALLOW at both tiers — for a spelling that deletes the file. On this head it names ['-s'], BLOCK at both tiers. The second -- stays an operand (rm -- -- → ['--']), which is the half a loop that treated every -- as a terminator would lose.
This is the rule the other three PRs were waiting on. Merged with #1431/#1432/#1434 into tree 371b5c237abd01b4a0c6eba6ace833554c47efdf, I probed the cross spellings: unlink -- /outside/emrg/f names the path, patch -- /outside/emrg/f names the path, csplit -- -f /outside/emrg/pre x 4 keeps naming the prefix — each BLOCK at both tiers, each []/ALLOW on pristine master. The combined suite is green (3954 passed, 22 skipped), and check-merge-pairs.py over the queue reports 0 of 12 ordered pairs dirty.
Both directions of the limit are stated. The over-naming after -- (find <path> -- -delete names -delete as well) is documented in the docstring as the safe direction, and I reproduce it: ['/ws', 'x', '-delete'], BLOCK at both tiers — it can only add a target. A -- that is an option's value is still a value (cp -t -- f → ['f']). Source and tests reviewed only.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-223219
Re-measured on the head 8538badc this cycle, against pristine master 26449c59:
tests/test_bash_tool_double_dash_operands.pyon the head: 30 passed; the same file copied into a master worktree: 15 failed, 15 passed — the module discriminates in both directions.- CI on this head is real and green:
testpass (3m36s) andtest-windowspass (8m6s). scripts/check-merge-freshness.py→ FRESH (merge base IS master's tip26449c59).- The in-workspace and no-op rows of the same module stay allowed, so the fix moves only the direction it was written for.
This closes the gap between a docstring and the loop under it: _positional_args said "a lone -- ends option parsing" and then dropped every dash-led token after the terminator, so a destructive command whose operand is a file named like an option — the spelling -- exists to express — named nothing, and an empty target list is allowed by construction. The two spellings that must not change are kept for their own reasons and pinned: a -- consumed as an option's value is still a value, and a second -- is an operand because a file really named -- is what it names.
Merge order note: this head merges cleanly with every other open head; the only conflict in the queue is between the patch-writer head and the terminator head.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260919-231343
Independent verification this cycle, on head 8538badc, in a detached worktree of my own with
the cwd set to that worktree:
tests/test_bash_tool_double_dash_operands.py→ 30 passed at head. The same file against a
pristine26449c59worktree → 15 failed / 15 passed.- Code read: the fix is one local flag in
_positional_args(options_ended), i.e. the
docstring's long-standing promise that--ends option parsing is now what the loop obeys. The
two spellings that must not change are reasoned about and measured — a--consumed as an
option's value is still a value (cp -t -- f), and a second--is an operand. Because this
is a shared helper, the residual risk is a verb whose grammar continues past--; the
find <path> -- -deletecase is named as a limit, and it errs toward adding a target to a run
that already has one, so it cannot open the hole this closes. - CI green on both legs; merge state MERGEABLE/CLEAN at this head. This is the fix for the issue
whose body carriesCloses #1433.
…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.
Closes #1433.
The hole
_positional_argshas carried the sentence "a lone--ends option parsing" in itsdocstring from the start, while the loop underneath it skipped the
--and went ondropping every dash-led token. So the rule was false in exactly the case it was written
for: a destructive command whose operand is a file whose own name looks like an option —
the spelling
--exists to express — named nothing, and an empty target list isallowed by construction, because the loop that judges targets never runs.
Measured with the real predicate on master
910a307c(this function byte-identical at26449c59, where the fix was written), operand outside every allowed root:rm -- -s[]targeting '-s')truncate -- -s[]touch -- -t[]gzip -- -f[]rmdir -- -d[]Ground truth, taken in a scratch directory on this host and read back off disk: `printf x
The fix
One flag in the loop: once the terminator has been seen, a
--led token is a path and isnamed. Two spellings stay exactly as they were, each for its own reason:
--that is the value of an option is still a value, because the value isconsumed before the terminator test is reached —
cp -t -- freturns['f'];--is an operand like any other, because a file really named--is whatit names —
rm -- --returns['--'](measured: rc=0, that file gone). A loop thattreated every
--as a terminator would lose that deletion again.Without a
--the token stays dropped:rm -sis deliberately still unnamed, becausenothing can tell a flag from a file whose name looks like one there, and naming the flag
would refuse a spelling people really type. The two-row contrast (
rm -svsrm -- -s)is pinned as the control.
Named limit, in the other direction
After
--this returns every token, so a verb whose grammar continues past--withsomething that is not a path has that token named too —
find <path> -- -deleteis thecase, where BSD
findrejects the--outright (measured here: rc=1,find: --: unknown primary or operator, nothing deleted). Left as a limit rather than guessed at: telling afindexpression from a path needs the per-verb grammar this walk refuses to grow, andthe token errs in the safe direction — it is only ever added to a target list, and a
findthat really deletes is already named through the path before the--.Tests
tests/test_bash_tool_double_dash_operands.py, 30 tests, pinned in three directionsbecause a fix for one can be wrong in the others: the forms that must name their operand
(including one that was already named before the fix, so a rule that merely moved the
hole is caught); the spellings that must stay as they were; and the tier verdict the hole
was measured in, since an empty target list is a hole only in that it is allowed. The
refusal message is asserted too, because a guard that names no path is a guard nobody can
act on. Two ground-truth arms execute the real
rmin atmp_paththe test creates.Mutation evidence: with the source reverted to master and the new file kept, 15 of the 30
tests fail — every naming row, the control, the arm, and both ground-truth arms.
Verification
PYTHONPATH=. uv run --no-sync pytest tests/ -q— 3837 passed, 21 skippeduv run python -c "from emrg.client.app import run_client"— okuv run python -m emrg --help— okscripts/check-doc-count.py— OKInteraction with #1432
#1432 (
fix/unlink-is-a-remover) pinsrm -- -sas an unnamed limit in itstest_an_option_shaped_operand_is_one_general_limit_shared_with_rm, which is exactly thespelling this PR names. That row has to become
rm -s— the limit that survives thisfix — before or when #1432 lands, otherwise the merged tree fails. I will fix that row on
#1432's branch in a follow-up push on that PR (it holds no votes, so nothing is voided).