feat(agent): structured output for codex adapter via stdio MCP#1885
feat(agent): structured output for codex adapter via stdio MCP#1885ryans-posthog wants to merge 3 commits intomainfrom
Conversation
Codex has no native equivalent of Claude's outputFormat. When a task has json_schema + an onStructuredOutput callback, inject an in-process stdio MCP server exposing a create_output tool that validates against the schema with AJV, and watch for a completed tool_call_update on the ACP stream to fire onStructuredOutput exactly once. Path resolution walks up from import.meta.dirname so the compiled MCP script is found regardless of which bundle entry imports this module (dist/agent.js, dist/server/bin.cjs, harness bundles). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/agent/src/adapters/codex/codex-client.ts
Line: 63
Comment:
**Permissive `includes` match can collide with user-defined tools**
`title.includes("create_output")` will match any tool whose name contains that substring (e.g. `my_create_output`, `mcp__vendor__create_output_report`, etc.). When codex-acp uses the prefixed form `mcp__posthog_output__create_output`, the server name `posthog_output` is available in the title too. A more defensive check would also require the known server name to be present so an unrelated user tool named `create_output` doesn't silently trigger `onStructuredOutput` with the wrong payload.
```suggestion
return (
title.includes("posthog_output") && title.includes(STRUCTURED_OUTPUT_TOOL_NAME)
) || title === STRUCTURED_OUTPUT_TOOL_NAME;
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/agent/src/adapters/codex/codex-client.ts
Line: 53
Comment:
**`STRUCTURED_OUTPUT_TOOL_NAME` duplicated across three files**
`"create_output"` is declared as `STRUCTURED_OUTPUT_TOOL_NAME` here, as `STRUCTURED_OUTPUT_TOOL_NAME` (exported) in `codex-agent.ts`, and as `OUTPUT_TOOL_NAME` in `structured-output-mcp-server.ts`. This is a OnceAndOnlyOnce violation — a single constant change now requires three edits. `codex-client.ts` could import the exported constant from `codex-agent.ts`, and the MCP server (which is a separate bundle entry) can either duplicate it or be refactored to share a tiny constants file.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/agent/src/adapters/codex/structured-output-mcp-server.ts
Line: 51-64
Comment:
**Redundant AJV validation — Zod already validates before the handler is called**
`McpServer.tool()` parses and validates the incoming arguments against the Zod schema before invoking the handler, so by the time `validate(args)` runs, AJV re-validates an already-valid object. If the motivation is richer error messages, the Zod errors are available via `safeParse`. The AJV pass is a superfluous part that doubles the schema compilation cost on every server start and adds an extra dependency path.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/agent/src/adapters/codex/structured-output-mcp-server.ts
Line: 19-37
Comment:
**Silent `process.exit(1)` calls leave no diagnostic trace**
All three early-exit paths exit without printing anything to stderr. If codex-acp spawns this server and it immediately dies with exit code 1, the parent has no way to distinguish a missing env var from a bad schema or an unsupported schema type. Even a single `process.stderr.write(...)` line before each exit would make failures much easier to diagnose in production.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(agent): structured output for codex..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| */ | ||
| function isStructuredOutputToolCall(title: string | undefined | null): boolean { | ||
| if (!title) return false; | ||
| return title.includes(STRUCTURED_OUTPUT_TOOL_NAME); |
There was a problem hiding this comment.
includes match can collide with user-defined tools
title.includes("create_output") will match any tool whose name contains that substring (e.g. my_create_output, mcp__vendor__create_output_report, etc.). When codex-acp uses the prefixed form mcp__posthog_output__create_output, the server name posthog_output is available in the title too. A more defensive check would also require the known server name to be present so an unrelated user tool named create_output doesn't silently trigger onStructuredOutput with the wrong payload.
| return title.includes(STRUCTURED_OUTPUT_TOOL_NAME); | |
| return ( | |
| title.includes("posthog_output") && title.includes(STRUCTURED_OUTPUT_TOOL_NAME) | |
| ) || title === STRUCTURED_OUTPUT_TOOL_NAME; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/codex/codex-client.ts
Line: 63
Comment:
**Permissive `includes` match can collide with user-defined tools**
`title.includes("create_output")` will match any tool whose name contains that substring (e.g. `my_create_output`, `mcp__vendor__create_output_report`, etc.). When codex-acp uses the prefixed form `mcp__posthog_output__create_output`, the server name `posthog_output` is available in the title too. A more defensive check would also require the known server name to be present so an unrelated user tool named `create_output` doesn't silently trigger `onStructuredOutput` with the wrong payload.
```suggestion
return (
title.includes("posthog_output") && title.includes(STRUCTURED_OUTPUT_TOOL_NAME)
) || title === STRUCTURED_OUTPUT_TOOL_NAME;
```
How can I resolve this? If you propose a fix, please make it concise.| onStructuredOutput?: (output: Record<string, unknown>) => Promise<void>; | ||
| } | ||
|
|
||
| const STRUCTURED_OUTPUT_TOOL_NAME = "create_output"; |
There was a problem hiding this comment.
STRUCTURED_OUTPUT_TOOL_NAME duplicated across three files
"create_output" is declared as STRUCTURED_OUTPUT_TOOL_NAME here, as STRUCTURED_OUTPUT_TOOL_NAME (exported) in codex-agent.ts, and as OUTPUT_TOOL_NAME in structured-output-mcp-server.ts. This is a OnceAndOnlyOnce violation — a single constant change now requires three edits. codex-client.ts could import the exported constant from codex-agent.ts, and the MCP server (which is a separate bundle entry) can either duplicate it or be refactored to share a tiny constants file.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/codex/codex-client.ts
Line: 53
Comment:
**`STRUCTURED_OUTPUT_TOOL_NAME` duplicated across three files**
`"create_output"` is declared as `STRUCTURED_OUTPUT_TOOL_NAME` here, as `STRUCTURED_OUTPUT_TOOL_NAME` (exported) in `codex-agent.ts`, and as `OUTPUT_TOOL_NAME` in `structured-output-mcp-server.ts`. This is a OnceAndOnlyOnce violation — a single constant change now requires three edits. `codex-client.ts` could import the exported constant from `codex-agent.ts`, and the MCP server (which is a separate bundle entry) can either duplicate it or be refactored to share a tiny constants file.
How can I resolve this? If you propose a fix, please make it concise.| const valid = validate(args); | ||
| if (!valid) { | ||
| const errors = validate.errors | ||
| ?.map((e) => `${e.instancePath || "/"}: ${e.message}`) | ||
| .join("; "); | ||
| return { | ||
| content: [ | ||
| { | ||
| type: "text" as const, | ||
| text: `Validation failed: ${errors}. Please fix the output and try again.`, | ||
| }, | ||
| ], | ||
| isError: true, | ||
| }; |
There was a problem hiding this comment.
McpServer.tool() parses and validates the incoming arguments against the Zod schema before invoking the handler, so by the time validate(args) runs, AJV re-validates an already-valid object. If the motivation is richer error messages, the Zod errors are available via safeParse. The AJV pass is a superfluous part that doubles the schema compilation cost on every server start and adds an extra dependency path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/codex/structured-output-mcp-server.ts
Line: 51-64
Comment:
**Redundant AJV validation — Zod already validates before the handler is called**
`McpServer.tool()` parses and validates the incoming arguments against the Zod schema before invoking the handler, so by the time `validate(args)` runs, AJV re-validates an already-valid object. If the motivation is richer error messages, the Zod errors are available via `safeParse`. The AJV pass is a superfluous part that doubles the schema compilation cost on every server start and adds an extra dependency path.
How can I resolve this? If you propose a fix, please make it concise.| const schemaEnv = process.env.POSTHOG_OUTPUT_SCHEMA; | ||
| if (!schemaEnv) { | ||
| process.exit(1); | ||
| } | ||
|
|
||
| let jsonSchema: Record<string, unknown>; | ||
| try { | ||
| jsonSchema = JSON.parse(Buffer.from(schemaEnv, "base64").toString("utf-8")); | ||
| } catch (_err) { | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const ajv = new Ajv({ allErrors: true }); | ||
| const validate = ajv.compile(jsonSchema); | ||
|
|
||
| const zodType = z.fromJSONSchema(jsonSchema); | ||
| if (!(zodType instanceof z.ZodObject)) { | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
process.exit(1) calls leave no diagnostic trace
All three early-exit paths exit without printing anything to stderr. If codex-acp spawns this server and it immediately dies with exit code 1, the parent has no way to distinguish a missing env var from a bad schema or an unsupported schema type. Even a single process.stderr.write(...) line before each exit would make failures much easier to diagnose in production.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/agent/src/adapters/codex/structured-output-mcp-server.ts
Line: 19-37
Comment:
**Silent `process.exit(1)` calls leave no diagnostic trace**
All three early-exit paths exit without printing anything to stderr. If codex-acp spawns this server and it immediately dies with exit code 1, the parent has no way to distinguish a missing env var from a bad schema or an unsupported schema type. Even a single `process.stderr.write(...)` line before each exit would make failures much easier to diagnose in production.
How can I resolve this? If you propose a fix, please make it concise.- Require both server and tool name when matching prefixed tool titles so unrelated user tools containing "create_output" can't trigger onStructuredOutput. - Hoist STRUCTURED_OUTPUT_MCP_NAME / STRUCTURED_OUTPUT_TOOL_NAME into a shared constants module so codex-agent, codex-client, and the spawned MCP server stay in sync. - Drop redundant AJV pass in the MCP server — McpServer.tool() already validates against the Zod shape derived from the schema. - Print a diagnostic to stderr before each early process.exit(1) so spawn failures aren't silent. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
joshsny
left a comment
joshsny
left a comment
There was a problem hiding this comment.
wasn't able to run this locally, so the review is low context, but assuming you've tested this locally on a cloud run lets merge and see what it's like in prod
| * up until we find the script so each bundle locates the shared dist asset. | ||
| */ | ||
| function resolveStructuredOutputMcpScript(): string { | ||
| const override = process.env.POSTHOG_STRUCTURED_OUTPUT_MCP_SCRIPT; |
There was a problem hiding this comment.
what's the override here for, thsi seems a bit odd versus just importing the default script but maybe i am missing something here?
There was a problem hiding this comment.
this would be nice to support as a general local MCP server that folks can extend with their own tools, rather than just for structured output, and then we can add the create_output tool here as the first one available - we'd want it available in claude too if we go that route, but we can do that in a later PR if someone asks for it
2 participants
Problem
The Codex adapter has no equivalent to Claude's
outputFormatfor structured output. Callers that pass_meta.jsonSchemathrough the agent server expecting a validated JSON payload back got plain-text completions instead — Codex simply ignored the schema.Changes
packages/agent/src/adapters/codex/structured-output-mcp-server.ts) that registers a singlecreate_outputtool whose input shape is generated from the caller-supplied JSON schema. AJV validates each invocation; invalid input is returned as a tool error so the model can retry.CodexAcpAgent.applyStructuredOutputinjects this MCP server intonewSession/loadSession/unstable_resumeSession/unstable_forkSessionwhenever_meta.jsonSchemais set and anonStructuredOutputcallback is wired. The schema is passed to the child process as a base64-encoded env var to avoid shell escaping. A short system-prompt note is appended via_meta.systemPromptinstructing the model to callcreate_outputwith the final result.createCodexClientinterceptstool_call/tool_call_updatenotifications: it capturesrawInputacross status transitions and firesonStructuredOutput(...)exactly once per tool-call id when status reachescompleted. State is cleared on fire so a re-emitted update can't double-fire.POSTHOG_STRUCTURED_OUTPUT_MCP_SCRIPToverrides the lookup.onStructuredOutputthroughacp-connection.ts→CodexAcpAgent→createCodexClient, and adds@modelcontextprotocol/sdkas a dependency.How did you test this?
pnpm --filter agent typecheckreports only pre-existing errors inenrichment/file-enricher.ts(same errors are present onmain, unrelated to this PR).codex-client.ts(one-shot fire acrosstool_call+tool_call_update) andapplyStructuredOutput's MCP-server / system-prompt injection are both unit-testable and would be worth covering in a follow-up — the existingcodex-client.test.tsalready mocks the upstream connection and is the natural place to add a test.