From a6c2e0392b862ceee2c88859b861298b521c87a2 Mon Sep 17 00:00:00 2001 From: Dotta Date: Wed, 13 May 2026 11:26:24 -0500 Subject: [PATCH] fix(plugin): address kubernetes greptile follow-up Co-Authored-By: Paperclip --- .../sandbox-providers/kubernetes/README.md | 1 + .../kubernetes/src/manifest.ts | 7 +++ .../kubernetes/src/plugin.ts | 28 +++++++----- .../sandbox-providers/kubernetes/src/types.ts | 6 +++ .../kubernetes/test/unit/manifest.test.ts | 4 ++ .../kubernetes/test/unit/plugin.test.ts | 43 ++++++++++++++++++- .../kubernetes/test/unit/types.test.ts | 20 +++++++++ 7 files changed, 97 insertions(+), 12 deletions(-) diff --git a/packages/plugins/sandbox-providers/kubernetes/README.md b/packages/plugins/sandbox-providers/kubernetes/README.md index 1faeeaef..1e521033 100644 --- a/packages/plugins/sandbox-providers/kubernetes/README.md +++ b/packages/plugins/sandbox-providers/kubernetes/README.md @@ -63,6 +63,7 @@ Common optional fields: | `backend` | `"sandbox-cr"` | `sandbox-cr` (alpha, requires agent-sandbox controller) or `job` (stable, one-shot entrypoint). | | `adapterType` | `"claude_local"` | One of the supported adapter types (claude_local, codex_local, gemini_local, cursor_local, opencode_local, acpx_local, pi_local). Determines runtime image + env keys + egress allow-list. | | `namespacePrefix` | `"paperclip-"` | Prefix for the per-company tenant namespace. | +| `paperclipServerNamespace` | `"paperclip"` | Namespace where paperclip-server pods run. Generated egress policies use this so agent pods can call back to the server. | | `companySlug` | derived from companyId | Override the auto-derived company slug. | | `imageRegistry` | (none) | Override the default registry for agent runtime images. | | `imageAllowList` | `[]` | Glob patterns of allowed `target.imageOverride` values. Empty = no override permitted. | diff --git a/packages/plugins/sandbox-providers/kubernetes/src/manifest.ts b/packages/plugins/sandbox-providers/kubernetes/src/manifest.ts index d8c20179..6b52476b 100644 --- a/packages/plugins/sandbox-providers/kubernetes/src/manifest.ts +++ b/packages/plugins/sandbox-providers/kubernetes/src/manifest.ts @@ -43,6 +43,13 @@ const manifest: PaperclipPluginManifestV1 = { maxLength: 20, description: "Prefix for the per-company tenant namespace (default: paperclip-).", }, + paperclipServerNamespace: { + type: "string", + maxLength: 63, + pattern: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$", + description: + "Namespace where paperclip-server pods run. Used by generated egress policies so agent pods can call back to the server (default: paperclip).", + }, companySlug: { type: "string", maxLength: 43, diff --git a/packages/plugins/sandbox-providers/kubernetes/src/plugin.ts b/packages/plugins/sandbox-providers/kubernetes/src/plugin.ts index e14c2eb9..05bb763c 100644 --- a/packages/plugins/sandbox-providers/kubernetes/src/plugin.ts +++ b/packages/plugins/sandbox-providers/kubernetes/src/plugin.ts @@ -38,11 +38,6 @@ import { paperclipLabels, } from "./utils.js"; -// The namespace paperclip-server itself runs in. Used when building -// NetworkPolicy manifests so the tenant namespace allows inbound traffic -// from the server pod. -const PAPERCLIP_SERVER_NAMESPACE = "paperclip"; - // Name of the ServiceAccount created inside each tenant namespace by ensureTenant. const TENANT_SERVICE_ACCOUNT = "paperclip-tenant-sa"; @@ -80,7 +75,7 @@ export function extractAdapterEnvFromProcess( const missing: string[] = []; for (const k of envKeys) { const v = process.env[k]; - if (v) { + if (v !== undefined) { out[k] = v; } else { missing.push(k); @@ -94,6 +89,20 @@ export function extractAdapterEnvFromProcess( return out; } +function shellQuoteArg(arg: string): string { + return "'" + arg.replace(/'/g, "'\\''") + "'"; +} + +export function buildSandboxExecShellCommand( + params: Pick, +): string { + if (typeof params.command === "string" && params.command.trim().length > 0) { + return params.command; + } + + return params.args?.map(shellQuoteArg).join(" ") ?? ""; +} + function generateBootstrapToken(): string { // TODO: paperclip-server's actual callback auth scheme is separate and is // out of M4b scope. This per-run random token is stored in the per-run @@ -217,7 +226,7 @@ const plugin = definePlugin({ await ensureTenant(clients, { namespace, companyId: params.companyId, - paperclipServerNamespace: PAPERCLIP_SERVER_NAMESPACE, + paperclipServerNamespace: config.paperclipServerNamespace, serviceAccountAnnotations: config.serviceAccountAnnotations, egressMode: config.egressMode, egressAllowFqdns: [...adapterDefaults.allowFqdns, ...config.egressAllowFqdns], @@ -481,10 +490,7 @@ const plugin = definePlugin({ // Build the command to exec. If params.command is provided use it; // otherwise wrap in a login shell so profile scripts run. - const rawCommand = - typeof params.command === "string" && params.command.trim().length > 0 - ? params.command - : params.args?.join(" ") ?? ""; + const rawCommand = buildSandboxExecShellCommand(params); const execCommand = rawCommand.length > 0 ? ["/bin/sh", "-lc", rawCommand] diff --git a/packages/plugins/sandbox-providers/kubernetes/src/types.ts b/packages/plugins/sandbox-providers/kubernetes/src/types.ts index a16eacb7..26251b4e 100644 --- a/packages/plugins/sandbox-providers/kubernetes/src/types.ts +++ b/packages/plugins/sandbox-providers/kubernetes/src/types.ts @@ -29,6 +29,12 @@ export const kubernetesProviderConfigSchema = z kubeconfig: z.string().optional(), namespacePrefix: z.string().regex(/^[a-z0-9-]{1,20}$/).default("paperclip-"), + paperclipServerNamespace: z + .string() + .min(1) + .max(63) + .regex(/^[a-z0-9]([-a-z0-9]*[a-z0-9])?$/) + .default("paperclip"), companySlug: z.string().regex(/^[a-z0-9-]{1,43}$/).optional(), imageRegistry: z.string().url().optional(), diff --git a/packages/plugins/sandbox-providers/kubernetes/test/unit/manifest.test.ts b/packages/plugins/sandbox-providers/kubernetes/test/unit/manifest.test.ts index 85571a99..ae44654b 100644 --- a/packages/plugins/sandbox-providers/kubernetes/test/unit/manifest.test.ts +++ b/packages/plugins/sandbox-providers/kubernetes/test/unit/manifest.test.ts @@ -12,6 +12,10 @@ describe("manifest", () => { it("keeps namespace inputs within the Kubernetes DNS label length limit", () => { expect(configSchema.properties.namespacePrefix.maxLength).toBe(20); + expect(configSchema.properties.paperclipServerNamespace.maxLength).toBe(63); + expect(configSchema.properties.paperclipServerNamespace.pattern).toBe( + "^[a-z0-9]([-a-z0-9]*[a-z0-9])?$", + ); expect(configSchema.properties.companySlug.maxLength).toBe(43); }); diff --git a/packages/plugins/sandbox-providers/kubernetes/test/unit/plugin.test.ts b/packages/plugins/sandbox-providers/kubernetes/test/unit/plugin.test.ts index daa30ea9..777de3b0 100644 --- a/packages/plugins/sandbox-providers/kubernetes/test/unit/plugin.test.ts +++ b/packages/plugins/sandbox-providers/kubernetes/test/unit/plugin.test.ts @@ -1,5 +1,8 @@ import { describe, it, expect } from "vitest"; -import plugin, { extractAdapterEnvFromProcess } from "../../src/plugin.js"; +import plugin, { + buildSandboxExecShellCommand, + extractAdapterEnvFromProcess, +} from "../../src/plugin.js"; describe("plugin", () => { it("exports the kubernetes driver", () => { @@ -34,6 +37,7 @@ describe("plugin", () => { expect.objectContaining({ namespacePrefix: "paperclip-", egressMode: "standard", + paperclipServerNamespace: "paperclip", jobTtlSecondsAfterFinished: 900, podActivityDeadlineSec: 3600, adapterType: "claude_local", @@ -120,4 +124,41 @@ describe("plugin", () => { } } }); + + it("preserves intentionally empty adapter env values", () => { + const warnMessages: string[] = []; + const originalValue = process.env.PAPERCLIP_TEST_EMPTY_KEY; + process.env.PAPERCLIP_TEST_EMPTY_KEY = ""; + try { + const result = extractAdapterEnvFromProcess( + ["PAPERCLIP_TEST_EMPTY_KEY"], + (message) => warnMessages.push(message), + ); + expect(result).toEqual({ PAPERCLIP_TEST_EMPTY_KEY: "" }); + expect(warnMessages).toHaveLength(0); + } finally { + if (originalValue === undefined) { + delete process.env.PAPERCLIP_TEST_EMPTY_KEY; + } else { + process.env.PAPERCLIP_TEST_EMPTY_KEY = originalValue; + } + } + }); + + it("quotes args before passing them to /bin/sh -lc", () => { + expect( + buildSandboxExecShellCommand({ + args: ["git", "commit", "-m", "feat: add feature", "it's ready"], + }), + ).toBe("'git' 'commit' '-m' 'feat: add feature' 'it'\\''s ready'"); + }); + + it("uses command verbatim when command is provided", () => { + expect( + buildSandboxExecShellCommand({ + command: "pnpm test -- --runInBand", + args: ["ignored"], + }), + ).toBe("pnpm test -- --runInBand"); + }); }); diff --git a/packages/plugins/sandbox-providers/kubernetes/test/unit/types.test.ts b/packages/plugins/sandbox-providers/kubernetes/test/unit/types.test.ts index 9e85ccc6..4ac57443 100644 --- a/packages/plugins/sandbox-providers/kubernetes/test/unit/types.test.ts +++ b/packages/plugins/sandbox-providers/kubernetes/test/unit/types.test.ts @@ -6,6 +6,7 @@ describe("kubernetesProviderConfigSchema", () => { const parsed = parseKubernetesProviderConfig({ inCluster: true }); expect(parsed.inCluster).toBe(true); expect(parsed.namespacePrefix).toBe("paperclip-"); + expect(parsed.paperclipServerNamespace).toBe("paperclip"); expect(parsed.imageAllowList).toEqual([]); expect(parsed.egressMode).toBe("standard"); expect(parsed.jobTtlSecondsAfterFinished).toBe(900); @@ -40,6 +41,25 @@ describe("kubernetesProviderConfigSchema", () => { ).toThrow(); }); + it("accepts a custom paperclip-server namespace", () => { + const parsed = parseKubernetesProviderConfig({ + inCluster: true, + paperclipServerNamespace: "paperclip-prod", + }); + expect(parsed.paperclipServerNamespace).toBe("paperclip-prod"); + }); + + it("rejects invalid paperclip-server namespace values", () => { + for (const namespace of ["Paperclip", "paperclip_", "-paperclip", "paperclip-"]) { + expect(() => + parseKubernetesProviderConfig({ + inCluster: true, + paperclipServerNamespace: namespace, + }), + ).toThrow(); + } + }); + it("rejects whitespace-only kubeconfig", () => { expect(() => parseKubernetesProviderConfig({ inCluster: false, kubeconfig: " " }),