fix(#1470): retry review submission without inline comments on 422 - #1474
fix(#1470): retry review submission without inline comments on 422#1474fullsend-ai-coder[bot] wants to merge 1 commit into
Conversation
When CreatePullRequestReview returns a 422 Unprocessable Entity error and inline comments were attached, the error is likely caused by stale or invalid diff positions. Instead of failing the entire review run, retry the submission without inline comments so the formal review disposition (APPROVE/REQUEST_CHANGES) is still recorded. A warning is logged noting the dropped inline comments. The retry only triggers when both conditions are met: - The error is a GitHub API 422 - Inline comments were present in the failed request Non-422 errors and 422 errors without inline comments propagate unchanged, preserving existing behavior for other failure modes. Also adds CreatePullRequestReviewFunc to FakeClient for per-call error control in tests. Closes #1470
Site previewPreview: https://136b2a4d-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsLow
No blocking findings. The retry logic is correct: |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| // Pull request reviews for ListPullRequestReviews. | ||
| PRReviews map[string][]PullRequestReview // key: "owner/repo/number" | ||
|
|
||
| // CreatePullRequestReviewFunc, when non-nil, overrides the default |
There was a problem hiding this comment.
[low] style
CreatePullRequestReviewFunc is invoked while f.mu is held. The doc comment should warn that callbacks must not call other FakeClient methods, as sync.Mutex is not reentrant and such calls would deadlock.
Suggested fix: Add a note to the doc comment: "The callback runs under f.mu — do not call other FakeClient methods from within it."
fullsend-ai-review
Bot
added
the
ready-for-merge
|
I dont think this is worth pursuing, the real fix is to cancel previous reviews when one is incoming. We may need a debounce system as the one proposed for retro in one of the PRs currently open. |
Retro: PR #1474 — Closed without merge (symptom-level fix for already-fixed problem)Timeline
Root cause of wasted workThe retro agent filed #1470 as a near-duplicate of the still-open #1067, which describes the same 422 error in the same function ( Existing issues that cover the identified gapsAll improvement areas from this workflow are already tracked:
Housekeeping note#1067 is still open with No new proposals filed — all gaps are covered by existing open issues. |
1 participant
When CreatePullRequestReview returns a 422 Unprocessable Entity error and inline comments were attached, the error is likely caused by stale or invalid diff positions. Instead of failing the entire review run, retry the submission without inline comments so the formal review disposition (APPROVE/REQUEST_CHANGES) is still recorded. A warning is logged noting the dropped inline comments.
The retry only triggers when both conditions are met:
Non-422 errors and 422 errors without inline comments propagate unchanged, preserving existing behavior for other failure modes.
Also adds CreatePullRequestReviewFunc to FakeClient for per-call error control in tests.
Closes #1470
Post-script verification
agent/1470-422-retry-graceful)4342f307b57386cde8b751fe1a1a9002ea7fabde..HEAD)