fix: address issue #5489 - #5500
Conversation
Preflight [1/8] skipped the install_nvidia_container_toolkit / CDI remediation when NVIDIA GPU hardware was present but the driver was not loaded. The CDI assessment gate requires assessment.hasNvidiaGpu, yet detectNvidiaGpu() derived that flag solely from nvidia-smi. With the driver unloaded nvidia-smi is unavailable, so hasNvidiaGpu was false, cdiNvidiaGpuSpecMissing stayed false, and onboard advanced past [1/8] without emitting the blocking toolkit/CDI remediation block. Fall back to an lspci PCI-bus hardware probe when nvidia-smi is absent/empty so a physically present NVIDIA GPU is still detected and the missing-spec + install_nvidia_container_toolkit remediation fires and blocks onboarding. Closes #5489 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The #5489 fix commit accidentally added auto_fix_result.json — an internal automation result file — to the repo root. It is not part of the codebase and should not ship. Remove it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The nvidia-smi-unavailable fallback matched any lspci line containing "nvidia", so hosts with NVIDIA/Mellanox NICs or other non-GPU NVIDIA PCI devices were marked hasNvidiaGpu and forced through blocking CDI/toolkit remediation. Match only display-class devices (VGA compatible controller, 3D controller, Display controller) that are also NVIDIA. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex review (automated, advisory)
Verdict: Needs changes; the fallback should restrict detection to GPU/display PCI classes. Addressed in this PRRestricted the lspci hardware fallback in detectNvidiaGpuHardware to display-class PCI devices (VGA compatible controller / 3D controller / Display controller) that are also NVIDIA, instead of matching any line containing 'nvidia'. This prevents NVIDIA/Mellanox NICs and other non-GPU NVIDIA PCI devices from being marked hasNvidiaGpu and forced through blocking CDI/toolkit remediation. Added a preflight-cdi test asserting a host with only NVIDIA/Mellanox NICs is not flagged as having a GPU; the existing #5489 GPU repro still passes. |
|
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:
📝 WalkthroughWalkthroughAdds ChangesNVIDIA GPU lspci fallback and CDI enforcement fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
PR Review Advisor — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
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 `@auto_fix_result.json`:
- Around line 1-3: The auto_fix_result.json file is an internal automation
artifact that should not be included in the codebase. Remove this file entirely
from the commit to ensure it is not shipped with the final deliverable.
🪄 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: c98b26d2-fecb-49b6-ba0f-5db37683e717
📒 Files selected for processing (3)
auto_fix_result.jsonsrc/lib/onboard/preflight-cdi.test.tssrc/lib/onboard/preflight.ts
The auto-fix pipeline's codex-revise pass re-created auto_fix_result.json at the repo root after the earlier drop commit, so the internal automation result file shipped in this PR's net diff. Remove it; it is not part of the codebase. Refs #5489 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Jason Ma <jama@nvidia.com>
…aded The lspci detection in this PR correctly sets assessHost().hasNvidiaGpu and makes planHostRemediation return the blocking install_nvidia_container_toolkit action — but onboard's [1/8] gate (assertCdiNvidiaGpuSpecPresent) skipped enforcement whenever sandbox GPU passthrough was disabled, INCLUDING the auto-disable that happens when nvidia-smi is unavailable (the #5489 scenario). So the remediation was computed but never enforced and onboard advanced past [1/8]. Pass only the EXPLICIT GPU opt-out (--no-gpu) to the gate, not the auto-disable, via a new pure shouldEnforceCdiNvidiaGpuSpec() helper (unit-tested). An explicit --no-gpu still skips enforcement so a host with an unusable GPU can onboard CPU-only. Verified on a real NVIDIA H100 host (nvidia-smi hidden, toolkit removed, Docker CDI configured, spec wiped): onboard now blocks at [1/8] with the install_nvidia_container_toolkit remediation and exits non-zero, instead of continuing to [2/8]. Refs #5489 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Jason Ma <jama@nvidia.com>
Update: fixed the enforcement gap (detection alone didn't fix #5489)Runtime-verified the prior state of this PR on a real NVIDIA H100 host (recreated #5489: Root cause: the lspci detection here correctly sets Pushed
Re-verified on the same H100: onboard now blocks at |
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 `@src/lib/onboard.ts`:
- Around line 1594-1599: The detailed explanatory comments about issue `#5489` in
src/lib/onboard.ts at lines 1594-1599 and lines 1661-1664 are causing the file
to exceed the growth guardrail limit. Either significantly trim these comment
blocks to be more concise (removing the detailed rationale about sandbox GPU
passthrough, auto-disable behavior, and the specifics of how the CDI gate
contract works), or move the detailed `#5489` rationale explanation to
src/lib/onboard/preflight.ts or to test files, keeping only a brief reference
comment in the main onboard.ts file if needed. This will allow
src/lib/onboard.ts to remain net-neutral in size as required by CI.
🪄 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: 04a3c12e-e6bc-424f-bd90-d38ecc121175
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/onboard/preflight-cdi.test.tssrc/lib/onboard/preflight.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard/preflight.ts
prekshivyas
left a comment
There was a problem hiding this comment.
Both fixes are correct. lspciLineIsNvidiaGpu anchoring on PCI class label before checking vendor prevents Mellanox NICs and other non-GPU NVIDIA PCI devices from triggering GPU detection — the regex approach is standard. Separating explicitlyOptedOutGpuPassthrough from the auto-disable path and extracting shouldEnforceCdiNvidiaGpuSpec correctly fixes the bypass where auto-disable was masking enforcement.
Test coverage is thorough: three new assessHost integration cases and four unit cases for shouldEnforceCdiNvidiaGpuSpec cover the primary bug, the false-positive NIC case, the explicit --no-gpu escape hatch, and the healthy-spec no-op.
One thing to note: this is an automated fix and several verification boxes are unchecked (hooks, secrets scan, docs). Please confirm npx prek run --from-ref main --to-ref HEAD passes and no secrets were introduced before merge.
Approve pending hook verification.
…module Relocates the CDI guard function from onboard.ts to onboard/preflight.ts, where all its dependencies (shouldEnforceCdiNvidiaGpuSpec, planHostRemediation, isWslDockerDesktopRuntime) already live. This brings onboard.ts back to net-neutral per the codebase-growth-guardrails check (-26/+15 vs main). Also removes the !sandboxGpuConfig.sandboxGpuEnabled term from the opted-out check (the #5489 bypass) and drops verbose comment blocks — the parameter name explicitlyOptedOutGpuPassthrough makes the intent self-documenting. Signed-off-by: Preksha Vyas <prekshiv@nvidia.com>
The codebase-growth-guardrails check requires test files not to add if statements. The two new lspci fallback tests each used a 3-branch if chain inside their runCaptureImpl inline. Extract those to runCaptureWithLspci() which encodes command dispatch as a Map lookup and uses ternaries, keeping the test bodies linear and the total if-statement count at 12 (unchanged from base). Signed-off-by: Preksha Vyas <prekshiv@nvidia.com>
…eflight-skips-install-nvidia' into fix-5500-ci
Selective E2E Results — ✅ All requested jobs passedRun: 28204392989
|
## Summary Addresses issue NVIDIA#5489. ## Related Issue Fixes NVIDIA#5489 ## Changes - Automated Claude Code fix selected by `auto_fix/auto_fix_recent_issues.py`. - See the commits on this branch for the exact file-level changes. ## 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) ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [ ] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Tests added or updated for new or changed behavior - [ ] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `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) Verification details reported by automation: - Verification (local; host unreachable): build=ok; repro tests on fix: PASS. - Codex review (independent, advisory) posted as a PR comment. - Addressed from codex review: Restricted the lspci hardware fallback in detectNvidiaGpuHardware to display-class PCI devices (VGA compatible controller / 3D controller / Display controller) that are also NVIDIA, instead of matching any line containing 'nvidia'. This prevents NVIDIA/Mellanox NICs and other non-GPU NVIDIA PCI devices from being marked hasNvidiaGpu and forced through blocking CDI/toolkit remediation. Added a preflight-cdi test asserting a host with only NVIDIA/Mellanox NICs is not flagged as having a GPU; the existing NVIDIA#5489 GPU repro still passes. - Run focused tests for the touched module, then `npm test` when feasible. --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Jason Ma <jama@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved NVIDIA GPU detection by falling back to PCI hardware probing when software-based checks are missing or return no results. * Refined CDI NVIDIA GPU spec enforcement so remediation is triggered only when specs are missing or unhealthy, and is skipped only when the operator explicitly disables GPU passthrough (not when it’s disabled implicitly). * **Tests** * Added coverage for enforcement gating and for NVIDIA GPU classification across different `lspci` outcomes, including scenarios where `nvidia-smi` returns empty results. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jason Ma <jama@nvidia.com> Signed-off-by: Preksha Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
Summary
Addresses issue #5489.
Related Issue
Fixes #5489
Changes
auto_fix/auto_fix_recent_issues.py.Type of Change
Verification
npx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Verification details reported by automation:
npm testwhen feasible.Signed-off-by: Jason Ma jama@nvidia.com
Summary by CodeRabbit
lspcioutcomes, including scenarios wherenvidia-smireturns empty results.