review: name the producing reviewer on every posted finding, in collapsed footers - #350
Conversation
…r on every posted finding, in footers collapsed by default Every inline comment and pr-level body fold ends with a collapsed <details> attribution footer (summary chip 'review details') naming the reviewer that produced the finding and, when cross-source dedup merged duplicates into it, each other reviewer that flagged the same defect (differing anchor line, and a clusterer-merged copy's own subject). Collapsed one-liners (the low-confidence section, a hold comment's claim list) carry a short trailing <sub>(<source>)</sub> tag instead. The dedup merge record moves from a prose note appended to the survivor's discussion to a structured also_flagged_by field on the claim, rendered at the posting surface (submission.ts): the claim-validator's corrected.discussion rewrite could previously drop the note silently, and the old 'Also flagged by' wording never named the surviving reviewer. The version/config footer on review bodies and the guidance comment wraps in the same collapsed block (details/summary/sub are all sanitizer-allowed tags, so the block survives ingest). Text-similarity comparisons against previously posted bodies (open-thread suppression, the adjudicated corpus) strip the footers first, so boilerplate shared by every bot comment cannot inflate similarity between unrelated findings.
🦋 Changeset detectedLatest commit: e73f5c2 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 Guidancegithub-actions (7 files)
Common patterns4 files: Test assertions migrated from - expect(claims[0].discussion).toContain(
- "Also flagged by completeness, first-principles, skill-auditor (out-of-lane).",
- );
+ expect(claims[0].also_flagged_by).toEqual([
+ {source: "completeness"},
+ {source: "first-principles"},
+ {source: "skill-auditor (out-of-lane)"},
+ ]);2 files: Version-footer rendering/assertions wrapped in - return `<sub>${segments.join(" | ")}</sub>`;
+ return renderCollapsedFooter(segments.join(" | "));Excluded from review (5 files)Not individually reviewed — generated, formatting-only, or
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| */ | ||
| const threadProse = (body: string): string => | ||
| body | ||
| stripFooters(body) |
There was a problem hiding this comment.
thought (non-blocking): Moving the merge note out of discussion makes open-thread suppression asymmetric against old posted comments. openThreadScore tokenizes the new claim from claim.subject+discussion+failure_scenario (no longer containing the note) but the old body from threadProse(thread.body), and stripFooters only removes the new <details>/<sub> footer shapes — it cannot remove the pre-existing markdown Also flagged by ... note, so its tokens count only on the old side. This is a transitional edge affecting merged findings only and self-heals as old comments age out, but the PR description's claim that "suppression against old threads is unchanged" is not strictly true for that subset.
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus 5ca2d116455d (10 cases).
Improvements (candidate caught, baseline missed)
Adversarial hard gate: PASSED on the candidate arm. Agent failures
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 |
… in attribution footers; make the bare-sub strip targeted Review feedback on #350: - A merged copy's model-authored subject is now HTML-escaped before interpolation, so a literal </details> cannot close the collapsed block early or truncate stripFooters. - stripFooters no longer blanket-deletes every <sub> span: it removes only the two posted shapes (whole-line version-footer spans and end-of-line parenthesized source tags), so prose quoting <sub> survives similarity comparison. - Pin the pr-level fold attribution footer and both hold-comment source tags in tests.
|
Addressed the review feedback in e73f5c2:
|
There was a problem hiding this comment.
Approved — no blocking issues found.
4 of 5 prior review threads resolved; 1 still unaddressed as of e73f5c2:
1 non-blocking thread still open
- thought (non-blocking)
workflows/review/lib/dedup.ts:304: Moving the merge note out ofdiscussionmakes open-thread suppression asymmetric against old posted comments. `openThr...
2 participants
Summary
Every posted finding now names the reviewer that produced it, in a footer collapsed by default.
<details>attribution footer (summary chipreview details) naming the producing reviewer and, when cross-source dedup merged duplicates into it, each other reviewer that flagged the same defect, with its differing anchor line; a copy merged by the claim-clusterer (the sub-agent that groups duplicate findings across reviewers) also carries its own subject. Collapsed one-liners (the low-confidence section, a hold comment's claim list) carry a short trailing<sub>(source)</sub>tag instead.discussionto a structuredalso_flagged_byfield on the claim, rendered at the posting surface (submission.ts). Two defects this closes: the claim-validator'scorrected.discussionrewrite could silently drop the note, and the old "Also flagged by" wording never named the surviving reviewer.details,summary, andsubare all on the ingest sanitizer's allowed-tag list, so both footers survive posting.stripFootersin the newlib/attribution.ts), so boilerplate shared by every bot comment cannot inflate similarity between unrelated findings.Notes
renderClaimCommentstays byte-identical torenderCommenton the same finding; the footer is appended at the plan surface.renderCommentdoes not emulate the footer; merges are measured through themergesartifact (seam documented ineval/live-dedup.ts).reviewminor); the new footers reach PRs after the next release tag and consumer re-pin.Testing
lib/attribution.test.ts; updated dedup/dispatch/submission/version-footer tests.