fix(sessions): persist deferred interrupted-turn items when the approval resume continues the run - #4828
fix(sessions): persist deferred interrupted-turn items when the approval resume continues the run#4828dixso wants to merge 7 commits into
Conversation
…val resume continues the run With output guardrails and a non-default tool_use_behavior, _should_defer_interrupted_session_items defers the interrupted turn's session items at interruption time. When the approval resume resolves into next_step_run_again (or a handoff), the resume-side write only carried the resolved turn's new items - the tool output - and no later write recovered the deferred function_call. The Session ended up with a function_call_output whose call was never persisted, and the Responses API rejects every later run over that Session with 'No tool call found for function call output'. Persist the deferred prefix (the current response's session items, located via the resumed response boundary) ahead of the resolved turn's items once the resume commits to continuing the run, in both the streamed and non-streamed paths. The final-output path is untouched: its persistence already reconstructs the full current response. A resume that interrupts again keeps deferring.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
Codex review: the selection lived twice, once per resume path, and AGENTS.md wants runtime logic under run_internal. It now lives next to the gate that governs it (_deferred_interrupted_session_prefix in blocked_output.py) and both paths call it.
There was a problem hiding this comment.
💡 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".
Codex review: re-evaluating _should_defer_interrupted_session_items against the live configuration at resume time loses the deferred function_call again when the caller resumes with tool_use_behavior='run_llm_again' (reproduced before changing anything). A non-deferred interruption write bumps _current_turn_persisted_item_count, so persisted_count == 0 identifies the deferred park on its own — the helper now keys on checkpoint state only, which also keeps the prefix empty (no double write) when the interruption-time write actually ran. Two tests: the behavior-change resume, and the non-deferred park not being written twice (mutation-checked: dropping the persisted-count guard turns it red).
There was a problem hiding this comment.
💡 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".
Codex review: a resume can interrupt again (partial approval of a multi-approval response). If the gate no longer defers, that re-interruption write is the deferred prefix's last chance — it bumps the persisted count, so writing only the approved tool's output there orphaned BOTH parked calls for every later resume (reproduced: 2 orphans before this change). The streamed re-interruption branch now prepends the prefix exactly as the non-streaming path already did; a gate that still defers keeps deferring, and the still-deferring variant recovers everything at final output (verified). Regression test proven red against the previous commit.
…every resume exit Three defects in the previous commits, each reproduced before changing anything: 1. A resume whose approved tool IS terminal ends in final output, and _final_turn_items_for_persistence only rebuilds the current response when the agent has output guardrails — a resume may legitimately run without the ones the park had, and the parked function_call was dropped again (both runners). The prefix now rides that exit too. 2. persisted_count can legitimately lie: the resumed-safety validator resets it to zero for a DETACHED resume, and that reset outlives the run, so a later resume reconnecting the original Session rewrote items it already held (duplicate function_calls, measured). The prefix is now CONFIRMED against the Session's own tail using the existing fingerprint helpers, so the write is idempotent by construction and a detached resume degrades to writing nothing. 3. An empty resolved turn (a handoff input_filter can drop every item) must not strand the prefix on its own: a call written without its output poisons the Session exactly as the orphaned output does. It keeps deferring instead, and both runners now agree on that. The helper moves to session_persistence, where the Session read and the fingerprint helpers already live, and becomes async. Five new tests, the three new ones proven red against the previous commit.
There was a problem hiding this comment.
💡 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".
| base_session_items=base_session_items, | ||
| persisted_count=streamed_result._current_turn_persisted_item_count, | ||
| session_start=resumed_response_boundary.session_start, | ||
| reasoning_item_id_policy=streamed_result._reasoning_item_id_policy, | ||
| ) |
There was a problem hiding this comment.
For a context-aware Session, this streaming call omits wrapper=streamed_result.context_wrapper, unlike the non-streaming call and every subsequent streamed write. In the supported detached-resume/reconnect case that resets the persisted counter, the lookup therefore inspects the default scope instead of the active tenant scope; it can conclude that an already persisted prefix is missing and duplicate it, or suppress a required item because another scope happens to contain the same fingerprint.
AGENTS.md reference: AGENTS.md:L147-L148
Useful? React with 👍 / 👎.
| # and would recover the deferred prefix, but WITHOUT | ||
| # them it returns these items verbatim — and a resume | ||
| # may run without the guardrails the park had. | ||
| deferred_session_prefix + list(turn_session_items), |
There was a problem hiding this comment.
When a nondeferred checkpoint was already persisted, a detached partial-approval resume resets the counter, and a later reconnect approves a terminal tool, deferred_interrupted_session_prefix correctly filters the already-present calls from this list. With output guardrails still enabled, however, _final_turn_items_for_persistence reconstructs the complete response from RunState and adds those calls back, so the final append duplicates the previously persisted function calls; the streamed final-output path does the same. Preserve the helper's confirmed-present set through the final guardrail sweep rather than rebuilding those items into the append batch.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
… identity Codex review: filtering the prefix against an unordered set of content fingerprints drops an item that merely LOOKS like one already there. An assistant preamble repeats verbatim across turns, so a tail holding an identical preamble from an EARLIER turn made the current one vanish while its calls were still appended — a legitimate occurrence lost from history (reproduced before changing anything). Matching the whole prefix as an ordered block was the obvious answer and is wrong too: a partially written response (calls persisted by an earlier attempt, output not yet) then matches nothing and duplicates the calls. Measured, both ways. So suppression is now keyed on identity that cannot collide — (type, call_id), unique per turn — and anything without one is kept unconditionally. A partially written response contributes exactly its missing half; nothing is ever dropped for looking familiar. Regression test proven red against the previous commit.
There was a problem hiding this comment.
💡 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".
…efix Codex review: not every item family names its id 'call_id'. A hosted MCP approval request identifies itself with 'id' and its response points back with 'approval_request_id', so _identity_key returned None for both (measured) and a partially written response would append requests the Session already holds — duplicate request ids corrupt the history the next model call reads. The request identity is read through get_hosted_mcp_approval_request_identity, the repository's canonical helper, rather than a local rule. Request and response keep DISTINCT identities (same id, different type), so persisting one never suppresses the other. Items with no collision-free id still return None and are therefore never suppressed. Regression test proven red against the previous commit.
There was a problem hiding this comment.
💡 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".
| present = {key for item in tail if (key := _identity_key(item)) is not None} | ||
| if not present: | ||
| return prefix | ||
| return [item for item, written in paired if _identity_key(written) not in present] |
There was a problem hiding this comment.
When a persisted interrupted response contains an unkeyed item such as an assistant preamble or reasoning item alongside approval calls, a detached partial resume can reset persisted_count before a later resume reconnects the original Session. This return removes the already-present keyed calls but always retains the unkeyed siblings, so the next append duplicates the preamble or reasoning item; duplicated reasoning can make later Responses input invalid. Fresh evidence beyond the earlier ordered-prefix thread is that the current identity-only rewrite preserves every unkeyed item even when the complete current-response prefix is already stored. Detect the fully present ordered prefix first and return nothing before applying partial keyed recovery.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
| await _save_resumed_items( | ||
| list(turn_session_items), | ||
| (deferred_session_prefix + list(turn_session_items)) | ||
| if turn_session_items | ||
| else [], |
There was a problem hiding this comment.
When the last pending approval is resolved by a streamed resume with session=None, this save is necessarily a no-op, but the checkpoint is immediately changed to NextStepRunAgain. If the consumer requests supported after_turn cancellation after receiving the tool event, the serialized checkpoint no longer enters the interruption-resume branch when it is later reattached to the original Session, so neither the deferred call nor its executed output is ever reconciled into that Session. Carry the pending deferred batch across the NextStepRunAgain checkpoint so the next attached resume can persist it before another model turn.
AGENTS.md reference: AGENTS.md:L149-L149
Useful? React with 👍 / 👎.
sylvesterkaczmarek
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
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?
|
@sylvesterkaczmarek Reproduced before answering, and it fails exactly as you describe. With
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 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.
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:
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
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
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?
2 participants
Fixes #4827.
The bug
With both
output_guardrailsand a non-defaulttool_use_behavior,_should_defer_interrupted_session_itemsdefers the interrupted turn's session items atinterruption time (
_finalize_streamed_interruptionpersists[]). When the approval resumeresolves into
next_step_run_again(the approved tool is not terminal) or a handoff, theresume-side write only carried the resolved turn's
new_step_items— the tool output —and no later write recovered the deferred
function_call: the final-output sweep(
_final_turn_items_for_persistence) only reconstructs the final response, which is a latermodel response by then.
The Session ends up with a
function_call_outputwhosefunction_callwas never persisted,and the Responses API rejects every later run over that Session:
Since the orphan is durable, the conversation is permanently dead. 0.21.1 was correct (the
call was written at interruption time); 0.22.0 hid this path behind #4611; #4613 unblocks the
resume and exposes it.
The fix
Once the resume commits to continuing the run (run-again / handoff), persist the deferred
prefix — the current response's session items, located via the already-computed resumed
response boundary (
resumed_response_boundary.session_start) — ahead of the resolved turn'sitems, in the same write. Both the streamed (
run_loop.py) and non-streamed (run.py) resumepaths get the mirror change.
Deliberately untouched:
_final_turn_items_for_persistencealready reconstructs the fullcurrent response (processed items + run-state suffixes, deduped by identity), so a resume
that lands directly on a terminal output persists the deferred items today.
_should_defer_interrupted_session_itemsto approvals whose tool can actually becometerminal output, but that changes park-time semantics for the custom-callable
tool_use_behaviorcase; recovering on resume is strictly additive.Testing
tests/test_deferred_interrupted_session_write.py, red onmainbefore the fixwith exactly the orphaned output, green after. It round-trips the
RunStatethrough JSONbetween the two runs, as any app that parks approvals in an external store must.
git metadata, legacy-httpx websocket cases) are byte-identical to a clean
mainrun underthe same environment.
ruff format --check,ruff check,check_optional_truthiness, andmypyclean on thechanged files.
StopAtTools, MCP-generatedneeds_approvaltools, a Redis-parkedRunStateresumed in aseparate process, and an HTTP-backed Session): before the fix the approving turn orphaned
the call and every later turn failed with the 400; after it, the pair is complete and the
conversation continues. Happy to share details on that setup if useful.