fix(cli): auto-detect sandbox name from registry in nemoclaw debug (#1728) - #1878
Conversation
📝 WalkthroughWalkthrough
Changes
Sequence Diagram(s)sequenceDiagram
actor DebugModule as Debug
participant Registry as Registry (listSandboxes)
participant Shell as Openshell CLI
Debug->>Registry: call listSandboxes()
alt registry returns defaultSandbox or sandbox name
Registry-->>Debug: defaultSandbox / first sandbox name
Debug-->>Debug: return sandbox name
else registry fails or returns no names
Registry--xDebug: throws / empty
Debug->>Shell: run "openshell sandbox list"
Shell-->>Debug: parsed sandbox name or empty
alt Shell returns name
Debug-->>Debug: return parsed name
else no name
Debug-->>Debug: return "default"
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/debug.ts (1)
153-155: Optional hardening: trim registry names before returning.A whitespace-only
defaultSandboxor sandboxnamefrom malformed config would currently pass through as a target value.Suggested hardening patch
- if (registry.defaultSandbox) return registry.defaultSandbox; - const names = registry.sandboxes.map((s) => s.name).filter(Boolean); - if (names.length > 0) return names[0]; + const defaultName = registry.defaultSandbox?.trim(); + if (defaultName) return defaultName; + const firstName = registry.sandboxes + .map((s) => s.name.trim()) + .find((name) => name.length > 0); + if (firstName) return firstName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/debug.ts` around lines 153 - 155, The registry selection logic should trim whitespace-only names before returning them: when checking registry.defaultSandbox, trim it and return only if the trimmed value is non-empty; when building names from registry.sandboxes (used in the names variable), map each s.name to its trimmed value and filter out empty/whitespace-only strings (e.g., filter by trimmed truthiness) so you return the first valid trimmed name. Update checks around registry.defaultSandbox and the names array generation to operate on trimmed strings.
🤖 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/debug.ts`:
- Around line 153-155: The registry selection logic should trim whitespace-only
names before returning them: when checking registry.defaultSandbox, trim it and
return only if the trimmed value is non-empty; when building names from
registry.sandboxes (used in the names variable), map each s.name to its trimmed
value and filter out empty/whitespace-only strings (e.g., filter by trimmed
truthiness) so you return the first valid trimmed name. Update checks around
registry.defaultSandbox and the names array generation to operate on trimmed
strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85fc1387-6de5-45c1-844c-1f2c7e2e0637
📒 Files selected for processing (1)
src/lib/debug.ts
detectSandboxName() fell back to the hardcoded string "default" when openshell sandbox list returned nothing or wasn't available. This meant `nemoclaw debug --quick` always targeted a sandbox named "default" even when the user's sandbox was named "my-assistant" or anything else — producing diagnostics for a non-existent sandbox. Check the local registry (sandboxes.json) first, which knows the user's defaultSandbox from onboard. Only fall back to the openshell probe and then "default" if the registry is empty or unreadable. Refs: NVIDIA#1728 Signed-off-by: ColinM-sys <cmcdonough@50words.com>
56a29d8 to
cab00a6
Compare
|
@ColinM-sys DCO check will fail on this one too — add this line to your PR description: Code looks good, ready to merge once DCO passes. |
|
Thank you so much — added the sign-off to the PR description here too! |
Summary
detectSandboxName()insrc/lib/debug.tsnow checks the local registry (sandboxes.json) fordefaultSandboxbefore falling back toopenshell sandbox listand then the hardcoded"default".Fixes #1728.
Why
nemoclaw debug --quickalways collected diagnostics for a sandbox named"default"becausedetectSandboxName()only triedopenshell sandbox list(which may return nothing or time out) and then fell back to the literal string"default". The user's actual sandbox (e.g."my-assistant") was ignored. The docs say sandbox name is "auto-detected" but the implementation never checked the authoritative source — the local registry.Test plan
npm run build:cli— compiles cleanly.nemoclaw debug --quickwith a sandbox named anything other than "default" — should now target the correct sandbox.Summary by CodeRabbit
Signed-off-by: ColinM-sys cmcdonough@50words.com