Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions apps/api/src/services/persistent-agent-pool-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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}`,
Expand Down
73 changes: 73 additions & 0 deletions apps/api/src/services/repo-pool-service.test.ts
Original file line number Diff line number Diff line change
@@ -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 ───────────────────────────────────────────────────────────

Expand Down Expand Up @@ -102,6 +105,7 @@ import {
deleteNetworkPolicy,
killOrphanedAgentInPod,
parseJsonEnv,
execTaskInRepoPod,
} from "./repo-pool-service.js";

// ── resolveImage ────────────────────────────────────────────────────
Expand Down Expand Up @@ -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<typeof import("node:child_process")>("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 });
}
});
});
14 changes: 5 additions & 9 deletions apps/api/src/services/repo-pool-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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; }`,
Expand Down
31 changes: 31 additions & 0 deletions apps/api/src/services/task-service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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() },
}));
Expand Down Expand Up @@ -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)
Expand Down
5 changes: 4 additions & 1 deletion apps/api/src/services/task-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
5 changes: 3 additions & 2 deletions apps/api/src/services/workflow-pool-service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
Expand Down
13 changes: 4 additions & 9 deletions apps/api/src/services/workflow-pool-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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; }`,
Expand Down
108 changes: 108 additions & 0 deletions apps/api/src/utils/pod-env.test.ts
Original file line number Diff line number Diff line change
@@ -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 });
}
});
});
Loading