ci: run kurtosis E2E safely on fork PRs - #2380
Conversation
Fork PRs never receive secrets or an OIDC token on `pull_request`, so all three kurtosis e2e legs were structurally unable to run on external contributions. Reviewers had no e2e signal at merge time. Add a `pull_request_target` trigger so fork PRs run these legs with base-repo credentials against the fork's head commit, gated on the repo's outside-collaborator approval setting. The workflow definition always comes from the base branch, so a fork cannot rewrite the steps; each new push needs approving again. The two PR triggers are made mutually exclusive per job, since both firing would report the same check name twice. `Checkout bor` now pins the ref explicitly: `pull_request_target` defaults to the base branch, which would silently test develop instead of the PR. The build jobs gain an explicit read-only token, which fork `pull_request` runs previously got for free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
🟡 Changes recommended
The new job-level permissions override drops required actions scope for artifact uploads in the build jobs, which will break the workflow end-to-end.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the Kurtosis E2E GitHub Actions workflows to make fork-based pull requests able to run the three Kurtosis E2E legs by adding pull_request_target triggers (with maintainer approval) while keeping same-repo PR behavior on pull_request and avoiding duplicate job execution.
Changes:
- Add
pull_request_targettriggers to the three Kurtosis E2E workflows to enable fork PR runs with base-repo credentials behind approval. - Add per-job conditional guards to ensure only one of
pull_requestvspull_request_targetexecutes for a given PR origin. - Pin the Bor checkout
refforpull_request_targetruns to the PR head SHA and explicitly set job token permissions.
File summaries
| File | Description |
|---|---|
| .github/workflows/kurtosis-stateless-e2e.yml | Adds pull_request_target, job-level origin guards, and PR-head-SHA checkout pinning for fork PR coverage. |
| .github/workflows/kurtosis-pipeline-e2e.yml | Same trigger/guard/checkout adjustments to enable fork PR execution for pipeline E2E. |
| .github/workflows/kurtosis-e2e.yml | Same trigger/guard/checkout adjustments to enable fork PR execution for the main Kurtosis E2E leg. |
Review details
Suppressed comments (3)
.github/workflows/kurtosis-e2e.yml:79
permissionsis restricted tocontents: read, but this job uploads artifacts viaactions/upload-artifact, which requires theactionspermission. Without it, the upload step will be denied and downstream e2e jobs won't be able to download the images.
permissions:
contents: read
.github/workflows/kurtosis-stateless-e2e.yml:79
permissionsis restricted tocontents: read, but this job uploads artifacts viaactions/upload-artifact, which requires theactionspermission. Without it, the upload step will be denied and downstream e2e jobs won't be able to download the images.
permissions:
contents: read
.github/workflows/kurtosis-pipeline-e2e.yml:85
permissionsis restricted tocontents: read, but this job uploads artifacts viaactions/upload-artifact, which requires theactionspermission. Without it, the upload step will be denied and downstream e2e jobs won't be able to download the images.
permissions:
contents: read
- Files reviewed: 3/3 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #2380 +/- ##
===========================================
+ Coverage 55.21% 55.28% +0.06%
===========================================
Files 912 912
Lines 165870 165870
===========================================
+ Hits 91591 91697 +106
+ Misses 68818 68726 -92
+ Partials 5461 5447 -14 see 28 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Both `pull_request` and `pull_request_target` resolve `github.event.pull_request.number` to the same value, so a PR created one run per trigger and both landed in the same concurrency group. With `cancel-in-progress: true`, whichever was queued second cancelled the other — so the all-skipped run could cancel the real one, nondeterministically, on any PR rather than only fork PRs. Add `github.event_name` to the group. Cancellation within one event is unchanged, so successive pushes to a PR still supersede the previous run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Heads-up for reviewers: pushed ee75d83 to fix a concurrency-group collision that Copilot caught on the sibling heimdall-v2 PR (0xPolygon/heimdall-v2#641) — it applied identically here.
Note this affected same-repo PRs too, not just fork PRs, so it would have been a broad CI flake rather than a fork-only edge case. |
There was a problem hiding this comment.
🟡 Changes recommended
The pull_request_target path builds fork-authored code after checkout with persisted credentials enabled, which can leak the base repo GITHUB_TOKEN via the Docker build context/artifacts without an additional hardening tweak.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces pull_request_target execution with base-repo credentials (security-sensitive) and depends on repository-level approval settings that require maintainer verification before merge.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
Under `pull_request_target` these jobs check out fork-authored code with base-repo credentials. `actions/checkout` leaves the token in the local repo config, `COPY . .` ships that directory into the build context, and the built image is saved into a publicly downloadable artifact — so a PR-modified Dockerfile could read the token out of the build context. The version stamping in the build reads local tags, so the directory has to stay in the context; dropping the persisted credential is the narrower fix. Nothing in these jobs needs authenticated access after checkout, and neither repo fetches submodules here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces pull_request_target execution paths (security-sensitive) and depends on repository-level approval settings that must be validated by a maintainer before safely merging.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
codegenie review |
🧞 Codegenie ReviewWarning Degraded run: planner fallback. The planner failed, so deterministic default coverage was used. Downstream review completed, but consider rerunning. Reviewed all 19 hunks of the fork-safe Kurtosis E2E CI change (normal depth; planner ran degraded but coverage was complete). One verified issue: the new fork-only setup step in all three Kurtosis workflows installs the PyPI CoverageReviewed 19/19 hunks.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧞 Codegenie Review
Reviewed 12/12 hunks.
Coverage levels: deep 12, normal 0, light 0, skip 0.
— codegenie v0.5.5 (58f82a9b2c) · View Workflow Job
There was a problem hiding this comment.
🟡 Changes recommended
The workflows do not currently add the pull_request_target trigger described in the PR metadata (so the intended approval-gated fork behavior isn’t implemented), and the newly added cache-restore steps appear non-functional without any cache-save path in the repo.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
.github/workflows/kurtosis-e2e.yml:119
- This adds an
actions/cache/restorestep for/tmp/docker-images, but there is no correspondingactions/cache/saveusage anywhere in this repo (searched fordocker-images-developandactions/cache/save). As written, the restore will never hit and the subsequent load loop is effectively dead code; either add a save step in a trusted job that populates/tmp/docker-imagesor remove these steps to avoid extra complexity/time.
uses: actions/cache/restore@v5
with:
path: /tmp/docker-images
key: docker-images-develop-${{ github.sha }}
restore-keys: docker-images-develop-
.github/workflows/kurtosis-stateless-e2e.yml:119
- This adds an
actions/cache/restorestep for/tmp/docker-images, but there is no correspondingactions/cache/saveusage anywhere in this repo (searched fordocker-images-developandactions/cache/save). As written, the restore will never hit and the subsequent load loop is effectively dead code; either add a save step in a trusted job that populates/tmp/docker-imagesor remove these steps to avoid extra complexity/time.
uses: actions/cache/restore@v5
with:
path: /tmp/docker-images
key: docker-images-develop-${{ github.sha }}
restore-keys: docker-images-develop-
.github/workflows/kurtosis-pipeline-e2e.yml:134
- This adds an
actions/cache/restorestep for/tmp/docker-images, but there is no correspondingactions/cache/saveusage anywhere in this repo (searched fordocker-images-developandactions/cache/save). As written, the restore will never hit and the subsequent load loop is effectively dead code; either add a save step in a trusted job that populates/tmp/docker-imagesor remove these steps to avoid extra complexity/time.
uses: actions/cache/restore@v5
with:
path: /tmp/docker-images
key: docker-images-develop-${{ github.sha }}
restore-keys: docker-images-develop-
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
pratikspatil024
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
The implementation does not match the PR description regarding restoring a base-branch public-image cache for fork runs (no actions/cache/restore step is present), so either the workflows or the PR description needs to be corrected.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 3
- Review effort level: Lite
|
codegenie review |
There was a problem hiding this comment.
🧞 Codegenie Review
Warning
Degraded run: planner fallback. The planner failed, so deterministic default coverage was used. Downstream review completed, but consider rerunning.
Reviewed all 19 hunks of the fork-safe Kurtosis E2E CI change (normal depth; planner ran degraded but coverage was complete). One verified issue: the new fork-only setup step in all three Kurtosis workflows installs the PyPI yq package pinned to 4.1.2, a mikefarah/yq (Go) release identifier, so the fork leg either fails at install or runs a different yq implementation than the trusted leg. Reported as a single consolidated finding since the identical line appears in kurtosis-e2e.yml, kurtosis-pipeline-e2e.yml, and kurtosis-stateless-e2e.yml.
Reviewed 19/19 hunks.
Coverage levels: deep 0, normal 19, light 0, skip 0.
Coverage disclosure:
- planner degraded; deterministic default plan used
🙋 Needs human attention:
- Do
actions/upload-artifact@v7/actions/download-artifact@v8in these jobs need anyactionsscope now that build-bor and build-heimdall-v2 declarepermissions: contents: read(which zeroes all other scopes)? - Do actions/upload-artifact@v7 and actions/download-artifact@v8 require the GITHUB_TOKEN actions scope for same-run artifacts, now that an explicit permissions block sets actions to none in build-bor, build-heimdall-v2, and e2e-tests?
- Do the pos-workflows scripts (kurtosis_stateless_test.sh, kurtosis_pipeline_test.sh) invoke yq with mikefarah v4 syntax?
— codegenie v0.5.5 (58f82a9b2c) · View Workflow Job
|
@claude review |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it's grown substantially since my last review on this PR (persist-credentials hardening, reduced job permissions, and a whole new fork-PR credential-free install path across all three workflows) and touches CI secret/permission handling, a human look would still be worthwhile.
What was reviewed: the trusted-vs-fork gating (if: conditions ensuring secrets.DOCKERHUB* only flow on same-repo/non-PR runs), the added persist-credentials: false and scoped-down job permissions blocks, and the pinned/checksum-verified Kurtosis .deb install plus pinned foundry-toolchain/free-disk-space actions used only on the fork path. I also checked the python3 -m pip install yq==4.1.2 step flagged as a possible PEP 668 "externally-managed-environment" failure on ubuntu-latest — GitHub-hosted runner images keep the default system Python pip-installable, so this doesn't look like a blocking issue, though there's still an open automated review comment on that same line worth a maintainer's confirmation.
Extended reasoning...
Overview
The diff (three GitHub Actions workflow files: kurtosis-e2e.yml, kurtosis-pipeline-e2e.yml, kurtosis-stateless-e2e.yml) has grown considerably since my prior review, which was posted right after the first commit. Five commits now build out a complete credential-free fork-PR path: persist-credentials: false on every checkout, contents: read-only permissions on build jobs, removal of actions: write from the kurtosis job, an if-gated split between a "(trusted)" pre-run step (which alone receives secrets.DOCKERHUB*) and a parallel set of "(fork)" steps that install Kurtosis via a checksum-verified .deb, yq via pip, and Foundry via a pinned toolchain action.
Security risks
The core security property — that fork-authored PR runs never receive repository secrets or OIDC tokens — is enforced correctly by construction: GitHub itself withholds secrets/OIDC from fork pull_request runs (no pull_request_target is used), and the code additionally gates the one step that explicitly references secrets.DOCKERHUB* behind a same-repo/non-PR condition, so there's no redundant path where a fork run could reach that step. The fork-only install steps use a checksum-verified download and SHA-pinned third-party actions rather than floating tags, which is a reasonable mitigation for supply-chain risk on artifacts fetched without secrets. I did not have outbound network access in this sandbox to independently re-fetch and hash the Kurtosis .deb or resolve the pinned action SHAs, so I relied on the PR's own verification claims for those specific values.
Level of scrutiny
This is CI-only and does not touch Go/consensus code, but it does change trust boundaries around secrets and permissions in shared workflows, which merits a careful human pass — especially given the unusually long review/iteration history already visible on this PR (multiple rounds of Copilot and github-actions bot inline comments across several commits), which signals the author and other automated reviewers found real things to fix along the way. The most recent commit added the fork-only Kurtosis/yq/Foundry install steps, and there is a still-open (not resolved by anyone but the author) automated review comment on the exact pip install yq line in kurtosis-stateless-e2e.yml, posted after that commit landed.
Other factors
No bug reports were generated by this run's structured findings and I did not identify any myself. The one candidate I evaluated (PEP 668 "externally-managed-environment" possibly breaking the pip install on ubuntu-latest) appears unlikely to be a real blocker based on how GitHub's hosted runner images configure the default Python, but I can't fully rule it out without actually running the workflow, and there is an unresolved bot comment on that same line, so I'm surfacing it for a maintainer to confirm rather than treating it as settled.
There was a problem hiding this comment.
🔵 Needs a closer look
It changes CI security boundaries and fork PR execution behavior in multiple workflows, so a final human review should validate the threat model and runtime behavior end-to-end.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
* ci: run kurtosis e2e on fork PRs behind maintainer approval Fork PRs never receive secrets or an OIDC token on `pull_request`, so both kurtosis e2e legs were structurally unable to run on external contributions. Reviewers had no e2e signal at merge time. Same gap as 0xPolygon/bor#2380. Add a `pull_request_target` trigger, carrying the same `main`/`develop` base filter as the existing `pull_request` trigger, so fork PRs run these legs with base-repo credentials against the fork's head commit, gated on the repo's outside-collaborator approval setting. The workflow definition always comes from the base branch, so a fork cannot rewrite the steps; each new push needs approving again. The two PR triggers are made mutually exclusive per job, since both firing would report the same check name twice. `Checkout heimdall-v2` now pins its ref explicitly, since `pull_request_target` defaults to the base branch and would otherwise test develop instead of the PR. `Checkout bor` needs no pin — it builds pinned upstream bor, not PR-authored code. The build jobs gain an explicit read-only token, which fork `pull_request` runs previously got for free. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: keep the two PR triggers in separate concurrency groups Both `pull_request` and `pull_request_target` resolve `github.event.pull_request.number` to the same value, so a PR created one run per trigger and both landed in the same concurrency group. With `cancel-in-progress: true`, whichever was queued second cancelled the other — so the all-skipped run could cancel the real one, nondeterministically, on any PR rather than only fork PRs. Add `github.event_name` to the group. Cancellation within one event is unchanged, so successive pushes to a PR still supersede the previous run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: do not persist checkout credentials for fork-authored code Under `pull_request_target` these jobs check out fork-authored code with base-repo credentials. `actions/checkout` leaves the token in the local repo config, `COPY . .` ships that directory into the build context, and the built image is saved into a publicly downloadable artifact — so a PR-modified Dockerfile could read the token out of the build context. The version stamping in the build reads local tags, so the directory has to stay in the context; dropping the persisted credential is the narrower fix. Nothing in these jobs needs authenticated access after checkout, and neither repo fetches submodules here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: run fork E2E without privileged PR target * ci: verify fork E2E tool downloads * ci: update Foundry setup for fork E2E --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…cade) 10 commits of drift, no conflicts. Brings in WIT2 BP-signed witness announcements (#2208), the live-tracer system-transaction fix (#2353), V2 sender-nonce precompute (#2383), fork-PR kurtosis e2e (#2380) and dependency bumps. The 34 WIT2 files merged as pure additions — the base had no competing edits, so every witness and wit-protocol file resolved to develop's content unchanged. The sole witness-path difference against develop is core/stateless/stats.go, whose leaf-depth bounds clamp predates this merge on the base side and is preserved.
Summary
Run all three Kurtosis E2E workflows for external fork PRs without making repository credentials available to fork-authored code.
Security model
pull_requesttrigger handles both internal and external PRs. There is nopull_request_target.yqand Foundry versions.persist-credentials: false.contents: read; the unnecessaryactions: writepermission was removed.This also removes the duplicate-trigger and skipped-required-check failure modes from the earlier implementation.
reopenedis included on the single PR trigger.Verification
actionlint.git diff --check.kurtosis-pos@v1.4.2was verified anonymously pullable from its direct public reference.CI-only change; no client or consensus behavior is modified.