fix(noema): don't treat a deleted file's expected head-content 404 as a review-blocking error

scripts/ci/noema_review_gate.py

@@ -105,6 +105,7 @@ def graphql(query: str, **fields: str | int) -> dict[str, Any]:
       body
       isDraft
       headRefOid
+      baseRefOid
       reviewDecision
       reviewThreads(first: 100) {
         nodes {
@@ -380,10 +381,38 @@ def fetch_changed_file_paths(repo: str, number: int) -> list[str]:
     return [line.strip() for line in output.splitlines() if line.strip()]
 
 
-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_changed_files(repo: str, number: int) -> list[tuple[str, str]]:
+    """Fetch each changed file's path together with its PR status.
+
+    Unlike :func:`fetch_changed_file_paths`, this also returns GitHub's
+    per-file ``status`` (``added``, ``modified``, ``removed``, ``renamed``,
+    ...) from the same ``pulls/{number}/files`` response, so callers can tell
+    a file that no longer exists at the PR head apart from one that does.
+    """
+    output = run(
+        [
+            "gh",
+            "api",
+            f"repos/{repo}/pulls/{number}/files",
+            "--paginate",
+            "--jq",
+            r'.[] | .filename + "\t" + .status',
+        ]
+    )
+    files: list[tuple[str, str]] = []
+    for line in output.splitlines():
+        stripped = line.strip()
+        if not stripped:
+            continue
+        path, _, status = stripped.partition("\t")
+        files.append((path, status))
+    return files
+
+
+def fetch_head_file_content(repo: str, path: str, ref: str) -> str:
+    """Fetch a changed file's text content at ``ref`` through the GitHub API."""
     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",
@@ -399,15 +428,38 @@ 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:
+def removed_file_context_section(repo: str, path: str, base_sha: str) -> str:
+    """Build the context section for a file deleted by this PR.
+
+    A ``removed``-status file does not exist at the PR head by definition, so
+    fetching it there always 404s and carries no signal. Instead this fetches
+    the file's pre-deletion content at the PR base ref, which gives the
+    reviewer real evidence for judging whether the deletion is safe.
+    """
+    if not base_sha:
+        return f"### {path}\n[File removed in this PR — no head-side content applicable; base SHA unavailable for pre-deletion content.]"
+    try:
+        content = fetch_head_file_content(repo, path, base_sha)
+    except RuntimeError as exc:
+        reason = scrub_sensitive_data(str(exc)) or "unknown error"
+        return f"### {path}\n[File removed in this PR.] Unavailable from base content API: {reason}"
+    if not content:
+        return f"### {path}\n[File removed in this PR — no UTF-8 text content available from base content API.]"
+    return f"### {path}\n[File removed in this PR. Pre-deletion content at base ref:]\n{truncate_text(content, MAX_FILE_CONTEXT_CHARS)}"
+
+
+def changed_file_context(repo: str, number: int, head_sha: str, base_sha: str = "") -> str:
     """Build bounded changed-file context for cross-file review reasoning."""
     if not head_sha:
         return "Changed file context unavailable: missing PR head SHA."
-    paths = fetch_changed_file_paths(repo, number)
-    if not paths:
+    files = fetch_changed_files(repo, number)
+    if not files:
         return "Changed file context unavailable: PR reported no changed files."
     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, base_sha))
+            continue
         try:
             content = fetch_head_file_content(repo, path, head_sha)
         except RuntimeError as exc:
@@ -418,8 +470,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)
 
 
@@ -466,7 +518,9 @@ def build_review_context(repo: str, number: int, pr: dict[str, Any]) -> str:
     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 "")
+    )
     if files:
         sections.append("## Changed file context\n" + files)
     return truncate_text("\n\n".join(sections), MAX_REVIEW_CONTEXT_CHARS)

tests/test_noema_review_gate.py

@@ -168,10 +168,10 @@ def test_review_context_builders_include_codegraph_threads_and_files(monkeypatch
     assert "truncated 2 characters" in noema.truncate_text("abcdef", 4)
     assert "missing PR head SHA" in noema.changed_file_context("owner/repo", 7, "")
 
-    original_fetch_paths = noema.fetch_changed_file_paths
-    monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: [])
+    original_fetch_files = noema.fetch_changed_files
+    monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [])
     assert "no changed files" in noema.changed_file_context("owner/repo", 7, "head")
-    monkeypatch.setattr(noema, "fetch_changed_file_paths", original_fetch_paths)
+    monkeypatch.setattr(noema, "fetch_changed_files", original_fetch_files)
 
     encoded = base64.b64encode(b"print('hello')\n").decode("ascii")
     calls = []
@@ -180,7 +180,7 @@ def fake_run(args, stdin=None):
         calls.append(args)
         target = args[2]
         if target.endswith("/files"):
-            return "src/a.py\nREADME.md\nempty.txt\n"
+            return "src/a.py\tmodified\nREADME.md\tmodified\nempty.txt\tmodified\n"
         if "contents/src/a.py" in target:
             return encoded
         if "contents/README.md" in target:
