From e96583bcb3540ecae62ff54c8b994f7bea2606d2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 16:12:12 +0000 Subject: [PATCH 01/10] fix(noema): stop treating a deleted file's expected head-content 404 as a review-blocking error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Noema's required review check on ContextualWisdomLab/.github#1486 (which deletes fuzz/fuzz_opencode_normalize_output.py) refused to complete a real verdict, citing "File content unavailable due to HTTP 404 error" as a high-severity finding and returning only COMMENT. Root cause: fetch_changed_file_paths() discarded each file's PR `status` field, so changed_file_context() fetched every changed file's content at the PR's own head SHA — including files whose status is "removed", which by definition cannot exist at head and always 404. changed_file_context() then reported that expected 404 with the same "Unavailable ... error" phrasing used for genuine anomalies, and the LLM reviewer reasonably read it as a real data-integrity problem serious enough to block a full review. Fix: add fetch_changed_files() to return each file's (path, status) from the same `gh api pulls/{n}/files` response, and special-case status "removed" in changed_file_context(): instead of probing head_sha (guaranteed 404), fetch the file's pre-deletion content at the PR's base ref (now carried via PR_QUERY's baseRefOid) so the reviewer sees what's being deleted. A genuine head-content fetch failure on a still-existing file is unchanged. fetch_changed_file_paths() itself is untouched and still serves its other caller (inspect_and_review's changed_paths for validate_substantive_verdict). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw --- scripts/ci/noema_review_gate.py | 74 ++++++++++++++++--- tests/test_noema_review_gate.py | 123 ++++++++++++++++++++++++++++++-- 2 files changed, 181 insertions(+), 16 deletions(-) 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.""" From db106d50f2134ece147bc5318e389aeb124d198c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:21:06 +0000 Subject: [PATCH 02/10] test(ci): close main's post-#1546 scheduler coverage regression Protected main regressed to 99% scripts/ci coverage after #1546 added live_head_matches, a no-active/no-stale fall-through in prepare_autofix_slot, and an "already queued or running" wait branch to pr_review_fix_scheduler.py without covering them, while the pre-existing inspect_pr conflicted-draft/conflicted-unauthorized returns and pr_review_merge_scheduler.py's fetch_workflow_names_by_check_suite_rest pagination/filtering/ permission-denied paths stayed untested. Every PR rebasing onto main inherits this via the coverage-evidence required check regardless of its own diff. Test-only change; no production code touched. --- CHANGELOG.md | 9 +++ tests/test_pr_review_fix_scheduler.py | 49 ++++++++++++++ ...ew_fix_scheduler_rest_workflow_identity.py | 67 +++++++++++++++++++ 3 files changed, 125 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5810d5308..1c46c64657 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Close a 99% `scripts/ci` coverage regression on protected main: merged #1546 added an + uncovered `live_head_matches` helper, an uncovered no-active/no-stale-runs fall-through in + `prepare_autofix_slot`, and an uncovered "current-head autofix run is already queued or + running" wait path in `pr_review_fix_scheduler.py::inspect_pr`, while the pre-existing + conflicted-draft and conflicted-unauthorized `inspect_pr` returns and the REST + `fetch_workflow_names_by_check_suite_rest` pagination/name-filtering/permission-denied paths + in `pr_review_merge_scheduler.py` remained untested. Every PR rebasing onto main inherited + this failure via the `coverage-evidence` required check regardless of its own diff; this adds + test-only coverage for all of the above with no production code change. - Avoid redundant merge-scheduler wakes when the trusted receipt predicate already finds a substantive exact-head OpenCode verdict. Missing, stale, or fallback-only evidence still dispatches review work, while receipt lookup or diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index 9860eeaec7..f6abd64b0f 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -177,6 +177,40 @@ def test_prepare_autofix_slot_preserves_new_head_workers_after_head_advance(monk workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, dry_run=False, ) is None + + +def test_prepare_autofix_slot_returns_directly_with_no_active_or_stale_runs(monkeypatch): + """An empty Actions run list needs no reconciliation and skips cancellation.""" + monkeypatch.setattr(fix, "run_json", lambda _args: {"workflow_runs": []}) + monkeypatch.setattr( + fix, + "force_cancel_workflow_runs", + lambda *_args: pytest.fail("no stale runs must not attempt cancellation"), + ) + + assert fix.prepare_autofix_slot( + "owner/repo", + make_pr(), + workflow=fix.DEFAULT_AUTOFIX_WORKFLOW, + workflow_repository=fix.DEFAULT_AUTOFIX_REPOSITORY, + dry_run=False, + ) is False + + +def test_live_head_matches_compares_case_insensitively_and_fails_closed(monkeypatch): + """Live head lookup normalizes case and rejects malformed or mismatched payloads.""" + head = "a" * 40 + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": head.upper()}}) + assert fix.live_head_matches("owner/repo", make_pr(headRefOid=head)) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"head": {"sha": "b" * 40}}) + assert not fix.live_head_matches("owner/repo", make_pr(headRefOid=head)) + + monkeypatch.setattr(fix, "run_json", lambda _args: {"nothead": {}}) + assert not fix.live_head_matches("owner/repo", make_pr(headRefOid=head)) + + def test_terminal_failed_check_triggers_rca_without_prior_opencode_review(): """Exact-head check evidence can start RCA without a circular review prerequisite.""" pr = make_pr( @@ -1329,6 +1363,21 @@ def test_fix_inspect_skip_wait_and_error_paths(monkeypatch): monkeypatch.setattr(fix, "issue_comments", lambda repo, number: [{"body": f"{fix.FIX_MARKER} head_sha={'a' * 40} epoch={int(time.time())} -->"}]) assert fix.inspect_pr("owner/repo", make_pr(), args) == ("wait", ("recent autofix marker exists for this head",)) + assert fix.inspect_pr( + "owner/repo", make_pr(mergeStateStatus="DIRTY", isDraft=True), args + ) == ("skip", ("draft PR",)) + assert fix.inspect_pr("owner/repo", make_pr(mergeStateStatus="DIRTY"), args) == ( + "skip", + ("merge conflict is not authorized for repair",), + ) + + monkeypatch.setattr(fix, "issue_comments", lambda repo, number: []) + monkeypatch.setattr(fix, "prepare_autofix_slot", lambda *_args, **_kwargs: True) + assert fix.inspect_pr("owner/repo", make_pr(), args) == ( + "wait", + ("current-head autofix run is already queued or running",), + ) + pr1 = make_pr(number=1) pr2 = make_pr(number=2) monkeypatch.setattr(fix, "fetch_open_prs", lambda repo, max_prs: [pr1, pr2]) diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index c24cfb05f9..f261ce5beb 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -154,3 +154,70 @@ def fake_api(path: str) -> Any: assert merge.is_strix_context(context) assert merge.strix_evidence_state(pr) == expected_state assert fix.current_head_failed_checks(pr) == () + + +def test_fetch_workflow_names_by_check_suite_rest_paginates_past_100( + monkeypatch: Any, +) -> None: + """A first page of exactly 100 runs must fetch a second page and merge both.""" + head_sha = "e" * 40 + page1 = [ + {"check_suite_id": i, "name": f"workflow-{i}"} for i in range(100) + ] + page2 = [{"check_suite_id": 100, "name": "workflow-100"}] + calls: list[str] = [] + + def fake_api(path: str) -> Any: + calls.append(path) + if path.endswith("page=1"): + return {"workflow_runs": page1} + if path.endswith("page=2"): + return {"workflow_runs": page2} + raise AssertionError(f"unexpected path {path}") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) + + assert names == {i: f"workflow-{i}" for i in range(101)} + assert calls == [ + f"repos/owner/repo/actions/runs?head_sha={head_sha}&per_page=100&page=1", + f"repos/owner/repo/actions/runs?head_sha={head_sha}&per_page=100&page=2", + ] + + +def test_fetch_workflow_names_by_check_suite_rest_skips_entries_missing_suite_id_or_name( + monkeypatch: Any, +) -> None: + """A run with no check-suite id or a blank name must not populate the map.""" + head_sha = "f" * 40 + + def fake_api(path: str) -> Any: + return { + "workflow_runs": [ + {"check_suite_id": None, "name": "orphaned run"}, + {"check_suite_id": 900, "name": ""}, + {"check_suite_id": 901, "name": "kept run"}, + ] + } + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + names = merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) + + assert names == {901: "kept run"} + + +def test_fetch_workflow_names_by_check_suite_rest_propagates_non_access_errors( + monkeypatch: Any, +) -> None: + """A page-fetch failure unrelated to integration access must fail closed.""" + head_sha = "0" * 40 + + def fake_api(path: str) -> Any: + raise RuntimeError("gh: HTTP 502 (exhausted retries)") + + monkeypatch.setattr(merge, "gh_api_json", fake_api) + + with pytest.raises(RuntimeError, match="HTTP 502"): + merge.fetch_workflow_names_by_check_suite_rest("owner/repo", head_sha) From 85c2469e1e624f8d4dfbc71c79fe18c927d315ab Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:38:38 +0000 Subject: [PATCH 03/10] docs(gap-baseline): record post-#1546 scheduler coverage regression Adds a dated traceability entry for the coverage gap this PR closes: root cause (#1546's uncovered additions plus the older #1547/#1551/ #1554 gap, neither of which merged or transfers evidence here), the fix and its verification, the resolved Devin false-positive on sub-clause coverage, and the known pre-existing SIGPIPE test flake left unremediated as out of scope. --- docs/product-technical-gap-baseline.md | 48 ++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md index 76d85b949b..812f068e34 100644 --- a/docs/product-technical-gap-baseline.md +++ b/docs/product-technical-gap-baseline.md @@ -2344,6 +2344,54 @@ contract assertion, and `docs/adr/0003-contextual-orchestrator-vendored-free-zdr "today" reference. Landed in the same PR (`#1463`) as the streaming revert, not split out, since the revert is unsafe without it. +## 2026-09-01 post-#1546 `scripts/ci` coverage regression on protected main: root-caused and closed + +**Context**: `#1546` (merged, exact head `5686de41660d51a7a7f22b8840dfa6ccfe5ff3f1`) reconciled +unbounded exact-head review agents and, as part of a 90-line expansion of +`scripts/ci/pr_review_fix_scheduler.py`, added a `live_head_matches` helper, a no-active/no-stale +fall-through branch in `prepare_autofix_slot`, and an "already queued or running" wait branch in +`inspect_pr` — none of which any test exercised directly. This compounded a narrower, older gap in +the same file (`inspect_pr`'s conflicted-draft and conflicted-unauthorized returns) and in +`scripts/ci/pr_review_merge_scheduler.py::fetch_workflow_names_by_check_suite_rest` (pagination, +missing-suite-id/blank-name filtering, non-access-error propagation), first found and attempted in +now-closed, unmerged `#1547`/`#1551`/`#1554` — none of whose evidence or diffs transferred here; +this pass re-derived the current gap from a clean `origin/main` clone rather than assuming those +predecessors were still accurate against `#1546`'s shifted line numbers and new branches. Verified +directly: `coverage report --show-missing` on unmodified `main` showed +`scripts/ci/pr_review_fix_scheduler.py` at 97% (missing 116-121, 459->466, 495, 503, 546) and +`scripts/ci/pr_review_merge_scheduler.py` at 99% (missing 1003, 1008->1005, 1012) — total repo-wide +99%, below the `pyproject.toml` `fail_under = 100` gate. Because `opencode-review-dispatch.yml`'s +`coverage-evidence` job measures the **merged** PR tree (base + head) and hard-fails below 100%, +every PR rebasing onto main inherited this failure regardless of its own diff — org-wide impact, +not scoped to one PR. + +**Fix**: `#1567` (test-only, no production code) adds direct unit coverage for `live_head_matches` +(case-insensitive match, mismatch, malformed-payload paths), `prepare_autofix_slot`'s empty-run +fall-through, the `inspect_pr` conflicted-draft/conflicted-unauthorized/already-queued cases, and +the `fetch_workflow_names_by_check_suite_rest` pagination/filtering/error-propagation paths. +Verified on the fix commit (`db106d50f2134ece147bc5318e389aeb124d198c`): `coverage run -m pytest +tests -q` (2251 passed, 1 skipped, 21 subtests), `coverage report` (repo-wide 100%, both files +individually 100% statement and 100% branch), `interrogate` (100.0%). + +**Devin Review raised a false positive on the fix itself**, claiming +`test_live_head_matches_compares_case_insensitively_and_fails_closed` left non-object-payload, +non-string-SHA, and wrong-length-SHA branches uncovered. Re-verified against the actual gate rather +than accepted at face value: `live_head_matches` has exactly one `if` statement (two arcs, both +exercised by the committed test), and its final `return (isinstance(...) and len(...) == 40 and +...)` is a single boolean expression with no `if`/`else` of its own — `coverage.py`'s branch mode +(what `fail_under = 100` actually measures here) tracks control-flow arcs between statements, not +sub-clause condition coverage within one expression. The cited cases are additional test +thoroughness, not something the gate is currently failing on; confirmed by a full-suite run on the +exact same head showing both files at 100% branch coverage with zero missing branches. Replied with +this evidence on the review thread and did not widen the PR's diff for a claim that does not hold +against this repo's own tooling. + +**One test in the full suite remains a known, pre-existing flake**, unrelated to this change: +`tests/test_opencode_required_verdict_regression.py::test_scheduler_wake_reuses_trusted_receipt_predicate` +intermittently exits 141 (SIGPIPE) under full-suite parallel load; reproduces identically on +unmodified `origin/main` and passes cleanly in file isolation. Not remediated here — out of scope +for a coverage-gap-only PR, and not itself a coverage regression. + ## 5. 실행 루프와 고객의 다음 행동 각 hourly pass는 아래 순서를 유지한다. From 6f40a0637da94da60f43ca72086d27e1034e8bbc Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 16:46:11 +0900 Subject: [PATCH 04/10] test(ci): document nested REST fixture helpers Raise scoped docstring coverage for the newly added scheduler REST regression helpers to 100% without changing test behavior or production code. --- tests/test_pr_review_fix_scheduler_rest_workflow_identity.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py index f261ce5beb..4e36544061 100644 --- a/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py +++ b/tests/test_pr_review_fix_scheduler_rest_workflow_identity.py @@ -168,6 +168,7 @@ def test_fetch_workflow_names_by_check_suite_rest_paginates_past_100( calls: list[str] = [] def fake_api(path: str) -> Any: + """Return deterministic paginated workflow-run fixtures.""" calls.append(path) if path.endswith("page=1"): return {"workflow_runs": page1} @@ -193,6 +194,7 @@ def test_fetch_workflow_names_by_check_suite_rest_skips_entries_missing_suite_id head_sha = "f" * 40 def fake_api(path: str) -> Any: + """Return workflow runs that exercise incomplete-identity filtering.""" return { "workflow_runs": [ {"check_suite_id": None, "name": "orphaned run"}, @@ -215,6 +217,7 @@ def test_fetch_workflow_names_by_check_suite_rest_propagates_non_access_errors( head_sha = "0" * 40 def fake_api(path: str) -> Any: + """Simulate a non-access REST failure that must propagate.""" raise RuntimeError("gh: HTTP 502 (exhausted retries)") monkeypatch.setattr(merge, "gh_api_json", fake_api) From 69481751e0029ea9fe791a52fc103a72027759eb Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 17:01:37 +0900 Subject: [PATCH 05/10] fix(tests): drain dispatch fixture stdin to break CI dependency cycle RCA: the #1567 exact-head Hourly NVIDIA NIM Review Repair run failed in test_scheduler_wake_reuses_trusted_receipt_predicate with exit 141. The production block pipes jq JSON into gh api --input -, while the test fake exited without reading stdin. Under pipefail that can SIGPIPE jq. Reuse the already RED/GREEN-verified #1569 fixture blob and drain stdin before recording the fake dispatch. This makes #1567 self-contained so the central 100% coverage repair no longer depends on a separate PR that itself inherits the coverage failure. --- tests/test_opencode_required_verdict_regression.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_opencode_required_verdict_regression.py b/tests/test_opencode_required_verdict_regression.py index 8f8047ff10..0e5d30805b 100644 --- a/tests/test_opencode_required_verdict_regression.py +++ b/tests/test_opencode_required_verdict_regression.py @@ -173,6 +173,7 @@ def test_scheduler_wake_reuses_trusted_receipt_predicate( elif [[ "$*" == *"/pulls/7/reviews"* ]]; then printf '[%s]' "$FAKE_REVIEWS" elif [[ "$*" == *"repos/ContextualWisdomLab/.github/dispatches"* ]]; then + cat >/dev/null printf 'dispatch\n' >>"$DISPATCH_CALLS" fi """, From d4cea2f8d4ecb7bbf55f4aaf53a029cb242776df Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 17:15:09 +0900 Subject: [PATCH 06/10] docs(changelog): record base-side deleted file context for Noema reviews --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f5810d5308..b546e62eb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ this file. The format follows Keep a Changelog, and versioned releases follow Semantic Versioning where the repository publishes a release. ## [Unreleased] +- Review deleted files from base-side evidence in Noema review gate: + `fetch_changed_files()` now captures the PR file `status`, and + `changed_file_context()` fetches pre-deletion content at `baseRefOid` + for removed files rather than attempting a head fetch that guaranteed + a 404 error and blocked substantive reviews. - Avoid redundant merge-scheduler wakes when the trusted receipt predicate already finds a substantive exact-head OpenCode verdict. Missing, stale, or fallback-only evidence still dispatches review work, while receipt lookup or From 400f2b5a63a5cdaf95a42ee4d49a4e492132738b Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 17:38:01 +0900 Subject: [PATCH 07/10] fix(noema): remove dead CodeGraph context branch from Noema review gate (#1491) * fix(noema): remove dead CodeGraph context branch from Noema review gate load_codegraph_context() read NOEMA_CODEGRAPH_CONTEXT_PATH and always returned "" because no workflow ever set that variable -- a full-repo grep confirms only this script and its own unit tests referenced it, and noema-review.yml never mentions CodeGraph. Every production Noema review therefore rendered an always-empty "## CodeGraph context" section while its own LLM prompt claimed CodeGraph context was supplied. Wiring the capability up for real would mean running the CodeGraph CLI's trusted-root setup against untrusted PR code inside noema-review.yml, a workflow that currently never checks out PR head content at all (it only materializes the trusted gate script itself and reads PR data through the GitHub content API) -- a security-sensitive feature addition, not a minimal fix. Since CodeGraph context was never wired for Noema and nothing else in the repo advertises it as a Noema capability, remove the hollow branch, the dead helper, and the prompt's false claim instead. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw * test(ci): port main coverage-gap fix from .github#1547 onto this branch Rebasing onto current main inherited the same pre-existing coverage gap #1547 (not yet merged) fixes: two REST-fallback scheduler code paths (fetch_workflow_names_by_check_suite_rest's pagination/error-handling, inspect_pr's conflicted-branch draft/unauthorized-conflict skips) with no existing test coverage. Ported #1547's identical test additions here rather than leaving this PR red waiting on that one to merge first -- this will no-op once #1547 lands and this branch rebases again. Full suite: 2228 passed, 1 skipped, 21 subtests. 100% coverage, 100% docstrings. --------- Co-authored-by: Claude Co-authored-by: Claude Code Co-authored-by: opencode-agent[bot] <219766164+opencode-agent[bot]@users.noreply.github.com> --- scripts/ci/noema_review_gate.py | 17 +---------------- tests/test_noema_review_gate.py | 15 ++------------- tests/test_pr_review_fix_scheduler.py | 7 +++++++ ...tory_branch_coverage_javascript_and_noema.py | 2 -- ...epository_branch_coverage_reporting_edges.py | 1 - 5 files changed, 10 insertions(+), 32 deletions(-) diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 249f94f6b7..68adc75c77 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -474,24 +474,9 @@ 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.""" 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) @@ -958,7 +943,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( { diff --git a/tests/test_noema_review_gate.py b/tests/test_noema_review_gate.py index 6272ff2b59..cc08797476 100644 --- a/tests/test_noema_review_gate.py +++ b/tests/test_noema_review_gate.py @@ -1378,7 +1378,7 @@ def test_current_actor_rejects_unbound_action_identity(monkeypatch, actor, insta noema.current_actor() -def test_review_context_builders_include_codegraph_threads_and_files(monkeypatch, tmp_path): +def test_review_context_builders_include_threads_and_files(monkeypatch): assert noema.truncate_text("abc", 10) == "abc" assert "truncated 2 characters" in noema.truncate_text("abcdef", 4) assert "missing PR head SHA" in noema.changed_file_context("owner/repo", 7, "") @@ -1405,9 +1405,6 @@ def fake_run(args, stdin=None): raise AssertionError(args) monkeypatch.setattr(noema, "run", fake_run) - codegraph_path = tmp_path / "codegraph.md" - codegraph_path.write_text("call graph: src/a.py -> tests", encoding="utf-8") - monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(codegraph_path)) pr = make_pr( headRefOid="head sha", reviewThreads={ @@ -1431,8 +1428,6 @@ def fake_run(args, stdin=None): context = noema.build_review_context("owner/repo", 7, pr) - assert "## CodeGraph context" in context - assert "call graph: src/a.py -> tests" in context assert "Thread open at src/a.py:3" in context assert "reviewer: check call site" in context assert "### src/a.py" in context @@ -1442,13 +1437,7 @@ def fake_run(args, stdin=None): assert any("/files" in call[2] for call in calls) -def test_review_context_reports_omitted_files_and_missing_codegraph(monkeypatch, tmp_path): - monkeypatch.delenv("NOEMA_CODEGRAPH_CONTEXT_PATH", raising=False) - assert noema.load_codegraph_context() == "" - - monkeypatch.setenv("NOEMA_CODEGRAPH_CONTEXT_PATH", str(tmp_path / "missing.md")) - assert "CodeGraph context unavailable" in noema.load_codegraph_context() - +def test_review_context_reports_omitted_files(monkeypatch): 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") diff --git a/tests/test_pr_review_fix_scheduler.py b/tests/test_pr_review_fix_scheduler.py index f6abd64b0f..32c20738ea 100644 --- a/tests/test_pr_review_fix_scheduler.py +++ b/tests/test_pr_review_fix_scheduler.py @@ -1352,6 +1352,13 @@ def test_fix_inspect_skip_wait_and_error_paths(monkeypatch): assert fix.inspect_pr("owner/repo", make_pr(headRepository={"nameWithOwner": "fork/repo"}), args)[1] == ( "external PR head is not writable by repository workflow credentials", ) + assert fix.inspect_pr( + "owner/repo", make_pr(mergeStateStatus="DIRTY", isDraft=True), args + ) == ("skip", ("draft PR",)) + assert fix.inspect_pr("owner/repo", make_pr(mergeStateStatus="DIRTY"), args) == ( + "skip", + ("merge conflict is not authorized for repair",), + ) monkeypatch.setattr(fix, "needs_autofix", lambda pr: (False, ())) assert fix.inspect_pr("owner/repo", make_pr(), args) == ( 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) == "" From 53911d5b63af8ea9c17442f3b1192414efd74564 Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 19:57:40 +0900 Subject: [PATCH 08/10] test(noema): isolate deleted-file base-context regression --- tests/test_noema_removed_file_context.py | 108 +++++++++++++++++++++++ 1 file changed, 108 insertions(+) create mode 100644 tests/test_noema_removed_file_context.py 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 From 1e54a79509cb9bba62e9989e873f545949d420ce Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 19:59:59 +0900 Subject: [PATCH 09/10] fix(noema): reconcile deleted-file context with transport retry --- scripts/ci/noema_review_gate.py | 169 +++++--------------------------- 1 file changed, 24 insertions(+), 145 deletions(-) diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index 351f08e5ab..a8520a6d75 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -7,6 +7,7 @@ import ast import base64 import hashlib +import http.client import ipaddress import json import os @@ -620,10 +621,6 @@ def _json_nesting_within_bound(text: str, start: int, max_depth: int) -> bool: if stack and stack[-1] == "{": stack.pop() if not stack: - # Only "{" can empty the stack: text[start] is always - # "{" (this function's own contract), so it is always - # the bottom-most, last-popped element; a "]" popping - # an inner "[" can never reach an empty stack itself. return True elif char == "]" and stack and stack[-1] == "[": stack.pop() @@ -634,73 +631,7 @@ def _json_nesting_within_bound(text: str, start: int, max_depth: int) -> bool: def extract_json_object(text: str) -> dict[str, Any]: - """Extract a JSON object from a strict or lightly wrapped LLM response. - - Fails closed with ``RuntimeError`` — the same "no usable verdict" failure - path ``call_llm`` already raises for an unsupported decision, a missing - summary, or a malformed finding — instead of letting a malformed or - truncated LLM response's ``json.JSONDecodeError`` propagate as an - unhandled exception and crash the review job. Only top-level brace groups - are candidates: a ``{`` is a candidate only while a bracket-type stack - (tracking ``{``/``[`` opens against their own matching ``}``/``]`` - closes) is empty, so a valid nested object cannot escape a malformed - outer *object or array* wrapper. Every candidate starts at a ``{``, - making each successful parse a JSON object (``dict``); only the decode - failure itself needs converting. Once a top-level candidate begins, a - decode failure rejects the response rather than scanning forward to a - later verdict; multiple objects remain supported only when the first - candidate decodes successfully. - - A closer that cannot legally match the innermost open bracket — nothing - open at all, or the innermost open bracket is the other type — stops - candidate discovery outright instead of being a no-op on the stack. Only - ignoring the mismatch (popping nothing, but continuing to scan) is not - enough: a *later*, otherwise-well-formed ``[``/``]`` or ``{``/``}`` pair - can still legitimately re-close the stack down to empty despite the - earlier mismatch, so a subsequent ``{`` would again be seen as a fresh - top-level candidate even though the response as a whole was never - cleanly-formed JSON (Devin review on PR #1507, e.g. ``[} ] {...}``: the - stray ``}`` is a no-op, but the following ``]`` still validly closes the - ``[``, and the ``{`` after that would wrongly look top-level again). Any - closer this malformed anywhere in the response is treated as proof the - whole response cannot be trusted to contain a clean top-level object - from that point on, not just proof that one bracket group failed to - close. - - The raised diagnostic never embeds the raw (or scrubbed) model response. - This is a ``pull_request_target`` workflow whose Actions logs are public - on this org's public repos, and ``scrub_sensitive_data`` is a finite, - pattern-based scrubber: an LLM can echo back or hallucinate a credential - in a shape none of its patterns recognize (mid-sentence, base64-wrapped, - or simply a shape nobody anticipated). A regex allowlist of known secret - *shapes* cannot be a complete defense, so instead of trying to perfect - it, the raw content is never logged at all. Only a length and a SHA-256 - content fingerprint are logged — enough to correlate repeat failures for - the same underlying (unlogged) response without exposing its bytes. - - Excessive nesting is rejected by an explicit ``_json_nesting_within_bound`` - check against ``MAX_JSON_NESTING_DEPTH`` (100 — generously above the - verdict schema's own real maximum of roughly 5 levels: object -> - ``findings``/``reviewed_lines``/``adversarial_validation.probes`` -> - each list's object entries), evaluated *before* ``raw_decode`` is ever - attempted, rather than by trusting ``json.JSONDecoder``'s own recursion - behavior to raise on deep input. That behavior is not a stable contract: - a real ``depth = max(20_000, sys.getrecursionlimit() * 2)`` nested-array - payload raises ``RecursionError`` from the C-accelerated scanner on - Python 3.11-3.13, but is decoded successfully (no exception at all) on - the Python 3.14.7 hosted runner this job actually runs on (job - 99642234627, commit ``ec23350e``: - ``test_extract_json_object_fails_closed_on_excessive_nesting`` failed - with "DID NOT RAISE RuntimeError" against that exact real payload). - Relying on ``RecursionError`` alone would make this fail-closed guarantee - a property of whichever CPython version happens to run the job, not of - this function. The explicit bound removes that dependency; a residual - ``except RecursionError`` is kept only as defense-in-depth for whatever - lies within the bound (``RecursionError`` is itself a ``RuntimeError`` - subclass, so even an unhandled one here would already surface through - ``call_llm``'s own ``except RuntimeError`` around this call and every - post-decode field read). - """ + """Extract a JSON object from a strict or lightly wrapped LLM response.""" stripped = text.strip() decoder = json.JSONDecoder() decode_error: json.JSONDecodeError | None = None @@ -727,13 +658,6 @@ def extract_json_object(text: str) -> dict[str, Any]: stack.append("[") elif character == "}": if not stack or stack[-1] != "{": - # A closer that cannot legally appear here (nothing open, or - # the innermost open bracket is a "[") is proof this response - # is not cleanly-formed JSON at all, not just proof that one - # bracket group failed to close. Stop finding new candidates - # rather than let bracket-type matching alone "resync" past - # it and treat a later, structurally-unrelated { as a fresh - # top-level verdict (Devin review on PR #1507). break stack.pop() elif character == "]": @@ -784,24 +708,7 @@ def extract_json_object(text: str) -> dict[str, Any]: def extract_llm_message_content(raw: str) -> str: - """Parse and validate the OpenAI-compatible chat-completion HTTP envelope. - - Fails closed with the same bounded ``RuntimeError`` ``call_llm`` already - uses for an unusable verdict, instead of letting a malformed gateway - reply crash the review job before it ever reaches the verdict-JSON - repair boundary handled by ``extract_json_object``. Covers a non-JSON - raw body, a non-object top-level JSON value, a wrong-shaped ``choices`` - or ``message`` field, and non-string ``content`` — each rejected with an - explicit ``isinstance`` check rather than a broad ``except``, so a - genuine programming error elsewhere in this module still surfaces as - itself. A missing or empty ``choices``/``message``/``content`` is left - to fall through to an empty string, matching the original code's - leniency for an absent (not malformed) field; ``extract_json_object`` - already fails closed on empty content. - - None of the raised messages embed any part of the untrusted response - body — only JSON-value type names, which cannot carry a credential. - """ + """Parse and validate the OpenAI-compatible chat-completion HTTP envelope.""" try: data = json.loads(raw) except json.JSONDecodeError as exc: @@ -841,26 +748,7 @@ def extract_llm_message_content(raw: str) -> str: def decode_llm_response_body(raw_bytes: bytes) -> str: - """Decode the raw gateway HTTP response body as UTF-8 text. - - Devin Review bug finding on PR #1507 round 3: a gateway reply containing - invalid UTF-8 used to raise ``UnicodeDecodeError`` at the plain - ``response.read().decode("utf-8")`` call in ``call_llm``, before that - body ever reached ``extract_llm_message_content`` or the verdict-JSON - repair boundary. That crashed the required review check with an - unhandled traceback instead of getting the same one-time schema-repair - retry every other malformed-envelope shape already gets. Call this - inside ``call_llm``'s existing repair-retry ``try`` block so a decode - failure converts to the same bounded ``RuntimeError`` and gets the same - fail-closed treatment. - - The raised diagnostic never embeds the raw response bytes — not even - the undecodable fragment. Only a length and a SHA-256 content - fingerprint are logged, matching ``extract_json_object``'s no-raw-content - pattern: a body containing invalid UTF-8 could still contain a - credential-adjacent byte sequence, and this is a ``pull_request_target`` - workflow whose Actions logs are public on this org's public repos. - """ + """Decode the raw gateway HTTP response body as UTF-8 text.""" try: return raw_bytes.decode("utf-8") except UnicodeDecodeError as exc: @@ -903,13 +791,7 @@ def _http_origin(parsed: urllib.parse.ParseResult) -> tuple[str, str, int] | Non def is_allowed_orchestrator_sidecar_url(api_url: str) -> bool: - """Return True only for the process-local orchestrator sidecar loopback origin. - - ``localhost`` and other private hosts stay rejected. A loopback literal - (``127.0.0.1`` / ``::1``) is allowed only when it matches the exact - ``CONTEXTUAL_ORCHESTRATOR_BASE_URL`` origin. The via-orchestrator marker is - metadata only and never widens this allowlist. - """ + """Return True only for the process-local orchestrator sidecar loopback origin.""" origin = _http_origin(urllib.parse.urlparse(api_url)) if origin is None: return False @@ -976,19 +858,19 @@ def call_llm( review_context: str = "", changed_paths: Sequence[str] = (), repair_error: str = "", + is_retry: bool = False, ) -> dict[str, Any]: """Call the configured OpenAI-compatible LLM endpoint for a review verdict. ``expected_head`` is the same normalized (lowercase) SHA ``inspect_and_review`` already checks before model work and before publication. It is threaded through here so the one-time repair-retry - request below — fired only after the first attempt's verdict was - malformed — can also confirm the PR head has not moved before spending a - second, potentially multi-hour model call on a - review that ``inspect_and_review``'s own post-call stale-head check would - discard anyway once this function returns. See ``fetch_pr`` for the live - lookup and ``StaleHeadDuringRepairRetryError`` for how that stale - condition is reported distinctly to the caller. + request below can confirm the PR head has not moved before spending a + second model call on a review that would otherwise be stale. + + ``is_retry`` tracks retry state independently of ``repair_error``'s text: + transport exceptions can stringify to an empty string, so gating retry + state on the diagnostic text would permit unbounded retries. """ api_url = os.environ.get("NOEMA_LLM_API_URL", "").strip() api_key = os.environ.get("NOEMA_LLM_API_KEY", "").strip() @@ -1048,10 +930,11 @@ def call_llm( "Use request_changes only for blocking, concrete issues. A generic no-issues statement is not review evidence.", *( [ - f"Your prior verdict was rejected by the trusted validator: {repair_error}", + "Your prior verdict was rejected by the trusted validator: " + f"{repair_error or 'no diagnostic message was available'}", "Return one corrected JSON verdict using only exact changed-side locations from the supplied diff.", ] - if repair_error + if is_retry else [] ), f"Repository: {repo}", @@ -1084,9 +967,9 @@ def call_llm( method="POST", ) opener = urllib.request.build_opener(NoRedirectHandler()) - with opener.open(request) as response: # nosec B310 - raw_bytes = response.read() try: + with opener.open(request) as response: # nosec B310 + raw_bytes = response.read() raw = decode_llm_response_body(raw_bytes) content = extract_llm_message_content(raw) verdict = extract_json_object(content) @@ -1114,9 +997,11 @@ def call_llm( if decision == "request_changes" and not findings: raise RuntimeError("Noema LLM request_changes response did not contain a substantive finding") validate_substantive_verdict(verdict, diff, changed_paths) - except RuntimeError as exc: - if repair_error: - raise + except (RuntimeError, urllib.error.URLError, http.client.HTTPException, OSError) as exc: + if is_retry: + if isinstance(exc, RuntimeError): + raise + raise RuntimeError(str(exc)) from exc if str(fetch_pr(repo, number).get("headRefOid") or "").lower() != expected_head: raise StaleHeadDuringRepairRetryError( "Pull request head changed during review; stale before repair retry." @@ -1131,6 +1016,7 @@ def call_llm( review_context, changed_paths, str(exc), + is_retry=True, ) return verdict @@ -1216,14 +1102,7 @@ def submit_review(repo: str, number: int, pr: dict[str, Any], actor: str, verdic def inspect_and_review(repo: str, number: int, expected_head: str) -> int: - """Inspect PR state and submit Noema's independent LLM review. - - ``expected_head`` is normalized defensively before the stale-head - comparisons below, and before the one ``call_llm`` performs on its own - repair-retry path (see ``StaleHeadDuringRepairRetryError``). The CLI and - workflow require canonical lowercase SHA input so equivalent casing - cannot split the workflow concurrency group. - """ + """Inspect PR state and submit Noema's independent LLM review.""" expected_head = expected_head.strip().lower() pr = fetch_pr(repo, number) try: From 22fbbdefe9b35e7938325068540505bad23376bc Mon Sep 17 00:00:00 2001 From: Seongho Bae Date: Tue, 1 Sep 2026 20:12:38 +0900 Subject: [PATCH 10/10] fix(noema): bind deleted-file evidence to merge base --- scripts/ci/noema_review_gate.py | 335 +++++++++++++++++++++++++------- 1 file changed, 261 insertions(+), 74 deletions(-) diff --git a/scripts/ci/noema_review_gate.py b/scripts/ci/noema_review_gate.py index a8520a6d75..ef270872a2 100644 --- a/scripts/ci/noema_review_gate.py +++ b/scripts/ci/noema_review_gate.py @@ -396,28 +396,13 @@ 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.""" - output = run( - [ - "gh", - "api", - f"repos/{repo}/pulls/{number}/files", - "--paginate", - "--jq", - ".[].filename", - ] - ) - return [line.strip() for line in output.splitlines() if line.strip()] - - def fetch_changed_files(repo: str, number: int) -> list[tuple[str, str]]: - """Fetch each changed file's path together with its PR status. + """Fetch changed paths and statuses without corrupting whitespace in paths. - 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. + 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( [ @@ -426,21 +411,32 @@ def fetch_changed_files(repo: str, number: int) -> list[tuple[str, str]]: f"repos/{repo}/pulls/{number}/files", "--paginate", "--jq", - r'.[] | .filename + "\t" + .status', + r'.[] | [.filename, .status] | @json', ] ) files: list[tuple[str, str]] = [] for line in output.splitlines(): - stripped = line.strip() - if not stripped: + if not line: continue - path, _, status = stripped.partition("\t") - files.append((path, status)) + 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, ref: str) -> str: - """Fetch a changed file's text content at ``ref`` 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(ref, safe="") content = run( @@ -458,40 +454,102 @@ def fetch_head_file_content(repo: str, path: str, ref: str) -> str: return base64.b64decode(compact).decode("utf-8", errors="replace") -def removed_file_context_section(repo: str, path: str, base_sha: str) -> str: - """Build the context section for a file deleted by this PR. +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() + - 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. +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 not base_sha: - return f"### {path}\n[File removed in this PR — no head-side content applicable; base SHA unavailable for pre-deletion 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_head_file_content(repo, path, base_sha) + 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.] Unavailable from base content API: {reason}" + 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 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)}" + 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 = "") -> str: - """Build bounded changed-file context for cross-file review reasoning.""" +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." - files = fetch_changed_files(repo, number) + 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, status in files[:MAX_CONTEXT_FILES]: if status == "removed": - sections.append(removed_file_context_section(repo, path, base_sha)) + 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}") @@ -527,29 +585,23 @@ 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 ""), str(pr.get("baseRefOid") or "") + repo, + number, + str(pr.get("headRefOid") or ""), + str(pr.get("baseRefOid") or ""), + changed_files, ) if files: sections.append("## Changed file context\n" + files) @@ -621,6 +673,10 @@ def _json_nesting_within_bound(text: str, start: int, max_depth: int) -> bool: if stack and stack[-1] == "{": stack.pop() if not stack: + # Only "{" can empty the stack: text[start] is always + # "{" (this function's own contract), so it is always + # the bottom-most, last-popped element; a "]" popping + # an inner "[" can never reach an empty stack itself. return True elif char == "]" and stack and stack[-1] == "[": stack.pop() @@ -631,7 +687,73 @@ def _json_nesting_within_bound(text: str, start: int, max_depth: int) -> bool: def extract_json_object(text: str) -> dict[str, Any]: - """Extract a JSON object from a strict or lightly wrapped LLM response.""" + """Extract a JSON object from a strict or lightly wrapped LLM response. + + Fails closed with ``RuntimeError`` — the same "no usable verdict" failure + path ``call_llm`` already raises for an unsupported decision, a missing + summary, or a malformed finding — instead of letting a malformed or + truncated LLM response's ``json.JSONDecodeError`` propagate as an + unhandled exception and crash the review job. Only top-level brace groups + are candidates: a ``{`` is a candidate only while a bracket-type stack + (tracking ``{``/``[`` opens against their own matching ``}``/``]`` + closes) is empty, so a valid nested object cannot escape a malformed + outer *object or array* wrapper. Every candidate starts at a ``{``, + making each successful parse a JSON object (``dict``); only the decode + failure itself needs converting. Once a top-level candidate begins, a + decode failure rejects the response rather than scanning forward to a + later verdict; multiple objects remain supported only when the first + candidate decodes successfully. + + A closer that cannot legally match the innermost open bracket — nothing + open at all, or the innermost open bracket is the other type — stops + candidate discovery outright instead of being a no-op on the stack. Only + ignoring the mismatch (popping nothing, but continuing to scan) is not + enough: a *later*, otherwise-well-formed ``[``/``]`` or ``{``/``}`` pair + can still legitimately re-close the stack down to empty despite the + earlier mismatch, so a subsequent ``{`` would again be seen as a fresh + top-level candidate even though the response as a whole was never + cleanly-formed JSON (Devin review on PR #1507, e.g. ``[} ] {...}``: the + stray ``}`` is a no-op, but the following ``]`` still validly closes the + ``[``, and the ``{`` after that would wrongly look top-level again). Any + closer this malformed anywhere in the response is treated as proof the + whole response cannot be trusted to contain a clean top-level object + from that point on, not just proof that one bracket group failed to + close. + + The raised diagnostic never embeds the raw (or scrubbed) model response. + This is a ``pull_request_target`` workflow whose Actions logs are public + on this org's public repos, and ``scrub_sensitive_data`` is a finite, + pattern-based scrubber: an LLM can echo back or hallucinate a credential + in a shape none of its patterns recognize (mid-sentence, base64-wrapped, + or simply a shape nobody anticipated). A regex allowlist of known secret + *shapes* cannot be a complete defense, so instead of trying to perfect + it, the raw content is never logged at all. Only a length and a SHA-256 + content fingerprint are logged — enough to correlate repeat failures for + the same underlying (unlogged) response without exposing its bytes. + + Excessive nesting is rejected by an explicit ``_json_nesting_within_bound`` + check against ``MAX_JSON_NESTING_DEPTH`` (100 — generously above the + verdict schema's own real maximum of roughly 5 levels: object -> + ``findings``/``reviewed_lines``/``adversarial_validation.probes`` -> + each list's object entries), evaluated *before* ``raw_decode`` is ever + attempted, rather than by trusting ``json.JSONDecoder``'s own recursion + behavior to raise on deep input. That behavior is not a stable contract: + a real ``depth = max(20_000, sys.getrecursionlimit() * 2)`` nested-array + payload raises ``RecursionError`` from the C-accelerated scanner on + Python 3.11-3.13, but is decoded successfully (no exception at all) on + the Python 3.14.7 hosted runner this job actually runs on (job + 99642234627, commit ``ec23350e``: + ``test_extract_json_object_fails_closed_on_excessive_nesting`` failed + with "DID NOT RAISE RuntimeError" against that exact real payload). + Relying on ``RecursionError`` alone would make this fail-closed guarantee + a property of whichever CPython version happens to run the job, not of + this function. The explicit bound removes that dependency; a residual + ``except RecursionError`` is kept only as defense-in-depth for whatever + lies within the bound (``RecursionError`` is itself a ``RuntimeError`` + subclass, so even an unhandled one here would already surface through + ``call_llm``'s own ``except RuntimeError`` around this call and every + post-decode field read). + """ stripped = text.strip() decoder = json.JSONDecoder() decode_error: json.JSONDecodeError | None = None @@ -658,6 +780,13 @@ def extract_json_object(text: str) -> dict[str, Any]: stack.append("[") elif character == "}": if not stack or stack[-1] != "{": + # A closer that cannot legally appear here (nothing open, or + # the innermost open bracket is a "[") is proof this response + # is not cleanly-formed JSON at all, not just proof that one + # bracket group failed to close. Stop finding new candidates + # rather than let bracket-type matching alone "resync" past + # it and treat a later, structurally-unrelated { as a fresh + # top-level verdict (Devin review on PR #1507). break stack.pop() elif character == "]": @@ -708,7 +837,24 @@ def extract_json_object(text: str) -> dict[str, Any]: def extract_llm_message_content(raw: str) -> str: - """Parse and validate the OpenAI-compatible chat-completion HTTP envelope.""" + """Parse and validate the OpenAI-compatible chat-completion HTTP envelope. + + Fails closed with the same bounded ``RuntimeError`` ``call_llm`` already + uses for an unusable verdict, instead of letting a malformed gateway + reply crash the review job before it ever reaches the verdict-JSON + repair boundary handled by ``extract_json_object``. Covers a non-JSON + raw body, a non-object top-level JSON value, a wrong-shaped ``choices`` + or ``message`` field, and non-string ``content`` — each rejected with an + explicit ``isinstance`` check rather than a broad ``except``, so a + genuine programming error elsewhere in this module still surfaces as + itself. A missing or empty ``choices``/``message``/``content`` is left + to fall through to an empty string, matching the original code's + leniency for an absent (not malformed) field; ``extract_json_object`` + already fails closed on empty content. + + None of the raised messages embed any part of the untrusted response + body — only JSON-value type names, which cannot carry a credential. + """ try: data = json.loads(raw) except json.JSONDecodeError as exc: @@ -748,7 +894,26 @@ def extract_llm_message_content(raw: str) -> str: def decode_llm_response_body(raw_bytes: bytes) -> str: - """Decode the raw gateway HTTP response body as UTF-8 text.""" + """Decode the raw gateway HTTP response body as UTF-8 text. + + Devin Review bug finding on PR #1507 round 3: a gateway reply containing + invalid UTF-8 used to raise ``UnicodeDecodeError`` at the plain + ``response.read().decode("utf-8")`` call in ``call_llm``, before that + body ever reached ``extract_llm_message_content`` or the verdict-JSON + repair boundary. That crashed the required review check with an + unhandled traceback instead of getting the same one-time schema-repair + retry every other malformed-envelope shape already gets. Call this + inside ``call_llm``'s existing repair-retry ``try`` block so a decode + failure converts to the same bounded ``RuntimeError`` and gets the same + fail-closed treatment. + + The raised diagnostic never embeds the raw response bytes — not even + the undecodable fragment. Only a length and a SHA-256 content + fingerprint are logged, matching ``extract_json_object``'s no-raw-content + pattern: a body containing invalid UTF-8 could still contain a + credential-adjacent byte sequence, and this is a ``pull_request_target`` + workflow whose Actions logs are public on this org's public repos. + """ try: return raw_bytes.decode("utf-8") except UnicodeDecodeError as exc: @@ -791,7 +956,13 @@ def _http_origin(parsed: urllib.parse.ParseResult) -> tuple[str, str, int] | Non def is_allowed_orchestrator_sidecar_url(api_url: str) -> bool: - """Return True only for the process-local orchestrator sidecar loopback origin.""" + """Return True only for the process-local orchestrator sidecar loopback origin. + + ``localhost`` and other private hosts stay rejected. A loopback literal + (``127.0.0.1`` / ``::1``) is allowed only when it matches the exact + ``CONTEXTUAL_ORCHESTRATOR_BASE_URL`` origin. The via-orchestrator marker is + metadata only and never widens this allowlist. + """ origin = _http_origin(urllib.parse.urlparse(api_url)) if origin is None: return False @@ -865,12 +1036,20 @@ def call_llm( ``expected_head`` is the same normalized (lowercase) SHA ``inspect_and_review`` already checks before model work and before publication. It is threaded through here so the one-time repair-retry - request below can confirm the PR head has not moved before spending a - second model call on a review that would otherwise be stale. + request below — fired only after the first attempt's verdict was + malformed — can also confirm the PR head has not moved before spending a + second, potentially multi-hour model call on a + review that ``inspect_and_review``'s own post-call stale-head check would + discard anyway once this function returns. See ``fetch_pr`` for the live + lookup and ``StaleHeadDuringRepairRetryError`` for how that stale + condition is reported distinctly to the caller. ``is_retry`` tracks retry state independently of ``repair_error``'s text: - transport exceptions can stringify to an empty string, so gating retry - state on the diagnostic text would permit unbounded retries. + several transport exceptions (a bare ``OSError``/``TimeoutError`` or + ``http.client.HTTPException`` raised with no message) stringify to an + empty string, so gating on ``repair_error``'s truthiness alone would let + an empty-message failure retry unboundedly instead of failing closed + after one attempt. """ api_url = os.environ.get("NOEMA_LLM_API_URL", "").strip() api_key = os.environ.get("NOEMA_LLM_API_KEY", "").strip() @@ -894,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( { @@ -1102,7 +1281,14 @@ def submit_review(repo: str, number: int, pr: dict[str, Any], actor: str, verdic def inspect_and_review(repo: str, number: int, expected_head: str) -> int: - """Inspect PR state and submit Noema's independent LLM review.""" + """Inspect PR state and submit Noema's independent LLM review. + + ``expected_head`` is normalized defensively before the stale-head + comparisons below, and before the one ``call_llm`` performs on its own + repair-retry path (see ``StaleHeadDuringRepairRetryError``). The CLI and + workflow require canonical lowercase SHA input so equivalent casing + cannot split the workflow concurrency group. + """ expected_head = expected_head.strip().lower() pr = fetch_pr(repo, number) try: @@ -1125,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: