test(cli): lock in #1555 destroy-side session clear so list cannot resurrect destroyed sandboxes (#1641)#1677
Open
ColinM-sys wants to merge 1 commit intoNVIDIA:mainfrom
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughA Vitest regression test was added to verify Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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.
Actionable comments posted: 1
🤖 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/inventory-commands.test.ts`:
- Around line 23-58: The test currently stubs loadLastSession to { sandboxName:
null } so it never exercises the destroy codepath; instead create a shared
in-memory session/registry object and call the actual destroy flow before
invoking listSandboxesCommand so the test verifies the destroy logic clears the
session. Concretely: introduce a mutable shared object (e.g., const sharedState
= { session: { sandboxName: "X" }, registry: {...} }), wire your mocks passed to
listSandboxesCommand to read from sharedState (loadLastSession returns
sharedState.session, recoverRegistryEntries reads sharedState.registry), then
import and invoke the real destroy routine used in tests (the project’s destroy
command function) to mutate sharedState (clearing session.sandboxName) and
finally call listSandboxesCommand to assert the onboarding hint and absence of
recovery messages.
🪄 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: 9cd0f27d-fcfc-493f-bfc9-3f5cfba9ee32
📒 Files selected for processing (1)
src/lib/inventory-commands.test.ts
Comment on lines
+23
to
+58
| it("regression #1641: after destroy clears the session, list shows the onboard hint instead of resurrecting the sandbox", async () => { | ||
| // Reproduces the loop the reporter described in #1641: a sandbox | ||
| // is destroyed, registry is empty, the destroy code clears | ||
| // session.sandboxName to null (added in #1555), and the next | ||
| // `nemoclaw list` MUST NOT recover the destroyed sandbox from | ||
| // the session. Without the destroy-side clear, list would print | ||
| // "Recovered sandbox inventory from the last onboard session" | ||
| // and the user would loop between `list` (showing the sandbox) | ||
| // and `connect` (removing it as stale). | ||
| const lines: string[] = []; | ||
| await listSandboxesCommand({ | ||
| recoverRegistryEntries: async () => ({ | ||
| sandboxes: [], | ||
| defaultSandbox: null, | ||
| recoveredFromSession: false, | ||
| recoveredFromGateway: 0, | ||
| }), | ||
| getLiveInference: () => null, | ||
| // Post-destroy session: sandboxName has been cleared. The | ||
| // session object itself still exists (other fields populated) | ||
| // but loadLastSession returning a session with sandboxName=null | ||
| // must NOT trip the "last onboarded sandbox was 'X'" hint. | ||
| loadLastSession: () => ({ sandboxName: null }), | ||
| log: (message = "") => lines.push(message), | ||
| }); | ||
|
|
||
| expect(lines).toContain( | ||
| " No sandboxes registered. Run `nemoclaw onboard` to get started.", | ||
| ); | ||
| expect( | ||
| lines.some((line) => | ||
| line.includes("Recovered sandbox inventory from the last onboard session"), | ||
| ), | ||
| ).toBe(false); | ||
| expect(lines.some((line) => line.includes("last onboarded sandbox was"))).toBe(false); | ||
| }); |
Contributor
There was a problem hiding this comment.
Regression scope is too narrow to catch destroy-path regressions.
Line 33–46 hardcodes the post-destroy state (loadLastSession already returns { sandboxName: null }), so this test only verifies list rendering. If destroy stops clearing the session again, this test will still pass because destroy is never exercised. Please add/convert this to a flow that performs destroy and then list against shared state so #1555 is actually pinned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/inventory-commands.test.ts` around lines 23 - 58, The test currently
stubs loadLastSession to { sandboxName: null } so it never exercises the destroy
codepath; instead create a shared in-memory session/registry object and call the
actual destroy flow before invoking listSandboxesCommand so the test verifies
the destroy logic clears the session. Concretely: introduce a mutable shared
object (e.g., const sharedState = { session: { sandboxName: "X" }, registry:
{...} }), wire your mocks passed to listSandboxesCommand to read from
sharedState (loadLastSession returns sharedState.session, recoverRegistryEntries
reads sharedState.registry), then import and invoke the real destroy routine
used in tests (the project’s destroy command function) to mutate sharedState
(clearing session.sandboxName) and finally call listSandboxesCommand to assert
the onboarding hint and absence of recovery messages.
wscurran
added
NemoClaw CLI
Contributor
|
✨ Thanks for submitting this PR, which proposes a way to add a regression test for the destroy-side session clear and may help prevent similar bugs in the future. Possibly related open PRs: Possibly related open issues: |
wscurran
added
the
enhancement: testing
ColinM-sys
force-pushed
the
fix/1641-list-no-resurrect-test
branch
2 times, most recently
from
1493057 to
27df867
Compare
This was referenced Apr 15, 2026
ColinM-sys
force-pushed
the
fix/1641-list-no-resurrect-test
branch
from
27df867 to
ae710b5
Compare
3 tasks
wscurran
added
status: rebase
ColinM-sys
force-pushed
the
fix/1641-list-no-resurrect-test
branch
from
ae710b5 to
13c625f
Compare
…not resurrect destroyed sandboxes NVIDIA#1641 reported that after `nemoclaw <name> destroy`, `nemoclaw list` would print "Recovered sandbox inventory from the last onboard session" and re-show the destroyed sandbox, then `nemoclaw <name> connect` would mark it stale and remove it again — looping forever between the two commands. The reporter was on v0.0.6-19-gea9a6f2, which is 13 commits behind ecc1277 (PR NVIDIA#1555), the commit that added `s.sandboxName = null` to `sandboxDestroy()`. So the bug is already fixed on main, but there is no regression test pinning the behavior, which means a future refactor of the destroy or list path could silently re-introduce the loop. Add a regression test in src/lib/inventory-commands.test.ts under "NVIDIA#1641" that exercises the post-destroy state explicitly: registry empty, session present but with sandboxName cleared. Assert that list prints the onboard hint and does NOT print either of the recovery / "last onboarded sandbox was" messages that signal the loop has come back. Refs: NVIDIA#1641, NVIDIA#1555 Signed-off-by: ColinM-sys <cmcdonough@50words.com>
ColinM-sys
force-pushed
the
fix/1641-list-no-resurrect-test
branch
from
13c625f to
781686c
Compare
wscurran
added
the
status: rfr
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
2 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
Refs #1641, #1555.
Why
#1641 reported that after `nemoclaw destroy`, `nemoclaw list` re-recovered the destroyed sandbox from the onboard session, and `nemoclaw connect` then marked it stale and removed it — looping forever between the two commands. The reporter ran on `v0.0.6-19-gea9a6f2`, which is 13 commits behind `ecc1277d` (PR #1555). #1555 added the destroy-side `session.sandboxName = null` clear that breaks the loop. So the bug is already fixed on `main`, but there is no regression test pinning the behavior, which means a future refactor of either the destroy path or the list/recovery path could silently re-introduce it.
What changed
Test plan
Reporter follow-up
Suggesting on the issue that the reporter retest on current `main` (post-#1555). If the loop still reproduces on a current build, that's a separate bug and I'll re-investigate.
Summary by CodeRabbit
Signed-off-by: ColinM-sys cmcdonough@50words.com