mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-09-30 12:43:16 +00:00
* reviewer: fetch PR diff via GitHub API to re-enable add_finding validation The previous hotfix in reviewer.py set diff_line_set=None because the sandbox-based diff prep was sometimes producing empty diffs. That made every bad anchor a publish-time 422 instead of a creation-time rejection — the agent burned tokens producing unanchorable findings, and we had to add a publish-time retry safety net (#1338) to clean up. Fetch the PR's unified diff via the GitHub REST API at reviewer startup and populate diff_text + diff_line_set so add_finding can reject bad anchors immediately. The API path is reliable and is the same diff GitHub validates against when posting inline review comments. If the fetch fails, fall back to the previous behavior (validation disabled, publish-time retry handles it). Also extract the PR-diff fetch into reviewer_diff.fetch_pr_diff so both reviewer.py and publish_review.py share one implementation instead of two copies. * reviewer: make diff_line_set validation side-aware compute_diff_line_set previously returned only new-side line numbers, so re-enabling add_finding's validation would wrongly reject findings with side=LEFT (deleted-line bugs whose only anchor is an old-side line). Return {file: {"RIGHT": {new_lines}, "LEFT": {old_lines}}} instead, and have is_range_in_diff select the matching side from the finding's recorded side. add_finding and publish_review's retry filter both pass the finding's side through.
162 lines
6.6 KiB
Python
162 lines
6.6 KiB
Python
"""Tool: ``add_finding``. Records one review finding on the reviewer thread."""
|
|
|
|
from __future__ import annotations
|
|
|
|
import asyncio
|
|
from typing import Any
|
|
|
|
from langgraph.config import get_config
|
|
|
|
from ..reviewer_diff import is_range_in_diff
|
|
from ..reviewer_findings import (
|
|
MAX_SUGGESTION_LINES,
|
|
Confidence,
|
|
DiffSide,
|
|
Finding,
|
|
Severity,
|
|
append_finding,
|
|
clip_suggestion,
|
|
get_thread_id_from_runtime,
|
|
new_finding,
|
|
)
|
|
|
|
|
|
def add_finding(
|
|
severity: str,
|
|
confidence: 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 ``low``, ``medium``, ``high``, ``critical``.
|
|
confidence: One of ``low``, ``medium``, ``high``.
|
|
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 line in the new (post-PR) file where the
|
|
relevant range begins. For a single-line finding, this is the
|
|
line the issue is about. For a multi-line finding, this is the
|
|
first line of the relevant range. Omit (with ``end_line``) for
|
|
file-level findings.
|
|
end_line: 1-based line where the relevant range ends. GitHub
|
|
anchors the inline comment at ``end_line`` and renders the
|
|
``start_line..end_line`` span as the highlighted snippet, so
|
|
choose ``end_line`` as the *last* line that matters — typically
|
|
the line the comment is most directly about. For a single-line
|
|
finding, set ``end_line == start_line`` (or omit it). Prefer
|
|
the natural range of the issue over a single line: GitHub
|
|
shows context above ``end_line``, so a one-line anchor often
|
|
buries the issue under unrelated context. Defaults to
|
|
``start_line`` when omitted.
|
|
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". **Only set this for small,
|
|
obvious fixes that fit in 4 lines or fewer** (e.g. a one-liner
|
|
rename, a missing guard, a typo). Longer suggestions are dropped
|
|
because they read as rewrites rather than reviews — leave those
|
|
cases as a description-only finding so the author can decide how
|
|
to fix it.
|
|
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 {"low", "medium", "high", "critical"}:
|
|
return {"success": False, "error": f"Invalid severity: {severity}"}
|
|
if confidence not in {"low", "medium", "high"}:
|
|
return {"success": False, "error": f"Invalid confidence: {confidence}"}
|
|
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, side=_cast_side(side)
|
|
):
|
|
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)
|
|
|
|
clipped_suggestion, suggestion_dropped = clip_suggestion(suggestion)
|
|
|
|
finding: Finding = new_finding(
|
|
severity=_cast_severity(severity),
|
|
confidence=_cast_confidence(confidence),
|
|
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=clipped_suggestion,
|
|
diff_hunk=diff_hunk,
|
|
)
|
|
|
|
thread_id = get_thread_id_from_runtime()
|
|
asyncio.run(append_finding(thread_id, finding))
|
|
result: dict[str, Any] = {"success": True, "finding_id": finding["id"]}
|
|
if suggestion_dropped:
|
|
result["suggestion_dropped"] = True
|
|
result["warning"] = (
|
|
f"Suggestion exceeded the {MAX_SUGGESTION_LINES}-line cap and was "
|
|
"dropped — the finding was recorded with description only. Only "
|
|
"include `suggestion` for small, obvious fixes."
|
|
)
|
|
return result
|
|
|
|
|
|
def _cast_severity(value: str) -> Severity:
|
|
return value # type: ignore[return-value]
|
|
|
|
|
|
def _cast_confidence(value: str) -> Confidence:
|
|
return value # type: ignore[return-value]
|
|
|
|
|
|
def _cast_side(value: str) -> DiffSide:
|
|
return value # type: ignore[return-value]
|