Repository navigation
fix: detach RunState interruption snapshots - #4409
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: 768ba9ad63
ℹ️ 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".
768ba9a to
46b5c92
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46b5c92e45
ℹ️ 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".
46b5c92 to
0a12daa
Compare
|
I found a separate snapshot-isolation gap adjacent to this change. This PR detaches the As a result, two states created from one interrupted result share the SDK-owned approval and invocation ledger: decided = result.to_state()
untouched = result.to_state()
before = untouched.to_json()["context"]["approvals"]
decided.approve(decided.get_interruptions()[0])
assert untouched.to_json()["context"]["approvals"] == before
# Fails: the decision made through decided appears in untouched.On released This is distinct from detaching the public interruption objects: the leaked mutable state is the decision ledger owned by the wrapper. I reproduced it without an API key across non-streamed and streamed results. I am preparing a focused regression test and content-addressed AgentRunProof record; I can send a follow-up fix unless you would prefer this folded into #4409. |
|
Thanks for pointing it out. I'll resolve it in a follow-up PR before making the upcoming release. |
|
Thanks — I’ll avoid opening a competing PR. I have now published a pinned AgentRunProof counterexample for the released
The no-API-key run records both non-streamed and streamed variants. In each, deciding one sibling mutates the untouched sibling, and resuming the untouched state executes the protected tool once. The bundle is tied to the tested wheel/source, reproduced in Linux CI, and can be checked offline with the released I’m keeping a reference fix and a broader regression matrix ready (approve/reject, streamed/non-streamed pause and resume, nested |
This pull request fixes a mutable-aliasing bug at the
RunStateinterruption boundary.RunState.get_interruptions()already returned a new list, but theToolApprovalItemobjects inside that list still shared mutable content with the pending approvals owned byRunState. As a result, callers could unintentionally modify authoritative resumable state by changing fields such astool_name,raw_item.arguments, or nested metadata on a returned interruption.Those mutations could alter the approval identity retained for resume, interfere with direct or nested approval routing, or otherwise make subsequent
approve()andreject()behavior depend on caller-side changes to what was expected to be a snapshot.This change makes
get_interruptions()return fully detachedToolApprovalItemsnapshots while preserving the behavior callers rely on:ToolApprovalItemare independent of the pending approval stored byRunState.Agentis intentionally not copied, preserving its identity.approve()orreject(), including approvals owned by nested agent-tool runs.The supported Pydantic model adapters are derived from
ToolApprovalRawItem, keeping the copy boundary aligned with the production approval pipeline. This includes function, custom, shell, hosted MCP, and local-shell approval payloads, includingResponseCustomToolCallandResponseFunctionShellToolCall.If a payload cannot be cloned safely,
get_interruptions()raises an actionable, payload-safe SDKUserErrorbefore returning any partial result. The implementation does not deep-copyAgentinstances, callbacks, arbitrary run state, or unsupportedBaseModelobjects, and it does not change the serializedRunStateschema.The regression coverage verifies mutation isolation, canonical approve/reject compatibility, nested approval routing, all supported typed approval models, mapping payloads, ambiguous ownership, hostile payload hooks, and all-or-nothing failure behavior. Existing corruption-path tests now mutate authoritative pending state explicitly instead of relying on aliasing through the public snapshot API.