review: adjudicated-corpus suppression drops its same-path key - #366
Conversation
…ssion drops its same-path key A human-settled defect's rephrasing routinely re-anchors on another file (the spec instead of the implementation, the test instead of the function), and the path key is what let the webapp#41290 duplicate families re-post for two weeks after the author adjudicated them. Measured on that frozen corpus (12 adjudicated threads, 33 labeled candidates, kept privately in the planning tree): path-keyed matching scores 2/12 true variants suppressed with 1 false suppression (a same-path vocabulary-split pair); with the key dropped, 6/12 at the same single false suppression and correct family attribution on every match. Tripled recall, zero added false suppressions. The asymmetry that licenses the wide match is the corpus's human membership signal: a false match here eats a finding a human already settled, and never a blocking one. The open-thread corpus stays path-keyed (there a false cross-file match hides an undecided finding); blocking candidates are still never suppressed; every other #332 fail-closed guard is unchanged.
🦋 Changeset detectedLatest commit: 1b12133 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 live A/BNo reviewable delta: review.md is byte-identical in both arms (baseline |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested — see inline comments.
Note: claim validation not assessed this run (claim-validator output unavailable).
review details
review-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation
- the changeset now bumps review (minor, matching the 1.17.0 precedent for a suppression-behavior change) - an adjudicated thread staged without a usable path stays inert under ignorePath instead of becoming a PR-wide matcher, with a test pinning both the undefined and empty-path shapes - the cross-file floor choice is documented as measured (every cross-file negative on the frozen corpus fails jaccard at 0.168 vs the 0.2 floor; the weakest true match sits at 7 shared bigrams, so the pr-level floor of 8 would cost a true variant for no measured precision) and pinned by a fixture in the 6-8 bigram band where the two floors disagree - the sibling-copy interaction with the cross-file merge ordering is documented in dedup-crossfile.ts as deliberate and pinned in both directions (open corpus: tracked file's copy exits, sibling posts; adjudicated corpus: the settled thread takes the near-identical sibling copy too), plus a best-match-across-files attribution test - the circular asymmetry sentence is rewritten (a false match eats an undecided finding; what licenses the reach is the membership rule plus the blocking exemption), vocabulary-split is spelled out, the stale module-header pointer is fixed, and the README feedback contract now says any file
|
Pushed 33479df addressing the round; inline replies aren't available from this session, so per finding: The changeset (both blocking comments plus the conventions/documentation echoes): frontmatter now bumps The cross-file floor (holistic/correctness/first-principles): turns out the corpus answers this directly, so I re-derived instead of arguing from the convention. Every cross-file negative pair on the frozen 41290 corpus fails jaccard, not bigrams: the strongest scores 0.168 against the 0.2 floor while reaching 13 shared bigrams, so jaccard is the guard that holds and a bigram-keyed CROSS_FILE_FLOOR polices the wrong axis. The weakest true cross-file match sits at exactly 7 shared bigrams, so the pr-level tier's floor of 8 drops recall from 6/12 to 5/12 and suppresses zero additional negatives. The floor stays at OTHER_LINE_FLOOR, but the calibration is now written into The suppress-before-merge ordering (the correctness blocker): the interaction is real and it's deliberate, so it's now explicit rather than implied. The ordering rationale still holds for the open corpus, which stays path-keyed. For the adjudicated corpus, a settled thread on file A taking B's near-identical copy is the corpus's semantics: the human declined that exact ask once, the reach is capped at non-blocking, and a blocking re-presentation posts. Note the path key never protected the matching same-file case either; a fresh same-file instance of a settled defect was already suppressed, so this widens the reach of an accepted risk rather than adding a new failure class. The anchorless-thread suggestion was a genuine catch: under The rest:
On the private corpus (the reproducibility question): it's verbatim webapp thread text and it leaked into this public repo once already on a deleted branch, so it stays in the planning tree. What's re-runnable here: the qualitative behaviors are pinned as fixtures (now including the floor band and both ordering directions), and the doc carries the exact numbers as the re-derivation target. The final diff was re-validated by running the frozen corpus through the production |
This comment has been minimized.
This comment has been minimized.
…sion out of dedup.ts The merge with main put dedup.ts at 1021 lines, over eslint's max-lines budget of 1000 (each side was under it alone; CI lints the merge commit). Open-thread suppression is a self-contained concern dedup.ts hosted beside the merge tiers, so it moves to dedup-threads.ts whole, following the dedup-text.ts and dedup-cluster.ts precedent. The calibrated similarity floors move to dedup-text.ts beside the primitives they are calibrated against, which is what keeps the two consumers (dedup.ts and dedup-threads.ts) cycle-free. No behavior change; importers now name the new module, and prose references elsewhere are repointed (including threads.ts's adjudicatedThreadsFromStaged pointer, which named dedup.ts while the function lives in dedup-adjudicated.ts).
Guidance for reviewersTriage notes for reviewers: risky files by owning team, repeated changes, and files excluded from review. github-actions (6 files)
Common patterns4 files: Import path repointed from - import {suppressOpenThreadDuplicates} from "./dedup";
+ import {suppressOpenThreadDuplicates} from "./dedup-threads";5 files: Comment/doc cross-references to - * text-similarity comparison (dedup.ts's `threadProse`). Every posted
+ * text-similarity comparison (dedup-threads.ts's `threadProse`). Every postedExcluded from review (8 files)Not individually reviewed — generated, formatting-only, or
review detailsreview-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation |
There was a problem hiding this comment.
All 16 prior review threads are resolved.
Note: divergence tripwire re-armed a full review (unreviewed share 0.97).
review details
review-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation…s review feedback The widened adjudicated match had only its recall side pinned: the sole negative fixture was same-path and died on bigrams, so the jaccard 0.2 floor (the guard that actually holds cross-file, measured at 0.168 on the frozen corpus's strongest negative) had no CI coverage, and a false cross-file suppression drops a finding silently. The new fixture was built against the real tokenizer to land where only jaccard rejects: 6 shared bigrams (exactly the floor), overlap 0.444, jaccard 0.157. Also from the review: OTHER_LINE_FLOOR's definition-site doc now names its second (cross-file) calibration basis, the moved PR_LEVEL_FLOOR doc no longer says 'this module scores' about a module that scores nothing, and bestOpenThreadMatch's options bag is a defaulted parameter like every other trailing options bag in the lib.
There was a problem hiding this comment.
Approved — no blocking issues found.
All 6 prior review threads are resolved.
Non-blocking observations (4)
workflows/review/lib/dedup-text.ts:68suggestion (non-blocking, documentation): The new OTHER_LINE_FLOOR paragraph restates the calibration figures it then points at. (documentation)workflows/review/lib/dedup-adjudicated.test.ts:342suggestion (non-blocking): The hard-negative fixture's calibration band (6 bigrams / 0.444 overlap / 0.157 jaccard) is documented in a comment but never asserted. (correctness-reviewer)workflows/review/lib/dedup-adjudicated.test.ts:265question (non-blocking): A human resolve can mean "I fixed this file", not only "I decline this ask". (first-principles)workflows/review/lib/dedup-threads.ts:290suggestion (non-blocking): The corpus measured only path-keyed and no-key; a directory-scoped key was not measured. (first-principles)
review details
review-v1.18.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation
2 participants
The adjudicated-corpus suppression was path-keyed, and that key is what let the webapp#41290 duplicate families re-post for two weeks after the author had adjudicated them: a settled defect's rephrasing routinely re-anchors on another file (the spec instead of the implementation, the test instead of the function), and every re-anchoring escaped the corpus. So let's just drop the same-path key in the adjudicated pass only.
The change is measured, not argued. On the frozen 41290 corpus (12 adjudicated threads, 33 hand-labeled candidates: true variants, vocabulary-sharing hard negatives, controls; kept privately in the planning tree since it's verbatim webapp text):
Tripled recall, zero added false suppressions. The remaining 6 escapees are heavily reworded members token floors can't reach at any key width; they're deliberately not chased (the corpus memory self-heals: an escapee that posts gets downvoted or acknowledged once and joins the corpus itself).
What licenses the wide match is the corpus's membership rule plus the blocking exemption: every member carries an explicit human act (a resolve or a 👎), a suppression here only ever eats a non-blocking candidate, and a defect that matters enough to block re-presents at blocking severity and posts. Everything else is unchanged: the open-thread corpus stays path-keyed (there a false cross-file match hides an undecided finding), and the other #332 fail-closed guards hold. Mechanically it's an
ignorePathoption onbestOpenThreadMatchwhose only caller is the adjudicated pass, plus tests pinning the cross-file suppression, the blocking exemption, and the open-corpus asymmetry.Most of the diff by line count is a module split, not behavior: dedup.ts sat at 1021 lines against the repo's 1000-line max-lines budget, so open-thread suppression moved out to a new dedup-threads.ts, following the dedup-cluster.ts/dedup-text.ts precedent; nothing still imports a moved symbol from ./dedup. The behavior change itself is the one-line key drop plus its guards.
The review round tightened four things: an adjudicated thread staged without a usable path now stays inert under
ignorePathinstead of becoming a PR-wide matcher; the cross-file floor choice is documented as measured (every cross-file negative on the corpus fails jaccard at 0.168 against the 0.2 floor, the weakest true match sits at 7 shared bigrams, so the pr-level tier's floor of 8 would cost a true variant for no measured precision) and pinned by a fixture in the 6-8 bigram band where the two floors disagree; the interaction with the cross-file merge ordering is documented as deliberate and pinned in both directions (an open thread takes only its own file's copy and the sibling posts alone, an adjudicated thread takes the near-identical sibling copy too); and the changeset bumpsreviewas a minor, matching the 1.17.0 precedent. The exact final diff was re-validated against the private fixture through the production pass: recall 6/12, the same single false suppression, correct attribution throughout. A later round added the precision half's fixture: a cross-file hard negative built against the real tokenizer to land where only the jaccard guard rejects (6 shared bigrams, exactly the floor; overlap 0.444; jaccard 0.157 against 0.2), so a loosened floor now fails CI instead of silently dropping findings, and the floor docs in dedup-text.ts name the second (cross-file) calibration basis.KORE-2468