Stop announcing a reviewer's agreement as a rebuttal#40
Conversation
crq surfaces a bot's reply to a declined finding so a rebuttal is never buried. But it announced EVERY reply that was not an explicit withdrawal as "Reviewer contests your reply — re-address or reply again", including plain agreement — and an agent told to re-address a rebuttal that did not exist looped on the artifact instead of moving on. `dialect.IsReviewFindingRetained` already exists for exactly this and was called only from a golden test, never in production. It is now what decides the claim: - a reply that really retains the finding → reported as a contest; - anything else → reported as a reply to read and confirm. Visibility is deliberately unchanged. Both keep the same major floor, because a possible rebuttal that gets buried is the worse failure — only the assertion about what the reply *means* was wrong. Also adds the regression test for a property that has no test today: a "Review skipped" refusal must never block the account. That notice ships with the rate-limit marker embedded, and reading it as a timed block was catastrophic rather than merely wrong — the block is account-WIDE, nothing about waiting clears "this PR has 119 files", and every re-fire re-read the notice and re-blocked, so one oversized PR stalled review for every repository. The classifier discriminates them; the cost of a regression lands in the queue, so the test asserts the consequence: no block is recorded, and an unrelated PR still fires. While verifying, two items from the plan turned out to be already fixed and needed no change: the refusal misclassification (PR #35, with a golden row whose comment describes this failure mode) and the co-review wait's evidence floor, which already anchors on when the head appeared rather than when crq noticed it.
📝 WalkthroughWalkthroughThe PR differentiates rebuttal finding titles based on retained replies and adds replay coverage ensuring skipped review refusals do not block unrelated pull requests at the account level. ChangesRebuttal title classification
Replay refusal isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/feedback.go`:
- Around line 1179-1182: Move the user-facing title templates currently assigned
in threadRebuttal to an allowed lower-layer package, and have threadRebuttal
call that package for the title based on IsReviewFindingRetained(last.Body).
Keep threadRebuttal limited to classification and finding assembly, with no bot
wording or duplicated decision logic in internal/crq.
🪄 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: de83eb9a-dff3-41b6-99bd-d46ef8c58dcd
📒 Files selected for processing (3)
internal/crq/feedback.gointernal/crq/feedback_test.gointernal/crq/replay_test.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
internal/crq/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
internal/crq/**/*.go: Keepinternal/crqorchestration-only; it owns service flow and wires packages but must not contain bot wording or duplicate engine decision logic.
Use the observe → decide → apply flow: observation is built once, engine decisions remain pure, andcrq/service.gois the only effects executor for CAS writes andPostIssueComment.
DryRunmust short-circuit apply by reporting without writing state or posting comments.
Keepcrq loopexit codes frozen: 0 for converged/skipped, 10 for findings, and 2 for timeout.
Files:
internal/crq/replay_test.gointernal/crq/feedback.gointernal/crq/feedback_test.go
internal/{dialect,engine,state,crq,gh}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Maintain the dependency direction
dialect ← engine ← crq,state ← crq, andgh ← {state, crq}; do not introduce dependency cycles.
Files:
internal/crq/replay_test.gointernal/crq/feedback.gointernal/crq/feedback_test.go
🔇 Additional comments (2)
internal/crq/replay_test.go (1)
709-773: LGTM!internal/crq/feedback_test.go (1)
1934-1935: LGTM!Also applies to: 1952-1963
Both reviewers flagged the title templates sitting in internal/crq, and they are right — I had declined the same point at preflight on the grounds that dialect owns bot wording rather than crq's own output. That reasoning does not hold: dialect already derives finding titles (TitleOf) and reason strings, and the package rule is that internal/crq is orchestration only. So the classification and the words describing it now travel together. `dialect.ClassifyDeclineReply` returns a `ReplyVerdict` — withdrawn, retained or unclear — and the verdict carries its own `TitlePrefix`. crq reads the verdict and assembles the finding; it no longer decides how a verdict reads. This also removes the shape that caused the bug: two independent boolean calls, where consulting only the first meant every non-withdrawal reply was announced as a contest. A single verdict cannot be half-classified. The golden rows now assert the verdict for every captured reply and that only a stated rebuttal claims to contest, so the wording cannot drift from the classification it describes.
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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.
|
1 participant
Problem
crq surfaces a bot's reply to a declined finding so a rebuttal is never buried. But it announced every reply that was not an explicit withdrawal as
including plain agreement. An agent told to re-address a rebuttal that did not exist looped on the artifact instead of moving on. From a real session:
dialect.IsReviewFindingRetainedalready existed for exactly this discrimination, and was called only from a golden test — never in production.Change
The retained classifier now decides the claim:
Visibility is deliberately unchanged. Both keep the same
majorfloor, because a possible rebuttal that gets buried is the worse failure. Only the assertion about what the reply means was wrong.Also: a regression test for a fleet-wide property
CodeRabbit's "Review skipped" refusal ships with the rate-limit marker embedded, and reading it as a timed block was catastrophic rather than merely wrong: the block is account-wide, nothing about waiting clears "this PR has 119 files", and every re-fire re-read the same notice and re-blocked. One oversized PR stalled review for every repository.
The classifier discriminates them today, but the cost of a regression lands in the queue rather than in classification, so this asserts the consequence: after observing the refusal no account block exists, and an unrelated PR still fires. It also asserts the refusal was actually processed, so the test cannot pass vacuously.
Verified, not changed
Two items this PR was scoped to fix turned out to be already fixed, and are recorded here so they are not re-investigated:
Tests
gofmt -l .,go vet ./...,go test ./...clean. Local CodeRabbit preflight clean after addressing its findings.Summary by CodeRabbit
Bug Fixes
Tests
Ref #42.