Add federated Claude PR review

[tobyhede]

Summary

• add advisory Claude review for trusted, same-repository pull requests
• authenticate through Anthropic workload identity federation with no API key
• restrict permissions and Claude tools to repository reads and PR comments
• add focused workflow, fork-boundary, and OIDC classification guards
• document the non-publishing OIDC exchange and add the required changeset
• let the review read the diff (scoped gh pr diff / gh pr view) and post its summary (gh pr comment --edit-last): in agent mode the action gives Claude no PR context and creates no comment
• correct SECURITY.md: OIDC holders are asserted in one equality over the union of the publishing and non-publishing lists

Validation

• pnpm test:scripts — 922 passed
• pnpm --dir e2e exec vitest run tests/supply-chain.e2e.test.ts — 26 passed
• pnpm exec turbo run typecheck — 16 tasks passed
• actionlint .github/workflows/claude-review.yml
• focused Biome checks and pnpm exec changeset status

The complete pnpm test run reached the package suites but the CLI live-test
collector could not resolve the locally unavailable
@cipherstash/eql-upgrade-baseline/sql; 90 other CLI test files passed.

Follow-up: #997 (parallel review lenses defined as repo skills).

[changeset-bot]

🦋 Changeset detected

Latest commit: 95300fd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages

Name	Type
stash	Patch
@cipherstash/wizard	Patch
@cipherstash/basic-example	Patch
@cipherstash/e2e	Patch
@cipherstash/stack-drizzle	Patch
@cipherstash/stack-prisma	Patch
@cipherstash/stack-supabase	Patch
@cipherstash/stack	Patch
@cipherstash/bench	Patch
@cipherstash/prisma-example	Patch
@cipherstash/test-kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[freshtonic]

Approving. The security posture is right and the guards are real, not decorative. Three follow-ups below, none blocking.

What I checked and agree with

• Fork boundary. pull_request rather than pull_request_target, plus an explicit head.repo.full_name == github.repository job condition, and the job is registered in EXPECTED_FORK_GUARDED_JOBS. The trigger set is pinned by an equality (Object.keys(triggers)), so the dead github.event_name != 'pull_request' disjunct cannot quietly become a bypass when somebody adds a second trigger — that test fails first. Worth keeping the equality in mind if the disjunct ever tempts anyone to delete it.
• No static credentials. anthropic_api_key and claude_code_oauth_token are asserted absent, not merely omitted. The action is SHA-pinned and the SHA is pinned again in the test. persist-credentials: false on the checkout.
• Least privilege. contents: read at workflow level, the three scopes granted per job, and pull-requests: write justified by the sticky comment and the inline-comment MCP tool.
• Prompt construction. No user-controlled string is interpolated into the prompt — only repository, PR number and head SHA. That closes the script-injection path that usually sinks these workflows, and it is the part most PRs of this kind get wrong. The tool denylist covers the exfiltration and mutation routes.
• Housekeeping. Changeset present and correctly scoped to stash and @cipherstash/wizard, since skills/ ships in both tarballs. stash-supply-chain-security is the right skill to have touched.

Follow-up 1 — the check is green and the review did not run

On this PR the action skipped:

Skipping action due to workflow validation: Workflow validation failed. The workflow file must exist and have identical content to the version on the repository's default branch.

The step then reported outcome=success and the review check passed. The vendor says this is expected when the workflow file itself is in the diff, and it is — nothing to fix here, and nothing gates on it today, since main carries no branch protection.

The durable problem is the shape. Any future PR that edits claude-review.yml gets a green review check that reviewed nothing, and nothing says so. This repository argues against precisely that elsewhere — workflow-publish-permissions.test.mjs pins the OIDC holder set because "a scan that finds none of them passes having verified nothing", and integration-workflow-paths.test.mjs carries a required floor for the same reason.

I do not think it blocks the merge, because the behaviour is the vendor's and the first real run only happens after this lands. Worth a follow-up that either fails the job on the validation skip, or records the behaviour so the next person does not read a green tick as a review.

Follow-up 2 — the classification exempts the file, not just the job

NON_PUBLISH_OIDC_JOBS correctly keeps the Claude job out of the publisher equality. But it also drops the whole file out of the "leaves the non-publishing jobs of a publishing workflow read-only" scan, because that filter now requires an OIDC holder that is not classified.

Nothing is lost today: the workflow has one job, and claude-review-workflow.test.mjs pins its permissions exactly. The gap is a second job added later with, say, contents: write and no id-token — the equality on OIDC holders would not see it, the sibling scan no longer covers the file, and no test pins the job set.

Cheapest fix is an equality on Object.keys(workflow.jobs) in the dedicated test. Alternatively keep the file inside the sibling scan and exempt only the publisher-specific assertions. Either way the reasoning in the new doc comment stays correct.

Follow-up 3 (documentation — for a separate PR)

SECURITY.md now says the test holds publishers and named non-publishing exchanges as "separate equalities". There is one equality, and it is over the union: holders.sort() against OIDC_JOBS.sort(). The two lists are separate lists feeding separate predicates, and REPO_WRITE_JOBS is a spread of the union rather than an independent equality. The distinction matters in this file specifically, because it is what a security reporter reads to work out which guard would have caught a given mistake.

Everything else in the SECURITY.md and skill wording reads accurately, including "keep its static credential inputs absent", which the test does enforce.

Note, not a finding

The four federation identifiers are echoed into a public build log by the preflight step. That is inherent to holding them in vars rather than secrets, and it is the right call — the trust sits in the OIDC token's repository and workflow claims, not in the identifiers. Flagging only so the choice is on the record as deliberate.

[tobyhede]

Follow-up spec for running the review as parallel, skill-defined lenses (including the official /code-review plugin), and for documenting the rules for changing this workflow: #997.

[tobyhede]

@freshtonic thanks. Status of the three follow-ups:

1. Green check, no review. 7d54c27 adds "Require completed Claude review", which fails the job unless the action reports conclusion=success. The action's workflow-validation skip returns without setting it (src/entrypoints/run.ts:179 at the pinned SHA). So this PR's own review check is now red, as expected, and the error message names the likely cause.
2. The file drops out of the sibling scan. claude-review-workflow.test.mjs now pins Object.keys(workflow.jobs) to ['review'].
3. SECURITY.md. Corrected in 95300fd: the OIDC holders are asserted in one equality over the union of the two lists, and the lists feed separate predicates.

While auditing I found a bigger problem the green checks had been hiding. A prompt on a pull_request event puts the action in agent mode, which gives Claude no PR context and creates no comment, so use_sticky_comment does nothing. With Bash disallowed and a depth-1 checkout, the first real run could not have seen the diff, and its summary would have been thrown away while the check still passed. 95300fd allows scoped gh pr diff, gh pr view and gh pr comment --edit-last --create-if-none. I left track_progress: true alone because it switches to tag mode, which grants git commit/push and accepts edits. None of this can be verified until the first PR after merge.

Follow-up spec for parallel, skill-defined review lenses: #997.