diff --git a/docs/modules/council/README.md b/docs/modules/council/README.md new file mode 100644 index 00000000..1488978b --- /dev/null +++ b/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 | diff --git a/src/app/apply.rs b/src/app/apply.rs index 730b32c8..1975b10a 100644 --- a/src/app/apply.rs +++ b/src/app/apply.rs @@ -536,6 +536,7 @@ mod tests { applicable: None, late: false, identity: None, + corroboration: 1, } } diff --git a/src/app/review.rs b/src/app/review.rs index 866d0559..10072050 100644 --- a/src/app/review.rs +++ b/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); diff --git a/src/config/defaults.toml b/src/config/defaults.toml index b1dab342..de275c95 100644 --- a/src/config/defaults.toml +++ b/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 diff --git a/src/config/remote_test.rs b/src/config/remote_test.rs index 115a0f8d..8c231ad3 100644 --- a/src/config/remote_test.rs +++ b/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:?}" + ); +} diff --git a/src/config/types.rs b/src/config/types.rs index 51d29ad2..34c4b7ad 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -227,6 +227,8 @@ pub struct Config { pub retrieval: Retrieval, /// Per-lane overrides, keyed by lane id. pub lanes: BTreeMap, + /// 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, +} + +/// 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, + /// A tier name (`scan`, `deep`) or an explicit model id. Absent inherits + /// the lane's own model. + pub model: Option, + /// 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, +} + /// 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 diff --git a/src/config/validate.rs b/src/config/validate.rs index 04a02e97..278dbfd6 100644 --- a/src/config/validate.rs +++ b/src/config/validate.rs @@ -30,6 +30,7 @@ pub fn validate(config: &Config) -> Vec { 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) { } } +/// The council: who reviews, with what character. +fn validate_council(config: &Config, problems: &mut Vec) { + 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 diff --git a/src/council/agree.rs b/src/council/agree.rs new file mode 100644 index 00000000..8998ceeb --- /dev/null +++ b/src/council/agree.rs @@ -0,0 +1,76 @@ +//! Deciding when two reviewers found the same thing. +//! +//! Always compiled. Pure, offline, and deliberately separate from +//! [`crate::findings::anchor`]: the two answer different questions and +//! collapsing them would break cross-push dedupe. +//! +//! # Why not just compare fingerprints +//! +//! `Finding::fingerprint` hashes the lane, the path, the **rule** and the +//! anchored code. That is exactly right for its job — deciding whether this is +//! the same finding as one already posted — because a rule id is required to be +//! stable for one class of problem across runs. +//! +//! It is wrong for this job. `rule` is model-authored free text, and two +//! reviewers looking at the same missing bounds check will write +//! `unchecked-index` and `missing-bounds-check`. Grouping on the fingerprint +//! would call those two separate findings and post both, which is the noise the +//! council is supposed to reduce rather than create. +//! +//! So corroboration uses a looser rule — same file, overlapping anchored lines +//! — and it is used **only** for grouping. `Finding::identity` is left exactly +//! as `anchor::stamp` computed it, because that is what the `tinysweeper:fp=` +//! marker carries onto GitHub and what suppression reads back. Loosening +//! identity to match this would resurrect comments that were already posted and +//! answered. + +use crate::findings::types::Finding; + +/// How far apart two anchors may sit and still be one observation. +/// +/// Matches `crate::eval::score`'s tolerance, and for the same reason: a +/// reviewer anchors to the guard, the call, or the line under it depending on +/// what it quoted, so a few lines of slack is the same defect described from a +/// different angle. +pub const LINE_TOLERANCE: u64 = 3; + +/// Whether `a` and `b` are the same observation by two reviewers. +/// +/// Never compares titles or bodies. Two agents describing one defect will word +/// it differently by construction — that is the entire reason for running more +/// than one — so wording is evidence of nothing here. +pub fn corroborates(a: &Finding, b: &Finding) -> bool { + if a.lane != b.lane || a.path != b.path { + return false; + } + + // An identical fingerprint is conclusive when both have one: same lane, + // path, rule and anchored code is the same finding by the strictest rule + // the crate has. + if let (Some(left), Some(right)) = (&a.identity, &b.identity) + && left == right + { + return true; + } + + match (a.range(), b.range()) { + (Some((a_start, a_end)), Some((b_start, b_end))) => { + let low = b_start.saturating_sub(LINE_TOLERANCE); + let high = b_end.saturating_add(LINE_TOLERANCE); + a_start <= high && a_end >= low + } + // Neither could be placed. Both were demoted to the check-run summary + // for the same file, and posting two unplaceable findings about one + // file is the worst version of this noise: a reader cannot even tell + // them apart by line. + (None, None) => true, + // One was placed and the other was not. They may well be the same + // defect, but there is no evidence of it, and merging on no evidence + // would silently delete a finding. + _ => false, + } +} + +#[cfg(test)] +#[path = "agree_test.rs"] +mod tests; diff --git a/src/council/agree_test.rs b/src/council/agree_test.rs new file mode 100644 index 00000000..3ac1ad04 --- /dev/null +++ b/src/council/agree_test.rs @@ -0,0 +1,129 @@ +//! When two reviewers are talking about the same thing. + +use super::*; +use crate::config::types::{LaneId, Severity}; + +fn finding(path: &str, line: Option, rule: &str) -> Finding { + Finding { + lane: LaneId::Critique, + severity: Severity::Medium, + confidence: 0.8, + path: path.into(), + line, + end_line: None, + rule: rule.into(), + title: "t".into(), + body: "b".into(), + suggestion: None, + late: false, + identity: None, + applicable: None, + corroboration: 1, + } +} + +#[test] +fn the_same_line_with_different_rule_ids_corroborates() { + // The case the fingerprint gets wrong. `rule` is model-authored free text, + // so two reviewers on one missing bounds check will name it differently — + // and grouping on the fingerprint would post both. + let a = finding("src/a.rs", Some(10), "unchecked-index"); + let b = finding("src/a.rs", Some(10), "missing-bounds-check"); + assert!(corroborates(&a, &b)); +} + +#[test] +fn wording_is_never_evidence_either_way() { + // Two agents describing one defect word it differently by construction — + // that is the entire reason for running more than one. + let mut a = finding("src/a.rs", Some(10), "x"); + a.title = "Guard the index".into(); + a.body = "panics".into(); + let mut b = finding("src/a.rs", Some(10), "x"); + b.title = "Bounds check missing before dereference".into(); + b.body = "this will abort the process".into(); + + assert!(corroborates(&a, &b)); +} + +#[test] +fn different_files_never_corroborate() { + let a = finding("src/a.rs", Some(10), "x"); + let b = finding("src/b.rs", Some(10), "x"); + assert!(!corroborates(&a, &b)); +} + +#[test] +fn different_lanes_never_corroborate() { + // `Finding::fingerprint` hashes the lane first, so critique and security + // can never collide — and two lanes are two subjects, not two opinions. + let a = finding("src/a.rs", Some(10), "x"); + let mut b = finding("src/a.rs", Some(10), "x"); + b.lane = LaneId::Security; + assert!(!corroborates(&a, &b)); +} + +#[test] +fn the_tolerance_is_three_lines_either_way() { + let anchor = finding("src/a.rs", Some(10), "x"); + for line in [7, 10, 13] { + assert!( + corroborates(&anchor, &finding("src/a.rs", Some(line), "y")), + "line {line} should corroborate" + ); + } + for line in [6, 14] { + assert!( + !corroborates(&anchor, &finding("src/a.rs", Some(line), "y")), + "line {line} should not" + ); + } +} + +#[test] +fn overlapping_ranges_corroborate_even_when_the_starts_differ() { + let mut a = finding("src/a.rs", Some(10), "x"); + a.end_line = Some(40); + let b = finding("src/a.rs", Some(35), "y"); + assert!(corroborates(&a, &b)); +} + +#[test] +fn two_unplaceable_findings_on_one_file_corroborate() { + // Both were demoted to the check-run summary. Posting two unplaceable + // findings about one file is the worst version of this noise: a reader + // cannot even tell them apart by line. + let a = finding("src/a.rs", None, "x"); + let b = finding("src/a.rs", None, "y"); + assert!(corroborates(&a, &b)); +} + +#[test] +fn a_placed_finding_does_not_absorb_an_unplaceable_one() { + // They may well be the same defect, but there is no evidence of it, and + // merging on no evidence silently deletes a finding. + let placed = finding("src/a.rs", Some(10), "x"); + let floating = finding("src/a.rs", None, "y"); + assert!(!corroborates(&placed, &floating)); + assert!(!corroborates(&floating, &placed)); +} + +#[test] +fn an_identical_fingerprint_is_conclusive() { + // Same lane, path, rule and anchored code is the same finding by the + // strictest rule the crate has — so it corroborates even where the line + // numbers have drifted apart. + let mut a = finding("src/a.rs", Some(10), "x"); + a.identity = Some("deadbeef".into()); + let mut b = finding("src/a.rs", Some(400), "x"); + b.identity = Some("deadbeef".into()); + + assert!(corroborates(&a, &b)); +} + +#[test] +fn corroboration_is_symmetric() { + let a = finding("src/a.rs", Some(10), "x"); + let b = finding("src/a.rs", Some(12), "y"); + assert_eq!(corroborates(&a, &b), corroborates(&b, &a)); +} diff --git a/src/council/merge.rs b/src/council/merge.rs new file mode 100644 index 00000000..bf391bfc --- /dev/null +++ b/src/council/merge.rs @@ -0,0 +1,123 @@ +//! Folding several reviewers' findings into one set. +//! +//! Always compiled, pure, and offline. +//! +//! # Agreement raises confidence. It never gates posting. +//! +//! The obvious design is a quorum: post a finding when two of three reviewers +//! raised it. It is the falsification failure wearing a different hat, and +//! `docs/modules/falsify/README.md` already explains why — a checker that saw +//! less than the reviewer rejects what it cannot confirm, and what it cannot +//! confirm is the good half. +//! +//! Here it is worse than that, because the architecture is *deliberately built* +//! so reviewers do not overlap. `harness::prompt::ISOLATION_CLAUSE` exists +//! precisely to stop N conversations noticing the same cross-file problem. A +//! reviewer given a different persona, a different slice, or — later — a tool +//! belt the others do not have, will find things the others structurally cannot +//! see. Gating on agreement deletes exactly those, which is to say exactly 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 and breaks ties in the comment +//! cap, and a singleton passes through on its own merit, judged by the same +//! `confidence_min` it would have faced alone. +//! +//! # The representative is verbatim +//! +//! When several reviewers describe one defect, one of their findings 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. + +use crate::council::agree::corroborates; +use crate::findings::types::Finding; + +/// The ceiling on merged confidence. +/// +/// Noisy-OR climbs towards 1.0 fast, and a finding reported as *certain* +/// because three models happened to agree is a claim none of them made. The cap +/// keeps agreement a strong signal rather than an infallible one. +const CONFIDENCE_CEILING: f64 = 0.99; + +/// Fold per-reviewer findings into one set. +/// +/// `reviews` is one vector per reviewer, in configuration order. The result is +/// ordered by first appearance, so a single-reviewer council returns its input +/// untouched — which is what makes turning the council on with one agent a +/// provable no-op. +pub fn merge(reviews: Vec>) -> Vec { + let mut merged: Vec = Vec::new(); + + for review in reviews { + // Only what earlier reviewers contributed may absorb this reviewer's + // findings, and each of those may absorb at most once per round. + // + // Both halves matter. Merging *within* one reviewer's output would make + // the council change behaviour at a single agent — two findings three + // lines apart from one reviewer would silently become one — and that + // breaks the property this whole slice rests on. It is also not the + // council's job: one reviewer reporting the same defect twice is a + // dedupe question, and `lane_proposal` already owns it. + // + // This is not hypothetical. It shipped, and `eval` caught it on + // `ts-0068` — where the critique lane legitimately raises two findings + // on adjacent lines of one file. + let settled = merged.len(); + let mut claimed = vec![false; settled]; + let mut additions = Vec::new(); + + for finding in review { + let hit = merged[..settled] + .iter() + .enumerate() + .position(|(index, kept)| !claimed[index] && corroborates(kept, &finding)); + match hit { + Some(index) => { + claimed[index] = true; + absorb(&mut merged[index], finding); + } + None => additions.push(finding), + } + } + + merged.extend(additions); + } + + merged +} + +/// Fold `other` into `kept`, keeping whichever is the better statement of it. +fn absorb(kept: &mut Finding, other: Finding) { + // Noisy-OR: independent observers each with their own chance of being + // wrong. Monotone by construction — the result is never below either input + // — which is the property that makes the merge safe to leave on. + let combined = 1.0 - (1.0 - kept.confidence) * (1.0 - other.confidence); + let corroboration = kept.corroboration.saturating_add(other.corroboration); + // The highest anyone assigned, read before either side is moved. Merging + // must not talk a review *down*: if one reviewer thought this was critical, + // that opinion survives being outvoted on wording. + let severity = kept.severity.max(other.severity); + + // The stronger statement wins the body, and it is taken whole. Confidence + // first, then severity: a reviewer that is sure of a medium-severity defect + // has described it better than one guessing at a high-severity one. + if (other.confidence, other.severity) > (kept.confidence, kept.severity) { + // `identity` is what the posted marker carries and what suppression + // reads back, so it belongs to the finding as first seen. Letting the + // representative bring its own would repost a finding that had already + // been answered. + let identity = kept.identity.clone(); + *kept = other; + kept.identity = identity; + } + + kept.confidence = combined.min(CONFIDENCE_CEILING); + kept.corroboration = corroboration; + kept.severity = severity; +} + +#[cfg(test)] +#[path = "merge_test.rs"] +mod tests; diff --git a/src/council/merge_test.rs b/src/council/merge_test.rs new file mode 100644 index 00000000..191a4aa8 --- /dev/null +++ b/src/council/merge_test.rs @@ -0,0 +1,213 @@ +//! Folding reviewers together, and the properties that make it safe to leave on. + +use super::*; +use crate::config::types::{LaneId, Severity}; + +fn finding(path: &str, line: u64, rule: &str, confidence: f64) -> Finding { + Finding { + lane: LaneId::Critique, + severity: Severity::Medium, + confidence, + path: path.into(), + line: Some(line), + end_line: None, + rule: rule.into(), + title: format!("Finding at {path}:{line}"), + body: "why it matters".into(), + suggestion: None, + late: false, + identity: None, + applicable: None, + corroboration: 1, + } +} + +#[test] +fn one_reviewer_is_returned_untouched() { + // The property that makes turning the council on with one agent a provable + // no-op, and therefore the one worth asserting first. + let only = vec![ + finding("src/a.rs", 10, "unchecked-index", 0.7), + finding("src/b.rs", 3, "leak", 0.9), + ]; + let merged = merge(vec![only.clone()]); + + assert_eq!(merged.len(), 2); + assert_eq!(merged[0].confidence, only[0].confidence); + assert_eq!(merged[1].confidence, only[1].confidence); + assert_eq!(merged[0].corroboration, 1); + assert_eq!(merged[0].title, only[0].title); +} + +#[test] +fn two_reviewers_on_the_same_line_become_one_finding() { + let merged = merge(vec![ + vec![finding("src/a.rs", 10, "unchecked-index", 0.6)], + // A different rule id for the same defect — which is the normal case, + // because `rule` is model-authored free text. + vec![finding("src/a.rs", 11, "missing-bounds-check", 0.6)], + ]); + + assert_eq!(merged.len(), 1, "{merged:?}"); + assert_eq!(merged[0].corroboration, 2); + // Noisy-OR: 1 - 0.4*0.4. + assert!( + (merged[0].confidence - 0.84).abs() < 1e-9, + "{}", + merged[0].confidence + ); +} + +#[test] +fn merging_never_lowers_a_confidence() { + // The invariant that makes the merge safe to leave on: no finding is ever + // worse off for the council having run, so enabling it cannot silently + // drop something below `confidence_min`. + for a in [0.0, 0.1, 0.5, 0.75, 0.99, 1.0] { + for b in [0.0, 0.1, 0.5, 0.75, 0.99, 1.0] { + let merged = merge(vec![ + vec![finding("src/a.rs", 10, "x", a)], + vec![finding("src/a.rs", 10, "y", b)], + ]); + let combined = merged[0].confidence; + assert!( + combined >= a.min(0.99) - 1e-9 && combined >= b.min(0.99) - 1e-9, + "{a} + {b} = {combined}" + ); + assert!(combined <= 0.99 + 1e-9, "{a} + {b} = {combined}"); + } + } +} + +#[test] +fn the_representative_is_one_reviewers_words_and_not_a_blend() { + // A merge step that can author text is a second reviewer nobody gated. + let mut weak = finding("src/a.rs", 10, "x", 0.4); + weak.title = "Possibly unchecked".into(); + weak.body = "not sure".into(); + let mut strong = finding("src/a.rs", 10, "y", 0.9); + strong.title = "Guard the index".into(); + strong.body = "`items[i]` panics on an empty slice.".into(); + + let merged = merge(vec![vec![weak], vec![strong]]); + + assert_eq!(merged[0].title, "Guard the index"); + assert_eq!(merged[0].body, "`items[i]` panics on an empty slice."); +} + +#[test] +fn merging_keeps_the_highest_severity_anyone_assigned() { + // Merging must not talk a review down. A reviewer outvoted on wording keeps + // its opinion about how much the defect matters. + let mut minor = finding("src/a.rs", 10, "x", 0.9); + minor.severity = Severity::Low; + let mut major = finding("src/a.rs", 10, "y", 0.5); + major.severity = Severity::Critical; + + let merged = merge(vec![vec![minor], vec![major]]); + assert_eq!(merged[0].severity, Severity::Critical); + // ...while the clearer statement still supplies the words. + assert!(merged[0].title.contains("src/a.rs:10")); +} + +#[test] +fn the_identity_of_the_first_sighting_survives_a_replacement() { + // `identity` is what the `tinysweeper:fp=` marker carries and what + // suppression reads back. Letting the representative bring its own would + // repost a finding that had already been answered. + let mut first = finding("src/a.rs", 10, "x", 0.4); + first.identity = Some("firstfp0".into()); + let mut second = finding("src/a.rs", 10, "y", 0.9); + second.identity = Some("secondfp".into()); + + let merged = merge(vec![vec![first], vec![second]]); + assert_eq!(merged[0].identity.as_deref(), Some("firstfp0")); +} + +#[test] +fn a_singleton_passes_through_on_its_own_merit() { + // The finding only one reviewer could see is the entire reason for running + // more than one. Gating on agreement would delete exactly these. + let merged = merge(vec![ + vec![finding("src/a.rs", 10, "x", 0.8)], + vec![finding("src/b.rs", 99, "y", 0.65)], + ]); + + assert_eq!(merged.len(), 2); + let lonely = merged.iter().find(|f| f.path == "src/b.rs").expect("kept"); + assert_eq!(lonely.confidence, 0.65, "unchanged"); + assert_eq!(lonely.corroboration, 1); +} + +#[test] +fn the_same_rule_on_two_call_sites_stays_two_findings() { + let merged = merge(vec![ + vec![ + finding("src/a.rs", 10, "unchecked-index", 0.8), + finding("src/a.rs", 90, "unchecked-index", 0.8), + ], + vec![finding("src/a.rs", 91, "unchecked-index", 0.8)], + ]); + + assert_eq!(merged.len(), 2, "{merged:?}"); + assert_eq!(merged[0].corroboration, 1); + assert_eq!(merged[1].corroboration, 2); +} + +#[test] +fn three_reviewers_accumulate_rather_than_pairing_off() { + let merged = merge(vec![ + vec![finding("src/a.rs", 10, "x", 0.5)], + vec![finding("src/a.rs", 10, "y", 0.5)], + vec![finding("src/a.rs", 10, "z", 0.5)], + ]); + + assert_eq!(merged.len(), 1); + assert_eq!(merged[0].corroboration, 3); + // 1 - 0.5^3. + assert!((merged[0].confidence - 0.875).abs() < 1e-9); +} + +#[test] +fn an_empty_council_merges_to_nothing_rather_than_panicking() { + assert!(merge(vec![]).is_empty()); + assert!(merge(vec![vec![], vec![]]).is_empty()); +} + +#[test] +fn one_reviewers_own_findings_are_never_merged_into_each_other() { + // The council must not change behaviour at a single agent. Two findings + // three lines apart from one reviewer stay two — merging them would make + // enabling the merge a silent behaviour change, and it is not the council's + // job anyway: one reviewer repeating itself is a dedupe question that + // `lane_proposal` already owns. + // + // Regression: this shipped, and `eval` caught it on `ts-0068`. + let merged = merge(vec![vec![ + finding("src/a.rs", 10, "x", 0.7), + finding("src/a.rs", 11, "y", 0.7), + ]]); + + assert_eq!(merged.len(), 2, "{merged:?}"); + assert_eq!(merged[0].corroboration, 1); + assert_eq!(merged[1].corroboration, 1); + assert_eq!(merged[0].confidence, 0.7); +} + +#[test] +fn a_second_reviewer_absorbs_each_finding_at_most_once() { + // Two findings from the second reviewer both land near one from the first. + // Letting both absorb would report three reviewers agreeing when only two + // ran, and would delete a finding the second reviewer meant separately. + let merged = merge(vec![ + vec![finding("src/a.rs", 10, "x", 0.5)], + vec![ + finding("src/a.rs", 10, "y", 0.5), + finding("src/a.rs", 11, "z", 0.5), + ], + ]); + + assert_eq!(merged.len(), 2, "{merged:?}"); + assert_eq!(merged[0].corroboration, 2); + assert_eq!(merged[1].corroboration, 1); +} diff --git a/src/council/mod.rs b/src/council/mod.rs new file mode 100644 index 00000000..8d0500c1 --- /dev/null +++ b/src/council/mod.rs @@ -0,0 +1,96 @@ +//! Several reviewers on one piece of evidence, folded into one review. +//! +//! Always compiled. Nothing here calls a model: the council decides *who* runs +//! and what becomes of their findings, and the lanes still do the calling. +//! +//! # What a council is for +//! +//! Not a second opinion. `src/falsify` is already that, and its README explains +//! at length why asking a second model "are these correct?" deletes the best +//! half of a review. A council is the opposite direction: more reviewers so +//! that *more is found*, with agreement used only to rank what comes back. +//! +//! The diversity that pays is diversity of **subject**, not of vendor. The +//! repository already learned this the expensive way — `lanes::fanout` splits a +//! lane into one conversation per file and `ISOLATION_CLAUSE` exists because, +//! without it, every one of N reviewers on overlapping evidence reports the +//! same cross-file problem. Two reviewers reading the same file for different +//! failure classes are additive; two reading it for the same thing are a +//! duplicate with a bill attached. +//! +//! # Off by default, and a no-op at one agent +//! +//! `council.enabled = false` ships in `defaults.toml`, and `[council]` is not +//! overridable by a reviewed repository — every key in it spends the operator's +//! money, which is the same line `src/config/remote` draws around `[models]`. +//! +//! With one agent configured, [`merge::merge`] returns its input untouched and +//! a persona-less agent builds the lane's own prompt byte for byte. That is +//! deliberate: it makes the wiring provable before the second agent is what is +//! being judged. + +pub mod agree; +pub mod merge; +pub mod persona; + +pub use crate::council::agree::corroborates; +pub use crate::council::merge::merge; + +use crate::config::types::{Config, LaneId}; + +/// One reviewer, resolved from configuration. +#[derive(Debug, Clone, Copy)] +pub struct Reviewer<'a> { + /// The agent's id, for the check-run summary and the cost line. + pub id: &'a str, + /// The model id it calls, already resolved from tier to id. + pub model: &'a str, + /// The persona text appended to the lane instructions. Empty is the lane's + /// own prompt, unchanged. + pub persona: &'static str, +} + +/// Who reviews `lane`, in configuration order. +/// +/// Always at least one. A disabled council, or one whose agents all sit out +/// this lane, yields the single default reviewer — the lane's own model and no +/// persona — so every caller has one code path rather than a council branch and +/// a legacy branch that drift apart. +pub fn reviewers<'a>(config: &'a Config, lane: LaneId) -> Vec> { + let solo = || { + vec![Reviewer { + id: "reviewer", + model: config.model_for(lane), + persona: persona::NONE, + }] + }; + + if !config.council.enabled { + return solo(); + } + + let agents: Vec> = config + .council + .agents + .iter() + .filter(|agent| agent.lanes.is_empty() || agent.lanes.contains(&lane)) + .map(|agent| Reviewer { + id: &agent.id, + model: config.model_for_agent(agent, lane), + // Validated at load, so an unknown name cannot reach here. Falling + // back to no persona rather than panicking keeps a configuration + // mistake a weaker review instead of an outage. + persona: agent + .persona + .as_deref() + .and_then(persona::lookup) + .unwrap_or(persona::NONE), + }) + .collect(); + + if agents.is_empty() { solo() } else { agents } +} + +#[cfg(test)] +#[path = "mod_test.rs"] +mod tests; diff --git a/src/council/mod_test.rs b/src/council/mod_test.rs new file mode 100644 index 00000000..7be1055c --- /dev/null +++ b/src/council/mod_test.rs @@ -0,0 +1,120 @@ +//! Who reviews, and the guarantee that one reviewer is the old behaviour. + +use super::*; +use crate::config::types::{Config, CouncilAgent, ModelRef}; + +fn config() -> Config { + crate::config::DEFAULTS + .parse::() + .unwrap() + .try_into() + .unwrap() +} + +fn agent(id: &str, persona: Option<&str>, lanes: Vec) -> CouncilAgent { + CouncilAgent { + id: id.into(), + lanes, + model: None, + persona: persona.map(str::to_string), + } +} + +#[test] +fn a_disabled_council_yields_the_lanes_own_reviewer() { + let config = config(); + let reviewers = reviewers(&config, LaneId::Critique); + + assert_eq!(reviewers.len(), 1); + // The lane's own model and no persona: the prompt is byte-identical to the + // one built before the council existed. + assert_eq!(reviewers[0].model, config.model_for(LaneId::Critique)); + assert_eq!(reviewers[0].persona, persona::NONE); + assert!(persona::NONE.is_empty()); +} + +#[test] +fn an_enabled_council_with_no_agent_for_this_lane_still_reviews_it() { + // Every caller has one code path. A council branch and a legacy branch + // would drift apart, and the lane that fell through the gap would go + // unreviewed rather than loudly failing. + let mut config = config(); + config.council.enabled = true; + config.council.agents = vec![agent("sec", None, vec![LaneId::Security])]; + + let reviewers = reviewers(&config, LaneId::Critique); + assert_eq!(reviewers.len(), 1); + assert_eq!(reviewers[0].id, "reviewer"); +} + +#[test] +fn agents_run_in_configuration_order() { + let mut config = config(); + config.council.enabled = true; + config.council.agents = vec![ + agent("first", Some("correctness"), vec![]), + agent("second", Some("integration"), vec![]), + ]; + + let reviewers = reviewers(&config, LaneId::Critique); + let ids: Vec<&str> = reviewers.iter().map(|r| r.id).collect(); + // Order decides which reviewer's prose becomes the summary, so it has to be + // the operator's rather than a hash map's. + assert_eq!(ids, ["first", "second"]); + assert_ne!(reviewers[0].persona, reviewers[1].persona); +} + +#[test] +fn an_agent_with_no_model_inherits_the_lanes() { + // This is what makes a one-agent council identical to no council. + let mut config = config(); + config.council.enabled = true; + config.council.agents = vec![agent("solo", None, vec![])]; + + let reviewers = reviewers(&config, LaneId::Critique); + assert_eq!(reviewers[0].model, config.model_for(LaneId::Critique)); +} + +#[test] +fn an_agent_may_name_a_tier_or_an_explicit_model() { + let mut config = config(); + config.council.enabled = true; + config.council.agents = vec![ + CouncilAgent { + model: Some(ModelRef("deep".into())), + ..agent("tiered", None, vec![]) + }, + CouncilAgent { + model: Some(ModelRef("qwen/qwen3.7-plus".into())), + ..agent("explicit", None, vec![]) + }, + ]; + + let reviewers = reviewers(&config, LaneId::Critique); + // The same three-way rule `Config::model_for` uses, so there is one + // resolution rule in the codebase rather than three shapes of it. + assert_eq!(reviewers[0].model, config.models.deep); + assert_eq!(reviewers[1].model, "qwen/qwen3.7-plus"); +} + +#[test] +fn a_lane_scoped_agent_sits_out_the_lanes_it_did_not_name() { + let mut config = config(); + config.council.enabled = true; + config.council.agents = vec![ + agent("everywhere", Some("correctness"), vec![]), + agent("security-only", Some("adversary"), vec![LaneId::Security]), + ]; + + let critique: Vec<&str> = reviewers(&config, LaneId::Critique) + .iter() + .map(|r| r.id) + .collect(); + assert_eq!(critique, ["everywhere"]); + + let security: Vec<&str> = reviewers(&config, LaneId::Security) + .iter() + .map(|r| r.id) + .collect(); + assert_eq!(security, ["everywhere", "security-only"]); +} diff --git a/src/council/persona.rs b/src/council/persona.rs new file mode 100644 index 00000000..85c57a46 --- /dev/null +++ b/src/council/persona.rs @@ -0,0 +1,149 @@ +//! What makes one reviewer look at a diff differently from another. +//! +//! Always compiled. Every persona is a `&'static str` in this file, selected by +//! name, and that is a security boundary rather than a style choice. +//! +//! # Personas are never free text +//! +//! `src/config/remote.rs` excludes `path_instructions` from what a reviewed +//! repository may override, 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 a configurable one would be a second door, and `[council]` is +//! excluded from the overridable set for exactly this reason. +//! +//! Naming an unknown persona is a configuration error reported by +//! `tinysweeper check`, not a silently weaker reviewer. +//! +//! # Why a persona is not a second opinion +//! +//! Asking one model the same question twice at the same temperature produces +//! the same answer twice, and paying for both is not a council. A persona has +//! to change *what the reviewer looks at* — the failure classes it reaches for +//! first — rather than merely how it phrases the answer. That is why each one +//! below names concrete things to go and check, and why none of them says +//! "be thorough" or "think step by step". + +/// The persona applied when a council agent names none. +/// +/// Empty rather than a default flavour: an agent with no persona is the lane's +/// own instructions, unmodified, which is what makes a one-agent council a +/// provable no-op. +pub const NONE: &str = ""; + +/// Resolve a persona name to the text appended to the lane instructions. +/// +/// `None` for an unknown name, so `config::validate` can report it once at load +/// rather than every review silently running a reviewer with no character. +pub fn lookup(name: &str) -> Option<&'static str> { + Some(match name { + "correctness" => CORRECTNESS, + "integration" => INTEGRATION, + "adversary" => ADVERSARY, + _ => return None, + }) +} + +/// Every persona name, for error messages and `doctor`. +pub const NAMES: [&str; 3] = ["correctness", "integration", "adversary"]; + +/// Local correctness: what this code does on its own, read closely. +const CORRECTNESS: &str = r#" + +## Your angle + +Read this change as though you were stepping through it. Your subject is what +the code in front of you does on inputs the author did not picture: the empty +collection, the zero, the value that arrives twice, the error branch nobody +took. Boundaries, ordering, arithmetic, and the path taken when something +returns nothing. + +You are one of several reviewers with different angles. Report what *this* +angle sees and leave the rest; another reviewer is reading for the things you +are being told to skip."#; + +/// Integration: what this change does to code that already exists. +const INTEGRATION: &str = r#" + +## Your angle + +Read this change for its effect on everything it touches but does not show. +Your subject is the contract: a function whose meaning moved while its +signature did not, a caller that now receives something it never handled, an +invariant asserted somewhere else that this quietly breaks, a value that used +to be impossible and now is not. + +Where the retrieved context shows you a caller or a definition, use it — that is +what it is for. Where it does not, say what you would need to see rather than +guessing, and lower your confidence accordingly. + +You are one of several reviewers with different angles. Another is reading the +same code line by line for local correctness, so leave that to them."#; + +/// Adversary: what a hostile input does with this change. +const ADVERSARY: &str = r#" + +## Your angle + +Read this change as somebody trying to make it misbehave. Your subject is the +input the author did not consider hostile: the field they assumed was short, +the path they assumed was inside the directory, the identifier they assumed was +theirs, the loop they assumed terminates. + +A finding needs a source, a sink and a path between them that you can point at. +"This looks dangerous" is not one. If you cannot name all three from what you +were shown, you do not have a finding. + +You are one of several reviewers with different angles. Style, naming and test +coverage belong to somebody else."#; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn every_named_persona_resolves() { + for name in NAMES { + assert!( + lookup(name).is_some(), + "`{name}` is listed but not resolved" + ); + } + } + + #[test] + fn an_unknown_persona_is_none_rather_than_a_default() { + // A typo must be a configuration error, not a reviewer that silently + // has no character. + assert!(lookup("corectness").is_none()); + assert!(lookup("").is_none()); + } + + #[test] + fn every_persona_tells_the_reviewer_it_is_not_alone() { + // Without this clause each agent reports the same cross-cutting + // observation and the author gets it N times — the failure + // `harness::prompt::ISOLATION_CLAUSE` was written against. + for name in NAMES { + let text = lookup(name).expect("resolves"); + assert!( + text.contains("one of several reviewers"), + "`{name}` does not tell the reviewer others are running" + ); + } + } + + #[test] + fn no_persona_overrides_the_lanes_own_subject() { + // A persona narrows *within* a lane. One that told the reviewer to + // ignore the lane instructions would make the check run mean something + // different from its name. + for name in NAMES { + let text = lookup(name).expect("resolves").to_lowercase(); + for forbidden in ["ignore the", "instead of the instructions", "disregard"] { + assert!(!text.contains(forbidden), "`{name}` contains `{forbidden}`"); + } + } + } +} diff --git a/src/eval/score_test.rs b/src/eval/score_test.rs index 9c7a54ab..6a1c2077 100644 --- a/src/eval/score_test.rs +++ b/src/eval/score_test.rs @@ -66,6 +66,7 @@ fn finding(path: &str, line: u64, title: &str, body: &str) -> Finding { applicable: None, late: false, identity: Some("abcd1234".into()), + corroboration: 1, } } diff --git a/src/falsify/test.rs b/src/falsify/test.rs index 0b102ef6..2dc0c695 100644 --- a/src/falsify/test.rs +++ b/src/falsify/test.rs @@ -34,6 +34,7 @@ fn finding(title: &str) -> Finding { applicable: None, late: false, identity: None, + corroboration: 1, } } @@ -206,6 +207,7 @@ async fn a_finding_about_code_not_in_the_diff_survives() { applicable: None, late: false, identity: None, + corroboration: 1, }; let outcome = filter(&model, vec![finding_about_absent_code.clone()]).await; diff --git a/src/findings/anchor.rs b/src/findings/anchor.rs index 7ed1e001..d99c83fb 100644 --- a/src/findings/anchor.rs +++ b/src/findings/anchor.rs @@ -88,6 +88,7 @@ mod tests { applicable: None, late: false, identity: None, + corroboration: 1, } } diff --git a/src/findings/render.rs b/src/findings/render.rs index c421dcda..86c02bdd 100644 --- a/src/findings/render.rs +++ b/src/findings/render.rs @@ -398,6 +398,7 @@ mod tests { applicable: None, late: false, identity: None, + corroboration: 1, } } diff --git a/src/findings/suggest_test.rs b/src/findings/suggest_test.rs index 0d40d727..dc74104d 100644 --- a/src/findings/suggest_test.rs +++ b/src/findings/suggest_test.rs @@ -31,6 +31,7 @@ fn finding(line: Option, end_line: Option, suggestion: Option<&str>) - applicable: None, late: false, identity: None, + corroboration: 1, } } diff --git a/src/findings/types.rs b/src/findings/types.rs index 93c494bf..3f5f8020 100644 --- a/src/findings/types.rs +++ b/src/findings/types.rs @@ -86,6 +86,23 @@ pub struct Finding { /// publishes. #[serde(default)] pub identity: Option, + /// How many reviewers independently raised this. + /// + /// One for everything a single reviewer produced, which is why the default + /// is one rather than zero — a proposal written before the council existed + /// deserializes as "one reviewer said so", not "nobody did". + /// + /// It only ever breaks ties. Agreement raises confidence and survives the + /// `max_comments` truncation first; it is never a gate, because the + /// reviewer best placed to find something is often the only one who can + /// see it. See `src/council/merge.rs`. + #[serde(default = "one")] + pub corroboration: u8, +} + +/// The corroboration a finding nobody merged carries. +fn one() -> u8 { + 1 } impl Finding { @@ -173,6 +190,7 @@ impl From for Finding { applicable: None, late: false, identity: None, + corroboration: 1, } } } @@ -197,6 +215,7 @@ mod tests { applicable: None, late: false, identity: None, + corroboration: 1, } } diff --git a/src/harness/cassette.rs b/src/harness/cassette.rs index b038c6ed..f55d1903 100644 --- a/src/harness/cassette.rs +++ b/src/harness/cassette.rs @@ -287,6 +287,15 @@ impl Model for Cassette { if self.mode == Mode::Strict { state.misses += 1; + // Named at warn level as well as returned, because the error is + // aggregated by the time a human reads it and "which call" is the + // first thing they need in order to re-record the right thing. + tracing::warn!( + schema = %request.schema_name, + model = %request.model, + %key, + "cassette miss" + ); return Err(Error::Model(format!( "cassette miss in {}: no recorded answer for a `{}` call to `{}` (key {key}). \ The prompt changed since this was recorded — re-record the corpus, or replay \ diff --git a/src/harness/prompt.rs b/src/harness/prompt.rs index 96692fbf..5c548240 100644 --- a/src/harness/prompt.rs +++ b/src/harness/prompt.rs @@ -141,6 +141,14 @@ pub struct PromptInputs<'a> { /// The single file this prompt is scoped to, when the lane fans out one /// conversation per changed file. pub focus_path: Option<&'a str>, + /// The reviewing angle this conversation is given, when a council is + /// running several reviewers over the same evidence. + /// + /// A `&'static str` chosen by name in `council::persona` — never text from + /// configuration, and never anything a reviewed repository can set. It sits + /// in the prefix because it is constant for the whole conversation, which + /// means each agent gets its own cache stream rather than sharing one. + pub persona: &'static str, /// Findings the deterministic scanners already produced, rendered for /// adjudication. Untrusted only in the sense that it quotes paths, but /// fenced like everything else. @@ -178,6 +186,8 @@ impl<'a> PromptInputs<'a> { evidence_label: "diff", changed_paths: &[], focus_path: None, + // No council: the lane's own instructions, unmodified. + persona: crate::council::persona::NONE, scanner_evidence: "", pull_request_text: "", retrieved_context: "", @@ -191,6 +201,11 @@ pub fn build(inputs: &PromptInputs<'_>) -> Prompt { // Layer 1 — lane instructions. Never varies. prefix.push_str(instructions(inputs.lane)); + // Layer 1a — the council persona, before the shared rules so it reads as + // part of the job rather than as an afterthought to it. Empty for every + // caller that is not running a council, which is what keeps the prompt + // byte-identical to the pre-council one. + prefix.push_str(inputs.persona); prefix.push_str(SHARED_RULES); // Layer 1b — the per-file isolation clause, for lanes that fan out one diff --git a/src/harness/schema.rs b/src/harness/schema.rs index 48003b0e..587a711a 100644 --- a/src/harness/schema.rs +++ b/src/harness/schema.rs @@ -93,6 +93,8 @@ impl RawFinding { title: truncate(&scrub(&self.title), 80), body: scrub(&self.body), suggestion: self.suggestion.as_deref().map(scrub), + // One reviewer said so. The council raises it when another agrees. + corroboration: 1, // Stamped later, by `findings::suggest`, which has the diff. applicable: None, late: self.late, diff --git a/src/lanes/anchor.rs b/src/lanes/anchor.rs index 231f5cb4..1663058d 100644 --- a/src/lanes/anchor.rs +++ b/src/lanes/anchor.rs @@ -75,6 +75,7 @@ mod tests { applicable: None, late: false, identity: None, + corroboration: 1, } } diff --git a/src/lanes/critique.rs b/src/lanes/critique.rs index 0c7501a4..6a6b6cdc 100644 --- a/src/lanes/critique.rs +++ b/src/lanes/critique.rs @@ -36,10 +36,12 @@ use std::sync::Arc; use async_trait::async_trait; use crate::config::types::{Config, LaneId}; +use crate::council; use crate::error::Result; use crate::evidence::diff::FileDiff; use crate::evidence::replay; use crate::falsify::{Falsifier, Rejection}; +use crate::findings::types::Finding; use crate::harness::prompt::{self, PromptInputs}; use crate::harness::schema::{self, RawFinding}; use crate::lanes::fanout::{FileReview, per_file_with_budget}; @@ -128,12 +130,106 @@ async fn review_file( ) -> Result { let config: &Config = input.config; let evidence = replay::render(std::slice::from_ref(diff)); + let reviewers = council::reviewers(config, LaneId::Critique); + + let mut spend = Spend::default(); + let mut per_reviewer: Vec> = Vec::with_capacity(reviewers.len()); + let mut summary = String::new(); + let mut resolved: Vec = Vec::new(); + let mut unanchored = 0usize; + let mut discarded = 0usize; + + for reviewer in &reviewers { + // One reviewer's failure is not the lane's. With a council configured, + // losing one angle should cost that angle and nothing else — the same + // rule `lanes::fanout` applies to a file that could not be reviewed. + let asked = ask(model, input, changed_paths, diff, &evidence, reviewer).await; + let asked = match asked { + Ok(asked) => asked, + Err(err) if reviewers.len() > 1 => { + tracing::warn!(agent = reviewer.id, %err, "a council reviewer failed"); + continue; + } + Err(err) => return Err(err), + }; + + spend.merge(asked.spend); + unanchored += asked.unanchored; + discarded += asked.discarded; + // The first reviewer's prose, taken whole. Blending N summaries would + // author text no reviewer wrote, which is the objection `src/falsify` + // raises to a filter that can return findings of its own. + if summary.is_empty() { + summary = asked.summary; + resolved = asked.resolved; + } + per_reviewer.push(asked.findings); + } + + if per_reviewer.is_empty() { + return Err(crate::error::Error::lane( + "critique", + format!("every reviewer failed on {}", diff.path), + )); + } + + // Corroboration is a separate switch from the council itself, so the merge + // can be measured before a second agent is what is being judged. Off, the + // findings are concatenated exactly as the reviewers produced them. + let findings: Vec = if config.council.corroboration { + council::merge(per_reviewer) + } else { + per_reviewer.into_iter().flatten().collect() + }; + + // Step 5, once over the merged set rather than once per reviewer. The + // filter can only reject, so more inputs in one pass is identical semantics + // at a fraction of the calls. + let filtered = Falsifier::new(model, config) + .filter(LaneId::Critique, findings, &evidence) + .await; + spend.merge(filtered.spend); + + Ok(FileReview { + summary: summarise( + summary.trim(), + unanchored, + discarded, + &filtered.rejected, + filtered.findings.len(), + ), + findings: filtered.findings, + resolved, + spend, + }) +} + +/// What one reviewer said about one file. +struct Asked { + summary: String, + resolved: Vec, + findings: Vec, + spend: Spend, + unanchored: usize, + discarded: usize, +} + +/// Ask one reviewer about one file, and place what it said. +async fn ask( + model: &dyn Model, + input: &LaneInput<'_>, + changed_paths: &[String], + diff: &FileDiff, + evidence: &str, + reviewer: &council::Reviewer<'_>, +) -> Result { + let config: &Config = input.config; let built = prompt::build(&PromptInputs { repo_policy: input.repo_policy, extracted_rules: input.extracted_rules, prior_findings: input.prior_findings, - new_evidence: &evidence, + new_evidence: evidence, // Every path the pull request touched, not just this one. This selects // which `path_instructions` are injected, and narrowing it to the focus // file would silently drop the rules for every other changed path from @@ -141,6 +237,7 @@ async fn review_file( // well as the rules. changed_paths, focus_path: Some(&diff.path), + persona: reviewer.persona, retrieved_context: input.retrieved_context, ..PromptInputs::new(LaneId::Critique, config) }); @@ -150,7 +247,7 @@ async fn review_file( // message is the one part guaranteed to be sent first. let response = model .complete(ModelRequest { - model: config.model_for(LaneId::Critique).to_string(), + model: reviewer.model.to_string(), messages: vec![ Message::system(built.prefix()), Message::user(built.suffix()), @@ -202,7 +299,7 @@ async fn review_file( diff: Some(diff), file: input.file_contents.get(&raw.path).map(String::as_str), comment: &comment, - rendered_diff: &evidence, + rendered_diff: evidence, }, &mut spend, ) @@ -232,24 +329,16 @@ async fn review_file( findings.push(finding); } - // Step 5, on the findings that survived positioning. It sees only the - // diff, and it can only remove. - let filtered = Falsifier::new(model, config) - .filter(LaneId::Critique, findings, &evidence) - .await; - spend.merge(filtered.spend); - - Ok(FileReview { - summary: summarise( - parsed.summary.trim(), - unanchored, - discarded, - &filtered.rejected, - filtered.findings.len(), - ), - findings: filtered.findings, + // Falsification is deliberately *not* here: it runs once over the merged + // set in `review_file`, because a reject-only filter given more inputs in + // one pass has identical semantics at a fraction of the calls. + Ok(Asked { + summary: parsed.summary, resolved: parsed.resolved, + findings, spend, + unanchored, + discarded, }) } diff --git a/src/lanes/description.rs b/src/lanes/description.rs index 2ffd7002..6957d2a0 100644 --- a/src/lanes/description.rs +++ b/src/lanes/description.rs @@ -158,6 +158,7 @@ fn empty_body_outcome(pr: &PullRequest, files: usize) -> LaneOutcome { applicable: None, late: false, identity: None, + corroboration: 1, }], resolved: vec![], spend: Default::default(), diff --git a/src/lanes/mod.rs b/src/lanes/mod.rs index 5c6fc58e..39914dc9 100644 --- a/src/lanes/mod.rs +++ b/src/lanes/mod.rs @@ -251,6 +251,7 @@ mod outcome_tests { applicable: None, late: false, identity: None, + corroboration: 1, } } diff --git a/src/lib.rs b/src/lib.rs index 07348c45..06645eba 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -13,6 +13,7 @@ pub mod app; pub mod automerge; pub mod chunk; pub mod config; +pub mod council; pub mod error; pub mod eval; pub mod evidence; diff --git a/src/overview/test.rs b/src/overview/test.rs index 537d98d1..32cc769b 100644 --- a/src/overview/test.rs +++ b/src/overview/test.rs @@ -46,6 +46,7 @@ fn finding(path: &str, severity: Severity) -> Finding { applicable: None, late: false, identity: None, + corroboration: 1, } }