emrg: a duplication operand is a descriptor, so read-only stops refusing ordinary reads

emrg/tools/bash_tool.py

@@ -654,8 +654,12 @@ def _is_redirect_operator(tok: str) -> bool:
     `workspace-write`. Both genuinely write: in a throwaway directory bash and sh
     each created the file for `>|` (the clobber redirect) and bash created it for
     `<>` (read-write), so an empty target list there is a hole rather than an
-    opinion. `>>|`, `>>&` and `&>>` created nothing, so they are covered only
-    incidentally.
+    opinion. `>>|`, `>>&` and `&>>` created nothing under the shell this walk's
+    own runtime uses (`create_subprocess_shell` -> `/bin/sh`; measured here as
+    rc=2 syntax errors that write no file), which is why they are covered only
+    incidentally — on bash 4+, where `&>>` and `>>&` are valid append-both
+    redirects that do write, recognising them as operators is the safe direction
+    rather than an unnecessary one.
 
     The shape test is: the token contains a `>` and no character that could be
     part of a path. That also covers the fd-prefixed spellings (`1>|`, `0<>`,
@@ -673,6 +677,28 @@ def _is_redirect_operator(tok: str) -> bool:
     )
 
 
+def _is_fd_operand(tok: str) -> bool:
+    """True when ``tok`` is a file *descriptor* rather than a path.
+
+    Only ever asked about the operand of the ``>&`` operator, where the shell
+    reads an all-digits or ``-`` operand as a descriptor to duplicate onto
+    (``2>&1``, ``2>&-``) instead of a file to open. The order of ``&`` and ``>``
+    is the whole discriminator, and it is the shell's, not a convention: ``&>1``
+    is the other operator and really does write a file called ``1``, while
+    ``>&1x`` and ``>& out.log`` name files (so spacing and "the operand is
+    numeric" are both insufficient signals).
+
+    Measured on master `39edaefa` (`bash_tool.py` `4dce1ffde8902bc1`), each
+    command run by bash **and** sh in a fresh scratch directory: six duplication
+    spellings (`2>&1`, `1>&2`, `>&2`, `2>&-`, `2>& 1`, and `2>&1` beside a later
+    real redirect) created no file in either shell, while every genuine-file
+    spelling in the same corpus (`> 1`, `&>1`, `>&1x`, `>& out.log`) really wrote
+    the file the walk names. `[0-9]+` rather than ``str.isdigit`` because the
+    latter is true of Unicode digits (`²`, `١`), which are not descriptors.
+    """
+    return tok == "-" or re.fullmatch(r"[0-9]+", tok) is not None
+
+
 def _fully_quoted_token_indexes(cmd: str, tokens: list[str]) -> set[int]:
     """Indexes of ``tokens`` whose word was **entirely quoted** in the source line.
 
@@ -772,6 +798,13 @@ def _extract_write_targets(cmd: str, _depth: int = 0) -> list[str]:
     interpreter can always write a file); the honest boundary stays
     ``enforcement="partial"``.
 
+    An operator's operand is not always a path: the operand of `>&` is a file
+    *descriptor* when it is all digits or `-`, so `grep -n x f.txt 2>&1` opens
+    nothing and now names no target, instead of being refused for "targeting
+    '1'" in the very tier a dirty-tree downgrade uses (`_is_fd_operand`, issue
+    #1275). The operator's spelling is what decides it, never the operand alone:
+    `&>1` and `echo x > 1` both really write a file called `1` and still name it.
+
     Heredoc bodies that no shell executes are masked first: a body is text on
     some command's stdin, and reading it as shell code named prose and the
     delimiter word as write targets (`_mask_data_heredoc_bodies`).
@@ -810,7 +843,17 @@ def is_operator(index: int) -> bool:
             while j < len(tokens) and is_operator(j):
                 j += 1
             if j < len(tokens) and tokens[j] not in _COMMAND_SEPARATORS:
-                targets.append(tokens[j])
+                # …but the operand of `>&` is not always a path: when it is all
+                # digits or `-` the shell duplicates onto that descriptor and
+                # opens nothing (`2>&1`, `2>&-`). Recording it made ordinary
+                # read-only diagnostics — `grep -n x f.txt 2>&1` — refusals for
+                # "targeting '1'" in the one tier whose documented recovery flow
+                # is to run read commands from inside it (issue #1275). The
+                # operator's spelling decides this, never the operand alone:
+                # `&>1` writes a real file called `1` and `echo x > 1` writes it
+                # too, so both keep naming their target here.
+                if not (tokens[i] == ">&" and _is_fd_operand(tokens[j])):
+                    targets.append(tokens[j])
                 i = j + 1
                 continue
         elif word == "rm" or word == "rmdir":

tests/test_bash_tool_sandbox.py

@@ -18,6 +18,7 @@
     _check_sandbox,
     _extract_write_targets,
     _fully_quoted_token_indexes,
+    _is_fd_operand,
     _is_redirect_operator,
     _split_command_tokens,
     _flag_part,
@@ -962,6 +963,80 @@ def quoted(cmd: str) -> set[int]:
     assert quoted("'a' it's") == set()
 
 
+# Issue #1275. A duplication operand is a file *descriptor*, not a path, so
+# these open nothing — measured with **both** shells in a fresh scratch
+# directory per row, because a verdict mismatch alone is not a bug and `/bin/sh`
+# does not always agree with bash. Each one used to report target `'1'`/`'2'`
+# (or `'-'`) and be refused at `read-only`, the tier a dirty-tree downgrade
+# forces a cycle into — an over-block of ordinary diagnostics where they are
+# most needed. `>>&` is deliberately absent: it is a bash **syntax error**
+# (`unexpected token`), so its line never runs and is not this class.
+FD_DUPLICATIONS = [
+    "grep -n x f.txt 2>&1",
+    "echo hi 2>&1",
+    "grep -n x f.txt 1>&2",
+    "grep -n x f.txt >&2",
+    "grep -n x f.txt 2>&-",
+    "grep -n x f.txt 2>& 1",
+]
+
+
+@pytest.mark.parametrize("cmd", FD_DUPLICATIONS)
+def test_an_fd_duplication_operand_is_not_a_write_target(cmd: str):
+    """`2>&1` merges stderr into stdout; it names a descriptor, not a file."""
+    targets = _extract_write_targets(cmd)
+    assert "1" not in targets and "2" not in targets and "-" not in targets, targets
+    allowed, reason, _ = _check_sandbox(cmd, "read-only")
+    assert allowed is True, f"{cmd!r} must be allowed (got {reason!r})"
+
+
+def test_a_real_redirect_beside_a_duplication_still_names_its_target():
+    """The fix must not swallow the write on the same line.
+
+    `2>&1` is skipped, but the `> out.log` next to it is a real file: dropping
+    the descriptor must not drop the target, or a write would become invisible.
+    """
+    for cmd, expected in (
+        ("echo hi 2>&1 > out.log", ["out.log"]),
+        ("echo hi > out.log 2>&1", ["out.log"]),
+        ("echo hi 2>&1 > out.log 2>&1", ["out.log"]),
+    ):
+        assert _extract_write_targets(cmd) == expected, cmd
+        allowed, reason, _ = _check_sandbox(cmd, "read-only")
+        assert allowed is False, f"{cmd!r} writes a file and must be refused ({reason!r})"
+
+
+def test_the_operator_spelling_decides_a_duplication_not_the_operand():
+    """`&>` is the *other* operator: its numeric operand really is a file name.
+
+    This is the discriminator the fix has to use, and the tempting shortcuts
+    both fail here — "the operand is numeric" would drop a real write to a file
+    called `1`, and "the operand is separated by a space" would drop `out.log`
+    in `>& out.log`, which bash and sh both create. `read-only` is the tier
+    asserted, because these targets are relative and a relative target inside
+    the workspace is exactly what `workspace-write` exists to allow.
+    """
+    written = {
+        "echo x > 1": ["1"],
+        "echo x &>1": ["1"],
+        "echo x >&1x": ["1x"],
+        "echo x >& out.log": ["out.log"],
+        "echo x >&'out.log'": ["out.log"],
+    }
+    for cmd, expected in written.items():
+        assert _extract_write_targets(cmd) == expected, cmd
+        allowed, reason, _ = _check_sandbox(cmd, "read-only")
+        assert allowed is False, f"{cmd!r} must be blocked at read-only ({reason!r})"
+
+
+def test_is_fd_operand_accepts_only_the_spellings_the_shell_reads_as_descriptors():
+    """Direct test of the predicate, including the case `str.isdigit` gets wrong."""
+    for tok in ("0", "1", "2", "10", "-"):
+        assert _is_fd_operand(tok) is True, tok
+    for tok in ("", "1x", "out.log", "./1", "-1", "1 ", ">", "²", "١", "١٢"):
+        assert _is_fd_operand(tok) is False, tok
+
+
 OUTSIDE_CLOBBER = "/etc/emrg-clobber-probe.txt"
 
 # The spellings a shell really writes with and the walk did not call operators.