diff --git a/loopx/capabilities/issue_fix/reviewer_recommendation.py b/loopx/capabilities/issue_fix/reviewer_recommendation.py index 55b151db37..1db50658ac 100644 --- a/loopx/capabilities/issue_fix/reviewer_recommendation.py +++ b/loopx/capabilities/issue_fix/reviewer_recommendation.py @@ -79,6 +79,26 @@ def _run_git(repo_path: Path, args: Sequence[str]) -> str: return result.stdout +def _run_git_bytes(repo_path: Path, args: Sequence[str]) -> bytes: + """Run a path-listing git command without text decoding. + + ``text=True`` applies universal-newline translation before any framing + happens, so a pathname carrying CR or CRLF can arrive as an extra LF and + split one record into two. Callers that parse machine output frame the raw + bytes on their own separator. + """ + result = subprocess.run( + ["git", *args], + cwd=repo_path, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + timeout=20, + ) + if result.returncode != 0: + raise RuntimeError(f"git {' '.join(args)} failed with exit code {result.returncode}") + return result.stdout + + def _normalise_changed_file(value: Any) -> str: text = str(value or "").strip().replace("\\", "/") while text.startswith("./"): @@ -89,6 +109,23 @@ def _normalise_changed_file(value: Any) -> str: return path.as_posix() +def _validate_derived_changed_file(value: Any) -> str: + """Validate a Git path after NUL framing without rewriting its bytes. + + Git already emits a repository-relative name. Treating a POSIX literal + backslash as a Windows separator made a valid root file such as + ``\\notes.txt`` look absolute during packet validation. Keep the explicit + ``--changed-file`` normalization above as the compatibility boundary for + caller-supplied path syntax, and validate derived names as emitted. + """ + if not isinstance(value, str) or not value: + raise ValueError("changed files must be non-empty repo-relative paths") + path = PurePosixPath(value) + if path.as_posix() == "." or path.is_absolute() or ".." in path.parts: + raise ValueError("changed files must be non-empty repo-relative paths") + return value + + def _normalise_changed_files(values: Sequence[Any]) -> list[str]: files: list[str] = [] for raw in values: @@ -624,14 +661,34 @@ def _apply_reviewer_sources_for_path( def _derive_changed_files(repo_path: Path, base_ref: str) -> list[str]: - output = _run_git(repo_path, ["diff", "--name-only", f"{base_ref}...HEAD"]) - # `git diff --name-only` frames one pathname per LF. Under the default - # `core.quotePath=true` a non-ASCII pathname arrives octal-escaped, but a - # repository may set `core.quotePath=false`, and a pathname carrying - # U+0085/U+2028/U+2029 is then emitted raw. `str.splitlines()` treats those - # as record boundaries and would tear one path into two, so frame on LF and - # drop the empty trailing record instead. - return _normalise_changed_files([line for line in output.split("\n") if line]) + output = _run_git_bytes( + repo_path, + ["diff", "--name-only", "-z", "--no-ext-diff", f"{base_ref}...HEAD"], + ) + # `-z` frames one raw pathname per NUL byte, so the name reaches this + # function exactly as Git recorded it. With the default + # `core.quotePath=true`, Git C-quotes non-ASCII and control-character + # names in a line-oriented listing; normalising those quotes as literal + # path text produced names such as "/344/270/255/..." that match neither + # CODEOWNERS rules nor commit history. NUL framing avoids the quoting + # entirely, and decoding each record strictly keeps CR, CRLF, leading and + # trailing spaces and POSIX literal backslashes intact. + files: list[str] = [] + for record in output.split(b"\0"): + if not record: + continue + try: + name = record.decode("utf-8") + except UnicodeDecodeError as error: + raise ValueError( + "changed files derived from git must be valid UTF-8" + ) from error + _validate_derived_changed_file(name) + if name not in files: + files.append(name) + if len(files) > 100: + raise ValueError("reviewer recommendation accepts at most 100 changed files") + return files def _collect_history( @@ -641,9 +698,17 @@ def _collect_history( revision: str, history_limit: int, ) -> list[tuple[str, str]]: + # NUL framing gives an exact pathname, but ``git log -- `` still + # interprets the argument as a pathspec. A changed file whose name + # contains a wildcard or starts with pathspec magic such as + # ``:(exclude)`` would then collect commits for other files and credit + # their authors as historical contributors of this path. Literal + # pathspecs apply to the exact changed file and to the module fallback + # below, which share this helper. output = _run_git( repo_path, [ + "--literal-pathspecs", "log", f"--max-count={history_limit}", "--format=%aN%x1f%aE", @@ -967,7 +1032,7 @@ def validate_issue_fix_reviewer_recommendation_packet( changed_files = [] for path in changed_files: try: - _normalise_changed_file(path) + _validate_derived_changed_file(path) except ValueError: errors.append("changed_files must contain only repo-relative paths") break diff --git a/tests/capabilities/test_issue_fix_reviewer_history_framing.py b/tests/capabilities/test_issue_fix_reviewer_history_framing.py index aeaddf10b1..71e2e97ea1 100644 --- a/tests/capabilities/test_issue_fix_reviewer_history_framing.py +++ b/tests/capabilities/test_issue_fix_reviewer_history_framing.py @@ -1,5 +1,8 @@ from __future__ import annotations +import contextlib +import io +import json import os import subprocess from pathlib import Path @@ -9,7 +12,9 @@ from loopx.capabilities.issue_fix.reviewer_recommendation import ( _collect_history, _derive_changed_files, + build_issue_fix_reviewer_recommendation_packet, ) +from loopx.cli import main as cli_main AUTHOR_NAME_WITH_SEPARATOR = "Zo{separator}e" @@ -111,6 +116,185 @@ def test_derive_changed_files_frames_records_on_lf_when_quote_path_is_disabled( assert _derive_changed_files(repo, "HEAD~1") == [path] +@pytest.mark.parametrize( + "path", + [ + "中文名字.txt", + "cr\rname.txt", + "crlf\r\nname.txt", + " leading-space.txt", + "trailing-space.txt ", + "back\\slash.txt", + ], +) +def test_derive_changed_files_keeps_paths_git_would_c_quote( + tmp_path: Path, + path: str, +) -> None: + """One `git diff --name-only -z` record must keep the exact pathname. + + With the default `core.quotePath=true`, a line-oriented listing C-quotes + non-ASCII and control-character names. Replacing the quote escapes with + slashes turned `中文名字.txt` into `/344/270/255/...`, so CODEOWNERS rules + and commit history were queried for a path that does not exist. NUL framing + and strict record decoding must preserve the exact name, including CR, + CRLF, surrounding spaces and a POSIX literal backslash. + """ + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init", "-b", "main") + _git(repo, "config", "user.name", "LoopX Test") + _git(repo, "config", "user.email", "loopx@example.invalid") + + (repo / "base.txt").write_text("base\n", encoding="utf-8") + _git(repo, "add", "base.txt") + _git(repo, "commit", "-m", "base") + + (repo / path).write_text("x\n", encoding="utf-8") + _git(repo, "add", "--", path) + _git(repo, "commit", "-m", "add a path Git would quote") + + assert _derive_changed_files(repo, "HEAD~1") == [path] + + +@pytest.mark.parametrize( + ("path", "other"), + [ + ("note\n*.txt", "note\nX.txt"), + (":(exclude)skip\n.txt", "other.txt"), + ("\\notes.txt", "other.txt"), + (" ", "other.txt"), + ("back\\..\\slash.txt", "other.txt"), + ], +) +def test_reviewer_plan_history_binds_the_literal_changed_path( + tmp_path: Path, + path: str, + other: str, +) -> None: + """History must query the changed path literally, not as a pathspec. + + A pathname containing a wildcard, or starting with pathspec magic such as + ``:(exclude)``, was trusted to Git as a pathspec by ``git log -- ``. + The query then collected commits for different files and ranked their + author first as an exact changed-path contributor. + """ + work_repo = tmp_path / "repo" + work_repo.mkdir() + _git(work_repo, "init", "-b", "main") + _git(work_repo, "config", "user.name", "LoopX Test") + _git(work_repo, "config", "user.email", "loopx@example.invalid") + + (work_repo / path).write_text("base\n", encoding="utf-8") + (work_repo / other).write_text("other\n", encoding="utf-8") + _git(work_repo, "--literal-pathspecs", "add", "--", path, other) + _git(work_repo, "commit", "-m", "base") + + (work_repo / other).write_text("other change\n", encoding="utf-8") + env = dict(os.environ) + env.update( + GIT_AUTHOR_NAME="Other Author", + GIT_AUTHOR_EMAIL="other@example.invalid", + GIT_COMMITTER_NAME="LoopX Test", + GIT_COMMITTER_EMAIL="loopx@example.invalid", + ) + _git(work_repo, "commit", "-am", "other author edits its own file", env=env) + + (work_repo / path).write_text("target change\n", encoding="utf-8") + _git(work_repo, "--literal-pathspecs", "add", "--", path) + _git(work_repo, "commit", "-m", "adds the changed path") + + packet = build_issue_fix_reviewer_recommendation_packet( + repo_path=work_repo, + repo="approved_local_repo", + base_ref="HEAD~1", + history_limit=10, + execute=True, + generated_at="2026-08-01T00:00:00Z", + ) + + assert packet["changed_files"] == [path] + assert [row["display_name"] for row in packet["candidates"]] == ["LoopX Test"] + assert packet["candidates"][0]["history_commit_count"] == 1 + assert "changed_path_commit_history" in packet["candidates"][0]["reason_codes"] + + +def test_reviewer_plan_cli_accepts_derived_literal_backslash_root_path( + tmp_path: Path, +) -> None: + r"""The public CLI must accept a derived root file whose name starts ``\``.""" + work_repo = tmp_path / "repo" + work_repo.mkdir() + _git(work_repo, "init", "-b", "main") + _git(work_repo, "config", "user.name", "LoopX Test") + _git(work_repo, "config", "user.email", "loopx@example.invalid") + + path = "\\notes.txt" + (work_repo / path).write_text("base\n", encoding="utf-8") + _git(work_repo, "--literal-pathspecs", "add", "--", path) + _git(work_repo, "commit", "-m", "base") + (work_repo / path).write_text("target change\n", encoding="utf-8") + _git(work_repo, "--literal-pathspecs", "add", "--", path) + _git(work_repo, "commit", "-m", "target change") + + output = io.StringIO() + with contextlib.redirect_stdout(output): + result = cli_main( + [ + "--format", + "json", + "issue-fix", + "reviewer-plan", + "--repo-path", + str(work_repo), + "--repo", + "approved_local_repo", + "--base-ref", + "HEAD~1", + "--execute", + "--generated-at", + "2026-08-01T00:00:00Z", + ] + ) + assert result == 0, output.getvalue() + packet = json.loads(output.getvalue()) + assert packet["ok"] is True + assert packet["changed_files"] == [path] + assert [row["display_name"] for row in packet["candidates"]] == ["LoopX Test"] + assert packet["candidates"][0]["history_commit_count"] == 1 + + +def test_reviewer_plan_preview_keeps_explicit_path_normalisation( + tmp_path: Path, +) -> None: + """Explicit --changed-file input keeps its established separator normalisation.""" + missing_repo = tmp_path / "not-read" + output = io.StringIO() + with contextlib.redirect_stdout(output): + result = cli_main( + [ + "--format", + "json", + "issue-fix", + "reviewer-plan", + "--repo-path", + str(missing_repo), + "--repo", + "approved_local_repo", + "--changed-file", + "docs\\one.py", + "--generated-at", + "2026-08-01T00:00:00Z", + ] + ) + assert result == 0, output.getvalue() + packet = json.loads(output.getvalue()) + assert packet["changed_files"] == ["docs/one.py"] + assert packet["recommendation_status"] == "preview_only" + assert packet["private_repo_state_read"] is False + assert not missing_repo.exists() + + def test_derive_changed_files_keeps_ordinary_ascii_paths(tmp_path: Path) -> None: repo = tmp_path / "repo" repo.mkdir()