From 6553d82cc3f258be52c9cb9ed533fe416eacc3b4 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 15 Aug 2026 19:17:38 -0700 Subject: [PATCH 1/4] fix(adapters): seed the bundled backends inside register_multiplexer (#565) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_BACKENDS` is an ordered list and every consumer takes the first entry under a name (`_factory_by_name` and all three `_select` loops), so whichever registration lands first owns that name. `_load_builtin_backends` was only reached from `_select` / `get_multiplexer` / `detect_multiplexers`, which made "bundled backends register first" a coincidence of which import ran first rather than an invariant. The trigger is reachable, not theoretical: a plugin's `[python]` module is exec'd in-process by `plugins/registry.py` (`spec_from_file_location` + `exec_module`), which has no ordering relationship to the first mux resolution. A module-level `register_multiplexer("tmux", ...)` there landed ahead of the bundled tmux entry and was selected in its place — the whole process silently driving a third-party transport. `register_multiplexer` now calls `_load_builtin_backends()` first, mirroring the already-correct adapter twin (`register_adapter` in `adapters/registry.py`). `_BACKENDS` stays an append-only list: a shadowing external is still registered, just behind the bundled entry, so first-match consumers pick the builtin. No dedup was added — the ordered list is load-bearing for selection precedence. `_BUILTINS_LOADED = True` moves from after the registrations to between the two imports and the registrations. Both halves of that position are load-bearing: below the imports so a transient import failure leaves the seeding retryable (the property the old trailing comment claimed), above the registrations because they now re-enter the function through `register_multiplexer` and a flag set afterwards recurses without end. The adapter twin sets its flag at the very top only because its builtins are lazy thunks with nothing to import first. The existing `tests/test_external_backends.py::test_externals_load_after_builtins` cannot catch this. It arms the entry-point scan and calls `_select`, which runs `_load_builtin_backends()` before `_load_external_backends()` — so the builtins are already in `_BACKENDS` before the external's registration runs. It passes for a reason unrelated to `register_multiplexer` seeding anything. It is not wrong (it pins a real, still-true fact about the scan path), so it is kept unchanged; it is simply insufficient, which is why the new test exists. Ablations run singly, restoring from a `cp` backup between each (md5-verified), never via `git checkout`: - T1 — removed only the `_load_builtin_backends()` call from `register_multiplexer`: `test_builtins_win_over_an_external_registered_before_ any_resolution` FAILED on `assert isinstance(get_multiplexer(), TmuxMultiplexer)` -> `assert False ... isinstance(, TmuxMultiplexer)` (the sentinel external was selected). Under that same ablation `test_externals_load_after_builtins` PASSED (1 passed) — the asymmetry is the finding. `test_builtin_seeding_is_reentrant...` also reddened here, since it depends on the same seeding call. - T2 — moved `_BUILTINS_LOADED = True` back below the two `register_multiplexer(...)` calls: `test_builtin_seeding_is_reentrant_and_registers_each_builtin_once` FAILED with `RecursionError: maximum recursion depth exceeded` at multiplexer.py:453, pytest reporting "Recursion detected (same locals & position)". - T3 — moved `_BUILTINS_LOADED = True` to the very top, above the two imports: `test_a_failed_builtin_import_leaves_the_seeding_retryable` FAILED on `assert fresh_registry._BUILTINS_LOADED is False` -> `assert True is False`. The `pytest.raises(ImportError)` and `_BACKENDS == []` assertions still held, so the ablation isolates exactly the retryability property. T1's name-shape assertions (`names[0] == "tmux"`, `count == 2`) do not discriminate on their own — the shadowing external registers under "tmux" too, so both hold under the T1 ablation. They are kept as a shape pin against a future dedup, and the discriminating half is asserted explicitly: the first "tmux" entry is the bundled `TmuxMultiplexer` factory. Verified: full suite 5681 passed / 50 skipped (`-n logical`), `uv run pyright` 0 errors, `trunk fmt` and `trunk check` clean. --- src/bmad_loop/adapters/multiplexer.py | 50 +++++++++++--- tests/test_backend_registry.py | 94 +++++++++++++++++++++++++++ 2 files changed, 134 insertions(+), 10 deletions(-) diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index 0d994b9f..eb8d4fba 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -12,10 +12,13 @@ ``TerminalMultiplexer`` is the contract a backend author implements. Operation names mirror today's call sites verbatim so the migration is mechanical. Backends register themselves through :func:`register_multiplexer` (bundled ones from -:func:`_load_builtin_backends`; out-of-tree ones at import time, triggered by the -``bmad_loop.mux_backends`` entry-point scan in :func:`_load_external_backends` — -so a pip/uv co-installed adapter package is selectable with no config step); the -process-wide backend is selected by registry and returned by :func:`get_multiplexer`. +:func:`_load_builtin_backends`, which :func:`register_multiplexer` seeds first so +a bundled name keeps first-wins no matter who registers earliest; out-of-tree +ones at import time — usually the ``bmad_loop.mux_backends`` entry-point scan in +:func:`_load_external_backends`, so a pip/uv co-installed adapter package is +selectable with no config step, but *any* import reaches it, a plugin's +``[python]`` module included); the process-wide backend is selected by registry +and returned by :func:`get_multiplexer`. Selection precedence (issue #87): the ``BMAD_LOOP_MUX_BACKEND`` env var, then the policy ``[mux] backend`` choice (installed once per CLI invocation via @@ -430,8 +433,24 @@ def register_multiplexer( ) -> None: """Register a transport backend. ``matches(sys.platform)`` decides automatic selection; ``name`` is the key for the ``BMAD_LOOP_MUX_BACKEND`` override. - Bundled backends register from :func:`_load_builtin_backends`; an out-of-tree - backend calls this at import time — no core edit required.""" + Bundled backends register from :func:`_load_builtin_backends`, seeded here + rather than only by the resolution entry points, so an out-of-tree package can + never shadow a bundled name. An out-of-tree backend calls this at import time + — no core edit required. + + Seeding on *this* side is what makes first-wins an invariant instead of an + ordering coincidence. ``_BACKENDS`` is an ordered list and every consumer + takes the first entry under a name (:func:`_factory_by_name` and all three + :func:`_select` loops), so whichever registration lands first owns the name. + An external module runs its ``register_multiplexer`` calls as an import side + effect, and that import is not always triggered by a mux resolution: a + plugin's ``[python]`` module is exec'd in-process by ``plugins/registry.py``, + which has no ordering relationship to the first :func:`get_multiplexer` call. + Arriving first, it would land ahead of the bundled tmux entry and be selected + in its place. Seeding keeps the bundled entry first; the external stays behind + it, since this list appends rather than dedups — which is exactly what a + shadowed name should look like.""" + _load_builtin_backends() _BACKENDS.append((name, matches, factory)) get_multiplexer.cache_clear() # a later registration must not be shadowed by a cached pick @@ -440,17 +459,29 @@ def _load_builtin_backends() -> None: """Register the bundled backends — tmux (POSIX) and psmux (native Windows); every other backend is out-of-tree and arrives via :func:`_load_external_backends` or a manual import. Idempotent and lazy - (called from :func:`get_multiplexer`, not at - module import) to stay cycle-safe. Registers inline rather than via + (called from :func:`get_multiplexer` and from :func:`register_multiplexer`, + not at module import) to stay cycle-safe. Registers inline rather than via tmux_backend's import side effect so the registry can be cleared and re-loaded deterministically (a re-import is a no-op once cached) — - mirroring ``process_host._load_builtin_hosts``.""" + mirroring ``process_host._load_builtin_hosts``. + + The flag sits between the imports and the registrations, and both halves of + that position are load-bearing: below the imports so a transient import + failure leaves the seeding retryable, above the registrations because they + re-enter this function through :func:`register_multiplexer`. The adapter twin + sets it at the very top only because its builtins are lazy thunks with + nothing to import first.""" global _BUILTINS_LOADED if _BUILTINS_LOADED: return from .psmux_backend import PsmuxMultiplexer from .tmux_backend import TmuxMultiplexer + # Set after the imports but BEFORE the registrations. Below the imports so a + # transient import failure still retries; above the registrations because they + # re-enter this function through register_multiplexer, and a flag set + # afterwards would recurse without end. + _BUILTINS_LOADED = True # tmux is the default everywhere except native Windows (no tmux binary there); # get_multiplexer still falls back to tmux when no backend matches. Builtins # register before externals, so tmux keeps first-wins on any name collision. @@ -458,7 +489,6 @@ def _load_builtin_backends() -> None: # psmux speaks the tmux CLI through its own distinctly-named binary, so # native Windows gets the tmux-family backend with a PowerShell dialect. register_multiplexer("psmux", lambda platform: platform == "win32", PsmuxMultiplexer) - _BUILTINS_LOADED = True # set only after a successful import so a transient failure retries # The entry-point group an out-of-tree backend package advertises its module diff --git a/tests/test_backend_registry.py b/tests/test_backend_registry.py index 28b92127..e1c17681 100644 --- a/tests/test_backend_registry.py +++ b/tests/test_backend_registry.py @@ -505,3 +505,97 @@ def test_win32_bottoms_out_at_psmux_with_no_externals(fresh_registry, monkeypatc assert (name, reason) == ("psmux", "fallback") # psmux is both the win32 platform default and the sole bundled win32 match assert fresh_registry._PLATFORM_DEFAULTS.get("win32") == "psmux" + + +# Builtin seeding inside `register_multiplexer` (#565). First-wins only protects a +# bundled name if the bundled entry is guaranteed to be registered first; these pin +# that guarantee and the flag position that makes the seeding safe to re-enter. + + +def test_builtins_win_over_an_external_registered_before_any_resolution( + fresh_registry, monkeypatch +): + """The shadowing hole that first-wins ALONE does not close. + + An out-of-tree backend registers as an import side effect, and that import is + not always triggered by a mux resolution: a plugin's ``[python]`` module is + exec'd in-process by ``plugins/registry.py``, with no ordering relationship to + the first ``get_multiplexer()`` call. Arriving first under a bundled name, it + would sit ahead of the bundled tmux entry in the ordered ``_BACKENDS`` list and + every first-match consumer would pick it — the whole process silently driving a + third-party transport. Only ``register_multiplexer`` seeding the builtins on + its own side makes first-wins an invariant instead of an ordering coincidence. + + ABLATION: drop the ``_load_builtin_backends()`` call from + ``register_multiplexer`` and this reddens — while + ``tests/test_external_backends.py``'s ``test_externals_load_after_builtins`` + stays green, because ``_select`` seeds the builtins on that path before the + scan's registration ever runs. That asymmetry is the finding: the existing test + pins the scan path and cannot see this hole at all.""" + sentinel = object() + # The plugin trigger: a direct registration under a bundled name, with no + # entry-point scan involved (the fixture parks `_EXTERNALS_LOADED = True`). + fresh_registry.register_multiplexer("tmux", lambda p: True, lambda: sentinel) + + # (a) the bundled backend still wins the name. Forcing by name bypasses both + # the platform predicate and available(), so this is deterministic on the + # Linux and Windows CI legs alike. + monkeypatch.setenv("BMAD_LOOP_MUX_BACKEND", "tmux") + fresh_registry.get_multiplexer.cache_clear() + assert isinstance(fresh_registry.get_multiplexer(), TmuxMultiplexer) + + # (b) the ordering that makes (a) true: the bundled entry is first and the + # external is still present behind it — an append-only list, not a dedup'ing + # dict, so a shadowed name legitimately appears twice. + names = [name for name, _, _ in fresh_registry._BACKENDS] + assert names[0] == "tmux" and names.count("tmux") == 2 + # Names alone do not discriminate: the shadowing external registers under + # "tmux" too, so both assertions above still hold under the ABLATION. The + # first entry being the *bundled* factory is the half that carries the + # guarantee, so pin it explicitly rather than inferring it from the name. + first_tmux = next(factory for name, _, factory in fresh_registry._BACKENDS if name == "tmux") + assert first_tmux is TmuxMultiplexer + + +def test_builtin_seeding_is_reentrant_and_registers_each_builtin_once(fresh_registry): + """One ordinary registration seeds the builtins exactly once and terminates. + + ``_load_builtin_backends`` registers its two backends by calling + ``register_multiplexer``, which now calls ``_load_builtin_backends`` — so the + seeding re-enters itself, and only the flag's position stops it. + + ABLATION: move ``_BUILTINS_LOADED = True`` back below the two + ``register_multiplexer(...)`` calls and this reddens with a RecursionError.""" + # Without the flag move, the `register_multiplexer` calls inside + # `_load_builtin_backends` re-enter it with the flag still False, and it + # recurses without end. + fresh_registry.register_multiplexer("extra", lambda p: False, lambda: object()) + names = [name for name, _, _ in fresh_registry._BACKENDS] + # Seeded once, in bundled order, with the caller's own entry appended last. + assert names == ["tmux", "psmux", "extra"] + + +def test_a_failed_builtin_import_leaves_the_seeding_retryable(fresh_registry, monkeypatch): + """A transient import failure must not permanently poison the registry. + + ABLATION: move ``_BUILTINS_LOADED = True`` to the very top of + ``_load_builtin_backends``, above the two imports, and this reddens — the flag + reads True after the failed import and the retry early-outs on a registry that + is permanently missing both bundled backends.""" + # The flag sits BELOW the two backend imports precisely so a transient import + # failure retries; the function's docstring and comment both claim exactly + # that property. The adapter twin sets its flag at the very top only because + # its builtins are lazy thunks with nothing to import first. + key = "bmad_loop.adapters.psmux_backend" + # A None value in sys.modules makes `from ... import ...` raise + # ModuleNotFoundError (an ImportError subclass) without touching the disk. + monkeypatch.setitem(sys.modules, key, None) + with pytest.raises(ImportError): + fresh_registry._load_builtin_backends() + assert fresh_registry._BACKENDS == [] + assert fresh_registry._BUILTINS_LOADED is False + + # Let the import succeed again and confirm the retry seeds both builtins. + monkeypatch.delitem(sys.modules, key) + fresh_registry._load_builtin_backends() + assert [name for name, _, _ in fresh_registry._BACKENDS] == ["tmux", "psmux"] From de7a6372b9b28a04b394a59fffd32ca0b857392f Mon Sep 17 00:00:00 2001 From: t Date: Sat, 15 Aug 2026 19:24:54 -0700 Subject: [PATCH 2/4] fix(adapters): keep every same-named entry-point failure (#566) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `importlib.metadata.entry_points(group=...)` does not deduplicate across distributions: two installed packages may both advertise `acme` in one group and the scan yields both. All three external-load scans recorded failures by plain assignment into a name-keyed dict, so the second failing same-named distribution overwrote the first. The operator saw one `warning: ... failed to load`, fixed that package, and met the second package's failure on the next run with nothing saying it had ever been there — the recorded-degrade contract (a failure is never a crash, but it IS always surfaced) was silently under-delivered. The complete inventory is three sites, all now routed through the new leaf `adapters/entrypoints.record_load_error`: - registry.py `_load_external_adapters` -> `_EXTERNAL_ERRORS` - profile.py `_load_external_profiles` -> `_PROFILE_LOAD_ERRORS` - multiplexer.py `_load_external_backends` -> `_EXTERNAL_ERRORS` #566 names the first two only; the mux site is real and is the one it omits. The three `[""]` writes use a fixed singleton key, cannot collapse, and are untouched. Shape: append under the stable key, and label each reason with its distribution when one is resolvable. The key stays `ep.name`, so `detail["entry_point"]` in `validate --json` is unchanged and both `detail` values stay `str` — `VALIDATE_SCHEMA_VERSION` stays 1. Keying on `(distribution, name)` would have been more informative but is not contract-preserving. Both halves are needed: without the label, two packages failing identically render as the same sentence twice with nothing to tell them apart (and the entry-point name is not the name you `pip uninstall`); without the append, the second reason is still lost. Visible behavior diff: every recorded external-load reason now carries its distribution name when resolvable — in `bmad-loop adapters`, `bmad-loop mux`, `validate` text and `validate --json`. The honest limit: two failing same-named distributions still produce exactly ONE finding / ONE warning line, whose text now carries both reasons. The row count does not double. That is the price of the contract-preserving shape, and it meets #566's stated acceptance criterion. Deliberate scope addition: the mux scan is now sorted by (name, distribution) like the other two. Commit 90a7ca9 ordered the adapter and profile scans and missed this one. It belongs here rather than alone because the append order IS this fix's output — unsorted, the two accumulated reasons would render in `sys.path` order, so the same two packages would read differently on two hosts and the test proving the accumulation would be non-deterministic by construction. Ablations (each run singly, restored from a `cp` backup between runs): - registry.py write reverted to the single-key assignment: the adapter test FAILED (`assert 'alpha-adapter' in "ImportError: No module named 'zeta_dep'"`) while the profile and mux twins stayed green. - profile.py only: the profile test FAILED (`'alpha-profiles' in 'RuntimeError: zeta half-installed'`), adapter/mux green. - multiplexer.py only: the mux test FAILED (`'alpha-backend' in "ImportError: No module named 'zeta_dep'"`), adapter/profile green. That per-site independence is what proves the fix reached all three scans. - mux sort key reduced to `key=lambda e: e.name`: `zeta-first` FAILED (recorded `zeta-backend: ...; alpha-backend: ...`) while `alpha-discovered-first` stayed green — the same asymmetry the adapter-side precedent records. - `record_load_error` made to append unconditionally: the lone-failure test FAILED on the leading `"; "`. Full suite 5687 passed / 50 skipped; `uv run pyright` clean; `trunk check` clean (the mux site's `except Exception` still carries no `# noqa: BLE001`, matching what was there before). --- src/bmad_loop/adapters/entrypoints.py | 63 ++++++++++++++++ src/bmad_loop/adapters/multiplexer.py | 35 +++++++-- src/bmad_loop/adapters/profile.py | 16 +++- src/bmad_loop/adapters/registry.py | 14 +++- tests/test_adapter_registry.py | 62 +++++++++++++++ tests/test_external_backends.py | 104 +++++++++++++++++++++++++- tests/test_profile.py | 39 ++++++++++ 7 files changed, 318 insertions(+), 15 deletions(-) create mode 100644 src/bmad_loop/adapters/entrypoints.py diff --git a/src/bmad_loop/adapters/entrypoints.py b/src/bmad_loop/adapters/entrypoints.py new file mode 100644 index 00000000..359ae38c --- /dev/null +++ b/src/bmad_loop/adapters/entrypoints.py @@ -0,0 +1,63 @@ +"""Shared recording for the three ``bmad_loop.*`` entry-point scans. + +The adapter registry (:mod:`~.registry`), the profile loader (:mod:`~.profile`) +and the multiplexer registry (:mod:`~.multiplexer`) each scan their own +entry-point group, and each degrades a broken third-party distribution to a +*recorded* reason rather than a crash. This module owns the one thing all three +recordings must agree on: how a failure becomes an entry in that map. + +A leaf on purpose — standard library only, and **no import of a sibling +adapters module**. Those three do not import each other today, and the package's +builtins load lazily to keep that so; an edge from here into any of them would +put a cycle one refactor away. + +Why the map stays keyed on the entry-point NAME. The key is what reaches +``detail["entry_point"]`` in ``bmad-loop validate --json``. That document is +schema-versioned and evolves additively (see ``documents.validate_document``, +which contracts ``check`` as the matchable identity and states outright that +``message``/``detail`` are for humans) — so widening the key to +``(distribution, name)`` would change a value consumers can already see, while +widening only the reason TEXT does not. + +Why reasons accumulate instead of overwriting. +``importlib.metadata.entry_points(group=...)`` does not deduplicate across +distributions: two installed packages may both advertise ``acme`` in one group, +and the scan yields both. A plain assignment let the second failure silently +overwrite the first, so an operator fixed one package and met the other on the +next run with no sign it had ever been there. Reasons are joined with ``"; "`` +— reasons already contain colons, so a colon separator would be unreadable. + +Why the distribution labels each reason. The entry-point name is not the name +you ``pip uninstall``, and two packages failing the same way otherwise render as +the same sentence twice, with nothing to tell the operator there are two. + +The honest limit: a same-named collision still records ONE row, whose text now +carries both reasons. The row count does not double — that is the price of +leaving the key (and therefore ``--json``) untouched, and it still puts every +reason in front of the operator. +""" + +from __future__ import annotations + +from typing import Any + + +def record_load_error(errors: dict[str, str], ep: Any, exc: BaseException) -> None: + """Record ``exc`` against ``ep``'s name in ``errors``, appending to whatever a + same-named entry point already recorded. + + ``ep`` is annotated ``Any`` deliberately: the callers pass a real + ``importlib.metadata.EntryPoint`` but every test passes a hand-rolled double, + so naming ``EntryPoint`` here would claim a contract this function does not + require (it touches ``.name`` and, defensively, ``.dist.name``). + + The doubled ``getattr`` tolerates a double with no ``dist`` attribute at all + as well as a real entry point whose ``dist`` is ``None``; the truthiness + check then treats an empty distribution name as absent, the same + normalization the scans' ``or ""`` sort keys apply.""" + dist = getattr(getattr(ep, "dist", None), "name", None) + reason = f"{type(exc).__name__}: {exc}" + if dist: + reason = f"{dist}: {reason}" + prior = errors.get(ep.name) + errors[ep.name] = f"{prior}; {reason}" if prior else reason diff --git a/src/bmad_loop/adapters/multiplexer.py b/src/bmad_loop/adapters/multiplexer.py index eb8d4fba..dc26ff5c 100644 --- a/src/bmad_loop/adapters/multiplexer.py +++ b/src/bmad_loop/adapters/multiplexer.py @@ -40,6 +40,7 @@ from pathlib import Path from .. import envvars +from .entrypoints import record_load_error class MultiplexerError(Exception): @@ -510,13 +511,32 @@ def _load_external_backends() -> None: and the ``validate`` preflight via :func:`external_backend_errors`), not raised. Unlike ``_BUILTINS_LOADED``, the loaded-flag is set up front: a third-party import failure is not transient, and retrying on every - selection would re-import (and re-fail) each time.""" + selection would re-import (and re-fail) each time. + + Entry points are visited in (name, distribution) order. ``importlib.metadata`` + yields them in distribution-discovery order, which varies with ``sys.path``, so + without an explicit sort two hosts carrying the same packages could register a + collision in a different order — and the order failures are recorded in would + be a fact about the install rather than about the packages. + + The distribution belongs in the key because the name alone is NOT a total + order. ``entry_points(group=...)`` does not dedup across distributions, so two + packages advertising the same entry-point name come back as two entries, and + ``sorted`` is stable — a name-only key resolves that tie straight back into + ``sys.path`` order. + + Such a same-named failure now ACCUMULATES rather than overwriting: recording + goes through :func:`~.entrypoints.record_load_error`, which appends under the + entry-point name and labels each reason with its distribution.""" global _EXTERNALS_LOADED if _EXTERNALS_LOADED: return _EXTERNALS_LOADED = True try: - eps = importlib.metadata.entry_points(group=MUX_BACKENDS_GROUP) + eps = sorted( + importlib.metadata.entry_points(group=MUX_BACKENDS_GROUP), + key=lambda e: (e.name, getattr(e.dist, "name", "") or ""), + ) except Exception as exc: # diagnostics path, never crash selection _EXTERNAL_ERRORS[""] = f"{type(exc).__name__}: {exc}" return @@ -524,12 +544,17 @@ def _load_external_backends() -> None: try: ep.load() # module import runs register_multiplexer(...) except Exception as exc: # one bad package must not hide the rest - _EXTERNAL_ERRORS[ep.name] = f"{type(exc).__name__}: {exc}" + record_load_error(_EXTERNAL_ERRORS, ep, exc) def external_backend_errors() -> dict[str, str]: - """Entry-point name -> failure reason for every external backend that failed - to load this process (empty when all loaded). For diagnostics surfaces.""" + """Entry-point name -> failure reason(s) for every external backend that failed + to load this process (empty when all loaded). For diagnostics surfaces. + + One value may carry MORE than one reason, ``"; "``-joined: two distributions + may advertise the same entry-point name, and each of their failures is kept + (see :func:`~.entrypoints.record_load_error`). Each reason is labelled with + its distribution whenever one is resolvable.""" return dict(_EXTERNAL_ERRORS) diff --git a/src/bmad_loop/adapters/profile.py b/src/bmad_loop/adapters/profile.py index 8d9d107f..e7859db8 100644 --- a/src/bmad_loop/adapters/profile.py +++ b/src/bmad_loop/adapters/profile.py @@ -16,7 +16,8 @@ co-installed adapter package ships both its class and the profile that selects it with zero project config. Precedence is packaged < entry-point < project (a project TOML always wins). A broken entry point degrades to a recorded reason -(:func:`external_profile_errors`), never a crash. +(:func:`external_profile_errors`), never a crash — one per failing distribution, +kept even when two of them advertise the same name (:mod:`~.entrypoints`). Which adapter *class* drives a profile is the ``adapter`` field, resolved against the :mod:`~.registry` — it is read here but intentionally **not** checked against @@ -40,6 +41,7 @@ import regex from ..platform_util import has_parent_ref, is_absolute_path, names_tree_root +from .entrypoints import record_load_error USAGE_PARSERS = {"claude-jsonl", "codex-rollout", "gemini-chat", "copilot-events", "none"} HOOK_DIALECTS = { @@ -523,13 +525,19 @@ def _load_external_profiles() -> dict[str, CLIProfile]: for profile in _coerce_profiles(produced, ep.name): _EXTERNAL_PROFILES.setdefault(profile.name, profile) except Exception as exc: # noqa: BLE001 — one bad package must not hide the rest - _PROFILE_LOAD_ERRORS[ep.name] = f"{type(exc).__name__}: {exc}" + record_load_error(_PROFILE_LOAD_ERRORS, ep, exc) return _EXTERNAL_PROFILES def external_profile_errors() -> dict[str, str]: - """Entry-point name -> failure reason for every external profile provider that - failed to load this process (empty when all loaded). For diagnostics surfaces. + """Entry-point name -> failure reason(s) for every external profile provider + that failed to load this process (empty when all loaded). For diagnostics + surfaces. + + One value may carry MORE than one reason, ``"; "``-joined: two distributions + may advertise the same entry-point name, and each of their failures is kept + (see :func:`~.entrypoints.record_load_error`). Each reason is labelled with + its distribution whenever one is resolvable. Performs the scan rather than assuming a neighbouring call already did. The only other trigger is :func:`load_profiles`, which ``validate`` reaches through diff --git a/src/bmad_loop/adapters/registry.py b/src/bmad_loop/adapters/registry.py index ab29bd41..8a1f4b94 100644 --- a/src/bmad_loop/adapters/registry.py +++ b/src/bmad_loop/adapters/registry.py @@ -34,7 +34,8 @@ are seeded by :func:`register_adapter` itself, so an external can never shadow a bundled name however early its import lands. A broken third-party distribution degrades to a recorded, surfaced reason (:func:`external_adapter_errors`) and can -never break selection. +never break selection — one reason per failing distribution, kept even when two of +them advertise the same entry-point name (:mod:`~.entrypoints`). **Two deliberate asymmetries versus the multiplexer seam** (this is not a copy-paste omission): @@ -58,6 +59,8 @@ from collections.abc import Callable from dataclasses import dataclass +from .entrypoints import record_load_error + # The two bundled kind names, as constants rather than literals scattered across # modules. `validate`'s httpx check keys on OPENCODE_HTTP because httpx is *that # family's* optional extra — a fact about one bundled family, which is a different @@ -241,13 +244,18 @@ def _load_external_adapters() -> None: try: ep.load() # module import runs register_adapter(...) except Exception as exc: # noqa: BLE001 — one bad package must not hide the rest - _EXTERNAL_ERRORS[ep.name] = f"{type(exc).__name__}: {exc}" + record_load_error(_EXTERNAL_ERRORS, ep, exc) def external_adapter_errors() -> dict[str, str]: - """Entry-point name -> failure reason for every external adapter that failed + """Entry-point name -> failure reason(s) for every external adapter that failed to load this process (empty when all loaded). For diagnostics surfaces. + One value may carry MORE than one reason, ``"; "``-joined: two distributions + may advertise the same entry-point name, and each of their failures is kept + (see :func:`~.entrypoints.record_load_error`). Each reason is labelled with + its distribution whenever one is resolvable. + Performs the scan itself rather than relying on a neighbouring ``known_adapter_kinds`` / ``detect_adapters`` call having run first — an accessor whose emptiness depends on call order reads as "nothing failed".""" diff --git a/tests/test_adapter_registry.py b/tests/test_adapter_registry.py index c50505e4..3ffb1921 100644 --- a/tests/test_adapter_registry.py +++ b/tests/test_adapter_registry.py @@ -421,6 +421,68 @@ def load(): assert registry.get_adapter_kind("acme").needs_mux is False # alpha-adapter's +def test_same_named_broken_distributions_both_record_a_reason(scan_adapter_registry): + """Two distributions may advertise the SAME entry-point name, and both may be + broken. A name-keyed assignment let the second overwrite the first: the + operator fixed the package they were shown and met the other one on the next + run, with nothing saying it had ever been there. Both reasons are kept now, + each labelled with its distribution — the entry-point name is not the name you + `pip uninstall`, and two packages failing identically would otherwise render as + the same sentence twice. + + The fixture trap #566 itself flags: a test that only asserts "two reasons are + present" passes for the wrong reason if the two entry points accidentally got + DIFFERENT names (`_FakeEntryPoint` defaults `dist` per name, not the reverse). + Pinning the single shared key is what forecloses that — and it is the shape + decision's own assertion besides, per the comment below. + + Ablation: restore the single-key write + (`_EXTERNAL_ERRORS[ep.name] = f"{type(exc).__name__}: {exc}"`) in + `_load_external_adapters` and this test fails on the missing `alpha-adapter` + half, while the profile and mux twins stay green — that per-site independence + is what proves the fix reached all three scans rather than one.""" + registry, arm = scan_adapter_registry + + def boom(msg): + def load(): + raise ImportError(msg) + + return load + + arm( + _FakeEntryPoint("acme", boom("No module named 'alpha_dep'"), dist="alpha-adapter"), + _FakeEntryPoint("acme", boom("No module named 'zeta_dep'"), dist="zeta-adapter"), + ) + assert registry.get_adapter_kind("generic").needs_mux is True # selection still works + reason = registry.external_adapter_errors()["acme"] + assert "alpha-adapter" in reason and "zeta-adapter" in reason + assert "alpha_dep" in reason and "zeta_dep" in reason + # Still ONE key — the shape decision. The key set is what reaches + # `detail["entry_point"]` in `validate --json`, so it deliberately does not grow; + # only the human-facing reason string widens. + assert list(registry.external_adapter_errors()) == ["acme"] + + +def test_a_lone_failure_records_exactly_one_reason(scan_adapter_registry): + """The accumulation must not cost a lone failure a leading separator or a + duplicate: one broken entry point still records exactly one reason. Spelled out + in full rather than substring-matched, because a substring check is blind to + precisely the leading `"; "` this guards. + + Ablation: make `record_load_error` append unconditionally + (`prior = errors.get(ep.name, "")` then `f"{prior}; {reason}"`) and this test + fails on the leading separator.""" + registry, arm = scan_adapter_registry + + def boom(): + raise ImportError("No module named 'ghost_dependency'") + + arm(_FakeEntryPoint("lonely", boom, dist="lonely-adapter")) + errors = registry.external_adapter_errors() + assert errors["lonely"] == ("lonely-adapter: ImportError: No module named 'ghost_dependency'") + assert "; " not in errors["lonely"] + + def test_real_dist_info_metadata_is_discovered(fresh_adapter_registry, monkeypatch, tmp_path): """End-to-end against genuine packaging metadata: a real ``*.dist-info`` + module on sys.path is found by the unpatched importlib scan and its import diff --git a/tests/test_external_backends.py b/tests/test_external_backends.py index 8a23ab2d..983f3c58 100644 --- a/tests/test_external_backends.py +++ b/tests/test_external_backends.py @@ -29,12 +29,22 @@ from bmad_loop.adapters.tmux_backend import TmuxMultiplexer +class _FakeDist: + """Stands in for ``EntryPoint.dist``; the scan orders on its ``.name``.""" + + def __init__(self, name): + self.name = name + + class _FakeEntryPoint: - """Duck-typed stand-in for importlib.metadata.EntryPoint: the loader only - touches ``.name`` and ``.load()``.""" + """Duck-typed stand-in for importlib.metadata.EntryPoint: the loader touches + ``.name``, ``.dist`` (the scan's tiebreak — see `_load_external_backends`) and + ``.load()``. ``dist`` defaults to a distinct-per-name stand-in so the ordering + of same-named entries is only ever decided by a test that sets it.""" - def __init__(self, name, load): + def __init__(self, name, load, dist=None): self.name = name + self.dist = _FakeDist(dist if dist is not None else f"{name}-dist") self._load = load def load(self): @@ -163,6 +173,94 @@ def load(): assert len(calls) == 1 +def test_same_named_broken_distributions_both_record_a_reason(scan_registry, monkeypatch): + """Two distributions may advertise the SAME entry-point name in this group, and + both may be broken. A name-keyed assignment let the second overwrite the first: + the operator fixed the package they were shown and met the other one on the next + run, with nothing saying it had ever been there. Both reasons are kept now, each + labelled with its distribution — the entry-point name is not the name you + `pip uninstall`, and two packages failing identically would otherwise render as + the same sentence twice. (#566 names the adapter and profile scans only; this + third site is the one it omits.) + + The fixture trap #566 itself flags: asserting only "two reasons are present" + passes for the wrong reason if the two entry points accidentally got DIFFERENT + names. Pinning the single shared key forecloses that, and is the shape + decision's own assertion besides — see the comment below. + + Ablation: restore the single-key write + (`_EXTERNAL_ERRORS[ep.name] = f"{type(exc).__name__}: {exc}"`) in + `_load_external_backends` and this test fails on the missing `alpha-backend` + half, while the adapter and profile twins stay green — that per-site + independence is what proves the fix reached all three scans rather than one.""" + registry, arm = scan_registry + + def boom(msg): + def load(): + raise ImportError(msg) + + return load + + arm( + _FakeEntryPoint("acme", boom("No module named 'alpha_dep'"), dist="alpha-backend"), + _FakeEntryPoint("acme", boom("No module named 'zeta_dep'"), dist="zeta-backend"), + ) + monkeypatch.setattr(sys, "platform", "linux") + backend, name, _reason = registry._select() + assert isinstance(backend, TmuxMultiplexer) and name == "tmux" # selection still works + reason = registry.external_backend_errors()["acme"] + assert "alpha-backend" in reason and "zeta-backend" in reason + assert "alpha_dep" in reason and "zeta_dep" in reason + # Still ONE key — the shape decision. The key set is what reaches + # `detail["entry_point"]` in `validate --json`, so it deliberately does not grow; + # only the human-facing reason string widens. + assert list(registry.external_backend_errors()) == ["acme"] + + +@pytest.mark.parametrize( + "order", [("alpha", "zeta"), ("zeta", "alpha")], ids=["alpha-discovered-first", "zeta-first"] +) +def test_same_named_entry_points_are_visited_in_distribution_order( + scan_registry, monkeypatch, order +): + """The mux analogue of the adapter scan's + `test_same_named_entry_points_resolve_by_distribution_not_install_order`: this + scan was left unsorted when commit 90a7ca9 ordered the other two. + + It matters here because the accumulated reason is now READ in append order. A + bare `entry_points(group=...)` yields distributions in `sys.path` order, so + without the sort the assertion below would be a fact about the machine running + the test — the same two packages would render their two reasons in the opposite + order on another host, and the accumulation test above would be + non-deterministic by construction. Sorting on (name, distribution) is what makes + the recording a fact about the packages. + + Both parameters arm the identical pair and differ only in the order the scan + yields them; `alpha-backend`'s reason must come first either way. + + ABLATION: drop the `getattr(e.dist, ...)` half of the sort key in + `_load_external_backends` and the `zeta-first` case reddens while + `alpha-discovered-first` stays green — which is the finding: the name-only key + is right only when the install happens to agree with it.""" + registry, arm = scan_registry + + def boom(msg): + def load(): + raise ImportError(msg) + + return load + + eps = { + "alpha": _FakeEntryPoint("acme", boom("alpha broke"), dist="alpha-backend"), + "zeta": _FakeEntryPoint("acme", boom("zeta broke"), dist="zeta-backend"), + } + arm(*(eps[k] for k in order)) + monkeypatch.setattr(sys, "platform", "linux") + registry._select() + + assert registry.external_backend_errors()["acme"].startswith("alpha-backend: ") + + def test_mux_command_surfaces_load_failures(scan_registry, monkeypatch, capsys, tmp_path): """`bmad-loop mux` names a failed external package — the one place an operator looks when an installed backend is missing from the table.""" diff --git a/tests/test_profile.py b/tests/test_profile.py index ce683293..bc9de017 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -698,6 +698,45 @@ def test_profile_provider_returning_a_non_iterable_is_rejected(profile_scan): assert "iterable of CLIProfile" in profile_mod.external_profile_errors()["acme"] +def test_same_named_broken_distributions_both_record_a_reason(profile_scan): + """Two distributions may advertise the SAME entry-point name in this group, and + both may be broken. A name-keyed assignment let the second overwrite the first: + the operator fixed the package they were shown and met the other one on the next + run, with nothing saying it had ever been there. Both reasons are kept now, each + labelled with its distribution — the entry-point name is not the name you + `pip uninstall`, and two providers failing identically would otherwise render as + the same sentence twice. + + The fixture trap #566 itself flags: asserting only "two reasons are present" + passes for the wrong reason if the two entry points accidentally got DIFFERENT + names. Pinning the single shared key forecloses that, and is the shape + decision's own assertion besides — see the comment below. + + Ablation: restore the single-key write + (`_PROFILE_LOAD_ERRORS[ep.name] = f"{type(exc).__name__}: {exc}"`) in + `_load_external_profiles` and this test fails on the missing `alpha-profiles` + half, while the adapter and mux twins stay green.""" + + def boom(msg): + def load(): + raise RuntimeError(msg) + + return load + + profile_scan( + _FakeEntryPoint("acme", boom("alpha half-installed"), dist="alpha-profiles"), + _FakeEntryPoint("acme", boom("zeta half-installed"), dist="zeta-profiles"), + ) + assert "claude" in load_profiles() # built-ins unaffected + reason = profile_mod.external_profile_errors()["acme"] + assert "alpha-profiles" in reason and "zeta-profiles" in reason + assert "alpha half-installed" in reason and "zeta half-installed" in reason + # Still ONE key — the shape decision. The key set is what reaches + # `detail["entry_point"]` in `validate --json`, so it deliberately does not grow; + # only the human-facing reason string widens. + assert list(profile_mod.external_profile_errors()) == ["acme"] + + def test_profile_scan_failure_degrades(profile_scan): """The enumeration itself blowing up leaves built-in loading working, with the scan failure recorded.""" From 4cadcd3894242b2d43ea843671458a5f884a84f8 Mon Sep 17 00:00:00 2001 From: t Date: Sat, 15 Aug 2026 19:34:03 -0700 Subject: [PATCH 3/4] fix(runsetup): name the profile and kind when an adapter rejects a bootstrap keyword (#569) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `make_adapters` converts a family-DECLARED construction failure into a clean `error:` line via `builder.construct_error`. A SIGNATURE mismatch is not that: an out-of-tree adapter class whose `__init__` does not accept a keyword the bootstrap passes is refused by the interpreter, not by the family, so the `TypeError` no family declares escaped both arms as a bare traceback. A second arm now gives it the same treatment the `ImportError` arm above it already had. Discriminator: traceback DEPTH. Argument binding fails before any `__init__` frame is pushed, so a mismatch carries this frame alone (`tb_next is None`), while a `TypeError` raised inside a working `__init__` carries that frame too and stays a bug in that package that must surface as itself. Verified on CPython 3.11-3.14, and re-confirmed here on 3.13.14. It is valid ONLY because the `except` shares a frame with the `cls(**build_kwargs)` call — extracting that call into a helper or widening the `try` would break it silently, which the arm's comment says in place. Its failure direction is safe. A genuine mismatch hidden behind a Python-level metaclass `__call__`, or one raised by a `super().__init__()` call in the body, reads as deeper and re-raises — a false NEGATIVE that is exactly today's behavior. The dangerous direction, relabelling a real bug in someone's `__init__` as a signature mismatch, is the one the check refuses. `except builder.construct_error` stays FIRST, so a family that declares `TypeError` in its own `construct_error` keeps its existing `error: {e}` line. No bundled family declares one today (`()` for generic, `(OpencodeServerError,)` for opencode), so that pins the contract for a future one. MESSAGE QUALITY ONLY — the issue's "after the run dir exists" framing has aged out. #569 argues the escape strands a run directory because `compose_run` has already written the run state and pid. That is no longer true: both composers wrap the composition in `except BaseException: _unwind_composition(...); raise` (runsetup.py:1053 and :1190), and `BaseException` catches a bare `TypeError` exactly as it catches a `SystemExit`. The defect the issue names did not age; its urgency framing did. Nothing here reorders the composition, restructures `compose_run`, or adds an `inspect.signature` pre-flight. VISIBLE BEHAVIOR DIFF. At run launch, where a traceback used to print: error: profile 'narrowcli': adapter kind 'narrowkind' rejected this run's adapter keywords: TypeError: _Narrow.__init__() got an unexpected keyword argument 'profile' CPython already names the offending keyword, and `{e}` carries it verbatim, so the arm does not parse it out. Process exit code is unchanged at 1. `_unwind_composition`'s docstring enumerated "five sites" where `make_adapters` raises `SystemExit`; this adds a sixth, so that count and its list are corrected, along with the same claim at runsetup.py:480, `_claim_run_dir`'s docstring, and three repetitions in tests/test_runsetup.py and tests/test_engine.py. Tests (tests/test_adapter_registry.py, beside the existing construct_error pair), with the wiring and the predicate ablated on separate axes — an ablation removing the whole arm proves nothing about the discriminator inside it: T1 signature mismatch -> SystemExit naming profile, kind and keyword T2 TypeError from the constructor body is not relabelled T3 TypeError raised two frames deep is not relabelled T4 a declared TypeError keeps the original construct_error line Ablation results (source `cp`-backed up outside the repo and restored from that backup, never `git checkout`; md5 re-verified equal after each restore): A. WIRING — delete the whole `except TypeError` arm: T1 FAILED with `TypeError: ... got an unexpected keyword argument 'profile'` propagating from runsetup.py:551, i.e. the pre-fix behavior. T2, T3, T4 all PASSED — a bare TypeError is what T2/T3 already assert, which is the honest way to show they do not measure the wiring. B. PREDICATE — keep the arm, delete the two discriminator lines so every TypeError becomes a SystemExit: T2 FAILED and T3 FAILED, both on the mislabel the check exists to prevent — `SystemExit: error: profile 'bodyboom': adapter kind 'bodyboom' rejected this run's adapter keywords: TypeError: a real bug, not a signature mismatch`. T1 PASSED, T4 PASSED. C. ARM ORDER — swap the two `except` arms so `except TypeError` runs first: T4 FAILED alone, `TypeError: the server refused the handshake` escaping because the body-raised TypeError is deeper than one frame and that arm re-raises it instead of letting the declared arm convert it. T1, T2, T3 and the existing construct_error test all PASSED. Verified: `uv run pytest -q -n logical` 5691 passed / 50 skipped; `uv run pyright` 0 errors; `trunk fmt` + `trunk check` clean. --- src/bmad_loop/runsetup.py | 41 ++++++-- tests/test_adapter_registry.py | 172 +++++++++++++++++++++++++++++++++ tests/test_engine.py | 9 +- tests/test_runsetup.py | 9 +- 4 files changed, 217 insertions(+), 14 deletions(-) diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index 73a5f153..c685de41 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -477,7 +477,7 @@ def make_adapters( # escaping ImportError used to strand that run directory behind a # traceback, recorded as an accepted consequence; it no longer does — # both composers unwind the whole composition on any escape (see - # `_unwind_composition`), and this raise is one of the five SystemExits + # `_unwind_composition`), and this raise is one of the six SystemExits # that path exists for. What that changes is the run dir, not the # message: the narrowing below is a separate decision and still holds. # ImportError ONLY, on the same rule as `construct_error` below: a @@ -535,6 +535,15 @@ def make_adapters( # `(OpencodeServerError,)` for one that can — and becomes a SystemExit # so a run aborts with a clean message instead of a traceback. # `except ():` catches nothing, which is exactly right for the `()` case. + # A SIGNATURE mismatch is not a declared failure and no family names it, + # so it escaped both arms as a bare traceback until the second one below + # (#569): the bootstrap keyword set grows, and an out-of-tree class whose + # `__init__` does not accept a keyword this function passes is refused by + # the interpreter, not by the family. That arm keys on traceback DEPTH + # because depth is what separates the two TypeErrors — binding fails + # before any `__init__` frame is pushed, a raise from inside one carries + # that frame. ORDER MATTERS: a family that declares `TypeError` in its own + # `construct_error` keeps the `error: {e}` line above, unchanged. cls = builder.dev if synthesizes else builder.plain build_kwargs = {**common, "paths": paths} if synthesizes else common try: @@ -542,6 +551,25 @@ def make_adapters( by_cfg[key] = cls(**build_kwargs) # pyright: ignore[reportArgumentType] except builder.construct_error as e: raise SystemExit(f"error: {e}") from e + except TypeError as e: + # A binding failure is raised by the interpreter BEFORE any __init__ + # frame is pushed, so the traceback holds this frame alone. A + # TypeError from inside a working __init__ carries that frame too and + # is a bug in that package: it must surface as itself, on the same + # rule the ImportError arm above states. Errs toward re-raising — a + # mismatch behind a Python-level metaclass `__call__` or a + # `super().__init__` call reads as deeper and re-raises, which is + # today's behavior; relabelling a real bug is the direction that would + # cost a diagnosis. Only valid while this `except` sits in the SAME + # FRAME as the call — do not extract the construct call into a helper + # or widen the `try`, either breaks it silently. + if e.__traceback__ is None or e.__traceback__.tb_next is not None: + raise + raise SystemExit( + f"error: profile {profile.name!r}: adapter kind " + f"{profile.adapter!r} rejected this run's adapter keywords: " + f"{type(e).__name__}: {e}" + ) from e adapters[role] = by_cfg[key] return adapters @@ -821,7 +849,7 @@ def _claim_run_dir(run_dir: Path) -> None: MUST stay outside the composers' ``try`` — a refusal that reached the unwind arm would delete the very run it exists to protect. ``SystemExit`` matches the other launch-time refusals an operator reads as an ``error:`` line - (``_reject_bad_run_id``, and ``make_adapters``' five sites). + (``_reject_bad_run_id``, and ``make_adapters``' six sites). Applied to a minted id too, not just a supplied one. ``new_run_id`` is a timestamp plus two random bytes, so a same-second collision is remote rather @@ -845,11 +873,12 @@ def _unwind_composition(project: Path, run_dir: Path, journal: Journal | None) - provably this composition's, never a pre-existing one the caller named. Reached from an ``except BaseException`` arm, because the failure it exists - for is a :class:`SystemExit`: :func:`make_adapters` raises one at five sites + for is a :class:`SystemExit`: :func:`make_adapters` raises one at six sites (unresolvable profile, unknown adapter kind, a kind that fails to load, a - construction failure, an unusable multiplexer), every one of them *after* - ``save_state`` has published a run dir carrying ``finished=False`` / - ``crashed=False`` and no ``run-start``. Nothing reconciles that shape — + construction failure, an adapter class that rejects a bootstrap keyword, an + unusable multiplexer), every one of them *after* ``save_state`` has published + a run dir carrying ``finished=False`` / ``crashed=False`` and no + ``run-start``. Nothing reconciles that shape — :func:`runs.reconcile_stale_worktrees` only touches ``is_finished`` runs — so it lingers as a resumable-looking empty run. diff --git a/tests/test_adapter_registry.py b/tests/test_adapter_registry.py index 3ffb1921..3b9e707f 100644 --- a/tests/test_adapter_registry.py +++ b/tests/test_adapter_registry.py @@ -695,6 +695,178 @@ def __init__(self, **kwargs): runsetup.make_adapters(project.project, _run_dir(project.project), pol) +def test_make_adapters_signature_mismatch_becomes_systemexit(fresh_adapter_registry, project): + """#569: an out-of-tree class whose `__init__` refuses a keyword the bootstrap + passes is refused by the INTERPRETER, not by the family, so `TypeError` is a + failure no `construct_error` declares and it escaped as a bare traceback. It now + reads like the rest of the bootstrap: one `error:` line naming the profile, the + kind and — carried verbatim out of CPython's own message — the keyword itself. + + ABLATION (wiring): delete the whole `except TypeError` arm in `make_adapters` + and this reddens, the bare TypeError propagating instead of a SystemExit. T2/T3 + (`..._from_the_constructor_body_...`, `..._raised_deeper_...`) stay GREEN under + that same ablation — a bare TypeError is exactly what they assert — which is how + you can tell they pin the discriminator and not the wiring.""" + built: list[object] = [] + + class _Narrow: + # rejects `profile`, `extra_args`, `events_dir`, ... — every other keyword + # `make_adapters` builds into `common`. + def __init__(self, *, run_dir, policy): + built.append(self) + + fresh_adapter_registry.register_adapter( + "narrowkind", + needs_mux=False, + load=lambda: AdapterBuilder(plain=_Narrow, dev=_Narrow, construct_error=()), + ) + install_bmad_config(project) + _write_profile(project.project, "narrowcli", adapter="narrowkind") + _write_policy(project.project, '[adapter]\nname = "narrowcli"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + with pytest.raises(SystemExit, match=r"narrowcli.*narrowkind.*unexpected keyword argument"): + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + + # Re-raised for the field assertions the `match=` regex cannot make: the message + # reads as an `error:` line, and the keyword CPython named is one this class + # really does reject — pinning that `{e}` carries the offending keyword verbatim + # without depending on which of them CPython reports first. + with pytest.raises(SystemExit) as excinfo: + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + message = str(excinfo.value) + assert message.startswith("error: ") + assert "'narrowcli'" in message and "'narrowkind'" in message + named = message.split("unexpected keyword argument ")[1].strip().strip("'\"") + assert named in { + "profile", + "extra_args", + "usage_grace_s", + "stop_without_result_nudges", + "events_dir", + "paths", + } + # Binding fails before the constructor body runs, so nothing was ever built — + # the raise is the whole outcome, and no half-built adapter reached a caller. + assert built == [] + + +def test_make_adapters_typeerror_from_the_constructor_body_is_not_relabelled( + fresh_adapter_registry, project +): + """The other half of #569, and the same rule the `ImportError` arm states: a + DECLARED failure gets a clean `error:` line, and anything else is a bug in that + package that must surface as itself. A `TypeError` raised from inside a working + `__init__` is that second thing, and relabelling it "rejected this run's adapter + keywords" would hand an adapter author a misleading message where the traceback + was the whole diagnosis. + + The discriminator is traceback DEPTH, not the exception text: argument binding + fails before any `__init__` frame is pushed, so a mismatch carries this frame + alone (`tb_next is None`) while a raise from inside the body carries that frame + too. It is only valid because the `except` shares a frame with the `cls(...)` + call — extracting that call into a helper would silently break it. + + Do not "improve" the check into `tb_next.tb_next is None` or any fixed depth. + Its errors already run in the safe direction: a genuine mismatch hidden behind a + Python-level metaclass `__call__`, or one raised by a `super().__init__()` call + in the body, reads as deeper and re-raises — a false NEGATIVE that is exactly + today's pre-fix behavior. The dangerous direction is the one this check refuses. + + ABLATION (predicate): delete the two discriminator lines (`if e.__traceback__ is + None or e.__traceback__.tb_next is not None: raise`) so every TypeError becomes + a SystemExit, and this reddens together with T3 while T1 + (`..._signature_mismatch_...`) stays green.""" + + class _Exploding: + def __init__(self, **kwargs): + raise TypeError("a real bug, not a signature mismatch") + + fresh_adapter_registry.register_adapter( + "bodyboom", + needs_mux=False, + load=lambda: AdapterBuilder(plain=_Exploding, dev=_Exploding, construct_error=()), + ) + install_bmad_config(project) + _write_profile(project.project, "bodyboom", adapter="bodyboom") + _write_policy(project.project, '[adapter]\nname = "bodyboom"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + with pytest.raises(TypeError, match="a real bug, not a signature mismatch"): + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + + +def test_make_adapters_typeerror_raised_deeper_in_the_constructor_is_not_relabelled( + fresh_adapter_registry, project +): + """Same rule as the test above, one frame further down: the `TypeError` comes + from a helper the constructor calls, so the traceback is three frames deep + rather than two. + + This test exists so a later session cannot "simplify" the discriminator to a + fixed depth such as `tb_next.tb_next is None` — that spelling would pass the + two-frame test above and relabel this one, and a real bug in an adapter would + start reading as a signature mismatch. Only `tb_next is None` distinguishes + "no `__init__` frame was ever pushed" from "some number of them were". + + ABLATION (predicate): the same one T2 records — delete the two discriminator + lines and this reddens with it, while T1 stays green.""" + + def _helper(): + raise TypeError("a real bug two frames in") + + class _Exploding: + def __init__(self, **kwargs): + _helper() + + fresh_adapter_registry.register_adapter( + "deepboom", + needs_mux=False, + load=lambda: AdapterBuilder(plain=_Exploding, dev=_Exploding, construct_error=()), + ) + install_bmad_config(project) + _write_profile(project.project, "deepboom", adapter="deepboom") + _write_policy(project.project, '[adapter]\nname = "deepboom"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + with pytest.raises(TypeError, match="a real bug two frames in"): + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + + +def test_make_adapters_declared_typeerror_keeps_the_construct_error_line( + fresh_adapter_registry, project +): + """Pins the ARM ORDER. `except builder.construct_error` stays FIRST, so a family + that names `TypeError` in its own `construct_error` keeps the plain `error: {e}` + line it has always had — the new arm must not steal a declared failure and + re-word it as a signature mismatch. No bundled family declares `TypeError` + today (`()` for generic, `(OpencodeServerError,)` for opencode), so this pins + the contract for a future one rather than current behavior. + + ABLATION (arm order): swap the two `except` arms so `except TypeError` runs + first and this reddens alone — the body-raised TypeError is deeper than one + frame, so that arm re-raises it instead of letting the declared arm convert + it.""" + + class _Exploding: + def __init__(self, **kwargs): + raise TypeError("the server refused the handshake") + + fresh_adapter_registry.register_adapter( + "declaredtype", + needs_mux=False, + load=lambda: AdapterBuilder(plain=_Exploding, dev=_Exploding, construct_error=(TypeError,)), + ) + install_bmad_config(project) + _write_profile(project.project, "declaredtype", adapter="declaredtype") + _write_policy(project.project, '[adapter]\nname = "declaredtype"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + with pytest.raises(SystemExit, match="the server refused the handshake") as excinfo: + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + assert "rejected this run's adapter keywords" not in str(excinfo.value) + + def test_make_adapters_load_thunk_failure_becomes_systemexit(fresh_adapter_registry, project): """A load thunk that raises — the family's own module missing an optional dependency is the ordinary case — aborts with a clean SystemExit naming the diff --git a/tests/test_engine.py b/tests/test_engine.py index bbb3e55f..48a52c8c 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -7977,11 +7977,12 @@ def test_auto_sweep_system_exit_does_not_kill_the_parent(project): Not a hypothetical shape — `runsetup.make_adapters` raises exactly this for an unresolvable profile, an unknown/unloadable adapter kind, a failed adapter - construction, and an unusable multiplexer; that last gate re-probes live - (`mux_usable` bottoms out in a bare `shutil.which`) on every call, so a child - sweep can hit it in a parent run that launched fine. + construction, an adapter class that rejects a bootstrap keyword, and an + unusable multiplexer; that last gate re-probes live (`mux_usable` bottoms out + in a bare `shutil.which`) on every call, so a child sweep can hit it in a + parent run that launched fine. - Every one of those five sites is inside `compose_sweep`, ahead of the + Every one of those six sites is inside `compose_sweep`, ahead of the `on_started` boundary, so this models the raise WITHOUT signalling and the record is `sweep-auto-not-started`: no child run dir survives an adapter build that exits. diff --git a/tests/test_runsetup.py b/tests/test_runsetup.py index cd38bee8..a537cf89 100644 --- a/tests/test_runsetup.py +++ b/tests/test_runsetup.py @@ -9,7 +9,7 @@ The second concern here is composition atomicity: `compose_run` and `compose_sweep` publish a run dir before they can know the run will start, and -`make_adapters` raises `SystemExit` from five sites after that point. The unwind +`make_adapters` raises `SystemExit` from six sites after that point. The unwind that keeps a failed launch from stranding a resumable-looking empty run is pinned at the end of the file. """ @@ -312,7 +312,7 @@ def test_digest_raises_on_an_unresolvable_profile(pinned): RUN_ID = "20260812-101500-ab12" # The message `make_adapters` raises for an unusable multiplexer — the one of its -# five SystemExit sites that is reachable in a run that launched fine, since +# six SystemExit sites that is reachable in a run that launched fine, since # `mux_usable` bottoms out in a live `shutil.which` on every call. BOOM = "error: multiplexer backend TmuxBackend is not usable on this host" @@ -344,9 +344,10 @@ def _fake_paths(project): def unwinding(tmp_path): """A project plus a `make_adapters` that fails the way the real one does. - `runsetup.make_adapters` raises `SystemExit` at five sites (an unresolvable + `runsetup.make_adapters` raises `SystemExit` at six sites (an unresolvable profile, an unknown adapter kind, a kind that fails to load, a construction - failure, an unusable multiplexer), and every one lands *after* the composer has + failure, an adapter class that rejects a bootstrap keyword, an unusable + multiplexer), and every one lands *after* the composer has published the run dir, its `state.json` and the out-of-tree config-digest stamp. The fake records that all three exist at the moment it is called, so the assertions after the raise grade a *removal* — an "is it gone" assertion passes From b789ffce051e4e34f0731bcaebdbf184a0e4077a Mon Sep 17 00:00:00 2001 From: t Date: Sat, 15 Aug 2026 19:38:48 -0700 Subject: [PATCH 4/4] docs(changelog): record the registry residuals (#565, #566, #569) Three entries under `## [Unreleased]` -> `### Fixed`, one per issue. The #566 entry carries the two things a reader has to be told and cannot infer from the headline: that every recorded external-load failure now names its distribution (the entry-point name is not what you uninstall), and the honest limit that two failing same-named distributions still produce ONE finding whose text carries both reasons `"; "`-joined -- the row count does not double. That shape is what keeps `detail`'s key set and value types unchanged, so `VALIDATE_SCHEMA_VERSION` stays 1, worded after the `STATUS_SCHEMA_VERSION` precedent in the section above. The mux scan's new (name, distribution) ordering is called out as the deliberate scope addition it is. The #569 entry claims message quality only. It does not claim to fix run-directory stranding: both composers already wrap composition in `except BaseException: _unwind_composition(...); raise`, so the issue's "after the run dir exists" framing has aged out. No version section authored, no version string touched. --- CHANGELOG.md | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a354f4a4..cc37fbda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -272,6 +272,46 @@ breaking changes may land in a minor release. _implementing_ rate limiting prints all day — follow-up: #610. The four unseeded profiles (`codex`, `gemini`, `copilot`, `antigravity`) are unchanged and still ship none. +- **An out-of-tree module that claims a bundled transport's name is no longer driven in the bundled + backend's place (#565).** Backend selection breaks ties on registration order, and the bundled + `tmux` / `psmux` entries were seeded only on the first multiplexer resolution — so any + registration that landed before it owned that name, and the whole process silently ran on a + third-party transport. The trigger is reachable rather than theoretical: a plugin's `[python]` + module is executed in-process by the plugin registry, which has no ordering relationship to when + the mux is first resolved. Registering a backend now seeds the bundled ones first, making + builtins-first an invariant of the registration itself rather than a property of which import ran + first — the same fix the adapter registry already carries. A shadowing external is still + registered, just behind the bundled entry. + +- **Two broken distributions that publish the same entry-point name no longer collapse to a single + recorded error (#566).** `importlib.metadata` does not deduplicate entry points across + distributions, and all three external-load scans — adapters, profiles and mux backends — recorded + failures by assignment into a name-keyed map, so the second failing package overwrote the first. + You fixed the one failure you were shown, then met the other on the next run with nothing saying + it had ever been there. Both reasons now surface, and every recorded external-load failure names + its **distribution** when one is resolvable — in `bmad-loop adapters`, `bmad-loop mux`, + `validate`'s human output and `validate --json` — because the entry-point name is not the + distribution you uninstall. + + The limit, plainly: two failing same-named distributions still produce **one** finding and one + warning line, whose text now carries both reasons `"; "`-joined. The row count does not double. + That is what keeps the change contract-preserving — `detail`'s key set and value types are + unchanged, so `VALIDATE_SCHEMA_VERSION` stays 1. + + **A deliberate scope addition:** the multiplexer's entry-point scan is now ordered by (name, + distribution), as the adapter and profile scans already were, so which of two same-named + distributions is recorded first is a fact about the packages rather than about `sys.path`. + +- **A run launch whose adapter class rejects a bootstrap keyword now aborts on a clean `error:` + line instead of a bare traceback (#569).** The construct call converted only the failures an + adapter family declares, and a signature mismatch is refused by the interpreter rather than by + the family — so an out-of-tree class whose `__init__` does not accept a keyword the bootstrap + passes escaped uncaught. The abort now names the profile, the adapter kind and the rejected + keyword; the exit code is unchanged at 1. Deliberately unchanged: a `TypeError` raised from + inside a working `__init__` still surfaces as itself, because that is a bug in that package + rather than a declared failure, and relabelling it as a signature mismatch would bury it. This is + message quality only — an error escaping here already unwound the composition it had started. + ## [0.10.0] — 2026-08-14 Much of this section is the `release/0.9.x` hotfix line brought forward onto `main` (#433).