fix(sessions): declare the withheld interrupted write as a held pending Session write

[dixso]

This pull request fixes #4827 by giving the interruption park durable ownership of the
session batch it withholds, replacing the history-reconciliation approach that the
previous revision of this PR used and that review rejected.

The bug

_should_defer_interrupted_session_items withholds the interrupted turn's session
write when the agent has output guardrails and a non-default tool_use_behavior. The
park leaves no record of what it withheld, so no later code can prove it happened.
When the approval resume resolves into next_step_run_again, only the resolved turn's
items are written: the Session ends up holding a function_call_output whose
function_call was never persisted, and the Responses API rejects every later run
over that Session with No tool call found for function call output. The
conversation is permanently dead. This is reproducible with ScriptedModel alone
(see tests/test_deferred_interrupted_session_write.py, red on main).

The fix: registering is not writing

RunState._pending_session_write already is the durable single-slot primitive for
"one canonical resumed append awaiting acknowledgement", with digest-based crash
recovery. The park now registers the withheld batch on that slot with a held
marker:

• Registering touches only the checkpoint, never the Session, so the output-guardrail
  persistence gate is preserved exactly: nothing reaches the Session until the gate
  stops applying to the parked response.
• At a gate-legal exit of a later resume, the batch lands ahead of the resolved
  turn's items in one ordered save_result_to_session append (call before output by
  construction) and re-registers as the one ordinary pending write, inheriting the
  existing digest reconciliation for a crash mid-append.
• A run-again checkpoint (what an after_turn cancellation of a detached resume
  leaves behind) settles at resume entry: the parked response's outputs already went
  back to the model, which only happens after the gate stopped applying, and the
  run-again turn's saves never arm resumed_write_state, so entry is the only settle
  point that checkpoint will ever reach.
• A detached exit folds the resolved items into the standing batch so the reattaching
  resume settles call and output together.
• An emptied resolved turn (a handoff input_filter can drop every item) settles
  the pairs the batch already holds and drops the rest: by then the approved tool has
  run and its output is in the batch, so emptiness of the turn is the wrong predicate
  and pairing is the right one, exactly as at every other settle.
• The blocked-output redaction never sees the batch raw: the tripwire path discards
  the declaration once the blocked outcome is decided, and persists only the
  sanitized rebuild.

The history-reconciliation machinery from the previous revision
(deferred_interrupted_session_prefix, _identity_key, _PREFIX_MATCH_LOOKBACK) is
deleted.

How this addresses each review point

1. "Looking up the deferred prefix after the approved tool executed": there is no
   lookup anymore. The batch is on the checkpoint before the resume starts.
2. "The streamed lookup drops the context wrapper": the wrapper-less read is deleted;
   every settle flows through the existing save and settle machinery, whose call
   sites carry wrapper=context_wrapper. Pinned by
   test_settle_reaches_a_context_aware_session_through_the_wrapper.
3. "Legacy no-argument Sessions fail": the per-resume limit= read is deleted with
   the lookup. The settle inherits the pre-existing limit= reads of
   resume_pending_session_write; a companion change makes _session_get_items
   probe the signature and fall back to a full read with the released latest-N
   semantics applied locally. Pinned by
   test_a_session_without_optional_kwargs_survives_a_deferred_resume.
4. "Finalization or reconnect can drop or duplicate the batch": the declaration
   survives serialization and the step flip to run-again, and every exit either
   settles, extends, or discards it deliberately. to_state() on a completed run
   never carries a stale record. Pinned by
   test_after_turn_cancel_keeps_the_held_batch_for_the_next_attach and the
   state._pending_session_write is None assertions.

Serialized-state note (per .agents/references/runstate-schema.md)

The held variant adds keys the 1.17 reader rejects by exact key set, and 1.17 readers
are already on main, so writing it under that label would emit checkpoints they cannot
load. It therefore has its own schema version: CURRENT_SCHEMA_VERSION is now
1.18, 1.17 keeps the four-key form and the original summary it shipped with, and the
held keys are gated to 1.18 while 1.17 payloads still restore and settle eagerly as
before. Corpus fixtures (minimal/v1_18.json, features/v1_18_held_pending_session_write.json),
sources.json, the corpus README and the supported-version boundary test are updated.
Pinned by test_pending_session_write_without_the_held_key_keeps_its_meaning (the
1.17 form still loads and settles) and by the held-under-1-17 case of
test_pending_session_write_rejects_invalid_serialized_checkpoint (the held variant
under the older label is refused).

Contract-surface inventory for held

surface	producers	consumers	default/missing	invalid
pending_session_write.held	the four park sites: fresh and re-park, both runners (defer_interrupted_session_write)	from_json validation, resume-entry settle rules, exit settles (take_held_session_write), detached folds (extend_held_session_write), to_json and both checkpoint copies (opaque deepcopy), the non-streamed result bridge	absent or false keeps the released meaning: eager settle at resume entry	non-bool rejected; held with recorded before digests rejected

Await-boundary inventory for the settle

• Operation: settling the held batch at a resume exit. Snapshot: the batch claimed
  from the slot (take_held_session_write frees the slot first). Suspension point:
  the composed save_result_to_session append. While suspended: the write is
  registered as the one ordinary pending write with before digests, so a crash or
  cancellation mid-append is recovered by the existing committed-versus-unchanged
  reconciliation on the next resume (pinned by
  test_a_failed_settle_of_the_held_batch_is_recovered_on_the_next_resume, 16
  interleavings across both runners and serialization).
• Ownership mechanism: the existing single-slot rule. A settling write registers
  itself; a second batch while one is unresolved fails fast with the existing
  UserError.
• Known pre-existing limitation, unchanged by this PR: two independently restored
  copies of the same checkpoint have no cross-object interlock
  (resume_pending_session_write documents that the application must serialize
  access), so the held variant inherits exactly the same contract.

Decisions taken with a stated default

• Decision: the held variant gets its own schema version (1.18) rather than extending
  1.17 in place, because the 1.17 reader validates the pending write by exact key set
  and would reject a checkpoint written under its own label.
• Decision: an emptied resolved turn settles the batch's paired part rather than
  discarding it wholesale. Discarding was defensible while the batch could only hold
  the parked call; once the resumed turn's committer folds the executed output in, a
  wholesale discard loses the Session's only record that the tool ran and lets the
  next run re-issue its side effect.
• Decision: a run-again checkpoint settles the batch at entry, because that
  checkpoint shape only exists after the parked response's outputs went back to the
  model and no later settle point exists on that path.
• Decision: the legacy get_items signature probe is a companion fix in this PR
  because the settle machinery it protects predates this change; without it a
  pre-limit structural Session hard-fails any pending-write recovery.
• Decision: a held resume with a different attached Session fails at boot with the
  existing same-session error, before the approved tool can execute; a detached
  resume still rides.
• Decision: a held call settles only when paired. Pairing is per call, so a handoff
  input_filter that drops one resolved output takes exactly that call with it,
  while calls whose approvals are still open settle unpaired on purpose: their
  outputs have not run yet, matching how a non-deferred park persists a call before
  its output. The entry settle applies the same contract against the batch alone.
• Decision: a fresh park during a detached resume folds the new call into the
  standing held batch (the declaration carries the session identity), and a detached
  completion discards the batch at the final exit so a completed run's checkpoint
  stays loadable in both runners.
• Decision: the held batch carries only the withheld response, never the run
  input. The gate withholds model output, not the user's accepted input, so a
  deferred park persists any still-unsaved input (the sandbox runtime defers the
  pre-turn save) exactly as the non-deferred arm does, and a tripwire discard can
  never take the Session's only copy of the input with it.
• Decision: a settling held batch always registers as the one pending write before
  its append, regardless of the current step, so a crash inside the settle fails
  closed with the batch recorded. The resulting mid-settle checkpoint is rejected
  on load on purpose: the run ended mid-settle, and failing closed beats replaying
  an approved side effect as if nothing happened.
• Decision: the attached entry settle re-applies the Conversations-specific
  sanitization before the direct append, restoring the invariant for items a
  detached extension added without a Session in hand.
• Decision: when the guardrail rebuild already carries every held request, the batch
  is dropped as redundant at the shared settle choke point, because the item
  deduplication cannot key its unkeyed companions (an assistant preamble, an id-less
  reasoning item) and feeding it again would duplicate them.
• Decision: the settling batch's tool outputs count when deciding whether to defer
  compaction. The batch reaches the canonical path through the original_input slot,
  so a decision that only inspected new_items would compact the very response whose
  output had just landed; the record also carries the store setting of the turn it was
  withheld in, so the deferral resolves the mode the ordinary path would have.
• Decision: response_id and store are refused on an ordinary pending write, where
  they describe nothing, and the 1.18 corpus entries are what the recorded 1.17 writer
  emits with only the schema label changed, with matching generator scenarios so the
  corpus stays reproducible.
