From f36e7a686dbe8879451242fba055d63e2e230d59 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Tue, 4 Aug 2026 19:35:11 -0700 Subject: [PATCH 1/2] fix(review): grant checks and statuses read for the CI rollup --- .github/workflows/claude-pr-review.yml | 23 +++- .github/workflows/tests.yml | 7 ++ tests/context-step-test.sh | 25 +++-- tests/workflow-lint-test.sh | 149 +++++++++++++++++++++++++ 4 files changed, 191 insertions(+), 13 deletions(-) create mode 100755 tests/workflow-lint-test.sh diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 29668d2..0d60818 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -17,13 +17,26 @@ jobs: contents: read pull-requests: write id-token: write - # actions: read for the failing-job log excerpts, issues: read for the PR - # conversation (/issues/{n}/comments is the PR's own comment thread). The job - # declares permissions explicitly, so anything not listed here is `none` and both - # reads 403 -- silently, into "(log unavailable)" and "Could not read PR - # conversation comments.", since every block in the context step is guarded. + # One line per extra read the context step makes. The job declares permissions + # explicitly, so anything not listed here is `none` and the read fails -- quietly, into + # that block's "could not read" sentence, because every block is guarded. + # + # actions: read -- the failing-job log excerpts (/actions/jobs/{id}/logs) + # issues: read -- the PR conversation (/issues/{n}/comments is the PR's own thread) + # checks: read -- the CheckRun half of statusCheckRollup + # statuses: read -- the StatusContext half of it (Aikido, Terraform Cloud, anything + # posting a commit status rather than a check run) + # + # checks and statuses were both missing on the first production run, and the shape of + # that failure is worth remembering: `gh pr view --json statusCheckRollup` resolves one + # GraphQL field needing both, so it fails the whole rollup with "Resource not accessible + # by integration" rather than omitting the half it may not read. The CI block -- the + # reason this step exists -- rendered "Could not read check status." on a PR with 12 + # checks. A personal access token cannot reproduce it, because it has every scope. actions: read issues: read + checks: read + statuses: read steps: - uses: actions/checkout@v6.0.2 with: diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 0495f56..d28cf13 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -16,6 +16,13 @@ jobs: with: fetch-depth: 1 + # First, because it is the check whose absence took the org down: a workflow that + # Actions cannot parse never starts, so the required check never reports and every pull + # request in the org blocks. Nothing else in this suite can see that -- the file is + # valid YAML and the shell runs it happily. + - name: Workflow validity + run: tests/workflow-lint-test.sh + - name: Review cycle counter run: tests/review-cycle-test.sh diff --git a/tests/context-step-test.sh b/tests/context-step-test.sh index afd505c..2359959 100755 --- a/tests/context-step-test.sh +++ b/tests/context-step-test.sh @@ -36,9 +36,12 @@ extract_step() { ' "$WORKFLOW" } +# Assembled rather than written literally: this file must not contain the delimiter either, +# or the grep below finds itself when someone greps the test suite for it. +EXPR_OPEN="\${$(printf '%s' '{')" extract_step \ - | sed -e 's/\${{ github.event.pull_request.number }}/"$STUB_PR"/g' \ - -e 's/\${{ github.repository }}/"$STUB_REPO"/g' \ + | sed -e "s/${EXPR_OPEN} github.event.pull_request.number }}/\"\$STUB_PR\"/g" \ + -e "s/${EXPR_OPEN} github.repository }}/\"\$STUB_REPO\"/g" \ > "$WORK/step.sh" if [ "$(wc -l < "$WORK/step.sh")" -lt 100 ]; then @@ -46,12 +49,18 @@ if [ "$(wc -l < "$WORK/step.sh")" -lt 100 ]; then "extraction no longer matches the workflow" >&2 exit 1 fi -# Comments are allowed to discuss ${{ }}; code is not allowed to contain one this test -# does not substitute, because an unsubstituted expression would run here as literal text -# and hide whatever the real workflow splices in. -if grep -vE '^[[:space:]]*#' "$WORK/step.sh" | grep -q '\${{'; then - echo "FAIL: the step gained a \${{ }} interpolation this test does not substitute:" >&2 - grep -nE '\${{' "$WORK/step.sh" | grep -vE ':[[:space:]]*#' >&2 +# No Actions expression delimiter may survive anywhere in the extracted script -- not in +# code, and not in a comment either. The comment exemption this check used to carry is what +# shipped a broken workflow to every repo in the org: a shell comment reading "never a +# ${OPEN} interpolation" parses as an *empty expression*, which Actions rejects outright, so +# the workflow never started, no required check ever reported, and every PR in the org sat +# behind "Please close and reopen the PR to trigger this workflow". bash does not care what +# is in a comment; the Actions expression parser does. +if grep -q "$EXPR_OPEN" "$WORK/step.sh"; then + echo "FAIL: an Actions expression delimiter survives in the extracted step script." >&2 + echo " Either this test needs to substitute it, or -- if it is inside a comment --" >&2 + echo " the comment has to stop spelling the delimiter out." >&2 + grep -n "$EXPR_OPEN" "$WORK/step.sh" >&2 exit 1 fi diff --git a/tests/workflow-lint-test.sh b/tests/workflow-lint-test.sh new file mode 100755 index 0000000..0191498 --- /dev/null +++ b/tests/workflow-lint-test.sh @@ -0,0 +1,149 @@ +#!/usr/bin/env bash +# +# Asserts the workflows are valid to GitHub, not merely valid YAML. +# +# This file exists because of an outage. A shell comment inside a `run:` block explained that +# PR title and body deliberately avoid an Actions expression -- and spelled the delimiter out +# to say so. Actions parses those delimiters everywhere in a workflow file, including inside +# a run block's shell comments, and an empty pair is a syntax error. The workflow became +# unparseable, so every run started with zero jobs, the required check never reported, and +# every open pull request across the org sat behind "Please close and reopen the PR to +# trigger this workflow" until the comment was reworded. +# +# Nothing in the previous suite could see it. `yaml.safe_load` accepts the file, the shell +# runs the comment happily, and tests/context-step-test.sh explicitly *exempted* comments +# from its delimiter check. The lesson is narrow and worth encoding: YAML-valid is not +# Actions-valid, and the gap is expression syntax. +# +# actionlint is the real check and runs when available. The scan below is the part that +# always runs, because CI must not depend on a tool being installed to catch the specific +# defect that caused an outage. + +set -euo pipefail + +cd "$(dirname "$0")/.." + +failures=0 +WORKFLOW_FILE=.github/workflows/claude-pr-review.yml + +# The delimiter, assembled rather than written, so this file does not trip its own scan. +OPEN="\${$(printf '%s' '{')" +CLOSE="$(printf '%s' '}')}" + +for wf in .github/workflows/*.yml; do + # The distinction that caused the outage, and the one the scan has to make: a delimiter in + # a *YAML* comment is stripped by the YAML parser and never reaches Actions, while the same + # characters inside a block scalar are part of the string value and are parsed. So block + # scalars are scanned line for line, comments included, and outside them YAML comment lines + # are skipped. Scanning raw text without that distinction reports the harmless env: comment + # that has sat in this workflow since before any of this and would train the reader to + # ignore the check. + bad=$(awk -v open="$OPEN" -v shut="$CLOSE" ' + function scan(line, where, i, rest, j, expr) { + while ((i = index(line, open)) > 0) { + rest = substr(line, i + length(open)) + j = index(rest, shut) + if (j == 0) { print FILENAME ":" FNR ": unterminated expression (" where ")"; return } + expr = substr(rest, 1, j - 1) + gsub(/^[ \t]+|[ \t]+$/, "", expr) + if (expr == "") + print FILENAME ":" FNR ": empty expression in " where \ + " -- Actions rejects the whole workflow" + else if (expr !~ /^[A-Za-z_0-9.,()!<>=&|*'\''"\[\] \t-]+$/) + print FILENAME ":" FNR ": odd expression in " where ": " expr + line = substr(rest, j + length(shut)) + } + } + { + indent = match($0, /[^ ]/) - 1 + if (indent < 0) indent = length($0) + if (in_block) { + if ($0 ~ /^[ \t]*$/) next + if (indent < block_indent) in_block = 0 + else { scan($0, "a block scalar (a shell comment counts)"); next } + } + if ($0 ~ /:[ \t]*[|>][-+0-9]*[ \t]*$/) { + in_block = 1 + block_indent = indent + 1 + next + } + # Outside a block scalar a whole-line YAML comment is invisible to Actions. + if ($0 ~ /^[ \t]*#/) next + scan($0, "a value") + } + ' "$wf") || { + echo "FAIL the expression scan itself failed on $wf; the scan proves nothing" + failures=$((failures + 1)) + continue + } + if [ -n "$bad" ]; then + echo "FAIL $wf has invalid Actions expressions:" + printf '%s\n' "$bad" | sed 's/^/ /' + failures=$((failures + 1)) + else + echo "ok $wf expressions are well formed" + fi +done + +# Every read the context step makes needs a permission declared on the job, because the job +# declares `permissions:` explicitly and anything unlisted is `none`. That failure is silent +# by design -- each block degrades to its "could not read" sentence -- so a missing line here +# does not turn a run red, it just quietly empties the block. Two were missing on the first +# production run (checks and statuses), and no local test could see it: a personal access +# token has every scope, so the step works on a laptop and fails on the runner. +# +# The table is endpoint-shape to permission. It is deliberately coarse; the point is that +# adding a new API call to the step forces a decision about its permission. +step_script=$(awk ' + /^ - name: Gather review context$/ { in_step = 1 } + in_step && /^ run: \|$/ { in_run = 1; next } + in_run && /^ [a-z]/ { exit } + in_run { print } +' "$WORKFLOW_FILE") +declared=$(awk '/^ permissions:$/ { p = 1; next } p && /^ [a-z-]+:/ { print $1 } p && /^ [a-z]/ { exit }' \ + "$WORKFLOW_FILE" | tr -d ':') + +# check +check_permission() { + printf '%s\n' "$step_script" | grep -qF -- "$1" || return 0 + if printf '%s\n' "$declared" | grep -qx "$2"; then + echo "ok $2 is declared for $3" + else + echo "FAIL the step calls $3 but the job never grants $2: read" + failures=$((failures + 1)) + fi +} + +check_permission "/actions/jobs/" actions "the failing-job log excerpts" +check_permission "/issues/" issues "the PR conversation comments" +check_permission "statusCheckRollup" checks "the CheckRun half of the CI rollup" +check_permission "statusCheckRollup" statuses "the StatusContext half of the CI rollup" +check_permission "/compare/" contents "the since-last-review comparison" +check_permission "/pulls/" pull-requests "the PR reads" + +# The scan above is a backstop for one class. actionlint checks the schema, the expression +# grammar, and the shell; run it when it is on PATH. shellcheck findings are excluded because +# the run blocks here intentionally use unquoted word splitting for job ids. +if command -v actionlint >/dev/null 2>&1; then + if out=$(actionlint .github/workflows/*.yml 2>&1); then + echo "ok actionlint reports no findings" + else + remaining=$(printf '%s\n' "$out" | grep -v 'shellcheck reported' || true) + if printf '%s\n' "$remaining" | grep -qE '\[(expression|syntax-check|events|workflow-call)\]'; then + echo "FAIL actionlint reports errors that would stop the workflow from starting:" + printf '%s\n' "$remaining" | grep -E '\[(expression|syntax-check|events|workflow-call)\]' \ + | sed 's/^/ /' + failures=$((failures + 1)) + else + echo "ok actionlint reports no startup-fatal findings" + fi + fi +else + echo "skip actionlint not installed; only the expression scan ran" +fi + +if [ "$failures" -ne 0 ]; then + echo "$failures test(s) failed" + exit 1 +fi +echo "all tests passed" From 492edecce28f67e97cca524fea2ab7c36d5687c6 Mon Sep 17 00:00:00 2001 From: Zac Farrell Date: Tue, 4 Aug 2026 19:58:19 -0700 Subject: [PATCH 2/2] test(review): run actionlint in CI and stop failing open --- .github/workflows/tests.yml | 20 ++++++++++++++++++++ tests/context-step-test.sh | 4 ++-- tests/workflow-lint-test.sh | 29 +++++++++++++++++++++++++---- 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index d28cf13..bb80cf7 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -16,6 +16,26 @@ jobs: with: fetch-depth: 1 + # actionlint is the check that would have caught the outage, so it has to actually run + # here: it is not on the runner image, and without this step the lint test reports + # "skip actionlint not installed" forever and only its own awk backstop runs. + # + # Downloaded and checksummed rather than pulled in as a third-party action, because + # this repository's workflows are what gate every merge in the org -- an unpinned + # `uses:` here would be a supply-chain hole in the one place it hurts most. + - name: Install actionlint + env: + ACTIONLINT_VERSION: 1.7.12 + ACTIONLINT_SHA256: 8aca8db96f1b94770f1b0d72b6dddcb1ebb8123cb3712530b08cc387b349a3d8 + run: | + curl -fsSL -o actionlint.tar.gz \ + "https://github.com/rhysd/actionlint/releases/download/v${ACTIONLINT_VERSION}/actionlint_${ACTIONLINT_VERSION}_linux_amd64.tar.gz" + echo "${ACTIONLINT_SHA256} actionlint.tar.gz" | sha256sum --check --strict + tar -xzf actionlint.tar.gz actionlint + sudo install -m 0755 actionlint /usr/local/bin/actionlint + rm -f actionlint actionlint.tar.gz + actionlint --version + # First, because it is the check whose absence took the org down: a workflow that # Actions cannot parse never starts, so the required check never reports and every pull # request in the org blocks. Nothing else in this suite can see that -- the file is diff --git a/tests/context-step-test.sh b/tests/context-step-test.sh index 2359959..6874eb9 100755 --- a/tests/context-step-test.sh +++ b/tests/context-step-test.sh @@ -36,8 +36,8 @@ extract_step() { ' "$WORKFLOW" } -# Assembled rather than written literally: this file must not contain the delimiter either, -# or the grep below finds itself when someone greps the test suite for it. +# One definition, shared by the two sed patterns and the grep below, so the thing being +# substituted and the thing being forbidden cannot drift apart. EXPR_OPEN="\${$(printf '%s' '{')" extract_step \ | sed -e "s/${EXPR_OPEN} github.event.pull_request.number }}/\"\$STUB_PR\"/g" \ diff --git a/tests/workflow-lint-test.sh b/tests/workflow-lint-test.sh index 0191498..3ba3aaa 100755 --- a/tests/workflow-lint-test.sh +++ b/tests/workflow-lint-test.sh @@ -30,7 +30,16 @@ WORKFLOW_FILE=.github/workflows/claude-pr-review.yml OPEN="\${$(printf '%s' '{')" CLOSE="$(printf '%s' '}')}" -for wf in .github/workflows/*.yml; do +# Actions loads .yaml as well as .yml, and an unscanned workflow would go unmentioned +# rather than reported. +shopt -s nullglob +WORKFLOWS=(.github/workflows/*.yml .github/workflows/*.yaml) +if [ "${#WORKFLOWS[@]}" -eq 0 ]; then + echo "FAIL no workflow files found; the scan proves nothing" + exit 1 +fi + +for wf in "${WORKFLOWS[@]}"; do # The distinction that caused the outage, and the one the scan has to make: a delimiter in # a *YAML* comment is stripped by the YAML parser and never reaches Actions, while the same # characters inside a block scalar are part of the string value and are parsed. So block @@ -46,11 +55,14 @@ for wf in .github/workflows/*.yml; do if (j == 0) { print FILENAME ":" FNR ": unterminated expression (" where ")"; return } expr = substr(rest, 1, j - 1) gsub(/^[ \t]+|[ \t]+$/, "", expr) + # Empty and unterminated only. A character class over what an expression may contain + # rejects valid ones -- hashFiles and format calls use slashes, braces and percent + # signs that no reasonable class covers -- and a heuristic that hard-fails CI on + # correct input gets deleted rather than fixed. actionlint checks the grammar + # properly, and now actually runs in CI. if (expr == "") print FILENAME ":" FNR ": empty expression in " where \ " -- Actions rejects the whole workflow" - else if (expr !~ /^[A-Za-z_0-9.,()!<>=&|*'\''"\[\] \t-]+$/) - print FILENAME ":" FNR ": odd expression in " where ": " expr line = substr(rest, j + length(shut)) } } @@ -100,6 +112,15 @@ step_script=$(awk ' in_run && /^ [a-z]/ { exit } in_run { print } ' "$WORKFLOW_FILE") +# Fail loudly if the extraction drifted. check_permission returns early when the pattern is +# absent from the script, so an empty step_script silently turns all six checks into no-ops -- +# in the one file whose purpose is catching a permission that is silently missing. (declared +# fails safe: empty means every check reports FAIL.) +if [ "$(printf '%s\n' "$step_script" | wc -l)" -lt 100 ]; then + echo "FAIL the context-step extraction no longer matches $WORKFLOW_FILE;" \ + "the permission table proves nothing" + failures=$((failures + 1)) +fi declared=$(awk '/^ permissions:$/ { p = 1; next } p && /^ [a-z-]+:/ { print $1 } p && /^ [a-z]/ { exit }' \ "$WORKFLOW_FILE" | tr -d ':') @@ -125,7 +146,7 @@ check_permission "/pulls/" pull-requests "the PR reads" # grammar, and the shell; run it when it is on PATH. shellcheck findings are excluded because # the run blocks here intentionally use unquoted word splitting for job ids. if command -v actionlint >/dev/null 2>&1; then - if out=$(actionlint .github/workflows/*.yml 2>&1); then + if out=$(actionlint "${WORKFLOWS[@]}" 2>&1); then echo "ok actionlint reports no findings" else remaining=$(printf '%s\n' "$out" | grep -v 'shellcheck reported' || true)