test(e2e): verify disabled Jetson sandbox GPU access - #8875
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
|
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:
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds Jetson device detection, GPU policy grants, and a root-owned device-group bootstrap helper. Sandbox creation and recreation propagate group-preservation settings. Tests cover helper validation, image packaging, policy behavior, workflow changes, and CPU-only Jetson fallback. ChangesJetson GPU support
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit a2624a3 in the TypeScript / code-coverage/cliThe overall coverage in commit a2624a3 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 4 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: None 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: San Dang <sdang@nvidia.com>
sandl99
changed the title
|
🌿 Preview your docs: https://nvidia-preview-pr-8875.docs.buildwithfern.com/nemoclaw |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/lib/onboard/initial-policy.test.ts (1)
362-383: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative case that proves no Jetson grant appears without device paths.
The suite covers the grant path. It does not prove the fail-closed default. Without a negative case, a future change that pushes
/opt/nvidiaunconditionally would still pass.Add an assertion that
buildDirectGpuPolicyYamlomits/opt/nvidiawhenjetsonGpuDevicePathsis empty or absent.💚 Proposed negative-case coverage
+ it("omits Jetson grants when no device paths are detected (`#7610`)", () => { + const gpuDoc = YAML.parse(buildDirectGpuPolicyYaml(BASE_POLICY_FIXTURE, {})); + + expect(gpuDoc.filesystem_policy.read_only).not.toContain("/opt/nvidia"); + expect(gpuDoc.filesystem_policy.read_write).not.toContain("/opt/nvidia"); + });🤖 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 `@src/lib/onboard/initial-policy.test.ts` around lines 362 - 383, Add a negative test for buildDirectGpuPolicyYaml with jetsonGpuDevicePaths empty or omitted, and assert the generated filesystem_policy.read_only does not contain /opt/nvidia. Keep the existing positive Jetson-device grant test unchanged.
🤖 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 `@src/lib/onboard/initial-policy.test.ts`:
- Around line 403-409: Strengthen the Hermes filesystem exclusion assertion by
checking each entry in devicePaths individually with a per-path negative
containment assertion, ensuring no device path is leaked. Move the
defaultOpenclaw and hermes cleanup calls into an unconditional teardown hook so
temporary policy files are removed even when earlier assertions fail.
In `@src/lib/onboard/sandbox-gpu-create-run-attempt.ts`:
- Line 150: Thread input.preserveJetsonDeviceGroupMembership through the managed
replacement boundary into createLifecycle, ensuring native Jetson recreation
receives the preservation intent and includes the required Tegra groups in
startup-command. Preserve existing compatibility-route behavior and add a native
Jetson test covering the propagated option and resulting groups.
In `@test/e2e/live/jetson-nvmap-gpu.test.ts`:
- Around line 179-181: Update the host prerequisite assertion in the test around
hostNvmap to run `test -c /dev/nvmap` instead of relying on `ls -l`, while
preserving the existing exit-code and diagnostic assertion so the check only
passes for a character device.
In `@test/jetson-device-group-bootstrap.test.ts`:
- Around line 276-286: Update the WSL Vitest lane configuration to provision
Docker and pull the exact FIXTURE_BASE_IMAGE digest before running the suite,
ensuring the Docker build in the test around dockerSpawnSync succeeds;
alternatively, explicitly exclude test/jetson-device-group-bootstrap.test.ts
from that lane.
---
Nitpick comments:
In `@src/lib/onboard/initial-policy.test.ts`:
- Around line 362-383: Add a negative test for buildDirectGpuPolicyYaml with
jetsonGpuDevicePaths empty or omitted, and assert the generated
filesystem_policy.read_only does not contain /opt/nvidia. Keep the existing
positive Jetson-device grant test unchanged.
🪄 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: d35f1127-e268-4f26-b75c-9b199b7b1e64
📒 Files selected for processing (31)
Dockerfiledocs/reference/troubleshooting.mdxscripts/jetson-device-group-bootstrap.shsrc/lib/onboard/docker-gpu-jetson-groups.test.tssrc/lib/onboard/docker-gpu-jetson-groups.tssrc/lib/onboard/docker-gpu-patch-clone.tssrc/lib/onboard/docker-gpu-patch-jetson.test.tssrc/lib/onboard/docker-gpu-patch-recreate.tssrc/lib/onboard/docker-gpu-patch-types.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/docker-gpu-sandbox-create.tssrc/lib/onboard/docker-startup-command-agent.test.tssrc/lib/onboard/docker-startup-command-agent.tssrc/lib/onboard/docker-startup-command-patch.test.tssrc/lib/onboard/docker-startup-command-patch.tssrc/lib/onboard/docker-startup-command-sandbox-create.tssrc/lib/onboard/initial-policy.test.tssrc/lib/onboard/initial-policy.tssrc/lib/onboard/sandbox-create-step.tssrc/lib/onboard/sandbox-gpu-create-flow.test.tssrc/lib/onboard/sandbox-gpu-create-flow.tssrc/lib/onboard/sandbox-gpu-create-run-attempt.tssrc/lib/sandbox/build-context.tstest/e2e/docs/jetson-colossus-dispatch.mdtest/e2e/live/jetson-nvmap-gpu.test.tstest/helpers/vitest-watch-triggers.tstest/jetson-device-group-bootstrap.test.tstest/openclaw-final-image-layout.test.tstest/sandbox-build-context.test.tstest/sandbox-provisioning-helper-permissions.test.tstest/vitest-watch-triggers.test.ts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
src/lib/onboard/sandbox-gpu-create-run-attempt.ts:150 forwards preserveJetsonDeviceGroupMembership only to the non-managed patch factory. When input.managedBootstrap exists, runtimePatch instead uses managedLifecycle.patch, but the lifecycle input and src/lib/onboard/managed-bootstrap/docker-runtime.ts do not receive or apply this option. Managed OpenClaw Jetson recreation can therefore omit the bootstrap entrypoint and detected device groups, reproducing the access failure this PR intends to fix. Thread the preservation intent through the managed lifecycle contract and managed Docker replacement, then add a regression test that creates the managed lifecycle for native Jetson OpenClaw and asserts the bootstrap entrypoint and detected groups. The test at src/lib/onboard/sandbox-gpu-create-flow.test.ts:565 mocks the non-managed factory, so it passes without exercising this route; the CPU-only Jetson E2E also cannot detect the regression.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
wscurran
added
area: onboarding
Remove the Jetson GPU implementation from this PR while #7610 remains open. Keep only the CPU-only live target and its documented acceptance boundary. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
changed the title
|
@cv, your requested managed Jetson recreation change is now addressed by scope: #8875 no longer contains the GPU implementation or any managed-recreation changes. Its net diff is only the CPU-only Jetson live target and runbook. The GPU work is being moved to a separate draft PR blocked on #7610, where your managed native-recreation finding remains a requirement. Exact head Would you please take a fresh independent review of this CPU-only scope? |
|
@cjagwani your agent's use of the expression "exact-head" indicates it hasn't read or decided not to follow writing, comms and docs guidelines, which further indicates it's not following other instructions and guidelines as well. Please have it re-evaluate this entire PR and consider rewriting. |
Replace workflow shorthand with repository terms. Apply the terms to the Jetson test and its controller documentation. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Describe the remote workspace used by the Jetson controller. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
changed the title
|
@cv, I re-evaluated the complete PR against I made these corrections:
All applicable GitHub checks pass for the latest PR commit, The GPU implementation remains outside this PR. Issue #7610 remains unresolved. Would you review the CPU-only scope again? |
4 participants
Summary
Issue #7610 prevents CUDA verification for a non-root user through OpenShell on the tested Jetson configuration. This PR sets
NEMOCLAW_SANDBOX_GPU=0for the existing Jetson live E2E test. The test verifies CPU-only onboarding and confirms that the sandbox excludes/dev/nvmap.This PR does not add Jetson GPU support. The Jetson GPU implementation remains separate until issue #7610 is resolved and maintainers accept its product scope.
Related Issue
Related to #7610. This PR does not resolve or close that issue.
Changes
jetson-nvmap-gpulive E2E test withNEMOCLAW_SANDBOX_GPU=0./dev/nvmap, including symbolic links.install.shdoes not accept--no-gpu. The test usesNEMOCLAW_SANDBOX_GPU=0, which selects the same CPU-only onboarding behavior.Type of Change
Quality Gates
/dev/nvmap.Documentation Writer Review
docs-updatedtest/e2e/docs/jetson-dispatch.mdandtest/e2e/live/jetson-nvmap-gpu.test.ts; no blocking findings or further edits required.Jetson E2E Evidence
a2624a3547d60c8817b6134ce166edaacf7b13ddon an NVIDIA Jetson AGX Thor Developer Kit. The device reported Jetson Linux R39 revision 2.1 and kernel6.8.12-1021-tegra.absent:/dev/nvmap. Cleanup removed the NemoClaw sandbox, OpenShell sandbox, and OpenShell gateway without failures.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every published commit appears asVerifiedin GitHuba2624a3547d60c8817b6134ce166edaacf7b13dd.npm run test:e2e-phases:checkpassed for 125 tests across 81 files; the live target skipped on a non-Jetson host and released its registered resources; the docs build completed with 0 errors and 2 pre-existing warnings.a2624a3547d60c8817b6134ce166edaacf7b13dd, including 12 CLI test shards, CodeQL, CodeRabbit, and both PR review advisors.npm run docsbuilds without warnings (doc changes only) — the docs build completed with 0 errors and 2 pre-existing warnings.Signed-off-by: San Dang sdang@nvidia.com
Signed-off-by: Charan Jagwani cjagwani@nvidia.com