• Decision: the pairing guard delegates to the canonical drop_orphan_function_calls,
  so every tool-call family in _TOOL_CALL_TO_OUTPUT_TYPE pairs (shell and apply-patch
  included) and a reasoning item riding before a dropped call is pruned with it; hosted
  MCP approvals pair alongside through the canonical request identity.
• Decision: a Conversations-backed registration forces the reasoning-id policy to
  None like the normal save, and settled held items count toward the turn's persisted
  count so a later gate-enabled resume fails fast instead of re-appending.
• Decision: a committed tool output folds into the held batch at the resumed turn's
  output-commit boundary, so a post-output callback that raises cannot leave a retry
  that skips the completed invocation and drops the executed call and its result.
• Decision: a max-turn handler ends the run, so a held batch still standing there is
  discarded in both runners; the finished run's checkpoint stays loadable and the two
  runners report the same terminal state. The discard sits below
  validate_handler_final_output, because only past that validation does the handler
  actually end the run.
• Decision: the compaction deferral inspects the original_input slot only on the
  calls that settle a batch through it. That slot carries the caller's own input on
  an ordinary save, and an earlier tool output sitting there says nothing about
  whether this response produced one.
• Decision: the settled count is reported by the append itself rather than added as
  the batch's raw length. The resolved turn re-delivers the outputs the batch already
  folded in, they dedup away, and the count slices a later save of the same turn. The
  compaction-deferral branch returns the same combined count: it is the branch every
  held settle with outputs takes on a compaction-aware backend, so returning the
  run-item count alone there undercounted exactly the sessions that defer.
• Decision: the local-continuation classification covers the hosted MCP approval
  response in both of its carriers (the settled dict and the run item the
  non-deferred resume commits). The approval response is the locally produced half
  of its pair and must stay associated with the response chain that carried the
  request; classifying one carrier and not the other would defer or compact the same
  response depending on which path persisted it.
• Decision: the held record owns the conversion policy of its items
  (reasoning_item_id_policy, gated and validated with the other held keys). A
  detached fold cannot see the Session backend, so converting under the resuming
  run's own policy could strip a Conversations server reasoning id at the one point
  where nothing can restore it; the record carries the policy the park actually
  used, and folds convert under it.
• Decision: exit-based settles run the compaction bookkeeping for the resume's
  response, not the parked one. At every exit a newer response is the session's
  compaction frontier and the batch lands inside its append; the recorded
  response_id and store are for the entry settle, the one place where the
  parked response still is the frontier.

Test plan

• tests/test_deferred_interrupted_session_write.py: park, approve, reject,
  re-park, detached carry with after_turn cancellation, tripwire (with a preamble
  pinning that the redaction's drops are not resurrected), guardrail crash, emptied
  turn, context-aware and legacy sessions, single ordered write, and failed-settle
  recovery; each scenario runs both runners and a serialized checkpoint round trip.
• tests/test_run_impl_resume_paths.py: held validation (non-bool, held with
  before) and the no-marker compatibility pin.
• A mutation pass over the new logic: each registration site, each settle, each
  extend, each discard, the pairing guard and its pending-approval exemption, the
  session-identity check, the bridge, the slot release, the signature probe, the
  emptied-turn settle, the compaction predicate, the settled count and the max-turn
  discard ordering, every one proven to turn at least one test red.
• make format, make lint, make typecheck, make tests via
  .agents/skills/code-change-verification/scripts/run.sh.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6e52216fa5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3730311a98

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9ba9fefa94

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d93e3bf76a

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c6e6470a2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b09d8d14a2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[sylvesterkaczmarek]

I don’t think (type, call_id) is collision-free across the Session. Custom model providers can reuse a call ID on a later turn; if an older matching call is still in this tail window, present suppresses the current deferred call and its output can again be persisted without its call. Could this key include a response/turn identity, or otherwise scope the match to the current response instead of treating call_id as globally unique?

[dixso]

@sylvesterkaczmarek Reproduced before answering, and it fails exactly as you describe.

With call_1 already present in the Session tail from an earlier turn, and the current deferred response emitting call_1 again, deferred_interrupted_session_prefix returns []. The new call is suppressed while its output can still be persisted, recreating the exact orphan this PR is meant to prevent, reached through a different path.

tests/test_tool_approval_call_id_reuse.py also suggests the repo already treats call ID reuse as a real case, so I don't think we can rely on (type, call_id) being unique enough for reconciliation.

Scoping the match to the current response is the right direction, but I couldn't find a reliable way to do that from Session history alone. A Session persists a flat sequence of function_call / function_call_output items, with no marker identifying which model response or turn produced them. So adding response or turn identity to the key doesn't help unless that identity is persisted too, which feels like a broader Session contract change rather than something this fix should introduce implicitly.

I also tested the narrower alternative of matching the entire converted prefix as an ordered block instead of matching individual items. That fixes the collision case, but breaks partial writes: if an earlier attempt persisted the calls but failed before persisting the output, the full prefix no longer matches and the calls are appended again.

That seems to be the recurring signal from the edge cases on this PR: we're trying to answer "was this batch already written?" from Session history, but Session history doesn't contain enough provenance to answer that reliably.

So I think the cleaner direction is to stop inferring it.

RunState._pending_session_write already represents almost exactly what we need: a canonical session append that is pending acknowledgement. It's serialized by to_json, restored by from_json, and reconciled in order by resume_pending_session_write.

I prototyped changing the deferred park so that it records the withheld batch as the pending session write rather than dropping it and reconstructing it later. On resume, we then reconcile a declared batch instead of guessing from history. That removes the id-reuse, partial-write, detached-resume and cancellation cases I was able to construct, and actually deletes a fair amount of the reconciliation logic added by this PR.

There are two semantics I don't want to choose on behalf of the maintainers, though:

1. RunResult.to_state() creates a fresh RunState through _populate_state_from_result, and _pending_session_write only survives when the result already carries a _state. So the first park currently loses it. We'd need to propagate it through the result, similar to _current_turn_persisted_item_count.

2. Settling that deferred batch makes _current_turn_persisted_item_count > 0, which then triggers Cannot resume an approval checkpoint with output guardrails after current-turn items were persisted. Whether settling the deferred batch should count toward that guard is really a question about the intended invariant.

Full write-up and reproducer are in #4827.

I can push the prototype to this PR, open it separately, or hand the approach over if you'd rather own that shape. If you'd prefer to keep #4828 narrow and land the deeper persistence change separately, I can also just adjust the key here.

[sylvesterkaczmarek]

I think the non-streaming loop still has the deferred-prefix loss case. On NextStepRunAgain, it clears deferred_session_prefix even when turn_session_items is empty and therefore nothing was persisted; the streamed loop now guards that case. Could the non-streaming path keep the deferred prefix until at least one resolved turn item is actually saved?

[dixso]

@sylvesterkaczmarek I reproduced this before answering, and the result is clearer than I expected.

I built the empty-resolved-turn case end to end: park a gated call, approve it, then resolve into a turn whose session items are emptied by a handoff input_filter. I ran it on both runners. They already behave identically, and identically badly: neither writes a dangling call, but both lose the deferred batch completely. The approved tool executes, yet neither its call nor its output reaches the Session.

streamed  items=[('user',''), ('message','')]  call_PARKED present: False
nostream  items=[('user',''), ('message','')]  call_PARKED present: False

I then traced the non-streaming loop for that exact run. The NextStepRunAgain check at run.py:1264 is evaluated, but the clear at :1267 never executes because the emptied turn resolves into NextStepHandoff, not RunAgain.

I also couldn't construct a RunAgain case with empty turn_session_items: approving or rejecting an interruption always produces an output item, while the only path I found that empties the turn is a handoff filter, which takes the other branch.

Finally, as a mutation test, I deleted the clear entirely and re-ran the eight reproducers from this PR plus the full suite. The outcomes were byte-identical.

The variable only lives for a single resume pass: that branch is entered while _current_step is an interruption, and update_run_state_after_resume replaces it before the loop continues. Nothing reads the variable after that pass, and a later resume recomputes it from state.

So my earlier comment on that line ("later turns of this run must not re-send it") was incorrect.

The loss your review points out is real, but it isn't reachable through that clear, and retaining the variable can't bridge it. Within the pass there is no later consumer; across runs, the only carrier is reconciliation against Session history, which your collision finding already showed can't be made reliable.

The one place the batch survives all of this is the serialized checkpoint. That's the _pending_session_write direction in #4827, pending the two maintainer decisions listed there.

What I did push is a regression test (23124284) pinning what we can verify: an emptied resolved turn writes no dangling call and no orphaned output in either runner, and both runners produce identical Session contents in that shape. It's proven red against 9ba9fefa, where the streamed path wrote both calls dangling.

I left the clear itself alone. Deleting it changes nothing measurable, and changing dead code as if it fixed this would be misleading.

If a maintainer weighs in on #4827, I'll finish the declarative version, which makes this whole family of cases unreachable.

[seratch]

The ordinary orphan-output case is real, but the current prefix-inference approach still has supported resume failures: the lookup happens after approved tool execution, the streamed lookup drops the context wrapper, legacy no-argument Sessions fail, and finalization/reconnect can duplicate or lose the batch. I recommend redesigning around the deferred response's durable ownership instead of adding another inference branch. Reusing pending-write recovery must also preserve the output-guardrail persistence gate; eagerly writing the held prefix before that gate is not a safe replacement.

[dixso]

@seratch

Thanks for the direction, it was the right call. I have replaced the reconciliation
approach entirely with durable ownership on the checkpoint, as you suggested.

The park now registers the withheld batch on the existing
RunState._pending_session_write slot with a held marker. Registering touches only
the checkpoint, so the output-guardrail persistence gate is fully preserved; the batch
settles at a gate-legal exit of a later resume, landing ahead of the resolved turn's
items in one ordered append and inheriting the existing digest-based crash recovery.
The lookup, the identity heuristics, and the lookback constant are deleted.

Each of your four points now fails by construction rather than by inference: there is
no post-execution lookup, every settle flows through wrapper-carrying machinery, the
legacy-session read shape is gone (plus a signature probe for the settle's inherited
limit= reads), and the declaration survives serialization, the run-again flip, and
detached reconnects, with explicit settle, extend, or discard at every exit.

The PR description is rewritten with the full design, the serialized-state note
(held extends unreleased 1.17, following the same pattern as the commit that
introduced pending_session_write), and the decisions I took with their rationale.
The new tests drive every scenario through both runners and a serialized round trip,
and I ran a mutation pass proving each new guard turns at least one test red.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 47bfebebf9

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

openai-agents-python/src/agents/run_internal/session_persistence.py

Lines 795 to 799 in fe5790e

	resumed_write_state=(
	run_state
	if run_state is not None
	and isinstance(run_state._current_step, NextStepRunAgain | NextStepInterruption)
	else None

Arm recovery before settling a final held batch

When a streamed approval resume resolves directly to NextStepFinalOutput, _save_resumed_stream_items has already removed the held batch via take_held_session_write, but this condition excludes the final step from resumed_write_state. A Session append failure or lost acknowledgement therefore leaves no pending write in the checkpoint; retrying skips the completed tool while its call/output exchange remains absent from the Session. Route final settlement through the pending-write recovery path before clearing the held record.

AGENTS.md reference: AGENTS.md:L104-L104

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f007612498

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b470baec5c

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Checkpoint held outputs at the commit boundary

When an approved tool finishes but a supported post-output callback such as custom_data_extractor or on_tool_end raises, tool_output_committer has already recorded the output and marked the invocation complete, but resolve_interrupted_turn() exits before this later held-batch extension runs. Retrying skips the completed invocation, treats its output as an existing pre-step item, and produces no new session items, so the empty-turn branch discards the still call-only held batch and the Session permanently misses both the call and output. Fresh evidence beyond the earlier pre-execution reconciliation thread is the failure window after the output commit but before this extension; fold committed outputs into the held record at the commit boundary.

AGENTS.md reference: AGENTS.md:L104-L104

Useful? React with 👍 / 👎.

[dixso]

Confirmed reachable and left as a declared limitation, with the reasoning. I reproduced it: an on_tool_end callback that raises after the approved tool committed its output leaves the run at a point where a retry skips the completed invocation, the resolved turn is empty, and the call-only held batch is discarded, losing both call and output.

The correct fix is the one you name: fold the committed output into the held record at the commit boundary, inside tool_output_committer. That is outside this PR's surface (the committer and resolve_interrupted_turn are not touched here), and doing it safely means moving the batch extension from the turn exit into the commit path, which changes the commit ordering for every resumed tool, not just the deferred one. I would rather land that as its own change with its own tests than graft it onto this PR's settle logic, where I cannot prove it crash-safe against the other commit-boundary callers.

This is the same class as the final-settle crash window already documented in the description: a user callback failing inside the commit-to-extension gap. I have added it to the Known limitations so it is explicit rather than silent. Happy to open the follow-up if you agree with the placement.

[dixso]

Fixed rather than left as the declared limitation. The resumed turn's output committer now folds the committed output into the held batch as it commits it, so a post-output callback that raises (custom_data_extractor, on_tool_end) cannot leave a retry that skips the completed invocation and drops the executed call and its result. Reproduced before the fix (the batch held the call alone and the pair vanished from history) and pinned afterwards by test_a_post_output_callback_failure_keeps_the_executed_output in both runners, mutation-proven red.

[chatgpt-codex-connector]

💡 Codex Review

openai-agents-python/src/agents/run_internal/session_persistence.py

Lines 703 to 710 in 90868a8

	resumed_write_state._pending_session_write = {
	"session_id": session.session_id,
	"items": copy.deepcopy(items_to_save),
	"before": None,
	"persisted_count": (
	resumed_write_state._current_turn_persisted_item_count + saved_run_items_count
	),
	}

Include held items in the pending write recovery count

When a held multi-approval checkpoint is resumed with the persistence gate off but without a new approval decision, the approval placeholders trigger settlement but convert to zero new_items, while the held calls are passed through original_input; consequently saved_run_items_count is zero and this pending record also stores a zero persisted count. If the append fails or loses its acknowledgement, resume_pending_session_write() reconciles the held calls but restores that zero count, so a later gate-enabled resume can pass the output-guardrail safety check and append those calls again. Fresh evidence beyond the prior successful-settle counting thread is that save_resumed_turn_items() adds len(held_input) only after success, leaving this failure-recovery metadata uncorrected.

AGENTS.md reference: AGENTS.md:L104-L104

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 821afdc3f7

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[sylvesterkaczmarek]

I don't think the current head is ready to approve yet. There are still unresolved correctness/compatibility issues on the current diff: the new pending_session_write.held serialization needs schema-version treatment consistent with the released reader contract; held-write reattachment bypasses the Responses compaction bookkeeping used by canonical persistence; and the streaming max-turn-handler terminal path can retain a held write after reporting completion. The post-tool callback failure window documented in the open thread is also a real durability gap at the commit boundary. Please resolve the remaining current-head threads, or narrow/document the compatibility contract sufficiently, before re-requesting review.

[dixso]

Thanks, that was a precise list. All four are addressed on the current head, each with a regression test proven red against the previous commit.

Schema. You and Codex were right and my reasoning was wrong: I leaned on 1.17 being unreleased, but the 1.17 reader validates the pending write by exact key set, so a checkpoint written under that label with the extra keys is not loadable by a 1.17 reader, and my own regression called the four-key form released behaviour. CURRENT_SCHEMA_VERSION is now 1.18, 1.17 keeps its four-key form and its original summary, the held keys are gated to 1.18, and the corpus fixtures, sources, README and version-boundary test are updated. Both directions are pinned: a 1.17 payload still restores and settles eagerly, and the held variant under a 1.17 label is refused.

Compaction bookkeeping. Fixed at the root rather than patched: the entry settle no longer appends behind the canonical path, it goes through save_result_to_session like every other settle, so it inherits the Conversations sanitization, the ordered dedup, the pending-write registration and the compaction bookkeeping. That needed the response the batch belongs to, which the park now records (the second key the version bump covers). Pinned by a test that asserts a compaction-aware backend receives the bookkeeping for the parked response.

Max-turn terminal path. Both runners now discard a still-standing held batch when a max-turn handler ends the run, so the finished run's checkpoint stays loadable and the streaming result no longer reports terminal output while carrying a resumable pending write the non-streaming result had already dropped.

Post-tool callback window. Fixed rather than documented: the resumed turn's output committer folds the committed output into the held batch as it commits it, so a callback that raises afterwards cannot leave a retry that skips the completed invocation and drops the executed call and its result.

One more round, self-inflicted. Reviewing my own diff afterwards turned up four defects it had introduced, all fixed on this head with a test proven red against the previous commit:

• The resumed turn's committer folds the executed output into the held batch (the post-tool fix above), which made a wholesale discard on an emptied resolved turn wrong: it threw away a fully paired call and output, so the Session lost its only record that the approved tool ran and the next run would re-issue the side effect. The emptied turn now settles the batch's paired part and drops only the unpaired requests, which is the predicate every other settle already uses.
• Widening the compaction check to see the settling batch made it read the whole original_input slot, which carries the caller's own input on an ordinary save. It now reads that slot only on the calls that actually settle a batch.
• The settled count added the batch's raw length, overcounting whatever the dedup dropped. The append reports what it wrote, because that count slices a later save of the same turn positionally.
• The max-turn discard ran before validate_handler_final_output, so a wrongly typed handler output lost a batch the streamed runner keeps. It now sits below the validation.

Verification on this head: full suite green (9471 passed) except one pre-existing sandbox failure that also fails here with this branch stashed, mypy and mypy --platform win32 clean, pyright clean, and a mutation pass in which every new guard was proven to turn a test red.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 14c8315506

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve held writes until max-turn finalization succeeds

When a detached approval resume carries a held batch into a max-turn handler and a final-output hook or output guardrail raises, this consumes the declaration before either fallible stage runs. No terminal result is produced, but a retry or reattachment can no longer settle the already-executed tool's call/output into the original Session. Clear the held write only after successful finalization, while handling an intentional tripwire disposition separately.

AGENTS.md reference: AGENTS.md:L104-L105

Useful? React with 👍 / 👎.

[dixso]

Fixed, and the fix is the class rather than this site. Confirmed first: with a final-output hook that raises, the batch was gone and the run produced no terminal result, so a retry or reattachment could no longer settle the executed tool's call and output.

The rule now is that choosing a terminal step is not ending the run. finalize_max_turns_handler_output disposes of the batch only after the validation, the hooks and the guardrails have all completed, and an OutputGuardrailTripwireTriggered disposes of it separately as the decided blocked outcome it is, exactly as you suggested.

I inventoried the nine sites that consume the batch instead of fixing only the two you flagged. Four carried this ordering: this one, the detached final output in both runners, and the detached final output on the resumed streamed loop, which nothing had flagged and which is the very path this PR exists for. All four now dispose after finalization. The remaining five follow an outcome that is already decided (an input-guardrail trip, a blocked-output redaction) and are unchanged on purpose.

Pinned by test_a_failed_max_turns_finalization_keeps_the_held_record, mutation-proven red.

[chatgpt-codex-connector]

Delay detached write disposal until final output is finalized

When a detached resumed run reaches NextStepFinalOutput, this clears the held call/output before _finalize_streamed_final_output() runs its hooks and output guardrails. If either raises, stream_events() reports failure rather than completion, yet the live checkpoint has lost the only batch that a later attached retry could persist; the non-streaming detached-final branch has the same ordering. Dispose the batch only after successful completion or an explicit blocked-output outcome.

AGENTS.md reference: AGENTS.md:L104-L105

Useful? React with 👍 / 👎.

[dixso]

Fixed in both runners, and reproduced before answering: with an output guardrail that raises, the batch was gone on the streamed detached completion and on the non-streaming one, exactly as you describe.

Both now dispose of the batch after _finalize_streamed_final_output (and after the guardrails and the final save on the non-streaming side) rather than before. A fourth site had the same ordering and nothing had flagged it, the detached final output on the resumed streamed loop, which is the path this PR exists for; it is fixed with the rest.

One honest note on evidence. Three of the four are pinned by tests proven red against the previous ordering. The fourth, the main streaming loop's detached completion, is aligned for consistency but I could not build a scenario that distinguishes it: measured, that site only executes on the success path, because a failing finalization leaves through the resumed loop's site instead. I am not claiming a test covers it.

Pinned by test_a_failed_detached_completion_keeps_the_held_record, parameterized over both runners and over a final output on the resumed turn and on a later turn, because the first shape alone left the non-streaming site unmutated.

[chatgpt-codex-connector]

Preserve a held null store setting across re-parks

When the original parked response used the ordinary store=None setting and a partial-approval resume re-interrupts under store=False or store=True, this truthiness-based selection overwrites the park-time value even though response_id still identifies the original response. A later OpenAIResponsesCompactionSession settle therefore resolves compaction mode using the resume's storage setting—for example forcing input-mode compaction for an originally default-stored response. Preserve the existing key even when its value is None; only use the new value when the pending record lacks the key.

AGENTS.md reference: AGENTS.md:L102-L102

Useful? React with 👍 / 👎.

[dixso]

Correct, and this one is worth recording because I had ruled it out for the wrong reason. I checked this merge earlier by varying response_id only, found that the pending record was always absent at the re-park in the scenarios I ran, and concluded the branch was unreachable. Varying store is the case I did not run, and it reaches it exactly as you describe.

Measured on all four combinations: a park under store=None was overwritten by a re-interruption's False or True, while False and True parks were preserved. So the settle would resolve the original response's compaction mode from the resuming turn's storage setting.

Presence of the key decides now, not its truthiness, and response_id follows the same rule for the same reason: the batch belongs to the parked response, so both of that response's settle inputs come from the park. Pinned by test_a_re_park_keeps_the_storage_setting_the_response_was_produced_under across None, False and True, mutation-proven red on the None case, which is the only one the old code got wrong.

[chatgpt-codex-connector]

💡 Codex Review

openai-agents-python/src/agents/run_internal/session_persistence.py

Line 765 in e4ce522

return saved_run_items_count

Include settled items in the compaction return count

When a partial-approval turn settles a held call/output batch into an OpenAIResponsesCompactionSession, this local-output branch returns only saved_run_items_count, omitting the settled_batch_items that were appended. The current persisted count can therefore remain zero; if the caller later re-enables the output-guardrail gate and approves the remaining call, the resumed-safety check permits the final sweep to append the already-stored calls again, corrupting Session history. Fresh evidence beyond the earlier count thread is that this compaction-only return bypasses the corrected common return on line 790. Return the combined count here as well.

AGENTS.md reference: AGENTS.md:L102-L102

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[chatgpt-codex-connector]

Preserve a resumable step when detached finalization fails

When a detached approval resume reaches NextStepFinalOutput and a final-output hook or output guardrail raises, delaying this disposal retains the held batch but leaves _current_step as NextStepFinalOutput. That step serializes as null, after which RunState.from_json() rejects the held pending write because it only accepts one alongside NextStepRunAgain or NextStepInterruption; even a live reattachment does not settle it because the held-entry path only settles NextStepRunAgain. Fresh evidence beyond the earlier disposal-order thread is that the retained state is therefore not actually retryable or reattachable. Restore a supported resumable step on failed finalization, or serialize and consume the terminal step explicitly.

AGENTS.md reference: AGENTS.md:L104-L104

Useful? React with 👍 / 👎.

[dixso]

Thank you for pushing on the retry semantics here: it made me verify a property I had only reasoned about, which is worth it regardless of the outcome. I tried to reproduce this before changing anything, and I could not make the premise hold on this head. Measured in both runners, with both failure shapes (an output guardrail that raises, and an on_agent_end hook that raises) on a detached approval resume:

• _current_step after the failure is NextStepRunAgain, not NextStepFinalOutput: the resumed loop resets the step to run-again when it re-enters the turn, and a failed finalization never reaches the _current_step = None assignment.
• The serialized checkpoint carries {"type": "next_step_run_again"} and RunState.from_json() loads it: the held validator accepts a run-again step.
• The reattachment then settles the pair: reattaching the loaded state against the original Session wrote the parked function_call and its output at entry, which is precisely the held-entry rule (a run-again checkpoint only exists once the parked response's outputs went back to the model, so the gate has expired by construction).

So the retained state is both loadable and settleable, in both runners. If there is a path that reaches this disposal with a step that serializes as null, I would genuinely like to see it and will fix it, but I could not construct one.

[chatgpt-codex-connector]

Defer compaction for held MCP approval responses

When a held hosted-MCP approval settles into an OpenAIResponsesCompactionSession, mcp_approval_response is absent from this set because it is not part of _TOOL_CALL_TO_OUTPUT_TYPE. The settle consequently runs compaction immediately instead of deferring it; in previous_response_id mode the compaction request uses only the parked response ID and then replaces the underlying Session, dropping the locally appended approval response before the next model request can consume it, so the approved hosted call may not proceed or may be requested again. Fresh evidence beyond the earlier held-compaction thread is that the new classification covers mapped tool outputs but omits the separately supported MCP approval pair. Include the MCP approval response in the local continuation classification.

AGENTS.md reference: AGENTS.md:L102-L102

Useful? React with 👍 / 👎.

[dixso]

Confirmed by probe before answering: a held MCP approval pair settling into a compaction-aware session ran run_compaction immediately (deferred=[], compacted=[resp]), exactly as you describe.

Fixed in both carriers rather than only the settled dict, because the classification has two consumption paths and fixing one would leave them inconsistent: the constant is now _LOCAL_CONTINUATION_OUTPUT_TYPES (the canonical call-to-output kinds plus mcp_approval_response, with the comment stating why the approval response is the locally produced half of its pair), and the run-item check alongside it now recognizes MCPApprovalResponseItem, which the non-deferred resume commits through new_items with the same association requirement. Both pinned (test_a_held_mcp_approval_pair_defers_compaction_when_it_settles, test_an_ordinary_mcp_approval_response_defers_compaction_too), each mutation-proven red against its own carrier.