Skip to content

fix: report disabled primary accurately - #63

Merged
kristofferR merged 2 commits into
mainfrom
fix/primary-off-messaging
Aug 14, 2026
Merged

fix: report disabled primary accurately#63
kristofferR merged 2 commits into
mainfrom
fix/primary-off-messaging

Conversation

@kristofferR

@kristofferRkristofferR commented Aug 1, 2026

Copy link
Copy Markdown
Owner

When a repository disabled its primary reviewer, co-reviewer-only rounds still logged that CodeRabbit had already reviewed the head. That wording repeatedly led agents to infer that CodeRabbit worked on repositories where it was intentionally disabled.

Use the engine decision reason in orchestration logs and distinguish an explicitly disabled primary from a skipped or unavailable review. The primary-off engine test now pins the user-facing reason.

Validated with go test ./... and go vet ./.... Local CodeRabbit preflight was attempted but account-rate-limited with no findings.

Implementation environment: GPT-5.6 Sol (xhigh) through the Codex harness.

Summary by CodeRabbit

  • Bug Fixes
    • Improved review decision messaging when the primary reviewer is disabled.
    • Co-review requirements are now clearly identified in decision reasons and logs.
    • Preserved accurate handling of other primary-review availability issues.

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 41617834-0207-423b-b901-d9a093c276c1

📥 Commits

Reviewing files that changed from the base of the PR and between 148f639 and d4d6ddd.

📒 Files selected for processing (1)
  • internal/crq/service.go
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (3)
internal/{dialect,engine,crq,gh,state,workspace}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Preserve the dependency direction dialect ← engine ← crq, state ← crq, gh ← {state, crq}, and workspace ← crq; packages must not introduce dependency cycles.

Files:

  • internal/crq/service.go
internal/crq/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

internal/crq/**/*.go: Keep internal/crq limited to orchestration and effects application; CAS state writes and PostIssueComment belong in the apply phase, while observation and decisions remain in their designated layers.
Keep DryRun from performing apply effects: it must report without writing state or posting issue comments.

Files:

  • internal/crq/service.go
internal/{engine,crq}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Do not mix GitHub REST quota (Throttle) with CodeRabbit account quota (AccountQuota or account-blocked state); the literal phrase rate limit is reserved for gh and dialect.

Files:

  • internal/crq/service.go
🧠 Learnings (3)
📚 Learning: 2026-07-27T01:11:18.244Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/feedback.go:91-92
Timestamp: 2026-07-27T01:11:18.244Z
Learning: When reviewing code in `internal/crq` that deals with bot configuration, keep the separation of concerns intact: `Config.isConfiguredBot(login)` should only be evaluated against the fleet-wide primary `Config.Bot`. Do not override or reinterpret `Config.Bot` based on repo-specific reviewer/co-reviewer data. If repo-added co-reviewers are involved, they must be handled via repo-derived bot sets (e.g., `Config.evidenceBots()`), and not via `isConfiguredBot`. `Config.ForRepo(...)` should adjust co-reviewer-derived fields like `CoBots`, `RequiredBots`, `Reviewers`, and `FeedbackBots`, but intentionally not change the meaning of `Config.Bot` used by `isConfiguredBot`.

Applied to files:

  • internal/crq/service.go
📚 Learning: 2026-07-27T01:11:20.071Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/repoconfig.go:163-183
Timestamp: 2026-07-27T01:11:20.071Z
Learning: In internal/crq, treat the “observe → decide → apply” effects restriction as applying only to effects produced by review decisions. Explicit operator mutation commands (e.g., Service.SetReviewers, Service.ClearReviewers, enqueue/cancel flows) are allowed to perform state mutations outside the standard apply path, so review logic should not incorrectly require them to follow observe/decide/apply.
For CRQ_DRY_RUN: it should suppress review requests and fire-record writes, but it must not make explicit configuration/mutation commands (e.g., `crq reviewers set`) silently succeed without persisting the requested changes.
Ensure Service.applyFire revalidates repository reviewer overrides just before posting, to protect against stale/changed overrides since earlier reads.

Applied to files:

  • internal/crq/service.go
📚 Learning: 2026-07-28T06:12:02.107Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 58
File: internal/crq/service.go:2113-2116
Timestamp: 2026-07-28T06:12:02.107Z
Learning: In the internal/crq package, treat `rate-limit-command` and `gate-repo` as host-local configuration (not fleet-owned settings). `Service.readQuota` may accept a fleet-derived `Config` only to use the fleet-owned `Scope` and `CalibrationTTL`. Ensure the calibration probe command and any gate-repository logic continue to use the host-local service configuration, not the fleet-owned configuration.

Applied to files:

  • internal/crq/service.go
🔇 Additional comments (1)
internal/crq/service.go (1)

1513-1518: LGTM!

Also applies to: 1664-1668


📝 Walkthrough

Walkthrough

The fire decision now identifies disabled primary reviewers separately from unavailable primary reviews. Co-review-only documentation and logs include the decision reason and posted co-reviewers. The engine test verifies the explicit disabled-primary reason.

Changes

Primary-disabled co-review handling

Layer / File(s)Summary
Decision reason and co-review reporting
internal/engine/fire.go, internal/engine/engine_test.go, internal/crq/service.go
coAwareDedupe returns a distinct disabled-primary reason. The test verifies the reason. fireCoOnly documents the supported conditions and logs the reason with posted co-reviewers.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

Poem

A rabbit reads the reason clear,
“Primary disabled” now appears.
Co-reviewers hop and logs explain,
The test confirms the change again.
🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title clearly summarizes the primary change: accurately reporting when the primary reviewer is disabled.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/primary-off-messaging

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kristofferR

Copy link
Copy Markdown
OwnerAuthor

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit:148f63930e

