Skip to content

Commit e3909d9

Browse files
IsmaelMartinezavidspartan1
authored andcommitted
fix: address review - record-after-success, stable GitHub anchor, lazy logger import
Addresses the Qodo review on PR The-PR-Agent#2424: - GitHub publish_inline_comments now records fingerprints only after a publish path runs without raising, tracks within-batch duplicates in a local set, and skips dedup on the disable_fallback re-publish. Previously the store was populated before create_review succeeded, so a 422 fallback retry could be wrongly skipped and silently drop a comment. - GitHub fingerprints are anchored on (path, content) instead of the diff position, which shifts as the PR gains commits; this keeps the fingerprint stable across runs (the persistent behaviour the feature is about). - inline_comment_dedup imports get_logger lazily inside the failure path, so the module no longer imports pr_agent.log at import time and can be imported standalone without the pre-existing log/config circular-import fragility. The test no longer needs an import-order workaround. - isort-format the new multi-line imports in both providers. The broad except in InlineCommentStore.load() is kept deliberately (fail-open: dedup must never break comment publishing), matching existing provider patterns.
1 parent 868cb48 commit e3909d9

4 files changed

Lines changed: 34 additions & 19 deletions

File tree

pr_agent/algo/inline_comment_dedup.py

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,6 @@
3535
import re
3636
from typing import Iterator, Optional
3737

38-
from pr_agent.log import get_logger
39-
4038
BODY_MARKER_RE = re.compile(r"<!-- pr-agent-dedup: ([a-f0-9]{12}) -->")
4139
CODE_MARKER_RE = re.compile(r"<!-- pr-agent-dedup-code: ([a-f0-9]{12}) -->")
4240

