diff --git a/loopx/canary/premerge.py b/loopx/canary/premerge.py index 0df7aeda38..d0d266344f 100644 --- a/loopx/canary/premerge.py +++ b/loopx/canary/premerge.py @@ -303,14 +303,14 @@ def _path_matches(path: str, tokens: tuple[str, ...]) -> bool: def _dedupe(values: list[str]) -> list[str]: + """Deduplicate path identities without rewriting Git-provided values.""" seen: set[str] = set() deduped: list[str] = [] for value in values: - normalized = _norm_path(value) - if not normalized or normalized in seen: + if not value or value in seen: continue - seen.add(normalized) - deduped.append(normalized) + seen.add(value) + deduped.append(value) return deduped diff --git a/loopx/cli_commands/canary.py b/loopx/cli_commands/canary.py index f85f743a58..557813319b 100644 --- a/loopx/cli_commands/canary.py +++ b/loopx/cli_commands/canary.py @@ -70,6 +70,18 @@ def _dedupe_preserving_order(values: list[str]) -> list[str]: return deduped +def _dedupe_exact_paths(values: list[str]) -> list[str]: + """Deduplicate Git-derived paths without rewriting their identity.""" + seen: set[str] = set() + deduped: list[str] = [] + for value in values: + if not value or value in seen: + continue + seen.add(value) + deduped.append(value) + return deduped + + def _run_git_name_only(repo_root: Path, args: list[str]) -> dict[str, Any]: # `-z` frames each pathname on NUL. LF framing is only unambiguous while # `core.quotePath` escapes non-ASCII pathnames, and a repository may turn @@ -79,17 +91,31 @@ def _run_git_name_only(repo_root: Path, args: list[str]) -> dict[str, Any]: completed = subprocess.run( command, check=False, - text=True, encoding="utf-8", errors="replace", - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + capture_output=True, ) - files = [name for name in completed.stdout.split("\0") if name] + records = [record for record in completed.stdout.split(b"\0") if record] + files: list[str] = [] + undecodable_count = 0 + for record in records: + try: + files.append(record.decode("utf-8")) + except UnicodeDecodeError: + undecodable_count += 1 + + path_error = None + if undecodable_count: + path_error = ( + f"Git returned {undecodable_count} changed pathname(s) that are not valid UTF-8; " + "canary cannot preserve their identity in the selector output" + ) return { - "ok": completed.returncode == 0, + "ok": completed.returncode == 0 and path_error is None, "returncode": completed.returncode, "command": command, - "changed_files": files, - "stderr_tail": completed.stderr[-800:], + "changed_files": files if path_error is None else [], + "undecodable_path_count": undecodable_count, + "path_error": path_error, + "stderr_tail": completed.stderr.decode("utf-8", "replace")[-800:], } @@ -121,7 +147,7 @@ def collect_git_diff_changed_files( "unstaged": _run_git_name_only(repo_root, ["diff", "--name-only"]), "untracked": _run_git_name_only(repo_root, ["ls-files", "--others", "--exclude-standard"]), } - changed_files = _dedupe_preserving_order( + changed_files = _dedupe_exact_paths( [ file for source in sources.values() @@ -138,23 +164,43 @@ def collect_git_diff_changed_files( "source": name, "returncode": source.get("returncode"), "stderr_tail": source.get("stderr_tail"), + **({"error": source["path_error"]} if source.get("path_error") else {}), } for name, source in sources.items() if not source.get("ok") ] + undecodable_path_count = sum( + int(source.get("undecodable_path_count") or 0) + for source in sources.values() + ) + path_error = ( + "Git returned changed pathnames that are not valid UTF-8; canary " + "cannot preserve their identity in the selector output" + if undecodable_path_count + else None + ) return { - "ok": bool(successful_sources), + "ok": bool(successful_sources) and path_error is None, "base_ref": base_ref, "repo_root": str(repo_root), - "changed_files": changed_files, + "changed_files": [] if path_error is not None else changed_files, "changed_file_count": len(changed_files), "successful_sources": successful_sources, + "undecodable_path_count": undecodable_path_count, + **({"error": path_error} if path_error else {}), "warnings": warnings, } -def _resolve_canary_changed_files(args: argparse.Namespace) -> tuple[list[str], dict[str, Any] | None]: - changed_files = list(args.changed_file or []) +def _resolve_canary_changed_files( + args: argparse.Namespace, + *, + normalize_manual: bool = False, +) -> tuple[list[str], dict[str, Any] | None]: + manual_files = list(args.changed_file or []) + if normalize_manual: + manual_files = [path.strip().replace("\\", "/") for path in manual_files] + changed_files = _dedupe_preserving_order(manual_files) git_diff_selector = None if bool(getattr(args, "from_git_diff", False)): repo_root = _resolve_git_repo_root(Path.cwd()) @@ -162,12 +208,40 @@ def _resolve_canary_changed_files(args: argparse.Namespace) -> tuple[list[str], repo_root=repo_root, base_ref=str(getattr(args, "git_diff_base", "origin/main") or "origin/main"), ) - changed_files.extend( + derived = [ file for file in (git_diff_selector.get("changed_files") or []) if isinstance(file, str) - ) - return _dedupe_preserving_order(changed_files), git_diff_selector + ] + changed_files = _dedupe_exact_paths([*changed_files, *derived]) + return changed_files, git_diff_selector + + +def _git_diff_selector_error(git_diff_selector: dict[str, Any] | None) -> str | None: + if git_diff_selector is None or git_diff_selector.get("ok"): + return None + return str( + git_diff_selector.get("error") + or "Git changed-path selection failed; refusing to return an incomplete path set" + ) + + +def _reject_git_diff_selector( + git_diff_selector: dict[str, Any] | None, + args: argparse.Namespace, + print_payload: PrintPayload, + format_selector: FormatSelector, +) -> bool: + error = _git_diff_selector_error(git_diff_selector) + if error is None or git_diff_selector is None: + return False + payload = { + "ok": False, + "error": error, + "selector_sources": {"git_diff": git_diff_selector}, + } + print_payload(payload, format_selector(args), lambda value: str(value["error"]) + "\n") + return True def _attach_selector_sources( @@ -178,6 +252,10 @@ def _attach_selector_sources( if git_diff_selector is None: return payload["selector_sources"] = {"git_diff": git_diff_selector} + error = _git_diff_selector_error(git_diff_selector) + if error is not None: + payload["ok"] = False + payload["error"] = error def _print_smoke_suite_progress(event: dict[str, Any]) -> None: @@ -587,6 +665,10 @@ def handle_canary_command( renderer = render_exact_release_commit_qualification_markdown elif args.canary_command == "plan": changed_files, git_diff_selector = _resolve_canary_changed_files(args) + if _reject_git_diff_selector( + git_diff_selector, args, print_payload, output_format + ): + return 1 payload = build_catalog_canary_plan( catalog_path=args.catalog, changed_files=changed_files, @@ -601,6 +683,10 @@ def handle_canary_command( renderer = render_catalog_canary_plan_markdown elif args.canary_command == "run": changed_files, git_diff_selector = _resolve_canary_changed_files(args) + if _reject_git_diff_selector( + git_diff_selector, args, print_payload, output_format + ): + return 1 payload = build_catalog_canary_run( repo_root=_resolve_git_repo_root(Path.cwd()), catalog_path=args.catalog, @@ -619,6 +705,10 @@ def handle_canary_command( renderer = render_catalog_canary_run_markdown elif args.canary_command == "smoke-suite": changed_files, git_diff_selector = _resolve_canary_changed_files(args) + if _reject_git_diff_selector( + git_diff_selector, args, print_payload, output_format + ): + return 1 payload = build_canary_smoke_suite_run( repo_root=_resolve_git_repo_root(Path.cwd()), suite=str(args.suite or "default-public"), @@ -660,7 +750,13 @@ def handle_canary_command( ) renderer = render_quality_surface_catalog_audit_markdown elif args.canary_command == "premerge": - changed_files, git_diff_selector = _resolve_canary_changed_files(args) + changed_files, git_diff_selector = _resolve_canary_changed_files( + args, normalize_manual=True + ) + if _reject_git_diff_selector( + git_diff_selector, args, print_payload, output_format + ): + return 1 target_repo_root = _resolve_git_repo_root(Path.cwd()) try: catalog_path = resolve_canary_catalog_path(args.catalog, repo_root=target_repo_root) diff --git a/tests/canary/test_git_path_framing.py b/tests/canary/test_git_path_framing.py index babc2cc810..061923c766 100644 --- a/tests/canary/test_git_path_framing.py +++ b/tests/canary/test_git_path_framing.py @@ -2,12 +2,17 @@ from __future__ import annotations +import json +import os import subprocess from pathlib import Path +import pytest + from loopx.canary.maintainability_ratchet import tracked_python_paths from loopx.canary.runner import _tracked_change_paths -from loopx.cli_commands.canary import _run_git_name_only +from loopx.cli_commands.canary import _run_git_name_only, collect_git_diff_changed_files +from tests.source_cli import run_source_cli NEL = "\x85" @@ -77,3 +82,180 @@ def test_tracked_change_paths_keeps_one_record_per_path(tmp_path: Path) -> None: assert ok is True, detail assert paths == sorted([committed.encode("utf-8"), staged.encode("utf-8")]) + + +def test_run_git_name_only_preserves_cr_and_surrounding_spaces(tmp_path: Path) -> None: + """Text decoding must not rewrite a valid Git pathname. + + Universal-newline translation turns a literal carriage return into LF, and + trimming removes valid leading or trailing filename spaces. Both altered + the canary selector's path identity before it could match changed files. + """ + 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.py").write_text("x = 1\n", encoding="utf-8") + _git(repo, "add", "base.py") + _git(repo, "commit", "-m", "base") + + names = [" leading.py", "cr\rname.py", "trailing.py "] + for name in names: + (repo / name).write_text("x = 2\n", encoding="utf-8") + _git(repo, "add", "--", name) + _git(repo, "commit", "-m", "add exact path records") + + result = _run_git_name_only(repo, ["diff", "--name-only", "HEAD~1...HEAD"]) + assert result["ok"] is True + assert result["changed_files"] == sorted(names) + + selector = collect_git_diff_changed_files(repo_root=repo, base_ref="HEAD~1") + assert selector["changed_files"] == sorted(names) + assert selector["changed_file_count"] == len(names) + +@pytest.mark.skipif(os.name != "posix", reason="non-UTF-8 Git paths are POSIX-only") +def test_git_byte_paths_fail_closed_before_display_and_dedupe(tmp_path: Path) -> None: + """An undecodable name must not collide with a literal repr-like name.""" + 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.py").write_text("x = 1\n", encoding="utf-8") + _git(repo, "add", "base.py") + _git(repo, "commit", "-m", "base") + + literal_repr = "b'\\xff.py'" + (repo / literal_repr).write_text("literal\n", encoding="utf-8") + _git(repo, "add", literal_repr) + blob = subprocess.run( + ["git", "-C", str(repo), "hash-object", "-w", "--stdin"], + input=b"raw\n", stdout=subprocess.PIPE, check=True, + ).stdout.strip() + raw_path = b"\xff.py" + subprocess.run( + ["git", "-C", str(repo), "update-index", "-z", "--index-info"], + input=b"100644 " + blob + b" 0\t" + raw_path + b"\0", check=True, + ) + _git(repo, "commit", "-m", "literal and raw byte paths") + + raw_records = subprocess.run( + ["git", "-C", str(repo), "diff", "--name-only", "-z", "HEAD~1...HEAD"], + stdout=subprocess.PIPE, check=True, + ).stdout + assert {record for record in raw_records.split(b"\0") if record} == { + literal_repr.encode("utf-8"), + raw_path, + } + + source = _run_git_name_only(repo, ["diff", "--name-only", "HEAD~1...HEAD"]) + assert source["ok"] is False + assert source["undecodable_path_count"] == 1 + assert source["changed_files"] == [] + + selector = collect_git_diff_changed_files(repo_root=repo, base_ref="HEAD~1") + assert selector["ok"] is False + assert selector["undecodable_path_count"] >= 1 + assert "not valid UTF-8" in selector["error"] + assert selector["changed_files"] == [] + + response = run_source_cli( + [ + "--format", "json", "canary", "plan", "--from-git-diff", + "--git-diff-base", "HEAD~1", + ], + cwd=repo, check=False, timeout=60, + ) + assert response.returncode != 0 + assert response.stdout, response.stderr + payload = json.loads(response.stdout) + assert payload["ok"] is False + assert "not valid UTF-8" in payload["error"] + assert payload["selector_sources"]["git_diff"]["undecodable_path_count"] >= 1 + assert "selection_inputs" not in payload + + +@pytest.mark.skipif( + os.name != "posix", + reason="literal backslash and trailing space pathnames are POSIX-only", +) +def test_premerge_preserves_exact_git_path_identities(tmp_path: Path) -> None: + """Every Git path identity must survive premerge selection unchanged.""" + from loopx.canary.planner import DEFAULT_CATALOG_PATH + + 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") + catalog = repo / "docs/concepts/interaction-pattern-catalog.md" + catalog.parent.mkdir(parents=True) + catalog.write_bytes(DEFAULT_CATALOG_PATH.read_bytes()) + _git(repo, "add", "docs/concepts/interaction-pattern-catalog.md") + _git(repo, "commit", "-m", "catalog") + + names = [ + "docs/back\\slash.txt", + "fixtures/back\\slash.py", + "fixtures/cr\rname.py", + "fixtures/leading.py ", + "fixtures/trailing.py", + "fixtures/trailing.py ", + ] + (repo / "fixtures").mkdir() + (repo / "docs").mkdir(exist_ok=True) + for name in names: + (repo / name).write_text("value = 1\n", encoding="utf-8") + _git(repo, "add", "--", name) + _git(repo, "commit", "-m", "exact changed paths") + + selector = collect_git_diff_changed_files(repo_root=repo, base_ref="HEAD~1") + assert selector["changed_files"] == names + assert selector["changed_file_count"] == 6 + + response = run_source_cli( + [ + "--format", "json", "canary", "premerge", "--from-git-diff", + "--git-diff-base", "HEAD~1", "--tier", "quick", "--no-execute", + ], + cwd=repo, check=False, timeout=60, + ) + assert response.returncode == 0, response.stderr + payload = json.loads(response.stdout) + assert payload["changed_files"] == names + assert payload["classification"]["changed_files"] == names + assert payload["classification"]["changed_file_count"] == 6 + assert payload["catalog_run"]["selection_inputs"]["changed_files"] == names + boundary_run = payload["boundary_run"] + assert boundary_run is not None + assert boundary_run["selection_inputs"]["changed_files"] == names + assert boundary_run["selection_inputs"]["scan_paths"] == names + + +@pytest.mark.skipif(os.name != "posix", reason="POSIX CLI path spelling") +def test_premerge_keeps_explicit_changed_file_normalization(tmp_path: Path) -> None: + """Manual --changed-file paths keep their historical slash normalization.""" + from loopx.canary.planner import DEFAULT_CATALOG_PATH + + 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") + catalog = repo / "docs/concepts/interaction-pattern-catalog.md" + catalog.parent.mkdir(parents=True) + catalog.write_bytes(DEFAULT_CATALOG_PATH.read_bytes()) + _git(repo, "add", "docs/concepts/interaction-pattern-catalog.md") + _git(repo, "commit", "-m", "catalog") + + response = run_source_cli( + [ + "--format", "json", "canary", "premerge", "--changed-file", + "docs\\notes.py", "--tier", "quick", "--no-execute", + ], + cwd=repo, check=False, timeout=60, + ) + assert response.returncode == 0, response.stderr + payload = json.loads(response.stdout) + assert payload["changed_files"] == ["docs/notes.py"]