fix: match pending nested agent-as-tool runs by call_id on resume#3749
fix: match pending nested agent-as-tool runs by call_id on resume#3749tao-hpu wants to merge 3 commits into
Conversation
_restore_pending_nested_agent_tool_runs paired the unfiltered serialized function entries against the filtered deserialized runs with a positional zip. _deserialize_actions drops entries whose tool no longer resolves or whose tool call fails to parse, so one dropped entry shifted every later pairing: a pending nested interruption could bind to the wrong tool call, or fall off the end and be lost. Match entries to runs by tool_call call_id instead, and add a regression test where a preceding entry's tool has been removed from the agent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4aa4718f78
ℹ️ 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".
Match serialized function entries to deserialized runs by the full tool-call signature the ephemeral agent-tool-state cache keys on, falling back to a per-call_id FIFO queue for drop tolerance. Previously the restore map kept only the first run per call_id, so two surviving agent-as-tool calls sharing a call_id but differing in arguments collapsed onto the first call and the second lost its pending interruption on resume. Add a regression test covering two duplicate-call_id entries with distinct arguments and distinct nested pending states. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 221a01e519
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Fix mypy union-attr errors where LocalShellCall and dict members of the raw_item union have no name attribute. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thank you for identifying this issue and for providing a concrete implementation and tests. Your key observation was exactly right: We decided to proceed with a maintainer-side implementation #3753 that preserves the association at the point of deserialization. Each successfully deserialized function action carries its normalized nested run-state data, rather than reconstructing the association after the original and filtered lists have diverged. We prefer this approach because it:
We also expanded the regression coverage around independent cache scopes, namespaced and deferred tools, multiple pending states, JSON round-trips, and both approval and rejection after restoration. The proposed signature-based implementation is thoughtful and works for the cases it covers, but it repairs the association after deserialization. The maintainer implementation instead preserves the original association through the filtering boundary, which we believe is simpler and less likely to drift as tool-call identity or cache behavior evolves. Given the overlap, we are going to proceed with the maintainer implementation rather than merge this PR. We genuinely appreciate the report, analysis, and implementation idea—they were instrumental in getting this fixed. |
Bug
_restore_pending_nested_agent_tool_runspairs the unfiltered serialized entries (processed_response_data["functions"]) against the filtered deserialized runs with a positionalzip(..., strict=False)._deserialize_actionsdrops an entry when its tool no longer resolves (if not tool: continue) or its tool call fails to parse, so a single dropped entry shifts every later pairing.Trigger: resume a paused run where a function whose tool the resuming agent no longer exposes precedes an agent-as-tool call that carries
agent_run_statewith a pending nested interruption. The pending state then binds to the wrong tool call, or (at the tail) is silently lost, so the nested approval never re-surfaces.Fix
Build a
{tool_call.call_id: run}lookup from the deserialized runs and match each serialized entry by itstool_call.call_idinstead of by position. Behavior for the aligned case is unchanged.Tests
Added
test_restore_pending_nested_run_when_earlier_entry_dropped: a serializedfunctionslist whose first entry references a removed tool, followed by an agent-as-tool entry with a realagent_run_state(built via the existing HITL helpers) carrying a pending approval. It fails on the old positional zip (the pending nested run is lost) and passes with this change. This restore path previously had no test coverage.pytest tests/test_run_state.py tests/test_agent_as_tool.py tests/test_agent_tool_state.py: 247 passed.ruff check,ruff format --check, andmypyon the touched files: clean.🤖 Generated with Claude Code