feat(onboard): add --from <Dockerfile> option for custom sandbox images#1059
Open
adityanjothi wants to merge 7 commits intoNVIDIA:mainfrom
Open
feat(onboard): add --from <Dockerfile> option for custom sandbox images#1059adityanjothi wants to merge 7 commits intoNVIDIA:mainfrom
adityanjothi wants to merge 7 commits intoNVIDIA:mainfrom
Conversation
Accept an optional --from <path> flag on `nemoclaw onboard` so callers can supply a custom Dockerfile instead of the stock NemoClaw image. - CLI: pre-pass extracts --from before the unknown-arg validator - createSandbox: new 6th param; custom path copies parent dir via the existing copyBuildContextDir filter (excludes node_modules, .git, etc.) - onboard: reads fromDockerfile from opts or NEMOCLAW_FROM_DOCKERFILE env var (non-interactive); records it in session.metadata for resume safety - getResumeConfigConflicts: flags --resume with a different --from path - Tests: conflict detection, build context staging, missing-path exit
feat(onboard): add --from <Dockerfile> option for custom sandbox images
Contributor
📝 WalkthroughWalkthroughAdded an optional --from / NEMOCLAW_FROM_DOCKERFILE flow to nemoclaw onboard: CLI parsing, propagation into onboard(), resume conflict detection against session.metadata.fromDockerfile, staging a custom Dockerfile’s parent directory as the build context, and tests for happy/error paths. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as nemoclaw.js
participant Onboard as onboard()
participant Sandbox as createSandbox()
participant FS as FileSystem
participant Docker as Docker Build
User->>CLI: nemoclaw onboard --from custom.Dockerfile
CLI->>CLI: Extract & validate `--from` (or read NEMOCLAW_FROM_DOCKERFILE)
CLI->>Onboard: runOnboard(opts with fromDockerfile)
Onboard->>Onboard: Resolve fromDockerfile\nCheck session.metadata.fromDockerfile
alt Resume conflict
Onboard->>User: Error (fromDockerfile mismatch)
else No conflict
Onboard->>Sandbox: createSandbox(..., fromDockerfile)
Sandbox->>FS: Verify custom Dockerfile exists
Sandbox->>FS: Copy parent directory into staged build context
Sandbox->>FS: Copy/rename file to staged `Dockerfile` if needed
Sandbox->>Docker: Build image using staged context
Docker-->>Sandbox: Build result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
adityanjothi
changed the title
![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: 4
🧹 Nitpick comments (1)
docs/reference/commands.md (1)
61-61: Switch these sentences to active voice.The changed prose uses passive constructions like “is used”, “can also be supplied”, and “is attempted”.
As per coding guidelines "Active voice required. Flag passive constructions."
Also applies to: 70-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/commands.md` at line 61, Rewrite the passive sentences in the docs text to active voice: change "The entire parent directory of the specified file is used as the Docker build context" to something like "Docker uses the entire parent directory of the specified file as the build context," change "any files your Dockerfile references (scripts, config, etc.) must live alongside it" to an active-form sentence ("Place any files your Dockerfile references ... alongside it"), and similarly convert other passive phrases such as "can also be supplied" and "is attempted" in the same section (and lines 70–76) into active constructions that name the actor (e.g., "You can also supply..." or "Docker attempts...") so the prose follows the Active voice required guideline.
🤖 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/lib/onboard.js`:
- Around line 2807-2812: The current fromDockerfile value is only taken from the
current CLI/env so resuming can drop the originally recorded Dockerfile; update
the logic that computes fromDockerfile to: if opts.fromDockerfile is set use
that, else if resume is true and the onboardSession has a stored dockerfile path
use that stored value, and only then fall back to the env/non-interactive
branch; when persisting the dockerfile into session metadata (where the code
currently writes the original value) convert the path to an absolute path (e.g.,
via path.resolve) so relative CWD changes on resume still resolve correctly;
adjust uses of fromDockerfile (the variable passed into acquireOnboardLock and
later rebuild logic) to rely on this merged/normalized value.
- Around line 1327-1335: The current conflict check in onboard.js only flags
when both requestedFrom and recordedFrom are non-null; update the logic for the
fromDockerfile conflict so that any mismatch (one null and the other non-null OR
different resolved paths) is treated as a conflict: use the existing variables
requestedFrom and recordedFrom to compare and push to conflicts whenever
requestedFrom !== recordedFrom (including when one is null). Also update the
resume/error rendering for the fromDockerfile conflict to display the null case
as “stock image” so messages read clearly when one side is missing (reference
the session.metadata.fromDockerfile and the conflicts entry with field
"fromDockerfile").
- Around line 1776-1789: The current check only verifies existence so passing a
directory for --from leads to an uncaught EISDIR in fs.copyFileSync; update the
fromDockerfile handling (the fromResolved branch used with copyBuildContextDir,
stagedDockerfile and fs.copyFileSync) to verify that fromResolved is a regular
file (use fs.statSync or fs.lstatSync and isFile()) before attempting to copy;
if it is not a file, print a clear error like "Custom Dockerfile is not a file:
<path>" and exit(1) so the friendly CLI error path runs and temp build context
is cleaned up.
In `@docs/reference/commands.md`:
- Line 67: The docs claim the Dockerfile "must be named `Dockerfile`" but the
implementation in bin/lib/onboard.js remaps other basenames into
buildCtx/Dockerfile; update docs/reference/commands.md to reflect that any
Dockerfile basename is accepted and will be copied/renamed into
buildCtx/Dockerfile by onboard.js (referencing bin/lib/onboard.js and its
remapping behavior) — replace the restrictive sentence with one that explains
the remapping behavior and how the file will be used.
---
Nitpick comments:
In `@docs/reference/commands.md`:
- Line 61: Rewrite the passive sentences in the docs text to active voice:
change "The entire parent directory of the specified file is used as the Docker
build context" to something like "Docker uses the entire parent directory of the
specified file as the build context," change "any files your Dockerfile
references (scripts, config, etc.) must live alongside it" to an active-form
sentence ("Place any files your Dockerfile references ... alongside it"), and
similarly convert other passive phrases such as "can also be supplied" and "is
attempted" in the same section (and lines 70–76) into active constructions that
name the actor (e.g., "You can also supply..." or "Docker attempts...") so the
prose follows the Active voice required guideline.
🪄 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: de8c19bc-e151-4760-8134-2eee8061febc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
bin/lib/onboard.jsbin/nemoclaw.jsdocs/reference/commands.mdtest/onboard.test.js
Comment on lines
+1776
to
+1789
| if (fromDockerfile) { | ||
| const fromResolved = path.resolve(fromDockerfile); | ||
| if (!fs.existsSync(fromResolved)) { | ||
| console.error(` Custom Dockerfile not found: ${fromResolved}`); | ||
| process.exit(1); | ||
| } | ||
| // Copy the entire parent directory as build context. copyBuildContextDir | ||
| // already filters out node_modules, .git, .venv, __pycache__, etc. | ||
| copyBuildContextDir(path.dirname(fromResolved), buildCtx); | ||
| // If the caller pointed at a file not named "Dockerfile", copy it to the | ||
| // location openshell expects (buildCtx/Dockerfile). | ||
| if (path.basename(fromResolved) !== "Dockerfile") { | ||
| fs.copyFileSync(fromResolved, stagedDockerfile); | ||
| } |
Contributor
There was a problem hiding this comment.
Validate that --from points to a regular file.
The new check only guards existence. Passing a directory here falls through to fs.copyFileSync() and throws an uncaught EISDIR, which skips the friendly CLI error path and leaves the temp build context behind.
🛠 Suggested validation
if (!fs.existsSync(fromResolved)) {
console.error(` Custom Dockerfile not found: ${fromResolved}`);
process.exit(1);
}
+ if (!fs.statSync(fromResolved).isFile()) {
+ console.error(` Custom Dockerfile must be a file: ${fromResolved}`);
+ process.exit(1);
+ }
// Copy the entire parent directory as build context. copyBuildContextDir📝 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
| if (fromDockerfile) { | |
| const fromResolved = path.resolve(fromDockerfile); | |
| if (!fs.existsSync(fromResolved)) { | |
| console.error(` Custom Dockerfile not found: ${fromResolved}`); | |
| process.exit(1); | |
| } | |
| // Copy the entire parent directory as build context. copyBuildContextDir | |
| // already filters out node_modules, .git, .venv, __pycache__, etc. | |
| copyBuildContextDir(path.dirname(fromResolved), buildCtx); | |
| // If the caller pointed at a file not named "Dockerfile", copy it to the | |
| // location openshell expects (buildCtx/Dockerfile). | |
| if (path.basename(fromResolved) !== "Dockerfile") { | |
| fs.copyFileSync(fromResolved, stagedDockerfile); | |
| } | |
| if (fromDockerfile) { | |
| const fromResolved = path.resolve(fromDockerfile); | |
| if (!fs.existsSync(fromResolved)) { | |
| console.error(` Custom Dockerfile not found: ${fromResolved}`); | |
| process.exit(1); | |
| } | |
| if (!fs.statSync(fromResolved).isFile()) { | |
| console.error(` Custom Dockerfile must be a file: ${fromResolved}`); | |
| process.exit(1); | |
| } | |
| // Copy the entire parent directory as build context. copyBuildContextDir | |
| // already filters out node_modules, .git, .venv, __pycache__, etc. | |
| copyBuildContextDir(path.dirname(fromResolved), buildCtx); | |
| // If the caller pointed at a file not named "Dockerfile", copy it to the | |
| // location openshell expects (buildCtx/Dockerfile). | |
| if (path.basename(fromResolved) !== "Dockerfile") { | |
| fs.copyFileSync(fromResolved, stagedDockerfile); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/onboard.js` around lines 1776 - 1789, The current check only verifies
existence so passing a directory for --from leads to an uncaught EISDIR in
fs.copyFileSync; update the fromDockerfile handling (the fromResolved branch
used with copyBuildContextDir, stagedDockerfile and fs.copyFileSync) to verify
that fromResolved is a regular file (use fs.statSync or fs.lstatSync and
isFile()) before attempting to copy; if it is not a file, print a clear error
like "Custom Dockerfile is not a file: <path>" and exit(1) so the friendly CLI
error path runs and temp build context is cleaned up.
…andling - Broaden fromDockerfile conflict detection to catch one-null mismatches (stock→custom and custom→stock) not just different non-null paths - Replace null label substitution with purpose-built messages for each mismatch case so users know exactly what to do to resolve the conflict - On --resume, fall back to the session-recorded fromDockerfile when --from is omitted so the original image is reused automatically - Store fromDockerfile as an absolute path in session metadata so relative paths survive CWD changes between runs - Update docs: any filename is accepted, not just 'Dockerfile'
fix(onboard): address review feedback on --from resume and conflict h…
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 `@bin/lib/onboard.js`:
- Around line 2886-2891: The createSession function currently only preserves
gatewayName from overrides.metadata, causing fromDockerfile to be lost when
saving sessions; modify createSession to include fromDockerfile in the returned
metadata object (use overrides.metadata?.fromDockerfile || null) alongside
gatewayName, and update filterSafeUpdates so it does not strip fromDockerfile
(ensure metadata filtering preserves metadata.gatewayName and
metadata.fromDockerfile) so subsequent updateSession calls retain the
fromDockerfile value.
🪄 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: 08732457-737c-4eb7-91ac-293393932827
📒 Files selected for processing (3)
bin/lib/onboard.jsdocs/reference/commands.mdtest/onboard.test.js
✅ Files skipped from review due to trivial changes (1)
- docs/reference/commands.md
- createSession now includes fromDockerfile in metadata alongside gatewayName via a buildSessionMetadata helper (keeps createSession within complexity limit) - filterSafeUpdates preserves metadata.fromDockerfile so updateSession calls do not silently drop the recorded Dockerfile path
fix(onboard): persist fromDockerfile through session create and update
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard-session.js (1)
373-378: Consider adding type validation forfromDockerfile.The
gatewayNamefield is validated as a string (line 373), butfromDockerfileis not type-checked before being stored. This could allow non-string truthy values to pass through. For consistency and defensive coding, consider validating the type:♻️ Suggested change
if (isObject(updates.metadata) && typeof updates.metadata.gatewayName === "string") { safe.metadata = { gatewayName: updates.metadata.gatewayName, - fromDockerfile: updates.metadata.fromDockerfile || null, + fromDockerfile: typeof updates.metadata.fromDockerfile === "string" ? updates.metadata.fromDockerfile : null, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard-session.js` around lines 373 - 378, The metadata assignment allows non-string values for fromDockerfile; update the block that sets safe.metadata (checking isObject(updates.metadata) and typeof updates.metadata.gatewayName === "string") to also validate updates.metadata.fromDockerfile is a string before assigning it, otherwise set fromDockerfile to null — i.e., use the existing safe.metadata/gatewayName logic and apply a typeof check for fromDockerfile so only string values are stored.
🤖 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/lib/onboard-session.js`:
- Around line 373-378: The metadata assignment allows non-string values for
fromDockerfile; update the block that sets safe.metadata (checking
isObject(updates.metadata) and typeof updates.metadata.gatewayName === "string")
to also validate updates.metadata.fromDockerfile is a string before assigning
it, otherwise set fromDockerfile to null — i.e., use the existing
safe.metadata/gatewayName logic and apply a typeof check for fromDockerfile so
only string values are stored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e51e24c0-487a-4950-8185-b245e8537c06
📒 Files selected for processing (1)
bin/lib/onboard-session.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
Adds a
--from <Dockerfile>flag tonemoclaw onboardso callers can build the sandbox image from a custom Dockerfile instead of the stock NemoClaw image. The entire parent directory of the specified file is used as the Docker build context. All NemoClaw build arguments are still injected at build time, so the flag works with every inference provider and integrates safely with--resume.Changes
bin/nemoclaw.js): pre-pass extracts--from <path>before the unknown-arg validator; passesfromDockerfiletorunOnboard()createSandbox(bin/lib/onboard.js): new 6th paramfromDockerfile; when set, copies the Dockerfile's parent directory via the existingcopyBuildContextDirfilter (excludes
node_modules,.git,.venv, etc.) instead of staging the stock imageonboard(): readsfromDockerfilefromoptsorNEMOCLAW_FROM_DOCKERFILEenv var (non-interactive/CI); records it insession.metadatafor resume trackinggetResumeConfigConflicts(): flags--resumecombined with a different--frompath than the original session recordeddocs/reference/commands.md): documents the flag, env var, build context behaviour, and resume conflict behaviourType of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.Checklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
Summary by CodeRabbit
New Features
Documentation
Tests