refactor(cli): prune dead code and harden onboarding#3632
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@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:
📝 WalkthroughWalkthroughRemoves legacy onboarding/dashboard recovery and migration tooling, relocates GPU/credential helpers, slims CLI/debug contracts, prunes scripts/package deps, and adds many onboarding utilities and tests. ChangesOnboarding and repository cleanup
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
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 8301-8303: The non-TTY tier listing printed by
allTiers.forEach(...) no longer shows numeric indices even though the prompt
expects "Select tier [1-N]"; update the loop in the same block (the
allTiers.forEach callback that references defaultTier, RADIO_ON, RADIO_OFF) to
include the index (use the forEach second parameter i or map with index) and
change the console.log to print the numeric label (i+1) alongside the marker and
t.label so redirected/non-TTY runs can match numbers to tiers.
🪄 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: df7fe4e2-5f2e-4966-a38c-047270bb31d1
📒 Files selected for processing (26)
scripts/bump-version.tsscripts/find-source-shape-tests.tsscripts/ts-migration-assist.tsscripts/validate-configs.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/vm-dns-monkeypatch.tssrc/lib/agent/onboard.tssrc/lib/cli/command-registry.test.tssrc/lib/deploy/index.tssrc/lib/inference/local.test.tssrc/lib/onboard.tssrc/lib/onboard/usage-notice.tssrc/lib/registry-recovery-action.tssrc/lib/sandbox/create-stream.tssrc/lib/shields/index.tssrc/lib/state/sandbox-session.test.tssrc/lib/tunnel/services-sandbox.test.tssrc/nemoclaw.tstest/credential-exposure.test.tstest/e2e/runtime/resolver/validator.tstest/install-preflight.test.tstest/onboard.test.tstest/policies.test.tstest/rebuild-policy-presets.test.tstest/secret-redaction.test.tstest/wait.test.ts
💤 Files with no reviewable changes (14)
- src/lib/deploy/index.ts
- src/lib/registry-recovery-action.ts
- src/lib/actions/sandbox/policy-channel.ts
- test/credential-exposure.test.ts
- scripts/ts-migration-assist.ts
- test/secret-redaction.test.ts
- test/install-preflight.test.ts
- src/lib/tunnel/services-sandbox.test.ts
- src/lib/onboard/usage-notice.ts
- src/lib/cli/command-registry.test.ts
- test/onboard.test.ts
- src/lib/state/sandbox-session.test.ts
- test/policies.test.ts
- scripts/bump-version.ts
Comment on lines
+8301
to
8303
| allTiers.forEach((t) => { | ||
| const marker = t.name === defaultTier.name ? RADIO_ON : RADIO_OFF; | ||
| console.log(` ${marker} ${t.label}`); |
Contributor
There was a problem hiding this comment.
Restore numeric labels in the non-TTY tier selector.
This fallback still asks for Select tier [1-N], but the list no longer shows which number maps to each tier. Redirected/non-TTY runs now have to guess the ordering.
Suggested fix
- allTiers.forEach((t) => {
+ allTiers.forEach((t, index) => {
const marker = t.name === defaultTier.name ? RADIO_ON : RADIO_OFF;
- console.log(` ${marker} ${t.label}`);
+ console.log(` ${index + 1}) ${marker} ${t.label}`);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| allTiers.forEach((t) => { | |
| const marker = t.name === defaultTier.name ? RADIO_ON : RADIO_OFF; | |
| console.log(` ${marker} ${t.label}`); | |
| allTiers.forEach((t, index) => { | |
| const marker = t.name === defaultTier.name ? RADIO_ON : RADIO_OFF; | |
| console.log(` ${index + 1}) ${marker} ${t.label}`); | |
| }); |
🤖 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.ts` around lines 8301 - 8303, The non-TTY tier listing
printed by allTiers.forEach(...) no longer shows numeric indices even though the
prompt expects "Select tier [1-N]"; update the loop in the same block (the
allTiers.forEach callback that references defaultTier, RADIO_ON, RADIO_OFF) to
include the index (use the forEach second parameter i or map with index) and
change the console.log to print the numeric label (i+1) alongside the marker and
t.label so redirected/non-TTY runs can match numbers to tiers.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25946296361
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cjagwani
approved these changes
May 16, 2026
cjagwani
left a comment
Contributor
There was a problem hiding this comment.
Clean refactor — subtractive only, all 25+ CI checks green, every removed export verified zero-caller. 7/7 self-declared scope clauses met. Two non-blocking nits worth flagging:
scripts/bump-version.ts:548–580— droppedupdateDocsConf+updateDocsProjectJson. They're orphaned per grep, but thenemoclaw-maintainer-cut-release-tagskill calls intobump-version.ts; worth confirming with the release flow that docs version files don't expect those to fire.test/credential-exposure.test.ts:30— the two removed SOURCE_FILES constants point to source files that vanished in a prior PR; the test now only meaningfully validatesONBOARD_JS+RUNNER_TS. Worth a follow-up to retire or refocus it.
Approving.
12 tasks
## Summary Reintroduces the source-shape guard as a stacked follow-up to #3632, but fixes the detector so it catches Node `assert.*` source-text assertions instead of reporting a false zero. It then completes the source-shape purge by removing the detected source-text assertions from `test/onboard.test.ts` and ratcheting the budget back to zero. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Stacked on #3632. ## Changes <!-- Bullet list of key changes. --> - Restored `scripts/find-source-shape-tests.ts`, `source-shape:*` npm scripts, and the prek source-shape budget hook. - Added detection for `assert.match`, `assert.doesNotMatch`, `assert.ok`, and related Node assert-style source-text assertions. - Avoided counting temp Dockerfile fixture behavior tests as source-shape offenders. - Removed all detected source-shape tests from `test/onboard.test.ts` where behavior is covered by existing helper/integration tests. - Ratcheted the source-shape budget back to zero. ## 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. --> - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make 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) --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Enhanced test validation infrastructure to track and enforce limits on source-shape test cases across the codebase. * Updated test coverage with new behavioral assertions and removed outdated static-source regression checks. * **Chores** * Added development tools and CI configuration to support automated source-shape test monitoring and budgeting. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3636?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25949687267
|
15 tasks
## Summary Splits helper-oriented coverage out of the oversized `test/onboard.test.ts` file into focused unit test files. The PR keeps source-shape coverage at zero, preserves equivalent behavior coverage, and makes the remaining onboard orchestration tests easier to navigate. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Stacked on #3632 / #3636. ## Changes <!-- Bullet list of key changes. --> - Moved GPU helper coverage into focused module tests: - `src/lib/onboard/gateway-gpu-passthrough.test.ts` - `src/lib/onboard/sandbox-gpu-create.test.ts` - `src/lib/onboard/sandbox-gpu-mode.test.ts` - `src/lib/onboard/initial-policy.test.ts` - Moved policy suggestion helper coverage into `test/onboard-policy-suggestions.test.ts`. - Moved dashboard helper coverage into `test/onboard-dashboard.test.ts` and dashboard port helper coverage into `src/lib/onboard/dashboard-port.test.ts`. - Moved sandbox naming helper coverage into `test/onboard-sandbox-name.test.ts`. - Moved OpenShell version/pinning helper coverage into `test/onboard-openshell-version.test.ts`. - Moved Dockerfile patch helper coverage into `src/lib/onboard/dockerfile-patch.test.ts`. - Moved gateway runtime / Docker-driver / CDI helper coverage into `test/onboard-gateway-runtime.test.ts`. - Moved Model Router orchestration coverage into `test/onboard-model-router.test.ts`. - Moved messaging setup/provider coverage into `test/onboard-messaging.test.ts`. - Moved custom Dockerfile orchestration coverage into `test/onboard-custom-dockerfile.test.ts`. - Moved provider routing/probe/provider-CRUD helper coverage into owning module tests: - `src/lib/inference/config.test.ts` - `src/lib/inference/onboard-probes.test.ts` - `src/lib/onboard/providers.test.ts` - `src/lib/validation.test.ts` - `src/lib/core/url-utils.test.ts` - `src/lib/adapters/http/probe.test.ts` - Moved web-search support coverage into `src/lib/onboard/web-search-support.test.ts` / `test/onboard-brave-validation.test.ts`. - Moved build-context and sandbox-create recovery hint coverage into `src/lib/build-context.test.ts`. - Extracted focused production helpers with direct module coverage: - `src/lib/onboard/credential-env.ts` - `src/lib/onboard/summary.ts` - additional exports in `src/lib/onboard/sandbox-gpu-mode.ts` and `src/lib/onboard/dashboard-port.ts` - Removed the corresponding helper-only cases from `test/onboard.test.ts`; it is now much smaller while keeping the remaining orchestration/integration-style coverage. ## 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. --> - [x] `npm run source-shape:check` passes - [x] `npm run build:cli` passes - [x] `npx tsc -p tsconfig.cli.json --noUnusedLocals --noUnusedParameters --pretty false` passes - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make 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) --- <!-- DCO sign-off required by CI. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Tests** * Added comprehensive test coverage for GPU configuration, sandbox naming, credential management, dashboard port handling, custom Dockerfile support, messaging channels, Model Router setup, and OpenShell version management. * Added tests for probe error handling, policy validation, and sandbox state detection. * **Refactor** * Reorganized onboarding helpers into dedicated modules for improved maintainability. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3640?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
8108-8115:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore numeric labels in the non-TTY tier selector.
Line 8114 still prompts for
Select tier [1-N], but the fallback list on Lines 8108-8110 no longer shows which number maps to each tier.Suggested fix
- allTiers.forEach((t) => { + allTiers.forEach((t, index) => { const marker = t.name === defaultTier.name ? RADIO_ON : RADIO_OFF; - console.log(` ${marker} ${t.label}`); + console.log(` ${index + 1}) ${marker} ${t.label}`); });🤖 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.ts` around lines 8108 - 8115, The printed fallback list for non-TTY tier selection no longer shows numeric labels, causing the prompt `Select tier [1-${allTiers.length}]` to be misleading; update the loop that renders tiers (where allTiers.forEach, RADIO_ON/RADIO_OFF, and defaultTier are used) to include the index number for each tier (e.g., display `${i + 1}` alongside `marker` and `t.label`) so users can see which number corresponds to each tier before the prompt call to prompt(...).
🤖 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/sandbox-gpu-mode.ts`:
- Around line 72-74: The current condition lets mere device presence flip mode
to "1" (overriding the earlier Jetson default "0"); change the check so mode is
set to "1" only when a GPU is present AND the user explicitly enabled
passthrough (e.g., options.flag === "enable" or envMode === "1"). Update the if
that references device, options.flag, envMode, and mode so it requires explicit
enablement rather than just not-"disable"/not-"0".
---
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 8108-8115: The printed fallback list for non-TTY tier selection no
longer shows numeric labels, causing the prompt `Select tier
[1-${allTiers.length}]` to be misleading; update the loop that renders tiers
(where allTiers.forEach, RADIO_ON/RADIO_OFF, and defaultTier are used) to
include the index number for each tier (e.g., display `${i + 1}` alongside
`marker` and `t.label`) so users can see which number corresponds to each tier
before the prompt call to prompt(...).
🪄 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: fe6fd633-fc51-4db4-8d87-0ecb8e23c23d
📒 Files selected for processing (33)
src/lib/adapters/http/probe.test.tssrc/lib/build-context.test.tssrc/lib/core/url-utils.test.tssrc/lib/inference/config.test.tssrc/lib/inference/onboard-probes.test.tssrc/lib/onboard.tssrc/lib/onboard/credential-env.test.tssrc/lib/onboard/credential-env.tssrc/lib/onboard/dashboard-port.test.tssrc/lib/onboard/dashboard-port.tssrc/lib/onboard/dockerfile-patch.test.tssrc/lib/onboard/gateway-gpu-passthrough.test.tssrc/lib/onboard/initial-policy.test.tssrc/lib/onboard/providers.test.tssrc/lib/onboard/sandbox-gpu-create.test.tssrc/lib/onboard/sandbox-gpu-mode.test.tssrc/lib/onboard/sandbox-gpu-mode.tssrc/lib/onboard/summary.test.tssrc/lib/onboard/summary.tssrc/lib/onboard/vm-dns-monkeypatch.test.tssrc/lib/onboard/web-search-support.test.tssrc/lib/validation.test.tstest/gateway-state.test.tstest/onboard-brave-validation.test.tstest/onboard-custom-dockerfile.test.tstest/onboard-dashboard.test.tstest/onboard-gateway-runtime.test.tstest/onboard-messaging.test.tstest/onboard-model-router.test.tstest/onboard-openshell-version.test.tstest/onboard-policy-suggestions.test.tstest/onboard-sandbox-name.test.tstest/onboard.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/onboard/gateway-gpu-passthrough.test.ts
…d-code # Conflicts: # test/onboard.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
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/sandbox-gpu-mode.ts`:
- Around line 72-74: The block that sets mode = "1" when device && (options.flag
=== "enable" || envMode === "1") can override an explicit disable flag; update
resolveSandboxGpuMode (or the conditional immediately around the mode
assignment) so that options.flag === "disable" always wins: either remove the
device/envMode override entirely or change the condition to check flag !==
"disable" before applying envMode/device logic (e.g., require options.flag !==
"disable" && (options.flag === "enable" || envMode === "1")), and ensure any
existing device+mode incompatibility error is only pushed when the flag does not
explicitly disable GPU.
🪄 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: 8100c697-1ed1-4964-989c-bb913f4be38f
📒 Files selected for processing (2)
src/lib/onboard/sandbox-gpu-mode.test.tssrc/lib/onboard/sandbox-gpu-mode.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard/sandbox-gpu-mode.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25968322630
|
Contributor
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/sandbox-gpu-mode.ts`:
- Around line 69-77: The current validation skips rejecting device selectors
when options.flag === "disable"; update the check to reject any requestedDevice
unless the effective mode is "1". Replace the condition using
explicitEnable/explicitDisable with a direct check against mode (e.g. if
(requestedDevice && mode !== "1") { errors.push(...); }) so selectors are only
accepted when mode === "1"; leave the sandboxGpuDevice assignment (const
sandboxGpuDevice = mode === "1" ? requestedDevice : null) as-is.
🪄 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: 9ae4af79-1007-4dbd-9cb9-98b92349e1ca
📒 Files selected for processing (5)
docs/reference/commands.mdsrc/lib/commands/onboard/common.tssrc/lib/onboard/legacy-command.tssrc/lib/onboard/sandbox-gpu-mode.test.tssrc/lib/onboard/sandbox-gpu-mode.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/commands/onboard/common.ts
- src/lib/onboard/legacy-command.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25969431429
|
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
3 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
Prunes unused CLI and onboarding code after the TypeScript migration, removes obsolete migration scaffolding, and keeps the command/test surface aligned with current oclif-owned behavior. The PR also preserves the active onboarding behavior by tightening sandbox GPU device selection so device selectors require explicit GPU enablement and do not create disabled-mode drift.
Changes
src/nemoclaw.ts,src/lib/onboard.ts, runtime recovery, Docker adapters, credentials, dashboard helpers, inference helpers, and command modules.execadevDependency and plugincommanderdependency.assert.*source-text assertions while avoiding fixture false positives.--sandbox-gpu-device/NEMOCLAW_SANDBOX_GPU_DEVICEnow requires explicit sandbox GPU mode1, and disabled GPU configs clear the recorded device to avoid drift.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com