refactor(sandbox): default to mutable config, make shields opt-in - #2227
Merged
Conversation
Flip the sandbox default from immutable to mutable. Config starts as 600 sandbox:sandbox so agents can natively manage their own config, install skills, and write to standard home-directory paths without workarounds. Shields UP becomes an explicit opt-in for operators who want config immutability on sensitive workloads. Shields DOWN returns to the mutable default. What this removes: - --dangerously-skip-permissions flag and shieldsDownPermanent() - config set / config rotate-token commands (edit config natively) - Startup chattr +i hardening and symlink validation - 14 tool cache redirects to /tmp (home is now writable) - Skill install dual-write mirror workaround - .openclaw-data directory and symlink bridge (merged into .openclaw) What stays: - Shields up/down toggle via kubectl exec (DAC + chattr) - Network policy system, presets, and operator approval - config get (read-only) - Audit logging, SSRF validation, secret scanning - Config integrity hash verification at startup
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRemoves the permissive skip-permissions flow and nightly E2E job; consolidates agent state into unified writable directories ( Changes
Sequence Diagram(s)sequenceDiagram
participant Entrypoint as Entrypoint (container)
participant TokenGen as TokenGenerator
participant Gateway as GatewayProcess
participant Operator as Nemoclaw CLI
participant Sandbox as Sandbox User
Entrypoint->>TokenGen: generate_gateway_token()
TokenGen-->>Entrypoint: export OPENCLAW_GATEWAY_TOKEN
Entrypoint->>Gateway: start (env includes token)
Operator->>Entrypoint: shields up / shields down
alt shields up
Entrypoint->>Entrypoint: lock files (chown root, chattr +i)
Entrypoint-->>Sandbox: `.openclaw` enforced locked
else shields down
Entrypoint->>Entrypoint: unlock files (chown sandbox, chattr -i)
Entrypoint-->>Sandbox: `.openclaw` writable by sandbox
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Upstream commits: - feat(snapshot): add --name flag and version restore selectors (#2184) - fix: reject unrecognized config paths in config set (#2036) Conflict resolution: - src/lib/sandbox-config.ts: keep ours (configSet and helpers removed) - test/config-set.test.ts: keep deleted (command removed) - test/e2e/test-shields-config.sh: keep ours (rewritten for mutable default) The config-set path validation fix (#2036) is moot since config set no longer exists — agents edit config natively in the mutable default.
- test/nemoclaw-start.test.ts: replace validate_openclaw_symlinks boundary with write_auth_profile (symlink validation was removed) - scripts/nemoclaw-start.sh: update configure guard comments and error messages — config is mutable by default, not Landlock read-only - test/e2e/test-hermes-e2e.sh: flip immutability assertion to expect writable config directory (mutable default)
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/sandbox-state.ts (1)
44-53:⚠️ Potential issue | 🟠 MajorRestore compatibility break for existing backups (
writableDir→dir).Line 529 reads
manifest.dironly. Backups created before this schema rename will havewritableDir, so restore can fail with an undefined target path.Backward-compatible fix
export interface RebuildManifest { version: number; @@ - /** Single config/state directory */ - dir: string; + /** Single config/state directory (new schema) */ + dir?: string; + /** Backward compatibility for pre-migration backups */ + writableDir?: string; @@ export function restoreSandboxState( @@ - const dir = manifest.dir; + const dir = manifest.dir || manifest.writableDir; + if (!dir) { + _log("FAILED: Manifest missing dir/writableDir"); + return { success: false, restoredDirs: [], failedDirs: [...manifest.stateDirs] }; + }Also applies to: 529-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-state.ts` around lines 44 - 53, The manifest schema change renamed writableDir to dir but restore code still reads manifest.dir, breaking older backups; update the RebuildManifest handling to accept both fields and fallback to the legacy name: keep the interface (RebuildManifest) but treat manifest.dir = manifest.dir || manifest.writableDir (or normalize when loading/parsing), and update any code paths that reference manifest.dir (the restore/restore-target resolution) to use the normalized value so old backups with writableDir continue to work.docs/reference/troubleshooting.md (1)
475-485:⚠️ Potential issue | 🟠 Major
rebuildis the wrong remediation for config changes.The sandbox guard in
scripts/nemoclaw-start.sh:495-567tells users to exit and runnemoclaw onboard --resume.nemoclaw <sandbox> rebuildrecreates the sandbox from the existing baked config, so the current instructions will leave/sandbox/.openclaw/openclaw.jsonunchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/troubleshooting.md` around lines 475 - 485, The remediation text is incorrect: replace the current advice that tells users to run "nemoclaw <sandbox> rebuild" with instructions to exit the sandbox and run "nemoclaw onboard --resume" from the host (as the sandbox guard in nemoclaw-start.sh expects) because "rebuild" recreates the sandbox from the existing baked config and will not update /sandbox/.openclaw/openclaw.json; update the troubleshooting entry for the "openclaw config set/unset is blocked" section to explain that users must exit and resume onboarding to change the baked OpenClaw configuration and explicitly state that "nemoclaw <sandbox> rebuild" does not apply.src/nemoclaw.ts (1)
1355-1367:⚠️ Potential issue | 🔴 CriticalUndefined variable
sbreferenced at line 1360.The variable
sbat line 1360 (const agentName = sb?.agent || "openclaw") is not defined in this scope. The onlysbdefinition insandboxConnectis at line 1244, which is inside a separate try-catch block and not accessible at line 1360.This will cause a
ReferenceErrorat runtime whenprocess.stdout.isTTYis true andNEMOCLAW_NO_CONNECT_HINTis not set.🐛 Proposed fix
if ( process.stdout.isTTY && !["1", "true"].includes(String(process.env.NEMOCLAW_NO_CONNECT_HINT || "")) ) { console.log(""); - const agentName = sb?.agent || "openclaw"; + const sb = registry.getSandbox(sandboxName); + const agentName = sb?.agent || "openclaw"; const agentCmd = agentName === "openclaw" ? "openclaw tui" : agentName;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1355 - 1367, The code references an out-of-scope variable sb (used in const agentName = sb?.agent || "openclaw") causing a ReferenceError; fix by either (A) replacing sb with the sandbox info variable that exists in this scope (e.g., sandbox or sandboxInfo) if one holds the sandbox metadata, or (B) when sb is created inside sandboxConnect, assign its agent to an outer-scope const agentName (e.g., agentName = sb.agent || "openclaw") so this block can read agentName; update the reference in this block (the const agentName = ...) to use that scoped variable instead of sb.test/e2e-gateway-isolation.sh (1)
239-259:⚠️ Potential issue | 🟡 MinorDuplicate test numbers: Tests 13 and 14 appear twice.
Lines 239-259 define Tests 13 and 14, but these numbers were already used earlier at lines 192-237. This causes confusion and may affect test result interpretation.
Proposed fix: Renumber the duplicate tests
-# ── Test 13: Sandbox user cannot write to .nemoclaw parent ──────── +# ── Test 28: Sandbox user cannot write to .nemoclaw parent ──────── # Note: /sandbox itself is sandbox-owned (DAC allows writes). Landlock makes it # read-only in production — tested in checks/04-landlock-readonly.sh instead. -info "13. Sandbox user cannot create files in /sandbox/.nemoclaw" +info "28. Sandbox user cannot create files in /sandbox/.nemoclaw" OUT=$(run_as_sandbox "touch /sandbox/.nemoclaw/testfile 2>&1 || echo BLOCKED") if echo "$OUT" | grep -q "BLOCKED\|Permission denied"; then pass "sandbox cannot create files in .nemoclaw parent (root-owned)" else fail "sandbox CAN create files in .nemoclaw parent: $OUT" fi -# ── Test 14: Sandbox user cannot modify blueprints ──────────────── +# ── Test 29: Sandbox user cannot modify blueprints ──────────────── -info "14. Sandbox user cannot modify blueprints" +info "29. Sandbox user cannot modify blueprints"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-gateway-isolation.sh` around lines 239 - 259, Rename the duplicated test numbers in the second block so they don’t clash with the earlier tests: update the comment header "# ── Test 13: Sandbox user cannot create files in /sandbox/.nemoclaw" and its info line info "13. Sandbox user cannot create files in /sandbox/.nemoclaw" to use the next available number (e.g. 15), and update the following comment header "# ── Test 14: Sandbox user cannot modify blueprints" and its info line info "14. Sandbox user cannot modify blueprints" to the next number after that (e.g. 16), ensuring the header comments and the info strings match and are changed consistently.
🧹 Nitpick comments (5)
docs/.docs-skip (1)
48-49: Consider removing stale skip-terms.Since
config rotate-tokenhas been removed from the codebase (per PR objectives), these skip-terms entries may no longer be necessary. However, keeping them is harmless and provides defense-in-depth against accidental mention of removed features in generated docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/.docs-skip` around lines 48 - 49, The skip-terms list still contains the stale entries "config rotate-token" and "rotate-token"; remove those two lines from the skip-terms file (or alternatively add an inline comment explaining why you intentionally keep them for defense-in-depth) so the skip list reflects current features and won't hide references to removed commands.nemoclaw/src/security/secret-scanner.test.ts (1)
189-203: Remove duplicate test cases for the same assertions.
matches .openclaw/memory/ pathsandmatches MEMORY.md anchored to .openclaware each defined twice with identical expectations. Keeping one of each will make this suite cleaner.♻️ Proposed cleanup
- it("matches .openclaw/memory/ paths", () => { - expect(isMemoryPath("/sandbox/.openclaw/memory/notes.md")).toBe(true); - }); - it("matches MEMORY.md anchored to .openclaw", () => { expect(isMemoryPath("/sandbox/.openclaw/MEMORY.md")).toBe(true); }); - - it("matches MEMORY.md anchored to .openclaw", () => { - expect(isMemoryPath("/sandbox/.openclaw/MEMORY.md")).toBe(true); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/security/secret-scanner.test.ts` around lines 189 - 203, Remove the duplicate test cases that assert the same behavior for isMemoryPath: keep a single test for the ".openclaw/memory/ paths" assertion and a single test for the "MEMORY.md anchored to .openclaw" assertion, deleting the repeated blocks so each expectation appears exactly once; ensure the remaining tests still call isMemoryPath("/sandbox/.openclaw/memory/project.md") or "/notes.md" (choose one) and isMemoryPath("/sandbox/.openclaw/MEMORY.md") respectively and retain their descriptive it(...) strings.agents/hermes/policy-additions.yaml (1)
6-6: Fix stale path wording in policy comment.Line 6 still references
.hermes-dataand repeats.openclawon the old side, which is confusing after the single-directory migration.Suggested comment-only cleanup
-# - .hermes / .hermes-data instead of .openclaw / .openclaw +# - .hermes instead of .openclaw (single config/state directory)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents/hermes/policy-additions.yaml` at line 6, Update the policy comment that currently reads ".hermes / .hermes-data instead of .openclaw / .openclaw" to reflect the single-directory migration: replace the stale wording with a clear statement like ".hermes (formerly .openclaw)" or ".hermes instead of .openclaw" so it no longer repeats ".openclaw" and accurately indicates the new single directory; locate and edit the comment in agents/hermes/policy-additions.yaml where the ".hermes-data" / ".openclaw" text appears.test/e2e/test-shields-config.sh (1)
414-447: Auto-restore timer test may be flaky in CI environments.The 25s wait for a 10s timer provides reasonable buffer, but the test acknowledges this with the fallback path at lines 433-437. The info message at line 433 correctly notes that "timer runs as detached process" which may not always complete predictably.
Consider increasing the wait margin or adding a retry loop if this test proves flaky in CI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-shields-config.sh` around lines 414 - 447, The auto-restore timer check using a fixed sleep (sleep 25) is flaky; replace the static wait with a retry/poll loop that queries "nemoclaw \"${SANDBOX_NAME}\" shields status" (the STATUS_AFTER_TIMER check) until either the shields show "DOWN" or "mutable" or a larger overall timeout (e.g., 60s) is reached, polling every few seconds; after success break and run the existing PERMS_TIMER stat check on "${CONFIG_PATH}", and on timeout fall back to the existing manual cleanup (nemoclaw ... shields down) and fail path—update references to STATUS_AFTER_TIMER, PERMS_TIMER, nemoclaw shields status/up/down and CONFIG_PATH accordingly.docs/deployment/sandbox-hardening.md (1)
109-109: One sentence per line.Line 109 contains multiple sentences on the same line, which makes diffs harder to read.
LLM pattern detected.
Proposed fix
-Landlock LSM requires Linux kernel 5.13 or later with `CONFIG_SECURITY_LANDLOCK=y`. -The NemoClaw sandbox policy uses `compatibility: best_effort`, which means Landlock enforcement is silently skipped on kernels that do not support it. +Landlock LSM requires Linux kernel 5.13 or later with `CONFIG_SECURITY_LANDLOCK=y`. +The NemoClaw sandbox policy uses `compatibility: best_effort`, which means Landlock enforcement is silently skipped on kernels that do not support it.(Split into two source lines while preserving the rendered paragraph.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/deployment/sandbox-hardening.md` at line 109, The line containing "The NemoClaw sandbox policy uses `compatibility: best_effort`, which means Landlock enforcement is silently skipped on kernels that do not support it." should be split into two source lines so each sentence is on its own line: one line with "The NemoClaw sandbox policy uses `compatibility: best_effort`." and a second line with "This means Landlock enforcement is silently skipped on kernels that do not support it." Preserve the exact wording and formatting (including the inline code span) so the rendered paragraph remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents/hermes/Dockerfile`:
- Around line 69-71: The shields-up implementation currently only locks
configPath/configDir returned by resolveAgentConfig() leaving .env and
.config-hash writable; update the fix by making resolveAgentConfig() (in
sandbox-config.ts) return all three artifact paths (configPath, envFile,
hashFile) OR modify lockAgentConfig()/unlockAgentConfig() (in shields.ts) to
explicitly compute and lock/unlock the envFile and hashFile alongside configPath
and configDir; ensure you reference the same identifiers used in the diff
(configPath, configDir, envFile, hashFile) when adding the extra
fs.chmod/fs.chown or locking logic so .env and .config-hash are made immutable
during shields-up and restored on shields-down.
In `@agents/hermes/policy-permissive.yaml`:
- Around line 26-31: The test failure is caused by making /sandbox/.hermes
writable in policy-permissive.yaml (the read_write entry for /sandbox/.hermes),
while test/e2e/test-hermes-e2e.sh still asserts that /sandbox/.hermes is
immutable; either revert /sandbox/.hermes to read-only in the read_write list or
update test/e2e/test-hermes-e2e.sh (the assertion around touching
/sandbox/.hermes/test-write) to expect a successful write; pick one approach and
update the corresponding file (policy-permissive.yaml's read_write block or the
test's expectation/assertion) so the policy and test are consistent.
In `@agents/hermes/start.sh`:
- Around line 84-91: The integrity check currently reads .config-hash from
mutable HERMES_DIR; change verify_config_integrity to use a separate, immutable
hash location (e.g. a HERMES_HASH_FILE outside HERMES_DIR) and/or enforce that
the hash file is root-owned and not writable by the agent before validating it;
specifically, update verify_config_integrity to reference the new
HERMES_HASH_FILE (instead of "${HERMES_DIR}/.config-hash"), and add pre-checks
that the file exists, is owned by UID 0 (root) and has restrictive permissions
(e.g., mode 0400 or not group/other-writable) and fail startup if those checks
do not pass so an agent-controlled user cannot replace both config and hash.
- Around line 79-82: HERMES_HOME (/sandbox/.hermes / HERMES_DIR) is set to a
directory owned 700 sandbox:sandbox which prevents the unprivileged gateway user
from reading config when we later run "gosu gateway ..."; change the startup
logic that sets HERMES_HOME (both the root-privilege-drop block and the non-root
fallback that export HERMES_HOME) so that either (a) the gateway user is granted
read/execute access to the config directory (adjust ownership/permissions to
allow gateway read/exec but still restrict writes), or (b) move runtime state
files (PID, state.db, .channel_directory) into a separate writable directory
(e.g., HERMES_STATE) owned by sandbox while keeping HERMES_HOME
immutable/readable by gateway; also separate the .config-hash into an immutable
or root-owned location (not the mutable config dir) so the sandbox user cannot
tamper with configs and precompute the hash before restart. Ensure references to
HERMES_HOME / HERMES_DIR and the .config-hash handling are updated consistently
in both code paths.
In `@nemoclaw/src/security/secret-scanner.ts`:
- Around line 109-123: The MEMORY_PATH_SEGMENTS list in secret-scanner.ts omits
openclaw.json, letting writes to /.openclaw/openclaw.json bypass isMemoryPath()
checks; update the MEMORY_PATH_SEGMENTS constant to include either the broad
"/.openclaw/" segment (preferred) or explicitly "/.openclaw/openclaw.json" so
that isMemoryPath() and the scanner catch config writes and prevent persistent
leakage of secrets; modify the MEMORY_PATH_SEGMENTS array (referenced by
isMemoryPath()) accordingly and run tests to ensure existing path matching still
behaves as expected.
In `@src/lib/shields.ts`:
- Around line 424-434: The code currently treats a missing shields state as
"shields up"; change the guards so absence of a state file is treated as mutable
(shields down) by default: replace the early-return check that uses if
(!state.shieldsDown) with a check that first ensures state exists and only
returns when state.shieldsDown is true (e.g., if (state && state.shieldsDown) {
... return; }), and apply the same default-exists logic to the other related
branches referenced (around the snapshotPath handling and lines 504–507) so
missing state does not short-circuit into reporting lockdown.
In `@test/e2e/e2e-cloud-experimental/checks/04-landlock-readonly.sh`:
- Around line 58-65: The test mutates the real /sandbox/.bashrc by appending
"echo '# test' >> /sandbox/.bashrc" (seen in the sandbox_exec call and OUT
check) and never reverts it; change the test to write to a disposable file
(e.g., /sandbox/.bashrc.test or /sandbox/.bashrc.$$) or ensure the appended line
is removed in the test cleanup (reverse the append with sed -i '/# test$/d'
/sandbox/.bashrc or rm the temp file), and apply the same fix to the other
occurrence referenced in the comment (the block around the other check).
In `@test/exec-approvals-path-regression.test.ts`:
- Around line 27-29: The test expectation for the permission mode is incorrect:
update the assertion that checks src for the chmod command to expect "chmod 700
/sandbox/.openclaw" instead of "chmod 755 /sandbox/.openclaw" (i.e., modify the
expect(src).toContain(...) call that currently checks for 755 to check for 700)
so it matches the Dockerfile's chmod 700 behavior.
---
Outside diff comments:
In `@docs/reference/troubleshooting.md`:
- Around line 475-485: The remediation text is incorrect: replace the current
advice that tells users to run "nemoclaw <sandbox> rebuild" with instructions to
exit the sandbox and run "nemoclaw onboard --resume" from the host (as the
sandbox guard in nemoclaw-start.sh expects) because "rebuild" recreates the
sandbox from the existing baked config and will not update
/sandbox/.openclaw/openclaw.json; update the troubleshooting entry for the
"openclaw config set/unset is blocked" section to explain that users must exit
and resume onboarding to change the baked OpenClaw configuration and explicitly
state that "nemoclaw <sandbox> rebuild" does not apply.
In `@src/lib/sandbox-state.ts`:
- Around line 44-53: The manifest schema change renamed writableDir to dir but
restore code still reads manifest.dir, breaking older backups; update the
RebuildManifest handling to accept both fields and fallback to the legacy name:
keep the interface (RebuildManifest) but treat manifest.dir = manifest.dir ||
manifest.writableDir (or normalize when loading/parsing), and update any code
paths that reference manifest.dir (the restore/restore-target resolution) to use
the normalized value so old backups with writableDir continue to work.
In `@src/nemoclaw.ts`:
- Around line 1355-1367: The code references an out-of-scope variable sb (used
in const agentName = sb?.agent || "openclaw") causing a ReferenceError; fix by
either (A) replacing sb with the sandbox info variable that exists in this scope
(e.g., sandbox or sandboxInfo) if one holds the sandbox metadata, or (B) when sb
is created inside sandboxConnect, assign its agent to an outer-scope const
agentName (e.g., agentName = sb.agent || "openclaw") so this block can read
agentName; update the reference in this block (the const agentName = ...) to use
that scoped variable instead of sb.
In `@test/e2e-gateway-isolation.sh`:
- Around line 239-259: Rename the duplicated test numbers in the second block so
they don’t clash with the earlier tests: update the comment header "# ── Test
13: Sandbox user cannot create files in /sandbox/.nemoclaw" and its info line
info "13. Sandbox user cannot create files in /sandbox/.nemoclaw" to use the
next available number (e.g. 15), and update the following comment header "# ──
Test 14: Sandbox user cannot modify blueprints" and its info line info "14.
Sandbox user cannot modify blueprints" to the next number after that (e.g. 16),
ensuring the header comments and the info strings match and are changed
consistently.
---
Nitpick comments:
In `@agents/hermes/policy-additions.yaml`:
- Line 6: Update the policy comment that currently reads ".hermes / .hermes-data
instead of .openclaw / .openclaw" to reflect the single-directory migration:
replace the stale wording with a clear statement like ".hermes (formerly
.openclaw)" or ".hermes instead of .openclaw" so it no longer repeats
".openclaw" and accurately indicates the new single directory; locate and edit
the comment in agents/hermes/policy-additions.yaml where the ".hermes-data" /
".openclaw" text appears.
In `@docs/.docs-skip`:
- Around line 48-49: The skip-terms list still contains the stale entries
"config rotate-token" and "rotate-token"; remove those two lines from the
skip-terms file (or alternatively add an inline comment explaining why you
intentionally keep them for defense-in-depth) so the skip list reflects current
features and won't hide references to removed commands.
In `@docs/deployment/sandbox-hardening.md`:
- Line 109: The line containing "The NemoClaw sandbox policy uses
`compatibility: best_effort`, which means Landlock enforcement is silently
skipped on kernels that do not support it." should be split into two source
lines so each sentence is on its own line: one line with "The NemoClaw sandbox
policy uses `compatibility: best_effort`." and a second line with "This means
Landlock enforcement is silently skipped on kernels that do not support it."
Preserve the exact wording and formatting (including the inline code span) so
the rendered paragraph remains the same.
In `@nemoclaw/src/security/secret-scanner.test.ts`:
- Around line 189-203: Remove the duplicate test cases that assert the same
behavior for isMemoryPath: keep a single test for the ".openclaw/memory/ paths"
assertion and a single test for the "MEMORY.md anchored to .openclaw" assertion,
deleting the repeated blocks so each expectation appears exactly once; ensure
the remaining tests still call
isMemoryPath("/sandbox/.openclaw/memory/project.md") or "/notes.md" (choose one)
and isMemoryPath("/sandbox/.openclaw/MEMORY.md") respectively and retain their
descriptive it(...) strings.
In `@test/e2e/test-shields-config.sh`:
- Around line 414-447: The auto-restore timer check using a fixed sleep (sleep
25) is flaky; replace the static wait with a retry/poll loop that queries
"nemoclaw \"${SANDBOX_NAME}\" shields status" (the STATUS_AFTER_TIMER check)
until either the shields show "DOWN" or "mutable" or a larger overall timeout
(e.g., 60s) is reached, polling every few seconds; after success break and run
the existing PERMS_TIMER stat check on "${CONFIG_PATH}", and on timeout fall
back to the existing manual cleanup (nemoclaw ... shields down) and fail
path—update references to STATUS_AFTER_TIMER, PERMS_TIMER, nemoclaw shields
status/up/down and CONFIG_PATH accordingly.
🪄 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: 019e6e93-1eb5-4caa-83bd-7e8c30cf14f0
📒 Files selected for processing (57)
.github/workflows/nightly-e2e.yamlDockerfileagents/hermes/Dockerfileagents/hermes/manifest.yamlagents/hermes/policy-additions.yamlagents/hermes/policy-permissive.yamlagents/hermes/start.shagents/openclaw/manifest.yamlagents/openclaw/policy-permissive.yamldocs/.docs-skipdocs/about/ecosystem.mddocs/about/how-it-works.mddocs/deployment/sandbox-hardening.mddocs/deployment/set-up-telegram-bridge.mddocs/reference/commands.mddocs/reference/troubleshooting.mddocs/security/best-practices.mdnemoclaw-blueprint/policies/openclaw-sandbox-permissive.yamlnemoclaw-blueprint/policies/openclaw-sandbox.yamlnemoclaw/src/blueprint/snapshot.test.tsnemoclaw/src/blueprint/snapshot.tsnemoclaw/src/index.tsnemoclaw/src/register.test.tsnemoclaw/src/security/secret-scanner.test.tsnemoclaw/src/security/secret-scanner.tsscripts/nemoclaw-start.shsrc/lib/agent-defs.test.tssrc/lib/agent-defs.tssrc/lib/onboard-command.test.tssrc/lib/onboard-command.tssrc/lib/onboard.tssrc/lib/policies.tssrc/lib/registry.tssrc/lib/sandbox-config.tssrc/lib/sandbox-state.tssrc/lib/sandbox-version.test.tssrc/lib/shields.tssrc/lib/skill-install.test.tssrc/lib/skill-install.tssrc/nemoclaw.tstest/config-rotate-token.test.tstest/config-set.test.tstest/e2e-gateway-isolation.shtest/e2e/e2e-cloud-experimental/checks/04-landlock-readonly.shtest/e2e/e2e-cloud-experimental/features/skill/verify-sandbox-skill-via-agent.shtest/e2e/test-rebuild-openclaw.shtest/e2e/test-sandbox-rebuild.shtest/e2e/test-sandbox-survival.shtest/e2e/test-shields-config.shtest/e2e/test-skip-permissions-policy.shtest/e2e/test-snapshot-commands.shtest/e2e/test-upgrade-stale-sandbox.shtest/exec-approvals-path-regression.test.tstest/onboard.test.tstest/openclaw-data-ownership.test.tstest/rebuild-policy-presets.test.tstest/service-env.test.ts
💤 Files with no reviewable changes (5)
- src/lib/registry.ts
- test/openclaw-data-ownership.test.ts
- test/config-rotate-token.test.ts
- test/e2e/test-skip-permissions-policy.sh
- test/config-set.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/test-hermes-e2e.sh (2)
375-389:⚠️ Potential issue | 🟠 MajorStale data-dir assertion conflicts with unified Hermes directory model
This still requires
/sandbox/.hermes-data, but the new model uses a single/sandbox/.hermestree for state. Seeagents/hermes/manifest.yaml(Lines 45-60) andagents/hermes/Dockerfile(Lines 63-102). This check will fail even when the sandbox is correctly configured.Proposed fix
-# 4e: Verify writable data directory exists +# 4e: Verify Hermes state directory exists under unified config dir data_dir_check=$($TIMEOUT_CMD ssh -F "$ssh_config" \ -o StrictHostKeyChecking=no \ -o UserKnownHostsFile=/dev/null \ -o ConnectTimeout=10 \ -o LogLevel=ERROR \ "openshell-${SANDBOX_NAME}" \ - "test -d /sandbox/.hermes-data && echo EXISTS || echo MISSING" \ + "test -d /sandbox/.hermes/memories && echo EXISTS || echo MISSING" \ 2>&1) || true if echo "$data_dir_check" | grep -q "EXISTS"; then - pass "Hermes writable data directory exists at /sandbox/.hermes-data" + pass "Hermes state directory exists at /sandbox/.hermes/memories" else - fail "Hermes writable data directory not found at /sandbox/.hermes-data" + fail "Hermes state directory not found at /sandbox/.hermes/memories" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-hermes-e2e.sh` around lines 375 - 389, Update the e2e check that asserts the old Hermes writable directory by changing the SSH test command in test/e2e/test-hermes-e2e.sh (the block that sets data_dir_check using $TIMEOUT_CMD ssh with $ssh_config and "openshell-${SANDBOX_NAME}") to test for /sandbox/.hermes instead of /sandbox/.hermes-data; keep the same echo EXISTS || echo MISSING logic and the downstream if that greps for "EXISTS" and calls pass/fail so the assertion matches the unified Hermes directory model.
358-370:⚠️ Potential issue | 🟠 MajorWritability assertion can false-pass due mixed probe output
Line 364 can emit both
WRITABLEandREAD_ONLY(e.g., create succeeds but cleanup fails). Line 367 then passes onWRITABLEsubstring, masking failure states.Proposed fix
-writable_check=$($TIMEOUT_CMD ssh -F "$ssh_config" \ +writable_check=$($TIMEOUT_CMD ssh -F "$ssh_config" \ -o StrictHostKeyChecking=no \ -o UserKnownHostsFile=/dev/null \ -o ConnectTimeout=10 \ -o LogLevel=ERROR \ "openshell-${SANDBOX_NAME}" \ - "touch /sandbox/.hermes/test-write 2>&1 && echo WRITABLE && rm -f /sandbox/.hermes/test-write || echo READ_ONLY" \ + 'f="/sandbox/.hermes/.write-test.$$"; + if touch "$f" 2>/dev/null && rm -f "$f" 2>/dev/null; then + echo WRITABLE + else + echo READ_ONLY + fi' \ 2>&1) || true -if echo "$writable_check" | grep -q "WRITABLE"; then +probe_result="$(printf '%s\n' "$writable_check" | tr -d '\r' | tail -n1)" +if [ "$probe_result" = "WRITABLE" ]; then pass "Hermes config directory is writable (mutable default)" -elif echo "$writable_check" | grep -q "READ_ONLY"; then +elif [ "$probe_result" = "READ_ONLY" ]; then fail "Hermes config directory is read-only — should be writable by default" else skip "Could not determine config directory mutability: ${writable_check:0:100}" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/test-hermes-e2e.sh` around lines 358 - 370, The probe can emit both WRITABLE and READ_ONLY (create succeeds but cleanup fails), causing false-passes; update the remote command used in the writable_check SSH call so it only prints a single final token by performing the touch and cleanup before echoing WRITABLE (e.g., run "touch /sandbox/.hermes/test-write && rm -f /sandbox/.hermes/test-write && echo WRITABLE || echo READ_ONLY"), so reference the writable_check variable and the SSH command that currently does "touch ... && echo WRITABLE && rm -f ... || echo READ_ONLY" and change the order/logic to ensure only one of WRITABLE or READ_ONLY is ever emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/sandbox-state.ts`:
- Around line 516-517: The shell command strings being built interpolate dir and
manifest directory names directly (e.g., the .map((d) => `[ -d "${dir}/${d}" ]
&& echo "${d}"`) expression), which is unsafe for spaces/special chars and
injection; fix by safely shell-quoting/escaping both dir and d when building
these commands (wrap each path operand in single quotes with proper escaping of
any internal single quotes, or call a centralized helper like shellEscape(path)
before interpolation) and pass -- before path operands to commands/tests (e.g.,
use test -d -- <quoted-path>); apply the same fix to the other occurrences
called out (the similar interpolations at lines referenced: the map at 543, the
commands built at 636-637, and 648) and ensure the echoed directory name is the
safely-quoted/escaped value or printed via a safe mechanism.
- Line 598: The code reads manifest.dir directly which breaks restores for
legacy manifests that only have writableDir; update the assignment for the local
variable (currently declared as const dir = manifest.dir) to fallback to
manifest.writableDir (e.g. use nullish coalescing or equivalent) and, if neither
exists, surface a clear error or version-gate using manifest.version so the
restore either fails with a helpful message or handles older manifests
explicitly; ensure the change is made where the const dir = manifest.dir is
declared and add a short log/error when both fields are missing.
---
Outside diff comments:
In `@test/e2e/test-hermes-e2e.sh`:
- Around line 375-389: Update the e2e check that asserts the old Hermes writable
directory by changing the SSH test command in test/e2e/test-hermes-e2e.sh (the
block that sets data_dir_check using $TIMEOUT_CMD ssh with $ssh_config and
"openshell-${SANDBOX_NAME}") to test for /sandbox/.hermes instead of
/sandbox/.hermes-data; keep the same echo EXISTS || echo MISSING logic and the
downstream if that greps for "EXISTS" and calls pass/fail so the assertion
matches the unified Hermes directory model.
- Around line 358-370: The probe can emit both WRITABLE and READ_ONLY (create
succeeds but cleanup fails), causing false-passes; update the remote command
used in the writable_check SSH call so it only prints a single final token by
performing the touch and cleanup before echoing WRITABLE (e.g., run "touch
/sandbox/.hermes/test-write && rm -f /sandbox/.hermes/test-write && echo
WRITABLE || echo READ_ONLY"), so reference the writable_check variable and the
SSH command that currently does "touch ... && echo WRITABLE && rm -f ... || echo
READ_ONLY" and change the order/logic to ensure only one of WRITABLE or
READ_ONLY is ever emitted.
🪄 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: 2148138f-04e2-4b9f-9d57-358082c1003f
📒 Files selected for processing (5)
scripts/nemoclaw-start.shsrc/lib/sandbox-state.tssrc/nemoclaw.tstest/e2e/test-hermes-e2e.shtest/nemoclaw-start.test.ts
The runtime exec-approvals compatibility patch was removed from the Dockerfile during the directory merge (Dockerfile.base still has it for the base image). Replace the removed assertions with checks for the mutable-default permission setup.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.agents/skills/nemoclaw-user-configure-security/references/best-practices.md (1)
425-425: Clarify scanner path scope for consistency with implementation.This list mixes one absolute
.openclawpath with relative-looking entries (workspace/,agents/, etc.). Consider making all entries explicit under/.openclaw/...(and optionally noting/.nemoclaw/) to match actual scanner coverage and avoid ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/nemoclaw-user-configure-security/references/best-practices.md at line 425, The entries in the scanner coverage list mix `.openclaw/memory/` with relative names (`workspace/`, `agents/`, `skills/`, `hooks/`, `MEMORY.md`) which is ambiguous; update the row so each entry is an explicit scanner path (e.g., `/.openclaw/memory/`, `/.openclaw/workspace/`, `/.openclaw/agents/`, `/.openclaw/skills/`, `/.openclaw/hooks/`, `/.openclaw/MEMORY.md`) and optionally add a note that equivalent `/.nemoclaw/...` paths are included if the scanner covers that prefix, ensuring the list matches the actual scanner implementation referenced by 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
@.agents/skills/nemoclaw-user-configure-security/references/best-practices.md:
- Around line 217-224: The doc incorrectly states directory mode 600; update the
"DAC permissions (default)" bullet and the table Default cell to state that the
config file `.openclaw` is mode 0600 (owner rw) and the containing directory
`/sandbox/.openclaw` is mode 0700 (owner rwx); ensure the wording around the
`shields up`/`shields down` commands remains consistent with those modes (i.e.,
`shields up` locks the file and directory and `shields down` restores file 0600
and dir 0700).
In @.agents/skills/nemoclaw-user-deploy-remote/references/sandbox-hardening.md:
- Around line 91-93: Summary: The doc currently contradicts itself by saying
unsupported kernels fall back to DAC-only while also claiming files outside
writable paths are blocked regardless of DAC; fix by clarifying that when
Landlock is unsupported no Landlock restrictions apply and DAC (UNIX
ownership/permissions) determines access. Edit the two sentences in
sandbox-hardening.md that read "protection falls back to DAC (file ownership and
permissions) only." and "Files outside the writable paths would be inaccessible
to the agent regardless of DAC permissions." to instead state that on kernels
without Landlock the kernel enforces only DAC rules (so access is governed by
UNIX permissions), and only when Landlock is active are additional writable-path
restrictions enforced; make the language explicit that the writable-path
enforcement applies only when Landlock is available.
---
Nitpick comments:
In
@.agents/skills/nemoclaw-user-configure-security/references/best-practices.md:
- Line 425: The entries in the scanner coverage list mix `.openclaw/memory/`
with relative names (`workspace/`, `agents/`, `skills/`, `hooks/`, `MEMORY.md`)
which is ambiguous; update the row so each entry is an explicit scanner path
(e.g., `/.openclaw/memory/`, `/.openclaw/workspace/`, `/.openclaw/agents/`,
`/.openclaw/skills/`, `/.openclaw/hooks/`, `/.openclaw/MEMORY.md`) and
optionally add a note that equivalent `/.nemoclaw/...` paths are included if the
scanner covers that prefix, ensuring the list matches the actual scanner
implementation referenced by 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 Plus
Run ID: 314c1435-b0c5-42e0-8d0a-8a3b52f12a5a
📒 Files selected for processing (7)
.agents/skills/nemoclaw-user-configure-security/references/best-practices.md.agents/skills/nemoclaw-user-deploy-remote/SKILL.md.agents/skills/nemoclaw-user-deploy-remote/references/sandbox-hardening.md.agents/skills/nemoclaw-user-overview/references/ecosystem.md.agents/skills/nemoclaw-user-overview/references/how-it-works.md.agents/skills/nemoclaw-user-reference/references/commands.md.agents/skills/nemoclaw-user-reference/references/troubleshooting.md
✅ Files skipped from review due to trivial changes (4)
- .agents/skills/nemoclaw-user-overview/references/how-it-works.md
- .agents/skills/nemoclaw-user-deploy-remote/SKILL.md
- .agents/skills/nemoclaw-user-reference/references/commands.md
- .agents/skills/nemoclaw-user-reference/references/troubleshooting.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nemoclaw.ts (1)
1260-1261:⚠️ Potential issue | 🔴 CriticalMove
sbout of the innertryblock to function scope.Line 1260 declares
const sbinside a try block (lines 1259–1283), but line 1376 readssb?.agentoutside that block. JavaScriptconstis block-scoped, so this causes aReferenceErrorat runtime when the connect hint is printed.🐛 Proposed fix
async function sandboxConnect(sandboxName) { + const sb = registry.getSandbox(sandboxName); const { isSandboxReady, parseSandboxStatus } = require("./lib/onboard"); await ensureLiveSandboxOrExit(sandboxName, { allowNonReadyPhase: true }); @@ try { - const sb = registry.getSandbox(sandboxName); if (sb && sb.provider && sb.model) { const live = parseGatewayInference( captureOpenshell(["inference", "get"], { ignoreError: true }).output, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 1260 - 1261, The variable sb is currently declared with const inside a try block (via registry.getSandbox(sandboxName)), causing a ReferenceError when sb?.agent is accessed later; move the declaration of sb into the surrounding function scope (e.g., let sb: ReturnType<typeof registry.getSandbox> | undefined) before the try and assign to it inside the try block where registry.getSandbox(sandboxName) is called so sb is visible later when printing the connect hint and when accessing sb?.agent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 6698-6738: The code treats an explicit empty recordedPolicyPresets
([]) as "no choice" and re-enters selection; fix by treating any
recordedPolicyPresets that is an array (including empty) as an explicit recorded
choice when computing resumePolicies and when calling
setupPoliciesWithSelection. Concretely, change the resumePolicies expression to
check Array.isArray(recordedPolicyPresets) and if true set resumePolicies =
resume && sandboxName && true (i.e., respect the saved array), otherwise fall
back to arePolicyPresetsApplied(..., recordedPolicyPresets || []); and pass
selectedPresets: Array.isArray(recordedPolicyPresets) ? recordedPolicyPresets :
null to setupPoliciesWithSelection so an explicit [] is preserved (also ensure
skippedStepMessage/onboardSession.markStepComplete use recordedPolicyPresets
unchanged).
In `@src/nemoclaw.ts`:
- Around line 3127-3129: The "connect" case currently ignores any actionArgs, so
unknown or removed flags (e.g., --dangerously-skip-permissions) are silently
dropped; update the switch case handling for "connect" (where
sandboxConnect(cmd) is called) to validate actionArgs and fail fast: if
actionArgs.length > 0 throw or return an error mentioning the invalid arguments
(or emit a deprecation message listing the removed flag), otherwise proceed to
await sandboxConnect(cmd); ensure the error message references the unknown args
so users know which flags were rejected.
- Around line 1439-1441: The status misreports default-mutable sandboxes because
shields.isShieldsDown(sandboxName) returns false when the shields state file is
absent; update sandbox creation to persist shieldsDown: true for new sandboxes
so the state file exists and shields.isShieldsDown reads true (or alternatively
change the status logic to derive shields state from the sandbox's effective
mode), e.g., set shieldsDown flag when creating the sandbox (the
creator/initializer for sandboxes) so the permissions line and the nemoclaw
status reflect "Shields: DOWN" instead of defaulting to UP; ensure you update
the code path that writes the shields state file and any tests that assert
shields status for fresh sandboxes.
---
Outside diff comments:
In `@src/nemoclaw.ts`:
- Around line 1260-1261: The variable sb is currently declared with const inside
a try block (via registry.getSandbox(sandboxName)), causing a ReferenceError
when sb?.agent is accessed later; move the declaration of sb into the
surrounding function scope (e.g., let sb: ReturnType<typeof registry.getSandbox>
| undefined) before the try and assign to it inside the try block where
registry.getSandbox(sandboxName) is called so sb is visible later when printing
the connect hint and when accessing sb?.agent.
🪄 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: 38cfa71d-4a5f-4521-998b-cc02ce5bd7b1
📒 Files selected for processing (3)
src/lib/onboard.tssrc/lib/registry.tssrc/nemoclaw.ts
💤 Files with no reviewable changes (1)
- src/lib/registry.ts
- sandbox-state.ts: add writableDir fallback for pre-consolidation backup manifests (backward compat) - nemoclaw.ts: hoist `sb` to function scope to fix ReferenceError when printing connect hint - troubleshooting.md: fix remediation to `onboard --resume` (not `rebuild`) matching the entrypoint guard - test-hermes-e2e.sh: update .hermes-data → .hermes assertion - secret-scanner.test.ts: remove duplicate test cases - e2e-gateway-isolation.sh: renumber duplicate test 13/14 → 13b/14b - hermes/policy-additions.yaml: fix stale comment
Update both base images (openclaw + hermes) to create state dirs directly in .openclaw/.hermes — no symlink bridge or -data directory. Remove the exec-approvals sed patch (writable .openclaw makes it unnecessary). Add migrate_legacy_layout() to both entrypoints for upgrading users: detects the old .openclaw-data symlink layout, copies real data into .openclaw, removes the old directory. Idempotent, no-op on new sandboxes. Add -h (dereference) to tar backup/restore so symlinks in old sandboxes are followed instead of triggering the symlink-escape security audit. Also fixes: - agent-runtime.ts: HERMES_HOME → /sandbox/.hermes - test/Dockerfile.sandbox: remove .openclaw-data chown - test/snapshot.test.ts: writableDir → dir - test/test-rebuild-hermes.sh: .hermes-data → .hermes - test/exec-approvals-path-regression.test.ts: update for simplified Dockerfile.base (no exec-approvals patch)
- shields.ts: shieldsUp() treated missing state file as "already
locked" — fix to allow first shields-up on fresh sandboxes. Skip
policy restore when no prior shields-down snapshot exists (fresh
sandbox already has the restrictive baseline).
- test-shields-config.sh: fix grep patterns to match actual CLI
output ("Lockdown active" / "Config unlocked" / "already active" /
"already unlocked" instead of "Shields UP/DOWN").
- test-snapshot-commands.sh: match "Snapshot.*created" for versioned
output from upstream snapshot naming feature.
Upstream commits add security hardening (#2181): - emit_sandbox_sourced_file() for root-owned 444 proxy-env.sh - validate_tmp_permissions() for pre-launch permission audit - _AXIOS_FIX_SCRIPT variable for NODE_OPTIONS preload Conflict resolution: - Dockerfile.base: keep upstream's #2181 comment, merge with our simplified layout - nemoclaw-start.sh: keep emit_sandbox_sourced_file and validate_tmp_permissions, drop _TOOL_REDIRECTS and pre-create block (home is now writable), drop validate_openclaw_symlinks and harden_openclaw_symlinks (removed features) - service-env.test.ts: keep extractEmitHelper and 444 permission check, drop extractToolRedirects and XDG redirect assertions
Extend lockAgentConfig/unlockAgentConfig to iterate over a sensitiveFiles list, locking .config-hash for all agents and .env for Hermes. Previously only the primary config file was locked, leaving .env and .config-hash writable during shields-up.
…to shields down --timeout
wscurran
added
refactor
Contributor
|
Heads-up: PR #2223 ( |
3 tasks
…view (#2439) ## Summary Follow-up to PR #2227 (`refactor(sandbox): default to mutable config, make shields opt-in`). Addresses all five findings from the 2026-04-24 security and vulnerability review so #2227 can proceed to merge without blocking on these concerns. Fixes #2300 (partial — shared concern about mutable config attack surface) ## Changes ### NC-2227-01 (Critical): Legacy migration can undo `shields up` `migrate_legacy_layout()` in `scripts/nemoclaw-start.sh` now has three guards: 1. **Sentinel** — `.migration-complete` marker prevents re-running on already-migrated sandboxes 2. **Root-only** — refuses to run if not root (blocks agent from triggering it) 3. **Ownership** — rejects data directories owned by `sandbox` user (detects agent-planted triggers; legitimate legacy dirs are root-owned from the image build) Additionally: replaced blanket `chown -R sandbox:sandbox "$config_dir"` with targeted subdirectory-only chown, preserving root ownership on `openclaw.json`, `.config-hash`, `.env`. Reapplies shields-up ownership after migration if shields were previously active. ### NC-2227-02 (High): Fresh mutable sandboxes report `Shields: UP` Introduced a three-state model in `src/lib/shields.ts`: `mutable_default`, `locked`, `temporarily_unlocked`. Fresh sandboxes (no state file) now report `NOT CONFIGURED (default mutable state)` instead of falsely claiming lockdown is active. `isShieldsDown()` returns true for both `mutable_default` and `temporarily_unlocked` — only `locked` (after explicit `shields up`) returns false. ### NC-2227-03 (High): Auto-restore does not re-lock sensitive sidecar files `src/lib/shields-timer.ts` now calls `resolveAgentConfig(sandboxName)` to get the full lock target (including `.config-hash`, `.env`) before calling `lockAgentConfig`. Falls back to bare `configPath`/`configDir` from argv if resolution fails. ### NC-2227-04 (High): Backup dereferences untrusted symlinks Removed `tar -h` flag from both backup and restore in `src/lib/sandbox-state.ts`. Added a pre-backup `find`-based audit that rejects symlinks (`-type l`) and special files before creating the tar, preventing exfiltration of config contents via agent-planted symlinks. ### NC-2227-05 (High): `shields up` leaves co-located state directories writable Added `HIGH_RISK_STATE_DIRS` constant (`skills`, `hooks`, `cron`, `agents`, `extensions`, `plugins`). `lockAgentConfig()` now recursively `chown -R root:root` and `chmod 755` these directories during shields-up. `unlockAgentConfig()` restores `sandbox:sandbox` ownership during shields-down. ## Type of Change - [X] Code change (feature, bug fix, or refactor) ## Verification - [X] Tests added or updated for new or changed behavior - [X] No secrets, API keys, or credentials committed Tests added: - `test/nemoclaw-start.test.ts` — 6 tests for migration guards (sentinel, root-only, ownership, no blanket chown, shields reapplication, sentinel permissions) - `test/shields.test.ts` — 10 tests covering three-state model derivation, tar flag regression, and state directory locking Note: NC-2227-02 unit tests replicate `deriveShieldsMode` logic inline (CJS module not directly importable in vitest). Full code path is exercised by E2E tests in `test/e2e/test-shields-config.sh`. --- Signed-off-by: Andrew Erickson <aerickson@nvidia.com> --------- Signed-off-by: Andrew Erickson <aerickson@nvidia.com>
Merge main into remove-config-immutability, resolving conflicts from PRs #2324 (SSRF blocklist), #2378 (gateway token externalization), and various CI/docs/test updates. Key resolution decisions: - Keep mutable-by-default model (PR #2227) for config layout - Keep gateway token externalization (PR #2378) in Dockerfile and start scripts - Remove config set/rotate-token from sandbox-config.ts (PR #2227 removes host-side mutation) - Keep SSRF blocklist in private-networks.ts (PR #2324) for remaining consumers - Update sandbox-provisioning tests for the unified .openclaw layout - Delete test-skip-permissions-policy.sh (feature removed by PR #2227)
Accept PR's mutable-default architecture (remove dangerouslySkipPermissions, single .openclaw dir, shields opt-in) while incorporating main's TypeScript type guards and validators in surviving code. Key resolutions: - Delete test/config-set.test.ts and test/openclaw-data-ownership.test.ts (features no longer exist) - Update isRebuildManifest guard to accept both dir and writableDir fields - Fix configPaths in auto-merged test fixtures (immutableDir/writableDir → dir) - Remove permanent field from isShieldsState guard - Add Record<string, unknown> type annotations in shields test
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/reference/troubleshooting.md (2)
618-618: 💤 Low valueConsider rewording to active voice.
"are intercepted" uses passive voice. As per coding guidelines, documentation should use active voice consistently.
Suggested rewording: "Changes made inside the running sandbox do not persist across rebuilds, so the entrypoint guard intercepts
openclaw channelscommands that mutate the config."As per coding guidelines, active voice is required in documentation — the rule states "Active voice required. Flag passive constructions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/troubleshooting.md` at line 618, Reword the passive sentence to active voice: replace "so `openclaw channels` commands that mutate the config are intercepted" with an active construction such as "so the entrypoint guard intercepts `openclaw channels` commands that mutate the config", updating the line mentioning changes inside the running sandbox to ensure the documentation uses active voice and references the entrypoint guard and the `openclaw channels` command.
635-635: 💤 Low valueConsider rewording to active voice.
"is baked into" uses passive voice. As per coding guidelines, documentation should use active voice consistently.
Suggested rewording: "NemoClaw bakes the sandbox's OpenClaw configuration (
/sandbox/.openclaw/openclaw.json) into the container image at build time."As per coding guidelines, active voice is required in documentation — the rule states "Active voice required. Flag passive constructions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/troubleshooting.md` at line 635, Replace the passive sentence "The sandbox's OpenClaw configuration (`/sandbox/.openclaw/openclaw.json`) is baked into the container image at build time." with an active-voice version that names the actor; for example: "NemoClaw bakes the sandbox's OpenClaw configuration (`/sandbox/.openclaw/openclaw.json`) into the container image at build time." Update the sentence in docs/reference/troubleshooting.md where that exact phrase appears to use this active construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/reference/troubleshooting.md`:
- Line 618: Reword the passive sentence to active voice: replace "so `openclaw
channels` commands that mutate the config are intercepted" with an active
construction such as "so the entrypoint guard intercepts `openclaw channels`
commands that mutate the config", updating the line mentioning changes inside
the running sandbox to ensure the documentation uses active voice and references
the entrypoint guard and the `openclaw channels` command.
- Line 635: Replace the passive sentence "The sandbox's OpenClaw configuration
(`/sandbox/.openclaw/openclaw.json`) is baked into the container image at build
time." with an active-voice version that names the actor; for example: "NemoClaw
bakes the sandbox's OpenClaw configuration (`/sandbox/.openclaw/openclaw.json`)
into the container image at build time." Update the sentence in
docs/reference/troubleshooting.md where that exact phrase appears to use this
active construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c847cddf-494a-4d40-a79b-ee9977d3ec7c
📒 Files selected for processing (5)
agents/hermes/start.shdocs/reference/troubleshooting.mdscripts/nemoclaw-start.shtest/nemoclaw-start.test.tstest/sandbox-init.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/sandbox-init.test.ts
- agents/hermes/start.sh
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25144459663
|
7 tasks
ericksoa
added a commit
that referenced
this pull request
Apr 30, 2026
## Summary - Follow-up to #2227: keep `/sandbox/.bashrc` and `/sandbox/.profile` as static source shims instead of rewriting them at startup. - Generate gateway token export and the NemoClaw configure guard into `/tmp/nemoclaw-proxy-env.sh` via the existing trusted `emit_sandbox_sourced_file` path. - Make the base image create shell startup files as `root:root` mode `444`, and update docs/tests for the new default behavior. ## Validation - `npm test -- test/nemoclaw-start.test.ts test/service-env.test.ts test/sandbox-provisioning.test.ts test/sandbox-init.test.ts` - `NEMOCLAW_TEST_IMAGE=nemoclaw-override-test-d7ea5b15 bash test/e2e/test-runtime-overrides.sh` ## Notes - This targets the raw `docker run` runtime-overrides failure where root had already dropped `CAP_DAC_OVERRIDE` and could no longer create `/sandbox/.bashrc` temp files. - The OpenClaw default `$HOME/.openclaw` mutable state behavior from #2227 remains unchanged. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Refactor** * Hardened sandbox shell startup by making user RC files root-owned and read-only, and moving runtime token/export handling to a generated runtime env script that is sourced via a static shim. * **Documentation** * Updated security best-practices to describe the new runtime env script flow and revised default locked-down posture wording. * **Tests** * Adjusted and added tests to validate the runtime env emission, shim backfill, and immutable RC file expectations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced May 2, 2026
This was referenced May 4, 2026
prekshivyas
added a commit
that referenced
this pull request
May 6, 2026
Some bugs are filed against behavior that was deliberately removed or changed in a merged PR. Running the standard rubric on these produces a misleading verdict — the symptom "still reproduces" but the right answer is "won't fix, see PR #X." Issue #2791 is the prototype: `config set` was removed in #2227, the reporter tested a version that already had it gone, and the standard rubric would have buried that context under a low-confidence `verify-inconclusive` label. Add Step 8.5 with three independent detection signals: - Maintainer-attribution comment phrasing ("removed in #N", "by design", "wontfix", "intentional") from MEMBER/OWNER/COLLABORATOR authors. - A removal commit between reported version and `$LATEST` whose subject matches `\b(remove|delete|drop|deprecate)\b` and whose diff deletes the symbol implicated by the reproducer. - The implicated symbol absent from both reported version and `$LATEST`, meaning it was already gone when the issue was filed. When any signal fires, skip Step 9 scoring and Brev provisioning, apply a new `wontfix-by-design` label, and post a focused comment that links to the responsible PR. Detection on signals 2 and 3 can run as soon as the reported version is parsed, saving Brev cost on the entire class. Generalize the Step 3 idempotency marker regex from `v1` to `v\d+` so future skill versions can re-verify older-marked issues by tightening the regex to require a specific marker version. Add `wontfix-by-design` to the idempotency label list, the activity log entry options, the session summary, and the release sweep in `nemoclaw-maintainer-cut-release-tag`. Update the skill's frontmatter description to mention the new label and the local-first short-circuit. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
11 tasks
prekshivyas
added a commit
that referenced
this pull request
May 6, 2026
Path-extraction map in Step 9 was written from assumption rather than verification — `src/lib/sandbox/`, `src/lib/openshell/`, and `src/lib/policy/` don't exist in the current repo, and OpenShell lives in a separate repo entirely. The +25 commits-touched-component signal would have silently misfired on every Sandbox / OpenShell / policy issue, underscoring real fixes. Replace with paths verified against the current tree (`nemoclaw/src/blueprint/`, `nemoclaw-blueprint/`, `nemoclaw/src/commands/`, etc.) and explicitly note the cross-repo OpenShell case is out of scope for v1. Step 8 reset wiped `~/.nemoclaw` but not `~/.openclaw`. Sandbox state has been writable-by-default under `.openclaw` since #2227, so it persists across the baseline → latest reinstall and contaminates the latest run. Add it to the reset. Anchor the `pkill -9 -f nemoclaw|openshell` patterns to a leading slash (`/nemoclaw`, `/openshell`) so the kill matches actual installed paths but not unrelated processes that mention these strings — including the agent harness running this skill if its working directory contains the word. Reorder the Step 10 redaction table to match the stated "longest, most specific patterns first" rule. The previous order had the generic `(token|secret|password|...)` pattern executing before JWT/AWS/NVIDIA patterns, which would have masked specific-token redactions with generic ones — losing the signal of *what kind* of credential leaked. Replace `git ls-remote git@github.com:NVIDIA/...` (Steps 2 and 4) with `gh api repos/NVIDIA/NemoClaw/tags --paginate --jq '.[].name'`. The SSH path required keys to be configured; `gh api` reuses whatever auth the user already has. Add a CLI-deps precondition check (`command -v gh brev jq python3 curl`) to Step 6.5 so the skill fails fast if any required dependency is missing, instead of failing late and confusingly mid-run. Step 11's keep-box-on-inconclusive said "delay cleanup by 30 minutes" but had no implementation. A backgrounded `sleep && brev delete` doesn't survive session end. Replace with skip-the-trap-entirely plus an explicit manual-cleanup reminder printed to the run output. Out-of-Scope was contradicting itself on macOS — Step 6.7 explicitly allows macOS for local-first runs. Clarify: Brev path is Linux-only, the local-first short-circuit on a maintainer's laptop works on macOS for manual single-issue runs. Add a `local (no Brev — Step 6.7 short-circuit)` option to the Step 12 log entry's Box field so local-first runs round-trip the activity log correctly. Frontmatter description said "latest release" but NemoClaw uses tags, not GitHub Releases — fix to "latest tag" to match Step 2. Step 4's implementer note said "Two real failure modes" but listed three; fix the off-by-one. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
added a commit
that referenced
this pull request
May 6, 2026
The previous signal-2 candidate query narrowed by commit subject (`remove|delete|drop|deprecate`) before checking whether the diff deleted the implicated symbol. That filter excludes the common case where a removal lands inside a `refactor(...)` or `feat(...)` commit — e.g., PR #2227 removed `--dangerously-skip-permissions` under a "refactor(sandbox): default to mutable config" subject. A literal follower of the previous rule would have concluded signal 2 doesn't fire on issue #2168 even though the flag was deliberately removed. Switch the primary candidate lookup to `git log -S<symbol>` pickaxe search, which finds every commit whose diff changes the count of the symbol regardless of subject wording. Keep the subject-keyword query as a supplementary lookup for narrowing when the pickaxe returns many commits. Note explicitly that the commit's actual subject doesn't need to mention removal. Surfaced while testing the new Step 8.5 rigor against #2168. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
added a commit
that referenced
this pull request
May 7, 2026
Six concrete gaps that surfaced during the first end-to-end Brev run (issue #2007). Each almost produced a wrong verdict or a wasted run; each is now encoded so the next agent doesn't have to re-discover. 1. Step 9 baseline-validation cap-removal rule was too permissive. The `unless commits-touched OR PR-mention also fires` escape hatch let inferred fix evidence override the absence of a baseline run, which produced a misleading 100/100 on #2007 despite zero baseline confirmation. Tightened: cap at 84 holds regardless of corroboration; PR-search signals raise the score within the cap, never past it. Step 10 now requires an explicit one-line caveat naming the cap and the reason in the rendered Verdict section. 2. Step 6.5 install URL default switched from `nemoclaw.nvidia.com` (NVIDIA-internal, doesn't resolve from Brev) to `www.nvidia.com/ nemoclaw.sh` (public Akamai 301 redirect). Brev runs were dying at bootstrap on the wrong host. 3. Step 7 CPU SKU picker now biases by reproducer-implied memory needs. On #2007 the cheapest 2 GB SKU couldn't load a 4.8 GiB Ollama probe; onboard failed at provider validation and we burned ~25 min before re-provisioning a 16 GB box. Adds a `CPU_RAM_FLOOR` env var and a memory-floor heuristic (16 GB when reproducer references a model server, 8 GB for sandbox onboarding without a model, 4 GB for pure-CLI bugs). 4. Step 11 failure taxonomy now has a "baseline-build rot" bucket distinct from "binary install rot." Same `BASELINE_INSTALL_FAILED=1` flag and same downstream cap-and-degrade behavior, but failures at the in-image Dockerfile build phase (what we hit on v0.0.18 — the `.openclaw-data/workspace/media` symlink layer, removed entirely by #2227) get a separate label so reviewers can see *why* the old image no longer builds without re-running. 5. New Step 8a.5b documents the two non-obvious `brev exec` quirks that reproducer scripts have to handle every time: PATH does not include `~/.local/bin` in non-login shells (so reproducers must `export PATH` at the top, or callers must wrap with `bash -lc`); and the docker group requires `sg docker -c '...'` because adding the user via `usermod -aG` doesn't take effect within the same Brev session. 6. New Step 8d.5 architectural-drift check. When the diff between `$REPORTED_VERSION` and `$LATEST` touches the *tool* the reproducer's expected output depends on (e.g. `openshell forward` between v0.0.18 and v0.0.35), don't trust the reproducer's surface alone — multi-axis verification on OS-level surfaces (host listeners, NAT rules, docker ports, SSH tunnels, etc.) is required before claiming fixed-on-latest. This is the five-axis pattern we used to confirm #2007 wasn't a false positive; codifies it as a check the skill applies whenever pickaxe shows the reproducer's tool was reworked. Surfaced from: end-to-end Brev verification of issue #2007. Build the skill, exercise it on a real issue, fix what breaks, repeat — every fix here came from a concrete failure mode in one real run. Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
wscurran
added
area: sandbox
2 tasks
This was referenced Jul 16, 2026
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.
Summary
Flips the sandbox default from immutable to mutable. Agents start with writable config (
600 sandbox:sandbox) so they can natively manage their own configuration, install skills, and write to standard home-directory paths — no workarounds needed.Shields DOWN is now the default state. Operators who want config immutability on sensitive workloads opt in explicitly with
nemoclaw <name> shields up, which applies444 root:root+chattr +i. This positions shields as the foundation for a future hardened/immutable agent mode.What changes
444 root:rootwithchattr +i600 sandbox:sandbox, writable/tmpinclude_workdir: true), no redirectsconfig set/config rotate-tokenbypass commands--dangerously-skip-permissionsflag$HOME/.openclaw/skills/.openclaw(immutable) +.openclaw-data(writable) + symlinks.openclawdirectoryWhat's removed (~2,000 lines deleted)
--dangerously-skip-permissionsflag,shieldsDownPermanent(), and all permanent state logicconfig setandconfig rotate-tokencommands and their kubectl exec write pathschattr +ihardening and symlink validation functions_TOOL_REDIRECTSentries and/tmppre-creation block.openclaw-datadirectory, symlink bridge, and allimmutableDir/writableDirabstractionstest/config-set.test.ts,test/config-rotate-token.test.ts,test/openclaw-data-ownership.test.ts,test/e2e/test-skip-permissions-policy.shWhat stays
config get— read-only inspection works regardless of shields stateVerification
All 5 grep checks pass with zero matches:
dangerouslySkipPermissions/dangerously-skip-permissionsinsrc/→ 0configSet/configRotateTokeninsrc/→ 0mirrorDir/immutableDir/writableDirinsrc/→ 0_TOOL_REDIRECTS/XDG_CACHE_HOMEinscripts/→ 0openclaw-datain all code/test/doc files → 0Test plan
npm test— unit and integration tests pass (updated assertions for mutable default)make check— linters and type checks passtest-shields-config.sh— validates mutable default → shields up → shields down lifecyclee2e-gateway-isolation.sh— validates sandbox can write to.openclaw(mutable default)04-landlock-readonly.sh— validates/sandboxand.openclaware writable, system paths read-onlytest-sandbox-rebuild.sh,test-snapshot-commands.sh— workspace paths updated to.openclawopenclaw doctorpasses immediately on sandbox startup (no shields down needed)Summary by CodeRabbit
Removed Features
Changed Behavior
Documentation
New Features