From 1d4f1aed33ea0f3c1381ff58fe9a027831822db6 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Wed, 3 Jun 2026 11:38:56 -0700 Subject: [PATCH] fix: reviewer publishes against stale head_sha on mid-run re-review (#1393) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: resolve reviewer head_sha from thread metadata, not frozen run config A push that lands while a reviewer run is in flight is delivered as a queued message into that run. The run's configurable is frozen at creation, so its head_sha still names the commit the run was created for — not the commit just pushed. publish_review then anchored the GitHub review to the stale commit and regressed last_reviewed_sha to it, and add_finding/update_finding stamped findings with the stale SHA. Persist the current head in thread metadata at every reviewer dispatch (both the ready-for-review and push paths, before they branch to create a run or queue a message), and add resolve_review_head_sha() which prefers the metadata head over the run config. Wire it into publish_review (review commit_id + last_reviewed_sha), add_finding (first_seen_sha) and update_finding (last_confirmed_sha). Falls back to the run config when metadata carries no head (first review, eval, tests). * fix: persist head_sha in manual review dispatch (trigger_pr_review_from_ref) resolve_review_head_sha prefers metadata[head_sha] over the run config, and the push/ready dispatchers write it — but trigger_pr_review_from_ref (Slack/GitHub @open-swe review, request_pr_review tool) created a run with a freshly-fetched config head while leaving metadata's head stale from a prior dispatch. A manual re-review at a newer commit would then resolve to the old head and publish/advance findings against it. Persist head_sha in that dispatch's metadata write too, so every run-creating reviewer dispatch keeps metadata in sync with the head its run targets. Caught by the Open SWE reviewer on this PR. --- agent/reviewer_findings.py | 26 ++++++++++++++ agent/tools/add_finding.py | 8 +++-- agent/tools/publish_review.py | 6 ++++ agent/tools/update_finding.py | 8 +++-- agent/webapp.py | 6 ++-- tests/test_github_issue_webhook.py | 9 +++-- tests/test_pr_ready_auto_review.py | 6 ++++ tests/test_reviewer_findings.py | 39 ++++++++++++++++++++ tests/test_reviewer_publish.py | 56 +++++++++++++++++++++++++++++ tests/test_reviewer_tools.py | 57 ++++++++++++++++++++++++++++++ tests/test_reviewer_watch.py | 10 +++++- 11 files changed, 219 insertions(+), 12 deletions(-) diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py index 8972b7d3..0973d487 100644 --- a/agent/reviewer_findings.py +++ b/agent/reviewer_findings.py @@ -294,6 +294,24 @@ async def get_thread_metadata(thread_id: str) -> dict[str, Any]: return metadata if isinstance(metadata, dict) else {} +async def resolve_review_head_sha(thread_id: str, configurable: dict[str, Any]) -> str: + """Return the current PR head SHA for a reviewer run. + + A push that lands while a reviewer run is in flight is delivered as a queued + message into that run, whose frozen ``configurable`` still names the head the + run was created for. The dispatching webhook records the current head in + thread metadata, so prefer that; fall back to the run's config when metadata + carries no head (first review, eval, tests). + """ + config_head = configurable.get("head_sha") if isinstance(configurable, dict) else None + config_head = config_head if isinstance(config_head, str) else "" + if not thread_id: + return config_head + metadata = await get_thread_metadata(thread_id) + meta_head = metadata.get("head_sha") + return meta_head if isinstance(meta_head, str) and meta_head else config_head + + async def list_findings(thread_id: str) -> list[Finding]: """Return all findings persisted on the reviewer thread.""" metadata = await get_thread_metadata(thread_id) @@ -438,6 +456,7 @@ async def set_reviewer_thread_metadata( *, pr: ReviewerPRMeta | None = None, last_reviewed_sha: str | None = None, + head_sha: str | None = None, watch: bool | None = None, findings: list[Finding] | None = None, slack_thread: ReviewerSlackThread | None = None, @@ -448,6 +467,11 @@ async def set_reviewer_thread_metadata( Always sets ``kind=reviewer`` so the future UI can list reviewer threads by filtering on metadata. Only includes the fields the caller passed in (langgraph metadata updates merge rather than overwrite). + + ``head_sha`` records the current PR head the dispatching webhook is acting + on. A push that lands mid-run is queued into the still-running run, whose + frozen config can't be updated; persisting the head here lets the reviewer + tools resolve the live head via ``resolve_review_head_sha``. """ client = get_client() metadata: dict[str, Any] = {"kind": REVIEWER_THREAD_KIND} @@ -455,6 +479,8 @@ async def set_reviewer_thread_metadata( metadata["pr"] = pr if last_reviewed_sha is not None: metadata["last_reviewed_sha"] = last_reviewed_sha + if head_sha is not None: + metadata["head_sha"] = head_sha if watch is not None: metadata["watch"] = watch if findings is not None: diff --git a/agent/tools/add_finding.py b/agent/tools/add_finding.py index b1af3ebe..ba08880b 100644 --- a/agent/tools/add_finding.py +++ b/agent/tools/add_finding.py @@ -20,6 +20,7 @@ from ..reviewer_findings import ( get_thread_id_from_runtime, new_finding, normalize_finding_title, + resolve_review_head_sha, ) @@ -112,7 +113,6 @@ def add_finding( config = get_config() configurable = config.get("configurable", {}) if isinstance(config, dict) else {} diff_line_set = configurable.get("diff_line_set") if isinstance(configurable, dict) else None - head_sha = configurable.get("head_sha", "") if isinstance(configurable, dict) else "" diff_text = configurable.get("diff_text", "") if isinstance(configurable, dict) else "" if isinstance(diff_line_set, dict) and not is_range_in_diff( @@ -134,6 +134,9 @@ def add_finding( clipped_suggestion, suggestion_dropped = clip_suggestion(suggestion) + thread_id = get_thread_id_from_runtime() + head_sha = asyncio.run(resolve_review_head_sha(thread_id, configurable)) + finding: Finding = new_finding( severity=_cast_severity(severity), confidence=_cast_confidence(confidence), @@ -142,14 +145,13 @@ def add_finding( start_line=start_line, end_line=end_line, description=description, - sha=str(head_sha) if isinstance(head_sha, str) else "", + sha=head_sha, title=normalized_title, side=_cast_side(side), suggestion=clipped_suggestion, diff_hunk=diff_hunk, ) - thread_id = get_thread_id_from_runtime() asyncio.run(append_finding(thread_id, finding)) result: dict[str, Any] = {"success": True, "finding_id": finding["id"]} if suggestion_dropped: diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index c465f95f..69cf8dda 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -18,6 +18,7 @@ from ..reviewer_findings import ( get_thread_metadata, get_thread_slack_ref, replace_findings, + resolve_review_head_sha, set_reviewer_thread_metadata, ) from ..reviewer_findings import ( @@ -202,6 +203,11 @@ async def _publish_review_async( trace_link_config_override: object = None, ) -> dict[str, Any]: thread_id = get_thread_id_from_runtime() + # The run config's head_sha is frozen at run creation; a push that arrived + # mid-run updated the live head in thread metadata. Prefer that so the + # review anchors to (and last_reviewed_sha advances to) the commit actually + # reviewed, not the stale one this run was created for. + head_sha = await resolve_review_head_sha(thread_id, {"head_sha": head_sha}) review_trace_url = await _resolve_review_trace_url(thread_id, trace_link_config_override) findings = await _backfill_findings_from_pr_threads( thread_id=thread_id, diff --git a/agent/tools/update_finding.py b/agent/tools/update_finding.py index b6048c35..1d66a75f 100644 --- a/agent/tools/update_finding.py +++ b/agent/tools/update_finding.py @@ -15,6 +15,7 @@ from ..reviewer_findings import ( get_thread_id_from_runtime, list_findings, normalize_finding_title, + resolve_review_head_sha, update_finding_fields, ) from ..utils.reviewer_outcomes import emit_finding_status_outcome @@ -127,9 +128,10 @@ def update_finding( config = get_config() configurable = config.get("configurable", {}) if isinstance(config, dict) else {} - head_sha = configurable.get("head_sha", "") if isinstance(configurable, dict) else "" - if status == "open" and isinstance(head_sha, str) and head_sha: - updates["last_confirmed_sha"] = head_sha + if status == "open": + head_sha = asyncio.run(resolve_review_head_sha(get_thread_id_from_runtime(), configurable)) + if head_sha: + updates["last_confirmed_sha"] = head_sha if not updates: if suggestion_dropped: diff --git a/agent/webapp.py b/agent/webapp.py index 4256d4e7..0f6825a8 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -1781,7 +1781,7 @@ async def trigger_pr_review_from_ref( "thread_ts": slack_thread_ts, } await set_reviewer_thread_metadata( - thread_id, pr=pr_meta, watch=True, slack_thread=slack_thread_meta + thread_id, pr=pr_meta, watch=True, slack_thread=slack_thread_meta, head_sha=head_sha ) prompt = build_github_pr_review_prompt(repo_config, pr_ref.number, pr_url, base_sha, head_sha) @@ -1959,7 +1959,7 @@ async def _dispatch_first_review_from_pr_payload(payload: dict[str, Any], *, sou logger.warning("Could not persist bot token for reviewer thread %s", thread_id) return - await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True) + await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True, head_sha=head_sha) is_re_review = bool(last_reviewed_sha) if is_re_review: @@ -2400,7 +2400,7 @@ async def process_github_push_event(payload: dict[str, Any]) -> None: "head_ref": head_ref, "base_ref": base_ref, } - await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True) + await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True, head_sha=head_sha) re_review_prompt = ( f"A new commit has been pushed to PR #{pr_number}. The new HEAD is " diff --git a/tests/test_github_issue_webhook.py b/tests/test_github_issue_webhook.py index bbabccd2..0906a190 100644 --- a/tests/test_github_issue_webhook.py +++ b/tests/test_github_issue_webhook.py @@ -911,8 +911,9 @@ def test_process_github_pr_review_request_creates_reviewer_run(monkeypatch) -> N runs = _FakeRunsClient() threads = _FakeThreadsClient() - async def fake_set_reviewer_thread_metadata(thread_id: str, **_kwargs: object) -> None: + async def fake_set_reviewer_thread_metadata(thread_id: str, **kwargs: object) -> None: captured["set_metadata_thread_id"] = thread_id + captured["set_metadata_kwargs"] = kwargs monkeypatch.setattr( webapp, "get_github_app_installation_token", fake_get_github_app_installation_token @@ -1011,8 +1012,9 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: runs = _FakeRunsClient() threads = _FakeThreadsClient() - async def fake_set_reviewer_thread_metadata(thread_id: str, **_kwargs: object) -> None: + async def fake_set_reviewer_thread_metadata(thread_id: str, **kwargs: object) -> None: captured["set_metadata_thread_id"] = thread_id + captured["set_metadata_kwargs"] = kwargs monkeypatch.setattr( webapp, "get_github_app_installation_token", fake_get_github_app_installation_token @@ -1065,6 +1067,9 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: "channel_id": "C123", "thread_ts": "1700000000.000100", } + # The live head must be persisted to metadata so resolve_review_head_sha + # doesn't return a stale head left by a prior push/ready dispatch. + assert captured["set_metadata_kwargs"]["head_sha"] == "head-sha" def test_trigger_pr_review_from_ref_respects_dashboard_opt_in(monkeypatch) -> None: diff --git a/tests/test_pr_ready_auto_review.py b/tests/test_pr_ready_auto_review.py index ae1dcc97..a3fbbc4a 100644 --- a/tests/test_pr_ready_auto_review.py +++ b/tests/test_pr_ready_auto_review.py @@ -191,6 +191,12 @@ async def test_pr_ready_for_review_uses_re_review_after_previous_review( assert configurable["last_reviewed_sha"] == "oldsha" assert configurable["head_sha"] == "headsha" assert "marked ready for review" in kwargs["input"]["messages"][0]["content"] + head_sha_writes = [ + c.kwargs.get("head_sha") + for c in webapp.set_reviewer_thread_metadata.await_args_list + if c.kwargs.get("head_sha") is not None + ] + assert "headsha" in head_sha_writes @pytest.mark.asyncio diff --git a/tests/test_reviewer_findings.py b/tests/test_reviewer_findings.py index 30c04bc1..70e550b1 100644 --- a/tests/test_reviewer_findings.py +++ b/tests/test_reviewer_findings.py @@ -16,6 +16,7 @@ from agent.reviewer_findings import ( new_finding, new_finding_id, replace_findings, + resolve_review_head_sha, set_reviewer_thread_metadata, update_finding_fields, ) @@ -181,3 +182,41 @@ async def test_set_reviewer_thread_metadata_includes_kind() -> None: assert metadata["last_reviewed_sha"] == "sha" assert "pr" not in metadata assert "findings" not in metadata + + +@pytest.mark.asyncio +async def test_set_reviewer_thread_metadata_persists_head_sha() -> None: + fake_client = AsyncMock() + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + await set_reviewer_thread_metadata("tid", head_sha="newhead") + metadata = fake_client.threads.update.await_args.kwargs["metadata"] + assert metadata["head_sha"] == "newhead" + + +@pytest.mark.asyncio +async def test_resolve_review_head_sha_prefers_metadata_over_config() -> None: + """A mid-run push records the live head in thread metadata; it must win over + the stale head frozen in the run's config.""" + fake_client = AsyncMock() + fake_client.threads.get.return_value = {"metadata": {"head_sha": "metahead"}} + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + head = await resolve_review_head_sha("tid", {"head_sha": "confighead"}) + assert head == "metahead" + + +@pytest.mark.asyncio +async def test_resolve_review_head_sha_falls_back_to_config_when_metadata_empty() -> None: + fake_client = AsyncMock() + fake_client.threads.get.return_value = {"metadata": {}} + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + head = await resolve_review_head_sha("tid", {"head_sha": "confighead"}) + assert head == "confighead" + + +@pytest.mark.asyncio +async def test_resolve_review_head_sha_falls_back_without_thread_id() -> None: + fake_client = AsyncMock() + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + head = await resolve_review_head_sha("", {"head_sha": "confighead"}) + assert head == "confighead" + fake_client.threads.get.assert_not_called() diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 72297874..95490433 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -45,6 +45,12 @@ def _isolate_publish_review_pr_state() -> Iterator[None]: patch("agent.tools.publish_review.fetch_pr_review_threads", AsyncMock(return_value=[])), patch("agent.tools.publish_review.replace_findings", AsyncMock()), patch("agent.tools.publish_review.open_swe_review_exists", AsyncMock(return_value=False)), + patch( + "agent.tools.publish_review.resolve_review_head_sha", + AsyncMock( + side_effect=lambda thread_id, configurable: configurable.get("head_sha") or "" + ), + ), ): yield @@ -562,6 +568,56 @@ async def test_publish_review_skips_duplicate_empty_summary_when_open_swe_alread set_metadata.assert_awaited_once_with("tid", last_reviewed_sha="newsha") +@pytest.mark.asyncio +async def test_publish_review_uses_resolved_head_sha_for_commit_and_last_reviewed() -> None: + """A push that landed mid-run updates the live head in thread metadata. + publish_review must anchor the GitHub review to that head and advance + last_reviewed_sha to it, not the stale head frozen in the run config.""" + from agent.tools.publish_review import _publish_review_async + + finding = _f(id="f_new", file="b.py", start_line=2, end_line=2) + post_review = AsyncMock(return_value={"id": 4242}) + set_metadata = AsyncMock() + + with ( + patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"), + patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=[finding])), + patch( + "agent.tools.publish_review.resolve_review_head_sha", + AsyncMock(return_value="freshhead"), + ), + patch("agent.tools.publish_review.post_pull_request_review", post_review), + patch("agent.tools.publish_review.fetch_review_comments", AsyncMock(return_value=[])), + patch( + "agent.tools.publish_review._resolve_threads_for_resolved_findings", + new_callable=AsyncMock, + return_value=0, + ), + patch("agent.tools.publish_review.set_reviewer_thread_metadata", set_metadata), + patch( + "agent.tools.publish_review._maybe_post_slack_completion_reply", + new_callable=AsyncMock, + ), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="stalehead", + token="t", + severity_threshold="medium", + cap=15, + is_re_review=False, + langgraph_run_id="run-x", + ) + + assert result["success"] is True + assert post_review.await_args.kwargs["head_sha"] == "freshhead" + final = set_metadata.await_args_list[-1] + assert final.args[0] == "tid" + assert final.kwargs["last_reviewed_sha"] == "freshhead" + + @pytest.mark.asyncio async def test_publish_review_skips_review_existence_check_on_re_review() -> None: """When re_review is already True we know a prior review exists, so the diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index 45d6a07e..e0d74cf0 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -2,15 +2,38 @@ from __future__ import annotations +from collections.abc import Iterator from typing import Any from unittest.mock import AsyncMock, patch +import pytest + from agent.tools.add_finding import add_finding from agent.tools.list_findings import list_findings from agent.tools.resolve_finding_thread import resolve_finding_thread from agent.tools.update_finding import update_finding +@pytest.fixture(autouse=True) +def _stub_resolve_review_head_sha() -> Iterator[None]: + """Resolve the review head from the run config (no thread-metadata fetch). + + Mirrors the production fallback when metadata carries no head, keeping these + unit tests offline. Tests that exercise the metadata-override path patch + ``resolve_review_head_sha`` themselves. + """ + + def _head(thread_id: str, configurable: dict[str, Any]) -> str: + head = configurable.get("head_sha") + return head if isinstance(head, str) else "" + + with ( + patch("agent.tools.add_finding.resolve_review_head_sha", AsyncMock(side_effect=_head)), + patch("agent.tools.update_finding.resolve_review_head_sha", AsyncMock(side_effect=_head)), + ): + yield + + def _config(**configurable_overrides: Any) -> dict[str, Any]: base: dict[str, Any] = { "configurable": { @@ -200,6 +223,40 @@ def test_add_finding_persists_to_thread_metadata() -> None: assert persisted["confidence"] == "high" +def test_add_finding_uses_resolved_head_sha_for_provenance() -> None: + """A net-new finding filed during a mid-run re-review must record the live + head (from thread metadata), not the stale head frozen in the run config.""" + captured: list[Any] = [] + + async def fake_append(thread_id: str, finding: Any) -> Any: + captured.append(finding) + return finding + + with ( + patch("agent.tools.add_finding.get_config", return_value=_config()), + patch("agent.tools.add_finding.get_thread_id_from_runtime", return_value="tid-1"), + patch( + "agent.tools.add_finding.resolve_review_head_sha", + AsyncMock(return_value="freshhead"), + ), + patch("agent.tools.add_finding.append_finding", side_effect=fake_append), + ): + result = add_finding( + severity="medium", + confidence="high", + category="style", + file="foo.py", + title="Rename breaks reference", + description="rename", + start_line=11, + end_line=12, + ) + + assert result["success"] is True + assert captured[0]["first_seen_sha"] == "freshhead" + assert captured[0]["last_confirmed_sha"] == "freshhead" + + def test_add_finding_allows_file_level_with_no_lines() -> None: with ( patch("agent.tools.add_finding.get_config", return_value=_config()), diff --git a/tests/test_reviewer_watch.py b/tests/test_reviewer_watch.py index 032bc7ae..3a04ba3b 100644 --- a/tests/test_reviewer_watch.py +++ b/tests/test_reviewer_watch.py @@ -265,7 +265,7 @@ async def test_push_event_triggers_re_review_run_when_watching() -> None: patch( "agent.webapp.set_reviewer_thread_metadata", new_callable=AsyncMock, - ), + ) as set_meta, patch("agent.webapp.is_thread_active", new_callable=AsyncMock, return_value=False), patch("agent.webapp.get_client", return_value=fake_client), ): @@ -278,6 +278,14 @@ async def test_push_event_triggers_re_review_run_when_watching() -> None: assert configurable["re_review"] is True assert configurable["last_reviewed_sha"] == "oldsha" assert configurable["head_sha"] == "newsha" + # The live head is persisted to thread metadata so a re-review queued into + # an in-flight run can resolve it despite the run's frozen config. + head_sha_writes = [ + c.kwargs.get("head_sha") + for c in set_meta.await_args_list + if c.kwargs.get("head_sha") is not None + ] + assert "newsha" in head_sha_writes @pytest.mark.asyncio