From 57a2da1bf8cc99f8dac376deeda8fb352347fd05 Mon Sep 17 00:00:00 2001 From: Mengye Ren Date: Fri, 23 Oct 2026 00:55:42 -0400 Subject: [PATCH 1/3] Advisory review covers the head its event named: pinned diff, envelope head_sha, post refusal and commit_id; base-tree brief note for forks --- CHANGELOG.md | 1 + src/outerloop/github.py | 23 +++++- src/outerloop/posting.py | 12 ++- src/outerloop/review.py | 11 ++- src/outerloop/review_agent.py | 67 +++++++++++++++-- src/outerloop/review_post_cli.py | 26 +++++++ src/outerloop/review_summarize_cli.py | 26 ++++++- tests/test_posting.py | 8 +- tests/test_review_agent.py | 104 +++++++++++++++++++++++++- tests/test_review_summarize.py | 23 +++++- 10 files changed, 284 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93d63595..c86b8b8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Versions follow [SemVer](https://semver.org). ## [Unreleased] +- The advisory review covers exactly the head its triggering event named (#462): with `REVIEW_HEAD_SHA` set, the session skips clean, running no model, if the PR head has moved, and otherwise reads the diff between the base and that head as fixed commits; every envelope carries the reviewed `head_sha`; the posting job posts nothing when the envelope's or the event's head differs from the PR's current head, and attaches the review to the reviewed commit (`commit_id`), so a push during posting cannot move it. The summarizer carries the lenses' common `head_sha` through and skips clean if they disagree. With `REVIEW_CHECKOUT_BASE=1` (a fork PR, whose code is not checked out) the brief says the tree is the base branch and the change is only in the diff. Upgrading: envelopes without `head_sha` (from an older reviewer ref) still post; callers that do not pass REVIEW_HEAD_SHA behave as before. - When check runs cannot be read on a token deployment, the warning now says the token cannot read them (fine-grained tokens have no checks permission) instead of asking for an App permission. ## [0.5.0] - 2026-10-23 diff --git a/src/outerloop/github.py b/src/outerloop/github.py index 0a0abe69..dc13b022 100644 --- a/src/outerloop/github.py +++ b/src/outerloop/github.py @@ -741,6 +741,23 @@ def get_pull_request_diff(self, repo: str, number: int) -> str: ) return self.raw_transport(request) + def get_compare_diff(self, repo: str, base: str, head: str) -> str: + """The unified diff `base...head` (from their merge base, as a PR diff + is) between two fixed commits, so a later push cannot change it.""" + path = ( + f"/repos/{urllib.parse.quote(repo)}/compare/" + f"{urllib.parse.quote(base, safe='')}...{urllib.parse.quote(head, safe='')}" + ) + request = urllib.request.Request( + f"{API}{path}", + method="GET", + headers={ + "Authorization": f"Bearer {self.auth.token()}", + "Accept": "application/vnd.github.v3.diff", + }, + ) + return self.raw_transport(request) + def list_directory(self, repo: str, path: str, ref: str) -> list[dict[str, Any]]: """Entries of a directory at a ref ([] when absent or not a dir). @@ -841,8 +858,10 @@ def create_pr_review( number: int, body: str, comments: list[dict[str, Any]] | None = None, + commit_id: str = "", ) -> None: - """Post a PR review with optional inline comments. + """Post a PR review with optional inline comments, attached to + `commit_id` when given (else GitHub picks the PR's current head). The event is hard-coded to COMMENT: this client must never be able to approve or request changes — a Write-role bot review that blocks @@ -861,6 +880,8 @@ def create_pr_review( payload: dict[str, Any] = {"body": body, "event": "COMMENT"} if comments: payload["comments"] = comments + if commit_id: + payload["commit_id"] = commit_id self._request( "POST", f"/repos/{urllib.parse.quote(repo)}/pulls/{number}/reviews", diff --git a/src/outerloop/posting.py b/src/outerloop/posting.py index fd068100..f784fdbe 100644 --- a/src/outerloop/posting.py +++ b/src/outerloop/posting.py @@ -160,16 +160,24 @@ def post_round_review( pr_data: dict, fallback_body: str, reviewed_by: str = "", + commit_id: str = "", ) -> str: """The Reviews-API sibling of post_round: body summary plus anchored inline comments, event COMMENT always (the client hard-codes it). A posting failure falls back to a plain issue comment carrying fallback_body — the FULL single-comment rendering, because the review body alone may say no more than "findings are attached" while the - findings live in the rejected inline payload.""" + findings live in the rejected inline payload. `commit_id` pins the + review to the head that was reviewed, so a push meanwhile cannot move it.""" stamp, round_label = _round_stamp(client, repo, number, marker, pr_data, reviewed_by) try: - client.create_pr_review(repo, number, body.replace(marker, f"{marker}\n{stamp}", 1), inline) + client.create_pr_review( + repo, + number, + body.replace(marker, f"{marker}\n{stamp}", 1), + inline, + commit_id=commit_id, + ) except EXPECTED_FAILURES as exc: log.warning("inline review failed (%s); falling back to a comment", exc) client.comment(repo, number, fallback_body.replace(marker, f"{marker}\n{stamp}", 1)) diff --git a/src/outerloop/review.py b/src/outerloop/review.py index 278fc600..aefd9363 100644 --- a/src/outerloop/review.py +++ b/src/outerloop/review.py @@ -361,6 +361,7 @@ def build_agent_brief( lens: str = "", guidance: str = "", panel: bool = False, + base_tree: bool = False, ) -> str: """The reviewer brief for an agent session: the shared rubric, the investigation instruction, and the PR itself, built on the shared @@ -376,14 +377,22 @@ def build_agent_brief( raise ValueError(f"unknown review lens {lens!r} (have: {sorted(REVIEW_LENSES)})") focus = f"\n\n{REVIEW_LENSES[lens]}" if lens and lens != "general" else "" rules = f"\n\n{PANEL_BLOCKING}" if panel else "" + # a fork PR's head is never checked out: its code stays off the runner + tree = f"\n\n{BASE_TREE_NOTE}" if base_tree else "" # the guidance rides the trusted prompt BEFORE the untrusted PR text, so a # report cannot forge or swallow it return ( f"{SYSTEM_PROMPT}{focus}{rules}{guidance_section(guidance)}\n\n" - f"{_agent_investigation(syscall_cmd)}\n\n{build_prompt(pr, today)}" + f"{_agent_investigation(syscall_cmd)}{tree}\n\n{build_prompt(pr, today)}" ) +BASE_TREE_NOTE = ( + "The working tree is the pull request's base branch, not its head: the change exists " + "only in the diff below. Read the surrounding code in the tree and the change in the diff." +) + + MEASUREMENT_PROVENANCE_CAUTION = ( "Numbers measured on different commits are not directly comparable; check the commits " "in the checkout history before attributing a gap between them." diff --git a/src/outerloop/review_agent.py b/src/outerloop/review_agent.py index 9afef669..bdafb9d9 100644 --- a/src/outerloop/review_agent.py +++ b/src/outerloop/review_agent.py @@ -13,6 +13,7 @@ import contextlib import json import logging +import os from datetime import UTC, datetime from pathlib import Path from typing import Any @@ -89,9 +90,21 @@ def sanitize_checkout(tree: Path) -> tuple[int, int]: return renamed, failed -def pull_request(client: GitHubClient, repo: str, number: int) -> tuple[PullRequest, dict]: +def pull_request( + client: GitHubClient, repo: str, number: int, *, pin: str = "" +) -> tuple[PullRequest, dict]: + """The PR and its diff. With `pin` (a head SHA) and the PR still at it, the + diff is taken between fixed commits, so a push after the metadata read + cannot swap in another head's diff.""" pr_data = client.get_pull_request(repo, number) - diff = client.get_pull_request_diff(repo, number) + head = pr_data.get("head") + base = pr_data.get("base") + head_sha = str(head.get("sha", "")) if isinstance(head, dict) else "" + base_sha = str(base.get("sha", "")) if isinstance(base, dict) else "" + if pin and head_sha == pin and base_sha: + diff = client.get_compare_diff(repo, base_sha, pin) + else: + diff = client.get_pull_request_diff(repo, number) pr = PullRequest( repo=repo, number=number, @@ -119,10 +132,12 @@ def emit_envelope( detail: str = "", reviewed_by: str = "", lens: str = "", + head_sha: str = "", ) -> None: """Write the posting step's input. repo/number ride along so the poster can refuse an envelope that does not match its own PR reference; - reviewed_by (backend/model) rides along for the round stamp.""" + reviewed_by (backend/model) rides along for the round stamp; head_sha + lets the poster refuse a review of a head the PR has since left.""" path.parent.mkdir(parents=True, exist_ok=True) path.write_text( json.dumps( @@ -134,6 +149,7 @@ def emit_envelope( "detail": detail, "reviewed_by": reviewed_by, "lens": lens, + "head_sha": head_sha, } ) ) @@ -169,8 +185,14 @@ def run_agent_review( """ spec = spec or reviewer_spec() today = today or datetime.now(UTC).date().isoformat() + head_sha = "" + # The workflow names the head the maintainer saw. A fork can push between + # the label and this run, so review exactly that head or none. + requested = os.environ.get("REVIEW_HEAD_SHA", "").strip() try: - pr, pr_data = pull_request(client, repo, number) + pr, pr_data = pull_request(client, repo, number, pin=requested) + head = pr_data.get("head") + head_sha = str(head.get("sha", "")) if isinstance(head, dict) else "" skip = skip_reason(pr, bot_login) if skip is not None: log.info("skipping agent review of %s#%s: %s", repo, number, skip) @@ -178,12 +200,41 @@ def run_agent_review( # even a clean skip leaves an envelope: the posting job can # then REQUIRE an artifact, so "no artifact" always means a # broken session, never an ambiguous quiet day - emit_envelope(emit_path, repo, number, kind="skip-clean", detail=skip) + emit_envelope( + emit_path, + repo, + number, + kind="skip-clean", + detail=skip, + head_sha=head_sha, + ) + return None + if requested and head_sha != requested: + detail = ( + "head moved since the review was requested " + f"({requested[:8]} -> {head_sha[:8] or 'unknown'}); no review run" + ) + log.info("skipping agent review of %s#%s: %s", repo, number, detail) + if emit_path is not None: + emit_envelope( + emit_path, + repo, + number, + kind="skip-clean", + detail=detail, + head_sha=head_sha, + ) return None from outerloop.syscall import tool_command - brief = build_agent_brief(pr, today, syscall_cmd=tool_command(workspace), lens=lens) + brief = build_agent_brief( + pr, + today, + syscall_cmd=tool_command(workspace), + lens=lens, + base_tree=os.environ.get("REVIEW_CHECKOUT_BASE", "").strip() == "1", + ) role_result = redact_role_result(run_role(spec, harness, brief, workspace), harness) review = review_result_from_role(role_result) if review is None: @@ -208,6 +259,7 @@ def run_agent_review( kind="skip-stub", detail=detail, reviewed_by=backend_id(harness), + head_sha=head_sha, ) elif outage(role_result.session) or budget_exhausted(role_result.session): post_skip_stub(client, repo, number, "advisory review", RuntimeError(detail)) @@ -225,6 +277,7 @@ def run_agent_review( data=role_result.data, reviewed_by=backend_id(harness), lens=lens, + head_sha=head_sha, ) cost = role_result.session.cost_usd log.info( @@ -252,6 +305,7 @@ def run_agent_review( pr_data, fallback_body=full, reviewed_by=backend_id(harness), + commit_id=head_sha, ) cost = role_result.session.cost_usd log.info( @@ -278,5 +332,6 @@ def run_agent_review( kind="skip-stub", detail=detail, reviewed_by=backend_id(harness), + head_sha=head_sha, ) return None diff --git a/src/outerloop/review_post_cli.py b/src/outerloop/review_post_cli.py index 3db7bf1b..174b87b2 100644 --- a/src/outerloop/review_post_cli.py +++ b/src/outerloop/review_post_cli.py @@ -66,6 +66,31 @@ def post_from_file( if skip is not None: log.info("skipping post on %s#%s: %s", repo, number, skip) return None + # The head moved after the session read it: post nothing rather than + # stamp findings onto a head nobody reviewed. An envelope without + # head_sha (an older reviewer ref) is checked by the event head alone. + head = pr_data.get("head") + current = str(head.get("sha", "")) if isinstance(head, dict) else "" + envelope_head = str(envelope.get("head_sha") or "") + if envelope_head and envelope_head != current: + log.info( + "skipping post on %s#%s: envelope reviewed head %s, current head %s", + repo, + number, + envelope_head[:8], + current[:8] or "unknown", + ) + return None + requested = os.environ.get("REVIEW_HEAD_SHA", "").strip() + if requested and requested != current: + log.info( + "skipping post on %s#%s: requested head %s, current head %s", + repo, + number, + requested[:8], + current[:8] or "unknown", + ) + return None if kind == "skip-stub": # sanitize: the detail crossed the job boundary and lands in a # comment — collapse newlines, neutralize markdown, cap length @@ -104,6 +129,7 @@ def post_from_file( pr_data, fallback_body=full, reviewed_by=str(envelope.get("reviewed_by", "")), + commit_id=envelope_head, ) log.info( "posted review (%s) on %s#%s (reviewer=%s)", diff --git a/src/outerloop/review_summarize_cli.py b/src/outerloop/review_summarize_cli.py index 6bb56815..f7822fca 100644 --- a/src/outerloop/review_summarize_cli.py +++ b/src/outerloop/review_summarize_cli.py @@ -82,13 +82,29 @@ def main() -> int: number = int(number_raw) emit_path = Path(emit_env).resolve() + envelopes = _load_envelopes(Path(src).resolve(), repo, number) + # Lens sessions must all have read the same head; a panel split across + # two heads cannot be merged into one round (empty means an older ref). + heads = {str(e.get("head_sha") or "") for e in envelopes} - {""} + if len(heads) > 1: + detail = "lens envelopes disagree on head_sha: " + ", ".join(sorted(h[:8] for h in heads)) + log.warning("%s; nothing to merge", detail) + emit_envelope(emit_path, repo, number, kind="skip-clean", detail=detail) + return 0 + head_sha = next(iter(heads), "") + def stub(detail: str) -> int: emit_envelope( - emit_path, repo, number, kind="skip-stub", detail=detail, reviewed_by="summarizer" + emit_path, + repo, + number, + kind="skip-stub", + detail=detail, + reviewed_by="summarizer", + head_sha=head_sha, ) return 0 - envelopes = _load_envelopes(Path(src).resolve(), repo, number) # A lens that CRASHED before uploading any envelope is invisible in the # artifact set — the caller declares the expected panel so the delta is # reported rather than silently omitted from the merged round. @@ -111,7 +127,9 @@ def stub(detail: str) -> int: # all skipped/failed: ONE stub summarizing why (clean skips stay # clean — the poster's own skip re-check silences bot/opt-out PRs) if all(e.get("kind") == "skip-clean" for e in envelopes): - emit_envelope(emit_path, repo, number, kind="skip-clean", detail=details) + emit_envelope( + emit_path, repo, number, kind="skip-clean", detail=details, head_sha=head_sha + ) return 0 return stub(f"no lens produced findings ({details})") failed = [e for e in envelopes if e.get("kind") == "skip-stub"] + vanished @@ -129,6 +147,7 @@ def stub(detail: str) -> int: data=data, reviewed_by=str(only.get("reviewed_by", "")), lens=str(only.get("lens", "")), + head_sha=head_sha, ) log.info("single real opinion (%s); passed through", only.get("lens") or "general") return 0 @@ -156,6 +175,7 @@ def stub(detail: str) -> int: kind="findings", data=_with_lost_lenses(dict(role_result.data), failed), reviewed_by=f"summarizer:{backend_id(harness)} over {lenses}", + head_sha=head_sha, ) log.info("merged %d opinions (%s)", len(reals), lenses) return 0 diff --git a/tests/test_posting.py b/tests/test_posting.py index e3b804ab..0f732c25 100644 --- a/tests/test_posting.py +++ b/tests/test_posting.py @@ -31,7 +31,9 @@ def list_pr_reviews(self, repo: str, number: int, max_pages: int = 10) -> list[d def comment(self, repo: str, number: int, body: str) -> None: self.posted.append({"body": body, "kind": "comment"}) - def create_pr_review(self, repo: str, number: int, body: str, comments: Any = None) -> None: + def create_pr_review( + self, repo: str, number: int, body: str, comments: Any = None, commit_id: str = "" + ) -> None: self.posted.append({"body": body, "kind": "review", "inline": comments or []}) @@ -165,7 +167,9 @@ def test_post_round_review_posts_inline_on_success() -> None: def test_post_round_review_falls_back_to_a_comment_carrying_full_findings() -> None: class _NoInline(_FakeClient): - def create_pr_review(self, repo: str, number: int, body: str, comments: Any = None) -> None: + def create_pr_review( + self, repo: str, number: int, body: str, comments: Any = None, commit_id: str = "" + ) -> None: raise GitHubError(422, "/repos/org/repo", "unprocessable") client = _NoInline() diff --git a/tests/test_review_agent.py b/tests/test_review_agent.py index 9a36bc72..9ab07553 100644 --- a/tests/test_review_agent.py +++ b/tests/test_review_agent.py @@ -92,6 +92,8 @@ def __init__(self, *, author: str = "alice", labels: Any = _UNSET) -> None: self._author = author self._labels = [] if labels is _Client._UNSET else labels self.reviews: list[tuple[str, list[dict]]] = [] + self.review_commits: list[str] = [] + self.compared: list[tuple[str, str]] = [] self.comments: list[str] = [] def get_pull_request(self, repo: str, number: int) -> dict: @@ -101,19 +103,27 @@ def get_pull_request(self, repo: str, number: int) -> dict: "user": {"login": self._author}, "labels": self._labels, "head": {"sha": "deadbeef1234"}, + "base": {"sha": "0badc0de0000"}, } def get_pull_request_diff(self, repo: str, number: int) -> str: return _DIFF + def get_compare_diff(self, repo: str, base: str, head: str) -> str: + self.compared.append((base, head)) + return _DIFF + def list_comments(self, repo: str, number: int) -> list[dict]: return [] def list_pr_reviews(self, repo: str, number: int) -> list[dict]: return [] - def create_pr_review(self, repo: str, number: int, body: str, inline: list[dict]) -> None: + def create_pr_review( + self, repo: str, number: int, body: str, inline: list[dict], commit_id: str = "" + ) -> None: self.reviews.append((body, inline)) + self.review_commits.append(commit_id) def comment(self, repo: str, number: int, body: str) -> None: self.comments.append(body) @@ -289,9 +299,49 @@ def test_emit_mode_writes_findings_and_posts_nothing(tmp_path: Path) -> None: envelope = json.loads(out.read_text()) assert envelope["repo"] == "org/repo" and envelope["number"] == 7 assert envelope["kind"] == "findings" + assert envelope["head_sha"] == "deadbeef1234" # the head it reviewed assert envelope["data"]["findings"][0]["file"] == "models/encoder.py" +def test_moved_head_skips_before_the_session(tmp_path: Path, monkeypatch: Any) -> None: + # the fork pushed between the label and this run: review exactly the head + # the maintainer saw, or nothing + monkeypatch.setenv("REVIEW_HEAD_SHA", "cafebabe9999") + client, harness = _Client(), _Harness(_FINDINGS) + out = tmp_path / "findings.json" + label = run_agent_review( + client, # type: ignore[arg-type] + "org/repo", + 7, + harness, + _WORKSPACE, + bot_login="autoresearch-bot", + emit_path=out, + ) + assert label is None + assert harness.briefs == [] # no model session ran + envelope = json.loads(out.read_text()) + assert envelope["kind"] == "skip-clean" + assert "cafebabe" in envelope["detail"] and "deadbeef" in envelope["detail"] + + +def test_unset_head_sha_reviews_as_before(tmp_path: Path, monkeypatch: Any) -> None: + monkeypatch.delenv("REVIEW_HEAD_SHA", raising=False) + client, harness = _Client(), _Harness(_FINDINGS) + out = tmp_path / "findings.json" + label = run_agent_review( + client, # type: ignore[arg-type] + "org/repo", + 7, + harness, + _WORKSPACE, + bot_login="autoresearch-bot", + emit_path=out, + ) + assert label == "emitted" + assert json.loads(out.read_text())["kind"] == "findings" + + def test_emit_mode_outage_writes_skip_stub_marker(tmp_path: Path) -> None: client = _Client() harness = _Harness("", is_error=True, detail="rate_limit_error: slow down") @@ -404,6 +454,58 @@ def test_post_from_file_posts_with_opinion_label(tmp_path: Path) -> None: assert any(c.get("path") == "models/encoder.py" for c in inline) +def test_post_refuses_a_stale_envelope_head(tmp_path: Path) -> None: + # the PR moved after the session read it: the write side must not stamp + # findings onto a head nobody reviewed + from outerloop.review_post_cli import post_from_file + + client = _Client() + path = _findings_envelope(tmp_path, head_sha="0123456789abcdef") + assert post_from_file(client, "org/repo", 7, "autoresearch-bot", path) is None # type: ignore[arg-type] + assert client.reviews == [] and client.comments == [] + + +def test_post_pins_the_review_to_the_reviewed_head(tmp_path: Path) -> None: + # a push between this check and the API call must not move the review + from outerloop.review_post_cli import post_from_file + + client = _Client() + path = _findings_envelope(tmp_path, head_sha="deadbeef1234") + post_from_file(client, "org/repo", 7, "autoresearch-bot", path) # type: ignore[arg-type] + assert client.review_commits == ["deadbeef1234"] + + +def test_pinned_review_reads_the_diff_between_fixed_commits( + tmp_path: Path, monkeypatch: Any +) -> None: + # the PR diff endpoint follows the live head; a push after the metadata + # read must not swap in another head's diff + monkeypatch.setenv("REVIEW_HEAD_SHA", "deadbeef1234") + client, harness = _Client(), _Harness(_FINDINGS) + run_agent_review( + client, # type: ignore[arg-type] + "org/repo", + 7, + harness, + _WORKSPACE, + bot_login="autoresearch-bot", + emit_path=tmp_path / "findings.json", + ) + assert client.compared == [("0badc0de0000", "deadbeef1234")] + + +def test_post_refuses_when_the_event_head_moved(tmp_path: Path, monkeypatch: Any) -> None: + # an envelope from an older ref carries no head_sha; the event's head is + # still checked against the PR's current head + from outerloop.review_post_cli import post_from_file + + monkeypatch.setenv("REVIEW_HEAD_SHA", "cafebabe9999") + client = _Client() + path = _findings_envelope(tmp_path) + assert post_from_file(client, "org/repo", 7, "autoresearch-bot", path) is None # type: ignore[arg-type] + assert client.reviews == [] and client.comments == [] + + def test_post_from_file_refuses_wrong_pr_envelope(tmp_path: Path) -> None: from outerloop.review_post_cli import post_from_file diff --git a/tests/test_review_summarize.py b/tests/test_review_summarize.py index b59f85c2..8476357c 100644 --- a/tests/test_review_summarize.py +++ b/tests/test_review_summarize.py @@ -66,13 +66,34 @@ def test_single_real_opinion_passes_through_without_a_session(tmp_path, monkeypa # no model call: the one real opinion is the round — and the FAILED # sibling lenses still reach the posted notes (never hidden by a lone # success) - _envelope(tmp_path, "a", kind="findings", data={"findings": []}, lens="credentials") + _envelope( + tmp_path, + "a", + kind="findings", + data={"findings": []}, + lens="credentials", + head_sha="deadbeef1234", + ) _envelope(tmp_path, "b", kind="skip-stub", detail="key missing", lens="coverage") merged = _run_summarize(tmp_path, monkeypatch) assert merged["kind"] == "findings" and merged["lens"] == "credentials" + assert merged["head_sha"] == "deadbeef1234" # the common head rides through assert "did NOT run" in merged["data"]["notes"] and "coverage" in merged["data"]["notes"] +def test_disagreeing_lens_heads_emit_a_clean_skip(tmp_path, monkeypatch) -> None: + # lenses read different heads: there is no one head to stamp the round on + _envelope( + tmp_path, "a", kind="findings", data={"findings": []}, lens="credentials", head_sha="a" * 40 + ) + _envelope( + tmp_path, "b", kind="findings", data={"findings": []}, lens="deployment", head_sha="b" * 40 + ) + merged = _run_summarize(tmp_path, monkeypatch) + assert merged["kind"] == "skip-clean" + assert "disagree on head_sha" in merged["detail"] + + def test_vanished_lens_is_reported_not_silently_omitted(tmp_path, monkeypatch) -> None: # a lens that crashed before uploading ANY envelope: the caller-declared # expected panel makes the delta visible in the merged notes From 23587e302ceda9197eb9ca1ed3c60861e831c125 Mon Sep 17 00:00:00 2001 From: Mengye Ren Date: Fri, 23 Oct 2026 01:12:49 -0400 Subject: [PATCH 2/3] Pinned review: no fallback comment once the head moved; unheaded lens findings cannot be merged under a pinned head; shorter changelog entry --- CHANGELOG.md | 2 +- src/outerloop/posting.py | 13 +++++++++++++ src/outerloop/review_summarize_cli.py | 9 ++++++--- tests/test_posting.py | 27 +++++++++++++++++++++++++++ tests/test_review_summarize.py | 8 +++++--- 5 files changed, 52 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c86b8b8f..c3dd15ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ Versions follow [SemVer](https://semver.org). ## [Unreleased] -- The advisory review covers exactly the head its triggering event named (#462): with `REVIEW_HEAD_SHA` set, the session skips clean, running no model, if the PR head has moved, and otherwise reads the diff between the base and that head as fixed commits; every envelope carries the reviewed `head_sha`; the posting job posts nothing when the envelope's or the event's head differs from the PR's current head, and attaches the review to the reviewed commit (`commit_id`), so a push during posting cannot move it. The summarizer carries the lenses' common `head_sha` through and skips clean if they disagree. With `REVIEW_CHECKOUT_BASE=1` (a fork PR, whose code is not checked out) the brief says the tree is the base branch and the change is only in the diff. Upgrading: envelopes without `head_sha` (from an older reviewer ref) still post; callers that do not pass REVIEW_HEAD_SHA behave as before. +- The advisory review covers exactly the head its triggering event named (`REVIEW_HEAD_SHA`, #462). If the PR has moved off that head, nothing is reviewed or posted. Otherwise the diff is read between fixed commits, every envelope records the reviewed `head_sha`, and the review is attached to that commit, with no fallback comment once the head has moved. The summarizer skips clean when its lenses read different heads. For a fork PR (`REVIEW_CHECKOUT_BASE=1`) the brief says the tree is the base branch and the change is only in the diff. Upgrading: envelopes without `head_sha` (from an older reviewer ref) still post; callers that do not pass REVIEW_HEAD_SHA behave as before. - When check runs cannot be read on a token deployment, the warning now says the token cannot read them (fine-grained tokens have no checks permission) instead of asking for an App permission. ## [0.5.0] - 2026-10-23 diff --git a/src/outerloop/posting.py b/src/outerloop/posting.py index f784fdbe..4f970c89 100644 --- a/src/outerloop/posting.py +++ b/src/outerloop/posting.py @@ -179,6 +179,19 @@ def post_round_review( commit_id=commit_id, ) except EXPECTED_FAILURES as exc: + if commit_id: + # a push can be why the pinned review was refused; an unpinned + # comment would land the old findings on the new head + head = client.get_pull_request(repo, number).get("head") + current = str(head.get("sha", "")) if isinstance(head, dict) else "" + if current != commit_id: + log.warning( + "pinned review failed (%s) and the head moved (%s -> %s); nothing posted", + exc, + commit_id[:8], + current[:8] or "unknown", + ) + return "" log.warning("inline review failed (%s); falling back to a comment", exc) client.comment(repo, number, fallback_body.replace(marker, f"{marker}\n{stamp}", 1)) return round_label diff --git a/src/outerloop/review_summarize_cli.py b/src/outerloop/review_summarize_cli.py index f7822fca..e2f8e088 100644 --- a/src/outerloop/review_summarize_cli.py +++ b/src/outerloop/review_summarize_cli.py @@ -84,10 +84,13 @@ def main() -> int: envelopes = _load_envelopes(Path(src).resolve(), repo, number) # Lens sessions must all have read the same head; a panel split across - # two heads cannot be merged into one round (empty means an older ref). + # two heads cannot be merged into one round. Findings without a head (an + # older ref) cannot be shown to share it; workflow backstop stubs carry none. heads = {str(e.get("head_sha") or "") for e in envelopes} - {""} - if len(heads) > 1: - detail = "lens envelopes disagree on head_sha: " + ", ".join(sorted(h[:8] for h in heads)) + unheaded = any(e.get("kind") == "findings" and not e.get("head_sha") for e in envelopes) + if len(heads) > 1 or (heads and unheaded): + named = sorted(h[:8] for h in heads) + (["unrecorded"] if unheaded else []) + detail = "lens envelopes disagree on head_sha: " + ", ".join(named) log.warning("%s; nothing to merge", detail) emit_envelope(emit_path, repo, number, kind="skip-clean", detail=detail) return 0 diff --git a/tests/test_posting.py b/tests/test_posting.py index 0f732c25..1be80ddb 100644 --- a/tests/test_posting.py +++ b/tests/test_posting.py @@ -188,6 +188,33 @@ def create_pr_review( assert posted["kind"] == "comment" and "FULL findings in the fallback" in posted["body"] +def test_pinned_review_has_no_fallback_once_the_head_moved() -> None: + # a push can be why GitHub refused the pinned review; a plain comment + # would put the old findings on the new head + class _Moved(_FakeClient): + def create_pr_review( + self, repo: str, number: int, body: str, comments: Any = None, commit_id: str = "" + ) -> None: + raise GitHubError(422, "/repos/org/repo", "commit_id is not part of the pull request") + + def get_pull_request(self, repo: str, number: int) -> dict: + return {"head": {"sha": "newhead"}} + + client = _Moved() + posting.post_round_review( + client, # type: ignore[arg-type] + "org/repo", + 1, + _MARKER, + f"{_MARKER}\nsummary only", + [], + {"head": {"sha": "oldhead"}}, + fallback_body=f"{_MARKER}\nfindings", + commit_id="oldhead", + ) + assert client.posted == [] + + def test_skip_stub_redacts_the_provided_key_any_provider() -> None: # backend-agnostic: whatever key the caller passes (the harness owns its # own key) is scrubbed before the error text — which can echo request diff --git a/tests/test_review_summarize.py b/tests/test_review_summarize.py index 8476357c..735f2c30 100644 --- a/tests/test_review_summarize.py +++ b/tests/test_review_summarize.py @@ -81,13 +81,15 @@ def test_single_real_opinion_passes_through_without_a_session(tmp_path, monkeypa assert "did NOT run" in merged["data"]["notes"] and "coverage" in merged["data"]["notes"] -def test_disagreeing_lens_heads_emit_a_clean_skip(tmp_path, monkeypatch) -> None: - # lenses read different heads: there is no one head to stamp the round on +@pytest.mark.parametrize("other", ["b" * 40, ""], ids=["another-head", "unrecorded"]) +def test_disagreeing_lens_heads_emit_a_clean_skip(tmp_path, monkeypatch, other) -> None: + # lenses read different heads, or one cannot say which (an older ref): + # there is no one head to stamp the round on _envelope( tmp_path, "a", kind="findings", data={"findings": []}, lens="credentials", head_sha="a" * 40 ) _envelope( - tmp_path, "b", kind="findings", data={"findings": []}, lens="deployment", head_sha="b" * 40 + tmp_path, "b", kind="findings", data={"findings": []}, lens="deployment", head_sha=other ) merged = _run_summarize(tmp_path, monkeypatch) assert merged["kind"] == "skip-clean" From 3f09e11b1a061fcacddc4209df028572559c3241 Mon Sep 17 00:00:00 2001 From: Mengye Ren Date: Fri, 23 Oct 2026 09:15:40 -0400 Subject: [PATCH 3/3] Reviewer workflows: allow_labeled_forks input; fork PRs check out the base commit; REVIEW_HEAD_SHA to both jobs --- .github/workflows/advisory-review-agent.yml | 42 +++++++++++++++---- .../workflows/advisory-review-summarize.yml | 17 +++++++- 2 files changed, 48 insertions(+), 11 deletions(-) diff --git a/.github/workflows/advisory-review-agent.yml b/.github/workflows/advisory-review-agent.yml index 2f25cb59..0d3be967 100644 --- a/.github/workflows/advisory-review-agent.yml +++ b/.github/workflows/advisory-review-agent.yml @@ -4,8 +4,9 @@ # another caller job with a different backend/model/opinion label. # # Topology (every backend, claude included): -# session job — READ-ONLY permissions. Checks out the PR head (never -# executed), runs the review session, emits raw findings (or a skip-stub +# session job — READ-ONLY permissions. Checks out the PR head (the base +# commit for a fork; no workflow step runs PR code, though the session +# holds a shell), runs the review session, emits raw findings (or a skip-stub # marker) as an artifact. The only secrets present are the session's own # model key and read-scoped tokens (job token + the reviewer repo's # read-only deploy key while private) — nothing worth stealing, so even @@ -18,7 +19,9 @@ # the round stamp. # # Constraints: never reviews bot-authored PRs, never approves, never fails -# the caller's CI, same-repo PRs only. By design: the "Round N" counter is +# the caller's CI, same-repo PRs only (plus fork PRs a maintainer labels, with +# allow_labeled_forks on; the review then covers the labeled head or nothing). +# By design: the "Round N" counter is # shared across all opinions on a PR (the number is cosmetic; the stamp names # the reviewer), and a missing/expired backend key posts a could-not-run stub # naming its opinion label on each triggering PR until fixed — the noise IS the @@ -28,6 +31,17 @@ name: advisory-review-agent on: workflow_call: inputs: + allow_labeled_forks: + description: >- + Review a fork PR when the triggering event is a label being applied. + Applying a label needs triage access, so the label is the maintainer's + approval; filter the label name in the caller's `if:`. The fork's head + is never checked out (the session reads the base tree plus the diff), + but its diff reaches the model, and its key, through the prompt. Needs + a reviewer_ref that pins the labeled head (0.5.1 or later). + type: boolean + required: false + default: false bot_login: description: Login of your bot account — its PRs are never reviewed. Required. type: string @@ -153,9 +167,13 @@ jobs: continue-on-error: true # Backstop a hung session so it can't burn the 360-min default runner budget. timeout-minutes: 70 - # Fork PRs must not reach any secret. `pull_request_target` runs in the - # base repo's context, so this gate is what closes the pwn-request hole. - if: github.event.pull_request.head.repo.full_name == github.repository + # Fork PRs must not reach any secret unasked. `pull_request_target` runs in + # the base repo's context, so this gate is what closes the pwn-request hole; + # a maintainer's label opens it per PR when the caller opts in. A fork's + # code is never checked out (the session gets the base tree and the diff). + if: >- + github.event.pull_request.head.repo.full_name == github.repository || + (inputs.allow_labeled_forks && github.event.action == 'labeled') # READ-ONLY: the session runs here, so the job token must not be worth # stealing. pull-requests:read lets the CLI fetch the PR metadata/diff. permissions: @@ -163,6 +181,9 @@ jobs: pull-requests: read env: PR_NUMBER: ${{ github.event.pull_request.number }} + # the head the triggering event saw; the review covers it or nothing + REVIEW_HEAD_SHA: ${{ github.event.pull_request.head.sha }} + REVIEW_CHECKOUT_BASE: ${{ github.event.pull_request.head.repo.full_name != github.repository && '1' || '' }} BACKEND: ${{ inputs.backend }} steps: # Checks out the reviewer, never the pull request's code for execution. @@ -176,10 +197,10 @@ jobs: ssh-key: ${{ secrets.checkout_ssh_key }} path: .autoresearch persist-credentials: false - - name: Check out the PR head (read-only; never executed) + - name: Check out the PR head, or the base for a fork (read-only) uses: actions/checkout@v7 with: - ref: refs/pull/${{ env.PR_NUMBER }}/head + ref: ${{ env.REVIEW_CHECKOUT_BASE == '1' && github.event.pull_request.base.sha || env.REVIEW_HEAD_SHA }} path: pr-head persist-credentials: false - uses: astral-sh/setup-uv@v10.3.0 @@ -291,7 +312,9 @@ jobs: needs: session # inputs.post=false -> emit-only: a lens opinion whose artifact the # summarizer merges; nothing is posted from this call. - if: ${{ inputs.post && !cancelled() && github.event.pull_request.head.repo.full_name == github.repository }} + if: >- + ${{ inputs.post && !cancelled() && (github.event.pull_request.head.repo.full_name == github.repository || + (inputs.allow_labeled_forks && github.event.action == 'labeled')) }} continue-on-error: true timeout-minutes: 15 # The write half: no model session runs in this job. @@ -300,6 +323,7 @@ jobs: pull-requests: write env: PR_NUMBER: ${{ github.event.pull_request.number }} + REVIEW_HEAD_SHA: ${{ github.event.pull_request.head.sha }} steps: - name: Check out the reviewer uses: actions/checkout@v7 diff --git a/.github/workflows/advisory-review-summarize.yml b/.github/workflows/advisory-review-summarize.yml index 6a3d0931..9c632e56 100644 --- a/.github/workflows/advisory-review-summarize.yml +++ b/.github/workflows/advisory-review-summarize.yml @@ -9,6 +9,13 @@ name: advisory-review-summarize on: workflow_call: inputs: + allow_labeled_forks: + description: >- + Summarize a fork PR when the triggering event is a label being applied + (set it the same as on the lens calls). + type: boolean + required: false + default: false bot_login: description: Login of your bot account — its PRs are never reviewed. Required. type: string @@ -81,7 +88,9 @@ jobs: runs-on: ubuntu-latest continue-on-error: true timeout-minutes: 30 - if: github.event.pull_request.head.repo.full_name == github.repository + if: >- + github.event.pull_request.head.repo.full_name == github.repository || + (inputs.allow_labeled_forks && github.event.action == 'labeled') # READ-ONLY, like the reviewer session job: the merge session must not # run next to a write token. permissions: @@ -89,6 +98,7 @@ jobs: pull-requests: read env: PR_NUMBER: ${{ github.event.pull_request.number }} + REVIEW_HEAD_SHA: ${{ github.event.pull_request.head.sha }} BACKEND: ${{ inputs.backend }} steps: - name: Check out the reviewer @@ -194,7 +204,9 @@ jobs: post: runs-on: ubuntu-latest needs: summarize - if: ${{ !cancelled() && github.event.pull_request.head.repo.full_name == github.repository }} + if: >- + ${{ !cancelled() && (github.event.pull_request.head.repo.full_name == github.repository || + (inputs.allow_labeled_forks && github.event.action == 'labeled')) }} continue-on-error: true timeout-minutes: 15 permissions: @@ -202,6 +214,7 @@ jobs: pull-requests: write env: PR_NUMBER: ${{ github.event.pull_request.number }} + REVIEW_HEAD_SHA: ${{ github.event.pull_request.head.sha }} steps: - name: Check out the reviewer uses: actions/checkout@v7