Count CodeRabbit's completion reply toward loop convergence#17
Conversation
A re-review with nothing new to say produces no review object — CodeRabbit replies "Review finished" to the command and submits nothing — so Feedback's commit-checked ReviewedBy scan never flips and crq loop times out on a round that is complete. The completion reply is the missing signal. A bare issue comment carries no commit, so it only counts when crq's own state proves a command was fired for the current head (InFlight, History, or the live feedback wait) and the reply landed at/after that fire; rate-limit replies are excluded. Without a state anchor the pre-existing rule stands: issue comments never satisfy ReviewedBy. Claude-Session: https://claude.ai/code/session_01W4RViwpNDBmT2fs3qhQNx7
📝 WalkthroughWalkthroughAdds a ChangesCompletion-marker config
Completion-reply convergence
Service flow updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Service
participant Feedback
participant Store
participant IssueComments
Service->>Service: existingReviewCommand
Service->>Feedback: Feedback(repo, pr, head)
Feedback->>Store: load persisted wait state
Store-->>Feedback: fired time / awaiting entry
Feedback->>IssueComments: scan for completion replies
IssueComments-->>Feedback: matching comments
Feedback-->>Service: ReviewedBy / convergence state
Service->>Service: requiredFeedbackComplete / sweepFeedbackWaits
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 1b680e98cf
ℹ️ 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.
|
The configured bot also posts acknowledgement/skip/progress replies after a command; accepting any post-fire bot comment could converge a round that is still running. The fallback now matches a configurable CompletionMarker (CRQ_COMPLETION_MARKER, default "Review finished"); an empty marker disables the fallback. 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: 17434c02e4
ℹ️ 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.
|
An earlier round still finishing can post its "Review finished" after a new head's command fires, so a timestamp-only check could converge the new round before its review ran. The bot answers every command with exactly one auto-generated reply, so replies are now paired chronologically with the earliest unanswered command; only a completion whose paired command is the one fired for this round counts. Unpairable replies are ignored, failing toward the old wait-then-timeout behavior. Claude-Session: https://claude.ai/code/session_01W4RViwpNDBmT2fs3qhQNx7
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/config.go`:
- Line 95: The CompletionMarker config in config.go is treating an explicit
empty CRQ_COMPLETION_MARKER as unset because it uses stringEnv, so the default
“Review finished” always wins. Update the configuration lookup in the config
loading path to distinguish presence from value for CompletionMarker, or add a
helper that preserves empty strings, so CRQ_COMPLETION_MARKER="" stays empty
instead of falling back to the default.
In `@internal/crq/feedback.go`:
- Around line 201-240: The fallback completion-reply logic in Feedback is
correct but too dense; extract it into a dedicated helper to simplify
orchestration and improve testability. Move the anchor resolution and completion
scan block into a method such as s.applyCompletionReplyFallback, keeping the
current InFlight/History/AwaitingFeedback lookup and the configured-bot,
non-rate-limited, completion-marker, notBefore checks unchanged. Update Feedback
to call the helper and use the existing symbols like needsConfiguredBotReview,
feedbackWaitStart, s.store.Load, and s.isCompletionReply so the behavior stays
identical.
🪄 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: b2ffb2d9-4f7c-4e07-b816-5b8417409a37
📒 Files selected for processing (3)
internal/crq/config.gointernal/crq/feedback.gointernal/crq/feedback_test.go
📜 Review details
🔇 Additional comments (3)
internal/crq/config.go (1)
16-34: LGTM!internal/crq/feedback.go (1)
1007-1030: LGTM!internal/crq/feedback_test.go (1)
72-156: LGTM!Traced all five scenarios (fired-after, pre-fire, no-anchor, rate-limit, non-completion ack) against the implementation in
feedback.go— assertions match the intended semantics.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41b36830a2
ℹ️ 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".
| if strings.TrimSpace(c.Body) == command && !s.isConfiguredBot(c.User.Login) { | ||
| pending = append(pending, commentTime(c)) |
There was a problem hiding this comment.
When the same PR already contains an older @coderabbitai review that completed by submitting a review object (the existing inflightStatus path accepts that as done without requiring an issue-comment reply), this queues that old command in pending and nothing ever removes it. The current Review finished reply is then paired with the old timestamp, notBefore(cmdAt, firedAt) is false, and the configured bot remains unreviewed, so clean reruns still time out on PRs with earlier review commands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| if !firedAt.IsZero() && s.completionReplyForFiredCommand(issueComments, firedAt) { | ||
| markReviewed(report.ReviewedBy, s.cfg.Bot) |
There was a problem hiding this comment.
When a later regular push is queued with a head commit whose committer date predates an earlier CodeRabbit command, existingReviewCommand can adopt that earlier command because a no-findings round leaves no review object for it to reject. The fresh evidence is that recordExistingReviewPosted then records that old command under the new head, so this completion fallback accepts the old Review finished reply and marks the new head reviewed without ever firing a review for it; this can make crq loop converge stale feedback.
Useful? React with 👍 / 👎.
| if !firedAt.IsZero() && s.completionReplyForFiredCommand(issueComments, firedAt) { | ||
| markReviewed(report.ReviewedBy, s.cfg.Bot) |
There was a problem hiding this comment.
For autoreview-fired rounds that finish with only CodeRabbit's completion reply and no review object, there may be no Loop polling to call clearFeedbackWait; this branch only flips the transient FeedbackReport. Pump still clears/sweeps AwaitingFeedback from submitted reviews, so these completed no-finding rounds remain shown as waiting until the timeout even though the completion signal has arrived.
Useful? React with 👍 / 👎.
|
@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: 08b8febb14
ℹ️ 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".
| if len(pending) > 0 { | ||
| pending = pending[1:] |
There was a problem hiding this comment.
When an older @coderabbitai review submits a review object and then its auto-reply is posted/updated to Review finished after a newer command has been fired, this branch removes the old command from pending before the old auto-reply is processed. The old completion is then paired with the newer pending command, so completionReplyForFiredCommand can mark the new head reviewed before its own review has completed; the review should not make a later auto-reply available to satisfy a different command.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| case s.isConfiguredBot(c.User.Login) && s.isAutoReply(c.Body): | ||
| events = append(events, event{kind: eventAutoReply, at: commentTime(c), id: c.ID, comment: c}) |
There was a problem hiding this comment.
When a configured reviewer posts a completion-only reply that contains CRQ_COMPLETION_MARKER but not the CodeRabbit calibration footer (for example a custom bot, or a CodeRabbit reply whose hidden footer changes), this gate never records the comment as an auto-reply because it requires isAutoReply/CRQ_CAL_REPLY_MARKER. In that setup completionReplyForFiredCommand returns false and no-findings rounds still time out even though the operator configured a completion marker; the completion marker itself needs to qualify as a terminal reply.
Useful? React with 👍 / 👎.
| pending = pending[1:] | ||
| out = append(out, reviewCommandReply{ | ||
| commandID: cmd.ID, | ||
| commandAt: commentTime(cmd), | ||
| completion: s.isCompletionReply(ev.comment.Body) && !s.isRateLimited(ev.comment.Body), |
There was a problem hiding this comment.
If the bot emits a progress/ack auto-reply as a separate issue comment before the final Review finished reply, this pop removes the command even though the appended pair is marked completion: false. The later completion then finds no pending command and is ignored, so a clean no-findings round can sit in AwaitingFeedback until timeout; non-terminal auto-replies should not close the command unless they are the completion/rate-limit reply being handled.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/crq/service.go (1)
1111-1166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated clear-and-reload logic.
The "clear feedback wait, then reload state (falling back to the stale
stateon error)" block is duplicated verbatim in both exit branches (all-reviewed vs completion-reply). Extracting avoids future divergence if one path is updated without the other.♻️ Proposed refactor
+func (s *Service) clearFeedbackWaitAndReload(ctx context.Context, repo string, pr int, head string, state State) State { + s.clearFeedbackWait(ctx, repo, pr, head) + if updated, _, err := s.store.Load(ctx); err == nil { + return updated + } + return state +} + func (s *Service) sweepFeedbackWaits(ctx context.Context, state State) State { ... reviewedBy := reviewedByForRound(reviews, botSet(awaited), oldest.Head, oldest.StartedAt) if allReviewed(reviewedBy) { - s.clearFeedbackWait(ctx, oldest.Repo, oldest.PR, oldest.Head) - if updated, _, err := s.store.Load(ctx); err == nil { - return updated - } - return state + return s.clearFeedbackWaitAndReload(ctx, oldest.Repo, oldest.PR, oldest.Head, state) } ... if !s.completionReplyForFiredCommand(comments, reviews, oldest.StartedAt) { return state } - s.clearFeedbackWait(ctx, oldest.Repo, oldest.PR, oldest.Head) - if updated, _, err := s.store.Load(ctx); err == nil { - return updated - } - return state + return s.clearFeedbackWaitAndReload(ctx, oldest.Repo, oldest.PR, oldest.Head, state) }🤖 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.go` around lines 1111 - 1166, The clear-feedback-wait followed by store reload fallback logic is duplicated in sweepFeedbackWaits, in both the allReviewed and completionReply branches. Extract that repeated sequence into a small helper in Service (or a local helper) that calls clearFeedbackWait and then s.store.Load, returning the refreshed state when successful and the original state on error, and use it in both places to keep the behavior identical.
🤖 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/config_test.go`:
- Around line 103-114: In TestLoadConfigPreservesEmptyCompletionMarkerFromFile,
the t.Cleanup closure ignores errors from os.Setenv and os.Unsetenv, which trips
errcheck. Update the cleanup logic to handle the return values explicitly, using
the existing old/had environment-state handling in the test, and make sure any
failure restoring CRQ_COMPLETION_MARKER is reported through the test (for
example via t.Fatal/t.Errorf) rather than discarded.
---
Outside diff comments:
In `@internal/crq/service.go`:
- Around line 1111-1166: The clear-feedback-wait followed by store reload
fallback logic is duplicated in sweepFeedbackWaits, in both the allReviewed and
completionReply branches. Extract that repeated sequence into a small helper in
Service (or a local helper) that calls clearFeedbackWait and then s.store.Load,
returning the refreshed state when successful and the original state on error,
and use it in both places to keep the behavior identical.
🪄 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: 8e257626-45ff-4180-ad2a-40d67bafe00b
📒 Files selected for processing (6)
internal/crq/config.gointernal/crq/config_test.gointernal/crq/feedback.gointernal/crq/feedback_test.gointernal/crq/service.gointernal/crq/service_test.go
📜 Review details
🧰 Additional context used
🪛 golangci-lint (2.12.2)
internal/crq/config_test.go
[error] 110-110: Error return value of os.Setenv is not checked
(errcheck)
[error] 112-112: Error return value of os.Unsetenv is not checked
(errcheck)
🔇 Additional comments (7)
internal/crq/config.go (1)
34-34: LGTM! This correctly resolves the previously-flagged issue whereCRQ_COMPLETION_MARKER=""was indistinguishable from unset —stringEnvAllowEmptynow preserves an explicit empty value, matching the new tests inconfig_test.go.Also applies to: 95-95, 173-179
internal/crq/config_test.go (1)
4-4: LGTM!Also applies to: 90-102
internal/crq/feedback.go (2)
45-223: LGTM! The extraction intoapplyCompletionReplyFallback/completionFallbackFiredAt/reviewCommandRepliescorrectly resolves the previously-flagged density concern, and the FIFO command/reply pairing logic checks out against all three new test scenarios (fired-head completion, pre-fire rejection, cross-round rejection).Also applies to: 980-991, 1025-1136
992-1023: 🚀 Performance & ScalabilityNo redundant state load here.
Feedbackdoesn’t already thread a loadedStateintocompletionFallbackFiredAt, so this fallback lookup stands on its own.> Likely an incorrect or invalid review comment.internal/crq/feedback_test.go (1)
72-277: LGTM! Solid coverage of the completion-reply convergence contract, including the cross-round rejection and command-pairing edge cases called out in the PR objectives.internal/crq/service.go (1)
452-550: LGTM!reviewCommandHasCompletionReplyadoption guard, thereviewedByForRound/allReviewed/feedbackCompleteForRoundextraction, and therequiredFeedbackComplete/inflightStatussignature updates all check out against the new tests.Also applies to: 943-947, 961-993, 1000-1055
internal/crq/service_test.go (1)
843-893: LGTM! Fixtures correctly exercise the adoption-rejection, in-flight-clearing, and sweep-clearing paths added inservice.go.Also applies to: 1025-1072, 1271-1316
| func TestLoadConfigPreservesEmptyCompletionMarkerFromFile(t *testing.T) { | ||
| old, had := os.LookupEnv("CRQ_COMPLETION_MARKER") | ||
| if err := os.Unsetenv("CRQ_COMPLETION_MARKER"); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| t.Cleanup(func() { | ||
| if had { | ||
| os.Setenv("CRQ_COMPLETION_MARKER", old) | ||
| } else { | ||
| os.Unsetenv("CRQ_COMPLETION_MARKER") | ||
| } | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unchecked errors from os.Setenv/os.Unsetenv in cleanup.
golangci-lint's errcheck flags both calls in the t.Cleanup closure.
🔧 Proposed fix
t.Cleanup(func() {
if had {
- os.Setenv("CRQ_COMPLETION_MARKER", old)
+ if err := os.Setenv("CRQ_COMPLETION_MARKER", old); err != nil {
+ t.Errorf("failed to restore CRQ_COMPLETION_MARKER: %v", err)
+ }
} else {
- os.Unsetenv("CRQ_COMPLETION_MARKER")
+ if err := os.Unsetenv("CRQ_COMPLETION_MARKER"); err != nil {
+ t.Errorf("failed to unset CRQ_COMPLETION_MARKER: %v", err)
+ }
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestLoadConfigPreservesEmptyCompletionMarkerFromFile(t *testing.T) { | |
| old, had := os.LookupEnv("CRQ_COMPLETION_MARKER") | |
| if err := os.Unsetenv("CRQ_COMPLETION_MARKER"); err != nil { | |
| t.Fatal(err) | |
| } | |
| t.Cleanup(func() { | |
| if had { | |
| os.Setenv("CRQ_COMPLETION_MARKER", old) | |
| } else { | |
| os.Unsetenv("CRQ_COMPLETION_MARKER") | |
| } | |
| }) | |
| func TestLoadConfigPreservesEmptyCompletionMarkerFromFile(t *testing.T) { | |
| old, had := os.LookupEnv("CRQ_COMPLETION_MARKER") | |
| if err := os.Unsetenv("CRQ_COMPLETION_MARKER"); err != nil { | |
| t.Fatal(err) | |
| } | |
| t.Cleanup(func() { | |
| if had { | |
| if err := os.Setenv("CRQ_COMPLETION_MARKER", old); err != nil { | |
| t.Errorf("failed to restore CRQ_COMPLETION_MARKER: %v", err) | |
| } | |
| } else { | |
| if err := os.Unsetenv("CRQ_COMPLETION_MARKER"); err != nil { | |
| t.Errorf("failed to unset CRQ_COMPLETION_MARKER: %v", err) | |
| } | |
| } | |
| }) |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 110-110: Error return value of os.Setenv is not checked
(errcheck)
[error] 112-112: Error return value of os.Unsetenv is not checked
(errcheck)
🤖 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/config_test.go` around lines 103 - 114, In
TestLoadConfigPreservesEmptyCompletionMarkerFromFile, the t.Cleanup closure
ignores errors from os.Setenv and os.Unsetenv, which trips errcheck. Update the
cleanup logic to handle the return values explicitly, using the existing old/had
environment-state handling in the test, and make sure any failure restoring
CRQ_COMPLETION_MARKER is reported through the test (for example via
t.Fatal/t.Errorf) rather than discarded.
Source: Linters/SAST tools
1 participant
Problem
crq loopgates convergence onreviewed_by— a bot review object whose commit matches the exact head. When CodeRabbit processes an@coderabbitai reviewcommand but has nothing new to say (typical for trivial incremental pushes), it replies "Review finished" without submitting a review object, soreviewed_bystays false and the loop exits 2 (timeout) on a round that is in fact complete. This hit PRs #13, #15, and #16 during their review loops.Fix
Feedbacknow counts a configured-bot issue comment as satisfyingReviewedBy— but only under a strict anchor: crq's own state (InFlight,History, or the live feedback wait) must prove a review command was fired for the current head, and the reply must have landed at/after that fire. Rate-limit replies are excluded. Without a state anchor, the pre-existing rule stands: issue comments never satisfyReviewedBy, so a stale completion summary from an earlier head can't falsely converge a new round.Covered by tests for the completion, pre-fire-reply, no-anchor, and rate-limit-reply cases.
https://claude.ai/code/session_01W4RViwpNDBmT2fs3qhQNx7
Summary by CodeRabbit