mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 03:23:13 +00:00
feat: clean-review auto-approve for unsolicited verdicts [BLOCKED — security] (#217)
Some checks failed
CI / Lint (push) Has been cancelled
CI / Format check (push) Has been cancelled
CI / Typecheck (push) Has been cancelled
CI / Unit tests (push) Has been cancelled
CI / Playwright E2E (push) Has been cancelled
CI / Docker build smoke (push) Has been cancelled
CI / Triage ledger up to date (push) Has been cancelled
CI / ui bun.lock in sync (push) Has been cancelled
Some checks failed
CI / Lint (push) Has been cancelled
CI / Format check (push) Has been cancelled
CI / Typecheck (push) Has been cancelled
CI / Unit tests (push) Has been cancelled
CI / Playwright E2E (push) Has been cancelled
CI / Docker build smoke (push) Has been cancelled
CI / Triage ledger up to date (push) Has been cancelled
CI / ui bun.lock in sync (push) Has been cancelled
* 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. * chore(security): record accepted-risk suppressions for clean-review auto-approve Two confirmed-HIGH findings from /sh-security-review on the clean-review auto-approve change are accepted and deferred (Adam, 2026-07-21), tracked in #218. Machine-recorded per the mandatory-security-review policy; the revisit trigger is promotion from dev to main/prod.
This commit is contained in:
parent
ac569b9a0a
commit
c99bd78179
6 changed files with 222 additions and 26 deletions
|
|
@ -1,5 +1,25 @@
|
|||
{
|
||||
"suppressions": [
|
||||
{
|
||||
"id": "AUTHZ-CLEAN-AUTOAPPROVE-INJECTION-001",
|
||||
"title": "Clean-review auto-approve derives APPROVE authority from a model-controlled signal on the untrusted auto-review path (prompt-injection-mintable APPROVE)",
|
||||
"file": "agent/tools/publish_review.py",
|
||||
"severity": "high",
|
||||
"status": "confirmed",
|
||||
"suppression_justification": "ACCEPTED (Adam, 2026-07-21) as a knowingly-deferred risk merged into the dev integration branch via admin merge in PR #217. /sh-security-review returned BLOCK: moving `approve` authorization from the deterministic verdict_requested flag to open_findings_count==0 lets a prompt-injected auto-review (which never sets verdict_requested) land a real bot APPROVE on an attacker-controlled external/fork PR, potentially satisfying branch protection. This reintroduces the hole PR #214 closed. Merged to dev only (NOT main/prod). Compensating controls: (a) confirm dev does not auto-deploy to sh-openswe; (b) on sensitive repos, esp. payments-dashboard, configure rulesets so the seahaven-openswe[bot] APPROVE does not by itself satisfy required approvals. Tracked in issue #218 with the full redesign checklist. REVISIT TRIGGER: MUST be resolved before this change promotes from dev to main/prod, and immediately if dev is found to auto-deploy. Verified HIGH by the sh-security-review detector fan-out (6 independent detectors converged).",
|
||||
"owner": "adam@seahavenind.com",
|
||||
"added": "2026-07-21"
|
||||
},
|
||||
{
|
||||
"id": "AUTHZ-CLEAN-AUTOAPPROVE-THREADLAUNDER-002",
|
||||
"title": "Clean-review auto-approve gate reads reconcile-derived finding status, so a PR author can launder a dirty PR to clean via GitHub thread resolve/outdate",
|
||||
"file": "agent/tools/publish_review.py",
|
||||
"severity": "high",
|
||||
"status": "confirmed",
|
||||
"suppression_justification": "ACCEPTED (Adam, 2026-07-21), deferred with AUTHZ-CLEAN-AUTOAPPROVE-INJECTION-001 in PR #217 (admin-merged to dev). The open_findings_count gate reads finding status after reconcile_findings_with_review_threads, which flips open->resolved when the GitHub thread is is_resolved/is_outdated — both author-controllable ('Resolve conversation' or a trivial hunk-outdating commit) without fixing the defect, yielding an auto-approve on an unfixed PR. Same compensating controls and revisit trigger as ...-001. Tracked in issue #218 (redesign: compute the gate from the reviewer's own authoritative finding state, not author-influenced thread state). Verified HIGH by /sh-security-review.",
|
||||
"owner": "adam@seahavenind.com",
|
||||
"added": "2026-07-21"
|
||||
},
|
||||
{
|
||||
"id": "AUTHZ-SLACK-BOT-DEFAULT-001",
|
||||
"title": "Slack entrypoint lacks a per-user repo-access check; default-bot PR authoring removes the implicit per-user repo boundary",
|
||||
|
|
|
|||
|
|
@ -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`.
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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":
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue