-
Notifications
You must be signed in to change notification settings - Fork 1k
fix(agent): resume nested agent-as-tool interrupts across rehydration #3008
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c80f512
bb6a85c
f89a286
225a361
38bf61e
0e31ed6
f9949f2
1fe1aed
4a018dc
c4804ea
54a0090
f4798e0
d545d5c
b734ef7
7d30fee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,12 +9,14 @@ | |
| import copy | ||
| import logging | ||
| import threading | ||
| from typing import TYPE_CHECKING, Any | ||
| from typing import TYPE_CHECKING, Any, cast | ||
|
|
||
| from typing_extensions import override | ||
|
|
||
| from ..agent.state import AgentState | ||
| from ..interrupt import Interrupt | ||
| from ..types._events import AgentAsToolStreamEvent, ToolInterruptEvent, ToolResultEvent | ||
| from ..types._snapshot import Snapshot | ||
| from ..types.content import Messages | ||
| from ..types.interrupt import InterruptResponseContent | ||
| from ..types.tools import AgentTool, ToolGenerator, ToolSpec, ToolUse | ||
|
|
@@ -183,10 +185,31 @@ async def stream(self, tool_use: ToolUse, invocation_state: dict[str, Any], **kw | |
|
|
||
| try: | ||
| # Determine if we are resuming the sub-agent from an interrupt. | ||
| if self._is_sub_agent_interrupted(): | ||
| prompt = self._build_interrupt_responses() | ||
| sub_agent_snapshot = self._get_sub_agent_snapshot(invocation_state, tool_use_id) | ||
| if sub_agent_snapshot is not None: | ||
| # Resume routes both the interrupt response and the interrupted turn as data, | ||
| # carried in the parent's persisted interrupt record. It does not depend on the | ||
| # orchestrator and sub-agent sharing an in-memory Interrupt object, so it works | ||
| # even when both have been independently rebuilt from storage (e.g. a stateless | ||
| # Lambda that recreates every agent each invocation). | ||
| try: | ||
| prompt = self._restore_from_snapshot(sub_agent_snapshot, invocation_state) | ||
| except Exception as restore_error: | ||
| # Log at ERROR so this is alertable. A failed restore silently destroys the | ||
| # human's approval: the broad except below would convert it to an ordinary | ||
| # tool error and the interrupt would be deactivated. Re-raise so callers see | ||
| # a clear failure rather than a quiet success with lost state. | ||
| logger.error( | ||
| "tool_name=<%s>, tool_use_id=<%s> | " | ||
| "failed to restore sub-agent from interrupt snapshot, " | ||
| "the pending interrupt approval may be lost: %s", | ||
| self._tool_name, | ||
| tool_use_id, | ||
| restore_error, | ||
| ) | ||
| raise | ||
| logger.debug( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Non-blocking: a failed restore here silently destroys the human's approval. Any exception from Reproduced at this head, all ending Worth catching restore failures separately from sub-agent execution failures and failing loud (ERROR-level, alertable) — and ideally leaving the interrupt pending so a fixed deploy can retry, rather than converting it into a terminal tool error. The broad
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can easily pull the restore into its own try/except with an ERROR log so it's loud and alertable rather than silently swallowed. Happy to add that here if others agree.. Re-emitting the interrupt so it stays retryable touches the contract between _AgentAsTool and the event loop in a way that feels like its own ticket.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 on the separate try/except. Agree that re-emitting is out of scope |
||
| "tool_name=<%s>, tool_use_id=<%s> | resuming sub-agent from interrupt", | ||
| "tool_name=<%s>, tool_use_id=<%s> | resuming sub-agent from serialized interrupt snapshot", | ||
| self._tool_name, | ||
| tool_use_id, | ||
| ) | ||
|
|
@@ -214,7 +237,12 @@ async def stream(self, tool_use: ToolUse, invocation_state: dict[str, Any], **kw | |
|
|
||
| # Propagate sub-agent interrupts to the parent agent. | ||
| if result.stop_reason == "interrupt" and result.interrupts: | ||
| yield ToolInterruptEvent(tool_use, list(result.interrupts)) | ||
| namespaced_interrupts, interrupt_id_map = self._namespace_interrupts( | ||
| tool_use_id, list(result.interrupts) | ||
| ) | ||
| yield ToolInterruptEvent( | ||
| tool_use, namespaced_interrupts, sub_agent_snapshot=self._build_sub_agent_snapshot(interrupt_id_map) | ||
| ) | ||
| return | ||
|
|
||
| if result.structured_output: | ||
|
|
@@ -268,25 +296,103 @@ def _reset_agent_state(self, tool_use_id: str) -> None: | |
| self._agent.messages = copy.deepcopy(self._initial_messages) | ||
| self._agent.state = AgentState(self._initial_state.get()) | ||
|
|
||
| def _is_sub_agent_interrupted(self) -> bool: | ||
| """Check whether the wrapped agent is in an activated interrupt state.""" | ||
| return self._agent._interrupt_state.activated | ||
| @staticmethod | ||
| def _namespace_interrupts(tool_use_id: str, interrupts: list[Interrupt]) -> tuple[list[Interrupt], dict[str, str]]: | ||
| """Create parent-visible copies of interrupts with IDs namespaced by the outer tool call. | ||
|
|
||
| Prefixing with the outer ``tool_use_id`` guarantees uniqueness at the parent level | ||
| when multiple sub-agents are invoked concurrently. | ||
|
|
||
| Args: | ||
| tool_use_id: The outer (orchestrator-level) tool use ID for this agent-as-tool call. | ||
| interrupts: The sub-agent-local interrupt objects. | ||
|
|
||
| Returns: | ||
| A tuple of (namespaced interrupt copies, mapping from parent_id to local_id). | ||
| """ | ||
| namespaced: list[Interrupt] = [] | ||
| id_map: dict[str, str] = {} | ||
| for interrupt in interrupts: | ||
| parent_id = f"{tool_use_id}:{interrupt.id}" | ||
| id_map[parent_id] = interrupt.id | ||
| namespaced.append(Interrupt(id=parent_id, name=interrupt.name, reason=interrupt.reason)) | ||
| return namespaced, id_map | ||
|
|
||
| def _build_sub_agent_snapshot(self, interrupt_id_map: dict[str, str]) -> dict[str, Any]: | ||
| """Capture the sub-agent's session state for resuming this interrupted invocation. | ||
|
|
||
| def _build_interrupt_responses(self) -> list[InterruptResponseContent]: | ||
| """Build interrupt response payloads from the sub-agent's interrupt state. | ||
| Uses ``take_snapshot(preset="session")`` to capture all session fields (messages, state, | ||
| conversation_manager_state, interrupt_state, model_state) as a versioned snapshot. | ||
|
|
||
| The parent agent's ``_interrupt_state.resume()`` sets ``.response`` on the shared | ||
| ``Interrupt`` objects (registered by the executor), so we re-package them in the | ||
| format expected by ``Agent.stream_async``. | ||
| Args: | ||
| interrupt_id_map: Mapping from parent-visible (namespaced) interrupt IDs to | ||
| sub-agent-local IDs. | ||
|
|
||
| Returns: | ||
| List of interrupt response content blocks for resuming the sub-agent. | ||
| Serializable snapshot of the interrupted invocation. | ||
| """ | ||
| return [ | ||
| {"interruptResponse": {"interruptId": interrupt.id, "response": interrupt.response}} | ||
| for interrupt in self._agent._interrupt_state.interrupts.values() | ||
| if interrupt.response is not None | ||
| ] | ||
| session_snapshot = self._agent.take_snapshot(preset="session") | ||
| return { | ||
| "session_snapshot": session_snapshot.to_dict(), | ||
| "interrupt_id_map": interrupt_id_map, | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Persist the complete resumable session state. This custom snapshot ends at With the real OpenAI Responses request formatter, Would using the existing versioned session snapshot/load path be simpler here? Otherwise capture and defensively restore these fields too, with regressions for both cases. |
||
|
|
||
| def _get_sub_agent_snapshot(self, invocation_state: dict[str, Any], tool_use_id: str) -> dict[str, Any] | None: | ||
| """Return the sub-agent snapshot for this invocation, if the parent is resuming it. | ||
|
|
||
| Args: | ||
| invocation_state: Tool invocation context, populated by the event loop with | ||
| ``_sub_agent_interrupt_resume`` when resuming from an interrupt. | ||
| tool_use_id: The toolUseId of this agent-as-tool call, used to key the snapshot. | ||
|
|
||
| Returns: | ||
| The snapshot for this invocation, or ``None`` if this is not a sub-agent resume. | ||
| """ | ||
| sub_agent_interrupt_resume = invocation_state.get("_sub_agent_interrupt_resume") | ||
| if not sub_agent_interrupt_resume: | ||
| return None | ||
| snapshots = sub_agent_interrupt_resume.get("snapshots") or {} | ||
| return cast("dict[str, Any] | None", snapshots.get(tool_use_id)) | ||
|
|
||
| def _restore_from_snapshot( | ||
| self, snapshot: dict[str, Any], invocation_state: dict[str, Any] | ||
| ) -> list[InterruptResponseContent]: | ||
| """Restore the sub-agent from a snapshot and build the resume prompt. | ||
|
|
||
| Loads the full session state via ``load_snapshot``, then filters and translates | ||
| the parent's interrupt responses back to sub-agent-local IDs. | ||
|
|
||
| Args: | ||
| snapshot: Snapshot produced by ``_build_sub_agent_snapshot`` (possibly round-tripped | ||
| through session serialization). | ||
| invocation_state: Tool invocation context carrying the parent's resume responses | ||
| under ``_sub_agent_interrupt_resume``. | ||
|
|
||
| Returns: | ||
| The interrupt responses destined for this sub-agent, ready to pass to ``stream_async``. | ||
| """ | ||
| session_snapshot_dict = snapshot["session_snapshot"] | ||
| self._agent.load_snapshot(Snapshot.from_dict(session_snapshot_dict)) | ||
|
|
||
| interrupt_id_map: dict[str, str] = snapshot.get("interrupt_id_map") or {} | ||
|
|
||
| sub_agent_interrupt_resume = invocation_state.get("_sub_agent_interrupt_resume") or {} | ||
| responses = sub_agent_interrupt_resume.get("responses") or [] | ||
|
|
||
| local_responses: list[InterruptResponseContent] = [] | ||
| for response in responses: | ||
| parent_id = response["interruptResponse"]["interruptId"] | ||
| local_id = interrupt_id_map.get(parent_id) | ||
| if local_id is not None: | ||
| local_responses.append( | ||
| { | ||
| "interruptResponse": { | ||
| "interruptId": local_id, | ||
| "response": response["interruptResponse"]["response"], | ||
| } | ||
| } | ||
| ) | ||
| return local_responses | ||
|
|
||
| @override | ||
| def get_display_properties(self) -> dict[str, str]: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -380,9 +380,30 @@ def message(self) -> str: | |
| class ToolInterruptEvent(TypedEvent): | ||
| """Event emitted when a tool is interrupted.""" | ||
|
|
||
| def __init__(self, tool_use: ToolUse, interrupts: list[Interrupt]) -> None: | ||
| """Set interrupt in the event payload.""" | ||
| super().__init__({"tool_interrupt_event": {"tool_use": tool_use, "interrupts": interrupts}}) | ||
| def __init__( | ||
| self, | ||
| tool_use: ToolUse, | ||
| interrupts: list[Interrupt], | ||
| sub_agent_snapshot: dict[str, Any] | None = None, | ||
| ) -> None: | ||
| """Set interrupt in the event payload. | ||
|
|
||
| Args: | ||
| tool_use: The tool use that was interrupted. | ||
| interrupts: The interrupts raised during tool execution. | ||
| sub_agent_snapshot: Serializable snapshot for resuming an interrupted sub-agent | ||
| invocation. ``None`` for ordinary tool interrupts. Stored as private metadata, | ||
| not included in the dict payload. | ||
| """ | ||
| super().__init__( | ||
| { | ||
| "tool_interrupt_event": { | ||
| "tool_use": tool_use, | ||
| "interrupts": interrupts, | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Keep this internal snapshot out of the emitted event dictionary. Please carry this as private event metadata consumed by the event loop. If public exposure is intentional, it needs an API review plus explicit redaction, compatibility, and versioning contracts before merge. |
||
| } | ||
| ) | ||
| self._sub_agent_snapshot = sub_agent_snapshot | ||
|
|
||
| @property | ||
| def is_interrupt(self) -> bool: | ||
|
|
@@ -404,6 +425,11 @@ def interrupts(self) -> list[Interrupt]: | |
| """The interrupt instances.""" | ||
| return cast(list[Interrupt], self["tool_interrupt_event"]["interrupts"]) | ||
|
|
||
| @property | ||
| def sub_agent_snapshot(self) -> dict[str, Any] | None: | ||
| """Serializable state for resuming an interrupted sub-agent, or None.""" | ||
| return self._sub_agent_snapshot | ||
|
|
||
|
|
||
| class ModelMessageEvent(TypedEvent): | ||
| """Event emitted when the model invocation has completed. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove snapshots. The session management and rehydration of agents are responsibility of session managers, so for a subagent to be able to rehydrate, it should have it's own session manager