fix(#1559): preserve prior successful review on agent failure - #1560
fix(#1559): preserve prior successful review on agent failure#1560fullsend-ai-coder[bot] wants to merge 2 commits into
Conversation
When the review agent encounters a tool-failure after previously posting a successful review, the sticky comment was overwritten with a blanket "NOT reviewed" notice, collapsing the valid review findings into a hidden <details> block. This created a misleading signal for anyone checking the PR's review status. postFailureNotice() now checks for an existing sticky comment with a prior successful review before posting. When one exists, the failure is shown as a warning banner while the previous review findings remain prominently visible. Prior failures and stale-head notices are not preserved (replaced as before). Added ExtractCurrentContent() to the sticky package to extract the top-level content from a sticky comment body, stripping the marker, footer, and collapsed history blocks. Closes #1559 Signed-off-by: fullsend-code <fullsend-code@users.noreply.github.com>
Site previewPreview: https://6ed4aec4-site.fullsend-ai.workers.dev Commit: |
ReviewFindingsLow
Previous runReviewFindingsHigh
Medium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
| priorContent := sticky.ExtractCurrentContent(existing.Body, cfg) | ||
| if priorContent != "" && !isFailureOrStaleContent(priorContent) { | ||
| body = fmt.Sprintf("## Review\n\n> **⚠️ Latest review run failed** (%s). Showing results from previous successful review.\n\n%s", reason, priorContent) | ||
| printer.StepInfo("Preserving prior successful review") |
There was a problem hiding this comment.
[high] correctness
isFailureOrStaleContent does not match the format string produced by postFailureNotice. The format string wraps the emoji inside bold markers (
Suggested fix: Align the search string with the format string — either change the format to place the emoji outside bold, or change the search to
| {"empty", "", false}, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { |
There was a problem hiding this comment.
[medium] correctness
The "prior preserved failure" test case uses input with the emoji outside bold markers, which matches the search pattern but not the actual format string from postFailureNotice. The test passes but validates the wrong input format.
Suggested fix: Change the test input to match the actual format string. The test should then fail, confirming the bug, and both should be fixed together.
| This PR was NOT reviewed. Do not count this as an approval.`, reason) | ||
| } | ||
|
|
||
| // When a prior successful review exists, preserve it prominently |
There was a problem hiding this comment.
[low] style
GetAuthenticatedUser error silently discarded without logging, inconsistent with sticky.Post which logs a warning for the same failure.
Suggested fix: Log a warning via printer.StepInfo when the error is non-nil.
|
/fs-fix |
…Notice - Fix isFailureOrStaleContent to match the actual format string produced by postFailureNotice (emoji inside bold markers, not outside) - Update test case to use the correct format string - Log warning when GetAuthenticatedUser fails, consistent with sticky.Post Addresses review feedback on #1560 Signed-off-by: fullsend-fix <fullsend-code@users.noreply.github.com>
🔧 Fix agent — iteration 1 (bot-triggered)Addressed all 3 review findings: fixed the string mismatch between isFailureOrStaleContent and the format string in postFailureNotice (high), corrected the test case to use the actual format (medium), and added error logging for GetAuthenticatedUser (low). Fixed (3):
Tests: passed Updated by fullsend fix agent |
| printer.StepInfo("Could not determine bot user, marker spoofing protection degraded") | ||
| } | ||
| comments, listErr := client.ListIssueComments(ctx, owner, repo, pr) | ||
| if listErr == nil { |
There was a problem hiding this comment.
[low] redundant-api-calls
postFailureNotice now calls GetAuthenticatedUser and ListIssueComments to check for prior successful reviews, then unconditionally calls sticky.Post() which makes the same two API calls internally (sticky.go lines 43-53). This doubles the API round-trips on every failure path.
Suggested fix: Consider extracting the comment lookup into a shared helper or passing the found comment/bot-user to sticky.Post to avoid redundant calls.
fullsend-ai-review
Bot
added
the
ready-for-merge
|
A lot of changes to the review agent in the last week, closing to see if we reproduce this again. |
Retro: PR #1560 — fix(#1559): preserve prior successful review on agent failureOutcome: Closed without merging after 12 days. Human determined recent review agent changes may have already addressed the underlying problem. Timeline
What went well
What went wrong
ProposalsNo new proposals — all identified improvements are already tracked in existing open issues. Resolving #1569 and #1326 would have eliminated the 8-hour human intervention delay and the misdirected fix runs on this PR. |
1 participant
When the review agent encounters a tool-failure after previously posting a successful review, the sticky comment was overwritten with a blanket "NOT reviewed" notice, collapsing the valid review findings into a hidden
Details
block. This created a misleading signal for anyone checking the PR's review status.postFailureNotice() now checks for an existing sticky comment with a prior successful review before posting. When one exists, the failure is shown as a warning banner while the previous review findings remain prominently visible. Prior failures and stale-head notices are not preserved (replaced as before).
Added ExtractCurrentContent() to the sticky package to extract the top-level content from a sticky comment body, stripping the marker, footer, and collapsed history blocks.
Closes #1559
Post-script verification
agent/1559-preserve-review-on-failure)6a20b831b8d7655aa0d550d342676b404dfd8214..HEAD)