@@ -235,14 +235,125 @@ def test_review_context_reports_omitted_files_and_missing_codegraph(monkeypatch,
     assert "CodeGraph context unavailable" in noema.load_codegraph_context()
 
     paths = [f"src/file_{index}.py" for index in range(noema.MAX_CONTEXT_FILES + 1)]
-    monkeypatch.setattr(noema, "fetch_changed_file_paths", lambda repo, number: paths)
-    monkeypatch.setattr(noema, "fetch_head_file_content", lambda repo, path, head_sha: "x")
+    monkeypatch.setattr(noema, "fetch_changed_files", lambda repo, number: [(p, "modified") for p in paths])
+    monkeypatch.setattr(noema, "fetch_head_file_content", lambda repo, path, ref: "x")
 
     context = noema.changed_file_context("owner/repo", 7, "head")
 
     assert "1 changed files omitted from context budget" in context
 
 
+def test_fetch_changed_file_paths_parses_plain_filenames(monkeypatch):
+    monkeypatch.setattr(noema, "run", lambda args, stdin=None: "a.py\n\nb.py\n")
+    assert noema.fetch_changed_file_paths("owner/repo", 7) == ["a.py", "b.py"]
+
+
+def test_changed_file_context_removed_file_base_content_empty(monkeypatch):
+    def fake_run(args, stdin=None):
+        target = args[2]
+        if target.endswith("/files"):
+            return "gone.py\tremoved\n"
+        if "contents/gone.py?ref=base-sha" in target:
+            return ""
+        raise AssertionError(args)
+
+    monkeypatch.setattr(noema, "run", fake_run)
+    context = noema.changed_file_context("owner/repo", 1486, "head-sha", "base-sha")
+
+    assert "no UTF-8 text content available from base content API" in context
+
+
+def test_fetch_changed_files_parses_path_and_status(monkeypatch):
+    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_changed_file_context_removed_file_uses_base_content_not_head_error(monkeypatch):
+    """A deleted file must not surface as a generic head-content-fetch error.
+
+    Regression test for the false-positive "Unable to review due to missing
+    file content" failure Noema produced on ContextualWisdomLab/.github#1486,
+    which deleted fuzz/fuzz_opencode_normalize_output.py.
+    """
+    encoded = base64.b64encode(b"def doomed():\n    pass\n").decode("ascii")
+
+    def fake_run(args, stdin=None):
+        target = args[2]
+        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 "Unavailable from head content API" not in context
+    assert "File removed in this PR" in context
+    assert "def doomed" in context
+
+
+def test_changed_file_context_removed_file_without_base_sha(monkeypatch):
+    monkeypatch.setattr(noema, "run", lambda args, stdin=None: "gone.py\tremoved\n")
+    context = noema.changed_file_context("owner/repo", 1486, "head-sha", "")
+
+    assert "Unavailable from head content API" not in context
+    assert "no head-side content applicable" in context
+
+
+def test_changed_file_context_removed_file_base_fetch_failure(monkeypatch):
+    def fake_run(args, stdin=None):
+        target = args[2]
+        if target.endswith("/files"):
+            return "gone.py\tremoved\n"
+        raise RuntimeError("Command failed: 404 Not Found (token secret)")
+
+    monkeypatch.setattr(noema, "run", fake_run)
+    context = noema.changed_file_context("owner/repo", 1486, "head-sha", "base-sha")
+
+    assert "Unavailable from head content API" not in context
+    assert "Unavailable from base content API" in context
+    assert "token secret" not in context
+
+
+def test_changed_file_context_non_removed_head_fetch_failure_is_unchanged(monkeypatch):
+    """A genuine head-content 404 on a file that still exists stays a real error."""
+
+    def fake_run(args, stdin=None):
+        target = args[2]
+        if target.endswith("/files"):
+            return "still-here.py\tmodified\n"
+        raise RuntimeError("Command failed: token secret")
+
+    monkeypatch.setattr(noema, "run", fake_run)
+    context = noema.changed_file_context("owner/repo", 1486, "head-sha", "base-sha")
+
+    assert "Unavailable from head content API" in context
+
+
+def test_build_review_context_forwards_base_sha_to_changed_file_context(monkeypatch):
+    captured = {}
+
+    def fake_changed_file_context(repo, number, head_sha, base_sha=""):
+        captured["args"] = (repo, number, head_sha, base_sha)
+        return "files"
+
+    monkeypatch.setattr(noema, "changed_file_context", fake_changed_file_context)
+    pr = make_pr(headRefOid="head-sha", baseRefOid="base-sha")
+
+    noema.build_review_context("owner/repo", 7, pr)
+
+    assert captured["args"] == ("owner/repo", 7, "head-sha", "base-sha")
+
+
 class FakeResponse:
     """Small context-manager response for urllib monkeypatches."""