From 2dd1fbe8f34c587626f8a23d0cb05dc3312fb213 Mon Sep 17 00:00:00 2001 From: Raghuram Banda Date: Tue, 7 Jul 2026 22:52:13 -0400 Subject: [PATCH] fix: resolve all review issues in openshell_direct adapter - Replace require("fs") with ESM import (P0: ReferenceError in ESM) - Remove redundant require() block in execute(); use getSandbox() helper (P0) - Fix phase comparisons to use proto enum names SANDBOX_PHASE_READY/ERROR (P0) - Use Map instead of singleton to support multiple endpoints (P1) - Add null guard for sandboxId before execInSandbox call (P1) - Remove sh -c shell wrapper; pass command array directly to ExecSandbox (P1) - Map resources to correct proto fields (SandboxTemplate, ResourceRequirements) - Export getSandbox() helper from openshell-client - Remove unused cpu/memory string config (resources via proto Struct) --- .../adapters/openshell-direct/src/index.ts | 57 +++++------------ .../openshell-direct/src/openshell-client.ts | 64 ++++++++++++------- 2 files changed, 59 insertions(+), 62 deletions(-) diff --git a/packages/adapters/openshell-direct/src/index.ts b/packages/adapters/openshell-direct/src/index.ts index 93c9ba36c0..8427bd2380 100644 --- a/packages/adapters/openshell-direct/src/index.ts +++ b/packages/adapters/openshell-direct/src/index.ts @@ -16,6 +16,7 @@ import { deleteSandbox, healthCheck, listSandboxes, + getSandbox, } from "./openshell-client.js"; function asString(v: unknown, fallback: string): string { @@ -32,7 +33,6 @@ function parseObject(v: unknown): Record { return {}; } -// Paperclip ServerAdapterModule interface interface AdapterExecutionContext { runId: string; agent: { id: string; name: string; companyId: string }; @@ -68,13 +68,19 @@ interface ServerAdapterModule { agentConfigurationDoc: string; } +/** + * Shell-escape a string for safe inclusion in a single-quoted shell argument. + * Replaces each ' with '\'' (end quote, escaped quote, reopen quote). + */ +function shellEscape(s: string): string { + return "'" + s.replace(/'/g, "'\\''") + "'"; +} + async function execute(ctx: AdapterExecutionContext): Promise { const { config, runId, agent, context, onLog } = ctx; const endpoint = asString(config.gatewayEndpoint, "openshell.openshell.svc:8080"); const image = asString(config.sandboxImage, "ghcr.io/nvidia/openshell-community/sandboxes/base:latest"); - const cpu = asString(config.cpu, "2"); - const memory = asString(config.memory, "4Gi"); const gpu = Boolean(config.gpu); const reuseStrategy = asString(config.reuseStrategy, "per-run"); const timeoutSecs = asNumber(config.timeoutSecs, 600); @@ -95,12 +101,11 @@ async function execute(ctx: AdapterExecutionContext): Promise s.name === sandboxName && s.phase.toLowerCase().includes("running"))) { + if (existing.some((s) => s.name === sandboxName && s.phase === "SANDBOX_PHASE_READY")) { await onLog("stdout", `[openshell] Reusing existing sandbox: ${sandboxName}\n`); needsCreate = false; } @@ -109,14 +114,11 @@ async function execute(ctx: AdapterExecutionContext): Promise((resolve, reject) => { - const { getClient: _gc, ...mod } = require("./openshell-client.js"); - // Use the client directly - const grpc = require("@grpc/grpc-js"); - const protoLoader = require("@grpc/proto-loader"); - const protoPath = "/paperclip/adapters/openshell-direct/proto/openshell.proto"; - const pkgDef = protoLoader.loadSync(protoPath, { - keepCase: false, longs: String, enums: String, defaults: true, oneofs: true, - includeDirs: ["/paperclip/adapters/openshell-direct/proto"], - }); - const proto = grpc.loadPackageDefinition(pkgDef); - const c = new proto.openshell.v1.OpenShell(endpoint, grpc.credentials.createInsecure()); - c.GetSandbox({ name: sandboxName }, { deadline: Date.now() + 10000 }, (err: any, res: any) => { - if (err) return reject(err); - resolve(res?.sandbox); - }); - }); - const sandboxId = sbInfo?.metadata?.id; + const sbInfo = await getSandbox(endpoint, sandboxName); + const sandboxId = sbInfo.id; + if (!sandboxId) { + throw new Error(`Sandbox ${sandboxName} has no ID -- cannot exec`); + } await onLog("stdout", `[openshell] Sandbox ID: ${sandboxId}\n`); - const cmd = [ - "sh", "-c", - `${agentCommand} "${wakePrompt.replace(/"/g, '\\"')}" 2>&1 || echo "[openshell] Agent exited with code $?"`, - ]; + const cmd = [agentCommand, "--print", "-", "--prompt", wakePrompt]; const result = await execInSandbox(endpoint, sandboxId, cmd, { timeoutSecs, }); - // Stream output if (result.stdout) { await onLog("stdout", result.stdout); } @@ -182,7 +164,6 @@ async function execute(ctx: AdapterExecutionContext): Promise(); function getClient(endpoint: string): any { - if (_client) return _client; + const existing = _clients.get(endpoint); + if (existing) return existing; const packageDef = protoLoader.loadSync(PROTO_PATH, { keepCase: false, @@ -34,17 +36,19 @@ function getClient(endpoint: string): any { }); const proto = grpc.loadPackageDefinition(packageDef) as any; - _client = new proto.openshell.v1.OpenShell( + const client = new proto.openshell.v1.OpenShell( endpoint, grpc.credentials.createInsecure() ); - return _client; + _clients.set(endpoint, client); + return client; } export interface SandboxInfo { name: string; phase: string; + id?: string; } export async function healthCheck(endpoint: string): Promise { @@ -56,6 +60,21 @@ export async function healthCheck(endpoint: string): Promise { }); } +export async function getSandbox(endpoint: string, name: string): Promise { + const client = getClient(endpoint); + return new Promise((resolve, reject) => { + client.GetSandbox({ name }, { deadline: Date.now() + 10000 }, (err: any, res: any) => { + if (err) return reject(new Error(`GetSandbox failed: ${err.message}`)); + const sb = res?.sandbox; + resolve({ + name: sb?.metadata?.name || name, + phase: sb?.status?.phase || "unknown", + id: sb?.metadata?.id, + }); + }); + }); +} + export async function createSandbox( endpoint: string, opts: { @@ -70,17 +89,16 @@ export async function createSandbox( ): Promise { const client = getClient(endpoint); - const spec: any = {}; - if (opts.image) { - spec.template = { image: opts.image }; + const template: any = {}; + if (opts.image) template.image = opts.image; + if (opts.environment) template.environment = opts.environment; + if (opts.labels) template.labels = opts.labels; + + const spec: any = { template }; + + if (opts.gpu) { + spec.resourceRequirements = { gpu: { count: 1 } }; } - if (opts.cpu || opts.memory) { - spec.resources = {}; - if (opts.cpu) spec.resources.cpu = opts.cpu; - if (opts.memory) spec.resources.memory = opts.memory; - } - if (opts.gpu) spec.gpu = true; - if (opts.environment) spec.environment = opts.environment; const request: any = { spec }; if (opts.name) request.name = opts.name; @@ -93,6 +111,7 @@ export async function createSandbox( resolve({ name: sb?.metadata?.name || opts.name || "unknown", phase: sb?.status?.phase || "unknown", + id: sb?.metadata?.id, }); }); }); @@ -114,9 +133,9 @@ export async function waitForSandboxReady( }); }); - if (phase === "RUNNING" || phase === "Running" || phase === "running") return; - if (phase === "FAILED" || phase === "Failed") { - throw new Error(`Sandbox ${name} failed to start`); + if (phase === "SANDBOX_PHASE_READY") return; + if (phase === "SANDBOX_PHASE_ERROR") { + throw new Error(`Sandbox ${name} failed to start (phase: ${phase})`); } await new Promise((r) => setTimeout(r, 2000)); @@ -192,6 +211,7 @@ export async function listSandboxes(endpoint: string): Promise { const items = (res?.sandboxes || []).map((sb: any) => ({ name: sb?.metadata?.name || "unknown", phase: sb?.status?.phase || "unknown", + id: sb?.metadata?.id, })); resolve(items); });