Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/claude-review-self.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ name: Claude Review (self)
# On a pull request from a fork this degrades rather than misbehaves. The merge
# commit is a commit in this repository, so the tooling checkout resolves; the
# workflow being run is still the fork's code merged into `master`, but
# `pull_request` gives a fork run no secrets, so `anthropic-api-key` arrives
# empty and the review step fails — it cannot spend tokens.
# `pull_request` gives a fork run no secrets, so both auth secrets arrive
# empty and the job fails its auth-secret check before the review runs — it
# cannot spend tokens.
# `require-write-access` is left at its default `true`, which stops a
# non-writer's pull request before that.

Expand Down
38 changes: 36 additions & 2 deletions .github/workflows/claude-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@
# pull-requests: write
# secrets:
# anthropic-api-key: ${{ secrets.CLAUDE_API_KEY }}
# # or, on subscription auth instead of API billing — set exactly one
# # of the two; mapping both fails the job:

Check notice on line 44 in .github/workflows/claude-review.yml

View workflow job for this annotation

GitHub Actions / claude-review / Automatic PR review

LOW: Snippet says "mapping both fails the job" but the guard tests values, so a mistyped secret name resolves empty, passes, and silently bills the other credential.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOW — "mapping both fails the job" describes a mapping-based rule; the guard is value-based. It compares $API_KEY and $OAUTH_TOKEN, and GitHub gives a mapped-but-nonexistent secret the same empty string as an unmapped one, so mapping both lines passes whenever only one of the two repo secrets actually exists. The secret descriptions below get this right ("when both are set"); these two snippets — here and README.md:183 — do not.

The gap it hides is the one this repo's own guidelines put first. A caller mapping claude-code-oauth-token: ${{ secrets.CLAUDE_CODE_OATH_TOKEN }} — one transposed letter — resolves empty, the guard sees a single credential and passes, and the review runs on API billing while the caller believes it is on their subscription. Nothing says so. That is not fixable from here (Actions cannot distinguish "unmapped" from "mapped to nothing"), so the useful move is wording that does not promise a check the guard cannot make: "setting both fails the job" rather than "mapping both".

Not a repeat of the earlier thread on this line — that one was about the snippet shipping both mappings as live YAML, which this push fixed.

# # claude-code-oauth-token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
#
# The job fails on a blocking finding. Whether that stops a merge is branch
# protection's decision, made per repo, and reversible without touching this.
Expand Down Expand Up @@ -88,8 +91,17 @@
default: 'master'
secrets:
anthropic-api-key:
description: 'Anthropic API key. Mapped by the caller, since the secret name differs per repo.'
required: true
description: >-
Anthropic API key. Set exactly one of this and
claude-code-oauth-token; a step fails the job when both are missing,
and also when both are set. Mapped by the caller, since the secret
name differs per repo.
required: false
claude-code-oauth-token:
description: >-
Claude Code OAuth token (from `claude setup-token`) — the
subscription alternative to an API key. Same exactly-one rule.
required: false

# One review per PR. Rapid pushes previously started overlapping reviews that
# raced to overwrite the same sticky comment, and paid for every superseded run.
Expand Down Expand Up @@ -178,6 +190,25 @@
echo "required status check, which would report an unreviewed PR as reviewed."
exit 1

# Fail before spending a runner minute on a checkout: with neither
# secret the review step errors anyway, only later and less clearly.
# Both is also an error — which credential would win is the CLI's
# undocumented pick, and the two bill different accounts.
- name: Check exactly one auth secret is present
env:
API_KEY: ${{ secrets.anthropic-api-key }}
OAUTH_TOKEN: ${{ secrets.claude-code-oauth-token }}
run: |
if [ -z "$API_KEY" ] && [ -z "$OAUTH_TOKEN" ]; then
echo "::error::pass anthropic-api-key or claude-code-oauth-token; neither is set."
exit 1
fi
if [ -n "$API_KEY" ] && [ -n "$OAUTH_TOKEN" ]; then
echo "::error::pass only one of anthropic-api-key and claude-code-oauth-token; both are set."
echo "They bill different accounts, so which one wins would be undocumented."
exit 1
fi

Check notice on line 210 in .github/workflows/claude-review.yml

View workflow job for this annotation

GitHub Actions / claude-review / Automatic PR review

LOW: Failing pre-checkout leaves the two if: always() tail steps running node .claude-review/tooling scripts that are not checked out, adding two Cannot find module failures.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOW — This guard fails before the checkout, and the two if: always() steps at the end of the job then run against a workspace that has no tooling in it. Report denied tool requests (line 408) and Enforce review threshold (line 428) both invoke node .claude-review/tooling/review/*.mjs, and neither file exists yet, so each dies with Cannot find module. A caller who mapped neither secret, or both, sees three red steps and two module-resolution errors after the annotation that actually explains the problem.

The job still fails closed, so this is legibility only. It is also not new — Assert the triggering event and Check the access gate ran have the same shape above. What this change does is move it onto the common path: a trigger misconfiguration or a check-member infrastructure failure is rare, while "I mapped the wrong number of auth secrets" is the first thing every repo adopting the OAuth token gets wrong, and the step's own comment gives clearer failure as its reason for existing. steps.review.outcome != '', or a tooling-ready marker output, would keep the two tail steps out of a pre-checkout exit.


- name: Checkout repository
uses: actions/checkout@v4
with:
Expand Down Expand Up @@ -272,7 +303,10 @@
id: review
uses: anthropics/claude-code-action@v1.0.187
with:
# Both pass through as env vars; the action treats an empty one as
# absent. The guard step above enforces exactly one is set.
anthropic_api_key: ${{ secrets.anthropic-api-key }}
claude_code_oauth_token: ${{ secrets.claude-code-oauth-token }}
# Passed so the action uses this token directly instead of exchanging
# its OIDC token for an Anthropic GitHub App token. `setupGitHubToken`
# returns early on it, and the exchange is the *only* thing that runs
Expand Down
9 changes: 8 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,9 @@ jobs:
pull-requests: write
secrets:
anthropic-api-key: ${{ secrets.CLAUDE_API_KEY }}
# or, on subscription auth instead of API billing — set exactly one of
# the two; mapping both fails the job:
# claude-code-oauth-token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
```

No `id-token: write`: nothing here mints an OIDC token, because the workflow
Expand All @@ -197,7 +200,11 @@ either, so granting it in a caller has no effect on the token the job runs with.
| `tooling-ref` | `master` | Ref this repo's `review/` assets come from; see below |

The secret is named, not inherited, because the repos call it different things
(`CLAUDE_API_KEY` vs `CLAUDE_TOKEN`). The `anthropics/claude-code-action`
(`CLAUDE_API_KEY` vs `CLAUDE_TOKEN`). Auth is one of two secrets: an Anthropic
API key, or a Claude Code OAuth token (from `claude setup-token`) for
subscription billing. Set exactly one — a job with neither, or with both,
fails before the checkout; the two bill different accounts, so an undocumented
winner is not a choice the workflow will make silently. The `anthropics/claude-code-action`
version is hardcoded rather than an input: `uses:` does not evaluate
expressions, and a configurable version is how the consumers ended up on
v1.0.182, v1.0.154 and v1.0.134 in the first place. Bump it here and every
Expand Down
Loading