From 917ebbde55b0320714d352f8e2ff8447b36e9bf0 Mon Sep 17 00:00:00 2001 From: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> Date: Sun, 8 Nov 2026 10:04:21 +0000 Subject: [PATCH 1/2] fix(change-quality): read Git paths without quoting or newline splitting Signed-off-by: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> --- loopx/capabilities/change_quality/scope.py | 89 ++++++---------------- tests/test_change_quality_scope_paths.py | 81 ++++++++++++++++++++ 2 files changed, 104 insertions(+), 66 deletions(-) create mode 100644 tests/test_change_quality_scope_paths.py diff --git a/loopx/capabilities/change_quality/scope.py b/loopx/capabilities/change_quality/scope.py index 5a66a75a42..593c898aa5 100644 --- a/loopx/capabilities/change_quality/scope.py +++ b/loopx/capabilities/change_quality/scope.py @@ -1,43 +1,14 @@ from __future__ import annotations import hashlib -import os import subprocess from pathlib import Path from typing import Any -from ...extensions.process_runtime import retire_owned_process CHANGE_QUALITY_SCOPE_SCHEMA_VERSION = "change_quality_scope_v0" -# The scope reads a checkout the operator may not control. A repository blocked -# on a stale index lock, a network mount or an oversized history must not hold -# the receipt open with no answer, so every read shares one budget. -GIT_PROBE_TIMEOUT_SECONDS = 10.0 - - -class GitProbeUnavailable(ValueError): - """Git did not answer inside the read budget, or could not start.""" - - -def _retire_owned_probe(process: subprocess.Popen[Any]) -> None: - """Stop the read's own session, then drop its pipes unread. - - Git runs helpers -- fsmonitor hooks, pagers -- inside the session it starts - in, so killing the leader alone leaves them behind. An inherited pipe also - outlives the leader, so the pipes are closed instead of drained: a drain - would wait on work this call no longer owns. - """ - - try: - retire_owned_process(process, process_tree_owned=True) - except (OSError, RuntimeError, ValueError, subprocess.SubprocessError): - # Cleanup is best effort. The read's own timeout or cancellation is the - # result the caller must receive, even when termination is unconfirmed. - pass - - def _git( repo_root: Path, *args: str, @@ -47,35 +18,14 @@ def _git( # text mode: a non-None encoding would silently flip bytes-mode callers # into text mode because CPython treats any of text/encoding/errors as # text-mode requests. - argv = ["git", "-C", str(repo_root), *args] - budget = GIT_PROBE_TIMEOUT_SECONDS - try: - process = subprocess.Popen( - argv, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=text, - encoding="utf-8" if text else None, - errors="replace" if text else None, - start_new_session=os.name == "posix", - ) - except OSError as exc: - raise GitProbeUnavailable(f"git {' '.join(args)} could not start: {exc}") from exc - try: - stdout, stderr = process.communicate(timeout=budget) - except subprocess.TimeoutExpired: - _retire_owned_probe(process) - raise GitProbeUnavailable( - f"git {' '.join(args)} did not answer within {budget:.1f}s" - ) from None - except BaseException: - # A terminal Ctrl-C reaches this process's group, not the session the - # read started, so the cancellation retires its helpers here and then - # keeps travelling to its own handler. - _retire_owned_probe(process) - raise - return subprocess.CompletedProcess( - args=argv, returncode=process.returncode, stdout=stdout, stderr=stderr + return subprocess.run( + ["git", "-C", str(repo_root), *args], + check=False, + text=text, + encoding="utf-8" if text else None, + errors="replace" if text else None, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, ) @@ -99,15 +49,19 @@ def _required_git_text(repo_root: Path, *args: str) -> str: def _name_only(repo_root: Path, *args: str) -> list[str]: + """Return the exact paths Git reported, one per NUL-separated record. + + ``-z`` is required at every call site: without it Git quotes non-ASCII + names (``"\\346\\226\\226..."``) and separates records with newlines, so a + legal file name that contains a newline or spaces would arrive as + mangled fragments and the caller could not open it. + """ + result = _git(repo_root, *args, text=True) if result.returncode != 0: detail = str(result.stderr or "").strip() raise ValueError(f"git {' '.join(args)} failed: {detail}") - return [ - line.strip().replace("\\", "/") - for line in str(result.stdout or "").splitlines() - if line.strip() - ] + return [name for name in str(result.stdout or "").split("\0") if name] def _dedupe(values: list[str]) -> list[str]: @@ -174,12 +128,15 @@ def build_change_quality_scope( repo_root, "diff", "--name-only", + "-z", committed_range, "--", ) - staged = _name_only(repo_root, "diff", "--name-only", "--cached") - unstaged = _name_only(repo_root, "diff", "--name-only") - untracked = _name_only(repo_root, "ls-files", "--others", "--exclude-standard") + staged = _name_only(repo_root, "diff", "--name-only", "-z", "--cached") + unstaged = _name_only(repo_root, "diff", "--name-only", "-z") + untracked = _name_only( + repo_root, "ls-files", "--others", "--exclude-standard", "-z" + ) changed_files = _dedupe([*committed, *staged, *unstaged, *untracked]) digest = hashlib.sha256() diff --git a/tests/test_change_quality_scope_paths.py b/tests/test_change_quality_scope_paths.py new file mode 100644 index 0000000000..0bc2d5d78e --- /dev/null +++ b/tests/test_change_quality_scope_paths.py @@ -0,0 +1,81 @@ +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +import pytest + +from loopx.capabilities.change_quality.scope import build_change_quality_scope + + +def _repository(tmp_path: Path) -> Path: + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init", "-q", "-b", "main", str(repo)], check=True) + subprocess.run( + ["git", "-C", str(repo), "config", "user.email", "fixture@example.invalid"], + check=True, + ) + subprocess.run( + ["git", "-C", str(repo), "config", "user.name", "Fixture"], check=True + ) + (repo / "base.txt").write_text("base\n", encoding="utf-8") + subprocess.run(["git", "-C", str(repo), "add", "base.txt"], check=True) + subprocess.run(["git", "-C", str(repo), "commit", "-qm", "base"], check=True) + return repo + + +def _scope(repo: Path) -> dict: + return build_change_quality_scope(repo_path=repo, base_ref="HEAD") + + +@pytest.mark.parametrize("name", ["中文文件.txt", "café.txt", "ファイル.txt"]) +def test_untracked_names_outside_ascii_keep_their_exact_spelling( + tmp_path: Path, name: str +) -> None: + """Git quotes non-ASCII names by default; the scope must still open them.""" + + repo = _repository(tmp_path) + (repo / name).write_text("content\n", encoding="utf-8") + + first = _scope(repo) + assert first["changed_files"] == [name] + assert first["sources"]["untracked"] == [name] + + second = _scope(repo) + assert second["scope_fingerprint"] == first["scope_fingerprint"] + + +def test_untracked_names_keep_surrounding_spaces(tmp_path: Path) -> None: + repo = _repository(tmp_path) + name = " padded file .txt" + (repo / name).write_text("content\n", encoding="utf-8") + + scope = _scope(repo) + assert scope["changed_files"] == [name] + + +@pytest.mark.skipif(os.name != "posix", reason="a newline in a path is POSIX-only") +def test_untracked_names_with_a_newline_stay_one_entry(tmp_path: Path) -> None: + repo = _repository(tmp_path) + name = "line\nbreak.txt" + (repo / name).write_text("content\n", encoding="utf-8") + + scope = _scope(repo) + assert scope["changed_files"] == [name] + assert scope["changed_file_count"] == 1 + + +def test_changing_one_byte_of_an_untracked_file_changes_the_fingerprint( + tmp_path: Path, +) -> None: + repo = _repository(tmp_path) + name = "中文文件.txt" + (repo / name).write_text("content\n", encoding="utf-8") + before = _scope(repo)["scope_fingerprint"] + + (repo / name).write_text("contenu\n", encoding="utf-8") + after = _scope(repo)["scope_fingerprint"] + + assert after != before From 2157052a418d00c3d458bcdcc73aaf468714904b Mon Sep 17 00:00:00 2001 From: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> Date: Sun, 8 Nov 2026 14:25:07 +0000 Subject: [PATCH 2/2] fix(change-quality): keep exact paths and the bounded Git probe Signed-off-by: ChengLong GU <88359731+gcl-coder@users.noreply.github.com> --- loopx/capabilities/change_quality/result.py | 47 +++++++++----- loopx/capabilities/change_quality/scope.py | 70 ++++++++++++++++++--- tests/test_change_quality_scope_paths.py | 60 ++++++++++++++++++ 3 files changed, 150 insertions(+), 27 deletions(-) diff --git a/loopx/capabilities/change_quality/result.py b/loopx/capabilities/change_quality/result.py index 8ede4c277b..9f3f8b3f01 100644 --- a/loopx/capabilities/change_quality/result.py +++ b/loopx/capabilities/change_quality/result.py @@ -105,14 +105,28 @@ def _bounded_text(value: Any, *, field: str, limit: int) -> str: return text +def _exact_text(value: Any, *, field: str, limit: int) -> str: + """Keep the caller's value byte for byte; only reject what cannot be a path. + + Evidence names a file the scope just enumerated, so trimming or collapsing + whitespace here would turn ``" padded.py"`` into a different, unrecordable + name. + """ + + text = str(value or "") + validate_public_safe_text(field, text) + if len(text) > limit: + raise ValueError(f"{field} exceeds {limit} characters") + return text + + def _relative_path(value: Any, *, field: str) -> str | None: - text = str(value or "").strip().replace("\\", "/") + text = _exact_text(value, field=field, limit=1000) if not text: return None path = PurePosixPath(text) if path.is_absolute() or ".." in path.parts: raise ValueError(f"{field} must be a repo-relative path") - validate_public_safe_text(field, text) return path.as_posix() @@ -138,7 +152,7 @@ def _bounded_text_list( def _normalize_evidence_ref(value: Any, *, field: str) -> str: - text = _bounded_text(value, field=field, limit=400) + text = _exact_text(value, field=field, limit=400) kind, separator, target = text.partition(":") if not separator or kind not in EVIDENCE_REF_KINDS: raise ValueError( @@ -155,15 +169,13 @@ def _normalize_evidence_ref(value: Any, *, field: str) -> str: def _normalize_evidence_refs(value: Any, *, field: str) -> list[str]: - refs = [ - _normalize_evidence_ref(item, field=f"{field}[]") - for item in _bounded_text_list( - value, - field=field, - item_limit=400, - count_limit=20, - ) - ] + if value is None: + return [] + if not isinstance(value, list): + raise ValueError(f"{field} must be an array") + if len(value) > 20: + raise ValueError(f"{field} supports at most 20 items") + refs = [_normalize_evidence_ref(item, field=f"{field}[]") for item in value] if len(set(refs)) != len(refs): raise ValueError(f"{field} must not contain duplicates") return refs @@ -270,12 +282,13 @@ def _normalize_validation(value: Any, *, index: int) -> dict[str, Any]: if status in {"failed", "skipped"} and not reason: raise ValueError(f"{field} with status={status} requires reason") if "covers_paths" in value: + covers = value["covers_paths"] + if not isinstance(covers, list): + raise ValueError(f"{field}.covers_paths must be an array") + if len(covers) > 200: + raise ValueError(f"{field}.covers_paths supports at most 200 items") evidence["covers_paths"] = [ - _relative_path(path, field=f"{field}.covers_paths[]") - for path in _bounded_text_list( - value["covers_paths"], field=f"{field}.covers_paths", - item_limit=240, count_limit=200, - ) + _relative_path(item, field=f"{field}.covers_paths[]") for item in covers ] if "failure_attribution" in value: _validate_attribution_public_safety(value["failure_attribution"], field=field) diff --git a/loopx/capabilities/change_quality/scope.py b/loopx/capabilities/change_quality/scope.py index 593c898aa5..d526b52621 100644 --- a/loopx/capabilities/change_quality/scope.py +++ b/loopx/capabilities/change_quality/scope.py @@ -1,14 +1,43 @@ from __future__ import annotations import hashlib +import os import subprocess from pathlib import Path from typing import Any +from ...extensions.process_runtime import retire_owned_process CHANGE_QUALITY_SCOPE_SCHEMA_VERSION = "change_quality_scope_v0" +# The scope reads a checkout the operator may not control. A repository blocked +# on a stale index lock, a network mount or an oversized history must not hold +# the receipt open with no answer, so every read shares one budget. +GIT_PROBE_TIMEOUT_SECONDS = 10.0 + + +class GitProbeUnavailable(ValueError): + """Git did not answer inside the read budget, or could not start.""" + + +def _retire_owned_probe(process: subprocess.Popen[Any]) -> None: + """Stop the read's own session, then drop its pipes unread. + + Git runs helpers -- fsmonitor hooks, pagers -- inside the session it starts + in, so killing the leader alone leaves them behind. An inherited pipe also + outlives the leader, so the pipes are closed instead of drained: a drain + would wait on work this call no longer owns. + """ + + try: + retire_owned_process(process, process_tree_owned=True) + except (OSError, RuntimeError, ValueError, subprocess.SubprocessError): + # Cleanup is best effort. The read's own timeout or cancellation is the + # result the caller must receive, even when termination is unconfirmed. + pass + + def _git( repo_root: Path, *args: str, @@ -18,14 +47,35 @@ def _git( # text mode: a non-None encoding would silently flip bytes-mode callers # into text mode because CPython treats any of text/encoding/errors as # text-mode requests. - return subprocess.run( - ["git", "-C", str(repo_root), *args], - check=False, - text=text, - encoding="utf-8" if text else None, - errors="replace" if text else None, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + argv = ["git", "-C", str(repo_root), *args] + budget = GIT_PROBE_TIMEOUT_SECONDS + try: + process = subprocess.Popen( + argv, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=text, + encoding="utf-8" if text else None, + errors="replace" if text else None, + start_new_session=os.name == "posix", + ) + except OSError as exc: + raise GitProbeUnavailable(f"git {' '.join(args)} could not start: {exc}") from exc + try: + stdout, stderr = process.communicate(timeout=budget) + except subprocess.TimeoutExpired: + _retire_owned_probe(process) + raise GitProbeUnavailable( + f"git {' '.join(args)} did not answer within {budget:.1f}s" + ) from None + except BaseException: + # A terminal Ctrl-C reaches this process's group, not the session the + # read started, so the cancellation retires its helpers here and then + # keeps travelling to its own handler. + _retire_owned_probe(process) + raise + return subprocess.CompletedProcess( + args=argv, returncode=process.returncode, stdout=stdout, stderr=stderr ) @@ -53,8 +103,8 @@ def _name_only(repo_root: Path, *args: str) -> list[str]: ``-z`` is required at every call site: without it Git quotes non-ASCII names (``"\\346\\226\\226..."``) and separates records with newlines, so a - legal file name that contains a newline or spaces would arrive as - mangled fragments and the caller could not open it. + legal file name that contains a newline, leading or trailing spaces, or a + literal backslash would arrive as a fragment the caller could not open. """ result = _git(repo_root, *args, text=True) diff --git a/tests/test_change_quality_scope_paths.py b/tests/test_change_quality_scope_paths.py index 0bc2d5d78e..60e09ba028 100644 --- a/tests/test_change_quality_scope_paths.py +++ b/tests/test_change_quality_scope_paths.py @@ -6,6 +6,12 @@ import pytest +from loopx.capabilities.change_quality.result import ( + _validate_evidence_targets, + _normalize_evidence_ref, + _normalize_validation, + _relative_path, +) from loopx.capabilities.change_quality.scope import build_change_quality_scope @@ -79,3 +85,57 @@ def test_changing_one_byte_of_an_untracked_file_changes_the_fingerprint( after = _scope(repo)["scope_fingerprint"] assert after != before + + +@pytest.mark.parametrize( + "name", + [" padded.py ", "line\nbreak.txt", "back\\slash.py", "café.txt"], +) +def test_record_evidence_keeps_the_exact_scope_path(name: str) -> None: + """Evidence must name the file the scope reported, not a normalized copy.""" + + ref = _normalize_evidence_ref(f"path:{name}", field="evidence_refs[]") + assert ref == f"path:{name}" + assert _relative_path(name, field="risk.path") == name + + evidence = _normalize_validation( + { + "validator": "pytest", + "status": "passed", + "scope": "unit", + "covers_paths": [name], + }, + index=0, + ) + assert evidence["covers_paths"] == [name] + + +@pytest.mark.parametrize("target", ["/abs/file.txt", "../outside.py", "a/../../b"]) +def test_record_evidence_still_rejects_escapes(target: str) -> None: + with pytest.raises(ValueError, match="repo-relative"): + _normalize_evidence_ref(f"path:{target}", field="evidence_refs[]") + + +@pytest.mark.skipif(os.name != "posix", reason="special names are POSIX-only") +def test_scope_names_match_the_path_evidence_they_are_recorded_with( + tmp_path: Path, +) -> None: + """The names the scope reports must be the names evidence resolves against.""" + + repo = _repository(tmp_path) + names = [" padded.py ", "line\nbreak.txt", "back\\slash.py", "café.txt"] + for name in names: + (repo / name).write_text("content\n", encoding="utf-8") + + changed = set(_scope(repo)["changed_files"]) + assert changed == set(names) + + for name in names: + ref = _normalize_evidence_ref(f"path:{name}", field="evidence_refs[]") + _validate_evidence_targets( + [ref], + field="evidence_refs", + changed_files=changed, + instruction_refs=set(), + validator_ids=set(), + )