Hold native delivery while a Codex thread waits on a human

[schickling]

Why

ThreadStatus's active arm carries a required activeFlags: ThreadActiveFlag[], where
ThreadActiveFlag is exactly "waitingOnApproval" | "waitingOnUserInput". observe_thread_status
took status: &str — one word — so all three call sites (:702, :724, :732) read
.../status/type and discarded the rest of the status object at the parse site. st2 therefore could
not distinguish "the model is working" from "the model has stopped and is waiting for a human".

The delivery consequence follows from st2's own code: an approval happens mid-turn, so the session is
Active { turn_id }, and maybe_request maps Active to CodexDeliveryMethod::Steer
(:415-419). st2 steers a [DING] into a session that is sitting on an approval dialog.
That consequence is inferred, not observed — see ## Limits.

What

One localized change to observe_thread_status and its three call sites.

• Parse the field. New human_blocking_flag(status: Option<&Value>) -> Option<CodexHoldReason>
  reads activeFlags from the status object and returns the first flag that means "blocked on a
  human". It is called at all three sites, passing the status object each already had in hand.
• Map it onto an existing hold. Two CodexHoldReason variants, WaitingOnApproval and
  WaitingOnUserInput. A flag arriving while Active { turn_id } becomes
  Held { reason, turn_id: Some(turn_id) }.
• Release the same turn when the flag clears. {"type":"active","activeFlags":[]} arriving on a
  flagged hold returns to Active { turn_id }. This is the case a naive fix gets wrong: the old
  "active" arm preserved state only for Active and Held{Review|Compaction|ConflictingTurn}, so
  a cleared flag would have fallen through to Held{ActiveWithoutTurn, turn_id: None} — turn ID
  lost, and no second turn/started arrives mid-turn to restore it. Native delivery would have
  stalled for the rest of the turn.
• Unknown flag values degrade to plain active, and a missing or malformed array reads as no
  flag rather than failing the frame. observe's error propagates via ? (:1944), so a strict
  accessor on a schema surprise would kill the control watcher.
• A more specific hold still outranks the flag. Review, Compaction and ConflictingTurn
  keep their own turn IDs, which the turn-lifecycle handlers depend on.

maybe_request is not modified. It already returns None for every Held reason
(:420-421), so the behavioural half is one more input mapped onto the existing, invariant-pinned
hold path — the same path Review and Compaction take.

