fix(post-review): post file-level comments in a separate review - #6001
fix(post-review): post file-level comments in a separate review#6001rh-hemartin wants to merge 1 commit into
Conversation
File-level comments (findings outside diff hunks) were mixed into the same review batch as inline comments. A single invalid file-level comment would 422 the entire review submission, dropping all inline comments with it. Split findingsToReviewComments into two return slices (inline and fileLevel) and post file-level comments in a preceding COMMENT review. If the file-level review fails, warn and continue; the main review and sticky comment are unaffected. Relates-to: fullsend-ai/agents#430 Relates-to: fullsend-ai/agents#193 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Hector Martinez <hemartin@redhat.com>
PR Summary by QodoIsolate file-level PR review comments into a separate COMMENT review
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
|
🤖 Finished Review · ✅ Success · Started 10:23 AM UTC · Completed 10:36 AM UTC Commit: |
|
Design note: per-comment isolation An alternative considered was posting each file-level comment as its own review call, so a single invalid comment cannot take down the others. The tradeoff is API call volume: 10 file-level comments would cost 10 calls instead of 1. For repos that hit GitHub rate limits during busy periods, that multiplier adds up. The current two-batch approach (one COMMENT review for file-level, one for inline) gets most of the isolation benefit at 2 calls instead of N+1. If file-level batch rejections turn out to be common in practice, we can revisit with per-comment posting for file-level comments while keeping inline comments batched. |
Code Review by Qodo
1. File-level failure under-logged
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| if err := client.CreatePullRequestReview(ctx, owner, repo, pr, "COMMENT", "", commitSHA, fileLevelComments); err != nil { | ||
| printer.StepWarn(fmt.Sprintf("File-level comments failed (%v), findings remain in sticky comment", err)) | ||
| } else { | ||
| printer.StepDone(fmt.Sprintf("Posted %d file-level comment(s)", len(fileLevelComments))) |
There was a problem hiding this comment.
1. File-level failure under-logged 🐞 Bug ◔ Observability
When the separate file-level COMMENT review fails, submitFormalReview only logs the formatted error string and omits structured GitHub API error details and rejected-comment context, making validation failures difficult to diagnose. This is introduced by the new isolated file-level posting path which does not reuse the existing error-detail logging helpers used for the main review submission.
Agent Prompt
### Issue description
The new isolated file-level review submission logs only `File-level comments failed (%v)` and does not emit the structured GitHub error details (`gh.APIError.Errors[]`) or which specific comments were rejected. Since this call is intentionally fail-open, better diagnostics are important to troubleshoot why file-level comments didn’t appear.
### Issue Context
`logAPIErrorDetails` and `logRejectedComments` already exist and are used on the main review submission path (especially for 422 validation failures). The file-level submission path should use the same helpers (at least on 422), without blocking the main review.
### Fix Focus Areas
- internal/cli/postreview.go[351-359]
- internal/cli/postreview.go[383-400]
- internal/cli/postreview.go[476-505]
### Suggested change
- In the `CreatePullRequestReview(... fileLevelComments)` error branch:
- Call `logAPIErrorDetails(err, printer)` so field-level validation errors are visible.
- Optionally, when `is422Error(err)` is true, also call `logRejectedComments(fileLevelComments, err, printer)` to print which file-level comment(s) were rejected.
- Keep the current behavior of not returning an error (main review proceeds).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ReviewFindingsLow
Labels: Bug fix in Go CLI post-review command (review agent pipeline) |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| // don't poison the main review batch. A single invalid file-level | ||
| // comment would otherwise 422 the entire submission. | ||
| if len(fileLevelComments) > 0 { | ||
| if err := client.CreatePullRequestReview(ctx, owner, repo, pr, "COMMENT", "", commitSHA, fileLevelComments); err != nil { |
There was a problem hiding this comment.
[low] edge-case
When the review event is COMMENT and all findings fall outside diff hunks (producing only file-level comments, zero inline comments), the file-level comments are posted in a separate COMMENT review, then the main review is skipped at the early-return. This path has no dedicated integration test covering the COMMENT verdict + only-file-level-findings scenario.
Suggested fix: Add a test case with a COMMENT verdict where all findings are outside diff hunks. Verify that exactly one COMMENT review is created (the file-level one) and the main review is skipped.
|
|
||
| // Post file-level comments in a separate COMMENT review so they | ||
| // don't poison the main review batch. A single invalid file-level | ||
| // comment would otherwise 422 the entire submission. |
There was a problem hiding this comment.
[low] error-handling
The file-level COMMENT review is posted with an empty body string. While GitHub's API accepts empty-body reviews when comments are present, this is an implicit contract that could break if the API changes its validation.
fullsend-ai-review
Bot
added
ready-for-merge
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review findings (2 posted inline; 1 additional finding below on a line outside the diff hunk that GitHub won't let me anchor inline):
[MEDIUM] internal/cli/postreview.go:367 — Misleading skip message and doc comment after a file-level review has already been posted
When event == "COMMENT" and every eligible finding falls outside a diff hunk, fileLevelComments is non-empty and inlineComments is empty. The code first posts a COMMENT review with the file-level comments (lines 354-360), then hits if event == "COMMENT" && len(inlineComments) == 0 (line 367) and logs "Skipping formal COMMENT review (sticky comment already updated)" before returning nil. Both this log line and the function's doc comment (lines 296-299: "COMMENT: skipped when no inline-eligible findings exist ... sticky comment already covers it") imply no review was submitted in this run, when in fact a separate COMMENT review carrying the file-level comments was just posted moments earlier.
Suggestion: rephrase the log message and doc comment to acknowledge that a file-level review may already have been posted in this branch, e.g. only claim "sticky comment already covers it" when fileLevelComments is also empty, or say "Skipping formal COMMENT review with inline comments (file-level findings posted separately, if any)".
| printer.StepInfo(fmt.Sprintf("%d finding(s) posted as file-level comment(s) (line outside diff hunk)", len(fileLevelComments))) | ||
| } | ||
|
|
||
| // Post file-level comments in a separate COMMENT review so they |
There was a problem hiding this comment.
[MEDIUM] PR's stated rationale for isolating file-level comments is not supported by the linked issues
The comment here and the PR description assert that "a single invalid file-level comment would 422 the entire review submission, dropping all inline comments with it." Both linked issues (fullsend-ai/agents#430 and fullsend-ai/agents#193) describe a different, already-fixed failure mode: out-of-hunk inline comments (not Line=0 file-level comments) causing a 422 because they weren't excluded from the inline array before submission. Neither issue documents an observed case where a file-level comment itself (Line=0, no position) was invalid and 422'd a batch that also contained valid inline comments. The new isolation mechanism reads as speculative hardening for an unconfirmed failure mode rather than a fix for the cited root cause.
Suggestion: either confirm a concrete scenario where a file-level comment itself is rejected by GitHub, or soften the code comment/PR description to describe this as defense-in-depth rather than a proven root cause.
| // don't poison the main review batch. A single invalid file-level | ||
| // comment would otherwise 422 the entire submission. | ||
| if len(fileLevelComments) > 0 { | ||
| if err := client.CreatePullRequestReview(ctx, owner, repo, pr, "COMMENT", "", commitSHA, fileLevelComments); err != nil { |
There was a problem hiding this comment.
[MEDIUM] No 422 retry/fallback for the file-level review, unlike the main review
The main review submission below (around lines 383-400) has an explicit 422 fallback: it retries without inline comments and folds them into the review body via buildFallbackReviewBody, preserving visibility of the findings. This new file-level COMMENT review has no equivalent fallback — on any failure the file-level comments are simply dropped with a printer.StepWarn, even though they could similarly be appended to a fallback body so the findings remain visible on the review itself rather than only in the sticky comment.
Suggestion: reuse buildFallbackReviewBody-style logic for the file-level comments so a failed file-level review still surfaces the findings' content in a body, consistent with the main review's 422 fallback behavior.
2 participants
Summary
findingsToReviewCommentsnow returns two separate slices (inlineandfileLevel) instead of one mixed slice.Relates-to: fullsend-ai/agents#430
Relates-to: fullsend-ai/agents#193
Test plan
TestFindingsToReviewComments-- verifies inline/fileLevel separation with nil diffHunksTestFindingsToReviewComments_FiltersByDiffHunks-- out-of-hunk finding lands in fileLevel sliceTestFindingsToReviewComments_EmptyPatchSkipsLineFiltering-- binary/empty patches skip line filteringTestFindingsToReviewComments_AllSeveritiesPassThrough-- all severities remain inline with nil diffHunksTestFindingsToReviewComments_AllSeveritiesFallbackToFileLevel-- all severities fall back to file-level when out of hunkTestSubmitFormalReview_FiltersByPRFileDiffs-- integration: two reviews created (COMMENT + REQUEST_CHANGES)TestSubmitFormalReview_FileLevelCommentPostedSeparately-- file-level COMMENT review posted before main reviewTestSubmitFormalReview_FileLevelFailureDoesNotBlockMainReview-- file-level 422 does not block main review🤖 Generated with Claude Code