Skip to content

fix(claude-review): review caller-editing PRs with the job token, fail closed when nothing was reviewed - #300

Merged
topcoder1 merged 5 commits into
mainfrom
claude/review-caller-token-fail-closed
Oct 17, 2026
Merged

topcoder1 merged 5 commits into
mainfrom
claude/review-caller-token-fail-closed

Conversation

@topcoder1

@topcoder1 topcoder1 commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

Problem

anthropics/claude-code-action (pinned at a4f54ef, v1.0.229) gets its GitHub token by exchanging the job's OIDC token for a Claude GitHub App token. The exchange refuses any run whose calling workflow differs from the default branch's copy ("workflow validation"). The action then logs a warning and returns without failing its step (src/github/token.ts, src/entrypoints/run.ts).

So review / Claude Review concludes success in about 30 seconds with nothing reviewed on every install PR and every PR that edits its caller, and every other file in those diffs goes unreviewed too. Where a ruleset requires the check, that green satisfies it. Fleet evidence (private): topcoder1/dotclaude#519.

Change

C: review those PRs with the job token. A new "Caller check" step runs before the review.

  • It reads the caller's path from github.workflow_ref (in a reusable workflow the github context is the caller's). It then compares the caller's blob at HEAD (git rev-parse HEAD:<path>, the merge commit the run took its caller from) with the default branch's copy, read through the contents API.
  • changed=false only on a proven match. A different blob (an edit), a 404 "Not Found" (an install PR) or any read failure gives changed=true, which is fail safe: the review runs.
  • Whenever changed != 'false', the review step passes github_token: ${{ github.token }}. setupGitHubToken() returns a given token before it requests an OIDC token, and the action's "Revoke app token" step runs only when inputs.github_token == '' (action.yml). Every other PR keeps the App identity.
  • The job token holds the job's existing permissions: (contents: read, pull-requests: write). Nothing was widened. Comments made with it start no workflow runs.
  • Inline comments on those runs come from github-actions[bot], so the lost-findings guard's REVIEW_BOT follows the same predicate. Every workflow's job token shares that identity, so on job-token runs the guard counts only inline comments at a path:line that the review's own inline-comment calls named. App-token runs are unchanged.
  • The caller check has continue-on-error: true. If it dies before writing its outputs, the predicate still selects the job token and the review runs.

B: fail closed when nothing was reviewed. A new "Review-ran check" step runs after the review.

  • A review step that ended success with no conclusion output reviewed nothing. On the skip path the action sets neither conclusion nor execution_file. A real run sets conclusion, and any failure fails the step.
  • The step writes reviewed=false first. It then posts, or on later runs PATCHes, ONE sticky comment that says what the caller check found, and exits 1.
  • The sticky is the github-actions[bot] comment that starts with <!-- claude-review:not-reviewed --> and ends with <!-- claude-review:not-reviewed:end -->. The review summary can never match it: the model writes the summary's start, but the summary step appends its own footer last.
  • A failed post retries 3x, then degrades to a warning with the body in the log, :: and ##[ broken. The verdict stands either way.
  • With C in place, B fires only on a skip nobody expected.

README: the "Install PR's Claude Review check may fail" caveat now describes the real behaviour.

Testing

