Skip to content

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

Closed
Closed
pr-review: model confusion about line numbers and old/new parts of diffs#151
Assignees
MJjainam
Labels
area/qualityTracks quality issueskind/bugSomething isn't workingpriority/p1Important and should be addressed in the near term
Milestone
v0.1.19

Description

@ghost
ghost
opened on Aug 8, 2025

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

Activity

  1. gemini-cli
    added
    kind/bugSomething isn't working
    area/workflows
    area/quality
    on Aug 8, 2025

  2. leehagoodjames commented on Aug 8, 2025

    @leehagoodjames
    leehagoodjames
    on Aug 8, 2025
    Contributor

    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?

  3. leehagoodjames
    self-assigned this
    on Aug 8, 2025

  5. leehagoodjames commented on Aug 8, 2025

    @leehagoodjames
    leehagoodjames
    on Aug 8, 2025
    Contributor

    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?
  6. leehagoodjames
    added
    priority/p1Important and should be addressed in the near term
    and removed
    area/workflows
    area/quality
    on Aug 8, 2025

  8. leehagoodjames commented on Aug 14, 2025

    @leehagoodjames
    leehagoodjames
    on Aug 14, 2025
    Contributor

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

  9. hackertronix commented on Aug 19, 2025

    @hackertronix
    hackertronix
    on Aug 19, 2025

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

    Image Image

    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.

  10. jerop
    added
    area/qualityTracks quality issues
    on Aug 20, 2025

  12. 6 remaining items

  13. jerop
    added a parent issue on Sep 10, 2025
  14. jerop
    modified the milestones: v0.1.13, v0.1.14 on Sep 16, 2025
  15. jerop
    unassigned
    leehagoodjames
    on Sep 17, 2025

  16. michalszelagsonos commented on Oct 19, 2025

    @michalszelagsonos
    michalszelagsonos
    on Oct 19, 2025

    I've had really good success with a diff pre-processor approach. It takes a unified diff and creates a JSON based file that contains line numbers and chunks. I was seeing significant error rate before and now it is nailing exact numbers every time. I think this is the right approach. It is a python script. Would it make sense to convert it to JS given that this project seems to prefer it?

  17. jerop
    modified the milestones: v0.1.14, v0.1.15 on Oct 23, 2025
  18. MJjainam
    self-assigned this
    on Nov 17, 2025

  19. MJjainam commented on Nov 17, 2025

    @MJjainam
    MJjainam
    on Nov 17, 2025
    Contributor

    @dan-ri Is there a way to consistently reproduce this?

  20. michalszelagsonos commented on Nov 17, 2025

    @michalszelagsonos
    michalszelagsonos
    on Nov 17, 2025

    I noticed that certain PRs would produce much higher rates of errors although not always. I suspect you can probably find a PR that has a higher chance of showing this problem. Unfortunately I cannot share mine since they are in private repos.

  21. jerop
    removed this from the v0.1.15 milestone on Nov 18, 2025

  22. MJjainam commented on Dec 9, 2025

    @MJjainam
    MJjainam
    on Dec 9, 2025
    Contributor

    @michalszelagsonos Did you update Gemini CLI - this should be fixed in latest versions.

  23. MJjainam
    added this to the v0.1.18 milestone on Dec 10, 2025

  24. MJjainam commented on Dec 23, 2025

    @MJjainam
    MJjainam
    on Dec 23, 2025
    Contributor

    This is not reproducible with later models. If the issue persists, please re-open.

  25. MJjainam
    closed this as completedon Dec 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area/qualityTracks quality issueskind/bugSomething isn't workingpriority/p1Important and should be addressed in the near term

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions