perf(onboard): cache WeChat plugin in sandbox base#3682
Conversation
Fixes NVIDIA#3677 Signed-off-by: San Dang <sdang@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
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:
📝 WalkthroughWalkthroughMoves the WeChat plugin installation into the cached base image, preserves any existing ChangesWeChat Plugin Base Caching and Config Preservation
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 unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Dockerfile (1)
385-385: Run the container E2E subset for this onboard build-path simplification.
openclaw doctor --fix --non-interactivechanges build-time runtime-dependency staging behavior; please validate with the recommended E2E jobs before merge.As per coding guidelines:
Dockerfilelayer ordering and baked config changes are only testable with a real container build and the listed E2E workflows.🤖 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` at line 385, The Dockerfile addition RUN openclaw doctor --fix --non-interactive alters build-time runtime-dependency staging and must be validated by running the recommended E2E jobs; run the container build and execute the prescribed E2E subset (the onboard build-path simplification E2E workflows) against this image to verify layer ordering and baked config behavior, capture any failures or differences in dependency staging, and report back before merging so we can revert or adjust the RUN step if tests fail.Dockerfile.base (1)
204-211: Run the container E2E suite for this base-layer bake change.This change affects baked plugin state and permissions in the sandbox base image; please run the recommended nightly E2E subset before merge:
cloud-e2e,sandbox-survival-e2e,hermes-e2e,rebuild-openclaw-e2e.As per coding guidelines:
Dockerfile.basechanges are only fully testable with a real container build and the listed E2E jobs.🤖 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.base` around lines 204 - 211, This change to Dockerfile.base modifies baked plugin state and sandbox permissions (see the RUN line installing '`@tencent-weixin/openclaw-weixin`@2.4.2' and the USER sandbox/USER root switches), so before merging run the recommended nightly E2E subset against a real container build: cloud-e2e, sandbox-survival-e2e, hermes-e2e and rebuild-openclaw-e2e; rebuild the base image locally or via CI, execute those jobs, and report any failing tests or permission/plugin issues back in the PR.
🤖 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`:
- Line 385: The Dockerfile addition RUN openclaw doctor --fix --non-interactive
alters build-time runtime-dependency staging and must be validated by running
the recommended E2E jobs; run the container build and execute the prescribed E2E
subset (the onboard build-path simplification E2E workflows) against this image
to verify layer ordering and baked config behavior, capture any failures or
differences in dependency staging, and report back before merging so we can
revert or adjust the RUN step if tests fail.
In `@Dockerfile.base`:
- Around line 204-211: This change to Dockerfile.base modifies baked plugin
state and sandbox permissions (see the RUN line installing
'`@tencent-weixin/openclaw-weixin`@2.4.2' and the USER sandbox/USER root
switches), so before merging run the recommended nightly E2E subset against a
real container build: cloud-e2e, sandbox-survival-e2e, hermes-e2e and
rebuild-openclaw-e2e; rebuild the base image locally or via CI, execute those
jobs, and report any failing tests or permission/plugin issues back in the PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a305a709-ac3d-45d8-805a-74a1cd9557b6
📒 Files selected for processing (5)
DockerfileDockerfile.basescripts/generate-openclaw-config.pyscripts/seed-wechat-accounts.pytest/generate-openclaw-config.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/docs/parity-inventory.generated.json (1)
1-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd SPDX license header metadata to this generated JSON file.
Line 1 starts directly with JSON content and does not include SPDX licensing information, which violates repository compliance requirements for source files.
As per coding guidelines, "Every source file must include an SPDX license header for copyright and Apache-2.0 license".
🤖 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 `@test/e2e/docs/parity-inventory.generated.json` around lines 1 - 3, Prepend SPDX metadata header lines to the top of the generated JSON (the file that contains the "generated_by" and "entrypoints" fields) by adding comment lines like "/* SPDX-FileCopyrightText: YEAR YourOrganization */" and "/* SPDX-License-Identifier: Apache-2.0 */" immediately before the JSON opening brace, and update the generator script extract-legacy-assertions.ts so it emits those two SPDX comment lines when producing the JSON to ensure every regenerated file includes the required license header.test/e2e/docs/parity-map.yaml (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required SPDX license header to this YAML file.
test/e2e/docs/parity-map.yamlis missing the SPDX header required for YAML source files.As per coding guidelines,
**/*.{js,ts,tsx,jsx,sh,yaml,yml,json,md,mdx}: Every source file must include an SPDX license header for copyright and Apache-2.0 license.🤖 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 `@test/e2e/docs/parity-map.yaml` at line 1, Prepend the required SPDX license header to the top of the YAML file by adding comment lines before the existing "scripts:" key — e.g. add lines starting with "# SPDX-FileCopyrightText: © YEAR YourCopyrightHolder" and "# SPDX-License-Identifier: Apache-2.0" (replace YEAR/holder as appropriate) so the SPDX header appears above the "scripts:" top-level key.
🤖 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.
Outside diff comments:
In `@test/e2e/docs/parity-inventory.generated.json`:
- Around line 1-3: Prepend SPDX metadata header lines to the top of the
generated JSON (the file that contains the "generated_by" and "entrypoints"
fields) by adding comment lines like "/* SPDX-FileCopyrightText: YEAR
YourOrganization */" and "/* SPDX-License-Identifier: Apache-2.0 */" immediately
before the JSON opening brace, and update the generator script
extract-legacy-assertions.ts so it emits those two SPDX comment lines when
producing the JSON to ensure every regenerated file includes the required
license header.
In `@test/e2e/docs/parity-map.yaml`:
- Line 1: Prepend the required SPDX license header to the top of the YAML file
by adding comment lines before the existing "scripts:" key — e.g. add lines
starting with "# SPDX-FileCopyrightText: © YEAR YourCopyrightHolder" and "#
SPDX-License-Identifier: Apache-2.0" (replace YEAR/holder as appropriate) so the
SPDX header appears above the "scripts:" top-level key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 59ecc062-fafe-4c7d-ae18-afde014a5e1a
📒 Files selected for processing (2)
test/e2e/docs/parity-inventory.generated.jsontest/e2e/docs/parity-map.yaml
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
# Conflicts: # test/e2e/docs/parity-inventory.generated.json
ericksoa
left a comment
There was a problem hiding this comment.
Approved after maintainer follow-ups on the stale sandbox-base fallback and rebuild credential-scan false positive. Current head 45d8df0 is mergeable, required CI is green, and the recommended nightly E2E set completed successfully.
…#3913) The brew policy preset advertises "Homebrew (Linuxbrew) package manager access" and whitelists the linuxbrew binary paths, but the sandbox cannot actually install Homebrew at runtime: 1. nemoclaw-blueprint/policies/openclaw-sandbox.yaml does not list /home/linuxbrew under filesystem_policy.read_write, so Landlock denies writes there. 2. The sandbox runs as the unprivileged `sandbox` user with no sudo (Dockerfile.base creates `sandbox` with HOME=/sandbox). Homebrew's install script's first step is `sudo` to create + chown /home/linuxbrew/.linuxbrew, which the sandbox cannot grant. Applying the brew preset and running the documented bootstrap line in the sandbox fails immediately with "Insufficient permissions to install Homebrew to /home/linuxbrew/.linuxbrew". The preset's binary whitelist for /home/linuxbrew/.linuxbrew/bin/* becomes dead code. This change bakes Homebrew core into the sandbox base image during build (running as root, then dropping to sandbox via gosu for the clone). The companion baseline-policy change adds /home/linuxbrew to filesystem_policy.read_write so brew can extract bottles and manage Cellar/opt symlinks at runtime. After the next base-image rebuild, every sandbox starts with a working brew at /home/linuxbrew/.linuxbrew/bin/brew. Applying the brew preset plus running `brew install <formula>` works end-to-end with no bootstrap required. Precedent: NVIDIA#3682 (in v0.0.46, closes NVIDIA#3677) baked the WeChat plugin into the sandbox base image for the same reason. Files: - Dockerfile.base: add the brew install step at the end, after the WeChat plugin block. Image-build cost is ~80 to 150 MB for Homebrew core (formulae download on demand). - nemoclaw-blueprint/policies/openclaw-sandbox.yaml: add /home/linuxbrew to filesystem_policy.read_write with an inline comment explaining the link to the Dockerfile.base step. - test/policies.test.ts: add a behavior-style assertion that the baseline read_write list includes the Homebrew prefix, so a future removal of this entry trips CI. Out of scope for this PR: - agents/hermes/Dockerfile.base. Hermes does not currently get the WeChat plugin baking either; same scoping rationale. - Adding /home/linuxbrew/.linuxbrew/bin to the system PATH. Users can run `brew` via the full path or via `eval "$(brew shellenv)"`. PR NVIDIA#3846 documents the latter. Closes NVIDIA#3913 Refs NVIDIA#1767, NVIDIA#3757 Signed-off-by: latenighthackathon <support@latenighthackathon.com>
…#3916) ## Summary Closes #3913. Bakes Homebrew core into the sandbox base image so the `brew` policy preset actually works end-to-end. Without this, applying the preset and running `brew install <formula>` inside the sandbox fails with `Insufficient permissions to install Homebrew to "/home/linuxbrew/.linuxbrew"` because the sandbox lacks filesystem write to `/home/linuxbrew/` AND runs as an unprivileged user with no sudo. ## Problem Two filesystem constraints block runtime Homebrew installation today: 1. `nemoclaw-blueprint/policies/openclaw-sandbox.yaml` lists only `[/tmp, /dev/null, /sandbox/.openclaw, /sandbox/.nemoclaw]` plus the workdir under `filesystem_policy.read_write`. Landlock denies writes to `/home/linuxbrew/`. 2. `Dockerfile.base` runs the sandbox as `useradd -r -g sandbox -d /sandbox -s /bin/bash sandbox` (no sudo). Homebrew's install script's first step is `sudo` to create + chown `/home/linuxbrew/.linuxbrew`, which the sandbox cannot grant. Result: the `brew` preset's binary whitelist for `/home/linuxbrew/.linuxbrew/bin/*` is dead code. Two QA reports already documented the symptom: [#1767](#1767) (filed 2026-04-10) and [#3757](#3757) (filed 2026-05-18). [#3846](#3846) was an interim docs-only attempt that we closed in favor of this PR; documenting a bootstrap that cannot actually succeed in the default sandbox would have set wrong expectations. ## Approach Follow the [#3682](#3682) precedent (WeChat plugin baked into the base image in v0.0.46): provision the tool at image-build time, when the build runs as root and can create + chown the prefix. - `Dockerfile.base`: after the existing WeChat plugin install block, add a step that creates `/home/linuxbrew/.linuxbrew/bin`, chowns to `sandbox:sandbox`, clones Homebrew core as the sandbox user via `gosu` at the pinned `ARG HOMEBREW_VERSION=5.1.12` tag, symlinks the `brew` entry point, and asserts `brew --version` succeeds. The `ARG` keeps the base-image layer reproducible across rebuilds and can be bumped by editing the default or passing `--build-arg`. - `nemoclaw-blueprint/policies/openclaw-sandbox.yaml`: add `/home/linuxbrew` to `filesystem_policy.read_write` so runtime `brew install <formula>` can extract bottles and manage Cellar/opt symlinks. - `test/policies.test.ts`: behavior-style assertion that the baseline `read_write` list includes the Homebrew prefix. Trips CI if a future change drops the entry. ## Cost Approximately 80 to 150 MB on the base image (Homebrew core only; formulae download on demand). Acceptable relative to the existing ~2.4 GB sandbox image and consistent with the trade-off the project already accepted for the WeChat plugin baking. ## Follow-up docs PR planned Once this merges and the base image rebuilds, a small docs PR will land covering the new flow on the matching MDX pages: apply the `brew` preset, run `brew install <formula>`. The previous attempt at documenting a runtime bootstrap (closed [#3846](#3846)) becomes obsolete and the new docs will reflect that `brew` is included by default. ## Out of scope - **`agents/hermes/Dockerfile.base`** — Hermes does not currently get the WeChat plugin baking either; same scoping rationale. Can extend in a follow-up if maintainers want Homebrew under Hermes. - **System `PATH`** — users can run `brew` via the full path `/home/linuxbrew/.linuxbrew/bin/brew` or via `eval "$(/home/linuxbrew/.linuxbrew/bin/brew shellenv)"`. A separate PR could add the prefix to `/etc/bash.bashrc` if a cleaner default UX is wanted. - **Option B (drop the brew preset entirely)** — discussed in [#3913](#3913); rejected because removing a Balanced-tier preset is a user-visible regression. ## Test plan - [x] `prek run --files` clean on all changed files - [x] New `test/policies.test.ts` assertion passes (`Homebrew prefix` test) - [x] Behavior-style test (parses YAML and checks the structured `read_write` array); satisfies the source-shape budget - [x] Verified the pinned `HOMEBREW_VERSION=5.1.12` tag exists upstream (`gh api repos/Homebrew/brew/releases` + direct `https://github.com/Homebrew/brew/releases/tag/5.1.12` returns 200) - [ ] **CI to verify:** sandbox base image rebuilds successfully with the new step - [ ] **CI to verify:** `brew --version` runs inside a freshly-created sandbox (covered by the build-time `gosu sandbox /home/linuxbrew/.linuxbrew/bin/brew --version` assertion in `Dockerfile.base`) - [ ] **Manual smoke after base-image publish:** `nemoclaw onboard` -> `<name> policy-add brew --yes` -> `<name> connect` -> `brew install hello` -> succeeds Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Homebrew package manager is now pre-installed and available in the sandbox environment, enabling users to install and manage software packages using standard Homebrew commands. * **Tests** * Added validation tests for Homebrew policy configuration and binary permissions. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/3916?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: latenighthackathon <support@latenighthackathon.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: Aaron Erickson <aerickson@nvidia.com>
Summary
Caches the OpenClaw WeChat plugin in
Dockerfile.baseso per-sandbox onboard builds no longer reinstall it. The final image now preserves the base plugin install registry, seeds captured WeChat account state during config generation, and runs OpenClaw doctor to stagebundled channel runtime deps before npm is locked offline.
Related Issue
Fixes #3677
Changes
@tencent-weixin/openclaw-weixin@2.4.2installation intoDockerfile.base.plugins.installsentries whengenerate-openclaw-config.pyrewritesopenclaw.json.seed-wechat-accounts.pyfrom config generation only whenopenclaw-weixinis already registered.openclaw doctor --fix --non-interactive.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Chores
Tests