diff --git a/.github/scripts/claude-review-outcome-wiring.test.cjs b/.github/scripts/claude-review-outcome-wiring.test.cjs new file mode 100644 index 0000000..a0fba27 --- /dev/null +++ b/.github/scripts/claude-review-outcome-wiring.test.cjs @@ -0,0 +1,111 @@ +"use strict"; + +// The review lane consumes the claude-lane-outcome composite through a +// SHA-pinned `uses:`, so the composite's checked-in source and the version +// the lane actually runs can diverge — exactly the window in which a gate +// reading a not-yet-pinned output evaluates false on every run and silently +// skips the step it guards (the review-count upsert stopped writing for one +// PR window this way). These tests pin the wiring from both ends: the gate +// conditions the lane must carry, and the rule that every consumed output of +// the composite is one its source declares — checked against the pinned +// revision when the object is reachable, and against the working tree always. + +const assert = require("node:assert/strict"); +const { execFileSync } = require("node:child_process"); +const fs = require("node:fs"); +const path = require("node:path"); +const test = require("node:test"); + +const repositoryRoot = path.join(__dirname, "..", ".."); +const workflow = fs.readFileSync( + path.join(repositoryRoot, ".github", "workflows", "claude-review.yml"), + "utf8", +); + +const COMPOSITE_PATH = ".github/actions/claude-lane-outcome/action.yml"; + +function stepSource(stepName) { + const start = workflow.indexOf(` - name: ${stepName}\n`); + assert.notEqual(start, -1, `step not found: ${stepName}`); + const rest = workflow.slice(start + 1); + const next = rest.indexOf("\n - name: "); + return next === -1 ? rest : rest.slice(0, next); +} + +// Every output name the composite's action.yml declares. Indentation-anchored +// to the `outputs:` block's two-space keys so step ids and input names never +// leak into the set. +function declaredOutputs(actionYaml) { + const block = actionYaml.slice( + actionYaml.indexOf("\noutputs:") + 1, + actionYaml.indexOf("\nruns:"), + ); + return new Set( + [...block.matchAll(/^ {2}([\w-]+):/gmu)].map((match) => match[1]), + ); +} + +const consumedOutputs = new Set( + [...workflow.matchAll(/steps\.review-outcome\.outputs\.([\w-]+)/gu)].map( + (match) => match[1], + ), +); + +test("the count upsert and stale-comment clear gate on a review that RAN", () => { + // review-failed == 'false' is also true of a validation skip, which + // reviewed nothing; only review-ran distinguishes a review that happened. + for (const stepName of [ + "Update the review-count status comment", + "Clear stale failure comment after successful review", + ]) { + assert.match( + stepSource(stepName), + /^ {8}if: "!cancelled\(\) && steps\.review-outcome\.outputs\.review-ran == 'true'"$/mu, + `'${stepName}' must gate on review-ran, not on not-failed`, + ); + } +}); + +test("every consumed outcome output is declared by the composite's source", () => { + assert.ok(consumedOutputs.size > 0, "the lane must consume outcome outputs"); + const declared = declaredOutputs( + fs.readFileSync(path.join(repositoryRoot, COMPOSITE_PATH), "utf8"), + ); + for (const name of consumedOutputs) { + assert.ok( + declared.has(name), + `the lane reads steps.review-outcome.outputs.${name}, which ${COMPOSITE_PATH} does not declare`, + ); + } +}); + +test("every consumed outcome output is declared at the pinned revision, when reachable", (t) => { + // The tree check above cannot see the one divergence that matters: a tree + // that declares an output the PINNED revision predates. Reading the + // composite at the pin closes that hole — but only where the pinned object + // exists locally (a shallow CI checkout does not carry history), so an + // unreachable pin skips rather than fails: this half is a local/deep-clone + // guard, and the tree check above is the floor that always runs. + const pin = workflow.match( + /uses: melodic-software\/ci-workflows\/\.github\/actions\/claude-lane-outcome@([0-9a-f]{40})/u, + ); + assert.ok(pin, "the outcome composite must be SHA-pinned"); + let pinnedYaml; + try { + pinnedYaml = execFileSync( + "git", + ["-C", repositoryRoot, "show", `${pin[1]}:${COMPOSITE_PATH}`], + { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"] }, + ); + } catch { + t.skip(`pinned revision ${pin[1]} is not reachable in this clone`); + return; + } + const declared = declaredOutputs(pinnedYaml); + for (const name of consumedOutputs) { + assert.ok( + declared.has(name), + `the lane reads steps.review-outcome.outputs.${name}, which the composite at pinned ${pin[1]} does not declare — repoint the pin and the gates in the same commit`, + ); + } +}); diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 07850c5..8c49445 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -801,7 +801,7 @@ jobs: if: >- !cancelled() && steps.freshness.outputs.superseded != 'true' && steps.review-count.outputs.capped != 'true' - uses: melodic-software/ci-workflows/.github/actions/claude-lane-outcome@b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd # b5d54bf 2026-07-27 + uses: melodic-software/ci-workflows/.github/actions/claude-lane-outcome@7415d4e8e33564b7997763805583e5ad1b062dbe # 7415d4e 2026-08-08 with: outcome: ${{ steps.attempt.outputs.outcome }} execution-file: ${{ steps.attempt.outputs.execution_file }} @@ -838,9 +838,13 @@ jobs: # Symmetric counterpart to the failure step above: a transient infra # failure (e.g. an OIDC blip) can leave the marker comment behind on the # PR. Once a retry (rerun or new push) succeeds, clear that stale - # warning so it is not mistaken for a still-open problem. + # warning so it is not mistaken for a still-open problem. Gated on + # review-ran, not review-failed == 'false': a validation skip is also + # "not failed", but it reviewed nothing, and letting it clear the + # warning would erase the one PR-visible signal that review has not + # been happening. - name: Clear stale failure comment after successful review - if: "!cancelled() && steps.review-outcome.outputs.review-failed == 'false'" + if: "!cancelled() && steps.review-outcome.outputs.review-ran == 'true'" # Tidying a stale comment is housekeeping, and a rejected API call # during it must not turn a successful review red. continue-on-error: true @@ -850,11 +854,14 @@ jobs: mode: clear-on-success # The visible counter the review-count gate reads. Upserted only after a - # successful review (review-failed == 'false' implies the outcome step - # ran, so the run was neither superseded nor capped), so failed and - # skipped runs never inflate the count. + # 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 + # comment is the human "was this reviewed" signal and it charges the + # max-reviews-per-pr budget, so counting a run that reviewed nothing + # both misinforms the reader and burns allowance on nothing. - name: Update the review-count status comment - if: "!cancelled() && steps.review-outcome.outputs.review-failed == 'false'" + if: "!cancelled() && steps.review-outcome.outputs.review-ran == 'true'" # Housekeeping: a rejected API call must not redden a successful # advisory review. continue-on-error: true