From 485274a271afb15415447861c740c822f703702a Mon Sep 17 00:00:00 2001 From: Aaron Erickson Date: Mon, 13 Jul 2026 14:14:12 -0700 Subject: [PATCH 1/6] refactor(openshell): remove skill SSH transport Signed-off-by: Aaron Erickson --- docs/reference/commands.mdx | 6 + scripts/checks/legacy-sandbox-transports.ts | 3 - src/lib/actions/sandbox/skill-install.test.ts | 105 +++++------ src/lib/actions/sandbox/skill-install.ts | 73 ++++---- .../adapters/openshell/grpc-gateway-config.ts | 14 +- .../openshell/sandbox-control-routing.test.ts | 59 +++++- .../openshell/sandbox-control-routing.ts | 44 ++++- src/lib/skill-install.test.ts | 81 ++++++--- src/lib/skill-install.ts | 65 ++++--- src/lib/skill-remote.test.ts | 171 +++++++++--------- src/lib/skill-remote.ts | 153 ++++++---------- 11 files changed, 430 insertions(+), 344 deletions(-) diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 0a43fe93d86..64f83ea6b6b 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`. diff --git a/scripts/checks/legacy-sandbox-transports.ts b/scripts/checks/legacy-sandbox-transports.ts index f41ca728cbd..c0378ae9c7d 100644 --- a/scripts/checks/legacy-sandbox-transports.ts +++ b/scripts/checks/legacy-sandbox-transports.ts @@ -38,8 +38,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], @@ -56,7 +54,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..e34e58b56af 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,17 +197,13 @@ 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 () => { 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); @@ -233,17 +219,16 @@ describe("sandbox skill action orchestration", () => { ), ); 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..e5720e808ef 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,30 +279,21 @@ 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 }; + const ctx = { control: selected.control, 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 + // advisory for install only: transient control-plane or remote shell + // 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 + // is destructive. Once OpenShell exposes a typed stat API or exec 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 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}`, @@ -305,7 +302,7 @@ export async function installSandboxSkill( 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 +312,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 +322,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 +334,6 @@ export async function installSandboxSkill( process.exit(1); } } finally { - tmpSshConfig.cleanup(); + selected.close(); } } 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/sandbox-control-routing.test.ts b/src/lib/adapters/openshell/sandbox-control-routing.test.ts index d4f87339854..393a4c17103 100644 --- a/src/lib/adapters/openshell/sandbox-control-routing.test.ts +++ b/src/lib/adapters/openshell/sandbox-control-routing.test.ts @@ -4,8 +4,12 @@ import { describe, expect, it, vi } from "vitest"; import type { GrpcOpenShellSandboxControl } from "./grpc-sandbox-control"; +import { OpenShellGrpcEdgeTunnelRequiredError } from "./grpc-gateway-config"; import type { OpenShellSandboxControl, 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) { @@ -109,3 +113,56 @@ 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.deps.cli); + 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.deps.cli, + transport: "cli-edge-tunnel", + close: expect.any(Function), + }); + selected.close(); + expect(test.close).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 f170bb100a6..453323b4b37 100644 --- a/src/lib/adapters/openshell/sandbox-control-routing.ts +++ b/src/lib/adapters/openshell/sandbox-control-routing.ts @@ -3,7 +3,10 @@ import { log } from "../../cli/logger"; import { OPENSHELL_OPERATION_TIMEOUT_MS } from "./timeouts"; -import { createGrpcOpenShellSandboxControlForGateway } from "./grpc-gateway-config"; +import { + createGrpcOpenShellSandboxControlForGateway, + OpenShellGrpcEdgeTunnelRequiredError, +} from "./grpc-gateway-config"; import { createCliOpenShellSandboxControl, type OpenShellSandboxControl, @@ -24,6 +27,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.cli, + transport: "cli-edge-tunnel", + close: () => {}, + }; + } +} + /** * Prefer direct gRPC for a read-only sandbox exec and retry through the * OpenShell CLI when gateway resolution or transport fails. This retry policy 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..9792fe8fd65 100644 --- a/src/lib/skill-remote.test.ts +++ b/src/lib/skill-remote.test.ts @@ -1,76 +1,99 @@ // 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, + sandboxExec, + type SandboxControlContext, + 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" }; - const paths = resolveSkillPaths(null, "test-skill"); + 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 +109,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 +132,17 @@ 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(); - }); - - 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(); +describe("checkExisting", () => { + it("returns null when sandbox execution is unavailable", async () => { + await expect( + checkExisting(context(), resolveSkillPaths(null, "test-skill")), + ).resolves.toBeNull(); }); - 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..7ebaf5c296a 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,22 @@ 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. */ -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 +77,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 +121,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"; } From c50f9740b48477e25dac6fa7bfeab17719ae7583 Mon Sep 17 00:00:00 2001 From: Aaron Erickson Date: Mon, 13 Jul 2026 21:08:51 -0700 Subject: [PATCH 2/6] test(openshell): cover retried skill removal Signed-off-by: Aaron Erickson --- src/lib/skill-remote.test.ts | 54 +++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/src/lib/skill-remote.test.ts b/src/lib/skill-remote.test.ts index 9792fe8fd65..fb160a7082d 100644 --- a/src/lib/skill-remote.test.ts +++ b/src/lib/skill-remote.test.ts @@ -8,8 +8,8 @@ import { validateSkillName } from "./skill-name"; import { checkExisting, removeSkill, - sandboxExec, type SandboxControlContext, + sandboxExec, verifyRemove, } from "./skill-remote"; @@ -90,6 +90,58 @@ describe("removeSkill", () => { }); }); + 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 existingPaths = new Set([paths.uploadDir, mirrorDir, unrelatedPath]); + const commands: string[] = []; + let mirrorAttempts = 0; + const execImpl = vi.fn(async (_ctx: SandboxControlContext, command: string) => { + commands.push(command); + if (command === `rm -rf '${paths.uploadDir}'`) { + existingPaths.delete(paths.uploadDir); + return { status: 0, stdout: "", stderr: "" }; + } + if (command === `rm -rf "${mirrorDir}"`) { + mirrorAttempts += 1; + if (mirrorAttempts === 1) return { status: 1, stdout: "", stderr: "failed" }; + existingPaths.delete(mirrorDir); + return { status: 0, stdout: "", stderr: "" }; + } + if (command.startsWith("printf '{}' >")) { + return { status: 0, stdout: "", stderr: "" }; + } + if (command.startsWith("test ! -e")) { + return { + status: 0, + stdout: + existingPaths.has(paths.uploadDir) || existingPaths.has(mirrorDir) ? "EXISTS" : "GONE", + stderr: "", + }; + } + return { status: 1, stdout: "", stderr: `unexpected command: ${command}` }; + }); + + 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(existingPaths.has(paths.uploadDir)).toBe(false); + expect(existingPaths.has(mirrorDir)).toBe(false); + expect(existingPaths.has(unrelatedPath)).toBe(true); + expect(commands.every((command) => !command.includes(unrelatedPath))).toBe(true); + }); + it("removes OpenClaw upload and mirror dirs, then clears sessions", async () => { const commands: string[] = []; const result = await removeSkill(context(), resolveSkillPaths(null, "test-skill"), { From b5fb4ae6908e98debc122e04241b31befd662b60 Mon Sep 17 00:00:00 2001 From: Aaron Erickson Date: Mon, 13 Jul 2026 23:24:24 -0700 Subject: [PATCH 3/6] test(openshell): script skill removal retry Signed-off-by: Aaron Erickson --- src/lib/skill-remote.test.ts | 62 +++++++++++++++++++----------------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/src/lib/skill-remote.test.ts b/src/lib/skill-remote.test.ts index fb160a7082d..b97cbd6a43c 100644 --- a/src/lib/skill-remote.test.ts +++ b/src/lib/skill-remote.test.ts @@ -94,33 +94,38 @@ describe("removeSkill", () => { const paths = resolveSkillPaths(null, "test-skill"); const mirrorDir = paths.mirrorDir!; const unrelatedPath = "/sandbox/.openclaw/skills/other-skill"; - const existingPaths = new Set([paths.uploadDir, mirrorDir, unrelatedPath]); - const commands: string[] = []; - let mirrorAttempts = 0; + 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) => { - commands.push(command); - if (command === `rm -rf '${paths.uploadDir}'`) { - existingPaths.delete(paths.uploadDir); - return { status: 0, stdout: "", stderr: "" }; - } - if (command === `rm -rf "${mirrorDir}"`) { - mirrorAttempts += 1; - if (mirrorAttempts === 1) return { status: 1, stdout: "", stderr: "failed" }; - existingPaths.delete(mirrorDir); - return { status: 0, stdout: "", stderr: "" }; - } - if (command.startsWith("printf '{}' >")) { - return { status: 0, stdout: "", stderr: "" }; - } - if (command.startsWith("test ! -e")) { - return { - status: 0, - stdout: - existingPaths.has(paths.uploadDir) || existingPaths.has(mirrorDir) ? "EXISTS" : "GONE", - stderr: "", - }; - } - return { status: 1, stdout: "", stderr: `unexpected command: ${command}` }; + 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({ @@ -136,10 +141,7 @@ describe("removeSkill", () => { }); await expect(verifyRemove(context(), paths, { execImpl })).resolves.toBe(true); - expect(existingPaths.has(paths.uploadDir)).toBe(false); - expect(existingPaths.has(mirrorDir)).toBe(false); - expect(existingPaths.has(unrelatedPath)).toBe(true); - expect(commands.every((command) => !command.includes(unrelatedPath))).toBe(true); + expect(scriptedResults).toEqual([]); }); it("removes OpenClaw upload and mirror dirs, then clears sessions", async () => { From cbfc62b026ec9afa2a3f91f3090e75e3619fd03a Mon Sep 17 00:00:00 2001 From: Aaron Erickson Date: Mon, 13 Jul 2026 23:34:12 -0700 Subject: [PATCH 4/6] fix(openshell): fail closed on skill probe errors Signed-off-by: Aaron Erickson --- src/lib/actions/sandbox/skill-install.test.ts | 26 +++++++++++++++---- src/lib/actions/sandbox/skill-install.ts | 16 +++++------- src/lib/skill-remote.test.ts | 10 ++++++- src/lib/skill-remote.ts | 4 ++- 4 files changed, 40 insertions(+), 16 deletions(-) diff --git a/src/lib/actions/sandbox/skill-install.test.ts b/src/lib/actions/sandbox/skill-install.test.ts index e34e58b56af..cde0d6302ae 100644 --- a/src/lib/actions/sandbox/skill-install.test.ts +++ b/src/lib/actions/sandbox/skill-install.test.ts @@ -201,11 +201,11 @@ describe("sandbox skill action orchestration", () => { 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(); 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 }); @@ -214,10 +214,26 @@ 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( { control, sandboxName: "alpha" }, skillDir, diff --git a/src/lib/actions/sandbox/skill-install.ts b/src/lib/actions/sandbox/skill-install.ts index e5720e808ef..6b81d099c20 100644 --- a/src/lib/actions/sandbox/skill-install.ts +++ b/src/lib/actions/sandbox/skill-install.ts @@ -285,19 +285,17 @@ export async function installSandboxSkill( try { const ctx = { control: selected.control, sandboxName }; - // 5. Check if skill already exists (update vs fresh install). This probe is - // advisory for install only: transient control-plane or remote shell - // 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 exec probe - // failures are reliably distinguishable from absent dirs across supported - // versions, remove this fallback and fail before upload. + // 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; diff --git a/src/lib/skill-remote.test.ts b/src/lib/skill-remote.test.ts index b97cbd6a43c..8a45b2a9f42 100644 --- a/src/lib/skill-remote.test.ts +++ b/src/lib/skill-remote.test.ts @@ -187,12 +187,20 @@ describe("verifyRemove", () => { }); describe("checkExisting", () => { - it("returns null when sandbox execution is unavailable", async () => { + it("maps a selected-control execution failure to an inconclusive result", async () => { await expect( checkExisting(context(), resolveSkillPaths(null, "test-skill")), ).resolves.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 directories so removal can clean partial uploads", async () => { const commands: string[] = []; const exists = await checkExisting(context(), resolveSkillPaths(null, "test-skill"), { diff --git a/src/lib/skill-remote.ts b/src/lib/skill-remote.ts index 7ebaf5c296a..73ddbd16f88 100644 --- a/src/lib/skill-remote.ts +++ b/src/lib/skill-remote.ts @@ -50,7 +50,9 @@ export async function sandboxExec( /** * 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. + * 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 async function checkExisting( ctx: SandboxControlContext, From b17c47282358ceff0df472eafac1259fbe5aec56 Mon Sep 17 00:00:00 2001 From: Aaron Erickson Date: Tue, 14 Jul 2026 04:09:27 -0700 Subject: [PATCH 5/6] fix(openshell): scope mutation fallback to gateway Signed-off-by: Aaron Erickson --- src/lib/adapters/openshell/sandbox-control-routing.test.ts | 7 +++++-- src/lib/adapters/openshell/sandbox-control-routing.ts | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/lib/adapters/openshell/sandbox-control-routing.test.ts b/src/lib/adapters/openshell/sandbox-control-routing.test.ts index c87a263beb0..f542089fe8c 100644 --- a/src/lib/adapters/openshell/sandbox-control-routing.test.ts +++ b/src/lib/adapters/openshell/sandbox-control-routing.test.ts @@ -33,6 +33,7 @@ function dependencies(grpcResult: SandboxExecResult | Error, cliResult?: Sandbox const debug = vi.fn(); return { close, + cli, grpcExec, cliExec, createCli, @@ -244,7 +245,8 @@ describe("mutating OpenShell sandbox control routing", () => { const selected = selectOpenShellSandboxControlForMutation("nemoclaw", test.deps); expect(selected).toMatchObject({ control: expect.any(Object), transport: "grpc" }); - expect(selected.control).not.toBe(test.deps.cli); + expect(selected.control).not.toBe(test.cli); + expect(test.createCli).not.toHaveBeenCalled(); selected.close(); expect(test.close).toHaveBeenCalledOnce(); }); @@ -258,10 +260,11 @@ describe("mutating OpenShell sandbox control routing", () => { const selected = selectOpenShellSandboxControlForMutation("edge", test.deps); expect(selected).toEqual({ - control: test.deps.cli, + control: test.cli, transport: "cli-edge-tunnel", close: expect.any(Function), }); + expect(test.createCli).toHaveBeenCalledWith("edge"); selected.close(); expect(test.close).not.toHaveBeenCalled(); }); diff --git a/src/lib/adapters/openshell/sandbox-control-routing.ts b/src/lib/adapters/openshell/sandbox-control-routing.ts index 1e5edfe1602..413b0f85f3c 100644 --- a/src/lib/adapters/openshell/sandbox-control-routing.ts +++ b/src/lib/adapters/openshell/sandbox-control-routing.ts @@ -65,7 +65,7 @@ export function selectOpenShellSandboxControlForMutation( } catch (error) { if (!(error instanceof OpenShellGrpcEdgeTunnelRequiredError)) throw error; return { - control: dependencies.cli, + control: dependencies.createCli(gatewayName), transport: "cli-edge-tunnel", close: () => {}, }; From 346a124498bb537a373c2c758b92554c8b793794 Mon Sep 17 00:00:00 2001 From: Aaron Erickson Date: Tue, 14 Jul 2026 04:18:13 -0700 Subject: [PATCH 6/6] test(openshell): guard mutation fallback endpoint Signed-off-by: Aaron Erickson --- .../openshell/sandbox-control-routing.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/lib/adapters/openshell/sandbox-control-routing.test.ts b/src/lib/adapters/openshell/sandbox-control-routing.test.ts index 71a765976ee..e315d2ab41b 100644 --- a/src/lib/adapters/openshell/sandbox-control-routing.test.ts +++ b/src/lib/adapters/openshell/sandbox-control-routing.test.ts @@ -304,6 +304,22 @@ describe("mutating OpenShell sandbox control routing", () => { 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");