test(e2e): harden protected runtime cleanup authority - #8918
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
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 (1)
📝 WalkthroughWalkthroughThe change propagates managed-bootstrap state roots through sandbox creation and failure injection. Protected runtime qualification now validates provider container identities, performs authority-aware cleanup, uses name-aware readiness checks, and validates final inventory. ChangesManaged runtime safety
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🟡 Moderate · up to The PR hardens protected-runtime cleanup and diagnostics, but a changed readiness test may assert redaction on raw stderr even though redaction applies only to stored artifacts. Resolve or explicitly reconcile this test contract before merging. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 1112427 in the TypeScript / code-coverage/cliThe overall coverage in commit 1112427 in the Show a code coverage summary of the most impacted files.
Updated |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/live/managed-image-protected-runtime-helpers.ts (1)
636-667: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the vLLM readiness loop to its command timeout.
The vLLM loop runs up to 300 attempts. Each attempt can consume up to 5 seconds of
curl --max-timeplus a 2-secondsleep, so the worst case is about 35 minutes. The caller allows only 11 minutes (Line 615). A stalled provider therefore reaches the harness timeout, and the script never runsdocker logs --tail 200. The redacted container diagnostics are lost for exactly the failure that needs them.
protectedNimReadinessCommandalready solves this with an explicitdeadlinethat is smaller than its 21-minute command timeout. Use the same pattern here. Also add--noprofile --norc, which every other command in this file passes.🛠️ Proposed fix to bound the loop and match the NIM pattern
return { command: "bash", captureLimitBytes: PROTECTED_READINESS_CAPTURE_LIMIT_BYTES, args: [ + "--noprofile", + "--norc", "-c", `set -euo pipefail attempt=0 -for attempt in $(seq 1 300); do +deadline=$((SECONDS + 600)) +while [ "$SECONDS" -lt "$deadline" ]; do + attempt=$((attempt + 1)) if curl -fsS --connect-timeout 2 --max-time 5 http://127.0.0.1:8000/v1/models >/dev/null 2>&1; then printf 'managed-image-vllm-ready attempts=%s\n' "$attempt" exit 0 fi if ! docker container inspect "${containerName}" --format '{{.State.Running}}' | grep -Fx true >/dev/null; then break fi sleep 2 doneNote: the support test at
test/e2e/support/managed-image-protected-runtime-readiness.test.tsstubsseqto print a single attempt. Update that stub after this change so the bounded-probe tests still exercise one attempt.🤖 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/live/managed-image-protected-runtime-helpers.ts` around lines 636 - 667, Update protectedVllmReadinessCommand to follow protectedNimReadinessCommand: add an explicit deadline that bounds the readiness loop below the caller’s command timeout, stop probing when the deadline is reached, and preserve execution of the docker logs diagnostics afterward. Add --noprofile and --norc to the bash arguments, and update the readiness support test’s seq stub so bounded-probe coverage still performs one attempt.Source: Path instructions
🧹 Nitpick comments (2)
test/e2e/live/managed-image-protected-runtime-helpers.ts (2)
99-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated Docker name-contract validation.
The same length check and regular expression appear in
protectedProviderReportedContainerId,protectedProviderContainerPreflightCommand,protectedVllmReadinessCommand, andprotectedNimReadinessCommand. Four copies can drift. Extract oneassertProtectedProviderContainerName(name)helper and call it from each site.Also applies to: 129-134
🤖 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/live/managed-image-protected-runtime-helpers.ts` around lines 99 - 105, Extract the shared Docker name-contract check into an assertProtectedProviderContainerName(name) helper. Replace the duplicated validation in protectedProviderReportedContainerId, protectedProviderContainerPreflightCommand, protectedVllmReadinessCommand, and protectedNimReadinessCommand with calls to this helper, preserving the existing error behavior.
482-533: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winInspect the provider container by its recorded exact ID.
inspectProtectedProviderContainerpassesstate.nametodocker container inspect. The name is mutable Docker state. The recordedstate.reportedContainerIdis the immutable identity that the precedingdocker runreturned.The later comparison at Line 523 does catch a reused name, because the reused container reports a different ID. Inspecting by the recorded ID is still stronger: it removes the name-resolution step from the trust path, and it matches the recorded-ID authority that
protectedProviderContainerCleanupCommandalready uses.♻️ Proposed refactor to inspect by recorded ID
async function inspectProtectedProviderContainer( host: HostCliClient, state: ProtectedProviderContainerState, requestedImage: string, artifactName: string, ): Promise<ProtectedProviderContainerAuthority> { + if (!state.reportedContainerId) { + throw new Error(`provider authority inspection has no recorded ID for ${state.name}`); + } const result = await host.command( "docker", [ "container", "inspect", "--format", '{{.Id}}|{{.Name}}|{{.Config.Image}}|{{.Image}}|{{ index .Config.Labels "io.nvidia.nemoclaw.e2e-owner" }}|{{ index .Config.Labels "io.nvidia.nemoclaw.e2e-provider" }}', - state.name, + state.reportedContainerId, ],🤖 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/live/managed-image-protected-runtime-helpers.ts` around lines 482 - 533, Update inspectProtectedProviderContainer to pass state.reportedContainerId, rather than state.name, as the target argument to docker container inspect. Keep the existing identity and authority comparisons unchanged, including actualName validation against the expected container name.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 `@test/e2e/support/managed-image-protected-runtime-readiness.test.ts`:
- Around line 714-748: Make the curl flag contract observable in the test around
protectedVllmReadinessCommand: have the curl stub record its received arguments
while still returning the intended probe failure, then assert the recorded argv
contains -fsS, --connect-timeout 2, and --max-time 5. Remove the inline flag
conditionals that can independently trigger the same readiness outcome, while
preserving the existing readiness and diagnostic assertions.
In `@test/managed-image-protected-runtime-contract.test.ts`:
- Around line 85-102: Extend the test around failureInjectingAdapter to create
the stateRoot/managed-bootstrap journal directory, invoke
adapter.recoverUnfinishedTransactions(), and assert that it returns an empty
report. Retain the existing adapter shape and stateRoot directory assertions
while exercising the journal path derived from the supplied stateRoot.
---
Outside diff comments:
In `@test/e2e/live/managed-image-protected-runtime-helpers.ts`:
- Around line 636-667: Update protectedVllmReadinessCommand to follow
protectedNimReadinessCommand: add an explicit deadline that bounds the readiness
loop below the caller’s command timeout, stop probing when the deadline is
reached, and preserve execution of the docker logs diagnostics afterward. Add
--noprofile and --norc to the bash arguments, and update the readiness support
test’s seq stub so bounded-probe coverage still performs one attempt.
---
Nitpick comments:
In `@test/e2e/live/managed-image-protected-runtime-helpers.ts`:
- Around line 99-105: Extract the shared Docker name-contract check into an
assertProtectedProviderContainerName(name) helper. Replace the duplicated
validation in protectedProviderReportedContainerId,
protectedProviderContainerPreflightCommand, protectedVllmReadinessCommand, and
protectedNimReadinessCommand with calls to this helper, preserving the existing
error behavior.
- Around line 482-533: Update inspectProtectedProviderContainer to pass
state.reportedContainerId, rather than state.name, as the target argument to
docker container inspect. Keep the existing identity and authority comparisons
unchanged, including actualName validation against the expected container name.
🪄 Autofix
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: 19ef1ea2-0184-4b8e-bd44-9b57fc190097
📒 Files selected for processing (8)
scripts/checks/run-managed-image-openshell-e2e.tssrc/lib/onboard/sandbox-gpu-create-flow.test.tssrc/lib/onboard/sandbox-gpu-create-flow.tssrc/lib/onboard/sandbox-gpu-create-run-attempt.tstest/e2e/README.mdtest/e2e/live/managed-image-protected-runtime-helpers.tstest/e2e/support/managed-image-protected-runtime-readiness.test.tstest/managed-image-protected-runtime-contract.test.ts
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 @.agents/skills/nemoclaw-maintainer-e2e/SKILL.md:
- Line 43: Update the managed-image-protected-runtime guidance to require
revoking the exposed NVIDIA_API_KEY, or rotating it and disabling the old value,
through the issuing NVIDIA service. Instruct maintainers to verify that the
exposed key is no longer valid before considering remediation complete.
🪄 Autofix
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: c686fdee-571a-4d18-8a4d-f4b13095e313
📒 Files selected for processing (1)
.agents/skills/nemoclaw-maintainer-e2e/SKILL.md
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
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/managed-image-protected-runtime-readiness.test.ts`:
- Line 716: Update the fake curl argument logging around the FAKE_CURL_ARGV_LOG
write to preserve positional-argument boundaries by emitting each argument
separately with "$@". Adjust the corresponding assertion to read and validate
the resulting per-argument array, ensuring a single argument containing spaces
remains distinct from multiple arguments.
🪄 Autofix
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: 7ef87482-e43b-40f0-b246-c08c2500f37b
📒 Files selected for processing (5)
.agents/skills/nemoclaw-maintainer-e2e/SKILL.mdtest/e2e/README.mdtest/e2e/live/managed-image-protected-runtime-helpers.tstest/e2e/support/managed-image-protected-runtime-readiness.test.tstest/managed-image-protected-runtime-contract.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/managed-image-protected-runtime-contract.test.ts
- .agents/skills/nemoclaw-maintainer-e2e/SKILL.md
- test/e2e/README.md
- test/e2e/live/managed-image-protected-runtime-helpers.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Exact-head refresh: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Current-main refresh: exact head is now |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Current-main refresh: exact head is now |
|
@coderabbitai review |
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Docs-only current-main refresh: exact head is now |
|
@coderabbitai review |
|
|
Advisor disposition for exact head |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Current-main refresh: exact head is now Exact-head local evidence:
CodeRabbit is successful with zero unresolved threads. The prior protected run was canceled after its base became stale. Fresh protected qualification is run 31662574004, correlation |
|
Closing this PR because it was opened outside the requested B4-D execution boundary. The work will proceed only through existing PR #8061; nothing from this PR was merged. |
1 participant
Summary
Make the trusted protected-runtime harness restore rollback state from the canonical state root and fail closed when provider cleanup authority cannot be proved. This prerequisite lets B4-D's protected qualification produce authoritative rollback and cleanup evidence before the candidate branch advances.
Related Issue
Supports #7744 and #8061.
Changes
Type of Change
Quality Gates
dd44d6c34; no blockers or suggestions remain after exact-ID inspection, bounded and argument-observable vLLM diagnostics, state-root recovery coverage, and explicit provider-key invalidation guidance.Documentation Writer Review
docs-updated.agents/skills/nemoclaw-maintainer-e2e/SKILL.md,test/e2e/README.md; reviewed all nine changed paths for terminology, structure, voice, command construction and exact argument boundaries, credential location and invalidation, provider-native failure evidence, rollback state-root authority, exact-ID cleanup authority, and documentation accuracy. No Fern page update is needed because this prerequisite changes the protected test harness and an injected test seam, not supported user behavior.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable —npm run validate:prpassed on exact headdd44d6c34againstorigin/main7c721ae4d.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) — build completed with zero errors; its two hidden-page warnings pre-exist this change.markdownlint-cli2passed and the exact-tree writer review found no blockers or suggestions.Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests
Documentation