Out of scope and untouched: observe_turn_completed (#264), the item/started / item/completed
handling (#266), and SUPPORTED_CODEX_CLI_VERSIONS (#267).

Impact

st2 declines to steer into a Codex session that has stopped for a human, and resumes steering the
same turn once the human answers.

Why this is fail-closed, not fail-open. Declining is not dropping. A hold makes maybe_request
return None, which is the already-tested behaviour for Review and Compaction
(review_compaction_and_dnd_hold_the_unread_fifo_head): the unread FIFO head stays in the inbox, no
delivery ownership is recorded, no paste or archive happens, and the same head is delivered on the
next poll once the state clears. The new test asserts exactly that round trip — held twice with the
message still on disk, then turn/steer with expectedTurnId: turn-1 after release. The failure
direction the change introduces is deferral; the failure direction it removes is an unobservable
steer into a modal. No new delivery path is created and no Delivered classification is relaxed.

One deliberate edge, pinned but not fixed here: a turn that completes while still flagged falls into
the pre-existing ConflictingTurn catch-all in observe_turn_completed and is released by the next
idle status. That function is #264's region, so it was read, not edited; the test asserts only that
the flagged state cannot decay into a steerable turn.

INVARIANTS.md is unchanged. Per the issue's "or is documented as deliberately unpinned" branch,
the steerability rule stays a code comment (:160-163): a single hold-reason mapping is not the tier
CLAUDE.md reserves rows for, and INVARIANTS.md is a shared file three sibling PRs are near. The
local proof is the named test below.

Checks

Schema evidence, generated locally against the two versions st2 actually supports:

# 0.145.0 and 0.146.0 installed into throwaway trees; each tree's own binary was version-checked
# before generating, so the schema is attributed to the version that actually emitted it.
npm install @openai/codex@0.145.0        # ./node_modules/.bin/codex --version -> codex-cli 0.145.0
npm install @openai/codex@0.146.0        # ./node_modules/.bin/codex --version -> codex-cli 0.146.0
./node_modules/.bin/codex app-server generate-json-schema --out schema145   # from the 0.145.0 tree
./node_modules/.bin/codex app-server generate-json-schema --out schema146   # from the 0.146.0 tree
codex app-server generate-json-schema --out schema147                       # installed 0.147.0

v2/ThreadStatusChangedNotification.json → ThreadStatus.ActiveThreadStatus:
  0.145.0  required: ["activeFlags", "type"]   activeFlags: array<ThreadActiveFlag>
  0.146.0  required: ["activeFlags", "type"]   activeFlags: array<ThreadActiveFlag>
  0.147.0  required: ["activeFlags", "type"]   activeFlags: array<ThreadActiveFlag>
  ThreadActiveFlag = enum ["waitingOnApproval", "waitingOnUserInput"]   (identical on all three)

activeFlags is present, required, and identically shaped on 0.145.0 and 0.146.0 — the complete
SUPPORTED_CODEX_CLI_VERSIONS set. The issue's "unverified on a supported version" caveat is
retired; #267 is unaffected either way, and :38 is untouched.

Test, written before the fix and confirmed failing against it:

nix develop --command cargo test --locked --lib waiting_on_a_human

  before (variants added, no parse):
    panicked at src/codex_app_server.rs:3916:
    assertion failed: state.observe(&status_changed(flags)).unwrap()
    → the frame changed nothing; st2 stayed Active{turn-1} and would have steered

  after:
    test codex_app_server::tests::
      waiting_on_a_human_holds_the_exact_turn_and_releases_it_when_the_flag_clears ... ok

It covers the four cases the issue lists — [], ["waitingOnApproval"], ["waitingOnUserInput"],
an unknown value — plus a mixed array, thread/started carrying the field before any turn is known,
a status arm with no activeFlags at all, the clear-and-release round trip, and the delivery
assertion.

Delivery path, since the behavioural half is included:

nix develop --command cargo test --locked --lib ding::
  → 56 passed; 0 failed

  All 18 tests named in the Fail-closed observed native DING row of INVARIANTS.md are in that
  run and green, including poke_text_normalizes_and_bounds_untrusted_fields,
  pty_delivery_uses_face607_delay_order_and_seconds,
  successful_transport_with_retained_or_unproven_pixels_is_not_delivered,
  staged_ownership_survives_archive_and_never_repastes,
  archived_not_retained_releases_fifo_without_repasting_owned_notice, and
  pending_delivery_ignores_busy_but_respects_fresh_dnd_archive_and_retry.

nix develop --command cargo test --locked --lib codex_app_server
  → 35 passed; 0 failed
  Includes control_initializes_before_recording_the_first_thread_only, the existing
  per-direction request-ID test that injects item/commandExecution/requestApproval (:3214).

Full suite, honestly reported:

nix develop --command cargo test --locked --no-fail-fast
  lib: 322 passed; 0 failed   (321 on the base commit, +1 new test)
  13 integration tests fail.

The same 13 fail identically on the unmodified base commit f177520 in this worktree, so the sets
are equal and this change adds none of them. They are environment- and concurrency-bound (real PTY,
systemd scopes, cgroups, and sibling agents running the full suite on the same machine):
classification_only_and_nested_agent_filename_changes_are_exact,
canonical_agents_freeze_the_admitted_route_across_post_boot_catalog_mutation,
clean_path_supports_help_validate_env_and_doctor,
exec_task_survives_transport_cgroup_cascade, managed_agent_color_contract_crosses_systemd_scope,
pty_task_survives_transport_cgroup_cascade, st2_down_tears_down_a_spec_fleet,
st2_up_boots_a_specs_team, st2_up_once_atomically_respawns_a_hard_killed_agent,
st2_up_spec_supervises_and_respawns_a_killed_agent,
targeted_once_real_pty_preserves_sibling_generation_across_selected_lifecycle,
tracked_product_surface_contains_only_native_names,
up_materialize_only_writes_the_overlay_without_needing_pty.

cargo fmt --check is dirty repo-wide on main (438 diffs). Every hunk this branch adds is
rustfmt-clean; the remaining diffs in src/codex_app_server.rs are at lines this branch does not
touch. No Darwin run.

Limits

• The consequence is inferred, not observed. No capture has ever shown a populated
  activeFlags. The one live attempt saw "activeFlags":[] because the account hit its ChatGPT
  usage limit before an approval was reachable, and a re-attempt on this branch hit the same limit —
  the error payload names 2026-08-20 as the reset. That a session sitting on an approval prompt
  reports active with ["waitingOnApproval"] follows from the enum's names and the field's
  placement inside the active arm. It is schema evidence, not wire evidence. Nothing in this PR
  should be read as a claim that the steer-into-dialog behaviour was observed.
• Version caveat, weakened but not silent. The schema check above is a real check on both
  supported versions, so the field is no longer "unverified on a supported version". What remains
  unverified is that 0.145.0 and 0.146.0 populate it in the situation the flag names — the same
  gap as the point above, on every version. The field is required on ActiveThreadStatus on all
  three, so its presence is not in question.
• Unknown flag values stay steerable. This is the issue's explicit requirement, and it is a
  fail-open edge: a future codex-cli flag meaning "blocked" would read as plain active until
  someone classifies it. The control is the version gate — the comment at :35-37 requires a
  delivery-critical schema comparison before any new version is admitted, and that comparison is
  where a new flag value has to be classified. Erring the other way would let one new benign flag
  silently stop native delivery fleet-wide.
• activeFlags is persisted as a derived hold, not as the raw array. The delivery-relevant
  content of the array is whether a known human-blocking flag is present, and the hold reason names
  which one. If both flags are ever set at once, only the first is reported.
• Staggered binary upgrade. CodexControlState is #[serde(deny_unknown_fields)] (:192) and
  no field is added here, but two CodexHoldReason variants are. An older binary reading a record
  containing "reason":"waitingOnApproval" fails serde_json::from_slice in
  load_current_control_state (:2231) — an unknown enum variant is a hard error, not an ignored
  key, and the schema string st2.codex-control-state.v1 is unchanged so nothing catches it
  earlier. The blast radius is bounded by the record itself: it is keyed to
  runtime_incarnation, so only the exact same incarnation reads it back, and today no caller
  outside this module's tests reads it. Stated, not solved.
• The server→client approval requests are still on the floor. The ten requestApproval /
  requestUserInput / elicitation methods still fall through observe's _ => return Ok(false). activeFlags says that an agent is blocked; those say what on, and pairing them
  with serverRequest/resolved is what would make blocked dwell time measurable. Not in this PR.

Related

• Closes observe_thread_status takes only the status word, so a required ThreadStatus field is parsed and discarded at all three call sites #265.
• st2 refuses the locally installed codex-cli, and the version pin is now behind by one release #267 — the version pin. The schema check above says activeFlags needs nothing from it, but
  :38 is that PR's region and is untouched here.
• A failed Codex turn is reported as two conflicting turns and stops accepting messages until the next idle #264, exitedReviewMode is never matched, so Held{Review} clears only when the thread reports idle #266 — sibling defects in the same file; their regions are untouched.

Ready for review.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[schickling]

Rebased onto main, plus one integration change worth reviewing

main moved while this was open: #270 and #272 merged. Rebased onto 02fbed8.

#270 turned observe_turn_completed into an exhaustive match with no catch-all, and this branch adds two new CodexHoldReason variants — so the rebase did not compile:

error[E0004]: non-exhaustive patterns: `Held { reason: WaitingOnApproval, .. }`
              and `Held { reason: WaitingOnUserInput, .. }` not covered
   --> src/codex_app_server.rs:881:31

That is #270's exhaustive match doing exactly what it was written to do — its own comment says it "stays exhaustive so a new observed state cannot silently arrive as a conflict it never was". Before #270, these two variants would have fallen through the old _ arm and been silently reported as ConflictingTurn.

Resolution: both variants were added to the preserved arm, following #270's stated rule — "Only the signal that minted the hold releases it." The waiting-on-human holds are minted from activeFlags on a thread status, so they are cleared by the next thread status that omits the flag, not by a turn completing. The comment above the arm was extended to say so.

This supersedes the note in the original PR description that a turn completing while flagged "hits the pre-existing ConflictingTurn catch-all and is released by the next idle status" — that catch-all no longer exists, and the behaviour is now explicit rather than incidental.

Worth a reviewer's eye, because it is a behavioural decision made during integration rather than in the original change: a turn that completes while still flagged now stays held until a thread status clears the flag, instead of decaying to ConflictingTurn and waiting for idle. I believe that is the honest reading and it is consistent with how every other non-lifecycle hold is treated, but it is the one judgement here that was not in the reviewed change.

Verification after rebase: cargo test --locked --lib codex_app_server → 36 passed, 0 failed. cargo test --locked --lib ding → 68 passed, 0 failed, covering the tests named in the Fail-closed observed native DING invariant row. Conflicts with #271 are gone; both branches now sit directly on main.