feat(adapters): coding-CLI adapter registry (Seam A) — out-of-tree adapters with zero core edits - #239
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe change adds registry-based coding-CLI adapters, external profile loading, adapter-aware launch setup, validation, and the ChangesCoding-CLI adapter extension
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: ⚪ Minimal · up to The PR adds out-of-tree adapter selection and profile registration without any supplied actionable correctness or merge-blocking risk; it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant ProfileLoader
participant AdapterRegistry
participant RunSetup
participant CLI
ProfileLoader->>AdapterRegistry: discover external adapter registrations
ProfileLoader-->>RunSetup: provide profile.adapter
RunSetup->>AdapterRegistry: resolve adapter kind and load builder
AdapterRegistry-->>RunSetup: return adapter classes and mux requirement
RunSetup->>CLI: construct selected adapter
CLI-->>RunSetup: report validation or construction error
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Triage 2026-08-08 (self-note): still needed — profile.py:254-266 landed the project overlay but no entry-point scan ( |
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.
eccdf09 to
a5a0fcd
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5a0fcd7a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return _EXTERNAL_PROFILES | ||
| for ep in eps: | ||
| try: | ||
| provider = ep.load() |
There was a problem hiding this comment.
When a package follows the documented pattern of defining both entry points in one module, loading its profile provider here also executes its register_adapter calls before _load_builtin_adapters() has run. If it accidentally registers a bundled name such as generic, the registry's first-registration-wins setdefault preserves the external implementation and all default profiles are redirected to it, defeating the stated guarantee that bundled kinds cannot be shadowed. Ensure built-ins are registered before importing any external profile provider.
Useful? React with 👍 / 👎.
| 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() |
There was a problem hiding this comment.
When a selected profile names an unknown adapter kind, a real run reaches this lookup and aborts, but cmd_run --dry-run returns earlier through _dry_run and never resolves the registry. It therefore exits successfully and prints a plausible invocation for a configuration that cannot run; for a registered external family, that invocation is also still synthesized using the generic-versus-hookless assumptions rather than the selected builder. The dry-run path should resolve the kind and use an adapter-specific rendering contract before reporting success.
Useful? React with 👍 / 👎.
| kind = get_adapter_kind(profile.adapter) | ||
| except AdapterError as e: | ||
| raise SystemExit(f"error: profile {profile.name!r}: {e}") from e | ||
| builder = kind.load() |
There was a problem hiding this comment.
When an external adapter's documented lazy _load imports a missing optional dependency or otherwise fails, the exception is raised at this line before the construct_error handling below, producing a raw traceback after compose_run has already persisted the run state and PID. Neither validate nor bmad-loop adapters detects this because both intentionally avoid invoking the thunk, so run construction needs to convert loader failures into a typed, clean startup error rather than leaving a partially initialized run.
AGENTS.md reference: AGENTS.md:L78-L78
Useful? React with 👍 / 👎.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/test_profile.py (1)
462-508: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClear the scan state in the fixture setup, not only inside
arm.
armresets_EXTERNALS_LOADED,_EXTERNAL_PROFILESand_PROFILE_LOAD_ERRORS. Every current test callsarm, so isolation holds today. A future test that requestsprofile_scanand assertsexternal_profile_errors() == {}without callingarmwould read whatever a previous test left in the module globals, and would pass for the wrong reason.Moving the three resets to fixture setup keeps
armresponsible only for installing the fake entry points.♻️ Proposed refactor
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 = False + profile_mod._EXTERNAL_PROFILES.clear() + profile_mod._PROFILE_LOAD_ERRORS.clear() def arm(*eps, scan_error=None): @@ 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() + profile_mod._EXTERNALS_LOADED = False # re-arm after an earlier load in the same test🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_profile.py` around lines 462 - 508, Move the initial resets of profile_mod._EXTERNALS_LOADED, _EXTERNAL_PROFILES, and _PROFILE_LOAD_ERRORS from arm into the profile_scan fixture setup before yielding. Keep the corresponding saved-state restoration in teardown, and leave arm responsible only for installing fake entry points and any per-call setup required by its arguments.src/bmad_loop/adapters/profile.py (1)
407-412: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider sorting the entry points so first-registration-wins is reproducible.
setdefaultat Line 412 makes the first provider win a name collision.importlib.metadata.entry_pointsyields providers in metadata discovery order, which depends on the installed distributions andsys.pathorder. Two hosts with the same packages installed can therefore resolve a colliding profile name differently, and the recorded reason names no conflict.Sorting by entry-point name makes the winner a property of the names rather than of the install order.
♻️ Proposed refactor
- for ep in eps: + for ep in sorted(eps, key=lambda ep: ep.name):The bundled adapter registry has the same shape (
register_adapterusessetdefault), but there builtins register before the scan, so a bundled name is never shadowed. Profiles have no such ordering guarantee between two external providers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bmad_loop/adapters/profile.py` around lines 407 - 412, Sort the entry points deterministically by their names before iterating in the external profile registration flow, so the existing setdefault call in the loop around _coerce_profiles and _EXTERNAL_PROFILES consistently gives the same provider precedence for name collisions. Preserve the current provider loading and profile coercion behavior.src/bmad_loop/adapters/registry.py (1)
181-205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm that a failed external adapter import cannot leave a partially registered kind.
ep.load()imports a third-party module. That module can callregister_adapterfor kind A, then raise while registering kind B. The loader records the failure for the entry point, but kind A stays registered and selectable. Selection then succeeds for a package the operator was told failed to load.This matches the documented degrade-not-crash contract, so it may be intended. Confirm the intent, and if partial registration is acceptable, state it in the docstring so a reader does not assume a failed entry point registered nothing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bmad_loop/adapters/registry.py` around lines 181 - 205, Clarify the _load_external_adapters docstring that if ep.load() fails after registering some adapter kinds, those partial registrations remain available and selectable while the entry point failure is recorded. Preserve the existing degrade-without-raising behavior and do not add rollback unless the intended contract is to reject partial registration.src/bmad_loop/runsetup.py (1)
455-459: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConvert adapter-thunk
ImportErrorinto a startup error.
AdapterKind.load()imports adapter classes and optional dependencies. CatchImportErrorand raiseSystemExitwith profile and adapter context. Keep unrelated exceptions uncaught so adapter bugs still produce tracebacks. The bundledopencode_httploader remains safe because it importshttpxlazily.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bmad_loop/runsetup.py` around lines 455 - 459, Update the adapter loading flow around kind.load() to catch ImportError and raise SystemExit with both profile and adapter context, preserving the existing startup-error format. Keep AdapterError handling unchanged and allow unrelated exceptions from AdapterKind.load() to propagate normally.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/adapters/profile.py`:
- Around line 222-226: Update the shared adapter validation in the profile
validator to strip whitespace before checking whether adapter is non-empty,
matching the existing name and binary validation behavior. Ensure
whitespace-only adapter values are rejected for entry-point profiles while
preserving non-empty adapter values after normalization.
In `@src/bmad_loop/cli.py`:
- Around line 567-572: Import load_profiles alongside the existing profile
helpers, then invoke it unconditionally before iterating
external_profile_errors() so the bmad_loop.profiles entry-point scan runs even
when policy parsing prevents get_profile from executing. Preserve the existing
warning report and error details.
In `@tests/test_adapter_registry.py`:
- Line 603: Update the unpacking assignment in the test using
scan_adapter_registry to rename the unused registry variable to _registry,
matching the existing convention while preserving arm usage.
---
Nitpick comments:
In `@src/bmad_loop/adapters/profile.py`:
- Around line 407-412: Sort the entry points deterministically by their names
before iterating in the external profile registration flow, so the existing
setdefault call in the loop around _coerce_profiles and _EXTERNAL_PROFILES
consistently gives the same provider precedence for name collisions. Preserve
the current provider loading and profile coercion behavior.
In `@src/bmad_loop/adapters/registry.py`:
- Around line 181-205: Clarify the _load_external_adapters docstring that if
ep.load() fails after registering some adapter kinds, those partial
registrations remain available and selectable while the entry point failure is
recorded. Preserve the existing degrade-without-raising behavior and do not add
rollback unless the intended contract is to reject partial registration.
In `@src/bmad_loop/runsetup.py`:
- Around line 455-459: Update the adapter loading flow around kind.load() to
catch ImportError and raise SystemExit with both profile and adapter context,
preserving the existing startup-error format. Keep AdapterError handling
unchanged and allow unrelated exceptions from AdapterKind.load() to propagate
normally.
In `@tests/test_profile.py`:
- Around line 462-508: Move the initial resets of profile_mod._EXTERNALS_LOADED,
_EXTERNAL_PROFILES, and _PROFILE_LOAD_ERRORS from arm into the profile_scan
fixture setup before yielding. Keep the corresponding saved-state restoration in
teardown, and leave arm responsible only for installing fake entry points and
any per-call setup required by its arguments.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 008d673f-f959-4678-9035-5adc7ff0bf18
📒 Files selected for processing (15)
AGENTS.mdCHANGELOG.mddocs/FEATURES.mddocs/README.mddocs/adapter-authoring-guide.mdsrc/bmad_loop/adapters/profile.pysrc/bmad_loop/adapters/registry.pysrc/bmad_loop/checks.pysrc/bmad_loop/cli.pysrc/bmad_loop/data/profiles/opencode.tomlsrc/bmad_loop/runsetup.pytests/test_adapter_registry.pytests/test_cli.pytests/test_profile.pytests/test_runsetup.py
| 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}, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
external_profile_errors() can report nothing when policy.toml fails to load.
The bmad_loop.profiles entry-point scan runs inside load_profiles, which validate reaches only through get_profile at line 316. That call sits inside the try block whose except policy_mod.PolicyError fires at line 321. If the policy fails to parse, role_names is never computed, get_profile never runs, the profile scan never happens, and this loop iterates an empty mapping. A broken installed profile package is then absent from the report for a reason unrelated to that package.
The adapter half does not have this gap, because known_adapter_kinds() at line 545 runs the adapter scan unconditionally. Trigger the profile scan the same way.
🛡️ Proposed fix
+ # Force the `bmad_loop.profiles` entry-point scan even when policy failed to
+ # load: otherwise a broken profile package goes unreported for a reason that
+ # has nothing to do with that package. `load_profiles` is idempotent.
+ try:
+ load_profiles(project)
+ except ProfileError:
+ pass # a malformed overlay is already reported as `adapter.profile` above
for ep_name, reason in sorted(external_profile_errors().items()):This needs load_profiles added to the import at line 299.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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}, | |
| ) | |
| # Force the `bmad_loop.profiles` entry-point scan even when policy failed to | |
| # load: otherwise a broken profile package goes unreported for a reason that | |
| # has nothing to do with that package. `load_profiles` is idempotent. | |
| try: | |
| load_profiles(project) | |
| except ProfileError: | |
| pass # a malformed overlay is already reported as `adapter.profile` above | |
| 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}, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/bmad_loop/cli.py` around lines 567 - 572, Import load_profiles alongside
the existing profile helpers, then invoke it unconditionally before iterating
external_profile_errors() so the bmad_loop.profiles entry-point scan runs even
when policy parsing prevents get_profile from executing. Preserve the existing
warning report and error details.
| ): | ||
| """`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 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Prefix the unused unpacked variable with an underscore.
Ruff reports RUF059 here: registry is never used in this test. Line 689 already uses _registry for the same unpacking shape, so the convention exists in this file.
🧹 Proposed fix
- registry, arm = scan_adapter_registry
+ _registry, arm = scan_adapter_registry📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| registry, arm = scan_adapter_registry | |
| _registry, arm = scan_adapter_registry |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 603-603: Unpacked variable registry is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_adapter_registry.py` at line 603, Update the unpacking assignment
in the test using scan_adapter_registry to rename the unused registry variable
to _registry, matching the existing convention while preserving arm usage.
Source: Linters/SAST tools
…back-compat, loader escalation 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.
|
@codex review |
|
@coderabbitai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17012b8399
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # `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" |
There was a problem hiding this comment.
Now that adapter and hooks.dialect are independent, cmd_validate still applies OpenCode's provider/model rule whenever prof.hookless is true. Consequently, an out-of-tree hookless adapter using ordinary bare model names receives a misleading OpenCode warning, while an opencode-http profile with a non-none hook dialect misses the warning and can silently fall back to its default model. Gate this check on prof.adapter == adapter_registry.OPENCODE_HTTP, as the adjacent httpx check already does.
AGENTS.md reference: AGENTS.md:L38-L38
Useful? React with 👍 / 👎.
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.
|
@codex review |
|
@coderabbitai review |
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98f0bb7fef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not profile.adapter.strip(): | ||
| raise fail("adapter must be a non-empty string naming an adapter kind") |
There was a problem hiding this comment.
When an entry-point provider leaves the new adapter field at its generic default while using HookSpec("none", ...), or TOML explicitly combines those values, this validation accepts the profile and validate reports both the kind and hooklessness as OK. The generic adapter relies on a Stop hook, while initialization skips hook registration for this profile, so an interactive CLI remains open until the session timeout rather than completing; reject the generic/none pairing while continuing to allow hookless profiles for custom kinds.
AGENTS.md reference: AGENTS.md:L25-L25
Useful? React with 👍 / 👎.
…bution 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.
|
@codex review |
|
@coderabbitai review |
|
…ld 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90a7ca930d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not profile.adapter.strip(): | ||
| raise fail("adapter must be a non-empty string naming an adapter kind") |
There was a problem hiding this comment.
When a bmad_loop.profiles provider returns an otherwise valid profile with surrounding whitespace in adapter (for example, " acme " while acme is registered), this check validates the stripped copy but installs the original frozen value. Unlike the TOML path, which stores raw_adapter.strip(), the entry-point profile is then reported as an unknown kind and every real run refuses it, without recording the provider as malformed. Normalize the value before installation or reject noncanonical whitespace at this boundary.
AGENTS.md reference: AGENTS.md:L78-L78
Useful? React with 👍 / 👎.
…rovider 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.
|
@codex review |
|
@coderabbitai review |
|
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What & why
The transport axis has long been extensible out-of-tree (
register_multiplexer+ thebmad_loop.mux_backendsentry-point group). The coding-CLI adapter axis had no equivalent: a CLI needing a new adapter class forced a name-branch in the run bootstrap (if profile.hookless: …) and a hardcoded valid-kinds set. This PR lands the missing registry so a new adapter family — and its selecting profile — plugs in with zero core.pyedits, exactly like a transport backend.This is Session 1 (Seam A) of the PR #226 review plan. No Hermes-specific code, no hook-scope changes, no relay changes.
Design
Mirrors
adapters/multiplexer.py, with two deliberate asymmetries (documented in the module):lru_cache/cache_clear— adapters are built per run inmake_adapters(its ownby_cfgcache), so there is nothing to cache.configure_*/matches(platform)/platform defaults — an adapter kind is selected by theprofile.adapterfield alone, not a policy knob orsys.platformpredicate.Two dataclasses:
AdapterKind(name, needs_mux, load)—loadis a lazy thunk, so registration/validation/listing never import a heavy adapter module (nor an optional dep likehttpx).AdapterBuilder(plain, dev, construct_error)— the plain class, the_DevSynthesisMixin-composed dev class, and the family's construction-failure type(s) (()= none;(OpencodeServerError,)for HTTP).Changes
adapters/registry.py:register_adapter(first-wins so builtins can't be shadowed), builtin loader (genericneeds_mux=True,opencode-httpneeds_mux=False), thebmad_loop.adaptersentry-point scan degrading into_EXTERNAL_ERRORS(never raises),get_adapter_kind(fail loud, naming known kinds),known_adapter_kinds,external_adapter_errors,detect_adapters.runsetup.make_adaptersrewritten to a registry lookup keyed onprofile.adapter; the shared mux is resolved + usability-checked only whenkind.needs_mux; the solehooklessselection branch is gone; thesynthesizes/pathsaxis stays a documented cross-family pipeline contract;construct_error→SystemExit.cli.pykeeps only its re-export seam.config_digestpinsadapter— it supersedeshooklessas the field selecting the argv builder, so a session rewriting it mid-run moves the pin the auto-sweep gates on instead of swapping the launch shape underneath it.adapters/profile.py:CLIProfile.adapter(default"generic"), parsed but not membership-validated (validity is enforced against the live registry).opencode.tomlmigrated toadapter = "opencode-http".load_profilesgains abmad_loop.profilesentry-point scan (precedence packaged < entry-point < project), degrade-not-crash intoexternal_profile_errors(). Value-level invariants extracted into_validate_profile, run by both routes into the profile map.bmad-loop adapterscommand +cmd_validateadapter.kindfinding (viaknown_adapter_kinds(), never a hardcoded set) + external-error surfaces; three new check ids inchecks.VALIDATE_CHECKS.adapter.httpxre-keyed on the adapter kind.Rebase deltas (versus the original cut)
runsetup.make_adapters(runsetup.py), preserving theinitvendors the hook relay into the agent-writable workspace, turning a file-write primitive into unattended persistent code execution #461profiles is not Nonepath so a digest-gated caller still launches the bytes it validated — the kind is read off the pinned profile, not a second read.cli.py:86's re-import seam is kept (existingmonkeypatch.setattr(cli, "_make_adapters", …)tests still bite; a test pins the identity so a future merge can't resurrect a dead duplicate).adapterjoins theconfig_digestlaunch payload, and the docstring's union-completeness rule is re-derived for open-ended external builders: the reads stay a closed set (the adapter's kwargs) butPolicyis wider than the hashed launch surface, and the hashedadapterbounds the gap to fields of the kind already launched.cmd_validateblock re-anchored after main'sinitvendors the hook relay into the agent-writable workspace, turning a file-write primitive into unattended persistent code execution #461 relay-stat block;adapter.httpxre-keyed on the adapter kind rather thanhookless— 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._parse_profileenforces — dialect set,env_fault_patternsregex compilation, path containment. Value-level checks were extracted into_validate_profile, called by both routes, so a Python package cannot install a state the TOML parser refuses. The sharp case: an invalid env-fault regex otherwise trades a load-time error for a silent never-match inside a session's fault classification. Scope is semantic, not type-level, and the boundary is stated in the docstring.adapterTOML value funnels intoProfileErrorper provision_worktree's exclude patterns are repo-wide and permanent: new files under .claude/skills silently stop being staged in the main checkout #384 instead ofstr()coercion (str(["x"])would carry"['x']"toget_adapter_kindand name that as the unknown kind).[adapter] name(picks the profile) is disambiguated from the profile'sadapter(picks the kind).Guardrails honored
profile.adapterenforced via the registry (known_adapter_kinds), never a hardcoded set. (Theadapter.httpxcheck names one bundled kind — a fact about one family's optional dep, not a valid-kinds set — via a registry constant, pinned by a test.)registry.pyimports adapter classes only inside lazyloadthunks.profile.pydoes not checkadaptermembership at parse time (shape only).opencode-httpmigrated to a registered builtin, with a regression pin that its dispatch is unchanged.Tests
tests/test_adapter_registry.py(30 tests): registration, builtins-first-wins, unknown-kind fail-loud, broken-external degrades/recorded, one-bad-package-doesn't-hide-the-rest, real*.dist-infodiscovery,detect_adapters, theby_cfg (cfg, synthesizes)cache, both directions of theneeds_muxgate,construct_error→SystemExitand that an undeclared failure is not swallowed, thecli._make_adaptersalias identity, the digest pin, the httpx re-key, theprofiles=path deciding the kind, and opencode-http dispatching unchanged.tests/test_profile.py: entry-point discovery, plus a 15-row parity table proving an entry-point profile is held to every invariant a TOML profile is, a whole-batch rejection test, and a valid-profile control.adapterpin, and the httpx re-key. The stale "hookless selects the adapter" docstrings intest_cli.py/test_runsetup.pyare retexted.Verification
uv run pytest -q -n auto: 5197 passed, 49 skipped, 5 xfailed, 0 failed.uv run pyright: 0 errors.trunk check --all: no issues.*.dist-info, both entry-point groups) onPYTHONPATH—bmad-loop adapterslists it (scratch external no scratch), and bothrunsetup.make_adaptersandcli._make_adaptersselect it for all three roles with no multiplexer resolved, with no edits to any core.py.Summary by CodeRabbit
New Features
bmad-loop adaptersto list available adapters, requirements, associated profiles, and loading issues.Bug Fixes