fix(guardian): anchor inline comments to a verbatim quote, not the model's line (#181)

[zaebee]

What

Closes #181. Guardian posted inline comments at whatever line the finder model emitted; for new/rewritten files every line is "commentable", so hallucinated coordinates passed the diff-index filter and landed on the wrong line (repro: PR #180, symbols.py:13 for an import actually at :3).

Implements the issue's fix #2 (content anchoring) plus the quote half of #1:

• Finding gains an optional anchor — the exact source line, copied verbatim by the finder.
• 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): model line trusted if it matches, else nearest match, and 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 + backward-compatible: with no diff_content, the model line is used as before.

Live calibration (mistral-medium-latest on PR #180)

Ran the real finder against the #180 diff with the new prompt, then anchored:

finding                                            model_line → anchored   verified new-side line
symbols.py  "from cgis.resolver.indices import …"      18 → 4              ✅ 4
engine.py   "self._resolver = SymbolResolver(…)"        34 → 31             ✅ 31
engine.py   "self._index = self._resolver.index"        35 → 32             ✅ 32
symbols.py  "self.index: SymbolIndex = IndexBuilder…"   25 → 23             ✅ 23

summary: corrected=4  demoted=0  unchanged=0  no_anchor=0

4 of 4 findings carried an anchor (prompt works) and 4 of 4 had a wrong model line — off by 14/3/2/2 — every one corrected to the verifiably-correct new-side line (cross-checked against diff_line_content). This is exactly the #181 failure class, now fixed end-to-end. The demotion path (hallucinated quote → body) is covered by unit test.

Tests

8 new (diff_line_content mapping, line correction, hallucinated-anchor demotion, legacy no-content path). make format lint type-check pytest doc-coverage green — 1008 tests, mypy strict, doc 99.6%.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a deterministic anchoring mechanism for inline review comments by matching a verbatim quote (the new anchor field or evidence) against the actual diff content, rather than relying solely on the model's line number prediction. If the quote cannot be located, the comment is safely demoted to a file-level body comment. The review feedback points out a potential issue in the matching logic where checking if a short line is a substring of the quote (s in needle) can lead to false positives on trivial lines (like a single closing parenthesis). It is recommended to simplify this to a one-way substring check (needle in s).

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[zaebee]

Code review — anchor inline comments to a verbatim quote (#181)

Reviewed as the one who filed #181 — checked out the head and dogfooded _anchored_line directly. The direction is right and the structure is clean: diff_line_index derived from diff_line_content (can't diverge), the demotion-to-body path for an unlocatable quote, backward-compat when no diff_content, and the live mistral calibration on #180 (4/4 wrong model lines corrected) is exactly the failure class. One real correctness issue in the matcher, though — and it reproduces the very #181 symptom inside the fix.

🔴 The bidirectional loose substring can prefer a FALSE match over the exact one

matches = [n for n, text in content.items() if (s := text.strip()) and (needle in s or s in needle)]
...
return min(matches, key=lambda n: (abs(n - (finding.line or matches[0])), n))

needle in s or s in needle is loose in both directions, and there's no preference for an exact match. So a short/common changed line (), else:, pass, a repeated return None) can loose-match an unrelated finding, and since the winner is "nearest to the model's line", a hallucinated model line can pull the anchor onto the false match. Dogfooded on the head:

S1  anchor exactly == line 4 ("…import IndexBuilder)"); line 20 is ")" (a substring
    of the anchor, so s-in-needle matches too); model hallucinated line 18
    → _anchored_line returns 20   (correct = 4)     ← lands a confidently-wrong inline comment

S3  anchor = ")"; line 12 is "qux()"  → needle-in-s matches → returns 12   (should demote → None)

S1 is precisely the #181 failure (off-by-N inline on the wrong line) re-created by the anchoring logic itself: the exact match at 4 loses to the spurious ) match at 20 because 20 is nearer the hallucinated 18. The live calibration didn't catch it only because those 4 anchors were long unique lines with no collision — real diffs full of short lines (), decorators, return None) are where it bites, and short lines are more commentable on new files (the exact case #181 is about).

Fix (cheap, on the existing structure): prefer exact strip-equality, fall back to substring only when there's no exact hit, and drop the s in needle direction (the realistic model imperfection is quoting a substring of the real line, i.e. needle in s, not the reverse). Roughly:

exact = [n for n, t in content.items() if t.strip() == needle]
loose = exact or [n for n, t in content.items() if needle and needle in t.strip()]
if not loose:
    return None
if finding.line in loose:
    return finding.line
return min(loose, key=lambda n: (abs(n - (finding.line or loose[0])), n))

That makes S1 → 4 and S3 → None (demote). A trivial-anchor guard (skip needles below a few chars) would harden it further.

🟢 Minor

• _anchored_line uses only the first non-empty line of a multi-line quote as the needle — fine, worth a one-word note since anchor is documented as "the exact source line" (singular) anyway.
• Duplicate exact lines (e.g. return None twice) are inherently ambiguous → nearest-to-model wins; acceptable, but with the exact-first fix at least it won't also lose to a substring false match.

The DRY index/content refactor and the demotion safety net are genuinely good. #1 is the one I'd fix before merge — it's a one-function change and it's the heart of the feature. 🚀

[zaebee]

Excellent catch — and a genuinely funny one: the loose matcher re-created a #181-class bug inside the #181 fix. Fixed in e468ff3.

You're exactly right that s in needle was the culprit and that "nearest to the model's line" let a hallucinated coordinate prefer the spurious ) over the exact match. Applied the fuller fix on the same structure rather than gemini's minimal needle in s (which fixes S1 but not S3):

• Exact stripped-line equality wins. Only with no exact hit do we fall back to substring.
• Substring fallback guarded by len(needle) >= 5 — so ) / else: / pass-ish trivial lines can't substring-match anything (closes S3: ) → demote, not latch onto qux()).
• Dropped s in needle entirely. A model quoting more than the real line now safely demotes to a body comment — nothing is mis-anchored (demotion is the safe direction, per the whole point of guardian: inline comment line numbers are model-estimated, not anchored to code #181).

Both your dogfooded scenarios are now regression tests:

• test_exact_match_beats_a_spurious_substring: exact line 4 wins over a nearer ) at 20 despite the model's hallucinated 18 → 4.
• test_trivial_anchor_does_not_substring_match: anchor ) with qux() in the diff → demoted to body.

The live-calibration result is unchanged (those 4 anchors were long unique lines → exact matches). 1010 tests, mypy strict, ruff, doc 99.6% green.

Re your minor notes: kept first-non-empty-line-of-the-quote as the needle (the anchor field is documented as a single line), and duplicate-exact-lines still resolve nearest-to-model — but now they can't lose to a substring false match. Thanks for the rigor — this is exactly the failure mode the feature exists to kill.

[zaebee]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a deterministic anchoring mechanism for inline review comments on GitHub. By extracting verbatim source line content from the git diff and matching it against a new anchor field in the Finding model, the system can correctly position comments even if the model's line number prediction is inaccurate. The review feedback suggests two improvements: using an explicit is not None check instead of the or operator when defaulting diff_content to prevent overriding an empty dictionary, and simplifying the fallback logic in _anchored_line when finding.line is None to enhance readability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[zaebee]

Both applied in eeb3f20 — thanks.

• _anchored_line None-fallback → explicit if finding.line is None: return matches[0], dropping the (finding.line or matches[0]) lambda trick. One tweak on top of your snippet: bound the narrowed value to a local (model_line) before the min() closure — mypy strict doesn't narrow finding.line inside a closure (it can't prove the frozen field is stable across the deferred call), so abs(n - finding.line) tripped int - None. The local fixes it and reads the same.
• diff_content if diff_content is not None else {} → applied; behaviour-identical here (an empty dict already degrades to legacy line-handling) but matches the "don't override an explicit empty collection" convention.

The original high-priority substring finding stays fixed (exact-first + length-guarded substring + no reverse). 1010 tests, mypy strict, ruff, doc 99.6% green.

[zaebee]

Colleague review — anchor inline comments to a verbatim quote (#181)

Verdict: 🟢 Approve, three low/discussion-level notes. Solid, well-tested. Checked the anchoring logic and the diff_line_content/diff_line_index refactor by hand; ran the poster + diff-index suites (19/19 green).

Verified ✅

• diff_line_index derived from diff_line_content — no divergence risk; both tested to yield identical key sets, same RIGHT-side accounting (added/context numbered, removed/\ skipped, +++ /dev/null excluded).
• Exact-match-first, then bounded substring is the right order. The _MIN_SUBSTRING_ANCHOR = 5 guard against trivial tokens (), else:) is well-targeted and pinned by test_exact_match_beats_a_spurious_substring / test_trivial_anchor_does_not_substring_match.
• Graceful demotion preserves the finding — test_hallucinated_anchor_demotes_to_body confirms an unlocatable quote drops to the body, nothing lost.
• Re-anchored line is always in diff_index by construction (both derive from the same content), so a successful anchor can never land on a line GitHub would reject.
• Backward compatible when diff_content is omitted (legacy model-line path). mypy-strict handled via the model_line local.

