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.
• Held settlement derives its pairing evidence from the resolved session view, never
  from the batch itself. HandoffInputData.new_items is the session-history axis by
  contract (input_items exists precisely to filter model input while preserving
  history), so an output the commit boundary folded into the batch this turn settles
  only if the filter's view kept it; a removed output stays out of the Session and
  takes its call with it. Carried prior-turn history (a detached carry riding a
  checkpoint) still settles, because a later turn's filter is not entitled to
  unpersist earlier turns, exactly as the eager path cannot.
• 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 released (v0.22.x), so writing it under that label would emit checkpoints they cannot
load. The held keys are therefore gated to 1.18, the unreleased label main introduced for
MCP recipient bindings and agent-scoped approvals while this PR was in review, and the
1.18 summary now names both. This is the same move 1.17 made when it absorbed the pending
Session write before release, and it leaves no gap in the corpus that a private 1.19 would
have opened. 1.17 keeps the four-key form and the summary it shipped with, and 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.

Why the field could not travel before this change

The non-streamed result rebuilds a RunState from _*_for_state shadow attributes
declared on the result (result.py), and _pending_session_write had no shadow
declared at all, unlike _pending_input_for_state. The original omission is a
missing shadow declaration, not a missed assignment in the copy block, which is why
the field mechanically could not ride that path; this PR declares the shadow and
reads it back. Credit for this framing goes to an independent reader of #4827 who
verified the report from source.

Decisions taken with a stated default

• Decision: the held variant rides the unreleased 1.18 label main already introduced
  rather than extending 1.17 in place (the released 1.17 reader validates the pending
  write by exact key set and would reject a checkpoint written under its own label) or
  taking a private 1.19 (which would leave 1.18 without a corpus entry the moment it
  stopped being the current version).
• Decision: the filter's session authority is scoped to one turn and tracked by a
  deliberately transient, per-turn set of folded call ids on the run state (no schema
  change; same lifecycle precedent as _session_write_in_progress). After a
  checkpoint the set is empty on purpose: an output folded by an earlier process is
  carried history, and the reattaching entry settle keeps the carried batch whole. An
  earlier revision settled the batch's paired part regardless of the filter; the
  documented new_items/input_items split is the controlling contract and this
  revision follows it.
• 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]

💡 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]

💡 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".

[sylvesterkaczmarek]

Rechecked current head 46599c9f. The only branch-specific change since my last clean review is the schema-relabel compatibility update: production behavior is unchanged apart from a run-state summary line wrap, while the tests now strip the current 1.18-only approval/MCP fields before exercising 1.17 reader semantics. The held-write/filter behavior I reviewed remains unchanged. No remaining blocker from my review.