diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ad1e5fe..e2fd3995 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -260,6 +260,29 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Fixed +- **The upstream `bmad-dev-auto` → `bmad-build-auto` rename no longer breaks a project (#393).** + The dev primitive is resolved on disk — `bmad-build-auto` preferred, a marker-complete + `bmad-dev-auto` accepted — so `validate`/`run`/`sweep`/`resume` pass on either era with no + `policy.toml` edit. The forwarding shim upstream left behind is refused as marker-incomplete + (`skills.base-shim`) — no step files, no `customize.toml`, which is also what a truncated + install looks like, so the message names both causes: the shim is a valid slash command, so an + unattended session dispatched into it would HALT on its interactive migration gate having + written nothing. Every session prompt spells + the resolved name, per skill tree, so a run mixing `.claude/skills` and `.agents/skills` at + different eras gets the right one per role; the no-spec fallback result marker is matched under + both prefixes. The name is resolved against the **workspace**, so a run resumed into an existing + worktree spells the era that worktree actually carries: resume re-mounts a worktree without + re-provisioning it, and a main checkout upgraded across the pause would otherwise dispatch a + command that worktree does not have, halting the session having written nothing. On a project + mid-upgrade, with a different era in each tree, the orphaned-override warning says to **copy** + `_bmad/custom/bmad-dev-auto.toml` rather than rename it — the legacy tree still applies it, so a + rename would move the customization between trees instead of migrating it. `--dry-run` now says + on stderr when its preview is not runnable rather than + printing a plausible schedule for an install that would abort at preflight (exit code stays 0, + stdout untouched). The skills preflight also stopped gating a triage-only CLI's skill tree — a + hard FAIL, with no `--force`, over skills no triage session ever dispatches. `[dev] skill` stays + `bmad-dev-auto`: it is the adapter discriminator, not the invoked name. + - **The worktree git-add shield no longer hides new files in your own checkout (#384).** Worktree provisioning shielded the tool files it writes (`.claude/skills`, the per-CLI hook config, seeded configs) by appending to `.git/info/exclude` — a file shared with the main checkout and every @@ -801,6 +824,507 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories routing input to the upstream skill, so the frontmatter is **never** mutated at review launch — every mechanism is observation or a prose-append, never a status write. +## [0.9.1] — 2026-08-01 + +Compatibility hotfix for the BMad Method's `bmad-dev-auto` → `bmad-build-auto` rename +(BMAD-METHOD#2651, first shipped in bmad-method 6.10.1-next.33) and the two upstream changes +that rode the same window. Both skill eras are supported; the rename itself needs no +`policy.toml` edit. + +### Fixed + +- **The dev primitive is now resolved on disk, so the upstream rename no longer breaks a + project (#405).** `bmad-build-auto` is preferred and a marker-complete `bmad-dev-auto` is + accepted, so `validate`/`run`/`sweep`/`resume` pass on either era. The forwarding shim + upstream left behind is refused by a new `skills.base-shim` check — its migration prompt is + interactive and would HALT an unattended session with nothing written to disk. The ok line + and `validate --json` name the primitive that actually resolved, and worktree isolation + copies the new skill directory. `[dev] skill` stays `bmad-dev-auto`: it is the adapter + discriminator, not the invoked name. + +- **Every session prompt spells the resolved primitive (#405).** Dev, review, repair, restore, + stories dispatch and all three sweep bundle legs hardcoded `/bmad-dev-auto`, which + post-rename dispatches the shim. The name is resolved per skill tree, so a run mixing + `.claude/skills` and `.agents/skills` gets the right one per role, and the no-spec fallback + result marker is read under both the `bmad-build-auto-result-*` and legacy prefixes. + +- **`--dry-run` says when its preview is not runnable (#405).** `run`, `sweep` and stories dry + runs returned before their own skill preflight, so a broken install still got a + plausible-looking schedule — and post-rename the previewed `/bmad-dev-auto` is a _valid_ + command that would HALT on the shim. The preflight failures now print to stderr under a "NOT + runnable as-is" banner. Exit code stays 0 and stdout is untouched: a dry run is a diagnostic, + and rc 0 has always meant "the preview rendered", not "the project is ready". + +- **The skills preflight no longer gates a triage-only CLI's skill tree (#405).** Every + `skills.*` check asks a dev-primitive question — which primitive resolves, whether the three + review hunters it invokes inline are installed, whether a renderer stub's script unit is + whole — yet the tree list was built from all three adapter roles. So a `[adapter.triage]` of + `gemini` beside a claude dev/review pair demanded the whole BMad Method module in + `.agents/skills`, a tree whose only prompt (`/bmad-loop-sweep`) ships in this wheel and is + laid down by `bmad-loop init`. The over-breadth is pre-existing and was already a hard block — + 0.9.0 emitted `skills.base-missing` as a problem over the same over-broad tree list and its run + preflight refused on it — but the new renderer checks route through the same list, so 0.9.1 + added new ways for one to fire. The probe — and `validate`'s own copy of it, which had the same defect twice + — is now scoped to the dev and review roles, the same set `Engine._worktree_profiles` + provisions into a worktree, and the `skills.base` and `skills.stories-dispatch` ok lines + report that narrower `trees` list. Single-CLI projects and dev/review splits across two CLIs + are unaffected. + +- **A worktree that could not be given its upstream skills now pauses instead of stalling + (#405).** `provision_worktree` copies the BMad Method skills from the main repo behind a + containment guard, and skipped anything resolving outside the repo — which is exactly what a + skill tree symlinked to a shared machine-wide BMad install does. The run-start preflight stats + through that symlink and passes, so an isolated run dispatched into a worktree holding none of + its skills and every session stalled on `Unknown command` having written nothing. Provisioning + now reports the skills it could not deliver through the existing `worktree-seed-skipped` + journal channel, and the engine escalates and pauses before dispatch, naming them — the same + environment-fault treatment the renderer surface already got, since the seed reads the same + repo for every story. Unlike the renderer legs this carries no _renderer_-era condition: a + missing skill stalls a session whether its `SKILL.md` renders or is inline. It is scoped to the + primitive era the run actually dispatches, though — the skills list it reads names both eras + because copying one that isn't there is free, but a leftover `bmad-dev-auto` shim no prompt + spells is not a stall to pause over. The copy itself now merges per _file_, so a worktree that + already holds part of a skill directory is completed rather than skipped whole, and the + completeness check walks the repo's skill directory exactly the way the copy walks it, + reporting every file the seed could not deliver, naming the file rather than the directory + whenever the directory itself arrived (a wholly-absent skill still reports the coarse rel). A + fixed list of required files would have covered three of the thirteen a real `bmad-build-auto` + install carries — and would go stale the next time upstream renames a step file. Containment + is checked per file too, so one skill file, or one whole sub-directory, symlinked to a shared + install outside the repo is refused rather than read through, and reported rather than + silently missing. A skill tree that is committed (symlink and all) + is checked out into the worktree normally and is unaffected. Pre-existing since 0.7.0, which + is where the containment-guarded seed loop first shipped; found while reviewing this release. + +- **A renderer stub that cannot compose a prompt fails the preflight (#405).** Three new checks + block `validate`/`run`/`sweep`/`resume`: `skills.dev-renderer`, when the resolved `SKILL.md` + is the new renderer stub (BMAD-METHOD#2601) but its script unit is not whole — + `_bmad/scripts/render_skill.py` or the `_bmad/scripts/config_utils.py` it imports at module + scope; `skills.dev-renderer-config`, when a stub resolved but `_bmad/config.toml` — the + renderer's one required config layer — is absent; and `skills.dev-renderer-sources`, when the + skill's own render sources are short: no `workflow.md` for the renderer to compose from, or a + `[[bmad-snapshot:…]]` token naming something the renderer will not load as a source. Every + route ends in a + session that Stops having written no spec, and every one of them is a fact about the install + rather than the story, so every story after it does the same. A missing config exits `HALT:`; + a missing entry document or snapshot target does too. A missing script or sibling loses even + that line — `uv` fails before the renderer runs, or `ModuleNotFoundError` fires above its own + guard — leaving only the stub's own instruction to report the output and halt. The sibling is + required only when the installed `render_skill.py` actually imports it, and the source check + asks what the install itself declares — save the `workflow.md` entry name, which upstream + hardcodes too — so a later renderer that inlines the helper or + reorganizes its step files is not refused. They block rather than warn because only a green is + untrustworthy (uv on PATH is never probed) — a red is conclusive. The config check is emitted + once per project and only when a stub actually resolved, so a pre-renderer install stays + silent, and `--dry-run` names all three under its "NOT runnable as-is" banner. A fourth check, + `skills.customize-legacy`, is a warning: it fires when a tree resolved to `bmad-build-auto` + while an override still sits at `_bmad/custom/bmad-dev-auto[.user].toml`, where it no longer + applies — the session still runs, just unstyled. + +- **Worktree isolation carries the `_bmad/` config surface (#405).** The renderer-era primitive + is handed the worktree as its project root and hard-fails when that root has no `_bmad/` — + there is no walk-up — so on a project that gitignores it (most do, this one included) every + isolated session HALTed with nothing written. `provision_worktree` now merge-copies the repo's + `_bmad/` per file, copy-when-absent, so a checkout that commits it keeps every tracked file and + only the gitignored layers are filled in. `_bmad/scripts/` is seeded whole rather than curated + because `render_skill.py` bare-imports its sibling `config_utils`, and a seed that comes up + short — the realistic trigger is a symlinked `_bmad/`, how a shared BMad install is wired — + pauses the run _when the resolved dev primitive is a renderer stub_. Every story would drive + the same incomplete seed into the same result-less Stop, so it escalates once with the + worktree left mounted for inspection, rather than dispatching the whole backlog and reporting + `0 done`. `_bmad/config.toml` is checked the same way and named separately in the escalation: + a repo that centralises only that file behind a symlink seeds a complete `_bmad/scripts/` and + no config at all, which was silent from every angle — the repo-side preflight follows the + symlink and passes. On a pre-BMAD-METHOD#2601 inline `SKILL.md` nothing reads either path, + so the short seed costs the run nothing and stays an ordinary journaled report. Neither + sentinel can be forged: a `worktree_seed` entry spelling one is dropped rather than reported, + so a checkout that commits its whole `_bmad/` is never paused on a healthy worktree. + `_bmad/render/` is never seeded and is git-excluded inside the worktree, so the renderer's + in-session rewrite of it cannot be swept into a story commit; `init` now gitignores it as + well, which is the only protection under the default `isolation = "none"`. + +- **Worktree provisioning survives a filesystem it cannot fully read (#405).** The `_bmad/` seed + and the detector that checks it now share the skill seed's walk: `rglob` does not descend a + symlinked _sub_-directory and the detector mirrored the same `rglob`, so a symlinked + `_bmad/scripts/lib` one level in was seeded as nothing and reported as complete — the + under-seed and the check that should have caught it were wrong together. Pointing inside the + repo it is now seeded; pointing outside it is still dropped by the containment guard, but the + gate can see it. + That walk descends symlinks, so it carries a branch-local cycle guard — without one, + `_bmad/scripts/loop -> ..` is copied at every depth until the kernel's `ELOOP` stops it, a risk + the old `rglob` never ran because it refused the descent to begin with. Per file, a source the + walk cannot see inside, an occupied destination and a failed read or write are each a skip the + skills gate then names — a source that could never be delivered at all, a dangling link or a + FIFO, is still dropped by both halves in silence, and `_bmad/`'s own report stays the two + renderer sentinels — rather than an exception out of the seed: `provision_worktree` runs inside + `Engine._run_isolated` with no `try` around it, so one unreadable skill file ended the whole run + with a crash dump where a named, resumable escalation belonged. A dangling _destination_ symlink + now counts as occupied: writing through one landed the bytes at the link's target and left the + gate reading green through the now-live link. Every probe the skills and `_bmad/` seeds make is + now total — and, the part a `try` alone does not buy, reads the same on every supported + interpreter. `is_file()`/`exists()`/`is_dir()` raise on an unsearchable parent through 3.13 and + answer `False` on 3.14, where `False` is the silent drop and not the cautious reading, so a + `False` from `is_dir()` is re-asked of `stat()`, which still raises there. Two faults reach that + split: a directory readable but _not searchable_ (mode `0o444`, where listing succeeds and + stat-ing each child does not) and an unreadable skill tree one level above the walk's own root. + What the report can say degrades with what the filesystem gives up: a directory that cannot be + listed is named once, and one that can be listed but not stat'd through is named file by file. + The eager copies are unchanged and can still raise — user-authored `worktree_seed`, the + adapter-default and plugin seeds beside it, and the per-CLI hook-config write. + +- **A seed the worktree never got is now reported, and the `_bmad/` shield writes one exclude line, + not two (#405).** The two explicit seed loops drop an entry with a bare `continue` when the + resolve-and-contain guard refuses it, so a config the repo carries as a symlink _out_ of itself — + a dotfile-managed `.claude/settings.json`, a shared MCP config — delivered nothing and said + nothing: `worktree-seed-skipped` reports only the opposite case, an entry whose destination + already exists. Provisioning's result is now re-probed against the repo and the drops journaled + as `worktree-seed-dropped`, asking the two trees on disk rather than the loops' bookkeeping, so a + drop by any future path is caught the same way. Journaled and never escalated, unlike the + `_bmad/` and skills gates beside it: those name files the orchestrator dispatches or the renderer + HALTs on, while a seed entry is arbitrary user config whose canonical trigger is an ordinary + working setup, so pausing would refuse every run of such a project over a guard doing its job. A + broken link stays silent in both halves, as it already does for the skills gate; a FIFO does + not — these loops gate on `exists()`, so the copy reaches it and raises. The eager copies are + still unchanged: a copy that _fails_ raises rather than landing here. + `/_bmad/render/` goes into `.git/info/exclude` only when the blanket `/_bmad` this release also + adds is not — that file lives under the git common dir a linked worktree shares with the main + checkout and nothing here ever prunes it, and `/_bmad` prunes the directory before git descends, + so a sibling line would be permanent state in the operator's own repo that git never consults. `worktree-opened` is + journaled as soon as the worktree is mounted rather than after every provisioning gate has + passed, so the escalations that leave a half-provisioned worktree mounted for inspection now say + where it is. + +- **Seeding never writes _through_ a symlink, and a hook config is no longer its own alibi + (#405).** Both explicit seed loops resolved the destination and then used the resolved path for + the occupancy probe, the `mkdir` and the copy. `Path.resolve()` is non-strict, so a dangling + link answers for its target: `exists()` reported the slot free and the copy landed at the link's + target instead — an unrequested path inside the worktree that the exclude does not name and the + unit's `git add -A` would stage into the story branch. `git worktree add` produces exactly that + state for a tracked symlink whose target is untracked, so it arrives on a project's _first_ + story. Both loops now compare the raw destination against the resolved one and refuse when they + differ, each with its own guard — they share no code — and strictly _after_ the + copy-when-absent skip arm, since a live symlinked destination differs too and hoisting the + refusal would turn that ordinary no-op into a silent drop. What is refused is named afterwards + by `worktree-seed-dropped`, as every other refusal in those loops already is. The drop report in + turn stops trusting the destination for the one rel that cannot answer for itself: the per-CLI + hook step writes `profile.hooks.config_path` after both seed loops, so a config the loops + dropped is answered for by the hook's own bytes — and for gemini, copilot and antigravity that + path is the profile's _only_ default seed, so the false green was the gate's entire answer. + Those rels are now asked whether the source escapes the repo, the destination is a symlink, or + the destination escapes the worktree, replacing the existence probe rather than adding to it so + a destination that trips two of them is still named once. A live symlinked destination for a + hook config is consequently named here as well as in `worktree-seed-skipped`; both facts are + true at once and this channel journals rather than pauses. Two destination-containment guards — + the `_bmad/` merge's and the skill merge's — also gain their first tests: their existing + siblings arm the source guard and the occupancy check instead, so a live symlinked _directory_ + left them unwitnessed. + +- **`isolation = "worktree"` is refused when `repo_root` is overridden (#405).** A project whose + `_bmad/bmm/config.yaml` sets `repo_root` decouples the git root from the project dir — the + documented monorepo knob — but worktree provisioning reads `repo_root` for every surface it + seeds off disk (the upstream skill trees, `_bmad/` and its `_bmad/custom/` overrides, every + `seed_files`/`seed_globs` entry) and bakes the absolute hook-relay path from it into the + worktree's hook config, while `init`, `validate` and the run + preflight write and probe them under `project`. `load_paths` requires + `project/_bmad/bmm/config.yaml`, so that surface is under `project` by definition and + `repo_root/_bmad/` generally does not exist: the preflight approved a surface the isolated run + never received, every seed-completeness gate above went inert rather than firing, and each + story ended as a result-less Stop with nothing naming the cause. `validate` now reports the + pair as a `policy.isolation-repo-root` problem, and `run`, `sweep`, `resume`, `resolve`'s + re-arm and the auto-triggered child sweep refuse to start, naming both remediations — drop the + `repo_root` override, or set `isolation = "none"`. The child sweep raises rather than declining + quietly, so a mid-run flip is journaled as `sweep-auto-failed` instead of being recorded as a + sweep that ran. The dry-run banner lists the refusal ahead of the skill + ones it aborts before, and the TUI's pre-launch guard mirrors it — ahead of its own clean-tree + gate, as the CLI does — so the operator gets a toast rather than a pane that dies. + Pre-existing since worktree isolation shipped. Plumbing + `project` through provisioning instead — which would make the combination work rather than + refuse it — stays open as #414. + +- **`validate` warns when the renderer's output is already committed (#405).** The two shields + 0.9.1 adds — the `_bmad/render/` line `init` writes into `.gitignore`, and the `/_bmad/render/` + line provisioning writes into the repo's local git exclude — only help going forward, because a + tracked path ignores both entirely. A project that committed `_bmad/render/` earlier keeps + churning it into every story commit, gaining a snapshot directory per machine, per checkout path + and per upstream renderer bump, since the path is keyed on a hash of the absolute project root + plus a generation hash over the renderer, its sources and the resolved config. A new + `git.render-tracked` warning names the one-time + `git rm -r --cached _bmad/render`. A warning and not a problem — nothing about a tracked + `_bmad/render/` stops a session — and outside a git repo it stays quiet rather than fabricating + an ok (#409). All three sites now spell the path from one `install.RENDER_DIR_REL`, so the + probe cannot drift away from the shields it reports on. + +- **`verify`'s emptiness probes no longer read their answer out of git's stderr (#405).** + `worktree_clean` — and `path_tracked`, added earlier in this release — tested stdout and stderr + merged, but `ls-files` and + `status` exit 0 while still writing to stderr — a `core.fsmonitor` hook that cannot exec, an + unknown `core.fsyncMethod` — and that chatter is indistinguishable from an index entry or a + porcelain line. Both answers were silently inverted: the new `git.render-tracked` check would + have told operators to `git rm -r --cached` a path that was never committed, and a checkout with + nothing in it was reported dirty, blocking `run`, `sweep` and `validate` outright. Both now read + stdout alone; the error paths keep the merge, where stderr is the informative half. + +- **An undecodable `policy.toml` or `config.yaml` is reported, not a traceback (#405).** + `read_text` raises `UnicodeDecodeError` on a file saved as UTF-16 or latin-1, and that is a + `ValueError`, not an `OSError` — so it escaped every `except (PolicyError, OSError)` handler in + the codebase, which are precisely the ones whose job is to degrade to defaults. The TUI could + not open its dashboard, and `_configure_mux` runs before argument dispatch on every command, so + the CLI died at startup on the raw codec message its catch-all prints, naming no file, instead + of any of its named findings. `policy.load` and `bmadconfig.load_paths` now convert it to their + own typed errors, which fixes every caller at once; `validate` reports the file by name. + +- **Deferred review findings are harvested out of the spec's frontmatter (#405).** + BMAD-METHOD#2640 moved `defer`-triaged findings from `deferred-work.md` into the spec's own + `deferred:` list, silently starving the sweep pipeline. A successful dev or review session + now files each finding into the ledger with its `location` and `severity`, journals + `spec-deferrals-harvested`, and dedups on a fingerprint of the summary and location so a + _fixable_ retry, a resume replay or a second review pass never re-files one — including after a + sweep has already marked it done. (A NON-fixable retry re-files deliberately: the bullet below + reverts its entry along with the attempt, and the next attempt harvests it fresh.) Malformed items do not block their well-formed siblings: the loss is + journaled and filed as one aggregated ledger entry. The spec's frontmatter is never rewritten, + and the harvest keys on content rather than the installed skill name, so it works on both eras. + The review prompt no longer also asks the session to file the finding itself: that produced two + entries per finding which could never dedup, since an agent-written one carries neither the + fingerprinted `origin:` nor a `source_spec:` line. It stays neutral rather than banning ledger + edits outright — on a pre-BMAD-METHOD#2640 skill there is no frontmatter to harvest and the + session's own append is the finding's only record — and still forbids rewriting existing entries. + +- **A harvested deferral is reverted when its attempt rolls back (#405).** The harvest keys on + the spec's status and runs before the artifact gate, so a session that finalized its spec and + then failed a non-fixable check (a `baseline_revision` mismatch is the canonical trigger) left + its ledger entry behind, describing code the rollback had just discarded. The reset alone does + not remove it: the ledger sits under a protected artifact folder, which `_safe_reset`'s `keep` + shields from the untracked-file cleanup. The dev phase now snapshots the ledger before the + harvest and restores it around the rollback — unlinking the file when the harvest created it, + and on the stop-and-wait path too, which raises. Lossless, because the spec's `deferred:` + frontmatter is never mutated and the next attempt re-harvests from it. The snapshot is taken + ahead of every engine-side ledger write in that window rather than immediately ahead of the + harvest, so one growing inside the status reconcile or the state sync is covered by + construction. And it is persisted with the attempt rather than held in a local, so a + host death between the harvest and the rollback no longer loses it: the replayed attempt + writes back the same bytes the live path would have. That replaces a presence bit which could + only say whether a ledger existed at the attempt baseline — enough to delete one the harvest + had created, never enough to restore one that was already there, so a pre-existing _untracked_ + or _gitignored_ ledger kept the dead attempt's finding for good. Nothing else reverts one: + `reset --hard` skips ignored paths, there is no `git clean`, and the artifacts dir is + `keep`-shielded besides. The restore never unlinks a ledger git tracks — that one is the reset's + to restore, and deleting it would hand the next attempt's `git add -A` a deletion to commit. The + snapshot is armed at the attempt's pre-harvest save and cleared once that attempt's decision is + acted on, so a finished story carries no copy of the ledger in `state.json`. The clear on the + PROCEED path persists immediately, because a hard kill there resumes straight into the review + leg and would otherwise strand that copy on a task that is already terminal; the retry site + deliberately does not, since the same kill replays that attempt and it still wants the + snapshot — except when that retry pauses, which the next bullet covers. A defer still + keeps its harvested entries: on the in-place path `_stash_deferred_artifacts` has already moved + the spec out of the artifacts dir, so the ledger entry is the finding's only surviving record. + +- **The pre-harvest snapshot is spent by a stop-and-wait pause instead of riding it (#405).** + `rollback_on_failure = off` is the default and does not roll back: it hands the tree to a human + and raises. The window that opens is not a crash window but a human-scale one — the operator + reads the notice, inspects the tree, and may well append to the deferred-work ledger before + typing `resume`. The armed snapshot used to survive that wait in `state.json`, and the resume, + which replays the same attempt, wrote those stale bytes back over whatever the operator had + done: an unlink when no ledger existed at the attempt baseline, and otherwise an overwrite, + which has no tracked-file guard, so a committed ledger was rewritten too. Nothing spent the arm + either, so it happened again on every subsequent resume. The pause leg now disarms in the same + `finally` that restores, strictly after it, and persists that immediately — the point is the + value the resume reads off disk. The replayed attempt then re-arms from the tree as the + operator left it, so its own harvest has something to revert to if it fails in turn. Replays + that never reached the arm re-arm the same way rather than proceeding blind, which retires the + `ledger-snapshot-missing` journal line for that case. + +- **A harvest is reverted even when the ledger lives outside the repo (#405).** + `implementation_artifacts` may be configured out of tree — `ProjectPaths.rebased` keeps such a + dir put on purpose, as shared rather than per-checkout. The restore classified any ledger + outside the workspace as git's and left it alone, which is backwards: `reset --hard` runs in + the workspace and cannot reach a path outside it, so it restored nothing and the harvest's file + is a creation of the orchestrator's own. Every harvest revert on such a project was therefore a + silent no-op, and the entry outlived the code it described — open in the shared ledger, and + swept later as if that work still existed. It now reads as "not git's", on both retry legs and + under either isolation mode. The containment test is re-asked with both sides resolved before + that answer authorizes a delete, since a workspace root reached through a symlink is not + lexically a prefix of the ledger's real path; an `OSError` there degrades to leaving the file + alone, like every other probe in that `finally`. + +- **The harvest's own ledger write is no longer the session's proof of work (#405).** The harvest + runs four statements above the dev artifact gate, and that gate deliberately does not exclude + the ledger — a story whose whole authorized scope is ledger reconciliation has to register as + real work. So the gate could not tell the orchestrator's write from the session's: a session + that finalized its spec, changed no code and recorded one `deferred:` finding proceeded to done + on the strength of the line the engine had just written for it. The gate now excludes the ledger + on exactly the attempts whose harvest filed into it, keyed on a flag persisted with the attempt + rather than re-derived — a crash replay re-runs the harvest, which dedupes against the dead + attempt's entries and so reports filing nothing while those entries are still in the tree. + Narrow by construction: a session's own ledger edit still counts as work on every attempt that + did not harvest, and the flag is cleared per attempt, so a later attempt is never charged for an + earlier one's harvest. All three dev legs pass it through — sprint story, sweep bundle, and + folder+id stories. The bundle leg is the costlier one, since an attempt let through there marks + real ids `done` and `open_ids` re-bundles only `open` entries. + +- **…and a session's own ledger edit on that same attempt still is (#405).** That exclusion is + path-granular — it hides the whole ledger relpath, not the harvest's lines — so on an attempt + where both authors wrote, the session's edit went out with the orchestrator's. A + ledger-reconciliation story that also recorded one `deferred:` finding therefore read as "no + changes since baseline", and the non-fixable retry that follows _pauses the whole run_ under the + default `scm.rollback_on_failure = false`. The gate now stands down whenever the ledger had + already moved off the dev phase's baseline by the time that attempt's pre-harvest snapshot was + armed: its diff is then provably not the harvest's alone, and the gate judges the tree in full — + which is the conservative direction, since it sees more of the diff rather than less. The + baseline is a digest persisted beside `baseline_commit`, and persisting it is the point: a + resumed dev phase deliberately does not re-capture its baselines, so a phase-local reference + would be absent for every attempt after a crash-resume and the mask would survive there. + Introduced earlier in this same release. + +- **…and both answers now span a fixable retry (#405).** The two bullets above scope every piece + of harvest provenance to one attempt, and the dev phase's ledger baseline to the whole phase. + Those coincide on every run where at most one attempt's work sits above `baseline_commit` — and + a _fixable_ retry is the one construct that breaks that: it keeps the tree, and the harvest's + ledger line with it, on purpose. So the repair session was judged against a baseline that + predated the surviving line: the flag was cleared, the exclusion stood down, and an attempt that + reverted the offending change and wrote nothing else passed the proof-of-work gate on the + orchestrator's own write — with the `[verify] commands` that had failed now passing precisely + because the change was reverted. (Only for a tracked or untracked-but-not-ignored ledger; an + ignored one is invisible to the gate either way.) Both records now survive the continuation, and + the ledger baseline is re-based to match — forward onto the kept tree when the fixable retry is + taken, back onto the restored snapshot if that chain later rolls back. Latching the _answer_ + instead is the obvious pairing and re-opens the bullet above: the mask is path-granular, so a + stale one hides a repair session whose honest work genuinely is a ledger entry, and that pauses + the run. Same continuation, same reason, for the record of what the harvest intended to file: a + fixable retry followed by a stalled attempt used to defer with an empty record while the entry + it named sat in a worktree about to be dropped. + +- **The pre-harvest snapshot is scoped to the retry chain, not to one attempt (#405).** The + rollback it feeds always resets to the dev phase's `baseline_commit`, so after a fixable + retry — which keeps its tree, and its harvest, on purpose — a later non-fixable failure + discarded several attempts' code while restoring only the last one's ledger. The kept + attempt's finding was written straight back over the reset that removed the code it named: + open in the ledger, and driven by the next sweep as if that work existed. A fixable retry now + neither re-arms the snapshot nor spends it, so the chain holds the one taken before its first + attempt and the revert reaches as far as the reset does. Both halves are required; either + alone leaves the entry standing. The rolling ledger reference is re-based back to the restored + bytes at the same point, or the next attempt reads the restore itself as somebody else's write + and stands the proof-of-work exclusion down. On the `rollback_on_failure = off` default the + tree is kept and the restore now reverts the whole chain's ledger writes rather than the last + attempt's — matching the `git reset --hard ` the pause notice tells the + operator to run. The disarm that spent the snapshot on both retry legs moves onto the + non-fixable one, where it belongs; the stale comment claiming a fixable retry needed it, and + claiming the pause leg could not reach it, is replaced (the pause has disarmed in its own + `finally` since `0a8088f`, and a fresh attempt re-arms unconditionally, so neither clause held). + Not closed here: `_finish_inflight`'s restart arm resets to the same baseline with no ledger + restore at all, and cannot simply gain one — it also handles the resolved-escalation re-drive, + which preserves the artifacts folder through the reset on purpose. + +- **A sweep bundle's ledger closes are withheld until its attempt is accepted (#405).** The + orchestrator marks a bundle's deferred-work ids `done` itself, and did so above the artifact + gate — so an attempt that finalized its spec and then failed a non-fixable check was discarded + with the ledger already claiming its work resolved. A surviving close is worse than a surviving + finding: `open_ids` only ever re-bundles open entries, and the one reopen — `mark_open`, which + undoes only a close its own caller wrote — is not a general one, so no + later sweep looks at that id again and the work is silently lost rather than mis-recorded. On + the retry leg the dev phase's ledger snapshot reverted it; on the DEFER terminus nothing in the + dev phase could, because `_defer` takes its own snapshot after the close and writes it back + over its own reset — deliberately, so review-found deferrals survive a defer. The close now + runs below the gate, for a PROCEED decision only, which also excludes a CRITICAL escalation + (it preempts even a passing outcome) and a failing `[verify] commands` run. What remains after + acceptance is covered by the bullet below rather than by a revert. One deliberate consequence: that write no longer counts toward the gate's + proof-of-work diff, so a bundle session which changed no code now fails the gate instead of + passing on the orchestrator's own bookkeeping. + +- **A review-leg defer takes the bundle's ledger closes back (#405).** The bullet above stops a + DISCARDED dev attempt from closing anything; a bundle accepted at dev that then defers in + review is a different shape. There the closes were correct when written and are required — + `verify_review_bundle` gates on them — and only the later failure makes them wrong. `_defer` + then writes its own post-close ledger snapshot back over the `reset --hard` that had reverted + them, deliberately, since a harvested finding's ledger entry is its only surviving record once + the spec is stashed. But the restore replays the whole file, so the closes rode it too and + named code that no longer existed — on every route into a post-acceptance defer: review budget + exhausted, the repair phase exhausted after a clean review, the budget rescue's own verify + failing, `review.enabled = false` failing at its gate, and a blocking workflow deferring from + `post_dev_phase` / `post_review_result` / `pre_commit_gate`. The defer now re-opens the ids it + closed itself. `deferredwork.mark_open` is the undo of one specific `mark_done`, not a general + reopen: it refuses any entry that is not closed carrying exactly the resolution note the caller + wrote, so a close from an earlier sweep, from the legacy path where the session edits the + ledger, or from a human is never revoked — and the round trip restores what the close changed, + character for character. Unchanged on the + stop-and-wait path (`rollback_on_failure = off` keeps the tree, so there is nothing to undo) + and under worktree isolation (those closes live in the unit's own worktree, dropped unmerged). + +- **A deferred worktree unit carries its harvested findings out (#405).** Under + `isolation = "worktree"` the harvest writes the unit worktree's ledger, and a defer drops that + worktree unmerged — so no ledger a sweep reads ever saw the findings. (Nothing was destroyed: + `keep_failed` defaults on and the forensic patch takes untracked files, so they survive in the + kept worktree.) `_defer` now re-files what the final attempt's harvest intended to file into the + main checkout's ledger and commits that path alone — leaving it dirty would escalate the next + story's merge. `append_entry` dedups on `origin:` + `source_spec:`, so an entry already open + there is not duplicated. Only the final attempt's record carries: it is cleared at each attempt's + start, so an attempt whose session never completed cannot re-file the findings its predecessor's + rollback removed. The gap pre-dates this release; the harvest added a systematic producer of + such entries. + +- **…and so does a unit that LANDS (#405).** A landing unit looked untouched — its ledger edit + merges with the branch. Not when the ledger is gitignored: `finalize_commit` stages with + `git add -A`, which skips such a path in silence, so the entry never rode the branch and the + merge brings nothing over. (This repo's own default puts the ledger under a gitignored artifacts dir.) + The worktree then goes unconditionally, and the DONE leg takes no forensic patch, so the finding + simply disappears. That leg now runs the same carry — **after** the merge, since carrying first + files a fresh id where the merge would have delivered the entry for dedup. A tracked ledger + rides the branch as before and every row dedups, so nothing is committed. Not on the escalation + legs: those keep the branch for a human to merge, and that merge brings the entry with it. + +- **A sweep bundle's ledger closes reach the main checkout too (#405).** The same mechanism one + layer up. The close writes the unit worktree's ledger, and with a gitignored ledger seeded into + the worktree (`scm.worktree_seed`) the flip dies unmerged — the entries stay open, `open_ids` + re-bundles them, and every later sweep re-triages and re-drives work that is already done. The + DONE leg now re-applies the closes the bundle intended; `mark_done` is idempotent, so a tracked + ledger whose flip merged normally is a no-op. What is recorded is what the close **intended**, + not what it flipped: the close runs twice on a landing bundle — after dev, and again from the + review gate's idempotent reclose — and the second run finds every id already done, so a record + of the flips is empty on exactly the run that needs it. A DEFER still carries findings only and + never closes: it discarded the code they claim to resolve. Not closed here: an _unseeded_ + gitignored ledger is absent from the worktree entirely, so `verify_review_bundle` never sees the + ids done and the bundle defers rather than landing — loud, where this one was silent. + +- **…and a carry lost to a crash mid-landing replays on resume (#405).** `Phase.DONE` is + persisted before the merge runs, the merge ends by removing the worktree, and only then does + the carry fire — so a host death in that window destroyed the worktree's copy and skipped the + carry, and resume never looked again (it skips terminal tasks). The sibling DEFER leg carries + **before** its terminal save, which is why only DONE had the hole. Nothing was unrecoverable: + both payloads are already in `state.json`, so only the replay was missing. Resume now re-runs + an unlatched carry, once, guarded by a persisted latch — the writes are only partly idempotent, + since `append_entry` dedups against **open** entries and a later close would turn a blind + replay into a duplicate. Skipped when the worktree is still mounted (the branch may never have + landed, and the human's merge would bring the entry itself), on the in-place path, and on any + phase but DONE. The sweep half is the sharper one: a lost close leaves ids open, the + suppression filter does not cover DONE, and every later sweep re-drives already-merged work + into a non-fixable retry that pauses the run. + +- **An artifacts dir whose name holds `[` or `]` no longer misdirects the two paths above + (#405, #423).** Git reads a positional operand as a pathspec, so such a path was a glob that + also matched its neighbours. The harvest revert's tracked-probe answered "git owns it" off a + neighbour and skipped its unlink, leaving the entry open for a later sweep; and the carry's + commit swept a neighbour's uncommitted edit in under the story's name — or, for the gitignored + ledger it is built to hit, staged nothing and reported success, since `git add` refuses an + explicitly-named ignored path but skips a globbed one. Both now name the path literally. + +- **Three small contracts brought back into line (#405).** A ledger entry filed with no file:line + now carries `location: n/a` rather than no `location:` line: `deferred-work-format.md` has + always specified that value, and of the fields an entry is created with, `severity:` is the + only one it calls optional — so the orchestrator's own writer was the one producer disagreeing + with the format the sweep skill reads. The harvest's 200-character clamp can land on a word + break; the value is now stripped where it is produced rather than where it is written, so the + `origin:` dedup key and the `location:` line stay derived from the same string. Entries already + on disk without the line stay valid — read an absent `location:` as `n/a`. A plugin workflow's + completion-marker filename is now resolved on the skill tree of the adapter its own `role` runs + on, not always the dev one, so a run mixing `.claude/skills` and `.agents/skills` at different + upstream eras no longer spells a review session's marker after a primitive only the dev tree + carries. That one is the two halves agreeing rather than a repair: read-back was never at risk, + because both prefixes are matched unconditionally. And `diagnose` now pseudonymizes the `spec` + journal field, which is the customer's feature name and was shipping verbatim — the egress + backstop could only ever rescue the ones that happened to embed the story key. The value is + reduced to its basename first: the harvest kinds journal a bare filename while the reconcile + kinds journal an absolute path, so aliasing it raw would have given one spec two aliases in a + dump and parked a home path in the local `--legend` file. + ## [0.9.0] — 2026-07-21 ### Added @@ -2488,6 +3012,7 @@ enforced in CI. resumable on-disk run state. [Unreleased]: https://github.com/bmad-code-org/bmad-loop/compare/v0.9.0...HEAD +[0.9.1]: https://github.com/bmad-code-org/bmad-loop/releases/tag/v0.9.1 [0.9.0]: https://github.com/bmad-code-org/bmad-loop/releases/tag/v0.9.0 [0.8.1]: https://github.com/bmad-code-org/bmad-loop/releases/tag/v0.8.1 [0.8.0]: https://github.com/bmad-code-org/bmad-loop/releases/tag/v0.8.0 diff --git a/README.md b/README.md index 3b5dedb3..73885a3e 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`) 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. +- 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 dev primitive installed — `bmad-build-auto`, or a complete `bmad-dev-auto` on pre-rename releases (bmad-loop resolves whichever is on disk and drives it under that name, so either era works with no config edit; the bare forwarding shim the rename leaves behind is refused as incomplete — it carries none of the step files or `customize.toml` a real install has — because a session dispatched into it stalls on an interactive migration gate) — plus 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 @@ -165,7 +165,7 @@ Press **`g`** to edit `.bmad-loop/policy.toml` in a form grouped by section — bmad-loop drives the same dev → verify → review → commit loop from **either** of two story sources — chosen per project, or per run: - **Sprint mode (default).** Stories come from `sprint-status.yaml` (written by `bmad-sprint-planning` from your PRD/epics). The loop walks the board by `ready-for-dev` status, keyed by story ref (`1-2-account-mgmt`). This is what the rest of this README describes. -- **Stories mode (opt-in).** Stories come from a typed `stories.yaml` — the **Story Breakdown** output of `bmad-spec`, a fixed-name sibling of `SPEC.md` in the epic's spec folder. The loop dispatches each entry by **folder + id** (`/bmad-dev-auto Spec folder: . Story id: .`); the dev skill creates-or-resumes the story spec at `/stories/-.md`, and the orchestrator reads that id-keyed path back deterministically — no shared board to line-edit, no mtime-scan of result artifacts. +- **Stories mode (opt-in).** Stories come from a typed `stories.yaml` — the **Story Breakdown** output of `bmad-spec`, a fixed-name sibling of `SPEC.md` in the epic's spec folder. The loop dispatches each entry by **folder + id** (`/bmad-build-auto Spec folder: . Story id: .`, spelled with whichever primitive name resolves on disk); the dev skill creates-or-resumes the story spec at `/stories/-.md`, and the orchestrator reads that id-keyed path back deterministically — no shared board to line-edit, no mtime-scan of result artifacts. Turn it on per project with `[stories] source = "stories"` + `spec_folder = ""`, or per run with `bmad-loop run --spec ` (which overrides the policy). Everything downstream — dev/verify/review/commit, worktree isolation, gates, crash resume, the TUI — is identical; only the story source and the per-story controls below differ. @@ -191,7 +191,7 @@ A story may set both checkpoints (it pauses twice); `gates.mode` pauses stack on `bmad-loop run --dry-run --spec ` prints the linear schedule (list order, checkpoint markers, live on-disk state); `bmad-loop status` shows the same stories board. -> Stories mode requires a `bmad-dev-auto` new enough to support folder+id dispatch; the run preflight (and `bmad-loop validate`) checks for it and tells you to update the BMAD module if it is missing. Sprint mode is unaffected and remains the default indefinitely. +> Stories mode requires a dev primitive new enough to support folder+id dispatch; the run preflight (and `bmad-loop validate`) checks for it and tells you to update the BMAD module if it is missing. Sprint mode is unaffected and remains the default indefinitely. ## How a story flows @@ -200,14 +200,14 @@ A story may set both checkpoints (it pauses twice); `gates.mode` pauses stack on ```text sprint-status.yaml: 1-2-account-mgmt: ready-for-dev │ - ├─ DEV tmux window: claude "/bmad-dev-auto 1-2-account-mgmt" - │ bmad-dev-auto: plans a 1.5–4k-token spec, auto-approves it, + ├─ DEV tmux window: claude "/bmad-build-auto 1-2-account-mgmt" + │ bmad-build-auto: plans a 1.5–4k-token spec, auto-approves it, │ implements, self-reviews inline (parallel review layers), │ commits, finalizes spec → done … Stop hook signals the orchestrator ├─ VERIFY spec exists · status done · baseline matches · diff non-empty │ · run [verify].commands (pytest, ruff…) — a broken build never │ reaches review; a failure spawns a fix session fed the output - ├─ REVIEW fresh window: claude "/bmad-dev-auto " — re-invoking on a + ├─ REVIEW fresh window: claude "/bmad-build-auto " — re-invoking on a │ (gated) done spec runs a fresh independent step-04 review pass (parallel review │ layers → triage → auto-apply patches → ledger → defer ambiguity → │ commit). Gated on the skill's `followup_review_recommended` flag @@ -243,7 +243,7 @@ bmad-loop sweep [--no-prompt] [--decisions-only] [--max-bundles N] [--repeat] [- │ terminal (build / close / keep-open per option, with a │ recommendation); answers land in the ledger as `decision:` │ lines. Unattended runs skip this and leave decisions open. - └─ BUNDLES each bundle runs the normal pipeline: bmad-dev-auto (on the bundle + └─ BUNDLES each bundle runs the normal pipeline: bmad-build-auto (on the bundle spec, then re-invoked on the done spec for review) → verify commands → commit. The review gate also checks every bundle entry is `status: done` in the ledger. @@ -273,19 +273,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 review-layer skills its step-04 invokes inline are prerequisites installed by the BMad Method (bmm) module: +The orchestrator drives the upstream dev primitive — `bmad-build-auto`, or `bmad-dev-auto` on pre-rename releases — 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; the dev primitive 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` | 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` | installs the orchestrator tool from Git, then runs `bmad-loop init` + `validate` | +| Skill | Role | +| --------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `bmad-build-auto` | unattended implementation + follow-up review (**upstream** — bmm prerequisite, not bundled; named `bmad-dev-auto` before the rename, and either era is driven) | +| `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` | installs the orchestrator tool from Git, then runs `bmad-loop init` + `validate` | -`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. +`bmad-loop validate` preflights the dev primitive (always — reporting which name it resolved) 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 dev primitive 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: @@ -327,7 +327,7 @@ Your `.bmad-loop/policy.toml` is left untouched on upgrade — new keys are opti To remove bmad-loop from a project, see [Uninstalling](docs/setup-guide.md#uninstalling) — it reverses what `init` laid down (state, skills, hooks, gitignore) and uninstalls the tool. -**Via the BMAD-method installer.** The installer copies the bundled `bmad-loop-*` skills into your project (but not the orchestrator tool), alongside the upstream `bmad-dev-auto` skill the orchestrator drives. Finish setup with `/bmad-loop-setup`, which installs the tool from Git, asks which coding CLIs to drive, registers their hooks (`init` skips the already-present skills), and runs the preflight: +**Via the BMAD-method installer.** The installer copies the bundled `bmad-loop-*` skills into your project (but not the orchestrator tool), alongside the upstream dev primitive the orchestrator drives. Finish setup with `/bmad-loop-setup`, which installs the tool from Git, asks which coding CLIs to drive, registers their hooks (`init` skips the already-present skills), and runs the preflight: ```bash claude "/bmad-loop-setup accept all defaults" @@ -335,7 +335,7 @@ claude "/bmad-loop-setup accept all defaults" See **[docs/setup-guide.md](docs/setup-guide.md)** for the full walkthrough — choosing CLIs, installing the tool and TUI together or separately, and initializing codex/gemini. -The bundled skills must be installed together with the upstream `bmad-dev-auto` dev session: `bmad-loop-sweep` owns the canonical `deferred-work-format.md` that the orchestrator normalizes the ledger to, and `bmad-dev-auto` appends the flat deferred-work entries it normalizes. The `bmad-dev-auto` skill is driven unmodified, so its own `customize.toml` applies as-is; it needs no merge — it is consumed directly from the bmm module. There is no review fork to keep in sync: review is just a re-invocation of `bmad-dev-auto` on the done spec. +The bundled skills must be installed together with the upstream dev primitive: `bmad-loop-sweep` owns the canonical `deferred-work-format.md` that the orchestrator normalizes the ledger to, and the primitive appends the flat deferred-work entries it normalizes. The primitive is driven unmodified, so its own `customize.toml` applies as-is; it needs no merge — it is consumed directly from the bmm module. There is no review fork to keep in sync: review is just a re-invocation of the primitive on the done spec. ## Policy (`.bmad-loop/policy.toml`) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 2c4407d8..cc652938 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -40,7 +40,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Spec + implementation (dev stage) -- Drives the upstream `bmad-dev-auto` skill (unmodified) in a fresh tmux session: it plans a 1.5–4k-token spec, auto-approves it, implements, and self-finalizes the spec; the orchestrator syncs `sprint-status` and synthesizes `result.json` from the spec the skill leaves on disk. +- Drives the upstream dev primitive (unmodified) in a fresh tmux session — `bmad-build-auto`, or `bmad-dev-auto` on pre-rename releases, resolved from disk per skill tree and invoked under the name it resolves to, so either era runs with no config edit (the legacy name is accepted only when marker-complete, which refuses the forwarding shim the rename leaves behind): it plans a 1.5–4k-token spec, auto-approves it, implements, and self-finalizes the spec; the orchestrator syncs `sprint-status` and synthesizes `result.json` from the spec the skill leaves on disk. - Deterministic missing-marker catch + repair (#276): the review HALT intermittently finalizes a spec's frontmatter to a terminal `status:` without appending the `## Auto Run Result` section the harvest scan keys on, which once livelocked a finished story to a DEFER-drop (#224). Four harness-side mechanisms make catch + fix deterministic without ever mutating the launch frontmatter (load-bearing skill routing): a **launch-state content-hash snapshot** the fallback refuses to synthesize from when the candidate is byte-identical (a `done` spec merely re-opened for review — in every mode, killing the dead-window false positive); **mid-session status-transition observation** (a heartbeat-tick sighting of the spec moving off its launch status proves a later terminal frontmatter is this session's write, so synthesis needs one sighting, not two); **artifact repair** that appends the owed marker onto the spec on synthesis so it re-enters the normal scan; and — gated by `limits.dev_contract_nudge` (default on) — one **targeted contract nudge** per session asking the skill to append the section itself. Frontmatter synthesis stays the backstop for a session that never complies. - Spec-only contract between stages — review consumes the frozen spec, not the dev session's context. @@ -122,9 +122,9 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Stories mode (folder+id dispatch) - Opt-in second story source (`[stories] source = "stories"` + `spec_folder`, or `bmad-loop run --spec `): drives the same loop off a typed `stories.yaml` (a `bmad-spec` Story Breakdown, sibling of `SPEC.md`) instead of `sprint-status.yaml`. -- Dispatches each entry by **folder + id** (`/bmad-dev-auto Spec folder: . Story id: .`); the story spec lands at `/stories/-.md` and is read back by a deterministic id-keyed glob — no shared board to line-edit, no result-artifact mtime-scan. +- Dispatches each entry by **folder + id** (`/bmad-build-auto Spec folder: . Story id: .`, spelled with whichever primitive name resolves on disk); the story spec lands at `/stories/-.md` and is read back by a deterministic id-keyed glob — no shared board to line-edit, no result-artifact mtime-scan. - Strictly linear schedule (list order, no `depends_on`); `done` skipped, non-terminal statuses resumed on re-dispatch, `blocked`/sentinel/ambiguous stops the run for resolve. `bmad-loop run --dry-run --spec ` and `bmad-loop status` print the schedule/board (id · live disk state · checkpoint markers · title). -- Preflight content-probe: stories mode requires a `bmad-dev-auto` new enough for folder+id dispatch, or the run aborts with remediation. Sprint mode keeps working with any installed version. +- Preflight content-probe: stories mode requires a dev primitive new enough for folder+id dispatch, or the run aborts with remediation. Sprint mode keeps working with any installed version. - Sentinel recovery: a pre-planning-halt sentinel spec (`-unresolved.md` / `-ambiguous.md`) is auto-deleted with a preserved copy under the run dir on re-arm, matching the contract's delete-to-retry. ### Gates & human checkpoints @@ -182,8 +182,8 @@ 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 `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. +- `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`. +- 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. ### Command reference diff --git a/src/bmad_loop/checks.py b/src/bmad_loop/checks.py index bcc8a5e1..6e7d3356 100644 --- a/src/bmad_loop/checks.py +++ b/src/bmad_loop/checks.py @@ -68,6 +68,8 @@ "skills.base", "skills.base-missing", "skills.base-incomplete", + "skills.base-shim", + "skills.customize-legacy", "skills.review-layer-missing", "skills.review-layer-unresolved", "skills.review-layers-empty", diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 1afd313f..67fe42cb 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -230,12 +230,19 @@ def cmd_validate(args: argparse.Namespace) -> int: except policy_mod.PolicyError as e: report.fail("policy", str(e)) + # Built exactly the way run/sweep's real preflight builds it, so validate's + # verdict and their abort cannot disagree. Deliberately NOT `[p.skill_tree for p + # in profiles]`: that carries triage's tree, and every skills check below asks a + # dev-primitive question. The `pol is not None` guard is load-bearing — + # `resolved` never raises, so the except above fires only on a policy that failed + # to load, and an unguarded call would crash validate instead of reporting the + # parse failure. + dev_trees = _skill_trees(project, pol) if pol is not None else [] + stories_on, spec_folder = _stories_mode(args, pol) if paths: if stories_on: - _validate_stories_queue( - project, paths, spec_folder, [p.skill_tree for p in profiles], report - ) + _validate_stories_queue(project, paths, spec_folder, dev_trees, report) _validate_closes_deferred(paths, report, spec_folder=spec_folder) else: _validate_closes_deferred(paths, report) @@ -363,17 +370,34 @@ def cmd_validate(args: argparse.Namespace) -> int: {"role": role, "model": cfg.model, "profile": prof.name}, ) - base_findings = install.missing_base_skills(project, [p.skill_tree for p in profiles]) + base_findings = install.missing_base_skills(project, dev_trees) # 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): + # + # Gate on `dev_trees`, not `profiles`: policy never validates an adapter name + # (the first test is `get_profile`), so `[adapter] name = "nosuchcli"` beside a + # loadable `[adapter.triage]` leaves `profiles` non-empty while nothing dev-side + # resolved — and the ok line would then be a green sentence assembled from an + # empty probe. Can only tighten: `dev_trees` truthy implies `profiles` truthy. + if dev_trees and not any(f.severity == "problem" for f in base_findings): + # Name the primitive that actually resolved, not a hardcoded era: on an + # upgraded project this is the operator's confirmation that the rename was + # picked up (and, across trees, that both picked up the same one). + resolved = list( + dict.fromkeys( + name + for tree in dict.fromkeys(dev_trees) + if (name := install.resolve_dev_primitive(project, tree)) is not None + ) + ) report.ok( "skills.base", - "upstream skills present (bmad-dev-auto + review layers)", - {"trees": list(dict.fromkeys(p.skill_tree for p in profiles))}, + f"upstream skills present ({' + '.join(resolved)} + review layers)", + {"trees": list(dict.fromkeys(dev_trees)), "dev_primitive": resolved}, ) report.extend(base_findings) + report.extend(install.dev_primitive_warnings(project, dev_trees)) if getattr(args, "json", False): # getattr, not args.json: cmd_validate is called directly by tests (and by @@ -491,27 +515,108 @@ def _mux_set(project: Path, args: argparse.Namespace) -> int: return 0 +def _skill_trees(project: Path, pol) -> list[str]: + """The skill trees this run's dev-primitive adapters read, one per distinct name. + + Shared by the real preflight and `cmd_validate` so the two cannot drift: + validate's verdict has to key on exactly what makes run abort. Profiles that + fail to load are skipped rather than raising — an unknown adapter name is the + policy loader's problem, not the skill probe's. + + Scoped to :data:`install.DEV_PRIMITIVE_ROLES`, not :data:`ROLES`: every skill + these trees are asked about is one only a dev or review session dispatches, and + triage's whole prompt surface ships in this wheel. It is also the set + `WorktreeFlow.worktree_profiles` provisions, so what is gated and what is carried + into a worktree stay one decision. + + Deliberately does NOT consult `review.enabled`. Disabling review does not retire + the review ADAPTER: a plugin workflow may declare `role = "review"` and dispatch + on `adapters["review"]` with review disabled, and `worktree_profiles` drives + per-CLI Stop-signal hook registration as well as seeding — a worktree + provisioned without the review profile has no completion signal for those + sessions and stalls rather than merely missing a skill. See #424 for the narrow + residue that IS real.""" + from .adapters.profile import ProfileError, get_profile + + trees = [] + for name in dict.fromkeys( + pol.adapter.resolved(role).name for role in install.DEV_PRIMITIVE_ROLES + ): + try: + trees.append(get_profile(name, project).skill_tree) + except ProfileError: + continue + return trees + + +def _dev_skill_for_role(pol, project: Path, role: str) -> str: + """The dev-primitive skill name ``role``'s adapter would invoke, for dry-run + previews. Mirrors `_require_base_skills`' profile→skill_tree lookup so the + preview and the real dispatch (``Engine._dev_skill``) resolve identically — + a pre-rename project previews ``/bmad-dev-auto``, a post-rename one + ``/bmad-build-auto``. An unloadable profile falls back to the legacy name; + the run itself would fail preflight before ever dispatching.""" + from .adapters.profile import ProfileError, get_profile + + try: + tree = get_profile(pol.adapter.resolved(role).name, project).skill_tree + except ProfileError: + tree = None + return install.dev_primitive_or_default(project, tree) + + +def _warn_preflight_would_abort(project: Path, pol, *, require_stories: bool = False) -> None: + """Dry-run honesty banner: say so when the real command would refuse to run. + + ``--dry-run`` returns before `_require_base_skills` (cmd_run/cmd_sweep), so a + project whose skills are broken still gets a plausible-looking preview. Since + the upstream rename that preview is actively misleading rather than merely + incomplete: the forwarding shim IS a valid slash command, so a previewed + ``/bmad-dev-auto`` reads fine and would HALT an unattended session on the + shim's interactive migration gate. + + Reads the same finding list `_require_base_skills` gates on, so the two cannot + disagree about what "runnable" means. + + The exit code deliberately stays 0. A dry-run is a diagnostic — refusing to + print the schedule would withhold the very thing the operator asked for, and + every existing caller reads rc 0 as "the preview rendered", not as "the + project is ready". The banner goes to stderr so stdout stays the preview.""" + trees = _skill_trees(project, pol) + problems = [ + p.message + for p in install.missing_base_skills(project, trees) + + (install.missing_stories_support(project, trees) if require_stories else []) + if p.severity == "problem" + ] + if not problems: + return + print( + "note: this preview is NOT runnable as-is — the real command aborts at preflight:", + file=sys.stderr, + ) + for problem in problems: + print(f" FAIL: {problem}", file=sys.stderr) + print("run `bmad-loop validate` for details", file=sys.stderr) + + def _require_base_skills(project: Path, pol, *, require_stories: bool = False) -> bool: - """Preflight the upstream skills the orchestrator drives (bmad-dev-auto + the - review layers it invokes inline). + """Preflight the upstream skills the orchestrator drives (the dev primitive — + bmad-build-auto, or a complete pre-rename bmad-dev-auto — plus the review layers + 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. + A post-rename install left with only the forwarding shim fails here too: the + shim's interactive migration gate would HALT the session with nothing written. - ``require_stories`` additionally content-probes bmad-dev-auto for folder+id - dispatch — stories mode needs a newer skill than sprint mode, so an older - install must fail loudly here rather than HALT `no stories.yaml`-style at + ``require_stories`` additionally content-probes the resolved primitive for + folder+id dispatch — stories mode needs a newer skill than sprint mode, so an + older install must fail loudly here rather than HALT `no stories.yaml`-style at dispatch time.""" - from .adapters.profile import ProfileError, get_profile - - skill_trees = [] - for name in dict.fromkeys(pol.adapter.resolved(role).name for role in ROLES): - try: - skill_trees.append(get_profile(name, project).skill_tree) - except ProfileError: - continue + skill_trees = _skill_trees(project, pol) findings = install.missing_base_skills(project, skill_trees) if require_stories: findings += install.missing_stories_support(project, skill_trees) @@ -582,9 +687,9 @@ def _validate_stories_queue( ) -> None: """Stories-mode counterpart of ``cmd_validate``'s sprint-status gate: validate the ``stories.yaml`` manifest + ``SPEC.md`` and confirm the installed - ``bmad-dev-auto`` carries the folder+id dispatch flow stories mode needs (an - older skill would HALT at dispatch). Appends findings to ``report`` in place; - the probe carries its own remediation text ("update the bmm module").""" + dev primitive carries the folder+id dispatch flow stories mode needs (an older + skill would HALT at dispatch). Appends findings to ``report`` in place; the + probe carries its own remediation text ("update the bmm module").""" folder = stories_mod.resolve_spec_folder(paths.project, spec_folder) problem = _validate_stories_folder(paths, spec_folder) if problem: @@ -605,10 +710,20 @@ def _validate_stories_queue( ) stories_probs = install.missing_stories_support(project, skill_trees) if skill_trees and not stories_probs: + # The total form, not `resolve_dev_primitive`: this ok line only renders when + # the probe passed, and the probe ran against exactly this name — so an + # unresolvable tree that somehow satisfied it is still reported as what was + # actually read. + probed = list( + dict.fromkeys( + install.dev_primitive_or_default(project, tree) + for tree in dict.fromkeys(skill_trees) + ) + ) report.ok( "skills.stories-dispatch", - "bmad-dev-auto supports folder+id dispatch (stories mode)", - {"trees": list(dict.fromkeys(skill_trees))}, + f"{' + '.join(probed)} supports folder+id dispatch (stories mode)", + {"trees": list(dict.fromkeys(skill_trees)), "dev_primitive": probed}, ) report.extend(stories_probs) @@ -976,6 +1091,8 @@ def _dry_run( if stories_on: return _dry_run_stories(paths, pol, args, spec_folder) + _warn_preflight_would_abort(paths.project, pol) + def render(role: str, prompt: str) -> str: return _render_invocation(pol, paths.project, role, prompt) @@ -992,10 +1109,12 @@ def render(role: str, prompt: str) -> str: print("no actionable stories") return 0 print(f"would process {len(queue)} stories (gates={pol.gates.mode}):") + dev_skill = _dev_skill_for_role(pol, paths.project, "dev") + review_skill = _dev_skill_for_role(pol, paths.project, "review") for story in queue: print(f"\n {story.key} (epic {story.epic}, status {story.status})") - print(f" dev: {render('dev', f'/bmad-dev-auto {story.key}')}") - print(f" review: {render('review', '/bmad-dev-auto ')}") + print(f" dev: {render('dev', f'/{dev_skill} {story.key}')}") + print(f" review: {render('review', f'/{review_skill} ')}") print(f" env: BMAD_LOOP_MODE=1 BMAD_LOOP_STORY_KEY={story.key}") return 0 @@ -1016,6 +1135,7 @@ def _dry_run_stories( ) -> int: """Print the linear stories-mode schedule (list order, checkpoints, live on-disk state) — no topo waves, one story per line, spawns nothing.""" + _warn_preflight_would_abort(paths.project, pol, require_stories=True) folder = stories_mod.resolve_spec_folder(paths.project, spec_folder) # The real dispatch always uses the project-relative folder (the engine # relativizes it); render the identical string here so dry-run and run agree. @@ -1034,13 +1154,14 @@ def _dry_run_stories( f"(gates={pol.gates.mode}){spec_ok}" ) print("linear schedule (list order — no depends_on, strictly serial):") + dev_skill = _dev_skill_for_role(pol, paths.project, "dev") for row in rows: print(f"\n {row.position}. {row.id} ({row.label}){_checkpoint_badge(row)} {row.title}") # A spec_checkpoint story whose plan is not yet on disk dispatches leg 1 # (Halt after planning + BMAD_LOOP_PLAN_HALT); mirror the real dispatch's # markers so dry-run does not under-report what run would emit. plan_halt = stories_mod.is_plan_halt_leg(row.spec_checkpoint, row.state) - dispatch = f"/bmad-dev-auto Spec folder: {rel}. Story id: {row.id}." + dispatch = f"/{dev_skill} Spec folder: {rel}. Story id: {row.id}." if plan_halt: dispatch += " Halt after planning." print(f" dev: {_render_invocation(pol, paths.project, 'dev', dispatch)}") @@ -1161,6 +1282,9 @@ def cmd_sweep(args: argparse.Namespace) -> int: def _sweep_dry_run(paths: bmadconfig.ProjectPaths, pol) -> int: + # Before the no-ledger early return below: a broken install is worth saying so + # about whether or not there is anything to sweep. + _warn_preflight_would_abort(paths.project, pol) ledger = paths.deferred_work if not ledger.is_file(): print(f"no deferred-work ledger at {ledger}") diff --git a/src/bmad_loop/devcontract.py b/src/bmad_loop/devcontract.py index 0d64ad99..c956f0c1 100644 --- a/src/bmad_loop/devcontract.py +++ b/src/bmad_loop/devcontract.py @@ -92,9 +92,13 @@ ) # The skill's no-spec fallback artifact (HALT when {spec_file} is unknown/missing): -# `{implementation_artifacts}/bmad-dev-auto-result-.md`. It -# carries a terminal frontmatter `status:` but no `## Auto Run Result` heading. -FALLBACK_RESULT_PREFIX = "bmad-dev-auto-result-" +# `{implementation_artifacts}/-result-.md`. It carries a +# terminal frontmatter `status:` but no `## Auto Run Result` heading. BOTH eras are +# listed and matched unconditionally: the artifact is named after whichever skill +# wrote it, and a run can read an artifact left by the other era (a resume across an +# upstream upgrade, or a project mid-migration), so this must never be keyed on the +# skill name resolved on disk today. +FALLBACK_RESULT_PREFIXES = ("bmad-build-auto-result-", "bmad-dev-auto-result-") @dataclass(frozen=True) @@ -356,8 +360,9 @@ def is_result_artifact(path: Path, *, since_ns: int) -> bool: scanning a directory shared with every concurrent run (#261). Qualifies when the file was modified at/after ``since_ns`` (the session-launch - floor) AND either is the by-name no-spec fallback (`bmad-dev-auto-result-*`, - which carries no heading by design) or carries a real, non-fenced + floor) AND either is the by-name no-spec fallback (`bmad-build-auto-result-*`, + or `bmad-dev-auto-result-*` pre-rename — it carries no heading by design, and + both spellings match unconditionally) or carries a real, non-fenced ``## Auto Run Result`` heading — a fence-quoted example must not qualify the spec (#52). An unreadable/undecodable candidate cannot be SHOWN to carry a terminal section, so it does not qualify (UnicodeDecodeError is a ValueError, @@ -367,7 +372,7 @@ def is_result_artifact(path: Path, *, since_ns: int) -> bool: return False except OSError: return False - if path.name.startswith(FALLBACK_RESULT_PREFIX): + if path.name.startswith(FALLBACK_RESULT_PREFIXES): return True try: text = path.read_text(encoding="utf-8") @@ -420,7 +425,7 @@ def is_frontmatter_candidate(path: Path, *, since_ns: int) -> bool: territory — and has a terminal frontmatter ``status:`` (``done``, ``blocked``, or ``awaiting-operator``). Any unreadable/undecodable read degrades to False, never an exception.""" - if path.name.startswith(FALLBACK_RESULT_PREFIX): + if path.name.startswith(FALLBACK_RESULT_PREFIXES): return False try: if path.stat().st_mtime_ns < since_ns: diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 67508046..42602bff 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -33,6 +33,7 @@ preference_escalations, review_retry_or_exhaust, ) +from .install import dev_primitive_or_default from .journal import Journal, save_state from .model import ( PAUSE_EPIC_BOUNDARY, @@ -150,8 +151,9 @@ def render(self) -> str: # *infer* the completion-marker convention, and one that finishes its work but # never writes the marker leaves the orchestrator waiting (a completion-signal # livelock, bounded only by session_timeout_min). The orchestrator's adapter -# discovers the marker by its `bmad-dev-auto-result-` filename prefix and -# mtime, not by exact name. +# discovers the marker by its `-result-` filename prefix and +# mtime, not by exact name (devcontract.FALLBACK_RESULT_PREFIXES accepts both +# the pre- and post-rename spellings, so either resolution reads back). WORKFLOW_COMPLETION_CONTRACT = """ ## Completion signal (required) @@ -269,6 +271,13 @@ def __init__( # best-effort hint (None when the estimate could not be computed). self._graceful_stopped = False self._graceful_remaining: int | None = None + # dev-primitive name resolved from disk, memoized per (workspace project + # root, skill tree) — see _dev_skill. By tree because one run can mix them + # (dev=claude reads .claude/skills, review=codex reads .agents/skills), with + # None for an adapter that carries no profile at all; by project root + # because under isolation each unit resolves against its OWN worktree and + # one Engine drives every unit of a run. + self._dev_skill_cache: dict[tuple[Path, str | None], str] = {} # Per-unit worktree isolation + integration flow (issue #244 F-3/F-9a). # Built from narrow deps + engine callbacks; the same-name Engine._* worktree # methods below delegate to it. `emit` is late-bound (a lambda, not the bound @@ -1291,20 +1300,22 @@ def _record_dev_spec(self, task: StoryTask, result_json: dict | None) -> None: (``devcontract.synthesize_result``). No-op once set or when the claimed spec is absent. - The skill's no-spec fallback artifact (``bmad-dev-auto-result-*``, written - when intent was too unclear to even CREATE a spec) is refused: it is not the - story's spec, so every consumer here misreads it. ``rearm_escalation`` would - flip frontmatter on a marker no re-drive reads, ``_reset_spec_for_repair`` - would re-open it as if it were the frozen intent contract, and the repair - prompt would point the session at it — which the #261 read-back then pins to, - polling a stale marker while the re-drive's real spec goes unread.""" + The skill's no-spec fallback artifact (``bmad-build-auto-result-*``, or + ``bmad-dev-auto-result-*`` pre-rename — ``FALLBACK_RESULT_PREFIXES`` matches + both; written when intent was too unclear to even CREATE a spec) is refused: + it is not the story's spec, so every consumer here misreads it. + ``rearm_escalation`` would flip frontmatter on a marker no re-drive reads, + ``_reset_spec_for_repair`` would re-open it as if it were the frozen intent + contract, and the repair prompt would point the session at it — which the + #261 read-back then pins to, polling a stale marker while the re-drive's + real spec goes unread.""" if task.spec_file: return spec_file = (result_json or {}).get("spec_file") if not spec_file: return spec_path = verify.resolve_spec_path(str(spec_file), self.workspace.paths) - if spec_path.name.startswith(devcontract.FALLBACK_RESULT_PREFIX): + if spec_path.name.startswith(devcontract.FALLBACK_RESULT_PREFIXES): return if spec_path.is_file(): task.spec_file = str(spec_path) @@ -1995,6 +2006,39 @@ def _generic_dev(self) -> bool: a future alternative dev skill can re-introduce the legacy branch.""" return self.policy.dev.skill == "bmad-dev-auto" + def _dev_skill(self, role: str = "dev") -> str: + """The dev-primitive skill NAME to spell in ``role``'s session prompt. + + Upstream renamed the primitive ``bmad-dev-auto`` → ``bmad-build-auto`` + (BMAD-METHOD #2651), so the invoked name is resolved from what is + actually on disk rather than hardcoded: a target project can be on + either era. This is NOT ``policy.dev.skill`` — that stays the adapter + discriminator ``_generic_dev`` reads; only the spelled name moves. + + Resolution is per skill tree because one run can mix them (dev=claude → + ``.claude/skills``, review=codex → ``.agents/skills``), and memoized + because every prompt build would otherwise re-stat the tree. An adapter + with no ``profile`` (test fakes) yields tree None, which + ``dev_primitive_or_default`` maps to the legacy name. + + Resolved against the WORKSPACE, never the main checkout: the session runs + with ``cwd=self.workspace.root``, so the tree deciding whether the spelled + name is a command at all is the worktree's. The two agree on a freshly + provisioned unit — ``provision_worktree`` copies the primitive in from the + main repo — but NOT on resume: ``reopen_unit`` re-mounts an existing + worktree without re-provisioning it, so a main checkout upgraded across the + pause would resolve the new name into a worktree carrying only the legacy + one, and the session HALTs on an unknown command having written nothing. + The cache is keyed on the workspace root for the same reason: one Engine + drives every unit of a run, so a resumed unit's worktree must not answer + for the fresh worktrees mounted after it.""" + adapter = self.adapters.get(role) + tree = getattr(getattr(adapter, "profile", None), "skill_tree", None) + project = self.workspace.paths.project + if (project, tree) not in self._dev_skill_cache: + self._dev_skill_cache[project, tree] = dev_primitive_or_default(project, tree) + return self._dev_skill_cache[project, tree] + def _operator_park_enabled(self) -> bool: """Whether a dev session may park a story at ``awaiting-operator`` (#335). @@ -2646,7 +2690,7 @@ def _review_prompt(self, task: StoryTask) -> str: # ledger is append-only for sessions — new findings are fine, existing # entries are orchestrator-owned. return ( - f"/bmad-dev-auto {task.spec_file} — If this review defers new " + f"/{self._dev_skill('review')} {task.spec_file} — If this review defers new " f"findings, append them to the deferred-work ledger as NEW entries " f"only; do NOT modify, re-open, or rewrite existing ledger entries — " f"the orchestrator owns their status and resolution." @@ -2791,8 +2835,18 @@ def _run_session( # the same implementation-artifacts dir the dev adapter already # searches — correct in place and under worktree isolation alike, # because spec.cwd is self.workspace.root either way. + # This is the PRODUCER of the marker name. ``role``, not the default: + # a workflow declares its own role (WORKFLOW_ROLES = dev | review) and + # runs on THAT adapter, whose skill tree can be a different one at a + # different era — dev=claude on .claude/skills post-rename, review=codex + # on .agents/skills pre-rename. Resolving off the dev tree would name + # the session a primitive its own tree does not carry. The reader still + # accepts the legacy spelling because devcontract.FALLBACK_RESULT_PREFIXES + # matches both eras unconditionally, so discovery survives either + # resolution — the two halves agreeing, not a broken read-back. marker_path = ( - self.workspace.paths.implementation_artifacts / f"bmad-dev-auto-result-{task_id}.md" + self.workspace.paths.implementation_artifacts + / f"{self._dev_skill(role)}-result-{task_id}.md" ) prompt += WORKFLOW_COMPLETION_CONTRACT.format(marker_path=marker_path) spec = SessionSpec( @@ -3079,16 +3133,16 @@ def _generic_dev_prompt(self, task: StoryTask, feedback: Path | None) -> str: if feedback is None: if task.restore_patch and task.spec_file: return ( - f"/bmad-dev-auto Resume review of the in-review spec at " + f"/{self._dev_skill()} Resume review of the in-review spec at " f"`{task.spec_file}`. The attempted change was restored onto " f"the working tree after an intent-gap resolution; review it " f"against the amended spec." ) + self._operator_park_instruction() - return f"/bmad-dev-auto {task.story_key}" + self._operator_park_instruction() + return f"/{self._dev_skill()} {task.story_key}" + self._operator_park_instruction() self._reset_spec_for_repair(task) spec_ref = task.spec_file or task.story_key return ( - f"/bmad-dev-auto Resume the autonomous dev session on the in-progress " + f"/{self._dev_skill()} Resume the autonomous dev session on the in-progress " f"spec at `{spec_ref}`. The previous session's work failed deterministic " f"verification; repair the working tree so verification passes without " f"changing the spec's frozen intent contract. Verification evidence is " diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index 0a548fe7..87398d4f 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -55,26 +55,62 @@ ANTIGRAVITY_HOOK_GROUP = "bmad-loop" # The bmad-loop-* skills bundled in the wheel (bmad_loop/data/skills/) that -# `bmad-loop init` lays down. The inner dev primitive `bmad-dev-auto` is upstream -# (not bundled here): the orchestrator drives it as an already-installed skill. +# `bmad-loop init` lays down. The inner dev primitive (`bmad-build-auto`, formerly +# `bmad-dev-auto`) is upstream (not bundled here): the orchestrator drives it as an +# already-installed skill. MODULE_SKILLS = ( "bmad-loop-resolve", "bmad-loop-sweep", "bmad-loop-setup", ) +# The inner dev primitive, in both upstream eras. BMAD-METHOD PR #2651 (shipped in +# bmad-method 6.10.1-next.33) renamed `bmad-dev-auto` -> `bmad-build-auto` and left a +# forwarding SHIM behind under the old name: a lone SKILL.md whose customization +# migration gate is INTERACTIVE, so an unattended session that dispatches to it can +# HALT having written nothing to disk — no spec, no result artifact, nothing the +# post-session verification can read. The orchestrator therefore never accepts the +# shim: it resolves the primitive on disk (:func:`resolve_dev_primitive`) and fails +# the preflight when only the shim is installed. +# +# The shim carries no step files and no customize.toml, so DEV_PRIMITIVE_MARKERS — +# which already pinned "a real, complete install" — doubles as the shim detector. +# 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. +DEV_PRIMITIVE_NEW = "bmad-build-auto" +DEV_PRIMITIVE_LEGACY = "bmad-dev-auto" +DEV_PRIMITIVE_MARKERS = ("step-04-review.md", "customize.toml") + +# The adapter roles whose skill tree is asked for the dev primitive and for the +# review layers that primitive invokes inline — i.e. every question this module's +# skill checks ask. Triage is deliberately absent: its only prompt is +# `/bmad-loop-sweep`, which is in MODULE_SKILLS and is laid into that tree by +# `bmad-loop init`, so a triage-only CLI never needs one byte of the bmm module. +# Gating it anyway makes `[adapter.triage] name = "gemini"` under a claude dev/review +# pair demand the whole module in `.agents/skills` — a hard preflight FAIL over a +# tree no session ever dispatches one of these skills into. +# +# This must stay the same set `WorktreeFlow.worktree_profiles` provisions, which +# reads this constant for exactly that reason. A tree gated here that no worktree +# carries refuses runs over a skill no session will ever read; a tree provisioned but +# not gated ships a session into the `Unknown command` stall the preflight exists to +# catch. Neither has a defensible reading, so the two move together or not at all. +DEV_PRIMITIVE_ROLES: tuple[str, ...] = ("dev", "review") + # Upstream skills the orchestrator invokes but does NOT bundle in the wheel — the -# 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, 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. +# BMad Method (bmm) module installs them. Each must exist in every ``trees`` entry — +# callers pass the DEV_PRIMITIVE_ROLES trees — and carry its marker files (a +# half-installed or pre-automation skill is caught by the `bmad-loop validate` +# preflight). `{skill: (marker-rel-path, ...)}`. +# - the dev primitive — always required, and never substitutable. Its entry is +# keyed on the LEGACY name because this map doubles as the "lay down a pre-rename +# install" catalog; missing_base_skills does NOT walk it for the primitive, it +# resolves the installed name per tree first (resolve_dev_primitive). # - 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 +# reviewers are derived per tree from the dev primitive 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 @@ -83,7 +119,7 @@ # 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"), + DEV_PRIMITIVE_LEGACY: DEV_PRIMITIVE_MARKERS, "bmad-review-adversarial-general": (), "bmad-review-edge-case-hunter": (), } @@ -104,10 +140,18 @@ # 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: ()} +# BOTH primitive eras are listed: a worktree must carry whichever one the main +# checkout has, and copy-if-present makes naming both free. Adding the new name here +# is what keeps isolation working across the rename — provisioning unions this +# catalog with the resolved review layers, and the layers never name the primitive. +BASE_SKILLS = { + DEV_PRIMITIVE_NEW: DEV_PRIMITIVE_MARKERS, + **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: +# How the dev primitive 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 — @@ -128,17 +172,75 @@ # 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", -) + + +def _customize_overrides(skill: str) -> tuple[Path, ...]: + """Project overrides of ``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. + + Takes the RESOLVED primitive name and reads that pair ONLY — never both eras. + Upstream's own resolver keys on the skill directory, so on a renamed project the + legacy pair is simply not read at run time; merging it in here would make the + preflight resolve layers the session never applies, which is the exact + disagreement :func:`resolve_review_layers` exists to prevent. The orphaned file + is surfaced instead, as the ``skills.customize-legacy`` warning in + :func:`dev_primitive_warnings`. + """ + return ( + CUSTOMIZE_DIR / f"{skill}.toml", + CUSTOMIZE_DIR / f"{skill}.user.toml", + ) + + +def resolve_dev_primitive(project: Path, tree: str) -> str | None: + """The dev-primitive skill name to drive in ``tree``, or None when none is usable. + + Prefers :data:`DEV_PRIMITIVE_NEW`; falls back to :data:`DEV_PRIMITIVE_LEGACY` + only when that install is marker-complete, which is exactly what the post-rename + forwarding shim is not (see the constants block). None means "fail the preflight" + — never "drive the old name and hope". + + The new name needs only its SKILL.md to *resolve*: completeness is reported by + :func:`missing_base_skills` against the resolved dir. Requiring markers here + instead would make a truncated bmad-build-auto silently resolve to a legacy + install (or to the shim's failure message), hiding the real problem. + """ + if (project / tree / DEV_PRIMITIVE_NEW / "SKILL.md").is_file(): + return DEV_PRIMITIVE_NEW + legacy = project / tree / DEV_PRIMITIVE_LEGACY + if (legacy / "SKILL.md").is_file() and all( + (legacy / marker).is_file() for marker in DEV_PRIMITIVE_MARKERS + ): + return DEV_PRIMITIVE_LEGACY + return None + + +def _is_dev_primitive_shim(project: Path, tree: str) -> bool: + """True when ``tree`` holds a legacy-named skill that is only a forwarding shim + (SKILL.md present, at least one marker absent). Selects the failure *message* in + :func:`missing_base_skills`; it is never a resolution input.""" + legacy = project / tree / DEV_PRIMITIVE_LEGACY + if not (legacy / "SKILL.md").is_file(): + return False + return any(not (legacy / marker).is_file() for marker in DEV_PRIMITIVE_MARKERS) + + +def dev_primitive_or_default(project: Path, tree: str | None) -> str: + """Total form of :func:`resolve_dev_primitive` for callers that need a name. + + A prompt string — and a path this module has to probe *somewhere* to report on — + always has to name something, and the preflight has already refused the + unresolvable cases before any session is spawned. So an unresolvable tree (and a + None tree, which is what an adapter with no profile reports) falls back to the + legacy name rather than raising into prompt construction.""" + if tree is None: + return DEV_PRIMITIVE_LEGACY + return resolve_dev_primitive(project, tree) or DEV_PRIMITIVE_LEGACY class ReviewResolution(NamedTuple): - """Which skills the installed bmad-dev-auto's review step actually invokes. + """Which skills the installed dev primitive'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 — @@ -238,9 +340,14 @@ def _merge_layer_arrays(base: list[Any], override: list[Any]) -> list[Any]: return result -def _merged_review_layers(project: Path, tree: str) -> tuple[list[Any], tuple[str, ...]] | None: +def _merged_review_layers( + project: Path, tree: str, skill: str +) -> tuple[list[Any], tuple[str, ...]] | None: """The skill's shipped review layers with project overrides applied. + ``skill`` is the resolved dev-primitive name — the shipped config and its + overrides are read from the SAME era, because that is what the run does. + 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. @@ -249,12 +356,12 @@ def _merged_review_layers(project: Path, tree: str) -> tuple[list[Any], tuple[st 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") + data = _read_toml(project / tree / skill / "customize.toml") if data is None: return None layers = _layers_of(data) unreadable: list[str] = [] - for rel in _CUSTOMIZE_OVERRIDES: + for rel in _customize_overrides(skill): override = project / rel if not override.is_file(): continue @@ -275,7 +382,15 @@ def _layer_id(layer: dict[str, Any], index: int) -> str: 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 + The primitive's installed NAME is resolved from disk here rather than taken as + an argument (:func:`dev_primitive_or_default`): both call sites — the preflight + and worktree provisioning — hold exactly ``(project, tree)`` and would otherwise + each compute the same value to hand back. On a renamed project the legacy paths + are simply absent, so without this the resolution silently returns None and the + caller degrades to the static catalog — quietly seeding a worktree with the + wrong reviewers instead of the ones this project configured. + + Post-consolidation the primitive 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` @@ -292,7 +407,12 @@ def resolve_review_layers(project: Path, tree: str) -> ReviewResolution | None: the model in run context — undecidable here, so hard-requiring its skill would be a false FAIL. """ - merged = _merged_review_layers(project, tree) + # `primitive`, not `skill`: the layer loop below binds `skill` to each INVOKED + # review skill, and the step-04 fallback after it reads this name as a directory. + # Sharing one name is safe only while the layer branch returns first — a + # shadowing this function should not be one edit away from. + primitive = dev_primitive_or_default(project, tree) + merged = _merged_review_layers(project, tree, primitive) if merged is None: return None layers, unreadable = merged @@ -330,9 +450,7 @@ def resolve_review_layers(project: Path, tree: str) -> ReviewResolution | None: unreadable, ) try: - step04 = (project / tree / DEV_PRIMITIVE_SKILL / "step-04-review.md").read_text( - encoding="utf-8" - ) + step04 = (project / tree / primitive / "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 @@ -344,33 +462,39 @@ def resolve_review_layers(project: Path, tree: str) -> ReviewResolution | 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 +# Stories mode (folder+id dispatch, BMAD-METHOD #2549) needs a *newer* dev primitive # than sprint mode: one whose step-01 routes a spec-folder + story-id invocation. # File existence (missing_base_skills) can't tell the two skill versions apart, so # a content probe confirms the merged dispatch protocol is present. This literal is # stable prose in the merged step-01 ("this is a **folder+id dispatch**"). -STORIES_PROBE_SKILL = "bmad-dev-auto" +# STORIES_PROBE_SKILL names the FALLBACK era only — the probe runs against the skill +# resolve_dev_primitive picked for that tree, so a bmad-build-auto install is probed +# under its own name. +STORIES_PROBE_SKILL = DEV_PRIMITIVE_LEGACY STORIES_PROBE_FILE = "step-01-clarify-and-route.md" STORIES_PROBE_TEXT = "folder+id dispatch" def missing_stories_support(project: Path, trees: Sequence[str]) -> list[Finding]: - """Problems for stories mode's stricter bmad-dev-auto requirement. + """Problems for stories mode's stricter dev-primitive requirement. - Sprint mode drives any bmad-dev-auto; stories mode needs the folder+id - dispatch flow, which older skill versions lack. For each active CLI skill - tree, confirm ``bmad-dev-auto/step-01-clarify-and-route.md`` exists and - carries the dispatch-protocol marker. Returns one problem :class:`Finding` - per tree lacking it (empty = OK). Callers gate this on stories mode only — - sprint-mode runs must not require the newer skill. + Sprint mode drives any dev primitive; stories mode needs the folder+id + dispatch flow, which older skill versions lack. For every ``trees`` entry — + callers pass the :data:`DEV_PRIMITIVE_ROLES` trees, since only a dev or review + session ever dispatches one of these — confirm + ``/step-01-clarify-and-route.md`` exists and carries the + dispatch-protocol marker. Returns one problem :class:`Finding` per tree lacking + it (empty = OK). Callers gate this on stories mode only — sprint-mode runs must + not require the newer skill. The two failures are separate check ids because they are separate conditions with separate remediations: ``-missing`` is a half install (reinstall the module), ``-stale`` is an install that is simply too old (update it).""" problems: list[Finding] = [] for tree in dict.fromkeys(trees): - probe = project / tree / STORIES_PROBE_SKILL / STORIES_PROBE_FILE - detail = {"tree": tree, "skill": STORIES_PROBE_SKILL, "file": STORIES_PROBE_FILE} + skill = dev_primitive_or_default(project, tree) + probe = project / tree / skill / STORIES_PROBE_FILE + detail = {"tree": tree, "skill": skill, "file": STORIES_PROBE_FILE} try: text = probe.read_text(encoding="utf-8") except (OSError, UnicodeDecodeError): @@ -382,7 +506,7 @@ def missing_stories_support(project: Path, trees: Sequence[str]) -> list[Finding Finding( "skills.stories-dispatch-missing", "problem", - f"{tree}/{STORIES_PROBE_SKILL}/{STORIES_PROBE_FILE} not found — stories " + f"{tree}/{skill}/{STORIES_PROBE_FILE} not found — stories " f"mode needs folder+id dispatch; update the BMad Method (bmm) module", detail, ) @@ -393,9 +517,9 @@ def missing_stories_support(project: Path, trees: Sequence[str]) -> list[Finding Finding( "skills.stories-dispatch-stale", "problem", - f"{tree}/{STORIES_PROBE_SKILL} lacks folder+id dispatch (no " + f"{tree}/{skill} lacks folder+id dispatch (no " f"{STORIES_PROBE_TEXT!r} in {STORIES_PROBE_FILE}) — stories mode needs a " - f"newer bmad-dev-auto; update the bmm module", + f"newer {skill}; update the bmm module", {**detail, "marker": STORIES_PROBE_TEXT}, ) ) @@ -405,19 +529,36 @@ 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 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. + The dev primitive (bmad-build-auto, or a complete pre-rename 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 ``trees`` entry — + callers pass the :data:`DEV_PRIMITIVE_ROLES` trees, since only a dev or review + session ever dispatches one of these — 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 + The primitive is resolved per tree (:func:`resolve_dev_primitive`) before any + marker check, so the markers are asserted against the skill this run would + actually drive. That splits the failures three ways: + + - ``skills.base-incomplete`` — one resolved, but it is truncated. + - ``skills.base-shim`` — nothing resolved, yet a legacy-named SKILL.md is there. + - ``skills.base-missing`` — nothing at all under either name. + + A truncated *legacy* install is byte-for-byte the same shape as the shim (old + SKILL.md, absent markers), so it lands on ``base-shim`` rather than + ``base-incomplete``; nothing on disk can tell those two apart, so the message + names both causes and the single remediation they share. What the ids DO + separate is what a consumer can act on differently: resolved-but-truncated + (reinstall that skill) vs nothing-usable-resolved (update the module). + + The review skills are read from the installed primitive 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 @@ -425,7 +566,7 @@ def missing_base_skills(project: Path, trees: Sequence[str]) -> list[Finding]: 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. + by side — and, across the rename, a different primitive era in each. ``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 @@ -433,35 +574,132 @@ def missing_base_skills(project: Path, trees: Sequence[str]) -> list[Finding]: """ problems: list[Finding] = [] for tree in dict.fromkeys(trees): - skill_dir = project / tree / DEV_PRIMITIVE_SKILL - markers = DEV_BASE_SKILLS[DEV_PRIMITIVE_SKILL] - if not (skill_dir / "SKILL.md").is_file(): + resolved = resolve_dev_primitive(project, tree) + if resolved is None and _is_dev_primitive_shim(project, tree): + legacy_dir = project / tree / DEV_PRIMITIVE_LEGACY + absent = [m for m in DEV_PRIMITIVE_MARKERS if not (legacy_dir / m).is_file()] + problems.append( + Finding( + "skills.base-shim", + "problem", + f"{tree}/{DEV_PRIMITIVE_LEGACY} is unusable (missing " + f"{', '.join(absent)}) and {DEV_PRIMITIVE_NEW} is not installed — " + f"most likely the forwarding shim the BMad Method's rename left " + f"behind, otherwise a truncated install; update the bmm module. The " + f"shim's migration prompt is interactive and would HALT an unattended " + f"session without writing anything to disk", + { + "tree": tree, + "skill": DEV_PRIMITIVE_LEGACY, + "expected": DEV_PRIMITIVE_NEW, + "missing_markers": absent, + }, + ) + ) + elif resolved is None: problems.append( Finding( "skills.base-missing", "problem", - f"{tree}/{DEV_PRIMITIVE_SKILL} not found — the orchestrator drives this " + f"{tree}/{DEV_PRIMITIVE_NEW} 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}, + f"(BMAD-METHOD >= 6.10.0); install or update bmm in this project " + f"(older installs name it {DEV_PRIMITIVE_LEGACY})", + {"tree": tree, "skill": DEV_PRIMITIVE_NEW}, ) ) else: - absent = [m for m in markers if not (skill_dir / m).is_file()] + skill_dir = project / tree / resolved + absent = [m for m in DEV_PRIMITIVE_MARKERS if not (skill_dir / m).is_file()] if absent: problems.append( Finding( "skills.base-incomplete", "problem", - f"{tree}/{DEV_PRIMITIVE_SKILL} is incomplete (missing " + f"{tree}/{resolved} is incomplete (missing " f"{', '.join(absent)}) — reinstall it from the bmm module", - {"tree": tree, "skill": DEV_PRIMITIVE_SKILL, "missing_markers": absent}, + {"tree": tree, "skill": resolved, "missing_markers": absent}, ) ) problems.extend(_review_findings(project, tree)) return problems +def dev_primitive_warnings(project: Path, trees: Sequence[str]) -> list[Finding]: + """Advisory findings about a resolved dev primitive — validate-only, never a gate. + + One condition, and it is genuinely survivable — which is what keeps it out of + :func:`missing_base_skills`: + + - ``skills.customize-legacy``: at least one tree resolved to the NEW name while a + customization override still sits under the OLD one with no counterpart, i.e. + the rename silently orphaned it. Emitted once per project (the override files + are project-global, not per tree). The session still runs; it just runs + unstyled, so naming it is an operator heads-up rather than a gate. + + ⚠️ The remediation is COPY, not rename, whenever another active tree still + resolves to the legacy primitive. A project can sit mid-upgrade with a different + era in each tree (`.claude/skills` on the new name, `.agents/skills` still on a + marker-complete old one), and each tree resolves its overrides under its OWN + era — so the legacy file is orphaned for the new tree and LIVE for the legacy + one. Telling that operator to rename it moves the customization from one tree to + the other instead of fixing anything. Suppressing the finding instead would be + the opposite error: the new tree really is running unstyled, which is precisely + the silent degradation this warning exists to surface. + + Deliberately a warning and not a problem. :func:`missing_base_skills` feeds a + gate with no severity filter and no ``--force``: a false FAIL there pauses every + run behind a remediation nobody can apply, so on these checks a false green is + the safe direction. The orphaned file's layers really are inert for a new-era + tree — upstream's resolver keys on the skill dir — so the honest response is to + say so and name the rename, not to block. + + Returns [] when nothing resolves — :func:`missing_base_skills` owns that story. + """ + findings: list[Finding] = [] + resolved = {tree: resolve_dev_primitive(project, tree) for tree in dict.fromkeys(trees)} + new_trees = [tree for tree, name in resolved.items() if name == DEV_PRIMITIVE_NEW] + legacy_trees = [tree for tree, name in resolved.items() if name == DEV_PRIMITIVE_LEGACY] + if new_trees: + orphaned = [ + (CUSTOMIZE_DIR / f"{DEV_PRIMITIVE_LEGACY}{suffix}").as_posix() + for suffix in (".toml", ".user.toml") + if (project / CUSTOMIZE_DIR / f"{DEV_PRIMITIVE_LEGACY}{suffix}").is_file() + and not (project / CUSTOMIZE_DIR / f"{DEV_PRIMITIVE_NEW}{suffix}").is_file() + ] + if orphaned and legacy_trees: + # Mixed-era project: the file is live for the legacy tree, so name the + # tree it still styles and say copy. `legacy_trees` rides in `detail` + # only on this branch, keeping the all-new detail dict unchanged. + findings.append( + Finding( + "skills.customize-legacy", + "warning", + f"{', '.join(orphaned)} does not apply in " + f"{', '.join(new_trees)} (resolved {DEV_PRIMITIVE_NEW}) but still " + f"applies in {', '.join(legacy_trees)} — COPY the override file(s) " + f"to the {DEV_PRIMITIVE_NEW} name; renaming would drop the " + f"customization from {', '.join(legacy_trees)}", + { + "files": orphaned, + "skill": DEV_PRIMITIVE_NEW, + "legacy_trees": legacy_trees, + }, + ) + ) + elif orphaned: + findings.append( + Finding( + "skills.customize-legacy", + "warning", + f"{', '.join(orphaned)} no longer applies — the dev primitive is now " + f"{DEV_PRIMITIVE_NEW}; rename the override file(s) to match", + {"files": orphaned, "skill": DEV_PRIMITIVE_NEW}, + ) + ) + return findings + + 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: @@ -472,14 +710,21 @@ def _where_clause(layer_ids: Sequence[str]) -> str: def _review_findings(project: Path, tree: str) -> list[Finding]: - """Findings for the review skills this tree's bmad-dev-auto invokes. + """Findings for the review skills this tree's dev primitive 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. + + The primitive's name is resolved here for the MESSAGES — which tell an operator + which skill dir and which override file to go and fix, so a hardcoded era would + send them to a path that does not exist on a renamed project. The static-catalog + fallback below has no :class:`ReviewResolution` to carry the name, so this + resolves independently rather than widening that tuple. """ + primitive = dev_primitive_or_default(project, tree) resolved = resolve_review_layers(project, tree) if resolved is None: # Unknown shape: keep the long-standing static requirement, which both real @@ -516,10 +761,10 @@ def _review_findings(project: Path, tree: str) -> list[Finding]: Finding( "skills.review-layers-empty", "problem", - f"{tree}/{DEV_PRIMITIVE_SKILL} has no enabled review layer (every " + f"{tree}/{primitive} 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()}", + f"{_customize_overrides(primitive)[0].as_posix()}", {"tree": tree, "source": resolved.source}, ) ) @@ -530,7 +775,7 @@ def _review_findings(project: Path, tree: str) -> list[Finding]: Finding( "skills.review-layer-missing", "problem", - f"{tree}/{skill} not found — {tree}/{DEV_PRIMITIVE_SKILL}'s " + f"{tree}/{skill} not found — {tree}/{primitive}'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", @@ -549,7 +794,7 @@ def _review_findings(project: Path, tree: str) -> list[Finding]: Finding( "skills.review-layer-unresolved", "warning", - f"{tree}/{skill} not found — {tree}/{DEV_PRIMITIVE_SKILL}'s " + f"{tree}/{skill} not found — {tree}/{primitive}'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", diff --git a/src/bmad_loop/policy.py b/src/bmad_loop/policy.py index edb5aed4..69d0ba1a 100644 --- a/src/bmad_loop/policy.py +++ b/src/bmad_loop/policy.py @@ -251,6 +251,15 @@ class DevPolicy: # it writes no result.json — the GenericDevAdapter synthesizes one from the # spec the session leaves on disk. The field is retained (rather than inlined) # as the seam for a future alternative dev skill; see DEV_SKILLS. + # + # NOT the name a session is dispatched with. Upstream renamed the primitive + # bmad-dev-auto -> bmad-build-auto (BMAD-METHOD#2651), so the invoked name is + # resolved from what is actually on disk (Engine._dev_skill, via + # install.dev_primitive_or_default) and a project on either era works with + # this field untouched. This value is the ADAPTER DISCRIMINATOR — it selects + # the decoupled generic-dev behaviour seams (engine._generic_dev, + # runsetup.py's result-synthesis switch) — so it keeps the pre-rename + # spelling as a stable key rather than tracking the upstream directory name. skill: str = "bmad-dev-auto" @@ -859,7 +868,12 @@ def loads(text: str, plugin_schemas: dict[str, Any] | None = None) -> Policy: raise PolicyError('stories.source = "stories" requires stories.spec_folder to be set') dev = DevPolicy(skill=str(dev_d.get("skill", DevPolicy.skill))) if dev.skill not in DEV_SKILLS: - raise PolicyError(f"dev.skill must be one of {sorted(DEV_SKILLS)}: got {dev.skill!r}") + raise PolicyError( + f"dev.skill must be one of {sorted(DEV_SKILLS)}: got {dev.skill!r}. This is the " + f"adapter discriminator, not the invoked skill name — the name a session is " + f"dispatched with is resolved from the skill tree on disk, so a project on the " + f"post-rename bmad-build-auto needs no change here" + ) for legacy, replacement in ( ("model_dev", "[adapter.dev] model"), ("model_review", "[adapter.review] model"), diff --git a/src/bmad_loop/stories_engine.py b/src/bmad_loop/stories_engine.py index 2502d8aa..7576ceef 100644 --- a/src/bmad_loop/stories_engine.py +++ b/src/bmad_loop/stories_engine.py @@ -346,7 +346,8 @@ def _stories_dev_prompt(self, task: StoryTask, feedback: Path | None) -> str: repair leg falls back to the inherited explicit-spec-file resume. Fresh dispatch: - ``/bmad-dev-auto Spec folder: . Story id: .`` + ``/ Spec folder: . Story id: .`` + (the primitive is disk-resolved — see ``Engine._dev_skill``) + (plan-halt leg) `` Halt after planning.`` + (when ``invoke_dev_with`` non-empty) a newline then its verbatim text. @@ -361,14 +362,17 @@ def _stories_dev_prompt(self, task: StoryTask, feedback: Path | None) -> str: self._reset_spec_for_repair(task) spec_ref = task.spec_file or task.story_key return ( - f"/bmad-dev-auto Resume the autonomous dev session on the in-progress " + f"/{self._dev_skill()} Resume the autonomous dev session on the in-progress " f"spec at `{spec_ref}`. The previous session's work failed deterministic " f"verification; repair the working tree so verification passes without " f"changing the spec's frozen intent contract. Verification evidence is " f"in `{feedback}`." ) entry = self._entry_for(task) - prompt = f"/bmad-dev-auto Spec folder: {self._spec_folder_rel}. Story id: {task.story_key}." + prompt = ( + f"/{self._dev_skill()} Spec folder: {self._spec_folder_rel}. " + f"Story id: {task.story_key}." + ) if self._plan_halt_leg(task, entry): prompt += " Halt after planning." if entry is not None and entry.invoke_dev_with: diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index f9ece68d..966db6cf 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -1188,7 +1188,8 @@ def _dev_prompt(self, task: StoryTask, feedback: Path | None) -> str: return self._generic_bundle_prompt(task, feedback) def _generic_bundle_prompt(self, task: StoryTask, feedback: Path | None) -> str: - """Bundle invocation for the generic bmad-dev-auto dev skill: the self-contained + """Bundle invocation for the generic dev primitive (disk-resolved, see + ``Engine._dev_skill``): the self-contained intent.md (intent + verbatim ledger entries) is handed over as freeform intent. The orchestrator owns the deferred-work ledger — the skill is told not to edit it — and records resolution itself in `_post_dev_state_sync`. @@ -1204,7 +1205,7 @@ def _generic_bundle_prompt(self, task: StoryTask, feedback: Path | None) -> str: if feedback is None: if task.restore_patch and task.spec_file: return ( - f"/bmad-dev-auto Resume review of the in-review spec at " + f"/{self._dev_skill()} Resume review of the in-review spec at " f"`{task.spec_file}` for the deferred-work bundle `{bundle_ref}`. " f"The attempted change was restored onto the working tree after " f"an intent-gap resolution; review it against the amended spec. " @@ -1212,7 +1213,7 @@ def _generic_bundle_prompt(self, task: StoryTask, feedback: Path | None) -> str: f"resolution." ) return ( - f"/bmad-dev-auto Implement the deferred-work bundle described in " + f"/{self._dev_skill()} Implement the deferred-work bundle described in " f"`{bundle_ref}` — it carries the intent and the verbatim ledger " f"entries to resolve. Do NOT edit the deferred-work ledger; the " f"orchestrator records resolution." @@ -1220,7 +1221,7 @@ def _generic_bundle_prompt(self, task: StoryTask, feedback: Path | None) -> str: self._reset_spec_for_repair(task) spec_ref = task.spec_file or bundle_ref return ( - f"/bmad-dev-auto Resume the autonomous dev session on the in-progress " + f"/{self._dev_skill()} Resume the autonomous dev session on the in-progress " f"spec at `{spec_ref}` for the deferred-work bundle `{bundle_ref}`. The " f"previous session's work failed deterministic verification; repair the " f"working tree so verification passes without changing the frozen intent " diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index 30504fbe..99905f06 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -29,6 +29,7 @@ from .install import ( BASE_SKILLS, CUSTOMIZE_DIR, + DEV_PRIMITIVE_ROLES, HOOK_SCRIPT_REL, MODULE_SKILLS, _copy_traversable, @@ -216,16 +217,24 @@ def provision_worktree( _copy_traversable(skills_root.joinpath(skill), dst) # 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 + # worktree can still resolve the dev primitive and the review layers. Skip # silently when the main repo lacks them — the run-start preflight reports # it. # + # BASE_SKILLS names BOTH primitive eras, which is what carries the skill + # across the rename: the resolution below returns REVIEW skills only, so a + # primitive the catalog did not name would be silently left behind (the + # is_dir guard swallows the miss) and every isolated session would stall on + # an Unknown command. + # # 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. + # invokes are read from the installed primitive — resolved on disk, so a + # renamed project resolves its own layers rather than degrading to the + # static catalog — 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: @@ -408,10 +417,17 @@ def ensure_target_branch(self) -> None: def worktree_profiles(self) -> list[CLIProfile]: """The distinct CLI profiles of the dev + review adapters, for provisioning their skills/hooks into a worktree. Adapters without a `profile` (e.g. test - fakes) contribute nothing, so provisioning is a no-op for them.""" + fakes) contribute nothing, so provisioning is a no-op for them. + + The role set is :data:`install.DEV_PRIMITIVE_ROLES` — the same constant + `cli._skill_trees` gates on — rather than a local pair, so the provisioned + set and the gated set cannot drift apart. A tree gated but not provisioned + ships a session into the `Unknown command` stall the preflight exists to + catch; a tree provisioned but not gated refuses runs over a skill no session + reads.""" seen: dict[str, CLIProfile] = {} adapters = self._adapters_get() - for adapter in (adapters["dev"], adapters["review"]): + for adapter in (adapters[role] for role in DEV_PRIMITIVE_ROLES): profile = getattr(adapter, "profile", None) if profile is not None and profile.name not in seen: seen[profile.name] = profile diff --git a/tests/conftest.py b/tests/conftest.py index 885501ba..fb0170a4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -323,17 +323,95 @@ def install_dev_base_skills(root: Path, tree: str = ".claude/skills", *, folder_ return skills +def install_build_auto_skill( + root: Path, tree: str = ".claude/skills", *, folder_id: bool = True +) -> Path: + """The post-rename twin of :func:`install_dev_base_skills`: lay down the NEW dev + primitive (`install.DEV_PRIMITIVE_NEW`) plus the review hunters under ``root/tree``. + + Deliberately lays down ONE era. A test that wants both names on disk calls this + *and* :func:`install_dev_base_skills`; a test that wants only the legacy era calls + that one alone. (Note :func:`install_base_skills` lays down BOTH, because + `BASE_SKILLS` is the copy-if-present worktree catalog and names both eras — so a + scaffold built from it resolves to the new name.) + + ``folder_id`` writes the resolved primitive's step-01 carrying the dispatch marker + `install.missing_stories_support` content-probes for, exactly as the legacy twin + does — under `bmad-build-auto`, since that is the name that resolves here.""" + from bmad_loop.install import ( + DEV_BASE_SKILLS, + DEV_PRIMITIVE_LEGACY, + DEV_PRIMITIVE_MARKERS, + DEV_PRIMITIVE_NEW, + STORIES_PROBE_FILE, + STORIES_PROBE_TEXT, + ) + + # The hunters, read off the catalog rather than restated — but with the primitive + # entry swapped for the new name, since DEV_BASE_SKILLS is keyed on the legacy one. + hunters = {k: v for k, v in DEV_BASE_SKILLS.items() if k != DEV_PRIMITIVE_LEGACY} + skills = Path(root) / tree + _write_skill_stubs(skills, {DEV_PRIMITIVE_NEW: DEV_PRIMITIVE_MARKERS, **hunters}) + if folder_id: + (skills / DEV_PRIMITIVE_NEW / STORIES_PROBE_FILE).write_text( + f"This is a **{STORIES_PROBE_TEXT}** router.\n", encoding="utf-8" + ) + return skills + + +def install_dev_shim(root: Path, tree: str = ".claude/skills", *, with_review: bool = True) -> Path: + """Lay down ONLY the post-rename forwarding shim: a lone `bmad-dev-auto/SKILL.md` + with no marker files and no new-name skill beside it. + + This is the install `bmad-loop validate` must REFUSE (`skills.base-shim`) rather + than drive: the shim's customization-migration gate is interactive, so an + unattended session dispatched into it HALTs having written nothing to disk. + + ``with_review`` also stubs the merged reviewer, which satisfies `_review_findings`' + static fallback — so a shim test's findings are exactly the shim finding, and an + assertion on their count is not silently counting absent review layers too.""" + from bmad_loop.install import DEV_PRIMITIVE_LEGACY, MERGED_REVIEW_SKILL + + skills = Path(root) / tree + shim = skills / DEV_PRIMITIVE_LEGACY + shim.mkdir(parents=True, exist_ok=True) + (shim / "SKILL.md").write_text(f"# {DEV_PRIMITIVE_LEGACY}\n", encoding="utf-8") + if with_review: + _write_skill_stubs(skills, {MERGED_REVIEW_SKILL: ()}) + return skills + + def install_base_skills(paths: ProjectPaths, trees=(".claude/skills", ".agents/skills")) -> None: """Stub every non-bundled upstream skill (`install.BASE_SKILLS` — a superset of DEV_BASE_SKILLS that also covers what a worktree mount must copy) in each of a - sandbox project's active CLI skill trees. Sprint mode drives any bmad-dev-auto, - so no folder+id probe is written.""" + sandbox project's active CLI skill trees. Sprint mode drives any dev primitive, + so no folder+id probe is written. + + BASE_SKILLS names BOTH primitive eras, so this lays down both and the tree + resolves to `bmad-build-auto`. For a single-era scaffold use + :func:`install_dev_base_skills` (legacy) or :func:`install_build_auto_skill`.""" from bmad_loop.install import BASE_SKILLS for tree in trees: _write_skill_stubs(paths.project / tree, BASE_SKILLS) +def attach_profile(adapter, name: str = "claude", project: Path | None = None): + """Give a scripted adapter the ``profile`` a real CLI adapter carries, so the + seams that read ``adapter.profile.skill_tree`` — chiefly ``Engine._dev_skill``, + which resolves the invoked dev-primitive NAME off disk — see a real skill tree. + + `MockAdapter` deliberately has no `profile` at all, and that is not an + oversight to paper over globally: the profile-less shape IS the None-tree + fallback path (legacy name), so it stays the default and gets pinned by its + own test. Attach only where the resolved name is what's under test. Returns + the adapter for chaining.""" + from bmad_loop.adapters.profile import get_profile + + adapter.profile = get_profile(name, project) + return adapter + + def fault_read_text(monkeypatch, target: Path) -> None: """Make exactly ``target``'s ``read_text`` raise PermissionError; every other path still reads normally. A selective monkeypatch rather than chmod: chmod is a diff --git a/tests/test_cli.py b/tests/test_cli.py index c1d2a3e4..b4e50cb1 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -12,7 +12,9 @@ fault_read_text, git, install_bmad_config, + install_build_auto_skill, install_dev_base_skills, + install_dev_shim, machine_json, mark_ledger_done, spec_path, @@ -103,6 +105,136 @@ def test_dry_run_renders_per_stage_commands(project, capsys): assert "--model gpt-5-codex" in review_line +def _shim_only(paths) -> None: + """Post-rename install left with nothing but the forwarding shim, in every + tree the dual-client policy reads.""" + import shutil + + from conftest import install_base_skills + + install_base_skills(paths) + for tree in (".claude/skills", ".agents/skills"): + shutil.rmtree(paths.project / tree / "bmad-build-auto") + shutil.rmtree(paths.project / tree / "bmad-dev-auto") + install_dev_shim(paths.project, tree) + + +def test_dry_run_warns_when_preflight_would_abort(project, capsys): + """`--dry-run` returns before `_require_base_skills`, so a broken install still + renders a plausible preview. On a shim-only project that preview is a lie the + operator cannot see through — the shim IS a valid slash command, so + `/bmad-dev-auto ...` reads fine and would HALT the session. Say so. + + stdout keeps the schedule (a diagnostic must not withhold what was asked for) + and the exit code stays 0; the banner is stderr-only.""" + _shim_only(project) + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + _write_policy(project.project) + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + args = argparse.Namespace(epic=None, story=None, max_stories=None) + + assert cli._dry_run(project, pol, args) == 0 + out, err = capsys.readouterr() + assert "NOT runnable" in err and "bmad-build-auto" in err + assert "run `bmad-loop validate` for details" in err + assert "1-1-a" in out # the schedule itself still rendered + + +def test_dry_run_stories_warns_when_preflight_would_abort(project, capsys): + """Same banner on the stories preview, which additionally probes folder+id + dispatch support on the resolved primitive.""" + _shim_only(project) + _setup_stories_fixture(project, [_stories_entry("1")]) + _write_policy(project.project) + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + args = argparse.Namespace(spec=STORIES_SPEC_FOLDER, epic=None, story=None, max_stories=None) + + assert cli._dry_run(project, pol, args, True, STORIES_SPEC_FOLDER) == 0 + out, err = capsys.readouterr() + assert "NOT runnable" in err and "bmad-build-auto" in err + assert "Story id: 1." in out + + +def test_sweep_dry_run_warns_when_preflight_would_abort(project, capsys): + """`cmd_sweep` has the same shape — dry-run returns before its preflight. The + banner is emitted BEFORE the no-ledger early return, so a project with a broken + install and nothing to sweep still hears about the install.""" + _shim_only(project) + _write_policy(project.project) + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + assert not project.deferred_work.is_file() # the early-return leg + assert cli._sweep_dry_run(project, pol) == 0 + assert "NOT runnable" in capsys.readouterr().err + + +def test_dry_run_is_silent_when_preflight_would_pass(project, capsys): + """The banner must be evidence, not decoration: a complete install prints + nothing to stderr. Without this the warning could be unconditional and every + assertion above would still pass.""" + from conftest import install_base_skills + + install_base_skills(project) + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + _write_policy(project.project) + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + args = argparse.Namespace(epic=None, story=None, max_stories=None) + + assert cli._dry_run(project, pol, args) == 0 + out, err = capsys.readouterr() + assert err == "" + # ...and the preview spells the name that actually resolved: install_base_skills + # lays down BOTH eras, so the tree resolves to the post-rename primitive. + assert "/bmad-build-auto 1-1-a" in out + + +def test_dry_run_banner_stays_silent_for_a_warning_only_finding(project, capsys): + """Severity decides, exactly as in `_require_base_skills`. That gate steps over + non-problem findings — an unresolvable review layer is reported and the run + proceeds — so listing one under a banner that claims "the real command aborts" + would be a false alarm on a project that runs fine. A false "NOT runnable" is + the wrong direction for a check with no severity filter and no `--force`. + + (The shipped 0.9.1 banner had no severity filter; this is a deliberate + correction, not a port artefact.)""" + from conftest import install_base_skills + + from bmad_loop import install + + install_base_skills(project) + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + _write_policy(project.project) + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + args = argparse.Namespace(epic=None, story=None, max_stories=None) + + # The stub installer writes every marker as placeholder text, so the primitive's + # OWN customize.toml does not parse — and that is the one case `_merged_review_layers` + # reads as "shape unknown", which short-circuits to the static catalog and returns no + # finding at all. Give it a real layer so resolution SUCCEEDS; only then is an + # override reached, and only then can an unparseable one be reported. + layer = ( + "[[workflow.review_layers]]\n" + 'id = "adversarial"\n' + 'instruction = "Invoke the `bmad-review` skill with only the `adversarial` lens."\n' + ) + for tree in cli._skill_trees(project.project, pol): + primitive = project.project / tree / install.DEV_PRIMITIVE_NEW + (primitive / "customize.toml").write_text(layer, encoding="utf-8") + + # an unparseable override layer: the resolver skips it and the run still goes + custom = project.project / install.CUSTOMIZE_DIR + custom.mkdir(parents=True, exist_ok=True) + (custom / f"{install.DEV_PRIMITIVE_NEW}.toml").write_text("not = [toml\n", encoding="utf-8") + + findings = install.missing_base_skills(project.project, cli._skill_trees(project.project, pol)) + severities = {f.severity for f in findings} + assert severities == {"warning"}, f"fixture no longer produces a warning-only set: {findings}" + assert {f.check for f in findings} == {"skills.customize-unreadable"} + + assert cli._dry_run(project, pol, args) == 0 + assert capsys.readouterr().err == "" + + @pytest.mark.parametrize( "epic,story", [(None, "3-1"), (None, "3.1"), (3, "1"), (None, "user-auth"), (None, "3-1-user-auth")], @@ -3811,16 +3943,26 @@ def test_validate_stories_folder_known_selector_ok(project): CLAUDE_ONLY_POLICY = '[adapter]\nname = "claude"\nmodel = "opus"\n' -def _make_validate_pass(project, monkeypatch, capsys): +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 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.""" + means nothing. + + ``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 + tree) while keeping every other gate green — an rc-0 assertion about one check is + worthless if some unrelated gate is what is actually failing. ``skills`` is called + with the project root BEFORE the commit, so whatever it lays down is committed and + the worktree-clean gate still passes.""" install_bmad_config(project) - _write_policy(project.project, CLAUDE_ONLY_POLICY) + _write_policy(project.project, policy) write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) - install_dev_base_skills(project.project, folder_id=True) + if skills is None: + install_dev_base_skills(project.project, folder_id=True) + else: + skills(project.project) assert cli.main(["init", "--project", str(project.project)]) == 0 # registers the hooks git(project.project, "add", "-A") # every file above is a worktree change git(project.project, "commit", "-q", "-m", "validate fixture") @@ -4087,8 +4229,7 @@ def test_external_backend_failure_is_a_warning_not_a_note(mux_registry, monkeypa report.extend([finding]) report.render() assert capsys.readouterr().out == ( - " ok: warning: external mux backend 'brokenmux' failed to load: " - "ImportError: no ghost\n" + " ok: warning: external mux backend 'brokenmux' failed to load: ImportError: no ghost\n" ) @@ -5179,3 +5320,277 @@ def test_validate_operator_warnings_never_fail_the_run(project, capsys): _park_story(project, spec_status="done") cli._validate_operator_registry(project.project, project, report) assert report.findings and report.passed + + +# ------------- the dev primitive's rename: bmad-dev-auto -> bmad-build-auto --- + +# dev+review on claude (skill tree `.claude/skills`) with triage on a CLI whose +# profile reads a DIFFERENT tree (`.agents/skills`). This is the topology the +# 3-roles-vs-2 mismatch broke: nothing ever dispatches a bmm skill into triage's +# tree, so gating it refused runs over skills that tree will never need. +TRIAGE_SPLIT_POLICY = ( + '[adapter]\nname = "claude"\nmodel = "opus"\n[adapter.triage]\nname = "gemini"\n' +) + +# Two CLIs across two trees, with the review STAGE switched off. The adapter is +# still resolved, still provisioned, and still hooked — see #424 below. +REVIEW_DISABLED_POLICY = DUAL_CLIENT_POLICY + "[review]\nenabled = false\n" + + +def test_require_base_skills_does_not_gate_a_triage_only_skill_tree(project): + """A triage CLI on its own skill tree must not be able to refuse a run. + + ``_skill_trees`` iterates :data:`install.DEV_PRIMITIVE_ROLES` (dev, review), not + :data:`runsetup.ROLES` (dev, review, triage). Every skill this preflight asks + about — the dev primitive and the review layers its step-04 invokes inline — is + one only a dev or review session dispatches, and ``WorktreeFlow`` only ever + provisions those same two profiles. Gating all three meant + ``[adapter.triage] name = "gemini"`` beside a claude dev/review pair demanded the + whole bmm module in ``.agents/skills`` before the run could start: a hard FAIL + over a tree no session dispatches one of these skills into, on a gate with no + ``--force``. Triage's only prompt is ``/bmad-loop-sweep``, which ships in this + wheel and is laid down by ``bmad-loop init``. + + Ablation: point ``_skill_trees`` back at ``ROLES`` and the ``is True`` below goes + False — nothing installed ``.agents/skills/bmad-build-auto``, and nothing ever + will.""" + from bmad_loop.install import DEV_PRIMITIVE_ROLES + from bmad_loop.runsetup import ROLES + + _write_policy(project.project, TRIAGE_SPLIT_POLICY) + install_build_auto_skill(project.project, ".claude/skills") # claude's tree ONLY + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + + assert cli._require_base_skills(project.project, pol) is True + # ...because triage's tree was never asked about. Asserting the whole list, not + # `".agents/skills" not in`: the point is which trees ARE gated, and an empty + # list would satisfy the negative form while gating nothing at all. + assert cli._skill_trees(project.project, pol) == [".claude/skills"] + + # The structural half, so a `triage` that creeps back into the constant fails + # here by name rather than only through a filesystem shape. The two sets are one + # decision — `WorktreeFlow.worktree_profiles` reads the same constant — so the + # dev-primitive set being a STRICT subset is what keeps "gated" and "provisioned" + # from drifting apart in either direction. + assert DEV_PRIMITIVE_ROLES == ("dev", "review") + assert set(DEV_PRIMITIVE_ROLES) < set(ROLES) + + +def test_skill_trees_covers_review_even_when_review_is_disabled(project): + """``review.enabled = false`` must NOT narrow the gated trees (#424). + + This test exists to make a future narrowing FAIL. The obvious follow-on to the + role-scoping above is "and drop review's tree when the review stage is off" — it + is wrong, and it is wrong quietly. Disabling the review stage does not retire the + review ADAPTER: a plugin workflow can declare ``role = "review"`` and dispatch on + ``adapters["review"]`` with review disabled, and the same profile list drives + per-CLI Stop-signal hook registration as well as worktree seeding. A worktree + provisioned without the review profile has no completion signal for those + sessions, so narrowing here would convert a clean, actionable preflight refusal + into a silent stall that burns the run's whole budget.""" + _write_policy(project.project, REVIEW_DISABLED_POLICY) + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + assert pol.review.enabled is False, "the fixture only bites with the stage off" + + trees = cli._skill_trees(project.project, pol) + assert trees == [".claude/skills", ".agents/skills"] # dev=claude, review=codex + + +def test_gated_trees_and_provisioned_trees_stay_one_decision(project): + """Constraint 1, pinned from BOTH sides rather than asserted in a comment. + + ``cli._skill_trees`` decides which trees can REFUSE a run; + ``WorktreeFlow.worktree_profiles`` decides which profiles get carried INTO a + worktree. The only thing making those one decision is that both iterate + :data:`install.DEV_PRIMITIVE_ROLES` — nothing enforced it, so a one-sided edit + used to be invisible here. Either direction ships a silent bug: gated but not + provisioned drops the session into the `Unknown command` stall the preflight + exists to catch, provisioned but not gated refuses runs over a skill nothing + reads. + + The triage split is what makes the two sets separable at all — with one CLI + everywhere, both readings agree by construction and the test proves nothing. + + ``worktree_profiles`` is called unbound on a stub carrying only + ``_adapters_get``, which is the entirety of the ``self`` it touches: building a + whole `WorktreeFlow` here would drag in the engine callbacks without making the + assertion any stronger. The adapters dict carries ALL of `runsetup.ROLES`, so a + one-sided revert to `ROLES` fails on the set comparison below rather than + escaping as a `KeyError` on a missing triage key.""" + from types import SimpleNamespace + + from bmad_loop.adapters.profile import get_profile + from bmad_loop.runsetup import ROLES + from bmad_loop.worktree_flow import WorktreeFlow + + _write_policy(project.project, TRIAGE_SPLIT_POLICY) + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + adapters = { + role: SimpleNamespace(profile=get_profile(pol.adapter.resolved(role).name, project.project)) + for role in ROLES + } + + gated = cli._skill_trees(project.project, pol) + provisioned = WorktreeFlow.worktree_profiles(SimpleNamespace(_adapters_get=lambda: adapters)) + + # Pin the value, not just the agreement: two empty sets would satisfy the + # equality below while gating and provisioning nothing at all. + assert gated == [".claude/skills"] + assert {p.skill_tree for p in provisioned} == set(gated) + + +@pytest.mark.parametrize( + ("installer", "primitive"), + [ + (lambda root: install_build_auto_skill(root), "bmad-build-auto"), + (lambda root: install_dev_base_skills(root, folder_id=True), "bmad-dev-auto"), + ], + ids=["renamed", "legacy"], +) +def test_validate_names_the_dev_primitive_that_actually_resolved( + project, capsys, monkeypatch, installer, primitive +): + """The ``skills.base`` ok line names what is on disk, not a hardcoded era. + + It used to read "upstream skills present (bmad-dev-auto + review layers)" + unconditionally, which on an upgraded project is a green line asserting the + rename did not happen. On a rename this line is the operator's only confirmation + that the new name was picked up, so it is asserted whole rather than by + substring, and the machine-readable ``dev_primitive`` beside it is what a + consumer reads instead of parsing the sentence back apart.""" + _make_validate_pass(project, monkeypatch, capsys, skills=installer) + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys) + base = next(f for f in doc["findings"] if f["check"] == "skills.base") + assert base["message"] == f"upstream skills present ({primitive} + review layers)" + assert base["detail"]["dev_primitive"] == [primitive] + assert base["detail"]["trees"] == [".claude/skills"] + + +def test_validate_reports_a_different_primitive_era_in_each_tree(project, capsys, monkeypatch): + """Why ``dev_primitive`` is a list and the ok line joins it. + + A project can sit mid-upgrade: ``.claude/skills`` (claude, dev) already carries + ``bmad-build-auto`` while ``.agents/skills`` (codex, review) still carries a + complete pre-rename ``bmad-dev-auto``. Both are drivable, and each tree is driven + under its OWN name — the resolution is per tree, not per project. A scalar field + would have to pick a winner and would then tell the operator the rename landed + everywhere when it landed in one tree.""" + + def _mixed_eras(root): + install_build_auto_skill(root, ".claude/skills") + install_dev_base_skills(root, ".agents/skills", folder_id=True) + + _make_validate_pass(project, monkeypatch, capsys, policy=DUAL_CLIENT_POLICY, skills=_mixed_eras) + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys) + base = next(f for f in doc["findings"] if f["check"] == "skills.base") + assert base["detail"]["trees"] == [".claude/skills", ".agents/skills"] + assert base["detail"]["dev_primitive"] == ["bmad-build-auto", "bmad-dev-auto"] + assert base["message"] == ( + "upstream skills present (bmad-build-auto + bmad-dev-auto + review layers)" + ) + + +def test_a_forwarding_shim_install_fails_validate_and_aborts_the_run(project, capsys, monkeypatch): + """The shim upstream's rename left behind is REFUSED, not driven. + + Post-rename ``bmad-dev-auto`` is a lone SKILL.md whose customization-migration + gate is interactive. An unattended session dispatched into it HALTs having + written nothing to disk — no spec, no result artifact, nothing the post-session + verification can read — so the story stalls rather than fails, and the run burns + its budget on it. Both entry points must refuse it before any session is spawned, + and the remediation has to name the rename: without that, an operator is being + told a skill that is visibly PRESENT is missing.""" + install_bmad_config(project) + _write_policy(project.project, CLAUDE_ONLY_POLICY) + write_sprint(project, {"1-1-a": "ready-for-dev"}) + install_dev_shim(project.project) # lays the review stub too, so this is the only finding + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "shim fixture") + + findings = _validate_findings(project, capsys, rc=1) + shim = findings["skills.base-shim"] + assert shim["severity"] == "problem" + assert shim["detail"] == { + "tree": ".claude/skills", + "skill": "bmad-dev-auto", + "expected": "bmad-build-auto", + "missing_markers": ["step-04-review.md", "customize.toml"], + } + assert "skills.base" not in findings # no green line riding beside the refusal + + # ...and the same install aborts `run` through _require_base_skills, before the + # engine is reached at all. + monkeypatch.setattr(cli, "Engine", _StubEngine) + monkeypatch.setattr(cli, "_make_adapters", lambda *a, **k: {r: None for r in cli.ROLES}) + + assert cli.main(["run", "--project", str(project.project)]) == 1 + err = capsys.readouterr().err + assert "forwarding shim the BMad Method's rename left behind" in err + assert "bmad-build-auto is not installed" in err + + +def test_validate_reports_an_orphaned_legacy_override_as_a_warning(project, capsys, monkeypatch): + """The rename orphans ``_bmad/custom/bmad-dev-auto.toml`` — and that is a WARNING. + + Upstream's resolver keys on the skill directory, so on a renamed project the + legacy override is simply never read: the session still runs, it just runs + unstyled. The severity is the whole point. ``missing_base_skills`` feeds + ``_require_base_skills``, which has no severity filter and no ``--force`` — a + problem here would pause every run behind a remediation nobody can apply, so on a + survivable condition a false green is the safe direction and the honest response + is to name the rename rather than block.""" + custom = project.project / "_bmad" / "custom" + + def _renamed_with_an_orphan(root): + install_build_auto_skill(root) + custom.mkdir(parents=True, exist_ok=True) + (custom / "bmad-dev-auto.toml").write_text("# stale override\n", encoding="utf-8") + + _make_validate_pass(project, monkeypatch, capsys, skills=_renamed_with_an_orphan) + + findings = _validate_findings(project, capsys) # rc 0: the warning never gates + orphan = findings["skills.customize-legacy"] + assert orphan["severity"] == "warning" + assert orphan["detail"]["files"] == ["_bmad/custom/bmad-dev-auto.toml"] + assert findings["skills.base"]["detail"]["dev_primitive"] == ["bmad-build-auto"] + + # ...and it is the ORPHANING that is reported, not the legacy file's existence: + # land the counterpart and the finding goes. + (custom / "bmad-build-auto.toml").write_text("# migrated\n", encoding="utf-8") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "migrate the override") + + assert "skills.customize-legacy" not in _validate_findings(project, capsys) + + +def test_validate_does_not_gate_a_triage_only_skill_tree(project, capsys, monkeypatch): + """validate's verdict and run's abort are now one decision, on the same trees. + + ``cmd_validate`` builds its trees through the same ``_skill_trees`` the real + preflight calls, so a triage CLI on its own tree can no more fail validate than + it can abort a run. Before that, ``cmd_validate`` passed + ``[p.skill_tree for p in profiles]`` — every loaded profile, triage included — + and reported ``.agents/skills/bmad-build-auto not found`` as a hard problem about + a tree no session dispatches into.""" + _make_validate_pass( + project, + monkeypatch, + capsys, + policy=TRIAGE_SPLIT_POLICY, + skills=lambda root: install_build_auto_skill(root, ".claude/skills"), + ) + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys) + assert doc["ok"] is True and doc["counts"]["problem"] == 0 + base = next(f for f in doc["findings"] if f["check"] == "skills.base") + assert base["detail"]["trees"] == [".claude/skills"] # the member, not a prefix + # and nothing under skills.* so much as mentions triage's tree — keyed on the + # detail's own `tree` field, because a past bug here was a substring assertion + # that three unrelated review findings happened to satisfy. + assert not [ + f + for f in doc["findings"] + if f["check"].startswith("skills.") and (f["detail"] or {}).get("tree") == ".agents/skills" + ] diff --git a/tests/test_devcontract.py b/tests/test_devcontract.py index 54f986b9..cb9e7a67 100644 --- a/tests/test_devcontract.py +++ b/tests/test_devcontract.py @@ -424,11 +424,14 @@ def test_find_artifact_missing_dir(tmp_path): assert devcontract.find_result_artifact(tmp_path / "ghost", since_ns=0) is None -def test_find_artifact_accepts_no_spec_fallback_prefix(tmp_path): +@pytest.mark.parametrize("prefix", ["bmad-build-auto-result-", "bmad-dev-auto-result-"]) +def test_find_artifact_accepts_no_spec_fallback_prefix(tmp_path, prefix): # The no-spec fallback (intent too unclear to create a spec) carries a terminal # frontmatter status but NO `## Auto Run Result` heading — it is matched by its - # `bmad-dev-auto-result-` filename prefix instead. - fallback = tmp_path / "bmad-dev-auto-result-unclear-1234.md" + # `-result-` filename prefix instead. BOTH eras are matched: the artifact + # is named after whichever skill wrote it (BMAD-METHOD #2651 renamed + # bmad-dev-auto to bmad-build-auto), and a run can meet either. + fallback = tmp_path / f"{prefix}unclear-1234.md" fallback.write_text( "---\nstatus: blocked\n---\n\nBlocking condition: unclear intent\n", encoding="utf-8", @@ -436,6 +439,38 @@ def test_find_artifact_accepts_no_spec_fallback_prefix(tmp_path): assert devcontract.find_result_artifact(tmp_path, since_ns=0) == fallback +def test_every_dev_primitive_has_a_fallback_result_prefix(): + """The name the orchestrator WRITES a completion marker under and the set of + names it will MATCH are independent literals in two modules. Nothing derived + one from the other until this test. + + The failure this prevents is silent and total: `engine` names the marker + `f"{resolved_primitive}-result-{task_id}.md"`, and `find_result_artifact` + only looks at names starting with one of these prefixes. A marker outside the + set is not merely unmatched — it falls through to the `## Auto Run Result` + heading branch, which the workflow-completion contract never writes, so the + marker is invisible and every plugin workflow livelocks to + `session_timeout_min` with no error anywhere. + + Subset, not equality, and the direction is the whole point: a primitive with + no prefix is the unreadable-marker bug, while a prefix with no primitive is a + RETIRED era deliberately kept matchable — the comment on + `FALLBACK_RESULT_PREFIXES` asks for exactly that, so a resume can read an + artifact written before an upstream upgrade. + + Reading the constants off the module rather than restating them is what makes + this hold: a third `DEV_PRIMITIVE_*` name added tomorrow is enforced without + anyone remembering this file exists.""" + from bmad_loop import install + + primitives = { + v for n, v in vars(install).items() if n.startswith("DEV_PRIMITIVE_") and isinstance(v, str) + } + assert primitives, "no DEV_PRIMITIVE_* string constants found — has the naming changed?" + missing = {f"{p}-result-" for p in primitives} - set(devcontract.FALLBACK_RESULT_PREFIXES) + assert not missing, f"dev primitives whose completion marker cannot be read back: {missing}" + + def test_find_artifact_ignores_fence_quoted_heading(tmp_path): """A spec whose only `## Auto Run Result` is a fenced example must not qualify as a terminal artifact, even with a fresh mtime — otherwise the @@ -1004,10 +1039,15 @@ def test_frontmatter_candidates_includes_fence_quoted_heading_only(tmp_path): assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) == [p] -def test_frontmatter_candidates_excludes_no_spec_fallback_file(tmp_path): - """bmad-dev-auto-result-*.md is the skill's no-spec fallback — matched by - name on the normal scan, so the fallback scan must not double-claim it.""" - _write(tmp_path, "bmad-dev-auto-result-x.md", "---\nstatus: done\n---\n\nbody\n") +@pytest.mark.parametrize("prefix", ["bmad-build-auto-result-", "bmad-dev-auto-result-"]) +def test_frontmatter_candidates_excludes_no_spec_fallback_file(tmp_path, prefix): + """`-result-*.md` is the skill's no-spec fallback — matched by + name on the normal scan, so the fallback scan must not double-claim it. + + Both eras, because this is the OTHER consumer of `FALLBACK_RESULT_PREFIXES` + and it was pinned on the legacy spelling alone — which post-rename is the + era a project is least likely to be on.""" + _write(tmp_path, f"{prefix}x.md", "---\nstatus: done\n---\n\nbody\n") assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) == [] diff --git a/tests/test_engine.py b/tests/test_engine.py index b701ce9e..31a0f708 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -3497,15 +3497,20 @@ def test_expected_spec_withheld_from_labeled_workflow_session(project): assert pinned is None -def test_record_dev_spec_refuses_the_no_spec_fallback_marker(project): - """`bmad-dev-auto-result-*` is the skill's "intent too unclear to even create a +@pytest.mark.parametrize("prefix", ["bmad-build-auto-result-", "bmad-dev-auto-result-"]) +def test_record_dev_spec_refuses_the_no_spec_fallback_marker(project, prefix): + """`-result-*` is the skill's "intent too unclear to even create a spec" artifact. Recording it as the story's spec misroutes every consumer: the escalation re-arm flips frontmatter on a marker nothing reads, the repair leg re-opens it as the frozen intent contract, and the #261 read-back then pins to - it — polling a stale marker while the re-drive's real spec goes unread.""" + it — polling a stale marker while the re-drive's real spec goes unread. + + Both eras: this refusal reads `FALLBACK_RESULT_PREFIXES`, and pinning only the + legacy spelling left the post-rename marker — the one a current project + actually writes — free to be recorded as the story's spec.""" engine, _ = make_engine(project, []) task = StoryTask(story_key="1-1-a", epic=1) - marker = project.implementation_artifacts / "bmad-dev-auto-result-1-1-a-dev-1.md" + marker = project.implementation_artifacts / f"{prefix}1-1-a-dev-1.md" marker.write_text("---\nstatus: blocked\n---\n\nIntent unclear.\n") engine._record_dev_spec(task, {"spec_file": str(marker)}) @@ -7853,3 +7858,159 @@ def test_marker_repair_skips_on_fm_mismatch(project): (skipped,) = [e for e in engine.journal.entries() if e["kind"] == "spec-marker-repair-skipped"] assert skipped["reason"] == "fm-mismatch" assert "## Auto Run Result" not in sp.read_text() + + +# ------------------------------------ dev-primitive name resolution (BMAD-METHOD #2651) +# Upstream renamed the dev primitive `bmad-dev-auto` → `bmad-build-auto`, leaving a +# forwarding shim behind. The orchestrator therefore spells the invoked name from what +# is actually on disk (Engine._dev_skill) instead of hardcoding it, and must keep +# working against BOTH eras. The rest of this file's ~49 `/bmad-dev-auto` assertions +# pin the profile-less fallback (see the no-profile test below), not the resolution. + + +def _prompt_task(**kw) -> StoryTask: + return StoryTask(story_key="1-1-a", epic=1, **kw) + + +def test_dev_prompts_spell_the_post_rename_primitive(project): + """Every generic-dev leg (fresh, restore, repair) invokes the name resolved + from the dev adapter's skill tree — here the post-rename bmad-build-auto.""" + from conftest import attach_profile, install_build_auto_skill + + install_build_auto_skill(project.project, ".claude/skills") + engine, adapter = make_engine(project, []) + attach_profile(adapter) + + # The engine-injected awaiting-operator contract (#335) rides on the tail of + # every dev prompt, so the invocation is asserted as the head plus the + # explicit absence of the legacy spelling anywhere in the string. + fresh = engine._generic_dev_prompt(_prompt_task(), None) + assert fresh.startswith("/bmad-build-auto 1-1-a") + assert "bmad-dev-auto" not in fresh + + spec = str(spec_path(project, "1-1-a")) + restore = engine._generic_dev_prompt( + _prompt_task(spec_file=spec, restore_patch="/run/attempt.patch"), None + ) + assert restore.startswith("/bmad-build-auto Resume review of the in-review spec") + + feedback = project.implementation_artifacts / "feedback.md" + repair = engine._generic_dev_prompt(_prompt_task(), feedback) + assert repair.startswith("/bmad-build-auto Resume the autonomous dev session") + + +def test_dev_prompt_falls_back_to_the_legacy_name_without_a_profile(project): + """The no-profile shape (test fakes, and any adapter that carries no skill + tree) resolves to the pre-rename name. Pinned rather than incidental: it is + what keeps the rest of this suite — and a pre-rename target project whose + resolution fails open — dispatching a name that exists.""" + from conftest import install_build_auto_skill + + install_build_auto_skill(project.project, ".claude/skills") # present but unreachable + engine, adapter = make_engine(project, []) + assert getattr(adapter, "profile", None) is None + + assert engine._generic_dev_prompt(_prompt_task(), None).startswith("/bmad-dev-auto 1-1-a") + # the None tree IS the fallback path, not an unresolved tree that happened to miss + assert engine._dev_skill_cache == {(project.project, None): "bmad-dev-auto"} + + +def test_review_prompt_resolves_through_the_review_adapters_own_tree(project): + """A run can mix skill trees (dev=claude → .claude/skills, review=gemini → + .agents/skills) and the two trees can sit on different upstream eras. Each + prompt must spell the primitive ITS adapter would actually find, so the + per-role lookup and the per-tree memo are both load-bearing.""" + from conftest import attach_profile, install_build_auto_skill, install_dev_base_skills + + install_build_auto_skill(project.project, ".claude/skills") + install_dev_base_skills(project.project, ".agents/skills", folder_id=False) + review = attach_profile(MockAdapter([]), "gemini") + engine, dev = make_engine(project, [], review_adapter=review) + attach_profile(dev, "claude") + spec = str(spec_path(project, "1-1-a")) + + assert engine._generic_dev_prompt(_prompt_task(), None).startswith("/bmad-build-auto 1-1-a") + assert engine._review_prompt(_prompt_task(spec_file=spec)).startswith( + f"/bmad-dev-auto {spec} —" + ) + # each tree resolved independently — one memo entry per (workspace, tree), not + # one per run + assert engine._dev_skill_cache == { + (project.project, ".claude/skills"): "bmad-build-auto", + (project.project, ".agents/skills"): "bmad-dev-auto", + } + + +def test_workflow_marker_is_named_for_the_workflows_own_role_tree(project): + """The completion-marker filename (WORKFLOW_COMPLETION_CONTRACT) is PRODUCED + from ``_dev_skill(role)`` — the injected workflow's OWN role, not the dev + default. A plugin workflow declares `role = "dev" | "review"` (WORKFLOW_ROLES) + and runs on THAT adapter, whose skill tree can sit at a different upstream era + than dev's: here review=gemini on a pre-rename `.agents/skills` beside + dev=claude on a post-rename `.claude/skills`. Resolving off the dev tree would + name the marker after a primitive the session's own tree does not carry. + + Asserted at the `_run_session` seam — the lowest layer that runs the producer — + by reading the prompt that actually reached the review adapter.""" + from conftest import attach_profile, install_build_auto_skill, install_dev_base_skills + + install_build_auto_skill(project.project, ".claude/skills") # dev tree: post-rename + install_dev_base_skills(project.project, ".agents/skills", folder_id=False) # review: legacy + review = attach_profile(MockAdapter([SessionResult(status="completed")]), "gemini") + engine, dev = make_engine(project, [], review_adapter=review) + attach_profile(dev, "claude") + task = StoryTask(story_key="1-1-a", epic=1) + + engine._run_session(task, role="review", prompt="p", seq=1, label="tea.pre_commit_gate") + + (dispatched,) = review.sessions + assert not dev.sessions # the workflow ran on the role's adapter, not dev's + marker = project.implementation_artifacts / f"bmad-dev-auto-result-{dispatched.task_id}.md" + assert str(marker) in dispatched.prompt # the REVIEW tree's era + assert "bmad-build-auto-result-" not in dispatched.prompt # never the dev tree's + # ...and the dev tree genuinely resolves to the other era, so the two trees + # disagreeing is what the assertions above are reading — not two names that + # happen to coincide. + assert engine._dev_skill() == "bmad-build-auto" + assert engine._dev_skill_cache == { + (project.project, ".agents/skills"): "bmad-dev-auto", + (project.project, ".claude/skills"): "bmad-build-auto", + } + + +def test_dev_prompt_resolves_in_the_reopened_worktree_not_the_main_checkout(project, tmp_path): + """A resumed unit dispatches the name ITS OWN worktree carries. + + `reopen_unit` re-mounts an existing worktree without re-provisioning it (only + the fresh-mount path in `run_isolated` calls `provision_worktree`), so a main + checkout upgraded across the pause — the operator updated bmm while the run sat + at an escalation — leaves the worktree on the old era while the resume preflight + passes against the upgraded checkout. Resolving off the main checkout would + spell `/bmad-build-auto` into a worktree carrying only `bmad-dev-auto`: the + session runs with `cwd=self.workspace.root`, HALTs on an unknown command having + written nothing for verify to read, and burns its dev attempts through to DEFER. + + The second half pins the MEMO, which is the half a workspace-rooted resolution + alone gets wrong: one Engine drives every unit of a run, so the reopened + worktree's answer must not be served to the fresh worktrees mounted after it.""" + from conftest import attach_profile, install_build_auto_skill, install_dev_base_skills + + from bmad_loop.workspace import Workspace + + install_build_auto_skill(project.project, ".claude/skills") # main checkout: upgraded + worktree = tmp_path / "wt" + worktree.mkdir() + install_dev_base_skills(worktree, ".claude/skills", folder_id=False) # worktree: legacy + engine, adapter = make_engine(project, []) + attach_profile(adapter) + default = engine.workspace + + engine.workspace = Workspace(root=worktree, paths=engine.paths.rebased(worktree)) + assert engine._generic_dev_prompt(_prompt_task(), None).startswith("/bmad-dev-auto 1-1-a") + + engine.workspace = default + assert engine._generic_dev_prompt(_prompt_task(), None).startswith("/bmad-build-auto 1-1-a") + assert engine._dev_skill_cache == { + (worktree.resolve(), ".claude/skills"): "bmad-dev-auto", + (project.project, ".claude/skills"): "bmad-build-auto", + } diff --git a/tests/test_frontmatter.py b/tests/test_frontmatter.py index ecdc5311..ab2f17df 100644 --- a/tests/test_frontmatter.py +++ b/tests/test_frontmatter.py @@ -64,10 +64,17 @@ def test_flipping_to_the_status_already_there_returns_false_and_does_not_write(t def test_a_quoted_value_is_written_back_unquoted(tmp_path): """A deliberate non-preservation, and the most load-bearing pin in this file. - `conftest.write_spec` writes `status: ''`, and substring assertions read - the result back as an unquoted `status: done` (tests/test_runs.py:772, - tests/test_stories_e2e.py:594,793). A refactor that "also preserves the - value's quotes" breaks those three from here.""" + `conftest.write_spec` writes `status: ''`, and substring assertions read the + result back UNQUOTED: `status: in-review` in + `test_runs.test_rearm_restore_mode_sets_in_review_strips_arr_and_latches`, and + both that and `status: done` in + `test_stories_e2e.test_e2e_{sprint,sweep}_intent_gap_patch_restore`. A refactor + that "also preserves the value's quotes" breaks those three from here. + + Named rather than cited by line, and stated as the property rather than one + value: the old line numbers had gone stale before anyone noticed, and the old + wording named only `done` — which is not the value the first of those three + asserts at all.""" spec = _spec(tmp_path, "---\nstatus: 'in-review'\n---\nbody\n") assert frontmatter.set_frontmatter_status(spec, "done") is True assert spec.read_bytes().decode() == "---\nstatus: done\n---\nbody\n" diff --git a/tests/test_install.py b/tests/test_install.py index eede0ee6..ce04c68b 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -11,23 +11,33 @@ from pathlib import Path import pytest -from conftest import git +from conftest import git, install_build_auto_skill, install_dev_shim import bmad_loop.install as install_mod from bmad_loop import verify from bmad_loop.adapters.profile import get_profile from bmad_loop.install import ( BASE_SKILLS, + CUSTOMIZE_DIR, DEV_BASE_SKILLS, + DEV_PRIMITIVE_LEGACY, + DEV_PRIMITIVE_MARKERS, + DEV_PRIMITIVE_NEW, MODULE_SKILLS, _copy_traversable, _git_version_at_least, + _is_dev_primitive_shim, _shield_undo_extension, _worktree_local_exclude, + dev_primitive_or_default, + dev_primitive_warnings, install_into, merge_hooks, missing_base_skills, + missing_stories_support, provision_worktree, + resolve_dev_primitive, + resolve_review_layers, ) @@ -46,10 +56,29 @@ def _install_skills(root, tree, catalog): def _install_base_skills(root, tree=".claude/skills"): - """Lay down stubs of the non-bundled upstream skills the orchestrator drives.""" + """Lay down stubs of the non-bundled upstream skills the orchestrator drives. + + ⚠ `BASE_SKILLS` is the copy-if-present WORKTREE catalog, so it names BOTH dev + primitive eras — a tree built from it therefore RESOLVES to `bmad-build-auto` + (`resolve_dev_primitive` prefers the new name). A test that pokes at the + `bmad-dev-auto/` dir laid down here is poking at a directory nothing reads. + Use :func:`_era_catalog` for a scaffold that pins one era.""" _install_skills(root, tree, BASE_SKILLS) +def _era_catalog(primitive): + """`DEV_BASE_SKILLS` with its dev-primitive entry re-keyed to ``primitive``. + + DEV_BASE_SKILLS is keyed on the LEGACY name because it doubles as the "lay down a + pre-rename install" catalog, so a post-rename scaffold needs the same content + under the new one. Derived rather than restated, so a newly required marker or + review hunter reaches both eras instead of only the one someone remembered.""" + return { + primitive: DEV_PRIMITIVE_MARKERS, + **{k: v for k, v in DEV_BASE_SKILLS.items() if k != DEV_PRIMITIVE_LEGACY}, + } + + def _wt_private_exclude(wt): """The file the git-add shield writes: the exclude in the worktree's OWN gitdir (`.git/worktrees//info/exclude`), never the repo-wide one (#384). @@ -491,28 +520,35 @@ def test_missing_base_skills_reports_absent_and_incomplete(tmp_path): claude = get_profile("claude") # 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) + # the primitive's step-04 invokes them on every run) problems = missing_base_skills(tmp_path, [claude.skill_tree]) assert len(problems) == 3 + assert {p.check for p in problems} == {"skills.base-missing"} 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) assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] + # BASE_SKILLS names both primitive eras, so this tree resolves to the NEW one — + # and the marker checks below have to be made against the dir that resolves. + # Truncating `bmad-dev-auto/` here would be invisible: nothing reads it. + primitive = tmp_path / claude.skill_tree / DEV_PRIMITIVE_NEW + assert resolve_dev_primitive(tmp_path, claude.skill_tree) == DEV_PRIMITIVE_NEW + # remove the dev primitive's step-file marker → reported as incomplete - (tmp_path / claude.skill_tree / "bmad-dev-auto" / "step-04-review.md").unlink() + (primitive / "step-04-review.md").unlink() problems = missing_base_skills(tmp_path, [claude.skill_tree]) - assert len(problems) == 1 + assert [p.check for p in problems] == ["skills.base-incomplete"] assert "incomplete" in problems[0].message assert "step-04-review.md" in problems[0].message # restore it, then drop customize.toml (the review-layer config marker, # BMAD-METHOD #2535/#2550) → a pre-July bmm install is caught as incomplete - (tmp_path / claude.skill_tree / "bmad-dev-auto" / "step-04-review.md").write_text("x\n") - (tmp_path / claude.skill_tree / "bmad-dev-auto" / "customize.toml").unlink() + (primitive / "step-04-review.md").write_text("x\n") + (primitive / "customize.toml").unlink() problems = missing_base_skills(tmp_path, [claude.skill_tree]) - assert len(problems) == 1 + assert [p.check for p in problems] == ["skills.base-incomplete"] assert "incomplete" in problems[0].message assert "customize.toml" in problems[0].message @@ -563,44 +599,58 @@ def test_missing_base_skills_findings_carry_ids_and_detail(tmp_path): assert {f.check for f in absent} == {"skills.base-missing"} assert all(f.severity == "problem" for f in absent) assert all(f.check in VALIDATE_CHECKS for f in absent) + # an empty tree names the CURRENT spelling — the older one appears in the + # message as a hint, never as the thing a consumer keys on assert {f.detail["skill"] for f in absent} == { - "bmad-dev-auto", + DEV_PRIMITIVE_NEW, "bmad-review-adversarial-general", "bmad-review-edge-case-hunter", } assert all(f.detail["tree"] == claude.skill_tree for f in absent) _install_base_skills(tmp_path, claude.skill_tree) - (tmp_path / claude.skill_tree / "bmad-dev-auto" / "step-04-review.md").unlink() - (tmp_path / claude.skill_tree / "bmad-dev-auto" / "customize.toml").unlink() + # BASE_SKILLS names both eras, so the tree resolves to the new name: truncate + # the dir that actually resolves, or the check has nothing to report on + primitive = tmp_path / claude.skill_tree / DEV_PRIMITIVE_NEW + (primitive / "step-04-review.md").unlink() + (primitive / "customize.toml").unlink() incomplete = missing_base_skills(tmp_path, [claude.skill_tree]) assert len(incomplete) == 1 assert incomplete[0].check == "skills.base-incomplete" + assert incomplete[0].detail["skill"] == DEV_PRIMITIVE_NEW # a LIST of markers, not the joined string the message renders assert incomplete[0].detail["missing_markers"] == ["step-04-review.md", "customize.toml"] for marker in incomplete[0].detail["missing_markers"]: assert marker in incomplete[0].message -def test_merged_bmad_review_satisfies_review_layers(tmp_path): +@pytest.mark.parametrize("primitive", [DEV_PRIMITIVE_NEW, DEV_PRIMITIVE_LEGACY]) +def test_merged_bmad_review_satisfies_review_layers(tmp_path, primitive): """#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.""" + every lens itself, so a tree carrying it needs none of the hunters. + + Run against BOTH primitive eras: the substitution is a property of the review + catalog, and a marker-complete pre-rename install is still a supported topology, + so neither spelling may quietly stop satisfying it.""" claude = get_profile("claude") _install_skills( tmp_path, claude.skill_tree, - {"bmad-dev-auto": DEV_BASE_SKILLS["bmad-dev-auto"], "bmad-review": ()}, + {primitive: DEV_PRIMITIVE_MARKERS, "bmad-review": ()}, ) + assert resolve_dev_primitive(tmp_path, claude.skill_tree) == primitive 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") + _shutil.rmtree(tmp_path / claude.skill_tree / primitive) problems = missing_base_skills(tmp_path, [claude.skill_tree]) - assert len(problems) == 1 - assert problems[0].detail["skill"] == "bmad-dev-auto" + assert [p.check for p in problems] == ["skills.base-missing"] + # nothing resolved, so the finding names the CURRENT spelling whichever era + # just went missing — that is the name an operator has to install + assert problems[0].detail["skill"] == DEV_PRIMITIVE_NEW def test_verification_gap_never_required(tmp_path): @@ -709,12 +759,18 @@ def test_review_hunter_missing_without_merged_review_reported(tmp_path): """ -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" +def _install_dev_auto(root, tree, *, skill=DEV_PRIMITIVE_LEGACY, customize="x\n", step04="x\n"): + """Install the dev primitive with real customize.toml / step-04 content, so the + preflight reads the review shape it would read on a real install. + + ``skill`` picks the era's directory NAME and defaults to the pre-rename one, which + is what a lone-primitive tree resolves to. Pass :data:`DEV_PRIMITIVE_NEW` for a + post-rename install, or to overwrite the config of a tree that already resolves + there — writing this content under the legacy dir on such a tree lands it in a + directory nothing reads.""" + d = root / tree / skill d.mkdir(parents=True, exist_ok=True) - (d / "SKILL.md").write_text("# bmad-dev-auto\n", encoding="utf-8") + (d / "SKILL.md").write_text(f"# {skill}\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 @@ -863,10 +919,14 @@ def test_unreadable_customize_falls_back_to_static_catalog(tmp_path): # 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.""" +def _write_override(root, body, *, user=False, skill=DEV_PRIMITIVE_LEGACY): + """A project override of the dev primitive's shipped customize.toml. + + ``skill`` names the era the override file is FOR — `_customize_overrides` reads + the resolved primitive's pair and only that pair, so an override written under + the other spelling is inert (and reported by `dev_primitive_warnings` instead).""" suffix = "user.toml" if user else "toml" - path = root / "_bmad" / "custom" / f"bmad-dev-auto.{suffix}" + path = root / CUSTOMIZE_DIR / f"{skill}.{suffix}" path.parent.mkdir(parents=True, exist_ok=True) path.write_text(body, encoding="utf-8") return path @@ -1129,6 +1189,614 @@ def test_new_review_check_ids_are_registered(): } <= VALIDATE_CHECKS +# --- the bmad-dev-auto -> bmad-build-auto rename ------------------------------ +# +# BMAD-METHOD PR #2651 (bmad-method 6.10.1-next.33) renamed the dev primitive and +# left a forwarding SHIM under the old name: a lone SKILL.md, no step files, no +# customize.toml, whose customization-migration gate is INTERACTIVE. An unattended +# session dispatched into it HALTs having written nothing to disk — no spec, no +# result artifact, nothing the post-session verification can read — so the +# orchestrator resolves the primitive from disk and REFUSES the shim rather than +# driving it. Everything below pins that resolution and what each outcome reports. +# +# The failure mode being guarded is two-sided, and both sides are silent: +# resolving nothing on a healthy renamed project blocks every run behind a +# remediation nobody can apply, and reading the pre-rename paths on a renamed +# project degrades to the static catalog — a green preflight over the wrong +# reviewers and a worktree seeded with skills the session will not invoke. + + +@pytest.mark.parametrize( + ("catalog", "expected", "is_shim"), + [ + ({DEV_PRIMITIVE_NEW: DEV_PRIMITIVE_MARKERS}, DEV_PRIMITIVE_NEW, False), + ({DEV_PRIMITIVE_LEGACY: DEV_PRIMITIVE_MARKERS}, DEV_PRIMITIVE_LEGACY, False), + ( + { + DEV_PRIMITIVE_NEW: DEV_PRIMITIVE_MARKERS, + DEV_PRIMITIVE_LEGACY: DEV_PRIMITIVE_MARKERS, + }, + DEV_PRIMITIVE_NEW, + False, + ), + ({DEV_PRIMITIVE_NEW: ()}, DEV_PRIMITIVE_NEW, False), + ({DEV_PRIMITIVE_LEGACY: ()}, None, True), + ({DEV_PRIMITIVE_LEGACY: DEV_PRIMITIVE_MARKERS[:1]}, None, True), + ({DEV_PRIMITIVE_LEGACY: DEV_PRIMITIVE_MARKERS[1:]}, None, True), + ({}, None, False), + ], + ids=[ + "new-only", + "legacy-complete", + "both-prefers-new", + "new-truncated-still-resolves", + "shim-only", + "legacy-missing-customize", + "legacy-missing-step04", + "nothing-installed", + ], +) +def test_resolve_dev_primitive_matrix(tmp_path, catalog, expected, is_shim): + """The whole resolution matrix in one place. Two deliberate asymmetries live + here, and neither is safe to "simplify" into a uniform rule: + + - the NEW name resolves on its SKILL.md ALONE, while the LEGACY name needs every + marker. Requiring markers of the new name too would make a truncated + bmad-build-auto fall through to a legacy install (or to the shim's message), + reporting a wrong problem instead of the real one; accepting a marker-less + LEGACY install is accepting the forwarding shim, which HALTs the session. + - when BOTH are installed the new name wins outright. On a renamed project the + old directory IS the shim, so "prefer whichever looks complete" would be a coin + flip decided by whatever the upgrade happened to leave behind. + + `_is_dev_primitive_shim` is a MESSAGE selector, never a resolution input: it is + True for a legacy SKILL.md with ANY marker absent — which is also the shape of a + truncated pre-rename install, a case nothing on disk can tell apart. Its two + single-marker rows are here so that stays true for either marker, not just for + the first one a loop happens to check. + """ + claude = get_profile("claude") + tree = claude.skill_tree + _install_skills(tmp_path, tree, catalog) + + assert resolve_dev_primitive(tmp_path, tree) == expected + assert _is_dev_primitive_shim(tmp_path, tree) is is_shim + + +@pytest.mark.parametrize("absent_marker", DEV_PRIMITIVE_MARKERS) +def test_truncated_build_auto_is_incomplete_not_missing_or_shim(tmp_path, absent_marker): + """A bmad-build-auto missing a marker RESOLVES — SKILL.md is enough — and is then + reported against ITSELF as `skills.base-incomplete` ("reinstall this skill"). + + Not `base-missing` ("install or update bmm") and not `base-shim` ("the rename + left a forwarder behind"): both would send an operator after the wrong thing, + since the module is installed and what is on disk is not a shim. This is the + payoff of the resolution asymmetry — the mirror case, a truncated LEGACY install, + is byte-identical to the shim on disk and lands on base-shim instead.""" + claude = get_profile("claude") + tree = claude.skill_tree + markers = tuple(m for m in DEV_PRIMITIVE_MARKERS if m != absent_marker) + # the merged reviewer satisfies the static review fallback, so the ONE finding + # below is the primitive's and nothing else is being counted alongside it + _install_skills(tmp_path, tree, {DEV_PRIMITIVE_NEW: markers, "bmad-review": ()}) + assert not (tmp_path / tree / DEV_PRIMITIVE_LEGACY).exists() + assert resolve_dev_primitive(tmp_path, tree) == DEV_PRIMITIVE_NEW + + problems = missing_base_skills(tmp_path, [tree]) + assert [p.check for p in problems] == ["skills.base-incomplete"] + assert problems[0].severity == "problem" + assert problems[0].detail == { + "tree": tree, + "skill": DEV_PRIMITIVE_NEW, + "missing_markers": [absent_marker], + } + assert absent_marker in problems[0].message + assert f"{tree}/{DEV_PRIMITIVE_NEW} is incomplete" in problems[0].message + + +def test_shim_only_install_is_refused_with_the_halt_hazard_named(tmp_path): + """The install `bmad-loop validate` exists to REFUSE. A lone bmad-dev-auto/SKILL.md + with no markers is (almost always) the forwarding shim, and driving it is worse + than failing: the session HALTs on an interactive prompt having written nothing, + so the run neither succeeds nor produces an artifact anyone can diagnose.""" + from bmad_loop.checks import VALIDATE_CHECKS + + claude = get_profile("claude") + tree = claude.skill_tree + # install_dev_shim also stubs the merged reviewer, so the static review fallback + # is satisfied and the single finding below is the shim's alone + install_dev_shim(tmp_path, tree) + assert resolve_dev_primitive(tmp_path, tree) is None + + problems = missing_base_skills(tmp_path, [tree]) + assert [p.check for p in problems] == ["skills.base-shim"] + assert problems[0].severity == "problem" + assert problems[0].detail == { + "tree": tree, + "skill": DEV_PRIMITIVE_LEGACY, + "expected": DEV_PRIMITIVE_NEW, + "missing_markers": list(DEV_PRIMITIVE_MARKERS), + } + # the message carries BOTH halves of the diagnosis: the rename (so the operator + # knows which skill to install) and the hazard (so nobody "just runs it anyway") + assert DEV_PRIMITIVE_NEW in problems[0].message + assert "rename" in problems[0].message + assert "interactive" in problems[0].message + assert "HALT" in problems[0].message + for marker in DEV_PRIMITIVE_MARKERS: + assert marker in problems[0].message + assert "skills.base-shim" in VALIDATE_CHECKS + + +def test_shim_beside_a_real_build_auto_is_ignored(tmp_path): + """Once the new skill is installed the shim is just a leftover directory — and it + is what the old name IS on every renamed project, so treating its presence as a + problem would fail the preflight on the normal post-upgrade layout.""" + claude = get_profile("claude") + tree = claude.skill_tree + install_dev_shim(tmp_path, tree) + _install_skills(tmp_path, tree, {DEV_PRIMITIVE_NEW: DEV_PRIMITIVE_MARKERS}) + + # the shim really is still on disk and still shim-shaped: the green below is not + # a scaffold that quietly removed the thing under test + assert (tmp_path / tree / DEV_PRIMITIVE_LEGACY / "SKILL.md").is_file() + assert _is_dev_primitive_shim(tmp_path, tree) is True + assert resolve_dev_primitive(tmp_path, tree) == DEV_PRIMITIVE_NEW + + assert missing_base_skills(tmp_path, [tree]) == [] + + +def test_base_missing_names_the_new_skill_and_the_older_spelling(tmp_path): + """An empty tree is reported against the CURRENT name — that is what has to be + installed — with the pre-rename spelling named as a hint, because an operator on + an older bmm will be looking for the old directory in their own install and would + otherwise read the finding as "bmm ships something I don't have".""" + claude = get_profile("claude") + tree = claude.skill_tree + # only the merged reviewer, so the primitive's finding is the only one + _install_skills(tmp_path, tree, {"bmad-review": ()}) + + problems = missing_base_skills(tmp_path, [tree]) + assert [p.check for p in problems] == ["skills.base-missing"] + assert problems[0].severity == "problem" + assert problems[0].detail == {"tree": tree, "skill": DEV_PRIMITIVE_NEW} + assert f"{tree}/{DEV_PRIMITIVE_NEW} not found" in problems[0].message + assert f"older installs name it {DEV_PRIMITIVE_LEGACY}" in problems[0].message + + +def test_trees_resolve_their_primitive_era_independently(tmp_path): + """A project can sit mid-migration: one CLI's skill tree reinstalled from a + post-rename bmm, the other still on the pre-rename one. Each tree's primitive is + resolved from ITS OWN contents, so both are green at the same time.""" + claude, codex = get_profile("claude"), get_profile("codex") + assert claude.skill_tree != codex.skill_tree + _install_skills(tmp_path, claude.skill_tree, _era_catalog(DEV_PRIMITIVE_NEW)) + _install_skills(tmp_path, codex.skill_tree, DEV_BASE_SKILLS) + + assert resolve_dev_primitive(tmp_path, claude.skill_tree) == DEV_PRIMITIVE_NEW + assert resolve_dev_primitive(tmp_path, codex.skill_tree) == DEV_PRIMITIVE_LEGACY + assert missing_base_skills(tmp_path, [claude.skill_tree, codex.skill_tree]) == [] + + # ...and that green is earned per tree rather than by one tree's primitive + # standing in for the other's: removing the new-era one reports THAT tree only + import shutil as _shutil + + _shutil.rmtree(tmp_path / claude.skill_tree / DEV_PRIMITIVE_NEW) + problems = missing_base_skills(tmp_path, [claude.skill_tree, codex.skill_tree]) + assert [(p.check, p.detail["tree"]) for p in problems] == [ + ("skills.base-missing", claude.skill_tree) + ] + + +def test_dev_primitive_or_default_is_total(tmp_path): + """The name-returning form callers build a prompt or a probe path out of. It can + never raise into prompt construction, so every unresolvable tree falls back to the + legacy name — a placeholder for a message, not an endorsement: the preflight has + already refused those trees before any session is spawned.""" + claude = get_profile("claude") + tree = claude.skill_tree + + # nothing installed + assert dev_primitive_or_default(tmp_path, tree) == DEV_PRIMITIVE_LEGACY + # no tree at all — what an adapter with no profile reports + assert dev_primitive_or_default(tmp_path, None) == DEV_PRIMITIVE_LEGACY + # the shim: unresolvable, so still the fallback rather than a crash + install_dev_shim(tmp_path, tree) + assert resolve_dev_primitive(tmp_path, tree) is None + assert dev_primitive_or_default(tmp_path, tree) == DEV_PRIMITIVE_LEGACY + # resolved → the resolved name + _install_skills(tmp_path, tree, {DEV_PRIMITIVE_NEW: DEV_PRIMITIVE_MARKERS}) + assert dev_primitive_or_default(tmp_path, tree) == DEV_PRIMITIVE_NEW + + +def test_dev_primitive_or_default_returns_a_resolved_legacy_install(tmp_path): + """The legacy leg needs its own scaffold to mean anything: the fallback value and + a genuinely-resolved legacy install are the SAME string, so no assertion on the + return value alone can tell them apart. `resolve_dev_primitive` is asserted + alongside it here to pin which of the two produced it.""" + claude = get_profile("claude") + tree = claude.skill_tree + _install_skills(tmp_path, tree, {DEV_PRIMITIVE_LEGACY: DEV_PRIMITIVE_MARKERS}) + + assert resolve_dev_primitive(tmp_path, tree) == DEV_PRIMITIVE_LEGACY + assert dev_primitive_or_default(tmp_path, tree) == DEV_PRIMITIVE_LEGACY + + +_LEGACY_TEAM_TOML = (CUSTOMIZE_DIR / f"{DEV_PRIMITIVE_LEGACY}.toml").as_posix() +_LEGACY_USER_TOML = (CUSTOMIZE_DIR / f"{DEV_PRIMITIVE_LEGACY}.user.toml").as_posix() + + +def _write_customize_files(root, *names): + """Project customization override files, by bare file NAME — so a test can place + one under either era's spelling (and either suffix) without a kwarg matrix.""" + for name in names: + path = root / CUSTOMIZE_DIR / name + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text("[workflow]\n", encoding="utf-8") + + +@pytest.mark.parametrize( + ("files", "expected"), + [ + ((f"{DEV_PRIMITIVE_LEGACY}.toml",), [_LEGACY_TEAM_TOML]), + ((f"{DEV_PRIMITIVE_LEGACY}.user.toml",), [_LEGACY_USER_TOML]), + ( + (f"{DEV_PRIMITIVE_LEGACY}.toml", f"{DEV_PRIMITIVE_LEGACY}.user.toml"), + [_LEGACY_TEAM_TOML, _LEGACY_USER_TOML], + ), + ( + (f"{DEV_PRIMITIVE_LEGACY}.user.toml", f"{DEV_PRIMITIVE_NEW}.toml"), + [_LEGACY_USER_TOML], + ), + ], + ids=["team-orphaned", "user-orphaned", "both-orphaned", "counterpart-is-other-suffix"], +) +def test_orphaned_legacy_customize_warns_once(tmp_path, files, expected): + """The rename silently orphans a project's customization: upstream's resolver keys + on the skill DIRECTORY, so `_bmad/custom/bmad-dev-auto*.toml` stops applying the + moment the tree resolves to bmad-build-auto. The run still works — it just runs + unstyled — so this is an operator heads-up naming the files to rename. + + ONE finding for the whole project (the override files are project-global, not per + tree) listing every orphan, and the counterpart that suppresses it must match on + SUFFIX: a renamed `bmad-build-auto.toml` does not adopt a leftover + `bmad-dev-auto.user.toml`, which is the personal layer — a different file with + different content that nothing has migrated. + + TWO trees are passed for exactly the once-per-project half: an implementation + that emitted per tree would report the same file twice and read as two problems.""" + from bmad_loop.checks import VALIDATE_CHECKS + + claude, codex = get_profile("claude"), get_profile("codex") + trees = [claude.skill_tree, codex.skill_tree] + assert claude.skill_tree != codex.skill_tree + for tree in trees: + _install_skills(tmp_path, tree, {DEV_PRIMITIVE_NEW: DEV_PRIMITIVE_MARKERS}) + _write_customize_files(tmp_path, *files) + + findings = dev_primitive_warnings(tmp_path, trees) + assert [f.check for f in findings] == ["skills.customize-legacy"] + # ⚠ "warning", NOT "problem", and this is the whole reason the check lives in + # dev_primitive_warnings rather than in missing_base_skills: that one feeds a gate + # with no severity filter and no --force. A false FAIL here would pause every run + # behind a remediation that does not apply, over layers that really are inert. + assert findings[0].severity == "warning" + assert findings[0].detail == {"files": expected, "skill": DEV_PRIMITIVE_NEW} + for rel in expected: + assert rel in findings[0].message + assert DEV_PRIMITIVE_NEW in findings[0].message + assert "skills.customize-legacy" in VALIDATE_CHECKS + + +def test_mixed_era_orphan_says_copy_because_the_legacy_tree_still_applies_it(tmp_path): + """A project mid-upgrade carries a different era in each tree, and each tree + resolves its overrides under its OWN era — so `_bmad/custom/bmad-dev-auto.toml` + is orphaned for the new tree and LIVE for the legacy one. + + Still a finding: the new tree really is running unstyled, and suppressing it + there would be the silent degradation this warning exists to surface. But the + remediation flips to COPY, because following a rename would simply move the + customization from the legacy tree to the new one — trading one unstyled tree + for another rather than fixing anything. `legacy_trees` rides in `detail` only + on this branch, so the all-new dict above stays an exact-match oracle.""" + claude, codex = get_profile("claude"), get_profile("codex") + new_tree, legacy_tree = claude.skill_tree, codex.skill_tree + assert new_tree != legacy_tree + _install_skills(tmp_path, new_tree, {DEV_PRIMITIVE_NEW: DEV_PRIMITIVE_MARKERS}) + _install_skills(tmp_path, legacy_tree, {DEV_PRIMITIVE_LEGACY: DEV_PRIMITIVE_MARKERS}) + _write_customize_files(tmp_path, f"{DEV_PRIMITIVE_LEGACY}.toml") + + findings = dev_primitive_warnings(tmp_path, [new_tree, legacy_tree]) + assert [f.check for f in findings] == ["skills.customize-legacy"] + assert findings[0].severity == "warning" + assert findings[0].detail == { + "files": [_LEGACY_TEAM_TOML], + "skill": DEV_PRIMITIVE_NEW, + "legacy_trees": [legacy_tree], + } + # names BOTH trees, so the operator can tell which one still styles the file... + assert new_tree in findings[0].message + assert legacy_tree in findings[0].message + # ...and the remediation is the non-destructive one + assert "COPY" in findings[0].message + assert "rename the override file(s) to match" not in findings[0].message + + +@pytest.mark.parametrize( + ("catalog", "files"), + [ + ( + {DEV_PRIMITIVE_NEW: DEV_PRIMITIVE_MARKERS}, + (f"{DEV_PRIMITIVE_LEGACY}.toml", f"{DEV_PRIMITIVE_NEW}.toml"), + ), + ( + {DEV_PRIMITIVE_NEW: DEV_PRIMITIVE_MARKERS}, + (f"{DEV_PRIMITIVE_LEGACY}.user.toml", f"{DEV_PRIMITIVE_NEW}.user.toml"), + ), + ({DEV_PRIMITIVE_LEGACY: DEV_PRIMITIVE_MARKERS}, (f"{DEV_PRIMITIVE_LEGACY}.toml",)), + ({}, (f"{DEV_PRIMITIVE_LEGACY}.toml",)), + ], + ids=[ + "team-counterpart-present", + "user-counterpart-present", + "tree-still-resolves-legacy", + "nothing-resolves", + ], +) +def test_legacy_customize_does_not_warn(tmp_path, catalog, files): + """Three ways the orphan story does not apply, each of them a false-advisory risk + (the first is exercised for both suffixes, since either can be the one migrated): + + - the operator already renamed the file, so both spellings are on disk; + - the tree still resolves to the LEGACY primitive, so the legacy override is the + one that applies — warning about it would be exactly backwards; + - nothing resolves at all, which is `missing_base_skills`' story to tell + (base-shim / base-missing); an override advisory stacked on top buries it.""" + claude = get_profile("claude") + tree = claude.skill_tree + _install_skills(tmp_path, tree, catalog) + _write_customize_files(tmp_path, *files) + + assert dev_primitive_warnings(tmp_path, [tree]) == [] + + +def test_resolve_review_layers_reads_the_resolved_primitives_customize(tmp_path): + """THE test for the rename: a renamed project's own review layers must still be + read. `resolve_review_layers` resolves the primitive's name from disk instead of + taking it as an argument, so a project whose only primitive is bmad-build-auto + resolves the layers IT configured. + + Reading the pre-rename path unconditionally would return None here and degrade + every caller to the static catalog — a preflight requiring the shipped hunters + instead of this project's reviewer, and a worktree seeded with skills the session + never invokes. Both are silent; neither shows up until a story fails.""" + claude = get_profile("claude") + tree = claude.skill_tree + _install_dev_auto( + tmp_path, + tree, + skill=DEV_PRIMITIVE_NEW, + customize="[workflow]\n" + _layer("house-style", "bmad-some-reviewer"), + ) + + resolved = resolve_review_layers(tmp_path, tree) + assert resolved is not None + assert resolved.source == "customize.toml" + assert resolved.layer_driven is True + assert resolved.required == {"bmad-some-reviewer": ("house-style",)} + assert resolved.active_layers == ("house-style",) + + +def test_review_layers_ignore_the_legacy_dir_on_a_renamed_project(tmp_path): + """The same customize.toml, in the pre-rename directory of a project that resolves + to the new name, is NOT read. The run's own resolver keys on the skill dir, so a + stale `bmad-dev-auto/` left behind by the upgrade configures nothing — and a + preflight that read it anyway would require a layer set no session executes.""" + claude = get_profile("claude") + tree = claude.skill_tree + _install_dev_auto( + tmp_path, + tree, + skill=DEV_PRIMITIVE_NEW, + customize="[workflow]\n" + _layer("house-style", "bmad-new-era-reviewer"), + ) + # byte-identical to the config the test above proved IS read, and this install is + # marker-complete — so it would resolve on its own were the new name absent. Only + # its DIRECTORY differs, which is the entire variable under test. + _install_dev_auto( + tmp_path, + tree, + skill=DEV_PRIMITIVE_LEGACY, + customize="[workflow]\n" + _layer("house-style", "bmad-some-reviewer"), + ) + assert resolve_dev_primitive(tmp_path, tree) == DEV_PRIMITIVE_NEW + + resolved = resolve_review_layers(tmp_path, tree) + assert resolved is not None + assert resolved.required == {"bmad-new-era-reviewer": ("house-style",)} + assert "bmad-some-reviewer" not in resolved.skills() + + # ...and the preflight requires only what the resolved config names + problems = missing_base_skills(tmp_path, [tree]) + assert [(p.check, p.detail["skill"]) for p in problems] == [ + ("skills.review-layer-missing", "bmad-new-era-reviewer") + ] + + +def test_step04_fallback_is_read_under_the_resolved_new_name(tmp_path): + """The pre-consolidation shape survives the rename: a bmad-build-auto whose + customize.toml carries no review_layers falls back to ITS OWN step-04.""" + claude = get_profile("claude") + tree = claude.skill_tree + _install_dev_auto( + tmp_path, + tree, + skill=DEV_PRIMITIVE_NEW, + customize=PRE_LAYER_CUSTOMIZE, + step04=STEP04_NAMED, + ) + + resolved = resolve_review_layers(tmp_path, tree) + assert resolved is not None + assert resolved.source == "step-04-review.md" + assert resolved.layer_driven is False + assert set(resolved.required) == { + "bmad-review-adversarial-general", + "bmad-review-edge-case-hunter", + } + + +def test_step04_under_the_legacy_dir_is_not_the_fallback_after_the_rename(tmp_path): + """The same silent degradation as the customize.toml case, one layer down: a + step-04 naming reviewers under the stale legacy dir must not become the fallback + for a tree that resolves to the new name.""" + claude = get_profile("claude") + tree = claude.skill_tree + _install_dev_auto(tmp_path, tree, skill=DEV_PRIMITIVE_NEW, customize=PRE_LAYER_CUSTOMIZE) + _install_dev_auto( + tmp_path, + tree, + skill=DEV_PRIMITIVE_LEGACY, + customize=PRE_LAYER_CUSTOMIZE, + step04=STEP04_NAMED, + ) + assert resolve_dev_primitive(tmp_path, tree) == DEV_PRIMITIVE_NEW + + # the resolved primitive's own step-04 names nobody, so the shape is UNKNOWN — + # specifically not "the two hunters the legacy dir's step-04 names" + assert resolve_review_layers(tmp_path, tree) is None + + +def test_customize_override_under_the_resolved_name_is_merged(tmp_path): + """`_customize_overrides` is derived from the RESOLVED primitive, so a renamed + project's `_bmad/custom/bmad-build-auto.toml` still overrides its layers — the + project keeps its customization across the rename once the file is renamed too.""" + claude = get_profile("claude") + tree = claude.skill_tree + _install_dev_auto( + tmp_path, + tree, + skill=DEV_PRIMITIVE_NEW, + customize="[workflow]\n" + _layer("blind", "bmad-review"), + ) + _install_skills(tmp_path, tree, {"bmad-review": ()}) + assert missing_base_skills(tmp_path, [tree]) == [] + + # merged, and merged BY ID: the base layer's skill stops being required and the + # override's starts, which a merge that never happened could not produce + _write_override(tmp_path, _layer("blind", "bmad-review-company"), skill=DEV_PRIMITIVE_NEW) + problems = missing_base_skills(tmp_path, [tree]) + assert [(p.check, p.detail["skill"]) for p in problems] == [ + ("skills.review-layer-missing", "bmad-review-company") + ] + + +def test_legacy_named_override_is_inert_and_reported_instead(tmp_path): + """Settled decision: read the resolved name's override pair ONLY, never both eras. + Merging the legacy pair in would make the preflight resolve layers the session + never applies — the exact preflight/run disagreement the on-disk resolution exists + to prevent. The orphan is surfaced as a warning rather than silently honoured.""" + claude = get_profile("claude") + tree = claude.skill_tree + _install_dev_auto( + tmp_path, + tree, + skill=DEV_PRIMITIVE_NEW, + customize="[workflow]\n" + _layer("blind", "bmad-review"), + ) + _install_skills(tmp_path, tree, {"bmad-review": ()}) + _write_override(tmp_path, _layer("blind", "bmad-review-company"), skill=DEV_PRIMITIVE_LEGACY) + + # not merged: the base layer still stands and bmad-review still satisfies it. (The + # override names a skill that is NOT installed, so a merge would have surfaced as + # a review-layer-missing finding — this green is not an absence of observables.) + assert missing_base_skills(tmp_path, [tree]) == [] + # ...and the operator is told the file stopped applying + warnings = dev_primitive_warnings(tmp_path, [tree]) + assert [(f.check, f.severity) for f in warnings] == [("skills.customize-legacy", "warning")] + assert _LEGACY_TEAM_TOML in warnings[0].message + + +def test_review_layers_empty_remediation_names_the_resolved_overrides_file(tmp_path): + """The remediation line tells an operator which file to edit, and it is derived + from the resolved primitive — a hardcoded era would send a renamed project's + operator to a path that does not exist on their disk.""" + claude = get_profile("claude") + tree = claude.skill_tree + _install_dev_auto( + tmp_path, + tree, + skill=DEV_PRIMITIVE_NEW, + customize='[workflow]\n\n[[workflow.review_layers]]\nid = "blind"\ninstruction = ""\n', + ) + + problems = missing_base_skills(tmp_path, [tree]) + assert [p.check for p in problems] == ["skills.review-layers-empty"] + assert f"{tree}/{DEV_PRIMITIVE_NEW}" in problems[0].message + assert f"_bmad/custom/{DEV_PRIMITIVE_NEW}.toml" in problems[0].message + assert _LEGACY_TEAM_TOML not in problems[0].message + + +def test_stories_probe_follows_the_resolved_primitive(tmp_path): + """The dispatch probe runs against the skill this tree would DRIVE. + STORIES_PROBE_SKILL names the legacy FALLBACK only — probing that path outright + would report every up-to-date renamed install as too old to run stories mode.""" + claude = get_profile("claude") + tree = claude.skill_tree + install_build_auto_skill(tmp_path, tree) # step-01 written under bmad-build-auto + assert resolve_dev_primitive(tmp_path, tree) == DEV_PRIMITIVE_NEW + + assert missing_stories_support(tmp_path, [tree]) == [] + + +def test_stories_probe_ignores_step01_under_the_legacy_dir(tmp_path): + """A complete pre-rename install sitting beside the new one does not answer the + probe on its behalf: the new name resolves, so only its own step-01 is read — and + the finding names bmad-build-auto, which is where the file has to end up.""" + from bmad_loop.install import STORIES_PROBE_FILE, STORIES_PROBE_TEXT + + claude = get_profile("claude") + tree = claude.skill_tree + install_build_auto_skill(tmp_path, tree, folder_id=False) + # the whole pre-rename install: marker-complete AND carrying the dispatch step-01 + _install_skills(tmp_path, tree, {DEV_PRIMITIVE_LEGACY: DEV_PRIMITIVE_MARKERS}) + (tmp_path / tree / DEV_PRIMITIVE_LEGACY / STORIES_PROBE_FILE).write_text( + f"route a **{STORIES_PROBE_TEXT}** invocation\n", encoding="utf-8" + ) + + problems = missing_stories_support(tmp_path, [tree]) + assert [p.check for p in problems] == ["skills.stories-dispatch-missing"] + assert problems[0].detail["skill"] == DEV_PRIMITIVE_NEW + assert f"{tree}/{DEV_PRIMITIVE_NEW}/{STORIES_PROBE_FILE}" in problems[0].message + + +def test_provision_worktree_copies_the_renamed_dev_primitive(tmp_path): + """Isolation across the rename. Provisioning unions BASE_SKILLS with the review + layers resolved from the repo, and the layers never name the PRIMITIVE — so + BASE_SKILLS naming bmad-build-auto is the only thing that carries it into the + worktree. Leave it out and the copy silently skips it (the `is_dir` guard swallows + the miss) and every isolated session stalls on an `Unknown command`.""" + wt, repo = tmp_path / "wt", tmp_path / "repo" + claude = get_profile("claude") + assert DEV_PRIMITIVE_NEW in BASE_SKILLS + assert BASE_SKILLS[DEV_PRIMITIVE_NEW] == DEV_PRIMITIVE_MARKERS + + # a post-rename checkout: only the new name is on disk + _install_skills(repo, claude.skill_tree, _era_catalog(DEV_PRIMITIVE_NEW)) + assert not (repo / claude.skill_tree / DEV_PRIMITIVE_LEGACY).exists() + + provision_worktree(wt, [claude], repo) + + primitive = wt / claude.skill_tree / DEV_PRIMITIVE_NEW + assert (primitive / "SKILL.md").is_file() + # the markers came along too, so the worktree's own preflight sees a complete + # install rather than reporting the copy as a truncated one + for marker in DEV_PRIMITIVE_MARKERS: + assert (primitive / marker).is_file() + assert resolve_dev_primitive(wt, claude.skill_tree) == DEV_PRIMITIVE_NEW + assert missing_base_skills(wt, [claude.skill_tree]) == [] + + 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 @@ -1136,9 +1804,14 @@ def test_provision_worktree_copies_derived_review_skill(tmp_path): wt, repo = tmp_path / "wt", tmp_path / "repo" claude = get_profile("claude") _install_base_skills(repo, claude.skill_tree) + # the config has to sit under the dir that RESOLVES: _install_base_skills lays + # both eras down, so this repo resolves to the new name and a layer written into + # `bmad-dev-auto/` would never be read — provisioning would silently fall back + # to the static catalog, which is the exact failure this test exists to catch _install_dev_auto( repo, claude.skill_tree, + skill=DEV_PRIMITIVE_NEW, customize="[workflow]\n" + _layer("house-style", "bmad-review-company"), ) _install_skills(repo, claude.skill_tree, {"bmad-review-company": ()}) @@ -1260,23 +1933,27 @@ def test_missing_stories_support_reports_non_utf8_probe_without_crashing(tmp_pat assert len(problems) == 1 and "not found" in problems[0].message -def test_new_dev_auto_skill_is_additive_for_sprint_mode(tmp_path): - """Scenario 6 additivity: installing the *new* bmad-dev-auto (folder+id +@pytest.mark.parametrize("primitive", [DEV_PRIMITIVE_NEW, DEV_PRIMITIVE_LEGACY]) +def test_new_dev_auto_skill_is_additive_for_sprint_mode(tmp_path, primitive): + """Scenario 6 additivity: installing the *new* dev primitive (folder+id dispatch present) satisfies both preflights — sprint mode's file-existence check (`missing_base_skills`, which never inspects the dispatch content) and stories mode's content probe (`missing_stories_support`). The new skill - breaks neither pipeline.""" - from bmad_loop.install import ( - STORIES_PROBE_FILE, - STORIES_PROBE_SKILL, - missing_stories_support, - ) + breaks neither pipeline. + + Run against both spellings of the primitive: additivity is a property of the + dispatch CONTENT, so it must not depend on which era the tree is on. The step-01 + is written under the dir that resolves — STORIES_PROBE_SKILL names the legacy + FALLBACK only, and the probe runs against whatever `resolve_dev_primitive` + picked.""" + from bmad_loop.install import STORIES_PROBE_FILE claude = get_profile("claude") tree = claude.skill_tree - _install_base_skills(tmp_path, tree) - # upgrade bmad-dev-auto in place to the folder+id dispatch version - step01 = tmp_path / tree / STORIES_PROBE_SKILL / STORIES_PROBE_FILE + _install_skills(tmp_path, tree, _era_catalog(primitive)) + assert resolve_dev_primitive(tmp_path, tree) == primitive + # upgrade the primitive in place to the folder+id dispatch version + step01 = tmp_path / tree / primitive / STORIES_PROBE_FILE step01.write_text("route a **folder+id dispatch** invocation\n", encoding="utf-8") # sprint mode (file existence) is unaffected by the new dispatch content … diff --git a/tests/test_stories_e2e.py b/tests/test_stories_e2e.py index e9ae2cc4..f4d650c9 100644 --- a/tests/test_stories_e2e.py +++ b/tests/test_stories_e2e.py @@ -28,6 +28,10 @@ Scenarios (3) `done_checkpoint` and (5) worktree isolation are covered deterministically at the engine level in test_stories_engine.py; here we prove the end-to-end CLI stack. + +(9) is the post-rename row (BMAD-METHOD #2651): the same two-story happy path +against a project carrying only `bmad-build-auto`, proving the real CLI resolves +the invoked primitive off disk instead of spelling a constant. """ from __future__ import annotations @@ -43,7 +47,7 @@ import pytest import yaml -from conftest import install_dev_base_skills +from conftest import install_build_auto_skill, install_dev_base_skills # Linux-only, not merely non-win32: every fake CLI below is bash + GNU coreutils # (`date +%s%N`; the detached-writer fake also needs setsid(1)) — BSD/macOS date @@ -62,9 +66,13 @@ story="$BMAD_LOOP_STORY_KEY"; folder="$BMAD_LOOP_SPEC_FOLDER" prompt="${1:-}" ts=$(date +%s%N) -mkdir -p "$rd/events" +mkdir -p "$rd/events" "$rd/tasks/$tid" printf '{"ts": %s, "event": "SessionStart", "task_id": "%s", "session_id": "fake-1"}' \ "$ts" "$tid" > "$rd/events/$ts-$tid-SessionStart.json" +# argv as it ARRIVED, after profile render + tmux quoting — the orchestrator's own +# tasks//prompt.txt records the pre-render prompt, so only this file can prove a +# dispatched skill NAME actually reached the binary. +printf '%s' "$prompt" > "$rd/tasks/$tid/fake-prompt.txt" baseline=$(git rev-parse HEAD) # SWEEP triage (`/bmad-loop-sweep`): the triage adapter is a plain GenericAdapter @@ -259,11 +267,16 @@ def _entry(story_id: str, **over) -> dict: return d -def _scaffold(root: Path, entries: list[dict]) -> None: +def _scaffold(root: Path, entries: list[dict], *, install_skills=install_dev_base_skills) -> None: """A committed, clean sandbox: git repo, BMAD config + artifact dirs, the base-skill stubs the stories preflight requires (incl. the folder+id dispatch probe), a stories.yaml + SPEC.md, the fake-CLI profile, and a stories-mode - policy — everything committed so the run-start worktree_clean gate passes.""" + policy — everything committed so the run-start worktree_clean gate passes. + + ``install_skills`` picks the dev-primitive ERA laid on disk: the legacy + `install_dev_base_skills` by default, `install_build_auto_skill` for the + post-rename tree. Both take ``(root, *, folder_id)`` and write the folder+id + dispatch probe under whichever name `resolve_dev_primitive` will pick.""" root.mkdir(parents=True, exist_ok=True) (root / "src.txt").write_text("original\n", encoding="utf-8") (root / ".gitignore").write_text(".bmad-loop/runs/\n", encoding="utf-8") @@ -279,7 +292,7 @@ def _scaffold(root: Path, entries: list[dict]) -> None: (root / "_bmad-output" / sub).mkdir(parents=True, exist_ok=True) (root / "_bmad-output" / sub / ".keep").write_text("", encoding="utf-8") - install_dev_base_skills(root, folder_id=True) # tree matches PROFILE_TOML's skill_tree + install_skills(root, folder_id=True) # tree matches PROFILE_TOML's skill_tree folder = root / SPEC_FOLDER (folder / "stories").mkdir(parents=True) @@ -504,6 +517,38 @@ def test_e2e_two_story_happy_path(tmp_path): assert _commit_count(root) == base + 2 +def test_e2e_two_story_happy_path_build_auto(tmp_path): + """Scenario 9, the post-rename twin of the happy path (BMAD-METHOD #2651): + the project carries ONLY `bmad-build-auto`, so the real CLI has to resolve the + invoked primitive off disk. The fake routes on env vars and spec status and + never on the skill name, so the run lands identically either era — the name + assertion below is the whole difference between this row and the legacy one, + and without it this test would pass with the resolution ablated. + + Asserted against the prompt the FAKE recorded (post-render, post-tmux argv), + not the orchestrator's own prompt.txt: a name that never reached the binary + would still be in prompt.txt.""" + root = tmp_path / "sbx" + _scaffold(root, [_entry("1"), _entry("2")], install_skills=install_build_auto_skill) + base = _commit_count(root) + + proc = _run(root, "run") + assert proc.returncode == 0, proc.stderr or proc.stdout + assert _status(root, "1") == "done" + assert _status(root, "2") == "done" + assert _commit_count(root) == base + 2 + + run_dir = root / ".bmad-loop" / "runs" / _run_id(root) + dispatched = [ + p.read_text(encoding="utf-8") for p in (run_dir / "tasks").glob("*/fake-prompt.txt") + ] + assert len(dispatched) == 2, dispatched # one dev session per story, both recorded + # The fakestories profile renders `{prompt}` verbatim, so the dispatch is the + # literal skill invocation; a codex-shaped profile would rewrite it to `$skill`. + assert all(p.startswith("/bmad-build-auto Spec folder: ") for p in dispatched), dispatched + assert not any("bmad-dev-auto" in p for p in dispatched), dispatched + + def test_e2e_spec_checkpoint_two_leg(tmp_path): root = tmp_path / "sbx" _scaffold(root, [_entry("1", spec_checkpoint=True)]) diff --git a/tests/test_stories_engine.py b/tests/test_stories_engine.py index 0efb0769..f594a402 100644 --- a/tests/test_stories_engine.py +++ b/tests/test_stories_engine.py @@ -7,7 +7,7 @@ import pytest import yaml -from conftest import git, write_spec +from conftest import attach_profile, git, install_build_auto_skill, write_spec from bmad_loop.adapters.base import SessionResult from bmad_loop.adapters.mock import MockAdapter @@ -495,6 +495,41 @@ def test_dev_prompt_repair_leg_is_explicit_spec_resume(project, tmp_path): assert "Story id:" not in prompt # repair is an explicit-spec-file invocation +def test_dev_prompt_spells_the_post_rename_primitive(project, tmp_path, monkeypatch): + """Every leg spells the primitive resolved from the dev adapter's skill tree, + not a hardcoded name (upstream renamed it bmad-dev-auto -> bmad-build-auto, + BMAD-METHOD #2651): fresh folder+id dispatch, its plan-halt tail, and the + inherited repair leg. + + Both the installer and `attach_profile` are load-bearing. `project` installs no + skills and `MockAdapter` carries no `profile`, so either one alone resolves the + tree to None and falls back to the LEGACY name — which the tests above already + pin, and which would make this one green for the wrong reason.""" + setup_stories(project, [entry("1")]) + install_build_auto_skill(project.project, ".claude/skills") + engine, adapter = make_engine(project, []) + attach_profile(adapter) # claude -> .claude/skills + task = StoryTask(story_key="1", epic=0) + + assert ( + engine._dev_prompt(task, None) + == "/bmad-build-auto Spec folder: _bmad-output/epic-1. Story id: 1." + ) + # the plan-halt tail rides the same f-string (third caller of the resolved name) + monkeypatch.setattr(engine, "_plan_halt_leg", lambda task, e: True) + assert engine._dev_prompt(task, None) == ( + "/bmad-build-auto Spec folder: _bmad-output/epic-1. Story id: 1. Halt after planning." + ) + + task.spec_file = str(story_spec(project, "1")) + write_spec(story_spec(project, "1"), "done", "abc") # _reset_spec_for_repair needs it + feedback = tmp_path / "fb.md" + feedback.write_text("boom") + assert engine._dev_prompt(task, feedback).startswith( + "/bmad-build-auto Resume the autonomous dev session on the in-progress" + ) + + # ------------------------------------------------------------- other seams diff --git a/tests/test_sweep.py b/tests/test_sweep.py index e079f858..e7450eb6 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -6,11 +6,13 @@ import pytest from conftest import ( + attach_profile, bundle_dev_effect, bundle_dev_escalates, bundle_review_effect, fault_read_text, git, + install_build_auto_skill, migrate_effect, triage_effect, write_ledger, @@ -1365,6 +1367,42 @@ def test_generic_bundle_prompt_restore_branch_points_at_spec(project): assert "Resume review of the in-review spec" not in fresh_prompt +def test_generic_bundle_prompt_spells_the_post_rename_primitive(project): + """All three bundle legs (restore, fresh implement, repair) spell the dev + primitive resolved off the dev adapter's skill tree, not a hardcoded name — + upstream renamed it bmad-dev-auto -> bmad-build-auto (BMAD-METHOD #2651). + + Both setup lines are load-bearing: the `project` fixture installs no skills + and `MockAdapter` carries no profile, so dropping either one resolves every + leg through `dev_primitive_or_default`'s legacy fallback — which the sibling + tests above already pin, and which would make this one pass for the wrong + reason.""" + install_build_auto_skill(project.project, ".claude/skills") + engine, adapter = make_sweep(project, []) + attach_profile(adapter) # claude -> .claude/skills, where the new name now lives + spec = str(project.implementation_artifacts / "spec-dw-fix.md") + task = StoryTask( + story_key="dw-fix", + epic=0, + dw_ids=["DW-1"], + bundle_file="/run/bundles/fix/intent.md", + spec_file=spec, + restore_patch="/run/artifacts/attempt-dw-fix.patch", + ) + + assert engine._generic_bundle_prompt(task, None).startswith( + "/bmad-build-auto Resume review of the in-review spec" + ) + task.restore_patch = None + assert engine._generic_bundle_prompt(task, None).startswith( + "/bmad-build-auto Implement the deferred-work bundle" + ) + feedback = project.implementation_artifacts / "feedback.md" + assert engine._generic_bundle_prompt(task, feedback).startswith( + "/bmad-build-auto Resume the autonomous dev session" + ) + + def test_sweep_bundle_restore_redrive_reaches_done_and_clears_latch(project, monkeypatch): # T-C + T-D: rearm with a restore patch, resume, land done; assert the dispatched # prompt pointed at the in-review spec, the patch apply seam fired, the dw id