fix: recover structured output for /codex:adversarial-review when the turn uses tools#407
fix: recover structured output for /codex:adversarial-review when the turn uses tools#407qinjie wants to merge 2 commits into
Conversation
…ses tools /codex:adversarial-review fails with "Unexpected token ... in JSON at position 0" on every non-trivial review. When Codex investigates the diff before answering, codex-cli 0.142.3 ends the outputSchema turn without emitting the forced final_answer, so the companion captures a phase:"commentary" preamble and parseStructuredOutput's bare JSON.parse throws. Plain /codex:review is unaffected (native reviewer, no schema). Fix, in plugins/codex/scripts/lib/codex.mjs: - Add an opt-in structuredRetryPrompt to runAppServerTurn. If a schema was requested but the captured message isn't JSON once the turn completes, resume the same thread with a JSON-only prompt (retry budget structuredRetryAttempts, default 3). Same-thread resume retains the investigation context, so the model emits the verdict it skipped. The retry is guarded by !looksLikeJson(finalMessage), so it never fires once the CLI emits final_answer directly (forward-compatible no-op). - Make parseStructuredOutput tolerate a fenced ```json block. The adversarial-review call site in codex-companion.mjs passes the prompt. The task path and /codex:review are untouched. Tests: a new fake-codex behavior (adversarial-tooluse-then-json) reproduces the 0.142.3 protocol (commentary + command, no final_answer; structured answer only on the follow-up turn), plus a runtime test asserting the review renders a verdict instead of the parse error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d87a86dcc9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| { onProgress: options.onProgress } | ||
| ); | ||
| reasoningSummary = mergeReasoningSections(reasoningSummary, retryState.reasoningSummary); | ||
| finalTurn = retryState.finalTurn ?? finalTurn; |
There was a problem hiding this comment.
Preserve failures from the investigation turn
When the first schema turn ends as failed or interrupted after producing only a commentary message, this retry path still runs and then replaces the original failed finalTurn with the retry turn. If that JSON-only retry completes, the later status calculation reports success, so /codex:adversarial-review can render a normal verdict and exit 0 even though the investigation turn actually failed. Please only retry after a completed first turn, or keep the overall status non-zero whenever turnState.finalTurn.status was not completed.
Useful? React with 👍 / 👎.
Four tests failed whenever the suite ran inside a Claude Code / Codex
session that exports CODEX_COMPANION_SESSION_ID or CLAUDE_PLUGIN_DATA.
They pass in clean CI, so the failures were invisible until now.
- The three status/result tests invoked the companion with only `cwd`,
so the child inherited CODEX_COMPANION_SESSION_ID from the host shell.
filterJobsForCurrentSession then kept only jobs matching that session
id, but the fixtures carry no sessionId, so every job was dropped and
the command reported "No jobs recorded yet." They now pass
`env: { ...process.env, CODEX_COMPANION_SESSION_ID: undefined }`, the
same scrubbing the session-scoped sibling tests already rely on (Node
omits an undefined-valued env entry).
- "resolveStateDir uses a temp-backed per-workspace directory" calls
resolveStateDir in-process, so ambient CLAUDE_PLUGIN_DATA redirected
the path away from os.tmpdir() and broke the assertion. It now
save/deletes/restores the var, matching the guard the sibling
CLAUDE_PLUGIN_DATA test already uses.
No product code touched. Full suite is now 91/91 whether or not those
vars are present in the environment.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Problem
/codex:adversarial-reviewfails withUnexpected token … in JSON at position 0on every non-trivial review. When Codex investigates the diff before answering, codex-cli 0.142.3 ends theoutputSchematurn without emitting the forcedfinal_answer, so the companion captures aphase: "commentary"preamble andparseStructuredOutput's bareJSON.parsethrows.Plain
/codex:reviewis unaffected — it uses the native reviewer (exitedReviewMode→reviewText), nooutputSchema, noJSON.parse.What the user sees
Turn duration is suspiciously short (~9–12s): Codex investigates, the turn completes, but no structured
final_answeris ever emitted.Root cause
Driving
codex app-serverdirectly (initialize →thread/start→turn/startwith anoutputSchemathat has averdictproperty) shows:item/completed { agentMessage, phase:"final_answer", text:'{"verdict":"approve",…}' }✅ schema works.agentMessage phase=commentary "I'll read…"→ severalcommandExecutionitems →turn/completedwith nofinal_answer❌.So this is a codex-cli behavior (forced-schema output dropped when one turn mixes tool calls +
outputSchema), but the plugin makes it fatal becauserecordItem()setslastAgentMessagefor everyagentMessage(including non-finalcommentary), andparseStructuredOutputhard-fails on prose.Fix
In
plugins/codex/scripts/lib/codex.mjs:runAppServerTurn— new opt-instructuredRetryPromptoption. If a schema was requested but the captured message isn't JSON once the turn completes, resume the same thread with a JSON-only prompt (retry budgetstructuredRetryAttempts, default 3). Same-thread resume retains the investigation context, so the model emits the verdict it skipped. Guarded by!looksLikeJson(finalMessage), so it never fires once the CLI emitsfinal_answerdirectly — a forward-compatible no-op once the upstream CLI bug is fixed.parseStructuredOutputtolerates a fenced ```json block via a smallstripJsonFenceshelper (also used by `looksLikeJson`).In
plugins/codex/scripts/codex-companion.mjs:structuredRetryPrompt. Thetaskpath and/codex:revieware untouched.Tests
tests/fake-codex-fixture.mjs: newadversarial-tooluse-then-jsonbehavior that, on the firstoutputSchematurn, emits acommentarypreamble + acommandExecutionand completes with nofinal_answer; the follow-up JSON-only turn emits the structuredfinal_answer. This reproduces the 0.142.3 protocol exactly.tests/runtime.test.mjs: assertsadversarial-reviewexits 0 and rendersVerdict:(not "did not return valid structured JSON") under that behavior.Verified fail-before / pass-after: the new test fails on
mainwith the exact parse error and passes with this change. The rest of the suite's pass/fail set is unchanged by this PR.Risk / scope
structuredRetryPrompt; thetaskpath and/codex:revieware untouched.final_answerafter tool use.🤖 Generated with Claude Code