Skip to content

review: name the producing reviewer on every posted finding, in collapsed footers #350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jwbron merged 2 commits into from
Aug 18, 2026
Merged

review: name the producing reviewer on every posted finding, in collapsed footers #350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51d6ede
[jwies/review-attribution-footers] review: name the producing reviewe…
jwbron Aug 13, 2026
e73f5c2
[jwies/review-attribution-footers] review: escape merged-subject HTML…
jwbron Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions .changeset/collapsed-attribution-footers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
`<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, 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 `<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 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.
35 changes: 26 additions & 9 deletions workflows/review/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<major>` 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 `<details>` 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):

```
<details><summary><sub>review details</sub></summary>
<sub>review-v<major>.<minor>.<patch> | schema <n> | depth <depth> | re-review <mode> [blocking-only] | enable <reviewer,...></sub>
</details>
```

`schema` is the finding-schema version (`FINDING_SCHEMA_VERSION` in
Expand All @@ -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 (`<!-- pr-reviewer:version ... -->`), but gh-aw's safe-output
The footer posts (collapsed, expandable) by necessity, not preference:
attribution originally rode a hidden HTML marker
(`<!-- pr-reviewer:version ... -->`), 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; `<sub>`
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 `<details>` section and a
hold comment's claim list) carry the short form, a trailing
`<sub>(<source>)</sub>` 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.
11 changes: 7 additions & 4 deletions workflows/review/eval/live-dedup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
: {}),
Expand Down
11 changes: 6 additions & 5 deletions workflows/review/eval/live-producer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
129 changes: 129 additions & 0 deletions workflows/review/lib/attribution.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,129 @@
import {describe, it, expect} from "vitest";

import {
renderAttributionFooter,
renderCollapsedFooter,
stripFooters,
} from "./attribution";

describe("renderCollapsedFooter", () => {
it("wraps one <sub> line in the shared collapsed block", () => {
expect(renderCollapsedFooter("schema 2")).toBe(
"<details><summary><sub>review details</sub></summary>\n" +
"<sub>schema 2</sub>\n" +
"</details>",
);
});

it("never emits an HTML comment (the sanitizer would delete it)", () => {
expect(renderCollapsedFooter("x")).not.toContain("<!--");
});
});

describe("renderAttributionFooter", () => {
it("names the producing reviewer alone when nothing merged", () => {
expect(renderAttributionFooter("correctness-reviewer")).toBe(
renderCollapsedFooter("found by correctness-reviewer"),
);
});

it("appends the merged copies with their differing anchors", () => {
expect(
renderAttributionFooter("correctness-reviewer", [
{source: "completeness"},
{source: "skill-auditor (out-of-lane)", line: 58},
]),
).toBe(
renderCollapsedFooter(
"found by correctness-reviewer | also flagged by " +
"completeness; skill-auditor (out-of-lane) (at line 58)",
),
);
});

it("quotes a tier-2 copy's own subject (the ask the survivor may not restate)", () => {
expect(
renderAttributionFooter("correctness-reviewer", [
{
source: "conventions",
subject: "Doc comment doesn't begin with the symbol name.",
},
]),
).toBe(
renderCollapsedFooter(
"found by correctness-reviewer | also flagged by " +
"conventions: Doc comment doesn't begin with the symbol name.",
),
);
});

it("escapes HTML in a merged copy's model-authored subject", () => {
const footer = renderAttributionFooter("correctness-reviewer", [
{
source: "conventions",
subject: "Unbalanced </details> & a <sub> tag.",
},
]);
expect(footer).not.toContain("Unbalanced </details>");
expect(footer).toContain(
"Unbalanced &lt;/details&gt; &amp; a &lt;sub&gt; tag.",
);
// The block still strips cleanly: the subject cannot close it early.
expect(stripFooters(`prose\n${footer}`)).toBe("prose\n");
});
});

describe("stripFooters", () => {
it("removes the collapsed footer block from a posted body", () => {
const body = `**issue (blocking):** The guard was removed.\n\n${renderAttributionFooter(
"correctness-reviewer",
[{source: "completeness"}],
)}`;
const stripped = stripFooters(body);
expect(stripped).not.toContain("found by");
expect(stripped).not.toContain("review details");
expect(stripped).toContain("The guard was removed.");
});

it("tolerates round-tripped whitespace inside the block", () => {
const body =
"prose\n<details>\n <summary> <sub>review details</sub> </summary>\n<sub>found by x</sub>\n</details>";
expect(stripFooters(body)).not.toContain("found by");
});

it("removes bare <sub> spans (version-footer lines, source tags)", () => {
expect(
stripFooters(
"- `a.ts:2` issue (blocking): s <sub>(correctness-reviewer)</sub>",
),
).toBe("- `a.ts:2` issue (blocking): s ");
expect(stripFooters("<sub>review-v1.13.0 | schema 2</sub>")).toBe("");
});

it("keeps a quoted <sub> span mid-prose (review content, not a footer)", () => {
const body =
"The strip targets <sub>spans</sub> quoted inside a sentence.";
expect(stripFooters(body)).toBe(body);
const backticked =
"the footer renders `<sub>content</sub>` inside the block, then prose";
expect(stripFooters(backticked)).toBe(backticked);
});

it("leaves other <details> blocks alone", () => {
const section =
"<details>\n<summary>Lower-confidence observations (2)</summary>\n\n- a\n\n</details>";
expect(stripFooters(section)).toBe(section);
});

it("removes every footer when a body carries more than one", () => {
const body = [
renderAttributionFooter("a"),
"prose",
renderCollapsedFooter("review-v1 | schema 2"),
].join("\n");
const stripped = stripFooters(body);
expect(stripped).not.toContain("found by");
expect(stripped).not.toContain("schema 2");
expect(stripped).toContain("prose");
});
});
124 changes: 124 additions & 0 deletions workflows/review/lib/attribution.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
/**
* The collapsed footer surfaces: one shared `<details>` wrapper for the
* run-level version/config footer (version-footer.ts) and the per-comment
* reviewer attribution, plus the strip that keeps footer boilerplate out of
* text-similarity comparisons.
*
* Why collapsed: both footers are metadata, not review content. Rendered
* open they add a visible line to every posted surface; wrapped in
* `<details>` they collapse to one small summary chip the author can expand.
* `details`, `summary`, and `sub` are all on gh-aw's ingest-sanitizer
* allowed-tag list (`SANITIZER_ALLOWED_TAGS`, sanitizer-normalize.ts mirrors
* sanitize_content_core.cjs v0.83.4), so the block survives ingest
* verbatim; that is the same property that made version-footer.ts pick
* `<sub>` over the hidden HTML marker the sanitizer deletes.
*
* Why attribution is rendered HERE (code, at the posting surface) and not
* carried in a claim's `discussion`: dedup runs before the claim-validator,
* whose `corrected.discussion` rewrite replaces the prose wholesale, so a
* merge note appended to `discussion` can be silently dropped. The
* structured `also_flagged_by` field on the claim survives every rewrite
* (applyVerifications never touches it), and this module turns it into text
* only when the comment is composed. Determinism boundary: everything here
* is code-owned wrapping around code-recorded facts (source names, line
* numbers) plus a merged copy's `subject` quoted verbatim; no prose about
* the code under review is synthesised.
*/

/**
* One duplicate copy dedup folded into a surviving claim: the reviewer that
* produced it, its own anchor line when it differs from the survivor's, and
* (for a tier-2, clusterer-merged copy only) its subject, verbatim, because
* the survivor's prose is not known to restate that ask (see dedup.ts).
*/
export type AlsoFlagged = {
source: string;
line?: number;
subject?: string;
};

/** The code-owned summary chip both collapsed footers render under. */
export const FOOTER_SUMMARY = "review details";

/**
* Wrap one `<sub>` content line in the shared collapsed `<details>` block.
* The shape is fixed so {@link stripFooters} can remove it mechanically.
*/
export const renderCollapsedFooter = (content: string): string =>
[
`<details><summary><sub>${FOOTER_SUMMARY}</sub></summary>`,
`<sub>${content}</sub>`,
"</details>",
].join("\n");

/**
* A merged copy's `subject` is model-authored text interpolated into the
* footer's HTML. Unescaped, a subject containing a literal `</details>`
* closes the collapsed block early and truncates {@link stripFooters}'s
* non-greedy match. Escape the HTML-significant characters; GitHub renders
* the entities back as the literal characters.
*/
const escapeHtml = (text: string): string =>
text.replace(/&/g, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;");

const flaggedBy = (entry: AlsoFlagged): string => {
const anchor =
entry.line === undefined
? entry.source
: `${entry.source} (at line ${entry.line})`;
return entry.subject === undefined
? anchor
: `${anchor}: ${escapeHtml(entry.subject)}`;
};

/**
* The per-comment attribution footer: which reviewer produced the finding,
* and (when cross-source dedup merged duplicates into it) which other
* reviewers flagged the same defect, each with its differing anchor line and
* (tier-2 copies) its own ask. Segments join with ` | ` like the version
* footer; merged entries join with `; ` because a quoted subject can carry
* commas.
*/
export const renderAttributionFooter = (
source: string,
alsoFlaggedBy: readonly AlsoFlagged[] = [],
): string => {
const segments = [`found by ${source}`];
if (alsoFlaggedBy.length > 0) {
segments.push(
`also flagged by ${alsoFlaggedBy.map(flaggedBy).join("; ")}`,
);
}
return renderCollapsedFooter(segments.join(" | "));
};

/**
* The collapsed-footer block, tolerant of the whitespace GitHub round-trips
* may introduce; non-greedy so it stops at the block's own `</details>` (a
* footer never nests another details block).
*/
const FOOTER_BLOCK_RE = new RegExp(
`<details>\\s*<summary>\\s*<sub>${FOOTER_SUMMARY}</sub>\\s*</summary>[\\s\\S]*?</details>`,
"gi",
);

/**
* Drop footer boilerplate from a previously-posted bot comment before
* text-similarity comparison (dedup.ts's `threadProse`). Every posted
* comment carries the same summary chip, `found by <source>` prefix, and
* version segments; tokens shared by ALL bot comments would inflate
* similarity between unrelated findings, exactly like the label template
* and rule-quote lines `threadProse` already strips. Removes the collapsed
* block and the two residual bare `<sub>…</sub>` shapes this codebase has
* posted: a whole-line span (the pre-collapse version-footer line) and a
* parenthesized span at end of line (the source tag on collapsed
* one-liners). Deliberately NOT a blanket `<sub>…</sub>` strip: a posted
* comment whose own discussion quotes a `<sub>` span mid-prose (even in
* backticks) is review content, and deleting it would distort the
* similarity text.
*/
export const stripFooters = (body: string): string =>
body
.replace(FOOTER_BLOCK_RE, "")
.replace(/^[ \t]*<sub>[^<]*<\/sub>[ \t]*$/gm, "")
.replace(/<sub>\([^<]*\)<\/sub>[ \t]*$/gm, "");
Loading
Loading