diff --git a/.github/actions/claude-lane-marker-comment/action.yml b/.github/actions/claude-lane-marker-comment/action.yml index c9bcdaa2..7ce03c90 100644 --- a/.github/actions/claude-lane-marker-comment/action.yml +++ b/.github/actions/claude-lane-marker-comment/action.yml @@ -39,7 +39,9 @@ inputs: default: "" body-copy: description: >- - Trailing guidance line of the warning body (post-failure mode). + Trailing guidance of the warning body (post-failure mode). May span + multiple lines: only the first line receives the `> ` blockquote + prefix, so every continuation line must carry its own `> ` prefix. required: false default: "Re-running the job, or pushing a new commit, will retry the review." diff --git a/.github/scripts/classify-infra-failure-render.test.cjs b/.github/scripts/classify-infra-failure-render.test.cjs deleted file mode 100644 index 7f3d9cc4..00000000 --- a/.github/scripts/classify-infra-failure-render.test.cjs +++ /dev/null @@ -1,184 +0,0 @@ -"use strict"; - -const assert = require("node:assert/strict"); -const { spawnSync } = 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 consumers = ["claude-review.yml", "claude-security-review.yml"]; - -function workflowSource(name) { - return fs.readFileSync( - path.join(repositoryRoot, ".github", "workflows", name), - "utf8", - ); -} - -test("both review reusables embed the classifier generated from one source", () => { - const renderer = path.join(__dirname, "render-classify-infra-failure.cjs"); - const result = spawnSync(process.execPath, [renderer, "--check"], { - encoding: "utf8", - }); - assert.equal(result.status, 0, `${result.stdout}\n${result.stderr}`); - - for (const name of consumers) { - assert.equal( - workflowSource(name).match( - /Source: \.github\/scripts\/classify-infra-failure\.sh/gu, - )?.length, - 1, - `${name} must embed the generated block exactly once`, - ); - } -}); - -// The annotation is the surface a receiver polls through the check-run API, so -// the class term's shape is a contract, not a formatting choice. -test("each review job emits the class as a bare term in its infra-failure annotation", () => { - for (const name of consumers) { - assert.match( - workflowSource(name), - /^ {10}echo "::error::Claude (security )?review exited with: \$REVIEW_OUTCOME class=\$failure_class" \\$/mu, - `${name} must carry the class as a bare class= term`, - ); - } -}); - -test("each marker-managed infra-status comment carries the class", () => { - for (const name of consumers) { - const source = workflowSource(name); - assert.match( - source, - /^ {10}REVIEW_CLASS: \$\{\{ steps\.review-outcome\.outputs\.failure_class \}\}$/mu, - `${name} must pass the class into the comment step`, - ); - assert.match( - source, - /^ {14}`> - Failure class: \\`\$\{process\.env\.REVIEW_CLASS\}\\``,$/mu, - `${name} must render the class as a comment field`, - ); - } -}); - -function stepSource(workflow, 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); -} - -// The two lanes diverge here, and the difference is the whole point (#266, -// adjudicated; narrowing the #228 non-goal). Infra pass-through is ratified for -// the ADVISORY lane: `review / review` gates nothing whether it is green or red, -// so reddening it on an Anthropic-side blip buys no safety and costs noise — -// an infra failure is not a code-quality signal. -// -// That rationale reasons from merge-irrelevance, so it does not transfer to the -// SECURITY lane, whose check is required precisely to prove a pass ran. There, -// passing on an infra failure certifies an execution that did not happen; it is -// asserted to fail closed by claude-security-review-fail-closed.test.cjs, which -// replays a rate-limited payload through the real outcome step. -// -// Both directions are pinned, because both are one edit away from silently -// inverting across every consumer repo. -test("an infrastructure failure never fails the ADVISORY review job", () => { - const name = "claude-review.yml"; - const source = workflowSource(name); - - // The action exits nonzero on infra errors; continue-on-error is what keeps - // that off the check's conclusion. - assert.match( - stepSource(source, "Claude review"), - /^ {8}continue-on-error: true$/mu, - `${name} must keep continue-on-error on the action step`, - ); - - // The outcome step reports the failure; it must not become the failure. - const outcome = stepSource(source, "Report review outcome"); - const exits = [...outcome.matchAll(/^\s*exit\s+(\d+)\s*$/gmu)].map( - (match) => match[1], - ); - assert.ok( - exits.length > 0, - `${name}: expected the outcome step to exit explicitly`, - ); - for (const code of exits) { - assert.equal( - code, - "0", - `${name}: the advisory lane's outcome step must not exit nonzero on an infra failure (found exit ${code}). If this lane is being promoted to a required check, that is a posture change needing its own ratification — see #266.`, - ); - } -}); - -// Applies to BOTH lanes: whatever a lane's conclusion policy is, the steps that -// merely annotate the PR must never be what decides it. These are -// actions/github-script steps, so a shell `exit` check would never fire on them. -// Two mechanisms can fail a JS step: an explicit failure call, and an unhandled -// rejection — every one of these steps awaits GitHub API calls that can reject -// for reasons unrelated to the code under review, and github-script turns a -// rejection into a failed step on its own. Only continue-on-error covers that -// second path, so it is asserted rather than assumed. -test("the comment steps never decide either lane's conclusion", () => { - for (const name of consumers) { - const source = workflowSource(name); - for (const step of [ - "Comment on genuine review failure", - "Clear stale failure comment after successful review", - ]) { - const body = stepSource(source, step); - assert.match( - body, - /^ {8}uses: actions\/github-script@/mu, - `${name}: "${step}" is no longer a github-script step; this assertion must be rewritten for its new failure mechanism`, - ); - assert.match( - body, - /^ {8}continue-on-error: true$/mu, - `${name}: "${step}" awaits GitHub API calls; without continue-on-error a rejected call fails the job`, - ); - assert.doesNotMatch( - body, - /\bcore\.setFailed\s*\(/u, - `${name}: "${step}" must not fail the job`, - ); - assert.doesNotMatch( - body, - /\bprocess\.exit\s*\(/u, - `${name}: "${step}" must not fail the job`, - ); - } - } -}); - -// The whole point of the projection is that the two free-text fields are read -// inside the generated block and never emitted. A future edit that echoes -// either one, or projects it into the detail JSON, must fail here. -test("neither reusable emits the model-authored result or the raw error stacks", () => { - for (const name of consumers) { - const source = workflowSource(name); - const emissions = [ - ...source.matchAll(/^ {10,}(echo .*|.*\$\{process\.env\..*)$/gmu), - ].map((match) => match[0]); - for (const line of emissions) { - assert.doesNotMatch( - line, - /\$\{?(last\.)?(result|errors)\b/u, - `${name} emits a free-text SDK field: ${line.trim()}`, - ); - } - assert.doesNotMatch( - source, - /^ {12,}result: \$last\.result,?$/mu, - `${name} must not project the model-authored result field`, - ); - assert.doesNotMatch( - source, - /^ {12,}errors: \$last\.errors,?$/mu, - `${name} must not project the raw error stacks`, - ); - } -}); diff --git a/.github/scripts/classify-infra-failure.sh b/.github/scripts/classify-infra-failure.sh deleted file mode 100644 index 4e870abb..00000000 --- a/.github/scripts/classify-infra-failure.sh +++ /dev/null @@ -1,103 +0,0 @@ -# shellcheck shell=bash - -# Project the last claude-code-action SDK result message into safe, structured -# metadata and classify a genuine infrastructure failure into exactly one coarse -# class: auth | rate-limit | overloaded | other. Reads EXECUTION_FILE (the -# action's execution_file output, which may be unset, missing, or unparsable) -# and sets review_detail plus failure_class as shell variables and step outputs. -# -# This runs on a PUBLIC repo. The result message's `result` field is -# model-authored free text and its `errors[]` entries are raw error stacks; both -# are read INSIDE this block only, and nothing but the class token and the -# structured projection below ever leaves it. Never widen the projection to -# either field, and never echo the result message wholesale. -# -# Classification order: -# 1. `api_error_status` — the Anthropic-API HTTP status the SDK records when -# the last assistant turn was an API error: 401/402/403 auth, 429 -# rate-limit, 5xx overloaded, every other status other. -# 2. an allowlisted substring over `errors[]`, matching the serialized -# Anthropic error body (`{"type":"error","error":{"type":"",...}}`) -# that the API SDK folds into an APIError's message, and so into its stack. -# 3. other. -# -# The allowlist mirrors the status mapping type-for-type, so a payload that -# recovers no numeric status still lands in the same class the status would -# have chosen. Against the published error table that is: `authentication_error` -# (401), `billing_error` (402), `permission_error` (403) -> auth; -# `rate_limit_error` (429) -> rate-limit; `api_error` (500), `timeout_error` -# (504), `overloaded_error` (529) -> overloaded. The remaining published types -# — `invalid_request_error` (400), `not_found_error` (404), `conflict_error` -# (409), `request_too_large` (413) — are client errors, not infrastructure, and -# are deliberately left to `other` on both paths. Adding a type to one path -# without the other reopens the asymmetry this note exists to prevent. -# -# `auth` is the credential-unusable class, not the 401-only class: it covers -# every status where the credential cannot be used until a human acts. Per the -# published error table that is 401 `authentication_error`, 402 `billing_error`, -# and 403 `permission_error`. 402 belongs here because the incident this -# classifier exists for (ci-workflows#228 / claude-code-plugins#1122) is -# described as a *usage-dead account credential* — a billing/entitlement death, -# which the API reports as 402, not 401. Leaving 402 in `other` would classify -# the originating incident as unremarkable. The downstream aggregator routes -# `auth` to a human-gated incident, correct for a billing death as much as for -# a revocation. -# -# The two sources are disjoint members of the SDK's result union: -# `api_error_status` exists only on the success variant (the shape a dead -# credential produces — subtype success, is_error true) and `errors[]` only on -# the error subtypes. So the substring pass is the error-variant path, not a -# second look at the same payload, and a present-but-unmapped status stays a -# status decision rather than falling through to it. -: "${GITHUB_OUTPUT:?GITHUB_OUTPUT is required}" - -review_detail="(no execution file was produced)" -failure_class="other" - -if [[ -n "${EXECUTION_FILE:-}" && -f "$EXECUTION_FILE" ]]; then - review_detail=$(jq -c ' - (.[-1] // empty) as $last - | if $last == null then empty - else - $last.api_error_status as $status - | (($last.errors // []) | map(tostring) | join("\n")) as $text - | ( - if ($status | type) == "number" then - if $status == 401 or $status == 402 or $status == 403 then "auth" - elif $status == 429 then "rate-limit" - elif $status >= 500 and $status < 600 then "overloaded" - else "other" - end - elif ($text | (contains("\"type\":\"authentication_error\"") - or contains("\"type\":\"billing_error\"") - or contains("\"type\":\"permission_error\""))) then "auth" - elif ($text | contains("\"type\":\"rate_limit_error\"")) then "rate-limit" - elif ($text | (contains("\"type\":\"overloaded_error\"") - or contains("\"type\":\"timeout_error\"") - or contains("\"type\":\"api_error\""))) then "overloaded" - else "other" - end - ) as $class - | { - subtype: $last.subtype, - is_error: $last.is_error, - num_turns: $last.num_turns, - duration_ms: $last.duration_ms, - total_cost_usd: $last.total_cost_usd, - api_error_status: $status, - class: $class - } - end - ' "$EXECUTION_FILE" 2>/dev/null || true) - if [[ -n "$review_detail" ]]; then - failure_class=$(jq -r '.class // "other"' <<<"$review_detail" 2>/dev/null || true) - [[ -n "$failure_class" ]] || failure_class="other" - else - review_detail="(execution file present but unparsable)" - fi -fi - -{ - echo "failure_class=$failure_class" - echo "review_detail=$review_detail" -} >>"$GITHUB_OUTPUT" diff --git a/.github/scripts/classify-infra-failure.test.sh b/.github/scripts/classify-infra-failure.test.sh deleted file mode 100644 index 4cd316fc..00000000 --- a/.github/scripts/classify-infra-failure.test.sh +++ /dev/null @@ -1,188 +0,0 @@ -# shellcheck shell=bash -set -euo pipefail - -core="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/classify-infra-failure.sh" -temporary_directory="$(mktemp -d)" -trap 'rm -rf -- "$temporary_directory"' EXIT -github_output="$temporary_directory/output" -execution_file="$temporary_directory/execution.json" - -# A distinctive string standing in for the model-authored `result` text and the -# raw error stacks: it must never reach an output, because those two fields are -# the ones this repo keeps off every public surface. -CANARY='canary-never-publish-this' - -result_message() { # api_error_status_json extra_json - extra="${2:-}" - [[ -n "$extra" ]] || extra='{}' - jq -cn --argjson status "$1" --argjson extra "$extra" --arg canary "$CANARY" ' - [{ - type: "result", - subtype: "success", - is_error: true, - num_turns: 1, - duration_ms: 1856, - total_cost_usd: 0, - result: ("model-authored free text " + $canary), - api_error_status: $status - } + $extra] - ' -} - -error_message() { # errors_json_array - jq -cn --argjson errors "$1" ' - [{ - type: "result", - subtype: "error_during_execution", - is_error: true, - num_turns: 2, - duration_ms: 42, - total_cost_usd: 0, - errors: $errors - }] - ' -} - -# The first element of a real error-variant `errors[]` is always a synthetic -# diagnostic line; no allowlist token may collide with its vocabulary. -diagnostic='"[ede_diagnostic] result_type=api_error last_content_type=text stop_reason=null"' # spellchecker:disable-line - -api_error_body() { # error_type -> a serialized APIError stack line - jq -cn --arg type "$1" --arg canary "$CANARY" ' - "APIError: {\"type\":\"error\",\"error\":{\"type\":\"" + $type + - "\",\"message\":\"" + $canary + "\"}}\n at request (/x/y.js:1:1)" - ' -} - -run() { # execution_file_contents -> sets status, out, class, detail - if [[ "$#" -gt 0 ]]; then - jq -c . <<<"$1" >"$execution_file" - fi - set +e - out="$(EXECUTION_FILE="$execution_file" GITHUB_OUTPUT="$github_output" bash "$core" 2>&1)" - status=$? - set -e - class="$(sed -n 's/^failure_class=//p' "$github_output" 2>/dev/null || true)" - detail="$(sed -n 's/^review_detail=//p' "$github_output" 2>/dev/null || true)" - rm -f -- "$github_output" -} - -fail() { - echo "FAIL: $1" >&2 - exit 1 -} - -assert_class() { # label expected_class contents - run "$3" - [[ "$status" == 0 ]] || fail "$1: expected exit 0, got $status ($out)" - [[ "$class" == "$2" ]] || fail "$1: expected class '$2', got '$class' (detail: $detail)" - grep -F "$CANARY" <<<"$out$detail$class" >/dev/null && - fail "$1: free-text field reached an output surface" - return 0 -} - -# 1-6. The structured Anthropic-API status decides the class on the shape a -# dead credential produces (subtype success, is_error true). 402 is -# `billing_error` — the usage-dead-credential shape of the originating -# incident — and is an `auth` (credential-unusable) case, not an unmapped -# one; see the classifier header. -assert_class '401' auth "$(result_message 401)" -assert_class '402' auth "$(result_message 402)" -assert_class '403' auth "$(result_message 403)" -assert_class '429' rate-limit "$(result_message 429)" -assert_class '500' overloaded "$(result_message 500)" -assert_class '529' overloaded "$(result_message 529)" - -# 7. A status outside the mapped set is still a status decision: it resolves to -# `other` rather than falling through to the substring pass. 413 -# `request_too_large` is a genuine non-infra client error, so it is the -# honest unmapped case. -assert_class 'unmapped status' other \ - "$(result_message 413 "{\"errors\":[$(api_error_body rate_limit_error)]}")" - -# 8. A status present but null is treated as absent, so the substring pass runs. -assert_class 'null status with auth text' auth \ - "$(result_message null "{\"errors\":[$diagnostic,$(api_error_body authentication_error)]}")" - -# 9-15. The error-variant substring pass, one case per allowlisted token. -assert_class 'authentication_error' auth \ - "$(error_message "[$diagnostic,$(api_error_body authentication_error)]")" -assert_class 'billing_error' auth \ - "$(error_message "[$diagnostic,$(api_error_body billing_error)]")" -assert_class 'permission_error' auth \ - "$(error_message "[$diagnostic,$(api_error_body permission_error)]")" -assert_class 'rate_limit_error' rate-limit \ - "$(error_message "[$diagnostic,$(api_error_body rate_limit_error)]")" -assert_class 'overloaded_error' overloaded \ - "$(error_message "[$diagnostic,$(api_error_body overloaded_error)]")" -assert_class 'timeout_error' overloaded \ - "$(error_message "[$diagnostic,$(api_error_body timeout_error)]")" -assert_class 'api_error' overloaded \ - "$(error_message "[$diagnostic,$(api_error_body api_error)]")" - -# 16-17. The client-error types stay `other` on the substring path exactly as they -# do on the status path: they are not infrastructure failures. -assert_class 'invalid_request_error' other \ - "$(error_message "[$diagnostic,$(api_error_body invalid_request_error)]")" -assert_class 'not_found_error' other \ - "$(error_message "[$diagnostic,$(api_error_body not_found_error)]")" - -# 18. The synthetic diagnostic line alone must not match any token, even though -# it can carry the literal text `api_error` in its result_type field. -assert_class 'diagnostic only' other "$(error_message "[$diagnostic]")" - -# 19. The observed incident payload, whose projection carried no class field at -# all: no status, no errors, nothing to classify. -assert_class 'unclassifiable' other \ - '[{"type":"result","subtype":"success","is_error":true,"num_turns":1,"duration_ms":1856,"total_cost_usd":0}]' - -# 20. The class token joins the safe projection alongside the numeric status. -run "$(result_message 401)" -[[ "$(jq -r '.class' <<<"$detail")" == auth ]] || fail "projection: missing class ($detail)" -[[ "$(jq -r '.api_error_status' <<<"$detail")" == 401 ]] || fail "projection: missing status ($detail)" -[[ "$(jq -r 'has("result") or has("errors")' <<<"$detail")" == false ]] || - fail "projection: leaked a free-text field ($detail)" - -# 21. An unparsable execution file degrades to `other` and says so, rather than -# emitting an empty detail that reads as a successful projection. -run -jq -c . <<<'[]' >"$execution_file" -run -[[ "$class" == other ]] || fail "empty array: expected other, got '$class'" -[[ "$detail" == '(execution file present but unparsable)' ]] || - fail "empty array: unexpected detail '$detail'" - -cp /dev/null "$execution_file" -run -[[ "$class" == other ]] || fail "unparsable: expected other, got '$class'" -[[ "$detail" == '(execution file present but unparsable)' ]] || - fail "unparsable: unexpected detail '$detail'" - -# 22. No execution file at all (the action failed before producing one). -rm -f -- "$execution_file" -run -[[ "$class" == other ]] || fail "missing file: expected other, got '$class'" -[[ "$detail" == '(no execution file was produced)' ]] || - fail "missing file: unexpected detail '$detail'" - -out="$(EXECUTION_FILE="" GITHUB_OUTPUT="$github_output" bash "$core" 2>&1)" -class="$(sed -n 's/^failure_class=//p' "$github_output")" -detail="$(sed -n 's/^review_detail=//p' "$github_output")" -rm -f -- "$github_output" -[[ "$class" == other ]] || fail "empty EXECUTION_FILE: expected other, got '$class'" -[[ "$detail" == '(no execution file was produced)' ]] || - fail "empty EXECUTION_FILE: unexpected detail '$detail'" - -# 23. An unset GITHUB_OUTPUT fails closed instead of silently discarding the -# class the annotation and the PR comment are both keyed on. -# `env -u` is required, not cosmetic: a GitHub runner exports GITHUB_OUTPUT -# into every step, so merely omitting the assignment would inherit the real -# one — the case would pass vacuously on a workstation and fail in CI, and -# the run would append this probe's outputs to the live step output. -set +e -out="$(env -u GITHUB_OUTPUT EXECUTION_FILE="" bash "$core" 2>&1)" -status=$? -set -e -[[ "$status" != 0 ]] || fail "unset GITHUB_OUTPUT: expected non-zero exit, got 0 ($out)" - -echo "classify-infra-failure.sh: all cases passed" diff --git a/.github/scripts/claude-review-superseded-guard.test.cjs b/.github/scripts/claude-review-superseded-guard.test.cjs index 68f02322..1048316f 100644 --- a/.github/scripts/claude-review-superseded-guard.test.cjs +++ b/.github/scripts/claude-review-superseded-guard.test.cjs @@ -26,10 +26,14 @@ test("the review concurrency group is keyed per head, not per PR", () => { assert.match(workflowSource, /^ {6}cancel-in-progress: true$/mu); }); -test("the superseded-head guard compares the event head to the live PR head", () => { +test("the superseded-head guard is the pinned freshness composite", () => { + // The live-head comparison itself lives in the composite; what this + // workflow owns is invoking it under the `freshness` id at a full-SHA + // self-reference pin (a local ./ ref would resolve against the caller's + // checkout in a reusable workflow). assert.match( workflowSource, - /id: freshness[\s\S]*?uses: actions\/github-script@[\s\S]*?github\.rest\.pulls\.get\(/u, + /id: freshness\s+uses: melodic-software\/ci-workflows\/\.github\/actions\/claude-lane-freshness@[0-9a-f]{40}/u, ); }); diff --git a/.github/scripts/claude-security-review-fail-closed.test.cjs b/.github/scripts/claude-security-review-fail-closed.test.cjs index 814035dc..a9b7a290 100644 --- a/.github/scripts/claude-security-review-fail-closed.test.cjs +++ b/.github/scripts/claude-security-review-fail-closed.test.cjs @@ -2,9 +2,13 @@ // The security lane's required check certifies EXECUTION, so "in scope and could // not run" must conclude failure — `neutral` and `skipped` both satisfy a -// required check and cannot express it (#266). These tests run the outcome step's -// real script rather than grepping for `exit 1`, because the thing worth pinning -// is the exit code a replayed 429 payload actually produces. +// required check and cannot express it (#266). The classification itself lives +// in the claude-lane-outcome composite (its corpus runs in +// .github/actions/claude-lane-outcome/classify.test.cjs); what this workflow +// owns — and what these tests pin — is the wiring around it: the resolve step +// that picks the effective attempt, the outcome composite reading that attempt, +// and the fail-closed step that raises the red on the one signal that means +// "in scope and did not run". const assert = require("node:assert/strict"); const { spawnSync } = require("node:child_process"); @@ -49,137 +53,8 @@ function runScript(stepName) { .join("\n"); } -// Run 30217744377's payload: the shape a rate-limited call leaves behind — the -// SDK's success variant with is_error true and no turns taken. This is the run -// #266 was filed from, and it concluded green. -function rateLimitedExecutionFile() { - return JSON.stringify([ - { - type: "result", - subtype: "success", - is_error: true, - num_turns: 1, - duration_ms: 480, - total_cost_usd: 0, - result: "model-authored free text that must never be published", - api_error_status: 429, - }, - ]); -} - -function reportOutcome({ - outcome, - executionFileContents, - isPullRequest = "true", -}) { - const directory = fs.mkdtempSync( - path.join(os.tmpdir(), "security-review-outcome-"), - ); - try { - const githubOutput = path.join(directory, "github-output"); - fs.writeFileSync(githubOutput, ""); - const environment = { - ...process.env, - REVIEW_OUTCOME: outcome, - IS_PULL_REQUEST: isPullRequest, - GITHUB_OUTPUT: githubOutput, - }; - if (executionFileContents !== undefined) { - const executionFile = path.join(directory, "execution.json"); - fs.writeFileSync(executionFile, executionFileContents); - environment.EXECUTION_FILE = executionFile; - } - const result = spawnSync( - "bash", - ["-c", runScript("Report review outcome")], - { - encoding: "utf8", - env: environment, - }, - ); - const outputs = Object.fromEntries( - fs - .readFileSync(githubOutput, "utf8") - .split("\n") - .filter((line) => line.includes("=")) - .map((line) => { - const separator = line.indexOf("="); - return [line.slice(0, separator), line.slice(separator + 1)]; - }), - ); - return { ...result, outputs }; - } finally { - fs.rmSync(directory, { recursive: true, force: true }); - } -} - -test("a replayed rate-limited review fails the job and still reports its class", () => { - const result = reportOutcome({ - outcome: "failure", - executionFileContents: rateLimitedExecutionFile(), - }); - - assert.equal( - result.status, - 1, - `the outcome step must exit nonzero so the required check concludes failure\n${result.stdout}\n${result.stderr}`, - ); - assert.equal(result.outputs.review_failed, "true"); - // Written before the exit, so the PR still gets its infra-status comment. - assert.equal(result.outputs.failure_class, "rate-limit"); - assert.match(result.stdout, /::error::Claude security review exited with:/u); -}); - -// The free-text SDK fields stay off this public repo's logs even on the path that -// now fails the job. -test("failing closed does not start publishing the model-authored result", () => { - const result = reportOutcome({ - outcome: "failure", - executionFileContents: rateLimitedExecutionFile(), - }); - assert.doesNotMatch(result.stdout, /must never be published/u); - for (const value of Object.values(result.outputs)) { - assert.doesNotMatch(value, /must never be published/u); - } -}); - -test("a completed review still passes", () => { - const result = reportOutcome({ outcome: "success" }); - assert.equal(result.status, 0, `${result.stdout}\n${result.stderr}`); - assert.equal(result.outputs.review_failed, "false"); -}); - -// A missing execution file must not become a second failure mode: the class -// degrades to `other` and the job still fails closed on the same signal. -test("an unreadable execution file still fails closed", () => { - const result = reportOutcome({ outcome: "failure" }); - assert.equal(result.status, 1, `${result.stdout}\n${result.stderr}`); - assert.equal(result.outputs.review_failed, "true"); - assert.equal(result.outputs.failure_class, "other"); -}); - -// The action rejects merge_group outright and rejects every non-PR event while -// track_progress is on, so those runs fail for a cause no head change can fix. -// Failing them closed would wedge a consumer's merge queue on a red check that -// the PR-gated comment steps cannot even explain. -test("a non-pull_request run reports the failure without failing the job", () => { - const result = reportOutcome({ - outcome: "failure", - executionFileContents: rateLimitedExecutionFile(), - isPullRequest: "false", - }); - assert.equal( - result.status, - 0, - `merge_group / workflow_dispatch / schedule runs must keep the historical pass-through\n${result.stdout}\n${result.stderr}`, - ); - // Still classified and still annotated — pass-through, not silence. - assert.equal(result.outputs.review_failed, "true"); - assert.equal(result.outputs.failure_class, "rate-limit"); -}); - -// The resolve step decides what the outcome step sees, so a bug here reddens -// every CLEAN review — the widest blast radius in this workflow. +// The resolve step decides what the outcome composite sees, so a bug here +// reddens every CLEAN review — the widest blast radius in this workflow. function resolveAttempt({ first, firstFile, retry, retryFile }) { const directory = fs.mkdtempSync( path.join(os.tmpdir(), "security-review-attempt-"), @@ -260,9 +135,58 @@ test("both attempts failing resolves to the retry's payload", () => { assert.equal(resolved.execution_file, "/tmp/retry.json"); }); +test("the outcome composite reads the resolved attempt, not just the first one", () => { + const step = stepSource("Report review outcome"); + assert.match( + step, + /uses: melodic-software\/ci-workflows\/\.github\/actions\/claude-lane-outcome@[0-9a-f]{40}/u, + "the outcome step must be the pinned claude-lane-outcome composite", + ); + assert.match( + step, + /^ {10}outcome: \$\{\{ steps\.attempt\.outputs\.outcome \}\}$/mu, + "reading steps.claude-review directly would ignore a successful retry", + ); + assert.match( + step, + /^ {10}execution-file: \$\{\{ steps\.attempt\.outputs\.execution_file \}\}$/mu, + "the classified payload must come from the attempt that actually ran last", + ); + // No continue-on-error: if the composite itself crashes before producing a + // verdict, the job must go red (closed), not silently green. + assert.doesNotMatch( + step, + /continue-on-error/u, + "a crashed outcome composite must fail the job, not pass through", + ); +}); + +test("an in-scope non-run fails the job through the fail-closed step", () => { + const step = stepSource("Fail closed on an in-scope non-run"); + + // The red is raised deliberately, on the one signal that means "in scope + // and did not run": the outcome composite recorded a failure AND this is a + // pull_request run (only a pull_request run gates a merge — the action + // rejects merge_group and, with track_progress on, every non-PR event, so + // reddening those would wedge a consumer's merge queue on a cause no head + // change can fix; they keep the historical pass-through by skipping this + // step). always() keeps the step reachable after the failed review step. + assert.match( + step, + /^ {8}if: >-\n {10}always\(\) && steps\.review-outcome\.outputs\.review-failed == 'true' &&\n {10}github\.event\.pull_request\.number != ''$/mu, + "the fail-closed condition must key on review-failed and pull_request presence exactly", + ); + assert.match(step, /^ {10}exit 1$/mu, "the step must conclude failure"); + assert.doesNotMatch( + step, + /continue-on-error/u, + "continue-on-error here would revert #266 wholesale", + ); +}); + // Everything below pins the three no-verdict paths that must NOT reach the -// failing step. Each is the reason the mapping keys on review_failed rather than -// on "no verdict was produced". +// failing step. Each is the reason the mapping keys on review-failed rather +// than on "no verdict was produced". test("fork PRs skip the job instead of failing closed forever", () => { const jobCondition = workflow.slice( workflow.indexOf(" security-review:"), @@ -297,7 +221,7 @@ test("a superseded head reports nothing, so it cannot fail closed", () => { assert.match( step, /^ {8}if: always\(\) && steps\.freshness\.outputs\.superseded != 'true'$/mu, - "a retired run must skip the outcome step, leaving review_failed unset", + "a retired run must skip the outcome step, leaving review-failed unset", ); }); @@ -332,7 +256,7 @@ test("the review retry is configured identically to the first attempt", () => { assert.match( retry, /^ {8}continue-on-error: true$/mu, - "the retry must not fail the job itself; the outcome step owns the red", + "the retry must not fail the job itself; the fail-closed step owns the red", ); assert.match( retry, @@ -359,41 +283,3 @@ test("the review retry is configured identically to the first attempt", () => { "the retry must back off rather than immediately re-hammering a rate-limited API", ); }); - -test("the outcome step reads the resolved attempt, not just the first one", () => { - const step = stepSource("Report review outcome"); - assert.match( - step, - /^ {10}REVIEW_OUTCOME: \$\{\{ steps\.attempt\.outputs\.outcome \}\}$/mu, - "reading steps.claude-review directly would ignore a successful retry", - ); - assert.match( - step, - /^ {10}EXECUTION_FILE: \$\{\{ steps\.attempt\.outputs\.execution_file \}\}$/mu, - "the classified payload must come from the attempt that actually ran last", - ); - // The harness supplies this variable itself, so without pinning the YAML a - // deleted or incorrect env line would leave it unset in CI and every event - // would take the pass-through branch — reverting #266 silently, and in the - // fail-OPEN direction this suite exists to prevent. - assert.match( - step, - /^ {10}IS_PULL_REQUEST: \$\{\{ github\.event\.pull_request\.number != '' \}\}$/mu, - "a missing or incorrect IS_PULL_REQUEST disables fail-closed on every event", - ); -}); - -// Belt and braces with the assertion above: even if the wiring is lost, the -// guard's sense must keep an unset value on the fail-closed path. -test("an unset IS_PULL_REQUEST fails closed rather than passing through", () => { - const result = reportOutcome({ - outcome: "failure", - executionFileContents: rateLimitedExecutionFile(), - isPullRequest: "", - }); - assert.equal( - result.status, - 1, - `an absent IS_PULL_REQUEST must not be read as "not a pull request"\n${result.stdout}\n${result.stderr}`, - ); -}); diff --git a/.github/scripts/render-classify-infra-failure.cjs b/.github/scripts/render-classify-infra-failure.cjs deleted file mode 100644 index e9ae815e..00000000 --- a/.github/scripts/render-classify-infra-failure.cjs +++ /dev/null @@ -1,74 +0,0 @@ -"use strict"; - -const fs = require("node:fs"); -const path = require("node:path"); - -const scriptsDirectory = __dirname; -const sourcePath = path.join(scriptsDirectory, "classify-infra-failure.sh"); -const workflowsDirectory = path.join(scriptsDirectory, "..", "workflows"); -// Both review reusables share the infra-failure surfacing shape, and both run -// on a caller-selected runner over the CALLER's checkout — this repo's scripts -// directory is not on disk there, so the source is embedded rather than called. -const workflowFiles = ["claude-review.yml", "claude-security-review.yml"]; -const startMarker = - " # BEGIN GENERATED CLASSIFY INFRA FAILURE - DO NOT EDIT"; -const endMarker = " # END GENERATED CLASSIFY INFRA FAILURE"; - -function bundledScript(source) { - // The `shell=bash` directive is for standalone ShellCheck of the source file. - // Inlined, it lands mid-run:-block, which ShellCheck rejects (a directive - // must precede all commands); actionlint already supplies the bash shell for - // embedded scripts, so drop it from the generated copy. - const body = source - .trimEnd() - .split(/\r?\n/u) - .filter((line) => line.trim() !== "# shellcheck shell=bash"); - while (body.length > 0 && body[0].trim() === "") { - body.shift(); - } - return [ - "# BEGIN GENERATED CLASSIFY INFRA FAILURE - DO NOT EDIT", - "# Source: .github/scripts/classify-infra-failure.sh", - ...body, - "# END GENERATED CLASSIFY INFRA FAILURE", - ] - .map((line) => (line.length === 0 ? "" : ` ${line}`)) - .join("\n"); -} - -function render(workflow, source, name) { - const start = workflow.indexOf(startMarker); - const end = workflow.indexOf(endMarker); - if (start < 0 || end < start) { - throw new Error( - `${name} is missing generated classify-infra-failure markers`, - ); - } - return `${workflow.slice(0, start)}${bundledScript(source)}${workflow.slice(end + endMarker.length)}`; -} - -const source = fs.readFileSync(sourcePath, "utf8"); -const check = process.argv.includes("--check"); -let drift = false; - -for (const name of workflowFiles) { - const workflowPath = path.join(workflowsDirectory, name); - const current = fs.readFileSync(workflowPath, "utf8"); - const expected = render(current, source, name); - if (check) { - if (current !== expected) { - process.stderr.write( - `${name} is out of sync; run node .github/scripts/render-classify-infra-failure.cjs\n`, - ); - drift = true; - } - } else { - fs.writeFileSync(workflowPath, expected, "utf8"); - } -} - -if (check && drift) { - process.exitCode = 1; -} - -module.exports = Object.freeze({ bundledScript, render }); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 28416356..2c847ccc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -379,8 +379,6 @@ jobs: run: bash .github/scripts/osv-release-digest.test.sh - name: Test tracking-issue resolver run: bash .github/scripts/find-tracking-issue.test.sh - - name: Test infra-failure classification - run: bash .github/scripts/classify-infra-failure.test.sh - name: Test Gitleaks scan policy guard run: bash .github/actions/gitleaks/scan.test.sh - name: Test Pulumi deployment guard diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index e2b35e76..6a66d45e 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -198,46 +198,14 @@ jobs: # A stale run must retire itself, not rely on being cancelled: with the # per-head concurrency key above, nothing cancels a run whose head was - # superseded while it sat in a queue. Ask the live API whether this - # event's head is still the PR's head; when it is not, a newer run owns - # the review, so every runner-consuming step below skips (posting - # feedback for an obsolete head is worse than doing nothing, and the - # newer run also owns the marker-comment lifecycle). Fails open — the - # `!= 'true'` step gates treat a missing output as fresh, and only a - # response shaped like a commit SHA may retire the run (a degenerate - # success such as jq's literal `null` must not) — so an API anomaly - # degrades to reviewing a possibly-stale head, never to silence. - # actions/github-script rather than a `gh api` run step: this reusable - # runs on whatever runner the caller selected, and a self-hosted image - # is not guaranteed to ship the gh CLI. github-script executes on the - # action's own bundled Node runtime, so the live freshness check has no - # runner-image dependency (same pattern as do-not-merge-gate.yml). + # superseded while it sat in a queue. The composite owns the live-head + # check and its fail-open contract — downstream `!= 'true'` gates treat + # a missing output as fresh. Pinned to this repo's own merged SHA: a + # local ./ ref resolves against the CALLER's checkout in a reusable + # workflow and cannot work. - name: Check whether this head is still current id: freshness - if: github.event.pull_request.number != '' - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - with: - script: | - let liveHead = ""; - try { - const { data: pr } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - }); - liveHead = pr.head.sha; - } catch (error) { - core.warning(`Could not refetch the PR head; treating this run as fresh: ${error.message}`); - } - const eventHeadSha = process.env.EVENT_HEAD_SHA ?? ""; - if (/^[0-9a-f]{40}$/.test(liveHead) && liveHead !== eventHeadSha) { - core.notice(`Skipping review: the PR head moved from ${eventHeadSha} to ${liveHead}, so a newer run owns the review.`); - core.setOutput("superseded", "true"); - } else { - core.setOutput("superseded", "false"); - } + uses: melodic-software/ci-workflows/.github/actions/claude-lane-freshness@b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd # b5d54bf 2026-07-27 # persist-credentials stays at its default (true) until upstream # claude-code-action#1236 ships: the action's setupBranch runs @@ -346,257 +314,50 @@ jobs: https://github.com/${{ github.repository }}/blob/${{ github.event.pull_request.head.sha }}/#L # Also gated on the guard: a superseded run has no outcome to report, and - # skipping this step leaves review_failed empty so both marker-comment + # skipping this step leaves review-failed empty so both marker-comment # steps below skip too — a retired run must not touch the newer run's PR - # comment state. + # comment state. The composite owns the classification (classify.cjs — + # the sole reader of the execution file's free-text fields) and the + # machine-readable `class=` annotation. - name: Report review outcome id: review-outcome if: always() && steps.freshness.outputs.superseded != 'true' - env: - REVIEW_OUTCOME: ${{ steps.claude-review.outcome }} - EXECUTION_FILE: ${{ steps.claude-review.outputs.execution_file }} - run: | - if [ "$REVIEW_OUTCOME" = "success" ]; then - echo "Claude review completed. Findings (if any) are in the PR comments." - echo "review_failed=false" >> "$GITHUB_OUTPUT" - exit 0 - fi - - # Prior to claude-code-action v1.0.172, the action reported `success` - # even when the underlying SDK result had is_error:true (upstream - # #1495, fixed in #1496) — this branch was effectively dead. Bumping - # past the fix makes REVIEW_OUTCOME finally reflect real failures, so - # this now needs to actually surface them instead of just logging. - # - # The raw SDKResultMessage is NOT safe to publish as-is on this public - # repo — the same leak risk that keeps display_report and - # show_full_output off (CLAUDE.md). The embedded block below is the - # sole reader of its free-text fields; its header carries the - # projection and classification contract. - # BEGIN GENERATED CLASSIFY INFRA FAILURE - DO NOT EDIT - # Source: .github/scripts/classify-infra-failure.sh - # Project the last claude-code-action SDK result message into safe, structured - # metadata and classify a genuine infrastructure failure into exactly one coarse - # class: auth | rate-limit | overloaded | other. Reads EXECUTION_FILE (the - # action's execution_file output, which may be unset, missing, or unparsable) - # and sets review_detail plus failure_class as shell variables and step outputs. - # - # This runs on a PUBLIC repo. The result message's `result` field is - # model-authored free text and its `errors[]` entries are raw error stacks; both - # are read INSIDE this block only, and nothing but the class token and the - # structured projection below ever leaves it. Never widen the projection to - # either field, and never echo the result message wholesale. - # - # Classification order: - # 1. `api_error_status` — the Anthropic-API HTTP status the SDK records when - # the last assistant turn was an API error: 401/402/403 auth, 429 - # rate-limit, 5xx overloaded, every other status other. - # 2. an allowlisted substring over `errors[]`, matching the serialized - # Anthropic error body (`{"type":"error","error":{"type":"",...}}`) - # that the API SDK folds into an APIError's message, and so into its stack. - # 3. other. - # - # The allowlist mirrors the status mapping type-for-type, so a payload that - # recovers no numeric status still lands in the same class the status would - # have chosen. Against the published error table that is: `authentication_error` - # (401), `billing_error` (402), `permission_error` (403) -> auth; - # `rate_limit_error` (429) -> rate-limit; `api_error` (500), `timeout_error` - # (504), `overloaded_error` (529) -> overloaded. The remaining published types - # — `invalid_request_error` (400), `not_found_error` (404), `conflict_error` - # (409), `request_too_large` (413) — are client errors, not infrastructure, and - # are deliberately left to `other` on both paths. Adding a type to one path - # without the other reopens the asymmetry this note exists to prevent. - # - # `auth` is the credential-unusable class, not the 401-only class: it covers - # every status where the credential cannot be used until a human acts. Per the - # published error table that is 401 `authentication_error`, 402 `billing_error`, - # and 403 `permission_error`. 402 belongs here because the incident this - # classifier exists for (ci-workflows#228 / claude-code-plugins#1122) is - # described as a *usage-dead account credential* — a billing/entitlement death, - # which the API reports as 402, not 401. Leaving 402 in `other` would classify - # the originating incident as unremarkable. The downstream aggregator routes - # `auth` to a human-gated incident, correct for a billing death as much as for - # a revocation. - # - # The two sources are disjoint members of the SDK's result union: - # `api_error_status` exists only on the success variant (the shape a dead - # credential produces — subtype success, is_error true) and `errors[]` only on - # the error subtypes. So the substring pass is the error-variant path, not a - # second look at the same payload, and a present-but-unmapped status stays a - # status decision rather than falling through to it. - : "${GITHUB_OUTPUT:?GITHUB_OUTPUT is required}" - - review_detail="(no execution file was produced)" - failure_class="other" - - if [[ -n "${EXECUTION_FILE:-}" && -f "$EXECUTION_FILE" ]]; then - review_detail=$(jq -c ' - (.[-1] // empty) as $last - | if $last == null then empty - else - $last.api_error_status as $status - | (($last.errors // []) | map(tostring) | join("\n")) as $text - | ( - if ($status | type) == "number" then - if $status == 401 or $status == 402 or $status == 403 then "auth" - elif $status == 429 then "rate-limit" - elif $status >= 500 and $status < 600 then "overloaded" - else "other" - end - elif ($text | (contains("\"type\":\"authentication_error\"") - or contains("\"type\":\"billing_error\"") - or contains("\"type\":\"permission_error\""))) then "auth" - elif ($text | contains("\"type\":\"rate_limit_error\"")) then "rate-limit" - elif ($text | (contains("\"type\":\"overloaded_error\"") - or contains("\"type\":\"timeout_error\"") - or contains("\"type\":\"api_error\""))) then "overloaded" - else "other" - end - ) as $class - | { - subtype: $last.subtype, - is_error: $last.is_error, - num_turns: $last.num_turns, - duration_ms: $last.duration_ms, - total_cost_usd: $last.total_cost_usd, - api_error_status: $status, - class: $class - } - end - ' "$EXECUTION_FILE" 2>/dev/null || true) - if [[ -n "$review_detail" ]]; then - failure_class=$(jq -r '.class // "other"' <<<"$review_detail" 2>/dev/null || true) - [[ -n "$failure_class" ]] || failure_class="other" - else - review_detail="(execution file present but unparsable)" - fi - fi - - { - echo "failure_class=$failure_class" - echo "review_detail=$review_detail" - } >>"$GITHUB_OUTPUT" - # END GENERATED CLASSIFY INFRA FAILURE - - # `class=` is a machine-readable term, not prose: this - # annotation is the detection surface a receiver polls through the - # check-run API, so keep it bare and greppable. - echo "::error::Claude review exited with: $REVIEW_OUTCOME class=$failure_class" \ - "(infrastructure error, not a code-quality signal — e.g. usage limit," \ - "OIDC failure, SDK crash, is_error result, or max-turns exhaustion)." \ - "Last SDK result: $review_detail" - - echo "review_failed=true" >> "$GITHUB_OUTPUT" + uses: melodic-software/ci-workflows/.github/actions/claude-lane-outcome@b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd # b5d54bf 2026-07-27 + with: + outcome: ${{ steps.claude-review.outcome }} + execution-file: ${{ steps.claude-review.outputs.execution_file }} + lane: Claude review - # Runs only on a genuine infra failure (never on a clean review) and only - # when the token can write: fork-triggered pull_request runs get a - # read-only GITHUB_TOKEN and no secrets by design (CLAUDE.md), so - # gh pr comment would just fail there — skip rather than error. - # Targets the marker comment by ID (never --edit-last, which edits the - # PR's last comment from ANY author) so a stuck placeholder comment from - # track_progress is never mistaken for "no findings" and an unrelated - # bot comment is never clobbered — this is the failure mode this whole - # change exists to fix. - # actions/github-script rather than `gh api`/`gh pr comment` run steps: - # this reusable runs on whatever runner the caller selected, and a - # self-hosted image is not guaranteed to ship the gh CLI. + # Runs only on a genuine infra failure (never on a clean review). The + # composite owns the ID-safe marker lifecycle, the fork guard, and the + # no-PR no-op — clobbering an unrelated bot comment is the failure mode + # this whole change exists to fix. - name: Comment on genuine review failure - if: >- - always() && steps.review-outcome.outputs.review_failed == 'true' && - github.event.pull_request.number != '' && - github.event.pull_request.head.repo.full_name == github.repository - # Reporting an outage must never become one. This step only annotates a - # PR, and its API calls can reject for reasons entirely unrelated to the - # code under review — a rejected listComments/createComment would - # otherwise fail the job through github-script's own error handling, - # turning the infra-failure path red without any explicit failure call. + if: always() && steps.review-outcome.outputs.review-failed == 'true' + # Reporting an outage must never become one: this step only annotates + # a PR, and a rejected API call must not turn the infra-failure path + # red (see the composite's description). continue-on-error: true - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} - REVIEW_DETAIL: ${{ steps.review-outcome.outputs.review_detail }} - REVIEW_CLASS: ${{ steps.review-outcome.outputs.failure_class }} + uses: melodic-software/ci-workflows/.github/actions/claude-lane-marker-comment@b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd # b5d54bf 2026-07-27 with: - script: | - const marker = ""; - const body = [ - marker, - "> [!WARNING]", - "> **Automated review did not complete** — this is an infrastructure failure, not a review verdict.", - ">", - "> Treat any Claude comment on this PR (including a placeholder like \"I'll analyze this and get back to you\") as incomplete, not \"no findings.\"", - ">", - `> - Run: ${process.env.RUN_URL}`, - `> - Failure class: \`${process.env.REVIEW_CLASS}\``, - `> - Last SDK result: \`${process.env.REVIEW_DETAIL}\``, - ">", - "> Re-running the job, or pushing a new commit, will retry the review.", - ].join("\n"); - - const comments = await github.paginate(github.rest.issues.listComments, { - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.payload.pull_request.number, - }); - const marked = comments.filter( - (comment) => comment.user?.login === "github-actions[bot]" && comment.body?.startsWith(marker), - ); - const existing = marked.length > 0 ? marked[marked.length - 1] : undefined; - - if (existing) { - await github.rest.issues.updateComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: existing.id, - body, - }); - } else { - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.payload.pull_request.number, - body, - }); - } + marker: "" + mode: post-failure + failure-class: ${{ steps.review-outcome.outputs.failure-class }} + review-detail: ${{ steps.review-outcome.outputs.review-detail }} # 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, and the failure-only condition previously meant no later code path - # ever touched it again. Once a retry (rerun or new push) succeeds, clear - # that stale warning so it is not mistaken for a still-open problem. - # Same fork guard and marker lookup as the failure step — never - # --edit-last, for the same clobbering risk. - # actions/github-script rather than a `gh api` run step, for the same - # runner-tool-availability reason as the failure-comment step above. + # PR. Once a retry (rerun or new push) succeeds, clear that stale + # warning so it is not mistaken for a still-open problem. - name: Clear stale failure comment after successful review - if: >- - always() && steps.review-outcome.outputs.review_failed == 'false' && - github.event.pull_request.number != '' && - github.event.pull_request.head.repo.full_name == github.repository - # Symmetric to the failure step above: tidying a stale comment is - # housekeeping, and a rejected API call during it must not turn a - # successful review red. + if: always() && steps.review-outcome.outputs.review-failed == 'false' + # Tidying a stale comment is housekeeping, and a rejected API call + # during it must not turn a successful review red. continue-on-error: true - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + uses: melodic-software/ci-workflows/.github/actions/claude-lane-marker-comment@b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd # b5d54bf 2026-07-27 with: - script: | - const marker = ""; - const comments = await github.paginate(github.rest.issues.listComments, { - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.payload.pull_request.number, - }); - const marked = comments.filter( - (comment) => comment.user?.login === "github-actions[bot]" && comment.body?.startsWith(marker), - ); - const existing = marked.length > 0 ? marked[marked.length - 1] : undefined; - if (existing) { - await github.rest.issues.deleteComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: existing.id, - }); - } + marker: "" + mode: clear-on-success # zizmor `artipacked` mitigation — scrub the GITHUB_TOKEN that checkout # persisted into .git/config (forced on by #1236, above). Drop this once diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml index 6085aa61..63310157 100644 --- a/.github/workflows/claude-security-review.yml +++ b/.github/workflows/claude-security-review.yml @@ -27,7 +27,8 @@ name: claude-security-review # Fork PRs and non-PR events (`merge_group`, `workflow_dispatch`, `schedule`) # cannot run the review at all — no secrets, or an event the action rejects — # so they keep the pass-through and their green is NOT execution evidence. -# See the fork bullet below and the guard in `Report review outcome`. +# See the fork bullet below and the `Fail closed on an in-scope non-run` +# step. # # The merge-queue half of that carve-out is TRACKED, not accepted forever: # melodic-software/ci-workflows#272, blocked on claude-code-action supporting @@ -362,7 +363,7 @@ jobs: # privileged triggers` could hard-fail it, turning a consumer's dangerous # misconfiguration into a green skipped check. Every non-pull_request event # must reach that tripwire; the ones it allows through then take the - # pass-through path in `Report review outcome`. + # pass-through path past `Fail closed on an in-scope non-run`. if: >- ${{ !cancelled() && needs.changes.outputs.relevant != 'false' @@ -406,46 +407,14 @@ jobs: # A stale run must retire itself, not rely on being cancelled: with the # per-head concurrency key above, nothing cancels a run whose head was - # superseded while it sat in a queue. Ask the live API whether this - # event's head is still the PR's head; when it is not, a newer run owns - # the review, so every runner-consuming step below skips (posting - # feedback for an obsolete head is worse than doing nothing, and the - # newer run also owns the marker-comment lifecycle). Fails open — the - # `!= 'true'` step gates treat a missing output as fresh, and only a - # response shaped like a commit SHA may retire the run (a degenerate - # success such as jq's literal `null` must not) — so an API anomaly - # degrades to reviewing a possibly-stale head, never to silence. - # actions/github-script rather than a `gh api` run step: this reusable - # runs on whatever runner the caller selected, and a self-hosted image - # is not guaranteed to ship the gh CLI. github-script executes on the - # action's own bundled Node runtime, so the live freshness check has no - # runner-image dependency (same pattern as do-not-merge-gate.yml). + # superseded while it sat in a queue. The composite owns the live-head + # check and its fail-open contract — downstream `!= 'true'` gates treat + # a missing output as fresh. Pinned to this repo's own merged SHA: a + # local ./ ref resolves against the CALLER's checkout in a reusable + # workflow and cannot work. - name: Check whether this head is still current id: freshness - if: github.event.pull_request.number != '' - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - with: - script: | - let liveHead = ""; - try { - const { data: pr } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: context.payload.pull_request.number, - }); - liveHead = pr.head.sha; - } catch (error) { - core.warning(`Could not refetch the PR head; treating this run as fresh: ${error.message}`); - } - const eventHeadSha = process.env.EVENT_HEAD_SHA ?? ""; - if (/^[0-9a-f]{40}$/.test(liveHead) && liveHead !== eventHeadSha) { - core.notice(`Skipping review: the PR head moved from ${eventHeadSha} to ${liveHead}, so a newer run owns the review.`); - core.setOutput("superseded", "true"); - } else { - core.setOutput("superseded", "false"); - } + uses: melodic-software/ci-workflows/.github/actions/claude-lane-freshness@b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd # b5d54bf 2026-07-27 # persist-credentials stays at its default (true) until upstream # claude-code-action#1236 ships: the action's setupBranch runs @@ -581,289 +550,92 @@ jobs: } >>"$GITHUB_OUTPUT" # Also gated on the guard: a superseded run has no outcome to report, and - # skipping this step leaves review_failed empty so both marker-comment - # steps below skip too — a retired run must not touch the newer run's PR - # comment state. + # skipping this step leaves review-failed empty so the fail-closed step + # and both marker-comment steps below skip too — a retired run must not + # touch the newer run's PR comment state. The composite owns the + # classification (classify.cjs — the sole reader of the execution file's + # free-text fields) and the machine-readable `class=` annotation; + # it records the failure WITHOUT failing, so the fail-closed step below + # owns the red. - name: Report review outcome id: review-outcome if: always() && steps.freshness.outputs.superseded != 'true' - env: - REVIEW_OUTCOME: ${{ steps.attempt.outputs.outcome }} - EXECUTION_FILE: ${{ steps.attempt.outputs.execution_file }} - IS_PULL_REQUEST: ${{ github.event.pull_request.number != '' }} + uses: melodic-software/ci-workflows/.github/actions/claude-lane-outcome@b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd # b5d54bf 2026-07-27 + with: + outcome: ${{ steps.attempt.outputs.outcome }} + execution-file: ${{ steps.attempt.outputs.execution_file }} + lane: Claude security review + + # FAIL CLOSED (#266). Failure is the only conclusion a required check + # does not accept — `neutral` and `skipped` both read as success, so + # neither can express "in scope and did not run". The composite above + # already wrote the class and detail outputs, and both marker-comment + # steps below are `always()`-gated, so the PR still gets its + # infra-status comment. + # + # Only a pull_request run gates a merge, so only a pull_request run + # fails closed. The action rejects other events outright — `merge_group` + # is not a supported event type at all, and `track_progress` (hardcoded + # on above) rejects every non-PR event — so those runs fail for a reason + # the head cannot fix and no retry can clear. Reddening them would wedge + # a consumer's merge queue permanently, and the comment steps below are + # PR-gated too, so the red would carry no explanation. They keep the + # historical pass-through: still annotated, still classified, not fatal. + # + # Reached only when the review was in scope, the head is still current, + # and both attempts failed. The three legitimate no-verdict paths never + # arrive here: out-of-scope and fork/skip-actor PRs skip the whole job, + # and a superseded head skips the outcome step above, leaving + # review-failed unset — which skips this step too. + - name: Fail closed on an in-scope non-run + if: >- + always() && steps.review-outcome.outputs.review-failed == 'true' && + github.event.pull_request.number != '' run: | - if [ "$REVIEW_OUTCOME" = "success" ]; then - echo "Claude security review completed. Findings (if any) are in the PR comments." - echo "review_failed=false" >> "$GITHUB_OUTPUT" - exit 0 - fi - - # The raw SDKResultMessage is NOT safe to publish as-is on this public - # repo — the same leak risk that keeps display_report and - # show_full_output off (CLAUDE.md). The embedded block below is the - # sole reader of its free-text fields; its header carries the - # projection and classification contract. - # BEGIN GENERATED CLASSIFY INFRA FAILURE - DO NOT EDIT - # Source: .github/scripts/classify-infra-failure.sh - # Project the last claude-code-action SDK result message into safe, structured - # metadata and classify a genuine infrastructure failure into exactly one coarse - # class: auth | rate-limit | overloaded | other. Reads EXECUTION_FILE (the - # action's execution_file output, which may be unset, missing, or unparsable) - # and sets review_detail plus failure_class as shell variables and step outputs. - # - # This runs on a PUBLIC repo. The result message's `result` field is - # model-authored free text and its `errors[]` entries are raw error stacks; both - # are read INSIDE this block only, and nothing but the class token and the - # structured projection below ever leaves it. Never widen the projection to - # either field, and never echo the result message wholesale. - # - # Classification order: - # 1. `api_error_status` — the Anthropic-API HTTP status the SDK records when - # the last assistant turn was an API error: 401/402/403 auth, 429 - # rate-limit, 5xx overloaded, every other status other. - # 2. an allowlisted substring over `errors[]`, matching the serialized - # Anthropic error body (`{"type":"error","error":{"type":"",...}}`) - # that the API SDK folds into an APIError's message, and so into its stack. - # 3. other. - # - # The allowlist mirrors the status mapping type-for-type, so a payload that - # recovers no numeric status still lands in the same class the status would - # have chosen. Against the published error table that is: `authentication_error` - # (401), `billing_error` (402), `permission_error` (403) -> auth; - # `rate_limit_error` (429) -> rate-limit; `api_error` (500), `timeout_error` - # (504), `overloaded_error` (529) -> overloaded. The remaining published types - # — `invalid_request_error` (400), `not_found_error` (404), `conflict_error` - # (409), `request_too_large` (413) — are client errors, not infrastructure, and - # are deliberately left to `other` on both paths. Adding a type to one path - # without the other reopens the asymmetry this note exists to prevent. - # - # `auth` is the credential-unusable class, not the 401-only class: it covers - # every status where the credential cannot be used until a human acts. Per the - # published error table that is 401 `authentication_error`, 402 `billing_error`, - # and 403 `permission_error`. 402 belongs here because the incident this - # classifier exists for (ci-workflows#228 / claude-code-plugins#1122) is - # described as a *usage-dead account credential* — a billing/entitlement death, - # which the API reports as 402, not 401. Leaving 402 in `other` would classify - # the originating incident as unremarkable. The downstream aggregator routes - # `auth` to a human-gated incident, correct for a billing death as much as for - # a revocation. - # - # The two sources are disjoint members of the SDK's result union: - # `api_error_status` exists only on the success variant (the shape a dead - # credential produces — subtype success, is_error true) and `errors[]` only on - # the error subtypes. So the substring pass is the error-variant path, not a - # second look at the same payload, and a present-but-unmapped status stays a - # status decision rather than falling through to it. - : "${GITHUB_OUTPUT:?GITHUB_OUTPUT is required}" - - review_detail="(no execution file was produced)" - failure_class="other" - - if [[ -n "${EXECUTION_FILE:-}" && -f "$EXECUTION_FILE" ]]; then - review_detail=$(jq -c ' - (.[-1] // empty) as $last - | if $last == null then empty - else - $last.api_error_status as $status - | (($last.errors // []) | map(tostring) | join("\n")) as $text - | ( - if ($status | type) == "number" then - if $status == 401 or $status == 402 or $status == 403 then "auth" - elif $status == 429 then "rate-limit" - elif $status >= 500 and $status < 600 then "overloaded" - else "other" - end - elif ($text | (contains("\"type\":\"authentication_error\"") - or contains("\"type\":\"billing_error\"") - or contains("\"type\":\"permission_error\""))) then "auth" - elif ($text | contains("\"type\":\"rate_limit_error\"")) then "rate-limit" - elif ($text | (contains("\"type\":\"overloaded_error\"") - or contains("\"type\":\"timeout_error\"") - or contains("\"type\":\"api_error\""))) then "overloaded" - else "other" - end - ) as $class - | { - subtype: $last.subtype, - is_error: $last.is_error, - num_turns: $last.num_turns, - duration_ms: $last.duration_ms, - total_cost_usd: $last.total_cost_usd, - api_error_status: $status, - class: $class - } - end - ' "$EXECUTION_FILE" 2>/dev/null || true) - if [[ -n "$review_detail" ]]; then - failure_class=$(jq -r '.class // "other"' <<<"$review_detail" 2>/dev/null || true) - [[ -n "$failure_class" ]] || failure_class="other" - else - review_detail="(execution file present but unparsable)" - fi - fi - - { - echo "failure_class=$failure_class" - echo "review_detail=$review_detail" - } >>"$GITHUB_OUTPUT" - # END GENERATED CLASSIFY INFRA FAILURE - - # `class=` is a machine-readable term, not prose: this - # annotation is the detection surface a receiver polls through the - # check-run API, so keep it bare and greppable. - echo "::error::Claude security review exited with: $REVIEW_OUTCOME class=$failure_class" \ - "(infrastructure error, not a security signal — e.g. usage limit," \ - "OIDC failure, SDK crash, is_error result, or max-turns exhaustion)." \ - "Last SDK result: $review_detail" - - echo "review_failed=true" >> "$GITHUB_OUTPUT" - - # Only a pull_request run gates a merge, so only a pull_request run - # fails closed. The action rejects other events outright — `merge_group` - # is not a supported event type at all, and `track_progress` (hardcoded - # on above) rejects every non-PR event — so those runs fail for a reason - # the head cannot fix and no retry can clear. Reddening them would wedge - # a consumer's merge queue permanently, and the comment steps below are - # PR-gated too, so the red would carry no explanation. They keep the - # historical pass-through: still annotated, still classified, not fatal. - # Tested for the PASS-THROUGH value, not the fail-closed one, so the - # default is closed: if this variable is ever unset or garbled — a - # deleted env line, a renamed step — the comparison is false and the - # run still fails closed. Written as `!= "true"` it would do the - # opposite, silently reverting #266 on every event. - if [ "$IS_PULL_REQUEST" = "false" ]; then - echo "Not a pull_request run; reporting the failure without failing the job." - exit 0 - fi - - # FAIL CLOSED (#266). Ordering is load-bearing: review_failed and the - # class are written to GITHUB_OUTPUT above, before this exit, so the - # two `always()`-gated comment steps below still fire and the PR still - # gets its infra-status comment. Exiting here is what makes the check - # report FAILURE, and failure is the only conclusion a required check - # does not accept — `neutral` and `skipped` both read as success, so - # neither can express "in scope and did not run". - # - # Reached only when the review was in scope, the head is still current, - # and both attempts failed. The three legitimate no-verdict paths never - # arrive here: out-of-scope and fork/skip-actor PRs skip the whole job, - # and a superseded head skips this step (leaving review_failed unset). + echo "The security review was in scope and did not run; the required check cannot report success (see POSTURE)." exit 1 - # Runs only on a genuine infra failure (never on a clean review) and only - # when the token can write: fork-triggered pull_request runs get a - # read-only GITHUB_TOKEN and no secrets by design (CLAUDE.md), so - # gh pr comment would just fail there — skip rather than error. - # Targets the marker comment by ID (never --edit-last, which edits the - # PR's last comment from ANY author) so a stuck placeholder comment from - # track_progress is never mistaken for "no findings" and an unrelated - # bot comment is never clobbered. - # actions/github-script rather than `gh api`/`gh pr comment` run steps: - # this reusable runs on whatever runner the caller selected, and a - # self-hosted image is not guaranteed to ship the gh CLI. + # Runs only on a genuine infra failure (never on a clean review). The + # composite owns the ID-safe marker lifecycle, the fork guard, and the + # no-PR no-op — clobbering an unrelated bot comment is the failure mode + # the marker-by-ID contract exists to prevent. The body-copy lines carry + # their own `> ` blockquote-continuation prefixes (the composite + # prefixes only the first line). - name: Comment on genuine review failure - if: >- - always() && steps.review-outcome.outputs.review_failed == 'true' && - github.event.pull_request.number != '' && - github.event.pull_request.head.repo.full_name == github.repository - # Reporting an outage must never become one. This step only annotates a - # PR, and its API calls can reject for reasons entirely unrelated to the - # code under review — a rejected listComments/createComment would - # otherwise fail the job through github-script's own error handling, - # turning the infra-failure path red without any explicit failure call. + if: always() && steps.review-outcome.outputs.review-failed == 'true' + # Reporting an outage must never become one: this step only annotates + # a PR, and a rejected API call must not fail the job on top of the + # deliberate red the fail-closed step already raised. continue-on-error: true - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - env: - RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} - REVIEW_DETAIL: ${{ steps.review-outcome.outputs.review_detail }} - REVIEW_CLASS: ${{ steps.review-outcome.outputs.failure_class }} + uses: melodic-software/ci-workflows/.github/actions/claude-lane-marker-comment@b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd # b5d54bf 2026-07-27 with: - script: | - const marker = ""; - const body = [ - marker, - "> [!WARNING]", - "> **Automated security review did not complete** — this is an infrastructure failure, not a review verdict.", - ">", - "> Treat any Claude comment on this PR (including a placeholder like \"I'll analyze this and get back to you\") as incomplete, not \"no findings.\"", - ">", - `> - Run: ${process.env.RUN_URL}`, - `> - Failure class: \`${process.env.REVIEW_CLASS}\``, - `> - Last SDK result: \`${process.env.REVIEW_DETAIL}\``, - ">", - "> **The check is red on purpose.** It certifies that a security pass ran, and this one did not complete, so it cannot report success. Where this check is required, merging is blocked until a review actually finishes.", - ">", - "> Re-running the job, or pushing a new commit, will retry the review — one automatic retry already ran.", - ">", - "> Re-running does NOT help for every class:", - "> - `rate-limit` that persists across re-runs, or `auth` — the credential or usage budget needs an **operator**; retrying will not clear it.", - "> - a run that exhausted its turn budget (`\"subtype\":\"error_max_turns\"` above) will exhaust it again. **As the PR author**, split the change into smaller PRs; raising `--max-turns` is a change to the *caller workflow*, not something you can set on this PR.", - ].join("\n"); - - const comments = await github.paginate(github.rest.issues.listComments, { - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.payload.pull_request.number, - }); - const marked = comments.filter( - (comment) => comment.user?.login === "github-actions[bot]" && comment.body?.startsWith(marker), - ); - const existing = marked.length > 0 ? marked[marked.length - 1] : undefined; - - if (existing) { - await github.rest.issues.updateComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: existing.id, - body, - }); - } else { - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.payload.pull_request.number, - body, - }); - } + marker: "" + mode: post-failure + headline: Automated security review did not complete + failure-class: ${{ steps.review-outcome.outputs.failure-class }} + review-detail: ${{ steps.review-outcome.outputs.review-detail }} + body-copy: |- + **The check is red on purpose.** It certifies that a security pass ran, and this one did not complete, so it cannot report success. Where this check is required, merging is blocked until a review actually finishes. + > + > Re-running the job, or pushing a new commit, will retry the review — one automatic retry already ran. + > + > Re-running does NOT help for every class: + > - `rate-limit` that persists across re-runs, or `auth` — the credential or usage budget needs an **operator**; retrying will not clear it. + > - a run that exhausted its turn budget (`"subtype":"error_max_turns"` above) will exhaust it again. **As the PR author**, split the change into smaller PRs; raising `--max-turns` is a change to the *caller workflow*, not something you can set on this PR. # 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, and the failure-only condition previously meant no later code path - # ever touched it again. Once a retry (rerun or new push) succeeds, clear - # that stale warning so it is not mistaken for a still-open problem. - # Same fork guard and marker lookup as the failure step — never - # --edit-last, for the same clobbering risk. - # actions/github-script rather than a `gh api` run step, for the same - # runner-tool-availability reason as the failure-comment step above. + # PR. Once a retry (rerun or new push) succeeds, clear that stale + # warning so it is not mistaken for a still-open problem. - name: Clear stale failure comment after successful review - if: >- - always() && steps.review-outcome.outputs.review_failed == 'false' && - github.event.pull_request.number != '' && - github.event.pull_request.head.repo.full_name == github.repository - # Symmetric to the failure step above: tidying a stale comment is - # housekeeping, and a rejected API call during it must not turn a - # successful review red. + if: always() && steps.review-outcome.outputs.review-failed == 'false' + # Tidying a stale comment is housekeeping, and a rejected API call + # during it must not turn a successful review red. continue-on-error: true - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + uses: melodic-software/ci-workflows/.github/actions/claude-lane-marker-comment@b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd # b5d54bf 2026-07-27 with: - script: | - const marker = ""; - const comments = await github.paginate(github.rest.issues.listComments, { - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.payload.pull_request.number, - }); - const marked = comments.filter( - (comment) => comment.user?.login === "github-actions[bot]" && comment.body?.startsWith(marker), - ); - const existing = marked.length > 0 ? marked[marked.length - 1] : undefined; - if (existing) { - await github.rest.issues.deleteComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: existing.id, - }); - } + marker: "" + mode: clear-on-success # zizmor `artipacked` mitigation — scrub the GITHUB_TOKEN that checkout # persisted into .git/config (forced on by #1236, above). Drop this once diff --git a/.github/workflows/selector-conformance.yml b/.github/workflows/selector-conformance.yml index 69c1b99d..fda0105f 100644 --- a/.github/workflows/selector-conformance.yml +++ b/.github/workflows/selector-conformance.yml @@ -13,10 +13,6 @@ on: - .github/workflows/claude-review.yml - .github/workflows/claude-security-review.yml - .github/osv-scanner-pin.json - - .github/scripts/classify-infra-failure.sh - - .github/scripts/classify-infra-failure.test.sh - - .github/scripts/classify-infra-failure-render.test.cjs - - .github/scripts/render-classify-infra-failure.cjs - .github/scripts/find-tracking-issue-render.test.cjs - .github/scripts/find-tracking-issue.sh - .github/scripts/find-tracking-issue.test.sh @@ -44,10 +40,6 @@ on: - .github/workflows/claude-review.yml - .github/workflows/claude-security-review.yml - .github/osv-scanner-pin.json - - .github/scripts/classify-infra-failure.sh - - .github/scripts/classify-infra-failure.test.sh - - .github/scripts/classify-infra-failure-render.test.cjs - - .github/scripts/render-classify-infra-failure.cjs - .github/scripts/find-tracking-issue-render.test.cjs - .github/scripts/find-tracking-issue.sh - .github/scripts/find-tracking-issue.test.sh @@ -96,8 +88,6 @@ jobs: run: node --test .github/scripts/*.test.cjs - name: Test tracking-issue resolver run: bash .github/scripts/find-tracking-issue.test.sh - - name: Test infra-failure classification - run: bash .github/scripts/classify-infra-failure.test.sh - name: Test OSV scanner exit-code policy run: bash .github/scripts/osv-scan-guard.test.sh - name: Test OSV release digest parsing diff --git a/docs/topics/claude-review-lanes/PLAN.md b/docs/topics/claude-review-lanes/PLAN.md index a758e60e..dc9f048b 100644 --- a/docs/topics/claude-review-lanes/PLAN.md +++ b/docs/topics/claude-review-lanes/PLAN.md @@ -297,7 +297,7 @@ runs at `https://github.com/melodic-software/ci-workflows/actions/runs/`):** 1.7.12. Shipped as `.github/actionlint.yaml` with justification + removal trigger (rhysd/actionlint#654); fleet distribution deferred to Phase 3c0. -### Phase 1: standards — REVIEW.md restructure + REVIEW-CREDENTIAL re-derivation [DOING] +### Phase 1: standards — REVIEW.md restructure + REVIEW-CREDENTIAL re-derivation [DONE] One PR to melodic-software/standards. Closes standards#264. Runs in parallel with Phase 0 (disjoint repos, no dependency). @@ -330,7 +330,25 @@ claude-code-plugins reclassification; standards#264 closed by the merge; five downstream sync PRs opened and (before 2b merges) merged — verify via `gh pr list --repo melodic-software/ --search review-instructions`. -### Phase 2: ci-workflows change set [TODO] +**Phase 1 evidence (2026-07-27):** standards#278 merged as `abeccc6` +(2026-07-27T00:03Z); #264 auto-closed by the merge. Post-merge greps on +standards main: `grep -c "Do not report" REVIEW.md` == 0; `grep -c "Cap the +nits" REVIEW.md` == 0; lane headings present (`## Code-review lane scope` +:62, `## Security lane scope` :88), each with an explicit mutual-exclusion +line ("does **not** report security findings … omitted here even when a +hunk plainly contains one" / "Everything else … is out of scope here and is +owned by the code-review lane; omit it"); `grep -ci public +distribution/REVIEW-CREDENTIAL.md` == 27 (public-state re-derivation +present). Three Codex review threads were fixed in `18bda17` before merge: +the mount prohibition now conditions on private content (with +`conventions/README.md` reconciled), the retained credential classification +is explicitly historical, and `ai-review-bot-composition.md`'s governed +baseline is the two scope-split lanes. Five `review-instructions` sync PRs +opened: ci-workflows#275, claude-code-plugins#1643, github-iac#230, +dotfiles#344, provisioning#221 — automerge armed; all five must be MERGED +before 2b merges (the 2b gate). + +### Phase 2: ci-workflows change set [DOING] After Phases 0 + 1 (2b additionally gated on the five sync PRs). Delivery: **PR-A1** (composites land, unreferenced), **PR-A2** (reusables repoint at the @@ -357,6 +375,25 @@ merged SHA — self-reference pins resolvable only post-merge), **PR-B** (featur (precedent: render-classify-infra-failure.cjs:9-11). - Update `claude-review-superseded-guard.test.cjs` deliberately (7-occurrence tripwire changes shape). +- **2a-addendum (PR-A2 execution record, 2026-07-27):** PR-A1 merged as + `b5d54bf7cb386b1f2c35426c6c5fb8d1686671bd`; composites pinned at that SHA. + #266's fail-closed change landed between plan approval and the repoint, so + the security lane's outcome step was not a pure embed swap: the composite + records the failure without failing, and a new inline `Fail closed on an + in-scope non-run` step owns the required-check red — the same + pull_request-only carve-out, expressed as a step condition + (`review-failed == 'true' && github.event.pull_request.number != ''`). + `claude-security-review-fail-closed.test.cjs` re-pins the wiring shape + (resolve step, composite inputs, fail-closed condition); the + classification behavior corpus lives in + `.github/actions/claude-lane-outcome/classify.test.cjs`. The security + lane's expanded marker body rides the composite's `body-copy` as + blockquote-continuation lines (input description updated, docs-only). + `selector-conformance.yml` also dropped its classifier path triggers + + test step — a consumer of the retired files the plan had not listed. + Known cosmetic delta until 2g: the composite annotation says "not a + code-quality signal" on both lanes (the security lane previously said + "not a security signal"); align the wording during the 2g copy rewrite. - **2b. Config currency (all three lanes):** pin bump → v1.0.183 (full SHA + `# v1.0.183`); `--model claude-sonnet-5` defaults (no `--effort`); `exclude_comments_by_actor: dependabot,dependabot[bot]` + upstream-#1514 comment