fix(onboard): persist model and provider to registry after sandbox creation#1884
Conversation
…eation
registry.updateSandbox() silently no-ops when the entry does not exist yet.
The call at line 3643 (inference setup, Step 4) ran before registerSandbox()
(Step 6: create sandbox), so model and provider were never persisted.
Fix: add a registry.updateSandbox(sandboxName, { model, provider }) call
immediately after createSandbox() returns, at which point the registry
entry is guaranteed to exist.
This makes `nemoclaw list` show the correct model and provider instead of
`model: unknown provider: unknown`.
Fixes NVIDIA#1881
Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughA one-line fix in the onboard flow calls Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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.
Pull request overview
Fixes a registry persistence bug in the onboarding flow where the selected inference model and provider were not being saved for newly created sandboxes, causing nemoclaw list to display unknown.
Changes:
- Persist
modelandproviderto the sandbox registry immediately aftercreateSandbox()returns (i.e., after the registry entry is guaranteed to exist).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Persist model and provider after the sandbox entry exists in the registry. | ||
| // updateSandbox() silently no-ops when the entry is missing, so this must | ||
| // run after createSandbox() / registerSandbox() — not before. Fixes #1881. | ||
| registry.updateSandbox(sandboxName, { model, provider }); | ||
| onboardSession.markStepComplete("sandbox", { sandboxName, provider, model, nimContainer }); |
There was a problem hiding this comment.
Add a regression test to ensure model/provider are persisted after sandbox creation. Since this bug was caused by calling registry.updateSandbox before registry.registerSandbox existed, a unit test (similar to the existing source-order regex assertions in test/onboard.test.ts) should assert that the onboard flow calls registry.updateSandbox(sandboxName, { model, provider }) after createSandbox(...) returns, so future refactors don’t reintroduce the silent no-op behavior.
…sistence
Assert that registry.updateSandbox(sandboxName, { model, provider })
appears in source AFTER createSandbox() to prevent reintroduction of
the bug where updateSandbox was called before registerSandbox().
Per Copilot review on NVIDIA#1884.
Signed-off-by: Benedikt Schackenberg <6381261+BenediktSchackenberg@users.noreply.github.com>
|
Added a source-order regression test in |
Problem
After
nemoclaw onboard,nemoclaw listshowsmodel: unknown provider: unknownfor all sandboxes. The registry hasmodel: nullandprovider: nulleven though onboard correctly displayed the configured values.Root cause
registry.updateSandbox(sandboxName, { model, provider })was called at line 3643 (Step 4: inference setup) — beforeregistry.registerSandbox()at line 2827 (Step 6: create sandbox). SinceupdateSandbox()silently returnsfalsewhen the entry doesn't exist (registry.ts:177), the values were never persisted.Fix
Added a
registry.updateSandbox(sandboxName, { model, provider })call immediately aftercreateSandbox()returns. At that point the registry entry is guaranteed to exist.Testing
nemoclaw onboard→nemoclaw listnow shows the correct model and provider~/.nemoclaw/sandboxes.jsonstores the configured values instead ofnullFixes #1881
Signed-off-by: Benedikt Schackenberg 6381261+BenediktSchackenberg@users.noreply.github.com
Summary by CodeRabbit
Bug Fixes
Tests