0.9.1: adopt upstream bmad-build-auto rename, harvest spec-frontmatter deferrals (#405) - #406
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:
WalkthroughChangesRuntime compatibility and release
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
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: e8979b601d
ℹ️ 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".
| # artifact gate that can still send this attempt back. The rollback | ||
| # half is enforced explicitly, by the snapshot armed above and the | ||
| # restore at the RETRY branch below. | ||
| self._harvest_spec_deferrals(task, result.result_json) |
There was a problem hiding this comment.
When scm.isolation = "worktree" and deferred-work.md is ignored, as in the standard _bmad-output/ layout, this harvest writes only into the unit worktree. finalize_commit's git add -A omits the ignored ledger, _merge_local merges only committed branch state, and _carry_harvested_deferrals runs only from _defer; consequently, successful teardown deletes the sole copy and later sweeps never see the accepted story's findings. Carry these recorded deferrals into the main ledger on the successful isolated path as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 22e79e0. Traced link by link against the code rather than against the commit messages; every link holds.
Mechanism
- the harvest writes
self.workspace.paths.deferred_work(engine.py:3147), and underscm.isolation = "worktree"self.workspaceis the unit's for the whole drive (_run_unitrebases it at:800-801,workspace.py:129) — so the unit worktree's ledger; finalize_commitstages withgit add -A(verify.py:1745), which skips a gitignored path in silence — so the entry never rode the branch;_integrate_unit's DONE arm (:834-845) calls_merge_local, which merges committed branch state only, so there is nothing to bring over;close_unit_workspace(success=True)removes the worktree unconditionally — thekeep_failedbranch is nested insideif not success:(workspace.py:179-215) — and the DONE leg never callscapture_diff, so unlike DEFER there is not even achanges.patchcopy;_carry_harvested_deferralshad exactly one call site::3828, inside_defer.
The existing guard test_a_done_isolated_unit_files_its_harvest_exactly_once passed only because its fixture ledger is not gitignored (conftest.py writes .bmad-loop/runs/ alone), so add -A staged it. The two DEFER-leg rows beside it do set .gitignore, which is why they caught the DEFER half in dc4f934 and this one stayed hidden.
Fix
_integrate_unit's DONE arm now calls a new base hook _carry_isolated_ledger_writes, whose base implementation is the harvest carry. (The hook, not a direct call, because SweepEngine needs the same seam for its ledger closes — 5c401f3, below.)
Three placement decisions, each ablated separately rather than argued:
- After
_merge_localreturns, never before. On a project whose ledger is tracked, the merge is what puts the open entry in front ofappend_entry's dedup. Carrying first files a fresh id and commits it, and the merge then lays the branch's own copy over that. - After it returns, not inside it. The ledger write is unguarded by design (only the commit is wrapped); placed before
close_unit_workspaceanOSErrorwould cost the unit its teardown — no forensic patch, worktree left mounted, nounit-closed— the price the DEFER leg has no choice but to accept. - Not on the escalation legs. Both
_keep_branch_and_escalatecalls always raiseRunPaused, so the line is unreachable there, deliberately: those legs keep the branch for a human to merge and that merge brings the branch's own entry with it. The conflict leg can also leave the repo mid-merge.
No idempotence guard, and that is checked rather than assumed. append_entry dedups against open entries (deferredwork.py:230-237), so a duplicate would need the merged copy to be status: done by the time the carry runs. It cannot be: the only writer that closes an existing entry is deferredwork.mark_done, whose in-story caller closes strictly task.dw_ids — assigned once at bundle materialization (sweep.py:616) and never appended to — while a harvested entry always gets a fresh DW-<next>.
Commit behaviour is unchanged from the DEFER leg's: non-gitignored → every row dedups, carried == [], no commit at all; gitignored → git add -- <ignored path> returns rc 1, caught, harvest-carry-uncommitted journalled, and the leftover uncommitted ledger is invisible to verify.dirty_paths (status --porcelain without --ignored), so it cannot dirty a later story's clean_incoming_collisions.
Two things beyond what the finding names
Designing the fix surfaced the same mechanism in two more producers:
SweepEngine._close_bundle_ledger_when_spec_status— fixed here,5c401f3. It writes the worktree's ledger too, and with a gitignored ledger the flip dies unmerged: the entries stayopen,deferredwork.open_idsre-bundles them, and every later sweep re-triages and re-drives work that is already done — an unbounded loop, not a one-time drop. The override records what the close intended (task.dw_ids), never whatmark_doneflipped: the close runs twice on a landing bundle (after dev, and again from_verify_review's idempotent reclose), and the second run finds every id alreadydone, so a record keyed off the flips is empty on exactly the run that needs it — the trapharvested_deferralsalready paid for once indc4f934._record_review_budget_followup— not fixed here, filed as A damped review-budget follow-up entry is dropped under worktree isolation with a gitignored ledger #425 with a measured reproduction. Third producer, sameadd -Amechanism, and the carry does not cover it because it records nothing to carry.
And one scope limit worth stating, since it bounds the sweep half: an unseeded gitignored ledger is absent from the unit worktree entirely, so verify_review_bundle (which reads the worktree's copy) never sees the bundle's ids done and the unit defers rather than landing. No DONE-leg carry can reach a unit that never reaches DONE. Measured and filed as #426; that failure is loud (review-verify-failed) where this one was silent.
Tests and ablations
New/amended rows in tests/test_engine_worktree.py and tests/test_sweep.py. Ablations are run against the whole suite (3014 passed, 1 skipped at baseline), one per edit rather than one per test, so an edit that pins nothing is as visible as a test that pins nothing:
| ablation | red |
|---|---|
drop the _carry_isolated_ledger_writes call (= previous HEAD) |
6 |
move the carry above _merge_local |
3 |
hoist it into _merge_local's prologue (reaches the escalation legs) |
3 |
| drop the empty-record early return | 2 |
drop the SweepEngine override |
4 |
record marked instead of task.dw_ids |
4 |
drop the override's super() call |
1 |
Two of those rows are there because the first pass of this method found them empty. The super()-call ablation reddened nothing — no row in the suite drove a sweep bundle that harvests under isolation — and a _generic_dev() guard the draft carried reddened nothing either, correctly: bundle_closes_intended is only ever assigned on the generic path, so the record already is the guard and a second predicate saying the same thing is a branch no test can falsify. The first gap got a test; the second got deleted.
One correction to my own first draft, since the ablation contradicted it: I had written that carrying before the merge produces an add/add conflict. It does not in the simple shape — append_entry writes no timestamp and computes the same next id from the same base, so the two copies are byte-identical and git resolves them. What actually shows is the extra commit and the non-empty carried-id list. The test asserts the dedup, not the downstream damage.
Suite green on py3.13 and py3.14 locally; CI 9/9.
| # harvest — handing the proof-of-work gate the orchestrator's own | ||
| # write, the hazard this flag exists to prevent (#405). | ||
| if not replayed: | ||
| task.harvest_wrote_ledger = False |
There was a problem hiding this comment.
When an attempt harvests a finding and then receives a fixable verification failure, its tree and engine-written ledger entry intentionally survive into the repair attempt. Clearing this flag for that next attempt means a deduplicated or absent second harvest leaves harvest_wrote_ledger false, so the proof-of-work gate counts the previous attempt's engine-written ledger diff as session work; if the repair session removes the source changes and a transient verification command now passes, the empty implementation can be accepted solely because of that ledger entry. Preserve the provenance of surviving harvest writes until the phase lands or rolls them back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, and fixed in 73da6d0 — with two corrections to the finding's own account, one of which changes the fix, plus a third bug the same root cause produces (7139980).
The mechanism, verified
Attempt N harvests (harvest_wrote_ledger = True, the exclusion fires), passes the artifact gate on real source, then fails verify_commands_outcome fixably — verify.py:1623 is the only fixable=True source on the dev leg, and it runs only after outcome.ok, so attempt N necessarily had real work. The fixable retry keeps the tree and the harvest's ledger line on purpose. Attempt N+1 clears the flag; the repair session reverts the source; the repair harvest early-returns or fully dedups, so filed is empty and the flag stays False; _harvest_gate_exclude returns (); has_changes_since counts attempt N's orchestrator-written ledger line as the repair session's proof of work. If the verify commands now pass — plausible precisely because the offending change was reverted — an empty implementation PROCEEDs. Exactly as described.
Correction 1 — the proposed fix is insufficient on its own
"Preserve the provenance of surviving harvest writes" is necessary and not sufficient. _harvest_gate_exclude has two early returns (engine.py:3257-3260):
if not task.harvest_wrote_ledger:
return ()
if task.ledger_changed_before_harvest:
return ()The second one is recomputed on every non-replayed attempt against baseline_ledger_digest, which was phase-scoped — recaptured only on a fresh _dev_phase entry. So attempt N's surviving entry makes ledger_changed_before_harvest True on attempt N+1, and the exclusion short-circuits at the second return regardless of what the first flag says. Latching harvest_wrote_ledger alone leaves the hazard fully live.
That second return is not removable — it exists because the exclusion is path-granular (it hides the whole relpath, not the harvest's lines), so without it a story that reconciles the ledger and records one deferred: finding reads as "no changes since baseline", and the non-fixable RETRY that follows PAUSES the run under the shipped scm.rollback_on_failure = off default.
So the fix is a rolling reference, not a latch: baseline_ledger_digest becomes "the last ledger state in which every post-baseline_commit byte is accounted for" rather than "the phase baseline". Two new re-bases — forward onto the kept tree at the fixable branch, and back onto the restored snapshot in the rollback's finally (computed from the snapshot, so no new raise site appears on that leg).
Latching the answer instead — skipping the recompute on a continuation — is the obvious pairing and it is wrong, in two independent ways. This is the load-bearing part:
- It re-creates the same hazard three attempts later. Attempt 1 harvests DW-1, fixable → digest re-based to
D(with DW-1). Attempt 2 fails non-fixably → reset + restore unlinks the ledger;feedback = None. Attempt 3 recomputes:D(absent) != D(with DW-1)→ledger_changed_before_harvest = True→ second early return → a session that writes no source but finalizes its spec withdeferred:re-files DW-1, andhas_changes_sincesees the newly created ledger → PROCEED on an empty implementation. Pairing the forward re-base with the post-restore one kills this. - It re-opens the hole
161c85eclosed. A latched exclusion masks a repair session whose honest work genuinely is a ledger entry → false "no changes" → non-fixable retry → PAUSE under the default. The rolling reference sees the session's write and stands the exclusion down.
Both are pinned. test_a_repair_session_that_writes_the_ledger_itself_still_proceeds reddens under the skip design and stays green under the rolling one; it is the row that chose the design, written before the fix.
Correction 2 — the scope excludes an ignored ledger
The finding does not bound its reachability, and the bound matters. An ignored ledger is invisible to untracked_files (--exclude-standard) and to git diff, so it can never be false proof of work in the first place. This hazard is reachable only for a tracked, or untracked-but-not-ignored, ledger.
That is the mirror image of the P1 finding on this same PR, which is reachable only for an ignored one. Worth stating explicitly because the two findings look like the same bug and have disjoint reachable configurations.
Two more bugs the same root cause produces
The root cause is one divergence: _rollback_or_pause always resets to task.baseline_commit, which is phase-scoped, while every piece of harvest provenance is attempt-scoped. Those coincide on every run where at most one attempt's work sits above the baseline, and the fixable retry is the only construct in the engine that breaks that. Two more instances, both fixed here:
- Findings lost under isolation (
73da6d0).task.harvested_deferrals = []on every fresh attempt. Attempt N harvests → fixable retry keeps the tree and the entry → attempt N+1's session stalls, so the harvest never runs → budget exhausted → DEFER → the carry returns on an empty record → the real entry dies with the unmerged worktree. Needs no spec change to reproduce. - The pre-harvest snapshot survived into the wrong chain (
7139980). Attempt N armsS_N, harvests DW-1, fixable-retries and disarms → attempt N+1 re-armsS_{N+1}including DW-1 → attempt N+1 fails non-fixably → the reset discards attempt N's source too, and thefinallywritesS_{N+1}back with DW-1 in it. A finding about deleted code survives, violating the contract the harvest's own docstring states. Same class as20d3dc0, new sequence. The snapshot is now scoped to the retry chain, and the disarm moved onto the non-fixable leg where it belongs. The comment that had claimed a fixable retry needed the disarm — and that the pause leg could not reach it — did not survive scrutiny: the pause has disarmed in its ownfinallysince0a8088f, and a fresh attempt re-arms unconditionally, so neither clause held.
The feedback local is the discriminator throughout rather than a new persisted field. It is assigned in exactly three places in _dev_phase and never reset per iteration, so at the top of iteration k it means precisely "iteration k−1 took the fixable branch" — the idiom _restore_patch's guard already uses. Both re-entries lose it and neither loses safety: a replay is replayed-guarded to the same answer a continuation wants, and a restart has already reset the tree and re-captured every baseline.
Ablations
Run against the whole suite (3014 passed, 1 skipped at baseline), one per edit rather than one per test:
| ablation | red |
|---|---|
drop and feedback is None from the flag clear |
1 |
| drop both halves of the rolling re-base | 1 |
| drop the forward re-base only | 1 |
| drop the post-restore re-base only | 1 (a different row) |
| the rejected skip-the-recompute design | 1 |
restore the unconditional harvested_deferrals clear |
1 |
| relax the snapshot arm guard | 2 |
move the disarm back outside the non-fixable else |
2 |
The two re-base sites redden disjoint rows, which is the partition the design predicts: site 1 protects the continuation, site 2 protects the attempt after a rollback. Each half of the snapshot fix is likewise insufficient alone.
Marker files in the new dev-leg rows live outside the project tree — inside, the marker is itself untracked proof of work and the row goes vacuous.
Suite green on py3.13 and py3.14 locally; CI 9/9.
…405) `_rollback_or_pause` always resets to `task.baseline_commit`, which is phase-scoped. Every piece of harvest provenance is attempt-scoped: `harvested_deferrals`, `harvest_wrote_ledger`, `ledger_changed_before_harvest` and the digest they are measured against. Those two scopes coincide on every run where at most one attempt's work sits above the baseline — and the FIXABLE retry is the only construct in the engine that breaks that, because it keeps the tree, and the harvest's ledger write with it, on purpose. A. The proof-of-work gate could accept an empty implementation. Attempt N harvests, passes the artifact gate on real source, then fails `verify_commands_outcome` FIXABLY (the only `fixable=True` source on the dev leg, and it runs only after `outcome.ok`, so attempt N necessarily did real work). Attempt N+1 cleared the flag; the repair session reverted the source; its own harvest deduped to nothing so the flag stayed False; the exclusion returned `()`; and `has_changes_since` counted attempt N's ORCHESTRATOR-written ledger line as the repair session's proof of work. The verify commands then pass — plausible precisely because the offending change was reverted — and an empty implementation PROCEEDs. Scope: an IGNORED ledger is invisible to `untracked_files` (`--exclude-standard`) and to `git diff`, so it can never be false proof of work; this reaches a tracked, or untracked-but-not-ignored, one. Preserving the flag alone does not fix it. `ledger_changed_before_harvest` is recomputed against the phase-scoped digest, so attempt N's surviving entry makes it True and the exclusion short-circuits regardless. Latching THAT across the continuation as well is the obvious pairing and is wrong twice over: it re-opens the hole `161c85e` closed — a path-granular mask hides a repair session whose honest work genuinely IS a ledger entry, and the false "no changes" pauses the run under the shipped default — and, three attempts on, it re-creates hazard A from the other side. So the reference MOVES instead: `baseline_ledger_digest` becomes "the last ledger state in which every post-`baseline_commit` byte is accounted for", re-based forward onto the kept tree at the fixable branch and back onto the restored snapshot in the rollback `finally`. The second re-base is computed from the snapshot via a new `_digest_of`, so no new raise site appears inside a `finally` that is usually propagating `RunPaused`. B. Findings were lost under isolation. The per-attempt clear of `harvested_deferrals` fires on a fixable continuation too, so: attempt N harvests, the fixable retry keeps the tree AND the entry, attempt N+1's session STALLS (the harvest never runs), the budget is spent, and `_defer` reaches `_carry_harvested_deferrals` with an empty record — the real entry dies with the unmerged worktree, with no journal line to show for it. `feedback is None` is the discriminator for both, not a new persisted field: it is assigned in exactly three places in `_dev_phase` and never reset per-iteration, so at the top of iteration k it means precisely "iteration k-1 took the fixable branch" — the same idiom `_restore_patch`'s guard already uses. The local is lost on both re-entries and neither loses safety; each of the four sites says why. Tests: T2i pins the design (a repair session whose entire honest diff is its own ledger entry must PROCEED — the row the rejected latch-the-answer design reddens), T2j its partition partner (the same chain with an empty repair session must NOT), and a worktree row for B. Ablated against the whole suite: each of the flag guard, the forward re-base, the rejected design and the record clear reddens exactly one row and nothing else. The post-restore re-base is defensive until the chain-scoped snapshot lands next; it is pinned there.
…hain (#405) `_rollback_or_pause` always resets to `task.baseline_commit`, which is captured once per `_dev_phase` entry. The pre-harvest ledger snapshot was captured once per ATTEMPT. A fixable retry is the only construct that lets the two diverge — it keeps the tree, so several attempts accumulate above one baseline — and there the divergence loses work: attempt N harvests DW-1, fails FIXABLY (tree and entry kept), and disarms; attempt N+1 re-arms over a tree that ALREADY holds DW-1; attempt N+1 fails NON-fixably; the reset discards attempt N's code too; the `finally` writes S_{N+1} back — DW-1 included. The finding then names deleted code, stays open, and the next sweep drives work for it. That is the `20d3dc0` bug class in a sequence the per-attempt snapshot cannot reach. Both halves are required and each alone is insufficient (ablated separately; each reddens both new rows): * the arm's guard gains `feedback is None`, so a fixable continuation does not re-arm and the chain keeps its FIRST snapshot. The `or not captured` relaxation stays — it is what lets a chain whose snapshot was spent by a pause re-arm from disk instead of running blind; * clear site 2 moves into the non-fixable `else:` body, below the try/finally and above `continue`. Structurally the same predicate at that point, but it says the thing directly, and it keeps the one property the old placement bought: the `RunPaused` raise still skips it. The rolling ledger reference's backward re-base becomes load-bearing here and is now pinned: without it the restore's own unlink reads as somebody else's write on the next attempt, the exclusion stands down, and a session that writes no source but re-harvests its spec PROCEEDs on the ledger the orchestrator just re-created. The comment the old placement carried does not survive scrutiny and is replaced rather than moved. Its second clause said control never reaches site 2 on the pause leg "because the pause's own resume replays this same attempt and still needs the snapshot" — but `0a8088f` later added the explicit pause-leg disarm with the opposite rationale. Its first clause named a hazard the code cannot produce: a fresh attempt N+1 re-arms unconditionally, so no stale arm ever reached a replay. Trade-off, stated: on the `rollback_on_failure = off` default the tree is kept and the restore now reverts the whole chain's ledger writes rather than the last attempt's. Defensible — the notice's own remedy is `git reset --hard <baseline_commit>`, the same phase baseline, so the revert matches the discard the operator is told to perform. Residual: an operator who repairs by hand and resumes WITHOUT resetting keeps the code and loses the entries; that loss was one attempt deep and is now chain deep. Rejected: reverting the harvest on the fixable leg (the entry describes work that is being KEPT, and `test_harvest_retry_reharvests_after_the_rollback` refuses it); restoring to a phase-baseline ledger TEXT (contradicts the documented reason the arm sits after `_run_session`, so the session's own edits survive the revert); entry-granular revert (genuinely better and also fixes the pause leg's tree/ledger asymmetry, but new `deferredwork` machinery — worth an issue, far too large for a hotfix). Not closed here: `_finish_inflight`'s restart arm resets to the same phase baseline with no ledger restore at all, then `_dev_phase` drops the snapshot unconsumed at site 0. A restore cannot simply be bolted on — that site also handles the `cause="resolved"` re-drive, which deliberately preserves the artifacts folder through the reset, so a blind restore would fight the human's correction. It cannot escalate into the proof-of-work hazard: the fresh `_dev_phase` entry re-captures `baseline_untracked` and `baseline_ledger_digest` with the stale entry already present.
…ledger (#405) `dc4f934` closed the DEFER leg of this. The DONE leg has the same hole and is the one a normal run actually takes. Under `scm.isolation = "worktree"` the harvest writes `workspace.paths.deferred_work` — the UNIT WORKTREE's ledger — and `finalize_commit` stages with `git add -A`, which SKIPS a gitignored path in silence. So on a project that gitignores its ledger (this repo's own shape: the default home is under a gitignored artifacts dir) the entry never rides the branch, `_merge_local` has nothing to bring over, and `close_unit_workspace(success=True)` removes the worktree unconditionally. The DONE leg takes no `capture_diff`, unlike DEFER, so not even a `changes.patch` copy survives: the finding is simply gone. `_integrate_unit`'s DONE arm now calls the carry, behind a new base hook `_carry_isolated_ledger_writes` — the seam a sweep bundle's ledger close will share. Placement is load-bearing, and both halves are ablated: - AFTER `_merge_local` returns, never before. On a project whose ledger IS tracked the merge is what puts the open entry in front of `append_entry`'s dedup; carrying first files a fresh id and commits it, and the merge then lays the branch's own copy over that. - After it RETURNS, not inside it. The ledger write is unguarded by design; placed before `close_unit_workspace` an `OSError` would cost the unit its teardown — the price the DEFER leg has no choice but to pay. - Not on the escalation legs: both `_keep_branch_and_escalate` calls always raise `RunPaused`, so the line is unreachable there. Deliberately — those legs keep the branch for a HUMAN to merge, and that merge brings the branch's own entry with it. No idempotence guard is needed. `append_entry` dedups against OPEN entries, so a duplicate would need the merged copy to be `status: done` already; the only writer that closes an existing entry is `deferredwork.mark_done`, whose in-story caller closes strictly `task.dw_ids` — assigned once at bundle materialization and never appended to — while a harvested entry always gets a fresh `DW-<next>`. `test_a_done_isolated_unit_files_its_harvest_exactly_once` passed before this only because its fixture ledger was not gitignored; it now also asserts the carry ran and deduped to nothing, and added no commit of its own.
…#405) Sibling of the harvest carry, one layer up. `_close_bundle_ledger_when_spec_status` writes `self.workspace.paths.deferred_work` — under `scm.isolation = "worktree"` the unit worktree's. With a GITIGNORED ledger seeded into the worktree (`scm.worktree_seed`), the flip lands there and `finalize_commit`'s `add -A` then skips the ignored path in silence, so it never rides the branch and the merge brings nothing over. The bundle's entries stay `open`, `deferredwork.open_ids` re-bundles them, and every later sweep re-triages and re-drives work that is already done — an unbounded loop, not a one-time drop. `SweepEngine` overrides `_carry_isolated_ledger_writes` (introduced with the harvest carry) to re-apply the closes to the MAIN checkout after the merge. `mark_done` is idempotent, so a project whose ledger is TRACKED — where the flip merged normally — gets a no-op, the same property that makes the harvest carry safe unconditionally. The record is the INTENT (`task.dw_ids`), never the ids `mark_done` managed to flip. The close runs TWICE on a landing bundle — after dev, and again from `_verify_review`'s idempotent reclose — and the second run finds every id already `done`, so its `marked` is empty. Keying the record off it would wipe the record on exactly the run that needs it, the trap `harvested_deferrals` already paid for once. DONE leg only: `_defer` keeps calling `_carry_harvested_deferrals` directly. A defer discarded the code a close claims to have resolved, and a wrong `done` is invisible to every future sweep, since `open_ids` re-bundles only `open` entries. SCOPE — deliberately narrower than it first looks. An UNSEEDED gitignored ledger is a different, still-open hole and no DONE-leg carry can reach it: the worktree checks out tracked files only, so the ledger is absent there, `verify_review_bundle` (which reads the worktree's copy) never sees the ids `done`, and the unit DEFERS on a fixable retry rather than landing. That failure is loud (`review-verify-failed`) where this one was silent. Filed separately. Ablation also removed a `_generic_dev()` guard the first draft carried: it reddened nothing, and correctly — `bundle_closes_intended` is only ever assigned on the generic path, so the record already is the guard, and a second predicate saying the same thing is a branch no test can falsify.
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: 4c15904274
ℹ️ 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".
| # never before: on a project whose ledger IS tracked the merge is what | ||
| # puts the open entry in front of `append_entry`'s dedup, and carrying | ||
| # first would file a fresh id and then merge the branch's copy on top. | ||
| self._carry_isolated_ledger_writes(task) |
There was a problem hiding this comment.
Under scm.isolation = "worktree" with an ignored ledger, if the host exits after _merge_local has merged the branch (and normally deleted its worktree) but before this call runs, the only copy of the harvested deferrals or bundle closes is lost. The task was already persisted as DONE, so resume skips it in _finish_inflight and never retries this carry. Fresh evidence beyond the earlier successful-unit carry report is this post-merge crash window; persist an integration/carry marker and replay the carry for terminal merged tasks before considering them recovered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in a784373.
Mechanism
_finalize_commit_phase does advance(task, Phase.DONE) + self._save(), so DONE is
durably persisted before _integrate_unit runs at all. The DONE arm then does
_merge_local(task, unit) → _carry_isolated_ledger_writes(task), and _merge_local ends
with close_unit_workspace(unit, success=True, …), which removes the worktree
unconditionally and — unlike the DEFER leg — takes no capture_diff. So a death between
those two calls destroys the worktree's copy and skips the carry. On resume _finish_inflight
opens with if task.terminal: continue, and DONE is terminal, so the task is never looked at
again.
The window is not narrow either: it spans clean_incoming_collisions, merge_branch, the
post_merge hook (arbitrary plugin code) and the whole worktree/branch teardown.
Two corrections to the account
1. "The only copy is lost" is not right — nothing was unrecoverable. Both payloads are
already persisted to state.json and survive the crash intact: harvested_deferrals and
bundle_closes_intended (both are persisted deliberately, because a replayed harvest dedups
against the dead attempt's own entries and a re-derived record comes back empty). So the
persistence half of the suggested remedy already existed — only the replay half was
missing, which is what made this cheap to close.
2. The sweep half is a liveness bug, not data loss, and it is the more severe one. A lost
bundle close leaves its ids open; deferredwork.open_ids re-bundles them, and the
suppression filter failed_ids covers DEFERRED/ESCALATED but not DONE. So every later
sweep re-triages and re-drives work that already merged → the re-drive changes nothing →
"no changes since baseline" → non-fixable RETRY → the run pauses under the default
scm.rollback_on_failure = off. Unbounded rather than one-time, and silent besides:
_warn_stranded_bundles filters terminal tasks.
Why only the DONE leg
The sibling terminus already gets this right. _defer does advance(DEFERRED) → carry →
_save() — the carry runs before the terminal phase is persisted, so a death there leaves
a non-terminal persisted phase that _finish_inflight re-drives normally. DONE is the only
leg that persists terminal and then carries. That asymmetry is the entire bug.
The fix
One persisted latch StoryTask.isolated_ledger_carried, set at the carry call sites with
its own _save(), plus a _replay_unlatched_ledger_carries() pre-pass in run().
- A latch, not idempotency alone.
append_entrydedups onorigin:+source_spec:
against open entries only, so once anything closes the carried entry a blind replay files
a duplicate under a fresh id. (mark_doneis fully idempotent — the harvest half is what
forces the latch.) - Set at the call sites, never inside the hook, because
SweepEngine's
_carry_isolated_ledger_writesoverride runs its half aftersuper(). - In
run(), and it must precede_loop().SweepEnginereplaces_loopwholesale but
does not overriderun(), so that frame is the one both engines share —sweep.pyis not
edited at all; the sweep half arrives through the override seam. And the ordering is
load-bearing: a carried close has to leave the open set before the loop reads the ledger, or
a fresh triage re-bundles merged work. - Guards: DONE only (a DEFER discarded the code its closes claim to resolve; ESCALATED keeps
the branch for a human whose merge brings the entry itself), never in-place, never while the
worktree is still mounted — that state is indistinguishable from "the branch never landed",
and carrying then would file an id the human's later merge duplicates. It costs the narrower
merged-but-not-torn-down sub-window, where the carry is lost exactly as today, but is never
wrongly applied.
Failure direction is safe: a lost latch means a replay, and a replay is a no-op in the ordinary
case (mark_done returns False, append_entry dedups against the still-open entry).
Ablation table
One ablation per edit, run against the whole suite (3025 collected, +10 rows), re-baselined
between rounds. Every edit reddens ≥1 row and no two edits are identical.
| Ablation | Red rows |
|---|---|
persist the latch (to_dict/from_dict) |
…round_trips, …defaults_false_for_legacy_state, …second_resume_files_no_duplicate, …clean_landing_latches_its_carry |
latch + _save() at the DONE-arm carry site |
…clean_landing_latches_its_carry |
the replay pass (call removed from run()) |
…crash_after_the_merge_replays_on_resume, …second_resume_files_no_duplicate, …bundle_close_lost_to_a_crash_replays… |
| DONE-phase guard | …deferred_bundles_closes_are_never_replayed |
| in-place guard | …in_place_done_story_never_replays… |
| mounted-worktree guard | …crash_before_the_merge_leaves_a_mounted_worktree_uncarried |
| latch check in the guard | …second_resume_files_no_duplicate, …clean_landing_latches_its_carry |
| empty-record guard | …silent_when_the_crashed_unit_had_nothing_to_carry |
call moved after _loop() |
…bundle_close_lost_to_a_crash_replays… |
call moved into _finish_inflight |
…bundle_close_lost_to_a_crash_replays… |
either _save() alone |
0 — stated gap |
Three things the ablations corrected rather than confirmed:
- The empty-record guard reddened nothing on the first pass. It was a live branch no row
could falsify, so it became one:resume-ledger-carryis a recovery signal an operator reads,
and the pass must not announce a rescue that carried nothing. - The escalation row does not falsify the DONE guard.
_keep_branch_and_escalatehardcodes
keep_failed=True, so an ESCALATED unit's worktree is always still mounted and the
mounted-worktree guard answers first. The row that actually pins the phase guard is the
deferred-bundle one, which setskeep_failed=Falseprecisely so the worktree is gone. - Guard order is load-bearing for testability.
Path("")isPosixPath(".")and reads as a
directory, so a barePath(task.worktree_path).is_dir()silently absorbs the in-place case and
leaves the in-place guard unfalsifiable. The mounted guard therefore comes first with an
explicit truthiness clause.
The two _save() calls are the one honest gap: run()'s finally: self._save() persists on
every in-process exit path, so no test can observe a lost latch. Their justification is the
threat model itself (SIGKILL / power loss, which does not run finally) plus the concrete
distance to the next natural save — in the base engine a _pick_next returning None runs a
whole nested _maybe_auto_sweep("run-end", …) first, and a nested sweep is the single most
likely thing to close the entry just carried.
Suite is 3024 passed / 1 skipped on 3.13 and 3.14; trunk check clean.
…sume (#405) `_finalize_commit_phase` persists `Phase.DONE` (`engine.py`) BEFORE `_integrate_unit` merges and carries, and `_merge_local` ends with `close_unit_workspace(success=True, …)`, which removes the worktree unconditionally with no `capture_diff` on this leg. A host death between the merge and `_carry_isolated_ledger_writes` therefore destroys the worktree's copy of the orchestrator's ledger writes and skips the carry that was going to rescue them — and `_finish_inflight` opens with `if task.terminal: continue`, so resume never looks at the task again. The window is not narrow: it holds `clean_incoming_collisions`, `merge_branch`, the `post_merge` hook (arbitrary plugin code) and the worktree/branch teardown. The sibling DEFER terminus already gets this right — `_defer` carries BEFORE its terminal `_save()`, so a death there leaves a non-terminal persisted phase that ordinary resume re-drives. DONE is the only leg that persists terminal and then carries, and that asymmetry is the whole bug. Nothing was unrecoverable, which is what makes the fix a replay rather than a rescue: both payloads (`harvested_deferrals`, `bundle_closes_intended`) are already persisted to `state.json` and survive the crash. Only the application to the main ledger was missing. One persisted latch (`StoryTask.isolated_ledger_carried`) set at the carry call sites with its own `_save()`, plus a `_replay_unlatched_ledger_carries` pre-pass in `run()`. A latch rather than idempotency alone: `append_entry` dedups on `origin:` + `source_spec:` against OPEN entries only, so once anything closes the carried entry a blind replay files a duplicate under a fresh id. (`mark_done` is fully idempotent; the harvest half is what forces the latch.) Set at the CALL SITES, never inside the hook — `SweepEngine`'s override runs its half AFTER `super()`. `run()` is the placement, and it is forced twice over: `SweepEngine` replaces `_loop` wholesale but does NOT override `run()`, so that frame is the one both engines share and `sweep.py` needs no edit at all — the sweep half arrives through the `_carry_isolated_ledger_writes` seam. And it must PRECEDE `_loop`, or a carried close reaches the ledger after the sweep has already re-triaged. Guards, each independently falsifiable: DONE only (a DEFER discarded the code its closes claim to resolve; ESCALATED keeps the branch for a human whose merge brings the entry itself), never in-place, never while the worktree is still mounted (indistinguishable from "the branch never landed"), and silent on an empty record. The mounted guard carries an explicit truthiness clause and comes first on purpose: `Path("")` is `PosixPath(".")` and reads as a directory, so a bare `is_dir()` would absorb the in-place case and leave that guard unfalsifiable. The sweep half is the sharper one and is a LIVENESS bug rather than data loss: a lost close leaves ids `open`, `open_ids` re-bundles them, and the `failed_ids` suppression filter covers DEFERRED/ESCALATED but not DONE — so every later sweep re-drives already-merged work, which changes nothing, reads as "no changes since baseline", takes a non-fixable RETRY and PAUSES the run under the default `scm.rollback_on_failure = off`. Silent besides: `_warn_stranded_bundles` filters terminal tasks. Ablated one-per-edit against the whole suite (3025 collected, +10 rows). Every edit reddens ≥1 row and no two are identical. Two `_save()` calls are the stated gap — measured at 0 red, because `run()`'s `finally: self._save()` persists on every in-process exit path and the threat model is SIGKILL/power loss, which does not run `finally`.
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.
|
Round close-out ( Codex P2 at Two things worth stating plainly:
Greptile's latest "review" ( Suite: 3024 passed / 1 skipped on 3.13 and 3.14 (+10 rows, 3025 collected). |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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.base-shim(above),
skills.dev-rendererandskills.dev-renderer-config(a renderer stub with norender script, or no central
_bmad/config.toml),skills.dev-renderer-sources(a stubmissing a source it declares in
workflow.md), andpolicy.isolation-repo-root. Two arewarnings:
skills.customize-legacy(an override still named for the old skill) andgit.render-tracked(rendered output already committed).Harvesting and resolution key on content, never on the installed skill name, so both
eras work.
Commits
The rename itself:
6b3ccda2b1a1509754f8410ce26cdec2cabReview-round hardening
Everything after
dec2cabis review response — ten Codex rounds, each of which foundsomething, then an adversarial pass over the whole diff run to the same standard, so the
seams those rounds kept landing on were closed rather than patched one finding at a time.
The last two findings and the work that followed them:
64c4f2c84783943de62701e5c030skills.dev-renderer-sources— a renderer stub is gated on the sources it declares29ea28fa8ab809d54d59507918cd0870941d1e60afdac373brepo_rootoverride — the refusal gate for #414 — and tracked render output warns, per #409e3dba252edd753location: n/a, per-role workflow marker, one spec under one name79a8fd00a8088fbaa2bac2a8714ecbaeb12#414 stays open: this branch ships the refusal gate, while the
repo_root/projectplumbing fix belongs on
main.Two ablations with no behavioural witness
Every gate in this PR was ablated whole-leg and conjunct-by-conjunct against the full suite.
Two of them cannot be caught by behaviour, and they are stated here rather than papered over —
they are the honest limit of the ablation discipline used everywhere else in this PR:
_absent_skill_filesand_merge_traversabledeliberately share_walk_traversable_files. An un-shared copierrecursion is byte-identical to the walker today, so no test can observe the difference
until one half is edited — at which point the rglob-substitution row catches the gate half.
Sharing is a structural guarantee, not a tested one. Measured twice, in two sessions:
whole suite green both times.
_save()that follows the pre-harvest arm (C1, "row i") is invisible to everybehavioural test.
run()'sfinally: self._save()re-persists the armed fields on any softcrash a fixture can inject, so the whole crash-replay family stays green without it. It is
caught only by a durability test written for that one purpose —
test_the_pre_harvest_snapshot_is_persisted_before_the_harvest_runs, which readsstate.jsonoff disk at the moment the harvest runs rather than asserting an outcome. The identical
masking recurs for the pause-path disarm and is closed the same way, by
test_the_disarm_is_on_disk_before_the_run_even_records_the_pause.Verification
uv run pytest -q— 2976 collected, 2975 passed / 1 skipped, on 3.13 and on a real3.14.6 interpreter (the version split in
_is_diris invisible to any single lane).trunk check --all --no-fixclean over all 230 files;python scripts/release.py checkgreen.rels' separators and for the symlink-free witnesses written to run there. (A ninth row reads
passfrom CodeRabbit; it is a skip, not a review result — reviews are disabled for thisbase branch, and it reported the draft skip before that. Codex, triggered manually, is the
external review of record here.)
each row run against the whole suite with its prediction written down first, and the rows
that did not redden read as carefully as the ones that did. Ablations run with
PYTHONPATH=<copy>/src: this venv resolvesbmad_loopthrough an editable.pthholdingthe main checkout's absolute path, so an ablated copy is a false green without it.
CLI, ablated both ways: validate ok line names the resolved primitive; legacy customize toml
→
skills.customize-legacy; stub without the render script →skills.dev-renderer;shim-only →
skills.base-shimFAIL andrunaborts rc 1;run --dry-runbanners on stderrand emits 0 bytes for a healthy project. Three later legs got their own E2E on the same
pattern — proof-of-work, the paused snapshot, and the dropped hook-config seed — each run
against the fix and against the reverted leg, with the import sentinel proven first.
uv.lockdiff is one line: bmad-loop0.9.0→0.9.1. The TUI was unchanged at the releasecommit, so asset regeneration auto-skipped; the later
dac373bisolation guard touchestui/app.pybut ships no visual change.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
Bug Fixes
Documentation