diff --git a/.changeset/collapsed-attribution-footers.md b/.changeset/collapsed-attribution-footers.md new file mode 100644 index 00000000..a68c9463 --- /dev/null +++ b/.changeset/collapsed-attribution-footers.md @@ -0,0 +1,26 @@ +--- +"review": minor +--- + +review: name the producing reviewer on every posted finding, in footers collapsed by default + +Every inline review comment and pr-level body fold now ends with a collapsed +`
` 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, with its +differing anchor line and (for clusterer-merged copies) its own subject. +Collapsed one-liners (the low-confidence section, a hold comment's claim list) +carry a short trailing `()` 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 at all. + +The version/config footer on review bodies and the guidance comment is wrapped +in the same collapsed block (`details`/`summary`/`sub` are all on the ingest +sanitizer's allowed-tag list, so the block survives posting). 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. diff --git a/workflows/review/README.md b/workflows/review/README.md index a7c9aba0..b5fbbe9a 100644 --- a/workflows/review/README.md +++ b/workflows/review/README.md @@ -726,12 +726,15 @@ v1.4.0 shipped still pointing at v1.2.2, before the sync existed). Semver is the behavior contract: a release that changes the reviewer's behavior bumps the major version, so a consumer pinned to `review-v` can assume the fundamental behavior holds within a major. For attribution and rollback, every submitted review -body and the risks/patterns guidance comment (Step 7) end with a visible one-line -footer, rendered in code by `lib/version-footer.ts` from the pinned checkout's -`package.json` and the staged run files (never composed by the model): +body and the risks/patterns guidance comment (Step 7) end with a footer collapsed +inside a `
` block (summary chip `review details`), rendered in code by +`lib/version-footer.ts` from the pinned checkout's `package.json` and the staged +run files (never composed by the model): ``` +
review details review-v.. | schema | depth | re-review [blocking-only] | enable +
``` `schema` is the finding-schema version (`FINDING_SCHEMA_VERSION` in @@ -742,11 +745,25 @@ the staging cannot state is omitted rather than guessed. A bad reviewer release rolls back by re-pinning the previous tag; the footer on each posted review makes attribution immediate. -The footer is visible by necessity, not preference: attribution originally rode a -hidden HTML marker (``), but gh-aw's safe-output +The footer posts (collapsed, expandable) by necessity, not preference: +attribution originally rode a hidden HTML marker +(``), but gh-aw's safe-output ingest sanitizer deletes ALL XML/HTML comments (`removeXmlComments` in `sanitize_content_core.cjs`, the same strip documented for the fingerprint stamp -in `lib/rereview-mode.ts`), so the marker never reached a posted comment; `` -is on the sanitizer's allowed-tag list and survives ingest. There is no separate -config-hash or drift-stamp mechanism; the release tag plus the footer's config -segments are the version surface. +in `lib/rereview-mode.ts`), so the marker never reached a posted comment; `sub`, +`details`, and `summary` are on the sanitizer's allowed-tag list and survive +ingest. There is no separate config-hash or drift-stamp mechanism; the release +tag plus the footer's config segments are the version surface. + +Every inline review comment (and each pr-level finding folded into the review +body) additionally ends with a per-comment attribution footer in the same +collapsed block, naming the reviewer that produced the finding and, when +cross-source dedup merged duplicates into it, each other reviewer that flagged +the same defect (`lib/attribution.ts`; the merge record is the structured +`also_flagged_by` field on the claim, so a validator discussion rewrite cannot +drop it). Collapsed one-liners (the low-confidence `
` section and a +hold comment's claim list) carry the short form, a trailing +`()` tag. Text-similarity comparisons against +previously-posted bodies (open-thread suppression, the adjudicated corpus) +strip these footers first, so the shared boilerplate cannot inflate similarity +between unrelated findings. diff --git a/workflows/review/eval/live-dedup.ts b/workflows/review/eval/live-dedup.ts index 46ead928..4f963c91 100644 --- a/workflows/review/eval/live-dedup.ts +++ b/workflows/review/eval/live-dedup.ts @@ -123,14 +123,17 @@ export const dedupeLiveFindings = async ( ) { return live; } - // The survivor's claim carries the "also flagged by" note (the lib - // projection puts the prose in `discussion`) and may have adopted a - // merged copy's suggestion; both must reach the rendered comment. + // The survivor's claim may have adopted a merged copy's + // suggestion, which must reach the rendered comment. The "also + // flagged by" record is structured (`also_flagged_by`) and is + // rendered by production's posting surface (submission.ts's + // collapsed attribution footer), which the eval's `renderComment` + // does not emulate; the merge itself is measured through + // `merges`, so the record adds no report column here. return { ...live, finding: { ...live.finding, - model_authored_prose: survivor.discussion, ...(survivor.suggestion !== undefined ? {suggested_patch: survivor.suggestion} : {}), diff --git a/workflows/review/eval/live-producer.test.ts b/workflows/review/eval/live-producer.test.ts index 903cac44..cf27577f 100644 --- a/workflows/review/eval/live-producer.test.ts +++ b/workflows/review/eval/live-producer.test.ts @@ -746,12 +746,13 @@ describe("produceLive cross-source dedup", () => { ) as string, ), ).toHaveLength(2); - // One finding survives, carrying the attribution note, and the - // validator is dispatched over the merged set only. + // One finding survives (the merge itself is recorded in the dedup + // report; the structured `also_flagged_by` record renders only at + // production's posting surface), and the validator is dispatched + // over the merged set only. expect(result.findings).toHaveLength(1); - expect(result.findings[0].finding.model_authored_prose).toContain( - "Also flagged by:\n- skill: Declaration doc comment doesn't begin " + - "with the symbol name.", + expect(result.findings[0].finding.model_authored_prose).not.toContain( + "Also flagged by", ); expect( JSON.parse( diff --git a/workflows/review/lib/attribution.test.ts b/workflows/review/lib/attribution.test.ts new file mode 100644 index 00000000..69ff00e5 --- /dev/null +++ b/workflows/review/lib/attribution.test.ts @@ -0,0 +1,129 @@ +import {describe, it, expect} from "vitest"; + +import { + renderAttributionFooter, + renderCollapsedFooter, + stripFooters, +} from "./attribution"; + +describe("renderCollapsedFooter", () => { + it("wraps one line in the shared collapsed block", () => { + expect(renderCollapsedFooter("schema 2")).toBe( + "
review details\n" + + "schema 2\n" + + "
", + ); + }); + + it("never emits an HTML comment (the sanitizer would delete it)", () => { + expect(renderCollapsedFooter("x")).not.toContain("$/); - expect(lines.at(-2)).toMatch(/^.*schema \d+.*<\/sub>$/); - expect(lines.at(-2)).not.toContain("`) the orchestrator - * was prompted to compose, but gh-aw's safe-output ingest sanitizer deletes - * ALL XML/HTML comments (`removeXmlComments` in `sanitize_content_core.cjs`; - * the same strip that already killed the fingerprint stamp, see - * rereview-mode.ts), so the marker never reached a single posted comment: - * verified on all 9 guidance comments and 13 review bodies posted to - * Khan/webapp on 2026-08-11/12. `` is on the sanitizer's allowed-tag - * list (GFM-safe tags, sanitize_content_core.cjs v0.83.4), so this footer - * survives ingest byte-for-byte; rendering it in code (from package.json and - * the staged run files, never from the model's memory) keeps the attribution - * trustworthy for rollback decisions. + * Why posted-and-visible-on-expand, and why code-rendered: the original + * attribution surface was a hidden HTML marker + * (``) the orchestrator was prompted to + * compose, but gh-aw's safe-output ingest sanitizer deletes ALL XML/HTML + * comments (`removeXmlComments` in `sanitize_content_core.cjs`; the same + * strip that already killed the fingerprint stamp, see rereview-mode.ts), + * so the marker never reached a single posted comment: verified on all 9 + * guidance comments and 13 review bodies posted to Khan/webapp on + * 2026-08-11/12. `sub`, `details`, and `summary` are all on the sanitizer's + * allowed-tag list (GFM-safe tags, sanitize_content_core.cjs v0.83.4), so + * this footer survives ingest byte-for-byte; rendering it in code (from + * package.json and the staged run files, never from the model's memory) + * keeps the attribution trustworthy for rollback decisions. * * The hidden version marker instruction is retired from review.md Step 7; * the fingerprint stamp emission stays (rereview-mode.ts explains why). */ +import {renderCollapsedFooter} from "./attribution"; import {FINDING_SCHEMA_VERSION} from "./finding-schema"; const REVIEW_DIR = "/tmp/gh-aw/review"; @@ -51,10 +56,10 @@ export type VersionFooterInputs = { }; /** - * Render the footer line. Pure; every segment that cannot be stated is - * omitted rather than guessed, so a degraded staging yields a shorter - * footer, never a wrong one. Contains no HTML comment by construction (the - * sanitizer would delete one). + * Render the collapsed footer block. Pure; every segment that cannot be + * stated is omitted rather than guessed, so a degraded staging yields a + * shorter footer, never a wrong one. Contains no HTML comment by + * construction (the sanitizer would delete one). */ export const renderVersionFooter = (inputs: VersionFooterInputs): string => { const segments: string[] = []; @@ -75,7 +80,7 @@ export const renderVersionFooter = (inputs: VersionFooterInputs): string => { if (inputs.enabledReviewers.length > 0) { segments.push(`enable ${inputs.enabledReviewers.join(",")}`); } - return `${segments.join(" | ")}`; + return renderCollapsedFooter(segments.join(" | ")); }; const readJson = (fs: VersionFooterFs, path: string): unknown => { @@ -91,7 +96,7 @@ const readJson = (fs: VersionFooterFs, path: string): unknown => { /** * Compose the footer from the staged run files and stage it at - * {@link FOOTER_OUT} (review.md Step 7 pastes the staged line verbatim into + * {@link FOOTER_OUT} (review.md Step 7 pastes the staged block verbatim into * the guidance comment; the submission CLI appends the returned string to * the review body). Reads: * diff --git a/workflows/review/review.md b/workflows/review/review.md index 7f867cfb..ea4d57a5 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -649,9 +649,11 @@ cd gh-aw-review-lib && npx -y tsx workflows/review/lib/submission.ts ``` It reads `dispatch-result.json`, renders the accountability section (`rereview.json`), computes the verdict (Step 4's mechanical rule plus the - reduced-depth flip floor), renders every inline comment and the full - review body (note lines, the visible attribution footer, and the - fingerprint stamp included), and writes + reduced-depth flip floor), renders every inline comment (each with its + collapsed per-comment attribution footer naming the producing reviewer + and any merged duplicates) and the full review body (note lines, the + collapsed version/config footer, and the fingerprint stamp included), + and writes `/tmp/gh-aw/review/submission-plan.json`. At full depth it also stages `/tmp/gh-aw/review/risks-patterns-key.txt`, the code-computed canonical signature Step 7 compares (never compose your own signature in @@ -722,7 +724,7 @@ never add, drop, reword, or re-anchor one. The review body and event are composed by the plan CLI (Step 3): the verdict head, the code-rendered re-review accountability section, every `Note:` line, -the visible attribution footer, and the hidden fingerprint stamp are all +the collapsed version/config footer, and the hidden fingerprint stamp are all already in the plan's `body`. Submit with **one** `submit-pull-request-review` call carrying the plan's `event` and `body` verbatim — except under the redundant-approval skip (Step 3), where you @@ -808,8 +810,8 @@ should only ever be one current risks/patterns comment: Begin the comment with the exact marker line below (so the comment is identifiable on later runs), then include the Guidance for reviewers team sections and/or the common-patterns section. Omit whichever is empty. End the comment with the -attribution footer: paste the one-line `...` footer from -`/tmp/gh-aw/review/version-footer.txt` **verbatim** as the final line (the plan +version/config footer: paste the collapsed `
` footer block from +`/tmp/gh-aw/review/version-footer.txt` **verbatim** as the final lines (the plan CLI staged it in Step 3; if the file is missing, re-stage it with `cd gh-aw-review-lib && npx -y tsx workflows/review/lib/version-footer.ts`). Never compose the footer yourself, and never use an HTML comment for it: the @@ -869,7 +871,9 @@ fully explained by a common pattern above:
+
review details review-v1.14.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness +
```` - Title the comment `## Guidance for reviewers`, follow it with the one-line @@ -1675,6 +1679,10 @@ Read from disk: producer's concrete failing scenario: specific inputs/state, then the wrong outcome), `confidence`, an optional `suggestion`, when the claim asserts a best-practice skill breach its `skill` name, + when cross-source dedup merged duplicate copies into it an `also_flagged_by` + list naming each other reviewer (with its anchor line where it differed, and + a clusterer-merged copy's own subject; treat those as corroboration to weigh, + never as extra claims to validate), and — when the claim re-raises a point the PR author has factually disputed in an existing review thread — an `author_dispute` quote of the author's grounds. - The diff: `/tmp/gh-aw/review/pr.diff`.