fix(azure-devops): implement incremental review (-i) support (#2379)#2381
Conversation
Closes The-PR-Agent#2379. `pr-agent review -i` against an Azure DevOps PR crashed with `object of type 'NoneType' has no len()` because AzureDevopsProvider inherited the no-op `GitProvider.get_incremental_commits` stub, leaving `incremental.commits_range` as None. The hasattr guard in `_can_run_incremental_review` was satisfied by the inherited stub. Bug: - Add `commits_range is None` guard in `_can_run_incremental_review` so any provider without real incremental support degrades gracefully. - Defensive `previous_review.html_url` read in the no-new-files branch. Feature (AzureDevopsProvider): - Implement `get_incremental_commits`, `_get_incremental_commits`, `_get_commit_range`, `get_previous_review` mirroring GitHubProvider. - `_AzureCommitAdapter` exposes the GitHub-shape (.sha, .commit.author.date, .commit.message, .parents) so shared code in pr_reviewer/IncrementalPR needs no changes. - Reverse Azure's newest-first commit order to oldest-first to match GitHub iteration. - Normalize tz-aware UTC datetimes (Azure SDK) to naive UTC, matching PyGithub semantics and `pr_reviewer`'s naive `datetime.now()` compare. - Bridge Azure Comment to GitHub-shape attributes (`html_url` via existing `get_comment_url`, `created_at` from `published_date`). - Filter `get_diff_files` to `unreviewed_files_set` and rebuild patches against `last_seen_commit_sha` when incremental, so token savings actually materialize. - `get_files` override returns the unreviewed set when incremental. - Skip merge commits (multiple parents) when collecting changes. - Early-return in `get_diff_files` when `pr.last_merge_commit is None` instead of crashing on `head_sha.commit_id`. Tests: tests/unittest/test_azure_devops_incremental.py (9 cases) covers tz normalization, adapter shape, no-previous-review fallback, full incremental path, merge-commit skip, and the commits_range None guard.
Review Summary by QodoImplement incremental review support for Azure DevOps provider
WalkthroughsDescription• Fix crash in incremental review for Azure DevOps by adding commits_range is None guard • Implement full incremental review support for Azure DevOps provider - Add get_incremental_commits, _get_commit_range, get_previous_review methods - Create _AzureCommitAdapter to expose GitHub-compatible commit shape - Normalize tz-aware UTC datetimes to naive UTC for consistency • Filter diff files to unreviewed set and rebuild patches against last seen commit • Add defensive null checks for previous_review.html_url access Diagramflowchart LR
A["Azure DevOps PR"] -->|get_incremental_commits| B["Fetch commits & previous review"]
B -->|_AzureCommitAdapter| C["GitHub-compatible commit shape"]
C -->|_get_commit_range| D["Filter commits after last review"]
D -->|get_changes| E["Collect unreviewed files"]
E -->|get_diff_files| F["Build patches for new files only"]
F -->|pr_reviewer| G["Run incremental review"]
H["commits_range is None guard"] -->|fallback| I["Full review"]
File Changes1. pr_agent/git_providers/azuredevops_provider.py
|
Code Review by Qodo
1.
|
- Guard against None additional_properties when parsing commit changes. - Apply filter_ignored/is_valid_file when building unreviewed_files_set so it matches get_diff_files() filtering. - When commits_range is None, disable incremental and proceed with a full review instead of returning early.
|
Persistent review updated to latest commit d2b3b96 |
- get_previous_review() now selects the most recent matching PR-Agent review by published_date instead of returning the first match. The prior behavior could feed a stale timestamp into the commit-range computation when multiple reviews existed. - Replace set(candidate_paths) with dict.fromkeys(...) so incremental file-list dedup is order-preserving and deterministic across runs. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Persistent review updated to latest commit 63b58a6 |
… fallback - _can_run_incremental_review() now returns False (not True) when incremental.commits_range is None, satisfying compliance ID 6 and the test_can_run_returns_false_when_commits_range_none unit test. - PRReviewer.run() distinguishes "gate disabled incremental" (fall through to full review) from "gate said skip" (return early), so the previous fallback log line now produces an actual full review. - _get_incremental_commits() tracks had_errors so that transient Azure get_changes failures disable incremental and fall back to a full review instead of being silently skipped as "no new files". Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Persistent review updated to latest commit 4041c38 |
…map, polish - get_diff_files: incremental path now substitutes for the base-SHA original fetch instead of fetching base then overwriting, saving one Azure get_item call per modified file in incremental runs. - get_diff_files: cache guard switched to `is not None` so the early-return on missing last_merge_commit/last_merge_target_commit is cached (`[]` is falsy and was being recomputed each call). - Rename `unreviewed_files_set` -> `unreviewed_files_map` across Azure, GitHub, Gitea providers, the reviewer, and the unit test. The attribute has always been a dict (path -> path/patch); the old name was misleading. - Wrap get_previous_review max() lambda for readability. - Normalize filter_ignored(..., "azure") to double-quoted literal. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Persistent review updated to latest commit 55cd982 |
…eliable - _get_commit_range now returns None (not []) when the previous review has no timestamp, no PR commits exist, or every commit author date is None, and disables incremental so PRReviewer's existing `commits_range is None` fallback path runs a full review instead of silently exiting via the threshold check. - _get_incremental_commits short-circuits when _get_commit_range returns None, avoiding TypeError on the iteration that follows. - Wraps the over-120-char incremental-skip conditional in PRReviewer.run for ruff compliance. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Persistent review updated to latest commit 2690d27 |
|
Persistent review updated to latest commit 5cddd81 |
|
Any chance to get it merged? |
|
Hey @agonzalesipcoop-cmyk, |
- Qodo The-PR-Agent#7: _get_commit_range() could return a non-empty commits_range without setting last_seen_commit when every commit is newer than the previous review. get_diff_files() then can't rebuild the incremental diff (needs last_seen_commit_sha) and silently uses full diffs while still marked incremental. Now degrade explicitly to a full review. - Qodo The-PR-Agent#1: reset the cached diff_files when entering incremental mode, so a reused provider instance doesn't return a stale full-PR diff from a prior get_diff_files() call. Add regression tests for both (each fails on the pre-fix code, passes now).
|
Code review by qodo was updated up to the latest commit 9ece709 |
Qodo review (discussion r3518938542): _get_commit_range() dropped commits with a None author date that sit between the last-seen baseline and a later dated commit, because the range was derived from the first dated-new commit's index. Compute the range by slicing after the baseline index instead, so any commit positioned after the baseline — including ones the adapter left with author.date=None — is included. Preserves the full-review fallback when no baseline predates the previous review. Add a regression test (fails on the old logic, passes now).
| original_file_content_str = self._get_pr_file_content(file, self.incremental.last_seen_commit_sha) | ||
| patch = load_large_diff(file.filename, new_file_content_str, original_file_content_str) | ||
| self.unreviewed_files_set[file.filename] = patch | ||
| self.unreviewed_files_map[file.filename] = patch |
|
Code review by qodo was updated up to the latest commit f65aabe |
Clears CodeQL 'explicit returns mixed with implicit returns' (alert 139) by returning None explicitly after the search loop. Behaviour-neutral (the loop already fell through to an implicit None).
|
Code review by qodo was updated up to the latest commit 093af84 |
Summary
Closes #2379.
pr-agent review -iagainst an Azure DevOps PR crashed withobject of type 'NoneType' has no len()becauseAzureDevopsProviderinherited the no-opGitProvider.get_incremental_commitsstub, leavingincremental.commits_rangeasNone. Thehasattrguard in_can_run_incremental_reviewwas satisfied by the inherited stub and did not protect thelen(...)call.This PR fixes the crash and ships full incremental review support for Azure DevOps.
Bug fix —
pr_agent/tools/pr_reviewer.pycommits_range is Noneguard in_can_run_incremental_reviewso any provider without a real incremental implementation degrades gracefully (logs and falls back to full review).previous_review.html_urlread in the no-new-files skip branch.Feature —
pr_agent/git_providers/azuredevops_provider.pyget_incremental_commits,_get_incremental_commits,_get_commit_range,get_previous_reviewmirroringGitHubProvider._AzureCommitAdapterexposes the GitHub-shape (.sha,.commit.author.date,.commit.message,.parents) so the sharedIncrementalPRandpr_reviewercode paths need no changes.pr_reviewer's naivedatetime.now()comparison.Commentto GitHub-shape (html_urlvia existingget_comment_url,created_atfrompublished_date).get_diff_filestounreviewed_files_setand rebuild patches againstlast_seen_commit_shawhen incremental — so-iactually saves tokens, not just file count.get_filesoverride returns the unreviewed set when incremental.get_diff_fileswhenpr.last_merge_commit is Noneinstead of crashing onhead_sha.commit_id(issue point Fix encoding error on special_tokens #7).Approach note
Used the adapter at the seam approach (option B from the issue) rather than renaming shared
IncrementalPRattributes — keepspr_reviewer.pyandGitHubProvideruntouched.Test plan
tests/unittest/test_azure_devops_incremental.py— 9 new cases:_to_naive_utc(tz-aware → naive, naive passthrough, None passthrough)_AzureCommitAdaptershape + missing-author handling_get_incremental_commitswith no previous review (disables incremental)commits_range,first_new_commit,last_seen_commit,unreviewed_files_set, filtersgitObjectType == "tree", setshtml_url_can_run_incremental_reviewreturnsFalse(not crash) whencommits_range is Nonetests/unittest/test_azure_devops_parsing.pyandtests/unittest/test_azure_devops_comment.pystill pass (no regressions)## PR Reviewer Guidecomment; subsequentreview -ino longer crashes, finds the previous review, computes the unreviewed set, and either runs an incremental review or skips cleanly with a working[previous PR Review](...)link🤖 Generated with Claude Code