diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index b77ed11c03..ef270872a2 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -108,6 +108,7 @@ def graphql(query: str, **fields: str | int) -> dict[str, Any]: isDraft state headRefOid + baseRefOid reviewDecision reviewThreads(first: 100) { nodes { @@ -395,8 +396,14 @@ def truncate_text(text: str, limit: int) -> str: return f"{text[:limit]}\n[truncated {omitted} characters]" -def fetch_changed_file_paths(repo: str, number: int) -> list[str]: - """Fetch changed file paths for the pull request.""" +def fetch_changed_files(repo: str, number: int) -> list[tuple[str, str]]: + """Fetch changed paths and statuses without corrupting whitespace in paths. + + The Files API is projected to one JSON-encoded two-element array per file. + JSON escaping preserves tabs, newlines, and edge spaces inside ``filename`` + while keeping pagination output line-delimited and parseable. Malformed + records fail closed instead of being reinterpreted as another path/status. + """ output = run( [ "gh", @@ -404,16 +411,34 @@ def fetch_changed_file_paths(repo: str, number: int) -> list[str]: f"repos/{repo}/pulls/{number}/files", "--paginate", "--jq", - ".[].filename", + r'.[] | [.filename, .status] | @json', ] ) - return [line.strip() for line in output.splitlines() if line.strip()] + files: list[tuple[str, str]] = [] + for line in output.splitlines(): + if not line: + continue + try: + record = json.loads(line) + except json.JSONDecodeError as exc: + raise RuntimeError("GitHub changed-file response was malformed") from exc + if ( + not isinstance(record, list) + or len(record) != 2 + or type(record[0]) is not str + or not record[0] + or type(record[1]) is not str + or not record[1] + ): + raise RuntimeError("GitHub changed-file response was malformed") + files.append((record[0], record[1])) + return files -def fetch_head_file_content(repo: str, path: str, head_sha: str) -> str: - """Fetch a changed file's current-head text content through the GitHub API.""" +def fetch_file_content_at_ref(repo: str, path: str, ref: str) -> str: + """Fetch one repository text file at an exact Git ref through GitHub.""" encoded_path = urllib.parse.quote(path, safe="/") - encoded_ref = urllib.parse.quote(head_sha, safe="") + encoded_ref = urllib.parse.quote(ref, safe="") content = run( [ "gh", @@ -429,17 +454,102 @@ def fetch_head_file_content(repo: str, path: str, head_sha: str) -> str: return base64.b64decode(compact).decode("utf-8", errors="replace") -def changed_file_context(repo: str, number: int, head_sha: str) -> str: - """Build bounded changed-file context for cross-file review reasoning.""" +def fetch_merge_base_sha(repo: str, base_sha: str, head_sha: str) -> str: + """Return the immutable merge-base SHA for the current base/head pair.""" + if not re.fullmatch(r"[0-9a-fA-F]{40}", base_sha): + raise RuntimeError("PR base SHA was unavailable or malformed") + if not re.fullmatch(r"[0-9a-fA-F]{40}", head_sha): + raise RuntimeError("PR head SHA was unavailable or malformed") + merge_base = run( + [ + "gh", + "api", + f"repos/{repo}/compare/{base_sha}...{head_sha}", + "--jq", + ".merge_base_commit.sha // empty", + ] + ).strip() + if not re.fullmatch(r"[0-9a-fA-F]{40}", merge_base): + raise RuntimeError("GitHub compare response did not contain a valid merge-base SHA") + return merge_base.lower() + + +def removed_file_context_section( + repo: str, + path: str, + merge_base_sha: str, + merge_base_error: str = "", +) -> str: + """Build review context for a file deleted relative to the merge base. + + A deleted path does not exist at the PR head. Its relevant pre-deletion + evidence is therefore the immutable merge base shared by the current base + and reviewed head, not the moving tip of the base branch. When merge-base + discovery or content retrieval is unavailable, the context records that + bounded evidence failure explicitly rather than inventing head content. + """ + if merge_base_error: + return ( + f"### {path}\n[File removed in this PR.] " + f"Merge-base lookup unavailable: {merge_base_error}" + ) + if not merge_base_sha: + return ( + f"### {path}\n[File removed in this PR — no head-side content applicable; " + "merge-base SHA unavailable for pre-deletion content.]" + ) + try: + content = fetch_file_content_at_ref(repo, path, merge_base_sha) + except RuntimeError as exc: + reason = scrub_sensitive_data(str(exc)) or "unknown error" + return ( + f"### {path}\n[File removed in this PR.] " + f"Unavailable from merge-base content API: {reason}" + ) + if not content: + return ( + f"### {path}\n[File removed in this PR — no UTF-8 text content " + "available from merge-base content API.]" + ) + return ( + f"### {path}\n[File removed in this PR. Pre-deletion content at merge base " + f"`{merge_base_sha}`:]\n{truncate_text(content, MAX_FILE_CONTEXT_CHARS)}" + ) + + +def changed_file_context( + repo: str, + number: int, + head_sha: str, + base_sha: str = "", + changed_files: Sequence[tuple[str, str]] | None = None, +) -> str: + """Build bounded changed-file context from one status-preserving snapshot.""" if not head_sha: return "Changed file context unavailable: missing PR head SHA." - paths = fetch_changed_file_paths(repo, number) - if not paths: + files = list(changed_files) if changed_files is not None else fetch_changed_files(repo, number) + if not files: return "Changed file context unavailable: PR reported no changed files." + + merge_base_sha = "" + merge_base_error = "" + if any(status == "removed" for _path, status in files[:MAX_CONTEXT_FILES]): + try: + merge_base_sha = fetch_merge_base_sha(repo, base_sha, head_sha) + except RuntimeError as exc: + merge_base_error = scrub_sensitive_data(str(exc)) or "unknown error" + sections: list[str] = [] - for path in paths[:MAX_CONTEXT_FILES]: + for path, status in files[:MAX_CONTEXT_FILES]: + if status == "removed": + sections.append( + removed_file_context_section( + repo, path, merge_base_sha, merge_base_error + ) + ) + continue try: - content = fetch_head_file_content(repo, path, head_sha) + content = fetch_file_content_at_ref(repo, path, head_sha) except RuntimeError as exc: reason = scrub_sensitive_data(str(exc)) or "unknown error" sections.append(f"### {path}\nUnavailable from head content API: {reason}") @@ -448,8 +558,8 @@ def changed_file_context(repo: str, number: int, head_sha: str) -> str: sections.append(f"### {path}\nNo UTF-8 text content available from head content API.") continue sections.append(f"### {path}\n{truncate_text(content, MAX_FILE_CONTEXT_CHARS)}") - if len(paths) > MAX_CONTEXT_FILES: - sections.append(f"[{len(paths) - MAX_CONTEXT_FILES} changed files omitted from context budget]") + if len(files) > MAX_CONTEXT_FILES: + sections.append(f"[{len(files) - MAX_CONTEXT_FILES} changed files omitted from context budget]") return "\n\n".join(sections) @@ -475,28 +585,24 @@ def review_thread_context(pr: dict[str, Any]) -> str: return "\n".join(lines) -def load_codegraph_context() -> str: - """Load optional precomputed CodeGraph context for structural review evidence.""" - path = os.environ.get("NOEMA_CODEGRAPH_CONTEXT_PATH", "").strip() - if not path: - return "" - try: - with open(path, encoding="utf-8") as handle: - return truncate_text(handle.read(), MAX_REVIEW_CONTEXT_CHARS) - except OSError as exc: - return f"CodeGraph context unavailable: {exc}" - - -def build_review_context(repo: str, number: int, pr: dict[str, Any]) -> str: - """Build bounded non-diff context for the Noema reviewer.""" +def build_review_context( + repo: str, + number: int, + pr: dict[str, Any], + changed_files: Sequence[tuple[str, str]] | None = None, +) -> str: + """Build bounded non-diff context from review threads and changed files.""" sections: list[str] = [] - codegraph = load_codegraph_context() - if codegraph: - sections.append("## CodeGraph context\n" + codegraph) threads = review_thread_context(pr) if threads: sections.append("## Prior review threads\n" + threads) - files = changed_file_context(repo, number, str(pr.get("headRefOid") or "")) + files = changed_file_context( + repo, + number, + str(pr.get("headRefOid") or ""), + str(pr.get("baseRefOid") or ""), + changed_files, + ) if files: sections.append("## Changed file context\n" + files) return truncate_text("\n\n".join(sections), MAX_REVIEW_CONTEXT_CHARS) @@ -967,7 +1073,7 @@ def call_llm( "content": "\n".join( [ "You are Noema, an independent pull request reviewer for ContextualWisdomLab.", - "Review the PR diff plus the additional changed-file, review-thread, and CodeGraph context for correctness, security, maintainability, and behavioral regressions.", + "Review the PR diff plus the additional changed-file and review-thread context for correctness, security, maintainability, and behavioral regressions.", "Return only JSON with this shape:", json.dumps( { @@ -1205,8 +1311,9 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: print("Current head already has a Noema review; nothing to do.") return 0 diff, truncated = fetch_diff(repo, number) - changed_paths = fetch_changed_file_paths(repo, number) - review_context = build_review_context(repo, number, pr) + changed_files = fetch_changed_files(repo, number) + changed_paths = tuple(path for path, _status in changed_files) + review_context = build_review_context(repo, number, pr, changed_files) try: verdict = call_llm(repo, number, pr, diff, truncated, expected_head, review_context, changed_paths) except StaleHeadDuringRepairRetryError: diff --git a/tests/test_noema_removed_file_context.py b/tests/test_noema_removed_file_context.py new file mode 100644 index 0000000000..8c5d8ca539 --- /dev/null +++ b/tests/test_noema_removed_file_context.py @@ -0,0 +1,108 @@ +"""Regression tests for Noema deleted-file review context.""" + +from __future__ import annotations + +import base64 + +from scripts.ci import noema_review_gate as noema + + +def test_fetch_changed_files_preserves_path_and_status(monkeypatch): + """The paginated Files API adapter must retain each file status.""" + monkeypatch.setattr( + noema, + "run", + lambda args, stdin=None: "a.py\tmodified\n\nb.py\tremoved\nfuzz/x.py\tadded\n", + ) + + assert noema.fetch_changed_files("owner/repo", 7) == [ + ("a.py", "modified"), + ("b.py", "removed"), + ("fuzz/x.py", "added"), + ] + + +def test_removed_file_context_uses_base_content(monkeypatch): + """A deleted file must be reviewed from immutable pre-deletion evidence.""" + encoded = base64.b64encode(b"def doomed():\n pass\n").decode("ascii") + calls: list[str] = [] + + def fake_run(args, stdin=None): + target = args[2] + calls.append(target) + if target.endswith("/files"): + return "fuzz/fuzz_opencode_normalize_output.py\tremoved\n" + if "contents/fuzz/fuzz_opencode_normalize_output.py?ref=base-sha" in target: + return encoded + raise AssertionError(args) + + monkeypatch.setattr(noema, "run", fake_run) + + context = noema.changed_file_context( + "owner/repo", 1486, "head-sha", "base-sha" + ) + + assert "File removed in this PR. Pre-deletion content at base ref" in context + assert "def doomed" in context + assert not any("ref=head-sha" in target for target in calls) + + +def test_removed_file_context_fails_closed_without_base_sha(monkeypatch): + """Missing base identity must be explicit and must not trigger a head fetch.""" + monkeypatch.setattr( + noema, + "fetch_changed_files", + lambda repo, number: [("gone.py", "removed")], + ) + monkeypatch.setattr( + noema, + "fetch_head_file_content", + lambda *args, **kwargs: (_ for _ in ()).throw(AssertionError("unexpected fetch")), + ) + + context = noema.changed_file_context("owner/repo", 7, "head-sha", "") + + assert "base SHA unavailable" in context + + +def test_removed_file_base_fetch_failure_is_distinct_from_head_failure(monkeypatch): + """A base-side API failure must remain typed as base evidence failure.""" + monkeypatch.setattr( + noema, + "fetch_changed_files", + lambda repo, number: [("gone.py", "removed")], + ) + + def fail_fetch(repo, path, ref): + raise RuntimeError("HTTP 502: token ***") + + monkeypatch.setattr(noema, "fetch_head_file_content", fail_fetch) + + context = noema.changed_file_context( + "owner/repo", 7, "head-sha", "base-sha" + ) + + assert "Unavailable from base content API" in context + assert "Unavailable from head content API" not in context + + +def test_build_review_context_passes_live_base_ref(monkeypatch): + """The GraphQL base identity must reach changed-file context construction.""" + observed: list[tuple[str, int, str, str]] = [] + monkeypatch.setattr(noema, "review_thread_context", lambda pr: "") + monkeypatch.setattr(noema, "load_codegraph_context", lambda: "") + + def fake_context(repo, number, head_sha, base_sha=""): + observed.append((repo, number, head_sha, base_sha)) + return "files" + + monkeypatch.setattr(noema, "changed_file_context", fake_context) + + result = noema.build_review_context( + "owner/repo", + 7, + {"headRefOid": "head-sha", "baseRefOid": "base-sha"}, + ) + + assert observed == [("owner/repo", 7, "head-sha", "base-sha")] + assert "## Changed file context\nfiles" in result diff --git a/tests/test_repository_branch_coverage_javascript_and_noema.py b/tests/test_repository_branch_coverage_javascript_and_noema.py index 99793a4dfa..caeca87236 100644 --- a/tests/test_repository_branch_coverage_javascript_and_noema.py +++ b/tests/test_repository_branch_coverage_javascript_and_noema.py @@ -176,9 +176,7 @@ def test_noema_review_context_includes_locations_bodies_and_all_sections( assert "src/runtime.py:7" in rendered assert "reviewer: Fix this" in rendered - monkeypatch.setattr(noema, "load_codegraph_context", lambda: "graph") monkeypatch.setattr(noema, "changed_file_context", lambda *_args: "files") context = noema.build_review_context("owner/repo", 1, pr) - assert "CodeGraph context" in context assert "Prior review threads" in context assert "Changed file context" in context diff --git a/tests/test_repository_branch_coverage_reporting_edges.py b/tests/test_repository_branch_coverage_reporting_edges.py index b4527147c8..f5dbf1dae0 100644 --- a/tests/test_repository_branch_coverage_reporting_edges.py +++ b/tests/test_repository_branch_coverage_reporting_edges.py @@ -129,7 +129,6 @@ def test_noema_small_diff_and_empty_context_branches( rendered_context = noema.review_thread_context(pr) assert rendered_context == "- Thread open at src/runtime.py:\n - reviewer: note" - monkeypatch.setattr(noema, "load_codegraph_context", lambda: "") monkeypatch.setattr(noema, "review_thread_context", lambda _pr: "") monkeypatch.setattr(noema, "changed_file_context", lambda *_args: "") assert noema.build_review_context("owner/repo", 1, pr) == ""