Skip to content

Commit 89615a6

Browse files
authored
fix(connect): warn when Shields auto-relocks (#9508)
## Summary OpenClaw sessions opened through `nemoclaw <sandbox> connect` now surface a warning when Shields auto-relock while the terminal connection is active. Connect now uses the existing asynchronous child supervisor, so the parent event loop can poll without a worker thread; audit-read failures remain advisory and never interrupt the session. ## Related Issue Fixes #9453 ## Changes - Run `openshell sandbox connect` through the existing async child supervisor with inherited stdio and signal cleanup. - Start a parent-event-loop watcher only for connect-managed OpenClaw sessions while the child owns the terminal. - Poll the bounded Shields audit reader and emit one direct stderr warning for each new auto-relock event after the session begins. - Include a shell-quoted, validated `shields down --timeout` recovery command and a safe timeout fallback. - Clear the watcher in the child lifecycle's `finally` path before connect exit handling and preserve terminal-runtime behavior. - Share recovery-command formatting with the existing one-shot agent warning and add unit/flow coverage. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — at `25a7536900c9db5ece8a143e4a800b04f1e13b4d`, 107/107 watcher, connect-flow, terminal-skin, and shared child-supervisor tests passed locally; the earlier DGX Spark Ubuntu aarch64 behavior smoke at `437fe871b13e48c8b009afe48a25148208a67e75` emitted the expected warning and shell-quoted recovery command - [x] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: the owning connect-prefix suite passed 177/177; source-architecture, TypeScript, commit hooks, and pre-push hooks passed - [ ] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added recovery notices when Shields automatically relocks during an active sandbox connection. * Notices include actionable recovery guidance and avoid repeating stale or duplicate alerts. * Connected sessions now monitor relock events and cleanly stop monitoring when the session ends. * Improved sandbox command execution with support for working-directory and environment settings. * **Bug Fixes** * Relock monitoring gracefully handles unavailable audit information without interrupting sessions. * Restricted recovery warnings to supported host agent workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
1 parent c8ae4b5 commit 89615a6

9 files changed

Lines changed: 371 additions & 101 deletions

ci/source-architecture-budget.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
"defaultMax": 20,
3838
"maxByFile": {
3939
"src/lib/actions/inference-set.ts": 32,
40-
"src/lib/actions/sandbox/connect.ts": 41,
40+
"src/lib/actions/sandbox/connect.ts": 42,
4141
"src/lib/actions/sandbox/destroy.ts": 29,
4242
"src/lib/actions/sandbox/doctor.ts": 30,
4343
"src/lib/actions/sandbox/status-snapshot.ts": 21,
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
import { describe, expect, it, vi } from "vitest";
5+
import type { ShieldsAutoRestoreReadResult } from "../../../shields/audit";
6+
import {
7+
type ConnectShieldsRelockNoticeState,
8+
pollConnectShieldsRelockNotice,
9+
startConnectShieldsRelockWatcher,
10+
} from "./connect-shields-relock-notice";
11+
12+
function state(startedAtMs: number): ConnectShieldsRelockNoticeState {
13+
return {
14+
lastNotifiedRestoreMs: startedAtMs - 1,
15+
sandboxName: "alpha beta",
16+
startedAtMs,
17+
};
18+
}
19+
20+
describe("connected-session Shields auto-relock notice", () => {
21+
it("prints one actionable warning for a new auto-relock event (#9453)", () => {
22+
const startedAtMs = Date.parse("2026-08-18T17:00:00.000Z");
23+
const readRecent = vi.fn(() => ({
24+
kind: "event" as const,
25+
event: { timestamp: "2026-08-18T17:00:20.000Z", timeoutSeconds: 20 },
26+
}));
27+
const writeNotice = vi.fn();
28+
29+
const afterFirstPoll = pollConnectShieldsRelockNotice(
30+
state(startedAtMs),
31+
readRecent,
32+
writeNotice,
33+
);
34+
const afterSecondPoll = pollConnectShieldsRelockNotice(afterFirstPoll, readRecent, writeNotice);
35+
36+
expect(afterSecondPoll.lastNotifiedRestoreMs).toBe(Date.parse("2026-08-18T17:00:20.000Z"));
37+
expect(writeNotice).toHaveBeenCalledOnce();
38+
expect(writeNotice.mock.calls[0]?.[0]).toContain("Shields auto-relocked after 20s");
39+
expect(writeNotice.mock.calls[0]?.[0]).toContain("restricted operations may now fail");
40+
expect(writeNotice.mock.calls[0]?.[0]).toContain(
41+
"nemoclaw 'alpha beta' shields down --timeout 20s",
42+
);
43+
});
44+
45+
it("ignores relock history from before this connected session (#9453)", () => {
46+
const startedAtMs = Date.parse("2026-08-18T17:00:00.000Z");
47+
const writeNotice = vi.fn();
48+
49+
const result = pollConnectShieldsRelockNotice(
50+
state(startedAtMs),
51+
() => ({
52+
kind: "event",
53+
event: { timestamp: "2026-08-18T16:59:59.999Z", timeoutSeconds: 20 },
54+
}),
55+
writeNotice,
56+
);
57+
58+
expect(result).toEqual(state(startedAtMs));
59+
expect(writeNotice).not.toHaveBeenCalled();
60+
});
61+
62+
it("keeps the connected session available when audit visibility is degraded (#9453)", () => {
63+
const startedAtMs = Date.parse("2026-08-18T17:00:00.000Z");
64+
const writeNotice = vi.fn();
65+
66+
expect(
67+
pollConnectShieldsRelockNotice(
68+
state(startedAtMs),
69+
() => ({ kind: "unreadable" }),
70+
writeNotice,
71+
),
72+
).toEqual(state(startedAtMs));
73+
expect(writeNotice).not.toHaveBeenCalled();
74+
});
75+
76+
it("polls on the parent event loop and stops cleanly (#9453)", () => {
77+
vi.useFakeTimers();
78+
const startedAtMs = Date.parse("2026-08-18T17:00:00.000Z");
79+
vi.setSystemTime(startedAtMs);
80+
const readRecent = vi
81+
.fn<(sandboxName: string) => ShieldsAutoRestoreReadResult>()
82+
.mockReturnValueOnce({ kind: "none" })
83+
.mockReturnValue({
84+
kind: "event",
85+
event: { timestamp: "2026-08-18T17:00:00.500Z", timeoutSeconds: 20 },
86+
});
87+
const writeNotice = vi.fn();
88+
89+
const watcher = startConnectShieldsRelockWatcher("alpha", readRecent, writeNotice);
90+
expect(readRecent).toHaveBeenCalledOnce();
91+
92+
vi.advanceTimersByTime(1_000);
93+
expect(writeNotice).toHaveBeenCalledOnce();
94+
95+
watcher?.stop();
96+
vi.advanceTimersByTime(2_000);
97+
expect(readRecent).toHaveBeenCalledTimes(2);
98+
vi.useRealTimers();
99+
});
100+
});
Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
import {
5+
readRecentShieldsAutoRestore,
6+
type ShieldsAutoRestoreReadResult,
7+
} from "../../../shields/audit";
8+
import { runSandboxExecChild, type SandboxExecChildOptions, type SpawnLikeResult } from "../exec";
9+
import {
10+
formatShieldsDownRecoveryCommand,
11+
normalizeShieldsRelockTimeoutSeconds,
12+
} from "./passthrough-shields-warning";
13+
14+
const CONNECT_SHIELDS_RELOCK_LOOKBACK_MS = 10 * 60 * 1000;
15+
const CONNECT_SHIELDS_RELOCK_POLL_MS = 1000;
16+
17+
type ConnectShieldsRelockNoticeReader = (sandboxName: string) => ShieldsAutoRestoreReadResult;
18+
19+
export interface ConnectShieldsRelockNoticeState {
20+
readonly lastNotifiedRestoreMs: number;
21+
readonly sandboxName: string;
22+
readonly startedAtMs: number;
23+
}
24+
25+
export interface ConnectShieldsRelockWatcher {
26+
stop(): void;
27+
}
28+
29+
function formatConnectShieldsRelockNotice(
30+
sandboxName: string,
31+
timeoutSeconds: number | null,
32+
): string {
33+
const safeTimeout = normalizeShieldsRelockTimeoutSeconds(timeoutSeconds);
34+
const afterPart = safeTimeout === null ? "" : ` after ${String(safeTimeout)}s`;
35+
return (
36+
`\n ⚠ Shields auto-relocked${afterPart}. This connected session remains open, but restricted operations may now fail.\n` +
37+
` Run \`${formatShieldsDownRecoveryCommand(sandboxName, safeTimeout)}\` on the host to lower Shields again.\n`
38+
);
39+
}
40+
41+
export function pollConnectShieldsRelockNotice(
42+
state: ConnectShieldsRelockNoticeState,
43+
readRecent: ConnectShieldsRelockNoticeReader = (sandboxName) =>
44+
readRecentShieldsAutoRestore(sandboxName, CONNECT_SHIELDS_RELOCK_LOOKBACK_MS),
45+
writeNotice: (value: string) => void = (value) => {
46+
process.stderr.write(value);
47+
},
48+
): ConnectShieldsRelockNoticeState {
49+
const result = readRecent(state.sandboxName);
50+
if (result.kind !== "event") return state;
51+
const restoreMs = new Date(result.event.timestamp).getTime();
52+
if (
53+
!Number.isFinite(restoreMs) ||
54+
restoreMs < state.startedAtMs ||
55+
restoreMs <= state.lastNotifiedRestoreMs
56+
) {
57+
return state;
58+
}
59+
writeNotice(formatConnectShieldsRelockNotice(state.sandboxName, result.event.timeoutSeconds));
60+
return { ...state, lastNotifiedRestoreMs: restoreMs };
61+
}
62+
63+
export function startConnectShieldsRelockWatcher(
64+
sandboxName: string,
65+
readRecent: ConnectShieldsRelockNoticeReader = (name) =>
66+
readRecentShieldsAutoRestore(name, CONNECT_SHIELDS_RELOCK_LOOKBACK_MS),
67+
writeNotice: (value: string) => void = (value) => {
68+
process.stderr.write(value);
69+
},
70+
): ConnectShieldsRelockWatcher | null {
71+
try {
72+
const startedAtMs = Date.now();
73+
let state: ConnectShieldsRelockNoticeState = {
74+
lastNotifiedRestoreMs: startedAtMs - 1,
75+
sandboxName,
76+
startedAtMs,
77+
};
78+
const poll = () => {
79+
try {
80+
state = pollConnectShieldsRelockNotice(state, readRecent, writeNotice);
81+
} catch {
82+
// Audit visibility is advisory. Keep the connected session available.
83+
}
84+
};
85+
poll();
86+
const timer = setInterval(poll, CONNECT_SHIELDS_RELOCK_POLL_MS);
87+
timer.unref();
88+
return {
89+
stop(): void {
90+
clearInterval(timer);
91+
},
92+
};
93+
} catch {
94+
// Advisory visibility must never prevent or terminate a connect session.
95+
return null;
96+
}
97+
}
98+
99+
export async function runConnectChildWithShieldsRelockNotice(
100+
binary: string,
101+
args: readonly string[],
102+
options: SandboxExecChildOptions,
103+
sandboxName: string,
104+
watchShields: boolean,
105+
): Promise<SpawnLikeResult> {
106+
const watcher = watchShields ? startConnectShieldsRelockWatcher(sandboxName) : null;
107+
try {
108+
return await runSandboxExecChild(binary, args, options);
109+
} finally {
110+
watcher?.stop();
111+
}
112+
}

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

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,10 @@ import {
2222
// - Presentation boundary: sandbox names are user-controlled command text and
2323
// must remain shell-quoted. Direct stderr output is deliberate so the warning
2424
// is visible in a one-shot CLI while machine-readable stdout stays clean.
25-
// - Source-fix constraint: an already-running in-sandbox TUI has no host CLI
26-
// interception point. That surface needs an upstream structured relock error
27-
// or a separate extend-on-activity design; this helper covers only host
28-
// `nemoclaw <name> agent` dispatches.
25+
// - Source-fix constraint: this helper covers only host `nemoclaw <name> agent`
26+
// dispatches. Connect-managed OpenClaw sessions use a separate bounded audit
27+
// watcher. Sessions entered outside NemoClaw still need an upstream structured
28+
// relock error or a separate extend-on-activity design.
2929
// - Regression tests cover validated/fallback timeouts, shell metacharacters
3030
// and embedded quotes, real-file JSON stdout separation, unreadable/absent
3131
// history, newer-down suppression, and terminal-runtime exclusion.
@@ -44,25 +44,34 @@ type ShieldsWarningProcess = {
4444

4545
type RecentShieldsAutoRestoreReader = (sandboxName: string) => ShieldsAutoRestoreReadResult;
4646

47+
export function normalizeShieldsRelockTimeoutSeconds(timeoutSeconds: number | null): number | null {
48+
return timeoutSeconds !== null &&
49+
Number.isInteger(timeoutSeconds) &&
50+
timeoutSeconds >= 1 &&
51+
timeoutSeconds <= 1800
52+
? timeoutSeconds
53+
: null;
54+
}
55+
56+
export function formatShieldsDownRecoveryCommand(
57+
sandboxName: string,
58+
timeoutSeconds: number | null,
59+
): string {
60+
const safeTimeout = normalizeShieldsRelockTimeoutSeconds(timeoutSeconds);
61+
return `${CLI_NAME} ${shellQuote(sandboxName)} shields down --timeout ${String(safeTimeout ?? 60)}s`;
62+
}
63+
4764
function emitShieldsRelockWarning(
4865
proc: ShieldsWarningProcess,
4966
relock: ShieldsAutoRestoreEvent,
5067
sandboxName: string,
5168
): void {
5269
// Defend the user-facing command suggestion even when tests or future
5370
// callers inject an event without going through the audit reader.
54-
const timeoutSeconds =
55-
relock.timeoutSeconds !== null &&
56-
Number.isInteger(relock.timeoutSeconds) &&
57-
relock.timeoutSeconds >= 1 &&
58-
relock.timeoutSeconds <= 1800
59-
? relock.timeoutSeconds
60-
: null;
71+
const timeoutSeconds = normalizeShieldsRelockTimeoutSeconds(relock.timeoutSeconds);
6172
const afterPart = timeoutSeconds !== null ? ` after ${String(timeoutSeconds)}s` : "";
62-
const timeoutSuggestion =
63-
timeoutSeconds !== null ? `--timeout ${String(timeoutSeconds)}s` : "--timeout 60s";
6473
proc.stderr.write(
65-
` ⚠ Shields auto-relocked${afterPart} — run \`${CLI_NAME} ${shellQuote(sandboxName)} shields down ${timeoutSuggestion}\` to extend.\n`,
74+
` ⚠ Shields auto-relocked${afterPart} — run \`${formatShieldsDownRecoveryCommand(sandboxName, timeoutSeconds)}\` to extend.\n`,
6675
);
6776
}
6877

0 commit comments

Comments
 (0)