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<string, client> 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)
This commit is contained in:
parent
2c26c98ac9
commit
2dd1fbe8f3
|
|
@ -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<string, unknown> {
|
|||
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<AdapterExecutionResult> {
|
||||
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<AdapterExecutionRe
|
|||
await onLog("stdout", `[openshell] Agent command: ${agentCommand}\n\n`);
|
||||
|
||||
try {
|
||||
// Check if sandbox already exists (reuse case)
|
||||
let needsCreate = true;
|
||||
if (reuseStrategy === "per-agent") {
|
||||
try {
|
||||
const existing = await listSandboxes(endpoint);
|
||||
if (existing.some((s) => 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<AdapterExecutionRe
|
|||
}
|
||||
}
|
||||
|
||||
// Create sandbox
|
||||
if (needsCreate) {
|
||||
await onLog("stdout", `[openshell] Creating sandbox...\n`);
|
||||
const sb = await createSandbox(endpoint, {
|
||||
name: sandboxName,
|
||||
image,
|
||||
cpu,
|
||||
memory,
|
||||
gpu,
|
||||
environment: {
|
||||
PAPERCLIP_RUN_ID: runId,
|
||||
|
|
@ -130,51 +132,31 @@ async function execute(ctx: AdapterExecutionContext): Promise<AdapterExecutionRe
|
|||
});
|
||||
await onLog("stdout", `[openshell] Sandbox created: ${sb.name} (phase: ${sb.phase})\n`);
|
||||
|
||||
// Wait for sandbox to be ready
|
||||
await onLog("stdout", `[openshell] Waiting for sandbox to be ready...\n`);
|
||||
await waitForSandboxReady(endpoint, sandboxName, 120000);
|
||||
await onLog("stdout", `[openshell] Sandbox is ready.\n\n`);
|
||||
}
|
||||
|
||||
// Execute agent command in sandbox
|
||||
await onLog("stdout", `[openshell] Executing: ${agentCommand} --prompt "..."\n`);
|
||||
await onLog("stdout", `[openshell] Executing: ${agentCommand}\n`);
|
||||
await onLog("stdout", `[openshell] Timeout: ${timeoutSecs}s\n\n`);
|
||||
|
||||
if (ctx.onSpawn) {
|
||||
await ctx.onSpawn({ pid: 0, processGroupId: null, startedAt: new Date().toISOString() });
|
||||
}
|
||||
|
||||
// Get sandbox ID for exec
|
||||
const sbInfo = await new Promise<any>((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<AdapterExecutionRe
|
|||
await onLog("stderr", result.stderr);
|
||||
}
|
||||
|
||||
// Cleanup per-run sandboxes
|
||||
if (reuseStrategy === "per-run") {
|
||||
await onLog("stdout", `\n[openshell] Cleaning up sandbox...\n`);
|
||||
try {
|
||||
|
|
@ -202,7 +183,6 @@ async function execute(ctx: AdapterExecutionContext): Promise<AdapterExecutionRe
|
|||
} catch (err: any) {
|
||||
await onLog("stderr", `[openshell] Error: ${err.message}\n`);
|
||||
|
||||
// Try cleanup on error
|
||||
if (reuseStrategy === "per-run") {
|
||||
try { await deleteSandbox(endpoint, sandboxName); } catch {}
|
||||
}
|
||||
|
|
@ -259,15 +239,12 @@ Required fields:
|
|||
Optional fields:
|
||||
- sandboxImage (string): Container image for sandboxes (default: base image)
|
||||
- agentCommand (string): Agent CLI to run inside sandbox (default: claude)
|
||||
- cpu (string): CPU request/limit (default: 2)
|
||||
- memory (string): Memory request/limit (default: 4Gi)
|
||||
- gpu (boolean): Request GPU (default: false)
|
||||
- reuseStrategy (string): "per-run" (ephemeral) or "per-agent" (reuse) (default: per-run)
|
||||
- timeoutSecs (number): Command execution timeout (default: 600)
|
||||
`,
|
||||
};
|
||||
|
||||
// Export for Paperclip external adapter loading
|
||||
export function createServerAdapter() {
|
||||
return openshellDirectAdapter;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,7 +1,6 @@
|
|||
/**
|
||||
* OpenShell gRPC client -- calls the OpenShell gateway directly
|
||||
* without ShoreGuard as a middleman.
|
||||
*
|
||||
* OpenShell gRPC client -- calls the OpenShell gateway directly.
|
||||
*
|
||||
* Uses dynamic proto loading via @grpc/proto-loader to avoid
|
||||
* needing a proto compilation step.
|
||||
*/
|
||||
|
|
@ -9,20 +8,23 @@ import * as grpc from "@grpc/grpc-js";
|
|||
import * as protoLoader from "@grpc/proto-loader";
|
||||
import { resolve, dirname } from "path";
|
||||
import { fileURLToPath } from "url";
|
||||
import { accessSync } from "node:fs";
|
||||
|
||||
const __dirname = dirname(fileURLToPath(import.meta.url));
|
||||
const PROTO_PATH_RELATIVE = resolve(__dirname, "../proto/openshell.proto");
|
||||
const PROTO_PATH_ABSOLUTE = "/paperclip/adapters/openshell-direct/proto/openshell.proto";
|
||||
const PROTO_PATH = existsSync(PROTO_PATH_RELATIVE) ? PROTO_PATH_RELATIVE : PROTO_PATH_ABSOLUTE;
|
||||
|
||||
function existsSync(p: string): boolean {
|
||||
try { require("fs").accessSync(p); return true; } catch { return false; }
|
||||
try { accessSync(p); return true; } catch { return false; }
|
||||
}
|
||||
|
||||
let _client: any = null;
|
||||
const PROTO_PATH = existsSync(PROTO_PATH_RELATIVE) ? PROTO_PATH_RELATIVE : PROTO_PATH_ABSOLUTE;
|
||||
|
||||
const _clients = new Map<string, any>();
|
||||
|
||||
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<boolean> {
|
||||
|
|
@ -56,6 +60,21 @@ export async function healthCheck(endpoint: string): Promise<boolean> {
|
|||
});
|
||||
}
|
||||
|
||||
export async function getSandbox(endpoint: string, name: string): Promise<SandboxInfo> {
|
||||
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<SandboxInfo> {
|
||||
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<SandboxInfo[]> {
|
|||
const items = (res?.sandboxes || []).map((sb: any) => ({
|
||||
name: sb?.metadata?.name || "unknown",
|
||||
phase: sb?.status?.phase || "unknown",
|
||||
id: sb?.metadata?.id,
|
||||
}));
|
||||
resolve(items);
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue