fix(noema): paginate large pull request diffs

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)

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"})

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

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,