feat(gpu): prefer native OpenShell with compatibility fallback#6333
Merged
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
|
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:
📝 WalkthroughWalkthroughThis PR replaces boolean GPU-patch handling with route plans, adds bounded native-first compatibility fallback, threads route-aware behavior through sandbox creation and onboarding, expands Hermes GPU E2E coverage, and updates GPU passthrough documentation. ChangesGPU Routing and Fallback Refactor
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant onboard.ts
participant runSandboxGpuCreateFlow
participant executeSandboxGpuCreatePlan
participant cleanupNativeGpuAttemptForFallback
participant dockerGpuLocalInference
onboard.ts->>runSandboxGpuCreateFlow: create with gpuRoutePlan
runSandboxGpuCreateFlow->>executeSandboxGpuCreatePlan: run native attempt
executeSandboxGpuCreatePlan->>cleanupNativeGpuAttemptForFallback: verify native cleanup
executeSandboxGpuCreatePlan->>executeSandboxGpuCreatePlan: run one compatibility retry
runSandboxGpuCreateFlow->>dockerGpuLocalInference: verify selected route after Ready
dockerGpuLocalInference-->>onboard.ts: verification result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-6333.docs.buildwithfern.com/nemoclaw |
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
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
|
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the branch. TypeScript / code-coverage/cliThe overall coverage in the branch remains at 79%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings This is an automated review. Required findings need action before merge. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
Contributor
E2E Target Results — ❌ Some jobs failedRun: 28810685415
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/lib/onboard/sandbox-gpu-create-attempt.test.ts (1)
194-207: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the fallback gating cases.
This test claims both route-plan and failure-classification gates, but disables both at once, so it cannot catch a regression in either gate independently.
Suggested test split
- it("does not fallback when the route plan or failure classification forbids it", async () => { + it("does not fallback when the route plan forbids it", async () => { + const routeIneligibleFailure = nativeFailure("create"); + const routeRunAttempt = vi.fn(async () => routeIneligibleFailure); + + await expect( + executeSandboxGpuCreatePlan("native-only", { + runAttempt: routeRunAttempt, + cleanupNativeFailure: async () => SAFE_CLEANUP, + }), + ).resolves.toBe(routeIneligibleFailure); + expect(routeRunAttempt).toHaveBeenCalledTimes(1); + }); + + it("does not fallback when the failure classification forbids it", async () => { const ineligibleFailure = { ...nativeFailure("create"), fallbackEligible: false, }; const runAttempt = vi.fn(async () => ineligibleFailure); const cleanupNativeFailure = vi.fn(async () => SAFE_CLEANUP); await expect( - executeSandboxGpuCreatePlan("native-only", { runAttempt, cleanupNativeFailure }), + executeSandboxGpuCreatePlan("native-with-fallback", { runAttempt, cleanupNativeFailure }), ).resolves.toBe(ineligibleFailure); expect(runAttempt).toHaveBeenCalledTimes(1); expect(cleanupNativeFailure).not.toHaveBeenCalled(); });As per path instructions, tests should prove observable behavior and avoid cases that pass without exercising their claim.
🤖 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/sandbox-gpu-create-attempt.test.ts` around lines 194 - 207, Split the combined fallback-gating assertion in executeSandboxGpuCreatePlan’s test so each gate is verified independently: one case should cover a route plan that forbids fallback while the failure remains eligible, and a separate case should cover an ineligible failure (fallbackEligible false) under a route plan that would otherwise allow fallback. Keep the existing symbols like executeSandboxGpuCreatePlan, runAttempt, and cleanupNativeFailure, but restructure the test names and expectations so each test proves only one gating condition.Source: Path instructions
src/lib/onboard/docker-gpu-patch-finalize.test.ts (1)
117-132: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRename-stage case doesn't prove the rollback stopped early.
For the
"rename"case, the test only checks the final outcome, not thatdockerStartwas skipped after the rename failure. The sibling test indocker-gpu-patch-rollback.test.ts(lines 168-195) does assert the next step (dockerRunDetached) is never called after a failed rename — the same rigor here would make this test prove "fails closed at rename" rather than just "final result happens to be false."As per path instructions for test files: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
✅ Suggested strengthening for the rename case
it.each([ - ["rename", { dockerRename: vi.fn(() => ({ status: null })) }], - ["start", { dockerStart: vi.fn(() => ({ status: null })) }], - ])("fails closed when rollback %s has no exit status", (_stage, override) => { - const outcome = finalizeDockerGpuPatchBackup( + ["rename", { dockerRename: vi.fn(() => ({ status: null })) }, "dockerStart"], + ["start", { dockerStart: vi.fn(() => ({ status: null })) }, undefined], + ] as const)("fails closed when rollback %s has no exit status", (_stage, override, unreachedKey) => { + const deps = { + dockerStop: vi.fn(() => ({ status: 0 })), + dockerRm: vi.fn(() => ({ status: 0 })), + dockerRename: vi.fn(() => ({ status: 0 })), + dockerStart: vi.fn(() => ({ status: 0 })), + ...override, + }; + const outcome = finalizeDockerGpuPatchBackup( { result: deferredCreateResult(), supervisorReady: false }, - { - dockerStop: vi.fn(() => ({ status: 0 })), - dockerRm: vi.fn(() => ({ status: 0 })), - dockerRename: vi.fn(() => ({ status: 0 })), - dockerStart: vi.fn(() => ({ status: 0 })), - ...override, - }, + deps, ); expect(outcome).toEqual({ backupRemoved: false, rolledBack: false }); + if (unreachedKey) expect(deps[unreachedKey]).not.toHaveBeenCalled(); });🤖 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/docker-gpu-patch-finalize.test.ts` around lines 117 - 132, The rename-stage branch in finalizeDockerGpuPatchBackup is too weak because it only asserts the final outcome and does not verify rollback stops after dockerRename fails. Strengthen the "rename" case in docker-gpu-patch-finalize.test.ts by asserting dockerStart is not called when dockerRename returns no exit status, mirroring the sibling rollback test’s pattern so the test proves the rollback halts at the failed step instead of passing via a broad mock.Source: Path instructions
test/e2e/live/hermes-gpu-startup.test.ts (1)
110-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTernary used purely for side effects.
This ternary discards its result and exists only to run
expect(...)in one branch or the other. Many ESLint configs enforceno-unused-expressions, which flags conditional/ternary expressions used as statements; this could fail CI lint even though the runtime behavior is correct.🧹 Suggested fix
- sandboxList.exitCode === 0 - ? expect(outputContainsSandbox(sandboxList, SANDBOX_NAME)).toBe(false) - : expect(resultText(sandboxList)).toMatch(GATEWAY_ALREADY_ABSENT); + if (sandboxList.exitCode === 0) { + expect(outputContainsSandbox(sandboxList, SANDBOX_NAME)).toBe(false); + } else { + expect(resultText(sandboxList)).toMatch(GATEWAY_ALREADY_ABSENT); + }🤖 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/hermes-gpu-startup.test.ts` around lines 110 - 112, The conditional in hermes-gpu-startup.test.ts is being used only to trigger assertions, which can trip no-unused-expressions lint rules. Update the test logic around sandboxList.exitCode so the two expect calls are executed through a normal if/else statement instead of a ternary expression, keeping the same behavior while avoiding a side-effect-only expression..github/workflows/e2e.yaml (1)
1707-1726: 🩺 Stability & Availability | 🔵 TrivialVerify GPU runner pool capacity for the 2-way matrix.
runs-on: linux-amd64-gpu-rtxpro6000-latest-1looks like a specific single-runner label. Withfail-fast: falseandmatrix.scenario: [native, fallback], both legs need concurrently idle runners matching that exact label; GitHub Actions queues (not fails) a job if none is idle, so if only one physical runner carries this label, the two legs will simply serialize rather than run in parallel, which could roughly double the wall-clock time within the 75-minute per-job timeout budget.Please confirm whether more than one runner is registered under this label, or whether serialization is acceptable.
🤖 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/e2e.yaml around lines 1707 - 1726, The hermes-gpu-startup matrix uses a single specific runs-on label with fail-fast disabled, so both matrix legs may serialize if only one runner matches. Verify that multiple runners are registered for linux-amd64-gpu-rtxpro6000-latest-1, or adjust the workflow to use a broader label/pool if parallel execution is required. Check the e2e job definition and matrix.scenario setup together to ensure the 2-way matrix fits the available GPU runner capacity.
🤖 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/e2e.yaml:
- Around line 1707-1726: The hermes-gpu-startup matrix uses a single specific
runs-on label with fail-fast disabled, so both matrix legs may serialize if only
one runner matches. Verify that multiple runners are registered for
linux-amd64-gpu-rtxpro6000-latest-1, or adjust the workflow to use a broader
label/pool if parallel execution is required. Check the e2e job definition and
matrix.scenario setup together to ensure the 2-way matrix fits the available GPU
runner capacity.
In `@src/lib/onboard/docker-gpu-patch-finalize.test.ts`:
- Around line 117-132: The rename-stage branch in finalizeDockerGpuPatchBackup
is too weak because it only asserts the final outcome and does not verify
rollback stops after dockerRename fails. Strengthen the "rename" case in
docker-gpu-patch-finalize.test.ts by asserting dockerStart is not called when
dockerRename returns no exit status, mirroring the sibling rollback test’s
pattern so the test proves the rollback halts at the failed step instead of
passing via a broad mock.
In `@src/lib/onboard/sandbox-gpu-create-attempt.test.ts`:
- Around line 194-207: Split the combined fallback-gating assertion in
executeSandboxGpuCreatePlan’s test so each gate is verified independently: one
case should cover a route plan that forbids fallback while the failure remains
eligible, and a separate case should cover an ineligible failure
(fallbackEligible false) under a route plan that would otherwise allow fallback.
Keep the existing symbols like executeSandboxGpuCreatePlan, runAttempt, and
cleanupNativeFailure, but restructure the test names and expectations so each
test proves only one gating condition.
In `@test/e2e/live/hermes-gpu-startup.test.ts`:
- Around line 110-112: The conditional in hermes-gpu-startup.test.ts is being
used only to trigger assertions, which can trip no-unused-expressions lint
rules. Update the test logic around sandboxList.exitCode so the two expect calls
are executed through a normal if/else statement instead of a ternary expression,
keeping the same behavior while avoiding a side-effect-only expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9e4bbda1-3a2a-461b-9f0e-0c754977f01f
📒 Files selected for processing (36)
.github/workflows/e2e.yamldocs/reference/commands-nemohermes.mdxdocs/reference/commands.mdxdocs/reference/troubleshooting.mdxsrc/lib/actions/sandbox/rebuild-target-runtime.tssrc/lib/onboard.tssrc/lib/onboard/docker-gpu-local-inference.test.tssrc/lib/onboard/docker-gpu-local-inference.tssrc/lib/onboard/docker-gpu-patch-finalize.test.tssrc/lib/onboard/docker-gpu-patch-finalize.tssrc/lib/onboard/docker-gpu-patch-rollback.test.tssrc/lib/onboard/docker-gpu-patch.test.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/docker-gpu-route.tssrc/lib/onboard/docker-gpu-sandbox-create.test.tssrc/lib/onboard/docker-gpu-sandbox-create.tssrc/lib/onboard/docker-gpu-supervisor-reconnect.test.tssrc/lib/onboard/docker-gpu-supervisor-reconnect.tssrc/lib/onboard/sandbox-create-intent-types.tssrc/lib/onboard/sandbox-create-launch.tssrc/lib/onboard/sandbox-create-plan.test.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/onboard/sandbox-dockerfile-patch-flow.test.tssrc/lib/onboard/sandbox-dockerfile-patch-flow.tssrc/lib/onboard/sandbox-gpu-create-attempt.test.tssrc/lib/onboard/sandbox-gpu-create-attempt.tssrc/lib/onboard/sandbox-gpu-create-flow.tssrc/lib/state/gateway.tstest/e2e/live/hermes-gpu-startup-fallback.tstest/e2e/live/hermes-gpu-startup-proof.tstest/e2e/live/hermes-gpu-startup.test.tstest/e2e/support/hermes-gpu-startup-fallback.test.tstest/e2e/support/hermes-workflow-boundary.test.tstest/e2e/support/upload-e2e-artifacts-workflow-boundary.test.tstools/e2e/hermes-gpu-startup-workflow-boundary.mtstools/e2e/upload-e2e-artifacts-workflow-boundary.mts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28812365881
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard/sandbox-create-plan.ts (1)
56-59: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRemove the legacy GPU sandbox create path or bound its coexistence.
src/lib/onboard/docker-gpu-sandbox-create.tsstill exportsresolveDockerGpuSandboxCreatePlan, andsrc/lib/onboard/sandbox-gpu-create.tsstill acceptssuppressGpuFlag; the related tests are still present. If this migration is meant to be complete, delete the old helper/tests in the same PR; otherwise link the retirement issue and state the compatibility window.🤖 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/sandbox-create-plan.ts` around lines 56 - 59, The sandbox create plan still exposes the legacy GPU create path through SandboxCreatePlanDeps while the old resolveDockerGpuSandboxCreatePlan helper and suppressGpuFlag-based sandbox GPU flow remain in use. Either remove the deprecated GPU path end-to-end by deleting the legacy helpers and their tests from the affected onboarding modules, or explicitly bound the coexistence by wiring in a retirement/compatibility plan and documenting the allowed overlap in the relevant sandbox create symbols.Source: Path instructions
🧹 Nitpick comments (1)
src/lib/onboard/sandbox-create-plan.ts (1)
266-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
dockerGpuPatchplaceholder from the intent
materializeSandboxCreatePlanrecomputes the real GPU-patch value fromgpuRoutePlan, so this hardcodedfalseonly makesSandboxCreateIntentlook authoritative when it isn’t. If the shape is needed for tests, document it as an internal placeholder instead.🤖 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/sandbox-create-plan.ts` around lines 266 - 278, The intent in materializeSandboxCreatePlan is carrying an unused dockerGpuPatch placeholder that is not the real source of truth. Remove the hardcoded dockerGpuPatch field from the policy options in SandboxCreateIntent, or if the shape must remain for compatibility, clearly mark it as an internal placeholder and keep the real GPU-patch value derived from gpuRoutePlan. Use the materializeSandboxCreatePlan and SandboxCreateIntent symbols to locate the change.
🤖 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/hermes-gpu-startup-integrity.test.ts`:
- Line 151: The new test title in the test block should follow the local
issue-ref convention for root-level integration tests under test/, so update the
it(...) description in the relevant test function to end with the required
(`#1234`)-style suffix. Keep the title behavior-oriented, and ensure the suffix is
appended to the existing title in the hermes-gpu-startup-integrity test case.
---
Outside diff comments:
In `@src/lib/onboard/sandbox-create-plan.ts`:
- Around line 56-59: The sandbox create plan still exposes the legacy GPU create
path through SandboxCreatePlanDeps while the old
resolveDockerGpuSandboxCreatePlan helper and suppressGpuFlag-based sandbox GPU
flow remain in use. Either remove the deprecated GPU path end-to-end by deleting
the legacy helpers and their tests from the affected onboarding modules, or
explicitly bound the coexistence by wiring in a retirement/compatibility plan
and documenting the allowed overlap in the relevant sandbox create symbols.
---
Nitpick comments:
In `@src/lib/onboard/sandbox-create-plan.ts`:
- Around line 266-278: The intent in materializeSandboxCreatePlan is carrying an
unused dockerGpuPatch placeholder that is not the real source of truth. Remove
the hardcoded dockerGpuPatch field from the policy options in
SandboxCreateIntent, or if the shape must remain for compatibility, clearly mark
it as an internal placeholder and keep the real GPU-patch value derived from
gpuRoutePlan. Use the materializeSandboxCreatePlan and SandboxCreateIntent
symbols to locate the change.
🪄 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: a9c0f369-38f4-4b52-a165-5139dd5436f8
📒 Files selected for processing (28)
docs/reference/troubleshooting.mdxsrc/lib/onboard.tssrc/lib/onboard/docker-gpu-local-inference.test.tssrc/lib/onboard/docker-gpu-local-inference.tssrc/lib/onboard/docker-gpu-patch-finalize.test.tssrc/lib/onboard/docker-gpu-patch-mode-selection.test.tssrc/lib/onboard/docker-gpu-patch-wsl.test.tssrc/lib/onboard/docker-gpu-patch.test.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/docker-gpu-route-patch-adapter.tssrc/lib/onboard/docker-gpu-route.test.tssrc/lib/onboard/docker-gpu-route.tssrc/lib/onboard/docker-gpu-sandbox-create-diagnostics.test.tssrc/lib/onboard/docker-gpu-sandbox-create.test.tssrc/lib/onboard/docker-gpu-sandbox-create.tssrc/lib/onboard/openshell-docker-sandbox-containers.test.tssrc/lib/onboard/openshell-docker-sandbox-containers.tssrc/lib/onboard/sandbox-create-plan.test.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/onboard/sandbox-gpu-create-attempt.test.tssrc/lib/onboard/sandbox-gpu-create-attempt.tssrc/lib/onboard/sandbox-gpu-create-flow.tssrc/lib/onboard/sandbox-gpu-route-policy.tstest/e2e/live/hermes-gpu-startup-fallback.tstest/e2e/live/hermes-gpu-startup-integrity.tstest/e2e/live/hermes-gpu-startup.test.tstest/e2e/support/hermes-gpu-startup-fallback.test.tstest/e2e/support/hermes-gpu-startup-integrity.test.ts
💤 Files with no reviewable changes (2)
- src/lib/onboard/docker-gpu-local-inference.test.ts
- src/lib/onboard/docker-gpu-patch.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/onboard/docker-gpu-patch-wsl.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/lib/onboard/sandbox-gpu-create-attempt.test.ts
- test/e2e/live/hermes-gpu-startup-fallback.ts
- src/lib/onboard/sandbox-gpu-create-flow.ts
- docs/reference/troubleshooting.mdx
- test/e2e/support/hermes-gpu-startup-fallback.test.ts
- src/lib/onboard/sandbox-gpu-create-attempt.ts
- src/lib/onboard/docker-gpu-local-inference.ts
- src/lib/onboard/sandbox-create-plan.test.ts
- test/e2e/live/hermes-gpu-startup.test.ts
- src/lib/onboard/docker-gpu-sandbox-create.test.ts
- src/lib/onboard.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28813686639
|
22 tasks
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28813822004
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard/docker-gpu-patch-mode.ts (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate timeout constant across files.
DOCKER_GPU_PATCH_TIMEOUT_MS = 30_000is duplicated identically indocker-gpu-patch-diagnostics.ts. Consider hoisting to a shared constants module to avoid drift as these files are further modularized.♻️ Proposed fix
-const DOCKER_GPU_PATCH_TIMEOUT_MS = 30_000; +import { DOCKER_GPU_PATCH_TIMEOUT_MS } from "./docker-gpu-patch-constants";🤖 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/docker-gpu-patch-mode.ts` at line 16, The timeout constant is duplicated in docker-gpu-patch-mode and docker-gpu-patch-diagnostics, so move DOCKER_GPU_PATCH_TIMEOUT_MS into a shared constants module and import it from both places. Update the references in the docker GPU patch helpers that use this value so both modules rely on the same source of truth and stay aligned as the code is split further.
🤖 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 `@src/lib/onboard/docker-gpu-patch-mode.ts`:
- Line 16: The timeout constant is duplicated in docker-gpu-patch-mode and
docker-gpu-patch-diagnostics, so move DOCKER_GPU_PATCH_TIMEOUT_MS into a shared
constants module and import it from both places. Update the references in the
docker GPU patch helpers that use this value so both modules rely on the same
source of truth and stay aligned as the code is split further.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 12d874ad-9f0d-449d-abfb-b0b7ef9eab3d
📒 Files selected for processing (20)
docs/reference/troubleshooting.mdxsrc/lib/onboard/docker-gpu-patch-diagnostics.tssrc/lib/onboard/docker-gpu-patch-mode.tssrc/lib/onboard/docker-gpu-patch-recreate-dns.test.tssrc/lib/onboard/docker-gpu-patch-recreate.tssrc/lib/onboard/docker-gpu-patch.test.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/docker-gpu-route.test.tssrc/lib/onboard/docker-gpu-sandbox-create.tssrc/lib/onboard/sandbox-create-intent-types.tssrc/lib/onboard/sandbox-create-plan-materialization.tssrc/lib/onboard/sandbox-create-plan.test.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/onboard/sandbox-gpu-create-attempt.test.tssrc/lib/onboard/sandbox-gpu-create-attempt.tssrc/lib/onboard/sandbox-gpu-create-flow.test.tssrc/lib/onboard/sandbox-gpu-create-flow.tstest/e2e/live/hermes-gpu-startup-fallback.tstest/e2e/support/hermes-gpu-startup-fallback.test.tstest/e2e/support/hermes-gpu-startup-integrity.test.ts
💤 Files with no reviewable changes (2)
- src/lib/onboard/docker-gpu-patch.test.ts
- src/lib/onboard/sandbox-create-intent-types.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- test/e2e/support/hermes-gpu-startup-integrity.test.ts
- src/lib/onboard/sandbox-gpu-create-attempt.test.ts
- docs/reference/troubleshooting.mdx
- src/lib/onboard/docker-gpu-route.test.ts
- test/e2e/live/hermes-gpu-startup-fallback.ts
- src/lib/onboard/sandbox-gpu-create-flow.ts
- src/lib/onboard/sandbox-gpu-create-attempt.ts
- src/lib/onboard/docker-gpu-sandbox-create.ts
This was referenced Jul 6, 2026
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28815042279
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28815516859
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/lib/onboard/docker-gpu-patch-clone.ts (1)
89-97: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueHost-network override silently no-ops without a rewritable
OPENSHELL_ENDPOINT.
buildDockerGpuCloneRunOptionsonly switches tonetworkMode: "host"when the inspected container also has anOPENSHELL_ENDPOINTpointing athost.openshell.internal(line 94-96). IfNEMOCLAW_DOCKER_GPU_PATCH_NETWORK=hostis set but that env var is absent/different, the recreated clone silently keepshost.NetworkMode(line 174 inbuildDockerGpuCloneRunArgs) instead of honoring the operator's override — there's no warning surfaced for this case.Worth confirming this coupling (host networking is gated on endpoint rewritability) is the intended design, since OpenShell sandboxes normally always carry this env var, making this a narrow edge case.
🤖 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/docker-gpu-patch-clone.ts` around lines 89 - 97, The host-network override in buildDockerGpuCloneRunOptions is currently gated on OPENSHELL_ENDPOINT being rewritable, so a requested NEMOCLAW_DOCKER_GPU_PATCH_NETWORK=host can silently fall back to the original network mode. Update buildDockerGpuCloneRunOptions and, if needed, buildDockerGpuCloneRunArgs to either honor the host override unconditionally or explicitly surface a warning/fallback path when OPENSHELL_ENDPOINT is missing or cannot be rewritten, using the existing dockerGpuHostEndpointFromOpenShellEndpoint and networkMode handling.src/lib/onboard/docker-gpu-patch-finalize.ts (1)
26-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isZeroStatusduplicated across multiple modules instead of centralized.This file redefines
isZeroStatuslocally, and the same strict-status tightening (result?.status === 0) was independently applied indocker-gpu-supervisor-reconnect.tsand, per the cohort summary, in the patch/rollback modules too. SinceDOCKER_GPU_PATCH_TIMEOUT_MSwas already centralized intodocker-gpu-patch-constants.tsin this same refactor,isZeroStatusis a natural candidate for the same treatment — duplicated status-gating logic can silently drift between modules on future changes.♻️ Suggested consolidation
+// docker-gpu-patch-constants.ts +export function isZeroStatus(result?: { status?: number | null } | null): boolean { + return result?.status === 0; +}Then import
isZeroStatusfrom./docker-gpu-patch-constantsindocker-gpu-patch-finalize.ts,docker-gpu-supervisor-reconnect.ts, and the patch/rollback modules instead of each defining it locally.🤖 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/docker-gpu-patch-finalize.ts` around lines 26 - 48, The status-check helper is duplicated across the docker GPU patch modules, so consolidate the strict exit-status logic into a single shared `isZeroStatus` helper in `docker-gpu-patch-constants` alongside `DOCKER_GPU_PATCH_TIMEOUT_MS`. Update `docker-gpu-patch-finalize`, `docker-gpu-supervisor-reconnect`, and the patch/rollback modules to import and use that shared helper instead of redefining it locally. Keep the helper’s behavior as the explicit zero-status check used by `isZeroStatus` so all cleanup and rollback paths stay consistent.
🤖 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/docker-gpu-patch-diagnostics.test.ts`:
- Around line 299-324: The test name claims to verify the fallback when
dockerCapture is omitted, but the current collectDockerGpuPatchDiagnostics call
still injects a dockerCapture mock, so it never exercises the
defaultDockerCapture path. Update this test in
docker-gpu-patch-diagnostics.test.ts to omit dockerCapture from the deps object
entirely and assert the observable outcome of the default fallback behavior
through collectDockerGpuPatchDiagnostics rather than checking the injected mock.
Keep the existing sandbox/context setup, but make sure the test truly covers the
missing-dependency case.
---
Nitpick comments:
In `@src/lib/onboard/docker-gpu-patch-clone.ts`:
- Around line 89-97: The host-network override in buildDockerGpuCloneRunOptions
is currently gated on OPENSHELL_ENDPOINT being rewritable, so a requested
NEMOCLAW_DOCKER_GPU_PATCH_NETWORK=host can silently fall back to the original
network mode. Update buildDockerGpuCloneRunOptions and, if needed,
buildDockerGpuCloneRunArgs to either honor the host override unconditionally or
explicitly surface a warning/fallback path when OPENSHELL_ENDPOINT is missing or
cannot be rewritten, using the existing
dockerGpuHostEndpointFromOpenShellEndpoint and networkMode handling.
In `@src/lib/onboard/docker-gpu-patch-finalize.ts`:
- Around line 26-48: The status-check helper is duplicated across the docker GPU
patch modules, so consolidate the strict exit-status logic into a single shared
`isZeroStatus` helper in `docker-gpu-patch-constants` alongside
`DOCKER_GPU_PATCH_TIMEOUT_MS`. Update `docker-gpu-patch-finalize`,
`docker-gpu-supervisor-reconnect`, and the patch/rollback modules to import and
use that shared helper instead of redefining it locally. Keep the helper’s
behavior as the explicit zero-status check used by `isZeroStatus` so all cleanup
and rollback paths stay consistent.
🪄 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: 3af4fadb-6c8f-4e9e-9338-7db1e1a89768
📒 Files selected for processing (29)
docs/reference/troubleshooting.mdxsrc/lib/onboard/docker-gpu-dns-fallback.tssrc/lib/onboard/docker-gpu-jetson-groups.tssrc/lib/onboard/docker-gpu-patch-clone.test.tssrc/lib/onboard/docker-gpu-patch-clone.tssrc/lib/onboard/docker-gpu-patch-constants.tssrc/lib/onboard/docker-gpu-patch-diagnostics.test.tssrc/lib/onboard/docker-gpu-patch-diagnostics.tssrc/lib/onboard/docker-gpu-patch-finalize.tssrc/lib/onboard/docker-gpu-patch-jetson.test.tssrc/lib/onboard/docker-gpu-patch-mode-selection.test.tssrc/lib/onboard/docker-gpu-patch-mode.tssrc/lib/onboard/docker-gpu-patch-recreate-dns.test.tssrc/lib/onboard/docker-gpu-patch-recreate.test.tssrc/lib/onboard/docker-gpu-patch-recreate.tssrc/lib/onboard/docker-gpu-patch-rollback.tssrc/lib/onboard/docker-gpu-patch.test.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/docker-gpu-pre-rollback-diagnostics.tssrc/lib/onboard/docker-gpu-route.test.tssrc/lib/onboard/docker-gpu-supervisor-reconnect.test.tssrc/lib/onboard/docker-gpu-supervisor-reconnect.tssrc/lib/onboard/sandbox-create-intent.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/onboard/sandbox-gpu-cleanup-verification.test.tssrc/lib/onboard/sandbox-gpu-create-attempt.tssrc/lib/onboard/sandbox-gpu-create-failure-classification.test.tssrc/lib/onboard/sandbox-gpu-create-flow.test.tssrc/lib/onboard/sandbox-gpu-fallback-orchestration.test.ts
💤 Files with no reviewable changes (2)
- src/lib/onboard/sandbox-gpu-fallback-orchestration.test.ts
- src/lib/onboard/docker-gpu-patch.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/onboard/sandbox-gpu-create-failure-classification.test.ts
- src/lib/onboard/docker-gpu-jetson-groups.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/onboard/docker-gpu-patch-recreate-dns.test.ts
- src/lib/onboard/docker-gpu-route.test.ts
- docs/reference/troubleshooting.mdx
- src/lib/onboard/docker-gpu-patch-mode.ts
- src/lib/onboard/docker-gpu-patch-diagnostics.ts
- src/lib/onboard/sandbox-gpu-create-attempt.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results — ❌ Some jobs failedRun: 28816169079
|
Contributor
E2E Target Results — ❌ Some jobs failedRun: 28816418199
|
Collaborator
|
Release sweep: deferring this PR from The active security change request is still present: the GPU fallback workflow stores Docker daemon state in a predictable permissive directory and restores it with mode Before re-targeting, please use private temporary state, preserve the original metadata on restore, guarantee cleanup/recovery, update the boundary tests, sync with |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com> # Conflicts: # src/lib/onboard.ts # src/lib/onboard/docker-gpu-patch-finalize.test.ts # src/lib/onboard/docker-gpu-patch-finalize.ts # src/lib/onboard/docker-gpu-patch.test.ts # src/lib/onboard/docker-gpu-patch.ts # src/lib/onboard/sandbox-create-launch.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
21 tasks
cv
added a commit
that referenced
this pull request
Jul 12, 2026
## Summary Repairs three deterministic failures found during v0.0.81 release validation. Hermes restart-safe recreation now preserves OpenShell's native CDI GPU attachment, the Slack proof finds the reviewed package in canonical managed npm projects, and the OpenShell auth contract collects under Vitest's fixture parser. ## Changes - Preserve inspected `Driver: cdi` device IDs as Docker `--device` arguments during startup-command recreation, without selecting compatibility GPU mode or adding broader privileges. - Discover `@openclaw/slack` one level below OpenClaw's managed npm projects only when the project manifest declares that dependency. - Destructure the live auth-contract fixtures so Vitest 4.1.9 can collect the test while passing the same scenario inputs. - Add regression coverage for the CDI clone arguments and managed Slack project discovery. - Note for #6333: that PR moves the Docker clone code into split files but does not yet preserve CDI device requests; whichever PR rebases second must retain this contract. The two `rebuild-hermes-stale-base` attempts ended with GitHub's hosted runner losing communication before Vitest exited or uploaded artifacts; the unchanged ordinary rebuild also lost a runner once and passed on retry. Jetson had no runner. Neither result has an evidence-backed repository change in this PR. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: this restores existing GPU passthrough and release-proof contracts without changing commands, flags, configuration, defaults, or required user actions. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent diff review confirmed CDI propagation is startup-command-only, adds no broader GPU privileges, and Slack discovery is bounded to one manifest-gated directory level. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — 67 Docker clone tests, 24 messaging-proof tests, live auth-contract collection to its intended availability skip, and 653 changed-scope tests passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Preserved native CDI GPU device attachments when recreating Docker containers, preventing GPU access from being lost during startup-command persistence. - Improved Slack integration discovery for installations managed through project-specific npm environments. - **Tests** - Added coverage for CDI GPU preservation and Slack package discovery across valid, unrelated, and malformed project directories. - Updated gateway authentication scenarios for more reliable end-to-end validation. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com> # Conflicts: # src/lib/onboard/docker-gpu-patch.ts # test/e2e/support/dockerhub-auth-workflow-boundary.test.ts # test/e2e/support/upload-e2e-artifacts-workflow-boundary.test.ts # tools/e2e/upload-e2e-artifacts-workflow-boundary.mts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
cv
approved these changes
Jul 12, 2026
21 tasks
cv
added a commit
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Updates the Hermes GPU live proof to assert the canonical `Docker container mode selected:` message introduced by #6333. The stale `Docker GPU mode selected:` expectation failed the exact-candidate fallback E2E after container recreation, Ready state, and CUDA verification had already succeeded. ## Changes - Require the canonical container-mode selection message for fallback and compatibility-only routes. - Verify that the native route does not emit the compatibility container-mode message. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes only an internal live-test assertion; user-facing GPU routing, configuration, commands, and documented behavior are unchanged. - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project e2e-support test/e2e/support/hermes-gpu-startup-fallback.test.ts test/e2e/support/hermes-gpu-startup-integrity.test.ts test/e2e/support/hermes-workflow-boundary.test.ts` (38 passed); live-test collection passed for native, fallback, and compatibility-only scenarios - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated end-to-end validation to reflect revised Docker startup messaging. * Added coverage for container-mode logging in compatibility and fallback scenarios. * Confirmed container-mode messaging is absent for other GPU routes. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Carlos Villela <cvillela@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This non-breaking follow-up to PR 6142 preserves automatic GPU onboarding while preferring native OpenShell GPU injection on eligible Linux Docker hosts and retrying exactly once through the compatibility path only after trusted failure classification, complete retry preparation, and proven-safe cleanup. It preserves the existing CLI, environment controls, registry behavior, and OpenShell supervisor/workload boundary.
Related Issue
Related to #6110. This PR intentionally does not close the issue; reporter-class DGX Spark aarch64 or DGX Station GB300 validation remains required.
Follow-up to PR 6142 and its final reviewer feedback.
Changes
none,native-only,compatibility-only, andnative-with-fallbackGPU route plans while preserving existing environment controls.NEMOCLAW_DOCKER_GPU_PATCH=0as native-only,=1as compatibility-only, and legacy nonzero compatibility routing.OPENSHELL_SANDBOX_COMMAND.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed3e82fd26e1dfab5e7fc9a12081a69f40af5f4ac0:npm run build:cli,npm run typecheck,npm run source-shape:check,git diff --check origin/main...HEAD, and 405 focused GPU/onboarding/rebuild/E2E-support tests passed3e82fd26e1dfab5e7fc9a12081a69f40af5f4ac0:npm run docs:strictreported 0 errors and 2 existing Fern warningsnvidia → runc → nvidiarunner restorationnpm run docsbuilds without warnings (doc changes only) — Fern reports two advisory warningsSigned-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
--gpurejection wrapper.NEMOCLAW_DOCKER_GPU_PATCHsemantics by platform.