From 9998119921b6f287707d1d0124bed198b983e232 Mon Sep 17 00:00:00 2001 From: "open-swe[bot]" <215916821+open-swe[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 15:28:55 -0700 Subject: [PATCH] feat: generate reviewer finding titles (#1355) Co-authored-by: open-swe[bot] Co-authored-by: Johannes du Plessis <51395795+johannes117@users.noreply.github.com> --- REVIEWER_DESIGN.md | 5 ++-- agent/reviewer.py | 11 +++++--- agent/reviewer_findings.py | 26 +++++++++++++++--- agent/reviewer_publish.py | 32 +++++++++++++--------- agent/tools/add_finding.py | 18 ++++++++++--- agent/tools/update_finding.py | 12 ++++++++- tests/test_reviewer_publish.py | 21 ++++++++++++++- tests/test_reviewer_tools.py | 49 ++++++++++++++++++++++++++++++++++ 8 files changed, 148 insertions(+), 26 deletions(-) diff --git a/REVIEWER_DESIGN.md b/REVIEWER_DESIGN.md index 79620fcc..cc54a246 100644 --- a/REVIEWER_DESIGN.md +++ b/REVIEWER_DESIGN.md @@ -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. diff --git a/agent/reviewer.py b/agent/reviewer.py index 0905ed81..007a8aa0 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -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: diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py index 59bb9e9b..94866b82 100644 --- a/agent/reviewer_findings.py +++ b/agent/reviewer_findings.py @@ -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( diff --git a/agent/reviewer_publish.py b/agent/reviewer_publish.py index d16427cc..06550288 100644 --- a/agent/reviewer_publish.py +++ b/agent/reviewer_publish.py @@ -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: - 🟡 **Title (first line of the description)** + 🟡 **Generated finding title** - + *(Refers to lines X-Y)* @@ -118,7 +118,7 @@ def render_inline_comment_body(finding: Finding) -> str: } marker = f"" - 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: diff --git a/agent/tools/add_finding.py b/agent/tools/add_finding.py index 4bd30ca3..b1af3ebe 100644 --- a/agent/tools/add_finding.py +++ b/agent/tools/add_finding.py @@ -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, diff --git a/agent/tools/update_finding.py b/agent/tools/update_finding.py index 61b3f4f5..ce03b8e6 100644 --- a/agent/tools/update_finding.py +++ b/agent/tools/update_finding.py @@ -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: diff --git a/tests/test_reviewer_publish.py b/tests/test_reviewer_publish.py index db3e0c72..f4d4369b 100644 --- a/tests/test_reviewer_publish.py +++ b/tests/test_reviewer_publish.py @@ -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 diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index 2ceb06cd..b2a08555 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -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,