Make loop feedback waits resumable#13
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds persisted awaiting-feedback state, threads it through queue and loop handling, updates dashboard output, and revises wait-state guidance. ChangesPersisted Feedback Wait
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3314c3f817
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/crq/service_test.go (1)
430-440: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSet a non-zero
FeedbackWaitTimeoutin this fixture.Lines 478-480 currently only prove
Deadline == StartedAt, because this config leavesFeedbackWaitTimeoutat0. That would still pass if the production code ignored the configured timeout entirely.Proposed fix
cfg := Config{ GateRepo: "owner/gate", StateRef: "crq-state", Host: "testhost", Bot: "coderabbitai[bot]", ReviewCommand: "`@coderabbitai` review", RateLimitMarker: "rate limited by coderabbit.ai", MinInterval: 0, InflightTimeout: time.Minute, PollInterval: time.Millisecond, + FeedbackWaitTimeout: time.Minute, FiredMax: 500, }Also applies to: 471-480
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/crq/service_test.go` around lines 430 - 440, The test fixture in the Config setup is leaving FeedbackWaitTimeout at zero, so it only validates Deadline equals StartedAt and won’t catch code that ignores the configured timeout. Update the Config used in the relevant service tests to set a non-zero FeedbackWaitTimeout, then adjust the assertions around Deadline/StartedAt so they verify the timeout is actually applied in the logic exercised by the test, using the existing Config and service behavior in service_test.go.internal/crq/feedback.go (1)
279-309: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClear the persisted wait using the loop’s target head.
report.Headcan change if the PR head advances while this loop is still waiting on the originalhead. Passingreport.Headcan leave the old wait uncleared, or worse, clear a newer wait written for the updated head. Use the immutableheadcaptured fromwaitResult/pullHead.Proposed fix
if len(report.Findings) > 0 { report.Status = "feedback" - s.clearFeedbackWait(ctx, repo, pr, report.Head) + s.clearFeedbackWait(ctx, repo, pr, head) return report, 10, nil } if report.Converged { - s.clearFeedbackWait(ctx, repo, pr, report.Head) + s.clearFeedbackWait(ctx, repo, pr, head) return report, 0, nil } @@ if time.Now().After(deadline) { report.Status = "timeout" - s.clearFeedbackWait(ctx, repo, pr, report.Head) + s.clearFeedbackWait(ctx, repo, pr, head) return report, 2, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/crq/feedback.go` around lines 279 - 309, The feedback wait cleanup in the waiting loop should use the immutable target head captured for the current wait, not the mutable report.Head. Update the calls in the feedback wait path to clear/persist state using head from the waitResult/pullHead flow so an advancing PR head does not leave stale wait state behind or clear a newer wait. Focus on the feedback wait helpers around clearFeedbackWait and the wait loop that handles report.Converged, timeout, and blocked polling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/crq/feedback_test.go`:
- Around line 591-604: The timeout-path test in feedback_test.go only checks the
returned report and elapsed time, so it misses stale-state regressions. After
calling Loop and asserting the timeout result, reload the CRQ state through the
same service path used elsewhere in the test and verify AwaitingFeedback is
cleared; use the existing Loop and state-loading helpers in this test to confirm
the dead wait entry is removed and won’t keep later enqueue/loop calls deduped.
In `@internal/crq/service.go`:
- Around line 269-272: The review path in service.go is treating benign adoption
races as Pump errors; update the existing-review flow around
recordExistingReviewPosted so ErrNoChange is handled the same way as the
reservation path and returns a lost_race-style PumpResult instead of bubbling an
error. Check the related logic in recordExistingReviewPosted, the
existing-review handling block, and the similar reservation-path branches
(including the other mentioned call sites) so st.InFlight != nil or queue-head
changes exit cleanly without failing the pump.
- Around line 259-269: The existing-review adoption flow in service.go is
anchoring `recordExistingReviewPosted` only to the review/commit timestamp,
which can mistakenly adopt a command from a previous head. Update the
`existingReviewCommand`/`recordExistingReviewPosted` path to anchor and validate
against the queued PR head captured by `Pump`, and pass that expected head
through the adoption check so `GetPull` cannot race to a different head before
marking it `Fired` or `AwaitingFeedback`. Revalidate the head inside the
adoption logic and only record the command when the expected head still matches
the current PR head.
- Around line 230-236: The wait-marker check in `service.go` is using the stale
`pending := state.AwaitingFeedback[key]` snapshot before `s.store.Update`, which
can resurrect `st.Fired[key]` after `Cancel` clears the key. Revalidate the
current wait marker inside the `s.store.Update` closure in the relevant service
method, and only assign `st.Fired[key] = head` when the key is still present in
`st.AwaitingFeedback` with the same `Head`; otherwise skip the update and keep
the dedupe state unchanged.
---
Outside diff comments:
In `@internal/crq/feedback.go`:
- Around line 279-309: The feedback wait cleanup in the waiting loop should use
the immutable target head captured for the current wait, not the mutable
report.Head. Update the calls in the feedback wait path to clear/persist state
using head from the waitResult/pullHead flow so an advancing PR head does not
leave stale wait state behind or clear a newer wait. Focus on the feedback wait
helpers around clearFeedbackWait and the wait loop that handles
report.Converged, timeout, and blocked polling.
In `@internal/crq/service_test.go`:
- Around line 430-440: The test fixture in the Config setup is leaving
FeedbackWaitTimeout at zero, so it only validates Deadline equals StartedAt and
won’t catch code that ignores the configured timeout. Update the Config used in
the relevant service tests to set a non-zero FeedbackWaitTimeout, then adjust
the assertions around Deadline/StartedAt so they verify the timeout is actually
applied in the logic exercised by the test, using the existing Config and
service behavior in service_test.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 875e789c-70bc-4de4-bab7-0679e00f33e6
📒 Files selected for processing (8)
internal/crq/auto.gointernal/crq/feedback.gointernal/crq/feedback_test.gointernal/crq/service.gointernal/crq/service_test.gointernal/crq/state.gointernal/crq/state_test.gointernal/crq/store.go
📜 Review details
🔇 Additional comments (11)
internal/crq/feedback_test.go (1)
491-551: LGTM!internal/crq/service_test.go (1)
374-384: LGTM!Also applies to: 483-538, 692-749
internal/crq/state.go (1)
22-36: LGTM!Also applies to: 61-69, 110-152, 183-198, 230-241, 271-287, 318-330, 362-368, 408-409
internal/crq/state_test.go (1)
58-107: LGTM!internal/crq/store.go (2)
184-201: LGTM!
180-202: 🩺 Stability & AvailabilityNo change needed. Both
GitStateStoreandMemoryStorenormalize onLoadandUpdate, so the state-shape contract is already enforced across the store implementations.internal/crq/service.go (2)
64-78: LGTM!
366-369: LGTM!Also applies to: 564-564, 841-841
internal/crq/feedback.go (2)
214-252: LGTM!Also applies to: 269-269
328-460: LGTM!internal/crq/auto.go (1)
293-295: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c47e023112
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
- Check DryRun before adopting an existing review command so a dry-run pump can't mutate queue state or report fired. - Stamp RequeuedAt on requeued items and use it as an adoption cutoff, so the stale command left by a rate-limited or timed-out fire isn't re-adopted into an endless requeue loop. - Skip adoption instead of wedging the pump when the head-commit cutoff lookup fails with a non-rate-limit error. - Clear the feedback wait when a review round ends with real feedback (submitted review or bot comment), and prune deadline-expired waits in Pump, so autoreview-only flows don't leave "awaiting feedback" entries stuck forever. Claude-Session: https://claude.ai/code/session_01W4RViwpNDBmT2fs3qhQNx7
…state # Conflicts: # internal/crq/service_test.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78b86779f4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Pump now clears a finished round's feedback wait only once every bot in CRQ_REQUIRED_BOTS has submitted a review — mirroring what flips Feedback's ReviewedBy — instead of dropping it when the first configured bot responds. Dry-run pumps no longer prune expired waits, matching the no-state-mutation rule the adoption path follows. Claude-Session: https://claude.ai/code/session_01W4RViwpNDBmT2fs3qhQNx7
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 214575aa81
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
Once an in-flight slot is released on a bare bot reaction, no Pump path re-runs inflightStatus, so a later submitted review could never clear the persisted wait without a Loop. Pump now sweeps the oldest lingering wait per tick — one ListReviews call — and clears it when every awaited bot has reviewed the fired head. The required-bots completeness check now also matches reviews to the fired head (the same CommitID prefix check Feedback uses for ReviewedBy), so a delayed review for a different push can't complete a round it doesn't belong to. Claude-Session: https://claude.ai/code/session_01W4RViwpNDBmT2fs3qhQNx7
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21f98fe092
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Dry-run pumps now simulate the closed-PR drop and dedupe decisions without writing state — previously the dedupe branches ran before the dry-run guard and could remove queued items. Adoption now also bounds the cutoff by the PR's last head force-push (via the GraphQL timeline, best-effort): a force-push can point the PR at a commit object whose committer date predates commands made for an earlier head, and those must not be adopted for the new head. Claude-Session: https://claude.ai/code/session_01W4RViwpNDBmT2fs3qhQNx7
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91fdb0b4d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/crq/service.go`:
- Around line 488-503: The existingReviewCommand flow does extra GitHub calls
before checking whether any review-command comment actually exists, which wastes
quota on the common no-candidate path. Move the command scan and hasCandidate
check immediately after ListIssueComments and return false early when no
matching comment body is found, before calling GetPull, GetCommit, or
headForcePushCutoff. Keep the adoption behavior unchanged for the matching case
by preserving the existing cutoff and head/commit checks in
existingReviewCommand.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37c899d9-c73b-48dc-b9da-f61814680a0a
📒 Files selected for processing (2)
internal/crq/service.gointernal/crq/service_test.go
📜 Review details
🔇 Additional comments (10)
internal/crq/service_test.go (4)
5-25: LGTM!Also applies to: 145-147, 223-231
539-607: LGTM!Also applies to: 609-636, 637-683
742-791: LGTM!Also applies to: 793-839, 881-922, 924-966
967-1049: LGTM!Also applies to: 1051-1101, 1103-1136, 1138-1179
internal/crq/service.go (6)
65-85: LGTM!
168-180: LGTM!Also applies to: 202-205, 231-251
269-297: LGTM!Also applies to: 312-331
524-560: LGTM!
884-946: LGTM!Also applies to: 978-978, 990-996
1012-1048: LGTM!Also applies to: 1050-1096
botsReviewedHead now mirrors Feedback's ReviewedBy exactly: a required bot's review whose commit matches the fired head counts regardless of submission time, since the bot may have reviewed the commit before the round was triggered. The submitted-after gate now applies only when there is no head to match. Claude-Session: https://claude.ai/code/session_01W4RViwpNDBmT2fs3qhQNx7
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8210e0a182
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Action performedReview finished.
|
A regular push has no timestamped head-update event, and the new head's committer date can predate an old review command (a local commit pushed later), so no cutoff can prove such a command belongs to the current head. Instead, treat a configured-bot review submitted at/after the candidate command as proof the command's round already completed and skip adoption — the worst case is a duplicate command, the pre-adoption behavior. Claude-Session: https://claude.ai/code/session_01W4RViwpNDBmT2fs3qhQNx7
|
@coderabbitai review |
✅ Action performedReview finished.
|
…state # Conflicts: # skills/coderabbit-queue/SKILL.md
1 participant
Summary
crq loopfrom the persisted wait instead of refiring or treating fired dedupe as convergence.Validation
go test ./...Ref #12
Summary by CodeRabbit