refactor(cli): delete unused code#3632
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
📝 WalkthroughWalkthroughRefactors onboarding/policy/dashboard/recovery, relocates GPU/credential helpers, slims CLI/debug contracts, removes ts-migration guard/tooling and related CI workflow, updates exports, and adds extensive onboarding/unit tests plus small import/test tidy-ups. ChangesOnboarding, CLI, 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)
|
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
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
| allTiers.forEach((t) => { | ||
| const marker = t.name === defaultTier.name ? RADIO_ON : RADIO_OFF; | ||
| console.log(` ${marker} ${t.label}`); |
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.
| 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>
Selective E2E Results — ✅ All requested jobs passedRun: 25946296361
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cjagwani
left a comment
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.
## 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>
Selective E2E Results — ✅ All requested jobs passedRun: 25949687267
|
## 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>
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>
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
| if (device && (options.flag === "enable" || envMode === "1")) { | ||
| mode = "1"; | ||
| } |
There was a problem hiding this comment.
Explicit flag="disable" can be overridden by envMode="1" + device presence.
When flag="disable" and envMode="1" with a device set:
resolveSandboxGpuModecorrectly returns mode"0"(flag wins)- Line 69-71 pushes an error about device+mode incompatibility
- Line 72 condition
device && (false || true)is true, so mode becomes"1"
This overrides the explicit disable flag and leaves an inconsistent error message. The explicit flag should take precedence over environment configuration.
Suggested fix
- if (device && (options.flag === "enable" || envMode === "1")) {
+ if (device && options.flag !== "disable" && (options.flag === "enable" || envMode === "1")) {
mode = "1";
}Alternatively, this block may be entirely redundant since resolveSandboxGpuMode already handles flag="enable" → mode "1" and envMode="1" → mode "1" (unless overridden by flag). Consider whether lines 72-74 add any value.
📝 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.
| if (device && (options.flag === "enable" || envMode === "1")) { | |
| mode = "1"; | |
| } | |
| if (device && options.flag !== "disable" && (options.flag === "enable" || envMode === "1")) { | |
| mode = "1"; | |
| } |
🤖 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-mode.ts` around lines 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.
Summary
Deletes unused legacy helpers, imports, exports, package dependencies, migration scaffolding, and stale compatibility leftovers across the CLI, onboarding flow, scripts, and tests. This reduces dead code and trims dependency metadata while preserving current behavior.
Changes
src/nemoclaw.tsandsrc/lib/onboard.ts.ts-prune/madge, including unused dashboard health/recovery modules and the old debug-argument parser path.chat-filtermodule and duplicate tests.assert.*source-text tests.execadevDependency and plugincommanderdependency.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Removals
Chores