emrg: a git flag with an attached value is the same flag (#1256)

emrg/tools/bash_tool.py

@@ -256,8 +256,14 @@ def _translate_windows_heredocs(cmd: str) -> tuple[str, str | None]:
                              "--no-color", "--ignore-case"})
 # `git branch -a` / `git tag -l` / `git tag` are reads; a delete or force flag
 # makes them destructive (`git branch -D old`, `git tag -d v1`).
+#
+# `--unset-upstream` belongs here for the same reason as `--delete`: it writes
+# (it removes the branch's upstream from `.git/config`), and it takes no
+# argument, so no positional rule can catch it — measured 2026-09-16, it was
+# ALLOWED under read-only and really did clear the upstream.
 _GIT_WRITE_FLAGS = frozenset({"-d", "-D", "--delete", "-f", "--force", "-m",
-                              "-M", "--move", "--set-upstream-to", "-u"})
+                              "-M", "--move", "--set-upstream-to", "-u",
+                              "--unset-upstream"})
 # `git config` reads unless it writes: read flags, or no positional key.
 _GIT_CONFIG_READ_FLAGS = frozenset({"--get", "--get-all", "--get-regexp",
                                     "-l", "--list", "--get-urlmatch"})
@@ -1091,6 +1097,30 @@ def _git_invocation_is_mutator(verb: str, rest: list[str]) -> str | None:
     return verb
 
 
+def _flag_part(tok: str) -> str:
+    """The spelling a flag table should be asked about, for ``tok``.
+
+    A flag and its value may be written as one token or two, and the tables hold
+    the flag. Comparing whole tokens therefore decides by *spelling* while
+    claiming to decide by flag: measured 2026-09-16 against master, read-only
+    ALLOWED `git branch --set-upstream-to=origin/main` and `git branch
+    -uorigin/main` (both really did set the upstream) while the two-token forms
+    `--set-upstream-to origin/main` / `-u origin/main` were BLOCKED — the same
+    write, three spellings, two verdicts. `git branch -dold` is refused by git
+    itself (rc=129), so widening here can only over-block a shape git rejects.
+
+    Only a leading flag is rewritten: `-` alone is a filename, ``--`` alone is
+    the argument terminator, and a short option keeps its first letter (`-u`),
+    which is how git reads it too. Nothing else about the token is touched, so
+    a path or a pattern is still compared as written.
+    """
+    if not tok.startswith("-") or tok in ("-", "--"):
+        return tok
+    if tok.startswith("--"):
+        return tok.split("=", 1)[0]
+    return tok[:2]
+
+
 def _git_positionals(rest: list[str]) -> list[str]:
     """The non-option arguments of a git subcommand, option *values* excluded.
 
@@ -1121,7 +1151,13 @@ def _shape_decided_verdict(verb: str, rest: list[str]) -> str | None:
     Returns the verb when the invocation writes, or None when it is a proven
     read. Every branch treats "not recognisably a read" as a write.
     """
+    # Two independent readings of the same token list, and both are needed:
+    # `_git_positionals` skips an option's *value* (so `git reflog -n 5` reads
+    # `5` as neither a subcommand nor a positional — issue #1240), while `flags`
+    # compares each option by its part, so an attached value cannot hide a flag
+    # (issue #1256). Taking either one alone re-opens the other's defect.
     positional = _git_positionals(rest)
+    flags = [_flag_part(t) for t in rest]
     sub = next(iter(positional), None)
     if verb == "stash":
         # `stash list` / `stash show` read; a bare `git stash` saves and cleans
@@ -1139,10 +1175,12 @@ def _shape_decided_verdict(verb: str, rest: list[str]) -> str | None:
         return None if sub is None or sub in ("show", "get-url", "v") else verb
     if verb in ("branch", "tag"):
         # A listing flag makes this a read even with a pattern argument; a
-        # delete/force/move flag makes it a write.
-        if any(t in _GIT_WRITE_FLAGS for t in rest):
+        # delete/force/move flag makes it a write. Both tests ask `flags`, not
+        # `rest`, so an attached value (`--delete=old`, `-uorigin/main`) is the
+        # same flag as the two-token form.
+        if any(t in _GIT_WRITE_FLAGS for t in flags):
             return verb
-        if any(t in _GIT_LIST_FLAGS for t in rest):
+        if any(t in _GIT_LIST_FLAGS for t in flags):
             return None
         # `git branch <name>` / `git tag <name>` create; only the bare form
         # (no positional argument at all) is the listing read.

