Supersedes #408: thread model + effort overrides through app-server spawn path (fixes #476)#2
Conversation
…ns.model takes effect
When `runAppServerTurn` / `runAppServerReview` are invoked with a `model`
option, that value was only delivered via the `thread/start` and
`turn/start` requests. However, the spawn of `codex app-server` itself
did not include a `-c model="..."` override, so the codex CLI session
booted with the CLI's built-in default model. On a ChatGPT account where
the default (`gpt-5.3-codex`) is unsupported, every task launched via
`codex-companion.mjs task --background --model <m>` failed with:
```
Codex error: 'gpt-5.3-codex' model is not supported when using Codex
with a ChatGPT account.
```
even though the user passed `--model gpt-5.4`.
This change threads the model option through to the spawn site:
- `lib/app-server.mjs`: when `options.model` is set, add
`-c model=${JSON.stringify(model)}` to the spawned argv. JSON quoting
yields a valid TOML string literal that the codex CLI's `-c` parser
accepts unambiguously (e.g. `model="gpt-5.4"`).
- `lib/codex.mjs` `withAppServer`: accept a third `clientOptions`
argument and forward it to `CodexAppServerClient.connect` for both
the primary and the direct-retry connection.
- `lib/codex.mjs` `runAppServerReview` / `runAppServerTurn`: pass
`{ model: options.model }` through to `withAppServer`.
`interruptAppServerTurn` and `findLatestTaskThread` still create their
own short-lived clients without a model override; they are not exercised
by the failing `task --background --model` path and are left for a
follow-up.
Verified locally on macOS / codex-cli 0.130.0 with:
```
node companion.mjs task --background --fresh --model gpt-5.4 "say hi"
```
Before this patch the job failed during `turn/start`; after, the job
completes in ~7s with the assistant reply.
…kground tasks honor --model The 2026-06-30 fix (0531356) only covered the direct/fallback client path. The broker path used by task --background dropped options.model at ensureBrokerSession, so the broker's long-lived codex app-server child always started on the CLI default model. Thread model through connect -> ensureBrokerSession -> spawnBrokerProcess -> app-server-broker serve --model -> inner connect. RCA: 根因=broker 子プロセス起動時に model が渡っていなかった（6/30 修正は direct 経路のみ） 再発防止=~/.claude/patches/ registry を4ファイル版に更新・README に broker 経路の注意書き Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…pawn Mirrors the -c model= override (openai#408, hiirott) for effort using the codex config key `model_reasoning_effort`, threaded through the full spawn/broker chain: - SpawnedCodexAppServerClient.initialize() argv - connect() -> ensureBrokerSession - spawnBrokerProcess argv (--effort) + app-server-broker.mjs parseArgs - withAppServer clientOptions -> runAppServerReview/Turn RPC params for model/effort (thread/start, turn/start, review/start) are dead: the app-server (serde, no deny_unknown_fields) drops them, so the override must reach codex at spawn time. Verified end-to-end against codex-cli 0.144.1: resolved reasoning_effort in turn_context reflects the override. Co-Authored-By: hiirott <hiiroakitmail@gmail.com>
… to spawn Reuses the review-command plumbing from openai#477 (axisrow), but on the working spawn-override mechanism instead of the dead review/start RPC params: - handleReviewCommand parses --effort, normalizes via normalizeReasoningEffort (and normalizes --model via normalizeRequestedModel so `spark` -> gpt-5.3-codex-spark) - executeReviewRun forwards effort to runAppServerReview and runAppServerTurn review/start RPC params stay { threadId, delivery, target } (no model/effort) — the override reaches the server via the spawn argv. Closes the --effort half of openai#476 for both review and adversarial-review. Co-Authored-By: axisrow <axisrow@users.noreply.github.com>
Both flags now work via the app-server spawn override, so advertising them on /codexreview and /codex:adversarial-review is honest. Pins the argument-hint + "runtime-selection flags" prose in commands.test.mjs.
…erver The fake codex fixture previously discarded the app-server spawn argv, so no test could prove a -c override reached codex (the existing lastTurnStart/lastReviewStart RPC-param assertions are dead for this — the server drops RPC model/effort). Now persists lastAppServerSpawnArgs. Adds: - review --model/--effort reach spawn argv as -c overrides (load-bearing effectiveness proof, RED before the spawn-override fix, GREEN after) - differing --effort on a warm broker respawns it (gap-B fix; asserts appServerStarts === 2 and the override in the respawn argv) Also records lastReviewStart in the fixture (forwarding smoke test).
The stale-broker-respawn path called teardownBrokerSession with killProcess=null (connect() never passes it) and never sent a broker/shutdown RPC, so the detached+unref'd broker node process and its codex app-server child were orphaned indefinitely when a differing --model/--effort override triggered a respawn. Previously teardown only ran for a dead broker, so this leak was newly introduced. Mirror the session-lifecycle-hook pattern: send sendBrokerShutdown first (graceful — the broker closes its app-server child itself), then pass killProcess: terminateProcessTree as a SIGKILL fallback. Verified: across repeated review runs with differing overrides, no orphaned codex app-server processes accumulate. Co-Authored-By: Claude <noreply@anthropic.com>
🔍 Local review (cycle 1/3)Reviewed locally (Claude subagent + Codex companion), no bots pinged.
The FIX finding (critical, now fixed in
|
… hang sendBrokerShutdown awaited the broker's broker/shutdown response with no timeout. If the existing broker is wedged enough to accept the TCP/Unix connect but never processes the RPC, emits no data, and never closes the socket, the promise never resolves — so ensureBrokerSession's respawn path (and every command that needs a respawn) hangs indefinitely, turning a stale-broker cleanup into a hard hang. Add a bounded timeout (default 2s): on expiry, destroy the socket and resolve so the caller falls back to the terminateProcessTree kill. Co-Authored-By: Claude <noreply@anthropic.com>
🔍 Local review (cycle 2/3)Reviewed locally (Claude subagent + Codex companion) on the updated diff (after cycle-1's broker-leak fix).
The FIX finding (high → critical, now fixed in
|
Hardening follow-up to the respawn-path kill: only pass killProcess when we confirmed the existing broker's endpoint was ready (so the pid still belongs to our broker). For a stale session whose endpoint is not ready, the pid likely points at a dead broker whose pid the OS may have reused for an unrelated process — in that case just drop the socket/session files and let any survivor exit on its own. This narrows the PID-reuse surface on the respawn path while preserving the orphaned-process fix (the live-broker respawn case still tree-kills). Co-Authored-By: Claude <noreply@anthropic.com>
🔍 Local review (cycle 3/3) — clean (no FIX)Reviewed locally (Claude subagent + Codex companion) on the updated diff (after cycle-2's sendBrokerShutdown timeout fix).
Codex's PID-reuse finding → SKIP, plus a targeted hardening (
|
2 participants
Summary
Fixes the
--model/--effortflags being silently ignored on/codex:reviewand/codex:adversarial-review(openai#476), and supersedes openai#408 by @hiirott by carrying its commits forward onto currentmainand extending them.Root cause (verified against codex-cli 0.144.1)
model/effortin the JSON-RPC params (thread/start,turn/start,review/start) are dead: the Rust app-server deserializes without#[serde(deny_unknown_fields)], so unknown fields are silently dropped and the server boots with the CLI's built-in default. The only effective override is-c key=valueatcodex app-serverspawn time.Measured end-to-end by reading the resolved config from the session
turn_context:reasoning_effortlow(default)effortinturn/startRPClow(dropped — no effect)-c model_reasoning_effort=highat spawnhigh✅Same pattern for
model(-c model="gpt-5.4"→ resolvedgpt-5.4; RPC field → no effect).What this PR does
Carries openai#408's two commits (model-only spawn override) onto current
main(hiirott's branch was 18 commits behind, base8e403f9), with authorship preserved, then extends:effort(model_reasoning_effort) across the spawn/broker chain everywhere fix(app-server): pass-c model="..."tocodex app-serverso options.model takes effect openai/codex-plugin-cc#408 handlesmodel:SpawnedCodexAppServerClient.initialize()argv,connect()→ensureBrokerSession,spawnBrokerProcess(--effort),app-server-broker.mjsparseArgs + inner connect,withAppServerclientOptions →runAppServerReview/runAppServerTurn.--efforton review/adversarial-review (reusing the clean review-command plumbing from the closed Add model and effort flags to review commands openai/codex-plugin-cc#477, but on the working spawn mechanism instead of the deadreview/startRPC params).review/startparams stay{ threadId, delivery, target }.-c model="..."tocodex app-serverso options.model takes effect openai/codex-plugin-cc#408:ensureBrokerSessionnow stores the model/effort the broker was spawned with and respawns when a per-call override differs (previously a warm broker silently ignored later overrides).lastAppServerSpawnArgs), so tests prove the override actually reachescodex app-server— not merely that it was forwarded to a dead RPC param. The existing RPC-param assertions (lastTurnStart/lastReviewStart) remain as forwarding smoke tests.Why this supersedes both prior PRs
-c model="..."tocodex app-serverso options.model takes effect openai/codex-plugin-cc#408 (hiirott): correct mechanism, but model-only and 18 commits behindmain. Carried forward here with authorship intact, pluseffortand the broker gap fix.review/startRPC mechanism. Its reusable parts (parsing, normalization, forwarding, docs, test pins) are folded in here on top of the working spawn path.Verification
node --test tests/commands.test.mjs→ 8/8 ✅node --test tests/runtime.test.mjs→ new spawn-argv test (RED before the fix, GREEN after) + broker-respawn test both green; all existing broker-reuse tests still green (appServerStarts === 1reuse preserved). Pre-existing env-related failures in setup/status/result tests are unchanged frommain(Test suite is not hermetic inside a live Claude Code session: 4 failures at HEAD + fixture state leaks into the user's real plugin data dir openai/codex-plugin-cc#455 — suite is not hermetic) and unrelated to this change.turn_context:task --model gpt-5.4→ resolvedmodel: "gpt-5.4"✅task --effort high→ resolvedreasoning_effort: "high"✅review --effort high→ resolvedreasoning_effort: "high"✅ (previously: silently ignored)Notes
model_reasoning_effort(full), noteffort. The broker CLI flag is--effort(short). Getting this wrong silently no-ops./codex:reviewhas noeffortfield inReviewStartParamsat all, so the spawn override is the only path by which review can honor--effort.Opening here in the fork first for local CI / coordination before proposing upstream.
@hiirott — your two commits are carried forward verbatim (cherry-picked with authorship preserved); happy to coordinate if you'd rather fold this into openai#408 directly.