diff --git a/apps/api/src/services/persistent-agent-pool-service.ts b/apps/api/src/services/persistent-agent-pool-service.ts index 855c84f5..fddb602b 100644 --- a/apps/api/src/services/persistent-agent-pool-service.ts +++ b/apps/api/src/services/persistent-agent-pool-service.ts @@ -19,6 +19,7 @@ import type { ContainerHandle, ContainerSpec, ExecSession } from "@optio/shared" import { PersistentAgentPodLifecycle, parseIntEnv, type RepoImageConfig } from "@optio/shared"; import { logger } from "../logger.js"; import { resolveImage } from "./repo-pool-service.js"; +import { buildEnvExports } from "../utils/pod-env.js"; const POD_PROVISION_TIMEOUT_MS = parseIntEnv("OPTIO_PERSISTENT_AGENT_POD_PROVISION_MS", 120_000); @@ -242,17 +243,11 @@ export async function execTurnInPod( name: pod.podName, }; - const envJson = JSON.stringify({ ...env, OPTIO_PERSISTENT_AGENT_TURN_ID: turnId }); - const envB64 = Buffer.from(envJson).toString("base64"); - const script = [ "set -e", - `eval $(echo '${envB64}' | base64 -d | python3 -c "`, - `import json, sys, shlex`, - `env = json.load(sys.stdin)`, - `for k, v in env.items():`, - ` print(f'export {k}={shlex.quote(v)}')`, - `")`, + // Env values (including the prompt) are embedded as inert single-quoted + // exports — see buildEnvExports. + ...buildEnvExports({ ...env, OPTIO_PERSISTENT_AGENT_TURN_ID: turnId }), `for i in $(seq 1 120); do [ -f /workspace/.ready ] && break; sleep 1; done`, `[ -f /workspace/.ready ] || { echo "[optio] ERROR: pod not ready after 120s"; exit 1; }`, `mkdir -p /workspace/turns/${turnId}`, diff --git a/apps/api/src/services/repo-pool-service.test.ts b/apps/api/src/services/repo-pool-service.test.ts index e4b39072..1cea5897 100644 --- a/apps/api/src/services/repo-pool-service.test.ts +++ b/apps/api/src/services/repo-pool-service.test.ts @@ -1,4 +1,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { mkdtempSync, readFileSync, readdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; // ── Mocks ─────────────────────────────────────────────────────────── @@ -102,6 +105,7 @@ import { deleteNetworkPolicy, killOrphanedAgentInPod, parseJsonEnv, + execTaskInRepoPod, } from "./repo-pool-service.js"; // ── resolveImage ──────────────────────────────────────────────────── @@ -1170,3 +1174,72 @@ describe("getOrCreateRepoPod — service account propagation", () => { expect(spec.serviceAccountName).toBeUndefined(); }); }); + +// ── execTaskInRepoPod — env injection safety ──────────────────────── + +describe("execTaskInRepoPod", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("embeds a hostile prompt inertly — nothing executes before the agent command", async () => { + // Regression for the Phase 4F symptoms ("n: command not found", + // "optio/task-*: No such file or directory"): prompts with markdown + // backticks, $HOME, wildcards, and literal newlines must reach the pod + // byte-for-byte without the shell interpreting any of it. + const hostilePrompt = [ + "Fix the `pr_opened` handling.", + "", + "1. Inspect $HOME and run `git status`.", + "2. Ignore branches named optio/task-* entirely.", + "3. Don't rewrite 'quoted' text.", + "```bash", + "touch injected-from-fenced-block", + "```", + "$(touch injected-from-substitution)", + ].join("\n"); + + mockRuntimeExec.mockResolvedValueOnce({ stdin: { write: vi.fn() } }); + const pod = { + id: "pod-1", + repoUrl: "https://github.com/org/repo", + podName: "optio-repo-org-repo-0", + podId: "k8s-pod-1", + state: "ready", + }; + + await execTaskInRepoPod(pod as any, "task-1", [`echo "[optio] agent"`], { + OPTIO_PROMPT: hostilePrompt, + OPTIO_REPO_BRANCH: "main", + }); + + const execCall = mockRuntimeExec.mock.calls[0]; + expect(execCall[1][0]).toBe("bash"); + expect(execCall[1][1]).toBe("-c"); + const script: string = execCall[1][2]; + expect(script).not.toContain("eval $("); + + // Run the script prefix (set -e + env exports) through real bash and + // verify the prompt round-trips exactly with no side effects. + const lines = script.split("\n"); + const readyIdx = lines.findIndex((l) => l.includes("Waiting for repo to be ready")); + expect(readyIdx).toBeGreaterThan(0); + const prefix = lines.slice(0, readyIdx).join("\n"); + + const { execFileSync } = + await vi.importActual("node:child_process"); + const dir = mkdtempSync(join(tmpdir(), "repo-pool-env-")); + try { + execFileSync("bash", ["-c", `${prefix}\nprintf '%s' "$OPTIO_PROMPT" > prompt-out`], { + cwd: dir, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + expect(readFileSync(join(dir, "prompt-out"), "utf8")).toBe(hostilePrompt); + // No canary files — prompt contents never executed + expect(readdirSync(dir)).toEqual(["prompt-out"]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/apps/api/src/services/repo-pool-service.ts b/apps/api/src/services/repo-pool-service.ts index 30e07f63..a55e3527 100644 --- a/apps/api/src/services/repo-pool-service.ts +++ b/apps/api/src/services/repo-pool-service.ts @@ -29,6 +29,7 @@ import { } from "./envoy-sidecar.js"; import { parseIntEnv } from "@optio/shared"; import { withSpan } from "../telemetry/spans.js"; +import { buildEnvExports } from "../utils/pod-env.js"; const IDLE_TIMEOUT_MS = parseIntEnv("OPTIO_REPO_POD_IDLE_MS", 600000); // 10 min default const REPO_INIT_TIMEOUT_MS = parseIntEnv("OPTIO_REPO_INIT_TIMEOUT_MS", 120000); // 2 min default @@ -902,13 +903,13 @@ export async function execTaskInRepoPod( .set({ worktreeState: "active", lastPodId: pod.id, updatedAt: new Date() }) .where(eq(tasks.id, taskId)); - // Build the exec command - const envJson = JSON.stringify({ + // Build the exec command. Env values (including the task prompt) are + // embedded as inert single-quoted exports — see buildEnvExports. + const envExports = buildEnvExports({ ...env, OPTIO_TASK_ID: taskId, REPO_INIT_TIMEOUT_SECS: String(Math.ceil(REPO_INIT_TIMEOUT_MS / 1000)), }); - const envB64 = Buffer.from(envJson).toString("base64"); const runToken = randomUUID(); // Build worktree setup commands based on whether we're resetting or creating fresh @@ -964,12 +965,7 @@ export async function execTaskInRepoPod( const script = [ "set -e", - `eval $(echo '${envB64}' | base64 -d | python3 -c "`, - `import json, sys, shlex`, - `env = json.load(sys.stdin)`, - `for k, v in env.items():`, - ` print(f'export {k}={shlex.quote(v)}')`, - `")`, + ...envExports, `echo "[optio] Waiting for repo to be ready..."`, `for i in $(seq 1 \${REPO_INIT_TIMEOUT_SECS}); do [ -f /workspace/.ready ] && break; sleep 1; done`, `[ -f /workspace/.ready ] || { echo "[optio] ERROR: repo not ready after \${REPO_INIT_TIMEOUT_SECS}s (increase OPTIO_REPO_INIT_TIMEOUT_MS to extend)"; exit 1; }`, diff --git a/apps/api/src/services/task-service.test.ts b/apps/api/src/services/task-service.test.ts index 5ce39635..36fb8d6e 100644 --- a/apps/api/src/services/task-service.test.ts +++ b/apps/api/src/services/task-service.test.ts @@ -35,6 +35,9 @@ vi.mock("../db/schema.js", () => ({ })); vi.mock("./event-bus.js", () => ({ publishEvent: vi.fn() })); +vi.mock("../workers/webhook-worker.js", () => ({ + enqueueWebhookEvent: vi.fn().mockResolvedValue(undefined), +})); vi.mock("../logger.js", () => ({ logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, })); @@ -133,6 +136,34 @@ describe("transitionTask", () => { ); }); + it("clears stale errorMessage and resultSummary when a PR is detected (pr_opened)", async () => { + // Regression: the agent can exit non-zero after opening a valid PR. + // updateTaskResult persists e.g. "Exit code: 1" before PR detection runs, + // so the pr_opened transition must wipe those stale fields — a task with + // an open PR must not look like a failed task. + const task = { + id: "t1", + state: "running", + startedAt: new Date(), + ticketSource: null, + errorMessage: "Exit code: 1", + resultSummary: "Agent exited with code 1", + }; + vi.mocked(db.select().from(undefined as any).where).mockResolvedValueOnce([task]); + vi.mocked(db as any).returning.mockResolvedValueOnce([ + { ...task, state: "pr_opened", errorMessage: null, resultSummary: null }, + ]); + vi.mocked(db.insert(undefined as any).values).mockResolvedValueOnce(undefined as any); + await transitionTask("t1", TaskState.PR_OPENED, "pr_detected", "https://github.com/o/r/pull/1"); + expect(db.update(undefined as any).set).toHaveBeenCalledWith( + expect.objectContaining({ + state: TaskState.PR_OPENED, + errorMessage: null, + resultSummary: null, + }), + ); + }); + it("throws StateRaceError when atomic update returns 0 rows", async () => { const task = { id: "t1", state: "queued", startedAt: null }; vi.mocked(db.select().from(undefined as any).where) diff --git a/apps/api/src/services/task-service.ts b/apps/api/src/services/task-service.ts index 1087fdc6..4cb7ed44 100644 --- a/apps/api/src/services/task-service.ts +++ b/apps/api/src/services/task-service.ts @@ -250,7 +250,10 @@ export async function transitionTask( updateFields.completedAt = new Date(); } // Clear error fields on successful completion (PR merged after prior errors) - if (toState === TaskState.COMPLETED) { + // and on PR-open: the agent can exit non-zero after opening a valid PR, in + // which case updateTaskResult has already persisted e.g. "Exit code: 1". + // A task with an open PR is not completed, but it must not look failed. + if (toState === TaskState.COMPLETED || toState === TaskState.PR_OPENED) { updateFields.errorMessage = null; updateFields.resultSummary = null; } diff --git a/apps/api/src/services/workflow-pool-service.test.ts b/apps/api/src/services/workflow-pool-service.test.ts index 037bf664..77b2f7d6 100644 --- a/apps/api/src/services/workflow-pool-service.test.ts +++ b/apps/api/src/services/workflow-pool-service.test.ts @@ -462,8 +462,9 @@ describe("execRunInPod", () => { const execCall = mockRuntimeExec.mock.calls[0]; expect(execCall[1][0]).toBe("bash"); expect(execCall[1][1]).toBe("-c"); - // The script should contain the base64-encoded env - expect(execCall[1][2]).toContain("base64"); + // Env vars are embedded as inert single-quoted exports (no eval/word-splitting) + expect(execCall[1][2]).toContain("export MY_VAR='hello'"); + expect(execCall[1][2]).not.toContain("eval $("); // And cd into per-run working directory expect(execCall[1][2]).toContain("/workspace/runs/run-1"); }); diff --git a/apps/api/src/services/workflow-pool-service.ts b/apps/api/src/services/workflow-pool-service.ts index 3373f722..9e32a843 100644 --- a/apps/api/src/services/workflow-pool-service.ts +++ b/apps/api/src/services/workflow-pool-service.ts @@ -12,6 +12,7 @@ import { import { logger } from "../logger.js"; import { resolveImage } from "./repo-pool-service.js"; import { getWorkloadManager, isStatefulSetEnabled } from "./k8s-workload-service.js"; +import { buildEnvExports } from "../utils/pod-env.js"; const IDLE_TIMEOUT_MS = parseIntEnv("OPTIO_WORKFLOW_POD_IDLE_MS", 600000); // 10 min default @@ -396,17 +397,11 @@ export async function execRunInPod( }) .where(eq(workflowPods.id, pod.id)); - const envJson = JSON.stringify({ ...env, OPTIO_WORKFLOW_RUN_ID: runId }); - const envB64 = Buffer.from(envJson).toString("base64"); - const script = [ "set -e", - `eval $(echo '${envB64}' | base64 -d | python3 -c "`, - `import json, sys, shlex`, - `env = json.load(sys.stdin)`, - `for k, v in env.items():`, - ` print(f'export {k}={shlex.quote(v)}')`, - `")`, + // Env values (including the prompt) are embedded as inert single-quoted + // exports — see buildEnvExports. + ...buildEnvExports({ ...env, OPTIO_WORKFLOW_RUN_ID: runId }), `echo "[optio] Waiting for workflow pod to be ready..."`, `for i in $(seq 1 120); do [ -f /workspace/.ready ] && break; sleep 1; done`, `[ -f /workspace/.ready ] || { echo "[optio] ERROR: workflow pod not ready after 120s"; exit 1; }`, diff --git a/apps/api/src/utils/pod-env.test.ts b/apps/api/src/utils/pod-env.test.ts new file mode 100644 index 00000000..58e589f4 --- /dev/null +++ b/apps/api/src/utils/pod-env.test.ts @@ -0,0 +1,108 @@ +import { describe, it, expect } from "vitest"; +import { execFileSync } from "node:child_process"; +import { mkdtempSync, readFileSync, readdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { shellSingleQuote, buildEnvExports } from "./pod-env.js"; + +/** + * Regression payload for the Phase 4F shell-quoting bug: a realistic task + * prompt with markdown backticks, `$HOME`, wildcard text like `optio/task-*`, + * literal newlines, and single quotes. It also carries canary commands — if + * any of them run, the env injection leaked prompt content to the shell. + */ +const HOSTILE_PROMPT = [ + "Fix the `pr_opened` handling before broader autonomous use.", + "", + "Steps:", + "1. Inspect $HOME and run `git status` in the worktree.", + "2. Don't touch branches named optio/task-* — they belong to other agents.", + "3. Preserve 'single-quoted' text exactly as written.", + "```bash", + "touch injected-from-fenced-block", + "```", + "$(touch injected-from-substitution)", + "`touch injected-from-backquotes`", + "rm -rf $HOME/should-never-expand", +].join("\n"); + +function runBash(script: string, cwd: string): string { + return execFileSync("bash", ["-c", script], { + cwd, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); +} + +describe("shellSingleQuote", () => { + it("wraps plain values in single quotes", () => { + expect(shellSingleQuote("hello")).toBe("'hello'"); + }); + + it("escapes embedded single quotes", () => { + expect(shellSingleQuote("don't")).toBe("'don'\\''t'"); + }); + + it("handles empty strings", () => { + expect(shellSingleQuote("")).toBe("''"); + }); +}); + +describe("buildEnvExports", () => { + it("emits one export statement per env entry", () => { + expect(buildEnvExports({ A: "1", B_2: "two" })).toEqual(["export A='1'", "export B_2='two'"]); + }); + + it("rejects env names bash would not accept as identifiers", () => { + expect(() => buildEnvExports({ "BAD-NAME": "x" })).toThrow(/Invalid environment variable/); + expect(() => buildEnvExports({ "PATH; touch pwned": "x" })).toThrow( + /Invalid environment variable/, + ); + expect(() => buildEnvExports({ "1LEADING": "x" })).toThrow(/Invalid environment variable/); + }); + + it("round-trips a hostile prompt through bash without executing its contents", () => { + const dir = mkdtempSync(join(tmpdir(), "pod-env-")); + try { + const script = [ + "set -e", + ...buildEnvExports({ + OPTIO_PROMPT: HOSTILE_PROMPT, + OPTIO_TASK_ID: "task-1", + }), + `printf '%s' "$OPTIO_PROMPT" > prompt-out`, + `printf '%s' "$OPTIO_TASK_ID" > task-id-out`, + ].join("\n"); + + // Throws on non-zero exit — e.g. "command not found" from a prompt + // line leaking to the shell under `set -e`. + const stdout = runBash(script, dir); + expect(stdout).toBe(""); + + // Exact round-trip: newlines, backticks, $HOME, globs, quotes intact. + expect(readFileSync(join(dir, "prompt-out"), "utf8")).toBe(HOSTILE_PROMPT); + expect(readFileSync(join(dir, "task-id-out"), "utf8")).toBe("task-1"); + + // No canary commands executed — only our two output files exist. + expect(readdirSync(dir).sort()).toEqual(["prompt-out", "task-id-out"]); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("keeps values with only quotes and whitespace intact", () => { + const dir = mkdtempSync(join(tmpdir(), "pod-env-")); + try { + const value = ` ' " \t '' \n `; + const script = [ + "set -e", + ...buildEnvExports({ TRICKY: value }), + `printf '%s' "$TRICKY" > out`, + ].join("\n"); + runBash(script, dir); + expect(readFileSync(join(dir, "out"), "utf8")).toBe(value); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/apps/api/src/utils/pod-env.ts b/apps/api/src/utils/pod-env.ts new file mode 100644 index 00000000..333cd254 --- /dev/null +++ b/apps/api/src/utils/pod-env.ts @@ -0,0 +1,37 @@ +/** + * Helpers for injecting environment variables into agent pod exec scripts. + * + * Values (including the task prompt) are emitted as single-quoted `export` + * statements embedded directly in the bash script. Single-quoted strings are + * fully inert in bash — no command substitution, variable expansion, glob + * expansion, or word splitting — and may span multiple lines, so prompt text + * containing markdown backticks, `$VARS`, wildcards like `optio/task-*`, or + * literal newlines round-trips exactly. + * + * The previous implementation piped base64-encoded JSON through python3 into + * `eval $(...)`. Because the command substitution was unquoted, bash applied + * word splitting and pathname expansion to the shlex-quoted output before + * eval re-parsed it, which collapsed newlines in prompts to spaces and could + * surface prompt fragments to the shell before the agent started. + */ + +const VALID_ENV_NAME = /^[A-Za-z_][A-Za-z0-9_]*$/; + +/** Escape a value as an inert single-quoted bash literal (`'\''` for embedded quotes). */ +export function shellSingleQuote(value: string): string { + return `'${value.replaceAll("'", "'\\''")}'`; +} + +/** + * Build `export KEY='value'` script lines for every env entry. + * Throws on names bash would reject as identifiers — interpolating an + * arbitrary name into `export =` would otherwise allow injection. + */ +export function buildEnvExports(env: Record): string[] { + return Object.entries(env).map(([key, value]) => { + if (!VALID_ENV_NAME.test(key)) { + throw new Error(`Invalid environment variable name for pod exec: ${JSON.stringify(key)}`); + } + return `export ${key}=${shellSingleQuote(String(value))}`; + }); +}