review: human-resolved bot threads become an adjudicated suppression corpus - #332
Conversation
🦋 Changeset detectedLatest commit: 1c2a123 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Review GuidanceCommon patterns2 files: collectUnresolvedThreads renamed to collectReviewThreads at its definition and its call site (old name re-introduced as a thin filter wrapper in threads.ts) - const allThreads = await collectUnresolvedThreads(ghGraphql, owner, repoName, prNumber);
+ const fetchedThreads = await collectReviewThreads(ghGraphql, owner, repoName, prNumber);2 files: Parallel documentation additions describing adjudicated-threads.json and the adjudicated-thread suppression pass in both the feature overview and the agent-facing staging reference -duplicate; a suppressed blocking candidate still floors the verdict), and
+duplicate; a suppressed blocking candidate still floors the verdict),
+adjudicated-thread suppression (a non-blocking candidate re-deriving a defect
+a human settled by resolving the bot's thread posts no new thread; a blocking
+candidate is never suppressed this way, so a regression re-flag stays
+visible), and |
There was a problem hiding this comment.
Approved — no blocking issues found.
Note: correctness-reviewer not assessed this run (correctness-reviewer output unavailable).
Note: skill-auditor not assessed this run (skill-auditor output unavailable).
Note: test-adequacy not assessed this run (test-adequacy output unavailable).
Note: holistic not assessed this run (holistic output unavailable).
Note: first-principles not assessed this run (first-principles output unavailable).
Note: conventions not assessed this run (conventions output unavailable).
Note: documentation not assessed this run (documentation output unavailable).
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus cc11988c0918 (9 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
…ity primitives out of dedup.ts The merge with #328 pushed dedup.ts past the max-lines budget (1008). contentTokens, bigrams, and intersectionSize are dependency-free and shared by every similarity floor, so they move to dedup-text.ts (the dedup-cluster.ts precedent), with the calibration-coupling warning in the module doc. No behavior change; the floors and their calibration notes stay in dedup.ts beside the rules they price.
| @@ -0,0 +1,43 @@ | |||
| /** | |||
There was a problem hiding this comment.
thought (non-blocking): New file dedup-text.ts (STOPWORDS/contentTokens/bigrams/intersectionSize extracted from dedup.ts) is not mentioned in the PR's Structure section. The extraction is a behavior-neutral code move (verified: the primitives are byte-identical to those deleted from dedup.ts, and dedup-adjudicated.ts does not even import it), so this is silent scope rather than a functional gap — the otherwise-exhaustive Structure section just omits this one added file. Non-blocking; the feature itself fully matches its stated intent.
| * The text-similarity primitives every dedup tier scores with: content | ||
| * tokenization (lowercased alphanumerics, stopwords and short words | ||
| * dropped), token bigrams, and set intersection. Split from `dedup.ts` for | ||
| * its max-lines budget (the dedup-cluster.ts precedent); they are |
There was a problem hiding this comment.
thought (non-blocking): Extraction restores compliance but almost no headroom under the 1000-line cap. I verified the premise holds: without this split dedup.ts would be ~1007 lines against the @khanacademy/eslint-config 1000-line cap, so the extraction is justified. But only the three dependency-free text primitives (~33 lines) were carved out, and only dedup.ts imports dedup-text.ts — the seam was chosen for minimal motion, not durable headroom, leaving the file one small feature away from the cap again.
A sketch, not a committable replacement:
While the file is already being split, consider extracting a larger coherent tier (e.g., the floor constants plus the scoring/matcher helpers that dedup-adjudicated.ts consumes) into the shared module, so the next change to dedup.ts isn't immediately renegotiating the line budget.
…locking-only re-reviews (#340) 🖍 _This is an audit!_ 🖍 ## What Two coupled changes, one install: 1. **Bumps this repo's installed PR reviewer from `review-v1.12.0` to `review-v1.13.0`.** 3-way merge (base = v1.12.0 release source, ours = installed copy, theirs = v1.13.0 release source) with one trivial conflict at the KHAN/ACTIONS LOCAL OVERRIDE comment that rides the checkout `ref:`, then `gh aw compile review`. `source:` and the lib checkout `ref:` stay in lockstep at `review-v1.13.0`; the lock diff is purely version refs plus recompile hashes. 2. **Opts this repo into the new dial**: the ROUTING re-review line becomes `re-review scoped blocking-only` (scoped was already enabled). Repeat reviews keep the full scoped roster, so blocking recall is unchanged; only blocking findings post inline, and validated non-blocking findings collapse to one line each in the review body. First full reviews, divergence-tripwire re-arms, and guard-degraded full runs still post everything. ## What v1.13.0 brings over v1.12.0 - **#328**: pr-level (pathless) claims respect open-thread suppression (stricter `PR_LEVEL_FLOOR` paying for the missing anchor), and long pr-level body folds collapse past 400 chars. - **#329**: the `blocking-only` re-review modifier this PR enables, with the cap-overflow neutral wording and the set-membership modifier parse. - **#336**: the clusterer prompt example no longer promises cross-file grouping the same-path rule forbids. ## Configuration audit No other consumer configuration is needed: v1.13.0 adds no imports, no safe outputs, and no `.github/aw/` config files (verified against the v1.12.0→v1.13.0 diff; the prompt delta is 9 lines). The modifier is consumer-side ROUTING parse in the same release, so the bump and the opt-in belong in one PR: on v1.12.0 the extra field would not parse, and without the ROUTING edit the bump is behavior-neutral. Part of the review-bot feedback effort; the adjudication work (#332/#333/#334) lands in the next release and a follow-up bump. Author: jwbron Reviewers: jeresig, somewhatabstract Required Reviewers: Approved By: Checks: ⌛ 1 check is pending, ✅ 4 checks were successful Pull Request URL: #340
…eads become an adjudicated suppression corpus Resolving a bot thread previously removed it from the only suppression corpus (threads.json stages unresolved threads only), so the next run was free to re-derive the same defect with fresh wording as a brand-new thread, which every later accountability recap then enumerated as still unaddressed. Observed on webapp#41290: the author replied to and resolved six variants of one concern at moderation_helpers.go:135 over two days, and a seventh rephrasing posted anyway. The staging now fetches resolution state (isResolved + resolvedBy) and writes adjudicated-threads.json: bot-opened threads a HUMAN resolved. The dispatcher runs a second suppression pass over it after open-thread suppression, with two deliberate asymmetries: a thread the bot itself resolved (the reconciler marking a defect fixed) never joins the corpus, and a blocking candidate is never suppressed by it, so a fixed-then-regressed defect worth stopping the PR for always posts. Every guard fails closed toward posting a duplicate, never toward dropping a finding; an older staging without the file degrades to an empty corpus. Split per the max-lines convention: the corpus filter, the suppression pass, and the composed two-pass entry point live in dedup-adjudicated.ts (dedup.ts exports its matcher internals); the dispatch-level tests live in dispatch-adjudicated.test.ts.
…origin/main' into jwies-review-adjudicated-suppression
|
Post-deploy data point for the family-matching follow-up (window 2026-08-11T16:00Z through 2026-08-12T19:35Z in webapp; the reviewer deployed at the window start): webapp#41290 now carries five bot threads on one mechanism (the tmv2 CEDAR side effect on flagged turns) across two files. Pre-deploy: r3756443933 and r3756443949 ( |
…s a corrected discussion erases Merge resolution: main's suppressTrackedDuplicates (open + adjudicated corpora, #332/#333) replaces the open-only pass inside suppressThenMergeCrossFile; the cross-file merge still runs after both suppression passes, and shapeFailure rides the composed result. Blocking re-review finding: applyVerifications applies a validator corrected.discussion by wholesale replacement, which erased the merged survivor's 'Also applies to' line and lost the sibling file's finding from all posted output. The line is now one renderer (alsoAppliesLine) and reapplyCrossFileOccurrences re-builds it from the merge records after validation; unit and dispatch-level tests cover the corrected, untouched, and dropped-survivor shapes.
What
Resolving a bot thread now means "settled" instead of "open season for a rephrase". The staging fetches resolution state (
isResolved+resolvedByover GraphQL) and writes a newadjudicated-threads.json: bot-opened threads a HUMAN resolved. The dispatcher runs a second suppression pass over that corpus after open-thread suppression, using the same defect-identity match (same path, no line window, the #245 similarity floors), and drops any non-blocking candidate that re-derives an adjudicated defect before validation ever sees it.Why
Suppression was keyed on
threads.json, which stages only UNRESOLVED threads, so resolving a bot thread quietly removed the defect from the suppression shield; the next run could re-post the same concern with fresh wording as a brand-new thread, which every later accountability recap then enumerated as "still unaddressed".Measured on webapp#41290: the author replied to and resolved six variants of one concern at
moderation_helpers.go:135over two days, and a seventh rephrasing posted anyway (review 4875909452), then headlined every subsequent recap. From the author's side that is indistinguishable from the bot ignoring every resolution; it is the exact "resolving the comments does nothing" report from the review-bot feedback thread.Semantics
Two deliberate asymmetries against open-thread suppression, both fail-safe:
resolvedBy(an unattributable resolver, e.g. a deleted account, reads as no adjudication).A candidate matching BOTH corpora is attributed to the OPEN thread (pass order is a property of the composed entry point, not the call site), so the verdict floor reads the open thread's blocking state. Suppression records carry
adjudicated: truefor the audit trail; they can never floor the verdict (the floor requires the candidate's label to be blocking, which no adjudicated suppression's is). Every guard fails closed toward posting a duplicate, never toward dropping a finding, and an older staging without the file degrades to an empty corpus.Structure
Split per the max-lines convention: the corpus filter, the suppression pass, and the composed two-pass entry point live in
dedup-adjudicated.ts(dedup.tsexports its matcher internals). A follow-up commit extracted the dependency-free text-similarity primitives (STOPWORDS,contentTokens,bigrams,intersectionSize) intodedup-text.ts, byte-identical, to keepdedup.tsunder the 1000-line cap. Dispatch-level tests indispatch-adjudicated.test.ts, unit tests indedup-adjudicated.test.ts, staging tests extendstage-threads.test.tswith the producer-to-consumer byte bind.Tests
New: corpus admission and every fail-closed guard; non-blocking suppression with the
adjudicatedmarker; the blocking exemption; both-corpora attribution to the open thread; staging partition (human-resolved bot thread lands in the adjudicated file and nowhere else, bot-resolved and human-opened resolved threads excluded, noresolvedByleak intothreads.json); end-to-end dispatch suppression pre-validation and the blocking re-flag reaching the validator. Fullworkflows/reviewsuite green;tscand eslint clean.Follow-ups (not in this PR)
/reconsiderper-finding re-evaluation (needs a design note first).