feat(sandbox-mgmt): phase 1 MVP — shields, config get, audit logging#1849
Closed
feat(sandbox-mgmt): phase 1 MVP — shields, config get, audit logging#1849
Conversation
…NEMOCLAW_PREFERRED_API override Backends like SGLang expose /v1/responses and pass the existing non-streaming validation probe, but their streaming mode only emits lifecycle events (created/in_progress/completed) without the granular content deltas OpenClaw requires (output_text.delta, etc.). This causes runtime failures after onboarding succeeds. Changes: - Add runStreamingEventProbe() in http-probe.ts that sends a stream:true request and verifies the SSE event stream includes response.output_text.delta - Integrate the streaming probe into probeOpenAiLikeEndpoint for custom endpoints (probeStreaming: true) — falls back to /chat/completions when streaming events are incomplete - Add shouldForceCompletionsApi() in validation.ts checking NEMOCLAW_PREFERRED_API env var so users can bypass /responses entirely - Wire both into validateCustomOpenAiLikeSelection - Add unit tests for the new functions (11 new test cases) - Document NEMOCLAW_PREFERRED_API, the NEMOCLAW_INFERENCE_API_OVERRIDE workaround, and a troubleshooting entry for the runtime failure scenario Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…file ARG precedence NEMOCLAW_INFERENCE_API_OVERRIDE only patches openclaw.json at container startup — it does not update the Dockerfile ARG baked into the image. On recreate-sandbox the baked value wins. The reliable fix is a fresh nemoclaw onboard which re-probes and rebakes the image. Updated all three doc pages to recommend nemoclaw onboard instead of the override env var, and added a note explaining the limitation. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
- Distinguish transport failures from missing-events in streaming probe fallback: only fall back to /chat/completions when missingEvents is non-empty; surface transport errors as hard validation failures - Make shouldForceCompletionsApi() pure by accepting the preferred API value as a parameter instead of reading process.env directly, keeping validation.ts free of I/O per its module contract - Fix passive voice and second-person wording in docs Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Implements shields down/up/status, config get, and audit logging from the Sandbox Management Commands RFC. Security invariants enforced: - Host-only mutations - Credential redaction - 30-minute max timeout - Exact policy snapshot restore RFC: NemoClaw Sandbox Management Commands v0.1 Signed-off-by: Test User <test@example.com>
Signed-off-by: Test User <test@example.com>
Contributor
📝 WalkthroughWalkthroughAdds shields (per-sandbox lockdown) and sandbox config management: state schema extensions, policy snapshot/restore with auto-restore timers and audit logging, host-side config get/set/rotate-token with SSRF checks, duration parsing, CLI + slash-command integrations, tests, and E2E coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as NemoClaw CLI
participant Shields as shields.ts
participant State as State Store
participant Runner as Policy Runner
participant Timer as Auto-Restore Timer
participant Audit as Audit Log
CLI->>Shields: shieldsDown(sandbox, {timeout, reason, policy})
Shields->>Runner: capture current policy
Runner-->>Shields: policy YAML
Shields->>State: write policy snapshot + down state
Shields->>Runner: apply relaxed policy
Shields->>Timer: fork timer (snapshot, restore-at)
Shields->>Audit: append shields_down entry
Shields-->>CLI: confirm DOWN
sequenceDiagram
participant Timer as Timer Process
participant State as State Store
participant Runner as Policy Runner
participant Audit as Audit Log
Timer->>Timer: wait until restore time
Timer->>State: verify snapshot exists
alt snapshot exists
Timer->>Runner: apply snapshot (restore)
Runner-->>Timer: success
Timer->>State: clear down fields
Timer->>Audit: append shields_auto_restore
Timer-->>Timer: exit 0
else failure
Timer->>Audit: append shields_up_failed
Timer-->>Timer: exit 1
end
sequenceDiagram
participant CLI as NemoClaw CLI
participant Config as sandbox-config.ts
participant Resolver as Agent Resolver
participant Sandbox as Running Sandbox
participant Validator as URL Validator
participant Audit as Audit Log
CLI->>Config: configGet(sandbox, {key?, format})
Config->>Resolver: resolve agent config path/format
Resolver-->>Config: agent details
Config->>Sandbox: exec cat <configPath>
Sandbox-->>Config: raw config
Config->>Config: parse, redact, remove gateway
alt key provided
Config->>Config: extract dotpath
end
Config-->>CLI: formatted output
CLI->>Config: configSet(sandbox, {key,value,restart})
Config->>Sandbox: read current config
Config->>Validator: validate URL values if applicable
Validator-->>Config: ok / error
Config->>Sandbox: write updated config
opt restart
Config->>Sandbox: signal reload
end
Config->>Audit: append config_set entry
Config-->>CLI: confirm change
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
Contributor
|
🚀 Docs preview ready! |
Signed-off-by: Test User <test@example.com>
ericksoa
changed the title
Hermes caches skill slash-commands in a module-global dict on first scan, making skills installed after gateway startup invisible. This adds a nemoclaw_reload_skills tool that clears the cache and re-scans, plus auto-refresh on session start, so new skills are available without a gateway restart. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eload" This reverts commit 297d175.
Contributor
|
✅ Brev E2E (all): PASSED on branch
|
Contributor
|
✅ Brev E2E (all): PASSED on branch
|
Contributor
|
❌ Brev E2E (all): FAILED on branch
|
Contributor
|
✅ Brev E2E (all): PASSED on branch
|
wscurran
added
security
Resolve conflicts: - nemoclaw/src/blueprint/state.ts: merge rebuild + shields fields - src/nemoclaw.ts: merge skill/rebuild/snapshot + shields/config cases - state.test.ts: add rebuild fields to full state assertion Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…ules - shields.ts: remove @ts-nocheck, import duration parsing from shared duration.ts, import audit logging from shields-audit.ts, add types - sandbox-config.ts: remove @ts-nocheck, import stripCredentials from credential-filter.ts instead of inlining, add types - Delete unused credential-strip.ts (sandbox-config.ts now uses credential-filter.ts, plugin config-show.ts does its own redaction) All 1645 tests pass. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
![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: 17
🧹 Nitpick comments (2)
src/lib/sandbox-config.ts (1)
10-11: Consider using ESM imports instead of CommonJSrequire.This TypeScript file uses CommonJS
require()while using TypeScript syntax (interfaces, type annotations). For consistency with other files insrc/lib/and to enable better static analysis and tree-shaking, consider ESM imports:-const { runCapture, validateName, shellQuote } = require("./runner"); -const { stripCredentials } = require("./credential-filter"); +import { runCapture, validateName, shellQuote } from "./runner.js"; +import { stripCredentials } from "./credential-filter.js";🤖 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 10 - 11, The file mixes CommonJS require() with TypeScript; replace the require calls that import runCapture, validateName, shellQuote and stripCredentials with ESM import statements (e.g., import { runCapture, validateName, shellQuote } from "./runner" and import { stripCredentials } from "./credential-filter"), update any default/CommonJS export usage to named exports if needed, and ensure the module target in tsconfig supports ESM so TypeScript compiles the new import syntax correctly.nemoclaw/src/commands/slash.test.ts (1)
74-80: Same concern:blankStatemay be missing required fields.Similar to
shields-status.test.ts, thisblankState()helper is missinglastRebuildAtandlastRebuildBackupPathfields. Consider adding them for consistency and type safety:updatedAt: new Date().toISOString(), + lastRebuildAt: null, + lastRebuildBackupPath: null, shieldsDown: false,🤖 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 74 - 80, The blankState() test helper in slash.test.ts is missing the required fields lastRebuildAt and lastRebuildBackupPath (causing type/consistency issues); update the blankState object to include lastRebuildAt: null and lastRebuildBackupPath: null (or appropriate default values matching your State type) so it matches the other helper (shields-status.test.ts) and satisfies type safety for functions that consume blankState.
🤖 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 39-41: The output currently prints raw endpointUrl via
describeOnboardEndpoint(config), which can leak basic-auth usernames/passwords
or tokenized query params; update describeOnboardEndpoint (and callers such as
the template in config-show.ts) to parse and redact sensitive parts of
config.endpointUrl: remove username/password from URL.origin, strip or replace
query parameter values (e.g., show keys only or replace values with
"<redacted>"), and preserve endpointType for non-local routes; use a robust URL
parser (URL class) to reconstruct a safe string like "type
(https://host[:port]/path?param=<redacted>)" or simply "type
(https://host/path...)" so no credentials or token values are echoed.
- Around line 47-48: Update the user-facing hint strings in the config-show
command so they show the actual host CLI syntax accepted by the dispatcher:
replace "Use `nemoclaw config get <sandbox>` for the full sandbox config." with
"Use `nemoclaw <sandbox> config get` for the full sandbox config." in the
message array in commands/config-show.ts (the constant/array that contains the
two hint strings used when refusing sandbox-side config modification).
In `@nemoclaw/src/commands/shields-status.test.ts`:
- Around line 16-33: The helper function blankState() is missing the
NemoClawState fields lastRebuildAt and lastRebuildBackupPath; update
blankState() to include lastRebuildAt: null and lastRebuildBackupPath: null so
the returned object conforms to NemoClawState and avoids type errors.
In `@nemoclaw/src/commands/shields-status.ts`:
- Around line 42-45: Update the recovery hint emitted in shields-status.ts so it
includes the sandbox argument; specifically, change the string pushed in
lines.push (the warning about running "nemoclaw shields up") to either instruct
using "nemoclaw <sandbox> shields up" or interpolate the current sandbox name
using state.sandboxName when available (refer to state.sandboxName and the
lines.push call in shields-status.ts to locate and update the message).
In `@src/lib/sandbox-config.ts`:
- Around line 49-58: Remove the redundant try/catch around runCapture and
instead call runCapture(cmd, { ignoreError: false }) so errors propagate; then
wrap that call in a single catch that inspects the thrown error (and the
resulting raw output) to produce specific messages: if error.code === 'ENOENT'
log that the openshell binary was not found, if error.code indicates permission
denied log permission issues, if no error but raw is empty/whitespace log that
the sandbox config was not read (suggest sandbox not running or config missing),
and in all cases include the original error/message for diagnostics; update
references to runCapture, cmd, and raw in sandbox-config.ts accordingly.
In `@src/lib/shields-audit.ts`:
- Around line 4-10: The doc claims entries never contain credential values but
appendAuditEntry currently writes entries directly; update appendAuditEntry to
defensively sanitize any user-provided fields by running stripCredentials
(and/or a new sanitizeAuditEntry helper) on the reason and the whole entry
object before serializing to JSONL so no credential-like patterns are written;
ensure this happens inside appendAuditEntry (and any other writer helpers) so
callers need not trust they sanitized values.
- Around line 16-17: The current AUDIT_DIR/AUDIT_FILE fallback to "/tmp" when
HOME is unset is unsafe on multi-user systems; change the logic so that if HOME
is unset you either throw an explicit error (fail fast) or create a
process-specific, restricted directory instead of a global /tmp path.
Specifically update the code that computes AUDIT_DIR/AUDIT_FILE to: prefer HOME,
otherwise use a secure temp location (e.g., os.tmpdir() combined with a unique
suffix like process.pid or mkdtemp-style name or XDG_RUNTIME_DIR) and ensure the
directory is created with restrictive permissions (mode 0o700) before writing
shields-audit.jsonl; if you choose to fail, throw a clear error when HOME is
missing. Ensure these changes are applied around the AUDIT_DIR/AUDIT_FILE
definitions and any directory-creation logic that uses them.
In `@src/lib/shields-timer.ts`:
- Around line 77-97: The restore command is being executed with
run(buildPolicySetCommand(...), { ignoreError: true }) but the code always calls
updateState(...) and appendAudit(...) regardless of whether the restore actually
succeeded; change the logic so you first capture the result/error of
run(buildPolicySetCommand(...)) and only call updateState (clearing
shieldsDown*, shieldsDownAt, shieldsDownTimeout, shieldsDownReason,
shieldsDownPolicy) and appendAudit({... action: "shields_auto_restore" ...})
when the run completed successfully; if run fails, do not clear the shields
state or record a successful restore—log or record the failure instead so the
sandbox remains correctly marked as relaxed until a real restore occurs.
- Around line 1-15: Remove the top-line directive "// `@ts-nocheck`" from
shields-timer.ts and restore full TypeScript checking; replace CommonJS requires
with typed ES module imports for fs, path, run (from "./runner") and
buildPolicySetCommand (from "./policies"), add explicit type annotations for any
function parameters/variables used in the timer logic (e.g. the forked process
args: sandbox name, snapshot path, timeoutSeconds) and for the return types of
functions that call run or buildPolicySetCommand, and then run the ts compiler
(tsconfig.cli.json) to fix any resulting type errors reported and update code
accordingly until type-check passes. Ensure you reference and keep the existing
usage of run and buildPolicySetCommand intact while adding types.
In `@src/lib/shields.ts`:
- Around line 35-59: loadShieldsState()/saveShieldsState() currently persist a
single flat ShieldsState which causes cross-sandbox leakage; change their
storage shape to map sandboxName -> ShieldsState (e.g., Record<string,
ShieldsState>) keyed by the sandbox name, update both loadShieldsState and
saveShieldsState to read/write the per-sandbox entry (using STATE_FILE/STATE_DIR
as before) so callers like shieldsStatus, shieldsUp, and any code referencing
shieldsPolicySnapshotPath use the sandbox-scoped state; ensure saveShieldsState
merges the patch into the specific sandbox entry (preserving other sandboxes)
and that loadShieldsState returns the sandbox's ShieldsState (or {} if missing),
preserving updatedAt semantics and file permissions.
- Around line 164-202: If fork() fails we must not leave the sandbox relaxed:
after the catch in the timer-start block, roll back the previously persisted
relaxed policy by (1) restoring the captured snapshot file at snapshotPath (call
the existing snapshot restore routine—e.g. the function that performs sandbox
restore/restoreSnapshot/restoreSandboxFromSnapshot in this module or service),
and (2) call saveShieldsState(...) to clear shieldsDown (set shieldsDown: false,
clear
shieldsDownAt/shieldsDownTimeout/shieldsDownReason/shieldsDownPolicy/shieldsPolicySnapshotPath).
Do this before logging the warning and re-throw or return a non-zero failure so
the command fails rather than leaving the sandbox relaxed; reference
saveShieldsState, snapshotPath, fork, timerMarkerPath and the timer script names
(shields-timer.ts / shields-timer.js) to locate the code to change.
- Around line 87-100: The killTimer function currently unconditionally signals
the PID read from readTimerMarker(sandboxName) which risks killing an unrelated
reused PID; update the marker format (readTimerMarker and wherever it's written)
to include additional identity fields such as ppid, startTime (or command/argv)
and/or a unique token, then in killTimer verify the on-disk PID matches those
identity fields before calling process.kill(marker.pid, "SIGTERM") (e.g., check
process existence and compare /proc/<pid>/stat starttime or /proc/<pid>/cmdline
on Linux, or use a platform-appropriate ps API), and if verification fails avoid
sending the signal and still remove the marker via timerMarkerPath(sandboxName)
or implement a cancellation handshake/token file that the timer process checks
to exit itself instead of directly killing the PID.
In `@src/nemoclaw.ts`:
- Around line 2131-2136: The parser loop around actionArgs builds configOpts
with possible missing or invalid values which are silently accepted; update the
handling before calling sandboxConfig.configGet(cmd, configOpts) to validate
that when an option flag is present it has a following value (e.g. when
actionArgs[i] === "--key" or "--format" ensure actionArgs[i+1] exists and is not
another flag) and to validate allowed format values (accept only "json" or
"yaml"/"yml"); on invalid/missing values return a clear user-facing error or
throw so callers/scripts cannot silently proceed, and reference the existing
symbols configOpts, actionArgs and sandboxConfig.configGet to locate where to
add these checks.
In `@test/duration.test.ts`:
- Around line 1-3: Remove the leading "@ts-nocheck" and place the SPDX license
header as the very first lines of the file (move the current
SPDX-FileCopyrightText and SPDX-License-Identifier above any other content),
then run TypeScript (tsc) or your editor checks and fix any resulting type
errors in this test file (address incorrect typings, imports, or any implicit
any) so the file passes type checking; target the top-of-file directive and
existing test declarations in this file when making edits.
In `@test/shields-audit.test.ts`:
- Around line 29-31: The test currently reimplements appendAuditEntry(auditPath)
and never goes through the real audit logger or redaction/filtering logic;
replace the local appendAuditEntry with calls to the real module export from
src/lib/shields-audit.ts (import the audit logger or its write function) so the
test exercises directory creation, file-mode handling, and filtration/redaction
paths, and change the payload to include a secret-like value (e.g., a token
string) so redaction is triggered; also update the other occurrences noted
(lines ~99-114) to use the real logger and assert on file permissions, existence
of created directories, and that the secret is redacted in the audit file.
In `@test/shields.test.ts`:
- Around line 13-32: The tests are not exercising src/lib/shields.ts — update
the test cases to call the real exported functions shieldsDown() and shieldsUp()
instead of hand-writing nemoclaw.json, snapshots, timer markers, and audit files
so the actual implementation is exercised; ensure you import shieldsDown and
shieldsUp from src/lib/shields, trigger them in the relevant test flows, and
assert that mocked helpers (runner.run / runner.runCapture,
policies.buildPolicySetCommand / buildPolicyGetCommand / parseCurrentPolicy, and
child_process.fork) are invoked with the expected arguments and that
snapshot/audit outputs are produced by the implementation rather than being
manually created.
---
Nitpick comments:
In `@nemoclaw/src/commands/slash.test.ts`:
- Around line 74-80: The blankState() test helper in slash.test.ts is missing
the required fields lastRebuildAt and lastRebuildBackupPath (causing
type/consistency issues); update the blankState object to include lastRebuildAt:
null and lastRebuildBackupPath: null (or appropriate default values matching
your State type) so it matches the other helper (shields-status.test.ts) and
satisfies type safety for functions that consume blankState.
In `@src/lib/sandbox-config.ts`:
- Around line 10-11: The file mixes CommonJS require() with TypeScript; replace
the require calls that import runCapture, validateName, shellQuote and
stripCredentials with ESM import statements (e.g., import { runCapture,
validateName, shellQuote } from "./runner" and import { stripCredentials } from
"./credential-filter"), update any default/CommonJS export usage to named
exports if needed, and ensure the module target in tsconfig supports ESM so
TypeScript compiles the new import syntax correctly.
🪄 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: 3c3ff043-0541-4b15-bee3-99b6f17e76bf
📒 Files selected for processing (17)
nemoclaw/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.tssrc/lib/duration.tssrc/lib/sandbox-config.tssrc/lib/shields-audit.tssrc/lib/shields-timer.tssrc/lib/shields.tssrc/nemoclaw.tstest/duration.test.tstest/shields-audit.test.tstest/shields.test.ts
Phase 2 of the Sandbox Management Commands RFC: - config set: host-initiated config mutation with dotpath addressing, JSON value parsing, SSRF validation for URLs, gateway section protection, and optional sandbox restart signal - config rotate-token: credential rotation via --from-env or --from-stdin (never CLI args), saves to credentials.json, updates openshell provider - 32 new tests covering dotpath operations, URL validation, exports Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Config get/set/rotate-token were hardcoded to read/write /sandbox/.openclaw/openclaw.json. This breaks for Hermes sandboxes, which use /sandbox/.hermes/config.yaml (YAML format). Add resolveAgentConfig() which looks up the sandbox's agent from the registry, loads the agent definition from agents/*/manifest.yaml, and returns the correct config path, directory, format, and filename. All config commands now route through this resolver. - config get: outputs in agent's native format by default (JSON for openclaw, YAML for hermes) - config set: writes in agent's native format, shows agent name in output and audit trail - rotate-token: includes agent name in audit entries - CLI output now shows which agent is being configured Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
config set and config rotate-token now refuse to run unless shields are down. This enforces the security model where shields down is the explicit gate that unlocks config mutability, and shields up (or the auto-restore timer) re-locks it. Operator flow: nemoclaw <sandbox> shields down --reason "rotating token" nemoclaw <sandbox> config set --key inference.model --value "new-model" nemoclaw <sandbox> shields up Add isShieldsDown() to shields.ts and requireShieldsDown() guard in sandbox-config.ts. config get remains available with shields up (read-only, no mutation). Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…lity Shields down now makes the agent config file physically writable inside the sandbox. Shields up re-locks it. No software-level gate — the OS permissions ARE the enforcement. Mechanical changes: - shields down: after applying permissive policy, chmod 666 the config file and chmod 777 the config dir via openshell sandbox exec (runs as root, bypasses Landlock) - shields up: after restoring restrictive policy, chmod 444 the config file and chown root:root + chmod 755 the config dir - openclaw permissive policy: add /sandbox/.openclaw to read_write so Landlock also allows writes (UNIX perms + Landlock must both agree) - hermes permissive policy: move /sandbox/.hermes from read_only to read_write, same reasoning - Remove software requireShieldsDown() gate from config set/rotate-token — the filesystem permissions are the real enforcement Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Nightly E2E covering the full shields down/up and config get/set/ rotate-token lifecycle against a live sandbox: - Phase 2: config immutability with shields UP (UNIX perms block writes) - Phase 3: shields down makes config writable (chmod 666) - Phase 4: config get with credential redaction and gateway stripping - Phase 5: config set with dotpath, gateway protection, SSRF validation - Phase 6: shields status shows DOWN with reason and timeout - Phase 7: shields up re-locks config (chmod 444, chown root:root) - Phase 8: config changes persist through shields cycle - Phase 9: rotate-token via --from-env with redacted output - Phase 10: audit trail completeness and credential-free verification - Phase 11: auto-restore timer fires after short timeout - Phase 12: double shields-down rejected Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…' into feat/sandbox-management-commands
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
src/nemoclaw.ts (2)
2130-2137:⚠️ Potential issue | 🟡 Minor
config getsilently accepts invalid--formatvalues.The parsing loop doesn't validate that
--formatis one of the allowed values (json,yaml). Any unexpected value falls through to JSON output without an error, which could mislead scripts expecting YAML.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 2130 - 2137, The "get" handler (case "get") currently parses actionArgs into configOpts.format without validating allowed values, so invalid formats silently default to "json"; update the parsing loop that builds configOpts (referencing actionArgs and configOpts) to validate that any value provided for "--format" is one of the allowed values ("json" or "yaml") and, if not, print a clear error and exit (or throw) before calling sandboxConfig.configGet(cmd, configOpts); ensure the validation occurs immediately after consuming the format token so configGet always receives a valid format.
2102-2111:⚠️ Potential issue | 🟠 Major
shields downsilently accepts missing option values and unknown flags.The flag parsing loop blindly consumes the next token for
--timeout,--reason, and--policy. If a flag appears at the end without a value (e.g.,shields down --timeout), it silently falls back to defaults. Unknown flags are also silently ignored. For a security-sensitive command, this should fail fast.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 2102 - 2111, The current shields down parsing loop (shieldsFlags and opts used before calling shields.shieldsDown) silently accepts missing values and ignores unknown flags; change it to validate each flag: when encountering "--timeout", "--reason", or "--policy" ensure there is a next token (i+1 < shieldsFlags.length) and that that token is not another flag (doesn't start with "--"); if validation fails, throw or return a clear error message instead of falling back to defaults. Also treat any token that doesn't match the three known flags as an unknown flag and fail fast with an error instead of ignoring it, then only call shields.shieldsDown(cmd, opts) when all flags are validated and consumed.src/lib/shields.ts (3)
46-61:⚠️ Potential issue | 🔴 CriticalShields state is not namespaced by sandbox — cross-sandbox leakage.
loadShieldsState()andsaveShieldsState()operate on a single flatnemoclaw.jsonfile without keying by sandbox name. This causes incorrect behavior when managing multiple sandboxes:
- After
shields downon sandboxalpha, callingshieldsStatus("beta")reportsalpha's state.shieldsUp("beta")could restorealpha's snapshot ontobeta.The state file should store a
Record<string, ShieldsState>keyed by sandbox name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shields.ts` around lines 46 - 61, The current loadShieldsState and saveShieldsState always read/write a single flat ShieldsState to STATE_FILE causing cross-sandbox leakage; change the backing format to a Record<string, ShieldsState> keyed by sandbox name and update the functions to load the whole map, select or update the entry for the given sandbox, and persist the entire map to STATE_FILE (use STATE_DIR/STATE_FILE and same mkdir/write modes). Specifically modify loadShieldsState and saveShieldsState (or add loadShieldsMap/saveShieldsMap helpers) so they operate on the map, have saveShieldsState merge the patch into map[sandboxName] and set updatedAt for that sandbox only, and have loadShieldsState return the sandbox-specific ShieldsState (or undefined/empty) while preserving existing other sandbox entries in the file.
88-101:⚠️ Potential issue | 🟠 MajorPID reuse risk when killing timer process.
killTimer()sendsSIGTERMtomarker.pidread from disk without verifying the PID still belongs to the timer process. If the timer exited and the OS reused that PID for an unrelated process, this could terminate an innocent process.Consider storing additional identity info (e.g., start time, ppid, or a unique token) in the marker and verifying before signaling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shields.ts` around lines 88 - 101, The killTimer function currently reads marker.pid (from readTimerMarker) and signals it directly, risking killing an unrelated PID if the original timer exited and the PID was reused; change the marker format (readTimerMarker / timerMarkerPath) to include extra identity info (e.g., startTime or parentPid or a unique token) when writing the marker, then in killTimer verify that the process with marker.pid matches that identity before sending SIGTERM (for example, compare the process start time or ppid or command line from the OS /proc or platform-appropriate API against marker.startTime/marker.ppid/marker.token), only then call process.kill, and still remove the marker file on success or if the process is confirmed gone; ensure readTimerMarker is updated to parse the new fields and handle platforms gracefully.
195-218:⚠️ Potential issue | 🔴 CriticalTimer fork failure leaves sandbox relaxed indefinitely.
When
fork()fails (lines 214-218), the code only logs a warning and continues. At this point, the relaxed policy is already applied (line 161) and state is persisted asshieldsDown: true(lines 180-187). This breaks the time-bounded security invariant — the sandbox remains relaxed with no auto-restore.The code should roll back the policy change by restoring the snapshot and clearing the shields-down state before returning an error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shields.ts` around lines 195 - 218, The try/catch around fork(actualScript, ...) currently only logs on failure but leaves the sandbox relaxed and persisted as shieldsDown; on fork error you must roll back the policy change: in the catch block call the snapshot restore routine (e.g., restoreSnapshot or the existing function used elsewhere to revert to snapshotPath for sandboxName), clear the persisted shieldsDown state (the same state updater used where shieldsDown was set true), remove any timer marker file created via timerMarkerPath(sandboxName) if present, and then propagate the error (throw or return an error) instead of silently continuing; reference the fork call, actualScript, sandboxName, snapshotPath, timerMarkerPath and the shieldsDown state updater when implementing this rollback.src/lib/sandbox-config.ts (1)
149-160:⚠️ Potential issue | 🟡 MinorRedundant try-catch and ambiguous error messaging.
The try-catch around
runCapturewithignoreError: trueis redundant sincerunCapturealready handles errors internally. Additionally, the error message "Is the sandbox running?" doesn't distinguish between multiple failure modes (binary not found, sandbox not running, permission denied, empty config).🤖 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 149 - 160, Remove the redundant try/catch around runCapture and rely on its internal error handling; call raw = runCapture(cmd, { ignoreError: true }) directly, then replace the single vague error block with targeted checks: if the config file at target.configPath does not exist (use fs.existsSync) log "config file missing for {target.agentName}", else if raw is empty re-run runCapture(cmd) without ignoreError to capture the actual error/ stderr and include that in the log to distinguish "binary not found / permission denied / sandbox not running", and ensure messages include target.agentName and target.configPath for context.
🧹 Nitpick comments (1)
src/lib/shields.ts (1)
172-176: Consider using 755/644 instead of 777/666 for config permissions.Setting
chmod 777on the config directory andchmod 666on the config file grants world-readable/writable permissions. While this is inside a sandbox, using755and644(or775/664) would be sufficient to allow the sandbox user to write while maintaining better hygiene.Suggested change
- run(`${openshell} sandbox exec ${sandboxName} chmod 777 ${target.configDir}`, { ignoreError: true }); - run(`${openshell} sandbox exec ${sandboxName} chmod 666 ${target.configPath}`, { ignoreError: true }); + run(`${openshell} sandbox exec ${sandboxName} chmod 755 ${target.configDir}`, { ignoreError: true }); + run(`${openshell} sandbox exec ${sandboxName} chmod 644 ${target.configPath}`, { ignoreError: true }); + run(`${openshell} sandbox exec ${sandboxName} chown sandbox:sandbox ${target.configPath}`, { ignoreError: true });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shields.ts` around lines 172 - 176, The chmod calls in the try block use overly-permissive modes (777/666); update the two run(...) invocations that reference target.configDir and target.configPath (the calls using `${openshell} sandbox exec ${sandboxName} chmod ...`) to use safer modes such as 755 for directories and 644 for files (or 775/664 if group write is required by the sandbox), keeping the same ignoreError behavior and leaving the existing catch/error message intact.
🤖 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-config.ts`:
- Around line 478-485: The audit entry for the rotate-token operation uses the
wrong action value ("shields_down"); update the object passed to
appendAuditEntry in the rotate-token flow to use a clear, dedicated action like
"token_rotated" (keep sandbox: sandboxName, timestamp: new Date().toISOString(),
and reason: `rotate-token ${target.agentName}:${credentialEnv}` intact) so the
audit log correctly reflects the token rotation event.
- Around line 347-354: The audit entry for config mutations is using the wrong
action; change the call to appendAuditEntry in the config set flow to use
action: "config_set" instead of "shields_down", and then add "config_set" to the
allowed action types in shields-audit.ts (update the union/type/enum that
defines audit actions and any related validations). Also search for any
validation or switch logic that checks audit actions and ensure "config_set" is
handled consistently (e.g., in typing, serializers, or tests).
- Around line 175-189: The isPrivateIp function currently only checks IPv4 and
the literal IPv6 loopback “[::1]” — update isPrivateIp to normalize IPv6
hostnames by stripping surrounding brackets (e.g., "[fd00::1]"), then add checks
for IPv6 link-local and unique-local ranges: detect hostnames that start with
"fe80:" (fe80::/10) and those that start with "fc" or "fd" (fc00::/7) — e.g.,
add constants or regexes alongside PRIVATE_IP_PREFIXES/PRIVATE_IP_172_RE and
return true for these matches within isPrivateIp.
In `@test/config-set.test.ts`:
- Around line 88-89: Rename the test case title to match the assertion: change
the `it("accepts public http URLs", ...)` description to `it("accepts public
https URLs", ...)` so the test name aligns with the URL passed to
`validateUrlValue("https://example.com")`; update the `it` string near the
`validateUrlValue` call to reflect HTTPS.
---
Duplicate comments:
In `@src/lib/sandbox-config.ts`:
- Around line 149-160: Remove the redundant try/catch around runCapture and rely
on its internal error handling; call raw = runCapture(cmd, { ignoreError: true
}) directly, then replace the single vague error block with targeted checks: if
the config file at target.configPath does not exist (use fs.existsSync) log
"config file missing for {target.agentName}", else if raw is empty re-run
runCapture(cmd) without ignoreError to capture the actual error/ stderr and
include that in the log to distinguish "binary not found / permission denied /
sandbox not running", and ensure messages include target.agentName and
target.configPath for context.
In `@src/lib/shields.ts`:
- Around line 46-61: The current loadShieldsState and saveShieldsState always
read/write a single flat ShieldsState to STATE_FILE causing cross-sandbox
leakage; change the backing format to a Record<string, ShieldsState> keyed by
sandbox name and update the functions to load the whole map, select or update
the entry for the given sandbox, and persist the entire map to STATE_FILE (use
STATE_DIR/STATE_FILE and same mkdir/write modes). Specifically modify
loadShieldsState and saveShieldsState (or add loadShieldsMap/saveShieldsMap
helpers) so they operate on the map, have saveShieldsState merge the patch into
map[sandboxName] and set updatedAt for that sandbox only, and have
loadShieldsState return the sandbox-specific ShieldsState (or undefined/empty)
while preserving existing other sandbox entries in the file.
- Around line 88-101: The killTimer function currently reads marker.pid (from
readTimerMarker) and signals it directly, risking killing an unrelated PID if
the original timer exited and the PID was reused; change the marker format
(readTimerMarker / timerMarkerPath) to include extra identity info (e.g.,
startTime or parentPid or a unique token) when writing the marker, then in
killTimer verify that the process with marker.pid matches that identity before
sending SIGTERM (for example, compare the process start time or ppid or command
line from the OS /proc or platform-appropriate API against
marker.startTime/marker.ppid/marker.token), only then call process.kill, and
still remove the marker file on success or if the process is confirmed gone;
ensure readTimerMarker is updated to parse the new fields and handle platforms
gracefully.
- Around line 195-218: The try/catch around fork(actualScript, ...) currently
only logs on failure but leaves the sandbox relaxed and persisted as
shieldsDown; on fork error you must roll back the policy change: in the catch
block call the snapshot restore routine (e.g., restoreSnapshot or the existing
function used elsewhere to revert to snapshotPath for sandboxName), clear the
persisted shieldsDown state (the same state updater used where shieldsDown was
set true), remove any timer marker file created via timerMarkerPath(sandboxName)
if present, and then propagate the error (throw or return an error) instead of
silently continuing; reference the fork call, actualScript, sandboxName,
snapshotPath, timerMarkerPath and the shieldsDown state updater when
implementing this rollback.
In `@src/nemoclaw.ts`:
- Around line 2130-2137: The "get" handler (case "get") currently parses
actionArgs into configOpts.format without validating allowed values, so invalid
formats silently default to "json"; update the parsing loop that builds
configOpts (referencing actionArgs and configOpts) to validate that any value
provided for "--format" is one of the allowed values ("json" or "yaml") and, if
not, print a clear error and exit (or throw) before calling
sandboxConfig.configGet(cmd, configOpts); ensure the validation occurs
immediately after consuming the format token so configGet always receives a
valid format.
- Around line 2102-2111: The current shields down parsing loop (shieldsFlags and
opts used before calling shields.shieldsDown) silently accepts missing values
and ignores unknown flags; change it to validate each flag: when encountering
"--timeout", "--reason", or "--policy" ensure there is a next token (i+1 <
shieldsFlags.length) and that that token is not another flag (doesn't start with
"--"); if validation fails, throw or return a clear error message instead of
falling back to defaults. Also treat any token that doesn't match the three
known flags as an unknown flag and fail fast with an error instead of ignoring
it, then only call shields.shieldsDown(cmd, opts) when all flags are validated
and consumed.
---
Nitpick comments:
In `@src/lib/shields.ts`:
- Around line 172-176: The chmod calls in the try block use overly-permissive
modes (777/666); update the two run(...) invocations that reference
target.configDir and target.configPath (the calls using `${openshell} sandbox
exec ${sandboxName} chmod ...`) to use safer modes such as 755 for directories
and 644 for files (or 775/664 if group write is required by the sandbox),
keeping the same ignoreError behavior and leaving the existing catch/error
message intact.
🪄 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: 8688aa13-3ba9-43bd-aa8a-2ddabb0e1c0f
📒 Files selected for processing (9)
.agents/skills/nemoclaw-user-reference/references/commands.mdagents/hermes/policy-permissive.yamlnemoclaw-blueprint/policies/openclaw-sandbox-permissive.yamlsrc/lib/sandbox-config.tssrc/lib/shields.tssrc/nemoclaw.tstest/config-rotate-token.test.tstest/config-set.test.tstest/e2e/test-shields-config.sh
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/nemoclaw-user-reference/references/commands.md
Wire test/e2e/test-shields-config.sh into the nightly-e2e workflow as a new parallel job. Added to the notify-on-failure needs list. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
OpenShell rejects changing include_workdir on a live sandbox: "filesystem include_workdir cannot be changed on a live sandbox" The default policy uses include_workdir: false. The permissive policy used include_workdir: true, which made shields down fail on every running sandbox. Fix: set include_workdir: false in both permissive policies and explicitly list /sandbox and the config dir in read_write instead. This achieves the same filesystem access without triggering the OpenShell validation error. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
OpenShell also rejects removing read_only paths on a live sandbox: "filesystem read_only path '/sandbox' cannot be removed on a live sandbox" Fix: keep /sandbox and the config dir in both read_only AND read_write. Landlock grants the union of all matching rules, so a path in both lists gets read-write access. This satisfies OpenShell's constraint that read_only paths can only be added, never removed, on live sandboxes. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@agents/hermes/policy-permissive.yaml`:
- Around line 14-17: The include_workdir value in policy-permissive.yaml
(include_workdir: false) conflicts with the Hermes strict policy's
include_workdir (true); change include_workdir in this permissive policy to
match the default/strict policy (set include_workdir: true or remove the
override so it inherits the default) to avoid OpenShell rejecting live policy
changes—update the include_workdir key in the policy-permissive.yaml entry that
currently sets include_workdir to false to match the Hermes strict snippet.
🪄 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: 9fe3d493-0477-4141-8dc2-5b6a94be46c0
📒 Files selected for processing (2)
agents/hermes/policy-permissive.yamlnemoclaw-blueprint/policies/openclaw-sandbox-permissive.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- nemoclaw-blueprint/policies/openclaw-sandbox-permissive.yaml
Three layers protect the config file: Landlock (read_only), chattr +i (immutable bit), and UNIX perms (444 root:root). openshell sandbox exec runs INSIDE the Landlock domain and can't bypass any of them. Fix: route chmod/chattr through docker exec → kubectl exec, which starts a new process in the pod OUTSIDE the sandbox's Landlock ruleset. Verified empirically: chattr -i + chmod 666 via kubectl makes the config writable from inside the sandbox, and chattr +i + chmod 444 re-locks it. Also fix permissive policies: filesystem_policy section must match the default policy exactly (OpenShell rejects include_workdir changes and read_only path removals on live sandboxes). Network relaxation is the only policy change; config mutability is handled by kubectl exec. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Dump state file and docker containers after shields down to diagnose why shields status shows UP on CI despite shields down succeeding. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
force-pushed
the
feat/sandbox-management-commands
branch
from
e996ccd to
47c3ec2
Compare
Root cause of shields status showing UP immediately after shields down:
the timer was forked with a relative timeout (300s) at the START of
shieldsDown(), but steps 1-2b (policy apply + kubectl chmod) take 5+
minutes on CI. By the time saveShieldsState({shieldsDown: true}) runs
at step 3, the timer's setTimeout has already elapsed and fires
immediately, resetting shieldsDown to false.
Fix: pass an absolute ISO timestamp (restoreAt) to the timer instead
of relative seconds. The timer computes delayMs = restoreAt - now,
so it always fires at the intended wall-clock time regardless of how
long the parent function takes.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…ad/write readSandboxConfig was constructing a shell string instead of using captureOpenshellCommand with the correct argv array format including the -- separator. configSet was using the nonexistent openshell sandbox cp subcommand. Switch config reads to captureOpenshellCommand with proper argv and config writes to kubectl exec via docker (same pattern shields.ts uses to bypass Landlock). Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
… --name flag shields-timer: replace fork() with spawn(process.execPath, [...]) to eliminate the IPC channel that fork() creates even with stdio:'ignore'. On CI the IPC disconnect when the parent exits was causing the detached timer child to terminate early, immediately resetting shieldsDown to false via the finally block. sandbox-config: openshell sandbox exec requires --name as a flag, not a positional argument. Without it the sandbox name was consumed as part of <COMMAND>..., so cat ran in the default sandbox context and returned empty output. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
openshell policy get --full returns a metadata header (Version, Hash, etc.) followed by --- and the actual policy YAML. The snapshot was saving the raw output including the header. When shields up restored from this snapshot, openshell policy set rejected the unknown Version field. Use parseCurrentPolicy() to strip the header before writing. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
spawn() caused the timer child to silently crash on CI (stdio:'ignore' swallowed all errors). Revert to fork() which handles Node.js module resolution correctly, but explicitly include 'ipc' in the stdio array and call child.disconnect() before child.unref(). This cleanly tears down the IPC channel while the parent is still alive, avoiding the surprise disconnect event that was causing early timer firing on CI. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
openshell policy set --wait takes ~8s on CI. The timer was updating shieldsDown state after the policy restore, so callers checking state within those 8 seconds still saw shields as DOWN. Move the state update before the policy restore so it reflects the timer's intent immediately. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
openshell policy set --wait can time out on slow CI runners, causing the first shields down to fail. The double-down rejection test then incorrectly passes a second shields down that should have been blocked. Retry the first shields down once before testing the guard. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…, redaction Critical: - Namespace shields state per sandbox (shields-<name>.json) so multiple sandboxes don't stomp each other's state. - Roll back permissive policy and restore shields if the auto-timer fork fails, instead of leaving the sandbox relaxed indefinitely. - Check policy restore result in the auto-timer; revert state to DOWN if the restore command fails. Major: - Redact credentials/tokens from endpoint URLs in config-show. - Redact reason and error fields in shields audit entries. - Validate that --timeout/--reason/--policy have values; reject unknown flags on shields down. - Remove unnecessary @ts-nocheck from test/duration.test.ts. - Update test/shields.test.ts for sandbox-namespaced state files and add mock setup for disconnect/execFileSync/audit. Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/lib/sandbox-config.ts (3)
486-493:⚠️ Potential issue | 🟡 MinorAudit action type is incorrect for token rotation.
Similar to
config set, therotate-tokenoperation usesaction: "shields_down"which doesn't accurately describe the operation. This should use a dedicated action type like"token_rotated".Suggested fix
appendAuditEntry({ - action: "shields_down", + action: "token_rotated", sandbox: sandboxName, timestamp: new Date().toISOString(), reason: `rotate-token ${target.agentName}:${credentialEnv}`, });🤖 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 486 - 493, The audit entry for the rotate-token flow uses an incorrect action value "shields_down"; update the appendAuditEntry call used during rotate-token (the block that builds { action: "shields_down", sandbox: sandboxName, ... reason: `rotate-token ${target.agentName}:${credentialEnv}` }) to use a dedicated action like "token_rotated" (or another agreed token rotation constant) so the audit log accurately represents the operation performed by rotate-token; ensure you change only the action field in that appendAuditEntry invocation (in the rotate-token handling path).
354-361:⚠️ Potential issue | 🟡 MinorAudit action type is incorrect for config set operations.
The audit entry uses
action: "shields_down"forconfig setoperations, which is semantically incorrect.shields_downindicates a policy relaxation event, not a config mutation. This makes the audit log misleading.Suggested fix
Add
"config_set"to the allowed action types inshields-audit.ts, then:appendAuditEntry({ - action: "shields_down", + action: "config_set", sandbox: sandboxName, timestamp: new Date().toISOString(), reason: `config set ${target.agentName}:${opts.key}`, });🤖 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 354 - 361, The audit entry for config mutations currently uses action: "shields_down" which is semantically wrong; update the call to appendAuditEntry in src/lib/sandbox-config.ts to use action: "config_set" for config set operations (the appendAuditEntry invocation that sets action, sandbox, timestamp, reason), and also add "config_set" to the allowed action types enum/validation in shields-audit.ts so the new action is accepted by the audit schema/validator.
181-188:⚠️ Potential issue | 🟡 MinorIPv6 private/link-local ranges not covered.
The
isPrivateIpfunction checks IPv4 private ranges and[::1]but doesn't cover IPv6 unique-local addresses (fc00::/7likefd00::) or link-local addresses (fe80::/10). A URL likehttp://[fd00::1]:8080/would pass the SSRF validation.Suggested enhancement
function isPrivateIp(hostname: string): boolean { if (hostname === "localhost" || hostname === "[::1]") return true; + // IPv6 unique local (fc00::/7) — matches fd00::, fc00::, etc. + if (/^\[f[cd][0-9a-f]{2}:/i.test(hostname)) return true; + // IPv6 link-local (fe80::/10) + if (/^\[fe[89ab][0-9a-f]:/i.test(hostname)) return true; for (const prefix of PRIVATE_IP_PREFIXES) {🤖 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 181 - 188, The isPrivateIp function currently only handles IPv4 ranges and the literal "[::1]"; extend it to detect IPv6 unique-local (fc00::/7, e.g., fd00::/8) and IPv6 link-local (fe80::/10) addresses by normalizing/removing surrounding brackets from the hostname (handle inputs like "[fd00::1]"), then add tests against new regexes (e.g., PRIVATE_IPV6_ULA_RE and PRIVATE_IPV6_LINKLOCAL_RE) or inline checks after the existing PRIVATE_IP_172_RE check; update or add these constants and return true when either regex matches.src/lib/shields.ts (1)
110-123:⚠️ Potential issue | 🟠 MajorAvoid signaling an unverified PID from disk.
process.kill(marker.pid, "SIGTERM")trusts the PID persisted in the marker file without verification. If the timer process has exited and the OS reused that PID for an unrelated process, this could terminate the wrong process.Consider storing additional identity information (e.g., process start time from
/proc/<pid>/stat) in the marker and verifying it matches before signaling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shields.ts` around lines 110 - 123, The killTimer function currently calls process.kill(marker.pid, "SIGTERM") using an unverified PID from readTimerMarker; to fix, augment the timer marker to include an identity field (e.g., process start time / uptime or start timestamp) when creating the marker and in killTimer verify that the PID on disk matches the live process identity before signaling: read the marker (readTimerMarker), fetch the candidate process metadata (e.g., /proc/<pid>/stat starttime on Linux or equivalent), compare it to the stored identity value and only call process.kill(marker.pid, "SIGTERM") if they match; if they don't match, avoid killing and still remove the stale marker via timerMarkerPath(sandboxName). Ensure failures to read process metadata are handled gracefully (treat as mismatch) and document/update the marker writing code to include the added identity field.src/nemoclaw.ts (1)
2149-2157:⚠️ Potential issue | 🟡 MinorValidate
config getoptions instead of silently defaulting.The flag parsing for
--keyand--formatdoesn't validate:
- Missing values (e.g.,
--keyat end of args)- Invalid format values (only
json/yamlare valid)- Unknown flags
Scripts may silently receive different output than expected without an error.
Possible hardening
case "get": { const configOpts = { key: null, format: "json" }; for (let i = 1; i < actionArgs.length; i++) { - if (actionArgs[i] === "--key") configOpts.key = actionArgs[++i]; - else if (actionArgs[i] === "--format") configOpts.format = actionArgs[++i]; + if (actionArgs[i] === "--key") { + const value = actionArgs[++i]; + if (!value || value.startsWith("--")) { + console.error(" --key requires a value"); + process.exit(1); + } + configOpts.key = value; + } else if (actionArgs[i] === "--format") { + const value = actionArgs[++i]; + if (!value || !["json", "yaml"].includes(value)) { + console.error(" --format must be one of: json, yaml"); + process.exit(1); + } + configOpts.format = value; + } else { + console.error(` Unknown flag: ${actionArgs[i]}`); + process.exit(1); + } } sandboxConfig.configGet(cmd, configOpts); break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 2149 - 2157, The flag parsing in the "get" case (inside the switch handling actionArgs) silently defaults and can consume out-of-bounds args or accept invalid values; update the parsing around actionArgs and configOpts so that before doing configOpts.key = actionArgs[++i] or configOpts.format = actionArgs[++i] you verify that i+1 < actionArgs.length and produce an error (throw or call the existing error-handling/logger) if a value is missing, validate configOpts.format accepts only "json" or "yaml" and error on anything else, and treat any unrecognized flag (anything starting with "--" that is not --key or --format) as an error; only call sandboxConfig.configGet(cmd, configOpts) after these validations succeed.
🧹 Nitpick comments (6)
agents/hermes/policy-permissive.yaml (1)
29-33: Consider removing redundant subdirectory entries under/sandbox.With Line [29] granting write access to
/sandbox, Lines [32]-[33] are functionally redundant. Dropping them would reduce policy drift risk and make intent clearer.♻️ Optional cleanup
read_write: - /sandbox - /tmp - /dev/null - - /sandbox/.hermes-data - - /sandbox/.nemoclaw🤖 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 29 - 33, The policy grants write access to /sandbox as a top-level entry, so the more-specific entries /sandbox/.hermes-data and /sandbox/.nemoclaw are redundant and should be removed to reduce policy drift; update agents/hermes/policy-permissive.yaml by deleting those subdirectory lines, keep the general /sandbox entry, and run your policy linter/validation to ensure no other code expects the explicit subdirectory entries.test/shields.test.ts (2)
50-50: Add type annotation totmpDir.As per coding guidelines, TypeScript code should have proper type annotations. Adding
stringtype would make the intent clearer.Suggested fix
-let tmpDir; +let tmpDir: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/shields.test.ts` at line 50, The variable declaration for tmpDir lacks a TypeScript type annotation; update the declaration of tmpDir (the variable named tmpDir in test/shields.test.ts) to include an explicit type (e.g., string) so its intent is clear and conforms to the project's typing guidelines.
1-1: Consider removing@ts-nocheckand adding proper types.The
//@ts-nocheck`` directive disables all TypeScript checking in this file. While this may be a pragmatic choice for test files with complex mocking, it also means type errors won't be caught. Consider using more targeted type assertions or@ts-expect-errorcomments for specific problem areas instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/shields.test.ts` at line 1, Remove the top-level "// `@ts-nocheck`" from test/shields.test.ts and restore TypeScript checking; then explicitly type the test helpers and mocks (e.g., annotate mocked modules with jest.Mocked<T> or jest.MockedFunction<...>, cast temporary values with "as unknown as Type" where necessary, and add targeted "// `@ts-expect-error`" comments only for the specific lines that genuinely need suppressing). Update any untyped imports or variables used in describe/it blocks to have concrete types or interfaces, and replace broad "any" usage with narrow types so the file compiles cleanly under TypeScript without disabling checking globally.src/nemoclaw.ts (1)
2158-2167: Consider validatingconfig setflags in CLI layer.While
configSet()validates that--keyand--valueare provided, validating at the CLI parsing layer would give more immediate feedback. Currently, passing--keywithout a value would setkeyto undefined (fromactionArgs[++i]), which is caught later but with a generic error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 2158 - 2167, The CLI parsing in the "set" case currently assigns flags without validating presence of values, so incrementing i and reading actionArgs[++i] can yield undefined for --key/--value; add immediate validation after parsing each flag in the case "set" block to ensure that when actionArgs[i] === "--key" or "--value" the next token exists and is not another flag (throw or print a clear CLI error and exit if missing), and only then set setOpts.key/setOpts.value; keep the existing sandboxConfig.configSet(cmd, setOpts) call but ensure setOpts.key and setOpts.value are validated at this CLI layer before invoking sandboxConfig.configSet.src/lib/sandbox-config.ts (2)
15-25: Consider using ESM imports for consistency.The file uses CommonJS
require()syntax while the coding guidelines encourage ESM for TypeScript files. While this works, ESM imports would provide better type inference and align with modern TypeScript practices.🤖 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 15 - 25, Replace the CommonJS require() calls at the top of sandbox-config.ts with ESM-style imports: use "import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; import { execFileSync } from 'child_process'; import { validateName } from './runner'; import { stripCredentials } from './credential-filter'; import { appendAuditEntry } from './shields-audit'; import { runOpenshellCommand, captureOpenshellCommand } from './openshell';" and then adjust any subsequent usage if necessary; ensure your build/tsconfig or package.json module settings support ESM so these imports resolve at runtime.
329-333: Shell command with interpolated path — verify source is trusted.The command
cat > ${target.configPath}interpolates the config path into a shell command. Whiletarget.configPathis derived from the agent manifest (developer-controlled), consider using an array-based command construction to avoid potential issues if the path ever contains special characters.Defensive improvement
execFileSync("docker", [ "exec", "-i", K3S_CONTAINER, "kubectl", "exec", "-n", "openshell", sandboxName, "-c", "agent", "-i", "--", - "sh", "-c", `cat > ${target.configPath}`, + "sh", "-c", `cat > "$1"`, "--", target.configPath, ], { input: content, stdio: ["pipe", "pipe", "pipe"], timeout: 15000 });🤖 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 329 - 333, The current execFileSync call builds a shell string with interpolation (cat > ${target.configPath}) which risks mis-handling special characters from developer-controlled agent manifests; change the invocation to avoid injecting the path into the shell string — e.g., stop using a single interpolated "sh -c" argument and instead pass the path as a separate positional parameter to the shell (or use a non-shell approach like piping into tee) so the path is not interpreted by the shell; update the execFileSync call that uses K3S_CONTAINER, sandboxName and target.configPath to pass the config path as an argument (for example: "sh", "-c", "cat > \"$1\"", "--", target.configPath) or equivalent safe array-based construction.
🤖 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/onboard/config.ts`:
- Around line 49-51: The catch block that returns the raw endpointUrl on parse
failure can leak credentials; update the catch in nemoclaw/src/onboard/config.ts
to return a sanitized version instead of the original string by invoking a new
or existing sanitizer (e.g., redactCredentials or sanitizeEndpoint) that strips
user:pass@ host credentials and sensitive query params (or masks them) before
passing the value to slashConfigShow; ensure the sanitizer is used wherever
endpointUrl is shown (e.g., slashConfigShow) so malformed URLs cannot bypass
redaction.
---
Duplicate comments:
In `@src/lib/sandbox-config.ts`:
- Around line 486-493: The audit entry for the rotate-token flow uses an
incorrect action value "shields_down"; update the appendAuditEntry call used
during rotate-token (the block that builds { action: "shields_down", sandbox:
sandboxName, ... reason: `rotate-token ${target.agentName}:${credentialEnv}` })
to use a dedicated action like "token_rotated" (or another agreed token rotation
constant) so the audit log accurately represents the operation performed by
rotate-token; ensure you change only the action field in that appendAuditEntry
invocation (in the rotate-token handling path).
- Around line 354-361: The audit entry for config mutations currently uses
action: "shields_down" which is semantically wrong; update the call to
appendAuditEntry in src/lib/sandbox-config.ts to use action: "config_set" for
config set operations (the appendAuditEntry invocation that sets action,
sandbox, timestamp, reason), and also add "config_set" to the allowed action
types enum/validation in shields-audit.ts so the new action is accepted by the
audit schema/validator.
- Around line 181-188: The isPrivateIp function currently only handles IPv4
ranges and the literal "[::1]"; extend it to detect IPv6 unique-local (fc00::/7,
e.g., fd00::/8) and IPv6 link-local (fe80::/10) addresses by
normalizing/removing surrounding brackets from the hostname (handle inputs like
"[fd00::1]"), then add tests against new regexes (e.g., PRIVATE_IPV6_ULA_RE and
PRIVATE_IPV6_LINKLOCAL_RE) or inline checks after the existing PRIVATE_IP_172_RE
check; update or add these constants and return true when either regex matches.
In `@src/lib/shields.ts`:
- Around line 110-123: The killTimer function currently calls
process.kill(marker.pid, "SIGTERM") using an unverified PID from
readTimerMarker; to fix, augment the timer marker to include an identity field
(e.g., process start time / uptime or start timestamp) when creating the marker
and in killTimer verify that the PID on disk matches the live process identity
before signaling: read the marker (readTimerMarker), fetch the candidate process
metadata (e.g., /proc/<pid>/stat starttime on Linux or equivalent), compare it
to the stored identity value and only call process.kill(marker.pid, "SIGTERM")
if they match; if they don't match, avoid killing and still remove the stale
marker via timerMarkerPath(sandboxName). Ensure failures to read process
metadata are handled gracefully (treat as mismatch) and document/update the
marker writing code to include the added identity field.
In `@src/nemoclaw.ts`:
- Around line 2149-2157: The flag parsing in the "get" case (inside the switch
handling actionArgs) silently defaults and can consume out-of-bounds args or
accept invalid values; update the parsing around actionArgs and configOpts so
that before doing configOpts.key = actionArgs[++i] or configOpts.format =
actionArgs[++i] you verify that i+1 < actionArgs.length and produce an error
(throw or call the existing error-handling/logger) if a value is missing,
validate configOpts.format accepts only "json" or "yaml" and error on anything
else, and treat any unrecognized flag (anything starting with "--" that is not
--key or --format) as an error; only call sandboxConfig.configGet(cmd,
configOpts) after these validations succeed.
---
Nitpick comments:
In `@agents/hermes/policy-permissive.yaml`:
- Around line 29-33: The policy grants write access to /sandbox as a top-level
entry, so the more-specific entries /sandbox/.hermes-data and /sandbox/.nemoclaw
are redundant and should be removed to reduce policy drift; update
agents/hermes/policy-permissive.yaml by deleting those subdirectory lines, keep
the general /sandbox entry, and run your policy linter/validation to ensure no
other code expects the explicit subdirectory entries.
In `@src/lib/sandbox-config.ts`:
- Around line 15-25: Replace the CommonJS require() calls at the top of
sandbox-config.ts with ESM-style imports: use "import * as fs from 'fs'; import
* as os from 'os'; import * as path from 'path'; import { execFileSync } from
'child_process'; import { validateName } from './runner'; import {
stripCredentials } from './credential-filter'; import { appendAuditEntry } from
'./shields-audit'; import { runOpenshellCommand, captureOpenshellCommand } from
'./openshell';" and then adjust any subsequent usage if necessary; ensure your
build/tsconfig or package.json module settings support ESM so these imports
resolve at runtime.
- Around line 329-333: The current execFileSync call builds a shell string with
interpolation (cat > ${target.configPath}) which risks mis-handling special
characters from developer-controlled agent manifests; change the invocation to
avoid injecting the path into the shell string — e.g., stop using a single
interpolated "sh -c" argument and instead pass the path as a separate positional
parameter to the shell (or use a non-shell approach like piping into tee) so the
path is not interpreted by the shell; update the execFileSync call that uses
K3S_CONTAINER, sandboxName and target.configPath to pass the config path as an
argument (for example: "sh", "-c", "cat > \"$1\"", "--", target.configPath) or
equivalent safe array-based construction.
In `@src/nemoclaw.ts`:
- Around line 2158-2167: The CLI parsing in the "set" case currently assigns
flags without validating presence of values, so incrementing i and reading
actionArgs[++i] can yield undefined for --key/--value; add immediate validation
after parsing each flag in the case "set" block to ensure that when
actionArgs[i] === "--key" or "--value" the next token exists and is not another
flag (throw or print a clear CLI error and exit if missing), and only then set
setOpts.key/setOpts.value; keep the existing sandboxConfig.configSet(cmd,
setOpts) call but ensure setOpts.key and setOpts.value are validated at this CLI
layer before invoking sandboxConfig.configSet.
In `@test/shields.test.ts`:
- Line 50: The variable declaration for tmpDir lacks a TypeScript type
annotation; update the declaration of tmpDir (the variable named tmpDir in
test/shields.test.ts) to include an explicit type (e.g., string) so its intent
is clear and conforms to the project's typing guidelines.
- Line 1: Remove the top-level "// `@ts-nocheck`" from test/shields.test.ts and
restore TypeScript checking; then explicitly type the test helpers and mocks
(e.g., annotate mocked modules with jest.Mocked<T> or jest.MockedFunction<...>,
cast temporary values with "as unknown as Type" where necessary, and add
targeted "// `@ts-expect-error`" comments only for the specific lines that
genuinely need suppressing). Update any untyped imports or variables used in
describe/it blocks to have concrete types or interfaces, and replace broad "any"
usage with narrow types so the file compiles cleanly under TypeScript without
disabling checking globally.
🪄 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: 18be7ebd-c1e1-41f6-af6b-65d06e7c7008
📒 Files selected for processing (11)
agents/hermes/policy-permissive.yamlnemoclaw-blueprint/policies/openclaw-sandbox-permissive.yamlnemoclaw/src/onboard/config.tssrc/lib/sandbox-config.tssrc/lib/shields-audit.tssrc/lib/shields-timer.tssrc/lib/shields.tssrc/nemoclaw.tstest/duration.test.tstest/e2e/test-shields-config.shtest/shields.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/duration.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- nemoclaw-blueprint/policies/openclaw-sandbox-permissive.yaml
- src/lib/shields-audit.ts
- src/lib/shields-timer.ts
- test/e2e/test-shields-config.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
nemoclaw/src/onboard/config.ts (1)
49-51:⚠️ Potential issue | 🔴 CriticalCritical: parse-failure fallback still exposes raw endpoint secrets.
On Line 50, the catch path keeps
safeUrlunchanged, so malformed URLs can bypass masking and leak credentials/tokens in displayed output. Use conservative string-level redaction (or shared sanitizer) in the catch path instead of “show as-is.”Suggested fix
let safeUrl = config.endpointUrl; try { const parsed = new URL(config.endpointUrl); if (parsed.password) parsed.password = "****"; if (parsed.username) parsed.username = "****"; for (const key of [...parsed.searchParams.keys()]) { if (/(token|key|secret|auth|sig|credential|password)/i.test(key)) { parsed.searchParams.set(key, "****"); } } safeUrl = parsed.toString(); } catch { - // Not a valid URL — show as-is + // Not a valid URL — redact conservatively + safeUrl = config.endpointUrl + .replace(/\/\/([^:@\/\s]+):([^@\/\s]+)@/g, "//****:****@") + .replace( + /([?&][^=]*?(token|key|secret|auth|sig|credential|password)[^=]*=)[^&#]*/gi, + "$1****", + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/onboard/config.ts` around lines 49 - 51, The catch block that currently leaves safeUrl unchanged on parse failure must instead apply conservative string-level redaction to avoid leaking credentials; update the catch in onboard/config.ts where safeUrl is set so it calls the shared sanitizer (e.g., sanitizeRawEndpoint) or performs a simple redaction (mask any "://...@" userinfo and long token-like query/header fragments) and assigns that result to safeUrl before using or returning it; ensure you reference the same variable name safeUrl and replace the "show as-is" behavior with the sanitizer call.
🧹 Nitpick comments (3)
test/e2e/test-shields-config.sh (1)
450-491: Auto-restore timer test is valuable but timing-sensitive.The 10s timeout with 15s wait is reasonable, but CI runners under load might occasionally fail this test. Consider adding a retry or increasing the margin.
🤖 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 450 - 491, The auto-restore timer test is flaky under CI due to tight timing (shields down uses --timeout 10s and the script sleeps 15s); update the test to be more robust by increasing the timeout/wait margin or adding a retry loop: adjust the nemoclaw shields down --timeout value and the subsequent sleep (or replace the single sleep+check using STATUS_AFTER_TIMER with a short loop that queries nemoclaw "${SANDBOX_NAME}" shields status up to N times with a small sleep between attempts), and keep the existing cleanup logic; target the lines/variables STATUS_TIMER, STATUS_AFTER_TIMER, the shields down --timeout argument, and the sleep 15 call when making the change.src/lib/shields.ts (1)
197-203: Consider documenting the broad permissions during shields-down.
chmod 666on the config file andchmod 777on the config directory are intentionally permissive to allow sandbox writes. This is noted in comments, but consider adding a warning in the audit entry or status output so operators are aware of the relaxed state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/shields.ts` around lines 197 - 203, When performing the permissive chmods in the shields-down flow (the try block that calls kubectlExec(sandboxName, ["chattr", "-i", target.configPath]) and kubectlExec(sandboxName, ["chmod", "666", target.configPath]) / kubectlExec(sandboxName, ["chmod", "777", target.configDir])), emit an explicit audit/status warning that the config file and directory are now world-writable; add a one-line audit entry or status field labeled something like "shields-down: relaxed permissions" referencing target.configPath and target.configDir so operators see the relaxed 666/777 state in logs or UI.src/lib/sandbox-config.ts (1)
504-512:readStdincan hang indefinitely if stdin never closes.If the caller doesn't close stdin (e.g., piped from a hung process or interactive terminal with no EOF), this Promise never resolves. Consider adding a timeout.
Suggested enhancement
-function readStdin(): Promise<string> { - return new Promise((resolve, reject) => { +function readStdin(timeoutMs = 30000): Promise<string> { + return new Promise((resolve, reject) => { + const timer = setTimeout(() => { + process.stdin.removeAllListeners(); + reject(new Error("stdin read timed out")); + }, timeoutMs); const chunks: Buffer[] = []; process.stdin.on("data", (chunk: Buffer) => chunks.push(chunk)); - process.stdin.on("end", () => resolve(Buffer.concat(chunks).toString("utf-8").trim())); - process.stdin.on("error", reject); + process.stdin.on("end", () => { + clearTimeout(timer); + resolve(Buffer.concat(chunks).toString("utf-8").trim()); + }); + process.stdin.on("error", (err) => { + clearTimeout(timer); + reject(err); + }); process.stdin.resume(); }); }🤖 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 can hang forever if stdin never emits "end"; add a configurable timeout (e.g., default 10s) inside readStdin so the Promise rejects or resolves with a safe value when time expires. Implement by creating a timer (setTimeout) when the promise starts; on timeout clear the stdin listeners (the "data", "end", and "error" handlers attached to process.stdin), clear the timer on normal completion or error, and then reject (or resolve with an empty string) with a clear timeout error message. Ensure you reference the same event handlers/variables (chunks, process.stdin listeners) so cleanup removes exactly those listeners.
🤖 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/shields.ts`:
- Line 24: STATE_DIR currently falls back to "/tmp" which is insecure on
multi-user systems; change the resolution to prefer XDG_STATE_HOME, then HOME,
and only if neither is set create a secure per-process directory using
fs.mkdtemp (or similar) under os.tmpdir(), ensure the directory is created with
restrictive permissions (mode 0o700) and assign STATE_DIR to that path; update
any initialization code that uses the STATE_DIR constant (refer to STATE_DIR in
src/lib/shields.ts) to create the directory if missing and enforce the
permissions before using it.
In `@src/nemoclaw.ts`:
- Around line 2168-2176: The rotate-token handler in the "rotate-token" case
currently allows --from-env to consume undefined if it's the last element of
actionArgs; update the parsing logic around actionArgs, tokenOpts and the loop
in that case to validate that actionArgs[i+1] exists before incrementing i and
assigning tokenOpts.fromEnv, returning or throwing a user-facing error (or
printing usage) when the value is missing; also ensure mutual flag semantics for
--from-stdin/--from-env are respected before calling
sandboxConfig.configRotateToken(cmd, tokenOpts).
- Around line 2158-2167: The "set" case in the switch uses actionArgs parsing
that can consume undefined for "--key" or "--value" when those flags are last
and silently ignores unknown flags; update the parsing in the case "set" block
to validate that a next token exists before doing actionArgs[++i], reject or
error on unknown flags, and return a clear error when --key or --value are
missing/undefined before calling sandboxConfig.configSet (reference the setOpts
object, actionArgs array, and sandboxConfig.configSet call to locate the code).
---
Duplicate comments:
In `@nemoclaw/src/onboard/config.ts`:
- Around line 49-51: The catch block that currently leaves safeUrl unchanged on
parse failure must instead apply conservative string-level redaction to avoid
leaking credentials; update the catch in onboard/config.ts where safeUrl is set
so it calls the shared sanitizer (e.g., sanitizeRawEndpoint) or performs a
simple redaction (mask any "://...@" userinfo and long token-like query/header
fragments) and assigns that result to safeUrl before using or returning it;
ensure you reference the same variable name safeUrl and replace the "show as-is"
behavior with the sanitizer call.
---
Nitpick comments:
In `@src/lib/sandbox-config.ts`:
- Around line 504-512: readStdin can hang forever if stdin never emits "end";
add a configurable timeout (e.g., default 10s) inside readStdin so the Promise
rejects or resolves with a safe value when time expires. Implement by creating a
timer (setTimeout) when the promise starts; on timeout clear the stdin listeners
(the "data", "end", and "error" handlers attached to process.stdin), clear the
timer on normal completion or error, and then reject (or resolve with an empty
string) with a clear timeout error message. Ensure you reference the same event
handlers/variables (chunks, process.stdin listeners) so cleanup removes exactly
those listeners.
In `@src/lib/shields.ts`:
- Around line 197-203: When performing the permissive chmods in the shields-down
flow (the try block that calls kubectlExec(sandboxName, ["chattr", "-i",
target.configPath]) and kubectlExec(sandboxName, ["chmod", "666",
target.configPath]) / kubectlExec(sandboxName, ["chmod", "777",
target.configDir])), emit an explicit audit/status warning that the config file
and directory are now world-writable; add a one-line audit entry or status field
labeled something like "shields-down: relaxed permissions" referencing
target.configPath and target.configDir so operators see the relaxed 666/777
state in logs or UI.
In `@test/e2e/test-shields-config.sh`:
- Around line 450-491: The auto-restore timer test is flaky under CI due to
tight timing (shields down uses --timeout 10s and the script sleeps 15s); update
the test to be more robust by increasing the timeout/wait margin or adding a
retry loop: adjust the nemoclaw shields down --timeout value and the subsequent
sleep (or replace the single sleep+check using STATUS_AFTER_TIMER with a short
loop that queries nemoclaw "${SANDBOX_NAME}" shields status up to N times with a
small sleep between attempts), and keep the existing cleanup logic; target the
lines/variables STATUS_TIMER, STATUS_AFTER_TIMER, the shields down --timeout
argument, and the sleep 15 call when making the change.
🪄 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: 18be7ebd-c1e1-41f6-af6b-65d06e7c7008
📒 Files selected for processing (11)
agents/hermes/policy-permissive.yamlnemoclaw-blueprint/policies/openclaw-sandbox-permissive.yamlnemoclaw/src/onboard/config.tssrc/lib/sandbox-config.tssrc/lib/shields-audit.tssrc/lib/shields-timer.tssrc/lib/shields.tssrc/nemoclaw.tstest/duration.test.tstest/e2e/test-shields-config.shtest/shields.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/duration.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- agents/hermes/policy-permissive.yaml
- nemoclaw-blueprint/policies/openclaw-sandbox-permissive.yaml
- test/shields.test.ts
- src/lib/shields-timer.ts
…le-sandbox-e2e added upstream) Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/nemoclaw.ts (1)
2291-2378: Consider extractingshields/configparsing into dedicated handlers.This nested switch logic further increases dispatcher complexity. Pulling each action family into helper functions would keep this file maintainable and closer to the complexity target.
As per coding guidelines
**/*.{js,ts,tsx,jsx}: Limit cyclomatic complexity to 20 in JavaScript/TypeScript files, with target of 15.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nemoclaw.ts` around lines 2291 - 2378, The nested switch for "shields" and "config" should be moved into two helper functions (e.g. handleShieldsCommand(cmd, actionArgs) and handleConfigCommand(cmd, actionArgs)); move the entire parsing logic that builds opts/configOpts/setOpts/tokenOpts and the calls to shields.shieldsDown/shieldsUp/shieldsStatus and sandboxConfig.configGet/configSet/configRotateToken into those functions, preserving the exact flag validation, error messages, process.exit calls and default usage branches; replace the existing case "shields": and case "config": blocks with single calls to the new handlers to keep behavior identical while reducing cyclomatic complexity in the main dispatcher.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/nemoclaw.ts`:
- Around line 2291-2378: The nested switch for "shields" and "config" should be
moved into two helper functions (e.g. handleShieldsCommand(cmd, actionArgs) and
handleConfigCommand(cmd, actionArgs)); move the entire parsing logic that builds
opts/configOpts/setOpts/tokenOpts and the calls to
shields.shieldsDown/shieldsUp/shieldsStatus and
sandboxConfig.configGet/configSet/configRotateToken into those functions,
preserving the exact flag validation, error messages, process.exit calls and
default usage branches; replace the existing case "shields": and case "config":
blocks with single calls to the new handlers to keep behavior identical while
reducing cyclomatic complexity in the main dispatcher.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fa6e1ed8-1a84-4bb4-bcc3-47f8a1df44de
📒 Files selected for processing (2)
.github/workflows/nightly-e2e.yamlsrc/nemoclaw.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/nightly-e2e.yaml
…rebuild info Bring plugin branch coverage from 84.95% to 86.28% (above 86% ratchet): - config.test.ts: cover URL credential redaction and non-URL fallback - config-show.test.ts: cover empty credentialEnv branch - shields-status.test.ts: cover UP with snapshot path, DOWN with no timeout - slash.test.ts: cover lastRebuildAt and lastRebuildBackupPath display Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…/NVIDIA/NemoClaw into feat/sandbox-management-commands
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw/src/onboard/config.test.ts (1)
75-85: Expand redaction assertions to cover username leakage too.This test validates password/query-token masking, but it doesn’t assert that username/userinfo is removed (Line 78 includes
user:secret@...). Add an explicit negative assertion so credential redaction coverage is complete.Suggested test tweak
const result = describeOnboardEndpoint(config); expect(result).not.toContain("secret"); + expect(result).not.toContain("user:"); + expect(result).not.toContain("user@"); expect(result).not.toContain("abc123"); expect(result).toContain("api.example.com"); expect(result).toContain("****");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/onboard/config.test.ts` around lines 75 - 85, The test for describeOnboardEndpoint currently checks that password and query-token are redacted but misses asserting that the username/userinfo is also removed; update the test that uses makeConfig with endpointUrl "https://user:secret@..." to include an explicit negative assertion that "user" (or the username portion) is not present in result, while keeping existing positive checks for the host and the redaction marker (e.g., "****").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw/src/onboard/config.test.ts`:
- Around line 75-85: The test for describeOnboardEndpoint currently checks that
password and query-token are redacted but misses asserting that the
username/userinfo is also removed; update the test that uses makeConfig with
endpointUrl "https://user:secret@..." to include an explicit negative assertion
that "user" (or the username portion) is not present in result, while keeping
existing positive checks for the host and the redaction marker (e.g., "****").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07fc40c0-0d6a-40a7-9964-0a44c90d68e6
📒 Files selected for processing (4)
nemoclaw/src/commands/config-show.test.tsnemoclaw/src/commands/shields-status.test.tsnemoclaw/src/commands/slash.test.tsnemoclaw/src/onboard/config.test.ts
✅ Files skipped from review due to trivial changes (1)
- nemoclaw/src/commands/shields-status.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- nemoclaw/src/commands/config-show.test.ts
- nemoclaw/src/commands/slash.test.ts
3 tasks
Contributor
Author
|
Superseded by #1976 — clean branch with signed commit and updated description. |
ericksoa
added a commit
that referenced
this pull request
Apr 17, 2026
## 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 restore - `config get/set/rotate-token` — host-initiated config inspection and mutation - E2E test exercising the full lifecycle against a live sandbox ### Shields - Per-sandbox state namespacing (`shields-<name>.json`) - Auto-restore timer via detached `fork()` with explicit IPC disconnect - Rollback on timer fork failure (sandbox never left unguarded) - Timer verifies policy restore succeeded; reverts state on failure - Append-only audit trail with credential redaction ### Config - `config get`: read-only with credential stripping and gateway section removal - `config set`: SSRF validation, gateway write blocking, kubectl exec for Landlock bypass - `config rotate-token`: env var, stdin, or interactive prompt - All `openshell sandbox exec` calls use `--name` flag (not positional) ### Security hardening (CodeRabbit review) - Endpoint URL redaction in plugin `/nemoclaw config` slash command - Audit reason/error fields pass through `redact()` before writing - `shields down` rejects missing flag values and unknown flags - `parseCurrentPolicy()` strips metadata header from policy snapshots ## Test plan - [x] E2E: `test/e2e/test-shields-config.sh` — full 12-phase lifecycle (37/37 pass on CI) - [x] Unit: duration parsing, state management, audit format, config set validation, URL redaction - [x] Plugin branch coverage 86.28% (above 86% ratchet) Supersedes #1849. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added NemoClaw shields: `shields down|up|status` with timeout, reason, policy snapshot, audit logging, auto-restore, and CLI + slash command/status outputs. * Added sandbox config commands: `config get|set|rotate-token` with redacted displays, validation, and optional in-sandbox apply. * Improved endpoint display sanitization and duration parsing/limits. * **Tests** * Expanded unit, integration, and E2E coverage for shields, config, duration parsing, token rotation, and audit logging. * **Chores** * CI failure notification job updated; agent sandbox filesystem policies adjusted. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Draft implementation of Phase 1 MVP from the Sandbox Management
Commands RFC (v0.1, 2026-04-13). Ready to circulate alongside the
RFC for review — merge blocked until the RFC is finalized.
What's implemented
nemoclaw shields down/up/status— time-bounded policyrelaxation with auto-restore timer, policy snapshot capture,
and append-only JSONL audit trail
nemoclaw config get— read-only sandbox config inspectionwith credential redaction and dotpath extraction
/nemoclaw shields— read-only shields status slash command/nemoclaw config— read-only config display slash commandSecurity invariants enforced
New files
src/lib/shields.tssrc/lib/shields-timer.tssrc/lib/shields-audit.tssrc/lib/duration.tssrc/lib/sandbox-config.tssrc/lib/credential-strip.tsnemoclaw/src/commands/shields-status.tsnemoclaw/src/commands/config-show.tsOpen RFC questions (not blocking this draft)
Test plan
tsc --noEmit)tsconfig.cli.json)Summary by CodeRabbit
New Features
Improvements
Bug Fixes