ci: review PRs only on open (#2100)
This commit is contained in:
Carlos Florêncio
@@ -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:")
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user