fix(sessions): settle an accepted terminal output after a failed resumed append - #4698
Conversation
…med append A resumed turn that ends via tool_use_behavior appends its terminal batch after the output guardrails, but never armed the pending_session_write checkpoint for it, and NextStepFinalOutput was outside the steps RunState can own or serialize. An append failure therefore left a state that could neither recover nor reject the accepted result: retrying re-entered the model, re-ran the guardrail, repeated the agent lifecycle hooks, and returned a different final output while the Session could permanently lack the tool call/output pair. Let the resumed state own that batch when the accepted output is a plain string, publish the passing output guardrail results before the append can raise, and settle the same output on the next resume before any model call or hook. Richer outputs cannot round-trip through the RunState codec unchanged, so they keep the existing non-resumable behavior. Fixes openai#4690
![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: 9d2958d9ec
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…hat is owed Two review findings shared one cause: the terminal step was treated as settleable on its own, and the check ran too late in the resume. An accepted terminal output is durable only while its append checkpoint is still outstanding. Once resume_pending_session_write() reconciles that batch, later persistence work for it, such as compaction, can still fail, and the cleared checkpoint made the leftover step look settled. A retry then reported a completed run for an output whose required Session maintenance had failed. Split the predicate so arming keeps using the step alone, while settling and serialization also require an outstanding pending write, and capture it before reconciling clears it. The settle also ran after sandbox preparation, so retrying an already accepted output needlessly created and cleaned up a provider sandbox, and a sandbox startup failure could withhold an output that needed no further model or tool work. Move it to the top of the run loop in both runners, ahead of input guardrails and sandbox preparation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9b541f434
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d9b541f434
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…alize path Three review findings landed on the same rule, so this replaces it rather than subtracting another case from it. The checkpoint replays one Session append, so it may only be armed when that append really is all the run has left. Ownership is now decided by terminal_checkpoint_owner() and opted into by the caller, instead of being inferred from the step: - Only the finalize path that saw every output guardrail succeed passes settle_terminal_output. The tripwire, guardrail-error, and max-turns saves reach the same helper and must not arm a checkpoint, because an error there has to surface on the next resume rather than be settled away. - A compaction-aware Session also owes a deferred compaction for the batch, which the checkpoint does not carry. Those runs keep the existing non-resumable behavior instead of settling a batch whose maintenance the replay would skip. resumed_write_owner() now resolves the owning state at the call sites, so save_resumed_turn_items() forwards an already resolved owner and its signature is unchanged.
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
The plain-string terminal recovery addresses the demonstrated failure, including the repeated agent lifecycle hooks. The remaining support boundary is not safe yet.
Structured/custom terminal outputs and compaction-aware Sessions are supported public paths, but the new tests explicitly leave them with the old non-resumable behavior. Retrying those states can still enter the model and run lifecycle hooks again after the original tool effect and terminal-output hooks already completed.
Please use one terminal checkpoint boundary for every post-acceptance Session append failure. A losslessly persisted string can settle the original output. If the output cannot round-trip losslessly, or post-append maintenance cannot be replayed safely, persist an explicit terminal-unrecoverable state and reject resume before sandbox preparation, agent/model/tool/guardrail work, or lifecycle hooks. This should be a fail-closed branch, not another recovery mode.
2 participants
Summary
When a resumed, approval-gated run ends via
tool_use_behavior="stop_on_first_tool", the tool output becomes the terminal agent output. If the final client-managedSession.add_items()append fails, the exception propagates correctly but the resultingRunStatecan neither recover nor explicitly reject that accepted terminal result.Cause
The terminal batch is appended after the output guardrails so a tripwire can still redact it, which makes it the last fallible step of the run.
save_final_turn_items_after_guardrails()never passed aresumed_write_state, so thepending_session_writecheckpoint introduced in fix(sessions): recover failed resumed Session writes before model calls #4630 was not armed for that batch.run_state._current_step = None, so the live step staysNextStepFinalOutput. That type is outside theNextStepInterruption | NextStepRunAgain | NoneunionRunStatedeclares and serializes, so the snapshot recordscurrent_step = null.RunState, so the failed state keeps no evidence that the accepted output already cleared them.These are one defect rather than two:
from_json()already rejects a restoredpending_session_writewhosecurrent_stepis not a resumable step, so arming the checkpoint without also making the terminal step durable would fail every JSON round trip.Retrying the same live or JSON-restored state therefore re-enters the model, evaluates the guardrail against a new final output, repeats the agent lifecycle hooks, and returns a different result, while a fail-before-commit Session permanently lacks the tool call/output pair.
Fix
Treat an accepted terminal output as a durable checkpoint, and settle it on the next resume before any further work. The existing pending-write mechanism carries the exact function-tool batch; no new persistence mechanism, no new step type, and no
RunStateschema bump.The checkpoint replays one Session append, so it may only be armed when that append really is all the run has left. Three helpers keep that rule in one place:
terminal_checkpoint_owner()decides whether the resumed state may own the deferred append. Callers opt in throughsettle_terminal_output, so only the finalize path that saw every output guardrail succeed can arm it.resumed_write_owner()resolves the owning state for a resumed write generally: a run-again or interruption step always owns its write, a terminal step only under the stricter rule above.recoverable_terminal_step()decides whether a resumed state may be settled or serialized, and additionally requires an outstanding_pending_session_write. Reconciling clears that checkpoint, so both runners capture this before callingresume_pending_session_write().Settling happens at the top of the run loop in both runners, ahead of the first-turn input guardrails and
sandbox_runtime.prepare_agent(), so an already accepted output never creates or tears down a provider sandbox and a sandbox startup failure cannot withhold it.Support boundary
A terminal output is only settled when the checkpoint represents everything still owed for the batch:
json.dumps(..., default=str)and thenstr(value), which is a useful diagnostic codec but not an identity-preserving one, so structured and custom outputs keep the existing non-resumable behavior. Checking the accepted value subsumes checkingAgent.output_type, because the runtime already normalizes aNoneorstroutput type tostrbefore the step is built.Live and JSON-restored states apply the same rule, so both have one contract, and a snapshot never advertises a checkpoint it cannot honor.
Behavior
on_agent_start/on_agent_end.RunResult.to_input_list(), the restoredRunState, and the next model input each contain exactly one ordered call/output pair.resume_pending_session_write()checks.RunState.add_input()continues to reject a settled terminal state.Alternatives considered
from_json()rejects a pending write whosecurrent_stepis not resumable, so JSON resume would fail outright, and the retry would still re-enter the model.NextStepRunAgain, as the resumed handoff boundary does in fix(sessions): checkpoint a resumed handoff before the Session append #4689. Ruled out in the issue, and wrong here: understop_on_first_toolthe tool output is the answer, so replaying the model both changes an already accepted result and feeds that output back as model input.NextStepFinalOutput.output: Any. The generic codec cannot preserve type or value, so this would turn a narrow Session repair into an arbitrary-object serialization promise.response_idin the checkpoint and replay_defer_compaction()on resume, instead of declining to arm for compaction-aware Sessions. That widens the checkpoint payload, and the same gap already exists for the run-again and interruption recovery paths from fix(sessions): recover failed resumed Session writes before model calls #4630, so it seemed better raised separately than folded in here. Happy to do it in this PR if maintainers prefer the broader change.Notes for review
1.17is unreleased (the published0.22.0wheel ships1.16), so per.agents/references/runstate-schema.mdthis extends the existing1.17summary instead of bumping to1.18. A restore still fails closed for anext_step_final_outputstep carried on an older schema label. Happy to bump instead.RunResultStreamingis already seeded from the state, and the failed attempt already streamed those items. Deliberate, and easy to change.docs/change here. No published guidance describes this failure boundary, and per the documentation release timing rule any write-up belongs to separately timed docs-only work.Test plan
test_resumed_terminal_output_is_settled_before_next_modelcovers the full sync/stream, same-mode/cross-mode, live/JSON, fail-before-commit/commit-then-raise matrix (16 rows). Alongside it: agent lifecycle hook non-replay across modes, ambiguous-recovery fail-closed (missing, different, and changed Session), structured-output non-resumability, invalid serialized payloads (non-string output and older schema label), added-input rejection, sandbox preparation being skipped for a settled output, a guardrail error not being settled away, and a compaction-aware Session declining to arm a checkpoint.mainata40ae98: 28 failed, 60 passed. With this change: 88 passed.make tests: 7670 passed / 147 skipped parallel, 77 passed / 4 skipped serial. The remaining failures and collection errors in this local environment are pre-existing on unpatchedmain(Windows symlink, tar, and mount sandbox tests, plus uninstalled optional extras), verified by running the identical suite against unpatched source and diffing the results.make format,make lint, andmake mypyare clean on all touched files.make pyrightwas not run locally (Node is unavailable in this environment), so it is worth confirming in CI.Issue number
Fixes #4690
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR