fix: use structural matching for serialized approval checkpoint boundary - #4612
fix: use structural matching for serialized approval checkpoint boundary#4612szupzj18 wants to merge 6 commits into
Conversation
When an approval checkpoint with output guardrails is serialized and deserialized, all RunItem objects are rebuilt, destroying object identity. The _current_response_boundary function relied on object identity to locate the processed response's items within _generated_items and _session_items, so the boundary could not be proven for turns after the first, raising 'Cannot resume a serialized approval checkpoint with output guardrails'. Add _structural_item_key and _structural_sequence_start helpers that match items by (item_id, item_type, call_id) — the same identifiers used by _merge_generated_items_with_processed. When identity matching fails, fall back to structural matching to locate the processed items within the generated and session item lists. The existing turn-1 type-based prefix fallback is retained as a last resort for items without identifiers. Fixes openai#4611
![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: 05555f0dd9
ℹ️ 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".
| ] | ||
| ) | ||
| session_start = sess_structural | ||
| suffixes.extend(run_state._session_items[session_start:]) |
There was a problem hiding this comment.
When a turn>1 serialized approval is resumed with terminal tool behavior and its output guardrail trips, _generated_items and _session_items contain independently deserialized copies of the same current-response call. This branch appends both suffixes, while the later id(item) deduplication cannot recognize those copies as equivalent; _prepare_blocked_output_snapshot() consequently sees duplicate function calls, abandons the snapshot, and discards the response instead of retaining the blocked call/output pair in session history. Use one canonical matched suffix (while retaining both owner start indexes), or structurally deduplicate the two owner views before building the boundary.
Useful? React with 👍 / 👎.
After serialization, _generated_items and _session_items may contain independently deserialized copies of the same items. The id(item) deduplication cannot recognize those copies as equivalent, allowing duplicate items into the boundary when structural matching appends both suffixes. Track structural keys (item_id, item_type, call_id) alongside object identity in the deduplication loop. Items without identifiers fall back to identity-only deduplication.
|
Good catch. Fixed in 9d1c551: the deduplication loop now tracks structural keys ( All 166 streamed + 464 run_state + 16 guardrail tests pass; lint and typecheck clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d1c551c71
ℹ️ 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".
| if isinstance(raw, dict): | ||
| item_id = raw.get("id") | ||
| item_type = raw.get("type") | ||
| call_id = raw.get("call_id") |
There was a problem hiding this comment.
When a tool or model raw dict contains hash-colliding non-string keys, these dict.get calls invoke their __eq__ hooks, and the unchecked returned values may later invoke __eq__ or __hash__ during deduplication. _current_response_boundary calls this helper before the guarded snapshot/cleanup block, so a hostile hook can raise an exception containing payload data, replace the output-guardrail tripwire, and leave the secret-bearing response graph attached; this also regresses test_blocked_function_batch_ignores_hash_collision_key_hooks. Use the sanitizer's exact-string, hook-free field lookup and validate values before structural matching.
AGENTS.md reference: AGENTS.md:L127-L127
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81100e81c0
ℹ️ 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".
| if id(item) in seen_ids or (key != (None, None, None) and key in seen_keys): | ||
| continue |
There was a problem hiding this comment.
When a custom model returns two distinct items with the same (id, type, call_id)—for example, two output messages reusing an ID—this loop drops the second item even when both originate in processed_items, rather than merely deduplicating independently deserialized owner views. With output guardrails enabled, _final_turn_items_for_persistence() uses this boundary, so the session silently loses part of the model response and subsequent turns receive incomplete history. Preserve distinct occurrences within each source sequence and apply structural deduplication only when merging equivalent generated/session copies.
AGENTS.md reference: AGENTS.md:L166-L166
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 024808854f
ℹ️ 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".
| for start in range(len(container) - len(sequence) + 1): | ||
| if all( | ||
| _structural_item_key(container[start + offset]) == key | ||
| for offset, key in enumerate(sequence_keys) | ||
| ): | ||
| return start |
There was a problem hiding this comment.
When a serialized multi-turn checkpoint contains an earlier response with the same (id, type, call_id) sequence as the interrupted response—an identifier-reuse case the new tests explicitly allow—this forward scan selects the earlier turn. The resulting owner start indexes make final persistence include an old suffix or make blocked-output cleanup truncate from the wrong turn; a passing guardrail can therefore duplicate accepted history, while a tripping guardrail can discard the current sanitized call/output pair. Resolve the latest matching occurrence, or reject the boundary when its ownership remains ambiguous.
AGENTS.md reference: AGENTS.md:L166-L166
Useful? React with 👍 / 👎.
|
Thanks for sharing this patch. The issue was resolved by #4613 |
2 participants
Summary
When an approval checkpoint with output guardrails is serialized and deserialized, all
RunItemobjects are rebuilt, destroying object identity._current_response_boundaryrelied on object identity (_identity_sequence_start) to locate the processed response's items within_generated_itemsand_session_items, so the boundary could not be proven for any turn after the first — raisingUserError: Cannot resume a serialized approval checkpoint with output guardrails.The existing turn-1 type-based prefix fallback did not help because it was gated on
_current_turn == 1, and the processed items are not necessarily a prefix of_generated_itemson later turns (interruption items likeToolApprovalItemare appended after the processed response'snew_items).Change
Add
_structural_item_keyand_structural_sequence_starthelpers that match items by(item_id, item_type, call_id)— the same identifiers used by_merge_generated_items_with_processedfor deduplication. When identity matching fails, fall back to structural matching to locate the processed response's items within the generated and session item lists. The existing turn-1 type-based prefix fallback is retained as a last resort for items without identifiers.Test plan
test_serialized_approval_checkpoint_with_output_guardrail_resumes_after_earlier_turn(parametrized non-streamed/streamed): runs a 3-turn ScriptedModel scenario (normal tool → approval tool interruption → final message), serializes and deserializes theRunState, approves the interruption, and verifies the run completes withfinal_output == "done".test_ambiguous_serialized_approval_state_fails_before_tool_execution: corrupts the processed response's item identifiers after deserialization so structural matching cannot locate them, verifying the safety check still raisesUserErrorwhen the boundary genuinely cannot be proven.make format,make lint,make typecheck— all clean.make tests— parallel suite fully passes; serial suite: 77 passed, 4 skipped, 7 errors (all pre-existing Dapr Redis integration tests requiring Docker/testcontainers, unrelated to this change).Issue number
Fixes #4611
Checks
.agents/skills/code-change-verification/scripts/run.sh/reviewbefore submitting this PR