mirror of
https://github.com/Sea-Haven-Industries/open-swe.git
synced 2026-10-02 07:23:15 +00:00
feat: generate reviewer finding titles (#1355)
Co-authored-by: open-swe[bot] <open-swe@users.noreply.github.com> Co-authored-by: Johannes du Plessis <51395795+johannes117@users.noreply.github.com>
This commit is contained in:
parent
a2a1692493
commit
9998119921
8 changed files with 148 additions and 26 deletions
|
|
@ -63,11 +63,12 @@ Reviewer-thread metadata schema:
|
|||
|
||||
Tools the reviewer agent uses to mutate findings:
|
||||
|
||||
- `add_finding(severity, category, file, start_line, end_line, description, suggestion=None, side="RIGHT") -> id`
|
||||
- `add_finding(severity, category, file, title, description, start_line, end_line, suggestion=None, side="RIGHT") -> id`
|
||||
- `title` is a concise generated headline for the GitHub comment, not a copied/truncated description.
|
||||
- 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.
|
||||
- `update_finding(id, *, status?, severity?, title?, 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.
|
||||
|
|
|
|||
|
|
@ -194,8 +194,11 @@ carefully before reaching for unchanged code.
|
|||
stdlib, ORM, or framework call's semantics matter to the change, confirm
|
||||
the contract before assuming a bug or assuming safety.
|
||||
|
||||
Use `add_finding` to record each candidate. Don't over-investigate before
|
||||
recording — capture the finding, keep moving, then rank and prune before
|
||||
Use `add_finding` to record each candidate. Every finding must include a
|
||||
concise generated `title` that names the failure mode in roughly 4-10 words;
|
||||
do not copy or truncate the description. Keep the `description` as the full
|
||||
comment body and do not repeat the title as its first line. Don't over-investigate
|
||||
before recording — capture the finding, keep moving, then rank and prune before
|
||||
publishing.
|
||||
|
||||
# Before publish_review
|
||||
|
|
@ -538,9 +541,11 @@ def _format_existing_findings(findings: list[dict]) -> str:
|
|||
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}"
|
||||
title = f.get("title")
|
||||
title_prefix = f"{title}: " if isinstance(title, str) and title.strip() else ""
|
||||
lines.append(
|
||||
f"- [{f.get('id')}] ({f.get('severity')}, {f.get('category')}) "
|
||||
f"{location} — {f.get('description', '').strip()}"
|
||||
f"{location} — {title_prefix}{f.get('description', '').strip()}"
|
||||
)
|
||||
human_reply = f.get("last_human_reply_body")
|
||||
if isinstance(human_reply, str) and human_reply:
|
||||
|
|
|
|||
|
|
@ -28,6 +28,8 @@ REVIEWER_THREAD_KIND = "reviewer"
|
|||
# code for the author and clutters the comment. We cap at 4 lines and drop
|
||||
# longer suggestions; the description still gets posted on its own.
|
||||
MAX_SUGGESTION_LINES = 4
|
||||
MAX_FINDING_TITLE_LENGTH = 120
|
||||
DEFAULT_FINDING_TITLE = "Code review finding"
|
||||
|
||||
|
||||
def clip_suggestion(suggestion: str | None) -> tuple[str | None, bool]:
|
||||
|
|
@ -39,6 +41,19 @@ def clip_suggestion(suggestion: str | None) -> tuple[str | None, bool]:
|
|||
return suggestion, False
|
||||
|
||||
|
||||
def normalize_finding_title(title: str | None, description: str = "") -> str:
|
||||
"""Return a compact finding title suitable for a review comment headline."""
|
||||
raw = title.strip() if isinstance(title, str) else ""
|
||||
if not raw and description:
|
||||
raw = description.strip().split("\n", 1)[0].strip()
|
||||
compact = " ".join(raw.split())
|
||||
if not compact:
|
||||
return DEFAULT_FINDING_TITLE
|
||||
if len(compact) > MAX_FINDING_TITLE_LENGTH:
|
||||
return f"{compact[: MAX_FINDING_TITLE_LENGTH - 3].rstrip()}..."
|
||||
return compact
|
||||
|
||||
|
||||
Severity = Literal["low", "medium", "high", "critical"]
|
||||
Confidence = Literal["low", "medium", "high"]
|
||||
FindingStatus = Literal["open", "resolved", "dismissed"]
|
||||
|
|
@ -61,14 +76,15 @@ SEVERITY_ORDER: dict[Severity, int] = {
|
|||
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.
|
||||
All fields are optional at the TypedDict level so partial updates and
|
||||
legacy findings without generated titles are representable.
|
||||
"""
|
||||
|
||||
id: str
|
||||
severity: Severity
|
||||
confidence: Confidence
|
||||
category: str
|
||||
title: str
|
||||
file: str
|
||||
start_line: int | None
|
||||
end_line: int | None
|
||||
|
|
@ -160,6 +176,7 @@ def new_finding(
|
|||
end_line: int | None,
|
||||
description: str,
|
||||
sha: str,
|
||||
title: str | None = None,
|
||||
confidence: Confidence = "medium",
|
||||
side: DiffSide = "RIGHT",
|
||||
suggestion: str | None = None,
|
||||
|
|
@ -185,7 +202,7 @@ def new_finding(
|
|||
"last_github_sync_at": None,
|
||||
"last_error": None,
|
||||
}
|
||||
return {
|
||||
finding: Finding = {
|
||||
"id": resolved_id,
|
||||
"severity": severity,
|
||||
"confidence": confidence,
|
||||
|
|
@ -218,6 +235,9 @@ def new_finding(
|
|||
"surface": surface,
|
||||
"interactions": [],
|
||||
}
|
||||
if title is not None:
|
||||
finding["title"] = normalize_finding_title(title)
|
||||
return finding
|
||||
|
||||
|
||||
def _finding_fingerprint(
|
||||
|
|
|
|||
|
|
@ -27,7 +27,7 @@ from typing import Any, TypedDict
|
|||
|
||||
import httpx
|
||||
|
||||
from .reviewer_findings import DiffSide, Finding
|
||||
from .reviewer_findings import DiffSide, Finding, normalize_finding_title
|
||||
from .utils.github_token import GitHubAuthError
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
|
@ -91,9 +91,9 @@ def render_inline_comment_body(finding: Finding) -> str:
|
|||
|
||||
<!-- metadata marker -->
|
||||
|
||||
🟡 **Title (first line of the description)**
|
||||
🟡 **Generated finding title**
|
||||
|
||||
<remaining description detail>
|
||||
<finding description detail>
|
||||
|
||||
*(Refers to lines X-Y)*
|
||||
|
||||
|
|
@ -118,7 +118,7 @@ def render_inline_comment_body(finding: Finding) -> str:
|
|||
}
|
||||
marker = f"<!-- open-swe-review-comment {json.dumps(marker_payload, separators=(',', ':'))} -->"
|
||||
|
||||
title, detail = _split_title_and_detail(description)
|
||||
title, detail = _split_title_and_detail(description, finding.get("title"))
|
||||
line_ref = _format_line_reference(finding.get("start_line"), finding.get("end_line"))
|
||||
|
||||
body_parts = [marker, "", f"{_severity_emoji(severity)} **{title}**"]
|
||||
|
|
@ -144,20 +144,26 @@ def _severity_emoji(severity: str) -> str:
|
|||
}.get(severity, "🟡")
|
||||
|
||||
|
||||
def _split_title_and_detail(description: str) -> tuple[str, str]:
|
||||
"""Split a description into a short bold title and the remaining detail.
|
||||
def _split_title_and_detail(description: str, title: object = None) -> tuple[str, str]:
|
||||
"""Split a generated finding title from the review comment detail."""
|
||||
if isinstance(title, str) and title.strip():
|
||||
normalized_title = normalize_finding_title(title)
|
||||
detail = description.strip()
|
||||
if detail:
|
||||
lines = description.split("\n")
|
||||
if normalize_finding_title(lines[0]) == normalized_title:
|
||||
detail = "\n".join(lines[1:]).strip()
|
||||
return normalized_title, detail
|
||||
|
||||
The first line becomes the title; everything after it is the detail, so the
|
||||
title text is never duplicated in the body.
|
||||
"""
|
||||
if not description:
|
||||
return "Code review finding", ""
|
||||
return normalize_finding_title(None), ""
|
||||
lines = description.split("\n")
|
||||
first_line = lines[0].strip()
|
||||
detail = "\n".join(lines[1:]).strip()
|
||||
if len(first_line) > 120:
|
||||
return first_line[:117] + "...", description
|
||||
return first_line, detail
|
||||
normalized_title = normalize_finding_title(first_line)
|
||||
if not detail and normalized_title != " ".join(first_line.split()):
|
||||
return normalized_title, description
|
||||
return normalized_title, detail
|
||||
|
||||
|
||||
def _format_line_reference(start_line: int | None, end_line: int | None) -> str:
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@ from langgraph.config import get_config
|
|||
|
||||
from ..reviewer_diff import is_range_in_diff
|
||||
from ..reviewer_findings import (
|
||||
DEFAULT_FINDING_TITLE,
|
||||
MAX_SUGGESTION_LINES,
|
||||
Confidence,
|
||||
DiffSide,
|
||||
|
|
@ -18,6 +19,7 @@ from ..reviewer_findings import (
|
|||
clip_suggestion,
|
||||
get_thread_id_from_runtime,
|
||||
new_finding,
|
||||
normalize_finding_title,
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -26,6 +28,7 @@ def add_finding(
|
|||
confidence: str,
|
||||
category: str,
|
||||
file: str,
|
||||
title: str,
|
||||
description: str,
|
||||
start_line: int | None = None,
|
||||
end_line: int | None = None,
|
||||
|
|
@ -39,8 +42,9 @@ def add_finding(
|
|||
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
|
||||
diff. Prefer one finding per issue, with a concise generated ``title`` that
|
||||
names the failure mode, a clear ``description`` body, 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.
|
||||
|
|
@ -54,7 +58,10 @@ def add_finding(
|
|||
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.
|
||||
title: Concise generated headline for the finding. Name the failure mode
|
||||
in roughly 4-10 words; do not copy or truncate the description.
|
||||
description: Markdown body the user sees. Do not repeat ``title`` as the
|
||||
first line.
|
||||
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
|
||||
|
|
@ -89,6 +96,10 @@ def add_finding(
|
|||
if start_line is None and end_line is not None:
|
||||
start_line = end_line
|
||||
|
||||
normalized_title = normalize_finding_title(title)
|
||||
if normalized_title == DEFAULT_FINDING_TITLE:
|
||||
return {"success": False, "error": "title must be a non-empty generated headline"}
|
||||
|
||||
if severity not in {"low", "medium", "high", "critical"}:
|
||||
return {"success": False, "error": f"Invalid severity: {severity}"}
|
||||
if confidence not in {"low", "medium", "high"}:
|
||||
|
|
@ -132,6 +143,7 @@ def add_finding(
|
|||
end_line=end_line,
|
||||
description=description,
|
||||
sha=str(head_sha) if isinstance(head_sha, str) else "",
|
||||
title=normalized_title,
|
||||
side=_cast_side(side),
|
||||
suggestion=clipped_suggestion,
|
||||
diff_hunk=diff_hunk,
|
||||
|
|
|
|||
|
|
@ -8,11 +8,13 @@ from typing import Any
|
|||
from langgraph.config import get_config
|
||||
|
||||
from ..reviewer_findings import (
|
||||
DEFAULT_FINDING_TITLE,
|
||||
MAX_SUGGESTION_LINES,
|
||||
Finding,
|
||||
clip_suggestion,
|
||||
get_thread_id_from_runtime,
|
||||
list_findings,
|
||||
normalize_finding_title,
|
||||
update_finding_fields,
|
||||
)
|
||||
|
||||
|
|
@ -43,6 +45,7 @@ def update_finding(
|
|||
status: str | None = None,
|
||||
severity: str | None = None,
|
||||
confidence: str | None = None,
|
||||
title: str | None = None,
|
||||
description: str | None = None,
|
||||
suggestion: str | None = None,
|
||||
note: str | None = None,
|
||||
|
|
@ -61,7 +64,9 @@ def update_finding(
|
|||
severity: New severity, if reassessing.
|
||||
confidence: New confidence rating (``low``, ``medium``, ``high``), if
|
||||
new commits change how sure you are the finding is a real issue.
|
||||
description: New description body, if revising.
|
||||
title: New concise generated headline, if revising.
|
||||
description: New description body, if revising. Do not repeat ``title``
|
||||
as the first line.
|
||||
suggestion: New replacement text. Pass an empty string to clear it.
|
||||
Capped at 4 lines — longer values are dropped (the finding keeps
|
||||
its description). Only set this for small, obvious fixes.
|
||||
|
|
@ -86,6 +91,11 @@ def update_finding(
|
|||
updates["severity"] = severity
|
||||
if confidence is not None:
|
||||
updates["confidence"] = confidence
|
||||
if title is not None:
|
||||
normalized_title = normalize_finding_title(title)
|
||||
if normalized_title == DEFAULT_FINDING_TITLE:
|
||||
return {"success": False, "error": "title must be a non-empty generated headline"}
|
||||
updates["title"] = normalized_title
|
||||
if description is not None:
|
||||
updates["description"] = description
|
||||
if suggestion is not None:
|
||||
|
|
|
|||
|
|
@ -69,13 +69,32 @@ def test_render_inline_comment_body_uses_severity_emoji_and_bold_title() -> None
|
|||
assert "🔴 **Null deref**" in body
|
||||
|
||||
|
||||
def test_render_inline_comment_body_uses_generated_title() -> None:
|
||||
description = "This request can fail because the new path skips auth token refresh."
|
||||
body = render_inline_comment_body(_f(title="Refresh token skipped", description=description))
|
||||
|
||||
assert "🟠 **Refresh token skipped**" in body
|
||||
assert description in body
|
||||
assert "This request can fail because the new path skips auth token refresh" in body
|
||||
|
||||
|
||||
def test_render_inline_comment_body_does_not_duplicate_first_line() -> None:
|
||||
body = render_inline_comment_body(
|
||||
_f(description="Short summary line\n\nLonger detail paragraph."),
|
||||
)
|
||||
assert "**Short summary line**" in body
|
||||
assert "Longer detail paragraph." in body
|
||||
# The first line is the bold title and must not also appear in the detail body.
|
||||
assert body.count("Short summary line") == 1
|
||||
|
||||
|
||||
def test_render_inline_comment_body_does_not_duplicate_generated_title() -> None:
|
||||
body = render_inline_comment_body(
|
||||
_f(
|
||||
title="Short summary line", description="Short summary line\n\nLonger detail paragraph."
|
||||
),
|
||||
)
|
||||
assert "**Short summary line**" in body
|
||||
assert "Longer detail paragraph." in body
|
||||
assert body.count("Short summary line") == 1
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -40,6 +40,7 @@ def test_add_finding_rejects_invalid_severity() -> None:
|
|||
confidence="high",
|
||||
category="x",
|
||||
file="foo.py",
|
||||
title="Generated title",
|
||||
description="d",
|
||||
start_line=11,
|
||||
end_line=11,
|
||||
|
|
@ -48,6 +49,22 @@ def test_add_finding_rejects_invalid_severity() -> None:
|
|||
assert "severity" in result["error"].lower()
|
||||
|
||||
|
||||
def test_add_finding_rejects_empty_title() -> None:
|
||||
with patch("agent.tools.add_finding.get_config", return_value=_config()):
|
||||
result = add_finding(
|
||||
severity="high",
|
||||
confidence="high",
|
||||
category="correctness",
|
||||
file="foo.py",
|
||||
title=" ",
|
||||
description="d",
|
||||
start_line=11,
|
||||
end_line=11,
|
||||
)
|
||||
assert result["success"] is False
|
||||
assert "title" 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(
|
||||
|
|
@ -55,6 +72,7 @@ def test_add_finding_rejects_out_of_diff_lines() -> None:
|
|||
confidence="high",
|
||||
category="correctness",
|
||||
file="foo.py",
|
||||
title="Generated title",
|
||||
description="d",
|
||||
start_line=99,
|
||||
end_line=99,
|
||||
|
|
@ -89,6 +107,7 @@ def test_add_finding_accepts_left_side_anchor_on_old_line() -> None:
|
|||
confidence="high",
|
||||
category="correctness",
|
||||
file="foo.py",
|
||||
title="Release resources removed",
|
||||
description="deleted call to releaseResources()",
|
||||
start_line=51,
|
||||
end_line=51,
|
||||
|
|
@ -117,6 +136,7 @@ def test_add_finding_rejects_left_anchor_outside_old_side_set() -> None:
|
|||
confidence="high",
|
||||
category="correctness",
|
||||
file="foo.py",
|
||||
title="Generated title",
|
||||
description="d",
|
||||
start_line=99,
|
||||
end_line=99,
|
||||
|
|
@ -133,6 +153,7 @@ def test_add_finding_rejects_invalid_confidence() -> None:
|
|||
confidence="certain",
|
||||
category="correctness",
|
||||
file="foo.py",
|
||||
title="Generated title",
|
||||
description="d",
|
||||
start_line=11,
|
||||
end_line=11,
|
||||
|
|
@ -158,6 +179,7 @@ def test_add_finding_persists_to_thread_metadata() -> None:
|
|||
confidence="high",
|
||||
category="style",
|
||||
file="foo.py",
|
||||
title="Rename breaks reference",
|
||||
description="rename",
|
||||
start_line=11,
|
||||
end_line=12,
|
||||
|
|
@ -168,6 +190,7 @@ def test_add_finding_persists_to_thread_metadata() -> None:
|
|||
assert "finding_id" in result
|
||||
persisted_thread, persisted = captured[0]
|
||||
assert persisted_thread == "tid-1"
|
||||
assert persisted["title"] == "Rename breaks reference"
|
||||
assert persisted["file"] == "foo.py"
|
||||
assert persisted["start_line"] == 11
|
||||
assert persisted["end_line"] == 12
|
||||
|
|
@ -192,6 +215,7 @@ def test_add_finding_allows_file_level_with_no_lines() -> None:
|
|||
confidence="medium",
|
||||
category="style",
|
||||
file="missing.py",
|
||||
title="File-level issue",
|
||||
description="file-level note",
|
||||
)
|
||||
assert result["success"] is True
|
||||
|
|
@ -252,6 +276,28 @@ def test_update_finding_rejects_empty_update() -> None:
|
|||
assert "No fields" in result["error"]
|
||||
|
||||
|
||||
def test_update_finding_updates_title() -> None:
|
||||
captured: list[Any] = []
|
||||
|
||||
async def fake_update(thread_id: str, finding_id: str, updates: Any) -> Any:
|
||||
captured.append(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.list_findings",
|
||||
AsyncMock(return_value=[_existing_finding()]),
|
||||
),
|
||||
patch("agent.tools.update_finding.update_finding_fields", side_effect=fake_update),
|
||||
):
|
||||
result = update_finding(finding_id="f_a", title="new generated title")
|
||||
|
||||
assert result["success"] is True
|
||||
assert captured[0]["title"] == "new generated title"
|
||||
|
||||
|
||||
def test_add_finding_drops_long_suggestion() -> None:
|
||||
captured: list[Any] = []
|
||||
|
||||
|
|
@ -270,6 +316,7 @@ def test_add_finding_drops_long_suggestion() -> None:
|
|||
confidence="high",
|
||||
category="style",
|
||||
file="foo.py",
|
||||
title="Rewrite changes behavior",
|
||||
description="rewrite",
|
||||
start_line=11,
|
||||
end_line=12,
|
||||
|
|
@ -300,6 +347,7 @@ def test_add_finding_keeps_short_suggestion() -> None:
|
|||
confidence="medium",
|
||||
category="style",
|
||||
file="foo.py",
|
||||
title="Rename breaks reference",
|
||||
description="rename",
|
||||
start_line=11,
|
||||
end_line=12,
|
||||
|
|
@ -329,6 +377,7 @@ def test_add_finding_preserves_multi_line_range() -> None:
|
|||
confidence="low",
|
||||
category="style",
|
||||
file="foo.py",
|
||||
title="Range spans issue",
|
||||
description="span the relevant range",
|
||||
start_line=15,
|
||||
end_line=19,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue