fix(onboard): validate custom Dockerfile path before onboarding

[nvshaxie]

Summary

• Validate nemoclaw onboard --from <Dockerfile> during command parsing when the path is missing.
• Exit before calling the onboarding flow so bad paths do not start preflight, gateway, inference, or sandbox setup side effects.
• Add regression coverage that asserts runOnboard is not called for a missing --from path.

Test plan

• npm run build:cli
• npm run typecheck:cli
• npm test -- src/lib/onboard-command.test.ts
• Manual Brev check: node ./bin/nemoclaw.js onboard --from /tmp/no-such-dockerfile-2589 --non-interactive --yes-i-accept-third-party-software exits immediately with --from path not found: before [1/8] Preflight checks

Fixes #2589

Made with Cursor

Summary by CodeRabbit

• Bug Fixes

  • Improved validation for the --from <Dockerfile> argument: provided paths are resolved to absolute locations, missing targets produce a clear error message and the command exits with a failure status.

• Tests

  • Added tests exercising real, temporary Dockerfile paths and the failure path when a referenced Dockerfile does not exist.

Signed-off-by: Julie Yaunches jyaunches@nvidia.com

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds early validation of the --from Dockerfile argument during argument parsing: the path is resolved and checked for existence; missing files now print an error and cause immediate process.exit(1). Tests updated to use real temp Dockerfiles and to assert fast-fail for nonexistent paths.

Changes

Cohort / File(s)	Summary
Argument validation src/lib/onboard-command.ts	Enhance --from parsing: resolve path and call fs.existsSync; on missing file print error with resolved path and process.exit(1) to fail fast before onboarding side effects.
Onboard CLI tests src/lib/onboard-command.test.ts	Use real temporary Dockerfile paths; update assertions to match resolved temp paths; add runOnboardCommand test that ensures a nonexistent --from causes immediate exit (exit:1) and prevents invoking the onboarding handler.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CLI
  participant Parser
  participant FS
  participant Onboard
  User->>CLI: nemoclaw onboard --from <path>
  CLI->>Parser: parseOnboardArgs(...)
  Parser->>FS: path.resolve(<path>) / existsSync(<resolved>)
  alt file exists
    Parser->>Onboard: runOnboard(...)
    Onboard->>Onboard: perform onboarding steps (gateway, provider, etc.)
    Onboard-->>User: progress / success
  else file missing
    Parser-->>User: print error "Custom Dockerfile not found: <resolved>"
    Parser->>CLI: process.exit(1)
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I sniffed the path with careful care,
Resolved each step so none despair.
If Dockerfile's missing, I'll loudly cry,
Exit quick — no gateways left to try.
Hops of joy for mistakes caught shy.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title accurately and concisely describes the main change: adding validation for the custom Dockerfile path before onboarding begins.
Linked Issues check	✅ Passed	The code changes fully address issue #2589 by validating the --from path at argument-parse time, exiting immediately on missing paths, and adding test coverage for the missing-Dockerfile scenario.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the objective: argument validation enhancements in onboard-command.ts and corresponding test coverage in onboard-command.test.ts with no unrelated modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Review rate limit: 9/10 reviews remaining, refill in 6 minutes.}

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/lib/onboard-command.test.ts (1)

117-120: Clean up temp artifacts created by tests.

Both tests create temp directories/files but never remove them. Please add try/finally cleanup (fs.rmSync(..., { recursive: true, force: true })) to avoid filesystem residue across repeated runs.

Also applies to: 203-204

🤖 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 117 - 120, Wrap the temp-dir
creation and file writes in the tests that use tmpDir and dockerfilePath inside
a try/finally so the temp artifacts are always removed; in the finally call
fs.rmSync(tmpDir, { recursive: true, force: true }) (and similarly for the other
test at the region around lines 203-204 referencing its temp dir) to ensure
cleanup even on failures, keeping the tmpDir and dockerfilePath variable names
to locate where to insert the try/finally blocks.

🤖 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/onboard-command.ts`:
- Around line 74-77: The current check only verifies existence of
requestedFromDockerfile (resolvedFromDockerfile) but allows directories; update
the validation in the onboard command so after resolving requestedFromDockerfile
you verify it's a regular file (e.g., using fs.statSync or fs.lstatSync and
stat.isFile()) and if not a file call error(`  --from path not found:
${resolvedFromDockerfile}`) and exit(1) (same failure path as before); locate
the logic around resolvedFromDockerfile / requestedFromDockerfile in
onboard-command.ts to implement this extra isFile check.

---

Nitpick comments:
In `@src/lib/onboard-command.test.ts`:
- Around line 117-120: Wrap the temp-dir creation and file writes in the tests
that use tmpDir and dockerfilePath inside a try/finally so the temp artifacts
are always removed; in the finally call fs.rmSync(tmpDir, { recursive: true,
force: true }) (and similarly for the other test at the region around lines
203-204 referencing its temp dir) to ensure cleanup even on failures, keeping
the tmpDir and dockerfilePath variable names to locate where to insert the
try/finally blocks.

🪄 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: Enterprise

Run ID: 8aac8018-a60b-4c0e-8e2f-2543fe7b7253

📥 Commits

Reviewing files that changed from the base of the PR and between 0b49851 and 3437db0.

📒 Files selected for processing (2)

• src/lib/onboard-command.test.ts
• src/lib/onboard-command.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Reject non-file --from paths during parsing.

Current validation only checks path existence, so directories can pass and still fail later during onboarding. That weakens the fast-fail guarantee for invalid Dockerfile inputs.

Suggested fix

-    if (!fs.existsSync(resolvedFromDockerfile)) {
+    if (!fs.existsSync(resolvedFromDockerfile) || !fs.statSync(resolvedFromDockerfile).isFile()) {
       error(`  --from path not found: ${resolvedFromDockerfile}`);
       exit(1);
     }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	const resolvedFromDockerfile = path.resolve(requestedFromDockerfile);
	if (!fs.existsSync(resolvedFromDockerfile)) {
	error(` --from path not found: ${resolvedFromDockerfile}`);
	exit(1);
	const resolvedFromDockerfile = path.resolve(requestedFromDockerfile);
	if (!fs.existsSync(resolvedFromDockerfile) || !fs.statSync(resolvedFromDockerfile).isFile()) {
	error(` --from path not found: ${resolvedFromDockerfile}`);
	exit(1);
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard-command.ts` around lines 74 - 77, The current check only
verifies existence of requestedFromDockerfile (resolvedFromDockerfile) but
allows directories; update the validation in the onboard command so after
resolving requestedFromDockerfile you verify it's a regular file (e.g., using
fs.statSync or fs.lstatSync and stat.isFile()) and if not a file call error(` 
--from path not found: ${resolvedFromDockerfile}`) and exit(1) (same failure
path as before); locate the logic around resolvedFromDockerfile /
requestedFromDockerfile in onboard-command.ts to implement this extra isFile
check.

[coderabbitai]

♻️ Duplicate comments (1)

src/lib/onboard-command.ts (1)

75-79: ⚠️ Potential issue | 🟠 Major

Reject non-file --from paths during parsing.

existsSync still lets directories through, so a bad --from can make it past parsing and fail later in onboarding. Please also require resolvedFromDockerfile to be a regular file before exiting early.

Suggested fix

