feat(ci): dispatch eval execution for fork PR secret access#123
Conversation
Design for secure eval execution on fork PRs using workflow_run dispatch with collaborator-based trust gating. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three tasks: modify eval-pr.yml to discovery-only, create eval-pr-run.yml dispatch workflow, update spec cross-reference. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stage 1 of the workflow_run dispatch pattern for fork PR secret access. Eval execution moves to eval-pr-run.yml (next commit). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stage 2 of the workflow_run dispatch pattern. Downloads PR metadata from eval-pr.yml, gates untrusted authors via eval-protected environment, then runs evals with secret access and posts the PR review. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Security fixes from review:
- Derive PR number and author from GitHub API instead of artifact
(prevents trust gate bypass via modified eval-pr.yml in fork PRs)
- Validate skills format against [a-z0-9-] allowlist
- Pass artifact-sourced values through env vars, not ${{ }} interpolation
(prevents shell injection)
- Remove --verbose from Claude invocation (prevents secret leakage in logs)
- Add branches: [main] filter to workflow_run trigger
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use artifact's pr_number to disambiguate when multiple PRs share a commit. Remove head_sha from job outputs (never consumed downstream). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reviewer's GuideSplits the eval PR GitHub Actions workflow into a two-stage workflow_run dispatch flow so eval discovery runs safely on fork PRs without secrets, while a new workflow with trust-gating executes evals with secret access and posts reviews; also adds design docs describing the architecture and links them from the existing eval CI spec. Sequence diagram for eval PR two-stage workflow_run dispatchsequenceDiagram
actor Contributor
participant GitHub_PR as GitHub_PR
participant Eval_PR as Eval_PR_Workflow
participant Artifact as Artifact_Storage
participant Eval_PR_Run as Eval_PR_Run_Workflow
participant GitHub_API as GitHub_API
participant Eval_Protected as Eval_Protected_Environment
Contributor->>GitHub_PR: Open_or_update_pull_request
GitHub_PR-->>Eval_PR: Trigger_pull_request_event
Eval_PR->>Eval_PR: Checkout_repository
Eval_PR->>Eval_PR: Discover_changed_skills
Eval_PR->>Eval_PR: Write_PR_metadata_json
Eval_PR->>Artifact: Upload_eval_pr_metadata_artifact
Eval_PR-->>Eval_PR_Run: Emit_workflow_run_completed_event
Eval_PR_Run->>Artifact: Download_eval_pr_metadata
Eval_PR_Run->>GitHub_API: Get_pull_requests_for_head_sha
GitHub_API-->>Eval_PR_Run: PR_identity_and_author
Eval_PR_Run->>GitHub_API: Get_collaborator_permission_level
GitHub_API-->>Eval_PR_Run: Permission_level
alt Trusted_collaborator
Eval_PR_Run->>Eval_PR_Run: Set_trusted_true
Eval_PR_Run->>Eval_PR_Run: Skip_eval_protected_gate
else Untrusted_contributor
Eval_PR_Run->>Eval_Protected: Request_environment_approval
Eval_Protected-->>Eval_PR_Run: Approval_granted
end
Eval_PR_Run->>Eval_PR_Run: Checkout_PR_merge_commit
Eval_PR_Run->>Eval_PR_Run: Authenticate_to_Google_Cloud
Eval_PR_Run->>Eval_PR_Run: Install_Claude_Code
Eval_PR_Run->>Eval_PR_Run: Run_evals_for_each_skill
Eval_PR_Run->>GitHub_PR: Post_or_update_eval_results_review
Flow diagram for collaborator trust gating logic in eval PR run workflowflowchart TD
Start[Start_discover_job] --> CheckMetadata[Load_eval_pr_metadata]
CheckMetadata --> ValidateSkills[Validate_skills_format_regex]
ValidateSkills -->|invalid| FailSkills[Set_workflow_failed_invalid_skills]
ValidateSkills -->|valid| ResolvePR[Resolve_PR_by_head_sha_via_GitHub_API]
ResolvePR -->|no_PR_found| FailPR[Set_workflow_failed_no_PR_for_commit]
ResolvePR -->|PR_found| DeriveIdentity[Derive_PR_number_and_author]
DeriveIdentity --> CheckPerm[Get_collaborator_permission_level]
CheckPerm -->|admin_or_write| Trusted[Set_trusted_true]
CheckPerm -->|other_or_not_collaborator| Untrusted[Set_trusted_false]
Trusted --> SkillsCheckTrusted[Skills_non_empty]
Untrusted --> SkillsCheckUntrusted[Skills_non_empty]
SkillsCheckTrusted -->|skills_empty| EndNoSkillsTrusted[End_no_evals_run]
SkillsCheckTrusted -->|skills_present| RunEvalsTrusted[Run_evals_without_gate]
SkillsCheckUntrusted -->|skills_empty| EndNoSkillsUntrusted[End_no_evals_run]
SkillsCheckUntrusted -->|skills_present| Gate[Require_eval_protected_environment_approval]
Gate -->|approved| RunEvalsUntrusted[Run_evals_after_gate]
RunEvalsTrusted --> EndTrusted[Post_eval_results_review]
RunEvalsUntrusted --> EndUntrusted[Post_eval_results_review]
FailSkills --> EndFailSkills[Workflow_failed]
FailPR --> EndFailPR[Workflow_failed]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
eval-pr-run.yml'sdiscoverjob you currently trust theskillslist from the artifact (with a regex check); for stronger defense-in-depth consider recomputing the changed skills from the checked-out repo in Stage 2 instead of propagating them via a user-controllable artifact. - The
workflow_runtrigger ineval-pr-run.ymlis additionally filtered bybranches: [main]; sinceworkflow_runalready fires from runs of theEval PRworkflow, you may want to confirm this branch filter is actually needed and doesn’t prevent legitimate PR runs (e.g., ifEval PRis ever triggered from other base branches).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `eval-pr-run.yml`'s `discover` job you currently trust the `skills` list from the artifact (with a regex check); for stronger defense-in-depth consider recomputing the changed skills from the checked-out repo in Stage 2 instead of propagating them via a user-controllable artifact.
- The `workflow_run` trigger in `eval-pr-run.yml` is additionally filtered by `branches: [main]`; since `workflow_run` already fires from runs of the `Eval PR` workflow, you may want to confirm this branch filter is actually needed and doesn’t prevent legitimate PR runs (e.g., if `Eval PR` is ever triggered from other base branches).
## Individual Comments
### Comment 1
<location path=".github/workflows/eval-pr-run.yml" line_range="41-50" />
<code_context>
+ - name: Resolve PR identity and check trust
</code_context>
<issue_to_address>
**🚨 issue (security):** Relying on untrusted metadata.pr_number to pick a PR can mis-route reviews if a commit is shared
Here you prefer `metadata.pr_number` from an untrusted artifact over the authoritative PR list from `listPullRequestsAssociatedWithCommit`. If a commit is shared across multiple PRs (cherry-picks/backports), a malicious workflow could steer reviews to the wrong PR.
Given you already have `prs` from the GitHub API, consider either:
- Ignoring `metadata.pr_number` and selecting solely from `prs` (e.g., require `prs.length === 1` or pick based on expected base/head), or
- Validating `metadata.pr_number` against `prs` plus extra signals (base branch, author) and failing on mismatch.
This keeps the artifact informational-only and avoids relying on untrusted input for routing.
</issue_to_address>
### Comment 2
<location path=".github/workflows/eval-pr-run.yml" line_range="12-16" />
<code_context>
+
+name: Eval PR Run
+
+on:
+ workflow_run:
+ workflows: ["Eval PR"]
+ types: [completed]
+ branches: [main]
+
+permissions:
</code_context>
<issue_to_address>
**question (bug_risk):** The branches filter on workflow_run may unintentionally suppress runs for non-main PRs
Because `workflow_run` uses the *completed workflow’s* branch for `branches` filtering, `branches: [main]` here will typically not match PR runs (which run on the PR’s head branch, not `main`), so this runner may never fire for most PRs targeting `main`.
If you want evals for all PRs into `main`, consider either removing the `branches` filter and relying on the trust/approval checks, or checking the PR’s base branch in `discover` and exiting early when it isn’t `main` to avoid silently skipping valid PRs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
branches: [main] on workflow_run filters on the PR's head branch, not the base branch — confirmed broken by PR #123 (Stage 2 never triggered). Remove it; the triggering workflow's own filter already restricts to PRs targeting main. Also remove author from artifact metadata since it's derived from the GitHub API in Stage 2 for security. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Select PR solely from GitHub API (open PRs targeting main for the commit SHA). Artifact now carries only the skills CSV — no identity fields that could influence routing. Addresses Sourcery review feedback on PR #123. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
mrizzi
left a comment
There was a problem hiding this comment.
Addressing both items:
1. Skills from artifact: Fixed in 206a78d — PR identity (pr_number, author, head_sha) is no longer in the artifact at all. The artifact now carries only skills, which is validated against ^[a-z0-9-]+(,[a-z0-9-]+)*$ in Stage 2.
Recomputing skills in Stage 2 would require the base SHA for git diff, which isn't available from the workflow_run event context. The worst case with a spoofed skills list is running evals for skills that didn't change — wasted compute, not a security breach, since the trust check and PR selection are fully API-derived.
2. branches: [main]: Fixed in d3cc5f9. Removed. Confirmed broken by the PR's own CI (Stage 2 never triggered with the filter present).
Update spec to reflect security hardening changes: - Artifact carries only skills (not pr_number, head_sha, author) - PR identity derived from GitHub API via listPullRequestsAssociatedWithCommit - Skills validated against allowlist regex - Gate and run-evals conditions include needs.discover.result check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Stage 1 (eval-pr.yml) is now a minimal path-filtered trigger with no artifact. Stage 2 (eval-pr-run.yml) derives everything from the GitHub API and the checked-out PR merge commit: PR identity from the API, skills from git diff on the checkout. No data from Stage 1 is trusted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove all artifact references. Stage 1 is now a minimal trigger, Stage 2 derives everything from the GitHub API and git diff. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
listPullRequestsAssociatedWithCommit returns empty for fork PRs because the commit lives in the fork's repo, not the base repo's commit graph. Use pulls.list filtered by base branch and match by head.sha instead. Validated in https://github.com/mrizzi/test-workflow-dispact/actions/runs/25561566262 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use github.paginate to handle repos with many open PRs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
workflow_run workflows don't appear as status checks on the PR page. Use the Commit Status API to post pending/success/failure statuses on the PR's head SHA so authors and reviewers have visibility without checking the Actions tab. Validated in mrizzi/test-workflow-dispact#2 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change report-status condition from needs.discover.result == 'success' to always() so it runs even when discover fails. Adds discover result to the status logic with a specific failure message. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
branches: [main] on workflow_run filters on the PR's head branch, not the base branch — confirmed broken by PR #123 (Stage 2 never triggered). Remove it; the triggering workflow's own filter already restricts to PRs targeting main. Also remove author from artifact metadata since it's derived from the GitHub API in Stage 2 for security. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Select PR solely from GitHub API (open PRs targeting main for the commit SHA). Artifact now carries only the skills CSV — no identity fields that could influence routing. Addresses Sourcery review feedback on PR #123. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
eval-pr.ymlinto a two-stageworkflow_rundispatch pattern so fork PRs can run evals with secret accesseval-pr.yml) discovers changed skills and uploads metadata artifact — no secrets neededeval-pr-run.yml) downloads artifact, checks collaborator trust via GitHub API, gates untrusted authors behind a protected environment, then runs evals and posts the PR review${{ }}interpolation of untrusted values in shell,--verboseremovedContext
PR #90 from a fork failed because GitHub does not pass secrets to workflows triggered from forked repositories. This PR implements the officially documented
workflow_runpattern to solve this.Trust Model
eval-protectedenvironmentPost-Merge Setup
Create the
eval-protectedenvironment in repo settings:eval-protectedTest Plan
eval-pr.ymltriggers on a PR that modifies skills/evals fileseval-pr-run.ymlfires aftereval-pr.ymlcompletesbranches: [main]filter does not prevent Stage 2 from triggering — remove in follow-up if it does🤖 Generated with Claude Code
Summary by Sourcery
Introduce a two-stage eval PR CI pipeline that supports forked PRs with secure secret access and collaborator-based trust gating.
Enhancements:
Documentation: