Skip to content

fix(ci): fix rerun workflows for fork PRs#13512

Merged
petebacondarwin merged 3 commits intomainfrom
fix/rerun-workflows-fork-prs
Apr 17, 2026
Merged

fix(ci): fix rerun workflows for fork PRs#13512
petebacondarwin merged 3 commits intomainfrom
fix/rerun-workflows-fork-prs

Conversation

@petebacondarwin
Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin commented Apr 15, 2026

Fixes the rerun-codeowners workflow failing on fork PRs (e.g. https://github.com/cloudflare/workers-sdk/actions/runs/24451203502/job/71440264028?pr=12456).

For fork PRs, pull_request_review and pull_request events give a read-only GITHUB_TOKEN with no access to secrets, so the rerun workflows cannot call the Actions API to re-run jobs/workflows.

This PR fixes the issue by splitting each rerun workflow into a trigger + privileged pair using the workflow_run pattern:

  • Trigger workflow fires on the original event (pull_request_review or pull_request labeled/unlabeled), saves PR context (head SHA, workflow targets) as an artifact, and exits. No privileged API calls.
  • Privileged workflow fires on workflow_run when the trigger completes. workflow_run always runs from the default branch with full permissions, so it can perform the re-run even for fork PRs.

The two privileged workflows use different lookup strategies because of a GitHub API asymmetry:

Workflow Strategy Why
rerun-codeowners-privileged.yml Check-name lookup codeowners.yml uses pull_request_target — its runs are indexed by the base branch SHA, not the PR head. But check runs ARE on the PR head commit, so we find by check name.
rerun-remote-tests-privileged.yml Workflow-file lookup E2E workflows use pull_request — their runs are indexed by the PR head SHA. Also, E2E job names are matrix-expanded (e.g. "Wrangler E2E (Linux, shard 1/4)") making check-name lookup fragile.

Also renames rerun_codeowners.ymlrerun-codeowners.yml for consistency with the rest of the workflow files.


  • Tests

    • Tests included/updated
    • Automated tests not possible - manual testing has been completed as follows:
    • Additional testing not necessary because: CI workflow changes — will be validated on the next fork PR that triggers a review or label change
  • Public documentation

    • Cloudflare docs PR(s):
    • Documentation not necessary because: internal CI change only

Open with Devin

@petebacondarwin petebacondarwin added the no-changeset-required Skip PR check for valid changeset label Apr 15, 2026
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 15, 2026

⚠️ No Changeset found

Latest commit: d4656a6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk Apr 15, 2026
@workers-devprod workers-devprod requested review from a team and edmundhung and removed request for a team April 15, 2026 12:44
@workers-devprod
Copy link
Copy Markdown
Contributor

workers-devprod commented Apr 15, 2026

Codeowners approval required for this PR:

  • ✅ @cloudflare/wrangler
Show detailed file reviewers

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Apr 15, 2026

Let me now look closely at the security aspects. The rerun-codeowners.yml uses ${{ github.event.pull_request.head.sha }} directly in a run: block, and rerun-remote-tests.yml uses both ${{ github.event.pull_request.head.sha }} and ${{ github.event.label.name }} directly in run: blocks. Let me think about whether these are actual injection risks:

  1. github.event.pull_request.head.sha — This is a 40-char hex SHA. GitHub enforces the format; it cannot be crafted by an attacker. Not injectable.

  2. github.event.label.name — This is a label name on the PR. On fork PRs, external contributors cannot create labels (label management requires write access to the repo). The if: guard only matches two specific label names. Not injectable by external actors in practice, since labels can only be set by repo collaborators.

However, the rerun-codeowners.yml trigger workflow fires on any pull_request_review event but does NOT check whether the trigger workflow actually succeeded before the privileged workflow runs — wait, it does check github.event.workflow_run.conclusion == 'success'. Good.

Let me also check one more thing — the rerun-remote-tests.yml has a job-level if that filters to only run for the relevant labels. If the job is skipped, the workflow still "completes" with success conclusion. The privileged workflow then fires but the download-artifact step will fail (no artifact), handled by continue-on-error: true. The subsequent step checks steps.download-context.outcome == 'success' and skips. This is correctly handled.

Let me also think about a race condition: if both the codeowners and remote-tests trigger workflows use the artifact name pr-context, could there be a conflict? No — artifacts are scoped to a workflow run, so different workflow runs each have their own artifacts. And the privileged workflows download from run-id: ${{ github.event.workflow_run.id }}, which is the specific trigger run. This is fine.

One thing I want to verify — the rerun-remote-tests.yml if condition at the job level. When the job is skipped because the label doesn't match, the workflow conclusion is success (skipped jobs don't cause failure). The privileged workflow will fire, try to download the artifact (which doesn't exist since no artifact was uploaded), get an error from continue-on-error: true, and then the next step's if check prevents execution. This works, but it means the privileged workflow fires unnecessarily on every unrelated label change. This isn't a bug — it's a minor efficiency concern — but the design seems intentional given the constraints.

Now let me look at one potential issue more carefully. In rerun-codeowners-privileged.yml line 68, the jq expression extracts the job ID from the details URL. If check_runs is an empty array, check_runs[0] is null, and .details_url is null, so // "" gives "". Then split("/") | last gives "", which passes to split("?") | first giving "". The || true ensures no error exit. Then [ -n "" ] is false, so it falls to the else branch with a warning. This is handled correctly.

I've thoroughly reviewed the code and I don't see any actionable bugs, security issues, or backward compatibility problems. The approach is well-documented, the permissions are correctly scoped, and edge cases are handled.

LGTM

github run

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Apr 15, 2026

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@13512

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@13512

miniflare

npm i https://pkg.pr.new/miniflare@13512

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@13512

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@13512

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@13512

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13512

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@13512

wrangler

npm i https://pkg.pr.new/wrangler@13512

commit: d4656a6

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional findings.

Open in Devin Review

@petebacondarwin petebacondarwin added the run-remote-tests Run remote/E2E tests that require Cloudflare API credentials label Apr 15, 2026
devin-ai-integration[bot]

This comment was marked as resolved.

devin-ai-integration[bot]

This comment was marked as resolved.

@petebacondarwin petebacondarwin removed the run-remote-tests Run remote/E2E tests that require Cloudflare API credentials label Apr 15, 2026
Comment thread .github/workflows/rerun-codeowners.yml Outdated
Comment thread .github/workflows/rerun-remote-tests.yml
For fork PRs, pull_request_review and pull_request events give a
read-only GITHUB_TOKEN with no access to secrets. This means the
rerun-codeowners and rerun-remote-tests workflows cannot call the
Actions API to re-run jobs/workflows.

Fix this by splitting each workflow into a trigger + privileged pair:
- The trigger workflow fires on the original event, saves PR context
  (head SHA, targets) as an artifact, and exits.
- The privileged workflow fires on workflow_run when the trigger
  completes. workflow_run always runs from the default branch with full
  permissions, so it can perform the re-run even for fork PRs.

The two privileged workflows use different lookup strategies because of
a GitHub API asymmetry:
- Codeowners uses check-name lookup: pull_request_target runs are
  indexed by the base branch SHA, but check runs are on the PR head
  commit.
- Remote tests uses workflow-file lookup: pull_request runs are indexed
  by the PR head SHA, and E2E job names are matrix-expanded so
  check-name lookup would be fragile.

Also renames rerun_codeowners.yml to rerun-codeowners.yml for
consistency with the rest of the workflow files.
Update stale references to the old rerun_codeowners.yml filename and
document the new two-workflow design (trigger + privileged) in both
CODEOWNERS.md and .github/workflows/README.md. Also adds documentation
for the run-c3-frameworks-tests label support.
- rerun-remote-tests.yml: switch from two-workflow artifact pattern to
  single pull_request_target workflow (simpler, same security)
- rerun-codeowners.yml: remove artifact upload/download; privileged
  workflow now reads head SHA from github.event.workflow_run.head_sha
  (GitHub-provided metadata, not controllable by fork code)
- Delete rerun-remote-tests-privileged.yml (no longer needed)
@petebacondarwin petebacondarwin force-pushed the fix/rerun-workflows-fork-prs branch from a25f8b3 to d4656a6 Compare April 17, 2026 15:23
Copy link
Copy Markdown
Member

@edmundhung edmundhung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

Copy link
Copy Markdown
Contributor

@workers-devprod workers-devprod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codeowners reviews satisfied

@github-project-automation github-project-automation Bot moved this from Untriaged to Approved in workers-sdk Apr 17, 2026
@petebacondarwin petebacondarwin merged commit 3355bdc into main Apr 17, 2026
60 of 61 checks passed
@petebacondarwin petebacondarwin deleted the fix/rerun-workflows-fork-prs branch April 17, 2026 17:53
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk Apr 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changeset-required Skip PR check for valid changeset

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants