Rework how we locate chunks in code - #6491
Open
padenot wants to merge 3 commits into
Open
Conversation
format_patch_set() repeated "Filename: X" before every hunk and had no --- / +++ / @@ headers, so the diff shown to the model didn't pattern-match a real unified diff. Add diff --git/---/+++/@@ headers (printed once per file) while keeping the per-line number column that lets the model anchor comments without counting from the hunk header.
Trusting the model to output an absolute code_line requires it to count from a hunk header; miscounting either silently mislocates the comment (find_comment_scope only checked the line fell inside some hunk, not that it was the right one) or, if the number falls outside every hunk, raises and kills every other comment in the batch too. Adopt the approach used by alibaba/open-code-review: replace code_line with existing_code, a verbatim quote of the line(s) the comment is about. find_comment_location() deterministically matches that quote against the patch's hunks (new side, then old side) to derive the real line range, instead of trusting model arithmetic. Comments that can't be matched are now dropped and logged rather than raising, so one bad comment no longer costs every other one. Fixes REVIEWHELPER-API-2C
Dropping a comment because its existing_code doesn't match anywhere in the patch was only a warning, easy to miss. Log it at error level instead so it surfaces in Sentry (already capturing ERROR+ logs in the services that embed this library) rather than only appearing in debug logs no one is watching.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At first (2024), regular diffs were used to display the diffs. This led to inaccuracies in comment reporting. This tool was then modified to print the line number at the beginning of each line.
My own experience has shown that LLM understand diffs very well, and more importantly, can barf if it's a diff that's not exactly a diff (notably in my case,
jj's diff format). This could lead to higher quality of review.It might well be that LLMs today don't make the same mistakes, and so we can just switch. This is likely: my own local review tools are always accurate when displaying their comments in my e.g. https://github.com/padenot/lrv/, and that uses git-style diffs.
However, looking around, I found that alibaba/open-code-review has a different technique, implemented here: they quote the code the comment is to be attached to, and use "real" git diffs. Reference:
https://github.com/alibaba/open-code-review/blob/main/internal/diff/hunk.go
https://github.com/alibaba/open-code-review/blob/main/internal/diff/relocation.go
While it's currently logged (https://mozilla.sentry.io/issues/7600128725), the last commit promotes this to an error.