fix(dockerfile): patch plugin install for symlinked extensions dir#2377
Merged
Conversation
…2203, #2254) Patch 3: OpenClaw's install-safe-path and install-package-dir reject symlinked directories via lstat. NemoClaw symlinks ~/.openclaw/extensions to ~/.openclaw-data/extensions for writable persistence across sandbox rebuilds. Changing lstat to stat in these two modules lets symlinks resolve while the real security gates (realpath + isPathInside) remain intact. Patch 4: Plugin install persists metadata to openclaw.json via replaceConfigFile, but in the sandbox openclaw.json is immutable (444 root:root). Wrap the write with EACCES handling when OPENSHELL_SANDBOX=1 so the install succeeds gracefully — plugins auto-load from the extensions directory without config metadata. Closes #2203 Closes #2254 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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:
📝 WalkthroughWalkthroughDockerfile build now applies in-place text patches to embedded OpenClaw dist JS: replace Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client
participant CLI as OpenClaw CLI
participant Dist as OpenClaw dist (patched)
participant FS as Filesystem/Sandbox
User->>CLI: openclaw plugins install ...
CLI->>Dist: extract package & validate install path
Dist->>FS: fs.stat(baseDir)
alt base is real directory
FS-->>Dist: isDirectory = true
Dist-->>CLI: continue install
else not a directory
FS-->>Dist: error
Dist-->>CLI: throw error
end
Dist->>FS: write temporary config file
alt OPENSHELL_SANDBOX="1" and write fails with EACCES
FS--x Dist: EACCES
Dist-->>CLI: console.error (log) and continue
else write fails with other error
FS-->>Dist: error
Dist-->>CLI: throw error
end
CLI-->>User: install result
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 unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 169-173: The Dockerfile currently hard-codes config-CJQx-9zo.js;
instead locate the file dynamically by searching all JS chunks for the symbol
and operate on the matched file. Change the logic that sets rcf_file (and
subsequent greps) to: search under $OC_DIST for any file containing "async
function replaceConfigFile(params)" (e.g., grep -rl or find+grep), pick the
first match into rcf_file, then run the Python in-place patch against that file
(targeting the writeConfigFile(params.nextConfig...) pattern) and update the
final verification grep to look across the same matched file(s) for
OPENSHELL_SANDBOX.*EACCES; ensure references to replaceConfigFile,
writeConfigFile(params.nextConfig,...), OPENSHELL_SANDBOX and EACCES are used so
the patch works regardless of chunk filename.
- Around line 153-156: The replacement is too broad: sed currently replaces all
occurrences of "await fs.lstat(" in "$isp_file" which can silently change
unrelated lstat calls (e.g., dirLstat). Change the sed invocation in the Patch
3a block (the lines that build isp_file and run sed) to perform a targeted,
single-occurrence substitution that only replaces the exact "const baseLstat =
await fs.lstat(baseDir)" assignment with "const baseLstat = await
fs.stat(baseDir)" (i.e., remove the /g global flag and match the full
assignment), keeping the subsequent grep 'fs\.lstat(' check to fail if any other
lstat remains so the patch remains fail-closed; reference the isp_file variable,
the baseLstat pattern, the sed command and the grep check when locating the
change.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1303413e-5520-4165-8d2d-6c8d9b904eb7
📒 Files selected for processing (1)
Dockerfile
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
CodeRabbit's auto-fix commit (7bd9264) stripped the trailing newline at end of Dockerfile as a side effect of its other edits. This restores it so the `end-of-file-fixer` pre-commit hook passes. No logic change. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
CodeRabbit's auto-fix (7bd9264) correctly narrowed the install-safe-path sed from a broad `await fs\.lstat(` global replace to the specific `const baseLstat = await fs\.lstat(baseDir)` line — narrower is safer, doesn't risk over-replacing unrelated fs.lstat() calls elsewhere in the compiled file. But the post-sed assertion was left at the broad form, so it fires even when the patch is applied correctly: 0.839 ERROR: Patch 3a (install-safe-path) left lstat calls The file legitimately contains other fs.lstat() calls on intermediate path components that the narrowed sed (correctly) does not touch. Narrow the assertion to match the sed — verify the specific baseLstat line was replaced, not that every fs.lstat call in the file is gone. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Dockerfile (2)
153-156: Patch 3a correctly addresses previous review feedback.The sed replacement now targets the specific
baseLstatpattern without the/gflag, and the verification grep ensures no lstat calls remain.One edge-case consideration: if
grep -lreturns multiple files (unlikely given the specific glob), the double-quoted"$isp_file"would be treated as a single filename with embedded newlines, causing sed to fail. This is currently safe becauseinstall-safe-path-*.jswith this pattern likely matches exactly one file, but for consistency with the Patch 1/2 pattern (which usesfor f in ...), consider iterating:Optional hardening
- isp_file="$(grep -RIlE --include='*.js' 'const baseLstat = await fs\.lstat\(baseDir\)' "$OC_DIST/install-safe-path-"*.js)"; \ - test -n "$isp_file" || { echo "ERROR: install-safe-path baseLstat pattern not found" >&2; exit 1; }; \ - sed -i 's/const baseLstat = await fs\.lstat(baseDir)/const baseLstat = await fs.stat(baseDir)/' "$isp_file"; \ - if grep -q 'const baseLstat = await fs\.lstat(baseDir)' "$isp_file"; then echo "ERROR: Patch 3a (install-safe-path) left baseLstat lstat call" >&2; exit 1; fi; \ + isp_files="$(grep -RIlE --include='*.js' 'const baseLstat = await fs\.lstat\(baseDir\)' "$OC_DIST/install-safe-path-"*.js)"; \ + test -n "$isp_files" || { echo "ERROR: install-safe-path baseLstat pattern not found" >&2; exit 1; }; \ + printf '%s\n' "$isp_files" | xargs sed -i 's/const baseLstat = await fs\.lstat(baseDir)/const baseLstat = await fs.stat(baseDir)/'; \ + if grep -REq --include='*.js' 'const baseLstat = await fs\.lstat\(baseDir\)' "$OC_DIST/install-safe-path-"*.js 2>/dev/null; then echo "ERROR: Patch 3a (install-safe-path) left baseLstat lstat call" >&2; exit 1; fi; \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 153 - 156, The current patch sets isp_file from grep -l and runs sed on "$isp_file", which can fail if grep returns multiple filenames; change the logic to iterate over matching install-safe-path-*.js files (use the install-safe-path glob) and for each file (e.g., variable f) run sed -i to replace "const baseLstat = await fs.lstat(baseDir)" with "const baseLstat = await fs.stat(baseDir)" and then verify each file no longer contains the lstat call; reference symbols: isp_file, install-safe-path-*.js, baseLstat, fs.lstat, fs.stat, sed and the verification grep so the check is performed per-file rather than on a possibly newline-joined list.
169-172: Patch 4 correctly addresses the hardcoded filename issue from previous review.The dynamic file discovery via
grep -RIlEis the right approach. However,head -n 1silently ignores additional matches—ifreplaceConfigFileexists in multiple dist chunks (e.g., due to code splitting changes), only the first would be patched.Consider adding a uniqueness check for fail-closed behavior:
Suggested improvement
rcf_file="$(grep -RIlE --include='*.js' 'async function replaceConfigFile\(params\)' "$OC_DIST" | head -n 1)"; \ test -n "$rcf_file" || { echo "ERROR: replaceConfigFile function not found in OpenClaw dist" >&2; exit 1; }; \ + rcf_count="$(grep -RIlE --include='*.js' 'async function replaceConfigFile\(params\)' "$OC_DIST" | wc -l)"; \ + [ "$rcf_count" -eq 1 ] || { echo "ERROR: replaceConfigFile found in $rcf_count files, expected 1" >&2; exit 1; }; \The Python one-liner uses exact whitespace matching (tabs), which is inherently fragile—but the
assert old in srcmakes this fail-closed, which is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 169 - 172, The current discovery sets rcf_file="$(grep -RIlE --include='*.js' 'async function replaceConfigFile\(params\)' "$OC_DIST" | head -n 1)" which silently picks the first match and can miss additional dist chunks; change the logic that finds replaceConfigFile so it fails-closed when there are zero or multiple matches: run grep -RIlE to collect all matches into a list, if the list is empty exit with an error, if the list contains more than one file exit with an error asking for investigation (do not silently use head -n 1), then set rcf_file to the single path and proceed with the Python patch and the existing OPENSHELL_SANDBOX / EACCES checks; reference the symbols grep -RIlE 'async function replaceConfigFile(params)', rcf_file, and OPENSHELL_SANDBOX when implementing this uniqueness check.
🤖 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 153-156: The current patch sets isp_file from grep -l and runs sed
on "$isp_file", which can fail if grep returns multiple filenames; change the
logic to iterate over matching install-safe-path-*.js files (use the
install-safe-path glob) and for each file (e.g., variable f) run sed -i to
replace "const baseLstat = await fs.lstat(baseDir)" with "const baseLstat =
await fs.stat(baseDir)" and then verify each file no longer contains the lstat
call; reference symbols: isp_file, install-safe-path-*.js, baseLstat, fs.lstat,
fs.stat, sed and the verification grep so the check is performed per-file rather
than on a possibly newline-joined list.
- Around line 169-172: The current discovery sets rcf_file="$(grep -RIlE
--include='*.js' 'async function replaceConfigFile\(params\)' "$OC_DIST" | head
-n 1)" which silently picks the first match and can miss additional dist chunks;
change the logic that finds replaceConfigFile so it fails-closed when there are
zero or multiple matches: run grep -RIlE to collect all matches into a list, if
the list is empty exit with an error, if the list contains more than one file
exit with an error asking for investigation (do not silently use head -n 1),
then set rcf_file to the single path and proceed with the Python patch and the
existing OPENSHELL_SANDBOX / EACCES checks; reference the symbols grep -RIlE
'async function replaceConfigFile(params)', rcf_file, and OPENSHELL_SANDBOX when
implementing this uniqueness check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e5dabfc3-d7ce-4e27-8054-623e44b84dbf
📒 Files selected for processing (1)
Dockerfile
cv
approved these changes
Apr 24, 2026
ericksoa
approved these changes
Apr 24, 2026
ericksoa
left a comment
Contributor
There was a problem hiding this comment.
Build-time patches for OpenClaw's plugin install — reasonable approach given NemoClaw's symlinked extensions directory.
Looks good
- Security boundary preserved. The
lstat → statchange lets symlinks resolve, but the downstreamrealpath+isPathInsidecontainment checks still catch escapes from the base tree. The actual security gate is path containment, not symlink detection. - Fail-close pattern. Every patch has pre-checks (
grep+test -n) and post-checks (grep+|| exit 1). A pattern mismatch from an OpenClaw update breaks the build rather than silently skipping the patch. - EACCES scoping. Patch 4 only swallows EACCES when
OPENSHELL_SANDBOX=1— outside the sandbox, the original throw is preserved. - Patch 3b
isSymbolicLink() → falseis correctly redundant —fs.stat(notlstat) always returns false forisSymbolicLink()since it follows symlinks. The comment makes the intent explicit for future readers.
Note (non-blocking)
These patches are inherently fragile against OpenClaw version bumps (exact string matching on compiled JS). The fail-close guards are the right mitigation — just flagging that an OpenClaw upgrade will likely require re-checking these patterns.
LGTM.
DemianHeyGen
pushed a commit
to DemianHeyGen/NemoClaw
that referenced
this pull request
Apr 30, 2026
…VIDIA#2377) ## Summary Patches OpenClaw's plugin install path at image build time to support NemoClaw's symlinked extensions directory (`~/.openclaw/extensions` → `~/.openclaw-data/extensions`) and to gracefully handle the immutable `openclaw.json` config file. Without these patches, `openclaw plugins install` fails with "Invalid extensions directory" (NVIDIA#2203) and, if bypassed, crashes with EACCES on the config write (NVIDIA#2254). ## Related Issue Closes NVIDIA#2203 Closes NVIDIA#2254 ## Changes - **Patch 3 (install-safe-path + install-package-dir):** Replace `fs.lstat` with `fs.stat` in OpenClaw's `install-safe-path-*.js` and neutralize the `isSymbolicLink()` guard in `install-package-dir-*.js`'s `assertInstallBaseStable`. The real security gates — `realpath` + `isPathInside` containment checks — remain intact and still block symlinks escaping the base tree. - **Patch 4 (config-CJQx-9zo.js):** Wrap the `writeConfigFile` call inside `replaceConfigFile` with a try/catch that detects EACCES when `OPENSHELL_SANDBOX=1` and emits a warning instead of crashing. Plugins auto-load from the extensions directory via OpenClaw's discovery mechanism without needing config metadata. - Both patches follow the existing fail-close assertion pattern (grep pre-check, apply, grep post-check). ## 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 - [x] `npx prek run --all-files` passes - [x] `npm test` passes - [ ] 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) ## AI Disclosure - [x] AI-assisted — tool: Claude Code --- Signed-off-by: prekshivyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Extended build-time distribution patching and added verification to ensure the expected modifications are applied before completing the build. * **Bug Fixes** * Relaxed plugin installation path checks to avoid false rejections for valid setups (including symlinked paths). * Prevented build failures in sandboxed environments from permission-denied errors by logging them instead of aborting. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Ubuntu <ubuntu@nemoclaw-f6a691-inst-3cs11f3ytmmzubymi5nzdncnwia.us-west1-a.c.brevdevprod.internal> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai> Co-authored-by: Aaron Erickson 🦞 <aerickson@nvidia.com>
12 tasks
laitingsheng
added a commit
that referenced
this pull request
May 14, 2026
Patch 4 is a regex-based monkey-patch over OpenClaw's compiled JS that suppresses EACCES inside replaceConfigFile. Its source-shape coupling has broken three times in eight days (#2377, #2484, #2876) chasing upstream refactors; #2686 and #3497 report the latest casualty, where the regex no longer finds the function in 2026.4.24 and the image build fails. Patch 4 is also unnecessary by design: * In mutable-default mode, openclaw.json is chmod 660 sandbox:sandbox and the gateway UID is in the sandbox group (#2681), so plugin install writes through without ever hitting EACCES. * In shields-up mode, the entire config tree (file + parent dir + the plugin/extensions state dirs in HIGH_RISK_STATE_DIRS) is locked to root:root by design — refusing runtime mutations is the whole point of shields-up. Suppressing the EACCES masked that refusal and made the install appear to succeed silently while only the auto-discovery half landed. The expected flow is configure-in-mutable-mode → shields up → run. Plugin install attempted while shielded should fail cleanly, which is what happens without Patch 4. Reverts the rcf-shim replacement attempt; the require-hook approach does not catch OpenClaw's ESM named imports anyway (capture-at-import- time semantics). Resolves #2686 Resolves #3497 Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
cv
pushed a commit
that referenced
this pull request
May 15, 2026
## Summary Patch 4 in the sandbox `Dockerfile` is a regex-based monkey-patch over OpenClaw's compiled JS that wraps `replaceConfigFile` in an EACCES try/catch suppression. It is source-shape-coupled and has been rewritten three times in eight days chasing OpenClaw refactors: - `fefd69fa2` (#2377) — original literal-string match against [openclaw/openclaw@v2026.4.9](https://github.com/openclaw/openclaw/releases/tag/v2026.4.9). - `5dcb0a9b9` (#2484) — updated the literal string for the restructured write block in [openclaw/openclaw@v2026.4.24](https://github.com/openclaw/openclaw/releases/tag/v2026.4.24). - `e0290e153` (#2876) — hardened to a tolerant whitespace/property-order-aware regex against [openclaw/openclaw@v2026.4.24](https://github.com/openclaw/openclaw/releases/tag/v2026.4.24). #2686 and #3497 are the latest break: in current OpenClaw, the regex no longer finds the function shape and the image build aborts at Step 17/56. Patch 4 is also unnecessary by design. The EACCES it was suppressing does not happen in the supported flows: - **Mutable-default mode** (fresh sandbox, before `nemoclaw shields up`): `openclaw.json` is `chmod 660 sandbox:sandbox` and the gateway UID is in the sandbox group, courtesy of #2851 (closing #2681; superseding the EACCES-swallow attempt in #2693). `openclaw plugins install` writes through normally; no EACCES. - **Shields-up mode** (locked): the entire config tree — file, parent directory, and the `extensions`/`plugins` state dirs from [HIGH_RISK_STATE_DIRS](src/lib/shields/index.ts#L292-L306) — is locked to `root:root` by design. Shields-up exists *to refuse* runtime config and plugin mutations. Suppressing the EACCES masked that refusal and made `openclaw plugins install` appear to succeed silently while only the auto-discovery half landed. The expected lifecycle is **configure-in-mutable-mode → `shields up` → run**. Plugin install attempted while shielded should fail cleanly; that is exactly what happens without Patch 4. This PR therefore deletes Patch 4 entirely. ## Related Issue Resolves #2686 Resolves #3497 Related context: - #2681 — original "make `.openclaw` group-writable" issue, closed by #2851. - #2851 — PR that made mutable-mode plugin install work without an EACCES swallow. - #2693 — closed earlier EACCES-swallow attempt, superseded once #2851 landed. - #2544 — NemoClaw issue tracking the broader "plugin config requires multi-minute rebuild" problem. - openclaw/openclaw#72950 — upstream defect (no env-var or writable-overlay path for `plugins.entries.<id>.config`); the real fix has to land there. ## Changes - `Dockerfile`: drop the Patch 4 block (the `COPY scripts/rcf_patch.py` line and the inline Python invocation + grep guard). - `scripts/rcf_patch.py`: deleted. - `src/lib/sandbox/build-context.ts`: stop staging `scripts/rcf_patch.py` into the sandbox build context. - `test/rcf-patch.test.ts`: deleted. ## 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 - [x] `npx prek run --all-files` passes - [x] `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) Signed-off-by: Tinson Lai <tinsonl@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Removed OpenClaw patching logic from Docker build process and related artifact copies * Updated build context script staging behavior * **Tests** * Enhanced sandbox configuration test suite with environment variable passthrough support * Added version-based conditional patching validation and warning behavior tests <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
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
Patches OpenClaw's plugin install path at image build time to support NemoClaw's symlinked extensions directory (
~/.openclaw/extensions→~/.openclaw-data/extensions) and to gracefully handle the immutableopenclaw.jsonconfig file. Without these patches,openclaw plugins installfails with "Invalid extensions directory" (#2203) and, if bypassed, crashes with EACCES on the config write (#2254).Related Issue
Closes #2203
Closes #2254
Changes
fs.lstatwithfs.statin OpenClaw'sinstall-safe-path-*.jsand neutralize theisSymbolicLink()guard ininstall-package-dir-*.js'sassertInstallBaseStable. The real security gates —realpath+isPathInsidecontainment checks — remain intact and still block symlinks escaping the base tree.writeConfigFilecall insidereplaceConfigFilewith a try/catch that detects EACCES whenOPENSHELL_SANDBOX=1and emits a warning instead of crashing. Plugins auto-load from the extensions directory via OpenClaw's discovery mechanism without needing config metadata.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)AI Disclosure
Signed-off-by: prekshivyas prekshiv@nvidia.com
Summary by CodeRabbit
Chores
Bug Fixes