emrg: a trailing rsync option value is a value, not the destination

emrg/tools/bash_tool.py

@@ -361,18 +361,73 @@ def _translate_windows_heredocs(cmd: str) -> tuple[str, str | None]:
 #
 # `-n` is the dry-run letter, and it is the one letter in this family that turns
 # the run into a read; every rsync short option that takes a *value* (`-e`, `-f`,
-# `-T`, `-M`, `-B`) has a different letter, so no value can be mistaken for it,
-# while `--dry-run`, `--list-only` and `-n` inside a cluster (`-an`, `-avzn`) all
+# `-T`, `-M`, `-B`, and GNU's `-@`) has a different letter, so no value can be
+# mistaken for it (the table below is where those spellings are enumerated), while
+# `--dry-run`, `--list-only` and `-n` inside a cluster (`-an`, `-avzn`) all
 # mean the same thing.
 _RSYNC_READ_LETTERS = frozenset({"n"})
 _RSYNC_READ_LONG = frozenset({"--dry-run", "--list-only"})
 
+# The options that take their value as the **next token**. Without this table a trailing
+# spaced value displaces the destination: consumed as an operand, it becomes the last
+# one, which is the position the rule above reads as `DEST`.
+#
+# Measured on master `edba48ca` with the real predicate, nothing executed, the
+# destination outside every allowed root: `rsync -a src/ /out/dest/ --exclude pat`
+# reported `['pat']` and **ALLOW at both tiers**, while the same command without the
+# trailing option reported `['/out/dest/']` and was refused — so the option's value, not
+# the operand rule, was what displaced it. The run really does write the destination:
+# `rsync -a src/ dst/ --exclude pat` in a scratch tree left the file at `dst/` (rc=0).
+# The same displacement was measured for `-e ssh`, `-f …`, `-B …`, `-T …`,
+# `--out-format …`, and for the clustered `-ve ssh`. Option-**first** spellings
+# (`rsync -a --exclude pat src/ dst/`) were already correct, which is why the file's
+# other tests never caught it: they pin that spelling and the attached `--exclude=pat`.
+#
+# The table is keyed by option *shape*, not by run, so it must cover both implementations
+# the guard meets. On this host (`openrsync`, "rsync version 2.6.9 compatible",
+# 2026-09-20) each entry was measured in a scratch tree against flag controls
+# (`--delete`, `--stats`, `--progress`, `-v`, `-r` all came back *not* value-taking, so
+# the discriminator was shown to discriminate before it was believed): an entry is listed
+# when the following token was consumed — rc=0 with the extra source left uncopied, or a
+# diagnostic naming that very token as a bad numeric/filter/directory argument. The long
+# options openrsync rejects outright ("unknown option") are still listed when GNU rsync
+# takes a value for them: an option the running tool rejects cannot have a path for a
+# value either, and CI runs GNU rsync, where it takes one.
+_RSYNC_OPTIONS_WITH_VALUE = frozenset({
+    # Short spellings, measured here. `-@` is GNU's `--modify-window`, which openrsync
+    # spells in the long form only (listed below).
+    "-e", "-f", "-B", "-M", "-T", "-@",
+    # Measured value-taking on the installed openrsync.
+    "--exclude", "--include", "--filter", "--exclude-from", "--files-from",
+    "--chmod", "--bwlimit", "--timeout", "--max-size", "--log-file",
+    "--out-format", "--log-format", "--log-file-format", "--suffix",
+    "--backup-dir", "--temp-dir", "--partial-dir", "--link-dest", "--rsync-path",
+    "--port", "--protocol", "--sockopts", "--address", "--modify-window",
+    "--compress-level", "--checksum-seed", "--contimeout", "--max-delete",
+    "--write-batch", "--only-write-batch", "--password-file",
+    # GNU-only spellings: rejected here, value-taking there.
+    "--min-size", "--max-alloc", "--compare-dest", "--copy-dest",
+    "--checksum-choice", "--cc", "--compress-choice", "--zc",
+    "--compress-threads", "--zt", "--skip-compress", "--block-size", "--stderr",
+    "--info", "--debug", "--usermap", "--groupmap", "--chown", "--early-input",
+    "--outbuf", "--stop-at", "--stop-after", "--time-limit", "--confine-root",
+    "--config", "--dparam", "--remote-option", "--copy-as", "--iconv",
+})
+
+# The letters of the short entries above, **derived** rather than written twice. A
+# trailing value can also arrive inside a cluster, where an exact-token test finds
+# nothing: `rsync -a src/ dst/ -ve ssh` carries its value in the cluster's last letter,
+# and a cluster's last letter is the one that takes the value.
+_RSYNC_SHORT_VALUE_LETTERS = frozenset(
+    opt[1] for opt in _RSYNC_OPTIONS_WITH_VALUE if len(opt) == 2 and opt[0] == "-"
+)
+
 # Named residual of the rule above: `--write-batch=<file>` /
 # `--only-write-batch=<file>` make rsync write a *second* path — the option's own
 # value — beside the destination operand. It is left unnamed because a batch file
-# is a debugging artefact of a transfer, not the transfer, and adding it means
-# reading one more option's value in both spellings; the destination operand this
-# rule exists for is named either way.
+# is a debugging artefact of a transfer, not the transfer; the destination operand
+# this rule exists for is named either way, and the table below consumes the value
+# in both spellings so it is never mistaken for that operand.
 
 # `split` writes a **family** of derived paths, and its last operand is the only
 # place their common prefix is spelled: `split -b 3 in.txt pre` creates `preaa`,
@@ -1108,7 +1163,10 @@ def _rsync_run_is_a_read(tokens: list[str], i: int) -> bool:
 
 
 def _positional_args(
-    tokens: list[str], i: int, options_with_value: frozenset | None = None
+    tokens: list[str],
+    i: int,
+    options_with_value: frozenset | None = None,
+    cluster_value_letters: frozenset[str] = frozenset(),
 ) -> list[str]:
     """The non-option *operands* of the command starting at ``tokens[i]``.
 
@@ -1126,6 +1184,14 @@ def _positional_args(
     table. Omitting it keeps the historical flat table, which is what the
     earlier callers (`rm`, `mv`, `cp`, `find`, the in-place writers) still read.
 
+    ``cluster_value_letters`` is the table's **short letters**, and it is opt-in: a
+    value can also arrive inside a cluster, where the exact-token test above finds
+    nothing (``rsync -a src/ dst/ -ve ssh`` carries `ssh` as `-ve`'s value, since a
+    cluster's last letter is the one that takes one). It stays opt-in because adding
+    the rule to a caller that did not ask for it can *lose* a destination rather than
+    gain one: `cp -at <dir> src` is read by `_target_directory_values`, which does not
+    parse clusters, so consuming `<dir>` here would leave one operand and name nothing.
+
     A ``--`` **ends option parsing**, and that sentence was here before the loop
     below obeyed it (issue #1433): the loop skipped the ``--`` and went on
     dropping every dash-led token, so a command whose operand is a file whose own
@@ -1172,6 +1238,12 @@ def _positional_args(
             # spaced form consumes the next one.
             if tok in table:
                 skip_next = True
+            elif (
+                cluster_value_letters
+                and not tok.startswith("--")
+                and tok[-1] in cluster_value_letters
+            ):
+                skip_next = True
             continue
         out.append(tok)
     return out
@@ -1925,7 +1997,9 @@ def is_separator(index: int) -> bool:
             # destination, and it is rewritten — unless the run is a read form
             # (`-n`/`--dry-run`/`--list-only`), which writes nothing at all.
             if not _rsync_run_is_a_read(tokens, i):
-                args = _positional_args(tokens, i, _NO_OPTION_WITH_VALUE)
+                args = _positional_args(
+                    tokens, i, _RSYNC_OPTIONS_WITH_VALUE, _RSYNC_SHORT_VALUE_LETTERS
+                )
                 # A single operand is a *listing* of the source, not a copy.
                 if len(args) >= 2:
                     targets.append(args[-1])

tests/test_bash_tool_rsync_destination.py

@@ -114,6 +114,86 @@ def test_the_protected_daemon_file_is_refused_at_both_tiers():
     assert "protected" in (ww_reason or ""), ww_reason
 
 
+# ── a trailing option's value is a value, not the destination ───────────────────────
+#
+# Every writing form above carries its options **before** the operands, and the attached
+# `--exclude=foo` row is attached. Neither spelling can see the defect below: when a
+# value-taking option *follows* the operands, its value is the last non-option token,
+# which is exactly the position the last-operand rule reads as `DEST` — so the option's
+# value displaced the destination and the run was allowed. Measured on master `edba48ca`
+# with the real predicate, nothing executed, destination outside every allowed root:
+# `rsync -a src/ /outside/emrg/dst --exclude pat` answered `['pat']` and **ALLOW at both
+# tiers**, while the same command without the trailing option answered
+# `['/outside/emrg/dst']` and was refused. It is not a false alarm: in a scratch tree
+# `rsync -a src/ dst/ --exclude pat` is rc=0 and the file really lands under `dst/`.
+# The option-first spelling was already correct, which is why the rows above stayed green
+# through it — the two spellings differ only in where the option sits.
+TRAILING_VALUE_FORMS = (
+    ("exclude", f"rsync -a {WORKSPACE}/src/ {{dest}} --exclude pat"),
+    ("include", f"rsync -a {WORKSPACE}/src/ {{dest}} --include pat"),
+    ("rsh", f"rsync -a {WORKSPACE}/src/ {{dest}} -e ssh"),
+    ("rsh inside a cluster", f"rsync -a {WORKSPACE}/src/ {{dest}} -ve ssh"),
+    ("temp-dir", f"rsync -a {WORKSPACE}/src/ {{dest}} -T /tmp/scratch"),
+    ("block-size", f"rsync -a {WORKSPACE}/src/ {{dest}} -B 4096"),
+    ("filter", f"rsync -a {WORKSPACE}/src/ {{dest}} -f rule"),
+    ("out-format", f"rsync -a {WORKSPACE}/src/ {{dest}} --out-format %n"),
+    ("log-file", f"rsync -a {WORKSPACE}/src/ {{dest}} --log-file /tmp/scratch/log"),
+    ("two trailing values", f"rsync -a {WORKSPACE}/src/ {{dest}} --exclude a --include b"),
+)
+
+
+@pytest.mark.parametrize(
+    "row,cmd", TRAILING_VALUE_FORMS, ids=[row for row, _ in TRAILING_VALUE_FORMS]
+)
+def test_a_trailing_value_does_not_displace_the_destination(row, cmd):
+    """The option's value must be consumed, so the operand is still the one named."""
+    dest = f"{OUTSIDE}/dst"
+    assert _extract_write_targets(cmd.format(dest=dest)) == [dest], row
+
+
+@pytest.mark.parametrize(
+    "row,cmd", TRAILING_VALUE_FORMS, ids=[row for row, _ in TRAILING_VALUE_FORMS]
+)
+def test_a_displaced_destination_is_refused_at_both_tiers(row, cmd):
+    """The same two tiers as every other writing form, for the same reason."""
+    for tier in ("read-only", "workspace-write"):
+        allowed, reason, _ = _check_sandbox(
+            cmd.format(dest=f"{OUTSIDE}/dst"), tier, WORKSPACE
+        )
+        assert allowed is False, f"{row}: {tier} allowed a write to {OUTSIDE}"
+        assert reason, f"{row}: {tier} refused without a reason"
+
+
+def test_a_trailing_flag_does_not_eat_the_destination():
+    """The table's own control: an entry the option does not have would trade the hole
+    for the opposite mistake.
+
+    A value-taking entry that a flag does not deserve consumes the operand the rule
+    names — a false block with two operands left, a silent miss with one. Each spelling
+    here was measured on this host as a *flag*, in a scratch tree: the token after it
+    stayed a source and was really copied as a second one (`--delete`, `--stats`,
+    `--progress`, `-v`, `-r` all came back not-value-taking). The table is only
+    trustworthy with the positive rows above and this negative row together.
+    """
+    for flags in ("--delete", "--stats", "--progress", "-v", "-avz", "--ignore-times"):
+        cmd = f"rsync -a {WORKSPACE}/src/ {OUTSIDE}/dst {flags}"
+        assert _extract_write_targets(cmd) == [f"{OUTSIDE}/dst"], flags
+
+
+def test_the_cluster_rule_is_the_rsync_branch_s_alone():
+    """`cp -at <dir> src` keeps naming its operand: the cluster letters are not shared.
+
+    The clustered-value rule is opt-in for this reason. `cp` reads `-t` through
+    `_target_directory_values`, which does not parse clusters, so consuming a cluster's
+    value inside `_positional_args` for every caller would leave one operand and name
+    nothing — the `rsync` table's letters are passed by the `rsync` branch only.
+    """
+    cmd = f"cp -at {OUTSIDE}/dst {WORKSPACE}/src.txt"
+    assert _extract_write_targets(cmd) == [f"{OUTSIDE}/dst"]
+    allowed, _reason, _ = _check_sandbox(cmd, "workspace-write", WORKSPACE)
+    assert allowed is False
+
+
 # ── read forms: the operand is not written, so naming it would be a false block ─────
 READING_FORMS = (
     ("-an cluster", f"rsync -an {WORKSPACE}/src.txt {{dest}}"),