fix(onboard): document custom Dockerfile build context (Fixes #2541)#2586
fix(onboard): document custom Dockerfile build context (Fixes #2541)#2586cv merged 3 commits intoNVIDIA:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughDocuments that Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant CLI as Nemoclaw CLI
participant FS as Temp Build Context (fs)
participant Docker as Docker Daemon
User->>CLI: run `nemoclaw onboard --from /path/Dockerfile`
CLI->>FS: resolve Dockerfile -> buildContext = dirname(Dockerfile)
CLI->>FS: stage/copy files (excluding ignores)
FS-->>CLI: return bytes & file count (collectBuildContextStats)
CLI->>User: if bytes > 100_000_000 -> emit WARN (size, file count)
CLI->>Docker: send build-context tarball and request image build
Docker-->>CLI: streaming build logs (include custom Dockerfile markers)
CLI->>User: stream build logs / result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/onboard.ts (1)
3393-3417:⚠️ Potential issue | 🟡 MinorClean up the temp context on early failures.
buildCtxis created before the exit handler is registered, so anfs.cpSync/collectBuildContextStatsfailure, or theEACCESearly-exit path, can leave a temp dir behind. Remove it before exiting or install the cleanup earlier.♻️ Suggested fix
} catch (err) { const errorObject = typeof err === "object" && err !== null ? err : null; if (isErrnoException(errorObject) && errorObject.code === "EACCES") { + try { + fs.rmSync(buildCtx, { recursive: true, force: true }); + } catch {} console.error( ` Permission denied while copying build context from: ${path.dirname(fromResolved)}`, ); console.error( " The --from flag uses the Dockerfile's parent directory as the Docker build context.", ); console.error(" Move your Dockerfile to a dedicated directory and retry."); process.exit(1); } + try { + fs.rmSync(buildCtx, { recursive: true, force: true }); + } catch {} throw err; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 3393 - 3417, After creating buildCtx with fs.mkdtempSync, ensure the temp directory is cleaned up on early failures: either register the cleanup handler immediately after mkdtempSync or delete buildCtx in the catch paths before exiting/throwing. Specifically, after fs.mkdtempSync(...) (symbol: buildCtx) register a removal (or set a try/finally) so that failures in fs.cpSync (symbol: fs.cpSync), collectBuildContextStats, or the EACCES branch (symbols: isErrnoException / errorObject) call fs.rmSync/fs.rmdirSync with recursive/force and then exit or rethrow; likewise, in the generic catch path delete buildCtx before rethrowing to avoid leaving stale temp directories.
🧹 Nitpick comments (1)
docs/reference/commands.md (1)
169-170: Avoid superlative phrasing in docs prose (LLM pattern detected).“For the safest workflow” uses a superlative. Prefer neutral, direct wording.
As per coding guidelines: "Superlatives and marketing language ('powerful,' 'robust,' 'seamless,' 'cutting-edge'). Say what it does, not how great it is."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/commands.md` around lines 169 - 170, Replace the superlative phrase "For the safest workflow" with neutral, descriptive wording; e.g., change the sentence that begins "For the safest workflow, create a dedicated directory that contains only the Dockerfile and the files it needs:" to a plain instruction that states the action and intent (such as "To create an isolated build context, create a dedicated directory containing only the Dockerfile and the files it needs:") so the docs avoid marketing/superlative language.
🤖 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 43-46: Update the help text array in src/lib/onboard-command.ts
(the help/usage strings, likely defined as a constant like helpText or similar
inside onboard-command module) to append a short explicit sentence after the
line listing skipped directories: state that generated build/output directories
such as "dist/", "build/", and "target/" are still included in the Docker build
context unless moved or excluded, so users should be aware to exclude or
relocate them; keep the wording concise and consistent with surrounding
messages.
---
Outside diff comments:
In `@src/lib/onboard.ts`:
- Around line 3393-3417: After creating buildCtx with fs.mkdtempSync, ensure the
temp directory is cleaned up on early failures: either register the cleanup
handler immediately after mkdtempSync or delete buildCtx in the catch paths
before exiting/throwing. Specifically, after fs.mkdtempSync(...) (symbol:
buildCtx) register a removal (or set a try/finally) so that failures in
fs.cpSync (symbol: fs.cpSync), collectBuildContextStats, or the EACCES branch
(symbols: isErrnoException / errorObject) call fs.rmSync/fs.rmdirSync with
recursive/force and then exit or rethrow; likewise, in the generic catch path
delete buildCtx before rethrowing to avoid leaving stale temp directories.
---
Nitpick comments:
In `@docs/reference/commands.md`:
- Around line 169-170: Replace the superlative phrase "For the safest workflow"
with neutral, descriptive wording; e.g., change the sentence that begins "For
the safest workflow, create a dedicated directory that contains only the
Dockerfile and the files it needs:" to a plain instruction that states the
action and intent (such as "To create an isolated build context, create a
dedicated directory containing only the Dockerfile and the files it needs:") so
the docs avoid marketing/superlative language.
🪄 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: d28c9197-e9f1-437a-b419-72a8c3047d70
📒 Files selected for processing (5)
docs/reference/commands.mdsrc/lib/onboard-command.test.tssrc/lib/onboard-command.tssrc/lib/onboard.tstest/onboard.test.ts
wscurran
added
bug
deepujain
force-pushed
the
fix/2541-build-context-help
branch
from
3082b7c to
93b6c09
Compare
|
Rebased on latest main. Cleaned up custom build-context temp dirs on early failures, added the generated-output note, and adjusted the docs wording. npm run build:cli and npm test -- src/lib/onboard-command.test.ts test/onboard.test.ts pass. |
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/onboard.ts`:
- Around line 83-84: The constant CUSTOM_BUILD_CONTEXT_WARN_BYTES is using 100 *
1024 * 1024 (100 MiB) but the docs say 100 MB (decimal); update the value used
by CUSTOM_BUILD_CONTEXT_WARN_BYTES in src/lib/onboard.ts to a decimal
100,000,000 (e.g. 100_000_000 or 100 * 10**6) so the threshold matches the
documented 100 MB; ensure the symbol name remains 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: Enterprise
Run ID: e724b6a6-ed91-42fa-9af4-7a82db748082
📒 Files selected for processing (5)
docs/reference/commands.mdsrc/lib/onboard-command.test.tssrc/lib/onboard-command.tssrc/lib/onboard.tstest/onboard.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/lib/onboard-command.ts
- src/lib/onboard-command.test.ts
- docs/reference/commands.md
🚧 Files skipped from review as they are similar to previous changes (1)
- test/onboard.test.ts
|
Aligned the build-context warning threshold with the documented 100 MB value. npm run build:cli and npm test -- src/lib/onboard-command.test.ts test/onboard.test.ts pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/onboard.test.ts (1)
5636-5638: Strengthen the build-context assertion to avoid a false positiveAt Line 5638, matching
/custom-image/against all stdout can pass from theUsing custom Dockerfile:line even if theDocker build context:value is wrong. Assert against the context line directly.Suggested test hardening
assert.equal(payload.sandboxName, "my-assistant"); assert.match(result.stdout, /Using custom Dockerfile:/); assert.match(result.stdout, /Docker build context:/); - assert.match(result.stdout, /custom-image/); + const contextLine = result.stdout + .split("\n") + .find((line) => line.includes("Docker build context:")); + assert.ok(contextLine, `expected Docker build context line in stdout:\n${result.stdout}`); + const escapedBuildDir = customBuildDir.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + assert.match(contextLine, new RegExp(escapedBuildDir));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/onboard.test.ts` around lines 5636 - 5638, The test's final assertion is too broad: instead of matching /custom-image/ against all of result.stdout (which can be satisfied by the earlier "Using custom Dockerfile:" line), narrow the check to the "Docker build context:" line; update the assertion around the existing assert.match calls (the lines asserting "Using custom Dockerfile:" and "Docker build context:") to assert that the "Docker build context:" line itself contains "custom-image" (e.g., by matching /Docker build context:.*custom-image/ or extracting the capture from /Docker build context:\s*(.*)/ and asserting the captured value includes "custom-image"). Ensure the change references the same assertions in test/onboard.test.ts so only the context line is validated.
🤖 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.ts`:
- Around line 3583-3586: The size warning currently computes sizeMb using binary
MiB (buildContextStats.totalBytes / (1024 * 1024)) which mismatches the
100,000,000-byte threshold; update the calculation that sets sizeMb (and its use
in the console.warn) to use decimal megabytes (divide by 1_000_000) and format
to a reasonable precision (e.g., one decimal place) so the displayed value
aligns with the threshold and avoids confusing messages referencing
buildContextStats.totalBytes and sizeMb.
---
Nitpick comments:
In `@test/onboard.test.ts`:
- Around line 5636-5638: The test's final assertion is too broad: instead of
matching /custom-image/ against all of result.stdout (which can be satisfied by
the earlier "Using custom Dockerfile:" line), narrow the check to the "Docker
build context:" line; update the assertion around the existing assert.match
calls (the lines asserting "Using custom Dockerfile:" and "Docker build
context:") to assert that the "Docker build context:" line itself contains
"custom-image" (e.g., by matching /Docker build context:.*custom-image/ or
extracting the capture from /Docker build context:\s*(.*)/ and asserting the
captured value includes "custom-image"). Ensure the change references the same
assertions in test/onboard.test.ts so only the context line is validated.
🪄 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: 96de23cf-9690-4261-b5bc-80681b8f6ade
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
3579-3582:⚠️ Potential issue | 🟡 MinorUse decimal MB in the warning display to match the 100 MB threshold.
At Line 3579, the warning text is labeled
MB, but the conversion uses1024 * 1024(MiB). Since the threshold is decimal (100_000_000bytes), this can print a lower MB number than users expect when the warning triggers.🔧 Proposed fix
- const sizeMb = Math.round(buildContextStats.totalBytes / (1024 * 1024)); + const sizeMb = (buildContextStats.totalBytes / 1_000_000).toFixed(1); console.warn( ` WARN: build context contains about ${sizeMb} MB across ${buildContextStats.fileCount} files.`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 3579 - 3582, The warning displays MB using binary MiB conversion; change the calculation for sizeMb to use decimal megabytes (divide buildContextStats.totalBytes by 1_000_000) so the displayed value matches the decimal 100_000_000 byte threshold — update the code around sizeMb and the warning that references buildContextStats.totalBytes and buildContextStats.fileCount to compute and show decimal MB.
🤖 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.ts`:
- Around line 3579-3582: The warning displays MB using binary MiB conversion;
change the calculation for sizeMb to use decimal megabytes (divide
buildContextStats.totalBytes by 1_000_000) so the displayed value matches the
decimal 100_000_000 byte threshold — update the code around sizeMb and the
warning that references buildContextStats.totalBytes and
buildContextStats.fileCount to compute and show decimal MB.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fb6baf52-0c4f-4a0e-bf7b-fa59b6e22362
📒 Files selected for processing (2)
src/lib/onboard.tstest/onboard.test.ts
There was a problem hiding this comment.
I’m trying to reason through the large build-context warning path.
If a user runs nemoclaw onboard --from /tmp/Dockerfile, and /tmp is very large, when do we actually learn the context size today? It looks like we call fs.cpSync(path.dirname(fromResolved), buildCtx, ...) first, then call collectBuildContextStats(buildCtx) afterward.
Does that mean the warning only appears after we’ve already paid the cost of copying the large context into /tmp? If so, does this still address the main failure mode from #2541, where accidentally choosing a huge parent directory can stall or consume disk before the user gets useful feedback?
Would it be better to collect the same stats from path.dirname(fromResolved) before staging, using the same skip rules, and print the warning before the expensive copy/build work starts?
One smaller consistency question: the threshold is 100_000_000 bytes and the docs say 100 MB, but the displayed value divides by 1024 * 1024. Should the warning display use decimal MB as well, so the message matches the documented threshold?
deepujain
force-pushed
the
fix/2541-build-context-help
branch
from
a570e5b to
12b0a52
Compare
|
Rebased on latest main. The custom build-context warning now runs before staging, uses the same ignore rules as the copy step, and reports decimal MB. npm run build:cli and npm test -- src/lib/onboard-command.test.ts test/onboard.test.ts test/sandbox-build-context.test.ts pass. |
There was a problem hiding this comment.
Reviewed against main. The PR cleanly addresses #2541. Second commit absorbed ericksoa's feedback (stats run pre-cpSync, units consistent). Exit handler at onboard.ts:3797 still wraps buildCtx, so cleanup-on-failure is preserved. Skill docs (.agents/skills/...) lag the new --from paragraphs but that's auto-regenerated post-merge by the Docs to Skills workflow, no action needed here.
Security — sensitive files in the build context
The ignore list (node_modules, .git, .venv, __pycache__) is bulk-filter, not security-filter. With this PR explicitly documenting that the entire parent directory ships to Docker, "user puts Dockerfile in ~/" tarballs ~/.ssh/, ~/.aws/, ~/.netrc, etc. The repo's own .dockerignore already lists the right set (.env*, .ssh/, .aws/, .netrc, .npmrc, secrets/, *.pem, *.key, credentials.json). Suggest extending CUSTOM_BUILD_CONTEXT_IGNORES to mirror those — silent omission is the safer default for credentials, matching the existing four-entry pattern.
Pre-existing footguns this PR makes more reachable (follow-up)
--fromaccepts a directory silently.existsSyncreturns true for directories.--from /tmpwalks/copies/. Fix:statSync(fromResolved).isFile()with a clear error.- Empty context. Dockerfile under
./node_modules/foo/Dockerfile→ cpSync filter rejects the root by basename → empty staged dir (verified: destination is not even created) → opaque openshell error. Detect pre-cpSync and warn. - Symlinked Dockerfile.
path.resolvedoesn't dereference.--from /symlink/Dockerfileuses the symlink's parent.fs.realpathSyncfirst if we care.
Tests
The PR doesn't exercise its own new behavior:
- No test for the
> 100 MBwarning path. - No test that the ignore filter skips
node_modules. - No test for
cleanupCustomBuildCtxon cpSync failure.
Other notes
- Decimal MB is consistent now. ✓
- Single predicate reused for
cpSyncandcollectBuildContextStats— no drift. ✓ - Bulk of the
onboard.tsline count is prettier reflow. - Only one runtime consumer of
fromDockerfile. No parallel path to keep in sync. ✓
Verdict
Nothing functionally bad introduced. Footguns and security-ignore gap are pre-existing — fine as follow-ups.
Fixes NVIDIA#2541 Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
Signed-off-by: Deepak Jain <deepujain@gmail.com>
deepujain
force-pushed
the
fix/2541-build-context-help
branch
from
7795980 to
acff4d8
Compare
|
Pushed a follow-up for the custom build-context review. Credential-style files and dirs are now skipped, --from rejects directories or ignored roots clearly, and tests cover the large-context warning, ignored staging, and cleanup on staging failure. npm run build:cli, npm run typecheck:cli, and npm test -- src/lib/onboard-command.test.ts test/onboard.test.ts test/sandbox-build-context.test.ts pass. |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Re-reviewed acff4d8a. Author addressed every actionable item from the previous round:
- Security ignore list extended to mirror
.dockerignore(.aws,.ssh,.env*,*.pem,*.key,secrets/,credentials.json,service-account*.json, etc., case-insensitive). ✓ --fromdirectory check added at both the CLI parse layer (onboard-command.ts:88) and the createSandbox layer (onboard.ts:3774). ✓- Empty-context edge case handled by
isInsideIgnoredCustomBuildContextPathwalking each path segment and erroring before cpSync. ✓ - Tests added for the 100MB warning path (statSync mock returning 101MB), the ignore filter actually skipping
node_modules,.ssh,.aws,secrets,.env.local,.npmrc,*.pem,credentials.json, and the cleanup-on-cpSync-failure path. ✓ - Help text + docs updated to mention the credential skip list. ✓
One remaining nit (not a blocker)
isInsideIgnoredCustomBuildContextPath applies the file-extension predicates (.endsWith(".key"), .endsWith("_rsa"), etc.) to every directory segment. A user whose project lives under ~/keys/api.key/ or ~/projects/auth_rsa/ will hit the rejection on a directory that just happens to share a credential file's suffix. The error message ("Move your Dockerfile to a dedicated directory") gives them a workaround, so it's fine — but cleaner long-term would be to split the predicate: extension/suffix checks only for cpSync's per-entry filter (file names), exact-match set only for the segment walk (directory names).
Verdict
LGTM. Skill docs still lag but auto-regenerate post-merge via the Docs to Skills workflow. Symlinked-Dockerfile remains a follow-up.
…2541) (NVIDIA#2586) ## Summary `nemoclaw onboard --from` already used the Dockerfile parent directory as the Docker build context, but the help text and docs did not say that. This PR makes that behavior explicit and adds a size warning for unusually large custom contexts. ## Changes - Document the `--from <Dockerfile>` build context behavior in CLI help and reference docs. - Print the resolved Docker build context when a custom Dockerfile is used. - Warn when the custom context is larger than 100 MB so users can move the Dockerfile into a smaller build directory. - Extend onboard help and custom-Dockerfile tests. ## Testing - `npm run build:cli` passed. - `npm run typecheck:cli` passed. - `npm test -- src/lib/onboard-command.test.ts test/onboard.test.ts` passed: 152 tests. - Full `npm test -- --reporter=dot` was attempted. In this local checkout it still fails outside this change in installer/uninstall/onboard/build-context tests, including temp-source generated-dist lookup and a few timeout/status checks. ## Evidence it works The focused onboard suite now checks that help text names the Dockerfile parent directory and that custom onboarding output includes the resolved build context. Fixes NVIDIA#2541 Signed-off-by: Deepak Jain <deepujain@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Expanded help for the onboard --from Dockerfile workflow and added a recommended directory layout for staging. * **New Features** * Common large directories (node_modules, .git, .venv, __pycache__) are excluded during Docker-context staging. * Pre-build warning shown when the staged context exceeds ~100 MB. * **Tests** * Updated tests to assert the expanded help output and validate observable Docker build log markers. * **Refactor** * Improved onboarding messaging, warnings, and formatting for clarity and robustness. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Deepak Jain <deepujain@gmail.com>
5 participants
Summary
nemoclaw onboard --fromalready used the Dockerfile parent directory as the Docker build context, but the help text and docs did not say that. This PR makes that behavior explicit and adds a size warning for unusually large custom contexts.Changes
--from <Dockerfile>build context behavior in CLI help and reference docs.Testing
npm run build:clipassed.npm run typecheck:clipassed.npm test -- src/lib/onboard-command.test.ts test/onboard.test.tspassed: 152 tests.npm test -- --reporter=dotwas attempted. In this local checkout it still fails outside this change in installer/uninstall/onboard/build-context tests, including temp-source generated-dist lookup and a few timeout/status checks.Evidence it works
The focused onboard suite now checks that help text names the Dockerfile parent directory and that custom onboarding output includes the resolved build context.
Fixes #2541
Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
Documentation
New Features
Tests
Refactor