fix(blueprint): chown copied files to sandbox user after restore (#1229)#1667
Conversation
|
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 (1)
📝 WalkthroughWalkthrough
Changes
Sequence Diagram(s)sequenceDiagram
participant CL as Caller
participant OS as OpenShell CLI
participant SB as Sandbox (container filesystem)
CL->>OS: sandbox cp <bundle> <sandbox>:/sandbox/.openclaw
alt cp fails (exitCode != 0)
OS-->>CL: non-zero exitCode -> restoreIntoSandbox returns false
else cp succeeds
OS-->>SB: files copied as root:root
CL->>OS: sandbox exec <sandbox> -- chown -R sandbox:sandbox /sandbox/.openclaw-data (reject:false)
OS-->>CL: chown result (ignored)
CL-->>CL: restoreIntoSandbox returns true
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
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 `@nemoclaw/src/blueprint/snapshot.ts`:
- Around line 110-123: Add regression tests that cover the new best-effort chown
branch: in tests for the code that calls execa("openshell", ["sandbox","exec",
sandboxName, "--", "chown", "-R", "sandbox:sandbox",
"/sandbox/.openclaw-data"]), assert that the second execa invocation is executed
(mock/stub execa and verify it was called with the exact args including
sandboxName and "/sandbox/.openclaw-data") and assert that when that chown
invocation fails (simulate non-zero exit via reject:false behavior or a thrown
error) the surrounding function still returns true; add tests co-located with
snapshot.ts (and follow the project test pattern for
{nemoclaw/src/blueprint,bin/lib}/*) so the security-sensitive ownership
correction path is covered.
🪄 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: 3dac8d77-f735-418a-848a-bb62e8e7e9e0
📒 Files selected for processing (1)
nemoclaw/src/blueprint/snapshot.ts
| await execa( | ||
| "openshell", | ||
| [ | ||
| "sandbox", | ||
| "exec", | ||
| sandboxName, | ||
| "--", | ||
| "chown", | ||
| "-R", | ||
| "sandbox:sandbox", | ||
| "/sandbox/.openclaw-data", | ||
| ], | ||
| { reject: false }, | ||
| ); |
There was a problem hiding this comment.
Add regression tests for the new best-effort chown branch.
Line 110-Line 123 introduces a security-sensitive ownership correction, but there’s no co-located test asserting (1) the second execa call is made and (2) a failing chown still returns true.
Suggested test additions
diff --git a/nemoclaw/src/blueprint/snapshot.test.ts b/nemoclaw/src/blueprint/snapshot.test.ts
@@
it("calls openshell sandbox cp and returns true on success", async () => {
addDir(`${SNAP}/openclaw`);
mockExeca.mockResolvedValue({ exitCode: 0 });
expect(await restoreIntoSandbox(SNAP, "mybox")).toBe(true);
@@
});
+ it("runs best-effort chown after successful copy", async () => {
+ addDir(`${SNAP}/openclaw`);
+ mockExeca
+ .mockResolvedValueOnce({ exitCode: 0 }) // cp
+ .mockResolvedValueOnce({ exitCode: 1 }); // chown (ignored)
+
+ expect(await restoreIntoSandbox(SNAP, "mybox")).toBe(true);
+ expect(mockExeca).toHaveBeenNthCalledWith(
+ 2,
+ "openshell",
+ [
+ "sandbox",
+ "exec",
+ "mybox",
+ "--",
+ "chown",
+ "-R",
+ "sandbox:sandbox",
+ "/sandbox/.openclaw-data",
+ ],
+ { reject: false },
+ );
+ });As per coding guidelines {nemoclaw/src/blueprint,bin/lib}/**/*.{js,ts}: Security-sensitive code paths in isolation/sandbox features must have extra test coverage to prevent credential leaks and sandbox escapes.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await execa( | |
| "openshell", | |
| [ | |
| "sandbox", | |
| "exec", | |
| sandboxName, | |
| "--", | |
| "chown", | |
| "-R", | |
| "sandbox:sandbox", | |
| "/sandbox/.openclaw-data", | |
| ], | |
| { reject: false }, | |
| ); | |
| it("calls openshell sandbox cp and returns true on success", async () => { | |
| addDir(`${SNAP}/openclaw`); | |
| mockExeca.mockResolvedValue({ exitCode: 0 }); | |
| expect(await restoreIntoSandbox(SNAP, "mybox")).toBe(true); | |
| }); | |
| it("runs best-effort chown after successful copy", async () => { | |
| addDir(`${SNAP}/openclaw`); | |
| mockExeca | |
| .mockResolvedValueOnce({ exitCode: 0 }) // cp | |
| .mockResolvedValueOnce({ exitCode: 1 }); // chown (ignored) | |
| expect(await restoreIntoSandbox(SNAP, "mybox")).toBe(true); | |
| expect(mockExeca).toHaveBeenNthCalledWith( | |
| 2, | |
| "openshell", | |
| [ | |
| "sandbox", | |
| "exec", | |
| "mybox", | |
| "--", | |
| "chown", | |
| "-R", | |
| "sandbox:sandbox", | |
| "/sandbox/.openclaw-data", | |
| ], | |
| { reject: false }, | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemoclaw/src/blueprint/snapshot.ts` around lines 110 - 123, Add regression
tests that cover the new best-effort chown branch: in tests for the code that
calls execa("openshell", ["sandbox","exec", sandboxName, "--", "chown", "-R",
"sandbox:sandbox", "/sandbox/.openclaw-data"]), assert that the second execa
invocation is executed (mock/stub execa and verify it was called with the exact
args including sandboxName and "/sandbox/.openclaw-data") and assert that when
that chown invocation fails (simulate non-zero exit via reject:false behavior or
a thrown error) the surrounding function still returns true; add tests
co-located with snapshot.ts (and follow the project test pattern for
{nemoclaw/src/blueprint,bin/lib}/*) so the security-sensitive ownership
correction path is covered.
wscurran
added
OpenShell
|
✨ Thanks for submitting this PR, which proposes a way to fix the file ownership issue in the sandbox and may help resolve issues related to file access and permissions. Possibly related open issues: |
ColinM-sys
force-pushed
the
fix/1229-chown-after-restore
branch
from
1a18f62 to
58c8278
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw/src/blueprint/snapshot.ts (1)
110-123: Consider logging chown failures for observability.The chown is intentionally best-effort, but discarding the result entirely makes it hard to diagnose when writes later fail due to ownership issues. A debug-level log on non-zero exit would aid troubleshooting without violating the "don't fail the restore" semantics.
💡 Optional: capture and log chown result
- await execa( + const chownResult = await execa( "openshell", [ "sandbox", "exec", sandboxName, "--", "chown", "-R", "sandbox:sandbox", "/sandbox/.openclaw-data", ], { reject: false }, ); + if (chownResult.exitCode !== 0) { + // Best-effort: log but don't fail + console.debug( + `chown in sandbox ${sandboxName} exited ${chownResult.exitCode}: ${chownResult.stderr}`, + ); + } return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/blueprint/snapshot.ts` around lines 110 - 123, Capture the result of the execa call in snapshot.ts (e.g., const result = await execa(...)) instead of discarding it, then check result.exitCode (or result.failed) and, on non-zero exit, emit a debug-level log including sandboxName and the command output (result.stderr / result.stdout) so chown failures are observable while preserving the best-effort semantics of the existing chown call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw/src/blueprint/snapshot.ts`:
- Around line 110-123: Capture the result of the execa call in snapshot.ts
(e.g., const result = await execa(...)) instead of discarding it, then check
result.exitCode (or result.failed) and, on non-zero exit, emit a debug-level log
including sandboxName and the command output (result.stderr / result.stdout) so
chown failures are observable while preserving the best-effort semantics of the
existing chown call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11a498b2-76ae-4c38-b3da-2ce5056bbe7b
📒 Files selected for processing (1)
nemoclaw/src/blueprint/snapshot.ts
|
We appreciate this fix — ensuring copied files are chowned to the sandbox user after restore is the kind of detail that prevents subtle permission failures at runtime. The codebase has changed significantly since this was opened, including a TypeScript migration (#1673) and OpenShell updates beyond v0.0.16. Could you rebase onto the current main? And since you also have #1656, #1676, and #1677 open, a joint rebase across all four would be ideal. |
wscurran
added
the
status: rebase
`openshell sandbox cp` runs as root inside the pod, so files copied into /sandbox/.openclaw via restoreIntoSandbox land as root:root. The symlinks under /sandbox/.openclaw point at the writable /sandbox/.openclaw-data tree, so without an explicit chown the agent workspace and per-agent runtime dirs end up unwritable by the sandbox user. That broke writes to models.json, agent state, and workspace markdown files with `EACCES: permission denied`. After a successful cp, run a best-effort recursive `chown -R sandbox:sandbox /sandbox/.openclaw-data` via `openshell sandbox exec`. The chown is intentionally best-effort (its failure does NOT flip the restore result) so a future runtime that already gets ownership right won't break the migration. Tests: snapshot.test.ts has pre-existing vitest 4.x mocking breakage on main (8/16 tests fail before this change, same 8/16 fail after — no new regressions). The mock-fs harness needs a separate fix; once that lands, regression tests can be added that assert the chown call shape and the best-effort semantics. Refs: NVIDIA#1229 Signed-off-by: ColinM-sys <cmcdonough@50words.com>
ColinM-sys
force-pushed
the
fix/1229-chown-after-restore
branch
from
58c8278 to
80f59f6
Compare
ericksoa
left a comment
ericksoa
left a comment
There was a problem hiding this comment.
LGTM — correct fix for the root:root ownership bug after sandbox cp. Best-effort chown semantics are right. We'll follow up separately with debug logging and test coverage once the snapshot test harness is repaired.
…store Follow-up to #1667 — the chown-after-restore path had no test coverage and silently discarded failures. Add three tests (chown called, chown failure still returns true, chown skipped on cp failure) and a console.debug breadcrumb when chown exits non-zero. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…store (#1899) ## Summary Follow-up to #1667 — the best-effort chown-after-restore path had no test coverage and silently discarded failures. - Add three tests for the chown behavior in `restoreIntoSandbox`: - chown is called after successful cp with correct args - chown failure still returns true (best-effort semantics) - chown is not called when cp fails - Add `console.debug` breadcrumb when chown exits non-zero, so ownership failures leave a trace instead of vanishing silently ## Test plan - [x] `npx vitest run nemoclaw/src/blueprint/snapshot.test.ts` — 19/19 pass - [x] `npm run build` (plugin) — clean - [x] `make check` — all hooks pass - [x] Pre-existing `test/policies.test.ts` failures (15) confirmed on main — unrelated Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **Improvements** * Enhanced sandbox restoration process with improved logging and error handling for permission operations. * **Tests** * Added comprehensive test coverage for sandbox restoration scenarios, including success, partial failure, and error cases. * **Documentation** * Added documentation for AI-assisted label triage workflow and guidelines. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Thanks Aaron! Glad the best-effort approach is the right call. |
4 participants
Summary
restoreIntoSandbox()callsopenshell sandbox cp, run a best-effortchown -R sandbox:sandbox /sandbox/.openclaw-dataso the writable side of the symlink tree is owned by the sandbox user.Fixes #1229.
Why
openshell sandbox cpruns as root inside the pod, so files copied into/sandbox/.openclawviarestoreIntoSandboxland asroot:root. The symlinks under/sandbox/.openclawpoint at the writable/sandbox/.openclaw-datatree, so without an explicit chown the agent workspace and per-agent runtime dirs end up unwritable by the sandbox user. The reporter saw:Test plan
nemoclaw/src/blueprint/snapshot.test.tshas pre-existing vitest 4.x mocking breakage onmain— 8 of its 16 tests fail before this change because thevi.mock("node:fs", ...)harness doesn't return what the production code expects under vitest 4.x. The failure surfaces asexpected null not to be nullfromcreateSnapshot()and identical patterns inrestoreIntoSandbox. This change adds zero new failures (8/16 fail before, still 8/16 fail after — same tests). I held back regression tests for the chown call shape because they would have hit the same mock-fs issue. Happy to add them in a follow-up PR once the snapshot test harness is repaired (which is itself worth tracking as a separate issue).Summary by CodeRabbit
Signed-off-by: ColinM-sys cmcdonough@50words.com