diff --git a/.github/scripts/claude-code-review-workflow-contract_test.py b/.github/scripts/claude-code-review-workflow-contract_test.py index cab2f8d6ee..c74d87ef62 100644 --- a/.github/scripts/claude-code-review-workflow-contract_test.py +++ b/.github/scripts/claude-code-review-workflow-contract_test.py @@ -2,15 +2,61 @@ """Contract tests for the Claude Code fork-review workflow.""" from pathlib import Path +import re import unittest REPO_ROOT = Path(__file__).resolve().parents[2] WORKFLOW = REPO_ROOT / ".github" / "workflows" / "claude-code-review.yml" +MENTION_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "claude.yml" ALLOWED_USERS_INPUT = "allowed_non_write_users: ${{ github.event.pull_request.user.login }}" +def activity_types(workflow: str, event: str) -> list[str]: + trigger_section = workflow.partition("on:\n")[2].partition("\nconcurrency:")[0] + event_block = trigger_section.partition(f" {event}:\n")[2] + event_block = re.split(r"\n [a-z_]+:\n", event_block, maxsplit=1)[0] + match = re.search(r"^ types: \[([^]]+)]$", event_block, re.MULTILINE) + if match is None: + raise AssertionError(f"{event} activity types are missing") + return [activity.strip() for activity in match.group(1).split(",")] + + class ClaudeCodeReviewWorkflowContractTest(unittest.TestCase): + def test_review_workflow_ignores_pr_updates(self) -> None: + workflow = WORKFLOW.read_text(encoding="utf-8") + + self.assertEqual(["opened"], activity_types(workflow, "pull_request")) + self.assertEqual( + ["opened", "labeled"], + activity_types(workflow, "pull_request_target"), + ) + self.assertNotIn(" strip-safe-to-review:", workflow) + + def test_fork_review_uses_only_open_or_safe_to_review_label(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( + "github.event.action == 'labeled' && github.event.label.name == 'safe-to-review'", + fork_job, + ) + self.assertRegex( + fork_job, + r"github\.event\.action == 'opened' &&\s+" + r"vars\.CLAUDE_REVIEW_ALLOWLIST != ''", + ) + + def test_manual_claude_mention_can_request_another_review(self) -> None: + workflow = MENTION_WORKFLOW.read_text(encoding="utf-8") + + self.assertIn("issue_comment:\n types: [created]", workflow) + self.assertIn( + "contains(github.event.comment.body, '@claude')", + workflow, + ) + 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:") diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 701676e921..b919264f17 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -9,17 +9,16 @@ name: Claude Code Review # available on the pull_request trigger. This path uses GITHUB_TOKEN # to bypass the OIDC exchange; findings are issue comments authored by # github-actions[bot]. -# Each job gates on github.event_name AND head.repo so exactly one job runs -# per PR push. +# Each job gates on github.event_name AND head.repo so exactly one automatic +# review runs when a PR is opened. Later reviews are requested by commenting +# `@claude review` on the PR. on: pull_request: - types: [opened, synchronize, ready_for_review, reopened] + types: [opened] pull_request_target: - # `labeled` is needed so a maintainer applying `safe-to-review` to a fork - # PR triggers a review against the current head SHA. Subsequent fork pushes - # (synchronize) do NOT re-trigger the label path — see claude-review-fork's - # if-gate and strip-safe-to-review below. - types: [opened, synchronize, ready_for_review, reopened, labeled] + # `labeled` lets a maintainer approve the initial review of an untrusted + # fork by applying `safe-to-review`. Pushes never trigger another review. + types: [opened, labeled] concurrency: group: claude-review-${{ github.event_name }}-${{ github.event.pull_request.number || github.ref }} @@ -74,55 +73,23 @@ jobs: --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*)" --disallowedTools "Edit,Write,MultiEdit,NotebookEdit,WebFetch,WebSearch,Bash(git add:*),Bash(git commit:*),Bash(git push:*),Bash(git rebase:*),Bash(git reset:*),Bash(gh pr edit:*),Bash(curl:*),Bash(wget:*)" - # Strip safe-to-review label on new pushes to fork PRs. - # Together with the `github.event.action != 'synchronize'` guard on the - # label path in claude-review-fork's if-gate, this gives true per-commit - # opt-in: the maintainer must re-apply `safe-to-review` after each push for - # the review job to run again. A new push on its own never re-triggers the - # review on the label path. - strip-safe-to-review: - if: > - github.event_name == 'pull_request_target' && - github.event.action == 'synchronize' && - github.event.pull_request.head.repo.full_name != github.repository && - contains(github.event.pull_request.labels.*.name, 'safe-to-review') - runs-on: ubuntu-latest - permissions: - pull-requests: write - steps: - - uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7 - with: - script: | - try { - await github.rest.issues.removeLabel({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.payload.pull_request.number, - name: 'safe-to-review', - }); - } catch (error) { - if (error.status !== 404) throw error; - core.info('safe-to-review label already absent; nothing to remove.'); - } - claude-review-fork: # Fork PRs only, via pull_request_target so secrets are available. - # Two approval paths: - # - CLAUDE_REVIEW_ALLOWLIST repo variable (trusted login list, all events; + # Two initial-review approval paths: + # - CLAUDE_REVIEW_ALLOWLIST repo variable (trusted login list on open; # JSON array, e.g. ["alice","bob"], in Settings -> Secrets and variables # -> Actions -> Variables) - # - `safe-to-review` label (maintainer opt-in, per-commit). Explicitly - # blocked on `synchronize` so a contributor's push cannot re-use a - # stale label — the maintainer must re-apply it after each push for - # the `labeled` event to trigger a fresh review against the new SHA. + # - `safe-to-review` label (maintainer opt-in for an untrusted fork). # Branch order matters: keep cheap/safe checks before `fromJSON(vars.*)`, # which throws (and skips the whole job) if the repo variable is # non-empty but unparseable JSON. if: > github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name != github.repository && - ((github.event.action != 'synchronize' && contains(github.event.pull_request.labels.*.name, 'safe-to-review')) || - (vars.CLAUDE_REVIEW_ALLOWLIST != '' && contains(fromJSON(vars.CLAUDE_REVIEW_ALLOWLIST), github.event.pull_request.user.login))) + ((github.event.action == 'labeled' && github.event.label.name == 'safe-to-review') || + (github.event.action == 'opened' && + vars.CLAUDE_REVIEW_ALLOWLIST != '' && + contains(fromJSON(vars.CLAUDE_REVIEW_ALLOWLIST), github.event.pull_request.user.login))) runs-on: ubuntu-latest permissions: contents: read diff --git a/docs/specs/claude-fork-review-allowlist/spec.md b/docs/specs/claude-fork-review-allowlist/spec.md index 742f26e29b..e8905fdb1f 100644 --- a/docs/specs/claude-fork-review-allowlist/spec.md +++ b/docs/specs/claude-fork-review-allowlist/spec.md @@ -8,21 +8,25 @@ owner: kdlbs ## 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. +Trusted fork contributors listed in the repository's Claude review allowlist should receive an automatic review when they open a pull request, without requiring a maintainer to label it. Further review rounds should be explicit so pushes do not repeatedly consume Claude tokens. The workflow must pass the already-authorized contributor to Claude's separate non-write-user permission gate. ## 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. +- A fork pull request actor listed in `CLAUDE_REVIEW_ALLOWLIST` receives one automatic review when they open a pull request and 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. +- A maintainer may apply `safe-to-review` to request the initial review of an untrusted fork pull request. +- Pushes, ready-for-review transitions, and reopenings do not automatically start another Claude review. A maintainer can request a later review through the existing Claude mention workflow, for example by commenting `@claude review` on the pull request. +- The same-repository review path remains unchanged except for the open-only trigger policy. - 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** `CLAUDE_REVIEW_ALLOWLIST` is `["ClemDNL"]`, **WHEN** `ClemDNL` opens 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 pull request has received its initial review, **WHEN** its contributor pushes another commit, marks it ready for review, or reopens it, **THEN** the Claude review workflow does not run again. +- **GIVEN** a fork contributor is absent from `CLAUDE_REVIEW_ALLOWLIST`, **WHEN** they open or update their pull request without `safe-to-review`, **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. +- **GIVEN** a maintainer wants another review round, **WHEN** they comment `@claude review` on the pull request, **THEN** the existing Claude mention workflow recognizes the `@claude` mention and starts the requested review. ## Out of scope