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:
📝 WalkthroughWalkthroughExtracted onboard CLI parsing and alias-deprecation logic into a new typed module Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI\n(bin/nemoclaw.js)"
participant OnboardCmd as "runOnboardCommand\n(src/lib/onboard-command.ts)"
participant AliasCmd as "runDeprecatedOnboardAliasCommand\n(src/lib/onboard-command.ts)"
participant Env as "process.env"
participant RunOnboard as "runOnboard\n(injected)"
participant Logger as "console.log / console.error"
CLI->>OnboardCmd: runOnboardCommand({ args, notice flags, env, runOnboard, ... })
OnboardCmd->>Env: read notice accept env var
OnboardCmd->>Logger: emit errors/usage and exit (if invalid or --help)
OnboardCmd->>RunOnboard: invoke with parsed OnboardCommandOptions
CLI->>AliasCmd: runDeprecatedOnboardAliasCommand({ kind, args, ... })
AliasCmd->>Logger: emit deprecation message
AliasCmd->>OnboardCmd: delegate to runOnboardCommand with same deps
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
🧹 Nitpick comments (2)
src/lib/onboard-command.test.ts (2)
69-87: Also assert that the alias still delegates.This test currently passes on log text alone; if
runDeprecatedOnboardAliasCommand()stopped invoking onboarding, the suite would still stay green.Suggested assertion
expect(lines.join("\n")).toContain("setup-spark` is deprecated"); expect(lines.join("\n")).toContain("Use `nemoclaw onboard` instead"); + expect(runOnboard).toHaveBeenCalledTimes(1); + expect(runOnboard).toHaveBeenCalledWith({ + nonInteractive: false, + resume: false, + acceptThirdPartySoftware: false, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-command.test.ts` around lines 69 - 87, The test only checks deprecation text but not that the alias delegates; update the test for runDeprecatedOnboardAliasCommand to also assert the provided runOnboard mock was invoked (e.g., verify vi.fn runOnboard was called exactly once and with expected parameters such as kind "setup-spark" and args []), so locate the runDeprecatedOnboardAliasCommand call and add assertions that runOnboard was called (and optionally that it received the same args/flags/env passed into runDeprecatedOnboardAliasCommand).
13-47: Please cover the invalid-arg branch too.The new parser’s exit/error path is now centralized behavior, but this suite only exercises successful parses. One case that asserts exit code
1and the usage text would close the main gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-command.test.ts` around lines 13 - 47, Add a test in src/lib/onboard-command.test.ts that exercises parseOnboardArgs' invalid-arg branch by calling parseOnboardArgs with an unknown flag (e.g., "--bad-flag") and injecting mocks for env, error, and exit: have error capture the printed message and have exit either throw or record the code so the test can assert exit was invoked with 1; finally assert the captured error output contains the usage/help text (e.g., "Usage" or the expected usage string). This targets the parseOnboardArgs invalid-path behavior to ensure exit(1) and usage text are produced when encountering an invalid argument.
🤖 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/onboard-command.test.ts`:
- Around line 69-87: The test only checks deprecation text but not that the
alias delegates; update the test for runDeprecatedOnboardAliasCommand to also
assert the provided runOnboard mock was invoked (e.g., verify vi.fn runOnboard
was called exactly once and with expected parameters such as kind "setup-spark"
and args []), so locate the runDeprecatedOnboardAliasCommand call and add
assertions that runOnboard was called (and optionally that it received the same
args/flags/env passed into runDeprecatedOnboardAliasCommand).
- Around line 13-47: Add a test in src/lib/onboard-command.test.ts that
exercises parseOnboardArgs' invalid-arg branch by calling parseOnboardArgs with
an unknown flag (e.g., "--bad-flag") and injecting mocks for env, error, and
exit: have error capture the printed message and have exit either throw or
record the code so the test can assert exit was invoked with 1; finally assert
the captured error output contains the usage/help text (e.g., "Usage" or the
expected usage string). This targets the parseOnboardArgs invalid-path behavior
to ensure exit(1) and usage text are produced when encountering an invalid
argument.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34fd5188-cbbb-48b8-b4ae-74e69d959f78
📒 Files selected for processing (4)
bin/nemoclaw.jssrc/lib/onboard-command.test.tssrc/lib/onboard-command.tstest/runner.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/nemoclaw.js (1)
795-833: Consider a small helper for the shared onboard dependency bag.
onboard,setup, andsetupSparknow repeat the samerequire("./lib/onboard")and injected fields. Pulling that into one helper would keep these wrappers truly thin and avoid drift when this dependency shape changes.♻️ Suggested cleanup
+function buildOnboardDeps(args) { + const { onboard: runOnboard } = require("./lib/onboard"); + return { + args, + noticeAcceptFlag: NOTICE_ACCEPT_FLAG, + noticeAcceptEnv: NOTICE_ACCEPT_ENV, + env: process.env, + runOnboard, + error: console.error, + exit: (code) => process.exit(code), + }; +} + async function onboard(args) { - const { onboard: runOnboard } = require("./lib/onboard"); - await runOnboardCommand({ - args, - noticeAcceptFlag: NOTICE_ACCEPT_FLAG, - noticeAcceptEnv: NOTICE_ACCEPT_ENV, - env: process.env, - runOnboard, - error: console.error, - exit: (code) => process.exit(code), - }); + await runOnboardCommand(buildOnboardDeps(args)); } async function setup(args = []) { - const { onboard: runOnboard } = require("./lib/onboard"); await runDeprecatedOnboardAliasCommand({ - kind: "setup", - args, - noticeAcceptFlag: NOTICE_ACCEPT_FLAG, - noticeAcceptEnv: NOTICE_ACCEPT_ENV, - env: process.env, - runOnboard, + ...buildOnboardDeps(args), + kind: "setup", log: console.log, - error: console.error, - exit: (code) => process.exit(code), }); } async function setupSpark(args = []) { - const { onboard: runOnboard } = require("./lib/onboard"); await runDeprecatedOnboardAliasCommand({ - kind: "setup-spark", - args, - noticeAcceptFlag: NOTICE_ACCEPT_FLAG, - noticeAcceptEnv: NOTICE_ACCEPT_ENV, - env: process.env, - runOnboard, + ...buildOnboardDeps(args), + kind: "setup-spark", log: console.log, - error: console.error, - exit: (code) => process.exit(code), }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/nemoclaw.js` around lines 795 - 833, Extract the repeated require("./lib/onboard") and the common dependency bag passed into runOnboardCommand / runDeprecatedOnboardAliasCommand into a small helper (e.g., getOnboardDeps) that returns { runOnboard, commonDeps } or the full args object; then update runOnboardCommand, setup, and setupSpark to call getOnboardDeps() and spread or pass the returned commonDeps along with their own kind/args values. Ensure the helper references NOTICE_ACCEPT_FLAG, NOTICE_ACCEPT_ENV and process.env so callers (runOnboardCommand, runDeprecatedOnboardAliasCommand) receive the same fields and avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/nemoclaw.js`:
- Around line 795-833: Extract the repeated require("./lib/onboard") and the
common dependency bag passed into runOnboardCommand /
runDeprecatedOnboardAliasCommand into a small helper (e.g., getOnboardDeps) that
returns { runOnboard, commonDeps } or the full args object; then update
runOnboardCommand, setup, and setupSpark to call getOnboardDeps() and spread or
pass the returned commonDeps along with their own kind/args values. Ensure the
helper references NOTICE_ACCEPT_FLAG, NOTICE_ACCEPT_ENV and process.env so
callers (runOnboardCommand, runDeprecatedOnboardAliasCommand) receive the same
fields and avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f37f7bae-4fe3-4c95-adba-7d1fd2af3317
📒 Files selected for processing (3)
bin/nemoclaw.jssrc/lib/onboard-command.test.tssrc/lib/onboard-command.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard-command.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard-command.test.ts (1)
131-149: Alias test should also coversetupbranch and assert delegation.This currently validates only
kind: "setup-spark"text and doesn’t assert that delegation happened. Add akind: "setup"test and verifyrunOnboardwas called.Suggested test additions
it("prints the setup-spark deprecation text before delegating", async () => { const lines: string[] = []; const runOnboard = vi.fn(async () => {}); await runDeprecatedOnboardAliasCommand({ kind: "setup-spark", @@ }); expect(lines.join("\n")).toContain("setup-spark` is deprecated"); expect(lines.join("\n")).toContain("Use `nemoclaw onboard` instead"); + expect(runOnboard).toHaveBeenCalled(); }); + + it("prints the setup deprecation text before delegating", async () => { + const lines: string[] = []; + const runOnboard = vi.fn(async () => {}); + await runDeprecatedOnboardAliasCommand({ + kind: "setup", + args: [], + noticeAcceptFlag: "--yes-i-accept-third-party-software", + noticeAcceptEnv: "NEMOCLAW_ACCEPT_THIRD_PARTY_SOFTWARE", + env: {}, + runOnboard, + log: (message = "") => lines.push(message), + error: () => {}, + exit: ((code: number) => { + throw new Error(String(code)); + }) as never, + }); + expect(lines.join("\n")).toContain("`nemoclaw setup` is deprecated"); + expect(lines.join("\n")).toContain("Use `nemoclaw onboard` instead"); + expect(runOnboard).toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard-command.test.ts` around lines 131 - 149, Add assertions and a new test for the "setup" alias: update the existing "setup-spark" test (calling runDeprecatedOnboardAliasCommand) to also assert the stubbed runOnboard was invoked (e.g., expect(runOnboard).toHaveBeenCalled()), and add a new it(...) block that calls runDeprecatedOnboardAliasCommand with kind: "setup" (same args, env, log, error, exit helpers) and asserts the deprecation message appears and that runOnboard was called/delegated to; reference runDeprecatedOnboardAliasCommand and the runOnboard mock to locate where to add these expectations.
🤖 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/onboard-command.test.ts`:
- Around line 131-149: Add assertions and a new test for the "setup" alias:
update the existing "setup-spark" test (calling
runDeprecatedOnboardAliasCommand) to also assert the stubbed runOnboard was
invoked (e.g., expect(runOnboard).toHaveBeenCalled()), and add a new it(...)
block that calls runDeprecatedOnboardAliasCommand with kind: "setup" (same args,
env, log, error, exit helpers) and asserts the deprecation message appears and
that runOnboard was called/delegated to; reference
runDeprecatedOnboardAliasCommand and the runOnboard mock to locate where to add
these expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 808aa471-60c7-42b3-8197-fd6611b1243b
📒 Files selected for processing (3)
src/lib/onboard-command.test.tssrc/lib/onboard-command.tstest/cli.test.js
✅ Files skipped from review due to trivial changes (1)
- test/cli.test.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
test/onboard.test.js (4)
1548-1562: Missing temp directory cleanup.The
tmpDircreated at line 1470 is not cleaned up in thefinallyblock. Other tests in this file (e.g., lines 853-862) clean up temp directories to avoid resource accumulation. Consider adding cleanup:sandboxCreateStream.streamSandboxCreate = originalStreamSandboxCreate; process.env.HOME = originalHome; process.env.PATH = originalPath; if (originalGateway === undefined) delete process.env.OPENSHELL_GATEWAY; else process.env.OPENSHELL_GATEWAY = originalGateway; if (originalNonInteractive === undefined) delete process.env.NEMOCLAW_NON_INTERACTIVE; else process.env.NEMOCLAW_NON_INTERACTIVE = originalNonInteractive; + fs.rmSync(tmpDir, { recursive: true, force: true }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 1548 - 1562, The tmpDir created earlier (tmpDir) is not being removed in the finally block; add cleanup there to mirror other tests (e.g., remove the temporary directory created at tmpDir) by checking if tmpDir is set and deleting it recursively (handle missing path or errors safely), using the same synchronous/asynchronous fs removal pattern used elsewhere in this test file so resources are not leaked; place this logic alongside the other environment and mock restorations in the finally block.
3124-3153: Missing temp directory cleanup.The test creates a
tmpDirandcustomBuildDirthat should be cleaned up after the test completes:+ try { const result = spawnSync(process.execPath, [scriptPath], { // ... }); // ... assertions ... + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 3124 - 3153, The test creates temporary directories tmpDir and customBuildDir but never removes them; update the test (in test/onboard.test.js around the spawnSync block and the surrounding test function) to remove these dirs in a finally/afterEach cleanup step (or use try/finally within the test) by calling fs.rmSync(tmpDir, { recursive: true, force: true }) and fs.rmSync(customBuildDir, { recursive: true, force: true }) (or fs.promises.rm with await if the test is async) so both tmpDir and customBuildDir are always deleted even on assertion failures.
1636-1658: Missing temp directory cleanup.Similar to the previous test,
tmpDirshould be cleaned up. Consider wrapping in try/finally:♻️ Suggested cleanup pattern
+ try { const result = spawnSync(process.execPath, [scriptPath], { // ... }); assert.equal(result.status, 0, result.stderr); const commands = JSON.parse(result.stdout.trim().split("\n").pop()); assert.ok( commands.some((entry) => entry.command.includes("'forward' 'start' '--background' '0.0.0.0:18789' 'my-assistant'"), ), "expected remote dashboard forward target", ); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 1636 - 1658, This test creates a temporary directory in the tmpDir variable but never removes it; wrap the invocation that calls spawnSync (the block starting with const result = spawnSync(...)) in a try/finally so that in the finally you synchronously remove tmpDir (e.g., fs.rmSync or fs.rmdirSync with recursive/force options) to ensure cleanup regardless of assertions (references: tmpDir, spawnSync, result, and the surrounding test function).
2376-2390: Missing temp directory cleanup - same pattern as other rewritten tests.Add
fs.rmSync(tmpDir, { recursive: true, force: true });at the end of the finally block for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.js` around lines 2376 - 2390, The finally block that restores globals (references: runner.run, registry.registerSandbox, preflight.checkPortAvailable, process.env variables) is missing cleanup of the temporary test directory variable tmpDir; add a call to fs.rmSync(tmpDir, { recursive: true, force: true }) as the last statement in that finally block to remove the temp directory, and ensure fs is required/imported in the test file if not already present.
🤖 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/onboard.test.js`:
- Around line 1548-1562: The tmpDir created earlier (tmpDir) is not being
removed in the finally block; add cleanup there to mirror other tests (e.g.,
remove the temporary directory created at tmpDir) by checking if tmpDir is set
and deleting it recursively (handle missing path or errors safely), using the
same synchronous/asynchronous fs removal pattern used elsewhere in this test
file so resources are not leaked; place this logic alongside the other
environment and mock restorations in the finally block.
- Around line 3124-3153: The test creates temporary directories tmpDir and
customBuildDir but never removes them; update the test (in test/onboard.test.js
around the spawnSync block and the surrounding test function) to remove these
dirs in a finally/afterEach cleanup step (or use try/finally within the test) by
calling fs.rmSync(tmpDir, { recursive: true, force: true }) and
fs.rmSync(customBuildDir, { recursive: true, force: true }) (or fs.promises.rm
with await if the test is async) so both tmpDir and customBuildDir are always
deleted even on assertion failures.
- Around line 1636-1658: This test creates a temporary directory in the tmpDir
variable but never removes it; wrap the invocation that calls spawnSync (the
block starting with const result = spawnSync(...)) in a try/finally so that in
the finally you synchronously remove tmpDir (e.g., fs.rmSync or fs.rmdirSync
with recursive/force options) to ensure cleanup regardless of assertions
(references: tmpDir, spawnSync, result, and the surrounding test function).
- Around line 2376-2390: The finally block that restores globals (references:
runner.run, registry.registerSandbox, preflight.checkPortAvailable, process.env
variables) is missing cleanup of the temporary test directory variable tmpDir;
add a call to fs.rmSync(tmpDir, { recursive: true, force: true }) as the last
statement in that finally block to remove the temp directory, and ensure fs is
required/imported in the test file if not already present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a55b3ed-760d-4dd6-878a-8b2dc7ee611a
📒 Files selected for processing (1)
test/onboard.test.js
…mmand # Conflicts: # test/onboard.test.js
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
1 participant
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
Extract the top-level
nemoclaw onboard,setup, andsetup-sparkwrappers frombin/nemoclaw.jsinto a typed module undersrc/lib/onboard-command.ts.Why
This continues the #924 follow-through by shrinking the monolithic dispatcher and moving simple command logic into reusable typed modules before the eventual CLI framework migration.
Changes
src/lib/onboard-command.tswith:parseOnboardArgs()runOnboardCommand()runDeprecatedOnboardAliasCommand()src/lib/onboard-command.test.tstest/runner.test.jsto match the extracted alias implementationbin/nemoclaw.jsto thin wrappers around the extracted handlersTesting
npm run build:clinpx vitest run src/lib/onboard-command.test.ts test/cli.test.js test/runner.test.jsRelates to #924.
Summary by CodeRabbit
Refactor
Tests