Skip to content

fix(runtime): raise on same-kind bundles claiming one class name - #117

Merged
sagi5060 merged 4 commits into
devfrom
fix/82-same-kind-duplicate-class-names
Aug 6, 2026
Merged

fix(runtime): raise on same-kind bundles claiming one class name#117
sagi5060 merged 4 commits into
devfrom
fix/82-same-kind-duplicate-class-names

Conversation

@sagi5060

@sagi5060 sagi5060 commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

What

PluginRegistry._scan (agentdeck/runtime/registry.py) collected discovered classes into
found[attr.__name__] = attr, so two bundles of the same kind exporting the same class
name deduped silently, in sorted bundle order. Copying agents/greeter/ to
agents/greeter-v2/ to iterate and forgetting to rename the class made the original
greeter bundle vanish from the registry with no error — every request routed to that name
silently ran the other agent. Fixed at the point of discovery: _scan now tracks which
bundle claimed each class name and raises ConfigError naming both bundle paths and the
shared class name the moment a second, different class claims a name already taken.
AgentRegistry, WorkflowRegistry, App.load(), and InvocableRegistry (which discovers
through the same v1 scan) all inherit the fix for free.

Sibling nit from the same review (#82's Notes): a bundle whose module raises while
importing (SyntaxError, a missing dependency) used to surface as a raw traceback through
the import machinery, 12–14 frames deep. The import call in _scan is now wrapped, so it
raises ConfigError naming the offending bundle path with the original exception chained
as the cause.

Refs #82 — Done-when item 2 ("import/build failures wrapped") is only half done: import is
wrapped, build()/build_graph() is not (see judgment ledger and #119 below). Not closing
#82 until that's tracked to completion or explicitly descoped there.

Judgment ledger

  • Raise, not warn. The issue asks to decide deliberately between a hard ConfigError and
    a loud warning that keeps both under distinguished names. A silently missing agent (today's
    bug) and a silently renamed agent (auto-disambiguating a warn-and-rename scheme) are the
    same failure mode from a caller's perspective — a request that used to reach one agent now
    silently reaches a different one, or an invented name nobody asked for. Raising is the only
    option where a name-routing mistake can't hide. I looked for a legitimate layout the loud
    version breaks and did not find one: v1 already keeps agents and workflows in disjoint
    registries and disjoint type_dirs, so two unrelated bundles that happen to pick the same
    class name for different concepts is exactly the case this issue exists to catch, not a
    case to accommodate.
  • Collision check is identity-based, not name-based (fixed in review round 2 — see
    below): vars(module) yields one entry per binding, not per class, so a bundle that
    aliases its own class under a second name (GreeterAgent = Greeter, kept after a rename)
    must not trip the guard against itself. The check is claimant is not None and claimant is not attr, not attr.__name__ in found.
  • This changes v1's frozen discovery error surface, as flagged in the issue. It is
    additive on the success path (a project with no name collisions, and no cross-bundle
    aliasing, behaves identically) and turns one specific silent-data-loss case into a
    load-time ConfigError. One existing test
    (tests/test_app.py::test_injected_session_factory_closed_when_load_fails) pinned the old
    raw RuntimeError a broken bundle's import used to raise; updated to expect the new
    ConfigError wrapping, since that behavior change is Notes item 2 of this same issue. No
    golden/SSE fixture exercises a same-kind collision, an alias, or an import failure, so
    nothing else moved.
  • A ConfigError a bundle itself raises at module scope (not just SyntaxError/missing
    dep) is now re-wrapped too
    — the import except Exception is broad by design, so a
    bundle author's own ConfigError raised during import gets caught and re-raised as a new
    ConfigError naming the bundle path, with the original chained as __cause__. Type and
    message are preserved (nothing swallowed); the outer message is a superset (bundle path +
    str(original)), so no caller matching on the message's start breaks — checked, nothing in
    the codebase does.
  • Scope of "import/build failures are wrapped": wrapped the importlib.import_module
    call inside _scan — the failure mode the issue's Notes describes concretely
    (SyntaxError, missing dep, a raw traceback through import machinery). Did not thread
    bundle-path context through BaseAgent.build() / BaseWorkflow.build_graph() — those are
    called from several sites (App.load(), InvocableRegistry.load(), run_agent, …) with no
    bundle-path association surviving past the scan (PluginRegistry.list() returns
    dict[str, type[T]], not a class-to-path mapping), and build() already has its own
    ConfigError paths for user-facing misconfiguration. Threading bundle paths through every
    build() call site is a larger, separately-scoped change — filed as discovery: build()/build_graph() failures still surface a bare exception with no bundle path #119 rather than
    done opportunistically here, and the PR now says Refs #82 instead of Closes #82 because
    of it.
  • Message wording: f"two bundles under '{self.type_dir}/' both define the {self.label} class {attr.__name__!r}: '...' and '...'; one name is one invocable — rename one of the classes." — matches the lowercase, semicolon-joined style of the existing cross-kind
    message in agentdeck/runtime/discovery.py's _add rather than inventing a new voice.
  • PluginRegistry's class docstring now states the collision/alias behavior — it
    previously documented only laziness and caching, so the new failure mode (and the
    non-failure alias case) wasn't discoverable from the class itself.
  • CHANGELOG: kept the two new Unreleased "Fixed" entries. Reverted the edits to the
    two already-released 2.0.0b4 entries (from PR feat(registry): discover invocables into an InvocableRegistry #81) that stated "two bundles of the same
    kind ... still collapse to a single invocable" — verified via gh release view v2.0.0b4
    that the published release notes still say this, so a b4 reader was told the truth as of
    b4 and that section is the published artifact, not a living description of current
    behavior (the file's own header: entries are "written to be attached to a release as-is").
    dev's CHANGELOG now matches the release verbatim for that section; only the new
    Unreleased entries describe today's behavior.

Test evidence

Tests in tests/test_errors.py and tests/test_app.py:

  • test_two_same_kind_bundles_sharing_a_class_name_raise_naming_both — two agent bundles
    (greeter, greeter-v2) both defining class Greeter; asserts the exact quoted bundle
    paths ('agents/greeter' and 'agents/greeter-v2', not bare substrings — one is a
    substring of the other, which is exactly how the previous version of this test passed
    even when the message named only one bundle) both appear in the ConfigError message.
  • test_one_bundle_aliasing_its_own_class_is_not_a_collision (new, review round 2) — one
    bundle with GreeterAgent = Greeter; asserts App().load() succeeds with exactly one
    invocable. Pins the false-positive fix.
  • test_bundle_import_failure_is_wrapped_with_its_path — a bundle that raises RuntimeError
    at import now raises ConfigError naming its path, with the original exception chained.
  • test_injected_session_factory_closed_when_load_fails (existing, updated) — same import
    repro, now asserting the wrapped ConfigError instead of the raw RuntimeError.

Verified all of these have teeth by stashing the registry.py fix, rerunning — the
collision, alias, and import-wrapping tests all failed against the pre-fix code — then
restoring the fix and rerunning green.

make check equivalent (fresh python3.12 -m venv + pip install -e ".[dev,serve,durability]",
since the shared repo .venv's editable install resolves to the main worktree, not this
one): ruff check — pass; ruff format --check — pass; ty check agentdeck — pass;
lint-imports — 12/12 contracts kept; pytest tests/ — 688 passed, 96 skipped (expected:
optional Redis/Postgres backends without services running), 0 failed. Re-ran after merging
origin/dev in (brought in #112's control-lifecycle events, unrelated to this PR;
CHANGELOG.md conflict resolved as a union) — still fully green.

Review round 2

Addressed request-changes review:

  1. (blocking, fixed) Collision check was name-based, so a single bundle aliasing its own
    class (GreeterAgent = Greeter) tripped the guard against itself with a
    self-contradicting message naming the same bundle path twice. Now identity-based
    (claimant is not attr). Added the false-positive regression test.
  2. (blocking, fixed) The collision test's assertions were bare substrings
    ("agents/greeter" in message), satisfied even if the message named only
    agents/greeter-v2. Now asserts the exact quoted forms, which are not substrings of each
    other.
  3. (blocking, fixed) Restored the two already-released 2.0.0b4 CHANGELOG entries
    verbatim — dev and the published v2.0.0b4 release notes now agree again. Only the new
    Unreleased entries describe current behavior.
  4. (advisory, acted on) Filed discovery: build()/build_graph() failures still surface a bare exception with no bundle path #119 for the build()/build_graph() half of "import/build
    failures are wrapped", and changed Closes #82 to Refs #82 since that Done-when item is
    only half satisfied by this PR.

Also took the wording/docstring advisories and added the re-wrapped-ConfigError ledger
line above.

🤖 Generated with Claude Code

sagi5060 and others added 2 commits August 6, 2026 12:17
PluginRegistry._scan collected discovered classes into a plain dict keyed by
class name, so two bundles of the same kind (two agents, or two workflows)
exporting a class of the same name silently collapsed into one entry, in
sorted bundle order — the classic "copied a bundle, forgot to rename the
class" mistake vanished the original with no error. It now raises ConfigError
naming both bundle paths and the shared class name. A bundle that raises
while importing (SyntaxError, missing dependency) is also wrapped in a
ConfigError naming its path instead of surfacing a raw traceback.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sagi5060 and others added 2 commits August 6, 2026 12:48
…4 changelog

Review round 2 on #82/PR #117:

- The same-name guard was keyed on class name, so a bundle that binds its own
  class under a second name (an alias kept after a rename, e.g.
  `GreeterAgent = Greeter`) tripped the guard against itself with a
  self-contradicting message naming the same bundle path twice. `vars(module)`
  yields one entry per binding, not per class. Now keyed on identity
  (`claimant is not attr`), with a regression test pinning that one bundle
  aliasing its own class still loads as a single invocable.
- The collision test's assertions were bare substrings
  ("agents/greeter" in message), which "agents/greeter-v2" alone satisfies —
  the fix's core guarantee (naming *both* bundles) was unpinned. Now asserts
  the exact quoted forms, which are not substrings of each other.
- Restored the two already-released 2.0.0b4 CHANGELOG entries verbatim — the
  published release notes still carry the caveat this PR removes, so `dev`
  must not silently diverge from what was shipped. Only the new Unreleased
  entries describe current behavior.
- Reworded the collision message to match the existing cross-kind message's
  style in discovery.py, and documented the collision/alias behavior on
  PluginRegistry's class docstring.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CHANGELOG.md conflict resolved as a union, kept in Added/Fixed order per the
file's own header.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sagi5060

sagi5060 commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Review round 2 addressed — pushed to this branch, ready for re-review.

  1. (blocking, fixed) Collision check was name-based (attr.__name__ in found), so a
    bundle aliasing its own class under a second name (GreeterAgent = Greeter) tripped the
    guard against itself with the self-contradicting message you found
    ('agents/greeter' and 'agents/greeter'). Reproduced it, then switched to identity
    (claimant is not attr) and added test_one_bundle_aliasing_its_own_class_is_not_a_collision,
    which fails on the pre-fix code with exactly that message.
  2. (blocking, fixed) test_two_same_kind_bundles_sharing_a_class_name_raise_naming_both
    now asserts the exact quoted forms ("'agents/greeter'", "'agents/greeter-v2'") instead
    of bare substrings. Verified by mutating the message to drop bundle_of[...] (naming only
    the second bundle) — the old assertions still passed, the new ones fail.
  3. (blocking, fixed) Restored both 2.0.0b4 CHANGELOG entries verbatim, matching
    gh release view v2.0.0b4. Only the new Unreleased entries describe current behavior; dev
    no longer disagrees with the published release notes.
  4. (advisory, acted on) Filed discovery: build()/build_graph() failures still surface a bare exception with no bundle path #119 for the build()/build_graph() half of "import/build
    wrapped with the bundle path" (deliberately out of scope here — no bundle-path association
    survives past PluginRegistry.list(), and it touches several call sites, not one choke
    point). Changed Closes #82 to Refs #82 accordingly.

Also took the wording (agentdeck/runtime/discovery.py-style message), docstring, and
ConfigError-re-wrap ledger advisories.

make check equivalent green after merging origin/dev (brought in #112, unrelated):
ruff, ruff-format, ty, lint-imports (12/12 kept), pytest tests/ — 688 passed, 96 skipped, 0
failed.

@sagi5060
sagi5060 merged commit ced2f40 into dev Aug 6, 2026
1 check passed
sagi5060 added a commit that referenced this pull request Aug 6, 2026
…gress-reports

One real conflict. #114 restructured the langgraph adapter: `start` and `resume` now both
delegate to `_drive`, which is where the `RunnableConfig` is built and which v1's bridge
wraps. So the reporter injection moves there — one line instead of two, and v1's workflow
runs inherit it rather than being the one path whose nodes cannot report.

Also records what the two node-level channels are for, now that they sit next to each
other: #114's `get_stream_writer()` write is whatever a node chose, so it travels as a
namespaced `custom`; status and progress are canonical kinds, which is D10's promotion
rather than another `custom`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sagi5060 added a commit that referenced this pull request Aug 10, 2026
…failures

A bundle defining only an AgentDeclaration/WorkflowDeclaration subclass and
never instantiating it now fails Deck.from_project() naming the bundle file
and what to add, instead of silently contributing nothing (#174) — the
natural shape of a v1 bundle ported to v3, which scans for instances rather
than subclasses. A discovered agent/workflow whose compile_agent()/
build_graph() raises now names its bundle path in a chained ConfigError,
the same way #82/#117 already did for import failures (#119).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sagi5060 added a commit that referenced this pull request Aug 10, 2026
…failures (#186)

A bundle defining only an AgentDeclaration/WorkflowDeclaration subclass and
never instantiating it now fails Deck.from_project() naming the bundle file
and what to add, instead of silently contributing nothing (#174) — the
natural shape of a v1 bundle ported to v3, which scans for instances rather
than subclasses. A discovered agent/workflow whose compile_agent()/
build_graph() raises now names its bundle path in a chained ConfigError,
the same way #82/#117 already did for import failures (#119).

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
@sagi5060
sagi5060 deleted the fix/82-same-kind-duplicate-class-names branch August 10, 2026 07:27
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.

1 participant