diff --git a/.github/scripts/opencode-code-review-workflow-contract_test.py b/.github/scripts/opencode-code-review-workflow-contract_test.py new file mode 100644 index 0000000000..09fab4ff4a --- /dev/null +++ b/.github/scripts/opencode-code-review-workflow-contract_test.py @@ -0,0 +1,50 @@ +#!/usr/bin/env python3 +"""Contract tests for the OpenCode fork-review workflow.""" + +from pathlib import Path +import unittest + + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOW = REPO_ROOT / ".github" / "workflows" / "opencode-code-review.yml" +LINT_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "lint-action-pinning.yml" + + +class OpenCodeCodeReviewWorkflowContractTest(unittest.TestCase): + def test_safe_to_review_approval_survives_follow_up_pushes(self) -> None: + workflow = WORKFLOW.read_text(encoding="utf-8") + _, separator, fork_job = workflow.partition(" opencode-review-fork:") + + self.assertTrue(separator, "OpenCode fork-review job is missing") + self.assertIn("github.event_name == 'pull_request_target'", fork_job) + self.assertIn( + "github.event.pull_request.head.repo.full_name != github.repository", + fork_job, + ) + self.assertIn( + "((contains(github.event.pull_request.labels.*.name, 'safe-to-review')) ||", + fork_job, + ) + self.assertNotIn( + "github.event.action != 'synchronize' && contains(" + "github.event.pull_request.labels.*.name, 'safe-to-review')", + fork_job, + ) + + def test_workflow_has_no_synchronize_time_label_cleanup(self) -> None: + workflow = WORKFLOW.read_text(encoding="utf-8") + + self.assertNotIn(" strip-safe-to-review:", workflow) + self.assertNotIn("github.rest.issues.removeLabel", workflow) + + def test_lint_workflow_runs_opencode_contract_test(self) -> None: + workflow = LINT_WORKFLOW.read_text(encoding="utf-8") + + self.assertIn( + "python3 .github/scripts/opencode-code-review-workflow-contract_test.py", + workflow, + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/scripts/preview-env-workflow-contract_test.py b/.github/scripts/preview-env-workflow-contract_test.py index 53c76ec458..488acc4815 100644 --- a/.github/scripts/preview-env-workflow-contract_test.py +++ b/.github/scripts/preview-env-workflow-contract_test.py @@ -31,6 +31,18 @@ class PreviewEnvironmentWorkflowContractTest(unittest.TestCase): deploy_job, ) + def test_safe_to_test_approval_survives_follow_up_pushes(self) -> None: + workflow = WORKFLOW.read_text(encoding="utf-8") + _, separator, deploy_job = workflow.partition(" deploy-fork:") + + self.assertTrue(separator, "Fork preview deploy job is missing") + self.assertIn( + "((contains(github.event.pull_request.labels.*.name, 'safe-to-test')) ||", + deploy_job, + ) + self.assertNotIn(" strip-safe-to-test:", workflow) + self.assertNotIn("github.rest.issues.removeLabel", workflow) + if __name__ == "__main__": unittest.main() diff --git a/.github/workflows/lint-action-pinning.yml b/.github/workflows/lint-action-pinning.yml index ab571bb6a1..a5facc5216 100644 --- a/.github/workflows/lint-action-pinning.yml +++ b/.github/workflows/lint-action-pinning.yml @@ -53,6 +53,9 @@ jobs: - name: Test preview environment workflow contract run: python3 .github/scripts/preview-env-workflow-contract_test.py + - name: Test OpenCode code review workflow contract + run: python3 .github/scripts/opencode-code-review-workflow-contract_test.py + - name: Test frontend tests workflow contract run: python3 .github/scripts/frontend-tests-workflow-contract_test.py diff --git a/.github/workflows/opencode-code-review.yml b/.github/workflows/opencode-code-review.yml index b13455533a..1425c5bf4f 100644 --- a/.github/workflows/opencode-code-review.yml +++ b/.github/workflows/opencode-code-review.yml @@ -6,7 +6,7 @@ on: 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 - # do not re-use the stale label — see strip-safe-to-review below. + # re-use the maintainer's approval for the new head. types: [opened, synchronize, ready_for_review, reopened, labeled] concurrency: @@ -239,48 +239,19 @@ jobs: path: .opencode-review/ if-no-files-found: ignore - # Strip safe-to-review label on new pushes to fork PRs. - # Together with the `github.event.action != 'synchronize'` guard on the - # fork review job's label path, this gives per-commit maintainer opt-in. - 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@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - 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.'); - } - opencode-review-fork: # Fork PRs only, via pull_request_target so OPENCODE_API_KEY is available. # Two approval paths: # - OPENCODE_REVIEW_ALLOWLIST repo variable (trusted login list, all events; # JSON array, e.g. ["alice","bob"]) - # - `safe-to-review` label (maintainer opt-in, per-commit). Explicitly - # blocked on `synchronize` so a contributor push cannot re-use a stale - # label — the maintainer must re-apply it after each push. + # - `safe-to-review` label (maintainer opt-in, durable until removed). + # It authorizes the current and subsequent fork heads. if: > vars.OPENCODE_REVIEW_ENABLED == 'true' && github.event_name == 'pull_request_target' && github.event.pull_request.draft == false && github.event.pull_request.head.repo.full_name != github.repository && - ((github.event.action != 'synchronize' && contains(github.event.pull_request.labels.*.name, 'safe-to-review')) || + ((contains(github.event.pull_request.labels.*.name, 'safe-to-review')) || (vars.OPENCODE_REVIEW_ALLOWLIST != '' && contains(fromJSON(vars.OPENCODE_REVIEW_ALLOWLIST), github.event.pull_request.user.login))) runs-on: ubuntu-latest timeout-minutes: 15 diff --git a/.github/workflows/preview-env.yml b/.github/workflows/preview-env.yml index c8dc7e0dc9..6211f83810 100644 --- a/.github/workflows/preview-env.yml +++ b/.github/workflows/preview-env.yml @@ -10,8 +10,7 @@ on: pull_request_target: # `labeled` is needed so a maintainer applying `safe-to-test` to a fork PR # triggers a deploy against the current head SHA. Subsequent fork pushes - # (synchronize) do NOT re-trigger the label path — see deploy-fork's - # if-gate and strip-safe-to-test below. + # (synchronize) re-use the maintainer's approval for the new head. types: [opened, synchronize, reopened, closed, labeled] concurrency: @@ -73,12 +72,10 @@ jobs: # JSON array of logins) # - CLAUDE_REVIEW_ALLOWLIST repo variable (trusted login list, all events; # the Claude workflow also adds the approval labels on open) - # - `safe-to-test` 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 deploy of the new SHA. This - # matters more here than for claude-code-review.yml because deploy - # actually runs the fork's `cmd/preview` code with SPRITES_API_TOKEN. + # - `safe-to-test` label (maintainer opt-in, durable until removed). It + # authorizes the current and subsequent fork heads. This matters more + # here than for claude-code-review.yml because deploy actually runs the + # fork's `cmd/preview` code with SPRITES_API_TOKEN. # ----------------------------------------------------------------------- # Branch order matters: keep cheap/safe checks before `fromJSON(vars.*)`, # which throws (and skips the whole job) if the repo variable is @@ -88,7 +85,7 @@ jobs: github.event.action != 'closed' && 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-test')) || + ((contains(github.event.pull_request.labels.*.name, 'safe-to-test')) || (vars.PREVIEW_ENV_ALLOWLIST != '' && contains(fromJSON(vars.PREVIEW_ENV_ALLOWLIST), github.event.pull_request.user.login)) || (vars.CLAUDE_REVIEW_ALLOWLIST != '' && contains(fromJSON(vars.CLAUDE_REVIEW_ALLOWLIST), github.event.pull_request.user.login))) runs-on: ubuntu-latest @@ -104,8 +101,8 @@ jobs: # NOTE: The allowlist gate (safe-to-test, PREVIEW_ENV_ALLOWLIST, # CLAUDE_REVIEW_ALLOWLIST) # authorizes running the fork's cmd/preview code with SPRITES_API_TOKEN and GH_TOKEN. - # The safe-to-test label must be re-applied after each new push for - # the label-only path (see strip-safe-to-test job). + # The safe-to-test label remains an explicit maintainer approval for + # follow-up pushes until the maintainer removes it. repository: ${{ github.event.pull_request.head.repo.full_name }} ref: ${{ github.event.pull_request.head.sha }} @@ -135,39 +132,6 @@ jobs: SPRITES_API_TOKEN: ${{ secrets.SPRITES_API_TOKEN }} GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - # ----------------------------------------------------------------------- - # Strip safe-to-test label on new pushes to fork PRs. - # Together with the `github.event.action != 'synchronize'` guard on the - # label path in deploy-fork's if-gate, this gives true per-commit opt-in: - # the maintainer must re-apply `safe-to-test` after each push for the - # deploy job to run again. A new push on its own never re-triggers deploy - # on the label path. - # ----------------------------------------------------------------------- - strip-safe-to-test: - 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-test') - runs-on: ubuntu-latest - permissions: - pull-requests: write - steps: - - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 - 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-test', - }); - } catch (error) { - if (error.status !== 404) throw error; - core.info('safe-to-test label already absent; nothing to remove.'); - } - # ----------------------------------------------------------------------- # Cleanup (all closed PRs — no allowlist check) # The sprite must be destroyed regardless of how the PR was closed or diff --git a/docs/decisions/2026-08-07-claude-allowlist-label-bridge.md b/docs/decisions/2026-08-07-claude-allowlist-label-bridge.md index 1a8df90515..41bdf55602 100644 --- a/docs/decisions/2026-08-07-claude-allowlist-label-bridge.md +++ b/docs/decisions/2026-08-07-claude-allowlist-label-bridge.md @@ -1,6 +1,6 @@ # ADR-2026-08-07-claude-allowlist-label-bridge: Use the Claude Allowlist as a Trusted Preview Gate -**Status:** accepted +**Status:** accepted (amended by 2026-08-22-persistent-fork-approval-labels) **Date:** 2026-08-07 **Area:** infra, workflow, security @@ -30,9 +30,9 @@ non-closed pull-request-target events. The labels remain visible approval markers and continue to support the manual label paths; privileged workflow execution does not depend on a recursive `labeled` event. -The new labeling job applies only to fork pull requests on open. Existing -per-commit cleanup and manual re-approval behavior for `safe-to-test` remains -unchanged. +The new labeling job applies only to fork pull requests on open. Approval-label +persistence and follow-up push behavior are defined by +2026-08-22-persistent-fork-approval-labels. ## Consequences diff --git a/docs/decisions/2026-08-22-persistent-fork-approval-labels.md b/docs/decisions/2026-08-22-persistent-fork-approval-labels.md new file mode 100644 index 0000000000..7aee7703fe --- /dev/null +++ b/docs/decisions/2026-08-22-persistent-fork-approval-labels.md @@ -0,0 +1,54 @@ +# ADR-2026-08-22-persistent-fork-approval-labels: Persist Fork Approval Labels Across Pushes + +**Status:** accepted +**Date:** 2026-08-22 +**Area:** infra, workflow, security + +## Context + +The `safe-to-test` and `safe-to-review` labels are explicit maintainer approval +markers for fork pull requests. The preview and OpenCode workflows currently +remove those labels on every `pull_request_target` `synchronize` event, which +forces maintainers to repeat the same approval after routine follow-up commits. +This does not match the repository's normal review workflow, where contributors +often push several fixes before a pull request is complete. + +## Decision + +Treat `safe-to-test` and `safe-to-review` as durable maintainer approvals that +remain on a fork pull request until a maintainer removes them. The preview and +OpenCode fork jobs evaluate those labels on `synchronize` events, so an approved +fork pull request remains eligible for the current head after follow-up pushes. + +Remove the per-commit label cleanup jobs and the corresponding synchronize +exclusions from `.github/workflows/preview-env.yml` and +`.github/workflows/opencode-code-review.yml`. + +The direct `PREVIEW_ENV_ALLOWLIST`, `CLAUDE_REVIEW_ALLOWLIST`, and +`OPENCODE_REVIEW_ALLOWLIST` paths remain independent authorization sources. +Labels added by `GITHUB_TOKEN` still do not recursively trigger another +workflow run. The Claude review workflow keeps its existing open/labeled-only +follow-up policy; label persistence alone does not make Claude review every +push. + +## Consequences + +- Maintainers apply each approval label once and can push multiple follow-up + commits without repeating the approval. +- An approved fork push can run the preview code with its existing deployment + credentials and can start the existing OpenCode review path for the new head. +- Maintainers must remove the relevant label when approval is revoked; there is + no automatic per-commit safety reset. +- The workflow contract tests must protect both durable label presence and + synchronize-event eligibility. + +## Alternatives Considered + +- Keep per-commit cleanup and require re-approval after every push. Rejected + because it creates repeated maintenance work for normal review iterations. +- Persist labels but keep synchronize exclusions. Rejected because the labels + would remain visible while not authorizing the workflows on the events that + matter for follow-up commits. +- Replace labels with a new token or GitHub App approval mechanism. Rejected + because it adds another credential and does not improve the requested + maintainer workflow. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index 035e520963..37bfb1e2b0 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -187,4 +187,5 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-19-repository-qualified-comparison-targets | [Qualify Git Comparison Targets by Repository](2026-08-19-repository-qualified-comparison-targets.md) | accepted | backend, agentctl, frontend, protocol, GitHub, GitLab | 2026-08-19 | | 2026-08-20-startup-configuration-source-parity | [Startup Configuration Uses One Typed Source Model](2026-08-20-startup-configuration-source-parity.md) | accepted | backend, agentctl, frontend, cli, security, operations | 2026-08-20 | | 2026-08-20-acp-client-non-underscore-extension-methods | [Route Non-Underscore Inbound Client Methods to the Extension Handler](2026-08-20-acp-client-non-underscore-extension-methods.md) | accepted | backend, protocol | 2026-08-20 | +| 2026-08-22-persistent-fork-approval-labels | [Persist Fork Approval Labels Across Pushes](2026-08-22-persistent-fork-approval-labels.md) | accepted | infra, workflow, security | 2026-08-22 | | 2026-08-22-plugin-version-retention | [Keep Exactly One Superseded Plugin Version](2026-08-22-plugin-version-retention.md) | accepted | backend | 2026-08-22 | diff --git a/docs/plans/persistent-fork-approval-labels/plan.md b/docs/plans/persistent-fork-approval-labels/plan.md new file mode 100644 index 0000000000..5ae43b337b --- /dev/null +++ b/docs/plans/persistent-fork-approval-labels/plan.md @@ -0,0 +1,122 @@ +--- +spec: docs/specs/claude-fork-review-allowlist/spec.md +created: 2026-08-22 +status: implemented +--- + +# Implementation Plan: Persist Fork Approval Labels + +## Overview + +Change the fork approval workflows so `safe-to-test` and `safe-to-review` are +durable maintainer approvals. Remove the synchronize-time cleanup jobs, allow +the existing label paths on follow-up pushes, and add contract coverage that +prevents either workflow from silently returning to per-commit re-approval. +Update the related security decision and allowlist spec in the same change. + +## Confirmed root cause + +PR #2815 is a fork pull request. The GitHub audit trail shows both labels added +by a maintainer and later removed by `github-actions[bot]` after contributor +pushes. The corresponding successful workflow runs executed +`strip-safe-to-test` and `strip-safe-to-review`. The jobs and the +`event.action != 'synchronize'` guards are explicit in the current workflow +files. + +## Workflow changes + +- `.github/workflows/preview-env.yml` + - Keep `safe-to-test` in the `deploy-fork` authorization expression. + - Remove the `github.event.action != 'synchronize'` exclusion from that label + path so an existing approval authorizes the current fork head after pushes. + - Remove the `strip-safe-to-test` job and stale per-commit comments. +- `.github/workflows/opencode-code-review.yml` + - Keep `safe-to-review` in the fork review authorization expression. + - Remove the synchronize exclusion from that label path. + - Remove the `strip-safe-to-review` job and stale per-commit comments. +- `.github/workflows/lint-action-pinning.yml` + - Run the OpenCode workflow contract test in the existing always-on workflow + contract suite. + +## Contract tests + +- `.github/scripts/preview-env-workflow-contract_test.py` + - Assert the preview workflow keeps the label authorization path. + - Assert the synchronize exclusion and `strip-safe-to-test` job are absent. +- `.github/scripts/opencode-code-review-workflow-contract_test.py` + - Add focused coverage for the fork label gate, synchronize eligibility, and + absence of `strip-safe-to-review`/`removeLabel` cleanup. +- Keep the Claude workflow contract test passing to prove the separate + open/labeled-only Claude review policy remains unchanged. + +## Documentation and decision records + +- Amend `docs/specs/claude-fork-review-allowlist/spec.md` with durable-label + scenarios and the changed OpenCode/preview behavior. +- Amend `docs/decisions/2026-08-07-claude-allowlist-label-bridge.md` to point + the superseded per-commit clause at the new decision. +- Add `docs/decisions/2026-08-22-persistent-fork-approval-labels.md` and index + it in `docs/decisions/INDEX.md`. + +## Tests + +- **What:** The preview label path authorizes a fork `synchronize` event and no + cleanup job removes `safe-to-test`. + **File:** `.github/scripts/preview-env-workflow-contract_test.py`. + **How:** Read the workflow text and assert the label expression remains, the + synchronize exclusion is absent, and the cleanup job is absent. +- **What:** The OpenCode label path authorizes a fork `synchronize` event and + no cleanup job removes `safe-to-review`. + **File:** `.github/scripts/opencode-code-review-workflow-contract_test.py`. + **How:** Read the workflow text and assert the trigger, label gate, and + absence of cleanup and synchronize exclusion. +- **What:** Existing fork allowlist labeling and Claude review behavior remain + intact. + **File:** `.github/scripts/claude-code-review-workflow-contract_test.py`. + **How:** Run the existing contract suite without changing its open/labeled + assertions. + +## Verification commands + +Run from the repository root: + +```bash +python3 .github/scripts/preview-env-workflow-contract_test.py +python3 .github/scripts/opencode-code-review-workflow-contract_test.py +python3 .github/scripts/claude-code-review-workflow-contract_test.py +python3 .github/scripts/lint-action-pinning_test.py +python3 .github/scripts/lint-action-pinning.py +zizmor .github/workflows +git diff --check +``` + +## Implementation Waves + +Wave 1 (sequential; one coupled security-boundary change): + +- [x] [Task 01: Persist fork approval labels](task-01-persist-fork-approval-labels.md) + +## Risks and out of scope + +- This intentionally makes a maintainer label approval apply to future fork + heads. Maintainers must remove labels when trust should be revoked. +- `CLAUDE_REVIEW_ALLOWLIST` and the other direct allowlist paths remain + independent and fail-closed. +- The Claude review workflow does not gain a `synchronize` trigger in this + change. This task does not alter same-repository review behavior or add a new + token/App-based label event bridge. + +## Verification Results + +- RED: The new preview and OpenCode contract assertions failed against the + pre-change workflows because the synchronize exclusions and cleanup jobs + were still present. +- GREEN: `preview-env-workflow-contract_test.py` (2 tests), + `opencode-code-review-workflow-contract_test.py` (3 tests), and the existing + `claude-code-review-workflow-contract_test.py` (9 tests) pass. +- Security checks: `lint-action-pinning_test.py` (9 tests) and + `lint-action-pinning.py` pass; `git diff --check` passes. +- `zizmor .github/workflows` ran but exits non-zero on existing repository-wide + findings, including pre-existing dangerous-trigger, permissions, artifact, + cache, and template-injection findings. This change did not expand that + audit scope. diff --git a/docs/plans/persistent-fork-approval-labels/task-01-persist-fork-approval-labels.md b/docs/plans/persistent-fork-approval-labels/task-01-persist-fork-approval-labels.md new file mode 100644 index 0000000000..bc24f767a3 --- /dev/null +++ b/docs/plans/persistent-fork-approval-labels/task-01-persist-fork-approval-labels.md @@ -0,0 +1,91 @@ +--- +id: "01-persist-fork-approval-labels" +title: "Persist fork approval labels" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/claude-fork-review-allowlist/spec.md" +--- + +# Task 01: Persist fork approval labels + +## Acceptance + +- `safe-to-test` remains on a fork pull request after a contributor push and + keeps the preview fork job eligible for the current head on `synchronize`. +- `safe-to-review` remains on a fork pull request after a contributor push and + keeps the OpenCode fork-review job eligible for the current head on + `synchronize`. +- Neither workflow contains a synchronize-time label cleanup job, and the + existing Claude open/labeled-only review policy remains unchanged. + +## Verification + +Run from the repository root: + +```bash +python3 .github/scripts/preview-env-workflow-contract_test.py +python3 .github/scripts/opencode-code-review-workflow-contract_test.py +python3 .github/scripts/claude-code-review-workflow-contract_test.py +python3 .github/scripts/lint-action-pinning_test.py +python3 .github/scripts/lint-action-pinning.py +zizmor .github/workflows +git diff --check +``` + +## Files likely touched + +- `.github/workflows/preview-env.yml` +- `.github/workflows/opencode-code-review.yml` +- `.github/workflows/lint-action-pinning.yml` +- `.github/scripts/preview-env-workflow-contract_test.py` +- `.github/scripts/opencode-code-review-workflow-contract_test.py` +- `docs/specs/claude-fork-review-allowlist/spec.md` +- `docs/decisions/2026-08-07-claude-allowlist-label-bridge.md` +- `docs/decisions/2026-08-22-persistent-fork-approval-labels.md` +- `docs/decisions/INDEX.md` + +## Dependencies + +None. The root cause and desired behavior are confirmed. + +## Parallelism + +`sequential`; workflow gates, contract tests, and security records form one +coupled change. + +## Inputs + +- `docs/specs/claude-fork-review-allowlist/spec.md` +- `docs/decisions/2026-08-07-claude-allowlist-label-bridge.md` +- `docs/decisions/2026-08-22-persistent-fork-approval-labels.md` +- `.github/AGENTS.md` +- `.github/workflows/preview-env.yml` +- `.github/workflows/opencode-code-review.yml` + +## Risks + +Persisting a label makes a maintainer approval apply to future fork heads. +Do not broaden the workflow conditions beyond the existing external-PR label +gates or remove the direct allowlist paths. + +## Output contract + +Report the exact workflow and contract-test changes, RED and GREEN results for +the focused tests, security lint results, `git diff --check`, any live fork +verification still needed after merge, and synchronized task/plan statuses. + +## Results + +- Removed the preview and OpenCode synchronize-time label cleanup jobs. +- Removed both label-path `synchronize` exclusions so existing maintainer + approvals authorize follow-up fork heads. +- Added preview and OpenCode contract tests, and registered the OpenCode test + in the always-on action-pinning workflow. +- RED passed as an expected failure against the old workflows. GREEN passed + with 2 preview tests, 3 OpenCode tests, and 9 Claude tests. +- `lint-action-pinning_test.py` (9 tests), `lint-action-pinning.py`, and + `git diff --check` pass. +- `zizmor .github/workflows` ran and reported existing repository-wide + findings; it is not a clean baseline gate for this change. diff --git a/docs/specs/claude-fork-review-allowlist/spec.md b/docs/specs/claude-fork-review-allowlist/spec.md index 74974bfddf..676290c5a0 100644 --- a/docs/specs/claude-fork-review-allowlist/spec.md +++ b/docs/specs/claude-fork-review-allowlist/spec.md @@ -6,13 +6,13 @@ owner: kdlbs # Claude Fork Review Allowlist -Decision: ADR-2026-07-31-isolate-manual-pr-review-content; ADR-2026-08-07-claude-allowlist-label-bridge +Decision: ADR-2026-07-31-isolate-manual-pr-review-content; ADR-2026-08-07-claude-allowlist-label-bridge; ADR-2026-08-22-persistent-fork-approval-labels ## Why 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. -The same trusted contributors should also receive the repository's existing review and preview approval markers, so maintainers do not need to maintain a second identity list or apply two labels manually. +The same trusted contributors should also receive the repository's existing review and preview approval markers, so maintainers do not need to maintain a second identity list or apply two labels manually. Maintainers also use those labels for unallowlisted fork pull requests and expect an explicit approval to remain valid across routine follow-up commits. ## What @@ -21,7 +21,9 @@ The same trusted contributors should also receive the repository's existing revi - The Claude action receives the already job-authorized pull request author through its `allowed_non_write_users` input. - When an allowlisted fork pull request opens, the base-controlled workflow adds both existing labels, `safe-to-review` and `safe-to-test`, to the pull request. The label operation is idempotent and applies only to fork pull requests whose opening author matches the allowlist. - The `CLAUDE_REVIEW_ALLOWLIST` gate remains a direct authorization path for the fork review job, so adding labels with the repository `GITHUB_TOKEN` does not create a second review run through the `labeled` event. -- The preview workflow treats an author in `CLAUDE_REVIEW_ALLOWLIST` as trusted for fork preview deployment on every non-closed pull-request-target event it already handles (`opened`, `synchronize`, `reopened`, and `labeled`). This keeps preview deployment automatic even though the existing per-commit `safe-to-test` cleanup removes that label after a fork push. +- The preview workflow treats an author in `CLAUDE_REVIEW_ALLOWLIST` as trusted for fork preview deployment on every non-closed pull-request-target event it already handles (`opened`, `synchronize`, `reopened`, and `labeled`). A maintainer-applied `safe-to-test` label remains valid across later `synchronize` events and also authorizes the preview deployment path for the current fork head. +- A maintainer-applied `safe-to-review` label remains valid across later `synchronize` events and authorizes the existing OpenCode fork-review path for the current fork head. The label is not removed automatically after a push. +- Approval labels remain until a maintainer removes them. A contributor push does not revoke either label or require the maintainer to repeat the same approval for each follow-up commit. - 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 by commenting `@claude review` on the pull request. That requested review reads the current pull request head, including files newly added by the pull request. - Manual pull request reviews keep the trusted default branch at the workflow root and do not check out pull request content. Claude may use read-only local tools for trusted surrounding code, reads the current diff through constrained GitHub commands, and reads complete current-head PR files only through a path-validated, size-limited GET helper bound to the event's PR number. Its only write capability is posting review comments. @@ -33,7 +35,7 @@ The same trusted contributors should also receive the repository's existing revi - Only the base-controlled `pull_request_target` workflow may add the two labels. It does not check out or execute pull-request content for the labeling step. - A matching `CLAUDE_REVIEW_ALLOWLIST` entry authorizes the existing fork review path and the preview workflow's fork deployment path, which has access to the preview deployment credentials. Repository maintainers are responsible for keeping this variable restricted to trusted contributors. -- A non-allowlisted fork author still needs the existing maintainer-applied labels; the new automation does not broaden the manual label paths. +- A non-allowlisted fork author still needs the existing maintainer-applied labels. Those labels authorize the current and subsequent fork heads until a maintainer removes them; maintainers are responsible for revoking approval when the contributor or proposed change is no longer trusted. ## Failure modes @@ -45,7 +47,10 @@ The same trusted contributors should also receive the repository's existing revi - **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** `CLAUDE_REVIEW_ALLOWLIST` is `["ClemDNL"]`, **WHEN** `ClemDNL` opens a fork pull request, **THEN** the pull request receives both `safe-to-review` and `safe-to-test` labels without a maintainer action. -- **GIVEN** an allowlisted fork pull request has a new commit pushed, **WHEN** the preview workflow handles the `synchronize` event, **THEN** the preview deploy job remains eligible through `CLAUDE_REVIEW_ALLOWLIST` even if the existing cleanup removes `safe-to-test`. +- **GIVEN** an allowlisted fork pull request has a new commit pushed, **WHEN** the preview workflow handles the `synchronize` event, **THEN** the preview deploy job remains eligible through `CLAUDE_REVIEW_ALLOWLIST` and any existing approval labels remain present. +- **GIVEN** an unallowlisted fork pull request has `safe-to-test`, **WHEN** its contributor pushes a follow-up commit, **THEN** `safe-to-test` remains present and the preview deploy job is eligible for the current head on the `synchronize` event. +- **GIVEN** an unallowlisted fork pull request has `safe-to-review`, **WHEN** its contributor pushes a follow-up commit, **THEN** `safe-to-review` remains present and the OpenCode fork-review job is eligible for the current head on the `synchronize` event. +- **GIVEN** a maintainer removes an approval label from a fork pull request, **WHEN** the contributor pushes another commit, **THEN** the corresponding workflow path is not authorized by that removed label. - **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 fork contributor is absent from `CLAUDE_REVIEW_ALLOWLIST`, **WHEN** they open a pull request, **THEN** the automatic labeling job adds neither approval label and the preview workflow does not deploy unless its existing `PREVIEW_ENV_ALLOWLIST` or `safe-to-test` path authorizes it. @@ -61,6 +66,5 @@ The same trusted contributors should also receive the repository's existing revi - Changing the pinned Claude Code Action version. - Changing the Claude OAuth token, GitHub token, or OIDC strategy. - Creating a second preview-specific allowlist or removing `PREVIEW_ENV_ALLOWLIST`. -- Re-adding approval labels after every fork push or using a new personal access token/GitHub App solely to make label events recurse into other workflows. -- Changing the OpenCode review workflow. +- Changing the Claude automatic follow-up review policy; approval-label persistence does not add a `synchronize` trigger to the Claude review workflow. - Changing the automatic review behavior for same-repository pull requests.