ℹ️ 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".

@kristofferR

Copy link
Copy Markdown
OwnerAuthor

@coderabbitai review

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/service.go`:
- Around line 1513-1518: Update the comment for fireCoOnly to explicitly include
primaries unavailable for the current head alongside finished or disabled
primaries, matching the apply-layer behavior described by coAwareDedupe.
Preserve the existing documentation of gating co-reviewers, trigger claims, and
FireSlot handling.
- Line 1664: Update the logging statement in the co-reviewer trigger flow to
derive the listed reviewers from the successfully posted triggers in posts,
rather than all claims in claimed. Preserve the existing log format and
reason/context while ensuring failed postCoTrigger attempts are excluded.
🪄 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 Plus

Run ID: 551b6ec9-3ec9-4df7-8271-5dcfa4ccaa1c

📥 Commits

Reviewing files that changed from the base of the PR and between 09dc94a and 148f639.

📒 Files selected for processing (3)
  • internal/crq/service.go
  • internal/engine/engine_test.go
  • internal/engine/fire.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
internal/{dialect,engine,crq,gh,state,workspace}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Preserve the dependency direction dialect ← engine ← crq, state ← crq, gh ← {state, crq}, and workspace ← crq; packages must not introduce dependency cycles.

Files:

  • internal/engine/fire.go
  • internal/crq/service.go
  • internal/engine/engine_test.go
internal/engine/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

internal/engine/**/*.go: Keep internal/engine pure decision logic: pass time explicitly, perform no I/O, and do not depend on context or GitHub transport.
DecideFire is the single owner of fire decisions, and Completion is the single definition of whether a round is done; fire guards must remain consolidated there.

Files:

  • internal/engine/fire.go
  • internal/engine/engine_test.go
internal/{engine,crq}/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

Do not mix GitHub REST quota (Throttle) with CodeRabbit account quota (AccountQuota or account-blocked state); the literal phrase rate limit is reserved for gh and dialect.

Files:

  • internal/engine/fire.go
  • internal/crq/service.go
  • internal/engine/engine_test.go
internal/crq/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

internal/crq/**/*.go: Keep internal/crq limited to orchestration and effects application; CAS state writes and PostIssueComment belong in the apply phase, while observation and decisions remain in their designated layers.
Keep DryRun from performing apply effects: it must report without writing state or posting issue comments.

Files:

  • internal/crq/service.go
internal/engine/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

Table-test convergence and fire rules in engine_test.go.

Files:

  • internal/engine/engine_test.go
🧠 Learnings (3)
📚 Learning: 2026-07-27T01:11:18.244Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/feedback.go:91-92
Timestamp: 2026-07-27T01:11:18.244Z
Learning: When reviewing code in `internal/crq` that deals with bot configuration, keep the separation of concerns intact: `Config.isConfiguredBot(login)` should only be evaluated against the fleet-wide primary `Config.Bot`. Do not override or reinterpret `Config.Bot` based on repo-specific reviewer/co-reviewer data. If repo-added co-reviewers are involved, they must be handled via repo-derived bot sets (e.g., `Config.evidenceBots()`), and not via `isConfiguredBot`. `Config.ForRepo(...)` should adjust co-reviewer-derived fields like `CoBots`, `RequiredBots`, `Reviewers`, and `FeedbackBots`, but intentionally not change the meaning of `Config.Bot` used by `isConfiguredBot`.

Applied to files:

  • internal/crq/service.go
📚 Learning: 2026-07-27T01:11:20.071Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 57
File: internal/crq/repoconfig.go:163-183
Timestamp: 2026-07-27T01:11:20.071Z
Learning: In internal/crq, treat the “observe → decide → apply” effects restriction as applying only to effects produced by review decisions. Explicit operator mutation commands (e.g., Service.SetReviewers, Service.ClearReviewers, enqueue/cancel flows) are allowed to perform state mutations outside the standard apply path, so review logic should not incorrectly require them to follow observe/decide/apply.
For CRQ_DRY_RUN: it should suppress review requests and fire-record writes, but it must not make explicit configuration/mutation commands (e.g., `crq reviewers set`) silently succeed without persisting the requested changes.
Ensure Service.applyFire revalidates repository reviewer overrides just before posting, to protect against stale/changed overrides since earlier reads.

Applied to files:

  • internal/crq/service.go
📚 Learning: 2026-07-28T06:12:02.107Z
Learnt from: kristofferR
Repo: kristofferR/coderabbit-queue PR: 58
File: internal/crq/service.go:2113-2116
Timestamp: 2026-07-28T06:12:02.107Z
Learning: In the internal/crq package, treat `rate-limit-command` and `gate-repo` as host-local configuration (not fleet-owned settings). `Service.readQuota` may accept a fleet-derived `Config` only to use the fleet-owned `Scope` and `CalibrationTTL`. Ensure the calibration probe command and any gate-repository logic continue to use the host-local service configuration, not the fleet-owned configuration.

Applied to files:

  • internal/crq/service.go
🔇 Additional comments (2)
internal/engine/fire.go (1)

344-348: LGTM!

internal/engine/engine_test.go (1)

1236-1238: LGTM!

Comment threadinternal/crq/service.go Outdated
Comment threadinternal/crq/service.go Outdated
@kristofferR

Copy link
Copy Markdown
OwnerAuthor

@coderabbitai review

@kristofferR

Copy link
Copy Markdown
OwnerAuthor

@codex review

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit:d4d6ddd50f

ℹ️ 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".

@kristofferR

Copy link
Copy Markdown
OwnerAuthor

@coderabbitai review

@coderabbitai

coderabbitaiBot commented Aug 1, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@kristofferR
kristofferR merged commit 8c45ced into mainAug 14, 2026
2 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@kristofferR