diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 90a69bed31..96003b71b1 100644 --- a/scripts/ci/noema_review_gate.py +++ b/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) diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 338d46ba81..c984b83173 100644 --- a/tests/test_noema_review_gate.py +++ b/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."""