Repository navigation
Conversation
Fork PRs don't have access to repo secrets (API keys, HF_TOKEN), causing e2e tests, vendor tests, benchmarks, and go-bindings-e2e to fail with missing credentials after maintainer approval. Add fork detection guard to all secret-dependent jobs: github.event.pull_request.head.repo.full_name == github.repository This skips these jobs on fork PRs while still running them on: - push to main - internal PRs (same repo) Affected jobs: benchmarks, e2e-1gpu-chat, e2e-1gpu-embeddings, e2e-1gpu-gateway, e2e-2gpu-responses, e2e-2gpu-pd, e2e-vendor (anthropic-messages, openai-responses, openai-realtime, xai-responses), go-bindings-e2e. Cascading jobs (e2e-2gpu-chat, e2e-4gpu-chat) auto-skip when their dependencies skip. The finish job already handles skipped results correctly (checks for "failure", not "skipped"). Fork PRs will still run: pre-commit, python-lint, unit-tests, build-wheel, python-unit-tests, go-unit-tests, grpc-proto-build-check. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
Note Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds CI workflow gating and a new fork-safe integration workflow: tightens pull_request job conditionals to require the PR head repo match the workflow repo; adds a workflow_call input to e2e GPU jobs to allow checking out a specific ref; introduces a fork-focused pull_request_target workflow that runs gated builds, artifact production, and multi-engine E2E/benchmark jobs. Changes
Sequence Diagram(s)sequenceDiagram
participant Contributor as Contributor (fork PR)
participant GitHub as GitHub Actions
participant BaseRepo as Base repo (pull_request_target)
participant Runner as Runner (build/test)
participant Storage as Artifact Storage
Note over Contributor,GitHub: Fork PR labeled "safe-to-test"
Contributor->>GitHub: Open PR (fork)
GitHub->>BaseRepo: trigger pull_request_target workflow (reads label)
BaseRepo->>Runner: Checkout PR head SHA via base repo context
Runner->>Runner: Build wheel / Go FFI / WASM / client types
Runner->>Storage: Upload artifacts
GitHub->>Runner: Start matrixed E2E jobs (download artifacts)
Runner->>Storage: Download artifacts
Runner->>Runner: Run E2E tests (GPU/CPU matrices)
Runner->>Storage: Upload test results / sccache stats
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Create pr-test-fork-integration.yml — a separate workflow that runs secret-dependent jobs for fork PRs via `pull_request_target`. Flow: 1. Fork PR opened → pr-test-rust.yml runs non-secret jobs (lint, build, unit tests). Secret-dependent jobs are skipped. 2. Maintainer reviews the fork's code 3. Maintainer adds `safe-to-test` label 4. This workflow triggers via pull_request_target (labeled event), checks out the PR's HEAD SHA, runs all integration tests with access to repo secrets Security: - Only triggers on `labeled` event (not synchronize) — if the contributor pushes new commits, maintainer must remove and re-add the label after reviewing the new code - Guard job checks both the label name AND that it's a fork PR (internal PRs are handled by pr-test-rust.yml) - All checkout steps use the PR's HEAD SHA, not a branch ref Also adds `checkout_ref` input to e2e-gpu-job.yml reusable workflow so callers can specify which ref to checkout (needed for pull_request_target which defaults to the base branch). Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2faadc5c1d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| on: | ||
| pull_request_target: | ||
| branches: [main] | ||
| types: [labeled] |
There was a problem hiding this comment.
pull_request_target is limited to types: [labeled], so after safe-to-test is added the secret-dependent workflow runs only once for that label event. If the fork author pushes another commit afterward, pr-test-rust.yml now skips the secret jobs on forks and this workflow does not retrigger, leaving the latest PR head SHA without integration coverage. Add synchronize (and gate on the label being present) so approved fork PRs are revalidated on subsequent pushes.
Useful? React with 👍 / 👎.
1 participant
Summary
Fork PRs can now run the full integration test suite (including secret-dependent jobs) after a maintainer adds the
safe-to-testlabel.What changed
1.
pr-test-rust.yml— skip secret jobs on fork PRsAdded fork detection guard to 8 secret-dependent jobs so they show as "skipped" (not "failed") on fork PRs:
Non-secret jobs (lint, build, unit tests) always run for everyone.
2.
pr-test-fork-integration.yml— NEW label-gated workflowRuns secret-dependent jobs for fork PRs via
pull_request_target:safe-to-testlabel3.
e2e-gpu-job.yml— addedcheckout_refinputThe reusable workflow now accepts an optional
checkout_refinput so callers can specify which git ref to checkout (needed forpull_request_targetwhich defaults to the base branch).Security model
pr-test-rust.ymlruns non-secret CI automatically (lint, build, unit tests)safe-to-testlabelpr-test-fork-integration.ymltriggers, runs integration tests with secretslabeled, notsynchronize)Why
GitHub never passes secrets to
pull_requestevents from forks. The only way to get secrets for fork PRs ispull_request_target, which runs in the base repo's context. A separate workflow with a label gate is the safest approach — it isolates thepull_request_targettrigger and requires explicit maintainer approval.Test plan
pr-test-rust.ymlPR_HEAD_SHAsafe-to-testlabel in the repoSummary by CodeRabbit