Skip to content

Breaking: refuse repeated Deep Agents activity names instead of dropping them - #175

Open
campoy wants to merge 3 commits into
mainfrom
fix/deep-agents-python-activity-dedupe-silent-drop
Open

campoy wants to merge 3 commits into
mainfrom
fix/deep-agents-python-activity-dedupe-silent-drop

Conversation

@campoy

@campoy campoy commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

What was changed

DeepAgentsPlugin.configure_worker no longer drops every repeated Activity name from the Worker's Activity list. Repeated names now fail at Worker start, as they do without the plugin, except in the one case the module docstring describes: one instance on the Client and one on the Worker.

  • In that case only (exactly one DeepAgentsPlugin on the Client and one on the Worker, the same object or two), the Worker instance skips an Activity it added when an entry from before its call is still registered after it, under the same Activity name (for a dynamic Activity, the Worker's one dynamic slot), and is:
    • the same callable, or a callable that wraps it (__wrapped__);
    • for a second object, the Client instance's copy of the same built-in Activity, or a wrapper of it.
  • "Same" is identity: the same object, or two bound methods of one function on one object. The check never calls __eq__, so two distinct callables that compare equal reach the Worker's checks.
  • Skipping such an entry never changes which code runs under that name. The Worker keeps what the Client side registered, including wrappers a later Client plugin put around it, and keeps any other Activity the Worker instance adds. A registration that was replaced or renamed (for example to alias.deepagents.invoke_tool) never removes the Worker instance's own Activities.
  • The check reads only the Activity lists before and after the call and keeps no state, so a Client plugin that hands on a copy of the config, or Workers built from several threads at once, behave the same.
  • The four built-in Activities are kept in a tuple apart from the public activities setting, which callers may change. _dedupe_activities is removed.
  • Every other repeated name reaches the SDK's ValueError: More than one activity named ..., and a second, different dynamic Activity reaches its TypeError: More than one dynamic activity.
  • README and module docstring: the four Activity names and this rule.

Breaking change: Workers that start today with a repeated name now fail at construction: two DeepAgentsPlugin instances on one side, two caller Activities with one name, or a caller Activity named like a plugin Activity (deepagents.invoke_model, deepagents.invoke_model_streaming, deepagents.invoke_tool, deepagents.backend_op). Remove or rename the extra registration. The title starts with Breaking: because release notes come from commit subjects.

Rollback: no version gate. The change runs at Worker construction, not in Workflow code. Activity names, commands, payloads and History don't change, so recorded histories replay with this change and without it. A rolled-back Worker starts again with the silent drop.

The branch keeps the first version and two review revisions as separate commits; review the combined diff against main.

Why?

See #174. With two plugin instances on one Client, the Worker silently drops the second instance's Activities, so every model call goes to the first instance's model_provider, with its endpoint and key. A caller Activity can be dropped or replaced the same way. Nothing warns.

Checklist

  1. Closes [Bug] deepagents: Worker silently drops Activities that share a name #174

  2. How was this tested:

    • New tests/test_activity_registration.py, 25 cases, building Workers from a dev-server Client:
      • test_repeated_activity_names_are_refused, 7 setups: two instances on the Client, two on the Worker, one on the Client and two on the Worker, two caller Activities named fetch, a caller Activity named deepagents.invoke_tool, an Activity named fetch appended to the Worker instance's activities beside a caller fetch, and two compare-equal callables with one name. Each must raise ValueError: More than one activity named.
      • test_two_dynamic_activities_are_refused, 4 setups: two different dynamic Activities, as two functions and as two compare-equal callables, with one object and with two. Each must raise TypeError: More than one dynamic activity.
      • test_one_instance_on_each_side_keeps_the_client_registration, 14 setups: the same object or two; a Client wrapper plugin; an extra Activity on the Worker instance (list and callable setting); a dynamic Activity on one object on both sides (list and callable); a Client plugin that copies the config; a caller copy of the object's own invoke_model; a Client plugin that replaces the plugin Activities with alias.deepagents.* wrappers; a callable on the Worker instance that replaces the Client instance's built-ins with its own. The Worker must hold each plugin name and fetch once, with the expected callables.
    • Against main, the 7 named refusals fail with DID NOT RAISE ValueError and the other 18 pass. With the patch, all 25 pass. The keep setups guard behavior main has and earlier revisions of this branch broke.
    • Locally, pytest: 118 passed. ruff check --select I, ruff format --check, mypy, basedpyright, pyright and pydocstyle pass. These ran in Docker with uv 0.13.1, outside the plugin's required-version, so not through make. CI covers make lint and make test.
    • [Bug] deepagents: Worker silently drops Activities that share a name #174's reproduction prints defect reproduced on 0.0.3 and main. With the patch, it stops at the two-plugins Worker with ValueError: More than one activity named deepagents.invoke_model.
    • Real Workers with a local dev server (Temporal CLI 1.9.1, Server 1.32.0): 13 one-on-each-side setups like those above each run a Deep Agent to the end with the patch and on main, with the Client instance's provider answering except where the Worker instance replaces it. With a dynamic Activity, a Workflow calling an unregistered name gets its answer. Two threads that build Workers from one plugin at the same time both start.
  3. Any docs updates needed?
    The README paragraph and module docstring, included here.

Not covered:

  • A wrapper is recognized only through __wrapped__. With one object on both sides, a Client plugin that replaces the Activities with callables that don't set it makes the Worker fail at start with the SDK's ValueError; it does not start with a wrong registration. Not run.
  • temporalio 1.34.x still bundles the old code as temporalio.contrib.deepagents. No backport here.

Open questions:

  • With one object on both sides, should a caller's extra copy of that object's own built-in Activity be refused? This PR lets it start, as main does, because both entries are one function.
  • The SDK's Plugin API gives configure_worker no way to know which call of a plugin on both sides is which; this PR avoids needing it. Would an SDK hook for this be worth adding, so plugins can be stricter?
  • Should temporalio 1.34.x get a backport?

Draft while these questions are open.

@campoy
campoy marked this pull request as ready for review October 28, 2026 23:58
@campoy
campoy requested a review from a team as a code owner October 28, 2026 23:58
…ing them

DeepAgentsPlugin.configure_worker kept the first activity of each name in
the Worker's whole activity list. With two plugin instances on one side, the
Worker silently dropped the second instance's activities, so every model
call used the first instance's model_provider, with its endpoint and key.
A caller activity whose name repeated another activity's name was dropped
or replaced the same way.

The plugin now skips its own activities only in the case its docs describe:
one instance on the Client and one on the Worker. The Worker keeps the
Client instance's activities, as before. Every other repeated name reaches
the Worker's own check.

Breaking change: a Worker with two DeepAgentsPlugin instances on the same
side, or with a caller activity whose name repeats another activity's name
(including deepagents.invoke_model, deepagents.invoke_model_streaming,
deepagents.invoke_tool and deepagents.backend_op), now fails at
construction with "ValueError: More than one activity named ...". Before,
it started and used the first registration. Remove or rename the extra
registration. Workflow code and histories do not change.
Review of the first version found setups that start on main and broke on
it. With one DeepAgentsPlugin on the Client and one on the Worker, the
Worker instance now skips only an activity that is already registered:
the same callable, a callable that wraps it (__wrapped__), or, for a second
instance, the Client instance's copy of the same built-in activity. It no
longer restores the whole earlier list, so other activities given to the
Worker instance stay, and a Client plugin that wraps the activities keeps
working. The check keeps no state between calls, so a Client plugin that
copies the config and Workers built from several threads behave the same.
The built-in activities are kept apart from the public activities setting.

Breaking change, unchanged from the previous commit: two DeepAgentsPlugin
instances on one side, or a caller activity whose name repeats another
activity's name with a different function, now fail at Worker
construction with "ValueError: More than one activity named ...".
Review revisions after 253dcce: the Worker instance skips a repeat only
while a registration with the same activity name, or the dynamic slot,
stays in the list, and only when that registration is the same callable,
wraps it, or is the Client instance's copy of the same built-in activity.

Registrations are now compared by identity (two bound methods of one
function on one object count as one callable), never with __eq__. Two
distinct callables that compare equal, such as callable dataclass
instances, are different registrations, so they reach the Worker's
duplicate-name or duplicate-dynamic check instead of one being dropped.

Breaking change, unchanged: two DeepAgentsPlugin instances on one side, or
a repeated activity name or second dynamic activity with a different
callable, fail at Worker construction.
@campoy
campoy force-pushed the fix/deep-agents-python-activity-dedupe-silent-drop branch from 6abb9e0 to 01b5e42 Compare October 29, 2026 00:01
@campoy
campoy marked this pull request as draft October 29, 2026 16:33
@campoy
campoy marked this pull request as ready for review October 30, 2026 07:55

@DABH DABH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review, at head 01b5e42 (three commits over c4fb1ae). On a worktree at the head, make lint is clean and make test passes 118 of 118. Each finding below was checked with temporary tests against a local dev server, since removed: a real Worker with one DeepAgentsPlugin on the Client and a differently configured one on the Worker; the error text for two instances on one side; a Replayer with two instances; and a one-shot activities iterable, compared with main's SimplePlugin path. I also prototyped the fix proposed for the first finding on the head. Checked and fine: the same instance on both sides (list, tuple and callable activities). Two instances on one side are refused, and so is a caller Activity named deepagents.invoke_tool. The before/after comparison covers both forms of SimplePlugin's activities parameter (a list to append, or a callable). Replayer construction is unaffected: configure_replayer never touches Activities, so two instances on a Replayer construct, as on main. I left nothing material unverified.

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

Where the problems cluster: the refusals are correct, and the 25 new cases cover them well. The open question is the exemption. It accepts any two instances, one on each side, not only the same plugin twice. A Worker-side instance's model_provider and backends are therefore still replaced, without an error, by the Client instance's. That is #174's impact in a different placement, and the rule is looser than the one #161, #162 and #202 apply on the TypeScript side. The other two items are the error message for two instances and a regression for one-shot iterables.

Commenting rather than requesting changes: the remaining P2 is behaviour main already has, not a regression from this PR, but I think it should be settled before this closes #174.

PR body: it says 'Draft while these questions are open', but the PR is not marked draft. 'Closes #174' holds for the issue's two reproductions (two instances on one Client, two caller fetch Activities). The issue's Impact paragraph (model calls reaching another provider's endpoint with its key) still applies with one instance on each side; see the first inline comment. On open question 1 (a caller's extra copy of the shared instance's own built-in Activity): it registers the same function under the same name, so accepting it cannot change which code runs. I would leave it as is.

on_worker = [
p for p in config.get("plugins") or [] if isinstance(p, DeepAgentsPlugin)
]
if len(on_client) == 1 and len(on_worker) == 1 and on_worker[0] is self:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: the exemption accepts any two instances, one on each side, so a Worker-side DeepAgentsPlugin configured differently from the Client's has its model_provider and backends replaced without an error; that is #174's impact in another placement. Probe on the dev server:

  • Client: plugins=[DeepAgentsPlugin(model_provider=mock_model_provider(['answer from CLIENT provider']), model_activity_options={'start_to_close_timeout': timedelta(seconds=11)})].
  • Worker: plugins=[DeepAgentsPlugin(model_provider=mock_model_provider(['answer from WORKER provider']), model_activity_options={'start_to_close_timeout': timedelta(seconds=22)}, backends={'fs': ...})].

The Worker starts. Every registered plugin Activity is bound to the Client instance's DeepAgentActivities, and the run answers answer from CLIENT provider. A TemporalBackend('fs') call would fail with the non-retryable Backend factory 'fs' is not configured on this worker. Configure DeepAgentsPlugin(backends=...) (_activity.py:398-406), even though the Worker plugin does configure it.

Meanwhile _serde.get_settings().model_activity_options holds the Worker instance's 22 s. model_activity_options, streaming_topic and tool_activity_options are process globals set by whichever instance was constructed last (_serde.py:69-76, _tools.py:77-84), so the Worker runs a mix of the two configurations. The only signal is the SDK's generic warning ('The same plugin type ... may run twice'), which describes neither effect.

The setup is plausible. The README's 'Composing with other plugins' connect() helper puts a default DeepAgentsPlugin() on the Client, and the class docstring says API keys live in the Worker-side model_provider. #161, #162 and #202 accept a repeated plugin only when both instances were built from the same provider and options.

Fix, prototyped on this head:

  1. Record the constructor's model_provider, backends and streaming_batch_interval.
  2. Return the Client instance here only when it is self or matches on those: model_provider by identity, backends as a dict of factories, the interval by equality. Optionally compare the dispatch settings too, since they are globals.

With that change, two default instances and every setup in test_one_instance_on_each_side_keeps_the_client_registration still start (all 25 registration tests pass). The probe above then fails with ValueError: More than one activity named deepagents.invoke_model.

Also add a case with a different model_provider to test_repeated_activity_names_are_refused. In the README, say in user terms which setups start (the same instance on both sides, or two instances built from the same provider and backends) and that any other second instance fails. If you keep the current rule, the README needs to say that the Worker instance's model_provider and backends are ignored in this case.

]
if len(on_client) == 1 and len(on_worker) == 1 and on_worker[0] is self:
return on_client[0]
return None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: two instances on one side fail with the SDK's ValueError('More than one activity named deepagents.invoke_model'), which names neither DeepAgentsPlugin nor the fix. Probe: [DeepAgentsPlugin(), DeepAgentsPlugin()] on the Client, or on the Worker, gives exactly that text. A user who added a second instance to reach a second provider (#174's setup) sees only an Activity name they never wrote. The lists built at 268-271 already tell this method how many instances there are, so it can raise first. For example: ValueError('DeepAgentsPlugin is registered 2 times for this Worker (2 on the Client, 0 on the Worker). Register one instance: its model_provider receives each model name, so one provider can route to several, and backends= takes every factory.'). Caller-Activity collisions can stay with the SDK's check. The TypeScript siblings (#161, #162, #166, #202) raise a plugin-specific message that says what to change. Non-blocking.

def configure_worker(self, config: Any) -> Any:
"""Deduplicate activity registrations after the base configuration."""
"""Skip activities the Client's instance already registered."""
before = list(config.get("activities") or [])

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: this line consumes a one-shot iterable before SimplePlugin.configure_worker reads the same object, so caller Activities passed as a generator are dropped without an error whenever this plugin configures the Worker first. Probe: Worker(client_with_plugin, workflows=[...], activities=(a for a in [fetch])) registers only the four deepagents.* names at this head. Main's path (SimplePlugin.configure_worker, then the name dedupe) also registers fetch, and the SDK on its own iterates activities only once. This happens with or without a second instance. The parameter is typed Sequence[Callable], so this input is outside the contract, but the failure has #174's shape: an Activity goes missing with no error until a Workflow schedules it. Fix: after taking the copy, write a separate list back when the key is set (if config.get('activities') is not None: config['activities'] = list(before)). It must be a separate list, so that a callable activities setting that appends in place cannot change before. Non-blocking.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] deepagents: Worker silently drops Activities that share a name

2 participants