fix(agent): resume nested agent-as-tool interrupts across rehydration - #3675
fix(agent): resume nested agent-as-tool interrupts across rehydration#3675strandly-the-agent wants to merge 9 commits into
Conversation
Resuming an interrupt raised inside a nested agent-as-tool only worked while the orchestrator and the sub-agent shared the same in-memory Interrupt object. The resume path read the human's answer off the sub-agent's copy, which the executor registered into the orchestrator by reference, so the two were the same object. Once either agent is rebuilt from storage - a stateless handler recreating every agent per request - they are two independent objects and the answer written to the orchestrator's copy is invisible to the sub-agent's. Resume now works as data. Sub-agent interrupts are propagated as copies whose ids are namespaced by the outer toolUseId, so ids stay unique when several sub-agents are invoked in one turn, and the sub-agent-local id is recoverable by stripping the prefix. On resume the orchestrator's persisted interrupt responses are mapped back to local ids and handed to the sub-agent. An ephemeral sub-agent (preserve_context=False) cannot have a session manager, so its interrupted turn is stored as one keyed entry in the orchestrator's interrupt context and consumed on resume. A sub-agent used with preserve_context=True owns its state and keeps its interrupted turn in its own session; if it has none, the resume reports an actionable error instead of silently dropping the answer. Parking an interrupt now preserves interrupt-context keys the event loop does not own, matching how Graph and Swarm already treat their own interrupt context. Co-authored-by: seanalbert <seanalbert@users.noreply.github.com>
An interrupted turn stored on the orchestrator was removed before it was deserialized, so a load failure - a schema version the running SDK does not accept, for instance - destroyed the only copy of the turn the human had already answered. The stored turn is now dropped only once it has loaded, and the failed tool result distinguishes a turn that could not be loaded from a sub-agent that never had one.
|
Review loop — round 1. An independent fresh-context reviewer found two real bugs; both are fixed in
After the fixes: The disputed nit, and what round 1 verified for itselfWhy the broad Verified independently by round 1 (re-run by the reviewer rather than taken from me): |
Tool use ids and interrupt ids are both model-derived strings, so one call's raw prefix could be a literal prefix of another call's namespaced ids - a tool use id of "<other-id>:v1" matches every interrupt the other call raises, because hook interrupt ids start with "v1:". The answer was then routed to the wrong sub-agent with a mangled id, and the other sub-agent's genuine interrupt was lost: it failed with "no interrupt found" and the turn ended as though the human had resolved it. Percent-encoding the tool use id keeps the separator out of the prefix, so prefix matching is exact while a tool use id that itself contains the separator still round-trips. Also documents on _reset_agent_state that sharing one Agent instance as a tool across several orchestrators lets one call clear a turn another has parked.
|
Review loop — round 2 (adversarial). Five focused probes against the propagation/resume mechanism. One real routing bug, fixed in 🔴 Answers could be routed to the wrong sub-agent call. Namespacing used a raw ✅ Held: no double execution (approve → re-interrupt → approve again runs each tool call exactly once, in-process and across a real restart); no wrong recipient in the ordinary case (answer A only → A runs once, B never runs and stays pending, then answering B runs it once); rehydration with a stateful sub-agent ( The foot-gun this surfaced, and why it is documented rather than fixedSharing one Fixing that properly means making the reset aware of a foreign parked turn, which is a behaviour change beyond this bug and would need its own review. I documented it on After both fixes: |
…defensively The round-trip test asserted only that a separator-bearing tool use id maps its own answers back, which the unescaped scheme also satisfied, so it did not guard the escaping; it now pins the encoded id. Removing a consumed turn uses pop so a concurrent removal cannot raise, and the restore failure path records that a partial load leaves the sub-agent mismatched until its next fresh call.
|
Review loop — round 3: APPROVE. A third fresh reviewer verified the two fix commits and cleared them; its two non-blocking notes and one test-quality catch are applied in
At head |
|
@strandly-the-agent do an independent review, use subagents with fresh context to get them to review our changes. are we good? |
mkmeral
left a comment
There was a problem hiding this comment.
I think we can clean up the code a bit more, especially internal helpers, but not a blocker for bugfix
|
Are we good? Not quite — one 🔴, five 🟡. Six fresh-context passes on
Blocking question for you, I can't formally approve/request-changes my own PR, so this is a comment. Two findings are pre-existing (not this PR's) and I'd file them separately rather than block here — say the word. 🔴 1 — the retained continuation is destroyed by
|
turn 2 stop_reason |
interrupts |
tool ran | |
|---|---|---|---|
merge base a10881c7 |
'interrupt' (re-prompts — bad, but self-announcing, operation still pending) |
new id | no |
head b097fc36 |
'end_turn' |
[] |
no |
The PR body says config D "returns an actionable tool error rather than dropping the approval". The error result is real, but the approval itself is gone and not re-submittable — which is the part that gets dropped. The deferral of the deeper fix (your 2026-08-05 call: "that's a new feature") stands; what's missing is that the failure mode changed, and the residuals list doesn't say so.
Cheapest honest improvement — warn at park time, before the human is asked for an approval that can never be applied, instead of at resume time after it's lost (_stash_continuation, _agent_as_tool.py:445):
if self._preserve_context:
if getattr(self._agent, "_session_manager", None) is None:
logger.warning(
"tool_name=<%s>, tool_use_id=<%s> | sub-agent uses preserve_context=True with no session "
"manager: its interrupted turn is held only in memory and will not survive a restart",
self._tool_name, tool_use_id,
)
returnFires in config C too (where in-process resume works fine), hence warning not error. ~5 lines, no behaviour change, respects the deferral.
For the record, the API pass measured the deferred feature rather than speculating: moving the gate from not self._preserve_context to "the sub-agent has no session manager" is 2 lines, leaves cells A/B/C byte-identical and makes cell D resume across a real rebuild. It also rolls back a sub-agent that accumulated history between park and resume — which is exactly the multiagent-session-semantics question you said deserves its own design. Input for that follow-up, not a request here.
🟡 3 — toolUseId == "v1" still collides with the SDK's own interrupt sentinel
Round 3's percent-encoding closed the raw-separator hole, but every SDK interrupt id starts with the literal v1: (hooks/events.py:169,244,438, types/tools.py:162, _middleware/stages.py:134) and quote("v1", safe="") is a no-op — I checked: quote("v1", safe="") + ":" → 'v1:'. So _namespace_prefix("v1") prefix-matches every interrupt the orchestrator raised itself, at _agent_as_tool.py:368 and :391.
Reproduced: orchestrator confirms its own dangerous_action while a sub-agent also interrupts, human approves both → the orchestrator's own answer is stripped and forwarded down to the sub-agent, which raises KeyError (no interrupt found), the tool errors, and the sub-agent's approved dangerous_action("prod-db") never runs while the turn reports end_turn. Control with tooluse_worker_123: both actions execute.
Reachability: narrow — 🟡, not a blocker. No Bedrock/OpenAI/Anthropic id format and no direct tool call (tools/_caller.py:109) ever emits "v1"; it needs a custom/local provider, a hook rewriting tool_use, or a prompt-steered model. But it's the same threat model round 3 accepted when it added the encoding, with the one remaining reserved key — and the fix is one line, in a namespace the SDK controls rather than something derived from model text:
return f"v1:agent_as_tool:{quote(tool_use_id, safe='')}:"That also fixes the cosmetic complaint that nested ids no longer start with v1: while every sibling id site does. Cost: 8 tests hard-code the f"{tool_use_id}:{local}" shape — they'd be format-agnostic if they built expectations via _namespace_prefix. Worth deciding before merge either way, since these ids are client-persisted.
🟡 4–6 — docs, approval-UI legibility, test gaps
4 · The one documented preserve_context=True example is the cell that stays broken. agents-as-tools.mdx:141-147 is the only place customers see preserve_context=True, and the snippet has no session manager — so the documented happy path is config D, which per finding 2 now fails more silently than before. I'd previously called docs a follow-up; that was the wrong call, and the reason isn't AGENTS.md:339 in the abstract, it's that the existing docs now point at a newly-quieter failure. Three cheap spots: the preserve_context docstring (agent.py:977-981, duplicated at _agent_as_tool.py:72-76), the mdx caution inline, and interrupts.mdx (which documents Graph/Swarm nesting in detail and never mentions agent-as-tool).
5 · Two sub-agents' interrupts are indistinguishable to a human or LLM approval UI. _namespace_interrupts (:338-342) correctly leaves name/reason alone — but that means the only legible fields carry no agent identity, and AgentResult.interrupts threads bare Interrupts with no tool_use attached. Two sub-agents sharing one hook class (a normal reuse pattern) produce byte-identical name/reason, separable only by parsing an opaque compound id that with a real model-generated tool use id (tooluse_z7iPwa8Xr1Ww1i0RHGjNRV) says nothing. One-line fix: name=f"{self._tool_name}: {interrupt.name}" (makes _namespace_interrupts an instance method).
6 · Test gaps, both mutation-verified. (a) Config B — preserve_context=True + the sub-agent's own FileSessionManager — is the literal text of #3076's repro and has no end-to-end test; only config A does. It works (verified independently across a real two-process restart), it's just not pinned by CI. (b) Reverting only the event_loop.py:884 _park_interrupt_context call site passes the whole suite: test_park_interrupt_context_keeps_keys_the_event_loop_does_not_own proves the helper, not that call site, and the pre-existing after-tools test only asserts loop-owned keys. Seed a foreign key before the hook raises and assert it survives.
Ledger, what held, appendix, and what I suppressed
Ran: head b097fc36 vs merge base a10881c71. Six fresh-context passes — correctness, adversarial (advanced, real multi-process restarts), test-quality (mutation-tested), API/DevX (advanced), issue-alignment (two-process repro), LLMINFO (live Bedrock anthropic.claude-haiku-4-5) — after a triage route (ACCEPT) and a context-build pass. Triage wanted to skip four of these on the grounds that the PR's own tests cover them; that's circular for an agent-authored PR, so I overrode it and fanned out wide. Every file:line I cite above I re-read myself at this SHA.
✅ 1239 tests re-run green on the touched suites (the body's 4840-pass full-suite figure independently corroborated in shape). ✅ All three negative controls re-run and confirmed: regression test fails on main (assert [] == ['prod-db']), reverting the escaping fails both namespacing tests, reverting the stored-turn fix fails its test. ✅ Round-3's rewrite of the round-trip test does now earn its keep.
Held under attack (worth knowing what's not broken): 3-level nesting across a real restart; two concurrent sub-agents with byte-identical local interrupt ids, partial answers, exactly-once execution each; two interrupts inside one sub-agent call; a Graph node whose agent has an interrupting agent-as-tool; continuation GC across turns (deactivate() prevents accumulation); percent-encoding injectivity for every plausible id; no double execution on approve→re-interrupt→approve across restarts; the preserve_context=True + own-session-manager escape hatch; _park_interrupt_context is necessary, not a drive-by (reverting it returns config A to a silent drop). Scope matches the body digit-for-digit; types/_events.py is untouched, as claimed; the Co-authored-by: seanalbert trailer is on 73077edb2.
Alignment: fixes #3076 as filed. The issue's literal step 2 says preserve_context=True, and three converging pieces of evidence — the issue's own bug-verify repro (which gave the sub-agent a FileSessionManager), "rebuild both from the session store", and your #3008 comment — make config B the intended reading. Config B and the reporter's actual stated use case (config A, ephemeral, no session manager) both resume. Config D is broken only under an assumption the reporter never stated, and is disclosed.
⚪ appendix, non-blocking: _InterruptState.to_dict() returns context by reference, so a stashed continuation aliases the live dict (interrupt.py:120-126; unreachable through any path the adversarial pass could build — one deepcopy would close it for free) · an ephemeral sub-agent's parked tool input now lands in the orchestrator's session store where it previously landed nowhere, stranded if the human never answers (not caller-visible, cleaned on resume — probably a docstring line) · a sub-agent answer is single-use where an orchestrator's own answer is durable (recoverable, contrived trigger) · Interrupt.id has no documented opacity statement while every sibling handle in the SDK has one (snapshot_session_manager.py:108, checkpoint.py:47, storage.py:77) · neither new logger.error logs the sub-agent's own .name · _park_interrupt_context's "foreign keys survive" contract is unenforced folklore — a Final set of loop-owned keys would make it greppable.
Pre-existing, not this PR's — I'd file these separately rather than block: a transient hook failure on the resume cycle consumes an approval identically (verified byte-identical on the merge base, so it's the interrupt system's general error semantics) · Graph/Swarm route interrupts by a stored id list and would collapse two children raising the same v1:before_tools:{uuid5(name)} id into one entry (graph.py:739-744) — a latent bug this PR's namespacing happens to avoid for agent-as-tool.
Suppressed on purpose (so you can check my precision): the broad except Exception in _restore_continuation — declined round 1, no pass brought new evidence · issue links on the three new tests — declined round 3, and the test-quality pass independently confirmed AGENTS.md/TESTING.md supports the decline · the shared-Agent-instance residual — already disclosed, no pass found it worse than admitted. The other residual ("partial load_snapshot… self-heals") is not suppressed: that claim is false, and it's folded into finding 1.
Non-blocking questions: does a contract-only change (no public signature, but Interrupt.id shape and preserve_context resume semantics both moved) want an api/needs-review label under API_BAR_RAISING.md's "customer-facing contracts" clause? And is Interrupt.id opacity a deliberate non-commitment or just never written down?
|
@strandly-the-agent fix and iterate, run review subagents to review your fixes and final pr. make sure everything is good |
…ot be reinstated A parked sub-agent turn that failed to load was deliberately left in place so a later attempt could still apply the human's response, but the call then yielded a failed tool result. With no interrupt left in the round the event loop ends the turn and clears the agent's whole interrupt record, taking the parked turn, the pending interrupt and the response with it - and a resubmission of the same interrupt id then raises. Raise the interrupt again instead, so the turn stays parked behind it and the response can be applied once the turn loads. Two further defences on the same path: - Namespaced interrupt ids now open with an SDK-owned marker. Every interrupt id the SDK generates opens with the v1: scheme marker and percent-encoding leaves "v1" untouched, so a tool use id of exactly "v1" produced a prefix matching every interrupt the orchestrator raised itself, handing its own responses down to a sub-agent. - A context-preserving sub-agent with no session manager is warned about when its interrupt parks, rather than only when a response arrives that can no longer be applied. The failed-resume tool result now also tells the model the guarded action did not run and that it must not report success, which is what a model reads before telling the user. The orchestrator's side of a call moves into a _ParentCall object that binds the orchestrator to the call's id namespace, replacing seven static helpers that each re-derived both from invocation_state.
Parking a turn refreshes the loop's own keys and carries everyone else's, which was three string literals in a comprehension. Name them once so the rule is greppable, and cover the after-tools rescue call site: reverting only that site left the whole suite green, because the existing test asserts the loop's own keys and never a foreign one.
…eserve_context mode preserve_context decides who keeps a sub-agent's interrupted turn, and so whether a resume survives a restart, but it was documented purely as conversation history - and the one place preserve_context=True is shown to customers has no session manager, which is the configuration that cannot resume after a restart. Interrupts documented multi-agent nesting for Swarm and Graph only, so agents-as-tools gets its own section covering id namespacing and where the turn lives. Interrupt.id is now stated to be opaque, matching every sibling handle in the SDK.
| def _namespace_prefix(tool_use_id: str) -> str: | ||
| """Build the prefix that namespaces a sub-agent interrupt id to one agent-as-tool call. | ||
|
|
||
| Two sub-agents invoked in the same turn can raise the same interrupt id, because each derives it | ||
| from its own tool use id. The prefix keeps them distinct in the orchestrator's interrupt record and | ||
| keeps the sub-agent-local id recoverable by stripping it back off. | ||
|
|
||
| Tool use ids and interrupt ids are both model-derived, so the prefix defends against both ways one | ||
| call's prefix can match another id: the tool use id is percent-encoded, because otherwise one | ||
| containing the separator matches another call's ids, and the prefix opens with a reserved marker, | ||
| because otherwise a tool use id of ``v1`` matches every interrupt the orchestrator raised itself. | ||
|
|
||
| Args: | ||
| tool_use_id: Tool use ID of the agent-as-tool call. | ||
|
|
||
| Returns: | ||
| Prefix, separator included, for interrupt IDs belonging to that call. | ||
| """ | ||
| return f"{_NAMESPACE_TAG}{quote(tool_use_id, safe='')}:" | ||
|
|
||
|
|
||
| def _namespace_interrupts(tool_use_id: str, interrupts: list[Interrupt]) -> list[Interrupt]: | ||
| """Copy sub-agent interrupts with their ids namespaced to one agent-as-tool call. | ||
|
|
||
| Only the id changes: ``name`` and ``reason`` are what a human or an approval UI reads, so they are | ||
| passed through untouched. | ||
|
|
||
| Args: | ||
| tool_use_id: Tool use ID of the agent-as-tool call. | ||
| interrupts: Interrupts raised inside the sub-agent. | ||
|
|
||
| Returns: | ||
| Orchestrator-visible copies carrying namespaced ids. | ||
| """ | ||
| prefix = _namespace_prefix(tool_use_id) | ||
| return [ | ||
| Interrupt(id=f"{prefix}{interrupt.id}", name=interrupt.name, reason=interrupt.reason) | ||
| for interrupt in interrupts | ||
| ] |
There was a problem hiding this comment.
Do these really need to be their own functions?
There was a problem hiding this comment.
No — both are gone in a61c9f3b1. _ParentCall already derived the prefix, so namespacing is now a method on it (parent_call.namespace(interrupts)), and the parent lookup is inlined at its one call site per Unshure's related note. Three places that each re-derived the prefix collapsed into one owner, and the round is a net deletion rather than more structure.
| return self._tool_use_id | ||
|
|
||
| @property | ||
| def awaiting_resume(self) -> bool: |
There was a problem hiding this comment.
There's a lot of new terminology introduced: awaiting_ resume, parked and interrupted. Should we clarify these terms and consolidate where needed so we're consistent through the class and codebase?
There was a problem hiding this comment.
Agreed — and the constant was the worst of it: _PARKED_TURNS_KEY = "sub_agent_continuations" used two different words for one thing while the docstrings called it a third ("interrupted turn").
Settled on one: a sub-agent's interrupted turn is stored and restored. parked, reinstate and continuation are gone from the source, the tests and the persisted key (now sub_agent_interrupted_turns), and awaiting_resume reads is_resuming. a61c9f3b1
| f"Agent '{self._tool_name}' did NOT run and the human's response was NOT applied: its " | ||
| "interrupted turn did not survive the restart. A sub-agent used with preserve_context=True " | ||
| "keeps its own state, so it needs its own session manager for that state to survive. Do not " | ||
| "report the requested action as completed or successful; tell the user it failed." |
There was a problem hiding this comment.
Does the orchestrator know if preserve_context=True was used by the sub agent?
There was a problem hiding this comment.
No, and it doesn't need to — the adapter knows (self._preserve_context), and the adapter is what writes this message.
But you caught a real bug by asking: the message asserted preserve_context=True as the cause unconditionally, so an ephemeral sub-agent whose turn went missing was told to attach a session manager it isn't even allowed to have (construction rejects that combination). It now only names preserve_context when that is actually the cause. a61c9f3b1
| finally: | ||
| self._lock.release() | ||
|
|
||
| def _reset_agent_state(self, tool_use_id: str) -> None: |
There was a problem hiding this comment.
Should this also reset _interrupt_state?
There was a problem hiding this comment.
Yes — good catch, fixed in a61c9f3b1 with a test.
It's safe because _reset_agent_state only runs on a fresh call, never on a resume, so any interrupt state still on the agent belongs to a turn nobody is resuming. Leaving it made the next call fail rather than the old one: the sub-agent believes it is mid-interrupt and rejects a plain prompt. Test is test_stream_resets_stale_interrupt_state_on_a_fresh_call, which fails if the reset line is removed.
…l awaits The orchestrator keeps every interrupt id it has been handed until its own turn ends, so a sub-agent that interrupts twice leaves its first id recorded there after that step has already run. Re-raising everything with the call's prefix therefore offered the caller an id the reinstated sub-agent no longer holds; answering it made resume raise, the call failed, and the round ended with no interrupt left - so the parked turn and the answer were cleared, which is the failure this path exists to prevent. Offer only what the parked turn is still waiting on. Also: park a copy of the turn rather than the snapshot's own dicts, which alias the sub-agent's live interrupt context, and correct the claim that a sub-agent is never invoked after a partly applied load - it is, if it is still activated from an earlier turn in this process.
|
Fixed and iterated — and the review of my fixes found a blocker in the fix itself, which is now fixed too. Head is
✅ Verified at Two judgement calls I'd rather you make than me (both in the body's residuals):
I did not file the two pre-existing bugs as issues yet — say the word and I will. The blocker in my own fix, in detailThe orchestrator keeps every interrupt id it has been handed until its own turn ends — nothing GCs a consumed one. So for a sub-agent with two guarded steps ("read the row, then delete it"), by the second interrupt the orchestrator holds My And after (same script, unmodified): The fix reads the parked snapshot's own A side effect worth noting: this makes the What the fix review ruled out, and what's still openHeld under attack (adversarial pass, real multi-process restarts): re-parking is idempotent and terminates — three re-park cycles replayed no tool, grew no messages, made no model calls and charged no tokens, with the unrelated tool in the same batch still running exactly once; two sub-agent calls where one is resumable and one corrupt — the answered one ran once, the corrupt one stayed pending; no silent auto-approval from the stale Test-quality pass: all four claimed negative controls reproduced independently, the merge-base failure for config B confirmed, and the vacuous test I'd flagged confirmed genuinely fixed. It found one gap I'd missed — the Still not automated (listed in the body): two sub-agent calls interrupting concurrently in one turn, three-level nesting, and a resume prompt mixing sub-agent and orchestrator answers. All three pass by hand; the concurrency one is the most worth adding, since namespacing exists precisely for it. Deliberately not done: no migration for interrupt ids parked by an earlier revision of this branch under the old prefix — that format never shipped. The failure shape there is a silent replay of the sub-agent's turn rather than a loud reject; if you'd rather it were loud pre-merge, that's a one-line warning. |
| def _namespace_prefix(tool_use_id: str) -> str: | ||
| """Build the prefix that namespaces a sub-agent interrupt id to one agent-as-tool call. | ||
|
|
||
| Two sub-agents invoked in the same turn can raise the same interrupt id, because each derives it | ||
| from its own tool use id. The prefix keeps them distinct in the orchestrator's interrupt record and | ||
| keeps the sub-agent-local id recoverable by stripping it back off. | ||
|
|
||
| Tool use ids and interrupt ids are both model-derived, so the prefix defends against both ways one | ||
| call's prefix can match another id: the tool use id is percent-encoded, because otherwise one | ||
| containing the separator matches another call's ids, and the prefix opens with a reserved marker, | ||
| because otherwise a tool use id of ``v1`` matches every interrupt the orchestrator raised itself. | ||
|
|
||
| Args: | ||
| tool_use_id: Tool use ID of the agent-as-tool call. | ||
|
|
||
| Returns: | ||
| Prefix, separator included, for interrupt IDs belonging to that call. | ||
| """ | ||
| return f"{_NAMESPACE_TAG}{quote(tool_use_id, safe='')}:" | ||
|
|
||
|
|
||
| def _namespace_interrupts(tool_use_id: str, interrupts: list[Interrupt]) -> list[Interrupt]: | ||
| """Copy sub-agent interrupts with their ids namespaced to one agent-as-tool call. | ||
|
|
||
| Only the id changes: ``name`` and ``reason`` are what a human or an approval UI reads, so they are | ||
| passed through untouched. | ||
|
|
||
| Args: | ||
| tool_use_id: Tool use ID of the agent-as-tool call. | ||
| interrupts: Interrupts raised inside the sub-agent. | ||
|
|
||
| Returns: | ||
| Orchestrator-visible copies carrying namespaced ids. | ||
| """ | ||
| prefix = _namespace_prefix(tool_use_id) | ||
| return [ | ||
| Interrupt(id=f"{prefix}{interrupt.id}", name=interrupt.name, reason=interrupt.reason) | ||
| for interrupt in interrupts | ||
| ] |
| prompt = str(tool_input) | ||
|
|
||
| tool_use_id = tool_use["toolUseId"] | ||
| parent_call = _ParentCall.resolve(invocation_state, tool_use_id) |
There was a problem hiding this comment.
Just general question on code readability: _ParentCall.resolve returns an object if invocation_state.get("agent") is not None, else it returns none. If that logic lived inline here, I think it would be easier for a human to read and follow the code since there are less jumps/redirects. This code is valid and fine, but curious if we should try to index on "human readability" of code in these reviews?
There was a problem hiding this comment.
Agreed, and inlined in a61c9f3b1:
# A tool invoked directly rather than by an agent has no orchestrator, and cannot interrupt.
parent = invocation_state.get("agent")
parent_call = _ParentCall(parent, tool_use_id) if parent is not None else Noneresolve is gone. And yes — I think human readability is the right axis to index on here, especially for a private adapter where the extra indirection buys nothing. Your note plus opieter-aws's two made me take a second pass at the whole shape: the same reasoning removed the two module-level helpers and restructured the resume block, so the commit is a net −6 lines.
| An agent exposed as a tool with [Agents as Tools](./multi-agent/agents-as-tools.md) propagates its interrupts up to the orchestrator, so the orchestrator's caller sees them and answers them like any other interrupt. Two things are worth knowing: | ||
|
|
||
| - **Interrupt ids are namespaced by the tool call.** A sub-agent's interrupt arrives with an id derived from the orchestrator's tool call, so two sub-agents interrupting in the same turn stay distinct. Treat the id as opaque: pass it back unchanged to resume, and use `name` and `reason` for anything you show a human. | ||
| - **Where the interrupted turn is kept depends on `preserve_context`.** With the default `preserve_context=False` the orchestrator carries the sub-agent's interrupted turn, so the resume survives the orchestrator and sub-agent being rebuilt from storage. With `preserve_context=True` the sub-agent owns its own state and needs its own session manager for the resume to survive a restart. |
There was a problem hiding this comment.
This comment makes sense in context to this pr, but reads as a bit out of place in this doc page. If im reading this, I dont really care where the interrupt is kept, I just want to know that I can interrupt my agent as a tool. Im worried that including this might just confuse people
There was a problem hiding this comment.
Cut. The page now keeps only the part a caller has to act on — the id is opaque, pass it back unchanged, use name/reason for anything shown to a human — and the storage detail is gone from here entirely. It moved under an advanced heading on the agents-as-tools page, per your other comment. a61c9f3b1
| </Tab> | ||
| </Tabs> | ||
|
|
||
| Interrupts raised inside a sub-agent resume automatically, but where the interrupted turn is kept depends on this setting. With the default <Syntax py="preserve_context=False" ts="preserveContext: false" />, the orchestrator carries the sub-agent's interrupted turn, so a resume works even when every agent is rebuilt from storage on the next request. With <Syntax py="preserve_context=True" ts="preserveContext: true" /> the sub-agent owns its own state: give it its own session manager if the resume has to survive a restart, otherwise the interrupt can only be resumed by the same process that raised it. |
There was a problem hiding this comment.
Maybe put this under an advanced section? This feels like an implementation detail, and I dont think needs to be up front to customers.
There was a problem hiding this comment.
Moved to a #### Advanced: Interrupts and Session State subsection after Context Management, with the interrupts page linked for the general picture. a61c9f3b1
| carried = { | ||
| key: value for key, value in agent._interrupt_state.context.items() if key not in _LOOP_OWNED_CONTEXT_KEYS | ||
| } | ||
| agent._interrupt_state.context = {**carried, "tool_use_message": message, "tool_results": tool_results} |
There was a problem hiding this comment.
The **carried is really the actual bug fix in this pull request right? We are propagating context from a tool that was previously not serialized and lost, right?
Do we need to worry about carried being serializable?
There was a problem hiding this comment.
Two answers, because they come apart.
Is **carried the fix? Necessary but not sufficient. Reverting just that line does return the ephemeral case to a silent no-resume (0 executions, verified), because the stored turn is discarded the moment the turn parks. But the human's response also has to travel: after a restart the orchestrator's and the sub-agent's Interrupt objects are different objects with the same id, so the ids are namespaced per tool call and mapped back on resume. **carried keeps the turn; the namespacing routes the response. Both are load-bearing.
Serializability: no new requirement. carried only ever holds what other components already put in _interrupt_state.context, and that whole dict was already persisted wholesale by _InterruptState.to_dict() into the session record — parking was discarding those keys, not failing to serialize them. The one value this PR adds is a Snapshot.to_dict(), which is JSON by construction (it is exactly what SnapshotSessionManager persists). So nothing here can become unserializable that wasn't already.
| logger.error( | ||
| "tool_name=<%s>, agent_name=<%s>, tool_use_id=<%s> | cannot apply the interrupt " | ||
| "response yet, raising the interrupt again so it survives to be answered once " | ||
| "more", | ||
| self._tool_name, | ||
| self._agent_name, | ||
| tool_use_id, | ||
| ) |
There was a problem hiding this comment.
Does this error happen when a agent-as-tool has multiple interrupts, and only some of them have been responded to? If so, could we mention that in the error message?
| logger.error( | |
| "tool_name=<%s>, agent_name=<%s>, tool_use_id=<%s> | cannot apply the interrupt " | |
| "response yet, raising the interrupt again so it survives to be answered once " | |
| "more", | |
| self._tool_name, | |
| self._agent_name, | |
| tool_use_id, | |
| ) | |
| logger.error( | |
| "tool_name=<%s>, agent_name=<%s>, tool_use_id=<%s> | cannot apply the interrupt " | |
| "response yet, there are still unanswered interrupts (maybe print interrupt ids here as well), raising the interrupt again so it survives to be answered once " | |
| "more", | |
| self._tool_name, | |
| self._agent_name, | |
| tool_use_id, | |
| ) |
There was a problem hiding this comment.
Not quite, and the real trigger is narrower — worth correcting rather than documenting.
Partially-answered interrupts don't reach here: an unanswered one just means the sub-agent is re-invoked without a response for it and re-raises it, no error. This fires when the stored turn itself could not be restored (e.g. a snapshot the running build won't load).
I took the interrupt-ids part of your suggestion, which is useful either way — a61c9f3b1:
tool_name=<%s>, agent_name=<%s>, tool_use_id=<%s>, interrupt_ids=<%s> | the interrupted turn
could not be restored, so the response cannot be applied yet: raising the interrupt again so
it survives to be answered once more
| "tool_name=<%s>, agent_name=<%s>, tool_use_id=<%s> | cannot resume: the sub-agent's " | ||
| "interrupted turn is not available, so the interrupt response cannot be applied", |
There was a problem hiding this comment.
Seems like this happens if an orchestrator with a session manager uses a sub-agent that has preserve_context=True, but no session manager. If the sub-agent interrupts, and then the application stops and restores through session, then the application breaks here.
Is there some way we can prevent this early with errors? Or maybe pass session manager to the child agent? I guess returning a toolResultEvent does help avoid this, but it feels like its just masking the underlying issue that would keep coming up.
There was a problem hiding this comment.
"Prevent this early" — now we do, as early as it is knowable. When the interrupted turn is stored, if a preserve_context=True sub-agent has no session manager, it warns before the human is ever asked:
interrupted sub-agent uses preserve_context=True with no session manager, so its interrupted
turn is held only in memory: the interrupt resumes in this process but not after a restart
At construction there is nothing to validate — we can't know whether an interrupt will happen, or whether the orchestrator persists anything.
"Pass the session manager to the child" is the real fix and I measured it rather than guessing: moving the gate from preserve_context to "the sub-agent has no session manager" is a ~2-line change that makes this exact case resume across a real restart and leaves the other three configurations byte-identical. The costs are that it makes the orchestrator a state store for an agent that owns its own state, and that load_snapshot would roll back a sub-agent that accumulated history between the interrupt and the resume. mkmeral deferred precisely that as its own feature ("that's a new feature… I'd rather focus on the bug ahead"), which is why it isn't here.
On masking: the tool result isn't the remedy, it's damage control — without it the response is dropped and the model narrates success over an action that never ran (I reproduced that against a live model earlier in this PR). Happy to land the inheritance version here instead if you and mkmeral would rather not wait for the follow-up.
| if self._preserve_context: | ||
| if getattr(self._agent, "_session_manager", None) is None: | ||
| logger.warning( | ||
| "tool_name=<%s>, agent_name=<%s>, tool_use_id=<%s> | interrupted sub-agent uses " |
There was a problem hiding this comment.
Follow up from related comment, do we just want to throw in this case, or maybe inherit the parent agents session manager?
There was a problem hiding this comment.
Same underlying question as your comment above, so one answer for both.
Throwing here would be worse than the error result: it surfaces as a generic tool failure mid-turn, the caller can't act on it, and the human's response is lost either way — with the error result at least the model is told the action did not run and not to report success.
Inheriting the parent's session manager is the fix I'd actually want, and it measures well (~2 lines; this case resumes, other configurations unchanged). It is the piece mkmeral deferred as a separate feature, so I've kept it out — say the word and I'll bring it into this PR.
|
|
||
| try: | ||
| self._agent.load_snapshot(Snapshot.from_dict(turn)) | ||
| except Exception as error: |
There was a problem hiding this comment.
Can we choose a more specific error here?
There was a problem hiding this comment.
Done in a61c9f3b1:
except (SnapshotException, ValueError, KeyError, TypeError) as error:Those are the ways a turn written by a different build of the SDK actually fails to load: an unsupported schema version or scope (SnapshotException), a state a component rejects (ValueError — e.g. a conversation-manager class mismatch), and a field that has since been renamed or retyped (KeyError/TypeError). Anything outside that set is a bug rather than stale data, and now propagates instead of being swallowed.
For context, I'd pushed back on narrowing this twice in earlier rounds, on the grounds that an unanticipated type escaping would cost a human's approval. Naming the set is the better answer: it keeps the stale-data cases handled deliberately and stops the clause hiding real bugs.
| if parent_call is not None and parent_call.awaiting_resume: | ||
| if not self._reinstate_turn(parent_call) and not self._agent._interrupt_state.activated: |
There was a problem hiding this comment.
Im having a bit of trouble following the logic here:
In the documentation, you mention that interrupts are stored in different locations if preserve_context is set to true or false right? Where in this code block are we checking the interrupt context if preserve_context=False or the agents sessions if preserve_context=True?
Is there an opportunity to restructure the logic here to make it a bit easier to follow what is going in the different code paths?
There was a problem hiding this comment.
Fair — that was the least obvious part of the change. Restructured in a61c9f3b1 so both paths are named where the decision happens:
if parent_call is not None and parent_call.is_resuming:
# The interrupted turn comes from one of two places: an ephemeral sub-agent's turn is
# stored on the orchestrator, so restoring it reads the orchestrator's interrupt record,
# while a preserve_context=True sub-agent restores its own from its session manager and
# so arrives already activated.
restored = self._restore_interrupted_turn(parent_call) or self._agent._interrupt_state.activated
if not restored:
...So, to answer directly: _restore_interrupted_turn is the preserve_context=False path — it reads the turn the orchestrator stored in its own interrupt context. The preserve_context=True path is the activated check: nothing is stored for that sub-agent, its own session manager restored its interrupt state, so it comes back already activated and there is nothing for the orchestrator to do. If neither produced a turn, the response can't be applied and we fall into the branch below.
|
@strandly-the-agent update the code based on comments. I want minimal changes. |
…errupts Review feedback on the shape of the code rather than its behaviour: - Fold the two module-level namespacing helpers into _ParentCall, which already derives the prefix, and inline the parent lookup at its single call site. One object owns the id namespace instead of three functions re-deriving it. - Settle on one word for one thing. A sub-agent's interrupted turn is "stored" and "restored", never also "parked", "reinstated" or a "continuation" - including the context key, which said continuations while its constant said parked. awaiting_resume reads is_resuming. - pending_interrupts() reads the stored turn itself rather than taking the awaited ids as an argument, so the resume path no longer assembles them at the call site. - Name where an interrupted turn comes from in each configuration, which was the part of the resume block hardest to follow: an ephemeral sub-agent's turn is restored from the orchestrator, a context-preserving one arrives already activated from its own session. - Catch the exceptions a turn written by another build actually raises rather than bare Exception, and let a genuine bug propagate. - Log the interrupt ids when raising an interrupt again, and only blame preserve_context in the failed-resume message when that is really the cause. Behaviour change: resetting an ephemeral sub-agent for a fresh call now also clears its interrupt state. It is reset to its construction baseline, and interrupt state nobody is resuming belongs to that baseline as much as its messages do; left in place, the sub-agent refuses the fresh prompt because it believes it is mid-interrupt. Docs: the concepts page keeps only what a caller acts on - ids are opaque, pass them back - and the preserve_context detail moves under an advanced heading on the agents-as-tools page.
|
All 13 threads answered inline; head is
✅ At One thing I did not do, because it's a behaviour change rather than a cleanup: inheriting the parent's session manager for a |
Description
Resuming an interrupt raised inside a nested agent-as-tool only worked while the orchestrator and the sub-agent shared the same in-memory
Interrupt. The resume path read the human's answer off the sub-agent's copy, which the tool executor had registered into the orchestrator by reference, so the two were literally the same object. Once either agent is rebuilt from storage — a stateless handler recreating every agent per request — they are two independent objects, and the answer written to the orchestrator's copy is invisible to the sub-agent's: the sub-agent re-raises the same interrupt forever and the approved tool never runs.This implements the simplification agreed in #3008's thread, and supersedes the approach in that PR (no
ToolInterruptEventchange, noinvocation_staterouting):v1:agent_as_tool:) and percent-encodes the tool use id, because both ids are model-derived: without the encoding a tool use id containing the separator matches another call's ids, and without the marker a tool use id of exactlyv1matches every interrupt the orchestrator raised itself. On resume, the orchestrator's already-persisted_interrupt_state.context["responses"]are filtered by that prefix and mapped back to local ids.preserve_context=Falsesub-agents cannot have a session manager (_agent_as_tool.pyrejects that combination), so their turn is stored as one keyed entry in the orchestrator's own interrupt context and freed once it has been reinstated. Apreserve_context=Truesub-agent owns its state and keeps its turn in its own session; if it has none, that is warned about when the interrupt parks — before a human is asked for a response that could not be applied — and the resume reports an actionable error rather than silently dropping the answer.The adapter grew more than the behaviour change alone needs: seven static helpers that each re-derived the orchestrator and the id prefix from
invocation_statewere consolidated into one_ParentCallobject, per the review note about internal helpers.Related Issues
Fixes #3076. Implements the design
mkmeralsettled on in #3008 and supersedes the snapshot-plumbing approach there. Co-authored withseanalbert, whose PR framed the problem and the stateless-Lambda use case (Co-authored-by:trailer is on the first commit).Documentation PR
Included here rather than deferred.
preserve_contextdecides who keeps a sub-agent's interrupted turn, and therefore whether a resume survives a restart, but it was documented purely as conversation history — andagents-as-tools.mdxis the only place customers seepreserve_context=True, in a snippet with no session manager, which is the one configuration that cannot resume after a restart.interrupts.mdxdocumented multi-agent nesting for Swarm and Graph only, so agents-as-tools now has its own section covering id namespacing and where the turn lives.Interrupt.idis now documented as opaque, matching every sibling handle in the SDK.Type of Change
Bug fix
Testing
Behaviour: the four agent-as-tool configurations, each across a process boundary
preserve_context=False, no session managerpreserve_context=True+ its ownFileSessionManagerpreserve_context=True, no session managerpreserve_context=True, no session managerA and B are now both pinned by end-to-end tests with a real
FileSessionManagerand every agent rebuilt between turns; B is the configuration issue #3076 describes literally, and it fails at the merge base (verified:turn2 stop_reason=interrupt executions=[]ona10881c71,end_turn/['prod-db']at this head).Gates — run directly;
hatchis not available in my environment, and this sandbox kills any single shell command at ~60s, so the suite was run in four chunks rather than one invocation:ruff format --check src testsacross the whole tree reports13 files would be reformatted— byte-identical at the merge base, so pre-existing and untouched by this PR.What the new tests pin down, and that they fail without the fix. Every one of these was verified by reverting the specific hunk and watching the test fail:
test_nested_interrupt_resumes_after_rehydration(#3076 regression)main:assert [] == ['prod-db']test_nested_interrupt_resumes_after_rehydration_with_a_sub_agent_session_managertest_nested_interrupt_survives_a_parked_turn_that_fails_to_loadtest_nested_interrupt_that_reraises_twice_runs_each_confirmed_action_oncetest_stream_resume_reraises_the_interrupt_when_the_parked_turn_cannot_be_loadedtest_stream_resume_reraises_only_the_interrupts_the_parked_turn_still_awaitstest_namespaced_interrupt_ids_are_not_captured_by_a_tool_use_id_of_the_scheme_marker_NAMESPACE_TAG = ""test_namespaced_interrupt_ids_*(2)test_stream_interrupt_warns_when_a_context_preserving_sub_agent_has_no_session_managertest_stream_interrupt_parks_a_turn_that_is_isolated_from_the_sub_agenttest_event_loop_cycle_interrupts_preserved_when_after_tools_hook_raisesTwo tests were fixed rather than added, because they passed while the behaviour they named was untrue:
test_stream_resume_keeps_stored_turn_when_it_cannot_be_loadedasserted an in-memory dict on a mock orchestrator and could not see that the event loop then cleared it (replaced by the re-raise test above), andtest_stream_interrupt_resume_skips_state_resetconstructed the tool after setting the sub-agent's messages, so its reset baseline equalled the expected result and a reset would have been invisible. Two tests for a removed one-line private accessor were deleted; the branch that used it is covered by the resume tests.Also exercised by hand on this branch: partial answers, three-level nesting, two sub-agents whose local interrupt ids collide, a sub-agent with a
SummarizingConversationManagerand one with a stateful model (conversation_manager_stateandmodel_stateboth intact after a real restart, against a genuinely fresh manager/model as the control), aGraphnode whose agent has an interrupting agent-as-tool, and a check that nothing new reaches the caller.Checklist
Review loop ledger
b3dc2cd0); 2 nits applied, 1 declined40e08f22)b097fc36)65d3730…30f0355)21fd240); rest belowRound 4 found what rounds 1–3 missed, and round 5 found a defect in round 4's own fix — a sub-agent that interrupts twice had its first, already-consumed interrupt id re-raised, which re-entered this very bug. Both are now fixed with tests that fail without them. Rounds 1–3 were dispatched by the same identity that wrote the code; rounds 4–5 were not given that assumption.
Declined, with reasons
except Exceptionin the reinstate path. It guards deserializing persisted data possibly written by a different SDK version, where the failure modes are open-ended; narrowing it would let an unanticipated type escape into the adapter's generic handler, which reports a plain tool error atWARNING— exactly the silent loss of a human's approval this PR exists to prevent. It logs at ERROR and returns a specific message, and Graph/Swarm call_InterruptState.from_dictwith no guard at all.AGENTS.md, only a regression test for a filed bug links its issue; the two regression tests for [BUG] Nested agent-as-tool interrupts don't resume across rehydration (stateless / distributed execution) #3076 do link it.name. It would make two sub-agents' interrupts easier to tell apart in an approval UI, butnameis caller-visible and matched on, so prefixing it risks breaking name-based routing to fix a legibility problem. The tool call is already identifiable from the id, and a structured field is the better answer — worth its own change.Residual / known limitations
ERRORlog. This is deliberate: the alternative is to give up after N attempts and discard an approval a human already gave, and a recoverable annoyance beats destroying the answer. An operator who fixes the underlying cause gets a working resume. Worth a maintainer's opinion.Agentinstance as a tool across several orchestrators is a foot-gun:_reset_agent_stateis unconditional, so one orchestrator's call can clear a turn another has parked. The parked orchestrator still resumes correctly; the other call fails with a confusing error. Documented on that method rather than fixed, because making the reset aware of a foreign parked turn is a behaviour change deserving its own review.load_snapshotthat fails after validation leaves the sub-agent with some fields applied and others not. The parked turn is kept, the interrupt stays pending, and an ephemeral sub-agent is reset on its next fresh call, so it self-heals. It is not invoked in that state unless it is itself still activated from an earlier turn in this process; noted in the code.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.