fix(improve): clean up progress comment on cancellation - #2850
Conversation
Signed-off-by: hwan <3373484735@qq.com>
PR Summary by QodoClean up /improve progress comments on cancellation
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Signed-off-by: hwan <3373484735@qq.com>
|
Code review by qodo was updated up to the latest commit e88c5f5 |
Signed-off-by: hwan <3373484735@qq.com>
|
Code review by qodo was updated up to the latest commit e5289f8 |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Merging once the suggestion is in. Qodo's "persistent summary remains cleanup-owned" is resolved for the first run, but the identity check leaves the second run open, and all four of your tests return [] from get_issue_comments, so none reach it.
On a second run the publish edits the existing persistent comment, _clean_up_progress_note deletes the progress note, and the call returns the persistent comment, so is does not match. Cancellation then edits and removes a comment that is already gone: two 404s, each logged with a full traceback. With the suggestion applied the edit disappears and remove_comment goes from two calls to one.
A test for that path would be good, and it would sit next to the edit-failure branch Qodo still has open.
| identity_marker=PRCodeSuggestionsIdentity.SUMMARY.value, | ||
| legacy_initial_header=PRCodeSuggestionsHeader.SUMMARY.value, | ||
| ) | ||
| if published_comment is self.progress_response: |
There was a problem hiding this comment.
The publish returns the persistent comment on a second run, not the progress note, so this never fires and the handler later touches a comment _clean_up_progress_note already deleted. Checking for any returned comment covers both runs, and still leaves the note alone on the check-run early return, which returns None.
| if published_comment is self.progress_response: | |
| if published_comment is not None: |
Address review on The-PR-Agent#2850: publish_persistent_comment_with_history returns the pre-existing persistent comment on a second run, not the progress note, so the identity check never fired and the cancellation handler could touch a comment _clean_up_progress_note already removed. Checking for any returned comment covers both runs and still leaves the progress handle intact for the check-run early return, which yields None.
|
Code review by qodo was updated up to the latest commit d9b3064 |
Follow-up to d9b3064: the check-run early return in publish_persistent_comment_with_history yields None, so the 'is not None' guard left self.progress_response pointing at a comment _clean_up_progress_note never ran on that path. The progress status then stayed visible indefinitely after a normal run. Move _clean_up_progress_note above the check-run branch, invoke it before returning, and return progress_response so the caller drops the handle rather than leaving a dangling reference. Covers the path with test_run_cleans_up_progress_comment_on_check_run_publish.
|
Code review by qodo was updated up to the latest commit 15f6b9f |
Address review on The-PR-Agent#2850: the check-run branch returned progress_response unconditionally, so run() cleared self.progress_response even when _clean_up_progress_note() caught an edit or removal failure and the comment stayed visible — orphaning it with no handle for later cleanup or cancellation. Return None on cleanup failure so the caller keeps the tracked handle; the successful check-run path is unchanged. Covered by test_run_retains_progress_handle_when_check_run_cleanup_fails.
|
Code review by qodo was updated up to the latest commit 5bb7f7d |
|
Thanks for the suggestion @IsmaelMartinez — applied in While testing I noticed the check-run early return in A follow-up review flagged that returning Added coverage for both the successful check-run cleanup and the failed-cleanup case. Lifecycle suite is 7 passed; core/identity suites are 83 passed. CI is green. |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Merging, as promised once the suggestion landed. Clearing progress_response means the cancellation handler's guard short-circuits, so a second run no longer edits and removes a comment that is already gone. All seven go red on revert against today's main.
Two things I am not holding this for, both going into one follow-up issue: the second-run test I asked about is still missing, and the except Exception handler still reads a cleared progress_response as "nothing was published", so it can post a failure comment over suggestions that published fine.
Thanks for the quick turnarounds here and on #2865.
2 participants
Fixes #2849
What
Clean up the
/improveprogress comment when the command task is cancelledafter progress publication.
Why
asyncio.CancelledErrorbypasses the existingexcept Exceptioncleanup path.The cancellation correctly propagates, but the provider-owned progress comment
is left without a deletion attempt and can remain visible as stale status.
Implementation
asyncio.CancelledErrorbefore the ordinary exception handler.remove_comment()only when this run has a progress handle.cleanup failure that must not mask cancellation.
Tests
pytest -q tests/unittest/test_pr_code_suggestions_lifecycle.py tests/unittest/test_pr_code_suggestions_core.py— 71 passedpytest -q tests/unittest— 2576 passed, 1 skipped, 1 xfailed, 89 warningspython -m py_compile pr_agent/tools/pr_code_suggestions.py tests/unittest/test_pr_code_suggestions_lifecycle.pygit diff --checkNo real model, token, external provider, or production deployment was used.