A ci-workflows PR cannot exercise its own reusable change, because its caller pins @main. The selftest is the proof.

  • selftest/test_claude_review_ran_check.sh (new, 36 cases, added to the pytest wrapper list):
    • It runs the SHIPPED bash of the caller check against a real git checkout (a shallow clone too) and a stubbed contents API. It runs the review-ran check and the guard against a stubbed gh.
    • It EVALUATES the step if:s and the github_token / REVIEW_BOT expressions with a small GitHub-expression evaluator, for changed = true, false, empty and unset, and on the bot-skip, failed-review and cancelled paths.
    • Controls cover the decision-first ordering, the job-token comment filter and the && '' || falsy-middle trap.
    • It also pins the default merge-ref checkout and the action SHA, so an action bump has to re-read the behaviour both steps rely on.
  • Mutation check (not committed): 24 hand-written mutants of the workflow, and the original main workflow, each fail the selftest. Examples: exit 0 instead of 1; the decision written after posting; a static REVIEW_BOT; the falsy-middle token expression; no continue-on-error; unchanged as the default; the author or marker filters loosened; no PATCH path; log prefixes kept; a blind post; a missing default-branch ref; any 404 read as an install; conclusion=failure read as unreviewed; an unvalidated caller path; a checkout of the PR head; an unpinned action; the guard filter off; the end marker dropped.
  • uv run pytest on this head: 768 passed, 1 skipped. Baseline on main before fix(audit-required-checks): fail loudly on API errors, file findings apart #299 landed: 766 passed, 1 skipped; fix(audit-required-checks): fail loudly on API errors, file findings apart #299 and this PR each add one wrapper entry.
  • actionlint 1.7.12 (the lint lane's pin), shellcheck and prettier 3 on the two READMEs: clean. The new selftest also passes under macOS /bin/bash 3.2.
  • Checked against the live API (read-only, on this repo): gh api -X GET repos/…/contents/<path> -f ref=<branch> returns the same blob sha as git rev-parse HEAD:<path>. A missing file is a 404 "Not Found"; a missing ref is a 404 "No commit found for the ref".
  • Independent review (a separate read-only reviewer, given the action source): approve, no findings at 80% confidence or above. Its one open question is the validation-commit caveat below; the matched-caller wording was changed to cover it.

Accepted, not addressed (from the Codex pre-review)

  • Race with a default-branch change. A caller change landing on the default branch between the caller check and the token exchange (seconds) still skips. B fails the job, and a re-run reviews with the job token. Documented at the step.
  • Which commit the validation reads. The exchange's backend is not public, so it is not documented whether validation compares the caller in the merge commit (what the run checks out, and what the caller check reads) or another commit of the PR. If it is another commit, a PR branched before a caller change landed would take the App path, skip, and fail closed until its branch is updated. The matched-caller comment names this and says to update the branch.
  • Same-line collision. Job-token attribution is by path:line, so another workflow commenting on the very same line in the same minutes, while this review's comment was lost, still counts. Body matching would misfire because the action's sanitizer rewrites bodies, and buffered comments' IDs never reach the transcript.
  • Overlapping review runs on one PR. A cancelled run's late buffered comments can land after the next run's anchor. This is the guard's existing time-anchor limitation, the same on the App path, and the callers' cancel-in-progress concurrency bounds it (see the guard's OVERLAP NOTE). Anchoring on the reviewed commit would not close it: the action posts buffered comments on the PR's head at posting time (src/entrypoints/post-buffered-inline-comments.ts), so a cancelled run's late comments carry the new head.
  • Public markers. A workflow that deliberately writes a comment with both markers could have it overwritten. Two concurrently failing runs could race to create two stickies; the callers' concurrency group prevents that, and it would only be cosmetic.

After merge

  • The next install PR or caller edit in the fleet is the live check.
  • A dotclaude doc follow-up brings the README "Caller-PR caveat" and the installer wording in line with C + B.

Auto-merge rationale: manual click-merge: every PR to topcoder1/ci-workflows is manual, and this changes the reusable behind a fleet-wide required check.
Codex rounds: 6 (cap reached). Round 1 clean; round 2 three P2, fixed; round 3 clean; round 4 two P1 and one P2, two fixed and the race documented; round 5 one P2 (a narrower residual of round 4), documented; round 6 one P1 and two P2, dispositioned above (the P1 is the existing overlap limit).

🤖 Generated with Claude Code

topcoder1 and others added 5 commits October 16, 2026 10:08
…l closed when nothing was reviewed

claude-code-action exchanges the job's OIDC token for a Claude App token,
and the exchange refuses any run whose calling workflow differs from the
default branch's copy. The action then warns and exits 0 without reviewing
(src/github/token.ts, src/entrypoints/run.ts at the a4f54ef pin), so
`review / Claude Review` went green with nothing reviewed on every install
PR and every caller edit.

- Caller check: compare the caller (github.workflow_ref) at HEAD with the
  default branch's copy via the contents API. When it differs, is absent
  (an install PR), or cannot be read, pass `github_token: github.token`,
  which skips the exchange, so the PR gets a real review. The lost-findings
  guard's REVIEW_BOT follows: github-actions[bot] on those runs.
- Review-ran check: step success with no `conclusion` output means nothing
  was reviewed. Write reviewed=false first, post or PATCH one sticky
  `<!-- claude-review:not-reviewed -->` comment, and fail the job.
- selftest/test_claude_review_ran_check.sh runs the shipped bash against a
  stubbed gh and a real git checkout, and evaluates the step `if:`s and the
  token / REVIEW_BOT expressions.
- README: the caller-PR caveat now describes the real behaviour.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ut and the action SHA

Codex pre-review round 2:
- The review summary is a github-actions[bot] comment too, so a summary
  that quoted `<!-- claude-review:not-reviewed -->` would have been PATCHed
  over by the next not-reviewed run. The sticky lookup now requires the
  body to start with the marker (the pattern claude-author-automerge uses
  for its own sticky).
- The selftest pins the default merge-ref checkout the caller check reads
  HEAD from, and the claude-code-action SHA whose token and skip behaviour
  both new steps rely on, so a bump has to re-read that source.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sticky needs both markers

Codex pre-review round 4:
- On a job-token run the lost-findings guard counted every
  github-actions[bot] inline comment in the window, the identity every
  workflow's job token shares, so another workflow's comments could stand in
  for this review's lost findings. It now counts only comments at a
  path#line the review's own inline-comment calls named (the action posts
  path and line as given and rewrites the body; an outdated comment keeps
  the line as original_line). A transcript it cannot read leaves the count
  unfiltered. App-token runs are unchanged.
- The not-reviewed sticky now also ends with
  `<!-- claude-review:not-reviewed:end -->`, and the lookup requires both
  markers. The model writes the start of the review summary, but the
  summary step appends its own footer last, so no summary can match.
- The review-ran comment no longer rules out the validation skip when the
  caller matched: a caller change can land on the default branch between
  the caller check and the token exchange. That race fails closed, and a
  re-run reviews with the job token; documented at the caller check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…attribution

Codex pre-review round 5 asked to match job-token inline comments by more
than path and line. Not taken: the action's sanitizer rewrites bodies, so
body matching would misfire on ordinary reviews, and buffered comments post
after the session, so their IDs never reach the transcript. The residual
(another workflow commenting on the very same path and line within the
review window while this review's comment was lost) is now named at the
filter.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on when the caller matched

Independent review: whether the action's validation compares the caller in
the merge commit this run checks out or another commit of the PR is not
documented. The comment for the matched-caller case now names both ways a
validation skip could still happen (the default branch changed during the
run, or another commit was read) and says to update the branch, instead of
ruling the skip out.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@topcoder1
topcoder1 marked this pull request as ready for review October 17, 2026 00:19
@topcoder1
topcoder1 merged commit 71322c5 into main Oct 17, 2026
19 checks passed
@topcoder1
topcoder1 deleted the claude/review-caller-token-fail-closed branch October 17, 2026 00:20
@github-actions github-actions Bot added the risk:blocked Risk class: blocked label Oct 17, 2026
@github-actions

Copy link
Copy Markdown

Risk class: blocked — manual merge required.

This PR touches one of the blocked path categories from .github/risk-paths.yml (Dockerfiles, docker-compose, .github/workflows/**, **/.env*, **/secrets*, infra/, terraform/, k8s/, or the classifier config itself).

Auto-merge is refused by claude-author-automerge.yml. A maintainer should review the diff and click "Squash and merge" themselves.

(This is a policy notice, not a code-quality failure. The classify job itself does not fail — required CI checks remain authoritative for "is the code green.")

@github-actions

Copy link
Copy Markdown

The summary and guard steps (lines 715, 820) use !cancelled() without a status function — GitHub Actions prepends success() && automatically. So if ran exits 1, those downstream steps are correctly suppressed. No issue there.

Summary of findings: The caller check correctly fails safe (changed=true) on any read error, crash, or unrecognised workflow_ref format; the != 'false' predicate is evaluated (not just compared as text) against all four states of changed (true/false/empty/unset) in the selftest; the review-ran check writes its verdict before any API call and retries comment posting 3× before degrading to a warning; the sticky-comment deduplication correctly distinguishes the not-reviewed marker from review summaries, human comments, and other bots; and the job-token spot filter prevents another workflow's comments from masking lost findings. No bugs found.

No issues found. The logic is fail-safe throughout and the selftest covers the key edge cases well.

Claude Review · workflow run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:blocked Risk class: blocked

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant