feat(review): preserve findings across review reruns - #2722
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
75348a5 to
d9a235a
Compare
PR Summary by QodoPreserve review findings across reruns via persistent comment state
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit 2ca9a24 |
|
Code review by qodo was updated up to the latest commit 0b66cd9 |
|
Code review by qodo was updated up to the latest commit 53f75f8 |
|
Code review by qodo was updated up to the latest commit c04cc62 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for taking #2453 on, and for turning the Qodo findings around so fast. Four notes inline, two with suggestions; the suite stays green with both applied.
| """Serialize state deterministically so repeated updates are diffable.""" | ||
| if not _is_valid_state(state): | ||
| raise ValueError("Invalid review finding state") | ||
| payload = json.dumps(state, ensure_ascii=False, sort_keys=True, separators=(",", ":")) |
There was a problem hiding this comment.
A finding body containing --> closes the marker early and the rest of the payload renders in the review, and parse_review_state still calls that state valid, so it repeats every run. Escaping both brackets fixes it without changing the decoded state.
It does not close Qodo's item 12: append_review_state prints bodies verbatim in the human section too.
| payload = json.dumps(state, ensure_ascii=False, sort_keys=True, separators=(",", ":")) | |
| payload = json.dumps(state, ensure_ascii=False, sort_keys=True, separators=(",", ":")) | |
| # '<' and '>' occur only inside JSON strings, so escaping cannot change the decoded state | |
| payload = payload.replace("<", "\\u003c").replace(">", "\\u003e") |
| allow_resolution = ( | ||
| bool(self.prediction) | ||
| and not bool(getattr(self.incremental, "is_incremental", False)) | ||
| and not bool(self.remaining_files_list) | ||
| and parsed.valid | ||
| and current_findings is not None | ||
| and len(current_findings) < max_findings | ||
| ) |
There was a problem hiding this comment.
With num_max_findings at its default of 3, two findings enable resolution, and nothing checks that the commit moved. Re-running /review on an unchanged head marks a finding RESOLVED just because the model did not repeat it.
Gate on the previous SHA rather than the current one: _review_head_sha reads last_commit_id, which only GitHub and Gitea set, so testing the current SHA turns two of your own tests red.
| allow_resolution = ( | |
| bool(self.prediction) | |
| and not bool(getattr(self.incremental, "is_incremental", False)) | |
| and not bool(self.remaining_files_list) | |
| and parsed.valid | |
| and current_findings is not None | |
| and len(current_findings) < max_findings | |
| ) | |
| previous_head_sha = str(((parsed.state or {}).get("last_run") or {}).get("head_sha") or "") | |
| current_head_sha = self._review_head_sha() | |
| allow_resolution = ( | |
| bool(self.prediction) | |
| and not bool(getattr(self.incremental, "is_incremental", False)) | |
| and not bool(self.remaining_files_list) | |
| and parsed.valid | |
| and current_findings is not None | |
| and len(current_findings) < max_findings | |
| and (not previous_head_sha or current_head_sha != previous_head_sha) | |
| ) |
| artifact={"error": e}) | ||
| else: | ||
| get_logger().exception(f"Failed to edit github comment", artifact={"error": e}) | ||
| raise |
There was a problem hiding this comment.
Not behind persistent_finding_state, and the same line lands in azuredevops_provider.py:237 and bitbucket_provider.py:438, so edit_comment propagates on all three, including the 403 branch this code calls "usually due to polling".
Mostly a gain: a failed edit on the suggestions comment now republishes them where main drops them. It escapes the function only when the fallback write fails too, and then /improve publishes nothing. Both worth a line in the description.
| ) | ||
| except Exception as e: | ||
| get_logger().exception(f"Failed to edit comment, error: {e}") | ||
| raise |
There was a problem hiding this comment.
Hard conflict with #2724, which returns False here. Worth agreeing the resolution before either merges, since as noted on github_provider.py the two are not equivalent.
There was a problem hiding this comment.
Hard conflict with #2724, which returns False here. Worth agreeing the resolution before either merges, since as noted on
github_provider.pythe two are not equivalent.
Agreed. Since #2724 owns the broader Azure path and already validates the False return contract end-to-end, I’ll align #2722 with that behavior rather than keep the competing exception-propagation change.
There was a problem hiding this comment.
Thanks. I traced the edit_comment() call sites further and narrowed #2722 so the two PRs no longer need to own the same Azure behavior.
#2722 now leaves AzureDevopsProvider.edit_comment() and the /improve path untouched. It only updates the shared persistent-comment publisher to treat an explicit False return as an edit failure, while preserving fallback_on_error=False for lifecycle updates so a failed edit cannot create a duplicate persistent review.
That means #2724 can keep the Azure True/False contract and its /improve caller handling independently.
I also added regression coverage for both paths:
False+fallback_on_error=False→ no fallback commentFalse+fallback_on_error=True→ normal fallback
The full unit suite is green locally.
|
Code review by qodo was updated up to the latest commit e97e318 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks for chasing this down, but the narrowing has not landed on this branch.
AzureDevopsProvider.edit_comment() is still modified here: head e97e3184 adds raise to its except block, which is the line the conflict was about. Merged onto today's main, this and #2724 still collide on azuredevops_provider.py and git_provider.py.
Worth knowing before you change anything: simply dropping the Azure raise would not be safe on its own. On main edit_comment returns None, not False, so the is False guard you added to publish_persistent_comment_full would never fire, and your fallback_on_error=False callers would read a failed edit as success. That only becomes safe once #2724's True/False contract is in.
So this is an ordering question. If #2724 lands first, this can drop the Azure change entirely. If this lands first, #2724 has to adopt the raise instead.
You're right. I found the residual change: the Azure raise and its regression test were introduced in an earlier commit and survived my later scope narrowing. My last update only narrowed the newest patch, not the full PR diff against main. I also understand why simply removing it now would be unsafe while main still returns None on that path. I'll coordinate the merge order: once the Azure True/False contract is established, I'll rebase #2722, remove the Azure-specific change and test, and verify the complete PR diff against main before updating the PR. |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Verdict: the design is sound but not mergeable yet, because the persisted state is writable by anyone who can comment on the PR. Six notes inline, four with committable suggestions.
Credit where it is due: versioning the marker, treating malformed state as unsafe rather than silently ignoring it, and refusing to resolve findings on incremental or partial runs are all the right calls, and your 64 new tests made every check below cheap for me to run.
The two blockers are the trust boundary in _load_review_finding_state and the unbounded state growth that eventually fails /review outright. The regex ones are one-liners. The comment ordering and the /improve fallout need a decision from you.
| def _load_review_finding_state(self): | ||
| header = f"{PRReviewHeader.REGULAR.value} 🔍" | ||
| try: | ||
| comments = list(self.git_provider.get_issue_comments()) | ||
| invalid_marker_found = False | ||
| for comment in reversed(comments): | ||
| body = GitProvider._get_comment_body(comment) | ||
| if not isinstance(body, str) or not body.startswith(header): | ||
| continue |
There was a problem hiding this comment.
Any PR participant can seed the state this reads back. header is the rendered heading, so a comment that starts ## PR Reviewer Guide 🔍 and carries a <!-- pr-agent-review-state:v1 ... --> block is accepted as the agent's own state. I ran it against a fake provider on head e97e3184: parsed.valid is True, and the injected finding body comes back out through _render_resolved_section into the agent's own review comment under "✅ Resolved findings".
Matching only on the hidden marker does not fix it, and I checked before suggesting it. It drops the heading requirement, so a plain comment carrying the marker is accepted where today it is not, and POST /markdown renders the marker to nothing, so the bait is invisible either way. All 52 tests still passed with that change, which is the point: nothing here tests the trust boundary.
What closes it is checking who wrote the comment before trusting its state. github_provider.py:995-1000 already has that pattern, and its deployment_type app-vs-user split is also why this is not a one-liner, so no suggestion from me.
| body = comment_body or "" | ||
| namespace_count = body.count(_STATE_MARKER_NAMESPACE) | ||
| matches = list(_STATE_MARKER_RE.finditer(body)) | ||
| if namespace_count == 0: | ||
| return ParsedReviewState(None, present=False, valid=True) | ||
| if namespace_count != 1 or len(matches) != 1: | ||
| return ParsedReviewState(None, present=True, valid=False) |
There was a problem hiding this comment.
finditer runs before the uniqueness guard, so a comment full of marker openers is scanned quadratically, and _load_review_finding_state feeds this every comment that starts with the heading, which is the same door as the note on pr_reviewer.py:284.
Timed on head e97e3184 by doubling the input: 4.0x per doubling, 0.22s at GitHub's 65536-char comment cap, 3.48s at 240KB. Moving the scan after the guard makes it linear, 1.9x per doubling and 0.06ms at 240KB, with all 52 tests in the three new files still green.
| body = comment_body or "" | |
| namespace_count = body.count(_STATE_MARKER_NAMESPACE) | |
| matches = list(_STATE_MARKER_RE.finditer(body)) | |
| if namespace_count == 0: | |
| return ParsedReviewState(None, present=False, valid=True) | |
| if namespace_count != 1 or len(matches) != 1: | |
| return ParsedReviewState(None, present=True, valid=False) | |
| body = comment_body or "" | |
| namespace_count = body.count(_STATE_MARKER_NAMESPACE) | |
| if namespace_count == 0: | |
| return ParsedReviewState(None, present=False, valid=True) | |
| # Scan only once the namespace is unique: the lazy payload is quadratic over repeated openers. | |
| if namespace_count != 1: | |
| return ParsedReviewState(None, present=True, valid=False) | |
| matches = list(_STATE_MARKER_RE.finditer(body)) | |
| if len(matches) != 1: | |
| return ParsedReviewState(None, present=True, valid=False) |
|
|
||
| The optional limit is reserved for the complete hidden marker. | ||
| """ | ||
| body = _STATE_MARKER_RE.sub("", review_body or "").rstrip() |
There was a problem hiding this comment.
Same regex, second call site, no guard at all. Measured by doubling: 4.2x per doubling, 0.23s at 60KB, 3.91s at 240KB. The input here is the freshly built review markdown rather than a comment, so it is only reachable when the model echoes repeated openers back from the diff, but the guard is free and takes it to 0.1ms at 240KB with the suite green.
| body = _STATE_MARKER_RE.sub("", review_body or "").rstrip() | |
| body = review_body or "" | |
| if body.count(_STATE_MARKER_NAMESPACE) == 1: | |
| # Strip only a well-formed marker: the lazy payload is quadratic over repeated openers. | |
| body = _STATE_MARKER_RE.sub("", body) | |
| body = body.rstrip() |
| prev_comments = list(self.get_issue_comments()) | ||
| for comment in prev_comments: | ||
| if comment.body.startswith(initial_header): | ||
| for comment in reversed(prev_comments): |
There was a problem hiding this comment.
reversed() picks the oldest comment on Azure DevOps. AzureDevopsProvider.get_issue_comments calls threads.reverse() at azuredevops_provider.py:985-995, so it already returns newest-first and reversing again inverts it. I ran the real method with a stubbed client over three threads: main's for comment in prev_comments picks NEWEST, this picks OLDEST.
test_persistent_update_uses_latest_matching_comment and test_persistent_update_accepts_dict_comments_and_uses_latest still pass, because their fake provider returns oldest-first, so the intent is clear and only Azure comes out backwards. pr_reviewer.py:289 reverses the same list, so on Azure the state is read from the oldest review rather than the latest, which is how a resolved finding comes back.
get_issue_comments carries no ordering in the base class, so this needs either an ordering contract there or normalising inside the Azure provider, neither of which fits a suggestion block.
| marker = serialize_review_state(state) | ||
| if max_chars is not None: | ||
| if not isinstance(max_chars, int) or max_chars < len(marker) + 1: | ||
| raise ValueError( | ||
| "Comment limit is too small for the persistent " | ||
| "review state marker" | ||
| ) | ||
| human_budget = max_chars - len(marker) - 3 | ||
| if len(human_body) > human_budget: | ||
| if human_budget <= 0: | ||
| human_body = "" | ||
| elif human_budget < 3: | ||
| human_body = human_body[:human_budget] | ||
| else: | ||
| human_body = human_body[: human_budget - 3] + "..." |
There was a problem hiding this comment.
The hidden marker is budgeted ahead of the reviewer's text, so the state wins as it grows. Against max_chars=65536: at 60 resolved findings the marker is 61406 chars and the review body is cut to 4127 chars mid-sentence; once the marker passes the cap this raises ValueError, which run() turns into "Failed to review PR" with no way back, since the state lives in the comment and never shrinks.
That is reachable on defaults. allow_resolution requires len(current_findings) < num_max_findings, default 3, so any run that returns 3 findings resolves nothing, and _retained_findings caps RESOLVED only while ACTIVE grows without bound. Simulating repeated 3-finding runs: 135 findings after 45 runs, marker 75700 chars, hard failure.
Failing soft keeps the review. It reds test_append_review_state_reserves_space_for_complete_marker and test_prepare_and_persisted_state_round_trip_preserves_marker_and_history, which encode the opposite trade-off, so which side to keep is your call; the other 50 stay green and the simulation runs to 60 with the review intact.
| marker = serialize_review_state(state) | |
| if max_chars is not None: | |
| if not isinstance(max_chars, int) or max_chars < len(marker) + 1: | |
| raise ValueError( | |
| "Comment limit is too small for the persistent " | |
| "review state marker" | |
| ) | |
| human_budget = max_chars - len(marker) - 3 | |
| if len(human_body) > human_budget: | |
| if human_budget <= 0: | |
| human_body = "" | |
| elif human_budget < 3: | |
| human_body = human_body[:human_budget] | |
| else: | |
| human_body = human_body[: human_budget - 3] + "..." | |
| marker = serialize_review_state(state) | |
| if max_chars is not None: | |
| if not isinstance(max_chars, int) or max_chars < 4: | |
| raise ValueError("Comment limit is too small for a review comment") | |
| if len(human_body) + len(marker) + 3 > max_chars: | |
| # The review is the product: drop the hidden state before the reviewer's own text. | |
| marker = "" | |
| if len(human_body) + 1 > max_chars: | |
| human_body = human_body[: max_chars - 4] + "..." |
| artifact={"error": e}) | ||
| else: | ||
| get_logger().exception(f"Failed to edit github comment", artifact={"error": e}) | ||
| raise |
There was a problem hiding this comment.
Following up my earlier note here, I was too generous: the default /improve path has an unguarded call site. pr_code_suggestions.py:603 sits after the try/except in publish_persistent_comment_with_history, on the branch taken when no previous suggestions comment exists, which is every first run with persistent_comment=true (the default, configuration.toml:185). The one I flagged before at :273 needs persistent_comment=false.
I ran it with a provider whose edit_comment raises: today the function returns {'id': 1}, with this change the RuntimeError propagates out. run()'s handler at pr_code_suggestions.py:312-317 then removes the progress comment and, because self.progress_response is still set, skips the "Failed to generate code suggestions" fallback, so the user is left with nothing at all instead of a stuck progress note. Worth guarding that call, or catching in the caller, before the raise lands.
…2510 has landed The-PR-Agent#2510 was merged upstream while this was being assembled, so the fork no longer carries it. Only The-PR-Agent#2722 sits on top of main now. The one conflict is the same as before with the sides swapped: both additions land at the same point in _prepare_pr_review and neither knows about the other. Finding state is appended before the outputs are pushed, so a sink receives the markdown the reader sees rather than one missing the lifecycle state - which is the only reason to want the sink. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e97e318 to
14889f5
Compare
|
@IsmaelMartinez Thanks again for the detailed review. I’ve now rebased #2722 onto the latest The updated branch now covers:
Validation after the final rebase:
A final test-only CodeQL cleanup then ran
I’d appreciate another look when you have time. Thanks again for pushing on the trust boundary and failure semantics in particular. |
|
Thanks for turning all of that round, and for the trust-boundary work in particular. One thing left before I can approve. The fail-closed path is wider than it needs to be. Only GitHub, GitLab and Azure DevOps override Both override Could that Separately, |
8374aea to
c7467e8
Compare
Thanks, good catch. I narrowed the fail-closed path so providers without lifecycle-state support now keep their existing plain persistent-review behavior, while lifecycle-capable providers still fail closed when authorship cannot be verified. I also rebased onto current main and resolved the Gitea test conflict. The fix is on c7467e8, and both Build-and-test and CodeQL are green. Thanks again for the careful review. |
Code Review by QodoSorry, something went wrongWe weren't able to complete the code review on our side. Please try again manually by commenting/agentic_review on this PR.
|
|
Quick follow-up: I ran an additional red-team pass after the last update and found a same-HEAD lifecycle edge case. An ACTIVE finding could be incorrectly resolved if a complete rerun on the same commit nondeterministically omitted it. I tightened resolution so it now also requires a known changed HEAD; same-HEAD and missing-HEAD reruns preserve ACTIVE findings. I then ran a real GitHub + model lifecycle smoke test. The same canonical review comment stayed in place through ACTIVE → ACTIVE → RESOLVED → RESOLVED, with no duplicate authoritative comments or false same-HEAD resolution. The fix is on cc60818, and Build-and-test and CodeQL are green. |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks, that is the narrowing I was after, and the same-HEAD case was a good one to catch on your own.
Approving. Verified per provider on cc608182: Gitea, Bitbucket and Bitbucket Server all report state not in play and route to plain publish_persistent_comment, so their single-comment behaviour is unchanged. GitHub, GitLab and Azure keep the fail-closed path. Full suite 3598 passed on the merged tree.
3 participants
Summary
/reviewrerunsDesign notes
Resolution is deliberately conservative.
Findings are only marked resolved after a complete, valid, full-scope review. Incremental reviews, partial analysis, malformed predictions, provider failures, and degraded persistence do not trigger negative state transitions.
Persistent lifecycle state is trusted only when the existing review comment can be verified as authored by PR-Agent. Hidden markers identify the comment type, but are not treated as authentication.
If authorship cannot be verified,
/reviewstill produces a visible standalone review without adopting or modifying an untrusted canonical comment.Provider ordering is explicit for persistent-comment lookup. Azure DevOps comments and replies are normalized using timestamp-based chronology rather than relying on raw API response order.
Lifecycle persistence also degrades safely when state grows too large: preserving the human review takes priority, and degraded runs do not silently resolve active findings.
Upstream integration
This branch has been rebased onto the latest
mainused for this update and incorporates the relevant upstream contracts from:edit_comment()success/failure behaviorpush_outputsThe PR keeps those upstream contracts rather than reintroducing overlapping behavior.
Validation
Final validation after addressing the provider fallback regression and the same-HEAD false-resolution edge case:
527 passed3586 passed, 1 skipped, 1 xfailed, 1 failedreasoning_effortcapability case and was reproduced independently on clean upstreammaingit diff --check: passedBuild-and-test: passedCodeQL: passedAn additional red-team pass found that a complete rerun on the same commit could incorrectly resolve an ACTIVE finding when model output omitted it nondeterministically.
Resolution now requires both a complete valid full review and a known changed HEAD. Same-HEAD and missing-HEAD reruns preserve ACTIVE findings conservatively.
A real GitHub + real-model smoke test was also completed on
yefuyou/pr-agent#2:5536025289ACTIVEACTIVERESOLVEDRESOLVEDCurrent PR head:
cc6081822901aa0d9d21e549af8c30b06c3893bdAI disclosure
This PR was developed with assistance from Codex and validated with targeted tests, full unit-suite runs, upstream CI, and manual contract review by the contributor.
Closes #2453