From 039dee9db0409f694ccf59c7ce32b1558f15ba4c Mon Sep 17 00:00:00 2001 From: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> Date: Wed, 8 Jul 2026 14:17:50 -0700 Subject: [PATCH] fix(claude-review): skip cleanly when a commit has no unique open PR The review stage (Stage 2) is a workflow_run workflow: GitHub runs it from the default branch and labels every run with that branch. When the PR-resolution guard could not map the trusted head SHA to exactly one open PR, it exited 1 -- painting the Actions tab with red "Claude PR Review" runs labelled with the default branch, which read as broken post-merge reviews even though the guard was correctly refusing to review anything. This is a common, benign case: a workflow_run fires from a collect run whose PR was merged/closed by the time this stage resolves it (e.g. dependabot rebasing several branches onto a freshly-merged tip), or a force-push/close race. Downgrade only the "0 or >1 open PRs" case -- an absence of work, not a forged input -- to a clean skip: set resolved=false, exit 0, and gate every downstream step (checkout, tooling, Bedrock auth, agent) on resolved=true. The security property is unchanged: nothing is reviewed or posted unless a unique open PR is resolved. Malformed head_sha or a resolved PR failing pr_number/base_sha validation still hard-fails. Signed-off-by: Stephen Crowe <6042774+crowecawcaw@users.noreply.github.com> --- .../workflows/reusable_claude_pr_review.yml | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/.github/workflows/reusable_claude_pr_review.yml b/.github/workflows/reusable_claude_pr_review.yml index 23a1d73..36bf722 100644 --- a/.github/workflows/reusable_claude_pr_review.yml +++ b/.github/workflows/reusable_claude_pr_review.yml @@ -85,7 +85,25 @@ jobs: # head repo matches the trusted head_repository, and require exactly one # match. The PR number and base SHA are taken from that PR -- so the # comment target is guaranteed to be the PR that actually contains the - # reviewed commit. Force-push/close races resolve to != 1 and fail closed. + # reviewed commit. + # + # When the commit does not resolve to exactly one open PR, there is simply + # nothing to review, and we SKIP CLEANLY (set resolved=false, exit 0) so + # the run finishes green and every downstream step is gated off. This is + # the common, benign case: a workflow_run fires from a collect run whose + # PR was merged/closed by the time this stage resolves it (dependabot + # rebasing several branches onto a freshly-merged tip is a frequent + # source), or a force-push/close race. Failing hard here painted the + # Actions tab with red "Claude PR Review" runs -- always labelled with the + # default branch, since workflow_run runs from the default branch -- which + # read as broken post-merge reviews even though the guard was doing its + # job. Skipping quietly keeps the security property (nothing is reviewed or + # posted unless a unique open PR is resolved) while removing the noise. + # + # Note this is NOT a relaxation of the guard: a malformed head_sha, or a + # resolved PR whose pr_number/base_sha fail validation, is a real integrity + # problem and still hard-fails (exit 1). Only "0 or >1 open PRs" -- an + # absence of work, not a forged input -- downgrades to a clean skip. - name: Resolve and validate PR from trusted head SHA id: meta env: @@ -111,8 +129,13 @@ jobs: count=$(echo "$matches" | jq 'length') if [[ "$count" -ne 1 ]]; then - echo "::error::expected exactly 1 open PR for $HEAD_SHA from $EXPECTED_HEAD_REPO, found $count" - exit 1 + # No unique open PR for this commit -> nothing to review. Skip + # cleanly (green) instead of erroring; downstream steps are gated on + # resolved=true. This is an absence of work, not a forged input, so + # it is not a failure. (See the step comment above.) + echo "::notice::No unique open PR for $HEAD_SHA from $EXPECTED_HEAD_REPO (found $count); nothing to review, skipping." + echo "resolved=false" >> "$GITHUB_OUTPUT" + exit 0 fi PR_NUMBER=$(echo "$matches" | jq -r '.[0].number') @@ -120,6 +143,7 @@ jobs: [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || { echo "::error::bad pr_number"; exit 1; } [[ "$BASE_SHA" =~ ^[0-9a-f]{40}$ ]] || { echo "::error::bad base_sha"; exit 1; } + echo "resolved=true" >> "$GITHUB_OUTPUT" echo "pr_number=$PR_NUMBER" >> "$GITHUB_OUTPUT" echo "head_sha=$HEAD_SHA" >> "$GITHUB_OUTPUT" echo "base_sha=$BASE_SHA" >> "$GITHUB_OUTPUT" @@ -144,7 +168,12 @@ jobs: # unresolved findings may be posted again (re-anchored at their new line). # We identify our own comments by the marker, not by login, so this works # identically on forks and upstream. + # All steps below run only when a unique open PR was resolved above. If the + # commit did not map to exactly one open PR, `resolved` is false and every + # remaining step (including the credential and agent steps) is skipped, so + # the job finishes green without touching AWS or posting anything. - name: Collect prior review state + if: steps.meta.outputs.resolved == 'true' env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} OWNER: ${{ github.repository_owner }} @@ -200,6 +229,7 @@ jobs: # is false so no token is left on disk, submodules are not fetched, and the # repo root is left untouched. The agent reads pr-head/ but never runs it. - name: Checkout PR head (read-only) + if: steps.meta.outputs.resolved == 'true' uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.0 with: repository: ${{ inputs.head_repository }} @@ -212,11 +242,13 @@ jobs: # Remove any symlinks from the checkout so the agent's read tools can only # see files inside pr-head/. - name: Strip symlinks from PR head + if: steps.meta.outputs.resolved == 'true' run: find pr-head -type l -delete # Add the base commit so the agent can compute the review diff with # `git diff $BASE_SHA..HEAD`. No PR code is executed. - name: Fetch base commit for diff + if: steps.meta.outputs.resolved == 'true' working-directory: pr-head env: BASE_SHA: ${{ steps.meta.outputs.base_sha }} @@ -230,6 +262,7 @@ jobs: # read the version-pinned requirements file. persist-credentials is false # so no token is left on disk. - name: Check out workflow tooling (pinned requirements) + if: steps.meta.outputs.resolved == 'true' uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.0 with: repository: ${{ inputs.tooling_repository }} @@ -244,6 +277,7 @@ jobs: # present means a compromised release has no role credentials to exfiltrate # at install time. --only-binary blocks arbitrary setup.py execution. - name: Install Bedrock token generator (no AWS creds in env) + if: steps.meta.outputs.resolved == 'true' run: | set -euo pipefail python3 -m pip install --quiet --only-binary :all: \ @@ -253,6 +287,7 @@ jobs: # the IAM credentials, then unset those credentials before the agent runs. # role-duration-seconds bounds the token's lifetime to about an hour. - name: Configure AWS credentials (OIDC, mint scope only) + if: steps.meta.outputs.resolved == 'true' uses: aws-actions/configure-aws-credentials@254c19bd240aabef8777f48595e9d2d7b972184b # v6.0.0 with: role-to-assume: ${{ secrets.bedrock_role_arn }} @@ -260,6 +295,7 @@ jobs: role-duration-seconds: 3600 - name: Mint Bedrock bearer token and drop IAM credentials + if: steps.meta.outputs.resolved == 'true' env: AWS_REGION: ${{ inputs.aws_region }} run: | @@ -279,6 +315,7 @@ jobs: # MCP tools -- and the run is bounded by the job-level timeout-minutes. The # agent step's env carries only the Bedrock bearer token and GITHUB_TOKEN. - name: Claude PR review + if: steps.meta.outputs.resolved == 'true' uses: anthropics/claude-code-action@0b1b62002952733671bde978d429b50b51c51c85 # v1.0.136 env: AWS_BEARER_TOKEN_BEDROCK: ${{ env.AWS_BEARER_TOKEN_BEDROCK }}