diff --git a/CHANGELOG.md b/CHANGELOG.md index c3dd15ae..8e37e654 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 (`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. +- 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. When GitHub refuses its inline comments, the fallback is a body-only review on the same commit, never an unpinned comment (#569). 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 4f970c89..3d656c5b 100644 --- a/src/outerloop/posting.py +++ b/src/outerloop/posting.py @@ -168,8 +168,11 @@ def post_round_review( 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. `commit_id` pins the - review to the head that was reviewed, so a push meanwhile cannot move it.""" + review to the head that was reviewed, so a push meanwhile cannot move it; + its fallback is a body-only review pinned to the same head, never an + unpinned comment.""" stamp, round_label = _round_stamp(client, repo, number, marker, pr_data, reviewed_by) + full = fallback_body.replace(marker, f"{marker}\n{stamp}", 1) try: client.create_pr_review( repo, @@ -179,21 +182,17 @@ 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)) + if not commit_id: + log.warning("inline review failed (%s); falling back to a comment", exc) + client.comment(repo, number, full) + return round_label + log.warning("pinned inline review failed (%s); retrying without inline comments", exc) + try: + client.create_pr_review(repo, number, full, [], commit_id=commit_id) + except EXPECTED_FAILURES as exc2: + # the head left the PR (force-push); nothing is posted + log.warning("pinned review to %s refused (%s); nothing posted", commit_id[:8], exc2) + return "" return round_label diff --git a/tests/test_posting.py b/tests/test_posting.py index 1be80ddb..27c57473 100644 --- a/tests/test_posting.py +++ b/tests/test_posting.py @@ -188,18 +188,41 @@ 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 +def test_pinned_review_falls_back_to_a_body_only_review_on_the_same_head() -> None: + class _NoInline(_FakeClient): + def create_pr_review( + self, repo: str, number: int, body: str, comments: Any = None, commit_id: str = "" + ) -> None: + if comments: + raise GitHubError(422, "/repos/org/repo", "unprocessable") + self.posted.append({"body": body, "kind": "review", "commit_id": commit_id}) + + client = _NoInline() + label = posting.post_round_review( + client, # type: ignore[arg-type] + "org/repo", + 1, + _MARKER, + f"{_MARKER}\nsummary only", + [{"path": "x.py", "line": 1}], + {"head": {"sha": "oldhead"}}, + fallback_body=f"{_MARKER}\nFULL findings", + commit_id="oldhead", + ) + assert label == "**Round 1**" + (posted,) = client.posted + assert posted["kind"] == "review" and posted["commit_id"] == "oldhead" + assert "FULL findings" in posted["body"] + + +def test_pinned_review_posts_nothing_once_the_head_left_the_pr() -> None: + # 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]