0.9.1: adopt upstream bmad-build-auto rename, harvest spec-frontmatter deferrals (#405) - #406
0.9.1: adopt upstream bmad-build-auto rename, harvest spec-frontmatter deferrals (#405)#406pbean wants to merge 25 commits into
Conversation
BMAD-METHOD #2651 (bmad-method >= 6.10.1-next.33) renamed the dev primitive bmad-dev-auto -> bmad-build-auto and left a forwarding shim under the old name. On an upgraded target project the shim failed `skills.base-incomplete` (it has no step files and no customize.toml), which hard-blocked validate/run/sweep/ resume, and worktree isolation never copied the new skill. Resolve the primitive on disk per skill tree instead of hardcoding a name: prefer bmad-build-auto, fall back to a marker-complete bmad-dev-auto, never accept the shim — its migration gate is interactive and would HALT an unattended session with nothing written to disk. - install: resolve_dev_primitive / dev_primitive_or_default / dev_primitive_warnings; missing_base_skills and missing_stories_support work off the resolved skill; BASE_SKILLS carries both eras so isolation copies whichever is installed - checks: skills.base-shim (problem), skills.dev-renderer + skills.customize-legacy (warnings — a renderer stub without _bmad/scripts/render_skill.py, and an orphaned _bmad/custom/bmad-dev-auto*.toml) - cli: validate names the resolved primitive and reports the new warnings - devcontract: match the no-spec fallback artifact under both result prefixes - policy: document that [dev] skill is the adapter discriminator, not the invoked name — a target project must not edit policy.toml to survive a rename
…405) Phase 1 resolved the dev primitive on disk (bmad-build-auto, else a marker-complete bmad-dev-auto) but every session prompt still spelled the pre-rename name, so an upgraded target project would dispatch a slash command that no longer exists. New `Engine._dev_skill(role="dev")` resolves the invoked NAME from the role's adapter `profile.skill_tree` via `install.dev_primitive_or_default`, memoized per tree — one run can mix trees (dev=claude reads .claude/skills, review=codex reads .agents/skills) and the two can sit on different upstream eras. An adapter with no profile yields tree None and the legacy name, which keeps the existing suite and any resolution-failure path dispatching something that exists. `policy.dev.skill` is untouched: it stays the adapter discriminator `_generic_dev` reads; only the spelled name moves. Threaded through every prompt site: `_review_prompt` (via the REVIEW adapter's tree), the workflow completion-marker filename, all three `_generic_dev_prompt` legs, `StoriesEngine`'s folder+id dispatch + repair leg, and `SweepEngine`'s three bundle legs. `cli._dev_skill_for_role` gives both dry-run previews the same resolution, so a preview cannot promise a dispatch `run` would not make. Tests pin the post-rename spelling on every leg, the per-role/per-tree split (one preview legitimately shows both eras), the profile-less legacy fallback, and the marker prefix. Ablating `_dev_skill` to the hardcoded legacy name fails all five engine-side tests; ablating `_dev_skill_for_role` fails both CLI ones.
`cmd_run` returns from `_dry_run` at cli.py:799 — before `_require_base_skills` at :820 — and `cmd_sweep` has the same shape (:1106 before :1113), so a project whose skills are broken still gets a plausible-looking preview at rc 0. Pre-rename that was merely incomplete: the previewed `/bmad-dev-auto` named a skill that did not exist, and the operator would find out loudly. Post-rename it is misleading, because upstream leaves a forwarding SHIM behind — the slash command resolves, so the preview reads fine and the dispatch it promises would HALT an unattended session on the shim's interactive migration gate. Found by the sandbox E2E, not by the unit suite. All three dry-run entry points now run the same skill probes the real preflight runs and print the resulting FAILs to stderr under a "NOT runnable as-is" banner. The probe lookup moves into a shared `_skill_trees`, so the banner and `_require_base_skills` cannot drift apart — a preview that claims run would abort must key on exactly what makes run abort. The exit code stays 0 on purpose. A dry-run is a diagnostic: refusing to print the schedule would withhold what the operator asked for, and existing callers read rc 0 as "the preview rendered", not as "the project is ready". stdout is untouched, so the schedule stays parseable. Tests cover all three entry points plus the negative — a complete install prints nothing to stderr — so the banner cannot be unconditional and still pass. Removing the three calls fails the three positive tests and leaves the negative one green.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dec2cab73c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for skill in BASE_SKILLS: | ||
| dst = tree_dir / skill | ||
| if dst.exists(): |
There was a problem hiding this comment.
When [scm] isolation = "worktree" is used with the new renderer-stub skill, this copies bmad-build-auto but not its required _bmad/scripts/render_skill.py. _bmad/ is gitignored, so a fresh worktree will not contain the script even though validation found it in the main checkout; because the session runs with the worktree as its cwd, the stub then halts before producing a spec. Provision the renderer dependency automatically alongside the skill rather than requiring users to discover and configure a custom worktree seed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 9ca3ee1.
provision_worktree now merge-seeds the repo's whole _bmad/ surface into the worktree, not
just the renderer script — three things forced that shape:
render_skill.pybare-imports its siblingconfig_utilsoffsys.path[0], so
_bmad/scripts/is one multi-file unit. A curated "copy render.py" seed yields a raw
ModuleNotFoundErrorabove the renderer's owntry/except, which loses even the
HALT: <error>contract line the orchestrator can read.- The renderer also needs
_bmad/config.toml(required layer) plus the usually-gitignored
config.user.toml/custom/layers, so file presence of the script alone is not sufficient
to render. - The copy is per file and copy-when-absent, so a checkout that commits its
_bmad/keeps
every tracked file untouched and only the gitignored layers are filled in.
_bmad/render/ is excluded from the seed (BMAD_SEED_EXCLUDES, skipped before descending) —
its snapshot dirs are keyed on a hash of the project root's absolute path, so seeding the main
checkout's copy would carry its paths in and make parallel sessions race on one tree. It also
gets a /_bmad/render/ git exclude inside the worktree (gated on the worktree actually having a
_bmad/) so the renderer's in-session rewrite can't be swept into a story commit, and init
now gitignores it for the default isolation = "none".
One case your description doesn't cover, added here: the seed can come up short without
failing. A symlinked _bmad/ or _bmad/scripts/ — how a shared BMad install is wired — is
walked by rglob (it is the walk root), so every file under it resolves outside repo_root and
is dropped one at a time by the containment guard; the destination lands empty and provisioning
otherwise reports success. _bmad_scripts_seed_incomplete now reports that through the existing
worktree-seed-skipped journal channel.
Also shipped alongside: a skills.dev-renderer-config validate warning (377fcac) for the
non-worktree half of the same failure — a stub resolved but no _bmad/config.toml at all.
Follow-ups from the cut list of this port are tracked in #407, #408, #409 and #410.
| # success status this gates on) and before the convergence gate, so a | ||
| # converging pass's ledger edit still lands in the story commit. The | ||
| # dev leg's already-filed findings dedup on their fingerprint. | ||
| self._harvest_spec_deferrals(task, rj) |
There was a problem hiding this comment.
When a follow-up review defers a new finding, _review_prompt still explicitly tells it to append that finding directly to deferred-work.md (lines 2626-2629), while the renamed primitive also records it in the spec's deferred: frontmatter. This new harvest then appends a second canonical entry because the agent-written entry lacks the fingerprinted origin: used for deduplication, leaving duplicate sweep work for the same finding. The review prompt should make the ledger orchestrator-only, as the sweep prompts already do.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 8eb05b2 — with one deliberate deviation from the suggested remedy.
The duplicate is real and cannot dedup, for exactly the reason you give: append_entry matches
on origin: and source_spec:, and an agent-written entry carries neither (the format doc
tells agents to write prose origins). Two ledger entries, two sweep bundles, one finding.
Not taken: "make the ledger orchestrator-only, as the sweep prompts already do." That ban is
correct in the sweep prompts and wrong here. The dev/review prompts have to run against both
skill eras. On a pre-BMAD-METHOD#2640 primitive there is no deferred: frontmatter to harvest,
and the session's own flat append is the finding's only record — an outright "do NOT edit the
deferred-work ledger" would silently lose findings on every such project.
So the prompt goes neutral instead: the affirmative append clause is dropped, and the
prohibition on rewriting or reclosing existing entries stays (that is the prevention side of
SweepEngine._verify_review's reclose, unrelated to this duplicate). That removes the
orchestrator-caused double-file without suppressing the old era's only mechanism.
Ablated to confirm the new test bites: restoring the "append them to the deferred-work ledger"
clause turns the prompt test red.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/engine.py`:
- Around line 1772-1777: Update the deferred-work snapshot and rollback flow
around _harvest_spec_deferrals and _defer so harvested spec-deferrals entries
are not lost or retained inconsistently: either refresh the snapshot after
harvesting or exclude harvested entries when restoring it. Preserve the
requirement that ledger entries only describe work remaining in the final tree.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 295f580d-850e-41e5-b9cb-28058b3c20db
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.claude-plugin/marketplace.jsonCHANGELOG.mdmodule.yamlpyproject.tomlsrc/bmad_loop/__init__.pysrc/bmad_loop/checks.pysrc/bmad_loop/cli.pysrc/bmad_loop/data/skills/bmad-loop-setup/assets/module.yamlsrc/bmad_loop/deferredwork.pysrc/bmad_loop/devcontract.pysrc/bmad_loop/engine.pysrc/bmad_loop/install.pysrc/bmad_loop/policy.pysrc/bmad_loop/stories_engine.pysrc/bmad_loop/sweep.pytests/conftest.pytests/test_cli.pytests/test_deferredwork.pytests/test_devcontract.pytests/test_engine.pytests/test_install.pytests/test_plugin_workflows.pytests/test_stories_engine.pytests/test_sweep.py
…rrals (#405) The review prompt told the session to "append them to the deferred-work ledger as NEW entries only". Post-BMAD-METHOD#2640 the primitive already records its `defer` findings in the spec's frontmatter, and `_harvest_spec_deferrals` files those into the same ledger — so every finding landed twice, and the pair can never dedup: `append_entry` matches on `origin:` + `source_spec:` and an agent-written entry carries neither. Two ledger entries, two sweep bundles, one finding. Drop the affirmative clause and keep the prohibition on rewriting existing entries (the prevention side of SweepEngine._verify_review's reclose). Deliberately neutral rather than the sweep prompts' outright "do NOT edit the ledger": on a pre-#2640 skill there is no frontmatter to harvest and the session's own flat append is the only record, so a ban would lose findings.
…405) The harvest docstring claimed the ledger edit "rolls back atomically with the entries it guards (`git reset` on a defer takes both, or neither)". It does not, and the ordering-contract test's docstring repeated the claim. `_defer` snapshots deferred-work.md after the harvest wrote it, resets, then writes the snapshot back — so harvested entries survive a defer by design. That is the correct behaviour, not the bug: `_stash_deferred_artifacts` runs first and moves the spec out of the artifacts dir, so after a defer the `deferred:` frontmatter is no longer where a re-drive would re-harvest it and the ledger entry is the finding's only surviving record. The atomic reading describes the RETRY path, which has no restore. State the asymmetry instead of flattening it, and cover the defer leg — previously untested in either direction.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ca3ee1359
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # the tree the story commit squashes — a ledger entry recording | ||
| # work deferred by an attempt that is later rolled back would be a | ||
| # claim about code that no longer exists. | ||
| self._harvest_spec_deferrals(task, result.result_json) |
There was a problem hiding this comment.
When a completed session reaches done but fails a non-fixable artifact gate (for example, a baseline mismatch), this harvest can create deferred-work.md before _rollback_or_pause resets the attempt. If the project had no tracked ledger at the attempt baseline, the new ledger is untracked under the protected BMAD output directory, and safe_rollback deliberately preserves untracked files there; the rolled-back attempt's finding therefore survives and may be committed by a later successful attempt or consumed by a sweep even though the code it described was discarded. Snapshot/remove the newly harvested ledger state on the Action.RETRY rollback path, or defer harvesting until the attempt has passed verification.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — including the mechanism — and fixed in 20d3dc0 by the first of your two suggested
shapes: snapshot before the harvest, restore on the Action.RETRY rollback path.
Reproduced with the discriminating ablation, which shows git reset semantics alone are not
the whole story:
| Scenario | Ledger after the rollback |
|---|---|
| Untracked (the harvest created it) | survives — entry present, source change reverted |
| Tracked at the attempt baseline (control) | reverted |
Untracked, _protected_relpaths() ablated to () |
file deleted |
The third row is the one that names the proximate cause: _safe_reset passes
keep=(".bmad-loop", *self._protected_relpaths()) and that list includes
implementation_artifacts, where deferred-work.md lives — so safe_rollback computes the
ledger into created and then explicitly skips it. Untrackedness and the keep shield are both
load-bearing. End to end the attempt-1 entry was never re-harvested (attempt 2's spec carried no
deferred: field) yet still landed in the story's squashed commit; dedup does not help, since it
prevents a duplicate, not a stale entry whose code was discarded.
Your second suggestion — defer harvesting until the attempt has passed verification — was not
taken. It reads cleaner but the DEFER path still needs the harvest to run before _defer
stashes the spec out of the artifacts dir, so the call would have to be duplicated onto two
branches, and moving it breaks the explicit ordering contract
test_spec_deferrals_harvested_before_the_dev_decision pins. Snapshot/restore leaves that
contract intact and has the smaller blast radius.
Two implementation notes, both of which the tests caught rather than the reading:
- The snapshot carries a separate boolean alongside the text.
Nonecannot double as "no
snapshot taken" and "the ledger did not exist before the harvest" — the second one is the
common case and requires anunlink, not a no-op. - The restore sits in a
try/finallyaround_rollback_or_pause, because
_pause_for_manual_recoveryalways raises: with the defaultscm.rollback_on_failure
off, a plain post-call restore never runs, and the manualreset --hardthe operator is told
to run would not remove an untracked ledger either.
Five new tests (untracked revert, tracked control, re-harvest on the successful attempt 2, the
pause path, and the worktree-isolation leg), each seen failing under its paired ablation. The
docstrings that stated the invariant unconditionally — including the one this PR's own R1 commit
had just written — now say that the restore, not the reset, is what makes the revert
unconditional.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Amends the existing (untagged) 0.9.1 section in place rather than cutting a new version: the third validate warning, the _bmad/ worktree seed, the review prompt's dropped append clause, and the harvest's rollback revert.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54a5d9b45d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ledger_before = self._ledger_text() | ||
| ledger_snapshotted = True | ||
| self._harvest_spec_deferrals(task, result.result_json) |
There was a problem hiding this comment.
When the host dies after _harvest_spec_deferrals writes the ledger but before the non-fixable RETRY rollback completes, _resumable_session replays the completed result and this local snapshot is taken from the already-harvested ledger. The replay deduplicates the entry, then _restore_ledger restores that same post-harvest text, so the finding survives even though the attempt is rolled back. Fresh evidence beyond the earlier rollback report is this crash-replay path; persist the pre-harvest snapshot with the attempt or otherwise recover the ledger's attempt baseline on resume.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 2c553df — and the finding is worse than reported, which is what
picked the fix.
The mechanism you name is right. _dev_phase re-enters its loop from the top on a replay, and
the two lines that re-arm ledger_before = None / ledger_snapshotted = False run
unconditionally — unlike the baseline capture ten lines above them, which is guarded by
resume_result is None. So the snapshot is re-taken from the dead attempt's post-harvest text,
the re-harvest dedups on the fingerprinted origin:, and the finally writes that text back.
On a tracked ledger it is not "the entry survives" — it is "the fix re-creates it".
baseline_commit is persisted and replay-stable, so _rollback_or_pause's
git reset --hard <baseline_commit> has already reverted the dead attempt's harvest by the
time that finally runs. _restore_ledger then puts the edit back. On that leg 20d3dc0 is
worse than having no restore at all, which is what the second new test pins.
Not taken: "persist the pre-harvest snapshot with the attempt." The ledger is append-only
across a run, so persisting its text in StoryTask grows state.json without bound. Your
comment's second option is the one taken — recover the ledger's attempt baseline on resume — and
it needs no schema growth at all, because the baseline is already persisted. A replayed local
suppresses the snapshot (there is nothing honest to restore; it died with the host), and the
finally calls _drop_ledger_created_since_baseline, which partitions on
task.baseline_untracked:
- listed there → untracked and present at the baseline. Pre-harvest content genuinely
unrecoverable, so hands off rather than guess; the harvest's own fingerprint dedup keeps a
re-harvest quiet. - untracked now and absent from it → this attempt created the file (the first-ever harvest
does exactly this) →unlink. - neither → tracked, and the reset has already spoken. Touching it is precisely what puts the
edit back.
Two things encoded at the call site rather than left for the next reader:
- The untracked probe is an observation, so it degrades. It runs in a
finallythat is
usually propagatingRunPausedout of_pause_for_manual_recovery—
scm.rollback_on_failureis off by default — and aGitErrorraised there would replace
the pause with a git failure. It journals and hands off; theunlinkitself still raises. - One asymmetry, deliberate. On that same default pause path no reset runs, so a tracked
ledger keeps its harvest edit here where the non-crash path would have restored pre-harvest
text. The run is paused and the operator's ownreset --hardreverts it; checking out the
baseline blob from inside afinallythat is already unwinding a pause is the worse trade.
Tests: the two axes were disjoint — every harvest/rollback test ran in one live process, and
every resume/replay test had no deferred: frontmatter. Both new legs compose them via
resume_engine plus a crash raised from _emit("post_dev_verify"); the crash window persists
DEV_VERIFY, not DEV_RUNNING, and still replays because task.spec_file is empty (the
artifact gate failed), so _finish_inflight falls through to _resumable_session.
Ablations: restoring the snapshot on the replayed path turns both new tests red while every
pre-existing harvest/rollback test stays green. A second one — deleting the baseline_untracked shield —
initially passed the whole suite, because nothing covered a ledger untracked and present at
the baseline, where dropping the shield deletes operator content no reset would have touched.
test_crash_replay_never_unlinks_a_ledger_untracked_at_the_baseline now sits on that side of the
discrimination. (25997c4 follows up on that same test: its precondition built the rel with
str(Path.relative_to(...)), which is native-separator, so it self-failed on Windows —
untracked_files returns git's posix rels. Production was never affected; the helper and
task.baseline_untracked are posix on both sides.)
| skill_trees = _skill_trees(project, pol) | ||
| problems = install.missing_base_skills(project, skill_trees) | ||
| if require_stories: | ||
| problems += install.missing_stories_support(project, skill_trees) |
There was a problem hiding this comment.
When the resolved primitive is a renderer stub but _bmad/scripts/render_skill.py or the required _bmad/config.toml is absent, missing_base_skills returns no problem, so _require_base_skills allows the real run to launch. The newly added checks are validate-only warnings even though this module documents that either absence deterministically makes the renderer HALT without a spec, causing every unattended session to fail; add these missing-file conditions to the run-blocking preflight.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in ed28777 — with one correction to the remedy's shape.
The finding is exact: missing_base_skills returned nothing for either condition, so
_require_base_skills let the real run launch. And this module's own comment argued against
itself — "only the script's presence is probed, never uv on PATH" is an argument about a false
green, not a false red. Absence is sufficient for the HALT, which is the only question
a gate asks; that presence is not sufficient for success argues against trusting a pass, not
against blocking on a fail. Both comment blocks now say that instead.
Not taken as written: "add these conditions to the run-blocking preflight." There is nothing
to add there — _require_base_skills has no severity filter at all, it prints every Finding
missing_base_skills returns as FAIL: and aborts. Promoting the two findings' severity in
place would equally have changed nothing, because the preflight never reads
dev_primitive_warnings. The emitters had to move. skills.dev-renderer and
skills.dev-renderer-config now live in missing_base_skills at severity problem, with ids,
messages and detail dicts byte-identical and the stub test extracted to _is_renderer_stub.
dev_primitive_warnings keeps only skills.customize-legacy, which a session genuinely
survives — it runs, just unstyled.
Two knock-ons worth naming:
run --dry-run's "NOT runnable as-is" banner reads the same list, so it picked both
conditions up for free. It was silent on them before, i.e. it previewed a run that could not
possibly produce a result.- This reverses a documented cut. PR #155 disposition: partly superseded by the 0.9.1 renderer port, needs reconciling on main #410's table recorded the same one-liner as declined
because it "would newly abort projects that pass today". That did not survive inspection:
"pass" meant passvalidate, not work — a resolved stub with norender_skill.pyHALTs
every unattended session — and both check ids are new in 0.9.1 and unreleased, so the
promotion takes away no run any project in the field completes today. PR #155 disposition: partly superseded by the 0.9.1 renderer port, needs reconciling on main #410's body now records
the reversal and the shape difference.
Sandbox E2E on the real CLI (fake-adapter profile, no LLM), run against 54a5d9b and against
the fix:
| 54a5d9b | ed28777 | |
|---|---|---|
validate, stub + no render_skill.py |
rc 0, advisory warning | rc 1 |
run, same project |
dispatches a session, then 0 done |
rc 1, refuses at preflight |
run --dry-run, both files absent |
rc 0, stderr empty — the preview reads fine | rc 0, both FAIL: lines on stderr |
On that last row stdout is byte-identical between the two, which is the property the
banner was supposed to preserve and now demonstrably does.
Ablations: deleting the skills.dev-renderer emission turns the new run-abort test red. The
config predicate needs both legs — dropping not (project / CENTRAL_CONFIG_REL).is_file()
leaves the finding firing on the stub alone, so only the clearing assert (write the config →
== []) catches it.
| if _bmad_scripts_seed_incomplete(worktree, repo_root): | ||
| skipped.append(f"{BMAD_DIR}/scripts") |
There was a problem hiding this comment.
Under [scm] isolation = "worktree" with a shared BMAD installation whose _bmad/scripts is symlinked outside the repository, the containment guard skips the renderer files and this branch merely adds _bmad/scripts to the returned skipped list. Engine._open_unit only journals that list and proceeds to dispatch, so the renderer stub runs in a worktree without render_skill.py/config_utils.py and produces a result-less Stop. Treat an incomplete required renderer seed as a provisioning failure for renderer-based primitives rather than an informational skipped seed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in f325920. One correction to the mechanism, one to the remedy.
The path is exactly as you describe: the containment guard drops the symlinked _bmad/scripts
files one at a time (the symlinked dir is walked, because it is the rglob root), the rel
lands in the skipped list, and the engine journaled it and dispatched anyway.
The method is _run_isolated, not _open_unit. _open_unit builds the unit;
_run_isolated is where provision_worktree's skipped list comes back and where the
worktree-seed-skipped line is journaled. That is where the gate went — after the journal line
and before the workspace swap, so the half-seeded worktree stays mounted for the operator to
inspect, which is the courtesy _run_isolated's own docstring already promises for a RunPaused
out of drive.
Not taken: "treat it as a provisioning failure." provision_worktree has no failure
vocabulary at all — every containment violation is a bare continue, and it reports by
returning a list[str] that test_provision_worktree_reports_bmad_scripts_not_seeded pins.
Raising there would put the policy decision (escalate vs defer, notify, pause) in a helper that
owns neither the state machine nor the journal. The engine decides instead, and pauses the
run. Escalate rather than defer, per the doctrine already stated on VerifyOutcome.env_fault:
the fault is identical for every story and no repair session can fix it, so deferring would walk
the whole backlog into the same wall.
PENDING has no legal move to ESCALATED in statemachine.TRANSITIONS, so the phase is set
directly — the same shape as the sibling worktree-open-failed branch, which sets DEFERRED that
way for the same reason. The sentinel is now the exported install.BMAD_SCRIPTS_SEED_REL, so
neither side matches a magic string.
Sandbox E2E on the real CLI, [scm] isolation = "worktree", _bmad/ gitignored and
_bmad/scripts symlinked outside the repo. Preflight passes there (the symlink resolves, so
render_skill.py is a file), which is what makes this reachable only at provisioning:
- 54a5d9b:
run-start → worktree-seed-skipped(['_bmad/scripts']) → worktree-opened → session-start → session-end → run-stop, reporting0 done, 0 deferred, 0 escalated. - f325920:
run-start → worktree-seed-skipped(['_bmad/scripts']) → story-escalated → run-paused(stage=escalation),1 escalated,PAUSED: _bmad/scripts is incomplete in the worktree …. Noworktree-opened, no session started, and the mounted worktree's_bmad/
holdsbmm/+config.tomland noscripts/— the exact tree the renderer would have run in.
One ablation note, because it cost a whole extra test: the obvious gate — escalate on any
skipped seed — passed the entire suite. The happy-path clearing leg produces an empty skipped
list, so it cannot tell a narrow gate from a universal one. test_a_benign_skipped_seed_does_not_pause
arms a real no-op worktree_seed entry so the channel actually fires on the other side of the
discrimination.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15f0a80be4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if rel in untracked: | ||
| ledger.unlink(missing_ok=True) |
There was a problem hiding this comment.
When deferred-work.md is ignored—which is the normal _bmad-output/ layout—verify.untracked_files() omits it because it runs git ls-files --others --exclude-standard. Consequently, after a crash following harvest, this condition is false even when the failed attempt created the ledger, so the stale harvested finding survives the replayed rollback and may later be swept or committed despite its originating code being discarded. Fresh evidence beyond the earlier crash-replay report is that this recovery implementation classifies ignored files as if they were tracked, while the added test exercises only a deliberately non-ignored ledger.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in e370528.
The finding is right about the mechanism and understates the blast radius slightly: both inputs were ignore-blind, not just the now-probe. task.baseline_untracked is captured from verify.untracked_files too, so an ignored ledger was missing from the baseline record and the current probe — it landed in the third bucket ("neither ⇒ tracked ⇒ the reset already reverted it"), which is false three ways over: reset --hard skips ignored paths, safe_rollback runs no git clean at all, and _safe_reset's keep shields the artifacts dir besides.
That symmetry is also why the obvious fix is worse than the bug. I built it first to check: making only the now-probe ignore-aware passes a bug-repro test and deletes an operator's pre-existing ignored ledger, because that one is absent from baseline_untracked for exactly the same reason a harvest-created one is. Nothing in the git signals separates them.
So the baseline record moved off git entirely:
StoryTask.baseline_ledger_present: bool | None, captured besidebaseline_commit/baseline_untrackedas a plainPath.is_file().from_dictdeliberately usesd.get(k) is not Nonerather than the neighbouringbool(d.get(k, False))idiom —Falseis the only value that authorizes a delete, so a task persisted before the field existed has to rehydrate toNone, notFalse.- New
verify.path_tracked(git ls-files -- <rel>, reading only the output's emptiness —core.quotePathmangles non-ASCII). Not--error-unmatch, which reports "untracked" and "git blew up" with the same rc; notcheck-ignore, which answers whether a rule matches rather than whether git owns the path, so agit add -f'd ledger would read wrong. - Only one cell of the truth table moves (absent-at-baseline + ignored: return → unlink). The tracked guard is what keeps it to one: without it the fix introduces a destructive change on a ledger the operator deleted from the worktree without committing — the reset restores it, an unconditional unlink deletes it, and the next attempt's
git add -Acommits the deletion. That leg needed no crash at all, so_restore_ledgerwas carrying the same bug on the strictly-more-reachable live path; both now share_ledger_is_gits_to_restore, which degrades toTrueon aGitError(uncertainty must never authorize a delete).
On the test point — you were right that the existing coverage proved nothing here, and the fixture trap was sharper than it looks: the .gitignore edit has to be committed, because an uncommitted one is a tracked modification that the rollback's own reset --hard reverts, leaving the ledger plain-untracked by the time the classifier runs. The test would then have gone green on the buggy build for entirely the wrong reason. Three preconditions are asserted mid-test (the harvest really fired, the rel is not in untracked_files, git ls-files is empty) so a silently-failed ignore rule self-reports.
Also verified in a sandbox end to end, since the unit suite demonstrably cannot see this: a project-local plugin bound to post_dev_verify os._exits the host in the crash window, then bmad-loop resume replays into the rollback. Pre-fix the run prints 1 done, journals rollback-auto → story-done → run-complete, and leaves the dead attempt's DW-1 sitting in the ledger. Post-fix the journal is identical and the ledger is gone.
| if _bmad_scripts_seed_incomplete(worktree, repo_root): | ||
| skipped.append(BMAD_SCRIPTS_SEED_REL) |
There was a problem hiding this comment.
In the worktree provisioning path, this marker is added whenever the repository happens to contain render_skill.py, regardless of whether any active resolved primitive is actually a renderer stub. Thus a project using a valid pre-render inline primitive, but retaining a shared or symlinked _bmad/scripts tree from another installation, passes the run preflight and then pauses before every dispatch because the engine treats this marker as fatal. Only emit/escalate this condition when an active primitive satisfies _is_renderer_stub, matching the preflight's renderer-specific behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 19d21d5 — with one deliberate departure from the suggested shape.
The diagnosis is exactly right: _bmad_scripts_seed_incomplete keys on two repo-side facts only (the repo has render_skill.py; some file under _bmad/scripts/ has no worktree counterpart) and never reads a SKILL.md, while _run_isolated escalated on bare membership. A project on a pre-#2601 inline primitive that merely carries a symlinked renderer-era _bmad/ got its whole backlog halted on story 1. And preflight cannot pre-empt it, which is the part that makes it stick: skills.dev-renderer and skills.dev-renderer-config are both gated on _is_renderer_stub via resolved_stub, so an inline install passes clean and the run only dies later.
The departure is "emit/escalate": I gated only the escalation, not the reporting. provision_worktree stays a pure reporter and its return value is byte-identical — the sentinel still rides the worktree-seed-skipped channel on an inline project, it just no longer stops the run. Two reasons:
- Provisioning is handed
repo_root, while the dev primitive resolves againstpaths.project— and those genuinely diverge when the BMAD config declaresrepo_root(bmadconfig.py). Answering the era question insideprovision_worktreewould mean threading a third resolution root into it, which is the wrong direction for a bug that is two layers disagreeing about the skill era. - A short seed is still a real fact about the worktree, and the operator should see it journaled either way. What changes is only whether it is fatal — which is the decision the caller that owns the state machine, the journal and
notifywas always making.
So: new public install.renderer_stub_resolved(project, trees) (the resolved_stub flag missing_base_skills already computes, lifted out for the caller that wants the answer without the findings) as a conjunct at the engine gate, deriving its trees from the same profiles local the provisioning call uses — so the gate and the provisioning it judges can never consult different trees. ANY over dev+review rather than dev only: resolution is per tree, a run can mix .claude/skills and .agents/skills, and one stubbed tree means one of the run's session kinds needs the renderer. _is_renderer_stub stays private — it takes a skill dir, so any external caller would have to redo the resolve_dev_primitive + project / tree / resolved dance, and that duplication is this bug's own mechanism.
Tests, since this is where the round earned its keep: deleting the and renderer_stub_resolved(...) conjunct passed all 145 worktree+install tests. The existing test_incomplete_bmad_scripts_seed_pauses_before_dispatch had no skill tree on disk at all — it was asserting a pause with no renderer anywhere — so it is now armed with a real #2601 stub. Two new legs:
test_a_short_scripts_seed_does_not_pause_an_inline_primitive— byte-identical environment, inline primitive, whole backlog must run. Fails three independent ways on the un-gated build.test_a_stub_in_the_review_tree_alone_still_pauses— dev inline, review stubbed. The only ablation that catches implementing the gate as dev-role-only; every other test in the family passes under that bug.
Plus a sandbox E2E on the same environment: fixed build journals worktree-seed-skipped → worktree-opened → session-start and dispatches; reverting just this conjunct turns it into story-escalated → run-paused(stage=escalation) on story 1. The stub scenario still pauses.
…tub (#405) `_bmad_scripts_seed_incomplete` can only see the REPO: it fires on any project carrying a renderer-era `_bmad/scripts/` the seed cannot follow — a shared BMad install wired as a symlink is the canonical shape. `_run_isolated` escalated on bare membership of that sentinel, so a project whose resolved dev primitive is a pre-#2601 INLINE `SKILL.md` had its whole backlog halted on story 1 for a renderer it never invokes. Preflight cannot pre-empt it: `skills.dev-renderer` is itself stub-gated, so an inline install passes clean. New public `install.renderer_stub_resolved(project, trees)` — the `resolved_stub` flag `missing_base_skills` folds into its renderer findings, lifted out for the caller that needs the answer without the findings — becomes a conjunct on the gate. Engine-side rather than provision-side because provisioning is handed `repo_root` while the primitive resolves against `paths.project`, and those genuinely diverge under a configured `repo_root`. ANY over the dev+review trees, deduped: resolution is per tree and either session kind needing the renderer is enough. `provision_worktree` stays a pure reporter; its output is byte-identical. The escalation reason now leads with the evidence the gate can actually back. Tests: `test_incomplete_bmad_scripts_seed_pauses_before_dispatch`'s fixture had no skill tree at all, so it asserted a pause with no renderer on disk — armed with a real stub + `attach_profile`. Two new legs, because deleting the conjunct passed all 145 worktree+install tests: `test_a_short_scripts_seed_does_not_pause_an_inline_primitive` (the era discriminator — byte-identical environment, inline primitive, whole backlog runs) and `test_a_stub_in_the_review_tree_alone_still_pauses` (the only ablation that catches a dev-role-only implementation).
…it (#405) `_drop_ledger_created_since_baseline` classified the deferred-work ledger from `task.baseline_untracked` plus `verify.untracked_files`, and both come from `git ls-files --others --exclude-standard`, which excludes IGNORED files. A gitignored ledger is absent from both, so it fell into the third bucket ("neither ⇒ tracked ⇒ `reset --hard` already reverted it") — false: the reset skips ignored paths, `safe_rollback` runs no `git clean` at all, and the artifacts dir is `keep`-shielded besides. The dead attempt's harvested finding survived the rollback and was later swept or committed. Not hypothetical: this repo gitignores `_bmad-output/`, where the default ledger lives, while `bmad-loop init` does not write that rule — so both layouts exist in the wild. Widening only the NOW probe would have been worse than the bug: an ignored ledger the operator already had is also absent from `baseline_untracked`, so it would classify as attempt-created and be unlinked. The baseline record and the now-probe have to move together. - `StoryTask.baseline_ledger_present: bool | None`, captured beside `baseline_commit`/`baseline_untracked` as a plain `Path.is_file()`. `from_dict` uses `d.get(k) is not None`, NOT the neighbours' `bool(d.get(k, False))`: a pre-upgrade task must rehydrate to None, since False is the only value that authorizes a delete. - New `verify.path_tracked` (`git ls-files -- <rel>`; only the output's emptiness is read). Not `--error-unmatch`, which conflates "untracked" with "git blew up"; not `check-ignore`, which answers whether a RULE matches — a `git add -f`'d ledger must read tracked. - Both ledger methods now share `_ledger_is_gits_to_restore`, which degrades to True: uncertainty must never authorize a delete. `_restore_ledger(None)` needed it too and needs NO crash to reach — on a tracked ledger the operator deleted from the worktree it was unlinking a file the reset had just restored. Verified end to end in a sandbox (crash plugin bound to post_dev_verify → resume → rollback): pre-fix, the run reports `1 done` / `run-complete` with the dead attempt's entry still in the ledger.
Amended in place — 0.9.1 is unreleased on this branch. The harvest-revert bullet claimed the replayed attempt "recovers the ledger's pre-harvest state from the persisted baseline" without qualification, which is the exact sentence the gitignored case falsifies. It now names that case, says the baseline record is a filesystem check rather than a git query, and records that neither path unlinks a ledger git tracks. The worktree `_bmad/` bullet's "a seed that comes up short … pauses the run" gains its condition: only when the resolved dev primitive is a renderer stub. On a pre-BMAD-METHOD#2601 inline SKILL.md the short seed is journaled and the run proceeds.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0321876d32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| if _is_renderer_stub(skill_dir): | ||
| resolved_stub = True | ||
| if not (project / RENDERER_SCRIPT_REL).is_file(): |
There was a problem hiding this comment.
When a renderer-stub installation has render_skill.py and _bmad/config.toml but is missing _bmad/scripts/config_utils.py, this preflight reports no problem and allows the run to start. Fresh evidence beyond the earlier renderer-preflight report is _seed_bmad_tree's documented requirement that render_skill.py bare-imports this sibling; without it, every session fails before it can produce a spec or even the renderer's normal HALT output. Include the helper in the run-blocking renderer dependency check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — fixed in 2e3c560.
Verified by execution, not reading. Full-CLI sandbox (fake adapter, no LLM), renderer stub + render_skill.py + _bmad/config.toml, sibling absent:
at 0321876 |
at 2e3c560 |
|
|---|---|---|
validate |
rc 0 | rc 1, names _bmad/scripts/config_utils.py |
run |
rc 0, dispatches, run-complete |
rc 1, 0 sessions dispatched |
run --dry-run |
rc 0, empty stderr | rc 0, "NOT runnable as-is" banner names the file |
Two deliberate departures from the suggested remedy:
1. Content-keyed, not unconditional. The sibling is required only when the installed render_skill.py actually names config_utils — read the way _is_renderer_stub reads SKILL.md, failing open on OSError/UnicodeDecodeError. _require_base_skills has no severity filter and no --force, this ships in a patch release, and both upstream files have exactly one commit (c2530ea5, BMAD-METHOD#2601, two days old). If a later bmm inlines or renames the helper the import disappears and the gate goes quiet, rather than refusing every run with a remediation nobody can apply. Both directions are pinned: test_renderer_stub_without_config_utils_fails_the_preflight and test_a_renderer_that_does_not_import_the_helper_is_not_gated differ only in the script's text.
2. Reuses skills.dev-renderer; no new check id. A missing entry point and a missing sibling are the same result-less Stop with the same remediation, and checks.py:32-36 splits ids only where the outcomes are genuinely different findings — install.py already calls _bmad/scripts/ "one multi-file unit". detail["script"] becomes detail["missing_scripts"] (a list, like missing_markers). That key removal is safe under documents.py's additive-only contract only because it has no shipped consumer: git show v0.9.0:src/bmad_loop/checks.py has none of the renderer ids, so nothing at 0.9.0 reads it.
Your "or even the renderer's normal HALT output" is the precise part, and it caught a false claim elsewhere: CHANGELOG.md said "Either way the stub's uv run exits HALT:", which is untrue for this leg — the ModuleNotFoundError fires at import, above the renderer's except (ConfigError, RenderError, OSError, UnicodeError, ValueError). Amended in 87d71af.
Ablated before restoring: dropping RENDERER_CONFIG_UTILS_REL from the unit tuple fails 2 tests; making the content key unconditional fails 1; hoisting the check out of the _is_renderer_stub gate fails 1.
| return any( | ||
| src.is_file() and not (dst_scripts / src.relative_to(scripts)).is_file() | ||
| for src in scripts.rglob("*") |
There was a problem hiding this comment.
With worktree isolation and an individually symlinked _bmad/config.toml whose target is outside the repository, the main preflight accepts the symlink via is_file(), but _seed_bmad_tree drops it at the containment guard. This completeness check only compares _bmad/scripts, so the engine receives no fatal seed marker and dispatches the renderer in a worktree lacking its required central config, causing every session to halt without a spec. Fresh evidence beyond the earlier symlinked-scripts report is this independently required config-file path; report it as an incomplete renderer seed too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — fixed in 2e3c560.
Measured against the real functions, _bmad/scripts/ a real dir and _bmad/config.toml symlinked outside the repo:
(repo/_bmad/config.toml).is_file() = True <- preflight follows the symlink
_seed_bmad_tree returned = ['_bmad'] <- "the whole tree is ours"
worktree has _bmad/config.toml = False
_bmad_scripts_seed_incomplete(wt, repo) = False <- no sentinel, engine never fires
missing_base_skills(repo, [tree]) = []
One notch worse than reported: the containment drop is a bare continue, so there is no worktree-seed-skipped line either — the run is silent from every angle, and the one signal provisioning does emit claims the whole tree was seeded. Full-CLI sandbox E2E confirms it end to end: at 0321876 that project journals worktree-opened -> ... -> story-done -> run-complete with 0 escalated; at 2e3c560 it journals worktree-seed-skipped ['_bmad/config.toml'] -> story-escalated -> run-paused, 0 sessions dispatched, worktree left mounted with scripts/ whole and no config.
Three departures:
1. Two predicates behind a shared sentinel tuple, not one widened comparison. RENDERER_SEED_SENTINELS = (BMAD_SCRIPTS_SEED_REL, CENTRAL_CONFIG_REL); the engine intersects it and the escalation reason names whichever leg fired. The two have different remediations, and sending an operator to _bmad/scripts when it is intact is exactly the drift the shared constant exists to prevent. _bmad_scripts_seed_incomplete's docstring stays untouched and each predicate keeps its own ablation.
2. No era gate on the config predicate. The scripts gate exists to bound an rglob and to assert "this is a renderer-era scripts dir"; here the premise is one named file's presence in the repo. Provisioning is a pure reporter — renderer_stub_resolved in the engine discriminates era for both sentinels in one place, and that conjunct is still pinned (deleting it fails the inline-primitive test).
3. One extra fix rides the same commit (C3), not from this review. Found while validating yours and confirmed by running provision_worktree: a user worktree_seed entry spelling a sentinel exactly lands in skipped the moment the checkout already carries the path, and the _is_under_bmad strip at the old install.py:1025 is conditional on the merge having seeded something — which a checkout that commits its whole _bmad/ never does. A forged sentinel then CRITICAL-escalates and RunPauseds a healthy run. Unreleased (the sentinel is this PR's), but adding _bmad/config.toml doubles the colliding surface — it is the more natural worktree_seed entry of the two — so the reservation ships here.
engine.py's sentinel branch had never executed on Windows CI (every test arming it is skipif win32), and this PR already shipped one POSIX-invisible bug. The new predicate-level and sentinel-reservation tests are symlink-free, so they now do.
Out of scope and filed as #414: under repo_root != project the preflight probes project/_bmad/... while provisioning seeds and probes repo_root/_bmad/..., so both legs of this check go inert for such a project — the split turns out to predate the renderer surface and hits the hook relay and base-skill seeding on main too.
Ablated before restoring: dropping the config append fails 3 tests; dropping CENTRAL_CONFIG_REL from the sentinel tuple fails 2; making the reason always name the scripts sentinel fails 1; dropping the forgery filter fails 1; relaxing the worktree probe to exists() fails 1; dropping the repo-side conjunct fails 3.
Codex round R10. The preflight and the worktree completeness check should cover the same three project-global files the renderer needs; today the preflight covers two and the completeness check one. C1 (Codex P1) — `_bmad/scripts/config_utils.py` joins the run-blocking preflight. `render_skill.py` imports it at module scope off sys.path[0], above its own `except (ConfigError, RenderError, OSError, UnicodeError, ValueError)`, so a half-present unit dies on a bare ModuleNotFoundError: the same result-less Stop, with the structured `HALT: <error>` line replaced by a traceback. One check id for both members (same finding, same remediation); `detail["script"]` becomes `detail["missing_scripts"]`, which has no shipped consumer — no renderer id exists at v0.9.0. Content-keyed on purpose: the sibling is required only when the INSTALLED `render_skill.py` actually names it, read the way `_is_renderer_stub` reads SKILL.md and failing open the same way. This gate has no severity filter and no `--force`, it ships in a patch release, and both upstream files are one commit old (c2530ea5, BMAD-METHOD#2601) — if a later bmm inlines or renames the helper the gate must go quiet, not refuse every run with a remediation that cannot fix it. C2 (Codex P2) — the seed completeness check gains a `_bmad/config.toml` leg. A repo that centralises only that file behind a symlink seeds a whole `_bmad/scripts/` and no config: the contain guard drops it with a bare `continue`, `_seed_bmad_tree` still reports the whole tree as seeded, and the repo-side preflight follows the symlink and passes. Measured end to end at 0321876: no journal line at all, the run dispatches, `run-complete`. The engine now intersects RENDERER_SEED_SENTINELS and names whichever leg fired — sending an operator to `_bmad/scripts` when it is intact is the drift the shared constants exist to prevent. C3 (found while validating C2) — the sentinels are reserved. A user `worktree_seed` entry spelling one exactly lands in `skipped` the moment the checkout already carries the path, and the `_is_under_bmad` strip is conditional on the merge having seeded something, which a checkout that commits its whole `_bmad/` never does. Confirmed by running provision_worktree: a forged sentinel CRITICAL-escalates a healthy run. Unreleased (the sentinel is this PR's), and C2 doubles the colliding surface, so it ships here. Verified by full 4-scenario sandbox E2E, each run twice (HEAD vs install.py/engine.py at 0321876) — the unit suite cannot see a renderer HALT. At 0321876 scenario 1 dispatches and reports `run-complete` with the sibling absent, and scenario 3 shows no `worktree-seed-skipped` line at all. Ten ablations, each seen to fail its paired test before restoring.
The preflight bullet said "Either way the stub's `uv run` exits `HALT:`", which C1 makes false for the new leg: a missing `config_utils.py` raises ModuleNotFoundError above the renderer's guard, so there is no `HALT:` line. Names the script unit and the content key. The worktree bullet gains the `_bmad/config.toml` leg and the sentinel reservation.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87d71af7a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {"tree": tree, "skill": resolved, "missing_markers": absent}, | ||
| ) | ||
| ) | ||
| if _is_renderer_stub(skill_dir): |
There was a problem hiding this comment.
When [adapter.triage] uses a different skill tree from dev/review, _skill_trees passes that tree into this check even though triage runs bmad-loop-sweep, not the dev primitive. If that unused tree contains a renderer-stub skill while the actual dev/review trees use legacy inline skills, missing _bmad/scripts or _bmad/config.toml now causes run and sweep to abort despite no dispatched session needing the renderer. Limit the renderer dependency checks to the dev/review trees, matching _worktree_profiles, while retaining any intentional all-role base-skill validation separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1e564e3 — with one deliberate departure from the suggested remedy.
The finding is right, and it is broader than the renderer checks: every skills.* check asks
a dev-primitive question — which primitive resolves, whether the three review hunters step-04
invokes inline are installed, whether a stub's renderer unit is whole. Triage's entire prompt
surface is /bmad-loop-sweep, which this wheel bundles (install.MODULE_SKILLS) and bmad-loop init lays into that tree, so the skills checked and the skills triage needs have empty
intersection. Reproduced on the exact config you describe (dev/review=claude on a healthy inline
primitive, [adapter.triage] name = "gemini", a renderer stub in .agents/skills, no _bmad/):
validate rc 1 and _require_base_skills False before, rc 0 and True after; the identical stub
moved into .claude/skills still fails both, so the gate is scoped, not blinded.
Not taken: "limit the renderer dependency checks to the dev/review trees, retaining the all-role
base-skill validation separately." skills.dev-renderer-config is emitted once per project off
a resolved_stub flag initialised above the per-tree loop, flipped inside it, and read after it
closes — the same loop that emits skills.base-*. It does not split cleanly, and splitting would
undo 0b61083, which deliberately moved the renderer checks into missing_base_skills because
each is a deterministic HALT-without-writing-a-spec. So the narrowing is applied at the root
instead, via a new install.DEV_PRIMITIVE_ROLES = ("dev", "review") — the same set
_worktree_profiles provisions, which is the correspondence you point at, now asserted from both
ends rather than coincidental.
Worth separating for the release note: the all-role base-skill leg is pre-existing on
release/0.9.x — what this PR added is the two renderer checks, which are problems rather than
warnings, so they turned an old over-breadth into a hard block. A config 0.9.0 merely nagged about
became unrunnable on 0.9.1, which is the wrong direction for a hotfix.
ROLES itself is unchanged and stays all-role: _make_adapters, policy.model-qualified and
cmd_init all legitimately need triage — cmd_init is the site that lays the bundled sweep skill
into that tree, which is the self-sufficiency the new constant asserts.
Your description covers _skill_trees; cmd_validate reproduces the defect twice more,
independently, because it builds its own all-role list — the base-skills block and
_validate_stories_queue's argument. All three now share one dev_trees local. One extra
tightening fell out: the skills.base ok line gated on profiles, and policy never validates an
adapter name (the first test is get_profile), so [adapter] name = "nosuchcli" beside a
loadable [adapter.triage] left profiles non-empty while nothing dev-side resolved — printing
upstream skills present ( + review hunters), a green sentence assembled from an empty probe.
Seven tests, each seen failing with its own leg reverted, and the two required non-interactions
confirmed by execution rather than by reading (leg 2 reverted does not fail the stories test; leg
3 reverted does not fail the base-skills test — that policy has no [stories] table, so the line
is unreachable). Both controls were ablated too: _skill_trees on a split dev/review pair goes
red under DEV_PRIMITIVE_ROLES = ("dev",), and _require_base_skills on a dev-tree renderer stub
goes red when the renderer checks are deleted outright — without that second one, removing the
gate rather than scoping it would have passed the new test.
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 — but the tree list was built from all three adapter roles. Triage's entire prompt surface is `/bmad-loop-sweep`, which this wheel bundles and `bmad-loop init` lays into that tree, so the skills checked and the skills triage needs have empty intersection. `[adapter.triage] name = "gemini"` beside a claude dev/review pair therefore demanded the whole bmm module in `.agents/skills`. The over-breadth is pre-existing on release/0.9.x; what makes it a hotfix is that #405's two renderer checks are problems rather than warnings, so a config 0.9.0 merely nagged about hard-blocks run/sweep/resume on 0.9.1. New `install.DEV_PRIMITIVE_ROLES = ("dev", "review")` — the same set `Engine._worktree_profiles` provisions, so what is gated and what a worktree carries stay one decision. `ROLES` is unchanged: `_make_adapters`, `policy.model-qualified` and `cmd_init` all legitimately need triage, and `cmd_init` is what lays the bundled sweep skill into that tree. Three legs, since `cmd_validate` reproduced the defect independently by building its own all-role list: - `_skill_trees` (gates `_require_base_skills` and the dry-run banner) - `cmd_validate`'s base-skills block, now fed a shared `dev_trees` local - `_validate_stories_queue`'s argument The `skills.base` ok line additionally gates on `dev_trees` rather than `profiles`: policy never validates an adapter name, so an unknown dev CLI beside a loadable triage one left `profiles` non-empty while nothing dev-side resolved, printing "upstream skills present ( + review hunters)" — a green sentence assembled from an empty probe. It can only tighten. Departure from the reported remedy: the renderer checks are not split out. `skills.dev-renderer-config` is once-per-project off a `resolved_stub` flag accumulated inside the same loop that emits `skills.base-*`, so it does not split cleanly, and splitting would undo 0b61083. Narrowed at the root instead. Seven tests, each seen failing with its own leg reverted; the two controls (`_skill_trees` on a split dev/review pair, `_require_base_skills` on a dev-tree renderer stub) were seen failing under `DEV_PRIMITIVE_ROLES = ("dev",)` and under deletion of the renderer checks respectively.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
bmad-loop/src/bmad_loop/install.py
Lines 1170 to 1172 in 1e564e3
Abort when required symlinked skills cannot be provisioned
When worktree isolation uses an active primitive or review-hunter directory that is an ignored/untracked symlink to a shared installation outside the repository, the run preflight follows that symlink and accepts the skill, but this containment guard silently skips copying it. The fresh worktree therefore lacks a skill that the engine later dispatches, causing an unknown-command stall or failed review; report the incomplete skill seed to the caller and pause before dispatch instead of continuing.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review finding on #406. `provision_worktree` copies BASE_SKILLS from the main repo's tree behind a `src.resolve().is_relative_to(repo_root)` guard whose two legs are not alike. `not src.is_dir()` means the repo genuinely lacks the skill, which the run-start preflight already refused — skipping is right. A containment failure does not: the skill IS there, as a symlink to a shared machine-wide BMad install, and `missing_base_skills` stats through the link and passes. The guard's comment claimed the preflight covered both legs; it covers only the first. Measured, with the symlink as the only variable: a symlinked `.claude/skills/bmad-build-auto` leaves `missing_base_skills` empty and `resolve_dev_primitive` naming the primitive, while the worktree receives NOTHING and provisioning reports success. Every session then stalls on `Unknown command` having written nothing — the failure the preflight exists to prevent, reached by a project that passed it. Same environment-fault shape as the renderer sentinels: the seed reads the same repo for every unit, so no repair session fixes it and no later story escapes it. New `install.base_skills_seed_incomplete` reports the rels through the existing `worktree-seed-skipped` channel and the engine escalates before dispatch, naming the skills rather than the renderer surface. Two deliberate differences from the renderer legs: - NO era gate. A missing skill stalls a session whether its SKILL.md renders or is inline, so gating on `renderer_stub_resolved` would let the whole pre-#2601 world dispatch into the stall. - The engine re-probes instead of reading `skipped_seeds` back, so a user `worktree_seed` entry spelling a skill rel cannot forge a pause — which is why this needs no equivalent of the RENDERER_SEED_SENTINELS forgery filter. Gated on the repo having the skill and the worktree lacking it, so a committed (checked-out) symlink and a tracked skill tree are both unaffected. Pre-existing since 1c17828 (0.6.5) and byte-identical on release/0.9.x — not a #405 regression, fixed here because #405 owns this seam this cycle. Three tests, each seen failing under its own ablation: dropping the report kills the renderer-era one, deleting the escalation kills both escalation tests, and era-gating it the way the sentinel legs are kills the inline one alone. The must-not-fire control dies when the worktree conjunct is dropped from the predicate. The four existing seed tests stay green under all four ablations.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Confirmed and fixed in 897988f. Reproduced with the symlink as the only variable between two
So the diagnosis is exact. What makes it worse than "a skill is missing" is that the guard's two Fixed as the skills half of the shape the renderer sentinels already use: a new
Gated on the repo having the skill and the worktree lacking it, so a committed symlink One correction to the framing: this is pre-existing, not something this PR introduced — the Each test was seen failing under its own ablation: dropping the report kills the renderer-era Adjacent, not fixed here: the same silent-containment shape exists at the |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 897988f0af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for skill in BASE_SKILLS: | ||
| if (repo_root / tree / skill).is_dir() and not (worktree / tree / skill).is_dir(): | ||
| missing.append(f"{tree}/{skill}") |
There was a problem hiding this comment.
When both primitive names exist in the main checkout but the worktree already contains the selected bmad-build-auto while the unused legacy directory is omitted (for example, the new skill is tracked and the leftover bmad-dev-auto shim is an untracked symlink to a shared install), this loop reports the legacy directory missing because it checks every entry in BASE_SKILLS. Engine._open_unit treats any returned path as fatal and pauses before dispatch even though every prompt resolves to the available new primitive. Limit this completeness gate to the primitive returned by resolve_dev_primitive (plus genuinely required review skills), rather than every copy-if-present compatibility entry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 7344440 — with one deliberate departure from the suggested remedy, and the finding understates its own blast radius.
Reproduced end to end, not just read. Repo with an ordinary bmad-build-auto dir beside a bmad-dev-auto symlinked to a shared install outside the repo, driven through the real Engine.run():
missing_base_skills(repo, [tree]) -> [] # preflight GREEN
dev_primitive_or_default(repo, tree) -> bmad-build-auto # what every prompt spells
worktree has bmad-build-auto/SKILL.md -> True # dispatch target present
base_skills_seed_incomplete(...) -> [.claude/skills/bmad-dev-auto]
dispatched: [] summary.paused: True escalated: 1 phase: escalated
Control with the legacy as an ordinary dir reaches dispatch, so the variable is isolated. Your literal scenario (new skill tracked, shim an untracked symlink) reproduces identically.
Root cause, stated precisely: BASE_SKILLS is a copy-if-present catalog — its own comment says "every non-bundled skill that MIGHT need copying" — and it names both eras because naming both is free when the only question is "copy it if it is there". Reading it as a requirement set demands one skill too many. The gate now skips the era the run will not invoke, resolved through dev_primitive_or_default, the same call Engine._dev_skill makes to spell the prompt — so what is gated and what is dispatched stay one decision.
Worse than reported — the mirror case. A resolved legacy install beside a SKILL.md-less bmad-build-auto dir (an aborted install) escalates too: nothing can resolve that dir and the preflight never stats it, but is_dir() is all the gate asked. Same one-line fix covers both directions; both now have tests.
The departure: bmad-review stays gated, against "plus genuinely required review skills". It is outside the preflight so pre-merge bmm installs keep validating — but on a merged-lens install the three hunter ids are thin forwarders to it, so a worktree lacking one the repo HAS really does break those forwards. Preflight-optional and seeded-or-not are different questions, and the repo-has-it conjunct already keeps the pre-merge case silent. There is a test pinning that decision whose ablation is your suggested shape.
Two corrections to the finding. The site is Engine._run_isolated, not _open_unit (that function does not exist). And the pause is not story-scoped: RunPaused(..., PAUSE_ESCALATION, ...) unwinds to Engine.run, so the entire remaining backlog never dispatches.
Four new tests, each shipped with the ablation that reddens it: unused era ignored (delete the era skip), unresolvable new-primitive dir ignored (same), dropped LEGACY primitive still reported (hardcode the skip to the legacy name — the bound that stops "scope to the resolved primitive" degenerating into "always check the new name"), dropped bmad-review still reported (skip it — your literal remedy). _symlink_skill_tree gained an explicit skills list plus a _real_skill_dirs sibling: it symlinked the whole dispatched set at once, so the resolved primitive was always among the casualties and no fixture could express the mixed shape — which is why this shipped green last round.
That matrix also caught a decorative assert in the previous round's own tests: ..._pauses_an_inline_primitive_too checked only for the .claude/skills/ prefix, which the three hunters satisfy alone, so it stayed GREEN under an ablation that stopped checking the primitive entirely. It now names bmad-build-auto, and reddens.
Full suite 2837 passed / 1 skipped; trunk check clean. Two unrelated crash levers found while probing that loop are filed separately as #416 and #417 — both pre-existing on release/0.9.x.
…#405) Codex review finding on #406, against the gate the previous round added. `base_skills_seed_incomplete` iterated all of `BASE_SKILLS` and the engine treats any rel it returns as a CRITICAL escalation that pauses the whole run before dispatch. But `BASE_SKILLS` is a copy-if-PRESENT catalog — "every non-bundled skill that MIGHT need copying" — and it names both primitive eras precisely because naming both is free when the only question is "copy it if it is there". Read as a requirement set it demands one skill too many. Measured end to end, both directions. A repo whose `bmad-build-auto` is an ordinary dir beside a `bmad-dev-auto` symlinked to a shared machine-wide install passes the preflight, seeds the worktree with every skill a session names, and was still escalated — over a shim no prompt spells, since `Engine._dev_skill` resolves the name from disk and picks the new one. The mirror case escalated too: a resolved legacy install beside a `SKILL.md`-less `bmad-build-auto` dir, which nothing can resolve and the preflight never stats, but `is_dir()` sees. Both halt the backlog with a reason claiming a stall that cannot happen. The gate now skips the era this run will not invoke, resolved through `dev_primitive_or_default` — the same call the engine makes to spell the prompt, so what is gated and what is dispatched stay one decision. That is the doctrine the tree-scoping round already set: a skill gated here that no session reads refuses runs over nothing. `bmad-review` stays gated, against the finding's suggested shape. It is outside the preflight so pre-merge bmm installs keep validating, but on a merged-lens install the three hunter ids are thin forwarders to it, so a worktree lacking one the repo HAS really does break those forwards; the repo-has-it conjunct already keeps the pre-merge case silent. Preflight-optional and seeded-or-not are different questions. Not a released regression: both the two-era `BASE_SKILLS` and this gate landed in this PR. Tests. Four new ones, each with the ablation that reddens it: the unused era is ignored (delete the era skip), an unresolvable new-primitive dir is ignored (same), a dropped LEGACY primitive is still reported (hardcode the skip to the legacy name — the bound that stops "scope to the resolved primitive" degenerating into "always check the new name"), and a dropped `bmad-review` is still reported (skip it, the finding's literal remedy). Plus an engine-level run that must reach DONE. `_symlink_skill_tree` gains an explicit `skills` list and a `_real_skill_dirs` sibling: it symlinked the whole dispatched set at once, so the resolved primitive was always among the casualties and no fixture could express the mixed shape — which is why the over-breadth shipped green. One existing assert tightened. `..._pauses_an_inline_primitive_too` checked only for the `.claude/skills/` prefix, which the three hunters satisfy on their own, so it stayed GREEN under an ablation that stopped checking the primitive entirely. It now names `bmad-build-auto`, and reddens.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7344440fd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (repo_root / tree / skill).is_dir() and not (worktree / tree / skill).is_dir(): | ||
| missing.append(f"{tree}/{skill}") |
There was a problem hiding this comment.
When a fresh worktree already contains only part of the selected skill directory—for example, a tracked bmad-build-auto/SKILL.md while its ignored step-04-review.md and customize.toml exist only in the main checkout—provision_worktree skips copying the entire destination, but this check treats any directory as complete. The main-checkout preflight therefore passes, this runtime gate returns no missing skill, and the session is dispatched with required files absent. Compare the resolved primitive's marker files, and at least SKILL.md for review skills, rather than checking only is_dir().
Useful? React with 👍 / 👎.
1 participant
Patch release 0.9.1, cut from tag
v0.9.0intorelease/0.9.x. Everything onmainsince the tag is next-major work and must not ship in a patch, so this hotfix is a
standalone line.
Tracking issue: #405. That issue stays open here — the follow-up forward-port PR to
maincarries the closing keyword.
Why
BMad Method 6.10.1-next.33 renamed the dev primitive
bmad-dev-auto→bmad-build-auto(BMAD-METHOD#2651) and left a forwarding shim under the old name: a lone
SKILL.mdwhosecustomization-migration prompt is interactive, so an unattended session HALTs on it with
zero disk writes. On an upgraded target project,
bmad-loop validatefailsskills.base-incomplete, the same preflight hard-blocksrun/sweep/resume, and worktreeisolation never copies the new skill directory.
Two other upstream changes rode the same window:
SKILL.mdis now a renderer stub(BMAD-METHOD#2601 — HALTs when
_bmad/scripts/render_skill.pyis absent), anddefer-triaged reviewfindings moved into the spec's frontmatter
deferred:list instead ofdeferred-work.md(BMAD-METHOD#2640), which silently starves the sweep pipeline.
npm
latest(6.10.0) is still pre-rename, so 0.9.1 has to work against both skill eras.What
bmad-build-auto, fall back to amarker-complete
bmad-dev-auto, never accept the shim (newskills.base-shimcheck).[dev] skillinpolicy.tomlis unchanged: it is the adapter discriminator, not theinvoked name, so no target project has to edit config to survive the rename.
dispatch, all three sweep bundle legs, the fallback result marker, and both dry-run
previews. Resolution is per skill tree, so a run mixing
.claude/skillsand.agents/skillsgets the right era per role.--dry-runnow says when its preview is not runnable.cmd_run/cmd_sweepreturnfrom the dry-run branch before
_require_base_skills, so a broken install still got aplausible-looking schedule — and post-rename the previewed
/bmad-dev-autois a validcommand that would HALT on the shim. Preflight failures print to stderr under a "NOT
runnable as-is" banner; rc 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").
session, deduped by fingerprint across retry, resume-replay and the second review pass —
including after a sweep already marked the entry done. Frontmatter is never rewritten.
skills.dev-renderer(renderer stub without the renderscript) and
skills.customize-legacy(an override still named for the old skill).Harvesting and resolution key on content, never on the installed skill name, so both
eras work.
Commits
6b3ccda2b1a1509754f8410ce26cdec2cabVerification
uv run pytest -q— 2771 passed, 1 skipped (+31 over the tag).trunk check(no filter) clean;python scripts/release.py checkgreen.restore — notably the all-status ledger pre-scan, whose removal fails only the
done-entry dedup test while the plain-replay test and 11 other harvest tests stay green.
line names the resolved primitive; legacy customize toml →
skills.customize-legacy;stub without the render script →
skills.dev-renderer(and it clears when the scriptappears, so it keys on absence, not on stub-ness); shim-only →
skills.base-shimFAIL andrunaborts rc 1;run --dry-runpreviews/bmad-build-autoon dev and review, bannerson stderr for the shim-only project, and emits 0 bytes of stderr for a healthy one. This
run is what found the dry-run preflight gap the unit suite could not see.
uv.lockdiff is one line: bmad-loop0.9.0→0.9.1. TUI unchanged since the tag, soasset regeneration auto-skipped.
Notes for review
line off a tag ships as a single PR carrying code, tests, CHANGELOG and the version stamp
as sequential commits.
release/0.9.x, which predates thetypecheckjob, so the absenttypecheckcheck is expected, not a skipped run.validaterenders warnings asok: warning: …. The doubled prefix is documentedshipped output in
checks.ValidationReport.renderwith an explicit rationale — pleaseleave it.
Summary by CodeRabbit
New Features
bmad-build-autodevelopment skill while supporting legacy installations.Documentation
Chores