diff --git a/.mcp.json.example b/.mcp.json.example index 25bec14..1dcecd9 100644 --- a/.mcp.json.example +++ b/.mcp.json.example @@ -6,7 +6,7 @@ "/ABSOLUTE/PATH/TO/codex-orchestrator/dist/src/mcp-server.js" ], "env": { - "CODEX_ORCHESTRATOR_MODE": "mcp-server", + "CODEX_ORCHESTRATOR_MODE": "exec", "CODEX_ORCHESTRATOR_MAX_RUNTIME_MS": "1800000" } } diff --git a/README.md b/README.md index 157298f..9a02a1f 100644 --- a/README.md +++ b/README.md @@ -83,10 +83,10 @@ reports progress. See [The /massa command](#the-massa-command-for-claude-code). # from a clean clone: npm install # installs pinned deps npm run build # compiles src/ + test/ to dist/ -npm test # builds, then runs the full test suite (13 tests) +npm test # builds, then runs the full test suite ``` -`npm test` output ends with `pass 13 / fail 0`. +`npm test` must finish with zero failures. --- @@ -129,7 +129,7 @@ Start an autonomous loop; returns a `run_id` and runs in the background. | `max_iterations` | int | default 10 | | `verification_commands` | string[] | default `[]` | | `sandbox` | enum | `read-only` \| `workspace-write` \| `danger-full-access`; default **`workspace-write`** | -| `approval_policy` | enum | `untrusted` \| `on-failure` \| `on-request` \| `never`; default **`never`** | +| `approval_policy` | enum | `on-request` \| `never`; default **`never`** | ### `codex_task` Send one prompt to Codex; returns its output + thread/session id. Outgoing @@ -187,7 +187,7 @@ Replace the absolute path. A copy is in [`.mcp.json.example`](.mcp.json.example) "/ABSOLUTE/PATH/TO/codex-orchestrator/dist/src/mcp-server.js" ], "env": { - "CODEX_ORCHESTRATOR_MODE": "mcp-server", + "CODEX_ORCHESTRATOR_MODE": "exec", "CODEX_ORCHESTRATOR_MAX_RUNTIME_MS": "1800000" } } @@ -199,7 +199,7 @@ Equivalent CLI registration: ```bash claude mcp add codex-orchestrator \ - -e CODEX_ORCHESTRATOR_MODE=mcp-server \ + -e CODEX_ORCHESTRATOR_MODE=exec \ -e CODEX_ORCHESTRATOR_MAX_RUNTIME_MS=1800000 \ -- node /ABSOLUTE/PATH/TO/codex-orchestrator/dist/src/mcp-server.js ``` @@ -246,8 +246,8 @@ Selected via `CODEX_ORCHESTRATOR_MODE` (MCP server) or `--mode` / `--dry-run` (C | Mode | Transport | Use | |---|---|---| | `mock` | none (scripted) | dry-run / tests; applies steps from `/mock-plan.json` (or an inline plan) | -| `mcp-server` | `codex mcp-server` over stdio (MCP SDK) | **primary** real path | -| `exec` | `codex exec --json` per call | fallback when MCP transport is impractical | +| `exec` | `codex exec --json` per call | **default** real path for Codex CLI 0.155+ | +| `mcp-server` | `codex mcp-server` over stdio (MCP SDK) | explicit legacy mode for older CLIs that still provide this subcommand | --- @@ -277,13 +277,17 @@ printenv OPENAI_API_KEY | codex login --with-api-key # API key via stdin ```bash codex --help -codex mcp-server --help # primary transport -codex exec --help # fallback transport +codex exec --help # default worker transport +codex exec resume --help # continuation support ``` +Only when explicitly selecting legacy `--mode mcp-server` on an older CLI, +check `codex mcp-server --help`. This subcommand was removed in Codex CLI 0.155; +it is separate from this package's own MCP server, which still exposes the five tools. + Config lives at `~/.codex/config.toml` (`CODEX_HOME` defaults to `~/.codex`). -The orchestrator passes `cwd`, `sandbox`, and `approval-policy` per call; you do -not need to pre-configure those. +The exec transport runs in the project directory and passes `sandbox_mode` and +`approval_policy` through per-call `-c` overrides; you do not need to pre-configure those. --- @@ -438,8 +442,8 @@ Resulting `final-report.md` (iteration log): ``` **Running it for real (with Codex):** drop the `--dry-run` flag and ensure Codex -is installed/authenticated. The loop will use `codex mcp-server`, threading the -conversation across iterations via the returned `threadId`. To exercise the real +is installed/authenticated. The loop will use `codex exec --json`, threading the +conversation across iterations via `codex exec resume `. To exercise the real `git diff` review on the sample, `git init` the target first: ```bash @@ -453,27 +457,40 @@ node dist/src/cli.js run --project-dir ./examples/sample-project --verify "node The Codex flag/tool surface drifts between versions. All version-dependent names are centralized in [`config.ts`](src/config.ts) (`DEFAULT_CODEX_CLI`) so they can -be adjusted in one place. As built (verified against the current Codex CLI): +be adjusted in one place. The default exec transport targets Codex CLI 0.155+: -- **Launch MCP:** `codex mcp-server` (over stdio). *Not* `codex mcp serve` +- **Legacy MCP only:** `codex mcp-server` (over stdio), on older CLIs that still + provide it. *Not* `codex mcp serve` (`codex mcp` is the separate "manage external servers" command). -- **MCP tools:** `codex` (start) and `codex-reply` (continue). The `codex` tool +- **Legacy MCP tools:** `codex` (start) and `codex-reply` (continue). The `codex` tool takes **kebab-case** args (`prompt`, `cwd`, `sandbox`, `approval-policy`) and **rejects unknown fields**; `codex-reply` takes **camelCase** `{ threadId, prompt }`. -- **Session id:** returned in `structuredContent.threadId`; reused as `threadId`. -- **exec fallback:** `codex exec --json ""`; resume with +- **Legacy MCP session id:** returned in `structuredContent.threadId`; reused as `threadId`. +- **Default exec:** `codex exec --json -c 'sandbox_mode="workspace-write"' -c 'approval_policy="never"' ""`; resume with `codex exec resume `. The JSONL stream is tagged by dotted `type` (`thread.started` carries `thread_id`; the final answer is in an `item.completed` whose `item.type === "agent_message"`, field `text`). A legacy `EventMsg` fallback parser is included. - **Sandbox values:** `read-only` | `workspace-write` | `danger-full-access`. -- **Approval values:** `untrusted` | `on-failure` | `on-request` | `never`. +- **Approval values:** `on-request` | `never`. Retired `untrusted` and + `on-failure` values are rejected before execution, including in legacy mode. + Codex's granular approval objects are not exposed by this package's string API. +- **Exec failures:** nonzero exits, signals, and timeouts fail the task even if + stdout contains a partial reply. CLI tasks exit nonzero with diagnostics; MCP + tasks return `status: "error"`. Loops retain the error as a gap and cannot + complete that iteration merely because existing verification checks pass. - **Guarded:** `--dangerously-bypass-approvals-and-sandbox` / `--yolo` and `--sandbox danger-full-access` are never emitted. If a future Codex renames a tool or flag, edit `DEFAULT_CODEX_CLI` — no other code changes are needed. +To check the supported string policies against a real CLI without credentials or +a model call, run `node scripts/check-codex-policies.mjs [path/to/codex]`. +It uses an empty temporary `CODEX_HOME`, a read-only sandbox, and an intentionally +missing output schema to stop accepted policies before task execution; retired +policies must fail CLI argument validation. + --- ## Troubleshooting @@ -484,7 +501,7 @@ code changes are needed. server scans the *server's* cwd. Launch the MCP server from the project dir, or use the CLI `status` command with `--project-dir`. - **Real Codex run errors immediately** — run `codex login status` and - `codex mcp-server --help`; confirm the binary is on `PATH` (override with the + `codex exec --help`; confirm the binary is on `PATH` (override with the `CODEX_BIN` env var). - **Git diff shows "(not a git repository)"** — the diff-based guards/review need a git repo; `git init` the target. The checklist + verification still work diff --git a/scripts/check-codex-policies.mjs b/scripts/check-codex-policies.mjs new file mode 100644 index 0000000..1dfb6ed --- /dev/null +++ b/scripts/check-codex-policies.mjs @@ -0,0 +1,36 @@ +// Validate the real CLI argument parser, stopping before authentication or a model call. +import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; + +const bin = process.argv[2] ?? process.env.CODEX_BIN ?? 'codex'; +const home = mkdtempSync(join(tmpdir(), 'massa-policy-')); +try { + const env = { ...process.env, CODEX_HOME: home }; + const version = spawnSync(bin, ['--version'], { env, encoding: 'utf8', timeout: 10000 }); + assert.equal(version.status, 0, version.stderr || String(version.error)); + console.log(version.stdout.trim()); + for (const policy of ['never', 'on-request', 'untrusted', 'on-failure']) { + const result = spawnSync(bin, [ + '--ask-for-approval', policy, 'exec', '--ignore-user-config', '--ephemeral', + '--sandbox', 'read-only', '--output-schema', join(home, 'missing.json'), + '--json', 'policy validation only', + ], { env, cwd: home, input: '', encoding: 'utf8', timeout: 10000 }); + assert.ifError(result.error); + if (policy === 'never' || policy === 'on-request') { + // The deliberately absent schema stops execution after argument parsing. + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /Failed to read output schema file/); + console.log(`${policy}: accepted; stopped at missing schema before task execution`); + } else { + assert.equal(result.status, 2, result.stderr); + assert.match(result.stderr, /invalid value/); + assert.match(result.stderr, /possible values: on-request, never/); + console.log(`${policy}: rejected by CLI policy parser`); + } + } +} finally { + rmSync(home, { recursive: true, force: true }); +} diff --git a/src/codex-client.ts b/src/codex-client.ts index 933fe54..59906a1 100644 --- a/src/codex-client.ts +++ b/src/codex-client.ts @@ -3,9 +3,9 @@ * * MockCodexClient — no network, deterministic; applies a scripted plan of * file writes/deletes so the full loop can be proven. - * McpCodexClient — PRIMARY path: drives `codex mcp-server` over stdio using + * McpCodexClient — LEGACY path: drives `codex mcp-server` over stdio using * the official MCP SDK client. - * ExecCodexClient — FALLBACK only: shells out to `codex exec --json` and + * ExecCodexClient — DEFAULT: shells out to `codex exec --json` and * parses the JSONL event stream. * * All version-dependent Codex names (subcommands, tool names, flags) come from @@ -20,6 +20,7 @@ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; import { DEFAULT_CODEX_CLI, + validateSafetyConfig, type CodexCliConfig, } from './config.js'; import { runCommand } from './verifier.js'; @@ -256,7 +257,7 @@ export class McpCodexClient implements CodexClient { } // ========================================================================= -// exec --json (fallback) +// exec --json (default) // ========================================================================= /** @@ -332,6 +333,7 @@ export class ExecCodexClient implements CodexClient { async runTask(input: CodexTaskInput): Promise { const cwd = path.resolve(input.projectDir); + validateSafetyConfig(input.sandbox, input.approvalPolicy); // First turn: codex exec --json -c sandbox_mode=... -c approval_policy=... "" // Continuation: codex exec resume --json -c ... "" // `resume` MUST come immediately after `exec`. Sandbox and approval go in as @@ -351,8 +353,10 @@ export class ExecCodexClient implements CodexClient { const command = parts.join(' '); const res = await runCommand(command, cwd, input.timeoutMs); - if (res.error && !res.stdout) { - throw new Error(`codex exec failed: ${res.error}`); + if (res.error || res.exitCode !== 0) { + const reason = res.error ?? `exit code ${res.exitCode ?? 'unknown'}`; + const diagnostics = (res.stderr || res.stdout).trim(); + throw new Error(`codex exec failed: ${reason}${diagnostics ? `\n${diagnostics}` : ''}`); } const { text, threadId } = parseExecJsonl(res.stdout); return { diff --git a/src/config.ts b/src/config.ts index 2b50840..698ea33 100644 --- a/src/config.ts +++ b/src/config.ts @@ -55,7 +55,7 @@ export interface CodexCliConfig { mcpStartTool: string; /** Tool name to CONTINUE a Codex conversation. */ mcpReplyTool: string; - /** Subcommand for one-shot non-interactive runs (exec fallback). */ + /** Subcommand for one-shot non-interactive runs (default exec transport). */ execArgs: string[]; /** Flag that switches `codex exec` to JSONL event output. */ execJsonFlag: string; @@ -88,11 +88,16 @@ export function validateSafetyConfig( sandbox: SandboxMode, approvalPolicy: ApprovalPolicy, ): void { + if (approvalPolicy !== 'on-request' && approvalPolicy !== 'never') { + throw new ConfigError( + `Unsupported approval_policy="${approvalPolicy}"; use "on-request" or "never" (Codex CLI 0.155+).`, + ); + } if (sandbox === DANGER_SANDBOX && approvalPolicy === 'never') { throw new ConfigError( `Refusing to run with sandbox="${DANGER_SANDBOX}" and approval_policy="never": ` + `full filesystem access unattended is not allowed. Use sandbox="workspace-write" ` + - `or set approval_policy to "on-request"/"untrusted" and supervise the run.`, + `or set approval_policy to "on-request" and supervise the run.`, ); } } diff --git a/src/loop.ts b/src/loop.ts index c20c468..91baa86 100644 --- a/src/loop.ts +++ b/src/loop.ts @@ -320,6 +320,11 @@ export class LoopController { verification, diff, ); + // Existing passing checks cannot turn a failed worker call into completion. + if (taskError) { + reviewResult.complete = false; + reviewResult.remainingGaps.push(`Codex task failed: ${taskError}`); + } this.state.checklist = checklist; await this.store.writeChecklist(this.state.goalFile, checklist); await this.store.appendEvent('review', iteration, 'review complete', reviewResult); diff --git a/src/mcp-server.ts b/src/mcp-server.ts index 41cced8..970d092 100644 --- a/src/mcp-server.ts +++ b/src/mcp-server.ts @@ -110,7 +110,7 @@ export function buildServer(): McpServer { .enum(['read-only', 'workspace-write', 'danger-full-access']) .default(DEFAULT_SANDBOX), approval_policy: z - .enum(['untrusted', 'on-failure', 'on-request', 'never']) + .enum(['on-request', 'never']) .default(DEFAULT_APPROVAL_POLICY), }, }, diff --git a/src/types.ts b/src/types.ts index 4a03826..7b30d2f 100644 --- a/src/types.ts +++ b/src/types.ts @@ -9,8 +9,8 @@ /** Codex sandbox modes (exact strings accepted by the Codex CLI `--sandbox`). */ export type SandboxMode = 'read-only' | 'workspace-write' | 'danger-full-access'; -/** Codex approval policies (exact strings accepted by `--ask-for-approval`). */ -export type ApprovalPolicy = 'untrusted' | 'on-failure' | 'on-request' | 'never'; +/** Supported string approval policies for Codex CLI 0.155+ and legacy MCP. */ +export type ApprovalPolicy = 'on-request' | 'never'; /** Which transport the Codex client is using. */ export type CodexMode = 'mock' | 'mcp-server' | 'exec'; diff --git a/test/exec.test.ts b/test/exec.test.ts new file mode 100644 index 0000000..747e948 --- /dev/null +++ b/test/exec.test.ts @@ -0,0 +1,120 @@ +import test from 'node:test'; +import assert from 'node:assert/strict'; +import { promises as fs } from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; +import { fileURLToPath } from 'node:url'; +import { ExecCodexClient } from '../src/codex-client.js'; +import { DEFAULT_CODEX_CLI, validateSafetyConfig } from '../src/config.js'; +import type { ApprovalPolicy, CodexTaskInput } from '../src/types.js'; +import { LoopController, newRunId } from '../src/loop.js'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; + +const exec = promisify(execFile); +const cliPath = fileURLToPath(new URL('../src/cli.js', import.meta.url)); +const serverPath = fileURLToPath(new URL('../src/mcp-server.js', import.meta.url)); + +async function fixture(t: test.TestContext, body: string) { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'massa-exec-')); + t.after(() => fs.rm(dir, { recursive: true, force: true })); + const bin = path.join(dir, 'fake-codex'); + await fs.writeFile(bin, `#!/bin/sh\n${body}\n`, { mode: 0o755 }); + const client = new ExecCodexClient({ ...DEFAULT_CODEX_CLI, bin }); + const input: CodexTaskInput = { + projectDir: dir, prompt: 'Say hello', sandbox: 'workspace-write', + approvalPolicy: 'never', timeoutMs: 2000, + }; + return { dir, bin, client, input }; +} + +const message = JSON.stringify({ type: 'item.completed', item: { type: 'agent_message', text: 'hello' } }); + +test('exec rejects stderr-only nonzero exit and preserves its diagnostics', async (t) => { + const { client, input } = await fixture(t, 'echo "fake configuration failure" >&2\nexit 42'); + await assert.rejects(client.runTask(input), /codex exec failed.*42.*fake configuration failure/s); +}); + +test('exec rejects a nonzero exit even after an assistant message', async (t) => { + const { client, input } = await fixture(t, `echo '${message}'\necho 'partial failure' >&2\nexit 7`); + await assert.rejects(client.runTask(input), /codex exec failed.*7.*partial failure/s); +}); + +test('exec rejects a signal termination', async (t) => { + const { client, input } = await fixture(t, 'echo terminated >&2\nkill -TERM $$'); + await assert.rejects(client.runTask(input), /codex exec failed/); +}); + +test('exec rejects timeout even after partial output', async (t) => { + const { client, input } = await fixture(t, `echo '${message}'\nexec sleep 5`); + await assert.rejects(client.runTask({ ...input, timeoutMs: 100 }), /timed out/); +}); + +test('exec retains successful structured output and resume arguments', async (t) => { + const thread = JSON.stringify({ type: 'thread.started', thread_id: 'thread-123' }); + const { client, input } = await fixture(t, `printf '%s\\n' "$@" >&2\necho '${thread}'\necho '${message}'`); + const first = await client.runTask(input); + assert.equal(first.output, 'hello'); + assert.equal(first.threadId, 'thread-123'); + const next = await client.runTask({ ...input, threadId: first.threadId }); + assert.equal(next.output, 'hello'); + assert.match((next.raw as { stderr: string }).stderr, /^exec\nresume\nthread-123\n--json\n/); + assert.match((next.raw as { stderr: string }).stderr, /approval_policy="never"/); +}); + +test('exec retains successful legacy structured output', async (t) => { + const legacy = JSON.stringify({ msg: { type: 'agent_message', message: 'legacy hello' } }); + const { client, input } = await fixture(t, `echo '${legacy}'`); + assert.equal((await client.runTask(input)).output, 'legacy hello'); +}); + +test('CLI task exits nonzero and never reports ok for a failed worker', async (t) => { + const { dir, bin } = await fixture(t, 'echo "fake configuration failure" >&2\nexit 42'); + await assert.rejects(exec(process.execPath, [cliPath, 'task', '--project-dir', dir, '--prompt', 'Say hello'], { + env: { ...process.env, CODEX_BIN: bin }, + }), (err: any) => { + assert.equal(err.code, 1); + assert.match(err.stderr, /42.*fake configuration failure/s); + assert.doesNotMatch(err.stdout, /"status": "ok"/); + return true; + }); +}); + +test('MCP task returns error with worker diagnostics', async (t) => { + const { dir, bin } = await fixture(t, 'echo "fake configuration failure" >&2\nexit 42'); + const transport = new StdioClientTransport({ command: process.execPath, args: [serverPath], + env: { ...process.env, CODEX_BIN: bin, CODEX_ORCHESTRATOR_MODE: 'exec' } }); + const client = new Client({ name: 'exec-regression', version: '1' }); + await client.connect(transport); + t.after(() => client.close()); + const result = await client.callTool({ name: 'codex_task', arguments: { project_dir: dir, prompt: 'Say hello' } }); + const content = result.content as Array<{ type: string; text?: string }>; + const obj = JSON.parse(content.find(c => c.type === 'text')!.text!); + assert.equal(obj.status, 'error'); + assert.match(obj.error, /42.*fake configuration failure/s); +}); + +test('loop cannot complete after worker failure even if verification passes', async (t) => { + const { dir, client } = await fixture(t, 'echo "fake configuration failure" >&2\nexit 42'); + await fs.writeFile(path.join(dir, 'PROJECT_GOAL.md'), '# Goal\nSay hello'); + const controller = new LoopController(newRunId(), { + projectDir: dir, goalFile: 'PROJECT_GOAL.md', maxIterations: 1, maxRuntimeMs: 5000, + verificationCommands: ['true'], sandbox: 'workspace-write', approvalPolicy: 'never', codexMode: 'exec', + }, client); + await controller.init(); + const state = await controller.runToCompletion(); + assert.equal(state.status, 'limit_reached'); + assert.equal(state.iterations[0]!.review!.complete, false); + assert.match(state.iterations[0]!.review!.remainingGaps.join('\n'), /42.*fake configuration failure/s); +}); + +test('approval validation rejects retired and unknown values before execution', () => { + for (const policy of ['untrusted', 'on-failure', 'invalid']) { + assert.throws(() => validateSafetyConfig('workspace-write', policy as ApprovalPolicy), /approval.*on-request.*never/i); + } + for (const policy of ['on-request', 'never'] as const) { + assert.doesNotThrow(() => validateSafetyConfig('workspace-write', policy)); + } +}); diff --git a/test/mcp.test.ts b/test/mcp.test.ts index 95029c6..7f5e46b 100644 --- a/test/mcp.test.ts +++ b/test/mcp.test.ts @@ -33,6 +33,8 @@ test('server registers all 5 tools with the exact specified input names', async assert.ok(byName.has(n), `missing tool: ${n}`); } assert.equal(tools.length, 5, 'exactly five tools must be registered'); + const policySchema = (byName.get('start_project_loop')!.inputSchema as any).properties.approval_policy; + assert.deepEqual(policySchema.enum, ['on-request', 'never']); const props = (name: string) => Object.keys((byName.get(name)!.inputSchema as { properties?: object }).properties ?? {});