feat(council): several reviewers on one lane, merged monotonically

docs/modules/council/README.md

@@ -0,0 +1,131 @@
+# `src/council`
+
+Several reviewers on one lane's evidence, folded into one review. Always
+compiled; nothing here calls a model.
+
+Off by default. `[council]` is on the **not overridable** list in
+`src/config/remote`, because every key in it either spends the operator's money
+— a second reviewer is a second call per file — or decides what a model is told.
+A pull request that could add reviewers to its own review would be no gate at
+all.
+
+## It is not a second opinion
+
+`src/falsify` is already that, and [its README](../falsify/README.md) explains
+at length why asking a second model *"are these correct?"* deletes the best half
+of a review: a checker that saw less than the reviewer rejects what it cannot
+confirm, and what it cannot confirm is exactly what needed the extra context.
+
+A council runs in the opposite direction. More reviewers so that **more is
+found**; agreement used only to rank what comes back.
+
+## Agreement raises confidence. It never gates posting.
+
+The obvious design is a quorum — post a finding when two of three raise it — and
+it is the falsification failure in a different hat. Worse here, because the
+architecture is *deliberately built* so reviewers do not overlap:
+`harness::prompt::ISOLATION_CLAUSE` exists precisely to stop N conversations
+reporting the same cross-file problem. A reviewer with a different angle, a
+different slice, or later a tool belt the others lack will find things the
+others structurally cannot see. Gating on agreement deletes those, which is to
+say the findings that justify running more than one reviewer at all.
+
+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 — three
+models agreeing is not a certainty any of them claimed), and breaks ties last in
+`lane_proposal`'s sort so a corroborated finding survives the `max_comments`
+truncation. A singleton passes through on its own merit, judged by the same
+`confidence_min` it would have faced alone.
+
+## Grouping is not the fingerprint
+
+`Finding::fingerprint` hashes the lane, path, **rule** and anchored code. That
+is right for its job — deciding whether this is a finding already posted — and
+wrong for this one, because `rule` is model-authored free text and two reviewers
+on one missing bounds check will write `unchecked-index` and
+`missing-bounds-check`. Grouping on the fingerprint would post both.
+
+`agree::corroborates` therefore uses a looser rule: same lane, same file,
+anchored ranges overlapping within three lines. It never compares titles or
+bodies — two agents describing one defect word it differently by construction,
+which is the entire reason for running more than one.
+
+That looseness is used **only** for grouping. `Finding::identity` stays exactly
+as `anchor::stamp` computed it, and a merge keeps the identity of the *first*
+sighting: it is what the `tinysweeper:fp=` marker carries and what suppression
+reads back, so letting the representative bring its own would repost a finding
+that had already been answered.
+
+## The representative is verbatim
+
+Where several reviewers describe one defect, one finding is chosen whole —
+highest confidence, then highest severity. Nothing is blended, rewritten or
+summarised. A merge step that can author text is a second reviewer nobody
+gated, which is the same objection `src/falsify` raises to a filter that can
+return findings of its own.
+
+Severity is the *highest* anyone assigned rather than the representative's.
+Merging must not talk a review down: a reviewer outvoted on wording keeps its
+opinion about how much the defect matters.
+
+## A merge never crosses reviewers' own findings
+
+Only findings contributed by *earlier* reviewers may absorb a later one, and
+each may absorb at most once per round.
+
+Both halves are load-bearing. Merging within one reviewer's output would make
+the council change behaviour at a single agent — two findings three lines apart
+would silently become one — which destroys the property the whole rollout rests
+on. It is also not the council's job: one reviewer repeating itself is a dedupe
+question `lane_proposal` already owns.
+
+This is not hypothetical. It shipped in the first draft and `tinysweeper eval`
+caught it on `ts-0068`, where the critique lane legitimately raises two findings
+on adjacent lines of one file.
+
+## Personas are names, never text
+
+`src/config/remote` excludes `path_instructions` from what a repository may set,
+with the reason stated there: it is free text injected straight into a lane's
+instructions, unfenced, and repository prose reaches a prompt through exactly
+one door — the sandboxed extraction in `crate::knowledge`. A persona is the same
+shape of text in the same position, so it is a `&'static str` in
+`council::persona` selected by name, and an unknown name is a configuration
+error reported by `tinysweeper check`.
+
+A persona must change *what the reviewer looks at* — the failure classes it
+reaches for first — not merely how it phrases the answer. Asking one model the
+same question twice at the same temperature produces the same answer twice, and
+paying for both is not a council. Each persona also states that other reviewers
+are running, for the same reason `ISOLATION_CLAUSE` exists.
+
+## One agent is a provable no-op
+
+With the council off, `reviewers()` yields the lane's own model and the empty
+persona, so the prompt is byte-identical to the pre-council one. With one agent
+configured and no persona, the same holds and `merge` returns its input
+untouched.
+
+That is asserted twice: directly, in `merge_test.rs`, and empirically — the
+committed `evals/` corpus replays against its existing cassettes, which are
+keyed on the full prompt text. A prompt that had moved by one byte would miss.
+
+## Cost
+
+One extra reviewer is one extra call per file for that lane, plus nothing else:
+falsification runs **once over the merged set** rather than per reviewer, since
+a reject-only filter given more inputs in one pass has identical semantics at a
+fraction of the calls.
+
+Each agent has its own cache stream, because the persona sits in the prefix. Two
+agents on one model means two prefixes to warm rather than one — real, and small
+against the call itself.
+
+## Files
+
+| File | Role |
+| --- | --- |
+| `mod.rs` | resolves configuration into the reviewers for a lane |
+| `agree.rs` | whether two reviewers found the same thing |
+| `merge.rs` | folding them together, monotonically |
+| `persona.rs` | the reviewing angles, as in-tree text selected by name |

