Skip to content

fix(github): use committer date for incremental review commit range after rebase - #3417

Merged
IsmaelMartinez merged 1 commit into
The-PR-Agent:mainfrom
utsab345:fix/incremental-rebase-committer-date
Sep 16, 2026
Merged

IsmaelMartinez merged 1 commit into
The-PR-Agent:mainfrom
utsab345:fix/incremental-rebase-committer-date

Conversation

@utsab345

Copy link
Copy Markdown
Contributor

References to other Issues or PRs

Fixes #3407. Supersedes #2844.

Brief description of what is fixed or changed

/review -i compares commit dates against the previous review timestamp to decide what changed. get_commit_range in the GitHub provider used the author date, which git preserves across a rebase. After a force-push the range therefore came back empty and the incremental review reported "no files changed" even though the code did change.

This change:

  • Prefers the committer date (advances on rebase), falling back to the author date when no committer is present. This matches what _GitLabIncrementalCommit already does on the GitLab side.
  • Adds a guard for a fully rebased branch: when every commit post-dates the previous review there is no baseline commit to diff against, so the review falls back to a full review instead of silently diffing against a None reference.

Validation

Five regression tests in test_github_provider_incremental.py:

  • rebased commit (older author date, newer committer date) is detected as new
  • commit older than the review is excluded
  • author-date fallback when committer is missing
  • fully rebased branch falls back to a full review
  • mixed commit range returns only commits after the review

Full unit suite: 8565 passed, 33 skipped, 1 xfailed. Ruff and all applicable pre-commit hooks pass on the changed files.

…fter rebase

The author date is preserved across rebases, so after a force-push the
incremental review range appears empty and the review silently skips
changed code. Switch to the committer date which advances on rebase,
with an author-date fallback for commits that lack a committer (matching
the GitLab adapter). Add a guard for fully rebased branches where every
commit post-dates the previous review and no baseline commit is
available; fall back to a full review instead of diffing against a None
reference.

Fixes The-PR-Agent#3407.
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Fix incremental GitHub reviews after branch rebases

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Uses committer dates to detect rebased commits in GitHub incremental reviews.
• Falls back to full reviews when rebases leave no baseline commit.
• Adds regression coverage for date fallback and mixed commit ranges.
Diagram

graph TD
    A["Previous Review"] --> B["GitHub Provider"] --> C["Commit Metadata"] --> D["Timeline Date"] --> E{"Baseline Exists?"}
    E -->|Yes| F["Incremental Review"]
    E -->|No| G["Full Review"]
Loading
High-Level Assessment

The current approach is the best scoped fix: committer dates reflect rebases, while author-date fallback preserves compatibility with incomplete commit metadata and matches GitLab behavior. Persisting reviewed commit SHAs or computing ancestry could avoid timestamp comparisons, but would require broader state-format and provider changes disproportionate to this bug.

Files changed (2) +91 / -1

Bug fix (1) +18 / -1
github_provider.pyAnchor incremental ranges on committer dates +18/-1

Anchor incremental ranges on committer dates

• Selects the committer timestamp for incremental range classification, with an author timestamp fallback. When every commit post-dates the prior review and no baseline commit exists, it disables incremental mode so the provider performs a full review.

pr_agent/git_providers/github_provider.py

Tests (1) +73 / -0
test_github_provider_incremental.pyCover rebased incremental commit selection +73/-0

Cover rebased incremental commit selection

• Adds five regression tests covering rebased commits, older commits, missing committer metadata, fully rebased branches, and mixed reviewed/new commit ranges.

tests/unittest/test_github_provider_incremental.py

@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (2) 📎 Requirement gaps (0) 🎨 UX issues (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. New date lookup mixes quote styles 📘 Rule violation ⚙ Maintainability
Description
_commit_timeline_date adds 'committer' and 'date' with single quotes even though this module
predominantly uses double quotes for simple literals. A later edit to the same lookup has
conflicting local examples and must guess which style to preserve.
Code

pr_agent/git_providers/github_provider.py[244]

+        committer_date = getattr(getattr(commit.commit, 'committer', None), 'date', None)
Evidence
Compliance rule 2694657 requires each Python file to use its predominant quote style for simple
literals. The added lookup on line 244 introduces two single-quoted attribute names while nearby and
predominant module literals use double quotes.

Rule 2694657: Use a single quote style per file for Python string literals
pr_agent/git_providers/github_provider.py[240-245]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new date lookup uses single-quoted string literals in a module that predominantly uses double quotes.
## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[244-244]
## Recommended Fix
Change the `getattr` attribute-name literals from `'committer'` and `'date'` to `"committer"` and `"date"` without altering the fallback behavior.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Three behavior comments are narrative 📘 Rule violation ⚙ Maintainability
Description
The fallback block at lines 220-222 and the test comments at lines 27 and 51 describe behavior as
narrative statements rather than imperative instructions. Contributors extending these rebase cases
now encounter three new examples using a different voice from the required comment convention.
Code

pr_agent/git_providers/github_provider.py[R220-222]

+                # Every commit post-dates the review (e.g. the branch was fully rebased), so there
+                # is no baseline commit to diff against. Fall back to a full review rather than
+                # diffing against a None ref, which silently yields empty original content.
Evidence
Compliance rule 2694688 requires newly added behavior comments to use imperative phrasing. The
production fallback explanation and two regression-test explanations begin with declarative subjects
and narrate behavior instead.

Rule 2694688: Docstrings and comments must use imperative phrasing
pr_agent/git_providers/github_provider.py[220-222]
tests/unittest/test_github_provider_incremental.py[27-27]
tests/unittest/test_github_provider_incremental.py[51-51]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Three added comments narrate rebase behavior instead of expressing their explanations as imperative instructions.
## Fix Focus Areas
- pr_agent/git_providers/github_provider.py[220-222]
- tests/unittest/test_github_provider_incremental.py[27-27]
- tests/unittest/test_github_provider_incremental.py[51-51]
## Recommended Fix
Rewrite the production comment as an imperative such as `Handle a fully rebased branch by falling back to a full review when no baseline exists`. Rewrite the test comments with imperative openings such as `Verify` or `Model`, preserving their technical meaning and the 120-character line limit.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


  • Author self-review: I have reviewed the code review findings, and addressed the relevant ones.

Grey Divider

Tip of the day
💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@IsmaelMartinez IsmaelMartinez left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for picking up the help-wanted issue within the hour; this is the fix #3407 asked for.

Verified: the rebased-commit and fully-rebased cases go red against main and green here, the author-date fallback matches the GitLab adapter, and the full suite is clean on today's main.

Qodo's two notes are style only (quote style on line 244, comment voice) and do not need a change before this lands.

@IsmaelMartinez
IsmaelMartinez merged commit fa06013 into The-PR-Agent:main Sep 16, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/review -i reports "no files changed" after a rebase or force-push

2 participants