fix(review): count blank context lines in addedLinesFromPatch so blocker anchors don't drift - #9781
Conversation
…ker anchors don't drift `addedLinesFromPatch` (the authoritative anchor validator for blocker-severity inline findings since JSONbored#9076) still had the pre-JSONbored#9076 walker: it `continue`d past a zero-length patch line (a context line whose single leading space the split stripped) without advancing `right`, desyncing every later added-line number. On the patch `"@@ -1,3 +1,3 @@\n one\n\n+three"` it returned {2} instead of {3}, so a correctly-anchored blocker on the added line 3 was silently dropped and a blocker the model anchored on the blank *context* line 2 was accepted and posted — exactly the "your bug is on a line you did not write" failure JSONbored#9076 prevents. Mirror `rightSideLinesFromPatch` line-for-line: drop the trailing empty split artifact before the walk, and let a zero-length line fall through to `right += 1` (still not added, but counted as the context line it is). `"-"`/`"\\"` handling and the added-only set are unchanged. Closes JSONbored#9663
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 08:34:30 UTC
Review summary Nits — 4 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9781 +/- ##
===========================================
- Coverage 90.30% 76.60% -13.71%
===========================================
Files 912 283 -629
Lines 113555 59494 -54061
Branches 26955 8488 -18467
===========================================
- Hits 102544 45573 -56971
- Misses 9682 13639 +3957
+ Partials 1329 282 -1047
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Held for manual review: the gate and required CI are green, but GitHub reports this pull request's mergeable state as |
What & why
src/review/has two parallel unified-diff line-number walkers over the samepayload.patch:rightSideLinesFromPatch(RIGHT-side commentable lines) andaddedLinesFromPatch(ADDED+lines only). #9076 fixed a line-number desync in the first and documented it inline, butaddedLinesFromPatchstill had the pre-#9076 code — and since #9076 it is the authoritative anchor validator for blocker-severity inline findings (inline-comments-select.ts:103) and gates every committable suggestion viaisSuggestionAnchorable.The bug: a zero-length patch line (a context line whose single leading space the split stripped → marker
undefined) wascontinued past without advancingright, desyncing every later added-line number. On the patch"@@ -1,3 +1,3 @@\n one\n\n+three",rightSideLinesFromPatchreturns{1,2,3}butaddedLinesFromPatchreturned{2}instead of{3}. So a correctly-anchored blocker on the added line 3 was silently dropped, and a blocker the model anchored on the blank context line 2 was accepted and posted — exactly the "telling a contributor their bug is on a line they did not write" failure #9076 exists to prevent. A second divergence: no trailing-empty-element pop, so a patch ending in a newline counted one extra line.The fix
Mirror
rightSideLinesFromPatchline-for-line:rawLines.pop()when the last element is"").marker === undefinedfrom the skip condition so a zero-length line falls through toright += 1— still not added (only"+"adds), but counted as the context line it is."-"/"\\"handling and the added-only semantics are unchanged; no caller signature changes;rightSideLinesFromPatchandisSuggestionAnchorableuntouched.Both walkers now agree on every line number for every patch shape.
Tests
test/unit/inline-suggestion-anchor.test.ts(orb(review): fixaddedLinesFromPatchdropping blank context lines — blocker anchors drift #9663 suite): a zero-length context line advancesrightso"+three"is line 3 (was 2); a trailing-newline patch yields the same set as without;"-"/"\\"stay skipped;isSuggestionAnchorablereturnstruefor line 3 andfalsefor the blank context line 2.test/unit/inline-comments.test.ts(end-to-end, Deliverable 3):selectInlineCommentsposts a blocker anchored on the added line 3 and drops one anchored on the blank context line 2 — the orb(ai-review): prompt-injection defang collapses newlines, shifting every inline anchor after it onto the wrong line #9076 guarantee holds across a blank context line.Each behavior-changing case fails against
main(regression-proven). Diff coverage on the changed file is 100% line and branch.Validation
npm run typecheckgreen; the three inline suites green.git diff --check <base> HEADclean; diff is three files (one src + two tests), no route/schema/migration change.Closes #9663