@@ -126,6 +124,7 @@ def load(self) -> set:
126124
for match in marker_re.finditer(body or ""):
127125
self._keys.add(match.group(1))
128126
except Exception as e:
127+
from pr_agent.log import get_logger
129128
get_logger().info(
130129
f"Persistent inline comments: could not load existing comments, "
131130
f"within-run dedup only. error={e}"

pr_agent/git_providers/github_provider.py

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,8 @@
1818
from ..algo.file_filter import filter_ignored
1919
from ..algo.git_patch_processing import extract_hunk_headers
2020
from ..algo.inline_comment_dedup import (body_fingerprint, build_markers,
21-
code_fingerprint, get_inline_comment_store,
22-
inline_comment_line)
21+
code_fingerprint,
22+
get_inline_comment_store)
2323
from ..algo.language_handler import is_valid_file
2424
from ..algo.types import EDIT_TYPE
2525
from ..algo.utils import (PRReviewHeader, Range, clip_tokens,
@@ -419,25 +419,36 @@ def create_inline_comment(self, body: str, relevant_file: str, relevant_line_in_
419419
return dict(body=body, path=path, position=position) if subject_type == "LINE" else {}
420420

421421
def publish_inline_comments(self, comments: list[dict], disable_fallback: bool = False):
422-
if get_settings().get("config.persistent_inline_comments", False):
422+
store = None
423+
pending_fingerprints = []
424+
# Dedup only on the top-level call. A fallback re-publish passes
425+
# disable_fallback=True; it must not re-filter, or it could drop a
426+
# comment that has not actually been posted yet.
427+
if not disable_fallback and get_settings().get("config.persistent_inline_comments", False):
423428
store = get_inline_comment_store(self)
429+
local_seen = set()
424430
deduped = []
425431
for comment in comments:
426432
if not comment:
427433
deduped.append(comment)
428434
continue
429435
path = comment.get("path", "")
430-
line = inline_comment_line(comment)
431436
body = comment.get("body", "")
432-
body_fp = body_fingerprint(path, line, body)
433-
code_fp = code_fingerprint(path, line, body)
434-
if store.seen(body_fp) or store.seen(code_fp):
437+
# GitHub committable comments are anchored by diff position, which
438+
# shifts as the PR gains commits; anchor the fingerprint on the file
439+
# path and comment content instead so it stays stable across runs.
440+
body_fp = body_fingerprint(path, None, body)
441+
code_fp = code_fingerprint(path, None, body)
442+
if (store.seen(body_fp) or store.seen(code_fp)
443+
or body_fp in local_seen or (code_fp and code_fp in local_seen)):
435444
continue
436445
marked = dict(comment)
437446
marked["body"] = f"{body}\n\n{build_markers(body_fp, code_fp)}"
438447
deduped.append(marked)
439-
store.add(body_fp)
440-
store.add(code_fp)
448+
local_seen.add(body_fp)
449+
if code_fp:
450+
local_seen.add(code_fp)
451+
pending_fingerprints.append((body_fp, code_fp))
441452
if not any(deduped):
442453
get_logger().info("Persistent inline comments: all suggestions already posted; nothing to publish")
443454
return
@@ -459,6 +470,13 @@ def publish_inline_comments(self, comments: list[dict], disable_fallback: bool =
459470
get_logger().error(f"Failed to publish inline code comments fallback, error: {e}")
460471
raise e
461472

473+
# Record fingerprints only after a publish path has run without raising,
474+
# so a failed publish does not block a retry of the same comment this run.
475+
if store is not None:
476+
for body_fp, code_fp in pending_fingerprints:
477+
store.add(body_fp)
478+
store.add(code_fp)
479+
462480
def get_review_thread_comments(self, comment_id: int) -> list[dict]:
463481
"""
464482
Retrieves all comments in the same thread as the given comment.

pr_agent/git_providers/gitlab_provider.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,8 @@
1515
from ..algo.file_filter import filter_ignored
1616
from ..algo.git_patch_processing import decode_if_bytes
1717
from ..algo.inline_comment_dedup import (body_fingerprint, build_markers,
18-
code_fingerprint, get_inline_comment_store)
18+
code_fingerprint,
19+
get_inline_comment_store)
1920
from ..algo.language_handler import is_valid_file
2021
from ..algo.utils import (clip_tokens,
2122
find_line_number_of_relevant_line_in_file,

tests/unittest/test_inline_comment_dedup.py

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,8 @@
11
from unittest.mock import MagicMock, patch
22

3-
# Import a provider module first: it pulls in pr_agent.log via its transitive
4-
# imports before config load, avoiding the partially-initialised-module
5-
# circular import that triggers when pr_agent.log is the first pr_agent import.
3+
from pr_agent.algo import inline_comment_dedup as d
64
from pr_agent.git_providers.github_provider import GithubProvider
75
from pr_agent.git_providers.gitlab_provider import GitLabProvider
8-
from pr_agent.algo import inline_comment_dedup as d
96

107

118
# --------------------------------------------------------------------------- #
@@ -106,7 +103,7 @@ def _patch_flag(value):
106103

107104

108105
def test_github_filters_seen_and_marks_new():
109-
seen_fp = d.body_fingerprint("a.py", 10, "old body")
106+
seen_fp = d.body_fingerprint("a.py", None, "old body")
110107
p = _gh_provider([f"old body\n\n<!-- pr-agent-dedup: {seen_fp} -->"])
111108
gs = _patch_flag(True)
112109
try:
@@ -123,7 +120,7 @@ def test_github_filters_seen_and_marks_new():
123120

124121

125122
def test_github_all_duplicates_skips_publish():
126-
seen_fp = d.body_fingerprint("a.py", 10, "old body")
123+
seen_fp = d.body_fingerprint("a.py", None, "old body")
127124
p = _gh_provider([f"old body\n\n<!-- pr-agent-dedup: {seen_fp} -->"])
128125
gs = _patch_flag(True)
129126
try:
@@ -241,7 +238,7 @@ def test_github_code_fingerprint_or_match_across_runs():
241238
# existing comment carries ONLY a code marker; a new comment with different
242239
# prose but the same suggestion block must be dropped via the code fp even
243240
# though its body fingerprint differs.
244-
code_fp = d.code_fingerprint("a.py", 10, "p\n```suggestion\nx = 1\n```")
241+
code_fp = d.code_fingerprint("a.py", None, "p\n```suggestion\nx = 1\n```")
245242
p = _gh_provider([f"earlier wording\n\n<!-- pr-agent-dedup-code: {code_fp} -->"])
246243
gs = _patch_flag(True)
247244
try:

0 commit comments

Comments
 (0)