open-swe/tests/test_reviewer_publish.py

2228 lines
84 KiB
Python
Raw Normal View History

feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
"""Unit tests for the publish_review rendering and orchestration helpers."""
from __future__ import annotations
from collections.abc import Iterator
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
from typing import Any
from unittest.mock import AsyncMock, MagicMock, patch
import pytest
from agent.reviewer_findings import Finding, new_finding
from agent.reviewer_publish import (
clear_review_started_comment,
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
fetch_pr_review_threads,
open_swe_review_exists,
parse_review_comment_marker,
post_pull_request_review,
post_review_started_comment,
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
render_inline_comment_body,
render_inline_comment_payload,
render_resolution_comment,
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
render_review_body,
render_status_comment,
reply_to_review_comment,
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
resolve_review_thread,
review_summary_marker,
status_comment_marker,
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
)
def _f(**overrides: Any) -> Finding:
base = new_finding(
severity="high",
feat: tune reviewer for precision — web/wiki tools + recalibrated prompt (#1312) * feat: tune reviewer for precision — web/wiki tools + recalibrated prompt Reviewer agent now has web_search, fetch_url, and http_request alongside the finding tools, so it can verify library semantics and consult the DeepWiki auto-generated wiki for public repos (https://deepwiki.com/<owner>/<repo>) before flagging cross-file or architectural concerns. Prompt rewritten to push precision over recall: - explicit severity ladder pushing reviews toward bimodal high/low instead of defaulting to medium - ≤200-char description target (gold set averages ~186 chars; we were at ~436) - mandatory docs / wiki / code lookup before flagging concurrency, security, or perf — the three categories that dominated false positives - "do not flag" list covering compiler/linter-catchable nits, speculative claims without a concrete attacker/interleaving/scale, style preferences the codebase doesn't share, and test-quality nits on non-test diffs - smart file-selection guidance for large PRs (deprioritize generated / vendored / pure-rename hunks) Eval config switched to openai:gpt-5.5 + high reasoning effort for the next benchmark run. * trim prompt * subagent prompting * confidence ratings * added medium * enforce confidence threshold * . * reviewer: precision-tuned prompt + drop confidence gate Rewrites the reviewer system prompt around a defensibility bar (anchor + failure mode + maintainer wouldn't say "not a bug"), an explicit do-not-file list (style nits, speculation, scope-policing, same-bug fan-out), and a checklist of 10 bug archetypes drawn from a per-PR audit of the eval golden set. The audit showed 145 FPs in the last eval split ~28% speculative, ~26% style-nit, ~31% real-but-unscored (mostly same-archetype fan-out); the new prompt targets each class directly. Confidence is still recorded on every finding for post-hoc calibration but no longer gates publication — the audit showed the gate was a no-op (agent self-rated 65% of findings "high" regardless), and the prompt's defensibility bar is the actual discipline. Drops CONFIDENCE_ORDER, CONFIDENCE_THRESHOLD, the confidence_threshold kwarg on filter_findings_for_publish, the confidence_filtered score_mode, and the min_confidence kwarg on the eval target's _extract_comments — all dead once the gate is gone. Also removes the "informational" severity tier from the Severity enum, SEVERITY_ORDER, and all validators / tests / docstrings. It was reserved for FYI observations the dataset never rewards. * benchmax * adding google provider * slight steering * tuning * more tuning * fix * cleanup * reducing overfitting * Add per-repo review style profiles and inject them into the reviewer. Dashboard users can analyze historical PR review feedback per repository, edit the resulting style guide, and have it loaded from LangGraph Store at reviewer runtime (including Martian eval runs) keyed by owner/name. Co-authored-by: Cursor <cursoragent@cursor.com> * Fix review style job errors leaking exception details to clients. Return generic dashboard messages while logging full stack traces server-side. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
2026-05-20 11:35:00 -07:00
confidence="high",
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
category="correctness",
file="src/foo.py",
start_line=10,
end_line=10,
description="boom",
sha="abc",
)
base.update(overrides) # type: ignore[arg-type]
return base
@pytest.fixture(autouse=True)
def _isolate_publish_review_pr_state() -> Iterator[None]:
with (
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.clear_review_started_comment", AsyncMock()),
fix: reviewer publishes against stale head_sha on mid-run re-review (#1393) * 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.
2026-06-03 11:38:56 -07:00
patch(
"agent.tools.publish_review.resolve_review_head_sha",
AsyncMock(
side_effect=lambda thread_id, configurable: configurable.get("head_sha") or ""
),
),
):
yield
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
def test_render_inline_comment_body_without_suggestion() -> None:
body = render_inline_comment_body(_f(description="just text"))
assert "<!-- open-swe-review-comment" in body
assert '"id":"f_' in body
assert "just text" in body
assert "Your feedback helps Open SWE learn." in body
assert "👍 or 👎" in body
assert "tell us if this review comment was useful" in body
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
def test_render_inline_comment_body_with_suggestion_appends_block() -> None:
body = render_inline_comment_body(
_f(description="needs fix", suggestion="x = 1\nx += 1"),
)
assert "needs fix" in body
assert "```suggestion" in body
assert "x = 1\nx += 1" in body
def test_render_inline_comment_body_uses_severity_emoji_and_bold_title() -> None:
body = render_inline_comment_body(_f(severity="critical", description="Null deref"))
assert "🔴 **Null deref**" in body
def test_render_inline_comment_body_uses_generated_title() -> None:
description = "This request can fail because the new path skips auth token refresh."
body = render_inline_comment_body(_f(title="Refresh token skipped", description=description))
assert "🟠 **Refresh token skipped**" in body
assert description in body
assert "This request can fail because the new path skips auth token refresh" in body
def test_render_inline_comment_body_does_not_duplicate_first_line() -> None:
body = render_inline_comment_body(
_f(description="Short summary line\n\nLonger detail paragraph."),
)
assert "**Short summary line**" in body
assert "Longer detail paragraph." in body
assert body.count("Short summary line") == 1
def test_render_inline_comment_body_does_not_duplicate_generated_title() -> None:
body = render_inline_comment_body(
_f(
title="Short summary line", description="Short summary line\n\nLonger detail paragraph."
),
)
assert "**Short summary line**" in body
assert "Longer detail paragraph." in body
assert body.count("Short summary line") == 1
def test_render_inline_comment_body_single_line_has_no_detail() -> None:
body = render_inline_comment_body(_f(description="just text"))
assert body.count("just text") == 1
def test_render_inline_comment_body_line_reference_range() -> None:
assert "*(Refers to lines 10-12)*" in render_inline_comment_body(_f(start_line=10, end_line=12))
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:
body = render_resolution_comment(_f(status="resolved"), "resolved", note="Fixed at line 5")
assert body == "✅ **Resolved**: Fixed at line 5"
def test_render_resolution_comment_returns_none_without_agent_note() -> None:
body = render_resolution_comment(_f(status="resolved"), "resolved")
assert body is None
def test_render_resolution_comment_dismissed_uses_note() -> None:
body = render_resolution_comment(_f(status="dismissed"), "dismissed", note="Intended behavior")
assert body == "❌ **Dismissed**: Intended behavior"
def test_render_resolution_comment_uses_stored_resolution_note() -> 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."
def test_parse_review_comment_marker_accepts_valid_marker() -> None:
finding = _f(
id="f_marker",
file="agent/webapp.py",
start_line=10,
end_line=12,
side="RIGHT",
)
marker = parse_review_comment_marker(render_inline_comment_body(finding))
assert marker == {
"id": "f_marker",
"file_path": "agent/webapp.py",
"start_line": 10,
"end_line": 12,
"side": "RIGHT",
}
def test_parse_review_comment_marker_rejects_malformed_marker() -> None:
assert parse_review_comment_marker("plain body") is None
assert parse_review_comment_marker("<!-- open-swe-review-comment {} -->") is None
assert (
parse_review_comment_marker(
'<!-- open-swe-review-comment {"id":"f1","file_path":"x.py","side":"BAD"} -->'
)
is None
)
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
def test_render_inline_comment_payload_single_line() -> None:
payload = render_inline_comment_payload(_f(start_line=10, end_line=10))
assert payload is not None
assert payload["path"] == "src/foo.py"
assert payload["line"] == 10
assert payload["side"] == "RIGHT"
assert "boom" in payload["body"]
assert "<!-- open-swe-review-comment" in payload["body"]
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
def test_render_inline_comment_payload_multi_line_uses_start_fields() -> None:
payload = render_inline_comment_payload(_f(start_line=8, end_line=12))
assert payload is not None
assert payload["start_line"] == 8
assert payload["start_side"] == "RIGHT"
assert payload["line"] == 12
def test_render_inline_comment_payload_returns_none_for_file_level() -> None:
payload = render_inline_comment_payload(_f(start_line=None, end_line=None))
assert payload is None
def test_render_review_body_with_findings_uses_potential_issue_phrasing() -> None:
body = render_review_body(pr_number=123, surfaced_count=2)
assert body.startswith("**Open SWE Review** found 2 potential issues.")
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
assert "<!-- open-swe-reviewer pr=123 -->" in body
fix(open-swe): always post a review summary, even with no findings (#1256) * fix(reviewer): log every push/close early-return so 'silent ignore' is debuggable Pushes to PRs that haven't had a first review fall through the watch handler because the reviewer thread doesn't have kind=reviewer set. Without log lines on the early-return paths, this scenario was indistinguishable from 'webhook reached the handler at all' in the hosted log stream. Now every early-return logs at info or debug: - info when a real PR exists but the reviewer thread isn't set up (with a hint pointing at the trigger paths the user can use) - info when the repo isn't in the reviewer allowlist - debug for benign skips (non-branch refs, branch deletions, already-reviewed head_sha) * fix(reviewer): always post a summary review, even with no findings The publish_review tool gated POSTing on `inline_comments or summary`, so when the agent called publish_review() with no args on a clean PR the result returned `success: true` but no GitHub review was posted — the user got silence instead of a "no issues found" comment. - Drop the gate so publish_review always POSTs. - Friendlier no-findings render: `**No issues found.**` when the findings list is empty, vs. `**No issues at or above \`<sev>\` severity.**` with hidden count when only sub-threshold findings exist. Agent summary renders below. - Prompt now requires the agent to always pass a `summary` so the body is meaningful; calls out specifically not to skip on a clean PR.
2026-05-07 15:16:20 -07:00
def test_render_review_body_singular_finding() -> None:
body = render_review_body(pr_number=123, surfaced_count=1)
assert body.startswith("**Open SWE Review** found 1 potential issue.")
fix(open-swe): always post a review summary, even with no findings (#1256) * fix(reviewer): log every push/close early-return so 'silent ignore' is debuggable Pushes to PRs that haven't had a first review fall through the watch handler because the reviewer thread doesn't have kind=reviewer set. Without log lines on the early-return paths, this scenario was indistinguishable from 'webhook reached the handler at all' in the hosted log stream. Now every early-return logs at info or debug: - info when a real PR exists but the reviewer thread isn't set up (with a hint pointing at the trigger paths the user can use) - info when the repo isn't in the reviewer allowlist - debug for benign skips (non-branch refs, branch deletions, already-reviewed head_sha) * fix(reviewer): always post a summary review, even with no findings The publish_review tool gated POSTing on `inline_comments or summary`, so when the agent called publish_review() with no args on a clean PR the result returned `success: true` but no GitHub review was posted — the user got silence instead of a "no issues found" comment. - Drop the gate so publish_review always POSTs. - Friendlier no-findings render: `**No issues found.**` when the findings list is empty, vs. `**No issues at or above \`<sev>\` severity.**` with hidden count when only sub-threshold findings exist. Agent summary renders below. - Prompt now requires the agent to always pass a `summary` so the body is meaningful; calls out specifically not to skip on a clean PR.
2026-05-07 15:16:20 -07:00
def test_render_review_body_no_findings_message() -> None:
body = render_review_body(pr_number=99, surfaced_count=0)
assert "## ✅ Open SWE Review: No issues found" in body
assert "Open SWE reviewed this PR and found no potential bugs to report." in body
assert "additional" not in body
def test_render_review_body_with_additional_findings_and_ui_link() -> None:
body = render_review_body(
pr_number=99,
surfaced_count=0,
additional_findings_count=2,
ui_url="https://dash.example/agents/reviews/o/r/99",
)
assert "## ✅ Open SWE Review: No issues found" in body
assert "2 additional findings can be viewed in the web app." in body
assert "[Open in Web](https://dash.example/agents/reviews/o/r/99)" in body
def test_render_review_body_with_single_additional_finding_uses_singular() -> None:
body = render_review_body(pr_number=99, surfaced_count=0, additional_findings_count=1)
assert "1 additional finding can be viewed in the web app." in body
def test_render_review_body_with_surfaced_and_additional_findings() -> None:
body = render_review_body(
pr_number=99,
surfaced_count=3,
additional_findings_count=2,
ui_url="https://dash.example/agents/reviews/o/r/99",
)
assert "found 3 potential issues." in body
assert "2 additional findings can be viewed in the web app." in body
def test_render_review_body_additional_findings_zero_omits_line() -> None:
body = render_review_body(pr_number=99, surfaced_count=0, additional_findings_count=0)
assert "additional" not in body
def test_render_status_comment_reviewing_includes_ui_link(monkeypatch: Any) -> None:
monkeypatch.setenv("DASHBOARD_BASE_URL", "https://dash.example")
body = render_status_comment(pr_number=7, thread_id="tid-1")
assert "🔍 Open SWE Review: in progress" in body
assert "[Open in Web](https://dash.example/agents/tid-1)" in body
assert status_comment_marker(7) in body
def test_render_review_body_includes_ui_link() -> None:
body = render_review_body(
pr_number=7, surfaced_count=0, ui_url="https://dash.example/agents/tid-1"
)
assert "[Open in Web](https://dash.example/agents/tid-1)" in body
def test_render_review_body_orders_ui_link_before_trace() -> None:
body = render_review_body(
pr_number=7,
surfaced_count=1,
ui_url="https://dash.example/agents/tid-1",
trace_url="https://trace.example/x",
)
assert "[Open in Web](https://dash.example/agents/tid-1) • [View Open SWE trace]" in body
@pytest.mark.asyncio
async def test_post_review_started_comment_posts_and_persists_id() -> None:
post = AsyncMock(return_value=4242)
set_meta = AsyncMock()
with (
patch("agent.reviewer_publish.get_thread_metadata", AsyncMock(return_value={})),
patch("agent.reviewer_publish.post_status_comment", post),
patch("agent.reviewer_publish.delete_status_comment", AsyncMock()) as delete,
patch("agent.reviewer_publish.set_reviewer_thread_metadata", set_meta),
):
cid = await post_review_started_comment(
thread_id="tid", owner="o", repo="r", pr_number=7, token="t"
)
assert cid == 4242
delete.assert_not_called()
post.assert_awaited_once()
set_meta.assert_awaited_once_with("tid", extra={"status_comment_id": 4242})
@pytest.mark.asyncio
async def test_post_review_started_comment_deletes_lingering_before_reposting() -> None:
post = AsyncMock(return_value=500)
delete = AsyncMock(return_value=True)
with (
patch(
"agent.reviewer_publish.get_thread_metadata",
AsyncMock(return_value={"status_comment_id": 99}),
),
patch("agent.reviewer_publish.post_status_comment", post),
patch("agent.reviewer_publish.delete_status_comment", delete),
patch("agent.reviewer_publish.set_reviewer_thread_metadata", AsyncMock()),
):
cid = await post_review_started_comment(
thread_id="tid", owner="o", repo="r", pr_number=7, token="t"
)
assert cid == 500
delete.assert_awaited_once()
assert delete.await_args.kwargs["comment_id"] == 99
@pytest.mark.asyncio
async def test_clear_review_started_comment_deletes_and_clears_metadata() -> None:
delete = AsyncMock(return_value=True)
set_meta = AsyncMock()
with (
patch(
"agent.reviewer_publish.get_thread_metadata",
AsyncMock(return_value={"status_comment_id": 99}),
),
patch("agent.reviewer_publish.delete_status_comment", delete),
patch("agent.reviewer_publish.set_reviewer_thread_metadata", set_meta),
):
await clear_review_started_comment(thread_id="tid", owner="o", repo="r", token="t")
delete.assert_awaited_once()
assert delete.await_args.kwargs["comment_id"] == 99
set_meta.assert_awaited_once_with("tid", extra={"status_comment_id": None})
@pytest.mark.asyncio
async def test_clear_review_started_comment_noop_without_tracked_id() -> None:
delete = AsyncMock()
set_meta = AsyncMock()
with (
patch("agent.reviewer_publish.get_thread_metadata", AsyncMock(return_value={})),
patch("agent.reviewer_publish.delete_status_comment", delete),
patch("agent.reviewer_publish.set_reviewer_thread_metadata", set_meta),
):
await clear_review_started_comment(thread_id="tid", owner="o", repo="r", token="t")
delete.assert_not_called()
set_meta.assert_not_called()
def test_render_review_body_with_only_out_of_diff_findings() -> None:
body = render_review_body(
pr_number=7,
surfaced_count=0,
out_of_diff_findings=[
_f(title="Caller passes stale arg", description="boom", file="x/caller.py")
],
)
assert "No issues found" not in body
assert "found no issues in the changed lines" in body
assert "<details>" in body
assert "1 out-of-diff finding</summary>" in body
assert "**Caller passes stale arg**" in body
assert "`x/caller.py" in body
def test_render_review_body_combines_inline_and_out_of_diff() -> None:
body = render_review_body(
pr_number=7,
surfaced_count=2,
out_of_diff_findings=[_f(title="A"), _f(title="B")],
)
assert "found 2 potential issues." in body
assert "2 out-of-diff findings</summary>" in body
assert "<!-- open-swe-reviewer pr=7 -->" in body
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
def test_render_review_body_includes_trace_link_when_provided() -> None:
body = render_review_body(
pr_number=123,
surfaced_count=0,
trace_url="https://smith.langchain.com/o/t/project/p/t/thread-id",
)
assert "[View Open SWE trace](https://smith.langchain.com/o/t/project/p/t/thread-id)" in body
assert body.endswith("<!-- open-swe-reviewer pr=123 -->")
def test_publish_review_eval_mode_does_not_call_github() -> None:
from agent.tools.publish_review import publish_review
findings = [
_f(id="f_high", severity="high", file="a.py", start_line=1, end_line=1),
_f(id="f_low", severity="low", file="b.py", start_line=2, end_line=2),
]
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",
"reviewer_eval": True,
},
"metadata": {},
},
),
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)),
patch("agent.tools.publish_review.set_reviewer_thread_metadata", AsyncMock()) as set_meta,
patch("agent.tools.publish_review.get_github_token") as get_token,
patch("agent.tools.publish_review.post_pull_request_review", AsyncMock()) as post_review,
):
result = publish_review()
assert result["success"] is True
assert result["dry_run"] is True
assert result["surfaced_count"] == 1
assert result["hidden_count"] == 1
get_token.assert_not_called()
post_review.assert_not_called()
set_meta.assert_awaited_once_with("tid", last_reviewed_sha="sha")
@pytest.mark.asyncio
async def test_publish_review_surfaces_additional_findings_count_in_body() -> None:
"""When all surfaced findings are above threshold but sub-threshold findings
exist, the review body must mention how many additional findings are in the
web app."""
from agent.tools.publish_review import _publish_review_async
findings = [
_f(id="f_low_1", severity="low", file="a.py", start_line=1, end_line=1),
_f(id="f_low_2", severity="low", file="b.py", start_line=2, end_line=2),
]
post_review = AsyncMock(return_value={"id": 555})
fetch_comments = AsyncMock(return_value=[])
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=findings)),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch("agent.tools.publish_review.fetch_review_comments", fetch_comments),
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", AsyncMock()),
patch("agent.tools.publish_review._maybe_post_slack_completion_reply", AsyncMock()),
):
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,
)
assert result["success"] is True
assert result["surfaced_count"] == 0
posted_body = post_review.await_args.kwargs["body"]
assert "No issues found" in posted_body
assert "2 additional findings can be viewed in the web app." in posted_body
def test_publish_review_forwards_trace_link_config_override() -> None:
from agent.tools.publish_review import publish_review
publish_async = AsyncMock(return_value={"success": True})
with (
patch(
"agent.tools.publish_review.get_config",
return_value={
"configurable": {
"thread_id": "reviewer-thread-id",
"repo": {"owner": "o", "name": "r"},
"pr_number": 7,
"head_sha": "sha",
"review_trace_link_enabled": False,
},
"metadata": {},
},
),
patch("agent.tools.publish_review.get_github_token", return_value="token"),
patch("agent.tools.publish_review._publish_review_async", publish_async),
):
result = publish_review()
assert result == {"success": True}
assert publish_async.call_args.kwargs["trace_link_config_override"] is False
@pytest.mark.asyncio
async def test_resolve_review_trace_url_enabled_by_team_setting() -> None:
from agent.tools.publish_review import _resolve_review_trace_url
with (
patch(
"agent.tools.publish_review.get_team_review_trace_links_enabled",
AsyncMock(return_value=True),
),
patch(
"agent.tools.publish_review.get_langsmith_trace_url",
return_value="https://smith/t",
),
):
url = await _resolve_review_trace_url("reviewer-thread-id", None)
assert url == "https://smith/t"
@pytest.mark.asyncio
async def test_resolve_review_trace_url_disabled_by_team_setting() -> None:
from agent.tools.publish_review import _resolve_review_trace_url
trace_url = MagicMock(return_value="https://smith/t")
with (
patch(
"agent.tools.publish_review.get_team_review_trace_links_enabled",
AsyncMock(return_value=False),
),
patch("agent.tools.publish_review.get_langsmith_trace_url", trace_url),
):
url = await _resolve_review_trace_url("reviewer-thread-id", None)
assert url is None
trace_url.assert_not_called()
@pytest.mark.asyncio
async def test_resolve_review_trace_url_config_override_skips_team_lookup() -> None:
from agent.tools.publish_review import _resolve_review_trace_url
team_lookup = AsyncMock(return_value=True)
trace_url = MagicMock(return_value="https://smith/t")
with (
patch("agent.tools.publish_review.get_team_review_trace_links_enabled", team_lookup),
patch("agent.tools.publish_review.get_langsmith_trace_url", trace_url),
):
url = await _resolve_review_trace_url("reviewer-thread-id", False)
assert url is None
team_lookup.assert_not_called()
trace_url.assert_not_called()
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
@pytest.mark.asyncio
async def test_resolve_review_thread_returns_true_on_success() -> None:
response = MagicMock()
response.json.return_value = {
"data": {"resolveReviewThread": {"thread": {"id": "T_1", "isResolved": True}}}
}
response.raise_for_status.return_value = None
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.post = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
ok = await resolve_review_thread(thread_node_id="T_1", token="t")
assert ok is True
@pytest.mark.asyncio
async def test_fetch_pr_review_threads_handles_null_repository() -> None:
"""GitHub returns ``repository: null`` when the token can't read the repo
(SAML, expired token, private/deleted). ``dict.get(k, {})`` does not coalesce
explicit null, so the fetch must guard against it and return collected threads."""
response = MagicMock()
response.json.return_value = {"data": {"repository": None}}
response.raise_for_status.return_value = None
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.post = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
threads = await fetch_pr_review_threads(owner="o", repo="r", pr_number=1, token="t")
assert threads == []
@pytest.mark.asyncio
async def test_post_pull_request_review_non_dict_body_surfaces_status_and_excerpt() -> None:
"""A non-dict GitHub response body must surface status code + body excerpt
via ``_error`` rather than collapsing to a bare ``None`` (which the
user-facing tool would render as the unhelpful ``Failed to POST PR review``)."""
response = MagicMock()
response.status_code = 200
response.json.return_value = ["unexpected", "list", "body"]
response.text = '["unexpected", "list", "body"]'
response.raise_for_status.return_value = None
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.post = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
result = await post_pull_request_review(
owner="o",
repo="r",
pr_number=1,
head_sha="sha",
body="b",
inline_comments=[],
token="t",
)
assert isinstance(result, dict)
assert "_error" in result
err = result["_error"]
assert "HTTP 200" in err
assert "non-dict" in err
assert "unexpected" in err
# The bare legacy string must not be the only signal anymore.
assert err != "Failed to POST PR review"
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
@pytest.mark.asyncio
async def test_resolve_review_thread_returns_false_on_graphql_errors() -> None:
response = MagicMock()
response.json.return_value = {"errors": [{"message": "no perms"}]}
response.raise_for_status.return_value = None
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.post = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
ok = await resolve_review_thread(thread_node_id="T_1", token="t")
assert ok is False
@pytest.mark.asyncio
async def test_publish_review_skips_findings_already_published() -> None:
"""Re-runs must not re-post findings that already have a github_review_comment_id."""
from agent.tools.publish_review import _publish_review_async
findings = [
_f(id="f_old", severity="high", file="a.py", github_review_comment_id=42),
_f(id="f_new", severity="high", file="b.py"),
]
list_async = AsyncMock(return_value=findings)
post_review = AsyncMock(return_value={"id": 999})
fetch_comments = AsyncMock(return_value=[])
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", list_async),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch("agent.tools.publish_review.fetch_review_comments", fetch_comments),
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,
),
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
):
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,
feat: implement reviewer findings, publish_review, and watch mode (#1253) * feat: implement reviewer findings, publish_review, and watch mode Build out the reviewer agent end-to-end against the design in REVIEWER_DESIGN.md: - Findings as first-class state on the reviewer thread metadata (`agent/reviewer_findings.py`): Finding TypedDict with start_line/end_line ranges, suggestion text for ```suggestion blocks, github_review_comment_id for cross-run reconciliation, diff_hunk for UI rendering. Thread-level metadata gets `kind=reviewer`, `pr`, `last_reviewed_sha`, `watch` so a future frontend can list reviewer threads via the langgraph SDK. - Diff utilities (`agent/reviewer_diff.py`): parse_unified_diff, compute_diff_line_set for in-diff validation, extract_diff_hunk for caching the hunk on a Finding, compute_diff_in_sandbox for SHA-to-SHA diffs against the prepped repo. - Tools: `add_finding` (validates against the diff line set so out-of-diff ranges fail at creation, not at GitHub-publish), `update_finding`, `list_findings`, `publish_review`. The reviewer agent's tool list is swapped from `[]` (direct shell `gh api` calls) to these four. - Publish path (`agent/reviewer_publish.py` + `agent/tools/publish_review.py`): one POST /reviews call with body + inline comments + ```suggestion blocks, per-comment IDs stored back on findings, GraphQL `resolveReviewThread` fired for findings transitioning open->resolved on a re-review. - Reviewer graph: deterministic clone-or-fetch + checkout in the factory before the agent's first model call (warm- and cold-path symmetric); computed diff and in-diff line set passed via runnable config; system prompt rewritten for the single-evolving-findings model, severity ladder, in-diff-only discipline, and watch-mode reconciliation flow. - Watch mode in webapp.py: `push` event + `pull_request` closed/reopened added to supported events. New `process_github_push_event` resolves the open PR for the pushed branch, gates on the reviewer thread's `watch` flag, builds a re-review configurable, and triggers a run on the same canonical thread. `process_github_pr_close` toggles watch on closed/reopened. `set_reviewer_thread_metadata` is called on first review to install `kind=reviewer` + PR identity + watch=True. - Eval harness: target.py now extracts `add_finding` calls (mapped to the legacy {file, line, body, severity} shape the judge expects) and passes the right configurable so the prep step has base/head SHAs. - Tests: new unit suites for findings helpers, diff parsing, finding tools, publish rendering + GraphQL resolve, and watch-mode webhook handlers (push triggers re-review only when watching, idempotent on unchanged head SHA, PR close disables watch). Updated existing reviewer-webhook tests to mock `set_reviewer_thread_metadata`. - REVIEWER_EVAL_PLAN.md removed per user request; folded relevant context into REVIEWER_DESIGN.md. * fix(reviewer): correct git diff flags, scope, dedup, and review-comments URL Address PR #1253 review findings: - compute_diff_in_sandbox dropped the invalid `--no-prefix=false` flag (`option no-prefix takes no value` — every prep run was failing silently and the agent saw an empty diff). - compute_diff_in_sandbox grew a `merge_base` flag. First-review path now uses three-dot `base...head` (the merge-base diff GitHub renders on Files-changed) so we don't pick up changes that landed on the base branch after the PR diverged. Re-review delta keeps two-dot `last_reviewed_sha..head` since that's exactly the new commits. - publish_review skips findings that already carry `github_review_comment_id`. Without this, watched re-reviews re-posted every previously surfaced finding, and only the most-recent duplicate's id would later resolve when the issue got addressed. - fetch_review_comments URL now includes `{pull_number}` — `/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments` is the canonical endpoint; the old form 404s, so comment ids were never stored and watch-mode resolution couldn't run. Three new tests cover: three-dot vs two-dot wiring, no `--no-prefix` flag in the executed command, and that publish_review does not re-post findings whose `github_review_comment_id` is set. * fix(reviewer): default publish cap from 15 to 4 A clean PR with one critical issue padded out by three lower-severity findings is fine; fifteen is review spam. The agent can override per call when a PR genuinely warrants more.
2026-05-07 14:48:43 -07:00
)
assert result["success"] is True
assert result["surfaced_count"] == 1
posted = post_review.await_args.kwargs["inline_comments"]
paths = {c["path"] for c in posted}
assert paths == {"b.py"}
fix(open-swe): always post a review summary, even with no findings (#1256) * fix(reviewer): log every push/close early-return so 'silent ignore' is debuggable Pushes to PRs that haven't had a first review fall through the watch handler because the reviewer thread doesn't have kind=reviewer set. Without log lines on the early-return paths, this scenario was indistinguishable from 'webhook reached the handler at all' in the hosted log stream. Now every early-return logs at info or debug: - info when a real PR exists but the reviewer thread isn't set up (with a hint pointing at the trigger paths the user can use) - info when the repo isn't in the reviewer allowlist - debug for benign skips (non-branch refs, branch deletions, already-reviewed head_sha) * fix(reviewer): always post a summary review, even with no findings The publish_review tool gated POSTing on `inline_comments or summary`, so when the agent called publish_review() with no args on a clean PR the result returned `success: true` but no GitHub review was posted — the user got silence instead of a "no issues found" comment. - Drop the gate so publish_review always POSTs. - Friendlier no-findings render: `**No issues found.**` when the findings list is empty, vs. `**No issues at or above \`<sev>\` severity.**` with hidden count when only sub-threshold findings exist. Agent summary renders below. - Prompt now requires the agent to always pass a `summary` so the body is meaningful; calls out specifically not to skip on a clean PR.
2026-05-07 15:16:20 -07:00
feat: auto-review PRs on opened / ready-for-review (#1325) * feat: auto-review PRs on opened / ready-for-review Trigger Open SWE Review on `pull_request` actions `opened` and `ready_for_review` against the canonical reviewer thread (no need to request open-swe[bot] as a reviewer). `converted_to_draft` now also flips watch=False on the existing reviewer thread. Draft PRs are gated by a tri-state user setting on the profile: inherit team default, always on, or always off. The team-wide `review_draft_prs` setting is the org-wide default; each user can override it in My Settings. External contributors with no Open SWE profile fall back to the team default. * fix: PR review comments — auth source + draft-aware watch toggle - `process_github_pr_ready` now dispatches with `source="github"` so the auth resolver finds the bot token persisted on the thread. The previous `source="github_auto"` fell through to the email-based path in non bot-token-only deployments and failed with a missing-user-email error. - `converted_to_draft` no longer unconditionally clears `watch`. When the PR author's effective `review_draft_prs` setting is on, watch stays on so subsequent pushes still trigger re-reviews while the PR is in draft. * feat(reviewer): skip "no issues found" comment on empty re-reviews A re-review run with no new findings to surface no longer posts another "Open SWE Review: No issues found" comment on the PR. The "no issues" summary now only appears on the first review of a PR — matching Devin's behavior, where subsequent reviews are silent unless there's something new to flag. Resolved-thread reconciliation and ``last_reviewed_sha`` persistence still happen on the skipped path, so findings the user just fixed still get their GitHub threads marked resolved, and the next push event sees an up-to-date dedup SHA. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-22 13:36:14 -07:00
@pytest.mark.asyncio
async def test_publish_review_skips_post_on_re_review_with_no_new_findings() -> None:
"""Re-review with nothing new to surface must not spam another comment."""
from agent.tools.publish_review import _publish_review_async
# All findings already have github_review_comment_id from the prior publish
# (so none are "unpublished"), plus one previously-resolved finding whose
# thread still needs to be resolved on GitHub.
findings = [
{
"id": "f_old",
"severity": "high",
"category": "correctness",
"file": "a.py",
"start_line": 1,
"end_line": 1,
"side": "RIGHT",
"description": "x",
"suggestion": None,
"status": "resolved",
"first_seen_sha": "s",
"last_confirmed_sha": "s",
"github_review_comment_id": 100,
},
]
list_async = AsyncMock(return_value=findings)
post_review = AsyncMock()
set_metadata = AsyncMock()
resolve_threads = AsyncMock(return_value=1)
with (
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
patch("agent.tools.publish_review.list_findings_async", list_async),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch(
"agent.tools.publish_review._resolve_threads_for_resolved_findings",
resolve_threads,
),
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="newsha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=True,
)
post_review.assert_not_called()
resolve_threads.assert_awaited_once()
set_metadata.assert_awaited_once()
assert result["success"] is True
assert result["review_id"] is None
assert result["surfaced_count"] == 0
assert result["resolved_thread_count"] == 1
assert result["skipped_empty_re_review"] is True
@pytest.mark.asyncio
async def test_publish_review_does_not_surface_out_of_diff_finding() -> None:
"""Out-of-diff findings are disabled: a finding anchored outside the diff is
never surfaced on the PR. On a re-review with nothing else to post, it is
treated as an empty re-review."""
from agent.tools.publish_review import _publish_review_async
findings = [
_f(
id="f_ood",
file="caller.py",
in_diff=False,
first_seen_sha="newsha",
github_review_comment_id=None,
github_review_id=None,
)
]
post_review = AsyncMock(return_value={"id": 555})
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=findings)),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch(
"agent.tools.publish_review._open_swe_already_reviewed",
AsyncMock(return_value=True),
),
patch(
"agent.tools.publish_review._resolve_threads_for_resolved_findings",
AsyncMock(return_value=0),
),
patch("agent.tools.publish_review.set_reviewer_thread_metadata", AsyncMock()),
patch("agent.tools.publish_review.clear_review_started_comment", AsyncMock()),
patch("agent.tools.publish_review.settle_review_check_run", AsyncMock()),
patch("agent.tools.publish_review._maybe_post_slack_completion_reply", AsyncMock()),
patch("agent.tools.publish_review._resolve_review_trace_url", AsyncMock(return_value=None)),
):
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="newsha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=True,
)
post_review.assert_not_called()
assert result["success"] is True
assert result["review_id"] is None
assert result["surfaced_count"] == 0
assert "out_of_diff_count" not in result
assert result["skipped_empty_re_review"] is True
@pytest.mark.asyncio
async def test_publish_review_skips_duplicate_empty_summary_when_open_swe_already_reviewed() -> (
None
):
"""A push landing mid-run is queued into the still-running first-review run,
whose configurable still says re_review=False. With nothing to surface, the
empty-review guard must key off the existing Open SWE review summary on the
PR (not the stale flag) so it does not post a duplicate "No issues found"."""
from agent.tools.publish_review import _publish_review_async
post_review = AsyncMock()
set_metadata = AsyncMock()
resolve_threads = AsyncMock(return_value=0)
review_exists = AsyncMock(return_value=True)
slack_reply = 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=[])),
patch("agent.tools.publish_review.open_swe_review_exists", review_exists),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch("agent.tools.publish_review._resolve_threads_for_resolved_findings", resolve_threads),
patch("agent.tools.publish_review.set_reviewer_thread_metadata", set_metadata),
patch("agent.tools.publish_review._maybe_post_slack_completion_reply", slack_reply),
):
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="newsha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=False,
)
post_review.assert_not_called()
review_exists.assert_awaited_once()
resolve_threads.assert_awaited_once()
slack_reply.assert_not_called()
assert result["success"] is True
assert result["review_id"] is None
assert result["surfaced_count"] == 0
assert result["skipped_empty_re_review"] is True
set_metadata.assert_awaited_once_with("tid", last_reviewed_sha="newsha")
fix: reviewer publishes against stale head_sha on mid-run re-review (#1393) * 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.
2026-06-03 11:38:56 -07:00
@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
empty-review guard must short-circuit without an extra reviews API call."""
from agent.tools.publish_review import _publish_review_async
review_exists = AsyncMock(return_value=True)
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=[])),
patch("agent.tools.publish_review.open_swe_review_exists", review_exists),
patch("agent.tools.publish_review.post_pull_request_review", AsyncMock()) as post_review,
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", new_callable=AsyncMock),
):
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="newsha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=True,
)
review_exists.assert_not_called()
post_review.assert_not_called()
assert result["skipped_empty_re_review"] is True
@pytest.mark.asyncio
async def test_publish_review_dedup_keys_off_durable_last_reviewed_sha() -> None:
"""A non-empty ``last_reviewed_sha`` on thread metadata means this thread
already published once. The empty-summary guard must trust that durable
signal and suppress without ever hitting the reviews API."""
from agent.tools.publish_review import _publish_review_async
review_exists = AsyncMock(return_value=False)
post_review = 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=[])),
patch(
"agent.tools.publish_review.get_thread_metadata",
AsyncMock(return_value={"last_reviewed_sha": "oldsha"}),
),
patch("agent.tools.publish_review.open_swe_review_exists", review_exists),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
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", new_callable=AsyncMock),
):
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="newsha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=False,
)
review_exists.assert_not_called()
post_review.assert_not_called()
assert result["skipped_empty_re_review"] is True
@pytest.mark.asyncio
async def test_publish_review_posts_summary_when_review_existence_unknown() -> None:
"""When the reviews API can't answer (``open_swe_review_exists`` returns
``None``) and there is no durable prior-review signal, the guard must NOT
suppress — re-posting the summary is the safe failure mode, never silently
swallowing the only review the user sees."""
from agent.tools.publish_review import _publish_review_async
review_exists = AsyncMock(return_value=None)
post_review = AsyncMock(return_value={"id": 321})
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=[])),
patch(
"agent.tools.publish_review.get_thread_metadata",
AsyncMock(return_value={}),
),
patch("agent.tools.publish_review.open_swe_review_exists", review_exists),
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", new_callable=AsyncMock),
patch("agent.tools.publish_review._maybe_post_slack_completion_reply", AsyncMock()),
):
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="newsha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=False,
)
review_exists.assert_awaited_once()
post_review.assert_awaited_once()
assert "skipped_empty_re_review" not in result
assert result["review_id"] == 321
@pytest.mark.asyncio
async def test_open_swe_review_exists_detects_summary_marker() -> None:
response = MagicMock()
response.json.return_value = [
{"id": 1, "body": "some human review"},
{"id": 2, "body": f"## ✅ Open SWE Review\n\n{review_summary_marker(7)}"},
]
response.raise_for_status.return_value = None
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.get = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
exists = await open_swe_review_exists(owner="o", repo="r", pr_number=7, token="t")
assert exists is True
@pytest.mark.asyncio
async def test_open_swe_review_exists_false_without_marker() -> None:
response = MagicMock()
response.json.return_value = [{"id": 1, "body": "looks good to me"}]
response.raise_for_status.return_value = None
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.get = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
exists = await open_swe_review_exists(owner="o", repo="r", pr_number=7, token="t")
assert exists is False
@pytest.mark.asyncio
async def test_open_swe_review_exists_returns_none_on_http_error() -> None:
"""A failed reviews API call is reported as ``None`` (unknown), never
``False`` — the empty-summary dedup must not treat a transient failure as
"no prior review exists" and double-post."""
import httpx
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.get = AsyncMock(side_effect=httpx.HTTPError("boom"))
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
exists = await open_swe_review_exists(owner="o", repo="r", pr_number=7, token="t")
assert exists is None
@pytest.mark.asyncio
async def test_re_review_backfills_existing_marker_and_skips_duplicate_post() -> None:
from agent.tools.publish_review import _publish_review_async
finding = _f(id="f_old", first_seen_sha="oldsha", github_review_comment_id=None)
findings = [finding]
thread = {
"id": "THREAD_1",
"is_resolved": False,
"is_outdated": False,
"comments": [
{
"id": 101,
"author": "open-swe[bot]",
"body": render_inline_comment_body(finding),
"created_at": "2026-05-27T10:00:00Z",
}
],
}
post_review = AsyncMock()
with (
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
patch(
"agent.tools.publish_review.fetch_pr_review_threads", AsyncMock(return_value=[thread])
),
patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)),
patch("agent.reviewer_reconcile.list_findings", AsyncMock(return_value=findings)),
patch("agent.reviewer_reconcile.replace_findings", AsyncMock()),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
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", new_callable=AsyncMock),
):
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="newsha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=True,
)
post_review.assert_not_called()
assert result["skipped_empty_re_review"] is True
assert findings[0]["github_review_comment_id"] == 101
assert findings[0]["github_review_thread_id"] == "THREAD_1"
@pytest.mark.asyncio
async def test_re_review_backfills_and_resolves_duplicate_existing_threads() -> None:
from agent.tools.publish_review import _publish_review_async
finding = _f(
id="f_old",
first_seen_sha="oldsha",
github_review_comment_id=None,
status="resolved",
resolution_note="The duplicate threads are fixed by the latest commit.",
)
findings = [finding]
threads = [
{
"id": "THREAD_1",
"is_resolved": False,
"is_outdated": False,
"comments": [
{
"id": 101,
"author": "open-swe[bot]",
"body": render_inline_comment_body(finding),
"created_at": "2026-05-27T10:00:00Z",
}
],
},
{
"id": "THREAD_2",
"is_resolved": False,
"is_outdated": False,
"comments": [
{
"id": 102,
"author": "open-swe[bot]",
"body": render_inline_comment_body(finding),
"created_at": "2026-05-27T10:01:00Z",
}
],
},
]
resolve_thread = AsyncMock(return_value=True)
reply_comment = AsyncMock(return_value={"id": 555})
with (
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
patch(
"agent.tools.publish_review.fetch_pr_review_threads", AsyncMock(return_value=threads)
),
patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)),
patch("agent.reviewer_reconcile.list_findings", AsyncMock(return_value=findings)),
patch("agent.reviewer_reconcile.replace_findings", AsyncMock()),
patch("agent.tools.publish_review.post_pull_request_review", AsyncMock()),
patch("agent.tools.publish_review.resolve_review_thread", resolve_thread),
patch(
"agent.tools.publish_review.fetch_review_thread_id_for_comment",
AsyncMock(return_value=None),
),
patch("agent.tools.publish_review.reply_to_review_comment", reply_comment),
patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock),
):
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="newsha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=True,
)
assert result["success"] is True
assert result["review_id"] is None
assert result["resolved_thread_count"] == 2
assert resolve_thread.await_count == 2
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."
)
assert findings[0]["github_review_comment_ids"] == [101, 102]
assert findings[0]["github_review_thread_ids"] == ["THREAD_1", "THREAD_2"]
assert findings[0]["github_resolved_thread_ids"] == ["THREAD_1", "THREAD_2"]
assert findings[0]["github_posted_resolution_comment_ids"] == [101, 102]
assert findings[0]["github_thread_resolved"] is True
@pytest.mark.asyncio
async def test_publish_review_backfills_from_threads_when_review_comments_are_empty() -> None:
from agent.tools.publish_review import _publish_review_async
finding = _f(id="f_new", first_seen_sha="sha")
findings = [finding]
thread = {
"id": "THREAD_1",
"is_resolved": False,
"is_outdated": False,
"comments": [
{
"id": 202,
"author": "open-swe[bot]",
"body": render_inline_comment_body(finding),
"created_at": "2026-05-27T10:00:00Z",
}
],
}
fetch_threads = AsyncMock(side_effect=[[], [thread]])
with (
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
patch("agent.tools.publish_review.fetch_pr_review_threads", fetch_threads),
patch("agent.tools.publish_review.list_findings_async", AsyncMock(return_value=findings)),
patch("agent.reviewer_reconcile.list_findings", AsyncMock(return_value=findings)),
patch("agent.reviewer_reconcile.replace_findings", AsyncMock()),
patch(
"agent.tools.publish_review.post_pull_request_review",
AsyncMock(return_value={"id": 999}),
),
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", new_callable=AsyncMock),
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="sha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=False,
)
assert result["success"] is True
assert result["review_id"] == 999
assert fetch_threads.await_count == 2
assert findings[0]["github_review_id"] == 999
assert findings[0]["github_review_comment_id"] == 202
assert findings[0]["github_review_thread_id"] == "THREAD_1"
@pytest.mark.asyncio
async def test_re_review_only_posts_current_head_unpublished_findings() -> None:
from agent.tools.publish_review import _publish_review_async
old = _f(id="f_old", first_seen_sha="oldsha", file="old.py")
new = _f(id="f_new", first_seen_sha="newsha", file="new.py")
findings = [old, new]
post_review = AsyncMock(return_value={"id": 888})
fetch_comments = AsyncMock(
return_value=[
{
"id": 303,
"path": "new.py",
"line": 10,
"body": render_inline_comment_body(new),
}
]
)
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=findings)),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch("agent.tools.publish_review.fetch_review_comments", fetch_comments),
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", new_callable=AsyncMock),
):
result = await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="newsha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=True,
)
assert result["success"] is True
assert result["surfaced_count"] == 1
inline_comments = post_review.await_args.kwargs["inline_comments"]
assert [comment["path"] for comment in inline_comments] == ["new.py"]
assert old["github_review_id"] is None
assert new["github_review_id"] == 888
assert new["github_review_comment_id"] == 303
@pytest.mark.asyncio
async def test_publish_review_matches_comment_ids_by_marker_not_path_line_body() -> None:
"""Two findings on the same path/line with identical rendered bodies must
each get their OWN comment id, matched via the embedded marker. The old
``(path, line, body)`` fallback collided here and cached one comment id on
both findings, breaking resolve-on-fix."""
from agent.tools.publish_review import _publish_review_async
f1 = _f(id="f_one", file="dup.py", start_line=5, end_line=5, description="same text")
f2 = _f(id="f_two", file="dup.py", start_line=5, end_line=5, description="same text")
findings = [f1, f2]
post_review = AsyncMock(return_value={"id": 700})
# GitHub returns one comment per finding; the only thing that distinguishes
# them is the marker embedded in each body.
fetch_comments = AsyncMock(
return_value=[
{"id": 901, "path": "dup.py", "line": 5, "body": render_inline_comment_body(f1)},
{"id": 902, "path": "dup.py", "line": 5, "body": render_inline_comment_body(f2)},
]
)
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=findings)),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch("agent.tools.publish_review.fetch_review_comments", fetch_comments),
patch(
"agent.tools.publish_review._resolve_threads_for_resolved_findings",
new_callable=AsyncMock,
return_value=0,
),
patch("agent.tools.publish_review._store_thread_ids_on_findings", new_callable=AsyncMock),
patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock),
patch("agent.tools.publish_review._maybe_post_slack_completion_reply", AsyncMock()),
):
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,
)
assert result["success"] is True
by_id = {f["id"]: f for f in findings}
assert by_id["f_one"]["github_review_comment_id"] == 901
assert by_id["f_two"]["github_review_comment_id"] == 902
@pytest.mark.asyncio
async def test_publish_review_records_review_id_and_comment_id_in_single_write() -> None:
"""The post-publish bookkeeping stamps review id + comment id onto findings
in one ``replace_findings`` call, so a finding is never persisted with a
review id but no comment id."""
from agent.tools.publish_review import _publish_review_async
finding = _f(id="f_new", file="x.py", start_line=3, end_line=3)
findings = [finding]
post_review = AsyncMock(return_value={"id": 555})
fetch_comments = AsyncMock(
return_value=[
{"id": 808, "path": "x.py", "line": 3, "body": render_inline_comment_body(finding)},
]
)
replace = 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=findings)),
patch("agent.tools.publish_review.replace_findings", replace),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch("agent.tools.publish_review.fetch_review_comments", fetch_comments),
patch("agent.tools.publish_review._store_thread_ids_on_findings", new_callable=AsyncMock),
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", new_callable=AsyncMock),
patch("agent.tools.publish_review._maybe_post_slack_completion_reply", AsyncMock()),
):
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,
)
assert result["success"] is True
# Exactly one persisted snapshot carries both ids together — never a
# half-stamped intermediate state.
persisted_snapshots = [call.args[1] for call in replace.await_args_list]
assert any(
snap[0].get("github_review_id") == 555 and snap[0].get("github_review_comment_id") == 808
for snap in persisted_snapshots
)
assert all(
not (
snap[0].get("github_review_id") == 555
and snap[0].get("github_review_comment_id") is None
)
for snap in persisted_snapshots
)
fix(open-swe): always post a review summary, even with no findings (#1256) * fix(reviewer): log every push/close early-return so 'silent ignore' is debuggable Pushes to PRs that haven't had a first review fall through the watch handler because the reviewer thread doesn't have kind=reviewer set. Without log lines on the early-return paths, this scenario was indistinguishable from 'webhook reached the handler at all' in the hosted log stream. Now every early-return logs at info or debug: - info when a real PR exists but the reviewer thread isn't set up (with a hint pointing at the trigger paths the user can use) - info when the repo isn't in the reviewer allowlist - debug for benign skips (non-branch refs, branch deletions, already-reviewed head_sha) * fix(reviewer): always post a summary review, even with no findings The publish_review tool gated POSTing on `inline_comments or summary`, so when the agent called publish_review() with no args on a clean PR the result returned `success: true` but no GitHub review was posted — the user got silence instead of a "no issues found" comment. - Drop the gate so publish_review always POSTs. - Friendlier no-findings render: `**No issues found.**` when the findings list is empty, vs. `**No issues at or above \`<sev>\` severity.**` with hidden count when only sub-threshold findings exist. Agent summary renders below. - Prompt now requires the agent to always pass a `summary` so the body is meaningful; calls out specifically not to skip on a clean PR.
2026-05-07 15:16:20 -07:00
@pytest.mark.asyncio
async def test_publish_review_posts_summary_when_no_findings() -> None:
"""An empty findings list must still post a review so the user sees feedback."""
from agent.tools.publish_review import _publish_review_async
list_async = AsyncMock(return_value=[])
post_review = AsyncMock(return_value={"id": 555})
fetch_comments = AsyncMock(return_value=[])
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", list_async),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch("agent.tools.publish_review.fetch_review_comments", fetch_comments),
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,
),
fix(open-swe): always post a review summary, even with no findings (#1256) * fix(reviewer): log every push/close early-return so 'silent ignore' is debuggable Pushes to PRs that haven't had a first review fall through the watch handler because the reviewer thread doesn't have kind=reviewer set. Without log lines on the early-return paths, this scenario was indistinguishable from 'webhook reached the handler at all' in the hosted log stream. Now every early-return logs at info or debug: - info when a real PR exists but the reviewer thread isn't set up (with a hint pointing at the trigger paths the user can use) - info when the repo isn't in the reviewer allowlist - debug for benign skips (non-branch refs, branch deletions, already-reviewed head_sha) * fix(reviewer): always post a summary review, even with no findings The publish_review tool gated POSTing on `inline_comments or summary`, so when the agent called publish_review() with no args on a clean PR the result returned `success: true` but no GitHub review was posted — the user got silence instead of a "no issues found" comment. - Drop the gate so publish_review always POSTs. - Friendlier no-findings render: `**No issues found.**` when the findings list is empty, vs. `**No issues at or above \`<sev>\` severity.**` with hidden count when only sub-threshold findings exist. Agent summary renders below. - Prompt now requires the agent to always pass a `summary` so the body is meaningful; calls out specifically not to skip on a clean PR.
2026-05-07 15:16:20 -07:00
):
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,
fix(open-swe): always post a review summary, even with no findings (#1256) * fix(reviewer): log every push/close early-return so 'silent ignore' is debuggable Pushes to PRs that haven't had a first review fall through the watch handler because the reviewer thread doesn't have kind=reviewer set. Without log lines on the early-return paths, this scenario was indistinguishable from 'webhook reached the handler at all' in the hosted log stream. Now every early-return logs at info or debug: - info when a real PR exists but the reviewer thread isn't set up (with a hint pointing at the trigger paths the user can use) - info when the repo isn't in the reviewer allowlist - debug for benign skips (non-branch refs, branch deletions, already-reviewed head_sha) * fix(reviewer): always post a summary review, even with no findings The publish_review tool gated POSTing on `inline_comments or summary`, so when the agent called publish_review() with no args on a clean PR the result returned `success: true` but no GitHub review was posted — the user got silence instead of a "no issues found" comment. - Drop the gate so publish_review always POSTs. - Friendlier no-findings render: `**No issues found.**` when the findings list is empty, vs. `**No issues at or above \`<sev>\` severity.**` with hidden count when only sub-threshold findings exist. Agent summary renders below. - Prompt now requires the agent to always pass a `summary` so the body is meaningful; calls out specifically not to skip on a clean PR.
2026-05-07 15:16:20 -07:00
)
assert result["success"] is True
assert result["surfaced_count"] == 0
assert result["review_id"] == 555
post_review.assert_awaited_once()
posted_body = post_review.await_args.kwargs["body"]
posted_inline = post_review.await_args.kwargs["inline_comments"]
assert posted_inline == []
assert "No issues found" in posted_body
@pytest.mark.asyncio
async def test_publish_review_posts_slack_reply_on_first_review_with_slack_ref() -> None:
"""A first review with a slack_thread metadata ref posts a one-line summary."""
from agent.tools.publish_review import _publish_review_async
metadata = {
"kind": "reviewer",
"slack_thread": {"channel_id": "C1", "thread_ts": "1234.5"},
}
slack_post = AsyncMock(return_value=True)
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=[])),
patch(
"agent.tools.publish_review.post_pull_request_review",
AsyncMock(return_value={"id": 42}),
),
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", new_callable=AsyncMock),
patch(
"agent.tools.publish_review.get_thread_metadata",
new_callable=AsyncMock,
return_value=metadata,
),
patch("agent.tools.publish_review.post_slack_thread_reply", slack_post),
):
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,
)
slack_post.assert_awaited_once()
args = slack_post.await_args.args
assert args[0] == "C1"
assert args[1] == "1234.5"
assert "No issues found" in args[2]
assert "https://github.com/o/r/pull/7#pullrequestreview-42" in args[2]
@pytest.mark.asyncio
async def test_publish_review_uses_plural_findings_in_slack_reply() -> None:
"""Surfaced count > 1 should pluralize 'issues' in the slack summary."""
from agent.tools.publish_review import _publish_review_async
findings = [
_f(id="f1", file="a.py", start_line=1, end_line=1),
_f(id="f2", file="b.py", start_line=2, end_line=2),
]
metadata = {
"kind": "reviewer",
"slack_thread": {"channel_id": "C1", "thread_ts": "1234.5"},
}
slack_post = AsyncMock(return_value=True)
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=findings)),
patch(
"agent.tools.publish_review.post_pull_request_review",
AsyncMock(return_value={"id": 99}),
),
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", new_callable=AsyncMock),
patch(
"agent.tools.publish_review.get_thread_metadata",
new_callable=AsyncMock,
return_value=metadata,
),
patch("agent.tools.publish_review.post_slack_thread_reply", slack_post),
):
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,
)
slack_post.assert_awaited_once()
text = slack_post.await_args.args[2]
assert "found 2 potential issues" in text
@pytest.mark.asyncio
async def test_publish_review_skips_slack_reply_on_re_review() -> None:
"""Re-reviews must NOT post to Slack even when slack_thread metadata is set."""
from agent.tools.publish_review import _publish_review_async
metadata = {
"kind": "reviewer",
"slack_thread": {"channel_id": "C1", "thread_ts": "1234.5"},
}
slack_post = AsyncMock(return_value=True)
get_metadata = AsyncMock(return_value=metadata)
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=[])),
patch(
"agent.tools.publish_review.post_pull_request_review",
AsyncMock(return_value={"id": 1}),
),
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", new_callable=AsyncMock),
patch("agent.tools.publish_review.get_thread_metadata", get_metadata),
patch("agent.tools.publish_review.post_slack_thread_reply", slack_post),
):
await _publish_review_async(
owner="o",
repo="r",
pr_number=7,
head_sha="sha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=True,
)
slack_post.assert_not_awaited()
# Re-review path should also avoid even fetching the slack metadata.
get_metadata.assert_not_awaited()
@pytest.mark.asyncio
async def test_publish_review_skips_slack_reply_when_no_slack_ref() -> None:
"""A review started from GitHub (no slack_thread metadata) must not post to Slack."""
from agent.tools.publish_review import _publish_review_async
slack_post = AsyncMock(return_value=True)
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=[])),
patch(
"agent.tools.publish_review.post_pull_request_review",
AsyncMock(return_value={"id": 1}),
),
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", new_callable=AsyncMock),
patch(
"agent.tools.publish_review.get_thread_metadata",
new_callable=AsyncMock,
return_value={"kind": "reviewer"},
),
patch("agent.tools.publish_review.post_slack_thread_reply", slack_post),
):
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,
)
slack_post.assert_not_awaited()
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
@pytest.mark.asyncio
async def test_fetch_pr_review_threads_parses_threads_and_comments() -> None:
"""GraphQL response is mapped into the simplified thread dicts."""
response = MagicMock()
response.json.return_value = {
"data": {
"repository": {
"pullRequest": {
"reviewThreads": {
"pageInfo": {"hasNextPage": False, "endCursor": None},
"nodes": [
{
"id": "THREAD_1",
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
"isResolved": True,
"isOutdated": False,
"path": "a/b.py",
"line": 37,
"originalLine": 37,
"comments": {
"nodes": [
{
"databaseId": 101,
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
"author": {"login": "open-swe[bot]"},
"authorAssociation": "MEMBER",
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
"body": "additionalTtlPrefixes removes lifecycle rules",
"createdAt": "2026-05-23T10:00:00Z",
},
{
"databaseId": 102,
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
"author": {"login": "human"},
"authorAssociation": "MEMBER",
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
"body": "We added defaults in the template",
"createdAt": "2026-05-24T11:00:00Z",
},
]
},
},
{
"id": "THREAD_2",
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
"isResolved": False,
"isOutdated": False,
"path": "c.py",
"line": 9,
"originalLine": None,
"comments": {
"nodes": [
{
"databaseId": 201,
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
"author": {"login": "rev"},
"authorAssociation": "CONTRIBUTOR",
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
"body": "this looks fishy",
"createdAt": "2026-05-24T12:00:00Z",
}
]
},
},
],
}
}
}
}
}
response.raise_for_status.return_value = None
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.post = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
threads = await fetch_pr_review_threads(owner="o", repo="r", pr_number=1, token="t")
assert len(threads) == 2
assert threads[0]["id"] == "THREAD_1"
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
assert threads[0]["path"] == "a/b.py"
assert threads[0]["is_resolved"] is True
assert threads[0]["line"] == 37
assert len(threads[0]["comments"]) == 2
assert threads[0]["comments"][0]["id"] == 101
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
assert threads[0]["comments"][1]["author"] == "human"
assert "added defaults" in threads[0]["comments"][1]["body"]
assert threads[1]["is_resolved"] is False
@pytest.mark.asyncio
async def test_fetch_pr_review_threads_returns_empty_on_http_error() -> None:
import httpx
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.post = AsyncMock(side_effect=httpx.HTTPError("boom"))
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
fix: inject existing PR review threads into reviewer context (#1331) * fix(reviewer): inject existing PR review threads into reviewer context The reviewer agent was filing the same inline comment on every re-review because it only saw findings recorded on its own thread metadata — not the live PR review-thread state on GitHub. When a previous finding was still open (code unchanged, or a human reply explained it), the agent rediscovered the same defect on the next push and called `add_finding` again, producing duplicate comments. This change fetches the PR's review threads (across all reviewers, with replies and isResolved status) via GraphQL and renders them into the first-review and re-review contexts as a "Pre-existing PR review threads" block. The system prompt now lists overlap with that block as a hard "Do NOT file" rule, and treats threads addressed by a human reply as resolved. This also gives the reviewer comment-awareness on its very first run on a PR, so it skips findings already raised by another reviewer or bot. * fix(reviewer): wrap PR review threads in untrusted-data XML block Addresses the reviewer comment on this PR (https://github.com/langchain-ai/open-swe/pull/1331#discussion_r3295497533): PR review comment bodies are attacker-controlled (anyone who can comment on the PR can put anything in them), and they were being concatenated into the reviewer's system prompt with instruction-priority. Switches the existing-threads section from a Markdown block to an XML data block: <pr_review_threads> <thread location="path:line" status="open"> <comment author="open-swe[bot]"> <body>...</body> </comment> <comment author="romain-priour-lc"> <body>We added defaults in the template</body> </comment> </thread> </pr_review_threads> The system prompt now explicitly names the wrapper, tells the agent that everything inside it is untrusted data from the PR (not instructions), and that prompt-injection payloads inside bodies must be disregarded. We keep the bodies so the agent can actually read engineer replies — that's the whole point of comment-awareness — but they're delimited as data, not concatenated as prose. Modern frontier models are well-trained to honor this contract. Additional defenses: - Author logins are validated against the GitHub username grammar; any unexpected value is rendered as "unknown" so the `author` attribute can't smuggle freeform text. - Literal closing tags (`</body>`, `</pr_review_threads>`, etc.) in bodies are neutered so a body can't break out of its wrapper. - Body length is capped at 4000 chars per comment to bound the prompt. --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-05-24 16:36:50 -07:00
threads = await fetch_pr_review_threads(owner="o", repo="r", pr_number=1, token="t")
assert threads == []
@pytest.mark.asyncio
async def test_reply_to_review_comment_posts_reply_payload() -> None:
response = MagicMock()
response.status_code = 201
response.json.return_value = {"id": 456, "body": "Thanks for the context."}
response.raise_for_status.return_value = None
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.post = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
result = await reply_to_review_comment(
owner="o",
repo="r",
pr_number=7,
review_comment_id=123,
body="Thanks for the context.",
token="t",
)
assert result == {"id": 456, "body": "Thanks for the context."}
args = client_cm.post.await_args
assert args.args[0] == "https://api.github.com/repos/o/r/pulls/7/comments/123/replies"
assert args.kwargs["json"] == {"body": "Thanks for the context."}
fix: publish_review HTTP 422 "Path/Line could not be resolved" — agent retries with identical args instead of dropping unresolvable findings (#1338) * publish_review: drop unresolvable findings and retry once on GitHub 422 GitHub returns 422 with 'Path could not be resolved' or 'Line could not be resolved' when an inline comment anchors to a file/line not in the PR diff. Previously the agent retried publish_review with byte-identical args multiple times before draining to skipped_empty_re_review=true, silently losing findings. - reviewer_publish.post_pull_request_review: parse 422 body and tag with _error_kind='unresolved_anchor' plus _raw_errors so callers can act. - tools/publish_review._publish_review_async: when that signal fires, cross-check each finding's range against the run config's diff_line_set, drop the bad ones, and re-POST once with only the valid findings. Return unresolvable_findings + hint so the agent calls update_finding instead of retrying the same payload. - reviewer.py: one-line prompt addendum telling the agent that unresolvable_findings means update_finding, not retry. - tests: cover 422 tagging (path + line), the drop-and-retry success path, the retry-still-fails path, and the don't-blind-retry path when no diff_line_set is available. * publish_review: fetch PR diff on demand for 422 retry filter Reviewer runs clear configurable['diff_line_set'] before the agent starts, so the unresolved-anchor retry path had no diff data to filter against — in the reachable production case it dropped nothing and returned success=False with empty unresolvable_findings, losing the otherwise-valid comments. Fall back to fetching the PR's unified diff via the GitHub REST API and recomputing the line set on the fly when no cached set is available. The cached set is still preferred when present. --------- Co-authored-by: issues-agent <issues-agent@langchain.dev> Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
2026-05-26 18:37:56 -07:00
@pytest.mark.asyncio
async def test_post_pull_request_review_tags_unresolved_anchor_on_422() -> None:
"""A GitHub 422 with 'Path could not be resolved' must be tagged as
``unresolved_anchor`` and carry the raw errors so the tool layer can act
on it (drop offending findings + retry) instead of bubbling an opaque
error string that the agent will only retry with identical args."""
import httpx
response = MagicMock()
response.status_code = 422
response.text = '{"errors":["Path could not be resolved"]}'
response.json.return_value = {"errors": ["Path could not be resolved"]}
response.raise_for_status.side_effect = httpx.HTTPStatusError(
"Unprocessable Entity",
request=MagicMock(),
response=response,
)
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.post = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
fix: publish_review HTTP 422 "Path/Line could not be resolved" — agent retries with identical args instead of dropping unresolvable findings (#1338) * publish_review: drop unresolvable findings and retry once on GitHub 422 GitHub returns 422 with 'Path could not be resolved' or 'Line could not be resolved' when an inline comment anchors to a file/line not in the PR diff. Previously the agent retried publish_review with byte-identical args multiple times before draining to skipped_empty_re_review=true, silently losing findings. - reviewer_publish.post_pull_request_review: parse 422 body and tag with _error_kind='unresolved_anchor' plus _raw_errors so callers can act. - tools/publish_review._publish_review_async: when that signal fires, cross-check each finding's range against the run config's diff_line_set, drop the bad ones, and re-POST once with only the valid findings. Return unresolvable_findings + hint so the agent calls update_finding instead of retrying the same payload. - reviewer.py: one-line prompt addendum telling the agent that unresolvable_findings means update_finding, not retry. - tests: cover 422 tagging (path + line), the drop-and-retry success path, the retry-still-fails path, and the don't-blind-retry path when no diff_line_set is available. * publish_review: fetch PR diff on demand for 422 retry filter Reviewer runs clear configurable['diff_line_set'] before the agent starts, so the unresolved-anchor retry path had no diff data to filter against — in the reachable production case it dropped nothing and returned success=False with empty unresolvable_findings, losing the otherwise-valid comments. Fall back to fetching the PR's unified diff via the GitHub REST API and recomputing the line set on the fly when no cached set is available. The cached set is still preferred when present. --------- Co-authored-by: issues-agent <issues-agent@langchain.dev> Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
2026-05-26 18:37:56 -07:00
result = await post_pull_request_review(
owner="o",
repo="r",
pr_number=1,
head_sha="sha",
body="b",
inline_comments=[{"path": "missing.py", "line": 1, "side": "RIGHT", "body": "x"}],
token="t",
)
assert isinstance(result, dict)
assert result.get("_error_kind") == "unresolved_anchor"
assert result.get("_status") == 422
assert result.get("_raw_errors") == ["Path could not be resolved"]
assert "HTTP 422" in result.get("_error", "")
@pytest.mark.asyncio
async def test_post_pull_request_review_tags_unresolved_anchor_on_line_error() -> None:
"""A 'Line could not be resolved' 422 must also be tagged as
``unresolved_anchor`` so a line that's not in the diff is treated the same
way as a path that's not in the diff."""
import httpx
response = MagicMock()
response.status_code = 422
response.text = '{"errors":["Line could not be resolved"]}'
response.json.return_value = {"errors": ["Line could not be resolved"]}
response.raise_for_status.side_effect = httpx.HTTPStatusError(
"Unprocessable Entity",
request=MagicMock(),
response=response,
)
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.post = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
fix: publish_review HTTP 422 "Path/Line could not be resolved" — agent retries with identical args instead of dropping unresolvable findings (#1338) * publish_review: drop unresolvable findings and retry once on GitHub 422 GitHub returns 422 with 'Path could not be resolved' or 'Line could not be resolved' when an inline comment anchors to a file/line not in the PR diff. Previously the agent retried publish_review with byte-identical args multiple times before draining to skipped_empty_re_review=true, silently losing findings. - reviewer_publish.post_pull_request_review: parse 422 body and tag with _error_kind='unresolved_anchor' plus _raw_errors so callers can act. - tools/publish_review._publish_review_async: when that signal fires, cross-check each finding's range against the run config's diff_line_set, drop the bad ones, and re-POST once with only the valid findings. Return unresolvable_findings + hint so the agent calls update_finding instead of retrying the same payload. - reviewer.py: one-line prompt addendum telling the agent that unresolvable_findings means update_finding, not retry. - tests: cover 422 tagging (path + line), the drop-and-retry success path, the retry-still-fails path, and the don't-blind-retry path when no diff_line_set is available. * publish_review: fetch PR diff on demand for 422 retry filter Reviewer runs clear configurable['diff_line_set'] before the agent starts, so the unresolved-anchor retry path had no diff data to filter against — in the reachable production case it dropped nothing and returned success=False with empty unresolvable_findings, losing the otherwise-valid comments. Fall back to fetching the PR's unified diff via the GitHub REST API and recomputing the line set on the fly when no cached set is available. The cached set is still preferred when present. --------- Co-authored-by: issues-agent <issues-agent@langchain.dev> Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
2026-05-26 18:37:56 -07:00
result = await post_pull_request_review(
owner="o",
repo="r",
pr_number=1,
head_sha="sha",
body="b",
inline_comments=[],
token="t",
)
assert isinstance(result, dict)
assert result.get("_error_kind") == "unresolved_anchor"
@pytest.mark.asyncio
async def test_post_pull_request_review_does_not_tag_unrelated_422() -> None:
"""A 422 whose errors don't match the anchor patterns must NOT be tagged
as ``unresolved_anchor`` — the retry path is only safe for known
per-comment anchor failures."""
import httpx
response = MagicMock()
response.status_code = 422
response.text = '{"errors":["something else"]}'
response.json.return_value = {"errors": ["something else"]}
response.raise_for_status.side_effect = httpx.HTTPStatusError(
"Unprocessable Entity",
request=MagicMock(),
response=response,
)
client_cm = AsyncMock()
client_cm.__aenter__.return_value = client_cm
client_cm.post = AsyncMock(return_value=response)
feat: shared GitHub HTTP helper with retries, rate-limit handling [closes OPE-45] (#1565) * feat: shared GitHub HTTP helper with retries, rate-limit handling, and sane timeouts Introduces agent/utils/github_http.py — a single place for GitHub API HTTP calls with 30s/10s-connect timeouts (vs httpx's 5s default), exponential backoff with jitter, Retry-After header support, and 429/secondary-rate-limit detection. Migrates the reviewer publish path (reviewer_publish.py, reviewer_diff.py, github_checks.py, github_ci.py) from one-shot httpx.AsyncClient() calls to the shared helper. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry transport errors on non-idempotent GitHub writes POST/DELETE/PATCH can create side effects server-side even when the client gets a timeout or connection reset. Only retry transport errors for idempotent methods (GET, HEAD, PUT, DELETE). 429/5xx status codes are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> * fix: don't retry 502/504 on non-idempotent GitHub writes 502 (bad gateway) and 504 (gateway timeout) are ambiguous — the upstream may have processed the write before the gateway returned an error. Only retry these for idempotent methods. 429 and 503 are still retried for all methods since the server explicitly did not process the request. Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> --------- Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com>
2026-06-17 14:45:57 -07:00
with patch("agent.utils.github_http.httpx.AsyncClient", return_value=client_cm):
fix: publish_review HTTP 422 "Path/Line could not be resolved" — agent retries with identical args instead of dropping unresolvable findings (#1338) * publish_review: drop unresolvable findings and retry once on GitHub 422 GitHub returns 422 with 'Path could not be resolved' or 'Line could not be resolved' when an inline comment anchors to a file/line not in the PR diff. Previously the agent retried publish_review with byte-identical args multiple times before draining to skipped_empty_re_review=true, silently losing findings. - reviewer_publish.post_pull_request_review: parse 422 body and tag with _error_kind='unresolved_anchor' plus _raw_errors so callers can act. - tools/publish_review._publish_review_async: when that signal fires, cross-check each finding's range against the run config's diff_line_set, drop the bad ones, and re-POST once with only the valid findings. Return unresolvable_findings + hint so the agent calls update_finding instead of retrying the same payload. - reviewer.py: one-line prompt addendum telling the agent that unresolvable_findings means update_finding, not retry. - tests: cover 422 tagging (path + line), the drop-and-retry success path, the retry-still-fails path, and the don't-blind-retry path when no diff_line_set is available. * publish_review: fetch PR diff on demand for 422 retry filter Reviewer runs clear configurable['diff_line_set'] before the agent starts, so the unresolved-anchor retry path had no diff data to filter against — in the reachable production case it dropped nothing and returned success=False with empty unresolvable_findings, losing the otherwise-valid comments. Fall back to fetching the PR's unified diff via the GitHub REST API and recomputing the line set on the fly when no cached set is available. The cached set is still preferred when present. --------- Co-authored-by: issues-agent <issues-agent@langchain.dev> Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
2026-05-26 18:37:56 -07:00
result = await post_pull_request_review(
owner="o",
repo="r",
pr_number=1,
head_sha="sha",
body="b",
inline_comments=[],
token="t",
)
assert isinstance(result, dict)
assert result.get("_error_kind") is None
assert result.get("_raw_errors") == ["something else"]
@pytest.mark.asyncio
async def test_publish_review_drops_unresolvable_findings_and_retries_once() -> None:
"""When GitHub rejects the batch with an ``unresolved_anchor`` 422, the
tool must filter the bad findings against the PR diff_line_set, re-POST
with only the valid ones, return ``success=True``, and report the dropped
finding ids via ``unresolvable_findings`` plus a corrective hint."""
from agent.tools.publish_review import _publish_review_async
findings = [
_f(id="f_good", severity="high", file="in_diff.py", start_line=10, end_line=10),
_f(id="f_bad", severity="high", file="not_in_diff.py", start_line=99, end_line=99),
]
# The PR diff only covers in_diff.py:10. f_bad anchors to a file/line not
# in the diff, so it must be dropped on retry.
fix: fetch PR diff via GitHub API to re-enable add_finding validation (#1339) * reviewer: fetch PR diff via GitHub API to re-enable add_finding validation The previous hotfix in reviewer.py set diff_line_set=None because the sandbox-based diff prep was sometimes producing empty diffs. That made every bad anchor a publish-time 422 instead of a creation-time rejection — the agent burned tokens producing unanchorable findings, and we had to add a publish-time retry safety net (#1338) to clean up. Fetch the PR's unified diff via the GitHub REST API at reviewer startup and populate diff_text + diff_line_set so add_finding can reject bad anchors immediately. The API path is reliable and is the same diff GitHub validates against when posting inline review comments. If the fetch fails, fall back to the previous behavior (validation disabled, publish-time retry handles it). Also extract the PR-diff fetch into reviewer_diff.fetch_pr_diff so both reviewer.py and publish_review.py share one implementation instead of two copies. * reviewer: make diff_line_set validation side-aware compute_diff_line_set previously returned only new-side line numbers, so re-enabling add_finding's validation would wrongly reject findings with side=LEFT (deleted-line bugs whose only anchor is an old-side line). Return {file: {"RIGHT": {new_lines}, "LEFT": {old_lines}}} instead, and have is_range_in_diff select the matching side from the finding's recorded side. add_finding and publish_review's retry filter both pass the finding's side through.
2026-05-27 10:03:33 -07:00
diff_line_set = {"in_diff.py": {"RIGHT": {10}, "LEFT": set()}}
fix: publish_review HTTP 422 "Path/Line could not be resolved" — agent retries with identical args instead of dropping unresolvable findings (#1338) * publish_review: drop unresolvable findings and retry once on GitHub 422 GitHub returns 422 with 'Path could not be resolved' or 'Line could not be resolved' when an inline comment anchors to a file/line not in the PR diff. Previously the agent retried publish_review with byte-identical args multiple times before draining to skipped_empty_re_review=true, silently losing findings. - reviewer_publish.post_pull_request_review: parse 422 body and tag with _error_kind='unresolved_anchor' plus _raw_errors so callers can act. - tools/publish_review._publish_review_async: when that signal fires, cross-check each finding's range against the run config's diff_line_set, drop the bad ones, and re-POST once with only the valid findings. Return unresolvable_findings + hint so the agent calls update_finding instead of retrying the same payload. - reviewer.py: one-line prompt addendum telling the agent that unresolvable_findings means update_finding, not retry. - tests: cover 422 tagging (path + line), the drop-and-retry success path, the retry-still-fails path, and the don't-blind-retry path when no diff_line_set is available. * publish_review: fetch PR diff on demand for 422 retry filter Reviewer runs clear configurable['diff_line_set'] before the agent starts, so the unresolved-anchor retry path had no diff data to filter against — in the reachable production case it dropped nothing and returned success=False with empty unresolvable_findings, losing the otherwise-valid comments. Fall back to fetching the PR's unified diff via the GitHub REST API and recomputing the line set on the fly when no cached set is available. The cached set is still preferred when present. --------- Co-authored-by: issues-agent <issues-agent@langchain.dev> Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
2026-05-26 18:37:56 -07:00
first_response = {
"_error": "HTTP 422: ...",
"_error_kind": "unresolved_anchor",
"_raw_errors": ["Path could not be resolved"],
"_status": 422,
}
retry_response = {"id": 7777}
post_review = AsyncMock(side_effect=[first_response, retry_response])
fetch_comments = AsyncMock(return_value=[])
set_metadata = AsyncMock()
with (
patch(
"agent.tools.publish_review.get_config",
return_value={
"configurable": {
"thread_id": "tid",
"diff_line_set": diff_line_set,
},
},
),
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
patch(
"agent.tools.publish_review.list_findings_async",
AsyncMock(return_value=findings),
),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch("agent.tools.publish_review.fetch_review_comments", fetch_comments),
patch(
"agent.tools.publish_review._resolve_threads_for_resolved_findings",
new_callable=AsyncMock,
return_value=0,
),
patch(
"agent.tools.publish_review._store_thread_ids_on_findings",
new_callable=AsyncMock,
),
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="sha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=False,
)
assert post_review.await_count == 2
# Retry must contain only the in-diff finding.
retry_inline = post_review.await_args_list[1].kwargs["inline_comments"]
assert {c["path"] for c in retry_inline} == {"in_diff.py"}
assert result["success"] is True
assert result["review_id"] == 7777
assert result["surfaced_count"] == 1
assert result["unresolvable_findings"] == ["f_bad"]
assert "update_finding" in result["hint"]
@pytest.mark.asyncio
async def test_publish_review_reports_unresolvable_when_retry_still_fails() -> None:
"""If even the filtered retry fails, the tool surfaces
``success=False`` plus the offending finding ids and a hint — it must
NOT collapse into the opaque retry-with-same-args loop."""
from agent.tools.publish_review import _publish_review_async
findings = [
_f(id="f_good", severity="high", file="in_diff.py", start_line=10, end_line=10),
_f(id="f_bad", severity="high", file="not_in_diff.py", start_line=99, end_line=99),
]
fix: fetch PR diff via GitHub API to re-enable add_finding validation (#1339) * reviewer: fetch PR diff via GitHub API to re-enable add_finding validation The previous hotfix in reviewer.py set diff_line_set=None because the sandbox-based diff prep was sometimes producing empty diffs. That made every bad anchor a publish-time 422 instead of a creation-time rejection — the agent burned tokens producing unanchorable findings, and we had to add a publish-time retry safety net (#1338) to clean up. Fetch the PR's unified diff via the GitHub REST API at reviewer startup and populate diff_text + diff_line_set so add_finding can reject bad anchors immediately. The API path is reliable and is the same diff GitHub validates against when posting inline review comments. If the fetch fails, fall back to the previous behavior (validation disabled, publish-time retry handles it). Also extract the PR-diff fetch into reviewer_diff.fetch_pr_diff so both reviewer.py and publish_review.py share one implementation instead of two copies. * reviewer: make diff_line_set validation side-aware compute_diff_line_set previously returned only new-side line numbers, so re-enabling add_finding's validation would wrongly reject findings with side=LEFT (deleted-line bugs whose only anchor is an old-side line). Return {file: {"RIGHT": {new_lines}, "LEFT": {old_lines}}} instead, and have is_range_in_diff select the matching side from the finding's recorded side. add_finding and publish_review's retry filter both pass the finding's side through.
2026-05-27 10:03:33 -07:00
diff_line_set = {"in_diff.py": {"RIGHT": {10}, "LEFT": set()}}
fix: publish_review HTTP 422 "Path/Line could not be resolved" — agent retries with identical args instead of dropping unresolvable findings (#1338) * publish_review: drop unresolvable findings and retry once on GitHub 422 GitHub returns 422 with 'Path could not be resolved' or 'Line could not be resolved' when an inline comment anchors to a file/line not in the PR diff. Previously the agent retried publish_review with byte-identical args multiple times before draining to skipped_empty_re_review=true, silently losing findings. - reviewer_publish.post_pull_request_review: parse 422 body and tag with _error_kind='unresolved_anchor' plus _raw_errors so callers can act. - tools/publish_review._publish_review_async: when that signal fires, cross-check each finding's range against the run config's diff_line_set, drop the bad ones, and re-POST once with only the valid findings. Return unresolvable_findings + hint so the agent calls update_finding instead of retrying the same payload. - reviewer.py: one-line prompt addendum telling the agent that unresolvable_findings means update_finding, not retry. - tests: cover 422 tagging (path + line), the drop-and-retry success path, the retry-still-fails path, and the don't-blind-retry path when no diff_line_set is available. * publish_review: fetch PR diff on demand for 422 retry filter Reviewer runs clear configurable['diff_line_set'] before the agent starts, so the unresolved-anchor retry path had no diff data to filter against — in the reachable production case it dropped nothing and returned success=False with empty unresolvable_findings, losing the otherwise-valid comments. Fall back to fetching the PR's unified diff via the GitHub REST API and recomputing the line set on the fly when no cached set is available. The cached set is still preferred when present. --------- Co-authored-by: issues-agent <issues-agent@langchain.dev> Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
2026-05-26 18:37:56 -07:00
first_response = {
"_error": "HTTP 422: ...",
"_error_kind": "unresolved_anchor",
"_raw_errors": ["Path could not be resolved"],
"_status": 422,
}
retry_response = {"_error": "HTTP 500: boom"}
post_review = AsyncMock(side_effect=[first_response, retry_response])
with (
patch(
"agent.tools.publish_review.get_config",
return_value={
"configurable": {
"thread_id": "tid",
"diff_line_set": diff_line_set,
},
},
),
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
patch(
"agent.tools.publish_review.list_findings_async",
AsyncMock(return_value=findings),
),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
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", new_callable=AsyncMock),
):
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,
)
assert result["success"] is False
assert result["unresolvable_findings"] == ["f_bad"]
assert "update_finding" in result["hint"]
@pytest.mark.asyncio
async def test_publish_review_does_not_retry_when_no_findings_can_be_dropped() -> None:
"""When the unresolved_anchor 422 fires but the diff_line_set rules out
no findings (e.g., diff data unavailable), the tool must NOT retry — it
must surface the structured error so the agent stops looping."""
from agent.tools.publish_review import _publish_review_async
findings = [
_f(id="f_only", severity="high", file="in_diff.py", start_line=10, end_line=10),
]
# No cached diff_line_set, and the on-demand fetch fails — no way to tell
# which finding is bad.
first_response = {
"_error": "HTTP 422: ...",
"_error_kind": "unresolved_anchor",
"_raw_errors": ["Path could not be resolved"],
"_status": 422,
}
post_review = AsyncMock(return_value=first_response)
with (
patch(
"agent.tools.publish_review.get_config",
return_value={"configurable": {"thread_id": "tid"}},
),
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
patch(
"agent.tools.publish_review.list_findings_async",
AsyncMock(return_value=findings),
),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
patch(
"agent.tools.publish_review._resolve_diff_line_set",
new_callable=AsyncMock,
return_value=None,
),
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", new_callable=AsyncMock),
):
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,
)
# Only one attempt — never retry blindly.
assert post_review.await_count == 1
assert result["success"] is False
assert result["unresolvable_findings"] == []
assert "update_finding" in result["hint"]
@pytest.mark.asyncio
async def test_publish_review_fetches_pr_diff_when_diff_line_set_missing() -> None:
"""Reviewer runs clear ``diff_line_set`` from config before the agent
starts, so the publish-time retry path must fall back to fetching the
PR's unified diff on demand and recomputing the line set — otherwise no
finding is ever droppable and the retry surfaces empty
``unresolvable_findings`` for the reachable production case."""
from agent.tools.publish_review import _publish_review_async
findings = [
_f(id="f_good", severity="high", file="in_diff.py", start_line=10, end_line=10),
_f(id="f_bad", severity="high", file="not_in_diff.py", start_line=99, end_line=99),
]
first_response = {
"_error": "HTTP 422: ...",
"_error_kind": "unresolved_anchor",
"_raw_errors": ["Path could not be resolved"],
"_status": 422,
}
retry_response = {"id": 9999}
post_review = AsyncMock(side_effect=[first_response, retry_response])
pr_diff = (
"diff --git a/in_diff.py b/in_diff.py\n"
"--- a/in_diff.py\n"
"+++ b/in_diff.py\n"
"@@ -1,1 +10,1 @@\n"
"+touched\n"
)
with (
patch(
"agent.tools.publish_review.get_config",
return_value={"configurable": {"thread_id": "tid"}},
),
patch("agent.tools.publish_review.get_thread_id_from_runtime", return_value="tid"),
patch(
"agent.tools.publish_review.list_findings_async",
AsyncMock(return_value=findings),
),
patch("agent.tools.publish_review.post_pull_request_review", post_review),
fix: fetch PR diff via GitHub API to re-enable add_finding validation (#1339) * reviewer: fetch PR diff via GitHub API to re-enable add_finding validation The previous hotfix in reviewer.py set diff_line_set=None because the sandbox-based diff prep was sometimes producing empty diffs. That made every bad anchor a publish-time 422 instead of a creation-time rejection — the agent burned tokens producing unanchorable findings, and we had to add a publish-time retry safety net (#1338) to clean up. Fetch the PR's unified diff via the GitHub REST API at reviewer startup and populate diff_text + diff_line_set so add_finding can reject bad anchors immediately. The API path is reliable and is the same diff GitHub validates against when posting inline review comments. If the fetch fails, fall back to the previous behavior (validation disabled, publish-time retry handles it). Also extract the PR-diff fetch into reviewer_diff.fetch_pr_diff so both reviewer.py and publish_review.py share one implementation instead of two copies. * reviewer: make diff_line_set validation side-aware compute_diff_line_set previously returned only new-side line numbers, so re-enabling add_finding's validation would wrongly reject findings with side=LEFT (deleted-line bugs whose only anchor is an old-side line). Return {file: {"RIGHT": {new_lines}, "LEFT": {old_lines}}} instead, and have is_range_in_diff select the matching side from the finding's recorded side. add_finding and publish_review's retry filter both pass the finding's side through.
2026-05-27 10:03:33 -07:00
patch(
"agent.tools.publish_review.fetch_pr_diff",
AsyncMock(return_value=pr_diff),
),
fix: publish_review HTTP 422 "Path/Line could not be resolved" — agent retries with identical args instead of dropping unresolvable findings (#1338) * publish_review: drop unresolvable findings and retry once on GitHub 422 GitHub returns 422 with 'Path could not be resolved' or 'Line could not be resolved' when an inline comment anchors to a file/line not in the PR diff. Previously the agent retried publish_review with byte-identical args multiple times before draining to skipped_empty_re_review=true, silently losing findings. - reviewer_publish.post_pull_request_review: parse 422 body and tag with _error_kind='unresolved_anchor' plus _raw_errors so callers can act. - tools/publish_review._publish_review_async: when that signal fires, cross-check each finding's range against the run config's diff_line_set, drop the bad ones, and re-POST once with only the valid findings. Return unresolvable_findings + hint so the agent calls update_finding instead of retrying the same payload. - reviewer.py: one-line prompt addendum telling the agent that unresolvable_findings means update_finding, not retry. - tests: cover 422 tagging (path + line), the drop-and-retry success path, the retry-still-fails path, and the don't-blind-retry path when no diff_line_set is available. * publish_review: fetch PR diff on demand for 422 retry filter Reviewer runs clear configurable['diff_line_set'] before the agent starts, so the unresolved-anchor retry path had no diff data to filter against — in the reachable production case it dropped nothing and returned success=False with empty unresolvable_findings, losing the otherwise-valid comments. Fall back to fetching the PR's unified diff via the GitHub REST API and recomputing the line set on the fly when no cached set is available. The cached set is still preferred when present. --------- Co-authored-by: issues-agent <issues-agent@langchain.dev> Co-authored-by: Johannes du Plessis <johannes@langchain.dev>
2026-05-26 18:37:56 -07:00
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._store_thread_ids_on_findings",
new_callable=AsyncMock,
),
patch("agent.tools.publish_review.set_reviewer_thread_metadata", new_callable=AsyncMock),
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="sha",
token="t",
severity_threshold="medium",
cap=15,
is_re_review=False,
)
assert post_review.await_count == 2
retry_inline = post_review.await_args_list[1].kwargs["inline_comments"]
assert {c["path"] for c in retry_inline} == {"in_diff.py"}
assert result["success"] is True
assert result["unresolvable_findings"] == ["f_bad"]
def test_publish_review_tool_returns_structured_error_when_thread_missing() -> None:
"""A missing reviewer thread surfaces as a do-not-retry tool result instead
of an exception the middleware swallows into an empty tool message."""
from agent.reviewer_findings import ReviewerThreadMissingError
from agent.tools.publish_review import publish_review
publish_async = AsyncMock(
side_effect=ReviewerThreadMissingError("tid", RuntimeError("thread tid not found"))
)
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 = publish_review()
assert result["success"] is False
assert result["error"] == "thread_not_found"
assert result["thread_id"] == "tid"
assert "Do not retry" in result["note"]