brooktrails/pr-reviewer
Watch
2
0
Fork
You've already forked pr-reviewer
 0
Code Issues 1 Pull requests 1 Projects Releases Packages Wiki Activity Actions

fix(webhook): retry accepts a review that couldn't run, not only one that failed #117

Open
rcsheets wants to merge 1 commit from fix/retry-unavailable into main
pull from: fix/retry-unavailable
merge into: brooktrails:main
Conversation 1 Commits 1 Files changed 2 +65 -7
rcsheets commented 2026-10-02 06:29:44 +00:00
Owner
Copy link

@pr-reviewer-bot retry refused to retry a review that couldn't run.

What happened

Seen on #116 (comment). The review ended with "that model isn't loaded on the inference service right now" and its comment closed with "Comment @pr-reviewer-bot retry to try again". The bot answered that with "There's nothing to retry — the most recent review attempt on this PR didn't fail."

Cause

The retry command asks ConversationContext.LatestFailedReview whether there is anything to retry, and that matched the status failed alone. A model that isn't loaded, or a service that isn't reachable, is recorded as unavailable — a status added after the command was written. The runner invites a retry for both outcomes, so the command refused its own invitation for one of them.

The unavailable status on #116 is inferred from the comment's wording, which the runner only produces for that status; the production row was not inspected.

Fix

LatestFailedReview accepts failed or unavailable. It stays an allowlist, so a status added later is not retryable until someone decides it should be.

Testing

  • TestRetryCommandSucceedsWithUnavailablePrior reproduces the case: it fails against the old check with the bot's "nothing to retry" outcome and passes against the new one.
  • TestLatestFailedReviewCoversEveryStatus walks the status vocabulary and pins which are retryable.
  • go test -race ./... passes. The Postgres-backed tests skipped locally; nothing here touches the database.

This is independent of #116 and branches from main.

🤖 Generated with Claude Code

`@pr-reviewer-bot retry` refused to retry a review that couldn't run. ## What happened Seen on #116 ([comment](https://git.brooktrails.org/brooktrails/pr-reviewer/pulls/116#issuecomment-4763)). The review ended with "that model isn't loaded on the inference service right now" and its comment closed with "Comment `@pr-reviewer-bot retry` to try again". The bot answered that with "There's nothing to retry — the most recent review attempt on this PR didn't fail." ## Cause The retry command asks `ConversationContext.LatestFailedReview` whether there is anything to retry, and that matched the status `failed` alone. A model that isn't loaded, or a service that isn't reachable, is recorded as `unavailable` — a status added after the command was written. The runner invites a retry for both outcomes, so the command refused its own invitation for one of them. The `unavailable` status on #116 is inferred from the comment's wording, which the runner only produces for that status; the production row was not inspected. ## Fix `LatestFailedReview` accepts `failed` or `unavailable`. It stays an allowlist, so a status added later is not retryable until someone decides it should be. ## Testing - `TestRetryCommandSucceedsWithUnavailablePrior` reproduces the case: it fails against the old check with the bot's "nothing to retry" outcome and passes against the new one. - `TestLatestFailedReviewCoversEveryStatus` walks the status vocabulary and pins which are retryable. - `go test -race ./...` passes. The Postgres-backed tests skipped locally; nothing here touches the database. This is independent of #116 and branches from `main`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
fix(webhook): retry accepts a review that couldn't run, not only one that failed
All checks were successful
ci / check (pull_request) Successful in 49s
Details
ci / integration (pull_request) Successful in 40s
Details
ci / browser (pull_request) Successful in 39s
Details
a4124011a2 
Seen on #116: the review ended "that model isn't loaded on the inference
service right now", its comment closed with "Comment `@pr-reviewer-bot
retry` to try again", and the bot answered that with "There's nothing to
retry — the most recent review attempt on this PR didn't fail."

The retry command asks ConversationContext.LatestFailedReview whether there
is anything to retry, and that matched the status 'failed' alone. A model
that isn't loaded, or a service that isn't reachable, is recorded as
'unavailable' — a status added after the command was written. The runner
invites a retry for both outcomes, so the command refused its own
invitation for one of them.

LatestFailedReview now accepts 'failed' or 'unavailable'. It stays an
allowlist, so a status added later is not retryable until someone decides
it should be.

TestRetryCommandSucceedsWithUnavailablePrior fails against the old check
with the bot's "nothing to retry" outcome and passes against the new one.
TestLatestFailedReviewCoversEveryStatus walks the status vocabulary and
pins which are retryable.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pr-reviewer-bot commented 2026-10-02 06:29:46 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.52.3 | Safety Check | Nemotron 3 Nano | tracking id r-bf4f5a-a08bfe
This is an AI-generated review and may contain mistakes.

Status: ❌ Failed

Review failed. Tracking id r-bf4f5a-a08bfe — see logs for details.

Comment @pr-reviewer-bot retry to try again.

<!-- pr-reviewer:review --> *Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.52.3 | Safety Check | Nemotron 3 Nano | tracking id `r-bf4f5a-a08bfe`* *This is an AI-generated review and may contain mistakes.* **Status:** ❌ Failed --- Review failed. Tracking id `r-bf4f5a-a08bfe` — see logs for details. Comment `@pr-reviewer-bot retry` to try again.
All checks were successful
ci / check (pull_request) Successful in 49s
Details
ci / integration (pull_request) Successful in 40s
Details
ci / browser (pull_request) Successful in 39s
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/retry-unavailable:fix/retry-unavailable
git switch fix/retry-unavailable

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff fix/retry-unavailable
git switch fix/retry-unavailable
git rebase main
git switch main
git merge --ff-only fix/retry-unavailable
git switch fix/retry-unavailable
git rebase main
git switch main
git merge --no-ff fix/retry-unavailable
git switch main
git merge --squash fix/retry-unavailable
git switch main
git merge --ff-only fix/retry-unavailable
git switch main
git merge fix/retry-unavailable
git push origin main
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
brooktrails/pr-reviewer!117
No description provided.