src/app/apply.rs

@@ -536,6 +536,7 @@ mod tests {
             applicable: None,
             late: false,
             identity: None,
+            corroboration: 1,
         }
     }
 

src/app/review.rs

@@ -858,6 +858,13 @@ fn lane_proposal(
         b.severity
             .cmp(&a.severity)
             .then(b.confidence.total_cmp(&a.confidence))
+            // Corroboration breaks ties and nothing more. It sits *after*
+            // severity and confidence deliberately: agreement between
+            // reviewers is evidence about which of two equally-rated findings
+            // to keep when `max_comments` bites, not a reason to rank a minor
+            // finding above a serious one. Without a council every finding
+            // carries 1 and this term never fires.
+            .then(b.corroboration.cmp(&a.corroboration))
     });
 
     let over_cap = findings.len().saturating_sub(config.review.max_comments);

src/config/defaults.toml

@@ -119,6 +119,22 @@ max_tokens = 16000
 reasoning_effort = "high"
 budget_usd_per_pr = 1.0
 
+# Several reviewers on one lane's evidence, folded into one review.
+#
+# Off, and off is the honest default: a second reviewer doubles the calls for
+# that lane, and whether it is worth the money is a question `tinysweeper eval`
+# answers rather than one anybody should guess. `[council]` is also on the *not
+# overridable* list in `src/config/remote` — every key here spends the
+# operator's money, which is the same line drawn around `[models]`.
+#
+# `corroboration` is a separate switch on purpose. With one agent the merge is
+# a provable no-op, so it can be turned on and measured before a second agent
+# is what is being judged.
+[council]
+enabled = false
+corroboration = true
+agents = []
+
 # The knowledge centre. `files` are read from the pull request's head commit
 # through the forge and are untrusted input — they are put through a sandboxed
 # extraction pass rather than injected. Plain filenames only: a name containing

src/config/remote_test.rs

@@ -466,3 +466,35 @@ async fn a_fetched_config_is_filtered_exactly_as_a_parsed_one_is() {
     assert!(!overlaid.config.automerge.enabled);
     assert_eq!(overlaid.ignored, vec!["automerge.enabled".to_string()]);
 }
+
+#[test]
+fn council_keys_are_not_overridable_by_a_reviewed_repository() {
+    // Every key under `[council]` either spends the operator's money — a second
+    // reviewer is a second call per file — or decides what a model is told. That
+    // is the same line already drawn around `[models]`, and a pull request that
+    // could add reviewers to its own review would be no gate at all.
+    for key in [
+        "council.enabled",
+        "council.corroboration",
+        "council.agents",
+        "council.agents.persona",
+    ] {
+        assert!(!overridable(key), "`{key}` must not be repo-settable");
+    }
+}
+
+#[test]
+fn a_repository_cannot_convene_a_council_about_itself() {
+    // The base config has the council off. A document that turns it on must
+    // change nothing and say that it was ignored.
+    let (config, ignored) = applied(
+        "[council]\nenabled = true\ncorroboration = false\n\n[[council.agents]]\nid = \"mine\"\n",
+    );
+
+    assert!(!config.council.enabled, "the council stayed off");
+    assert!(config.council.agents.is_empty(), "no agent was added");
+    assert!(
+        ignored.iter().any(|key| key.starts_with("council")),
+        "the drop has to be reported, not silent: {ignored:?}"
+    );
+}

src/config/types.rs

