From f546d52910fcdbc4dc5aeff714548d94a6c153f9 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Mon, 29 Jun 2026 10:55:52 +0800 Subject: [PATCH 01/14] fix(sandbox): warn before agent dispatch when shields auto-relocked (#5922) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../actions/sandbox/agent/passthrough.test.ts | 52 +++++++++++++ src/lib/actions/sandbox/agent/passthrough.ts | 17 +++++ src/lib/shields/audit.test.ts | 59 ++++++++++++++ src/lib/shields/audit.ts | 76 ++++++++++++++++++- 4 files changed, 203 insertions(+), 1 deletion(-) diff --git a/src/lib/actions/sandbox/agent/passthrough.test.ts b/src/lib/actions/sandbox/agent/passthrough.test.ts index 792bf71da96..97fed64a5d3 100644 --- a/src/lib/actions/sandbox/agent/passthrough.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough.test.ts @@ -447,6 +447,58 @@ describe("runAgentPassthrough", () => { ); }); + it("emits a shields-relock warning with timeout when shields auto-relocked recently (#5922)", async () => { + getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); + const { writes, proc } = makeProcMock(); + await runAgentPassthrough( + "alpha", + { extraArgs: ["--agent", "main", "-m", "hi"] }, + { + process: proc, + getRecentShieldsAutoRestore: () => ({ + timestamp: new Date().toISOString(), + timeoutSeconds: 20, + }), + }, + ); + expect(execMock).toHaveBeenCalled(); + const all = writes.join(""); + expect(all).toMatch(/[Ss]hields auto-relocked after 20s/); + expect(all).toMatch(/shields down --timeout 20s/); + }); + + it("emits a shields-relock warning with fallback timeout when timeoutSeconds is null (#5922)", async () => { + getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); + const { writes, proc } = makeProcMock(); + await runAgentPassthrough( + "alpha", + { extraArgs: ["--agent", "main", "-m", "hi"] }, + { + process: proc, + getRecentShieldsAutoRestore: () => ({ + timestamp: new Date().toISOString(), + timeoutSeconds: null, + }), + }, + ); + expect(execMock).toHaveBeenCalled(); + const all = writes.join(""); + expect(all).toMatch(/[Ss]hields auto-relocked/); + expect(all).toMatch(/shields down --timeout 60s/); + }); + + it("emits no shields warning when there was no recent auto-restore (#5922)", async () => { + getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); + const { writes } = makeProcMock(); + await runAgentPassthrough( + "alpha", + { extraArgs: ["--agent", "main", "-m", "hi"] }, + { getRecentShieldsAutoRestore: () => null }, + ); + expect(execMock).toHaveBeenCalled(); + expect(writes.join("")).not.toMatch(/[Ss]hields auto-relocked/); + }); + it("rejects with exit 1 + recovery hints when sandbox phase is non-Ready", async () => { ensureLiveMock.mockResolvedValueOnce({ output: "Phase: Error" }); getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); diff --git a/src/lib/actions/sandbox/agent/passthrough.ts b/src/lib/actions/sandbox/agent/passthrough.ts index e2d99c6b4ac..0581d00f36a 100644 --- a/src/lib/actions/sandbox/agent/passthrough.ts +++ b/src/lib/actions/sandbox/agent/passthrough.ts @@ -89,6 +89,7 @@ import { type AgentDefinition, isTerminalAgent, listAgents, loadAgent } from "../../../agent/defs"; import { CLI_NAME } from "../../../cli/branding"; +import { type ShieldsAutoRestoreEvent, readRecentShieldsAutoRestore } from "../../../shields/audit"; import { parseSandboxPhase } from "../../../state/gateway"; import * as registry from "../../../state/registry"; import { execSandbox } from "../exec"; @@ -127,6 +128,7 @@ export interface AgentPassthroughDeps { ensureLive?: typeof ensureLiveSandboxOrExit; exec?: typeof execSandbox; execJson?: typeof runAgentJsonPassthrough; + getRecentShieldsAutoRestore?: (sandboxName: string) => ShieldsAutoRestoreEvent | null; process?: { exit(code: number): never; stdout?: { write(s: string): unknown }; @@ -414,6 +416,21 @@ export async function runAgentPassthrough( if (isOpenClawPassthroughCommand(command) && !hasTargetSelector(extraArgs)) { rejectNoTargetSelector(proc); } + const checkShields = + deps.getRecentShieldsAutoRestore ?? + ((name: string) => readRecentShieldsAutoRestore(name, 10 * 60 * 1000)); + const relock = checkShields(sandboxName); + if (relock) { + const afterPart = + relock.timeoutSeconds !== null ? ` after ${String(relock.timeoutSeconds)}s` : ""; + const timeoutSuggestion = + relock.timeoutSeconds !== null + ? `--timeout ${String(relock.timeoutSeconds)}s` + : "--timeout 60s"; + proc.stderr.write( + ` ⚠ Shields auto-relocked${afterPart} — run \`${CLI_NAME} ${sandboxName} shields down ${timeoutSuggestion}\` to extend.\n`, + ); + } if (isOpenClawPassthroughCommand(command) && requestsOpenClawJsonOutput(extraArgs)) { const execJson = deps.execJson ?? runAgentJsonPassthrough; execJson(sandboxName, command, { diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 7e86abc5c62..1df0863e155 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -3,6 +3,7 @@ import { describe, it, expect, beforeEach, afterEach } from "vitest"; import fs from "node:fs"; +import { readRecentShieldsAutoRestore } from "./audit"; import path from "node:path"; import os from "node:os"; @@ -116,3 +117,61 @@ describe("shields-audit", () => { expect(line).not.toContain("sk-"); }); }); + +describe("readRecentShieldsAutoRestore", () => { + it("returns timestamp and timeoutSeconds when shields_down precedes shields_auto_restore (#5922)", () => { + const now = new Date().toISOString(); + fs.appendFileSync( + auditPath, + JSON.stringify({ + action: "shields_down", + sandbox: "alpha", + timestamp: new Date(Date.now() - 25 * 1000).toISOString(), + timeout_seconds: 20, + }) + + "\n" + + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: now }) + + "\n", + ); + const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); + expect(result?.timestamp).toBe(now); + expect(result?.timeoutSeconds).toBe(20); + }); + + it("returns timestamp with null timeoutSeconds when no shields_down entry exists (#5922)", () => { + const now = new Date().toISOString(); + fs.appendFileSync( + auditPath, + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: now }) + "\n", + ); + const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); + expect(result?.timestamp).toBe(now); + expect(result?.timeoutSeconds).toBeNull(); + }); + + it("returns null when the most recent shields_auto_restore entry is older than the window (#5922)", () => { + const old = new Date(Date.now() - 20 * 60 * 1000).toISOString(); + fs.appendFileSync( + auditPath, + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: old }) + "\n", + ); + const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); + expect(result).toBeNull(); + }); + + it("returns null when the recent shields_auto_restore entry is for a different sandbox (#5922)", () => { + const now = new Date().toISOString(); + fs.appendFileSync( + auditPath, + JSON.stringify({ action: "shields_auto_restore", sandbox: "other-sb", timestamp: now }) + + "\n", + ); + const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); + expect(result).toBeNull(); + }); + + it("returns null when the audit file does not exist (#5922)", () => { + const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); + expect(result).toBeNull(); + }); +}); diff --git a/src/lib/shields/audit.ts b/src/lib/shields/audit.ts index 5d25e0a6277..74ba0e9d625 100644 --- a/src/lib/shields/audit.ts +++ b/src/lib/shields/audit.ts @@ -10,7 +10,7 @@ * Entries never contain credential values — only key names and policy labels. */ -import { appendFileSync } from "node:fs"; +import { appendFileSync, readFileSync } from "node:fs"; import { join } from "node:path"; import { redactFull } from "../security/redact"; import { ensureConfigDir } from "../state/config-io"; @@ -56,4 +56,78 @@ export function appendAuditEntry(entry: ShieldsAuditEntry): void { appendFileSync(AUDIT_FILE, JSON.stringify(safe) + "\n", { mode: 0o600 }); } +export interface ShieldsAutoRestoreEvent { + /** ISO timestamp written by the auto-restore timer. */ + timestamp: string; + /** + * Original timeout in seconds from the preceding `shields_down` entry, or + * null when that entry is not found in the audit log. + */ + timeoutSeconds: number | null; +} + +/** + * Scan the audit log in reverse and return details about the most recent + * `shields_auto_restore` event for the given sandbox that falls within + * `withinMs` milliseconds of now. Also reads the preceding `shields_down` + * entry to recover the original timeout so callers can echo it back. + * + * Returns null when no matching entry is found or the file is unreadable. + * The optional `auditFile` parameter overrides the default path; used in tests. + */ +export function readRecentShieldsAutoRestore( + sandboxName: string, + withinMs: number, + auditFile: string = AUDIT_FILE, +): ShieldsAutoRestoreEvent | null { + let content: string; + try { + content = readFileSync(auditFile, "utf8"); + } catch { + return null; + } + const cutoff = Date.now() - withinMs; + const lines = content.split("\n"); + + function parseEntry(line: string): Record | null { + const trimmed = line.trim(); + if (!trimmed) return null; + try { + const parsed = JSON.parse(trimmed); + if (parsed !== null && typeof parsed === "object" && !Array.isArray(parsed)) { + return parsed as Record; + } + } catch { + // malformed line — skip + } + return null; + } + + // Scan backwards for the most recent shields_auto_restore within the window. + for (let i = lines.length - 1; i >= 0; i--) { + const entry = parseEntry(lines[i]); + if ( + entry?.action === "shields_auto_restore" && + entry.sandbox === sandboxName && + typeof entry.timestamp === "string" && + new Date(entry.timestamp).getTime() >= cutoff + ) { + const restoreTs = entry.timestamp; + // Continue backwards to find the preceding shields_down to get timeout_seconds. + let timeoutSeconds: number | null = null; + for (let j = i - 1; j >= 0; j--) { + const prev = parseEntry(lines[j]); + if (prev?.action === "shields_down" && prev.sandbox === sandboxName) { + if (typeof prev.timeout_seconds === "number") { + timeoutSeconds = prev.timeout_seconds; + } + break; + } + } + return { timestamp: restoreTs, timeoutSeconds }; + } + } + return null; +} + export { AUDIT_FILE, AUDIT_DIR }; From a2ac6bb82df8dc67ed8afa90531cb545b7f8c53d Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Mon, 29 Jun 2026 11:09:41 +0800 Subject: [PATCH 02/14] fix(sandbox): address PRA-4/5/6 on shields-relock warning (#5922) 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 --- .../actions/sandbox/agent/passthrough.test.ts | 3 +++ src/lib/actions/sandbox/agent/passthrough.ts | 7 ++++++- src/lib/shields/audit.test.ts | 21 +++++++++++++++++++ src/lib/shields/audit.ts | 8 ++++++- 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/src/lib/actions/sandbox/agent/passthrough.test.ts b/src/lib/actions/sandbox/agent/passthrough.test.ts index 97fed64a5d3..518fd780cd0 100644 --- a/src/lib/actions/sandbox/agent/passthrough.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough.test.ts @@ -32,6 +32,9 @@ vi.mock("../../../agent/defs", () => ({ listAgents: listAgentsMock, loadAgent: loadAgentMock, })); +// Default to no recent shields auto-restore so tests that don't inject +// getRecentShieldsAutoRestore don't read ~/.nemoclaw/state/shields-audit.jsonl. +vi.mock("../../../shields/audit", () => ({ readRecentShieldsAutoRestore: vi.fn(() => null) })); import { type AgentPassthroughDeps, runAgentPassthrough } from "./passthrough"; diff --git a/src/lib/actions/sandbox/agent/passthrough.ts b/src/lib/actions/sandbox/agent/passthrough.ts index 0581d00f36a..59a88b5e6e2 100644 --- a/src/lib/actions/sandbox/agent/passthrough.ts +++ b/src/lib/actions/sandbox/agent/passthrough.ts @@ -74,7 +74,8 @@ // unparseable phase fail-closed path, the OpenClaw no-selector rejection, and // the `--flag=value` selector-acceptance branch, plus the OpenClaw JSON // captured transport path used to append failure provenance without polluting -// machine-readable stdout. +// machine-readable stdout, plus shields auto-relock warning with timeout, +// null-timeout fallback, and no-warning path. // // Removal conditions: // @@ -86,6 +87,10 @@ // missing selector with a clean exit 2 and an actionable message. // - Drop the simple-token parser when terminal runtime manifests expose // argv arrays natively. +// - Drop the shields-relock warning when OpenClaw exposes the relock cause +// directly (e.g., a distinct exit code or structured error field for +// missing-scope-after-relock), or when NemoClaw implements +// extend-on-activity so the scope never lapses mid-session. import { type AgentDefinition, isTerminalAgent, listAgents, loadAgent } from "../../../agent/defs"; import { CLI_NAME } from "../../../cli/branding"; diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 1df0863e155..48267d3adb0 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -174,4 +174,25 @@ describe("readRecentShieldsAutoRestore", () => { const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); expect(result).toBeNull(); }); + + it("returns null timeoutSeconds for out-of-bounds timeout values in shields_down entry (#5922)", () => { + const now = new Date().toISOString(); + for (const bad of [0, -1, 1801, 9999, 1.5, Number.POSITIVE_INFINITY, Number.NaN]) { + fs.writeFileSync( + auditPath, + JSON.stringify({ + action: "shields_down", + sandbox: "alpha", + timestamp: new Date(Date.now() - 25 * 1000).toISOString(), + timeout_seconds: bad, + }) + + "\n" + + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: now }) + + "\n", + ); + const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); + expect(result?.timestamp, `bad value ${String(bad)}`).toBe(now); + expect(result?.timeoutSeconds, `bad value ${String(bad)}`).toBeNull(); + } + }); }); diff --git a/src/lib/shields/audit.ts b/src/lib/shields/audit.ts index 74ba0e9d625..08dd3aa472c 100644 --- a/src/lib/shields/audit.ts +++ b/src/lib/shields/audit.ts @@ -118,7 +118,13 @@ export function readRecentShieldsAutoRestore( for (let j = i - 1; j >= 0; j--) { const prev = parseEntry(lines[j]); if (prev?.action === "shields_down" && prev.sandbox === sandboxName) { - if (typeof prev.timeout_seconds === "number") { + if ( + typeof prev.timeout_seconds === "number" && + Number.isFinite(prev.timeout_seconds) && + Number.isInteger(prev.timeout_seconds) && + prev.timeout_seconds >= 1 && + prev.timeout_seconds <= 1800 + ) { timeoutSeconds = prev.timeout_seconds; } break; From 7d06316f1ec97a651e60b3c4610df549bbd05c31 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Mon, 29 Jun 2026 11:27:27 +0800 Subject: [PATCH 03/14] test(sandbox): fix two test gaps flagged by CodeRabbit (#5922) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../actions/sandbox/agent/passthrough.test.ts | 4 ++-- src/lib/shields/audit.test.ts | 22 ++++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/lib/actions/sandbox/agent/passthrough.test.ts b/src/lib/actions/sandbox/agent/passthrough.test.ts index 518fd780cd0..7733e9074e3 100644 --- a/src/lib/actions/sandbox/agent/passthrough.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough.test.ts @@ -492,11 +492,11 @@ describe("runAgentPassthrough", () => { it("emits no shields warning when there was no recent auto-restore (#5922)", async () => { getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); - const { writes } = makeProcMock(); + const { writes, proc } = makeProcMock(); await runAgentPassthrough( "alpha", { extraArgs: ["--agent", "main", "-m", "hi"] }, - { getRecentShieldsAutoRestore: () => null }, + { getRecentShieldsAutoRestore: () => null, process: proc }, ); expect(execMock).toHaveBeenCalled(); expect(writes.join("")).not.toMatch(/[Ss]hields auto-relocked/); diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 48267d3adb0..1442a7415e6 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -177,7 +177,8 @@ describe("readRecentShieldsAutoRestore", () => { it("returns null timeoutSeconds for out-of-bounds timeout values in shields_down entry (#5922)", () => { const now = new Date().toISOString(); - for (const bad of [0, -1, 1801, 9999, 1.5, Number.POSITIVE_INFINITY, Number.NaN]) { + // JSON.stringify serializes these correctly (finite numbers) + for (const bad of [0, -1, 1801, 9999, 1.5]) { fs.writeFileSync( auditPath, JSON.stringify({ @@ -195,4 +196,23 @@ describe("readRecentShieldsAutoRestore", () => { expect(result?.timeoutSeconds, `bad value ${String(bad)}`).toBeNull(); } }); + + it("returns null timeoutSeconds when shields_down has NaN or Infinity as a raw string payload (#5922)", () => { + // JSON.stringify(NaN) and JSON.stringify(Infinity) both produce "null", + // so write the JSONL line manually to exercise the non-finite path. + const now = new Date().toISOString(); + for (const rawValue of ["NaN", "Infinity", "-Infinity"]) { + const downLine = `{"action":"shields_down","sandbox":"alpha","timestamp":"${new Date(Date.now() - 25 * 1000).toISOString()}","timeout_seconds":${rawValue}}`; + const restoreLine = JSON.stringify({ + action: "shields_auto_restore", + sandbox: "alpha", + timestamp: now, + }); + fs.writeFileSync(auditPath, downLine + "\n" + restoreLine + "\n"); + // Malformed JSON (NaN/Infinity are not valid JSON) → parseEntry returns null → timeoutSeconds stays null + const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); + expect(result?.timestamp, `raw value ${rawValue}`).toBe(now); + expect(result?.timeoutSeconds, `raw value ${rawValue}`).toBeNull(); + } + }); }); From 62785c62527756744e0a8f4d84345a8e7a9dbe5c Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Mon, 29 Jun 2026 12:04:42 +0800 Subject: [PATCH 04/14] fix(sandbox): reject future-dated audit entries in shields restore check (#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 --- src/lib/shields/audit.test.ts | 10 ++++++++ src/lib/shields/audit.ts | 46 ++++++++++++++++++----------------- 2 files changed, 34 insertions(+), 22 deletions(-) diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 1442a7415e6..40852f79a4a 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -149,6 +149,16 @@ describe("readRecentShieldsAutoRestore", () => { expect(result?.timeoutSeconds).toBeNull(); }); + it("returns null when the shields_auto_restore entry is future-dated (#5922)", () => { + const future = new Date(Date.now() + 60 * 1000).toISOString(); + fs.appendFileSync( + auditPath, + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: future }) + "\n", + ); + const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); + expect(result).toBeNull(); + }); + it("returns null when the most recent shields_auto_restore entry is older than the window (#5922)", () => { const old = new Date(Date.now() - 20 * 60 * 1000).toISOString(); fs.appendFileSync( diff --git a/src/lib/shields/audit.ts b/src/lib/shields/audit.ts index 08dd3aa472c..ec8eb9100cc 100644 --- a/src/lib/shields/audit.ts +++ b/src/lib/shields/audit.ts @@ -103,35 +103,37 @@ export function readRecentShieldsAutoRestore( return null; } + const now = Date.now(); // Scan backwards for the most recent shields_auto_restore within the window. for (let i = lines.length - 1; i >= 0; i--) { const entry = parseEntry(lines[i]); if ( - entry?.action === "shields_auto_restore" && - entry.sandbox === sandboxName && - typeof entry.timestamp === "string" && - new Date(entry.timestamp).getTime() >= cutoff - ) { - const restoreTs = entry.timestamp; - // Continue backwards to find the preceding shields_down to get timeout_seconds. - let timeoutSeconds: number | null = null; - for (let j = i - 1; j >= 0; j--) { - const prev = parseEntry(lines[j]); - if (prev?.action === "shields_down" && prev.sandbox === sandboxName) { - if ( - typeof prev.timeout_seconds === "number" && - Number.isFinite(prev.timeout_seconds) && - Number.isInteger(prev.timeout_seconds) && - prev.timeout_seconds >= 1 && - prev.timeout_seconds <= 1800 - ) { - timeoutSeconds = prev.timeout_seconds; - } - break; + entry?.action !== "shields_auto_restore" || + entry.sandbox !== sandboxName || + typeof entry.timestamp !== "string" + ) + continue; + const restoreMs = new Date(entry.timestamp).getTime(); + if (!Number.isFinite(restoreMs) || restoreMs < cutoff || restoreMs > now) continue; + const restoreTs = entry.timestamp; + // Continue backwards to find the preceding shields_down to get timeout_seconds. + let timeoutSeconds: number | null = null; + for (let j = i - 1; j >= 0; j--) { + const prev = parseEntry(lines[j]); + if (prev?.action === "shields_down" && prev.sandbox === sandboxName) { + if ( + typeof prev.timeout_seconds === "number" && + Number.isFinite(prev.timeout_seconds) && + Number.isInteger(prev.timeout_seconds) && + prev.timeout_seconds >= 1 && + prev.timeout_seconds <= 1800 + ) { + timeoutSeconds = prev.timeout_seconds; } + break; } - return { timestamp: restoreTs, timeoutSeconds }; } + return { timestamp: restoreTs, timeoutSeconds }; } return null; } From 9f4275cff1b961c9f76267da9daa53e562f47c88 Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Mon, 29 Jun 2026 12:09:02 +0800 Subject: [PATCH 05/14] style(sandbox): apply Biome format to audit files (#5922) Refs #5922 Signed-off-by: Dongni Yang --- src/lib/shields/audit.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 40852f79a4a..814fd37d9d7 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -153,7 +153,8 @@ describe("readRecentShieldsAutoRestore", () => { const future = new Date(Date.now() + 60 * 1000).toISOString(); fs.appendFileSync( auditPath, - JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: future }) + "\n", + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: future }) + + "\n", ); const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); expect(result).toBeNull(); From 5363d0f3cd43109bf382b626bfc92bb3d07bc62a Mon Sep 17 00:00:00 2001 From: Dongni Yang Date: Mon, 29 Jun 2026 12:50:02 +0800 Subject: [PATCH 06/14] refactor(sandbox): address PRA required + resolve items for #5922 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../actions/sandbox/agent/passthrough.test.ts | 43 ++++++---------- src/lib/actions/sandbox/agent/passthrough.ts | 31 ++++++++---- src/lib/shields/audit.test.ts | 50 +++++++++++++++++++ src/lib/shields/audit.ts | 15 +++++- 4 files changed, 100 insertions(+), 39 deletions(-) diff --git a/src/lib/actions/sandbox/agent/passthrough.test.ts b/src/lib/actions/sandbox/agent/passthrough.test.ts index 7733e9074e3..41ff68ffbb5 100644 --- a/src/lib/actions/sandbox/agent/passthrough.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough.test.ts @@ -450,7 +450,11 @@ describe("runAgentPassthrough", () => { ); }); - it("emits a shields-relock warning with timeout when shields auto-relocked recently (#5922)", async () => { + // Shared helper for shields-warning tests: wires up the OpenClaw mock and + // proc, dispatches with a fixed extraArgs, and returns the stderr output. + async function runShieldsWarningTest( + restore: { timeoutSeconds: number | null } | null, + ): Promise { getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); const { writes, proc } = makeProcMock(); await runAgentPassthrough( @@ -458,48 +462,31 @@ describe("runAgentPassthrough", () => { { extraArgs: ["--agent", "main", "-m", "hi"] }, { process: proc, - getRecentShieldsAutoRestore: () => ({ - timestamp: new Date().toISOString(), - timeoutSeconds: 20, - }), + getRecentShieldsAutoRestore: () => + restore !== null ? { timestamp: new Date().toISOString(), ...restore } : null, }, ); + return writes.join(""); + } + + it("emits a shields-relock warning with timeout when shields auto-relocked recently (#5922)", async () => { + const all = await runShieldsWarningTest({ timeoutSeconds: 20 }); expect(execMock).toHaveBeenCalled(); - const all = writes.join(""); expect(all).toMatch(/[Ss]hields auto-relocked after 20s/); expect(all).toMatch(/shields down --timeout 20s/); }); it("emits a shields-relock warning with fallback timeout when timeoutSeconds is null (#5922)", async () => { - getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); - const { writes, proc } = makeProcMock(); - await runAgentPassthrough( - "alpha", - { extraArgs: ["--agent", "main", "-m", "hi"] }, - { - process: proc, - getRecentShieldsAutoRestore: () => ({ - timestamp: new Date().toISOString(), - timeoutSeconds: null, - }), - }, - ); + const all = await runShieldsWarningTest({ timeoutSeconds: null }); expect(execMock).toHaveBeenCalled(); - const all = writes.join(""); expect(all).toMatch(/[Ss]hields auto-relocked/); expect(all).toMatch(/shields down --timeout 60s/); }); it("emits no shields warning when there was no recent auto-restore (#5922)", async () => { - getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); - const { writes, proc } = makeProcMock(); - await runAgentPassthrough( - "alpha", - { extraArgs: ["--agent", "main", "-m", "hi"] }, - { getRecentShieldsAutoRestore: () => null, process: proc }, - ); + const all = await runShieldsWarningTest(null); expect(execMock).toHaveBeenCalled(); - expect(writes.join("")).not.toMatch(/[Ss]hields auto-relocked/); + expect(all).not.toMatch(/[Ss]hields auto-relocked/); }); it("rejects with exit 1 + recovery hints when sandbox phase is non-Ready", async () => { diff --git a/src/lib/actions/sandbox/agent/passthrough.ts b/src/lib/actions/sandbox/agent/passthrough.ts index 59a88b5e6e2..a33226bf62d 100644 --- a/src/lib/actions/sandbox/agent/passthrough.ts +++ b/src/lib/actions/sandbox/agent/passthrough.ts @@ -91,6 +91,7 @@ // directly (e.g., a distinct exit code or structured error field for // missing-scope-after-relock), or when NemoClaw implements // extend-on-activity so the scope never lapses mid-session. +// (The shields_auto_restore audit entry is appended by timer.ts:295–302.) import { type AgentDefinition, isTerminalAgent, listAgents, loadAgent } from "../../../agent/defs"; import { CLI_NAME } from "../../../cli/branding"; @@ -352,6 +353,23 @@ function hasTargetSelector(args: readonly string[]): boolean { return false; } +function emitShieldsRelockWarning( + proc: NonNullable, + relock: ShieldsAutoRestoreEvent, + sandboxName: string, +): void { + const afterPart = + relock.timeoutSeconds !== null ? ` after ${String(relock.timeoutSeconds)}s` : ""; + // timeoutSeconds is pre-validated by readRecentShieldsAutoRestore (finite integer, 1–1800). + const timeoutSuggestion = + relock.timeoutSeconds !== null + ? `--timeout ${String(relock.timeoutSeconds)}s` + : "--timeout 60s"; + proc.stderr.write( + ` ⚠ Shields auto-relocked${afterPart} — run \`${CLI_NAME} ${sandboxName} shields down ${timeoutSuggestion}\` to extend.\n`, + ); +} + function rejectNoTargetSelector(proc: NonNullable): never { proc.stderr.write( " No target session selected. Use --agent , --session-key , --session-id , or --to .\n", @@ -421,20 +439,15 @@ export async function runAgentPassthrough( if (isOpenClawPassthroughCommand(command) && !hasTargetSelector(extraArgs)) { rejectNoTargetSelector(proc); } + // 10-min window: shields timeouts range 1–1800s; 10 min covers even the max + // 30-min timeout with a 2× buffer. A longer window risks false-positive warnings + // on a relock from a prior session. Adjust if the upstream max changes. const checkShields = deps.getRecentShieldsAutoRestore ?? ((name: string) => readRecentShieldsAutoRestore(name, 10 * 60 * 1000)); const relock = checkShields(sandboxName); if (relock) { - const afterPart = - relock.timeoutSeconds !== null ? ` after ${String(relock.timeoutSeconds)}s` : ""; - const timeoutSuggestion = - relock.timeoutSeconds !== null - ? `--timeout ${String(relock.timeoutSeconds)}s` - : "--timeout 60s"; - proc.stderr.write( - ` ⚠ Shields auto-relocked${afterPart} — run \`${CLI_NAME} ${sandboxName} shields down ${timeoutSuggestion}\` to extend.\n`, - ); + emitShieldsRelockWarning(proc, relock, sandboxName); } if (isOpenClawPassthroughCommand(command) && requestsOpenClawJsonOutput(extraArgs)) { const execJson = deps.execJson ?? runAgentJsonPassthrough; diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 814fd37d9d7..9c91d64d71d 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -208,6 +208,56 @@ describe("readRecentShieldsAutoRestore", () => { } }); + it("returns correct timeoutSeconds when a malformed JSONL line sits between shields_down and shields_auto_restore (#5922)", () => { + const now = new Date().toISOString(); + const downLine = JSON.stringify({ + action: "shields_down", + sandbox: "alpha", + timestamp: new Date(Date.now() - 25 * 1000).toISOString(), + timeout_seconds: 30, + }); + // Malformed line between the two valid entries; parseEntry must skip it and + // continue to find the preceding shields_down. + fs.writeFileSync( + auditPath, + downLine + + "\n{not valid json\n" + + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: now }) + + "\n", + ); + const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); + expect(result?.timestamp).toBe(now); + expect(result?.timeoutSeconds).toBe(30); + }); + + it("uses the immediately-preceding shields_down when multiple exist (#5922)", () => { + const now = new Date().toISOString(); + // Two shields_down entries with different timeout_seconds. The second (most + // recent) should be used because it immediately precedes the auto-restore. + fs.writeFileSync( + auditPath, + JSON.stringify({ + action: "shields_down", + sandbox: "alpha", + timestamp: new Date(Date.now() - 60 * 1000).toISOString(), + timeout_seconds: 120, + }) + + "\n" + + JSON.stringify({ + action: "shields_down", + sandbox: "alpha", + timestamp: new Date(Date.now() - 25 * 1000).toISOString(), + timeout_seconds: 45, + }) + + "\n" + + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: now }) + + "\n", + ); + const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); + expect(result?.timestamp).toBe(now); + expect(result?.timeoutSeconds).toBe(45); + }); + it("returns null timeoutSeconds when shields_down has NaN or Infinity as a raw string payload (#5922)", () => { // JSON.stringify(NaN) and JSON.stringify(Infinity) both produce "null", // so write the JSONL line manually to exercise the non-finite path. diff --git a/src/lib/shields/audit.ts b/src/lib/shields/audit.ts index ec8eb9100cc..29cc0cdae28 100644 --- a/src/lib/shields/audit.ts +++ b/src/lib/shields/audit.ts @@ -72,7 +72,16 @@ export interface ShieldsAutoRestoreEvent { * `withinMs` milliseconds of now. Also reads the preceding `shields_down` * entry to recover the original timeout so callers can echo it back. * - * Returns null when no matching entry is found or the file is unreadable. + * Returns null when no matching entry is found OR when the file is unreadable. + * Fail-open is intentional: blocking agent dispatch on audit I/O errors (e.g. + * EACCES, EIO) would be a DoS vector — callers must treat null as "no warning" + * rather than "no event." Future-dated entries are rejected as a clock-skew + * defense so a crafted entry cannot pin the warning permanently. + * + * File-size note: the audit file is user-owned and written only by NemoClaw at + * ~200 bytes per entry. An unbounded readFileSync is acceptable for this + * warning-only path; add a size cap if the audit log gains a rotation policy. + * * The optional `auditFile` parameter overrides the default path; used in tests. */ export function readRecentShieldsAutoRestore( @@ -98,7 +107,9 @@ export function readRecentShieldsAutoRestore( return parsed as Record; } } catch { - // malformed line — skip + // Intentional resilient skip: a malformed or truncated JSONL line (e.g. + // from a partial write or manual edit) must not prevent finding valid + // surrounding entries. The reverse scan continues past it. } return null; } From 3ec75abfa0bfa9eec5a0f348baa8f65e14ab7915 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Tue, 30 Jun 2026 01:02:09 -0700 Subject: [PATCH 07/14] fix(sandbox): harden shields relock warning Signed-off-by: Carlos Villela --- .../agent/passthrough-shields-warning.test.ts | 131 ++++++++++++++++++ .../actions/sandbox/agent/passthrough.test.ts | 43 +----- src/lib/actions/sandbox/agent/passthrough.ts | 80 ++++++++--- src/lib/shields/audit.test.ts | 128 +++++++++++++---- src/lib/shields/audit.ts | 78 ++++++++--- 5 files changed, 355 insertions(+), 105 deletions(-) create mode 100644 src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts diff --git a/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts b/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts new file mode 100644 index 00000000000..0b9b71ec0f6 --- /dev/null +++ b/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts @@ -0,0 +1,131 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { ShieldsAutoRestoreReadResult } from "../../../shields/audit"; + +const execMock = vi.hoisted(() => vi.fn(async () => {})); +const ensureLiveMock = vi.hoisted(() => + vi.fn(async () => ({ state: "present", output: "Phase: Ready" }) as { output?: string }), +); +const getSandboxMock = vi.hoisted(() => vi.fn(() => ({ agent: "openclaw" }))); +const listAgentsMock = vi.hoisted(() => vi.fn(() => ["langchain-deepagents-code", "openclaw"])); +const loadAgentMock = vi.hoisted(() => + vi.fn((name: string) => ({ + name, + runtime: + name === "langchain-deepagents-code" + ? { kind: "terminal", interactive_command: "dcode", headless_command: "dcode -n" } + : undefined, + })), +); +const isTerminalAgentMock = vi.hoisted(() => + vi.fn((agent: { runtime?: { kind?: string } }) => agent.runtime?.kind === "terminal"), +); + +vi.mock("../exec", () => ({ execSandbox: execMock })); +vi.mock("../gateway-state", () => ({ ensureLiveSandboxOrExit: ensureLiveMock })); +vi.mock("../../../state/registry", () => ({ getSandbox: getSandboxMock })); +vi.mock("../../../agent/defs", () => ({ + isTerminalAgent: isTerminalAgentMock, + listAgents: listAgentsMock, + loadAgent: loadAgentMock, +})); +vi.mock("../../../shields/audit", () => ({ + readRecentShieldsAutoRestore: vi.fn(() => ({ kind: "none" })), +})); + +import { runAgentPassthrough } from "./passthrough"; + +function makeProcMock() { + const writes: string[] = []; + return { + writes, + proc: { + exit: ((code: number): never => { + throw new Error(`__exit:${code}`); + }) as (code: number) => never, + stderr: { write: (value: string) => writes.push(value) }, + }, + }; +} + +describe("runAgentPassthrough shields-relock warning", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + async function runWarning(result: ShieldsAutoRestoreReadResult): Promise { + getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); + const { writes, proc } = makeProcMock(); + await runAgentPassthrough( + "alpha", + { extraArgs: ["--agent", "main", "-m", "hi"] }, + { process: proc, getRecentShieldsAutoRestore: () => result }, + ); + return writes.join(""); + } + + it("emits the original timeout after a recent auto-relock (#5922)", async () => { + const output = await runWarning({ + kind: "event", + event: { timestamp: new Date().toISOString(), timeoutSeconds: 20 }, + }); + expect(execMock).toHaveBeenCalled(); + expect(output).toMatch(/[Ss]hields auto-relocked after 20s/); + expect(output).toMatch(/shields down --timeout 20s/); + }); + + it("uses the safe fallback timeout when the original timeout is unavailable (#5922)", async () => { + const output = await runWarning({ + kind: "event", + event: { timestamp: new Date().toISOString(), timeoutSeconds: null }, + }); + expect(execMock).toHaveBeenCalled(); + expect(output).toMatch(/[Ss]hields auto-relocked/); + expect(output).toMatch(/shields down --timeout 60s/); + }); + + it("defensively rejects an invalid injected timeout from the command suggestion (#5922)", async () => { + const output = await runWarning({ + kind: "event", + event: { timestamp: new Date().toISOString(), timeoutSeconds: 9999 }, + }); + expect(output).not.toContain("9999s"); + expect(output).toMatch(/shields down --timeout 60s/); + }); + + it("emits no relock warning when the audit has no recent event (#5922)", async () => { + const output = await runWarning({ kind: "none" }); + expect(execMock).toHaveBeenCalled(); + expect(output).not.toMatch(/[Ss]hields auto-relocked/); + }); + + it("reports unreadable audit history without blocking agent dispatch (#5922)", async () => { + const output = await runWarning({ kind: "unreadable" }); + expect(execMock).toHaveBeenCalled(); + expect(output).toMatch(/Could not read shields audit history/); + expect(output).toMatch(/shields status/); + }); + + it("does not consult OpenClaw relock history for terminal-runtime passthroughs (#5922)", async () => { + getSandboxMock.mockReturnValueOnce({ agent: "langchain-deepagents-code" }); + const getRecentShieldsAutoRestore = vi.fn( + (): ShieldsAutoRestoreReadResult => ({ + kind: "event", + event: { timestamp: new Date().toISOString(), timeoutSeconds: 20 }, + }), + ); + const { writes, proc } = makeProcMock(); + + await runAgentPassthrough( + "alpha", + { extraArgs: ["--help"] }, + { process: proc, getRecentShieldsAutoRestore }, + ); + + expect(execMock).toHaveBeenCalledWith("alpha", ["dcode", "--help"], { tty: false }); + expect(getRecentShieldsAutoRestore).not.toHaveBeenCalled(); + expect(writes.join("")).not.toMatch(/[Ss]hields auto-relocked/); + }); +}); diff --git a/src/lib/actions/sandbox/agent/passthrough.test.ts b/src/lib/actions/sandbox/agent/passthrough.test.ts index 41ff68ffbb5..1cd877d4ddc 100644 --- a/src/lib/actions/sandbox/agent/passthrough.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough.test.ts @@ -34,7 +34,9 @@ vi.mock("../../../agent/defs", () => ({ })); // Default to no recent shields auto-restore so tests that don't inject // getRecentShieldsAutoRestore don't read ~/.nemoclaw/state/shields-audit.jsonl. -vi.mock("../../../shields/audit", () => ({ readRecentShieldsAutoRestore: vi.fn(() => null) })); +vi.mock("../../../shields/audit", () => ({ + readRecentShieldsAutoRestore: vi.fn(() => ({ kind: "none" })), +})); import { type AgentPassthroughDeps, runAgentPassthrough } from "./passthrough"; @@ -450,45 +452,6 @@ describe("runAgentPassthrough", () => { ); }); - // Shared helper for shields-warning tests: wires up the OpenClaw mock and - // proc, dispatches with a fixed extraArgs, and returns the stderr output. - async function runShieldsWarningTest( - restore: { timeoutSeconds: number | null } | null, - ): Promise { - getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); - const { writes, proc } = makeProcMock(); - await runAgentPassthrough( - "alpha", - { extraArgs: ["--agent", "main", "-m", "hi"] }, - { - process: proc, - getRecentShieldsAutoRestore: () => - restore !== null ? { timestamp: new Date().toISOString(), ...restore } : null, - }, - ); - return writes.join(""); - } - - it("emits a shields-relock warning with timeout when shields auto-relocked recently (#5922)", async () => { - const all = await runShieldsWarningTest({ timeoutSeconds: 20 }); - expect(execMock).toHaveBeenCalled(); - expect(all).toMatch(/[Ss]hields auto-relocked after 20s/); - expect(all).toMatch(/shields down --timeout 20s/); - }); - - it("emits a shields-relock warning with fallback timeout when timeoutSeconds is null (#5922)", async () => { - const all = await runShieldsWarningTest({ timeoutSeconds: null }); - expect(execMock).toHaveBeenCalled(); - expect(all).toMatch(/[Ss]hields auto-relocked/); - expect(all).toMatch(/shields down --timeout 60s/); - }); - - it("emits no shields warning when there was no recent auto-restore (#5922)", async () => { - const all = await runShieldsWarningTest(null); - expect(execMock).toHaveBeenCalled(); - expect(all).not.toMatch(/[Ss]hields auto-relocked/); - }); - it("rejects with exit 1 + recovery hints when sandbox phase is non-Ready", async () => { ensureLiveMock.mockResolvedValueOnce({ output: "Phase: Error" }); getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); diff --git a/src/lib/actions/sandbox/agent/passthrough.ts b/src/lib/actions/sandbox/agent/passthrough.ts index a33226bf62d..0e1a0ad0872 100644 --- a/src/lib/actions/sandbox/agent/passthrough.ts +++ b/src/lib/actions/sandbox/agent/passthrough.ts @@ -3,7 +3,8 @@ // Source-of-truth boundary for the `nemoclaw agent` passthrough. // -// The wrapper enforces three host-side mirrors of upstream contracts: +// The wrapper enforces three host-side mirrors of upstream contracts and one +// advisory diagnostic: // // 1. Agent-kind guard (registry mirror). // @@ -67,6 +68,20 @@ // selector case is intercepted; everything else still flows through to // the in-sandbox binary. // +// 4. Recent shields-relock diagnostic (advisory audit mirror). +// +// - Invalid state: after shields auto-relock, the next host CLI +// `openclaw agent` dispatch can fail with only `missing scope: +// operator.write`, which does not explain the recovery action. +// - Source boundary: OpenShell/OpenClaw own current scope state. NemoClaw's +// local audit JSONL is non-authoritative and is used only to add likely +// relock context; unreadable history never blocks dispatch, and terminal +// runtimes are excluded from this OpenClaw-specific diagnostic. +// - Source-fix constraint: an already-running in-sandbox OpenClaw TUI has no +// host CLI interception point. Covering that surface requires an upstream +// structured relock error or a separate extend-on-activity design. This +// wrapper intentionally covers only `nemoclaw agent` dispatches. +// // Regression tests: `passthrough.test.ts` covers the Hermes redirect, the // forwarded argv, the registry-miss fallback to OpenClaw, registry and // manifest-resolution fail-closed paths, quoted manifest command rejection, @@ -74,8 +89,9 @@ // unparseable phase fail-closed path, the OpenClaw no-selector rejection, and // the `--flag=value` selector-acceptance branch, plus the OpenClaw JSON // captured transport path used to append failure provenance without polluting -// machine-readable stdout, plus shields auto-relock warning with timeout, -// null-timeout fallback, and no-warning path. +// machine-readable stdout. `passthrough-shields-warning.test.ts` covers the +// OpenClaw-only relock diagnostic, validated and fallback timeouts, unreadable +// and absent audit history, and terminal-runtime exclusion. // // Removal conditions: // @@ -95,7 +111,11 @@ import { type AgentDefinition, isTerminalAgent, listAgents, loadAgent } from "../../../agent/defs"; import { CLI_NAME } from "../../../cli/branding"; -import { type ShieldsAutoRestoreEvent, readRecentShieldsAutoRestore } from "../../../shields/audit"; +import { + readRecentShieldsAutoRestore, + type ShieldsAutoRestoreEvent, + type ShieldsAutoRestoreReadResult, +} from "../../../shields/audit"; import { parseSandboxPhase } from "../../../state/gateway"; import * as registry from "../../../state/registry"; import { execSandbox } from "../exec"; @@ -125,6 +145,11 @@ const OPENCLAW_AGENT_VALUE_FLAGS = new Set([ const OPENCLAW_AGENT_BOOLEAN_FLAGS = new Set(["--deliver"]); +// A relock remains useful context briefly after it happens. This is a +// relevance window measured from the restore event, independent of the +// original shields-down timeout; a longer window risks stale-session warnings. +const SHIELDS_RELOCK_WARNING_WINDOW_MS = 10 * 60 * 1000; + export interface AgentPassthroughOptions { extraArgs?: readonly string[]; } @@ -134,7 +159,7 @@ export interface AgentPassthroughDeps { ensureLive?: typeof ensureLiveSandboxOrExit; exec?: typeof execSandbox; execJson?: typeof runAgentJsonPassthrough; - getRecentShieldsAutoRestore?: (sandboxName: string) => ShieldsAutoRestoreEvent | null; + getRecentShieldsAutoRestore?: (sandboxName: string) => ShieldsAutoRestoreReadResult; process?: { exit(code: number): never; stdout?: { write(s: string): unknown }; @@ -358,18 +383,32 @@ function emitShieldsRelockWarning( relock: ShieldsAutoRestoreEvent, sandboxName: string, ): void { - const afterPart = - relock.timeoutSeconds !== null ? ` after ${String(relock.timeoutSeconds)}s` : ""; - // timeoutSeconds is pre-validated by readRecentShieldsAutoRestore (finite integer, 1–1800). + // Defend the user-facing command suggestion even when tests or future + // callers inject an event without going through the audit reader. + const timeoutSeconds = + relock.timeoutSeconds !== null && + Number.isInteger(relock.timeoutSeconds) && + relock.timeoutSeconds >= 1 && + relock.timeoutSeconds <= 1800 + ? relock.timeoutSeconds + : null; + const afterPart = timeoutSeconds !== null ? ` after ${String(timeoutSeconds)}s` : ""; const timeoutSuggestion = - relock.timeoutSeconds !== null - ? `--timeout ${String(relock.timeoutSeconds)}s` - : "--timeout 60s"; + timeoutSeconds !== null ? `--timeout ${String(timeoutSeconds)}s` : "--timeout 60s"; proc.stderr.write( ` ⚠ Shields auto-relocked${afterPart} — run \`${CLI_NAME} ${sandboxName} shields down ${timeoutSuggestion}\` to extend.\n`, ); } +function emitShieldsAuditUnreadableWarning( + proc: NonNullable, + sandboxName: string, +): void { + proc.stderr.write( + ` ⚠ Could not read shields audit history; continuing without relock context. Run \`${CLI_NAME} ${sandboxName} shields status\` to verify current state.\n`, + ); +} + function rejectNoTargetSelector(proc: NonNullable): never { proc.stderr.write( " No target session selected. Use --agent , --session-key , --session-id , or --to .\n", @@ -439,15 +478,16 @@ export async function runAgentPassthrough( if (isOpenClawPassthroughCommand(command) && !hasTargetSelector(extraArgs)) { rejectNoTargetSelector(proc); } - // 10-min window: shields timeouts range 1–1800s; 10 min covers even the max - // 30-min timeout with a 2× buffer. A longer window risks false-positive warnings - // on a relock from a prior session. Adjust if the upstream max changes. - const checkShields = - deps.getRecentShieldsAutoRestore ?? - ((name: string) => readRecentShieldsAutoRestore(name, 10 * 60 * 1000)); - const relock = checkShields(sandboxName); - if (relock) { - emitShieldsRelockWarning(proc, relock, sandboxName); + if (isOpenClawPassthroughCommand(command)) { + const checkShields = + deps.getRecentShieldsAutoRestore ?? + ((name: string) => readRecentShieldsAutoRestore(name, SHIELDS_RELOCK_WARNING_WINDOW_MS)); + const relock = checkShields(sandboxName); + if (relock.kind === "event") { + emitShieldsRelockWarning(proc, relock.event, sandboxName); + } else if (relock.kind === "unreadable") { + emitShieldsAuditUnreadableWarning(proc, sandboxName); + } } if (isOpenClawPassthroughCommand(command) && requestsOpenClawJsonOutput(extraArgs)) { const execJson = deps.execJson ?? runAgentJsonPassthrough; diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 9c91d64d71d..9dfbd47e6fe 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -1,11 +1,15 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { describe, it, expect, beforeEach, afterEach } from "vitest"; import fs from "node:fs"; -import { readRecentShieldsAutoRestore } from "./audit"; -import path from "node:path"; import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + readRecentShieldsAutoRestore, + type ShieldsAutoRestoreEvent, + type ShieldsAutoRestoreReadResult, +} from "./audit"; // Test the audit entry format and JSONL structure using the same logic // as the production module but with a controllable output path. @@ -34,6 +38,12 @@ function appendAuditEntry(entry: AuditRecord) { fs.appendFileSync(auditPath, JSON.stringify(entry) + "\n", { mode: 0o600 }); } +function requireEvent(result: ShieldsAutoRestoreReadResult): ShieldsAutoRestoreEvent { + expect(result.kind).toBe("event"); + if (result.kind !== "event") throw new Error(`expected event result, got ${result.kind}`); + return result.event; +} + describe("shields-audit", () => { it("creates file on first write and writes valid JSONL", () => { expect(fs.existsSync(auditPath)).toBe(false); @@ -133,9 +143,9 @@ describe("readRecentShieldsAutoRestore", () => { JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: now }) + "\n", ); - const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); - expect(result?.timestamp).toBe(now); - expect(result?.timeoutSeconds).toBe(20); + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp).toBe(now); + expect(event.timeoutSeconds).toBe(20); }); it("returns timestamp with null timeoutSeconds when no shields_down entry exists (#5922)", () => { @@ -144,12 +154,12 @@ describe("readRecentShieldsAutoRestore", () => { auditPath, JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: now }) + "\n", ); - const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); - expect(result?.timestamp).toBe(now); - expect(result?.timeoutSeconds).toBeNull(); + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp).toBe(now); + expect(event.timeoutSeconds).toBeNull(); }); - it("returns null when the shields_auto_restore entry is future-dated (#5922)", () => { + it("returns no event when the shields_auto_restore entry is future-dated (#5922)", () => { const future = new Date(Date.now() + 60 * 1000).toISOString(); fs.appendFileSync( auditPath, @@ -157,20 +167,20 @@ describe("readRecentShieldsAutoRestore", () => { "\n", ); const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); - expect(result).toBeNull(); + expect(result).toEqual({ kind: "none" }); }); - it("returns null when the most recent shields_auto_restore entry is older than the window (#5922)", () => { + it("returns no event when the most recent shields_auto_restore entry is older than the window (#5922)", () => { const old = new Date(Date.now() - 20 * 60 * 1000).toISOString(); fs.appendFileSync( auditPath, JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: old }) + "\n", ); const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); - expect(result).toBeNull(); + expect(result).toEqual({ kind: "none" }); }); - it("returns null when the recent shields_auto_restore entry is for a different sandbox (#5922)", () => { + it("returns no event when the recent shields_auto_restore entry is for a different sandbox (#5922)", () => { const now = new Date().toISOString(); fs.appendFileSync( auditPath, @@ -178,12 +188,12 @@ describe("readRecentShieldsAutoRestore", () => { "\n", ); const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); - expect(result).toBeNull(); + expect(result).toEqual({ kind: "none" }); }); - it("returns null when the audit file does not exist (#5922)", () => { + it("returns no event when the audit file does not exist (#5922)", () => { const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); - expect(result).toBeNull(); + expect(result).toEqual({ kind: "none" }); }); it("returns null timeoutSeconds for out-of-bounds timeout values in shields_down entry (#5922)", () => { @@ -202,9 +212,9 @@ describe("readRecentShieldsAutoRestore", () => { JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: now }) + "\n", ); - const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); - expect(result?.timestamp, `bad value ${String(bad)}`).toBe(now); - expect(result?.timeoutSeconds, `bad value ${String(bad)}`).toBeNull(); + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp, `bad value ${String(bad)}`).toBe(now); + expect(event.timeoutSeconds, `bad value ${String(bad)}`).toBeNull(); } }); @@ -225,9 +235,9 @@ describe("readRecentShieldsAutoRestore", () => { JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: now }) + "\n", ); - const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); - expect(result?.timestamp).toBe(now); - expect(result?.timeoutSeconds).toBe(30); + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp).toBe(now); + expect(event.timeoutSeconds).toBe(30); }); it("uses the immediately-preceding shields_down when multiple exist (#5922)", () => { @@ -253,9 +263,9 @@ describe("readRecentShieldsAutoRestore", () => { JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp: now }) + "\n", ); - const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); - expect(result?.timestamp).toBe(now); - expect(result?.timeoutSeconds).toBe(45); + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp).toBe(now); + expect(event.timeoutSeconds).toBe(45); }); it("returns null timeoutSeconds when shields_down has NaN or Infinity as a raw string payload (#5922)", () => { @@ -271,9 +281,69 @@ describe("readRecentShieldsAutoRestore", () => { }); fs.writeFileSync(auditPath, downLine + "\n" + restoreLine + "\n"); // Malformed JSON (NaN/Infinity are not valid JSON) → parseEntry returns null → timeoutSeconds stays null - const result = readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath); - expect(result?.timestamp, `raw value ${rawValue}`).toBe(now); - expect(result?.timeoutSeconds, `raw value ${rawValue}`).toBeNull(); + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp, `raw value ${rawValue}`).toBe(now); + expect(event.timeoutSeconds, `raw value ${rawValue}`).toBeNull(); } }); + + it("distinguishes unreadable audit history from an absent audit file (#5922)", () => { + fs.mkdirSync(auditPath); + expect(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)).toEqual({ + kind: "unreadable", + }); + }); + + it("skips invalid restore timestamps and finds the next valid recent event (#5922)", () => { + const validTimestamp = new Date(Date.now() - 1000).toISOString(); + fs.writeFileSync( + auditPath, + [ + JSON.stringify({ + action: "shields_auto_restore", + sandbox: "alpha", + timestamp: validTimestamp, + }), + JSON.stringify({ + action: "shields_auto_restore", + sandbox: "alpha", + timestamp: "not-a-timestamp", + }), + ].join("\n") + "\n", + ); + + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp).toBe(validTimestamp); + }); + + it("skips non-object JSONL tail rows while finding a valid restore event (#5922)", () => { + const timestamp = new Date().toISOString(); + fs.writeFileSync( + auditPath, + [ + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp }), + "null", + "[]", + '"text"', + "42", + ].join("\n") + "\n", + ); + + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp).toBe(timestamp); + }); + + it("reads only the bounded audit tail and discards its partial first line (#5922)", () => { + const timestamp = new Date().toISOString(); + fs.writeFileSync( + auditPath, + "x".repeat(1024 * 1024 + 100) + + "\n" + + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp }) + + "\n", + ); + + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp).toBe(timestamp); + }); }); diff --git a/src/lib/shields/audit.ts b/src/lib/shields/audit.ts index 29cc0cdae28..e7e2ff869c6 100644 --- a/src/lib/shields/audit.ts +++ b/src/lib/shields/audit.ts @@ -10,7 +10,7 @@ * Entries never contain credential values — only key names and policy labels. */ -import { appendFileSync, readFileSync } from "node:fs"; +import { appendFileSync, closeSync, fstatSync, openSync, readSync } from "node:fs"; import { join } from "node:path"; import { redactFull } from "../security/redact"; import { ensureConfigDir } from "../state/config-io"; @@ -66,21 +66,64 @@ export interface ShieldsAutoRestoreEvent { timeoutSeconds: number | null; } +export type ShieldsAutoRestoreReadResult = + | { kind: "event"; event: ShieldsAutoRestoreEvent } + | { kind: "none" } + | { kind: "unreadable" }; + +const MAX_RECENT_AUDIT_BYTES = 1024 * 1024; + +function readAuditTail(auditFile: string): string { + const fd = openSync(auditFile, "r"); + try { + const size = fstatSync(fd).size; + const bytesToRead = Math.min(size, MAX_RECENT_AUDIT_BYTES); + if (bytesToRead === 0) return ""; + + const offset = size - bytesToRead; + const buffer = Buffer.alloc(bytesToRead); + let bytesRead = 0; + while (bytesRead < bytesToRead) { + const count = readSync(fd, buffer, bytesRead, bytesToRead - bytesRead, offset + bytesRead); + if (count === 0) break; + bytesRead += count; + } + const content = buffer.subarray(0, bytesRead).toString("utf8"); + if (offset === 0) return content; + + // The bounded read can begin in the middle of a JSONL entry. Drop that + // partial first line and retain only complete entries from the tail. + const firstNewline = content.indexOf("\n"); + return firstNewline === -1 ? "" : content.slice(firstNewline + 1); + } finally { + closeSync(fd); + } +} + /** * Scan the audit log in reverse and return details about the most recent * `shields_auto_restore` event for the given sandbox that falls within * `withinMs` milliseconds of now. Also reads the preceding `shields_down` * entry to recover the original timeout so callers can echo it back. * - * Returns null when no matching entry is found OR when the file is unreadable. - * Fail-open is intentional: blocking agent dispatch on audit I/O errors (e.g. - * EACCES, EIO) would be a DoS vector — callers must treat null as "no warning" - * rather than "no event." Future-dated entries are rejected as a clock-skew - * defense so a crafted entry cannot pin the warning permanently. + * This log is non-authoritative UX input. Missing, corrupt, or locally tampered + * rows must never make policy or current shield-state decisions; callers use an + * `event` result only to explain a likely relock. Current state is queried by + * the shields commands themselves. + * + * Missing files return `none`. Other read failures return `unreadable` so the + * caller can surface degraded audit visibility while still dispatching the + * agent. Fail-open is intentional: blocking dispatch on EACCES/EIO would turn + * this advisory check into a denial-of-service boundary. Future-dated entries + * are rejected so a crafted row cannot pin the warning permanently. * - * File-size note: the audit file is user-owned and written only by NemoClaw at - * ~200 bytes per entry. An unbounded readFileSync is acceptable for this - * warning-only path; add a size cap if the audit log gains a rotation policy. + * Only the last 1 MiB is read. This bounds synchronous work on the dispatch + * path while retaining thousands of normal audit entries; if the matching + * `shields_down` row falls outside that tail, the event remains useful with a + * null timeout and the caller uses its safe fallback suggestion. + * + * Remove this reader when OpenClaw exposes a structured relock cause or when + * extend-on-activity removes the mid-session relock condition. * * The optional `auditFile` parameter overrides the default path; used in tests. */ @@ -88,12 +131,15 @@ export function readRecentShieldsAutoRestore( sandboxName: string, withinMs: number, auditFile: string = AUDIT_FILE, -): ShieldsAutoRestoreEvent | null { +): ShieldsAutoRestoreReadResult { + if (!Number.isFinite(withinMs) || withinMs <= 0) return { kind: "none" }; + let content: string; try { - content = readFileSync(auditFile, "utf8"); - } catch { - return null; + content = readAuditTail(auditFile); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return { kind: "none" }; + return { kind: "unreadable" }; } const cutoff = Date.now() - withinMs; const lines = content.split("\n"); @@ -144,9 +190,9 @@ export function readRecentShieldsAutoRestore( break; } } - return { timestamp: restoreTs, timeoutSeconds }; + return { kind: "event", event: { timestamp: restoreTs, timeoutSeconds } }; } - return null; + return { kind: "none" }; } -export { AUDIT_FILE, AUDIT_DIR }; +export { AUDIT_DIR, AUDIT_FILE }; From 64a1d566ad0aa53631bec8e2643ac3fbcffc6351 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Tue, 30 Jun 2026 01:06:15 -0700 Subject: [PATCH 08/14] test(sandbox): keep shields audit assertions linear Signed-off-by: Carlos Villela --- src/lib/shields/audit.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 9dfbd47e6fe..51a68552029 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -40,8 +40,7 @@ function appendAuditEntry(entry: AuditRecord) { function requireEvent(result: ShieldsAutoRestoreReadResult): ShieldsAutoRestoreEvent { expect(result.kind).toBe("event"); - if (result.kind !== "event") throw new Error(`expected event result, got ${result.kind}`); - return result.event; + return (result as Extract).event; } describe("shields-audit", () => { From fd2554392bba70ac1711fb6891d88a3b925c551e Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Tue, 30 Jun 2026 01:15:55 -0700 Subject: [PATCH 09/14] fix(sandbox): quote relock recovery commands Signed-off-by: Carlos Villela --- .../agent/passthrough-shields-warning.test.ts | 19 +++++- .../agent/passthrough-shields-warning.ts | 63 +++++++++++++++++++ src/lib/actions/sandbox/agent/passthrough.ts | 53 +--------------- 3 files changed, 83 insertions(+), 52 deletions(-) create mode 100644 src/lib/actions/sandbox/agent/passthrough-shields-warning.ts diff --git a/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts b/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts index 0b9b71ec0f6..d45396c9ffe 100644 --- a/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts @@ -55,11 +55,14 @@ describe("runAgentPassthrough shields-relock warning", () => { vi.clearAllMocks(); }); - async function runWarning(result: ShieldsAutoRestoreReadResult): Promise { + async function runWarning( + result: ShieldsAutoRestoreReadResult, + sandboxName = "alpha", + ): Promise { getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); const { writes, proc } = makeProcMock(); await runAgentPassthrough( - "alpha", + sandboxName, { extraArgs: ["--agent", "main", "-m", "hi"] }, { process: proc, getRecentShieldsAutoRestore: () => result }, ); @@ -95,6 +98,18 @@ describe("runAgentPassthrough shields-relock warning", () => { expect(output).toMatch(/shields down --timeout 60s/); }); + it("shell-quotes sandbox names in recovery command suggestions (#5922)", async () => { + const output = await runWarning( + { + kind: "event", + event: { timestamp: new Date().toISOString(), timeoutSeconds: 20 }, + }, + "alpha; touch /tmp/pwn", + ); + expect(output).toContain("nemoclaw 'alpha; touch /tmp/pwn' shields down --timeout 20s"); + expect(output).not.toContain("nemoclaw alpha; touch /tmp/pwn"); + }); + it("emits no relock warning when the audit has no recent event (#5922)", async () => { const output = await runWarning({ kind: "none" }); expect(execMock).toHaveBeenCalled(); diff --git a/src/lib/actions/sandbox/agent/passthrough-shields-warning.ts b/src/lib/actions/sandbox/agent/passthrough-shields-warning.ts new file mode 100644 index 00000000000..003643b9726 --- /dev/null +++ b/src/lib/actions/sandbox/agent/passthrough-shields-warning.ts @@ -0,0 +1,63 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { CLI_NAME } from "../../../cli/branding"; +import { shellQuote } from "../../../core/shell-quote"; +import { + readRecentShieldsAutoRestore, + type ShieldsAutoRestoreEvent, + type ShieldsAutoRestoreReadResult, +} from "../../../shields/audit"; + +// A relock remains useful context briefly after it happens. This is a +// relevance window measured from the restore event, independent of the +// original shields-down timeout; a longer window risks stale-session warnings. +const SHIELDS_RELOCK_WARNING_WINDOW_MS = 10 * 60 * 1000; + +type ShieldsWarningProcess = { + stderr: { write(value: string): unknown }; +}; + +type RecentShieldsAutoRestoreReader = (sandboxName: string) => ShieldsAutoRestoreReadResult; + +function emitShieldsRelockWarning( + proc: ShieldsWarningProcess, + relock: ShieldsAutoRestoreEvent, + sandboxName: string, +): void { + // Defend the user-facing command suggestion even when tests or future + // callers inject an event without going through the audit reader. + const timeoutSeconds = + relock.timeoutSeconds !== null && + Number.isInteger(relock.timeoutSeconds) && + relock.timeoutSeconds >= 1 && + relock.timeoutSeconds <= 1800 + ? relock.timeoutSeconds + : null; + const afterPart = timeoutSeconds !== null ? ` after ${String(timeoutSeconds)}s` : ""; + const timeoutSuggestion = + timeoutSeconds !== null ? `--timeout ${String(timeoutSeconds)}s` : "--timeout 60s"; + proc.stderr.write( + ` ⚠ Shields auto-relocked${afterPart} — run \`${CLI_NAME} ${shellQuote(sandboxName)} shields down ${timeoutSuggestion}\` to extend.\n`, + ); +} + +function emitShieldsAuditUnreadableWarning(proc: ShieldsWarningProcess, sandboxName: string): void { + proc.stderr.write( + ` ⚠ Could not read shields audit history; continuing without relock context. Run \`${CLI_NAME} ${shellQuote(sandboxName)} shields status\` to verify current state.\n`, + ); +} + +export function maybeEmitShieldsRelockWarning( + proc: ShieldsWarningProcess, + sandboxName: string, + getRecentShieldsAutoRestore: RecentShieldsAutoRestoreReader = (name) => + readRecentShieldsAutoRestore(name, SHIELDS_RELOCK_WARNING_WINDOW_MS), +): void { + const relock = getRecentShieldsAutoRestore(sandboxName); + if (relock.kind === "event") { + emitShieldsRelockWarning(proc, relock.event, sandboxName); + } else if (relock.kind === "unreadable") { + emitShieldsAuditUnreadableWarning(proc, sandboxName); + } +} diff --git a/src/lib/actions/sandbox/agent/passthrough.ts b/src/lib/actions/sandbox/agent/passthrough.ts index 0e1a0ad0872..6a9e072e816 100644 --- a/src/lib/actions/sandbox/agent/passthrough.ts +++ b/src/lib/actions/sandbox/agent/passthrough.ts @@ -111,17 +111,14 @@ import { type AgentDefinition, isTerminalAgent, listAgents, loadAgent } from "../../../agent/defs"; import { CLI_NAME } from "../../../cli/branding"; -import { - readRecentShieldsAutoRestore, - type ShieldsAutoRestoreEvent, - type ShieldsAutoRestoreReadResult, -} from "../../../shields/audit"; +import type { ShieldsAutoRestoreReadResult } from "../../../shields/audit"; import { parseSandboxPhase } from "../../../state/gateway"; import * as registry from "../../../state/registry"; import { execSandbox } from "../exec"; import { ensureLiveSandboxOrExit } from "../gateway-state"; import { hasAgentPassthroughHelpToken, printAgentPassthroughHelp } from "./passthrough-help"; import { type AgentJsonPassthroughProcess, runAgentJsonPassthrough } from "./passthrough-json"; +import { maybeEmitShieldsRelockWarning } from "./passthrough-shields-warning"; export { hasAgentPassthroughHelpToken, @@ -145,11 +142,6 @@ const OPENCLAW_AGENT_VALUE_FLAGS = new Set([ const OPENCLAW_AGENT_BOOLEAN_FLAGS = new Set(["--deliver"]); -// A relock remains useful context briefly after it happens. This is a -// relevance window measured from the restore event, independent of the -// original shields-down timeout; a longer window risks stale-session warnings. -const SHIELDS_RELOCK_WARNING_WINDOW_MS = 10 * 60 * 1000; - export interface AgentPassthroughOptions { extraArgs?: readonly string[]; } @@ -378,37 +370,6 @@ function hasTargetSelector(args: readonly string[]): boolean { return false; } -function emitShieldsRelockWarning( - proc: NonNullable, - relock: ShieldsAutoRestoreEvent, - sandboxName: string, -): void { - // Defend the user-facing command suggestion even when tests or future - // callers inject an event without going through the audit reader. - const timeoutSeconds = - relock.timeoutSeconds !== null && - Number.isInteger(relock.timeoutSeconds) && - relock.timeoutSeconds >= 1 && - relock.timeoutSeconds <= 1800 - ? relock.timeoutSeconds - : null; - const afterPart = timeoutSeconds !== null ? ` after ${String(timeoutSeconds)}s` : ""; - const timeoutSuggestion = - timeoutSeconds !== null ? `--timeout ${String(timeoutSeconds)}s` : "--timeout 60s"; - proc.stderr.write( - ` ⚠ Shields auto-relocked${afterPart} — run \`${CLI_NAME} ${sandboxName} shields down ${timeoutSuggestion}\` to extend.\n`, - ); -} - -function emitShieldsAuditUnreadableWarning( - proc: NonNullable, - sandboxName: string, -): void { - proc.stderr.write( - ` ⚠ Could not read shields audit history; continuing without relock context. Run \`${CLI_NAME} ${sandboxName} shields status\` to verify current state.\n`, - ); -} - function rejectNoTargetSelector(proc: NonNullable): never { proc.stderr.write( " No target session selected. Use --agent , --session-key , --session-id , or --to .\n", @@ -479,15 +440,7 @@ export async function runAgentPassthrough( rejectNoTargetSelector(proc); } if (isOpenClawPassthroughCommand(command)) { - const checkShields = - deps.getRecentShieldsAutoRestore ?? - ((name: string) => readRecentShieldsAutoRestore(name, SHIELDS_RELOCK_WARNING_WINDOW_MS)); - const relock = checkShields(sandboxName); - if (relock.kind === "event") { - emitShieldsRelockWarning(proc, relock.event, sandboxName); - } else if (relock.kind === "unreadable") { - emitShieldsAuditUnreadableWarning(proc, sandboxName); - } + maybeEmitShieldsRelockWarning(proc, sandboxName, deps.getRecentShieldsAutoRestore); } if (isOpenClawPassthroughCommand(command) && requestsOpenClawJsonOutput(extraArgs)) { const execJson = deps.execJson ?? runAgentJsonPassthrough; From bc1d8d0d1c493adb70e2c66608c79432efafc45e Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Tue, 30 Jun 2026 01:21:46 -0700 Subject: [PATCH 10/14] docs(sandbox): clarify relock audit writers Signed-off-by: Carlos Villela --- src/lib/actions/sandbox/agent/passthrough.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/lib/actions/sandbox/agent/passthrough.ts b/src/lib/actions/sandbox/agent/passthrough.ts index 6a9e072e816..b887e7d3d2b 100644 --- a/src/lib/actions/sandbox/agent/passthrough.ts +++ b/src/lib/actions/sandbox/agent/passthrough.ts @@ -107,7 +107,8 @@ // directly (e.g., a distinct exit code or structured error field for // missing-scope-after-relock), or when NemoClaw implements // extend-on-activity so the scope never lapses mid-session. -// (The shields_auto_restore audit entry is appended by timer.ts:295–302.) +// (shields_auto_restore audit entries are written by the shields timer +// and inline expired-timer recovery paths.) import { type AgentDefinition, isTerminalAgent, listAgents, loadAgent } from "../../../agent/defs"; import { CLI_NAME } from "../../../cli/branding"; From 70fe8c53c1179fe6526c0bb9db8700d224f9fe3f Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Tue, 30 Jun 2026 01:29:29 -0700 Subject: [PATCH 11/14] fix(shields): validate relock audit chronology Signed-off-by: Carlos Villela --- .../agent/passthrough-shields-warning.test.ts | 62 ++++++++++++++++++- .../actions/sandbox/agent/passthrough.test.ts | 4 +- src/lib/shields/audit.test.ts | 32 ++++++++++ src/lib/shields/audit.ts | 7 ++- 4 files changed, 101 insertions(+), 4 deletions(-) diff --git a/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts b/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts index d45396c9ffe..844590d7fd7 100644 --- a/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts @@ -1,6 +1,10 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + import { beforeEach, describe, expect, it, vi } from "vitest"; import type { ShieldsAutoRestoreReadResult } from "../../../shields/audit"; @@ -35,7 +39,7 @@ vi.mock("../../../shields/audit", () => ({ readRecentShieldsAutoRestore: vi.fn(() => ({ kind: "none" })), })); -import { runAgentPassthrough } from "./passthrough"; +import { type AgentPassthroughDeps, runAgentPassthrough } from "./passthrough"; function makeProcMock() { const writes: string[] = []; @@ -110,6 +114,62 @@ describe("runAgentPassthrough shields-relock warning", () => { expect(output).not.toContain("nemoclaw alpha; touch /tmp/pwn"); }); + it("keeps JSON stdout parseable while warning from a real audit file on stderr (#5922)", async () => { + const actualAudit = + await vi.importActual("../../../shields/audit"); + const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-shields-warning-")); + const auditPath = path.join(tempDir, "shields-audit.jsonl"); + const restoreTimestamp = new Date().toISOString(); + const stdoutWrites: string[] = []; + const { writes, proc } = makeProcMock(); + const processWithStdout = { + ...proc, + stdout: { write: (value: string) => stdoutWrites.push(value) }, + }; + const execJson = vi.fn(((_sandboxName, _command, jsonProc): never => { + jsonProc?.stdout.write('{"ok":true}\n'); + throw new Error("__json-exit:0"); + }) as NonNullable); + + try { + fs.writeFileSync( + auditPath, + [ + JSON.stringify({ + action: "shields_down", + sandbox: "alpha", + timestamp: new Date(Date.now() - 20 * 1000).toISOString(), + timeout_seconds: 20, + }), + JSON.stringify({ + action: "shields_auto_restore", + sandbox: "alpha", + timestamp: restoreTimestamp, + }), + ].join("\n") + "\n", + ); + + await expect( + runAgentPassthrough( + "alpha", + { extraArgs: ["--agent", "main", "-m", "hi", "--json"] }, + { + process: processWithStdout, + execJson, + getRecentShieldsAutoRestore: (sandboxName) => + actualAudit.readRecentShieldsAutoRestore(sandboxName, 10 * 60 * 1000, auditPath), + }, + ), + ).rejects.toThrow("__json-exit:0"); + + expect(JSON.parse(stdoutWrites.join(""))).toEqual({ ok: true }); + expect(writes.join("")).toMatch(/Shields auto-relocked after 20s/); + expect(execJson).toHaveBeenCalledOnce(); + } finally { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + }); + it("emits no relock warning when the audit has no recent event (#5922)", async () => { const output = await runWarning({ kind: "none" }); expect(execMock).toHaveBeenCalled(); diff --git a/src/lib/actions/sandbox/agent/passthrough.test.ts b/src/lib/actions/sandbox/agent/passthrough.test.ts index 1cd877d4ddc..ab3a8d370a1 100644 --- a/src/lib/actions/sandbox/agent/passthrough.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough.test.ts @@ -160,14 +160,14 @@ describe("runAgentPassthrough", () => { await runAgentPassthrough( "alpha", - { extraArgs: ["--agent", "work", "--some-future-value-flag", "--json"] }, + { extraArgs: ["--agent", "work", "--json-output", "--json"] }, { execJson }, ); expect(execJson).not.toHaveBeenCalled(); expect(execMock).toHaveBeenCalledWith( "alpha", - ["openclaw", "agent", "--agent", "work", "--some-future-value-flag", "--json"], + ["openclaw", "agent", "--agent", "work", "--json-output", "--json"], { tty: false }, ); }); diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 51a68552029..6fc02673281 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -267,6 +267,30 @@ describe("readRecentShieldsAutoRestore", () => { expect(event.timeoutSeconds).toBe(45); }); + it("rejects a shields_down timeout timestamped after its auto-restore (#5922)", () => { + const restoreTimestamp = new Date().toISOString(); + fs.writeFileSync( + auditPath, + JSON.stringify({ + action: "shields_down", + sandbox: "alpha", + timestamp: new Date(Date.now() + 60 * 1000).toISOString(), + timeout_seconds: 45, + }) + + "\n" + + JSON.stringify({ + action: "shields_auto_restore", + sandbox: "alpha", + timestamp: restoreTimestamp, + }) + + "\n", + ); + + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp).toBe(restoreTimestamp); + expect(event.timeoutSeconds).toBeNull(); + }); + it("returns null timeoutSeconds when shields_down has NaN or Infinity as a raw string payload (#5922)", () => { // JSON.stringify(NaN) and JSON.stringify(Infinity) both produce "null", // so write the JSONL line manually to exercise the non-finite path. @@ -345,4 +369,12 @@ describe("readRecentShieldsAutoRestore", () => { const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); expect(event.timestamp).toBe(timestamp); }); + + it("returns no event when the bounded tail has no complete JSONL entry (#5922)", () => { + fs.writeFileSync(auditPath, "x".repeat(1024 * 1024 + 100)); + + expect(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)).toEqual({ + kind: "none", + }); + }); }); diff --git a/src/lib/shields/audit.ts b/src/lib/shields/audit.ts index e7e2ff869c6..a0de03cd068 100644 --- a/src/lib/shields/audit.ts +++ b/src/lib/shields/audit.ts @@ -92,7 +92,8 @@ function readAuditTail(auditFile: string): string { if (offset === 0) return content; // The bounded read can begin in the middle of a JSONL entry. Drop that - // partial first line and retain only complete entries from the tail. + // partial first line and retain only complete entries from the tail. If + // there is no newline, the tail contains no complete JSONL entry. const firstNewline = content.indexOf("\n"); return firstNewline === -1 ? "" : content.slice(firstNewline + 1); } finally { @@ -178,7 +179,11 @@ export function readRecentShieldsAutoRestore( for (let j = i - 1; j >= 0; j--) { const prev = parseEntry(lines[j]); if (prev?.action === "shields_down" && prev.sandbox === sandboxName) { + const downMs = + typeof prev.timestamp === "string" ? new Date(prev.timestamp).getTime() : Number.NaN; if ( + Number.isFinite(downMs) && + downMs <= restoreMs && typeof prev.timeout_seconds === "number" && Number.isFinite(prev.timeout_seconds) && Number.isInteger(prev.timeout_seconds) && From 265236412441f4221e595c1fceefb3b4ae6f8eb1 Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Tue, 30 Jun 2026 01:42:21 -0700 Subject: [PATCH 12/14] fix(shields): surface oversized audit entries Signed-off-by: Carlos Villela --- .../sandbox/agent/passthrough-json.test.ts | 2 +- .../actions/sandbox/agent/passthrough.test.ts | 5 +++-- src/lib/actions/sandbox/agent/passthrough.ts | 8 +++++--- src/lib/shields/audit.test.ts | 4 ++-- src/lib/shields/audit.ts | 18 ++++++++++++------ 5 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/lib/actions/sandbox/agent/passthrough-json.test.ts b/src/lib/actions/sandbox/agent/passthrough-json.test.ts index be9e4931d40..a361720c181 100644 --- a/src/lib/actions/sandbox/agent/passthrough-json.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough-json.test.ts @@ -149,7 +149,7 @@ describe("runAgentJsonPassthrough", () => { runAgentJsonPassthrough("alpha", ["openclaw", "agent", "--json"], proc, { getOpenshellBinary: () => "/usr/local/bin/openshell", provenanceLines: () => { - throw new RangeError("Maximum call stack size exceeded"); + throw new SyntaxError("Unexpected token in OpenClaw JSON output"); }, spawnSync, }), diff --git a/src/lib/actions/sandbox/agent/passthrough.test.ts b/src/lib/actions/sandbox/agent/passthrough.test.ts index ab3a8d370a1..50ee249844d 100644 --- a/src/lib/actions/sandbox/agent/passthrough.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough.test.ts @@ -158,16 +158,17 @@ describe("runAgentPassthrough", () => { }) as NonNullable); getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); + // Unknown --json-* flags stay conservative until added to the documented value-flag set. await runAgentPassthrough( "alpha", - { extraArgs: ["--agent", "work", "--json-output", "--json"] }, + { extraArgs: ["--agent", "work", "--json-something", "--json"] }, { execJson }, ); expect(execJson).not.toHaveBeenCalled(); expect(execMock).toHaveBeenCalledWith( "alpha", - ["openclaw", "agent", "--agent", "work", "--json-output", "--json"], + ["openclaw", "agent", "--agent", "work", "--json-something", "--json"], { tty: false }, ); }); diff --git a/src/lib/actions/sandbox/agent/passthrough.ts b/src/lib/actions/sandbox/agent/passthrough.ts index b887e7d3d2b..8dc75d9585c 100644 --- a/src/lib/actions/sandbox/agent/passthrough.ts +++ b/src/lib/actions/sandbox/agent/passthrough.ts @@ -325,9 +325,11 @@ function requestsOpenClawJsonOutput(extraArgs: readonly string[]): boolean { // a value by another OpenClaw option. Source boundary: upstream OpenClaw owns // the complete argv grammar; NemoClaw mirrors documented flags only to choose // the host transport path. Unknown options fail conservative to normal - // passthrough, where OpenClaw parses argv itself. Regression tests cover each - // documented value flag, documented equals-form value flags, documented - // boolean flags, unknown flag fallback, and the `--` terminator. Removal + // passthrough, where OpenClaw parses argv itself. Any newly documented value + // flag, including a `--json-*` name, must be added to the value-flag set and + // its tests together. Regression tests cover each documented value flag, + // documented equals-form value flags, documented boolean flags, unknown flag + // fallback, and the `--` terminator. Removal // condition: OpenClaw exposes a machine-readable argv schema or NemoClaw stops // special-casing the JSON transport path. let skipNextValue = false; diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 6fc02673281..2c2bb1edcfe 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -370,11 +370,11 @@ describe("readRecentShieldsAutoRestore", () => { expect(event.timestamp).toBe(timestamp); }); - it("returns no event when the bounded tail has no complete JSONL entry (#5922)", () => { + it("reports an oversized unterminated JSONL entry as unreadable (#5922)", () => { fs.writeFileSync(auditPath, "x".repeat(1024 * 1024 + 100)); expect(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)).toEqual({ - kind: "none", + kind: "unreadable", }); }); }); diff --git a/src/lib/shields/audit.ts b/src/lib/shields/audit.ts index a0de03cd068..1629e0794e6 100644 --- a/src/lib/shields/audit.ts +++ b/src/lib/shields/audit.ts @@ -93,9 +93,12 @@ function readAuditTail(auditFile: string): string { // The bounded read can begin in the middle of a JSONL entry. Drop that // partial first line and retain only complete entries from the tail. If - // there is no newline, the tail contains no complete JSONL entry. + // there is no newline, an oversized unterminated entry has consumed the + // entire tail; report degraded visibility instead of treating it as an + // empty audit log. const firstNewline = content.indexOf("\n"); - return firstNewline === -1 ? "" : content.slice(firstNewline + 1); + if (firstNewline === -1) throw new Error("audit JSONL entry exceeds bounded tail"); + return content.slice(firstNewline + 1); } finally { closeSync(fd); } @@ -118,10 +121,13 @@ function readAuditTail(auditFile: string): string { * this advisory check into a denial-of-service boundary. Future-dated entries * are rejected so a crafted row cannot pin the warning permanently. * - * Only the last 1 MiB is read. This bounds synchronous work on the dispatch - * path while retaining thousands of normal audit entries; if the matching - * `shields_down` row falls outside that tail, the event remains useful with a - * null timeout and the caller uses its safe fallback suggestion. + * Only the last 1 MiB is read. This intentionally keeps the one-shot CLI read + * synchronous so the warning is ordered before dispatch while bounding the + * work to thousands of normal audit entries. If the matching `shields_down` + * row falls outside that tail, the event remains useful with a null timeout + * and the caller uses its safe fallback suggestion. Revisit the synchronous + * API if audit storage moves off the local filesystem or into a long-lived + * process. * * Remove this reader when OpenClaw exposes a structured relock cause or when * extend-on-activity removes the mid-session relock condition. From 6341d03852ad64eda3ead0f863139c78bba2966f Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Tue, 30 Jun 2026 01:57:40 -0700 Subject: [PATCH 13/14] fix(shields): suppress stale relock warnings Signed-off-by: Carlos Villela --- src/lib/actions/sandbox/agent/passthrough.ts | 12 +++++--- src/lib/shields/audit.test.ts | 30 ++++++++++++++++++++ src/lib/shields/audit.ts | 21 ++++++++++---- 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/src/lib/actions/sandbox/agent/passthrough.ts b/src/lib/actions/sandbox/agent/passthrough.ts index 8dc75d9585c..92f86adcb5d 100644 --- a/src/lib/actions/sandbox/agent/passthrough.ts +++ b/src/lib/actions/sandbox/agent/passthrough.ts @@ -72,11 +72,14 @@ // // - Invalid state: after shields auto-relock, the next host CLI // `openclaw agent` dispatch can fail with only `missing scope: -// operator.write`, which does not explain the recovery action. +// operator.write`, which does not explain the recovery action. An older +// relock warning is also stale once the user lowers shields again. // - Source boundary: OpenShell/OpenClaw own current scope state. NemoClaw's // local audit JSONL is non-authoritative and is used only to add likely -// relock context; unreadable history never blocks dispatch, and terminal -// runtimes are excluded from this OpenClaw-specific diagnostic. +// relock context. Validated audit chronology can suppress stale context +// but never establishes current policy state; unreadable history never +// blocks dispatch, and terminal runtimes are excluded from this +// OpenClaw-specific diagnostic. // - Source-fix constraint: an already-running in-sandbox OpenClaw TUI has no // host CLI interception point. Covering that surface requires an upstream // structured relock error or a separate extend-on-activity design. This @@ -91,7 +94,8 @@ // captured transport path used to append failure provenance without polluting // machine-readable stdout. `passthrough-shields-warning.test.ts` covers the // OpenClaw-only relock diagnostic, validated and fallback timeouts, unreadable -// and absent audit history, and terminal-runtime exclusion. +// and absent audit history, newer-down suppression, and terminal-runtime +// exclusion. // // Removal conditions: // diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit.test.ts index 2c2bb1edcfe..34f9ed41aa0 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit.test.ts @@ -291,6 +291,36 @@ describe("readRecentShieldsAutoRestore", () => { expect(event.timeoutSeconds).toBeNull(); }); + it("suppresses stale relock context after a newer shields_down (#5922)", () => { + const now = Date.now(); + fs.writeFileSync( + auditPath, + [ + JSON.stringify({ + action: "shields_down", + sandbox: "alpha", + timestamp: new Date(now - 30 * 1000).toISOString(), + timeout_seconds: 20, + }), + JSON.stringify({ + action: "shields_auto_restore", + sandbox: "alpha", + timestamp: new Date(now - 20 * 1000).toISOString(), + }), + JSON.stringify({ + action: "shields_down", + sandbox: "alpha", + timestamp: new Date(now - 10 * 1000).toISOString(), + timeout_seconds: 60, + }), + ].join("\n") + "\n", + ); + + expect(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)).toEqual({ + kind: "none", + }); + }); + it("returns null timeoutSeconds when shields_down has NaN or Infinity as a raw string payload (#5922)", () => { // JSON.stringify(NaN) and JSON.stringify(Infinity) both produce "null", // so write the JSONL line manually to exercise the non-finite path. diff --git a/src/lib/shields/audit.ts b/src/lib/shields/audit.ts index 1629e0794e6..7f70d0a8dc5 100644 --- a/src/lib/shields/audit.ts +++ b/src/lib/shields/audit.ts @@ -169,16 +169,27 @@ export function readRecentShieldsAutoRestore( const now = Date.now(); // Scan backwards for the most recent shields_auto_restore within the window. + // A later shields_down for the same sandbox makes older relock context stale, + // but remains advisory: it suppresses only this warning and never establishes + // current shield state. + let newerShieldsDownMs: number | null = null; for (let i = lines.length - 1; i >= 0; i--) { const entry = parseEntry(lines[i]); - if ( - entry?.action !== "shields_auto_restore" || - entry.sandbox !== sandboxName || - typeof entry.timestamp !== "string" - ) + if (entry?.sandbox !== sandboxName) continue; + if (entry.action === "shields_down") { + const downMs = + typeof entry.timestamp === "string" ? new Date(entry.timestamp).getTime() : Number.NaN; + if (Number.isFinite(downMs) && downMs <= now) { + newerShieldsDownMs = Math.max(newerShieldsDownMs ?? downMs, downMs); + } continue; + } + if (entry?.action !== "shields_auto_restore" || typeof entry.timestamp !== "string") continue; const restoreMs = new Date(entry.timestamp).getTime(); if (!Number.isFinite(restoreMs) || restoreMs < cutoff || restoreMs > now) continue; + if (newerShieldsDownMs !== null && newerShieldsDownMs >= restoreMs) { + return { kind: "none" }; + } const restoreTs = entry.timestamp; // Continue backwards to find the preceding shields_down to get timeout_seconds. let timeoutSeconds: number | null = null; From 7b20a64708ce62048e5e3b2fa942621324cb012a Mon Sep 17 00:00:00 2001 From: Carlos Villela Date: Tue, 30 Jun 2026 02:11:51 -0700 Subject: [PATCH 14/14] test(shields): split audit reader coverage Signed-off-by: Carlos Villela --- .../agent/passthrough-shields-warning.test.ts | 11 ++ .../agent/passthrough-shields-warning.ts | 24 ++++ .../actions/sandbox/agent/passthrough.test.ts | 4 +- src/lib/actions/sandbox/agent/passthrough.ts | 30 +---- src/lib/shields/audit-format.test.ts | 118 +++++++++++++++++ .../{audit.test.ts => audit-reader.test.ts} | 121 ++++-------------- src/lib/shields/audit.ts | 4 +- 7 files changed, 184 insertions(+), 128 deletions(-) create mode 100644 src/lib/shields/audit-format.test.ts rename src/lib/shields/{audit.test.ts => audit-reader.test.ts} (79%) diff --git a/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts b/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts index 844590d7fd7..397c133d6c7 100644 --- a/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough-shields-warning.test.ts @@ -114,6 +114,17 @@ describe("runAgentPassthrough shields-relock warning", () => { expect(output).not.toContain("nemoclaw alpha; touch /tmp/pwn"); }); + it("escapes embedded single quotes in recovery command suggestions (#5922)", async () => { + const output = await runWarning( + { + kind: "event", + event: { timestamp: new Date().toISOString(), timeoutSeconds: 20 }, + }, + "alpha'beta", + ); + expect(output).toContain("nemoclaw 'alpha'\\''beta' shields down --timeout 20s"); + }); + it("keeps JSON stdout parseable while warning from a real audit file on stderr (#5922)", async () => { const actualAudit = await vi.importActual("../../../shields/audit"); diff --git a/src/lib/actions/sandbox/agent/passthrough-shields-warning.ts b/src/lib/actions/sandbox/agent/passthrough-shields-warning.ts index 003643b9726..d16705ae5b6 100644 --- a/src/lib/actions/sandbox/agent/passthrough-shields-warning.ts +++ b/src/lib/actions/sandbox/agent/passthrough-shields-warning.ts @@ -9,6 +9,30 @@ import { type ShieldsAutoRestoreReadResult, } from "../../../shields/audit"; +// Source-of-truth boundary for the host CLI relock diagnostic: +// +// - Invalid state: after shields auto-relock, OpenClaw can report only +// `missing scope: operator.write`; an older relock warning also becomes stale +// after the user lowers shields again. +// - Source boundary: OpenShell/OpenClaw own current scope state. NemoClaw audit +// JSONL is non-authoritative context. Validated chronology may suppress stale +// context but never establishes current policy state, and unreadable history +// never blocks dispatch. The audit writers are the shields timer and inline +// expired-timer recovery paths. +// - Presentation boundary: sandbox names are user-controlled command text and +// must remain shell-quoted. Direct stderr output is deliberate so the warning +// is visible in a one-shot CLI while machine-readable stdout stays clean. +// - Source-fix constraint: an already-running in-sandbox TUI has no host CLI +// interception point. That surface needs an upstream structured relock error +// or a separate extend-on-activity design; this helper covers only host +// `nemoclaw agent` dispatches. +// - Regression tests cover validated/fallback timeouts, shell metacharacters +// and embedded quotes, real-file JSON stdout separation, unreadable/absent +// history, newer-down suppression, and terminal-runtime exclusion. +// - Removal condition: drop this diagnostic when OpenClaw exposes the relock +// cause directly or NemoClaw prevents mid-session relock by extending on +// activity. + // A relock remains useful context briefly after it happens. This is a // relevance window measured from the restore event, independent of the // original shields-down timeout; a longer window risks stale-session warnings. diff --git a/src/lib/actions/sandbox/agent/passthrough.test.ts b/src/lib/actions/sandbox/agent/passthrough.test.ts index 50ee249844d..de1bf2d267e 100644 --- a/src/lib/actions/sandbox/agent/passthrough.test.ts +++ b/src/lib/actions/sandbox/agent/passthrough.test.ts @@ -152,13 +152,13 @@ describe("runAgentPassthrough", () => { ); }); - it("keeps --json after an unknown future value flag on the normal passthrough path", async () => { + it("keeps --json-something --json on the normal passthrough path", async () => { const execJson = vi.fn(((): never => { throw new Error("__unexpected-json"); }) as NonNullable); getSandboxMock.mockReturnValueOnce({ agent: "openclaw" }); - // Unknown --json-* flags stay conservative until added to the documented value-flag set. + // The first unknown flag selects conservative passthrough before the later --json token. await runAgentPassthrough( "alpha", { extraArgs: ["--agent", "work", "--json-something", "--json"] }, diff --git a/src/lib/actions/sandbox/agent/passthrough.ts b/src/lib/actions/sandbox/agent/passthrough.ts index 92f86adcb5d..a9d77d9f963 100644 --- a/src/lib/actions/sandbox/agent/passthrough.ts +++ b/src/lib/actions/sandbox/agent/passthrough.ts @@ -68,22 +68,9 @@ // selector case is intercepted; everything else still flows through to // the in-sandbox binary. // -// 4. Recent shields-relock diagnostic (advisory audit mirror). -// -// - Invalid state: after shields auto-relock, the next host CLI -// `openclaw agent` dispatch can fail with only `missing scope: -// operator.write`, which does not explain the recovery action. An older -// relock warning is also stale once the user lowers shields again. -// - Source boundary: OpenShell/OpenClaw own current scope state. NemoClaw's -// local audit JSONL is non-authoritative and is used only to add likely -// relock context. Validated audit chronology can suppress stale context -// but never establishes current policy state; unreadable history never -// blocks dispatch, and terminal runtimes are excluded from this -// OpenClaw-specific diagnostic. -// - Source-fix constraint: an already-running in-sandbox OpenClaw TUI has no -// host CLI interception point. Covering that surface requires an upstream -// structured relock error or a separate extend-on-activity design. This -// wrapper intentionally covers only `nemoclaw agent` dispatches. +// 4. Recent shields-relock diagnostic (advisory audit mirror). Its complete +// source-boundary analysis lives with the focused implementation in +// `passthrough-shields-warning.ts`. // // Regression tests: `passthrough.test.ts` covers the Hermes redirect, the // forwarded argv, the registry-miss fallback to OpenClaw, registry and @@ -92,10 +79,7 @@ // unparseable phase fail-closed path, the OpenClaw no-selector rejection, and // the `--flag=value` selector-acceptance branch, plus the OpenClaw JSON // captured transport path used to append failure provenance without polluting -// machine-readable stdout. `passthrough-shields-warning.test.ts` covers the -// OpenClaw-only relock diagnostic, validated and fallback timeouts, unreadable -// and absent audit history, newer-down suppression, and terminal-runtime -// exclusion. +// machine-readable stdout. The focused shields diagnostic owns its tests. // // Removal conditions: // @@ -107,12 +91,6 @@ // missing selector with a clean exit 2 and an actionable message. // - Drop the simple-token parser when terminal runtime manifests expose // argv arrays natively. -// - Drop the shields-relock warning when OpenClaw exposes the relock cause -// directly (e.g., a distinct exit code or structured error field for -// missing-scope-after-relock), or when NemoClaw implements -// extend-on-activity so the scope never lapses mid-session. -// (shields_auto_restore audit entries are written by the shields timer -// and inline expired-timer recovery paths.) import { type AgentDefinition, isTerminalAgent, listAgents, loadAgent } from "../../../agent/defs"; import { CLI_NAME } from "../../../cli/branding"; diff --git a/src/lib/shields/audit-format.test.ts b/src/lib/shields/audit-format.test.ts new file mode 100644 index 00000000000..b0f3864a88c --- /dev/null +++ b/src/lib/shields/audit-format.test.ts @@ -0,0 +1,118 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +// Test the audit entry format and JSONL structure using the same logic +// as the production module but with a controllable output path. + +type AuditScalar = string | number | boolean | null | undefined; +type AuditValue = AuditScalar | AuditRecord | AuditValue[]; +type AuditRecord = { [key: string]: AuditValue }; + +let tmpDir: string; +let auditPath: string; + +beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "shields-audit-test-")); + auditPath = path.join(tmpDir, "shields-audit.jsonl"); +}); + +afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); +}); + +/** + * Inline audit append — mirrors the production appendAuditEntry() but writes + * to our test-controlled path instead of ~/.nemoclaw/state/. + */ +function appendAuditEntry(entry: AuditRecord) { + fs.appendFileSync(auditPath, JSON.stringify(entry) + "\n", { mode: 0o600 }); +} + +describe("shields-audit format", () => { + it("creates file on first write and writes valid JSONL", () => { + expect(fs.existsSync(auditPath)).toBe(false); + + appendAuditEntry({ + action: "shields_down", + sandbox: "openclaw", + timestamp: "2026-04-13T14:30:00Z", + timeout_seconds: 300, + reason: "Installing Slack plugin", + policy_applied: "permissive", + policy_snapshot: "/tmp/snapshot.yaml", + }); + + expect(fs.existsSync(auditPath)).toBe(true); + const content = fs.readFileSync(auditPath, "utf-8"); + const lines = content.trim().split("\n"); + expect(lines).toHaveLength(1); + + const entry = JSON.parse(lines[0]); + expect(entry.action).toBe("shields_down"); + expect(entry.sandbox).toBe("openclaw"); + expect(entry.timeout_seconds).toBe(300); + }); + + it("appends multiple entries as separate lines", () => { + appendAuditEntry({ + action: "shields_down", + sandbox: "openclaw", + timestamp: "2026-04-13T14:30:00Z", + }); + + appendAuditEntry({ + action: "shields_up", + sandbox: "openclaw", + timestamp: "2026-04-13T14:32:00Z", + restored_by: "operator", + duration_seconds: 120, + }); + + const lines = fs.readFileSync(auditPath, "utf-8").trim().split("\n"); + expect(lines).toHaveLength(2); + + const second = JSON.parse(lines[1]); + expect(second.action).toBe("shields_up"); + expect(second.restored_by).toBe("operator"); + expect(second.duration_seconds).toBe(120); + }); + + it("each line is valid JSON", () => { + for (let i = 0; i < 5; i++) { + appendAuditEntry({ + action: "shields_down", + sandbox: `sandbox-${i}`, + timestamp: new Date().toISOString(), + }); + } + + const lines = fs.readFileSync(auditPath, "utf-8").trim().split("\n"); + expect(lines).toHaveLength(5); + + for (const line of lines) { + expect(() => JSON.parse(line)).not.toThrow(); + } + }); + + it("never includes credential-like values in entries", () => { + const entry = { + action: "shields_down", + sandbox: "openclaw", + timestamp: "2026-04-13T14:30:00Z", + reason: "Installing plugin", + policy_applied: "permissive", + }; + + appendAuditEntry(entry); + + const line = fs.readFileSync(auditPath, "utf-8").trim(); + expect(line).not.toContain("nvapi-"); + expect(line).not.toContain("ghp_"); + expect(line).not.toContain("sk-"); + }); +}); diff --git a/src/lib/shields/audit.test.ts b/src/lib/shields/audit-reader.test.ts similarity index 79% rename from src/lib/shields/audit.test.ts rename to src/lib/shields/audit-reader.test.ts index 34f9ed41aa0..5f9acf26cea 100644 --- a/src/lib/shields/audit.test.ts +++ b/src/lib/shields/audit-reader.test.ts @@ -11,13 +11,6 @@ import { type ShieldsAutoRestoreReadResult, } from "./audit"; -// Test the audit entry format and JSONL structure using the same logic -// as the production module but with a controllable output path. - -type AuditScalar = string | number | boolean | null | undefined; -type AuditValue = AuditScalar | AuditRecord | AuditValue[]; -type AuditRecord = { [key: string]: AuditValue }; - let tmpDir: string; let auditPath: string; @@ -30,103 +23,11 @@ afterEach(() => { fs.rmSync(tmpDir, { recursive: true, force: true }); }); -/** - * Inline audit append — mirrors the production appendAuditEntry() but writes - * to our test-controlled path instead of ~/.nemoclaw/state/. - */ -function appendAuditEntry(entry: AuditRecord) { - fs.appendFileSync(auditPath, JSON.stringify(entry) + "\n", { mode: 0o600 }); -} - function requireEvent(result: ShieldsAutoRestoreReadResult): ShieldsAutoRestoreEvent { expect(result.kind).toBe("event"); return (result as Extract).event; } -describe("shields-audit", () => { - it("creates file on first write and writes valid JSONL", () => { - expect(fs.existsSync(auditPath)).toBe(false); - - appendAuditEntry({ - action: "shields_down", - sandbox: "openclaw", - timestamp: "2026-04-13T14:30:00Z", - timeout_seconds: 300, - reason: "Installing Slack plugin", - policy_applied: "permissive", - policy_snapshot: "/tmp/snapshot.yaml", - }); - - expect(fs.existsSync(auditPath)).toBe(true); - const content = fs.readFileSync(auditPath, "utf-8"); - const lines = content.trim().split("\n"); - expect(lines).toHaveLength(1); - - const entry = JSON.parse(lines[0]); - expect(entry.action).toBe("shields_down"); - expect(entry.sandbox).toBe("openclaw"); - expect(entry.timeout_seconds).toBe(300); - }); - - it("appends multiple entries as separate lines", () => { - appendAuditEntry({ - action: "shields_down", - sandbox: "openclaw", - timestamp: "2026-04-13T14:30:00Z", - }); - - appendAuditEntry({ - action: "shields_up", - sandbox: "openclaw", - timestamp: "2026-04-13T14:32:00Z", - restored_by: "operator", - duration_seconds: 120, - }); - - const lines = fs.readFileSync(auditPath, "utf-8").trim().split("\n"); - expect(lines).toHaveLength(2); - - const second = JSON.parse(lines[1]); - expect(second.action).toBe("shields_up"); - expect(second.restored_by).toBe("operator"); - expect(second.duration_seconds).toBe(120); - }); - - it("each line is valid JSON", () => { - for (let i = 0; i < 5; i++) { - appendAuditEntry({ - action: "shields_down", - sandbox: `sandbox-${i}`, - timestamp: new Date().toISOString(), - }); - } - - const lines = fs.readFileSync(auditPath, "utf-8").trim().split("\n"); - expect(lines).toHaveLength(5); - - for (const line of lines) { - expect(() => JSON.parse(line)).not.toThrow(); - } - }); - - it("never includes credential-like values in entries", () => { - const entry = { - action: "shields_down", - sandbox: "openclaw", - timestamp: "2026-04-13T14:30:00Z", - reason: "Installing plugin", - policy_applied: "permissive", - }; - - appendAuditEntry(entry); - - const line = fs.readFileSync(auditPath, "utf-8").trim(); - expect(line).not.toContain("nvapi-"); - expect(line).not.toContain("ghp_"); - expect(line).not.toContain("sk-"); - }); -}); - describe("readRecentShieldsAutoRestore", () => { it("returns timestamp and timeoutSeconds when shields_down precedes shields_auto_restore (#5922)", () => { const now = new Date().toISOString(); @@ -400,6 +301,28 @@ describe("readRecentShieldsAutoRestore", () => { expect(event.timestamp).toBe(timestamp); }); + it("uses a null timeout when shields_down falls outside the bounded tail (#5922)", () => { + const timestamp = new Date().toISOString(); + fs.writeFileSync( + auditPath, + JSON.stringify({ + action: "shields_down", + sandbox: "alpha", + timestamp: new Date(Date.now() - 25 * 1000).toISOString(), + timeout_seconds: 20, + }) + + "\n" + + "x".repeat(1024 * 1024 + 100) + + "\n" + + JSON.stringify({ action: "shields_auto_restore", sandbox: "alpha", timestamp }) + + "\n", + ); + + const event = requireEvent(readRecentShieldsAutoRestore("alpha", 10 * 60 * 1000, auditPath)); + expect(event.timestamp).toBe(timestamp); + expect(event.timeoutSeconds).toBeNull(); + }); + it("reports an oversized unterminated JSONL entry as unreadable (#5922)", () => { fs.writeFileSync(auditPath, "x".repeat(1024 * 1024 + 100)); diff --git a/src/lib/shields/audit.ts b/src/lib/shields/audit.ts index 7f70d0a8dc5..74836a827c8 100644 --- a/src/lib/shields/audit.ts +++ b/src/lib/shields/audit.ts @@ -119,7 +119,9 @@ function readAuditTail(auditFile: string): string { * caller can surface degraded audit visibility while still dispatching the * agent. Fail-open is intentional: blocking dispatch on EACCES/EIO would turn * this advisory check into a denial-of-service boundary. Future-dated entries - * are rejected so a crafted row cannot pin the warning permanently. + * are rejected strictly so a crafted row cannot pin the warning permanently; + * the same host clock writes and reads this local log, and a rare false + * negative after a backward clock adjustment is safer than stale guidance. * * Only the last 1 MiB is read. This intentionally keeps the one-shot CLI read * synchronous so the warning is ordered before dispatch while bounding the