Job records never capture the resolved model/effort/sandbox a job ran with #481

Activity

1. axisrow commented on Jul 12, 2026

   

   axisrow

   on Jul 12, 2026

   Last edited by axisrow

   More actions

   Seconding this from the reviewer's side — I just hit every trap you described, with concrete numbers, trying to answer "did --effort high actually reach the model in this review?".

   I ran a codex adversarial-review --effort high (via the plugin's companion, codex-cli 0.144.1) and then tried to confirm the resolved effort. What I found maps 1:1 onto your write-up:

   • The job record has no model/effort. jobs/review-<id>.json persisted status, threadId, turnId, startedAt/completedAt — but nothing about the model or reasoning_effort it ran with. Exactly your point about createJobRecord / runTrackedJob.
   • The only place the resolved effort exists is the rollout turn_context, and it's hard to join:
     • The job's createdAt was 2026-07-12T05:30Z (UTC), but the rollout landed in the 2026/07/11/ date directory — the thread's original-date dir, not today's. Same local-vs-UTC drift you called out.
     • The rollout filename carries a session-id (019f50ff-…), not the threadId (019f54ce-…) the job record stores. Joining job threadId → rollout filename fails outright.
     • The only thing that reliably located the rollout was grepping for the review's own output text, not any id.
   • ccusage (great tool, btw) didn't surface this session at all — presumably because its date-dir scan skipped the 07/11 placement for a job that "ran" on 07/12. A clean illustration of why external tooling can't rely on the join.

   For the record, the resolved effort was correct once I finally found the rollout: the main turn_context entries carried reasoning_effort: "high" (the low entries were Codex's internal helper event_msgs, not the override leaking). So the override works — it's purely the observability that's missing.

   Your proposed fix (write resolved model/effort/sandbox from the thread/start response into the job record on all paths) is exactly what would have saved me ~30 minutes of grepping rollout dirs. +1 to that direction. Happy to test/review a PR, or to contribute the job-record write against your design if you'd rather hand it off.

   (Side note: this is also the same invisibility theme as #476 — review silently ignoring --effort. Once the override is wired through, #481 is what lets anyone prove it took effect.)

   cc @hiirott (#408, spawn--c model fix — knowing the resolved model up front at spawn would make writing it into the job record trivial), @klmklmnb (#418, forwarding extra args to the Codex CLI — adjacent, about inputs the job never records), @jianjettfu-oss (#309, the unsupported-default-model boot failure on review) — your takes on where the resolved model/effort should be captured would be useful here.
2. cubicj commented on Jul 12, 2026

   

   cubicj

   on Jul 12, 2026

   Author

   More actions

   Thanks for the independent reproduction — the join pain you hit (UTC job timestamps against the thread's original date dir, session-id filenames against the job's threadId) is exactly the cost this issue was meant to capture, and confirming the override itself worked makes the gap cleaner to see.

   I originally filed this as an issue rather than a PR because I expected "which values count as resolved" to depend on which override mechanism lands, whether that's #471's thread/start.config route, spawn-time -c from #408, or today's request fields. I went through the code again before replying, and I no longer think it does. ThreadStartResponse and ThreadResumeResponse already carry resolved model, modelProvider, reasoningEffort, and sandbox as a full SandboxPolicy object, with the config.toml fallback applied — I probed this over JSON-RPC earlier: omitted params come back resolved from config, explicit params come back echoed. The plugin receives all of it and throws it away. startThread returns the full response (plugins/codex/scripts/lib/codex.mjs:733), but both callers read only response.thread.id (codex.mjs:1010 for native review, codex.mjs:1114 for the turn path). Since it's the server reporting what it resolved, the capture point stays truthful under any of the pending mechanism decisions, including a config-driven sandbox from #482. The one gap is that TurnStartResponse is { turn } only while the task path passes effort at turn/start (codex-companion.mjs:490), so the thread-level echo needs an options.effort ?? response.reasoningEffort overlay to reflect an explicit per-turn override.

   The change I'd propose is observability-only and reuses the plumbing threadId already rides. In lib/codex.mjs, pick resolved = { model, modelProvider, reasoningEffort, sandbox } from the thread/start/thread/resume response in runAppServerTurn and runAppServerReview, attach it to the existing "Thread ready" progress event, and include it in the returned result. In lib/tracked-jobs.mjs, let the progress updater patch resolved into the job file mid-run the same way it patches threadId, and add resolved: execution.resolved to the completion write — tracked-jobs.mjs:158 spreads the in-memory record, so a mid-run-only patch would be clobbered there. In codex-companion.mjs, lift result.resolved into the execution objects next to threadId/turnId; every job kind shares runTrackedJob, so foreground and background paths get it without per-path changes. Tests would extend the fake-codex fixture's thread/start response and assert resolved lands in the job JSON across task, background worker, native review, and adversarial review, plus a failure case keeping the mid-run value.

   The mid-run patch is what addresses your scenario directly: a failed or cancelled job still records what it actually ran with, so proving an override took effect never requires the rollout join again. Keeping resolved as a nested object next to the existing raw request fields also leaves both layers visible, requested versus server-confirmed, which is the comparison #476 debugging needs. The only call I'd still want from a maintainer is the field shape — nested resolved: {...} versus flat fields on the record. Happy to send the PR on this design, and the rollout cross-checking you've already done here is exactly the validation it needs.