diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 2db5086..e2cebd5 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -212,8 +212,18 @@ jobs: exit 0 fi - gh pr comment "$PR" --body "Skipped: bot-authored PR (\`$ACTOR\`), all ${TOTAL} commit(s) bot-authored. Dependency bumps are reviewed by the risk classifier and human merge gate. Pushing a non-bot commit to this branch re-enables the full review." + # Decision first, announcement second. The comment is reporting, + # not the gate: on 2026-08-17 a ~2-minute GitHub comments-API 503 + # window failed this call, which under `set -e` failed the step — + # turning the REQUIRED review check red on five wxa-graph PRs whose + # decision had already been made (run 32055207194) — AND aborted + # before the skipped=true write, so downstream steps saw no + # decision at all. Record the decision, then post; a failed post + # degrades to a warning. selftest/test_comment_nonfatal_reporting.sh + # pins this. echo "skipped=true" >> "$GITHUB_OUTPUT" + gh pr comment "$PR" --body "Skipped: bot-authored PR (\`$ACTOR\`), all ${TOTAL} commit(s) bot-authored. Dependency bumps are reviewed by the risk classifier and human merge gate. Pushing a non-bot commit to this branch re-enables the full review." \ + || echo "::warning::could not post the skip-announcement comment on PR #${PR} (comments API failure; non-fatal — the skip decision stands)" # Pre-install the Claude Code native binary explicitly. Workaround for # anthropics/claude-code-action#1290 (opened 2026-05-06): upstream diff --git a/.github/workflows/coverage-floor.yml b/.github/workflows/coverage-floor.yml index 2527cfa..949e135 100644 --- a/.github/workflows/coverage-floor.yml +++ b/.github/workflows/coverage-floor.yml @@ -769,6 +769,15 @@ jobs: - name: Post sticky PR comment if: github.event_name == 'pull_request' && always() + # Reporting only — the enforce step above is the gate and has already + # passed or failed by the time this runs. Without this, a + # comments-API failure fails the JOB and turns a PASSING required + # check red: observed 2026-08-17 (run 32055207104), when a ~2-minute + # GitHub 503 window redded `coverage-floor` on wxa-graph PRs that had + # measured 83.0% against a floor of 80.2%. Pinned by + # test_sticky_comment_action_steps_are_nonfatal in + # selftest/test_workflow_guards.py. + continue-on-error: true uses: marocchino/sticky-pull-request-comment@v3 with: header: coverage-floor diff --git a/.github/workflows/dependabot-auto-merge.yml b/.github/workflows/dependabot-auto-merge.yml index 22c6623..625101d 100644 --- a/.github/workflows/dependabot-auto-merge.yml +++ b/.github/workflows/dependabot-auto-merge.yml @@ -264,9 +264,16 @@ jobs: fi gh pr merge --disable-auto "$PR" --repo "$REPO" - gh pr comment "$PR" --repo "$REPO" --body "Auto-merge revoked: this PR was opened by \`${ACTOR}\` and armed for a dependency bump, but it now carries commits that are not bot-authored. The arming decision was made from dependabot metadata and never saw this code, so it no longer applies. Review and merge manually, or move the change to its own PR." echo "Revoked auto-merge on PR #${PR}." + # The revoke above is the enforcement and stays fatal. The comment + # below reports a decision already executed: failing the step on a + # comments-API blip (the 2026-08-17 503 shape) would make a + # SUCCEEDED revoke look failed. Degrade to a warning instead. + # selftest/test_comment_nonfatal_reporting.sh pins both halves. + gh pr comment "$PR" --repo "$REPO" --body "Auto-merge revoked: this PR was opened by \`${ACTOR}\` and armed for a dependency bump, but it now carries commits that are not bot-authored. The arming decision was made from dependabot metadata and never saw this code, so it no longer applies. Review and merge manually, or move the change to its own PR." \ + || echo "::warning::could not post the revoke explanation comment on PR #${PR} (comments API failure; non-fatal — the revoke itself succeeded)" + # RESIDUAL RACE: if every required check is already green when the # non-bot commit lands, GitHub can merge before this job finishes. # Narrowed by construction rather than eliminated — the same push diff --git a/.github/workflows/openapi-types-drift.yml b/.github/workflows/openapi-types-drift.yml index c3f7bbf..52f6773 100644 --- a/.github/workflows/openapi-types-drift.yml +++ b/.github/workflows/openapi-types-drift.yml @@ -430,14 +430,27 @@ jobs: env: GH_TOKEN: ${{ github.token }} PR: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} run: | set -euo pipefail + # Cleanup on the PASSING path — the drift gate already said clean. + # Every gh call here is reporting hygiene: a comments-API failure + # (the 2026-08-17 ~2-minute 503 shape) must degrade to a warning, + # not turn a passing check red. A stale comment then survives until + # the next clean run, which is fine. + # selftest/test_comment_nonfatal_reporting.sh pins this. marker="" - existing=$(gh api "/repos/${{ github.repository }}/issues/$PR/comments" \ - --jq ".[] | select(.body | contains(\"$marker\")) | .id" | head -1) + if ! existing=$(gh api "/repos/$REPO/issues/$PR/comments" \ + --jq ".[] | select(.body | contains(\"$marker\")) | .id" | head -1); then + echo "::warning::could not list PR comments to clean up a stale drift comment (comments API failure; non-fatal — types are clean)" + exit 0 + fi if [ -n "$existing" ]; then - gh api -X DELETE "/repos/${{ github.repository }}/issues/comments/$existing" >/dev/null - echo "Removed stale drift comment ($existing) — types are now clean." + if gh api -X DELETE "/repos/$REPO/issues/comments/$existing" >/dev/null; then + echo "Removed stale drift comment ($existing) — types are now clean." + else + echo "::warning::could not remove stale drift comment ($existing) (comments API failure; non-fatal — types are clean)" + fi fi - name: Fail if drift detected diff --git a/selftest/test_comment_nonfatal_reporting.sh b/selftest/test_comment_nonfatal_reporting.sh new file mode 100644 index 0000000..7e8e924 --- /dev/null +++ b/selftest/test_comment_nonfatal_reporting.sh @@ -0,0 +1,308 @@ +#!/usr/bin/env bash +# A gate's REPORTING must not be able to fail the gate. +# +# INCIDENT (whois-api-llc/wxa-graph #422-#426, 2026-08-17 18:29-18:36Z). +# GitHub's comments API served `HTTP 503: No server is currently available +# to service your request` for about two minutes. Two reusable lanes had +# already DECIDED — claude-review's bot-skip had computed `total=1 +# non-bot=0`, coverage-floor had measured 83.0% against a floor of 80.2% — +# and then failed a REQUIRED check on the post-decision comment call +# (runs 32055207194 and 32055207104). All five open dependabot PRs sat +# blocked on gates that had passed. Worse than the red: in the bot-skip +# step the failed `gh pr comment` also aborted the step BEFORE +# `skipped=true` reached GITHUB_OUTPUT, so downstream steps saw no decision +# at all. +# +# THE INVARIANT: once the decision is made, posting about it is reporting. +# A reporting failure degrades to a ::warning:: and the decision stands — +# it must never red the check (mask a pass), and it must never make the +# decision output unreachable. +# +# THE COUNTER-INVARIANT, pinned just as hard: calls that ARE the +# enforcement stay fatal. `gh pr merge --disable-auto` failing must still +# fail the revoke step — a swallowed revoke failure leaves a stale arm +# live, which is fail-open. Deliberately NOT covered here for the same +# reason: the lost-findings fallback comment in claude-review.yml (its +# failure is load-bearing — a green check over unpostable findings is the +# #165 bug) and the Codex verdict comment in codex-review.yml (it delivers +# findings that the automerge findings gate reads; see +# test_codex_verdict_gate_is_wired_and_opt_in in test_workflow_guards.py). +# coverage-floor's sticky comment is an action step, not bash — its +# continue-on-error pin lives in test_workflow_guards.py +# (test_sticky_comment_action_steps_are_nonfatal). +# +# This extracts each workflow's SHIPPED bash — not a mirrored copy — and +# executes it against a `gh` stub whose comment/list/delete/merge calls can +# be told to fail the way 2026-08-17 did. +# +# Run from the repo root: +# bash selftest/test_comment_nonfatal_reporting.sh +set -euo pipefail + +REVIEW_WF=.github/workflows/claude-review.yml +MERGE_WF=.github/workflows/dependabot-auto-merge.yml +DRIFT_WF=.github/workflows/openapi-types-drift.yml + +failed=0 +T=$(mktemp -d) +trap 'rm -rf "$T"' EXIT +mkdir -p "$T/bin" + +pass() { echo "✓ $1"; } +fail() { + echo "✗ $1" + failed=1 +} + +extract_run() { + local wf="$1" step_name="$2" out="$3" + awk -v name=" - name: $step_name" ' + $0 == name { in_step = 1; next } + in_step && /^ run: \|/ { in_run = 1; next } + in_run { + if ($0 ~ /^ / || $0 == "") { sub(/^ /, ""); print } + else { exit } + } + ' "$wf" > "$out" + if [ ! -s "$out" ]; then + fail "could not extract '$step_name' from $wf — step renamed or reindented?" + return 1 + fi +} + +# A `gh` stub. Reads always work; the write surfaces (pr comment, comment +# list, comment delete, pr merge) fail on request — each with the literal +# 503 the outage served. +cat > "$T/bin/gh" <<'STUB' +#!/usr/bin/env bash +set -uo pipefail +echo "gh $*" >> "$GH_LOG" +fail_503() { + echo "HTTP 503: No server is currently available to service your request (https://api.github.com/graphql)" >&2 + exit 1 +} +case "${1:-}" in + api) + if [ "${2:-}" = "-X" ]; then + case "${3:-}" in + DELETE) + [ "${GH_DELETE_FAIL:-0}" = "1" ] && fail_503 + exit 0 + ;; + *) echo "STUB: unexpected 'gh api -X ${3:-}'" >&2; exit 99 ;; + esac + fi + case "${2:-}" in + */commits) + cat "$GH_ROWS_FILE" + ;; + */issues/*/comments) + [ "${GH_LIST_FAIL:-0}" = "1" ] && fail_503 + printf '%s' "${GH_LIST_IDS:-}" + ;; + *) + # PR head read, used by the revoke step's ownership guard. + echo "${GH_HEAD:-EVENTSHA}" + ;; + esac + ;; + pr) + case "${2:-}" in + comment) + [ "${GH_COMMENT_FAIL:-0}" = "1" ] && fail_503 + exit 0 + ;; + merge) + [ "${GH_MERGE_FAIL:-0}" = "1" ] && fail_503 + exit 0 + ;; + view) echo "${GH_ARMED:-true}" ;; + *) echo "STUB: unexpected 'gh pr ${2:-}'" >&2; exit 99 ;; + esac + ;; + *) echo "STUB: unexpected 'gh ${1:-}'" >&2; exit 99 ;; +esac +STUB +chmod +x "$T/bin/gh" + +# Rows are `author|committer|verified` — same fixtures as +# test_bot_skip_commit_authorship.sh. +rows_genuine=$'dependabot[bot]|web-flow|true' +rows_human=$'topcoder1|topcoder1|false' + +# =========================================================================== +# 1. claude-review.yml — the required review's bot-skip step. +# The decision output is `skipped=true`; the comment is the announcement. +# =========================================================================== +extract_run "$REVIEW_WF" "Skip review for bot-authored PRs (dependabot/renovate)" "$T/review.sh" || true + +if [ -s "$T/review.sh" ]; then + review_case() { + local label="$1" rows="$2" comment_fail="$3" + echo "· scenario review/$label" + printf '%s\n' "$rows" > "$T/rows" + : > "$T/out" + : > "$T/ghlog" + rc=0 + ( + PATH="$T/bin:$PATH" \ + GH_ROWS_FILE="$T/rows" GH_LOG="$T/ghlog" GH_COMMENT_FAIL="$comment_fail" \ + GITHUB_OUTPUT="$T/out" GH_TOKEN=stub PR=422 ACTOR='dependabot[bot]' \ + REPO='whois-api-llc/wxa-graph' \ + bash "$T/review.sh" + ) > "$T/stdout" 2>&1 || rc=$? + } + + # THE INCIDENT: all-bot PR, decision made, comments API down. + review_case "incident" "$rows_genuine" 1 + if [ "$rc" -eq 0 ]; then + pass "review/comment API down: step still exits 0 (required check stays green)" + else + fail "review/comment API down: step exited $rc — a comments-API 503 redded the required check (the 2026-08-17 defect)" + sed 's/^/ /' "$T/stdout" + fi + if grep -q '^skipped=true$' "$T/out"; then + pass "review/comment API down: skipped=true still reaches GITHUB_OUTPUT" + else + fail "review/comment API down: skipped=true never written — downstream steps see no decision at all" + fi + if grep -q '::warning::' "$T/stdout"; then + pass "review/comment API down: degraded to a ::warning::" + else + fail "review/comment API down: no ::warning:: emitted — the lost comment is invisible in the log" + fi + + # Control: comments API healthy — decision identical, announcement posted. + review_case "healthy" "$rows_genuine" 0 + if [ "$rc" -eq 0 ] && grep -q '^skipped=true$' "$T/out" && grep -q '^gh pr comment' "$T/ghlog"; then + pass "review/comment API healthy: skip decided and announced (control)" + else + fail "review/comment API healthy: expected exit 0 + skipped=true + a posted comment (control broke)" + sed 's/^/ /' "$T/stdout" + fi + + # Harness validity: a human commit must still send the PR to a full + # review, with no comment attempted — proves the extracted script's + # decision logic actually ran rather than short-circuiting. + review_case "human" "$rows_human" 1 + if [ "$rc" -eq 0 ] && ! grep -q '^skipped=true$' "$T/out" && ! grep -q '^gh pr comment' "$T/ghlog"; then + pass "review/non-bot commit: full review, no skip, no comment attempted (decision logic intact)" + else + fail "review/non-bot commit: expected exit 0, no skip, no comment (rc=$rc)" + sed 's/^/ /' "$T/stdout" + fi +fi + +# =========================================================================== +# 2. dependabot-auto-merge.yml — the revoke step. The revoke is enforcement; +# the explanation comment afterwards is reporting. +# =========================================================================== +extract_run "$MERGE_WF" "Revoke the arm if a non-bot commit is present" "$T/revoke.sh" || true + +if [ -s "$T/revoke.sh" ]; then + revoke_case() { + local label="$1" comment_fail="$2" merge_fail="$3" + echo "· scenario revoke/$label" + : > "$T/ghlog" + rc=0 + ( + PATH="$T/bin:$PATH" \ + GH_LOG="$T/ghlog" GH_ARMED=true GH_HEAD=EVENTSHA HEAD_SHA=EVENTSHA \ + GH_COMMENT_FAIL="$comment_fail" GH_MERGE_FAIL="$merge_fail" \ + GH_TOKEN=stub PR=422 ACTOR='dependabot[bot]' NON_BOT=1 \ + REPO='whois-api-llc/wxa-graph' \ + bash "$T/revoke.sh" + ) > "$T/stdout" 2>&1 || rc=$? + } + + # THE PATTERN: revoke executed, then the comments API eats the explanation. + revoke_case "comment down" 1 0 + if [ "$rc" -eq 0 ] && grep -q '^gh pr merge --disable-auto' "$T/ghlog"; then + pass "revoke/comment API down: revoke executed and step exits 0 — a SUCCEEDED revoke no longer reads as failed" + else + fail "revoke/comment API down: rc=$rc — the explanation comment failure masks a revoke that already succeeded" + sed 's/^/ /' "$T/stdout" + fi + if grep -q 'Revoked auto-merge' "$T/stdout" && grep -q '::warning::' "$T/stdout"; then + pass "revoke/comment API down: outcome logged, comment loss degraded to a ::warning::" + else + fail "revoke/comment API down: expected the 'Revoked auto-merge' log line plus a ::warning::" + sed 's/^/ /' "$T/stdout" + fi + + # COUNTER-INVARIANT: the revoke itself failing must stay fatal. Swallowing + # it would leave a stale arm live on a PR carrying non-bot commits. + revoke_case "merge down" 0 1 + if [ "$rc" -ne 0 ]; then + pass "revoke/--disable-auto fails: step stays fatal (enforcement is not reporting)" + else + fail "revoke/--disable-auto fails: step exited 0 — a failed revoke was swallowed, stale arm stays live (fail-open)" + sed 's/^/ /' "$T/stdout" + fi +fi + +# =========================================================================== +# 3. openapi-types-drift.yml — stale-comment cleanup on the PASSING path. +# drift=0 is the gate saying "clean"; everything in this step is hygiene. +# =========================================================================== +extract_run "$DRIFT_WF" "Remove stale drift comment when clean" "$T/drift_clean.sh" || true + +if [ -s "$T/drift_clean.sh" ]; then + drift_case() { + local label="$1" list_fail="$2" ids="$3" delete_fail="$4" + echo "· scenario drift-clean/$label" + : > "$T/ghlog" + rc=0 + ( + PATH="$T/bin:$PATH" \ + GH_LOG="$T/ghlog" GH_LIST_FAIL="$list_fail" GH_LIST_IDS="$ids" \ + GH_DELETE_FAIL="$delete_fail" \ + GH_TOKEN=stub PR=422 REPO='whois-api-llc/wxa-graph' \ + bash "$T/drift_clean.sh" + ) > "$T/stdout" 2>&1 || rc=$? + } + + drift_case "list down" 1 '' 0 + if [ "$rc" -eq 0 ] && grep -q '::warning::' "$T/stdout"; then + pass "drift-clean/comment listing down: passing gate stays green, loss is a ::warning::" + else + fail "drift-clean/comment listing down: rc=$rc — a comments-API 503 redded the PASSING path" + sed 's/^/ /' "$T/stdout" + fi + + drift_case "delete down" 0 $'9001\n' 1 + if [ "$rc" -eq 0 ] && grep -q '::warning::' "$T/stdout"; then + pass "drift-clean/comment DELETE down: passing gate stays green, loss is a ::warning::" + else + fail "drift-clean/comment DELETE down: rc=$rc — a comments-API 503 redded the PASSING path" + sed 's/^/ /' "$T/stdout" + fi + + # Controls: with a healthy API the cleanup still actually cleans up… + drift_case "delete works" 0 $'9001\n' 0 + if [ "$rc" -eq 0 ] && grep -q '^gh api -X DELETE' "$T/ghlog" && grep -q 'Removed stale drift comment' "$T/stdout"; then + pass "drift-clean/stale comment present: deleted (control)" + else + fail "drift-clean/stale comment present: expected exit 0 + a DELETE + the removal log line (rc=$rc)" + sed 's/^/ /' "$T/stdout" + fi + + # …and with nothing stale it touches nothing. + drift_case "nothing stale" 0 '' 0 + if [ "$rc" -eq 0 ] && ! grep -q 'DELETE' "$T/ghlog"; then + pass "drift-clean/no stale comment: no-op (control)" + else + fail "drift-clean/no stale comment: expected exit 0 and no DELETE (rc=$rc)" + sed 's/^/ /' "$T/stdout" + fi +fi + +# --------------------------------------------------------------------------- +if [ "$failed" -eq 0 ]; then + echo + echo "ALL PASS: reporting failures degrade to warnings; decisions and enforcement stand." +else + echo + echo "FAILURES above." +fi +exit "$failed" diff --git a/selftest/test_workflow_guards.py b/selftest/test_workflow_guards.py index c703e6b..5f233b5 100644 --- a/selftest/test_workflow_guards.py +++ b/selftest/test_workflow_guards.py @@ -39,6 +39,7 @@ "selftest/test_claude_review_lost_findings_guard.sh", "selftest/test_claude_review_cost_guardrails.sh", "selftest/test_codex_verdict_gate.sh", + "selftest/test_comment_nonfatal_reporting.sh", "selftest/test_findings_reply_narrowing.sh", "selftest/test_pr_files_listing.sh", "selftest/test_prettier_scope_failsafe.sh", @@ -759,3 +760,51 @@ def test_lint_ruff_version_is_pinned(): "the step must reject non-exact ruff_version values; a caller passing " "`0.15.*` would float to latest-matching while still looking pinned" ) +def test_sticky_comment_action_steps_are_nonfatal(): + """A sticky-comment ACTION step is reporting, not the gate. + + coverage-floor.yml renders its markdown table after the enforce step has + already passed or failed. On 2026-08-17 a ~2-minute GitHub comments-API + 503 window failed the sticky-comment action itself, which failed the job + and turned the REQUIRED `coverage-floor` check red on wxa-graph PRs that + had measured 83.0% against an 80.2% floor (run 32055207104). + `continue-on-error: true` keeps a lost comment a step-level annotation + instead of a gate verdict. + + The shell-step half of the same invariant (claude-review's bot-skip, + dependabot-auto-merge's revoke explanation, openapi-types-drift's + stale-comment cleanup) is behavioral, in + selftest/test_comment_nonfatal_reporting.sh; action steps can't be + executed there, so this sweep pins them structurally. It walks EVERY + workflow so the next sticky-comment step added to any lane inherits the + invariant, and it is anchored to coverage-floor.yml so a rename or + restructure cannot leave it sweeping nothing and passing vacuously. + """ + sticky = "marocchino/sticky-pull-request-comment" + found_in = set() + for wf in sorted(WORKFLOWS_DIR.glob("*.yml")): + lines = wf.read_text().splitlines() + step_starts = [ + i + for i, line in enumerate(lines) + if line.lstrip().startswith(("- name:", "- uses:")) + ] + for i, line in enumerate(lines): + if sticky not in line or "uses:" not in line: + continue + found_in.add(wf.name) + start = max((s for s in step_starts if s <= i), default=0) + end = min((s for s in step_starts if s > i), default=len(lines)) + block = "\n".join(lines[start:end]) + assert "continue-on-error: true" in block, ( + f"{wf.name}: the sticky-comment step at line {i + 1} can fail " + "its job on a comments-API blip — reporting must not red a " + "gate that already decided (2026-08-17, run 32055207104). " + "Add `continue-on-error: true` to the step." + ) + assert "coverage-floor.yml" in found_in, ( + "coverage-floor.yml no longer posts its sticky comment via " + "marocchino/sticky-pull-request-comment — re-anchor this sweep to " + "however the reporting step is implemented now, so it keeps guarding " + "the real one" + )