-    if (!fs.existsSync(resolvedFromDockerfile)) {
+    if (!fs.existsSync(resolvedFromDockerfile) || !fs.statSync(resolvedFromDockerfile).isFile()) {
       error(`  --from path not found: ${resolvedFromDockerfile}`);
       exit(1);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard-command.ts` around lines 75 - 79, The check using
fs.existsSync allows directories; update the validation for
resolvedFromDockerfile (constructed via path.resolve(requestedFromDockerfile))
to ensure it exists and is a regular file before proceeding: after resolving,
replace or augment the existsSync check with a file stat (e.g., fs.statSync or
fs.lstatSync) and call .isFile() to verify it is a file, and if not call error(`
--from path not found: ${resolvedFromDockerfile}`) followed by exit(1); keep the
same error message and use the same symbols (resolvedFromDockerfile, error,
exit) so the early exit happens at parse time.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/lib/onboard-command.ts`:
- Around line 75-79: The check using fs.existsSync allows directories; update
the validation for resolvedFromDockerfile (constructed via
path.resolve(requestedFromDockerfile)) to ensure it exists and is a regular file
before proceeding: after resolving, replace or augment the existsSync check with
a file stat (e.g., fs.statSync or fs.lstatSync) and call .isFile() to verify it
is a file, and if not call error(`  --from path not found:
${resolvedFromDockerfile}`) followed by exit(1); keep the same error message and
use the same symbols (resolvedFromDockerfile, error, exit) so the early exit
happens at parse time.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 454eb415-bf43-46c9-b632-6ee240ae2458

📥 Commits

Reviewing files that changed from the base of the PR and between 3437db0 and a9ee1bc.

📒 Files selected for processing (2)

• src/lib/onboard-command.test.ts
• src/lib/onboard-command.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/lib/onboard-command.test.ts

[coderabbitai]

♻️ Duplicate comments (1)

src/lib/onboard-command.ts (1)

75-79: ⚠️ Potential issue | 🟠 Major

Reject non-file --from paths during parse-time validation.

At Line 76, existence-only validation still accepts directories. That breaks the “invalid Dockerfile path fails immediately” guarantee.

Suggested fix

-    if (!fs.existsSync(resolvedFromDockerfile)) {
+    if (!fs.existsSync(resolvedFromDockerfile) || !fs.statSync(resolvedFromDockerfile).isFile()) {
       error(`  --from path not found: ${resolvedFromDockerfile}`);
       exit(1);
     }

Use this read-only check to confirm current behavior and test coverage:

#!/bin/bash
set -euo pipefail

echo "Check parse-time --from validation logic:"
rg -n --type=ts 'resolvedFromDockerfile|existsSync|statSync|isFile|--from path not found' src/lib/onboard-command.ts

echo
echo "Check tests for directory-path rejection and fast-fail behavior:"
rg -n --type=ts 'path not found|--from|runOnboard|directory|isFile' src/lib/onboard-command.test.ts

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/onboard-command.ts` around lines 75 - 79, The current parse-time
validation only checks fs.existsSync(resolvedFromDockerfile) which allows
directories; replace the existence-only check with a file check by using
fs.statSync(resolvedFromDockerfile).isFile() (or lstatSync) and if it returns
false call error(...) and exit(1) to reject non-file --from paths; update any
related tests (onboard-command.test.ts) to assert directories are rejected and
maintain the same error message/fast-fail behavior used where
requestedFromDockerfile, resolvedFromDockerfile, error, and exit are referenced.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/lib/onboard-command.ts`:
- Around line 75-79: The current parse-time validation only checks
fs.existsSync(resolvedFromDockerfile) which allows directories; replace the
existence-only check with a file check by using
fs.statSync(resolvedFromDockerfile).isFile() (or lstatSync) and if it returns
false call error(...) and exit(1) to reject non-file --from paths; update any
related tests (onboard-command.test.ts) to assert directories are rejected and
maintain the same error message/fast-fail behavior used where
requestedFromDockerfile, resolvedFromDockerfile, error, and exit are referenced.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 9f6a9c9e-dbdc-4c42-a060-1052f3619033

📥 Commits

Reviewing files that changed from the base of the PR and between a9ee1bc and 5c99bc1.

📒 Files selected for processing (2)

• src/lib/onboard-command.test.ts
• src/lib/onboard-command.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/lib/onboard-command.test.ts

[jyaunches]

LGTM — clean, focused fix. Early validation of --from path prevents side effects. Tests are solid.