fix: multiline comment invalidation (#12950)

Found issues during the process of invalidation of a multiline comment (link to #12582):
* Update a line in the middle of the comment
* Update/Delete the last line of the comment

No problem with:
* Deleting a line in the middle of the comment
* Update/Delete the first line of the comment

I added all these cases in the pull_review_test.go

### Tests for Go changes

- I added test coverage for Go changes...
  - [X] in their respective `*_test.go` for unit tests.
- I ran...
  - [X] `make pr-go` before pushing

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12950
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>

models/issues/comment.go

@@ -897,8 +897,43 @@ func (c *Comment) CheckLineRangeValid(ctx context.Context, repo *repo_model.Repo
 		}
 		anchorResolvedLine := anchorBlame.LineNumber
 
-		// A line that no longer resolves is treated as "modified but present" and tolerated; only a
+		// Catch a line changed by a later commit by comparing each range line's current content
-		// resolved line landing at an unexpected offset (lines inserted/removed inside the range) is a break.
+		// to the comment's Patch (its content at creation; lines the PR itself changed already match it).
+		// The Patch uses the comment's line coordinates, so trust it only
+		// when the anchor's recorded content equals its current content — otherwise fall back below.
+		if expected := git.PatchRightSideContent(c.Patch); len(expected) > 0 {
+			if headLines, ok := c.headFileLines(gitRepo, currentHead, anchorBlame.FilePath); ok {
+				lineAt := func(n uint64) (string, bool) {
+					if n >= 1 && n <= uint64(len(headLines)) {
+						return headLines[n-1], true
+					}
+					return "", false
+				}
+				anchorExpected, hasAnchor := expected[int64(c.UnsignedLine())]
+				anchorCurrent, hasCurrent := lineAt(anchorResolvedLine)
+				if hasAnchor && hasCurrent && anchorExpected == anchorCurrent {
+					trusted := true
+					for i := int64(1); i <= c.ExtraLinesCount; i++ {
+						exp, okExp := expected[int64(c.UnsignedLine())+i]
+						cur, okCur := lineAt(anchorResolvedLine + uint64(i))
+						if !okExp || !okCur {
+							trusted = false // mapping incomplete -> fall back to the offset-only check
+							break
+						}
+						if exp != cur {
+							return "invalid", nil // a range line was changed after the comment was made
+						}
+					}
+					if trusted {
+						return "valid", nil
+					}
+				}
+			}
+		}
+
+		// Fallback (offset-only): a line that no longer resolves is treated as "modified but present"
+		// and tolerated; only a resolved line landing at an unexpected offset (lines inserted/removed
+		// inside the range) is a break.
 		startLine := c.UnsignedLine()
 		for i := int64(1); i <= c.ExtraLinesCount; i++ {
 			blame, err := c.resolveLineAtHead(gitRepo, startLine+uint64(i), currentHead)
@@ -919,6 +954,25 @@ func (c *Comment) CheckLineRangeValid(ctx context.Context, repo *repo_model.Repo
 	return resultJSON == "valid", nil
 }
 
+// headFileLines reads the content of treePath at the given commit and returns its lines
+// (dropping the trailing empty element produced by a final newline). ok is false when the
+// file can't be read at head.
+func (c *Comment) headFileLines(gitRepo *git.Repository, head, treePath string) (lines []string, ok bool) {
+	commit, err := gitRepo.GetCommit(head)
+	if err != nil {
+		return nil, false
+	}
+	content, err := commit.GetFileContent(treePath, -1)
+	if err != nil {
+		return nil, false
+	}
+	lines = strings.Split(content, "\n")
+	if n := len(lines); n > 0 && lines[n-1] == "" {
+		lines = lines[:n-1]
+	}
+	return lines, true
+}
+
 // CodeCommentLink returns the url to a comment in code
 func (c *Comment) CodeCommentLink(ctx context.Context) string {
 	err := c.LoadIssue(ctx)

modules/git/diff.go

@@ -278,6 +278,53 @@ func CutDiffAroundLine(originalDiff io.Reader, line int64, old bool, numbersOfLi
 	return strings.Join(newHunk, "\n"), nil
 }
 
+// PatchRightSideContent parses a unified diff patch (such as a stored code-comment
+// Patch) and returns the content of each line on the right (new) side, keyed by its
+// new line number. Added ('+') and context (' ') lines are included; removed ('-')
+// lines and the "\ No newline at end of file" marker are skipped. It lets callers
+// recover the file content captured when a comment was created, to detect whether
+// the commented lines were changed afterwards.
+func PatchRightSideContent(patch string) map[int64]string {
+	result := make(map[int64]string)
+	scanner := bufio.NewScanner(strings.NewReader(patch))
+	scanner.Buffer(make([]byte, 0, 64*1024), 4*1024*1024)
+
+	var rightLine int64
+	inHunk := false
+	for scanner.Scan() {
+		line := scanner.Text()
+		if strings.HasPrefix(line, "@@") {
+			submatches := hunkRegex.FindStringSubmatch(line)
+			if submatches == nil {
+				inHunk = false
+				continue
+			}
+			for i, name := range hunkRegex.SubexpNames() {
+				if name == "beginNew" {
+					rightLine, _ = strconv.ParseInt(submatches[i], 10, 64)
+				}
+			}
+			inHunk = rightLine > 0
+			continue
+		}
+		if !inHunk || len(line) == 0 {
+			continue
+		}
+		switch line[0] {
+		case '+', ' ':
+			result[rightLine] = line[1:]
+			rightLine++
+		case '-', '\\':
+			// '-' is left-only; '\' is the "no newline at end of file" marker
+			break
+		default:
+			// a non-hunk line (e.g. the next "diff --git" header); stop this hunk
+			inHunk = false
+		}
+	}
+	return result
+}
+
 var ErrLineNotFound = errors.New("line not found in diff")
 
 type LinePlacement struct {

modules/git/diff_test.go

@@ -436,3 +436,47 @@ index 2d203fb..d0cb63f 100644
 		require.ErrorIs(t, err, ErrLineNotFound)
 	})
 }
+
+func TestPatchRightSideContent(t *testing.T) {
+	t.Run("empty", func(t *testing.T) {
+		assert.Empty(t, PatchRightSideContent(""))
+	})
+
+	t.Run("single hunk with a replacement", func(t *testing.T) {
+		patch := "diff --git a/file1.md b/file1.md\n" +
+			"--- a/file1.md\n" +
+			"+++ b/file1.md\n" +
+			"@@ -48,3 +48,3 @@\n" +
+			" Line 48\n" +
+			" Line 49\n" +
+			"-Line 50\n" +
+			"+Line 50--modified"
+		assert.Equal(t, map[int64]string{
+			48: "Line 48",
+			49: "Line 49",
+			50: "Line 50--modified",
+		}, PatchRightSideContent(patch))
+	})
+
+	t.Run("added lines shift the right side", func(t *testing.T) {
+		patch := "@@ -10,2 +10,4 @@\n" +
+			" ctx\n" +
+			"+added a\n" +
+			"+added b\n" +
+			" after"
+		assert.Equal(t, map[int64]string{
+			10: "ctx",
+			11: "added a",
+			12: "added b",
+			13: "after",
+		}, PatchRightSideContent(patch))
+	})
+
+	t.Run("removed lines do not consume right-side numbers", func(t *testing.T) {
+		patch := "@@ -5,3 +5,1 @@\n" +
+			"-gone 1\n" +
+			"-gone 2\n" +
+			" kept"
+		assert.Equal(t, map[int64]string{5: "kept"}, PatchRightSideContent(patch))
+	})
+}

tests/integration/pull_review_test.go

@@ -35,6 +35,7 @@ import (
 	"forgejo.org/modules/test"
 	issue_service "forgejo.org/services/issue"
 	"forgejo.org/services/mailer"
+	pull_service "forgejo.org/services/pull"
 	repo_service "forgejo.org/services/repository"
 	files_service "forgejo.org/services/repository/files"
 	"forgejo.org/tests"
@@ -2016,10 +2017,15 @@ func TestPullRequestCommentPlacement(t *testing.T) {
 			// Push a second commit that changes lines BEFORE the range (removing lines 1-10),
 			// which shifts the range but keeps it contiguous.
 			content = strings.Replace(content, "Line 1\nLine 2\nLine 3\nLine 4\nLine 5\nLine 6\nLine 7\nLine 8\nLine 9\nLine 10\n", "", 1)
-			tester.changeFile("file1.md", content)
+			newSHA := tester.changeFile("file1.md", content)
+
+			// Run the invalidation pass synchronously instead of waiting for the async
+			// goroutine, then check the comment is still valid.
+			pr, err := issues_model.GetPullRequestByIndex(t.Context(), tester.repo.ID, tester.pr.Index)
+			require.NoError(t, err)
+			require.NoError(t, pull_service.InvalidateCodeComments(t.Context(),
+				issues_model.PullRequestList{pr}, tester.user, tester.repo, newSHA))
 
-			// Wait a bit for async invalidation to run, then check the comment is still valid.
-			time.Sleep(2 * time.Second)
 			commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID})
 			assert.False(t, commentReloaded.Invalidated)
 
@@ -2038,6 +2044,92 @@ func TestPullRequestCommentPlacement(t *testing.T) {
 			}
 			tester.assertFilesChangedDiff(diff)
 		})
