fix(code-review): check git-quoted diff paths instead of skipping them

Author: wongk

CHANGELOG.md

@@ -8,7 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 #### Fixed
 - Reviewer and verifier agents resolved repo-relative source paths against the invoking session's working directory instead of the checkout under review (ISS-7382). `resolve-scope` emitted `review_root: ""` for every scope kind except a local PR-head worktree, and `shared_prompt.txt` and `verifier_prompt.txt` read an empty root as "read from the working directory" — which for a spawned agent is the session's checkout, not the tree the diff came from. `resolve-scope` now resolves `review_root` for every scope kind (the PR-head worktree when one is created, otherwise `git rev-parse --show-toplevel` of the invoking checkout), records the commit it resolved at as `review_root_sha`, and exits `3` instead of emitting an empty root when the working directory is not inside a git worktree.
-- The four stages that hand work to agents — `derive-spawn-spec`, `derive-static-spec`, `verify-prepare`, and `review-dismissed-prepare` — re-prove `review_root` before dispatching and exit `3` when they cannot. The root must be absolute, free of control characters, `<`, `>` and backticks, exist, and be the root of a git worktree. A PR-head worktree's HEAD must still equal `review_root_sha`, and a live checkout must still contain that commit. Every non-removed file in `diff_data.json` must exist under the root (git C-quoted paths are skipped), and a root with neither a recorded commit nor a resolvable changed file is refused.
+- The four stages that hand work to agents — `derive-spawn-spec`, `derive-static-spec`, `verify-prepare`, and `review-dismissed-prepare` — re-prove `review_root` before dispatching and exit `3` when they cannot. The root must be absolute, free of control characters, `<`, `>` and backticks, exist, and be the root of a git worktree. A PR-head worktree's HEAD must still equal `review_root_sha`, and a live checkout must still contain that commit. Every non-removed file in `diff_data.json` must exist under the root, and a root with neither a recorded commit nor a resolvable changed file is refused. `parse-diff` records names as `git diff --name-only` prints them, so a path holding a non-ASCII, control, `"` or `\` byte is stored C-quoted; those names are decoded (named escapes and 3-digit octal escapes) to the name on disk and go through the existence and drift checks like any other file, and an entry that starts with `"` but is not valid quoting is refused.
 - Dispatch is also refused when a file the diff changes no longer matches `review_root_sha` in the working tree: a commit, reset, or uncommitted edit to that file after the scope was resolved, including uncommitted edits already present when a branch review starts. The comparison is one `git diff --name-only -z --no-renames <review_root_sha>` call intersected with the diff's file list, so no pathspec is passed and no path is glob-interpreted, and a failing git call refuses rather than passes. Changes to files outside the diff are still allowed, and `staged` scope is exempt because its diff is index-vs-HEAD.
 - Exit `3` now aborts the walk regardless of the stage's `on_failure`. `derive-spawn-spec`, `derive-static-spec`, and `verify-prepare` are `on_failure: continue`, so a plain non-zero exit would fall back to the static reviewer table or skip verification and dispatch agents against the same unproven tree. `_execute_stage_inprocess` and both walkers in `prefix_golden_harness.py` apply the override; every other non-zero exit still follows `on_failure`. `start.md` tells the orchestrator not to resume a stage that exited `3` from the `run-prefix` error fallback.
 - A positional scope argument that is a git ref or revision range (e.g. `/code-review origin/main...HEAD`) is rejected with a pointer to `--base`, instead of being folded into a `--` pathspec that matched nothing and produced an empty, clean review.

plugins/code-review/tools/python/code_review_helpers.py

