diff --git a/internal/crq/feedback.go b/internal/crq/feedback.go index 2e93415..85d81b1 100644 --- a/internal/crq/feedback.go +++ b/internal/crq/feedback.go @@ -1158,22 +1158,31 @@ 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) } - // A contested decline deserves attention even when the finding's own severity - // is a nitpick, so floor an unknown severity at major. + // 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 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 := verdict.TitlePrefix() 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") + } +} 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 {