diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index d47fef8638a..b0557a3a66b 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2068,6 +2068,11 @@ $$nemoclaw my-assistant mcp remove github [--force] Deploy a skill directory to a running sandbox. The command validates the `SKILL.md` frontmatter (a `name` field is required), uploads all non-dot files preserving subdirectory structure, and performs agent-specific post-install steps. +NemoClaw selects one OpenShell sandbox execution transport before it changes the skill. +It uses the named gateway's direct gRPC API for plaintext, mTLS, and OIDC gateways. +It preselects the OpenShell CLI only when Cloudflare edge-tunnel authentication requires the CLI-owned tunnel. +Any other direct-client configuration error stops the command before dispatch. +After dispatch starts, NemoClaw never retries the mutation through another transport. ```bash $$nemoclaw my-assistant skill install ./my-skill/ @@ -2111,6 +2116,7 @@ For new installs, the agent session index is refreshed so the agent discovers th Remove an installed skill from a running sandbox by skill name. The command validates the skill name, removes the sandbox upload directory, and refreshes the agent session index so the remaining skills are rediscovered on the next session. +It uses the same preselected, fail-closed OpenShell control path as `skill install`. @@ -2195,7 +2201,11 @@ Pass `-f` / `--file ` to point at the manifest; `--yes` confirms th List OpenClaw conversation sessions in the sandbox. With no subcommand the in-sandbox CLI lists stored sessions for the configured default agent. -NemoClaw invokes `openclaw sessions` via `openshell sandbox exec` and forwards OpenClaw flags verbatim, but filters default list output so internal `nemoclaw-onboard-warmup-*` sessions created during onboarding are hidden from user-facing output. +NemoClaw invokes the read-only `openclaw sessions` command through the sandbox's named OpenShell gateway and forwards OpenClaw flags verbatim. +It prefers the direct gRPC sandbox execution API. +If gateway configuration or the gRPC transport fails before NemoClaw receives a command result, it retries this read-only list operation through the OpenShell CLI. +A completed command, including a nonzero result, is never replayed during fallback. +NemoClaw filters default list output so internal `nemoclaw-onboard-warmup-*` sessions created during onboarding are hidden from user-facing output. ```bash $$nemoclaw my-assistant sessions diff --git a/scripts/checks/legacy-sandbox-transports.ts b/scripts/checks/legacy-sandbox-transports.ts index afcf4fb9278..b3ee4d654b7 100644 --- a/scripts/checks/legacy-sandbox-transports.ts +++ b/scripts/checks/legacy-sandbox-transports.ts @@ -39,8 +39,6 @@ const REVIEWED_SITE_TUPLES = [ ["src/lib/actions/sandbox/process-recovery.ts", "privileged-sandbox-exec", 2], ["src/lib/actions/sandbox/process-recovery.ts", "ssh-command", 1], ["src/lib/actions/sandbox/process-recovery.ts", "ssh-temp-config", 1], - ["src/lib/actions/sandbox/skill-install.ts", "openshell-ssh-config", 2], - ["src/lib/actions/sandbox/skill-install.ts", "ssh-temp-config", 2], ["src/lib/actions/sandbox/snapshot.ts", "docker-exec-command", 1], ["src/lib/adapters/docker/container.ts", "docker-exec-command", 1], ["src/lib/adapters/openshell/client.ts", "openshell-ssh-config", 1], @@ -57,7 +55,6 @@ const REVIEWED_SITE_TUPLES = [ ["src/lib/share-command.ts", "sshfs-command", 1], ["src/lib/shields/index.ts", "privileged-sandbox-exec", 4], ["src/lib/shields/mutable-config-repair.ts", "privileged-sandbox-exec", 2], - ["src/lib/skill-remote.ts", "ssh-command", 1], ["src/lib/state/openclaw-config-restore-input.ts", "ssh-command", 1], ["src/lib/state/openclaw-plugin-restore.ts", "ssh-command", 2], ["src/lib/state/openclaw-plugin-restore.ts", "ssh-temp-config", 1], diff --git a/src/lib/actions/sandbox/skill-install.test.ts b/src/lib/actions/sandbox/skill-install.test.ts index 9f1dba812de..cde0d6302ae 100644 --- a/src/lib/actions/sandbox/skill-install.test.ts +++ b/src/lib/actions/sandbox/skill-install.test.ts @@ -6,7 +6,10 @@ import os from "node:os"; import path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -const captureSandboxSshConfig = vi.hoisted(() => vi.fn()); +const selectOpenShellSandboxControlForMutation = vi.hoisted(() => vi.fn()); +const closeControl = vi.hoisted(() => vi.fn()); +const control = vi.hoisted(() => ({ exec: vi.fn() })); +const getSandbox = vi.hoisted(() => vi.fn()); const getSessionAgent = vi.hoisted(() => vi.fn()); const ensureLiveSandboxOrExit = vi.hoisted(() => vi.fn()); const skillInstall = vi.hoisted(() => ({ @@ -22,10 +25,12 @@ const skillInstall = vi.hoisted(() => ({ verifyInstall: vi.fn(), })); -vi.mock("../../adapters/openshell/runtime", () => ({ - captureSandboxSshConfig, +vi.mock("../../adapters/openshell/sandbox-control-routing", () => ({ + selectOpenShellSandboxControlForMutation, })); +vi.mock("../../state/registry", () => ({ getSandbox })); + vi.mock("../../agent/runtime", () => ({ getSessionAgent, })); @@ -57,14 +62,6 @@ function restoreExitCode(previousExitCode: typeof process.exitCode): void { process.exitCode = previousExitCode; } -function expectTempSshConfigCleanedUp(configFile: string): void { - const configDir = path.dirname(configFile); - expect(configDir).not.toBe(os.tmpdir()); - expect(path.basename(configDir)).toMatch(/^nemoclaw-ssh-skill-/); - expect(path.basename(configFile)).toBe("ssh_config"); - expect(fs.existsSync(configDir)).toBe(false); -} - describe("sandbox skill action orchestration", () => { let previousExitCode: typeof process.exitCode; @@ -73,34 +70,39 @@ describe("sandbox skill action orchestration", () => { process.exitCode = undefined; vi.clearAllMocks(); - captureSandboxSshConfig.mockReturnValue({ status: 0, output: "Host openshell-alpha\n" }); + selectOpenShellSandboxControlForMutation.mockReturnValue({ + control, + transport: "grpc", + close: closeControl, + }); + getSandbox.mockReturnValue({ gatewayName: "nemoclaw-9090", gatewayPort: 9090 }); ensureLiveSandboxOrExit.mockResolvedValue(undefined); getSessionAgent.mockReturnValue(agent); skillInstall.validateSkillName.mockReturnValue(true); skillInstall.resolveSkillPaths.mockReturnValue(paths); - skillInstall.checkExisting.mockReturnValue(true); - skillInstall.removeSkill.mockReturnValue({ + skillInstall.checkExisting.mockResolvedValue(true); + skillInstall.removeSkill.mockResolvedValue({ success: true, removedUploadDir: true, removedMirrorDir: true, clearedSessions: true, messages: [], }); - skillInstall.verifyRemove.mockReturnValue(true); + skillInstall.verifyRemove.mockResolvedValue(true); skillInstall.parseFrontmatter.mockReturnValue({ name: "demo-skill" }); skillInstall.collectFiles.mockReturnValue({ files: ["SKILL.md"], skippedDotfiles: [], unsafePaths: [], }); - skillInstall.uploadDirectory.mockReturnValue({ + skillInstall.uploadDirectory.mockResolvedValue({ uploaded: 1, failed: [], skippedDotfiles: [], unsafePaths: [], }); - skillInstall.postInstall.mockReturnValue({ success: true, messages: [] }); - skillInstall.verifyInstall.mockReturnValue(true); + skillInstall.postInstall.mockResolvedValue({ success: true, messages: [] }); + skillInstall.verifyInstall.mockResolvedValue(true); }); afterEach(() => { @@ -108,8 +110,10 @@ describe("sandbox skill action orchestration", () => { vi.restoreAllMocks(); }); - it("fails skill remove when SSH config capture fails", async () => { - captureSandboxSshConfig.mockReturnValue({ status: 1, output: "" }); + it("fails skill remove before dispatch when control selection fails", async () => { + selectOpenShellSandboxControlForMutation.mockImplementation(() => { + throw new Error("invalid mTLS material"); + }); const error = vi.spyOn(console, "error").mockImplementation(() => undefined); const exit = vi.spyOn(process, "exit").mockImplementation(((code?: string | number | null) => { throw new Error(`process.exit ${code}`); @@ -120,19 +124,16 @@ describe("sandbox skill action orchestration", () => { ); expect(ensureLiveSandboxOrExit).toHaveBeenCalledWith("alpha"); - expect(captureSandboxSshConfig).toHaveBeenCalledWith("alpha", expect.any(Object)); - expect(error).toHaveBeenCalledWith(" Failed to obtain SSH configuration for the sandbox."); + expect(selectOpenShellSandboxControlForMutation).toHaveBeenCalledWith("nemoclaw-9090"); + expect(error).toHaveBeenCalledWith( + " Failed to configure OpenShell sandbox execution: invalid mTLS material", + ); expect(skillInstall.checkExisting).not.toHaveBeenCalled(); expect(exit).toHaveBeenCalledWith(1); }); - it("treats unknown skill existence as fatal for remove and deletes the temp SSH config", async () => { - let tempConfig = ""; - skillInstall.checkExisting.mockImplementation((ctx) => { - tempConfig = ctx.configFile; - expect(fs.existsSync(tempConfig)).toBe(true); - return null; - }); + it("treats unknown skill existence as fatal for remove and closes the control", async () => { + skillInstall.checkExisting.mockResolvedValue(null); const error = vi.spyOn(console, "error").mockImplementation(() => undefined); await removeSandboxSkill("alpha", { name: "demo-skill" }); @@ -143,16 +144,11 @@ describe("sandbox skill action orchestration", () => { ); expect(skillInstall.removeSkill).not.toHaveBeenCalled(); expect(skillInstall.verifyRemove).not.toHaveBeenCalled(); - expect(tempConfig).not.toBe(""); - expect(fs.existsSync(tempConfig)).toBe(false); + expect(closeControl).toHaveBeenCalledOnce(); }); - it("reports an absent skill for remove and deletes the temp SSH config", async () => { - let tempConfig = ""; - skillInstall.checkExisting.mockImplementation((ctx) => { - tempConfig = ctx.configFile; - return false; - }); + it("reports an absent skill for remove and closes the control", async () => { + skillInstall.checkExisting.mockResolvedValue(false); const error = vi.spyOn(console, "error").mockImplementation(() => undefined); await removeSandboxSkill("alpha", { name: "demo-skill" }); @@ -161,14 +157,12 @@ describe("sandbox skill action orchestration", () => { expect(error).toHaveBeenCalledWith(" Skill 'demo-skill' is not installed in sandbox 'alpha'."); expect(skillInstall.removeSkill).not.toHaveBeenCalled(); expect(skillInstall.verifyRemove).not.toHaveBeenCalled(); - expect(tempConfig).not.toBe(""); - expect(fs.existsSync(tempConfig)).toBe(false); + expect(closeControl).toHaveBeenCalledOnce(); }); - it("removes and verifies an existing skill, then deletes the temp SSH config", async () => { - let tempConfig = ""; - skillInstall.checkExisting.mockImplementation((ctx, resolvedPaths) => { - tempConfig = ctx.configFile; + it("removes and verifies an existing skill, then closes the selected control", async () => { + skillInstall.checkExisting.mockImplementation(async (ctx, resolvedPaths) => { + expect(ctx.control).toBe(control); expect(resolvedPaths).toBe(paths); return true; }); @@ -179,17 +173,13 @@ describe("sandbox skill action orchestration", () => { expect(ensureLiveSandboxOrExit).toHaveBeenCalledWith("alpha"); expect(getSessionAgent).toHaveBeenCalledWith("alpha"); expect(skillInstall.resolveSkillPaths).toHaveBeenCalledWith(agent, "demo-skill"); - expect(skillInstall.removeSkill).toHaveBeenCalledWith( - expect.objectContaining({ configFile: tempConfig, sandboxName: "alpha" }), - paths, - ); + expect(skillInstall.removeSkill).toHaveBeenCalledWith({ control, sandboxName: "alpha" }, paths); expect(skillInstall.verifyRemove).toHaveBeenCalledWith( - expect.objectContaining({ configFile: tempConfig, sandboxName: "alpha" }), + { control, sandboxName: "alpha" }, paths, ); expect(log).toHaveBeenCalledWith(expect.stringContaining("Skill 'demo-skill' removed")); - expect(fs.existsSync(tempConfig)).toBe(false); - expectTempSshConfigCleanedUp(tempConfig); + expect(closeControl).toHaveBeenCalledOnce(); expect(process.exitCode).toBeUndefined(); }); @@ -207,19 +197,15 @@ describe("sandbox skill action orchestration", () => { } expect(ensureLiveSandboxOrExit).toHaveBeenCalledWith("alpha"); - expect(captureSandboxSshConfig).not.toHaveBeenCalled(); + expect(selectOpenShellSandboxControlForMutation).not.toHaveBeenCalled(); expect(skillInstall.uploadDirectory).not.toHaveBeenCalled(); }); - it("continues skill install when the existence probe is unknown because upload plus verify are authoritative", async () => { + it("fails skill install before upload when the existence probe is inconclusive", async () => { const skillDir = makeSkillDir(); - let tempConfig = ""; - skillInstall.checkExisting.mockImplementation((ctx) => { - tempConfig = ctx.configFile; - return null; - }); + skillInstall.checkExisting.mockResolvedValue(null); const error = vi.spyOn(console, "error").mockImplementation(() => undefined); - const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + vi.spyOn(console, "log").mockImplementation(() => undefined); try { await installSandboxSkill("alpha", { command: "install", path: skillDir }); @@ -228,22 +214,37 @@ describe("sandbox skill action orchestration", () => { } expect(error).toHaveBeenCalledWith( - expect.stringContaining( - "Warning: could not check sandbox for existing skill — treating as fresh install.", - ), + " Could not check if skill 'demo-skill' exists — sandbox may be unreachable. No files were uploaded.", ); + expect(skillInstall.uploadDirectory).not.toHaveBeenCalled(); + expect(skillInstall.postInstall).not.toHaveBeenCalled(); + expect(skillInstall.verifyInstall).not.toHaveBeenCalled(); + expect(closeControl).toHaveBeenCalledOnce(); + expect(process.exitCode).toBe(1); + }); + + it("installs when the existence probe confirms the skill is absent", async () => { + const skillDir = makeSkillDir(); + skillInstall.checkExisting.mockResolvedValue(false); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + + try { + await installSandboxSkill("alpha", { command: "install", path: skillDir }); + } finally { + fs.rmSync(skillDir, { recursive: true, force: true }); + } + expect(skillInstall.uploadDirectory).toHaveBeenCalledWith( - expect.objectContaining({ configFile: tempConfig, sandboxName: "alpha" }), + { control, sandboxName: "alpha" }, skillDir, paths.uploadDir, ); expect(skillInstall.verifyInstall).toHaveBeenCalledWith( - expect.objectContaining({ configFile: tempConfig, sandboxName: "alpha" }), + { control, sandboxName: "alpha" }, paths, ); expect(log).toHaveBeenCalledWith(expect.stringContaining("Skill 'demo-skill' installed")); - expect(fs.existsSync(tempConfig)).toBe(false); - expectTempSshConfigCleanedUp(tempConfig); + expect(closeControl).toHaveBeenCalledOnce(); expect(process.exitCode).toBeUndefined(); }); }); diff --git a/src/lib/actions/sandbox/skill-install.ts b/src/lib/actions/sandbox/skill-install.ts index 3eb9d547257..6b81d099c20 100644 --- a/src/lib/actions/sandbox/skill-install.ts +++ b/src/lib/actions/sandbox/skill-install.ts @@ -3,13 +3,16 @@ import fs from "node:fs"; import path from "node:path"; -import { captureSandboxSshConfig } from "../../adapters/openshell/runtime"; -import { OPENSHELL_PROBE_TIMEOUT_MS } from "../../adapters/openshell/timeouts"; +import { + type OpenShellMutationControlSelection, + selectOpenShellSandboxControlForMutation, +} from "../../adapters/openshell/sandbox-control-routing"; import * as agentRuntime from "../../agent/runtime"; import { CLI_NAME } from "../../cli/branding"; import { D, G, R, YW } from "../../cli/terminal-style"; -import { createTempSshConfig } from "../../sandbox/temp-ssh-config"; +import { resolveSandboxGatewayName } from "../../onboard/gateway-binding"; import * as skillInstall from "../../skill-install"; +import * as registry from "../../state/registry"; import { ensureLiveSandboxOrExit } from "./gateway-state"; export function printSkillInstallUsage(): void { @@ -80,6 +83,18 @@ export function printPluginInstallHint(): void { ); } +function selectSkillControl(sandboxName: string): OpenShellMutationControlSelection { + try { + return selectOpenShellSandboxControlForMutation( + resolveSandboxGatewayName(registry.getSandbox(sandboxName)), + ); + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + console.error(` Failed to configure OpenShell sandbox execution: ${detail}`); + process.exit(1); + } +} + /** * Remove an installed skill from a live sandbox by name. */ @@ -114,21 +129,12 @@ export async function removeSandboxSkill( const agent = agentRuntime.getSessionAgent(sandboxName); const paths = skillInstall.resolveSkillPaths(agent, skillName); - const sshConfigResult = captureSandboxSshConfig(sandboxName, { - ignoreError: true, - timeout: OPENSHELL_PROBE_TIMEOUT_MS, - }); - if (sshConfigResult.status !== 0) { - console.error(" Failed to obtain SSH configuration for the sandbox."); - process.exit(1); - } - - const tmpSshConfig = createTempSshConfig(sshConfigResult.output, "nemoclaw-ssh-skill-"); + const selected = selectSkillControl(sandboxName); try { - const ctx = { configFile: tmpSshConfig.file, sandboxName }; + const ctx = { control: selected.control, sandboxName }; - const existsCheck = skillInstall.checkExisting(ctx, paths); + const existsCheck = await skillInstall.checkExisting(ctx, paths); if (existsCheck === null) { console.error( ` Could not check if skill '${skillName}' exists — sandbox may be unreachable.`, @@ -142,7 +148,7 @@ export async function removeSandboxSkill( return; } - const result = skillInstall.removeSkill(ctx, paths); + const result = await skillInstall.removeSkill(ctx, paths); for (const msg of result.messages) { if (msg.startsWith("Warning:")) { console.error(` ${YW}${msg}${R}`); @@ -151,7 +157,7 @@ export async function removeSandboxSkill( } } - const gone = skillInstall.verifyRemove(ctx, paths); + const gone = await skillInstall.verifyRemove(ctx, paths); if (gone) { console.log(` ${G}✓${R} Skill '${skillName}' removed`); } else { @@ -161,7 +167,7 @@ export async function removeSandboxSkill( return; } } finally { - tmpSshConfig.cleanup(); + selected.close(); } } @@ -273,39 +279,28 @@ export async function installSandboxSkill( const agent = agentRuntime.getSessionAgent(sandboxName); const paths = skillInstall.resolveSkillPaths(agent, frontmatter.name); - // 4. Get SSH config - const sshConfigResult = captureSandboxSshConfig(sandboxName, { - ignoreError: true, - timeout: OPENSHELL_PROBE_TIMEOUT_MS, - }); - if (sshConfigResult.status !== 0) { - console.error(" Failed to obtain SSH configuration for the sandbox."); - process.exit(1); - } - - const tmpSshConfig = createTempSshConfig(sshConfigResult.output, "nemoclaw-ssh-skill-"); + // 4. Select one control-plane transport before any mutation. + const selected = selectSkillControl(sandboxName); try { - const ctx = { configFile: tmpSshConfig.file, sandboxName }; - - // 5. Check if skill already exists (update vs fresh install). This probe is - // advisory for install only: stale SSH config files and transient remote - // shell startup failures can make the stat probe inconclusive even when a - // subsequent upload succeeds. Upload plus verifyInstall() remain the - // source of truth for install success; remove keeps null fatal because it - // is destructive. Once OpenShell exposes a typed stat API or SSH probe - // failures are reliably distinguishable from absent dirs across supported - // versions, remove this fallback and fail before upload. - const existingCheck = skillInstall.checkExisting(ctx, paths); + const ctx = { control: selected.control, sandboxName }; + + // 5. Check if skill already exists (update vs fresh install). A successful + // sentinel response distinguishes absence from an existing directory; + // an inconclusive control result is an execution failure, not absence. + // Fail closed before upload so a probe failure cannot precede mutation. + const existingCheck = await skillInstall.checkExisting(ctx, paths); if (existingCheck === null) { console.error( - ` ${YW}Warning: could not check sandbox for existing skill — treating as fresh install.${R}`, + ` Could not check if skill '${frontmatter.name}' exists — sandbox may be unreachable. No files were uploaded.`, ); + process.exitCode = 1; + return; } const isUpdate = existingCheck === true; // 6. Upload skill directory - const { uploaded, failed } = skillInstall.uploadDirectory(ctx, skillDir, paths.uploadDir); + const { uploaded, failed } = await skillInstall.uploadDirectory(ctx, skillDir, paths.uploadDir); if (failed.length > 0) { console.error(` Failed to upload ${failed.length} file(s): ${failed.join(", ")}`); process.exit(1); @@ -315,7 +310,7 @@ export async function installSandboxSkill( // 7. Post-install (OpenClaw mirror + refresh, or restart hint). // OpenClaw caches skill content per session, so always refresh the // session index after an install/update to avoid stale SKILL.md data. - const post = skillInstall.postInstall(ctx, paths, skillDir); + const post = await skillInstall.postInstall(ctx, paths, skillDir); for (const msg of post.messages) { if (msg.startsWith("Warning:")) { console.error(` ${YW}${msg}${R}`); @@ -325,7 +320,7 @@ export async function installSandboxSkill( } // 8. Verify - const verified = skillInstall.verifyInstall(ctx, paths); + const verified = await skillInstall.verifyInstall(ctx, paths); if (verified) { const verb = isUpdate ? "updated" : "installed"; console.log(` ${G}✓${R} Skill '${frontmatter.name}' ${verb}`); @@ -337,6 +332,6 @@ export async function installSandboxSkill( process.exit(1); } } finally { - tmpSshConfig.cleanup(); + selected.close(); } } diff --git a/src/lib/adapters/openshell/client.test.ts b/src/lib/adapters/openshell/client.test.ts index d3e31e3e7bf..205dd8896d0 100644 --- a/src/lib/adapters/openshell/client.test.ts +++ b/src/lib/adapters/openshell/client.test.ts @@ -8,9 +8,11 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { captureOpenshellCommand, captureOpenshellCommandAsync, + captureOpenshellCommandBinary, captureSandboxSshConfigCommand, getInstalledOpenshellVersion, type OpenshellSpawnSync, + type OpenshellSpawnSyncBinary, parseVersionFromText, runOpenshellCommand, stripAnsi, @@ -41,6 +43,17 @@ function stubSpawnSync(spec: SpawnResultSpec): OpenshellSpawnSync { return () => makeSpawnResult(spec); } +function stubBinarySpawnSync(stdout: Buffer, stderr = Buffer.alloc(0)): OpenshellSpawnSyncBinary { + return () => ({ + pid: 123, + output: [stdout, stderr], + stdout, + stderr, + status: 0, + signal: null, + }); +} + function timeoutError(): Error { return Object.assign(new Error("spawnSync openshell ETIMEDOUT"), { code: "ETIMEDOUT" }); } @@ -125,6 +138,32 @@ describe("openshell helpers", () => { }); }); + it("captures raw stdout and stderr without UTF-8 decoding", () => { + const bytes = Buffer.from([0, 255, 128, 10]); + const stderr = Buffer.from("warning"); + const result = captureOpenshellCommandBinary("openshell", ["sandbox", "exec"], { + spawnSyncImpl: stubBinarySpawnSync(bytes, stderr), + }); + + expect(result).toEqual({ status: 0, stdout: bytes, stderr }); + }); + + it("preserves real child-process ENOBUFS metadata and partial raw output", () => { + const result = captureOpenshellCommandBinary( + process.execPath, + ["-e", "process.stdout.write(Buffer.from([0xc3, 0xa9]))"], + { maxBuffer: 1 }, + ); + + // Node can report either the completed exit status or null when ENOBUFS + // races a fast child exit. The error code and retained raw prefix are stable. + expect((result.error as NodeJS.ErrnoException | undefined)?.code).toBe("ENOBUFS"); + expect(Buffer.isBuffer(result.stdout)).toBe(true); + expect(result.stdout.length).toBeGreaterThan(0); + expect(result.stdout[0]).toBe(0xc3); + expect(Buffer.isBuffer(result.stderr)).toBe(true); + }); + it("returns the spawn result when the command succeeds", () => { const result = runOpenshellCommand("openshell", ["status"], { spawnSyncImpl: stubSpawnSync({ @@ -251,6 +290,24 @@ describe("openshell helpers", () => { }); }); + it("pipes binary stdin through captured OpenShell commands", () => { + const stdin = Buffer.from([0, 255, 10]); + let observedStdio: unknown; + let observedInput: unknown; + const result = captureOpenshellCommand("openshell", ["sandbox", "exec"], { + input: stdin, + spawnSyncImpl: (_command, _args, options) => { + observedStdio = options.stdio; + observedInput = options.input; + return makeSpawnResult({ status: 0, stdout: "ok\n", stderr: "" }); + }, + }); + + expect(observedStdio).toEqual(["pipe", "pipe", "pipe"]); + expect(observedInput).toBe(stdin); + expect(result).toEqual({ status: 0, output: "ok" }); + }); + it("verifies sandbox existence before requesting SSH config", () => { const calls: string[][] = []; const spawnSyncImpl: OpenshellSpawnSync = (_command, args) => { diff --git a/src/lib/adapters/openshell/client.ts b/src/lib/adapters/openshell/client.ts index 39b3c5b6355..54f5512ab6a 100644 --- a/src/lib/adapters/openshell/client.ts +++ b/src/lib/adapters/openshell/client.ts @@ -18,6 +18,12 @@ export type OpenshellSpawnSync = ( options: SpawnSyncOptionsWithStringEncoding, ) => SpawnSyncReturns; +export type OpenshellSpawnSyncBinary = ( + command: string, + args: readonly string[], + options: SpawnSyncOptions, +) => SpawnSyncReturns; + export type OpenshellSpawn = typeof spawn; interface OpenshellSpawnOptions { @@ -31,7 +37,9 @@ interface OpenshellSpawnOptions { exit?: (code: number) => never; } -function openshellSpawnEnv(opts: OpenshellSpawnOptions): NodeJS.ProcessEnv { +function openshellSpawnEnv( + opts: Pick, +): NodeJS.ProcessEnv { const explicitEnv = Object.fromEntries( Object.entries(opts.env ?? {}).filter( (entry): entry is [string, string] => entry[1] !== undefined, @@ -42,12 +50,13 @@ function openshellSpawnEnv(opts: OpenshellSpawnOptions): NodeJS.ProcessEnv { export interface RunOpenshellOptions extends OpenshellSpawnOptions { stdio?: SpawnSyncOptions["stdio"]; - input?: string; + input?: string | Buffer; } export interface CaptureOpenshellOptions extends OpenshellSpawnOptions { includeStderr?: boolean; includeStreams?: boolean; + input?: string | Buffer; maxBuffer?: number; } @@ -65,6 +74,22 @@ export interface CaptureOpenshellResult { signal?: NodeJS.Signals | null; } +export interface CaptureOpenshellBinaryOptions + extends Pick< + CaptureOpenshellOptions, + "cwd" | "env" | "replaceEnv" | "input" | "timeout" | "maxBuffer" + > { + spawnSyncImpl?: OpenshellSpawnSyncBinary; +} + +export interface CaptureOpenshellBinaryResult { + status: number | null; + stdout: Buffer; + stderr: Buffer; + error?: Error; + signal?: NodeJS.Signals | null; +} + const ANSI_RE = /\x1b\[[0-9;]*m/g; export function stripAnsi(value = ""): string { @@ -212,7 +237,8 @@ export function captureOpenshellCommand( cwd: opts.cwd, env: openshellSpawnEnv(opts), encoding: "utf-8", - stdio: ["ignore", "pipe", "pipe"], + stdio: [opts.input === undefined ? "ignore" : "pipe", "pipe", "pipe"], + input: opts.input, timeout: opts.timeout, maxBuffer: opts.maxBuffer, }); @@ -235,6 +261,30 @@ export function captureOpenshellCommand( }; } +/** Capture raw OpenShell output so byte limits are enforced before decoding. */ +export function captureOpenshellCommandBinary( + binary: string, + args: string[], + opts: CaptureOpenshellBinaryOptions = {}, +): CaptureOpenshellBinaryResult { + const spawnSyncImpl = opts.spawnSyncImpl ?? spawnSync; + const result = spawnSyncImpl(binary, args, { + cwd: opts.cwd, + env: openshellSpawnEnv(opts), + stdio: [opts.input === undefined ? "ignore" : "pipe", "pipe", "pipe"], + input: opts.input, + timeout: opts.timeout, + maxBuffer: opts.maxBuffer, + }); + return { + status: result.status, + stdout: result.stdout ?? Buffer.alloc(0), + stderr: result.stderr ?? Buffer.alloc(0), + ...(result.error ? { error: result.error } : {}), + ...(result.signal !== null ? { signal: result.signal } : {}), + }; +} + export function captureSandboxSshConfigCommand( binary: string, sandboxName: string, diff --git a/src/lib/adapters/openshell/grpc-gateway-config.ts b/src/lib/adapters/openshell/grpc-gateway-config.ts index ace3611bf52..4492dc915db 100644 --- a/src/lib/adapters/openshell/grpc-gateway-config.ts +++ b/src/lib/adapters/openshell/grpc-gateway-config.ts @@ -38,6 +38,16 @@ export interface ResolvedOpenShellGrpcGateway { clientConfig: OpenShellGrpcClientConfig; } +/** Direct gRPC cannot traverse OpenShell's CLI-owned Cloudflare edge tunnel. */ +export class OpenShellGrpcEdgeTunnelRequiredError extends Error { + constructor() { + super( + "OpenShell Cloudflare JWT gateways require the OpenShell edge tunnel and are not supported by the direct gRPC client", + ); + this.name = "OpenShellGrpcEdgeTunnelRequiredError"; + } +} + function validateGatewayName(name: string): void { if ( !name || @@ -234,9 +244,7 @@ export function resolveOpenShellGrpcGateway( } clientConfig = oidcConfig(metadata.gateway_endpoint, gatewayDir, nowSeconds); } else if (authMode === "cloudflare_jwt") { - throw new Error( - "OpenShell Cloudflare JWT gateways require the OpenShell edge tunnel and are not supported by the direct gRPC client", - ); + throw new OpenShellGrpcEdgeTunnelRequiredError(); } else { throw new Error(`Unsupported OpenShell gateway auth mode '${authMode}'`); } diff --git a/src/lib/adapters/openshell/grpc-sandbox-control.test.ts b/src/lib/adapters/openshell/grpc-sandbox-control.test.ts index ab1d5c7fa86..0663702cdd9 100644 --- a/src/lib/adapters/openshell/grpc-sandbox-control.test.ts +++ b/src/lib/adapters/openshell/grpc-sandbox-control.test.ts @@ -25,7 +25,10 @@ import { OpenShellGrpcOutputLimitError, OpenShellGrpcPreDispatchError, } from "./grpc-sandbox-control"; -import { OpenShellExecRequestValidationError } from "./sandbox-control"; +import { + OPENSHELL_EXEC_MAX_OUTPUT_BYTES, + OpenShellExecRequestValidationError, +} from "./sandbox-control"; class FakeStream extends EventEmitter { cancelled = false; @@ -79,7 +82,15 @@ function fakeApi(fixture: FakeApiOptions = {}): { return Object.assign(stream, { request }); }, }; - return { api, stream, getMetadata, execMetadata, getOptions, execOptions, close }; + return { + api, + stream, + getMetadata, + execMetadata, + getOptions, + execOptions, + close, + }; } function serviceError(message: string): ServiceError { @@ -114,7 +125,10 @@ describe("gRPC OpenShell sandbox control", () => { fake.api, ); - const result = await control.exec({ sandboxName: "alpha", command: ["bad\ncommand"] }); + const result = await control.exec({ + sandboxName: "alpha", + command: ["bad\ncommand"], + }); expect(result).toMatchObject({ status: null, stdout: "", stderr: "" }); expect(result.error).toBeInstanceOf(OpenShellExecRequestValidationError); @@ -122,6 +136,50 @@ describe("gRPC OpenShell sandbox control", () => { expect(fake.execMetadata).toEqual([]); }); + it("rejects an oversized encoded request before lookup or exec", async () => { + const fake = fakeApi({ sandboxId: "00000000-0000-0000-0000-000000000000" }); + const control = createGrpcOpenShellSandboxControl( + { endpoint: "http://127.0.0.1:8080" }, + fake.api, + ); + + const result = await control.exec({ + sandboxName: "alpha", + command: ["sh", "-s"], + stdin: Buffer.alloc(1_048_527), + }); + + expect(result.error).toBeInstanceOf(OpenShellExecRequestValidationError); + expect((result.error as OpenShellExecRequestValidationError).issue.kind).toBe( + "encoded-request-too-large", + ); + expect(fake.getMetadata).toEqual([]); + expect(fake.execMetadata).toEqual([]); + }); + + it("revalidates the encoded boundary with the sandbox id returned by lookup", async () => { + const fake = fakeApi({ sandboxId: "00000000-0000-0000-0000-000000000000x" }); + const control = createGrpcOpenShellSandboxControl( + { endpoint: "http://127.0.0.1:8080" }, + fake.api, + ); + + const result = await control.exec({ + sandboxName: "alpha", + command: ["sh", "-s"], + stdin: Buffer.alloc(1_048_526), + }); + + expect(result.error).toBeInstanceOf(OpenShellExecRequestValidationError); + expect((result.error as OpenShellExecRequestValidationError).issue).toEqual({ + kind: "encoded-request-too-large", + actualBytes: 1_048_577, + maxBytes: 1_048_576, + }); + expect(fake.getMetadata).toHaveLength(1); + expect(fake.execMetadata).toEqual([]); + }); + it("dispatches an exact 32768-byte UTF-8 argument unchanged", async () => { const fake = fakeApi({ emit(stream) { @@ -136,7 +194,10 @@ describe("gRPC OpenShell sandbox control", () => { const boundaryArgument = "é".repeat(16 * 1024); await expect( - control.exec({ sandboxName: "alpha", command: ["printf", boundaryArgument] }), + control.exec({ + sandboxName: "alpha", + command: ["printf", boundaryArgument], + }), ).resolves.toMatchObject({ status: 0 }); expect( @@ -165,6 +226,7 @@ describe("gRPC OpenShell sandbox control", () => { control.exec({ sandboxName: "alpha", command: ["sh", "-lc", "echo hello"], + stdin: "request body", }), ).resolves.toEqual({ status: 7, @@ -176,6 +238,7 @@ describe("gRPC OpenShell sandbox control", () => { expect((fake.stream as FakeStream & { request: unknown }).request).toEqual({ sandboxId: "sb-id", command: ["sh", "-lc", "echo hello"], + stdin: Buffer.from("request body"), }); control.close(); expect(fake.close).toHaveBeenCalledOnce(); @@ -208,7 +271,7 @@ describe("gRPC OpenShell sandbox control", () => { }, execSandbox( call: ServerWritableStream< - { sandboxId: string; command: string[] }, + { sandboxId: string; command: string[]; stdin?: Buffer }, { stdout?: { data: Buffer }; stderr?: { data: Buffer }; @@ -229,7 +292,11 @@ describe("gRPC OpenShell sandbox control", () => { }); try { await expect( - control.exec({ sandboxName: "alpha", command: ["printf", "wire"] }), + control.exec({ + sandboxName: "alpha", + command: ["cat"], + stdin: Buffer.from([0, 255, 10]), + }), ).resolves.toEqual({ status: 0, stdout: "wire stdout", @@ -237,7 +304,11 @@ describe("gRPC OpenShell sandbox control", () => { }); expect(requests).toEqual([ { name: "alpha" }, - { sandboxId: "wire-id", command: ["printf", "wire"] }, + { + sandboxId: "wire-id", + command: ["cat"], + stdin: Buffer.from([0, 255, 10]), + }, ]); } finally { control.close(); @@ -278,7 +349,11 @@ describe("gRPC OpenShell sandbox control", () => { ); const before = Date.now(); - await control.exec({ sandboxName: "alpha", command: ["true"], timeoutMs: 1500 }); + await control.exec({ + sandboxName: "alpha", + command: ["true"], + timeoutMs: 1500, + }); expect(fake.getOptions[0].deadline).toBeInstanceOf(Date); expect(fake.execOptions[0].deadline).toBe(fake.getOptions[0].deadline); @@ -314,6 +389,8 @@ describe("gRPC OpenShell sandbox control", () => { it.each([ [{ maxOutputBytes: -1 }, "maxOutputBytes"], + [{ maxOutputBytes: 1.5 }, "maxOutputBytes"], + [{ maxOutputBytes: OPENSHELL_EXEC_MAX_OUTPUT_BYTES + 1 }, "maxOutputBytes"], [{ timeoutMs: 1.5 }, "timeoutMs"], ])("rejects invalid execution limits before gateway lookup", async (limits, field) => { const fake = fakeApi(); @@ -322,8 +399,13 @@ describe("gRPC OpenShell sandbox control", () => { fake.api, ); - const result = await control.exec({ sandboxName: "alpha", command: ["true"], ...limits }); + const result = await control.exec({ + sandboxName: "alpha", + command: ["true"], + ...limits, + }); + expect(result.error).toBeInstanceOf(OpenShellExecRequestValidationError); expect(result.error?.message).toContain(field); expect(fake.getMetadata).toEqual([]); }); @@ -352,6 +434,72 @@ describe("gRPC OpenShell sandbox control", () => { expect(fake.stream.cancelled).toBe(true); }); + it("measures raw invalid UTF-8 bytes without a false output-limit failure", async () => { + const fake = fakeApi({ + emit(stream) { + stream.emit("data", { stdout: { data: Buffer.from([0xff]) } }); + stream.emit("data", { exit: { exitCode: 0 } }); + stream.emit("end"); + }, + }); + const control = createGrpcOpenShellSandboxControl( + { endpoint: "http://127.0.0.1:8080" }, + fake.api, + ); + + await expect( + control.exec({ sandboxName: "alpha", command: ["true"], maxOutputBytes: 1 }), + ).resolves.toEqual({ status: 0, stdout: "\ufffd", stderr: "" }); + }); + + it("reports raw-byte overflow when the cap splits a multibyte sequence", async () => { + const fake = fakeApi({ + emit(stream) { + stream.emit("data", { stdout: { data: Buffer.from("é") } }); + }, + }); + const control = createGrpcOpenShellSandboxControl( + { endpoint: "http://127.0.0.1:8080" }, + fake.api, + ); + + await expect( + control.exec({ sandboxName: "alpha", command: ["true"], maxOutputBytes: 1 }), + ).resolves.toEqual({ + status: null, + stdout: "\ufffd", + stderr: "", + error: expect.any(OpenShellGrpcOutputLimitError), + }); + expect(fake.stream.cancelled).toBe(true); + }); + + it("treats zero as a zero-byte output cap", async () => { + const fake = fakeApi({ + emit(stream) { + stream.emit("data", { stdout: { data: Buffer.from("x") } }); + }, + }); + const control = createGrpcOpenShellSandboxControl( + { endpoint: "http://127.0.0.1:8080" }, + fake.api, + ); + + const result = await control.exec({ + sandboxName: "alpha", + command: ["true"], + maxOutputBytes: 0, + }); + + expect(result).toEqual({ + status: null, + stdout: "", + stderr: "", + error: expect.any(OpenShellGrpcOutputLimitError), + }); + expect(fake.stream.cancelled).toBe(true); + }); + it("preserves partial output when the stream fails", async () => { const error = serviceError("relay reset"); const fake = fakeApi({ diff --git a/src/lib/adapters/openshell/grpc-sandbox-control.ts b/src/lib/adapters/openshell/grpc-sandbox-control.ts index bd6709c388d..8806e7c91c1 100644 --- a/src/lib/adapters/openshell/grpc-sandbox-control.ts +++ b/src/lib/adapters/openshell/grpc-sandbox-control.ts @@ -10,14 +10,15 @@ import * as grpc from "@grpc/grpc-js"; import * as protoLoader from "@grpc/proto-loader"; import { - openShellExecRequestValidationFailure, - validateOpenShellExecCommand, + OPENSHELL_EXEC_DEFAULT_MAX_OUTPUT_BYTES, + OpenShellExecOutputLimitError, type OpenShellSandboxControl, + openShellExecRequestValidationFailure, type SandboxExecRequest, type SandboxExecResult, + validateOpenShellExecRequest, } from "./sandbox-control"; -const DEFAULT_EXEC_MAX_OUTPUT_BYTES = 1024 * 1024; const PROTO_VERSION = "0.0.72"; interface GetSandboxResponse { @@ -42,7 +43,12 @@ export interface OpenShellGrpcApi { callback: (error: grpc.ServiceError | null, response?: GetSandboxResponse) => void, ): unknown; execSandbox( - request: { sandboxId: string; command: readonly string[]; timeoutSeconds?: number }, + request: { + sandboxId: string; + command: readonly string[]; + stdin?: Buffer; + timeoutSeconds?: number; + }, metadata: grpc.Metadata, options: grpc.CallOptions, ): ExecEventStream; @@ -66,14 +72,7 @@ export interface GrpcOpenShellSandboxControl extends OpenShellSandboxControl { close(): void; } -export class OpenShellGrpcOutputLimitError extends Error { - readonly code = "ENOBUFS"; - - constructor(readonly maxOutputBytes: number) { - super(`OpenShell gRPC exec output exceeded ${maxOutputBytes} bytes`); - this.name = "OpenShellGrpcOutputLimitError"; - } -} +export { OpenShellExecOutputLimitError as OpenShellGrpcOutputLimitError } from "./sandbox-control"; export class OpenShellGrpcPreDispatchError extends Error { constructor(readonly cause: Error) { @@ -223,12 +222,17 @@ function execute( metadata: grpc.Metadata, options: grpc.CallOptions, ): Promise { - const maxOutputBytes = request.maxOutputBytes ?? DEFAULT_EXEC_MAX_OUTPUT_BYTES; + const maxOutputBytes = request.maxOutputBytes ?? OPENSHELL_EXEC_DEFAULT_MAX_OUTPUT_BYTES; return new Promise((resolve) => { const timeoutSeconds = request.timeoutMs && request.timeoutMs > 0 ? Math.ceil(request.timeoutMs / 1000) : undefined; const stream = client.execSandbox( - { sandboxId: id, command: request.command, timeoutSeconds }, + { + sandboxId: id, + command: request.command, + stdin: request.stdin === undefined ? undefined : Buffer.from(request.stdin), + timeoutSeconds, + }, metadata, options, ); @@ -257,7 +261,7 @@ function execute( if (destination === "stdout") stdout += stdoutDecoder.write(retained); else stderr += stderrDecoder.write(retained); if (retained.length < data.length) { - finish(new OpenShellGrpcOutputLimitError(maxOutputBytes)); + finish(new OpenShellExecOutputLimitError(maxOutputBytes)); stream.cancel(); } }; @@ -290,29 +294,10 @@ export function createGrpcOpenShellSandboxControl( return { close: () => client.close(), async exec(request): Promise { - const validationError = validateOpenShellExecCommand(request.command); + // Validate against the exact v0.0.72 UUID-width request before lookup so + // transport-independent limits cannot cause any gateway activity. + const validationError = validateOpenShellExecRequest(request); if (validationError) return openShellExecRequestValidationFailure(validationError); - - const maxOutputBytes = request.maxOutputBytes ?? DEFAULT_EXEC_MAX_OUTPUT_BYTES; - if (!Number.isSafeInteger(maxOutputBytes) || maxOutputBytes < 0) { - return { - status: null, - stdout: "", - stderr: "", - error: new Error("maxOutputBytes must be a non-negative safe integer"), - }; - } - if ( - request.timeoutMs !== undefined && - (!Number.isSafeInteger(request.timeoutMs) || request.timeoutMs < 0) - ) { - return { - status: null, - stdout: "", - stderr: "", - error: new Error("timeoutMs must be a non-negative safe integer"), - }; - } const metadata = callMetadata(config.bearerToken); const deadline = request.timeoutMs && request.timeoutMs > 0 @@ -331,6 +316,10 @@ export function createGrpcOpenShellSandboxControl( error: new OpenShellGrpcPreDispatchError(cause), }; } + const requestValidationError = validateOpenShellExecRequest(request, id); + if (requestValidationError) { + return openShellExecRequestValidationFailure(requestValidationError); + } return execute(client, id, request, metadata, options); }, }; diff --git a/src/lib/adapters/openshell/runtime.test.ts b/src/lib/adapters/openshell/runtime.test.ts new file mode 100644 index 00000000000..13309c1cfdf --- /dev/null +++ b/src/lib/adapters/openshell/runtime.test.ts @@ -0,0 +1,68 @@ +// 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"; + +const mocks = vi.hoisted(() => ({ + captureOpenshellCommand: vi.fn(), + captureOpenshellCommandBinary: vi.fn(), +})); + +vi.mock("./client", () => ({ + captureOpenshellCommand: mocks.captureOpenshellCommand, + captureOpenshellCommandBinary: mocks.captureOpenshellCommandBinary, + captureOpenshellCommandAsync: vi.fn(), + captureSandboxSshConfigCommand: vi.fn(), + getInstalledOpenshellVersion: vi.fn(), + runOpenshellCommand: vi.fn(), +})); + +vi.mock("./resolve", () => ({ + resolveOpenshell: () => "/test/openshell", +})); + +import { ROOT } from "../../runner"; +import { captureOpenshell, captureOpenshellBinary } from "./runtime"; + +describe("OpenShell runtime capture", () => { + beforeEach(() => { + mocks.captureOpenshellCommand.mockReset(); + mocks.captureOpenshellCommand.mockReturnValue({ status: 0, output: "ok" }); + mocks.captureOpenshellCommandBinary.mockReset(); + mocks.captureOpenshellCommandBinary.mockReturnValue({ + status: 0, + stdout: Buffer.from("ok"), + stderr: Buffer.alloc(0), + }); + }); + + it("forwards binary stdin to the command capture boundary", () => { + const input = Buffer.from([0, 255, 10]); + const args = ["sandbox", "exec", "--name", "alpha", "--", "cat"]; + + captureOpenshell(args, { input }); + + expect(mocks.captureOpenshellCommand).toHaveBeenCalledWith( + "/test/openshell", + args, + expect.objectContaining({ input }), + ); + expect(mocks.captureOpenshellCommand.mock.calls[0]?.[2]?.input).toBe(input); + }); + + it("runs raw captures from the standard NemoClaw working directory", () => { + const input = Buffer.from([0, 255, 10]); + const args = ["sandbox", "exec", "--name", "alpha", "--", "cat"]; + + captureOpenshellBinary(args, { input, maxBuffer: 4096, timeout: 30_000 }); + + expect(mocks.captureOpenshellCommandBinary).toHaveBeenCalledWith("/test/openshell", args, { + cwd: ROOT, + env: undefined, + replaceEnv: undefined, + input, + maxBuffer: 4096, + timeout: 30_000, + }); + }); +}); diff --git a/src/lib/adapters/openshell/runtime.ts b/src/lib/adapters/openshell/runtime.ts index 178159b13f8..ed1d490a597 100644 --- a/src/lib/adapters/openshell/runtime.ts +++ b/src/lib/adapters/openshell/runtime.ts @@ -7,6 +7,7 @@ import { ROOT } from "../../runner"; import { captureOpenshellCommand, captureOpenshellCommandAsync, + captureOpenshellCommandBinary, captureSandboxSshConfigCommand, getInstalledOpenshellVersion, runOpenshellCommand, @@ -20,7 +21,7 @@ type RunnerOptions = { env?: NodeJS.ProcessEnv; replaceEnv?: boolean; stdio?: StdioOptions; - input?: string; + input?: string | Buffer; ignoreError?: boolean; includeStderr?: boolean; includeStreams?: boolean; @@ -70,6 +71,7 @@ export function captureOpenshell(args: CommandArgs, opts: RunnerOptions = {}) { ignoreError: opts.ignoreError, includeStderr: opts.includeStderr, includeStreams: opts.includeStreams, + input: opts.input, timeout: opts.timeout, maxBuffer: opts.maxBuffer, errorLine: console.error, @@ -77,6 +79,18 @@ export function captureOpenshell(args: CommandArgs, opts: RunnerOptions = {}) { }); } +/** Run an OpenShell command and preserve output as raw bytes. */ +export function captureOpenshellBinary(args: CommandArgs, opts: RunnerOptions = {}) { + return captureOpenshellCommandBinary(getOpenshellBinary(), args, { + cwd: ROOT, + env: opts.env, + replaceEnv: opts.replaceEnv, + input: opts.input, + timeout: opts.timeout, + maxBuffer: opts.maxBuffer, + }); +} + /** Capture the SSH config OpenShell emits for a sandbox. */ export function captureSandboxSshConfig(sandboxName: string, opts: RunnerOptions = {}) { return captureSandboxSshConfigCommand(getOpenshellBinary(), sandboxName, { diff --git a/src/lib/adapters/openshell/sandbox-control-routing.test.ts b/src/lib/adapters/openshell/sandbox-control-routing.test.ts index e1664056f34..f7ed4427781 100644 --- a/src/lib/adapters/openshell/sandbox-control-routing.test.ts +++ b/src/lib/adapters/openshell/sandbox-control-routing.test.ts @@ -3,17 +3,22 @@ import { describe, expect, it, vi } from "vitest"; +import { OpenShellGrpcEdgeTunnelRequiredError } from "./grpc-gateway-config"; import { type GrpcOpenShellSandboxControl, OpenShellGrpcPreDispatchError, } from "./grpc-sandbox-control"; import { - type OpenShellSandboxControl, + OPENSHELL_EXEC_MAX_OUTPUT_BYTES, OpenShellExecRequestValidationError, + type OpenShellSandboxControl, openShellExecRequestValidationFailure, type SandboxExecResult, } from "./sandbox-control"; -import { execSandboxReadOnlyWithGrpcFallback } from "./sandbox-control-routing"; +import { + execSandboxReadOnlyWithGrpcFallback, + selectOpenShellSandboxControlForMutation, +} from "./sandbox-control-routing"; import { OPENSHELL_OPERATION_TIMEOUT_MS } from "./timeouts"; function dependencies(grpcResult: SandboxExecResult | Error, cliResult?: SandboxExecResult) { @@ -29,6 +34,7 @@ function dependencies(grpcResult: SandboxExecResult | Error, cliResult?: Sandbox const debug = vi.fn(); return { close, + cli, grpcExec, cliExec, createCli, @@ -89,8 +95,31 @@ describe("read-only OpenShell sandbox control routing", () => { expect(test.cliExec).not.toHaveBeenCalled(); }); + it.each([ + -1, + 1.5, + OPENSHELL_EXEC_MAX_OUTPUT_BYTES + 1, + ])("rejects maxOutputBytes=%s before creating either transport", async (maxOutputBytes) => { + const test = dependencies({ status: 0, stdout: "unused", stderr: "" }); + + const result = await execSandboxReadOnlyWithGrpcFallback( + "nemoclaw", + { ...request, maxOutputBytes }, + test.deps, + ); + + expect(result.error).toBeInstanceOf(OpenShellExecRequestValidationError); + expect((result.error as OpenShellExecRequestValidationError).issue.kind).toBe( + "max-output-out-of-range", + ); + expect(test.createGrpc).not.toHaveBeenCalled(); + expect(test.createCli).not.toHaveBeenCalled(); + }); + it("does not route a thrown typed validation error through the CLI", async () => { - const error = new OpenShellExecRequestValidationError({ kind: "empty-command" }); + const error = new OpenShellExecRequestValidationError({ + kind: "empty-command", + }); const test = dependencies(error); await expect( @@ -198,7 +227,12 @@ describe("read-only OpenShell sandbox control routing", () => { it("does not replay a post-dispatch gRPC stream failure", async () => { const grpcError = new Error("stream reset"); - const result = { status: null, stdout: "partial", stderr: "", error: grpcError }; + const result = { + status: null, + stdout: "partial", + stderr: "", + error: grpcError, + }; const test = dependencies(result); await expect( @@ -267,3 +301,74 @@ describe("read-only OpenShell sandbox control routing", () => { ); }); }); + +describe("mutating OpenShell sandbox control routing", () => { + it("selects direct gRPC before a mutation and exposes its close hook", () => { + const test = dependencies({ status: 0, stdout: "grpc", stderr: "" }); + + const selected = selectOpenShellSandboxControlForMutation("nemoclaw", test.deps); + + expect(selected).toMatchObject({ control: expect.any(Object), transport: "grpc" }); + expect(selected.control).not.toBe(test.cli); + expect(test.createCli).not.toHaveBeenCalled(); + selected.close(); + expect(test.close).toHaveBeenCalledOnce(); + }); + + it("preselects the CLI only for the edge-tunnel auth mode", () => { + const test = dependencies({ status: 0, stdout: "unused", stderr: "" }); + test.createGrpc.mockImplementation(() => { + throw new OpenShellGrpcEdgeTunnelRequiredError(); + }); + + const selected = selectOpenShellSandboxControlForMutation("edge", test.deps); + + expect(selected).toEqual({ + control: test.cli, + transport: "cli-edge-tunnel", + close: expect.any(Function), + }); + expect(test.createCli).toHaveBeenCalledWith("edge"); + selected.close(); + expect(test.close).not.toHaveBeenCalled(); + }); + + it("fails closed when the mutation CLI factory refuses an endpoint override", () => { + const refusal = new Error("Unset OPENSHELL_GATEWAY_ENDPOINT and retry"); + const test = dependencies({ status: 0, stdout: "unused", stderr: "" }); + test.createGrpc.mockImplementation(() => { + throw new OpenShellGrpcEdgeTunnelRequiredError(); + }); + test.createCli.mockImplementation(() => { + throw refusal; + }); + + expect(() => selectOpenShellSandboxControlForMutation("edge", test.deps)).toThrow(refusal); + + expect(test.createCli).toHaveBeenCalledWith("edge"); + expect(test.cliExec).not.toHaveBeenCalled(); + }); + + it("does not turn a completed mutation into failure when the client cannot close", () => { + const test = dependencies({ status: 0, stdout: "grpc", stderr: "" }); + const error = new Error("close failed"); + test.close.mockImplementation(() => { + throw error; + }); + const selected = selectOpenShellSandboxControlForMutation("nemoclaw", test.deps); + + expect(() => selected.close()).not.toThrow(); + expect(test.debug).toHaveBeenCalledWith("OpenShell direct gRPC client close failed", error); + }); + + it("fails before dispatch for every other direct-client configuration error", () => { + const test = dependencies({ status: 0, stdout: "unused", stderr: "" }); + const error = new Error("invalid mTLS material"); + test.createGrpc.mockImplementation(() => { + throw error; + }); + + expect(() => selectOpenShellSandboxControlForMutation("nemoclaw", test.deps)).toThrow(error); + expect(test.cliExec).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/adapters/openshell/sandbox-control-routing.ts b/src/lib/adapters/openshell/sandbox-control-routing.ts index 579ed0d4eac..c0f85a6592a 100644 --- a/src/lib/adapters/openshell/sandbox-control-routing.ts +++ b/src/lib/adapters/openshell/sandbox-control-routing.ts @@ -2,7 +2,10 @@ // SPDX-License-Identifier: Apache-2.0 import { log } from "../../cli/logger"; -import { createGrpcOpenShellSandboxControlForGateway } from "./grpc-gateway-config"; +import { + createGrpcOpenShellSandboxControlForGateway, + OpenShellGrpcEdgeTunnelRequiredError, +} from "./grpc-gateway-config"; import { type GrpcOpenShellSandboxControl, OpenShellGrpcPreDispatchError, @@ -10,11 +13,11 @@ import { import { createGatewayScopedCliOpenShellSandboxControl, OpenShellExecRequestValidationError, - openShellExecRequestValidationFailure, type OpenShellSandboxControl, + openShellExecRequestValidationFailure, type SandboxExecRequest, type SandboxExecResult, - validateOpenShellExecCommand, + validateOpenShellExecRequest, } from "./sandbox-control"; import { OPENSHELL_OPERATION_TIMEOUT_MS } from "./timeouts"; @@ -30,6 +33,45 @@ const defaultDependencies: ReadOnlyRoutingDependencies = { debug: (message, context) => log.debug(message, context), }; +export interface OpenShellMutationControlSelection { + control: OpenShellSandboxControl; + transport: "grpc" | "cli-edge-tunnel"; + close(): void; +} + +/** + * Select one transport before a mutating workflow starts. Direct gRPC is the + * default. The CLI is selected only for the explicit Cloudflare edge-tunnel + * mode that a direct client cannot traverse. This function never retries an + * operation after dispatch. + */ +export function selectOpenShellSandboxControlForMutation( + gatewayName: string, + dependencies: ReadOnlyRoutingDependencies = defaultDependencies, +): OpenShellMutationControlSelection { + try { + const grpc = dependencies.createGrpc(gatewayName); + return { + control: grpc, + transport: "grpc", + close: () => { + try { + grpc.close(); + } catch (error) { + dependencies.debug("OpenShell direct gRPC client close failed", error); + } + }, + }; + } catch (error) { + if (!(error instanceof OpenShellGrpcEdgeTunnelRequiredError)) throw error; + return { + control: dependencies.createCli(gatewayName), + transport: "cli-edge-tunnel", + close: () => {}, + }; + } +} + /** * Prefer direct gRPC for the OpenClaw session-list read and retry through the * OpenShell CLI only when configuration or sandbox lookup fails before Exec is @@ -49,7 +91,11 @@ export async function execSandboxReadOnlyWithGrpcFallback( request: SandboxExecRequest, dependencies: ReadOnlyRoutingDependencies = defaultDependencies, ): Promise { - const validationError = validateOpenShellExecCommand(request.command); + const routedRequest = { + ...request, + timeoutMs: request.timeoutMs ?? OPENSHELL_OPERATION_TIMEOUT_MS, + }; + const validationError = validateOpenShellExecRequest(routedRequest); if (validationError) return openShellExecRequestValidationFailure(validationError); let grpc: GrpcOpenShellSandboxControl; @@ -63,18 +109,12 @@ export async function execSandboxReadOnlyWithGrpcFallback( "OpenShell direct gRPC configuration failed; retrying through the CLI", error, ); - return dependencies.createCli(gatewayName).exec({ - ...request, - timeoutMs: request.timeoutMs ?? OPENSHELL_OPERATION_TIMEOUT_MS, - }); + return dependencies.createCli(gatewayName).exec(routedRequest); } let preDispatchError: OpenShellGrpcPreDispatchError | undefined; try { - const result = await grpc.exec({ - ...request, - timeoutMs: request.timeoutMs ?? OPENSHELL_OPERATION_TIMEOUT_MS, - }); + const result = await grpc.exec(routedRequest); if (!(result.error instanceof OpenShellGrpcPreDispatchError)) return result; preDispatchError = result.error; } catch (error) { @@ -96,8 +136,5 @@ export async function execSandboxReadOnlyWithGrpcFallback( "OpenShell direct gRPC lookup failed before dispatch; retrying through the CLI", preDispatchError.cause, ); - return dependencies.createCli(gatewayName).exec({ - ...request, - timeoutMs: request.timeoutMs ?? OPENSHELL_OPERATION_TIMEOUT_MS, - }); + return dependencies.createCli(gatewayName).exec(routedRequest); } diff --git a/src/lib/adapters/openshell/sandbox-control.test.ts b/src/lib/adapters/openshell/sandbox-control.test.ts index c418b6276f8..0a5e3762fdc 100644 --- a/src/lib/adapters/openshell/sandbox-control.test.ts +++ b/src/lib/adapters/openshell/sandbox-control.test.ts @@ -3,12 +3,15 @@ import { describe, expect, it, vi } from "vitest"; -import type { CaptureOpenshellResult } from "./client"; +import { type CaptureOpenshellBinaryResult, captureOpenshellCommandBinary } from "./client"; import { createCliOpenShellSandboxControl, createGatewayScopedCliOpenShellSandboxControl, + OPENSHELL_EXEC_MAX_OUTPUT_BYTES, + OpenShellExecOutputLimitError, OpenShellExecRequestValidationError, validateOpenShellExecCommand, + validateOpenShellExecRequest, } from "./sandbox-control"; function expectValidationIssue( @@ -96,14 +99,139 @@ describe("OpenShell exec command validation", () => { }); }); +describe("OpenShell exec request validation", () => { + it("allows the exact unary request boundary and rejects one byte more", () => { + const exactRequest = { + sandboxName: "alpha", + command: ["sh", "-s"], + stdin: Buffer.alloc(1_048_526), + }; + + expect(validateOpenShellExecRequest(exactRequest)).toBeNull(); + const error = validateOpenShellExecRequest({ + ...exactRequest, + stdin: Buffer.alloc(1_048_527), + }); + expect(error?.issue).toEqual({ + kind: "encoded-request-too-large", + actualBytes: 1_048_577, + maxBytes: 1_048_576, + }); + }); + + it("includes timeoutSeconds in the encoded request boundary", () => { + const exactRequest = { + sandboxName: "alpha", + command: ["sh", "-s"], + stdin: Buffer.alloc(1_048_524), + timeoutMs: 120_000, + }; + + expect(validateOpenShellExecRequest(exactRequest)).toBeNull(); + expect( + validateOpenShellExecRequest({ + ...exactRequest, + stdin: Buffer.alloc(1_048_525), + })?.issue, + ).toEqual({ + kind: "encoded-request-too-large", + actualBytes: 1_048_577, + maxBytes: 1_048_576, + }); + }); + + it("accepts the uint32 timeout boundary and rejects values protobufjs would wrap", () => { + const request = { + sandboxName: "alpha", + command: ["x"], + stdin: Buffer.alloc(1_048_525), + timeoutMs: 0xffff_ffff * 1000, + }; + + expect(validateOpenShellExecRequest(request)).toBeNull(); + expect( + validateOpenShellExecRequest({ + ...request, + stdin: Buffer.alloc(1_048_526), + })?.issue, + ).toEqual({ + kind: "encoded-request-too-large", + actualBytes: 1_048_577, + maxBytes: 1_048_576, + }); + expect( + validateOpenShellExecRequest({ + ...request, + timeoutMs: request.timeoutMs + 1, + })?.issue, + ).toEqual({ + kind: "timeout-out-of-range", + actualMs: 0xffff_ffff * 1000 + 1, + maxMs: 0xffff_ffff * 1000, + }); + }); + + it.each([ + -1, + 1.5, + Number.NaN, + Number.POSITIVE_INFINITY, + ])("rejects an invalid timeout of %s", (timeoutMs) => { + expect( + validateOpenShellExecRequest({ + sandboxName: "alpha", + command: ["true"], + timeoutMs, + })?.issue.kind, + ).toBe("timeout-out-of-range"); + }); + + it.each([ + -1, + 1.5, + Number.NaN, + Number.POSITIVE_INFINITY, + OPENSHELL_EXEC_MAX_OUTPUT_BYTES + 1, + ])("rejects an invalid output limit of %s", (maxOutputBytes) => { + expect( + validateOpenShellExecRequest({ + sandboxName: "alpha", + command: ["true"], + maxOutputBytes, + })?.issue, + ).toEqual({ + kind: "max-output-out-of-range", + actualBytes: maxOutputBytes, + minBytes: 0, + maxBytes: OPENSHELL_EXEC_MAX_OUTPUT_BYTES, + }); + }); + + it("accepts zero and the hard maximum output limits", () => { + expect( + validateOpenShellExecRequest({ + sandboxName: "alpha", + command: ["true"], + maxOutputBytes: 0, + }), + ).toBeNull(); + expect( + validateOpenShellExecRequest({ + sandboxName: "alpha", + command: ["true"], + maxOutputBytes: OPENSHELL_EXEC_MAX_OUTPUT_BYTES, + }), + ).toBeNull(); + }); +}); + describe("CLI OpenShell sandbox control", () => { it("maps a typed exec request to the existing CLI contract", async () => { const capture = vi.fn( - (): CaptureOpenshellResult => ({ + (): CaptureOpenshellBinaryResult => ({ status: 0, - output: "hello", - stdout: "hello\n", - stderr: "warning\n", + stdout: Buffer.from("hello\n"), + stderr: Buffer.from("warning\n"), }), ); const control = createCliOpenShellSandboxControl(capture); @@ -111,13 +239,18 @@ describe("CLI OpenShell sandbox control", () => { const result = await control.exec({ sandboxName: "alpha", command: ["openclaw", "sessions", "list", "--json"], + stdin: Buffer.from("request body"), maxOutputBytes: 4096, timeoutMs: 30_000, }); expect(capture).toHaveBeenCalledWith( ["sandbox", "exec", "--name", "alpha", "--", "openclaw", "sessions", "list", "--json"], - { ignoreError: true, includeStreams: true, maxBuffer: 4096, timeout: 30_000 }, + { + input: Buffer.from("request body"), + maxBuffer: 4096, + timeout: 30_000, + }, ); expect(result).toEqual({ status: 0, @@ -127,11 +260,14 @@ describe("CLI OpenShell sandbox control", () => { }); it("preserves transport failures without throwing", async () => { - const error = Object.assign(new Error("spawnSync openshell ENOBUFS"), { code: "ENOBUFS" }); + const error = Object.assign(new Error("spawnSync openshell EIO"), { + code: "EIO", + }); const capture = vi.fn( - (): CaptureOpenshellResult => ({ + (): CaptureOpenshellBinaryResult => ({ status: null, - output: "partial", + stdout: Buffer.from("partial"), + stderr: Buffer.alloc(0), error, signal: "SIGTERM", }), @@ -149,11 +285,10 @@ describe("CLI OpenShell sandbox control", () => { it("pins fallback execution to the requested gateway", async () => { const capture = vi.fn( - (): CaptureOpenshellResult => ({ + (): CaptureOpenshellBinaryResult => ({ status: 0, - output: "ok", - stdout: "ok\n", - stderr: "", + stdout: Buffer.from("ok\n"), + stderr: Buffer.alloc(0), }), ); const control = createGatewayScopedCliOpenShellSandboxControl("nemoclaw-19080", capture); @@ -162,12 +297,12 @@ describe("CLI OpenShell sandbox control", () => { expect(capture).toHaveBeenCalledWith( ["--gateway", "nemoclaw-19080", "sandbox", "exec", "--name", "alpha", "--", "true"], - expect.objectContaining({ ignoreError: true, includeStreams: true }), + expect.objectContaining({ maxBuffer: 1024 * 1024 }), ); }); it("rejects an ambient endpoint that could override the fallback gateway", () => { - const capture = vi.fn<() => CaptureOpenshellResult>(); + const capture = vi.fn<() => CaptureOpenshellBinaryResult>(); expect(() => createGatewayScopedCliOpenShellSandboxControl("nemoclaw-19080", capture, { @@ -178,10 +313,13 @@ describe("CLI OpenShell sandbox control", () => { }); it("returns a standard failure without capture for invalid commands", async () => { - const capture = vi.fn<() => CaptureOpenshellResult>(); + const capture = vi.fn<() => CaptureOpenshellBinaryResult>(); const control = createCliOpenShellSandboxControl(capture); - const result = await control.exec({ sandboxName: "alpha", command: ["bad\ncommand"] }); + const result = await control.exec({ + sandboxName: "alpha", + command: ["bad\ncommand"], + }); expect(capture).not.toHaveBeenCalled(); expect(result).toMatchObject({ status: null, stdout: "", stderr: "" }); @@ -193,19 +331,223 @@ describe("CLI OpenShell sandbox control", () => { }); }); + it("rejects an oversized encoded request without invoking the CLI", async () => { + const capture = vi.fn<() => CaptureOpenshellBinaryResult>(); + const control = createCliOpenShellSandboxControl(capture); + + const result = await control.exec({ + sandboxName: "alpha", + command: ["sh", "-s"], + stdin: Buffer.alloc(1_048_527), + }); + + expect(result.error).toBeInstanceOf(OpenShellExecRequestValidationError); + expect((result.error as OpenShellExecRequestValidationError).issue.kind).toBe( + "encoded-request-too-large", + ); + expect(capture).not.toHaveBeenCalled(); + }); + + it("rejects a timeout that cannot be represented by the v0.0.72 request", async () => { + const capture = vi.fn<() => CaptureOpenshellBinaryResult>(); + const control = createCliOpenShellSandboxControl(capture); + + const result = await control.exec({ + sandboxName: "alpha", + command: ["true"], + timeoutMs: 0xffff_ffff * 1000 + 1, + }); + + expect(result.error).toBeInstanceOf(OpenShellExecRequestValidationError); + expect((result.error as OpenShellExecRequestValidationError).issue.kind).toBe( + "timeout-out-of-range", + ); + expect(capture).not.toHaveBeenCalled(); + }); + + it("implements a zero-byte output cap without passing Node's unlimited maxBuffer=0", async () => { + const capture = vi.fn( + (): CaptureOpenshellBinaryResult => ({ + status: 0, + stdout: Buffer.from("visible output"), + stderr: Buffer.from("warning"), + }), + ); + const control = createCliOpenShellSandboxControl(capture); + + const result = await control.exec({ + sandboxName: "alpha", + command: ["true"], + maxOutputBytes: 0, + }); + + expect(result).toEqual({ + status: null, + stdout: "", + stderr: "", + error: expect.any(OpenShellExecOutputLimitError), + }); + expect(capture).toHaveBeenCalledWith( + expect.any(Array), + expect.objectContaining({ maxBuffer: 1 }), + ); + }); + + it("allows a command with a zero-byte output cap when it emits nothing", async () => { + const capture = vi.fn( + (): CaptureOpenshellBinaryResult => ({ + status: 0, + stdout: Buffer.alloc(0), + stderr: Buffer.alloc(0), + }), + ); + const control = createCliOpenShellSandboxControl(capture); + + await expect( + control.exec({ sandboxName: "alpha", command: ["true"], maxOutputBytes: 0 }), + ).resolves.toEqual({ status: 0, stdout: "", stderr: "" }); + expect(capture).toHaveBeenCalledWith( + expect.any(Array), + expect.objectContaining({ maxBuffer: 1 }), + ); + }); + + it("retains at most the requested combined CLI output and normalizes ENOBUFS", async () => { + const captureError = Object.assign(new Error("spawnSync openshell ENOBUFS"), { + code: "ENOBUFS", + }); + const capture = vi.fn( + (): CaptureOpenshellBinaryResult => ({ + status: null, + stdout: Buffer.from("abcdef"), + stderr: Buffer.from("warning"), + error: captureError, + }), + ); + const control = createCliOpenShellSandboxControl(capture); + + const result = await control.exec({ + sandboxName: "alpha", + command: ["true"], + maxOutputBytes: 4, + }); + + expect(result).toEqual({ + status: null, + stdout: "abcd", + stderr: "", + error: expect.any(OpenShellExecOutputLimitError), + }); + expect((result.error as NodeJS.ErrnoException).code).toBe("ENOBUFS"); + }); + + it("measures raw invalid UTF-8 bytes without a false output-limit failure", async () => { + const capture = vi.fn( + (): CaptureOpenshellBinaryResult => ({ + status: 0, + stdout: Buffer.from([0xff]), + stderr: Buffer.alloc(0), + }), + ); + const control = createCliOpenShellSandboxControl(capture); + + await expect( + control.exec({ sandboxName: "alpha", command: ["true"], maxOutputBytes: 1 }), + ).resolves.toEqual({ status: 0, stdout: "\ufffd", stderr: "" }); + }); + + it("preserves a complete multibyte sequence at its exact raw-byte boundary", async () => { + const capture = vi.fn( + (): CaptureOpenshellBinaryResult => ({ + status: 0, + stdout: Buffer.from("é"), + stderr: Buffer.alloc(0), + }), + ); + const control = createCliOpenShellSandboxControl(capture); + + await expect( + control.exec({ sandboxName: "alpha", command: ["true"], maxOutputBytes: 2 }), + ).resolves.toEqual({ status: 0, stdout: "é", stderr: "" }); + }); + + it("reports raw-byte overflow even when the cap splits a multibyte sequence", async () => { + const captureError = Object.assign(new Error("spawnSync openshell ENOBUFS"), { + code: "ENOBUFS", + }); + const capture = vi.fn( + (): CaptureOpenshellBinaryResult => ({ + status: null, + stdout: Buffer.from("é"), + stderr: Buffer.alloc(0), + error: captureError, + }), + ); + const control = createCliOpenShellSandboxControl(capture); + + await expect( + control.exec({ sandboxName: "alpha", command: ["true"], maxOutputBytes: 1 }), + ).resolves.toEqual({ + status: null, + stdout: "\ufffd", + stderr: "", + error: expect.any(OpenShellExecOutputLimitError), + }); + }); + + it("normalizes real child-process ENOBUFS through the CLI boundary", async () => { + const control = createCliOpenShellSandboxControl((_args, options) => + captureOpenshellCommandBinary( + process.execPath, + ["-e", "process.stdout.write(Buffer.from([0xc3, 0xa9]))"], + options, + ), + ); + + await expect( + control.exec({ sandboxName: "alpha", command: ["true"], maxOutputBytes: 1 }), + ).resolves.toMatchObject({ + status: null, + stdout: "\ufffd", + stderr: "", + error: expect.any(OpenShellExecOutputLimitError), + }); + }); + + it("normalizes the real child-process path back to a requested zero-byte cap", async () => { + const control = createCliOpenShellSandboxControl((_args, options) => + captureOpenshellCommandBinary( + process.execPath, + ["-e", "process.stdout.write(Buffer.from([0x78]))"], + options, + ), + ); + + await expect( + control.exec({ sandboxName: "alpha", command: ["true"], maxOutputBytes: 0 }), + ).resolves.toEqual({ + status: null, + stdout: "", + stderr: "", + error: expect.any(OpenShellExecOutputLimitError), + }); + }); + it("forwards an exact-boundary argument unchanged", async () => { const capture = vi.fn( - (_args: readonly string[]): CaptureOpenshellResult => ({ + (_args: readonly string[]): CaptureOpenshellBinaryResult => ({ status: 0, - output: "", - stdout: "", - stderr: "", + stdout: Buffer.alloc(0), + stderr: Buffer.alloc(0), }), ); const control = createCliOpenShellSandboxControl(capture); const boundaryArgument = "é".repeat(16 * 1024); - await control.exec({ sandboxName: "alpha", command: ["printf", boundaryArgument] }); + await control.exec({ + sandboxName: "alpha", + command: ["printf", boundaryArgument], + }); expect(capture).toHaveBeenCalledOnce(); expect(capture.mock.calls[0]?.[0]).toEqual([ diff --git a/src/lib/adapters/openshell/sandbox-control.ts b/src/lib/adapters/openshell/sandbox-control.ts index 5f41deb6c77..b4b1fe0d4cb 100644 --- a/src/lib/adapters/openshell/sandbox-control.ts +++ b/src/lib/adapters/openshell/sandbox-control.ts @@ -1,17 +1,19 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import type { CaptureOpenshellResult } from "./client"; -import { captureOpenshell } from "./runtime"; import { assertNoOpenShellGatewayEndpointOverride, type OpenShellGatewayEndpointEnvironment, } from "../../openshell-gateway-endpoint-guard"; +import type { CaptureOpenshellBinaryResult } from "./client"; +import { captureOpenshellBinary } from "./runtime"; export interface SandboxExecRequest { sandboxName: string; command: readonly string[]; - /** Maximum combined stdout and stderr bytes retained by the transport. */ + /** Optional bytes supplied to the remote command's standard input. */ + stdin?: string | Buffer; + /** Maximum combined raw stdout and stderr bytes retained before UTF-8 decoding. */ maxOutputBytes?: number; /** End-to-end lookup and execution deadline. Zero means no deadline. */ timeoutMs?: number; @@ -32,7 +34,19 @@ export interface OpenShellSandboxControl { export type OpenShellExecRequestValidationIssue = | { kind: "empty-command" } | { kind: "too-many-arguments"; actual: number; max: number } - | { kind: "assembled-command-too-large"; actualBytes: number; maxBytes: number } + | { + kind: "assembled-command-too-large"; + actualBytes: number; + maxBytes: number; + } + | { kind: "encoded-request-too-large"; actualBytes: number; maxBytes: number } + | { + kind: "max-output-out-of-range"; + actualBytes: number; + minBytes: number; + maxBytes: number; + } + | { kind: "timeout-out-of-range"; actualMs: number; maxMs: number } | { kind: "argument-too-large"; index: number; @@ -46,6 +60,8 @@ export type OpenShellExecRequestValidationIssue = }; export const OPENSHELL_EXEC_INVALID_ARGUMENT = "OPENSHELL_EXEC_INVALID_ARGUMENT"; +export const OPENSHELL_EXEC_DEFAULT_MAX_OUTPUT_BYTES = 1024 * 1024; +export const OPENSHELL_EXEC_MAX_OUTPUT_BYTES = 256 * 1024 * 1024; /** A command OpenShell v0.0.72 will reject before attempting sandbox execution. */ export class OpenShellExecRequestValidationError extends Error { @@ -57,11 +73,25 @@ export class OpenShellExecRequestValidationError extends Error { } } -type CaptureOpenShell = typeof captureOpenshell; +/** A transport-neutral combined stdout/stderr retention failure. */ +export class OpenShellExecOutputLimitError extends Error { + readonly code = "ENOBUFS"; + + constructor(readonly maxOutputBytes: number) { + super(`OpenShell exec output exceeded ${String(maxOutputBytes)} bytes`); + this.name = "OpenShellExecOutputLimitError"; + } +} + +type CaptureOpenShellBinary = typeof captureOpenshellBinary; const OPENSHELL_V0072_MAX_EXEC_COMMAND_ARGS = 1024; const OPENSHELL_V0072_MAX_EXEC_ARGUMENT_BYTES = 32 * 1024; const OPENSHELL_V0072_MAX_ASSEMBLED_COMMAND_BYTES = 256 * 1024; +const OPENSHELL_V0072_MAX_DECODED_GRPC_MESSAGE_BYTES = 1024 * 1024; +const OPENSHELL_V0072_MAX_EXEC_TIMEOUT_SECONDS = 0xffff_ffff; +const OPENSHELL_V0072_MAX_EXEC_TIMEOUT_MS = OPENSHELL_V0072_MAX_EXEC_TIMEOUT_SECONDS * 1000; +const OPENSHELL_V0072_SANDBOX_ID_PLACEHOLDER = "00000000-0000-0000-0000-000000000000"; function openShellExecRequestValidationMessage(issue: OpenShellExecRequestValidationIssue): string { switch (issue.kind) { @@ -71,6 +101,12 @@ function openShellExecRequestValidationMessage(issue: OpenShellExecRequestValida return `command array exceeds ${String(issue.max)} argument limit`; case "assembled-command-too-large": return `assembled command string exceeds ${String(issue.maxBytes)} byte limit`; + case "encoded-request-too-large": + return `encoded exec request exceeds ${String(issue.maxBytes)} byte limit`; + case "max-output-out-of-range": + return `maxOutputBytes must be a safe integer from ${String(issue.minBytes)} through ${String(issue.maxBytes)}`; + case "timeout-out-of-range": + return `timeoutMs must be a non-negative safe integer no greater than ${String(issue.maxMs)}`; case "argument-too-large": return `command argument ${String(issue.index)} exceeds ${String(issue.maxBytes)} byte limit`; case "argument-control-character": @@ -166,17 +202,133 @@ export function validateOpenShellExecCommand( return null; } +function protobufVarintByteLength(value: number): number { + let remaining = value; + let bytes = 1; + while (remaining >= 0x80) { + remaining = Math.floor(remaining / 0x80); + bytes += 1; + } + return bytes; +} + +function protobufLengthDelimitedFieldByteLength(valueBytes: number): number { + // Every ExecSandboxRequest field used here has a one-byte protobuf tag. + return 1 + protobufVarintByteLength(valueBytes) + valueBytes; +} + +function openShellExecRequestEncodedByteLength( + request: SandboxExecRequest, + sandboxId: string, +): number { + let bytes = protobufLengthDelimitedFieldByteLength(Buffer.byteLength(sandboxId, "utf8")); + for (const argument of request.command) { + bytes += protobufLengthDelimitedFieldByteLength(Buffer.byteLength(argument, "utf8")); + } + if (request.timeoutMs !== undefined && request.timeoutMs > 0) { + const timeoutSeconds = Math.ceil(request.timeoutMs / 1000); + bytes += 1 + protobufVarintByteLength(timeoutSeconds); + } + if (request.stdin !== undefined) { + const stdinBytes = Buffer.isBuffer(request.stdin) + ? request.stdin.length + : Buffer.byteLength(request.stdin, "utf8"); + bytes += protobufLengthDelimitedFieldByteLength(stdinBytes); + } + return bytes; +} + +/** Match v0.0.72's 1 MiB decoded unary gRPC request boundary. */ +export function validateOpenShellExecRequest( + request: SandboxExecRequest, + sandboxId: string = OPENSHELL_V0072_SANDBOX_ID_PLACEHOLDER, +): OpenShellExecRequestValidationError | null { + const commandError = validateOpenShellExecCommand(request.command); + if (commandError) return commandError; + + if ( + request.maxOutputBytes !== undefined && + (!Number.isSafeInteger(request.maxOutputBytes) || + request.maxOutputBytes < 0 || + request.maxOutputBytes > OPENSHELL_EXEC_MAX_OUTPUT_BYTES) + ) { + return new OpenShellExecRequestValidationError({ + kind: "max-output-out-of-range", + actualBytes: request.maxOutputBytes, + minBytes: 0, + maxBytes: OPENSHELL_EXEC_MAX_OUTPUT_BYTES, + }); + } + + if ( + request.timeoutMs !== undefined && + (!Number.isSafeInteger(request.timeoutMs) || + request.timeoutMs < 0 || + request.timeoutMs > OPENSHELL_V0072_MAX_EXEC_TIMEOUT_MS) + ) { + return new OpenShellExecRequestValidationError({ + kind: "timeout-out-of-range", + actualMs: request.timeoutMs, + maxMs: OPENSHELL_V0072_MAX_EXEC_TIMEOUT_MS, + }); + } + + const actualBytes = openShellExecRequestEncodedByteLength(request, sandboxId); + if (actualBytes > OPENSHELL_V0072_MAX_DECODED_GRPC_MESSAGE_BYTES) { + return new OpenShellExecRequestValidationError({ + kind: "encoded-request-too-large", + actualBytes, + maxBytes: OPENSHELL_V0072_MAX_DECODED_GRPC_MESSAGE_BYTES, + }); + } + return null; +} + export function openShellExecRequestValidationFailure( error: OpenShellExecRequestValidationError, ): SandboxExecResult { return { status: null, stdout: "", stderr: "", error }; } -function normalizeExecResult(result: CaptureOpenshellResult): SandboxExecResult { +function isOutputLimitError(error: Error | undefined): boolean { + return (error as NodeJS.ErrnoException | undefined)?.code === "ENOBUFS"; +} + +function retainCombinedOutput( + stdout: Buffer, + stderr: Buffer, + maxOutputBytes: number, +): { stdout: Buffer; stderr: Buffer; truncated: boolean } { + const retainedStdout = stdout.subarray(0, maxOutputBytes); + const remaining = Math.max(0, maxOutputBytes - retainedStdout.length); + const retainedStderr = stderr.subarray(0, remaining); + return { + stdout: retainedStdout, + stderr: retainedStderr, + truncated: retainedStdout.length < stdout.length || retainedStderr.length < stderr.length, + }; +} + +function normalizeExecResult( + result: CaptureOpenshellBinaryResult, + maxOutputBytes: number, +): SandboxExecResult { + const retained = retainCombinedOutput(result.stdout, result.stderr, maxOutputBytes); + const stdout = retained.stdout.toString("utf8"); + const stderr = retained.stderr.toString("utf8"); + if (retained.truncated || isOutputLimitError(result.error)) { + return { + status: null, + stdout, + stderr, + error: new OpenShellExecOutputLimitError(maxOutputBytes), + ...(result.signal !== undefined ? { signal: result.signal } : {}), + }; + } const normalized: SandboxExecResult = { status: result.status, - stdout: result.stdout ?? result.output, - stderr: result.stderr ?? "", + stdout, + stderr, }; if (result.error) normalized.error = result.error; if (result.signal !== undefined) normalized.signal = result.signal; @@ -184,15 +336,18 @@ function normalizeExecResult(result: CaptureOpenshellResult): SandboxExecResult } function createCliSandboxControl( - capture: CaptureOpenShell, + capture: CaptureOpenShellBinary, gatewayName?: string, ): OpenShellSandboxControl { return { async exec(request): Promise { - const validationError = validateOpenShellExecCommand(request.command); + // v0.0.72 creates UUID sandbox ids. Reserve that exact encoded width + // before invoking the CLI, which resolves the name to the id internally. + const validationError = validateOpenShellExecRequest(request); if (validationError) return openShellExecRequestValidationFailure(validationError); const gatewayArgs = gatewayName ? ["--gateway", gatewayName] : []; + const maxOutputBytes = request.maxOutputBytes ?? OPENSHELL_EXEC_DEFAULT_MAX_OUTPUT_BYTES; const result = capture( [ ...gatewayArgs, @@ -204,19 +359,20 @@ function createCliSandboxControl( ...request.command, ], { - ignoreError: true, - includeStreams: true, - maxBuffer: request.maxOutputBytes, + input: request.stdin, + // Node treats maxBuffer=0 as unlimited. One byte is the smallest + // bounded capture; normalize it back to the requested zero-byte cap. + maxBuffer: maxOutputBytes === 0 ? 1 : maxOutputBytes, timeout: request.timeoutMs, }, ); - return normalizeExecResult(result); + return normalizeExecResult(result, maxOutputBytes); }, }; } export function createCliOpenShellSandboxControl( - capture: CaptureOpenShell = captureOpenshell, + capture: CaptureOpenShellBinary = captureOpenshellBinary, ): OpenShellSandboxControl { return createCliSandboxControl(capture); } @@ -224,7 +380,7 @@ export function createCliOpenShellSandboxControl( /** Bind every CLI fallback invocation to the gateway selected by the caller. */ export function createGatewayScopedCliOpenShellSandboxControl( gatewayName: string, - capture: CaptureOpenShell = captureOpenshell, + capture: CaptureOpenShellBinary = captureOpenshellBinary, env: OpenShellGatewayEndpointEnvironment = process.env, ): OpenShellSandboxControl { assertNoOpenShellGatewayEndpointOverride(env); diff --git a/src/lib/skill-install.test.ts b/src/lib/skill-install.test.ts index d3d1438327b..72155f7deec 100644 --- a/src/lib/skill-install.test.ts +++ b/src/lib/skill-install.test.ts @@ -12,10 +12,18 @@ import { postInstall, resolveSkillPaths, shellQuote, + uploadFile, validateRelativePath, verifyInstall, } from "./skill-install"; +const controlContext = { + control: { + exec: async () => ({ status: 0, stdout: "", stderr: "" }), + }, + sandboxName: "alpha", +}; + describe("parseFrontmatter", () => { it("extracts name from valid frontmatter", () => { const result = parseFrontmatter("---\nname: my-skill\ndescription: test\n---\n# Body"); @@ -251,17 +259,17 @@ describe("resolveSkillPaths", () => { }); describe("postInstall", () => { - it("refreshes OpenClaw sessions after installing an updated skill", () => { + it("refreshes OpenClaw sessions after installing an updated skill", async () => { const skillDir = mkdtempSync(join(tmpdir(), "skill-postinstall-")); const commands: string[] = []; try { writeFileSync(skillDir + "/SKILL.md", "---\nname: weather\n---\n# Weather\n"); - const result = postInstall( - { configFile: "/tmp/ssh-config", sandboxName: "alpha" }, + const result = await postInstall( + controlContext, resolveSkillPaths(null, "weather"), skillDir, { - sshExecImpl: (_ctx, command) => { + execImpl: async (_ctx, command) => { commands.push(command); return { status: 0, stdout: "", stderr: "" }; }, @@ -277,7 +285,7 @@ describe("postInstall", () => { } }); - it("mirrors the uploaded skill into the OpenClaw home dir so the agent loads it", () => { + it("mirrors the uploaded skill into the OpenClaw home dir so the agent loads it", async () => { // Regression for #4819: on sandboxes whose agent $HOME differs from the // OpenClaw state dir, `skills list` shows the upload dir while the agent // loads skills from $HOME/.openclaw/skills. Install must populate that @@ -287,8 +295,8 @@ describe("postInstall", () => { try { writeFileSync(skillDir + "/SKILL.md", "---\nname: report-writer\n---\n# Report\n"); const paths = resolveSkillPaths(null, "report-writer"); - postInstall({ configFile: "/tmp/ssh-config", sandboxName: "alpha" }, paths, skillDir, { - sshExecImpl: (_ctx, command) => { + await postInstall(controlContext, paths, skillDir, { + execImpl: async (_ctx, command) => { commands.push(command); return { status: 0, stdout: "", stderr: "" }; }, @@ -307,24 +315,19 @@ describe("postInstall", () => { } }); - it("warns when the OpenClaw home mirror cannot be created", () => { + it("warns when the OpenClaw home mirror cannot be created", async () => { const skillDir = mkdtempSync(join(tmpdir(), "skill-postinstall-mirror-fail-")); try { writeFileSync(skillDir + "/SKILL.md", "---\nname: report-writer\n---\n# Report\n"); const paths = resolveSkillPaths(null, "report-writer"); - const result = postInstall( - { configFile: "/tmp/ssh-config", sandboxName: "alpha" }, - paths, - skillDir, - { - sshExecImpl: (_ctx, command) => ({ - // Fail only the mirror command; session refresh still succeeds. - status: command.includes("$HOME/.openclaw/skills") ? 1 : 0, - stdout: "", - stderr: "", - }), - }, - ); + const result = await postInstall(controlContext, paths, skillDir, { + execImpl: async (_ctx, command) => ({ + // Fail only the mirror command; session refresh still succeeds. + status: command.includes("$HOME/.openclaw/skills") ? 1 : 0, + stdout: "", + stderr: "", + }), + }); expect(result.success).toBe(true); expect(result.messages.some((m) => m.startsWith("Warning:") && m.includes("mirror"))).toBe( @@ -337,14 +340,14 @@ describe("postInstall", () => { }); describe("verifyInstall", () => { - it("requires SKILL.md in the OpenClaw home mirror, not only the upload dir (#4819)", () => { + it("requires SKILL.md in the OpenClaw home mirror, not only the upload dir (#4819)", async () => { // The agent loads skills from the home mirror, so an install whose mirror // copy failed must NOT verify as installed — otherwise the CLI reports // success while the skill stays invisible to the agent. const paths = resolveSkillPaths(null, "report-writer"); const commands: string[] = []; - const ok = verifyInstall({ configFile: "/tmp/ssh-config", sandboxName: "alpha" }, paths, { - sshExecImpl: (_ctx, command) => { + const ok = await verifyInstall(controlContext, paths, { + execImpl: async (_ctx, command) => { commands.push(command); return { status: 0, stdout: "EXISTS", stderr: "" }; }, @@ -357,14 +360,38 @@ describe("verifyInstall", () => { ).toBe(true); }); - it("returns false when the upload dir has SKILL.md but the home mirror does not", () => { + it("returns false when the upload dir has SKILL.md but the home mirror does not", async () => { const paths = resolveSkillPaths(null, "report-writer"); - const ok = verifyInstall({ configFile: "/tmp/ssh-config", sandboxName: "alpha" }, paths, { + const ok = await verifyInstall(controlContext, paths, { // A combined `test -f A && test -f B` shell command fails (non-zero, // no EXISTS) when the mirror file is absent. - sshExecImpl: () => ({ status: 1, stdout: "", stderr: "" }), + execImpl: async () => ({ status: 1, stdout: "", stderr: "" }), }); expect(ok).toBe(false); }); }); + +describe("uploadFile", () => { + it("sends file bytes through sandbox exec stdin", async () => { + const dir = mkdtempSync(join(tmpdir(), "skill-upload-")); + const localPath = join(dir, "binary.dat"); + const input = Buffer.from([0, 255, 10]); + writeFileSync(localPath, input); + let observedInput: string | Buffer | undefined; + try { + const result = await uploadFile(controlContext, localPath, "/sandbox/skill", "binary.dat", { + execImpl: async (_ctx, command, options) => { + expect(command).toBe("mkdir -p '/sandbox/skill' && cat > '/sandbox/skill/binary.dat'"); + observedInput = options?.input; + return { status: 0, stdout: "", stderr: "" }; + }, + }); + + expect(result?.status).toBe(0); + expect(observedInput).toEqual(input); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/src/lib/skill-install.ts b/src/lib/skill-install.ts index ffdcaf06e70..f6f5a6d7724 100644 --- a/src/lib/skill-install.ts +++ b/src/lib/skill-install.ts @@ -3,7 +3,7 @@ // // Skill install/remove logic for `nemoclaw skill install ` // and `nemoclaw skill remove `. -// Validates a local SKILL.md, uploads it to the sandbox via SSH, and +// Validates a local SKILL.md, uploads it through OpenShell sandbox exec, and // performs agent-specific post-install steps (session refresh for // OpenClaw). Non-OpenClaw agents get a "restart gateway" hint until a // generic refresh contract is defined in the manifest schema. @@ -16,18 +16,19 @@ import YAML from "yaml"; import { isObjectRecord } from "./core/json-types"; import { validateSkillName } from "./skill-name"; -import type { SshContext, SshResult } from "./skill-remote"; -import { shellQuote, sshExec } from "./skill-remote"; +import type { SandboxCommandResult, SandboxControlContext, SandboxExecImpl } from "./skill-remote"; +import { sandboxExec, shellQuote } from "./skill-remote"; export { validateSkillName } from "./skill-name"; export { checkExisting, type RemoveResult, removeSkill, - type SshContext, - type SshResult, + type SandboxCommandResult, + type SandboxControlContext, + type SandboxExecImpl, + sandboxExec, shellQuote, - sshExec, verifyRemove, } from "./skill-remote"; @@ -146,19 +147,20 @@ export function validateRelativePath(rel: string): boolean { // ── Upload helpers ─────────────────────────────────────────────── /** - * Upload a file to the sandbox by piping its content through SSH stdin. + * Upload a file to the sandbox through the selected OpenShell control plane. * Creates the target directory and writes the file in a single remote command. */ -export function uploadFile( - ctx: SshContext, +export async function uploadFile( + ctx: SandboxControlContext, localPath: string, remoteDir: string, remoteFilename: string, -): SshResult | null { + opts: { execImpl?: SandboxExecImpl } = {}, +): Promise { const content = fs.readFileSync(localPath); const remotePath = `${remoteDir}/${remoteFilename}`; const script = `mkdir -p ${shellQuote(remoteDir)} && cat > ${shellQuote(remotePath)}`; - return sshExec(ctx, script, { input: content }); + return (opts.execImpl ?? sandboxExec)(ctx, script, { input: content }); } export interface CollectedFiles { @@ -171,7 +173,7 @@ export interface CollectedFiles { * Collect files under `dir` recursively, returning paths relative to `dir`. * Dotfiles (names starting with `.`) are excluded by default and reported * separately so the caller can warn. Paths with unsafe characters are - * rejected to prevent shell injection when interpolated into SSH commands. + * rejected to prevent shell injection when interpolated into remote commands. */ export function collectFiles(dir: string): CollectedFiles { const files: string[] = []; @@ -204,11 +206,17 @@ export function collectFiles(dir: string): CollectedFiles { * Upload an entire skill directory to the sandbox, preserving subdirectory * structure. Rejects files with unsafe path characters and skips dotfiles. */ -export function uploadDirectory( - ctx: SshContext, +export async function uploadDirectory( + ctx: SandboxControlContext, localDir: string, remoteDir: string, -): { uploaded: number; failed: string[]; skippedDotfiles: string[]; unsafePaths: string[] } { + opts: { execImpl?: SandboxExecImpl } = {}, +): Promise<{ + uploaded: number; + failed: string[]; + skippedDotfiles: string[]; + unsafePaths: string[]; +}> { const { files, skippedDotfiles, unsafePaths } = collectFiles(localDir); if (unsafePaths.length > 0) { return { uploaded: 0, failed: unsafePaths, skippedDotfiles, unsafePaths }; @@ -217,7 +225,7 @@ export function uploadDirectory( for (const rel of files) { const localFile = path.join(localDir, rel); const remoteSubdir = rel.includes("/") ? `${remoteDir}/${path.dirname(rel)}` : remoteDir; - const result = uploadFile(ctx, localFile, remoteSubdir, path.basename(rel)); + const result = await uploadFile(ctx, localFile, remoteSubdir, path.basename(rel), opts); if (!result || result.status !== 0) { failed.push(rel); } @@ -229,17 +237,17 @@ export function uploadDirectory( * Run post-install steps: session refresh for OpenClaw, or * non-OpenClaw restart hint. */ -export function postInstall( - ctx: SshContext, +export async function postInstall( + ctx: SandboxControlContext, paths: SkillPaths, _localSkillDir: string, opts: { skipRefresh?: boolean; - sshExecImpl?: typeof sshExec; + execImpl?: SandboxExecImpl; } = {}, -): { success: boolean; messages: string[] } { +): Promise<{ success: boolean; messages: string[] }> { const messages: string[] = []; - const runSsh = opts.sshExecImpl ?? sshExec; + const run = opts.execImpl ?? sandboxExec; if (paths.isOpenClaw) { // Mirror the uploaded skill into the agent's home dir @@ -259,7 +267,7 @@ export function postInstall( // restricted to [A-Za-z0-9._-] by parseFrontmatter / the name regex. const dst = `"${paths.mirrorDir}"`; const mirrorParent = `"${paths.mirrorDir.slice(0, paths.mirrorDir.lastIndexOf("/"))}"`; - const mirrorResult = runSsh( + const mirrorResult = await run( ctx, `[ ${src} -ef ${dst} ] || { mkdir -p ${mirrorParent} && rm -rf ${dst} && cp -a ${src} ${dst}; }`, ); @@ -273,7 +281,7 @@ export function postInstall( // Clear sessions.json so OpenClaw re-discovers skills on the next // session even after an in-place skill update. if (paths.sessionFile && !opts.skipRefresh) { - const refreshResult = runSsh(ctx, `printf '{}' > ${shellQuote(paths.sessionFile)}`); + const refreshResult = await run(ctx, `printf '{}' > ${shellQuote(paths.sessionFile)}`); if (!refreshResult || refreshResult.status !== 0) { messages.push("Warning: failed to clear sessions (agent may need manual restart)"); } @@ -294,11 +302,11 @@ export function postInstall( * otherwise the CLI reports success while the skill stays invisible to the * agent. This mirrors verifyRemove(), which already checks both paths. */ -export function verifyInstall( - ctx: SshContext, +export async function verifyInstall( + ctx: SandboxControlContext, paths: SkillPaths, - opts: { sshExecImpl?: typeof sshExec } = {}, -): boolean { + opts: { execImpl?: SandboxExecImpl } = {}, +): Promise { const checks = [`test -f ${shellQuote(`${paths.uploadDir}/SKILL.md`)}`]; if (paths.isOpenClaw && paths.mirrorDir) { // mirrorDir contains $HOME, which must expand on the remote shell, so we @@ -306,7 +314,6 @@ export function verifyInstall( // restricted to [A-Za-z0-9._-]. checks.push(`test -f "${paths.mirrorDir}/SKILL.md"`); } - const runSsh = opts.sshExecImpl ?? sshExec; - const result = runSsh(ctx, `${checks.join(" && ")} && echo EXISTS`); + const result = await (opts.execImpl ?? sandboxExec)(ctx, `${checks.join(" && ")} && echo EXISTS`); return result !== null && result.stdout === "EXISTS"; } diff --git a/src/lib/skill-remote.test.ts b/src/lib/skill-remote.test.ts index 729514f657d..8a45b2a9f42 100644 --- a/src/lib/skill-remote.test.ts +++ b/src/lib/skill-remote.test.ts @@ -1,76 +1,153 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; +import type { OpenShellSandboxControl } from "./adapters/openshell/sandbox-control"; import { resolveSkillPaths } from "./skill-install"; import { validateSkillName } from "./skill-name"; -import { checkExisting, removeSkill, verifyRemove } from "./skill-remote"; +import { + checkExisting, + removeSkill, + type SandboxControlContext, + sandboxExec, + verifyRemove, +} from "./skill-remote"; + +function context( + exec: OpenShellSandboxControl["exec"] = vi.fn(async () => { + throw new Error("unreachable"); + }), +): SandboxControlContext { + return { control: { exec }, sandboxName: "test-sandbox" }; +} describe("validateSkillName", () => { - it("accepts valid skill names", () => { - expect(validateSkillName("my-skill")).toBe(true); - expect(validateSkillName("my_skill")).toBe(true); - expect(validateSkillName("my.skill")).toBe(true); - expect(validateSkillName("MySkill123")).toBe(true); - expect(validateSkillName("digicon-zeiss-ai-strategy")).toBe(true); - }); - - it("rejects empty string", () => { - expect(validateSkillName("")).toBe(false); - }); - - it("rejects names with spaces", () => { - expect(validateSkillName("my skill")).toBe(false); - }); - - it("rejects names with shell metacharacters", () => { - expect(validateSkillName("my;skill")).toBe(false); - expect(validateSkillName("my$skill")).toBe(false); - expect(validateSkillName("my/skill")).toBe(false); - expect(validateSkillName("../escape")).toBe(false); - expect(validateSkillName("my`skill`")).toBe(false); - }); + it.each([ + "my-skill", + "my_skill", + "my.skill", + "MySkill123", + "digicon-zeiss-ai-strategy", + ])("accepts %s", (name) => expect(validateSkillName(name)).toBe(true)); + + it.each([ + "", + "my skill", + "my;skill", + "my$skill", + "my/skill", + "../escape", + "my`skill`", + ".", + "..", + ])("rejects %s", (name) => expect(validateSkillName(name)).toBe(false)); +}); - it("rejects dot and double-dot to prevent directory traversal on rm -rf", () => { - expect(validateSkillName(".")).toBe(false); - expect(validateSkillName("..")).toBe(false); +describe("sandboxExec", () => { + it("runs one shell command through the selected control without transport retry", async () => { + const exec = vi.fn(async () => ({ status: 0, stdout: "ok\n", stderr: " warning\n" })); + const ctx = context(exec); + + await expect( + sandboxExec(ctx, "cat > /tmp/file", { input: Buffer.from("body") }), + ).resolves.toEqual({ + status: 0, + stdout: "ok", + stderr: "warning", + }); + expect(exec).toHaveBeenCalledOnce(); + expect(exec).toHaveBeenCalledWith({ + sandboxName: "test-sandbox", + command: ["sh", "-lc", "cat > /tmp/file"], + stdin: Buffer.from("body"), + timeoutMs: 30_000, + }); }); }); -describe("removeSkill (unit — no SSH)", () => { - it("returns success=false and a warning when sshExec returns null (sandbox unreachable)", () => { - const paths = resolveSkillPaths(null, "test-skill"); - - const ctx = { configFile: "/nonexistent/ssh.conf", sandboxName: "test-sandbox" }; - const result = removeSkill(ctx, paths); +describe("removeSkill", () => { + it("returns failure and warnings when sandbox execution is unavailable", async () => { + const result = await removeSkill(context(), resolveSkillPaths(null, "test-skill")); expect(result.success).toBe(false); expect(result.removedUploadDir).toBe(false); - expect(result.messages.some((m) => m.startsWith("Warning:"))).toBe(true); + expect(result.messages.some((message) => message.startsWith("Warning:"))).toBe(true); }); - it("success is false for OpenClaw when mirrorDir removal fails even if uploadDir was removed", () => { - const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; - const paths = resolveSkillPaths(null, "test-skill"); - const result = removeSkill(ctx, paths, { - sshExecImpl: (_ctx, command) => ({ + it("fails when the OpenClaw mirror removal fails after upload removal", async () => { + const result = await removeSkill(context(), resolveSkillPaths(null, "test-skill"), { + execImpl: async (_ctx, command) => ({ status: command.includes("$HOME/.openclaw/skills") ? 1 : 0, stdout: "", stderr: "", }), }); - expect(result.removedUploadDir).toBe(true); - expect(result.removedMirrorDir).toBe(false); - expect(result.success).toBe(false); + expect(result).toMatchObject({ + removedUploadDir: true, + removedMirrorDir: false, + success: false, + }); }); - it("removes OpenClaw upload and mirror dirs, then clears sessions", () => { - const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; + it("converges when retried after removing only the upload directory", async () => { const paths = resolveSkillPaths(null, "test-skill"); + const mirrorDir = paths.mirrorDir!; + const unrelatedPath = "/sandbox/.openclaw/skills/other-skill"; + const clearSessions = `printf '{}' > '${paths.sessionFile}'`; + const verifyGone = `test ! -e '${paths.uploadDir}' && test ! -e "${mirrorDir}" && echo GONE || echo EXISTS`; + const scriptedResults = [ + { + command: `rm -rf '${paths.uploadDir}'`, + result: { status: 0, stdout: "", stderr: "" }, + }, + { + command: `rm -rf "${mirrorDir}"`, + result: { status: 1, stdout: "", stderr: "failed" }, + }, + { command: clearSessions, result: { status: 0, stdout: "", stderr: "" } }, + { + command: `rm -rf '${paths.uploadDir}'`, + result: { status: 0, stdout: "", stderr: "" }, + }, + { + command: `rm -rf "${mirrorDir}"`, + result: { status: 0, stdout: "", stderr: "" }, + }, + { command: clearSessions, result: { status: 0, stdout: "", stderr: "" } }, + { + command: verifyGone, + result: { status: 0, stdout: "GONE", stderr: "" }, + }, + ]; + const execImpl = vi.fn(async (_ctx: SandboxControlContext, command: string) => { + const next = scriptedResults.shift(); + expect(next).toBeDefined(); + expect(command).toBe(next!.command); + expect(command).not.toContain(unrelatedPath); + return next!.result; + }); + + await expect(removeSkill(context(), paths, { execImpl })).resolves.toMatchObject({ + removedUploadDir: true, + removedMirrorDir: false, + success: false, + }); + await expect(removeSkill(context(), paths, { execImpl })).resolves.toMatchObject({ + clearedSessions: true, + removedMirrorDir: true, + removedUploadDir: true, + success: true, + }); + await expect(verifyRemove(context(), paths, { execImpl })).resolves.toBe(true); + + expect(scriptedResults).toEqual([]); + }); + + it("removes OpenClaw upload and mirror dirs, then clears sessions", async () => { const commands: string[] = []; - const result = removeSkill(ctx, paths, { - sshExecImpl: (_ctx, command) => { + const result = await removeSkill(context(), resolveSkillPaths(null, "test-skill"), { + execImpl: async (_ctx, command) => { commands.push(command); return { status: 0, stdout: "", stderr: "" }; }, @@ -86,28 +163,17 @@ describe("removeSkill (unit — no SSH)", () => { }); }); -describe("verifyRemove (unit — no SSH)", () => { - it("returns false when SSH is unreachable (conservative — treat failure as not-gone)", () => { - const paths = resolveSkillPaths(null, "test-skill"); - const ctx = { configFile: "/nonexistent/ssh.conf", sandboxName: "test-sandbox" }; - expect(verifyRemove(ctx, paths)).toBe(false); - }); - - it("returns false for non-OpenClaw paths when SSH is unreachable", () => { - const paths = resolveSkillPaths( - { name: "hermes", configPaths: { dir: "/sandbox/.hermes" } }, - "test-skill", +describe("verifyRemove", () => { + it("fails conservatively when sandbox execution is unavailable", async () => { + await expect(verifyRemove(context(), resolveSkillPaths(null, "test-skill"))).resolves.toBe( + false, ); - const ctx = { configFile: "/nonexistent/ssh.conf", sandboxName: "test-sandbox" }; - expect(verifyRemove(ctx, paths)).toBe(false); }); - it("verifies both OpenClaw skill directories are gone", () => { - const paths = resolveSkillPaths(null, "test-skill"); - const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; + it("verifies both OpenClaw skill directories are gone", async () => { const commands: string[] = []; - const gone = verifyRemove(ctx, paths, { - sshExecImpl: (_ctx, command) => { + const gone = await verifyRemove(context(), resolveSkillPaths(null, "test-skill"), { + execImpl: async (_ctx, command) => { commands.push(command); return { status: 0, stdout: "GONE", stderr: "" }; }, @@ -120,28 +186,25 @@ describe("verifyRemove (unit — no SSH)", () => { }); }); -describe("checkExisting (unit — no SSH)", () => { - it("returns null when SSH is unreachable for OpenClaw paths", () => { - const paths = resolveSkillPaths(null, "test-skill"); - const ctx = { configFile: "/nonexistent/ssh.conf", sandboxName: "test-sandbox" }; - expect(checkExisting(ctx, paths)).toBeNull(); +describe("checkExisting", () => { + it("maps a selected-control execution failure to an inconclusive result", async () => { + await expect( + checkExisting(context(), resolveSkillPaths(null, "test-skill")), + ).resolves.toBeNull(); }); - it("returns null when SSH is unreachable for non-OpenClaw paths", () => { - const paths = resolveSkillPaths( - { name: "hermes", configPaths: { dir: "/sandbox/.hermes" } }, - "test-skill", - ); - const ctx = { configFile: "/nonexistent/ssh.conf", sandboxName: "test-sandbox" }; - expect(checkExisting(ctx, paths)).toBeNull(); + it("maps a successful absence sentinel to false", async () => { + const execImpl = vi.fn(async () => ({ status: 0, stdout: "ABSENT", stderr: "" })); + + await expect( + checkExisting(context(), resolveSkillPaths(null, "test-skill"), { execImpl }), + ).resolves.toBe(false); }); - it("probes skill directories so removal can clean partial uploads", () => { - const paths = resolveSkillPaths(null, "test-skill"); - const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; + it("probes directories so removal can clean partial uploads", async () => { const commands: string[] = []; - const exists = checkExisting(ctx, paths, { - sshExecImpl: (_ctx, command) => { + const exists = await checkExisting(context(), resolveSkillPaths(null, "test-skill"), { + execImpl: async (_ctx, command) => { commands.push(command); return { status: 0, stdout: "EXISTS", stderr: "" }; }, diff --git a/src/lib/skill-remote.ts b/src/lib/skill-remote.ts index 8b19d1780a7..73ddbd16f88 100644 --- a/src/lib/skill-remote.ts +++ b/src/lib/skill-remote.ts @@ -1,61 +1,46 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { spawnSync } from "node:child_process"; - +import type { OpenShellSandboxControl } from "./adapters/openshell/sandbox-control"; import { shellQuote } from "./core/shell-quote"; import type { SkillPaths } from "./skill-install"; export { shellQuote }; -export interface SshContext { - configFile: string; +export interface SandboxControlContext { + control: OpenShellSandboxControl; sandboxName: string; } -export interface SshResult { - status: number; +export interface SandboxCommandResult { + status: number | null; stdout: string; stderr: string; } -/** - * Run a command on the sandbox via SSH with optional stdin content. - * Uses the same SSH flags as executeSandboxCommand in sandbox-process-recovery-action.ts. - */ -export function sshExec( - ctx: SshContext, +export type SandboxExecImpl = ( + ctx: SandboxControlContext, + command: string, + opts?: { input?: string | Buffer; timeout?: number }, +) => Promise; + +/** Execute one shell command through the selected OpenShell control plane. */ +export async function sandboxExec( + ctx: SandboxControlContext, command: string, opts: { input?: string | Buffer; timeout?: number } = {}, -): SshResult | null { +): Promise { try { - const result = spawnSync( - "ssh", - [ - "-F", - ctx.configFile, - "-o", - "StrictHostKeyChecking=no", - "-o", - "UserKnownHostsFile=/dev/null", - "-o", - "ConnectTimeout=10", - "-o", - "LogLevel=ERROR", - `openshell-${ctx.sandboxName}`, - command, - ], - { - encoding: "utf-8", - stdio: [opts.input !== undefined ? "pipe" : "ignore", "pipe", "pipe"], - input: opts.input, - timeout: opts.timeout ?? 30_000, - }, - ); + const result = await ctx.control.exec({ + sandboxName: ctx.sandboxName, + command: ["sh", "-lc", command], + stdin: opts.input, + timeoutMs: opts.timeout ?? 30_000, + }); return { - status: result.status ?? 1, - stdout: (result.stdout || "").trim(), - stderr: (result.stderr || "").trim(), + status: result.status, + stdout: result.stdout.trim(), + stderr: result.stderr.trim(), }; } catch { return null; @@ -63,30 +48,24 @@ export function sshExec( } /** - * Check whether a skill directory already exists on the sandbox at the upload - * path or (for OpenClaw) the mirror path. Probing directories instead of only - * SKILL.md lets `skill remove` clean up partial uploads whose manifest write - * failed after the directory was created. - * - * Returns: - * true — skill exists - * false — skill is absent - * null — SSH probe failed; existence could not be determined + * Check whether a skill directory already exists at the upload path or, for + * OpenClaw, the mirror path. Directory probes let removal clean partial + * uploads whose manifest write failed. A recognized sentinel from a + * successful command maps to true or false. Transport errors, nonzero status, + * and malformed output map to null so callers can fail before mutation. */ -export function checkExisting( - ctx: SshContext, +export async function checkExisting( + ctx: SandboxControlContext, paths: SkillPaths, - opts: { sshExecImpl?: typeof sshExec } = {}, -): boolean | null { + opts: { execImpl?: SandboxExecImpl } = {}, +): Promise { const checks = [`test -e ${shellQuote(paths.uploadDir)}`]; - if (paths.isOpenClaw && paths.mirrorDir) { - checks.push(`test -e "${paths.mirrorDir}"`); - } - const runSsh = opts.sshExecImpl ?? sshExec; - const result = runSsh(ctx, `{ ${checks.join(" || ")}; } && echo EXISTS || echo ABSENT`); - if (result === null || result.status !== 0) { - return null; - } + if (paths.isOpenClaw && paths.mirrorDir) checks.push(`test -e "${paths.mirrorDir}"`); + const result = await (opts.execImpl ?? sandboxExec)( + ctx, + `{ ${checks.join(" || ")}; } && echo EXISTS || echo ABSENT`, + ); + if (result === null || result.status !== 0) return null; if (result.stdout === "EXISTS") return true; if (result.stdout === "ABSENT") return false; return null; @@ -100,48 +79,33 @@ export interface RemoveResult { messages: string[]; } -/** - * Remove a skill from the sandbox by name. - * Deletes the immutable upload directory, the OpenClaw mirror directory - * (if applicable), and clears sessions.json so the agent re-discovers - * the remaining skills on the next session. - * - * Only the named skill directory is deleted — other skills are untouched. - */ -export function removeSkill( - ctx: SshContext, +/** Remove one named skill and clear OpenClaw's session index. */ +export async function removeSkill( + ctx: SandboxControlContext, paths: SkillPaths, - opts: { sshExecImpl?: typeof sshExec } = {}, -): RemoveResult { + opts: { execImpl?: SandboxExecImpl } = {}, +): Promise { const messages: string[] = []; - const runSsh = opts.sshExecImpl ?? sshExec; + const run = opts.execImpl ?? sandboxExec; - // 1. Remove the immutable upload directory (/sandbox/.openclaw/skills//) - const uploadDir = shellQuote(paths.uploadDir); - const removeUpload = runSsh(ctx, `rm -rf ${uploadDir}`); + const removeUpload = await run(ctx, `rm -rf ${shellQuote(paths.uploadDir)}`); const removedUploadDir = removeUpload !== null && removeUpload.status === 0; if (!removedUploadDir) { messages.push(`Warning: failed to remove upload directory ${paths.uploadDir}`); } - // 2. Remove the OpenClaw mirror ($HOME/.openclaw/skills//) - // mirrorDir contains $HOME which must expand on the remote shell, so we - // use double quotes (not shellQuote). This is safe because skill names - // are restricted to [A-Za-z0-9._-] by parseFrontmatter / the name - // validation regex, so $HOME expansion is the only variable substitution. let removedMirrorDir = false; if (paths.isOpenClaw && paths.mirrorDir) { - const removeMirror = runSsh(ctx, `rm -rf "${paths.mirrorDir}"`); + const removeMirror = await run(ctx, `rm -rf "${paths.mirrorDir}"`); removedMirrorDir = removeMirror !== null && removeMirror.status === 0; if (!removedMirrorDir) { messages.push(`Warning: failed to remove mirror directory ${paths.mirrorDir}`); } } - // 3. Clear sessions.json so the agent re-discovers the remaining skills. let clearedSessions = false; if (paths.isOpenClaw && paths.sessionFile) { - const clearResult = runSsh(ctx, `printf '{}' > ${shellQuote(paths.sessionFile)}`); + const clearResult = await run(ctx, `printf '{}' > ${shellQuote(paths.sessionFile)}`); clearedSessions = clearResult !== null && clearResult.status === 0; if (!clearedSessions) { messages.push("Warning: failed to clear sessions (agent may need manual restart)"); @@ -159,20 +123,17 @@ export function removeSkill( }; } -/** - * Verify the skill directory no longer exists on the sandbox. - * For OpenClaw sandboxes, both the upload dir and the mirror dir must be gone. - */ -export function verifyRemove( - ctx: SshContext, +/** Verify that both managed skill directories are absent. */ +export async function verifyRemove( + ctx: SandboxControlContext, paths: SkillPaths, - opts: { sshExecImpl?: typeof sshExec } = {}, -): boolean { + opts: { execImpl?: SandboxExecImpl } = {}, +): Promise { const checks = [`test ! -e ${shellQuote(paths.uploadDir)}`]; - if (paths.isOpenClaw && paths.mirrorDir) { - checks.push(`test ! -e "${paths.mirrorDir}"`); - } - const runSsh = opts.sshExecImpl ?? sshExec; - const result = runSsh(ctx, `${checks.join(" && ")} && echo GONE || echo EXISTS`); + if (paths.isOpenClaw && paths.mirrorDir) checks.push(`test ! -e "${paths.mirrorDir}"`); + const result = await (opts.execImpl ?? sandboxExec)( + ctx, + `${checks.join(" && ")} && echo GONE || echo EXISTS`, + ); return result !== null && result.stdout === "GONE"; }