diff --git a/CHANGELOG.md b/CHANGELOG.md index 15906aa..acf146b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,41 @@ breaking changes may land in a minor release. ### Fixed +- **`validate` requires the review skills your `bmad-dev-auto` actually invokes (#260).** The + preflight held every project to a fixed catalog that included `bmad-review-verification-gap`, + which no tagged BMAD-METHOD release ships (absent from v6.10.0; on current sources only a + forwarder to the merged `bmad-review`), and misdiagnosed it as "install the BMad Method (bmm) + module" on projects where bmm _was_ installed — so `validate`/`run`/`resume`/`sweep` could not + pass on a stock install. The required reviewers are now read from the installed skill itself: + its `customize.toml` `[[workflow.review_layers]]` when present (honoring disabled layers and + `_bmad/custom/bmad-dev-auto.toml` overrides), else the reviewers `step-04-review.md` names + inline. A merged-`bmad-review` install needs only `bmad-review`; a v6.10.0 install needs the + two hunters it names; and a tree whose configured layers reference a skill it does not have is + now caught by a new `skills.review-layer-missing` problem instead of passing and then failing + on every dev run. Unreadable configs fall back to the previous static requirement, and all + messages name the real remedy (install/update bmm, BMAD-METHOD >= 6.10.0). + + Derivation is held to what the run really resolves: + - Override merging matches BMAD's own resolver (`resolve_customization.py`) exactly — arrays of + tables opt into keyed merge only when _every_ combined item carries the same identifier, and + `code` is checked before `id`; anything mixed or key-less appends. Keying on `id` alone + silently dropped base layers in one direction and kept superseded ones in the other. + - An override that is not valid TOML no longer abandons derivation: the resolver skips a broken + layer and carries on, so the preflight does too, reporting the file as a + `skills.customize-unreadable` warning. + - A `customize.toml` whose `workflow` key is valid TOML of the wrong _shape_ (a string, list or + number) no longer raises `AttributeError` out of `validate`/`run`/`resume`/`sweep`. + - Disabling every review layer is reported as `skills.review-layers-empty` rather than passing: + `step-04-review.md` HALTs blocked with `no active review layers`. + - Layers that cannot be resolved statically — gated by a `when` condition the model evaluates at + run time, or naming a skill in a phrasing this check cannot confirm is a handoff — are reported + as `skills.review-layer-unresolved` **warnings** and never block. `validate` and the run + preflight now branch on severity, so an advisory can no longer abort a run. + - Isolated worktrees get what was validated: `provision_worktree` copies the review skills the + project's own layers name (not just the fixed base catalog) and seeds `_bmad/custom/`, whose + `*.user.toml` layer the upstream installer gitignores — without it the preflight resolved one + layer set from the main checkout while the worktree run resolved another. + - **`notify.desktop` works on macOS/Windows, and warns when it can't (#231).** The desktop notification channel was `notify-send`-only, so on macOS and Windows `notify.desktop` (default `true`) was silently inert — every "a human is needed" path (escalations, deferrals, diff --git a/README.md b/README.md index 715d941..40e0e24 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ Inspired by the original [bmad-automator](https://github.com/bmad-code-org/bmad- - **Python 3.11+**, a **terminal multiplexer** (tmux is the bundled default), and a supported coding CLI — `claude` by default; `codex`, `gemini`, `copilot`, and `antigravity` (`agy`) via [profiles](#other-coding-clis). - **Linux or macOS** (or **Windows via WSL**, which _is_ Linux — it runs as-is). tmux is the bundled terminal-multiplexer backend (externals like the [herdr adapter](https://github.com/pbean/bmad-loop-adapter-herdr) co-install as packages and self-register — see [Terminal multiplexer backends](docs/multiplexer-backends.md)), and all of it sits behind a pluggable **registry** of OS seams (transport, process lifecycle, hook interpreter) with availability-aware selection — env var → persisted `[mux] backend` choice (`bmad-loop mux set `) → platform default (`psmux` on Windows, `tmux` elsewhere) → first available platform match — so a native-Windows backend slots in as new files + a registration line each, with no engine edits — see [Porting bmad-loop to a new OS](docs/porting-to-a-new-os.md). Native Windows is not yet shipped. -- A **BMAD v6 project** (`_bmad/bmm/config.yaml`, a `sprint-status.yaml` from `bmad-sprint-planning`) with the upstream `bmad-dev-auto` skill (and the three review-hunter skills its step-04 invokes inline: `bmad-review-adversarial-general`, `bmad-review-edge-case-hunter`, `bmad-review-verification-gap`) and the bmad-loop skill module from this repo installed (`bmad-loop-resolve`, `bmad-loop-sweep` — see [Installing the skill module](#installing-the-skill-module)). Standard BMAD skills stay untouched. +- A **BMAD v6 project** (`_bmad/bmm/config.yaml`, a `sprint-status.yaml` from `bmad-sprint-planning`) on **BMAD-METHOD ≥ 6.10.0**, with the upstream `bmad-dev-auto` skill (and the review-layer skills its step-04 invokes inline: `bmad-review-adversarial-general` + `bmad-review-edge-case-hunter`, or the merged `bmad-review` skill that supersedes them in newer releases) and the bmad-loop skill module from this repo installed (`bmad-loop-resolve`, `bmad-loop-sweep` — see [Installing the skill module](#installing-the-skill-module)). Standard BMAD skills stay untouched. ## Quick start @@ -243,19 +243,19 @@ Bundle dev sessions can themselves append new deferred entries (split-off goals, ## Installing the skill module -The orchestrator drives the upstream `bmad-dev-auto` skill as its inner dev primitive — unmodified, so there is no fork to keep in sync; it both implements and (re-invoked on the done spec) runs the follow-up review — plus its own bundled `bmad-loop-*` skills for escalation, sweep, and setup. Your standard BMAD install is never modified. The three bundled skills ship in the `bmad-loop` wheel (canonical source: `src/bmad_loop/data/skills/`, BMAD module code `bmad-loop`) so `bmad-loop init` lays them down for you; `bmad-dev-auto` and the three review-hunter skills its step-04 invokes inline are prerequisites installed by the BMad Method (bmm) module: +The orchestrator drives the upstream `bmad-dev-auto` skill as its inner dev primitive — unmodified, so there is no fork to keep in sync; it both implements and (re-invoked on the done spec) runs the follow-up review — plus its own bundled `bmad-loop-*` skills for escalation, sweep, and setup. Your standard BMAD install is never modified. The three bundled skills ship in the `bmad-loop` wheel (canonical source: `src/bmad_loop/data/skills/`, BMAD module code `bmad-loop`) so `bmad-loop init` lays them down for you; `bmad-dev-auto` and the review-layer skills its step-04 invokes inline are prerequisites installed by the BMad Method (bmm) module: -| Skill | Role | -| --------------------------------- | ----------------------------------------------------------------------------------------------- | -| `bmad-dev-auto` | unattended implementation + follow-up review (**upstream** — bmm prerequisite, not bundled) | -| `bmad-review-adversarial-general` | inline step-04 review layer (**upstream** — bmm prerequisite, not bundled) | -| `bmad-review-edge-case-hunter` | inline step-04 review layer (**upstream** — bmm prerequisite, not bundled) | -| `bmad-review-verification-gap` | inline step-04 review layer, newest (BMAD-METHOD#2550) (**upstream** — bmm prereq, not bundled) | -| `bmad-loop-resolve` | interactive CRITICAL-escalation resolution (`/bmad-loop-resolve `) | -| `bmad-loop-sweep` | deferred-work ledger triage (automation-only) | -| `bmad-loop-setup` | registers the module in `_bmad/` config + help | +| Skill | Role | +| --------------------------------- | ------------------------------------------------------------------------------------------- | +| `bmad-dev-auto` | unattended implementation + follow-up review (**upstream** — bmm prerequisite, not bundled) | +| `bmad-review-adversarial-general` | inline step-04 review layer (**upstream** — bmm prerequisite, not bundled) | +| `bmad-review-edge-case-hunter` | inline step-04 review layer (**upstream** — bmm prerequisite, not bundled) | +| `bmad-review` | merged lens-based reviewer, supersedes the hunters (**upstream** — bmm prereq, not bundled) | +| `bmad-loop-resolve` | interactive CRITICAL-escalation resolution (`/bmad-loop-resolve `) | +| `bmad-loop-sweep` | deferred-work ledger triage (automation-only) | +| `bmad-loop-setup` | registers the module in `_bmad/` config + help | -`bmad-loop validate` preflights all four upstream skills; a target project on a pre-July bmm install missing `bmad-review-verification-gap` (or a `bmad-dev-auto` without its `customize.toml` review-layer config) is reported with bmm-module remediation before any run starts. +`bmad-loop validate` preflights `bmad-dev-auto` (always) plus the review skills that copy of the skill will actually invoke — read from its `customize.toml` review layers, or from `step-04-review.md` on releases that name their reviewers inline. So a merged-`bmad-review` install needs only `bmad-review`, a v6.10.0 install needs the two hunters it names, and a tree whose configured layers reference a skill it does not have is reported instead of failing on every dev run. Missing skills (or a `bmad-dev-auto` without its `customize.toml`) are reported with bmm-module remediation before any run starts. **Via uv + `bmad-loop init` (self-sufficient).** Installing the tool and running `init` is all you need — `init` installs the `bmad-loop-*` skills into `.claude/skills/` (claude) and/or `.agents/skills/` (codex/gemini) for the CLIs you select, alongside the hooks and policy: diff --git a/docs/FEATURES.md b/docs/FEATURES.md index f73edd0..d66e4e8 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -175,7 +175,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 three inline review-hunter skills `bmad-dev-auto` invokes (`bmad-review-adversarial-general`, `bmad-review-edge-case-hunter`, `bmad-review-verification-gap`) 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, hook registration, and the review skills the installed `bmad-dev-auto` actually invokes — 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`. - Non-invasive: drives the upstream `bmad-dev-auto` skill 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. ### Command reference diff --git a/docs/setup-guide.md b/docs/setup-guide.md index 3046327..be40515 100644 --- a/docs/setup-guide.md +++ b/docs/setup-guide.md @@ -97,8 +97,59 @@ installs all the bundled skills together (`bmad-loop-resolve`, `bmad-loop-sweep` the orchestrator normalizes the ledger to. The dev primitive `bmad-dev-auto` is **not** bundled: it is the upstream skill the orchestrator drives (for both implementation and the follow-up review), installed by the BMad Method (bmm) -module. `bmad-loop validate` checks it — plus the three review hunters it -invokes inline — are present before a run starts. +module. `bmad-loop validate` checks it — plus the review layers it invokes +inline — are present before a run starts. + +**Minimum BMAD-METHOD: v6.10.0** for sprint mode. Beyond `bmad-dev-auto` itself (with +its `customize.toml`), `bmad-loop validate` holds you to no fixed list of review +skills: it reads the `bmad-dev-auto` you actually have installed and requires the +reviewers that copy will actually invoke. + +- **Layer-driven (post-6.10.0)** — `customize.toml` defines + `[[workflow.review_layers]]`, and each layer's `instruction` names the skill it hands + off to. Current sources define four layers: `blind-hunter`, `edge-case-hunter`, and + `verification-gap`, each invoking the merged `bmad-review` skill with one lens, plus + `intent-alignment`, a self-contained prompt that invokes no skill at all. Such a tree + needs `bmad-review` and none of the standalone hunters. An intermediate release whose + layers name the standalone hunters needs exactly those instead. +- **v6.10.0** — no `review_layers` section at all; `step-04-review.md` names + `bmad-review-adversarial-general` and `bmad-review-edge-case-hunter` inline, so those + two are what is required. + +A layer disabled with an empty `instruction`, or replaced in +`_bmad/custom/bmad-dev-auto.toml` by a recipe that runs something else, requires +nothing. If the shape cannot be read at all, `validate` falls back to requiring the two +v6.10.0 hunters — which a tree carrying `bmad-review` satisfies. + +`bmad-review-verification-gap` is no longer required unconditionally (#260): no tagged +release ships it as a standalone skill, and on current sources it is only a thin +forwarder to `bmad-review`. It is required exactly when your installed layers name it. + +### Customizing the review layers + +Your project overrides in `_bmad/custom/bmad-dev-auto.toml` (team) and +`_bmad/custom/bmad-dev-auto.user.toml` (personal, gitignored by the bmm installer) are +applied before the requirement is computed, using the same merge rules BMAD's own +resolver uses: an array of tables merges by key only when _every_ entry — default and +override alike — carries the same `code` or `id`; otherwise the override appends. So +adding a layer with a new `id` adds a requirement, and replacing one by `id` moves it. + +Two things `validate` reports but never blocks on, because neither can be decided +without running the review: + +- a layer gated by a `when` condition, which the model evaluates against the diff; +- a layer naming a skill in a phrasing that is not recognizably a handoff (the + convention is ``Invoke the `skill-name` skill``). + +Both come back as `skills.review-layer-unresolved` warnings, so a customized layer can +never block a run the way the old fixed catalog did. What _is_ a problem: every layer +disabled at once (`skills.review-layers-empty`), which makes `bmad-dev-auto` HALT +blocked with `no active review layers`. + +Under `isolation = "worktree"` the skills your layers name are copied into each unit's +worktree along with `_bmad/custom/`, so an isolated run resolves the same layers +`validate` checked — including from the gitignored `*.user.toml`, which a fresh checkout +would not carry. ## Choosing which CLIs to drive diff --git a/src/bmad_loop/checks.py b/src/bmad_loop/checks.py index 2ffc2fe..033a823 100644 --- a/src/bmad_loop/checks.py +++ b/src/bmad_loop/checks.py @@ -68,6 +68,10 @@ "skills.base", "skills.base-missing", "skills.base-incomplete", + "skills.review-layer-missing", + "skills.review-layer-unresolved", + "skills.review-layers-empty", + "skills.customize-unreadable", "skills.stories-dispatch", "skills.stories-dispatch-missing", "skills.stories-dispatch-stale", diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index e8a2ac6..120b97d 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -353,14 +353,17 @@ def cmd_validate(args: argparse.Namespace) -> int: {"role": role, "model": cfg.model, "profile": prof.name}, ) - base_problems = install.missing_base_skills(project, [p.skill_tree for p in profiles]) - if profiles and not base_problems: + base_findings = install.missing_base_skills(project, [p.skill_tree for p in profiles]) + # gated on PROBLEMS, not on any finding: an advisory review layer (a `when` + # gate, a phrasing this check can't confirm, a broken override) is a warning + # that must ride alongside the ok line rather than suppress it. + if profiles and not any(f.severity == "problem" for f in base_findings): report.ok( "skills.base", - "upstream skills present (bmad-dev-auto + review hunters)", + "upstream skills present (bmad-dev-auto + review layers)", {"trees": list(dict.fromkeys(p.skill_tree for p in profiles))}, ) - report.extend(base_problems) + report.extend(base_findings) if getattr(args, "json", False): # getattr, not args.json: cmd_validate is called directly by tests (and by @@ -480,11 +483,12 @@ def _mux_set(project: Path, args: argparse.Namespace) -> int: def _require_base_skills(project: Path, pol, *, require_stories: bool = False) -> bool: """Preflight the upstream skills the orchestrator drives (bmad-dev-auto + the - three review hunters it invokes inline). + review layers it invokes inline). Returns True when everything is in place; otherwise prints the problems and returns False so the caller can abort before spawning any session (a missing skill would otherwise stall as an `Unknown command` until the run times out). + Warnings are printed but never abort — only ``problem`` findings block. ``require_stories`` additionally content-probes bmad-dev-auto for folder+id dispatch — stories mode needs a newer skill than sprint mode, so an older @@ -498,9 +502,16 @@ def _require_base_skills(project: Path, pol, *, require_stories: bool = False) - skill_trees.append(get_profile(name, project).skill_tree) except ProfileError: continue - problems = install.missing_base_skills(project, skill_trees) + findings = install.missing_base_skills(project, skill_trees) if require_stories: - problems += install.missing_stories_support(project, skill_trees) + findings += install.missing_stories_support(project, skill_trees) + # Severity decides. A review layer we can't statically resolve (a `when` gate, + # an unrecognized handoff phrasing, an unparseable override) is reported and + # then stepped over: aborting on it would be the false FAIL of #260, only now + # on every run rather than only on validate. + problems = [f for f in findings if f.severity == "problem"] + for warning in (f for f in findings if f.severity != "problem"): + print(f"warning: {warning.message}", file=sys.stderr) if problems: for problem in problems: print(f"FAIL: {problem.message}", file=sys.stderr) diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index cfd25b0..d03f290 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -17,11 +17,14 @@ from __future__ import annotations import json +import re import shutil import subprocess +import tomllib from collections.abc import Iterable, Sequence from importlib import resources from pathlib import Path +from typing import Any, NamedTuple from .adapters.profile import ALIASES, CLIProfile, ProfileError, load_profiles from .checks import Finding @@ -75,29 +78,282 @@ # BMad Method (bmm) module installs them. Each must exist in every active CLI skill # tree and carry its marker files (a half-installed or pre-automation skill is # caught by the `bmad-loop validate` preflight). `{skill: (marker-rel-path, ...)}`. -# - bmad-dev-auto: the inner dev primitive — always required. Markers pin BOTH a -# step file (catches a truncated copy) AND customize.toml, the layer/handoff -# config step-04 resolves review_layers from (BMAD-METHOD #2535/#2550): a -# pre-July bmm install predating it would let every dev run's step-04 fail. -# - the three review hunters bmad-dev-auto's step-04 invokes inline on EVERY dev -# run (and on each follow-up review re-invocation) — also always required, no -# longer gated on a separate review session. bmad-review-verification-gap is -# the newest layer (BMAD-METHOD #2550): a target project missing it makes the -# verification-gap review layer fail on every run. +# - bmad-dev-auto: the inner dev primitive — always required, and never +# substitutable. Markers pin BOTH a step file (catches a truncated copy) AND +# customize.toml, the layer/handoff config step-04 resolves review_layers from +# (BMAD-METHOD #2535/#2550): a pre-July bmm install predating it would let +# every dev run's step-04 fail. +# - the two review hunters v6.10.0 ships. These are only the FALLBACK review +# requirement, used when the installed skill's shape can't be read: normally the +# reviewers are derived per tree from bmad-dev-auto itself +# (resolve_review_layers), because which skills the review step invokes is a +# property of that skill version, not of a catalog pinned in here (#260). +# bmad-review-verification-gap is not in the fallback set: no tagged BMAD-METHOD +# release ships it standalone (on current sources it is a thin forwarder to the +# merged bmad-review), so demanding it of every project made `validate` +# unsatisfiable on real installs. A project whose review layers DO name it still has +# it required — by derivation from its own config, not by this list. DEV_BASE_SKILLS = { "bmad-dev-auto": ("step-04-review.md", "customize.toml"), "bmad-review-adversarial-general": (), "bmad-review-edge-case-hunter": (), - "bmad-review-verification-gap": (), } -# Every non-bundled skill that might need copying into an isolated worktree. -# bmad-review is the merged lens-based reviewer (BMAD-METHOD core-streamline): -# on new bmm installs the three hunter IDs above are thin forwarders to it, so a -# worktree must carry the real skill for those forwards to resolve. It is NOT in -# DEV_BASE_SKILLS (preflight) so pre-merge bmm installs — which have the three -# real hunters and no bmad-review — keep validating; provision_worktree skips -# skills the main repo lacks, so copy-if-present is safe in both directions. -BASE_SKILLS = {**DEV_BASE_SKILLS, "bmad-review": ()} +# The merged lens-based reviewer (BMAD-METHOD core-streamline). Current sources make +# step-04 layer-driven off customize.toml's [[workflow.review_layers]]: four layers +# (blind-hunter, edge-case-hunter, verification-gap — each invoking bmad-review with +# one lens — plus intent-alignment, an inline prompt invoking no skill at all), so +# bmad-review alone satisfies every layer that needs a skill. Named here for the +# fallback path only; the derived path sees it named in the layers themselves. +MERGED_REVIEW_SKILL = "bmad-review" +# The DEV_BASE_SKILLS entries MERGED_REVIEW_SKILL subsumes. bmad-dev-auto (the dev +# primitive) is NOT here — the merged reviewer never substitutes for it. +_REVIEW_LAYER_SKILLS = frozenset( + {"bmad-review-adversarial-general", "bmad-review-edge-case-hunter"} +) +# Every non-bundled skill that might need copying into an isolated worktree: the +# preflight set above plus the merged reviewer and the pre-consolidation standalone +# verification-gap forwarder (carried so a hand-installed forwarder still resolves, +# never validated). provision_worktree skips skills the main repo lacks, so this +# copy-if-present superset is safe in both directions. +BASE_SKILLS = {**DEV_BASE_SKILLS, "bmad-review-verification-gap": (), MERGED_REVIEW_SKILL: ()} + +DEV_PRIMITIVE_SKILL = "bmad-dev-auto" +# How bmad-dev-auto names a skill it hands a review off to, in both shapes: +# "Invoke the `bmad-review` skill with only the `adversarial` lens" (a +# customize.toml review layer) and "Invoke the `bmad-review-edge-case-hunter` +# skill on this diff" (pre-consolidation step-04). Deliberately narrow — +# backticked, `bmad-` prefixed — because a false match here is a false FAIL, +# the exact failure mode #260 was. +_INVOKE_SKILL_RE = re.compile(r"[Ii]nvoke the `(bmad-[a-z0-9-]+)` skill") +# Any backticked bmad-* token in a layer's instruction. What this matches and +# _INVOKE_SKILL_RE does not is a skill reference we cannot confirm is an +# invocation — upstream itself writes "use the `bmad-code-review` skill" +# elsewhere, so the narrow phrasing is a convention, not a contract. Such a +# reference is reported as a WARNING and never hard-required: an override may +# legitimately mention a skill it does not invoke, and blocking on that would +# rebuild #260's false FAIL. Lens names (`adversarial`) lack the prefix, so the +# shipped layers add nothing here. +_SKILL_REF_RE = re.compile(r"`(bmad-[a-z0-9-]+)`") +# Where project-level customization of an upstream skill lives. Both the +# preflight (which reads the overrides) and worktree provisioning (which seeds +# them, so an isolated run resolves the same layers) derive their paths from +# this one constant, or they drift. +CUSTOMIZE_DIR = Path("_bmad") / "custom" +# Overrides of the skill's shipped customize.toml, in precedence order (later +# wins), per customize.toml's own header. The `.user.toml` layer is personal and +# gitignored by the upstream installer. +_CUSTOMIZE_OVERRIDES = ( + CUSTOMIZE_DIR / f"{DEV_PRIMITIVE_SKILL}.toml", + CUSTOMIZE_DIR / f"{DEV_PRIMITIVE_SKILL}.user.toml", +) + + +class ReviewResolution(NamedTuple): + """Which skills the installed bmad-dev-auto's review step actually invokes. + + ``source`` is the file it was read from (for the finding's detail). + ``required`` maps each invoked skill to the review-layer ids invoking it — + empty ids for the pre-consolidation shape, which has no layers. ``advisory`` + is the same shape for references we cannot confirm will run (see + :data:`_SKILL_REF_RE` and ``when``-gated layers): missing ones warn, never + fail. ``active_layers`` names every layer with a non-empty ``instruction`` — + all of them disabled is a run that HALTs. ``unreadable`` names override files + that failed to parse. + """ + + source: str + required: dict[str, tuple[str, ...]] + advisory: dict[str, tuple[str, ...]] + layer_driven: bool + active_layers: tuple[str, ...] + unreadable: tuple[str, ...] + + def skills(self) -> tuple[str, ...]: + """Every skill this review step may invoke, required or advisory.""" + return tuple(dict.fromkeys((*self.required, *self.advisory))) + + +def _read_toml(path: Path) -> dict[str, Any] | None: + """Parse a TOML file, or None if it is absent/unreadable/malformed.""" + try: + return tomllib.loads(path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, tomllib.TOMLDecodeError): + return None + + +def _layers_of(data: Any) -> list[Any]: + """The ``[[workflow.review_layers]]`` array, or [] for any other shape. + + Every hop is type-guarded independently: `workflow` is only a table by + convention, and a syntactically valid `workflow = "..."` would otherwise + raise AttributeError straight out of the preflight — crashing validate, run, + resume and sweep on a file whose only sin is being misconfigured. + """ + workflow = data.get("workflow") if isinstance(data, dict) else None + layers = workflow.get("review_layers") if isinstance(workflow, dict) else None + return list(layers) if isinstance(layers, list) else [] + + +# BMAD's own merge keys for arrays of tables, in detection precedence order. +_KEYED_MERGE_FIELDS = ("code", "id") + + +def _keyed_merge_field(items: list[Any]) -> str | None: + """The key an array of tables merges on, or None to append. + + Port of `_detect_keyed_merge_field` in BMAD-METHOD + `src/scripts/resolve_customization.py`. EVERY item — base and override + combined — must be a table carrying the same key, and `code` is checked + before `id`; a mixed or key-less array falls through to append. Guessing + differently here means the preflight requires a different skill set than the + resolver the run actually uses. + """ + if not items or not all(isinstance(item, dict) for item in items): + return None + for candidate in _KEYED_MERGE_FIELDS: + if all(item.get(candidate) is not None for item in items): + return candidate + return None + + +def _merge_layer_arrays(base: list[Any], override: list[Any]) -> list[Any]: + """Merge two arrays of tables the way BMAD's resolver does. + + Port of `_merge_arrays`/`_merge_by_key` in BMAD-METHOD + `src/scripts/resolve_customization.py`: keyed merge when the combined array + opts into one (matching keys replace in place, new keys append), plain + concatenation otherwise. + """ + key = _keyed_merge_field(base + override) + if key is None: + return base + override + result: list[Any] = [] + index_by_key: dict[Any, int] = {} + for item in base: + if not isinstance(item, dict): + continue + if item.get(key) is not None: + index_by_key[item[key]] = len(result) + result.append(dict(item)) + for item in override: + if not isinstance(item, dict): + result.append(item) + continue + item_key = item.get(key) + if item_key is not None and item_key in index_by_key: + result[index_by_key[item_key]] = dict(item) + else: + if item_key is not None: + index_by_key[item_key] = len(result) + result.append(dict(item)) + return result + + +def _merged_review_layers(project: Path, tree: str) -> tuple[list[Any], tuple[str, ...]] | None: + """The skill's shipped review layers with project overrides applied. + + Returns ``(layers, unreadable_override_paths)``, or None when the skill's OWN + customize.toml is absent or unparseable — the one case that genuinely means + "shape unknown", so the caller falls back to the static catalog. + + An unparseable OVERRIDE is not that case: BMAD's resolver warns and treats it + as empty, still resolving every other layer. Matching that keeps the preflight + agreeing with the run; the broken file is reported separately as a warning. + """ + data = _read_toml(project / tree / DEV_PRIMITIVE_SKILL / "customize.toml") + if data is None: + return None + layers = _layers_of(data) + unreadable: list[str] = [] + for rel in _CUSTOMIZE_OVERRIDES: + override = project / rel + if not override.is_file(): + continue + extra = _read_toml(override) + if extra is None: + unreadable.append(rel.as_posix()) + continue + layers = _merge_layer_arrays(layers, _layers_of(extra)) + return layers, tuple(unreadable) + + +def _layer_id(layer: dict[str, Any], index: int) -> str: + """A layer's id for reporting — positional when it declares none.""" + lid = layer.get("id") + return lid if isinstance(lid, str) and lid else f"#{index + 1}" + + +def resolve_review_layers(project: Path, tree: str) -> ReviewResolution | None: + """Read the review skills this project will really invoke, or None if unknown. + + Post-consolidation bmad-dev-auto is layer-driven: each + ``[[workflow.review_layers]]`` entry carries its whole execution recipe, and a + layer that runs a skill names it inline (an empty ``instruction`` disables the + layer, and a layer may legitimately name no skill at all — `intent-alignment` + is a self-contained prompt). Pre-consolidation, ``step-04-review.md`` names its + reviewers directly instead. + + Reading whichever is installed keeps the preflight honest as upstream moves: + a project whose configured layers invoke a skill it does not have is otherwise + green here and broken on every dev run (#260). None means the shape could not + be determined, so the caller falls back to the static catalog. + + A ``when``-gated layer contributes ADVISORY requirements only: step-04 skips + every layer whose condition does not hold, and that condition is evaluated by + the model in run context — undecidable here, so hard-requiring its skill would + be a false FAIL. + """ + merged = _merged_review_layers(project, tree) + if merged is None: + return None + layers, unreadable = merged + if layers: + required: dict[str, list[str]] = {} + advisory: dict[str, list[str]] = {} + active: list[str] = [] + for index, layer in enumerate(layers): + if not isinstance(layer, dict): + continue + instruction = layer.get("instruction") + if not isinstance(instruction, str) or not instruction.strip(): + continue + layer_id = _layer_id(layer, index) + active.append(layer_id) + when = layer.get("when") + gated = isinstance(when, str) and bool(when.strip()) + invoked = _INVOKE_SKILL_RE.findall(instruction) + # A gated layer's invocations are advisory; so is any bmad-* token we + # can't tie to an invocation phrasing, in either kind of layer. + hard = [] if gated else invoked + soft = [s for s in _SKILL_REF_RE.findall(instruction) if gated or s not in invoked] + for bucket, skills in ((required, hard), (advisory, soft)): + for skill in skills: + ids = bucket.setdefault(skill, []) + if layer_id not in ids: + ids.append(layer_id) + return ReviewResolution( + "customize.toml", + {s: tuple(i) for s, i in required.items()}, + # never warn about a skill another layer already hard-requires + {s: tuple(i) for s, i in advisory.items() if s not in required}, + True, + tuple(active), + unreadable, + ) + try: + step04 = (project / tree / DEV_PRIMITIVE_SKILL / "step-04-review.md").read_text( + encoding="utf-8" + ) + except (OSError, UnicodeDecodeError): + return None + # step-04 is a whole prose document rather than a self-contained execution + # recipe, so only the invocation phrasing counts here — a backticked mention + # anywhere in it is not evidence of a handoff. + named = dict.fromkeys(_INVOKE_SKILL_RE.findall(step04), ()) + if not named: + return None + return ReviewResolution("step-04-review.md", dict(named), {}, False, (), unreadable) + # Stories mode (folder+id dispatch, BMAD-METHOD #2549) needs a *newer* bmad-dev-auto # than sprint mode: one whose step-01 routes a spec-folder + story-id invocation. @@ -160,48 +416,165 @@ def missing_stories_support(project: Path, trees: Sequence[str]) -> list[Finding def missing_base_skills(project: Path, trees: Sequence[str]) -> list[Finding]: """Problems for the upstream skills the orchestrator drives but doesn't bundle. - The dev primitive (bmad-dev-auto) and the three review hunters it invokes - inline — adversarial-general, edge-case-hunter, and verification-gap — are - installed by the BMad Method module, not by `bmad-loop init`. Each must exist - in every active CLI skill tree and carry its marker files. Returns one problem - :class:`Finding` per missing/incomplete skill; empty list means OK. Run as a - preflight so a missing skill fails loudly with remediation instead of stalling - as an `Unknown command` until the run times out. + The dev primitive (bmad-dev-auto) and the review layers its step-04 invokes + inline are installed by the BMad Method module, not by `bmad-loop init`. Each + must exist in every active CLI skill tree and carry its marker files. Returns + one :class:`Finding` per missing/incomplete skill; empty list means OK. Run as + a preflight so a missing skill fails loudly with remediation instead of + stalling as an `Unknown command` until the run times out. + + Not every finding is fatal: review layers that are conditional, ambiguously + phrased, or configured by an unparseable override come back as ``warning`` + (see :func:`_review_findings`). **Callers must branch on severity** — treating + a non-empty return as failure turns every advisory into a blocked run. + + The review skills are read from the installed bmad-dev-auto itself + (:func:`resolve_review_layers`) so the preflight requires what this project + will really invoke: a tree whose configured layers call the merged + ``bmad-review`` needs that skill and not the standalone hunters, and a tree on + the pre-consolidation step-04 needs the two hunters it names. When the shape + can't be read we fall back to the static catalog, with a present + ``bmad-review`` satisfying the hunters. Everything is per tree, since a project + can have a post-consolidation `.claude` tree and a pre-merge `.agents` one side + by side. ``skills.base-incomplete`` carries ``missing_markers`` as a list — the message joins it with ", " for the human line, which a consumer would otherwise have to split back apart on a separator the message is free to change. """ - required = dict(DEV_BASE_SKILLS) problems: list[Finding] = [] for tree in dict.fromkeys(trees): - for skill, markers in required.items(): - skill_dir = project / tree / skill - if not (skill_dir / "SKILL.md").is_file(): - problems.append( - Finding( - "skills.base-missing", - "problem", - f"{tree}/{skill} not found — install the BMad Method (bmm) module " - f"(the orchestrator drives this upstream skill directly)", - {"tree": tree, "skill": skill}, - ) + skill_dir = project / tree / DEV_PRIMITIVE_SKILL + markers = DEV_BASE_SKILLS[DEV_PRIMITIVE_SKILL] + if not (skill_dir / "SKILL.md").is_file(): + problems.append( + Finding( + "skills.base-missing", + "problem", + f"{tree}/{DEV_PRIMITIVE_SKILL} not found — the orchestrator drives this " + f"upstream dev primitive directly; it ships with the bmm module " + f"(BMAD-METHOD >= 6.10.0); install or update bmm in this project", + {"tree": tree, "skill": DEV_PRIMITIVE_SKILL}, ) - continue + ) + else: absent = [m for m in markers if not (skill_dir / m).is_file()] if absent: problems.append( Finding( "skills.base-incomplete", "problem", - f"{tree}/{skill} is incomplete (missing {', '.join(absent)}) — " - f"reinstall it from the bmm module", - {"tree": tree, "skill": skill, "missing_markers": absent}, + f"{tree}/{DEV_PRIMITIVE_SKILL} is incomplete (missing " + f"{', '.join(absent)}) — reinstall it from the bmm module", + {"tree": tree, "skill": DEV_PRIMITIVE_SKILL, "missing_markers": absent}, ) ) + problems.extend(_review_findings(project, tree)) return problems +def _where_clause(layer_ids: Sequence[str]) -> str: + """How a finding's message names the layers that reach for a skill.""" + if len(layer_ids) > 1: + return f"review layers {', '.join(layer_ids)} invoke" + if layer_ids: + return f"review layer {layer_ids[0]} invokes" + return "review step invokes" + + +def _review_findings(project: Path, tree: str) -> list[Finding]: + """Findings for the review skills this tree's bmad-dev-auto invokes. + + Problems block; warnings never do. A skill is only a problem when the + installed config says this run WILL invoke it — anything conditional or + ambiguous warns instead, because a false FAIL here is #260 all over again. + Callers must therefore honour severity rather than treating any finding as + fatal. + """ + resolved = resolve_review_layers(project, tree) + if resolved is None: + # Unknown shape: keep the long-standing static requirement, which both real + # topologies satisfy — the two hunters, or the merged reviewer instead. + if (project / tree / MERGED_REVIEW_SKILL / "SKILL.md").is_file(): + return [] + return [ + Finding( + "skills.base-missing", + "problem", + f"{tree}/{skill} not found — this review layer ships with the bmm module " + f"(BMAD-METHOD >= 6.10.0), as does the consolidated {MERGED_REVIEW_SKILL} " + f"skill that supersedes it in newer releases; install or update bmm in " + f"this project", + {"tree": tree, "skill": skill}, + ) + for skill in sorted(_REVIEW_LAYER_SKILLS) + if not (project / tree / skill / "SKILL.md").is_file() + ] + findings: list[Finding] = [] + for rel in resolved.unreadable: + findings.append( + Finding( + "skills.customize-unreadable", + "warning", + f"{rel} could not be parsed as TOML — the run's resolver skips a broken " + f"override layer, so this project's review layers resolve without it; " + f"fix the file or remove it", + {"tree": tree, "file": rel}, + ) + ) + if resolved.layer_driven and not resolved.active_layers: + findings.append( + Finding( + "skills.review-layers-empty", + "problem", + f"{tree}/{DEV_PRIMITIVE_SKILL} has no enabled review layer (every " + f"`instruction` is empty) — every dev run would HALT blocked with " + f"'no active review layers'; re-enable a layer in " + f"{_CUSTOMIZE_OVERRIDES[0].as_posix()}", + {"tree": tree, "source": resolved.source}, + ) + ) + for skill, layer_ids in sorted(resolved.required.items()): + if (project / tree / skill / "SKILL.md").is_file(): + continue + findings.append( + Finding( + "skills.review-layer-missing", + "problem", + f"{tree}/{skill} not found — {tree}/{DEV_PRIMITIVE_SKILL}'s " + f"{_where_clause(layer_ids)} it ({resolved.source}), so every dev run's " + f"review would fail; install or update the bmm module so this project's " + f"review layers resolve", + { + "tree": tree, + "skill": skill, + "layers": list(layer_ids), + "source": resolved.source, + }, + ) + ) + for skill, layer_ids in sorted(resolved.advisory.items()): + if (project / tree / skill / "SKILL.md").is_file(): + continue + findings.append( + Finding( + "skills.review-layer-unresolved", + "warning", + f"{tree}/{skill} not found — {tree}/{DEV_PRIMITIVE_SKILL}'s " + f"{_where_clause(layer_ids)} it conditionally, or names it in prose this " + f"check cannot confirm is a handoff ({resolved.source}); install it if " + f"that layer is meant to run", + { + "tree": tree, + "skill": skill, + "layers": list(layer_ids), + "source": resolved.source, + }, + ) + ) + return findings + + def _hook_command(project: Path, profile: CLIProfile, canonical_event: str) -> str: host = get_process_host() interp = host.hook_interpreter() diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index 5b999a5..6bd92b7 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -28,11 +28,13 @@ from . import gates, verify from .install import ( BASE_SKILLS, + CUSTOMIZE_DIR, HOOK_SCRIPT_REL, MODULE_SKILLS, _copy_traversable, _worktree_local_exclude, merge_hooks, + resolve_review_layers, ) from .model import Phase from .process_host import get_process_host @@ -81,9 +83,12 @@ def provision_worktree( server. Lay the bundled skills + signal hook into the worktree for the active CLI profiles, and copy the `seed_files` configs in from the main repo. The upstream skills the orchestrator drives (BASE_SKILLS: bmad-dev-auto + the review - hunters) are not bundled in the wheel, so they are copied from the MAIN REPO's - installed tree instead. Quiet (no stdout) — unlike `install_into` this runs - inside the engine loop under a TUI. No-op when there's nothing to do. + hunters, plus whatever review layers this project's own config names) are not + bundled in the wheel, so they are copied from the MAIN REPO's installed tree + instead — together with `_bmad/custom/`, the customization those layers resolve + through, so the isolated run resolves the same layer set the preflight + validated. Quiet (no stdout) — unlike `install_into` this runs inside the + engine loop under a TUI. No-op when there's nothing to do. seed_globs are project-relative glob patterns (e.g. ".claude/skills/*") expanded against the main repo; every match is copied into the worktree under the same @@ -196,12 +201,21 @@ def provision_worktree( if dst.exists(): continue _copy_traversable(skills_root.joinpath(skill), dst) - # The orchestrator-driven upstream skills (BASE_SKILLS) are not in the - # wheel; copy them from the MAIN REPO's installed tree (same tree path) so - # an isolated worktree can still resolve /bmad-dev-auto and the review - # hunters. Skip silently when the main repo lacks them — the run-start - # preflight reports it. - for skill in BASE_SKILLS: + # The orchestrator-driven upstream skills are not in the wheel; copy them + # from the MAIN REPO's installed tree (same tree path) so an isolated + # worktree can still resolve /bmad-dev-auto and the review layers. Skip + # silently when the main repo lacks them — the run-start preflight reports + # it. + # + # BASE_SKILLS is only the floor. The review layers this project actually + # invokes are read from the installed bmad-dev-auto, exactly as the + # preflight reads them, so a reviewer named by a project override (a + # custom or renamed skill) is provisioned too. Validating a skill here and + # then not copying it is how preflight passes in the main checkout while + # the isolated review fails on a skill that was never there. + resolved = resolve_review_layers(repo_root, tree) + required = dict.fromkeys((*BASE_SKILLS, *(resolved.skills() if resolved else ()))) + for skill in required: dst = tree_dir / skill if dst.exists(): continue @@ -210,6 +224,33 @@ def provision_worktree( continue _copy_traversable(src, dst) + # The project's customization of those upstream skills (_bmad/custom/*.toml). + # The preflight resolves review layers through these files, but the run inside + # the worktree resolves them again from ITS OWN project root — and they are + # routinely absent from a fresh checkout: the upstream installer gitignores + # `*.user.toml`, and plenty of projects gitignore `_bmad/` whole. Without this + # the two disagree, and validate approves a layer set the isolated run never + # runs. Copy-when-absent at file granularity, so a checkout that tracks its + # own customization keeps every tracked file untouched. + # + # Deliberately NOT routed through seed_files: `skipped` reports user-authored + # `worktree_seed` entries that silently no-op, and this internal seed is not + # one of those — journaling it would read as a project misconfiguration. + customize_seeded = False + src_customize = (repo_root / CUSTOMIZE_DIR).resolve() + dst_customize = (worktree / CUSTOMIZE_DIR).resolve() + if ( + src_customize.is_dir() + and src_customize.is_relative_to(repo_root) + and dst_customize.is_relative_to(worktree) + ): + if not dst_customize.exists(): + dst_customize.parent.mkdir(parents=True, exist_ok=True) + _copy_traversable(src_customize, dst_customize) + customize_seeded = True + elif dst_customize.is_dir(): + customize_seeded = _copy_traversable(src_customize, dst_customize, skip_existing=True) + # per-CLI signal-hook registration, baked to the main repo's relay (absolute). # Hookless profiles (HTTP/SSE transport) have no config to merge. for profile in profiles: @@ -241,6 +282,8 @@ def provision_worktree( # as the pattern "/", git-excluding the entire worktree. patterns |= {f"/{p.hooks.config_path}" for p in profiles if not p.hookless} patterns |= {f"/{rel}" for rel in seeded} + if customize_seeded: + patterns.add(f"/{CUSTOMIZE_DIR.as_posix()}") _worktree_local_exclude(worktree, sorted(patterns)) return skipped diff --git a/tests/test_cli.py b/tests/test_cli.py index 76ae8f1..a92adbd 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -3214,6 +3214,31 @@ def _boom(): STORIES_POLICY = '[stories]\nsource = "stories"\nspec_folder = "_bmad-output/epic-1"\n' +def test_require_base_skills_warnings_do_not_block_the_run(tmp_path, monkeypatch, capsys): + """A review layer the preflight cannot statically resolve (a `when` gate, a + handoff phrasing it can't confirm, an unparseable override) is reported and then + stepped over. Aborting on it would be #260's false FAIL — this time on every + run, not just on validate — so severity decides, not emptiness.""" + from bmad_loop import install as install_mod + from bmad_loop.checks import Finding + + _write_policy(tmp_path) + pol = policy_mod.load(tmp_path / ".bmad-loop" / "policy.toml") + warning = Finding("skills.review-layer-unresolved", "warning", "conditional layer", {}) + monkeypatch.setattr(install_mod, "missing_base_skills", lambda *a, **k: [warning]) + + assert cli._require_base_skills(tmp_path, pol) is True + err = capsys.readouterr().err + assert "conditional layer" in err + assert "FAIL" not in err + + # ...and a real problem alongside it still aborts + problem = Finding("skills.review-layer-missing", "problem", "missing reviewer", {}) + monkeypatch.setattr(install_mod, "missing_base_skills", lambda *a, **k: [warning, problem]) + assert cli._require_base_skills(tmp_path, pol) is False + assert "FAIL: missing reviewer" in capsys.readouterr().err + + def _validate_output(capsys): out = capsys.readouterr() return (out.out + out.err).lower() diff --git a/tests/test_install.py b/tests/test_install.py index 97b1f1a..ed1530f 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -1,4 +1,5 @@ import json +import re import sys import zipfile @@ -9,6 +10,7 @@ from bmad_loop.adapters.profile import get_profile from bmad_loop.install import ( BASE_SKILLS, + DEV_BASE_SKILLS, LEGACY_MODULE_SKILLS, MODULE_SKILLS, _copy_traversable, @@ -20,9 +22,13 @@ ) -def _install_base_skills(root, tree=".claude/skills"): - """Lay down stubs of the non-bundled upstream skills the orchestrator drives.""" - for skill, markers in BASE_SKILLS.items(): +def _install_skills(root, tree, catalog): + """Lay down stubs of exactly ``catalog`` ({skill: (marker, ...)}) under root/tree. + + Takes the catalog explicitly so a test can build one precise upstream topology + (e.g. the v6.10.0 shape: no `bmad-review`, no verification-gap) rather than the + everything-installed superset.""" + for skill, markers in catalog.items(): d = root / tree / skill d.mkdir(parents=True, exist_ok=True) (d / "SKILL.md").write_text(f"# {skill}\n", encoding="utf-8") @@ -30,6 +36,11 @@ def _install_base_skills(root, tree=".claude/skills"): (d / marker).write_text("x\n", encoding="utf-8") +def _install_base_skills(root, tree=".claude/skills"): + """Lay down stubs of the non-bundled upstream skills the orchestrator drives.""" + _install_skills(root, tree, BASE_SKILLS) + + def _registrations(profile, command="python3 /x/.bmad-loop/bmad_loop_hook.py {event}"): return { native: command.format(event=canonical) @@ -601,12 +612,12 @@ def test_provision_worktree_copies_base_skills_from_repo(tmp_path): def test_missing_base_skills_reports_absent_and_incomplete(tmp_path): claude = get_profile("claude") - # nothing installed → dev primitive + all three inline review hunters reported - # missing (the hunters are always required — bmad-dev-auto's step-04 invokes - # them on every run, regardless of the orchestrator's follow-up review) + # nothing installed → dev primitive + the two inline review layers reported + # missing (the hunters are required whenever the merged reviewer is absent — + # bmad-dev-auto's step-04 invokes them on every run) problems = missing_base_skills(tmp_path, [claude.skill_tree]) - assert len(problems) == 4 - assert all("install the BMad Method" in p.message for p in problems) + assert len(problems) == 3 + assert all("BMAD-METHOD >= 6.10.0" in p.message for p in problems) # install everything → no problems _install_base_skills(tmp_path, claude.skill_tree) @@ -628,15 +639,13 @@ def test_missing_base_skills_reports_absent_and_incomplete(tmp_path): assert "incomplete" in problems[0].message assert "customize.toml" in problems[0].message - # the newest review layer (verification-gap) reported by name when absent + # #260: verification-gap is NOT a requirement — no tagged BMAD-METHOD release + # ships it, so removing it from an otherwise complete tree must still pass _install_base_skills(tmp_path, claude.skill_tree) # re-complete everything import shutil as _shutil _shutil.rmtree(tmp_path / claude.skill_tree / "bmad-review-verification-gap") - problems = missing_base_skills(tmp_path, [claude.skill_tree]) - assert len(problems) == 1 - assert "bmad-review-verification-gap" in problems[0].message - assert "install the BMad Method" in problems[0].message + assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] def test_missing_stories_support_probes_step01_content(tmp_path): @@ -681,7 +690,6 @@ def test_missing_base_skills_findings_carry_ids_and_detail(tmp_path): "bmad-dev-auto", "bmad-review-adversarial-general", "bmad-review-edge-case-hunter", - "bmad-review-verification-gap", } assert all(f.detail["tree"] == claude.skill_tree for f in absent) @@ -697,6 +705,626 @@ def test_missing_base_skills_findings_carry_ids_and_detail(tmp_path): assert marker in incomplete[0].message +def test_merged_bmad_review_satisfies_review_layers(tmp_path): + """#260: post-consolidation bmm installs ship the merged `bmad-review` skill, with + the standalone hunter IDs as thin forwarders to it. The merged reviewer provides + every lens itself, so a tree carrying it needs none of the hunters.""" + claude = get_profile("claude") + _install_skills( + tmp_path, + claude.skill_tree, + {"bmad-dev-auto": DEV_BASE_SKILLS["bmad-dev-auto"], "bmad-review": ()}, + ) + assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] + + # ...but it never substitutes for the dev primitive + import shutil as _shutil + + _shutil.rmtree(tmp_path / claude.skill_tree / "bmad-dev-auto") + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert len(problems) == 1 + assert problems[0].detail["skill"] == "bmad-dev-auto" + + +def test_verification_gap_never_required(tmp_path): + """#260: the latest-release (v6.10.0) shape — dev primitive + the two review + layers that release actually ships, no `bmad-review-verification-gap` (no tagged + release has ever shipped it) and no merged reviewer — must validate. Requiring it + made `validate` (and the run/resume/sweep preflight) unsatisfiable everywhere.""" + claude = get_profile("claude") + _install_skills(tmp_path, claude.skill_tree, DEV_BASE_SKILLS) + # guard the topology: neither escape hatch is present, so [] below is earned by + # verification-gap not being required, not by the merged-reviewer bypass + assert not (tmp_path / claude.skill_tree / "bmad-review-verification-gap").exists() + assert not (tmp_path / claude.skill_tree / "bmad-review").exists() + + assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] + + +def test_review_hunter_missing_without_merged_review_reported(tmp_path): + """A genuinely broken pre-consolidation install still fails — and its message must + not misdiagnose the cause as "bmm is not installed" (#260): bmm is exactly what + ships the layer, so a user whose bmm is installed could not act on the old line.""" + claude = get_profile("claude") + _install_skills( + tmp_path, + claude.skill_tree, + {k: v for k, v in DEV_BASE_SKILLS.items() if k != "bmad-review-edge-case-hunter"}, + ) + + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert len(problems) == 1 + assert problems[0].check == "skills.base-missing" + assert problems[0].severity == "problem" + assert problems[0].detail == { + "tree": claude.skill_tree, + "skill": "bmad-review-edge-case-hunter", + } + assert "bmad-review-edge-case-hunter" in problems[0].message + assert "install the BMad Method" not in problems[0].message + assert "update bmm" in problems[0].message + + +# Verbatim shape of BMAD-METHOD main's bmad-dev-auto/customize.toml: four review +# layers, three invoking the merged `bmad-review` with one lens each, plus +# intent-alignment — a self-contained prompt that invokes no skill at all. +LAYER_CUSTOMIZE = """ +[workflow] +implementation_handoff = "irrelevant here" + +[[workflow.review_layers]] +id = "blind-hunter" +name = "Blind Hunter" +instruction = ''' +Launch a subagent with no prior conversation context, with this prompt: + +> Invoke the `bmad-review` skill with only the `adversarial` lens on this diff: +> +> {diff_output} +''' + +[[workflow.review_layers]] +id = "edge-case-hunter" +name = "Edge Case Hunter" +instruction = ''' +> Invoke the `bmad-review` skill with only the `edge-case-hunter` lens on this diff: +> +> {diff_output} +''' + +[[workflow.review_layers]] +id = "verification-gap" +name = "Verification Gap Reviewer" +instruction = ''' +> Invoke the `bmad-review` skill with only the `verification-gap` lens on this diff: +> +> {diff_output} +''' + +[[workflow.review_layers]] +id = "intent-alignment" +name = "Intent Alignment Auditor" +instruction = ''' +> You are an intent-alignment auditor. Here is the diff: +> +> {diff_output} +''' +""" + +# Pre-consolidation (v6.10.0) shape: a valid customize.toml that simply has no +# review_layers section, and a step-04 that names the two reviewers it invokes. +PRE_LAYER_CUSTOMIZE = """ +[workflow] +activation_steps_prepend = [] +persistent_facts = ["file:{project-root}/**/project-context.md"] +on_complete = "" +""" + +STEP04_NAMED = """ +### Step 2: Review layers + +- Launch a subagent, with this prompt: + > Invoke the `bmad-review-adversarial-general` skill on this diff: + > {diff_output} +- Launch a subagent, with this prompt: + > Invoke the `bmad-review-edge-case-hunter` skill on this diff: + > {diff_output} +""" + + +def _install_dev_auto(root, tree, *, customize="x\n", step04="x\n"): + """Install bmad-dev-auto with real customize.toml / step-04 content, so the + preflight reads the review shape it would read on a real install.""" + d = root / tree / "bmad-dev-auto" + d.mkdir(parents=True, exist_ok=True) + (d / "SKILL.md").write_text("# bmad-dev-auto\n", encoding="utf-8") + (d / "customize.toml").write_text(customize, encoding="utf-8") + (d / "step-04-review.md").write_text(step04, encoding="utf-8") + return d + + +def test_layer_driven_review_requires_the_merged_skill_it_names(tmp_path): + """Post-consolidation topology: the layers invoke `bmad-review` by name, so that + skill — and none of the standalone hunters — is what the tree must carry.""" + claude = get_profile("claude") + _install_dev_auto(tmp_path, claude.skill_tree, customize=LAYER_CUSTOMIZE) + _install_skills(tmp_path, claude.skill_tree, {"bmad-review": ()}) + + assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] + + +def test_layer_driven_review_reports_unresolvable_layer(tmp_path): + """The gap this closes: a project whose customize.toml is post-consolidation but + whose skills are the pre-consolidation standalone hunters. The old preflight was + green here while three of the four layers would fail on every dev run.""" + claude = get_profile("claude") + _install_dev_auto(tmp_path, claude.skill_tree, customize=LAYER_CUSTOMIZE) + _install_skills( + tmp_path, + claude.skill_tree, + { + "bmad-review-adversarial-general": (), + "bmad-review-edge-case-hunter": (), + "bmad-review-verification-gap": (), + }, + ) + + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert len(problems) == 1 + assert problems[0].check == "skills.review-layer-missing" + assert problems[0].severity == "problem" + assert problems[0].detail["skill"] == "bmad-review" + # the three lens layers name it; intent-alignment invokes no skill at all + assert problems[0].detail["layers"] == [ + "blind-hunter", + "edge-case-hunter", + "verification-gap", + ] + assert problems[0].detail["source"] == "customize.toml" + assert "bmad-review" in problems[0].message + + +def test_review_layer_check_id_is_registered(tmp_path): + from bmad_loop.checks import VALIDATE_CHECKS + + assert "skills.review-layer-missing" in VALIDATE_CHECKS + + +def test_pre_consolidation_step04_requires_the_skills_it_names(tmp_path): + """v6.10.0 shape: no review_layers, so the requirement comes from the two skills + step-04 invokes by name — and a tree carrying only the merged reviewer does NOT + satisfy them, because that step-04 names the hunters, not `bmad-review`.""" + claude = get_profile("claude") + tree = claude.skill_tree + _install_dev_auto(tmp_path, tree, customize=PRE_LAYER_CUSTOMIZE, step04=STEP04_NAMED) + _install_skills( + tmp_path, + tree, + {"bmad-review-adversarial-general": (), "bmad-review-edge-case-hunter": ()}, + ) + assert missing_base_skills(tmp_path, [tree]) == [] + + import shutil as _shutil + + _shutil.rmtree(tmp_path / tree / "bmad-review-edge-case-hunter") + problems = missing_base_skills(tmp_path, [tree]) + assert len(problems) == 1 + assert problems[0].check == "skills.review-layer-missing" + assert problems[0].detail["skill"] == "bmad-review-edge-case-hunter" + assert problems[0].detail["layers"] == [] + assert problems[0].detail["source"] == "step-04-review.md" + + +def test_disabled_review_layer_is_not_required(tmp_path): + """An empty `instruction` disables a layer — its skill must not be required.""" + claude = get_profile("claude") + disabled = LAYER_CUSTOMIZE.replace( + """instruction = ''' +> Invoke the `bmad-review` skill with only the `verification-gap` lens on this diff: +> +> {diff_output} +'''""", + 'instruction = ""', + ) + _install_dev_auto(tmp_path, claude.skill_tree, customize=disabled) + _install_skills(tmp_path, claude.skill_tree, {"bmad-review": ()}) + + assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] + + +def test_project_override_replaces_review_layer(tmp_path): + """`_bmad/custom/bmad-dev-auto.toml` merges arrays of tables by `id`. A layer + overridden to run an external reviewer no longer requires the default's skill — + requiring it anyway would be exactly the kind of false FAIL #260 was.""" + claude = get_profile("claude") + only_one_layer = LAYER_CUSTOMIZE.split("[[workflow.review_layers]]")[0] + ( + """[[workflow.review_layers]] +id = "blind-hunter" +instruction = ''' +> Invoke the `bmad-review` skill with only the `adversarial` lens on this diff: +''' +""" + ) + _install_dev_auto(tmp_path, claude.skill_tree, customize=only_one_layer) + assert len(missing_base_skills(tmp_path, [claude.skill_tree])) == 1 + + override = tmp_path / "_bmad" / "custom" / "bmad-dev-auto.toml" + override.parent.mkdir(parents=True, exist_ok=True) + override.write_text( + """[[workflow.review_layers]] +id = "blind-hunter" +instruction = "Run `my-external-reviewer` via bash on the diff." +""", + encoding="utf-8", + ) + assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] + + +def test_unreadable_customize_falls_back_to_static_catalog(tmp_path): + """A malformed customize.toml must not crash the preflight, and must not be read + as "no layers configured" either — fall back to the static catalog.""" + claude = get_profile("claude") + _install_dev_auto(tmp_path, claude.skill_tree, customize="this is not = valid toml [[[") + + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert {p.detail["skill"] for p in problems} == { + "bmad-review-adversarial-general", + "bmad-review-edge-case-hunter", + } + assert {p.check for p in problems} == {"skills.base-missing"} + + # ...and the merged reviewer still satisfies that fallback + _install_skills(tmp_path, claude.skill_tree, {"bmad-review": ()}) + assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] + + +# --- derivation vs. what the run really resolves (PR #283 review) ------------- +# +# Everything below pins the preflight to BMAD's own resolver and to step-04's own +# skip rules. The failure mode being guarded is asymmetric: requiring a skill the +# run never invokes is a false FAIL (#260), and accepting a layer the run cannot +# resolve is a green validate followed by a broken review on every story. + + +def _write_override(root, body, *, user=False): + """A project override of bmad-dev-auto's shipped customize.toml.""" + suffix = "user.toml" if user else "toml" + path = root / "_bmad" / "custom" / f"bmad-dev-auto.{suffix}" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(body, encoding="utf-8") + return path + + +def _layer(layer_id, skill, *, key="id", when=None, phrasing="Invoke the"): + when_line = f'when = "{when}"\n' if when else "" + return f""" +[[workflow.review_layers]] +{key} = "{layer_id}" +{when_line}instruction = "{phrasing} `{skill}` skill on this diff." +""" + + +def _severities(findings): + return {(f.check, f.severity) for f in findings} + + +def test_appended_override_layer_requires_the_skill_it_names(tmp_path): + """A new `id` appends rather than replaces, so an override that adds a reviewer + adds a requirement — the run will invoke it, so the preflight must too.""" + claude = get_profile("claude") + _install_dev_auto(tmp_path, claude.skill_tree, customize=LAYER_CUSTOMIZE) + _install_skills(tmp_path, claude.skill_tree, {"bmad-review": ()}) + assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] + + _write_override(tmp_path, _layer("house-style", "bmad-review-company")) + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert len(problems) == 1 + assert problems[0].check == "skills.review-layer-missing" + assert problems[0].severity == "problem" + assert problems[0].detail["skill"] == "bmad-review-company" + assert problems[0].detail["layers"] == ["house-style"] + + # ...and installing it clears the problem, rather than the skill being + # unreachable because it is not in any catalog this package pins. + _install_skills(tmp_path, claude.skill_tree, {"bmad-review-company": ()}) + assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] + + +def test_standalone_verification_gap_required_when_a_layer_names_it(tmp_path): + """Dropping bmad-review-verification-gap from the static catalog must not make + it unrequirable: a project whose layers DO name it still needs it installed.""" + claude = get_profile("claude") + customize = "[workflow]\n" + _layer("verification-gap", "bmad-review-verification-gap") + _install_dev_auto(tmp_path, claude.skill_tree, customize=customize) + + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert [p.detail["skill"] for p in problems] == ["bmad-review-verification-gap"] + assert problems[0].severity == "problem" + + _install_skills(tmp_path, claude.skill_tree, {"bmad-review-verification-gap": ()}) + assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] + + +def test_pre_and_post_consolidation_trees_resolve_independently(tmp_path): + """A project can carry a post-consolidation .claude tree and a pre-merge .agents + one at once. Each tree's requirement comes from ITS OWN installed skill.""" + claude, codex = get_profile("claude"), get_profile("codex") + assert claude.skill_tree != codex.skill_tree + + _install_dev_auto(tmp_path, claude.skill_tree, customize=LAYER_CUSTOMIZE) + _install_skills(tmp_path, claude.skill_tree, {"bmad-review": ()}) + _install_dev_auto( + tmp_path, codex.skill_tree, customize=PRE_LAYER_CUSTOMIZE, step04=STEP04_NAMED + ) + _install_skills( + tmp_path, + codex.skill_tree, + {"bmad-review-adversarial-general": (), "bmad-review-edge-case-hunter": ()}, + ) + assert missing_base_skills(tmp_path, [claude.skill_tree, codex.skill_tree]) == [] + + # the merged reviewer in the OTHER tree must not satisfy the pre-merge one + import shutil as _shutil + + _shutil.rmtree(tmp_path / codex.skill_tree / "bmad-review-edge-case-hunter") + problems = missing_base_skills(tmp_path, [claude.skill_tree, codex.skill_tree]) + assert len(problems) == 1 + assert problems[0].detail["tree"] == codex.skill_tree + assert problems[0].detail["skill"] == "bmad-review-edge-case-hunter" + + +def test_malformed_override_warns_and_still_resolves_base_layers(tmp_path): + """BMAD's resolver warns on an unparseable override and carries on with the + layers below it. Falling back to a static catalog instead would preflight a + requirement set the run does not use.""" + claude = get_profile("claude") + _install_dev_auto(tmp_path, claude.skill_tree, customize=LAYER_CUSTOMIZE) + _write_override(tmp_path, "this is not = valid toml [[[") + + findings = missing_base_skills(tmp_path, [claude.skill_tree]) + assert ("skills.customize-unreadable", "warning") in _severities(findings) + # the base layers still drive the requirement: `bmad-review`, not the static + # two-hunter catalog the old code fell back to + problems = [f for f in findings if f.severity == "problem"] + assert [p.detail["skill"] for p in problems] == ["bmad-review"] + assert all(p.check == "skills.review-layer-missing" for p in problems) + + _install_skills(tmp_path, claude.skill_tree, {"bmad-review": ()}) + findings = missing_base_skills(tmp_path, [claude.skill_tree]) + # the broken file is still surfaced, but nothing blocks + assert [f.severity for f in findings] == ["warning"] + assert findings[0].detail["file"].endswith("bmad-dev-auto.toml") + + +def test_user_override_wins_over_team_override(tmp_path): + """Precedence is base -> team -> user, so the personal layer decides.""" + claude = get_profile("claude") + _install_dev_auto( + tmp_path, claude.skill_tree, customize="[workflow]\n" + _layer("blind", "bmad-review") + ) + _write_override(tmp_path, _layer("blind", "bmad-review-team")) + _write_override(tmp_path, _layer("blind", "bmad-review-personal"), user=True) + + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert [p.detail["skill"] for p in problems] == ["bmad-review-personal"] + + +def test_every_review_layer_disabled_is_reported(tmp_path): + """Emptying every `instruction` disables every layer, and step-04 then HALTs + blocked with 'no active review layers'. Preflight must not call that green.""" + claude = get_profile("claude") + disabled = re.sub( + r"instruction = '''.*?'''", 'instruction = ""', LAYER_CUSTOMIZE, flags=re.DOTALL + ) + _install_dev_auto(tmp_path, claude.skill_tree, customize=disabled) + _install_skills(tmp_path, claude.skill_tree, {"bmad-review": ()}) + + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert len(problems) == 1 + assert problems[0].check == "skills.review-layers-empty" + assert problems[0].severity == "problem" + assert "no active review layers" in problems[0].message + + +def test_code_keyed_layers_merge_by_code_like_upstream(tmp_path): + """BMAD's resolver keys arrays of tables on `code` OR `id` — `code` first. A + code-keyed layer that an override replaces must not survive as a requirement.""" + claude = get_profile("claude") + _install_dev_auto( + tmp_path, + claude.skill_tree, + customize="[workflow]\n" + _layer("R1", "bmad-review-old", key="code"), + ) + _write_override(tmp_path, _layer("R1", "bmad-review-new", key="code")) + + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + # replaced in place: only the override's skill is required + assert [p.detail["skill"] for p in problems] == ["bmad-review-new"] + + +def test_keyless_override_item_forces_append_like_upstream(tmp_path): + """The keyed merge is opt-in for the array as a WHOLE: one override item with no + identifier and the resolver appends everything, leaving the base layer in place. + Replacing by id anyway drops a reviewer the run still executes — a green + validate followed by a review that fails on a skill nobody checked for.""" + claude = get_profile("claude") + _install_dev_auto( + tmp_path, + claude.skill_tree, + customize="[workflow]\n" + _layer("blind", "bmad-review-base"), + ) + _write_override( + tmp_path, + _layer("blind", "bmad-review-replacement") + + '\n[[workflow.review_layers]]\ninstruction = "Invoke the `bmad-review-extra` skill."\n', + ) + + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert sorted(p.detail["skill"] for p in problems) == [ + "bmad-review-base", + "bmad-review-extra", + "bmad-review-replacement", + ] + # the id-less layer is still reported by position, so the finding is actionable + extra = next(p for p in problems if p.detail["skill"] == "bmad-review-extra") + assert extra.detail["layers"] == ["#3"] + + +@pytest.mark.parametrize("value", ['"wrong shape"', "[1, 2]", "42", "true"]) +def test_non_table_workflow_does_not_crash_the_preflight(tmp_path, value): + """Syntactically valid TOML of the wrong SHAPE used to raise AttributeError out + of the preflight, taking validate/run/resume/sweep with it.""" + claude = get_profile("claude") + _install_dev_auto(tmp_path, claude.skill_tree, customize=f"workflow = {value}\n") + + # no layers readable -> the static fallback, not an exception + findings = missing_base_skills(tmp_path, [claude.skill_tree]) + assert {f.check for f in findings} == {"skills.base-missing"} + + +def test_when_gated_layer_warns_instead_of_blocking(tmp_path): + """step-04 skips every layer whose `when` does not hold, and that condition is + evaluated by the model in run context. Undecidable here, so it must not FAIL.""" + claude = get_profile("claude") + _install_dev_auto( + tmp_path, + claude.skill_tree, + customize="[workflow]\n" + + _layer("blind", "bmad-review") + + _layer("perf", "bmad-review-performance", when="the diff touches hot paths"), + ) + _install_skills(tmp_path, claude.skill_tree, {"bmad-review": ()}) + + findings = missing_base_skills(tmp_path, [claude.skill_tree]) + assert len(findings) == 1 + assert findings[0].check == "skills.review-layer-unresolved" + assert findings[0].severity == "warning" + assert findings[0].detail["skill"] == "bmad-review-performance" + assert findings[0].detail["layers"] == ["perf"] + + +def test_unrecognized_handoff_phrasing_warns_instead_of_blocking(tmp_path): + """The invocation phrasing is a convention, not a contract — upstream itself + writes "use the `x` skill" elsewhere. An unconfirmable reference is surfaced, + never blocked on: guessing wrong rebuilds #260.""" + claude = get_profile("claude") + _install_dev_auto( + tmp_path, + claude.skill_tree, + customize="[workflow]\n" + + _layer("blind", "bmad-review") + + _layer("house", "bmad-review-company", phrasing="Use the"), + ) + _install_skills(tmp_path, claude.skill_tree, {"bmad-review": ()}) + + findings = missing_base_skills(tmp_path, [claude.skill_tree]) + assert [(f.check, f.severity) for f in findings] == [ + ("skills.review-layer-unresolved", "warning") + ] + assert findings[0].detail["skill"] == "bmad-review-company" + + +def test_skill_required_by_one_layer_is_never_also_advisory(tmp_path): + """A hard requirement wins: the same skill named by a gated layer and an + ungated one is reported once, as a problem.""" + claude = get_profile("claude") + _install_dev_auto( + tmp_path, + claude.skill_tree, + customize="[workflow]\n" + + _layer("blind", "bmad-review") + + _layer("perf", "bmad-review", when="sometimes"), + ) + + findings = missing_base_skills(tmp_path, [claude.skill_tree]) + assert [(f.check, f.severity) for f in findings] == [("skills.review-layer-missing", "problem")] + + +def test_new_review_check_ids_are_registered(): + """A check id that isn't in the registry asserts at emit time — i.e. ships as a + crash on exactly the misconfigured project it was meant to report.""" + from bmad_loop.checks import VALIDATE_CHECKS + + assert { + "skills.review-layer-unresolved", + "skills.review-layers-empty", + "skills.customize-unreadable", + } <= VALIDATE_CHECKS + + +def test_provision_worktree_copies_derived_review_skill(tmp_path): + """Validating a custom reviewer and then not provisioning it is how preflight + passes in the main checkout while the isolated review fails on a skill that was + never there. The worktree gets what the layers actually name.""" + wt, repo = tmp_path / "wt", tmp_path / "repo" + claude = get_profile("claude") + _install_base_skills(repo, claude.skill_tree) + _install_dev_auto( + repo, + claude.skill_tree, + customize="[workflow]\n" + _layer("house-style", "bmad-review-company"), + ) + _install_skills(repo, claude.skill_tree, {"bmad-review-company": ()}) + + provision_worktree(wt, [claude], repo) + + assert (wt / claude.skill_tree / "bmad-review-company" / "SKILL.md").is_file() + # the floor is still copied, so a tree whose config we cannot read is unchanged + for skill in BASE_SKILLS: + assert (wt / claude.skill_tree / skill / "SKILL.md").is_file() + + +def test_provision_worktree_seeds_bmad_custom(tmp_path): + """The run inside the worktree resolves review layers from ITS OWN project + root. `*.user.toml` is gitignored by the upstream installer (and plenty of + projects gitignore `_bmad/` whole), so without seeding, validate approves a + layer set the isolated run never resolves.""" + wt, repo = tmp_path / "wt", tmp_path / "repo" + claude = get_profile("claude") + _install_base_skills(repo, claude.skill_tree) + _write_override(repo, _layer("house-style", "bmad-review-company"), user=True) + + provision_worktree(wt, [claude], repo) + + seeded = wt / "_bmad" / "custom" / "bmad-dev-auto.user.toml" + assert seeded.is_file() + assert "bmad-review-company" in seeded.read_text(encoding="utf-8") + + +def test_provision_worktree_bmad_custom_does_not_clobber_checkout(tmp_path): + """A checkout that tracks its own customization keeps every tracked file — only + the children it lacks (the gitignored personal layer) are filled in.""" + wt, repo = tmp_path / "wt", tmp_path / "repo" + claude = get_profile("claude") + _install_base_skills(repo, claude.skill_tree) + _write_override(repo, _layer("team", "bmad-review-repo-side")) + _write_override(repo, _layer("personal", "bmad-review-mine"), user=True) + # the worktree checked out the TRACKED team layer, at a different revision + tracked = wt / "_bmad" / "custom" / "bmad-dev-auto.toml" + tracked.parent.mkdir(parents=True, exist_ok=True) + tracked.write_text("# checked out\n", encoding="utf-8") + + provision_worktree(wt, [claude], repo) + + assert tracked.read_text(encoding="utf-8") == "# checked out\n" + assert (wt / "_bmad" / "custom" / "bmad-dev-auto.user.toml").is_file() + + +def test_provision_worktree_bmad_custom_shielded_in_local_exclude(project, tmp_path): + """Seeded customization must stay out of the unit's `git add -A` — a project + that doesn't gitignore `_bmad/` would otherwise merge it back on every story.""" + repo = project.project + _write_override(repo, _layer("house-style", "bmad-review-company"), user=True) + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + + provision_worktree(wt, [get_profile("claude")], repo) + + assert (wt / "_bmad" / "custom" / "bmad-dev-auto.user.toml").is_file() + exclude = (repo / ".git" / "info" / "exclude").read_text(encoding="utf-8") + assert "/_bmad/custom" in exclude.splitlines() + + def test_missing_stories_support_findings_split_absent_from_stale(tmp_path): """#205: a half install and a too-old install are different conditions with different remediations, so they get different check ids — a script pinning a