ci(e2e): select post-reboot recovery for status - #7828
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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:
📝 WalkthroughWalkthroughThe PR adds a post-reboot recovery typed E2E target, maps matching status runtime changes to it, extends trusted workflow matrix generation and dispatch allowlisting, and strengthens workflow-boundary validation with mutation-based tests. ChangesPost-reboot E2E target routing
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit b70b864 in the TypeScript / code-coverage/cliThe overall coverage in commit b70b864 in the Show a code coverage summary of the most impacted files.
Updated |
Security review — exact head
|
wscurran
added
area: ci
![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/e2e/README.md`:
- Around line 580-583: Update the descriptive text associated with the
ubuntu-repo-docker-post-reboot-recovery target to use “host forwarding” instead
of “host forward,” preserving the documented E2E terminology.
In `@tools/e2e/workflow-boundary.mts`:
- Around line 4399-4406: Update the validation around controllerMatrixScript and
trustedTargetMappings to inspect each target’s corresponding case branch rather
than counting raw JSON occurrences. For each trusted target, assert that its
branch emits the exact expected matrix entry and runner, so comments or dead
code cannot satisfy validation; preserve validation for both listed mappings.
🪄 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: 874cab6f-1629-4b78-a4fc-ecd65bf89d82
📒 Files selected for processing (7)
.github/workflows/e2e.yamltest/e2e/README.mdtest/pr-e2e-gate-typed-target.test.tstest/pr-risk-plan.test.tstools/advisors/risk-plan.mtstools/e2e/operations-workflow-boundary.mtstools/e2e/workflow-boundary.mts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported 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 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Security review — exact head
|
Security review — exact head
|
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/e2e/support/e2e-workflow.test.ts`:
- Around line 415-436: Update the test around validateE2eWorkflow to first
assert the unmodified workflow does not contain the trusted-controller runner
error and that trustedMapping is present in controllerMatrix.run before
replacing it. Then retain the mutation and final rejection assertion so the test
proves the altered runner value causes the validation failure.
In `@tools/e2e/workflow-boundary.mts`:
- Around line 607-624: Update shellCaseBranchCommands to validate a unique
controller case structure rather than stopping at the first matching case
"${TARGETS}" block. Detect and reject duplicate or alternate case/esac blocks,
including later executed blocks, so approved mappings cannot hide an unsafe
routing branch; apply the same validation to the related logic around the
alternate location.
🪄 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: 525b1580-0b58-4e27-bfa6-e331f0477487
📒 Files selected for processing (3)
test/e2e/README.mdtest/e2e/support/e2e-workflow.test.tstools/e2e/workflow-boundary.mts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Security review — exact head
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/support/e2e-workflow.test.ts (1)
443-468:⚠️ Potential issue | 🟡 MinorProve that this mutation causes the rejection.
The test should first assert that the unmodified workflow does not contain the expected error and that
runcontainstrustedMapping. Otherwise, a pre-existing validator failure or no-op.replace()can make the assertion pass without testing unsafe routing.Suggested adjustment
const run = controllerMatrix.run!; + const expectedError = + "trusted controller matrix must pin typed target runner to ubuntu-latest"; + expect(validateE2eWorkflow(workflow)).not.toContain(expectedError); + requireFixture( + run.includes(trustedMapping), + "trusted target fixture mapping is missing", + ); + const caseStart = run.indexOf('case "${TARGETS}" in');Reuse
expectedErrorin the final assertion.As per path instructions, tests should establish behavioral confidence through the public validator rather than pass on a pre-existing failure or a no-op mutation.
🤖 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/e2e/support/e2e-workflow.test.ts` around lines 443 - 468, Strengthen the test around controllerMatrix.run by asserting the unmodified workflow passes the public validator without expectedError and that run contains trustedMapping before applying the mutation. Ensure the replacement actually changes the trusted runner to self-hosted, then reuse expectedError in the final validator assertion to prove the mutated workflow is rejected.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.
Duplicate comments:
In `@test/e2e/support/e2e-workflow.test.ts`:
- Around line 443-468: Strengthen the test around controllerMatrix.run by
asserting the unmodified workflow passes the public validator without
expectedError and that run contains trustedMapping before applying the mutation.
Ensure the replacement actually changes the trusted runner to self-hosted, then
reuse expectedError in the final validator assertion to prove the mutated
workflow is rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8dea1736-c2a2-4587-9483-026f35a832d8
📒 Files selected for processing (2)
test/e2e/support/e2e-workflow.test.tstools/e2e/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/e2e/workflow-boundary.mts
Security review — exact head
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tools/e2e/workflow-boundary.mts`:
- Around line 4440-4441: Extend the trusted controller matrix validation around
controllerMatrixScript so it rejects any assignment or mutation of matrix after
the trusted case block and before the existing printf output. Ensure validation
reflects the final emitted matrix, and add a regression test covering a
post-esac matrix override to a self-hosted runner.
🪄 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: 936b604b-69ab-4088-af0c-00ec455f5fd1
📒 Files selected for processing (2)
test/e2e/support/trusted-target-routing-workflow-boundary.test.tstools/e2e/workflow-boundary.mts
Security review — exact head
|
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Security review — exact head
|
Security review — exact head
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/e2e/support/trusted-target-routing-workflow-boundary.test.ts`:
- Around line 79-90: In the test “rejects a matrix override after approved
target routing,” assert that validateE2eWorkflow(workflow) does not contain
EXPECTED_ERROR immediately after fixture creation and before mutating
controllerMatrix.run. Keep the existing post-mutation rejection assertion so the
test verifies the unsafe override causes the failure.
🪄 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: f845ce19-c748-4d45-a923-fcf7cb802ed6
📒 Files selected for processing (2)
test/e2e/support/trusted-target-routing-workflow-boundary.test.tstools/e2e/workflow-boundary.mts
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Security review — exact head
|
|
Reviewed exact head No actionable findings. The change is in product scope as validation infrastructure for #7824: it enables the existing post-reboot recovery target on an exact PR head without changing production recovery behavior or creating a new supported surface. Security review passed all nine categories. Focused validation passed:
GitHub currently reports the PR as mergeable with all required checks passing; its blocked status is Approved from my code, security, and product-scope review. |
senthilr-nv
left a comment
senthilr-nv
left a comment
There was a problem hiding this comment.
Approved at exact head 0ba00271cf9cb260b9a74c5c939afde9ffd64c4a. No actionable code, security, or product-scope findings.
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Sensitive-path security reviewReviewed exact head Result: PASS — no findings.
Reviewed files:
Verification: 95 focused E2E-support tests, 25 workflow-plan tests, and 71 integration tests passed on the exact head (191 total). Normal pre-push CLI type-check also passed. |
3 participants
Summary
Status recovery changes cannot currently run the registered post-reboot recovery target on an exact PR SHA because the trusted controller does not allow that typed target. This change adds the existing target to the trusted projection and selects it for the status snapshot runtime that owns post-reboot delivery recovery.
Related Issue
Related to #7824
Changes
ubuntu-repo-docker-post-reboot-recoveryto the trusted PR E2E typed-target allowlist.src/lib/actions/sandbox/status-snapshot.tschanges.ubuntu-latest, including the combined-target plan, and retain exact equality with the candidate-generated matrix.test/e2e/README.md.The current consumer is the #7824 status recovery fix. A same-PR self-enable is insufficient because PR controller policy and the dispatch workflow are loaded from trusted
main; this prerequisite must land before the fixing PR can produce exact-head evidence.Type of Change
Quality Gates
b70b8648on currentmainwith no findings. The trusted allowlist, complete controller-script hosted-runner projection through the sole final output, exact head/base/plan binding, pre-checkout controller validation, and candidate-matrix equality remain fail closed.Documentation Writer Review
docs-updatedtest/e2e/README.mdaccurately documents the status-snapshot typed-target selection and host-forwarding recovery coverage. The reviewer verified that the eight-file diff's terminology, structure, voice, and test titles follow the writing guide; no product documentation is needed because this changes internal PR E2E routing.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 unavailableb70b8648, three focused E2E-support files passed 95 tests, the workflow-plan support file passed 25 tests, and two integration files passed 71 tests, for 191 focused tests total. Normal commit hooks and the normal pre-push CLI type-check passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Pull-request CI will provide the applicable broad gate.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com