Skip to content

fix(codex): resolve the current turn from the rollout when the Stop payload carries no turn_id - #1534

Merged
backnotprop merged 1 commit into
mainfrom
fix/codex-turn-id-fallback
Sep 15, 2026
Merged

backnotprop merged 1 commit into
mainfrom
fix/codex-turn-id-fallback

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

Release-QA finding: on Codex builds that ship the hooks engine but send a Stop payload without turn_id, plan review silently stops — no UI, no message, exit 0. v0.27.14 opened a review on the same session.

The Codex evidence

Checked against ~/oss-agents/codex (pulled today):

Tag codex-rs/hooks/src/schema.rs → StopCommandInput
rust-v0.113.0 no codex-rs/hooks/ crate at all
rust-v0.114.0 8 fields, no turn_id
rust-v0.115.0 8 fields, no turn_id
rust-v0.116.0 8 fields, no turn_id
rust-v0.117.0 pub turn_id: String — "Codex extension: expose the active turn id to internal turn-scoped hooks"
HEAD (a8964cb1ba) same as v0.117.0

So the hooks engine shipped in rust-v0.114.0 and the field landed three minor versions later; #1169 (551b908) assumed it had always been there (apps/hook/server/codex-session.ts:497-498: "Codex itself always sends one").

The rollouts at those tags do carry the id, which is what makes the fallback possible:

  • rust-v0.116.0:codex-rs/core/src/codex.rs:956 — TurnContext::to_turn_context_item() writes turn_id: Some(self.sub_id.clone()) on every turn_context
  • rust-v0.114.0:codex-rs/core/src/codex.rs:846 — same line, so the earliest affected tag is covered too
  • rust-v0.116.0:codex-rs/protocol/src/protocol.rs:2496-2498 — TurnContextItem.turn_id

The fallback rule

resolveCodexStopPlan now resolves the turn in two tiers:

  1. options.turnId when the payload carries the field — byte-identical behavior to before on Codex ≥ 0.117.
  2. The rollout's own markers when it does not: the id of the last id-carrying turn_context / task_started marker in the file (findLatestRolloutTurnId). One rule covers both shapes the owner described — a turn still in flight owns the last markers by construction (a completed turn's markers can only be followed by a newer turn's), and when Stop fires at turn end the last marker is that turn's own.

The resolved id then runs the unchanged #1169 machinery: findTurnStartIndex anchors on that turn's first marker, so mid-turn compaction (which re-emits turn_context with the same id) still cannot hide a plan produced before the compaction point, and getPlanItemText's per-turn filtering is untouched.

Still fails closed:

  • a rollout with no id-carrying turn marker at all → missing-turn-marker;
  • a payload carrying turn_id as a blank string → missing-turn-id, refused before the file is read, as the finding asked. The tier is keyed on the field being absent (options.turnId !== undefined), and index.ts maps a present-but-non-string value to "" rather than collapsing it to "absent", so a truncated or foreign payload can never take the fallback.

Using the fallback writes one line to stderr, unconditionally (logCodexStopTurnIdFallback) — stdout is the hook's JSON decision channel, and Codex only reads a Stop hook's stderr at exit code 2 (rust-v0.116.0:codex-rs/hooks/src/events/stop.rs:198-216), which this hook never uses, so the line is a captured breadcrumb and never becomes agent input.

Docs

  • apps/marketing/src/content/docs/guides/troubleshooting.md — a "Minimum Codex version" subsection with a verified version table (rust-v0.114.0 hooks engine, codex_hooks off by default → rust-v0.117.0 turn_id → rust-v0.125.0 default-on (features/src/lib.rs:756-760) → rust-v0.131.0 key renamed to hooks with codex_hooks as a legacy alias (features/src/lib.rs:844-848, features/src/legacy.rs:45)), plus the explicit statement that a turn_id-less Codex is handled by the rollout fallback.
  • AGENTS.md (CLAUDE.md is a symlink to it) — a "Codex Stop hook: which turn the plan belongs to" section under Plan Review Flow documenting both tiers and the fail-closed cases.

Tests

apps/hook/server/codex-session.test.ts — new Codex Stop turn id fallback describe (6 tests):

  • payload without turn_id + rollout with one turn → plan resolved, fallbackTurnId reported
  • without turn_id + compaction-re-emitted turn_context → the plan written before compaction is resolved
  • without turn_id + two turns where only the older one has a plan → null (stale plan never re-reviewed)
  • no id-carrying turn marker → missing-turn-marker
  • with turn_id → unchanged: the payload-named turn wins and no fallback is reported
  • the stderr notice fires with no debug flag and names the turn

