From 90ae3497331fa12d3e4fede8dd642dccf6798500 Mon Sep 17 00:00:00 2001 From: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> Date: Wed, 11 Nov 2026 08:00:39 +0000 Subject: [PATCH 1/3] fix(canary): preserve exact changed Git paths The canary selector captured `--name-only -z` with text mode. Python's universal-newline translation changed a literal CR in a Git pathname into LF, while the common dedupe helper stripped valid leading and trailing filename spaces. The selector then reported names that did not exist and could miss the checks for the changed files. Keep Git output framed as bytes, decode valid UTF-8 losslessly, and retain raw display identities for invalid byte names. Deduplicate derived names without rewriting them; explicit --changed-file input keeps its existing normalisation. Signed-off-by: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> --- loopx/cli_commands/canary.py | 40 +++++++++++++++++++++------ tests/canary/test_git_path_framing.py | 33 +++++++++++++++++++++- 2 files changed, 64 insertions(+), 9 deletions(-) diff --git a/loopx/cli_commands/canary.py b/loopx/cli_commands/canary.py index f85f743a58..d9553ce524 100644 --- a/loopx/cli_commands/canary.py +++ b/loopx/cli_commands/canary.py @@ -70,6 +70,26 @@ 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 _display_git_path(record: bytes) -> str: + """Render a raw pathname without losing bytes that Git emitted.""" + try: + return record.decode("utf-8") + except UnicodeDecodeError: + return repr(record) + + 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 +99,20 @@ 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, ) - files = [name for name in completed.stdout.split("\0") if name] + files = [ + _display_git_path(record) + for record in completed.stdout.split(b"\0") + if record + ] return { "ok": completed.returncode == 0, "returncode": completed.returncode, "command": command, "changed_files": files, - "stderr_tail": completed.stderr[-800:], + "stderr_tail": completed.stderr.decode("utf-8", "replace")[-800:], } @@ -121,7 +144,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() @@ -154,7 +177,7 @@ def collect_git_diff_changed_files( def _resolve_canary_changed_files(args: argparse.Namespace) -> tuple[list[str], dict[str, Any] | None]: - changed_files = list(args.changed_file or []) + changed_files = _dedupe_preserving_order(list(args.changed_file or [])) git_diff_selector = None if bool(getattr(args, "from_git_diff", False)): repo_root = _resolve_git_repo_root(Path.cwd()) @@ -162,12 +185,13 @@ 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 _attach_selector_sources( diff --git a/tests/canary/test_git_path_framing.py b/tests/canary/test_git_path_framing.py index babc2cc810..1ba8e290b9 100644 --- a/tests/canary/test_git_path_framing.py +++ b/tests/canary/test_git_path_framing.py @@ -7,7 +7,7 @@ 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 NEL = "\x85" @@ -77,3 +77,34 @@ 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) From 72e0bca67d45873dfb1db7f5a493150bfda9dd1f Mon Sep 17 00:00:00 2001 From: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> Date: Wed, 11 Nov 2026 09:45:18 +0000 Subject: [PATCH 2/3] fix(canary): fail closed on undecodable Git paths Signed-off-by: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> --- loopx/cli_commands/canary.py | 101 +++++++++++++++++++++----- tests/canary/test_git_path_framing.py | 66 +++++++++++++++++ 2 files changed, 148 insertions(+), 19 deletions(-) diff --git a/loopx/cli_commands/canary.py b/loopx/cli_commands/canary.py index d9553ce524..ae57880af6 100644 --- a/loopx/cli_commands/canary.py +++ b/loopx/cli_commands/canary.py @@ -82,14 +82,6 @@ def _dedupe_exact_paths(values: list[str]) -> list[str]: return deduped -def _display_git_path(record: bytes) -> str: - """Render a raw pathname without losing bytes that Git emitted.""" - try: - return record.decode("utf-8") - except UnicodeDecodeError: - return repr(record) - - 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 @@ -99,19 +91,30 @@ def _run_git_name_only(repo_root: Path, args: list[str]) -> dict[str, Any]: completed = subprocess.run( command, check=False, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + capture_output=True, ) - files = [ - _display_git_path(record) - for record in completed.stdout.split(b"\0") - if record - ] + 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, + "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:], } @@ -161,17 +164,30 @@ 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, } @@ -194,6 +210,33 @@ def _resolve_canary_changed_files(args: argparse.Namespace) -> tuple[list[str], 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( payload: dict[str, Any], *, @@ -202,6 +245,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: @@ -611,6 +658,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, @@ -625,6 +676,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, @@ -643,6 +698,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"), @@ -685,6 +744,10 @@ 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) + 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 1ba8e290b9..9c3ecd663e 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, collect_git_diff_changed_files +from tests.source_cli import run_source_cli NEL = "\x85" @@ -108,3 +113,64 @@ def test_run_git_name_only_preserves_cr_and_surrounding_spaces(tmp_path: Path) - 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 From 5d89a2431c7769cecb611aa07c71cf0d89e27c7a Mon Sep 17 00:00:00 2001 From: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> Date: Wed, 11 Nov 2026 21:23:40 +0800 Subject: [PATCH 3/3] fix(canary): preserve exact Git paths through premerge Premerge deduplicated with _norm_path, which rewrote literal backslashes and stripped trailing spaces after selection. Keep Git-provided identities intact for classification, catalog selection, direct checks and boundary scans, while retaining path normalization only for surface matching. Keep explicit --changed-file slash normalization for premerge callers. Add a real Git regression with six distinct paths, including a literal backslash, CR, leading space, and two names differing only by a trailing space. Signed-off-by: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> --- loopx/canary/premerge.py | 8 +-- loopx/cli_commands/canary.py | 15 ++++- tests/canary/test_git_path_framing.py | 85 +++++++++++++++++++++++++++ 3 files changed, 101 insertions(+), 7 deletions(-) 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 ae57880af6..557813319b 100644 --- a/loopx/cli_commands/canary.py +++ b/loopx/cli_commands/canary.py @@ -192,8 +192,15 @@ def collect_git_diff_changed_files( } -def _resolve_canary_changed_files(args: argparse.Namespace) -> tuple[list[str], dict[str, Any] | None]: - changed_files = _dedupe_preserving_order(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()) @@ -743,7 +750,9 @@ 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 ): diff --git a/tests/canary/test_git_path_framing.py b/tests/canary/test_git_path_framing.py index 9c3ecd663e..061923c766 100644 --- a/tests/canary/test_git_path_framing.py +++ b/tests/canary/test_git_path_framing.py @@ -174,3 +174,88 @@ def test_git_byte_paths_fail_closed_before_display_and_dedupe(tmp_path: Path) -> 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"]