feat(reviewer): clean-review auto-approve for unsolicited publish_review verdicts

An unsolicited publish_review(verdict="approve") — a run dispatched
without verdict_requested — is now honored when the review has zero open
findings, so clean auto-reviews land a real APPROVE. With open findings
it downgrades to a comment review (verdict_ignored_reason=
"approve_with_open_findings"). request_changes stays explicit-request-
only; the self-review, head-moved, and author-unknown downgrades and the
shell verdict guard are unchanged. The reviewer base prompt now instructs
the clean-approve call on auto-reviews.
This commit is contained in:
Adam Moussa 2026-07-21 20:00:35 -04:00
parent ac569b9a0a
commit 49160f258c
No known key found for this signature in database
5 changed files with 202 additions and 26 deletions

View file

@ -82,7 +82,7 @@ The system prompt instructs the agent to call a tool every turn, and `ensure_no_
Other middleware exists in `agent/middleware/` (`ExcludeToolsMiddleware`) but isn't wired into the default agent. The reviewer uses a leaner stack (see `reviewer.py:get_reviewer_agent` for the authoritative order), including `SanitizeToolInputsMiddleware`, `ModelCallLimitMiddleware`, `ToolErrorMiddleware`, `PullRequestVerdictGuardMiddleware`, `SlackAssistantStatusMiddleware`, and `settle_review_check_on_exit`.
**Verdict gating (Sea Haven fork):** `PullRequestVerdictGuardMiddleware` (`agent/middleware/pr_verdict_guard.py`) is wired into BOTH graphs (`server.py:get_agent` after `PullRequestCreationGuardMiddleware`; `reviewer.py:get_reviewer_agent` after `ToolErrorMiddleware`). It blocks shell-path review verdicts (`gh pr review --approve/-a/--request-changes/-r`, `gh api`/`curl` posting `event=APPROVE|REQUEST_CHANGES` to `/pulls/N/reviews`); comment reviews and reads pass through. Verdicts go exclusively through `publish_review(verdict=...)`, which honors them only when the run's `configurable["verdict_requested"] is True` — set solely by the explicit-mention dispatch path (`trigger_pr_review_from_ref(request_verdict=True)` → `_build_reviewer_configurable`). Auto-review dispatches never set it. The tool layer also downgrades self-reviews (PR author in `INTERNAL_BOT_LOGINS`) to comment reviews and best-effort dismisses a recorded stale APPROVE when a later publish surfaces new findings.
**Verdict gating (Sea Haven fork):** `PullRequestVerdictGuardMiddleware` (`agent/middleware/pr_verdict_guard.py`) is wired into BOTH graphs (`server.py:get_agent` after `PullRequestCreationGuardMiddleware`; `reviewer.py:get_reviewer_agent` after `ToolErrorMiddleware`). It blocks shell-path review verdicts (`gh pr review --approve/-a/--request-changes/-r`, `gh api`/`curl` posting `event=APPROVE|REQUEST_CHANGES` to `/pulls/N/reviews`); comment reviews and reads pass through. Verdicts go exclusively through `publish_review(verdict=...)`: `request_changes` is honored only when the run's `configurable["verdict_requested"] is True` — set solely by the explicit-mention dispatch path (`trigger_pr_review_from_ref(request_verdict=True)` → `_build_reviewer_configurable`); auto-review dispatches never set it. `approve` is additionally honored on non-verdict-requested runs when the review is clean (zero open findings — the clean-review auto-approve); with open findings it downgrades to a comment (`verdict_ignored_reason="approve_with_open_findings"`). The tool layer also downgrades self-reviews (PR author in `INTERNAL_BOT_LOGINS`) to comment reviews and best-effort dismisses a recorded stale APPROVE when a later publish surfaces new findings.
There is intentionally no after-agent safety net that opens a PR for the agent. The agent itself is responsible for committing, pushing, opening/updating the draft PR, and replying in the source channel — all via `GH_TOKEN=dummy gh` and `slack_thread_reply` / `linear_comment`.

View file

@ -124,7 +124,7 @@ Each invocation creates a deterministic thread ID, so follow-up messages on the
**Engineering conventions & attribution (Sea Haven fork):** the main agent's system prompt is tuned to the Sea Haven engineering handbook — branch names are `feature|bug|hotfix/<kebab-desc>` (optional resolvable `<KEY>-` prefix), PR bodies use `## Summary / Validation / Tests / Notes`, and commit messages follow the handbook format (≤50-char imperative subject, *why* over *what*). The **PR title rule is repo-aware**: when the target repo enforces a conventional-commit title (an `amannn/action-semantic-pull-request` workflow, a `commitlint` config, or a documented requirement in `AGENTS.md` / `CONTRIBUTING.md`), the agent emits a conforming `type(scope): …` title that reads the action's allowed types/scopes — this lets it pass gates like this repo's own `PR Title Lint` and upstream `langchain-ai/open-swe` without manual retitling; otherwise it falls back to the Sea Haven imperative style with no `type:` prefix. PRs that resolve a GitHub issue **auto-link it** in the body (`Closes #<n>` for full fixes, `Refs #<n>`/`Part of #<n>` for partial work, `Closes owner/repo#<n>` cross-repo); because the Sea Haven flow targets `dev` rather than the default branch, the issue closes when `dev` is promoted, not at dev-merge. **No agent/AI attribution is added to any artifact** — no `Co-authored-by` bot trailer, no `Made by [Open SWE]` footer, no "generated by an agent" notes. Commits are currently authored as the **triggering user** (the upstream behavior, which keeps Vercel preview deploys resolvable); flipping authorship to the bot account is tracked separately in issue #11 pending the Vercel-resolvability decision.
**PR reviews & verdicts (Sea Haven fork):** a separate read-only **reviewer** graph reviews PRs and publishes findings as a GitHub Review. Auto-reviews (PR opened / ready-for-review / push re-review / finding replies) are always **advisory** (`event=COMMENT`). When a user's `@openswe` mention *explicitly asks for a verdict* ("approve if it meets the bar; request changes if not"), the coding agent forwards the request via `request_pr_review(instructions=..., request_verdict=True)`; the reviewer run is then authorized — enforced in code via `configurable["verdict_requested"]`, not prompt — to submit a real **APPROVE** or **REQUEST_CHANGES** through `publish_review(verdict=...)`. Requester instructions travel as an escaped `<requester_instructions>` data block (untrusted: they may set focus/merge bar, never override safety rules). Safeguards: verdicts on Open SWE's own PRs are downgraded to comment reviews (self-review guard); unauthorized verdicts are dropped with `verdict_ignored`; a recorded APPROVE is best-effort dismissed when a later review surfaces new findings; and `PullRequestVerdictGuardMiddleware` blocks the shell path on both graphs.
**PR reviews & verdicts (Sea Haven fork):** a separate read-only **reviewer** graph reviews PRs and publishes findings as a GitHub Review. Auto-reviews (PR opened / ready-for-review / push re-review / finding replies) with findings are **advisory** (`event=COMMENT`) — but a **clean review (zero open findings) auto-approves**: the reviewer calls `publish_review(verdict="approve")` and it lands as a real **APPROVE** even without an explicit verdict request. When a user's `@openswe` mention *explicitly asks for a verdict* ("approve if it meets the bar; request changes if not"), the coding agent forwards the request via `request_pr_review(instructions=..., request_verdict=True)`; the reviewer run is then authorized — enforced in code via `configurable["verdict_requested"]`, not prompt — to submit a real **APPROVE** or **REQUEST_CHANGES** through `publish_review(verdict=...)`. Requester instructions travel as an escaped `<requester_instructions>` data block (untrusted: they may set focus/merge bar, never override safety rules). Safeguards: verdicts on Open SWE's own PRs are downgraded to comment reviews (self-review guard); unauthorized `request_changes` verdicts are dropped with `verdict_ignored`, and an unsolicited approve alongside open findings is downgraded to a comment (`approve_with_open_findings`); a recorded APPROVE is best-effort dismissed when a later review surfaces new findings; and `PullRequestVerdictGuardMiddleware` blocks the shell path on both graphs.
### 7. Validation — Prompt-Driven

