From 089db06135e77b1cd4f691a5bdb35290fb3d0c89 Mon Sep 17 00:00:00 2001 From: Johannes du Plessis Date: Thu, 7 May 2026 19:59:25 -0700 Subject: [PATCH] feat(open-swe): collapse oversized finding ranges to start line (#1263) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Anchoring a finding to a 25-line range (e.g. an entire function) makes GitHub render the whole block as context above the comment, burying the review text. Cap ranges at 10 lines and collapse to the start line when exceeded; small ranges (≤10 lines) still render as a multi-line anchor. Updated the reviewer prompt to anchor tightly rather than range-select the whole function. --- agent/reviewer.py | 8 +++-- agent/reviewer_findings.py | 19 ++++++++++++ agent/tools/add_finding.py | 17 +++++++++++ tests/test_reviewer_tools.py | 57 +++++++++++++++++++++++++++++++++++- 4 files changed, 98 insertions(+), 3 deletions(-) diff --git a/agent/reviewer.py b/agent/reviewer.py index a601964d..f419e319 100644 --- a/agent/reviewer.py +++ b/agent/reviewer.py @@ -109,8 +109,12 @@ Cloning is optional — for most PRs the diff alone is enough. issue worth surfacing; `low` = small nit; `informational` = FYI / context, not a flaw. Inflated severities erode trust — be honest. - `category`: e.g. `correctness`, `security`, `perf`, `style`, `flag`. - - `file`, `start_line`, `end_line`: anchor inside the PR diff. Use a range - when the issue spans multiple lines (e.g. an entire function). + - `file`, `start_line`, `end_line`: anchor inside the PR diff. Keep + ranges tight — anchor to the few lines that actually matter (the call + site, the signature line, the conditional). **Do not range-select an + entire function**; GitHub renders the full range as context above the + comment and buries the point. Ranges over ~10 lines are auto-collapsed + to the start line. - `description`: what's wrong, in 1–4 sentences. Markdown is fine. - `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 diff --git a/agent/reviewer_findings.py b/agent/reviewer_findings.py index 19214c87..e7c134d7 100644 --- a/agent/reviewer_findings.py +++ b/agent/reviewer_findings.py @@ -29,6 +29,13 @@ REVIEWER_THREAD_KIND = "reviewer" # longer suggestions; the description still gets posted on its own. MAX_SUGGESTION_LINES = 4 +# Anchoring a finding to a giant range (e.g. an entire function) makes the +# GitHub comment dump dozens of lines of context above the actual review +# text, which buries the point. When the range exceeds this, collapse it to +# the first line — the comment still anchors near the issue without showing +# the whole block. ~5-line ranges are still useful, so the cap is generous. +MAX_FINDING_RANGE_LINES = 10 + def clip_suggestion(suggestion: str | None) -> tuple[str | None, bool]: """Return (suggestion_or_none, was_dropped). Drops if over the line cap.""" @@ -39,6 +46,18 @@ def clip_suggestion(suggestion: str | None) -> tuple[str | None, bool]: return suggestion, False +def clip_finding_range( + start_line: int | None, + end_line: int | None, +) -> tuple[int | None, int | None, bool]: + """Return (start, end, was_collapsed). Collapses end→start if over cap.""" + if start_line is None or end_line is None: + return start_line, end_line, False + if end_line - start_line + 1 > MAX_FINDING_RANGE_LINES: + return start_line, start_line, True + return start_line, end_line, 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 2295f16b..7ae5c3d7 100644 --- a/agent/tools/add_finding.py +++ b/agent/tools/add_finding.py @@ -9,11 +9,13 @@ from langgraph.config import get_config from ..reviewer_diff import is_range_in_diff from ..reviewer_findings import ( + MAX_FINDING_RANGE_LINES, MAX_SUGGESTION_LINES, DiffSide, Finding, Severity, append_finding, + clip_finding_range, clip_suggestion, get_thread_id_from_runtime, new_finding, @@ -56,6 +58,11 @@ def add_finding( ``end_line`` for single-line findings; less than ``end_line`` for ranges. Omit (with ``end_line``) for file-level findings. end_line: 1-based end line, inclusive. Defaults to ``start_line``. + Keep ranges tight — anchor to the few lines that actually matter + for the comment. Ranges over 10 lines are auto-collapsed to a + single line because GitHub renders the full range as context + above the comment, which buries the point. Don't anchor to an + entire function; pick the call site or signature line instead. 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, @@ -99,6 +106,8 @@ def add_finding( ), } + start_line, end_line, range_collapsed = clip_finding_range(start_line, end_line) + diff_hunk: str | None = None if isinstance(diff_text, str) and diff_text: from ..reviewer_diff import extract_diff_hunk @@ -130,6 +139,14 @@ def add_finding( "dropped — the finding was recorded with description only. Only " "include `suggestion` for small, obvious fixes." ) + if range_collapsed: + result["range_collapsed"] = True + result["range_warning"] = ( + f"Range exceeded the {MAX_FINDING_RANGE_LINES}-line cap and was " + f"collapsed to a single line ({start_line}). Anchor findings to " + "the most relevant line — GitHub renders large ranges as walls " + "of context that bury the comment." + ) return result diff --git a/tests/test_reviewer_tools.py b/tests/test_reviewer_tools.py index 1f74a115..4cc0c6a1 100644 --- a/tests/test_reviewer_tools.py +++ b/tests/test_reviewer_tools.py @@ -16,7 +16,7 @@ def _config(**configurable_overrides: Any) -> dict[str, Any]: "thread_id": "tid-1", "head_sha": "sha-head", "diff_text": "", - "diff_line_set": {"foo.py": [10, 11, 12]}, + "diff_line_set": {"foo.py": list(range(10, 41))}, }, "metadata": {}, } @@ -175,6 +175,61 @@ def test_add_finding_keeps_short_suggestion() -> None: assert captured[0]["suggestion"] == short_suggestion +def test_add_finding_collapses_oversized_range() -> None: + captured: list[Any] = [] + + async def fake_append(thread_id: str, finding: Any) -> Any: + captured.append(finding) + return finding + + 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="correctness", + file="foo.py", + description="big function", + start_line=15, + end_line=40, # 26 lines — well over the cap + ) + + assert result["success"] is True + assert result.get("range_collapsed") is True + assert "range_warning" in result + assert captured[0]["start_line"] == 15 + assert captured[0]["end_line"] == 15 + + +def test_add_finding_keeps_small_range() -> None: + captured: list[Any] = [] + + async def fake_append(thread_id: str, finding: Any) -> Any: + captured.append(finding) + return finding + + 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="low", + category="style", + file="foo.py", + description="five lines", + start_line=15, + end_line=19, # 5 lines — within the cap + ) + + assert result["success"] is True + assert "range_collapsed" not in result + assert captured[0]["start_line"] == 15 + assert captured[0]["end_line"] == 19 + + def test_update_finding_rejects_long_suggestion_without_clobbering() -> None: """Over-cap suggestion alongside other fields: drop suggestion, keep the rest.""" captured: list[Any] = []