Repository navigation
fix(sessions): persist deferred approval history across resumes - #4828
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".
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. |
de0e92d
Head branch was pushed to by a user without write access
dpiet-oai
left a comment
dpiet-oai
left a comment
There was a problem hiding this comment.
Reviewed de0e92d. No actionable correctness or compatibility findings in this bounded static review.
The streamed terminal-retry path now settles held history even when no new items are produced, after the persistence and guardrail gates. The regression covers serialized callback-failure recovery, retained call/output history, and a loadable completed checkpoint. The storage-policy and detached-turn fixes preserve response ownership, and held-write fields remain gated to schema 1.18 while 1.17 remains readable.
Scope: changed runtime paths for held-history registration, both runners' settlement, callback-failure checkpointing, handoff filtering, storage policy, append recovery and schema validation, with representative regression coverage. Tests and builds were not run locally; no live backend checks were performed.
Hosted evidence includes successful exact-head Tests, CodeQL and Release Readiness runs, but newer runs for the same head are action_required. This approval is a source-review conclusion and does not assert that all current CI gates are green.
|
Thanks for the update. The held-write approach addresses the missing call/output history, but v0.23.0 has now shipped with RunState schema 1.18. Please move the new held-write format to schema 1.19 and update the schema summary, fixtures, generator, and version assertions accordingly. Split validation so the compaction metadata already released in 1.18 remains accepted under 1.18, while Please add compatibility coverage using a checkpoint emitted by the v0.23.0 writer, alongside coverage showing that held-write checkpoints emit 1.19 and cannot be relabeled as 1.18. The released 1.18 reader rejects these new fields, so extending that schema under the same label would misidentify the durable format. If other maintainers persuades this PR w/ co-authored-by credit of the original contributor, that should be fine too. |
v0.23.0 released schema 1.18 with the compaction metadata on the pending write and a reader that validates it by exact key set. A held record under that label is therefore rejected by the released reader, so these tests pin the contract seratch asked for: compaction metadata keeps loading under 1.18, a held checkpoint emits the version that introduced the held keys, and relabeling it as 1.18 or 1.17 is refused. The emit and relabel cases are red on the current head.
…lit validator v0.23.0 shipped RunState schema 1.18 with the compaction metadata on the pending write, and its reader validates that object by exact key set. The held keys arrived after that release, so writing them under the 1.18 label emits checkpoints the released reader rejects and misidentifies the durable format. The held pending write now introduces 1.19. The validator gates each key to the version that introduced it: the four base keys from 1.17, the compaction metadata from 1.18, and held, reasoning_item_id_policy and current_response from 1.19, so a released 1.18 payload keeps loading while a held record is refused under any older label. The 1.18 summary is restored to the text v0.23.0 published and 1.19 carries the held write. Version assertions compare against CURRENT_SCHEMA_VERSION or the version tuple instead of a 1.18 literal, and the corpus generator records v0.23.0 as the historical 1.18 writer and this commit as the 1.19 writer; the regenerated fixtures follow in the next commit.
The corpus now carries 1.18 fixtures emitted by the v0.23.0 tag itself, both the minimal payload and a pending write holding the compaction metadata that release published, so the reader branch for the released label is covered by historical-writer output instead of a relabeled canonical payload. The held pending write moves to 1.19 fixtures emitted by this branch's own bump commit. Only these entries were regenerated. tests/fixtures/run_state/generate_corpus.py cannot rebuild the whole corpus in this checkout: the pending-approval scenario pins writer 92aa1b9 (2026-08-05) while its code imports agents.testing, which landed in openai#4362 on 2026-08-13, so that scenario fails before it can emit.
The relabel helper rewrote agent-scoped approvals and MCP recipient bindings for every target label, but schema 1.18 supports both. Relabeling a current payload as 1.18 therefore handed the reader a 1.17-shaped payload wearing a newer label, and the test asserting that the released 1.18 format still loads would have stayed green through a regression in that reader. The helper now downgrades only below 1.18, and the held-gate test relabels the batch untouched so the held keys are the only possible reason for the refusal. Both are proven red against a reader whose approval gate moves off 1.18. The generator and corpus README now record that the 1.19 writer SHA lives on this branch and must be re-pinned to the squashed merge commit, the same way 821afdc stopped resolving for the entry this one replaces.
|
Done in The held-write change now introduces schema version 1.19, and the validator gates each key on the version that introduced it: base keys from 1.17, compaction metadata from 1.18, and The corpus now records the v0.23.0 tag as the historical writer for 1.18, so both its minimal and feature fixtures come from actual writer output. Two regressions cover the remaining boundary: a held checkpoint emits the current version, and the same payload relabeled as 1.18 is rejected. I also verified this against the released reader, not just the test suite. With v0.23.0 installed, its own 1.18 checkpoint loads, a 1.19 held checkpoint is rejected as unsupported, and the same checkpoint relabeled as 1.18 is rejected as invalid. The current branch loads the v0.23.0 checkpoint and preserves its compaction metadata. |
|
Additional offline validation of The sequence tested was:
All four combinations of initial runner and approval/rejection passed on this head: resolved call/output pairs were saved in order exactly once in these runs, rejected tools did not execute, and the subsequent model input retained the pair. This exercises persistence across reconstructed objects, not a process crash. In the larger public-Runner matrix, head passed 45 checks; base No additional runtime fix emerged from this scope. Validation used Python 3.14.6, scripted models, SQLite, the head's locked dependency environment, and blocked network access. It does not establish real-provider behavior, crash safety, concurrent exactly-once semantics, or a full-diff approval. AI disclosure: Codex generated and ran these checks; these are automated test observations. |
|
Independent offline confirmation for #4827 and this existing fix, using ScriptedModel with fictional tool results and SQLite sessions reopened from disk. I tested streamed/non-streamed × JSON-round-tripped/in-memory RunState × guardrail/no-guardrail (eight cases). On main Python 3.14.7, SDK source metadata 0.23.1, tracing disabled, network denied, no API keys or calls. This is narrow local persistence evidence; it does not replace the full test suite, CI or broader guardrail/redaction/retry coverage. The exact diagnostic is below. """Offline public Runner/SQLiteSession approval persistence diagnostic."""
import asyncio
import json
import tempfile
from pathlib import Path
from agents import Agent, GuardrailFunctionOutput, Runner, RunState, SQLiteSession, StopAtTools, function_tool, output_guardrail, set_tracing_disabled
from agents.testing import ModelStep, ScriptedModel, assistant_message, function_call
set_tracing_disabled(True)
@function_tool(needs_approval=True)
def write_thing(query: str) -> str:
return f"fictional:{query}"
@function_tool
def look_up(query: str) -> str:
return f"fictional lookup:{query}"
@output_guardrail
async def always_fine(ctx, agent, output):
return GuardrailFunctionOutput(output_info=None, tripwire_triggered=False)
async def run(agent, inp, session, streamed):
if not streamed:
return await Runner.run(agent, inp, session=session)
result = Runner.run_streamed(agent, inp, session=session)
async for _ in result.stream_events():
pass
return result
async def case(streamed, serialized, guarded):
model = ScriptedModel([
ModelStep(output=[function_call('look_up', {'query':'x'}, call_id='call_LOOKUP')]),
ModelStep(output=[function_call('write_thing', {'query':'x'}, call_id='call_APPROVAL')]),
ModelStep(output=[assistant_message('done')]),
])
agent = Agent(name='fictional', model=model, tools=[look_up, write_thing], output_guardrails=[always_fine] if guarded else [], tool_use_behavior=StopAtTools(stop_at_tool_names=['finish']))
with tempfile.TemporaryDirectory() as directory:
database = str(Path(directory)/'session.sqlite')
session = SQLiteSession('fictional', database)
try:
first = await run(agent, 'fictional request', session, streamed)
assert len(first.interruptions)==1
state = first.to_state()
if serialized:
state = await RunState.from_json(agent, json.loads(json.dumps(state.to_json())))
state.approve(state.get_interruptions()[0])
final = await run(agent, state, session, streamed)
assert final.final_output == 'done'
finally:
session.close()
reader = SQLiteSession('fictional', database)
try:
items = await reader.get_items()
finally:
reader.close()
calls = {i.get('call_id') for i in items if i.get('type')=='function_call'}
orphans = [i.get('call_id') for i in items if i.get('type')=='function_call_output' and i.get('call_id') not in calls]
return dict(streamed=streamed, serialized=serialized, output_guardrail=guarded, orphan_call_ids=orphans, persisted_types=[i.get('type') or i.get('role') for i in items])
async def main():
rows = []
for guarded in (False, True):
for streamed in (False, True):
for serialized in (False, True):
rows.append(await case(streamed, serialized, guarded))
report = {'cases':rows, 'orphan_cases':sum(bool(r['orphan_call_ids']) for r in rows), 'live_api_calls':0, 'scope':'ScriptedModel with fictional tool results and temporary SQLite databases reopened for persistence checks.'}
print(json.dumps(report))
asyncio.run(main())Diagnostic assistance: prepared with Codex and independently executed against the pinned source; all inputs are fictional. |
dpiet-oai
left a comment
dpiet-oai
left a comment
There was a problem hiding this comment.
Reviewed e63e1af against merge base d7e52c3. No actionable correctness or compatibility findings in this bounded static review of the complete diff and relevant runtime context.
Both runners preserve deferred approval history through serialized and detached resumes while retaining handoff filtering, current-response redaction, recoverable settlement and response-owned storage policy. Callback recovery records committed outputs in model order; the regression deliberately reverses completion order and verifies ordered Session history after serialized retry in both runners. Schema 1.19 correctly separates the held-write fields from released 1.18 compaction metadata.
Source-only review. No code, tests, builds or live backend probes were executed locally. Exact-head hosted Tests and CodeQL workflows passed, including Windows and native macOS coverage. All 21 returned jobs were successful; Release Readiness's substantive validation steps were skipped, so its green result is not evidence of executed release validation.
I am going to land this, and we can deal with aftermath later
8 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.
The branch includes current main. A streamed retry that finishes from a previously committed tool output now settles held history even when the retry produces no new items, and its completed checkpoint remains loadable.
Test plan
Issue number
Closes #4827.
Checks
.agents/skills/code-change-verification/scripts/run.sh