Notes

1. (low-medium, please confirm intended) Coverage tradeoff: a valid model line is demoted to body when the quote doesn't match.
runner now always passes diff_content, so every finding is re-anchored. When neither anchor nor evidence matches a verbatim line (model paraphrased, the offending construct spans lines, or the quote carries surrounding context) _anchored_line returns None → body — even when finding.line was valid and present in the diff. On main that finding posts inline at the model line. This conflates "model gave a real anchor that's wrong → demote" (good) with "model's line was fine, quote just didn't match → lost a correct inline" (regression). A safer variant: when the quote is unlocatable but finding.line is in diff_index, fall back to the model line instead of None. That keeps #181's win (a correct anchor still overrides) without shedding inline coverage. Fine to keep as-is if the stricter behaviour is the deliberate choice — just want it conscious.

2. (low) File-level findings (line=None) get silently promoted to inline. evidence is required and is a verbatim quote, so a genuinely file-level finding whose evidence text appears anywhere in the diff hits return matches[0] and becomes a line comment at the first textual match. line=None was the model's explicit "this is file-level" signal; anchoring it to the lowest matching line number is dubious. Consider returning None early when finding.line is None and finding.anchor is None (keep it file-level).

3. (low) Substring fallback can still mis-anchor on ≥5-char keywords. anchor/evidence like return/config/raise substring-matches multiple lines; the nearest-to-model_line tiebreak may pick the wrong one. Mitigated by exact-first and by the model usually quoting full lines — noting for completeness, not asking for a change.

All three are minor. Mergeable as-is; I'd just want note #1 acknowledged one way or the other.

[zaebee]

Thanks for the thorough pass — verdict-by-note:

Note 2 (file-level → inline): fixed in 1749bf4. You're right that evidence is supporting text, not a positional signal. _anchored_line now returns None early for line=None + no explicit anchor, so a genuinely file-level finding stays in the body; an explicit anchor still positions a lineless finding (model opted in). Two tests pin both directions.

Note 1 (over-demotion): keeping strict demotion — deliberate, and the suggested fallback re-opens #181. I dogfooded the proposed "if quote unlocatable but finding.line in diff_index, keep the model line" variant and it breaks on the exact case #181 cites. On a new/rewritten file every line 1..N is commentable:

new.py @@ -0,0 +1,5 @@  → commentable lines [1,2,3,4,5]

so finding.line in content is trivially true for any hallucinated line, and the fallback would keep it — re-creating the off-by-N inline #181 exists to kill. Since #181's headline degeneracy is new files, demotion-to-body is the safe direction. The finding isn't lost (it renders in the body); we only forgo a clickable anchor we can't trust. Conscious choice, as you offered.

Note 3 (keyword substring): accepting the residual risk — exact-first handles the common case, the len ≥ 5 guard + nearest-to-model tiebreak bound it, and the model quoting full lines makes a partial-keyword collision rare. Noting it in the docstring is enough; a fraction-of-line guard would be over-engineering for the observed failure rate.

1012 tests, mypy strict, ruff, doc 99.6% green. The DRY refactor + demotion net you verified are unchanged.

[zaebee]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a deterministic inline comment anchoring mechanism by mapping changed files to their line contents and using a verbatim anchor string from the model's findings to resolve the correct line number. This prevents misplaced inline comments caused by model line-number hallucinations. The review feedback points out that the minimum substring anchor length of 5 is too short and would allow generic terms like else: to match incorrectly, suggesting it be increased to 10.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[zaebee]

Fixed in 1eea25b — genuine off-by-one on my side. _MIN_SUBSTRING_ANCHOR=5 with len >= 5 did let else:/self./break (all 5) through the substring path, exactly contradicting the docstring that listed else: as excluded. Bumped to 10 (a real anchor is a full statement, well over 10 chars; exact-match still handles full short lines, so no coverage lost) and corrected the docstring. Added a regression test: else: no longer latches onto something_else:. 1013 tests green.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud