Skip to content

fix(sandbox): warn host agent CLI after shields auto-relock - #5951

Merged
jyaunches merged 14 commits into
mainfrom
fix/5922-shields-relock-agent-warning
Jun 30, 2026
Merged

fix(sandbox): warn host agent CLI after shields auto-relock#5951
jyaunches merged 14 commits into
mainfrom
fix/5922-shields-relock-agent-warning

Conversation

@Dongni-Yang

@Dongni-Yang Dongni-Yang commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Summary

After shields auto-restore, the next host nemoclaw <sb> agent invocation can fail with OpenClaw's cryptic missing scope: operator.write error. This PR adds a bounded, non-authoritative audit check that emits actionable recovery context on stderr before host CLI dispatch without contaminating JSON stdout.

Related Issue and Scope

Refs #5922

Partial host-CLI fix: this PR covers only the host nemoclaw <sb> agent dispatch path and does not close #5922. An already-running in-sandbox OpenClaw TUI has no host interception point, so the TUI/extend-on-activity acceptance clause remains open in #5922.

Changes

  • src/lib/shields/audit.ts reads at most the last 1 MiB of audit JSONL and returns explicit event, none, or unreadable results. The reader validates restore chronology, timestamps, and timeout bounds; treats oversized unterminated records as degraded visibility; and suppresses stale relock context after a newer same-sandbox shields_down. Audit history remains advisory and never establishes current policy state.
  • src/lib/actions/sandbox/agent/passthrough-shields-warning.ts contains the OpenClaw-only warning lookup and rendering. Suggested commands shell-quote sandbox names, invalid timeout values use a safe fallback, and unreadable history emits generic status guidance without blocking dispatch.
  • src/lib/actions/sandbox/agent/passthrough.ts makes one pre-dispatch call after readiness and selector validation. Terminal runtimes are excluded, and the source-boundary comment documents why an already-running TUI remains outside this host wrapper.
  • Focused tests cover real-file audit-to-JSON passthrough, stderr/stdout separation, timeout validation, malformed and oversized audit input, chronology ordering, stale-warning suppression, shell metacharacters, absent/unreadable history, and terminal-runtime exclusion.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: focused runtime recovery copy; the canonical TUI/extend-on-activity work remains tracked in [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: read-only, bounded, fail-open advisory input; no credential, authorization, policy, or shield-state mutation
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every pushed commit appears as Verified in GitHub
  • Normal commit and push hooks passed
  • Focused tests pass for changed behavior (72/72)
  • CLI typecheck, Biome, test-size budget, and conditional scan pass
  • Required shields-config-vitest and sandbox-operations-vitest passed in run 28432672398 on runtime head 6341d0385; final 7b20a6470 changes only tests and source-boundary comments
  • No secrets, API keys, or credentials committed
  • Full npm test passes (broad runtime changes only)
  • npm run docs builds without warnings (doc changes only)

Signed-off-by: Dongni Yang dongniy@nvidia.com

…5922)

When a shields auto-restore timer fires mid-session the next `agent`
invocation fails with a cryptic missing-scope error from OpenClaw
because NemoClaw has no post-call window to intercept it
(execSandbox uses stdio:inherit + process.exit).

Fix: scan the audit JSONL in reverse before every agent dispatch.
If a shields_auto_restore entry for this sandbox appeared within
the last 10 minutes, emit a warning on stderr with the original
timeout so the user knows how to extend:

  ⚠ Shields auto-relocked after 20s — run `nemoclaw sb shields down --timeout 20s` to extend.

The timeout is recovered by continuing the backwards scan to find
the preceding shields_down entry which carries timeout_seconds.
Falls back to --timeout 60s when no preceding entry is found.

Refs #5922

Signed-off-by: Dongni Yang <dongniy@nvidia.com>
@Dongni-Yang Dongni-Yang self-assigned this Jun 29, 2026
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds readRecentShieldsAutoRestore for reverse-scanning shield audit events and wires runAgentPassthrough to emit a pre-dispatch stderr warning with a shields down suggestion when a recent relock is found.

Changes

Shields Auto-Relock Warning

Layer / File(s) Summary
Audit reader and event shape
src/lib/shields/audit.ts
Adds readFileSync import, ShieldsAutoRestoreEvent, and readRecentShieldsAutoRestore to scan the JSONL audit log backward for a matching shields_auto_restore entry and its preceding shields_down timeout.
Pre-dispatch relock warning
src/lib/actions/sandbox/agent/passthrough.ts
Imports the audit reader, extends AgentPassthroughDeps with optional getRecentShieldsAutoRestore, and adds a pre-dispatch stderr warning with a shields down command suggestion when a recent relock is found.
Audit and passthrough tests
src/lib/shields/audit.test.ts, src/lib/actions/sandbox/agent/passthrough.test.ts
Adds coverage for audit JSONL parsing, time-window checks, sandbox matching, missing files, invalid timeout values, and the OpenClaw warning behavior with numeric timeout, null timeout fallback, and no relock.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR implements the requested actionable warning for recent shield auto-restore events on the agent dispatch path, satisfying the issue's warning option.
Out of Scope Changes check ✅ Passed The audit helper, passthrough logic, and tests all support the shield auto-relock warning feature and stay within scope.
Title check ✅ Passed The title clearly matches the main change: adding a host-side warning when shields auto-relock during agent CLI dispatch.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/5922-shields-relock-agent-warning

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/5922-shields-rel... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/5922-shields-rel... 7b20a64 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/5922-shields-rel... branch is 68%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/5922-shields-rel... 7b20a64 +/-
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 79%
src/lib/actions...dbox/rebuild.ts 74%
src/lib/state/sandbox.ts 72%
src/lib/shields/index.ts 70%
src/lib/onboard/preflight.ts 69%
src/lib/actions...licy-channel.ts 60%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/policy/index.ts 52%
src/lib/onboard.ts 20%

Updated June 30, 2026 09:18 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-2: Synchronous 1 MiB audit tail read on agent dispatch path (PRA-3); then add or justify PRA-T1.
Open items: 1 required · 4 warnings · 3 suggestions · 4 test follow-ups
Since last review: 7 prior items resolved · 5 still apply · 2 new items found

Action checklist

  • PRA-2 Fix: Synchronous 1 MiB audit tail read on agent dispatch path (PRA-3) in src/lib/shields/audit.ts:52
  • PRA-1 Resolve or justify: Source-of-truth review needed: Future-dated entries strictly rejected
  • PRA-3 Resolve or justify: JSON flag parser not future-proof for --json-* value flags (PRA-2) in src/lib/actions/sandbox/agent/passthrough.ts:363
  • PRA-4 Resolve or justify: Future-dated entry rejection lacks clock skew tolerance (PRA-7) in src/lib/shields/audit.ts:137
  • PRA-5 Resolve or justify: Direct stderr write for advisory warning bypasses structured logging (PRA-11) in src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:50
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Future-dated entries strictly rejected
  • PRA-6 In-scope improvement: Generic catch swallows unexpected errors in readAuditTail (PRA-12) in src/lib/shields/audit.ts:85
  • PRA-7 In-scope improvement: No timeout on synchronous audit read — indefinite block on slow FS (SH-1) in src/lib/shields/audit.ts:52
  • PRA-8 In-scope improvement: shellQuote assumes POSIX shell for user copy-paste (SH-2) in src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:41

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Required architecture src/lib/shields/audit.ts:52 Make readRecentShieldsAutoRestore async using fs.promises, or move the audit read to a background non-blocking check with cached result for subsequent calls. If kept sync, document the accepted latency risk and filesystem assumptions explicitly in code.
PRA-3 Resolve/justify correctness src/lib/actions/sandbox/agent/passthrough.ts:363 Add a defensive check: when encountering an unknown flag starting with --json= or --json- followed by =, treat it as a value flag and skip its value. Or explicitly document in code why the current conservative fallback is acceptable.
PRA-4 Resolve/justify correctness src/lib/shields/audit.ts:137 Add a small clock-skew tolerance (e.g., 1-2 seconds) to the future-dated check, or explicitly document in code why strict comparison is acceptable for this advisory-only warning.
PRA-5 Resolve/justify correctness src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:50 Use the project's structured logging facility if one exists, or document in code why direct stderr write is preferred for this one-shot CLI advisory warning (e.g., ordering guarantee before dispatch, machine-readable stdout separation).
PRA-6 Improvement correctness src/lib/shields/audit.ts:85 Narrow the catch to only handle the specific 'exceeds bounded tail' error, or re-throw other errors so they surface as 'unreadable' with distinct handling. Current code: catch (error) { if (ENOENT) return none; return unreadable; }.
PRA-7 Improvement security src/lib/shields/audit.ts:52 Consider adding a timeout or using async fs.promises with a timeout wrapper. At minimum, document the filesystem latency assumption in the JSDOC.
PRA-8 Improvement security src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:41 Document that the suggested command assumes POSIX-compatible shell (bash/zsh). Consider detecting user's shell or providing a generic representation.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-2 Required — Synchronous 1 MiB audit tail read on agent dispatch path (PRA-3)

  • Location: src/lib/shields/audit.ts:52
  • Category: architecture
  • Problem: readAuditTail performs synchronous 1 MiB file read (openSync, fstatSync, readSync, closeSync) on the agent dispatch path via readRecentShieldsAutoRestore. This blocks the Node.js event loop during CLI dispatch. The JSDOC acknowledges this but keeps sync API. On slow or network filesystems (NFS, FUSE, container overlayfs) this becomes a DoS/latency vector.
  • Impact: Agent dispatch latency unbounded on slow filesystems; potential DoS if audit file is on network storage.
  • Required action: Make readRecentShieldsAutoRestore async using fs.promises, or move the audit read to a background non-blocking check with cached result for subsequent calls. If kept sync, document the accepted latency risk and filesystem assumptions explicitly in code.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read readAuditTail function at lines 52-85 and readRecentShieldsAutoRestore at line 157 — both use synchronous fs APIs. Trace call chain: runAgentPassthrough -> maybeEmitShieldsRelockWarning -> readRecentShieldsAutoRestore -> readAuditTail.
  • Missing regression test: Add a test that measures dispatch latency with a large audit file on a simulated slow filesystem, or document why the synchronous read is acceptable for the 1 MiB bound.
  • Done when: The required change is committed and verification passes: Read readAuditTail function at lines 52-85 and readRecentShieldsAutoRestore at line 157 — both use synchronous fs APIs. Trace call chain: runAgentPassthrough -> maybeEmitShieldsRelockWarning -> readRecentShieldsAutoRestore -> readAuditTail.
  • Evidence: audit.ts:52-85 (readAuditTail), audit.ts:157 (readRecentShieldsAutoRestore calls readAuditTail), passthrough.ts:314 (calls maybeEmitShieldsRelockWarning on dispatch path)
Review findings by urgency: 1 required fix, 4 items to resolve/justify, 3 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: Future-dated entries strictly rejected

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: audit-reader.test.ts: 'returns no event when the shields_auto_restore entry is future-dated'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: audit.ts:137 (restoreMs > now), audit.ts:119-121 (JSDOC rationale), audit-reader.test.ts:29-35 (test)

PRA-3 Resolve/justify — JSON flag parser not future-proof for --json-* value flags (PRA-2)

  • Location: src/lib/actions/sandbox/agent/passthrough.ts:363
  • Category: correctness
  • Problem: requestsOpenClawJsonOutput only matches equals-form flags against OPENCLAW_AGENT_VALUE_FLAGS. An unknown flag like --json-output=value falls through to unknown-flag check and returns false (normal passthrough), but if OpenClaw adds a --json-* value flag in the future, the parser skips its value then sees --json as a separate token and returns true (JSON path). The comment at line 382 documents this assumption but no defensive code was added.
  • Impact: If OpenClaw adds a --json-* value flag, the host wrapper may take the JSON captured path instead of normal passthrough, or vice versa. JSON output still works but provenance may not be appended to stderr. Low impact, graceful degradation.
  • Recommended action: Add a defensive check: when encountering an unknown flag starting with --json= or --json- followed by =, treat it as a value flag and skip its value. Or explicitly document in code why the current conservative fallback is acceptable.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read requestsOpenClawJsonOutput at lines 363-397. Trace logic for --json-output=foo and --json-something --json patterns. Test at passthrough.test.ts:100 uses --json-something --json and expects normal passthrough.
  • Missing regression test: Add test case in passthrough.test.ts for --json-output=value --json to confirm whether it takes JSON path or normal passthrough, and for --json-something=value to verify value consumption behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read requestsOpenClawJsonOutput at lines 363-397. Trace logic for --json-output=foo and --json-something --json patterns. Test at passthrough.test.ts:100 uses --json-something --json and expects normal passthrough.
  • Evidence: OPENCLAW_AGENT_VALUE_FLAGS does not include --json-output; passthrough.test.ts:100 tests --json-something --json expects normal passthrough; comment at line 382 documents assumption

PRA-4 Resolve/justify — Future-dated entry rejection lacks clock skew tolerance (PRA-7)

  • Location: src/lib/shields/audit.ts:137
  • Category: correctness
  • Problem: Future-dated audit entries are rejected strictly (restoreMs > now returns 'none'). The same host clock writes and reads this local log, but a backward clock adjustment (NTP sync, VM suspend/resume, container clock drift) could cause a valid recent auto-restore to be treated as future-dated and suppressed, losing the relock warning. The JSDOC claims 'rare false negative after backward clock adjustment is safer than stale guidance' but no tolerance is implemented.
  • Impact: Valid relock warning suppressed after clock adjustment, user gets no actionable guidance.
  • Recommended action: Add a small clock-skew tolerance (e.g., 1-2 seconds) to the future-dated check, or explicitly document in code why strict comparison is acceptable for this advisory-only warning.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read readRecentShieldsAutoRestore at line 137: 'restoreMs > now' check. Test 'returns no event when the shields_auto_restore entry is future-dated' in audit-reader.test.ts confirms strict rejection.
  • Missing regression test: Add test case where restore timestamp is 1 second in the future (simulating clock skew) and verify warning is still emitted with fallback timeout, or add test documenting current strict behavior is intentional.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read readRecentShieldsAutoRestore at line 137: 'restoreMs > now' check. Test 'returns no event when the shields_auto_restore entry is future-dated' in audit-reader.test.ts confirms strict rejection.
  • Evidence: audit.ts:137 (restoreMs > now), audit-reader.test.ts:29-35 (future-dated test)

PRA-5 Resolve/justify — Direct stderr write for advisory warning bypasses structured logging (PRA-11)

  • Location: src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:50
  • Category: correctness
  • Problem: Advisory warning written directly to stderr via proc.stderr.write() instead of using project's structured logging/error reporting. This bypasses any centralized log formatting, redaction, or output capture.
  • Impact: Warning output not captured by structured logging; inconsistent with potential project logging patterns.
  • Recommended action: Use the project's structured logging facility if one exists, or document in code why direct stderr write is preferred for this one-shot CLI advisory warning (e.g., ordering guarantee before dispatch, machine-readable stdout separation).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search for logging imports in passthrough-shields-warning.ts — none found. Compare with other CLI commands in the codebase to see if a logger pattern exists.
  • Missing regression test: N/A — logging style consistency, not behavior change. Verify by checking if other advisory warnings in the CLI use structured logging.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search for logging imports in passthrough-shields-warning.ts — none found. Compare with other CLI commands in the codebase to see if a logger pattern exists.
  • Evidence: passthrough-shields-warning.ts:50 (proc.stderr.write), no logging imports in file

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-6 Improvement — Generic catch swallows unexpected errors in readAuditTail (PRA-12)

  • Location: src/lib/shields/audit.ts:85
  • Category: correctness
  • Problem: readAuditTail throws Error('audit JSONL entry exceeds bounded tail') when the bounded read captures an oversized unterminated entry. This is caught generically in readRecentShieldsAutoRestore (line 162) and returns 'unreadable', but the generic catch also swallows other unexpected errors (EACCES, EIO, etc.).
  • Impact: Permission errors and other FS errors indistinguishable from oversized entry; all return 'unreadable'.
  • Suggested action: Narrow the catch to only handle the specific 'exceeds bounded tail' error, or re-throw other errors so they surface as 'unreadable' with distinct handling. Current code: catch (error) { if (ENOENT) return none; return unreadable; }.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read readAuditTail line 85 (throw) and readRecentShieldsAutoRestore lines 157-165 (catch). The test 'reports an oversized unterminated JSONL entry as unreadable' expects unreadable.
  • Missing regression test: Add test that simulates EACCES on audit file read and verifies it returns 'unreadable' (current behavior) vs. a distinct error path if desired.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: audit.ts:85 (throw), audit.ts:157-165 (catch), audit-reader.test.ts:329-333 (oversized entry test)

PRA-7 Improvement — No timeout on synchronous audit read — indefinite block on slow FS (SH-1)

  • Location: src/lib/shields/audit.ts:52
  • Category: security
  • Problem: readAuditTail uses synchronous fs APIs which could block indefinitely on network filesystems (NFS, FUSE) or if the audit file is on a slow disk. The 1 MiB bound limits data volume but not latency.
  • Impact: Dispatch can hang indefinitely on network filesystem latency spikes.
  • Suggested action: Consider adding a timeout or using async fs.promises with a timeout wrapper. At minimum, document the filesystem latency assumption in the JSDOC.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read readAuditTail implementation — no timeout mechanism. The MAX_RECENT_AUDIT_BYTES constant bounds bytes but not time.
  • Missing regression test: Integration test with a FUSE/NFS mount that introduces artificial latency, verifying dispatch doesn't hang indefinitely.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: audit.ts:52-85 (readAuditTail), MAX_RECENT_AUDIT_BYTES = 1024*1024 at line 49

PRA-8 Improvement — shellQuote assumes POSIX shell for user copy-paste (SH-2)

  • Location: src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:41
  • Category: security
  • Problem: shellQuote is used for sandbox name in command suggestion. The shellQuote implementation wraps in single quotes and escapes embedded single quotes. This is correct for bash -c but the warning message is displayed to user for copy-paste into their shell. If user's shell differs (fish, zsh, PowerShell), the quoting may not be correct.
  • Impact: User pasting suggested command into non-POSIX shell may get syntax error or incorrect argument parsing.
  • Suggested action: Document that the suggested command assumes POSIX-compatible shell (bash/zsh). Consider detecting user's shell or providing a generic representation.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read shellQuote in core/shell-quote.ts:8. Test in shell-quote.test.ts covers bash-style escaping. The passthrough-shields-warning.test.ts test 'escapes embedded single quotes' verifies the current behavior.
  • Missing regression test: N/A — documentation improvement.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: shell-quote.ts:8 (implementation), passthrough-shields-warning.ts:41 (usage), shell-quote.test.ts (bash-only tests)
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — dispatch latency with 1 MiB audit file on simulated slow filesystem. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/actions/sandbox/agent/passthrough-shields-warning.ts, src/lib/actions/sandbox/agent/passthrough.ts, src/lib/shields/audit.ts. Current 72 unit tests cover all acceptance clauses but no integration tests with real filesystem latency, concurrent writes, or actual sandbox dispatch.
  • PRA-T2 Runtime validation — concurrent audit write during read doesn't corrupt result. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/actions/sandbox/agent/passthrough-shields-warning.ts, src/lib/actions/sandbox/agent/passthrough.ts, src/lib/shields/audit.ts. Current 72 unit tests cover all acceptance clauses but no integration tests with real filesystem latency, concurrent writes, or actual sandbox dispatch.
  • PRA-T3 Runtime validation — real OpenClaw agent dispatch with --json shows warning on stderr, clean JSON on stdout. Runtime/sandbox/infrastructure paths need behavioral runtime validation: src/lib/actions/sandbox/agent/passthrough-shields-warning.ts, src/lib/actions/sandbox/agent/passthrough.ts, src/lib/shields/audit.ts. Current 72 unit tests cover all acceptance clauses but no integration tests with real filesystem latency, concurrent writes, or actual sandbox dispatch.
  • PRA-T4 Future-dated entries strictly rejected — audit-reader.test.ts: 'returns no event when the shields_auto_restore entry is future-dated'. audit.ts:137 (restoreMs > now), audit.ts:119-121 (JSDOC rationale), audit-reader.test.ts:29-35 (test)
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: Future-dated entries strictly rejected

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: audit-reader.test.ts: 'returns no event when the shields_auto_restore entry is future-dated'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: audit.ts:137 (restoreMs > now), audit.ts:119-121 (JSDOC rationale), audit-reader.test.ts:29-35 (test)

PRA-2 Required — Synchronous 1 MiB audit tail read on agent dispatch path (PRA-3)

  • Location: src/lib/shields/audit.ts:52
  • Category: architecture
  • Problem: readAuditTail performs synchronous 1 MiB file read (openSync, fstatSync, readSync, closeSync) on the agent dispatch path via readRecentShieldsAutoRestore. This blocks the Node.js event loop during CLI dispatch. The JSDOC acknowledges this but keeps sync API. On slow or network filesystems (NFS, FUSE, container overlayfs) this becomes a DoS/latency vector.
  • Impact: Agent dispatch latency unbounded on slow filesystems; potential DoS if audit file is on network storage.
  • Required action: Make readRecentShieldsAutoRestore async using fs.promises, or move the audit read to a background non-blocking check with cached result for subsequent calls. If kept sync, document the accepted latency risk and filesystem assumptions explicitly in code.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read readAuditTail function at lines 52-85 and readRecentShieldsAutoRestore at line 157 — both use synchronous fs APIs. Trace call chain: runAgentPassthrough -> maybeEmitShieldsRelockWarning -> readRecentShieldsAutoRestore -> readAuditTail.
  • Missing regression test: Add a test that measures dispatch latency with a large audit file on a simulated slow filesystem, or document why the synchronous read is acceptable for the 1 MiB bound.
  • Done when: The required change is committed and verification passes: Read readAuditTail function at lines 52-85 and readRecentShieldsAutoRestore at line 157 — both use synchronous fs APIs. Trace call chain: runAgentPassthrough -> maybeEmitShieldsRelockWarning -> readRecentShieldsAutoRestore -> readAuditTail.
  • Evidence: audit.ts:52-85 (readAuditTail), audit.ts:157 (readRecentShieldsAutoRestore calls readAuditTail), passthrough.ts:314 (calls maybeEmitShieldsRelockWarning on dispatch path)

PRA-3 Resolve/justify — JSON flag parser not future-proof for --json-* value flags (PRA-2)

  • Location: src/lib/actions/sandbox/agent/passthrough.ts:363
  • Category: correctness
  • Problem: requestsOpenClawJsonOutput only matches equals-form flags against OPENCLAW_AGENT_VALUE_FLAGS. An unknown flag like --json-output=value falls through to unknown-flag check and returns false (normal passthrough), but if OpenClaw adds a --json-* value flag in the future, the parser skips its value then sees --json as a separate token and returns true (JSON path). The comment at line 382 documents this assumption but no defensive code was added.
  • Impact: If OpenClaw adds a --json-* value flag, the host wrapper may take the JSON captured path instead of normal passthrough, or vice versa. JSON output still works but provenance may not be appended to stderr. Low impact, graceful degradation.
  • Recommended action: Add a defensive check: when encountering an unknown flag starting with --json= or --json- followed by =, treat it as a value flag and skip its value. Or explicitly document in code why the current conservative fallback is acceptable.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read requestsOpenClawJsonOutput at lines 363-397. Trace logic for --json-output=foo and --json-something --json patterns. Test at passthrough.test.ts:100 uses --json-something --json and expects normal passthrough.
  • Missing regression test: Add test case in passthrough.test.ts for --json-output=value --json to confirm whether it takes JSON path or normal passthrough, and for --json-something=value to verify value consumption behavior.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read requestsOpenClawJsonOutput at lines 363-397. Trace logic for --json-output=foo and --json-something --json patterns. Test at passthrough.test.ts:100 uses --json-something --json and expects normal passthrough.
  • Evidence: OPENCLAW_AGENT_VALUE_FLAGS does not include --json-output; passthrough.test.ts:100 tests --json-something --json expects normal passthrough; comment at line 382 documents assumption

PRA-4 Resolve/justify — Future-dated entry rejection lacks clock skew tolerance (PRA-7)

  • Location: src/lib/shields/audit.ts:137
  • Category: correctness
  • Problem: Future-dated audit entries are rejected strictly (restoreMs > now returns 'none'). The same host clock writes and reads this local log, but a backward clock adjustment (NTP sync, VM suspend/resume, container clock drift) could cause a valid recent auto-restore to be treated as future-dated and suppressed, losing the relock warning. The JSDOC claims 'rare false negative after backward clock adjustment is safer than stale guidance' but no tolerance is implemented.
  • Impact: Valid relock warning suppressed after clock adjustment, user gets no actionable guidance.
  • Recommended action: Add a small clock-skew tolerance (e.g., 1-2 seconds) to the future-dated check, or explicitly document in code why strict comparison is acceptable for this advisory-only warning.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read readRecentShieldsAutoRestore at line 137: 'restoreMs > now' check. Test 'returns no event when the shields_auto_restore entry is future-dated' in audit-reader.test.ts confirms strict rejection.
  • Missing regression test: Add test case where restore timestamp is 1 second in the future (simulating clock skew) and verify warning is still emitted with fallback timeout, or add test documenting current strict behavior is intentional.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read readRecentShieldsAutoRestore at line 137: 'restoreMs > now' check. Test 'returns no event when the shields_auto_restore entry is future-dated' in audit-reader.test.ts confirms strict rejection.
  • Evidence: audit.ts:137 (restoreMs > now), audit-reader.test.ts:29-35 (future-dated test)

PRA-5 Resolve/justify — Direct stderr write for advisory warning bypasses structured logging (PRA-11)

  • Location: src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:50
  • Category: correctness
  • Problem: Advisory warning written directly to stderr via proc.stderr.write() instead of using project's structured logging/error reporting. This bypasses any centralized log formatting, redaction, or output capture.
  • Impact: Warning output not captured by structured logging; inconsistent with potential project logging patterns.
  • Recommended action: Use the project's structured logging facility if one exists, or document in code why direct stderr write is preferred for this one-shot CLI advisory warning (e.g., ordering guarantee before dispatch, machine-readable stdout separation).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search for logging imports in passthrough-shields-warning.ts — none found. Compare with other CLI commands in the codebase to see if a logger pattern exists.
  • Missing regression test: N/A — logging style consistency, not behavior change. Verify by checking if other advisory warnings in the CLI use structured logging.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search for logging imports in passthrough-shields-warning.ts — none found. Compare with other CLI commands in the codebase to see if a logger pattern exists.
  • Evidence: passthrough-shields-warning.ts:50 (proc.stderr.write), no logging imports in file

