feat(open-swe): cap reviewer suggestions at 4 lines (#1262)

* feat(open-swe): cap reviewer suggestions at 4 lines

Long suggestion blocks read as the reviewer rewriting the code rather
than flagging an issue, which clutters PR comments. Steer the reviewer
toward description-only findings for non-trivial fixes, and enforce the
cap in `add_finding` / `update_finding` so suggestions over 4 lines are
dropped (the finding itself still publishes).

* fix(open-swe): don't clobber prior suggestion on over-cap update

`update_finding` was setting `suggestion=None` whenever clip_suggestion
dropped the input, which silently wiped any existing suggestion on the
finding. Distinguish the three cases: empty string clears, valid value
sets, over-cap value is rejected without touching the stored field.

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."
     )
 
 

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"]

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:

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

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] = []