chore: remove committed dist/ artifacts, build from source via prepare script - #473
Merged
Conversation
…e script Fixes #457. Removes 76 compiled TypeScript files from git tracking. The npm `prepare` lifecycle script now builds dist on-the-fly during `npm install` and `npm install -g git+url`, using `env -u` to clear inherited npm config vars that would otherwise break the child install in global/production contexts. - Add `prepare` script to root package.json - Remove `!nemoclaw/dist/` negation from .gitignore - Convert Dockerfile to multi-stage build (builder compiles TS, runtime copies artifacts) - Remove hard-fail dist check from setup.sh (Dockerfile builds from source now)
Contributor
|
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 (1)
📝 WalkthroughWalkthroughRefactors build/distribution to stop tracking compiled artifacts: Changes
Sequence Diagram(s)sequenceDiagram
participant Dev/CI
participant Git
participant DockerBuilder as Docker (builder)
participant RuntimeImage as Docker (runtime)
participant npm as npm/tsc
Dev/CI->>Git: push source (no committed `nemoclaw/dist/`)
Dev/CI->>DockerBuilder: docker build (multi-stage)
DockerBuilder->>npm: npm install (in `nemoclaw`) & run `npm run build` / `tsc`
npm-->>DockerBuilder: outputs `nemoclaw/dist/`
DockerBuilder->>RuntimeImage: copy `dist/` into runtime image (COPY --from=builder)
RuntimeImage-->>Dev/CI: runtime image contains compiled plugin
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 (1)
Dockerfile (1)
3-8: Copypackage-lock.jsonto ensure reproducible builds.The builder stage copies
package.jsonbut notpackage-lock.json. Sincepackage-lock.jsonexists in the repository, including it and switching tonpm ci(instead ofnpm install) ensures dependency versions are locked and builds remain reproducible across environments.Proposed fix
# Stage 1: Build TypeScript plugin from source FROM node:22-slim AS builder -COPY nemoclaw/package.json nemoclaw/tsconfig.json /opt/nemoclaw/ +COPY nemoclaw/package.json nemoclaw/package-lock.json nemoclaw/tsconfig.json /opt/nemoclaw/ COPY nemoclaw/src/ /opt/nemoclaw/src/ WORKDIR /opt/nemoclaw -RUN npm install && npm run build +RUN npm ci && npm run build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 3 - 8, The Docker builder stage currently copies package.json but not package-lock.json and uses npm install; update the builder stage (FROM node:22-slim AS builder) to also COPY nemoclaw/package-lock.json into /opt/nemoclaw and replace the RUN npm install && npm run build step with RUN npm ci && npm run build so the build uses locked dependencies for reproducible builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Dockerfile`:
- Around line 3-8: The Docker builder stage currently copies package.json but
not package-lock.json and uses npm install; update the builder stage (FROM
node:22-slim AS builder) to also COPY nemoclaw/package-lock.json into
/opt/nemoclaw and replace the RUN npm install && npm run build step with RUN npm
ci && npm run build so the build uses locked dependencies for reproducible
builds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1f562808-7eb3-465d-90b1-eacaba3a2dda
⛔ Files ignored due to path filters (76)
nemoclaw/dist/blueprint/exec.d.tsis excluded by!**/dist/**nemoclaw/dist/blueprint/exec.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/exec.jsis excluded by!**/dist/**nemoclaw/dist/blueprint/exec.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/fetch.d.tsis excluded by!**/dist/**nemoclaw/dist/blueprint/fetch.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/fetch.jsis excluded by!**/dist/**nemoclaw/dist/blueprint/fetch.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/resolve.d.tsis excluded by!**/dist/**nemoclaw/dist/blueprint/resolve.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/resolve.jsis excluded by!**/dist/**nemoclaw/dist/blueprint/resolve.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/state.d.tsis excluded by!**/dist/**nemoclaw/dist/blueprint/state.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/state.jsis excluded by!**/dist/**nemoclaw/dist/blueprint/state.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/verify.d.tsis excluded by!**/dist/**nemoclaw/dist/blueprint/verify.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/blueprint/verify.jsis excluded by!**/dist/**nemoclaw/dist/blueprint/verify.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/cli.d.tsis excluded by!**/dist/**nemoclaw/dist/cli.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/cli.jsis excluded by!**/dist/**nemoclaw/dist/cli.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/connect.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/connect.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/connect.jsis excluded by!**/dist/**nemoclaw/dist/commands/connect.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/eject.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/eject.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/eject.jsis excluded by!**/dist/**nemoclaw/dist/commands/eject.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/launch.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/launch.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/launch.jsis excluded by!**/dist/**nemoclaw/dist/commands/launch.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/logs.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/logs.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/logs.jsis excluded by!**/dist/**nemoclaw/dist/commands/logs.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/migrate.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/migrate.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/migrate.jsis excluded by!**/dist/**nemoclaw/dist/commands/migrate.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/migration-state.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/migration-state.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/migration-state.jsis excluded by!**/dist/**nemoclaw/dist/commands/migration-state.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/onboard.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/onboard.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/onboard.jsis excluded by!**/dist/**nemoclaw/dist/commands/onboard.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/slash.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/slash.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/slash.jsis excluded by!**/dist/**nemoclaw/dist/commands/slash.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/status.d.tsis excluded by!**/dist/**nemoclaw/dist/commands/status.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/commands/status.jsis excluded by!**/dist/**nemoclaw/dist/commands/status.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/index.d.tsis excluded by!**/dist/**nemoclaw/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/index.jsis excluded by!**/dist/**nemoclaw/dist/index.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/config.d.tsis excluded by!**/dist/**nemoclaw/dist/onboard/config.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/config.jsis excluded by!**/dist/**nemoclaw/dist/onboard/config.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/prompt.d.tsis excluded by!**/dist/**nemoclaw/dist/onboard/prompt.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/prompt.jsis excluded by!**/dist/**nemoclaw/dist/onboard/prompt.js.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/validate.d.tsis excluded by!**/dist/**nemoclaw/dist/onboard/validate.d.ts.mapis excluded by!**/dist/**,!**/*.mapnemoclaw/dist/onboard/validate.jsis excluded by!**/dist/**nemoclaw/dist/onboard/validate.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (4)
.gitignoreDockerfilepackage.jsonscripts/setup.sh
jacobtomlinson
left a comment
jacobtomlinson
left a comment
Member
There was a problem hiding this comment.
The install script fails for me.
[3/7] Creating sandbox
──────────────────────────────────────────────────
Sandbox name (lowercase, numbers, hyphens) [my-assistant]: fizzbuzz
Creating sandbox 'fizzbuzz' (this takes a few minutes on first run)...
Building image openshell/sandbox-from:1774001214 from /tmp/nemoclaw-build-RWfy2j/Dockerfile
Context: /tmp/nemoclaw-build-RWfy2j
Gateway: nemoclaw
Building image openshell/sandbox-from:1774001214 from /tmp/nemoclaw-build-RWfy2j/Dockerfile
Step 1/27 : FROM node:22-slim AS builder
---> 1ee8d39405cd
Step 2/27 : COPY nemoclaw/package.json nemoclaw/tsconfig.json /opt/nemoclaw/
---> 68dc4cbac3d2
Step 3/27 : COPY nemoclaw/src/ /opt/nemoclaw/src/
Error: × Docker build stream error
╰─▶ Docker stream error: COPY failed: file not found in build context or excluded by .dockerignore: stat nemoclaw/src/:
file does not exist
Sandbox creation failed (exit 1).
Try: openshell sandbox list # check gateway state
Try: nemoclaw onboard # retry from scratch
Thoughts from claude
Bug: Docker build fails with "nemoclaw/src/ does not exist" during nemoclaw onboard
When nemoclaw onboard assembles the temporary Docker build context, it explicitly removes nemoclaw/src/ from it (in
bin/lib/onboard.js). However, the Dockerfile's Stage 1 builder attempts to COPY nemoclaw/src/ in order to compile
TypeScript from source — causing the build to fail.
This is unnecessary because npm install (run earlier in the install script via the prepare hook) already compiles the
TypeScript and produces nemoclaw/dist/. The pre-built dist/ is present on disk by the time nemoclaw onboard runs.
Fix direction: Remove the Stage 1 builder from the Dockerfile and COPY the pre-built nemoclaw/dist/ directly into the
runtime image, matching what the build context already provides.
Member
|
This patch got things working for me diff --git a/bin/lib/onboard.js b/bin/lib/onboard.js
index 060b5ba..968983a 100644
--- a/bin/lib/onboard.js
+++ b/bin/lib/onboard.js
@@ -450,7 +450,7 @@ async function createSandbox(gpu) {
run(`cp -r "${path.join(ROOT, "nemoclaw")}" "${buildCtx}/nemoclaw"`);
run(`cp -r "${path.join(ROOT, "nemoclaw-blueprint")}" "${buildCtx}/nemoclaw-blueprint"`);
run(`cp -r "${path.join(ROOT, "scripts")}" "${buildCtx}/scripts"`);
- run(`rm -rf "${buildCtx}/nemoclaw/node_modules" "${buildCtx}/nemoclaw/src"`, { ignoreError: true });
+ run(`rm -rf "${buildCtx}/nemoclaw/node_modules"`, { ignoreError: true });
// Create sandbox (use -- echo to avoid dropping into interactive shell)
// Pass the base policy so sandbox starts in proxy mode (required for policy updates later) |
Contributor
|
jacobtomlinson
approved these changes
Mar 20, 2026
jacobtomlinson
left a comment
jacobtomlinson
left a comment
Member
There was a problem hiding this comment.
I pushed the patch to this branch. Looks good to go from my end.
Contributor
Author
|
@jacobtomlinson Good catch — applied your patch. The Verified 9/9 from a clean clone: no more |
This was referenced Mar 20, 2026
4 tasks
2 tasks
4 tasks
Ryuketsukami
pushed a commit
to Ryuketsukami/NemoClaw
that referenced
this pull request
Mar 24, 2026
…e script (NVIDIA#473) * chore: remove committed dist/ artifacts, build from source via prepare script Fixes NVIDIA#457. Removes 76 compiled TypeScript files from git tracking. The npm `prepare` lifecycle script now builds dist on-the-fly during `npm install` and `npm install -g git+url`, using `env -u` to clear inherited npm config vars that would otherwise break the child install in global/production contexts. - Add `prepare` script to root package.json - Remove `!nemoclaw/dist/` negation from .gitignore - Convert Dockerfile to multi-stage build (builder compiles TS, runtime copies artifacts) - Remove hard-fail dist check from setup.sh (Dockerfile builds from source now) * Fix docker build issue --------- Co-authored-by: Jacob Tomlinson <jtomlinson@nvidia.com>
jessesanford
pushed a commit
to jessesanford/NemoClaw
that referenced
this pull request
Mar 24, 2026
…e script (NVIDIA#473) * chore: remove committed dist/ artifacts, build from source via prepare script Fixes NVIDIA#457. Removes 76 compiled TypeScript files from git tracking. The npm `prepare` lifecycle script now builds dist on-the-fly during `npm install` and `npm install -g git+url`, using `env -u` to clear inherited npm config vars that would otherwise break the child install in global/production contexts. - Add `prepare` script to root package.json - Remove `!nemoclaw/dist/` negation from .gitignore - Convert Dockerfile to multi-stage build (builder compiles TS, runtime copies artifacts) - Remove hard-fail dist check from setup.sh (Dockerfile builds from source now) * Fix docker build issue --------- Co-authored-by: Jacob Tomlinson <jtomlinson@nvidia.com>
wscurran
added
area: ci
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
4 participants
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
preparelifecycle script that builds dist on-the-fly duringnpm installandnpm install -g git+urlThis change affects every install path. If the prepare script fails silently, new users get a broken install with no dist/ and no error message. Please review carefully.
The
preparescript usesenv -u npm_config_global -u npm_config_prefix -u npm_config_omitto clear inherited npm environment variables. This is required becausenpm install -gexports these vars into child processes, which causes the childnpm installinside the prepare script to skip devDependencies (including typescript) or attempt a global install instead of a local one. This took 6 rounds of testing to get right — the npm lifecycle environment inheritance is poorly documented and full of edge cases.Install paths affected
npm install(local dev) — uses preparenpm install -g git+https://...(primary user install via install.sh) — uses preparenpm install -g .(local global install) — uses preparedocker build(sandbox creation) — multi-stage build, no longer depends on pre-built distscripts/setup.sh(onboarding) — no longer hard-fails on missing distWhat could go wrong
env -uisn't available on a platform (unlikely — it's POSIX)npm install --ignore-scripts— dist won't be built (expected, documented)Test plan
Tested through 6 rounds with an independent tester instance. Earlier rounds failed due to:
tscnot available (devDeps skipped in production mode)tscpackage resolved instead of typescriptnpx -p typescript tsccouldn't resolve the binary in global context--include=devcouldn't override inheritednpm_config_omitnpm_config_global=trueinherited, child npm did global installenv -uto clear inherited varsTests passed (15/15)
rm -rf nemoclaw/dist/ && npm install— prepare builds dist from sourcenpm install -g .succeeds in clean environment — the ship-blocker testnemoclaw helpworks after installdocker build .succeeds (multi-stage)npm run buildstill worksSummary by CodeRabbit