diff --git a/.changeset/review-design-notes.md b/.changeset/review-design-notes.md new file mode 100644 index 00000000..e92010d7 --- /dev/null +++ b/.changeset/review-design-notes.md @@ -0,0 +1,4 @@ +--- +--- + +Docs only: add `workflows/review/design/` with two design notes; mechanism-level cross-file suppression (the #332 follow-up) and a roster-wide non-blocking volume cap for first reviews. Both are eval-gated plans, not shipped behavior. diff --git a/workflows/review/design/family-suppression.md b/workflows/review/design/family-suppression.md new file mode 100644 index 00000000..bc0ee6ce --- /dev/null +++ b/workflows/review/design/family-suppression.md @@ -0,0 +1,130 @@ +# Design note: mechanism-level (cross-file) suppression + +Follow-up to Khan/actions#332 (adjudicated suppression corpus). Status: design +only; no code should land before the eval plan at the bottom has corpus cases +and a measured precision/recall baseline. + +## The measured problem + +Khan/webapp#41290 carries five bot threads on one mechanism: the tmv2 CEDAR +mode-change event publishing from inside a pre-flight modifier, which the PR's +in-parallel arm lets fire on flagged turns. + +- Pre-deploy (before the 2026-08-11 reviewer deploy): r3756443933 and + r3756443949, both anchored on `chat/modifiers/v2/tmv2.go`. +- Post-deploy: r3767202100 and r3767482526, anchored on two different lines of + `moderation/spec/SPEC.md`. +- r3761098824 (`SPEC.md`), a provider-transmission variant of the same + side-effects-on-flagged-turns concern, which the author downvoted; the + thumbs sweep's "why?" follow-up got the closed-vocabulary answer + `duplicate` 92 seconds later. + +Each thread is individually defensible: different anchor, different wording, +a genuinely distinct consequence of the one mechanism. Together they read as +the bot re-arguing a concern the author has already engaged with, and they +are the post-#312 remainder of the "keeps flagging the same things" report: +in-run clustering and the #332 corpus both key on path, so a variant that +moves to a different file always posts. + +## Why #332 misses it + +The adjudicated corpus filter reuses the open-thread suppression matcher: +same path (no line window), then the #245 similarity floors over the claim +text. That path key is deliberate; it is what keeps the matcher's +false-suppression risk low enough to run pre-validation with no model call. +A cross-file variant fails the path key before similarity is ever computed, +so the corpus cannot suppress it however similar the prose is. + +## Candidate designs + +Three candidates, ordered by how much judgment they add. Each keeps the #332 +fail-closed rules (below); the question each must answer is what its +false-suppression rate is on findings that merely share vocabulary with an +adjudicated thread. + +### (a) Reconciler judgment pass + +Extend the `thread-reconciler` sub-agent's mandate: alongside its +keep/resolve decisions over the bot's open threads, it reads the adjudicated +corpus and marks each new non-blocking candidate as re-deriving a settled +mechanism or not. Matching becomes a model judgment with the full thread +reply chains as context. + +- For: the reply chains are exactly the signal a mechanism match needs (the + author's "we accept this side effect" settles every variant of it), and the + reconciler already reads them. +- Against: a model judgment can suppress a finding for reasons no one can + audit; the suppression records would need to carry the reconciler's stated + rationale, and the dispatch gate would need to verify every suppression is + backed by a staged reconciler decision (the same pattern the thread + resolutions use today). Adds model spend on every re-review with a + non-empty corpus. + +### (b) Cross-file similarity + +Drop the path key for the adjudicated corpus only (open-thread suppression +keeps it): a non-blocking candidate is suppressed when its claim text clears +the similarity floors against any adjudicated thread, whatever the paths. + +- For: pure code, no new model call, smallest diff. +- Against: the similarity floors were tuned with the path key in front of + them. Without it, two findings that share domain vocabulary ("flagged + turns", "pre-flight modifier") but describe different defects are the + obvious false-suppression case. The floors would need re-tuning against a + corpus that contains hard negatives, and the 41290 family itself shows the + wording of true variants diverges a lot (a spec-caveat suggestion versus a + telemetry-leak note), so recall may stay low even after tuning. This + candidate is cheap to evaluate and plausibly not good enough. + +### (c) Mechanism fingerprints minted by finders + +Add an optional `mechanism` field to the finding schema: a short, code-like +identifier for the underlying mechanism (for the family above, something like +`tmv2-cedar-publish-on-flagged-turn`), minted by the finder sub-agents and +carried through staging into the corpora. Matching is then exact or +near-exact string comparison in code. + +- For: matching stays deterministic and auditable; no new model pass at + suppression time. +- Against: the fingerprint is only as stable as the models' naming. Two runs + can mint two names for one mechanism (splitting the family), or one name + for two mechanisms (merging them, the false-suppression case). Needs a + schema version bump, prompt changes in every finder, and a measured + name-stability rate before it can be trusted. Unknown today: whether + name stability across runs and across finder roles is anywhere near high + enough; nothing in the current corpus measures it. + +## Fail-closed rules that carry over unchanged + +Whatever the matcher, the #332 asymmetries stay: + +- A blocking candidate is never suppressed by the adjudicated corpus. A + regression worth stopping the PR for re-presents at blocking severity and + posts. +- A thread the bot resolved never joins the corpus; membership requires a + human `resolvedBy` (or, with #333, a downvoted opener). +- Absent, malformed, or unmatchable data reads as no adjudication: every + guard fails toward posting a duplicate, never toward dropping a finding. +- A candidate matching both corpora is attributed to the open thread, so the + verdict floor keeps reading the open thread's blocking state. + +## Eval plan (before any code) + +1. Build corpus cases from the 41290 family: the real threads and reply + chains as the adjudicated corpus, plus candidate findings in three + classes: true variants (the five threads above, re-derived), hard + negatives (findings on the same files sharing vocabulary but describing + different defects), and unrelated controls. +2. Score each candidate design as a classifier over those cases: suppression + precision (hard negatives must post) and recall (true variants must be + suppressed). Candidate (b) can be scored offline from the existing + matcher; (a) and (c) need producer support to dispatch the judgment pass + or mint fingerprints. +3. Set the acceptance bar before running: a false suppression is worse than + a duplicate post, so precision gates and recall ranks. A candidate that + cannot beat the do-nothing baseline's annoyance cost (measured duplicate + rate) does not land. +4. Whatever lands, add the suppression records to the run artifact with the + match basis (path, similarity score, reconciler rationale, or + fingerprint), so a wrong suppression is diagnosable from the artifact + alone. diff --git a/workflows/review/design/volume-cap.md b/workflows/review/design/volume-cap.md new file mode 100644 index 00000000..de6459e8 --- /dev/null +++ b/workflows/review/design/volume-cap.md @@ -0,0 +1,82 @@ +# Design note: a roster-wide non-blocking cap for first reviews + +Status: design only. The cap is a posting-surface change with a real recall +cost if done wrong, so it ships opt-in via `ROUTING` (the way `enable` lines +and `re-review` modes do) and only after the live A/B has priced it. + +## The measured problem + +Khan/webapp#41440 drew 15 inline comments in a single first review: 2 +blocking, 13 non-blocking. The non-blocking set contained overlap clusters +the in-run clusterer (#312) did not merge: an exact duplicate body posted on +two sibling eval files (r3764122555 and r3764122558, which the path-keyed +clusterer cannot group), a note plus a question on the same VisualJudge +default change, and four comments circling one decision-tree interaction +(the YES-table/Q3 bypass). The author engaged with all of it, but 15 threads +on one push is the attention failure mode the documentation reviewer's +volume policy already names: the marginal thread costs reviewer attention +that the marginal finding does not repay. + +Re-reviews already have a lever (`blocking-only`, #329). First reviews have +none: every validated finding posts inline, however many there are. + +## Prior art to build on + +- **The documentation reviewer's caps** (one finding per comment, two per + file, five per review) with ranked clause shedding: the policy's clauses + are ordered and the tail drops from the bottom. This is the shape to + generalize: a cap plus an explicit ranking, so what sheds is chosen, not + arbitrary. +- **The `blocking-only` modifier's collapse rendering** (#329): validated + non-blocking findings render as one line each inside a `
` block in + the review body instead of posting inline. The rendering path exists; a + first-review cap can reuse it for the tail rather than dropping findings. +- **Recap damping** (#334): the same principle (full text once, label plus + link after) applied to the accountability section. + +## Design questions + +1. **Ranking signal.** Candidates: severity label (blocking always posts and + never counts against the cap; then suggestion over note over question over + nitpick over thought), validator confidence if the validator can be made + to emit one, finding source (correctness findings over advisory + reviewers'), risk tier of the anchored file. Unknown: whether the + validator's confidence is meaningful enough to rank on; nothing measures + it today. +2. **Cap value and grain.** The documentation reviewer uses 5 per review and + 2 per file. A roster-wide cap has more sources feeding it; whether the + right number is 8, 10, or per-file-only needs the A/B, not intuition. + 41440's 15 would have been over any plausible value. +3. **Collapse versus drop.** The tail should collapse into the review body's + `
` block (the #329 rendering), not drop: the finding was + validated, the author may want it, and a collapsed line costs near-zero + attention. Dropping is only defensible if the A/B shows collapsed tails go + unread anyway. +4. **Interaction with clustering.** The cap counts post-dedup, post-cluster + findings; otherwise it double-punishes the clusterer's misses. The 41440 + sibling-file duplicate argues for fixing cross-file clustering first, so + the cap does not paper over a dedup gap. + +## Invariants + +- **The verdict floor is untouched.** The verdict stays computed from every + validated claim; the cap moves findings off the inline surface, never out + of the verdict. This is the same rule `blocking-only` already follows, and + it is what keeps the cap from ever flipping an outcome. +- **Blocking findings never collapse.** Same as #329 and #334. +- **The shed is disclosed.** The review body says how many findings + collapsed and why (cap), the way budget sheds and skipped dimensions are + disclosed today; a silent cap is indistinguishable from lost coverage. +- **The dispatch gate extends naturally**: a queued inline comment set larger + than the staged plan's capped set is a conformance violation like any + other plan deviation. + +## Measurement before default + +Price the cap with the live A/B: arms identical except a `ROUTING` line +(spelling to be decided; a `first-review` counterpart to the `re-review` +line, or a modifier on it). Score comment volume, must-catch recall (a +capped run must not collapse a must-catch into the tail), and the judge's +quality read, with the usual caveat that recall against labeled specs is the +load-bearing metric. Ship as opt-in; a default change needs its own measured +argument, the way `re-review` modes earn theirs.