fix(adversarial): report the finding denominator, not just the reviewer one - #47
Merged
Merged
Conversation
…er one 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: 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: 35 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. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
stranske
added a commit
that referenced
this pull request
Aug 23, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-on to #33, which fixed the reviewer shortfall. That fix made the reviewer denominator
visible and left the finding denominator invisible — which 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_promptasks each reviewer for "the single most serious problem"._first_jsonkeeps the first object carrying ablockerkey.So a reviewer contributes at most one verdict however many claims the context held, and verdicts
are unattributed — 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 produced #33 got verdicts on 1 of 5 submitted findings while the
payload implied all five had been examined.
The fix
aggregate_vetoandreview()gain an optionalfindings_submitted.unknown, because absence must never read as "all covered" — the same rule the Brain already
applies to missing cost telemetry.
findings_adjudicated_max,findings_unexamined_min(a rigorousfloor, since one verdict settles at most one claim),
findings_attributed: false, and appendsfindings adjudicated at most 1 of 5to the summary.INCONCLUSIVEfor the same reason a short panel is: asserting PASS overfive claims having examined at most one is not supportable. A corroborated
BLOCKEDstill wins —a real blocker is actionable whatever else went unexamined.
The exact audit shape now names both shortfalls at once:
{"verdict": "INCONCLUSIVE", "summary": "1 veto / threshold 2, reviewers returned 1 of 2, findings adjudicated at most 1 of 5", "findings_submitted": 5, "findings_adjudicated_max": 1, "findings_unexamined_min": 4, "findings_attributed": false}Test gate
python3 adversarial.py --selftest. Five new cases — unknown stays silent and byte-identical;5 claims / 3 verdicts is INCONCLUSIVE with a floor of 2; the exact audit shape names both
shortfalls; full coverage of one claim by a big-enough panel is still
PASS; a corroboratedBLOCKEDwins over incomplete coverage — plus a deliberate break → revert on the new_coverage_floorhelper that asserts"break did not change behaviour — test is vacuous".Dedup
Searched other sessions' transcripts for
aggregate_veto: the only hit was the audit session thatreported this: no session was building it.
runtime_ac_panel.adjudicate_panelremains untouched.Not fixed, deliberately
Per-finding verdicts would need multi-object parsing and a stricter reviewer schema — and the same
audit recorded that strict JSON shapes break subagents. The honest denominator ships first;
per-finding adjudication stays a separate decision rather than being smuggled in here.
Verification
python3 verify.py: 368 passed, 0 failed, 83/83 selftests, 5 of 5 gates green, zero skips.🤖 Generated with Claude Code