feat(sandbox-mgmt): experimental shields and config commands#1976
Open
feat(sandbox-mgmt): experimental shields and config commands#1976
Conversation
Add shields down/up/status and config get/set/rotate-token for sandbox management. These commands are experimental — interfaces may change and no documentation is published. The feature is exercised by a new E2E test (test/e2e/test-shields-config.sh) but is not formally supported. Shields: - Time-bounded policy relaxation with automatic restore timer - State namespaced per sandbox (shields-<name>.json) - Rollback on timer fork failure (never leaves sandbox unguarded) - Timer checks policy restore result; reverts state on failure - Audit trail (shields-audit.jsonl) with credential redaction Config: - config get: read-only inspection with credential stripping - config set: host-initiated mutation with SSRF validation - config rotate-token: credential rotation via env, stdin, or prompt - All commands use --name flag for openshell sandbox exec Security: - Endpoint URL redaction in config-show (plugin slash command) - Audit reason/error fields redacted via runner.ts patterns - shields down flag validation (reject missing values, unknown flags) Testing: - E2E: full shields lifecycle, config CRUD, timer auto-restore - Unit: duration parsing, state management, audit format, URL redaction - Plugin branch coverage 86.28% (above 86% ratchet) Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
6 tasks
Contributor
|
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 (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds host-side shields management and sandbox config commands: new CLI actions, persisted shields state fields, auto-restore timer and audit logging, policy snapshot/restore flows, agent policy adjustments, new utilities (duration, sandbox-config, shields-audit/timer), and many unit, integration, and e2e tests. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Host CLI
participant State as Local State File
participant Policy as Policy System
participant Timer as Auto-Restore Timer
participant Audit as Audit Log
CLI->>State: read state (merge blankState() with persisted)
State-->>CLI: current shields state
CLI->>Policy: capture current policy (snapshot)
Policy-->>CLI: policy YAML
CLI->>State: persist snapshot path & shieldsDown metadata
State-->>CLI: persisted
CLI->>Policy: apply relaxed policy (permissive or provided)
Policy-->>CLI: applied
CLI->>Timer: fork detached timer with restoreAt
Timer-->>CLI: started (PID marker)
CLI->>Audit: append shields_down entry
Audit-->>CLI: appended
Note over Timer: waits until restoreAt
Timer->>State: read snapshot path & state
State-->>Timer: state data
Timer->>Policy: verify snapshot exists
Policy-->>Timer: snapshot OK
Timer->>Policy: restore policy from snapshot
Policy-->>Timer: restored
Timer->>State: clear shieldsDown fields
State-->>Timer: persisted
Timer->>Audit: append shields_auto_restore entry
Audit-->>Timer: appended
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
nemoclaw-blueprint/policies/openclaw-sandbox-permissive.yaml (1)
9-9:⚠️ Potential issue | 🟡 MinorStale comment:
include_workdir: trueno longer matches actual value.Line 9 states "Filesystem: include_workdir: true makes the sandbox home directory writable" but the actual value on line 21 is now
false. This comment should be updated to reflect the current behavior.📝 Suggested fix
-# Filesystem: include_workdir: true makes the sandbox home directory writable. +# Filesystem: include_workdir must be false (see openclaw-sandbox.yaml for rationale). +# Config mutability is handled by shields down/up via kubectl exec (bypasses Landlock).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw-blueprint/policies/openclaw-sandbox-permissive.yaml` at line 9, Update the stale inline comment describing Filesystem behavior so it matches the current setting of include_workdir (which is false); locate the comment that reads "Filesystem: include_workdir: true makes the sandbox home directory writable" and change it to state that include_workdir: false makes the sandbox home directory read-only (or otherwise describe the current behavior), ensuring the comment and the include_workdir setting in openclaw-sandbox-permissive.yaml are consistent.nemoclaw/src/commands/slash.test.ts (1)
64-81:⚠️ Potential issue | 🟡 MinorTest fixture
blankState()is missinglastRebuildAtandlastRebuildBackupPathfields.The
NemoClawStateinterface (per context snippet fromnemoclaw/src/blueprint/state.ts) includeslastRebuildAtandlastRebuildBackupPath, but this test fixture omits them. This could cause type mismatches or incomplete state mocking.💡 Proposed fix
function blankState(): NemoClawState { return { lastRunId: null, lastAction: null, blueprintVersion: null, sandboxName: null, migrationSnapshot: null, hostBackupPath: null, createdAt: null, updatedAt: new Date().toISOString(), + lastRebuildAt: null, + lastRebuildBackupPath: null, shieldsDown: false, shieldsDownAt: null, shieldsDownTimeout: null, shieldsDownReason: null, shieldsDownPolicy: null, shieldsPolicySnapshotPath: null, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/commands/slash.test.ts` around lines 64 - 81, The test fixture blankState() returns an incomplete NemoClawState; add the missing fields lastRebuildAt and lastRebuildBackupPath to the returned object so the shape matches the NemoClawState interface: include lastRebuildAt: null (or ISO string when appropriate) and lastRebuildBackupPath: null (or an empty string if your code expects a string) alongside the other properties in the blankState() function to prevent type mismatches in tests.
🧹 Nitpick comments (3)
agents/hermes/policy-permissive.yaml (1)
14-17: Soften the “must match exactly” claim to avoid policy drift confusion.Line 14 says this section must match
policy-additions.yamlexactly, but this file also includes an extra write path at Line 33 (/sandbox/.nemoclaw). That wording can mislead future edits.Proposed wording tweak
- # Filesystem section MUST match the default Hermes policy (policy-additions.yaml) - # exactly — OpenShell rejects include_workdir changes and read_only path + # Filesystem section should mirror the default Hermes policy + # (policy-additions.yaml), except explicitly documented NemoClaw state paths. + # OpenShell rejects include_workdir changes and read_only path # removals on live sandboxes. Config file mutability is handled separately # by shields down/up via kubectl exec (bypasses Landlock).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@agents/hermes/policy-permissive.yaml` around lines 14 - 17, Update the comment that currently reads "Filesystem section MUST match the default Hermes policy (policy-additions.yaml) exactly" to a softer phrasing that conveys intent without implying absolute exactness—e.g., "Filesystem section should generally match the default Hermes policy (policy-additions.yaml); avoid changing include_workdir or removing read_only paths on live sandboxes." Ensure the new text also calls out the known exception (/sandbox/.nemoclaw write path) so future editors aren't misled, and retain the warning about using kubectl exec for config mutability and Landlock-related restrictions.src/lib/sandbox-config.ts (1)
504-512: Consider:readStdinhas no timeout.If stdin never closes (e.g., piped from a process that hangs), this promise will never resolve. For interactive CLI usage this is acceptable, but if
--from-stdinis used in automated pipelines, a timeout might be valuable.This is a minor robustness consideration for future hardening, not blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sandbox-config.ts` around lines 504 - 512, readStdin currently can hang indefinitely; update the readStdin function to accept an optional timeoutMillis (or use a sensible default) and implement a Promise.race between the stdin-reading promise and a timeout promise; when the timeout fires, reject with a descriptive error, and ensure you remove stdin listeners and clear any timers on both success and timeout to avoid leaks (refer to the readStdin function name and its "data"/"end"/"error" listeners when making these changes).test/e2e/test-shields-config.sh (1)
466-481: Auto-restore test has a potential race condition.The test waits 15 seconds for a 10-second timeout, which should be sufficient. However, if the timer process is delayed (e.g., slow CI runner, policy set --wait taking longer), the test may flap. The fallback at line 479 (
nemoclaw shields up || true) is good for cleanup, but consider increasing the sleep margin or adding a retry loop.🤖 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 466 - 481, The auto-restore check using STATUS_AFTER_TIMER and the command `nemoclaw "${SANDBOX_NAME}" shields status` can flake because it sleeps a fixed 15s for a 10s timer; modify the test to avoid the race by replacing the single sleep+check with a short retry loop: after the initial sleep, poll `nemoclaw "${SANDBOX_NAME}" shields status` up to a configurable timeout (e.g., 30s total) with small intervals and break when output contains "Shields: UP"; keep the existing cleanup fallback using `nemoclaw "${SANDBOX_NAME}" shields up || true` and ensure any failure still calls `fail "Auto-restore timer did not restore shields within Xs"` with the total wait reflected.
🤖 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/commands/config-show.ts`:
- Around line 32-35: The current rendering of credentialEnv in the config-show
command can echo unsafe or empty values; update the logic in the config-show
handler (where redactedCredential and lastFour are computed) to only render
`$NAME` when config.credentialEnv matches a strict env-var name regexp (e.g.
/^[A-Z0-9_]+$/); if it does not match or is empty, set redactedCredential to a
generic label like "(configured via env var)" or "(not configured)" and adjust
lastFour accordingly so raw tokens or malformed strings from loadOnboardConfig()
are never printed.
In `@nemoclaw/src/commands/shields-status.test.ts`:
- Around line 16-33: The blankState() test fixture returns an incomplete
NemoClawState missing the lastRebuildAt and lastRebuildBackupPath fields; update
blankState() to include these two properties (initialize them similarly to other
timestamp/backup fields—e.g., null) so the fixture matches the full
NemoClawState shape used in tests like slash.test.ts and avoids type/coverage
gaps when using lastRebuildAt or lastRebuildBackupPath in tests.
In `@src/lib/shields-timer.ts`:
- Around line 114-125: The catch block that calls
appendAudit("shields_up_failed", ...) currently lets execution fall into finally
which unconditionally calls process.exit(0), masking failures; modify
shields-timer.ts so the process exits with a non-zero status when an exception
occurs—e.g. set a local flag or call process.exit(1) from the catch (or set
process.exitCode = 1) and ensure the finally block uses that code or only
performs cleanup via cleanupMarker() without forcing exit(0); update the catch
handling around appendAudit(...) and the finally that calls cleanupMarker() /
process.exit(...) so errors produce a non-zero exit (use the function/variable
names appendAudit, cleanupMarker, and process.exit/process.exitCode to locate
changes).
In `@test/config-set.test.ts`:
- Around line 88-90: The test description string "accepts public http URLs" in
the it(...) block does not match the URL passed to validateUrlValue (currently
"https://example.com"); update the test so the description and input agree:
either change the description to "accepts public https URLs" or modify the URL
to "http://example.com" to match "http" — adjust the it(...) description or the
validateUrlValue argument accordingly.
---
Outside diff comments:
In `@nemoclaw-blueprint/policies/openclaw-sandbox-permissive.yaml`:
- Line 9: Update the stale inline comment describing Filesystem behavior so it
matches the current setting of include_workdir (which is false); locate the
comment that reads "Filesystem: include_workdir: true makes the sandbox home
directory writable" and change it to state that include_workdir: false makes the
sandbox home directory read-only (or otherwise describe the current behavior),
ensuring the comment and the include_workdir setting in
openclaw-sandbox-permissive.yaml are consistent.
In `@nemoclaw/src/commands/slash.test.ts`:
- Around line 64-81: The test fixture blankState() returns an incomplete
NemoClawState; add the missing fields lastRebuildAt and lastRebuildBackupPath to
the returned object so the shape matches the NemoClawState interface: include
lastRebuildAt: null (or ISO string when appropriate) and lastRebuildBackupPath:
null (or an empty string if your code expects a string) alongside the other
properties in the blankState() function to prevent type mismatches in tests.
---
Nitpick comments:
In `@agents/hermes/policy-permissive.yaml`:
- Around line 14-17: Update the comment that currently reads "Filesystem section
MUST match the default Hermes policy (policy-additions.yaml) exactly" to a
softer phrasing that conveys intent without implying absolute exactness—e.g.,
"Filesystem section should generally match the default Hermes policy
(policy-additions.yaml); avoid changing include_workdir or removing read_only
paths on live sandboxes." Ensure the new text also calls out the known exception
(/sandbox/.nemoclaw write path) so future editors aren't misled, and retain the
warning about using kubectl exec for config mutability and Landlock-related
restrictions.
In `@src/lib/sandbox-config.ts`:
- Around line 504-512: readStdin currently can hang indefinitely; update the
readStdin function to accept an optional timeoutMillis (or use a sensible
default) and implement a Promise.race between the stdin-reading promise and a
timeout promise; when the timeout fires, reject with a descriptive error, and
ensure you remove stdin listeners and clear any timers on both success and
timeout to avoid leaks (refer to the readStdin function name and its
"data"/"end"/"error" listeners when making these changes).
In `@test/e2e/test-shields-config.sh`:
- Around line 466-481: The auto-restore check using STATUS_AFTER_TIMER and the
command `nemoclaw "${SANDBOX_NAME}" shields status` can flake because it sleeps
a fixed 15s for a 10s timer; modify the test to avoid the race by replacing the
single sleep+check with a short retry loop: after the initial sleep, poll
`nemoclaw "${SANDBOX_NAME}" shields status` up to a configurable timeout (e.g.,
30s total) with small intervals and break when output contains "Shields: UP";
keep the existing cleanup fallback using `nemoclaw "${SANDBOX_NAME}" shields up
|| true` and ensure any failure still calls `fail "Auto-restore timer did not
restore shields within Xs"` with the total wait reflected.
🪄 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: ff620ec6-3c66-4952-920e-6950e758f7dd
📒 Files selected for processing (25)
.github/workflows/nightly-e2e.yamlagents/hermes/policy-permissive.yamlnemoclaw-blueprint/policies/openclaw-sandbox-permissive.yamlnemoclaw/src/blueprint/state.test.tsnemoclaw/src/blueprint/state.tsnemoclaw/src/commands/config-show.test.tsnemoclaw/src/commands/config-show.tsnemoclaw/src/commands/shields-status.test.tsnemoclaw/src/commands/shields-status.tsnemoclaw/src/commands/slash.test.tsnemoclaw/src/commands/slash.tsnemoclaw/src/onboard/config.test.tsnemoclaw/src/onboard/config.tssrc/lib/duration.tssrc/lib/sandbox-config.tssrc/lib/shields-audit.tssrc/lib/shields-timer.tssrc/lib/shields.tssrc/nemoclaw.tstest/config-rotate-token.test.tstest/config-set.test.tstest/duration.test.tstest/e2e/test-shields-config.shtest/shields-audit.test.tstest/shields.test.ts
…e, stale-registry gate
- Validate credentialEnv matches safe env-var pattern before echoing in
config-show (prevents leaking malformed persisted data)
- Exit with status 1 in shields-timer catch block instead of falling
through to finally { process.exit(0) }
- Add "shields" and "config" to the stale-registry recovery gate so
they work when the local registry is out of date
- Fix test description/URL mismatch in config-set test
- Add missing lastRebuildAt/lastRebuildBackupPath to blankState() test
fixtures in shields-status.test.ts and slash.test.ts
- Fix stale comments in permissive policy YAML files
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
wscurran
added
priority: high
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Experimental sandbox management commands for shields and config operations. This feature is not formally documented or advertised — it is gated behind the same experimental-feature conventions used elsewhere in the codebase.
shields down/up/status— time-bounded policy relaxation with automatic restoreconfig get/set/rotate-token— host-initiated config inspection and mutationShields
shields-<name>.json)fork()with explicit IPC disconnectConfig
config get: read-only with credential stripping and gateway section removalconfig set: SSRF validation, gateway write blocking, kubectl exec for Landlock bypassconfig rotate-token: env var, stdin, or interactive promptopenshell sandbox execcalls use--nameflag (not positional)Security hardening (CodeRabbit review)
/nemoclaw configslash commandredact()before writingshields downrejects missing flag values and unknown flagsparseCurrentPolicy()strips metadata header from policy snapshotsTest plan
test/e2e/test-shields-config.sh— full 12-phase lifecycle (37/37 pass on CI)Supersedes #1849.
Summary by CodeRabbit
shields down|up|statuswith timeout, reason, policy snapshot, audit logging, and auto-restore; plus matching slash command output.config get|set|rotate-tokenwith redacted displays, validation, and optional in-sandbox apply.