@@ -227,6 +227,8 @@ pub struct Config {
     pub retrieval: Retrieval,
     /// Per-lane overrides, keyed by lane id.
     pub lanes: BTreeMap<String, Lane>,
+    /// Several reviewers on one lane's evidence.
+    pub council: Council,
     /// Auto-merge policy.
     pub automerge: AutoMerge,
     /// Review-thread resolution.
@@ -604,6 +606,45 @@ pub enum Workload {
     ThreadReview,
 }
 
+/// Several reviewers on one lane's evidence.
+///
+/// Off by default and **not overridable by a reviewed repository** — every key
+/// here spends the operator's money or decides what a model is told, which is
+/// the same line `config::remote` draws around `[models]`. See
+/// `docs/modules/council/README.md`.
+#[derive(Debug, Clone, Default, Serialize, Deserialize)]
+#[serde(default, deny_unknown_fields)]
+pub struct Council {
+    /// Whether more than one reviewer runs at all.
+    pub enabled: bool,
+    /// Merge corroborating findings and raise their confidence.
+    ///
+    /// Separate from `enabled` so the merge can be measured on its own before
+    /// a second agent is what is being judged.
+    pub corroboration: bool,
+    /// The reviewers, in the order they run.
+    pub agents: Vec<CouncilAgent>,
+}
+
+/// One reviewer in the council.
+#[derive(Debug, Clone, Default, Serialize, Deserialize)]
+#[serde(default, deny_unknown_fields)]
+pub struct CouncilAgent {
+    /// Stable id, used in the cost line and the check-run summary.
+    pub id: String,
+    /// Which lanes this agent reviews. Empty means every enabled lane.
+    pub lanes: Vec<LaneId>,
+    /// A tier name (`scan`, `deep`) or an explicit model id. Absent inherits
+    /// the lane's own model.
+    pub model: Option<ModelRef>,
+    /// A persona name from `council::persona::NAMES`. Absent is the lane's own
+    /// prompt, unchanged.
+    ///
+    /// A **name**, never the text: repository prose reaches a prompt through
+    /// exactly one door, and this is not it.
+    pub persona: Option<String>,
+}
+
 /// Per-lane overrides.
 #[derive(Debug, Clone, Default, Serialize, Deserialize)]
 #[serde(default, deny_unknown_fields)]
@@ -887,6 +928,26 @@ impl Config {
         }
     }
 
+    /// Resolve a council agent to a concrete model id.
+    ///
+    /// The same three-way rule as [`Config::model_for`] — a tier name, an
+    /// explicit id, or nothing — so there is one resolution rule in the
+    /// codebase rather than three shapes of it. An agent that names no model
+    /// inherits its lane's, which is what makes a one-agent council identical
+    /// to no council.
+    ///
+    /// Deliberately **not** routed through [`Config::model_for_workload`]: that
+    /// match is exhaustive over *mechanical* work and pins everything to the
+    /// cheap tier, and a council agent is a reviewer.
+    pub fn model_for_agent<'a>(&'a self, agent: &'a CouncilAgent, lane: LaneId) -> &'a str {
+        match agent.model.as_ref().map(|r| r.0.as_str()) {
+            Some("deep") => &self.models.deep,
+            Some("scan") => &self.models.scan,
+            Some(explicit) => explicit,
+            None => self.model_for(lane),
+        }
+    }
+
     /// Resolve a non-lane [`Workload`] to a concrete model id.
     ///
     /// Every mechanical workload runs on the cheap tier, and the `match` is

src/config/validate.rs

@@ -30,6 +30,7 @@ pub fn validate(config: &Config) -> Vec<String> {
     validate_retrieval(config, &mut problems);
     validate_overview(config, &mut problems);
     validate_lanes(config, &mut problems);
+    validate_council(config, &mut problems);
     validate_automerge(config, &mut problems);
     validate_issues(config, &mut problems);
     validate_automation(config, &mut problems);
@@ -645,6 +646,55 @@ fn validate_sentry(config: &Config, problems: &mut Vec<String>) {
     }
 }
 
+/// The council: who reviews, with what character.
+fn validate_council(config: &Config, problems: &mut Vec<String>) {
+    let council = &config.council;
+
+    if council.enabled && council.agents.is_empty() {
+        problems.push(
+            "`council.enabled = true` with no `[[council.agents]]` reviews nothing differently; \
+             either add an agent or leave the council off"
+                .into(),
+        );
+    }
+
+    let mut seen = std::collections::BTreeSet::new();
+    for agent in &council.agents {
+        if agent.id.trim().is_empty() {
+            problems.push("a `[[council.agents]]` entry has no `id`".into());
+        } else if !seen.insert(agent.id.as_str()) {
+            // The id names the agent in the cost line and the check summary, so
+            // two agents sharing one makes the report unreadable.
+            problems.push(format!(
+                "two `[[council.agents]]` entries share the id `{}`",
+                agent.id
+            ));
+        }
+
+        if let Some(persona) = agent.persona.as_deref()
+            && crate::council::persona::lookup(persona).is_none()
+        {
+            // A persona is a name, never text: repository prose reaches a
+            // prompt through exactly one door and this is not it. So a typo has
+            // to be an error rather than a reviewer with no character.
+            problems.push(format!(
+                "`{}` names the persona `{persona}`, which does not exist. Known: {}",
+                agent.id,
+                known(&crate::council::persona::NAMES)
+            ));
+        }
+
+        for lane in &agent.lanes {
+            if !config.review.lanes.iter().any(|name| name == lane.as_str()) {
+                problems.push(format!(
+                    "`{}` reviews the `{lane}` lane, which `review.lanes` does not enable",
+                    agent.id
+                ));
+            }
+        }
+    }
+}
+
 /// Render a list of accepted values for an error message.
 fn known(values: &[&str]) -> String {
     values