refactor(cli): extract pure functions from onboard.js to typed TypeScript modules#1240
Merged
refactor(cli): extract pure functions from onboard.js to typed TypeScript modules#1240
Conversation
…ript modules Move ~210 lines of pure, side-effect-free functions from the 3,800-line onboard.js into five typed TypeScript modules under src/lib/: - gateway-state.ts: gateway/sandbox state classification - validation.ts: failure classification, API key validation, model ID checks - url-utils.ts: URL normalization, text compaction, env formatting - build-context.ts: Docker build context filtering, recovery hints - dashboard.ts: dashboard URL resolution and construction Infrastructure: - tsconfig.src.json compiles src/ → dist/ as CJS (dist/ already gitignored) - tsconfig.cli.json updated to type-check src/ - npm run build:cli added to package.json - Pre-commit test-cli hook builds TS and includes dist/lib/ in coverage onboard.js imports from compiled dist/lib/ output. All 542 CLI tests pass. No user-facing behavior changes. Closes #1237. Relates to #924 (shell consolidation). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
56 new tests across three co-located test files: - src/lib/validation.test.ts: classifyValidationFailure, classifyApplyFailure, classifySandboxCreateFailure, validateNvidiaApiKeyValue, isSafeModelId - src/lib/dashboard.test.ts: resolveDashboardForwardTarget, buildControlUiUrls - src/lib/url-utils.test.ts: compactText, stripEndpointSuffix, normalizeProviderBaseUrl, isLoopbackHostname, formatEnvAssignment, parsePolicyPresetEnv vitest.config.ts updated to include src/**/*.test.ts in the CLI project. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The existing callsite in printDashboard calls buildControlUiUrls() with no arguments when no token is available. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughExtracted multiple pure helper functions from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/url-utils.ts (1)
60-65: Consider stricter parameter typing or explicit union type.The parameter
valueis typed asstringbut the implementation handlesnull/undefinedvia(value || ""). This defensive coding is fine, but the type could bestring | null | undefinedto accurately reflect the runtime handling.💡 Optional: Align type with runtime behavior
-export function parsePolicyPresetEnv(value: string): string[] { +export function parsePolicyPresetEnv(value: string | null | undefined): string[] { return (value || "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/url-utils.ts` around lines 60 - 65, The function parsePolicyPresetEnv declares its parameter as string but defensively handles null/undefined via (value || ""); change the parameter type to string | null | undefined in parsePolicyPresetEnv to match runtime behavior, update any call sites that rely on strict string typing if necessary, and run type checks to ensure no additional casts are required; keep the internal implementation (value || "") as-is so behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/url-utils.ts`:
- Around line 60-65: The function parsePolicyPresetEnv declares its parameter as
string but defensively handles null/undefined via (value || ""); change the
parameter type to string | null | undefined in parsePolicyPresetEnv to match
runtime behavior, update any call sites that rely on strict string typing if
necessary, and run type checks to ensure no additional casts are required; keep
the internal implementation (value || "") as-is so behavior remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c626f2c-ae88-40e4-a29c-54bd11b300ab
📒 Files selected for processing (14)
.pre-commit-config.yamlbin/lib/onboard.jspackage.jsonsrc/lib/build-context.tssrc/lib/dashboard.test.tssrc/lib/dashboard.tssrc/lib/gateway-state.tssrc/lib/url-utils.test.tssrc/lib/url-utils.tssrc/lib/validation.test.tssrc/lib/validation.tstsconfig.cli.jsontsconfig.src.jsonvitest.config.ts
5 tasks
The tsc-js pre-push hook (jsconfig.json) type-checks bin/lib/onboard.js which requires dist/lib/ to exist. CI was not running npm run build:cli before the prek checks, causing TS2307 "Cannot find module" errors. - Add npm run build:cli step to .github/actions/basic-checks - Update tsc-js hook to run build:cli before tsc Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…law into cv/extract-onboard-pure-fns
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/actions/basic-checks/action.yaml (1)
37-39: Step placement is correct; build artifacts will exist before hooks run.The new step ensures
dist/lib/artifacts are compiled beforeprekhooks execute. This provides clearer error isolation in CI if the TypeScript build fails.Minor observation: the
tsc-jsandtest-clihooks (lines 197-204 and 228-234 in.pre-commit-config.yaml) each includenpm run build:cli &&in their entry commands, resulting in the CLI TypeScript compiling three times per CI run. Consider either removing the explicit build from the hook entries (since this action step guarantees artifacts exist) or accepting the redundancy for the defense-in-depth benefit when hooks run locally outside CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/basic-checks/action.yaml around lines 37 - 39, The CI now builds CLI TypeScript artifacts in the action step named "Build CLI TypeScript modules", so remove the redundant "npm run build:cli &&" prefixes from the pre-commit hook entry commands for the tsc-js and test-cli hooks in .pre-commit-config.yaml (or alternatively keep them if you prefer defense-in-depth); update the hook entries for the tsc-js and test-cli hooks to run only their checks (e.g., remove the explicit build invocation) so the CLI is not compiled three times per CI run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/actions/basic-checks/action.yaml:
- Around line 37-39: The CI now builds CLI TypeScript artifacts in the action
step named "Build CLI TypeScript modules", so remove the redundant "npm run
build:cli &&" prefixes from the pre-commit hook entry commands for the tsc-js
and test-cli hooks in .pre-commit-config.yaml (or alternatively keep them if you
prefer defense-in-depth); update the hook entries for the tsc-js and test-cli
hooks to run only their checks (e.g., remove the explicit build invocation) so
the CLI is not compiled three times per CI run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29f6e4dc-0801-45f9-9317-c2d1d4cf19d6
📒 Files selected for processing (2)
.github/actions/basic-checks/action.yaml.pre-commit-config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .pre-commit-config.yaml
## Summary - Convert `bin/lib/preflight.js` (357 lines) to `src/lib/preflight.ts` with full type definitions - Typed interfaces for all opts objects and return types: `PortProbeResult`, `MemoryInfo`, `SwapResult`, `CheckPortOpts`, `GetMemoryInfoOpts`, `EnsureSwapOpts` - Extract `parseLsofLines` helper to reduce duplication in `checkPortAvailable` - Incorporate #1227 fix: `sudo` → `sudo -n` (non-interactive) for lsof retry - `bin/lib/preflight.js` becomes a thin re-export shim — existing consumers unaffected - Co-locate tests: `test/preflight.test.js` → `src/lib/preflight.test.ts` - Add real net probe tests (EADDRINUSE detection on occupied ports) - Fix all co-located test imports to use `dist/` paths for coverage attribution - Add targeted dashboard/validation branch tests to maintain ratchet Stacked on #1240. Not touched by any #924 blocker PR. ## Test plan - [x] 612 CLI tests pass (601 existing + 11 new) - [x] `tsc -p tsconfig.src.json` compiles cleanly - [x] `tsc -p tsconfig.cli.json` type-checks cleanly - [x] `tsc -p jsconfig.json` type-checks cleanly (the pre-push check that caught the union issue) - [x] Coverage ratchet passes Relates to #924 (shell consolidation). Supersedes #1227. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/preflight.test.ts (1)
318-326: Consider strengthening the default threshold test.The test name says "uses default 12000 MB threshold" but doesn't actually verify that value. To truly test the default, consider using a memory value between potential defaults (e.g., 10000 MB which is below 12000 but above 0):
💡 Suggested test improvement
it("uses default 12000 MB threshold when minTotalMB is undefined", () => { + // 10000 MB is below the 12000 default, so swap should be offered + const resultBelowDefault = ensureSwap(undefined, { + platform: "linux", + memoryInfo: { totalRamMB: 10000, totalSwapMB: 0, totalMB: 10000 }, + dryRun: true, + swapfileExists: false, + }); + expect(resultBelowDefault.swapCreated).toBe(true); + + // 16000 MB exceeds the default, so no swap needed const result = ensureSwap(undefined, { platform: "linux", memoryInfo: { totalRamMB: 16000, totalSwapMB: 0, totalMB: 16000 }, }); expect(result.ok).toBe(true); expect(result.swapCreated).toBe(false); - expect(result.totalMB).toBe(16000); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/preflight.test.ts` around lines 318 - 326, Update the test for ensureSwap to actually validate the default 12000 MB threshold by using a RAM value between possible thresholds (e.g., set memoryInfo.totalRamMB to 10000) so the default should trigger swap creation; assert that ensureSwap(...) returns ok true and swapCreated true (and any other relevant fields your implementation fills) to prove the default 12000 MB threshold was applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/preflight.ts`:
- Around line 359-407: The code computes o.interactive but never uses it; before
attempting to create swap, check o.interactive (the interactive flag on the
local options object passed into ensureSwap) and if it's false return a
SwapResult that skips creation (e.g., { ok: true, totalMB: mem.totalMB,
swapCreated: false, reason: "non-interactive mode, skipping swap creation" })
instead of calling createSwapfile; add this check after diskSpaceResult is
computed (before calling createSwapfile(mem)) so non-interactive environments do
not try to create a swapfile.
---
Nitpick comments:
In `@src/lib/preflight.test.ts`:
- Around line 318-326: Update the test for ensureSwap to actually validate the
default 12000 MB threshold by using a RAM value between possible thresholds
(e.g., set memoryInfo.totalRamMB to 10000) so the default should trigger swap
creation; assert that ensureSwap(...) returns ok true and swapCreated true (and
any other relevant fields your implementation fills) to prove the default 12000
MB threshold was applied.
🪄 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: Pro
Run ID: ece9092d-d2cd-4f7e-89ba-a9b91f8d7e8e
📒 Files selected for processing (7)
bin/lib/preflight.jssrc/lib/dashboard.test.tssrc/lib/preflight.test.tssrc/lib/preflight.tssrc/lib/url-utils.test.tssrc/lib/validation.test.tstest/preflight.test.js
💤 Files with no reviewable changes (1)
- test/preflight.test.js
✅ Files skipped from review due to trivial changes (3)
- src/lib/url-utils.test.ts
- src/lib/dashboard.test.ts
- src/lib/validation.test.ts
Comment on lines
+359
to
+407
| export function ensureSwap(minTotalMB?: number, opts: EnsureSwapOpts = {}): SwapResult { | ||
| const o = { | ||
| platform: process.platform as NodeJS.Platform, | ||
| memoryInfo: null as MemoryInfo | null, | ||
| swapfileExists: fs.existsSync("/swapfile"), | ||
| dryRun: false, | ||
| interactive: process.stdout.isTTY && !process.env.NEMOCLAW_NON_INTERACTIVE, | ||
| getMemoryInfoImpl: getMemoryInfo, | ||
| ...opts, | ||
| }; | ||
| const threshold = minTotalMB ?? 12000; | ||
|
|
||
| if (o.platform !== "linux") { | ||
| return { ok: true, totalMB: 0, swapCreated: false }; | ||
| } | ||
|
|
||
| const mem = o.memoryInfo ?? o.getMemoryInfoImpl({ platform: o.platform }); | ||
| if (!mem) { | ||
| return { ok: false, reason: "could not read memory info" }; | ||
| } | ||
|
|
||
| if (mem.totalMB >= threshold) { | ||
| return { ok: true, totalMB: mem.totalMB, swapCreated: false }; | ||
| } | ||
|
|
||
| if (o.dryRun) { | ||
| if (o.swapfileExists) { | ||
| return { | ||
| ok: true, | ||
| totalMB: mem.totalMB, | ||
| swapCreated: false, | ||
| reason: "/swapfile already exists", | ||
| }; | ||
| } | ||
| return { ok: true, totalMB: mem.totalMB, swapCreated: true }; | ||
| } | ||
|
|
||
| const existingSwapResult = getExistingSwapResult(mem); | ||
| if (existingSwapResult) { | ||
| return existingSwapResult; | ||
| } | ||
|
|
||
| const diskSpaceResult = checkSwapDiskSpace(); | ||
| if (diskSpaceResult) { | ||
| return diskSpaceResult; | ||
| } | ||
|
|
||
| return createSwapfile(mem); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the original onboard.js had interactive checks for swap creation
rg -n -B3 -A3 'interactive.*swap|swap.*interactive' bin/lib/onboard.jsRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Find ensureSwap function and check if interactive is used
rg -n -B5 -A50 'ensureSwap.*interactive|interactive.*used|interactive.*passed' src/lib/preflight.tsRepository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Check the actual implementation of createSwapfile to see if it uses interactive
rg -n -B3 -A20 'function createSwapfile|const createSwapfile' src/lib/preflight.tsRepository: NVIDIA/NemoClaw
Length of output: 965
🏁 Script executed:
# Check how ensureSwap is called in the codebase
rg -n -B3 -A3 'ensureSwap\(' --type ts --type jsRepository: NVIDIA/NemoClaw
Length of output: 4073
🏁 Script executed:
# Check if there's any pattern of interactive mode checks in preflight.ts
rg -n 'interactive' src/lib/preflight.tsRepository: NVIDIA/NemoClaw
Length of output: 218
The interactive option is computed but never used to gate swap creation.
Line 365 computes interactive: process.stdout.isTTY && !process.env.NEMOCLAW_NON_INTERACTIVE, but this value is never checked in ensureSwap before calling createSwapfile. If non-interactive mode (e.g., CI, headless environments) should skip swap creation, the check is missing.
Suggested fix
if (mem.totalMB >= threshold) {
return { ok: true, totalMB: mem.totalMB, swapCreated: false };
}
+ if (!o.interactive) {
+ return {
+ ok: true,
+ totalMB: mem.totalMB,
+ swapCreated: false,
+ reason: "running in non-interactive mode, not creating swap",
+ };
+ }
+
if (o.dryRun) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/preflight.ts` around lines 359 - 407, The code computes o.interactive
but never uses it; before attempting to create swap, check o.interactive (the
interactive flag on the local options object passed into ensureSwap) and if it's
false return a SwapResult that skips creation (e.g., { ok: true, totalMB:
mem.totalMB, swapCreated: false, reason: "non-interactive mode, skipping swap
creation" }) instead of calling createSwapfile; add this check after
diskSpaceResult is computed (before calling createSwapfile(mem)) so
non-interactive environments do not try to create a swapfile.
brandonpelfrey
approved these changes
Apr 1, 2026
Collaborator
brandonpelfrey
left a comment
brandonpelfrey
left a comment
There was a problem hiding this comment.
I don't see any new issues here. Definitely a positive move towards TS.
Contributor
|
PR Review: Nice extraction — scoping this to pure/near-pure functions and leaving the stateful parts of 🔴 Blockers1. Duplicated Suggestion: export it from 2. The module already does DI extensively ( 🟡 Warnings3. No tests for 4. 5. 6. 🔵 Suggestions
Overall this is solid groundwork. The |
This was referenced Apr 1, 2026
wscurran
added
NemoClaw CLI
ericksoa
added a commit
that referenced
this pull request
Apr 1, 2026
PR #1240 replaced bin/lib/ CJS modules with thin shims that re-export from dist/lib/, but install.sh was never updated to run `npm run build:cli`. Every fresh install now crashes on `nemoclaw onboard` with "Cannot find module '../../dist/lib/preflight'". Add the missing build:cli step to both install paths (local source and GitHub clone). Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
3 tasks
cv
pushed a commit
that referenced
this pull request
Apr 1, 2026
## Summary - **install.sh missing build:cli**: PR #1240 replaced `bin/lib/` CJS modules with shims that re-export from `dist/lib/`, but `install.sh` never ran `npm run build:cli`. Every fresh install crashed on `nemoclaw onboard` with `Cannot find module '../../dist/lib/preflight'` - **E2E destroy verification**: The cleanup phase used `nemoclaw list` to verify a sandbox was gone, but `list` triggers gateway recovery which can restart a destroyed gateway and re-import the sandbox. Now checks the registry file directly instead - Uses `--if-present` so installs targeting older tags without `build:cli` don't break ## Test plan - [ ] Run `install.sh` on a clean VM and verify `nemoclaw onboard` no longer crashes - [ ] Verify `dist/lib/preflight.js` exists after install completes - [ ] Nightly E2E cloud-e2e passes the destroy verification phase --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
cv
added a commit
that referenced
this pull request
Apr 2, 2026
## Summary - Convert `bin/lib/runtime-recovery.js` (84 lines) to `src/lib/runtime-recovery.ts` - Typed `StateClassification` interface for sandbox/gateway state - 5 pure classifiers/parsers + 1 I/O function (`getRecoveryCommand`) - Co-locate tests with additional edge cases Stacked on #1240. 617 CLI tests pass. Coverage ratchet passes. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved runtime recovery logic to better parse CLI output and determine recovery actions, including more reliable sandbox/gateway state classification and recovery command selection. * **Bug Fixes** * Correctly treats "Disconnected" gateway status as inactive and handles error/empty lines when listing sandboxes. * **Tests** * Expanded test coverage for edge cases, malformed input, and empty states. * **Refactor** * Runtime recovery helpers consolidated to a shared compiled module. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cv
added a commit
that referenced
this pull request
Apr 2, 2026
## Summary - Convert `bin/lib/local-inference.js` (228 lines) to `src/lib/local-inference.ts` - Typed interfaces: `ValidationResult`, `GpuInfo`, `RunCaptureFn` - 9 pure functions + 4 with DI-injected `runCapture` - Co-locate tests (30 tests) Stacked on #1240. 616 CLI tests pass. Coverage ratchet passes. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added local inference provider validation and connectivity diagnostics for vllm-local and ollama-local. * Implemented Ollama model discovery, warmup/probe commands, and intelligent model selection based on GPU memory. * Added health-check and model readiness checks to surface configuration or networking issues earlier. * **Tests** * Enhanced test coverage for provider validation, model detection, and unknown-provider handling; updated test targets accordingly. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cv
added a commit
that referenced
this pull request
Apr 2, 2026
## Summary - Convert `bin/lib/nim.js` (259 lines) to `src/lib/nim.ts` - Typed interfaces: `NimModel`, `GpuDetection`, `NimStatus` - 4 pure functions + subprocess-heavy I/O functions (docker, nvidia-smi) - Co-locate tests (16 tests) with updated mock pattern for dist/ paths Stacked on #1240. 615 CLI tests pass. Coverage ratchet passes. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * GPU detection with capability assessment for NIM containers * Container lifecycle management (start, stop, status monitoring) with automatic health checks * Model enumeration and image selection for launching containers * **Refactor** * Simplified module surface and improved internal architecture for maintainability * **Tests** * Updated test suite to align with the refactored module behavior and stronger typings <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cv
added a commit
that referenced
this pull request
Apr 2, 2026
## Summary - Convert `bin/lib/onboard-session.js` (440 lines) to `src/lib/onboard-session.ts` - Typed interfaces: `Session`, `StepState`, `SessionFailure`, `LockResult`, `SessionUpdates`, `LockInfo`, `SessionMetadata` - Full migration including fs I/O (session persistence, file locking) and pure helpers (redaction, validation, normalization) - Co-locate tests (14 tests) with updated cache-clearing for dist/ path Stacked on #1240. 615 CLI tests pass. Coverage ratchet passes. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Reorganized onboarding session management module structure and improved distribution of the codebase. Session tracking, step-level state management, lock handling, and sensitive data redaction functionality have been consolidated and optimized for better maintainability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cv
added a commit
that referenced
this pull request
Apr 2, 2026
## Summary - Convert `bin/lib/inference-config.js` (143 lines) to `src/lib/inference-config.ts` - Typed interfaces: `ProviderSelectionConfig`, `GatewayInference` - All 3 exports are pure — straightforward full conversion - Co-locate tests: `test/inference-config.test.js` → `src/lib/inference-config.test.ts` Stacked on #1240. 614 CLI tests pass. Coverage ratchet passes. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Centralized inference configuration, provider selection, and gateway output parsing into a single consolidated module for more consistent behavior. * **Tests** * Updated tests to match the consolidated module and improved assertion patterns. * **Chores** * Install/prep process now includes a CLI build step during project setup (silently handled). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
…ript modules (#1240) ## Summary - Extract ~210 lines of pure, side-effect-free functions from the 3,800-line `onboard.js` into **5 typed TypeScript modules** under `src/lib/`: - `gateway-state.ts` — gateway/sandbox state classification from openshell output - `validation.ts` — failure classification, API key validation, model ID checks - `url-utils.ts` — URL normalization, text compaction, env formatting - `build-context.ts` — Docker build context filtering, recovery hints - `dashboard.ts` — dashboard URL resolution and construction - Add **56 co-located unit tests** (`src/lib/*.test.ts`) for the extracted modules - Set up CLI TypeScript compilation: `tsconfig.src.json` compiles `src/` → `dist/` as CJS - `onboard.js` imports from compiled `dist/lib/` output — transparent to callers - Pre-commit hook updated to build TS and include `dist/lib/` in coverage These functions are **not touched by any #924 blocker PR** (#781, #782, #819, #672, #634, #890), so this extraction is safe to land immediately. ## Test plan - [x] 598 CLI tests pass (542 existing + 56 new) - [x] `tsc -p tsconfig.src.json` compiles cleanly - [x] `tsc -p tsconfig.cli.json` type-checks cleanly - [x] `tsc -p jsconfig.json` type-checks cleanly - [x] Coverage ratchet passes with `dist/lib/` included Closes #1237. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved sandbox-creation recovery hints and targeted remediation commands. * Smarter dashboard URL resolution and control-UI URL construction. * **Bug Fixes** * More accurate gateway and sandbox state detection. * Enhanced classification of validation/apply failures and safer model/key validation. * Better provider URL normalization and loopback handling. * **Tests** * Added comprehensive tests covering new utilities. * **Chores** * CLI now builds before CLI tests; CI/commit hooks updated to run the CLI build. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
## Summary - **install.sh missing build:cli**: PR #1240 replaced `bin/lib/` CJS modules with shims that re-export from `dist/lib/`, but `install.sh` never ran `npm run build:cli`. Every fresh install crashed on `nemoclaw onboard` with `Cannot find module '../../dist/lib/preflight'` - **E2E destroy verification**: The cleanup phase used `nemoclaw list` to verify a sandbox was gone, but `list` triggers gateway recovery which can restart a destroyed gateway and re-import the sandbox. Now checks the registry file directly instead - Uses `--if-present` so installs targeting older tags without `build:cli` don't break ## Test plan - [ ] Run `install.sh` on a clean VM and verify `nemoclaw onboard` no longer crashes - [ ] Verify `dist/lib/preflight.js` exists after install completes - [ ] Nightly E2E cloud-e2e passes the destroy verification phase --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
## Summary - Convert `bin/lib/runtime-recovery.js` (84 lines) to `src/lib/runtime-recovery.ts` - Typed `StateClassification` interface for sandbox/gateway state - 5 pure classifiers/parsers + 1 I/O function (`getRecoveryCommand`) - Co-locate tests with additional edge cases Stacked on #1240. 617 CLI tests pass. Coverage ratchet passes. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Improved runtime recovery logic to better parse CLI output and determine recovery actions, including more reliable sandbox/gateway state classification and recovery command selection. * **Bug Fixes** * Correctly treats "Disconnected" gateway status as inactive and handles error/empty lines when listing sandboxes. * **Tests** * Expanded test coverage for edge cases, malformed input, and empty states. * **Refactor** * Runtime recovery helpers consolidated to a shared compiled module. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
## Summary - Convert `bin/lib/local-inference.js` (228 lines) to `src/lib/local-inference.ts` - Typed interfaces: `ValidationResult`, `GpuInfo`, `RunCaptureFn` - 9 pure functions + 4 with DI-injected `runCapture` - Co-locate tests (30 tests) Stacked on #1240. 616 CLI tests pass. Coverage ratchet passes. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added local inference provider validation and connectivity diagnostics for vllm-local and ollama-local. * Implemented Ollama model discovery, warmup/probe commands, and intelligent model selection based on GPU memory. * Added health-check and model readiness checks to surface configuration or networking issues earlier. * **Tests** * Enhanced test coverage for provider validation, model detection, and unknown-provider handling; updated test targets accordingly. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
## Summary - Convert `bin/lib/nim.js` (259 lines) to `src/lib/nim.ts` - Typed interfaces: `NimModel`, `GpuDetection`, `NimStatus` - 4 pure functions + subprocess-heavy I/O functions (docker, nvidia-smi) - Co-locate tests (16 tests) with updated mock pattern for dist/ paths Stacked on #1240. 615 CLI tests pass. Coverage ratchet passes. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * GPU detection with capability assessment for NIM containers * Container lifecycle management (start, stop, status monitoring) with automatic health checks * Model enumeration and image selection for launching containers * **Refactor** * Simplified module surface and improved internal architecture for maintainability * **Tests** * Updated test suite to align with the refactored module behavior and stronger typings <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
## Summary - Convert `bin/lib/onboard-session.js` (440 lines) to `src/lib/onboard-session.ts` - Typed interfaces: `Session`, `StepState`, `SessionFailure`, `LockResult`, `SessionUpdates`, `LockInfo`, `SessionMetadata` - Full migration including fs I/O (session persistence, file locking) and pure helpers (redaction, validation, normalization) - Co-locate tests (14 tests) with updated cache-clearing for dist/ path Stacked on #1240. 615 CLI tests pass. Coverage ratchet passes. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Reorganized onboarding session management module structure and improved distribution of the codebase. Session tracking, step-level state management, lock handling, and sensitive data redaction functionality have been consolidated and optimized for better maintainability. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
laitingsheng
pushed a commit
that referenced
this pull request
Apr 2, 2026
## Summary - Convert `bin/lib/inference-config.js` (143 lines) to `src/lib/inference-config.ts` - Typed interfaces: `ProviderSelectionConfig`, `GatewayInference` - All 3 exports are pure — straightforward full conversion - Co-locate tests: `test/inference-config.test.js` → `src/lib/inference-config.test.ts` Stacked on #1240. 614 CLI tests pass. Coverage ratchet passes. Relates to #924 (shell consolidation). 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Centralized inference configuration, provider selection, and gateway output parsing into a single consolidated module for more consistent behavior. * **Tests** * Updated tests to match the consolidated module and improved assertion patterns. * **Chores** * Install/prep process now includes a CLI build step during project setup (silently handled). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 2, 2026
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
4 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
onboard.jsinto 5 typed TypeScript modules undersrc/lib/:gateway-state.ts— gateway/sandbox state classification from openshell outputvalidation.ts— failure classification, API key validation, model ID checksurl-utils.ts— URL normalization, text compaction, env formattingbuild-context.ts— Docker build context filtering, recovery hintsdashboard.ts— dashboard URL resolution and constructionsrc/lib/*.test.ts) for the extracted modulestsconfig.src.jsoncompilessrc/→dist/as CJSonboard.jsimports from compileddist/lib/output — transparent to callersdist/lib/in coverageThese functions are not touched by any #924 blocker PR (#781, #782, #819, #672, #634, #890), so this extraction is safe to land immediately.
Test plan
tsc -p tsconfig.src.jsoncompiles cleanlytsc -p tsconfig.cli.jsontype-checks cleanlytsc -p jsconfig.jsontype-checks cleanlydist/lib/includedCloses #1237. Relates to #924 (shell consolidation).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores