From 378b95266ec479f082532dd2179ea85335ff7021 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Thu, 7 May 2026 14:48:43 -0700 Subject: [PATCH] feat: implement reviewer findings, publish_review, and watch mode (#1253) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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. --- REVIEWER_DESIGN.md | 217 ++++++++++++++++++++ REVIEWER_EVAL_PLAN.md | 91 --------- agent/reviewer.py | 314 +++++++++++++++++++++++++---- agent/reviewer_diff.py | 232 +++++++++++++++++++++ agent/reviewer_findings.py | 275 +++++++++++++++++++++++++ agent/reviewer_publish.py | 292 +++++++++++++++++++++++++++ agent/tools/__init__.py | 8 + agent/tools/add_finding.py | 124 ++++++++++++ agent/tools/list_findings.py | 36 ++++ agent/tools/publish_review.py | 297 +++++++++++++++++++++++++++ agent/tools/update_finding.py | 80 ++++++++ agent/webapp.py | 308 ++++++++++++++++++++++++++-- evals/reviewer/target.py | 60 ++++-- tests/test_github_issue_webhook.py | 10 +- tests/test_reviewer_diff.py | 112 ++++++++++ tests/test_reviewer_findings.py | 175 ++++++++++++++++ tests/test_reviewer_publish.py | 168 +++++++++++++++ tests/test_reviewer_tools.py | 180 +++++++++++++++++ tests/test_reviewer_watch.py | 232 +++++++++++++++++++++ 19 files changed, 3043 insertions(+), 168 deletions(-) create mode 100644 REVIEWER_DESIGN.md delete mode 100644 REVIEWER_EVAL_PLAN.md create mode 100644 agent/reviewer_diff.py create mode 100644 agent/reviewer_findings.py create mode 100644 agent/reviewer_publish.py create mode 100644 agent/tools/add_finding.py create mode 100644 agent/tools/list_findings.py create mode 100644 agent/tools/publish_review.py create mode 100644 agent/tools/update_finding.py create mode 100644 tests/test_reviewer_diff.py create mode 100644 tests/test_reviewer_findings.py create mode 100644 tests/test_reviewer_publish.py create mode 100644 tests/test_reviewer_tools.py create mode 100644 tests/test_reviewer_watch.py diff --git a/REVIEWER_DESIGN.md b/REVIEWER_DESIGN.md new file mode 100644 index 00000000..79620fcc --- /dev/null +++ b/REVIEWER_DESIGN.md @@ -0,0 +1,217 @@ +# Reviewer Agent — Design + +Design for the Open SWE Reviewer Agent. Goal is to match Devin Review's *perceived* quality (won internal A/B vs Graphite) on PR review, with a single evolving findings list and a "watch" mode that re-reviews on new commits. Eval plan is tracked separately in `REVIEWER_EVAL_PLAN.md`. + +## Goals + +- A reviewer agent that produces high-signal findings on a PR, surfaced as **inline GitHub review comments** at the relevant file/line range, with **```suggestion blocks** (the "Commit suggestion" UX) where the agent can offer a concrete fix. +- Filter what's surfaced to GitHub by severity — a clean PR shouldn't have five filler comments appended to one critical one. +- A single evolving findings list per PR — resolved findings are kept (status=resolved), not pruned. +- A "watch" mode that re-reviews on new commits to a previously reviewed PR, reconciling existing findings (resolved / still-open / updated), adding new ones, and resolving GitHub review threads for findings that the new commits addressed. +- One canonical reviewer thread per PR, regardless of how the review was triggered. +- Headless first; eventually a UI for the full findings list. + +## Non-goals (for now) + +- Multi-pass / multi-agent review orchestration. One reviewer agent, one thread, one evolving list. +- Deterministic resolution-detection logic. Reconciliation is LLM reasoning over (previous findings, new diff, sandbox state). + +## Dispatch model + +**One canonical reviewer thread per PR.** Thread id is derived deterministically from the PR URL (or `owner/repo#number`), the same way the existing webhook handlers in `agent/utils/` already derive thread ids per source. Every trigger surface routes to the same thread, so findings, sandbox, and watch state stay continuous. + +Trigger surfaces: + +1. **Slack** — handled by the main agent. The user asks the bot to review a PR; the main agent calls a `request_pr_review(pr_url, message)` tool. That tool resolves the canonical reviewer thread id from the PR URL and triggers a run via `langgraph_sdk` (same pattern as the existing GitHub PR webhook path). This already exists in some form (see `agent/tools/request_pr_review.py`). +2. **GitHub webhook — review requested.** Direct invocation of the reviewer graph against the canonical thread. +3. **GitHub webhook — push to a watched PR.** Direct invocation against the same canonical thread; the watch flag in thread metadata gates whether we trigger. + +This unifies all paths: the reviewer graph never has to ask "who triggered me?" — it always sees the same thread and the same state, with a structured user message describing what to do. + +## Findings as first-class state + +Findings live in **LangGraph thread state**, not in sandbox files. Sandboxes are evictable; thread state survives. + +```python +class Finding(TypedDict): + id: str # stable, e.g. "f_" + severity: Literal["informational", "low", "medium", "high", "critical"] + category: str # e.g. "correctness", "security", "perf", "style", "flag" + file: str + start_line: int | None # None when the finding is file-level + end_line: int | None # equals start_line for single-line findings; >start_line for ranges + side: Literal["LEFT", "RIGHT"] # LEFT = base/old, RIGHT = head/new — almost always RIGHT + description: str # the body the user sees + suggestion: str | None # if set, rendered as a ```suggestion block — gives the user a "Commit suggestion" button on GitHub. Must replace exactly start_line..end_line. + status: Literal["open", "resolved", "dismissed"] + first_seen_sha: str # SHA at which this finding was first introduced + last_confirmed_sha: str # most recent SHA where this finding was still open + github_review_comment_id: int | None # populated after publish; used to resolve the thread on re-review when status moves to resolved + diff_hunk: str | None # snippet of the relevant diff cached at finding-creation time, so the future UI / Slack / Linear renderers can render the diff alongside the finding without re-fetching from GitHub or the sandbox (which is evictable) +``` + +**Storage: thread metadata.** Findings live in LangGraph thread metadata under the reviewer thread, queried via the langgraph SDK client. Same pattern as existing thread metadata (`sandbox_id`, `github_token_encrypted`). Avoids fighting deepagents' state abstraction. The future UI lists "PRs being reviewed" by querying threads filtered on `metadata.kind == "reviewer"`, and reads each thread's findings list directly. + +Reviewer-thread metadata schema: + +- `kind: "reviewer"` — sentinel so the UI / SDK queries can filter to reviewer threads +- `pr: {owner, name, number, url, title, head_ref, base_ref}` — PR identity, what the UI displays in the list +- `findings: list[Finding]` +- `last_reviewed_sha: str | None` +- `watch: bool` (whether push events should re-trigger) +- (existing) `sandbox_id`, `github_token_encrypted` + +Tools the reviewer agent uses to mutate findings: + +- `add_finding(severity, category, file, start_line, end_line, description, suggestion=None, side="RIGHT") -> id` + - Single-line finding: `start_line == end_line`. + - File-level finding: both `None` (publishes as a top-level review body line, not inline). + - `suggestion`, when set, must be the exact replacement text for lines `start_line..end_line` inclusive — that's how GitHub's ```suggestion block works. +- `update_finding(id, *, status?, severity?, description?, suggestion?, note?)` — single tool for any post-creation mutation, including marking resolved/dismissed and revising a suggestion after the agent looks more carefully. +- `list_findings(status_filter?) -> list[Finding]` + +Resolved findings are kept in the list (status=`resolved`), hidden from the default top-K GitHub surfacing, surfaced in the eventual UI as "what's already addressed". This prevents the agent from re-finding the same issue across runs. + +## Publishing to GitHub (decoupled from finding production) + +The agent's job is to produce findings. A separate step publishes them to GitHub as a Review with inline comments. + +**Surfacing format: GitHub Pull Request Review.** Single API call (`POST /repos/{owner}/{repo}/pulls/{n}/reviews`) creates one review with: +- a top-level **review body** — agent-authored summary / overall take on the PR +- an array of **inline comments**, one per surfaced finding, anchored to `path` + `line` (+ `start_line` for ranges) + `side` +- `event: "COMMENT"` (not `REQUEST_CHANGES` — we don't want the reviewer agent to gate merges) + +Findings with a `suggestion` get the suggestion appended to the comment body as a ```suggestion fenced block, which gives the user GitHub's native **"Commit suggestion"** / **"Add suggestion to batch"** UX. For multi-line ranges, a multi-line ```suggestion block replaces the entire range. + +Severity ladder (matching Devin Review): `informational` < `low` < `medium` < `high` < `critical`. `informational` is for purely contextual / FYI observations (e.g. "this codebase uses pattern X elsewhere") — not flaws. It still supports suggestions; the agent can use it for stylistic nudges that aren't actually wrong. + +Severity-threshold filter (not pure top-K): publish all findings with severity ≥ `medium` by default, with a hard cap of 4 to avoid review spam. Pure top-K means a clean PR with one critical issue gets four filler findings appended — bad UX. `informational` and `low` findings are produced into state and visible in the eventual UI / full list, but not surfaced to the GitHub PR by default. + +**Mechanism: a `publish_review` tool the agent calls deliberately** at the end of its run. Why a tool, not after-agent middleware: + +- Clearer in traces — explicit step in LangSmith. +- The agent can choose to skip publishing on a re-review run where nothing changed surface-worthy. +- Failure modes (line not in diff, suggestion conflict, GitHub 422) surface back to the agent, which can adjust and retry. +- We don't have to bake the severity-threshold policy into middleware; the agent can override (e.g. publish a `low`-severity nit on a tiny PR if it's the only finding). + +Decoupling finding production from publishing is what lets us swap the GitHub Review surfacing for a richer UI later without touching the agent loop. + +### Inline comment constraints (and how to handle them) + +GitHub's inline review comments require the line to be **part of the PR diff** — meaning `line` (and `start_line..line` for ranges) must fall within an actual hunk in the PR's diff against its base. Implications: + +- **Findings on lines the PR didn't touch** (e.g. agent notices a pre-existing bug while reviewing context) cannot be inline comments. Two options: + 1. Drop them from the review (agent should be discouraged from reporting these in the first place — system prompt says "review the diff, not the surrounding code"). + 2. Append them to the **review body** as a "Pre-existing observations" section. + Default to (1); allow (2) only when severity is `high`/`critical`. +- **Suggestions only apply to lines in the diff**, since GitHub applies them as a follow-up commit on the PR branch. Suggestions on out-of-diff lines are a hard error from the GitHub API. +- **The reviewer's prep node should compute the set of (file, line) tuples in the diff** and pass it to the agent as part of the review context. The `add_finding` tool can validate against that set and reject (or auto-flag) findings that fall outside it, rather than failing at publish time. + +### Re-review and resolved threads + +On a re-review run, when a finding moves from `open` → `resolved`, the reviewer should also **resolve the corresponding GitHub review comment thread** (via the GraphQL `resolveReviewThread` mutation, since REST doesn't expose this). That's why `Finding.github_review_comment_id` is stored — without it, we can't reconcile back to the thread on GitHub. + +For findings that move from `open` → `open-but-updated` (e.g. the agent revises severity or suggestion based on new context), the simplest behavior is to leave the existing GitHub thread alone and let it stand; richer behavior (post a follow-up reply in that thread) is a follow-up. + +## Cold-start / warm-path entry contract + +Sandbox handling was reworked recently (#1249): `ensure_sandbox_for_thread` now does cache → ping → start-if-idle → refresh proxy → recreate only on hard failure. So warm-path is realistic between an initial review and a follow-up push, especially for tight feedback loops where the gap is short. + +**Deterministic prep node before the agent's first model call:** + +``` +if /workspace/ exists: + git fetch && git checkout +else: + gh repo clone / /workspace/ && git checkout +``` + +Either branch produces the same entry contract for the agent: + +> "You are in `/workspace/` checked out to ``. Existing findings: [...]. Diff since ``: [...]. Your job is to: ..." + +Why a deterministic prep node and not a prompted agent step: + +- No tokens burned on "okay, cloning now..." narration. +- Auth / network / missing-branch failures surface as discrete graph errors instead of the agent thrashing through tool retries. +- Discrete step in the LangSmith trace, not buried inside agent tool calls. +- Matches the existing pattern where sandbox creation + GitHub proxy config already live outside the agent in `get_agent`. + +The agent can still pull additional context (full PR diff, base branch, related files) via the standard tools when it decides it needs to. + +## Re-review prompt context + +The structured user message that triggers a re-review run on a watched PR's new commit: + +``` +A new commit has been pushed to the PR you previously reviewed. + +PR: /# +Previous reviewed SHA: +New HEAD SHA: + +Existing findings: +- [] (, ) : — [status: ] +- ... + +Diff since previous reviewed SHA: + + +For each existing open finding, decide whether the new commits: + - resolved it (mark resolved with update_finding(status="resolved")) + - left it unchanged (no action) + - changed it materially (update via update_finding with a note, or update + revised suggestion, or close + add_finding) + +Then review the new diff for any net-new issues and add them with add_finding. +Finally call publish_review to post inline comments + suggestions for the new findings, and resolve the GitHub threads for findings that just moved to resolved. +``` + +Diff scope is the diff *since `last_reviewed_sha`*, not the full base...head PR diff. That's what resolution detection actually needs. The agent can fetch the full diff on demand if it wants broader context. + +First-review just has empty findings list and a full base...head diff. + +## Watch mode + +- `watch: bool` flag on the reviewer thread, set to `True` implicitly on first successful review. +- New webhook handler for GitHub `push` events: if the push is to a PR's head ref and that PR's reviewer thread has `watch=True`, emit the structured re-review user message into the thread (via `langgraph_sdk`, same dispatch path as other webhook triggers). +- An explicit `unwatch_pr` tool / API endpoint stops watching (e.g. once the PR is merged or the user is done). Closing/merging a PR can auto-unwatch via the existing `pull_request` webhook. + +Edge cases worth thinking about (not blockers): + +- **Empty diff since last SHA** (rebase, force-push that resolves to same tree) — skip the run at the webhook level, don't trigger the agent. +- **Force-push that drops `last_reviewed_sha` from history** — `git fetch` will lose the old SHA's reachability. The diff scope falls back to base...head; treat as a fresh review of the new state, but keep existing findings as starting context for reconciliation. +- **Long gap between review and push** — sandbox evicted, falls into cold-start path automatically via the existing recreation logic. No special handling needed. + +## Graph shape (sketch) + +``` +reviewer_graph: + prep_sandbox # ensure_sandbox_for_thread (existing) + prep_repo # NEW: clone-or-fetch + checkout target_sha + build_review_context # NEW: assemble findings + diff + diff-line-set into user message (or branch on first-review vs re-review) + agent # create_deep_agent with reviewer-specific prompt + finding tools + # (publish_review is a tool the agent calls; not a node) +``` + +`prep_sandbox`, `prep_repo`, and `build_review_context` are all deterministic graph nodes, not agent tool calls. `build_review_context` reads thread state and the diff, computes the set of (file, line) tuples that are part of the PR diff (so `add_finding` can validate against it), and produces the user message that seeds the agent run. + +## Open questions + +1. **Severity threshold for default surfacing.** Default to `medium`+? `high`+? Worth tuning against the eval set. Independent of the cap (currently 4). `informational` is always below the threshold by design — it's a UI-only tier. +2. **Pre-existing-bug findings.** Drop entirely from the review (clean), or surface in the review body for `high`/`critical` only (more complete)? Currently leaning drop, with a system prompt instruction telling the agent not to report them in the first place. +3. **Findings dedup across runs.** When the agent calls `add_finding` on a re-review, do we trust it not to duplicate, or do we do server-side dedup based on (file, line, category, description-similarity)? Probably trust the agent for now (it sees existing findings in the prompt) and add server-side dedup only if we observe duplicates in eval. +4. **Updated-but-not-resolved findings on re-review.** When a finding stays open but the agent revises its suggestion (e.g. new commits made the original fix obsolete but the issue stands), do we leave the existing GitHub thread alone, post a follow-up reply, or close + repost? Leaning leave-alone for v1. +5. **Reviewer system prompt.** Needs to bake in: single evolving findings list, prefer updating existing findings over adding new ones, only surface in-diff findings, write actionable suggestions where possible, severity calibration matches Devin Review. Highest-leverage piece for matching Devin's perceived quality — iterate against the eval set. + +## Implementation order + +1. **Findings state + tools.** Add `Finding` schema (with `start_line`/`end_line`/`suggestion`/`side`), `add_finding` / `update_finding` / `list_findings` tools, thread-state extensions. No watch, no publish — just the agent producing a findings list visible in state. +2. **Prep nodes.** `prep_repo` (clone-or-fetch + checkout) and `build_review_context` (computes diff + diff-line-set, branches on first-review vs re-review) as deterministic graph nodes before the agent. +3. **`publish_review` tool.** Posts a GitHub PR Review with body + inline comments + ```suggestion blocks. Stores `github_review_comment_id` back on each Finding for later reconciliation. +4. **Reviewer system prompt iteration.** Calibrate against the eval set in `REVIEWER_EVAL_PLAN.md` — severity calibration, in-diff-only discipline, suggestion quality. +5. **Watch mode.** `watch` flag, push webhook handler, re-review user message format, `unwatch_pr` on PR close/merge, GraphQL `resolveReviewThread` on findings that move to resolved. +6. **(Future)** UI for full findings list, server-side dedup if needed, follow-up replies in existing threads when findings are revised. + +## Followups / notes + +- **Protected-attribute access in `_start_langsmith_sandbox_if_needed`** (`agent/server.py:76-77`) reaches into `sandbox._sandbox._client.get_sandbox_status`. If those internals shift upstream, the warm path silently degrades to always-recreate. Worth either pinning the assumption with a comment or a focused test against a mocked LangSmith client that exercises the start-if-idle branch specifically. Not urgent. diff --git a/REVIEWER_EVAL_PLAN.md b/REVIEWER_EVAL_PLAN.md deleted file mode 100644 index 709cec21..00000000 --- a/REVIEWER_EVAL_PLAN.md +++ /dev/null @@ -1,91 +0,0 @@ -# Goal - -Score Open SWE Reviewer on the 50 PRs from the martian offline benchmark, in conditions close to a real PR review, and compare against Devin Review's published numbers. Manual run, baseline only. - -## Dataset - -Import the 50 entries from `withmartian/code-review-benchmark` `offline/golden_comments/*.json` into a LangSmith dataset (`openswe-reviewer-v1`). - -For each PR, enrich the example up-front via `gh pr view --json` so the example carries everything needed to reproduce the PR's state: - -```json -{ - "inputs": { - "repo": "getsentry/sentry", - "fork_repo": "/sentry", - "pr_number": 12345, - "base_sha": "", - "head_sha": "", - "pr_title": "...", - "original_url": "" - }, - "outputs": { - "golden_comments": [ - {"comment": "...", "severity": "High"} - ] - } -} -``` - -`base_sha` must be the **PR's base commit at open time**, not today's main. `gh pr view --json baseRefOid,headRefOid` returns these — recover from upstream once and freeze them in the dataset. From this point the dataset is self-contained and reproducible regardless of upstream activity. - -## Repo setup - -Fork the 5 upstream repos (sentry, grafana, [cal.com](http://cal.com/), discourse, keycloak) into your personal org once. The agent clones from your forks rather than upstream — gives stable targets, isolates from upstream rate limits, no risk of upstream force-pushes invalidating SHAs. - -No need to fork the 50 PRs themselves — we're not relying on the GitHub-App-review flow. The PR's content is reconstructed from `base_sha` + the diff fetched from upstream once at dataset-build time (or fetched on demand via `git fetch origin pull//head`). - -## Target function - -```python -async def review_pr(inputs: dict) -> dict: - thread = await client.threads.create() - run = await client.runs.wait( - thread["thread_id"], - assistant_id="reviewer", - input={"pr_inputs": inputs}, - ) - return {"comments": extract_review_comments(run)} -``` - -Inside the reviewer graph, the first agent step (or a pre-agent middleware) runs in the sandbox: - -```bash -git clone --depth=200 .git /workspace -cd /workspace -git fetch origin pull//head:pr -git checkout -git merge --no-commit --no-ff pr # or: leave as two refs and let agent diff -``` - -This puts the sandbox in the same state a human reviewer would see when the PR was opened: main at the base SHA, plus the PR's changes applied/available. Agent then uses its normal tool set (`read_file`, `glob`, `grep`, `gh pr diff`) over that working tree. - -The reviewer must emit structured comments. Cleanest path: a `submit_review` tool whose args (`[{file, line, severity, body}, ...]`) become the run output. More reliable than parsing the final assistant message. - -## Evaluators - -Because `submit_review` already returns one structured entry per issue (`{file, line, severity, body}`), there's no prose to split and no summary-vs-inline duplication to collapse. We skip martian's `step2_extract_comments` and `step2_5_dedup_candidates` and feed the agent's list straight into the judge. Port the judge prompt verbatim from `step3_judge_comments.py` so scores stay comparable to Devin's published numbers. - -- **Per-example evaluator `judge_match`**: receives the agent's `comments` (from `run.outputs`) and the `golden_comments` (from `example.outputs`). For each `(candidate, golden)` pair, asks the judge LLM "do these describe the same underlying issue?". Tallies TP / FP / FN → returns `{precision, recall, f1, tp, fp, fn}` per example. -- **Summary evaluator `aggregate_pr`**: micro- and macro-averaged precision/recall across the 50 examples. - -Judge model: **`claude-opus-4-5`** — matches the model martian used to score Devin Review. - -## Run - -```python -client.evaluate( - review_pr, - data="openswe-reviewer-v1", - evaluators=[judge_match], - summary_evaluators=[aggregate_pr], - experiment_prefix="openswe-reviewer-baseline", - max_concurrency=5, -) -``` - -`max_concurrency=5` keeps sandbox provider load reasonable. Full run: ~50 examples × 5–15 min/PR ÷ 5 = ~1–2.5h wall. - -## Comparison - -Devin's published numbers come from the same 50 goldens + same judge model + same prompts. As long as we hold those three constant, the LangSmith experiment's aggregate precision/recall is directly comparable. Drop both into a side-by-side table; LangSmith's experiment-compare view also works if you import Devin's results as a separate experiment over the same dataset. \ No newline at end of file diff --git a/agent/reviewer.py b/agent/reviewer.py index 927d29d4..9afcc0d2 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -1,10 +1,17 @@ """Reviewer graph factory. -Mirrors `agent.server.get_agent`'s sandbox lifecycle but returns a deep agent -configured for code review only: narrowed tool set, reviewer-specific system -prompt, no commit/push/PR-opening. +Mirrors `agent.server.get_agent`'s sandbox lifecycle but configures a deep +agent for code review only: -Inline review comments are submitted by the agent through the GitHub CLI. +- Deterministic repo prep (clone-or-fetch + checkout) before the agent's first + model call so the LLM doesn't burn tokens narrating ``gh repo clone``. +- A computed unified diff and the set of (file, line) tuples in that diff, + passed via the runnable config so ``add_finding`` can validate at creation + time rather than failing at GitHub-publish time. +- A reviewer-specific tool set: ``add_finding``, ``update_finding``, + ``list_findings``, ``publish_review``. No commit/push/PR-opening tools. +- A system prompt that pins the single-evolving-findings model, in-diff-only + discipline, severity ladder, and the watch-mode reconciliation flow. """ # ruff: noqa: E402 @@ -21,6 +28,7 @@ warnings.filterwarnings("ignore", module="langchain_core._api.deprecation") warnings.filterwarnings("ignore", message=".*Pydantic V1.*", category=UserWarning) from deepagents import create_deep_agent +from deepagents.backends.protocol import SandboxBackendProtocol from langchain.agents.middleware import ModelCallLimitMiddleware from .middleware import ( @@ -28,6 +36,13 @@ from .middleware import ( SanitizeToolInputsMiddleware, ToolErrorMiddleware, ) +from .reviewer_diff import ( + compute_diff_in_sandbox, + compute_diff_line_set, +) +from .reviewer_findings import ( + list_findings as list_findings_async, +) from .server import ( DEFAULT_LLM_MAX_TOKENS, DEFAULT_LLM_MODEL_ID, @@ -37,6 +52,12 @@ from .server import ( ensure_sandbox_for_thread, graph_loaded_for_execution, ) +from .tools import ( + add_finding, + list_findings, + publish_review, + update_finding, +) from .utils.auth import resolve_github_token from .utils.github_token import get_github_token_from_thread from .utils.model import ModelKwargs, make_model @@ -44,53 +65,79 @@ from .utils.sandbox_paths import aresolve_sandbox_work_dir REVIEWER_PROMPT_TEMPLATE = """You are an expert code reviewer. -Your job is to review a single GitHub pull request and surface real issues — -bugs, security problems, correctness errors, race conditions, performance -regressions, and clear quality issues. Do not nitpick style. +Your job is to review one GitHub pull request, find real issues, record them +as structured findings, and publish a single GitHub review with the most +important findings as inline comments — with concrete suggestions where +possible so the user can click "Commit suggestion". ### Working environment -You are operating in a remote Linux sandbox at `{working_dir}`. +You are operating in a remote Linux sandbox at `{working_dir}`. The repository +has already been cloned and checked out to the PR head SHA before this run +started — you do **not** need to clone, fetch, or check out yourself. - The `gh` CLI is installed and authenticated by a sandbox proxy. Always invoke it as `GH_TOKEN=dummy gh `. -- The `execute` tool runs shell commands. The default timeout is generous - (~30 minutes); pass `timeout=` only if you need to override it. +- The `execute` tool runs shell commands. Default timeout ~30 minutes. +- `read_file`, `grep`, `glob` are available for code exploration. ### How to review -1. The user message tells you which PR to review (URL, repo, PR number, - base SHA, head SHA). -2. Clone the repo into `{working_dir}` and check out the **base SHA** so - the working tree matches `main` at the time the PR was opened. -3. Fetch the PR head and inspect the diff: - `GH_TOKEN=dummy gh pr diff --repo /` - or use `git diff ...`. -4. Read the files the PR changes — and any related files needed to - understand the change in context. Use `read_file`, `grep`, `glob`. -5. For each real issue you find, submit one inline review comment with - `GH_TOKEN=dummy gh api`: +1. The user message tells you which PR to review and includes the unified + diff. **Review the diff that's there. Don't review pre-existing code.** +2. For each real issue you find in the diff, call **`add_finding`** with: + - `severity`: one of `informational`, `low`, `medium`, `high`, `critical`. + Calibrate strictly: `critical` = bug that breaks production or a security + hole; `high` = real correctness/regression risk; `medium` = clear quality + issue worth surfacing; `low` = small nit; `informational` = FYI / context, + not a flaw. Inflated severities erode trust — be honest. + - `category`: e.g. `correctness`, `security`, `perf`, `style`, `flag`. + - `file`, `start_line`, `end_line`: anchor inside the PR diff. Use a range + when the issue spans multiple lines (e.g. an entire function). + - `description`: what's wrong, in 1–4 sentences. Markdown is fine. + - `suggestion`: a concrete replacement for `start_line..end_line` whenever + you can offer one. The published GitHub comment will render it as a + ```suggestion``` block so the user can click "Commit suggestion". +3. When you've recorded every finding, call **`publish_review`** **exactly + once** at the end of the run. It batches eligible findings into a single + GitHub PR Review with inline comments + suggestion blocks, and stores the + GitHub comment IDs back so re-reviews can later resolve threads. - `GH_TOKEN=dummy gh api repos///pulls//comments \ - -f body='' \ - -f commit_id='' \ - -f path='' \ - -F line= \ - -f side=RIGHT` +### Re-reviewing on a new commit - The `line` value must be a 1-based line number in the new post-PR file - and must be part of the PR diff. If the issue spans multiple lines, anchor - the comment to the most relevant changed line. +If the user message says **"A new commit has been pushed"**, this is a +re-review. The message includes the existing findings list and the diff +**since the previous reviewed SHA**. Your job is to: + +- For each existing **open** finding, decide whether the new commits: + - **resolved** it — call `update_finding(id, status="resolved")`. + - **left it unchanged** — do nothing. + - **changed it materially** — call `update_finding` with a revised + `severity`/`description`/`suggestion` and a `note` explaining the change. +- Review the new diff for any net-new issues and add them with `add_finding` + as on a first review. +- Finally call `publish_review` once. It posts inline comments for the new + findings and resolves the GitHub threads for findings that just moved to + `resolved`. + +You may use `list_findings()` at any time to inspect what's persisted. ### Hard rules - **You are read-only.** Do NOT commit. Do NOT push. Do NOT open or update - PRs. Do NOT post top-level PR comments via `gh pr comment`. -- One inline `gh api` comment per distinct issue. Multiple comments per review - are expected and correct. -- Do not summarize the PR in chat. Do not write a final review essay. Submit - only inline review comments for real findings. -- If you find no real issues, submit no comments and stop. + PRs. Do NOT use `gh pr review` or `gh api ... /reviews` directly — use the + `publish_review` tool instead so the findings list and GitHub stay in sync. +- **Only review the diff.** Do not flag pre-existing code that the PR didn't + touch. `add_finding` will reject ranges outside the PR diff. +- **One finding per distinct issue.** Don't split one bug into three findings, + and don't merge unrelated issues into one. +- **Prefer suggestions where you have one.** A description without a fix is + fine when there's no clear single-line fix; otherwise include the + `suggestion` field so the user gets the "Commit suggestion" button. +- **Skip nits on a clean PR.** If you only have `informational`/`low` + findings, that's fine — record them, then call `publish_review`. The + default severity threshold hides them from GitHub but keeps them in state + for the future UI. """ @@ -98,8 +145,115 @@ def _reviewer_system_prompt(working_dir: str) -> str: return REVIEWER_PROMPT_TEMPLATE.format(working_dir=working_dir) +async def _ensure_repo_checked_out( + sandbox_backend: SandboxBackendProtocol, + *, + work_dir: str, + owner: str, + repo: str, + base_sha: str, + head_sha: str, +) -> None: + """Clone-or-fetch + checkout the PR head into the sandbox. + + Idempotent: warm sandboxes that already have ``/`` just + fetch new objects and re-check out; cold sandboxes clone from scratch. + """ + repo_dir = f"{work_dir}/{repo}" + script = ( + f"set -e; " + f"if [ -d {repo_dir}/.git ]; then " + f" cd {repo_dir} && " + f" git fetch --no-tags origin {base_sha} {head_sha} && " + f" git checkout --force {head_sha}; " + f"else " + f" GH_TOKEN=dummy gh repo clone {owner}/{repo} {repo_dir} -- --quiet && " + f" cd {repo_dir} && " + f" git fetch --no-tags origin {base_sha} {head_sha} && " + f" git checkout --force {head_sha}; " + f"fi" + ) + import asyncio + + await asyncio.to_thread(sandbox_backend.execute, script) + + +def _build_first_review_context( + *, + pr_url: str, + repo_owner: str, + repo_name: str, + pr_number: int, + base_sha: str, + head_sha: str, + diff_text: str, +) -> str: + return ( + f"## Pull request to review\n\n" + f"- repo: {repo_owner}/{repo_name}\n" + f"- pr_number: {pr_number}\n" + f"- url: {pr_url}\n" + f"- base_sha: {base_sha}\n" + f"- head_sha: {head_sha}\n\n" + f"## Unified diff (review only what's here)\n\n" + f"```diff\n{diff_text}\n```\n\n" + f"This is a first review — there are no existing findings. Record real " + f"issues with `add_finding` (one per issue, with concrete `suggestion` " + f"text whenever you can offer one), then call `publish_review` once at " + f"the end." + ) + + +def _build_re_review_context( + *, + pr_url: str, + repo_owner: str, + repo_name: str, + pr_number: int, + last_reviewed_sha: str, + head_sha: str, + diff_since_last_review: str, + existing_findings_block: str, +) -> str: + return ( + f"## A new commit has been pushed\n\n" + f"- repo: {repo_owner}/{repo_name}\n" + f"- pr_number: {pr_number}\n" + f"- url: {pr_url}\n" + f"- previous reviewed SHA: {last_reviewed_sha}\n" + f"- new HEAD SHA: {head_sha}\n\n" + f"## Existing findings\n\n{existing_findings_block}\n\n" + f"## Diff since the previous reviewed SHA\n\n" + f"```diff\n{diff_since_last_review}\n```\n\n" + f"For each open finding above, decide whether the new commits resolved " + f'it (`update_finding(id, status="resolved")`), left it unchanged ' + f"(no action), or changed it materially (`update_finding` with new " + f"fields + a `note`). Then add any net-new findings introduced by the " + f"new diff, and call `publish_review` once at the end." + ) + + +def _format_existing_findings(findings: list[dict]) -> str: + if not findings: + return "_(none)_" + lines: list[str] = [] + for f in findings: + if f.get("status") != "open": + continue + location = f.get("file", "") + start = f.get("start_line") + end = f.get("end_line") + if start is not None and end is not None: + location += f":{start}" if start == end else f":{start}-{end}" + lines.append( + f"- [{f.get('id')}] ({f.get('severity')}, {f.get('category')}) " + f"{location} — {f.get('description', '').strip()}" + ) + return "\n".join(lines) if lines else "_(no open findings)_" + + async def get_reviewer_agent(config: RunnableConfig) -> Pregel: - """Get or create a reviewer agent with a sandbox for the given thread.""" + """Get or create a reviewer agent with a sandbox + prepped repo.""" thread_id = config["configurable"].get("thread_id", None) config["recursion_limit"] = DEFAULT_RECURSION_LIMIT @@ -122,15 +276,97 @@ async def get_reviewer_agent(config: RunnableConfig) -> Pregel: work_dir = await aresolve_sandbox_work_dir(sandbox_backend) + repo_config = config["configurable"].get("repo") or {} + repo_owner = str(repo_config.get("owner", "")) + repo_name = str(repo_config.get("name", "")) + base_sha = str(config["configurable"].get("base_sha", "") or "") + head_sha = str(config["configurable"].get("head_sha", "") or "") + pr_number = config["configurable"].get("pr_number") + pr_url = str(config["configurable"].get("pr_url", "") or "") + last_reviewed_sha = str(config["configurable"].get("last_reviewed_sha", "") or "") + is_re_review = bool(config["configurable"].get("re_review")) + + diff_text = "" + diff_line_set: dict[str, set[int]] = {} + if repo_owner and repo_name and base_sha and head_sha: + try: + await _ensure_repo_checked_out( + sandbox_backend, + work_dir=work_dir, + owner=repo_owner, + repo=repo_name, + base_sha=base_sha, + head_sha=head_sha, + ) + if is_re_review and last_reviewed_sha: + # Re-review delta: two-dot diff = "what changed on the head + # branch since the previous review" (forward-push case). The + # agent reconciles existing findings against this slice. + diff_text = await compute_diff_in_sandbox( + sandbox_backend, + work_dir=f"{work_dir}/{repo_name}", + base_ref=last_reviewed_sha, + head_ref=head_sha, + merge_base=False, + ) + else: + # First review: three-dot merge-base diff so we don't include + # changes that landed on `base` after the PR branch diverged. + # Matches what GitHub's "Files changed" tab shows. + diff_text = await compute_diff_in_sandbox( + sandbox_backend, + work_dir=f"{work_dir}/{repo_name}", + base_ref=base_sha, + head_ref=head_sha, + merge_base=True, + ) + diff_line_set = compute_diff_line_set(diff_text) + except Exception: + logger.exception("Reviewer prep failed for thread %s", thread_id) + + config["configurable"]["diff_text"] = diff_text + config["configurable"]["diff_line_set"] = { + path: sorted(lines) for path, lines in diff_line_set.items() + } + + review_context = "" + if pr_number is not None and isinstance(pr_number, int): + if is_re_review and last_reviewed_sha: + existing_findings = await list_findings_async(thread_id) + review_context = _build_re_review_context( + pr_url=pr_url, + repo_owner=repo_owner, + repo_name=repo_name, + pr_number=pr_number, + last_reviewed_sha=last_reviewed_sha, + head_sha=head_sha, + diff_since_last_review=diff_text, + existing_findings_block=_format_existing_findings(existing_findings), + ) + else: + review_context = _build_first_review_context( + pr_url=pr_url, + repo_owner=repo_owner, + repo_name=repo_name, + pr_number=pr_number, + base_sha=base_sha, + head_sha=head_sha, + diff_text=diff_text, + ) + model_id = os.environ.get("LLM_MODEL_ID", DEFAULT_LLM_MODEL_ID) model_kwargs: ModelKwargs = {"max_tokens": DEFAULT_LLM_MAX_TOKENS} if model_id == DEFAULT_LLM_MODEL_ID: model_kwargs["reasoning"] = DEFAULT_LLM_REASONING + system_prompt = _reviewer_system_prompt(f"{work_dir}/{repo_name}" if repo_name else work_dir) + if review_context: + system_prompt = f"{system_prompt}\n\n{review_context}" + return create_deep_agent( model=make_model(model_id, **model_kwargs), - system_prompt=_reviewer_system_prompt(work_dir), - tools=[], + system_prompt=system_prompt, + tools=[add_finding, update_finding, list_findings, publish_review], backend=sandbox_backend, middleware=[ SanitizeToolInputsMiddleware(), diff --git a/agent/reviewer_diff.py b/agent/reviewer_diff.py new file mode 100644 index 00000000..4b9c846a --- /dev/null +++ b/agent/reviewer_diff.py @@ -0,0 +1,232 @@ +"""Diff utilities for the reviewer agent. + +The reviewer needs three things from a PR diff: + +1. The set of (file, line) tuples that are part of the diff, so ``add_finding`` + can validate at creation time rather than at GitHub-publish time. +2. The hunk text relevant to a given (file, start_line, end_line) range, so we + can stash it on the Finding (``diff_hunk``) for rendering in the future UI + without re-fetching from GitHub or the (evictable) sandbox. +3. A way to compute the diff in the sandbox between two SHAs, used both on + first review (``base_sha..head_sha``) and on watched re-review + (``last_reviewed_sha..new_head_sha``). +""" + +from __future__ import annotations + +import asyncio +import logging +import re +from dataclasses import dataclass +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from deepagents.backends.protocol import SandboxBackendProtocol + +logger = logging.getLogger(__name__) + + +_DIFF_FILE_HEADER_RE = re.compile(r"^diff --git a/(?P.+?) b/(?P.+?)$") +_HUNK_HEADER_RE = re.compile( + r"^@@ -(?P\d+)(?:,(?P\d+))? " + r"\+(?P\d+)(?:,(?P\d+))? @@" +) + + +@dataclass(frozen=True) +class DiffHunk: + """One hunk for one file in a unified diff. + + ``new_start``/``new_end`` are inclusive 1-based line numbers in the + post-PR (RIGHT side) file. ``body`` is the raw hunk text including the + ``@@`` header — what gets stored on a Finding's ``diff_hunk``. + """ + + file: str + new_start: int + new_end: int + old_start: int + old_end: int + body: str + + +@dataclass(frozen=True) +class FileDiff: + """All hunks for one file in a unified diff.""" + + file: str + hunks: tuple[DiffHunk, ...] + + +def parse_unified_diff(diff_text: str) -> list[FileDiff]: + """Parse a unified diff into per-file hunk records. + + Skips ``--- ``/``+++ `` and binary file markers. Returns one ``FileDiff`` + per file with at least one hunk; files with no hunks (e.g., pure renames) + are dropped. + """ + files: list[FileDiff] = [] + lines = diff_text.splitlines() + i = 0 + while i < len(lines): + header_match = _DIFF_FILE_HEADER_RE.match(lines[i]) + if not header_match: + i += 1 + continue + file_path = header_match.group("b") + i += 1 + # Skip metadata lines until first hunk or next file header + hunks: list[DiffHunk] = [] + current_hunk_lines: list[str] = [] + current_meta: tuple[int, int, int, int] | None = None + while i < len(lines) and not _DIFF_FILE_HEADER_RE.match(lines[i]): + line = lines[i] + hunk_match = _HUNK_HEADER_RE.match(line) + if hunk_match: + if current_meta is not None and current_hunk_lines: + hunks.append( + DiffHunk( + file=file_path, + old_start=current_meta[0], + old_end=current_meta[1], + new_start=current_meta[2], + new_end=current_meta[3], + body="\n".join(current_hunk_lines), + ) + ) + old_start = int(hunk_match.group("old_start")) + old_count = int(hunk_match.group("old_count") or "1") + new_start = int(hunk_match.group("new_start")) + new_count = int(hunk_match.group("new_count") or "1") + # End line is inclusive; if count is 0 (deletion-only), end == start + old_end = old_start + max(old_count - 1, 0) + new_end = new_start + max(new_count - 1, 0) + current_meta = (old_start, old_end, new_start, new_end) + current_hunk_lines = [line] + elif current_meta is not None: + current_hunk_lines.append(line) + i += 1 + if current_meta is not None and current_hunk_lines: + hunks.append( + DiffHunk( + file=file_path, + old_start=current_meta[0], + old_end=current_meta[1], + new_start=current_meta[2], + new_end=current_meta[3], + body="\n".join(current_hunk_lines), + ) + ) + if hunks: + files.append(FileDiff(file=file_path, hunks=tuple(hunks))) + return files + + +def compute_diff_line_set(diff_text: str) -> dict[str, set[int]]: + """Return ``{file: {line, ...}}`` for the new-side lines covered by the diff. + + A ``Finding`` whose ``(file, start_line..end_line)`` range falls outside + this set cannot be rendered as an inline GitHub review comment, so + ``add_finding`` rejects it. + """ + out: dict[str, set[int]] = {} + for file_diff in parse_unified_diff(diff_text): + lines = out.setdefault(file_diff.file, set()) + for hunk in file_diff.hunks: + for line in range(hunk.new_start, hunk.new_end + 1): + lines.add(line) + return out + + +def extract_diff_hunk( + diff_text: str, + file: str, + start_line: int | None, + end_line: int | None, +) -> str | None: + """Extract the hunk body covering ``file:start_line..end_line``. + + Returns ``None`` if no hunk overlaps. For file-level findings (both lines + None) returns the first hunk in the file as best-effort context. + """ + file_diffs = [fd for fd in parse_unified_diff(diff_text) if fd.file == file] + if not file_diffs: + return None + hunks = file_diffs[0].hunks + if not hunks: + return None + if start_line is None or end_line is None: + return hunks[0].body + for hunk in hunks: + if hunk.new_start <= end_line and start_line <= hunk.new_end: + return hunk.body + return None + + +def is_range_in_diff( + line_set: dict[str, set[int]], + file: str, + start_line: int | None, + end_line: int | None, +) -> bool: + """Return True if every line in ``start_line..end_line`` is in the diff. + + File-level findings (both None) are always allowed. + """ + if start_line is None and end_line is None: + return True + if start_line is None or end_line is None: + return False + file_lines = line_set.get(file) + if not file_lines: + return False + return all(line in file_lines for line in range(start_line, end_line + 1)) + + +async def compute_diff_in_sandbox( + sandbox_backend: SandboxBackendProtocol, + work_dir: str, + base_ref: str, + head_ref: str, + *, + merge_base: bool = False, +) -> str: + """Run ``git diff`` inside the sandbox and return its stdout. + + Refs can be SHAs or branch names. Caller is responsible for ensuring both + refs exist locally (e.g., having fetched the PR head). + + Args: + merge_base: When ``True``, use three-dot ``base...head`` (the merge-base + diff — what GitHub shows on the PR's "Files changed" tab). Use this + for first review so we don't pick up changes that landed on the + base branch after the PR diverged. When ``False``, use two-dot + ``base..head`` — appropriate for re-review deltas where ``base`` is + the previously reviewed SHA and we want exactly the commits added + since. + """ + operator = "..." if merge_base else ".." + cmd = f"cd {work_dir} && git diff --no-color {base_ref}{operator}{head_ref}" + result = await asyncio.to_thread(sandbox_backend.execute, cmd) + return _stdout_from_result(result) + + +def _stdout_from_result(result: object) -> str: + """Best-effort extraction of stdout from a sandbox execute() result. + + Different sandbox providers return different shapes; this normalizes them. + """ + if isinstance(result, str): + return result + if isinstance(result, dict): + for key in ("stdout", "output", "text"): + value = result.get(key) + if isinstance(value, str): + return value + stdout = getattr(result, "stdout", None) + if isinstance(stdout, str): + return stdout + text = getattr(result, "text", None) + if isinstance(text, str): + return text + return "" diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py new file mode 100644 index 00000000..bd282547 --- /dev/null +++ b/agent/reviewer_findings.py @@ -0,0 +1,275 @@ +"""Findings storage for the reviewer agent. + +Findings live in LangGraph thread metadata under the canonical reviewer thread +for a PR. This file owns the Finding schema and the read/write helpers that +the reviewer's tools and webhook handlers go through. + +Why thread metadata: it survives sandbox eviction, is queryable cross-thread +via the langgraph SDK (a future UI lists all reviewer threads by filtering on +``metadata.kind == "reviewer"``), and matches existing patterns the codebase +already uses for ``sandbox_id``, ``github_token_encrypted``, etc. +""" + +from __future__ import annotations + +import logging +import uuid +from typing import Any, Literal, TypedDict, cast + +from langgraph.config import get_config +from langgraph_sdk import get_client + +logger = logging.getLogger(__name__) + +REVIEWER_THREAD_KIND = "reviewer" + +Severity = Literal["informational", "low", "medium", "high", "critical"] +FindingStatus = Literal["open", "resolved", "dismissed"] +DiffSide = Literal["LEFT", "RIGHT"] + +SEVERITY_ORDER: dict[Severity, int] = { + "informational": 0, + "low": 1, + "medium": 2, + "high": 3, + "critical": 4, +} + + +class Finding(TypedDict, total=False): + """A single review finding. + + All fields are optional at the TypedDict level so partial updates are + representable, but ``new_finding`` always returns a fully populated dict. + """ + + id: str + severity: Severity + category: str + file: str + start_line: int | None + end_line: int | None + side: DiffSide + description: str + suggestion: str | None + status: FindingStatus + first_seen_sha: str + last_confirmed_sha: str + github_review_comment_id: int | None + diff_hunk: str | None + + +class ReviewerPRMeta(TypedDict, total=False): + """PR identity stored on reviewer thread metadata, used by the UI.""" + + owner: str + name: str + number: int + url: str + title: str + head_ref: str + base_ref: str + + +def new_finding_id() -> str: + """Return a stable, short, URL-friendly finding id (``f_``).""" + return f"f_{uuid.uuid4().hex[:10]}" + + +def new_finding( + *, + severity: Severity, + category: str, + file: str, + start_line: int | None, + end_line: int | None, + description: str, + sha: str, + side: DiffSide = "RIGHT", + suggestion: str | None = None, + diff_hunk: str | None = None, + finding_id: str | None = None, +) -> Finding: + """Construct a fully-populated ``Finding`` ready to persist.""" + return { + "id": finding_id or new_finding_id(), + "severity": severity, + "category": category, + "file": file, + "start_line": start_line, + "end_line": end_line, + "side": side, + "description": description, + "suggestion": suggestion, + "status": "open", + "first_seen_sha": sha, + "last_confirmed_sha": sha, + "github_review_comment_id": None, + "diff_hunk": diff_hunk, + } + + +def _coerce_finding(value: Any) -> Finding | None: + if not isinstance(value, dict): + return None + if "id" not in value or not isinstance(value["id"], str): + return None + return cast(Finding, value) + + +def _coerce_findings_list(value: Any) -> list[Finding]: + if not isinstance(value, list): + return [] + out: list[Finding] = [] + for entry in value: + finding = _coerce_finding(entry) + if finding is not None: + out.append(finding) + return out + + +def get_thread_id_from_runtime() -> str: + """Return the thread id from the current LangGraph runnable config.""" + config = get_config() + configurable = config.get("configurable", {}) if isinstance(config, dict) else {} + thread_id = configurable.get("thread_id") if isinstance(configurable, dict) else None + if not isinstance(thread_id, str) or not thread_id: + msg = "No thread_id available in runtime config" + raise RuntimeError(msg) + return thread_id + + +async def get_thread_metadata(thread_id: str) -> dict[str, Any]: + """Fetch the current metadata for a thread. Returns ``{}`` on miss.""" + client = get_client() + try: + thread = await client.threads.get(thread_id) + except Exception: # noqa: BLE001 + logger.exception("Failed to fetch thread metadata for %s", thread_id) + return {} + metadata = thread.get("metadata") if isinstance(thread, dict) else None + return metadata if isinstance(metadata, dict) else {} + + +async def list_findings(thread_id: str) -> list[Finding]: + """Return all findings persisted on the reviewer thread.""" + metadata = await get_thread_metadata(thread_id) + return _coerce_findings_list(metadata.get("findings")) + + +async def get_finding(thread_id: str, finding_id: str) -> Finding | None: + """Return one finding by id, or ``None`` if not present.""" + findings = await list_findings(thread_id) + for finding in findings: + if finding.get("id") == finding_id: + return finding + return None + + +async def replace_findings(thread_id: str, findings: list[Finding]) -> None: + """Overwrite the findings list on a thread's metadata.""" + client = get_client() + await client.threads.update(thread_id=thread_id, metadata={"findings": findings}) + + +async def append_finding(thread_id: str, finding: Finding) -> Finding: + """Append a finding and persist the new list.""" + findings = await list_findings(thread_id) + findings.append(finding) + await replace_findings(thread_id, findings) + return finding + + +async def update_finding_fields( + thread_id: str, + finding_id: str, + updates: dict[str, Any], +) -> Finding | None: + """Apply field updates to one finding by id and persist.""" + findings = await list_findings(thread_id) + updated: Finding | None = None + for finding in findings: + if finding.get("id") == finding_id: + finding.update(updates) + updated = finding + break + if updated is None: + return None + await replace_findings(thread_id, findings) + return updated + + +async def set_reviewer_thread_metadata( + thread_id: str, + *, + pr: ReviewerPRMeta | None = None, + last_reviewed_sha: str | None = None, + watch: bool | None = None, + findings: list[Finding] | None = None, + extra: dict[str, Any] | None = None, +) -> None: + """Persist reviewer-thread-level metadata. + + Always sets ``kind=reviewer`` so the future UI can list reviewer threads by + filtering on metadata. Only includes the fields the caller passed in + (langgraph metadata updates merge rather than overwrite). + """ + client = get_client() + metadata: dict[str, Any] = {"kind": REVIEWER_THREAD_KIND} + if pr is not None: + metadata["pr"] = pr + if last_reviewed_sha is not None: + metadata["last_reviewed_sha"] = last_reviewed_sha + if watch is not None: + metadata["watch"] = watch + if findings is not None: + metadata["findings"] = findings + if extra: + metadata.update(extra) + await client.threads.update(thread_id=thread_id, metadata=metadata) + + +def get_thread_watch_flag(metadata: dict[str, Any]) -> bool: + return bool(metadata.get("watch")) + + +def get_thread_last_reviewed_sha(metadata: dict[str, Any]) -> str | None: + value = metadata.get("last_reviewed_sha") + return value if isinstance(value, str) and value else None + + +def get_thread_pr_meta(metadata: dict[str, Any]) -> ReviewerPRMeta | None: + pr = metadata.get("pr") + if not isinstance(pr, dict): + return None + return cast(ReviewerPRMeta, pr) + + +def filter_findings_for_publish( + findings: list[Finding], + *, + severity_threshold: Severity = "medium", + cap: int = 4, +) -> list[Finding]: + """Return findings to surface to GitHub. + + - status must be ``open`` + - severity must be at or above ``severity_threshold`` + - sorted by severity descending, then file/start_line for stable ordering + - capped at ``cap`` to avoid review spam + """ + threshold_rank = SEVERITY_ORDER[severity_threshold] + eligible = [ + finding + for finding in findings + if finding.get("status", "open") == "open" + and SEVERITY_ORDER.get(finding.get("severity", "informational"), 0) >= threshold_rank + ] + eligible.sort( + key=lambda f: ( + -SEVERITY_ORDER.get(f.get("severity", "informational"), 0), + f.get("file", ""), + f.get("start_line") or 0, + ) + ) + return eligible[:cap] diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py new file mode 100644 index 00000000..2bda2427 --- /dev/null +++ b/agent/reviewer_publish.py @@ -0,0 +1,292 @@ +"""GitHub Reviews API + GraphQL resolveReviewThread for the reviewer agent. + +The reviewer agent calls ``publish_review`` at the end of a run. That tool +batches eligible findings (severity ≥ threshold, status=open, capped) into a +single GitHub PR Review: + +- Review body: agent-authored summary line. +- Inline comments: one per surfaced finding, anchored to ``path`` + ``line`` + (+ ``start_line`` for ranges) + ``side``. +- Suggestion: when ``finding.suggestion`` is set, appended to the comment body + as a fenced ```suggestion``` block — gives the user the "Commit suggestion" + button on GitHub. + +After publish, the returned per-comment IDs get stored back on each Finding as +``github_review_comment_id``. On a re-review run, when a finding moves +``open`` → ``resolved``, ``resolve_review_thread`` is called for that ID via +the GraphQL ``resolveReviewThread`` mutation (REST doesn't expose this). +""" + +from __future__ import annotations + +import logging +from typing import Any + +import httpx + +from .reviewer_findings import Finding + +logger = logging.getLogger(__name__) + + +_GITHUB_API_BASE = "https://api.github.com" +_GITHUB_GRAPHQL = "https://api.github.com/graphql" +_GITHUB_HEADERS_VERSION = "2022-11-28" + + +def render_inline_comment_body(finding: Finding) -> str: + """Render the body of one inline review comment. + + Format: + + + + ```suggestion + + ``` + + The suggestion block is only included when ``finding.suggestion`` is set. + Multi-line suggestions just become multi-line ```suggestion``` blocks. + """ + description = finding.get("description", "") or "" + suggestion = finding.get("suggestion") + if not suggestion: + return description + return f"{description}\n\n```suggestion\n{suggestion}\n```" + + +def render_inline_comment_payload(finding: Finding) -> dict[str, Any] | None: + """Render one finding into the payload shape GitHub's Reviews API expects. + + Returns ``None`` for file-level findings (no line range), since the Reviews + API requires inline comments to be anchored to a line. + """ + file = finding.get("file") + start_line = finding.get("start_line") + end_line = finding.get("end_line") + side = finding.get("side", "RIGHT") + if not file or end_line is None: + return None + payload: dict[str, Any] = { + "path": file, + "line": end_line, + "side": side, + "body": render_inline_comment_body(finding), + } + if start_line is not None and start_line != end_line: + payload["start_line"] = start_line + payload["start_side"] = side + return payload + + +def render_review_body( + *, + pr_number: int, + surfaced_count: int, + total_open_count: int, + severity_threshold: str, + summary: str | None, +) -> str: + """Compose the top-level review body. + + Includes the agent's summary (if any) and a footer line so reviewers know + when findings were filtered out below the surfacing threshold. + """ + parts: list[str] = [] + if summary: + parts.append(summary.strip()) + if surfaced_count == 0: + parts.append("_No issues at or above the configured severity threshold._") + else: + hidden = total_open_count - surfaced_count + if hidden > 0: + parts.append( + f"_Showing {surfaced_count} finding{'s' if surfaced_count != 1 else ''} " + f"at severity ≥ `{severity_threshold}`; {hidden} lower-severity " + f"finding{'s' if hidden != 1 else ''} hidden._" + ) + parts.append(f"") + return "\n\n".join(p for p in parts if p) + + +async def post_pull_request_review( + *, + owner: str, + repo: str, + pr_number: int, + head_sha: str, + body: str, + inline_comments: list[dict[str, Any]], + token: str, +) -> dict[str, Any] | None: + """POST one GitHub PR Review with inline comments. Returns the API response or None.""" + url = f"{_GITHUB_API_BASE}/repos/{owner}/{repo}/pulls/{pr_number}/reviews" + payload: dict[str, Any] = { + "commit_id": head_sha, + "event": "COMMENT", + "body": body, + "comments": inline_comments, + } + headers = _github_headers(token) + async with httpx.AsyncClient() as client: + try: + response = await client.post(url, headers=headers, json=payload, timeout=30) + response.raise_for_status() + except httpx.HTTPError: + logger.exception("Failed to POST PR review for %s/%s#%s", owner, repo, pr_number) + return None + data = response.json() + return data if isinstance(data, dict) else None + + +async def fetch_review_comments( + *, + owner: str, + repo: str, + pr_number: int, + review_id: int, + token: str, +) -> list[dict[str, Any]]: + """List the inline comments for a posted review. + + GitHub's review-creation response includes a ``comments`` count but not the + per-comment IDs in all paths; this paginates the canonical list endpoint. + """ + url = f"{_GITHUB_API_BASE}/repos/{owner}/{repo}/pulls/{pr_number}/reviews/{review_id}/comments" + headers = _github_headers(token) + out: list[dict[str, Any]] = [] + params: dict[str, Any] = {"per_page": 100, "page": 1} + async with httpx.AsyncClient() as client: + while True: + try: + response = await client.get(url, headers=headers, params=params, timeout=30) + response.raise_for_status() + except httpx.HTTPError: + logger.exception( + "Failed to list review comments for review %s on %s/%s", + review_id, + owner, + repo, + ) + break + data = response.json() + if not isinstance(data, list) or not data: + break + out.extend(item for item in data if isinstance(item, dict)) + if len(data) < 100: # noqa: PLR2004 + break + params["page"] += 1 + return out + + +async def fetch_review_thread_id_for_comment( + *, + owner: str, + repo: str, + pr_number: int, + review_comment_id: int, + token: str, +) -> str | None: + """Resolve the GraphQL review-thread node id for a REST review-comment id. + + GitHub's GraphQL API resolves "threads" rather than individual comments; to + resolve a thread we need its node id. The REST review-comment id is mapped + to the thread by walking the PR's review threads. + """ + query = """ + query Threads($owner: String!, $repo: String!, $pr: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + reviewThreads(first: 50, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { + id + comments(first: 50) { nodes { databaseId } } + } + } + } + } + } + """ + cursor: str | None = None + async with httpx.AsyncClient() as client: + while True: + try: + response = await client.post( + _GITHUB_GRAPHQL, + headers={"Authorization": f"Bearer {token}"}, + json={ + "query": query, + "variables": { + "owner": owner, + "repo": repo, + "pr": pr_number, + "cursor": cursor, + }, + }, + timeout=30, + ) + response.raise_for_status() + except httpx.HTTPError: + logger.exception( + "Failed to fetch review threads for %s/%s#%s", + owner, + repo, + pr_number, + ) + return None + data = response.json() + threads = ( + data.get("data", {}) + .get("repository", {}) + .get("pullRequest", {}) + .get("reviewThreads", {}) + ) + for thread in threads.get("nodes", []) or []: + comment_ids = { + c.get("databaseId") for c in (thread.get("comments", {}).get("nodes") or []) + } + if review_comment_id in comment_ids: + node_id = thread.get("id") + return node_id if isinstance(node_id, str) else None + page_info = threads.get("pageInfo") or {} + if not page_info.get("hasNextPage"): + return None + cursor = page_info.get("endCursor") + + +async def resolve_review_thread(*, thread_node_id: str, token: str) -> bool: + """Mark a review thread as resolved via the GraphQL ``resolveReviewThread`` mutation.""" + mutation = """ + mutation Resolve($threadId: ID!) { + resolveReviewThread(input: {threadId: $threadId}) { + thread { id isResolved } + } + } + """ + async with httpx.AsyncClient() as client: + try: + response = await client.post( + _GITHUB_GRAPHQL, + headers={"Authorization": f"Bearer {token}"}, + json={"query": mutation, "variables": {"threadId": thread_node_id}}, + timeout=30, + ) + response.raise_for_status() + except httpx.HTTPError: + logger.exception("Failed to resolve review thread %s", thread_node_id) + return False + data = response.json() + if data.get("errors"): + logger.warning("resolveReviewThread errors: %s", data["errors"]) + return False + thread = data.get("data", {}).get("resolveReviewThread", {}).get("thread", {}) + return bool(thread.get("isResolved")) + + +def _github_headers(token: str) -> dict[str, str]: + return { + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": _GITHUB_HEADERS_VERSION, + } diff --git a/agent/tools/__init__.py b/agent/tools/__init__.py index 4a5baf86..144dace3 100644 --- a/agent/tools/__init__.py +++ b/agent/tools/__init__.py @@ -1,3 +1,4 @@ +from .add_finding import add_finding from .fetch_url import fetch_url from .http_request import http_request from .linear_comment import linear_comment @@ -7,12 +8,16 @@ from .linear_get_issue import linear_get_issue from .linear_get_issue_comments import linear_get_issue_comments from .linear_list_teams import linear_list_teams from .linear_update_issue import linear_update_issue +from .list_findings import list_findings +from .publish_review import publish_review from .request_pr_review import request_pr_review from .slack_read_thread_messages import slack_read_thread_messages from .slack_thread_reply import slack_thread_reply +from .update_finding import update_finding from .web_search import web_search __all__ = [ + "add_finding", "fetch_url", "http_request", "linear_comment", @@ -22,8 +27,11 @@ __all__ = [ "linear_get_issue_comments", "linear_list_teams", "linear_update_issue", + "list_findings", + "publish_review", "request_pr_review", "slack_read_thread_messages", "slack_thread_reply", + "update_finding", "web_search", ] diff --git a/agent/tools/add_finding.py b/agent/tools/add_finding.py new file mode 100644 index 00000000..d3033f0a --- /dev/null +++ b/agent/tools/add_finding.py @@ -0,0 +1,124 @@ +"""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] diff --git a/agent/tools/list_findings.py b/agent/tools/list_findings.py new file mode 100644 index 00000000..eb15544a --- /dev/null +++ b/agent/tools/list_findings.py @@ -0,0 +1,36 @@ +"""Tool: ``list_findings``. Return findings persisted on the reviewer thread.""" + +from __future__ import annotations + +import asyncio +from typing import Any + +from ..reviewer_findings import ( + get_thread_id_from_runtime, +) +from ..reviewer_findings import ( + list_findings as list_findings_async, +) + + +def list_findings(status_filter: str | None = None) -> dict[str, Any]: + """List findings on the reviewer thread, optionally filtered by status. + + Most useful on a re-review run to inspect what existed before deciding + which findings the new commits resolved. + + Args: + status_filter: One of ``open``, ``resolved``, ``dismissed``. ``None`` + (default) returns every finding regardless of status. + + Returns: + Dictionary with ``findings`` (list) and ``count`` (int). + """ + if status_filter is not None and status_filter not in {"open", "resolved", "dismissed"}: + return {"findings": [], "count": 0, "error": f"Invalid status_filter: {status_filter}"} + + thread_id = get_thread_id_from_runtime() + findings = asyncio.run(list_findings_async(thread_id)) + if status_filter is not None: + findings = [f for f in findings if f.get("status") == status_filter] + return {"findings": findings, "count": len(findings)} diff --git a/agent/tools/publish_review.py b/agent/tools/publish_review.py new file mode 100644 index 00000000..422d897f --- /dev/null +++ b/agent/tools/publish_review.py @@ -0,0 +1,297 @@ +"""Tool: ``publish_review``. Post the findings list to GitHub as a PR Review.""" + +from __future__ import annotations + +import asyncio +from typing import Any + +from langgraph.config import get_config + +from ..reviewer_findings import ( + Severity, + filter_findings_for_publish, + get_thread_id_from_runtime, + replace_findings, + set_reviewer_thread_metadata, +) +from ..reviewer_findings import ( + list_findings as list_findings_async, +) +from ..reviewer_publish import ( + fetch_review_comments, + fetch_review_thread_id_for_comment, + post_pull_request_review, + render_inline_comment_payload, + render_review_body, + resolve_review_thread, +) +from ..utils.github_token import get_github_token + + +def publish_review( + summary: str | None = None, + severity_threshold: str = "medium", + cap: int = 4, +) -> dict[str, Any]: + """Post all current findings to the PR as a GitHub Review. + + Call this once at the end of a review run, after you have finished adding + findings (and, on a re-review, after marking resolved findings via + ``update_finding``). It will: + + 1. Read findings from the reviewer thread. + 2. Filter to status=open and severity ≥ ``severity_threshold``, capped + at ``cap`` to avoid review spam. + 3. POST a single GitHub PR Review with the eligible findings as inline + comments. ``finding.suggestion`` becomes a ```suggestion``` block + (the "Commit suggestion" UX). + 4. Store the returned per-comment IDs back on each finding so a future + re-review can resolve those threads on GitHub when the issues are fixed. + 5. For findings whose status moved ``open`` → ``resolved`` since the last + publish, resolve their existing GitHub review threads via the GraphQL + ``resolveReviewThread`` mutation. + 6. Update ``last_reviewed_sha`` on the thread to the current head SHA. + + Args: + summary: Optional 1–2 sentence top-level take on the PR. Rendered as + the review body. Skip if you have nothing useful to say beyond + the per-finding comments. + severity_threshold: Lowest severity to surface to GitHub (default + ``medium``). Lower-severity findings stay in state and surface in + the future UI but not on the PR. + cap: Maximum number of inline comments to publish (default 4). + + Returns: + Dictionary with ``success``, ``review_id``, ``surfaced_count``, + ``hidden_count``, ``resolved_thread_count``. + """ + if severity_threshold not in {"informational", "low", "medium", "high", "critical"}: + return {"success": False, "error": f"Invalid severity_threshold: {severity_threshold}"} + + config = get_config() + configurable = config.get("configurable", {}) if isinstance(config, dict) else {} + repo_config = configurable.get("repo") if isinstance(configurable, dict) else None + pr_number = configurable.get("pr_number") if isinstance(configurable, dict) else None + head_sha = configurable.get("head_sha") if isinstance(configurable, dict) else None + + if ( + not isinstance(repo_config, dict) + or not repo_config.get("owner") + or not repo_config.get("name") + ): + return {"success": False, "error": "Missing repo info in run config"} + if not isinstance(pr_number, int): + return {"success": False, "error": "Missing pr_number in run config"} + if not isinstance(head_sha, str) or not head_sha: + return {"success": False, "error": "Missing head_sha in run config"} + + token = get_github_token() + if not token: + return {"success": False, "error": "No GitHub token available"} + + return asyncio.run( + _publish_review_async( + owner=str(repo_config["owner"]), + repo=str(repo_config["name"]), + pr_number=pr_number, + head_sha=head_sha, + token=token, + summary=summary, + severity_threshold=_cast_severity(severity_threshold), + cap=cap, + ) + ) + + +def _cast_severity(value: str) -> Severity: + return value # type: ignore[return-value] + + +async def _publish_review_async( + *, + owner: str, + repo: str, + pr_number: int, + head_sha: str, + token: str, + summary: str | None, + severity_threshold: Severity, + cap: int, +) -> dict[str, Any]: + thread_id = get_thread_id_from_runtime() + findings = await list_findings_async(thread_id) + + # Re-reviews only post NEW findings. Anything with a github_review_comment_id + # already lives on GitHub from a prior publish — reposting would create + # duplicate inline comments and break the resolve-on-fix flow (only + # whichever duplicate id we'd cache last would resolve later). + unpublished_findings = [ + f for f in findings if not isinstance(f.get("github_review_comment_id"), int) + ] + open_unpublished = [f for f in unpublished_findings if f.get("status", "open") == "open"] + eligible = filter_findings_for_publish( + unpublished_findings, severity_threshold=severity_threshold, cap=cap + ) + + inline_comments: list[dict[str, Any]] = [] + eligible_with_payload: list[tuple[dict[str, Any], dict[str, Any]]] = [] + for finding in eligible: + payload = render_inline_comment_payload(finding) + if payload is None: + continue + inline_comments.append(payload) + eligible_with_payload.append((dict(finding), payload)) + + review_body = render_review_body( + pr_number=pr_number, + surfaced_count=len(inline_comments), + total_open_count=len(open_unpublished), + severity_threshold=severity_threshold, + summary=summary, + ) + + review_id: int | None = None + if inline_comments or summary: + review_response = await post_pull_request_review( + owner=owner, + repo=repo, + pr_number=pr_number, + head_sha=head_sha, + body=review_body, + inline_comments=inline_comments, + token=token, + ) + if review_response is None: + return {"success": False, "error": "Failed to POST PR review"} + review_id = review_response.get("id") if isinstance(review_response, dict) else None + + if review_id is not None and inline_comments: + comment_records = await fetch_review_comments( + owner=owner, + repo=repo, + pr_number=pr_number, + review_id=review_id, + token=token, + ) + await _store_comment_ids_on_findings( + thread_id=thread_id, + findings=findings, + eligible_with_payload=eligible_with_payload, + comment_records=comment_records, + ) + + resolved_thread_count = await _resolve_threads_for_resolved_findings( + owner=owner, + repo=repo, + pr_number=pr_number, + token=token, + findings=await list_findings_async(thread_id), + ) + + await set_reviewer_thread_metadata(thread_id, last_reviewed_sha=head_sha) + + return { + "success": True, + "review_id": review_id, + "surfaced_count": len(inline_comments), + "hidden_count": max(len(open_unpublished) - len(inline_comments), 0), + "resolved_thread_count": resolved_thread_count, + } + + +async def _store_comment_ids_on_findings( + *, + thread_id: str, + findings: list[dict[str, Any]], + eligible_with_payload: list[tuple[dict[str, Any], dict[str, Any]]], + comment_records: list[dict[str, Any]], +) -> None: + """Match returned GitHub comment ids back to the findings that produced them. + + Match key is ``(path, line, body)`` since we don't have a server-side hint + pointing each REST comment to its source finding. + """ + by_key: dict[tuple[str, int, str], int] = {} + for record in comment_records: + path = record.get("path") + line = record.get("line") or record.get("original_line") + body = record.get("body", "") + comment_id = record.get("id") + if ( + isinstance(path, str) + and isinstance(line, int) + and isinstance(body, str) + and isinstance(comment_id, int) + ): + by_key[(path, line, body)] = comment_id + + updated = False + findings_by_id = {f.get("id"): f for f in findings} + for finding_snapshot, payload in eligible_with_payload: + line_value = payload.get("line") + if not isinstance(line_value, int): + continue + key = ( + str(payload.get("path", "")), + line_value, + str(payload.get("body", "")), + ) + comment_id = by_key.get(key) + if comment_id is None: + continue + finding = findings_by_id.get(finding_snapshot.get("id")) + if finding is None: + continue + finding["github_review_comment_id"] = comment_id + updated = True + + if updated: + await replace_findings(thread_id, list(findings_by_id.values())) + + +async def _resolve_threads_for_resolved_findings( + *, + owner: str, + repo: str, + pr_number: int, + token: str, + findings: list[dict[str, Any]], +) -> int: + """Resolve GitHub review threads for findings that just transitioned to resolved. + + A finding qualifies if: + - status == ``resolved`` + - has a ``github_review_comment_id`` from a prior publish + - has not already been GitHub-resolved (tracked via + ``github_thread_resolved`` flag we write back here) + """ + resolved_count = 0 + mutated = False + for finding in findings: + if finding.get("status") != "resolved": + continue + comment_id = finding.get("github_review_comment_id") + if not isinstance(comment_id, int): + continue + if finding.get("github_thread_resolved"): + continue + thread_node_id = await fetch_review_thread_id_for_comment( + owner=owner, + repo=repo, + pr_number=pr_number, + review_comment_id=comment_id, + token=token, + ) + if not thread_node_id: + continue + ok = await resolve_review_thread(thread_node_id=thread_node_id, token=token) + if ok: + finding["github_thread_resolved"] = True + mutated = True + resolved_count += 1 + + if mutated: + thread_id = get_thread_id_from_runtime() + await replace_findings(thread_id, findings) + + return resolved_count diff --git a/agent/tools/update_finding.py b/agent/tools/update_finding.py new file mode 100644 index 00000000..06e452be --- /dev/null +++ b/agent/tools/update_finding.py @@ -0,0 +1,80 @@ +"""Tool: ``update_finding``. Mutate an existing finding by id.""" + +from __future__ import annotations + +import asyncio +from typing import Any + +from langgraph.config import get_config + +from ..reviewer_findings import ( + get_thread_id_from_runtime, + update_finding_fields, +) + + +def update_finding( + finding_id: str, + status: str | None = None, + severity: str | None = None, + description: str | None = None, + suggestion: str | None = None, + note: str | None = None, +) -> dict[str, Any]: + """Update fields on an existing finding. + + Use this on a re-review run to mark an existing finding as resolved or + dismissed, or to revise its severity/description/suggestion if the new + commits changed the situation. + + Args: + finding_id: The id returned by ``add_finding`` (or shown in the + ``Existing findings`` block of the re-review user message). + status: New status (``open``, ``resolved``, ``dismissed``). + Use ``resolved`` when the new commits address the issue. + severity: New severity, if reassessing. + description: New description body, if revising. + suggestion: New replacement text. Pass an empty string to clear it. + note: Optional free-form note explaining the change. Persisted on the + finding under ``last_update_note``. + + Returns: + Dictionary with ``success`` and (on success) the updated ``finding``. + """ + if status is not None and status not in {"open", "resolved", "dismissed"}: + return {"success": False, "error": f"Invalid status: {status}"} + if severity is not None and severity not in { + "informational", + "low", + "medium", + "high", + "critical", + }: + return {"success": False, "error": f"Invalid severity: {severity}"} + + updates: dict[str, Any] = {} + if status is not None: + updates["status"] = status + if severity is not None: + updates["severity"] = severity + if description is not None: + updates["description"] = description + if suggestion is not None: + updates["suggestion"] = suggestion or None + if note is not None: + updates["last_update_note"] = note + + config = get_config() + configurable = config.get("configurable", {}) if isinstance(config, dict) else {} + head_sha = configurable.get("head_sha", "") if isinstance(configurable, dict) else "" + if status == "open" and isinstance(head_sha, str) and head_sha: + updates["last_confirmed_sha"] = head_sha + + if not updates: + return {"success": False, "error": "No fields provided to update"} + + thread_id = get_thread_id_from_runtime() + updated = asyncio.run(update_finding_fields(thread_id, finding_id, updates)) + if updated is None: + return {"success": False, "error": f"No finding found with id {finding_id}"} + return {"success": True, "finding": updated} diff --git a/agent/webapp.py b/agent/webapp.py index 6a36ab50..5ec64b78 100644 --- a/agent/webapp.py +++ b/agent/webapp.py @@ -16,6 +16,11 @@ from langchain_core.messages.content import create_text_block from langgraph_sdk import get_client from langgraph_sdk.client import LangGraphClient +from .reviewer_findings import ( + REVIEWER_THREAD_KIND, + ReviewerPRMeta, + set_reviewer_thread_metadata, +) from .utils.auth import ( is_bot_token_only_mode, persist_encrypted_github_token, @@ -1209,10 +1214,11 @@ _SUPPORTED_GH_EVENTS = frozenset( "pull_request", "pull_request_review_comment", "pull_request_review", + "push", ] ) _SUPPORTED_GH_ISSUE_ACTIONS = frozenset(["edited", "opened", "reopened"]) -_SUPPORTED_GH_PULL_REQUEST_ACTIONS = frozenset(["review_requested"]) +_SUPPORTED_GH_PULL_REQUEST_ACTIONS = frozenset(["review_requested", "closed", "reopened"]) _SUPPORTED_GH_COMMENT_ACTIONS = { "issue_comment": frozenset(["created", "edited"]), "pull_request_review_comment": frozenset(["created", "edited"]), @@ -1395,6 +1401,8 @@ async def trigger_pr_review_from_ref( head = pr_metadata.get("head", {}) head_sha = head.get("sha", "") branch_name = head.get("ref", "") + base_ref = pr_metadata.get("base", {}).get("ref", "") + pr_title = pr_metadata.get("title", "") pr_url = pr_metadata.get("html_url", "") or pr_ref.url if not base_sha or not head_sha: logger.warning("Missing base/head SHA for Slack PR review request") @@ -1411,17 +1419,29 @@ async def trigger_pr_review_from_ref( logger.warning("Could not persist bot token for reviewer thread %s", thread_id) return {"success": False, "error": "Could not persist reviewer token"} - prompt = build_github_pr_review_prompt(repo_config, pr_ref.number, pr_url, base_sha, head_sha) - configurable: dict[str, Any] = { - "source": source, - "github_login": github_login, - "github_user_id": github_user_id, - "repo": repo_config, - "pr_number": pr_ref.number, - "review_requested": True, + pr_meta: ReviewerPRMeta = { + "owner": pr_ref.owner, + "name": pr_ref.repo, + "number": pr_ref.number, + "url": pr_url, + "title": pr_title, + "head_ref": branch_name, + "base_ref": base_ref, } - if branch_name: - configurable["branch_name"] = branch_name + await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True) + + prompt = build_github_pr_review_prompt(repo_config, pr_ref.number, pr_url, base_sha, head_sha) + configurable = _build_reviewer_configurable( + source=source, + github_login=github_login, + github_user_id=github_user_id, + repo_config=repo_config, + pr_number=pr_ref.number, + pr_url=pr_url, + base_sha=base_sha, + head_sha=head_sha, + branch_name=branch_name, + ) thread_active = await is_thread_active(thread_id) if thread_active: @@ -1440,6 +1460,40 @@ async def trigger_pr_review_from_ref( return {"success": True, "queued": False, "thread_id": thread_id, "pr_url": pr_url} +def _build_reviewer_configurable( + *, + source: str, + github_login: str, + github_user_id: int | None, + repo_config: dict[str, str], + pr_number: int, + pr_url: str, + base_sha: str, + head_sha: str, + branch_name: str, + re_review: bool = False, + last_reviewed_sha: str = "", +) -> dict[str, Any]: + """Assemble the runnable-config ``configurable`` dict for a reviewer run.""" + configurable: dict[str, Any] = { + "source": source, + "github_login": github_login, + "github_user_id": github_user_id, + "repo": repo_config, + "pr_number": pr_number, + "pr_url": pr_url, + "base_sha": base_sha, + "head_sha": head_sha, + "review_requested": True, + "re_review": re_review, + } + if branch_name: + configurable["branch_name"] = branch_name + if last_reviewed_sha: + configurable["last_reviewed_sha"] = last_reviewed_sha + return configurable + + async def process_github_pr_review_request(payload: dict[str, Any]) -> None: """Trigger the reviewer agent when the Open SWE bot is requested on a PR.""" repo = payload.get("repository", {}) @@ -1451,8 +1505,10 @@ async def process_github_pr_review_request(payload: dict[str, Any]) -> None: pr_number = pull_request.get("number") pr_url = pull_request.get("html_url", "") or pull_request.get("url", "") branch_name = pull_request.get("head", {}).get("ref", "") + base_ref = pull_request.get("base", {}).get("ref", "") base_sha = pull_request.get("base", {}).get("sha", "") head_sha = pull_request.get("head", {}).get("sha", "") + pr_title = pull_request.get("title", "") github_login = payload.get("sender", {}).get("login", "") github_user_id = payload.get("sender", {}).get("id") @@ -1479,18 +1535,29 @@ async def process_github_pr_review_request(payload: dict[str, Any]) -> None: logger.warning("Could not persist bot token for reviewer thread %s", thread_id) return - prompt = build_github_pr_review_prompt(repo_config, pr_number, pr_url, base_sha, head_sha) - configurable: dict[str, Any] = { - "source": "github", - "github_login": github_login, - "github_user_id": github_user_id, - "repo": repo_config, - "pr_number": pr_number, - "review_requested": True, + pr_meta: ReviewerPRMeta = { + "owner": repo_config.get("owner", ""), + "name": repo_config.get("name", ""), + "number": pr_number, + "url": pr_url, + "title": pr_title, + "head_ref": branch_name, + "base_ref": base_ref, } + await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True) - if branch_name: - configurable["branch_name"] = branch_name + prompt = build_github_pr_review_prompt(repo_config, pr_number, pr_url, base_sha, head_sha) + configurable = _build_reviewer_configurable( + source="github", + github_login=github_login, + github_user_id=github_user_id, + repo_config=repo_config, + pr_number=pr_number, + pr_url=pr_url, + base_sha=base_sha, + head_sha=head_sha, + branch_name=branch_name, + ) thread_active = await is_thread_active(thread_id) if thread_active: @@ -1509,6 +1576,192 @@ async def process_github_pr_review_request(payload: dict[str, Any]) -> None: logger.info("Reviewer run created for thread %s from GitHub PR review request", thread_id) +async def _fetch_open_pr_for_branch( + repo_config: dict[str, str], head_ref: str, *, token: str +) -> dict[str, Any] | None: + """Find the open PR whose head ref matches ``head_ref``, if one exists.""" + owner = repo_config.get("owner", "") + repo = repo_config.get("name", "") + headers = { + "Accept": "application/vnd.github+json", + "Authorization": f"Bearer {token}", + "X-GitHub-Api-Version": "2022-11-28", + } + params = {"state": "open", "head": f"{owner}:{head_ref}", "per_page": 1} + async with httpx.AsyncClient() as http_client: + try: + response = await http_client.get( + f"https://api.github.com/repos/{owner}/{repo}/pulls", + headers=headers, + params=params, + ) + response.raise_for_status() + except httpx.HTTPError: + logger.exception("Failed to look up open PR for %s/%s head=%s", owner, repo, head_ref) + return None + data = response.json() + if not isinstance(data, list) or not data: + return None + pr = data[0] + return pr if isinstance(pr, dict) else None + + +async def _get_thread_metadata_safe(thread_id: str) -> dict[str, Any] | None: + """Fetch a thread's metadata; return ``None`` if the thread doesn't exist.""" + langgraph_client = get_client(url=LANGGRAPH_URL) + try: + thread = await langgraph_client.threads.get(thread_id) + except Exception as exc: # noqa: BLE001 + if _is_not_found_error(exc): + return None + logger.warning("Failed to fetch reviewer thread metadata for %s", thread_id) + return None + metadata = thread.get("metadata") if isinstance(thread, dict) else None + return metadata if isinstance(metadata, dict) else {} + + +async def process_github_pr_close(payload: dict[str, Any]) -> None: + """Disable watch on the canonical reviewer thread when the PR closes/reopens.""" + repo = payload.get("repository", {}) + pull_request = payload.get("pull_request", {}) + repo_config = { + "owner": repo.get("owner", {}).get("login", ""), + "name": repo.get("name", ""), + } + pr_number = pull_request.get("number") + if not pr_number or not isinstance(pr_number, int): + return + if not _is_repo_allowed_for_reviewer(repo_config): + return + + thread_id = generate_reviewer_thread_id( + repo_config.get("owner", ""), repo_config.get("name", ""), pr_number + ) + metadata = await _get_thread_metadata_safe(thread_id) + if metadata is None or metadata.get("kind") != REVIEWER_THREAD_KIND: + # No reviewer thread for this PR, nothing to do. + return + action = payload.get("action", "") + desired_watch = action == "reopened" + if metadata.get("watch") == desired_watch: + return + await set_reviewer_thread_metadata(thread_id, watch=desired_watch) + logger.info("Set watch=%s on reviewer thread %s after PR %s", desired_watch, thread_id, action) + + +async def process_github_push_event(payload: dict[str, Any]) -> None: + """Re-trigger the reviewer for a watched PR when its head branch is pushed to.""" + ref = payload.get("ref", "") + after_sha = payload.get("after", "") + if not ref.startswith("refs/heads/"): + return + if not isinstance(after_sha, str) or not after_sha or set(after_sha) == {"0"}: + # Branch deletion or missing SHA — nothing to review. + return + head_ref = ref[len("refs/heads/") :] + + repo = payload.get("repository", {}) + repo_config = { + "owner": repo.get("owner", {}).get("login", "") or repo.get("owner", {}).get("name", ""), + "name": repo.get("name", ""), + } + if not repo_config["owner"] or not repo_config["name"]: + return + if not _is_repo_allowed_for_reviewer(repo_config): + return + + app_token = await get_github_app_installation_token() + if not app_token: + logger.warning("No GitHub App token for push re-review on %s", head_ref) + return + + pr = await _fetch_open_pr_for_branch(repo_config, head_ref, token=app_token) + if not pr: + logger.debug( + "No open PR found for push to %s/%s head=%s", + repo_config["owner"], + repo_config["name"], + head_ref, + ) + return + + pr_number = pr.get("number") + pr_url = pr.get("html_url") or pr.get("url") or "" + base_sha = pr.get("base", {}).get("sha", "") + base_ref = pr.get("base", {}).get("ref", "") + head_sha = pr.get("head", {}).get("sha", after_sha) + pr_title = pr.get("title", "") + if not isinstance(pr_number, int) or not base_sha or not head_sha: + return + + thread_id = generate_reviewer_thread_id(repo_config["owner"], repo_config["name"], pr_number) + metadata = await _get_thread_metadata_safe(thread_id) + if metadata is None or metadata.get("kind") != REVIEWER_THREAD_KIND: + return + if not metadata.get("watch"): + logger.info("Push to %s ignored: reviewer thread %s is not watching", head_ref, thread_id) + return + + last_reviewed_sha = metadata.get("last_reviewed_sha") + if isinstance(last_reviewed_sha, str) and last_reviewed_sha == head_sha: + logger.info("Push to %s ignored: head_sha unchanged from last_reviewed_sha", head_ref) + return + + langgraph_client = get_client(url=LANGGRAPH_URL) + if not await _ensure_thread_exists_for_metadata(thread_id, langgraph_client): + return + try: + await persist_encrypted_github_token(thread_id, app_token) + except Exception: + logger.warning("Could not persist bot token for reviewer thread %s", thread_id) + return + + pr_meta: ReviewerPRMeta = { + "owner": repo_config["owner"], + "name": repo_config["name"], + "number": pr_number, + "url": pr_url, + "title": pr_title, + "head_ref": head_ref, + "base_ref": base_ref, + } + await set_reviewer_thread_metadata(thread_id, pr=pr_meta, watch=True) + + re_review_prompt = ( + f"A new commit has been pushed to PR #{pr_number}. The new HEAD is " + f"{head_sha}. Reconcile existing findings against the new diff, add any " + f"net-new findings, and call `publish_review` once you're done." + ) + configurable = _build_reviewer_configurable( + source="github_push", + github_login=payload.get("sender", {}).get("login", "") or "", + github_user_id=payload.get("sender", {}).get("id"), + repo_config=repo_config, + pr_number=pr_number, + pr_url=pr_url, + base_sha=base_sha, + head_sha=head_sha, + branch_name=head_ref, + re_review=True, + last_reviewed_sha=last_reviewed_sha if isinstance(last_reviewed_sha, str) else "", + ) + + thread_active = await is_thread_active(thread_id) + if thread_active: + logger.info("Reviewer thread %s busy, queuing push re-review", thread_id) + await queue_message_for_thread(thread_id, re_review_prompt) + return + + logger.info("Creating push re-review run for thread %s", thread_id) + await langgraph_client.runs.create( + thread_id, + "reviewer", + input={"messages": [{"role": "user", "content": re_review_prompt}]}, + config={"configurable": configurable, "metadata": _AGENT_VERSION_METADATA}, + if_not_exists="create", + ) + + async def _get_or_resolve_thread_github_token(thread_id: str, email: str) -> str | None: """Resolve and persist a GitHub token for a thread when available. @@ -1797,6 +2050,12 @@ async def github_webhook(request: Request, background_tasks: BackgroundTasks) -> "status": "ignored", "reason": f"Unsupported GitHub pull_request action: {action}", } + if action in {"closed", "reopened"}: + if not _is_repo_allowed_for_reviewer(webhook_repo_config): + return {"status": "ignored", "reason": "Repository not in reviewer allowlist"} + logger.info("Accepted GitHub PR %s webhook, scheduling reviewer watch update", action) + background_tasks.add_task(process_github_pr_close, payload) + return {"status": "accepted", "message": f"Processing PR {action} for reviewer watch"} if not _is_open_swe_reviewer_request(payload): logger.info("Ignoring PR review request for a different reviewer") return {"status": "ignored", "reason": "Review request is not for open-swe bot"} @@ -1816,6 +2075,13 @@ async def github_webhook(request: Request, background_tasks: BackgroundTasks) -> background_tasks.add_task(process_github_pr_review_request, payload) return {"status": "accepted", "message": "Processing GitHub PR review request"} + if event_type == "push": + if not _is_repo_allowed_for_reviewer(webhook_repo_config): + return {"status": "ignored", "reason": "Repository not in reviewer allowlist"} + logger.info("Accepted GitHub push webhook, scheduling reviewer watch evaluation") + background_tasks.add_task(process_github_push_event, payload) + return {"status": "accepted", "message": "Processing GitHub push for reviewer watch"} + if not _is_repo_org_allowed(webhook_repo_config): logger.warning( "Rejecting GitHub webhook: org '%s' not in ALLOWED_GITHUB_ORGS", diff --git a/evals/reviewer/target.py b/evals/reviewer/target.py index e23615ab..cd8afbb6 100644 --- a/evals/reviewer/target.py +++ b/evals/reviewer/target.py @@ -1,8 +1,10 @@ """Target function for the reviewer eval. Spawns the reviewer graph over `langgraph_sdk` for one PR, waits for -completion, and returns every `github_comment` tool call the agent made as -the structured output for the eval. +completion, and returns every `add_finding` tool call the agent made as the +structured output for the eval. Findings are normalized into the legacy +``{file, line, body, severity}`` shape so the judge prompt can stay the +verbatim form martian published. """ from __future__ import annotations @@ -48,11 +50,25 @@ def _build_user_message(inputs: dict[str, Any]) -> str: f"- head_sha: {inputs['head_sha']}\n" f"- base_ref: {inputs.get('base_ref', '')}\n" f"- head_ref: {inputs.get('head_ref', '')}\n\n" - f"Clone the repo, check out the base SHA, fetch the PR head, and review " - f"the diff. Record each issue you find with the `github_comment` tool." + f"Record each issue you find with the `add_finding` tool, then call " + f"`publish_review` once at the end." ) +def _build_configurable(inputs: dict[str, Any]) -> dict[str, Any]: + repo = inputs.get("repo", "") + owner, _, name = repo.partition("/") if isinstance(repo, str) else ("", "", "") + return { + "__is_for_execution__": True, + "repo": {"owner": owner, "name": name}, + "pr_number": inputs.get("pr_number"), + "pr_url": inputs.get("pr_url", ""), + "base_sha": inputs.get("base_sha", ""), + "head_sha": inputs.get("head_sha", ""), + "branch_name": inputs.get("head_ref", ""), + } + + async def review_pr(inputs: dict[str, Any]) -> dict[str, Any]: """LangSmith target: run the reviewer agent on one PR.""" client = get_client(url=LANGGRAPH_URL) @@ -63,13 +79,18 @@ async def review_pr(inputs: dict[str, Any]) -> dict[str, Any]: thread_id, assistant_id=REVIEWER_ASSISTANT_ID, input={"messages": [{"role": "user", "content": _build_user_message(inputs)}]}, - config={"configurable": {"__is_for_execution__": True}}, + config={"configurable": _build_configurable(inputs)}, ) return {"comments": _extract_comments(result)} def _extract_comments(result: Any) -> list[dict[str, Any]]: - """Collect every `github_comment` tool call from the run's message stream.""" + """Collect every ``add_finding`` tool call from the run's message stream. + + Normalizes the new finding shape (``start_line``/``end_line``/``description``) + into the legacy ``{file, line, body, severity}`` shape the judge prompt + consumes verbatim from martian's benchmark. + """ if not isinstance(result, dict): return [] comments: list[dict[str, Any]] = [] @@ -77,16 +98,23 @@ def _extract_comments(result: Any) -> list[dict[str, Any]]: if not isinstance(msg, dict): continue for tc in msg.get("tool_calls") or []: - if tc.get("name") != "github_comment": + if tc.get("name") != "add_finding": continue args = tc.get("args") or {} - if {"file", "line", "body", "severity"} <= args.keys(): - comments.append( - { - "file": args["file"], - "line": args["line"], - "body": args["body"], - "severity": args["severity"], - } - ) + file = args.get("file") + severity = args.get("severity") + description = args.get("description") or args.get("body") or "" + line = args.get("end_line") + if line is None: + line = args.get("start_line") + if not file or not severity: + continue + comments.append( + { + "file": file, + "line": line, + "body": description, + "severity": severity, + } + ) return comments diff --git a/tests/test_github_issue_webhook.py b/tests/test_github_issue_webhook.py index af133e08..6d5f8cc9 100644 --- a/tests/test_github_issue_webhook.py +++ b/tests/test_github_issue_webhook.py @@ -663,6 +663,9 @@ def test_process_github_pr_review_request_creates_reviewer_run(monkeypatch) -> N runs = _FakeRunsClient() threads = _FakeThreadsClient() + async def fake_set_reviewer_thread_metadata(thread_id: str, **_kwargs: object) -> None: + captured["set_metadata_thread_id"] = thread_id + monkeypatch.setattr( webapp, "get_github_app_installation_token", fake_get_github_app_installation_token ) @@ -670,6 +673,7 @@ def test_process_github_pr_review_request_creates_reviewer_run(monkeypatch) -> N webapp, "persist_encrypted_github_token", fake_persist_encrypted_github_token ) monkeypatch.setattr(webapp, "is_thread_active", fake_is_thread_active) + monkeypatch.setattr(webapp, "set_reviewer_thread_metadata", fake_set_reviewer_thread_metadata) monkeypatch.setattr(webapp, "get_client", lambda url: _FakeLangGraphClient()) asyncio.run( @@ -680,7 +684,7 @@ def test_process_github_pr_review_request_creates_reviewer_run(monkeypatch) -> N "pull_request": { "number": 1244, "html_url": "https://github.com/langchain-ai/open-swe/pull/1244", - "base": {"sha": "base-sha"}, + "base": {"sha": "base-sha", "ref": "main"}, "head": {"sha": "head-sha", "ref": "feature-branch"}, }, "repository": {"owner": {"login": "langchain-ai"}, "name": "open-swe"}, @@ -748,6 +752,9 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: runs = _FakeRunsClient() threads = _FakeThreadsClient() + async def fake_set_reviewer_thread_metadata(thread_id: str, **_kwargs: object) -> None: + captured["set_metadata_thread_id"] = thread_id + monkeypatch.setattr( webapp, "get_github_app_installation_token", fake_get_github_app_installation_token ) @@ -756,6 +763,7 @@ def test_trigger_pr_review_from_ref_creates_reviewer_run(monkeypatch) -> None: webapp, "persist_encrypted_github_token", fake_persist_encrypted_github_token ) monkeypatch.setattr(webapp, "is_thread_active", fake_is_thread_active) + monkeypatch.setattr(webapp, "set_reviewer_thread_metadata", fake_set_reviewer_thread_metadata) monkeypatch.setattr(webapp, "get_client", lambda url: _FakeLangGraphClient()) monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_REPOS", frozenset()) monkeypatch.setattr(webapp, "ALLOWED_REVIEWER_GITHUB_ORGS", frozenset()) diff --git a/tests/test_reviewer_diff.py b/tests/test_reviewer_diff.py new file mode 100644 index 00000000..81ba838b --- /dev/null +++ b/tests/test_reviewer_diff.py @@ -0,0 +1,112 @@ +"""Unit tests for the unified-diff parsing helpers.""" + +from __future__ import annotations + +import pytest + +from agent.reviewer_diff import ( + compute_diff_line_set, + extract_diff_hunk, + is_range_in_diff, + parse_unified_diff, +) + +_TWO_FILE_DIFF = """diff --git a/foo.py b/foo.py +index 1111111..2222222 100644 +--- a/foo.py ++++ b/foo.py +@@ -10,3 +10,4 @@ def existing(): + pass ++ new_line_13 = 1 ++ new_line_14 = 2 + return 1 +diff --git a/bar.py b/bar.py +index 3333333..4444444 100644 +--- a/bar.py ++++ b/bar.py +@@ -1,2 +1,3 @@ + import os ++import sys + print(os.getcwd()) +@@ -50,3 +51,4 @@ def other(): + line_a = 1 ++ line_b = 2 + line_c = 3 +""" + + +def test_parse_unified_diff_extracts_hunks_per_file() -> None: + files = parse_unified_diff(_TWO_FILE_DIFF) + assert [fd.file for fd in files] == ["foo.py", "bar.py"] + assert len(files[0].hunks) == 1 + assert len(files[1].hunks) == 2 + + +def test_compute_diff_line_set_covers_each_hunks_new_lines() -> None: + line_set = compute_diff_line_set(_TWO_FILE_DIFF) + assert line_set["foo.py"] == {10, 11, 12, 13} + assert line_set["bar.py"] == {1, 2, 3, 51, 52, 53, 54} + + +def test_is_range_in_diff_for_inline_and_file_level() -> None: + line_set = compute_diff_line_set(_TWO_FILE_DIFF) + assert is_range_in_diff(line_set, "foo.py", 11, 12) is True + assert is_range_in_diff(line_set, "foo.py", 11, 99) is False + assert is_range_in_diff(line_set, "missing.py", 1, 1) is False + assert is_range_in_diff(line_set, "foo.py", None, None) is True + + +def test_extract_diff_hunk_returns_overlapping_hunk_body() -> None: + hunk = extract_diff_hunk(_TWO_FILE_DIFF, "bar.py", 51, 52) + assert hunk is not None + assert "@@ -50,3 +51,4 @@" in hunk + assert "line_b" in hunk + + +def test_extract_diff_hunk_returns_none_for_unknown_file() -> None: + assert extract_diff_hunk(_TWO_FILE_DIFF, "unknown.py", 1, 1) is None + + +@pytest.mark.parametrize( + ("start", "end"), + [(1, 1), (1, 3)], +) +def test_extract_diff_hunk_supports_single_line_and_range(start: int, end: int) -> None: + hunk = extract_diff_hunk(_TWO_FILE_DIFF, "bar.py", start, end) + assert hunk is not None + assert "import sys" in hunk + + +@pytest.mark.asyncio +async def test_compute_diff_in_sandbox_uses_three_dot_for_merge_base() -> None: + """First-review path passes merge_base=True so we use base...head, not base..head.""" + from unittest.mock import MagicMock + + from agent.reviewer_diff import compute_diff_in_sandbox + + backend = MagicMock() + backend.execute = MagicMock(return_value="") + + await compute_diff_in_sandbox( + backend, work_dir="/w", base_ref="base", head_ref="head", merge_base=True + ) + cmd = backend.execute.call_args.args[0] + assert "base...head" in cmd + assert "base..head" not in cmd.replace("base...head", "") + assert "--no-prefix" not in cmd # invalid flag must not appear + + +@pytest.mark.asyncio +async def test_compute_diff_in_sandbox_uses_two_dot_by_default() -> None: + """Re-review delta path passes merge_base=False so we use base..head.""" + from unittest.mock import MagicMock + + from agent.reviewer_diff import compute_diff_in_sandbox + + backend = MagicMock() + backend.execute = MagicMock(return_value="") + + await compute_diff_in_sandbox(backend, work_dir="/w", base_ref="oldsha", head_ref="newsha") + cmd = backend.execute.call_args.args[0] + assert "oldsha..newsha" in cmd + assert "oldsha...newsha" not in cmd diff --git a/tests/test_reviewer_findings.py b/tests/test_reviewer_findings.py new file mode 100644 index 00000000..829d3c03 --- /dev/null +++ b/tests/test_reviewer_findings.py @@ -0,0 +1,175 @@ +"""Unit tests for the Finding schema + thread-metadata helpers.""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, patch + +import pytest + +from agent.reviewer_findings import ( + SEVERITY_ORDER, + Finding, + append_finding, + filter_findings_for_publish, + list_findings, + new_finding, + new_finding_id, + replace_findings, + set_reviewer_thread_metadata, + update_finding_fields, +) + + +def _f(**overrides: Any) -> Finding: + base = new_finding( + severity="high", + category="correctness", + file="foo.py", + start_line=10, + end_line=10, + description="boom", + sha="abc123", + ) + base.update(overrides) # type: ignore[arg-type] + return base + + +def test_new_finding_id_format() -> None: + fid = new_finding_id() + assert fid.startswith("f_") + assert len(fid) == len("f_") + 10 + + +def test_new_finding_defaults() -> None: + finding = _f() + assert finding["status"] == "open" + assert finding["side"] == "RIGHT" + assert finding["first_seen_sha"] == "abc123" + assert finding["last_confirmed_sha"] == "abc123" + assert finding["github_review_comment_id"] is None + assert finding["suggestion"] is None + + +def test_severity_order_monotonic() -> None: + assert ( + SEVERITY_ORDER["informational"] + < SEVERITY_ORDER["low"] + < SEVERITY_ORDER["medium"] + < SEVERITY_ORDER["high"] + < SEVERITY_ORDER["critical"] + ) + + +def test_filter_findings_for_publish_drops_below_threshold_and_resolved() -> None: + findings = [ + _f(id="f_a", severity="high", file="a.py", start_line=1, end_line=1), + _f(id="f_b", severity="low", file="b.py"), + _f(id="f_c", severity="critical", file="c.py", start_line=2, end_line=2), + _f(id="f_d", severity="high", file="d.py", status="resolved"), + _f(id="f_e", severity="informational", file="e.py"), + ] + surfaced = filter_findings_for_publish(findings, severity_threshold="medium", cap=10) + assert [f["id"] for f in surfaced] == ["f_c", "f_a"] + + +def test_filter_findings_for_publish_caps_results() -> None: + findings = [_f(id=f"f_{i}", severity="high", file=f"f{i}.py") for i in range(20)] + surfaced = filter_findings_for_publish(findings, severity_threshold="medium", cap=5) + assert len(surfaced) == 5 + + +@pytest.mark.asyncio +async def test_list_findings_returns_empty_on_missing_metadata() -> None: + fake_client = AsyncMock() + fake_client.threads.get.return_value = {"metadata": {}} + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + findings = await list_findings("tid") + assert findings == [] + + +@pytest.mark.asyncio +async def test_list_findings_coerces_bad_entries() -> None: + fake_client = AsyncMock() + fake_client.threads.get.return_value = { + "metadata": { + "findings": [ + {"id": "f_ok", "severity": "high", "file": "x.py"}, + {"missing_id": True}, + "not-a-dict", + ] + } + } + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + findings = await list_findings("tid") + assert [f["id"] for f in findings] == ["f_ok"] + + +@pytest.mark.asyncio +async def test_replace_findings_calls_threads_update() -> None: + fake_client = AsyncMock() + findings = [_f(id="f_x")] + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + await replace_findings("tid", findings) + fake_client.threads.update.assert_awaited_once_with( + thread_id="tid", metadata={"findings": findings} + ) + + +@pytest.mark.asyncio +async def test_append_finding_appends_to_existing_list() -> None: + existing = _f(id="f_a") + new = _f(id="f_b") + + fake_client = AsyncMock() + fake_client.threads.get.return_value = {"metadata": {"findings": [existing]}} + + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + result = await append_finding("tid", new) + + assert result["id"] == "f_b" + args = fake_client.threads.update.await_args + persisted = args.kwargs["metadata"]["findings"] + assert [f["id"] for f in persisted] == ["f_a", "f_b"] + + +@pytest.mark.asyncio +async def test_update_finding_fields_mutates_only_target() -> None: + a = _f(id="f_a", description="orig-a") + b = _f(id="f_b", description="orig-b") + + fake_client = AsyncMock() + fake_client.threads.get.return_value = {"metadata": {"findings": [a, b]}} + + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + updated = await update_finding_fields("tid", "f_b", {"status": "resolved"}) + + assert updated is not None + assert updated["status"] == "resolved" + persisted = fake_client.threads.update.await_args.kwargs["metadata"]["findings"] + by_id = {f["id"]: f for f in persisted} + assert by_id["f_a"]["status"] == "open" + assert by_id["f_b"]["status"] == "resolved" + + +@pytest.mark.asyncio +async def test_update_finding_fields_returns_none_for_unknown_id() -> None: + fake_client = AsyncMock() + fake_client.threads.get.return_value = {"metadata": {"findings": [_f(id="f_a")]}} + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + result = await update_finding_fields("tid", "f_missing", {"status": "resolved"}) + assert result is None + fake_client.threads.update.assert_not_called() + + +@pytest.mark.asyncio +async def test_set_reviewer_thread_metadata_includes_kind() -> None: + fake_client = AsyncMock() + with patch("agent.reviewer_findings.get_client", return_value=fake_client): + await set_reviewer_thread_metadata("tid", watch=True, last_reviewed_sha="sha") + metadata = fake_client.threads.update.await_args.kwargs["metadata"] + assert metadata["kind"] == "reviewer" + assert metadata["watch"] is True + assert metadata["last_reviewed_sha"] == "sha" + assert "pr" not in metadata + assert "findings" not in metadata diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py new file mode 100644 index 00000000..7b3d5683 --- /dev/null +++ b/tests/test_reviewer_publish.py @@ -0,0 +1,168 @@ +"""Unit tests for the publish_review rendering and orchestration helpers.""" + +from __future__ import annotations + +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 ( + render_inline_comment_body, + render_inline_comment_payload, + render_review_body, + resolve_review_thread, +) + + +def _f(**overrides: Any) -> Finding: + base = new_finding( + severity="high", + 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 + + +def test_render_inline_comment_body_without_suggestion() -> None: + body = render_inline_comment_body(_f(description="just text")) + assert body == "just text" + + +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_payload_single_line() -> None: + payload = render_inline_comment_payload(_f(start_line=10, end_line=10)) + assert payload == { + "path": "src/foo.py", + "line": 10, + "side": "RIGHT", + "body": "boom", + } + + +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_includes_summary_and_marker() -> None: + body = render_review_body( + pr_number=123, + surfaced_count=2, + total_open_count=3, + severity_threshold="medium", + summary="LGTM with two notes", + ) + assert "LGTM with two notes" in body + assert "" in body + assert "1 lower-severity finding hidden" in body + + +def test_render_review_body_surfaces_no_findings_message() -> None: + body = render_review_body( + pr_number=99, + surfaced_count=0, + total_open_count=0, + severity_threshold="medium", + summary=None, + ) + assert "No issues at or above" in body + + +@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) + + with patch("agent.reviewer_publish.httpx.AsyncClient", return_value=client_cm): + ok = await resolve_review_thread(thread_node_id="T_1", token="t") + assert ok is True + + +@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) + + with patch("agent.reviewer_publish.httpx.AsyncClient", return_value=client_cm): + 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), + ): + result = await _publish_review_async( + owner="o", + repo="r", + pr_number=7, + head_sha="sha", + token="t", + summary=None, + severity_threshold="medium", + cap=15, + ) + + 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"} diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py new file mode 100644 index 00000000..996620a3 --- /dev/null +++ b/tests/test_reviewer_tools.py @@ -0,0 +1,180 @@ +"""Unit tests for the add_finding / update_finding / list_findings tools.""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, patch + +from agent.tools.add_finding import add_finding +from agent.tools.list_findings import list_findings +from agent.tools.update_finding import update_finding + + +def _config(**configurable_overrides: Any) -> dict[str, Any]: + base: dict[str, Any] = { + "configurable": { + "thread_id": "tid-1", + "head_sha": "sha-head", + "diff_text": "", + "diff_line_set": {"foo.py": [10, 11, 12]}, + }, + "metadata": {}, + } + base["configurable"].update(configurable_overrides) + return base + + +def test_add_finding_rejects_invalid_severity() -> None: + with patch("agent.tools.add_finding.get_config", return_value=_config()): + result = add_finding( + severity="trivial", + category="x", + file="foo.py", + description="d", + start_line=11, + end_line=11, + ) + assert result["success"] is False + assert "severity" in result["error"].lower() + + +def test_add_finding_rejects_out_of_diff_lines() -> None: + with patch("agent.tools.add_finding.get_config", return_value=_config()): + result = add_finding( + severity="high", + category="correctness", + file="foo.py", + description="d", + start_line=99, + end_line=99, + ) + assert result["success"] is False + assert "not part of the PR diff" in result["error"] + + +def test_add_finding_persists_to_thread_metadata() -> None: + captured: list[Any] = [] + + async def fake_append(thread_id: str, finding: Any) -> Any: + captured.append((thread_id, finding)) + return finding + + with ( + patch("agent.tools.add_finding.get_config", return_value=_config()), + patch("agent.tools.add_finding.get_thread_id_from_runtime", return_value="tid-1"), + patch("agent.tools.add_finding.append_finding", side_effect=fake_append), + ): + result = add_finding( + severity="medium", + category="style", + file="foo.py", + description="rename", + start_line=11, + end_line=12, + suggestion="renamed = 1", + ) + + assert result["success"] is True + assert "finding_id" in result + persisted_thread, persisted = captured[0] + assert persisted_thread == "tid-1" + assert persisted["file"] == "foo.py" + assert persisted["start_line"] == 11 + assert persisted["end_line"] == 12 + assert persisted["suggestion"] == "renamed = 1" + assert persisted["status"] == "open" + assert persisted["first_seen_sha"] == "sha-head" + + +def test_add_finding_allows_file_level_with_no_lines() -> None: + with ( + patch("agent.tools.add_finding.get_config", return_value=_config()), + patch("agent.tools.add_finding.get_thread_id_from_runtime", return_value="tid-1"), + patch( + "agent.tools.add_finding.append_finding", + new_callable=AsyncMock, + side_effect=lambda _t, f: f, + ), + ): + result = add_finding( + severity="low", + category="style", + file="missing.py", + description="file-level note", + ) + assert result["success"] is True + + +def test_update_finding_rejects_invalid_status() -> None: + with patch("agent.tools.update_finding.get_config", return_value=_config()): + result = update_finding(finding_id="f_x", status="archived") + assert result["success"] is False + + +def test_update_finding_rejects_empty_update() -> None: + with patch("agent.tools.update_finding.get_config", return_value=_config()): + result = update_finding(finding_id="f_x") + assert result["success"] is False + assert "No fields" in result["error"] + + +def test_update_finding_passes_through_fields() -> None: + captured: list[Any] = [] + + async def fake_update(thread_id: str, finding_id: str, updates: Any) -> Any: + captured.append((thread_id, finding_id, updates)) + return {"id": finding_id, **updates} + + with ( + patch("agent.tools.update_finding.get_config", return_value=_config()), + patch("agent.tools.update_finding.get_thread_id_from_runtime", return_value="tid-1"), + patch("agent.tools.update_finding.update_finding_fields", side_effect=fake_update), + ): + result = update_finding( + finding_id="f_a", + status="resolved", + note="addressed by new commit", + ) + + assert result["success"] is True + _t, fid, updates = captured[0] + assert fid == "f_a" + assert updates["status"] == "resolved" + assert updates["last_update_note"] == "addressed by new commit" + + +def test_list_findings_filters_by_status() -> None: + findings = [ + {"id": "f_a", "status": "open"}, + {"id": "f_b", "status": "resolved"}, + {"id": "f_c", "status": "open"}, + ] + + async def fake_list(_thread_id: str) -> list[Any]: + return findings + + cfg = _config() + with ( + patch("agent.tools.list_findings.get_thread_id_from_runtime", return_value="tid-1"), + patch("agent.tools.list_findings.list_findings_async", side_effect=fake_list), + patch("agent.tools.add_finding.get_config", return_value=cfg), + ): + result = list_findings(status_filter="open") + + assert result["count"] == 2 + assert [f["id"] for f in result["findings"]] == ["f_a", "f_c"] + + +def test_list_findings_returns_all_when_filter_omitted() -> None: + findings = [{"id": "f_a", "status": "open"}, {"id": "f_b", "status": "resolved"}] + + async def fake_list(_thread_id: str) -> list[Any]: + return findings + + with ( + patch("agent.tools.list_findings.get_thread_id_from_runtime", return_value="tid-1"), + patch("agent.tools.list_findings.list_findings_async", side_effect=fake_list), + ): + result = list_findings() + + assert result["count"] == 2 diff --git a/tests/test_reviewer_watch.py b/tests/test_reviewer_watch.py new file mode 100644 index 00000000..50aff82e --- /dev/null +++ b/tests/test_reviewer_watch.py @@ -0,0 +1,232 @@ +"""Unit tests for the watch-mode webhook handlers.""" + +from __future__ import annotations + +from typing import Any +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + +from agent import webapp + + +def _push_payload(*, ref: str, after: str, owner: str = "lc", name: str = "repo") -> dict[str, Any]: + return { + "ref": ref, + "after": after, + "repository": {"owner": {"login": owner}, "name": name}, + "sender": {"login": "alice", "id": 7}, + } + + +def _pr_close_payload(*, action: str, number: int = 7) -> dict[str, Any]: + return { + "action": action, + "repository": {"owner": {"login": "lc"}, "name": "repo"}, + "pull_request": {"number": number, "head": {"ref": "feat-x"}}, + } + + +@pytest.mark.asyncio +async def test_push_event_skips_branch_deletion() -> None: + payload = _push_payload( + ref="refs/heads/feat-x", after="0000000000000000000000000000000000000000" + ) + with patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True): + await webapp.process_github_push_event(payload) + # If we got here without crashing and with no other patches needed, the + # function returned early on the deletion check. + + +@pytest.mark.asyncio +async def test_push_event_skips_when_thread_not_watching() -> None: + payload = _push_payload(ref="refs/heads/feat-x", after="newsha") + pr = { + "number": 7, + "html_url": "https://github.com/lc/repo/pull/7", + "title": "T", + "head": {"sha": "newsha", "ref": "feat-x"}, + "base": {"sha": "basesha", "ref": "main"}, + } + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + + with ( + patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp.get_github_app_installation_token", + new_callable=AsyncMock, + return_value="t", + ), + patch( + "agent.webapp._fetch_open_pr_for_branch", + new_callable=AsyncMock, + return_value=pr, + ), + patch( + "agent.webapp._get_thread_metadata_safe", + new_callable=AsyncMock, + return_value={"kind": "reviewer", "watch": False}, + ), + patch("agent.webapp.get_client", return_value=fake_client), + ): + await webapp.process_github_push_event(payload) + fake_client.runs.create.assert_not_called() + + +@pytest.mark.asyncio +async def test_push_event_triggers_re_review_run_when_watching() -> None: + payload = _push_payload(ref="refs/heads/feat-x", after="newsha") + pr = { + "number": 7, + "html_url": "https://github.com/lc/repo/pull/7", + "title": "T", + "head": {"sha": "newsha", "ref": "feat-x"}, + "base": {"sha": "basesha", "ref": "main"}, + } + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + + with ( + patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp.get_github_app_installation_token", + new_callable=AsyncMock, + return_value="t", + ), + patch( + "agent.webapp._fetch_open_pr_for_branch", + new_callable=AsyncMock, + return_value=pr, + ), + patch( + "agent.webapp._get_thread_metadata_safe", + new_callable=AsyncMock, + return_value={ + "kind": "reviewer", + "watch": True, + "last_reviewed_sha": "oldsha", + }, + ), + patch( + "agent.webapp._ensure_thread_exists_for_metadata", + new_callable=AsyncMock, + return_value=True, + ), + patch( + "agent.webapp.persist_encrypted_github_token", + new_callable=AsyncMock, + return_value="enc", + ), + patch( + "agent.webapp.set_reviewer_thread_metadata", + new_callable=AsyncMock, + ), + patch("agent.webapp.is_thread_active", new_callable=AsyncMock, return_value=False), + patch("agent.webapp.get_client", return_value=fake_client), + ): + await webapp.process_github_push_event(payload) + + fake_client.runs.create.assert_awaited_once() + args, kwargs = fake_client.runs.create.await_args + assert args[1] == "reviewer" + configurable = kwargs["config"]["configurable"] + assert configurable["re_review"] is True + assert configurable["last_reviewed_sha"] == "oldsha" + assert configurable["head_sha"] == "newsha" + + +@pytest.mark.asyncio +async def test_push_event_idempotent_when_head_unchanged() -> None: + payload = _push_payload(ref="refs/heads/feat-x", after="samesha") + pr = { + "number": 7, + "html_url": "https://github.com/lc/repo/pull/7", + "title": "T", + "head": {"sha": "samesha", "ref": "feat-x"}, + "base": {"sha": "basesha", "ref": "main"}, + } + fake_client = MagicMock() + fake_client.runs.create = AsyncMock() + + with ( + patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp.get_github_app_installation_token", + new_callable=AsyncMock, + return_value="t", + ), + patch( + "agent.webapp._fetch_open_pr_for_branch", + new_callable=AsyncMock, + return_value=pr, + ), + patch( + "agent.webapp._get_thread_metadata_safe", + new_callable=AsyncMock, + return_value={ + "kind": "reviewer", + "watch": True, + "last_reviewed_sha": "samesha", + }, + ), + patch("agent.webapp.get_client", return_value=fake_client), + ): + await webapp.process_github_push_event(payload) + fake_client.runs.create.assert_not_called() + + +@pytest.mark.asyncio +async def test_pr_close_disables_watch() -> None: + captured: list[Any] = [] + + async def fake_set(thread_id: str, **kwargs: Any) -> None: + captured.append((thread_id, kwargs)) + + with ( + patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._get_thread_metadata_safe", + new_callable=AsyncMock, + return_value={"kind": "reviewer", "watch": True}, + ), + patch("agent.webapp.set_reviewer_thread_metadata", side_effect=fake_set), + ): + await webapp.process_github_pr_close(_pr_close_payload(action="closed")) + assert captured and captured[0][1]["watch"] is False + + +@pytest.mark.asyncio +async def test_pr_reopened_re_enables_watch() -> None: + captured: list[Any] = [] + + async def fake_set(thread_id: str, **kwargs: Any) -> None: + captured.append((thread_id, kwargs)) + + with ( + patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._get_thread_metadata_safe", + new_callable=AsyncMock, + return_value={"kind": "reviewer", "watch": False}, + ), + patch("agent.webapp.set_reviewer_thread_metadata", side_effect=fake_set), + ): + await webapp.process_github_pr_close(_pr_close_payload(action="reopened")) + assert captured and captured[0][1]["watch"] is True + + +@pytest.mark.asyncio +async def test_pr_close_skips_non_reviewer_threads() -> None: + fake_set = AsyncMock() + with ( + patch("agent.webapp._is_repo_allowed_for_reviewer", return_value=True), + patch( + "agent.webapp._get_thread_metadata_safe", + new_callable=AsyncMock, + return_value={"kind": "agent"}, + ), + patch("agent.webapp.set_reviewer_thread_metadata", new=fake_set), + ): + await webapp.process_github_pr_close(_pr_close_payload(action="closed")) + fake_set.assert_not_called()