Conversation
![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".
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".
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".
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".
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".
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".
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?
|
@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 I then traced the non-streaming loop for that exact run. The I also couldn't construct a 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 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 What I did push is a regression test ( 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
left a comment
seratch
left a comment
There was a problem hiding this comment.
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.
2312428 to
47bfebe
Compare
dixso
changed the title
|
Thanks for the direction, it was the right call. I have replaced the reconciliation The park now registers the withheld batch on the existing Each of your four points now fails by construction rather than by inference: there is The PR description is rewritten with the full design, the serialized-state note |
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 Codex Review
openai-agents-python/src/agents/run_internal/session_persistence.py
Lines 795 to 799 in fe5790e
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".
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 Codex Review
openai-agents-python/src/agents/run_internal/session_persistence.py
Lines 703 to 710 in 90868a8
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".
There was a problem hiding this comment.
💡 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
left a comment
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
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.
|
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. Compaction bookkeeping. Fixed at the root rather than patched: the entry settle no longer appends behind the canonical path, it goes through 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:
Verification on this head: full suite green (9471 passed) except one pre-existing sandbox failure that also fails here with this branch stashed, |
There was a problem hiding this comment.
💡 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".
There was a problem hiding this comment.
💡 Codex Review
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".
…r reads them Main bumped the schema to 1.18 for agent-scoped approvals and MCP recipient bindings while this branch was in review, so the held pending write now shares the unreleased 1.18 label instead of taking a number of its own, the same way 1.17 absorbed the pending Session write before it. The two tests that relabel a current checkpoint as 1.17 must therefore also strip the 1.18-only context and response fields, exactly as main's own relabeling tests do, or the reader rejects the payload before the pending-write validator ever runs.
e222d67 to
46599c9
Compare
|
Rechecked current head |
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Maintainer decision: the need is Demonstrated. #4827 traces an ordinary supported combination—Session persistence, approval-required tools, output guardrails, and non-default tool-use behavior—to a resumed write containing a tool output without its deferred call. This is a durable history defect worth fixing independently of this PR.
Recommendation: Merge-worthy after focused changes. Recording the withheld batch at interruption time and reusing pending-write recovery is the right direction. Manual to_input_list() history management or removing the guardrail/tool-use configuration does not preserve the requested automatic Session behavior. Eagerly writing the withheld batch would violate the persistence gate, while reconstructing ownership from the Session tail repeats the ambiguity rejected earlier in this review.
Two P2 correctness findings remain inline: a rejected approval bypasses current-turn companion filtering, and a held batch spanning two responses duplicates the current response's preamble at terminal settlement. These are additional supported paths through the earlier filter/deduplication invariants, not repetitions of their fixed single-response, approved-tool cases. Please make the shared settlement logic distinguish current-response items from earlier carried history using the existing response boundary, and add public-runner regressions for the two scenarios. Avoid extending the successful-function-output marker into another independent ownership system.
Repository readiness: Rebase or conflict resolution required. GitHub currently reports conflicts. Current main includes #4835, which changes ordinary pending-write metadata and post-write compaction recovery; integrate the held variant with that path during the rebase, then obtain checks on the resulting head.
Reviewed the complete 16-file diff at 46599c9f5c129b46ab6a0919418f35ef1984cc1e, relative to merge base 588826c5be27cad21a3067463e21972ffea38561 with current main, including surrounding execution paths, changed tests, schema compatibility against v0.22.3, prior reviews, and #4827. Two independent fresh-context reviewers examined correctness and design. The issue timeline and bounded duplicate searches found no competing open implementation of #4827.
Validation is desk review only: no PR code, tests, imports, or runtime probes were executed. The current head has no reported check runs or commit statuses; historical test reports are not verification of this head.
jbeckwith-oai
changed the title
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed at e58fb2c. The current-response boundary addresses the rejected-approval filtering and multi-response overlap findings, and the held writes reuse the append-recovery path. I have two structural concerns to address before approval: centralize the repeated resumed-turn persistence decision and decompose the new 2,499-line test module. Inline comments give the narrower changes.
Source-only review of the complete diff and surrounding runner, persistence, state and item-conversion paths, with independent persistence and serialization review. All 21 hosted checks pass on this exact commit, including native macOS and Windows coverage. No repository tests were run locally.
dpiet-oai
left a comment
dpiet-oai
left a comment
There was a problem hiding this comment.
Blocking on the inline High finding.
dpiet-oai
left a comment
dpiet-oai
left a comment
There was a problem hiding this comment.
Blocking on the unresolved High finding already inline: #4828 (comment). The current head still discards the held Session batch after a successful max-turn fallback instead of settling it through the recoverable append path.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Reviewed at b811ef4. The shared resumed-turn persistence operation and six focused test modules address my two earlier structural concerns. The current-response lineage/count handling and hosted MCP checkpoint update fit the existing persistence model. The attached max-turn path settles the approved tool history before the fallback, consistent with the new regression.
One P2 remains in the existing compaction discussion: a same-response approval resume can apply a new run's storage setting to the parked response. I followed up in that thread with the precise path and requested regression.
Source-only review of the pinned diff and relevant runner, persistence, state and compaction paths, with independent persistence and serialization review. All 21 hosted checks pass on this exact commit, including native macOS and Windows coverage. No repository tests were run locally.
dpiet-oai
left a comment
dpiet-oai
left a comment
There was a problem hiding this comment.
[Medium] Preserve the parked response's storage setting when settling
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Re-reviewed 7d13f71, including the changes since b811ef4, the prior feedback and author replies. The original attached approval-resume storage-policy path is fixed, including guarded terminal/redacted exits. One detached, non-streamed path still loses the new response's storage policy; see inline.
The new public compaction regressions exercise the callback's response ID and mode, and the shared settlement/test decomposition from the prior revision remain intact. The remaining case changes the guardrail configuration before the detached re-park, which the new re-park test does not cover.
Validation was source-only with an independent persistence pass. All 21 exact-head hosted checks completed without failures; no local tests were run. I did not reproduce the separately discussed max-turn report and am not repeating it as a finding.
markstuart-oai
left a comment
markstuart-oai
left a comment
There was a problem hiding this comment.
Re-reviewed 1c16e5b2, including the complete PR against its unchanged base and the follow-up to the previous detached-resume finding. The fresh interruption branch now synchronizes the turn before registering held history, so response B retains its own storage policy when guardrails are disabled and the run later reattaches. The extended public regression covers both runners and serialized resumes; same-response approvals still retain the original policy.
No remaining actionable findings. The shared persistence operation and decomposed test suites address the earlier structural concerns, while settlement continues through the existing recoverable append and compaction path.
All 21 hosted checks passed on this commit. Source-only review here, with an independent pass over the turn/state fix; I did not run repository tests or builds locally.
|
Thanks for the quick turnaround on this. The current-response boundary is a cleaner ownership rule than the fold marker, and moving the storage selection ahead of tripwire redaction was a good catch. I verified I couldn't reproduce any duplicates or I ran the original incident three times, then exercised chained approvals, rejection, a page refresh while approval was pending, and approving one sibling while rejecting the other. I did find a few orphaned outputs, but traced those back to our application rather than the SDK. Our code writes a synthetic rejection output without resuming the run, which closes a I also wrote independent probes rather than rerunning the tests from this PR, and mutated the implementation to make sure those probes fail when the relevant guarantees are broken. Two results may be worth calling out:
A couple of limits so this isn't read as broader validation than it is: our application only uses the streamed runner, so non-streamed parity is covered by your tests rather than my application-level verification. This is also one application and one tool shape, and I didn't exercise the sandbox or voice paths. Happy to run anything else against the real stack if it would be useful, including cases that are awkward to reproduce in tests. |
This branch has not been deployed
6 participants
Summary
This pull request fixes missing tool calls in Session history when an approval resumes with output guardrails and a non-default
tool_use_behavior.Withheld response items survive serialized and detached approval resumes until a permitted save. Both runners share the resumed fold/defer/settle operation, preserving accepted history while honoring handoff filtering and output redaction. Local tool outputs and hosted MCP approval responses survive callback failures followed by serialized retry. Settlement reuses append recovery and post-write compaction.
Compaction uses the parked response's recorded storage policy, including when output guardrails block completion. Same-response partial approvals preserve that policy; a new detached park advances response identity and storage policy together. Both runners synchronize the owning turn before fresh interruption registration, including detached resumes where output guardrails have been disabled. Released schema 1.17 remains readable; held-write fields use unreleased schema 1.18. No public API or dependency is added.
Test plan
Issue number
Closes #4827.
Checks
.agents/skills/code-change-verification/scripts/run.sh