dreampiler/kandev
1
0
Fork 0
Code Issues 43 Pull Requests Actions 6 Packages Projects Releases Wiki Activity

fix(ci): isolate ineligible PR walkthrough triggers (#3862)

Browse Source
This commit is contained in:
Carlos Florêncio
2026-09-22 13:20:07 +01:00
committed by GitHub
parent 8690df2f79
commit 0eea98528c
7 changed files with 374 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.github/scripts/pr-walkthrough-workflow-contract_test.py
+58 -1
View File
@@ -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(
.github/workflows/pr-walkthrough.yml
+33 -1
View File
@@ -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:
docs/decisions/2026-09-22-pr-walkthrough-eligibility-concurrency.md
+55
View File
@@ -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-<number>`. 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.
docs/plans/pr-walkthrough-eligibility-concurrency/plan.md
+95
View File
@@ -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.
docs/plans/pr-walkthrough-eligibility-concurrency/task-01-guard-ineligible-triggers.md
+100
View File
@@ -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.
docs/specs/ci/requirements/unified-contributor-pr-automation.md
+6
View File
@@ -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
docs/specs/ci/system-design/unified-contributor-pr-automation.md
+27
View File
@@ -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-<number>` 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)