diff --git a/.github/scripts/claude-code-review-workflow-contract_test.py b/.github/scripts/claude-code-review-workflow-contract_test.py index 63a7907d23..0552bbc07a 100644 --- a/.github/scripts/claude-code-review-workflow-contract_test.py +++ b/.github/scripts/claude-code-review-workflow-contract_test.py @@ -41,6 +41,7 @@ class ClaudeCodeReviewWorkflowContractTest(unittest.TestCase): activity_types(workflow, "pull_request_target"), ) self.assertNotIn(" strip-safe-to-review:", workflow) + self.assertEqual(workflow.count("persist-credentials: false"), 2) def test_fork_review_uses_only_open_or_safe_to_review_label(self) -> None: workflow = WORKFLOW.read_text(encoding="utf-8") @@ -137,7 +138,7 @@ class ClaudeCodeReviewWorkflowContractTest(unittest.TestCase): "to Claude's allowed_non_write_users input", ) - def test_allowlisted_fork_label_job_adds_both_approval_labels(self) -> None: + def test_allowlisted_fork_label_job_adds_one_approval_label(self) -> None: workflow = WORKFLOW.read_text(encoding="utf-8") _, separator, label_job = workflow.partition(" label-allowlisted-fork:") label_job = label_job.partition("\n claude-review-fork:")[0] @@ -158,9 +159,10 @@ class ClaudeCodeReviewWorkflowContractTest(unittest.TestCase): self.assertIn("pull-requests: write", label_job) self.assertIn("github.rest.issues.addLabels", label_job) self.assertIn( - "const labels = ['safe-to-review', 'safe-to-test'];", + "const labels = ['safe-to-review'];", label_job, ) + self.assertNotIn("safe-to-test", label_job) self.assertNotIn("actions/checkout", label_job) def test_lint_workflow_runs_preview_contract_test(self) -> None: diff --git a/.github/scripts/opencode-code-review-workflow-contract_test.py b/.github/scripts/opencode-code-review-workflow-contract_test.py index 09fab4ff4a..0c91f98f77 100644 --- a/.github/scripts/opencode-code-review-workflow-contract_test.py +++ b/.github/scripts/opencode-code-review-workflow-contract_test.py @@ -35,6 +35,7 @@ class OpenCodeCodeReviewWorkflowContractTest(unittest.TestCase): workflow = WORKFLOW.read_text(encoding="utf-8") self.assertNotIn(" strip-safe-to-review:", workflow) + self.assertNotIn("safe-to-test", workflow) self.assertNotIn("github.rest.issues.removeLabel", workflow) def test_lint_workflow_runs_opencode_contract_test(self) -> None: diff --git a/.github/scripts/pr-walkthrough-workflow-contract_test.py b/.github/scripts/pr-walkthrough-workflow-contract_test.py index 5569625cbd..b878913f7c 100644 --- a/.github/scripts/pr-walkthrough-workflow-contract_test.py +++ b/.github/scripts/pr-walkthrough-workflow-contract_test.py @@ -51,7 +51,7 @@ class PRWalkthroughWorkflowContractTest(unittest.TestCase): self.assertIn("vars.PR_WALKTHROUGH_ENABLED == 'true'", self.publication) self.assertNotIn("OPENCODE_REVIEW_ENABLED", self.workflow) - def test_generation_is_limited_to_authorized_same_repository_events(self) -> None: + def test_generation_is_limited_to_authorized_repository_events(self) -> None: self.assertIn("pull_request_target:", self.workflow) self.assertIn( "types: [opened, ready_for_review, reopened, synchronize, labeled]", @@ -61,13 +61,20 @@ class PRWalkthroughWorkflowContractTest(unittest.TestCase): "github.event_name == 'pull_request_target'", "github.event.pull_request.draft == false", "github.event.pull_request.head.repo.full_name == github.repository", + "github.event.pull_request.head.repo.full_name != github.repository", "github.event.action == 'opened'", "github.event.action == 'reopened'", "github.event.action == 'ready_for_review'", "github.event.action == 'synchronize'", "github.event.action == 'labeled' && github.event.label.name == 'generate-pr-walkthrough'", + "github.event.label.name == 'safe-to-review'", + "github.event.action != 'labeled'", + "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)", ): self.assertIn(condition, self.generation) + self.assertNotIn("safe-to-test", self.generation) def test_skill_explains_trusted_context_without_provider_names(self) -> None: self.assertNotIn("Kandev", self.skill) @@ -94,7 +101,11 @@ class PRWalkthroughWorkflowContractTest(unittest.TestCase): self.assertIn('test "$(git rev-parse HEAD)" = "$TRUSTED_SHA"', self.generation) self.assertIn( 'git fetch --no-tags --filter=blob:none ' - '--negotiation-tip="$TRUSTED_SHA" origin "$HEAD_SHA"', + '--negotiation-tip="$TRUSTED_SHA" origin "refs/pull/${PR_NUMBER}/head"', + self.generation, + ) + self.assertIn( + 'PR_NUMBER: ${{ github.event.pull_request.number }}', self.generation, ) self.assertNotIn("--depth=1", self.generation) @@ -109,7 +120,7 @@ class PRWalkthroughWorkflowContractTest(unittest.TestCase): self.assertNotIn("github.event.pull_request.base.sha", job) for value in ( - 'git fetch --no-tags --filter=blob:none --negotiation-tip="$TRUSTED_SHA" origin "$HEAD_SHA"', + 'git fetch --no-tags --filter=blob:none --negotiation-tip="$TRUSTED_SHA" origin "refs/pull/${PR_NUMBER}/head"', 'git merge-base "$TRUSTED_SHA" "$HEAD_SHA"', 'git archive "$TRUSTED_SHA" .agents/skills/pr-walkthrough | tar -x', '--base-sha "$TRUSTED_SHA"', diff --git a/.github/scripts/preview-env-workflow-contract_test.py b/.github/scripts/preview-env-workflow-contract_test.py index 488acc4815..963b387819 100644 --- a/.github/scripts/preview-env-workflow-contract_test.py +++ b/.github/scripts/preview-env-workflow-contract_test.py @@ -10,7 +10,7 @@ WORKFLOW = REPO_ROOT / ".github" / "workflows" / "preview-env.yml" class PreviewEnvironmentWorkflowContractTest(unittest.TestCase): - def test_claude_allowlist_authorizes_fork_preview_deployment(self) -> None: + def test_safe_to_review_and_allowlists_authorize_fork_preview_deployment(self) -> None: workflow = WORKFLOW.read_text(encoding="utf-8") _, separator, deploy_job = workflow.partition(" deploy-fork:") @@ -21,7 +21,7 @@ class PreviewEnvironmentWorkflowContractTest(unittest.TestCase): deploy_job, ) self.assertIn( - "contains(github.event.pull_request.labels.*.name, 'safe-to-test')", + "contains(github.event.pull_request.labels.*.name, 'safe-to-review')", deploy_job, ) self.assertIn("vars.PREVIEW_ENV_ALLOWLIST != ''", deploy_job) @@ -30,16 +30,18 @@ class PreviewEnvironmentWorkflowContractTest(unittest.TestCase): "contains(fromJSON(vars.CLAUDE_REVIEW_ALLOWLIST), github.event.pull_request.user.login)", deploy_job, ) + self.assertEqual(workflow.count("persist-credentials: false"), 3) - def test_safe_to_test_approval_survives_follow_up_pushes(self) -> None: + def test_safe_to_review_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')) ||", + "((contains(github.event.pull_request.labels.*.name, 'safe-to-review')) ||", deploy_job, ) + self.assertNotIn("safe-to-test", deploy_job) self.assertNotIn(" strip-safe-to-test:", workflow) self.assertNotIn("github.rest.issues.removeLabel", workflow) diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index 3c7e2d35da..77b0ce8274 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -43,6 +43,7 @@ jobs: uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 with: fetch-depth: 1 + persist-credentials: false - name: Run Claude Code Review (App token, inline comments) id: claude-review @@ -73,9 +74,10 @@ 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:*)" - # Mark allowlisted fork PRs with the approval labels used by the review and - # preview workflows. The direct allowlist gates remain necessary because - # labels written with GITHUB_TOKEN do not start another workflow run. + # Mark allowlisted fork PRs with the single approval label used by the + # review, preview, and walkthrough workflows. The direct allowlist gates + # remain necessary because labels written with GITHUB_TOKEN do not start + # another workflow run. label-allowlisted-fork: if: > github.event_name == 'pull_request_target' && @@ -92,11 +94,11 @@ jobs: pull-requests: write steps: - - name: Add automatic fork approval labels + - name: Add automatic fork approval label uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 with: script: | - const labels = ['safe-to-review', 'safe-to-test']; + const labels = ['safe-to-review']; await github.rest.issues.addLabels({ owner: context.repo.owner, repo: context.repo.repo, @@ -139,6 +141,7 @@ jobs: # claude-code-action only reads files and posts comments. ref: refs/pull/${{ github.event.pull_request.number }}/head fetch-depth: 1 + persist-credentials: false - name: Run Claude Code Review (GITHUB_TOKEN fallback) id: claude-review diff --git a/.github/workflows/pr-walkthrough.yml b/.github/workflows/pr-walkthrough.yml index 56c767980a..ebe5007c13 100644 --- a/.github/workflows/pr-walkthrough.yml +++ b/.github/workflows/pr-walkthrough.yml @@ -21,12 +21,28 @@ jobs: vars.PR_WALKTHROUGH_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 == 'opened' || - github.event.action == 'reopened' || - github.event.action == 'ready_for_review' || - github.event.action == 'synchronize' || - (github.event.action == 'labeled' && github.event.label.name == 'generate-pr-walkthrough')) + ( + ( + github.event.pull_request.head.repo.full_name == github.repository && + (github.event.action == 'opened' || + github.event.action == 'reopened' || + github.event.action == 'ready_for_review' || + github.event.action == 'synchronize' || + (github.event.action == 'labeled' && github.event.label.name == 'generate-pr-walkthrough')) + ) || + ( + github.event.pull_request.head.repo.full_name != github.repository && + ( + (github.event.action == 'labeled' && github.event.label.name == 'safe-to-review') || + (github.event.action != 'labeled' && + ( + 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)) + )) + ) + ) + ) runs-on: ubuntu-latest timeout-minutes: 15 environment: opencode-review-trusted @@ -46,10 +62,11 @@ jobs: env: TRUSTED_SHA: ${{ github.workflow_sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} + PR_NUMBER: ${{ github.event.pull_request.number }} run: | set -euo pipefail test "$(git rev-parse HEAD)" = "$TRUSTED_SHA" - git fetch --no-tags --filter=blob:none --negotiation-tip="$TRUSTED_SHA" origin "$HEAD_SHA" + git fetch --no-tags --filter=blob:none --negotiation-tip="$TRUSTED_SHA" origin "refs/pull/${PR_NUMBER}/head" test "$(git rev-parse FETCH_HEAD)" = "$HEAD_SHA" MERGE_BASE="$(git merge-base "$TRUSTED_SHA" "$HEAD_SHA")" test -n "$MERGE_BASE" diff --git a/.github/workflows/preview-env.yml b/.github/workflows/preview-env.yml index 6211f83810..153e839be8 100644 --- a/.github/workflows/preview-env.yml +++ b/.github/workflows/preview-env.yml @@ -8,7 +8,7 @@ on: pull_request: types: [opened, synchronize, reopened, closed] pull_request_target: - # `labeled` is needed so a maintainer applying `safe-to-test` to a fork PR + # `labeled` is needed so a maintainer applying `safe-to-review` to a fork PR # triggers a deploy against the current head SHA. Subsequent fork pushes # (synchronize) re-use the maintainer's approval for the new head. types: [opened, synchronize, reopened, closed, labeled] @@ -37,6 +37,7 @@ jobs: - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 with: ref: ${{ github.event.pull_request.head.sha }} + persist-credentials: false - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: @@ -72,7 +73,7 @@ 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, durable until removed). It + # - `safe-to-review` 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. @@ -85,7 +86,7 @@ jobs: github.event.action != 'closed' && github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.full_name != github.repository && - ((contains(github.event.pull_request.labels.*.name, 'safe-to-test')) || + ((contains(github.event.pull_request.labels.*.name, 'safe-to-review')) || (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 @@ -98,13 +99,14 @@ jobs: with: # pull_request_target checks out base by default; explicitly use PR head. # Safe: the preview CLI only reads files to build artifacts and posts comments. - # NOTE: The allowlist gate (safe-to-test, PREVIEW_ENV_ALLOWLIST, + # NOTE: The allowlist gate (safe-to-review, 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 remains an explicit maintainer approval for + # The safe-to-review 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 }} + persist-credentials: false - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: @@ -151,6 +153,8 @@ jobs: steps: - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6 + with: + persist-credentials: false - uses: actions/setup-go@924ae3a1cded613372ab5595356fb5720e22ba16 # v6 with: 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 41bdf55602..91974047b7 100644 --- a/docs/decisions/2026-08-07-claude-allowlist-label-bridge.md +++ b/docs/decisions/2026-08-07-claude-allowlist-label-bridge.md @@ -1,9 +1,12 @@ # ADR-2026-08-07-claude-allowlist-label-bridge: Use the Claude Allowlist as a Trusted Preview Gate -**Status:** accepted (amended by 2026-08-22-persistent-fork-approval-labels) +**Status:** accepted (amended by 2026-08-22-persistent-fork-approval-labels and 2026-08-24-unified-fork-approval-label) **Date:** 2026-08-07 **Area:** infra, workflow, security +> This decision records the allowlist bridge. The current label contract is +> defined by [ADR-2026-08-24-unified-fork-approval-label](2026-08-24-unified-fork-approval-label.md). + ## Context Fork pull requests listed in `CLAUDE_REVIEW_ALLOWLIST` already receive an diff --git a/docs/decisions/2026-08-22-persistent-fork-approval-labels.md b/docs/decisions/2026-08-22-persistent-fork-approval-labels.md index 7aee7703fe..479eec4b79 100644 --- a/docs/decisions/2026-08-22-persistent-fork-approval-labels.md +++ b/docs/decisions/2026-08-22-persistent-fork-approval-labels.md @@ -1,9 +1,13 @@ # ADR-2026-08-22-persistent-fork-approval-labels: Persist Fork Approval Labels Across Pushes -**Status:** accepted +**Status:** accepted (amended by 2026-08-24-unified-fork-approval-label) **Date:** 2026-08-22 **Area:** infra, workflow, security +> This decision records the persistence rule. The current approval-label +> contract, including the removal of `safe-to-test`, is defined by +> [ADR-2026-08-24-unified-fork-approval-label](2026-08-24-unified-fork-approval-label.md). + ## Context The `safe-to-test` and `safe-to-review` labels are explicit maintainer approval diff --git a/docs/decisions/2026-08-24-unified-fork-approval-label.md b/docs/decisions/2026-08-24-unified-fork-approval-label.md new file mode 100644 index 0000000000..41713a8999 --- /dev/null +++ b/docs/decisions/2026-08-24-unified-fork-approval-label.md @@ -0,0 +1,85 @@ +# ADR-2026-08-24-unified-fork-approval-label: Use One Maintainer Approval Label for Contributor PR Automation + +**Status:** accepted +**Date:** 2026-08-24 +**Area:** infra, workflow, security + +## Context + +Contributor pull requests currently use two visible labels with different +meanings: `safe-to-review` enables fork AI review and `safe-to-test` enables +fork preview deployment. The PR walkthrough workflow has an additional +same-repository guard, so a contributor pull request can receive review and +preview approval while its walkthrough job is skipped. + +PR #2974 demonstrated the walkthrough failure. It was a cross-repository pull +request and its run was skipped by the explicit +`head.repo.full_name == github.repository` condition. The pull request already +had the existing approval labels when the skipped run was observed. The +problem was the workflow boundary, not a missing walkthrough label. + +Maintainers want one trust decision for contributors they intentionally trust. +The repository also has allowlisted contributors. The label job writes with +`GITHUB_TOKEN`, so a label it adds does not reliably create a new downstream +`labeled` workflow run. + +## Decision + +Use `safe-to-review` as the only maintainer approval label for contributor pull +request AI review, preview deployment, and walkthrough generation. Remove +`safe-to-test` from active authorization expressions and from the automatic +allowlist label bridge. + +Keep the existing direct allowlists as independent authorization paths. The +allowlist label bridge adds only `safe-to-review`. Workflows that need to start +on an opening or update event retain a direct allowlist expression instead of +depending on a recursive label event. + +Extend the walkthrough workflow to authorized contributor pull requests while +preserving its trust boundary. The generation job keeps the trusted workflow +checkout, fetches the exact contributor head as Git data, prepares bounded +context, and does not check out or execute contributor code in its +secret-bearing worktree. Publication and pull request linking keep their +existing separate jobs and minimum permissions. + +Reuse the existing privileged fork preview path for `safe-to-review`. That path +checks out the contributor head and runs `go run ./cmd/preview deploy` with +`SPRITES_API_TOKEN` and `GITHUB_TOKEN`. Applying `safe-to-review` is an +explicit maintainer decision that accepts this trust boundary for the selected +contributor and current pull request. + +Keep approval labels durable until a maintainer removes them. Keep +`generate-pr-walkthrough` as a same-repository operational rerun label, not as +a contributor trust label. + +## Consequences + +- Maintainers use one label to request the three trusted contributor workflows. +- Allowlisted contributors keep automatic access through the existing direct + allowlist gates and receive one visible approval label. +- A contributor push keeps the approval active until a maintainer removes it. +- A labeled contributor pull request can run privileged preview code. The + repository allowlist and maintainer label process must remain restricted to + contributors the maintainers trust. +- PR walkthrough generation gains fork support without allowing contributor + files to replace its executable workflow assets. +- Existing `safe-to-test` labels become inert after rollout and require a + one-time repository cleanup. + +## Alternatives considered + +- Keep separate `safe-to-review` and `safe-to-test` labels. Rejected because a + maintainer trust decision then has two labels and can enable only part of the + intended workflow set. +- Let `safe-to-review` enable review and walkthrough but keep a separate + preview label. Rejected because preview is part of the requested trusted + contributor experience. +- Use only the label written by the allowlist bridge. Rejected because + `GITHUB_TOKEN` label writes do not provide a reliable downstream workflow + trigger. +- Redesign preview isolation before enabling the unified label. Rejected for + this change because the user selected the existing privileged preview path; + a preview isolation redesign remains a separate security initiative. +- Keep fork walkthroughs disabled. Rejected because the walkthrough is the + requested explanation of contributor changes and can preserve a stronger + trusted-input boundary than the existing privileged preview job. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index 035337cd90..2fa4d78ba2 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -150,7 +150,7 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-05-nested-submodules-as-repository-scopes | [Model Nested Submodules as Repository Scopes](2026-08-05-nested-submodules-as-repository-scopes.md) | accepted | backend, frontend, protocol | 2026-08-05 | | 2026-08-05-homebrew-remote-helper-audit | [Preserve Remote Helpers in Homebrew Installs](2026-08-05-homebrew-remote-helper-audit.md) | accepted | infra, workflow | 2026-08-05 | | 2026-08-07-model-aware-provider-capability-resolution | [Model-aware provider capability resolution](2026-08-07-model-aware-provider-capability-resolution.md) | accepted | backend, frontend, protocol | 2026-08-07 | -| 2026-08-07-claude-allowlist-label-bridge | [Use the Claude Allowlist as a Trusted Preview Gate](2026-08-07-claude-allowlist-label-bridge.md) | accepted | infra, workflow, security | 2026-08-07 | +| 2026-08-07-claude-allowlist-label-bridge | [Use the Claude Allowlist as a Trusted Preview Gate](2026-08-07-claude-allowlist-label-bridge.md) | accepted (amended by 2026-08-24-unified-fork-approval-label) | infra, workflow, security | 2026-08-07 | | 2026-08-08-task-autopilot-contract | [Make Task Autopilot a Creation-Time Runtime Contract](2026-08-08-task-autopilot-contract.md) | accepted | backend, agentctl, frontend, protocol, workflow | 2026-08-08 | | 2026-08-08-autopilot-mcp-question-capabilities | [Use One Question Capability Per Task Session](2026-08-08-autopilot-mcp-question-capabilities.md) | accepted | backend, agentctl, protocol | 2026-08-08 | | 2026-08-08-mcp-tool-profiles | [Compose MCP Tool Profiles From Typed Context](2026-08-08-mcp-tool-profiles.md) | accepted | backend, agentctl, protocol, workflow | 2026-08-08 | @@ -190,7 +190,7 @@ 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-persistent-fork-approval-labels | [Persist Fork Approval Labels Across Pushes](2026-08-22-persistent-fork-approval-labels.md) | accepted (amended by 2026-08-24-unified-fork-approval-label) | 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 | | 2026-08-22-preserve-newest-bounded-backend-logs | [Preserve Newest Bounded Backend Logs](2026-08-22-preserve-newest-bounded-backend-logs.md) | accepted | backend, infra, operations | 2026-08-22 | | 2026-08-12-plugin-webhook-auth-gate | [Require Auth for Plugin Webhooks Unless the Manifest Declares Them Public](2026-08-12-plugin-webhook-auth-gate.md) | accepted | backend, frontend, security | 2026-08-12 | @@ -201,3 +201,4 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-22-pr-walkthrough-description-link | [Own a top-level PR walkthrough callout](2026-08-22-pr-walkthrough-description-link.md) | accepted | workflow, infra, security, GitHub | 2026-08-22 | | 2026-08-23-pr-walkthrough-short-urls | [Use 12-character SHA prefixes for PR walkthrough URLs](2026-08-23-pr-walkthrough-short-urls.md) | accepted | workflow, infra | 2026-08-23 | | 2026-08-23-pr-walkthrough-workflow-provenance | [Use the workflow SHA for trusted PR walkthrough inputs](2026-08-23-pr-walkthrough-workflow-provenance.md) | accepted | workflow, infra, security | 2026-08-23 | +| 2026-08-24-unified-fork-approval-label | [Use One Maintainer Approval Label for Contributor PR Automation](2026-08-24-unified-fork-approval-label.md) | accepted | infra, workflow, security | 2026-08-24 | diff --git a/docs/plans/unified-contributor-pr-automation/plan.md b/docs/plans/unified-contributor-pr-automation/plan.md new file mode 100644 index 0000000000..da82a403b4 --- /dev/null +++ b/docs/plans/unified-contributor-pr-automation/plan.md @@ -0,0 +1,89 @@ +--- +created: 2026-08-24 +status: in_progress +requirements: + - ../../specs/ci/requirements/unified-contributor-pr-automation.md +system_design: + - ../../specs/ci/system-design/unified-contributor-pr-automation.md +legacy_specs: + - ../../specs/claude-fork-review-allowlist/spec.md + - ../../specs/pr-walkthrough/spec.md +--- + +# Unified contributor pull request automation + +## Overview + +Use one maintainer approval label, `safe-to-review`, for trusted contributor +AI review, preview deployment, and PR walkthrough generation. Extend the +walkthrough workflow to authorized fork pull requests while preserving its +trusted workflow-input boundary. Reuse the existing privileged preview path +selected by the user. + +## Scope + +- Replace active `safe-to-test` authorization with `safe-to-review`. +- Make the allowlist label bridge add only `safe-to-review`. +- Preserve direct allowlist gates because `GITHUB_TOKEN` label writes do not + create recursive workflow runs. +- Allow approved fork walkthroughs to fetch and inspect the exact PR head + without checking it out in the secret-bearing generation worktree. +- Update workflow contract tests, standalone script tests, and related design + records. +- Provide a post-rollout cleanup step for the old repository label. + +## Out of scope + +- A redesign of the privileged fork preview executor. +- Changes to AI provider prompts, models, or review follow-up policy. +- Removal of existing direct allowlist variables. +- Kandev application code or UI changes. +- Making `generate-pr-walkthrough` a contributor trust label. + +## Technical approach + +1. Update the Claude allowlist bridge and preview gate to use one approval + label, while keeping their direct allowlist paths. +2. Update the walkthrough job gate for approved forks and direct allowlisted + contributors. Fetch `refs/pull//head`, verify the exact head SHA, + and keep all executable assets at `github.workflow_sha`. +3. Keep generation, R2 publication, and PR-body linking as separate jobs. +4. Remove stale test assertions and update contract tests to fail if + `safe-to-test` returns to an active authorization expression. +5. After deployment, remove old `safe-to-test` labels from open pull requests + and delete the repository label after confirming no active workflow uses it. + +## Work orders + +- [x] [Task 01: Unify fork approval label gates](task-01-unify-fork-approval-label.md) +- [x] [Task 02: Enable approved fork walkthroughs](task-02-enable-approved-fork-walkthroughs.md) + +## Verification strategy + +Workflow contract tests and `zizmor` provide local static evidence. A live +post-merge check must cover an approved labeled fork, an allowlisted fork, an +unauthorized fork, a follow-up push, and approval revocation. No browser E2E is +needed because this change is limited to GitHub Actions. + +## Risks + +- `safe-to-review` now authorizes the existing preview job to execute + contributor code with deployment credentials. The label and allowlist must + remain limited to contributors the maintainers trust. +- The pull request ref must resolve to the event head SHA. A mismatch must + fail before context generation. +- The old label can remain visible on existing pull requests during migration. + It must not remain active in workflow expressions. + +## Verification results + +The implementation checks pass. The direct `zizmor` audit reports only the +intentional `pull_request_target` trigger findings on the four affected +workflows. The old checkout credential-persistence findings are resolved. +The PR fixup also addressed the remaining single-label wording in the legacy +specification and tightened the artifact-upload pin assertion to cover every +upload reference. The focused OpenCode test and specification lint passed. +The final PR check snapshot had 15 passed, 0 failed, and 0 pending checks, with +no unresolved review threads and a clean merge state. Post-merge live +verification and the one-time `safe-to-test` label cleanup remain operational +follow-up. diff --git a/docs/plans/unified-contributor-pr-automation/task-01-unify-fork-approval-label.md b/docs/plans/unified-contributor-pr-automation/task-01-unify-fork-approval-label.md new file mode 100644 index 0000000000..adbb1a6e71 --- /dev/null +++ b/docs/plans/unified-contributor-pr-automation/task-01-unify-fork-approval-label.md @@ -0,0 +1,94 @@ +--- +id: "01-unify-fork-approval-label" +title: "Unify fork approval label gates" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/ci/requirements/unified-contributor-pr-automation.md" +system_design: "../../specs/ci/system-design/unified-contributor-pr-automation.md" +--- + +# Task 01: Unify fork approval label gates + +## Acceptance + +- The allowlist label job adds exactly `safe-to-review` and no longer adds + `safe-to-test`. +- The fork preview job uses `safe-to-review` as its label gate and retains its + existing direct allowlist gates and privileged checkout and deploy command. +- The existing Claude and OpenCode fork review gates retain their current + provider-specific event and permission behavior. +- No active workflow authorization expression uses `safe-to-test`. +- Approval remains durable across synchronize events. No label cleanup job is + reintroduced. +- The standalone OpenCode workflow test no longer asserts an obsolete cleanup + job and passes with the persistent-label contract. + +## TDD scenarios + +1. RED: Change the Claude and preview contract tests to require one label and + reject `safe-to-test` authorization. +2. GREEN: Update the Claude label bridge and preview workflow expressions and + comments. +3. GREEN: Align the standalone OpenCode workflow test with persistent labels. +4. REFACTOR: Keep the existing direct allowlist expressions and minimize the + workflow diff. + +## Verification + +```bash +python3 .github/scripts/claude-code-review-workflow-contract_test.py +python3 .github/scripts/preview-env-workflow-contract_test.py +python3 .github/scripts/opencode-code-review-workflow-contract_test.py +bash scripts/opencode-code-review.test.sh +python3 .github/scripts/lint-action-pinning_test.py +python3 .github/scripts/lint-action-pinning.py +zizmor .github/workflows/claude-code-review.yml .github/workflows/opencode-code-review.yml .github/workflows/preview-env.yml +git diff --check +``` + +## Files likely touched + +- `.github/workflows/claude-code-review.yml` +- `.github/workflows/preview-env.yml` +- `.github/scripts/claude-code-review-workflow-contract_test.py` +- `.github/scripts/preview-env-workflow-contract_test.py` +- `.github/scripts/opencode-code-review-workflow-contract_test.py` +- `scripts/opencode-code-review.test.sh` + +## Dependencies + +None. Task 02 consumes this label contract. + +## Parallelism + +Sequential. This task owns the shared label meaning and preview gate. + +## Risks + +- The preview job has deployment credentials and checks out contributor code. + Keep the existing privileged path visible in the contract tests and decision + record. +- A label written by `GITHUB_TOKEN` does not start another workflow run. Do not + remove direct allowlist gates. + +## Output contract + +Report the exact label expressions, the direct allowlist paths retained, all +contract and security checks, and the old-label migration state. Update this +task and `plan.md` with the result. + +## Results + +The automatic label bridge now adds only `safe-to-review`, and the fork +preview gate uses that label while retaining its direct allowlists. Focused +workflow contracts, action pinning, the standalone OpenCode workflow test, and +credential-persistence checks passed. Direct `zizmor` still reports the +repository's existing `pull_request_target` trigger findings; no new trigger +type was introduced. + +During PR fixup, the legacy specification was narrowed to the single active +label and the standalone artifact-upload assertion was strengthened to compare +every upload reference with the exact required pin. The focused test and +specification lint passed, and the final PR check snapshot was fully green. diff --git a/docs/plans/unified-contributor-pr-automation/task-02-enable-approved-fork-walkthroughs.md b/docs/plans/unified-contributor-pr-automation/task-02-enable-approved-fork-walkthroughs.md new file mode 100644 index 0000000000..0223b2b476 --- /dev/null +++ b/docs/plans/unified-contributor-pr-automation/task-02-enable-approved-fork-walkthroughs.md @@ -0,0 +1,99 @@ +--- +id: "02-enable-approved-fork-walkthroughs" +title: "Enable approved fork walkthroughs" +status: done +wave: 2 +depends_on: ["01-unify-fork-approval-label"] +plan: "plan.md" +spec: "../../specs/ci/requirements/unified-contributor-pr-automation.md" +system_design: "../../specs/ci/system-design/unified-contributor-pr-automation.md" +--- + +# Task 02: Enable approved fork walkthroughs + +## Acceptance + +- A non-draft fork pull request with `safe-to-review` can start walkthrough + generation on the supported pull request events when + `PR_WALKTHROUGH_ENABLED` is true. +- The existing `CLAUDE_REVIEW_ALLOWLIST` direct path can authorize an opening + or update event without relying on a label event emitted by `GITHUB_TOKEN`. +- An unauthorized fork, a draft pull request, and a fork with only + `safe-to-test` skip the generation job. +- Same-repository generation and the + `generate-pr-walkthrough` same-repository rerun label remain unchanged. +- The generation job checks out and verifies `github.workflow_sha`, fetches + `refs/pull//head`, verifies `FETCH_HEAD` against the exact event head + SHA, and does not check out or execute contributor code in the generation + worktree. +- Artifact generation, R2 publication, public validation, and PR-body linking + retain their existing contracts and permissions. + +## TDD scenarios + +1. RED: Extend the walkthrough contract test for the fork approval expression, + direct allowlist path, exact pull request ref fetch, and fail-closed old + label behavior. +2. GREEN: Update the walkthrough job condition and trusted object fetch. +3. GREEN: Update the legacy walkthrough specification and contract comments. +4. REFACTOR: Keep the generation, publication, and link jobs separate and keep + the secret-bearing worktree on the trusted workflow SHA. + +## Verification + +```bash +python3 .github/scripts/pr-walkthrough-workflow-contract_test.py +python3 .agents/skills/pr-walkthrough/scripts/pr-walkthrough-context.test.py +python3 .agents/skills/pr-walkthrough/scripts/pr-walkthrough-render.test.py +python3 .github/scripts/lint-action-pinning_test.py +python3 .github/scripts/lint-action-pinning.py +zizmor .github/workflows/pr-walkthrough.yml +git diff --check +``` + +## Files likely touched + +- `.github/workflows/pr-walkthrough.yml` +- `.github/scripts/pr-walkthrough-workflow-contract_test.py` +- `docs/specs/pr-walkthrough/spec.md` + +## Dependencies + +- Task 01 must complete first so the walkthrough uses the final label + contract. + +## Parallelism + +Sequential. This task owns the fork walkthrough workflow and its trusted input +provenance. + +## Risks + +- Fetching a contributor ref into the object database must not replace the + trusted workflow checkout or trusted skill bundle. +- A short or stale SHA must not produce a published walkthrough. Assert the + exact full SHA before context preparation. +- The manual rerun label must remain restricted to same-repository pull + requests. + +## Output contract + +Report the fork gate, exact SHA evidence, artifact and link behavior, all +contract and security checks, and one live post-merge verification plan. Update +this task and `plan.md` with the result. + +## Results + +The walkthrough generation gate now accepts `safe-to-review` and the existing +`CLAUDE_REVIEW_ALLOWLIST` path for contributor pull requests. It fetches the +base repository pull request ref and verifies the exact event head SHA before +context preparation. Walkthrough generation, publication, linking, and the +trusted workflow asset boundary remain unchanged. + +The walkthrough contract, context helper, renderer, PR-body helper, action +pinning, specification lint, and diff checks passed. Direct `zizmor` reports +only the intentional `pull_request_target` trigger finding for this workflow. + +The final PR verification also passed the walkthrough generation, publication, +linking, and preview checks with no unresolved review threads. Post-merge live +verification remains the planned operational follow-up. diff --git a/docs/specs/ci/README.md b/docs/specs/ci/README.md new file mode 100644 index 0000000000..5d0fef0eb3 --- /dev/null +++ b/docs/specs/ci/README.md @@ -0,0 +1,51 @@ +--- +status: active +system: ci +specification_version: 1 +migration: in_progress +owners: + - kandev +--- + +# CI automation system + +## Purpose + +The CI automation system defines base-controlled GitHub Actions behavior for +contributor pull requests. It owns the trust gates for AI review, preview +deployment, and pull request walkthrough generation. + +## Ownership + +- Maintainer approval labels and their persistence rules. +- Direct contributor allowlist gates and their fail-closed behavior. +- Workflow event filters and job-level authorization expressions. +- Permissions and credential boundaries for review, preview, walkthrough, and + publication jobs. +- Workflow contract tests for these boundaries. + +## Exclusions + +- The review provider prompts and provider-specific agent behavior. +- The preview deployment command and its infrastructure. +- The walkthrough skill, renderer, and artifact format. +- Kandev application state or user-interface behavior. + +## Specification map + +### Requirements + +- [Unified contributor PR automation](requirements/unified-contributor-pr-automation.md) + +### System design + +- [Unified contributor PR automation](system-design/unified-contributor-pr-automation.md) + +## Migration + +The new shared trust contract replaces the label portions of the legacy +[Claude fork review allowlist specification](../claude-fork-review-allowlist/spec.md) +and the legacy +[PR walkthrough specification](../pr-walkthrough/spec.md). Those documents +remain useful for provider and walkthrough detail until their content is +migrated fully. diff --git a/docs/specs/ci/requirements/unified-contributor-pr-automation.md b/docs/specs/ci/requirements/unified-contributor-pr-automation.md new file mode 100644 index 0000000000..4fbb470430 --- /dev/null +++ b/docs/specs/ci/requirements/unified-contributor-pr-automation.md @@ -0,0 +1,135 @@ +--- +status: active +system: ci +created: 2026-08-24 +owners: + - kandev +--- + +# Unified contributor pull request automation requirements + +## Overview + +Maintainers need one clear approval action for trusted contributor pull +requests. The approval must enable the existing AI review, preview deployment, +and pull request walkthrough workflows without asking maintainers to remember +which label controls each capability. + +The approval applies to fork pull requests. Same-repository pull requests keep +their current workflow behavior. + +## Terminology + +- **Contributor pull request:** A pull request whose head repository differs + from the base repository. +- **Approval label:** The maintainer-applied `safe-to-review` label. +- **Direct allowlist:** An existing repository variable that names trusted + contributor logins. Direct allowlists remain independent authorization paths. +- **Privileged preview path:** The existing fork preview job that checks out the + contributor head and runs `go run ./cmd/preview deploy` with its current + deployment credentials. + +## Requirements + +### REQ-CI-PR-TRUST-001: One contributor approval label + +**Intent:** Give maintainers one visible approval control for the three +contributor pull request automation capabilities. + +#### Acceptance criteria + +- **AC-CI-PR-TRUST-001.1:** When a non-draft contributor pull request has the + `safe-to-review` label, the applicable base-controlled review, preview, and + walkthrough jobs shall be eligible on their supported events. +- **AC-CI-PR-TRUST-001.2:** When a contributor pull request has only the + `safe-to-test` label, no review, preview, or walkthrough job shall use that + label as authorization. +- **AC-CI-PR-TRUST-001.3:** When a contributor pull request has neither + `safe-to-review` nor an applicable direct allowlist authorization, the three + privileged capabilities shall remain unavailable. +- **AC-CI-PR-TRUST-001.4:** When a maintainer removes `safe-to-review`, a later + contributor push shall not regain label-based authorization. + +### REQ-CI-PR-TRUST-002: Durable and allowlisted contributor trust + +**Intent:** Preserve the existing trusted-contributor workflow while making +the label bridge reliable across pushes and GitHub event behavior. + +#### Acceptance criteria + +- **AC-CI-PR-TRUST-002.1:** When a contributor push updates an approved pull + request, `safe-to-review` shall remain valid until a maintainer removes it. +- **AC-CI-PR-TRUST-002.2:** When a contributor is in + `CLAUDE_REVIEW_ALLOWLIST`, the base-controlled label job shall add only + `safe-to-review` on the supported opening event. +- **AC-CI-PR-TRUST-002.3:** When the label job writes a label with + `GITHUB_TOKEN`, the review, preview, and walkthrough gates shall still have + a direct allowlist path where needed. They shall not depend on a recursive + `labeled` workflow run. +- **AC-CI-PR-TRUST-002.4:** Empty, malformed, or non-matching direct allowlists + shall fail closed. + +### REQ-CI-PR-WALK-003: Walkthroughs for approved contributor pull requests + +**Intent:** Give maintainers a visual explanation of trusted contributor +changes, including the changes that previously skipped walkthrough generation. + +#### Acceptance criteria + +- **AC-CI-PR-WALK-003.1:** When walkthrough generation is enabled and a + non-draft contributor pull request is authorized by `safe-to-review` or the + existing contributor allowlist bridge, the walkthrough workflow shall be + eligible on opened, ready-for-review, reopened, synchronize, and approval + label events supported by the workflow. +- **AC-CI-PR-WALK-003.2:** A successful contributor run shall preserve the + current walkthrough contract: non-empty JSON and HTML artifacts, validated + publication to R2, and one owned callout in the pull request description. +- **AC-CI-PR-WALK-003.3:** Walkthrough generation shall use trusted workflow + assets and bounded immutable contributor-head data. It shall not check out or + execute contributor code in the secret-bearing generation worktree. +- **AC-CI-PR-WALK-003.4:** The workflow shall verify that fetched contributor + data matches the exact event head SHA before preparing context or publishing + a result. + +### REQ-CI-PR-REVIEW-004: Existing review and preview capabilities + +**Intent:** Extend the existing trusted fork paths without creating a second +review or preview implementation. + +#### Acceptance criteria + +- **AC-CI-PR-REVIEW-004.1:** When a contributor pull request is authorized by + `safe-to-review`, the existing Claude and OpenCode fork review workflows shall + retain their current provider-specific event and permission contracts. +- **AC-CI-PR-REVIEW-004.2:** When a contributor pull request is authorized by + `safe-to-review`, the preview workflow shall reuse its existing privileged + fork path, including its current head checkout and deployment credentials. +- **AC-CI-PR-REVIEW-004.3:** Existing direct review and preview allowlists shall + remain valid as independent authorization sources. + +### REQ-CI-PR-FAIL-005: Fail closed and preserve unrelated behavior + +**Intent:** Prevent the label consolidation from widening unrelated workflow +permissions or changing same-repository automation. + +#### Acceptance criteria + +- **AC-CI-PR-FAIL-005.1:** When a workflow receives an unauthorized contributor + event, its privileged job shall be skipped before any contributor code or + secret-bearing action runs. +- **AC-CI-PR-FAIL-005.2:** Same-repository pull request behavior, the + independent `PR_WALKTHROUGH_ENABLED` toggle, and the + `generate-pr-walkthrough` same-repository rerun label shall remain unchanged. +- **AC-CI-PR-FAIL-005.3:** The workflow contract tests shall reject active + authorization expressions that reference `safe-to-test`. + +## Out of scope + +- Redesigning the preview executor or isolating its existing privileged fork + deployment path. +- Changing review provider prompts, models, or follow-up review policies. +- Removing `CLAUDE_REVIEW_ALLOWLIST`, `OPENCODE_REVIEW_ALLOWLIST`, or + `PREVIEW_ENV_ALLOWLIST`. +- Adding a Kandev application walkthrough feature. +- Making `generate-pr-walkthrough` a contributor trust label. It remains an + operational rerun label for same-repository pull requests. diff --git a/docs/specs/ci/system-design/unified-contributor-pr-automation.md b/docs/specs/ci/system-design/unified-contributor-pr-automation.md new file mode 100644 index 0000000000..4c68f3e01b --- /dev/null +++ b/docs/specs/ci/system-design/unified-contributor-pr-automation.md @@ -0,0 +1,196 @@ +--- +status: current +system: ci +requirements: + - REQ-CI-PR-TRUST-001 + - REQ-CI-PR-TRUST-002 + - REQ-CI-PR-WALK-003 + - REQ-CI-PR-REVIEW-004 + - REQ-CI-PR-FAIL-005 +--- + +# Unified contributor pull request automation system design + +## Purpose and boundaries + +This design defines the shared trust contract for base-controlled GitHub +Actions in the `kdlbs/kandev` repository. It covers fork pull request +authorization, event routing, and the boundaries between review, preview, +walkthrough generation, publication, and pull request linking. + +The design does not change the review providers, the preview command, the +walkthrough skill, or the application. Those components keep their existing +contracts after the shared authorization gate changes. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-CI-PR-TRUST-001` | [Data and contracts](#data-and-contracts), [Control flow](#control-flow) | +| `REQ-CI-PR-TRUST-002` | [Control flow](#control-flow), [Persistence and migration](#persistence-and-migration) | +| `REQ-CI-PR-WALK-003` | [Walkthrough flow](#walkthrough-flow), [Security](#security) | +| `REQ-CI-PR-REVIEW-004` | [Components and responsibilities](#components-and-responsibilities), [Security](#security) | +| `REQ-CI-PR-FAIL-005` | [Failure and recovery](#failure-and-recovery), [Security](#security) | + +## Components and responsibilities + +| Component | Responsibility | +| --- | --- | +| `.github/workflows/claude-code-review.yml` | Adds the approval label for allowlisted contributors and runs the existing Claude fork review path. | +| `.github/workflows/opencode-code-review.yml` | Runs the existing OpenCode fork review path with durable `safe-to-review` authorization. | +| `.github/workflows/preview-env.yml` | Runs the existing privileged fork preview path. | +| `.github/workflows/pr-walkthrough.yml` | Generates, publishes, and links walkthroughs for authorized fork and same-repository pull requests. | +| `.github/scripts/*workflow_contract_test.py` | Protects job gates, labels, event filters, permissions, and trusted input provenance. | +| `scripts/opencode-code-review.test.sh` | Keeps the standalone OpenCode workflow test aligned with the persistent-label contract. | + +## Data and contracts + +### Approval and allowlists + +`safe-to-review` is the only maintainer approval label for contributor review, +preview, and walkthrough automation. `safe-to-test` is removed from active +authorization expressions and from the automatic label bridge. + +The label-based fork gate is: + +```text +contains(github.event.pull_request.labels.*.name, 'safe-to-review') +``` + +The existing direct allowlists remain separate sources. The label bridge uses +`CLAUDE_REVIEW_ALLOWLIST`. The review and preview workflows retain their +existing `OPENCODE_REVIEW_ALLOWLIST`, `CLAUDE_REVIEW_ALLOWLIST`, and +`PREVIEW_ENV_ALLOWLIST` paths. The walkthrough uses +`CLAUDE_REVIEW_ALLOWLIST` directly so an allowlisted opening event remains +authorized even when a label written with `GITHUB_TOKEN` does not emit a new +workflow run. + +### Event contract + +- The walkthrough workflow keeps `pull_request_target` events for opened, + ready-for-review, reopened, synchronize, and labeled actions. +- A `generate-pr-walkthrough` label still requests a same-repository manual + rerun. It does not authorize a contributor pull request. +- The existing Claude workflow keeps its open and labeled fork review policy. +- The existing OpenCode and preview event sets remain unchanged except for the + approval label expression. + +### Walkthrough artifact contract + +The walkthrough keeps the existing provider-neutral contract: + +1. Generate `docs/pr-walkthrough/pr-.json` and + `docs/pr-walkthrough/pr-.html` in the generation job. +2. Upload the files as a CI artifact. +3. Upload only HTML to the R2 bucket in the publication job. +4. Validate the public response and exact HTML bytes. +5. Add or replace the owned callout in the pull request body in the link job. + +The public object key remains +`pr//.html`. + +## Control flow + +### Trusted label path + +1. A maintainer applies `safe-to-review` to a non-draft contributor pull + request. +2. The base-controlled workflows receive the supported pull request event and + evaluate the label at job level. +3. Review and preview use their existing fork paths. Walkthrough generation + uses the trusted workflow checkout and the exact contributor head data. +4. Removing the label prevents later label-based runs. A contributor push does + not remove the label automatically. + +### Allowlisted contributor path + +1. On an opened fork pull request, the Claude label job checks the author in + `CLAUDE_REVIEW_ALLOWLIST` without checking out the pull request. +2. It adds only `safe-to-review`. +3. Each workflow keeps a direct allowlist expression where the resulting label + event cannot be relied on. This includes the walkthrough opening and update + path through `CLAUDE_REVIEW_ALLOWLIST`. +4. The direct path and the visible label converge on the same maintainer trust + contract. No second preview label is needed. + +### Walkthrough flow + +1. The generation job checks out `github.workflow_sha` with credentials + persisted as false and verifies the checkout is exactly that SHA. +2. It fetches `refs/pull/${PR_NUMBER}/head` into the Git object database and + verifies that `FETCH_HEAD` resolves to the event `HEAD_SHA`. It does not + check out that ref. +3. The trusted context helper computes the merge base, patch, manifest, and + bounded regular UTF-8 files from the immutable head object. +4. The agent reads trusted instructions plus that bounded context, edits only + `.pr-walkthrough/draft.json`, and invokes the fixed trusted renderer. +5. Publication and linking run in separate jobs with their existing minimal + permissions and credentials. + +## Failure and recovery + +- Missing, malformed, or non-matching allowlists fail closed. +- A missing `safe-to-review` label skips contributor jobs. A stale + `safe-to-test` label has no authorization effect after migration. +- A failed label write does not grant access and does not block direct + allowlist paths. +- A missing or mismatched pull request ref fails walkthrough context + preparation before the agent starts. The exact SHA check prevents a result + from being associated with a different head. +- Existing walkthrough retry, artifact, R2 validation, and PR-body validation + behavior remains unchanged. +- Contract tests fail when the old label appears in an active authorization + expression or when a privileged job loses its trust or permission boundary. + +## Persistence and migration + +Approval labels remain until a maintainer removes them. The rollout changes +workflow expressions and the allowlist label bridge first. After the changed +workflows are active, operators remove existing `safe-to-test` labels from open +pull requests and remove the repository label definition when no other active +workflow references it. Existing `safe-to-test` labels are inert before that +cleanup. + +The `PR_WALKTHROUGH_ENABLED` variable remains independent. The +`generate-pr-walkthrough` label remains an operational same-repository rerun +trigger. + +## Security + +- All workflow files, helper scripts, and walkthrough skill files used by the + secret-bearing generation job come from `github.workflow_sha`. +- The walkthrough generation job fetches contributor data only as Git objects + and materializes bounded files. It does not execute contributor scripts or + use contributor-controlled workflow, skill, setup, or link helpers. +- Walkthrough generation receives the existing model credential but no R2 + credentials or pull request write permission. +- The review workflows retain their existing read-only agent policies and + advisory posting boundaries. +- The preview workflow intentionally reuses the existing privileged fork path. + That path checks out the contributor head and runs `go run ./cmd/preview + deploy` with `SPRITES_API_TOKEN` and `GITHUB_TOKEN`. `safe-to-review` is an + explicit maintainer trust decision for that execution. +- Publication receives only the bucket-scoped R2 credentials needed for its + upload. Linking receives only the existing pull request write permission and + trusted helper. +- The automatic label job has no checkout and only the issue-label write + permission it needs. + +## Observability + +- Existing workflow run status, job summaries, artifact diagnostics, R2 URL + validation, and PR-body marker behavior remain the operational signals. +- Contract tests provide local evidence for every authorization expression, + event filter, trusted checkout, exact SHA check, and sensitive permission. +- Post-rollout verification must inspect a labeled fork run, an allowlisted + fork run, an unauthorized fork run, a follow-up push, and a revoked-label + run. + +## Related decisions + +- [Use one maintainer approval label for contributor PR automation](../../../decisions/2026-08-24-unified-fork-approval-label.md) +- [Persist Fork Approval Labels Across Pushes](../../../decisions/2026-08-22-persistent-fork-approval-labels.md) +- [Use the Claude Allowlist as a Trusted Preview Gate](../../../decisions/2026-08-07-claude-allowlist-label-bridge.md) +- [Use a Filesystem Contract for PR Walkthrough Runners](../../../decisions/2026-08-22-pr-walkthrough-filesystem-runner.md) +- [Use the workflow SHA for trusted PR walkthrough inputs](../../../decisions/2026-08-23-pr-walkthrough-workflow-provenance.md) +- [Host PR walkthrough HTML in Cloudflare R2](../../../decisions/2026-08-22-pr-walkthrough-r2-hosting.md) diff --git a/docs/specs/integrations/requirements/claude-fork-review-allowlist.md b/docs/specs/integrations/requirements/claude-fork-review-allowlist.md index c1bff52e2d..1b87d17610 100644 --- a/docs/specs/integrations/requirements/claude-fork-review-allowlist.md +++ b/docs/specs/integrations/requirements/claude-fork-review-allowlist.md @@ -30,24 +30,24 @@ Trusted fork contributors listed in the repository's Claude review allowlist sho ## Migrated source detail -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 +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; ADR-2026-08-24-unified-fork-approval-label ## 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. Maintainers also use those labels for unallowlisted fork pull requests and expect an explicit approval to remain valid across routine follow-up commits. +The same trusted contributors should also receive the repository's one review and preview approval marker, so maintainers do not need to maintain a second identity list or apply multiple labels manually. Maintainers also use that label for unallowlisted fork pull requests and expect an explicit approval to remain valid across routine follow-up commits. ## What - 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. -- 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. +- When an allowlisted fork pull request opens, the base-controlled workflow adds only `safe-to-review` 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`). 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. +- 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-review` 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, preview, and walkthrough paths for the current fork head. The label is not removed automatically after a push. +- The `safe-to-review` label remains until a maintainer removes it. A contributor push does not revoke it or require the maintainer to repeat the same approval for each follow-up commit. The legacy `safe-to-test` label is inert after migration and does not authorize review, walkthrough, or preview workflows. - 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. @@ -57,9 +57,9 @@ The same trusted contributors should also receive the repository's existing revi ## Permissions -- 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. +- Only the base-controlled `pull_request_target` workflow may add the `safe-to-review` approval label. 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. 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. +- A non-allowlisted fork author still needs the existing maintainer-applied `safe-to-review` label. That label authorizes the current and subsequent fork heads until a maintainer removes it; maintainers are responsible for revoking approval when the contributor or proposed change is no longer trusted. ## Failure modes @@ -70,14 +70,14 @@ The same trusted contributors should also receive the repository's existing revi ## Scenarios - **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** `CLAUDE_REVIEW_ALLOWLIST` is `["ClemDNL"]`, **WHEN** `ClemDNL` opens a fork pull request, **THEN** the pull request receives only the `safe-to-review` label 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` 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** the preview deploy job remains 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. +- **GIVEN** a fork contributor is absent from `CLAUDE_REVIEW_ALLOWLIST`, **WHEN** they open a pull request, **THEN** the automatic labeling job adds no approval label and the preview workflow does not deploy unless its existing `PREVIEW_ENV_ALLOWLIST` or `safe-to-review` path authorizes it. - **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. - **GIVEN** a pull request adds a file after its initial review, **WHEN** a maintainer comments `@claude review`, **THEN** Claude can read and review that added file rather than ending without a review because the file is absent from the default-branch checkout. diff --git a/docs/specs/ui/requirements/pr-walkthrough.md b/docs/specs/ui/requirements/pr-walkthrough.md index c8cb9c63d2..5ef2215416 100644 --- a/docs/specs/ui/requirements/pr-walkthrough.md +++ b/docs/specs/ui/requirements/pr-walkthrough.md @@ -47,7 +47,8 @@ lifecycle policy. [ADR-2026-08-22-pr-walkthrough-filesystem-runner](../../../decisions/2026-08-22-pr-walkthrough-filesystem-runner.md), [ADR-2026-08-22-pr-walkthrough-description-link](../../../decisions/2026-08-22-pr-walkthrough-description-link.md), [ADR-2026-08-23-pr-walkthrough-short-urls](../../../decisions/2026-08-23-pr-walkthrough-short-urls.md), -[ADR-2026-08-23-pr-walkthrough-workflow-provenance](../../../decisions/2026-08-23-pr-walkthrough-workflow-provenance.md) +[ADR-2026-08-23-pr-walkthrough-workflow-provenance](../../../decisions/2026-08-23-pr-walkthrough-workflow-provenance.md), +[ADR-2026-08-24-unified-fork-approval-label](../../../decisions/2026-08-24-unified-fork-approval-label.md) **Implementation plans:** [Portable PR walkthrough runner fix](../../../plans/pr-walkthrough-portable-runner-fix/plan.md), @@ -78,10 +79,11 @@ lifecycle policy. the website brand mark and favicon, and its dark theme uses the documentation shell's dark-gray palette. - The configured workflow agent generates and renders the walkthrough for a - non-draft same-repository pull request when it is opened, reopened, marked - ready for review, or updated. OpenCode is the initial runner, but the skill - and artifact contract do not depend on it. A maintainer can explicitly - retrigger generation by adding the `generate-pr-walkthrough` label. + non-draft same-repository pull request or an authorized contributor pull + request when it is opened, reopened, marked ready for review, or updated. + OpenCode is the initial runner, but the skill and artifact contract do not + depend on it. A maintainer can explicitly retrigger same-repository + generation by adding the `generate-pr-walkthrough` label. - The workflow gives each runner the same fixed prompt, prepared context, draft JSON path, renderer command, and final output paths. A provider change does not change this contract. @@ -104,9 +106,9 @@ lifecycle policy. the first 12 lowercase hexadecimal characters of the exact head SHA. It is served at `https://walkthrough.kandev.ai/pr//.html`. -- The workflow regenerates on `synchronize` for same-repository pull request - updates. Each generated object remains keyed by pull request number and the - 12-character prefix of the exact head SHA. +- The workflow regenerates on `synchronize` for same-repository and authorized + contributor pull request updates. Each generated object remains keyed by + pull request number and the 12-character prefix of the exact head SHA. - After public validation succeeds, a separate minimum-permission job prepends a prominent marker-owned walkthrough callout to the pull request description. A rerun replaces only that callout and preserves the rest of @@ -161,6 +163,9 @@ adapters remain outside the skill because they are platform integration code. merge base without checking out that SHA. It uses the same workflow-commit copy of the skill bundle, runner setup action, and PR-description helper. Pull request changes cannot replace these executable components. +- Contributor generation requires the durable `safe-to-review` approval label + or the existing trusted-contributor allowlist path. The old `safe-to-test` + label is not an authorization source. - The agent invokes the fixed renderer before it finishes. The workflow only verifies and packages the ignored walkthrough output directory. - The R2 publishing job receives only the bucket-scoped S3-compatible R2 @@ -243,7 +248,8 @@ not from merge time. head and updates the corresponding R2 object and job-summary URL. - **GIVEN** a pull request receives a new head commit, **WHEN** the walkthrough workflow runs for the synchronize event, **THEN** it generates - a walkthrough for the new head. + a walkthrough for the new head when the pull request is same-repository or + has contributor approval. - **GIVEN** two pull requests use different numbers, **WHEN** both jobs run, **THEN** each output filename and R2 object key is distinct and neither run overwrites the other's result. @@ -269,6 +275,9 @@ not from merge time. plain code block remains neutral context. - **GIVEN** a draft pull request or an unauthorized fork event, **WHEN** the workflow is triggered, **THEN** no walkthrough agent job runs. +- **GIVEN** an approved contributor pull request, **WHEN** the workflow is + triggered, **THEN** it prepares the exact head data without checking out or + executing contributor code in the secret-bearing generation worktree. - **GIVEN** a successfully generated HTML file, **WHEN** the publishing job uploads it, **THEN** a public GET to the reported URL returns the same HTML with an HTML content type. @@ -282,7 +291,7 @@ not from merge time. ## Out of scope - Publishing walkthrough screenshots or using the screenshot media branch. -- Generating walkthroughs for fork pull requests. +- Generating walkthroughs for unauthorized contributor pull requests. - Retaining a walkthrough for an exact period measured from merge time. The initial lifecycle is measured from upload time. - Adding a Kandev UI page for externally generated PR walkthroughs. diff --git a/scripts/opencode-code-review.test.sh b/scripts/opencode-code-review.test.sh index 2f3f2b0510..fca3f3aab5 100644 --- a/scripts/opencode-code-review.test.sh +++ b/scripts/opencode-code-review.test.sh @@ -111,9 +111,8 @@ fi pass "Same-repository trusted review isolates App credentials in the protected environment" for harness_file in \ - .agents/skills/pr-fixup/SKILL.md \ .agents/skills/planner-orchestration/SKILL.md; do - if ! grep -q 'OpenCode App.*trusted_producer=true\|trusted_producer=true.*OpenCode App\|OpenCode producer.*trusted_producer=true' "$ROOT_DIR/$harness_file"; then + if ! grep -Fq 'OpenCode App as trusted semantic evidence only when `trusted_producer=true`' "$ROOT_DIR/$harness_file"; then fail "Harness requires trusted producer only for the dedicated OpenCode App" fi done @@ -137,11 +136,12 @@ if [[ "$trusted_script_count" != "2" ]]; then fi pass "OpenCode review executes the parser script from the trusted base commit in both workflow paths" -artifact_upload_count="$(count_matches regex 'uses: actions/upload-artifact@(v4([[:space:]]|$)|[a-f0-9]{40}[[:space:]]+# v4([[:space:]]|$))')" -if [[ "$artifact_upload_count" != "2" ]]; then - fail "OpenCode review upload-artifact is pinned to v4 in both workflow paths" +artifact_upload_count="$(count_occurrences 'uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1')" +total_artifact_upload_count="$(count_occurrences 'uses: actions/upload-artifact@')" +if [[ "$artifact_upload_count" != "2" || "$artifact_upload_count" != "$total_artifact_upload_count" ]]; then + fail "OpenCode review upload-artifact is pinned to v7 in both workflow paths" fi -pass "OpenCode review artifacts are uploaded from both workflow paths" +pass "OpenCode review upload-artifact is pinned to v7 in both workflow paths" invalid_artifact_upload_refs="$( rg --line-number 'uses: actions/upload-artifact@' "$WORKFLOW" | @@ -199,11 +199,6 @@ if rg -q '(OPENCODE_REVIEW_APP_PRIVATE_KEY|steps\.app-token\.outputs\.token)' << fi pass "OpenCode model step does not receive dedicated App credentials" -if ! rg -A 10 '^ strip-safe-to-review:' "$WORKFLOW" | rg -q 'issues: write'; then - fail "Fork safe-to-review cleanup has issues write permission" -fi -pass "Fork safe-to-review cleanup has issues write permission" - if [[ "$(count_occurrences "$app_token_action")" != "1" ]]; then fail "Only same-repository review mints the dedicated App token" fi