Repository navigation
feat(stop): use agent.cancel() to stop the event loop (Python) - #4833
strandly-the-agent wants to merge 14 commits into
Conversation
|
@strandly-the-agent review this pull request |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Verdict: changes requested (posted as a comment — GitHub won't let strandly-the-agent request changes on its own PR). The implementation matches design 0019 and the suite is green, but deleting the request_state["stop_event_loop"] check is a silent breaking change: strands_tools.stop (PyPI strands-agents-tools) and any user tool built on that long-standing flag still report success, and the loop just keeps calling the model. Reproduced main vs head (details below). Needs a deprecation shim (inline suggestion) or a coordinated strands-tools release + breaking changelog entry.
One decision for Unshure before merge: a sub-agent that ends via the stop tool now surfaces to the parent as an error tool result ("Agent 'worker' cancelled") and its summary is lost — on main the parent got the content. The PR body defers this to "before the tool leaves experimental"; I think it needs deciding now since it regresses the shipped tool.
Public Agent.cancel() signature changed; the design went through #3506 but the PR carries neither api/needs-review nor api/review-complete.
✅ Verified (head 6873701)
pytest tests(xdist, 4 workers): 7363 passed, 31 skipped.ruff format --check/ruff check: clean.mypy ./src ./tests_typing: 21 errors, none in files this PR touches (missingmypy_boto3_*stubs in my env + the pre-existing ones).- Repro 1 — legacy flag: tool sets
tool_context.invocation_state["request_state"]["stop_event_loop"] = True, mocked model scripted[tool_use(stop), text, text].- main:
stop_reason=tool_use, model calls = 1. - head:
stop_reason=end_turn, model calls = 2, final message is the model's next turn. No warning, no error.
- main:
- Repro 2 —
stopinside an agent-as-tool sub-agent (child.as_tool()), child scripted[text + stop("Answer is 42")].- main: parent
toolResult.status=success, content = child text. - head: parent
toolResult.status=error, content ="Agent 'worker' cancelled".
- main: parent
- Read through: deferred flag only consulted at
event_loop.py:1002(after the batch), never bySequentialToolExecutor/_executor.py(they check_observe_cancellation()), so siblings do finish under both executors; state reset atagent.py:1499-1501sits in the samefinallyas the_cancel_signal.clear(); history after each cancel exit alternates roles correctly ([assistant toolUse][user toolResult][assistant text], or[user][assistant text]for the streaming path). - Scripts + output uploaded to the artifact bucket (
stop_flag_compat.py,subagent_stop.py,repro-output.txt,pytest.log).
Questions
Blocking
- Legacy flag: deprecation shim for one release, or hard break with a coordinated
strands-agents/toolsupdate? Either is fine; silent is not. - Sub-agent +
stop→ error tool result (repro 2). Accept the regression, special-case graceful stops in_agent_as_tool.py:252, or hold the stop-tool migration until the "override stop reason" follow-up lands?
Non-blocking
3. cancel(after_current_tools=True) when no tool batch is in flight (from an AfterModelCallEvent hook on end_turn, or before invoke) is a silent no-op for that invocation — the design lists this as a known con; worth one sentence in the docstring?
Reading order
event_loop.py:989-1012— the removed flag check and the new deferred exit.agent.py:627-684—cancel();agent.py:1499-1501— reset.event_loop.py:626-629— message override on the streaming-cancel path.experimental/tools/stop/stop.py, thentests/strands/agent/test_agent_cancellation.py:633+.
Appendix — non-blocking (2)
- ⚪
event_loop.py:628-629mutates theModelStopReasonmessage dict in place after that event was already yielded to stream consumers. Harmless today; a fresh dict would be cleaner. - ⚪ Thread-safety:
cancel()from another thread racing thefinallyreset atagent.py:1499can leak_deferred_cancelinto the next invocation — same class of race_cancel_signalalready has; not new.
Process note: the three independent review passes I spawned (correctness / API / adversarial) all timed out, so this review is a single pass by me plus the two repros above. Read it with that in mind.
|
@strandly-the-agent Review this PR |
Implements team/designs/0019-deferred-cancel.md for the Python SDK: Agent.cancel(message, after_current_tools=True) defers cancellation until the current tool batch completes, and the stop tool becomes a one-line call to it. The stop_event_loop request_state flag is removed.
- Agent.cancel(): preserve first non-None _cancel_message across repeat calls so idempotent re-invocations don't clobber an earlier message. - event_loop: move the cancelled-message overwrite below the AfterModelCallEvent hook so hooks still observe the real model output; preserve reasoningContent/toolUse/cachePoint blocks already emitted and only replace partial text with the cancel message. - event_loop: cancel precedence over structured_output.stop_loop in the tool-cycle terminal branch, forwarding structured_output_result through the cancelled stop event so it still reaches consumers. - _agent_as_tool: distinguish external cancel (parent's signal set) from the sub-agent terminating itself via the stop tool. External cancel still surfaces as an error tool result; a self-stop falls through to the success path so the terminal message reaches the parent, matching pre-cancel behavior. - BidiAgent.cancel(): accept (message, *, after_current_tools) for signature parity with Agent.cancel() so the stop tool no longer raises TypeError on BidiAgent; store/clear _cancel_message alongside the signal. - docs: update vended-tools.mdx to describe the new Python semantics (agent.cancel + stop_reason "cancelled" with the stop text as the last assistant message).
Mirror the change on BidiAgent.cancel() for signature parity and update all call sites and docstring examples. Keyword-only makes the terminal message argument self-documenting at call sites and leaves room to add future positional-free parameters without ambiguity.
The stop tool and Agent.cancel() signature made 'message' keyword-only; update the test call and Mock assertions to match so they stop raising TypeError / AssertionError.
…nt_loop - LocalAgent.cancel() now mirrors Agent.cancel's keyword-only message and after_current_tools so callers typed against the protocol stay in sync. - Move end_event_loop_cycle_span into each terminal branch so the span's final message matches what EventLoopStopEvent yields (end_turn, cancel). - Re-honor request_state['stop_event_loop'] with a DeprecationWarning so pre-0019 callers of the old stop contract keep working while they migrate to agent.cancel(after_current_tools=True).
…op readme - Build the cancel-overlay message as a new dict and swap it into last_event['stop'] rather than mutating the AfterModelCallEvent-visible message in place. Lazy-serializing hooks that retained a reference now see the real model output; the outer event_loop_cycle re-reads from the swapped tuple so downstream behavior is unchanged. - Update strands-ts stop/README.md: Python now halts via agent.cancel() with stop_reason='cancelled' and the stop text as the last assistant message, matching TS surface except for the stopReason label.
- Trim comments in _agent_as_tool, event_loop (post-hooks cancel-overlay, structured-output precedence) per reviewer feedback. - Update @tool decorator example in Agent.cancel docstring to @tool(context=True) now that the handler takes a ToolContext. - Rewrite agent.cancel(message, ...) → agent.cancel(message=..., ...) in vended-tools.mdx and strands-ts stop README to reflect keyword-only. - Drop the stray section-header banner in test_agent_cancellation.
Previously the error path was gated on the parent's cancel_signal, which missed sub.cancel(after_current_tools=False) called from inside the sub-agent (hook, timeout thread, non-stop tool) — the parent received a success tool result for an aborted sub-task. Switch to the sub-agent's own cancel_signal, captured during stream iteration before stream_async's finally clears it. A set signal covers both external propagation and internal immediate cancels; the stop tool's deferred cancel never sets it, so its terminal message continues to reach the parent as success.
…ename tool cancel local - event_loop._handle_model_execution: the stream aggregator always returns a single-text-block message on cancel, so the text-only filter in the cancel overlay was dead; simplify to a straight content replacement. - bidi.agent.cancel: docstring claimed 'preserves first non-None', the implementation is last-write-wins (matches Agent.cancel); align the Args block to the actual behavior. - event_loop._handle_tool_execution: rename local 'cancel_message' (per-tool error text written into failed tool_results) to 'tool_cancel_reason' so it no longer shadows 'agent._cancel_message' (the user-authored terminal assistant message).
ce4f2b0 to
68d7c2c
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
- event_loop._handle_model_execution: rebuild the cancel-overlay message with explicit role kwarg so mypy no longer flags a missing required TypedDict key through ``**message`` unpacking. - Add a test for the deprecated request_state['stop_event_loop'] branch covering the DeprecationWarning, the fall-through stop_reason, and the preserved request_state. - Add a BidiAgent.cancel test asserting message is stored with last-write-wins semantics and a no-arg call does not clobber it.
… tools, propagate signal discrimination to graph - _ToolCaller: snapshot and restore the agent's cancel state across the direct-tool-call path. The stop tool invoked via agent.tool.stop(...) no longer leaves _deferred_cancel / _cancel_message / cancel_signal set, which would otherwise stop the next invocation after one model call. - event_loop_cycle: observe agent._deferred_cancel after a plain-text model turn (no tool_use). cancel(after_current_tools=True) set from a hook was previously dropped on text-only responses. - multiagent.graph: discriminate between immediate cancel (signal set → FAILED) and the stop tool's deferred cancel (signal unset → COMPLETED) for AgentBase nodes, matching the agent-as-tool rule. - _agent_as_tool: split the "sub_cancel_signal_set" comment — terse rationale at the capture site, discrimination explanation at the conditional. - Drop test_tool_stream_never_requests_stop and test_tool_stream_error_does_not_set_stop; they only asserted the absence of the removed stop_event_loop flag. - Add tests for the three above behaviors.
- Revert the event_loop deferred-cancel-on-plain-text-turn fix; the cycle finishes normally with its natural stop_reason instead. The Agent.cancel docstring now documents this: a deferred cancel that the invocation never gets to apply is a no-op. - Revert the rename event_loop._handle_tool_execution: cancel_message stays as cancel_message. - Move the direct-tool-call cleanup from _ToolCaller to the start of Agent.stream_async. Clear only _deferred_cancel so a stale flag from agent.tool.stop(...) does not stop the next invocation; leave the cancel_signal and cancel_message intact so a caller's pre-invocation agent.cancel(message=...) still applies. - Agent.cancel docstring: drop the inline cooperative-stop example in favor of a link to strands.experimental.tools.stop (now the vended tool at strands.tools.stop). - Graduate the stop tool out of experimental: move strands/experimental/tools/stop/ to strands/tools/stop/ and export from strands.tools. Importing stop or make_stop from strands.experimental.tools now emits a DeprecationWarning. - BidiAgent.cancel docstring: reference the follow-up issue (strands-agents#5162) tracking the terminal-message surfacing in bidi output history.
| the current batch run to completion, and the loop exits before the next model call. This is | ||
| the cooperative stop a tool uses to end the loop from inside the agent. | ||
| the cooperative stop a tool uses to end the loop from inside the agent; see | ||
| :mod:`strands.experimental.tools.stop` for the vended tool. |
There was a problem hiding this comment.
Should this link to the vended tools package instead of the experimental package?
| surface it in output history (tracked by | ||
| https://github.com/strands-agents/harness-sdk/issues/5162). |
There was a problem hiding this comment.
"(tracked by #5162)" will get stale, so we can remove it.
| }.get(stop_reason, Status.COMPLETED) | ||
| # Immediate cancel (signal set) is an abort → FAILED; the stop tool's deferred | ||
| # cancel never sets the signal and resolves as a normal completion. | ||
| if stop_reason == "cancelled": |
There was a problem hiding this comment.
Let's also add this logic to the vended subagent tool, since it still treats graceful stops as errors.
| # Clear a deferred-cancel flag that bypassed stream_async's finally (e.g. | ||
| # agent.tool.stop(...) called directly). _cancel_signal and _cancel_message | ||
| # are left intact so pre-invocation agent.cancel(message=...) still applies. | ||
| self._deferred_cancel = False |
There was a problem hiding this comment.
I think _cancel_message still leaks into the next invocation. The comment above is about a pre-invocation immediate cancellation, but does not handle a deferred cancel which sets both _cancel_message and _deferred_cancel but not the cancel signal.
| self._deferred_cancel = False | |
| self._deferred_cancel = False | |
| if not self._cancel_signal.is_set(): | |
| self._cancel_message = None |
| ```python | ||
| from strands import Agent | ||
| from strands.experimental.tools import stop | ||
| from strands.tools import stop |
There was a problem hiding this comment.
Is the stop tool supposed to graduate into tools instead of vended_tools? I'm fine with either one, just confirming.
| This is a cooperative stop, not an abort. Any other tools the model requested in the same turn still run to completion; the loop halts after that batch without calling the model again. The final message defaults to a 4096-character cap; pass `max_message_length` / `maxMessageLength` to `make_stop` / `makeStop` when a longer summary is legitimate. | ||
|
|
||
| The two SDKs shim onto different loop-termination primitives, which produces a small difference in the final `AgentResult`. TypeScript halts via `AfterToolsEvent.endTurn` and returns `stopReason: "endTurn"` with the stop text as the last assistant message. Python halts via `invocation_state["request_state"]["stop_event_loop"]` and returns `stop_reason: "tool_use"` with the model's tool-use message as the final message; the stop text lives in history as the tool result, not as a new assistant turn. | ||
| The two SDKs shim onto different loop-termination primitives, which produces a |
There was a problem hiding this comment.
The doc links at the top and the code snippets still use the experimental paths. Let's update them.
- Move the graduated stop tool from strands.tools.stop to strands.vended_tools.stop; strands.tools stays reserved for the tool system itself, vended_tools is where ready-to-use tools live (shell, http_request, subagent, ...). The deprecation shim at strands.experimental.tools now redirects to strands.vended_tools. - Agent.stream_async start-of-invocation cleanup: also clear _cancel_message when the cancel_signal is unset, so a deferred cancel left behind by a direct agent.tool.stop(...) call no longer bleeds its message into a later cancel of the next invocation. Signal-set path still preserves the caller's message. - Vended subagent tool: apply the same immediate-vs-deferred discrimination as _AgentAsTool and the graph primitive. A child that cancelled via its signal (abort) surfaces as an error; a child that stopped itself via the stop tool (deferred cancel, signal unset) surfaces as success with the terminal message. - Replace test_cancelled_child_becomes_error_result with immediate / deferred pair, matching the new semantics. - BidiAgent.cancel docstring: drop the inline issue link (will go stale); the behavior gap stays described. - site/vended-tools.mdx: drop the "experimental" marker from the Stop section, update the Python import to strands.vended_tools, update the sourceLinks path to the new Python location.
Human Overview
I created this design: #3506
This pr is meant to allow tools to cancel the agent, while still allowing the current tool batch to finish.
This also deprecates the other ways to stop the event look (i.e. kwargs through tool use)
Description
Python half of design 0019 – Unified Agent Cancellation (#3506). Tools had no public way to end the loop gracefully: the experimental
stoptool wrote an undocumentedrequest_state["stop_event_loop"]flag intoinvocation_state, which is user-owned.agent.cancel()now covers that case:after_current_tools=Truedefers the cancel: sibling tools in the batch finish, then the loop exits before the next model call withstop_reason="cancelled".message, when given, becomes the final assistant message (appended to history) on every cancel exit — streaming, before tools, after tools. Omitted → today's behaviour (model's last message /"Cancelled by user"). Last write wins.stoptool is nowtool_context.agent.cancel(message, after_current_tools=True); thestop_event_loopcontract is gone. Existingcancel()callers are unaffected.Deferred/message state lives on the agent and is cleared where
_cancel_signalis cleared at the end of each invocation.Related Issues
Design: #3506. TypeScript half: separate PR (linked in the #3506 thread); site docs describing the cross-SDK stop behaviour are updated there, since they only become true once both land.
Documentation PR
In the TS PR (
site/.../vended-tools.mdx).Type of Change
New feature
Testing
ruff format --check/ruff checkclean;mypy ./src ./tests_typingshows only the 3 errors already present onmain;pytest tests: 7363 passed, 31 skipped. New tests cover deferred cancel with sibling tools, deferred cancel without a message, state reset between invocations, last-write-wins, immediatecancel(message)during streaming and before tools, interrupt-resume + deferred cancel, and the stop tool's call. Exercised end to end withMockedModelProvider(finish+save_filein one batch →save_fileruns, result iscancelledwith the summary as the last message).hatch run prepare(ran the equivalent ruff/mypy/pytest directly; hatch isn't available in my sandbox)Additional Details
Behaviour change to flag: agent-as-tool and the stop tool
The design chose
cancelledas the stop reason for a graceful stop. One consequence:_agent_as_tool.py:252maps a sub-agent result withstop_reason == "cancelled"to an error tool result ("Agent 'x' cancelled"). A sub-agent that ends via thestoptool previously returnedtool_useand its content flowed to the parent; now the parent sees an error. The same mapping exists in TS (agent-as-tool.ts:215). I left it untouched because the code can't distinguish a graceful stop from an abort — this is exactly the "override the stop reason" kwargliramon2raised on the design, which was deferred. Worth a decision before the stop tool leaves experimental.Scope decisions
LocalAgent.cancel(self)(protocol) andBidiAgent.cancel()are unchanged;Agent.cancelwidens compatibly.ToolContext.agentdefaults toAny, so the stop tool type-checks. Widening bidi is the follow-up the design mentions.AfterToolsEvent.end_turnhook path still runs before the cancel check, so a hook'send_turnwins over a deferred cancel in the same cycle (as it did overstop_event_loop).request_state)._append_messagesfor persistence). Please read with that in mind.Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.