@@ -529,13 +529,73 @@ def _git_drifted_paths(
     return [p for p in paths if p in differing]
 
 
+# Named escapes git's C-style path quoting emits; every other control or
+# non-ASCII byte becomes a 3-digit octal escape.
+_GIT_C_QUOTE_ESCAPES: dict[str, int] = {
+    "a": 0x07, "b": 0x08, "t": 0x09, "n": 0x0A, "v": 0x0B, "f": 0x0C,
+    "r": 0x0D, '"': 0x22, "\\": 0x5C,
+}
+
+
+def _git_unquote_path(recorded: str) -> str | None:
+    """The on-disk name for a path as ``git diff --name-only`` printed it.
+
+    Without ``-z`` git C-quotes any path holding a non-ASCII, control, double
+    quote or backslash byte: the name is wrapped in double quotes, each such
+    byte becomes one of ``_GIT_C_QUOTE_ESCAPES`` or a 3-digit octal escape,
+    and the escapes spell raw bytes, not text. An unquoted name is returned
+    unchanged. Returns None when a quoted name is not valid quoting
+    (unterminated, a bare inner quote, an unknown or truncated escape, an
+    out-of-range octal, or an empty name), so the caller refuses rather than
+    guesses.
+    """
+    if not recorded.startswith('"'):
+        return recorded
+    if len(recorded) < 3 or not recorded.endswith('"'):
+        return None
+    body = recorded[1:-1]
+    raw = bytearray()
+    i = 0
+    while i < len(body):
+        ch = body[i]
+        if ch == '"':
+            return None
+        if ch != "\\":
+            raw += ch.encode("utf-8", "surrogateescape")
+            i += 1
+            continue
+        escape = body[i + 1:i + 2]
+        if escape in _GIT_C_QUOTE_ESCAPES:
+            raw.append(_GIT_C_QUOTE_ESCAPES[escape])
+            i += 2
+            continue
+        octal = body[i + 1:i + 4]
+        if (
+            len(octal) == 3
+            and all(c in "01234567" for c in octal)
+            and int(octal, 8) <= 0xFF
+        ):
+            raw.append(int(octal, 8))
+            i += 4
+            continue
+        return None
+    return os.fsdecode(bytes(raw))
+
+
 def _diff_changed_files(cr_dir: str | Path) -> list[str]:
-    """Repo-relative diff files that must exist at the reviewed tip.
+    """On-disk names of the repo-relative diff files that must exist at the tip.
 
     Removals are excluded — they are absent from the head by definition.
     Empty when ``diff_data.json`` has not been written yet (stages that run
     before ``parse-diff``), which makes the containment check additive rather
     than a precondition on stage order.
+
+    ``parse-diff`` records names as ``git diff --name-only`` prints them, so a
+    path holding a non-ASCII, control, double quote or backslash byte is
+    stored C-quoted. It is decoded to the name on disk, never skipped: a
+    skipped entry would drop that file from both the containment and the drift
+    check, so an edit to it after resolution would go unnoticed. An entry that
+    looks quoted but does not decode raises ``ReviewRootError``.
     """
     data = _read_optional_json(Path(cr_dir) / "diff_data.json", None)
     if not isinstance(data, dict):
@@ -545,15 +605,22 @@ def _diff_changed_files(cr_dir: str | Path) -> list[str]:
     files = data.get("files_to_review")
     if not isinstance(files, list):
         return []
-    return [
-        f for f in files
-        if isinstance(f, str) and f and statuses.get(f) != "removed"
-        # git C-quotes a path containing non-ASCII, control, quote or
-        # backslash bytes, so the recorded string is not the name on disk and
-        # its absence proves nothing. An entry we cannot resolve must not
-        # produce a confident refusal.
-        and not f.startswith('"')
-    ]
+    changed: list[str] = []
+    for recorded in files:
+        if not isinstance(recorded, str) or not recorded:
+            continue
+        # file_statuses is keyed by the same quoted string parse-diff recorded.
+        if statuses.get(recorded) == "removed":
+            continue
+        name = _git_unquote_path(recorded)
+        if name is None:
+            raise ReviewRootError(
+                f"diff_data.json lists {recorded!r}, which is not valid git "
+                "path quoting, so it cannot be resolved to a file on disk and "
+                "the root cannot be proven to hold it.",
+            )
+        changed.append(name)
+    return changed
 
 
 def _require_review_root(cr_dir: str | Path, scope_meta: object) -> str:

plugins/code-review/tools/python/test_code_review_helpers.py

@@ -9986,23 +9986,90 @@ def test_root_proven_by_nothing_errors(self, tmp_path: Path) -> None:
 
         assert rc == 3
 
+    @staticmethod
+    def _seed_root_with_git_quoted_changed_file(
+        tmp_path: Path,
+    ) -> tuple[Path, Path, str]:
+        """A live checkout whose diff changes a file git records C-quoted.
+
+        The recorded name comes from git itself (``ls-files`` quotes paths
+        exactly as ``diff --name-only`` does), so diff_data.json carries the
+        string parse-diff would store rather than a hand-written escape.
+        """
+        cr_dir = tmp_path / "cr"
+        cr_dir.mkdir(parents=True)
+        root = Path(_make_review_root(
+            tmp_path / "checkout", {"src/café.py": "v1\n"},
+        ))
+        recorded = next(
+            line for line in git_fixture(root, "ls-files").splitlines()
+            if "caf" in line
+        )
+        assert recorded.startswith('"'), f"git did not quote {recorded!r}"
+        (cr_dir / "scope.json").write_text(json.dumps({
+            "review_root": str(root),
+            "review_root_sha": git_fixture(root, "rev-parse", "HEAD").strip(),
+        }))
+        (cr_dir / "diff_data.json").write_text(json.dumps({
+            "files_to_review": [recorded],
+            "file_statuses": {recorded: "added"},
+        }))
+        return cr_dir, root, recorded
+
     def test_git_quoted_path_does_not_refuse_a_correct_root(
         self, tmp_path: Path,
     ) -> None:
         # git C-quotes a non-ASCII path in `diff --name-only`, so the recorded
-        # string is not the name on disk. An entry we cannot resolve must not
-        # abort a review against a perfectly correct root.
-        cr_dir = tmp_path / "cr"
-        _seed_scope_review_root(cr_dir, tmp_path / "checkout")
+        # string is not the name on disk. It is decoded, and a root holding the
+        # decoded file unchanged must not be refused.
+        cr_dir, root, _recorded = self._seed_root_with_git_quoted_changed_file(
+            tmp_path,
+        )
+
+        finding = _make_validated_finding("bha_1", severity="HIGH")
+        rc, _manifest = _run_verify_prepare(tmp_path, [finding], cr_dir=cr_dir)
+
+        assert rc == 0
+        input_data = json.loads(
+            (cr_dir / "verifier_inputs" / "bha_1.json").read_text(),
+        )
+        assert input_data["review_root"] == str(root)
+
+    def test_git_quoted_changed_file_edited_after_resolution_errors(
+        self, tmp_path: Path,
+    ) -> None:
+        # Skipping quoted entries dropped this file from both the containment
+        # and the drift check, so an edit to it after resolution went unseen.
+        cr_dir, root, _recorded = self._seed_root_with_git_quoted_changed_file(
+            tmp_path,
+        )
+        (root / "src" / "café.py").write_text("edited, never committed\n")
+
+        finding = _make_validated_finding("bha_1", severity="HIGH")
+        rc, _manifest = _run_verify_prepare(tmp_path, [finding], cr_dir=cr_dir)
+
+        assert rc == 3
+        assert not (cr_dir / "verifier_inputs" / "bha_1.json").exists()
+
+    def test_malformed_git_quoted_entry_errors(
+        self, tmp_path: Path, capsys: pytest.CaptureFixture[str],
+    ) -> None:
+        # An entry that looks quoted but does not decode cannot be resolved on
+        # disk, so it refuses rather than being skipped or guessed at.
+        cr_dir, _root = self._seed_live_root_with_changed_file(tmp_path)
         (cr_dir / "diff_data.json").write_text(json.dumps({
-            "files_to_review": ['"src/caf\\303\\251.py"'],
-            "file_statuses": {'"src/caf\\303\\251.py"': "added"},
+            "files_to_review": ["src/a.py", '"src/unterminated.py'],
+            "file_statuses": {
+                "src/a.py": "modified", '"src/unterminated.py': "added",
+            },
         }))
 
         finding = _make_validated_finding("bha_1", severity="HIGH")
         rc, _manifest = _run_verify_prepare(tmp_path, [finding], cr_dir=cr_dir)
 
-        assert rc == 0
+        assert rc == 3
+        assert not (cr_dir / "verifier_inputs" / "bha_1.json").exists()
+        assert "not valid git path quoting" in capsys.readouterr().err
 
 
 class TestFooterWorktreeTeardown: