From edc14c2ee36b08cc583eeab4488314d580f89b62 Mon Sep 17 00:00:00 2001 From: ColumbusLabs <287001685+ColumbusLabs@users.noreply.github.com> Date: Wed, 12 Aug 2026 11:57:18 -0400 Subject: [PATCH 1/8] fix(test): skip broken opencode shims --- CHANGELOG.md | 4 +++ tests/opencode_support.py | 20 ++++++++++++ tests/test_opencode_http.py | 63 +++++++++++++++++++++++++++++++++++++ tests/test_opencode_live.py | 5 ++- 4 files changed, 89 insertions(+), 3 deletions(-) create mode 100644 tests/opencode_support.py diff --git a/CHANGELOG.md b/CHANGELOG.md index eaa06b60..1b426352 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,10 @@ breaking changes may land in a minor release. ### Fixed +- **The zero-token OpenCode live smoke skips stale or broken shims (#294).** Its availability + gate now requires `opencode --version` to succeed before starting a server; runnable installs + still fail loudly when the pinned API contract drifts. + - **Policy loading now enforces the declared timeout and result-less Stop nudge minima (#648).** The valid `session_timeout_min = 1` and `stop_without_result_nudges = 0` boundaries remain accepted, while smaller values now raise `PolicyError`. diff --git a/tests/opencode_support.py b/tests/opencode_support.py new file mode 100644 index 00000000..fed0a1e0 --- /dev/null +++ b/tests/opencode_support.py @@ -0,0 +1,20 @@ +"""Shared zero-token availability checks for the OpenCode test suites.""" + +from __future__ import annotations + +import shutil +import subprocess +import sys + + +def _opencode_runs() -> bool: + """Whether a usable POSIX ``opencode`` binary is available.""" + if sys.platform == "win32": + return False + if (binary := shutil.which("opencode")) is None: + return False + try: + probe = subprocess.run([binary, "--version"], capture_output=True, timeout=10, check=False) + except (OSError, subprocess.SubprocessError): + return False + return probe.returncode == 0 diff --git a/tests/test_opencode_http.py b/tests/test_opencode_http.py index 24c60c90..f33a35fd 100644 --- a/tests/test_opencode_http.py +++ b/tests/test_opencode_http.py @@ -23,6 +23,7 @@ import pytest from conftest import write_script_launcher +from opencode_support import _opencode_runs from bmad_loop.adapters import generic, opencode_http from bmad_loop.adapters.base import SessionHandle, SessionResult, SessionSpec @@ -47,6 +48,68 @@ from bmad_loop.policy import LimitsPolicy, NotifyPolicy, Policy from bmad_loop.process_host import ProcessHostError, get_process_host + +def test_live_gate_requires_a_successful_version_probe(monkeypatch): + calls = [] + + def failed_probe(command, **kwargs): + calls.append((command, kwargs)) + return subprocess.CompletedProcess([], returncode=2) + + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.setattr("opencode_support.shutil.which", lambda _binary: "/usr/bin/opencode") + monkeypatch.setattr("opencode_support.subprocess.run", failed_probe) + assert not _opencode_runs() + assert calls == [ + ( + ["/usr/bin/opencode", "--version"], + {"capture_output": True, "timeout": 10, "check": False}, + ) + ] + + +@pytest.mark.parametrize( + "error", + [OSError("broken shim"), subprocess.TimeoutExpired("opencode", timeout=10)], +) +def test_live_gate_treats_probe_errors_as_unavailable(monkeypatch, error): + calls = [] + + def raise_error(command, **kwargs): + calls.append((command, kwargs)) + raise error + + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.setattr("opencode_support.shutil.which", lambda _binary: "/usr/bin/opencode") + monkeypatch.setattr("opencode_support.subprocess.run", raise_error) + assert not _opencode_runs() + assert calls == [ + ( + ["/usr/bin/opencode", "--version"], + {"capture_output": True, "timeout": 10, "check": False}, + ) + ] + + +def test_live_gate_accepts_a_runnable_binary(monkeypatch): + calls = [] + + def successful_probe(command, **kwargs): + calls.append((command, kwargs)) + return subprocess.CompletedProcess([], returncode=0) + + monkeypatch.setattr(sys, "platform", "linux") + monkeypatch.setattr("opencode_support.shutil.which", lambda _binary: "/usr/bin/opencode") + monkeypatch.setattr("opencode_support.subprocess.run", successful_probe) + assert _opencode_runs() + assert calls == [ + ( + ["/usr/bin/opencode", "--version"], + {"capture_output": True, "timeout": 10, "check": False}, + ) + ] + + # A pinned example timestamp from the pins file (§4): OpenCode `time.*` values # are epoch MILLISECONDS. The proof-of-work floor must live in the same unit — # a ns-vs-ms comparison is always False and silently disables the poll diff --git a/tests/test_opencode_live.py b/tests/test_opencode_live.py index 535e8889..ff14b9f1 100644 --- a/tests/test_opencode_live.py +++ b/tests/test_opencode_live.py @@ -20,13 +20,12 @@ import json import re -import shutil import socket -import sys import pytest +from opencode_support import _opencode_runs -HAVE_OPENCODE = sys.platform != "win32" and shutil.which("opencode") is not None +HAVE_OPENCODE = _opencode_runs() pytestmark = pytest.mark.skipif( not HAVE_OPENCODE, reason="live smoke needs a real `opencode` binary (POSIX)" ) From 84de44d7d3c188e8e109a4099f06c0ed18b7449b Mon Sep 17 00:00:00 2001 From: t Date: Tue, 18 Aug 2026 16:37:21 -0700 Subject: [PATCH 2/8] test(opencode): move the runnable-binary gate into conftest (#294) Restructure only. The probe's behavior is unchanged and stays verified; the review found the fix had landed in the wrong shape, not with the wrong logic. - Retire tests/opencode_support.py. The helper moves into tests/conftest.py as a PUBLIC `opencode_runs()`, beside `needs_strict_codec`, matching the house rule that conftest holds lifecycle/isolation fixtures plus plain helpers imported by name. It stays a function rather than a `skipif` constant: a constant would fire a subprocess at conftest import, per xdist worker, on every pytest invocation, selected opencode tests or not. - The probe gains `stdin=subprocess.DEVNULL`. With a real tty on stdin a shim that prompts stalls the probe for the full timeout (measured: 4.00s with an inherited tty vs 0.00s with DEVNULL). - tests/test_opencode_live.py keeps its in-file `HAVE_OPENCODE` gate, which the *_live.py suffix convention requires, and widens the skip reason to name both causes: absent from PATH, or on PATH and failing --version. - tests/test_opencode_http.py is reverted to origin/main. The gate tests never belonged there, and leaving the file untouched keeps docs/testing.md's "trim the three dated ABLATION comments on next touch" register row honest. Confirmed safe: main has not moved that file since the merge-base (01ad843), so the checkout is a true revert, not a pull-forward of main's 23 commits. - The gate tests move to tests/test_conftest.py as four rows using SUBSET kwargs assertions rather than dict equality, so each fact is graded by exactly one row and an additive kwarg stays free. The fourth row is new: the win32 early-return was untested, and deleting it left every existing test green. Verification (Linux, opencode 1.18.2 installed and runnable): pytest tests/test_conftest.py tests/test_opencode_http.py -q 95 passed pytest tests/test_opencode_live.py -q 5 passed (ran for real against the binary; did not skip) pytest -q -n logical 5792 passed, 50 skipped, 0 failed pyright 0 errors, 0 warnings, 0 informations trunk check --no-fix no issues, 4 modified files git diff origin/main -- tests/test_opencode_http.py empty Ablations, run SINGLY against a `cp` backup of tests/conftest.py (never `git checkout`) and restored byte-identical -- md5 dcaf3437214ade6a616a5f31f0d32c04 before and after all five -- each graded by `pytest tests/test_conftest.py`: win32 early-return 1 failed, 6 passed gate_answers_win32_without_touching_the_host try/except 2 failed, 5 passed gate_refuses_a_binary_that_cannot_be_launched [launch-fault] and [timeout] returncode == 0 1 failed, 6 passed gate_refuses_a_binary_that_exits_nonzero timeout=10 1 failed, 6 passed gate_probes_the_resolved_binary stdin=DEVNULL 1 failed, 6 passed gate_probes_the_resolved_binary Every mutation reddens exactly one row. The try/except mutation reports two test ids because that row is parametrized over OSError and TimeoutExpired; both belong to the one row, which is what its own record predicts. --- tests/conftest.py | 42 +++++++++++ tests/opencode_support.py | 20 ------ tests/test_conftest.py | 139 +++++++++++++++++++++++++++++++++++- tests/test_opencode_http.py | 63 ---------------- tests/test_opencode_live.py | 8 ++- 5 files changed, 185 insertions(+), 87 deletions(-) delete mode 100644 tests/opencode_support.py diff --git a/tests/conftest.py b/tests/conftest.py index 4d52285e..c8680cc2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -69,6 +69,48 @@ def _codec_rejects_bad_byte() -> bool: ) +def opencode_runs() -> bool: + """Whether this host has an ``opencode`` binary that actually RUNS. + + ``shutil.which`` proves only that a name resolves to a path, and a path is + not a working program: a stale WSL interop stub, or an npm wrapper whose + target has been uninstalled, resolves happily and then exits nonzero on + every invocation. Gating the live smoke on ``which`` alone therefore drove + the whole module against a shim that could never serve a session, turning a + host-shaped absence into a spurious failure (#294). Asking the binary to + identify itself is the cheapest call that tells the two apart — and it + sends no prompt, so the zero-token invariant holds. + + ``stdin=subprocess.DEVNULL`` because a shim that prompts rather than runs + otherwise inherits the runner's tty and blocks until the timeout expires + (measured: 4.00s stall on an inherited tty, 0.00s with DEVNULL). win32 + returns before either step: opencode-on-Windows is unverified for this + adapter (README adapter table), so there is nothing to probe for. + + Deliberately a function and not a ``skipif`` constant beside + ``needs_strict_codec``: a module-level constant here would spawn a + subprocess at conftest import — on every pytest invocation, in every xdist + worker, whether or not any opencode test was selected. Callers keep their + own in-file ``HAVE_OPENCODE`` gate, which the ``*_live.py`` suffix + convention requires anyway. + """ + if sys.platform == "win32": + return False + if (binary := shutil.which("opencode")) is None: + return False + try: + probe = subprocess.run( + [binary, "--version"], + capture_output=True, + timeout=10, + check=False, + stdin=subprocess.DEVNULL, + ) + except (OSError, subprocess.SubprocessError): + return False + return probe.returncode == 0 + + @pytest.fixture def force_tmux_backend(monkeypatch): """Pin the tmux transport backend by name, regardless of host platform. diff --git a/tests/opencode_support.py b/tests/opencode_support.py deleted file mode 100644 index fed0a1e0..00000000 --- a/tests/opencode_support.py +++ /dev/null @@ -1,20 +0,0 @@ -"""Shared zero-token availability checks for the OpenCode test suites.""" - -from __future__ import annotations - -import shutil -import subprocess -import sys - - -def _opencode_runs() -> bool: - """Whether a usable POSIX ``opencode`` binary is available.""" - if sys.platform == "win32": - return False - if (binary := shutil.which("opencode")) is None: - return False - try: - probe = subprocess.run([binary, "--version"], capture_output=True, timeout=10, check=False) - except (OSError, subprocess.SubprocessError): - return False - return probe.returncode == 0 diff --git a/tests/test_conftest.py b/tests/test_conftest.py index ac41f2ae..9ddb42f1 100644 --- a/tests/test_conftest.py +++ b/tests/test_conftest.py @@ -1,12 +1,23 @@ -"""Contract tests for the sandbox fixtures in `tests/conftest.py`. +"""Contract tests for the shared fixtures and host-capability gates in +`tests/conftest.py`. `project` hands every test a copytree clone of a session-scoped template repo, so the template's shape is a shared dependency of most of the suite. What is pinned here is the part of that shape other modules rely on without asserting it. + +The gates need the same treatment for a sharper reason: `opencode_runs` decides +whether an entire `*_live.py` module runs or skips, and nothing downstream can +notice when it answers wrongly — a gate that wrongly says "absent" reports a +tidy skip, not a failure. Its call shape and each of its three refusals are +therefore pinned here, one fact per row. """ from __future__ import annotations +import subprocess + +import conftest +import pytest from conftest import make_git_noisy from bmad_loop import verify @@ -64,3 +75,129 @@ def test_make_git_noisy_produces_rc_zero_stderr(project): assert proc.stderr.strip() # git really did write to stderr sha = proc.stdout.strip() assert len(sha) == 40 and all(c in "0123456789abcdef" for c in sha) + + +def test_opencode_gate_probes_the_resolved_binary(monkeypatch): + """The one row that owns the probe's call shape. + + `opencode_runs` decides whether `tests/test_opencode_live.py` runs at all, + and the shape of this single call is what makes that decision mean + anything: the resolved path rather than the bare name (`which` already + answered that question), a bounded `timeout` so a wedged shim cannot hang + collection, `check=False` so a nonzero exit arrives as data instead of an + exception the caller never asked to handle, and `stdin=DEVNULL` so a shim + that prompts is refused immediately instead of stalling for the full + timeout on the runner's inherited tty. + + The `kwargs` assertions are deliberately a SUBSET, not a dict equality: + equality would make deleting `timeout=10` redden this row and both refusal + rows at once, grading none of them. Each fact is graded here and only here, + and an additive kwarg stays free. + + Ablation target: delete `timeout=10` from the `subprocess.run` call and this + test fails alone, on `KeyError: 'timeout'`; delete `stdin=subprocess.DEVNULL` + and it fails alone the same way. Neither mutation is visible to any other + row in this file.""" + calls = [] + + def probe(command, **kwargs): + calls.append((command, kwargs)) + return subprocess.CompletedProcess(command, returncode=0) + + monkeypatch.setattr(conftest.sys, "platform", "linux") + monkeypatch.setattr(conftest.shutil, "which", lambda _name: "/usr/bin/opencode") + monkeypatch.setattr(conftest.subprocess, "run", probe) + + assert conftest.opencode_runs() + + ((command, kwargs),) = calls + assert command == ["/usr/bin/opencode", "--version"] + assert kwargs["timeout"] == 10 + assert kwargs["capture_output"] is True + assert kwargs["check"] is False + assert kwargs["stdin"] is subprocess.DEVNULL + + +def test_opencode_gate_refuses_a_binary_that_exits_nonzero(monkeypatch): + """#294 itself: the dead shim `shutil.which` resolves without complaint. + + A stale WSL interop stub, or an npm wrapper whose target was uninstalled, + still occupies a PATH entry and still answers `--version` — nonzero. Before + the probe the live module read that as an install and ran the entire smoke + against something that could never serve a session. + + Ablation target: replace `return probe.returncode == 0` with `return True` + and this test fails alone, on the leading `not` — the call-shape row still + sees its one correctly-shaped call, and both launch-fault parameters still + return False out of the `except` without reaching the changed line.""" + calls = [] + + def failed_probe(command, **kwargs): + calls.append((command, kwargs)) + return subprocess.CompletedProcess(command, returncode=2) + + monkeypatch.setattr(conftest.sys, "platform", "linux") + monkeypatch.setattr(conftest.shutil, "which", lambda _name: "/usr/bin/opencode") + monkeypatch.setattr(conftest.subprocess, "run", failed_probe) + + assert not conftest.opencode_runs() + assert len(calls) == 1 + + +@pytest.mark.parametrize( + "error", + [OSError("broken shim"), subprocess.TimeoutExpired("opencode", timeout=10)], + ids=["launch-fault", "timeout"], +) +def test_opencode_gate_refuses_a_binary_that_cannot_be_launched(monkeypatch, error): + """The two ways a resolved path fails before it can exit at all: the exec + faults (`OSError` — a shim naming a deleted interpreter, a dropped mount), + or it never returns inside the bound (`TimeoutExpired`). Both are host-shaped + absence rather than a suite defect, so both have to become a skip — an + exception here escapes at module import of the live suite, where it is an + error, not a skip. + + Ablation target: delete the `except (OSError, subprocess.SubprocessError): + return False` and this test fails alone, in BOTH parameters, on the escaped + exception. `TimeoutExpired` is what proves the `SubprocessError` half of the + tuple is load-bearing: it is not an `OSError`, so an `except OSError` alone + reddens that parameter and only that one.""" + calls = [] + + def raise_error(command, **kwargs): + calls.append((command, kwargs)) + raise error + + monkeypatch.setattr(conftest.sys, "platform", "linux") + monkeypatch.setattr(conftest.shutil, "which", lambda _name: "/usr/bin/opencode") + monkeypatch.setattr(conftest.subprocess, "run", raise_error) + + assert not conftest.opencode_runs() + assert len(calls) == 1 + + +def test_opencode_gate_answers_win32_without_touching_the_host(monkeypatch): + """The win32 early-out, which nothing else in the suite grades. + + opencode-on-Windows is unverified for this adapter (README adapter table), + so the answer there is False by policy — and it has to be reached before the + PATH lookup and before the probe, because Windows CI should pay for + neither. Poisoning both `shutil.which` and `subprocess.run` is how the + ordering is asserted rather than just the return value: either one being + reached is an `AssertionError`. + + Ablation target: delete the `if sys.platform == "win32": return False` early + return and this test fails alone, on the `AssertionError` the poisoned + `shutil.which` raises. The other three rows all pin `platform` to "linux" to + stay host-independent, so they stay GREEN under that same mutation — which + is the whole reason this row exists: without it, deleting the early return + leaves this file, and the suite, entirely green.""" + + def refuse(*args, **kwargs): + raise AssertionError("win32 must answer before any PATH lookup or probe") + + monkeypatch.setattr(conftest.sys, "platform", "win32") + monkeypatch.setattr(conftest.shutil, "which", refuse) + monkeypatch.setattr(conftest.subprocess, "run", refuse) + + assert not conftest.opencode_runs() diff --git a/tests/test_opencode_http.py b/tests/test_opencode_http.py index f33a35fd..24c60c90 100644 --- a/tests/test_opencode_http.py +++ b/tests/test_opencode_http.py @@ -23,7 +23,6 @@ import pytest from conftest import write_script_launcher -from opencode_support import _opencode_runs from bmad_loop.adapters import generic, opencode_http from bmad_loop.adapters.base import SessionHandle, SessionResult, SessionSpec @@ -48,68 +47,6 @@ from bmad_loop.policy import LimitsPolicy, NotifyPolicy, Policy from bmad_loop.process_host import ProcessHostError, get_process_host - -def test_live_gate_requires_a_successful_version_probe(monkeypatch): - calls = [] - - def failed_probe(command, **kwargs): - calls.append((command, kwargs)) - return subprocess.CompletedProcess([], returncode=2) - - monkeypatch.setattr(sys, "platform", "linux") - monkeypatch.setattr("opencode_support.shutil.which", lambda _binary: "/usr/bin/opencode") - monkeypatch.setattr("opencode_support.subprocess.run", failed_probe) - assert not _opencode_runs() - assert calls == [ - ( - ["/usr/bin/opencode", "--version"], - {"capture_output": True, "timeout": 10, "check": False}, - ) - ] - - -@pytest.mark.parametrize( - "error", - [OSError("broken shim"), subprocess.TimeoutExpired("opencode", timeout=10)], -) -def test_live_gate_treats_probe_errors_as_unavailable(monkeypatch, error): - calls = [] - - def raise_error(command, **kwargs): - calls.append((command, kwargs)) - raise error - - monkeypatch.setattr(sys, "platform", "linux") - monkeypatch.setattr("opencode_support.shutil.which", lambda _binary: "/usr/bin/opencode") - monkeypatch.setattr("opencode_support.subprocess.run", raise_error) - assert not _opencode_runs() - assert calls == [ - ( - ["/usr/bin/opencode", "--version"], - {"capture_output": True, "timeout": 10, "check": False}, - ) - ] - - -def test_live_gate_accepts_a_runnable_binary(monkeypatch): - calls = [] - - def successful_probe(command, **kwargs): - calls.append((command, kwargs)) - return subprocess.CompletedProcess([], returncode=0) - - monkeypatch.setattr(sys, "platform", "linux") - monkeypatch.setattr("opencode_support.shutil.which", lambda _binary: "/usr/bin/opencode") - monkeypatch.setattr("opencode_support.subprocess.run", successful_probe) - assert _opencode_runs() - assert calls == [ - ( - ["/usr/bin/opencode", "--version"], - {"capture_output": True, "timeout": 10, "check": False}, - ) - ] - - # A pinned example timestamp from the pins file (§4): OpenCode `time.*` values # are epoch MILLISECONDS. The proof-of-work floor must live in the same unit — # a ns-vs-ms comparison is always False and silently disables the poll diff --git a/tests/test_opencode_live.py b/tests/test_opencode_live.py index ff14b9f1..acf3a8a4 100644 --- a/tests/test_opencode_live.py +++ b/tests/test_opencode_live.py @@ -23,11 +23,13 @@ import socket import pytest -from opencode_support import _opencode_runs +from conftest import opencode_runs -HAVE_OPENCODE = _opencode_runs() +HAVE_OPENCODE = opencode_runs() pytestmark = pytest.mark.skipif( - not HAVE_OPENCODE, reason="live smoke needs a real `opencode` binary (POSIX)" + not HAVE_OPENCODE, + reason="live smoke needs a runnable `opencode` binary (POSIX): not on PATH, or on " + "PATH but failing `--version` — e.g. a dead WSL/npm shim (#294)", ) httpx = pytest.importorskip("httpx") From ccf44cc80202096405219c525518b0b3a396c98b Mon Sep 17 00:00:00 2001 From: t Date: Tue, 18 Aug 2026 16:52:43 -0700 Subject: [PATCH 3/8] fix(validate): report a binary that is on PATH but will not run (#294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The production twin of the test-side gate phase 1 moved into conftest. The `adapter.binary` gate asked `shutil.which`, which a dead WSL/npm shim satisfies — it is a real file with the execute bit — so `bmad-loop validate` went green on an install that could not start a session. Worse, opencode_http's own "binary not found" error tells the user to "see `bmad-loop validate`", which then told them everything was fine. - probe.py gains `binary_runs(binary, timeout_s=10) -> int | None`: one ` --version`, returning the code, or None when the process could not be launched or timed out. It sits beside `_run_capture` and reuses its exact `(OSError, subprocess.SubprocessError)` guard. Never raising is load-bearing, not defensive style — machine.py records that every gate in `cmd_validate` runs inside a try so "the command has no error path of its own — its rc is purely the verdict". `stdin=DEVNULL` matches the conftest twin for the same measured reason (4.00s with an inherited tty vs 0.00s). No `text=True`: nothing reads the output, so the locale decode that forced `errors="replace"` onto `_run_capture` (#383) never happens and cannot raise the UnicodeDecodeError the guard does not name. - `run_version_help` is untouched and is deliberately NOT reused: it discards the return code by design and spawns TWO children at `timeout_s` each, so a hung shim would cost up to 20s per profile inside an interactive command (the TUI's `v` key runs the same path). - checks.py registers a NEW id, `adapter.binary-unrunnable`, rather than a third severity on `adapter.binary`. That is the rule the module already states for the skills.base-* family: ids split where the outcomes are genuinely different findings with different detail — here a resolved path and a return code. - cmd_validate leaves the existing `adapter.binary` branch exactly as it was (found -> ok, absent -> problem) and probes only after an ok, against the path `which` RETURNED, never the bare name — re-resolving would be a TOCTOU, and on Windows the PATHEXT shim `which` picked is the very file at issue. - Severity is `warning`, deliberately. `problem` IS validate's exit code (checks.py) and rc is a compatibility contract (AGENTS.md); nothing rules out one of claude/codex/gemini/copilot/antigravity answering `--version` nonzero on a live install, and that user must not start failing validate. - Gated on `rc != 0`, not an allowlist: #294's transcript reports rc 2 and a reproduction of the same shim exits 127 — the code is a property of the shell and the failure mode, so {126, 127} would miss the case being fixed. - VALIDATE_SCHEMA_VERSION stays 1. A new check id is purely additive under the documents.py contract. The TUI needs no change: widgets.py switches on severity, never on check id, `_detail_lines` already models int/None scalars, and the id is 25 chars, under `_FINDING_CHECK_WIDTH`. Test-side risk handled first: `_make_validate_pass` stubs `cli.shutil.which` to `/usr/bin/{tool}`, a path that does not exist, and fourteen call sites depend on it. It now also pins `binary_runs` to rc 0, with the docstring saying why — unpinned, every one of those tests would spawn a nonexistent path on each run. Ablations, each applied singly against a `cp` backup and reverted: probe.py guard -> `except OSError` only the timeout row reds (raised TimeoutExpired) guard -> `except SubprocessError` only the launch-fault row reds (raised FileNotFoundError) drop `stdin=subprocess.DEVNULL` only the contract row reds (KeyError) drop `check=False` only the contract row reds (KeyError); it is the stdlib default, so this is a behavioral no-op — recorded as such in the docstring `check=False` -> `check=True` rc-2, rc-127 AND the contract row red `return proc.returncode` -> `return 0` rc-2 and rc-127 red; rc-0 survives cli.py `report.warn` -> `report.fail` both cli legs red (severity is the rc) delete the probe branch both cli legs red on len([]) == 0 gate -> `rc in {126, 127}` the rc-2 leg reds ALONE — which is what the parametrized pair buys detail `path` -> the bare name both cli legs red (FileNotFoundError from samefile) The two guard-family ablations redden disjoint rows; that disjointness is the proof both families are named for a reason. Verification (Linux): pytest tests/test_probe.py tests/test_cli.py -q 476 passed, 2 skipped pytest -q -n logical 5800 passed, 50 skipped, 0 failed pyright 0 errors, 0 warnings, 0 informations trunk check --all --no-fix no issues, 258 files By hand, against a real dead shim (`exec /nonexistent/opencode "$@"`, exits 127) on a scratch project whose policy names opencode: baseline, real runnable opencode on PATH "ok: opencode found", no warning, rc 1 (pre-existing config/git problems) with the shim shadowing it "ok: warning: opencode is on PATH at but `opencode --version` exited 127 ...", rc STILL 1 — the warning adds nothing to the verdict --json with the shim whole stdout parses; schema_version 1; adapter.binary still ok; adapter.binary-unrunnable at warning with detail {binary, path: , returncode: 127} The baseline leg is the one that matters most: a real, working opencode install draws no warning, so the probe does not false-positive. --- CHANGELOG.md | 11 +++++ src/bmad_loop/checks.py | 1 + src/bmad_loop/cli.py | 33 ++++++++++++- src/bmad_loop/probe.py | 42 ++++++++++++++++ tests/test_cli.py | 96 ++++++++++++++++++++++++++++++++++-- tests/test_probe.py | 106 +++++++++++++++++++++++++++++++++++++++- 6 files changed, 282 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b426352..83613f81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,17 @@ breaking changes may land in a minor release. `failed` or `dirty` — never an exception message, which `diagnose` would refuse to emit at all. The `--json` key is additive and always present, so `STATUS_SCHEMA_VERSION` is unchanged. +- **`validate` now reports a binary that is on PATH but will not run (#294).** The + `adapter.binary` gate asked `shutil.which`, which a dead WSL/npm shim satisfies — it is a real + file with the execute bit — so validate went green on an install that could not start a session, + and the opencode adapter's own "binary not found" error sent the user to `bmad-loop validate` to + be told everything was fine. Each resolved binary is now run once as ` --version`; a + nonzero exit or a launch fault reports the new check id `adapter.binary-unrunnable`, carrying the + resolved path and the return code. The severity is `warning`, so validate's exit code is + unchanged for a live CLI that merely answers `--version` oddly, and `adapter.binary` keeps its + existing found/absent meaning. The check id is additive, so `VALIDATE_SCHEMA_VERSION` is + unchanged. + ### Changed - **Files the orchestrator replaces by name now land at `0600`.** Those writes pass diff --git a/src/bmad_loop/checks.py b/src/bmad_loop/checks.py index 0fb60268..a2b73892 100644 --- a/src/bmad_loop/checks.py +++ b/src/bmad_loop/checks.py @@ -50,6 +50,7 @@ "policy.isolation-repo-root", "adapter.profile", "adapter.binary", + "adapter.binary-unrunnable", "adapter.hookless", "adapter.httpx", "adapter.kind", diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 0fff9817..aa87207f 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -421,11 +421,42 @@ def cmd_validate(args: argparse.Namespace) -> int: {"platform": sys.platform}, ) + from . import probe as probe_mod + for tool in dict.fromkeys(p.binary for p in profiles): - if shutil.which(tool): + resolved = shutil.which(tool) + if resolved: report.ok("adapter.binary", f"{tool} found", {"binary": tool}) else: report.fail("adapter.binary", f"{tool} not found on PATH", {"binary": tool}) + continue + # #294: the gate above answers "a file with that name carries the execute + # bit", which a dead WSL/npm shim satisfies while every launch of it fails. + # So validate went green on an install that could not start a session — + # and opencode_http's own "binary not found" remedy points the user at + # `bmad-loop validate`, which then told them everything was fine. Probe the + # path `which` RETURNED rather than the bare name: re-resolving is a TOCTOU, + # and on Windows the PATHEXT shim `which` picked is the very file at issue. + rc = probe_mod.binary_runs(resolved) + if rc == 0: + continue + # Any nonzero code, never an allowlist: #294's own transcript reports rc 2 + # and a reproduction of the same shim exits 127, the code being a property + # of the shell and the failure mode. {126, 127} would miss the case fixed. + # + # `warning`, deliberately, and not to be promoted without evidence: severity + # `problem` is validate's exit code (checks.py), and rc is a compatibility + # contract (AGENTS.md). Nothing rules out one of claude/codex/gemini/copilot/ + # antigravity answering `--version` nonzero on a perfectly live install, and + # that user must not start failing validate. + outcome = "could not be launched" if rc is None else f"exited {rc}" + report.warn( + "adapter.binary-unrunnable", + f"{tool} is on PATH at {resolved} but `{tool} --version` {outcome} — " + "that is a stale or broken install (a dead WSL/npm shim is the usual " + "cause), and runs using it will fail to start; reinstall it or fix PATH", + {"binary": tool, "path": resolved, "returncode": rc}, + ) any_hooks_registered = False for profile in profiles: diff --git a/src/bmad_loop/probe.py b/src/bmad_loop/probe.py index b165563c..c2dfb062 100644 --- a/src/bmad_loop/probe.py +++ b/src/bmad_loop/probe.py @@ -194,6 +194,48 @@ def _run_capture(argv: list[str], timeout_s: float) -> str | None: return out.strip() or None +def binary_runs(binary: str, timeout_s: float = 10) -> int | None: + """Return the exit code of ``binary --version``, or None if it never ran. + + The liveness half of a PATH check. ``shutil.which`` answers "a file with that + name is on PATH and has the execute bit", which a dead WSL/npm shim satisfies + while every launch of it fails (#294) — so ``validate`` reported OK on an + install that could not start a session. Running the binary once is the only + thing that separates the two. + + Never raises, and that is load-bearing rather than defensive style: machine.py + records that every gate in ``cmd_validate`` runs inside a ``try`` so "the + command has no error path of its own — its rc is purely the verdict". A probe + that raised would give it one. The guard is ``_run_capture``'s exactly, and + the return is deliberately left as bytes (no ``text=True``): nothing here reads + the output, so the locale decode that forced ``errors="replace"`` on that + function never happens and cannot raise the ``UnicodeDecodeError`` the guard + does not name. + + None (could not launch, or timed out) and a nonzero code are separate answers + to the caller, not one sentinel: the first has no return code to report. + + ``stdin=DEVNULL`` is required, not cosmetic. With the caller's tty inherited, a + shim that prompts blocks on the read for the whole timeout — measured 4.00s + against 0.00s — inside an interactive command. + + Not folded into :func:`run_version_help`, which discards the return code by + design and spawns TWO children (``--version`` then ``--help``) at ``timeout_s`` + each: reusing it would cost up to 20s per profile here. + """ + try: + proc = subprocess.run( + [binary, "--version"], + capture_output=True, + check=False, + stdin=subprocess.DEVNULL, + timeout=timeout_s, + ) + except (OSError, subprocess.SubprocessError): + return None + return proc.returncode + + def run_version_help(binary: str, timeout_s: float = 10) -> FlagFinding: """Scrubbed ``--version``/``--help`` for a binary. Never raises.""" if not shutil.which(binary): diff --git a/tests/test_cli.py b/tests/test_cli.py index 0c10d237..4add03ab 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -28,12 +28,14 @@ spec_path, write_gated_ledger, write_ledger, + write_script_launcher, write_spec, write_sprint, ) from bmad_loop import cli, platform_util from bmad_loop import policy as policy_mod +from bmad_loop import probe as probe_mod from bmad_loop import runsetup from bmad_loop.adapters import multiplexer as mux_mod @@ -5182,11 +5184,17 @@ def test_validate_stories_folder_known_selector_ok(project): def _make_validate_pass(project, monkeypatch, capsys, *, policy=CLAUDE_ONLY_POLICY, skills=None): - """Set a project up so every validate gate passes, and pin the two gates whose + """Set a project up so every validate gate passes, and pin the three gates whose outcome is a property of the *host* rather than of the project: whether the CLI - binary is on PATH and whether a multiplexer is installed. Without those pins the - rc-0 leg would pass or fail by machine, which is exactly the kind of green that - means nothing. + binary is on PATH, whether it actually runs, and whether a multiplexer is + installed. Without those pins the rc-0 leg would pass or fail by machine, which + is exactly the kind of green that means nothing. + + The liveness pin (#294) is doubly load-bearing: `which` is stubbed to + `/usr/bin/{tool}`, a path that does not exist on this host, so an unpinned + `binary_runs` would have every one of these tests spawn a nonexistent path on + every run and report `adapter.binary-unrunnable`. Stubbed to rc 0 — the "the + binary is fine" answer — because that is the premise of a pass fixture. ``policy`` and ``skills`` exist so the dev-primitive-rename tests can vary the project's *topology* (which CLIs on which roles, which primitive era in which @@ -5205,6 +5213,7 @@ def _make_validate_pass(project, monkeypatch, capsys, *, policy=CLAUDE_ONLY_POLI git(project.project, "add", "-A") # every file above is a worktree change git(project.project, "commit", "-q", "-m", "validate fixture") monkeypatch.setattr(cli.shutil, "which", lambda tool: f"/usr/bin/{tool}") + monkeypatch.setattr(probe_mod, "binary_runs", lambda *_a, **_kw: 0) monkeypatch.setattr( cli, "_platform_preflight", @@ -5322,7 +5331,8 @@ def test_validate_json_detail_round_trips_for_every_real_shape(capsys): `machine.emit(validate_document(...))` still emits one whole, parseable document for every detail a caller actually builds. Each case below mirrors a real call site (str values, the nested `dict(role_names)` dict, str+int, install.py's - `{**detail, "marker": ...}`, and the `detail=None` leg). A future caller that + `{**detail, "marker": ...}`, #294's int and null `returncode`, and the + `detail=None` leg). A future caller that attaches a non-JSON-serializable detail fails here, by name, rather than at runtime on stdout. """ @@ -5343,6 +5353,16 @@ def test_validate_json_detail_round_trips_for_every_real_shape(capsys): "stale", {"tree": ".claude", "skill": "s", "file": "f", "marker": "m"}, ) + report.warn( # str + int `returncode` (cli.py, #294) + "adapter.binary-unrunnable", + "claude will not run", + {"binary": "claude", "path": "/usr/bin/claude", "returncode": 127}, + ) + report.warn( # the launch-fault leg: a null INSIDE a detail dict (cli.py, #294) + "adapter.binary-unrunnable", + "claude will not launch", + {"binary": "claude", "path": "/usr/bin/claude", "returncode": None}, + ) report.ok("git.worktree-clean", "clean") # the detail=None leg # the exact production path: cli.py does `machine.emit(validate_document(...))`. @@ -5354,6 +5374,10 @@ def test_validate_json_detail_round_trips_for_every_real_shape(capsys): assert by_check["queue.stories-manifest"]["detail"]["stories"] == 3 # int, not "3" assert by_check["skills.stories-dispatch-stale"]["detail"]["marker"] == "m" assert by_check["git.worktree-clean"]["detail"] is None # None -> null round-trips + # Listed, not dict-indexed: both #294 legs share one check id, so a by-check map + # would keep only the last and silently stop covering the int shape. + unrunnable = [f for f in parsed["findings"] if f["check"] == "adapter.binary-unrunnable"] + assert [f["detail"]["returncode"] for f in unrunnable] == [127, None] # int, and null-in-dict @pytest.mark.parametrize( @@ -5396,6 +5420,68 @@ def test_validate_json_every_emitted_check_is_registered(project, capsys, monkey assert emitted <= VALIDATE_CHECKS +@pytest.mark.parametrize("exit_code", [2, 127], ids=["rc-2", "rc-127"]) +def test_validate_warns_when_a_binary_on_path_refuses_to_run( + project, capsys, monkeypatch, tmp_path, exit_code +): + """#294: `which` answering yes is not the same question as "this install runs". + + A dead WSL/npm shim is a real file with the execute bit — `adapter.binary` went + green on it, and `opencode_http`'s own "binary not found" remedy sends the user + to `bmad-loop validate`, which then told them everything was fine. Driven with a + REAL non-runnable binary on a REAL PATH: a `which` stub cannot exercise the probe + at all, which is the whole of what this row is about. + + The two host pins `_make_validate_pass` installs are lifted back off on purpose — + they exist so the OTHER rows do not pass or fail by machine, and here they would + stub out the code under test. Everything else it sets up stays, so rc 0 below is + a statement about this gate rather than about some unrelated one. + + Both codes are #294's OWN evidence, not invented: its transcript reports rc 2 and + a reproduction of the same shim (`exec /nonexistent/opencode "$@"`) exits 127. The + pair is what pins the gate as `rc != 0` — an allowlist of shell-ish codes looks + right and would miss the case the issue is actually about. + + Ablation target: change `report.warn` to `report.fail` in cli.py's + `adapter.binary-unrunnable` branch and the rc-0 assertion inside `machine_json` + reddens (severity IS the exit code — checks.py); delete the branch entirely and + the `len(...) == 1` assertion reddens on an empty list. Neither is redundant: the + first is the severity contract, the second is the finding existing at all. + Narrow the gate to `rc in {126, 127}` and the rc-2 leg alone reddens. + """ + real_which, real_binary_runs = shutil.which, probe_mod.binary_runs + _make_validate_pass(project, monkeypatch, capsys) + monkeypatch.setattr(cli.shutil, "which", real_which) + monkeypatch.setattr(probe_mod, "binary_runs", real_binary_runs) + + bin_dir = tmp_path / "shimbin" + bin_dir.mkdir() + launcher = write_script_launcher(bin_dir, "claude", f"import sys\nsys.exit({exit_code})\n") + # Prepended, so it shadows any real `claude` this host happens to carry. + monkeypatch.setenv("PATH", str(bin_dir) + os.pathsep + os.environ.get("PATH", "")) + + # rc=0 is machine_json's default and IS the exit-code assertion: a warning must + # not flip validate's verdict for a user whose CLI merely answers oddly. + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys) + + assert doc["schema_version"] == 1, "purely additive — a new check id is not a break" + assert doc["ok"] is True # the document's own verdict, not just the rc + + unrunnable = [f for f in doc["findings"] if f["check"] == "adapter.binary-unrunnable"] + assert len(unrunnable) == 1, "one finding per binary, not per profile" + assert unrunnable[0]["severity"] == "warning" + assert unrunnable[0]["detail"]["binary"] == "claude" + assert unrunnable[0]["detail"]["returncode"] == exit_code + # The RESOLVED path, not the bare name — re-resolving in the probe would be a + # TOCTOU, and on Windows the PATHEXT shim `which` picked is the file at issue. + assert Path(unrunnable[0]["detail"]["path"]).samefile(launcher) + + # The pre-existing gate is untouched: it still answers "is it on PATH", and the + # answer is still yes. A fix that folded liveness into it would redden this. + found = [f for f in doc["findings"] if f["check"] == "adapter.binary"] + assert len(found) == 1 and found[0]["severity"] == "ok" + + @pytest.mark.parametrize("passing", [True, False], ids=["rc-0", "rc-1"]) def test_tui_renderer_draws_every_detail_shape_a_real_validate_emits( project, capsys, monkeypatch, passing diff --git a/tests/test_probe.py b/tests/test_probe.py index e77f6bb7..5ed821ec 100644 --- a/tests/test_probe.py +++ b/tests/test_probe.py @@ -3,10 +3,11 @@ import json import re +import subprocess import sys import pytest -from conftest import machine_json, needs_strict_codec +from conftest import machine_json, needs_strict_codec, write_script_launcher from test_probe_hook import run_hook from bmad_loop import cli, probe, sanitize @@ -750,3 +751,106 @@ def test_run_capture_replaces_undecodable_banner_bytes(tmp_path): assert out is not None assert "�" in out # the replacement, not a survivor assert "before" in out and "after" in out + + +# ------------------------------------------------------ liveness (#294) + + +@pytest.mark.parametrize("exit_code", [0, 2, 127], ids=["runs", "rc-2", "rc-127"]) +def test_binary_runs_returns_the_exit_code_of_a_real_child(tmp_path, exit_code): + """`binary_runs` reports the child's code, not a boolean — 0 is the only "this + install works" answer, and any other value is what the #294 finding carries as + its `returncode` detail. + + Both nonzero rows are the issue's OWN evidence, not invented codes: its + transcript reports rc 2 and a reproduction of the same dead shim exits 127. + They are here to pin `binary_runs` as a faithful pass-through, which is what + lets cli.py gate on `rc != 0` rather than on an allowlist of shell-ish codes — + the code depends on the shell and the failure mode, so {126, 127} would miss + the very case #294 is about. + + Driven through a real child rather than a patched `subprocess.run`: the point + of this function is that a process actually launched and exited, and a mock + proves nothing about that. + """ + launcher = write_script_launcher(tmp_path, "shim", f"import sys\nsys.exit({exit_code})\n") + + assert probe.binary_runs(str(launcher), timeout_s=30) == exit_code + + +def test_binary_runs_returns_none_when_the_process_cannot_be_launched(tmp_path): + """A path that is not there at all faults in `subprocess.run` (FileNotFoundError, + an OSError) and comes back as None — "there is no return code to report", which + is a different finding from "it ran and said 5", not the same sentinel. + + This is the guard machine.py depends on: every gate in `cmd_validate` runs inside + a try precisely so "the command has no error path of its own — its rc is purely + the verdict", and a probe that raised would hand it one. + + Ablation target: delete the `except (OSError, subprocess.SubprocessError)` clause + in `probe.binary_runs` and this reddens with a raised FileNotFoundError rather + than a None return — the two outcomes a bare "did not raise" could not tell + apart, which is why the assertion is on the value. + """ + missing = tmp_path / "nope" / "not-a-binary" + + assert probe.binary_runs(str(missing), timeout_s=30) is None + + +def test_binary_runs_returns_none_when_the_child_outlives_the_timeout(tmp_path): + """A child that never exits is killed at `timeout_s` and reported as None. + + TimeoutExpired is a `subprocess.SubprocessError`, not an OSError, so this row is + what proves the guard names BOTH families — a hang is the failure mode of a shim + that prompts, which is exactly the shape `stdin=DEVNULL` exists to avoid. + + Ablation target: narrow the guard to `except OSError` and this reddens with a + raised subprocess.TimeoutExpired. + """ + launcher = write_script_launcher(tmp_path, "hang", "import time\ntime.sleep(120)\n") + + assert probe.binary_runs(str(launcher), timeout_s=0.5) is None + + +def test_binary_runs_pins_devnull_stdin_and_the_caller_timeout(): + """The one contract row over the call itself: argv is ` --version`, stdin + is DEVNULL, the caller's timeout is honored, and a nonzero exit is data rather + than an exception (`check=False`). + + `stdin=DEVNULL` is required, not cosmetic: with the caller's tty inherited a shim + that prompts blocks on the read for the entire timeout (measured 4.00s against + 0.00s), inside an interactive command. Nothing else observes it — the real-child + rows above pass either way — so it is pinned here or not at all. + + Asserted as a SUBSET of the recorded kwargs, never dict equality: a future kwarg + (`env`, `cwd`, ...) is additive, and equality would turn every such addition into + a multi-row breakage in this file. + + Ablation target: drop `stdin=subprocess.DEVNULL` from `probe.binary_runs` and the + stdin assertion reddens with a KeyError. `check=False` is the stdlib default, so + dropping it reddens only this row (KeyError) and nothing else — it is pinned as + intent against a future flip to `check=True`, which is the mutation that bites: + that turns every nonzero exit into a CalledProcessError, a SubprocessError the + guard swallows into None, reddening both real-child rows above as well. + """ + recorded: dict = {} + + def fake_run(argv, **kwargs): + recorded["argv"] = argv + recorded["kwargs"] = kwargs + return subprocess.CompletedProcess(argv, 0) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(probe.subprocess, "run", fake_run) + assert probe.binary_runs("some-cli", timeout_s=3.5) == 0 + + assert recorded["argv"] == ["some-cli", "--version"] + kwargs = recorded["kwargs"] + assert kwargs["stdin"] is subprocess.DEVNULL + assert kwargs["timeout"] == 3.5 + assert kwargs["check"] is False + assert kwargs["capture_output"] is True + # No `text=True`: nothing reads the output, so the locale decode that forced + # `errors="replace"` onto `_run_capture` (#383) never happens here and cannot + # raise the UnicodeDecodeError the guard above does not name. + assert kwargs.get("text") is None From 3c0be906d57ffe90fc169ef4d1687fbe0a129593 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 18 Aug 2026 16:56:54 -0700 Subject: [PATCH 4/8] docs: note that validate probes a resolved binary (#294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `adapter.binary` gate answered `shutil.which`, so three docs promised resolution where the code now also requires the binary to run: - docs/testing.md — the zero-token live-gate table said the opencode smoke needs `opencode` "installed", and the suffix-convention bullet implied `HAVE_OPENCODE` is `which`-backed. Both now name the `--version` probe. - docs/adapter-authoring-guide.md — the `binary` profile-schema row promised only "resolved on `PATH`", so a profile author had no way to know validate will also probe it and warn. - docs/FEATURES.md — the validate preflight sentence listed "CLI binary" among things it resolves. No check-id table exists anywhere to update: validate ids are named only by example, so nothing else went stale. AGENTS.md needs no sync — its testing bullets state rules, and no rule changed. --- docs/FEATURES.md | 2 +- docs/adapter-authoring-guide.md | 2 +- docs/testing.md | 16 +++++++++------- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 1eaebf96..0b59ff0f 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -199,7 +199,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Setup & install - `bmad-loop init` installs the three `bmad-loop-*` skills (`bmad-loop-setup`, `bmad-loop-resolve`, `bmad-loop-sweep`, into `.claude/skills/` and/or `.agents/skills/`), the hook relay, `.bmad-loop/policy.toml`, and a gitignore covering the runs dir, plugin caches, and policy.toml itself (per-machine config). Flags: `--cli` (repeatable), `--no-skills`, `--force-skills`. -- `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary, hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. +- `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary (**probed, not just resolved**: a name that is on `PATH` but fails `--version`, typically a dead WSL/npm shim, is reported at warning severity rather than passing — [#294](https://github.com/bmad-code-org/bmad-loop/issues/294)), hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. - The preflight also **names the multiplexer selection reason wherever selection resolves** (`mux.selection`, e.g. `platform default for win32`), not only when a `BMAD_LOOP_MUX_BACKEND`/`[mux] backend` choice forced it. A `fallback` selection is reported as a warning (its own label says no available backend matches this platform); a selection that outright failed is carried by `mux.preflight`, and a detection that failed by `mux.backends-detected` at warning — so a missing `mux.selection` line is normally explained by another finding (the historical unregistered-tmux fallback is the one silent exception; see the `--json` contract note in `documents.py`). On top of that, `host.win32-on-wsl-path` warns when a **native-Windows interpreter is working on a `\\wsl.localhost\...` project** ([#332](https://github.com/bmad-code-org/bmad-loop/issues/332) — see [multiplexer-backends.md](multiplexer-backends.md) for why WSL can hand a bash prompt the Windows build). Both are diagnostics only: neither changes which backend is selected (psmux _is_ correct for a `win32` interpreter) and neither flips validate's exit code. `bmad-loop diagnose` carries the same two facts in its Environment block as `sys.platform` and `win32 on WSL distro path` (`yes`/`no`). - Non-invasive: drives the upstream dev primitive unmodified — there is no fork to keep in sync — and review is just a re-invocation of it on the `done` spec. Your standard BMAD install is never modified. diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 0eb08783..ab0a36dd 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -404,7 +404,7 @@ resolves to `claude`. | Field | Required | Default | Meaning | | ---------------------------------- | -------- | ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `name` | ✅ | — | Profile id, also the `--cli` value and override key. | -| `binary` | ✅ | — | Executable to launch (resolved on `PATH`). | +| `binary` | ✅ | — | Executable to launch (resolved on `PATH`). `bmad-loop validate` also probes it — a name that resolves but fails `--version` (typically a dead WSL/npm shim) is reported as `adapter.binary-unrunnable` at warning severity, #294. | | `[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. 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. | diff --git a/docs/testing.md b/docs/testing.md index 301f2d44..5e03f705 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -58,7 +58,9 @@ Placement rules: - **Suffix convention is the selection mechanism.** `*_e2e.py` and `*_live.py` name the modules with host requirements; each carries a module-level `pytestmark = pytest.mark.skipif(...)` naming its requirement (`HAVE_TMUX`, `HAVE_OPENCODE`, - `HAVE_PSMUX`; the opencode module adds an `importorskip` on httpx). Ordinary runs collect + `HAVE_PSMUX`; the opencode module adds an `importorskip` on httpx, and its `HAVE_OPENCODE` is + probe-backed rather than `which`-backed — conftest's `opencode_runs()` runs the binary's + `--version`, so a resolvable-but-dead shim skips instead of failing, #294). Ordinary runs collect them and skip them; a capable host runs them with no extra flags. Do not add a marker, an env-var opt-in, or a separate pytest invocation for these — the filename plus the in-file gate is the whole mechanism. - **Prefer a lower layer over a broader one.** If a defect is expressible as a pure-core case, @@ -163,12 +165,12 @@ Rules for adding or touching a guard: the live/E2E gates ("Live/E2E tests must consume zero LLM tokens"); in practice it holds everywhere, and the mechanism differs per gate — worth knowing before touching any of them: -| Gate | Real component | Zero-token mechanism | Runs where | -| ------------------------------------------------------------------ | -------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------- | -| `test_generic_tmux.py` (5 `HAVE_TMUX` test functions, 7 collected) | tmux server | The spawned "CLI" is a tiny shell script written by the test | CI Linux job + any POSIX dev box with tmux | -| `test_stories_e2e.py` | tmux + the real `bmad-loop run/resolve/resume` CLI | Scripted fake `claude` variants (bash, defined as string constants in the test module) wired in as a custom TOML profile (`fakestories`); the fake writes its own SessionStart/Stop hook events, so no `bmad-loop init` and no real CLI exists anywhere in the run | CI Linux job (Linux-only gate: the fakes use GNU coreutils + `setsid`) | -| `test_opencode_live.py` | A real `opencode serve` HTTP server | **Never sends a prompt**: only the spawn/teardown paths are used — nothing that prompts — and the tests touch health/doc/session/event endpoints; the prompt endpoint is asserted against the OpenAPI schema, never called. One test asserts the session's token/cost aggregates are zero | Manual — POSIX box with `opencode` installed | -| `test_psmux_live.py` | A real psmux on Windows | **Parked windows only**: every parked window runs `pwsh -NoProfile -Command exit 0`, and no coding CLI is ever launched. Includes `test_premise_*` probes with inverted semantics — a red probe means a workaround became droppable | Manual — Windows box with psmux on PATH | +| Gate | Real component | Zero-token mechanism | Runs where | +| ------------------------------------------------------------------ | -------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------- | +| `test_generic_tmux.py` (5 `HAVE_TMUX` test functions, 7 collected) | tmux server | The spawned "CLI" is a tiny shell script written by the test | CI Linux job + any POSIX dev box with tmux | +| `test_stories_e2e.py` | tmux + the real `bmad-loop run/resolve/resume` CLI | Scripted fake `claude` variants (bash, defined as string constants in the test module) wired in as a custom TOML profile (`fakestories`); the fake writes its own SessionStart/Stop hook events, so no `bmad-loop init` and no real CLI exists anywhere in the run | CI Linux job (Linux-only gate: the fakes use GNU coreutils + `setsid`) | +| `test_opencode_live.py` | A real `opencode serve` HTTP server | **Never sends a prompt**: only the spawn/teardown paths are used — nothing that prompts — and the tests touch health/doc/session/event endpoints; the prompt endpoint is asserted against the OpenAPI schema, never called. One test asserts the session's token/cost aggregates are zero | Manual — POSIX box with a **runnable** `opencode` — the gate probes `--version`, so a resolvable-but-dead shim skips (#294) | +| `test_psmux_live.py` | A real psmux on Windows | **Parked windows only**: every parked window runs `pwsh -NoProfile -Command exit 0`, and no coding CLI is ever launched. Includes `test_premise_*` probes with inverted semantics — a red probe means a workaround became droppable | Manual — Windows box with psmux on PATH | Never "fix" a gate to call a real CLI, and never add a completion path that trusts LLM output — sessions complete only on hook Stop events or window death, and the fakes exercise exactly From e3d026d7a560ca10f990c84e3e2e76178c07970f Mon Sep 17 00:00:00 2001 From: t Date: Tue, 18 Aug 2026 17:26:58 -0700 Subject: [PATCH 5/8] fix(validate): probe only a bare-name binary (#294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #294 liveness probe runs ` --version`, and `binary` is project-controlled end to end: policy.toml picks the profile and `.bmad-loop/profiles/*.toml` supplies its fields, both of which arrive with a clone. `shutil.which` returns a caller-supplied path UNCHANGED instead of searching PATH, so `binary = "./tool"` made `bmad-loop validate` execute a file the repository itself carries — in the one command a user runs to decide whether a checkout is safe to run at all, and which the TUI runs too. Inspection had become execution. Probe only a bare name. The predicate is `os.path.dirname`, the same test `shutil.which` itself uses to choose direct-path over PATH search, so the two cannot drift. All six packaged profiles ship a bare name, so the dead WSL/npm shim #294 is about is still caught; a path-bearing `binary` keeps the pre-#294 behavior of reported found, never launched. The new test drives a real executable through a real relative `binary` from the project directory and asserts a sentinel the child alone can write is absent — an absent `adapter.binary-unrunnable` finding would also be produced by a probe that ran and returned 0. Ablating the gate creates the sentinel, which is what makes the negative assertion mean something. Also stop diagnosing every rejected `--version` as a broken install: a declarative profile whose CLI does not implement the flag lands in this branch while its real launches work. Severity was already `warning` for that reason; the message now states the observation as fact and the broken install as the usual cause rather than as the verdict. Both reported by codex review on 3c0be90 (P1, P2). --- CHANGELOG.md | 9 +++-- docs/FEATURES.md | 2 +- docs/adapter-authoring-guide.md | 2 +- src/bmad_loop/cli.py | 20 +++++++++-- tests/test_cli.py | 62 +++++++++++++++++++++++++++++++++ 5 files changed, 88 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83613f81..9bf11e7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,9 +22,12 @@ breaking changes may land in a minor release. `adapter.binary` gate asked `shutil.which`, which a dead WSL/npm shim satisfies — it is a real file with the execute bit — so validate went green on an install that could not start a session, and the opencode adapter's own "binary not found" error sent the user to `bmad-loop validate` to - be told everything was fine. Each resolved binary is now run once as ` --version`; a - nonzero exit or a launch fault reports the new check id `adapter.binary-unrunnable`, carrying the - resolved path and the return code. The severity is `warning`, so validate's exit code is + be told everything was fine. Each binary named as a **bare name** is now run once as + ` --version`; a nonzero exit or a launch fault reports the new check id + `adapter.binary-unrunnable`, carrying the resolved path and the return code. A `binary` carrying + a path separator is resolved and reported found but never launched: profile fields are + project-supplied, and validate is the command used to decide whether a checkout is safe to run, + so it must not execute code a clone carries. The severity is `warning`, so validate's exit code is unchanged for a live CLI that merely answers `--version` oddly, and `adapter.binary` keeps its existing found/absent meaning. The check id is additive, so `VALIDATE_SCHEMA_VERSION` is unchanged. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 0b59ff0f..9b6ba71b 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -199,7 +199,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Setup & install - `bmad-loop init` installs the three `bmad-loop-*` skills (`bmad-loop-setup`, `bmad-loop-resolve`, `bmad-loop-sweep`, into `.claude/skills/` and/or `.agents/skills/`), the hook relay, `.bmad-loop/policy.toml`, and a gitignore covering the runs dir, plugin caches, and policy.toml itself (per-machine config). Flags: `--cli` (repeatable), `--no-skills`, `--force-skills`. -- `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary (**probed, not just resolved**: a name that is on `PATH` but fails `--version`, typically a dead WSL/npm shim, is reported at warning severity rather than passing — [#294](https://github.com/bmad-code-org/bmad-loop/issues/294)), hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. +- `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary (**probed, not just resolved**: a name that is on `PATH` but fails `--version`, typically a dead WSL/npm shim, is reported at warning severity rather than passing — [#294](https://github.com/bmad-code-org/bmad-loop/issues/294); only bare names are probed, since a profile-supplied path would make this diagnostic execute code a clone carries), hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. - The preflight also **names the multiplexer selection reason wherever selection resolves** (`mux.selection`, e.g. `platform default for win32`), not only when a `BMAD_LOOP_MUX_BACKEND`/`[mux] backend` choice forced it. A `fallback` selection is reported as a warning (its own label says no available backend matches this platform); a selection that outright failed is carried by `mux.preflight`, and a detection that failed by `mux.backends-detected` at warning — so a missing `mux.selection` line is normally explained by another finding (the historical unregistered-tmux fallback is the one silent exception; see the `--json` contract note in `documents.py`). On top of that, `host.win32-on-wsl-path` warns when a **native-Windows interpreter is working on a `\\wsl.localhost\...` project** ([#332](https://github.com/bmad-code-org/bmad-loop/issues/332) — see [multiplexer-backends.md](multiplexer-backends.md) for why WSL can hand a bash prompt the Windows build). Both are diagnostics only: neither changes which backend is selected (psmux _is_ correct for a `win32` interpreter) and neither flips validate's exit code. `bmad-loop diagnose` carries the same two facts in its Environment block as `sys.platform` and `win32 on WSL distro path` (`yes`/`no`). - Non-invasive: drives the upstream dev primitive unmodified — there is no fork to keep in sync — and review is just a re-invocation of it on the `done` spec. Your standard BMAD install is never modified. diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index ab0a36dd..9a64a80e 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -404,7 +404,7 @@ resolves to `claude`. | Field | Required | Default | Meaning | | ---------------------------------- | -------- | ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `name` | ✅ | — | Profile id, also the `--cli` value and override key. | -| `binary` | ✅ | — | Executable to launch (resolved on `PATH`). `bmad-loop validate` also probes it — a name that resolves but fails `--version` (typically a dead WSL/npm shim) is reported as `adapter.binary-unrunnable` at warning severity, #294. | +| `binary` | ✅ | — | Executable to launch (resolved on `PATH`). `bmad-loop validate` also probes it — a name that resolves but fails `--version` (typically a dead WSL/npm shim) is reported as `adapter.binary-unrunnable` at warning severity, #294. Only a **bare name** is probed, i.e. one `shutil.which` resolves by searching `PATH`; a `binary` carrying a path separator (`./tool`, an absolute path) is resolved and reported found but never launched, because profile fields arrive with a clone and validate must not execute repository-supplied code. | | `[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. 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. | diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index aa87207f..4d61acca 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -437,6 +437,20 @@ def cmd_validate(args: argparse.Namespace) -> int: # `bmad-loop validate`, which then told them everything was fine. Probe the # path `which` RETURNED rather than the bare name: re-resolving is a TOCTOU, # and on Windows the PATHEXT shim `which` picked is the very file at issue. + # Probe ONLY a bare name, i.e. one `shutil.which` resolved by SEARCHING + # PATH. `binary` is project-controlled (policy.toml picks the profile, + # `.bmad-loop/profiles/*.toml` supplies its fields), so `binary = "./tool"` + # would have this diagnostic EXECUTE a file carried by a cloned repo — + # `which` returns a caller-supplied path unchanged, and validate is exactly + # the command a user runs to decide whether a checkout is safe to run at + # all (the TUI runs it too). Inspection must not become execution there. + # The predicate is `os.path.dirname` because that is the same test + # `shutil.which` itself uses to choose direct-path over PATH search, so the + # two cannot drift. A path-bearing `binary` keeps the pre-#294 behavior: + # reported found, never launched. No packaged profile is affected — all six + # ship a bare name — so #294's dead WSL/npm shim is still caught. + if os.path.dirname(tool): + continue rc = probe_mod.binary_runs(resolved) if rc == 0: continue @@ -453,8 +467,10 @@ def cmd_validate(args: argparse.Namespace) -> int: report.warn( "adapter.binary-unrunnable", f"{tool} is on PATH at {resolved} but `{tool} --version` {outcome} — " - "that is a stale or broken install (a dead WSL/npm shim is the usual " - "cause), and runs using it will fail to start; reinstall it or fix PATH", + "the usual cause is a stale or broken install (a dead WSL/npm shim), " + "and runs using it would then fail to start; a CLI that does not " + "implement `--version` also lands here. Reinstall it or fix PATH, or " + "ignore this if that CLI has no `--version`.", {"binary": tool, "path": resolved, "returncode": rc}, ) diff --git a/tests/test_cli.py b/tests/test_cli.py index 4add03ab..0045dd14 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -5482,6 +5482,68 @@ def test_validate_warns_when_a_binary_on_path_refuses_to_run( assert len(found) == 1 and found[0]["severity"] == "ok" +def test_validate_never_executes_a_binary_the_project_supplied_a_path_for( + project, capsys, monkeypatch, tmp_path +): + """#294's liveness probe must not turn validate into a code-execution boundary. + + `binary` is project-controlled all the way down: policy.toml picks the profile + and `.bmad-loop/profiles/*.toml` supplies its fields, both of which arrive with + a clone. `shutil.which` returns a caller-supplied path UNCHANGED rather than + searching PATH, so `binary = "./pwn"` resolves to a file the repository itself + carries — and validate is precisely the command a user runs to decide whether a + checkout is safe (the TUI runs it too). Probing it would execute untrusted code + on the strength of reading a config file. + + Driven with a REAL executable on disk and a REAL relative `binary`, from the + project directory as a user who just cloned it would be: a stubbed `which` or + `binary_runs` would assert nothing about the boundary this is here to hold. + + The sentinel is the whole assertion — an absent `adapter.binary-unrunnable` + finding would also be produced by a probe that ran and returned 0, so it cannot + distinguish "not launched" from "launched and happy". Only a file that the + child alone can create does. + + Ablation target: delete the `if os.path.dirname(tool): continue` gate in cli.py + and the sentinel IS created, reddening this. Note the `adapter.binary` assertion + below is NOT the gate — it pins the surviving half, that a path-bearing binary + is still reported found; it stays green under that ablation. + """ + install_bmad_config(project) + _write_policy(project.project, '[adapter]\nname = "pwncli"\n') + + sentinel = tmp_path / "pwned.txt" + launcher = write_script_launcher( + project.project, + "pwn", + f"import pathlib\npathlib.Path({str(sentinel)!r}).write_text('executed')\n", + ) + profiles = project.project / ".bmad-loop" / "profiles" + profiles.mkdir(parents=True, exist_ok=True) + # `launcher.name`, not a bare "pwn": on Windows the launcher is `pwn.cmd`, and + # naming the real file keeps this about the path gate rather than about PATHEXT. + (profiles / "pwncli.toml").write_text( + f'name = "pwncli"\nbinary = "./{launcher.name}"\nbypass_args = ["--yes"]\n' + "\n[hooks]\n" + 'dialect = "claude-settings-json"\n' + 'config_path = ".pwncli/settings.json"\n' + 'events = { SessionStart = "SessionStart", Stop = "Stop" }\n', + encoding="utf-8", + ) + + # A relative `binary` resolves against the PROCESS cwd — the clone the user is + # standing in when they run `bmad-loop validate`. + monkeypatch.chdir(project.project) + cli.main(["validate", "--project", str(project.project), "--json"]) + doc = json.loads(capsys.readouterr().out) + + assert not sentinel.exists(), "validate executed a repository-supplied binary" + + # The surviving half: still resolved and reported, exactly as before #294. + found = [f for f in doc["findings"] if f["check"] == "adapter.binary"] + assert len(found) == 1 and found[0]["severity"] == "ok" + + @pytest.mark.parametrize("passing", [True, False], ids=["rc-0", "rc-1"]) def test_tui_renderer_draws_every_detail_shape_a_real_validate_emits( project, capsys, monkeypatch, passing From 5f8c3305701ed28c81e4c62f58287050420c0242 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 18 Aug 2026 17:30:04 -0700 Subject: [PATCH 6/8] docs: say what the unrunnable-binary probe does to validate's verdict (#294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The FEATURES.md preflight sentence said a binary that fails `--version` is "reported at warning severity rather than passing", which reads as though the `adapter.binary` gate itself stops passing. It does not: `adapter.binary` keeps its found/absent meaning and still reports ok, the probe adds a separate `adapter.binary-unrunnable` finding, and warning severity leaves validate's exit code alone — which is the whole reason the severity is `warning`. Reported by CodeRabbit on 3c0be90. --- docs/FEATURES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 9b6ba71b..46616295 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -199,7 +199,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Setup & install - `bmad-loop init` installs the three `bmad-loop-*` skills (`bmad-loop-setup`, `bmad-loop-resolve`, `bmad-loop-sweep`, into `.claude/skills/` and/or `.agents/skills/`), the hook relay, `.bmad-loop/policy.toml`, and a gitignore covering the runs dir, plugin caches, and policy.toml itself (per-machine config). Flags: `--cli` (repeatable), `--no-skills`, `--force-skills`. -- `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary (**probed, not just resolved**: a name that is on `PATH` but fails `--version`, typically a dead WSL/npm shim, is reported at warning severity rather than passing — [#294](https://github.com/bmad-code-org/bmad-loop/issues/294); only bare names are probed, since a profile-supplied path would make this diagnostic execute code a clone carries), hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. +- `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary (**probed, not just resolved**: a name that is on `PATH` but fails `--version`, typically a dead WSL/npm shim, adds an `adapter.binary-unrunnable` finding at warning severity — `adapter.binary` itself still reports ok, and validate's exit code is unchanged — [#294](https://github.com/bmad-code-org/bmad-loop/issues/294); only bare names are probed, since a profile-supplied path would make this diagnostic execute code a clone carries), hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. - The preflight also **names the multiplexer selection reason wherever selection resolves** (`mux.selection`, e.g. `platform default for win32`), not only when a `BMAD_LOOP_MUX_BACKEND`/`[mux] backend` choice forced it. A `fallback` selection is reported as a warning (its own label says no available backend matches this platform); a selection that outright failed is carried by `mux.preflight`, and a detection that failed by `mux.backends-detected` at warning — so a missing `mux.selection` line is normally explained by another finding (the historical unregistered-tmux fallback is the one silent exception; see the `--json` contract note in `documents.py`). On top of that, `host.win32-on-wsl-path` warns when a **native-Windows interpreter is working on a `\\wsl.localhost\...` project** ([#332](https://github.com/bmad-code-org/bmad-loop/issues/332) — see [multiplexer-backends.md](multiplexer-backends.md) for why WSL can hand a bash prompt the Windows build). Both are diagnostics only: neither changes which backend is selected (psmux _is_ correct for a `win32` interpreter) and neither flips validate's exit code. `bmad-loop diagnose` carries the same two facts in its Environment block as `sys.platform` and `win32 on WSL distro path` (`yes`/`no`). - Non-invasive: drives the upstream dev primitive unmodified — there is no fork to keep in sync — and review is just a re-invocation of it on the `done` spec. Your standard BMAD install is never modified. From 25e5323f4d28c9d224f11046ce3d4d2eb49f74ce Mon Sep 17 00:00:00 2001 From: t Date: Tue, 18 Aug 2026 17:37:07 -0700 Subject: [PATCH 7/8] fix(validate): gate the liveness probe on profile provenance (#294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous guard tested the SPELLING of `binary` and rejected a path (`./tool`). That left the same hole reachable by a clean name: a project overlay setting `binary = "pwn"` resolves through `shutil.which` into the checkout whenever a checkout-local directory is on PATH, and `validate` ran it. Two shapes of one family, which is the evidence that a predicate over the string cannot close it — there is always another spelling. Gate on who WROTE the profile instead. `CLIProfile` now carries `packaged`, stamped only by `load_profiles` at the single site that knows a file came out of `bmad_loop/data/profiles/` rather than off a project's disk. No TOML key sets it, so an overlay declaring `packaged = true` is read as an unknown key and not as a promotion, and the default is False so the untrusted answer is the one you get by forgetting. An entry-point profile is not packaged either: "installed alongside us" is a different claim from "shipped by us". `validate` probes only a binary a packaged profile named. #294's own case is packaged (opencode), so the dead WSL/npm shim is still caught; every other profile keeps the pre-#294 behavior of resolved, reported found, never launched. Provenance says nothing about the spelling of a packaged `binary`, and nothing in cli.py now does. That half is pinned where it is actually true — `test_every_packaged_profile_names_a_bare_binary` reddens at the file that would introduce a path-bearing bundled profile. The refusal test carries both shapes as rows, and the two ablation axes redden disjoint sets, which is what shows each guard does distinct work: dropping the gate reddens BOTH rows, while restoring the old `os.path.dirname` spelling gate reddens only `bare-name-on-path`. Reported by codex review on e3d026d (P1, round 2); the same remedy was named by both bots on 3c0be90. --- CHANGELOG.md | 12 +++-- docs/FEATURES.md | 2 +- docs/adapter-authoring-guide.md | 2 +- src/bmad_loop/adapters/profile.py | 24 +++++++++- src/bmad_loop/cli.py | 30 +++++++----- tests/test_cli.py | 79 +++++++++++++++++++++++-------- 6 files changed, 107 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9bf11e7d..172b4c51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,12 +22,14 @@ breaking changes may land in a minor release. `adapter.binary` gate asked `shutil.which`, which a dead WSL/npm shim satisfies — it is a real file with the execute bit — so validate went green on an install that could not start a session, and the opencode adapter's own "binary not found" error sent the user to `bmad-loop validate` to - be told everything was fine. Each binary named as a **bare name** is now run once as + be told everything was fine. Each binary named by a **packaged** profile is now run once as ` --version`; a nonzero exit or a launch fault reports the new check id - `adapter.binary-unrunnable`, carrying the resolved path and the return code. A `binary` carrying - a path separator is resolved and reported found but never launched: profile fields are - project-supplied, and validate is the command used to decide whether a checkout is safe to run, - so it must not execute code a clone carries. The severity is `warning`, so validate's exit code is + `adapter.binary-unrunnable`, carrying the resolved path and the return code. A project overlay's + profile is resolved and reported found but never launched: its fields are project-supplied, and + validate is the command used to decide whether a checkout is safe to run at all, so it must not + execute code a clone carries. That boundary is the profile's provenance and not the spelling of + `binary`, because a bare name still resolves into the checkout whenever a checkout-local + directory is on `PATH`. The severity is `warning`, so validate's exit code is unchanged for a live CLI that merely answers `--version` oddly, and `adapter.binary` keeps its existing found/absent meaning. The check id is additive, so `VALIDATE_SCHEMA_VERSION` is unchanged. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 46616295..ee65b32c 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -199,7 +199,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Setup & install - `bmad-loop init` installs the three `bmad-loop-*` skills (`bmad-loop-setup`, `bmad-loop-resolve`, `bmad-loop-sweep`, into `.claude/skills/` and/or `.agents/skills/`), the hook relay, `.bmad-loop/policy.toml`, and a gitignore covering the runs dir, plugin caches, and policy.toml itself (per-machine config). Flags: `--cli` (repeatable), `--no-skills`, `--force-skills`. -- `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary (**probed, not just resolved**: a name that is on `PATH` but fails `--version`, typically a dead WSL/npm shim, adds an `adapter.binary-unrunnable` finding at warning severity — `adapter.binary` itself still reports ok, and validate's exit code is unchanged — [#294](https://github.com/bmad-code-org/bmad-loop/issues/294); only bare names are probed, since a profile-supplied path would make this diagnostic execute code a clone carries), hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. +- `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary (**probed, not just resolved**: a name that is on `PATH` but fails `--version`, typically a dead WSL/npm shim, adds an `adapter.binary-unrunnable` finding at warning severity — `adapter.binary` itself still reports ok, and validate's exit code is unchanged — [#294](https://github.com/bmad-code-org/bmad-loop/issues/294); only **packaged** profiles are probed — a project overlay's binary is resolved but never launched, since this diagnostic must not execute code a clone carries), hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. - The preflight also **names the multiplexer selection reason wherever selection resolves** (`mux.selection`, e.g. `platform default for win32`), not only when a `BMAD_LOOP_MUX_BACKEND`/`[mux] backend` choice forced it. A `fallback` selection is reported as a warning (its own label says no available backend matches this platform); a selection that outright failed is carried by `mux.preflight`, and a detection that failed by `mux.backends-detected` at warning — so a missing `mux.selection` line is normally explained by another finding (the historical unregistered-tmux fallback is the one silent exception; see the `--json` contract note in `documents.py`). On top of that, `host.win32-on-wsl-path` warns when a **native-Windows interpreter is working on a `\\wsl.localhost\...` project** ([#332](https://github.com/bmad-code-org/bmad-loop/issues/332) — see [multiplexer-backends.md](multiplexer-backends.md) for why WSL can hand a bash prompt the Windows build). Both are diagnostics only: neither changes which backend is selected (psmux _is_ correct for a `win32` interpreter) and neither flips validate's exit code. `bmad-loop diagnose` carries the same two facts in its Environment block as `sys.platform` and `win32 on WSL distro path` (`yes`/`no`). - Non-invasive: drives the upstream dev primitive unmodified — there is no fork to keep in sync — and review is just a re-invocation of it on the `done` spec. Your standard BMAD install is never modified. diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 9a64a80e..9393ac38 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -404,7 +404,7 @@ resolves to `claude`. | Field | Required | Default | Meaning | | ---------------------------------- | -------- | ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `name` | ✅ | — | Profile id, also the `--cli` value and override key. | -| `binary` | ✅ | — | Executable to launch (resolved on `PATH`). `bmad-loop validate` also probes it — a name that resolves but fails `--version` (typically a dead WSL/npm shim) is reported as `adapter.binary-unrunnable` at warning severity, #294. Only a **bare name** is probed, i.e. one `shutil.which` resolves by searching `PATH`; a `binary` carrying a path separator (`./tool`, an absolute path) is resolved and reported found but never launched, because profile fields arrive with a clone and validate must not execute repository-supplied code. | +| `binary` | ✅ | — | Executable to launch (resolved on `PATH`). `bmad-loop validate` also probes it — a name that resolves but fails `--version` (typically a dead WSL/npm shim) is reported as `adapter.binary-unrunnable` at warning severity, #294. Only a **packaged** profile's binary is probed. A project overlay (or an entry-point package's) profile is resolved and reported found but never launched, whatever its `binary` is called — profile fields arrive with a clone, and validate must not execute repository-supplied code. The boundary is provenance rather than the spelling of `binary`, because a bare name still resolves into the checkout whenever a checkout-local directory is 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. 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. | diff --git a/src/bmad_loop/adapters/profile.py b/src/bmad_loop/adapters/profile.py index e7859db8..c6d559d4 100644 --- a/src/bmad_loop/adapters/profile.py +++ b/src/bmad_loop/adapters/profile.py @@ -34,7 +34,7 @@ import importlib.metadata import tomllib -from dataclasses import dataclass, field +from dataclasses import dataclass, field, replace from importlib import resources from pathlib import Path @@ -164,6 +164,24 @@ class CLIProfile: # across every tracked file. Override/extend via a project profile in # .bmad-loop/profiles/. env_fault_patterns: tuple[str, ...] = () + # Did this profile ship INSIDE the package (bmad_loop/data/profiles/*.toml)? + # Provenance, not configuration: it is stamped by `load_profiles` at the one + # place that knows which directory a file came from, and no TOML key sets it — + # a project overlay declaring `packaged = true` is read as an unknown key, not + # as a promotion. Default False, so the untrusted answer is the one you get by + # forgetting: an entry-point profile constructing this dataclass directly is + # not packaged either, because "installed alongside us" is not the same claim + # as "shipped by us". + # + # The one consumer is `validate`'s liveness probe (#294), which EXECUTES the + # binary. `binary` is project-controlled end to end — policy.toml picks the + # profile and .bmad-loop/profiles/*.toml supplies its fields, both arriving + # with a clone — so the probe needs a trust boundary that no spelling of + # `binary` can talk its way through: gating on the string (rejecting "./tool") + # still runs a bare "pwn" whenever a checkout-local directory is on PATH. + # Provenance is that boundary; it answers "who wrote this", which is the + # question actually being asked. + packaged: bool = False @property def hookless(self) -> bool: @@ -560,7 +578,9 @@ def load_profiles(project: Path | None = None) -> dict[str, CLIProfile]: 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 + # Stamped HERE and nowhere else: this loop is the only code that knows + # a profile came out of the package rather than off a project's disk. + profiles[profile.name] = replace(profile, packaged=True) profiles.update(_load_external_profiles()) if project is not None: user_dir = project / USER_PROFILES_REL diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 4d61acca..e72d1321 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -423,6 +423,7 @@ def cmd_validate(args: argparse.Namespace) -> int: from . import probe as probe_mod + packaged_binaries = {p.binary for p in profiles if p.packaged} for tool in dict.fromkeys(p.binary for p in profiles): resolved = shutil.which(tool) if resolved: @@ -437,19 +438,22 @@ def cmd_validate(args: argparse.Namespace) -> int: # `bmad-loop validate`, which then told them everything was fine. Probe the # path `which` RETURNED rather than the bare name: re-resolving is a TOCTOU, # and on Windows the PATHEXT shim `which` picked is the very file at issue. - # Probe ONLY a bare name, i.e. one `shutil.which` resolved by SEARCHING - # PATH. `binary` is project-controlled (policy.toml picks the profile, - # `.bmad-loop/profiles/*.toml` supplies its fields), so `binary = "./tool"` - # would have this diagnostic EXECUTE a file carried by a cloned repo — - # `which` returns a caller-supplied path unchanged, and validate is exactly - # the command a user runs to decide whether a checkout is safe to run at - # all (the TUI runs it too). Inspection must not become execution there. - # The predicate is `os.path.dirname` because that is the same test - # `shutil.which` itself uses to choose direct-path over PATH search, so the - # two cannot drift. A path-bearing `binary` keeps the pre-#294 behavior: - # reported found, never launched. No packaged profile is affected — all six - # ship a bare name — so #294's dead WSL/npm shim is still caught. - if os.path.dirname(tool): + # Probe ONLY a binary a PACKAGED profile named. `binary` is + # project-controlled end to end — policy.toml picks the profile and + # `.bmad-loop/profiles/*.toml` supplies its fields, both arriving with a + # clone — and this line EXECUTES it, inside the one command a user runs to + # decide whether a checkout is safe to run at all (the TUI runs it too). + # Inspection must not become execution there. + # + # The boundary is provenance because no test on the SPELLING of `binary` + # can hold: rejecting a path (`./tool`) still leaves a bare `pwn`, which + # `which` resolves to a repository file whenever a checkout-local + # directory is on PATH. "Who wrote this profile" is the question actually + # being asked, and it has a categorical answer. An overlay or entry-point + # profile keeps the pre-#294 behavior: resolved, reported found, never + # launched. #294's own case is a packaged profile (opencode), so the dead + # WSL/npm shim is still caught. + if tool not in packaged_binaries: continue rc = probe_mod.binary_runs(resolved) if rc == 0: diff --git a/tests/test_cli.py b/tests/test_cli.py index 0045dd14..ee5bbe78 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -5482,32 +5482,45 @@ def test_validate_warns_when_a_binary_on_path_refuses_to_run( assert len(found) == 1 and found[0]["severity"] == "ok" -def test_validate_never_executes_a_binary_the_project_supplied_a_path_for( - project, capsys, monkeypatch, tmp_path +@pytest.mark.parametrize("shape", ["relative-path", "bare-name-on-path"]) +def test_validate_never_executes_a_binary_an_overlay_profile_named( + project, capsys, monkeypatch, tmp_path, shape ): """#294's liveness probe must not turn validate into a code-execution boundary. `binary` is project-controlled all the way down: policy.toml picks the profile and `.bmad-loop/profiles/*.toml` supplies its fields, both of which arrive with - a clone. `shutil.which` returns a caller-supplied path UNCHANGED rather than - searching PATH, so `binary = "./pwn"` resolves to a file the repository itself - carries — and validate is precisely the command a user runs to decide whether a - checkout is safe (the TUI runs it too). Probing it would execute untrusted code - on the strength of reading a config file. - - Driven with a REAL executable on disk and a REAL relative `binary`, from the - project directory as a user who just cloned it would be: a stubbed `which` or + a clone. validate is precisely the command a user runs to decide whether a + checkout is safe to run at all (the TUI runs it too), so probing a binary an + overlay named would execute untrusted code on the strength of reading config. + + The two shapes are the same family reached by different spellings, which is why + they are parametrized rather than written as one row: + + - `relative-path` — `shutil.which` returns a caller-supplied path UNCHANGED + instead of searching PATH, so `./pwn` names a file the repository carries. + - `bare-name-on-path` — the spelling is clean, and `which` still resolves it + into the checkout because a checkout-local directory is on PATH. A guard that + tested the spelling of `binary` (rejecting a separator) passed this row. + + Only provenance covers both: an overlay profile is never probed whatever it is + called. `test_every_packaged_profile_names_a_bare_binary` holds the other half. + + Driven with a REAL executable and a REAL overlay profile, from the project + directory as a user who just cloned it would be: a stubbed `which` or `binary_runs` would assert nothing about the boundary this is here to hold. The sentinel is the whole assertion — an absent `adapter.binary-unrunnable` finding would also be produced by a probe that ran and returned 0, so it cannot - distinguish "not launched" from "launched and happy". Only a file that the - child alone can create does. - - Ablation target: delete the `if os.path.dirname(tool): continue` gate in cli.py - and the sentinel IS created, reddening this. Note the `adapter.binary` assertion - below is NOT the gate — it pins the surviving half, that a path-bearing binary - is still reported found; it stays green under that ablation. + distinguish "not launched" from "launched and happy". Only a file the child + alone can create does. + + Ablation target: drop the `if tool not in packaged_binaries` gate in cli.py and + BOTH rows create the sentinel. Restore it but gate on `os.path.dirname(tool)` + instead and `bare-name-on-path` alone reddens — that is the round-1 fix this + row exists to keep dead. The `adapter.binary` assertion below is NOT the gate: + it pins the surviving half (a rejected binary is still reported found) and + stays green under either ablation. """ install_bmad_config(project) _write_policy(project.project, '[adapter]\nname = "pwncli"\n') @@ -5518,12 +5531,18 @@ def test_validate_never_executes_a_binary_the_project_supplied_a_path_for( "pwn", f"import pathlib\npathlib.Path({str(sentinel)!r}).write_text('executed')\n", ) + if shape == "relative-path": + # `launcher.name`, not a bare "pwn": on Windows the launcher is `pwn.cmd`, + # and naming the real file keeps this about the gate, not about PATHEXT. + binary = f"./{launcher.name}" + else: + binary = "pwn" # resolved through PATH, into the checkout + monkeypatch.setenv("PATH", str(project.project) + os.pathsep + os.environ.get("PATH", "")) + profiles = project.project / ".bmad-loop" / "profiles" profiles.mkdir(parents=True, exist_ok=True) - # `launcher.name`, not a bare "pwn": on Windows the launcher is `pwn.cmd`, and - # naming the real file keeps this about the path gate rather than about PATHEXT. (profiles / "pwncli.toml").write_text( - f'name = "pwncli"\nbinary = "./{launcher.name}"\nbypass_args = ["--yes"]\n' + f'name = "pwncli"\nbinary = "{binary}"\nbypass_args = ["--yes"]\n' "\n[hooks]\n" 'dialect = "claude-settings-json"\n' 'config_path = ".pwncli/settings.json"\n' @@ -5544,6 +5563,26 @@ def test_validate_never_executes_a_binary_the_project_supplied_a_path_for( assert len(found) == 1 and found[0]["severity"] == "ok" +def test_every_packaged_profile_names_a_bare_binary(): + """The packaged half of the probe's trust boundary (#294). + + `validate` executes the binary of any profile stamped `packaged`, and it trusts + provenance rather than spelling — so nothing in cli.py stops a packaged profile + whose `binary` carried a path from being launched out of the working directory. + Nothing needs to, as long as no packaged profile ships one, and that is a + property of the shipped TOML rather than of the code: pin it where it is true. + + A future bundled profile that sets `binary = "./vendor/cli"` reddens here, at + the file that introduced it, rather than silently widening what a diagnostic + executes. + """ + from bmad_loop.adapters.profile import load_profiles + + packaged = {n: p for n, p in load_profiles(project=None).items() if p.packaged} + assert packaged, "no packaged profiles loaded — the gate would be vacuous" + assert [p.binary for p in packaged.values() if os.path.dirname(p.binary)] == [] + + @pytest.mark.parametrize("passing", [True, False], ids=["rc-0", "rc-1"]) def test_tui_renderer_draws_every_detail_shape_a_real_validate_emits( project, capsys, monkeypatch, passing From 402d1edbb6e9575473e9f3f8f81ee0d9c3d29a6f Mon Sep 17 00:00:00 2001 From: t Date: Tue, 18 Aug 2026 18:26:49 -0700 Subject: [PATCH 8/8] docs: bound what the probe's provenance gate actually promises (#294) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate was described as stopping validate from "executing code a clone carries", which promises more than it delivers and is the accurate half of codex's third report. Provenance decides WHICH NAME is probed; it says nothing about what that name resolves to. `shutil.which` consults the user's PATH, so a PATH carrying a checkout-local directory can answer `claude` with a file the clone ships, and no packaged/overlay distinction changes that. That residual is left standing rather than closed, and the comment now says why. In this shape the repository does not choose the name — it is ours — and the resolution is the one the session launch already performs: the generic adapter puts the bare `binary` at argv[0], and the opencode adapter calls the identical `shutil.which` before spawning. A PATH that redefines `claude` has redefined it for the run and for the user's own shell; the probe grants nothing further. The alternative guard, refusing checkout-local resolutions, rests on realpath containment, which leaks through symlinks, `..`, win32 case-folding, UNC paths, and worktree-root vs project-root. Docs, CHANGELOG and the code comment now state the bound rather than the absolute claim. --- CHANGELOG.md | 6 ++++-- docs/FEATURES.md | 2 +- docs/adapter-authoring-guide.md | 2 +- src/bmad_loop/cli.py | 15 ++++++++++++++- 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 172b4c51..ecd334eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,8 +26,10 @@ breaking changes may land in a minor release. ` --version`; a nonzero exit or a launch fault reports the new check id `adapter.binary-unrunnable`, carrying the resolved path and the return code. A project overlay's profile is resolved and reported found but never launched: its fields are project-supplied, and - validate is the command used to decide whether a checkout is safe to run at all, so it must not - execute code a clone carries. That boundary is the profile's provenance and not the spelling of + validate is the command used to decide whether a checkout is safe to run at all, so a clone's own + config cannot choose which binary it launches. The gate bounds which NAME is probed, not what + that name resolves to — resolution runs through the user's `PATH`, and a probed name resolves to + whatever the session launch would itself run. That boundary is the profile's provenance and not the spelling of `binary`, because a bare name still resolves into the checkout whenever a checkout-local directory is on `PATH`. The severity is `warning`, so validate's exit code is unchanged for a live CLI that merely answers `--version` oddly, and `adapter.binary` keeps its diff --git a/docs/FEATURES.md b/docs/FEATURES.md index ee65b32c..1694cf82 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -199,7 +199,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Setup & install - `bmad-loop init` installs the three `bmad-loop-*` skills (`bmad-loop-setup`, `bmad-loop-resolve`, `bmad-loop-sweep`, into `.claude/skills/` and/or `.agents/skills/`), the hook relay, `.bmad-loop/policy.toml`, and a gitignore covering the runs dir, plugin caches, and policy.toml itself (per-machine config). Flags: `--cli` (repeatable), `--no-skills`, `--force-skills`. -- `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary (**probed, not just resolved**: a name that is on `PATH` but fails `--version`, typically a dead WSL/npm shim, adds an `adapter.binary-unrunnable` finding at warning severity — `adapter.binary` itself still reports ok, and validate's exit code is unchanged — [#294](https://github.com/bmad-code-org/bmad-loop/issues/294); only **packaged** profiles are probed — a project overlay's binary is resolved but never launched, since this diagnostic must not execute code a clone carries), hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. +- `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary (**probed, not just resolved**: a name that is on `PATH` but fails `--version`, typically a dead WSL/npm shim, adds an `adapter.binary-unrunnable` finding at warning severity — `adapter.binary` itself still reports ok, and validate's exit code is unchanged — [#294](https://github.com/bmad-code-org/bmad-loop/issues/294); only **packaged** profiles are probed — a project overlay's binary is resolved but never launched, so a clone cannot choose which binary this diagnostic launches; resolution still goes through your `PATH`, so what a probed name resolves to is whatever the session launch would itself run), hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. - The preflight also **names the multiplexer selection reason wherever selection resolves** (`mux.selection`, e.g. `platform default for win32`), not only when a `BMAD_LOOP_MUX_BACKEND`/`[mux] backend` choice forced it. A `fallback` selection is reported as a warning (its own label says no available backend matches this platform); a selection that outright failed is carried by `mux.preflight`, and a detection that failed by `mux.backends-detected` at warning — so a missing `mux.selection` line is normally explained by another finding (the historical unregistered-tmux fallback is the one silent exception; see the `--json` contract note in `documents.py`). On top of that, `host.win32-on-wsl-path` warns when a **native-Windows interpreter is working on a `\\wsl.localhost\...` project** ([#332](https://github.com/bmad-code-org/bmad-loop/issues/332) — see [multiplexer-backends.md](multiplexer-backends.md) for why WSL can hand a bash prompt the Windows build). Both are diagnostics only: neither changes which backend is selected (psmux _is_ correct for a `win32` interpreter) and neither flips validate's exit code. `bmad-loop diagnose` carries the same two facts in its Environment block as `sys.platform` and `win32 on WSL distro path` (`yes`/`no`). - Non-invasive: drives the upstream dev primitive unmodified — there is no fork to keep in sync — and review is just a re-invocation of it on the `done` spec. Your standard BMAD install is never modified. diff --git a/docs/adapter-authoring-guide.md b/docs/adapter-authoring-guide.md index 9393ac38..7b8d4755 100644 --- a/docs/adapter-authoring-guide.md +++ b/docs/adapter-authoring-guide.md @@ -404,7 +404,7 @@ resolves to `claude`. | Field | Required | Default | Meaning | | ---------------------------------- | -------- | ------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `name` | ✅ | — | Profile id, also the `--cli` value and override key. | -| `binary` | ✅ | — | Executable to launch (resolved on `PATH`). `bmad-loop validate` also probes it — a name that resolves but fails `--version` (typically a dead WSL/npm shim) is reported as `adapter.binary-unrunnable` at warning severity, #294. Only a **packaged** profile's binary is probed. A project overlay (or an entry-point package's) profile is resolved and reported found but never launched, whatever its `binary` is called — profile fields arrive with a clone, and validate must not execute repository-supplied code. The boundary is provenance rather than the spelling of `binary`, because a bare name still resolves into the checkout whenever a checkout-local directory is on `PATH`. | +| `binary` | ✅ | — | Executable to launch (resolved on `PATH`). `bmad-loop validate` also probes it — a name that resolves but fails `--version` (typically a dead WSL/npm shim) is reported as `adapter.binary-unrunnable` at warning severity, #294. Only a **packaged** profile's binary is probed. A project overlay (or an entry-point package's) profile is resolved and reported found but never launched, whatever its `binary` is called — profile fields arrive with a clone, so a project's own config cannot choose which binary this diagnostic launches. The boundary is provenance rather than the spelling of `binary`, because a bare name still resolves into the checkout whenever a checkout-local directory is on `PATH`. Note what it bounds: **which name** is probed, not what that name resolves to. Resolution runs through your `PATH`, so validate launches whatever `PATH` says the CLI is — the same file the session launch itself would run. | | `[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. 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. | diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index e72d1321..18b27cd9 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -443,7 +443,6 @@ def cmd_validate(args: argparse.Namespace) -> int: # `.bmad-loop/profiles/*.toml` supplies its fields, both arriving with a # clone — and this line EXECUTES it, inside the one command a user runs to # decide whether a checkout is safe to run at all (the TUI runs it too). - # Inspection must not become execution there. # # The boundary is provenance because no test on the SPELLING of `binary` # can hold: rejecting a path (`./tool`) still leaves a bare `pwn`, which @@ -453,6 +452,20 @@ def cmd_validate(args: argparse.Namespace) -> int: # profile keeps the pre-#294 behavior: resolved, reported found, never # launched. #294's own case is a packaged profile (opencode), so the dead # WSL/npm shim is still caught. + # + # What this bounds is WHICH NAME is probed, never what that name resolves + # to. Resolution is `shutil.which` against the user's PATH, so a PATH + # carrying a checkout-local directory can still answer `claude` with a + # file the clone ships. That residual is deliberate and is NOT a hole this + # gate is failing to close: the name is ours rather than the project's, and + # the same resolution is what the session launch itself performs — the + # generic adapter puts this bare `binary` at argv[0] (adapters/generic.py) + # and the opencode adapter calls the identical `shutil.which` before + # spawning (adapters/opencode_http.py). A PATH that redefines `claude` + # has already redefined it for the run, and for the user's own shell. + # Refusing checkout-local RESOLUTIONS would be a different guard, over a + # predicate (realpath containment) that leaks through symlinks, `..`, + # win32 case-folding, UNC paths, and worktree-root vs project-root. if tool not in packaged_binaries: continue rc = probe_mod.binary_runs(resolved)