emrg: a trailing rsync option value is a value, not the destination - #1442
Conversation
A value-taking option that follows the operands puts its value in the last non-option position, which is the position the last-operand rule reads as DEST: `rsync -a src/ /out/dest/ --exclude pat` named `pat` and was allowed at both tiers, while the same command without the trailing option was refused. The rsync branch passed the empty option table, so no option's value was consumed anywhere in the run. Add `_RSYNC_OPTIONS_WITH_VALUE` (long spellings measured on the installed openrsync against flag controls; GNU-only spellings kept because CI runs GNU rsync) and the derived `_RSYNC_SHORT_VALUE_LETTERS` for the clustered form (`-ve ssh`), and pass them from the rsync branch. `_positional_args` reads clusters only when a caller asks for the letters: `cp -at <dir> src` is read by `_target_directory_values`, which does not parse clusters, so a global rule would leave one operand and name nothing.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-014221
Re-verified independently on master 35284a01 vs the head 239959b8 (both trees imported side by side),
and the crux ground truth executed here on this host's own implementation — which matters, because the
installed rsync is openrsync (rsync version 2.6.9 compatible, protocol 29), not GNU rsync:
| command | executed | master | head |
|---|---|---|---|
rsync -a src/ dst/ --exclude pat |
rc=0, dst/f.txt really lands |
names pat, workspace-write ALLOWS while the write goes outside |
names /outside/emrg/dst, refused |
rsync -a src/ dst/ -e ssh |
rc=0, file lands (trailing -e accepted) |
names ssh, ALLOWS |
names the destination, refused |
rsync -a src/ /outside/emrg/dst (control) |
— | names the destination | unchanged |
rsync -a --exclude pat src/ /outside/emrg/dst (control) |
— | names the destination | unchanged |
rsync -a src/ /outside/emrg/dst --delete / -avz (flag controls) |
— | name the destination | still name the destination |
Two things I checked because they are where this class usually goes wrong. First, the hole is only visible
at workspace-write: at read-only both trees refuse, because any named target is refused there — so the
"ALLOW" in the table above is measured at the tier that exposes it. Second, the trailing form is not a GNU
curiosity: openrsync really does accept an option after the operands and really writes to the destination,
so the fix is about the tool that ships here, not about a documented grammar alone.
The flag controls are the discriminating half — --delete, --stats, -avz name the destination on both
trees, so the new table is not a table that consumes any operand it meets. The opt-in boundary holds too:
cp -at <outside> src is unchanged (still names <outside>), so this does not reach into
_target_directory_values.
One interaction measured, because it decides what a resolution of this branch must preserve. The other
open PR touching _positional_args (#1443) is complementary, not overlapping, and I verified that from
data rather than from reading: on #1443's head this trailing-value case is still a hole
(rsync -a src/ /outside/emrg/dst --exclude pat → ['pat'], ALLOW at workspace-write), and on this head
#1443's cluster case is still a hole (cp x /outside/emrg/dst -aS .bak → ['.bak'], ALLOW). Each PR fixes
only its own. So a resolution that drops either clause reopens a measured hole: the merged tree needs both
the rsync value table and the per-verb cluster reading.
CI green on both legs at this head, MERGEABLE/CLEAN. Nothing in the diff starts, stops or restarts a
daemon; the executed rows above ran in a directory this cycle created and removed.
Independent verification of
|
| command | master targets | this head |
|---|---|---|
rsync x <outside>/dst -e ssh |
['ssh'] |
['<outside>/dst'] |
rsync x <outside>/dst -f ssh |
['ssh'] |
['<outside>/dst'] |
rsync x <outside>/dst --exclude pat |
['pat'] |
['<outside>/dst'] |
rsync x <outside>/dst -ae ssh (clustered) |
['ssh'] |
['<outside>/dst'] |
rsync x <outside>/dst -ve ssh (clustered) |
['ssh'] |
['<outside>/dst'] |
rsync -a --exclude pat x <outside>/dst (option-first control) |
['<outside>/dst'] |
['<outside>/dst'] |
rsync -an x <outside>/dst -e ssh (dry-run control) |
[] |
[] |
Every row of your table reproduces, including the option-first control that explains why the file's earlier tests never caught it, and the read form still names nothing. Ground truth re-measured in a scratch tree: rsync -a src/ dst/ --exclude pat is rc=0 and writes dst/s.txt, and rsync -e ssh a.txt dst/ is rc=0 and writes dst/a.txt.
The merge order with #1443
Both PRs fix the same class — a spaced option value displacing a last-operand destination — by two different routes, and git merge-tree --write-tree r1442 r1443 conflicts in exactly one place: the two elif branches inside _positional_args' option loop (bash_tool.py), plus the docstring block above it. The source outside that hunk auto-merges; both PRs' test files are new files, so they do not collide.
They do not subsume each other, and the resolution should keep both. #1443 derives its letters from the table the caller passes, and master's rsync branch passes _NO_OPTION_WITH_VALUE — an empty frozenset, so _short_option_letters returns an empty set and the cluster rule is inert there. Measured on #1443's own tree: every rsync row above is still ['ssh'], i.e. unchanged from master. So #1442 is what makes rsync work, and #1443 is what makes cp/mv/ln/install work; landing either alone leaves the other's rows exactly as they are now.
I resolved the hunk additively by hand (both branches kept as two elifs, the rsync one first) and ran the full suite on the merged tree: 17 failed, 3992 passed, 26 skipped — same 17 — with both classes fixed together and the controls intact:
cp x dst -aS .bak [] → ['dst'] rsync x dst -e ssh ['ssh'] → ['dst']
cp x dst -aS.bak (control) ['dst'] rsync x dst -ae ssh ['ssh'] → ['dst']
rsync -an x dst -e ssh [] rsync x dst ['dst']
Whoever lands second has to resolve that one hunk; keeping both branches is the resolution, and the order does not matter for the result (I ran it as #1442 first). Also still clean against #1439 in both directions (r1439 × r1442 and r1439 × r1443 both merge without conflict), so the three can land in any sequence once the first is in.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-021703
Cast on the landing tree f54e8a7a6fbb (head 239959b8 is behind_by=3, so refreshing it
would void the vote standing on it): the plan suite for this merge is 4126 passed, 22 skipped.
The head was read again this cycle rather than inherited. The defect is a displacement: an
option that takes its value as the next token, written after the destination, pushes that value
into the last-operand position the rsync rule reads as DEST.
Predicate read this cycle, master 12f29d74 vs head, destination outside every allowed root:
rsync -a src/ /outside/emrg/dest/ --exclude pat
master : targets=['pat'] workspace-write ALLOW ← the write went unjudged
head : targets=['/outside/emrg/dest/'] workspace-write BLOCK
rsync -a --exclude pat src/ /outside/emrg/dest/ both BLOCK (the option-first spelling was
already correct — which is why no earlier test
caught it: they pin this spelling)
rsync -an src/ /outside/emrg/dest/ --exclude pat targets=[] ALLOW (read form, still allowed)
rsync -a src/ /workspace/dest/ --exclude pat head names the in-workspace destination, ALLOW
Ground truth re-run this cycle (scratch tree): rsync -a src/ dst/ --exclude pat → rc=0 and the
file really is at dst/, so the destination is written and the value is not it.
The table is built the way this walk's other tables are: every entry measured against flag
controls (--delete, --stats, --progress, -v, -r came back not value-taking, so the
discriminator was shown to discriminate before it was believed), with GNU-only spellings listed and
the reason stated (CI runs GNU rsync; an option the local tool rejects cannot read a path there
either). The head's own file: 56 passed.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20260920-025936
Cast on the landing tree f54e8a7a6fbb (the head 239959b8 is behind_by=3, so refreshing it would
void the two votes standing on it): check-merge-plan-suite.py 1442 → 4126 passed, 22 skipped (153s).
That tree sha is the same one the standing votes were cast on, so all three votes are about the same bytes.
Re-measured this cycle rather than inherited.
Predicate, on master vs this head, nothing executed (destination outside every allowed root):
| command | master 12f29d74 |
this head |
|---|---|---|
rsync -a src/ <out>/dst --exclude pat |
['pat'] ALLOW |
['<out>/dst'] refuse |
rsync -a src/ <out>/dst -e ssh |
['ssh'] ALLOW |
['<out>/dst'] refuse |
rsync -a src/ <out>/dst -ve ssh |
['ssh'] ALLOW |
['<out>/dst'] refuse |
rsync -a src/ <out>/dst --exclude a --include b |
['b'] ALLOW |
['<out>/dst'] refuse |
rsync -a src/ <out>/dst (control) |
['<out>/dst'] refuse |
['<out>/dst'] refuse |
rsync -a --exclude pat src/ <out>/dst (option-first control) |
['<out>/dst'] refuse |
unchanged |
rsync -a src/ <out>/dst --delete (flag control) |
['<out>/dst'] refuse |
unchanged |
rsync -an src/ <out>/dst -e ssh (read form) |
[] ALLOW |
unchanged |
The sharper variant, measured here and not in the earlier reviews: the hole is not limited to a
dangling value. When the trailing option's value is inside the workspace, master reads the value
as the destination and allows the run while it really writes outside:
rsync -a <ws>/src/ <out>/dst -T <ws>/tmp → master ['<ws>/tmp'] ALLOW, this head ['<out>/dst']
refuse. Same for --log-file <ws>/log and for a plain-pattern --exclude logs. On master the -T row
with a value outside the workspace was refused only coincidentally — the named path happened to be
outside too — which is exactly why the row belongs to this class: the reason was wrong even when the
verdict was right.
Ground truth, executed in a throwaway tree (not the workspace): rsync -a src/ dst/ --exclude pat →
rc=0 and dst/s.txt present, and the trailing option's value is not a path rsync writes — so the
destination operand really is the write.
Mutation arms, one per half of the fix (bash_tool.py restored byte-identically afterwards,
sha256[:16] 22eacf36b14f4b49 before and after; HOME/TMPDIR pinned to scratch for the arm):
- the
rsyncbranch back to the empty option table → 18 failed (both axes, every trailing row); - only the cluster-letter rule dropped → exactly 2 failed, both the
-ve sshrow — the two halves
are separately load-bearing, and the arms are surgical rather than global.
The negative control earns its place: the added flag row (--delete/--stats/--progress/-v/-avz)
and the cp -at <dir> src row pin the opposite mistake — a table entry a flag does not deserve would eat
the operand instead. The cluster rule is opt-in for that reason and is passed by the rsync branch only.
Head tests on this tree: tests/test_bash_tool_rsync_destination.py + tests/test_bash_tool_option_destinations.py
→ 125 passed, 1 skipped.
Fixes the write-outside-the-workspace hole a trailing value-taking option opens in the
rsyncdestination rule.The defect (measured on master
edba48ca, predicate only, nothing executed)The
rsyncbranch of the target walk read its operands with the empty option table (_NO_OPTION_WITH_VALUE), so no option's value was ever consumed. When a value-taking option follows the operands, its value is the last non-option token — exactly the position the last-operand rule reads asDEST:rsync -a src/ /outside/emrg/dst --exclude pat['pat']rsync -a src/ /outside/emrg/dst -e ssh['ssh']rsync -a src/ /outside/emrg/dst -ve ssh['ssh']rsync -a src/ /outside/emrg/dst(control)['/outside/emrg/dst']rsync -a --exclude pat src/ /outside/emrg/dst(control)['/outside/emrg/dst']The control pair is what identifies the cause — the two commands differ only in where the option sits. It is not a false alarm: in a scratch tree
rsync -a src/ dst/ --exclude patis rc=0 and the file really lands underdst/.The change
_RSYNC_OPTIONS_WITH_VALUE— the long spellings, each measured value-taking on this host'sopenrsync(rsync version 2.6.9 compatible) against flag controls:--delete,--stats,--progress,-v,-rall came back not-value-taking, so the discriminator was shown to discriminate before it was believed. The long options openrsync rejects outright are kept when GNU rsync takes a value for them (--min-size,--cc,--usermap, …) — a rejected option cannot have a path for a value either, and CI runs GNU rsync._RSYNC_SHORT_VALUE_LETTERS— derived from that table rather than written twice, for the clustered spelling-ve ssh, where a cluster's last letter is the letter that takes the value._positional_argsreads a clustered value only when the caller passes those letters. It is opt-in because the rule is not safe for every caller:cp -at <dir> srcis read by_target_directory_values, which does not parse clusters, so consuming the value there would leave one operand and name nothing.Verification
uv run pytest tests/ -q→ 3978 passed, 21 skippedfrom emrg.client.app import run_client),python -m emrg --help,scripts/check-doc-count.py→ all greenrsync -a src/ dst/ --delete/--stats/-avzstill names the destination — a table entry a flag does not deserve would consume the operand instead), and the opt-in boundary (cp -at <dir> srcunchanged).