open-swe/agent/tools/add_finding.py
Johannes du Plessis 378b95266e
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

124 lines
4.7 KiB
Python

"""Tool: ``add_finding``. Records one review finding on the reviewer thread."""
from __future__ import annotations
import asyncio
from typing import Any
from langgraph.config import get_config
from ..reviewer_diff import is_range_in_diff
from ..reviewer_findings import (
DiffSide,
Finding,
Severity,
append_finding,
get_thread_id_from_runtime,
new_finding,
)
def add_finding(
severity: str,
category: str,
file: str,
description: str,
start_line: int | None = None,
end_line: int | None = None,
suggestion: str | None = None,
side: str = "RIGHT",
) -> dict[str, Any]:
"""Record a review finding on the reviewer thread.
Findings persist on the reviewer thread's metadata so they survive sandbox
eviction and are queryable across runs by the watch-mode reconciliation
flow and the future UI.
**When to use:** Once per distinct issue you find while reviewing the
diff. Prefer one finding per issue, with a clear ``description`` and, when
you can offer a concrete fix, a ``suggestion`` that exactly replaces lines
``start_line..end_line``.
**In-diff only:** ``start_line..end_line`` must be inside the PR diff.
File-level findings (both ``start_line`` and ``end_line`` None) are
accepted but won't render as inline GitHub comments — only use when the
issue truly isn't anchored to a line.
Args:
severity: One of ``informational``, ``low``, ``medium``, ``high``, ``critical``.
category: Short category label (``correctness``, ``security``, ``perf``,
``style``, ``flag``, etc.). Free-form; used for grouping in the UI.
file: Repo-relative path of the file the finding refers to.
description: Markdown body the user sees.
start_line: 1-based start line in the new (post-PR) file. Equal to
``end_line`` for single-line findings; less than ``end_line`` for
ranges. Omit (with ``end_line``) for file-level findings.
end_line: 1-based end line, inclusive. Defaults to ``start_line``.
suggestion: Replacement text for ``start_line..end_line``. When set,
the published GitHub comment includes a ```suggestion``` block so
the user can click "Commit suggestion".
side: ``RIGHT`` (post-PR file, default) or ``LEFT`` (base file). Almost
always ``RIGHT``.
Returns:
Dictionary with ``success``, ``finding_id`` and (on rejection) ``error``.
"""
if start_line is not None and end_line is None:
end_line = start_line
if start_line is None and end_line is not None:
start_line = end_line
if severity not in {"informational", "low", "medium", "high", "critical"}:
return {"success": False, "error": f"Invalid severity: {severity}"}
if side not in {"LEFT", "RIGHT"}:
return {"success": False, "error": f"Invalid side: {side}"}
if start_line is not None and end_line is not None and end_line < start_line:
return {"success": False, "error": "end_line must be >= start_line"}
config = get_config()
configurable = config.get("configurable", {}) if isinstance(config, dict) else {}
diff_line_set = configurable.get("diff_line_set") if isinstance(configurable, dict) else None
head_sha = configurable.get("head_sha", "") if isinstance(configurable, dict) else ""
diff_text = configurable.get("diff_text", "") if isinstance(configurable, dict) else ""
if isinstance(diff_line_set, dict) and not is_range_in_diff(
diff_line_set, file, start_line, end_line
):
return {
"success": False,
"error": (
f"Finding range {file}:{start_line}-{end_line} is not part of the PR diff. "
"Only review changes the PR introduces; do not flag pre-existing code."
),
}
diff_hunk: str | None = None
if isinstance(diff_text, str) and diff_text:
from ..reviewer_diff import extract_diff_hunk
diff_hunk = extract_diff_hunk(diff_text, file, start_line, end_line)
finding: Finding = new_finding(
severity=_cast_severity(severity),
category=category,
file=file,
start_line=start_line,
end_line=end_line,
description=description,
sha=str(head_sha) if isinstance(head_sha, str) else "",
side=_cast_side(side),
suggestion=suggestion,
diff_hunk=diff_hunk,
)
thread_id = get_thread_id_from_runtime()
asyncio.run(append_finding(thread_id, finding))
return {"success": True, "finding_id": finding["id"]}
def _cast_severity(value: str) -> Severity:
return value # type: ignore[return-value]
def _cast_side(value: str) -> DiffSide:
return value # type: ignore[return-value]