Skip to content

orb(review): fix addedLinesFromPatch dropping blank context lines — blocker anchors drift #9663

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

src/review/inline-comments-select.ts and src/review/inline-suggestion-anchor.ts contain two
parallel unified-diff line-number walkers over the same payload.patch string:

  • rightSideLinesFromPatch (src/review/inline-comments-select.ts:12-38) — RIGHT-side commentable lines.
  • addedLinesFromPatch (src/review/inline-suggestion-anchor.ts:10-26) — ADDED (+) lines only.

#9076 fixed a line-number desync in the first one and documented it inline
(src/review/inline-comments-select.ts:31-36):

an EMPTY patch line (marker undefined) is a context line whose single space was stripped, not a
line that does not exist. Skipping it without advancing right desynchronized every subsequent line
number in the file, so findings anchored after it pointed somewhere else entirely.

The second walker still has the pre-#9076 code:

const marker = raw[0];
if (marker === undefined || marker === "-" || marker === "\\") continue;   // inline-suggestion-anchor.ts:21
if (marker === "+") lines.add(right);
right += 1;

That matters more now, not less, because the same #9076 change made addedLinesByPath the
authoritative anchor validator for blocker-severity inline findings
(src/review/inline-comments-select.ts:103):

const validLines = finding.severity === "blocker" ? addedLines.get(finding.path) : rightLinesByPath.get(finding.path);

addedLinesFromPatch also feeds isSuggestionAnchorable
(src/review/inline-suggestion-anchor.ts:41-52), which gates every committable suggestion block.

Worked example on the patch ["@@ -1,3 +1,3 @@", " one", "", "+three"] (a blank context line — the
exact shape #9076's own regression fixture uses): rightSideLinesFromPatch returns {1,2,3}, but
addedLinesFromPatch returns {2} instead of {3}. So a correctly-anchored blocker on line 3 is
silently dropped, and a blocker the model anchored on line 2 (a context line) is accepted and
posted — precisely the "telling a contributor their bug is on a line they did not write" failure
#9076 exists to prevent.

A second, smaller divergence: src/review/inline-comments-select.ts:20 pops the trailing empty
element a patch ending in a newline produces; addedLinesFromPatch has no equivalent, so a trailing
newline is counted as one extra line.

Requirements

  • addedLinesFromPatch must treat a zero-length patch line as a context line: count it toward right
    (i.e. fall through to right += 1) instead of continue-ing past it, exactly as
    rightSideLinesFromPatch does.
  • addedLinesFromPatch must drop a single trailing empty element produced by a patch ending in a
    newline before the walk, exactly as rightSideLinesFromPatch does at
    src/review/inline-comments-select.ts:18-20.
  • "-" and "\\" markers must keep their current handling (skipped, right not advanced).
  • No change to rightSideLinesFromPatch, isSuggestionAnchorable, or any caller signature.

⚠️ Required pattern: mirror rightSideLinesFromPatch in src/review/inline-comments-select.ts:12-38
line-for-line in its blank-line and trailing-artifact handling. What does NOT satisfy this issue:
deleting one of the two walkers and re-pointing callers (they compute different sets — added-only vs
added+context — and #9076 deliberately depends on that difference); "fixing" it by making blockers
validate against rightLines instead; adding a comment explaining the divergence rather than
removing it; or a new third helper.

Deliverables

  • addedLinesFromPatch in src/review/inline-suggestion-anchor.ts advances right for a
    zero-length patch line, asserted by a new named case in test/unit/inline-suggestion-anchor.test.ts
    that feeds the patch "@@ -1,3 +1,3 @@\n one\n\n+three" and expects Set{3} (today: Set{2}).
  • The same test file asserts a trailing-newline patch produces the identical set as the same patch
    without the trailing newline.
  • A named regression test asserts selectInlineComments (src/review/inline-comments.ts:124)
    posts a blocker anchored on the added line 3 of that patch and drops one anchored on the blank
    context line 2 — i.e. the end-to-end 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.
  • A test asserts isSuggestionAnchorable returns true for line 3 and false for line 2 on the
    same patch.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for
example fixing the blank-line branch (Deliverable 1) without also removing the trailing-artifact
divergence (Deliverable 2), or adding the unit test without the end-to-end regression test
(Deliverable 3) — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted, and vitest.config.ts's
coverage.include covers src/**/*.ts — both touched files are measured. Every changed conditional
needs both arms exercised: the blank-line branch (marker undefined) and the non-blank branch, the
trailing-empty-element pop firing and not firing, and the "+" vs non-"+" arms. The end-to-end
regression test in Deliverable 3 is required by name, not optional.

Expected Outcome

Both diff walkers agree on every line number for every patch shape. A blocker-severity inline finding
can no longer be posted on a context line, and a correctly-anchored blocker after a blank context line
can no longer be silently dropped — the #9076 guarantee actually holds on the path that enforces it.

Links & Resources

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions