Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .changeset/review-design-notes.md
Original file line number Diff line number Diff line change
@@ -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.
130 changes: 130 additions & 0 deletions workflows/review/design/family-suppression.md
Original file line number Diff line number Diff line change
@@ -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.
82 changes: 82 additions & 0 deletions workflows/review/design/volume-cap.md
Original file line number Diff line number Diff line change
@@ -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 `<details>` 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
`<details>` 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.
Loading