Skip to content

fix(review): resolveInlineCommentAnchor's single-line fallback doesn't verify the start line is itself commentable #8352

Description

@JSONbored

Context

src/review/inline-comment-range.ts's resolveInlineCommentAnchor (lines 37-48) resolves the GitHub inline-comment anchor for an AI review finding. Its doc comment states: "Multi-line ONLY when every line in [start,end] is commentable on the RIGHT side; otherwise downgrade to the single start line (fail-safe, no 422)."

The fallback branch does not actually enforce that promise:

if (!validLines || !everyLineInSet(start, end, validLines)) {
  return { start, end: start, multiLine: false };
}

This returns a single-line anchor built from start unconditionally whenever the full range check fails — it never separately verifies validLines.has(start). So if start itself is not a commentable RIGHT-side line, the function still returns it as if it were a "safe, no 422" anchor. The function's own test ("downgrades when the file path is missing from the RIGHT-side line map") demonstrates this directly: calling it with an empty Map() returns {start: 1, end: 1, multiLine: false} for a path that was never validated at all.

Today this is safe only because the sole real caller chain (selectInlineComments in src/review/inline-comments.ts, via anchorableInlineFindings in src/review/inline-comments-select.ts) pre-filters findings to validLines.has(finding.line) before this function ever runs — an undocumented precondition this exported, independently-tested "pure" function doesn't itself enforce or even mention.

Requirements

  • Fix resolveInlineCommentAnchor in src/review/inline-comment-range.ts so its behavior matches its documented contract exactly: the fallback branch must also verify validLines.has(start) before returning a single-line anchor built from start.
  • Add an anchorable: boolean field to the function's return type ({ start: number; end: number; multiLine: boolean; anchorable: boolean }). Set it to false only when start itself is not a valid RIGHT-side commentable line (i.e. !validLines || !validLines.has(start)); true in every other returned case (both the existing single-line and multi-line success paths).
  • Update the caller chain — confirm via grep -rn resolveInlineCommentAnchor src/ that src/review/inline-comments-select.ts and src/review/inline-comments.ts are the only call sites — to check the new anchorable field and exclude any finding whose anchor comes back anchorable: false, instead of relying solely on its own separate pre-filter to guarantee this can never happen.
  • Do not change behavior for any finding whose start line IS valid — every existing passing case in test/unit/inline-comment-range.test.ts must keep passing with anchorable: true added to the expected result.

Deliverables

  • resolveInlineCommentAnchor returns anchorable: false (instead of a false "safe" single-line anchor) when start itself is not in validLines.
  • The RIGHT_LINES-consuming caller chain (inline-comments-select.ts / inline-comments.ts) updated to honor anchorable and exclude unanchorable findings from the rendered inline comments.
  • New test(s) in test/unit/inline-comment-range.test.ts asserting anchorable: false when start is invalid (e.g. resolveInlineCommentAnchor({path, line: 5, endLine: 10}, new Map([["path", new Set([20, 21, 22])]]))), and anchorable: true in the existing valid-single-line and valid-multi-line cases.

Test Coverage Requirements

99%+ Codecov patch coverage, branch-counted, on every changed line/branch in inline-comment-range.ts and the caller chain, per this repo's codecov.yml.

Expected Outcome

resolveInlineCommentAnchor's "fail-safe, no 422" guarantee is actually enforced by the function itself for every input, not only for inputs that happen to already satisfy an undocumented caller-side precondition.

Links & Resources

  • src/review/inline-comment-range.ts (resolveInlineCommentAnchor, lines 37-48)
  • src/review/inline-comments-select.ts (rightSideLinesFromPatch, anchorableInlineFindings)
  • src/review/inline-comments.ts (selectInlineComments)
  • test/unit/inline-comment-range.test.ts

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