View file

@ -299,8 +299,16 @@ severities — they're not findings.
- Read-only. Do not commit, push, or use `gh pr review` / `gh api .../reviews`.
Never approve or request changes on a PR via the shell or the GitHub API —
review verdicts go exclusively through `publish_review`, and only when this
run was explicitly authorized to submit one.
review verdicts go exclusively through `publish_review`. A
`request_changes` verdict is honored only when this run was explicitly
authorized to submit one.
- Clean-review auto-approve: when your finished review has zero open
findings and the diff meets a normal production merge bar, call
`publish_review(verdict="approve")` even without an explicit verdict
request — clean reviews land as real approvals. Do not pass a verdict
while open findings remain unless this run was explicitly authorized to
submit one; an unsolicited approve alongside open findings is downgraded
to a comment review.
- One finding per defect (with the fan-out rule above for cross-file bugs).
- Include `suggestion` only when the fix is ≤4 lines and obvious.
- Publish a concise review: prefer the highest-confidence findings that

View file

@ -90,12 +90,16 @@ async def publish_review(
mentioned in the review summary with a link to the web app, but are
not posted as inline PR comments.
verdict: Optional review verdict — ``"approve"`` or
``"request_changes"``. Honored ONLY when this run was explicitly
authorized to submit a verdict (the triggering user asked for one);
otherwise the review is published as a plain comment and the result
carries ``verdict_ignored: true``. Never describe an ignored
verdict as an approval. Verdicts are also downgraded to a comment
when the PR was authored by Open SWE itself (self-review).
``"request_changes"``. ``"request_changes"`` is honored ONLY when
this run was explicitly authorized to submit a verdict (the
triggering user asked for one). ``"approve"`` is also honored on a
run without that authorization when the review is clean — zero
open findings — so a clean review lands as a real APPROVE; with
open findings an unsolicited approve is downgraded to a plain
comment and the result carries ``verdict_ignored: true``. Never
describe an ignored verdict as an approval. Verdicts are also
downgraded to a comment when the PR was authored by Open SWE
itself (self-review).
Returns:
Dictionary with ``success``, ``review_id``, ``surfaced_count``,
``hidden_count``, ``resolved_thread_count``, and sometimes
@ -119,8 +123,9 @@ async def publish_review(
``verdict_submitted`` (GitHub confirmed the requested APPROVE/
REQUEST_CHANGES state) or ``verdict_ignored`` +
``verdict_ignored_reason`` (``"verdict_not_requested"``,
``"self_review"``, ``"head_moved"`` — the reviewed commit is no longer
the PR head — or ``"author_unknown"``).
``"approve_with_open_findings"`` — an unsolicited approve on a run
with open findings — ``"self_review"``, ``"head_moved"`` — the
reviewed commit is no longer the PR head — or ``"author_unknown"``).
"""
if severity_threshold not in {"low", "medium", "high", "critical"}:
return {"success": False, "error": f"Invalid severity_threshold: {severity_threshold}"}
@ -179,18 +184,25 @@ async def publish_review(
# Verdict authorization is enforced here in code, not in the prompt: only
# a run whose dispatching webhook set verdict_requested (the explicit
# mention path) may submit a real review state. Anything else — including
# a model that hallucinates authorization — publishes as a plain comment.
# mention path) may submit a blocking review state. Anything else —
# including a model that hallucinates authorization — publishes as a plain
# comment. Exception: an unsolicited "approve" is allowed through, and
# _publish_review_async honors it only when the review has zero open
# findings (clean-review auto-approve).
verdict_not_requested = False
unsolicited_approve = False
if verdict is not None and configurable.get("verdict_requested") is not True:
logger.info(
"publish_review verdict %r dropped: run not authorized to submit verdicts "
"(pr_number=%s)",
verdict,
pr_number,
)
verdict = None
verdict_not_requested = True
if verdict == "approve":
unsolicited_approve = True
else:
logger.info(
"publish_review verdict %r dropped: run not authorized to submit verdicts "
"(pr_number=%s)",
verdict,
pr_number,
)
verdict = None
verdict_not_requested = True
try:
result = await _publish_review_async(
@ -206,6 +218,7 @@ async def publish_review(
trace_link_config_override=configurable.get("review_trace_link_enabled"),
verdict=verdict,
verdict_requester=str(configurable.get("github_login") or ""),
unsolicited_approve=unsolicited_approve,
)
if verdict_not_requested:
result["verdict_ignored"] = True
@ -310,6 +323,7 @@ async def _publish_review_async(
trace_link_config_override: object = None,
verdict: str | None = None,
verdict_requester: str = "",
unsolicited_approve: bool = False,
) -> dict[str, Any]:
thread_id = get_thread_id_from_runtime()
verdict_ignored_reason: str | None = None
@ -370,9 +384,6 @@ async def _publish_review_async(
)
verdict = None
verdict_ignored_reason = "self_review"
event = _VERDICT_EVENTS.get(verdict or "", "COMMENT")
review_trace_url = await _resolve_review_trace_url(thread_id, trace_link_config_override)
review_ui_url = dashboard_review_url(owner, repo, pr_number)
findings = await _backfill_findings_from_pr_threads(
thread_id=thread_id,
owner=owner,
@ -381,6 +392,28 @@ async def _publish_review_async(
token=token,
)
# An unsolicited approve (no verdict_requested on the run) is honored only
# for a clean review: any open finding — new, previously published, or
# below the surfacing threshold — means changes are effectively being
# requested, so the approve downgrades to a comment.
if verdict == "approve" and unsolicited_approve:
open_findings_count = sum(1 for f in findings if f.get("status", "open") == "open")
if open_findings_count:
logger.info(
"publish_review unsolicited approve downgraded to comment: %d open "
"finding(s) for %s/%s#%s",
open_findings_count,
owner,
repo,
pr_number,
)
verdict = None
verdict_ignored_reason = "approve_with_open_findings"
event = _VERDICT_EVENTS.get(verdict or "", "COMMENT")
review_trace_url = await _resolve_review_trace_url(thread_id, trace_link_config_override)
review_ui_url = dashboard_review_url(owner, repo, pr_number)
# Re-reviews only post NEW findings. Anything with a github_review_comment_id
# already lives on GitHub from a prior publish — reposting would create
# duplicate inline comments and break the resolve-on-fix flow (only
@ -489,6 +522,7 @@ async def _publish_review_async(
verdict=verdict,
verdict_requester=verdict_requester,
verdict_ignored_reason=verdict_ignored_reason,
unsolicited_approve=unsolicited_approve,
)
review_response = await post_pull_request_review(
@ -530,6 +564,7 @@ async def _publish_review_async(
verdict=verdict,
verdict_requester=verdict_requester,
verdict_ignored_reason=verdict_ignored_reason,
unsolicited_approve=unsolicited_approve,
)
retry_response = await post_pull_request_review(
owner=owner,
@ -577,6 +612,7 @@ async def _publish_review_async(
verdict=verdict,
verdict_requester=verdict_requester,
verdict_ignored_reason=verdict_ignored_reason,
unsolicited_approve=unsolicited_approve,
)
verdict_only_response = await post_pull_request_review(
owner=owner,
@ -806,9 +842,12 @@ def _decorate_review_body(
verdict: str | None,
verdict_requester: str,
verdict_ignored_reason: str | None,
unsolicited_approve: bool = False,
) -> str:
"""Append verdict attribution / downgrade context to the review body."""
if verdict is not None:
if unsolicited_approve:
return f"{body}\n\nVerdict (`approve`) submitted — the review found no open issues."
requester = f"@{verdict_requester}" if verdict_requester else "the requester"
return f"{body}\n\nVerdict (`{verdict}`) submitted at the request of {requester}."
if verdict_ignored_reason == "self_review":

View file

@ -2429,7 +2429,7 @@ async def test_publish_review_drops_verdict_when_not_requested() -> None:
patch("agent.tools.publish_review.get_github_token", return_value="token"),
patch("agent.tools.publish_review._publish_review_async", publish_async),
):
result = await publish_review(verdict="approve")
result = await publish_review(verdict="request_changes")
assert publish_async.call_args.kwargs["verdict"] is None
assert result["verdict_ignored"] is True
@ -2437,6 +2437,38 @@ async def test_publish_review_drops_verdict_when_not_requested() -> None:
assert result["verdict_submitted"] is False
async def test_publish_review_forwards_unsolicited_approve() -> None:
"""An approve without verdict_requested is forwarded (not dropped) with the
unsolicited flag set, so the async layer can apply the clean-review gate."""
from agent.tools.publish_review import publish_review
publish_async = AsyncMock(
return_value={"success": True, "review_id": 9, "verdict_submitted": True}
)
with (
patch(
"agent.tools.publish_review.get_config",
return_value={
"configurable": {
"thread_id": "tid",
"repo": {"owner": "o", "name": "r"},
"pr_number": 7,
"head_sha": "sha",
},
"metadata": {},
},
),
patch("agent.tools.publish_review.get_github_token", return_value="token"),
patch("agent.tools.publish_review._publish_review_async", publish_async),
):
result = await publish_review(verdict="approve")
assert publish_async.call_args.kwargs["verdict"] == "approve"
assert publish_async.call_args.kwargs["unsolicited_approve"] is True
assert result["verdict_submitted"] is True
assert "verdict_ignored" not in result
async def test_publish_review_forwards_authorized_verdict() -> None:
from agent.tools.publish_review import publish_review
@ -2542,6 +2574,103 @@ async def test_publish_async_approve_with_zero_findings_bypasses_empty_skip() ->
assert "Verdict (`approve`) submitted at the request of @amoussa1229." in body
async def test_publish_async_unsolicited_approve_clean_posts_approve() -> None:
"""A clean review (zero open findings) lands an unsolicited approve as a
real APPROVE, with automatic (not requester) attribution."""
from contextlib import ExitStack
from agent.tools.publish_review import _publish_review_async
post_review = AsyncMock(return_value={"id": 2001})
with ExitStack() as stack:
for p in _verdict_publish_patches(findings=[], post_review=post_review):
stack.enter_context(p)
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="sha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=False,
verdict="approve",
verdict_requester="",
unsolicited_approve=True,
)
assert result["success"] is True
assert result["verdict_submitted"] is True
assert result["verdict_event"] == "APPROVE"
assert post_review.await_args.kwargs["event"] == "APPROVE"
body = post_review.await_args.kwargs["body"]
assert "Verdict (`approve`) submitted — the review found no open issues." in body
assert "at the request of" not in body
async def test_publish_async_unsolicited_approve_with_open_findings_downgrades() -> None:
"""An unsolicited approve alongside open findings is downgraded to a
comment review — approving while requesting changes is contradictory."""
from contextlib import ExitStack
from agent.tools.publish_review import _publish_review_async
findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)]
post_review = AsyncMock(return_value={"id": 2002})
with ExitStack() as stack:
for p in _verdict_publish_patches(findings=findings, post_review=post_review):
stack.enter_context(p)
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="sha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=False,
verdict="approve",
verdict_requester="",
unsolicited_approve=True,
)
assert result["success"] is True
assert result.get("verdict_submitted") is not True
assert result["verdict_ignored"] is True
assert result["verdict_ignored_reason"] == "approve_with_open_findings"
assert post_review.await_args.kwargs["event"] == "COMMENT"
async def test_publish_async_authorized_approve_ignores_open_findings_gate() -> None:
"""The explicit-request path is unchanged: an authorized approve is honored
even when open findings exist (the requester asked for the verdict)."""
from contextlib import ExitStack
from agent.tools.publish_review import _publish_review_async
findings = [_f(id="f_1", severity="high", file="a.py", start_line=1, end_line=1)]
post_review = AsyncMock(return_value={"id": 2003})
with ExitStack() as stack:
for p in _verdict_publish_patches(findings=findings, post_review=post_review):
stack.enter_context(p)
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="sha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=False,
verdict="approve",
verdict_requester="amoussa1229",
)
assert result["verdict_submitted"] is True
assert result["verdict_event"] == "APPROVE"
assert post_review.await_args.kwargs["event"] == "APPROVE"
async def test_publish_async_request_changes_maps_event() -> None:
from contextlib import ExitStack