tests/test_bash_tool_sandbox.py

@@ -17,6 +17,7 @@
     SANDBOX_MODES,
     _check_sandbox,
     _extract_write_targets,
+    _flag_part,
     _GIT_READ_VERBS,
     _GIT_SHAPE_DECIDED,
     check_workspace_write,
@@ -184,6 +185,94 @@ def test_check_read_only_blocks_git_mv():
     assert "read-only sandbox" in reason
 
 
+def test_flag_part_is_the_flag_and_not_the_spelling():
+    """The helper the git flag tables are asked about, pinned case by case.
+
+    A flag table holds a *flag*; a shell token may carry its value. Asking the
+    table about the token decides by spelling, which is how three spellings of
+    one write got two verdicts (issue #1238's family, measured 2026-09-16).
+    """
+    assert _flag_part("--set-upstream-to=origin/main") == "--set-upstream-to"
+    assert _flag_part("-uorigin/main") == "-u"
+    assert _flag_part("--sort=-committerdate") == "--sort"
+    assert _flag_part("-vv") == "-v"
+    # Things that are not flags keep every character: a lone `-` is a filename,
+    # `--` is the argument terminator, and a path or pattern is not a flag.
+    assert _flag_part("-") == "-"
+    assert _flag_part("--") == "--"
+    assert _flag_part("origin/main") == "origin/main"
+    assert _flag_part("v1.2.3") == "v1.2.3"
+
+
+def test_check_read_only_blocks_attached_value_git_write_flags():
+    """A write flag whose value is attached is the same write.
+
+    Measured 2026-09-16 against master: each of these was ALLOWED under
+    read-only and, executed in a real repository with a real upstream, really
+    did write — the branch's upstream in `.git/config` went from `origin/main`
+    to `origin/old`. Their two-token twins (`--set-upstream-to origin/main`,
+    `-u origin/main`) were already blocked, so the guard was deciding by
+    spelling. This test pins the flag comparison itself.
+    """
+    for cmd in (
+        "git branch --set-upstream-to=origin/main",
+        "git branch -uorigin/main",
+        # Spellings git itself refuses (rc=129) are blocked too. That is the
+        # harmless direction, and it is pinned so a later reader does not
+        # "fix" the truncation into an exemption.
+        "git branch -dold",
+        "git branch -Dold",
+        "git branch --delete=old",
+        "git tag -dv1",
+        "git tag --delete=v1",
+    ):
+        allowed, reason, _ = _check_sandbox(cmd, "read-only")
+        assert allowed is False, f"{cmd!r} should be blocked (got {reason!r})"
+        assert "read-only sandbox" in reason
+        assert "git" in reason, cmd
+
+
+def test_check_read_only_blocks_unset_upstream():
+    """The flag that takes no argument at all, so no positional rule sees it.
+
+    Measured 2026-09-16 against master: `git branch --unset-upstream` was
+    ALLOWED under read-only and really did clear the branch's upstream. It is a
+    separate mechanism from the attached-value case above — no spelling is
+    involved, the flag was simply absent from the table — so it fails on its
+    own if only that entry is removed.
+    """
+    allowed, reason, _ = _check_sandbox("git branch --unset-upstream", "read-only")
+    assert allowed is False, f"got {reason!r}"
+    assert "read-only sandbox" in reason
+    assert "git" in reason
+
+
+def test_check_read_only_still_allows_git_branch_and_tag_reads():
+    """The other direction: widening the flag tests must not eat the reads.
+
+    `--set-upstream-to` is a write; `--sort` in the same attached-value shape is
+    a read. Both go through `_flag_part`, so both are pinned here — a fix that
+    blocks the listing forms would be a usability regression with no safety
+    gain, which is what the read-only tier's own docstring warns about.
+    """
+    for cmd in (
+        "git branch",
+        "git branch -a",
+        "git branch -vv",
+        "git branch --show-current",
+        "git branch --contains HEAD",
+        "git branch --sort=-committerdate",
+        "git branch --list 'feat/*'",
+        "git tag",
+        "git tag -l",
+        "git tag --list",
+        "git tag -n",
+        "git tag --points-at HEAD",
+    ):
+        allowed, reason, _ = _check_sandbox(cmd, "read-only")
+        assert allowed is True, f"{cmd!r} should be allowed (got {reason!r})"
+
+
 def test_check_read_only_allows_git_reads():
     """Read-only keeps read-only git reads available — the read-only cycle
     still needs status/fetch/log/diff for scanning and review."""