Skip to content

fix(agent): stabilize review item fingerprints #732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lizhengfeng101 merged 2 commits into from
Aug 14, 2026
+34 −1
Merged

fix(agent): stabilize review item fingerprints #732

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0038643
fix(agent): stabilize review item fingerprints
Linxiushen Aug 5, 2026
86fd323
Merge branch 'main' into fix/stable-review-fingerprints
lizhengfeng101 Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion internal/agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,11 @@ func (a *Agent) reviewMode() string {
}

func reviewItemFingerprint(mode string, d model.Diff) string {
sum := sha256.Sum256([]byte(mode + "\x00" + d.OldPath + "\x00" + d.NewPath + "\x00" + d.Diff))
// The patch splitter can leave extra line endings on the final file in a
// multi-file patch. Unified diff content lines always carry a marker, so
// trimming CR/LF here removes only that position-dependent delimiter.
diffText := strings.TrimRight(d.Diff, "\r\n")

@github-actions github-actions Bot Aug 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[bug · low]
Potential issue: strings.TrimRight strips all trailing characters in the cutset (\r, \n), not just one trailing newline sequence. If the diff legitimately contains trailing blank lines (e.g., a context line that is empty, or a "\ No newline at end of file" scenario where the final line content itself ends with \r or \n), those would also be stripped — collapsing distinct diffs into the same fingerprint.

Consider using a more precise trim that only removes a single trailing line ending, e.g. strings.TrimSuffix applied once for \r\n then \n, to match what the comment describes ("removes only that position-dependent delimiter").

Suggestion:

Suggested change
diffText := strings.TrimRight(d.Diff, "\r\n")
diffText := d.Diff
diffText = strings.TrimSuffix(diffText, "\n")
diffText = strings.TrimSuffix(diffText, "\r")

@Linxiushen Linxiushen Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping TrimRight here intentionally. In a unified diff, every payload line carries a marker: context lines start with a space, additions/deletions with +/- and the no-newline marker with a backslash. A real empty trailing context line is therefore represented as a space-bearing line, never as bare CR/LF bytes. Any run of bare line endings after the final marked line is splitter delimiter noise, and trimming only one would reintroduce the cross-file-position instability this change fixes. The regression test covers both sides: multiple bare trailing newlines normalize to the same fingerprint, while a real empty context line (newline + space) changes it.

sum := sha256.Sum256([]byte(mode + "\x00" + d.OldPath + "\x00" + d.NewPath + "\x00" + diffText))
return fmt.Sprintf("%x", sum)
}

Expand Down
29 changes: 29 additions & 0 deletions internal/agent/agent_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,35 @@ func TestFilterLargeDiffs_ZeroMaxTokens(t *testing.T) {
}
}

func TestReviewItemFingerprintIgnoresTrailingLineEndings(t *testing.T) {
base := model.Diff{
OldPath: "main.go",
NewPath: "main.go",
Diff: "@@ -1 +1 @@\n-old\n+new",
}
want := reviewItemFingerprint(session.ReviewModeRange, base)

for name, suffix := range map[string]string{
"lf": "\n",
"crlf": "\r\n",
"extra blank line": "\n\n",
} {
t.Run(name, func(t *testing.T) {
d := base
d.Diff += suffix
if got := reviewItemFingerprint(session.ReviewModeRange, d); got != want {
t.Errorf("fingerprint = %q, want %q", got, want)
}
})
}

withContextLine := base
withContextLine.Diff += "\n "
if got := reviewItemFingerprint(session.ReviewModeRange, withContextLine); got == want {
t.Error("fingerprint ignored a real trailing context line")
}
}

func TestApplyResumeReusesCompletedItemsAcrossModels(t *testing.T) {
diffs := []model.Diff{
{OldPath: "a.go", NewPath: "a.go", Diff: "+a", Insertions: 1},
Expand Down
Loading