From 74d37c82a9a0609446bf96136979cd2b1758c052 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 24 Sep 2026 13:09:48 -0400 Subject: [PATCH 1/2] fix(claude-review): post no review-count comment when the cap is disabled With max-reviews-per-pr <= 0 the lane now skips the count lookup and never creates or updates the "Claude has reviewed this PR N times" comment. A positive cap keeps the comment and its gate unchanged. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../claude-review-outcome-wiring.test.cjs | 81 +++++++++++++++++++ .github/workflows/claude-review.yml | 32 ++++---- README.md | 6 +- 3 files changed, 100 insertions(+), 19 deletions(-) diff --git a/.github/scripts/claude-review-outcome-wiring.test.cjs b/.github/scripts/claude-review-outcome-wiring.test.cjs index a04d373..e93bbb6 100644 --- a/.github/scripts/claude-review-outcome-wiring.test.cjs +++ b/.github/scripts/claude-review-outcome-wiring.test.cjs @@ -148,3 +148,84 @@ for (const lane of LANES) { } }); } + +// Runs a review-count step's github-script body against a recording client. +const AsyncFunction = Object.getPrototypeOf(async () => {}).constructor; + +async function runCountStep(stepName, env) { + const source = stepSource(laneSource("claude-review.yml"), stepName); + const lines = source + .slice(source.indexOf("script: |\n") + "script: |\n".length) + .split("\n"); + const end = lines.findIndex((line) => line !== "" && !line.startsWith(" ".repeat(12))); + const script = lines + .slice(0, end === -1 ? undefined : end) + .map((line) => line.slice(12)) + .join("\n"); + const calls = []; + const outputs = {}; + const record = (name) => async () => { + calls.push(name); + return { data: {} }; + }; + const github = { + paginate: async () => { + calls.push("listComments"); + return []; + }, + rest: { + issues: { + listComments: record("listComments"), + createComment: record("createComment"), + updateComment: record("updateComment"), + }, + }, + }; + const core = { + setOutput: (name, value) => { + outputs[name] = value; + }, + info: () => {}, + notice: () => {}, + warning: () => {}, + }; + const set = { PR_NUMBER: "42", ...env }; + const previous = Object.fromEntries(Object.keys(set).map((key) => [key, process.env[key]])); + Object.assign(process.env, set); + try { + await new AsyncFunction("core", "github", "context", script)(core, github, { + repo: { owner: "melodic-software", repo: "consumer" }, + }); + } finally { + for (const [key, value] of Object.entries(previous)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } + return { calls, outputs }; +} + +test("a disabled cap (0) neither reads nor posts the review-count comment", async () => { + const lookup = await runCountStep("Check the per-PR review count", { + MAX_REVIEWS_PER_PR: "0", + }); + assert.deepEqual(lookup.calls, []); + assert.equal(lookup.outputs.capped, "false"); + const upsert = await runCountStep("Update the review-count status comment", { + MAX_REVIEWS_PER_PR: "0", + PRIOR_COUNT: "0", + }); + assert.deepEqual(upsert.calls, []); +}); + +test("a positive cap still reads and posts the review-count comment", async () => { + const lookup = await runCountStep("Check the per-PR review count", { + MAX_REVIEWS_PER_PR: "5", + }); + assert.deepEqual(lookup.calls, ["listComments"]); + const upsert = await runCountStep("Update the review-count status comment", { + MAX_REVIEWS_PER_PR: "5", + PRIOR_COUNT: "0", + }); + assert.deepEqual(upsert.calls, ["createComment"]); +}); diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 8f3df0f..db3366f 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -261,9 +261,9 @@ on: Maximum successful reviews per PR before the lane name-stable-skips further runs (caps spend on long-lived PRs). Counted via a visible per-PR status comment ("Claude has reviewed this PR N times") that - is upserted after each successful review — the comment doubles as - the human "was this reviewed" signal. Deleting the comment resets - the count (fail-open by design). Zero or negative disables the cap. + is upserted after each successful review. Deleting the comment + resets the count (fail-open by design). Zero or negative disables + the cap and the comment: the lane neither reads nor posts it. Soft cap: concurrent runs for different heads read the counter before either writes it, so a burst can briefly exceed the cap by the number of concurrent heads. @@ -567,7 +567,8 @@ jobs: pull-number: ${{ steps.resolve-pr.outputs.number }} head-sha: ${{ steps.resolve-pr.outputs.head-sha }} - # Review-count cap (max-reviews-per-pr). The counter is the visible + # Review-count cap (max-reviews-per-pr). A disabled cap (<= 0) skips the + # lookup, so the lane never reads or writes the comment. The counter is the visible # status comment this lane upserts after each successful review — chosen # over the runs API (which counts every advisory success including skips # and needs an `actions: read` grant no caller makes) and over a hidden @@ -590,10 +591,7 @@ jobs: const prNumber = Number(process.env.PR_NUMBER); core.setOutput("capped", "false"); core.setOutput("count", "0"); - if (!Number.isInteger(prNumber) || prNumber <= 0) return; - // The lookup runs even when the cap is disabled: the upsert step - // reads count and comment-id from here, so skipping it would - // degrade the upsert to an insert-per-review. + if (!capEnabled || !Number.isInteger(prNumber) || prNumber <= 0) return; let count = 0; try { const comments = await github.paginate(github.rest.issues.listComments, { @@ -620,7 +618,7 @@ jobs: return; } core.setOutput("count", String(count)); - if (capEnabled && count >= max) { + if (count >= max) { core.notice( `Skipping review: this PR already received ${count} reviews ` + `(max-reviews-per-pr=${max}). Delete the count comment to re-enable.`, @@ -1345,7 +1343,8 @@ jobs: mode: clear-on-success pull-number: ${{ steps.resolve-pr.outputs.number }} - # The visible counter the review-count gate reads. Upserted only after a + # The visible counter the review-count gate reads, posted only while the + # cap is enabled (max-reviews-per-pr > 0). Upserted only after a # review that actually RAN (review-ran == 'true' implies the outcome # step ran, so the run was neither superseded nor capped), so failed # runs, validation skips, and skipped runs never inflate the count: the @@ -1366,6 +1365,11 @@ jobs: HEAD_REPO: ${{ steps.resolve-pr.outputs.head-repo }} with: script: | + const max = Number(process.env.MAX_REVIEWS_PER_PR); + if (!(Number.isFinite(max) && max > 0)) { + core.info("max-reviews-per-pr is disabled; posting no count comment."); + return; + } const prNumber = Number(process.env.PR_NUMBER); if (!Number.isInteger(prNumber) || prNumber <= 0) { core.info("No pull request in this event; nothing to count."); @@ -1380,14 +1384,10 @@ jobs: return; } const count = (Number(process.env.PRIOR_COUNT) || 0) + 1; - const max = Number(process.env.MAX_REVIEWS_PER_PR); - const capNote = - Number.isFinite(max) && max > 0 - ? ` The lane skips further automatic reviews after ${max}; deleting this comment resets the count.` - : " Deleting this comment resets the count."; const body = `\n` + - `Claude has reviewed this PR ${count} time${count === 1 ? "" : "s"}.${capNote}`; + `Claude has reviewed this PR ${count} time${count === 1 ? "" : "s"}. ` + + `The lane skips further automatic reviews after ${max}; deleting this comment resets the count.`; const commentId = Number(process.env.PRIOR_COMMENT_ID); try { if (Number.isInteger(commentId) && commentId > 0) { diff --git a/README.md b/README.md index 1e023c0..84eee98 100644 --- a/README.md +++ b/README.md @@ -896,9 +896,9 @@ spent — so they do not retry on one. **Review-count cap (code-review lane only).** `claude-review.yml` stops reviewing a PR after `max-reviews-per-pr` successful reviews, capping spend on long-lived PRs. The counter is a **visible** per-PR status comment upserted -after each successful review — failed and skipped runs never inflate it — which -doubles as the human "was this reviewed" signal; -deleting it resets the count, which is fail-open by design. A capped run is a +after each successful review — failed and skipped runs never inflate it; +deleting it resets the count, which is fail-open by design. A cap of zero or +less disables both the cap and the comment. A capped run is a name-stable skip, not a red check. Treat it as a soft cap: concurrent runs for different heads read the counter before either writes it, so a burst can briefly exceed it by the number of concurrent heads. From ff951d98972988c01f34bc57d50825677b1a1e15 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 24 Sep 2026 13:15:16 -0400 Subject: [PATCH 2/2] style: biome-format the review-count step test Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/scripts/claude-review-outcome-wiring.test.cjs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/scripts/claude-review-outcome-wiring.test.cjs b/.github/scripts/claude-review-outcome-wiring.test.cjs index e93bbb6..9fabfdc 100644 --- a/.github/scripts/claude-review-outcome-wiring.test.cjs +++ b/.github/scripts/claude-review-outcome-wiring.test.cjs @@ -157,7 +157,9 @@ async function runCountStep(stepName, env) { const lines = source .slice(source.indexOf("script: |\n") + "script: |\n".length) .split("\n"); - const end = lines.findIndex((line) => line !== "" && !line.startsWith(" ".repeat(12))); + const end = lines.findIndex( + (line) => line !== "" && !line.startsWith(" ".repeat(12)), + ); const script = lines .slice(0, end === -1 ? undefined : end) .map((line) => line.slice(12)) @@ -190,7 +192,9 @@ async function runCountStep(stepName, env) { warning: () => {}, }; const set = { PR_NUMBER: "42", ...env }; - const previous = Object.fromEntries(Object.keys(set).map((key) => [key, process.env[key]])); + const previous = Object.fromEntries( + Object.keys(set).map((key) => [key, process.env[key]]), + ); Object.assign(process.env, set); try { await new AsyncFunction("core", "github", "context", script)(core, github, {