refactor(cli): extract sandbox doctor action - #2893
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
📝 WalkthroughWalkthroughThe ChangesSandbox Doctor Command Migration
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)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
## Summary Extract the sandbox status implementation from `src/nemoclaw.ts` into a dedicated action module. This removes `sandboxStatus` from the transitional runtime bridge while preserving the existing status output, gateway reconciliation, process health, and NIM reporting behavior. ## Stack Navigation - Position: 4 of 60 - Previous PR: [#2891 — refactor(cli): extract sandbox connect action](#2891) - Next PR: [#2893 — refactor(cli): extract sandbox doctor action](#2893) ## Changes - Added `src/lib/sandbox-status-action.ts` for sandbox status rendering, gateway lookup handling, local inference health, active session reporting, process health, and NIM status. - Updated `src/lib/sandbox-runtime-actions.ts` to call the extracted status action. - Removed `sandboxStatus` from `src/nemoclaw.ts` and `NemoClawRuntimeBridge`. ## 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 - [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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactoring** * Reorganized sandbox status command implementation for improved maintainability. * **Improvements** * Enhanced sandbox status reporting with comprehensive diagnostics including model information, provider details, inference status, GPU information, and policy checks. * Improved sandbox health verification with better error detection and guidance when state issues are encountered. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
LGTM — clean extraction of sandboxDoctor into src/lib/sandbox-doctor-action.ts with a thin oclif wrapper at src/lib/sandbox-doctor-cli-command.ts. Public surface preserved (<name> doctor [--json]), exit codes preserved, dispatch test added for the new oclif route. LegacyDispatch.target correctly narrowed.
Nits (non-blocking):
-
Orphan imports in
src/nemoclaw.ts— this extraction was the last consumer of six imports that are now unused; please clean up here or in a stack-end cleanup PR linked from this one:- line 7:
GATEWAY_PORT,OLLAMA_PORT(in destructure with the still-usedDASHBOARD_PORT) - line 46:
probeProviderHealth - line 47:
buildStatusCommandDeps - line 59:
recoverNamedGatewayRuntime(in destructure) - line 71:
isErrnoException
Plus the carryover
parseForwardListat line 79 from #2891 that's now drifted through two more PRs. - line 7:
-
Self-hosted e2e suite still in flight at review time (
build-sandbox-imagesin progress,test-e2e-sandbox/test-e2e-gateway-isolationqueued). Worth confirming green for this hash before merging — stack-tip green isn't enough since each link should stand alone.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/sandbox-doctor-action.ts (1)
429-430: ⚡ Quick winReplace the ESLint suppression with the repo’s Biome linting path.
// eslint-disable-next-line complexityis out of band for the JS/TS tooling this repo says to use, so this exception will not be enforced consistently. Use the Biome equivalent or splitrunSandboxDoctorenough to drop the suppression.As per coding guidelines, "Use Biome config in
biome.jsonfor linting and formatting JavaScript and TypeScript files".🤖 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/sandbox-doctor-action.ts` around lines 429 - 430, The file currently disables ESLint complexity for the function runSandboxDoctor via "// eslint-disable-next-line complexity"; remove that ESLint suppression and either (A) replace it with the repo's Biome directive (use the Biome inline comment/annotation supported by the project's biome configuration) or (B) refactor runSandboxDoctor to reduce complexity (extract logical blocks into smaller functions e.g., parseArgs, validateSandbox, performChecks) until the complexity rule is not violated; update or add any needed JSDoc/comments referencing runSandboxDoctor, parseArgs, validateSandbox or performChecks so linting passes under the Biome-based rules defined in biome.json.
🤖 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/sandbox-doctor-cli-command.ts`:
- Around line 10-19: The command is peeling this.argv which misparses positional
args/flags; instead declare oclif metadata on SandboxDoctorCliCommand: add
static args = [{ name: "sandboxName", required: true }] and static flags = {
json: Flags.boolean({ char: "j", description: "output JSON" }) } (and set static
strict = true), then in run() call const { args, flags } =
this.parse(SandboxDoctorCliCommand) and pass args.sandboxName to
runSandboxDoctor and include the json flag appropriately (e.g. await
runSandboxDoctor(args.sandboxName, flags.json ? ["--json"] : [] or merge with
any actionArgs) so flags are parsed/validated by oclif instead of reading
this.argv.
---
Nitpick comments:
In `@src/lib/sandbox-doctor-action.ts`:
- Around line 429-430: The file currently disables ESLint complexity for the
function runSandboxDoctor via "// eslint-disable-next-line complexity"; remove
that ESLint suppression and either (A) replace it with the repo's Biome
directive (use the Biome inline comment/annotation supported by the project's
biome configuration) or (B) refactor runSandboxDoctor to reduce complexity
(extract logical blocks into smaller functions e.g., parseArgs, validateSandbox,
performChecks) until the complexity rule is not violated; update or add any
needed JSDoc/comments referencing runSandboxDoctor, parseArgs, validateSandbox
or performChecks so linting passes under the Biome-based rules defined in
biome.json.
🪄 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: 2c3c7427-527a-48cb-8aa2-874f04d2ed3c
📒 Files selected for processing (6)
src/lib/legacy-oclif-dispatch.test.tssrc/lib/legacy-oclif-dispatch.tssrc/lib/oclif-commands.tssrc/lib/sandbox-doctor-action.tssrc/lib/sandbox-doctor-cli-command.tssrc/nemoclaw.ts
💤 Files with no reviewable changes (1)
- src/nemoclaw.ts
| function readProcessCommandLine(pid: number): string | null { | ||
| if (process.platform === "win32") { | ||
| return null; | ||
| } | ||
| try { | ||
| return fs.readFileSync(`/proc/${pid}/cmdline`, "utf-8"); | ||
| } catch { | ||
| try { | ||
| return execFileSync("ps", ["-p", String(pid), "-o", "comm=", "-o", "args="], { | ||
| encoding: "utf-8", | ||
| stdio: ["ignore", "pipe", "ignore"], | ||
| timeout: 1000, | ||
| }); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function isCloudflaredProcess(pid: number): boolean { | ||
| const commandLine = readProcessCommandLine(pid); | ||
| if (commandLine === null) { | ||
| return false; | ||
| } | ||
| return commandLineNamesCloudflared(commandLine); |
There was a problem hiding this comment.
Preserve “unknown” process identity instead of reporting a stale PID.
On Windows, Lines 285-287 always return null, and this helper turns that into false. That makes Line 326 classify any live cloudflared PID as stale even when process.kill(pid, 0) succeeds. Return a tri-state here or add a Windows-specific lookup so the doctor does not misreport healthy tunnels.
| export default class SandboxDoctorCliCommand extends Command { | ||
| static id = "sandbox:doctor"; | ||
| static strict = false; | ||
| static summary = "Diagnose sandbox and gateway health"; | ||
| static description = "Run host, gateway, sandbox, inference, messaging, and local service diagnostics."; | ||
| static usage = ["<name> doctor [--json]"]; | ||
|
|
||
| public async run(): Promise<void> { | ||
| const [sandboxName, ...actionArgs] = this.argv; | ||
| await runSandboxDoctor(sandboxName, actionArgs); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the target file exists and view its content
head -40 src/lib/sandbox-doctor-cli-command.tsRepository: NVIDIA/NemoClaw
Length of output: 864
🏁 Script executed:
# Find where runSandboxDoctor is defined to understand its signature
rg -n "runSandboxDoctor" --type tsRepository: NVIDIA/NemoClaw
Length of output: 388
🏁 Script executed:
# Check how oclif Command class is used elsewhere in the codebase for comparison
rg -n "extends Command" --type ts -A 5 | head -100Repository: NVIDIA/NemoClaw
Length of output: 7275
Declare the sandbox name and --json with oclif args/flags instead of peeling this.argv.
With static strict = false and no args/flags metadata, a direct sandbox:doctor invocation passes undefined as sandboxName (when called with no args), and a sandbox:doctor --json invocation binds "--json" as sandboxName. Declaring a required positional arg and a boolean flag through oclif gives built-in validation and help/usage errors.
🤖 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/sandbox-doctor-cli-command.ts` around lines 10 - 19, The command is
peeling this.argv which misparses positional args/flags; instead declare oclif
metadata on SandboxDoctorCliCommand: add static args = [{ name: "sandboxName",
required: true }] and static flags = { json: Flags.boolean({ char: "j",
description: "output JSON" }) } (and set static strict = true), then in run()
call const { args, flags } = this.parse(SandboxDoctorCliCommand) and pass
args.sandboxName to runSandboxDoctor and include the json flag appropriately
(e.g. await runSandboxDoctor(args.sandboxName, flags.json ? ["--json"] : [] or
merge with any actionArgs) so flags are parsed/validated by oclif instead of
reading this.argv.
## Summary Extract sandbox destroy and image cleanup helpers from `src/nemoclaw.ts` into a dedicated action module. This removes `sandboxDestroy` from the transitional runtime bridge while preserving sandbox deletion, messaging provider cleanup, gateway teardown, and Docker image cleanup behavior. ## Stack Navigation - Position: 6 of 60 - Previous PR: [#2893 — refactor(cli): extract sandbox doctor action](#2893) - Next PR: [#2896 — refactor(cli): extract sandbox rebuild action](#2896) ## Changes - Added `src/lib/sandbox-destroy-action.ts` for sandbox destroy orchestration, delete-result classification, gateway cleanup, service cleanup, and image cleanup helpers. - Updated `src/lib/sandbox-runtime-actions.ts` to call the extracted destroy action. - Removed `sandboxDestroy` and destroy-specific helpers from `src/nemoclaw.ts` and `NemoClawRuntimeBridge`. - Updated rebuild to use the extracted image/registry removal helper. - Reworked image cleanup tests to cover helper behavior instead of source-shape assertions against `src/nemoclaw.ts`. ## 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 - [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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved handling for sandbox deletion when the sandbox has already been removed. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
3 participants
Summary
Extract sandbox doctor diagnostics from
src/nemoclaw.tsinto a dedicated action module and route the command through oclif. This removes another legacy-dispatched public sandbox command while preserving the existing host, gateway, sandbox, inference, messaging, and local service diagnostics.Stack Navigation
Changes
src/lib/sandbox-doctor-action.tsfor doctor checks and report rendering.src/lib/sandbox-doctor-cli-command.tsand registeredsandbox:doctorin the oclif command map.doctorthrough oclif instead of the legacy target path.sandboxDoctorfromsrc/nemoclaw.ts.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
Release Notes
sandbox doctorcommand to diagnose sandbox and gateway health, running comprehensive checks across host, gateway, inference, messaging, and local services.--jsonflag for structured diagnostic output with status details and summary.