diff --git a/emrg/tools/bash_tool.py b/emrg/tools/bash_tool.py index 35aeb179..20e56a3c 100644 --- a/emrg/tools/bash_tool.py +++ b/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 ` / `git tag ` create; only the bare form # (no positional argument at all) is the listing read. diff --git a/tests/test_bash_tool_sandbox.py b/tests/test_bash_tool_sandbox.py index 745899e5..416c3bcc 100644 --- a/tests/test_bash_tool_sandbox.py +++ b/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."""