From 43f4c1550b946f04e8b72406c749394d9f8b700b Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 13:38:32 +0200 Subject: [PATCH 1/2] Stop announcing a reviewer's agreement as a rebuttal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/feedback.go | 18 ++++++++-- internal/crq/feedback_test.go | 10 ++++++ internal/crq/replay_test.go | 65 +++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index 2e93415..224962a 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -1165,15 +1165,27 @@ func threadRebuttal(thread reviewThread, bots map[string]struct{}) *dialect.Find return nil // a platform notice or ack, not a rebuttal (e.g. Codex's // "create an environment" boilerplate posted as a thread reply) } - // A contested decline deserves attention even when the finding's own severity - // is a nitpick, so floor an unknown severity at major. + // Say which of the two this actually is. Everything that was not a clear + // withdrawal used to be announced as a contest, so a bot AGREEING with the + // decline was reported as standing its ground — and an agent, told to + // re-address a rebuttal that did not exist, looped on the artifact. + // + // Surfacing an ambiguous reply is still right — a buried rebuttal is the + // worse failure — and it keeps the same major floor, so visibility is + // unchanged. Only the CLAIM is corrected: an unclassified reply is reported + // as something to read, and a contest is asserted only when the reply really + // retains the finding. severity := dialect.FloorSeverity(dialect.SeverityOf(last.Body), "major") + title := "Reviewer replied after your decline — read it and confirm the decline stands: " + if dialect.IsReviewFindingRetained(last.Body) { + title = "Reviewer contests your reply — re-address or reply again: " + } return &dialect.Finding{ Bot: last.Author.Login, Severity: severity, Path: firstNonEmpty(thread.Path, last.Path), Line: firstPositive(thread.Line, last.Line, last.OriginalLine), - Title: "Reviewer contests your reply — re-address or reply again: " + dialect.TitleOf(last.Body), + Title: title + dialect.TitleOf(last.Body), Body: strings.TrimSpace(last.Body), ThreadID: thread.ID, CommentID: last.DatabaseID, diff --git a/internal/crq/feedback_test.go b/internal/crq/feedback_test.go index eaa08a3..b28682c 100644 --- a/internal/crq/feedback_test.go +++ b/internal/crq/feedback_test.go @@ -1931,6 +1931,8 @@ func TestThreadRebuttalSurfacesContestedResolvedThreads(t *testing.T) { t.Fatal("a contested bot reply on a resolved thread must surface") } else if got.Source != "review_reply" || got.ThreadID != "PRRT_x" || got.CommentID != 3 { t.Fatalf("rebuttal finding mismatch: %#v", got) + } else if !strings.Contains(got.Title, "contests your reply") { + t.Errorf("a reply that really retains the finding must be reported as a contest, got %q", got.Title) } // Withdrawn rebuttal → not surfaced. @@ -1947,10 +1949,18 @@ func TestThreadRebuttalSurfacesContestedResolvedThreads(t *testing.T) { addThreadComment(&th, 1, "coderabbitai", "**Nitpick** rename this") addThreadComment(&th, 2, "kristofferR", "Declined: name is intentional.") addThreadComment(&th, 3, "coderabbitai", "Here is some additional context on the naming convention.") + // It surfaces at the same severity — never bury a possible rebuttal — but it + // must NOT be announced as a contest. Asserting one for every non-withdrawal + // reply meant a bot AGREEING with the decline read as standing its ground, + // and an agent told to re-address a rebuttal that did not exist looped on it. if got := threadRebuttal(th, bots); got == nil { t.Fatal("an ambiguous (non-withdrawal) reply must surface by default") } else if got.Severity != "major" { t.Fatalf("an unknown-severity rebuttal must floor at major, got %q", got.Severity) + } else if strings.Contains(got.Title, "contests your reply") { + t.Errorf("an ambiguous reply must not be asserted as a contest, got %q", got.Title) + } else if !strings.Contains(got.Title, "read it") { + t.Errorf("an ambiguous reply should tell the caller to read it, got %q", got.Title) } // No agent reply (just the bot's finding) → not a rebuttal. diff --git a/internal/crq/replay_test.go b/internal/crq/replay_test.go index ac4a3c9..ecbd9a3 100644 --- a/internal/crq/replay_test.go +++ b/internal/crq/replay_test.go @@ -706,3 +706,68 @@ func TestReplayEmptyReviewShellDoesNotConverge(t *testing.T) { t.Fatalf("real review must converge: %+v", report.ReviewedBy) } } + +// --- a refusal is not a rate limit ----------------------------------------- + +// TestReplaySkippedRefusalNeverBlocksTheAccount pins the fleet-wide property +// behind CodeRabbit's "Review skipped" refusal. +// +// 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 same notice and re-blocked. One oversized PR stalled review +// for every repository. +// +// The classifier discriminates them (TestGoldenClassification), but the cost of +// a regression lands here, so assert the consequence and not just the reading: +// after observing the refusal, no account block exists and an unrelated PR still +// fires. +func TestReplaySkippedRefusalNeverBlocksTheAccount(t *testing.T) { + base := time.Date(2026, 7, 26, 9, 0, 0, 0, time.UTC) + f := newReplayFixture(t, base) + oversized, other := 700, 701 + f.openPull("owner/repo", oversized, "aaaaaaaa1") + f.setCommitDate("aaaaaaaa1", base.Add(-time.Minute)) + f.openPull("owner/repo", other, "bbbbbbbb2") + f.setCommitDate("bbbbbbbb2", base.Add(-time.Minute)) + + f.enqueue("owner/repo", oversized) + if res := f.pump(); res.Action != "fired" { + t.Fatalf("the oversized PR should fire once, got %#v", res) + } + + // CodeRabbit refuses the head. The body carries the rate-limit marker. + f.clk.advance(time.Minute) + f.botComment("owner/repo", oversized, 900, + corpusMessage(t, "coderabbit/review-skipped-too-many-files.md"), f.clk.now()) + f.pump() + + // Prove the refusal was actually observed. Without this the assertions below + // would also pass if nothing had happened at all — and "no block" is only + // meaningful once the notice that used to create one has been read. + if r := f.round("owner/repo", oversized); r == nil || r.Phase == PhaseFired { + t.Fatalf("the refusal must move the round off fired, got %#v", r) + } + + st, _, err := f.store.Load(f.ctx) + if err != nil { + t.Fatal(err) + } + if st.Account.BlockedUntil != nil { + t.Fatalf("a refusal must not block the account: BlockedUntil = %s", st.Account.BlockedUntil) + } + + // And the block being absent has to mean something: an unrelated PR fires. + f.clk.advance(2 * time.Minute) + f.enqueue("owner/repo", other) + for i := 0; i < 4; i++ { + if f.reviewsPosted("owner/repo", other) > 0 { + break + } + f.clk.advance(time.Minute) + f.pump() + } + if got := f.reviewsPosted("owner/repo", other); got == 0 { + t.Fatal("an unrelated PR must still fire: one oversized PR cannot stall the fleet") + } +} From ae9d982a5de86c1eb80af705033d1287d116abaa Mon Sep 17 00:00:00 2001 From: kristofferR Date: Sun, 26 Jul 2026 14:33:26 +0200 Subject: [PATCH 2/2] Keep the verdict's wording with the classifier MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/crq/feedback.go | 19 +++++++--------- internal/dialect/golden_test.go | 12 ++++++++++ internal/dialect/reply.go | 40 +++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 11 deletions(-) diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index 224962a..85d81b1 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -1158,28 +1158,25 @@ func threadRebuttal(thread reviewThread, bots map[string]struct{}) *dialect.Find if !agentReplied { return nil // the bot is talking to itself, not answering a decline } - if dialect.IsReviewFindingWithdrawn(last.Body) { + verdict := dialect.ClassifyDeclineReply(last.Body) + if verdict == dialect.ReplyWithdrawn { return nil // conceded — the decline stands } if dialect.IsNonActionableText(last.Body) { return nil // a platform notice or ack, not a rebuttal (e.g. Codex's // "create an environment" boilerplate posted as a thread reply) } - // Say which of the two this actually is. Everything that was not a clear + // The verdict decides how this reads. Everything that was not a clear // withdrawal used to be announced as a contest, so a bot AGREEING with the // decline was reported as standing its ground — and an agent, told to // re-address a rebuttal that did not exist, looped on the artifact. // - // Surfacing an ambiguous reply is still right — a buried rebuttal is the - // worse failure — and it keeps the same major floor, so visibility is - // unchanged. Only the CLAIM is corrected: an unclassified reply is reported - // as something to read, and a contest is asserted only when the reply really - // retains the finding. + // Surfacing an unclear reply is still right — a buried rebuttal is the worse + // failure — and it keeps the same major floor, so visibility is unchanged. + // Only the CLAIM is corrected, and the wording for it lives with the + // classifier rather than here. severity := dialect.FloorSeverity(dialect.SeverityOf(last.Body), "major") - title := "Reviewer replied after your decline — read it and confirm the decline stands: " - if dialect.IsReviewFindingRetained(last.Body) { - title = "Reviewer contests your reply — re-address or reply again: " - } + title := verdict.TitlePrefix() return &dialect.Finding{ Bot: last.Author.Login, Severity: severity, diff --git a/internal/dialect/golden_test.go b/internal/dialect/golden_test.go index 0a3b6c3..cdae705 100644 --- a/internal/dialect/golden_test.go +++ b/internal/dialect/golden_test.go @@ -257,6 +257,18 @@ func TestGoldenReplyVerdict(t *testing.T) { if got := IsReviewFindingRetained(body); got != tc.retained { t.Errorf("IsReviewFindingRetained = %v, want %v", got, tc.retained) } + // The verdict is what callers act on, and its wording travels with it: + // only a stated rebuttal may claim to contest the decline. + verdict := ClassifyDeclineReply(body) + switch { + case tc.withdrawn && verdict != ReplyWithdrawn: + t.Errorf("ClassifyDeclineReply = %v, want ReplyWithdrawn", verdict) + case tc.retained && verdict != ReplyRetained: + t.Errorf("ClassifyDeclineReply = %v, want ReplyRetained", verdict) + } + if contests := strings.Contains(verdict.TitlePrefix(), "contests"); contests != (verdict == ReplyRetained) { + t.Errorf("TitlePrefix for %v claims contest=%v", verdict, contests) + } }) } } diff --git a/internal/dialect/reply.go b/internal/dialect/reply.go index a6d8e85..2b933d9 100644 --- a/internal/dialect/reply.go +++ b/internal/dialect/reply.go @@ -16,6 +16,46 @@ import "strings" // a rebuttal re-surfaces a settled finding and blocks convergence. const reviewWithdrawnMarker = "" +// ReplyVerdict is what a bot's reply to a declined finding amounts to. It exists +// so the CLASSIFICATION and the wording that describes it stay together: the +// caller orchestrates, and does not decide how a verdict reads. +type ReplyVerdict int + +const ( + // ReplyUnclear is neither a withdrawal nor a stated rebuttal. It is still + // surfaced — a buried rebuttal is the worse failure — but it must not be + // announced as a contest, which is what made an agent re-address a rebuttal + // that did not exist. + ReplyUnclear ReplyVerdict = iota + // ReplyWithdrawn concedes: the decline stands and the thread is done. + ReplyWithdrawn + // ReplyRetained contests: the finding stands and the agent must answer it. + ReplyRetained +) + +// ClassifyDeclineReply reads a bot's reply to a declined finding. +func ClassifyDeclineReply(text string) ReplyVerdict { + switch { + case IsReviewFindingWithdrawn(text): + return ReplyWithdrawn + case IsReviewFindingRetained(text): + return ReplyRetained + default: + return ReplyUnclear + } +} + +// TitlePrefix is how a verdict introduces itself in a finding title. Only a +// stated rebuttal claims to contest; anything else asks the caller to read it. +func (v ReplyVerdict) TitlePrefix() string { + switch v { + case ReplyRetained: + return "Reviewer contests your reply — re-address or reply again: " + default: + return "Reviewer replied after your decline — read it and confirm the decline stands: " + } +} + // IsReviewFindingWithdrawn reports whether a bot's reply concedes and withdraws // its finding — the agent's decline stands and the thread is done. func IsReviewFindingWithdrawn(text string) bool {