fix(image): route exec approvals to .openclaw-data#1823
Open
skainguyen1412 wants to merge 6 commits intoNVIDIA:mainfrom
Open
fix(image): route exec approvals to .openclaw-data#1823skainguyen1412 wants to merge 6 commits intoNVIDIA:mainfrom
skainguyen1412 wants to merge 6 commits intoNVIDIA:mainfrom
Conversation
Patch OpenClaw dist defaults in Dockerfile.base so exec approvals write to ~/.openclaw-data/exec-approvals.json and add a regression guard test.\n\nRefs: NVIDIA#1785
Add runtime compatibility patch in Dockerfile for stale GHCR base images and strengthen Dockerfile.base guard to scan all dist JS bundles with validation fallback. Update regression tests accordingly.
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Docker build steps that locate the global OpenClaw JS dist, search-and-replace legacy Changes
Sequence Diagram(s)sequenceDiagram
participant DockerBuild as Docker build
participant Npm as npm (global)
participant FS as Filesystem (openclaw/dist)
participant Shell as Shell (grep/sed)
DockerBuild->>Npm: resolve OPENCLAW_DIST_DIR ($(npm root -g)/openclaw/dist)
Npm-->>DockerBuild: return OPENCLAW_DIST_DIR
DockerBuild->>FS: check if dist directory exists
alt dist missing
FS-->>DockerBuild: not found
DockerBuild->>DockerBuild: fail build ("OpenClaw dist directory not found")
else dist present
DockerBuild->>Shell: grep *.js for legacy path
alt legacy occurrences found
Shell->>Shell: record files (mktemp), sed -i replace legacy → data path
else no legacy found
Shell->>Shell: verify new data path exists in dist (fail if not)
end
Shell->>Shell: grep for remaining legacy matches (with line numbers)
alt remaining found
DockerBuild->>DockerBuild: fail build ("OpenClaw exec approvals path patch failed")
else
DockerBuild-->>DockerBuild: continue build
end
end
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 |
2 tasks
![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: 1
🧹 Nitpick comments (1)
test/exec-approvals-path-regression.test.ts (1)
1-1: Avoid blanket@ts-nocheckin this testThis test is small and typed APIs are straightforward; removing global type suppression would preserve static checks with minimal cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/exec-approvals-path-regression.test.ts` at line 1, Remove the blanket "// `@ts-nocheck`" at the top of test/exec-approvals-path-regression.test.ts; run the type checker and fix the reported issues by adding explicit types to test variables/fixtures and imports used in the describe/it blocks (e.g., types for any helper like exec or approvalsPath), or where a true type mismatch cannot be resolved, replace a global suppression with a single-line "// `@ts-expect-error`" immediately adjacent to that specific statement so static checking remains enabled for the rest of the file.
🤖 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 220-230: The RUN step that sets OPENCLAW_DIST_DIR and searches for
the old path is too permissive: if the dist directory is missing or neither the
old nor new path strings are present the build can silently pass. Update the RUN
block that defines OPENCLAW_DIST_DIR and uses files_with_old_path/grep to
explicitly verify the target directory exists and then assert that at least one
of the expected strings is present (the old "~/.openclaw/exec-approvals.json" or
the new "~/.openclaw-data/exec-approvals.json"); if the directory is missing or
neither string is found, fail the build (exit 1) and emit a clear error. Use the
existing variable names (OPENCLAW_DIST_DIR, files_with_old_path) and the same
grep operations but add checks for -d "$OPENCLAW_DIST_DIR" and a grep -q for the
new path, and ensure the final verification step fails when neither path is
detected.
---
Nitpick comments:
In `@test/exec-approvals-path-regression.test.ts`:
- Line 1: Remove the blanket "// `@ts-nocheck`" at the top of
test/exec-approvals-path-regression.test.ts; run the type checker and fix the
reported issues by adding explicit types to test variables/fixtures and imports
used in the describe/it blocks (e.g., types for any helper like exec or
approvalsPath), or where a true type mismatch cannot be resolved, replace a
global suppression with a single-line "// `@ts-expect-error`" immediately adjacent
to that specific statement so static checking remains enabled for the rest of
the file.
🪄 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: Pro
Run ID: 47ee7215-50b8-442e-bf6b-6859d7e11020
📒 Files selected for processing (3)
DockerfileDockerfile.basetest/exec-approvals-path-regression.test.ts
Fail the runtime compatibility patch when the OpenClaw dist directory is missing or when neither old nor new exec approvals path markers can be found. Update regression assertions accordingly.
Contributor
|
✨ Thanks for submitting this PR, which proposes a fix for a bug with the exec approvals regression. Possibly related open issues: |
Avoid quoted tilde literals in exec approvals patch guards, remove the runtime-stage pipe that triggered hadolint, and tighten regression coverage for the new guard structure.
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 `@Dockerfile`:
- Around line 221-243: The stale-base fallback must ensure the redirected data
dir is writable by the sandbox user before patching paths: after determining
OPENCLAW_DIST_DIR and before using DATA_EXEC_APPROVALS_PATH, create the
~/.openclaw-data directory (e.g. /sandbox/.openclaw-data) if missing and set
ownership/permissions to the sandbox user (chown to sandbox:sandbox and/or chmod
to allow write) so OpenClaw can create exec-approvals.json; update the
Dockerfile sequence around the LEGACY_EXEC_APPROVALS_PATH /
DATA_EXEC_APPROVALS_PATH patch block to perform this chown/mkdir step alongside
the existing chown of logs/credentials/sandbox.
🪄 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: Pro Plus
Run ID: fdd794d6-b609-430a-8383-58559cf36264
📒 Files selected for processing (2)
DockerfileDockerfile.base
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile.base
wscurran
added
the
enhancement: integration
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
2 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
Fixes the exec approvals regression introduced by the
.openclaw/.openclaw-datasplit. OpenClaw now refuses to writeexec-approvals.jsonthrough a symlink, so this PR patches the installed OpenClaw dist bundles to write directly to~/.openclaw-data/exec-approvals.jsonwhile keeping/sandbox/.openclawimmutable.Related Issue
Fixes #1785
Changes
Dockerfile.basefrom~/.openclaw/exec-approvals.jsonto~/.openclaw-data/exec-approvals.jsonDockerfile.baseto scan all OpenClaw dist*.jsbundles and fail if the old path still exists or the path cannot be verifiedDockerfileso production builds still work when they pull a stale published GHCR base imagetest/exec-approvals-path-regression.test.tsfor both the base-image patch and the runtime compatibility shimType of Change
Testing
npx prek run --all-filespasses (or equivalentlymake check).npm testpasses.make docsbuilds without warnings. (for doc-only changes)Additional testing performed:
npm test -- test/exec-approvals-path-regression.test.tsChecklist
General
Code Changes
npx prek run --all-filesauto-fixes formatting (ormake formatfor targeted runs).Doc Changes
nemoclaw-contributor-update-docsagent skill to draft changes while complying with the style guide. For example, prompt your agent with "/nemoclaw-contributor-update-docscatch up the docs for the new changes I made in this PR."Signed-off-by: Thien Nguyen [email protected]
Summary by CodeRabbit
Bug Fixes
Tests