feat(ci): automate exact-head live E2E gating - #6589
Conversation
prekshivyas
added
area: ci
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds PR-scoped exact-commit validation and risk dispatch to ChangesPR-scoped required-live E2E risk gate
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CI Pull Request
participant Coordinator as pr-e2e-risk-gate.yaml
participant CLI as pr-risk-gate.mts
participant GitHub as GitHub API
participant E2E as e2e.yaml
CI->>Coordinator: workflow_run completed
Coordinator->>CLI: initialize and resolve exact PR
CLI->>GitHub: create check and fetch PR state
Coordinator->>Coordinator: poll for matching Advisor artifact
Coordinator->>CLI: start required-live plan
CLI->>E2E: dispatch risk_pr workflow
Coordinator->>CLI: finish or abandon
CLI->>GitHub: complete required-live check
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
4ab13e9 to
9f627e8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/pr-e2e-risk-gate.yaml (2)
13-17: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope
actions: write/checks: writeto the jobs that need them.Workflow-level
actions: writeandchecks: writegrant both jobs more than they need.cancel-supersededonly needsactions: write+contents: read;checks/pull-requestsare only used bycoordinate. Moving to per-jobpermissions:reduces blast radius on these trusted triggers.🔒 Suggested per-job least-privilege split
-permissions: - actions: write - checks: write - contents: read - pull-requests: read +permissions: {}cancel-superseded: if: ${{ github.repository == 'NVIDIA/NemoClaw' && github.event_name == 'pull_request_target' }} runs-on: ubuntu-latest + permissions: + actions: write + contents: read timeout-minutes: 5coordinate: if: ${{ github.repository == 'NVIDIA/NemoClaw' && github.event_name == 'workflow_run' && github.event.workflow_run.event == 'pull_request' }} runs-on: ubuntu-latest + permissions: + actions: write + checks: write + contents: read + pull-requests: read timeout-minutes: 130As per path instructions: "Keep permissions least-privileged and pass untrusted values as data rather than interpolating them into shell programs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-e2e-risk-gate.yaml around lines 13 - 17, The workflow-level permissions are too broad for both jobs in pr-e2e-risk-gate.yaml; scope them per job instead. Update the workflow so cancel-superseded keeps only the permissions it needs (actions and contents), and move checks and pull-requests permissions to the coordinate job where they are actually used. Use the job names cancel-superseded and coordinate to place the permissions blocks correctly.Sources: Path instructions, Linters/SAST tools
113-121: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPass resolved outputs as
envdata instead of${{ }}template expansion intorun:.Lines 116–121 interpolate
steps.resolve.outputs.*/steps.advisor.outputs.artifact_dirdirectly into the shell command (same pattern at Lines 144–147 and 156). Although these outputs are name-allowlisted and newline-rejected upstream, the path instruction requires passing PR-derived values as environment data rather than expanding them into the program text.🛡️ Example for the start step (apply the same to finish/abandon)
- id: start name: Build exact-head plan and dispatch required live E2E if: ${{ always() }} env: GITHUB_TOKEN: ${{ github.token }} + PR_NUMBER: ${{ steps.resolve.outputs.pr_number }} + BASE_SHA: ${{ steps.resolve.outputs.base_sha }} + HEAD_SHA: ${{ steps.resolve.outputs.head_sha }} + HEAD_REPO: ${{ steps.resolve.outputs.head_repo }} + CI_GREEN: ${{ steps.resolve.outputs.ci_green }} + ADVISOR_DIR: ${{ steps.advisor.outputs.artifact_dir }} run: >- node --experimental-strip-types tools/e2e-advisor/pr-risk-gate.mts --mode start - --pr "${{ steps.resolve.outputs.pr_number }}" - --base "${{ steps.resolve.outputs.base_sha }}" - --head "${{ steps.resolve.outputs.head_sha }}" - --head-repo "${{ steps.resolve.outputs.head_repo }}" - --ci-green "${{ steps.resolve.outputs.ci_green }}" - --advisor-dir "${{ steps.advisor.outputs.artifact_dir }}" + --pr "$PR_NUMBER" + --base "$BASE_SHA" + --head "$HEAD_SHA" + --head-repo "$HEAD_REPO" + --ci-green "$CI_GREEN" + --advisor-dir "$ADVISOR_DIR"As per path instructions: "pass untrusted values as data rather than interpolating them into shell programs."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/pr-e2e-risk-gate.yaml around lines 113 - 121, The PR-risk-gate workflow is interpolating resolved step outputs directly into the shell command instead of passing them as environment data. Update the start, finish, and abandon invocations in the workflow to put the resolved PR/base/head/repo/CI-green/advisor directory values into env on the step, and have the pr-risk-gate.mts command read them from environment variables rather than `${{ }}` expansion in run.Sources: Path instructions, Linters/SAST tools
tools/e2e-advisor/pr-risk-gate.mts (1)
267-283: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
cancel()should useargs.heador stop passing it.
It currently cancels every queued/in_progressE2E PR #<n> riskrun for the PR, so the current revision can’t be spared. If this is meant to keep only the latest head, filter byrun.head_sha; otherwise drop--headfrom the caller.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/e2e-advisor/pr-risk-gate.mts` around lines 267 - 283, The cancel() logic currently cancels every queued/in_progress E2E PR run matching the PR prefix, so it cannot preserve the current revision. Update cancel() in pr-risk-gate.mts to honor args.head by filtering the WorkflowRun results on run.head_sha before calling githubApi, or remove the --head path from the caller if that behavior is not intended; use the existing cancel() and githubRestPaginated()/githubApi flow as the place to apply the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/pr-e2e-risk-gate.yaml:
- Around line 13-17: The workflow-level permissions are too broad for both jobs
in pr-e2e-risk-gate.yaml; scope them per job instead. Update the workflow so
cancel-superseded keeps only the permissions it needs (actions and contents),
and move checks and pull-requests permissions to the coordinate job where they
are actually used. Use the job names cancel-superseded and coordinate to place
the permissions blocks correctly.
- Around line 113-121: The PR-risk-gate workflow is interpolating resolved step
outputs directly into the shell command instead of passing them as environment
data. Update the start, finish, and abandon invocations in the workflow to put
the resolved PR/base/head/repo/CI-green/advisor directory values into env on the
step, and have the pr-risk-gate.mts command read them from environment variables
rather than `${{ }}` expansion in run.
In `@tools/e2e-advisor/pr-risk-gate.mts`:
- Around line 267-283: The cancel() logic currently cancels every
queued/in_progress E2E PR run matching the PR prefix, so it cannot preserve the
current revision. Update cancel() in pr-risk-gate.mts to honor args.head by
filtering the WorkflowRun results on run.head_sha before calling githubApi, or
remove the --head path from the caller if that behavior is not intended; use the
existing cancel() and githubRestPaginated()/githubApi flow as the place to apply
the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 65ea3c76-535b-4662-bcf4-40fccabec849
📒 Files selected for processing (7)
.github/workflows/e2e.yaml.github/workflows/pr-e2e-risk-gate.yamltest/e2e/README.mdtest/pr-e2e-risk-gate-workflow.test.tstest/pr-e2e-risk-gate.test.tstools/e2e-advisor/post-merge-risk-gate.mtstools/e2e-advisor/pr-risk-gate.mts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/pr-e2e-risk-gate.test.ts (1)
58-68: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest title doesn't match what's actually being tested.
The title claims to test rejection of an artifact "from another revision," but both the artifact's
risk-plan.jsonand the call use the sameheadSha("a".repeat(40)); the mismatch actually exercised ischangedFiles. There's no dedicated coverage for an actualheadShamismatch case inadvisorJobs, per the upstream contract that checks bothartifactPlan.headSha !== headShaand thechangedFilesequality.Consider renaming this test to reflect the changed-files mismatch, and adding a separate case for a genuine
headShamismatch to cover both branches of the exact-head validation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pr-e2e-risk-gate.test.ts` around lines 58 - 68, Rename the advisorJobs test to reflect that it is exercising a changedFiles mismatch rather than a different revision, since the artifact() setup and advisorJobs() call both use the same headSha. Then add a separate test in pr-e2e-risk-gate.test.ts that passes a different headSha into advisorJobs to cover the artifactPlan.headSha !== headSha branch, keeping the existing exact-head validation coverage split between changed-files and head-sha mismatches.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-e2e-risk-gate.yaml:
- Around line 46-48: The coordinate job in pr-e2e-risk-gate.yaml has too little
timeout headroom relative to its internal waits, risking forced termination
before the always() cleanup in the finish/abandon steps can run. Increase the
job-level timeout on the coordinate job (or reduce the gh run watch budget
inside the same job) so the workflow_run handling, artifact polling, and cleanup
logic in coordinate, finish, and abandon all reliably complete within the job
limit.
- Around line 108-121: The `start` step in `pr-e2e-risk-gate.yaml` is
interpolating `steps.resolve.outputs.*` directly into the `run` script; move
those values into `env:` and reference them as shell variables in the command
instead. Apply the same pattern to the related `finish` and `abandon` steps,
using the existing `resolve` and `cancel` steps as the safe reference pattern,
so the workflow passes data to `pr-risk-gate.mts` without embedding untrusted
values in the shell program.
- Around line 13-17: Move the broad workflow-level permissions in this GitHub
Actions workflow into the specific jobs that need them so the token is
least-privileged. Keep `actions: write` only on the `cancel-superseded` job and
`checks: write` only on the `coordinate` job, while leaving the shared workflow
permissions minimal; use the job names `coordinate` and `cancel-superseded` to
place the scoped permissions correctly.
- Around line 19-21: The concurrency settings for the coordinator workflow use
different group keys for `pull_request_target` and `workflow_run`, which
prevents a newer run from cancelling an older superseded `coordinate` run.
Update the `concurrency.group` in `pr-e2e-risk-gate.yaml` to use one shared
identifier for both triggers, using the existing workflow context from the
coordinator job so `cancel-in-progress` can stop stale runs consistently.
---
Nitpick comments:
In `@test/pr-e2e-risk-gate.test.ts`:
- Around line 58-68: Rename the advisorJobs test to reflect that it is
exercising a changedFiles mismatch rather than a different revision, since the
artifact() setup and advisorJobs() call both use the same headSha. Then add a
separate test in pr-e2e-risk-gate.test.ts that passes a different headSha into
advisorJobs to cover the artifactPlan.headSha !== headSha branch, keeping the
existing exact-head validation coverage split between changed-files and head-sha
mismatches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c7315ca0-29e6-45f0-ac51-746a2f5e037c
📒 Files selected for processing (7)
.github/workflows/e2e.yaml.github/workflows/pr-e2e-risk-gate.yamltest/e2e/README.mdtest/pr-e2e-risk-gate-workflow.test.tstest/pr-e2e-risk-gate.test.tstools/e2e-advisor/post-merge-risk-gate.mtstools/e2e-advisor/pr-risk-gate.mts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/pr-e2e-risk-gate-workflow.test.ts
- tools/e2e-advisor/pr-risk-gate.mts
- tools/e2e-advisor/post-merge-risk-gate.mts
- .github/workflows/e2e.yaml
997be37 to
853e308
Compare
f7d04ab to
47bb7b1
Compare
f2a20cf to
ab1ae30
Compare
ab1ae30 to
ea581d2
Compare
|
Hold exact head |
ea581d2 to
eb15562
Compare
eb15562 to
b0bd5eb
Compare
|
Exact head b0bd5eb remains six commits behind current main (merge base 521bb13), so the post-#6583 refresh is still outstanding. It also fails codebase-growth-guardrails because test/post-merge-e2e-risk-gate.test.ts raises the test-file if count from 0 to 1; please linearize that branch when rebasing after #6583 lands. The existing hold and sequencing remain unchanged, so no live run is useful yet. |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
b0bd5eb to
b273dde
Compare
# Conflicts: # tools/e2e-advisor/post-merge-risk-gate.mts
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
prekshivyas
removed
the
needs: unblock
|
Exact-head security/gate follow-up for
The PR is still not merge-ready: it is stale against current Required sequence: sync current main; rerun ordinary gates and both trusted advisors on the resulting exact head; run exact-head |
|
@coderabbitai review |
✅ Action performedReview finished.
|
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Security/correctness review of exact head 8e7b07f9b916ae9f973b014de32d832dc0d09e2f: FAIL / changes required.
-
Every successful PR child is rejected by the evidence verifier.
.github/workflows/e2e.yaml:5names the childE2E PR #<pr> risk <correlation>, andpr-risk-gate.mts:348-357persistsprNumber. Butpost-merge-risk-gate.mts:627-645still requiresE2E risk <correlation>, andfinishRiskGate()never supplies the PR number (:894-898). A direct exact-head probe reproduces the identity mismatch. The 39 focused tests pass because no test finishes a PR-named child. ThreadprNumberinto the correlated identity contract and add a full PR finish regression. -
The secret-bearing child has no unforgeable coordinator authorization.
e2e.yaml:10-61exposes all risk inputs to ordinaryworkflow_dispatch;:111-127proves only a main-ref dispatch, an open same-repo PR at that SHA, and syntactically valid caller-provided jobs/hash/UUID. It does not prove a green canonical CI run, Advisor result, deterministic plan, three-job cap, approved human, or coordinator caller. A write collaborator/app can directly dispatche2e.yamlonmainwith an unmerged first-party PR SHA and arbitrary allowed jobs; downstream jobs checkout that PR code while DockerHub/provider/messaging secrets are available (:339-351,:447-453,:3593-3618,:3802-3835). The same GitHub Actions app can also emit the same check name, so the proposed name-only required status is not a distinct trust principal. Require an independently controlled authorization bound to{pr, headSha, jobs, planHash}and revalidate it in the child before any credential-bearing job—prefer a protected Environment with required maintainer reviewers and a Ruleset Required Workflow/dedicated App identity. Reject bots/apps and non-default-base PRs unless policy explicitly authorizes them. -
A stale workflow completion can cancel the current generation. Coordinator concurrency at
.github/workflows/pr-e2e-risk-gate.yaml:15-17is keyed only by head repo+branch. A late completion/rerun for old SHA A can start after SHA B's coordinator and cancel B before its own exact-head check fails. Nothing retriggers B, leaving the current required check failed or stuck. Key coordinate runs by exact SHA (and keep explicit superseded-child cancellation generation-aware), or otherwise reject stale generations before they can cancel current work. -
Large PRs can plan from a silently truncated file set.
pullChangedFiles()asks for 5001 and rejects only>5000(pr-risk-gate.mts:138-143), but GitHub's PR-files endpoint returns at most 3000 files. The guard is unreachable; a larger PR can omit risk-relevant paths and even reach “No live E2E required.” Fail closed at the API cap or derive and cross-check the complete exact base/head diff.
After fixing these, require an exact-head non-secret coordinator lifecycle smoke (including two out-of-order generations), focused tests, current-main sync, trusted advisors, and the Advisor-required full live matrix. Do not enable this as a required check until the authorization principal and end-to-end protocol are proven.
|
Concrete repository evidence for authorization finding 2: this is not hypothetical. Open Dependabot PR #5755 has The live rulesets also do not make all upstream topic branches a human-trusted namespace: the only non- |
E2E Target Results — ❌ Some jobs failedRun: 29123661854
|
|
Superseded by #6662 |
5 participants
Summary
Adds a trusted PR coordinator that lets normal CI and E2E Advisor run in parallel, then dispatches the bounded union of deterministic and Advisor-required live jobs only after exact-head CI succeeds. It reports one stable
E2E / Required Livecheck and cancels superseded PR-correlated runs on every new commit.Related Issue
Part of #6145. Depends on #6583 for automatic fork Advisor output; first-party coordination is otherwise independent.
Changes
CI / Pull Requestrun by exact head SHA, repository, branch, and canonical PR files API.E2E / Required Liveresult, cancel superseded coordinator/child runs immediately, and close the check on every controller failure path without overwriting evidence-specific failures.Type of Change
Quality Gates
Verification
npm run check:diffnpm run typecheck:clirebuild-hermeswithout logs/cleanup, and the exact-SHA isolated retry passedRollout
E2E / Required Livein advisory mode on representative first-party PRs.mainruleset and enable strict up-to-date required checks.The PR-specific controller remains separate only for PR lifecycle concerns—
workflow_runidentity resolution, fork rejection, and superseded-run cancellation. Planning, dispatch identity, state validation, evidence classification, and check completion reuse the existing post-merge risk-gate primitives.Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit
New Features
Documentation