diff --git a/.github/scripts/claude-code-review-workflow-contract_test.py b/.github/scripts/claude-code-review-workflow-contract_test.py new file mode 100644 index 0000000000..cab2f8d6ee --- /dev/null +++ b/.github/scripts/claude-code-review-workflow-contract_test.py @@ -0,0 +1,28 @@ +#!/usr/bin/env python3 +"""Contract tests for the Claude Code fork-review workflow.""" + +from pathlib import Path +import unittest + + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOW = REPO_ROOT / ".github" / "workflows" / "claude-code-review.yml" +ALLOWED_USERS_INPUT = "allowed_non_write_users: ${{ github.event.pull_request.user.login }}" + + +class ClaudeCodeReviewWorkflowContractTest(unittest.TestCase): + def test_fork_review_forwards_allowlist_to_claude_action(self) -> None: + workflow = WORKFLOW.read_text(encoding="utf-8") + _, separator, fork_job = workflow.partition(" claude-review-fork:") + + self.assertTrue(separator, "Claude fork-review job is missing") + self.assertIn( + ALLOWED_USERS_INPUT, + fork_job, + "fork review must forward its job-authorized pull request author " + "to Claude's allowed_non_write_users input", + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 7da44288e6..701676e921 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -153,6 +153,10 @@ jobs: # enough for the action to post review comments. Author will be # github-actions[bot] on this path. github_token: ${{ secrets.GITHUB_TOKEN }} + # The job gate above approves this fork author through either the + # repository allowlist or a maintainer's safe-to-review label. Pass + # that approved actor through Claude's independent permission gate. + allowed_non_write_users: ${{ github.event.pull_request.user.login }} track_progress: ${{ github.event.action != 'labeled' }} allowed_bots: "*" prompt: | diff --git a/.github/workflows/lint-action-pinning.yml b/.github/workflows/lint-action-pinning.yml index fe6a711e31..084461b0a8 100644 --- a/.github/workflows/lint-action-pinning.yml +++ b/.github/workflows/lint-action-pinning.yml @@ -7,6 +7,7 @@ on: - ".github/workflows/**" - ".github/scripts/lint-action-pinning.py" - ".github/scripts/lint-action-pinning_test.py" + - ".github/scripts/claude-code-review-workflow-contract_test.py" - ".github/scripts/collect-macos-desktop-diagnostics.sh" - ".github/scripts/release-workflow-contract_test.py" - ".github/release-signing-key.asc" @@ -17,6 +18,7 @@ on: - ".github/workflows/**" - ".github/scripts/lint-action-pinning.py" - ".github/scripts/lint-action-pinning_test.py" + - ".github/scripts/claude-code-review-workflow-contract_test.py" - ".github/scripts/collect-macos-desktop-diagnostics.sh" - ".github/scripts/release-workflow-contract_test.py" - ".github/release-signing-key.asc" @@ -44,6 +46,9 @@ jobs: - name: Test action-pinning lint run: python3 .github/scripts/lint-action-pinning_test.py + - name: Test Claude Code review workflow contract + run: python3 .github/scripts/claude-code-review-workflow-contract_test.py + - name: Test release workflow contract run: python3 .github/scripts/release-workflow-contract_test.py diff --git a/docs/plans/claude-fork-review-allowlist/plan.md b/docs/plans/claude-fork-review-allowlist/plan.md new file mode 100644 index 0000000000..d761082e1b --- /dev/null +++ b/docs/plans/claude-fork-review-allowlist/plan.md @@ -0,0 +1,39 @@ +--- +spec: docs/specs/claude-fork-review-allowlist/spec.md +created: 2026-07-30 +status: complete +--- + +# Implementation Plan: Claude Fork Review Allowlist + +## Overview + +The fork review job already authorizes trusted contributors at its job-level condition, but it does not forward that authorization into `anthropics/claude-code-action`. Add a focused workflow contract test first, then pass the already job-authorized pull request author to the action's `allowed_non_write_users` input. + +## Confirmed root cause + +PR #2072 is a cross-repository pull request authored and synchronized by `ClemDNL`. The repository variable is valid JSON and contains that login, which is why `claude-review-fork` started. Its log then showed an empty `ALLOWED_NON_WRITE_USERS`, resolved `ClemDNL` to repository permission `read`, and failed with `Actor does not have write permissions to the repository`. + +## Workflow + +- Add `.github/scripts/claude-code-review-workflow-contract_test.py` to assert that the fork job forwards its approved pull request author to the action's `allowed_non_write_users` input. +- Update `.github/workflows/claude-code-review.yml` to pass `github.event.pull_request.user.login` to the fork action. +- Update `.github/workflows/lint-action-pinning.yml` so workflow changes run the new contract test in CI. + +## Tests + +- **What:** An approved non-write fork author reaches Claude's internal permission bypass without evaluating the optional JSON allowlist again in the label-only path. +- **File:** `.github/scripts/claude-code-review-workflow-contract_test.py` +- **How:** A focused Python contract test reads the fork job and requires the exact `allowed_non_write_users` expression. It must fail before the workflow change and pass afterward. +- **Commands:** + - `python3 .github/scripts/claude-code-review-workflow-contract_test.py` + - `python3 .github/scripts/lint-action-pinning.py` + +## Implementation + +- [x] [Task 01: Forward the approved fork allowlist](task-01-forward-fork-allowlist.md) + +## Risks + +- GitHub expression syntax is runtime-specific. The contract test pins the expected expression, while final proof still requires a new fork workflow run after the change reaches the base branch. +- Re-evaluating the JSON variable in the action input could break the independent label-only path when that optional variable is empty. Passing the actor already approved by the job gate avoids that dependency. diff --git a/docs/plans/claude-fork-review-allowlist/task-01-forward-fork-allowlist.md b/docs/plans/claude-fork-review-allowlist/task-01-forward-fork-allowlist.md new file mode 100644 index 0000000000..ba04126b20 --- /dev/null +++ b/docs/plans/claude-fork-review-allowlist/task-01-forward-fork-allowlist.md @@ -0,0 +1,46 @@ +--- +id: "01-forward-fork-allowlist" +title: "Forward the approved fork allowlist" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/claude-fork-review-allowlist/spec.md" +--- + +# Task 01: Forward the approved fork allowlist + +## Acceptance + +- The fork Claude review action receives the job-approved pull request author through `allowed_non_write_users`. +- The job-authorized pull request author is passed to the pinned Claude action without re-evaluating the optional JSON repository variable. +- A focused contract test fails without the input, passes with it, and is run by the workflow-lint CI job. + +## Verification + +- `python3 .github/scripts/claude-code-review-workflow-contract_test.py` +- `python3 .github/scripts/lint-action-pinning.py` + +## Files likely touched + +- `.github/workflows/claude-code-review.yml` +- `.github/workflows/lint-action-pinning.yml` +- `.github/scripts/claude-code-review-workflow-contract_test.py` + +## Dependencies + +None. + +## Parallelism + +`sequential`; the test and both workflow changes form one contract. + +## Inputs + +- Spec scenarios in `docs/specs/claude-fork-review-allowlist/spec.md`. +- Failed `claude-review-fork` job 90922436522 from workflow run 30557734099. +- Pinned action input contract: `allowed_non_write_users` accepts comma-separated usernames only when `github_token` is provided. + +## Output contract + +Report the workflow expression added, files changed, RED and GREEN test results, action-pinning lint result, residual need for a live fork workflow run, and task/plan status updates. diff --git a/docs/specs/claude-fork-review-allowlist/spec.md b/docs/specs/claude-fork-review-allowlist/spec.md new file mode 100644 index 0000000000..742f26e29b --- /dev/null +++ b/docs/specs/claude-fork-review-allowlist/spec.md @@ -0,0 +1,32 @@ +--- +status: building +created: 2026-07-30 +owner: kdlbs +--- + +# Claude Fork Review Allowlist + +## Why + +Trusted fork contributors listed in the repository's Claude review allowlist should receive automatic reviews without requiring a maintainer to label every commit. The workflow currently starts for those contributors but the Claude action rejects them because its separate non-write-user permission gate receives no allowlist. + +## What + +- A fork pull request actor listed in `CLAUDE_REVIEW_ALLOWLIST` passes both the workflow job gate and the Claude action's non-write-user gate. +- `CLAUDE_REVIEW_ALLOWLIST` remains a JSON array for safe use in GitHub Actions expressions. +- The Claude action receives the already job-authorized pull request author through its `allowed_non_write_users` input. +- The existing `safe-to-review` label path and same-repository review path remain unchanged. +- Empty, malformed, or non-matching allowlists continue to fail closed at the workflow job gate. + +## Scenarios + +- **GIVEN** `CLAUDE_REVIEW_ALLOWLIST` is `["ClemDNL"]`, **WHEN** `ClemDNL` synchronizes a fork pull request, **THEN** the fork review job supplies `ClemDNL` through `allowed_non_write_users` and Claude does not reject the run solely because the actor has repository permission `read`. +- **GIVEN** a fork contributor is absent from `CLAUDE_REVIEW_ALLOWLIST`, **WHEN** they synchronize their pull request without a current `safe-to-review` approval, **THEN** the fork review job does not run. +- **GIVEN** a maintainer applies `safe-to-review` to a fork pull request, **WHEN** the labeled event runs, **THEN** the existing maintainer-approved review path remains available. + +## Out of scope + +- Changing the pinned Claude Code Action version. +- Changing the Claude OAuth token, GitHub token, or OIDC strategy. +- Changing the OpenCode review workflow. +- Changing review behavior for same-repository pull requests.