Skip to content

Commit 55cd982

Browse files
fix(azure-devops): rm redundant base fetch, cache empty diff, rename 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>
1 parent 4041c38 commit 55cd982

5 files changed

Lines changed: 47 additions & 42 deletions

File tree

pr_agent/git_providers/azuredevops_provider.py

Lines changed: 34 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ def __init__(
7979
self.pr = None
8080
self.temp_comments = []
8181
self.incremental = incremental
82-
self.unreviewed_files_set = {}
82+
self.unreviewed_files_map = {}
8383
self.pr_commits = None
8484
self.previous_review = None
8585
if pr_url:
@@ -194,7 +194,7 @@ def get_incremental_commits(self, incremental=None):
194194
incremental = IncrementalPR(False)
195195
self.incremental = incremental
196196
if self.incremental.is_incremental:
197-
self.unreviewed_files_set = {}
197+
self.unreviewed_files_map = {}
198198
self._get_incremental_commits()
199199

200200
def _get_incremental_commits(self):
@@ -245,10 +245,10 @@ def _get_incremental_commits(self):
245245

246246
if candidate_paths:
247247
deduped = list(dict.fromkeys(candidate_paths))
248-
filtered = filter_ignored(deduped, 'azure')
248+
filtered = filter_ignored(deduped, "azure")
249249
for path in filtered:
250250
if is_valid_file(path):
251-
self.unreviewed_files_set[path] = path
251+
self.unreviewed_files_map[path] = path
252252
elif had_errors and self.incremental.commits_range:
253253
get_logger().warning(
254254
"Failed to fetch changes for incremental commits; falling back to full review."
@@ -287,7 +287,10 @@ def get_previous_review(self, *, full: bool, incremental: bool):
287287
matches.append(comment)
288288
if not matches:
289289
return None
290-
latest = max(matches, key=lambda c: _to_naive_utc(getattr(c, "published_date", None)) or _dt.datetime.min)
290+
latest = max(
291+
matches,
292+
key=lambda c: _to_naive_utc(getattr(c, "published_date", None)) or _dt.datetime.min,
293+
)
291294
latest.html_url = self.get_comment_url(latest)
292295
latest.created_at = _to_naive_utc(getattr(latest, "published_date", None))
293296
return latest
@@ -311,8 +314,8 @@ def get_repo_settings(self):
311314
def get_files(self):
312315
if (isinstance(getattr(self, "incremental", None), IncrementalPR)
313316
and self.incremental.is_incremental
314-
and self.unreviewed_files_set):
315-
return list(self.unreviewed_files_set.keys())
317+
and self.unreviewed_files_map):
318+
return list(self.unreviewed_files_map.keys())
316319
return self._get_files_full()
317320

318321
def _get_files_full(self):
@@ -335,7 +338,7 @@ def _get_files_full(self):
335338
def get_diff_files(self) -> list[FilePatchInfo]:
336339
try:
337340

338-
if self.diff_files:
341+
if self.diff_files is not None:
339342
return self.diff_files
340343

341344
if self.pr.last_merge_commit is None or self.pr.last_merge_target_commit is None:
@@ -403,7 +406,7 @@ def get_diff_files(self) -> list[FilePatchInfo]:
403406
# diffs = list(set(diffs))
404407

405408
diffs_original = diffs
406-
diffs = filter_ignored(diffs_original, 'azure')
409+
diffs = filter_ignored(diffs_original, "azure")
407410
if diffs_original != diffs:
408411
try:
409412
get_logger().info(f"Filtered out [ignore] files for pull request:", extra=
@@ -415,11 +418,11 @@ def get_diff_files(self) -> list[FilePatchInfo]:
415418
incremental_active = (
416419
isinstance(getattr(self, "incremental", None), IncrementalPR)
417420
and self.incremental.is_incremental
418-
and bool(self.unreviewed_files_set)
421+
and bool(self.unreviewed_files_map)
419422
and self.incremental.last_seen_commit_sha
420423
)
421424
if incremental_active:
422-
diffs = [f for f in diffs if f in self.unreviewed_files_set]
425+
diffs = [f for f in diffs if f in self.unreviewed_files_map]
423426

424427
invalid_files_names = []
425428
for file in diffs:
@@ -459,51 +462,53 @@ def get_diff_files(self) -> list[FilePatchInfo]:
459462
elif "rename" in diff_types[file]: # diff_type can be `rename` | `edit, rename`
460463
edit_type = EDIT_TYPE.RENAMED
461464

462-
version = GitVersionDescriptor(
463-
version=base_sha.commit_id, version_type="commit"
464-
)
465465
if edit_type == EDIT_TYPE.ADDED or edit_type == EDIT_TYPE.RENAMED:
466466
original_file_content_str = ""
467-
else:
467+
elif incremental_active:
468+
inc_version = GitVersionDescriptor(
469+
version=self.incremental.last_seen_commit_sha, version_type="commit"
470+
)
468471
try:
469-
original_file_content_str = self.azure_devops_client.get_item(
472+
inc_original = self.azure_devops_client.get_item(
470473
repository_id=self.repo_slug,
471474
path=file,
472475
project=self.workspace_slug,
473-
version_descriptor=version,
476+
version_descriptor=inc_version,
474477
download=False,
475478
include_content=True,
476479
)
477-
original_file_content_str = original_file_content_str.content
480+
original_file_content_str = inc_original.content or ""
478481
except Exception as error:
479-
get_logger().error(f"Failed to retrieve original file content of {file} at version {version}", error=error)
482+
get_logger().warning(
483+
f"Failed to retrieve original of {file} at {self.incremental.last_seen_commit_sha}: {error}"
484+
)
480485
original_file_content_str = ""
481-
482-
if incremental_active:
483-
inc_version = GitVersionDescriptor(
484-
version=self.incremental.last_seen_commit_sha, version_type="commit"
486+
else:
487+
base_version = GitVersionDescriptor(
488+
version=base_sha.commit_id, version_type="commit"
485489
)
486490
try:
487-
inc_original = self.azure_devops_client.get_item(
491+
base_original = self.azure_devops_client.get_item(
488492
repository_id=self.repo_slug,
489493
path=file,
490494
project=self.workspace_slug,
491-
version_descriptor=inc_version,
495+
version_descriptor=base_version,
492496
download=False,
493497
include_content=True,
494498
)
495-
original_file_content_str = inc_original.content or ""
499+
original_file_content_str = base_original.content
496500
except Exception as error:
497-
get_logger().warning(
498-
f"Failed to retrieve original of {file} at {self.incremental.last_seen_commit_sha}: {error}"
501+
get_logger().error(
502+
f"Failed to retrieve original file content of {file} at version {base_version}",
503+
error=error,
499504
)
500505
original_file_content_str = ""
501506

502507
patch = load_large_diff(
503508
file, new_file_content_str, original_file_content_str, show_warning=False
504509
).rstrip()
505510
if incremental_active:
506-
self.unreviewed_files_set[file] = patch
511+
self.unreviewed_files_map[file] = patch
507512

508513
# count number of lines added and removed
509514
patch_lines = patch.splitlines(keepends=True)

pr_agent/git_providers/gitea_provider.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ def __init__(self, url: Optional[str] = None):
6464
self.diff_files = []
6565
self.incremental = IncrementalPR(False)
6666
self.comments_list = []
67-
self.unreviewed_files_set = dict()
67+
self.unreviewed_files_map = dict()
6868

6969
if "pulls" in url:
7070
self.pr_url = url
@@ -475,9 +475,9 @@ def get_diff_files(self) -> List[FilePatchInfo]:
475475
# Get file content from this pr
476476
head_file = self.file_contents.get(filename,"")
477477

478-
if self.incremental.is_incremental and self.unreviewed_files_set:
478+
if self.incremental.is_incremental and self.unreviewed_files_map:
479479
base_file = self._get_file_content_from_latest_commit(filename)
480-
self.unreviewed_files_set[filename] = patch
480+
self.unreviewed_files_map[filename] = patch
481481
else:
482482
if avoid_load:
483483
base_file = ""

pr_agent/git_providers/github_provider.py

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ def _get_issue_handle(self, issue_url) -> Optional[Issue]:
7979
def get_incremental_commits(self, incremental=IncrementalPR(False)):
8080
self.incremental = incremental
8181
if self.incremental.is_incremental:
82-
self.unreviewed_files_set = dict()
82+
self.unreviewed_files_map = dict()
8383
self._get_incremental_commits()
8484

8585
def is_supported(self, capability: str) -> bool:
@@ -162,7 +162,7 @@ def _get_incremental_commits(self):
162162
if commit.commit.message.startswith(f"Merge branch '{self._get_repo().default_branch}'"):
163163
get_logger().info(f"Skipping merge commit {commit.commit.message}")
164164
continue
165-
self.unreviewed_files_set.update({file.filename: file for file in commit.files})
165+
self.unreviewed_files_map.update({file.filename: file for file in commit.files})
166166
else:
167167
get_logger().info("No previous review found, will review the entire PR")
168168
self.incremental.is_incremental = False
@@ -194,8 +194,8 @@ def get_previous_review(self, *, full: bool, incremental: bool):
194194
return self.comments[index]
195195

196196
def get_files(self):
197-
if self.incremental.is_incremental and self.unreviewed_files_set:
198-
return self.unreviewed_files_set.values()
197+
if self.incremental.is_incremental and self.unreviewed_files_map:
198+
return self.unreviewed_files_map.values()
199199
try:
200200
git_files = context.get("git_files", None)
201201
if git_files:
@@ -296,10 +296,10 @@ def get_diff_files(self) -> list[FilePatchInfo]:
296296
else:
297297
new_file_content_str = self._get_pr_file_content(file, self.pr.head.sha) # communication with GitHub
298298

299-
if self.incremental.is_incremental and self.unreviewed_files_set:
299+
if self.incremental.is_incremental and self.unreviewed_files_map:
300300
original_file_content_str = self._get_pr_file_content(file, self.incremental.last_seen_commit_sha)
301301
patch = load_large_diff(file.filename, new_file_content_str, original_file_content_str)
302-
self.unreviewed_files_set[file.filename] = patch
302+
self.unreviewed_files_map[file.filename] = patch
303303
else:
304304
if avoid_load:
305305
original_file_content_str = ""

pr_agent/tools/pr_reviewer.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ async def run(self) -> None:
142142
# ticket extraction if exists
143143
await extract_and_cache_pr_tickets(self.git_provider, self.vars)
144144

145-
if self.incremental.is_incremental and hasattr(self.git_provider, "unreviewed_files_set") and not self.git_provider.unreviewed_files_set:
145+
if self.incremental.is_incremental and hasattr(self.git_provider, "unreviewed_files_map") and not self.git_provider.unreviewed_files_map:
146146
get_logger().info(f"Incremental review is enabled for {self.pr_url} but there are no new files")
147147
previous_review_url = ""
148148
if hasattr(self.git_provider, "previous_review") and self.git_provider.previous_review is not None:

tests/unittest/test_azure_devops_incremental.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,9 @@ def test_populates_commits_range_and_files(self):
127127
self.assertEqual(provider.incremental.first_new_commit.sha, "new1")
128128
self.assertEqual(provider.incremental.last_seen_commit.sha, "old1")
129129
self.assertEqual(provider.incremental.last_seen_commit_sha, "old1")
130-
self.assertIn("/foo.py", provider.unreviewed_files_set)
131-
self.assertIn("/bar.py", provider.unreviewed_files_set)
132-
self.assertNotIn("/somedir", provider.unreviewed_files_set)
130+
self.assertIn("/foo.py", provider.unreviewed_files_map)
131+
self.assertIn("/bar.py", provider.unreviewed_files_map)
132+
self.assertNotIn("/somedir", provider.unreviewed_files_map)
133133
self.assertEqual(prev.html_url, provider.get_comment_url(prev))
134134

135135
def test_skips_merge_commits(self):

0 commit comments

Comments
 (0)