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 35 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.
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 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b96b9dc — with a deliberate departure from the literal remedy, and one correction: the trigger you name does not exist in this repo, while three others do.
Reproduced, same fixture through both code paths. Repo carries a whole bmad-build-auto; the worktree carries only its SKILL.md:
7344440 b96b9dc
missing_base_skills -> [] []
provision_worktree -> [] []
worktree skill dir -> [SKILL.md] [SKILL.md, customize.toml,
review-prompts/adversarial.md,
step-04-review.md]
base_skills_seed_incomplete -> [] []
Your reading of 7344440 is exactly right: three layers, one requirement, and only the preflight asked correctly. The copy loop's dst.exists() was at DIRECTORY level, so a destination holding one file received zero bytes of the rest; the gate's is_dir() then called that complete.
The departure: fix the copier first, and only then the gate. You asked for a marker-aware gate. On its own that is unfixable — the copier will never fill a directory it skipped, so the operator gets a CRITICAL halt with no action that clears it, repeating on every resume. So the copy is now a per-FILE merge (_merge_traversable) and the gate is the second layer, over what the merge could not deliver:
# same fixture, but customize.toml is a symlink to a shared install OUTSIDE the repo
7344440 b96b9dc
provision_worktree -> [] ['.claude/skills/bmad-build-auto/customize.toml']
worktree skill dir -> [SKILL.md] [SKILL.md, review-prompts/adversarial.md,
step-04-review.md]
base_skills_seed_incomplete -> [] ['.claude/skills/bmad-build-auto/customize.toml']
Repairable → repaired silently. Undeliverable → named per file, escalated before dispatch. Half the fix would have paused the first case too, which is what test_a_partially_tracked_skill_dir_is_repaired_not_escalated pins.
Your marker set is the one that shipped: ("SKILL.md", *BASE_SKILLS[skill]) — SKILL.md alone for the review skills, exactly as you specified, since their marker tuples are empty. I rejected the obvious alternative (rglob parity with the repo's dir, the shape _bmad_scripts_seed_incomplete uses) on measurement: parity halts the entire backlog over a repo-only README.md, with no remedy available to the operator. Parity is right for _bmad/scripts/ precisely because that unit has no named surface — render_skill.py bare-imports config_utils, so "one file short" is unanalysable there. A skill dir does have one, and it is the one missing_base_skills already asserts, so the two layers now demand the same set. test_..._ignores_a_file_no_layer_requires pins that, and its ablation is your alternative.
Two further changes the gate needed, both found while testing yours:
- The precondition is now
(repo_skill/"SKILL.md").is_file(), notis_dir(). A repo dir with noSKILL.mdis not a dispatchable skill — nothing spells it,resolve_dev_primitivereturns None — so pausing over its absence refuses a run nothing is wrong with. The preflight already reports that shape. - Rel granularity follows the cause: coarse
<tree>/<skill>when the worktree has noSKILL.md(everything under it is missing; three rels for one cause is noise), fine<tree>/<skill>/<file>otherwise.
Correction: your reachability story does not hold in this repo. The trigger you describe — a tracked bmad-build-auto/SKILL.md beside ignored step-04-review.md and customize.toml — cannot occur here. .gitignore:8 ignores .claude/ wholesale and all three files hit that same single rule:
$ git ls-files .claude/skills | wc -l
0
$ git check-ignore -v .claude/skills/bmad-build-auto/{SKILL.md,step-04-review.md,customize.toml}
.gitignore:8:.claude/ .claude/skills/bmad-build-auto/SKILL.md
.gitignore:8:.claude/ .claude/skills/bmad-build-auto/step-04-review.md
.gitignore:8:.claude/ .claude/skills/bmad-build-auto/customize.toml
A fresh worktree here has no .claude/ at all, and no gitignore pattern, test or comment anywhere in the repo splits a skill dir. The defect is real; the route you gave for it is not.
Three routes that do reach it, none of which the finding names:
-
Seeding creates the directory first.
seed_files/seed_globsrun before the skills loop andmkdir(parents=True). Aworktree_seedentry naming a file inside a skill dir leaves a one-file shell, and the old loop then skipped the real copy. Measured — and worse than the shape you described, because the file left behind need not beSKILL.md:worktree_seed = [".claude/skills/bmad-build-auto/customize.toml"] 7344440 b96b9dc provision_worktree -> [] [] worktree skill dir -> [customize.toml] [SKILL.md, customize.toml, review-prompts/adversarial.md, step-04-review.md] base_skills_seed_incomplete -> [] []At
7344440that worktree has noSKILL.mdat all — a guaranteedUnknown commandstall — and every layer reports it healthy. The seed is even journaled as seeded, not skipped, becausedst.exists()was False for the seed itself. -
A run branch re-mounted across units.
open_unit_workspacere-checks-out an existing branch underbranch_per = "run", deliberately keeping the commits earlier units landed. If any earlier unit'sgit add -Astaged a file under a tracked skill tree, every later unit's worktree checks out a partial dir. -
Consumer projects.
bmad-loop initnever gitignores a skill tree, andprovision_worktree's own docstring exists to protect "a project that commits its own skill tree". That is the tracked-partial case in someone else's repo rather than this one — and it is the shape the new Windows-runnable test builds.
Scope note. The copy loop's dir-level skip is byte-identical on release/0.9.x, so nothing regressed. base_skills_seed_incomplete is PR-introduced — but so are _seed_bmad_tree and _bmad_scripts_seed_incomplete, which are already a per-file merge plus a per-file reporter. This finishes a pattern this PR started rather than opening one.
One behaviour change worth stating plainly: containment is now asked per file, not just of the skill directory. Before, a child file symlinked out of the repo was copied through by shutil.copy2 because only the directory was containment-checked; now it is refused and reported like a symlinked tree. That asymmetry was the inconsistency, and it matches what _seed_bmad_tree has always done for the _bmad/ surface — but it does convert a previously-silent configuration into a pause, so it is in the changelog.
Twelve new tests, and 15 single-line ablations run against them. Every ablation reddened at least one test; recorded actual vs predicted, since a whole-leg revert can bypass the gate it targets when an earlier conjunct short-circuits. The set: restore the dir-level skip in each of the two copy loops; delete the per-file dst.exists() skip; delete each containment leg separately; truncate the required set to SKILL.md; swap it for rglob parity; drop the repo-has-it conjunct; is_file → exists; the SKILL.md precondition → is_dir(); remove the primitive-era skip; collapse the fine rel to the directory and the coarse rel to a file; empty the local git exclude.
Two results worth reporting:
- Every rel assert is an exact
==on the full path, including/customize.toml. Collapsing the fine rel to<tree>/<skill>is an ablation the tests catch — a prefix or substring assert would have survived it silently, since that bare prefix is what the same gate emits for a wholly-absent skill. A prior round on this PR shipped green for precisely that reason. - One R13 test no longer discriminates what it was written for. With the primitive-era skip ablated out,
..._ignores_an_unresolvable_new_primitive_dirstays GREEN: its fixture is aSKILL.md-lessbmad-build-autodir, which the new precondition now skips independently. The era skip is still pinned by its two siblings, which do redden, so no coverage was lost — but that test is now redundant rather than discriminating.
Full suite 2849 passed / 1 skipped; trunk check --no-fix clean with no path filter; both Windows jobs green (the sole oracle for the new rel strings' separators — they stay POSIX because they are f-strings over a POSIX tree and POSIX marker constants, with no Path stringified). The new gate test and the repaired-worktree test are deliberately symlink-free so they run there.
Two previously-filed issues change shape as a side effect and are annotated rather than left to drift: #416's out-of-tree dangling-symlink crash becomes a structured gate report (the in-tree variant still raises, one frame over, at dst.parent.mkdir), and #417's "problem 2" — a half-copied skill dir reading as complete — is closed, while its problem 1 (the loop aborting on an unreadable dir) is unchanged and keeps that issue open.
The worktree skill copy skipped a destination directory that already existed, so a checkout carrying one file of a skill dir received zero bytes of the rest — and `base_skills_seed_incomplete` asked `is_dir()` on both sides, calling that state complete. The preflight has always asked per file, so a partial skill dir passed all three layers and dispatched a session whose step-04 had nothing to read. Both loops now merge per FILE (`_merge_traversable`), and the gate asks the preflight's own named surface (`SKILL.md` plus the skill's `BASE_SKILLS` markers), naming the missing file rather than the directory. Containment is per file too, so a single skill file symlinked out of the repo is reported instead of followed silently. The merge is what makes a gate-only fix wrong: a directory the copier refuses to fill can never be cleared by the operator, so the pause would repeat on every resume.
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: b96b9dc0b7
ℹ️ 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 task.baseline_ledger_present is not False: | ||
| if task.baseline_ledger_present is None: | ||
| self.journal.append("ledger-baseline-unknown", story_key=task.story_key) | ||
| return |
There was a problem hiding this comment.
When an untracked or ignored ledger already exists at the attempt baseline, a host crash after harvest but before a non-fixable rollback leaves baseline_ledger_present=True, so this branch preserves the post-harvest text even though the originating code is subsequently reset. The stale entry can then be swept or committed by the successful retry. Fresh evidence beyond the earlier crash-replay report is that the current regression tests explicitly retain HARVEST_A for both pre-existing untracked and ignored ledgers; persist and restore the pre-harvest contents rather than only recording file presence.
Useful? React with 👍 / 👎.
| return [ | ||
| rel | ||
| for rel in ("SKILL.md", *markers) | ||
| if (repo_skill / rel).is_file() and not (worktree_skill / rel).is_file() | ||
| ] |
There was a problem hiding this comment.
When an otherwise local skill contains an individually symlinked workflow file outside the repository—for example step-01-clarify-and-route.md—_merge_traversable drops that file at its containment guard, but this completeness check examines only SKILL.md, step-04-review.md, and customize.toml. The engine therefore dispatches a worktree whose selected primitive cannot execute its entry step. Fresh evidence beyond the earlier partial-directory report is this file-level containment path; compare the full workflow surface that was eligible for copying, or at least include every invoked step file.
Useful? React with 👍 / 👎.
Codex R15 finding C1. `_dev_phase` held its pre-harvest deferred-work snapshot in a plain local, so a host death between `_harvest_spec_deferrals` and the non-fixable RETRY rollback lost it. The replay leg then fell back to a persisted BOOLEAN, `StoryTask.baseline_ledger_present`, which could answer only "was there a ledger 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, and nothing else reverts one: `reset --hard` skips ignored paths, there is no `git clean`, and `_safe_reset`'s `keep` shields the artifacts dir besides. The successful retry then committed a finding describing code that no longer existed. The snapshot is now persisted with the attempt — `pre_harvest_ledger` + `pre_harvest_ledger_captured` — armed with a `_save()` before the harvest and cleared at four sites once that attempt's decision is acted on, so a finished story carries no ledger copy in state.json. Live and replayed paths restore the same bytes through one helper. The presence bit and `_drop_ledger_created_since_baseline` are deleted; net-subtractive. The split of text and flag is load-bearing and the `from_dict` spelling is the trap: `None` text means "no ledger existed, so the restore UNLINKS", which is a different state from "nothing was captured, hands off". A persisted `""` must rehydrate as `""`, so the text uses `is not None` and not the natural `str(d.get(k, "")) or None` — ablation viii shows that spelling rehydrating a persisted `None` as the literal string "None" and writing it into the ledger. The arm also moved AHEAD of `_post_dev_state_sync` (plan amendment F1, decided this round rather than inherited from the brief). The sweep engine closes a bundle's ledger entries `status: done` there, and a snapshot taken after it wrote those closes back on top of the `reset --hard` that had just correctly reverted them. A surviving close is worse than a surviving finding: `deferredwork.open_ids` only ever re-bundles open entries, so no later sweep would look at that id again — the work is silently lost, not merely mis-recorded. `test_bundle_ledger_close_reverted_on_non_fixable_retry` is the discriminator; nothing in the existing ledger family can see the snapshot's position, because there the ledger is only ever touched by the harvest. Ablations: 14 rows, each run against the WHOLE suite, every predicted RED and GREEN confirmed. The rows that deliberately stayed green were read too — the entire live-path family survives every replay-only ablation, and the review-leg defer control cannot redden under "restore on the DEFER path" (its ledger is tracked and site 1 has already disarmed), which is why the dev-leg defer test exists. Row i is the one ablation with no behavioural witness, and it is stated rather than papered over: `run()`'s `finally: self._save()` re-persists the armed fields on any soft crash a fixture can inject, so deleting the arm's own `_save()` leaves every crash-replay test green. Only the durability test `test_the_pre_harvest_snapshot_is_persisted_before_the_harvest_runs`, which reads state.json off disk at the moment the harvest runs, pins it. A replayed non-`completed` result is vacuous rather than untested: `_resumable_session` refuses any record that is not completed with a `result_json` and synthesizes `status="completed"` on what it hands back. The reachable neighbour — a LIVE non-completed attempt reaching the same restore leg with nothing armed — is pinned by `test_a_non_completed_attempt_never_touches_the_ledger`. 2858 collected (2857 passed, 1 skipped); full `trunk check` clean. pyright does not exist on the 0.9.x line. Three items from an independent clause-by-clause verification of the CHANGELOG against source, folded in rather than deferred. (1) Clear site 0 moved ABOVE the `pre_dev_phase` veto check: `_vetoed` drives the task terminal and saves, so a veto below it persisted a dead task still carrying the ledger copy. (2) "Neither path unlinks a ledger git tracks" was stale prose from the deleted two-branch shape — there is one path now. (3) The neighbouring harvest bullet claimed "a retry ... never re-files one"; a NON-fixable retry re-files by design, since this bullet's revert removes the entry first (`test_harvest_retry_reharvests_after_ the_rollback` asserts two harvests). Scoped to the fixable retry and the asymmetry named.
) The worktree seed gate asked for `SKILL.md` plus the two DEV_PRIMITIVE_MARKERS — 3 of the 12 files a real `bmad-build-auto` install carries. The other nine were not inert: since BMAD-METHOD #2601 `workflow.md` is the renderer's entry document and names its step files as `[[bmad-snapshot:…]]` sources, so a file the seed dropped makes `render_skill.py` print `HALT:` and the session Stop having written nothing — on every story, because the seed reads the same repo every time. That is the environment fault this gate exists to catch. `_absent_skill_files` now walks the repo's skill dir and reports every rel the worktree lacks. It walks it with `_walk_traversable_files`, the SAME generator `_merge_traversable` now copies from, not an rglob mirror of it: rglob does not descend a symlinked sub-directory while `iterdir()` + `is_dir()` does, so a `review-prompts/` pointed at a shared install is dropped by the copy and would be invisible to a mirrored check. This reverses the rationale `_absent_skill_files` carried, which rejected parity because it "would halt the whole backlog over a repo-only README.md". The per-FILE merge landed in the same commit as that argument and retired it: the copier filters nothing by name, so a repo-only README is delivered like anything else and parity stays silent. What parity can report is exactly the set of files the seed FAILED to deliver, and every one of those has an operator remedy. `test_..._ignores_a_file_no_layer_requires` pinned the reversed design by building its pair by hand; it is rewritten to drive the real `provision_worktree` and assert the README is DELIVERED. DEV_PRIMITIVE_MARKERS is untouched — it is still the shim detector and still the preflight's requirement. Ablation A14 (un-sharing the walk) is structurally untestable: an un-shared copier recursion is byte-identical to the walker today, so no test can observe the difference until one half is edited — at which point A2 catches the gate half. Sharing is a structural guarantee, not a tested one. To be stated in the PR rather than papered over. Fixture note: `install_build_auto_skill(renderer_stub=True)` and `_symlink_skill_tree(renderer_stub=True)` now also write `workflow.md`, so a #2601 stub and the entry it composes from stay one era. Inert until the renderer-sources preflight lands.
…pted (#405) The orchestrator marks a bundle's deferred-work ids `status: done` itself, and did so in `_post_dev_state_sync` — above the artifact gate that can still discard the attempt. A close is the most expensive engine-side ledger write to leave behind: `deferredwork.open_ids` re-bundles only `open` entries and the module has no reopen primitive, so a `done` id whose code was rolled back is invisible to every future sweep. The work is lost, not merely mis-recorded. On the non-fixable RETRY leg `_dev_phase`'s pre-harvest ledger snapshot reverted it. On the DEFER terminus nothing did, and no snapshot in `_dev_phase` could have: `_defer` takes its OWN snapshot after the close, resets, and writes those bytes back on purpose so review-found deferrals survive a defer. Confirmed data loss, and PRE-EXISTING — it reproduces identically at 64c4f2c^. New base seam `_post_dev_accepted_sync`, called on the PROCEED branch of `_dev_phase` and a no-op on the base path; `SweepEngine` moves the close there and overrides `_post_dev_state_sync` to a no-op (a bundle has no sprint row). The shared pre-gate call site could NOT simply move: `verify_dev` reads the sprint board that `_post_dev_state_sync` writes, so relocating it would fail every story's own gate. Gated on the DECISION being PROCEED rather than on `outcome.ok`, which excludes two further discards for free — a CRITICAL escalation preempts even a passing outcome, and a failing `[verify] commands` run replaces the outcome after the gate. One deliberate consequence: the close no longer contributes to the gate's proof-of-work diff (the ledger is not in `verify_dev_exclude_relpaths`), so a bundle session that changed no code now fails that gate instead of passing on the orchestrator's own bookkeeping. Tests: `..._withheld_when_a_completed_attempt_defers` is the discriminator the suite could not previously express — it differs from the retry test by one thing, a second COMPLETED attempt instead of a corpse, which is what reaches `_defer` with the ledger already rewritten. `..._withheld_when_a_critical_escalation_preempts_the_gate` is the only scenario separating PROCEED from `outcome.ok`. The retry test is renamed and its docstring rewritten: it no longer discriminates the snapshot's position, since the snapshot no longer guards any close. Ablations, each against the WHOLE suite (5 sandbox-only failures subtracted — the sandbox omits scripts/ and examples/; identical in every row, incl. the control): - whole-leg revert (close back above the gate) → the 3 withheld tests + the direct-call unreadable-spec test. Happy-path close tests stay GREEN, correctly: the close still fires, just earlier. - delete the call site → 6 "a close DOES happen" tests. The withheld tests stay GREEN. `_verify_review`'s reclose does NOT mask this, because those tests assert the `sweep-bundle-closed` kind specifically, not the end state. - below the gate but ungated by the decision → exactly the 3 withheld tests. - gate on `outcome.ok` instead of PROCEED → the CRITICAL test ALONE; retry and defer are insensitive because their outcomes are not ok. No `test_engine.py` ledger-family test moves under any row, confirming the change is confined to the sweep path. Prose corrected rather than left stale: the arm comment and `_harvest_spec_deferrals`/`_restore_persisted_ledger` no longer cite the close as something the snapshot guards, and `CHANGELOG` :142-145 is rewritten — it does not become true unscoped, because the mechanism it states (the snapshot reverts the closes) is no longer how they are protected. `_finish_inflight_bundles`' "a recovered bundle that defers or escalates keeps its ids open" was false as written and is now accurate: true for escalate and for a DEV-leg defer, explicitly NOT true for a REVIEW-leg defer, where the ids are already `done` and `_defer`'s indiscriminate restore rides them over the reset. That residual is pre-existing and out of this change's scope.
`_disarm_ledger_snapshot`'s docstring claimed it is "deliberately NOT followed by a `_save()`, at any call site, because a death in that window replays *that same attempt*". That rationale is load-bearing at exactly ONE of the four call sites, and reading it as universal left a real hole. - Site 0 (fresh entry): a `_save()` deliberately DOES follow on the veto path, and a death before it leaves the phase `pending`, so the resume RESTARTS the story rather than replaying the attempt. - Site 1 (PROCEED): the hole. `verify_dev` has already set `task.spec_file` and the phase is `DEV_VERIFY`, so a hard kill here resumes through `_finish_inflight`'s FIRST branch → `_resume_after_dev_verify` → `_review_and_commit`. `_dev_phase` is never re-entered and `_resumable_session` is never consulted, so nothing ever consumes the snapshot — a full copy of the ledger rides a terminal task in state.json for the rest of the run. - Site 2 (RETRY): where the rationale holds. The same kill replays that attempt, which still wants the snapshot, so the stale-looking persisted value is correct. - Site 3 (DEFER/escalate): `_defer`/`_escalate` save immediately below. Adds `self._save()` at site 1 only, and scopes the docstring to the retry site instead of stating it universally. Bounded severity — state.json residue on a hard kill, and nothing reads the field on a terminal task — but the universal CLAIM was the real defect, since it is the reason a reader would not add the save. Test: `test_the_proceed_disarm_is_persisted_before_the_review_leg` reads state.json off disk at the `post_dev_phase` emit, the first moment after the disarm. It has to: `run()`'s `finally: self._save()` re-persists the disarmed fields after any crash an in-process fixture can stage, so the pre-existing `test_a_finished_story_carries_no_ledger_copy` stays GREEN with this `_save()` deleted. Same technique, and same reason, as `test_the_pre_harvest_snapshot_is_persisted_before_the_harvest_runs` uses for the arm. Ablation against the WHOLE suite (delete the `_save()`): the new test alone reddens, and `..._carries_no_ledger_copy` stays green — which is the measurement showing the new test is not redundant with it rather than an assertion that it isn't.
…cards them (#405) The previous commit stops a DISCARDED dev attempt from closing anything. The review leg is a different shape and it could not cover it: a bundle accepted at dev has its ids closed correctly, and they have to be on disk because `verify_review_bundle` gates on them. Only the later review failure makes them wrong — and then `_defer` writes its own post-close ledger snapshot back over the `reset --hard` that had reverted them. That restore is deliberate and stays: `_stash_deferred_artifacts` has already moved the spec out of the artifacts dir, so a harvested finding's ledger entry is its only surviving record. But it replays the whole file, so the closes ride it too and name code the reset discarded — `status: done` with nothing behind it, invisible to `deferredwork.open_ids`, which re-bundles only `open` entries. Verified by an independent probe on FOUR routes into that `_defer`, reproduced on a pristine export of 8478394 as well as on this branch: review budget exhausted and not refileable, the repair phase exhausted after a clean review, the budget rescue's own `_verify_review` red, and `review.enabled = false` failing at its gate. Escalate and `rollback_on_failure = off` are unaffected — neither resets. New base seam `_reopen_ledger_after_defer`, called inside `_defer`'s `baseline_commit` branch after the restore; no-op on the base path. It is unreached when `_rollback_or_pause` raises (stop-and-wait keeps the tree) and the isolated branch returns before it (those closes live in the unit worktree, dropped unmerged). `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 passed, so a close written by an earlier sweep, by the legacy path where the session edits the ledger itself, or by a human is never revoked — the ledger has no field that could record who reopened what after the fact. The round trip is byte-exact, because `mark_done` writes its resolution on the line directly below the status and this removes exactly that line. Ablations, each against the WHOLE suite (5 sandbox-only failures subtracted): - delete the `_defer` call site → both review-leg integration tests, nothing else. - delete SweepEngine's override → identical set, so the seam is not accidentally satisfied by the base. - drop the note-match conjunct from `mark_open` → `..._refuses_a_close_it_did_not_ write` alone. - drop the `entry.open` conjunct → `..._refuses_an_entry_that_is_still_open` alone. That guard is NOT redundant with the note match: an entry the inner session appended can be open and still carry a `resolution:` line, and without the guard its note is silently deleted. Rows that did not redden, read: `test_defer_keeps_the_harvested_entry_after_rollback` stays green under every row, so the harvest-preservation contract `_defer`'s restore exists for is untouched; `test_sweep_worktree_bundle_merges_to_target` likewise, so the isolated path is unaffected. `_finish_inflight_bundles`' "a recovered bundle that defers or escalates keeps its ids open" is now true on both legs — by two different mechanisms, and it held on neither before this pair of commits.
#405) Response to an independent skeptic pass over 29ea28f..d54d595. Eight findings, one of them a real breakage on a lane that is not run locally. 1. **Windows CI breakage.** The review-disabled defer test used a POSIX shell counter (`$(...)`, `$((...))`, `test -lt`) as its verify command. `shell=True` on Windows is `cmd.exe /c`, which does not reject that string — it MIS-PARSES it into a constant exit code, so the command can no longer pass once and fail once and the test fails on windows-latest, which runs the full suite. Replaced with a `passes_once` conftest helper written for both shells, alongside the existing `_file_exists_cmd` family. It takes an explicit path: `run_verify_commands` passes no env, so the `$BMAD_LOOP_RUN_DIR` its neighbours use is unset there and the marker would latch at the filesystem root. Ablation: forcing the helper to a constant exit code — what cmd actually produces — reddens exactly that test. 2. **"No leg needs a revert" was false**, in three places. A blocking `post_dev_phase` workflow can defer an ACCEPTED attempt after the close, so the undo is load-bearing on a dev-phase leg too. The code was already right; the sentences were not. The CHANGELOG's "all four routes" was an undercount for the same reason. 3. **A coverage regression I introduced.** Rewriting the retry test removed the only test that pinned the pre-harvest arm's POSITION, while the comment still claimed the boundary was deliberate. Measured: sliding the arm below both syncs is green at HEAD and red at 8478394. The comment now says so — the boundary is defensive positioning for a future write, not an observable property. 4. **"Byte-exact" round trip was overstated.** `mark_done` rewrites the whole file through `write_text`, so a CRLF-stored ledger is normalised by the close and the reopen cannot put that back. Restated as character-for-character as `read_text` sees it, with the pre-existing normalisation named. 5. **"Nothing between the old position and this one reads the ledger" was false** — `_harvest_spec_deferrals` sits in that span and reads it twice. Effect is inert (its pre-scan matches the fingerprinted `origin:` across entries of every status, so a status flip cannot change what it files), but the enumeration read as exhaustive. Narrowed to "gates on", with the harvest named and its insensitivity explained. 6. **Three conjuncts no test could redden**, now pinned, each ablated to confirm it reddens exactly its own test: `mark_open`'s status-less-entry guard (its absence is an AttributeError raised from inside `_defer` — a crash instead of a deferral, and `parse_ledger` explicitly tolerates such entries); the `.strip()` on the note compare (a session-reformatted resolution line must still be recognised as our own close); and `_reopen_ledger_after_defer`'s `if reopened:` (pinned by asserting the dev-leg defer journals nothing — withholding the close is what fixes that leg, not undoing one). 7. `_reopen_ledger_after_defer` has no `_generic_dev()` guard, unlike both its siblings. Deliberate — they WRITE and must know which path owns the ledger, this one only undoes a note it recognises — and now said rather than left as an unexplained asymmetry. 8. "Runs only where a reset actually happened" was imprecise: `_rollback_or_pause` is a no-op on an already-clean tree. Not reachable with anything to undo (a written close makes the tree dirty), but reworded to what the code does. Suite 2873 collected (2872 passed, 1 skipped) from 2871; full trunk check clean.
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.
…#405) C2 Part 2. `missing_base_skills` asked a resolved renderer stub for its script unit and central config but never for the documents it composes FROM, so a truncated repo-side install passed `validate` and then HALTed every session — the marker pair answered for 3 of the 13 files a real `bmad-build-auto` carries. New `skills.dev-renderer-sources` (registered in `checks.VALIDATE_CHECKS`) asks two questions, both a deterministic `HALT:` upstream: `workflow.md` is a source, and every `[[bmad-snapshot:…]]` target is a source too. The source set and the token regex are byte-for-byte mirrors of `render_skill.py`'s own — the gate has no severity filter and no `--force`, so it asks what the renderer will SEE and only what the install DECLARES, never a fixed renderer-era file list that a reorganized upstream would trip. Deliberately mirrors the renderer's `rglob` rather than borrowing the copier's `_walk_traversable_files`, which descends symlinked sub-dirs the renderer never enumerates. Ablations: 19 rows + a control, each against the WHOLE suite. Part 1 (A1–A7, A15, A14) re-run unchanged; Part 2 A8–A13, A16, A17 each reddened exactly its predicted set, four of them with a sole witness. Both of CW's predicted GREENs confirmed: A1 leaves T3 green and A8 leaves every Part-1 row green. A14 (un-share the walk) reddens NOTHING — structurally untestable, to be stated in the PR description rather than papered over. A15 was run in both shapes; their union is exactly Phase 2's eight. CHANGELOG re-verified clause by clause against source, which turned up four false claims beyond the one this commit creates: the install carries THIRTEEN files, not twelve (fixed in all five copies); the defect is pre-existing since 0.7.0, not 0.6.5, which was never released; a missing render *script* cannot print `HALT:` — the stub's SKILL.md is what halts; and per-file naming is the else-branch only, a wholly-absent skill still reports the coarse rel.
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.
…#405) An adversarial pass over 3de6270 found the source-set mirror was missing three of upstream's four per-candidate guards. Measured, all four: workflow.md is a DIRECTORY -> upstream HALTs, gate returned [] workflow.md is a DANGLING symlink -> upstream HALTs, gate returned [] a FIFO named pipe.md in the tree -> upstream HALTs, gate HUNG FOREVER a source symlinked OUT of the dir -> upstream HALTs, gate returns [] `path.is_file()` closes the first three. The hang is the one that mattered: the docstring claimed both blind spots were "in the safe direction (a false green, never a false refusal)", and an unbounded read of a FIFO inside `validate`/`run`/`sweep`/`resume` is neither. The escaping-symlink row is left open, named in the docstring as the ONE remaining false green rather than papered over. Four conjuncts had no witness and now have one each (ablated, whole suite, sole witness apiece): `is_file()` -> a directory-named entry (portable, so Windows CI sees it); the `*.md` glob -> customize.toml `instruction` prose quoting a token, which upstream explicitly never scans and a widened glob would refuse a healthy project over; the `OSError` half of the read guard -> a chmod-000 source (POSIX-only; the undecodable-bytes test can only raise UnicodeDecodeError); the `set()` dedupe -> a repeated token, live on the real install, which step-01 names four times. Prose corrections, each refuted by code in the same commit: "missing X" in the finding message when X is present-but-not-loadable (now "unresolved"); "deliberately NOT a fixed file list" while RENDERER_ENTRY_REL is one (scoped, and the exception named with upstream's own hardcoding as the reason); "two questions and only two" reading as exhaustive over renderer HALTs; "a file the skill does not carry" refuted by this PR's own SKILL.md test; a fresh "twelve" I introduced in a docstring one commit after fixing the other five; and "Two findings" three lines above a four-id assert.
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.
…405) The `_bmad/` seed and its detector adopt the skill seed's walk, so a symlinked `_bmad/scripts/lib` is no longer seeded as nothing AND reported as complete by two agreeing `rglob`s. The walk descends symlinks, so it carries a branch-local cycle guard. Per file, an undeliverable source, an occupied destination and a failed read or write become skips the completeness gate names rather than an exception out of `provision_worktree`, which `Engine._run_isolated` does not wrap — one unreadable skill file ended the whole run with a crash dump where a named, resumable escalation belonged. A dangling destination symlink counts as occupied, since 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 total, and — the part a `try` alone does not buy — reads the same on all four supported interpreters. `is_file()`/`exists()`/`is_dir()` raise on an unsearchable parent through 3.13 and answer `False` on 3.14, and for `_is_dir` `False` is the silent drop rather than the cautious reading, so a `False` is re-asked of `stat()`, which still raises there. Measured: the wrapper alone passed both `0o444` witnesses on 3.11/3.12/3.13 and FAILED them on 3.14, where the seed dropped the subtree and the gate reported nothing. The new `test_is_dir_reads_a_refused_probe_...` emulates each reading, so every lane exercises both branches. Ablations, whole suite, both interpreter lanes: `probe-whole-leg` 1 red on 3.13 and 3 on 3.14; `probe-drop-errno-filter` 25 on both (absence read as refusal makes the gate name everything); `probe-drop-isinstance` and `probe-drop-valueerror` 1 each; `walk-root-bare-isdir` 3 on 3.13 and 0 on 3.14; `isfile-bare` 6 on 3.13 and 0 on 3.14. The `0o555` control is a false-positive witness and no guard ablation reddens it, so it is pinned instead against a constant-True `_is_dir` and an `os.access(W_OK)` one, which fail its two assertions separately. Also corrects claims from earlier commits on this PR, all re-measured by hand, so the widened scope is not silent: - CHANGELOG.md:46 — 0.9.0 already hard-blocked on the over-broad tree list; it emitted `skills.base-missing` as a problem and the run preflight returned False. Only the number of ways to trip it changed in 0.9.1. - install.py:92 — SKILL.md plus the two markers is 3 of 13 files, not "two markers are 3". - install.py:1748 — 0.9.0 named `worktree_seed = ["_bmad"]` as the BROKEN example under #230, never as the workaround. - The cycle counterfactual's "85 path components" is location-dependent (ELOOP bounds the absolute path; re-measured 82 from a shallower tmpdir) while the 42-file count holds — stated that way in both copies. - The unreadable-tree witness named the gate's split as the second raiser; the frames say it is the `_merge_traversable` leg, and there are five stacked probes rather than two. Each verified by reverting exactly that one call.
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.
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