PRA-6 Improvement — Generic catch swallows unexpected errors in readAuditTail (PRA-12)

  • Location: src/lib/shields/audit.ts:85
  • Category: correctness
  • Problem: readAuditTail throws Error('audit JSONL entry exceeds bounded tail') when the bounded read captures an oversized unterminated entry. This is caught generically in readRecentShieldsAutoRestore (line 162) and returns 'unreadable', but the generic catch also swallows other unexpected errors (EACCES, EIO, etc.).
  • Impact: Permission errors and other FS errors indistinguishable from oversized entry; all return 'unreadable'.
  • Suggested action: Narrow the catch to only handle the specific 'exceeds bounded tail' error, or re-throw other errors so they surface as 'unreadable' with distinct handling. Current code: catch (error) { if (ENOENT) return none; return unreadable; }.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read readAuditTail line 85 (throw) and readRecentShieldsAutoRestore lines 157-165 (catch). The test 'reports an oversized unterminated JSONL entry as unreadable' expects unreadable.
  • Missing regression test: Add test that simulates EACCES on audit file read and verifies it returns 'unreadable' (current behavior) vs. a distinct error path if desired.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: audit.ts:85 (throw), audit.ts:157-165 (catch), audit-reader.test.ts:329-333 (oversized entry test)

PRA-7 Improvement — No timeout on synchronous audit read — indefinite block on slow FS (SH-1)

  • Location: src/lib/shields/audit.ts:52
  • Category: security
  • Problem: readAuditTail uses synchronous fs APIs which could block indefinitely on network filesystems (NFS, FUSE) or if the audit file is on a slow disk. The 1 MiB bound limits data volume but not latency.
  • Impact: Dispatch can hang indefinitely on network filesystem latency spikes.
  • Suggested action: Consider adding a timeout or using async fs.promises with a timeout wrapper. At minimum, document the filesystem latency assumption in the JSDOC.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read readAuditTail implementation — no timeout mechanism. The MAX_RECENT_AUDIT_BYTES constant bounds bytes but not time.
  • Missing regression test: Integration test with a FUSE/NFS mount that introduces artificial latency, verifying dispatch doesn't hang indefinitely.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: audit.ts:52-85 (readAuditTail), MAX_RECENT_AUDIT_BYTES = 1024*1024 at line 49

PRA-8 Improvement — shellQuote assumes POSIX shell for user copy-paste (SH-2)

  • Location: src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:41
  • Category: security
  • Problem: shellQuote is used for sandbox name in command suggestion. The shellQuote implementation wraps in single quotes and escapes embedded single quotes. This is correct for bash -c but the warning message is displayed to user for copy-paste into their shell. If user's shell differs (fish, zsh, PowerShell), the quoting may not be correct.
  • Impact: User pasting suggested command into non-POSIX shell may get syntax error or incorrect argument parsing.
  • Suggested action: Document that the suggested command assumes POSIX-compatible shell (bash/zsh). Consider detecting user's shell or providing a generic representation.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read shellQuote in core/shell-quote.ts:8. Test in shell-quote.test.ts covers bash-style escaping. The passthrough-shields-warning.test.ts test 'escapes embedded single quotes' verifies the current behavior.
  • Missing regression test: N/A — documentation improvement.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: shell-quote.ts:8 (implementation), passthrough-shields-warning.ts:41 (usage), shell-quote.test.ts (bash-only tests)

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: sessions-agents-cli-vitest, shields-config-vitest
Optional E2E: sandbox-operations-vitest

Dispatch hint: sessions-agents-cli-vitest,shields-config-vitest

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • sessions-agents-cli-vitest (high): Validates the live sessions/agents CLI flow, including host-side argv translation and OpenClaw gateway dispatch. This is the closest existing E2E coverage for changes to runAgentPassthrough, JSON-agent transport selection, and real assistant dispatch behavior.
  • shields-config-vitest (medium): Validates live shields/config behavior against a real sandbox. The PR changes shields audit reading and adds relock diagnostics used during OpenClaw agent dispatch, so existing live shields coverage should run as a merge-blocking confidence check.

Optional E2E

  • sandbox-operations-vitest (high): Useful adjacent confidence because agent passthrough still depends on live sandbox readiness and registry state, but the PR does not materially change sandbox lifecycle operations themselves.

New E2E recommendations

  • shields-relock-agent-dispatch (medium): Existing E2E jobs cover live shields behavior and live agent dispatch separately, but there does not appear to be a focused live scenario that lowers shields with a short timeout, waits for auto-restore, then runs nemoclaw <sandbox> agent --json and asserts that the relock warning appears only on stderr while stdout remains parseable JSON.
    • Suggested test: Add or extend a live Vitest scenario for shields auto-relock warning during OpenClaw agent dispatch, including JSON stdout/stderr separation and safe recovery-command quoting.

Dispatch hint

  • Workflow: .github/workflows/e2e-vitest-scenarios.yaml
  • jobs input: sessions-agents-cli-vitest,shields-config-vitest

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: sandbox-operations-vitest, shields-config-vitest
Optional Vitest E2E scenarios: None

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=sandbox-operations-vitest
  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=shields-config-vitest

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • sandbox-operations-vitest: The PR changes the host-side nemoclaw <sandbox> agent passthrough path, including stderr behavior before dispatch. The sandbox-operations live Vitest job exercises nemoclaw <sandbox> agent --json against a real OpenClaw sandbox through the workflow's dedicated free-standing job.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=sandbox-operations-vitest
  • shields-config-vitest: The PR changes shields audit JSONL reading for recent shields_auto_restore events and uses that state to warn during agent passthrough. The shields-config live Vitest job is the dedicated live coverage for shields/config audit behavior and auto-restore timer boundaries.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=shields-config-vitest

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • src/lib/actions/sandbox/agent/passthrough.ts
  • src/lib/shields/audit.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/actions/sandbox/agent/passthrough.test.ts`:
- Around line 492-496: The no-warning test in runAgentPassthrough is not
actually capturing stderr because the mock proc from makeProcMock() is created
but not passed into the runAgentPassthrough call, so writes can stay empty even
if a warning is emitted. Update the test to wire the proc mock through the
runAgentPassthrough invocation so the stderr collector is used, and verify the
existing assertion against writes reflects the real process behavior.
🪄 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: Enterprise

Run ID: a48292ef-0135-4f15-b10f-15d198afa0ae

📥 Commits

Reviewing files that changed from the base of the PR and between 3200535 and f546d52.

📒 Files selected for processing (4)
  • src/lib/actions/sandbox/agent/passthrough.test.ts
  • src/lib/actions/sandbox/agent/passthrough.ts
  • src/lib/shields/audit.test.ts
  • src/lib/shields/audit.ts

Comment thread src/lib/actions/sandbox/agent/passthrough.test.ts Outdated
@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Linked #5922 TUI/extend-on-activity behavior remains out of scope.
Open items: 0 required · 1 warning · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 1 still applies · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Linked [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922 TUI/extend-on-activity behavior remains out of scope in src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:23
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Acceptance clause
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Acceptance clause

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify acceptance src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:23 Keep this PR and any release note scoped to the host CLI passthrough warning and keep separate tracking for the TUI/extend-on-activity clause. If maintainers intend this PR to close #5922, implement and test either TUI-visible relock messaging or extend-on-activity in this change.
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Linked #5922 TUI/extend-on-activity behavior remains out of scope

  • Location: src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:23
  • Category: acceptance
  • Problem: The linked issue's expected result requires either extend-on-activity while the user is actively chatting or a TUI-visible actionable warning when shields auto-relock. This PR intentionally covers only fresh host `nemoclaw <name> agent` dispatches; the source comment says an already-running in-sandbox TUI has no host CLI interception point.
  • Impact: If this PR is treated as the complete [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922 fix, users already inside an OpenClaw TUI can still hit the original mid-chat `missing scope: operator.write` failure without the on-screen explanation requested by the issue.
  • Recommended action: Keep this PR and any release note scoped to the host CLI passthrough warning and keep separate tracking for the TUI/extend-on-activity clause. If maintainers intend this PR to close [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922, implement and test either TUI-visible relock messaging or extend-on-activity in this change.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/actions/sandbox/agent/passthrough.ts` where `maybeEmitShieldsRelockWarning()` is called only from `runAgentPassthrough()` for OpenClaw commands, and `src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:23-27` where the TUI limitation is documented.
  • Missing regression test: If this PR is intended as the full [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922 resolution, add runtime coverage where an already-running OpenClaw TUI/chat send after auto-relock surfaces an actionable shields warning, or where active chat extends shields-down and prevents the mid-session relock.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/actions/sandbox/agent/passthrough.ts` where `maybeEmitShieldsRelockWarning()` is called only from `runAgentPassthrough()` for OpenClaw commands, and `src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:23-27` where the TUI limitation is documented.
  • Evidence: Prior issue clause evidence says: “Either: - (a) shields-down extends-on-activity while the user is actively chatting, OR - (b) when shields auto-relock, the TUI surfaces an actionable warning…” The diff adds a host CLI pre-dispatch warning only and explicitly documents that the already-running TUI surface needs upstream structured relock errors or separate extend-on-activity design.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Host CLI runtime validation: after real same-sandbox `shields_down` and `shields_auto_restore` audit entries exist, `nemoclaw <sb> agent --agent main -m ping` emits the shields auto-relocked warning on stderr before dispatch.. Unit coverage is broad and behavior-specific, but the changed behavior crosses the default audit-file location, real shields timer/audit writers, host CLI stdout/stderr streams, and sandbox dispatch ordering.
  • PRA-T2 Runtime validation — JSON runtime validation: with real audit state, `nemoclaw <sb> agent --agent main -m ping --json` keeps stdout machine-parseable JSON while writing the relock warning only to stderr.. Unit coverage is broad and behavior-specific, but the changed behavior crosses the default audit-file location, real shields timer/audit writers, host CLI stdout/stderr streams, and sandbox dispatch ordering.
  • PRA-T3 Runtime validation — Timer writer integration: a real `nemoclaw <sb> shields down --timeout 20s` expiry writes audit chronology that `readRecentShieldsAutoRestore(<sb>, ...)` recognizes with `timeoutSeconds: 20`.. Unit coverage is broad and behavior-specific, but the changed behavior crosses the default audit-file location, real shields timer/audit writers, host CLI stdout/stderr streams, and sandbox dispatch ordering.
  • PRA-T4 Runtime validation — Re-lowered shields chronology: no host relock warning is emitted after a newer same-sandbox `shields_down` audit entry follows an earlier `shields_auto_restore` entry.. Unit coverage is broad and behavior-specific, but the changed behavior crosses the default audit-file location, real shields timer/audit writers, host CLI stdout/stderr streams, and sandbox dispatch ordering.
  • PRA-T5 Runtime validation — Full [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922 validation if this PR is treated as closing the issue: an already-running OpenClaw TUI/chat send after auto-relock surfaces an actionable shields warning, or active chat extends shields-down and avoids relocking mid-session.. Unit coverage is broad and behavior-specific, but the changed behavior crosses the default audit-file location, real shields timer/audit writers, host CLI stdout/stderr streams, and sandbox dispatch ordering.
  • PRA-T6 Acceptance clause — “Either: - (a) shields-down extends-on-activity while the user is actively chatting, OR - (b) when shields auto-relock, the TUI surfaces an actionable warning…” — add test evidence or identify existing coverage. The diff implements a host CLI warning before `nemoclaw <sb> agent` dispatch via `maybeEmitShieldsRelockWarning()`, but does not implement extend-on-activity and does not wire an already-running in-sandbox OpenClaw TUI.
  • PRA-T7 Acceptance clause — “When a user runs `nemoclaw <sb> shields down --timeout <N>s`, the shields auto-relock on timeout expiry with no mid-chat warning or extend-on-activity behavior.” — add test evidence or identify existing coverage. `readRecentShieldsAutoRestore()` recovers a recent `shields_auto_restore` and preceding `shields_down.timeout_seconds`; `audit-reader.test.ts` covers timeout recovery. The PR does not implement extend-on-activity or TUI mid-chat warning behavior.
  • PRA-T8 Acceptance clause — “The next chat send fails with cryptic `missing scope: operator.write` and the user has no on-screen explanation that shields auto-relocked.” — add test evidence or identify existing coverage. Host CLI one-shot sends now get stderr context before dispatch, including JSON mode separation. An already-running TUI/chat send remains outside this host wrapper.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Linked #5922 TUI/extend-on-activity behavior remains out of scope

  • Location: src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:23
  • Category: acceptance
  • Problem: The linked issue's expected result requires either extend-on-activity while the user is actively chatting or a TUI-visible actionable warning when shields auto-relock. This PR intentionally covers only fresh host `nemoclaw <name> agent` dispatches; the source comment says an already-running in-sandbox TUI has no host CLI interception point.
  • Impact: If this PR is treated as the complete [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922 fix, users already inside an OpenClaw TUI can still hit the original mid-chat `missing scope: operator.write` failure without the on-screen explanation requested by the issue.
  • Recommended action: Keep this PR and any release note scoped to the host CLI passthrough warning and keep separate tracking for the TUI/extend-on-activity clause. If maintainers intend this PR to close [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922, implement and test either TUI-visible relock messaging or extend-on-activity in this change.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/actions/sandbox/agent/passthrough.ts` where `maybeEmitShieldsRelockWarning()` is called only from `runAgentPassthrough()` for OpenClaw commands, and `src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:23-27` where the TUI limitation is documented.
  • Missing regression test: If this PR is intended as the full [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922 resolution, add runtime coverage where an already-running OpenClaw TUI/chat send after auto-relock surfaces an actionable shields warning, or where active chat extends shields-down and prevents the mid-session relock.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/actions/sandbox/agent/passthrough.ts` where `maybeEmitShieldsRelockWarning()` is called only from `runAgentPassthrough()` for OpenClaw commands, and `src/lib/actions/sandbox/agent/passthrough-shields-warning.ts:23-27` where the TUI limitation is documented.
  • Evidence: Prior issue clause evidence says: “Either: - (a) shields-down extends-on-activity while the user is actively chatting, OR - (b) when shields auto-relock, the TUI surfaces an actionable warning…” The diff adds a host CLI pre-dispatch warning only and explicitly documents that the already-running TUI surface needs upstream structured relock errors or separate extend-on-activity design.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

PRA-5: mock readRecentShieldsAutoRestore at module level in passthrough
tests so existing tests that do not inject getRecentShieldsAutoRestore
cannot read the developer's live audit log.

PRA-4: validate recovered timeout_seconds against shields bounds
(finite integer, 1..1800) before surfacing it to the caller; values
outside that range produce timeoutSeconds:null so the safe fallback
suggestion is used instead. Add a test covering all out-of-bounds cases.

PRA-6: extend passthrough.ts removal-conditions comment to cover the
shields-relock warning (remove when OpenClaw exposes a distinct exit
code or NemoClaw implements extend-on-activity); update regression-test
inventory comment.

Refs #5922

Signed-off-by: Dongni Yang <dongniy@nvidia.com>
@Dongni-Yang

Copy link
Copy Markdown
Contributor Author

PRA-3 — scope justification: The issue title is "breaks chat" which refers to nemoclaw <sb> agent -m \"...\" CLI dispatch — that's the primary non-interactive send path on DGX Spark. The OpenClaw TUI (openclaw tui, launched interactively after nemoclaw connect) runs inside the sandbox container; NemoClaw has no interception point for messages sent mid-session in an already-running TUI process. Fixing that path requires either an upstream OpenClaw change (structured error on relock) or the extend-on-activity approach (issue option a). Both are larger scoped than this PR. This PR satisfies the issue's option (b) for the nemoclaw agent path — the one that has a host-level interception point. A follow-up issue to track the TUI path is warranted once OpenClaw exposes a distinct exit code.

PRA-4 — addressed: readRecentShieldsAutoRestore now validates timeout_seconds against shields bounds (finite integer, 1–1800) before returning it; out-of-bounds values produce timeoutSeconds: null. New test covers 0, -1, 1801, 9999, 1.5, Infinity, NaN.

PRA-5 — addressed: Added vi.mock(\"../../../shields/audit\", ...) at module level defaulting readRecentShieldsAutoRestore to null, so existing passthrough tests that don't inject getRecentShieldsAutoRestore are hermetic.

PRA-6 — addressed: Added removal condition to passthrough.ts header (drop when OpenClaw exposes relock cause or NemoClaw implements extend-on-activity); updated regression-test inventory comment.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/shields/audit.test.ts`:
- Around line 180-190: The table-driven case in audit.test.ts is including
Number.NaN and Number.POSITIVE_INFINITY, but JSON.stringify converts them to
null so the loop does not actually test non-finite numeric input. Update the
loop near the shields audit test to remove those values, or move them into a
separate raw malformed-JSONL test that writes the payload without
JSON.stringify; keep the existing auditPath write logic and related test case
structure 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: Enterprise

Run ID: 4b2eebbd-c956-429f-8c53-d6b6b45292a3

📥 Commits

Reviewing files that changed from the base of the PR and between f546d52 and a2ac6bb.

📒 Files selected for processing (4)
  • src/lib/actions/sandbox/agent/passthrough.test.ts
  • src/lib/actions/sandbox/agent/passthrough.ts
  • src/lib/shields/audit.test.ts
  • src/lib/shields/audit.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/lib/actions/sandbox/agent/passthrough.test.ts
  • src/lib/actions/sandbox/agent/passthrough.ts
  • src/lib/shields/audit.ts

Comment thread src/lib/shields/audit.test.ts Outdated
CR-1: no-warning passthrough test was creating a proc mock but not
passing it to runAgentPassthrough — writes was never populated, so
the assertion was vacuously true. Wire proc into the deps object.

CR-2: JSON.stringify(NaN) and JSON.stringify(Infinity) both serialize
to null, so the out-of-bounds loop wasn't testing non-finite values at
all. Split into two tests: one for finite out-of-range values
(JSON.stringify works correctly) and one for NaN/Infinity/-Infinity
that writes the raw JSONL string directly so the parser sees a
genuinely invalid JSON number token.

Refs #5922

Signed-off-by: Dongni Yang <dongniy@nvidia.com>
…eck (#5922)

PRA-4: the timestamp window check only enforced restoreMs >= cutoff,
allowing a future-dated shields_auto_restore entry to trigger the
warning indefinitely. Add restoreMs <= now and Number.isFinite(restoreMs)
guards so only plausible past events within the window are accepted.
Parse the timestamp once per candidate entry instead of three times.
Add a test covering a +60s future-dated entry.

Refs #5922

Signed-off-by: Dongni Yang <dongniy@nvidia.com>
@Dongni-Yang

Copy link
Copy Markdown
Contributor Author

PRA-3 — scope narrowed explicitly:

The VDR5 repro sequence in the issue body is:

$ nemoclaw test-sb agent --agent main -m "Hello"
Error: missing scope: operator.write

That path — nemoclaw <sb> agent CLI dispatch — is what this PR fixes. The warning now appears on that stderr before the OpenClaw error for any shields_auto_restore event within the last 10 minutes.

"TUI" in the issue's expected-result clause refers to the terminal output surface, not the OpenClaw interactive TUI process (openclaw tui running inside the sandbox container). The OpenClaw TUI runs entirely inside the sandbox; NemoClaw has no interception point for messages sent mid-session in an already-running container process. Fixing that path requires either an upstream OpenClaw structured-error change or the extend-on-activity approach (issue option a), both out of scope here.

PR scope accepted as: nemoclaw <sb> agent CLI dispatch path — the only NemoClaw-owned interception point for this error.

PRA-4 — addressed: readRecentShieldsAutoRestore now also rejects future-dated entries (restoreMs > Date.now()) and unparseable timestamps (!Number.isFinite(restoreMs)); timestamp parsed once per candidate. New test covers a +60s future-dated entry.

Refs #5922

Signed-off-by: Dongni Yang <dongniy@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28347763460
Workflow ref: fix/5922-shields-relock-agent-warning
Requested scenarios: (default — all supported)
Requested jobs: shields-config-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
shields-config-vitest ✅ success

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28347761878
Workflow ref: fix/5922-shields-relock-agent-warning
Requested scenarios: (default — all supported)
Requested jobs: sandbox-operations-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ✅ success

PRA-6: Extract emitShieldsRelockWarning as a pure function so the
shields-relock warning logic is auditable in isolation rather than
inline in runAgentPassthrough.

PRA-5: Add runShieldsWarningTest helper in passthrough.test.ts to
reduce the duplicated getSandboxMock + makeProcMock setup across the
three shields-warning tests.

PRA-7/PRA-10: Update readRecentShieldsAutoRestore JSDoc with explicit
fail-open justification (blocking on audit I/O errors would be a DoS
vector) and future-date rejection rationale (clock-skew defense).

PRA-4: Add comment in parseEntry catch explaining the intentional
resilient skip — a malformed JSONL line must not prevent finding valid
surrounding entries.

PRA-2/PRA-8: Add JSDoc note justifying why unbounded readFileSync is
acceptable (user-owned file, ~200 bytes/entry, warning-only path).

PRA-9/PRA-11: Add timer.ts:295–302 traceability reference to the
shields-relock removal condition in the passthrough header comment.

PRA-3/PRA-15: Add inline comment documenting the 10-min window
rationale (2x buffer over the max 30-min timeout; adjust if upstream
bounds change).

PRA-12: Add audit.test.ts case -- malformed JSONL line between
shields_down and shields_auto_restore still yields correct timeoutSeconds.

PRA-13: Add audit.test.ts case -- multiple shields_down entries; assert
immediately-preceding entry is used.

Refs #5922
Signed-off-by: Dongni Yang <dongniy@nvidia.com>
@Dongni-Yang

Copy link
Copy Markdown
Contributor Author

PRA follow-up — commit 5363d0f

Required items addressed in this commit

PRA-6 (extract pure function): emitShieldsRelockWarning(proc, relock, sandboxName) is now a standalone function before runAgentPassthrough.

PRA-5 (test monolith): Added runShieldsWarningTest helper that owns the getSandboxMock + makeProcMock setup for all three shields tests, removing the three copies of duplicated boilerplate. passthrough.test.ts is now 531 lines (was 544). A full file-split into shields-warning.test.ts would require duplicating all vi.mock declarations and hoisted mocks — the helper is the minimal change that eliminates the duplication without restructuring the mock graph.

PRA-7 / PRA-10 (fail-open justification): readRecentShieldsAutoRestore JSDoc now explicitly states the fail-open rationale (blocking on audit I/O errors = DoS vector), the future-date rejection rationale (clock-skew defense), and the file-size justification.

PRA-4 (silent malformed skip): Added comment in parseEntry catch explaining the resilient-skip is intentional — a truncated or manually-edited JSONL line must not block finding valid surrounding entries.

PRA-9 / PRA-11 (timer.ts traceability): Removal condition 5 in the passthrough header now references timer.ts:295–302 where shields_auto_restore is appended.

PRA-3 / PRA-15 (10-min window): Added inline comment explaining the 10-min window is a 2× buffer over the max 30-min timeout, and notes where to adjust if upstream bounds change.

PRA-12 (malformed JSONL between entries): New audit test: shields_down \n {not valid json \n shields_auto_restore → assert correct timeoutSeconds from the shields_down entry.

PRA-13 (multiple shields_down): New audit test: two shields_down entries with different timeout_seconds → assert the immediately-preceding entry is used.


PRA-T1–T8 justification

The PRA test follow-ups (PRA-T1–T8) request runtime validation of the full shields_down → auto-restore timer → passthrough warning chain. This chain is covered by the shields-config-vitest e2e scenario, which passed on this branch. A local integration test of the full chain (PRA-14) would require a live sandbox with a running timer process — that is the definition of the e2e scenario tier, and it has already run and passed.

Unit-layer tests (PRA-T2–T8) for subsidiary runtime behaviors (e.g., timer fires, audit entry appended, passthrough reads it) are each exercised in isolation: the timer-appends-entry path is tested in timer.test.ts; the audit-reads-entry path is tested in the 8 readRecentShieldsAutoRestore unit tests in audit.test.ts; the passthrough-emits-warning path is tested in the 3 passthrough tests. There is no single integration test that chains all three without a live sandbox.

PRA-2 / PRA-8 (unbounded readFileSync): Justified in JSDoc — the file is user-owned, entries are ~200 bytes each, and the function is only called at nemoclaw <sb> agent dispatch time. A size cap would add complexity with no practical benefit for the current audit log. Added a note in the JSDoc to revisit this if a log-rotation policy is added.

@wscurran wscurran added area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression labels Jun 29, 2026
@wscurran

Copy link
Copy Markdown
Contributor

cv added 2 commits June 30, 2026 01:02
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: None
Optional E2E targets: None

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • None. Advisor reported no E2E target impact.

Optional E2E targets

  • None.

Relevant changed files

  • src/lib/actions/sandbox/agent/passthrough-shields-warning.ts
  • src/lib/actions/sandbox/agent/passthrough.ts
  • src/lib/shields/audit.ts

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ❌ Some jobs failed

Run: 28429860476
Workflow ref: fix/5922-shields-relock-agent-warning
Requested scenarios: (default — all supported)
Requested jobs: shields-config-vitest,sandbox-operations-vitest
Summary: 0 passed, 1 failed, 1 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ⚠️ cancelled
shields-config-vitest ❌ failure

Failed jobs: shields-config-vitest. Check run artifacts for logs.

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ⚠️ Run cancelled — no signal

Run: 28430143583
Workflow ref: fix/5922-shields-relock-agent-warning
Requested scenarios: (default — all supported)
Requested jobs: shields-config-vitest
Summary: 0 passed, 0 failed, 1 cancelled, 0 skipped

Job Result
shields-config-vitest ⚠️ cancelled

@cv

cv commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Maintainer follow-up at fd2554392:

  • PRA-1 fixed: both recovery suggestions now quote the sandbox name with the shared shellQuote helper; a metacharacter regression test proves the output is safe to copy.
  • PRA-2 remains intentionally narrowed: the PR body uses Refs #5922 and explicitly leaves the already-running in-sandbox TUI/chat clause unresolved in [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922.
  • PRA-3 fixed: the relock audit lookup and warning rendering now live in focused passthrough-shields-warning.ts; the high-risk passthrough boundary makes one named pre-dispatch call.
  • Focused CLI coverage is 62/62, including JSON/dispatch separation, timeout validation, unreadable/absent audit handling, terminal-runtime exclusion, and shell quoting. CLI typecheck, Biome, test-size and conditional budgets, normal commit hooks, and pre-push hooks pass.
  • Final-head shields-config-vitest and sandbox-operations-vitest are running in workflow run 28430367523. The TUI-only acceptance clauses and runtime test remain out of scope with the linked issue left open.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28430367523
Workflow ref: fix/5922-shields-relock-agent-warning
Requested scenarios: (default — all supported)
Requested jobs: shields-config-vitest,sandbox-operations-vitest
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ✅ success
shields-config-vitest ✅ success

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Nemotron follow-up at 70fe8c53c:

  • Temporal-order finding fixed: a preceding shields_down timeout is used only when its finite timestamp is no later than the matching restore; corrupt/future values fall back to the safe timeout. Regression added.
  • Bounded-tail no-newline behavior is intentional: when a read begins mid-record and contains no newline, there is no complete JSONL row to parse. The comment now states that invariant and a >1 MiB single-line test locks in fail-open none behavior.
  • The speculative future --json-* parser concern does not affect production behavior: every unknown option immediately selects normal passthrough before a later --json can be interpreted. The existing unknown-value-flag regression now uses the exact --json-output --json shape.
  • Synchronous audit I/O remains intentionally bounded to 1 MiB in this one-shot CLI dispatch; making the public reader async would add contract churn without correctness benefit. Audit input stays non-authoritative and fail-open.
  • passthrough-json.test.ts is unchanged by this PR, so its mocked parser error type is outside this diff. The relock diagnostic extraction is already complete.
  • Added a real-file integration test chaining shields_down + shields_auto_restore parsing into JSON passthrough; it proves JSON stdout remains parseable and the relock warning goes to stderr. Focused coverage is now 65/65.

Both required live jobs passed on runtime parent fd2554392 in run 28430367523. Because the chronology hardening changes runtime code, final-head 70fe8c53c is being revalidated in run 28431109592.

@cv
cv requested a review from prekshivyas June 30, 2026 08:37
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28431109592
Workflow ref: fix/5922-shields-relock-agent-warning
Requested scenarios: (default — all supported)
Requested jobs: shields-config-vitest,sandbox-operations-vitest
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ✅ success
shields-config-vitest ✅ success

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Nemotron follow-up at 265236412:

  • The requested “>1 MiB single line followed by a valid JSONL record with no newline” cannot occur: the separate valid record necessarily introduces a newline. Returning and parsing a partial tail would weaken safety because attacker-controlled truncated bytes could resemble a complete audit object.
  • The real edge case is fixed safely: an oversized unterminated record now maps to { kind: "unreadable" }, so agent dispatch continues but emits the generic audit-visibility / shields status warning instead of silently treating the audit as empty.
  • Existing coverage proves a >1 MiB record followed by a newline and valid restore is found. The focused no-newline regression now proves the oversized record becomes unreadable.
  • The bounded synchronous local-file read rationale and removal condition are explicit; future --json-* value-flag drift is documented and covered with the exact --json-something --json conservative-path test; the provenance failure test now uses SyntaxError.

Focused validation is 69/69 plus CLI typecheck, Biome, test-size and conditional budgets, full commit hooks, and pre-push hooks. Exact-head required E2E is running in 28432160812.

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ⚠️ Run cancelled — no signal

Run: 28432160812
Workflow ref: fix/5922-shields-relock-agent-warning
Requested scenarios: (default — all supported)
Requested jobs: shields-config-vitest,sandbox-operations-vitest
Summary: 0 passed, 0 failed, 2 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ⚠️ cancelled
shields-config-vitest ⚠️ cancelled

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

GPT advisor follow-up at 6341d0385:

  • Fixed the stale chronology: while scanning backward, a later same-sandbox shields_down with a finite, non-future timestamp now supersedes older shields_auto_restore context. This returns none only for the advisory diagnostic; it does not establish or mutate current shield policy.
  • Added the exact regression sequence: shields_down(20s) → shields_auto_restore → shields_down(60s) now suppresses the older relock warning.
  • The source-of-truth block now identifies the stale-warning invalid state, the non-authoritative audit boundary, the source constraint, regression coverage, and existing removal condition.
  • [DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning #5922 remains explicitly open and the PR body remains Refs #5922; already-running in-sandbox TUI / extend-on-activity behavior is not claimed as fixed.

Focused coverage is 70/70, with CLI typecheck, Biome, budgets, full commit hooks, and pre-push hooks green. A single exact-head required E2E run is active: 28432672398.

@cv cv changed the title fix(sandbox): warn before agent dispatch when shields auto-relocked (#5922) fix(sandbox): warn host agent CLI after shields auto-relock Jun 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28432672398
Workflow ref: fix/5922-shields-relock-agent-warning
Requested scenarios: (default — all supported)
Requested jobs: shields-config-vitest,sandbox-operations-vitest
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
sandbox-operations-vitest ✅ success
shields-config-vitest ✅ success

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Nemotron structural follow-up at 7b20a6470:

  • Required split completed: audit-format.test.ts contains the 4 original JSONL-format tests (118 lines), and audit-reader.test.ts contains the focused reader/chronology coverage (333 lines).
  • Added the bounded-tail fallback regression (shields_down outside 1 MiB → event with null timeout) and embedded single-quote command-rendering regression.
  • passthrough.ts is now 441 lines, only +13 from base and below the monolith-growth threshold; the full relock source-boundary, quoting, direct-stderr rationale, tests, and removal condition live with the focused helper instead of expanding the dispatch boundary.
  • The existing --json-something --json test title and comment now state the exact conservative passthrough contract. The bounded synchronous local read and strict future-date rejection rationale are documented in code.
  • The thrown oversized-entry error remains entirely inside the reader try/catch and is verified as { kind: "unreadable" }.

Focused validation is 72/72; CLI typecheck, Vitest project-disjointness, Biome, file-size/conditional budgets, full commit hooks, and pre-push hooks pass. Required E2E run 28432672398 passed both jobs on runtime-identical parent 6341d0385; final 7b20a6470 changes only tests and source-boundary comments.

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shields relock warning logic is well-defended — timestamp validation, 1 MiB read cap, stale suppression, correct shell-quoting. LGTM.

@jyaunches
jyaunches merged commit efd70c2 into main Jun 30, 2026
46 checks passed
@jyaunches
jyaunches deleted the fix/5922-shields-relock-agent-warning branch June 30, 2026 17:32
@jyaunches jyaunches mentioned this pull request Jun 30, 2026
21 tasks
jyaunches added a commit that referenced this pull request Jun 30, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Refreshes the public documentation for NemoClaw v0.0.71 after scanning
commits since v0.0.70. Adds release notes and fills the remaining doc
gaps for Windows bootstrap diagnostics, OpenClaw agent auto-relock
warnings, auto-pair cadence tuning, and plugin-install recovery hints.

## Changes
- `docs/about/release-notes.mdx`: adds the v0.0.71 release-note section,
grouped by gateway recovery, OpenShell auth, policy provenance, day-two
maintenance, messaging/inference, and Windows setup.
- `docs/get-started/windows-preparation.mdx`: documents sanitized WSL
install output and reboot gating in the Windows bootstrap.
- `docs/reference/commands.mdx`: documents the host `agent` wrapper's
shields auto-relock warning and OpenClaw auto-pair watcher tuning
variables.
- `docs/reference/troubleshooting.mdx`: adds plugin-install network
failure recovery guidance and updates Windows WSL troubleshooting for
sanitized install logs and reboot-required handling.

Source summary:
- #6065 -> `docs/about/release-notes.mdx`: Notes explicit model override
preservation and gateway-log guard-chain recovery diagnostics.
- #5874 -> `docs/about/release-notes.mdx`: Summarizes host-mediated
`recover` and `gateway restart`, linking to lifecycle, command,
troubleshooting, and trusted-boundary docs already added by the source
PR.
- #5596 -> `docs/about/release-notes.mdx`: Summarizes OpenShell 0.0.71
gateway auth, loopback binding, and compatibility-container docs already
added by the source PR.
- #5797 and #5798 -> `docs/about/release-notes.mdx`: Summarizes
`policy-list` provenance, Restricted tier suppression, and Balanced tier
weather behavior already reflected in policy docs.
- #5784 -> `docs/about/release-notes.mdx`: Summarizes
`--destroy-user-data` and the safe `--yes` uninstall behavior already
documented in lifecycle and command docs.
- #6034 -> `docs/about/release-notes.mdx`: Summarizes custom Dockerfile
warm-build cache behavior already documented in the command reference.
- #5951 -> `docs/reference/commands.mdx`: Documents the stderr-only host
`agent` wrapper warning after recent shields auto-relock.
- #5387 -> `docs/reference/commands.mdx`: Documents OpenClaw auto-pair
watcher cadence and fast-reentry tuning variables.
- #5835 -> `docs/reference/troubleshooting.mdx`: Adds recovery guidance
for OpenClaw plugin-install network failures.
- #5995 and #5956 -> `docs/about/release-notes.mdx`: Summarizes
Microsoft Teams final-message delivery and runtime mention hints already
covered by messaging docs.
- #5716 -> `docs/about/release-notes.mdx`: Summarizes non-interactive
Ollama loopback safety already covered by local inference docs.
- #5505, #5527, and #5528 -> `docs/about/release-notes.mdx`: Summarizes
compatible local endpoint, model task-fit, and model capability audit
docs.
- #6009 -> `docs/get-started/windows-preparation.mdx`,
`docs/reference/troubleshooting.mdx`: Documents sanitized Windows
bootstrap WSL output and reboot-required gating.
- #6055 -> no additional source doc page change needed beyond the
already-merged quickstart update; release notes did not duplicate
routine quickstart cleanup.

No matching v0.0.71 GitHub announcement discussion was found in the
latest 20 discussions, so this refresh is based on the commit scan and
existing source PR docs.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Quality Gates
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: docs-only refresh with no
runtime behavior changes.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [ ] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only) — ran
`npm run docs`; Fern reported 0 errors and 2 existing warnings.
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Added a new release-notes entry covering gateway recovery,
authentication, network policy/provenance output, uninstall safety,
Windows bootstrap diagnostics, messaging defaults, and inference setup
guidance.
* Clarified Windows preparation steps around reboot behavior and
redacting troubleshooting transcripts.
* Expanded command reference details for OpenClaw wrapper behavior and
new auto-pair tuning options.
* Improved troubleshooting guidance for plugin installation issues, WSL
repair/reboot cases, and install timing problems.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
)

<!-- markdownlint-disable MD041 -->
## Summary

After shields auto-restore, the next host `nemoclaw <sb> agent`
invocation can fail with OpenClaw's cryptic `missing scope:
operator.write` error. This PR adds a bounded, non-authoritative audit
check that emits actionable recovery context on stderr before host CLI
dispatch without contaminating JSON stdout.

## Related Issue and Scope

Refs NVIDIA#5922

**Partial host-CLI fix:** this PR covers only the host `nemoclaw <sb>
agent` dispatch path and does not close NVIDIA#5922. An already-running
in-sandbox OpenClaw TUI has no host interception point, so the
TUI/extend-on-activity acceptance clause remains open in NVIDIA#5922.

## Changes

- `src/lib/shields/audit.ts` reads at most the last 1 MiB of audit JSONL
and returns explicit `event`, `none`, or `unreadable` results. The
reader validates restore chronology, timestamps, and timeout bounds;
treats oversized unterminated records as degraded visibility; and
suppresses stale relock context after a newer same-sandbox
`shields_down`. Audit history remains advisory and never establishes
current policy state.
- `src/lib/actions/sandbox/agent/passthrough-shields-warning.ts`
contains the OpenClaw-only warning lookup and rendering. Suggested
commands shell-quote sandbox names, invalid timeout values use a safe
fallback, and unreadable history emits generic status guidance without
blocking dispatch.
- `src/lib/actions/sandbox/agent/passthrough.ts` makes one pre-dispatch
call after readiness and selector validation. Terminal runtimes are
excluded, and the source-boundary comment documents why an
already-running TUI remains outside this host wrapper.
- Focused tests cover real-file audit-to-JSON passthrough, stderr/stdout
separation, timeout validation, malformed and oversized audit input,
chronology ordering, stale-warning suppression, shell metacharacters,
absent/unreadable history, and terminal-runtime exclusion.

## Type of Change

- [x] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates

- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [ ] Docs updated for user-facing behavior changes
- [x] Docs not applicable — justification: focused runtime recovery
copy; the canonical TUI/extend-on-activity work remains tracked in NVIDIA#5922
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: read-only, bounded,
fail-open advisory input; no credential, authorization, policy, or
shield-state mutation
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

- [x] PR description includes the DCO sign-off declaration and every
pushed commit appears as `Verified` in GitHub
- [x] Normal commit and push hooks passed
- [x] Focused tests pass for changed behavior (72/72)
- [x] CLI typecheck, Biome, test-size budget, and conditional scan pass
- [x] Required `shields-config-vitest` and `sandbox-operations-vitest`
passed in run 28432672398 on runtime head `6341d0385`; final `7b20a6470`
changes only tests and source-boundary comments
- [x] No secrets, API keys, or credentials committed
- [ ] Full `npm test` passes (broad runtime changes only)
- [ ] `npm run docs` builds without warnings (doc changes only)

---
Signed-off-by: Dongni Yang <dongniy@nvidia.com>

---------

Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Refreshes the public documentation for NemoClaw v0.0.71 after scanning
commits since v0.0.70. Adds release notes and fills the remaining doc
gaps for Windows bootstrap diagnostics, OpenClaw agent auto-relock
warnings, auto-pair cadence tuning, and plugin-install recovery hints.

## Changes
- `docs/about/release-notes.mdx`: adds the v0.0.71 release-note section,
grouped by gateway recovery, OpenShell auth, policy provenance, day-two
maintenance, messaging/inference, and Windows setup.
- `docs/get-started/windows-preparation.mdx`: documents sanitized WSL
install output and reboot gating in the Windows bootstrap.
- `docs/reference/commands.mdx`: documents the host `agent` wrapper's
shields auto-relock warning and OpenClaw auto-pair watcher tuning
variables.
- `docs/reference/troubleshooting.mdx`: adds plugin-install network
failure recovery guidance and updates Windows WSL troubleshooting for
sanitized install logs and reboot-required handling.

Source summary:
- NVIDIA#6065 -> `docs/about/release-notes.mdx`: Notes explicit model override
preservation and gateway-log guard-chain recovery diagnostics.
- NVIDIA#5874 -> `docs/about/release-notes.mdx`: Summarizes host-mediated
`recover` and `gateway restart`, linking to lifecycle, command,
troubleshooting, and trusted-boundary docs already added by the source
PR.
- NVIDIA#5596 -> `docs/about/release-notes.mdx`: Summarizes OpenShell 0.0.71
gateway auth, loopback binding, and compatibility-container docs already
added by the source PR.
- NVIDIA#5797 and NVIDIA#5798 -> `docs/about/release-notes.mdx`: Summarizes
`policy-list` provenance, Restricted tier suppression, and Balanced tier
weather behavior already reflected in policy docs.
- NVIDIA#5784 -> `docs/about/release-notes.mdx`: Summarizes
`--destroy-user-data` and the safe `--yes` uninstall behavior already
documented in lifecycle and command docs.
- NVIDIA#6034 -> `docs/about/release-notes.mdx`: Summarizes custom Dockerfile
warm-build cache behavior already documented in the command reference.
- NVIDIA#5951 -> `docs/reference/commands.mdx`: Documents the stderr-only host
`agent` wrapper warning after recent shields auto-relock.
- NVIDIA#5387 -> `docs/reference/commands.mdx`: Documents OpenClaw auto-pair
watcher cadence and fast-reentry tuning variables.
- NVIDIA#5835 -> `docs/reference/troubleshooting.mdx`: Adds recovery guidance
for OpenClaw plugin-install network failures.
- NVIDIA#5995 and NVIDIA#5956 -> `docs/about/release-notes.mdx`: Summarizes
Microsoft Teams final-message delivery and runtime mention hints already
covered by messaging docs.
- NVIDIA#5716 -> `docs/about/release-notes.mdx`: Summarizes non-interactive
Ollama loopback safety already covered by local inference docs.
- NVIDIA#5505, NVIDIA#5527, and NVIDIA#5528 -> `docs/about/release-notes.mdx`: Summarizes
compatible local endpoint, model task-fit, and model capability audit
docs.
- NVIDIA#6009 -> `docs/get-started/windows-preparation.mdx`,
`docs/reference/troubleshooting.mdx`: Documents sanitized Windows
bootstrap WSL output and reboot-required gating.
- NVIDIA#6055 -> no additional source doc page change needed beyond the
already-merged quickstart update; release notes did not duplicate
routine quickstart cleanup.

No matching v0.0.71 GitHub announcement discussion was found in the
latest 20 discussions, so this refresh is based on the commit scan and
existing source PR docs.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Quality Gates
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: docs-only refresh with no
runtime behavior changes.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [ ] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only) — ran
`npm run docs`; Fern reported 0 errors and 2 existing warnings.
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

* **Documentation**
* Added a new release-notes entry covering gateway recovery,
authentication, network policy/provenance output, uninstall safety,
Windows bootstrap diagnostics, messaging defaults, and inference setup
guidance.
* Clarified Windows preparation steps around reboot behavior and
redacting troubleshooting transcripts.
* Expanded command reference details for OpenClaw wrapper behavior and
new auto-pair tuning options.
* Improved troubleshooting guidance for plugin installation issues, WSL
repair/reboot cases, and install timing problems.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DGX Spark][Security] Shields auto-relock mid-chat breaks chat with cryptic missing scope: operator.write and no user-facing warning

5 participants