From 182e30e76969b07b1d56dca531b3f7d339480dfd Mon Sep 17 00:00:00 2001 From: zaebee Date: Sat, 13 Jun 2026 10:34:02 +0000 Subject: [PATCH 1/5] fix(guardian): anchor inline comments to a verbatim quote, not the model's line (#181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guardian posted inline comments at whatever line the finder emitted; for new/ rewritten files every line is "commentable", so hallucinated coordinates passed the diff-index filter and landed comments on the wrong line (repro: PR #180, symbols.py:13 vs the real import at :3). Content-anchoring (the issue's fix #2) + numbered/quote prompt (#1), composed: - Finding gains an optional `anchor` — the exact source line, copied verbatim. - `diff_line_content` exposes RIGHT-side `{lineno: text}`; `diff_line_index` is now derived from it so the two never diverge. - `build_review` re-derives each finding's line from its `anchor` (falling back to `evidence`) located in the changed lines: model line trusted if it matches, else nearest match; a quote found nowhere demotes to a body comment instead of a confidently-wrong inline anchor. Nothing is lost. - Finder prompt now asks for the verbatim `anchor` field. Deterministic and backward-compatible: with no `diff_content` the model line is used as before. 8 new tests (content map, line correction, hallucinated-anchor demotion, legacy path). 1008 tests, mypy strict, ruff, doc 99.6% — all green. Refs #181 Co-Authored-By: Claude Opus 4.8 --- src/cgis/guardian/diff_index.py | 33 +++++++++++----- src/cgis/guardian/findings.py | 3 ++ src/cgis/guardian/github_poster.py | 45 ++++++++++++++++++++- src/cgis/guardian/prompts.py | 5 +++ src/cgis/guardian/runner.py | 8 ++-- tests/unit/test_guardian_diff_index.py | 15 ++++++- tests/unit/test_guardian_poster.py | 55 ++++++++++++++++++++++++++ 7 files changed, 148 insertions(+), 16 deletions(-) diff --git a/src/cgis/guardian/diff_index.py b/src/cgis/guardian/diff_index.py index 38afaedd..b4908221 100644 --- a/src/cgis/guardian/diff_index.py +++ b/src/cgis/guardian/diff_index.py @@ -12,15 +12,16 @@ def _new_file_path(header: re.Match[str]) -> str | None: return None if path == "/dev/null" else path -def diff_line_index(diff_text: str) -> dict[str, set[int]]: - """Map each changed file (new path) to the set of RIGHT-side line numbers. +def diff_line_content(diff_text: str) -> dict[str, dict[int, str]]: + """Map each changed file (new path) to ``{RIGHT-side line number: line text}``. - GitHub only accepts inline review comments on lines present in the diff; - context and added lines count, removed lines do not (spec §6.2). Renames - are keyed by the new path so keys match Finding.file. Files deleted in the - PR (+++ /dev/null) have no RIGHT side and are excluded. + The text is the line content with its leading diff marker (``+``/`` ``) + stripped, so it can be matched verbatim against a finding's quote to anchor + the comment deterministically (#181). Same right-side accounting as + :func:`diff_line_index`: added and context lines carry a number, removed + lines don't, and ``+++ /dev/null`` deletions are excluded. """ - index: dict[str, set[int]] = {} + content: dict[str, dict[int, str]] = {} current: str | None = None in_hunk = False new_line = 0 @@ -39,10 +40,22 @@ def diff_line_index(diff_text: str) -> dict[str, set[int]]: if (hunk := _HUNK_RE.match(line)) and current is not None: new_line = int(hunk.group(1)) in_hunk = True - index.setdefault(current, set()) + content.setdefault(current, {}) continue if not in_hunk or current is None or line.startswith(("-", "\\")): continue # outside a hunk / removed line / "\ No newline" marker - index[current].add(new_line) # added or context line: has a RIGHT-side number + content[current][new_line] = line[1:] # drop the '+'/' ' marker, keep the text new_line += 1 - return {path: lines for path, lines in index.items() if lines} + return {path: lines for path, lines in content.items() if lines} + + +def diff_line_index(diff_text: str) -> dict[str, set[int]]: + """Map each changed file (new path) to the set of RIGHT-side line numbers. + + GitHub only accepts inline review comments on lines present in the diff; + context and added lines count, removed lines do not (spec §6.2). Renames + are keyed by the new path so keys match Finding.file. Files deleted in the + PR (+++ /dev/null) have no RIGHT side and are excluded. Derived from + :func:`diff_line_content` so the two never diverge. + """ + return {path: set(lines) for path, lines in diff_line_content(diff_text).items()} diff --git a/src/cgis/guardian/findings.py b/src/cgis/guardian/findings.py index b5cf5a6c..1ae5f31c 100644 --- a/src/cgis/guardian/findings.py +++ b/src/cgis/guardian/findings.py @@ -22,6 +22,9 @@ class Finding(BaseModel, frozen=True): problem: str fix: str confidence: int = Field(ge=0, le=100) + # Verbatim single source line the finding sits on, used to derive the inline + # anchor deterministically instead of trusting the model's ``line`` (#181). + anchor: str | None = None verdict: Verdict | None = None skeptic_note: str | None = None diff --git a/src/cgis/guardian/github_poster.py b/src/cgis/guardian/github_poster.py index 8bff421f..305f2340 100644 --- a/src/cgis/guardian/github_poster.py +++ b/src/cgis/guardian/github_poster.py @@ -8,12 +8,41 @@ from cgis.guardian.skeptic import visible_findings +def _anchored_line(finding: Finding, content: dict[int, str] | None) -> int | None: + """Derive the inline line from the finding's verbatim quote, not the model's guess (#181). + + Searches the file's RIGHT-side lines for the one matching the finding's + ``anchor`` (falling back to ``evidence``) and returns its real number: + - no quote or no file content → keep the model's ``line`` (legacy behaviour); + - the model's ``line`` is itself a match → trust it; + - otherwise → the match nearest the model's guess; + - quote present but found nowhere in the changed lines → ``None``, so a + hallucinated coordinate demotes to a file-level body comment instead of a + confidently-wrong inline anchor. + """ + quote = (finding.anchor or finding.evidence or "").strip() + if not content or not quote: + return finding.line + needle = next((seg.strip() for seg in quote.splitlines() if seg.strip()), "") + if not needle: + return finding.line + matches = [ + n for n, text in content.items() if (s := text.strip()) and (needle in s or s in needle) + ] + if not matches: + return None + if finding.line in matches: + return finding.line + return min(matches, key=lambda n: (abs(n - (finding.line or matches[0])), n)) + + def build_review( result: ReviewResult, *, diff_index: dict[str, set[int]], skeptic_model: str | None, footer: str = "", + diff_content: dict[str, dict[int, str]] | None = None, ) -> tuple[str, list[dict[str, object]]]: """Split findings into inline comments vs body text (pure, spec §6.2). @@ -22,10 +51,17 @@ def build_review( footer (model/tokens/coverage) is appended to the body: before inline posting existed it always reached the PR via the fallback comment, but a successful inline post skips that comment — so it must travel here too. + + When ``diff_content`` is supplied, each finding's line is first re-anchored + from its verbatim quote (#181) so the comment lands on the real line rather + than the model's estimate; an unlocatable quote demotes to a body comment. """ + content_by_file = diff_content or {} inline: list[Finding] = [] outside: list[Finding] = [] - for finding in visible_findings(result.findings): + for raw in visible_findings(result.findings): + line = _anchored_line(raw, content_by_file.get(raw.file)) + finding = raw if line == raw.line else raw.model_copy(update={"line": line}) if finding.line is not None and finding.line in diff_index.get(finding.file, set()): inline.append(finding) else: @@ -50,6 +86,7 @@ def post_inline_review( diff_index: dict[str, set[int]], skeptic_model: str | None, footer: str = "", + diff_content: dict[str, dict[int, str]] | None = None, ) -> None: """POST one COMMENT review via `gh api` (auto-auth in Actions, spec §6.4). @@ -58,7 +95,11 @@ def post_inline_review( decides the fallback (spec §6.5). """ body, comments = build_review( - result, diff_index=diff_index, skeptic_model=skeptic_model, footer=footer + result, + diff_index=diff_index, + skeptic_model=skeptic_model, + footer=footer, + diff_content=diff_content, ) payload = {"event": "COMMENT", "body": body, "comments": comments} subprocess.run( diff --git a/src/cgis/guardian/prompts.py b/src/cgis/guardian/prompts.py index 1c93d165..cc21d4a5 100644 --- a/src/cgis/guardian/prompts.py +++ b/src/cgis/guardian/prompts.py @@ -122,6 +122,7 @@ def build_user_prompt(context: dict[str, str]) -> str: Return ONLY a JSON object — no prose, no markdown fences — with this exact shape: {{"findings": [{{"file": "src/path/to/file.py", "line": 123, + "anchor": "", "severity": "critical|major|minor", "category": "logic|contract|tests|types|ontology", "title": "short headline", @@ -135,6 +136,10 @@ def build_user_prompt(context: dict[str, str]) -> str: - "category" maps to the focus areas: logic = Logic Bug, contract = Library Contract, tests = Test Coverage, types = Type Safety, ontology = Ontology. - "line" is the line number in the HEAD version of the file, or null for file-level findings. +- "anchor" is the single exact line of code the finding refers to, copied VERBATIM from the + diff in section 3 (no paraphrasing, no `+`/`-` marker). It is used to position the inline + comment deterministically — if your "line" guess is off, a correct "anchor" still lands the + comment on the right line. Use null only for genuinely file-level findings. - "confidence" must be >= 80 to include a finding (the gate above). - max 5 findings; fewer is fine; an empty list means LGTM. - "summary" is mandatory; for an LGTM it lists what you checked and found correct. diff --git a/src/cgis/guardian/runner.py b/src/cgis/guardian/runner.py index a0e1744d..ef85093f 100644 --- a/src/cgis/guardian/runner.py +++ b/src/cgis/guardian/runner.py @@ -8,7 +8,7 @@ from cgis.guardian.chunked import run_review_routed from cgis.guardian.collector import ContextCollector -from cgis.guardian.diff_index import diff_line_index +from cgis.guardian.diff_index import diff_line_content from cgis.guardian.github_poster import post_inline_review from cgis.guardian.metrics import record_review from cgis.guardian.providers.base import BaseProvider, ProviderUsage @@ -134,15 +134,17 @@ async def run_guardian( posted = False if inline_repo is not None and pr is not None: try: - index = diff_line_index(collector.get_git_diff()) + diff_text = collector.get_git_diff() + content = diff_line_content(diff_text) await asyncio.to_thread( # subprocess `gh api` call — keep the loop responsive post_inline_review, repo=inline_repo, pr=pr, result=result, - diff_index=index, + diff_index={path: set(lines) for path, lines in content.items()}, skeptic_model=skeptic[1] if skeptic else None, footer=footer, + diff_content=content, ) posted = True except Exception: diff --git a/tests/unit/test_guardian_diff_index.py b/tests/unit/test_guardian_diff_index.py index a82a7115..955c1499 100644 --- a/tests/unit/test_guardian_diff_index.py +++ b/tests/unit/test_guardian_diff_index.py @@ -1,6 +1,6 @@ """Unit tests for the pure unified-diff RIGHT-side line indexer (spec §6.2).""" -from cgis.guardian.diff_index import diff_line_index +from cgis.guardian.diff_index import diff_line_content, diff_line_index _SIMPLE = """\ diff --git a/src/x.py b/src/x.py @@ -101,3 +101,16 @@ def test_plus_plus_plus_content_line_inside_hunk() -> None: "+third\n" ) assert diff_line_index(diff) == {"notes.md": {1, 2, 3}} + + +def test_diff_line_content_maps_numbers_to_text() -> None: + """RIGHT-side lines map to their verbatim text (marker stripped) for anchoring (#181).""" + content = diff_line_content(_SIMPLE)["src/x.py"] + assert content == {10: "context1", 11: "added1", 12: "added2", 13: "context2"} + # index is derived from content → identical key set + assert diff_line_index(_SIMPLE)["src/x.py"] == set(content) + + +def test_diff_line_content_new_file() -> None: + """A brand-new file maps every added line to its text.""" + assert diff_line_content(_NEW_FILE)["brand.py"] == {1: "line1", 2: "line2"} diff --git a/tests/unit/test_guardian_poster.py b/tests/unit/test_guardian_poster.py index 8bd237aa..1e987a27 100644 --- a/tests/unit/test_guardian_poster.py +++ b/tests/unit/test_guardian_poster.py @@ -100,3 +100,58 @@ def test_footer_default_empty_keeps_body_unchanged() -> None: with_default, _ = build_review(result, diff_index={}, skeptic_model=None) with_empty, _ = build_review(result, diff_index={}, skeptic_model=None, footer="") assert with_default == with_empty + + +_CONTENT = {"src/x.py": {10: " context1", 11: " added1", 12: " added2", 13: " ctx2"}} + + +def _finding(**over: object) -> Finding: + base: dict[str, object] = { + "file": "src/x.py", + "line": 11, + "severity": "major", + "category": "logic", + "title": "t", + "evidence": "e", + "problem": "p", + "fix": "f", + "confidence": 90, + } + base.update(over) + return Finding(**base) # type: ignore[arg-type] + + +def test_anchor_corrects_a_wrong_model_line() -> None: + """A verbatim anchor overrides a hallucinated line — comment lands on the real line (#181).""" + f = _finding(line=999, anchor="added2") # model guessed 999; real line is 12 + result = ReviewResult(findings=[f], summary="s") + _, comments = build_review( + result, + diff_index={"src/x.py": {10, 11, 12, 13}}, + skeptic_model=None, + diff_content=_CONTENT, + ) + assert len(comments) == 1 + assert comments[0]["line"] == 12 + + +def test_hallucinated_anchor_demotes_to_body() -> None: + """A quote that appears nowhere in the diff becomes a body note, not a wrong inline anchor.""" + f = _finding(line=11, anchor="this text is not in the diff at all") + result = ReviewResult(findings=[f], summary="s") + body, comments = build_review( + result, + diff_index={"src/x.py": {10, 11, 12, 13}}, + skeptic_model=None, + diff_content=_CONTENT, + ) + assert comments == [] # not posted at the (wrong) model line 11 + assert "t" in body # finding survives in the body — nothing lost + + +def test_no_diff_content_keeps_legacy_behaviour() -> None: + """Without diff_content, the model line is used as before (backward compatible).""" + f = _finding(line=11, anchor="added2") + result = ReviewResult(findings=[f], summary="s") + _, comments = build_review(result, diff_index={"src/x.py": {10, 11, 12}}, skeptic_model=None) + assert comments[0]["line"] == 11 From e468ff366401fdcd3dd9997b65de76a7edf22882 Mon Sep 17 00:00:00 2001 From: zaebee Date: Sat, 13 Jun 2026 12:29:54 +0000 Subject: [PATCH 2/5] fix(guardian): exact-match-first anchoring, drop reverse-substring (#181 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught that the bidirectional loose match (`needle in s or s in needle`) re-created a #181-class bug: a trivial changed line (`)`, `else:`) is a substring of a longer anchor, so `s in needle` matched it, and with no exact-match preference a hallucinated model line could pull the comment onto that false match (exact line 4 losing to a spurious `)` at 20 because 20 is nearer the model's 18). Fix on the same structure: - prefer an EXACT stripped-line equality; - fall back to substring (`needle in line`) only when there's no exact hit AND the quote is >= 5 chars, so `)`/`else:` can't substring-match anything; - drop the `s in needle` direction entirely (a model quoting MORE than the real line safely demotes to a body comment — nothing is mis-anchored). 2 regression tests: exact match beats a nearer spurious `)` substring; a 1-char anchor demotes instead of latching onto `qux()`. 1010 tests, mypy strict, ruff, doc 99.6% — all green. Refs #181 Co-Authored-By: Claude Opus 4.8 --- src/cgis/guardian/github_poster.py | 22 ++++++++++++++-------- tests/unit/test_guardian_poster.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 8 deletions(-) diff --git a/src/cgis/guardian/github_poster.py b/src/cgis/guardian/github_poster.py index 305f2340..4ae6cad5 100644 --- a/src/cgis/guardian/github_poster.py +++ b/src/cgis/guardian/github_poster.py @@ -7,6 +7,10 @@ from cgis.guardian.render import render_inline_comment, render_review_body from cgis.guardian.skeptic import visible_findings +#: Below this length a quote is too generic to substring-match safely (``)``, +#: ``else:``) — only an exact line equality may anchor it (#181 review). +_MIN_SUBSTRING_ANCHOR = 5 + def _anchored_line(finding: Finding, content: dict[int, str] | None) -> int | None: """Derive the inline line from the finding's verbatim quote, not the model's guess (#181). @@ -14,11 +18,13 @@ def _anchored_line(finding: Finding, content: dict[int, str] | None) -> int | No Searches the file's RIGHT-side lines for the one matching the finding's ``anchor`` (falling back to ``evidence``) and returns its real number: - no quote or no file content → keep the model's ``line`` (legacy behaviour); - - the model's ``line`` is itself a match → trust it; - - otherwise → the match nearest the model's guess; - - quote present but found nowhere in the changed lines → ``None``, so a - hallucinated coordinate demotes to a file-level body comment instead of a - confidently-wrong inline anchor. + - an **exact** stripped-line match wins; only if there is none do we fall back + to a substring match (``needle in line``), and only for quotes long enough + that a substring is meaningful — so a ``)`` or ``else:`` can never pull a + comment onto an unrelated trivial line; + - among the candidates: the model's ``line`` if it is one, else the nearest; + - quote present but located nowhere → ``None``, demoting to a body comment + instead of a confidently-wrong inline anchor. """ quote = (finding.anchor or finding.evidence or "").strip() if not content or not quote: @@ -26,9 +32,9 @@ def _anchored_line(finding: Finding, content: dict[int, str] | None) -> int | No needle = next((seg.strip() for seg in quote.splitlines() if seg.strip()), "") if not needle: return finding.line - matches = [ - n for n, text in content.items() if (s := text.strip()) and (needle in s or s in needle) - ] + matches = [n for n, text in content.items() if text.strip() == needle] + if not matches and len(needle) >= _MIN_SUBSTRING_ANCHOR: + matches = [n for n, text in content.items() if needle in text.strip()] if not matches: return None if finding.line in matches: diff --git a/tests/unit/test_guardian_poster.py b/tests/unit/test_guardian_poster.py index 1e987a27..3014b02b 100644 --- a/tests/unit/test_guardian_poster.py +++ b/tests/unit/test_guardian_poster.py @@ -155,3 +155,31 @@ def test_no_diff_content_keeps_legacy_behaviour() -> None: result = ReviewResult(findings=[f], summary="s") _, comments = build_review(result, diff_index={"src/x.py": {10, 11, 12}}, skeptic_model=None) assert comments[0]["line"] == 11 + + +def test_exact_match_beats_a_spurious_substring() -> None: + """A trivial line that is a substring of the anchor must not outrank the exact + match, even when the model's hallucinated line is nearer to it (#181 review).""" + content = { + "src/x.py": { + 4: "from cgis.resolver.indices import IndexBuilder, SymbolIndex", + 20: ")", # substring of the anchor — must NOT win + } + } + f = _finding(line=18, anchor="from cgis.resolver.indices import IndexBuilder, SymbolIndex") + result = ReviewResult(findings=[f], summary="s") + _, comments = build_review( + result, diff_index={"src/x.py": {4, 20}}, skeptic_model=None, diff_content=content + ) + assert comments[0]["line"] == 4 # exact match wins over the nearer ")" at 20 + + +def test_trivial_anchor_does_not_substring_match() -> None: + """A 1-char anchor like ')' can't substring-match 'qux()' — it demotes to body.""" + content = {"src/x.py": {12: " qux()"}} + f = _finding(line=12, anchor=")") + result = ReviewResult(findings=[f], summary="s") + _, comments = build_review( + result, diff_index={"src/x.py": {12}}, skeptic_model=None, diff_content=content + ) + assert comments == [] # no exact ')' line, too short to substring → demoted From eeb3f206cacd7ec742327090be8fb396fe0f4f3b Mon Sep 17 00:00:00 2001 From: zaebee Date: Sat, 13 Jun 2026 12:37:55 +0000 Subject: [PATCH 3/5] refactor(guardian): clearer None-line fallback + explicit-None diff_content (#243 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gemini re-review (medium, readability): replace the convoluted `(finding.line or matches[0])` lambda fallback with an explicit `if finding.line is None: return matches[0]`, and use `diff_content if diff_content is not None else {}` instead of `or {}`. Both behaviour-preserving. Bound the narrowed line to a local (`model_line`) so the min() closure type-checks under mypy strict (the closure doesn't narrow `finding.line` on its own). 1010 tests, mypy strict, ruff, doc 99.6% — green. Refs #181 Co-Authored-By: Claude Opus 4.8 --- src/cgis/guardian/github_poster.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/cgis/guardian/github_poster.py b/src/cgis/guardian/github_poster.py index 4ae6cad5..e3d964dc 100644 --- a/src/cgis/guardian/github_poster.py +++ b/src/cgis/guardian/github_poster.py @@ -37,9 +37,12 @@ def _anchored_line(finding: Finding, content: dict[int, str] | None) -> int | No matches = [n for n, text in content.items() if needle in text.strip()] if not matches: return None - if finding.line in matches: - return finding.line - return min(matches, key=lambda n: (abs(n - (finding.line or matches[0])), n)) + if finding.line is None: + return matches[0] + model_line = finding.line # bound local so the closure narrows it to int (mypy) + if model_line in matches: + return model_line + return min(matches, key=lambda n: (abs(n - model_line), n)) def build_review( @@ -62,7 +65,7 @@ def build_review( from its verbatim quote (#181) so the comment lands on the real line rather than the model's estimate; an unlocatable quote demotes to a body comment. """ - content_by_file = diff_content or {} + content_by_file = diff_content if diff_content is not None else {} inline: list[Finding] = [] outside: list[Finding] = [] for raw in visible_findings(result.findings): From 1749bf40f2400c85f352fafba84d3e1b4ff6c455 Mon Sep 17 00:00:00 2001 From: zaebee Date: Sat, 13 Jun 2026 12:52:28 +0000 Subject: [PATCH 4/5] fix(guardian): keep file-level findings file-level when anchoring (#243 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Colleague review note 2: a finding the model marked file-level (line=None) with no explicit `anchor` was promoted to an inline comment whenever its `evidence` text happened to appear in the diff. `evidence` is supporting text, not a positional signal — only an explicit `anchor` (or a model line) should drive placement. Now `_anchored_line` returns None early for line=None + no-anchor, so the note stays in the body; an explicit anchor still positions a lineless finding. 2 tests (file-level stays file-level; explicit anchor still places a lineless finding). Notes 1 (over-demotion) and 3 (keyword substring) addressed in the PR discussion — strict demotion is deliberate: on a new/rewritten file every line is commentable, so falling back to the model line there would re-open #181. 1012 tests, mypy strict, ruff, doc 99.6% — all green. Refs #181 Co-Authored-By: Claude Opus 4.8 --- src/cgis/guardian/github_poster.py | 7 +++++++ tests/unit/test_guardian_poster.py | 24 ++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/src/cgis/guardian/github_poster.py b/src/cgis/guardian/github_poster.py index e3d964dc..12d2b7ad 100644 --- a/src/cgis/guardian/github_poster.py +++ b/src/cgis/guardian/github_poster.py @@ -25,7 +25,14 @@ def _anchored_line(finding: Finding, content: dict[int, str] | None) -> int | No - among the candidates: the model's ``line`` if it is one, else the nearest; - quote present but located nowhere → ``None``, demoting to a body comment instead of a confidently-wrong inline anchor. + + A finding the model marked file-level (``line is None``) with no explicit + ``anchor`` stays file-level: ``evidence`` is supporting text, not a + positional signal, so it must not promote a file-level note to an inline + comment at a coincidental textual match (#243 review). """ + if finding.line is None and finding.anchor is None: + return None quote = (finding.anchor or finding.evidence or "").strip() if not content or not quote: return finding.line diff --git a/tests/unit/test_guardian_poster.py b/tests/unit/test_guardian_poster.py index 3014b02b..715b93dd 100644 --- a/tests/unit/test_guardian_poster.py +++ b/tests/unit/test_guardian_poster.py @@ -183,3 +183,27 @@ def test_trivial_anchor_does_not_substring_match() -> None: result, diff_index={"src/x.py": {12}}, skeptic_model=None, diff_content=content ) assert comments == [] # no exact ')' line, too short to substring → demoted + + +def test_file_level_finding_stays_file_level() -> None: + """A file-level finding (line=None, no anchor) is NOT promoted to inline just + because its evidence text appears somewhere in the diff (#243 review).""" + content = {"src/x.py": {10: " added1", 11: " added2"}} + # evidence quotes a real diff line, but the model said this is file-level + f = _finding(line=None, anchor=None, evidence="added2") + result = ReviewResult(findings=[f], summary="s") + _, comments = build_review( + result, diff_index={"src/x.py": {10, 11}}, skeptic_model=None, diff_content=content + ) + assert comments == [] # stays in the body, not anchored to line 11 + + +def test_explicit_anchor_still_places_a_lineless_finding() -> None: + """line=None but an explicit anchor → the model opted into positioning, so anchor wins.""" + content = {"src/x.py": {10: " added1", 11: " added2"}} + f = _finding(line=None, anchor="added2") + result = ReviewResult(findings=[f], summary="s") + _, comments = build_review( + result, diff_index={"src/x.py": {10, 11}}, skeptic_model=None, diff_content=content + ) + assert comments[0]["line"] == 11 From 1eea25bf71a08810f44bb97b3ce1ad322eab76b2 Mon Sep 17 00:00:00 2001 From: zaebee Date: Sat, 13 Jun 2026 12:59:39 +0000 Subject: [PATCH 5/5] fix(guardian): raise substring-anchor threshold to 10 (#243 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gemini caught an off-by-one: _MIN_SUBSTRING_ANCHOR=5 with `len >= 5` actually ALLOWED 5-char keywords (`else:`, `self.`, `break`, `print`) to substring-match — contradicting the docstring that listed `else:` as excluded. `else:` collides with `something_else:`, `self.` with `myself.foo`. Bumped to 10 (a real anchor is a full statement, comfortably longer) and corrected the docstring. Exact-match is unaffected, so full short lines still anchor; only the partial-substring fallback tightens. +1 test: a 5-char `else:` anchor no longer latches onto `something_else:`. 1013 tests, mypy strict, ruff, doc 99.6% — green. Refs #181 Co-Authored-By: Claude Opus 4.8 --- src/cgis/guardian/github_poster.py | 9 ++++++--- tests/unit/test_guardian_poster.py | 12 ++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/cgis/guardian/github_poster.py b/src/cgis/guardian/github_poster.py index 12d2b7ad..7d816195 100644 --- a/src/cgis/guardian/github_poster.py +++ b/src/cgis/guardian/github_poster.py @@ -7,9 +7,12 @@ from cgis.guardian.render import render_inline_comment, render_review_body from cgis.guardian.skeptic import visible_findings -#: Below this length a quote is too generic to substring-match safely (``)``, -#: ``else:``) — only an exact line equality may anchor it (#181 review). -_MIN_SUBSTRING_ANCHOR = 5 +#: Below this length a quote is too generic to substring-match safely — short +#: keywords/patterns (``)``, ``else:``, ``self.``, ``return``) would collide with +#: unrelated lines (``something_else:``, ``myself.foo``). Such a quote may only +#: anchor via an EXACT line equality, never a substring (#181 review). A real +#: anchor is a full statement, comfortably longer than this. +_MIN_SUBSTRING_ANCHOR = 10 def _anchored_line(finding: Finding, content: dict[int, str] | None) -> int | None: diff --git a/tests/unit/test_guardian_poster.py b/tests/unit/test_guardian_poster.py index 715b93dd..8a43c3c4 100644 --- a/tests/unit/test_guardian_poster.py +++ b/tests/unit/test_guardian_poster.py @@ -207,3 +207,15 @@ def test_explicit_anchor_still_places_a_lineless_finding() -> None: result, diff_index={"src/x.py": {10, 11}}, skeptic_model=None, diff_content=content ) assert comments[0]["line"] == 11 + + +def test_short_keyword_anchor_does_not_substring_match() -> None: + """A 5-char keyword like 'else:' must not substring-match 'something_else:' — + only exact line equality may anchor below _MIN_SUBSTRING_ANCHOR (#243 review).""" + content = {"src/x.py": {5: " something_else: int = 1"}} + f = _finding(line=99, anchor="else:") # 5 chars; substring of the line but generic + result = ReviewResult(findings=[f], summary="s") + _, comments = build_review( + result, diff_index={"src/x.py": {5}}, skeptic_model=None, diff_content=content + ) + assert comments == [] # demoted: too short to substring, no exact match