From a5a0fcd7a7d53d274a18818778514715b1a1c77c Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 13:49:59 -0700 Subject: [PATCH 1/6] feat(adapters): coding-CLI adapter registry (Seam A), re-sited onto main MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The transport axis has long been extensible out-of-tree (register_multiplexer + the bmad_loop.mux_backends entry-point group); the CLI adapter axis had no such seam, so a CLI needing a new adapter *class* forced a name-branch in the run bootstrap and a hardcoded valid-kinds set. This lands the missing registry so a new adapter family — and its selecting profile — plugs in with zero core .py edits, exactly like a transport backend. Rebased as a redesign: main moved the integration surface after the original cut. - New adapters/registry.py: AdapterKind(name, needs_mux, load-thunk) + AdapterBuilder(plain, dev, construct_error), register_adapter (first-wins), builtin loader (GENERIC, OPENCODE_HTTP), the bmad_loop.adapters entry-point scan degrading into _EXTERNAL_ERRORS (never raises), get_adapter_kind (fail loud), known_adapter_kinds, detect_adapters. Deliberately NO lru_cache/ cache_clear and NO configure_*/platform machinery — adapters are built per-run and selected by profile.adapter data (documented in the module). - The dispatch lands in runsetup.make_adapters, not cli._make_adapters: main moved that function, and cli.py:86 now re-exports it. The registry lookup replaces the `if profile.hookless` selection branch there, and the #461 `profiles is not None` path is preserved — the kind is read off the profile the caller pinned, so a digest-gated caller still launches the bytes it validated. cli.py keeps only the re-export seam; no duplicate factory. - config_digest pins `adapter`. It supersedes `hookless` as the field selecting the argv builder, so a driven session rewriting it mid-run now moves the pin the auto-sweep gates on. `hookless` stays (it still reshapes what the opencode builder emits). The docstring's union-completeness rule is re-derived for open-ended external builders: the reads stay a closed set (the adapter's kwargs) but Policy is wider than the hashed launch surface, and the hashed `adapter` bounds the gap to fields of the kind already launched. - profile.py: CLIProfile.adapter (default "generic"); opencode migrated to adapter = "opencode-http". Value-level invariants extracted into _validate_profile, which BOTH routes into the profile map now run — so a bmad_loop.profiles entry point can no longer install a state the TOML parser refuses (the sharp case: an invalid env_fault_patterns regex, which otherwise trades a load-time error for a silent never-match at classification time). Scope is semantic, not type-level; the boundary is stated in the docstring. A malformed `adapter` value funnels into ProfileError per #384 rather than being str()-coerced. - cmd_validate: the adapter.kind / adapter.external / adapter.external-profile block is anchored after main's #461 relay-stat block, and adapter.httpx is re-keyed on the adapter KIND rather than hooklessness — httpx is the opencode family's extra, so a hookless profile driven by another kind no longer FAILs with a remedy that installs the wrong package. Three ids registered in checks.VALIDATE_CHECKS. - `bmad-loop adapters` lists the registered kinds and which profiles select them, naming a dangling kind reference and any failed out-of-tree package. - Docs: the out-of-tree recipe re-integrated into the rewritten (probe-adapter era) adapter-authoring guide, with the herdr reference corrected (it is a transport backend, not an adapter class) and the two `adapter` keys disambiguated; AGENTS.md adapter-axis sentence, docs/README.md, docs/FEATURES.md (seam claim, no-Python claim, command reference), CHANGELOG. - Tests: tests/test_adapter_registry.py covers registration, builtins-first-wins, unknown-kind fail-loud, external degradation/isolation, real dist-info discovery, the (cfg, synthesizes) cache, both directions of the needs_mux gate, construct_error->SystemExit (and that an UNdeclared failure is not swallowed), the cli alias identity, the digest pin, the httpx re-key, and the opencode-http dispatch-unchanged regression pin. Profile entry-point discovery plus a 15-row parity table proving an entry-point profile is held to the parser's invariants. Ablations run for the entry-point validation, the digest pin and the httpx re-key; the stale `hookless`-selects-the-adapter docstrings in test_cli.py / test_runsetup.py are retexted. --- AGENTS.md | 24 +- CHANGELOG.md | 28 + docs/FEATURES.md | 5 +- docs/README.md | 2 +- docs/adapter-authoring-guide.md | 111 +++- src/bmad_loop/adapters/profile.py | 286 +++++++-- src/bmad_loop/adapters/registry.py | 269 +++++++++ src/bmad_loop/checks.py | 3 + src/bmad_loop/cli.py | 126 +++- src/bmad_loop/data/profiles/opencode.toml | 5 + src/bmad_loop/runsetup.py | 151 +++-- tests/test_adapter_registry.py | 701 ++++++++++++++++++++++ tests/test_cli.py | 22 +- tests/test_profile.py | 281 +++++++++ tests/test_runsetup.py | 7 +- 15 files changed, 1870 insertions(+), 151 deletions(-) create mode 100644 src/bmad_loop/adapters/registry.py create mode 100644 tests/test_adapter_registry.py diff --git a/AGENTS.md b/AGENTS.md index 77c07367..891e98b2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,7 +35,7 @@ Things that break silently. Never violate; when in doubt, read the named module' ## Architecture -Two orthogonal seams: **which CLI** (adapter axis: `adapters/base.py` `CodingCLIAdapter`, TOML profiles in `src/bmad_loop/data/profiles/`, user overlay `.bmad-loop/profiles/*.toml` — a new coding CLI is a TOML profile plus `bmad-loop probe-adapter`, not Python) and **which transport** (mux axis: `adapters/multiplexer.py` `TerminalMultiplexer` registry; selection: `BMAD_LOOP_MUX_BACKEND` env > policy `[mux] backend` > platform default > first available match > fallback — full 5-step precedence in [docs/multiplexer-backends.md](docs/multiplexer-backends.md)). +Two orthogonal seams: **which CLI** (adapter axis: `adapters/base.py` `CodingCLIAdapter`, TOML profiles in `src/bmad_loop/data/profiles/`, user overlay `.bmad-loop/profiles/*.toml` — a new coding CLI is a TOML profile plus `bmad-loop probe-adapter`, not Python; a CLI needing its own adapter **class** registers one in `adapters/registry.py` (`register_adapter`, `bmad_loop.adapters` entry point), selected by the profile's `adapter` field) and **which transport** (mux axis: `adapters/multiplexer.py` `TerminalMultiplexer` registry; selection: `BMAD_LOOP_MUX_BACKEND` env > policy `[mux] backend` > platform default > first available match > fallback — full 5-step precedence in [docs/multiplexer-backends.md](docs/multiplexer-backends.md)). | Module | Role | | ------------------------------------------ | ------------------------------------------------------------------------------------------- | @@ -79,16 +79,16 @@ These rules apply to code you are already touching — do not initiate refactors ## Docs index -| Doc | Read when | -| ------------------------------------------------------------------ | -------------------------------------------- | -| [docs/setup-guide.md](docs/setup-guide.md) | installing/initializing a target project | -| [docs/FEATURES.md](docs/FEATURES.md) | any behavior or policy question | -| [docs/tui-guide.md](docs/tui-guide.md) | TUI work | -| [docs/adapter-authoring-guide.md](docs/adapter-authoring-guide.md) | adding/finalizing a coding-CLI profile | -| [docs/multiplexer-backends.md](docs/multiplexer-backends.md) | mux backend selection/porting | -| [docs/plugin-authoring-guide.md](docs/plugin-authoring-guide.md) | plugin work (incl. game-engine + TEA guides) | -| [docs/porting-to-a-new-os.md](docs/porting-to-a-new-os.md) | OS seams | -| [docs/testing.md](docs/testing.md) | writing/placing tests, guards, flake policy | -| [docs/ROADMAP.md](docs/ROADMAP.md) | planned vs deliberately-deferred work | +| Doc | Read when | +| ------------------------------------------------------------------ | ------------------------------------------------------- | +| [docs/setup-guide.md](docs/setup-guide.md) | installing/initializing a target project | +| [docs/FEATURES.md](docs/FEATURES.md) | any behavior or policy question | +| [docs/tui-guide.md](docs/tui-guide.md) | TUI work | +| [docs/adapter-authoring-guide.md](docs/adapter-authoring-guide.md) | adding/finalizing a coding-CLI profile or adapter class | +| [docs/multiplexer-backends.md](docs/multiplexer-backends.md) | mux backend selection/porting | +| [docs/plugin-authoring-guide.md](docs/plugin-authoring-guide.md) | plugin work (incl. game-engine + TEA guides) | +| [docs/porting-to-a-new-os.md](docs/porting-to-a-new-os.md) | OS seams | +| [docs/testing.md](docs/testing.md) | writing/placing tests, guards, flake policy | +| [docs/ROADMAP.md](docs/ROADMAP.md) | planned vs deliberately-deferred work | Full list: [docs/README.md](docs/README.md). diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a926602..58340a99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,17 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Added +- **Coding-CLI adapter registry: a new adapter class ships out-of-tree (#226).** The transport axis + has long been extensible out-of-tree; the CLI axis had no equivalent, so a CLI needing its own + adapter _class_ forced a name-branch in the run bootstrap. A profile's new `adapter` field names a + kind resolved against `adapters/registry.py`, and a co-installed package registers its own kind + via the `bmad_loop.adapters` entry point and the profile that selects it via `bmad_loop.profiles` + — with no core edit. `bmad-loop adapters` lists the registered kinds and which profiles select + them; `validate` gains `adapter.kind` (checked against the live registry, never a hardcoded set) + plus `adapter.external` / `adapter.external-profile` warnings. A broken third-party package + degrades to a recorded, surfaced reason and can never break selection. `opencode-http` is migrated + to a registered builtin behind a dispatch-unchanged regression pin. + - **docs/testing.md: the formal testing strategy.** Layer taxonomy and placement rules, fixture and ablation doctrine, the quality-guard inventory, zero-token and flake policy, and a tracked gap register (#545–#549); AGENTS.md, docs/README.md and CONTRIBUTING.md link here. @@ -123,6 +134,23 @@ whose seams had diverged enough that several ports needed a different fix, and t ### Changed +- **The mid-run config pin covers the adapter kind (#461).** `adapter` selects which argv builder + runs at all, so it joins the `config_digest` launch payload — a driven session rewriting it now + moves the pin the auto-triggered child sweep gates on, instead of swapping the whole launch shape + underneath it. The digest resolves the kind from the profile bytes it was handed, not a second + read. + +- **`validate`'s httpx check keys on the adapter kind, not hooklessness.** `httpx` is the + `opencode-http` family's optional extra; with the transport and driving class now separate axes, a + hookless profile driven by another kind no longer FAILs with a remedy that installs the wrong + package. + +- **Profiles from a `bmad_loop.profiles` entry point are validated like TOML ones.** Both routes into + the profile map now share one invariant set (hook dialect, path containment, `env_fault_patterns` + compilation, …), so a package can no longer install a profile state the parser would refuse — an + invalid env-fault regex used to trade a load-time error for a silent never-match at classification + time. A malformed `adapter` value funnels into `ProfileError` rather than being `str()`-coerced. + - **Lint the workflows, and smoke-test the built package.** `trunk check` now runs `actionlint` and `zizmor` over `.github/workflows/`, and a `build` CI job builds the sdist + wheel, runs `bmad-loop --version` from the installed wheel, and checks that wheel carries every data file diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 87acf526..087e2df6 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -139,13 +139,13 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Multi-CLI / multi-agent support -- Generic adapter drives any CLI fitting the injection + hook-signal transport; CLI specifics live in declarative TOML profiles. Two independent axes: the **CLI** (`CodingCLIAdapter` + profile) and the **terminal transport** (`TerminalMultiplexer`) — tmux ships bundled (with an experimental native-Windows `psmux` backend alongside it), and external backends (e.g. the [herdr adapter](https://github.com/pbean/bmad-loop-adapter-herdr)) co-install as packages that self-register ([how](multiplexer-backends.md)), behind a pluggable seam that lets a new backend slot in without touching the engine (see the [adapter authoring guide](adapter-authoring-guide.md#two-axes-cli-vs-transport)). +- Generic adapter drives any CLI fitting the injection + hook-signal transport; CLI specifics live in declarative TOML profiles. Two independent axes: the **CLI** (`CodingCLIAdapter` + profile) and the **terminal transport** (`TerminalMultiplexer`) — tmux ships bundled (with an experimental native-Windows `psmux` backend alongside it), and external backends (e.g. the [herdr adapter](https://github.com/pbean/bmad-loop-adapter-herdr)) co-install as packages that self-register ([how](multiplexer-backends.md)), behind a pluggable seam that lets a new backend slot in without touching the engine. The CLI axis has the same seam: a new adapter **class** registers via `register_adapter` and arrives through the `bmad_loop.adapters` entry-point group, with its selecting profile through `bmad_loop.profiles` — so an out-of-tree adapter family needs no core edit either (see the [adapter authoring guide](adapter-authoring-guide.md#two-axes-cli-vs-transport)). - The OS is abstracted by a **registry of seams**, each selecting an implementation by platform (with a test-override env var) and extended by a single registration line: the terminal multiplexer (`register_multiplexer`, with availability-aware selection: env var → persisted `[mux] backend` via `bmad-loop mux set` → platform default → first available platform match), the process-lifecycle `ProcessHost` (`register_process_host` — `terminate`/`force_kill`/`is_alive`/`identity`), and the hook interpreter (`ProcessHost.hook_interpreter()`); `bmad-loop validate` runs a platform preflight over them. Porting to a new OS is new files + registrations, no core edits — see [Porting bmad-loop to a new OS](porting-to-a-new-os.md). - Supported, E2E-verified: `claude` (reference), `codex` (≥ 0.139), `gemini` (≥ 0.46), `copilot` (GitHub Copilot CLI ≥ 2026-02 — the `copilot` binary, not the VS Code extension; `agentStop` turn-end, `-i` interactive launch, `--allow-all-tools`; pin a capable model — the free GPT-5 mini default is unreliable for multi-step skills). - Supported, E2E-verified over HTTP/SSE (no tmux window): `opencode` (OpenCode ≥ 1.18, profile `opencode-http`, alias `opencode`) — one headless `opencode serve` per session, SSE `session.idle` completion with an HTTP poll fallback, per-session server password, token usage read back over the API. Hookless (`[hooks] dialect = "none"`, no hook registration). With no pane to replay, the run logs split three ways: a curated readable transcript in `logs/.log` (agent/user prose, tool calls, slash commands, file edits, permission asks/replies, errors), the server's own stdout in `.server.out`, and a structured SSE trace in `.sse.jsonl`. Install the extra (`pip install 'bmad-loop[opencode]'`), auth once globally (`opencode auth login`), and set `model` as `provider/model`; the Unity plugin's window guards don't apply (there is no window). - Experimental, `isolation = "none"` only: `antigravity` (Google's `agy` ≥ 1.1.3) — `-i` interactive launch, `Stop` turn-end hook (flat handler in `.agents/hooks.json`, no SessionStart/SessionEnd), `--dangerously-skip-permissions` for unattended runs; `usage_parser = "none"` permanently — agy's transcript exposes no usage data (tokens live only in an internal SQLite/protobuf store). `agy` gates each workspace on an exact-path `trustedWorkspaces` entry and blocks on an interactive trust dialog, which `--dangerously-skip-permissions` does not bypass — so worktree isolation hangs ([#169](https://github.com/bmad-code-org/bmad-loop/issues/169)). Verify against your `agy` build with `probe-adapter antigravity`. - Per-stage CLI/model overrides: run dev on one CLI/model, review on another (`[adapter.dev]`, `[adapter.review]`, `[adapter.triage]`). -- Add a CLI without touching Python: drop a TOML profile in `.bmad-loop/profiles/.toml` (binary, prompt template, bypass flags, hook dialect, native→canonical event map). +- Add a CLI without touching Python: drop a TOML profile in `.bmad-loop/profiles/.toml` (binary, prompt template, bypass flags, hook dialect, native→canonical event map). A CLI that needs its own adapter _class_ still needs Python — but not a core edit: the profile's `adapter` field names a kind resolved against the registry, which a co-installed package extends. - `bmad-loop probe-adapter` collects + sanitizes the data needed to finalize/add a profile (hook payload shape, transcript location/format, token schema): a zero-launch scan by default, opt-in `--probe` for live capture. See the [adapter authoring guide](adapter-authoring-guide.md). ### Budgeting & cost tracking @@ -194,6 +194,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - `bmad-loop init` — install skills, hooks, policy, gitignore. - `bmad-loop validate` — preflight all prerequisites. `--json` instead emits a stable machine-readable document (schema-versioned; the `ok` verdict, the queue `mode`/`spec_folder`, per-severity `counts`, and every check as a flat emission-ordered finding with a stable `check` id, `severity`, human `message` and structured `detail`) per the [contract below](#machine-readable-output---json); a failing check still emits the whole document, at exit 1 — the nonzero code is the verdict, not a failure to produce one. - `bmad-loop mux` — list registered terminal-multiplexer backends (platform · availability · version · which is selected and why; a backend whose binary is present but crashed the version probe gets a `warning:` on stderr carrying the probe's own failure, since the `-` in the VERSION column cannot tell that apart from a binary that reports no version); `mux set ` persists a machine-scoped choice into policy.toml (`--clear` reverts to auto, `--force` allows a name only registered on the target machine). Bundled backend: `tmux`; external backends (e.g. the herdr adapter) register via the `bmad_loop.mux_backends` entry-point group — see [Terminal multiplexer backends](multiplexer-backends.md). +- `bmad-loop adapters` — list registered coding-CLI adapter **kinds** (name · builtin/external · whether the family drives a multiplexer · which profiles select it), the CLI axis's counterpart to `mux`. Unlike `mux` there is no global choice to persist: a kind is selected per profile by its `adapter` field. A profile referencing an unregistered kind, and any out-of-tree adapter/profile package that failed to load, get a `warning:` on stderr; `validate` reports the same as `adapter.kind` / `adapter.external` / `adapter.external-profile`. - `bmad-loop run` — drive the dev → review → verify → commit loop. - `bmad-loop sweep` — triage + execute open deferred-work entries. - `bmad-loop resume ` — continue a paused/interrupted run. diff --git a/docs/README.md b/docs/README.md index d07f1fd9..1cd0646e 100644 --- a/docs/README.md +++ b/docs/README.md @@ -12,7 +12,7 @@ guides below go deeper, roughly in the order you'll need them. ## Extending bmad-loop -- **[Finalizing a CLI adapter profile](adapter-authoring-guide.md)** — using `bmad-loop probe-adapter` to collect + sanitize the hook payload shape, transcript location, and token schema a new CLI profile needs. +- **[Authoring CLI adapters & profiles](adapter-authoring-guide.md)** — using `bmad-loop probe-adapter` to collect + sanitize the hook payload shape, transcript location, and token schema a new CLI profile needs, plus the `CodingCLIAdapter` ABC and how an adapter class (and the profile selecting it) ships out-of-tree. - **[Writing a bmad-loop plugin](plugin-authoring-guide.md)** — the plugin system: `plugin.toml` manifest, hooks, lifecycle stages, settings, the trust model, and workflow injection, with a worked walkthrough. - **[Writing a Game Engine plugin](game-engine-plugin-guide.md)** — the game-engine layer (built on the plugin system): driving a live engine Editor, the `editor_mode` ↔ `[scm] isolation` coupling, a minimal Godot example. - **[Writing a plugin for a specific Editor MCP](game-engine-mcp-guide.md)** — Editor-MCP specifics for the bundled Unity plugin: IvanMurzak vs CoplayDev, readiness probes, `per_worktree` isolation, and the full `BMAD_LOOP_*` env-var reference. diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 12e9ee2f..8b72b3b9 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -14,7 +14,10 @@ bmad-loop a new CLI: - **The advanced case — a new adapter class.** If the CLI does _not_ fit that transport (e.g. an HTTP/SSE service), see [Writing a new adapter class](#writing-a-new-adapter-class) for the - `CodingCLIAdapter` ABC. + `CodingCLIAdapter` ABC — and + [Shipping a new adapter class out-of-tree](#shipping-a-new-adapter-class-out-of-tree) + to register it (and the profile that selects it) from a co-installed package + with **zero core edits**, the same way a transport backend ships out-of-tree. ## Two axes: CLI vs transport @@ -22,7 +25,10 @@ These are independent and abstracted separately: - **CLI axis** — `CodingCLIAdapter` (`adapters/base.py`): _which_ binary to launch, how the prompt is rendered, the hook dialect, where the transcript lives. The - generic adapter + a TOML profile cover this; the rest of this guide is about it. + generic adapter + a TOML profile cover the common case; a CLI needing its own + adapter class registers one via `register_adapter(...)` (`adapters/registry.py`), + selected by the profile's `adapter` field and shippable out-of-tree just like a + transport backend. Most of this guide is about this axis. - **Transport axis** — `TerminalMultiplexer` (`adapters/multiplexer.py`): how sessions, windows, and panes are created, observed, and torn down. The generic adapter never shells out itself — it goes through `self.mux`, obtained from @@ -400,6 +406,7 @@ resolves to `claude`. | `name` | ✅ | — | Profile id, also the `--cli` value and override key. | | `binary` | ✅ | — | Executable to launch (resolved on `PATH`). | | `[hooks]` | ✅ | — | The `HookSpec` table (see below). | +| `adapter` | | `generic` | Which adapter **class** drives this CLI — a key resolved against the [adapter registry](#shipping-a-new-adapter-class-out-of-tree), not a fixed enum. `generic` = the bundled tmux + hook-signal adapter; `opencode-http` = the bundled HTTP/SSE adapter; an out-of-tree package registers its own. Membership is checked against the **live** registry (at run start, and by `bmad-loop validate`'s `adapter.kind`), never at parse time — so an unknown kind is a clear config error rather than a schema change. Independent of `hooks.dialect = "none"`: hooklessness is about the transport, this is about the driving class. | | `skill_tree` | | `.claude/skills` | Project-relative tree this CLI reads skills from (`.agents/skills` for codex/gemini); `bmad-loop init` installs the `bmad-loop-*` skills here. Must be relative. | | `prompt_template` | | `{prompt}` | How the canonical `/skill args` prompt is rendered. Placeholders: `{prompt}` (whole string), `{skill}` (leading slash-command name, no `/`), `{args}` (the remainder). | | `launch_args` | | `()` | Extra argv passed at launch, e.g. `["-i"]` to stay interactive (gemini/copilot). | @@ -600,8 +607,108 @@ adapter against a scripted stdlib FakeOpencode (no binary, no network beyond smoke-checks the pinned HTTP contract against a real local binary — skipped when absent, zero tokens spent. +### Shipping a new adapter class out-of-tree + +How does a new adapter class ever get _selected_ when it lives in its own package? +Which class drives a CLI is data — the profile's `adapter` field, resolved against +the adapter registry +([`adapters/registry.py`](../src/bmad_loop/adapters/registry.py)) — so a +co-installed package registers its own kind with no edit to any core `.py`, exactly +as a transport backend ships via `bmad_loop.mux_backends` +([the transport contract](#the-transport-contract-for-a-backend-author)). + +Advertise **two entry points** in the package's `pyproject.toml`: + +- **`bmad_loop.adapters`** → a module whose import registers the kind. Core scans + the group and imports each advertised module (after the builtins, so a bundled + name always keeps first registration) before it resolves any adapter: + + ```python + # acme_adapter/__init__.py + from bmad_loop.adapters.registry import AdapterBuilder, register_adapter + + def _load(): # lazy: imported only when a run builds this kind, + from .acme import AcmeAdapter, AcmeDevAdapter # so an optional dep stays unpaid + return AdapterBuilder( + plain=AcmeAdapter, # the plain class + dev=AcmeDevAdapter, # the _DevSynthesisMixin-composed dev/review class + construct_error=(), # exception type(s) __init__ may raise; () = none + ) + + register_adapter("acme", needs_mux=True, load=_load) # needs_mux: does it drive a multiplexer? + ``` + + ```toml + # pyproject.toml + [project.entry-points."bmad_loop.adapters"] + acme = "acme_adapter" + ``` + +- **`bmad_loop.profiles`** → a callable returning `CLIProfile`s (or an iterable of + them), so the profile that _selects_ your kind ships with it — no project TOML + required. Precedence is packaged < entry-point < project, so a project TOML can + still override it: + + ```python + # acme_adapter/__init__.py (same package) + from bmad_loop.adapters.profile import CLIProfile, HookSpec + + def profiles(): + return [CLIProfile(name="acme", binary="acme", adapter="acme", + hooks=HookSpec("none", "", {}))] + ``` + + ```toml + [project.entry-points."bmad_loop.profiles"] + acme = "acme_adapter:profiles" + ``` + +The two entry-point **values** differ on purpose: the adapter group's is a bare +module path (core only imports it — registration is the import's side effect, and +the entry-point name is just a diagnostic label), while the profile group's names a +provider object core actually calls. Installing the package into bmad-loop's +environment is the entire setup — e.g. +`uv tool install bmad-loop --with ` — with no core edit and no config +step. + +Once co-installed, `bmad-loop adapters` lists the kind and the profiles that select +it, `bmad-loop validate` checks the reference (an `adapter.kind` finding, resolved +against the live registry — never a hardcoded set), and a run whose policy +`[adapter] name` points at a profile carrying `adapter = "acme"` selects it. Note +those are two different keys: policy's `[adapter] name` picks the **profile**; the +profile's own `adapter` field picks the **kind**. + +A broken package can never break selection: the failure is recorded and reported by +`bmad-loop adapters` (a `warning: external adapter '' failed to load: ` +line, and the same for a failed profile provider) and by the `validate` preflight, +and selection proceeds without it. There is no out-of-tree adapter-**class** package +to copy yet; the packaging pattern is identical to the transport axis's +[bmad-loop-adapter-herdr](https://github.com/pbean/bmad-loop-adapter-herdr), which +registers a `TerminalMultiplexer` rather than an adapter class. + +Two seam facts worth internalizing: + +- **`needs_mux`** gates whether the run bootstrap resolves and usability-checks the + shared terminal multiplexer for your family. A tmux/hook family sets it `True`; a + self-hosted HTTP/SSE family (like opencode) sets it `False` and is never handed a + `mux`. +- **The `dev` / `plain` split is a pipeline concept, not a per-family branch.** When + a dev/review session runs the dev primitive (which writes no `result.json`), the + bootstrap builds the `dev` variant — the `_DevSynthesisMixin`-composed class — and + threads the project `paths` into it so it can synthesize the result from the spec + on disk; every other role builds `plain`. Both variants of a family share the + `(*args, paths, **kwargs)` dev `__init__` contract, so honoring it is all an + out-of-tree class must do to slot into that machinery. + +An entry-point profile is held to the same invariants a TOML profile is (hook +dialect, path containment, `env_fault_patterns` compilation, …): it is validated on +arrival, so a provider that ships an invalid profile is reported rather than +half-installed. + ### References +- [`adapters/registry.py`](../src/bmad_loop/adapters/registry.py) — the adapter-kind + registry and the two entry-point scans described above. - [`adapters/opencode_http.py`](../src/bmad_loop/adapters/opencode_http.py) — the worked example above: a real non-tmux (HTTP/SSE) transport. - [`adapters/mock.py`](../src/bmad_loop/adapters/mock.py) — the test-only reference diff --git a/src/bmad_loop/adapters/profile.py b/src/bmad_loop/adapters/profile.py index f4c8ed62..0fefa517 100644 --- a/src/bmad_loop/adapters/profile.py +++ b/src/bmad_loop/adapters/profile.py @@ -9,10 +9,29 @@ project-local TOML files in /.bmad-loop/profiles/*.toml overlay them (same name overrides, new names extend) — adding a CLI that clones an existing hook dialect needs no Python. + +An out-of-tree package advertises additional profiles under the +``bmad_loop.profiles`` entry-point group (:func:`load_profiles` scans it): the +companion to the ``bmad_loop.adapters`` registry (:mod:`~.registry`), so a +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. + +Which adapter *class* drives a profile is the ``adapter`` field, resolved against +the :mod:`~.registry` — it is read here but intentionally **not** checked against +the set of registered kinds at parse time (an unknown kind is caught at +construction and by ``validate``, against the live registry, never a hardcoded +set). Its *shape* is still enforced here, like every other field. + +Both routes into the profile map — the TOML parser and the entry-point scan — +converge on :func:`_validate_profile`, so a Python package cannot install a +profile state a TOML author would have been refused. """ from __future__ import annotations +import importlib.metadata import tomllib from dataclasses import dataclass, field from importlib import resources @@ -69,6 +88,15 @@ class CLIProfile: name: str binary: str hooks: HookSpec + # Which adapter *class* drives this CLI — a key resolved against the adapter + # registry (adapters/registry.py), not a hardcoded enum. "generic" = the + # bundled tmux-injection + hook-signal adapter; "opencode-http" = the bundled + # HTTP/SSE adapter; an out-of-tree package registers its own. Membership is NOT + # checked at parse time: an unknown kind fails loud at construction and as a + # `validate` finding, both against the live registry. A hookless HTTP profile + # (hooks.dialect = "none") MUST set this to its HTTP adapter kind — the + # transport (hookless) and the driving class are now decoupled axes. + adapter: str = "generic" # project-relative tree this CLI reads skills from, e.g. ".claude/skills" # (claude) or ".agents/skills" (codex/gemini); `bmad-loop init` installs the # bundled bmad-loop-* skills here. @@ -139,105 +167,164 @@ def render_prompt(self, prompt: str) -> str: return self.prompt_template.format(prompt=prompt, skill=skill, args=args) -def _parse_profile(doc: dict, source: str) -> CLIProfile: +def _validate_profile(profile: CLIProfile, source: str) -> None: + """Enforce every value-level invariant a ``CLIProfile`` must satisfy, whatever + built it. + + Two routes reach the profile map and only one has a parser in front of it: + :func:`_parse_profile` coerces a TOML document, while a ``bmad_loop.profiles`` + entry point hands over an already-constructed instance. Holding the invariants + in one function is what stops those routes drifting — the failure it closes is + a Python package installing a state the TOML parser would have refused, and + the sharpest instance is an ``env_fault_patterns`` entry that is not a valid + regex: unchecked, it trades a compile error at LOAD time for one at MATCH + time, inside a session's env-fault classification, where the caller degrades + rather than raises and the pattern silently never fires. + + Scope is semantic, not type-level. A package that constructs a ``CLIProfile`` + with the wrong runtime type in a field (a list where a ``str`` belongs) is a + bug this deliberately does not chase: reaching here already ran that package's + code in-process, so this is not a security boundary and hardening it as one + would invite it being trusted as one. What it does catch is the well-typed and + wrong profile — exactly what a TOML author is told about at parse time.""" + def fail(msg: str) -> ProfileError: return ProfileError(f"profile {source}: {msg}") - def str_list(key: str) -> tuple[str, ...]: - # TOML arrays parse as list; reject a bare string (which would iterate to - # per-character entries) or a scalar (a raw TypeError) with a friendly error. - raw = doc.get(key, []) - if not isinstance(raw, list) or not all(isinstance(x, str) for x in raw): - raise fail(f"{key} must be a list of strings") - return tuple(raw) - - name = str(doc.get("name", "")).strip() - binary = str(doc.get("binary", "")).strip() - if not name or not binary: + if not profile.name.strip() or not profile.binary.strip(): raise fail("'name' and 'binary' are required") - hooks_d = doc.get("hooks") - if not isinstance(hooks_d, dict): - raise fail("missing [hooks] table") - dialect = str(hooks_d.get("dialect", "")) - if dialect not in HOOK_DIALECTS: - raise fail(f"hooks.dialect must be one of {sorted(HOOK_DIALECTS)}: got {dialect!r}") - if dialect == "none": + hooks = profile.hooks + if hooks.dialect not in HOOK_DIALECTS: + raise fail(f"hooks.dialect must be one of {sorted(HOOK_DIALECTS)}: got {hooks.dialect!r}") + if hooks.dialect == "none": # hookless: nothing is ever registered, so a config_path or events map # is a contradiction — reject rather than silently ignore. - if hooks_d.get("config_path") or hooks_d.get("events"): + if hooks.config_path or hooks.events: raise fail('hookless profiles (dialect = "none") must not set hooks.config_path/events') - config_path = "" - events: dict[str, str] = {} else: - config_path = str(hooks_d.get("config_path", "")) if ( - names_tree_root(config_path) - or is_absolute_path(config_path) - or has_parent_ref(config_path) + names_tree_root(hooks.config_path) + or is_absolute_path(hooks.config_path) + or has_parent_ref(hooks.config_path) ): + # `names_tree_root("")` is True, so this arm also carries the + # "a real dialect must name a config_path at all" case. raise fail("hooks.config_path must be a project-relative path") - events_d = hooks_d.get("events") - if not isinstance(events_d, dict) or not events_d: + if not hooks.events: raise fail("hooks.events must map native event names to canonical ones") - events = {str(k): str(v) for k, v in events_d.items()} - bad = sorted(set(events.values()) - CANONICAL_EVENTS) + bad = sorted(set(hooks.events.values()) - CANONICAL_EVENTS) if bad: raise fail( f"hooks.events values must be canonical {sorted(CANONICAL_EVENTS)}: got {bad}" ) - usage_parser = str(doc.get("usage_parser", "none")) - if usage_parser not in USAGE_PARSERS: - raise fail(f"usage_parser must be one of {sorted(USAGE_PARSERS)}: got {usage_parser!r}") - - usage_grace_s = float(doc.get("usage_grace_s", 0.0)) - if usage_grace_s < 0: - raise fail(f"usage_grace_s must be >= 0: got {usage_grace_s}") - - raw_nudges = doc.get("stop_without_result_nudges") - stop_nudges = None if raw_nudges is None else int(raw_nudges) - if stop_nudges is not None and stop_nudges < 0: - raise fail(f"stop_without_result_nudges must be >= 0: got {stop_nudges}") - - skill_tree = str(doc.get("skill_tree", ".claude/skills")) - if names_tree_root(skill_tree) or is_absolute_path(skill_tree) or has_parent_ref(skill_tree): + # Shape only — membership against the registered kinds is deliberately NOT + # checked here (see the module docstring): that set is open-ended and lives in + # adapters/registry.py, which importing from here would make a cycle. + if not profile.adapter: + raise fail("adapter must be a non-empty string naming an adapter kind") + + if profile.usage_parser not in USAGE_PARSERS: + raise fail( + f"usage_parser must be one of {sorted(USAGE_PARSERS)}: got {profile.usage_parser!r}" + ) + + if profile.usage_grace_s < 0: + raise fail(f"usage_grace_s must be >= 0: got {profile.usage_grace_s}") + + nudges = profile.stop_without_result_nudges + if nudges is not None and nudges < 0: + raise fail(f"stop_without_result_nudges must be >= 0: got {nudges}") + + if ( + names_tree_root(profile.skill_tree) + or is_absolute_path(profile.skill_tree) + or has_parent_ref(profile.skill_tree) + ): raise fail("skill_tree must be a project-relative path") - seed_files = str_list("seed_files") # `names_tree_root` subsumes the emptiness check it replaced. These entries feed # provision_worktree's seed loop, where any spelling of the root ("", ".", "./", # ".\") resolves src to the repo root and dst to the worktree — both pass the # loop's containment checks, so the whole repo is copied in. - for seed in seed_files: + for seed in profile.seed_files: if names_tree_root(seed) or is_absolute_path(seed) or has_parent_ref(seed): raise fail(f"seed_files entries must be project-relative paths: got {seed!r}") - env_fault_patterns = str_list("env_fault_patterns") - for pattern in env_fault_patterns: + for pattern in profile.env_fault_patterns: try: regex.compile(pattern) # same engine the adapter matches with (timeout-guarded) except regex.error as e: raise fail(f"env_fault_patterns entry is not a valid regex: {pattern!r} ({e})") from e - return CLIProfile( - name=name, - binary=binary, - hooks=HookSpec(dialect=dialect, config_path=config_path, events=events), - skill_tree=skill_tree, + +def _parse_profile(doc: dict, source: str) -> CLIProfile: + """Coerce a TOML document into a :class:`CLIProfile`. + + SHAPE only: the container and element types a TOML document can get wrong and + a constructed dataclass cannot. Every value-level invariant lives in + :func:`_validate_profile`, called on the result — so the entry-point route, + which has no document to coerce, enforces exactly the same set.""" + + def fail(msg: str) -> ProfileError: + return ProfileError(f"profile {source}: {msg}") + + def str_list(key: str) -> tuple[str, ...]: + # TOML arrays parse as list; reject a bare string (which would iterate to + # per-character entries) or a scalar (a raw TypeError) with a friendly error. + raw = doc.get(key, []) + if not isinstance(raw, list) or not all(isinstance(x, str) for x in raw): + raise fail(f"{key} must be a list of strings") + return tuple(raw) + + hooks_d = doc.get("hooks") + if not isinstance(hooks_d, dict): + raise fail("missing [hooks] table") + events_d = hooks_d.get("events", {}) + if not isinstance(events_d, dict): + raise fail("hooks.events must map native event names to canonical ones") + + # A dedicated shape check rather than the `str()` coercion the neighbouring + # scalars get, because `adapter` has no parse-time membership test to land in + # afterwards: `str(["x"])` would coerce a TOML array to the literal `"['x']"` + # and carry it all the way to `get_adapter_kind`, which would then name that + # nonsense as the unknown kind. #384's rule — a malformed value funnels into + # ProfileError at the boundary, never a silent coercion. + raw_adapter = doc.get("adapter", "generic") + if not isinstance(raw_adapter, str): + raise fail(f"adapter must be a string: got {type(raw_adapter).__name__}") + + profile = CLIProfile( + name=str(doc.get("name", "")).strip(), + binary=str(doc.get("binary", "")).strip(), + hooks=HookSpec( + dialect=str(hooks_d.get("dialect", "")), + config_path=str(hooks_d.get("config_path", "")), + events={str(k): str(v) for k, v in events_d.items()}, + ), + adapter=raw_adapter.strip(), + skill_tree=str(doc.get("skill_tree", ".claude/skills")), prompt_template=str(doc.get("prompt_template", "{prompt}")), launch_args=str_list("launch_args"), bypass_args=str_list("bypass_args"), model_flag=str(doc.get("model_flag", "--model")), env={str(k): str(v) for k, v in doc.get("env", {}).items()}, - usage_parser=usage_parser, - usage_grace_s=usage_grace_s, - stop_without_result_nudges=stop_nudges, + usage_parser=str(doc.get("usage_parser", "none")), + # `float()`/`int()` are the raw coercions `_load_toml`'s CONVERSION_FAULTS + # funnel exists for: they answer OverflowError for `inf` and for an integer + # too large to be a float, both of which are legal TOML. + usage_grace_s=float(doc.get("usage_grace_s", 0.0)), + stop_without_result_nudges=( + None if (raw := doc.get("stop_without_result_nudges")) is None else int(raw) + ), subagent_stop_without_transcript=bool(doc.get("subagent_stop_without_transcript", False)), first_run_note=str(doc.get("first_run_note", "")), - seed_files=seed_files, - env_fault_patterns=env_fault_patterns, + seed_files=str_list("seed_files"), + env_fault_patterns=str_list("env_fault_patterns"), ) + _validate_profile(profile, source) + return profile def _load_toml(text: str, source: str) -> CLIProfile: @@ -260,14 +347,93 @@ def _load_toml(text: str, source: str) -> CLIProfile: raise ProfileError(f"profile {source}: malformed field value: {e}") from e +# The entry-point group an out-of-tree package advertises extra profiles under — +# the companion to adapters/registry.py's `bmad_loop.adapters` group. Each entry +# point loads to a provider: a callable returning an iterable of CLIProfile (or an +# iterable directly), e.g. one built from the package's own bundled TOML. Scanned +# once per process (a third-party import failure is not transient); the resulting +# profiles are process-global (project-independent), so only the project overlay +# is re-read per load_profiles call. A broken entry point is recorded, not raised. +PROFILES_GROUP = "bmad_loop.profiles" +_EXTERNALS_LOADED = False +_EXTERNAL_PROFILES: dict[str, CLIProfile] = {} +_PROFILE_LOAD_ERRORS: dict[str, str] = {} + + +def _coerce_profiles(produced: object, ep_name: str) -> list[CLIProfile]: + """A provider may return a callable's result or an iterable directly; either + way it must yield CLIProfile instances that satisfy the same invariants a TOML + profile does. Anything else is the package's bug — reported (per + :func:`external_profile_errors`), never trusted into the map. + + The :func:`_validate_profile` call is the point of this function: without it a + Python provider is the one route into the profile map with no parser in front + of it, and it could install a state ``_parse_profile`` would refuse.""" + try: + items = list(produced) # pyright: ignore[reportArgumentType] — TypeError is the check + except TypeError as exc: + raise ProfileError( + f"{ep_name}: profile provider must return an iterable of CLIProfile" + ) from exc + for item in items: + if not isinstance(item, CLIProfile): + raise ProfileError( + f"{ep_name}: profile provider yielded {type(item).__name__}, not CLIProfile" + ) + _validate_profile(item, f"entry point {ep_name}") + return items + + +def _load_external_profiles() -> dict[str, CLIProfile]: + """Import every ``bmad_loop.profiles`` entry point and collect the profiles it + provides, first-registration-wins on a name collision. Scan-once; failures are + recorded in ``_PROFILE_LOAD_ERRORS`` (surfaced via + :func:`external_profile_errors`), never raised — a broken adapter package must + not break profile loading for everything else. + + A provider is rejected WHOLE: one invalid profile in the returned batch drops + the batch, because ``_coerce_profiles`` raises before any of them is recorded. + Deliberate — a provider is one package's declaration, and half-installing it + would leave an operator with a profile set no error message accounts for.""" + global _EXTERNALS_LOADED + if _EXTERNALS_LOADED: + return _EXTERNAL_PROFILES + _EXTERNALS_LOADED = True + try: + eps = importlib.metadata.entry_points(group=PROFILES_GROUP) + except Exception as exc: # noqa: BLE001 — diagnostics path, never crash loading + _PROFILE_LOAD_ERRORS[""] = f"{type(exc).__name__}: {exc}" + return _EXTERNAL_PROFILES + for ep in eps: + try: + provider = ep.load() + produced = provider() if callable(provider) else provider + 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}" + 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.""" + return dict(_PROFILE_LOAD_ERRORS) + + def load_profiles(project: Path | None = None) -> dict[str, CLIProfile]: - """Packaged built-ins, overlaid by /.bmad-loop/profiles/*.toml.""" + """Packaged built-ins, overlaid by ``bmad_loop.profiles`` entry-point + profiles, overlaid by /.bmad-loop/profiles/*.toml. + + Precedence is packaged < entry-point < project: a co-installed adapter package + extends (or overrides) the bundled set, and a project TOML always wins.""" profiles: dict[str, CLIProfile] = {} packaged = resources.files("bmad_loop.data").joinpath("profiles") for entry in sorted(packaged.iterdir(), key=lambda e: e.name): if entry.name.endswith(".toml"): profile = _load_toml(entry.read_text(encoding="utf-8"), entry.name) profiles[profile.name] = profile + profiles.update(_load_external_profiles()) if project is not None: user_dir = project / USER_PROFILES_REL if user_dir.is_dir(): diff --git a/src/bmad_loop/adapters/registry.py b/src/bmad_loop/adapters/registry.py new file mode 100644 index 00000000..e05a3707 --- /dev/null +++ b/src/bmad_loop/adapters/registry.py @@ -0,0 +1,269 @@ +"""Coding-CLI adapter registry — the out-of-tree extension seam for the CLI axis. + +The transport axis (:mod:`~.multiplexer`) has long been extensible out-of-tree: +a backend registers through ``register_multiplexer`` and a co-installed package +is discovered via the ``bmad_loop.mux_backends`` entry-point group. This module +is the same seam for the *other* axis — which adapter **class** drives a coding +CLI. A CLI that fits the tmux-injection + hook-signal transport still needs no +Python at all (drop a TOML :class:`~.profile.CLIProfile` and run ``bmad-loop +probe-adapter``); this registry is for a CLI that needs a whole new adapter +subclass (the shipped example is the HTTP/SSE ``opencode-http`` adapter, which no +tmux profile can host). + +An adapter *kind* is selected by **data**: ``profile.adapter`` names the kind, and +:func:`get_adapter_kind` resolves it. Two registration-time dataclasses carry the +family: + +- :class:`AdapterKind` — ``name`` + ``needs_mux`` (does the family drive a + terminal multiplexer?) + ``load``, a lazy thunk returning the builder. The thunk + is why registration, validation (``known_adapter_kinds``) and listing + (``detect_adapters``) never import a heavy adapter module — nor an optional + dependency like ``httpx``, which only the opencode family pulls in at + construction. +- :class:`AdapterBuilder` — the ``plain`` class, the ``_DevSynthesisMixin``-composed + ``dev`` class (both share the ``(*args, paths, **kwargs)`` dev ``__init__``), and + the family's construction-failure exception type(s) (``()`` = none; + ``(OpencodeServerError,)`` for the HTTP family, which fails loud when its server + can't spawn). ``runsetup.make_adapters`` converts a raised ``construct_error`` + into a ``SystemExit``. + +Bundled kinds register from :func:`_load_builtin_adapters` (:data:`GENERIC`, +:data:`OPENCODE_HTTP`); out-of-tree kinds arrive at import time, triggered by the +``bmad_loop.adapters`` entry-point scan in :func:`_load_external_adapters` — so a +pip/uv co-installed adapter package is selectable with no config step. Builtins +load first, so an external can never shadow a bundled name. A broken third-party +distribution degrades to a recorded, surfaced reason +(:func:`external_adapter_errors`) and can never break selection. + +**Two deliberate asymmetries versus the multiplexer seam** (this is not a +copy-paste omission): + +- *No process-wide cache / no ``cache_clear``.* The multiplexer is a single + process-wide singleton behind an ``lru_cache``; adapters are built **per run** + in ``runsetup.make_adapters``, which keeps its own ``by_cfg`` cache keyed on + ``(resolved-config, synthesizes)``. Selection here is a pure registry lookup, so + there is nothing to cache and :func:`register_adapter` invalidates nothing. +- *No ``configure_*`` / ``matches(platform)`` / platform defaults.* The multiplexer + is chosen by a policy knob and a ``sys.platform`` predicate; an adapter kind is + chosen by the ``profile.adapter`` field alone. There is no host-dependent + auto-selection and no persisted choice to install, so none of that machinery + exists — ``get_adapter_kind(name)`` fails loud on an unknown name rather than + falling back. +""" + +from __future__ import annotations + +import importlib.metadata +from collections.abc import Callable +from dataclasses import dataclass + +# 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 +# thing from the set of VALID kinds (that set is only ever `known_adapter_kinds()`, +# never a literal). GENERIC is the `profile.adapter` default. +GENERIC = "generic" +OPENCODE_HTTP = "opencode-http" + + +class AdapterError(Exception): + """An adapter kind could not be resolved (an unknown ``profile.adapter``). + + Construction failures a *known* family raises during ``__init__`` are its own + types (e.g. ``OpencodeServerError``), carried by + :attr:`AdapterBuilder.construct_error`, not this seam-level type.""" + + +@dataclass(frozen=True) +class AdapterBuilder: + """The classes and failure modes of one adapter family. + + ``plain`` and ``dev`` are the two variants ``runsetup.make_adapters`` picks + between on the ``synthesizes`` axis (a ``bmad-dev-auto`` dev/review session + gets ``dev``, which takes an extra ``paths=`` kwarg; every other role gets + ``plain``). ``construct_error`` is the tuple of exception types the family's + constructor raises when the session cannot be built — empty for a family that + cannot fail construction (``generic``); the caller wraps a match into a + ``SystemExit`` so a run aborts with a clean message instead of a traceback.""" + + plain: type + dev: type + construct_error: tuple[type[BaseException], ...] = () + + +@dataclass(frozen=True) +class AdapterKind: + """One registered adapter family, keyed by ``name`` (the ``profile.adapter`` + value). ``needs_mux`` gates whether ``runsetup.make_adapters`` resolves and + usability-checks the shared terminal multiplexer for this family (a hookless + HTTP/SSE family needs no transport). ``load`` is a lazy thunk returning the + :class:`AdapterBuilder`; it is the *only* place the family's classes (and any + optional dependency they pull in) are imported.""" + + name: str + needs_mux: bool + load: Callable[[], AdapterBuilder] + + +# ---------------------------------------------------------------- builtins + + +def _generic_builder() -> AdapterBuilder: + from .generic import GenericAdapter, GenericDevAdapter + + return AdapterBuilder(plain=GenericAdapter, dev=GenericDevAdapter, construct_error=()) + + +def _opencode_http_builder() -> AdapterBuilder: + from .opencode_http import ( + OpencodeDevAdapter, + OpencodeHttpAdapter, + OpencodeServerError, + ) + + return AdapterBuilder( + plain=OpencodeHttpAdapter, + dev=OpencodeDevAdapter, + construct_error=(OpencodeServerError,), + ) + + +# The bundled kinds, as (name, needs_mux, load-thunk). A module constant, not +# mutable registry state, so detect_adapters can label a row builtin-vs-external +# without the fixtures having to snapshot it. `generic` drives tmux + hooks and +# needs the multiplexer; `opencode-http` is hookless HTTP/SSE and does not. +_BUILTIN_ADAPTERS: tuple[tuple[str, bool, Callable[[], AdapterBuilder]], ...] = ( + (GENERIC, True, _generic_builder), + (OPENCODE_HTTP, False, _opencode_http_builder), +) +_BUILTIN_NAMES = frozenset(name for name, _, _ in _BUILTIN_ADAPTERS) + +# The live registry: name -> AdapterKind. Unlike the multiplexer's ordered list +# (registration order breaks selection ties), adapter selection is a pure by-name +# lookup, so a dict with first-registration-wins semantics is the whole story. +_ADAPTERS: dict[str, AdapterKind] = {} +_BUILTINS_LOADED = False + + +def register_adapter(name: str, needs_mux: bool, load: Callable[[], AdapterBuilder]) -> None: + """Register an adapter kind. ``name`` is the ``profile.adapter`` key that + selects it; ``needs_mux`` declares whether the family drives a terminal + multiplexer; ``load`` is the lazy builder thunk. First registration of a name + wins — bundled kinds register from :func:`_load_builtin_adapters` before the + entry-point scan, so an out-of-tree package can never shadow a bundled name. + An out-of-tree kind calls this at import time — no core edit required. There + is no selection cache to invalidate (see the module docstring).""" + _ADAPTERS.setdefault(name, AdapterKind(name=name, needs_mux=needs_mux, load=load)) + + +def _load_builtin_adapters() -> None: + """Register the bundled adapter kinds. Idempotent and lazy (called from the + resolution entry points, not at module import) to stay cycle-safe: the load + thunks import ``generic`` / ``opencode_http``, which import back through the + package. Builtins register before externals so a bundled name keeps + first-wins on any collision.""" + global _BUILTINS_LOADED + if _BUILTINS_LOADED: + return + for name, needs_mux, load in _BUILTIN_ADAPTERS: + register_adapter(name, needs_mux, load) + _BUILTINS_LOADED = True + + +# The entry-point group an out-of-tree adapter package advertises its module +# under; importing the module runs its register_adapter call. Loader state: +# scanned-once flag + per-entry-point failure reasons for adapters/validate. +ADAPTERS_GROUP = "bmad_loop.adapters" +_EXTERNALS_LOADED = False +_EXTERNAL_ERRORS: dict[str, str] = {} + + +def _load_external_adapters() -> None: + """Import every ``bmad_loop.adapters`` entry point; each module self-registers + via :func:`register_adapter` at import time. Called after + :func:`_load_builtin_adapters`, so builtins keep first registration. + + A broken third-party distribution must never break adapter selection: + failures are recorded in ``_EXTERNAL_ERRORS`` (surfaced by ``bmad-loop + adapters`` and the ``validate`` preflight via :func:`external_adapter_errors`), + not raised. The loaded-flag is set up front: a third-party import failure is + not transient, and retrying on every resolution would re-import (and re-fail) + each time — mirroring the multiplexer's external scan.""" + global _EXTERNALS_LOADED + if _EXTERNALS_LOADED: + return + _EXTERNALS_LOADED = True + try: + eps = importlib.metadata.entry_points(group=ADAPTERS_GROUP) + except Exception as exc: # noqa: BLE001 — diagnostics path, never crash selection + _EXTERNAL_ERRORS[""] = f"{type(exc).__name__}: {exc}" + return + for ep in eps: + 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}" + + +def external_adapter_errors() -> dict[str, str]: + """Entry-point name -> failure reason for every external adapter that failed + to load this process (empty when all loaded). For diagnostics surfaces.""" + return dict(_EXTERNAL_ERRORS) + + +def _known() -> str: + return ", ".join(sorted(_ADAPTERS)) or "(none registered)" + + +def get_adapter_kind(name: str) -> AdapterKind: + """Resolve the adapter kind named ``name`` (a ``profile.adapter`` value), + loading the builtins and scanning the entry-point group first. + + Fails loud on an unknown name, listing the registered kinds — an explicit but + unregistered adapter is a misconfiguration (a typo, or a plugin package that + isn't installed), never something to silently fall back from. The caller + (``runsetup.make_adapters``) adds the offending profile's name to the + message.""" + _load_builtin_adapters() + _load_external_adapters() + kind = _ADAPTERS.get(name) + if kind is None: + raise AdapterError(f"unknown adapter kind {name!r}; known: {_known()}") + return kind + + +def known_adapter_kinds() -> list[str]: + """Sorted names of every registered adapter kind (builtins + successfully + loaded externals). The oracle for ``validate``'s ``adapter.kind`` finding — + validity of ``profile.adapter`` is enforced against this registry, never a + hardcoded set.""" + _load_builtin_adapters() + _load_external_adapters() + return sorted(_ADAPTERS) + + +@dataclass(frozen=True) +class AdapterKindInfo: + """One registered adapter kind's detection row, for ``bmad-loop adapters`` and + the ``validate`` preflight.""" + + name: str + needs_mux: bool + builtin: bool + + +def detect_adapters() -> list[AdapterKindInfo]: + """Enumerate every registered adapter kind (builtins + loaded externals), + sorted by name, each labelled builtin-vs-external. Never raises — this feeds + diagnostics, which must work on a misconfigured host. The ``load`` thunk is + never invoked, so listing stays free of any heavy adapter import.""" + _load_builtin_adapters() + _load_external_adapters() + return [ + AdapterKindInfo( + name=kind.name, + needs_mux=kind.needs_mux, + builtin=kind.name in _BUILTIN_NAMES, + ) + for kind in sorted(_ADAPTERS.values(), key=lambda k: k.name) + ] diff --git a/src/bmad_loop/checks.py b/src/bmad_loop/checks.py index aa443652..4077272a 100644 --- a/src/bmad_loop/checks.py +++ b/src/bmad_loop/checks.py @@ -52,6 +52,9 @@ "adapter.binary", "adapter.hookless", "adapter.httpx", + "adapter.kind", + "adapter.external", + "adapter.external-profile", "queue.sprint-status", "queue.sprint-status-unknown-keys", "queue.stories-manifest", diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 1c5bbfbf..049d3001 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -295,7 +295,8 @@ def cmd_validate(args: argparse.Namespace) -> int: # story-queue gate runs below: the sprint-status file (sprint mode) or the # stories.yaml manifest (stories mode). Loaded before the queue gate so a # stories-only project is not failed on a missing sprint-status.yaml. - from .adapters.profile import ProfileError, get_profile + from .adapters import registry as adapter_registry + from .adapters.profile import ProfileError, external_profile_errors, get_profile profiles = [] profile_by_name: dict[str, CLIProfile] = {} @@ -425,14 +426,17 @@ def cmd_validate(args: argparse.Namespace) -> int: any_hooks_registered = False for profile in profiles: - if profile.hookless: - report.ok( - "adapter.hookless", - f"{profile.name}: hookless (HTTP/SSE transport) — no hook registration needed", - {"profile": profile.name}, - ) - # The HTTP adapter needs httpx, which ships as an optional extra — - # surface a missing install here instead of at run start. + # Keyed on the adapter KIND, not on `hookless`. httpx is the bundled + # opencode family's optional extra — a fact about one adapter class, which + # is a different question from "does this profile register hooks". Those + # were the same question only while `hookless` selected the adapter; the + # registry decoupled them, so a hookless profile driven by some other + # registered kind needs nothing from `bmad-loop[opencode]` and must not be + # FAILed with a remedy that installs the wrong package. Naming one bundled + # kind here is not the hardcoded-valid-set the registry exists to remove: + # the set of VALID kinds is only ever `known_adapter_kinds()` (below). + if profile.adapter == adapter_registry.OPENCODE_HTTP: + # Surface a missing install here instead of at run start. if importlib.util.find_spec("httpx") is not None: report.ok( "adapter.httpx", @@ -446,6 +450,12 @@ def cmd_validate(args: argparse.Namespace) -> int: f"run `pip install 'bmad-loop[opencode]'`", {"profile": profile.name}, ) + if profile.hookless: + report.ok( + "adapter.hookless", + f"{profile.name}: hookless (HTTP/SSE transport) — no hook registration needed", + {"profile": profile.name}, + ) continue hook_config = project / profile.hooks.config_path hooks_ok = False @@ -527,6 +537,40 @@ def cmd_validate(args: argparse.Namespace) -> int: {"path": str(relay)}, ) + # Adapter-kind validity is enforced against the LIVE registry, never a + # hardcoded set: a profile.adapter naming no registered kind is a config error + # (a typo, or an uninstalled plugin package). External adapter/profile packages + # that failed to load are surfaced as warnings — selection already degraded + # past them (the same non-blocking treatment as a failed mux backend package). + kinds = adapter_registry.known_adapter_kinds() + for profile in profiles: + if profile.adapter in kinds: + report.ok( + "adapter.kind", + f"{profile.name}: adapter kind {profile.adapter!r} registered", + {"profile": profile.name, "adapter": profile.adapter}, + ) + else: + report.fail( + "adapter.kind", + f"{profile.name}: unknown adapter kind {profile.adapter!r} — " + f"known: {', '.join(kinds)} (install the plugin that provides it, " + f"or fix the profile's `adapter`)", + {"profile": profile.name, "adapter": profile.adapter, "known": kinds}, + ) + for ep_name, reason in sorted(adapter_registry.external_adapter_errors().items()): + report.warn( + "adapter.external", + f"external adapter '{ep_name}' failed to load: {reason}", + {"entry_point": ep_name, "error": reason}, + ) + for ep_name, reason in sorted(external_profile_errors().items()): + report.warn( + "adapter.external-profile", + f"external profile '{ep_name}' failed to load: {reason}", + {"entry_point": ep_name, "error": reason}, + ) + # opencode config-file model ids are "provider/model" (see the opencode_http docstring); # a bare model name silently falls back to the server's default model, so warn # (advisory — a note, not a FAIL: an empty model legitimately means "default"). @@ -816,6 +860,64 @@ def _warn_preflight_would_abort( print("run `bmad-loop validate` for details", file=sys.stderr) +def cmd_adapters(args: argparse.Namespace) -> int: + """List the registered coding-CLI adapter kinds (the CLI axis's counterpart to + `bmad-loop mux` for the transport axis) and name any out-of-tree adapter or + profile package that failed to load. Unlike `mux`, there is no global choice + to persist: an adapter kind is selected per profile by its `adapter` field.""" + from .adapters.profile import ProfileError, external_profile_errors, load_profiles + from .adapters.registry import detect_adapters, external_adapter_errors + + # Loading profiles also triggers the bmad_loop.profiles entry-point scan, so a + # broken profile package surfaces below alongside a broken adapter package. + # A malformed project overlay is the operator's own file and aborts — this is a + # listing command, and printing a table assembled from a profile set that + # silently lost an entry is worse than saying which file is wrong. + try: + profiles = load_profiles(_project(args)) + except ProfileError as e: + print(f"error: {e}", file=sys.stderr) + return 1 + by_kind: dict[str, list[str]] = {} + for prof in profiles.values(): + by_kind.setdefault(prof.adapter, []).append(prof.name) + + rows = detect_adapters() + header = ("NAME", "ORIGIN", "NEEDS MUX", "PROFILES") + table = [ + ( + r.name, + "builtin" if r.builtin else "external", + "yes" if r.needs_mux else "no", + ", ".join(sorted(by_kind.get(r.name, []))) or "-", + ) + for r in rows + ] + widths = [max(len(h), *(len(row[i]) for row in table), 0) for i, h in enumerate(header)] + for row in (header, *table): + print(" ".join(cell.ljust(w) for cell, w in zip(row, widths)).rstrip()) + # A profile whose adapter kind never registered (a typo, or an uninstalled + # plugin) is invisible in the table above — name it so an operator can see the + # dangling reference, exactly as `mux` names a failed backend package. + known = {r.name for r in rows} + for kind in sorted(set(by_kind) - known): + print( + f"warning: profile(s) {', '.join(sorted(by_kind[kind]))} reference unknown " + f"adapter kind '{kind}' (no registered kind or plugin provides it)", + file=sys.stderr, + ) + for ep_name, reason in sorted(external_adapter_errors().items()): + print(f"warning: external adapter '{ep_name}' failed to load: {reason}", file=sys.stderr) + for ep_name, reason in sorted(external_profile_errors().items()): + print(f"warning: external profile '{ep_name}' failed to load: {reason}", file=sys.stderr) + print( + "adapter kind is selected per profile by its `adapter` field " + "(default: generic); an out-of-tree package registers new kinds via the " + "bmad_loop.adapters + bmad_loop.profiles entry points" + ) + return 0 + + def _require_base_skills(project: Path, pol, *, require_stories: bool = False) -> bool: """Preflight the upstream skills the orchestrator drives (the dev primitive — bmad-build-auto, or a complete pre-rename bmad-dev-auto — plus the review layers @@ -3556,6 +3658,12 @@ def add(name: str, func, help: str, *, aliases=()) -> argparse.ArgumentParser: "backend that only registers on the target machine)", ) + add( + "adapters", + cmd_adapters, + "list registered coding-CLI adapter kinds + which profiles select them", + ) + probe_p = add( "probe-adapter", cmd_probe, diff --git a/src/bmad_loop/data/profiles/opencode.toml b/src/bmad_loop/data/profiles/opencode.toml index 240e1f2f..ce76cf9b 100644 --- a/src/bmad_loop/data/profiles/opencode.toml +++ b/src/bmad_loop/data/profiles/opencode.toml @@ -4,6 +4,11 @@ # API facts pinned against opencode 1.18.2 (2026-07-16): see the adapters/opencode_http.py docstring. name = "opencode-http" binary = "opencode" +# Driven by the bundled HTTP/SSE adapter class (adapters/registry.py), not the +# tmux generic adapter — the selector for the coding-CLI adapter axis. Distinct +# from `hooks.dialect = "none"` below, which says only that no hook config is +# ever registered: the two axes are decoupled. +adapter = "opencode-http" prompt_template = "Use the {skill} skill now: {args}" usage_parser = "none" skill_tree = ".claude/skills" diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index ad30b0b1..eb163694 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -154,9 +154,16 @@ def config_digest( ``profile.env`` plus one *generated* variable, which the ``skill_tree`` bullet below accounts for. See the union paragraph on why the subset does not narrow what is hashed. - * ``hookless`` — the transport, because it decides *which of those two argv - builders runs at all*. See the paragraph below on why a hard-coded token is - not the same thing as a safe one. + * ``adapter`` — the field naming the adapter KIND, because it decides *which + argv builder runs at all*. ``make_adapters`` resolves it against the adapter + registry (``adapters/registry.py``), so rewriting it does not add a token: it + swaps the entire builder, and with it every rule the bullets above assume. + See the paragraph below on why a hard-coded token is not the same thing as a + safe one — that argument was written about ``hookless`` and transferred here + intact when the registry made ``adapter`` the selector. + * ``hookless`` — the transport. It no longer *selects* a builder, but it still + decides what the opencode builder emits, and it is what gates hook + registration; kept for the same wholesale-rewrite reason. Three of those are easy to lose, and each was lost in an earlier cut of this function — which is why the rule above is stated rather than the list. @@ -173,9 +180,11 @@ def config_digest( ``shlex.quote``\\ s it, which bounds it to ONE token — no word-splitting — but one token is enough for the ``--opt=value`` form. - ``hookless`` was the fourth, and it was excluded here on a reading that turned - out to be wrong, so the correction is worth keeping: *a hard-coded argv token - is not the same thing as a safe one.* Flipping ``hooks.dialect`` to ``"none"`` + The builder selector was the fourth, and it was excluded here on a reading + that turned out to be wrong, so the correction is worth keeping — it is now + the argument for ``adapter``, since ``hookless`` selected the builder only + until the registry took that job over: *a hard-coded argv token is not the + same thing as a safe one.* Flipping ``hooks.dialect`` to ``"none"`` does not add a token — it swaps the whole builder, dropping ``launch_args``, the prompt and the ``bypass_args`` fallback and putting the literal ``"serve"`` at argv[1], which ``_spawn_server`` then runs with ``cwd`` at the workspace @@ -209,6 +218,32 @@ def config_digest( mid-run is a config change nobody automated made under a running loop, which is the condition this gate reports rather than a false alarm to suppress. + That completeness rule — *walk the builder; every token traces back to a + hashed field* — is only available for a builder whose code is ours, and since + the adapter registry that is no longer guaranteed: an out-of-tree kind arrives + through the ``bmad_loop.adapters`` entry point, and its field reads cannot be + walked from here. What the rule becomes for such a kind: + + * The reads are still drawn from a CLOSED set even though the builder is open. + An adapter is constructed from its kwargs and nothing else — the resolved + ``CLIProfile``, the frozen ``Policy``, and the per-role ``extra_args`` / + ``usage_grace_s`` / ``stop_without_result_nudges`` — so there is no field an + external builder can invent. But ``Policy`` is WIDER than the launch surface + hashed above: an external builder that read, say, a ``[limits]`` knob into an + argv token would be reading a field the exclusions below drop on the grounds + that *the bundled builders* cannot turn it into one. That reasoning is + builder-scoped, so for an external kind it does not carry. + * ``adapter`` being hashed bounds what that costs. A session cannot swap in an + unpinned builder mid-run — naming a different kind moves this digest. It can + only rewrite fields of the kind the run already launched under, and which of + those that kind reads was decided by that kind's own package. + + So: derived for a bundled kind; for an external kind this pins the selector + plus the bundled launch surface, and the remainder is that package's own trust + boundary — the same boundary an enabled plugin's ``[python]`` module already + sits behind (see the plugin gaps at the end), not something a wider hash here + could close. + Deliberately EXCLUDED: * The *bytes* behind ``binary``/``launch_args`` — this pins the launch @@ -343,10 +378,17 @@ def config_digest( # template formatted, and it need not reference {prompt} at all. "prompt_template": prof.prompt_template, "env": dict(prof.env), - # The transport, because it rewrites the argv WHOLESALE rather than - # adding a token: hookless drops launch_args/prompt/bypass_args and - # substitutes `serve --port … --print-logs`, whose literal "serve" an - # interpreter binary reads as a cwd-relative script path. + # THE builder selector: `make_adapters` resolves this against the + # adapter registry and the kind it names decides which argv builder + # runs at all. Rewriting it swaps the whole launch shape without + # moving one of the fields above. + "adapter": prof.adapter, + # The transport. It no longer selects the builder (`adapter` does), + # but it still rewrites what the opencode builder emits WHOLESALE + # rather than adding a token: hookless drops launch_args/prompt/ + # bypass_args and substitutes `serve --port … --print-logs`, whose + # literal "serve" an interpreter binary reads as a cwd-relative + # script path. "hookless": prof.hookless, # None (inherit profile.bypass_args) is NOT the same state as () (an # explicit override to no flags at all); json.dumps keeps them apart. @@ -371,9 +413,9 @@ def make_adapters( from :func:`resolve_profiles`; when given, no profile is re-read from disk, so a caller that gated on :func:`config_digest` launches the *same* bytes it validated (#461 point 4). Omitted, each role resolves fresh as before.""" - from .adapters.generic import GenericAdapter, GenericDevAdapter from .adapters.multiplexer import fold_version, get_multiplexer, mux_usable from .adapters.profile import ProfileError, get_profile + from .adapters.registry import AdapterError, get_adapter_kind # The dev skill (bmad-dev-auto) writes no result.json: its adapter # synthesizes the result from the spec, and so needs the project paths to @@ -389,7 +431,10 @@ def make_adapters( # session re-invokes the dev skill on the done spec for a follow-up pass), # and the skill writes no result.json — its adapter synthesizes the result # from the spec it leaves on disk, so it needs the project paths to find - # that spec and cannot be shared with the triage role even on identical config. + # that spec and cannot be shared with the triage role even on identical + # config. `synthesizes` is a bmad-dev-auto pipeline concept (which variant + # of a family to build + whether to thread `paths`), NOT a per-family + # branch — it stays a documented contract for every registered adapter. synthesizes = role in ("dev", "review") and policy.dev.skill == "bmad-dev-auto" key = (cfg, synthesizes) if key not in by_cfg: @@ -400,35 +445,32 @@ def make_adapters( profile = get_profile(cfg.name, project) except ProfileError as e: raise SystemExit(f"error: {e}") from e - if profile.hookless: - # Hookless profiles (opencode-http) are driven over HTTP/SSE — - # the tmux adapters below cannot host them. - from .adapters.opencode_http import ( - OpencodeDevAdapter, - OpencodeHttpAdapter, - OpencodeServerError, - ) - - common = dict( - run_dir=run_dir, - policy=policy, - profile=profile, - extra_args=cfg.extra_args, - usage_grace_s=cfg.usage_grace_s, - stop_without_result_nudges=cfg.stop_without_result_nudges, - ) - try: - # heterogeneous **kwargs: pyright unions the dict values; per-arg error is spurious - by_cfg[key] = ( - OpencodeDevAdapter(**common, paths=paths) - if synthesizes - else OpencodeHttpAdapter(**common) # pyright: ignore[reportArgumentType] - ) - except OpencodeServerError as e: - raise SystemExit(f"error: {e}") from e - else: - # Resolve and probe the shared multiplexer only when a profile - # actually uses it; hookless HTTP/SSE runs need no transport. + # Which adapter class drives this CLI is pure data — `profile.adapter` + # resolved against the registry. No adapter-name branching lives here; + # a new family plugs in with zero edits to this function. Note this + # reads the profile RESOLVED ABOVE, so under the `profiles is not None` + # path the kind comes from the same bytes `config_digest` pinned (#461 + # point 4) rather than a second read of a file a session can rewrite in + # between. An unknown kind fails loud naming the profile. + try: + kind = get_adapter_kind(profile.adapter) + except AdapterError as e: + raise SystemExit(f"error: profile {profile.name!r}: {e}") from e + builder = kind.load() + # Annotated: the literal below would otherwise fix the value type to + # `Path | CLIProfile`, and the `needs_mux` arm adds a multiplexer. + common: dict[str, object] = dict( + run_dir=run_dir, + policy=policy, + profile=profile, + extra_args=cfg.extra_args, + usage_grace_s=cfg.usage_grace_s, + stop_without_result_nudges=cfg.stop_without_result_nudges, + ) + if kind.needs_mux: + # Resolve and probe the shared multiplexer only when a kind + # actually drives one; a self-hosted HTTP/SSE family needs no + # transport (and a test asserts it is never even resolved). if mux is None: mux = get_multiplexer() if not mux_usable(mux): @@ -442,21 +484,20 @@ def make_adapters( "missing, the version is unsupported, or a required helper is " "absent (psmux needs `pwsh` on PATH); see `bmad-loop diagnose`" ) - common = dict( - run_dir=run_dir, - policy=policy, - profile=profile, - extra_args=cfg.extra_args, - usage_grace_s=cfg.usage_grace_s, - stop_without_result_nudges=cfg.stop_without_result_nudges, - mux=mux, - ) + common["mux"] = mux + # The synthesizing variant additionally needs `paths`; the plain + # variant does not accept it. `construct_error` is family-declared — + # `()` for a family that cannot fail construction (generic), or e.g. + # `(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. + cls = builder.dev if synthesizes else builder.plain + build_kwargs = {**common, "paths": paths} if synthesizes else common + try: # heterogeneous **kwargs: pyright unions the dict values; per-arg error is spurious - by_cfg[key] = ( - GenericDevAdapter(**common, paths=paths) - if synthesizes - else GenericAdapter(**common) # pyright: ignore[reportArgumentType] - ) + by_cfg[key] = cls(**build_kwargs) # pyright: ignore[reportArgumentType] + except builder.construct_error as e: + raise SystemExit(f"error: {e}") from e adapters[role] = by_cfg[key] return adapters diff --git a/tests/test_adapter_registry.py b/tests/test_adapter_registry.py new file mode 100644 index 00000000..a3a45752 --- /dev/null +++ b/tests/test_adapter_registry.py @@ -0,0 +1,701 @@ +"""Coding-CLI adapter-registry selection + discovery proof. + +The CLI axis selects its adapter *class* through a registry +(:func:`~bmad_loop.adapters.registry.register_adapter`) keyed on the +``profile.adapter`` field, rather than name-branching in +``runsetup.make_adapters``. So a new adapter family is a registration — not a core +edit — exactly as a new transport backend is +(:mod:`bmad_loop.adapters.multiplexer`). These tests pin the registry (builtin + +external registration, builtins-first-wins, unknown-kind fail-loud), the +out-of-tree entry-point discovery and its degrade-not-crash contract, and the +``runsetup.make_adapters`` dispatch: a registered kind builds, the +``(cfg, synthesizes)`` cache shares instances, ``needs_mux`` gates the transport +resolution, a construction failure becomes a clean ``SystemExit``, the +digest-gated ``profiles=`` mapping decides the kind, and — the regression pin — +``opencode-http`` still dispatches to the HTTP adapters unchanged. + +The registry is deliberately simpler than the multiplexer seam: adapters are built +per-run (``runsetup.make_adapters`` owns the cache), so there is no ``lru_cache`` / +``cache_clear`` and no ``configure_*`` / platform-default machinery — the +``fresh_adapter_registry`` fixture snapshots only ``_ADAPTERS`` / the two loaded +flags / ``_EXTERNAL_ERRORS``. + +Entry points are faked by monkeypatching ``importlib.metadata.entry_points`` +through the ``registry`` module's own binding; one test builds a real +``*.dist-info`` on ``sys.path`` to prove the scan works against genuine packaging +metadata. +""" + +from __future__ import annotations + +import argparse +import json + +import pytest +from conftest import install_bmad_config + +from bmad_loop import cli +from bmad_loop import policy as policy_mod +from bmad_loop import runsetup +from bmad_loop.adapters import multiplexer as mux_mod +from bmad_loop.adapters import profile as profile_mod +from bmad_loop.adapters import registry as m +from bmad_loop.adapters.profile import CLIProfile, HookSpec +from bmad_loop.adapters.registry import AdapterBuilder, AdapterError + +# --------------------------------------------------------------------------- # +# Stubs + fixtures + + +class _StubAdapter: + """Plain-variant double: records the kwargs make_adapters passes so a test can + assert the mux was (or was not) threaded in.""" + + def __init__(self, **kwargs): + self.kwargs = kwargs + self.profile = kwargs.get("profile") + + +class _StubDevAdapter(_StubAdapter): + """Dev-variant double: mirrors the real ``(*args, paths, **kwargs)`` dev + __init__ contract every registered family's dev class honors.""" + + def __init__(self, *, paths, **kwargs): + super().__init__(**kwargs) + self.paths = paths + + +def _stub_builder(*, construct_error=()): + return AdapterBuilder(plain=_StubAdapter, dev=_StubDevAdapter, construct_error=construct_error) + + +class _FakeEntryPoint: + """Duck-typed importlib.metadata.EntryPoint: the loader only touches + ``.name`` and ``.load()``.""" + + def __init__(self, name, load): + self.name = name + self._load = load + + def load(self): + return self._load() + + +@pytest.fixture +def fresh_adapter_registry(): + """Isolate the global adapter registry: snapshot, clear, restore. No + lru_cache / configured-choice to reset — the deliberate asymmetry vs. the mux + seam. The externals scan is parked as already-loaded so whatever adapters are + installed on the dev box can't leak into builtin tests (discovery tests re-arm + it via :func:`scan_adapter_registry`). The companion profile-module external + scan is suppressed too, so an installed ``bmad_loop.profiles`` package can't + leak a profile into the ``make_adapters`` integration tests.""" + saved_adapters = dict(m._ADAPTERS) + saved_loaded = m._BUILTINS_LOADED + saved_ext_loaded = m._EXTERNALS_LOADED + saved_ext_errors = dict(m._EXTERNAL_ERRORS) + m._ADAPTERS.clear() + m._BUILTINS_LOADED = False + m._EXTERNALS_LOADED = True # externals OFF by default; discovery tests opt back in + m._EXTERNAL_ERRORS.clear() + + saved_prof_loaded = profile_mod._EXTERNALS_LOADED + saved_prof_profiles = dict(profile_mod._EXTERNAL_PROFILES) + saved_prof_errors = dict(profile_mod._PROFILE_LOAD_ERRORS) + profile_mod._EXTERNALS_LOADED = True + profile_mod._EXTERNAL_PROFILES.clear() + profile_mod._PROFILE_LOAD_ERRORS.clear() + + yield m + + m._ADAPTERS.clear() + m._ADAPTERS.update(saved_adapters) + m._BUILTINS_LOADED = saved_loaded + m._EXTERNALS_LOADED = saved_ext_loaded + m._EXTERNAL_ERRORS.clear() + m._EXTERNAL_ERRORS.update(saved_ext_errors) + profile_mod._EXTERNALS_LOADED = saved_prof_loaded + profile_mod._EXTERNAL_PROFILES.clear() + profile_mod._EXTERNAL_PROFILES.update(saved_prof_profiles) + profile_mod._PROFILE_LOAD_ERRORS.clear() + profile_mod._PROFILE_LOAD_ERRORS.update(saved_prof_errors) + + +@pytest.fixture +def scan_adapter_registry(fresh_adapter_registry, monkeypatch): + """fresh_adapter_registry with the externals scan re-armed. Yields a hook: + call it with fake entry points (or a ``scan_error`` to raise from the scan + itself) and the next resolution performs that scan.""" + + def arm(*eps, scan_error=None): + def fake_entry_points(*, group): + assert group == m.ADAPTERS_GROUP + if scan_error is not None: + raise scan_error + return list(eps) + + monkeypatch.setattr(m.importlib.metadata, "entry_points", fake_entry_points) + m._EXTERNALS_LOADED = False + m._EXTERNAL_ERRORS.clear() + + yield fresh_adapter_registry, arm + + +def _write_policy(project, text) -> None: + d = project / ".bmad-loop" + d.mkdir(parents=True, exist_ok=True) + (d / "policy.toml").write_text(text, encoding="utf-8") + + +def _write_profile(project, name, *, adapter, hookless=True) -> None: + d = project / ".bmad-loop" / "profiles" + d.mkdir(parents=True, exist_ok=True) + if hookless: + hooks = '[hooks]\ndialect = "none"\n' + else: + hooks = ( + '[hooks]\ndialect = "claude-settings-json"\n' + 'config_path = ".hermes/settings.json"\n[hooks.events]\nStop = "Stop"\n' + ) + (d / f"{name}.toml").write_text( + f'name = "{name}"\nbinary = "{name}"\nadapter = "{adapter}"\n{hooks}', encoding="utf-8" + ) + + +def _run_dir(project): + return project / ".bmad-loop" / "runs" / "r" + + +# --------------------------------------------------------------------------- # +# Registry — builtin registration + fail-loud + + +def test_builtin_kinds_registered_with_needs_mux(fresh_adapter_registry): + """The two bundled kinds register with the correct transport requirement: + generic drives tmux (needs_mux), opencode-http is hookless HTTP/SSE (does not).""" + generic = fresh_adapter_registry.get_adapter_kind("generic") + http = fresh_adapter_registry.get_adapter_kind("opencode-http") + assert generic.needs_mux is True + assert http.needs_mux is False + assert fresh_adapter_registry.known_adapter_kinds() == ["generic", "opencode-http"] + + +def test_builtin_name_constants_match_the_registered_names(fresh_adapter_registry): + """`validate`'s httpx check keys on the GENERIC/OPENCODE_HTTP constants rather + than on literals. Pin them to what actually registers, so a rename cannot make + that check silently stop firing — an absent finding reads as a pass.""" + assert fresh_adapter_registry.GENERIC == "generic" + assert fresh_adapter_registry.OPENCODE_HTTP == "opencode-http" + assert set(fresh_adapter_registry.known_adapter_kinds()) == { + fresh_adapter_registry.GENERIC, + fresh_adapter_registry.OPENCODE_HTTP, + } + + +def test_load_thunk_returns_real_builder(fresh_adapter_registry): + """The lazy thunk resolves to the real adapter classes — imported only now, + never at registration/listing time.""" + from bmad_loop.adapters.generic import GenericAdapter, GenericDevAdapter + + builder = fresh_adapter_registry.get_adapter_kind("generic").load() + assert builder.plain is GenericAdapter + assert builder.dev is GenericDevAdapter + assert builder.construct_error == () + + +def test_unknown_kind_fails_loud_naming_known(fresh_adapter_registry): + """An explicit but unregistered kind is a misconfiguration: fail loud, listing + the registered kinds — never silently fall back.""" + with pytest.raises(AdapterError, match=r"nonesuch.*generic.*opencode-http"): + fresh_adapter_registry.get_adapter_kind("nonesuch") + + +def test_register_adapter_first_wins(fresh_adapter_registry): + """A duplicate registration of a name is ignored (first wins) — the mechanism + that lets builtins load first and shrug off a same-named external.""" + registry = fresh_adapter_registry + registry.register_adapter("dup", needs_mux=True, load=lambda: _stub_builder()) + registry.register_adapter("dup", needs_mux=False, load=lambda: _stub_builder()) + assert registry.get_adapter_kind("dup").needs_mux is True + + +def test_detect_adapters_labels_builtin(fresh_adapter_registry): + """detect_adapters lists every kind, sorted, labelled builtin-vs-external, + without invoking any load thunk (no heavy import).""" + registry = fresh_adapter_registry + registry.register_adapter("extra", needs_mux=False, load=lambda: _stub_builder()) + rows = {r.name: r for r in registry.detect_adapters()} + assert set(rows) == {"generic", "opencode-http", "extra"} + assert rows["generic"].builtin is True and rows["generic"].needs_mux is True + assert rows["opencode-http"].builtin is True and rows["opencode-http"].needs_mux is False + assert rows["extra"].builtin is False + assert [r.name for r in registry.detect_adapters()] == sorted(rows) + + +# --------------------------------------------------------------------------- # +# External discovery (the bmad_loop.adapters entry-point scan) + + +def test_entry_point_adapter_registers_and_is_selectable(scan_adapter_registry): + """The pip-install-and-go path: the entry point's module import registers the + kind; it resolves and lists like a builtin, with no load error.""" + registry, arm = scan_adapter_registry + + def load(): + registry.register_adapter("extadapter", needs_mux=False, load=lambda: _stub_builder()) + + arm(_FakeEntryPoint("extadapter", load)) + kind = registry.get_adapter_kind("extadapter") + assert kind.needs_mux is False + assert "extadapter" in {r.name for r in registry.detect_adapters()} + assert registry.external_adapter_errors() == {} + + +def test_builtins_first_wins_over_external(scan_adapter_registry): + """Ordering guarantee: builtins register before the scan, so an external that + tries to register the bundled name ``generic`` cannot shadow it.""" + registry, arm = scan_adapter_registry + + def load(): + # a hostile/clumsy external claiming the builtin name with wrong needs_mux + registry.register_adapter("generic", needs_mux=False, load=lambda: _stub_builder()) + + arm(_FakeEntryPoint("shadow", load)) + kind = registry.get_adapter_kind("generic") + assert kind.needs_mux is True # the builtin, not the external + assert kind.load().plain.__name__ == "GenericAdapter" + + +def test_broken_entry_point_degrades_and_is_recorded(scan_adapter_registry): + """A distribution whose import blows up must not break selection: the builtins + still resolve, and the failure is recorded for adapters/validate to show.""" + registry, arm = scan_adapter_registry + + def boom(): + raise ImportError("No module named 'ghost_dependency'") + + arm(_FakeEntryPoint("brokenadapter", boom)) + assert registry.get_adapter_kind("generic").needs_mux is True # selection still works + errors = registry.external_adapter_errors() + assert list(errors) == ["brokenadapter"] + assert "ghost_dependency" in errors["brokenadapter"] + + +def test_one_broken_package_does_not_hide_the_rest(scan_adapter_registry): + """Per-entry isolation: the loader keeps importing after a failure, so a + working adapter still registers alongside a broken one.""" + registry, arm = scan_adapter_registry + + def boom(): + raise RuntimeError("half-installed") + + def load(): + registry.register_adapter("goodadapter", needs_mux=True, load=lambda: _stub_builder()) + + arm(_FakeEntryPoint("brokenadapter", boom), _FakeEntryPoint("goodadapter", load)) + assert registry.get_adapter_kind("goodadapter").needs_mux is True + assert list(registry.external_adapter_errors()) == ["brokenadapter"] + + +def test_scan_failure_degrades(scan_adapter_registry): + """Even the entry-point enumeration itself blowing up leaves selection working, + with the scan failure recorded.""" + registry, arm = scan_adapter_registry + arm(scan_error=RuntimeError("metadata index corrupt")) + assert registry.get_adapter_kind("generic").needs_mux is True + assert "" in registry.external_adapter_errors() + + +def test_scan_runs_once_per_process(scan_adapter_registry): + """The loaded-flag is set up front: a second resolution does not re-scan (a + third-party import failure is not transient; re-importing would re-fail).""" + registry, arm = scan_adapter_registry + calls = [] + + def load(): + calls.append(1) + + arm(_FakeEntryPoint("extadapter", load)) + registry.known_adapter_kinds() + registry.known_adapter_kinds() + assert len(calls) == 1 + + +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 + registers the kind — proving the group name works outside our fakes.""" + site = tmp_path / "site" + site.mkdir() + (site / "extadapter_pkg.py").write_text( + "from bmad_loop.adapters.registry import register_adapter\n" + "def _load():\n" + " raise AssertionError('load thunk must stay lazy')\n" + "register_adapter('extadapter-real', False, _load)\n", + encoding="utf-8", + ) + dist = site / "extadapter-0.1.dist-info" + dist.mkdir() + (dist / "METADATA").write_text("Metadata-Version: 2.1\nName: extadapter\nVersion: 0.1\n") + (dist / "entry_points.txt").write_text( + "[bmad_loop.adapters]\nextadapter = extadapter_pkg\n", encoding="utf-8" + ) + monkeypatch.syspath_prepend(str(site)) + fresh_adapter_registry._EXTERNALS_LOADED = False # re-arm the (real) scan + assert "extadapter-real" in fresh_adapter_registry.known_adapter_kinds() + assert fresh_adapter_registry.external_adapter_errors() == {} + + +# --------------------------------------------------------------------------- # +# runsetup.make_adapters dispatch through the registry + + +def test_cli_make_adapters_alias_is_the_runsetup_factory(): + """`cli._make_adapters` is a re-export seam, not a second implementation: the + run/sweep/resume composers hand `make_adapters=cli._make_adapters` from this + module's namespace so `monkeypatch.setattr(cli, "_make_adapters", ...)` keeps + biting. Pin the identity — a rebase that resurrects a duplicate in cli.py + would leave both halves importable and only one of them dispatching.""" + assert cli._make_adapters is runsetup.make_adapters + + +def test_make_adapters_dispatches_registered_kind(fresh_adapter_registry, project, monkeypatch): + """A registered out-of-tree kind dispatches with no core branching: the + synthesizing dev/review roles build the dev variant with ``paths`` + the shared + mux, triage builds the plain variant, and the ``(cfg, synthesizes)`` cache + shares the dev instance across dev+review.""" + registry = fresh_adapter_registry + registry.register_adapter("hermes", needs_mux=True, load=lambda: _stub_builder()) + monkeypatch.setattr(mux_mod, "_usable", lambda mux: True) + install_bmad_config(project) + _write_profile(project.project, "hermes", adapter="hermes", hookless=False) + _write_policy(project.project, '[adapter]\nname = "hermes"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + adapters = runsetup.make_adapters(project.project, _run_dir(project.project), pol) + + assert isinstance(adapters["dev"], _StubDevAdapter) + assert adapters["dev"] is adapters["review"] # (cfg, synthesizes) sharing + assert adapters["dev"].paths.project == project.project + assert "mux" in adapters["dev"].kwargs # needs_mux=True threaded the transport + assert isinstance(adapters["triage"], _StubAdapter) + assert not isinstance(adapters["triage"], _StubDevAdapter) + assert adapters["triage"] is not adapters["dev"] + assert "mux" in adapters["triage"].kwargs + + +def test_make_adapters_kind_comes_from_the_passed_in_profiles( + fresh_adapter_registry, project, monkeypatch +): + """#461 point 4, extended to the field that now picks the argv builder: when a + caller hands in the already-resolved `profiles` it gated on, the adapter KIND + must come from THOSE bytes — not from a second read of a file a driven session + can rewrite in between. On-disk says `generic`; the passed-in mapping says + `hermes`; `hermes` must be what builds. + + ABLATION: move the `get_adapter_kind` call above the `profiles is not None` + branch (or re-read with `get_profile` for the kind) and this reddens — the + generic tmux adapter builds instead of the stub.""" + registry = fresh_adapter_registry + registry.register_adapter("hermes", needs_mux=False, load=lambda: _stub_builder()) + monkeypatch.setattr(mux_mod, "get_multiplexer", lambda: pytest.fail("no mux for this kind")) + install_bmad_config(project) + _write_profile(project.project, "swapme", adapter="generic", hookless=False) + _write_policy(project.project, '[adapter]\nname = "swapme"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + pinned = CLIProfile( + name="swapme", binary="swapme", adapter="hermes", hooks=HookSpec("none", "", {}) + ) + adapters = runsetup.make_adapters( + project.project, + _run_dir(project.project), + pol, + profiles=dict.fromkeys(runsetup.ROLES, pinned), + ) + assert isinstance(adapters["dev"], _StubDevAdapter) + assert adapters["dev"].profile is pinned + + +def test_config_digest_pins_the_adapter_kind(project): + """The kind selects the argv builder, so rewriting it mid-run must move the + digest the auto-sweep gate compares against — otherwise a driven session swaps + the whole launch shape without tripping the #461 pin. + + ABLATION: drop `"adapter": prof.adapter` from the launch payload and the two + digests below become equal.""" + install_bmad_config(project) + _write_profile(project.project, "swapme", adapter="generic", hookless=False) + _write_policy(project.project, '[adapter]\nname = "swapme"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + before = runsetup.config_digest(pol, project.project) + _write_profile(project.project, "swapme", adapter="opencode-http", hookless=False) + after = runsetup.config_digest(pol, project.project) + assert before != after + + +def test_make_adapters_skips_mux_for_needs_mux_false_kind( + fresh_adapter_registry, project, monkeypatch +): + """A ``needs_mux=False`` kind must never resolve the multiplexer — the same + guarantee the hookless HTTP adapter relies on.""" + registry = fresh_adapter_registry + registry.register_adapter("noxport", needs_mux=False, load=lambda: _stub_builder()) + + def no_mux(): + raise AssertionError("a needs_mux=False kind must not resolve a multiplexer") + + monkeypatch.setattr(mux_mod, "get_multiplexer", no_mux) + install_bmad_config(project) + _write_profile(project.project, "noxport", adapter="noxport") + _write_policy(project.project, '[adapter]\nname = "noxport"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + adapters = runsetup.make_adapters(project.project, _run_dir(project.project), pol) + assert isinstance(adapters["dev"], _StubDevAdapter) + assert "mux" not in adapters["dev"].kwargs + + +def test_make_adapters_needs_mux_true_kind_still_refuses_an_unusable_mux( + fresh_adapter_registry, project, monkeypatch +): + """The other half of the `needs_mux` gate: moving the transport probe inside it + must not make the refusal unreachable for a family that DOES drive one. + + Ablating the gate to `if True:` leaves this green and the previous test red, so + the pair is what pins the gate — this one alone would not.""" + registry = fresh_adapter_registry + registry.register_adapter("hermes", needs_mux=True, load=lambda: _stub_builder()) + monkeypatch.setattr(mux_mod, "_usable", lambda mux: False) + install_bmad_config(project) + _write_profile(project.project, "hermes", adapter="hermes", hookless=False) + _write_policy(project.project, '[adapter]\nname = "hermes"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + with pytest.raises(SystemExit, match="not usable"): + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + + +def test_make_adapters_construct_error_becomes_systemexit( + fresh_adapter_registry, project, monkeypatch +): + """A family-declared construction failure raised in __init__ is converted to a + clean SystemExit — a run aborts with a message, not a traceback.""" + + class _Boom(Exception): + pass + + class _Exploding: + def __init__(self, **kwargs): + raise _Boom("server would not start") + + registry = fresh_adapter_registry + registry.register_adapter( + "boomer", + needs_mux=False, + load=lambda: AdapterBuilder(plain=_Exploding, dev=_Exploding, construct_error=(_Boom,)), + ) + install_bmad_config(project) + _write_profile(project.project, "boomer", adapter="boomer") + _write_policy(project.project, '[adapter]\nname = "boomer"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + with pytest.raises(SystemExit, match="server would not start"): + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + + +def test_make_adapters_unrelated_construct_failure_is_not_swallowed( + fresh_adapter_registry, project +): + """`construct_error` is a family's DECLARED failure mode, not a catch-all: an + exception it does not name must propagate as itself, so a genuine bug in an + adapter surfaces as a traceback rather than a misleading `error:` line. + + ABLATION: widen the `except builder.construct_error` to `except Exception` and + this reddens (SystemExit is raised instead).""" + + class _Exploding: + def __init__(self, **kwargs): + raise ZeroDivisionError("a real bug, not a declared failure") + + fresh_adapter_registry.register_adapter( + "buggy", + needs_mux=False, + load=lambda: AdapterBuilder(plain=_Exploding, dev=_Exploding, construct_error=()), + ) + install_bmad_config(project) + _write_profile(project.project, "buggy", adapter="buggy") + _write_policy(project.project, '[adapter]\nname = "buggy"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + with pytest.raises(ZeroDivisionError): + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + + +def test_make_adapters_unknown_kind_systemexit_names_profile(fresh_adapter_registry, project): + """A profile whose ``adapter`` names no registered kind aborts the run with a + SystemExit that names both the profile and the known kinds.""" + install_bmad_config(project) + _write_profile(project.project, "weird", adapter="ghostkind") + _write_policy(project.project, '[adapter]\nname = "weird"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + with pytest.raises(SystemExit, match=r"weird.*ghostkind.*generic"): + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + + +def test_make_adapters_generic_shares_synthesizing_but_not_triage( + fresh_adapter_registry, project, monkeypatch +): + """The ``(cfg, synthesizes)`` cache with the real builtin generic kind: dev and + review (same cfg, both the dev primitive) share one GenericDevAdapter, triage on + the same cfg gets a separate plain GenericAdapter.""" + from bmad_loop.adapters.generic import GenericAdapter, GenericDevAdapter + + monkeypatch.setattr(mux_mod, "_usable", lambda mux: True) + install_bmad_config(project) + adapters = runsetup.make_adapters( + project.project, _run_dir(project.project), policy_mod.load(None) + ) + assert isinstance(adapters["dev"], GenericDevAdapter) + assert adapters["dev"] is adapters["review"] + assert isinstance(adapters["triage"], GenericAdapter) + assert not isinstance(adapters["triage"], GenericDevAdapter) + assert adapters["triage"] is not adapters["dev"] + + +def test_make_adapters_opencode_http_dispatch_unchanged( + fresh_adapter_registry, project, monkeypatch +): + """Regression pin: routing the ``opencode-http`` profile through the registry + yields exactly the pre-registry adapters — OpencodeDevAdapter for the + synthesizing roles (never resolving a mux), OpencodeHttpAdapter for triage.""" + from bmad_loop.adapters import opencode_http + from bmad_loop.adapters.opencode_http import OpencodeDevAdapter, OpencodeHttpAdapter + + def no_mux(): + raise AssertionError("hookless opencode-http must not resolve a multiplexer") + + monkeypatch.setattr(opencode_http, "_require_httpx", lambda: object()) + monkeypatch.setattr(mux_mod, "get_multiplexer", no_mux) + install_bmad_config(project) + _write_policy(project.project, '[adapter]\nname = "opencode"\n') # alias → opencode-http + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + adapters = runsetup.make_adapters(project.project, _run_dir(project.project), pol) + assert isinstance(adapters["dev"], OpencodeDevAdapter) + assert adapters["dev"] is adapters["review"] + assert adapters["dev"].profile.adapter == "opencode-http" + assert isinstance(adapters["triage"], OpencodeHttpAdapter) + assert not isinstance(adapters["triage"], OpencodeDevAdapter) + + +# --------------------------------------------------------------------------- # +# `bmad-loop adapters` + the validate findings + + +def test_adapters_command_lists_builtins_and_surfaces_failures( + scan_adapter_registry, capsys, tmp_path +): + """`bmad-loop adapters` renders the kind table and names a failed out-of-tree + package — the one place an operator looks when an installed adapter is missing.""" + registry, arm = scan_adapter_registry + + def boom(): + raise ImportError("No module named 'ghost_dependency'") + + arm(_FakeEntryPoint("brokenadapter", boom)) + args = argparse.Namespace(project=tmp_path) + assert cli.cmd_adapters(args) == 0 + captured = capsys.readouterr() + assert "generic" in captured.out # the table renders + assert "opencode-http" in captured.out + assert "brokenadapter" in captured.err + assert "ghost_dependency" in captured.err + + +def test_adapters_command_flags_dangling_kind_reference(fresh_adapter_registry, capsys, tmp_path): + """A project profile whose adapter kind never registered is named as a warning: + the table can't show a kind that isn't there, so the dangling reference is + surfaced explicitly.""" + _write_profile(tmp_path, "weird", adapter="ghostkind") + args = argparse.Namespace(project=tmp_path) + assert cli.cmd_adapters(args) == 0 + captured = capsys.readouterr() + assert "ghostkind" in captured.err + assert "weird" in captured.err + + +def test_adapters_command_reports_a_malformed_overlay(fresh_adapter_registry, capsys, tmp_path): + """A listing assembled from a profile set that silently lost an entry is worse + than a named error: a malformed project overlay exits 1 naming the file.""" + d = tmp_path / ".bmad-loop" / "profiles" + d.mkdir(parents=True) + (d / "bad.toml").write_text('name = "bad"\nbinary = "bad"\nadapter = 5\n[hooks]\n') + assert cli.cmd_adapters(argparse.Namespace(project=tmp_path)) == 1 + assert "adapter must be a string" in capsys.readouterr().err + + +def _validate_findings(project, capsys): + """Run the real `validate --json` and hand back its findings. Through the CLI + on purpose: the check ids then pass the `VALIDATE_CHECKS` assert in + `ValidationReport.add`, which is what makes a new id's absence a crash rather + than a quiet no-op.""" + cli.main(["validate", "--project", str(project), "--json"]) + return json.loads(capsys.readouterr().out)["findings"] + + +def test_validate_httpx_check_keys_on_the_adapter_kind_not_hooklessness( + fresh_adapter_registry, project, capsys +): + """The httpx extra belongs to the opencode-http FAMILY, not to hooklessness. + Once the two axes decoupled, a hookless profile driven by another registered + kind must draw no `adapter.httpx` finding at all — FAILing it would tell an + operator to `pip install bmad-loop[opencode]` for a package they do not use. + + ABLATION: re-key the check on `profile.hookless` and the `not any(...)` assert + reddens (an httpx finding appears for a kind that never imports httpx).""" + fresh_adapter_registry.register_adapter("hermes", needs_mux=False, load=lambda: _stub_builder()) + install_bmad_config(project) + _write_profile(project.project, "hermes", adapter="hermes") # hookless=True + _write_policy(project.project, '[adapter]\nname = "hermes"\n') + + findings = _validate_findings(project.project, capsys) + assert not any(f["check"] == "adapter.httpx" for f in findings) + # the transport question is still answered — only the family question moved + assert any(f["check"] == "adapter.hookless" for f in findings) + assert [f["severity"] for f in findings if f["check"] == "adapter.kind"] == ["ok"] + + +def test_validate_flags_an_unregistered_adapter_kind(fresh_adapter_registry, project, capsys): + """`adapter.kind` is resolved against the live registry, so a profile naming a + kind no installed package provides is a FAIL that names the known set.""" + install_bmad_config(project) + _write_profile(project.project, "weird", adapter="ghostkind") + _write_policy(project.project, '[adapter]\nname = "weird"\n') + + findings = [ + f for f in _validate_findings(project.project, capsys) if f["check"] == "adapter.kind" + ] + assert [f["severity"] for f in findings] == ["problem"] + assert "ghostkind" in findings[0]["message"] and "generic" in findings[0]["message"] + + +def test_validate_warns_on_a_broken_external_package(scan_adapter_registry, project, capsys): + """A half-installed out-of-tree package is a WARNING, not a failure: selection + already degraded past it, so the same non-blocking treatment a failed mux + backend package gets.""" + _registry, arm = scan_adapter_registry + + def boom(): + raise ImportError("No module named 'ghost_dependency'") + + arm(_FakeEntryPoint("brokenadapter", boom)) + install_bmad_config(project) + + findings = [ + f for f in _validate_findings(project.project, capsys) if f["check"] == "adapter.external" + ] + assert [f["severity"] for f in findings] == ["warning"] + assert "ghost_dependency" in findings[0]["message"] diff --git a/tests/test_cli.py b/tests/test_cli.py index a8c8f8b5..719edca5 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1668,15 +1668,21 @@ def test_make_adapters_review_synthesizes_from_spec(project, monkeypatch): def test_make_adapters_hookless_synthesizing_roles_get_dev_adapter(project, monkeypatch): - """Hookless dev/review (bmad-dev-auto roles) dispatch to OpencodeDevAdapter — - the _DevSynthesisMixin composed over the HTTP transport — sharing one - instance via the (cfg, synthesizes) key, while triage on the same config - gets a separate plain OpencodeHttpAdapter (it reads a real result.json).""" + """Dev/review on the `opencode-http` adapter KIND dispatch to + OpencodeDevAdapter — the _DevSynthesisMixin composed over the HTTP transport — + sharing one instance via the (cfg, synthesizes) key, while triage on the same + config gets a separate plain OpencodeHttpAdapter (it reads a real result.json). + + Named `hookless` for the era when `profile.hookless` was what selected the + adapter class. It no longer is: the kind comes from `profile.adapter` and + hooklessness only describes the hook transport. The opencode profile carries + both, so this still exercises the same dispatch — see + tests/test_adapter_registry.py for the axes tested apart.""" from bmad_loop.adapters import opencode_http from bmad_loop.adapters.opencode_http import OpencodeDevAdapter, OpencodeHttpAdapter def no_mux(): - raise AssertionError("hookless adapters must not resolve a multiplexer") + raise AssertionError("a needs_mux=False kind must not resolve a multiplexer") monkeypatch.setattr(opencode_http, "_require_httpx", lambda: object()) monkeypatch.setattr(mux_mod, "get_multiplexer", no_mux) @@ -1695,9 +1701,9 @@ def no_mux(): def test_make_adapters_hookless_triage_dispatches_http_adapter(project, monkeypatch): - """A hookless profile on a non-synthesizing role (triage) dispatches to the - HTTP adapter — resolved via the `opencode` alias — while dev/review keep the - shared spec-synthesizing tmux adapter. The HTTP adapter exposes `profile` + """An `opencode-http`-kind profile on a non-synthesizing role (triage) + dispatches to the HTTP adapter — resolved via the `opencode` alias — while + dev/review keep the shared spec-synthesizing tmux adapter. The HTTP adapter exposes `profile` (worktree provisioning keys off it) and never constructs a multiplexer.""" from bmad_loop.adapters import opencode_http from bmad_loop.adapters.generic import GenericDevAdapter diff --git a/tests/test_profile.py b/tests/test_profile.py index 9cfd5a46..b5805c8a 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -1,6 +1,9 @@ import pytest +from bmad_loop.adapters import profile as profile_mod from bmad_loop.adapters.profile import ( + CLIProfile, + HookSpec, ProfileError, get_profile, load_profiles, @@ -173,6 +176,52 @@ def test_unknown_profile_raises(): get_profile("acme-cli") +# --------------------------------------------------------------------------- # +# The `adapter` field (which adapter CLASS drives the profile) + + +def test_adapter_field_defaults_to_generic_and_parses(): + """The adapter kind is read at parse time: unset defaults to the bundled tmux + generic; opencode-http declares its HTTP adapter kind. Asserted across ALL + built-ins, not two spot checks — a profile silently defaulting to `generic` + would dispatch to the tmux adapter, which cannot host it.""" + profiles = load_profiles() + assert profiles["opencode-http"].adapter == "opencode-http" + assert {name for name, p in profiles.items() if p.adapter == "generic"} == ( + set(profiles) - {"opencode-http"} + ) + + +def test_adapter_kind_membership_is_not_checked_at_parse_time(tmp_path): + """A profile naming an unregistered adapter kind still PARSES — validity is + enforced later against the live registry (at construction / by `validate`), + never a set literal here that every new adapter would have to edit.""" + profiles_dir = tmp_path / ".bmad-loop" / "profiles" + profiles_dir.mkdir(parents=True) + (profiles_dir / "future.toml").write_text( + MINIMAL_PROFILE.replace("[hooks]", 'adapter = "not-a-real-kind-yet"\n[hooks]') + ) + assert load_profiles(tmp_path)["mycli"].adapter == "not-a-real-kind-yet" + + +@pytest.mark.parametrize("value", ["5", "[1]", "{ k = 1 }", "true", '""']) +def test_malformed_adapter_value_funnels_into_profile_error(tmp_path, value): + """#384: a malformed value funnels into ProfileError at the boundary rather + than being coerced. `adapter` is the one selector field with no parse-time + membership test to land in afterwards, so `str(["x"])` would carry the literal + `"['x']"` all the way to `get_adapter_kind` and name that as the unknown kind. + + ABLATION: replace the isinstance check with `str(doc.get("adapter", ...))` and + every row but `""` stops raising.""" + profiles_dir = tmp_path / ".bmad-loop" / "profiles" + profiles_dir.mkdir(parents=True) + (profiles_dir / "bad.toml").write_text( + MINIMAL_PROFILE.replace("[hooks]", f"adapter = {value}\n[hooks]") + ) + with pytest.raises(ProfileError, match="adapter"): + load_profiles(tmp_path) + + def test_render_prompt_passthrough_and_template(): claude = get_profile("claude") assert claude.render_prompt("/bmad-dev-auto 1-1-a") == "/bmad-dev-auto 1-1-a" @@ -404,3 +453,235 @@ def test_every_toml_value_type_parses_or_raises_profile_error(tmp_path, key, val load_profiles(tmp_path) except ProfileError: pass + + +# --------------------------------------------------------------------------- # +# Out-of-tree profile providers (the bmad_loop.profiles entry-point scan) + + +@pytest.fixture +def profile_scan(monkeypatch): + """Isolate + re-arm the profile entry-point scan: snapshot/clear the module's + external-scan state, then hand back a hook to install fake entry points.""" + saved_loaded = profile_mod._EXTERNALS_LOADED + saved_profiles = dict(profile_mod._EXTERNAL_PROFILES) + saved_errors = dict(profile_mod._PROFILE_LOAD_ERRORS) + + def arm(*eps, scan_error=None): + def fake_entry_points(*, group): + assert group == profile_mod.PROFILES_GROUP + if scan_error is not None: + raise scan_error + return list(eps) + + monkeypatch.setattr(profile_mod.importlib.metadata, "entry_points", fake_entry_points) + profile_mod._EXTERNALS_LOADED = False + profile_mod._EXTERNAL_PROFILES.clear() + profile_mod._PROFILE_LOAD_ERRORS.clear() + + yield arm + + profile_mod._EXTERNALS_LOADED = saved_loaded + profile_mod._EXTERNAL_PROFILES.clear() + profile_mod._EXTERNAL_PROFILES.update(saved_profiles) + profile_mod._PROFILE_LOAD_ERRORS.clear() + profile_mod._PROFILE_LOAD_ERRORS.update(saved_errors) + + +class _FakeEntryPoint: + def __init__(self, name, load): + self.name = name + self._load = load + + def load(self): + return self._load() + + +def _plugin_profile(name="acme", adapter="acme", **over): + fields = { + "name": name, + "binary": name, + "adapter": adapter, + "hooks": HookSpec("none", "", {}), + **over, + } + return CLIProfile(**fields) + + +def test_entry_point_profile_is_discovered(profile_scan): + """A pip-installed profile provider (a callable returning CLIProfiles) makes + its profile resolvable with no project TOML — the zero-config selection path.""" + profile_scan(_FakeEntryPoint("acme", lambda: (lambda: [_plugin_profile()]))) + prof = get_profile("acme") + assert prof.name == "acme" and prof.adapter == "acme" + assert profile_mod.external_profile_errors() == {} + + +def test_entry_point_profile_provider_may_be_iterable(profile_scan): + """The provider may be an iterable directly, not only a callable returning + one — both shapes are accepted.""" + profile_scan(_FakeEntryPoint("acme", lambda: [_plugin_profile()])) + assert "acme" in load_profiles() + + +def test_project_profile_overrides_entry_point(profile_scan, tmp_path): + """Precedence packaged < entry-point < project: a project-local TOML of the + same name wins over an entry-point profile.""" + profile_scan(_FakeEntryPoint("acme", lambda: [_plugin_profile(adapter="acme")])) + profiles_dir = tmp_path / ".bmad-loop" / "profiles" + profiles_dir.mkdir(parents=True) + (profiles_dir / "acme.toml").write_text( + MINIMAL_PROFILE.replace('name = "mycli"', 'name = "acme"') + ) + prof = load_profiles(tmp_path)["acme"] + assert prof.binary == "mycli" # the project TOML, not the entry-point profile + + +def test_entry_point_profile_can_override_packaged(profile_scan): + """Entry-point profiles overlay the packaged built-ins (packaged < + entry-point), so a plugin may re-point a bundled name.""" + profile_scan(_FakeEntryPoint("acme", lambda: [_plugin_profile(name="claude", adapter="acme")])) + assert load_profiles()["claude"].adapter == "acme" + + +def test_broken_profile_provider_degrades_and_is_recorded(profile_scan): + """A provider that blows up must not break profile loading: the built-ins + still load, and the failure is recorded for diagnostics.""" + + def boom(): + raise RuntimeError("half-installed plugin") + + profile_scan(_FakeEntryPoint("broken", boom)) + profiles = load_profiles() + assert "claude" in profiles # built-ins unaffected + assert list(profile_mod.external_profile_errors()) == ["broken"] + assert "half-installed" in profile_mod.external_profile_errors()["broken"] + + +def test_one_broken_profile_package_does_not_hide_the_rest(profile_scan): + """Per-entry isolation: a good provider still registers alongside a broken one.""" + + def boom(): + raise RuntimeError("broke") + + profile_scan( + _FakeEntryPoint("broken", boom), + _FakeEntryPoint("acme", lambda: [_plugin_profile()]), + ) + profiles = load_profiles() + assert "acme" in profiles + assert list(profile_mod.external_profile_errors()) == ["broken"] + + +def test_profile_provider_returning_junk_is_rejected(profile_scan): + """A provider that yields a non-CLIProfile is the package's bug — recorded, + never trusted into the profile map.""" + profile_scan(_FakeEntryPoint("acme", lambda: [object()])) + profiles = load_profiles() + assert "acme" not in profiles + assert "not CLIProfile" in profile_mod.external_profile_errors()["acme"] + + +def test_profile_provider_returning_a_non_iterable_is_rejected(profile_scan): + """`list(produced)` is the shape check: a provider handing back a scalar is + reported rather than raising a bare TypeError out of load_profiles.""" + profile_scan(_FakeEntryPoint("acme", lambda: 5)) + assert "acme" not in load_profiles() + assert "iterable of CLIProfile" in 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.""" + profile_scan(scan_error=RuntimeError("metadata index corrupt")) + assert "claude" in load_profiles() + assert "" in profile_mod.external_profile_errors() + + +# --------------------------------------------------------------------------- # +# Entry-point profiles obey the SAME invariants a TOML profile does + + +@pytest.mark.parametrize( + ("over", "match"), + [ + # the finding's own example: unchecked, this compile error moves from LOAD + # time to MATCH time, inside a session's env-fault classification, where + # the caller degrades rather than raises and the pattern never fires + ({"env_fault_patterns": ("API Error(unbalanced",)}, "not a valid regex"), + # a dialect the hook writer has no branch for + ({"hooks": HookSpec("mycli-json", ".mycli/s.json", {"Stop": "Stop"})}, "dialect"), + # a non-canonical event name silently never maps to a completion signal + ( + {"hooks": HookSpec("claude-settings-json", ".m/s.json", {"Stop": "TurnDone"})}, + "canonical", + ), + # path containment — the three fields provision_worktree/install resolve + ({"skill_tree": "/abs/skills"}, "skill_tree"), + ({"skill_tree": "."}, "skill_tree"), + ({"seed_files": ("/etc/passwd",)}, "seed_files"), + ({"seed_files": (".",)}, "seed_files"), + ({"hooks": HookSpec("claude-settings-json", "..", {"Stop": "Stop"})}, "relative"), + # a real dialect with nothing to write to + ({"hooks": HookSpec("claude-settings-json", "", {"Stop": "Stop"})}, "config_path"), + # hookless carrying hook plumbing is a contradiction either way in + ({"hooks": HookSpec("none", ".m/s.json", {})}, "hookless"), + # the remaining value-level knobs + ({"usage_parser": "magic"}, "usage_parser"), + ({"usage_grace_s": -1.0}, "usage_grace_s"), + ({"stop_without_result_nudges": -2}, "stop_without_result_nudges"), + ({"adapter": ""}, "adapter"), + ({"binary": " "}, "required"), + ], +) +def test_entry_point_profile_must_pass_the_parser_invariants(profile_scan, over, match): + """The trust-boundary fix: an entry point hands over an already-CONSTRUCTED + CLIProfile, so it is the one route into the profile map with no parser in + front of it. Every value-level invariant `_parse_profile` enforces has to + apply here too, or a Python package can install a state a TOML author would + have been refused. + + Rejection is degrade-and-record (the entry-point contract), so the proof is + that the profile never lands in the map AND the reason names the invariant. + + ABLATION: delete the `_validate_profile` call from `_coerce_profiles` and + every row here goes green-with-a-bad-profile-installed — `"acme" in profiles` + becomes true. Deleting it from `_parse_profile` instead reddens the TOML rows + in `test_invalid_profiles_rejected`, which is the other half of the pair.""" + profile_scan(_FakeEntryPoint("acme", lambda: [_plugin_profile(**over)])) + profiles = load_profiles() + assert "acme" not in profiles, "an invalid profile must never reach the map" + assert match in profile_mod.external_profile_errors()["acme"] + + +def test_entry_point_batch_is_rejected_whole(profile_scan): + """One invalid profile drops the provider's whole batch rather than + half-installing it: a provider is one package's declaration, and an operator + reading the recorded reason would otherwise be looking at a profile set the + error message does not account for.""" + profile_scan( + _FakeEntryPoint( + "acme", + lambda: [ + _plugin_profile(name="good"), + _plugin_profile(name="bad", env_fault_patterns=("(unbalanced",)), + ], + ) + ) + profiles = load_profiles() + assert "good" not in profiles and "bad" not in profiles + assert "not a valid regex" in profile_mod.external_profile_errors()["acme"] + + +def test_a_valid_entry_point_profile_still_lands(profile_scan): + """The control for the rejection rows above: a provider whose profiles DO + satisfy the invariants is installed unchanged, so those tests are failing on + the invariant rather than on the plumbing.""" + profile_scan( + _FakeEntryPoint( + "acme", + lambda: [_plugin_profile(env_fault_patterns=("API Error.*Connection refused",))], + ) + ) + assert load_profiles()["acme"].env_fault_patterns == ("API Error.*Connection refused",) + assert profile_mod.external_profile_errors() == {} diff --git a/tests/test_runsetup.py b/tests/test_runsetup.py index a612bf71..f3de22e2 100644 --- a/tests/test_runsetup.py +++ b/tests/test_runsetup.py @@ -181,8 +181,11 @@ def test_digest_ignores_the_adapter_model(pinned): def test_digest_moves_when_the_transport_flips_to_hookless(pinned): - """`hooks.dialect = "none"` swaps the argv BUILDER, not a token in it: - `make_adapters` routes to the HTTP adapter, whose `_serve_argv` drops + """`hooks.dialect = "none"` reshapes the argv WHOLESALE rather than moving a + token in it. (Since the adapter registry the field that picks the BUILDER is + `profile.adapter` — pinned by its own test; `hookless` still decides what the + opencode builder emits, which is what this row covers.) That builder's + `_serve_argv` drops `launch_args`, the prompt and the `bypass_args` fallback and puts the literal "serve" at argv[1] — run with `cwd` at the workspace root. Against an interpreter `binary` (python/sh/node, the real program in `launch_args` — From 17012b839991b32d2491ba7312af3a2205260769 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 14:17:49 -0700 Subject: [PATCH 2/6] =?UTF-8?q?fix(adapters):=20close=20the=20registry=20r?= =?UTF-8?q?eview=20round=20=E2=80=94=20shadowing,=20dispatch=20back-compat?= =?UTF-8?q?,=20loader=20escalation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 1 of the review gate on the adapter-registry seam (codex ×3, CodeRabbit ×3 + 4 nitpicks, plus an independent pass). - register_adapter seeds the builtins itself, so a bundled kind cannot be shadowed however early an external import lands. The documented packaging layout puts both entry points in ONE module, so the bmad_loop.profiles scan imports it — long before any kind is resolved — and setdefault kept the external under the bundled name. Proven with a real *.dist-info repro. - A profile predating the `adapter` field keeps its old dispatch: absent key + dialect "none" resolves to opencode-http, not the generic tmux adapter. A project overlay copied from the packaged opencode profile would otherwise have waited out session_timeout_min for a hook it never registers, with validate green (every check that would catch it also keys on hookless). - A failing lazy load thunk becomes a clean SystemExit naming the profile and kind. ImportError only — a missing dependency is a loader's declared failure; anything else is a bug in that package and still surfaces as a traceback, matching the construct_error rule. - The dry-run honesty banner reports an unregistered adapter kind, which a preview otherwise renders straight past. - external_profile_errors/external_adapter_errors perform their own scan instead of depending on a neighbouring call — a PolicyError used to abort before the profiles scan ever ran, reporting a broken package as absent. - _validate_profile strips `adapter`, closing the last TOML-vs-entry-point divergence; both entry-point scans visit in name order so first-wins is a fact about the packages, not about sys.path. Every fix lands with an ablation-verified test. --- CHANGELOG.md | 6 ++ docs/adapter-authoring-guide.md | 2 +- src/bmad_loop/adapters/profile.py | 54 +++++++++- src/bmad_loop/adapters/registry.py | 64 +++++++++--- src/bmad_loop/cli.py | 41 +++++++- src/bmad_loop/runsetup.py | 19 +++- tests/test_adapter_registry.py | 159 ++++++++++++++++++++++++++++- tests/test_profile.py | 57 ++++++++++- 8 files changed, 375 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58340a99..4face5e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -145,6 +145,12 @@ whose seams had diverged enough that several ports needed a different fix, and t hookless profile driven by another kind no longer FAILs with a remedy that installs the wrong package. +- **A profile written before the `adapter` field keeps its old dispatch.** `hooks.dialect = "none"` + used to be the class selector, so a project overlay copied from the packaged opencode profile + carries no `adapter` key; it now resolves to `opencode-http` rather than defaulting onto the tmux + generic adapter, where it would have waited out `session_timeout_min` for a hook a hookless profile + never registers. An explicit `adapter` is always honored, including hookless driven by another kind. + - **Profiles from a `bmad_loop.profiles` entry point are validated like TOML ones.** Both routes into the profile map now share one invariant set (hook dialect, path containment, `env_fault_patterns` compilation, …), so a package can no longer install a profile state the parser would refuse — an diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 8b72b3b9..75f2922f 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -406,7 +406,7 @@ resolves to `claude`. | `name` | ✅ | — | Profile id, also the `--cli` value and override key. | | `binary` | ✅ | — | Executable to launch (resolved on `PATH`). | | `[hooks]` | ✅ | — | The `HookSpec` table (see below). | -| `adapter` | | `generic` | Which adapter **class** drives this CLI — a key resolved against the [adapter registry](#shipping-a-new-adapter-class-out-of-tree), not a fixed enum. `generic` = the bundled tmux + hook-signal adapter; `opencode-http` = the bundled HTTP/SSE adapter; an out-of-tree package registers its own. Membership is checked against the **live** registry (at run start, and by `bmad-loop validate`'s `adapter.kind`), never at parse time — so an unknown kind is a clear config error rather than a schema change. Independent of `hooks.dialect = "none"`: hooklessness is about the transport, this is about the driving class. | +| `adapter` | | `generic` | Which adapter **class** drives this CLI — a key resolved against the [adapter registry](#shipping-a-new-adapter-class-out-of-tree), not a fixed enum. `generic` = the bundled tmux + hook-signal adapter; `opencode-http` = the bundled HTTP/SSE adapter; an out-of-tree package registers its own. Membership is checked against the **live** registry (at run start, and by `bmad-loop validate`'s `adapter.kind`), never at parse time — so an unknown kind is a clear config error rather than a schema change. Independent of `hooks.dialect = "none"`: hooklessness is about the transport, this is about the driving class — with one back-compat carve-out for files written before this field existed: when the key is **absent** and the dialect is `none`, the kind is `opencode-http` (what hooklessness used to select), not `generic`. | | `skill_tree` | | `.claude/skills` | Project-relative tree this CLI reads skills from (`.agents/skills` for codex/gemini); `bmad-loop init` installs the `bmad-loop-*` skills here. Must be relative. | | `prompt_template` | | `{prompt}` | How the canonical `/skill args` prompt is rendered. Placeholders: `{prompt}` (whole string), `{skill}` (leading slash-command name, no `/`), `{args}` (the remainder). | | `launch_args` | | `()` | Extra argv passed at launch, e.g. `["-i"]` to stay interactive (gemini/copilot). | diff --git a/src/bmad_loop/adapters/profile.py b/src/bmad_loop/adapters/profile.py index 0fefa517..2f628ee7 100644 --- a/src/bmad_loop/adapters/profile.py +++ b/src/bmad_loop/adapters/profile.py @@ -222,7 +222,11 @@ def fail(msg: str) -> ProfileError: # Shape only — membership against the registered kinds is deliberately NOT # checked here (see the module docstring): that set is open-ended and lives in # adapters/registry.py, which importing from here would make a cycle. - if not profile.adapter: + # `.strip()` because the TOML route strips before it gets here: testing the raw + # value would refuse `adapter = " "` from a file while admitting it from an + # entry-point provider, which is exactly the two-routes divergence this + # function exists to prevent. + if not profile.adapter.strip(): raise fail("adapter must be a non-empty string naming an adapter kind") if profile.usage_parser not in USAGE_PARSERS: @@ -259,6 +263,32 @@ def fail(msg: str) -> ProfileError: raise fail(f"env_fault_patterns entry is not a valid regex: {pattern!r} ({e})") from e +def _legacy_adapter_default(dialect: str) -> str: + """The adapter kind a TOML profile that predates the ``adapter`` field meant. + + Before the registry, ``hooks.dialect`` WAS the class selector: ``make_adapters`` + sent every hookless profile to the opencode HTTP adapters and everything else to + the generic ones. Project overlays are a documented customization point + (``/.bmad-loop/profiles/*.toml``, same name overrides), and the way to + tweak the opencode profile's ``binary``/``env``/``model`` was to copy the + packaged one — which carried no ``adapter`` key, because the key did not exist. + Taking the dataclass default for those files would silently move a working + hookless run onto tmux, where it launches the CLI in a window and waits out + ``session_timeout_min`` for a ``Stop`` hook a hookless profile never registers — + and ``validate`` stays green, because every check it would trip keys on + ``hookless`` too. So reproduce the old dispatch instead of defaulting. + + Only the absent key takes this path; an explicit ``adapter`` is always honored, + including the now-legal hookless-but-not-``opencode-http`` combination the axes + were decoupled to allow. Naming the two bundled kinds here is a fact about what + the *old* dispatch did, not a valid-kinds set — that set is only ever + ``registry.known_adapter_kinds()``. Imported inside the function so this module + keeps no import-time dependency on the registry.""" + from .registry import GENERIC, OPENCODE_HTTP + + return OPENCODE_HTTP if dialect == "none" else GENERIC + + def _parse_profile(doc: dict, source: str) -> CLIProfile: """Coerce a TOML document into a :class:`CLIProfile`. @@ -291,7 +321,9 @@ def str_list(key: str) -> tuple[str, ...]: # and carry it all the way to `get_adapter_kind`, which would then name that # nonsense as the unknown kind. #384's rule — a malformed value funnels into # ProfileError at the boundary, never a silent coercion. - raw_adapter = doc.get("adapter", "generic") + raw_adapter = doc.get("adapter") + if raw_adapter is None: + raw_adapter = _legacy_adapter_default(str(hooks_d.get("dialect", ""))) if not isinstance(raw_adapter, str): raise fail(f"adapter must be a string: got {type(raw_adapter).__name__}") @@ -394,13 +426,17 @@ def _load_external_profiles() -> dict[str, CLIProfile]: A provider is rejected WHOLE: one invalid profile in the returned batch drops the batch, because ``_coerce_profiles`` raises before any of them is recorded. Deliberate — a provider is one package's declaration, and half-installing it - would leave an operator with a profile set no error message accounts for.""" + would leave an operator with a profile set no error message accounts for. + + Entry points are visited in name order, so which provider wins a name + collision is a property of the packages rather than of ``sys.path`` ordering + (the adapter scan sorts for the same reason).""" global _EXTERNALS_LOADED if _EXTERNALS_LOADED: return _EXTERNAL_PROFILES _EXTERNALS_LOADED = True try: - eps = importlib.metadata.entry_points(group=PROFILES_GROUP) + eps = sorted(importlib.metadata.entry_points(group=PROFILES_GROUP), key=lambda e: e.name) except Exception as exc: # noqa: BLE001 — diagnostics path, never crash loading _PROFILE_LOAD_ERRORS[""] = f"{type(exc).__name__}: {exc}" return _EXTERNAL_PROFILES @@ -417,7 +453,15 @@ def _load_external_profiles() -> dict[str, CLIProfile]: 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.""" + failed to load this process (empty when all loaded). For diagnostics surfaces. + + Performs the scan rather than assuming a neighbouring call already did. The + only other trigger is :func:`load_profiles`, which ``validate`` reaches through + ``get_profile`` — inside the block that a ``PolicyError`` aborts. Reading a map + nothing had populated would report a broken profile package as absent for a + reason having nothing to do with that package, exactly when an operator is + already looking at a broken config. Scan-once still holds.""" + _load_external_profiles() return dict(_PROFILE_LOAD_ERRORS) diff --git a/src/bmad_loop/adapters/registry.py b/src/bmad_loop/adapters/registry.py index e05a3707..4ca17a0b 100644 --- a/src/bmad_loop/adapters/registry.py +++ b/src/bmad_loop/adapters/registry.py @@ -31,9 +31,10 @@ :data:`OPENCODE_HTTP`); out-of-tree kinds arrive at import time, triggered by the ``bmad_loop.adapters`` entry-point scan in :func:`_load_external_adapters` — so a pip/uv co-installed adapter package is selectable with no config step. Builtins -load first, so an external can never shadow a bundled name. A broken third-party -distribution degrades to a recorded, surfaced reason -(:func:`external_adapter_errors`) and can never break selection. +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. **Two deliberate asymmetries versus the multiplexer seam** (this is not a copy-paste omission): @@ -149,25 +150,39 @@ def register_adapter(name: str, needs_mux: bool, load: Callable[[], AdapterBuild """Register an adapter kind. ``name`` is the ``profile.adapter`` key that selects it; ``needs_mux`` declares whether the family drives a terminal multiplexer; ``load`` is the lazy builder thunk. First registration of a name - wins — bundled kinds register from :func:`_load_builtin_adapters` before the - entry-point scan, so an out-of-tree package can never shadow a bundled name. - An out-of-tree kind calls this at import time — no core edit required. There - is no selection cache to invalidate (see the module docstring).""" + wins, and the builtins are 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 kind calls this at import time — no core edit required. There is + no selection cache to invalidate (see the module docstring). + + Seeding on *this* side is what makes first-wins an invariant instead of an + ordering coincidence. An external module runs its ``register_adapter`` calls + as an import side effect, and the import is not always triggered by an + adapter resolution: the documented packaging layout puts both entry points in + one module, so the ``bmad_loop.profiles`` scan in :mod:`~.profile` — which + runs long before any kind is resolved — imports it too, as does any plugin + that imports the package directly. Any of those arriving first would have + ``setdefault`` keep the external under a bundled name and silently redirect + every default profile to it.""" + _load_builtin_adapters() _ADAPTERS.setdefault(name, AdapterKind(name=name, needs_mux=needs_mux, load=load)) def _load_builtin_adapters() -> None: """Register the bundled adapter kinds. Idempotent and lazy (called from the - resolution entry points, not at module import) to stay cycle-safe: the load - thunks import ``generic`` / ``opencode_http``, which import back through the - package. Builtins register before externals so a bundled name keeps - first-wins on any collision.""" + resolution entry points and from :func:`register_adapter`, not at module + import) to stay cycle-safe: the load thunks import ``generic`` / + ``opencode_http``, which import back through the package. Builtins register + before externals so a bundled name keeps first-wins on any collision. + + The flag is set BEFORE the loop because the loop re-enters through + ``register_adapter``; setting it afterwards would recurse without end.""" global _BUILTINS_LOADED if _BUILTINS_LOADED: return + _BUILTINS_LOADED = True for name, needs_mux, load in _BUILTIN_ADAPTERS: register_adapter(name, needs_mux, load) - _BUILTINS_LOADED = True # The entry-point group an out-of-tree adapter package advertises its module @@ -188,13 +203,26 @@ def _load_external_adapters() -> None: adapters`` and the ``validate`` preflight via :func:`external_adapter_errors`), not raised. The loaded-flag is set up front: a third-party import failure is not transient, and retrying on every resolution would re-import (and re-fail) - each time — mirroring the multiplexer's external scan.""" + each time — mirroring the multiplexer's external scan. + + A recorded failure does NOT mean the entry point registered nothing: a module + that registers kind A and then raises while registering kind B leaves A + registered and selectable. Deliberate — unwinding would mean tracking which + names a half-run import claimed, and a kind that registered cleanly is usable + whatever else its package got wrong. The recorded reason is a fact about the + import, not a promise about the registry. + + Entry points are visited in name order. ``importlib.metadata`` yields them in + distribution-discovery order, which varies with ``sys.path``, so without this + two hosts carrying the same packages could resolve a name collision + differently — and first-wins would be a fact about the install rather than + about the packages.""" global _EXTERNALS_LOADED if _EXTERNALS_LOADED: return _EXTERNALS_LOADED = True try: - eps = importlib.metadata.entry_points(group=ADAPTERS_GROUP) + eps = sorted(importlib.metadata.entry_points(group=ADAPTERS_GROUP), key=lambda e: e.name) except Exception as exc: # noqa: BLE001 — diagnostics path, never crash selection _EXTERNAL_ERRORS[""] = f"{type(exc).__name__}: {exc}" return @@ -207,7 +235,13 @@ def _load_external_adapters() -> None: def external_adapter_errors() -> dict[str, str]: """Entry-point name -> failure reason for every external adapter that failed - to load this process (empty when all loaded). For diagnostics surfaces.""" + to load this process (empty when all loaded). For diagnostics surfaces. + + 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".""" + _load_builtin_adapters() + _load_external_adapters() return dict(_EXTERNAL_ERRORS) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 049d3001..2d924d0f 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -809,6 +809,38 @@ def _dev_skill_for_role(pol, project: Path, role: str) -> str: return install.dev_primitive_or_default(project, tree) +def _unknown_adapter_kinds(project: Path, pol) -> list[str]: + """Problem lines for each role-selected profile whose ``adapter`` names no + registered kind — resolved against the live registry, never a hardcoded set. + + Such a profile renders a perfectly plausible dry-run preview (``_render_invocation`` + reads only ``binary``/``launch_args``/``prompt_template``) but aborts the real + run in ``make_adapters``, which is precisely the gap the banner exists to close. + Only the roles a run would actually build are checked; `validate` reports the + same condition over *every* profile as an ``adapter.kind`` finding. + + A profile that will not even parse is skipped rather than reported here: the + renderer immediately after raises the ``ProfileError`` itself, which names the + real problem better than a derived one would.""" + from .adapters.profile import ProfileError, get_profile + from .adapters.registry import known_adapter_kinds + + kinds = known_adapter_kinds() + problems: list[str] = [] + for name in dict.fromkeys(pol.adapter.resolved(role).name for role in ROLES): + try: + profile = get_profile(name, project) + except ProfileError: + continue + if profile.adapter not in kinds: + problems.append( + f"profile {profile.name!r} names unknown adapter kind " + f"{profile.adapter!r} — known: {', '.join(kinds)} " + f"(install the plugin that provides it, or fix the profile's `adapter`)" + ) + return problems + + def _warn_preflight_would_abort( paths: bmadconfig.ProjectPaths, pol, *, require_stories: bool = False ) -> None: @@ -821,9 +853,11 @@ def _warn_preflight_would_abort( ``/bmad-dev-auto`` reads fine and would HALT an unattended session on the shim's interactive migration gate. - Mirrors both of the refusals the dry-run's early return skips past, and only - those: the finding list `_require_base_skills` gates on, and the #414 isolation - conflict `_reject_isolation_conflict` refuses ahead of it. Reading the same + Mirrors the refusals the dry-run's early return skips past, and only those: + the finding list `_require_base_skills` gates on, the #414 isolation conflict + `_reject_isolation_conflict` refuses ahead of it, and the unregistered adapter + kind `make_adapters` aborts on (`_unknown_adapter_kinds` — a preview reads none + of the fields that would give the misconfiguration away). Reading the same sources as the gates themselves is what keeps the preview from disagreeing with the real command about what "runnable" means. Severity-filtered to `problem` for that same reason — `_require_base_skills` ignores warnings, so reporting one @@ -849,6 +883,7 @@ def _warn_preflight_would_abort( conflict = bmadconfig.worktree_isolation_conflict(paths, pol.scm.isolation) if conflict is not None: problems.insert(0, conflict) + problems += _unknown_adapter_kinds(paths.project, pol) if not problems: return print( diff --git a/src/bmad_loop/runsetup.py b/src/bmad_loop/runsetup.py index eb163694..603edc4e 100644 --- a/src/bmad_loop/runsetup.py +++ b/src/bmad_loop/runsetup.py @@ -456,7 +456,24 @@ def make_adapters( kind = get_adapter_kind(profile.adapter) except AdapterError as e: raise SystemExit(f"error: profile {profile.name!r}: {e}") from e - builder = kind.load() + # The load thunk is where a family's classes — and any optional + # dependency they pull in — are first imported, and it is deliberately + # never invoked by `validate` or `bmad-loop adapters` (both stay free + # of heavy imports), so a thunk that raises has had no earlier gate. + # By here `compose_run` has already written the run state and pid, so + # an escaping ImportError strands a run directory behind a traceback. + # ImportError ONLY, on the same rule as `construct_error` below: a + # missing dependency is a lazy loader's DECLARED failure, while + # anything else is a bug in that package and must surface as itself + # rather than as a misleading `error:` line. Widening this to + # `Exception` would contradict the pin two tests down. + try: + builder = kind.load() + except ImportError as e: + raise SystemExit( + f"error: profile {profile.name!r}: adapter kind " + f"{profile.adapter!r} failed to load: {type(e).__name__}: {e}" + ) from e # Annotated: the literal below would otherwise fix the value type to # `Path | CLIProfile`, and the `needs_mux` arm adds a multiplexer. common: dict[str, object] = dict( diff --git a/tests/test_adapter_registry.py b/tests/test_adapter_registry.py index a3a45752..c631b995 100644 --- a/tests/test_adapter_registry.py +++ b/tests/test_adapter_registry.py @@ -32,7 +32,7 @@ import json import pytest -from conftest import install_bmad_config +from conftest import install_bmad_config, write_sprint from bmad_loop import cli from bmad_loop import policy as policy_mod @@ -266,6 +266,53 @@ def load(): assert kind.load().plain.__name__ == "GenericAdapter" +def test_builtins_win_over_an_external_imported_by_the_profile_scan( + fresh_adapter_registry, monkeypatch +): + """The shadowing hole that first-wins ALONE does not close. + + The documented packaging layout puts both entry points in one module, so the + ``bmad_loop.profiles`` scan imports it too — and that scan runs on any + ``load_profiles`` call, which every command makes long before a kind is ever + resolved. The external's import-time ``register_adapter`` therefore lands + before ``_load_builtin_adapters`` would have run, and ``setdefault`` keeps it + under the bundled name: every default profile silently redirects to a + third-party class. Only ``register_adapter`` seeding the builtins on its own + side makes first-wins an invariant instead of an ordering coincidence. + + ABLATION: drop the ``_load_builtin_adapters()`` call from ``register_adapter`` + and this reddens — while ``test_builtins_first_wins_over_external`` above stays + green, because ``get_adapter_kind`` seeds the builtins on that path in.""" + registry = fresh_adapter_registry + + def provider(): + return [ + CLIProfile(name="ext", binary="ext", adapter="generic", hooks=HookSpec("none", "", {})) + ] + + def ep_load(): + # The import side effect of a module carrying BOTH entry points: a clumsy + # (or hostile) external claiming the builtin name with wrong needs_mux. + registry.register_adapter("generic", needs_mux=False, load=lambda: _stub_builder()) + return provider + + def fake_entry_points(*, group): + assert group == profile_mod.PROFILES_GROUP + return [_FakeEntryPoint("dual", ep_load)] + + monkeypatch.setattr(profile_mod.importlib.metadata, "entry_points", fake_entry_points) + profile_mod._EXTERNALS_LOADED = False # re-arm the scan the fixture parks + + # The real process order: profiles resolve first (cmd_adapters and cmd_run + # both do), and nothing has touched the adapter registry yet. + assert "ext" in profile_mod.load_profiles(None) + assert registry._ADAPTERS, "the external registered — otherwise this proves nothing" + + kind = registry.get_adapter_kind("generic") + assert kind.needs_mux is True # the builtin, not the external + assert kind.load().plain.__name__ == "GenericAdapter" + + def test_broken_entry_point_degrades_and_is_recorded(scan_adapter_registry): """A distribution whose import blows up must not break selection: the builtins still resolve, and the failure is recorded for adapters/validate to show.""" @@ -533,6 +580,54 @@ def __init__(self, **kwargs): runsetup.make_adapters(project.project, _run_dir(project.project), pol) +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 + profile and the kind, not a raw traceback. + + This is the one failure mode with no earlier gate: `validate` and `bmad-loop + adapters` both deliberately avoid invoking the thunk, and by the time + `make_adapters` runs, `compose_run` has already written the run state and pid, + so an escaping ImportError strands a run directory behind a traceback. + + ABLATION: drop the try/except around `kind.load()` and this reddens (the + ModuleNotFoundError propagates as itself).""" + + def _load(): + raise ModuleNotFoundError("No module named 'acmesdk'") + + fresh_adapter_registry.register_adapter("lazyboom", needs_mux=False, load=_load) + install_bmad_config(project) + _write_profile(project.project, "lazyboom", adapter="lazyboom") + _write_policy(project.project, '[adapter]\nname = "lazyboom"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + with pytest.raises(SystemExit, match=r"lazyboom.*failed to load.*acmesdk"): + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + + +def test_make_adapters_non_import_thunk_failure_is_not_swallowed(fresh_adapter_registry, project): + """The other half of the pair above, and the same rule `construct_error` follows: + a missing dependency is a lazy loader's DECLARED failure, but anything else is a + bug in that package and must surface as itself. Swallowing it would hand an + adapter author an `error:` line where the traceback was the whole diagnosis. + + ABLATION: widen the `except ImportError` to `except Exception` and this reddens + (SystemExit is raised instead).""" + + def _load(): + raise ZeroDivisionError("a real bug, not a missing dependency") + + fresh_adapter_registry.register_adapter("lazybug", needs_mux=False, load=_load) + install_bmad_config(project) + _write_profile(project.project, "lazybug", adapter="lazybug") + _write_policy(project.project, '[adapter]\nname = "lazybug"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + with pytest.raises(ZeroDivisionError): + runsetup.make_adapters(project.project, _run_dir(project.project), pol) + + def test_make_adapters_unknown_kind_systemexit_names_profile(fresh_adapter_registry, project): """A profile whose ``adapter`` names no registered kind aborts the run with a SystemExit that names both the profile and the known kinds.""" @@ -682,6 +777,68 @@ def test_validate_flags_an_unregistered_adapter_kind(fresh_adapter_registry, pro assert "ghostkind" in findings[0]["message"] and "generic" in findings[0]["message"] +def test_dry_run_says_an_unregistered_kind_would_abort(fresh_adapter_registry, project, capsys): + """`--dry-run` renders a preview from `binary`/`launch_args`/`prompt_template` + alone, so an unregistered `adapter` is invisible in it: the operator reads a + perfectly plausible invocation for a config `make_adapters` refuses to build. + That is exactly the gap the honesty banner exists to close, so the unknown kind + joins the refusals it already mirrors. + + Same contract as the other banner sources: stderr-only, the schedule still + renders on stdout, and the exit code stays 0 — a dry-run is a diagnostic. + + ABLATION: drop the `_unknown_adapter_kinds` call from + `_warn_preflight_would_abort` and this reddens (stderr is empty, rc still 0).""" + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + _write_profile(project.project, "weird", adapter="ghostkind") + _write_policy(project.project, '[adapter]\nname = "weird"\n') + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + args = argparse.Namespace(epic=None, story=None, max_stories=None) + + assert cli._dry_run(project, pol, args) == 0 + out, err = capsys.readouterr() + assert "NOT runnable" in err + assert "ghostkind" in err and "generic" in err + assert "1-1-a" in out # the schedule itself still rendered + + +def test_validate_reports_a_broken_profile_package_even_when_policy_fails( + fresh_adapter_registry, project, monkeypatch, capsys +): + """A broken profile package must be reported for its OWN reason, not silently + dropped because something else in the config is also wrong. + + The `bmad_loop.profiles` scan has exactly one other trigger — `load_profiles`, + which validate reaches via `get_profile`, inside the block a `PolicyError` + aborts. Reading the error map without scanning would print nothing here, which + an operator reads as "no profile package failed". The adapter half never had + the gap, because `known_adapter_kinds()` runs unconditionally. + + ABLATION: drop the `_load_external_profiles()` call from + `external_profile_errors` and this reddens (no adapter.external-profile + finding) while the `policy` failure below still reports.""" + + def fake_entry_points(*, group): + assert group == profile_mod.PROFILES_GROUP + + def boom(): + raise ImportError("No module named 'ghost_profile_dep'") + + return [_FakeEntryPoint("brokenprofiles", boom)] + + monkeypatch.setattr(profile_mod.importlib.metadata, "entry_points", fake_entry_points) + profile_mod._EXTERNALS_LOADED = False # re-arm the scan the fixture parks + + install_bmad_config(project) + _write_policy(project.project, "this is not = valid toml [[[\n") + + findings = _validate_findings(project.project, capsys) + assert [f["severity"] for f in findings if f["check"] == "policy"] == ["problem"] + external = [f for f in findings if f["check"] == "adapter.external-profile"] + assert [f["severity"] for f in external] == ["warning"] + assert "ghost_profile_dep" in external[0]["message"] + + def test_validate_warns_on_a_broken_external_package(scan_adapter_registry, project, capsys): """A half-installed out-of-tree package is a WARNING, not a failure: selection already degraded past it, so the same non-blocking treatment a failed mux diff --git a/tests/test_profile.py b/tests/test_profile.py index b5805c8a..227d7301 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -192,6 +192,46 @@ def test_adapter_field_defaults_to_generic_and_parses(): ) +def test_absent_adapter_on_a_hookless_profile_keeps_the_pre_registry_dispatch(tmp_path): + """Back-compat for the TOML files this field did not exist in. + + Before the registry, `hooks.dialect = "none"` WAS the class selector — every + hookless profile went to the opencode HTTP adapters. Copying the packaged + opencode profile into `.bmad-loop/profiles/` to tweak binary/env/model is the + documented customization, and that copy carries no `adapter` key. Taking the + dataclass default would move it onto the tmux generic adapter, where it waits + out `session_timeout_min` for a `Stop` hook a hookless profile never registers + — and every `validate` check that would catch it keys on `hookless` too, so the + preflight stays green. Non-hookless profiles keep defaulting to generic. + + ABLATION: restore `doc.get("adapter", "generic")` and the hookless row reddens + (it resolves to `generic`) while the dialect row stays green.""" + profiles_dir = tmp_path / ".bmad-loop" / "profiles" + profiles_dir.mkdir(parents=True) + # The pre-field bytes: hookless, and no `adapter` key anywhere. + (profiles_dir / "legacy.toml").write_text(HOOKLESS_PROFILE) + (profiles_dir / "hooked.toml").write_text(MINIMAL_PROFILE) + + profiles = load_profiles(tmp_path) + assert profiles["mycli-http"].hookless + assert profiles["mycli-http"].adapter == "opencode-http" + assert profiles["mycli"].adapter == "generic" + + +def test_explicit_adapter_beats_the_hookless_back_compat_default(tmp_path): + """The back-compat default fires ONLY on the absent key. An explicit `adapter` + is always honored — including hookless-driven-by-something-else, which is the + decoupling the registry exists to allow and which the old dispatch could not + express.""" + profiles_dir = tmp_path / ".bmad-loop" / "profiles" + profiles_dir.mkdir(parents=True) + (profiles_dir / "decoupled.toml").write_text( + HOOKLESS_PROFILE.replace("[hooks]", 'adapter = "some-other-http-kind"\n[hooks]') + ) + profile = load_profiles(tmp_path)["mycli-http"] + assert profile.hookless and profile.adapter == "some-other-http-kind" + + def test_adapter_kind_membership_is_not_checked_at_parse_time(tmp_path): """A profile naming an unregistered adapter kind still PARSES — validity is enforced later against the live registry (at construction / by `validate`), @@ -462,10 +502,21 @@ def test_every_toml_value_type_parses_or_raises_profile_error(tmp_path, key, val @pytest.fixture def profile_scan(monkeypatch): """Isolate + re-arm the profile entry-point scan: snapshot/clear the module's - external-scan state, then hand back a hook to install fake entry points.""" + external-scan state, then hand back a hook to install fake entry points. + + Setup parks the scan as ALREADY-LOADED over an empty map rather than leaving + whatever a previous test wrote — a test that takes this fixture without + calling `arm` would otherwise assert against leftovers and pass for the wrong + reason. Parked, not re-armed: arming without a fake `entry_points` in place + would run the REAL scan and leak whichever profile packages the dev box + happens to have installed into the assertions. `arm` re-opens it, exactly as + `fresh_adapter_registry` does for the adapter half.""" saved_loaded = profile_mod._EXTERNALS_LOADED saved_profiles = dict(profile_mod._EXTERNAL_PROFILES) saved_errors = dict(profile_mod._PROFILE_LOAD_ERRORS) + profile_mod._EXTERNALS_LOADED = True + profile_mod._EXTERNAL_PROFILES.clear() + profile_mod._PROFILE_LOAD_ERRORS.clear() def arm(*eps, scan_error=None): def fake_entry_points(*, group): @@ -631,6 +682,10 @@ def test_profile_scan_failure_degrades(profile_scan): ({"usage_grace_s": -1.0}, "usage_grace_s"), ({"stop_without_result_nudges": -2}, "stop_without_result_nudges"), ({"adapter": ""}, "adapter"), + # whitespace-only, not just empty: the TOML route strips before validating, + # so testing the raw value would refuse `adapter = " "` from a file while + # admitting it from a provider — the divergence this whole test denies + ({"adapter": " "}, "adapter"), ({"binary": " "}, "required"), ], ) From 98f0bb7fef86e29687b7e9e1c1b6e957bdd20523 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 14:32:49 -0700 Subject: [PATCH 3/6] fix(adapters): key validate's model-format check on the adapter kind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2 of the review gate (codex, one P2). `policy.model-qualified` is `adapter.httpx`'s sibling and needed the same re-keying this PR already gave httpx: "provider/model" is the opencode SERVER's config-file spelling, a fact about one adapter class, not about whether a profile registers hooks. Those were one question only while `hookless` selected the builder. Keyed on `hookless` the check is wrong in both directions. It warns an out-of-tree hookless family whose server takes bare model names, naming an opencode convention that family does not use; and it stays silent for an `opencode-http` profile carrying a hook dialect — legal once the axes decoupled, and exactly where a bare name does fall back to the server default. A test pins each direction, and the two pre-existing model-qualified tests are the control: the packaged opencode profile is both hookless and `opencode-http`, so the common case is untouched. Swept the rest of the `hookless` call sites — validate's hook-config skip, `_register_hooks`, the worktree hook shield, the config digest, `probe-adapter` — and they are all genuinely about transport. This was the only site keyed on the wrong axis. Also states, at the `adapter` field itself, that the two profile routes bind its "hookless MUST name an HTTP kind" rule differently: a TOML profile omitting the key keeps the old dialect dispatch, while a provider constructing the dataclass takes the default verbatim. --- CHANGELOG.md | 11 ++++--- src/bmad_loop/adapters/profile.py | 9 ++++++ src/bmad_loop/cli.py | 17 ++++++++++- tests/test_adapter_registry.py | 51 +++++++++++++++++++++++++++++++ 4 files changed, 83 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4face5e6..9081f397 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -140,10 +140,13 @@ whose seams had diverged enough that several ports needed a different fix, and t underneath it. The digest resolves the kind from the profile bytes it was handed, not a second read. -- **`validate`'s httpx check keys on the adapter kind, not hooklessness.** `httpx` is the - `opencode-http` family's optional extra; with the transport and driving class now separate axes, a - hookless profile driven by another kind no longer FAILs with a remedy that installs the wrong - package. +- **`validate`'s httpx and model-format checks key on the adapter kind, not hooklessness.** `httpx` + is the `opencode-http` family's optional extra and `provider/model` is its server's config-file + spelling; both are facts about one adapter class, not about whether a profile registers hooks. + With the transport and driving class now separate axes, a hookless profile driven by another kind + no longer FAILs with a remedy that installs the wrong package, nor draws a `policy.model-qualified` + warning naming a convention it does not use — and an `opencode-http` profile carrying a hook + dialect now gets the model warning it always needed. - **A profile written before the `adapter` field keeps its old dispatch.** `hooks.dialect = "none"` used to be the class selector, so a project overlay copied from the packaged opencode profile diff --git a/src/bmad_loop/adapters/profile.py b/src/bmad_loop/adapters/profile.py index 2f628ee7..e71d8d69 100644 --- a/src/bmad_loop/adapters/profile.py +++ b/src/bmad_loop/adapters/profile.py @@ -96,6 +96,15 @@ class CLIProfile: # `validate` finding, both against the live registry. A hookless HTTP profile # (hooks.dialect = "none") MUST set this to its HTTP adapter kind — the # transport (hookless) and the driving class are now decoupled axes. + # + # That MUST binds the two routes differently, which is why the default below + # is not the whole story. A TOML profile that OMITS the key is read as + # predating the field and keeps the old dialect-based dispatch + # (`_legacy_adapter_default`), so a hookless file still lands on the HTTP + # kind. A provider that constructs this dataclass directly takes the default + # verbatim — nothing infers a kind for it — so an entry-point profile really + # must set the field, and a hookless one that leaves it at "generic" resolves + # to a mux-driving kind that will never see a Stop hook. adapter: str = "generic" # project-relative tree this CLI reads skills from, e.g. ".claude/skills" # (claude) or ".agents/skills" (codex/gemini); `bmad-loop init` installs the diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 2d924d0f..0db5f723 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -574,11 +574,26 @@ def cmd_validate(args: argparse.Namespace) -> int: # opencode config-file model ids are "provider/model" (see the opencode_http docstring); # a bare model name silently falls back to the server's default model, so warn # (advisory — a note, not a FAIL: an empty model legitimately means "default"). + # + # Keyed on the adapter KIND, for the same reason `adapter.httpx` above is: the + # "provider/model" spelling is a fact about the opencode server's config file, + # not about whether a profile registers hooks. Those were one question only + # while `hookless` selected the builder; the registry decoupled them, and + # keying on `hookless` is now wrong in BOTH directions — it warns an + # out-of-tree hookless family whose server takes bare model names, naming a + # convention that family does not use, and it stays silent for an + # `opencode-http` profile carrying a hook dialect, which is exactly where the + # bare name really does fall back to the server default. if pol is not None: for role in ROLES: cfg = pol.adapter.resolved(role) prof = profile_by_name.get(cfg.name) - if prof is not None and prof.hookless and cfg.model and "/" not in cfg.model: + if ( + prof is not None + and prof.adapter == adapter_registry.OPENCODE_HTTP + and cfg.model + and "/" not in cfg.model + ): report.warn( "policy.model-qualified", f"{role} model {cfg.model!r} is not 'provider/model' — " diff --git a/tests/test_adapter_registry.py b/tests/test_adapter_registry.py index c631b995..f3c2a770 100644 --- a/tests/test_adapter_registry.py +++ b/tests/test_adapter_registry.py @@ -763,6 +763,57 @@ def test_validate_httpx_check_keys_on_the_adapter_kind_not_hooklessness( assert [f["severity"] for f in findings if f["check"] == "adapter.kind"] == ["ok"] +def test_validate_model_format_check_keys_on_the_adapter_kind_not_hooklessness( + fresh_adapter_registry, project, capsys +): + """`policy.model-qualified` is the httpx check's sibling and needed the same + re-keying: "provider/model" is a fact about the opencode SERVER's config file, + not about whether a profile registers hooks. An out-of-tree hookless family + whose server takes bare model names must draw no warning naming a spelling it + does not use. + + The `adapter.hookless` assert is the positive control, and the point of the + test: the profile IS hookless, so the old predicate would have fired here. The + absent warning is therefore the re-keying and not a profile that failed to + load, a model that never reached the check, or a validate that bailed early. + + ABLATION: re-key the check on `prof.hookless` and the `not any(...)` reddens.""" + fresh_adapter_registry.register_adapter("hermes", needs_mux=False, load=lambda: _stub_builder()) + install_bmad_config(project) + _write_profile(project.project, "hermes", adapter="hermes") # hookless=True + _write_policy(project.project, '[adapter]\nname = "hermes"\nmodel = "haiku"\n') + + findings = _validate_findings(project.project, capsys) + assert not any(f["check"] == "policy.model-qualified" for f in findings) + # controls: the profile loaded, its kind resolved, and it really is hookless + assert [f["severity"] for f in findings if f["check"] == "adapter.kind"] == ["ok"] + assert any(f["check"] == "adapter.hookless" for f in findings) + + +def test_validate_model_format_warns_on_an_opencode_kind_carrying_a_hook_dialect( + fresh_adapter_registry, project, capsys +): + """The other direction of the same miss: keyed on `hookless`, the check also + UNDER-fires. Decoupling the axes made `opencode-http` beside a hook dialect a + legal profile, and its bare model still falls back to the server's default — + the case the warning exists for — while `prof.hookless` reads False. + + ABLATION: re-key the check on `prof.hookless` and this reddens (no finding at + all, because the profile is not hookless).""" + install_bmad_config(project) + _write_profile(project.project, "ochooked", adapter="opencode-http", hookless=False) + _write_policy(project.project, '[adapter]\nname = "ochooked"\nmodel = "haiku"\n') + + findings = [ + f + for f in _validate_findings(project.project, capsys) + if f["check"] == "policy.model-qualified" + ] + assert findings, "a bare model on the opencode-http kind must warn" + assert {f["severity"] for f in findings} == {"warning"} + assert all("haiku" in f["message"] for f in findings) + + def test_validate_flags_an_unregistered_adapter_kind(fresh_adapter_registry, project, capsys): """`adapter.kind` is resolved against the live registry, so a profile naming a kind no installed package provides is a FAIL that names the known set.""" From 90a7ca930de62e99414c4e7d6d919bd2c5f41ea0 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 14:44:13 -0700 Subject: [PATCH 4/6] fix(adapters): refuse hookless+generic, and order the scans by distribution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3 of the review gate (codex P2, plus two independent passes that reached the same defect from the other side). Refuse a hookless profile that selects `generic`. That adapter injects into a tmux window and completes on a Stop hook; `dialect = "none"` means none is ever registered, so the pair only ever waits out `session_timeout_min` against a CLI that never exits — with `validate` green, since every check it would trip keys on `hookless` too. The rule lands in `_validate_profile` because that is the one point BOTH routes pass through, and both could reach the pair: a TOML file naming it outright, and an entry-point provider that builds a hookless HookSpec while leaving `adapter` at its dataclass default. The absent-key TOML case was already steered away by `_legacy_adapter_default`, which is precisely why the Python route needed saying out loud — it has no absent-key to detect, so the two routes disagreed about identical profile content. Naming one bundled kind is a fact about that adapter's completion contract, the same latitude the httpx check takes; hookless on any other kind stays legal. Order both entry-point scans by (name, distribution). Sorting on the name alone did not deliver what its docstring claimed: `entry_points(group=...)` does not dedup across distributions, so two packages advertising the same name come back as two entries, and `sorted` is stable — the tie fell straight back to `sys.path` order. That tie is the whole case the sort exists for, since a package conventionally names its entry point after the kind it registers. Verified against real `*.dist-info` metadata on 3.11 and 3.13: with a name-only key the winner flips with `sys.path` order; with the distribution in the key it does not. The test doubles gain a `dist` to match. The shadowing test's provider moves off `generic`/hookless — that pairing was incidental to what it proves, and is now refused. --- CHANGELOG.md | 7 ++++ src/bmad_loop/adapters/profile.py | 39 +++++++++++++++++-- src/bmad_loop/adapters/registry.py | 23 ++++++++--- tests/test_adapter_registry.py | 61 ++++++++++++++++++++++++++++-- tests/test_profile.py | 56 ++++++++++++++++++++++++++- 5 files changed, 171 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9081f397..91d08ff7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -154,6 +154,13 @@ whose seams had diverged enough that several ports needed a different fix, and t generic adapter, where it would have waited out `session_timeout_min` for a hook a hookless profile never registers. An explicit `adapter` is always honored, including hookless driven by another kind. +- **A hookless profile can no longer select the `generic` adapter.** `generic` completes on a Stop + hook and `dialect = "none"` means none is ever registered, so the pair described a session that + could only wait out `session_timeout_min` against a CLI that never exits — with `validate` green. + Both routes into the profile map now refuse it: a TOML file naming the pair outright, and an + entry-point provider that builds a hookless profile while leaving `adapter` at its default. + Hookless on any other kind stays legal — that decoupling is what the registry is for. + - **Profiles from a `bmad_loop.profiles` entry point are validated like TOML ones.** Both routes into the profile map now share one invariant set (hook dialect, path containment, `env_fault_patterns` compilation, …), so a package can no longer install a profile state the parser would refuse — an diff --git a/src/bmad_loop/adapters/profile.py b/src/bmad_loop/adapters/profile.py index e71d8d69..efc3a304 100644 --- a/src/bmad_loop/adapters/profile.py +++ b/src/bmad_loop/adapters/profile.py @@ -238,6 +238,31 @@ def fail(msg: str) -> ProfileError: if not profile.adapter.strip(): raise fail("adapter must be a non-empty string naming an adapter kind") + # The one coherence rule between the two axes, and the only place both routes + # pass through. `generic` is the bundled tmux adapter: it injects into a window + # and completes on a Stop hook (or the window dying), so pairing it with + # dialect = "none" — which means nothing ever registers that hook — describes a + # session that can only wait out `session_timeout_min` against an interactive + # CLI that never exits. Both routes could reach it: a TOML file naming the pair + # outright, and an entry-point provider that builds a hookless `HookSpec` while + # leaving `adapter` at its dataclass default. (The absent-key TOML case cannot: + # `_legacy_adapter_default` sends a hookless file to the HTTP kind.) + # + # This is the membership check's opposite, not an instance of it: naming ONE + # bundled kind is a fact about that adapter's completion contract, which this + # package owns, and the same latitude `validate`'s httpx check takes. Hookless + # on any OTHER kind stays legal — that decoupling is what the registry is for, + # and an out-of-tree kind's completion contract is its own to state. + from .registry import GENERIC + + if profile.hookless and profile.adapter.strip() == GENERIC: + raise fail( + f'hookless profiles (dialect = "none") cannot select the {GENERIC!r} adapter: ' + "it completes on a Stop hook a hookless profile never registers, so the " + "session would wait out session_timeout_min. Name the adapter kind that " + "drives this CLI over its own transport." + ) + if profile.usage_parser not in USAGE_PARSERS: raise fail( f"usage_parser must be one of {sorted(USAGE_PARSERS)}: got {profile.usage_parser!r}" @@ -437,15 +462,21 @@ def _load_external_profiles() -> dict[str, CLIProfile]: Deliberate — a provider is one package's declaration, and half-installing it would leave an operator with a profile set no error message accounts for. - Entry points are visited in name order, so which provider wins a name - collision is a property of the packages rather than of ``sys.path`` ordering - (the adapter scan sorts for the same reason).""" + Entry points are visited in (name, distribution) order, so which provider wins + a name collision is a property of the packages rather than of ``sys.path`` + ordering. The distribution is part of the key because the name alone is not a + total order — ``entry_points(group=...)`` does not dedup across distributions, + and ``sorted`` being stable would resolve a same-name tie back into discovery + order (the adapter scan sorts on the same key, for the same reason).""" global _EXTERNALS_LOADED if _EXTERNALS_LOADED: return _EXTERNAL_PROFILES _EXTERNALS_LOADED = True try: - eps = sorted(importlib.metadata.entry_points(group=PROFILES_GROUP), key=lambda e: e.name) + eps = sorted( + importlib.metadata.entry_points(group=PROFILES_GROUP), + key=lambda e: (e.name, getattr(e.dist, "name", "") or ""), + ) except Exception as exc: # noqa: BLE001 — diagnostics path, never crash loading _PROFILE_LOAD_ERRORS[""] = f"{type(exc).__name__}: {exc}" return _EXTERNAL_PROFILES diff --git a/src/bmad_loop/adapters/registry.py b/src/bmad_loop/adapters/registry.py index 4ca17a0b..d5442980 100644 --- a/src/bmad_loop/adapters/registry.py +++ b/src/bmad_loop/adapters/registry.py @@ -212,17 +212,28 @@ def _load_external_adapters() -> None: whatever else its package got wrong. The recorded reason is a fact about the import, not a promise about the registry. - Entry points are visited in name order. ``importlib.metadata`` yields them in - distribution-discovery order, which varies with ``sys.path``, so without this - two hosts carrying the same packages could resolve a name collision - differently — and first-wins would be a fact about the install rather than - about the packages.""" + 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 resolve a + collision differently — first-wins 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. That tie is the whole case the sort exists for: a package + conventionally names its entry point after the kind it registers, so packages + that collide on a kind normally collide on the entry-point name too.""" global _EXTERNALS_LOADED if _EXTERNALS_LOADED: return _EXTERNALS_LOADED = True try: - eps = sorted(importlib.metadata.entry_points(group=ADAPTERS_GROUP), key=lambda e: e.name) + eps = sorted( + importlib.metadata.entry_points(group=ADAPTERS_GROUP), + key=lambda e: (e.name, getattr(e.dist, "name", "") or ""), + ) except Exception as exc: # noqa: BLE001 — diagnostics path, never crash selection _EXTERNAL_ERRORS[""] = f"{type(exc).__name__}: {exc}" return diff --git a/tests/test_adapter_registry.py b/tests/test_adapter_registry.py index f3c2a770..596cd69d 100644 --- a/tests/test_adapter_registry.py +++ b/tests/test_adapter_registry.py @@ -69,12 +69,22 @@ def _stub_builder(*, construct_error=()): return AdapterBuilder(plain=_StubAdapter, dev=_StubDevAdapter, construct_error=construct_error) +class _FakeDist: + """Stands in for ``EntryPoint.dist``; the scan orders on its ``.name``.""" + + def __init__(self, name): + self.name = name + + class _FakeEntryPoint: - """Duck-typed importlib.metadata.EntryPoint: the loader only touches - ``.name`` and ``.load()``.""" + """Duck-typed importlib.metadata.EntryPoint: the loader touches ``.name``, + ``.dist`` (the scan's tiebreak — see `_load_external_adapters`) 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): @@ -286,8 +296,12 @@ def test_builtins_win_over_an_external_imported_by_the_profile_scan( registry = fresh_adapter_registry def provider(): + # Any loadable profile does — it exists so the scan has something to + # accept. The shadowing is the entry point's import side effect below, + # not anything about this profile. (Its kind is deliberately not + # `generic`: hookless + `generic` is refused as incoherent.) return [ - CLIProfile(name="ext", binary="ext", adapter="generic", hooks=HookSpec("none", "", {})) + CLIProfile(name="ext", binary="ext", adapter="ext-http", hooks=HookSpec("none", "", {})) ] def ep_load(): @@ -368,6 +382,45 @@ def load(): assert len(calls) == 1 +@pytest.mark.parametrize( + "order", [("alpha", "zeta"), ("zeta", "alpha")], ids=["alpha-discovered-first", "zeta-first"] +) +def test_same_named_entry_points_resolve_by_distribution_not_install_order( + scan_adapter_registry, order +): + """Two distributions may advertise the SAME entry-point name in one group — + `entry_points(group=...)` does not dedup across distributions — and a package + conventionally names its entry point after the kind it registers, so packages + colliding on a kind normally arrive as a NAME collision too. `sorted` is + stable, so a name-only key resolves that tie in distribution-discovery order, + which is `sys.path` order: the very same two packages would then pick + different winners on two hosts. Ordering on the distribution as well is what + makes first-wins a fact about the packages. + + Both parameters arm the identical pair and differ only in the order the scan + yields them; `alpha-adapter` must win either way. + + ABLATION: drop the `getattr(e.dist, ...)` half of the sort key in + `_load_external_adapters` and the `zeta-first` case reddens (needs_mux True) + 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_adapter_registry + + def register(needs_mux): + def load(): + registry.register_adapter("acme", needs_mux=needs_mux, load=lambda: _stub_builder()) + + return load + + eps = { + "alpha": _FakeEntryPoint("acme", register(False), dist="alpha-adapter"), + "zeta": _FakeEntryPoint("acme", register(True), dist="zeta-adapter"), + } + arm(*(eps[k] for k in order)) + + assert registry.get_adapter_kind("acme").needs_mux is False # alpha-adapter's + + 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_profile.py b/tests/test_profile.py index 227d7301..8a26ddbf 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -232,6 +232,48 @@ def test_explicit_adapter_beats_the_hookless_back_compat_default(tmp_path): assert profile.hookless and profile.adapter == "some-other-http-kind" +def test_hookless_profile_cannot_select_the_generic_adapter(tmp_path): + """The one coherence rule between the two axes, written out longhand. + + `generic` is the bundled tmux adapter and completes on a Stop hook (or the + window dying); `dialect = "none"` means nothing ever registers one. The pair + therefore describes a session that can only wait out `session_timeout_min` + against an interactive CLI that never exits — the exact failure + `_legacy_adapter_default` steers the ABSENT-key file away from, which an + explicit file could still spell out. + + ABLATION: drop the `profile.hookless and ... == GENERIC` guard from + `_validate_profile` and this reddens (the profile loads happily).""" + profiles_dir = tmp_path / ".bmad-loop" / "profiles" + profiles_dir.mkdir(parents=True) + (profiles_dir / "hangy.toml").write_text( + HOOKLESS_PROFILE.replace("[hooks]", 'adapter = "generic"\n[hooks]') + ) + with pytest.raises(ProfileError, match="cannot select the 'generic' adapter"): + load_profiles(tmp_path) + + +def test_entry_point_profile_at_the_default_adapter_while_hookless_is_refused(profile_scan): + """The same pair by the route `_legacy_adapter_default` cannot reach — and the + reason the rule lives in `_validate_profile` rather than in the TOML parser. + + A provider that builds a hookless `HookSpec` and never sets `adapter` takes the + dataclass default `generic`. Content a TOML file expresses by OMITTING the key, + which the parser steers to the HTTP kind; the Python route has no absent-key to + detect, so without this rule the two routes disagree about the same profile and + the provider's silently hangs at run time. Dropped with a reason instead. + + ABLATION: drop the guard and `acme` loads with `adapter == "generic"`.""" + profile_scan( + _FakeEntryPoint( + "acme", + lambda: [CLIProfile(name="acme", binary="acme", hooks=HookSpec("none", "", {}))], + ) + ) + assert "acme" not in load_profiles() + assert "generic" in profile_mod.external_profile_errors()["acme"] + + def test_adapter_kind_membership_is_not_checked_at_parse_time(tmp_path): """A profile naming an unregistered adapter kind still PARSES — validity is enforced later against the live registry (at construction / by `validate`), @@ -539,9 +581,21 @@ def fake_entry_points(*, group): profile_mod._PROFILE_LOAD_ERRORS.update(saved_errors) +class _FakeDist: + """Stands in for ``EntryPoint.dist``; the scan orders on its ``.name``.""" + + def __init__(self, name): + self.name = name + + class _FakeEntryPoint: - def __init__(self, name, load): + """Duck-typed EntryPoint. ``dist`` is the scan's ordering tiebreak (see + `_load_external_profiles`) and defaults to a distinct-per-name stand-in, so + only a test that sets it can decide a same-name collision.""" + + 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): From f13879ed47370533baf27658c9e9f6f4973850a4 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 14:46:22 -0700 Subject: [PATCH 5/6] docs(adapters): state the refused hookless+generic pairing in the field reference The `adapter` row claimed independence from `hooks.dialect` outright. That is now two qualifications short: the absent-key back-compat carve-out it already described, and the explicit `generic`/`none` pairing refused at load. Also says the part a provider needs: hookless profiles must set `adapter` rather than inherit the default. --- docs/adapter-authoring-guide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 75f2922f..8e9b6501 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -406,7 +406,7 @@ resolves to `claude`. | `name` | ✅ | — | Profile id, also the `--cli` value and override key. | | `binary` | ✅ | — | Executable to launch (resolved on `PATH`). | | `[hooks]` | ✅ | — | The `HookSpec` table (see below). | -| `adapter` | | `generic` | Which adapter **class** drives this CLI — a key resolved against the [adapter registry](#shipping-a-new-adapter-class-out-of-tree), not a fixed enum. `generic` = the bundled tmux + hook-signal adapter; `opencode-http` = the bundled HTTP/SSE adapter; an out-of-tree package registers its own. Membership is checked against the **live** registry (at run start, and by `bmad-loop validate`'s `adapter.kind`), never at parse time — so an unknown kind is a clear config error rather than a schema change. Independent of `hooks.dialect = "none"`: hooklessness is about the transport, this is about the driving class — with one back-compat carve-out for files written before this field existed: when the key is **absent** and the dialect is `none`, the kind is `opencode-http` (what hooklessness used to select), not `generic`. | +| `adapter` | | `generic` | Which adapter **class** drives this CLI — a key resolved against the [adapter registry](#shipping-a-new-adapter-class-out-of-tree), not a fixed enum. `generic` = the bundled tmux + hook-signal adapter; `opencode-http` = the bundled HTTP/SSE adapter; an out-of-tree package registers its own. Membership is checked against the **live** registry (at run start, and by `bmad-loop validate`'s `adapter.kind`), never at parse time — so an unknown kind is a clear config error rather than a schema change. Orthogonal to `hooks.dialect = "none"` — hooklessness is about the transport, this is about the driving class — with two qualifications. A back-compat carve-out for files written before this field existed: when the key is **absent** and the dialect is `none`, the kind is `opencode-http` (what hooklessness used to select), not `generic`. And one refused pairing: an explicit `generic` beside `dialect = "none"` is rejected at load — that adapter completes on a `Stop` hook a hookless profile never registers, so the session could only wait out `session_timeout_min`. Hookless on any **other** kind stays legal, which is the decoupling this field exists for; a provider shipping a hookless profile must therefore set `adapter` rather than leave it at the default. | | `skill_tree` | | `.claude/skills` | Project-relative tree this CLI reads skills from (`.agents/skills` for codex/gemini); `bmad-loop init` installs the `bmad-loop-*` skills here. Must be relative. | | `prompt_template` | | `{prompt}` | How the canonical `/skill args` prompt is rendered. Placeholders: `{prompt}` (whole string), `{skill}` (leading slash-command name, no `/`), `{args}` (the remainder). | | `launch_args` | | `()` | Extra argv passed at launch, e.g. `["-i"]` to stay interactive (gemini/copilot). | From 3b0dded5bc0dbfda478dc844704f2ee5d042b1c9 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 12 Aug 2026 14:53:19 -0700 Subject: [PATCH 6/6] fix(adapters): require canonical name/binary/adapter from a profile provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 4 of the review gate (codex P2). The round-1 `.strip()` closed half of a divergence and left the other half open: `_validate_profile` tests a STRIPPED copy, but the frozen original is what gets installed. The TOML route canonicalizes exactly three fields before construction, so `" acme "` is content `_parse_profile` cannot produce — and every consumer keys on the exact string. The profile lands under a map key `--cli acme` never finds, a `binary` `shutil.which` never resolves, and an `adapter` `get_adapter_kind` reports as an unknown kind, while the provider that shipped it is recorded as perfectly fine. Codex flagged `adapter`; `name` and `binary` are the same miss and are closed with it, since `_parse_profile` strips all three. `name` is the worse of the two it did not name: the profile is filed under a key nothing resolves, so the package looks installed and absent at once. Refused rather than normalized. This function validates and does not rewrite — rebuilding a frozen dataclass here would leave the caller holding the original anyway — and refusing is the louder half: the provider is dropped WITH a reason naming the field, which is what the recorded-degrade contract owes an operator. Ordered after the emptiness tests so `" "` still reads as empty rather than as non-canonical, which keeps the existing whitespace-only row's message intact. --- CHANGELOG.md | 5 ++++- src/bmad_loop/adapters/profile.py | 29 +++++++++++++++++++++++++++++ tests/test_profile.py | 9 +++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 91d08ff7..f49cad39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -165,7 +165,10 @@ whose seams had diverged enough that several ports needed a different fix, and t the profile map now share one invariant set (hook dialect, path containment, `env_fault_patterns` compilation, …), so a package can no longer install a profile state the parser would refuse — an invalid env-fault regex used to trade a load-time error for a silent never-match at classification - time. A malformed `adapter` value funnels into `ProfileError` rather than being `str()`-coerced. + time. A malformed `adapter` value funnels into `ProfileError` rather than being `str()`-coerced, + and `name`/`binary`/`adapter` must arrive already canonical — the TOML route strips them, so a + provider handing over `" acme "` would otherwise install a profile filed under a key no `--cli` + resolves, with the provider itself recorded as fine. - **Lint the workflows, and smoke-test the built package.** `trunk check` now runs `actionlint` and `zizmor` over `.github/workflows/`, and a `build` CI job builds the sdist + wheel, runs diff --git a/src/bmad_loop/adapters/profile.py b/src/bmad_loop/adapters/profile.py index efc3a304..1ca786c0 100644 --- a/src/bmad_loop/adapters/profile.py +++ b/src/bmad_loop/adapters/profile.py @@ -238,6 +238,35 @@ def fail(msg: str) -> ProfileError: if not profile.adapter.strip(): raise fail("adapter must be a non-empty string naming an adapter kind") + # The emptiness tests above use `.strip()` because the TOML route CANONICALIZES + # before it gets here — `_parse_profile` strips exactly these three fields — so + # testing the raw value would refuse `adapter = " "` from a file while + # admitting it from a provider. But validating a stripped COPY while the frozen + # original is what gets installed leaves the other half of that same divergence + # open: `" acme "` is content `_parse_profile` cannot produce, and every + # consumer keys on the exact string. The profile lands under a map key `--cli + # acme` never finds, a `binary` `shutil.which` never resolves, and an `adapter` + # `get_adapter_kind` reports as an unknown kind — while the provider that + # shipped it is recorded as perfectly fine. + # + # Refused, not normalized: this function validates and does not rewrite, and a + # frozen dataclass rebuilt here would leave the caller holding the original + # anyway. Refusing is also the louder half — the provider is dropped WITH a + # reason naming the field, which is what the recorded-degrade contract owes an + # operator. Ordered after the emptiness tests so `" "` still reads as empty + # rather than as non-canonical. + for label, value in ( + ("name", profile.name), + ("binary", profile.binary), + ("adapter", profile.adapter), + ): + if value != value.strip(): + raise fail( + f"{label} must not carry leading/trailing whitespace: {value!r} " + "(the TOML route strips it; a provider must hand over the " + "canonical value so both routes install the same profile)" + ) + # The one coherence rule between the two axes, and the only place both routes # pass through. `generic` is the bundled tmux adapter: it injects into a window # and completes on a Stop hook (or the window dying), so pairing it with diff --git a/tests/test_profile.py b/tests/test_profile.py index 8a26ddbf..c0e1ae09 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -741,6 +741,15 @@ def test_profile_scan_failure_degrades(profile_scan): # admitting it from a provider — the divergence this whole test denies ({"adapter": " "}, "adapter"), ({"binary": " "}, "required"), + # ...and the other half of that same divergence: NON-canonical, not empty. + # `_parse_profile` strips exactly these three, so an unstripped value is + # content the TOML route cannot produce. Validating a stripped copy while + # installing the frozen original would file the profile under a key no + # `--cli` finds / a binary no `which` resolves / a kind no registry has, + # with the provider recorded as fine. + ({"adapter": " acme "}, "whitespace"), + ({"name": " acme "}, "whitespace"), + ({"binary": " acme "}, "whitespace"), ], ) def test_entry_point_profile_must_pass_the_parser_invariants(profile_scan, over, match):