fix(sandbox): reset NPM_CONFIG_OFFLINE after build-time plugin install#4900
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
📝 WalkthroughWalkthroughScope the build-time npm offline lock to the plugin-install step and reset it for runtime. Dockerfile adds ChangesNPM Online Runtime Access for MCP Server Installation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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 docstrings
🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Dockerfile (1)
583-618: Run the Dockerfile-focused E2E subset before merge.Given this runtime env behavior change in the sandbox image, run the recommended nightly-e2e subset to validate container/runtime behavior beyond unit tests.
As per coding guidelines,
Dockerfilechanges are only fully testable with real container builds and the recommendedcloud-e2e,sandbox-survival-e2e,hermes-e2e,rebuild-openclaw-e2e, andopenclaw-tui-chat-correlation-e2ejobs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` around lines 583 - 618, The Dockerfile change modifies sandbox runtime behavior around NPM_CONFIG_OFFLINE and the OpenClaw plugin install (see ENV NPM_CONFIG_OFFLINE and the RUN openclaw plugins install /opt/nemoclaw block); before merging, run the recommended Dockerfile-focused E2E subset to validate container/runtime behavior by executing the nightly-e2e subset (including cloud-e2e, sandbox-survival-e2e, hermes-e2e, rebuild-openclaw-e2e, and openclaw-tui-chat-correlation-e2e) so the image build, plugin registration, and sandbox runtime installs are exercised end-to-end and any regressions from the ENV/RUN changes are caught.Source: Coding guidelines
scripts/nemoclaw-start.sh (1)
131-135: Validate with entrypoint-focused E2E jobs.This entrypoint env redirect change affects every sandbox boot path; run the recommended sandbox E2Es before merge.
As per coding guidelines,
scripts/nemoclaw-start.shchanges should be validated withsandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e, andopenclaw-slack-pairing-e2e.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/nemoclaw-start.sh` around lines 131 - 135, The change in scripts/nemoclaw-start.sh that pins npm runtime env vars ('npm_config_offline=false' and 'NPM_CONFIG_OFFLINE=false') affects every sandbox entrypoint; before merging, run and attach results from the entrypoint-focused E2E suites: sandbox-survival-e2e, sandbox-operations-e2e, cloud-e2e, and openclaw-slack-pairing-e2e to validate sandbox boot paths and ensure no regressions in openshell sandbox connect or PID 1 behavior; if any test fails, revert or adjust the env export logic in nemoclaw-start.sh and re-run the listed E2E jobs until all pass.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Dockerfile`:
- Around line 583-618: The Dockerfile change modifies sandbox runtime behavior
around NPM_CONFIG_OFFLINE and the OpenClaw plugin install (see ENV
NPM_CONFIG_OFFLINE and the RUN openclaw plugins install /opt/nemoclaw block);
before merging, run the recommended Dockerfile-focused E2E subset to validate
container/runtime behavior by executing the nightly-e2e subset (including
cloud-e2e, sandbox-survival-e2e, hermes-e2e, rebuild-openclaw-e2e, and
openclaw-tui-chat-correlation-e2e) so the image build, plugin registration, and
sandbox runtime installs are exercised end-to-end and any regressions from the
ENV/RUN changes are caught.
In `@scripts/nemoclaw-start.sh`:
- Around line 131-135: The change in scripts/nemoclaw-start.sh that pins npm
runtime env vars ('npm_config_offline=false' and 'NPM_CONFIG_OFFLINE=false')
affects every sandbox entrypoint; before merging, run and attach results from
the entrypoint-focused E2E suites: sandbox-survival-e2e, sandbox-operations-e2e,
cloud-e2e, and openclaw-slack-pairing-e2e to validate sandbox boot paths and
ensure no regressions in openshell sandbox connect or PID 1 behavior; if any
test fails, revert or adjust the env export logic in nemoclaw-start.sh and
re-run the listed E2E jobs until all pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2dab1719-9c69-4b6a-a788-08521e22d91e
📒 Files selected for processing (5)
Dockerfilescripts/nemoclaw-start.shtest/fetch-guard-patch-regression.test.tstest/sandbox-provisioning.test.tstest/service-env.test.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 27084018473
|
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 27084413711
|
Summary
ENV NPM_CONFIG_OFFLINE=trueatDockerfile:585is scoped to the local OpenClaw plugin-install RUN, but the directive persists into the runtime image. PID 1 in the sandbox therefore exportsNPM_CONFIG_OFFLINE=true, which forces every childnpm/npxintocache mode: only-if-cachedand fails dashboard "Add MCP", skill installers, and ad-hocnpx -yinvocations withENOTCACHED. Reset tofalseimmediately after the plugin install and pin the runtime value from the entrypoint as a backstop.Related Issue
Fixes #4773.
Changes
Dockerfile— resetNPM_CONFIG_OFFLINE=falseimmediately after theopenclaw plugins installRUN so the runtime image inheritsfalsewhile the build-time isolation around that single RUN is preserved.scripts/nemoclaw-start.sh— addnpm_config_offline=falseandNPM_CONFIG_OFFLINE=falseto_TOOL_REDIRECTSso PID 1 andopenshell sandbox connectsessions pin the value regardless of a stale base image or future build-time regression.test/sandbox-provisioning.test.ts— two runtime regression guards: replays every DockerfileENVdirective and asserts the runtime image ends withNPM_CONFIG_OFFLINE=false; replays the staged subset up to the local plugin-install RUN and asserts the offline lock is stilltrueat that point. Pure runtime assertions — no source-text shape.test/service-env.test.ts— extended emission test asserts the generatedproxy-env.shcarriesnpm_config_offline=falseandNPM_CONFIG_OFFLINE=false. Two new tests source the actual_TOOL_REDIRECTSexport loop from the entrypoint and re-source the emittedproxy-env.shin a cleanenv -i bash --noprofile --norcshell, asserting both spellings resolve tofalse(PID 1 + sandbox-connect parity).test/fetch-guard-patch-regression.test.ts— narrow the existing test's end marker to# Release the offline lockso it brackets only the plugin-install RUN and ignores the follow-on ENV directive.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com