diff --git a/.github/scripts/claude-review-outcome-wiring.test.cjs b/.github/scripts/claude-review-outcome-wiring.test.cjs index a04d373..9fabfdc 100644 --- a/.github/scripts/claude-review-outcome-wiring.test.cjs +++ b/.github/scripts/claude-review-outcome-wiring.test.cjs @@ -148,3 +148,88 @@ 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.