fix(agent): stabilize review item fingerprints - #732
Conversation
Normalize position-dependent trailing line endings before hashing per-file diffs so unchanged files can be reused across resumed reviews.
|
|
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // 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") |
There was a problem hiding this comment.
[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:
| diffText := strings.TrimRight(d.Diff, "\r\n") | |
| diffText := d.Diff | |
| diffText = strings.TrimSuffix(diffText, "\n") | |
| diffText = strings.TrimSuffix(diffText, "\r") |
There was a problem hiding this comment.
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.
|
Hi @Linxiushen, thanks for your contributions! I noticed that you've submitted several PRs but haven't signed our Contributor License Agreement (CLA) yet. Unfortunately, we are unable to review or merge any PRs until the CLA is signed. Could you please sign it at your earliest convenience? Once completed, we'll be happy to proceed with the review. Thank you! |
3 participants
Description
Normalize trailing CR/LF delimiters before hashing per-file review diffs. The patch splitter can leave an extra line ending on whichever file is last in a multi-file patch, which previously changed that file's fingerprint and defeated --resume reuse even when its hunks were unchanged.
The normalization is limited to line-ending characters. Real unified-diff content lines keep their leading marker and remain fingerprint-significant. Existing sessions may incur one clean re-review across this change; fingerprints are stable after that transition.
Type of Change
How Has This Been Tested?
Checklist
Related Issues
Closes #718