diff --git a/CLAUDE.md b/CLAUDE.md index 631ffd78..613c84f2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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`. diff --git a/README.md b/README.md index a847fb3c..2df103c7 100644 --- a/README.md +++ b/README.md @@ -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/` (optional resolvable `-` 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 #` for full fixes, `Refs #`/`Part of #` for partial work, `Closes owner/repo#` 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 `` 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 `` 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 diff --git a/agent/reviewer.py b/agent/reviewer.py index 7f3e5562..6402956b 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -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 diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py index 8e363507..3be23a9d 100644 --- a/agent/tools/publish_review.py +++ b/agent/tools/publish_review.py @@ -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": diff --git a/tests/reviewer/test_reviewer_publish.py b/tests/reviewer/test_reviewer_publish.py index cce5100c..9735b280 100644 --- a/tests/reviewer/test_reviewer_publish.py +++ b/tests/reviewer/test_reviewer_publish.py @@ -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