fix(ci): bind canonical E2E check authorization#7529
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
added
area: ci
📝 WalkthroughWalkthroughThe PR updates E2E check summaries to link the selected child run and broadens controller authentication to accept either the child-run URL or canonical controller check URL, with boundary and gate tests covering both forms. ChangesPR E2E controller authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
|
Exact-head standard CI, image builds, and self-hosted tests are green at |
|
Documented internal maintainer fallback was dispatched for the exact head/base pair (controller run 30170038805). It selected |
apurvvkumaria
added
the
area: security
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Synchronized current The conflict resolution preserves both authorization contracts:
Validation on the merged tree:
This push does not request or re-request a reviewer. Fresh exact-head CI and an independent approval remain required before merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/pr-e2e-gate.test.ts`:
- Around line 1260-1264: Update the summary assertion in the running update
expectation to verify the complete selected-run Markdown hyperlink, including
the exact URL for run 23, rather than only matching its label. Keep the existing
details_url assertion and ensure the assertion rejects summaries linking to a
different child run.
- Line 1424: Update the assertion for checkUpdates[1] to inspect only
body.output?.summary, rather than stringifying the entire body. Preserve the
existing expectation that the summary contains "upgrade-stale-sandbox".
🪄 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: fefae5a7-a0d1-43e1-865c-d8465552a899
📒 Files selected for processing (4)
.github/workflows/e2e.yamltest/e2e/support/e2e-operations-workflow-boundary.test.tstest/pr-e2e-gate.test.tstools/e2e/operations-workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/e2e.yaml
- tools/e2e/operations-workflow-boundary.mts
| expect(runningUpdate?.body).toMatchObject({ | ||
| details_url: "https://github.com/NVIDIA/NemoClaw/actions/runs/23", | ||
| output: { | ||
| summary: expect.stringContaining( | ||
| "Child run: https://github.com/NVIDIA/NemoClaw/actions/runs/23.", | ||
| ), | ||
| summary: expect.stringContaining("[Selected E2E run 23]"), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete selected-run hyperlink.
The test only checks [Selected E2E run 23], so it would pass if the summary linked to the wrong child run. Assert the full [Selected E2E run 23](https://github.com/NVIDIA/NemoClaw/actions/runs/23) value, or the exact summary contract.
🤖 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-gate.test.ts` around lines 1260 - 1264, Update the summary
assertion in the running update expectation to verify the complete selected-run
Markdown hyperlink, including the exact URL for run 23, rather than only
matching its label. Keep the existing details_url assertion and ensure the
assertion rejects summaries linking to a different child run.
Source: Path instructions
| ), | ||
| }, | ||
| }); | ||
| expect(JSON.stringify(checkUpdates[1]?.body)).toContain("upgrade-stale-sandbox"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the assertion scoped to output.summary.
Matching JSON.stringify(checkUpdates[1]?.body) allows upgrade-stale-sandbox to appear in any body field and does not prove that the summary reports the selected job. Assert directly on checkUpdates[1]?.body.output?.summary.
🤖 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-gate.test.ts` at line 1424, Update the assertion for
checkUpdates[1] to inspect only body.output?.summary, rather than stringifying
the entire body. Preserve the existing expectation that the summary contains
"upgrade-stale-sandbox".
Source: Path instructions
|
Superseded by merged PR #7515. Current main already binds the exact child Actions run in the trusted check summary, polls uncached state for the reviewed 45x2-second window, and deliberately does not authorize via details_url because GitHub canonicalizes that field. This PR's remaining production delta reintroduces an unstable details_url predicate and conflicts with the merged operator contract, so it should not be merged. The useful wrong-summary regression idea can be retained separately if coverage later proves incomplete. |
1 participant
Summary
Trusted PR E2E dispatches currently fail before checkout when GitHub preserves an Actions-owned coordination check's canonical URL instead of the requested child-run URL. This change binds the exact child run in the trusted check summary and accepts either GitHub URL representation only when every existing identity and plan check also matches.
Changes
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededWRITING.mdanddocs/CONTRIBUTING.md, including terminology, structure, voice, test titles, and code-sample presentation. No user-facing documentation contract changes.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run test/e2e/support/e2e-operations-workflow-boundary.test.ts test/pr-e2e-gate.test.tspassed 67/67; file-scoped hooks, repository checks, E2E semantic plans, source-shape budget, and test-size budget passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes