diff --git a/ci/source-architecture-budget.json b/ci/source-architecture-budget.json index 177b685d856..65a733343f6 100644 --- a/ci/source-architecture-budget.json +++ b/ci/source-architecture-budget.json @@ -25,7 +25,7 @@ "src/lib/messaging/channels/index.ts": 25, "src/lib/onboard/gateway-binding.ts": 52, "src/lib/runner.ts": 87, - "src/lib/security/redact.ts": 52, + "src/lib/security/redact.ts": 53, "src/lib/state/onboard-session.ts": 37, "src/lib/state/registry.ts": 101, "src/lib/state/state-root.ts": 21, @@ -51,6 +51,7 @@ "src/lib/inference/vllm.ts": 21, "src/lib/onboard.ts": 201, "src/lib/onboard/machine/handlers/sandbox.ts": 21, + "src/lib/policy/index.ts": 22, "src/lib/sandbox/config.ts": 22, "src/lib/shields/index.ts": 23 } diff --git a/docs/reference/troubleshooting.mdx b/docs/reference/troubleshooting.mdx index 2b1f19b224b..57d0906272b 100644 --- a/docs/reference/troubleshooting.mdx +++ b/docs/reference/troubleshooting.mdx @@ -1159,6 +1159,209 @@ echo '/swapfile none swap sw 0 0' | sudo tee -a /etc/fstab $$nemoclaw onboard ``` +### Onboarding Reports a Rejected or Unconfirmed Policy Update + +Onboarding can submit several policy mutations in sequence when you deselect policy presets and select others. +It submits deselection mutations before selection mutations. +Each successful mutation updates the live OpenShell gateway policy and the sandbox registry before the next mutation starts. +If a later mutation fails, the earlier successful mutations remain applied and recorded. + +Each mutation uses a temporary `policy.yaml` file in a `nemoclaw-policy-*` directory. +When the submission finishes, NemoClaw removes that directory, whether or not the gateway accepted the mutation. +If cleanup fails, NemoClaw reports the directory that still holds the policy instead of reporting the submission result. + +When NemoClaw reports this directory, do not retry the policy operation. +Use Bash to enter and validate the exact path from the error before you remove its contents: + +```bash +cleanup_retained_policy() { + local platform_tmp_root retained_policy_dir + + if ! platform_tmp_root=$(node -p "require('node:os').tmpdir()"); then + printf 'Validation failed: Node.js could not report the platform temporary directory.\n' >&2 + return 1 + fi + platform_tmp_root=${platform_tmp_root%/} + IFS= read -r -p 'Enter the exact temporary policy directory from the error: ' retained_policy_dir + python3 - "$platform_tmp_root" "$retained_policy_dir" <<'PY' +import os +import re +import stat +import sys + + +class CleanupError(Exception): + pass + + +def open_retained_policy_directory(platform_tmp_root, reported_path): + if not os.path.isabs(platform_tmp_root): + raise CleanupError("the platform temporary directory is not absolute") + platform_tmp_root_real = os.path.realpath(platform_tmp_root) + if platform_tmp_root_real == os.path.abspath(os.sep): + raise CleanupError("the platform temporary directory cannot be the filesystem root") + reported_basename = os.path.basename(reported_path) + reported_parent = os.path.dirname(reported_path) + if not re.fullmatch(r"nemoclaw-policy-.+", reported_basename): + raise CleanupError("the basename must match nemoclaw-policy-* and include a suffix") + if reported_parent != platform_tmp_root: + raise CleanupError( + f"the path must be a direct child of the actual platform temporary directory {platform_tmp_root!r}" + ) + if os.path.realpath(reported_path) != os.path.join(platform_tmp_root_real, reported_basename): + raise CleanupError("the canonical path is outside the actual platform temporary directory") + if ( + not hasattr(os, "geteuid") + or not hasattr(os, "O_DIRECTORY") + or not hasattr(os, "O_NOFOLLOW") + or os.stat not in os.supports_dir_fd + or os.unlink not in os.supports_dir_fd + ): + raise CleanupError("this platform cannot perform descriptor-relative cleanup") + + directory_flags = os.O_RDONLY | os.O_DIRECTORY | getattr(os, "O_CLOEXEC", 0) + root_stat = os.stat(platform_tmp_root_real, follow_symlinks=False) + if not stat.S_ISDIR(root_stat.st_mode): + raise CleanupError("the canonical platform temporary path is not a directory") + root_fd = os.open(platform_tmp_root_real, directory_flags | os.O_NOFOLLOW) + try: + opened_root_stat = os.fstat(root_fd) + if (opened_root_stat.st_dev, opened_root_stat.st_ino) != (root_stat.st_dev, root_stat.st_ino): + raise CleanupError("the platform temporary directory changed while it was being opened") + reported_stat = os.stat(reported_basename, dir_fd=root_fd, follow_symlinks=False) + if not stat.S_ISDIR(reported_stat.st_mode): + raise CleanupError("the path is not a directory or is a symbolic link") + if reported_stat.st_uid != os.geteuid(): + raise CleanupError("the current user does not own the directory") + policy_fd = os.open( + reported_basename, + directory_flags | os.O_NOFOLLOW, + dir_fd=root_fd, + ) + opened_stat = os.fstat(policy_fd) + if (opened_stat.st_dev, opened_stat.st_ino) != (reported_stat.st_dev, reported_stat.st_ino): + os.close(policy_fd) + raise CleanupError("the directory changed while it was being opened") + return root_fd, policy_fd, reported_basename, opened_stat + except Exception: + os.close(root_fd) + raise + + +def remove_retained_policy_material(policy_fd): + try: + os.unlink("policy.yaml", dir_fd=policy_fd) + except FileNotFoundError: + pass + remaining = os.listdir(policy_fd) + if remaining: + raise CleanupError(f"the directory contains unexpected entries: {remaining!r}") + + +def main(): + root_fd = None + policy_fd = None + try: + root_fd, policy_fd, basename, opened_stat = open_retained_policy_directory( + sys.argv[1], sys.argv[2] + ) + remove_retained_policy_material(policy_fd) + try: + current_stat = os.stat(basename, dir_fd=root_fd, follow_symlinks=False) + except FileNotFoundError as error: + raise CleanupError("the reported path changed during cleanup") from error + if (current_stat.st_dev, current_stat.st_ino) != (opened_stat.st_dev, opened_stat.st_ino): + raise CleanupError("the reported path was replaced during cleanup; the replacement was not removed") + print( + f"Removed retained policy material from {sys.argv[2]!r}. " + "The empty directory intentionally remains." + ) + except (CleanupError, OSError) as error: + print(f"Cleanup failed for {sys.argv[2]!r}: {error}.", file=sys.stderr) + raise SystemExit(1) from error + finally: + if policy_fd is not None: + os.close(policy_fd) + if root_fd is not None: + os.close(root_fd) + + +if __name__ == "__main__": + main() +PY +} +cleanup_retained_policy +``` + +The procedure opens the validated directory without following a symbolic link and removes `policy.yaml` relative to that open directory. +It fails if the directory contains anything else. +It intentionally leaves the empty directory because deleting it later by pathname would reintroduce a directory-replacement race. +Continue only after the procedure reports that the retained policy material was removed. +Do not remove the empty directory by pathname. +If validation or cleanup fails, preserve the exact path and complete error message for support. +Do not use another removal command on that path. + +After temporary-directory cleanup, treat the gateway state as unknown because the cleanup error replaced the submission result. +Restore access to the OpenShell gateway, then read the sandbox policy: + +```bash +$$nemoclaw policy list +``` + +If `policy list` reports `⚠ Could not query gateway — showing local state only.`, stop because the command did not read the gateway policy. +If it reports container-runtime recovery guidance, restore that runtime and run `policy list` again. +Do not resume or start fresh onboarding until `policy list` reports the live gateway policy. + +When NemoClaw reports a rejected or unconfirmed policy mutation, onboarding stops and: + +- leaves earlier successful mutations applied and recorded; +- does not record that mutation in the sandbox registry; +- marks the session failed and resumable; +- reports one of the two results below. + +After either result, run `policy list` before you resume or start fresh onboarding. + +The gateway read the policy and refused it: + +```text +OpenShell rejected the policy for sandbox 'my-assistant' (exit ): . The policy was not applied and re-applying it will be rejected again; change the preset selection instead. +``` + +An OpenShell refusal means this mutation did not change the live policy. +Earlier successful mutations in the same onboarding step remain applied and recorded. +Use the quoted OpenShell diagnostic to identify what the gateway refused. +After you inspect `policy list`, follow [Previous onboarding session failed](#previous-onboarding-session-failed) to start fresh onboarding and choose a different preset selection. + +NemoClaw could not confirm the result. +This covers a connection that ended before the result arrived, an unreachable gateway, an elapsed deadline, a rejected credential, and a refusal the gateway reported with a status NemoClaw does not recognize as final. +NemoClaw reports this whenever the gateway did not return an explicit refusal, because only an explicit refusal proves the policy was not applied: + +```text +Could not confirm the policy update for sandbox 'my-assistant': . The gateway may or may not have applied it; read the current policy back before retrying. +``` + +The gateway state is unknown, so read the sandbox policy with `policy list` before you retry. +`policy list` compares the sandbox registry with the live gateway policy and flags a preset that is applied in one place but not the other. +The unconfirmed mutation does not update the sandbox registry. + +The same connection problem that made the result unconfirmed can also stop `policy list` from reaching the gateway. +When that happens, `policy list` prints `⚠ Could not query gateway — showing local state only.` and still exits 0. +If the container runtime is down, it prints that runtime's recovery guidance instead. +Stop while `policy list` reports local state only because that output does not show whether the gateway applied the mutation. +Restore gateway access and run `policy list` again before you resume or start fresh onboarding. + +After `policy list` reads the live gateway policy, follow the result below. +The [failed-session recovery steps](#previous-onboarding-session-failed) provide the resume and fresh onboarding commands. + +- If an affected preset reports `active on gateway, missing from local state`, resume the failed session. + The gateway completed the addition, and resume can record the applied preset locally without submitting that policy change again. +- If an affected preset reports `recorded locally, not active on gateway`, do not resume or start fresh onboarding. + The gateway may have completed the removal while the sandbox registry retained the preset. + Preserve the original unconfirmed error and the complete `policy list` output for support. +- If `policy list` reports no disagreement for the unconfirmed mutation, compare the live preset set with the selection in the failed session. + Resume only if the live policy still requires the unconfirmed addition or removal to match that selection. + Otherwise, start fresh onboarding and select the exact preset set that `policy list` reports as active. + ### Previous onboarding session failed If a previous `$$nemoclaw onboard` attempt fails partway through (for example, a provider or inference-setup step reporting an error), NemoClaw records the failure in `~/.nemoclaw/onboard-session.json`. diff --git a/src/lib/onboard/policy-preset-sync-finality.test.ts b/src/lib/onboard/policy-preset-sync-finality.test.ts new file mode 100644 index 00000000000..d2eefd45ae5 --- /dev/null +++ b/src/lib/onboard/policy-preset-sync-finality.test.ts @@ -0,0 +1,109 @@ +// 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 { waitForPolicyMutation } from "./policy-preset-sync"; + +/** + * `waitForPolicyMutation` polls through `waitUntil(fn, 10, 2000)`. Only the + * explicit sandbox-not-found readiness condition reaches the 2-second poll. + */ +const MUTATION_POLL_INTERVAL_MS = 2_000; + +/** Produced by `policySetFailure` for an accepted refusal result. */ +const REJECTION_ERROR_MESSAGE = + "OpenShell rejected the policy for sandbox 'sb-9206' (exit 1): " + + "unsupported field in network_policies.weather. The policy was not applied and " + + "re-applying it will be rejected again; change the preset selection instead."; + +/** Produced by `policySetFailure` when the submission result is unconfirmed. */ +const UNCONFIRMED_ERROR_MESSAGE = + "Could not confirm the policy update for sandbox 'sb-9206': h2 protocol error. " + + "The gateway may or may not have applied it; read the current policy back before retrying."; + +/** Thrown while a sandbox is still starting; the one exception worth re-polling. */ +const TRANSIENT_STARTUP_ERROR_MESSAGE = "sandbox not found: sb-9206"; + +describe("waitForPolicyMutation", () => { + let clockMs = 0; + let sleptMs: number[] = []; + + beforeEach(() => { + // `waitUntil` measures its budget with Date.now and burns it in a blocking + // Atomics.wait. Driving one synthetic clock from the other keeps the real + // polling arithmetic while costing no wall-clock time. + clockMs = 1_700_000_000_000; + sleptMs = []; + vi.spyOn(Date, "now").mockImplementation(() => clockMs); + vi.spyOn(Atomics, "wait").mockImplementation((_typedArray, _index, _value, timeout) => { + const durationMs = typeof timeout === "number" ? timeout : 0; + sleptMs.push(durationMs); + clockMs += durationMs; + return "timed-out"; + }); + }); + + it("attempts a rejected policy submission exactly once (#9206)", () => { + let attempts = 0; + const mutate = (): boolean => { + attempts += 1; + throw new Error(REJECTION_ERROR_MESSAGE); + }; + + expect(() => waitForPolicyMutation("applyPresets(weather)", mutate)).toThrow( + REJECTION_ERROR_MESSAGE, + ); + expect(attempts).toBe(1); + expect(sleptMs).toEqual([]); + }); + + it("attempts a policy submission with an unconfirmed result exactly once (#9206)", () => { + let attempts = 0; + const mutate = (): boolean => { + attempts += 1; + throw new Error(UNCONFIRMED_ERROR_MESSAGE); + }; + + expect(() => waitForPolicyMutation("applyPresets(weather)", mutate)).toThrow( + UNCONFIRMED_ERROR_MESSAGE, + ); + expect(attempts).toBe(1); + expect(sleptMs).toEqual([]); + }); + + it("keeps re-polling a sandbox that has not appeared yet until the mutation lands (#9206)", () => { + const behaviours: ReadonlyArray<() => boolean> = [ + () => { + throw new Error(TRANSIENT_STARTUP_ERROR_MESSAGE); + }, + () => { + throw new Error(TRANSIENT_STARTUP_ERROR_MESSAGE); + }, + () => true, + ]; + let attempts = 0; + const mutate = (): boolean => { + const behaviour = behaviours[attempts] ?? (() => true); + attempts += 1; + return behaviour(); + }; + + expect(() => waitForPolicyMutation("applyPreset(slack)", mutate)).not.toThrow(); + expect(attempts).toBe(3); + expect(sleptMs).toEqual([MUTATION_POLL_INTERVAL_MS, MUTATION_POLL_INTERVAL_MS]); + }); + + it("attempts a policy mutation that returns false exactly once (#9206)", () => { + let attempts = 0; + const mutate = (): boolean => { + attempts += 1; + return false; + }; + + expect(() => waitForPolicyMutation("applyPreset(slack)", mutate)).toThrow( + "applyPreset(slack) returned false", + ); + expect(attempts).toBe(1); + expect(sleptMs).toEqual([]); + }); +}); diff --git a/src/lib/onboard/policy-preset-sync.ts b/src/lib/onboard/policy-preset-sync.ts index 82973871daf..2f18608e65a 100644 --- a/src/lib/onboard/policy-preset-sync.ts +++ b/src/lib/onboard/policy-preset-sync.ts @@ -11,8 +11,7 @@ function waitForPolicyMutation(description: string, mutate: () => boolean | void try { const result = mutate(); if (result === false) { - lastError = new Error(`${description} returned false`); - return false; + throw new Error(`${description} returned false`); } return true; } catch (err) { diff --git a/src/lib/policy/index.ts b/src/lib/policy/index.ts index 90547e9e1b5..15bf6f7e77f 100644 --- a/src/lib/policy/index.ts +++ b/src/lib/policy/index.ts @@ -29,6 +29,7 @@ import { assertNoOpenShellGatewayEndpointOverride } from "../openshell-gateway-e import { OPENSHELL_SANDBOX_HOST_BRIDGE } from "../private-networks"; import { ROOT, run, runCapture } from "../runner"; import { diagnosticPreview, isValidName, NAME_ALLOWED_FORMAT } from "../sandbox-name-contract"; +import { redact } from "../security/redact"; import * as registry from "../state/registry"; import type { BaselineExclusionRuntimeStatus } from "./baseline-exclusion"; import { @@ -49,6 +50,7 @@ import { stripProviderComposedPolicies, withoutProviderComposedPolicies, } from "./merge"; +import { classifyPolicySetResult, type PolicySetOutcome } from "./policy-set-outcome"; import { findUnexpectedExistingPolicyKey, PERSONAL_OPEN_INTERNET_PRESET_NAME, @@ -528,25 +530,133 @@ function assertOpenshellResolvable(options: { nonFatal?: boolean } = {}): boolea } /** - * Apply a policy file while optionally keeping control in the caller on - * failure. Lifecycle code that owns compensating actions must use nonFatal so - * a failed OpenShell mutation cannot bypass its rollback through process.exit. + * `run` never sets an encoding, so `spawnSync` hands back stdio as a Buffer. */ -function setPolicyFile( - policyFile: string, +function decodePolicySetStream(stream: string | Buffer | null | undefined): string { + if (stream === null || stream === undefined) return ""; + return typeof stream === "string" ? stream : stream.toString("utf-8"); +} + +/** Delete the private temp policy file and its directory, ignoring absence. */ +function tempPolicyRetentionError(tmpDir: string, reason: string): Error { + return new Error( + `Could not remove the temporary policy directory '${tmpDir}' (${reason}). It still holds ` + + "the composed sandbox policy; remove it before retrying.", + ); +} + +function removeTempPolicyMaterial(tmpDir: string): void { + try { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } catch (error) { + throw tempPolicyRetentionError(tmpDir, error instanceof Error ? error.message : String(error)); + } + if (fs.existsSync(tmpDir)) throw tempPolicyRetentionError(tmpDir, "the path still exists"); +} + +interface PolicySetSubmission { + readonly outcome: PolicySetOutcome; + /** + * The status the submission exited with, so a caller that ends the process + * still reports the code the runner would have reported. + */ + readonly status: number | null; +} + +/** + * Submit a composed policy document through a private temp file and classify + * what OpenShell did with it. + * + * `policy set` runs with `ignoreError` because the runner otherwise calls + * `process.exit` on a nonzero status, and `process.exit` does not unwind + * `finally`: that is exactly how a failed submission left the composed + * sandbox policy readable in `$TMPDIR` (#9206). Owning the temp material here + * means it is gone before any caller decides to end the process. + */ +function submitComposedPolicy( sandboxName: string, + policyDocument: string, + gatewayName?: string, +): PolicySetSubmission { + // `mkdtempSync` creates nothing when it throws, so only the write and the + // submission need the cleanup boundary. Writing inside it keeps a failed or + // partial write from leaving the composed policy readable in $TMPDIR. + // + // A cleanup failure deliberately supersedes whatever the body produced: a + // policy document still readable on disk is the condition that must never be + // reported as a clean result. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-")); + try { + const tmpFile = path.join(tmpDir, "policy.yaml"); + fs.writeFileSync(tmpFile, policyDocument, { encoding: "utf-8", mode: 0o600 }); + const result = run(buildPolicySetCommand(tmpFile, sandboxName), { + ignoreError: true, + ...(gatewayName ? { env: { OPENSHELL_GATEWAY: gatewayName } } : {}), + }); + return { + outcome: classifyPolicySetResult({ + status: result.status, + error: result.error, + stderr: decodePolicySetStream(result.stderr), + }), + status: result.status, + }; + } finally { + removeTempPolicyMaterial(tmpDir); + } +} + +/** + * Describe a failed `policy set` for the operator. An OpenShell diagnostic can + * quote the policy that was submitted, so every message is redacted before it + * reaches the console. + * + * A `rejected` verdict is final: OpenShell understood the document and refused + * it, so resubmitting only replays a policy it already declined. An `ambiguous` + * result proves nothing about gateway state, so the operator must read the + * policy back before deciding anything. + */ +function policySetFailure( + sandboxName: string, + outcome: Exclude, +): Error { + if (outcome.kind === "rejected") { + return new Error( + `OpenShell rejected the policy for sandbox '${sandboxName}' (exit ${outcome.status}): ` + + `${redact(outcome.message)}. The policy was not applied and re-applying it will be ` + + `rejected again; change the preset selection instead.`, + ); + } + return new Error( + `Could not confirm the policy update for sandbox '${sandboxName}': ${redact(outcome.detail)}. ` + + `The gateway may or may not have applied it; read the current policy back before retrying.`, + ); +} + +/** + * Apply a composed policy document while optionally keeping control in the + * caller on failure. Lifecycle code that owns compensating actions must use + * nonFatal so a failed OpenShell mutation cannot bypass its rollback through + * process.exit. + * + * The submission owns the temp policy file, so the composed policy is already + * deleted by the time this ends the process for a fatal caller (#9206). + */ +function setPolicyDocument( + sandboxName: string, + policyDocument: string, options: { nonFatal?: boolean; gatewayName?: string } = {}, ): boolean { - const result = run(buildPolicySetCommand(policyFile, sandboxName), { - ignoreError: options.nonFatal === true, - ...(options.gatewayName ? { env: { OPENSHELL_GATEWAY: options.gatewayName } } : {}), - }); - if (!options.nonFatal) return true; - if (!result.error && result.status === 0) return true; + const { outcome, status } = submitComposedPolicy( + sandboxName, + policyDocument, + options.gatewayName, + ); + if (outcome.kind === "applied") return true; - const detail = result.error?.message ?? `exit ${result.status ?? "unknown"}`; - console.error(` Failed to update policy for sandbox '${sandboxName}' (${detail}).`); - return false; + console.error(` ${policySetFailure(sandboxName, outcome).message}`); + if (options.nonFatal) return false; + process.exit(status || 1); } /** @@ -1292,29 +1402,12 @@ function removePreset( console.log(` Narrowing sandbox egress — removing: ${endpoints.join(", ")}`); } - // Run before creating temp resources so a missing-binary exit doesn't - // orphan files in $TMPDIR (the finally cleanup doesn't run on process.exit). + // Run before submitting so a missing-binary exit doesn't orphan files in + // $TMPDIR (the cleanup doesn't run on process.exit). if (!assertOpenshellResolvable(options)) return false; - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-")); - const tmpFile = path.join(tmpDir, "policy.yaml"); - fs.writeFileSync(tmpFile, updated, { encoding: "utf-8", mode: 0o600 }); - - try { - if (!setPolicyFile(tmpFile, sandboxName, options)) return false; - console.log(` Removed preset: ${presetName}`); - } finally { - try { - fs.unlinkSync(tmpFile); - } catch { - /* ignored */ - } - try { - fs.rmdirSync(tmpDir); - } catch { - /* ignored */ - } - } + if (!setPolicyDocument(sandboxName, updated, options)) return false; + console.log(` Removed preset: ${presetName}`); const sandbox = options.skipRegistryUpdate ? undefined : registry.getSandbox(sandboxName); if (sandbox) { @@ -1336,23 +1429,7 @@ function pushPolicyYaml( options: { nonFatal?: boolean; gatewayName?: string } = {}, ): boolean { if (!assertOpenshellResolvable(options)) return false; - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-")); - const tmpFile = path.join(tmpDir, "policy.yaml"); - fs.writeFileSync(tmpFile, updatedPolicy, { encoding: "utf-8", mode: 0o600 }); - try { - return setPolicyFile(tmpFile, sandboxName, options); - } finally { - try { - fs.unlinkSync(tmpFile); - } catch { - /* ignored */ - } - try { - fs.rmdirSync(tmpDir); - } catch { - /* ignored */ - } - } + return setPolicyDocument(sandboxName, updatedPolicy, options); } /** Round-trippable live policy body from `--base`, or null when unreadable. */ @@ -2079,8 +2156,8 @@ function applyPresetContent( console.error(` Preset '${presetName}' has invalid or missing network_policies.`); return false; } - const reservedKey = [OPENCLAW_NPM_PRESET_KEY, PERSONAL_OPEN_INTERNET_POLICY_KEY].find( - (key) => Object.prototype.hasOwnProperty.call(np, key), + const reservedKey = [OPENCLAW_NPM_PRESET_KEY, PERSONAL_OPEN_INTERNET_POLICY_KEY].find((key) => + Object.prototype.hasOwnProperty.call(np, key), ); if (reservedKey) { console.error(` Custom presets cannot own reserved network policy key '${reservedKey}'.`); @@ -2240,31 +2317,13 @@ function applyPresetContent( ); const policyChanged = requiresOwnedKeyRefresh || !policyDocumentsMatch(currentPolicy, merged); - // Run before creating temp resources so a missing-binary exit doesn't - // orphan files in $TMPDIR (the finally cleanup doesn't run on process.exit). + // Run before submitting so a missing-binary exit doesn't orphan files in + // $TMPDIR (the cleanup doesn't run on process.exit). if (policyChanged && !assertOpenshellResolvable(options)) return false; if (policyChanged) { - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-")); - const tmpFile = path.join(tmpDir, "policy.yaml"); - fs.writeFileSync(tmpFile, merged, { encoding: "utf-8", mode: 0o600 }); - - try { - if (!setPolicyFile(tmpFile, sandboxName, options)) return false; - - console.log(` Applied preset: ${presetName}`); - } finally { - try { - fs.unlinkSync(tmpFile); - } catch { - /* ignored */ - } - try { - fs.rmdirSync(tmpDir); - } catch { - /* ignored */ - } - } + if (!setPolicyDocument(sandboxName, merged, options)) return false; + console.log(` Applied preset: ${presetName}`); } // Some multi-resource lifecycle callers reserve ownership in the registry @@ -2451,27 +2510,13 @@ function applyPresets(sandboxName: string, presetNames: string[]): boolean { if (policyChanged) assertOpenshellResolvable(); if (policyChanged) { - const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-")); - const tmpFile = path.join(tmpDir, "policy.yaml"); - fs.writeFileSync(tmpFile, merged, { encoding: "utf-8", mode: 0o600 }); + // The shared fatal path preserves OpenShell's status after it removes the + // temporary policy. Onboarding defers that exit until its recovery state + // and outer cleanup have finished. + setPolicyDocument(sandboxName, merged); - try { - run(buildPolicySetCommand(tmpFile, sandboxName)); - - for (const preset of presetContents.filter((entry) => entry.state !== "match")) { - console.log(` Applied preset: ${preset.name}`); - } - } finally { - try { - fs.unlinkSync(tmpFile); - } catch { - /* ignored */ - } - try { - fs.rmdirSync(tmpDir); - } catch { - /* ignored */ - } + for (const preset of presetContents.filter((entry) => entry.state !== "match")) { + console.log(` Applied preset: ${preset.name}`); } } diff --git a/src/lib/policy/policy-apply-finality.test.ts b/src/lib/policy/policy-apply-finality.test.ts new file mode 100644 index 00000000000..687d7231799 --- /dev/null +++ b/src/lib/policy/policy-apply-finality.test.ts @@ -0,0 +1,378 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import type { SpawnSyncReturns } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const { addCustomPolicy, getSandbox, resolveOpenshell, run, runCapture, updateSandbox } = + vi.hoisted(() => ({ + addCustomPolicy: vi.fn(), + getSandbox: vi.fn(), + resolveOpenshell: vi.fn(), + run: vi.fn(), + runCapture: vi.fn(), + updateSandbox: vi.fn(), + })); + +vi.mock("../runner", async (importOriginal) => ({ + ...(await importOriginal()), + run, + runCapture, +})); + +vi.mock("../state/registry", async (importOriginal) => ({ + ...(await importOriginal()), + addCustomPolicy, + getSandbox, + updateSandbox, +})); + +vi.mock("../adapters/openshell/resolve", async (importOriginal) => ({ + ...(await importOriginal()), + resolveOpenshell, +})); + +import { applyPresetContent, applyPresets, removePreset } from "./index"; + +const SANDBOX = "finality-9206"; + +/** A round-trippable base policy that does not yet carry the weather preset. */ +const BASE_POLICY = `version: 1 +network_policies: + nvidia: + name: nvidia + endpoints: + - host: api.nvidia.com + port: 443 + protocol: rest + enforcement: enforce + rules: + - allow: { method: GET, path: "/**" } +`; + +/** The same base policy after the weather preset has been applied to it. */ +const BASE_POLICY_WITH_WEATHER = `${BASE_POLICY} weather: + name: weather + endpoints: + - host: wttr.in + port: 443 + protocol: rest + enforcement: enforce + rules: + - allow: { method: GET, path: "/**" } +`; + +/** Minimal preset content for the single-preset apply path. */ +const WEATHER_PRESET_CONTENT = `preset: + name: weather + description: "Read-only public weather" + +network_policies: + weather: + name: weather + endpoints: + - host: wttr.in + port: 443 + protocol: rest + enforcement: enforce + rules: + - allow: { method: GET, path: "/**" } +`; + +/** + * A credential-shaped token an OpenShell diagnostic could echo back out of the + * policy document it was handed. + */ +const CREDENTIAL_TOKEN = "nvapi-abcdefghijklmnopqrstuvwxyz"; + +/** A synthetic `code:`/`message:` frame the classifier accepts as a refusal. */ +function openshellRejection(message: string): string { + return ( + `Error: code: 'Failed precondition', message: '${message}', ` + + "source: tonic::Status { code: FailedPrecondition, grpc_status: 9 }" + ); +} + +function policySetResult(stderr: string): SpawnSyncReturns { + return { + pid: 4242, + output: [null, Buffer.alloc(0), Buffer.from(stderr, "utf-8")], + stdout: Buffer.alloc(0), + // `run` never passes an encoding, so spawnSync hands stderr back as a Buffer. + stderr: Buffer.from(stderr, "utf-8"), + status: 1, + signal: null, + }; +} + +function reportedText(): string { + return vi + .mocked(console.error) + .mock.calls.flat() + .map((entry) => String(entry)) + .join("\n"); +} + +function applyWeatherPreset(): unknown { + const exit = vi.spyOn(process, "exit").mockImplementation((code) => { + throw new Error(`process.exit(${String(code)}): ${reportedText()}`); + }); + try { + applyPresets(SANDBOX, ["weather"]); + return null; + } catch (error) { + return error; + } finally { + exit.mockRestore(); + } +} + +function removeTemporaryDirectory(directory: string, removeDirectory: typeof fs.rmSync): void { + removeDirectory(directory, { recursive: true, force: true }); +} + +describe("applyPresets finality when openshell rejects the composed policy", () => { + beforeEach(() => { + run.mockReset(); + runCapture.mockReset(); + getSandbox.mockReset(); + updateSandbox.mockReset(); + addCustomPolicy.mockReset(); + resolveOpenshell.mockReset(); + + resolveOpenshell.mockReturnValue("/usr/local/bin/openshell"); + runCapture.mockReturnValue(BASE_POLICY); + getSandbox.mockReturnValue({ name: SANDBOX, agent: "openclaw", policies: [] }); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + it("surfaces the authoritative OpenShell message rather than a bare exit status (#9206)", () => { + const message = 'network policy "weather" rejected: endpoint wttr.in conflicts with baseline'; + run.mockReturnValue(policySetResult(openshellRejection(message))); + + const error = applyWeatherPreset(); + + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toContain(message); + expect((error as Error).message).toContain("exit 1"); + expect((error as Error).message).not.toMatch(/^exit 1$/); + }); + + it("redacts a credential-shaped token in the OpenShell message before reporting it (#9206)", () => { + run.mockReturnValue( + policySetResult(openshellRejection(`rejected: header carries ${CREDENTIAL_TOKEN}`)), + ); + + const error = applyWeatherPreset(); + + expect((error as Error).message).not.toContain(CREDENTIAL_TOKEN); + expect((error as Error).message).toContain("nvap"); + const reported = [ + ...vi.mocked(console.error).mock.calls, + ...vi.mocked(console.log).mock.calls, + ].flat(); + expect(reported.filter((entry) => String(entry).includes(CREDENTIAL_TOKEN))).toEqual([]); + }); + + it("leaves local preset attribution unwritten when openshell rejects the policy (#9206)", () => { + run.mockReturnValue(policySetResult(openshellRejection("rejected: unsupported field"))); + + expect(applyWeatherPreset()).toBeInstanceOf(Error); + expect(updateSandbox).not.toHaveBeenCalled(); + expect(addCustomPolicy).not.toHaveBeenCalled(); + }); + + it("leaves local preset attribution unwritten when the outcome is unknown (#9206)", () => { + run.mockReturnValue( + policySetResult("Error: code: 'Internal error', message: 'h2 protocol error: http2 error'"), + ); + + const error = applyWeatherPreset(); + + expect((error as Error).message).toContain("read the current policy back before retrying"); + expect(updateSandbox).not.toHaveBeenCalled(); + expect(addCustomPolicy).not.toHaveBeenCalled(); + }); +}); + +/** + * The single-preset mutations keep the contract their lifecycle callers were + * written against: with `nonFatal` they report the failure, return `false`, + * and leave the compensating action to the caller. Only the leaked temp + * material and the bare exit status changed (#9206). + */ +describe("single-preset mutations when openshell rejects the composed policy", () => { + const REJECTION_MESSAGE = "unsupported field in network_policies.weather"; + + beforeEach(() => { + run.mockReset(); + runCapture.mockReset(); + getSandbox.mockReset(); + updateSandbox.mockReset(); + addCustomPolicy.mockReset(); + resolveOpenshell.mockReset(); + + resolveOpenshell.mockReturnValue("/usr/local/bin/openshell"); + getSandbox.mockReturnValue({ name: SANDBOX, agent: "openclaw", policies: ["weather"] }); + run.mockReturnValue(policySetResult(openshellRejection(REJECTION_MESSAGE))); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + it("returns false from a nonFatal removePreset and reports the OpenShell message (#9206)", () => { + runCapture.mockReturnValue(BASE_POLICY_WITH_WEATHER); + + expect(removePreset(SANDBOX, "weather", { nonFatal: true })).toBe(false); + expect(reportedText()).toContain(REJECTION_MESSAGE); + expect(reportedText()).toContain("change the preset selection instead"); + expect(updateSandbox).not.toHaveBeenCalled(); + }); + + it("returns false from a nonFatal applyPresetContent and reports the OpenShell message (#9206)", () => { + runCapture.mockReturnValue(BASE_POLICY); + + expect(applyPresetContent(SANDBOX, "weather", WEATHER_PRESET_CONTENT, { nonFatal: true })).toBe( + false, + ); + expect(reportedText()).toContain(REJECTION_MESSAGE); + expect(updateSandbox).not.toHaveBeenCalled(); + expect(addCustomPolicy).not.toHaveBeenCalled(); + }); + + it("redacts a credential-shaped token before reporting a nonFatal failure (#9206)", () => { + runCapture.mockReturnValue(BASE_POLICY); + run.mockReturnValue( + policySetResult(openshellRejection(`rejected: header carries ${CREDENTIAL_TOKEN}`)), + ); + + expect(applyPresetContent(SANDBOX, "weather", WEATHER_PRESET_CONTENT, { nonFatal: true })).toBe( + false, + ); + expect(reportedText()).not.toContain(CREDENTIAL_TOKEN); + }); +}); + +describe("applyPresets temporary policy material under local I/O failure", () => { + beforeEach(() => { + run.mockReset(); + runCapture.mockReset(); + getSandbox.mockReset(); + updateSandbox.mockReset(); + resolveOpenshell.mockReset(); + + resolveOpenshell.mockReturnValue("/usr/local/bin/openshell"); + runCapture.mockReturnValue(BASE_POLICY); + getSandbox.mockReturnValue({ name: SANDBOX, agent: "openclaw", policies: [] }); + run.mockReturnValue(policySetResult(openshellRejection("refused"))); + vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "error").mockImplementation(() => {}); + }); + + it("removes the private directory when the policy document cannot be written (#9206)", () => { + const created: string[] = []; + vi.spyOn(fs, "mkdtempSync").mockImplementation(((prefix: string) => { + const dir = `${prefix}fault${created.length}`; + fs.mkdirSync(dir, { recursive: true, mode: 0o700 }); + created.push(dir); + return dir; + }) as unknown as typeof fs.mkdtempSync); + vi.spyOn(fs, "writeFileSync").mockImplementation(() => { + throw new Error("ENOSPC: no space left on device"); + }); + + const error = applyWeatherPreset(); + + expect(error).toBeInstanceOf(Error); + expect(created).toHaveLength(1); + expect(fs.existsSync(created[0] as string)).toBe(false); + }); + + it("names the directory that still holds the composed policy when cleanup fails (#9206)", () => { + const created: string[] = []; + const realMkdtemp = fs.mkdtempSync; + const realRmSync = fs.rmSync; + const cleanupRoot = realMkdtemp(path.join(os.tmpdir(), "nemoclaw-policy-test-")); + vi.spyOn(fs, "mkdtempSync").mockImplementation(((prefix: string) => { + const dir = (realMkdtemp as (p: string) => string)( + path.join(cleanupRoot, path.basename(prefix)), + ); + created.push(dir); + return dir; + }) as unknown as typeof fs.mkdtempSync); + vi.spyOn(fs, "rmSync").mockImplementation(() => { + throw new Error("EPERM: operation not permitted"); + }); + + try { + const error = applyWeatherPreset(); + + expect(created).toHaveLength(1); + expect((error as Error).message).toContain(created[0] as string); + expect((error as Error).message).toContain("EPERM: operation not permitted"); + } finally { + vi.restoreAllMocks(); + removeTemporaryDirectory(cleanupRoot, realRmSync); + } + }); + + it("reports a residual directory that removal left behind without an error (#9206)", () => { + const created: string[] = []; + const realMkdtemp = fs.mkdtempSync; + const realRmSync = fs.rmSync; + const cleanupRoot = realMkdtemp(path.join(os.tmpdir(), "nemoclaw-policy-test-")); + vi.spyOn(fs, "mkdtempSync").mockImplementation(((prefix: string) => { + const dir = (realMkdtemp as (p: string) => string)( + path.join(cleanupRoot, path.basename(prefix)), + ); + created.push(dir); + return dir; + }) as unknown as typeof fs.mkdtempSync); + // Removal reports success while the directory survives. + vi.spyOn(fs, "rmSync").mockImplementation(() => undefined); + + try { + const error = applyWeatherPreset(); + + expect(created).toHaveLength(1); + expect((error as Error).message).toContain(created[0] as string); + expect((error as Error).message).toContain("the path still exists"); + } finally { + vi.restoreAllMocks(); + removeTemporaryDirectory(cleanupRoot, realRmSync); + } + }); + + it("reports retained policy material even when the submission itself failed (#9206)", () => { + const created: string[] = []; + const realMkdtemp = fs.mkdtempSync; + const realRmSync = fs.rmSync; + const cleanupRoot = realMkdtemp(path.join(os.tmpdir(), "nemoclaw-policy-test-")); + vi.spyOn(fs, "mkdtempSync").mockImplementation(((prefix: string) => { + const dir = (realMkdtemp as (p: string) => string)( + path.join(cleanupRoot, path.basename(prefix)), + ); + created.push(dir); + return dir; + }) as unknown as typeof fs.mkdtempSync); + run.mockImplementation(() => { + throw new Error("spawn failed before any result"); + }); + vi.spyOn(fs, "rmSync").mockImplementation(() => { + throw new Error("EPERM: operation not permitted"); + }); + + try { + const error = applyWeatherPreset(); + + expect(created).toHaveLength(1); + expect((error as Error).message).toMatch(/still holds the composed sandbox policy/iu); + } finally { + vi.restoreAllMocks(); + removeTemporaryDirectory(cleanupRoot, realRmSync); + } + }); +}); diff --git a/src/lib/policy/policy-set-outcome.test.ts b/src/lib/policy/policy-set-outcome.test.ts new file mode 100644 index 00000000000..b46edda3ef5 --- /dev/null +++ b/src/lib/policy/policy-set-outcome.test.ts @@ -0,0 +1,211 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { describe, expect, it } from "vitest"; + +import { classifyPolicySetResult, type PolicySetOutcome } from "./policy-set-outcome"; + +/** + * Narrow to the ambiguous arm so a test can assert on `detail` in a straight + * line. A non-ambiguous outcome returns its kind, which fails the assertion + * with the arm that was actually produced. + */ +function ambiguousDetail(outcome: PolicySetOutcome): string { + return outcome.kind === "ambiguous" ? outcome.detail : `outcome kind: ${outcome.kind}`; +} + +/** + * Observed verbatim in issue #8991. The stream died mid-call, so the + * server-side state is unknown; the identical `code:`/`message:` rendering + * used by semantic diagnostics is exactly why this string must not be read + * as a refusal. + */ +const HTTP2_RESET_STDERR = + "Error: code: 'Internal error', message: 'h2 protocol error: http2 error', " + + "source: tonic::transport::Error(Transport, hyper::Error(Http2, " + + "Error { kind: Reset(StreamId(3), PROTOCOL_ERROR, Library) }))"; + +/** Synthetic gRPC status 9 frame accepted by the classifier as a refusal. */ +const SYNTHETIC_REJECTION_FRAME = + "Error: code: 'Failed precondition', message: 'network policy \"team-web\" rejected: " + + 'preset "slack" declares an egress host that conflicts with the sandbox baseline\', ' + + "source: tonic::Status { code: FailedPrecondition, grpc_status: 9 }"; + +const NON_SUCCESS_STATUSES = [ + { statusLabel: "exit status 1", status: 1 }, + { statusLabel: "exit status 2", status: 2 }, + { statusLabel: "exit status 9", status: 9 }, + { statusLabel: "exit status 13", status: 13 }, + { statusLabel: "exit status 14", status: 14 }, + { statusLabel: "exit status 127", status: 127 }, + { statusLabel: "an absent exit status", status: null }, +] as const; + +const NON_SUCCESS_DIAGNOSTICS = [ + { diagnosticLabel: "an accepted synthetic refusal frame", stderr: SYNTHETIC_REJECTION_FRAME }, + { diagnosticLabel: "an HTTP/2 reset", stderr: HTTP2_RESET_STDERR }, + { diagnosticLabel: "empty stderr", stderr: "" }, + { diagnosticLabel: "absent stderr", stderr: null }, + { + diagnosticLabel: "an unstructured transport error", + stderr: "openshell: connection refused", + }, +] as const; + +const NON_SUCCESS_CASES = NON_SUCCESS_STATUSES.flatMap(({ statusLabel, status }) => + NON_SUCCESS_DIAGNOSTICS.map(({ diagnosticLabel, stderr }) => ({ + statusLabel, + status, + diagnosticLabel, + stderr, + })), +); + +describe("policy set outcome classification", () => { + it("reports a clean exit with no error as applied (#9206)", () => { + expect(classifyPolicySetResult({ status: 0 })).toEqual({ kind: "applied" }); + }); + + it("preserves the status and message from an accepted synthetic refusal frame (#9206)", () => { + const outcome = classifyPolicySetResult({ status: 1, stderr: SYNTHETIC_REJECTION_FRAME }); + + expect(outcome).toEqual({ + kind: "rejected", + status: 1, + message: + 'network policy "team-web" rejected: preset "slack" declares an egress host ' + + "that conflicts with the sandbox baseline", + }); + }); + + it("treats an HTTP/2 stream reset as ambiguous rather than a refusal (#9206)", () => { + const outcome = classifyPolicySetResult({ status: 1, stderr: HTTP2_RESET_STDERR }); + + expect(outcome.kind).toBe("ambiguous"); + expect(ambiguousDetail(outcome)).toContain("h2 protocol error"); + expect(ambiguousDetail(outcome)).toContain("Reset(StreamId(3), PROTOCOL_ERROR, Library)"); + }); + + it("treats a spawn-level failure with no exit status as ambiguous (#9206)", () => { + const outcome = classifyPolicySetResult({ + status: null, + error: { message: "spawnSync openshell ENOENT" }, + }); + + expect(outcome.kind).toBe("ambiguous"); + expect(ambiguousDetail(outcome)).toContain("ENOENT"); + }); + + it("treats a nonzero exit with no diagnostic output as ambiguous (#9206)", () => { + expect(classifyPolicySetResult({ status: 1, stderr: "" }).kind).toBe("ambiguous"); + expect(classifyPolicySetResult({ status: 1 }).kind).toBe("ambiguous"); + expect(classifyPolicySetResult({ status: 1, stderr: " \n " }).kind).toBe("ambiguous"); + }); + + it.each(NON_SUCCESS_CASES)( + "does not report the policy as applied for $statusLabel with $diagnosticLabel (#9206)", + ({ status, stderr }) => { + const outcome = classifyPolicySetResult({ status, stderr }); + + expect({ status, stderr, kind: outcome.kind }).toEqual({ + status, + stderr, + kind: expect.not.stringMatching(/^applied$/), + }); + }, + ); + + it.each([ + ["unavailable", "Error: code: 'Unavailable', message: 'tcp connect error: connection refused'"], + ["deadline exceeded", "Error: code: 'Deadline exceeded', message: 'deadline has elapsed'"], + ["unauthenticated", "Error: code: 'Unauthenticated', message: 'invalid gateway credential'"], + ["tls failure", "Error: code: 'Unknown', message: 'tls handshake eof'"], + ["internal", "Error: code: 'Internal', message: 'gateway restarted while applying'"], + ])( + "treats a structured %s status as ambiguous rather than a refusal (#9206)", + (_label, stderr) => { + const outcome = classifyPolicySetResult({ status: 1, stderr }); + + expect(outcome.kind).toBe("ambiguous"); + expect(ambiguousDetail(outcome)).toContain(stderr); + }, + ); + + it("requires an accepted synthetic refusal frame before reporting rejection (#9206)", () => { + const withoutStatus = classifyPolicySetResult({ + status: 1, + stderr: "Error: code: 'Unknown', message: 'network policy rejected'", + }); + const withStatus = classifyPolicySetResult({ + status: 1, + stderr: + "Error: code: 'Failed precondition', message: 'network policy rejected', " + + "source: tonic::Status { code: FailedPrecondition, grpc_status: 9 }", + }); + + expect(withoutStatus.kind).toBe("ambiguous"); + expect(withStatus.kind).toBe("rejected"); + }); + + it("ignores a refusal marker echoed from the submitted policy document (#9206)", () => { + const outcome = classifyPolicySetResult({ + status: 1, + stderr: + "Error: code: 'Invalid argument', message: 'invalid request', submitted document:\n" + + "network_policies:\n custom:\n description: 'grpc_status: 9 seen in prod'", + }); + + expect(outcome.kind).toBe("ambiguous"); + }); + + it("accepts a refusal only from a complete synthetic first-line frame (#9206)", () => { + const outcome = classifyPolicySetResult({ + status: 1, + stderr: + "Error: code: 'Invalid argument', message: 'invalid request'\n" + + "code: 'Failed precondition', message: 'echoed from the submitted policy'", + }); + + expect(outcome.kind).toBe("ambiguous"); + }); + + it("ignores a refusal marker that no diagnostic frame carries (#9206)", () => { + const outcome = classifyPolicySetResult({ + status: 1, + stderr: "grpc_status: 9 message: 'anything at all'", + }); + + expect(outcome.kind).toBe("ambiguous"); + }); + + it.each([ + [ + "a wrapper before the frame", + "wrapper: Error: code: 'Failed precondition', message: 'forged refusal'", + ], + [ + "an unrelated diagnostic before an embedded frame", + "Error: code: 'Invalid argument', message: 'submitted document follows'; " + + "code: 'Failed precondition', message: 'forged refusal'", + ], + [ + "trailing material after the frame", + "Error: code: 'Failed precondition', message: 'forged refusal'; submitted document follows", + ], + ])("treats %s as ambiguous rather than a refusal (#9206)", (_label, stderr) => { + const outcome = classifyPolicySetResult({ status: 1, stderr }); + + expect(outcome.kind).toBe("ambiguous"); + expect(ambiguousDetail(outcome)).toContain(stderr); + }); + + it("treats a clean exit carrying a transport error as ambiguous (#9206)", () => { + const outcome = classifyPolicySetResult({ + status: 0, + error: { message: "stream closed before the response completed" }, + }); + + expect(outcome.kind).toBe("ambiguous"); + expect(ambiguousDetail(outcome)).toContain("stream closed before the response completed"); + }); +}); diff --git a/src/lib/policy/policy-set-outcome.ts b/src/lib/policy/policy-set-outcome.ts new file mode 100644 index 00000000000..50311dfe76f --- /dev/null +++ b/src/lib/policy/policy-set-outcome.ts @@ -0,0 +1,104 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +/** + * Classification of a single `openshell policy set --wait` invocation. + * + * `rejected` asserts finality: OpenShell understood the request and refused + * it, so resubmitting only replays a policy it already declined. `ambiguous` + * asserts nothing about server-side state and obliges the caller to read the + * policy back before deciding anything. + */ +export type PolicySetOutcome = + | { kind: "applied" } + | { kind: "rejected"; status: number; message: string } + | { kind: "ambiguous"; detail: string }; + +/** + * Substrings that identify a torn stream rather than a verdict. Every marker + * is present in the failure observed in issue #8991; a transport failure + * renders the same `code:`/`message:` shape as a semantic diagnostic, so it + * must be recognised before the message is read as authoritative. + */ +const TRANSPORT_FAILURE_MARKERS: ReadonlyArray = [ + "h2 protocol error", + "http2 error", + "tonic::transport::error", +]; + +/** + * The one diagnostic shape the classifier accepts as a final refusal: the + * complete synthetic failed-precondition envelope on the first line. + * + * Status evidence and message must come from the SAME match. Scanning for + * them separately let a marker echoed anywhere in the output promote an + * unrelated failure into a refusal, because OpenShell can quote the submitted + * policy back and that document carries operator-supplied text. A + * `Deadline exceeded` whose echoed policy mentioned the marker was reported as + * final, which is the one verdict a deadline can never support. + */ +const AUTHORITATIVE_REFUSAL_PATTERN = + /^Error:\s+code:\s*'failed[ _]precondition',\s*message:\s*'([^'\r\n]+)'(?:,\s*source:\s*tonic::Status\s*\{\s*code:\s*FailedPrecondition,\s*grpc_status:\s*9\s*\})?\s*$/iu; + +function isTransportFailure(detail: string): boolean { + const haystack = detail.toLowerCase(); + return TRANSPORT_FAILURE_MARKERS.some((marker) => haystack.includes(marker)); +} + +function combineDetail( + error: { message?: string } | null | undefined, + stderr: string | null | undefined, +): string { + return [error?.message, stderr] + .map((part) => part?.trim() ?? "") + .filter((part) => part.length > 0) + .join("\n"); +} + +/** + * Return the message of a refusal frame accepted as authoritative, or null. + * + * Only the first line is read. The classifier accepts a verdict only there; + * anything after it can be quoted policy content, which would otherwise let a + * document carrying a refusal frame speak for the gateway. + */ +function authoritativeRefusalMessage(stderr: string | null | undefined): string | null { + const reportedDiagnostic = stderr?.split("\n", 1)[0] ?? ""; + const matched = reportedDiagnostic.match(AUTHORITATIVE_REFUSAL_PATTERN)?.[1]?.trim(); + return matched ? matched : null; +} + +/** + * Classify the result of `openshell policy set`. Pure: it inspects only the + * values handed to it. + * + * Uncertainty resolves to `ambiguous`, which costs a readback. Reporting a + * refusal we cannot evidence, or success we cannot prove, is not recoverable, + * so a nonzero or absent status never yields `applied`. + */ +export function classifyPolicySetResult(input: { + status: number | null; + error?: { message?: string } | null; + stderr?: string | null; +}): PolicySetOutcome { + const detail = combineDetail(input.error, input.stderr); + const ambiguous = (): PolicySetOutcome => ({ + kind: "ambiguous", + detail: detail || `openshell policy set exited with status ${String(input.status)}`, + }); + + // A torn stream leaves the server-side state unknown whatever else is set. + if (isTransportFailure(detail)) return ambiguous(); + + if (input.status === 0) { + // A clean exit alongside a transport error is not proof of application. + return input.error ? ambiguous() : { kind: "applied" }; + } + + // `spawnSync` reports ENOENT and timeouts as a null status: the command may + // never have run, or may have run and lost its result. + if (input.status === null) return ambiguous(); + + const message = authoritativeRefusalMessage(input.stderr); + return message === null ? ambiguous() : { kind: "rejected", status: input.status, message }; +} diff --git a/test/helpers/vitest-watch-triggers.ts b/test/helpers/vitest-watch-triggers.ts index 5c38a73a6ed..1954fd33fc0 100644 --- a/test/helpers/vitest-watch-triggers.ts +++ b/test/helpers/vitest-watch-triggers.ts @@ -40,6 +40,10 @@ function runTests(...tests: string[]): () => string[] { } export const vitestWatchTriggerPatterns: VitestWatchTriggerPattern[] = [ + { + pattern: /(?:^|\/)docs\/reference\/troubleshooting\.mdx$/, + testsToRun: runTests("test/policy-finality-docs.test.ts"), + }, { pattern: /(?:^|\/)(?:\.github\/workflows\/release-lkg-brev-image\.yaml|scripts\/release-lkg-brev-image\.sh)$/, diff --git a/test/mcp-policy-key-ownership.test.ts b/test/mcp-policy-key-ownership.test.ts index 9b0d03ad1f1..8ab9d742eda 100644 --- a/test/mcp-policy-key-ownership.test.ts +++ b/test/mcp-policy-key-ownership.test.ts @@ -259,7 +259,8 @@ describe("MCP-generated network policy ownership", () => { expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); expect(result.stdout).toContain('__RESULT__{"result":false,"policies":[]}'); - expect(result.stderr).toContain("Failed to update policy"); + expect(result.stderr).toContain("Could not confirm the policy update"); + expect(result.stderr).toContain("read the current policy back before retrying"); }); it("preserves MCP policy ownership state when policy removal fails", () => { @@ -267,19 +268,23 @@ describe("MCP-generated network policy ownership", () => { expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); expect(result.stdout).toContain('__RESULT__{"result":false,"policies":["mcp-bridge-example"]}'); - expect(result.stderr).toContain("Failed to update policy"); + expect(result.stderr).toContain("Could not confirm the policy update"); + expect(result.stderr).toContain("read the current policy back before retrying"); }); it.each([ [false, []], [true, ["mcp-bridge-example"]], - ] as const)("supports ownership-preserving policy removal (skipRegistryUpdate=%s)", (skipRegistryUpdate, expectedPolicies) => { - const result = runSuccessfulPolicyRemoval(skipRegistryUpdate); - expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); - expect(result.stdout).toContain( - `__RESULT__${JSON.stringify({ result: true, policies: expectedPolicies })}`, - ); - }); + ] as const)( + "supports ownership-preserving policy removal (skipRegistryUpdate=%s)", + (skipRegistryUpdate, expectedPolicies) => { + const result = runSuccessfulPolicyRemoval(skipRegistryUpdate); + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain( + `__RESULT__${JSON.stringify({ result: true, policies: expectedPolicies })}`, + ); + }, + ); it("does not delete an operator-owned same-key policy when add rolls back", () => { const home = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-mcp-policy-lifecycle-")); diff --git a/test/policy-finality-docs.test.ts b/test/policy-finality-docs.test.ts new file mode 100644 index 00000000000..c966af1f959 --- /dev/null +++ b/test/policy-finality-docs.test.ts @@ -0,0 +1,142 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { describe, expect, it } from "vitest"; + +const repoRoot = path.join(import.meta.dirname, ".."); +const troubleshootingPath = path.join(repoRoot, "docs", "reference", "troubleshooting.mdx"); + +function documentedCleanupCommand(): string { + const markdown = fs.readFileSync(troubleshootingPath, "utf-8"); + const section = markdown.slice( + markdown.indexOf("### Onboarding Reports a Rejected or Unconfirmed Policy Update"), + ); + const block = section.match( + /```bash\n(cleanup_retained_policy\(\) \{[\s\S]*?\ncleanup_retained_policy)\n```/u, + ); + expect(block).not.toBeNull(); + return block?.[1] ?? ""; +} + +function documentedCleanupPython(): string { + const command = documentedCleanupCommand(); + const block = command.match(/<<'PY'\n([\s\S]*?)\nPY/u); + expect(block).not.toBeNull(); + return block?.[1] ?? ""; +} + +function runDocumentedCleanup(reportedPath: string, envOverrides: NodeJS.ProcessEnv = {}) { + return spawnSync("bash", ["--noprofile", "--norc", "-c", documentedCleanupCommand()], { + encoding: "utf-8", + env: { ...process.env, ...envOverrides }, + input: `${reportedPath}\n`, + }); +} + +describe("retained policy cleanup documentation", () => { + it("removes policy material from the exact reported directory under the temp root (#9206)", () => { + const retainedDirectory = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-")); + try { + fs.writeFileSync(path.join(retainedDirectory, "policy.yaml"), "secret policy material"); + + const result = runDocumentedCleanup(retainedDirectory); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(fs.existsSync(path.join(retainedDirectory, "policy.yaml"))).toBe(false); + expect(fs.existsSync(retainedDirectory)).toBe(true); + expect(result.stdout).toContain("The empty directory intentionally remains"); + } finally { + fs.rmSync(retainedDirectory, { force: true, recursive: true }); + } + }); + + it("rejects a policy-shaped directory outside the platform temp root (#9206)", () => { + const result = runDocumentedCleanup("/home/operator/nemoclaw-policy-forged"); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("direct child of the actual platform temporary directory"); + }); + + it("rejects a platform temp-root alias that resolves to the filesystem root (#9206)", () => { + const canonicalTempRoot = fs.realpathSync(os.tmpdir()); + const rootChild = canonicalTempRoot.split(path.sep).filter(Boolean)[0]; + const rootAlias = `${path.parse(canonicalTempRoot).root}${rootChild}/..`; + const result = runDocumentedCleanup(`${rootAlias}/nemoclaw-policy-forged`, { + TMPDIR: rootAlias, + }); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("platform temporary directory cannot be the filesystem root"); + }); + + it("rejects a symbolic link at a reported temp path (#9206)", () => { + const retainedDirectory = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-target-")); + const reportedPath = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-link-")); + fs.rmdirSync(reportedPath); + fs.symlinkSync(retainedDirectory, reportedPath); + try { + const result = runDocumentedCleanup(reportedPath); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("canonical path is outside"); + expect(fs.existsSync(retainedDirectory)).toBe(true); + } finally { + fs.rmSync(reportedPath, { force: true, recursive: true }); + fs.rmSync(retainedDirectory, { force: true, recursive: true }); + } + }); + + it("does not remove a replacement directory after validation (#9206)", () => { + const retainedDirectory = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-policy-race-")); + const displacedDirectory = `${retainedDirectory}-validated`; + const replacementPolicy = path.join(retainedDirectory, "policy.yaml"); + fs.writeFileSync(path.join(retainedDirectory, "policy.yaml"), "validated policy material"); + const driver = ` +import os +import sys + +namespace = {"__name__": "documented_cleanup"} +exec(os.environ["DOCUMENTED_CLEANUP_PYTHON"], namespace) +platform_tmp_root, reported_path, displaced_path = sys.argv[1:4] + +remove_retained_policy_material = namespace["remove_retained_policy_material"] + + +def replace_then_remove(policy_fd): + os.rename(reported_path, displaced_path) + os.mkdir(reported_path) + with open(os.path.join(reported_path, "policy.yaml"), "w", encoding="utf-8") as replacement: + replacement.write("replacement policy material") + remove_retained_policy_material(policy_fd) + + +namespace["remove_retained_policy_material"] = replace_then_remove +sys.argv = ["documented-cleanup", platform_tmp_root, reported_path] +namespace["main"]() +`; + try { + const result = spawnSync( + "python3", + ["-c", driver, os.tmpdir(), retainedDirectory, displacedDirectory], + { + encoding: "utf-8", + env: { ...process.env, DOCUMENTED_CLEANUP_PYTHON: documentedCleanupPython() }, + }, + ); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(1); + expect(result.stderr).toContain( + "reported path was replaced during cleanup; the replacement was not removed", + ); + expect(fs.readFileSync(replacementPolicy, "utf-8")).toBe("replacement policy material"); + expect(fs.existsSync(path.join(displacedDirectory, "policy.yaml"))).toBe(false); + } finally { + fs.rmSync(retainedDirectory, { force: true, recursive: true }); + fs.rmSync(displacedDirectory, { force: true, recursive: true }); + } + }); +}); diff --git a/test/portable-policy-failure-finality.test.ts b/test/portable-policy-failure-finality.test.ts new file mode 100644 index 00000000000..6c6ec8dab7c --- /dev/null +++ b/test/portable-policy-failure-finality.test.ts @@ -0,0 +1,412 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { type SpawnSyncReturns, spawnSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; + +const repoRoot = path.join(import.meta.dirname, ".."); +const policyModulePath = path.join(repoRoot, "src", "lib", "policy", "index.ts"); +const registryModulePath = path.join(repoRoot, "src", "lib", "state", "registry.ts"); + +/** + * Distinctive network policy key carried by the stubbed `openshell policy get + * --base` output. It survives the preset merge, so finding it on disk after the + * child exits proves the composed sandbox policy itself leaked. + */ +const CANARY_POLICY_NAME = "leak-canary-9206"; +const CANARY_HOST = "canary-9206.invalid"; +const SANDBOX_NAME = "policy-final-9206"; +const PRESET_NAME = "weather"; + +/** Marks a driver call that returned instead of exiting. */ +const RETURN_MARKER = "__POLICY_MUTATION_RETURNED__"; + +/** The canary entry every stubbed base policy carries. */ +const CANARY_ENTRY = ` ${CANARY_POLICY_NAME}: + name: ${CANARY_POLICY_NAME} + endpoints: + - host: ${CANARY_HOST} + port: 443 + protocol: rest + enforcement: enforce + rules: + - allow: { method: GET, path: "/**" }`; + +/** A base policy the weather preset has not been applied to yet. */ +const BASE_POLICY_WITHOUT_PRESET = `version: 1 +network_policies: +${CANARY_ENTRY}`; + +/** + * A base policy that already carries the weather key, so `removePreset` finds + * something to delete and reaches its gateway submission. + */ +const BASE_POLICY_WITH_PRESET = `${BASE_POLICY_WITHOUT_PRESET} + ${PRESET_NAME}: + name: ${PRESET_NAME} + endpoints: + - host: wttr.in + port: 443 + protocol: rest + enforcement: enforce + rules: + - allow: { method: GET, path: "/**" }`; + +/** + * A synthetic `policy set` failure without an authoritative `message:` field, + * so it classifies as `ambiguous`. The nonzero status is deliberately not 1: + * the process must still exit with the status the child reported. + */ +const UNPARSEABLE_FAILURE_STDERR = "openshell: policy set: unexpected end of stream"; +const UNPARSEABLE_FAILURE_EXIT_CODE = 3; + +/** + * The synthetic refusal fixture puts the status and message together on the + * first diagnostic line. That shape is necessary but not sufficient: the same + * text appearing anywhere later in the output is treated as quoted policy + * content and stays `ambiguous`, so a document echoed back cannot speak for the + * gateway (#9206). + */ +const AUTHORITATIVE_REJECTION_MESSAGE = "unsupported field in network_policies.weather"; +const AUTHORITATIVE_REJECTION_STDERR = `Error: code: 'Failed precondition', message: '${AUTHORITATIVE_REJECTION_MESSAGE}'`; + +/** The torn gateway stream observed in issue #8991, verbatim. */ +const TRANSPORT_RESET_STDERR = + "Error: code: 'Internal error', message: 'h2 protocol error: http2 error', " + + "source: tonic::transport::Error(Transport, hyper::Error(Http2, " + + "Error { kind: Reset(StreamId(3), PROTOCOL_ERROR, Library) }))"; + +/** + * The generic runner diagnostic. `policy set` runs with `ignoreError`, so this + * text appearing would mean the submission took the runner's `process.exit` + * path again — the path that skips the cleanup `finally` (#9206). + */ +const GENERIC_RUNNER_FAILURE_TEXT = "Command failed (exit 1)"; + +interface PolicySetBehavior { + readonly exitCode: number; + readonly stderr: string; +} + +/** + * Stands in for the OpenShell CLI. `policy get --base` always succeeds with a + * round-trippable base policy so the driven mutation reaches its submission; + * the `policy set --wait` result is what each scenario varies. + */ +function buildOpenshellStub(policySet: PolicySetBehavior, basePolicy: string): string { + return `#!/bin/sh +if [ "$1" = "policy" ] && [ "$2" = "get" ]; then + cat <<'YAML' +${basePolicy} +YAML + exit 0 +fi +if [ "$1" = "policy" ] && [ "$2" = "set" ]; then + cat >&2 <<'POLICY_SET_STDERR' +${policySet.stderr} +POLICY_SET_STDERR + exit ${policySet.exitCode} +fi +echo "unexpected openshell argv: $*" >&2 +exit 2 +`; +} + +/** + * Drives one real exported policy mutation in a child process. Stubbing + * `process.exit` in-process would let execution fall through to the `finally` + * block and hide the leak, so the child must take a real `process.exit`. + */ +function buildDriver(call: string): string { + return `const policy = require(${JSON.stringify(policyModulePath)}); +const returned = policy.${call}; +console.log(${JSON.stringify(RETURN_MARKER)} + String(returned)); +`; +} + +const APPLY_PRESETS_DRIVER = + `const registry = require(${JSON.stringify(registryModulePath)});\n` + + `registry.registerSandbox({ name: ${JSON.stringify(SANDBOX_NAME)}, agent: "openclaw", policies: [] });\n` + + buildDriver(`applyPresets(${JSON.stringify(SANDBOX_NAME)}, [${JSON.stringify(PRESET_NAME)}])`); + +/** + * `removePreset` and `applyPreset` bypass the batch path that `applyPresets` + * takes, and each composes its own policy document through its own temp file. + */ +const REMOVE_PRESET_DRIVER = buildDriver( + `removePreset(${JSON.stringify(SANDBOX_NAME)}, ${JSON.stringify(PRESET_NAME)})`, +); +const APPLY_PRESET_DRIVER = buildDriver( + `applyPreset(${JSON.stringify(SANDBOX_NAME)}, ${JSON.stringify(PRESET_NAME)})`, +); + +interface ChildRun { + readonly result: SpawnSyncReturns; + readonly homeDir: string; + readonly tmpDir: string; +} + +function listNemoclawPolicyDirs(tmpDir: string): string[] { + return fs + .readdirSync(tmpDir) + .filter((entry) => entry.startsWith("nemoclaw-policy-")) + .map((entry) => path.join(tmpDir, entry)) + .sort(); +} + +function listFilesRecursively(dir: string): string[] { + return fs + .readdirSync(dir, { withFileTypes: true }) + .filter((entry) => !entry.name.startsWith("tsx-")) + .flatMap((entry) => { + const full = path.join(dir, entry.name); + return entry.isDirectory() ? listFilesRecursively(full) : [full]; + }); +} + +function readableFilesContaining(dir: string, needle: string): string[] { + return listFilesRecursively(dir).filter((file) => + fs.readFileSync(file, "utf-8").includes(needle), + ); +} + +/** + * Vitest source-maps every stack frame it finds in an assertion message, so + * quoting a child stack trace verbatim replaces the real failure with a + * source-map parse error. Keep child output frame-free in test messages. + */ +function withoutStackFrames(text: string): string { + return text + .split("\n") + .filter((line) => !/^\s+at\s/.test(line)) + .join("\n"); +} + +interface PolicyMutationRun { + readonly driver: string; + readonly policySet: PolicySetBehavior; + readonly basePolicy: string; +} + +function runPolicyMutation({ driver, policySet, basePolicy }: PolicyMutationRun): ChildRun { + const scratchDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-9206-fixture-")); + const tmpDir = path.join(scratchDir, "child-tmp"); + const homeDir = path.join(scratchDir, "home"); + fs.mkdirSync(tmpDir); + fs.mkdirSync(homeDir); + + const openshellStubPath = path.join(scratchDir, "openshell"); + fs.writeFileSync(openshellStubPath, buildOpenshellStub(policySet, basePolicy), { + encoding: "utf-8", + mode: 0o755, + }); + + const driverPath = path.join(scratchDir, "driver.js"); + fs.writeFileSync(driverPath, driver, "utf-8"); + + const result = spawnSync(process.execPath, ["--import", "tsx", driverPath], { + cwd: repoRoot, + encoding: "utf-8", + env: { + ...process.env, + HOME: homeDir, + NEMOCLAW_NON_INTERACTIVE: "1", + NEMOCLAW_OPENSHELL_BIN: openshellStubPath, + TMPDIR: tmpDir, + }, + timeout: 120000, + }); + + return { homeDir, result, tmpDir }; +} + +function runApplyPresets(policySet: PolicySetBehavior): ChildRun { + return runPolicyMutation({ + basePolicy: BASE_POLICY_WITHOUT_PRESET, + driver: APPLY_PRESETS_DRIVER, + policySet, + }); +} + +function describeChildRun(run: ChildRun): string { + return `exit=${run.result.status}\n--stderr--\n${withoutStackFrames(run.result.stderr)}`; +} + +interface PolicySetFailureScenario { + /** Names the scenario in the `describe.each` title. */ + readonly summary: string; + readonly policySetExitCode: number; + readonly policySetStderr: string; + /** Text only `policySetFailure` can produce, never the stub's own stderr. */ + readonly expectedOperatorMessage: string; + /** The recovery instruction the classification obliges the operator to follow. */ + readonly expectedGuidance: string; +} + +const POLICY_SET_FAILURES: ReadonlyArray = [ + { + summary: "an authoritative semantic rejection", + policySetExitCode: 1, + policySetStderr: AUTHORITATIVE_REJECTION_STDERR, + expectedOperatorMessage: + `OpenShell rejected the policy for sandbox '${SANDBOX_NAME}' (exit 1): ` + + AUTHORITATIVE_REJECTION_MESSAGE, + expectedGuidance: "change the preset selection instead", + }, + { + summary: "a torn transport stream", + policySetExitCode: UNPARSEABLE_FAILURE_EXIT_CODE, + policySetStderr: TRANSPORT_RESET_STDERR, + expectedOperatorMessage: `Could not confirm the policy update for sandbox '${SANDBOX_NAME}'`, + expectedGuidance: "read the current policy back before retrying", + }, +]; + +describe.each(POLICY_SET_FAILURES)( + "applyPresets when openshell policy set fails with $summary", + (scenario) => { + let run: ChildRun; + + beforeAll(() => { + run = runApplyPresets({ + exitCode: scenario.policySetExitCode, + stderr: scenario.policySetStderr, + }); + }, 180000); + + afterAll(() => { + fs.rmSync(path.dirname(run.tmpDir), { force: true, recursive: true }); + }); + + it("reports the OpenShell status and message and exits nonzero without returning to its caller (#9206)", () => { + const diagnostics = describeChildRun(run); + const stderr = withoutStackFrames(run.result.stderr); + expect(stderr, diagnostics).toContain(scenario.expectedOperatorMessage); + expect(stderr, diagnostics).toContain(scenario.expectedGuidance); + expect(stderr, diagnostics).not.toContain(GENERIC_RUNNER_FAILURE_TEXT); + expect(run.result.status, diagnostics).toBe(scenario.policySetExitCode); + expect(run.result.stdout, diagnostics).not.toContain(RETURN_MARKER); + }); + + it("removes the temporary policy directory it created (#9206)", () => { + expect(listNemoclawPolicyDirs(run.tmpDir), describeChildRun(run)).toEqual([]); + }); + + it("leaves no composed sandbox policy content on disk (#9206)", () => { + expect(readableFilesContaining(run.tmpDir, CANARY_HOST), describeChildRun(run)).toEqual([]); + expect(readableFilesContaining(run.tmpDir, "wttr.in"), describeChildRun(run)).toEqual([]); + }); + + it("leaves local preset attribution unwritten (#9206)", () => { + const registry = JSON.parse( + fs.readFileSync(path.join(run.homeDir, ".nemoclaw", "sandboxes.json"), "utf-8"), + ) as { sandboxes: Record }; + expect(registry.sandboxes[SANDBOX_NAME]?.policies).toEqual([]); + }); + }, +); + +describe("applyPresets when openshell policy set succeeds", () => { + let run: ChildRun; + + beforeAll(() => { + run = runApplyPresets({ exitCode: 0, stderr: "" }); + }, 180000); + + afterAll(() => { + fs.rmSync(path.dirname(run.tmpDir), { force: true, recursive: true }); + }); + + it("returns to its caller and exits zero (#9206)", () => { + const diagnostics = describeChildRun(run); + expect(run.result.stdout, diagnostics).toContain(`${RETURN_MARKER}true`); + expect(run.result.status, diagnostics).toBe(0); + }); + + it("removes the temporary policy directory it created (#9206)", () => { + expect(listNemoclawPolicyDirs(run.tmpDir), describeChildRun(run)).toEqual([]); + }); + + it("leaves no composed sandbox policy content on disk (#9206)", () => { + expect(readableFilesContaining(run.tmpDir, CANARY_HOST), describeChildRun(run)).toEqual([]); + expect(readableFilesContaining(run.tmpDir, "wttr.in"), describeChildRun(run)).toEqual([]); + }); +}); + +/** + * The single-preset mutations onboarding reaches for a deselected preset and + * for a preset the batch path does not cover. Neither goes through + * `applyPresets`, so each needs its own proof that a failed submission takes + * the composed policy with it. + */ +interface SinglePresetMutationScenario { + /** Names the scenario in the `describe.each` title. */ + readonly summary: string; + readonly driver: string; + readonly basePolicy: string; + readonly policySet: PolicySetBehavior; + /** Text only the policy-set failure reporting can produce. */ + readonly expectedOperatorMessage: string; + readonly expectedGuidance: string; + /** The status the child must exit with, matching what OpenShell reported. */ + readonly expectedExitCode: number; +} + +const SINGLE_PRESET_MUTATIONS: ReadonlyArray = [ + { + summary: "removePreset and openshell rejects the narrowed policy", + driver: REMOVE_PRESET_DRIVER, + basePolicy: BASE_POLICY_WITH_PRESET, + policySet: { exitCode: 1, stderr: AUTHORITATIVE_REJECTION_STDERR }, + expectedOperatorMessage: + `OpenShell rejected the policy for sandbox '${SANDBOX_NAME}' (exit 1): ` + + AUTHORITATIVE_REJECTION_MESSAGE, + expectedGuidance: "change the preset selection instead", + expectedExitCode: 1, + }, + { + summary: "applyPreset and the widened policy submission is unconfirmed", + driver: APPLY_PRESET_DRIVER, + basePolicy: BASE_POLICY_WITHOUT_PRESET, + policySet: { exitCode: UNPARSEABLE_FAILURE_EXIT_CODE, stderr: UNPARSEABLE_FAILURE_STDERR }, + expectedOperatorMessage: `Could not confirm the policy update for sandbox '${SANDBOX_NAME}'`, + expectedGuidance: "read the current policy back before retrying", + expectedExitCode: UNPARSEABLE_FAILURE_EXIT_CODE, + }, +]; + +describe.each(SINGLE_PRESET_MUTATIONS)("$summary", (scenario) => { + let run: ChildRun; + + beforeAll(() => { + run = runPolicyMutation({ + basePolicy: scenario.basePolicy, + driver: scenario.driver, + policySet: scenario.policySet, + }); + }, 180000); + + afterAll(() => { + fs.rmSync(path.dirname(run.tmpDir), { force: true, recursive: true }); + }); + + it("reports the OpenShell result and exits with its status without returning to its caller (#9206)", () => { + const diagnostics = describeChildRun(run); + const stderr = withoutStackFrames(run.result.stderr); + expect(stderr, diagnostics).toContain(scenario.expectedOperatorMessage); + expect(stderr, diagnostics).toContain(scenario.expectedGuidance); + expect(run.result.status, diagnostics).toBe(scenario.expectedExitCode); + expect(run.result.stdout, diagnostics).not.toContain(RETURN_MARKER); + }); + + it("removes the temporary policy directory it created (#9206)", () => { + expect(listNemoclawPolicyDirs(run.tmpDir), describeChildRun(run)).toEqual([]); + }); + + it("leaves no composed sandbox policy content on disk (#9206)", () => { + expect(readableFilesContaining(run.tmpDir, CANARY_HOST), describeChildRun(run)).toEqual([]); + }); +});