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). 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 0d994b9f..dc26ff5c 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 @@ -37,6 +40,7 @@ from pathlib import Path from .. import envvars +from .entrypoints import record_load_error class MultiplexerError(Exception): @@ -430,8 +434,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 +460,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 +490,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 @@ -480,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 @@ -494,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/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 c50505e4..3b9e707f 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 @@ -633,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_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"] 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_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.""" 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