Skip to content

Capture resolved model/effort/sandbox in job records - #506

Closed
cubicj wants to merge 30 commits into
openai:mainfrom
cubicj:capture-resolved-settings
Closed

cubicj wants to merge 30 commits into
openai:mainfrom
cubicj:capture-resolved-settings

Conversation

@cubicj

@cubicj cubicj commented Jul 15, 2026 •

Copy link
Copy Markdown

Summary

Job records never capture the model, reasoning effort, or sandbox a job actually ran with (#481). Background task jobs persist the raw CLI request, but those fields are null when flags are omitted; foreground tasks and review jobs record nothing at all. The only place the resolved values exist today is the Codex rollout file, and joining a job record to its rollout fails in practice: date directories use local time while job timestamps are UTC, and rollout filenames carry a session id that does not match the job's threadId (both hit independently in the issue thread).

Change

The app-server already reports what it resolved: thread/start and thread/resume responses echo model, modelProvider, reasoningEffort, and the full sandbox policy with config.toml fallback applied, and the plugin currently discards everything but thread.id. This PR captures that echo instead of adding any new resolution logic, so it stays truthful regardless of how the pending override-mechanism discussions (#471, #408, #476, #482) land.

  • lib/codex.mjs: runAppServerTurn and runAppServerReview build resolved = { model, modelProvider, reasoningEffort, sandbox } from the thread response, attach it to the existing "Thread ready" progress event, and include it in the returned result. turn/start responses carry no effort field, so an explicit per-turn --effort is overlaid onto resolved only after the turn/start request is accepted — a rejected or failed turn never records a requested effort as if it had run.
  • lib/tracked-jobs.mjs: the job progress updater patches resolved into the job file and state index mid-run, the same way it already handles threadId, so failed and cancelled jobs still record what they ran with. The completion write adds resolved explicitly because it spreads the in-memory running record.
  • codex-companion.mjs: each execution object lifts result.resolved next to threadId/turnId; all job kinds share runTrackedJob, so foreground tasks, background workers, native review, and adversarial review are covered without per-path changes.

Job records gain one nested field, next to the existing raw request fields, so both layers stay visible — requested versus server-confirmed:

"resolved": {
  "model": "gpt-5.4",
  "modelProvider": "openai",
  "reasoningEffort": "medium",
  "sandbox": { "type": "readOnly", "access": { "type": "fullAccess" }, "networkAccess": false }
}

Human-readable output is unchanged: renderers read job fields selectively, the progress-event patch rides the same write the threadId update already performs, and no log line formats change. status --json and result --json gain the field additively.

Verification

  • npm test: 92/92 passing. New coverage asserts resolved in the persisted job JSON for native review, adversarial review, resumed tasks, and the detached background worker (checked mid-run, while still running); an explicit --effort overriding the thread echo on success; and a turn/start failure case with an explicit --effort proving the failed record keeps the thread echo rather than the never-applied request.
  • npm run build (typecheck via tsconfig.app-server.json): clean.
  • git diff --check against the base commit: clean.
  • The fake-codex fixture's thread/start/thread/resume responses now mirror the real app-server's resolved echo (shape confirmed against the generated ThreadStartResponse/ThreadResumeResponse types and a live JSON-RPC probe against codex-cli 0.144.1).

Related Issues

Fixes #481

dkundel-openai and others added 30 commits March 30, 2026 09:42
Co-authored-by: Codex <[email protected]>
Added instructions for reloading plugins and setup command.
…#13)

On Windows, Node.js `spawnSync` without `shell: true` uses
`CreateProcess`, which only resolves `.exe` files. npm installs global
tools (like `codex`) as `.cmd` shims, so `spawnSync("codex", ...)`
returns ENOENT even when codex is correctly installed and on PATH.

Adding `shell: process.platform === "win32"` routes through `cmd.exe`
on Windows, which properly resolves `.cmd`, `.bat`, and PATHEXT entries.
No behavior change on macOS/Linux since the condition evaluates to false.
* chore: bump plugin version to 1.0.1

* chore: bump plugin metadata version to 1.0.1
* fix: add shell and windowsHide options for Windows spawn in app-server

On Windows, spawn("codex", ["app-server"]) fails with ENOENT because
Node.js cannot resolve .cmd shims without shell: true. This adds
platform-gated shell and windowsHide options to the app-server spawn
call, and uses terminateProcessTree for proper process tree cleanup
since shell: true wraps the child in cmd.exe.

Without terminateProcessTree, plain SIGTERM only kills cmd.exe and
leaves the actual codex node process orphaned — verified with 274+
zombie node.exe processes accumulating on Windows.

Fixes #32
Fixes #46


* fix: guard terminateProcessTree against PID reuse after process exit

ChildProcess.killed only reflects whether .kill() was called by this
process — it stays false when the child exits on its own. On Windows,
where PIDs are recycled quickly, the 50 ms timer could fire after
cmd.exe has exited and its PID has been reassigned, causing taskkill
to terminate an unrelated process.

Add an exitCode === null check so the tree-kill path is skipped once
the child has already exited.


---------
* fix: make test suite portable across platforms

Replace hardcoded macOS path with fileURLToPath for cross-platform
compatibility. Add .cmd wrapper creation and platform-aware PATH
separator in test fixtures so the fake codex binary is discoverable
on Windows. Add shell and windowsHide options to the test helper
run() function to match production behavior.

Test results on Windows improve from 12/64 pass to 59/64 pass.


* fix: skip shell for absolute-path commands to avoid Windows space-in-path breakage

When `process.execPath` resolves to a path with spaces (e.g.,
`C:\Program Files\nodejs\node.exe`), `shell: true` causes cmd.exe
to split the path at the space. Guard with `path.isAbsolute()` so
only bare command names (which need `.cmd` shim resolution) use the
shell.


---------

Co-authored-by: Dominik Kundel <[email protected]>
* fix: skip untracked directories in review context

* fix: skip broken untracked symlinks in reviews
* Use app-server auth status for Codex readiness

* fix: reuse existing app server for auth checks
…provided (#159)

`SpawnedCodexAppServerClient.initialize()` passes `this.options.env` to
`spawn()`, but no caller ever sets `env` in options. In Node.js, passing
`undefined` for `env` gives the child process **no** environment variables,
breaking any model provider that relies on env vars (e.g. DATABRICKS_TOKEN).

Fall back to `process.env` when `this.options.env` is not set, matching the
existing pattern in `broker-lifecycle.mjs` and `codex-companion.mjs`.

Co-authored-by: Isaac

Co-authored-by: Bhuvanesh Sridharan <[email protected]>
…#126)

* fix: gracefully handle unsupported thread/name/set on older Codex CLI

Codex CLI v0.118.0 does not recognize the thread/name/set JSON-RPC method,
causing startThread() to throw. Thread naming is cosmetic (for job log
labels) and should not block thread creation. Wraps the call in try/catch
so it fails silently on older CLI versions.

Fixes #119

* refactor: only suppress unsupported-method errors for thread/name/set

Address Codex review feedback: the bare catch swallowed all errors
including auth, network, and server failures. Now only suppresses
errors containing 'unknown variant' or 'unknown method' (the specific
error older CLI versions return) and rethrows everything else.
* fix: avoid embedding large adversarial review diffs

* fix: preserve untracked content in lightweight review

* address comments

* fix: handle ENOBUFS type check in git diff sizing
* codex: honor --cwd when reporting session runtime

* codex: keep session runtime lookup scoped to callers

---------

Co-authored-by: VOIDXAI <[email protected]>
* fix: declare model in codex-rescue agent frontmatter

The codex-rescue agent had no model field, leaving Claude Code to assign
whatever default it chooses. As a thin forwarding wrapper that issues a
single Bash call, this agent is well-suited to the haiku tier; declaring
it explicitly ensures a predictable cost profile and tier guarantee.


* Update codex-rescue.md

---------

Co-authored-by: Dominik Kundel <[email protected]>
Closes #77

Updated README to replace unsupported 'xhigh' value with 'high' to prevent configuration errors.
Unquoted \$ARGUMENTS in the ! shell commands allowed shell metacharacters
in user-supplied job IDs to be expanded before Node received them (e.g.,
`task-123; malicious-cmd` would execute the trailing command). This is
inconsistent with review.md and adversarial-review.md, which both wrap
"$ARGUMENTS" in double quotes.
…on (#234) (#235)

* fix: route /codex:rescue through the Agent tool to stop Skill recursion (#234)

`/codex:rescue` previously combined two things that together caused a hang:

- `context: fork` in the frontmatter, which spawns a `general-purpose`
  subagent for the command body.
- Body prose "Route this request to the `codex:codex-rescue` subagent."
  without naming the transport.

When the main agent called `Skill(codex:rescue)` programmatically, the
fork resolved the ambiguous prose by trying `Skill(codex:codex-rescue)`
(unknown skill) and then falling back to `Skill(codex:rescue)`, which
re-entered this command and hung the session until the user cancelled.
No Codex job was ever created.

Naming the transport as `Agent(codex:codex-rescue)` alone is not enough:
forked general-purpose subagents do not expose the `Agent` tool, so the
forked runner cannot reach the subagent that way either. The minimal fix
is therefore two coordinated changes:

- Drop `context: fork` so the command body runs inline in the calling
  agent's context, where `Agent` is in scope.
- Say explicitly "use the `Agent` tool with `subagent_type:
  "codex:codex-rescue"`", and call out that `Skill(codex:codex-rescue)`
  and `Skill(codex:rescue)` are not valid routing paths. Add `Agent`
  to `allowed-tools` so the call does not prompt for permission.

Everything else in rescue.md (resume-candidate check, flag handling,
background/foreground semantics, operating rules) is unchanged. The
`codex:codex-rescue` subagent itself is unchanged.

Tests pin the new allow-list, the explicit `subagent_type`, the ban on
`Skill(codex:codex-rescue)`, and the absence of `context: fork`. The
existing "run the `codex:codex-rescue` subagent in the background"
assertion continues to hold since that sentence still reads correctly
with the Agent-tool transport.

Fixes #234

* test: match quoted result and cancel command arguments

---------

Co-authored-by: Dominik Kundel <[email protected]>
* Remove shell expansion for git commands

* Version bump
@axisrow

axisrow commented Jul 16, 2026

Copy link
Copy Markdown

Independently verified this end-to-end against a live app-server on codex-cli 0.144.4 (your probe was 0.144.1, so this doubles as a newer-CLI check). Everything holds:

  • Echo is real on 0.144.4: raw thread/start returns model/modelProvider/reasoningEffort/sandbox (my config resolved to gpt-5.6-sol/medium); config: { model_reasoning_effort: "high" } comes back reasoningEffort: "high".
  • End-to-end on real jobs: foreground task persists resolved with live values; task --effort high records resolved.reasoningEffort: "high" — so the onResponse overlay works against the real server, not just the fixture.
  • The echo is truthful: cross-checked the job record against the Codex rollout — turn_context entries carry reasoning_effort: "high", matching the record. This is exactly the join Job records never capture the resolved model/effort/sandbox a job ran with #481 made painful, now unnecessary.
  • Resume: task --resume-last recorded the thread-inherited high via the resume echo — state that was previously invisible anywhere in the plugin.
  • Forensics on dead jobs: SIGKILL'd a background worker mid-run; the orphaned record (stuck at running, per codex-companion background jobs: no turn timeout, no PID-liveness reaping, shell-mangled taskkill (Windows) — jobs stick as 'running' forever #478) still had resolved from the mid-run patch. That's the scenario that motivated my comment on Job records never capture the resolved model/effort/sandbox a job ran with #481, and it works.
  • Tests: 88/92 here; the 4 failures (status/result/resolveStateDir) reproduce identically on unmodified main, so delta vs main is zero. Typecheck clean. No garbage log line from the empty-message overlay event (appendLogLine trims and skips).

Two minor observations, neither blocking:

  1. Sandbox echo shape drifts across CLI versions. On 0.144.4 the echo is { "type": "readOnly", "networkAccess": false } — no access field, unlike the fixture and the PR-body example (access: { type: "fullAccess" }, the 0.144.1 shape). Since the plugin stores it verbatim as opaque data this is harmless, but consumers of resolved.sandbox shouldn't assume access exists, and the fixture now mirrors an older shape than current CLIs return.
  2. thread/resume of a turn-less thread fails on 0.144.4 ("no rollout found") — the rollout is created lazily at the first turn. Irrelevant to this PR (real resumes always follow turns), just a protocol nuance worth knowing while touching this surface.

LGTM — this closes the observability half of the #476/#481 invisibility pair, and the mid-run patch means even wedged jobs finally tell you what they ran with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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