refactor(cli): extract docker command helpers - #2632
Merged
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughCentralizes Docker CLI interactions into a new typed helper library under Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
30-44: Run the onboarding E2E workflows for this helper migration.This refactor reroutes several core
src/lib/onboard.tsDocker paths through new helpers, so I'd validate the full gateway/sandbox lifecycle before merging withcloud-e2e,sandbox-operations-e2e, andrebuild-openclaw-e2e.As per coding guidelines,
src/lib/onboard.ts: "This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow." E2E test recommendation:cloud-e2e,sandbox-operations-e2e,rebuild-openclaw-e2e.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 30 - 44, The refactor in src/lib/onboard.ts reroutes Docker operations through new helpers (see require("./docker") and the docker helper symbols like dockerContainerInspectFormat, dockerPull, dockerRm, dockerRmi, dockerStop), so before merging you must run full onboarding E2E workflows to validate gateway/sandbox lifecycle; run the cloud-e2e, sandbox-operations-e2e, and rebuild-openclaw-e2e suites against this branch, exercise create/configure/start/stop/remove sandbox flows, verify docker image pulls, container inspect/exec, and volume cleanup behavior, and report/fix any failures observed in the onboard.ts paths that call the listed docker helpers.
🤖 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/agent-onboard.ts`:
- Around line 12-13: The module is missing the run import used by
handleAgentSetup, causing a runtime failure; restore the named import of run
from the runner module alongside ROOT (i.e., update the import line that
currently imports ROOT to also import run), so the existing call to run(...)
inside handleAgentSetup resolves correctly while keeping the existing
dockerBuild and dockerImageInspect imports intact.
In `@src/lib/docker/volume.ts`:
- Around line 13-36: The dockerRemoveVolumesByPrefix function currently allows
empty or whitespace-only prefixes which can match everything; add a fail-fast
validation at the start of dockerRemoveVolumesByPrefix (and optionally
dockerListVolumesByPrefix) to trim the prefix and throw an error (or return
no-op) when prefix.trim().length === 0 so you never call
dockerListVolumesByPrefix/dockerRemoveVolumes with a broad matcher; update the
function to check the trimmed prefix, throw a clear Error like "prefix must be a
non-empty string" and use the trimmed value when calling
dockerListVolumesByPrefix.
In `@src/lib/onboard.ts`:
- Around line 606-616: The code must keep base-image digest resolution
best-effort: call dockerPull(imageWithTag) with ignoreError: true (preserving
suppressOutput) so pulls don't throw, and call dockerImageInspectFormat("{{json
.RepoDigests}}", imageWithTag, { ignoreError: true }) instead of ignoreError:
false so inspect failures return null rather than fail-fast; after the inspect
call handle a falsy inspectOutput by returning null (maintaining the original
fallback to unpinned :latest). Use the existing dockerPull and
dockerImageInspectFormat symbols and the surrounding logic in the same function
to implement these option changes and the null-return path.
In `@src/nemoclaw.ts`:
- Around line 177-179: The call to
dockerRemoveVolumesByPrefix("openshell-cluster-...") passes ignoreError but the
initial "volume ls" probe in src/lib/docker/volume.ts does not receive/observe
opts, so errors from the probe (e.g., Docker unavailable) still bubble up;
modify the implementation in src/lib/docker/volume.ts so the initial listing
step either accepts and forwards the same opts (including ignoreError) from
dockerRemoveVolumesByPrefix or wraps the volume-ls call in a try/catch that
suppresses/returns on errors when opts.ignoreError is true, ensuring the helper
is truly best-effort.
In `@test/gateway-cleanup.test.ts`:
- Around line 17-20: The test currently checks file text which can falsely pass
because multiple dockerRemoveVolumesByPrefix callsites exist; update the test to
directly exercise destroyGateway() and assert that it invokes
dockerRemoveVolumesByPrefix with the specific prefix "openshell-cluster" (e.g.,
use jest.spyOn or mock the module exporting dockerRemoveVolumesByPrefix, call
destroyGateway from src/lib/onboard.ts, and expect the spy/mock to have been
called with "openshell-cluster"). Ensure you import the actual destroyGateway
function and restore mocks after the test.
In `@test/gateway-liveness-probe.test.ts`:
- Around line 20-23: The current test's docker regex is global and can be
satisfied by unrelated calls; narrow the check to the body of
verifyGatewayContainerRunning by locating the function declaration/definition
for verifyGatewayContainerRunning and asserting that its source contains a
docker inspect helper call (dockerInspect or dockerContainerInspectFormat).
Update the test to extract the function text for verifyGatewayContainerRunning
(by matching its declaration and body) and then assert that within that
extracted text there is a call to the Docker inspect helper, ensuring the probe
lives inside verifyGatewayContainerRunning.
In `@test/image-cleanup.test.ts`:
- Around line 16-18: The assertions are too broad and can be satisfied by
unrelated rmi calls (e.g., garbageCollectImages); narrow them to assert that the
removeSandboxImage function's body specifically calls the docker rmi helper.
Update the test to extract the removeSandboxImage function text (look for
"function removeSandboxImage(" or the removeSandboxImage identifier) and assert
that the extracted body matches /dockerRmi\(|docker.*\.rmi\(/ (i.e., dockerRmi
or a docker.rmi call appears inside that function), and make the same tightening
for the similar assertions around lines 78-81.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 30-44: The refactor in src/lib/onboard.ts reroutes Docker
operations through new helpers (see require("./docker") and the docker helper
symbols like dockerContainerInspectFormat, dockerPull, dockerRm, dockerRmi,
dockerStop), so before merging you must run full onboarding E2E workflows to
validate gateway/sandbox lifecycle; run the cloud-e2e, sandbox-operations-e2e,
and rebuild-openclaw-e2e suites against this branch, exercise
create/configure/start/stop/remove sandbox flows, verify docker image pulls,
container inspect/exec, and volume cleanup behavior, and report/fix any failures
observed in the onboard.ts paths that call the listed docker helpers.
🪄 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: b47e7142-cf78-486e-b60f-58f05b34c4b8
📒 Files selected for processing (17)
src/lib/agent-onboard.tssrc/lib/docker/container.tssrc/lib/docker/image.tssrc/lib/docker/index.test.tssrc/lib/docker/index.tssrc/lib/docker/info.tssrc/lib/docker/inspect.tssrc/lib/docker/pull.tssrc/lib/docker/run.tssrc/lib/docker/volume.tssrc/lib/nim.tssrc/lib/onboard.tssrc/nemoclaw.tstest/cli.test.tstest/gateway-cleanup.test.tstest/gateway-liveness-probe.test.tstest/image-cleanup.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/nemoclaw.ts (1)
175-177:⚠️ Potential issue | 🟠 Major
ignoreErrormay still be bypassed on gateway volume cleanup.Line 175 assumes best-effort cleanup, but if
dockerRemoveVolumesByPrefix()does not passoptsinto its internaldocker volume lsstep, this path can still fail when Docker is unavailable.#!/bin/bash set -euo pipefail # Verify whether dockerRemoveVolumesByPrefix forwards opts to the listing probe. fd -i 'volume.ts' src/lib/docker # Inspect both helper definitions and call flow. rg -n -C4 'function dockerListVolumesByPrefix|function dockerRemoveVolumesByPrefix|dockerListVolumesByPrefix\(' src/lib/docker/volume.tsExpected verification result:
dockerRemoveVolumesByPrefix(prefix, opts)should calldockerListVolumesByPrefix(prefix, opts).- If it calls
dockerListVolumesByPrefix(prefix)without opts, this issue is confirmed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 175 - 177, The call site uses dockerRemoveVolumesByPrefix(..., {ignoreError: true}) but dockerRemoveVolumesByPrefix may not forward opts into its internal listing step, so the ignoreError flag can be bypassed; update dockerRemoveVolumesByPrefix to pass the same opts object into dockerListVolumesByPrefix(prefix, opts) (instead of calling dockerListVolumesByPrefix(prefix)) and ensure any docker listing/removal errors are caught and suppressed when opts.ignoreError is true (preserve existing signature of dockerRemoveVolumesByPrefix and dockerListVolumesByPrefix and add a defensive try/catch around the list/remove flow to respect ignoreError).
🧹 Nitpick comments (4)
src/lib/docker/login.ts (1)
12-16: Keep login invariants non-overridable by caller options.Right now
optscan override password stdin wiring and stdio/encoding. Safer precedence is...optsfirst, then fixed fields.♻️ Proposed fix
return dockerSpawnSync(["login", registry, "-u", username, "--password-stdin"], { - input: password, - encoding: "utf-8", - stdio: ["pipe", "pipe", "pipe"], ...opts, + input: password, + encoding: "utf-8", + stdio: ["pipe", "pipe", "pipe"], });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/docker/login.ts` around lines 12 - 16, The call to dockerSpawnSync currently spreads caller-supplied opts after the fixed fields (input, encoding, stdio) allowing callers to override these security-critical login invariants; change the object spread order so opts are spread first and the fixed fields (input: password, encoding: "utf-8", stdio: ["pipe","pipe","pipe"]) are applied last to enforce them (i.e., use { ...opts, input: password, encoding: "utf-8", stdio: [...] }) in the dockerSpawnSync invocation to prevent callers from overriding stdin/stdio/encoding.src/lib/shields.ts (1)
40-67: Run the shields lifecycle E2E before merge.Given this touches the kubectl execution path used by shields down/up flows, I recommend running:
gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=shields-config-e2eAs per coding guidelines,
src/lib/shields*.tshas the recommendation:shields-config-e2e — shields lifecycle + config get/set/rotate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shields.ts` around lines 40 - 67, This change touches the kubectl execution helpers kubectlExecArgv, kubectlExec, and kubectlExecCapture—run the shields lifecycle E2E to validate shields up/down and config flows before merging by executing the GitHub Actions workflow: gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=shields-config-e2e (jobs=shields-config-e2e runs shields lifecycle + config get/set/rotate) and fix any failures in those functions if the E2E reveals issues.src/lib/docker/exec.ts (1)
14-14: Enforce UTF-8 at runtime, not just by type.Line 14 currently allows
optsto overrideencoding, which weakens the helper contract for untyped callers.♻️ Proposed fix
export function dockerExecFileSync( args: readonly string[], opts: DockerExecFileSyncOptions = {}, ): string { - return String(execFileSync("docker", [...args], { encoding: "utf-8", ...opts })); + return String(execFileSync("docker", [...args], { ...opts, encoding: "utf-8" })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/docker/exec.ts` at line 14, The current call to execFileSync spreads opts after setting encoding, allowing callers to override encoding; change the object spread order so encoding is enforced at runtime by using { ...opts, encoding: "utf-8" } (i.e., spread opts first, then set encoding) in the execFileSync invocation in src/lib/docker/exec.ts so callers cannot override the enforced UTF-8 encoding.src/nemoclaw.ts (1)
172-177: Run sandbox lifecycle E2Es for this CLI-path refactor.Recommended pre-merge run:
gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=sandbox-survival-e2e,sandbox-operations-e2e,skip-permissions-e2eAs per coding guidelines,
src/nemoclaw.tschanges should run:sandbox-survival-e2e,sandbox-operations-e2e, andskip-permissions-e2e.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 172 - 177, The change in src/nemoclaw.ts (notably the cleanupGatewayAfterLastSandbox function) requires you to run the nightly-e2e workflow with the sandbox lifecycle jobs before merging; trigger the GitHub Actions workflow nightly-e2e.yaml with the jobs sandbox-survival-e2e, sandbox-operations-e2e, and skip-permissions-e2e for your branch, confirm all three jobs pass, and attach the workflow run ID/results to the PR so reviewers can verify the sandbox E2E coverage for this CLI-path refactor.
🤖 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/nemoclaw.ts`:
- Around line 175-177: The call site uses dockerRemoveVolumesByPrefix(...,
{ignoreError: true}) but dockerRemoveVolumesByPrefix may not forward opts into
its internal listing step, so the ignoreError flag can be bypassed; update
dockerRemoveVolumesByPrefix to pass the same opts object into
dockerListVolumesByPrefix(prefix, opts) (instead of calling
dockerListVolumesByPrefix(prefix)) and ensure any docker listing/removal errors
are caught and suppressed when opts.ignoreError is true (preserve existing
signature of dockerRemoveVolumesByPrefix and dockerListVolumesByPrefix and add a
defensive try/catch around the list/remove flow to respect ignoreError).
---
Nitpick comments:
In `@src/lib/docker/exec.ts`:
- Line 14: The current call to execFileSync spreads opts after setting encoding,
allowing callers to override encoding; change the object spread order so
encoding is enforced at runtime by using { ...opts, encoding: "utf-8" } (i.e.,
spread opts first, then set encoding) in the execFileSync invocation in
src/lib/docker/exec.ts so callers cannot override the enforced UTF-8 encoding.
In `@src/lib/docker/login.ts`:
- Around line 12-16: The call to dockerSpawnSync currently spreads
caller-supplied opts after the fixed fields (input, encoding, stdio) allowing
callers to override these security-critical login invariants; change the object
spread order so opts are spread first and the fixed fields (input: password,
encoding: "utf-8", stdio: ["pipe","pipe","pipe"]) are applied last to enforce
them (i.e., use { ...opts, input: password, encoding: "utf-8", stdio: [...] })
in the dockerSpawnSync invocation to prevent callers from overriding
stdin/stdio/encoding.
In `@src/lib/shields.ts`:
- Around line 40-67: This change touches the kubectl execution helpers
kubectlExecArgv, kubectlExec, and kubectlExecCapture—run the shields lifecycle
E2E to validate shields up/down and config flows before merging by executing the
GitHub Actions workflow: gh workflow run nightly-e2e.yaml --ref <branch> -f
jobs=shields-config-e2e (jobs=shields-config-e2e runs shields lifecycle + config
get/set/rotate) and fix any failures in those functions if the E2E reveals
issues.
In `@src/nemoclaw.ts`:
- Around line 172-177: The change in src/nemoclaw.ts (notably the
cleanupGatewayAfterLastSandbox function) requires you to run the nightly-e2e
workflow with the sandbox lifecycle jobs before merging; trigger the GitHub
Actions workflow nightly-e2e.yaml with the jobs sandbox-survival-e2e,
sandbox-operations-e2e, and skip-permissions-e2e for your branch, confirm all
three jobs pass, and attach the workflow run ID/results to the PR so reviewers
can verify the sandbox E2E coverage for this CLI-path refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1624e6c9-13d7-4300-a9b8-ffea8a259be2
📒 Files selected for processing (9)
src/lib/debug.tssrc/lib/docker/exec.tssrc/lib/docker/image.tssrc/lib/docker/index.tssrc/lib/docker/login.tssrc/lib/nim.tssrc/lib/sandbox-config.tssrc/lib/shields.tssrc/nemoclaw.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/docker/index.ts
- src/lib/docker/image.ts
- src/lib/nim.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
30-44: Run the onboarding E2E trio for this core-path refactor.Since this PR rewires core onboarding Docker paths, I strongly recommend running the three targeted E2E jobs before merge:
cloud-e2e,sandbox-operations-e2e, andrebuild-openclaw-e2e.As per coding guidelines:
src/lib/onboard.ts: “This file contains core onboarding logic... E2E test recommendation: cloud-e2e, sandbox-operations-e2e, rebuild-openclaw-e2e.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/onboard.ts` around lines 30 - 44, Run the three targeted onboarding E2E jobs to validate the core-path Docker refactor: execute cloud-e2e, sandbox-operations-e2e, and rebuild-openclaw-e2e and confirm they pass; while running them, focus tests on code paths that use the imported docker exports (docker, dockerContainerInspectFormat, dockerExecArgv, dockerImageInspect, dockerImageInspectFormat, dockerInfo, dockerInfoFormat, dockerInspect, dockerPull, dockerRemoveVolumesByPrefix, dockerRm, dockerRmi, dockerStop) to ensure the rewired paths and calls still resolve and behave correctly (no missing exports, wrong formats, or failing Docker operations), and if any E2E fails capture the failing command and stack trace and adjust the corresponding import/usage of those docker symbols accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 30-44: Run the three targeted onboarding E2E jobs to validate the
core-path Docker refactor: execute cloud-e2e, sandbox-operations-e2e, and
rebuild-openclaw-e2e and confirm they pass; while running them, focus tests on
code paths that use the imported docker exports (docker,
dockerContainerInspectFormat, dockerExecArgv, dockerImageInspect,
dockerImageInspectFormat, dockerInfo, dockerInfoFormat, dockerInspect,
dockerPull, dockerRemoveVolumesByPrefix, dockerRm, dockerRmi, dockerStop) to
ensure the rewired paths and calls still resolve and behave correctly (no
missing exports, wrong formats, or failing Docker operations), and if any E2E
fails capture the failing command and stack trace and adjust the corresponding
import/usage of those docker symbols accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 77a1b383-8da7-4c69-998b-9330a928a095
📒 Files selected for processing (7)
src/lib/agent-onboard.tssrc/lib/docker/index.test.tssrc/lib/docker/volume.tssrc/lib/onboard.tstest/gateway-cleanup.test.tstest/gateway-liveness-probe.test.tstest/image-cleanup.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/gateway-liveness-probe.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/gateway-cleanup.test.ts
- src/lib/docker/index.test.ts
- src/lib/agent-onboard.ts
- src/lib/docker/volume.ts
- test/image-cleanup.test.ts
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 `@test/docker-abstraction-guard.test.ts`:
- Around line 55-57: The failure message template literal contains unnecessary
escaped quotes (\"docker\") which trip the no-useless-escape lint rule; edit the
template string that builds the message (the lines referencing
DOCKER_ABSTRACTION_PREFIX and the list of forbidden calls like run([...]),
runCapture([...]), spawnSync(\"docker\", ...), execFileSync(\"docker\", ...))
and remove the backslash escapes so the quotes are plain double quotes (e.g.,
spawnSync("docker", ...) and execFileSync("docker", ...)); keep the rest of the
wording and interpolation 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: e24d65dd-4787-4ce8-aa48-fe6f5aa9f34a
📒 Files selected for processing (1)
test/docker-abstraction-guard.test.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
## Summary `nemoclaw <sandbox> skill install --help` was treated like a path, and plugin-shaped directories only got a generic missing SKILL.md error. This PR makes the help path work and gives plugin users a clearer next step. ## Changes - Print skill install usage when `--help`, `-h`, or `help` follows `install`. - Detect OpenClaw plugin-shaped directories through `openclaw.plugin.json` or `package.json` metadata. - Add a targeted hint that plugins should be baked into a custom sandbox image with `nemoclaw onboard --from`. - Add CLI tests for both flows. ## Testing - `npm run build:cli` passed. - `npm run typecheck:cli` passed. - `npm test -- test/cli.test.ts` passed: 61 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 CLI test now verifies that `skill install --help` returns usage without a missing-file error and that plugin-shaped directories get the OpenClaw plugin hint. Fixes #2536 Signed-off-by: Deepak Jain <deepujain@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved error detection and messaging for `skill install` when given plugin-shaped directories, and added a clear suggestion to use the sandbox onboarding workflow instead. * Enhanced `--help` handling to display skill-install usage immediately, including when `--help` is passed as the positional path. * **Tests** * Expanded CLI tests for `skill install` help and plugin-detection scenarios, and restored related start-test coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Deepak Jain <deepujain@gmail.com>
## Summary NemoClaw docs did not explain how OpenClaw plugins should be installed under NemoClaw. This adds a deployment guide that points plugin users to the supported custom-sandbox-image path. ## Changes - Add an OpenClaw plugin install guide with a build-directory layout and Dockerfile example. - Link the guide from the docs deployment index. - Add a note under `skill install` that OpenClaw plugins are different from SKILL.md agent skills. - Regenerate the generated `nemoclaw-user` skills content. ## Testing - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user` passed. - `npm test -- test/skills-frontmatter.test.ts` passed: 38 tests. - `npm run build:cli` passed. - `npx prettier --check docs/deployment/install-openclaw-plugins.md docs/index.md docs/reference/commands.md .agents/skills/nemoclaw-user-deploy-remote/SKILL.md .agents/skills/nemoclaw-user-reference/references/commands.md` passed. - Full `npm test -- --reporter=dot` was attempted. In this local checkout it still fails in unrelated installer/uninstall/onboard tests and generated-dist lookup paths. ## Evidence it works The docs generator picked up the new plugin install page and updated the generated deployment skill content. Fixes #2538 Signed-off-by: Deepak Jain <deepujain@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added comprehensive OpenClaw plugins deployment guide with Dockerfile-based preparation, build and onboarding workflow, configuration notes, and troubleshooting. * Clarified that OpenClaw plugins are distinct from agent skills and require a separate installation process; updated command reference links accordingly. * Updated documentation navigation and renumbered onboarding steps to account for the new plugin content. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Deepak Jain <deepujain@gmail.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
) ## Summary NemoClaw's sandbox create stream only recognized the legacy Docker builder format, so BuildKit output would not be treated as active build progress once OpenShell emits it. This adds BuildKit progress markers to the same parser path as the existing legacy builder output. It keeps the current legacy behavior and makes `#1 [internal] ...`, `#2 CACHED`, and `#3 DONE ...` visible as build progress. ## Changes - `src/lib/sandbox-create-stream.ts`: recognize BuildKit step and completion lines while tracking the build phase. - `src/lib/sandbox-create-stream.test.ts`: cover BuildKit progress output and verify it is streamed to the user. ## Testing - `npm run build:cli` passed - `npm run typecheck:cli` passed - `npm test -- src/lib/sandbox-create-stream.test.ts` passed - `npm test` was also attempted. The full suite is not green on current main in this environment; failures are in existing installer/onboard/legacy-guard tests outside this change. ## Evidence it works The new focused test feeds BuildKit-style output into `streamSandboxCreate` and verifies that the lines are logged, collected in output, and mark sandbox creation as having seen progress. Fixes #2311 Signed-off-by: Deepak Jain <deepujain@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved detection and display of BuildKit and upload progress so progress markers and completion states are recognized reliably. * **Refactor** * Centralized progress-detection logic for more consistent handling of build and upload output. * **Tests** * Added a test ensuring BuildKit-formatted progress lines are captured, included in output, and reported to the log callback. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Deepak Jain <deepujain@gmail.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
## Summary Restore the `cloud-experimental-e2e` job that was accidentally deleted from `nightly-e2e.yaml` in PR #2472. ## Related Issue Fixes #2570 ## Changes Restores the `cloud-experimental-e2e` job that tests: - Landlock read-only enforcement (8 assertions on .bashrc, .profile, .openclaw, .openclaw-data, /tmp) - API key leak detection in process list - `openclaw tui` smoke test inside sandbox - Live chat via `openclaw agent` - Skill injection + agent verification - `inference.local` HTTPS probe The job runs unconditionally (no feature-flag gate). Added to `notify-on-failure` needs list. Removed the old `skip/05-network-policy.sh` step (now covered by the dedicated `network-policy-e2e` job). ## Type of Change - Code change (feature, bug fix, or refactor) ## Verification - YAML validated on fork: all jobs parse correctly - Verified on fork CI: cloud-experimental-e2e PASS in 14m 5s ## AI Disclosure - AI-assisted — tool: Cursor --- Signed-off-by: Truong Nguyen <tgnguyen@nvidia.com> Made with [Cursor](https://cursor.com) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added nightly cloud experimental end-to-end tests to broaden coverage. * Made the experimental job selectable from the manual job list for targeted runs. * Always-check documentation during these runs for improved QA. * Ensure experimental sandbox is torn down and verified after tests. * Upload an install-log artifact when the experimental job fails to aid troubleshooting. * Include the experimental job in failure notifications and PR reporting so results are tracked. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Truong Nguyen <tgnguyen@nvidia.com>
Contributor
|
🚀 Docs preview ready! |
ericksoa
approved these changes
Apr 28, 2026
ericksoa
left a comment
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Reviewed current PR diff at 705c8a0 against GitHub base 71c0f42. No blocking findings.\n\nLocal validation:\n- npm run build:cli\n- npm run typecheck:cli\n- npm test -- src/lib/docker/index.test.ts test/docker-abstraction-guard.test.ts test/gateway-cleanup.test.ts test/gateway-liveness-probe.test.ts test/image-cleanup.test.ts src/lib/agent-onboard.test.ts test/secret-redaction.test.ts\n- npm test -- test/cli.test.ts\n\nNote: current main has advanced to 4876d24 and the PR is not mergeable yet. A non-committing merge probe conflicts in src/lib/onboard.ts, src/nemoclaw.ts, test/gateway-cleanup.test.ts, and test/gateway-liveness-probe.test.ts.
ericksoa
approved these changes
Apr 28, 2026
ericksoa
left a comment
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Reviewed latest head 6ec124a against base a76a65b after the main merge. No blocking findings.\n\nLocal validation on latest head:\n- npm run build:cli\n- npm run typecheck:cli\n- npm test -- src/lib/docker/index.test.ts test/docker-abstraction-guard.test.ts test/gateway-cleanup.test.ts test/gateway-liveness-probe.test.ts test/image-cleanup.test.ts src/lib/agent-onboard.test.ts test/secret-redaction.test.ts\n- npm test -- test/cli.test.ts\n\nNote: CI was still in progress when I checked the latest head.
DemianHeyGen
pushed a commit
to DemianHeyGen/NemoClaw
that referenced
this pull request
Apr 30, 2026
## Summary This PR extracts Docker command patterns into a small `src/lib/docker/` helper module tree. It reduces direct Docker command launches outside `src/lib/docker/` from 30 to 0, leaving the literal `docker` process boundary centralized in the Docker helper layer. ## Changes - Add focused Docker helper modules under `src/lib/docker/`: - `run.ts` - `exec.ts` - `pull.ts` - `info.ts` - `inspect.ts` - `image.ts` - `container.ts` - `volume.ts` - `login.ts` - `index.ts` - Migrate Docker usage in `src/lib/onboard.ts`, `src/lib/nim.ts`, `src/lib/agent-onboard.ts`, `src/nemoclaw.ts`, `src/lib/debug.ts`, `src/lib/sandbox-config.ts`, and `src/lib/shields.ts` to the new helpers. - Add unit coverage for Docker helper argv construction, volume-prefix filtering, no-op removal, and `ignoreError` probe behavior. - Update source-based CLI/onboard tests to assert the helper-based implementation instead of literal shell snippets. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `make docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Additional verification run: - [x] `npm run build:cli` passes - [x] Targeted tests pass: - `src/lib/docker/index.test.ts` - `test/gateway-cleanup.test.ts` - `test/gateway-liveness-probe.test.ts` - `test/image-cleanup.test.ts` - `src/lib/agent-onboard.test.ts` - `test/secret-redaction.test.ts` - [x] Inventory check reports zero direct `docker` command heads outside `src/lib/docker/` Note: full `npm test` / `npx prek run --all-files` currently hit an unrelated `test/validate-e2e-coverage.test.ts` failure on the current base branch, so those template checkboxes are intentionally left unchecked. ## AI Disclosure - [x] AI-assisted — tool: pi coding agent --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Refactor** * Centralized Docker command execution through dedicated helper functions for improved code organization and consistency across the application. * **Tests** * Added automated guard test to enforce use of centralized Docker abstractions. * Updated integration tests to reflect refactored Docker command patterns. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Deepak Jain <deepujain@gmail.com> Signed-off-by: Truong Nguyen <tgnguyen@nvidia.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Deepak Jain <deepujain@users.noreply.github.com> Co-authored-by: Truong Nguyen <tgnguyen@nvidia.com>
13 tasks
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
5 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
This PR extracts Docker command patterns into a small
src/lib/docker/helper module tree. It reduces direct Docker command launches outsidesrc/lib/docker/from 30 to 0, leaving the literaldockerprocess boundary centralized in the Docker helper layer.Changes
src/lib/docker/:run.tsexec.tspull.tsinfo.tsinspect.tsimage.tscontainer.tsvolume.tslogin.tsindex.tssrc/lib/onboard.ts,src/lib/nim.ts,src/lib/agent-onboard.ts,src/nemoclaw.ts,src/lib/debug.ts,src/lib/sandbox-config.ts, andsrc/lib/shields.tsto the new helpers.ignoreErrorprobe behavior.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Additional verification run:
npm run build:clipassessrc/lib/docker/index.test.tstest/gateway-cleanup.test.tstest/gateway-liveness-probe.test.tstest/image-cleanup.test.tssrc/lib/agent-onboard.test.tstest/secret-redaction.test.tsdockercommand heads outsidesrc/lib/docker/Note: full
npm test/npx prek run --all-filescurrently hit an unrelatedtest/validate-e2e-coverage.test.tsfailure on the current base branch, so those template checkboxes are intentionally left unchecked.AI Disclosure
Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Release Notes
Refactor
Tests