pr-review: model confusion about line numbers and old/new parts of diffs

[dan-ri]

TL;DR

In the pr-review workflow the model frequently:

• Mentions line numbers that are incorrect
• Posts review comments on the wrong lines for what the comment is about
• Includes a Suggested change in a review comment that is completely wrong and unrelated to the comment
• Gets confused by diffs and:
  • Reports problems the PR fixes as problems that need to be fixed
  • Says updates need to be made that the PR already makes

Expected behavior

Review precision with respect to the current state of the branch and all line numbers.

Observed behavior

No response

Action YAML

# As in release v0.1.8

Log output

Additional information

No response

[leehagoodjames]

Hi @dan-ri, thanks for surfacing this - you are correct on the expected behavior.

There have been 4 changes to the PR review YAML in the last 24 hours. Can you please include the YAML (or version tag, ex v0.1.8) that you are using?

Can you please include any examples of this occurring? It would be helpful to for debugging.

Note: If you aren't using the latest (updated ~1h ago) version of review, can you copy the /review workflow from https://github.com/google-github-actions/run-gemini-cli/blob/main/examples/workflows/pr-review/gemini-pr-review.yml and let me know if it resolves any of the issues you are seeing?

[dan-ri]

Yes, this is with v0.1.8.

[leehagoodjames]

Thanks for confirming @dan-ri. Can you please provide some info to help debug in the form of:

• Logs including where the agent is planning to post the review
• Screenshots of how this appears on GitHub in the https://github.com/<OWNER>/<REPO>/pull/<PR_NUMBER>/files UI?

[dan-ri]

I put some work into prompt engineering for this and it's made things significantly better. In the process, though, I've rewritten most of the prompt and configured the action to read it from a separate markdown file.

[leehagoodjames]

@dan-ri that is great to hear. Please feel free to suggest that we include these changes into the main branch

[hackertronix]

@leehagoodjames I am running into the same on v0.1.22

Here's the log from the agent

Data collection is disabled.
Okay, I'm ready to start the code review. I'll begin by gathering the necessary information about the pull request.Got it. Repo is XXXX/XXXX-Android. Now, PR data.PR "Remove RawDayOfWeek usage from ProductivitySettingsFragment and friends" from branch prateek/date-api-cleanup-pt3 has 4 changed files, 40 additions, 35 deletions. Now, getting changed files list.Okay, changed files are changelog/prateek.date-api-cleanup-pt3.txt, XXXX-app-viewmodel/src/main/kotlin/com/XXXX/viewmodel/ProductivitySettingsViewModel.kt, XXXX-app/src/main/java/com/XXXX/fragment/ProductivitySettingsFragment.kt, and XXXX-app/src/main/java/com/XXXX/preference/IgnoreDaysPreference.kt. Ignoring the changelog. Getting PR number now.PR 9628. Checking for instructions.No extra instructions. I'll get the PR diff now.I've reviewed the diff; the PR's goal of replacing RawDayOfWeek with kotlinx.datetime.DayOfWeek is a positive change. The updates in ProductivitySettingsViewModel.kt, ProductivitySettingsFragment.kt, and IgnoreDaysPreference.kt are consistent and correct. I have minor suggestions for improvement and will now create the pending review with my comments.Okay, I'm adding a comment to the pending review for XXXX-app-viewmodel/src/main/kotlin/com/XXXX/viewmodel/ProductivitySettingsViewModel.kt. I'll suggest simplifying the DayOfWeek conversion from the verbose ignoreDays.map { JavaDayOfWeek.of(it.toCalendarDay().calendarWeekday) }.toSet() to the more concise ignoreDays.map { JavaDayOfWeek.of(it.value) }.toSet(), since both kotlinx.datetime.DayOfWeek and java.time.DayOfWeek use the same 1-7 mapping for days of the week.I'll skip commenting on the let block in ProductivitySettingsFragment.kt as it's a style preference.

In IgnoreDaysPreference.kt, I'll suggest simplifying the getViewIdForWeekday method. The current implementation uses a custom toXXXXDayOfWeek() extension function. Since kotlinx.datetime.DayOfWeek is an enum with constants defined in order from Monday to Sunday, I can use the ordinal property for a cleaner day-of-week calculation. The logic (weekday.ordinal - startDay.ordinal + 7) % 7 is equivalent and avoids the custom function. I will add a comment suggesting this refactor.Comments added. Submitting review. The PR's refactoring to kotlinx.datetime.DayOfWeek is a solid improvement. My minor suggestions focus on conciseness and better use of standard library features. Submitting now.(node:2031) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 11 abort listeners added to [AbortSignal]. MaxListeners is 10. Use events.setMaxListeners() to increase limit
(Use node --trace-warnings ... to show where the warning was created)
Review submitted. Done.

[dan-ri]

The root cause is that LLMs are poor at parsing diffs. The correct fix is to preprocess diffs and provide them to the model in a format that allows it to easily pick line number and side information instead of trying to compute it.