Release 0.7.6#23
Conversation
Phase A of the adapter-seam close-out: funnel every tmux invocation through
one overridable spawn primitive so a tmux-family backend (a future native-
Windows psmux) can subclass it without editing tmux_backend.py.
- New adapters/tmux_base.py :: BaseTmuxBackend holds all argv construction and
every contract method, routed through a single _run() primitive (_tmux uses
check=True; the ~14 tolerant sites use check=False, keeping their existing
which-guards and exception handling). TmuxError/TMUX_TIMEOUT_S/
PARKED_RETURN_DETACH move here.
- tmux_backend.py slims to TmuxMultiplexer(BaseTmuxBackend) plus back-compat
re-exports (subprocess/shutil/TmuxError/TMUX_TIMEOUT_S/PARKED_RETURN_DETACH).
- Portability guard: TMUX_BACKEND string -> TMUX_BACKENDS set covering both
files; the sole ["tmux", ...] literal now lives in tmux_base._run.
- Retarget test monkeypatches tmux_backend.{subprocess,shutil} -> tmux_base
(the spawn seam moved modules).
Behavior-preserving on POSIX. Full suite green (1119 passed); trunk clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the hardcoded TmuxMultiplexer construction in get_multiplexer() with a tiny registry: backends self-register via register_multiplexer(name, matches, factory) and are selected by platform match or forced by name through the BMAD_AUTO_MUX_BACKEND env var. The tmux backend registers itself; tmux stays the default everywhere except win32, with a safe tmux fallback, so POSIX behavior is unchanged. Phase B of the adapter-seam close-out: an out-of-tree backend now registers at import with zero core edits, and a bundled one (the in-flight psmux) costs only its own registration line plus one import — no edit to the selection policy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The kill/liveness primitives branched on sys.platform inline in platform_util, and stop_run had no way to force-kill a wedged engine — so a native-Windows backend (PR #19) would have to edit core. Introduce a ProcessHost seam (terminate / force_kill / is_alive / identity) with per-OS impls and a name-keyed registry mirroring the multiplexer backend selection, so adding an OS is a new subclass + one registration line. stop_run now escalates: an engine that ignores SIGTERM past the grace window is SIGKILL'd — but only while host.identity(pid) still matches the value recorded at stop-time (a pid-reuse guard); otherwise it raises the new StopRunError rather than risk an unrelated process. This is a deliberate POSIX behavior change, pinned by new tests. The graceful-first / single-writer-of- stopped invariant is unchanged. platform_util keeps terminate_pid/pid_alive as thin back-compat shims over the host; detach_kwargs stays put. The TUI worker widens its except to surface StopRunError/ProcessHostError. The portability guard swaps platform_util for process_host in KILL_PROBE_ALLOW and allows its Linux /proc identity read. Unity teardown de-dup is deferred: unity_teardown.py runs under a bare python3 and can't reliably import the package. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The hook-registration sites in install/probe hardcoded `python3`, and `cmd_validate` hardcoded a `tmux` PATH check — two more axes a native backend (psmux PR #19) would have to branch on `sys.platform` to cross. Route both through the platform-selected seams so a new OS registers rather than edits core: - Add `ProcessHost.hook_interpreter()` (POSIX `python3`; Windows `uv run --no-project python`); install/probe interpolate it instead of a `python3` literal. POSIX output is byte-for-byte unchanged. - Extract `_platform_preflight()` in cli.py: ask the multiplexer backend (`available()`/`version()`) and name the process host, replacing the hardcoded `tmux` probe in `cmd_validate`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Unity teardown re-implemented `_terminate_pid`/`_hard_kill_pid`/ `_alive`/`_taskkill` — the duplication the ProcessHost seam was meant to retire. Delegate to `get_process_host()` and keep only the Unity-specific `_lingering_pids*` worktree-discovery in the file. To make the import safe from the spawned helper, run plugin scripts under the orchestrator's own interpreter (`sys.executable`) instead of a PATH-resolved `python3` — otherwise a pipx-style install (PATH `python3` lacks the package) would crash the best-effort teardown. Deltas: Windows force-kill now adds `/T` (reaps the leaked MCP server's child tree); `pid <= 0` is guarded. The sweep keeps its exact terminate -> poll -> force-kill-survivors shape. Drops the now-unused unity entry from the portability guard's KILL_PROBE_ALLOW. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…cessHost) Add docs/porting-to-a-new-os.md mapping the four OS seams (terminal multiplexer, ProcessHost lifecycle, hook interpreter, validate preflight), their registries and override env vars, and the end-to-end native-Windows port cost. Refresh the adapter- and plugin-authoring guides, ROADMAP, setup-guide, README, docs index, and FEATURES to the post-registry world: register_multiplexer/register_process_host instead of "return from get_multiplexer()", helper scripts running under sys.executable and using get_process_host() for pid lifecycle, and the BMAD_AUTO_MUX_BACKEND / BMAD_AUTO_PROCESS_HOST overrides. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 30 minutes and 31 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds ProcessHost and multiplexer registries, refactors tmux into a shared base, routes hook and lifecycle operations through the new seams, adds CLI platform preflight, updates tests and docs, and bumps versions to 0.7.6. ChangesOS Seam Registry
Sequence Diagram(s)sequenceDiagram
participant cli.py
participant multiplexer.py
participant process_host.py
participant validate
cli.py->>multiplexer.py: get_multiplexer()
cli.py->>process_host.py: get_process_host()
multiplexer.py-->>validate: available()/version()
process_host.py-->>validate: selected host
validate-->>cli.py: notes/problems
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
🤖 Augment PR SummarySummary: This PR publishes the 0.7.6 release, completing the “platform seams” work to make OS-specific behavior pluggable without editing core engine code. Changes:
Technical Notes: Both seam selectors are 🤖 Was this summary useful? React with 👍 or 👎 |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/automator/adapters/multiplexer.py`:
- Around line 185-194: `register_multiplexer()` currently only appends to
`_BACKENDS`, so a previously cached result from `get_multiplexer()` can block
later-registered backends from being selected. Update `register_multiplexer()`
in `multiplexer.py` to invalidate the cached singleton when a new backend is
added, so the registry API works regardless of call order. Use the existing
`_BACKENDS` and `get_multiplexer()`/cache mechanism as the integration points.
In `@src/automator/adapters/tmux_base.py`:
- Around line 168-174: The shell trailer built in the tmux base adapter
interpolates return_opt directly into sh -c text, which allows the option key to
affect the command string. Update the return_trailer construction in the
tmux_base adapter to safely quote or shell-escape return_opt before embedding
it, keeping the rest of the logic in the trailer unchanged. Use the existing
return_trailer assembly near the tmux show-options / detach-client /
switch-client flow as the place to apply the fix.
In `@src/automator/cli.py`:
- Around line 111-140: The _platform_preflight() probe should not let
multiplexer readiness checks abort validation if the selected backend
misbehaves. In the backend block that calls get_multiplexer(), available(), and
version(), wrap the probe in a try/except Exception and append the exception
text to problems instead of letting it propagate. Keep the existing
notes/problems behavior intact, and reference _platform_preflight(),
get_multiplexer(), available(), and version() when updating the flow.
In `@src/automator/data/plugins/unity/unity_teardown.py`:
- Around line 162-176: The teardown logic in the process-killing loop can
accidentally act on a reused PID because `host.is_alive(pid)` only checks
existence. In the `unity_teardown.py` flow around `get_process_host()`,
`terminate()`, and `force_kill()`, snapshot `host.identity(pid)` before sending
terminate, then during the grace-period polling and final escalation only
proceed if the current identity still matches the saved one. This keeps the
`pids` cleanup logic from killing an unrelated process that reused the same PID.
In `@src/automator/install.py`:
- Around line 96-101: The hook command formatting in install/probe is using
POSIX quoting directly, which can break on Windows paths with spaces. Move the
hook-command string निर्माण behind ProcessHost so each host can apply its own
quoting rules, and update the call sites in install.py’s hook setup and
probe.py’s hook command generation to use that shared helper instead of
shlex.quote. Use the existing hook-related symbols like get_process_host(),
hook_interpreter(), and the hook command construction around HOOK_SCRIPT_REL to
locate and refactor the logic.
In `@src/automator/process_host.py`:
- Around line 235-240: The host चयन logic that reads BMAD_AUTO_PROCESS_HOST
should fail fast when the override is set but does not match any registered
host. In the host-selection helper that iterates over _HOSTS and currently falls
back to PosixProcessHost(), first check whether forced is set and, if so, raise
an explicit error when no factory name matches instead of silently returning the
POSIX default. Keep the normal platform-based matching path unchanged when
BMAD_AUTO_PROCESS_HOST is unset.
In `@tests/test_engine_plugin.py`:
- Around line 386-387: The lingering-PID test setup in the _force_kill_lingering
path uses a hardcoded PID that can match the test runner and get filtered out by
os.getpid(), causing flaky failures. Update the affected tests around
_lingering_pids to generate a PID dynamically from the current process id (for
example, a value guaranteed to differ from os.getpid()) instead of using 4242.
Keep the monkeypatching of mod.time.sleep and the same test flow, but ensure the
fake lingering PID cannot collide with the active process.
In `@tests/test_process_host.py`:
- Around line 73-76: The default-host test is hardcoded to PosixProcessHost, but
get_process_host() picks the first matching host based on sys.platform, so the
assertion must be platform-aware. Update test_default_host_is_posix_on_posix to
branch on sys.platform and assert PosixProcessHost on POSIX platforms and
WindowsProcessHost on Windows, while keeping the monkeypatch and
get_process_host.cache_clear() setup intact.
🪄 Autofix (Beta)
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
Run ID: f8b3f588-2a01-4f16-9813-3f1b400717ba
⛔ Files ignored due to path filters (9)
docs/images/dashboard.pngis excluded by!**/*.pngdocs/images/dashboard.svgis excluded by!**/*.svgdocs/images/demo.gifis excluded by!**/*.gifdocs/images/settings.svgis excluded by!**/*.svgdocs/images/start-run-modal.pngis excluded by!**/*.pngdocs/images/start-run-modal.svgis excluded by!**/*.svgdocs/images/sweep-decision.pngis excluded by!**/*.pngdocs/images/sweep-decision.svgis excluded by!**/*.svguv.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
.claude-plugin/marketplace.jsonCHANGELOG.mdREADME.mddocs/FEATURES.mddocs/README.mddocs/ROADMAP.mddocs/adapter-authoring-guide.mddocs/plugin-authoring-guide.mddocs/porting-to-a-new-os.mddocs/setup-guide.mdmodule.yamlpyproject.tomlsrc/automator/__init__.pysrc/automator/adapters/multiplexer.pysrc/automator/adapters/tmux_backend.pysrc/automator/adapters/tmux_base.pysrc/automator/cli.pysrc/automator/data/plugins/unity/unity_plugin.pysrc/automator/data/plugins/unity/unity_teardown.pysrc/automator/data/skills/bmad-auto-setup/assets/module.yamlsrc/automator/install.pysrc/automator/platform_util.pysrc/automator/probe.pysrc/automator/process_host.pysrc/automator/runs.pysrc/automator/tui/app.pysrc/automator/tui/data.pytests/test_backend_registry.pytests/test_cli.pytests/test_engine_plugin.pytests/test_generic_tmux.pytests/test_install.pytests/test_multiplexer.pytests/test_platform_util.pytests/test_portability_guard.pytests/test_process_host.pytests/test_runs.pytests/test_tui_data.pytests/test_tui_launch.py
… seams Validated the Augment/CodeRabbit review on PR #23 and fixed the 10 valid findings (1 was a false positive — the balanced parens in setup-guide.md). - cli: catch StopRunError/ProcessHostError in stop/delete/archive so a pid-reuse refusal returns a clean error instead of a traceback (A) - cli: guard _platform_preflight's backend available()/version() probe so a misbehaving backend surfaces as a problem, not a crashed `validate` (C) - multiplexer: set _BUILTINS_LOADED only after a successful load so a transient failure retries instead of latching an empty registry (D) - process_host/multiplexer: fail fast when BMAD_AUTO_PROCESS_HOST / BMAD_AUTO_MUX_BACKEND names no registered backend, instead of a silent (and on Windows unsafe) POSIX/tmux fallback (B) - process_host/multiplexer: register_* now invalidate the get_* lru_cache so a later registration is honored regardless of call order (F) - tmux_base: shlex.quote return_opt before embedding it in the sh -c trailer (G) - unity teardown: snapshot identity() before terminate and only force-kill on a still-matching identity, mirroring runs.stop_run's pid-reuse guard (H) - process_host: add a shell_quote() seam (POSIX shlex / Windows list2cmdline) and route the install/probe hook-command quoting through it (I) - tests: derive the fake lingering PID from os.getpid() (J); make the default-host assertion platform-aware (K); add regression tests for the invalid-override, cache-invalidation, identity-mismatch, and shell_quote paths Also moved tmux's registration from an import side effect into _load_builtin_backends (inline, like process_host._load_builtin_hosts) so the registry can be cleared and re-loaded deterministically in tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@CodeRabbit review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/automator/cli.py (1)
121-141: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCatch seam-selection errors in preflight.
Line 127 calls
get_multiplexer()and Line 141 callsget_process_host()outside the probe's error handling. Both selectors can raise on an invalidBMAD_AUTO_MUX_BACKEND/BMAD_AUTO_PROCESS_HOST, sobmad-auto validatestill aborts instead of surfacing a preflight problem.Proposed fix
def _platform_preflight() -> tuple[list[str], list[str]]: - from .adapters.multiplexer import get_multiplexer + from .adapters.multiplexer import MultiplexerError, get_multiplexer from .process_host import get_process_host notes: list[str] = [] problems: list[str] = [] - backend = get_multiplexer() - label = type(backend).__name__ try: - if backend.available(): - version = backend.version() - notes.append(f"multiplexer {label} available" + (f" ({version})" if version else "")) - else: - problems.append( - f"multiplexer {label} unavailable — its transport binary is not on PATH; " - f"see `bmad-auto diagnose`" - ) - except Exception as e: # noqa: BLE001 — a misbehaving backend must not abort validate - problems.append(f"multiplexer {label} readiness check failed: {e}") - - notes.append(f"process host: {type(get_process_host()).__name__}") + backend = get_multiplexer() + except MultiplexerError as e: + problems.append(str(e)) + else: + label = type(backend).__name__ + try: + if backend.available(): + version = backend.version() + notes.append(f"multiplexer {label} available" + (f" ({version})" if version else "")) + else: + problems.append( + f"multiplexer {label} unavailable — its transport binary is not on PATH; " + f"see `bmad-auto diagnose`" + ) + except Exception as e: # noqa: BLE001 — a misbehaving backend must not abort validate + problems.append(f"multiplexer {label} readiness check failed: {e}") + + try: + notes.append(f"process host: {type(get_process_host()).__name__}") + except ProcessHostError as e: + problems.append(str(e)) return notes, problems🤖 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/automator/cli.py` around lines 121 - 141, Catch selector failures in the preflight probe so invalid BMAD_AUTO_MUX_BACKEND or BMAD_AUTO_PROCESS_HOST values do not abort validate. Move the get_multiplexer() and get_process_host() lookups in src/automator/cli.py into the existing preflight error handling around the readiness checks, and record a problem message instead of letting exceptions escape. Keep the current notes/problems flow in the validate probe and ensure backend/process host selection errors are reported through problems using the existing get_multiplexer, get_process_host, and bmad-auto validate path.
🤖 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.
Outside diff comments:
In `@src/automator/cli.py`:
- Around line 121-141: Catch selector failures in the preflight probe so invalid
BMAD_AUTO_MUX_BACKEND or BMAD_AUTO_PROCESS_HOST values do not abort validate.
Move the get_multiplexer() and get_process_host() lookups in
src/automator/cli.py into the existing preflight error handling around the
readiness checks, and record a problem message instead of letting exceptions
escape. Keep the current notes/problems flow in the validate probe and ensure
backend/process host selection errors are reported through problems using the
existing get_multiplexer, get_process_host, and bmad-auto validate path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 281cd66d-e91b-4a91-83e5-b1a5cb42ecfe
📒 Files selected for processing (11)
src/automator/adapters/multiplexer.pysrc/automator/adapters/tmux_backend.pysrc/automator/adapters/tmux_base.pysrc/automator/cli.pysrc/automator/data/plugins/unity/unity_teardown.pysrc/automator/install.pysrc/automator/probe.pysrc/automator/process_host.pytests/test_backend_registry.pytests/test_engine_plugin.pytests/test_process_host.py
🚧 Files skipped from review as they are similar to previous changes (5)
- src/automator/probe.py
- src/automator/data/plugins/unity/unity_teardown.py
- src/automator/install.py
- src/automator/adapters/tmux_base.py
- src/automator/process_host.py
| notes: list[str] = [] | ||
| problems: list[str] = [] | ||
|
|
||
| backend = get_multiplexer() |
There was a problem hiding this comment.
src/automator/cli.py:127: _platform_preflight() calls get_multiplexer() (and later get_process_host()) outside the try/except, so a misconfigured override (e.g. unknown BMAD_AUTO_MUX_BACKEND / BMAD_AUTO_PROCESS_HOST) can crash bmad-auto validate with an uncaught exception instead of reporting it as a preflight problem.
Severity: medium
Other Locations
src/automator/cli.py:141
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Valid — fixed in f69b6d8. Both seam selection calls (get_multiplexer() and get_process_host()) are now inside per-seam try blocks, so a bad BMAD_AUTO_MUX_BACKEND/BMAD_AUTO_PROCESS_HOST is reported as a preflight problem instead of aborting validate. Verified end-to-end: BMAD_AUTO_MUX_BACKEND=bogus … validate now prints multiplexer preflight failed: …matches no registered backend; known: tmux and exits cleanly. Regression tests added.
Round-1's fail-fast change (get_multiplexer/get_process_host raise on an unknown BMAD_AUTO_MUX_BACKEND/BMAD_AUTO_PROCESS_HOST) combined with a guard that only wrapped available()/version() left both *selection* calls outside the try, so a bad override crashed `bmad-auto validate` with an uncaught traceback instead of reporting it — the very thing the preflight exists to do gracefully. Flagged by both reviewers (Augment inline, CodeRabbit outside-diff). Wrap each seam (multiplexer, process host) in its own try so a selection or readiness failure becomes a reported problem and one seam's failure still lets the other report. Broad Exception (matching the existing readiness guard) keeps validate crash-proof without new imports. Adds regression tests that a raising get_multiplexer()/get_process_host() is surfaced in problems rather than propagated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai Addressed your outside-diff finding on |
|
Tip For best results, initiate chat on the files or code changes.
Catching seam-selection failures per seam in 🐇👍 ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Release 0.7.6 — the platform-seam closeout series (multiplexer registry +
ProcessHost) plus its documentation.Changed
register_multiplexer(BMAD_AUTO_MUX_BACKENDoverride) instead of hardcoded inget_multiplexer(); tmux split into a reusableBaseTmuxBackendextension point (one overridable_run()primitive) + thinTmuxMultiplexerPOSIX leaf.ProcessHostseam — terminate / force-kill / liveness / PID-reuse-identity route throughget_process_host()(BMAD_AUTO_PROCESS_HOSToverride);WindowsProcessHostships ready to register. Hook registration takes its interpreter prefix fromProcessHost.hook_interpreter()(no hardcodedpython3), andbmad-auto validateruns a platform preflight. Linux/macOS/WSL behavior unchanged.ProcessHostand helper scripts run undersys.executable(may importautomatorseams).Added
Release mechanics
trunk checkclean; release + portability-guard + module-skills-sync tests pass.Once merged to
main, the Release workflow auto-creates thev0.7.6tag and GitHub release from the CHANGELOG.🤖 Generated with Claude Code
Summary by CodeRabbit
validatenow performs a platform preflight that reports backend readiness and the selected process host.