[campoy]

What was changed

TemporalAgent now refuses two tools with the same name, as Strands does, instead of letting the
later MCP server's tool silently replace the earlier one.

• One BeforeModelCallEvent hook per agent (it was one per MCP server) lists each server that is
  due, in order, as before, and checks the listings so far after each one: a name that is taken,
  or that differs from a taken one only by - or _, raises MCPToolNameConflictError, a
  ValueError and a non-retryable ApplicationError. Names are checked against each other and
  against the agent's other tools as they are after each listing, so a tool that a Signal handler
  adds while a listing runs counts, as ToolRegistry.register_tool() does (dynamic tools such as
  structured output's by exact name).
  A refused turn lists no further servers and leaves the registry as it was. Otherwise the turn's
  tools are applied together, so a name can move from one server to another in one turn.
• The error gives Strands' message, the servers involved and a fix: set a prefix or use
  tool_filters, or pass only one handle when two handles of one server list the same tools.
• MCPToolNameConflictError is an ApplicationError with non_retryable=True, so an uncaught
  refusal fails the run instead of retrying its Workflow Task (replay would raise it again), and a
  Workflow retry policy does not start another run. This holds with StrandsPlugin on the Worker
  only, whose failure converter does not apply, and on the client.
• After a tool call, Strands runs the next model call inside its event loop and wraps most errors
  from it in EventLoopException. TemporalAgent re-raises a wrapped MCPToolNameConflictError
  as itself, so a refusal on a later turn can be caught and fails the Workflow like one on the first
  turn. Like Strands' stream_async, it closes Strands' event loop in a finally and awaits it, so
  closing the agent's stream early still waits for AfterInvocationEvent hooks.
• TemporalMCPClient.load_tools() returns no tools, so every agent gets its MCP tools from the
  hook, whether it was built before or after its clients listed, and has none until its first
  model call. Before, an agent built after a listing got them at construction, where a clash
  raised Strands' plain ValueError, a hot-reloadable @tool could replace a listed tool or the
  other way round, and a listed tool took a sandbox tool's name.
• A cache_tools=True server is listed once, as before. Later turns check its stored listing
  again, so a refusal lasts as long as the duplicate, and an agent that shares an already-listed
  client gets its tools without listing again.
• The agent tracks its MCP tools by name. While a server lists a name, the name is the server's: a
  tool put under it (for example a wrapper) is replaced at the next turn, and removed, also from
  dynamic_tools, when the server stops listing it.
• A client passed twice to one agent is listed twice per turn, as before, and is not a duplicate. A
  server that lists one name twice is. So is an MCP tool named like a TemporalSandbox tool.
• README: the duplicate rule, when the error is raised, the failure type, the messages the error
  leaves in agent.messages, how to recover a run that met it, a structured_output_model named
  like an MCP tool (Strands' own ValueError, as in Strands), and which earlier runs can fail to
  replay. The TemporalMCPClient docstring says that two handles for one server go on different
  agents.

Breaking change: an agent whose tools share a name (two MCP servers that list the same tool
without distinct prefixes, two handles of one server, a server that lists a name twice, or an MCP
tool named like another tool on the agent, including a sandbox tool) now raises
MCPToolNameConflictError at the model call after the listing, and fails the Workflow if the error
is not caught. Before, the later tool replaced the earlier one. Give one server a prefix or drop
the tool with tool_filters. The title starts with Breaking: because release notes come from
commit subjects.

The change has no version gate, by decision: for now these plugin fixes add no gates and accept that
some runs from earlier versions do not replay. A run that met one of these duplicates before this
version fails replay with [TMPRL1100]. Runs with names that differ only by - or _ replay when
the earlier version raised Strands' error at the same listing, and fail otherwise, for example with
cache_tools=True, where it went on after one refusal. Runs whose servers changed their tools during
the run can fail too, though no names clashed: a tool that moves from one server to another in one
turn now stays (the earlier version could lose it), and an agent that shares an uncached client with
another agent now drops the tools the server stopped listing (the earlier version kept them). And
an agent built after another agent listed a client they share has no MCP tools until its first
model call, where the earlier version gave it the last listing at construction: a run whose
Workflow code read that agent's tool_names or tool_registry before then can schedule other
commands and fail too, though no names clashed and the servers kept their tools. Last, a turn's
listings are applied together, once every server due that turn has listed, where the earlier
version added each server's tools as soon as it listed. Until then the agent's tools stay as they
were before that turn, and they stay so if a listing fails. A run whose Workflow code read the
agent's tools before the turn's listings were applied can schedule other commands and fail too,
though no names clashed and the servers kept their tools: for example a Signal or Update handler
that ran while a later server listed, even if every listing then succeeded, or code that caught a
failed listing. Other runs whose tool names never clashed, whose servers kept their tools, and
whose Workflow code read an agent's tools only when none of its listings was pending or unapplied
(not during a turn's discovery, not after a failed listing, and not on an agent built after a
shared client listed before its first model call) schedule the same commands (replay checks
below).

Upgrade: let the runs above finish on the earlier version before you upgrade, or keep them
there with Worker Versioning: runs whose agents met a duplicate tool name, runs whose servers
changed their tools, runs whose Workflow code reads the tools of an agent built after a shared
client listed, before that agent's first model call, and runs whose Workflow code can read an
agent's tools while its servers list (Signal and Update handlers, other coroutines) or after
catching a failed listing.

The branch has the two earlier commits, two merges of main, d3f653a (one hook per agent, after
the first review), 487e5aa (after the second review), f2a5bf1 (after the third review), and
2819ae6 and 9c219ce after the fourth (the event loop closed and awaited when the stream closes,
the replay tests pinned to where they fail, the registry read after each listing, the
non-retryable error with the plugin on the Worker only, and the upgrade notes on tool visibility
during and after discovery).
Review the combined diff against main.

Why?

See #183. _reconcile_mcp_tools called registry.replace() for every listed name already in the
registry. Since #148, distinct prefixes keep two servers apart, but without them a call meant for
one server still runs on the other, with no error.

Checklist

1. Closes [Bug] strands_agents: two MCP servers that list the same tool name silently replace each other's tool #183

2. How was this tested:

   • 27 new test cases in tests/test_mcp.py (31 in the file). One runs Workflows with two servers
     that list echo, with the plugin on the Worker only: the one that catches the error gets the
     refusal, its Activities are checked and its history replays; the one that does not fails with
     a non-retryable MCPToolNameConflictError. Another runs a tool call, then a listing that
     clashes, caught and uncaught, with the plugin on the Worker only and on the client, and
     maximum_attempts=2; the uncaught run fails with a non-retryable MCPToolNameConflictError
     and is not retried. Four replay histories recorded on main at
     63d7aec: distinct names must replay; duplicate names, a shared client whose server dropped a
     tool, and a tool that moved between servers in one turn must fail with a nondeterminism error.
     The others run the agent's hooks through agent.hooks with stand-in listings, for agents built
     before and after their clients listed, check registry entries by identity against each
     server's latest listing, count the listings, and add or remove a tool while a listing runs.
     The replay tests for old histories match the Activity pair where the replay fails.
     On 487e5aa, the head before the third review, 7 of the cases then in the file fail: the
     refusal after a tool call, agents built after a listing, and a listed name held by another
     tool, its -/_ twin or a sandbox tool. On f2a5bf1, 3 fail: the two Worker-only
     non-retryable checks and the tool added during a listing.
   • A new test in tests/test_hooks.py closes the agent's stream after its first event, and in a
     control reads it to the end; an async AfterInvocationEvent hook runs an Activity. In both,
     the hook finishes and its Activity completes before the Workflow returns, and the history
     replays. It passes on main; without the finally that closes Strands' event loop, the early
     close returns before the hook finishes and Python logs async generator ignored GeneratorExit.
   • The recorded-history loader now lives in tests/common.py, for the two older legacy tests
     too.
   • Locally, ruff check --select I, ruff format --check, pyright, mypy, basedpyright,
     pydocstyle and scripts/ci/check_conventions.py pass, and the full suite passes (198 tests).
     These ran with Python 3.13 and uv 0.13.1, outside the plugin's required-version, so not
     through make. CI covers make lint and make test.
   • [Bug] strands_agents: two MCP servers that list the same tool name silently replace each other's tool #183's reproduction exits 1 on main (the call runs on server b) and 0 with the change
     (refused, naming both servers; run on d3f653a).
   • Replay checks on f2a5bf1, without a gate, on Workers against a dev server, with stdio MCP
     servers. 20 runs recorded on main 63d7aec: duplicates with and without cache_tools, a
     server that stops listing a duplicate, -/_ near-duplicates, shared clients with and without
     a local tool of the same name, names moving between servers, and prefixed servers. The 9 whose
     names never clashed, or that met Strands' own -/_ refusal at the same listing, replay on
     the change; the 11 that met a duplicate main let through, or went on past a cached
     near-duplicate, fail with [TMPRL1100]. Every run recorded with the change replays on it; a
     run that does not catch the refusal ends with WorkflowExecutionFailed
     (MCPToolNameConflictError). Of 10 runs started on a main Worker and finished on a new
     Worker with the change, 6 finish: 3 whose names never clashed, one whose duplicate appears
     after the upgrade and is refused then, and 2 whose turn before the upgrade met Strands' own
     -/_ refusal after the same listings. The other 4 fail with [TMPRL1100]. On main, the
     runs of these scenarios recorded with the change whose names never clashed replay; none of
     them reads an agent's tools (see below for runs that do). A server renamed in the
     Workflow fails replay with [TMPRL1100] (control). These scenarios do not hit the
     shared-client drop or the one-turn move; the two recorded histories above cover those. They
     were run on f2a5bf1 and again on this head, with the same results; every run recorded on
     f2a5bf1 replays on this head.
   • Stand-in MCP Activities, on the head and on f2a5bf1: a Signal that adds a local echo
     while the listing runs is now refused, and no remote call runs (on f2a5bf1 the call ran on
     the server). With maximum_attempts=2 and the plugin on the Worker only, an uncaught refusal,
     on the first turn and after a tool call, is non-retryable and fails one run (on f2a5bf1,
     retryable, and a second run started). A run recorded on main at 3488fbe whose Workflow
     code schedules an Activity when a second agent sharing a listed client has echo in
     tool_names replays on main and fails with [TMPRL1100] on this PR, cached and uncached.
     A run recorded on main whose agent lists server pa (echo), then fails to list server
     pb, and whose Workflow code catches the ActivityError and schedules an Activity when echo
     is in tool_names, kept echo and scheduled it on main; with this PR the agent has no
     tools and the Activity is not scheduled, so the main history fails with [TMPRL1100]. Runs
     where the first listing fails, or where both listings succeed, replay both ways (controls).
     A run recorded on main whose agent lists server pa (echo) and then server pw, both
     successfully, and whose Signal handler, run while pw lists, schedules an Activity when
     echo is in tool_names, saw ['echo'] and scheduled it on main; with this PR the handler
     sees no tools, so the main history fails with [TMPRL1100]. The same Signal sent after the
     turn's listings sees both tools on both versions, and its runs replay both ways (control).
     Rolling back is affected the same way: the runs of these three kinds recorded with this PR,
     with stable, distinct tool names, fail to replay on main with [TMPRL1100], so a run with
     no clash is not by that alone safe to move back to the earlier version.

3. Any docs updates needed?
   The MCP section of the README now states the duplicate rule, the failure type, how to recover a
   run that met it, and which earlier runs can fail to replay.

Not covered:

• A mixed rollout, with old and new Workers on one Task Queue.
• A TemporalMCPClient that outlives a run in one Worker process (an unsandboxed runner) keeps
  _fetched and _tools, so with cache_tools=True a second run skips the listing that a replay
  in a fresh process schedules. This is the same on main, and the change does not touch it.
• A failed or cancelled model Activity after a tool call is still wrapped in EventLoopException by
  Strands and leaves the run open, as on main; this PR unwraps only its own error.
• The hook still runs after user BeforeModelCallEvent hooks, as before. Running it first would
  move its Activities before theirs, which changes the commands of runs without a clash.
• temporalio 1.34.x still bundles the old code as temporalio.contrib.strands. No backport here.

[DABH]

Review of the duplicate-tool-name refusal. The main concern is the PR's claim that "runs whose tool names never clashed schedule the same commands": three paths below break that claim and were proven by recording a history on main and replaying it on this branch against a dev server. Lint and all 78 tests pass on the head.

Gate. Per the PR body, the first commit had a workflow.patched gate and the replay checks ran only on that gated version. The repo has two earlier command-altering Python changes that are gated with legacy-history tripwire tests (_temporal_model.py context-window-overflow recovery, and the deepagents retire-result-cache change). Old code emitted no marker when it silently replaced a duplicate, so operators cannot tell which in-flight runs are affected and the README remedy must be applied blind. Either restore the gate or record the decision explicitly, rerun the replay checks on the ungated build, and add main-recorded legacy_*.json histories for the clash and non-clash paths.

Altitude. A single BeforeModelCallEvent hook per agent (list every uncached provider in order, validate the turn's whole MCP tool set once against the agent's other tools, apply in place) matched main's list-tools sequence on every non-clash scenario tried and removes about 130 lines of per-provider state. See the comment on line 121.

Note on the private-API tests and one more README item are inline below.

[DABH]

Altitude. Per-provider hooks exist because the old code had them, and this PR then rebuilds a whole-set view by special-casing hook order (_held_by early-lists later providers, _mcp_listed_early tells their hooks to skip, _mcp_unaccepted remembers refusals, two except BaseException blocks repair those sets). Each new interaction in the findings below needed one more set or branch, the ownership predicate is written four times, and the __init__ comment above still says cache_tools=True is "guarded by _fetched". A prototype with one hook per agent matched main turn by turn on the scenarios I tried (including the shared-later-agent case this PR gets wrong), produced the same 14-activity history under old, PR and prototype with clean cross-replays, and passed the PR's 11 end-to-end tests (the 9 hook-level tests would need rewriting). Trade-offs to decide on purpose: every uncached provider is listed before a refusal, and a refused cached provider is not re-listed.

[campoy]

Thanks, done in d3f653a. There's now one BeforeModelCallEvent hook per agent: it lists each server that is due, in order, then checks the turn's whole MCP tool set against itself and the agent's other tools, and applies it. _held_by, the early listing, _mcp_unaccepted, _mcp_listed_early and both except BaseException blocks are gone, and the __init__ comment is true again.

I took both trade-offs: every uncached server is listed before a refusal (this only changes turns that clash), and a refused cached server is not listed again. Each turn checks its stored listing again instead.

[DABH]

_mcp_owned is seeded from provider._tools at construction. If a TemporalMCPClient object survives across workflow runs in one worker process (unsandboxed runner, passthrough_all_modules, or the handle defined in a module passed through the sandbox, which the class docstring recommends), run 2 starts from run 1's stale listing and makes different early-listing decisions than a fresh-process replay, which then fails with [TMPRL1100]. Not reproducible under the default sandbox (the module is re-imported per run), so this one is plausible rather than confirmed. Keeping listing state per run, or documenting that handles must be constructed inside the workflow, would close it.

[campoy]

The hook no longer uses this state to decide what to list. It lists every server that is due, in order, as main does, so a stale _tools can't change the commands with cache_tools=False.

What's left is main's own problem with a handle that outlives a run: _fetched and _tools carry over, so with cache_tools=True run 2 skips a listing that a fresh-process replay schedules. This PR doesn't change that, so I listed it under "Not covered" in the description and would fix it separately.

[DABH]

Same client instance listed twice refuses its own listing. self._mcp_providers.index(provider) returns the first occurrence. With a = TemporalMCPClient('a'); TemporalAgent(tools=[a, a]) (or nested lists that share a client, such as [*BASE, *RESEARCH]), hook #2 computes later = [a], _held_by is true because _mcp_owned[a] already holds echo, so it lists a a third time and seen flags echo: ValueError: Tool name 'echo' already exists on every turn, with a prefix/tool_filters hint that cannot apply. Main listed twice and replace()d with no error, and I verified a main-recorded history ['echo_a-list-tools','echo_a-list-tools','invoke_model'] fails replay on this branch with [TMPRL1100] even though no two tools ever shared a name. Fix: give each hook its position (enumerate in __init__) and use later = [o for o in self._mcp_providers[pos+1:] if o is not provider and not self._cached(o)]. Do not dedupe _mcp_providers, which would change main's command sequence.

[campoy]

Fixed: the hook keeps one listing per client, so tools=[a, a] lists a twice per turn, as main does, and is not a duplicate. test_mcp_same_client_twice_on_one_agent covers it.

[DABH]

A dash/underscore near-duplicate moving between servers changes the command sequence. On main, when server A lists x_y while B's x-y is registered, register_tool raises Strands' normalised-name error after one list-tools. Here _held_by normalises names, so B is early-listed (two commands) and the move succeeds. A main-recorded history in which the workflow caught the error and kept going fails replay on this branch ([TMPRL1100], "Activity machine does not handle this event ... TimerStarted"). Low likelihood, so the actionable part is the PR description and README scope: the README's "such a duplicate" carve-out describes replaced tools, not this case.

[campoy]

Agreed, kept as is. On main that turn raised after one listing; here it lists every due server first. The PR description now says that a main run that caught that error and went on doesn't replay, and the README rule names -/_ near-duplicates.

[DABH]

A refused or failed batch re-lists cache_tools=True providers that main would skip. Marking every already-listed provider _mcp_unaccepted here (and in refuse() below) includes a cached provider whose own listing succeeded. Scenario: agent built after shared client q listed echo, plus p = TemporalMCPClient('p', cache_tools=True); p lists, _held_by early-lists q, that activity fails, the workflow catches and re-invokes. Next turn main runs ['q-list-tools']; this branch runs ['p-list-tools','q-list-tools']. Verified on a dev server: a main-recorded history fails replay with [TMPRL1100], and the main run never raised a ValueError, so the README's "with such a duplicate" warning would not reach this user. It also means cache_tools=True stops meaning "list once" after any refusal (a cached server is re-listed every turn while the workflow keeps catching the error). Suggest keeping the refused _MCPListing on the agent for cached providers and re-reconciling from it instead of re-running the activity.

[campoy]

Fixed by the rewrite: a cached server is listed once, refused or not, and later turns check its stored listing again. In your scenario the next turn runs only q-list-tools, as on main. test_mcp_duplicate_refused_again_after_caught[True] and test_mcp_failed_listing_changes_nothing assert the listing counts.

[DABH]

An MCP tool named like a sandbox-vended default is refused, unlike native Strands. TemporalAgent(tools=[TemporalMCPClient('s')], sandbox=TemporalSandbox('sb')) where s exposes sandbox_shell (directly, or via MCPClient(prefix='sandbox') over a server exposing shell): load_tools() is empty at construction, so Agent.__init__ registers the vended sandbox_shell, and the first-turn hook finds the listed name held by a tool it does not own and raises on every turn. Native strands.Agent(tools=[provider], sandbox=...) lets the provider tool win (Strands skips the sandbox tool with a debug log), and so did main. The outcome also depends on construction order (an agent built after the shared client listed accepts it). Either let listings replace sandbox-vended defaults (record their identities at construction) or narrow the README claim and add a test documenting the refusal.

[campoy]

Went with the first option. TemporalSandbox.get_tools() now returns the same tool objects on every call, the agent records the ones Strands registered, and a listed tool with the same name replaces them, as on main and in native Strands. A -/_ variant of a sandbox tool's name is still refused, as Strands does when it registers the sandbox tools. test_mcp_tool_takes_a_sandbox_tool_name covers it.

[DABH]

taken consults only reg.registry, never reg.dynamic_tools. With structured_output_model=Weather and an MCP tool whose agent name is Weather, Strands registers the structured-output tool in dynamic_tools before the hook, so this check passes and the MCP tool is registered. get_all_tools_config prefers registry (the model sees the MCP schema) but _lookup_tool prefers dynamic_tools (calls hit the pydantic validator); with cache_tools=False the cycle-2 re-list's replace() then deletes the structured-output tool, the forced structured-output call is dispatched to the server as a real p-call-tool, and the next invocation raises from register_dynamic_tool before any hook runs, wedging the agent. Reproduced on this branch and on main. Pre-existing, but the README now promises "raises ValueError, as Strands does". One-line fix: name in reg.registry or name in reg.dynamic_tools, and check identity before the dynamic_tools.pop in the drop loop below.

[campoy]

Fixed: the check includes dynamic_tools, so an MCP tool named like the structured-output tool is refused. The drop no longer touches dynamic_tools: MCP tools are never dynamic, so there it could only remove another tool's entry. test_mcp_tool_named_like_a_dynamic_tool_is_refused covers it.

[DABH]

Two refusal mechanisms with two message formats. The exact-name taken check hand-copies Strands' message and adds the prefix/tool_filters hint, while the -/_ case is left to Strands behind this two-dict snapshot and the clear()+update() rollback below. Inside the try, replace() runs only for names already registered and register_tool() only for new names, so the only exception it can raise is Strands' normalised-name check (verified by instrumenting about 28k hook calls). A user whose second server lists foo_bar beside another server's foo-bar gets Strands' message with no hint, while the same-name case gets the hint (the tests pin both texts). One up-front validation of exact and normalised names against set(reg.registry) - drop plus the batch so far makes the mutation loop infallible and removes the snapshot, the rollback and the second refuse(). A variant doing this passed the PR's hook-level tests and a 3000-scenario differential against this PR with identical exceptions and state.

[campoy]

Done: one check up front covers exact and -/_ names, against each other and the agent's other tools, so registration can't fail afterwards. The snapshot, the rollback and the second refuse() are gone, and both cases get the same hint.

[DABH]

Documentation and diagnostics do not match the behavior: (1) "the agent's first model call raises ValueError" is wrong for cache_tools=False, where a clash that appears after a server restart raises at that later turn (the PR's own test_mcp_name_moves_between_servers does exactly this). (2) The unchanged next paragraph's "list the tools just once ... one fewer activity per turn" is false after a refusal. (3) The TemporalMCPClient docstring's "Multiple handles may reference the same server name" now yields a first-turn refusal if two such handles are on one agent. (4) The error names neither the holding server nor that prefix/tool_filters cannot unstick an in-flight run, because the recorded listing replays. The upgrade note should also name the version once chosen.

[campoy]

All four addressed:

1. The README now says the model call after the listing raises: the first one, or, with cache_tools=False, a later one if a server starts listing such a name.
2. True again: a refused cached server is not listed again.
3. The docstring now says two handles for one server go on different agents.
4. The error names the servers (MCP servers 'echo_a' and 'echo_b' both list it.), and the README says a new prefix or filter doesn't fix a run that already met the error: reset it to before that listing.

I'll put the version in the upgrade note once the release is picked.

[DABH]

The hook-level tests call closures from the private _make_mcp_refresh_hook with SimpleNamespace events, so they never execute the callbacks registered on agent.hooks. Verified with mutants: registering the MCP hooks in reverse order leaves these tests identical and green, while the production path (agent.hooks.invoke_callbacks_async(BeforeModelCallEvent(...)), which works outside a workflow) lists server c twice per turn and keeps c's tool instead of a's. A mutant that leaves a stale tool object in place instead of reg.replace(tool) passes all 9 hook cases because registry[name]._server == 'a' holds for any object the client ever produced. test_mcp_duplicate_tool_names_are_refused also does not assert get_activities(history) == ['echo_a-list-tools', 'echo_b-list-tools'], unlike its siblings. Suggest driving the in-order scenarios through invoke_callbacks_async, asserting identity against the provider's latest listing, and asserting the activity sequence; only the failed-listing test needs direct out-of-order invocation.

[campoy]

Reworked: the hook tests now go through agent.hooks.invoke_callbacks_async(BeforeModelCallEvent(agent=agent)), compare registry entries by identity with each server's latest listing, and count the listings. With one hook there's no order left to get wrong. test_mcp_duplicate_tool_names_are_refused now asserts the Activity list too.

[campoy]

On the gate from the review summary: no gate, by decision. For now these plugin fixes add no version gates and accept that some runs from earlier versions don't replay; the description says which runs and how to upgrade.

I reran the replay checks on the ungated build (d3f653a) against a dev server, with runs recorded on main at 63d7aec: the 9 runs whose names never clashed replay, and the 11 that met a duplicate don't. Details are in the description. I also added two main-recorded histories as tests: legacy_mcp_distinct_names.json must replay and legacy_mcp_duplicate_names.json must fail with a nondeterminism error.

[brianstrauch]

Reviewed d3f653a against base 63d7aec. make lint and all 153 existing tests pass with Python 3.13 and uv 0.12.5. Two additional hook-level regression checks pass against the base implementation and fail against this head; details are inline.

[brianstrauch]

[P2] Scope the cached-tool skip to the provider this agent accepted.

_mcp_names records names across every provider, so this condition can skip a cached provider whose tool has never been installed on this agent. Reproduction:

1. Let a warm-up agent list client A's echo.
2. Construct the target agent with [A, B], where B has cache_tools=True and has not listed yet. Construction installs A's echo through load_tools().
3. Let another agent list B's echo.
4. Have A stop listing echo, then run the target's before-model hooks.

Validation succeeds, but echo is in both new and the old _mcp_names, so it is not removed; B is then skipped here. The registry retains A's old stub and tool calls dispatch to a-call-tool even though only B now lists the name. On the base implementation this scenario loses the tool; this change newly leaves a callable routed to the wrong server.

Only skip a cached listing when this agent already accepted that provider's tool, rather than any MCP tool with the same name.

[campoy]

Thanks, fixed in 487e5aa by dropping the skip, which the other review also suggested. Every listed tool is now applied each turn, so in your scenario echo comes from B. test_mcp_name_moves_to_a_cached_server follows your steps.

[brianstrauch]

[P2] Remove a dropped MCP-owned tool's dynamic replacement too.

Removing only reg.registry[name] leaves the tool callable if its in-place replacement is dynamic. Reproduction: accept an MCP echo, load a replacement @tool(name="echo") through agent.tool_registry.process_tools([path]), then have the server return an empty listing. Strands marks the loaded replacement dynamic and stores it in both maps, but this loop removes only the main entry.

After refresh, get_all_tools_config() still advertises echo from dynamic_tools, and Strands' executor still resolves and executes that replacement. This regresses the base implementation, which removed it from both maps. Remove the matching dynamic entry when dropping the MCP-owned name, with an ownership check if needed to preserve unrelated dynamic tools.

[campoy]

Fixed in 487e5aa: a dropped name is removed from dynamic_tools too, as on main. No ownership check is needed there: register_dynamic_tool refuses names that are in the registry, so a dynamic entry under an MCP name can only be a replacement of the MCP tool. test_mcp_listing_replaces_a_tool_replaced_in_place now drops a dynamic replacement and checks both maps.

[DABH]

Second pass, on the rewritten head. The single-hook design, the name-based ownership set, the dynamic_tools check, the sandbox precedence rule, the legacy histories and the README notes address most of the previous round: the duplicate-instance, shared-cached-client, refused-batch and dynamic-tools findings no longer reproduce, and the hook-level tests now drive the real hook registry. Lint and the 17 hook-level plus legacy-replay tests pass; one finder fuzzed the hook with 12,000 random multi-turn trials and found no leaked Strands error. What follows was reproduced on this head unless marked otherwise.

Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

The gate question stands as documented (the author's call; the precedent is the gated context-window change in _temporal_model.py). The one new replay note is below on line 186: the old version also raised for some clashes, and those runs break too.

Not re-raised: the altitude alternatives weighed this round (listing in _convert_prompt_to_messages, a probe ToolRegistry to inherit Strands' rule, identity-based ownership) are not clearly better than what is here; _check_mcp_tools copies Strands' two-line rule and message text, which is a maintenance coupling worth a comment but not a defect. Deduping _mcp_providers or listing providers concurrently would both need a workflow.patched gate and are out of scope.

[DABH]

P2: the drop loop no longer pops reg.dynamic_tools, which main did. If a user replaced an MCP tool in place with a dynamic-marked tool (wrapper.mark_dynamic(); agent.tool_registry.replace(wrapper), or a path-loaded tool via process_tools, which Strands marks dynamic and lets overwrite because DecoratedFunctionTool.supports_hot_reload is True), then the server drops the name: only registry is popped, dynamic_tools['echo'] survives, get_all_tools_config() still advertises it and the executor resolves dynamic_tools first, so the model keeps calling a tool the server removed. When the server lists the name again, held (built from dynamic_tools) contains it and _check_mcp_tools raises 'another tool of the agent has it' on every later turn (reproduced by four independent probes). Either pop both dicts as before, or neither.

[campoy]

Fixed in 487e5aa: it pops both dicts again, as on main. test_mcp_listing_replaces_a_tool_replaced_in_place now covers a dynamic replacement.

[DABH]

P2: _vended is an identity match against sandbox.get_tools(), which breaks in both directions. (1) A TemporalSandbox subclass that overrides get_tools() to build fresh tools per call (the pattern Strands' own DockerSandbox/SSHSandbox use, e.g. to customize descriptions) yields _vended == {}, so a server listing sandbox_shell is refused with 'another tool of the agent has it' instead of taking the name as the README promises and as main did (reproduced). (2) A sandbox tool the user passed explicitly, tools=[*sb.get_tools(), a] or tools=[sb.get_tools()[1]], is the same object, so it is classified as vended and silently replaced by a listed tool, contrary to the rule that tools=[...] wins (reproduced). Nothing documents that get_tools() must return stable instances. Record what the sandbox loop in Agent.__init__ actually registered (by name plus origin) rather than relying on object identity, or state the contract on TemporalSandbox.get_tools.

[campoy]

Agreed, the identity match was fragile. I switched to the other option from your first round: an MCP tool named like a sandbox tool is now refused, like any other tool of the agent, and TemporalSandbox is back to main's version. The README rule lists sandbox tools, and test_mcp_tool_named_like_a_sandbox_tool_is_refused documents the refusal.

[DABH]

P2: a vended tool that gave way to an MCP tool is never restored. Turn 1: server a (cache_tools=False) lists sandbox_shell, the takeover is allowed and replace() installs the MCP tool. Turn 2: the restarted server no longer lists it, this loop pops the name, and the registry is left with sandbox_file_editor only while _vended still holds the shell tool (reproduced). The agent loses its shell for the rest of the run after a transient name overlap, which neither the tools=[...] analogy nor native Strands (static tool set) predicts. Re-registering self._vended[name] when popping a name it holds makes the precedence symmetric.

[campoy]

Moot now: a listed tool no longer takes a sandbox tool's name (see the thread on line 173), so a sandbox tool is never replaced.

[DABH]

P3: the two-servers message is unactionable for two handles of one server. TemporalAgent(tools=[TemporalMCPClient('x'), TemporalMCPClient('x', cache_tools=True)]) (the case the TemporalMCPClient docstring now describes) produces "MCP servers 'x' and 'x' both list it. Set a different prefix on one server's MCPClient", but both handles resolve to the single worker-side factory for x, so there is no second prefix to set (reproduced). holder is provider is False so the 'lists it twice' branch is not taken either, and that branch's wording is also off for one server listing foo-bar and foo_bar (two distinct names). Two handles with the same server are knowable in __init__; refusing there with 'pass one handle', or a dedicated message when holder.server == provider.server, would help. The two-handle case previously worked (the second handle's tools won), so in-flight runs using it also stop replaying.

[campoy]

Fixed. Two handles of one server now get "The agent has two TemporalMCPClient handles for MCP server 'x'. Pass only one.", and one server that lists foo-bar and foo_bar gets "MCP server 'x' lists both names." Both are cases in test_mcp_server_listing_a_name_twice_is_refused, and the README replay note names the two-handle case.

[DABH]

P3: 'is not a duplicate' holds only before the client has listed. load_tools() returns the current _tools, so an agent built after a shared client listed (a module-level client, as the docstring recommends, with a second agent later in the workflow) fails at construction from Strands: TemporalAgent(tools=[a, a]) raises ValueError: Failed to load tool <TemporalMCPClient ...>: Tool name 'echo' already exists. Cannot register tools with exact same name. (reproduced); tools=[_local_echo, a] fails the same way, while tools=[a, _local_echo] constructs because @tool functions are hot-reloadable and silently overwrite the MCP tool, which the hook then refuses on turn 1. Pre-existing Strands behavior, but this test and the README present construction order as irrelevant; mention construction-time refusal for agents built after a listing.

[campoy]

Added. The README now says that an agent built after its clients listed gets their tools at construction, so Strands may raise there instead. This test also checks that TemporalAgent(tools=[a, a]) raises at construction once a has listed.

[DABH]

P3 simplification: if registry.get(tool.tool_name) is tool is dead on the normal path and wrong on the only other one. After a successful Agent.__init__ every tool a provider's load_tools() returned is in the registry (process_tools raises otherwise). The only way the condition is false is an AgentInitializedEvent hook that replace()s a provider tool during construction, and there the filter leaves the wrapped name out of _mcp_names, so the first turn refuses the server's own tool with 'another tool of the agent has it', contradicting the in-place-replacement rule the tests pin. {t.tool_name for p in self._mcp_providers for t in p._tools} is simpler and consistent. The same identity filter on _vended (line 173) is subsumed by the check-time identity test at line 235, so _vended could be the plain list of the sandbox's tools (or read at check time) and the vended is None or ... clause goes.

[campoy]

Done: _mcp_names is now the plain set of the providers' tool names. _vended is gone with the sandbox change.

[DABH]

P3: the refresh hook is registered last at default order, so every user hook from hooks=[...] and Strands' own before-model hooks observe the previous turn's registry. A user hook that gates or cancels the model call based on available tools decides on stale data (and the listing activities, and the duplicate refusal, still run even though the model call is cancelled), and event.projected_input_tokens (consumed by ConversationManager proactive compression) excludes all MCP tool specs on turn 1 (reproduced). Pre-existing ordering, but the single-hook rewrite is the natural place to register with order=HookOrder.SDK_FIRST.

[campoy]

I left the order as is. Moving the hook first would schedule the list-tools Activities before any Activities that user BeforeModelCallEvent hooks schedule (for example with activity_as_hook). That changes the commands of runs whose names never clashed, so like deduping or listing concurrently, it belongs in its own change.

[DABH]

P3: vended tool objects are now shared by every agent that receives the same TemporalSandbox, so per-instance mutable state (mark_dynamic(), the tool_spec setter) leaks between agents where main vended fresh instances (reproduced: a1.tool_registry.registry['sandbox_shell'] is a2... is True). No Strands library path mutates vended tools, so this needs user code; low. The eager construction also runs make_file_editor/make_shell (two pydantic create_model calls, ~0.8 ms) on every TemporalSandbox(...) even when it is used standalone and never attached to an agent; building lazily on the first get_tools() keeps the identity the agent relies on.

[campoy]

Moot now: TemporalSandbox is back to main's version, which makes new tools on each call.

[DABH]

P3: this pins sdk-core's exact nondeterminism wording. A temporalio bump picked by the nightly sync-latest lane that rewords 'Complete workflow machine does not handle this event' (it has changed before) breaks the test and opens a spurious nightly issue although replay still correctly raises NondeterminismError. Matching only the exception type, or ActivityTaskScheduled, checks what the test proves.

[campoy]

Done: it now matches only NondeterminismError.

[DABH]

P3: _legacy_history is the third in-plugin copy of the recorded-history loader (tests/test_legacy_history_replay.py has HISTORIES plus _legacy_history, and tests/test_model_context_overflow_legacy_replay.py inlines it again), each with a different workflow-id argument. Promote one load_history(name, workflow_id=...) to tests/common.py, which this file already imports.

[campoy]

Done: load_history(name, workflow_id=...) is in tests/common.py, and all three files use it.

[campoy]

Thanks both. Round 2 is in 487e5aa, after a merge of main. Replies are inline. I also added a comment on _check_mcp_tools saying it applies ToolRegistry.register_tool()'s rule, and updated the description with the new replay results.

[brianstrauch]

Reviewed 487e5aa. make lint and all 190 shipped tests pass. One remaining upgrade-warning gap was confirmed with an additional recorded-history replay check.

[brianstrauch]

[P2] Include shared-client tool removals in the upgrade warning.

Tracking removals per agent also changes replay for runs whose tool names never clashed. Reproduction: construct agents A and B with the same workflow-local, uncached TemporalMCPClient; A lists echo, B subsequently lists other, then A runs again with other still listed. The base implementation retains A's old echo because it computes old_names from the shared client's latest listing; this loop now removes it.

I recorded a run on the exact base agent where the model calls echo on A's later turn. The history replays successfully on base but fails on this head with [TMPRL1100]: the scheduled review-call-tool Activity is replaced by a review-list-tools command because echo is no longer registered. No duplicate names occur, and the client is created inside the Workflow, so this is separate from the acknowledged cross-run client-state issue.

The new removal behavior fixes stale tools, but the README's duplicate-only upgrade warning and the PR's claim that non-clashing runs retain their command sequence miss this case. Please expand the warning to cover shared clients whose listings drop or rename tools, and add a recorded-history regression test; alternatively preserve compatibility for this path.

[campoy]

Thanks. I kept the new behavior, which drops the stale tool, and covered it in f2a5bf1:

• The README upgrade note now names an agent that shares an uncached client whose listing drops or renames tools.
• legacy_mcp_shared_client_drop.json, recorded on main at 63d7aec with your scenario (A lists echo, B lists other, A calls echo on its second turn), is a new case in test_mcp_rejects_history_with_changed_tools, which expects its replay to fail.
• The PR description no longer says that every run without a clash keeps its commands.

[DABH]

Third round, at head 487e5aa (six commits over 3488fbe). Recipe: the finder angles plus a replay-semantics finder, 32 candidates and checks in total (including a 14-mutant run of _refresh_mcp_tools and _check_mcp_tools in a separate synced worktree and seven wrapper-semantics probes), each settled by a fresh reproduction against an independent worktree: hook-level probes against both the head and main's agent class, end-to-end dev-server runs with WorkflowTaskFailed and attempt-counter evidence, and a real recording replayed across versions. Confirmed 10, merged into 8 findings; 1 plausible; 7 probes and 11 mutants clean. make lint and make test (190 tests) pass on the head; the conventions check is OK. Not verified: the shared-client history a commenter recorded (not attached; reproduced at hook level, and the sibling one-turn-move case with a real recording), a mixed old-and-new Worker rollout, and cross-run client state on an unsandboxed Worker, which the PR lists as not covered.

Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

Previous rounds. Round one is addressed or superseded throughout: one hook per agent with the identity bookkeeping gone, listings keyed by provider, cached providers not re-listed, the dedicated MCPToolNameConflictError registered as a failure type, the shared cached client getting its tools, the twice-listed name, name-based ownership for in-place replacement, dynamic_tools included, a single up-front check, the README and docstrings, and the tests driving agent.hooks. Round two: the drop loop pops dynamic_tools, the two-handles message is actionable, the check runs inside the loop, exact comparison for dynamic names, the loader shared, the nondeterminism assertion narrowed, the vended-tool threads superseded by the refuse decision. Decisions I take as recorded: no gate, the name-is-the-server's ownership rule, the hook order, the AfterModelCallEvent note. Still partial, and the source of this round's items: _mcp_names is still seeded from provider._tools at construction, the failure-type guarantee holds only for a cycle-one refusal, and the upgrade note lists only the duplicate cases.

Where this round's problems cluster: two things decide whether the new guarantee holds. First, where the refusal is raised: after a tool use, Strands wraps it in EventLoopException, which is not a registered failure type, so the run strands exactly on the mid-run duplicate the README documents. Second, when the agent was built: an agent constructed after its clients already listed takes the construction-time path, where the same duplicate is a bare ValueError, a user tool is silently replaced, and a sandbox-named tool is accepted. Both are fixable in the plugin without changing commands for runs that never clash.

Findings on lines outside the diff

• python/strands_agents/src/temporalio/strands_agents/_temporal_agent.py:167: P2: the same duplicate reached at construction, for an agent built after its clients listed, surfaces as Strands' bare ValueError, not MCPToolNameConflictError, so it is not a failure type, cannot be caught with the documented except, and strands the run; its message names a TemporalMCPClient object address instead of the server. Module-level clients a and b (the pattern the TemporalMCPClient docstring recommends): agent one tools=[a, b] is refused at its hook (both list echo), the Workflow catches it as README line 643 suggests, then builds agent two from the same clients (or any second agent with tools=[local_echo, a] or [a, a] after a listed). super().__init__ here runs Strands' process_tools, load_tools() (_temporal_mcp_client.py:114-123) returns the stored listing, and register_tool raises ValueError: Failed to load tool <temporalio.strands_agents._temporal_mcp_client.TemporalMCPClient object at 0x...>: Tool name 'echo' already exists. End to end: the run stays RUNNING with one WorkflowTaskFailed of type ValueError, non_retryable: false, and the pending task attempt climbing 2, 3, 4 over 35 s. README line 641 hedges 'Strands may raise there instead', but line 643 promises 'StrandsPlugin fails the Workflow when the error escapes Workflow code', which is false on this path, and the PR's own test_mcp_same_client_twice_on_one_agent (line 552) pins the bare ValueError. Check each provider's current _tools names against tools=[...] and each other before super().__init__ and raise MCPToolNameConflictError (replay-neutral: no Activity is scheduled at construction), or translate the construction-time ValueError; and give the client a __repr__ naming the server.
• python/strands_agents/src/temporalio/strands_agents/_failure_converter.py:29: P3: MCPToolNameConflictError fails the Workflow as a retryable ApplicationError (non_retryable=False), unlike the plugin's other deterministic Strands failures in _TERMINAL_EXCEPTIONS. A parent running execute_child_workflow(..., retry_policy=RetryPolicy(maximum_attempts=0)), or a Workflow started with a retry policy, for an agent whose two servers both list search lists both, refuses, fails with type MCPToolNameConflictError, and is retried with backoff for a configuration error that cannot succeed (observed end to end: cause.non_retryable = False). Adding the type here, converted with non_retryable=True, matches how MaxTokensReachedException, ContextWindowOverflowException, StructuredOutputException and SessionException are treated.
• python/strands_agents/src/temporalio/strands_agents/_temporal_mcp_client.py:123: P3 (design alternative, plausible): load_tools() returning the process-local _tools at construction, plus seeding _mcp_names from it, is the single root of the construction-order dependence behind the construction-time ValueError, the clobber and the sandbox-order items; returning [] and starting _mcp_names empty is replay-neutral and makes every clash a hook-level MCPToolNameConflictError. Simulated without editing the package: list-tools counts are identical to the head for an agent built after an uncached listing (one on its first turn), after a cached listing (zero; tools applied from the stored listing via 215-219), a shared cached client (one listing), and [a, a] after a listing (two per turn, as a fresh [a, a] agent); [local_echo, a], [a, local_echo] and a sandbox agent built after a listing are all refused uniformly by the hook. Running the PR's hook-level tests under the variant: 16 pass, 1 fails, the failure being test_mcp_same_client_twice_on_one_agent's assertion that construction raises after a listing, which the variant removes on purpose. Costs: agent.tool_names and the registry are empty before the first hook for an agent built after a listing (nothing in the plugin reads them earlier; user BeforeModelCallEvent hooks already observe the previous turn's registry), and the README sentence about construction-time raising would go. A smaller alternative is restoring d3f653a's identity guard for _mcp_names plus the construction-time translation above.

Minor doc nit: README line 641 says a tool put under a server's name 'is replaced at the next listing'; with cache_tools=True there is no next listing, so it is replaced at the next turn. Process note: the automated run fetched pull/184/head into the local checkout's pr-184-review branch ref; the working tree and index were not touched, and all verification used an independent clone.

[DABH]

P1: a refusal raised on any event-loop cycle after a tool use is wrapped by Strands into EventLoopException, which is not a registered Workflow failure type, so the run strands in Workflow Task retries and the documented except MCPToolNameConflictError or except ValueError never runs. Default cache_tools=False, TemporalAgent(tools=[TemporalMCPClient('a'), TemporalMCPClient('b')]): cycle one lists a {safe} and b {echo}, the model calls safe; before cycle two's model call a now also lists echo (a server redeployed mid-run, the case README line 641 covers: 'with cache_tools=False, a later one if a server starts listing such a name'). _refresh_mcp_tools raises at _temporal_agent.py:220, but cycle two runs inside recurse_event_loop, inside the outer cycle's second try in strands/event_loop/event_loop.py:394-410, whose pass-through list is only StructuredOutputException, EventLoopException, ContextWindowOverflowException, MaxTokensReachedException and InterruptException; everything else becomes EventLoopException(e) from e. Only a cycle-one refusal (the model-call try at 296-310) propagates unwrapped, which is the one case the PR's end-to-end test (tests/test_mcp.py:377) exercises. Reproduced on the dev server: activities [strands_get_model_config, a-list-tools, b-list-tools, invoke_model, a-call-tool, a-list-tools, b-list-tools], then RUNNING after 12 s with one failed task of type EventLoopException (cause MCPToolNameConflictError), retried with backoff until a reset or terminate. The caught variant: except ValueError does not fire; except Exception sees the chain [EventLoopException, MCPToolNameConflictError] with agent.messages holding prompt, tool use and tool result, so README line 643's 'remove the prompt that Strands added' is incomplete there too. The same wrapping turns an invoke_model ActivityError after a tool use into a task-failure loop on main as well, so one fix covers both: in TemporalAgent, unwrap an EventLoopException whose __cause__ is a FailureError or a registered failure type and re-raise the cause; registering EventLoopException itself would be too broad. README 641-643 and the MCPToolNameConflictError docstring (_temporal_mcp_client.py:36-42) should say which exception actually escapes.

[campoy]

Fixed in f2a5bf1: TemporalAgent._run_loop re-raises an MCPToolNameConflictError that Strands wrapped in EventLoopException, so Workflow code can catch it, and the plugin fails the Workflow with it. test_mcp_conflict_after_a_tool_call runs your shape end to end (a tool call, then a listing that clashes), both caught and uncaught; without the change it times out. The README now says to remove every message the call added (the prompt, and any tool calls and results).

I kept the broader unwrap for Temporal failures out of this PR. A failed or cancelled model Activity after a tool call is an existing problem on main that needs its own fix (it also has to cover cancels and Signal and Update handlers), so I'll track it separately.

[DABH]

P2: _mcp_names is seeded by name from provider._tools even when the registry entry under that name is the user's own tools=[...] tool, so the first hook silently replaces an explicitly passed tool instead of refusing the duplicate README line 641 promises. a already listed echo (earlier agent in the run, module-level client); TemporalAgent(tools=[a, local_echo]) with @tool(name='echo') local_echo: process_tools registers a's echo, then the hot-reloadable @tool overwrites it without error, so registry['echo'] is local_echo while _mcp_names == {'echo'}; the hook's held excludes echo (line 210), _check_mcp_tools passes, and reg.replace() at 232 installs the TemporalMCPTool over the user's tool (probe: hook: NO error; registry['echo'] type = TemporalMCPTool). The same agent with a not-yet-listed client is refused, so the outcome depends on whether another agent listed the client first. Main clobbered in both cases, so the behaviour is pre-existing; what is new is the README rule it contradicts and the inconsistency with the fresh-client path. The identity guard d3f653a had (if registry.get(tool.tool_name) is tool, removed in 487e5aa on the premise that every load_tools() tool stays in the registry) would exclude echo from _mcp_names and make the hook refuse; or seed nothing at construction (last comment).

[campoy]

Fixed in f2a5bf1 with your last option: TemporalMCPClient.load_tools() now returns no tools and _mcp_names starts empty, so every agent gets its MCP tools from the hook, whether it was built before or after the listing. [a, local_echo] built after a listed is now refused like a fresh agent, and the user's tool stays. test_mcp_tool_named_like_another_tool_is_refused builds agents both before and after the listing.

[DABH]

P2: two non-clashing paths change the command sequence relative to main and are missing from this upgrade note and from the PR's 'runs whose tool names never clashed schedule the same commands' claim. (a) Shared uncached client c in agents A and B: A lists {echo}; B then lists {other} (server changed); A runs again. Main computed old_names from the client-wide c._tools (B's listing), so A kept its stale echo stub; the head's per-agent self._mcp_names - new (lines 224-226) pops it (probe: head A tools = ['other'], main ['echo', 'other'], identical list-tools logs). If the model calls echo on A's later turn, main schedules c-call-tool and the head does not, so a main-recorded history fails replay with TMPRL1100; the still-unanswered comment 4417600465 on this PR reports exactly that recording. (b) tools=[b, a], uncached; turn one a={safe, echo}, b={other}; turn two a={safe, other}, b={echo}; the model calls echo. On main, hook(a)'s old_names - new popped the echo hook(b) had just installed, so the model got 'Unknown tool: echo' and no Activity; the head keeps b's echo (221-235, the intended 'a name can move' behaviour) and schedules b-call-tool. Reproduced end to end: a history recorded with main's agent class replays on main and fails on the head with [TMPRL1100] ... Activity type of scheduled event 'gap_mv_b-list-tools' does not match activity type of activity command 'gap_mv_b-call-tool'. The head's behaviour is the better one in both cases; the actionable part is this note (today it lists only duplicates, two handles and '-'/'_' names), the PR body, and main-recorded regression histories for both paths next to legacy_mcp_duplicate_names.json.

[campoy]

Agreed on both. In f2a5bf1:

• The README upgrade note covers them: a tool that moves between servers in one turn now stays, and an agent that shares an uncached client now drops the tools the server stopped listing. The PR description says the same.
• Both have main-recorded histories next to legacy_mcp_duplicate_names.json: legacy_mcp_shared_client_drop.json, and legacy_mcp_tool_moves.json (your tools=[b, a] swap; on main the model's echo call found no tool). test_mcp_rejects_history_with_changed_tools expects both to fail replay.

[DABH]

P2: whether an MCP tool named like a TemporalSandbox tool is refused depends on construction order; an agent built after its client listed silently takes sandbox_shell, and this drop loop then removes it without restoring the vended shell when the server stops listing it. a lists sandbox_shell (for example MCPClient(prefix='sandbox') over a server exposing shell). Built before the listing, TemporalAgent(tools=[a], sandbox=TemporalSandbox('sb')) is refused at the first hook, as README line 641 and tests/test_mcp.py:729 say. Built after it, process_tools registers a's sandbox_shell, Strands' Agent.__init__ skips the vended tool with a debug log, line 190 claims the name for the servers, the hook accepts it, and when a later lists nothing, this line pops the name and the registry is left with sandbox_file_editor only for the rest of the run (probe output: (b) hook: NO error, then registry={'sandbox_file_editor': ...}). Main behaved like the second path on both, so the PR adds the inconsistency and the README claim; the 'moot' reply on the vended-tool-restore thread holds only for the first path. Either refuse at construction too (replay-neutral), or record what the sandbox loop registered and restore it on drop, or narrow the README sentence.

[campoy]

Fixed by the same change as the thread on line 190. With load_tools() returning nothing, an agent built after its client listed gets the sandbox tools at construction, and the hook refuses sandbox_shell, as for a fresh agent. So a listed tool never takes a sandbox name, and the drop can't remove one. test_mcp_tool_named_like_another_tool_is_refused covers both build orders.

[DABH]

P3: a structured_output_model whose tool name equals an MCP tool the agent already accepted raises Strands' bare ValueError from register_dynamic_tool before any hook runs; the dynamic check here covers only the opposite order, and the error is neither MCPToolNameConflictError nor a failure type. Server a lists Weather, the agent accepts it on turn one; invoke_async(prompt, structured_output_model=Weather) runs StructuredOutputContext.register_tool in _execute_event_loop_cycle (strands/agent/agent.py around 1693, before the BeforeModelCallEvent hook) and gets ValueError: Tool 'Weather' already exists. A plain Strands registry with a user tool named Weather raises the same, so this is Strands parity and pre-existing; uncaught, it fails the Workflow Task repeatedly like the construction case. README line 641 and the body ('dynamic tools such as structured output's by exact name') read as if both directions are covered. Either compare _mcp_names with the structured-output tool name before the cycle (_convert_prompt_to_messages does not receive structured_output_model; the check belongs in invoke_async, stream_async or _execute_event_loop_cycle) or document the gap.

[campoy]

Documented in f2a5bf1 rather than checked: Strands does the same with any tool of that name. The README now says that a structured_output_model named like an MCP tool the agent already has gets Strands' own ValueError when the call starts, as in Strands.

[DABH]

P3 (tests): three behaviour-changing mutants of _refresh_mcp_tools and _check_mcp_tools survive this file, including one that turns the user-tool '-'/'_' twin path into a bare Strands ValueError and a task-failure loop. From a 14-mutant run in a separate synced worktree (baseline 24 passed): (M5) comparing name in held instead of the normalized key in held at _temporal_agent.py:80 passes all 24, yet with a user @tool(name='foo-bar') and a server listing foo_bar the head refuses via MCPToolNameConflictError while the mutant lets register_tool raise Strands' 'already exists as' ValueError; no test has a user tool whose name differs from a listed name only by '-' or '_'. (M2) dropping - self._mcp_names from dynamic at 212 passes; it would refuse a re-listing of a name whose MCP tool a dynamic replacement currently holds instead of replacing it, and test_mcp_listing_replaces_a_tool_replaced_in_place (706) covers a dynamic replacement only when the server drops the name. (M13) removing the 'Set a prefix ... tool_filters' hint passes; the tests match only the message prefix. The other 11 mutants are killed. Add a user foo-bar plus listed foo_bar refusal test, a re-listing over a dynamic in-place replacement, and pin the full hint once.

[campoy]

Thanks for the mutant run. Added in f2a5bf1:

• A user foo_bar with a listed foo-bar, as a case of test_mcp_tool_named_like_another_tool_is_refused. I used this direction because it also catches a mutant that compares the raw name with held's values; the same test pins the full hint once.
• A re-listing over a dynamic in-place replacement, in test_mcp_listing_replaces_a_tool_replaced_in_place.

[campoy]

Thanks both. Round 3 is in f2a5bf1, with replies inline. The items in the review summary:

• Construction-time ValueError (P2): gone. load_tools() now returns no tools, so an agent built from clients that already listed gets them from the hook, which raises MCPToolNameConflictError as for any agent. test_mcp_duplicate_refused_again_after_caught now builds a second agent from the same clients and expects that. With no construction-time message left, the client __repr__ isn't needed.
• Retryable failure (P3): MCPToolNameConflictError is now in _TERMINAL_EXCEPTIONS, so the Workflow fails with a non-retryable ApplicationError. test_mcp_conflict_after_a_tool_call checks cause.non_retryable, with the plugin on the client, since the failure converter comes from the client's data converter.
• Design alternative: taken, as above.
• Doc nit: the README now says a tool put under a server's name is replaced at the next turn.

The replay checks were rerun on this head; the results are in the description.

[DABH]

Fourth round, at head f2a5bf1 (one commit over 487e5aa). make lint passes and make test runs 195 tests green on the head; CI is green. Each item below was settled by a reproduction in a separate worktree: end-to-end dev-server runs comparing the head's TemporalAgent with a subclass that restores main's _run_loop, the two new histories replayed on main's agent (the plugin's source on main is unchanged since 63d7aec), a rerun of the two round-three mutants that had survived, and Strands' source from 1.51.0 to 1.56.0 and at 1.62.0. Checked and clean: both new histories replay on main and fail on the head with the Activity swaps the PR describes; M5 (name in held) and M2 (dynamic without - self._mcp_names) now fail one test each; each of those Strands releases sends stream_async through _run_loop and wraps the later-cycle error as EventLoopException(e) from e, so the unwrap applies across the declared range; an agent built after a listing schedules the same list-tools Activities as on main (the hook still skips _refresh for a fetched cached client and applies its stored listing); the exception cycle that raise err.__cause__ from None leaves through __context__ is harmless, since from None clears __cause__ and both failure converters build a fresh ApplicationError for this error that carries only __cause__. Not verified: the 20-run replay matrix and the old-to-new Worker runs in the PR body (not re-run), and a mixed rollout, which the PR lists as not covered.

Severity: P1 = wrong result or stuck run in realistic use; P2 = correctness or robustness in an edge case; P3 = docs, tests, cleanup.

Previous rounds. Round three is resolved. The P1 is addressed: _run_loop re-raises a conflict that Strands wrapped after a tool call, and test_mcp_conflict_after_a_tool_call runs that shape caught and uncaught. The construction-time ValueError, the silent replacement of a user tool and the sandbox-name order dependence are gone with load_tools() returning [] and _mcp_names starting empty (the design alternative, taken), with tests for both build orders. The upgrade note and the PR body now cover the shared-client drop and the one-turn move, each with a main-recorded history, which also answers the other reviewer's shared-client comment. MCPToolNameConflictError is in _TERMINAL_EXCEPTIONS; the surviving mutants are killed and the full hint is pinned; the doc nit is fixed. Declines I accept: the structured_output_model name clash documented as Strands parity; the broader EventLoopException unwrap for failed or cancelled model Activities, pre-existing on main and tracked separately; the client __repr__, moot without the construction path. Earlier decisions (no gate, the name-is-the-server's ownership rule, the hook order) stand.

Decision: comment rather than approve, because every earlier item is resolved and the one new defect is an edge-case regression in the new _run_loop wrapper with a three-line fix; with that in, nothing here blocks.

#190: it moves prefix and tool_filters from the worker-side MCPClient factory to TemporalMCPClient (there, a factory that still passes prefix= fails every MCP Activity) and edits the same load_tools() lines. Whichever PR lands second should keep load_tools() returning [] and point the conflict hint (_temporal_agent.py:45-47 and 59-61) and README line 641 at TemporalMCPClient(prefix=..., tool_filters=...).

[DABH]

P2: this wrapper never closes Strands' own _run_loop generator, so when stream_async is closed early, an AfterInvocationEvent hook that awaits anything stops at that await and the hooks after it never run, for every TemporalAgent, with or without MCP clients. Since Strands 1.55 (the lock has 1.56), stream_async ends with finally: await events.aclose(), so that _run_loop's finally (conversation management, then the AfterInvocationEvent hooks) runs on the event loop when the caller stops iterating or a callback_handler raises. That aclose() now reaches this wrapper, and the async for here does not forward it: the inner generator is released with the wrapper's frame and closed synchronously when it is collected, because the Workflow loop installs no async-generator finalizer, so its first suspending await ends the close with RuntimeError: async generator ignored GeneratorExit, reported as unraisable for Agent._run_loop.

Reproduced on the dev server with an async AfterInvocationEvent hook that logs, awaits an Activity (the README's activity_as_hook pattern) and logs again. A Workflow that breaks out of agent.stream_async('hi') and closes it: main's _run_loop (a subclass with _run_loop = Agent._run_loop) logs ['after-start', 'after-end'], the head ['after-start']. A callback_handler that raises on the first text chunk: the same split, with invoke_async re-raising the handler's error in both. The hook's Activity is still scheduled and runs; the code after the await is lost. Strands 1.51 to 1.54 have no explicit close, so main already lost such a hook there; the regression is from 1.55.

Fix, verified (the probe then logs after-end in both cases; make lint is clean and the 195 tests pass):

events = super()._run_loop(*args, **kwargs)
try:
    async for event in events:
        yield event
except EventLoopException as err:
    if isinstance(err.__cause__, MCPToolNameConflictError):
        raise err.__cause__ from None
    raise
finally:
    await events.aclose()

contextlib.aclosing does the same.

[campoy]

Thanks, fixed in 2819ae6 with your version: the wrapper keeps Strands' generator and closes it in finally, so Strands' cleanup, including AfterInvocationEvent hooks that await, still runs when the stream is closed early. test_after_invocation_hook_finishes_when_stream_closed_early in tests/test_hooks.py breaks out of stream_async and closes it, with a hook that awaits. On f2a5bf1 that hook logs only start, and Strands reports async generator ignored GeneratorExit.

[campoy]

Follow-up: 9c219ce keeps the same finally and replaces my test with test_closed_stream_awaits_after_invocation_hooks. Its hook runs an Activity, and a control reads the stream to the end.

[DABH]

P3: the failure is non-retryable only when StrandsPlugin is on the client; with the plugin on the Worker only, as in the README quick start (line 44) and the MCP example (line 627), an uncaught conflict still fails the Workflow, but as a retryable ApplicationError. SimplePlugin.configure_worker does not apply data_converter, so such a Worker keeps the client's default failure converter. Probe: MCPConflictAfterToolWorkflow with catch=False on a Worker with plugins=[StrandsPlugin(...)] and a plain client fails with cause.type == 'MCPToolNameConflictError' and cause.non_retryable == False; the PR's test passes because it adds the plugin to the client, as its comment at tests/test_mcp.py:559 says. The PR body says 'with a non-retryable ApplicationError' without that condition. Add to README line 643 what line 552 says for interrupts (attach the plugin to the client for the failure to be non-retryable), and qualify the PR body.

[campoy]

Done in 2819ae6: the README now says to attach StrandsPlugin to the client for that failure to be non-retryable, as it already says for interrupts. The PR description now says the same.

[campoy]

Follow-up: 9c219ce goes further. MCPToolNameConflictError is now an ApplicationError with non_retryable=True (and still a ValueError), so the failure is non-retryable with the plugin on the Worker only too. The README sentence about the client is gone, and so are the failure-converter and failure-type entries. test_mcp_conflict_after_a_tool_call now runs with the plugin on the Worker and on the client, with maximum_attempts=2, and checks that only one run fails.

[DABH]

P3 (tests): this test accepts any NondeterminismError, so it would keep passing for an unrelated reason, for example once #190 renames the MCP Activities to temporalio.mcp.{server}.list-tools and call-tool and every earlier MCP history fails at its first listing. On the head the two histories fail exactly where the PR says ([TMPRL1100] ... scheduled event 'shifting-call-tool' does not match activity type of activity command 'shifting-list-tools', and 'mv_b-list-tools' against 'mv_b-call-tool'), and both replay cleanly on main's agent. Pin that with match= on the Activity pair (one more parametrized value); the duplicate-names test at line 446 can do the same.

[campoy]

Done in 2819ae6: the two changed-tools cases now match their Activity pair ('shifting-call-tool' against 'shifting-list-tools', and 'mv_b-list-tools' against 'mv_b-call-tool'). The duplicate-names history fails where the run completes instead of scheduling the model call, so its message has no Activity pair. That test now matches ActivityTaskScheduled, which a failure at the first listing, such as one from #190's renamed Activities, would not match.

[campoy]

Thanks. Round 4 is in 2819ae6, with replies inline. On #190: noted. Whichever of the two lands second keeps load_tools() returning [], and if #190 moves prefix and tool_filters to TemporalMCPClient, points the conflict hint and the README at TemporalMCPClient(prefix=..., tool_filters=...).

[campoy]

Pushed 9c219ce on top of 2819ae6, with three more fixes from our own review of f2a5bf1:

• Tool added while a listing runs: the agent's other tool names were taken before the first listing was awaited. A Signal handler that registered a local echo during {server}-list-tools was not checked, so the listed echo replaced it and the call ran on the server. The check now reads the registry after each listing, and nothing is awaited between the last check and the registry changes. test_mcp_tool_added_while_listing_is_refused covers a tool added during a listing (refused, kept) and one removed (the name goes to the server).
• Retryable with the plugin on the Worker only: StrandsFailureConverter applies only when StrandsPlugin is on the client, so Worker(client, plugins=[StrandsPlugin(...)]) failed the Workflow with a retryable error, and a retry policy started a second run. MCPToolNameConflictError is now a non-retryable ApplicationError (and still a ValueError), so the failure-converter and failure-type entries are gone, and so is the README sentence 2819ae6 added about the client. test_mcp_conflict_after_a_tool_call runs with the plugin on the Worker and on the client, with maximum_attempts=2, and checks that one run fails.
• Two upgrade notes: since load_tools() returns no tools, an agent built after another agent listed a shared client has an empty tool_names until its first model call; on main it had the last listing. And since d3f653a, a turn's listings are applied together once every due server has listed, where main added each server's tools as soon as it listed, so a Signal handler that runs while a later server lists, or code that catches a failed later listing, sees none of that turn's tools. Workflow code that reads tool_names at those points can schedule other commands, and main histories that did so fail to replay, though no names clashed. The README and the PR body now name these cases. The designs stay.

The _run_loop change from 2819ae6 is unchanged; 9c219ce replaces its test with one whose hook runs an Activity, plus a control that reads the stream to the end.