review: merge one source's identical finding across files into one pattern-level comment - #348
Conversation
… finding across files into one pattern-level comment
🦋 Changeset detectedLatest commit: 53059ea 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}
…er open-thread suppression Review finding on #348: suppression only matches a thread to a claim on the thread's own path, so merging first let an open thread on the survivor's file swallow every other file's occurrence (author copies flawed a.ts to b.ts, a.ts tracked by an open thread: b.ts never posts, on any run). Order is now suppress-then-merge, composed and documented in dedup-crossfile.ts (suppressThenMergeCrossFile) with a dispatch-level regression test. Also documents the accepted validation asymmetry the re-review asked about (survivor-only validation across files).
Review Guidancegithub-actions (2 files)
|
| * floors above demand identical-or-near-identical text from ONE source, and | ||
| * the occurrence list is prose ("Also applies to..."), not an anchored | ||
| * claim, so a wrong merge publishes a weaker statement about the other file | ||
| * than a separate comment would have. A refuted survivor drops its whole |
There was a problem hiding this comment.
question (non-blocking): Since crossFileMerges records every dropped copy, could a refuted survivor un-merge instead of dropping the group? The header accepts that a refuted survivor drops its whole group and argues the same text is unlikely to be valid on only the sibling — but validation verdicts can hinge on the survivor's file specifically, and unlike the cross-source tiers (whose copies share a path, so one verdict genuinely covers them), here the verdict speaks for files the validator never opened. The merge record already carries enough (id, path, line) to resurrect copies for posting-without-revalidation or at least to emit a note line, keeping the failure cost at 'duplicate comment' rather than 'lost finding'.
…ltering branch; pin staging degradation Re-review notes on #348: the chain test excluded its third claim by label before grouping, so the star guard never ran as a filter; a bigram-chain fixture (A~B and B~C above the floor, A~C below) now reaches it and pins that a bridged member stays its own claim. Direct tests cover suppressThenMergeCrossFile's malformed-staging fallback. The un-merge-on-refutation question stays as designed: a refuted survivor drops its group (documented in the header).
There was a problem hiding this comment.
Changes requested — see inline comments.
2 of 3 prior review threads resolved; 1 still unaddressed as of dbc211d:
1 non-blocking thread still open
- question (non-blocking)
workflows/review/lib/dedup-crossfile.ts:45: Since crossFileMerges records every dropped copy, could a refuted survivor un-merge instead of dropping the group? The h...
…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.
There was a problem hiding this comment.
Changes requested — see inline comments.
1 of 2 prior review threads resolved; 1 still unaddressed as of 53059ea:
1 non-blocking thread still open
- question (non-blocking)
workflows/review/lib/dedup-crossfile.ts:45: Since crossFileMerges records every dropped copy, could a refuted survivor un-merge instead of dropping the group? The h...
| exactKey(a) === exactKey(b) || | ||
| describesSameDefect({...a, line: undefined}, {...b, line: undefined}); | ||
|
|
||
| const mergeableAcrossFiles = (a: Claim, b: Claim): boolean => |
There was a problem hiding this comment.
issue (blocking): A cross-file merge into an author-disputed survivor can soften the verdict, contradicting the module's stated invariant. Introduced by this change. mergeableAcrossFiles compares source, label, path, line, and text — but never author_dispute, and disputes are attached per (path, line) (dispatch.ts:784-802, matching the staged author-disputes entries against each candidate's own anchor), so a byte-identical finding on two sibling files can carry a dispute on exactly one copy. When the disputed copy wins survivorship (its file first in diff order), the undisputed sibling's blocking copy is merged away, and applyVerifications (dispatch-contracts.ts:677-680, checked on the checkout) then caps any non-confirmed disputed survivor to question (non-blocking) — including on the validator-unavailable path, and on a plausible verdict. Without the merge, the sibling copy posts and retains its blocking label (an undisputed, unmentioned claim is retained as-is), so the merge silently drops a blocking finding's force. That directly contradicts this header's guarantee that "a wrong merge can never soften a verdict (the survivor carries the same label every copy did)": label equality holds at merge time, but the dispute cap re-labels the survivor afterward. The path is reachable precisely because blocking candidates are exempted from adjudicated-thread suppression so a disputed re-flag stays visible — that disputed claim is then a live merge survivor. The one-line fix keeps the module's fail-toward-not-merging direction: require equal dispute state, so an asymmetric pair posts as two comments (the accepted duplicate-comment cost) instead of one capped comment.
A sketch, not a committable replacement:
const mergeableAcrossFiles = (a: Claim, b: Claim): boolean =>
a.source === b.source &&
a.label === b.label &&
// An asymmetric dispute must not merge: the dispute cap re-labels a
// non-confirmed disputed survivor, which would soften the verdict the
// undisputed copy carried (the invariant above forbids exactly that).
a.author_dispute === b.author_dispute &&
a.path !== undefined &&
b.path !== undefined &&
a.path !== b.path &&
a.line !== undefined &&
b.line !== undefined &&
sameCrossFileFinding(a, b);
Lower-confidence observations (2)
workflows/review/lib/dedup-crossfile.ts:107suggestion (non-blocking): The near-identical branch serves an unmeasured shape and carries most of the module's complexity — consider shipping exact-identical-only first.workflows/review/lib/dedup-crossfile.ts:7thought (non-blocking): The root cause is upstream: findings carry exactly one anchor, and no reviewer prompt asks a source to report a repeated pattern once.
| "A reader trusts the comment and edits the wrong versions entry.", | ||
| }); | ||
|
|
||
| describe("runDispatch cross-file duplicate merge", () => { |
There was a problem hiding this comment.
note (non-blocking): PR description undercounts the delivered tests (13→17 and 2→4). The description's Tests section enumerates two dispatch scenarios but the file adds two more (validator-corrected-discussion re-apply and the suppression-ordering regression), and lists 13 dedup cases against 17 present. This is over-delivery, not a gap — flagging only so the description count can be refreshed.
| * blocking claim, so it floors the verdict exactly once, and a wrong merge | ||
| * can never soften a verdict (the survivor carries the same label every | ||
| * copy did). | ||
| * - **Different paths, both anchored.** A same-path pair belongs to tier 1; |
There was a problem hiding this comment.
note (non-blocking): "A same-path pair belongs to tier 1" is inaccurate for the same-source pairs this module handles. Tier 1's mergeable requires a.source !== b.source (dedup.ts:618-624, checked on the checkout), so a SAME-source pair on the SAME path — one reviewer stamping identical text twice on one file, or a bridge-chained group whose star guard leaves a same-path member standing beside the survivor — merges in no tier at all and posts as two comments. The docstring here (and the dedup-crossfile.test.ts test name "leaves same-path pairs to tier 1") tells a future maintainer that case is handled elsewhere when it is handled nowhere. Posting duplicates is this module's accepted failure direction, so the behavior is fine; the pointer to tier 1 is what misleads. Consider rewording to say a same-source same-path pair is out of every tier's scope by design.
| @@ -0,0 +1,316 @@ | |||
| /** | |||
There was a problem hiding this comment.
nitpick (non-blocking): Module filename "crossfile" is unhyphenated where the repo hyphenates multi-word suffixes. The repo hyphenates word boundaries in multi-word module names (dedup-pr-level.ts, dispatch-gate-plan.ts, submission-blocking-only.test.ts), and this concept is spelled "cross-file" everywhere in the code itself (via: "cross-file", // Cross-file duplicate merge), but the new files are dedup-crossfile.ts and dispatch-crossfile.test.ts.
A sketch, not a committable replacement:
Rename to `dedup-cross-file.ts` and `dispatch-cross-file.test.ts` to match `dedup-pr-level.ts` and the "cross-file" spelling used throughout the code.
2 participants
What
A new dedup pass merges one source's identical (or near-identical) finding on several files into a single pattern-level comment. Both existing tiers require the same path and different sources by design: they merge agreement between reviewers, not repetition by one reviewer. The pass runs after the cross-source tiers settle (
dedupeClaimsWithCrossFileindedup-crossfile.ts, the entry point dispatch now calls), so it can only remove comments the tiers left standing, never a merge they made. Merged copies skip validation and posting and are recorded in the run artifact undercrossFileMerges.Why
Measured on Khan/webapp#41440, the first post-deploy window for v1.13.0: inline comments 3764122555 and 3764122558 are byte-identical 566-character
suggestion (non-blocking, documentation)bodies on two sibling eval YAML files, posted by one source in one run. In-run clustering (#312) could not reach the pair; every rule it has forbids exactly this shape (same path, different sources), and #336 already had to fix a clusterer example that promised the grouping the path rule forbids.Semantics
describesSameDefectwith the lines stripped, which forces the stricterOTHER_LINE_FLOOR: equal line numbers in different files are coincidence, not anchor evidence, and never buy the laxer exact-anchor tier.files.json; claim order when the staging is absent or does not know a path). Its discussion gains one trailing "Also applies to" line naming the other occurrences (path, and line where known). Those occurrences are prose, not anchors, so the change-provenance gate applies to the survivor's own anchor only; every copy already passed the gate individually before dedup ran.Tests
New
dedup-crossfile.test.ts(13 tests): the 41440 exact-duplicate shape; diff-order survivor election and the claim-order fallback; near-identical above the strict floor merges; a between-floors pair (clears exact-anchor, fails different-line) does not merge on coincidentally equal lines; label, source, same-path, and unanchored exclusions; a blocking group keeps exactly one blocking claim; a three-file group lists both other occurrences; the star guard. Newdispatch-crossfile.test.ts(2 tests): end-to-end collapse with the merged copy skipping validation and the artifact recording the merge, and a differing pair posting both. Fullworkflows/reviewsuite: 1341 tests pass;pnpm lintandpnpm typecheckclean.