diff --git a/agent/reviewer.py b/agent/reviewer.py index e84daa56..a601964d 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -62,8 +62,9 @@ REVIEWER_PROMPT_TEMPLATE = """You are an expert code reviewer. 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". +important findings as inline comments — with a concrete `suggestion` block +only when the fix is small enough (≤4 lines) that the user can scan it and +click "Commit suggestion". ### Working environment @@ -111,9 +112,12 @@ Cloning is optional — for most PRs the diff alone is enough. - `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". + - `suggestion`: **only** include for small, obvious fixes that fit in 4 + lines or fewer — a one-liner rename, a missing guard, a typo, a flipped + condition. Anything longer reads as a rewrite rather than a review and + is dropped by the tool. For non-trivial fixes, leave `suggestion` unset + and let the description explain what's wrong; the author decides how to + fix it. 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 @@ -151,9 +155,10 @@ You may use `list_findings()` at any time to inspect what's persisted. touch. Anchor every finding to a line that the PR actually changes. - **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. +- **Suggestions are for small, obvious fixes only.** If the fix is more than + ~4 lines, skip the `suggestion` field — the description alone is more + useful than a long rewrite. Description-only findings are the default; + `suggestion` is the exception for trivially-actionable changes. - **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 @@ -196,9 +201,9 @@ def _build_first_review_context( f"`GH_TOKEN=dummy gh pr diff {pr_number} --repo {repo_owner}/{repo_name}`, " f"then review only what's in that diff.\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." + f"issues with `add_finding` (one per issue; only include `suggestion` " + f"when the fix is ≤4 lines and obvious), then call `publish_review` " + f"once at the end." ) diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py index 04d9e6b1..19214c87 100644 --- a/agent/reviewer_findings.py +++ b/agent/reviewer_findings.py @@ -23,6 +23,22 @@ logger = logging.getLogger(__name__) REVIEWER_THREAD_KIND = "reviewer" +# Suggestions are only useful when the reader can scan them at a glance and +# accept with one click. Anything longer reads as the reviewer rewriting the +# 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 + + +def clip_suggestion(suggestion: str | None) -> tuple[str | None, bool]: + """Return (suggestion_or_none, was_dropped). Drops if over the line cap.""" + if not suggestion: + return suggestion, False + if suggestion.count("\n") + 1 > MAX_SUGGESTION_LINES: + return None, True + return suggestion, False + + Severity = Literal["informational", "low", "medium", "high", "critical"] FindingStatus = Literal["open", "resolved", "dismissed"] DiffSide = Literal["LEFT", "RIGHT"] diff --git a/agent/tools/add_finding.py b/agent/tools/add_finding.py index d3033f0a..2295f16b 100644 --- a/agent/tools/add_finding.py +++ b/agent/tools/add_finding.py @@ -9,10 +9,12 @@ from langgraph.config import get_config from ..reviewer_diff import is_range_in_diff from ..reviewer_findings import ( + MAX_SUGGESTION_LINES, DiffSide, Finding, Severity, append_finding, + clip_suggestion, get_thread_id_from_runtime, new_finding, ) @@ -56,7 +58,12 @@ def add_finding( 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". + 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``. @@ -98,6 +105,8 @@ def add_finding( 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), category=category, @@ -107,13 +116,21 @@ def add_finding( description=description, sha=str(head_sha) if isinstance(head_sha, str) else "", side=_cast_side(side), - suggestion=suggestion, + suggestion=clipped_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"]} + 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: diff --git a/agent/tools/update_finding.py b/agent/tools/update_finding.py index 06e452be..da903d71 100644 --- a/agent/tools/update_finding.py +++ b/agent/tools/update_finding.py @@ -8,6 +8,8 @@ from typing import Any from langgraph.config import get_config from ..reviewer_findings import ( + MAX_SUGGESTION_LINES, + clip_suggestion, get_thread_id_from_runtime, update_finding_fields, ) @@ -35,6 +37,8 @@ def update_finding( severity: New severity, if reassessing. description: New description body, if revising. 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. note: Optional free-form note explaining the change. Persisted on the finding under ``last_update_note``. @@ -53,6 +57,7 @@ def update_finding( return {"success": False, "error": f"Invalid severity: {severity}"} updates: dict[str, Any] = {} + suggestion_dropped = False if status is not None: updates["status"] = status if severity is not None: @@ -60,7 +65,12 @@ def update_finding( if description is not None: updates["description"] = description if suggestion is not None: - updates["suggestion"] = suggestion or None + if suggestion == "": + updates["suggestion"] = None + else: + clipped, suggestion_dropped = clip_suggestion(suggestion) + if not suggestion_dropped: + updates["suggestion"] = clipped if note is not None: updates["last_update_note"] = note @@ -71,10 +81,30 @@ def update_finding( updates["last_confirmed_sha"] = head_sha if not updates: + if suggestion_dropped: + return { + "success": False, + "suggestion_dropped": True, + "error": ( + f"Suggestion exceeded the {MAX_SUGGESTION_LINES}-line cap " + "and was rejected; no other fields were provided, so " + "nothing was updated. Only include `suggestion` for " + "small, obvious fixes." + ), + } 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} + result: dict[str, Any] = {"success": True, "finding": updated} + if suggestion_dropped: + result["suggestion_dropped"] = True + result["warning"] = ( + f"Suggestion exceeded the {MAX_SUGGESTION_LINES}-line cap and was " + "rejected — the finding's prior `suggestion` was left unchanged " + "and other fields were updated normally. Only include " + "`suggestion` for small, obvious fixes." + ) + return result diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index 996620a3..1f74a115 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -118,6 +118,121 @@ def test_update_finding_rejects_empty_update() -> None: assert "No fields" in result["error"] +def test_add_finding_drops_long_suggestion() -> None: + captured: list[Any] = [] + + async def fake_append(thread_id: str, finding: Any) -> Any: + captured.append(finding) + return finding + + long_suggestion = "\n".join(f"line_{i}" for i in range(6)) + 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="rewrite", + start_line=11, + end_line=12, + suggestion=long_suggestion, + ) + + assert result["success"] is True + assert result.get("suggestion_dropped") is True + assert "warning" in result + assert captured[0]["suggestion"] is None + + +def test_add_finding_keeps_short_suggestion() -> None: + captured: list[Any] = [] + + async def fake_append(thread_id: str, finding: Any) -> Any: + captured.append(finding) + return finding + + short_suggestion = "a\nb\nc\nd" # exactly 4 lines — at the cap + 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=short_suggestion, + ) + + assert result["success"] is True + assert "suggestion_dropped" not in result + assert captured[0]["suggestion"] == short_suggestion + + +def test_update_finding_rejects_long_suggestion_without_clobbering() -> None: + """Over-cap suggestion alongside other fields: drop suggestion, keep the rest.""" + captured: list[Any] = [] + + async def fake_update(thread_id: str, finding_id: str, updates: Any) -> Any: + captured.append(updates) + return {"id": finding_id, **updates} + + long_suggestion = "\n".join(f"line_{i}" for i in range(6)) + 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", + description="updated description", + suggestion=long_suggestion, + ) + + assert result["success"] is True + assert result.get("suggestion_dropped") is True + assert "suggestion" not in captured[0] + assert captured[0]["description"] == "updated description" + + +def test_update_finding_long_suggestion_only_returns_failure() -> None: + """Over-cap suggestion as the only field: fail outright rather than no-op.""" + long_suggestion = "\n".join(f"line_{i}" for i in range(6)) + 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"), + ): + result = update_finding(finding_id="f_a", suggestion=long_suggestion) + + assert result["success"] is False + assert result.get("suggestion_dropped") is True + assert "cap" in result["error"] + + +def test_update_finding_empty_string_clears_suggestion() -> 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.update_finding_fields", side_effect=fake_update), + ): + result = update_finding(finding_id="f_a", suggestion="") + + assert result["success"] is True + assert captured[0]["suggestion"] is None + + def test_update_finding_passes_through_fields() -> None: captured: list[Any] = []