+
+		// Helper: comment on lines 48-50 (anchor 48, middle 49, last 50, all modified by the PR), then a
+		// later commit applies `secondCommit` to the file and we check the resulting invalidation state.
+		runRangeInvalidation := func(t *testing.T, secondCommit func(content string) string, wantInvalidated bool) {
+			tester := newPullRequestCommentPlacementTester(t)
+
+			content := tester.fileContent
+			content = strings.Replace(content, "Line 48\n", "Line 48--modified\n", 1)
+			content = strings.Replace(content, "Line 49\n", "Line 49--modified\n", 1)
+			content = strings.Replace(content, "Line 50\n", "Line 50--modified\n", 1)
+			tester.changeFile("file1.md", content)
+			tester.createPR()
+
+			comment := tester.multiLineCommentFromFilesChanged("file1.md", 48, 2)
+			assert.EqualValues(t, 48, comment.Line)
+			assert.EqualValues(t, 2, comment.ExtraLinesCount)
+			assert.False(t, comment.Invalidated)
+
+			newSHA := tester.changeFile("file1.md", secondCommit(content))
+
+			if wantInvalidated {
+				assert.EventuallyWithT(t, func(t *assert.CollectT) {
+					commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID})
+					assert.True(t, commentReloaded.Invalidated)
+				}, 5*time.Second, 50*time.Millisecond)
+			} else {
+				// Run the invalidation pass synchronously instead of waiting for the async
+				// goroutine, then assert the comment stayed valid.
+				pr, err := issues_model.GetPullRequestByIndex(t.Context(), tester.repo.ID, tester.pr.Index)
+				require.NoError(t, err)
+				require.NoError(t, pull_service.InvalidateCodeComments(t.Context(),
+					issues_model.PullRequestList{pr}, tester.user, tester.repo, newSHA))
+
+				commentReloaded := unittest.AssertExistsAndLoadBean(t, &issues_model.Comment{ID: comment.ID})
+				assert.False(t, commentReloaded.Invalidated)
+			}
+		}
+
+		t.Run("multi-line comment invalidated when a middle line is deleted by a later commit", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			runRangeInvalidation(t, func(content string) string {
+				return strings.Replace(content, "Line 49--modified\n", "", 1)
+			}, true)
+		})
+
+		t.Run("multi-line comment invalidated when a middle line content changes in a later commit", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			runRangeInvalidation(t, func(content string) string {
+				return strings.Replace(content, "Line 49--modified\n", "Line 49--changed-again\n", 1)
+			}, true)
+		})
+
+		t.Run("multi-line comment invalidated when the last line content changes in a later commit", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			runRangeInvalidation(t, func(content string) string {
+				return strings.Replace(content, "Line 50--modified\n", "Line 50--changed-again\n", 1)
+			}, true)
+		})
+
+		t.Run("multi-line comment not invalidated when a line outside the range changes", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			runRangeInvalidation(t, func(content string) string {
+				return strings.Replace(content, "Line 60\n", "Line 60--modified\n", 1)
+			}, false)
+		})
+
+		t.Run("multi-line comment invalidated when the last line is deleted by a later commit", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			runRangeInvalidation(t, func(content string) string {
+				return strings.Replace(content, "Line 50--modified\n", "", 1)
+			}, true)
+		})
+
+		t.Run("multi-line comment invalidated when the first (anchor) line content changes in a later commit", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			runRangeInvalidation(t, func(content string) string {
+				return strings.Replace(content, "Line 48--modified\n", "Line 48--changed-again\n", 1)
+			}, true)
+		})
+
+		t.Run("multi-line comment invalidated when the first (anchor) line is deleted by a later commit", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+			runRangeInvalidation(t, func(content string) string {
+				return strings.Replace(content, "Line 48--modified\n", "", 1)
+			}, true)
+		})
 	})
 }