feat: persistent inline comments to prevent cross-run duplicates#2424
Open
IsmaelMartinez wants to merge 15 commits into
Open
feat: persistent inline comments to prevent cross-run duplicates#2424IsmaelMartinez wants to merge 15 commits into
IsmaelMartinez wants to merge 15 commits into
Conversation
Adds an opt-in config flag, persistent_inline_comments (default false). When enabled, the GitHub and GitLab providers fingerprint each inline comment and embed a hidden HTML marker in the posted body. On later runs the existing comment bodies are scanned for those markers, so a suggestion already posted is skipped instead of re-posted. This removes the duplicate inline comments that accumulate when the agent runs repeatedly on the same PR/MR (reported in particular on GitLab). Two fingerprints are matched with OR semantics: a body fingerprint over (file, line, normalised text) and a code fingerprint over the first ` ```suggestion ` block. The OR-match catches a re-emitted finding whether the model rephrases the prose or slightly changes the proposed code. The marker-scan store needs no external infrastructure; a database/cache backend could implement the same load/seen/add interface. Default behaviour is unchanged when the flag is off. Implements the feature requested in The-PR-Agent#2037.
Contributor
Review Summary by QodoPersistent inline comments to prevent cross-run duplicates
WalkthroughsDescription• Implements persistent inline comments to prevent duplicate suggestions across runs • Fingerprints comments with SHA-256 hashes (body and code) embedded as HTML markers • Scans existing PR/MR comments to skip already-posted suggestions using OR-match logic • Integrated into GitHub and GitLab providers with opt-in config flag (default false) • Includes comprehensive unit tests and documentation Diagramflowchart LR
A["Inline Comment"] --> B["Compute Fingerprints"]
B --> C["Body FP: file+line+text"]
B --> D["Code FP: file+line+code block"]
C --> E["Build Markers"]
D --> E
E --> F["Embed in Comment Body"]
F --> G["Post to PR/MR"]
H["Later Run"] --> I["Scan Existing Comments"]
I --> J["Extract Markers"]
J --> K["Check Store"]
K --> L{Already Posted?}
L -->|Yes| M["Skip Suggestion"]
L -->|No| N["Post with Markers"]
File Changes1. pr_agent/algo/inline_comment_dedup.py
|
Contributor
Code Review by Qodo
Context used 1.
|
…ests Addresses self-review findings on the persistent-inline-comments change: - Broaden the importance-tag regex so non-standard labels (hyphens, digits, capitals) are stripped from the body fingerprint, keeping it stable across runs instead of only for the lowercase-letter label set. - Move the inline_comment_dedup import into the sorted ..algo group in the GitLab provider, matching the GitHub provider. - Add provider-level tests: the cross-run code-fingerprint OR-match through GitHub (different prose, same suggestion block), the GitLab general-note fallback (marker append + fingerprint record), cross-run dedup via an existing-discussion marker scan, unsupported-provider degradation, and a non-standard-label fingerprint regression.
Contributor
|
Code review by qodo was updated up to the latest commit b8b6383 |
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…y 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.
Contributor
|
Code review by qodo was updated up to the latest commit f699e7e |
Author
|
Thanks for the review. Addressed in
The two inline comments are answered in their threads. All 41 unit tests pass locally. |
…ss runs The committable-suggestion fallback posts via mr.notes.create, which may not surface as a discussion. InlineCommentStore now also scans mr.notes.list, so a fallback comment's marker is found on later runs and not re-posted. Adds a regression test.
Contributor
|
Code review by qodo was updated up to the latest commit f30ccba |
…rate log - GitLab fingerprints anchor on the line the comment is attached to (source line for deletions, target line otherwise), so deletion comments dedup against their real anchor instead of always the target line. - Dedup markers are appended via a clip-aware helper, so the marker survives when max_comment_chars would otherwise truncate the body and drop it. - The all-duplicates log only fires when a comment was actually skipped as a duplicate, not when the deduped batch was merely empty. - Wrap the long log f-string and overlong test lines.
Author
|
Addressed the latest review batch in
On "GitHub anchor ignored": this is a deliberate trade-off and I have left it as is. The previous round flagged anchoring on the diff 44 unit tests pass locally. |
Contributor
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Contributor
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Contributor
|
Code review by qodo was updated up to the latest commit 194244f |
…trailing ws - The invalid-comment fixer strips the ```suggestion block, which also dropped the dedup markers appended after it; the fallback then re-published via disable_fallback=True with dedup skipped, so those comments duplicated on later runs. Dedup now separates filtering from marking: the fallback path is not filtered (a not-yet-posted comment must not be skipped) but is still marked and recorded, and a body that already carries a marker is left as is. - Wrap the all-duplicates info log to stay within the line-length limit. - Remove trailing whitespace on the blank lines around the record block.
Contributor
|
Code review by qodo was updated up to the latest commit a0869e0 |
Contributor
|
Code review by qodo was updated up to the latest commit b487346 |
…ot after fallback
Contributor
|
Code review by qodo was updated up to the latest commit 3dc8454 |
Contributor
|
Code review by qodo was updated up to the latest commit a2bac5f |
Contributor
|
Code review by qodo was updated up to the latest commit a749828 |
Contributor
|
Code review by qodo was updated up to the latest commit c9d7a17 |
Contributor
|
Code review by qodo was updated up to the latest commit ef9a46b |
(cherry picked from commit aefce47)
Contributor
|
Code review by qodo was updated up to the latest commit f6070fb |
Author
|
@naorpeled when you get a chance to look at this, one scope question. I have a follow-up in progress that adds a second, optional dedup tier for GitLab only: on top of the content fingerprinting in this PR, it also suppresses reworded duplicate inline comments by anchoring on the file and line position, so a finding that comes back slightly rephrased on a later push still gets recognised. It does not touch the GitHub path at all, that side keeps the line-independent behaviour from this PR unchanged. It is a fair chunk of code though (most of it GitLab provider logic and its tests), so folding it into this PR would make it noticeably bigger and harder to review. Would you rather I keep it as a separate follow-up PR once this one lands, or include it here? I am happy either way, so whichever is easier for you to review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Implements the feature requested in #2037: optional persistent inline comments, so the agent stops re-posting identical inline comments each time it runs on the same PR/MR (reported in particular on GitLab). Opt-in via a new
config.persistent_inline_commentsflag (defaultfalse); behaviour is unchanged when off.How it works
When enabled, each inline comment carries a hidden HTML marker with a short fingerprint, e.g.
<!-- pr-agent-dedup: a1b2c3d4e5f6 -->. On a later run the provider scans the existing comment bodies for those markers to rebuild the set of already-posted fingerprints, and skips any suggestion whose fingerprint is already present.Two fingerprints are matched with OR semantics:
**Suggestion:**lead and[category, importance: N]tag are stripped and whitespace collapsedThe OR-match catches a re-emitted finding whether the model rephrases the prose or slightly changes the proposed code. The marker-scan store needs no external infrastructure; a database/cache backend could implement the same
load/seen/addinterface.Scope
publish_inline_comments) and GitLab (send_inline_comment, including the general-note fallback). Other providers are unaffected: the listing adapter raisesNotImplementedError, which degrades to no dedup.pr_agent/algo/inline_comment_dedup.py; config flag inconfiguration.toml; docs indocs/docs/tools/improve.md.Tests
tests/unittest/test_inline_comment_dedup.py(15 tests): fingerprint normalisation/stability, code-block extraction, marker building, the store (including load-failure degradation and unsupported provider), and GitHub/GitLab integration (filters already-seen, marks new, within-batch dedup, all-duplicates skips publish, flag-off leaves bodies unmarked). All pass; existing provider tests unaffected.Notes
This is a native port of an approach we have been running in production on self-hosted GitLab (AWS Bedrock backend) since Monday this week (a few days, not a long soak yet). The GitLab integration here mirrors what is running there. The GitHub
publish_inline_commentspath is implemented and unit-tested but has not yet been exercised in production, so testing it, including on GHES, is especially valuable. @avidspartan1 — that GitHub path is the one to try.Credits
The GitHub code-fingerprint fix is from @avidspartan1, who tested the
publish_inline_commentspath on GHES, diagnosed thatvalidate_comments_inside_hunksrewrites a suggestion body when it has no valid hunk (so the code fingerprint recomputed afterwards no longer matched), and contributed the fix that captures the fingerprint before validation and threads it through. Folded in here as commit aefce47 with original authorship preserved, adding 3 GitHub-path tests (suite now 30).