fix(install): ensure .openclaw-data ownership for sandbox user (fixes #692)#1089
Merged
cv merged 13 commits intoNVIDIA:mainfrom Apr 7, 2026
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:
📝 WalkthroughWalkthroughAdds a non-root-only local function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/nemoclaw-start.sh (1)
317-320: Ensure parent directory exists before symlink creation.If
${openclaw_dir}(/sandbox/.openclaw) doesn't exist, thelncommand will fail silently. While other code assumes this directory exists, adding a guard would make the fix more robust, especially for edge cases in native installs.Proposed improvement
if [ ! -e "${openclaw_dir}/identity" ] && [ -d "${data_dir}/identity" ]; then + mkdir -p "${openclaw_dir}" 2>/dev/null || true ln -sf "${data_dir}/identity" "${openclaw_dir}/identity" 2>/dev/null || true echo "[setup] created identity symlink" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` around lines 317 - 320, The symlink creation can fail if the parent directory referenced by openclaw_dir doesn't exist; modify the block that checks for identity and creates the symlink to first ensure the openclaw_dir parent exists (e.g., create the directory if missing) before running ln -sf, while preserving the existing checks using openclaw_dir, data_dir, and the identity target and keeping the silent-fail behavior (redirect errors and || true). Ensure the change only creates the directory when necessary and does not alter the conditional semantics around the identity symlink creation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 317-320: The symlink creation can fail if the parent directory
referenced by openclaw_dir doesn't exist; modify the block that checks for
identity and creates the symlink to first ensure the openclaw_dir parent exists
(e.g., create the directory if missing) before running ln -sf, while preserving
the existing checks using openclaw_dir, data_dir, and the identity target and
keeping the silent-fail behavior (redirect errors and || true). Ensure the
change only creates the directory when necessary and does not alter the
conditional semantics around the identity symlink creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02d0a39f-e4d3-43c1-9bae-b9dbf25b130f
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
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 `@scripts/nemoclaw-start.sh`:
- Around line 313-316: The ownership-check currently uses find with -maxdepth 0
on ${data_dir} and then runs chown -R but always echoes "fixed ownership" even
when chown fails; update the logic so you use find to detect any files/dirs in
${data_dir} not owned by the current uid/gid (e.g. remove -maxdepth 0 and use
-user/! -user/-group), run chown -R and capture its exit status or errors, and
only print "[setup] fixed ownership on ${data_dir}" when chown succeeds; if
chown fails, surface a clear error message (do not suppress with || true) so
unresolved EACCES paths are visible.
- Around line 317-320: The startup script only creates the identity symlink when
"${openclaw_dir}/identity" is missing; modify the logic around openclaw_dir,
data_dir and the identity path to detect and remediate incorrect existing
states: if "${openclaw_dir}/identity" exists but is not a symlink to
"${data_dir}/identity" (including a stale/broken symlink or a regular
directory), remove or rename the existing entry and then create the correct
symlink, preserving any safe backup behavior as needed; use readlink (or test -L
and readlink -f) to compare targets and ensure ln -sf is executed only after the
bad path is removed so permissions/target issues are resolved.
🪄 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: 8e360189-b61c-4471-a0bf-fe64d047617e
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/nemoclaw-start.sh (1)
327-332: Symlink operations log success unconditionally, unlike the chown pattern.The chown block (lines 314-318) correctly checks the exit status before logging success. However, the
ln -snfcalls on lines 327 and 331 use|| truebut still unconditionally echo success messages. Iflnfails (e.g., permission denied on parent directory), the user sees misleading "[setup] repaired/created identity symlink" output.For consistency with the ownership fix pattern:
♻️ Proposed fix for consistent error reporting
if [ "$current_target" != "$expected_target" ]; then - ln -snf "${data_dir}/identity" "${openclaw_dir}/identity" 2>/dev/null || true - echo "[setup] repaired identity symlink" + if ln -snf "${data_dir}/identity" "${openclaw_dir}/identity" 2>/dev/null; then + echo "[setup] repaired identity symlink" + else + echo "[setup] could not repair identity symlink" >&2 + fi fi elif [ ! -e "${openclaw_dir}/identity" ]; then - ln -snf "${data_dir}/identity" "${openclaw_dir}/identity" 2>/dev/null || true - echo "[setup] created identity symlink" + if ln -snf "${data_dir}/identity" "${openclaw_dir}/identity" 2>/dev/null; then + echo "[setup] created identity symlink" + else + echo "[setup] could not create identity symlink" >&2 + fi else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` around lines 327 - 332, The install script logs successful identity symlink creation/repair unconditionally even though the ln -snf calls use "|| true", which can hide failures; change the two ln -snf "${data_dir}/identity" "${openclaw_dir}/identity" invocations to remove the "|| true" and wrap each in an if/then that checks ln's exit status (e.g. if ln -snf ...; then echo "[setup] created/ repaired identity symlink"; else echo "[setup] failed to create/repair identity symlink" >&2; fi) so the messages for the identity symlink mirror the chown pattern and reflect real success/failure for operations involving ${data_dir}, ${openclaw_dir} and the ln command.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 327-332: The install script logs successful identity symlink
creation/repair unconditionally even though the ln -snf calls use "|| true",
which can hide failures; change the two ln -snf "${data_dir}/identity"
"${openclaw_dir}/identity" invocations to remove the "|| true" and wrap each in
an if/then that checks ln's exit status (e.g. if ln -snf ...; then echo "[setup]
created/ repaired identity symlink"; else echo "[setup] failed to create/repair
identity symlink" >&2; fi) so the messages for the identity symlink mirror the
chown pattern and reflect real success/failure for operations involving
${data_dir}, ${openclaw_dir} and the ln command.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 137a8d43-b40e-41bb-955c-bcc3dc13783d
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
cv
approved these changes
Mar 30, 2026
wscurran
added
bug
Contributor
|
✨ Thanks for submitting this PR with a detailed summary, it addresses a bug with the curl installer on Linux and proposes a fix to ensure .openclaw-data ownership for the sandbox user, which could improve the installation process of NemoClaw. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/nemoclaw-start.sh (1)
334-336:⚠️ Potential issue | 🟠 MajorRemediate existing non-symlink
~/.openclaw/identityinstead of leaving it broken.Line 335 currently warns and keeps the path unchanged. If that entry is a root-owned directory/file, the startup path remains broken for identity writes.
🔧 Proposed fix
elif [ ! -e "${openclaw_dir}/identity" ]; then ln -snf "${data_dir}/identity" "${openclaw_dir}/identity" 2>/dev/null || true echo "[setup] created identity symlink" else - echo "[setup] ${openclaw_dir}/identity exists and is not a symlink; leaving as-is" >&2 + local backup="${openclaw_dir}/identity.bak.$(date +%s)" + if mv "${openclaw_dir}/identity" "$backup" 2>/dev/null && \ + ln -snf "${data_dir}/identity" "${openclaw_dir}/identity" 2>/dev/null; then + echo "[setup] replaced non-symlink identity path (backup: ${backup})" + else + echo "[setup] could not replace ${openclaw_dir}/identity; writes may fail" >&2 + return 1 + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/nemoclaw-start.sh` around lines 334 - 336, Replace the current branch that only echoes a warning when "${openclaw_dir}/identity" exists and is not a symlink: detect the non-symlink, move it aside to a timestamped backup (e.g. "${openclaw_dir}/identity.bak.$(date +%s)") or otherwise rename it so root-owned files don't block startup, then create the intended symlink pointing at "${openclaw_dir}/identity"; ensure the mv and ln -s steps are guarded (check success and log failures) so the startup can proceed to write identity files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 334-336: Replace the current branch that only echoes a warning
when "${openclaw_dir}/identity" exists and is not a symlink: detect the
non-symlink, move it aside to a timestamped backup (e.g.
"${openclaw_dir}/identity.bak.$(date +%s)") or otherwise rename it so root-owned
files don't block startup, then create the intended symlink pointing at
"${openclaw_dir}/identity"; ensure the mv and ln -s steps are guarded (check
success and log failures) so the startup can proceed to write identity files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a2adb4a-cf6f-4d7c-9a2d-8f18ccc051b7
📒 Files selected for processing (1)
scripts/nemoclaw-start.sh
Contributor
Author
|
All CodeRabbit review feedback has been addressed:
Latest commit d2cf5db passes all pre-commit hooks (shellcheck, shfmt, gitleaks, etc). |
auto-merge was automatically disabled
April 1, 2026 02:34
Head branch was pushed to by a user without write access
Contributor
Author
|
Friendly ping — all CodeRabbit feedback has been addressed. Any other concerns? Will close in 7 days if no response. 🙏 |
…VIDIA#692) The native curl installer may create .openclaw-data directories as root, causing EACCES when openclaw (running as the sandbox user) tries to write device-auth.json to the identity directory. Add a startup ownership check to nemoclaw-start.sh that: - Creates any missing writable subdirectories (mirrors Dockerfile setup) - Fixes ownership if files are not owned by the current user - Creates the identity symlink if missing on native installs The Docker path is unaffected (Dockerfile already sets correct ownership). Resubmitted as requested by @mnsami (original: NVIDIA#698).
Add mkdir -p for openclaw_dir before ln -sf, in case /sandbox/.openclaw doesn't exist yet (e.g. fresh native installs). Addresses CodeRabbit review nitpick.
Address CodeRabbit review feedback: 1. Ownership check: remove -maxdepth 0 so find scans all descendants for wrong-uid files; surface chown failure instead of suppressing 2. Identity symlink: handle stale/broken symlinks and wrong targets by comparing readlink -f; warn if path exists as non-symlink
…link identity - Wrap ln -snf calls in if/then to only log success when the command actually succeeds, matching the chown pattern (CodeRabbit review) - Replace 'leaving as-is' branch with backup+replace logic: move the non-symlink identity path aside with a timestamped backup, then create the correct symlink (CodeRabbit review)
All echo statements in the non-root startup block must write to stderr to avoid leaking diagnostics into bridge stdout. Four echo lines added in the previous commit were missing the >&2 redirect. Signed-off-by: kagura-agent <kagura.chen28@gmail.com>
kagura-agent
force-pushed
the
fix/692-identity-dir-permissions-v2
branch
from
82ae90c to
32fd438
Compare
Extract ensure_identity_symlink helper with early returns for each case (correct symlink, missing, wrong target, non-symlink entry) instead of nested if/elif/else. Also fixes SC2155 shellcheck warning. Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…link Tests ensure_identity_symlink (5 cases: create, no-op, already correct, wrong target repair, non-symlink backup) and fix_openclaw_data_ownership (subdirectory creation, no-op when .openclaw-data absent). Extracts function bodies from nemoclaw-start.sh to test in isolation without sourcing the full startup script. Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The CI container's /etc/bash.bashrc references $PS1 under set -u, producing stderr noise that broke "no output" assertions. Set PS1 and BASH_ENV in the test env to prevent bashrc side effects. Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
CI container's /etc/bash.bashrc emits "PS1: unbound variable" to stderr regardless of env settings. Check for absence of our "[setup]" prefix instead of requiring empty stderr. Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Missed one occurrence — "does nothing when identity dir does not exist" also needs the container-safe assertion. Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ra-agent/NemoClaw into fix/692-identity-dir-permissions-v2
4 tasks
cv
added a commit
that referenced
this pull request
Apr 8, 2026
…salvage (#1585) ## Summary - Adds 8 agent skills and 4 TypeScript scripts that automate the maintainer workflow: triage, gate-check, salvage, security sweep, test gaps, hotspots, sequencing - Skills are designed for `/loop 10m /nemoclaw-maintainer-loop` continuous operation - Scripts make triage scoring, gate checking, and hotspot detection deterministic (call `gh-pr-merge-now` as data engine) - Battle-tested in this session: triaged queue, salvaged 3 PRs (#1359, #898, #1089), merged all 4 including #1139 ## Test plan - [ ] Invoke `/nemoclaw-maintainer-loop` and verify triage output - [ ] Run `node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-loop/scripts/triage.ts --approved-only` - [ ] Run `node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-loop/scripts/check-gates.ts <pr-number>` - [ ] Verify skills YAML passes `prek` validation (tested in pre-commit) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added automated triage, prioritization, and merge-gate checks plus version/straggler tooling to streamline daily maintainer flows. * Added end-to-end maintainer skills for morning/day/evening run-books and release handoff guidance. * **Documentation** * Added workflows for PR review priorities, hotspot cooling, salvage PRs, security sweeps, sequencing work, and closing test gaps. * Added persistent state schema and guidance for maintainer loop operations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
…VIDIA#692) (NVIDIA#1089) Resubmitted as requested by @mnsami (original: NVIDIA#698). ## Problem When NemoClaw is installed via the curl installer on Linux, `.openclaw-data` directories may be created with root ownership. When openclaw runs as the `sandbox` user, it gets `EACCES: permission denied` trying to write `device-auth.json`. ## Fix Add a startup ownership check to `nemoclaw-start.sh` (non-root path) that: - Creates any missing writable subdirectories (mirrors Dockerfile setup) - Fixes ownership if files are not owned by the current user - Creates the identity symlink if missing on native installs The Docker path is unaffected (Dockerfile already sets correct ownership). ## Testing All pre-commit hooks pass (shellcheck, shfmt, gitleaks, etc). 420/422 unit tests pass; 2 pre-existing failures in `install-preflight.test.js` are unrelated (Node.js version detection message mismatch). Fixes NVIDIA#692 Signed-off-by: kagura-agent <kagura.chen28@gmail.com> --------- Signed-off-by: kagura-agent <kagura.chen28@gmail.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
gemini2026
pushed a commit
to gemini2026/NemoClaw
that referenced
this pull request
Apr 14, 2026
…salvage (NVIDIA#1585) ## Summary - Adds 8 agent skills and 4 TypeScript scripts that automate the maintainer workflow: triage, gate-check, salvage, security sweep, test gaps, hotspots, sequencing - Skills are designed for `/loop 10m /nemoclaw-maintainer-loop` continuous operation - Scripts make triage scoring, gate checking, and hotspot detection deterministic (call `gh-pr-merge-now` as data engine) - Battle-tested in this session: triaged queue, salvaged 3 PRs (NVIDIA#1359, NVIDIA#898, NVIDIA#1089), merged all 4 including NVIDIA#1139 ## Test plan - [ ] Invoke `/nemoclaw-maintainer-loop` and verify triage output - [ ] Run `node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-loop/scripts/triage.ts --approved-only` - [ ] Run `node --experimental-strip-types --no-warnings .agents/skills/nemoclaw-maintainer-loop/scripts/check-gates.ts <pr-number>` - [ ] Verify skills YAML passes `prek` validation (tested in pre-commit) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added automated triage, prioritization, and merge-gate checks plus version/straggler tooling to streamline daily maintainer flows. * Added end-to-end maintainer skills for morning/day/evening run-books and release handoff guidance. * **Documentation** * Added workflows for PR review priorities, hotspot cooling, salvage PRs, security sweeps, sequencing work, and closing test gaps. * Added persistent state schema and guidance for maintainer loop operations. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.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
3 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.
Resubmitted as requested by @mnsami (original: #698).
Problem
When NemoClaw is installed via the curl installer on Linux,
.openclaw-datadirectories may be created with root ownership. When openclaw runs as thesandboxuser, it getsEACCES: permission deniedtrying to writedevice-auth.json.Fix
Add a startup ownership check to
nemoclaw-start.sh(non-root path) that:The Docker path is unaffected (Dockerfile already sets correct ownership).
Testing
All pre-commit hooks pass (shellcheck, shfmt, gitleaks, etc). 420/422 unit tests pass; 2 pre-existing failures in
install-preflight.test.jsare unrelated (Node.js version detection message mismatch).Fixes #692
Signed-off-by: kagura-agent kagura.chen28@gmail.com