From 0eea98528c9da5f046241f2730feeb1868b1bc25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 22 Sep 2026 13:20:07 +0100 Subject: [PATCH] fix(ci): isolate ineligible PR walkthrough triggers (#3862) --- .../pr-walkthrough-workflow-contract_test.py | 59 ++++++++++- .github/workflows/pr-walkthrough.yml | 34 +++++- ...-pr-walkthrough-eligibility-concurrency.md | 55 ++++++++++ .../plan.md | 95 +++++++++++++++++ .../task-01-guard-ineligible-triggers.md | 100 ++++++++++++++++++ .../unified-contributor-pr-automation.md | 6 ++ .../unified-contributor-pr-automation.md | 27 +++++ 7 files changed, 374 insertions(+), 2 deletions(-) create mode 100644 docs/decisions/2026-09-22-pr-walkthrough-eligibility-concurrency.md create mode 100644 docs/plans/pr-walkthrough-eligibility-concurrency/plan.md create mode 100644 docs/plans/pr-walkthrough-eligibility-concurrency/task-01-guard-ineligible-triggers.md diff --git a/.github/scripts/pr-walkthrough-workflow-contract_test.py b/.github/scripts/pr-walkthrough-workflow-contract_test.py index cfcb1dff58..58533fb91a 100644 --- a/.github/scripts/pr-walkthrough-workflow-contract_test.py +++ b/.github/scripts/pr-walkthrough-workflow-contract_test.py @@ -346,13 +346,70 @@ class PRWalkthroughWorkflowContractTest(unittest.TestCase): def test_walkthrough_runs_are_serialized_for_the_entire_pr_pipeline(self) -> None: workflow_header = self.workflow.split("jobs:", 1)[0] + normalized_header = " ".join(workflow_header.split()) self.assertIn("concurrency:", workflow_header) - self.assertIn("group: pr-walkthrough-${{ github.event.pull_request.number }}", workflow_header) + self.assertIn( + "group: >- pr-walkthrough-${{ github.event.pull_request.number }}${{", + normalized_header, + ) self.assertIn("cancel-in-progress: true", workflow_header) self.assertNotIn("concurrency:", self.generation) self.assertIn("concurrency:", self.same_repo_review) self.assertIn("concurrency:", self.fork_review) + # @covers AC-CI-PR-WALK-003.5, AC-CI-PR-FAIL-005.1 + def test_walkthrough_concurrency_isolates_ineligible_label_events(self) -> None: + workflow_header = " ".join(self.workflow.split("jobs:", 1)[0].split()) + for value in ( + "group: >- pr-walkthrough-${{ github.event.pull_request.number }}${{", + "github.event.action == 'labeled'", + "github.event.pull_request.head.repo.full_name == github.repository", + "github.event.pull_request.head.repo.full_name != github.repository", + "github.event.label.name == 'generate-pr-walkthrough'", + "github.event.label.name == 'safe-to-review'", + "&& '-ineligible' || ''", + ): + self.assertIn(value, workflow_header) + self.assertIn("cancel-in-progress: true", workflow_header) + + # @covers AC-CI-PR-WALK-003.5, AC-CI-PR-FAIL-005.1 + def test_walkthrough_concurrency_matches_the_full_generation_gate(self) -> None: + workflow_header = " ".join(self.workflow.split("jobs:", 1)[0].split()) + for value in ( + "vars.PR_WALKTHROUGH_ENABLED != 'true'", + "github.event_name != 'pull_request_target'", + "github.event.pull_request.draft != false", + "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.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)", + ): + self.assertIn(value, workflow_header) + self.assertIn( + "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'))", + workflow_header, + ) + self.assertIn( + "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' && (", + workflow_header, + ) + self.assertIn("!(", workflow_header) + def test_walkthrough_label_does_not_run_the_normal_code_review(self) -> None: self.assertIn("github.event.action != 'labeled'", self.same_repo_review) self.assertIn( diff --git a/.github/workflows/pr-walkthrough.yml b/.github/workflows/pr-walkthrough.yml index 499efb2b5b..0787ee59f2 100644 --- a/.github/workflows/pr-walkthrough.yml +++ b/.github/workflows/pr-walkthrough.yml @@ -16,7 +16,39 @@ env: PR_WALKTHROUGH_VARIANT: high concurrency: - group: pr-walkthrough-${{ github.event.pull_request.number }} + # GitHub resolves workflow concurrency before it evaluates the job `if`. + # Keep every event rejected by the generation gate out of the canceling + # group so it cannot terminate an eligible run for the same pull request. + group: >- + pr-walkthrough-${{ github.event.pull_request.number }}${{ + ( + 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 == '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)) + )) + ) + ) + ) + ) && '-ineligible' || '' + }} cancel-in-progress: true jobs: diff --git a/docs/decisions/2026-09-22-pr-walkthrough-eligibility-concurrency.md b/docs/decisions/2026-09-22-pr-walkthrough-eligibility-concurrency.md new file mode 100644 index 0000000000..1fa0217324 --- /dev/null +++ b/docs/decisions/2026-09-22-pr-walkthrough-eligibility-concurrency.md @@ -0,0 +1,55 @@ +# ADR-2026-09-22-pr-walkthrough-eligibility-concurrency: Isolate ineligible PR walkthrough triggers + +**Status:** accepted +**Date:** 2026-09-22 +**Area:** workflow + +## Context + +The PR walkthrough workflow receives every supported pull request event, but +its generation decision runs in the job's `if` expression. GitHub applies the +workflow concurrency group before that job condition. Any event that fails the +generation gate can therefore cancel an eligible run for the same pull +request, including an unauthorized fork update, a draft event, or an event +received while generation is disabled. + +PR #3708 demonstrated this failure. The `safe-to-review` event started an +eligible generation run. A simultaneous `safe-to-test` event entered the same +per-pull-request group, canceled the eligible run, and then skipped its own +generation job because `safe-to-test` is not an authorization label. + +## Decision + +Keep one canceling concurrency group for eligible walkthrough triggers, and +route every event rejected by the full generation gate to a separate +per-pull-request group before the job-level authorization gate runs. + +The workflow-level expression mirrors the generation gate's toggle, event, +draft, repository, action, label, and allowlist conditions. Eligible +same-repository `generate-pr-walkthrough` events and contributor +`safe-to-review` or allowlisted events remain in +`pr-walkthrough-`. Every other event uses an `-ineligible` suffix. The +job-level authorization expression remains the source of permission and +continues to fail closed. + +## Consequences + +- A newer eligible trigger still cancels an older eligible pipeline for the + same pull request. +- An event rejected by the generation gate can no longer cancel generation, + publication, or linking for an eligible trigger. +- Rejected events still create skipped workflow runs, which preserve the + existing audit trail and do not grant any capability. +- The concurrency expression must remain aligned with the job-level trigger + eligibility rules, and the workflow contract test must protect both paths. + +## Alternatives Considered + +- Set `cancel-in-progress: false` for every walkthrough run. Rejected because + it changes the existing latest-eligible-run behavior and can spend model + capacity on stale eligible heads. +- Remove the `labeled` event trigger. Rejected because approved contributor + pull requests would lose the required `safe-to-review` activation path. +- Give every label its own concurrency group. Rejected because eligible + labels would no longer serialize with synchronize and other eligible events + for the same pull request. diff --git a/docs/plans/pr-walkthrough-eligibility-concurrency/plan.md b/docs/plans/pr-walkthrough-eligibility-concurrency/plan.md new file mode 100644 index 0000000000..3dd888eddc --- /dev/null +++ b/docs/plans/pr-walkthrough-eligibility-concurrency/plan.md @@ -0,0 +1,95 @@ +--- +created: 2026-09-22 +status: complete +requirements: + - REQ-CI-PR-WALK-003 + - REQ-CI-PR-TRUST-001 + - REQ-CI-PR-FAIL-005 +system_design: + - ../../specs/ci/system-design/unified-contributor-pr-automation.md +legacy_specs: [] +--- + +# Implementation Plan: PR walkthrough eligibility concurrency + +## Overview + +Update the PR walkthrough workflow so an event rejected by the generation gate +cannot cancel an eligible contributor walkthrough run. Keep latest-run +cancellation for eligible triggers, preserve the existing authorization gate, +and add workflow contract regression coverage. + +## Scope + +### In scope + +- Separate every event rejected by the full generation gate from the eligible + per-pull-request concurrency group. +- Preserve `safe-to-review` as the only contributor approval label. +- Add contract tests for the reported `safe-to-review` and `safe-to-test` race + and an unauthorized fork non-labeled event. +- Document the manual rerun procedure for an approved contributor PR. + +### Out of scope + +- Changing walkthrough generation, rendering, publication, or R2 hosting. +- Adding a new GitHub Actions trigger or changing the model configuration. +- Removing labels or changing PR #3708 from this repository. + +## Technical approach + +Change `.github/workflows/pr-walkthrough.yml` so its concurrency expression +mirrors the complete generation job gate, including the feature toggle, event +type, draft state, repository, action, label, and allowlist conditions. Eligible +same-repository `generate-pr-walkthrough` events and authorized contributor +events use the existing shared group. Every rejected event uses an +`-ineligible` suffix. Keep `cancel-in-progress: true` and the job-level +authorization expression unchanged. + +Extend `.github/scripts/pr-walkthrough-workflow-contract_test.py` to require the +ineligible group branch and the complete source, action, label, and allowlist +conditions. The test continues to require latest-run cancellation and the +absence of `safe-to-test` from authorization expressions. + +## Tests + +| Acceptance criterion | Evidence | +| --- | --- | +| AC-CI-PR-WALK-003.1 | Existing generation gate contract plus the eligible-group assertions. | +| AC-CI-PR-WALK-003.2 | Existing generation and publication contract tests. | +| AC-CI-PR-WALK-003.5 | New concurrency contract tests covering eligible labels, ineligible labels, and unauthorized fork non-labeled events. | +| AC-CI-PR-TRUST-001.2 | Existing assertion that `safe-to-test` is absent from authorization. | +| AC-CI-PR-FAIL-005.1 | Existing trusted job-gate contract plus the new group isolation assertion. | + +## Operational rerun + +For an approved contributor PR, a new head commit triggers `synchronize` while +the `safe-to-review` label remains present. A label-only rerun can remove any +stale `safe-to-test` label, remove `safe-to-review`, and add `safe-to-review` +again. The `safe-to-test` removal is not a trigger; the final `safe-to-review` +addition is the trigger. Same-repository PRs can use the existing +`generate-pr-walkthrough` label. + +## Work orders + +- [x] [Task 01: Guard ineligible walkthrough triggers](task-01-guard-ineligible-triggers.md) + +## Verification results + +- `python3 .github/scripts/pr-walkthrough-workflow-contract_test.py` passed. +- `python3 .github/scripts/lint-action-pinning_test.py` passed. +- `python3 scripts/list-docs.py validate` passed. +- `python3 scripts/lint-spec-files.py --all` passed. +- `git diff --check` passed. +- `zizmor .github/workflows` ran but remains non-zero for existing findings, + including the pre-existing `pull_request_target` warning in the changed + walkthrough workflow; the new concurrency expression introduced no finding. +- `go run github.com/rhysd/actionlint/cmd/actionlint@latest` with + `.github/workflows/pr-walkthrough.yml` passed. + +## Risks + +- The workflow-level expression must remain semantically aligned with the + complete job-level generation gate, including its toggle and draft checks. +- GitHub evaluates concurrency before the job condition, so the contract test + must protect the group split as well as the authorization expression. diff --git a/docs/plans/pr-walkthrough-eligibility-concurrency/task-01-guard-ineligible-triggers.md b/docs/plans/pr-walkthrough-eligibility-concurrency/task-01-guard-ineligible-triggers.md new file mode 100644 index 0000000000..ddf7c01f48 --- /dev/null +++ b/docs/plans/pr-walkthrough-eligibility-concurrency/task-01-guard-ineligible-triggers.md @@ -0,0 +1,100 @@ +--- +id: "01-guard-ineligible-triggers" +title: "Guard ineligible walkthrough triggers" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-CI-PR-WALK-003 + - REQ-CI-PR-TRUST-001 + - REQ-CI-PR-FAIL-005 +acceptance_criteria: + - AC-CI-PR-WALK-003.1 + - AC-CI-PR-WALK-003.5 + - AC-CI-PR-TRUST-001.2 + - AC-CI-PR-FAIL-005.1 +system_design: + - ../../specs/ci/system-design/unified-contributor-pr-automation.md +--- + +# Task 01: Guard ineligible walkthrough triggers + +## Summary + +Adjust the PR walkthrough workflow concurrency expression so an event rejected +by the full generation gate cannot cancel an eligible run for the same pull +request. Add contract coverage for label and non-labeled rejected events and +preserve the existing job-level authorization behavior. + +## In scope + +- Update the top-level concurrency group in + `.github/workflows/pr-walkthrough.yml`. +- Add assertions to + `.github/scripts/pr-walkthrough-workflow-contract_test.py`. +- Verify the existing `safe-to-review`, allowlist, draft, toggle, and + same-repository rerun gates remain unchanged. + +## Out of scope + +- Walkthrough content, renderer behavior, R2 publication, or PR-body linking. +- A new manual-dispatch workflow input. +- Changes to preview or code-review workflows. + +## Acceptance + +- Contributor `safe-to-review` events and same-repository + `generate-pr-walkthrough` events remain in the canceling per-PR group. +- An ineligible label event such as `safe-to-test` and an unauthorized fork + non-labeled event use a separate group and cannot cancel an eligible + walkthrough pipeline. +- The contract test passes and still rejects `safe-to-test` as an authorization + source. + +## Verification + +```bash +python3 .github/scripts/pr-walkthrough-workflow-contract_test.py +``` + +## Files likely touched + +- `.github/workflows/pr-walkthrough.yml` +- `.github/scripts/pr-walkthrough-workflow-contract_test.py` + +## Dependencies + +None. + +## Risks + +- A mismatch between workflow-level group selection and the job-level gate + could either waste a run or allow an unintended cancellation path. + +## Parallelism + +`sequential` + +## Inputs + +- `REQ-CI-PR-WALK-003.1` and `REQ-CI-PR-WALK-003.5`. +- `REQ-CI-PR-TRUST-001.2` and `REQ-CI-PR-FAIL-005.1`. +- [Unified contributor pull request automation system design](../../specs/ci/system-design/unified-contributor-pr-automation.md). +- [Eligibility concurrency ADR](../../decisions/2026-09-22-pr-walkthrough-eligibility-concurrency.md). + +## Results + +- Updated the workflow-level concurrency expression to mirror the complete + generation gate. Every rejected event uses a per-pull-request + `-ineligible` group while eligible triggers retain the canceling shared + group. +- Added contract coverage for source-specific labels, unauthorized fork + non-labeled events, the global gate conditions, the ineligible suffix, and + latest-run cancellation. +- `python3 .github/scripts/pr-walkthrough-workflow-contract_test.py` passed + with 29 tests. +- `python3 .github/scripts/lint-action-pinning_test.py` and + `git diff --check` passed. +- `go run github.com/rhysd/actionlint/cmd/actionlint@latest` with + `.github/workflows/pr-walkthrough.yml` passed. diff --git a/docs/specs/ci/requirements/unified-contributor-pr-automation.md b/docs/specs/ci/requirements/unified-contributor-pr-automation.md index 69d567a3a0..ba5431e902 100644 --- a/docs/specs/ci/requirements/unified-contributor-pr-automation.md +++ b/docs/specs/ci/requirements/unified-contributor-pr-automation.md @@ -90,6 +90,12 @@ changes, including the changes that previously skipped walkthrough generation. - **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. +- **AC-CI-PR-WALK-003.5:** When GitHub delivers multiple supported events for + one pull request, a run whose event does not satisfy the walkthrough + generation gate, including an unauthorized fork event, a draft event, or an + event received while walkthrough generation is disabled, shall not cancel or + supersede an eligible walkthrough run for the same pull request. Eligible + walkthrough runs shall retain latest-run cancellation behavior. ### REQ-CI-PR-REVIEW-004: Existing review and preview capabilities diff --git a/docs/specs/ci/system-design/unified-contributor-pr-automation.md b/docs/specs/ci/system-design/unified-contributor-pr-automation.md index 513e882e76..24901a3028 100644 --- a/docs/specs/ci/system-design/unified-contributor-pr-automation.md +++ b/docs/specs/ci/system-design/unified-contributor-pr-automation.md @@ -79,6 +79,29 @@ workflow run. - The existing OpenCode and preview event sets remain unchanged except for the approval label expression. +### Eligibility-aware concurrency + +GitHub applies workflow-level concurrency before it evaluates a job-level +`if`. The walkthrough workflow therefore mirrors the full generation gate when +it selects the per-pull-request concurrency group. The shared +`pr-walkthrough-` group is used only when walkthrough generation is +enabled, the event is non-draft and `pull_request_target`, and the source, +action, label, and contributor allowlist conditions authorize generation. +Events that fail any of those conditions use an `-ineligible` suffix. + +This includes unauthorized fork opened, ready-for-review, reopened, and +synchronize events, as well as ineligible label events, draft events, and +events received while generation is disabled. Same-repository +`generate-pr-walkthrough` events and contributor `safe-to-review` events remain +in the shared group. The existing job-level authorization gate remains the +source of permission; group selection does not authorize a job. + +The eligible group keeps `cancel-in-progress: true`, so a newer eligible +walkthrough trigger still replaces an older run for the same pull request. +An ineligible event can be skipped independently and cannot cancel an eligible +generation, publication, or linking pipeline. This preserves the +`safe-to-review` authorization boundary. + ### Walkthrough artifact contract The walkthrough keeps the existing provider-neutral contract: @@ -159,6 +182,9 @@ The public object key remains - 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. +- An event that fails the walkthrough generation gate cannot cancel an eligible + walkthrough pipeline; it is skipped independently after its separate + concurrency group is selected. - 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 @@ -235,3 +261,4 @@ trigger. - [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) +- [Isolate ineligible PR walkthrough triggers](../../../decisions/2026-09-22-pr-walkthrough-eligibility-concurrency.md)