fix(adversarial): a veto panel too small to block must not report PASS - #33
Conversation
`aggregate_veto` reported PASS whenever the reviewer population shrank
below `veto_threshold`. The threshold was then unreachable BY
CONSTRUCTION and the failure presented as silence.
Observed live, not inferred (experiment advice:a6cc531b8010; full
provenance in the capability's ledger notes): reviewers=[codex,vibe] with
veto_threshold=2, vibe returned null, and the function returned
{"verdict": "PASS", "n_reviewers": 1, "n_vetoes": 1} while CARRYING a
high-severity 0.99-confidence blocker. aggregate_veto([]) was the same
bug at its worst — zero reviewers returning read as a clean pass.
It survived because the selftest ASSERTED it: the old line read
`# one high veto below threshold 2 -> still PASS (needs corroboration)`,
which in a passing test is indistinguishable from the intended
minority-veto behaviour.
- n_reviewers < veto_threshold now yields a distinct INCONCLUSIVE verdict
and still carries the blockers, so the gate fails toward motion rather
than silence.
- The payload reports the drainable quantity beside the blocking one:
reviewers_requested, reviewers_missing, threshold_reachable, and a
summary of the form "1 veto / threshold 2, reviewers returned 1 of 2".
The measuring window (reviewers that RETURNED) was never the same
population as the window the threshold is chosen against (reviewers
REQUESTED); both are now stated in one place.
- review() passes reviewers_requested=len(reviewers).
- A REACHABLE threshold the panel declines to meet is still PASS, so the
no-single-voice-tyranny property is unchanged.
Selftest pins all four cases (1-of-2 shortfall, None placeholder, 0-of-3
total failure, genuine 1-of-3 minority) with a deliberate break -> revert
on _threshold_reachable proving the shortfall branch is not vacuous.
Dedup (CLAUDE.md 0): grepped by concept for veto / threshold / quorum /
inconclusive / n_reviewers / aggregate — no sufficiency check existed
anywhere. runtime_ac_panel.adjudicate_panel is the nearest neighbour and
is deliberately untouched; ARCHITECTURE.md:394 and roles.py:751 pin both
as rails. Not a new capability: the existing rail is extended.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 47 minutes Limit details: You’ve used the included review currently available. Your 70 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Comment |
…er one (#47) The reviewer-shortfall fix in #33 made the REVIEWER denominator visible and left the FINDING denominator invisible. That made #33 worse in one respect: once one shortfall is reported, silence about the other reads as deliberate. Mechanism, verified in code rather than inferred. `refute_prompt` asks each reviewer for "the single most serious problem" and `_first_json` keeps the FIRST object carrying a `blocker` key, so a reviewer contributes AT MOST ONE verdict however many claims the context held — and verdicts are unattributed, so nothing maps a verdict back to the claim it judges. Handed three verdicts in one response, only the first survives. That is why the audit run that found #33 got verdicts on 1 of 5 submitted findings and the payload implied five. - `aggregate_veto` and `review()` gain an optional `findings_submitted`. Omitted, the payload is byte-identical and existing callers are untouched: unknown stays unknown, because absence must never read as "all covered" — the same rule the Brain applies to missing cost telemetry. - Given a count it reports `findings_adjudicated_max`, `findings_unexamined_min` (a rigorous FLOOR, since one verdict settles at most one claim), `findings_attributed: false`, and appends "findings adjudicated at most 1 of 5" to the summary. - Incomplete coverage is INCONCLUSIVE for the same reason a short panel is: asserting PASS over five claims having examined at most one is not supportable. A corroborated BLOCKED still wins — a real blocker is actionable whatever else went unexamined. Selftest adds five cases (unknown stays silent; 5-of-3 coverage; the exact audit shape naming BOTH shortfalls; full coverage still PASS; BLOCKED wins) plus a deliberate break -> revert on the new `_coverage_floor` helper. NOT fixed, deliberately: per-finding verdicts need multi-object parsing and a stricter schema, and the same audit recorded that strict JSON shapes break subagents. The honest denominator ships first; per-finding adjudication stays a separate decision. Co-authored-by: Tim Stranske <tim@stranskemo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…es (#49) Audit recommendations 4 and 6 from the same run that produced #33 and #47. classify_task matched every signal as a bare PREFIX: the leading `(?<![a-z])` was there, the trailing boundary was not. With MIN_SIGNAL_HITS = 1, one substring is enough to bind a task type, so: "a read-only audit of the implementation of the config loader; do not change code" -> ['implement', 'review'] `implement` inside the noun "implementation" offered a code-mutating lane to work that must not touch code. `ui` likewise reached "uid", `test` reached "testgen". The rule is whole-word-with-intent: inflections that PRESERVE intent still count (SIGNAL_INFLECTIONS: plurals, participles, agent/result nouns), while derivational drift does not — above all `-ation`, which turns a verb into the name of a thing that already exists. Two-letter signals take no inflection, because they are initialisms and initialisms do not inflect. Without that carve-out `ui` still reached "uid" through the bare `-d` ending (which exists for the -e verbs: dedupe/deduped), so the boundary would have LOOKED like it fixed a false positive it had not. The rule was chosen from measurement, not intuition. Tested against a corpus of realistic task sentences, a naive trailing boundary caused three collateral losses: "run the testgen lane", "screenshot the output" and "formatting only" (gemination — `format` + t + ing is unreachable by any suffix rule). Each is restored by spelling the form out in TASK_SIGNALS, which is that table's existing idiom: it already lists "tests" beside "test" and "documentation" beside "docs". Re-measured after: exactly ONE behaviour change remains, the audit's own case. Also documents local_verify.py's precondition (recommendation 6): the fix must ALREADY be in the worktree. It is a phase-4 tool, so pointing it at a bare finding makes step 1 fail, and a step-1 failure means "your test command does not pass here", NOT "the finding is unreal" — the two read identically if you expected a verdict on the finding. Test gate: `python3 capability_advisor.py --selftest` pins the audit's case, the four inflected verb forms, the three restored signals, the initialism carve-out, and the -e verb keeping its bare -d, with a deliberate break -> revert on the trailing boundary. Co-authored-by: Tim Stranske <tim@stranskemo.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The defect
adversarial.aggregate_vetoreportedPASSwhenever the reviewer population shrank belowveto_threshold. The threshold was then unreachable by construction, and the failure presentedas silence.
Observed live, not inferred (experiment
advice:a6cc531b8010; full provenance in the capability'sledger
notes, since the evidence is instance-local and not committed). Withreviewers=["codex","vibe"]andveto_threshold=2, vibe returnednull:{"verdict": "PASS", "n_reviewers": 1, "n_vetoes": 1, "veto_threshold": 2, "blockers": [{"severity": "high", "confidence": 0.99, "finding": "..."}]}verdict: "PASS"while carrying a high-severity 0.99-confidence blocker.aggregate_veto([])was the same bug at its worst: zero reviewers returning read as a clean pass.
The module's own docstring says "Adjudicate, don't obey" — which is exactly why the aggregate must
refuse to say PASS when PASS is the only thing it could have said.
Why it survived: the selftest asserted it
In a passing test that is indistinguishable from the intended minority-veto behaviour. The two cases
were never separated: a panel that could have blocked and declined to (a real
PASS), versus apanel too small to block at all.
The fix
n_reviewers < veto_thresholdyields a distinctINCONCLUSIVEverdict that still carries theblockers — the gate fails toward motion, not silence.
reviewers_requested,reviewers_missing,threshold_reachable, and asummaryof the form"1 veto / threshold 2, reviewers returned 1 of 2". The measuring window (reviewers thatreturned) was never the same population as the window the threshold is chosen against (reviewers
requested); both are now stated in one place.
review()passesreviewers_requested=len(reviewers).PASS, so theno-single-voice-tyranny property the minority-veto design exists for is unchanged.
_threshold_reachableis a named function rather than an inline comparison specifically so theselftest can break it.
Behaviour, before → after
PASSINCONCLUSIVE—1 veto / threshold 2, reviewers returned 1 of 2PASSINCONCLUSIVE—0 vetoes / threshold 2, reviewers returned 0 of 3PASSPASS(unchanged)BLOCKEDBLOCKED(unchanged)Test gate
python3 adversarial.py --selftest(run byverify.py's selftest discovery). Pins all four casesabove, plus a
None-placeholder case, plus a deliberate break → revert on_threshold_reachablethat asserts
"break did not change behaviour — test is vacuous"— so the shortfall branch cannotsilently stop being load-bearing.
Dedup finding (CLAUDE.md §0)
Grepped by concept for veto / threshold / quorum / inconclusive /
n_reviewers/ aggregate across*.py. No sufficiency or quorum check existed anywhere.runtime_ac_panel.adjudicate_panelisthe nearest neighbour and is a separate aggregation this change deliberately does not touch —
ARCHITECTURE.md:394androles.py:751pin both as rails. The only other references toaggregate_vetowere docs and the module's own selftest. Not a new capability: the existing rail isextended. Recorded in the capability's ledger
notes, not only here.Downstream
tick.pypasses the verdict through tofeedback.record_completion_event(status=...), whichnormalises free-form status — no enum to extend.
pattern_miner.POSITIVE_VERDICTSexcludesinconclusive, so an inconclusive panel correctly doesnot train as a pass.
roles.adjudication_case_for_disagreementwill now readINCONCLUSIVEagainst a gatePASSas adisagreement and open an adjudication case. Intended — that is the fail-toward-motion behaviour —
and inert today because
ORCH_RUN_ADVERSARIAL_REVIEWdefaults off.Verification
python3 verify.pylocally: 365 passed, 3 failed, 82/82 selftests ran, ledger valid.The 3 failures are pre-existing and unrelated — proven by stashing this change and re-running,
where they reproduce identically. All three are the
evidence-acquisitioncapability, registered inthis instance's local ledger with entrypoint
evidence_acquisition.py:run, a file present in no treeand no commit. They are instance-local ledger state, so CI (which bootstraps an empty
ORCH_LOCAL_RUNTIME) should not see them. Being tracked separately; untouched here.Mirror verification is pending —
orch-sync-mirror.shdefaults its source to the canonical clone,which cannot carry this change until it is merged and pulled. The owner owns that sync.
🤖 Generated with Claude Code