Two existing tests were updated because they pinned the contract this PR deliberately changes: the missing-turn-id diagnostic test now covers blank-but-present ids (the case that still fails closed), and the "does not scrape an assistant proposed plan for a Stop event without a turn id" test gained a newer turn so it keeps guarding the real #1169 risk — an older, already-decided turn's plan is still invisible — rather than the old blanket refusal.

bun test apps/hook/server/ → 262 pass, 0 fail (14 files). bun run typecheck → clean. bunx tsc --noEmit on a scoped throwaway config over server/{index,codex-session,codex-session.test}.ts with bun-types → no errors in the touched files (the 5 "exit" is not assignable to "memoryPressure" hits are pre-existing node/bun-types clashes in packages/ai and packages/server).

Follow-up, deliberately not in this PR

scripts/install.sh is untouched. The related finding — the installer writes [features] hooks = true, which Codex only understands from rust-v0.131.0, and its awk normalizer rewrites a working codex_hooks = true into it (scripts/install.sh:1096-1152) — is pre-existing (the key changed in 6e3efe8e, already an ancestor of v0.27.14) and wants its own change: write both keys and make the awk additive, or gate the Codex leg on a codex --version probe. The troubleshooting doc added here tells affected users what to write in the meantime.

…ayload carries no turn_id

Codex added turn_id to StopCommandInput in rust-v0.117.0 (0.116.0-alpha.12),
after the hooks engine shipped in rust-v0.114.0, so stable rust-v0.114.0,
v0.115.0 and v0.116.0 fire the Stop hook with no turn_id at all. #1169 made
resolveCodexStopPlan fail closed on that payload, which silently disabled plan
review on those versions.

When the payload omits the field, derive the turn from the rollout instead: the
id of the last id-carrying turn_context/task_started marker, then the same
first-marker-anchored scan, so mid-turn compaction safety and per-turn plan
filtering are unchanged. A rollout with no turn markers still fails closed, and
so does a payload that carries turn_id as a blank string (truncated or foreign,
refused before the file is read).

Using the fallback writes one line to stderr, unconditionally — stdout is the
hook's JSON decision channel, and Codex only reads a Stop hook's stderr on exit
code 2, which this hook never uses.

Docs: minimum-Codex-version table in the Codex troubleshooting guide and a
turn-resolution section in AGENTS.md.
@backnotprop
backnotprop merged commit bfb3a55 into main Sep 15, 2026
28 checks passed
@backnotprop
backnotprop deleted the fix/codex-turn-id-fallback branch September 15, 2026 10:11
backnotprop added a commit that referenced this pull request Sep 15, 2026
…fallback path (#1539)

The `stop_hook_active` guard in `findPlanInTurn` anchors on the
`<hook_prompt>` USER message Codex records when a Stop hook blocks. That item
only arrived in rust-v0.117.0 (`build_hook_prompt_message`): rust-v0.114.0,
v0.115.0 and v0.116.0 — the exact versions #1534's rollout turn-id fallback
newly enables — record the continuation as
`DeveloperInstructions::new(continuation_prompt)`, i.e. a developer-role
message, and then continue the SAME turn with `stop_hook_active = true`.

The guard was therefore inert there: after "Request changes", if the model
answered without emitting a fresh `<proposed_plan>`, the next Stop resolved the
same turn, found no boundary, and re-served the unchanged denied plan for
review. Codex's continuation loop has no iteration cap, so denying again just
repeated it.

Generalize the boundary: the `<hook_prompt>` item stays authoritative, and the
last developer-role message in the turn is accepted as the boundary only on the
rollout-fallback path (a Stop payload with no `turn_id`), so every Codex that
sends `turn_id` keeps byte-identical behaviour.

Also:

- Correct the troubleshooting version table. Hooks became default-enabled at
  rust-v0.124.0 (`0.124.0-alpha.3`, `Stage::Stable` + `default_enabled: true`),
  not 0.125.0, and the `codex_hooks` -> `hooks` key rename landed at
  rust-v0.129.0 (`0.129.0-alpha.5`), not 0.131.0 — verified against
  `codex-rs/features/src/lib.rs` at each tag, with 0.128.0 still on
  `codex_hooks`. The derived prose now names the real codex_hooks-only range.
- Fail closed instead of throwing on a non-string `turn_id`: the coercion now
  lives inside `resolveCodexStopPlan` rather than only at the CLI call site, and
  the debug breadcrumb says the id was present but unusable rather than missing.
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.

1 participant