From 1f41a44879912310dd405eca27aea18c6c575d6a Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:51:31 -0400 Subject: [PATCH] fix(review): title the pr-review-cli completion notice and count demoted CRITICAL findings The completion notice now carries the annotation title claude-lane-review-complete on every path, as pr-review.yml and pr-review-security.yml emit it, so a merge gate that filters on the title finds the record. When one bad anchor moves the whole inline batch to the summary, the summary's critical-findings line now counts the moved CRITICAL findings, and one is added when the model left it out. Co-Authored-By: Claude Opus 5.5 --- .github/actions/publish-review/publish.cjs | 40 ++++++++- .github/scripts/publish-review.test.cjs | 98 +++++++++++++++++++++- .github/workflows/pr-review-cli.yml | 6 +- 3 files changed, 137 insertions(+), 7 deletions(-) diff --git a/.github/actions/publish-review/publish.cjs b/.github/actions/publish-review/publish.cjs index 3e8b59cc..a2095e5e 100644 --- a/.github/actions/publish-review/publish.cjs +++ b/.github/actions/publish-review/publish.cjs @@ -27,6 +27,12 @@ const CLASSES = new Set([ // The workflow compares publish with `!=`, which GitHub evaluates ignoring case. const isComments = (env) => (env.PUBLISH ?? "").toLowerCase() === "comments"; +// The completion notice's annotation title, which a merge gate filters on. +const RECORD = { title: "claude-lane-review-complete" }; +// The severity and summary-count lines a merge gate reads, in its own shapes. +const SEVERITY = /^\s*`?severity: (CRITICAL|IMPORTANT|SUGGESTION)`?\s*$/gmu; +const CRITICAL_LINE = /^(\s*`?critical-findings: )(\d{1,6})(`?\s*)$/gmu; + // The head-side (RIGHT) and base-side (LEFT) line numbers a review comment // can sit on in one file's unified-diff patch. function commentableLines(patch) { @@ -157,6 +163,32 @@ function placeComments(review, files, lane) { : { inline: [], unanchored: prepared, problems }; } +// Whether a finding counts as CRITICAL the way a merge gate reads a thread: +// anything but exactly one IMPORTANT or SUGGESTION severity line. +function isCritical(body) { + const severities = [...body.matchAll(SEVERITY)].map((m) => m[1]); + return severities.length !== 1 || severities[0] === "CRITICAL"; +} + +// The model's summary, its one critical-findings line raised by the CRITICAL +// findings moved out of the inline batch, which that line leaves out. With no +// line, one is added that counts every summary finding as CRITICAL; with more +// than one, a gate already counts them all, so the text stays as written. +function countedSummary(review, unanchored) { + const text = stripHtml(review.summary); + if (unanchored.length === 0) return text; + const demoted = unanchored.filter((c) => isCritical(c.body)).length; + const lines = [...text.matchAll(CRITICAL_LINE)].length; + if (lines === 1) + return text.replace( + CRITICAL_LINE, + (_, before, count, after) => `${before}${Number(count) + demoted}${after}`, + ); + if (lines > 1) return text; + const rest = Math.max(review.findings - review.comments.length, 0); + return `${text}\n\ncritical-findings: ${rest + demoted}`; +} + // The summary as posted: the model's summary, then any finding that could not // sit on a line, with its location. function summaryText(review, unanchored, lane) { @@ -165,7 +197,7 @@ function summaryText(review, unanchored, lane) { `- \`${comment.path}:${comment.line}\` (${comment.side}): ${comment.body.replace(`\n\nclaude-lane: ${lane}`, "")}`, ); return [ - stripHtml(review.summary), + countedSummary(review, unanchored), ...(extra.length > 0 ? [ "", @@ -320,8 +352,9 @@ async function publish({ github, context, core, env }) { stripHtml(env.LAST_SUMMARY).slice(0, MAX_SUMMARY), ); if (env.SKIP === "no-change") - core.notice(`${record} skip=no-change through=${env.SINCE}`); - else if (env.SKIP === "docs-only") core.notice(`${record} skip=docs-only`); + core.notice(`${record} skip=no-change through=${env.SINCE}`, RECORD); + else if (env.SKIP === "docs-only") + core.notice(`${record} skip=docs-only`, RECORD); return { skipped: true }; } @@ -438,6 +471,7 @@ async function publish({ github, context, core, env }) { const account = verdict.leg === "SUB" ? " account=fallback" : ""; core.notice( `${record} findings=${findings} inline=${inline} ${scope}${account}`, + RECORD, ); return { findings, inline }; } diff --git a/.github/scripts/publish-review.test.cjs b/.github/scripts/publish-review.test.cjs index 1b0ab4c9..ad2d7b54 100644 --- a/.github/scripts/publish-review.test.cjs +++ b/.github/scripts/publish-review.test.cjs @@ -143,7 +143,13 @@ function fakeGithub({ head = HEAD, posted = [], throws = false } = {}) { } function fakeCore() { - const log = { failed: [], notices: [], warnings: [], summary: "" }; + const log = { + failed: [], + notices: [], + titles: [], + warnings: [], + summary: "", + }; const summary = { addRaw: (text) => { log.summary += text; @@ -155,7 +161,10 @@ function fakeCore() { log, core: { setFailed: (m) => log.failed.push(m), - notice: (m) => log.notices.push(m), + notice: (m, properties) => { + log.notices.push(m); + log.titles.push(properties?.title); + }, warning: (m) => log.warnings.push(m), info: () => {}, summary, @@ -476,3 +485,88 @@ test("summary mode writes a scanned shadow report with the review and the leg's assert.equal(JSON.parse(withheld).failure_class, "withheld"); assert.doesNotMatch(withheld, /sk-ant|Off by one/u); }); + +test("every completion notice carries the title a merge gate filters on", async () => { + const runs = [ + await publish({}), + await publish({ KEY_REVIEW: JSON.stringify({ ...REVIEW, comments: [] }) }), + await publish({ + SCOPE_REVIEW: "false", + SKIP: "no-change", + SINCE: "c".repeat(40), + SKIP_REASON: "nothing changed", + }), + await publish({ + SCOPE_REVIEW: "false", + SKIP: "docs-only", + SKIP_REASON: "docs only", + }), + ]; + for (const { log } of runs) { + const records = log.notices + .map((notice, i) => [notice, log.titles[i]]) + .filter(([notice]) => notice.startsWith("claude-lane-review-complete")); + assert.equal(records.length, 1, JSON.stringify(log.notices)); + assert.equal(records[0][1], "claude-lane-review-complete"); + } +}); + +// The marker body a demoted batch posts, for a review whose comments carry +// the given bodies and whose summary is `summary`. +async function demotedMarker(summary, bodies, findings = bodies.length) { + const review = { + findings, + summary, + comments: bodies.map((body, i) => ({ + path: "a.js", + line: 9 + i, + side: "RIGHT", + body, + })), + }; + const { writes } = await publish({ KEY_REVIEW: JSON.stringify(review) }); + assert.deepEqual( + writes.map(([name]) => name), + ["createComment"], + ); + return writes[0][1].body; +} + +const criticalLines = (body) => + [...body.matchAll(/^\s*`?critical-findings: (\d{1,6})`?\s*$/gmu)].map((m) => + Number(m[1]), + ); + +test("a demoted CRITICAL batch raises the summary's critical-findings line in place", async () => { + const body = await demotedMarker("Summary.\n\ncritical-findings: 0", [ + "Injection.\n\nseverity: CRITICAL", + "Naming.\n\nseverity: SUGGESTION", + "Unmarked.", + ]); + assert.deepEqual(criticalLines(body), [2]); +}); + +test("a demoted CRITICAL batch adds a critical-findings line the model left out", async () => { + const body = await demotedMarker( + "Summary.", + ["Injection.\n\nseverity: CRITICAL", "Naming.\n\nseverity: IMPORTANT"], + 3, + ); + assert.deepEqual(criticalLines(body), [2]); +}); + +test("an inline batch and a summary with two count lines keep the model's text", async () => { + const inline = await publish({ + KEY_REVIEW: JSON.stringify({ + ...REVIEW, + summary: "Summary.\n\ncritical-findings: 0", + }), + }); + const marker = inline.writes.find(([name]) => name === "createComment"); + assert.deepEqual(criticalLines(marker[1].body), [0]); + const twice = await demotedMarker( + "critical-findings: 0\ncritical-findings: 1", + ["Injection.\n\nseverity: CRITICAL"], + ); + assert.deepEqual(criticalLines(twice), [0, 1]); +}); diff --git a/.github/workflows/pr-review-cli.yml b/.github/workflows/pr-review-cli.yml index 2ab9e8a1..da0d7aac 100644 --- a/.github/workflows/pr-review-cli.yml +++ b/.github/workflows/pr-review-cli.yml @@ -92,8 +92,10 @@ name: pr-review-cli # PUBLISH: `comments` posts one COMMENT review of inline comments, keeps the # lane's marker comment (head, base and summary) and leaves the # `claude-lane-review-complete lane= sha= findings= inline= -# scope=<...>` notice, ending ` account=fallback` when the subscription leg -# reviewed. Its status job is named `claude-review-status` (code) or +# scope=<...>` notice (title `claude-lane-review-complete`), ending +# ` account=fallback` when the subscription leg reviewed. When one bad anchor +# moves every comment to the summary, the summary's `critical-findings:` line +# counts the moved CRITICAL findings too. Its status job is named `claude-review-status` (code) or # `security-review` (security) and goes red, naming the class, when no # review happened. Any other value is summary mode: no comment, no review, # no marker and no notice; the status job writes a job summary and uploads