diff --git a/.github/claude-skip-actors b/.github/claude-skip-actors new file mode 100644 index 0000000000..bfd2f45246 --- /dev/null +++ b/.github/claude-skip-actors @@ -0,0 +1,25 @@ +# Repo-owned skip-actors list: ADR 0002's ratified skip-actor exception — the +# actors for whom the claude-review and claude-security-review lanes skip +# entirely. One actor login per line; `#` comments and blank lines are +# ignored. scripts/read-skip-actors.sh joins the entries into the +# comma-separated form the ci-workflows reusable workflows take, and it is the +# ONLY statement of the list: both caller workflows read it through that +# script, and both evidence guards (verify-claude-review-skill.sh, +# verify-security-review-evidence.sh) default from it. The list used to be +# restated verbatim at five sites, and the restatement drifted exactly as +# restatements do — 2b4d8abf added cursor[bot] to both workflow lines and left +# verify-security-review-evidence.sh's copy behind. +# +# ADDING an actor here admits it to a PRIVILEGED skip on security-sensitive +# paths and is an ADR 0002 decision, not a config tweak. REMOVING an actor is +# never a one-line change either: the security lane fails CLOSED for an actor +# the reusable's `allowed_bots` does not permit, so un-skipping for real means +# widening `allowed_bots` upstream first (ADR 0002, "skip-actors and the +# action's allowed_bots are different levers"). Removing `dependabot[bot]` +# additionally requires mirroring CLAUDE_CODE_OAUTH_TOKEN into the separate +# Dependabot secrets store — see the reusable's `skip-actors` input docs. +dependabot[bot] +claude[bot] +melodic-ai[bot] +melodic-standards-sync[bot] +cursor[bot] diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index a8f59e992b..8efdff707b 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -64,6 +64,19 @@ jobs: persist-credentials: false - name: Run claude-review skill-evidence guard tests run: bash scripts/verify-claude-review-skill.sh.test.sh + # The skip-actors exception lives in .github/claude-skip-actors — the one + # statement of ADR 0002's ratified list, read through its one parser. The + # value used to be restated inline here (and at four sibling sites), and + # the restatement drifted (2b4d8abf). + # Plain assignment first, never inside the echo: a command substitution + # in echo's argument returns ECHO's status under bash -e, so a parser + # failure would emit `list=` and the step would stay green. The + # assignment form fails the step the moment the read fails. + - name: Read the ratified skip-actors list + id: skip-actors + run: | + list="$(scripts/read-skip-actors.sh)" + echo "list=$list" >> "$GITHUB_OUTPUT" - name: Verify claude-review invoked the skill env: GH_TOKEN: ${{ github.token }} @@ -74,5 +87,5 @@ jobs: EVENT_HEAD_SHA: ${{ github.event.pull_request.head.sha }} LANE_RESULT: ${{ needs.review.result }} REVIEWER_LOGINS: claude[bot] - SKIP_ACTORS: dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot],cursor[bot] + SKIP_ACTORS: ${{ steps.skip-actors.outputs.list }} run: bash scripts/verify-claude-review-skill.sh diff --git a/.github/workflows/claude-security-review.yml b/.github/workflows/claude-security-review.yml index 239eccf704..f2b151c796 100644 --- a/.github/workflows/claude-security-review.yml +++ b/.github/workflows/claude-security-review.yml @@ -26,7 +26,64 @@ concurrency: cancel-in-progress: false jobs: + # The ratified skip-actors list lives in .github/claude-skip-actors — the + # one statement of ADR 0002's skip-actor exception, formerly restated at + # five sites (which drifted: 2b4d8abf). A workflow-call `with:` value cannot + # read a file, so this job reads the list and hands it across `needs`. + # + # It reads the PR's BASE copy of the list, the same discipline as the + # reusable's `paths-file` — and that is CONTENT protection only. On a + # pull_request event this workflow file, scripts/read-skip-actors.sh, and + # the checkout-with-base composite all come from the PR HEAD, so a head + # that rewrites the parser can still feed the lane a different list. That + # is the pre-existing trust boundary, not a regression (the literal used to + # live in this same head-controlled file), and it stays bounded by the + # reusable's allowed_bots gate on who can trigger a privileged run; the + # base read closes the cheap one-line list edit, not a determined head + # rewrite. A base that predates the file falls back to the head copy (the + # adopting PR itself), announced with a ::notice rather than silently. A + # failed read here skips the lane, which a ruleset would read as success — + # the evidence job below fails closed on that shape. + skip-actors: + runs-on: ubuntu-24.04 + timeout-minutes: 5 + outputs: + list: ${{ steps.read.outputs.list }} + steps: + - name: Check out + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - name: Fetch base + uses: ./.github/actions/checkout-with-base + # Each fallback is its own announced branch, never a silent 2>/dev/null + # catch-all: an unresolvable base ref is a hard failure (on pull_request + # the composite above fetched it, so its absence means something is + # broken, not merely pre-adoption), while the list file absent at a + # resolvable base is the one legitimate head-copy fallback — the + # adopting PR. The parser reads are plain assignments so a read failure + # fails the step under bash -e. + - name: Read the ratified skip-actors list + id: read + env: + BASE_REF: ${{ github.base_ref }} + run: | + src=".github/claude-skip-actors" + if [ -z "$BASE_REF" ] || ! git rev-parse --verify --quiet "origin/$BASE_REF" >/dev/null; then + echo "::error::base ref '${BASE_REF:-}' is unavailable; refusing to guess which skip-actors list applies" + exit 1 + fi + if git cat-file -e "origin/$BASE_REF:$src" 2>/dev/null; then + git show "origin/$BASE_REF:$src" >"$RUNNER_TEMP/claude-skip-actors" + list="$(scripts/read-skip-actors.sh "$RUNNER_TEMP/claude-skip-actors")" + else + echo "::notice::$src absent at origin/$BASE_REF (pre-adoption base); reading the head copy" + list="$(scripts/read-skip-actors.sh)" + fi + echo "list=$list" >> "$GITHUB_OUTPUT" + security-review: + needs: skip-actors permissions: contents: read # checkout + read the diff pull-requests: write # post the security review @@ -37,14 +94,16 @@ jobs: paths-file: .github/claude-security-paths # Stated explicitly, never left to the reusable's default: this list IS # ADR 0002's skip-actor exception, and the exception must be readable and - # reviewable in the repo it applies to. An inherited default silently - # rewrites the exception whenever ci-workflows changes it — which is how - # `claude[bot]` and `melodic-ai[bot]` entered it (#1766 dropped this line - # while re-pinning to a version whose default had widened). + # reviewable in the repo it applies to — it lives in + # .github/claude-skip-actors, read by the `skip-actors` job above. An + # inherited default silently rewrites the exception whenever ci-workflows + # changes it — which is how `claude[bot]` and `melodic-ai[bot]` entered + # it (#1766 dropped this line while re-pinning to a version whose default + # had widened). # # DO NOT delete this line at a re-pin, and do not "simplify" it away. # Deleting it is not a no-op even when the value matches the upstream - # default of the moment: the four actors here are ADR 0002's 2026-08-04 + # default of the moment: the actors in that file are ADR 0002's # ratified baseline, and inheriting instead re-delegates that decision to # whatever ci-workflows ships next. Every actor in an inherited default # satisfies this repo's REQUIRED `security-review / security-review` @@ -64,7 +123,7 @@ jobs: # not. runner-policy rejects inputs outside the reviewed contract but # cannot REQUIRE one, so dropping this line re-widens the exception with # nothing said. That residual gap is standards#308. - skip-actors: dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot],cursor[bot] + skip-actors: ${{ needs.skip-actors.outputs.list }} # One named secret (least privilege), never `secrets: inherit`. secrets: CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} @@ -86,7 +145,7 @@ jobs: # pin carries the outputs needs a version they can compare against, and the # guard's own message says the same thing the same way. security-review-evidence: - needs: security-review + needs: [skip-actors, security-review] if: >- always() && github.event_name == 'pull_request' && @@ -104,6 +163,16 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false + # A failed skip-actors read skips the lane, and a ruleset reads a skipped + # required check as success — so this supplement reddens that shape + # rather than letting a broken read double as an off switch. + - name: Fail closed when the skip-actors read failed + if: needs.skip-actors.result != 'success' + env: + SKIP_ACTORS_RESULT: ${{ needs.skip-actors.result }} + run: | + echo "::error::the skip-actors read job concluded '$SKIP_ACTORS_RESULT'; the security lane cannot have received the ratified list" + exit 1 # Self-test first, so a broken guard cannot mask the regression it exists # to catch — the shape #2517 was: the guard agreed with itself while # failing every pull request. @@ -122,5 +191,5 @@ jobs: LANE_REVIEW_RAN: ${{ needs.security-review.outputs.review-ran }} LANE_REVIEW_FAILED: ${{ needs.security-review.outputs.review-failed }} LANE_FAILURE_CLASS: ${{ needs.security-review.outputs.failure-class }} - SKIP_ACTORS: dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot],cursor[bot] + SKIP_ACTORS: ${{ needs.skip-actors.outputs.list }} run: bash scripts/verify-security-review-evidence.sh diff --git a/scripts/read-skip-actors.sh b/scripts/read-skip-actors.sh new file mode 100755 index 0000000000..04bda61a9f --- /dev/null +++ b/scripts/read-skip-actors.sh @@ -0,0 +1,54 @@ +#!/usr/bin/env bash +# Print the ratified review skip-actors list in the comma-separated, no-spaces +# form the ci-workflows reusable workflows take. +# +# scripts/read-skip-actors.sh [] +# +# The list itself lives in .github/claude-skip-actors (one actor per line) — +# the single statement of ADR 0002's skip-actor exception; this script owns +# the one parse of it. Consumers: the claude-security-review caller reads the +# lane's `skip-actors` input through it, the claude-review caller reads its +# evidence guard's SKIP_ACTORS through it, and both evidence guards default +# from it when SKIP_ACTORS is not in the environment. overrides the +# committed path so a caller can hand in a copy read from another ref (the +# security caller reads the PR's BASE copy, mirroring the reusable's +# `paths-file` discipline). +# +# FAIL CLOSED ON SHAPE: a missing file, an empty active set, or an entry that +# could corrupt the comma-joined form (embedded comma or whitespace) exits 2 +# with nothing on stdout. Printing an empty or mangled list would silently +# rewrite the exception, which is the drift this file replaced. +# +# Exit: 0 list printed; 2 usage, unreadable file, or a malformed entry. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" || exit 2 +# shellcheck source=lib/read-list.sh +. "$SCRIPT_DIR/lib/read-list.sh" || exit 2 + +if [[ $# -gt 1 ]]; then + echo "usage: $(basename "$0") []" >&2 + exit 2 +fi +file="${1:-$SCRIPT_DIR/../.github/claude-skip-actors}" + +actors=() +# `inline`: entries are actor logins, never regexes, and a login cannot +# contain `#`. +read_list::into actors "$file" --comments inline || exit 2 + +if [[ ${#actors[@]} -eq 0 ]]; then + echo "read-skip-actors: $file names no actors; an empty exception must be an explicit consumer decision, not a parsed-away file" >&2 + exit 2 +fi +for actor in "${actors[@]}"; do + if [[ "$actor" == *[,[:space:]]* ]]; then + echo "read-skip-actors: malformed entry (comma or whitespace) in $file: $actor" >&2 + exit 2 + fi +done + +( + IFS=, + printf '%s\n' "${actors[*]}" +) diff --git a/scripts/read-skip-actors.test.sh b/scripts/read-skip-actors.test.sh new file mode 100755 index 0000000000..63470cbb6f --- /dev/null +++ b/scripts/read-skip-actors.test.sh @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +# Unit tests for read-skip-actors.sh. Synthetic list files exercise the parse +# and every fail-closed shape; one case reads the real committed list so the +# published form stays a live assertion, not an example. +set -uo pipefail + +SELF_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SCRIPT="$SELF_DIR/read-skip-actors.sh" + +# shellcheck source=lib/test-harness.sh +. "$SELF_DIR/lib/test-harness.sh" + +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT + +write_list() { + printf '%s\n' "$@" >"$TMP/list" +} + +# --- comments and blanks are ignored; entries join with commas -------------- +write_list "# ratified" "" "dependabot[bot]" "cursor[bot] # org-installed" "" +if out="$(bash "$SCRIPT" "$TMP/list" 2>&1)" && [[ "$out" == "dependabot[bot],cursor[bot]" ]]; then + ok "joins active entries with commas, dropping comments and blanks" +else + fail "expected 'dependabot[bot],cursor[bot]', got: $out" +fi + +# --- a single entry prints with no trailing comma --------------------------- +write_list "claude[bot]" +if out="$(bash "$SCRIPT" "$TMP/list" 2>&1)" && [[ "$out" == "claude[bot]" ]]; then + ok "a single entry prints bare" +else + fail "expected 'claude[bot]', got: $out" +fi + +# --- fail-closed shapes exit 2 with nothing on stdout ----------------------- +expect_exit_2() { + local label="$1" + shift + local out status=0 + out="$(bash "$SCRIPT" "$@" 2>/dev/null)" || status=$? + if [[ "$status" -eq 2 && -z "$out" ]]; then + ok "$label exits 2 with empty stdout" + else + fail "$label should exit 2 with empty stdout, got status $status, out: $out" + fi +} + +expect_exit_2 "a missing file" "$TMP/no-such-file" +write_list "# only commentary" "" +expect_exit_2 "an empty active set" "$TMP/list" +write_list "dependabot[bot],cursor[bot]" +expect_exit_2 "an embedded comma" "$TMP/list" +write_list "depend abot[bot]" +expect_exit_2 "embedded whitespace" "$TMP/list" +write_list "claude[bot]" +expect_exit_2 "extra arguments" "$TMP/list" surplus + +# --- the committed list parses and carries the ratified five ---------------- +if out="$(bash "$SCRIPT" 2>&1)" && [[ "$out" == *"cursor[bot]"* && "$out" != *" "* ]]; then + ok "the committed .github/claude-skip-actors parses to a spaceless list naming cursor[bot]" +else + fail "committed list should parse and name cursor[bot], got: $out" +fi + +test_harness::report diff --git a/scripts/verify-claude-review-skill.sh b/scripts/verify-claude-review-skill.sh index b79adbea19..a66b7c453a 100755 --- a/scripts/verify-claude-review-skill.sh +++ b/scripts/verify-claude-review-skill.sh @@ -16,7 +16,8 @@ # Environment: # GITHUB_EVENT_NAME pull_request expected; anything else is not applicable # GITHUB_ACTOR PR author login -# SKIP_ACTORS comma-separated actors exempt from review +# SKIP_ACTORS comma-separated actors exempt from review (default: +# .github/claude-skip-actors via read-skip-actors.sh) # LANE_RESULT needs.review.result # GITHUB_REPOSITORY owner/repo # PR_NUMBER pull request number @@ -29,7 +30,13 @@ set -euo pipefail -SKIP_ACTORS="${SKIP_ACTORS:-dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot],cursor[bot]}" +# The default is the ratified list in .github/claude-skip-actors, read through +# its one parser — never a restated literal: the sibling security guard's +# literal drifted behind the workflow lines exactly that way (2b4d8abf added +# cursor[bot] everywhere but there). +if [[ -z "${SKIP_ACTORS:-}" ]]; then + SKIP_ACTORS="$("$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/read-skip-actors.sh")" || exit 2 +fi REVIEWER_LOGINS="${REVIEWER_LOGINS:-claude[bot]}" usage() { diff --git a/scripts/verify-security-review-evidence.sh b/scripts/verify-security-review-evidence.sh index 65359af1e6..648840e23c 100755 --- a/scripts/verify-security-review-evidence.sh +++ b/scripts/verify-security-review-evidence.sh @@ -29,7 +29,8 @@ # Environment: # GITHUB_EVENT_NAME pull_request expected; anything else is not applicable # GITHUB_ACTOR PR author login -# SKIP_ACTORS comma-separated actors exempt from review +# SKIP_ACTORS comma-separated actors exempt from review (default: +# .github/claude-skip-actors via read-skip-actors.sh) # LANE_RESULT needs.security-review.result # LANE_RELEVANT the lane's `relevant` output # LANE_REVIEW_RAN the lane's `review-ran` output @@ -44,7 +45,13 @@ set -euo pipefail -SKIP_ACTORS="${SKIP_ACTORS:-dependabot[bot],claude[bot],melodic-ai[bot],melodic-standards-sync[bot]}" +# The default is the ratified list in .github/claude-skip-actors, read through +# its one parser — never a restated literal: this guard's literal is the copy +# that drifted (2b4d8abf added cursor[bot] to both workflow lines and left it +# out here). +if [[ -z "${SKIP_ACTORS:-}" ]]; then + SKIP_ACTORS="$("$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/read-skip-actors.sh")" || exit 2 +fi usage() { cat <<'EOF'