diff --git a/.changes/review/feat-7777-review-lane-convergence-3f9c2a71.md b/.changes/review/feat-7777-review-lane-convergence-3f9c2a71.md new file mode 100644 index 00000000000..26cc3f2924c --- /dev/null +++ b/.changes/review/feat-7777-review-lane-convergence-3f9c2a71.md @@ -0,0 +1,11 @@ +--- +bump: minor +--- + +### Added + +- `lib/prior-findings.mjs` lists a CI review lane's own earlier findings on a pull request for its wrapper to hand the model: inline review threads, reviews and PR comments, kept only when the github-actions bot wrote them (by numeric id), no other account edited them, and they carry the lane's `claude-lane:` footer or reviewed-head marker. For each anchored finding on a thread still open it records whether `git diff HEAD` touches the lines it names (a resolved thread always answers yes), and it counts the lane's rounds (the earlier heads it posted a finding on, by thread, review or reviewed-head summary) against `loop.review-rounds` read from a default-branch `pr-pipeline.yaml` ([#7777](https://github.com/melodic-software/claude-code-plugins/issues/7777)). + +### Changed + +- `/review:code-review` and `/review:security-review` read a `PRIOR FINDINGS:` file when the wrapper names one: a finding the lane already posted on an open thread whose lines are unchanged since is not posted again, at any severity, and gets one `already posted` summary line that counts as the posted finding would (findings count, `critical-findings:`, `hold`), so a review that still finds it never reads clean and never closes the earlier thread once an agent resolves it. Past the round cap the code lane posts only blocking findings and the security lane only CRITICAL and IMPORTANT ones. With no such file both lanes review as before ([#7777](https://github.com/melodic-software/claude-code-plugins/issues/7777)). diff --git a/plugins/review/lib/prior-findings.mjs b/plugins/review/lib/prior-findings.mjs new file mode 100755 index 00000000000..4b7ae35c48d --- /dev/null +++ b/plugins/review/lib/prior-findings.mjs @@ -0,0 +1,284 @@ +#!/usr/bin/env node +// A CI review lane's prior findings on one pull request, for the lane's +// wrapper to run in a step with no model before the review. It lists only the +// lane's own earlier posts (inline review threads, reviews and PR comments) +// with, for each anchored one, whether the lines it names changed between the +// head it was posted on and this head, and counts the lane's rounds against +// the repo-owned cap. The model judges which candidate is the same finding +// (/review:code-review and /review:security-review, "Prior findings"). +// +// A post is the lane's own only when GitHub reports its author as the +// github-actions bot by numeric id, no other account edited it, and its last +// non-blank line is the lane's `claude-lane: ` footer, bare or in +// backticks (or its first line is the lane's reviewed-head marker). Text alone never makes a post the +// lane's: a lookalike from any other account is not listed, so it cannot +// suppress a finding. +// +// CLI env: REPOSITORY (owner/name), PR_NUMBER, HEAD_SHA, LANE (claude-review +// or claude-security-review), OUTPUT_PATH, GITHUB_TOKEN, optional +// GITHUB_API_URL, CAP_CONFIG (a pr-pipeline.yaml read from the default +// branch, never the head) and GITHUB_OUTPUT (gets `note=`). +// Diffs run with git in the current directory, a checkout holding HEAD_SHA; +// a missing commit is fetched with GITHUB_TOKEN in the environment, never argv. +// Any failure leaves no file and exits 1: with no file the lane reviews as if +// nothing were prior. +import { execFileSync } from "node:child_process"; +import { mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from "node:fs"; +import { dirname } from "node:path"; +import { pathToFileURL } from "node:url"; + +const SHA = /^[0-9a-f]{40}$/u; +const LANES = new Set(["claude-review", "claude-security-review"]); +// github-actions[bot]: ci-workflows pr-review-cli.yml's status job posts with +// the job token. GraphQL reports the login without the "[bot]" suffix. +export const ACTIONS_BOT_ID = 41898282; +const MAX_BODY = 4000; + +const ACTOR = "__typename login ... on Bot { databaseId }"; +const QUERY = `query($owner: String!, $name: String!, $number: Int!) { + repository(owner: $owner, name: $name) { + pullRequest(number: $number) { + reviewThreads(first: 100) { + pageInfo { hasNextPage } + nodes { + isResolved path originalLine originalStartLine diffSide + comments(first: 1) { + nodes { url body lastEditedAt originalCommit { oid } author { ${ACTOR} } editor { ${ACTOR} } } + } + } + } + reviews(first: 100) { + pageInfo { hasNextPage } + nodes { url body lastEditedAt commit { oid } author { ${ACTOR} } editor { ${ACTOR} } } + } + comments(first: 100) { + pageInfo { hasNextPage } + nodes { url body lastEditedAt author { ${ACTOR} } editor { ${ACTOR} } } + } + } + } +}`; + +const isBot = (actor) => + actor?.__typename === "Bot" && actor.databaseId === ACTIONS_BOT_ID && actor.login === "github-actions"; + +// The lane's footer line, bare or in backticks, as clear-escalations.mjs +// LANE_TAG and babysit_review_evidence.py LANE_TAG_RE read it. +const footer = (lane) => new RegExp(`^\`?claude-lane: ${lane}\`?$`, "u"); +const marker = (lane) => new RegExp(`^$`, "u"); +const nonBlank = (body) => + body + .split(/\r?\n/u) + .map((l) => l.trim()) + .filter(Boolean); + +// Whether `post` is `lane`'s own: the bot wrote it, nobody else edited it, and +// it carries the lane's footer as its last non-blank line or the lane's +// reviewed-head marker as its first line. +export function isLanePost(post, lane) { + if (!LANES.has(lane) || !isBot(post?.author) || typeof post.body !== "string") return false; + if (post.lastEditedAt != null && !isBot(post.editor)) return false; + const lines = nonBlank(post.body); + return footer(lane).test(lines.at(-1) ?? "") || marker(lane).test(lines[0] ?? ""); +} + +// A reviewed-head comment's head and the findings its status line counts, as +// ci-workflows publish-review/publish.cjs writes them: the marker, an optional +// reviewed-base line, then `**** reviewed ``: read N of M changed +// file(s); K finding(s), ...`. A skip's "needs no new review" line counts none. +function reviewedHead(body, lane) { + const lines = nonBlank(body); + const sha = marker(lane).exec(lines[0] ?? "")?.[1]; + if (!sha) return null; + const status = lines[1]?.startsWith("\nsummary`; + assert.equal(isLanePost({ author: BOT, body: marker, lastEditedAt: null }, LANE), true); + assert.equal(isLanePost({ author: PERSON, body: marker, lastEditedAt: null }, LANE), false); + }); +}); + +describe("lines changed since the finding was posted", () => { + test("a lane-authored prior finding on unchanged lines is suppressible: lines changed no", () => { + // Lines were inserted above it, so its line number moved; its text did not change. + const [found] = run([thread(BOT)], shifted).findings; + assert.equal(found.changed, "no"); + assert.equal(found.path, "gate.mjs"); + assert.equal(found.posted, v1); + assert.equal(found.body, FINDING); + }); + + test("the same finding on changed lines is re-posted: lines changed yes", () => { + const [found] = run([thread(BOT)], edited).findings; + assert.equal(found.changed, "yes"); + }); + + test("a resolved thread is never suppressible: only a thread still open is listed unchanged", () => { + // Same thread and unchanged lines as the case above, which answers no. + const [found] = run([{ ...thread(BOT), isResolved: true }], shifted).findings; + assert.match(found.changed, /^yes \(the thread is resolved/u); + assert.equal(run([{ ...thread(BOT), isResolved: undefined }], shifted).findings[0].changed.startsWith("yes"), true); + }); + + test("a finding on the head it was posted at is unchanged", () => { + assert.equal(run([thread(BOT)], v1).findings[0].changed, "no"); + }); + + test("a finding the job cannot judge is never suppressible", () => { + const failing = collect({ + threads: [thread(BOT)], + reviews: [], + comments: [], + lane: LANE, + head: shifted, + diff: () => { + throw new Error("no such commit"); + }, + }); + assert.match(failing.findings[0].changed, /^yes \(/u); + const left = run([{ ...thread(BOT), diffSide: "LEFT" }], shifted); + assert.match(left.findings[0].changed, /^yes \(/u); + const review = collect({ + threads: [], + reviews: [{ url: "u", body: `Summary finding\n\nclaude-lane: ${LANE}`, lastEditedAt: null, author: BOT, commit: { oid: v1 } }], + comments: [], + lane: LANE, + head: v1, + diff, + }); + assert.equal(review.findings[0].path, null); + assert.match(review.findings[0].changed, /^yes \(no line anchor/u); + }); + + test("touchesOriginal reads the old side of each hunk", () => { + // Old line 5 replaced: touches 5, not 1-3 or 7. + assert.equal(touchesOriginal("@@ -5 +7 @@\n", 5, 5), true); + assert.equal(touchesOriginal("@@ -5 +7 @@\n", 1, 3), false); + assert.equal(touchesOriginal("@@ -5 +7 @@\n", 7, 7), false); + // Two lines inserted after old line 0 border a range starting at 1 only. + assert.equal(touchesOriginal("@@ -0,0 +1,2 @@\n", 1, 1), true); + assert.equal(touchesOriginal("@@ -0,0 +1,2 @@\n", 5, 5), false); + }); +}); + +describe("rounds and the cap", () => { + test("rounds count the distinct earlier heads the lane posted findings on, never the head under review", () => { + const second = { ...thread(BOT), comments: { nodes: [{ ...thread(BOT).comments.nodes[0], originalCommit: { oid: shifted } }] } }; + // A rerun on `edited` already holds the lane's posts on it; they are not a + // round before this review. + const onHead = { ...thread(BOT), comments: { nodes: [{ ...thread(BOT).comments.nodes[0], originalCommit: { oid: edited } }] } }; + const result = collect({ + threads: [thread(BOT), thread(BOT), second, onHead, thread(PERSON, `x\n\nclaude-lane: ${LANE}`, { originalCommit: { oid: edited } })], + reviews: [ + { url: "u", body: `**${LANE}**: 1 inline finding(s) on \`${edited}\`; the summary is in this lane's pull request comment.\n\nclaude-lane: ${LANE}`, lastEditedAt: null, author: BOT, commit: { oid: edited } }, + ], + comments: [], + lane: LANE, + head: edited, + diff, + }); + assert.equal(result.rounds, 2); + }); + + test("a reviewed-head summary comment counts its head as a round only when it states a finding", () => { + // The status line ci-workflows publish-review/publish.cjs statusLine writes. + const summary = (sha, findings, author = BOT) => ({ + url: "u", + lastEditedAt: null, + author, + body: `\n\n**${LANE}** reviewed \`${sha}\`: read 2 of 2 changed file(s); ${findings} finding(s), 0 as inline comments.\n\nA finding.\n\ncritical-findings: 0`, + }); + const skip = { + url: "u", + lastEditedAt: null, + author: BOT, + body: `\n**${LANE}** needs no new review of \`${v1}\`: no file in scope changed.\n\n1 finding(s) carried forward.`, + }; + const rounds = (comments, head = edited) => collect({ threads: [], reviews: [], comments, lane: LANE, head, diff }).rounds; + assert.equal(rounds([summary(v1, 1), summary(shifted, 2)]), 2); + assert.equal(rounds([summary(v1, 0)]), 0); + assert.equal(rounds([skip]), 0); + assert.equal(rounds([summary(edited, 1)]), 0); + assert.equal(rounds([summary(v1, 1, PERSON)]), 0); + assert.equal(collect({ threads: [], reviews: [], comments: [summary(v1, 1)], lane: LANE, head: edited, diff }).findings[0].posted, v1); + }); + + test("readCap reads loop.review-rounds and nothing else", () => { + assert.equal(readCap("version: 2\nloop:\n review-rounds: 3\n dependency-review-rounds: 2\n"), 3); + assert.equal(readCap("version: 2\nloop:\n dependency-review-rounds: 2\n"), null); + assert.equal(readCap("review-rounds: 9\n"), null); + }); + + test("render states past the cap only once the rounds reach it", () => { + const at = render({ lane: LANE, findings: [], rounds: 3, cap: 3, more: false }); + assert.match(at, /^- past the cap: yes$/mu); + assert.match(render({ lane: LANE, findings: [], rounds: 2, cap: 3, more: false }), /^- past the cap: no$/mu); + assert.match(render({ lane: LANE, findings: [], rounds: 9, cap: null, more: false }), /^- past the cap: no \(no cap/mu); + }); +}); diff --git a/plugins/review/tests/prior-findings.test.sh b/plugins/review/tests/prior-findings.test.sh new file mode 100755 index 00000000000..b20b1812059 --- /dev/null +++ b/plugins/review/tests/prior-findings.test.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash +# Discovery wrapper: scripts/run-plugin-tests.sh finds plugins/**/*.test.sh, so +# this hands off to the Node suite. SKIPs (exit 0) when Node is unavailable. +# test-scope: plugins/review/lib/prior-findings.mjs +# test-scope: plugins/review/skills/code-review/SKILL.md +# test-scope: plugins/review/skills/security-review/SKILL.md +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +if ! command -v node >/dev/null 2>&1; then + echo "SKIP: node not installed" >&2 + exit 0 +fi + +exec node --test "$SCRIPT_DIR/prior-findings.test.mjs"