diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 5ab7e830f3..1ecae23e65 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -305,6 +305,7 @@ def graphql(query: str, **fields: str | int) -> dict[str, Any]: isDraft state headRefOid + changedFiles baseRefOid reviewDecision reviewThreads(first: 100) { @@ -471,27 +472,97 @@ def current_actor() -> str: return "" -def fetch_diff(repo: str, number: int) -> tuple[str, bool]: - """Fetch the PR diff and truncate it to the bounded LLM prompt size.""" - diff = run(["gh", "api", f"repos/{repo}/pulls/{number}", "-H", "Accept: application/vnd.github.v3.diff"]) - truncated = len(diff) > MAX_DIFF_CHARS - if truncated: - marker = "[overlong changed line content omitted]" - bounded = diff[: MAX_DIFF_CHARS - len(marker) - 2] - complete, separator, partial = bounded.rpartition("\n") - if not separator: - return diff[:MAX_DIFF_CHARS], truncated - last_hunk = max(complete.rfind("\n@@"), 0 if complete.startswith("@@") else -1) - last_file = max(complete.rfind("\ndiff --git "), 0 if complete.startswith("diff --git ") else -1) - inside_hunk = last_hunk > last_file - if partial.startswith(("+", "-")) and ( - inside_hunk or not partial.startswith(("+++", "---")) - ): - complete += f"\n{partial[0]}{marker}" - diff = complete +def fetch_diff( + repo: str, number: int, expected_files: int | None = None +) -> tuple[str, bool]: + """Build the PR diff from the paginated Files API and bound it for the LLM prompt. + + The ``.diff`` media type on ``pulls/{n}`` refuses pull requests with more + than 300 changed files (HTTP 406), so the unified diff is reconstructed from + ``pulls/{n}/files`` pages. ``expected_files`` (GraphQL ``changedFiles``) + fails closed on an incomplete listing; a file whose ``patch`` GitHub omits + marks the diff as truncated instead of posing as complete evidence. + """ + try: + pages = json.loads( + run( + [ + "gh", + "api", + f"repos/{repo}/pulls/{number}/files?per_page=100", + "--paginate", + "--slurp", + ] + ) + ) + except json.JSONDecodeError as exc: + raise RuntimeError("GitHub PR files response was not valid JSON") from exc + if not isinstance(pages, list) or any(not isinstance(page, list) for page in pages): + raise RuntimeError("GitHub PR files response had an unexpected shape") + files = [file for page in pages for file in page] + if any(not isinstance(file, dict) for file in files): + raise RuntimeError("GitHub PR files response had an unexpected file record") + if expected_files is not None and len(files) != expected_files: + raise RuntimeError( + f"GitHub returned {len(files)} of {expected_files} changed PR files" + ) + + sections: list[str] = [] + incomplete_patch = False + for file in files: + filename = str(file.get("filename") or "") + if not filename: + raise RuntimeError("GitHub PR file record omitted its filename") + old_filename = str(file.get("previous_filename") or filename) + status = str(file.get("status") or "modified") + old_label = "/dev/null" if status == "added" else _format_diff_path(old_filename, "a/") + new_label = "/dev/null" if status == "removed" else _format_diff_path(filename, "b/") + patch = file.get("patch") + if not isinstance(patch, str): + patch = "[patch unavailable from GitHub PR files API]" + incomplete_patch = True + sections.append( + f"diff --git {_format_diff_path(old_filename, 'a/')} " + f"{_format_diff_path(filename, 'b/')}\n" + f"--- {old_label}\n+++ {new_label}\n{patch}" + ) + diff = "\n".join(sections) + truncated = incomplete_patch or len(diff) > MAX_DIFF_CHARS + if len(diff) > MAX_DIFF_CHARS: + diff = _bound_diff(diff) return diff, truncated +def _bound_diff(diff: str) -> str: + """Cut an over-long diff at a line boundary, marking a severed changed line. + + The cut never lands mid-line: the last complete line is kept and, when the + severed remainder was a changed line inside a hunk, a ``+``/``-`` marker + line replaces it so ``changed_diff_locations`` still sees the change. + """ + marker = "[overlong changed line content omitted]" + bounded = diff[: MAX_DIFF_CHARS - len(marker) - 2] + complete, separator, partial = bounded.rpartition("\n") + if not separator: + return diff[:MAX_DIFF_CHARS] + last_hunk = max(complete.rfind("\n@@"), 0 if complete.startswith("@@") else -1) + last_file = max(complete.rfind("\ndiff --git "), 0 if complete.startswith("diff --git ") else -1) + inside_hunk = last_hunk > last_file + if partial.startswith(("+", "-")) and ( + inside_hunk or not partial.startswith(("+++", "---")) + ): + complete += f"\n{partial[0]}{marker}" + return complete + + +def _format_diff_path(path: str, prefix: str) -> str: + """Return a plain or JSON-quoted diff path that round-trips safely.""" + value = f"{prefix}{path}" + if any(character in '\t\n\r"\\' or not 0x20 <= ord(character) < 0x7F for character in value): + return json.dumps(value, ensure_ascii=True) + return value + + def changed_diff_locations(diff: str) -> set[tuple[str, int, str]]: """Return exact LEFT/RIGHT changed-line locations from a unified diff.""" locations: set[tuple[str, int, str]] = set() @@ -541,9 +612,14 @@ def parse_diff_path(raw: str, prefix: str) -> str: return "" if value.startswith('"'): try: - decoded = ast.literal_eval(value) - value = decoded.encode("latin-1").decode("utf-8") - except (SyntaxError, ValueError, UnicodeError): + value = json.loads(value) + except json.JSONDecodeError: + try: + decoded = ast.literal_eval(value) + value = decoded.encode("latin-1").decode("utf-8") + except (SyntaxError, ValueError, UnicodeError): + return "" + if not isinstance(value, str): return "" return value.removeprefix(prefix) @@ -1784,7 +1860,7 @@ def inspect_and_review(repo: str, number: int, expected_head: str) -> int: if existing_noema_review(pr, actor): print("Current head already has a Noema review; nothing to do.") return 0 - diff, truncated = fetch_diff(repo, number) + diff, truncated = fetch_diff(repo, number, pr.get("changedFiles")) 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) diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 5fa23dec53..d811a346b6 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -860,26 +860,41 @@ def app_identity(args, **kwargs): monkeypatch.setattr(noema, "run", app_identity) assert noema.current_actor() == "cwl-noema-review[bot]" - source = "complete\n" + "x" * (noema.MAX_DIFF_CHARS + 5) - monkeypatch.setattr(noema, "run", lambda *args, **kwargs: source) - diff, truncated = noema.fetch_diff("owner/repo", 1) - assert truncated - assert diff == "complete" + pages = [ + [ + { + "filename": "src/added.py", + "status": "added", + "patch": "@@ -0,0 +1 @@\n+added", + } + ], + [ + { + "filename": "src/current.py", + "previous_filename": "src/old.py", + "status": "renamed", + "patch": "@@ -0,0 +1 @@\n+" + "x" * noema.MAX_DIFF_CHARS, + } + ], + ] - source = "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -0,0 +1 @@\n+" + "x" * noema.MAX_DIFF_CHARS - monkeypatch.setattr(noema, "run", lambda *args, **kwargs: source) - diff, truncated = noema.fetch_diff("owner/repo", 1) + def paginated_files(args, **kwargs): + assert "repos/owner/repo/pulls/1/files?per_page=100" in args + assert "--paginate" in args + assert "--slurp" in args + return json.dumps(pages) + + monkeypatch.setattr(noema, "run", paginated_files) + diff, truncated = noema.fetch_diff("owner/repo", 1, expected_files=2) assert truncated assert diff.endswith("+[overlong changed line content omitted]") - assert ("a.py", 1, "RIGHT") in noema.changed_diff_locations(diff) assert len(diff) <= noema.MAX_DIFF_CHARS + assert ("src/current.py", 1, "RIGHT") in noema.changed_diff_locations(diff) + assert "--- /dev/null\n+++ b/src/added.py" in diff + assert "diff --git a/src/old.py b/src/current.py" in diff - source = "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -0,0 +1 @@\n+++" + "x" * noema.MAX_DIFF_CHARS - monkeypatch.setattr(noema, "run", lambda *args, **kwargs: source) - diff, truncated = noema.fetch_diff("owner/repo", 1) - assert truncated - assert diff.endswith("+[overlong changed line content omitted]") - assert ("a.py", 1, "RIGHT") in noema.changed_diff_locations(diff) + with pytest.raises(RuntimeError, match="returned 2 of 3"): + noema.fetch_diff("owner/repo", 1, expected_files=3) assert noema.extract_json_object('{"decision":"approve"}') == {"decision": "approve"} assert noema.extract_json_object('prefix {"decision":"comment"} suffix') == {"decision": "comment"} @@ -1281,6 +1296,85 @@ def read(self): noema.call_llm("owner/repo", 7, make_pr(), "diff", False, "head") +def test_bound_diff_keeps_line_boundaries_and_marks_severed_changed_lines(): + """The over-long cut lands on a line boundary and keeps a severed change visible.""" + source = "complete\n" + "x" * (noema.MAX_DIFF_CHARS + 5) + assert len(source) > noema.MAX_DIFF_CHARS + assert noema._bound_diff(source) == "complete" + + source = "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -0,0 +1 @@\n+" + "x" * noema.MAX_DIFF_CHARS + diff = noema._bound_diff(source) + assert diff.endswith("+[overlong changed line content omitted]") + assert ("a.py", 1, "RIGHT") in noema.changed_diff_locations(diff) + assert len(diff) <= noema.MAX_DIFF_CHARS + + source = "diff --git a/a.py b/a.py\n--- a/a.py\n+++ b/a.py\n@@ -0,0 +1 @@\n+++" + "x" * noema.MAX_DIFF_CHARS + diff = noema._bound_diff(source) + assert diff.endswith("+[overlong changed line content omitted]") + assert ("a.py", 1, "RIGHT") in noema.changed_diff_locations(diff) + + assert noema._bound_diff("x" * (noema.MAX_DIFF_CHARS + 5)) == "x" * noema.MAX_DIFF_CHARS + + +@pytest.mark.parametrize( + ("response", "message"), + [ + ("not-json", "not valid JSON"), + ("{}", "unexpected shape"), + ("[{}]", "unexpected shape"), + ("[[null]]", "unexpected file record"), + ("[[{}]]", "omitted its filename"), + ], +) +def test_fetch_diff_rejects_incomplete_paginated_responses( + monkeypatch, response, message +): + """Malformed or incomplete GitHub file pages fail closed.""" + monkeypatch.setattr(noema, "run", lambda _args: response) + with pytest.raises(RuntimeError, match=message): + noema.fetch_diff("owner/repo", 1) + + +def test_fetch_diff_round_trips_special_current_and_previous_filenames(monkeypatch): + """Synthesized diff headers preserve every Git-special filename byte.""" + previous = 'src/old\t"quoted"\\name\n.py' + current = "src/새 이름\tcurrent.py" + pages = [[{ + "filename": current, + "previous_filename": previous, + "status": "renamed", + "patch": "@@ -1 +1 @@\n-old\n+new", + }]] + monkeypatch.setattr(noema, "run", lambda *args, **kwargs: json.dumps(pages)) + + diff, truncated = noema.fetch_diff("owner/repo", 1, expected_files=1) + + assert not truncated + assert noema.changed_diff_locations(diff) == { + (previous, 1, "LEFT"), + (current, 1, "RIGHT"), + } + + +def test_parse_diff_path_rejects_non_string_json(monkeypatch): + """A malformed decoder result cannot become a trusted changed-file path.""" + monkeypatch.setattr(noema.json, "loads", lambda _value: None) + + assert noema.parse_diff_path('"a/file.py"', "a/") == "" + + +def test_fetch_diff_marks_unavailable_patch_as_truncated(monkeypatch): + """A file without GitHub patch text remains visible but incomplete.""" + response = json.dumps([[{"filename": "removed.bin", "status": "removed"}]]) + monkeypatch.setattr(noema, "run", lambda _args: response) + + diff, truncated = noema.fetch_diff("owner/repo", 1, expected_files=1) + + assert truncated + assert "+++ /dev/null" in diff + assert "[patch unavailable from GitHub PR files API]" in diff + + @pytest.mark.parametrize( ("actor", "installation_id", "source"), [ @@ -1824,7 +1918,7 @@ def test_inspect_and_review_skip_paths(monkeypatch): calls = [] monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: clean_pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") - monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number, expected_files=None: ("diff", False)) monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok", "findings": []}) @@ -1919,7 +2013,7 @@ def test_inspect_and_review_does_not_wait_for_other_reviews_or_checks(monkeypatc calls = [] monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") - monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number, expected_files=None: ("diff", False)) monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok"}) @@ -1958,7 +2052,7 @@ def test_head_movement_stops_before_review_publication(monkeypatch): ) monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") - monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number, expected_files=None: ("diff", False)) monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr( @@ -1981,7 +2075,7 @@ def test_closed_during_model_stops_before_review_publication(monkeypatch): ) monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") - monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number, expected_files=None: ("diff", False)) monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve"}) @@ -2000,7 +2094,7 @@ def test_uppercase_expected_head_is_not_stale_before_model_work(monkeypatch): pr = make_pr(headRefOid=head) monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: pr) monkeypatch.setattr(noema, "current_actor", lambda: "noema") - monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number, expected_files=None: ("diff", False)) monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) monkeypatch.setattr(noema, "build_review_context", lambda repo, number, value, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve", "summary": "ok"}) @@ -2017,7 +2111,7 @@ def test_uppercase_expected_head_is_not_stale_before_publication(monkeypatch): pull_requests = iter((make_pr(headRefOid=head), make_pr(headRefOid=head))) monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(pull_requests)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") - monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number, expected_files=None: ("diff", False)) monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr( @@ -2039,7 +2133,7 @@ def test_inspect_and_review_rechecks_head_before_publication(monkeypatch): submitted = [] monkeypatch.setattr(noema, "fetch_pr", lambda repo, number: next(responses)) monkeypatch.setattr(noema, "current_actor", lambda: "noema") - monkeypatch.setattr(noema, "fetch_diff", lambda repo, number: ("diff", False)) + monkeypatch.setattr(noema, "fetch_diff", lambda repo, number, expected_files=None: ("diff", False)) monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [("tool.py", "modified")]) monkeypatch.setattr(noema, "build_review_context", lambda repo, number, pr, changed_files=None: "context") monkeypatch.setattr(noema, "call_llm", lambda *args, **kwargs: {"decision": "approve"}) diff --git a/tests/test_repository_branch_coverage_javascript_and_noema.py b/tests/test_repository_branch_coverage_javascript_and_noema.py index caeca87236..2e62f26c37 100644 --- a/tests/test_repository_branch_coverage_javascript_and_noema.py +++ b/tests/test_repository_branch_coverage_javascript_and_noema.py @@ -142,7 +142,10 @@ def test_noema_fetch_diff_truncates_to_prompt_budget( ) -> None: """Oversized diffs are bounded and explicitly marked truncated.""" - monkeypatch.setattr(noema, "run", lambda _args: "x" * (noema.MAX_DIFF_CHARS + 1)) + response = json.dumps( + [[{"filename": "large.py", "patch": "x" * (noema.MAX_DIFF_CHARS + 1)}]] + ) + monkeypatch.setattr(noema, "run", lambda _args: response) diff, truncated = noema.fetch_diff("owner/repo", 1) assert truncated is True assert len(diff) == noema.MAX_DIFF_CHARS diff --git a/tests/test_repository_branch_coverage_reporting_edges.py b/tests/test_repository_branch_coverage_reporting_edges.py index f5dbf1dae0..fa663b6548 100644 --- a/tests/test_repository_branch_coverage_reporting_edges.py +++ b/tests/test_repository_branch_coverage_reporting_edges.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json from pathlib import Path import pytest @@ -107,8 +108,11 @@ def test_noema_small_diff_and_empty_context_branches( ) -> None: """Small diffs, invalid thread lines, and empty sections stay clean.""" - monkeypatch.setattr(noema, "run", lambda _args: "small diff") - assert noema.fetch_diff("owner/repo", 1) == ("small diff", False) + response = json.dumps([[{"filename": "small.py", "patch": "small diff"}]]) + monkeypatch.setattr(noema, "run", lambda _args: response) + diff, truncated = noema.fetch_diff("owner/repo", 1) + assert diff.endswith("small diff") + assert truncated is False pr = { "headRefOid": "a" * 40,