From 5da3d0c657e355add88fe12b5109c0bd6b47a121 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Fri, 26 Jun 2026 16:08:25 -0700 Subject: [PATCH] fix: post reviewer resolution notes verbatim (#1624) * fix: post reviewer resolution notes verbatim Co-authored-by: open-swe[bot] * fix: stabilize dashboard follow-up e2e Co-authored-by: open-swe[bot] * fix: preserve dashboard attribution in e2e Co-authored-by: open-swe[bot] * fix: make e2e attribution marker durable Co-authored-by: open-swe[bot] * fix: only echo found e2e attribution Co-authored-by: open-swe[bot] * fix: check live dashboard attribution in e2e Co-authored-by: open-swe[bot] --------- Co-authored-by: open-swe[bot] --- agent/reviewer.py | 29 +++++++++++++++------------ agent/reviewer_publish.py | 9 ++------- agent/reviewer_reconcile.py | 14 +++++++------ agent/tools/resolve_finding_thread.py | 2 +- agent/tools/update_finding.py | 5 +++-- tests/e2e/fake_llm.py | 20 +++++++++++++----- tests/e2e/tests/dashboard.spec.ts | 9 ++------- tests/test_reviewer_publish.py | 14 ++++++------- tests/test_reviewer_reconcile.py | 4 ++-- tests/test_reviewer_tools.py | 3 +-- 10 files changed, 57 insertions(+), 52 deletions(-) diff --git a/agent/reviewer.py b/agent/reviewer.py index a893fd4e..a989f8ec 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -138,22 +138,23 @@ Out-of-diff findings are disabled. `add_finding` rejects any finding whose line this PR actually changed. Re-review: for each open finding, `update_finding(id, status="resolved", note="...")` -if fixed (include a brief explanation of the fix in `note`), `update_finding` with +if fixed (write the full GitHub reply body in `note`), `update_finding` with new fields + `note` if changed, otherwise do nothing. Add net-new findings with `add_finding`. -When you mark a finding as resolved, `publish_review` will automatically post a -resolution comment to the GitHub thread explaining what was fixed, then close it. -The `note` field you provide in `update_finding` becomes part of that comment, so -be specific: "The current code at line X now does Y" beats "This is fixed". +When you mark a finding as resolved, `publish_review` will automatically post the +`note` field verbatim to the GitHub thread, then close it. Write the complete +human-facing reply yourself, including any desired status wording; the system does +not prepend "Resolved" or "Dismissed". If a human reply shows one of your published findings is invalid, call `resolve_finding_thread(finding_id, status="dismissed", note="...")` after verifying the claim (the note should explain why). If the finding is fixed by code, use -`update_finding(..., status="resolved", note="...")`. Do NOT use -`reply_to_finding_thread` for resolutions or dismissals — the system posts those -automatically. Use `reply_to_finding_thread` only when the user directly asks a -question or a short clarification is needed after pushback. +`update_finding(..., status="resolved", note="...")`. The note is posted verbatim +as the complete GitHub reply body; include any desired status wording yourself. +Do NOT use `reply_to_finding_thread` for resolutions or dismissals — the system +posts those automatically. Use `reply_to_finding_thread` only when the user +directly asks a question or a short clarification is needed after pushback. # The bar: file a finding only if it passes these criteria @@ -580,11 +581,12 @@ def _build_re_review_context( f'{last_reviewed_sha}...{head_sha} -H "Accept: application/vnd.github.v3.diff"`, ' f"then review only what's in that diff.\n\n" f"For each open finding above, decide whether the new commits resolved " - f'it (`update_finding(id, status="resolved", note="...")`), left it unchanged ' + f'it (`update_finding(id, status="resolved", note="")`), left it unchanged ' f"(no action), or changed it materially (`update_finding` with new " - f"fields + a `note`). If a human reply on a finding explains why your " + f"fields + a full reply-body `note`). If a human reply on a finding explains why your " f"comment was invalid, verify that analysis, then call " f'`resolve_finding_thread(id, status="dismissed", note="...")` to close it. ' + f"The `note` is posted verbatim, so write it as the complete GitHub reply body. " f"Reply only when directly asked or when a concise clarification is " f"necessary. Then add any net-new findings introduced by the " f"new diff — but skip anything already covered by an existing PR " @@ -636,8 +638,9 @@ def _build_finding_reply_context( f"## Existing findings\n\n{existing_findings_block}\n\n" f"{prior_threads_section}" f"Reassess only this finding. If the reply proves the finding is invalid, " - f'call `resolve_finding_thread(id, status="dismissed", note="...")`. If code now ' - f'fixes the finding, call `update_finding(id, status="resolved", note="...")`. ' + f'call `resolve_finding_thread(id, status="dismissed", note="")`. If code now ' + f'fixes the finding, call `update_finding(id, status="resolved", note="")`. ' + f"The `note` is posted verbatim, so write it as the complete GitHub reply body. " f"Use `reply_to_finding_thread` only when the user asked a direct " f"question or a concise clarification is necessary. Call `publish_review` " f"once at the end so pending GitHub thread state is reconciled." diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index 6dac0c11..bae2654c 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -197,13 +197,8 @@ def render_resolution_comment( status: str, note: str | None = None, ) -> str | None: - """Render the agent-provided resolution reply for a review thread.""" - body = _resolution_body(finding, note) - if body is None: - return None - if status == "resolved": - return f"✅ **Resolved**: {body}" - return f"❌ **Dismissed**: {body}" + """Render the agent-provided reply for a review thread.""" + return _resolution_body(finding, note) def _resolution_body(finding: Finding, note: str | None) -> str | None: diff --git a/agent/reviewer_reconcile.py b/agent/reviewer_reconcile.py index f43e93eb..389afa8f 100644 --- a/agent/reviewer_reconcile.py +++ b/agent/reviewer_reconcile.py @@ -163,10 +163,6 @@ def _sync_thread_status(finding: Finding, matches: list[ReviewThreadMatch]) -> b return False updated = False - if finding.get("status") == "open": - finding["status"] = "resolved" - updated = True - resolved_thread_ids = _str_list(finding.get("github_resolved_thread_ids")) all_resolved = True for review_thread, _comment_id in matches: @@ -180,12 +176,18 @@ def _sync_thread_status(finding: Finding, matches: list[ReviewThreadMatch]) -> b if resolved_thread_ids != _str_list(finding.get("github_resolved_thread_ids")): finding["github_resolved_thread_ids"] = resolved_thread_ids - if all_resolved and not finding.get("github_thread_resolved"): + if not all_resolved: + return updated + + if finding.get("status") == "open": + finding["status"] = "resolved" + updated = True + if not finding.get("github_thread_resolved"): finding["github_thread_resolved"] = True updated = True if isinstance(finding.get("id"), str): surface = _coerce_surface(finding, str(finding["id"])) - surface["state"] = "resolved" if all_resolved else "resolve_pending" + surface["state"] = "resolved" finding["surface"] = surface updated = True return updated diff --git a/agent/tools/resolve_finding_thread.py b/agent/tools/resolve_finding_thread.py index 920a4bca..a5156629 100644 --- a/agent/tools/resolve_finding_thread.py +++ b/agent/tools/resolve_finding_thread.py @@ -41,7 +41,7 @@ async def resolve_finding_thread( Use ``status="resolved"`` when the code now fixes the issue. Use ``status="dismissed"`` when analysis shows the original review comment was - not valid. ``note`` is required and becomes the GitHub reply body. + not valid. ``note`` is required and is posted verbatim as the full GitHub reply body. """ if status not in {"resolved", "dismissed"}: return {"success": False, "error": f"Invalid status: {status}"} diff --git a/agent/tools/update_finding.py b/agent/tools/update_finding.py index 59167cfd..8d5d11a5 100644 --- a/agent/tools/update_finding.py +++ b/agent/tools/update_finding.py @@ -71,7 +71,7 @@ async def update_finding( ``Existing findings`` block of the re-review user message). status: New status (``open``, ``resolved``, ``dismissed``). Use ``resolved`` when the new commits address the issue. Resolving - or dismissing requires a ``note`` with the message to post. + or dismissing requires a ``note`` with the full message to post. severity: New severity, if reassessing. confidence: New confidence rating (``low``, ``medium``, ``high``), if new commits change how sure you are the finding is a real issue. @@ -82,7 +82,8 @@ async def update_finding( Capped at 4 lines — longer values are dropped (the finding keeps its description). Only set this for small, obvious fixes. note: Optional free-form note explaining the change. Required when - resolving or dismissing because it becomes the GitHub reply body. + resolving or dismissing because it is posted verbatim as the full + GitHub reply body. Returns: Dictionary with ``success`` and (on success) the updated ``finding``. diff --git a/tests/e2e/fake_llm.py b/tests/e2e/fake_llm.py index 21dfe5d4..9ea10e99 100644 --- a/tests/e2e/fake_llm.py +++ b/tests/e2e/fake_llm.py @@ -226,12 +226,22 @@ def build_plan_script() -> list[Any]: FOLLOW_UP_REPLY = "Thanks! The PR is ready for review — anything else you'd like changed?" +_ATTRIBUTION_RE = re.compile(r"@([A-Za-z0-9-]+):") -def _step_followup(_messages: list[BaseMessage]) -> AIMessage: - # A web/Slack follow-up after the PR exists: a plain reply, no new PR. Its - # content lands in the thread transcript the dashboard renders. - return AIMessage(content=FOLLOW_UP_REPLY) +def _latest_attribution(messages: list[BaseMessage]) -> str | None: + for msg in reversed(messages): + if isinstance(msg, HumanMessage): + match = _ATTRIBUTION_RE.search(_text(msg.content)) + if match: + return f"@{match.group(1)}" + return None + + +def _step_followup(messages: list[BaseMessage]) -> AIMessage: + attribution = _latest_attribution(messages) + suffix = f" I saw this follow-up was from {attribution}." if attribution else "" + return AIMessage(content=f"{FOLLOW_UP_REPLY}{suffix}") def build_script() -> list[Any]: @@ -285,7 +295,7 @@ class FakeScriptedChatModel(BaseChatModel): if step < len(script): message = script[step](messages) else: - message = AIMessage(content="All set — let me know if you'd like anything else.") + message = _step_followup(messages) return ChatResult(generations=[ChatGeneration(message=message)]) diff --git a/tests/e2e/tests/dashboard.spec.ts b/tests/e2e/tests/dashboard.spec.ts index d6fa48bf..7b1ebb51 100644 --- a/tests/e2e/tests/dashboard.spec.ts +++ b/tests/e2e/tests/dashboard.spec.ts @@ -73,12 +73,7 @@ test.describe("Slack → web handoff (real dashboard UI)", () => { await expect(page.getByText(/anything else you'd like changed/)).toBeVisible(); // The non-owner's message is tagged server-side with their GitHub login, so - // the owner can tell who sent it. Visible once the transcript re-hydrates. - await expect(async () => { - await page.reload(); - await expect( - page.getByText(new RegExp(`@${OTHER_USER.login}`)).first(), - ).toBeVisible({ timeout: 8000 }); - }).toPass({ timeout: 60000 }); + // the owner can tell who sent it. + await expect(page.getByText(new RegExp(`@${OTHER_USER.login}`)).first()).toBeVisible(); }); }); diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index 738e136a..edeeee77 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -123,9 +123,9 @@ def test_render_inline_comment_body_line_reference_range() -> None: assert "*(Refers to line 10)*" in render_inline_comment_body(_f(start_line=10, end_line=10)) -def test_render_resolution_comment_resolved_uses_note() -> None: +def test_render_resolution_comment_resolved_uses_note_verbatim() -> None: body = render_resolution_comment(_f(status="resolved"), "resolved", note="Fixed at line 5") - assert body == "✅ **Resolved**: Fixed at line 5" + assert body == "Fixed at line 5" def test_render_resolution_comment_returns_none_without_agent_note() -> None: @@ -133,15 +133,15 @@ def test_render_resolution_comment_returns_none_without_agent_note() -> None: assert body is None -def test_render_resolution_comment_dismissed_uses_note() -> None: +def test_render_resolution_comment_dismissed_uses_note_verbatim() -> None: body = render_resolution_comment(_f(status="dismissed"), "dismissed", note="Intended behavior") - assert body == "❌ **Dismissed**: Intended behavior" + assert body == "Intended behavior" -def test_render_resolution_comment_uses_stored_resolution_note() -> None: +def test_render_resolution_comment_uses_stored_resolution_note_verbatim() -> None: finding = _f(status="resolved", resolution_note="The guard now returns before indexing.") body = render_resolution_comment(finding, "resolved") - assert body == "✅ **Resolved**: The guard now returns before indexing." + assert body == "The guard now returns before indexing." def test_parse_review_comment_marker_accepts_valid_marker() -> None: @@ -1205,7 +1205,7 @@ async def test_re_review_backfills_and_resolves_duplicate_existing_threads() -> assert reply_comment.await_count == 2 assert ( reply_comment.await_args_list[0].kwargs["body"] - == "✅ **Resolved**: The duplicate threads are fixed by the latest commit." + == "The duplicate threads are fixed by the latest commit." ) assert findings[0]["github_review_comment_ids"] == [101, 102] assert findings[0]["github_review_thread_ids"] == ["THREAD_1", "THREAD_2"] diff --git a/tests/test_reviewer_reconcile.py b/tests/test_reviewer_reconcile.py index 3e8b1180..c1e4588d 100644 --- a/tests/test_reviewer_reconcile.py +++ b/tests/test_reviewer_reconcile.py @@ -174,7 +174,7 @@ async def test_reconcile_duplicate_markers_require_all_threads_terminal() -> Non @pytest.mark.asyncio -async def test_reconcile_duplicate_markers_resolve_when_all_threads_terminal() -> None: +async def test_reconcile_duplicate_markers_stay_open_when_some_threads_only_outdated() -> None: findings = [ { "id": "f1", @@ -212,7 +212,7 @@ async def test_reconcile_duplicate_markers_resolve_when_all_threads_terminal() - ], ) - assert result[0]["status"] == "resolved" + assert result[0]["status"] == "open" assert "last_reconciliation_note" not in result[0] assert result[0]["github_resolved_thread_ids"] == ["THREAD_RESOLVED"] assert result[0].get("github_thread_resolved") is not True diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index 5e8ed372..567547e3 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -334,8 +334,7 @@ async def test_resolve_finding_thread_resolves_all_known_threads() -> None: ] assert [call.kwargs["review_comment_id"] for call in reply.await_args_list] == [11, 12] assert all( - "✅ **Resolved**: Fixed in the latest commit" in call.kwargs["body"] - for call in reply.await_args_list + call.kwargs["body"] == "Fixed in the latest commit" for call in reply.await_args_list ) updates = update.await_args.args[2] assert updates["github_thread_resolved"] is True