diff --git a/packages/review-tutor/README.md b/packages/review-tutor/README.md index 3cd3d7c..0da4d63 100644 --- a/packages/review-tutor/README.md +++ b/packages/review-tutor/README.md @@ -46,7 +46,11 @@ The model dialog lists the session's scoped models when `--models` or the settin ## Harness connectors -A harness connector owns model discovery, isolated invocation, and stream parsing while the shared runner owns process lifetime, bounds, and cancellation. Pi is registered by default; the Claude Code connector is available behind the connector flag, and Codex support is tracked in [#63](https://github.com/pickforge/pickforge-platform/issues/63). The `reviewTutorHarnessConnectors` flag defaults off. For local testing on main, set `REVIEW_TUTOR_FLAGS=reviewTutorHarnessConnectors` before starting Pi. Child processes receive only the shared environment allowlist plus keys explicitly declared by their connector, and runner failures redact common API keys, bearer credentials, and tokens before leaving the process boundary. The Claude Code connector forwards `CLAUDE_CONFIG_DIR` when present, but never forwards `ANTHROPIC_API_KEY`; users who rely on that environment key must sign in through Claude Code instead. +A harness connector owns model discovery, isolated invocation, and stream parsing while the shared runner owns process lifetime, bounds, and cancellation. Pi is registered by default; the Claude Code and Codex connectors are available behind `reviewTutorHarnessConnectors`. The flag defaults off. For local testing on main, set `REVIEW_TUTOR_FLAGS=reviewTutorHarnessConnectors` before starting Pi. Child processes receive only the shared environment allowlist plus keys explicitly declared by their connector, and runner failures redact common API keys, bearer credentials, and tokens before leaving the process boundary. + +The Claude Code connector forwards `CLAUDE_CONFIG_DIR` when present, but never forwards `ANTHROPIC_API_KEY`; users who rely on that environment key must sign in through Claude Code instead. + +The Codex connector discovers models through `codex app-server`, invokes reviews with `codex exec --json`, and relies on Codex's existing local authentication. ## Local data diff --git a/packages/review-tutor/src/connectors/codex.ts b/packages/review-tutor/src/connectors/codex.ts new file mode 100644 index 0000000..384ecd9 --- /dev/null +++ b/packages/review-tutor/src/connectors/codex.ts @@ -0,0 +1,245 @@ +import { execFile as nodeExecFile } from "node:child_process"; +import type { + ConnectorRequest, + Discovery, + DiscoveryDeps, + DiscoveryExecFile, + HarnessConnector, + ParseSink, + ParsedAnswer, + SpawnSpec, +} from "./types.ts"; +import { redact } from "./redact.ts"; +import { ConnectorError } from "./types.ts"; + +const MAX_CATALOG_BYTES = 1024 * 1024; +const DISCOVERY_TIMEOUT_MS = 10_000; +const FALLBACK_LEVELS = ["low", "medium", "high"]; +const MINIMUM_VERSION = [0, 140, 0] as const; +const DISCOVERY_ENV_KEYS = ["PATH", "HOME", "USER", "LOGNAME", "LANG", "LC_ALL", "CODEX_HOME"] as const; + +interface CodexModel { + slug?: unknown; + display_name?: unknown; + visibility?: unknown; + supported_reasoning_levels?: unknown; + priority?: unknown; +} + +interface CodexEvent { + type?: unknown; + message?: unknown; + error?: { message?: unknown }; + item?: { type?: unknown; text?: unknown }; + usage?: unknown; +} + +export function discoveryEnvironment(source: NodeJS.ProcessEnv = process.env): NodeJS.ProcessEnv { + return Object.fromEntries(DISCOVERY_ENV_KEYS.flatMap((key) => { + const value = source[key]; + return value === undefined ? [] : [[key, value]]; + })); +} + +const defaultExecFile: DiscoveryExecFile = (file, args, options) => new Promise((resolve, reject) => { + nodeExecFile(file, args, { ...options, env: discoveryEnvironment(), shell: false }, (error, stdout, stderr) => { + if (error) reject(error); + else resolve({ stdout, stderr }); + }); +}); + +function versionAtLeast(version: readonly number[]): boolean { + for (let index = 0; index < MINIMUM_VERSION.length; index += 1) { + const difference = version[index]! - MINIMUM_VERSION[index]!; + if (difference !== 0) return difference > 0; + } + return true; +} + +function validListedModel(value: unknown): value is CodexModel & { slug: string; visibility: "list" } { + if (!value || typeof value !== "object") return false; + const model = value as CodexModel; + return typeof model.slug === "string" && model.visibility === "list"; +} + +function parseModels(value: string): CodexModel[] { + const parsed = JSON.parse(value) as unknown; + const models = Array.isArray(parsed) + ? parsed + : parsed && typeof parsed === "object" && Array.isArray((parsed as { models?: unknown }).models) + ? (parsed as { models: unknown[] }).models + : undefined; + if (!models) throw new Error("invalid catalog"); + const listed = models.filter(validListedModel); + if (listed.length === 0) throw new Error("invalid catalog"); + return listed; +} + +function priority(model: CodexModel): number { + return typeof model.priority === "number" && Number.isFinite(model.priority) + ? model.priority + : Number.POSITIVE_INFINITY; +} + +function modelChoices(models: CodexModel[]) { + return models + .sort((left, right) => priority(left) - priority(right) + || (left.slug as string).localeCompare(right.slug as string)) + .flatMap((model) => { + const slug = model.slug as string; + const levels = model.supported_reasoning_levels === undefined + ? FALLBACK_LEVELS + : Array.isArray(model.supported_reasoning_levels) + ? model.supported_reasoning_levels + .flatMap((level) => level && typeof level === "object" + && typeof (level as { effort?: unknown }).effort === "string" + ? [(level as { effort: string }).effort] + : []) + .filter((effort) => effort !== "max" + && effort !== "ultra" + && /^[a-z][a-z0-9_-]*$/.test(effort)) + : []; + return levels.length === 0 ? [] : [{ + id: `codex:${slug}`, + label: typeof model.display_name === "string" && model.display_name || slug, + thinkingLevels: [...levels], + }]; + }); +} + +function parseEvent(line: string): CodexEvent | undefined { + try { + return JSON.parse(line) as CodexEvent; + } catch { + return undefined; + } +} + +function numericUsage(value: unknown): Record | undefined { + if (!value || typeof value !== "object") return undefined; + return Object.fromEntries(Object.entries(value) + .filter((entry): entry is [string, number] => typeof entry[1] === "number")); +} + +function providerMessage(message: string, model: string): ConnectorError { + let safe: string; + if (/401|unauthorized|missing bearer|not logged in|login/i.test(message)) { + safe = "Codex is not logged in. Run `codex login`, then ask again."; + } else if (/429|rate limit|quota|insufficient_quota/i.test(message)) { + safe = "Codex is rate-limited or out of quota right now. Try again later."; + } else if (/model .* not (found|supported)|unknown model|invalid model/i.test(message)) { + safe = `Codex rejected model ${model}.`; + } else { + safe = redact(message).replace(/\s+/g, " ").trim().slice(0, 200); + } + return new ConnectorError(safe || "Codex failed to complete the turn."); +} + +export class CodexConnector implements HarnessConnector { + readonly id = "codex" as const; + readonly label = "Codex"; + readonly envKeys = ["CODEX_HOME"] as const; + private completed = false; + private lastError?: string; + private model = "unknown"; + + async discover(deps: DiscoveryDeps): Promise { + const execFile = deps.execFile ?? defaultExecFile; + let versionOutput: string; + try { + const options = { + encoding: "utf8" as const, + maxBuffer: MAX_CATALOG_BYTES, + signal: AbortSignal.timeout(DISCOVERY_TIMEOUT_MS), + timeout: DISCOVERY_TIMEOUT_MS, + }; + ({ stdout: versionOutput } = await execFile("codex", ["--version"], options)); + } catch (error) { + return (error as NodeJS.ErrnoException).code === "ENOENT" + ? { available: false, reason: "Codex is not installed (codex not found on PATH)." } + : { available: false, reason: "Codex could not report a supported version." }; + } + const match = /codex-cli (\d+)\.(\d+)\.(\d+)/.exec(versionOutput); + if (!match) { + return { available: false, reason: "Codex could not report a supported version." }; + } + const version = match.slice(1, 4).map(Number); + const versionLabel = version.join("."); + if (!versionAtLeast(version)) { + return { + available: false, + reason: `Codex ${versionLabel} is too old; version 0.140.0 or newer is required.`, + }; + } + try { + const options = { + encoding: "utf8" as const, + maxBuffer: MAX_CATALOG_BYTES, + signal: AbortSignal.timeout(DISCOVERY_TIMEOUT_MS), + timeout: DISCOVERY_TIMEOUT_MS, + }; + const { stdout } = await execFile("codex", ["debug", "models"], options); + return { available: true, version: versionLabel, models: modelChoices(parseModels(stdout)) }; + } catch { + return { available: false, reason: "Codex could not list its models." }; + } + } + + spawnSpec(request: ConnectorRequest): SpawnSpec { + this.model = request.model; + return { + command: "codex", + args: [ + "exec", "--json", "--ephemeral", "--skip-git-repo-check", + "-s", "read-only", "-C", request.cwd, + "-m", request.model, "-c", `model_reasoning_effort=\"${request.thinking}\"`, + "-c", "shell_environment_policy.inherit=\"none\"", + "-c", "model_verbosity=\"low\"", "-", + ], + }; + } + + parseLine(line: string, sink: ParseSink): void { + const event = parseEvent(line); + if (!event) return; + if (event.type === "item.completed" + && event.item?.type === "agent_message" + && typeof event.item.text === "string") { + sink.delta(event.item.text); + sink.final(event.item.text); + return; + } + if (event.type === "error" && typeof event.message === "string") { + this.lastError = event.message; + return; + } + if (event.type === "turn.failed") { + const message = typeof event.error?.message === "string" + ? event.error.message + : this.lastError ?? "Codex failed to complete the turn."; + throw providerMessage(message, this.model); + } + if (event.type === "turn.completed") { + this.completed = true; + const usage = numericUsage(event.usage); + if (usage) sink.usage(usage); + } + } + + finish(sink: ParseSink): ParsedAnswer { + try { + if (!this.completed) { + if (this.lastError) throw providerMessage(this.lastError, this.model); + throw new ConnectorError("Codex exited without completing the turn."); + } + if (!sink.answer?.trim()) { + throw new ConnectorError("Codex returned an empty answer."); + } + return { answer: sink.answer, ...(sink.answerUsage ? { usage: sink.answerUsage } : {}) }; + } finally { + this.completed = false; + this.lastError = undefined; + this.model = "unknown"; + } + } +} diff --git a/packages/review-tutor/src/connectors/redact.ts b/packages/review-tutor/src/connectors/redact.ts index ed4fd81..dfab46f 100644 --- a/packages/review-tutor/src/connectors/redact.ts +++ b/packages/review-tutor/src/connectors/redact.ts @@ -5,6 +5,10 @@ const PATTERNS = [ /token=\S+/gi, /ghp_\w+/g, /gho_\w+/g, + /url:\s*\S+/gi, + /cf-ray:\s*\S+/gi, + /request id:\s*\S+/gi, + /thread[_ ]id:?\s*\S+/gi, ] as const; export function redact(value: string): string { diff --git a/packages/review-tutor/src/connectors/registry.ts b/packages/review-tutor/src/connectors/registry.ts index 5c76411..e754fc4 100644 --- a/packages/review-tutor/src/connectors/registry.ts +++ b/packages/review-tutor/src/connectors/registry.ts @@ -1,10 +1,12 @@ import { execFile } from "node:child_process"; import type { ReviewTutorFlags } from "../flags.ts"; import { ClaudeCodeConnector } from "./claude-code.ts"; +import { CodexConnector } from "./codex.ts"; import { PiConnector } from "./pi.ts"; import type { Discovery, DiscoveryDeps, + DiscoveryExecFile, HarnessConnector, HarnessId, ModelChoice, @@ -30,13 +32,15 @@ export function createConnectorRegistry(options: { piModels: ModelChoice[]; piVersion?: string; which?: DiscoveryDeps["which"]; + execFile?: DiscoveryExecFile; }): ConnectorRegistry { const pi = new PiConnector(); - const optionalConnectors: HarnessConnector[] = [new ClaudeCodeConnector()]; + const optionalConnectors: HarnessConnector[] = [new ClaudeCodeConnector(), new CodexConnector()]; const dependencies: DiscoveryDeps = { piModels: options.piModels, ...(options.piVersion ? { piVersion: options.piVersion } : {}), which: options.which ?? which, + ...(options.execFile ? { execFile: options.execFile } : {}), }; const connectors = (): HarnessConnector[] => [ diff --git a/packages/review-tutor/src/connectors/types.ts b/packages/review-tutor/src/connectors/types.ts index d100af5..7e48d2e 100644 --- a/packages/review-tutor/src/connectors/types.ts +++ b/packages/review-tutor/src/connectors/types.ts @@ -10,10 +10,21 @@ export interface HarnessConnector { readonly envKeys?: readonly string[]; } +export type DiscoveryExecFile = ( + file: string, + args: string[], + options: { + encoding: "utf8"; + maxBuffer: number; + timeout: number; + }, +) => Promise<{ stdout: string; stderr: string }>; + export interface DiscoveryDeps { piModels: ModelChoice[]; piVersion?: string; which(command: string): Promise; + execFile?: DiscoveryExecFile; } export type Discovery = diff --git a/packages/review-tutor/test/connector-codex.test.ts b/packages/review-tutor/test/connector-codex.test.ts new file mode 100644 index 0000000..a2280fc --- /dev/null +++ b/packages/review-tutor/test/connector-codex.test.ts @@ -0,0 +1,432 @@ +import { EventEmitter } from "node:events"; +import { readFile } from "node:fs/promises"; +import { PassThrough } from "node:stream"; +import { fileURLToPath } from "node:url"; +import { describe, expect, it, vi } from "vitest"; +import { CodexConnector, discoveryEnvironment } from "../src/connectors/codex.ts"; +import { createConnectorRegistry } from "../src/connectors/registry.ts"; +import type { DiscoveryDeps, ParseSink } from "../src/connectors/types.ts"; +import { createReviewTutorFlags } from "../src/flags.ts"; +import { TutorRunner } from "../src/runner.ts"; + +class FakeChild extends EventEmitter { + stdout = new PassThrough(); + stderr = new PassThrough(); + stdin = new PassThrough(); + pid = 123; + kill = vi.fn(); +} + +const fixture = (name: string) => fileURLToPath(new URL(`fixtures/codex/${name}`, import.meta.url)); +const catalog = await readFile(fixture("models.json"), "utf8"); +const piModels = [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }]; + +function fakeDiscovery(outputs: Record): DiscoveryDeps { + return { + piModels, + which: async () => undefined, + execFile: vi.fn(async (_file, args) => { + const key = args.join(" "); + const output = outputs[key]; + if (output instanceof Error) throw output; + if (output === undefined) throw new Error(`unexpected command: ${key}`); + return { stdout: output, stderr: "" }; + }), + }; +} + +function sink() { + let answer: string | undefined; + let answerUsage: Record | undefined; + return { + get answer() { return answer; }, + get answerUsage() { return answerUsage; }, + deltas: [] as string[], + usages: [] as Record[], + finals: [] as string[], + delta(text: string) { this.deltas.push(text); }, + usage(value: Record) { answerUsage = value; this.usages.push(value); }, + final(value: string) { answer = value; this.finals.push(value); }, + } satisfies ParseSink & { deltas: string[]; usages: Record[]; finals: string[] }; +} + +async function parseFixture(name: string, connector = new CodexConnector()) { + const output = sink(); + for (const line of (await readFile(fixture(name), "utf8")).split("\n")) { + if (line) connector.parseLine(line, output); + } + return { connector, output }; +} + +const request = { + model: "gpt-5.6-sol", + thinking: "low", + cwd: "/repo", + prompt: "Reply with the single word ok.", +}; + +describe("Codex discovery", () => { + it("filters discovery environment credentials", () => { + expect(discoveryEnvironment({ + PATH: "/bin", + CODEX_HOME: "/tmp/codex-home", + OPENAI_API_KEY: "sk-proj-fakefakefake", + })).toEqual({ PATH: "/bin", CODEX_HOME: "/tmp/codex-home" }); + }); + + it("reports a missing executable", async () => { + const connector = new CodexConnector(); + const missing = Object.assign(new Error("spawn codex ENOENT"), { code: "ENOENT" }); + await expect(connector.discover(fakeDiscovery({ "--version": missing }))).resolves.toEqual({ + available: false, + reason: "Codex is not installed (codex not found on PATH).", + }); + }); + + it("distinguishes other version failures from a missing executable", async () => { + const connector = new CodexConnector(); + await expect(connector.discover(fakeDiscovery({ "--version": new Error("timed out") }))).resolves.toEqual({ + available: false, + reason: "Codex could not report a supported version.", + }); + }); + + it("rejects unverified old versions", async () => { + const connector = new CodexConnector(); + await expect(connector.discover(fakeDiscovery({ "--version": "codex-cli 0.120.0\n" }))).resolves.toEqual({ + available: false, + reason: "Codex 0.120.0 is too old; version 0.140.0 or newer is required.", + }); + }); + + it("accepts the exact minimum version", async () => { + const connector = new CodexConnector(); + await expect(connector.discover(fakeDiscovery({ + "--version": "codex-cli 0.140.0\n", + "debug models": catalog, + }))).resolves.toMatchObject({ available: true, version: "0.140.0" }); + }); + + it("rejects unrecognized version output", async () => { + const connector = new CodexConnector(); + await expect(connector.discover(fakeDiscovery({ "--version": "codex 1.2.3\n" }))).resolves.toEqual({ + available: false, + reason: "Codex could not report a supported version.", + }); + }); + + it("discovers, sorts, namespaces, and filters the object catalog", async () => { + const deps = fakeDiscovery({ "--version": "codex-cli 0.147.0\n", "debug models": catalog }); + const connector = new CodexConnector(); + await expect(connector.discover(deps)).resolves.toEqual({ + available: true, + version: "0.147.0", + models: [ + { id: "codex:gpt-5.6-sol", label: "GPT-5.6 Sol", thinkingLevels: ["low", "medium", "high"] }, + { id: "codex:gpt-5.5", label: "gpt-5.5", thinkingLevels: ["low", "medium", "high"] }, + ], + }); + expect(deps.execFile).toHaveBeenNthCalledWith(1, "codex", ["--version"], { + encoding: "utf8", + maxBuffer: 1024 * 1024, + signal: expect.any(AbortSignal), + timeout: 10_000, + }); + expect(deps.execFile).toHaveBeenNthCalledWith(2, "codex", ["debug", "models"], { + encoding: "utf8", + maxBuffer: 1024 * 1024, + signal: expect.any(AbortSignal), + timeout: 10_000, + }); + expect(deps.execFile).toHaveBeenCalledTimes(2); + }); + + it("accepts a bare-array catalog", async () => { + const bare = JSON.stringify(JSON.parse(catalog).models); + const connector = new CodexConnector(); + await expect(connector.discover(fakeDiscovery({ + "--version": "codex-cli 0.147.0\n", + "debug models": bare, + }))).resolves.toMatchObject({ available: true, models: expect.arrayContaining([ + expect.objectContaining({ id: "codex:gpt-5.6-sol" }), + ]) }); + }); + + it("keeps valid listed models while skipping malformed entries and sorting absent priorities last", async () => { + const mixed = JSON.stringify({ models: [ + null, + { slug: 42, visibility: "list", priority: 1 }, + { slug: "hidden", visibility: "hidden", priority: 0 }, + { slug: "z-last", visibility: "list" }, + { slug: "first", visibility: "list", priority: 2 }, + { slug: "a-last", visibility: "list", priority: Number.NaN }, + ] }); + const connector = new CodexConnector(); + await expect(connector.discover(fakeDiscovery({ + "--version": "codex-cli 0.147.0\n", + "debug models": mixed, + }))).resolves.toEqual({ + available: true, + version: "0.147.0", + models: [ + { id: "codex:first", label: "first", thinkingLevels: ["low", "medium", "high"] }, + { id: "codex:a-last", label: "a-last", thinkingLevels: ["low", "medium", "high"] }, + { id: "codex:z-last", label: "z-last", thinkingLevels: ["low", "medium", "high"] }, + ], + }); + }); + + it("excludes models with no safe usable thinking levels", async () => { + const levels = JSON.stringify({ models: [ + { slug: "only-prohibited", visibility: "list", supported_reasoning_levels: [ + { effort: "max" }, { effort: "ultra" }, + ] }, + { slug: "empty", visibility: "list", supported_reasoning_levels: [] }, + { slug: "unsafe", visibility: "list", supported_reasoning_levels: [ + { effort: "low\"" }, { effort: "x\ny" }, + ] }, + { slug: "safe", visibility: "list", supported_reasoning_levels: [ + { effort: "low" }, { effort: "xhigh" }, { effort: "medium-fast" }, + ] }, + ] }); + const connector = new CodexConnector(); + await expect(connector.discover(fakeDiscovery({ + "--version": "codex-cli 0.147.0\n", + "debug models": levels, + }))).resolves.toMatchObject({ models: [{ + id: "codex:safe", + thinkingLevels: ["low", "xhigh", "medium-fast"], + }] }); + }); + + it("keeps a valid catalog available when all listed models have no usable levels", async () => { + const connector = new CodexConnector(); + await expect(connector.discover(fakeDiscovery({ + "--version": "codex-cli 0.147.0\n", + "debug models": JSON.stringify({ models: [ + { slug: "only-prohibited", visibility: "list", supported_reasoning_levels: [ + { effort: "max" }, { effort: "ultra" }, + ] }, + { slug: "empty", visibility: "list", supported_reasoning_levels: [] }, + ] }), + }))).resolves.toEqual({ available: true, version: "0.147.0", models: [] }); + }); + + it.each([ + "not json", + JSON.stringify({ models: "not an array" }), + JSON.stringify({ models: [] }), + JSON.stringify({ models: [{ slug: 42, visibility: "list" }] }), + new Error("timed out"), + ])("reports malformed or failing catalogs as unavailable", async (failure) => { + const connector = new CodexConnector(); + await expect(connector.discover(fakeDiscovery({ + "--version": "codex-cli 0.147.0\n", + "debug models": failure, + }))).resolves.toEqual({ available: false, reason: "Codex could not list its models." }); + }); +}); + +describe("Codex parsing", () => { + it("parses a completed turn", async () => { + const { connector, output } = await parseFixture("success.jsonl"); + expect(connector.finish(output)).toEqual({ + answer: "ok", + usage: { input_tokens: 12, output_tokens: 3, cached_input_tokens: 4, reasoning_output_tokens: 2 }, + }); + expect(output.deltas).toEqual(["ok"]); + expect(output.finals).toEqual(["ok"]); + }); + + it.each([ + ["429 rate limit exceeded", "Codex is rate-limited or out of quota right now. Try again later."], + ["unknown model", "Codex rejected model gpt-5.6-sol."], + ])("maps %s failures", (message, expected) => { + const connector = new CodexConnector(); + connector.spawnSpec(request); + expect(() => connector.parseLine(JSON.stringify({ + type: "turn.failed", + error: { message }, + }), sink())).toThrow(expected); + }); + + it("maps a final 401 without exposing provider internals", async () => { + const connector = new CodexConnector(); + connector.spawnSpec(request); + const output = sink(); + const lines = (await readFile(fixture("unauthorized.jsonl"), "utf8")).trim().split("\n"); + connector.parseLine(lines[0]!, output); + connector.parseLine(lines[1]!, output); + expect(() => connector.parseLine(lines[2]!, output)).toThrow( + "Codex is not logged in. Run `codex login`, then ask again.", + ); + expect(() => connector.parseLine(lines[2]!, output)).not.toThrow(/url:|request id:/i); + }); + + it("rejects an empty answer", async () => { + const { connector, output } = await parseFixture("empty.jsonl"); + expect(() => connector.finish(output)).toThrow("Codex returned an empty answer."); + }); + + it("ignores malformed lines", () => { + const connector = new CodexConnector(); + const output = sink(); + expect(() => connector.parseLine("not json", output)).not.toThrow(); + }); + + it("rejects output without turn.completed", async () => { + const { connector, output } = await parseFixture("incomplete.jsonl"); + expect(() => connector.finish(output)).toThrow("Codex exited without completing the turn."); + }); + + it("maps the last error when a turn exits before completion", () => { + const connector = new CodexConnector(); + const output = sink(); + connector.parseLine(JSON.stringify({ + type: "error", + message: "401 unauthorized url: https://provider.invalid request id: req_fixture", + }), output); + expect(() => connector.finish(output)).toThrow( + "Codex is not logged in. Run `codex login`, then ask again.", + ); + expect(() => connector.finish(sink())).toThrow("Codex exited without completing the turn."); + }); + + it("strips provider internals from fallback errors", () => { + const connector = new CodexConnector(); + connector.spawnSpec(request); + const output = sink(); + expect(() => connector.parseLine(JSON.stringify({ + type: "turn.failed", + error: { message: "provider failed url: https://provider.invalid cf-ray: ray_fixture request id: req_fixture" }, + }), output)).toThrow("provider failed"); + expect(() => connector.parseLine(JSON.stringify({ + type: "turn.failed", + error: { message: "provider failed url: https://provider.invalid cf-ray: ray_fixture request id: req_fixture" }, + }), output)).not.toThrow(/url:|cf-ray:|request id:/i); + let leaked: Error | undefined; + try { + connector.parseLine(JSON.stringify({ + type: "turn.failed", + error: { message: "upstream refused: Authorization: Bearer sk-proj-fakefakefake thread_id: thread_abc token=abcdef123" }, + }), output); + } catch (error) { + leaked = error as Error; + } + expect(leaked?.message).toContain("[redacted]"); + expect(leaked?.message).not.toMatch(/sk-proj|thread_abc|abcdef123|Bearer sk/); + }); +}); + +describe("Codex runner integration", () => { + it("pins argv and writes the prompt to stdin", async () => { + const child = new FakeChild(); + const spawn = vi.fn((_command: string, _args: readonly string[], _options: unknown) => child as never); + let prompt = ""; + child.stdin.on("data", (chunk) => { prompt += chunk.toString(); }); + const connector = new CodexConnector(); + const runner = new TutorRunner({ + spawn, + env: { + PATH: "/bin", + CODEX_HOME: "/tmp/codex-home", + OPENAI_API_KEY: "sk-proj-fakefakefake", + }, + }); + const done = runner.run({ ...request, connector }, () => {}); + child.stdout.end(`${JSON.stringify({ type: "item.completed", item: { type: "agent_message", text: "ok" } })}\n${JSON.stringify({ type: "turn.completed", usage: { input_tokens: 1, output_tokens: 1 } })}\n`); + child.emit("close", 0, null); + await expect(done).resolves.toMatchObject({ answer: "ok" }); + expect(prompt).toBe(request.prompt); + expect(child.stdin.writableEnded).toBe(true); + expect(spawn.mock.calls[0]?.[0]).toBe("codex"); + expect(spawn.mock.calls[0]?.[1]).toEqual([ + "exec", "--json", "--ephemeral", "--skip-git-repo-check", + "-s", "read-only", "-C", "/repo", + "-m", "gpt-5.6-sol", "-c", "model_reasoning_effort=\"low\"", + "-c", "shell_environment_policy.inherit=\"none\"", + "-c", "model_verbosity=\"low\"", "-", + ]); + expect(spawn.mock.calls[0]?.[2]).toMatchObject({ + cwd: "/repo", + env: { + PATH: "/bin", + CODEX_HOME: "/tmp/codex-home", + REVIEW_TUTOR_CHILD: "1", + }, + }); + expect(spawn.mock.calls[0]?.[2]).not.toMatchObject({ + env: { OPENAI_API_KEY: expect.anything() }, + }); + }); + + it("settles once when cancelled mid-run", async () => { + const child = new FakeChild(); + const terminate = vi.fn(); + const connector = new CodexConnector(); + const runner = new TutorRunner({ spawn: () => child as never, terminate }); + const done = runner.run({ ...request, connector }, () => {}); + runner.cancel(); + child.emit("close", null, "SIGTERM"); + await expect(done).rejects.toThrow(/cancelled/); + expect(terminate).toHaveBeenCalledTimes(1); + }); + + it("keeps answers per run on a shared connector", async () => { + const connector = new CodexConnector(); + const firstChild = new FakeChild(); + const secondChild = new FakeChild(); + const children = [firstChild, secondChild]; + const runner = new TutorRunner({ spawn: () => children.shift() as never }); + + const first = runner.run({ ...request, connector }, () => {}); + firstChild.stdout.end(`${JSON.stringify({ + type: "item.completed", + item: { type: "agent_message", text: "first" }, + })}\n${JSON.stringify({ type: "turn.completed" })}\n`); + firstChild.emit("close", 0, null); + await expect(first).resolves.toEqual({ answer: "first" }); + + const second = runner.run({ ...request, connector }, () => {}); + secondChild.stdout.end(`${JSON.stringify({ type: "turn.completed" })}\n`); + secondChild.emit("close", 0, null); + await expect(second).rejects.toThrow("Codex returned an empty answer."); + }); + + it("redacts Codex stderr failures", async () => { + const child = new FakeChild(); + const connector = new CodexConnector(); + const runner = new TutorRunner({ spawn: () => child as never }); + const done = runner.run({ ...request, connector }, () => {}); + child.stderr.end("sk-proj-fakefakefake"); + child.emit("close", 1, null); + await expect(done).rejects.toThrow(/\[redacted\]/); + await expect(done).rejects.not.toThrow(/sk-proj-fakefakefake/); + }); +}); + +describe("Codex registry", () => { + it("registers Codex only with the flag and preserves unavailable discovery", async () => { + const off = createConnectorRegistry({ + flags: createReviewTutorFlags({ get: () => false, set: () => {} }), + piModels, + }); + const on = createConnectorRegistry({ + flags: createReviewTutorFlags({ get: () => true, set: () => {} }), + piModels, + execFile: fakeDiscovery({ + "--version": Object.assign(new Error("spawn codex ENOENT"), { code: "ENOENT" }), + }).execFile, + }); + expect(off.connectors().map(({ id }) => id)).toEqual(["pi"]); + expect(on.connectors().map(({ id }) => id)).toEqual(["pi", "claude-code", "codex"]); + const discoveries = await on.discoveries(); + expect(discoveries.find(({ connector }) => connector.id === "codex")).toEqual({ + connector: on.byId("codex"), + discovery: { + available: false, + reason: "Codex is not installed (codex not found on PATH).", + }, + }); + }); +}); diff --git a/packages/review-tutor/test/connectors.test.ts b/packages/review-tutor/test/connectors.test.ts index ac86eed..6ac0eb9 100644 --- a/packages/review-tutor/test/connectors.test.ts +++ b/packages/review-tutor/test/connectors.test.ts @@ -55,9 +55,9 @@ describe("connector registry", () => { } }); - it("registers Claude Code only when the connector flag is on", () => { + it("registers optional connectors only when the flag is on", () => { expect(registry(false).connectors().map((connector) => connector.id)).toEqual(["pi"]); - expect(registry(true).connectors().map((connector) => connector.id)).toEqual(["pi", "claude-code"]); + expect(registry(true).connectors().map((connector) => connector.id)).toEqual(["pi", "claude-code", "codex"]); }); it("resolves namespaced and legacy Pi ids and rejects unknown harnesses", () => { @@ -72,6 +72,8 @@ describe("connector registry", () => { model: "ollama/qwen3:8b", }); expect(registry().resolve("codex:x")).toBeUndefined(); + expect(registry(true).resolve("codex:x")).toMatchObject({ model: "x" }); + expect(registry().resolve("unknown:x")).toBeUndefined(); }); it("namespaces Pi discovery without spawning a process", async () => { @@ -96,6 +98,11 @@ describe("connector failure boundary", () => { ["token=value", "[redacted]"], ["ghp_abcdefgh", "[redacted]"], ["gho_abcdefgh", "[redacted]"], + ["url: https://provider.invalid/path", "[redacted]"], + ["cf-ray: ray_fixture", "[redacted]"], + ["request id: req_fixture", "[redacted]"], + ["thread_id: thread_fixture", "[redacted]"], + ["thread id thread_fixture", "[redacted]"], ])("redacts %s", (input, expected) => { expect(redact(input)).toBe(expected); }); diff --git a/packages/review-tutor/test/fixtures/codex/empty.jsonl b/packages/review-tutor/test/fixtures/codex/empty.jsonl new file mode 100644 index 0000000..433ee35 --- /dev/null +++ b/packages/review-tutor/test/fixtures/codex/empty.jsonl @@ -0,0 +1,2 @@ +{"type":"turn.started"} +{"type":"turn.completed","usage":{"input_tokens":2,"output_tokens":0}} diff --git a/packages/review-tutor/test/fixtures/codex/incomplete.jsonl b/packages/review-tutor/test/fixtures/codex/incomplete.jsonl new file mode 100644 index 0000000..ec2b4a9 --- /dev/null +++ b/packages/review-tutor/test/fixtures/codex/incomplete.jsonl @@ -0,0 +1,2 @@ +{"type":"turn.started"} +{"type":"item.completed","item":{"type":"agent_message","text":"partial"}} diff --git a/packages/review-tutor/test/fixtures/codex/models.json b/packages/review-tutor/test/fixtures/codex/models.json new file mode 100644 index 0000000..87f6c3e --- /dev/null +++ b/packages/review-tutor/test/fixtures/codex/models.json @@ -0,0 +1,32 @@ +{ + "models": [ + { + "slug": "gpt-5.5", + "display_name": "", + "visibility": "list", + "priority": 20 + }, + { + "slug": "gpt-5.6-sol", + "display_name": "GPT-5.6 Sol", + "visibility": "list", + "supported_reasoning_levels": [ + { "effort": "low", "description": "Fast" }, + { "effort": "medium", "description": "Balanced" }, + { "effort": "high", "description": "Deep" }, + { "effort": "max", "description": "Maximum" }, + { "effort": "ultra", "description": "Prohibited" } + ], + "default_reasoning_level": "medium", + "priority": 10 + }, + { + "slug": "internal-model", + "display_name": "Internal", + "visibility": "hidden", + "supported_reasoning_levels": [{ "effort": "low", "description": "Fast" }], + "default_reasoning_level": "low", + "priority": 1 + } + ] +} diff --git a/packages/review-tutor/test/fixtures/codex/success.jsonl b/packages/review-tutor/test/fixtures/codex/success.jsonl new file mode 100644 index 0000000..cf492ba --- /dev/null +++ b/packages/review-tutor/test/fixtures/codex/success.jsonl @@ -0,0 +1,4 @@ +{"type":"thread.started","thread_id":"fixture-thread"} +{"type":"turn.started"} +{"type":"item.completed","item":{"type":"agent_message","text":"ok"}} +{"type":"turn.completed","usage":{"input_tokens":12,"output_tokens":3,"cached_input_tokens":4,"reasoning_output_tokens":2}} diff --git a/packages/review-tutor/test/fixtures/codex/unauthorized.jsonl b/packages/review-tutor/test/fixtures/codex/unauthorized.jsonl new file mode 100644 index 0000000..ea3159f --- /dev/null +++ b/packages/review-tutor/test/fixtures/codex/unauthorized.jsonl @@ -0,0 +1,3 @@ +{"type":"error","message":"Reconnecting 1/5 url: https://provider.invalid"} +{"type":"error","message":"Reconnecting 2/5 request id: req_fixture"} +{"type":"turn.failed","error":{"message":"401 unauthorized url: https://provider.invalid request id: req_fixture"}} diff --git a/packages/review-tutor/test/server-extension.test.ts b/packages/review-tutor/test/server-extension.test.ts index 25c3a65..37bd49f 100644 --- a/packages/review-tutor/test/server-extension.test.ts +++ b/packages/review-tutor/test/server-extension.test.ts @@ -325,6 +325,7 @@ describe("connector protocol boundary", () => { flags: createReviewTutorFlags({ get: () => true, set: () => {} }), piModels: [{ id: "provider/model", label: "Model", thinkingLevels: ["low"] }], which: async () => undefined, + execFile: async () => { throw Object.assign(new Error("spawn codex ENOENT"), { code: "ENOENT" }); }, }); const { server } = await start(new ControlledRunner(), { registry: connectorRegistry }); try {