Repository navigation
fix(sessions): reject resuming a run whose accepted terminal output was not persisted - #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.
… restored Structured and custom terminal outputs and compaction-aware Sessions are supported public paths, but they were left with the old non-resumable behavior. Retrying one of those states still entered the model and ran the agent lifecycle hooks again, after the tool effect and the terminal-output hooks had already completed once. That is the reported failure, only in the cases the previous boundary excluded. Every post-acceptance terminal append now takes the same checkpoint, and the checkpoint records which of two outcomes a resume gets: - a losslessly persisted string settles the original output, as before; - anything else persists an explicit terminal-unrecoverable state. That covers an output the codec cannot round-trip and a batch whose post-append maintenance the replay cannot reproduce. An unrecoverable state rejects the resume with an actionable UserError before the pending Session write, sandbox preparation, and any agent, model, tool, guardrail, or lifecycle hook work. It is a fail-closed branch rather than another recovery mode, and it stays closed on every later resume because the marker does not depend on the pending write.
|
Thanks, that boundary was the wrong call and I have replaced it in a9de927. You are right that leaving structured/custom outputs and compaction-aware Sessions on the old path was not safe. I had scoped them out as "not made worse", but they are supported public paths, and retrying one still re-entered the model and repeated There is now one terminal checkpoint for every post-acceptance append, and the checkpoint records which of two outcomes a resume gets. Settle. A losslessly persisted string settles the original output, unchanged from before. Reject. Anything else persists an explicit terminal-unrecoverable state: an output the codec cannot round-trip, and a batch whose post-append maintenance the replay cannot reproduce. Rejection raises an actionable Guardrail failures still do not reach this boundary at all: only the finalize path that saw every output guardrail succeed opts in, so a tripwire or a raised guardrail surfaces on the next resume instead of being checkpointed over. Tests
Each covers sync and streamed, live and JSON-restored states. On One judgment call worth confirming: for compaction-aware Sessions I chose to reject rather than to carry |
|
@codex review |
|
Note You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
seratch
left a comment
seratch
left a comment
There was a problem hiding this comment.
The released bug is demonstrated, but I do not think we should merge the hybrid recovery design. It adds terminal-output persistence, result reconstruction, string-only settlement, compaction-specific behavior, and new control flow in both runners for a narrow failure boundary.
Please reset this to the issue's explicit non-resumable contract:
- mark the RunState as terminal-unrecoverable after final output, output guardrails, and terminal hooks have completed but before the fallible Session append;
- serialize that marker using the unreleased schema boundary;
- reject every later resume before Session reconciliation, sandbox preparation, model calls, tools, guardrails, or hooks;
- clear the marker only after the append and any post-append maintenance complete successfully.
This should remove the string-output settlement path, build_recovered_final_output_result, and the compaction-specific recovery branch. Please retain focused sync/stream and live/JSON tests proving that fail-before-commit and commit-then-raise never repeat lifecycle hooks or model work.
…ession-append-recovery # Conflicts: # src/agents/run_internal/session_persistence.py # tests/test_run_impl_resume_paths.py
…as not persisted Replaces the hybrid recovery design with the issue's explicit non-resumable contract. Terminal-output persistence, result reconstruction, string-only settlement, the compaction-specific branch, and the settlement control flow in both runners are all gone. A resumed turn that ends via tool_use_behavior appends its terminal batch after the output guardrails, so the run owns an accepted result for the whole of that fallible append. If it failed, the exception propagated but nothing recorded that the output, its guardrails, and its terminal hooks had already completed, so retrying the state ran the model again and repeated the agent lifecycle hooks for a result the caller had already received. RunState now carries a terminal_unrecoverable marker. It is set once the final output, its output guardrails, and its terminal hooks have completed but before the fallible Session append, serialized on the unreleased 1.17 schema boundary, and cleared only after that append and any post-append maintenance both succeed. While it is set, every resume is rejected with an actionable UserError raised ahead of Session reconciliation, sandbox preparation, and any model, tool, guardrail, or hook work.
|
Reset to the non-resumable contract in ea43537, and rebuilt on top of current The hybrid design is removed: no terminal-output persistence, no
The source change is +62/-1 across four files, against +877/-30 before. Tests
Three focused cases alongside it:
On |
Ayushraj06-bit
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea435370fb
ℹ️ 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 |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
…turns output Two paths reached an accepted final output without the fail-closed marker following it. A streamed result exists before its terminal append does, so a caller can take a checkpoint from the failed attempt with `to_state()`. `_populate_state_from_result()` copied the pending write and the current step but not the marker, so that checkpoint looked like an ordinary resumable state and skipped the rejection entirely. The non-streamed max-turns handler produces a final output and runs its output guardrails and end hooks, then persists through its own save callback. That append was never marked, so retrying the state ran the handler and its hooks a second time. The streamed max-turns path already inherited the marker from the shared terminal finalizer. Both now arm and clear the same marker around the same post-acceptance append, so every accepted final output has one boundary.
|
Both review findings on Preserve the terminal marker when snapshotting a failed stream. Confirmed: a streamed terminal append failure left the live state marked but Mark max-turn fallback output before persisting it. Confirmed: with a resumed state reaching a The streamed max-turns path already inherited the marker from the shared terminal finalizer, so it needed no change. Verification: the test file is 90 passed here and both new tests fail on |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4847f9d263
ℹ️ 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".
… pass finalize_max_turns_handler_output() drives the same save callback from its guardrail-error path, so marking inside that callback closed the state for an output that never passed its guardrails and never reached the caller. A failed append there rejected every later resume, which is worse than the replay the marker exists to prevent: the run had nothing to protect and no way forward. Arm and clear at the success call site instead, which is only reachable once the handler output cleared its guardrails and ran its end hooks. That matches the streamed finalizer, which already wraps only its post-guardrail save, so both runners now arm on the same condition.
|
Fixed in c31638d, with a regression test that fails on 4847f9d. Confirmed exactly as described: with a The cause was arming inside Arming moved to that success call site, and the callback is unmarked again. This matches the streamed finalizer, which already wraps only its post-guardrail save, so both runners now arm on the same condition. It is also the same rule already applied to the ordinary terminal branches, where the tripwire and guardrail-error saves deliberately arm nothing.
Verification: 91 passed in the test file, full suite 7723 passed / 147 skipped parallel and 77 passed / 4 skipped serial, |
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 resultingRunStatecannot recover or 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. For the whole of that append the run owns an accepted result, but nothing recorded that fact:
run_state._current_step = None, and a terminal step serializes ascurrent_step = null, so the snapshot looks like an ordinary resumable state;Retrying the same live or JSON-restored state therefore re-enters the model, evaluates the guardrails against a new final output, and repeats
on_agent_start/on_agent_endfor a result the caller had already received.Fix
RunStatecarries a single_terminal_unrecoverablemarker, following the issue's explicit non-resumable contract:terminal_unrecoverableon the unreleased 1.17 schema boundary. A snapshot carrying it on an older label is rejected rather than honored.reject_unrecoverable_terminal_state(), raised beforeresume_pending_session_write(), sandbox preparation, and any model, tool, guardrail, or lifecycle hook work, so the rejection has no side effects of its own.to_state()on a failed attempt would otherwise emit a resumable snapshot.This is a fail-closed branch, not a recovery mode. The accepted output is not persisted, no result is reconstructed, and there is no settlement path, so the run reports its failure once and the caller starts a new run rather than silently paying for the tool twice.
Behavior
UserError, for both the atomic-failure and the commit-then-raise (lost acknowledgement) outcomes, and stays closed on repeated attempts.Alternatives considered
NextStepRunAgain, as the resumed handoff boundary now does after fix(sessions): recover resumed handoffs after session append failures #4725. 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.Test plan
test_terminal_session_append_failure_rejects_every_later_resumecovers the sync/stream, same-mode/cross-mode, live/JSON, fail-before-commit/commit-then-raise matrix (16 rows). Each row asserts the tool effect, the model call, andon_agent_endeach happened exactly once during the failing attempt, then resumes twice and asserts both are rejected with none of those counts moving.Alongside it:
test_unrecoverable_terminal_state_rejects_before_any_resumed_workpatchesSession.get_itemsandSandboxRuntime.prepare_agentto raise, proving the rejection precedes Session reconciliation and sandbox preparation.test_failed_stream_result_checkpoint_keeps_the_terminal_markertakes a checkpoint from the failed streamed attempt, since a streamed result exists before its terminal append does, and asserts both it and its JSON round trip stay rejected.test_max_turns_handler_output_is_marked_before_it_is_persistedcovers the max-turns fallback, asserting the handler and its end hook each run exactly once across the failure and the rejected retry.test_terminal_marker_is_cleared_once_the_turn_is_persistedguards the clear path.test_terminal_marker_rejects_an_older_schema_labelcovers the schema boundary.The test file on
main: 21 failed, 69 passed. With this change: 90 passed.make tests: 7722 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).Issue number
Fixes #4690
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR