Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "nemoclaw CLI"
participant Registry as "Registry (~/.nemoclaw/sandboxes.json)"
participant Openshell as "openshell"
participant Gateway as "Gateway runtime"
User->>CLI: nemoclaw reconnect [name]
CLI->>Registry: resolveReconnectSandboxName(name)
alt name provided
Registry-->>CLI: requested name
else no name
Registry-->>CLI: defaultSandbox (or error)
end
alt gateway missing or unhealthy (including missing_named)
CLI->>Openshell: gateway select nemoclaw
CLI->>Openshell: gateway start --name nemoclaw
Openshell-->>Gateway: start request
Gateway-->>CLI: started/healthy
end
CLI->>Openshell: sandbox get <name>
Openshell-->>CLI: sandbox info/state
CLI->>Openshell: forward start --background 18789 <name>
Openshell-->>CLI: forward started
CLI->>Openshell: sandbox connect <name>
Openshell-->>User: interactive session
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/cli.test.js (1)
160-211: Add forwarding assertion here too for flow parity.This test validates
sandbox getandsandbox connect, but unlike the default-path test it does not assertforward start --background 18789 beta. Adding it will better guard reconnect regressions for explicit-name dispatch.✅ Suggested assertion
expect(r.code).toBe(0); expect(r.out.includes("Reconnecting to sandbox 'beta'")).toBeTruthy(); const log = fs.readFileSync(markerFile, "utf8"); expect(log).toContain("sandbox get beta"); + expect(log).toContain("forward start --background 18789 beta"); expect(log).toContain("sandbox connect beta");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` around lines 160 - 211, The test "reconnect accepts an explicit sandbox name" is missing the forwarding assertion present in the default-path test; after reading the markerFile (variable markerFile) add an assertion that the recorded command log includes the forward invocation for the named sandbox — specifically assert markerFile contents contain "forward start --background 18789 beta" (or at minimum that it contains "forward start" and "beta" together) to ensure the openshell script received the forwarding command when runWithEnv("reconnect beta") triggers sandbox connect; place this check alongside the existing expectations that assert "sandbox get beta" and "sandbox connect beta".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/nemoclaw.js`:
- Around line 582-586: reconnect currently just resolves sandboxName and calls
sandboxConnect, which can skip gateway auto-start because
recoverNamedGatewayRuntime handles only named_unhealthy, named_unreachable, and
connected_other but not missing_named; update the logic so that reconnect
triggers the recovery path for a missing named gateway: either call
recoverNamedGatewayRuntime(sandboxName) (or extend recoverNamedGatewayRuntime to
treat missing_named the same as the other cases) before invoking sandboxConnect,
and ensure the code paths for named_unhealthy, named_unreachable,
connected_other, and missing_named will attempt to auto-start the gateway so
reconnect does not leave the system without an active named gateway.
---
Nitpick comments:
In `@test/cli.test.js`:
- Around line 160-211: The test "reconnect accepts an explicit sandbox name" is
missing the forwarding assertion present in the default-path test; after reading
the markerFile (variable markerFile) add an assertion that the recorded command
log includes the forward invocation for the named sandbox — specifically assert
markerFile contents contain "forward start --background 18789 beta" (or at
minimum that it contains "forward start" and "beta" together) to ensure the
openshell script received the forwarding command when runWithEnv("reconnect
beta") triggers sandbox connect; place this check alongside the existing
expectations that assert "sandbox get beta" and "sandbox connect beta".
🪄 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: f93a3873-981a-4c35-8b40-3aa583486b78
📒 Files selected for processing (2)
bin/nemoclaw.jstest/cli.test.js
|
Addressed the current review in
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/cli.test.js (2)
152-157: Consider asserting command order, not only presence.These checks currently use
toContain, which won’t catch sequence regressions (e.g., connect before forward start). Since reconnect orchestration order matters, index-based order assertions would make these tests more robust.✅ Example order assertion pattern
const log = fs.readFileSync(markerFile, "utf8"); - expect(log).toContain("sandbox get alpha"); - expect(log).toContain("forward start --background 18789 alpha"); - expect(log).toContain("sandbox connect alpha"); + const iGet = log.indexOf("sandbox get alpha"); + const iForward = log.indexOf("forward start --background 18789 alpha"); + const iConnect = log.indexOf("sandbox connect alpha"); + expect(iGet).toBeGreaterThanOrEqual(0); + expect(iForward).toBeGreaterThan(iGet); + expect(iConnect).toBeGreaterThan(iForward);Apply the same pattern to the named and missing-gateway reconnect tests (including
gateway selectbeforegateway start).Also applies to: 206-211, 299-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` around lines 152 - 157, Replace the loose toContain assertions on markerFile/log in the reconnect tests with explicit order checks using indexOf: compute indices for the expected strings (e.g., "sandbox get alpha", "forward start --background 18789 alpha", "sandbox connect alpha") and assert indexOf(first) !== -1 and indexOf(first) < indexOf(second) < indexOf(third); do the same in the named and missing-gateway reconnect tests to assert "gateway select" appears before "gateway start" (and any other ordered steps), referencing the markerFile/log and the specific strings used in the existing expectations.
169-180: Strengthen explicit-name test to catch fallback regressions.Line 179 currently sets
defaultSandboxto"beta", the same value passed in Line 201. If reconnect accidentally ignores explicit args and always uses default, this test would still pass.🧪 Proposed test hardening
JSON.stringify({ sandboxes: { + alpha: { + name: "alpha", + model: "test-model", + provider: "nvidia-prod", + gpuEnabled: false, + policies: [], + }, beta: { name: "beta", model: "test-model", provider: "nvidia-prod", gpuEnabled: false, policies: [], }, }, - defaultSandbox: "beta", + defaultSandbox: "alpha", }),Also applies to: 201-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` around lines 169 - 180, The test currently uses the same value for defaultSandbox and the explicit sandbox arg, so update the fixture(s) used in the "explicit-name" test to use a different defaultSandbox value (e.g., set defaultSandbox to "alpha" while leaving the explicit arg/sandbox name as "beta") to ensure reconnect honors the explicit argument; update both the JSON fixture at the sandboxes block (the object containing defaultSandbox) and the similar fixture used around lines 201-211 so default and explicit names differ, preserving existing names like "beta" for the explicit input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/cli.test.js`:
- Around line 152-157: Replace the loose toContain assertions on markerFile/log
in the reconnect tests with explicit order checks using indexOf: compute indices
for the expected strings (e.g., "sandbox get alpha", "forward start --background
18789 alpha", "sandbox connect alpha") and assert indexOf(first) !== -1 and
indexOf(first) < indexOf(second) < indexOf(third); do the same in the named and
missing-gateway reconnect tests to assert "gateway select" appears before
"gateway start" (and any other ordered steps), referencing the markerFile/log
and the specific strings used in the existing expectations.
- Around line 169-180: The test currently uses the same value for defaultSandbox
and the explicit sandbox arg, so update the fixture(s) used in the
"explicit-name" test to use a different defaultSandbox value (e.g., set
defaultSandbox to "alpha" while leaving the explicit arg/sandbox name as "beta")
to ensure reconnect honors the explicit argument; update both the JSON fixture
at the sandboxes block (the object containing defaultSandbox) and the similar
fixture used around lines 201-211 so default and explicit names differ,
preserving existing names like "beta" for the explicit input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 224a0397-d031-4b60-9240-87c77422bf7e
📒 Files selected for processing (2)
bin/nemoclaw.jstest/cli.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- bin/nemoclaw.js
WuKongAI-CMU
force-pushed
the
fix/910-reconnect
branch
from
41b383b to
385e921
Compare
|
Rebased this branch onto the latest Changes in this pass:
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/cli.test.js (1)
106-323: Consider extracting a shared reconnect test fixture helper.The repeated temp-home/registry/stub setup is sizable; a small helper would reduce duplication and make future reconnect scenarios easier to add.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cli.test.js` around lines 106 - 323, Extract the repeated temp-home/registry/stub setup in the three reconnect tests into a helper (e.g., setupReconnectFixture) that creates HOME, localBin, registryDir, markerFile (and optionally stateFile), writes sandboxes.json and creates the openshell stub; have it accept parameters for sandbox list, defaultSandbox, openshell behavior, and initial state so tests can call setupReconnectFixture(...) and then call runWithEnv("reconnect ..." , { HOME, PATH: `${localBin}:${process.env.PATH || ""}` }, timeout). Update the tests to use this helper and assert using the returned markerFile and paths instead of duplicating fs.mkdirSync/fs.writeFileSync code; keep runWithEnv and the existing expectation checks unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/nemoclaw.js`:
- Around line 560-568: The reconnect argument validation is too permissive:
resolveReconnectSandboxName accepts any syntactically valid name and ignores
extra positional args, which can lead to confusing downstream errors; update
resolveReconnectSandboxName to (1) reject when more than one positional argument
is provided, logging a clear error and exiting, and (2) if an explicit name is
given, verify it exists in the local registry (via registry.has or equivalent)
before proceeding—if the name is unknown, log a specific "unknown sandbox" error
and exit; apply the same stricter checks to the corresponding reconnect handler
for connectors (the analogous function around the other block at lines 584-588).
---
Nitpick comments:
In `@test/cli.test.js`:
- Around line 106-323: Extract the repeated temp-home/registry/stub setup in the
three reconnect tests into a helper (e.g., setupReconnectFixture) that creates
HOME, localBin, registryDir, markerFile (and optionally stateFile), writes
sandboxes.json and creates the openshell stub; have it accept parameters for
sandbox list, defaultSandbox, openshell behavior, and initial state so tests can
call setupReconnectFixture(...) and then call runWithEnv("reconnect ..." , {
HOME, PATH: `${localBin}:${process.env.PATH || ""}` }, timeout). Update the
tests to use this helper and assert using the returned markerFile and paths
instead of duplicating fs.mkdirSync/fs.writeFileSync code; keep runWithEnv and
the existing expectation checks unchanged.
🪄 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: d14cff9c-ed68-4b7d-bd40-44971fc886ad
📒 Files selected for processing (2)
bin/nemoclaw.jstest/cli.test.js
|
Addressing the current reconnect validation review in d0bb41a:
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
1 participant
Summary
nemoclaw reconnect [name]command for post-reboot recoveryCloses #910
Summary by CodeRabbit
New Features
reconnectcommand to restore a sandbox connection; accepts an optional sandbox name and falls back to the configured default. Help updated to document usage and argument limits.Bug Fixes / Reliability
Tests