feat(council): several reviewers on one lane, merged monotonically - #88
Conversation
Off by default. The machinery for running more than one reviewer over a lane's evidence, and folding what they say into one review. It is deliberately *not* a second opinion — `src/falsify` is that, and its README explains why asking a second model "are these correct?" deletes the best half of a review. A council runs the other way: more reviewers so more is found, with agreement used only to rank what comes back. **Agreement raises confidence and never gates posting.** A quorum is the falsification failure in a different hat, and worse here, because the architecture is built so reviewers *do not* overlap — `ISOLATION_CLAUSE` exists precisely to stop N conversations reporting the same cross-file problem. The reviewer best placed to find something is often the only one who can see it, so the merge is monotone: no finding is ever worse off for the council having run. Corroboration raises confidence by noisy-OR (capped at 0.99) and breaks ties last in `lane_proposal`'s sort, so it survives `max_comments` truncation. **Grouping is not the fingerprint.** `Finding::fingerprint` hashes `rule`, which is model-authored free text — two reviewers on one missing bounds check write `unchecked-index` and `missing-bounds-check`, and grouping on it would post both. `agree::corroborates` uses same lane, same file, ranges overlapping within three lines, and never compares wording. That looseness is used only for grouping: `identity` stays as `anchor::stamp` computed it and a merge keeps the *first* sighting's, because it is what the posted marker carries and what suppression reads back. **The representative is verbatim** — highest confidence then severity, taken whole. A merge step that can author text is a second reviewer nobody gated. Severity is the highest anyone assigned: merging must not talk a review down. **Personas are names, never text.** `config::remote` already keeps repository prose out of the instruction position; a settable persona would be a second door, so they are `&'static str` selected by name and an unknown name is a configuration error. `[council]` is not overridable at all — every key spends the operator's money or decides what a model is told. Falsification now runs **once over the merged set** rather than per reviewer: a reject-only filter given more inputs in one pass has identical semantics at a fraction of the calls. A bug worth recording, because `tinysweeper eval` is what caught it: the first draft merged findings *within* one reviewer's output as well as across reviewers, so two findings three lines apart silently became one — a behaviour change at a single agent, which destroys the property the rollout rests on. The committed corpus failed to replay on `ts-0068`, where critique legitimately raises two findings on adjacent lines. Only earlier reviewers may absorb a later finding now, at most once each. Verified: fmt, clippy --all-targets -D warnings, `cargo test --locked` (1284 offline tests, 32 new), `cargo check --all-features --all-targets`. The committed corpus replays byte-identically against its existing cassettes, which are keyed on full prompt text — an empirical proof that the disabled path is unchanged. A live two-agent council over a one-file range: $0.0007, 88% cache hit despite the per-persona prefix split. Co-authored-by: Medulla <medulla@tinyhumans.ai>
|
Warning Review limit reached
Next review available in: 16 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (29)
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 |
Off by default. The machinery for running more than one reviewer over a lane's evidence and folding what they say into one review — the third slice of the agent-council plan, after
local-review(#85) and the eval harness (#86).It is not a second opinion
src/falsifyis that, and its README explains why asking a second model "are these correct?" deletes the best half of a review. A council runs the other way: more reviewers so that more is found, with agreement used only to rank what comes back.Agreement raises confidence. It never gates posting.
The obvious design is a quorum — post when two of three agree — and it is the falsification failure in a different hat. Worse here, because the architecture is deliberately built so reviewers don't overlap:
ISOLATION_CLAUSEexists precisely to stop N conversations reporting the same cross-file problem. A reviewer with a different angle (or later, a tool belt the others lack) finds things the others structurally cannot see, and gating on agreement deletes exactly those.So the merge is monotone — no finding is ever worse off for the council having run, asserted as a property test over a grid of confidences. Corroboration raises confidence by noisy-OR, capped at 0.99 because three models agreeing is not a certainty any of them claimed, and breaks ties last in
lane_proposal's sort so a corroborated finding survivesmax_commentstruncation without outranking a more serious one.Grouping deliberately is not the fingerprint
Finding::fingerprinthashesrule, which is model-authored free text — two reviewers on one missing bounds check writeunchecked-indexandmissing-bounds-check, and grouping on it would post both.agree::corroboratesuses same lane, same file, ranges overlapping within three lines, and never compares wording (two agents describing one defect word it differently by construction — that's the point of running more than one).That looseness is used only for grouping.
Finding::identitystays exactly asanchor::stampcomputed it, and a merge keeps the first sighting's identity — it is what thetinysweeper:fp=marker carries and what suppression reads back, so letting the representative bring its own would repost a finding already answered.The representative is verbatim
Highest confidence then severity, taken whole. Nothing blended or rewritten — a merge step that can author text is a second reviewer nobody gated. Severity is the highest anyone assigned: merging must not talk a review down.
What the eval harness caught
Worth calling out, because it's the first time the corpus paid for itself. The first draft merged findings within one reviewer's output as well as across reviewers, so two findings three lines apart silently became one — a behaviour change at a single agent, which destroys the property the entire rollout rests on.
cargo testfailed onts-0068, where the critique lane legitimately raises two findings on adjacent lines of one file. Now only earlier reviewers may absorb a later finding, at most once each, with a regression test naming the case.Security surface
[council]is not overridable by a reviewed repository (council_keys_are_not_overridable_by_a_reviewed_repository, plus a test that a repo config trying to convene a council changes nothing and says so). Every key spends the operator's money or decides what a model is told.config::remotealready keeps repository prose out of the instruction position; a settable persona would be a second door. They're&'static strincouncil::persona, and an unknown name is a configuration error fromtinysweeper check.Cost
Falsification now runs once over the merged set rather than per reviewer — a reject-only filter given more inputs in one pass has identical semantics at a fraction of the calls. Each agent gets its own cache stream because the persona sits in the prefix; measured, that costs much less than expected.
Verification
The disabled path is proved unchanged empirically, not just argued. The committed
evals/corpus replays byte-identically against its existing cassettes, and those are keyed on full prompt text — a prompt that had moved by one byte would miss.Live two-agent council (
correctness+integration) over a one-file range:88% cache hit despite the per-persona prefix split, because the shared rules and policy dominate the prefix.
Not in this PR
Whether a second agent is worth the money. That is a question
tinysweeper evalanswers, and it cannot answer it yet — both corpus cases are forbidden-only, so recall and precision both rendern/a.council.enabled = falseships indefaults.tomland stays there until the corpus has cases that can measure the difference.corroborationis a separate switch precisely so the merge could be proved a no-op before a second agent is what is being judged.