fix(diff): never anchor a comment to an old-file line number

[PerryLink]

Fixes #1486

The problem

A comment matched on a deleted line reported an old-file line number with no side, and every consumer — JSON, SARIF, the GitHub Action, the VS Code extension — reads that number as a new-file line.

resolveFromHunk resolved the old-side pass through extractSideLines(hunk, false), which carries old-file numbers only, and LlmComment has no side field to tell them apart.

Approach

This follows the schema-compatible route suggested in the issue thread (thanks @chaojixinren) rather than adding a side field:

• A deleted-line match no longer publishes an old-file number. It stays 0 — the documented "unanchored" signal — which lets the file-content fallback try again.
• A context-line match publishes its own new-file number, since a context line exists on both sides.

Implementation: indexedLine gained newLineNum (0 for deleted lines); matchConsecutive was split so the matched index is available; newSideSpan() decides whether a whole span exists on the new side; and the old-side pass now continues to later hunks when a match cannot be anchored.

pages/src/content/docs/en/tools.md documents the resulting contract (only the en copy has that section).

Two judgement calls worth your review

1. Context-only old-side hits emit first_new..last_new. Across insertions this spans the inserted lines too: a 10-line insertion above a matched context block yields 5..7 rather than just 5. The anchor is correct; the range is wider than strictly necessary. If you'd rather emit a single line for context-only matches, that's a small change.
2. I changed 6 existing tests. Four of them (MultiLineHunkMatch, OldPathMapping, WhitespaceTolerant, TestResolveLineNumbers_SingleLineHunkMatch → renamed _DeletedLineIsNotAnchored) referenced deleted lines; I re-pointed them at added/replaced lines to preserve each test's original intent rather than flipping them to assert 0. You may prefer them to assert 0 directly — that's a defensible reading and I'm happy to switch.

Verification

go test ./internal/diff/...   -> ok
go test ./...                 -> 23 packages ok, 0 FAIL
make build                    -> exit 0 (dist/opencodereview, 78 MB)
make check                    -> license headers OK / no unapproved non-English text in 628 files / check passed
git diff --check              -> clean; three changed files contain zero CR bytes (LF preserved)

New tests include TestResolveLineNumbers_DeletedCodeBelowInsertions, which reproduces the issue's exact scenario (10 insertions above a deleted legacyCall()) and asserts 0 on both the hunk-only and NewFileContent paths, plus TestNewSideSpan_*.

Not run here: make test as written hardcodes -race, and this box has CGO_ENABLED=0 with no gcc (go: -race requires cgo). I ran the identical package set without -race. CI will run the real target. The committed code does not touch -race.

AI assistance (required disclosure)

Per CONTRIBUTING: this change was written with AI assistance. Tool used: DeepSeek Harness (dsh), an AI coding agent driving gh and a local checkout. It located the resolver path, implemented the fix, updated the tests and docs, and ran the commands above. The diff and every test expectation were reviewed before opening this PR.

No commits, messages, or trailers attribute this work to an AI: there is no Assisted-by or Co-developed-by, consistent with the project's policy.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s).

[chaojixinren]

Thanks for exploring the schema-compatible approach. I agree that resetting deleted-line coordinates to 0 prevents incorrect placement, but I think it is too lossy as the default behavior.

The resolver has already found a real location in the old file. Converting it to 0 makes a deleted-code finding indistinguishable from a comment that could not be resolved at all. In practice, it becomes summary/sidebar-only and loses navigation, highlighting, and the original review context.

An optional side field preserves backward compatibility: existing JSON without side can continue to mean RIGHT, while updated consumers can preserve LEFT coordinates. This is the approach implemented in PR #1519.

For #1486, I would prefer retaining the precise deleted-line location and carrying its side rather than making the finding unanchored. The current approach is a safe fallback, but it changes the behavior from “precisely located on the old side” to “location discarded.”

[PerryLink]

Thanks — and you are right that 0 is the wrong default.

Making an anchored finding indistinguishable from one that could not be resolved at all is a real
loss: it drops navigation, highlighting and the original review context for exactly the case the
resolver did resolve. I wrote it as a safety fallback (never point at an old-file line without a
side), but a fallback that discards a known-good location is not a conservative default — it is a
different defect wearing the same shape.

Your side field is the better shape, and #1519 is the complete version of it: JSON without side
still means RIGHT, and consumers that understand it get the LEFT coordinates back. That is both
backward compatible and lossless, which mine is not.

So I would rather not compete with #1519 on #1486. Two clean outcomes — your call:

1. I close fix(diff): never anchor a comment to an old-file line number #1518 and you land fix(diff): preserve diff side for review comment locations #1519. Happy to do that; it is your fix and it is the better one.
2. Keep fix(diff): never anchor a comment to an old-file line number #1518 as a narrow net for resolvers that genuinely cannot supply a side — in which case
   I will narrow it to exactly that case (fall back to 0 only when there is no side to carry), so
   the two do not overlap.

I will not push on this until you answer.

[PerryLink]

Thanks for the clear steer — and I agree with it.

My reasoning was that an anchor on a deleted line is worse than no anchor, because it places the comment somewhere the code no longer is. But you are right that 0 overshoots: the resolver had found a real old-file location, and collapsing it to 0 makes that finding indistinguishable from one that could not be resolved at all, losing navigation, highlighting, and the original review context along with it. Preserving the coordinates and carrying side keeps the information that was actually recovered, and the optional field keeps existing RIGHT-only consumers working. That is strictly better than what I proposed.

So #1518 is superseded, and I have closed it rather than leave a competing approach open. Since its three files are a subset of #1519's coverage there is nothing in it worth porting, but if any of the resolver tests or the internal/diff fixtures would be useful to you I am happy to hand them over — and if #1519 stalls and you want the fallback behaviour for the unresolvable case specifically, I can reopen this or open a narrower one.

One thing worth pinning whichever way it lands: after the change, a finding whose old-side location is genuinely unknown should still be distinguishable from one anchored on a deleted line. side does that for the resolvable case; the genuinely-unresolvable case presumably still needs its own representation, or it becomes the new 0.