feat(agent): structured output for codex adapter via stdio MCP#1885
Open
ryans-posthog wants to merge 3 commits intomainfrom
Open
feat(agent): structured output for codex adapter via stdio MCP#1885ryans-posthog wants to merge 3 commits intomainfrom
ryans-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.
Suggested change
| 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.
Comment on lines
+51
to
+64
| 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.
Comment on lines
+19
to
+37
| 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.