fix(setup): register into the BMAD v6.10 TOML config layout (#258)#285
fix(setup): register into the BMAD v6.10 TOML config layout (#258)#285pbean wants to merge 8 commits into
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
✨ Finishing Touches🧪 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 |
Comprehensive review findingsRecommendation: request changes; do not merge as-is. The v6.10 registration design is sound, but the TOML editor can corrupt a valid human-authored config and still report success. Triage: 2 high, 3 medium. The focused and full test suites are green, so these are untested behavioral gaps rather than ordinary test failures. Findings
Remediation plan
Verification
The documented catalog-regeneration caveat, manual pre-rename cleanup, and issues #154/#259 were treated as accepted or out of scope rather than review findings. |
The hand-rolled editor for _bmad/custom/config.toml found a table's span with a
regex, which is not safe on TOML: a header line may carry a trailing comment or
quoted key components, and a [-leading line may sit inside a multi-line string or
array. Each miss produced INVALID TOML from valid input -- a duplicate table when
the header was missed, a truncated value when a string body was mistaken for the
next table -- and the result was written before it was validated, so the script
still reported status: success and exited 0.
That failure was silent all the way down. BMAD's resolve_config.py loads the
custom layer as optional, so an unparseable file is swallowed as a warning and
merged as {}: our table is not registered AND every override the user wrote in
that file stops applying, with a zero exit code everywhere.
merge-config.py:
- scan_table_headers() replaces the regex: a real scanner tracking single- and
multi-line strings, comments, and array/inline-table depth, normalising bare and
quoted key paths.
- verify_candidate() is the gate that makes the edit trustworthy. It checks the
RESULT, not the scanner: the candidate must parse and must differ from the
original in nothing but modules.<code>. Duplicates, truncation and clobbered
neighbours all fail it, so a scanner bug refuses the run instead of corrupting a
config. Nothing is written unless it passes; a refusal exits non-zero and leaves
the file byte-identical. Needs tomllib, so the TOML branch now requires Python
3.11+ -- as BMAD's own resolver already does.
- Newline convention, and file mode, are preserved: text-mode I/O translated CRLF
to LF, rewriting every line of a CRLF config for a one-table edit.
- Trailing blank/comment lines are backed off the span at EOF too, so a footer
comment on the last table survives.
- The TOML branch no longer PARSES legacy YAML under --legacy-dir. It discards
those values anyway, and it read them after the write: a malformed leftover
raised, reporting failure for a registration that had succeeded.
merge-help-csv.py:
- align_rows() re-keys our rows into the catalog's own column order by name.
Appending positionally was only correct while the two schemas agreed; BMAD has
already renamed help columns once, and a reorder silently filed each description
under skill. Columns the target cannot hold are reported in columns_dropped.
- Both outputs are rendered in full before either is committed, and the source is
required to carry exactly one module code on the TOML branch.
Both scripts commit every write through a temp file and one atomic rename
(mirroring platform_util.atomic_replace, duplicated because these ship standalone),
so _config/bmad-help.csv -- shared with every module and not hash-tracked -- can
never be left truncated. An OSError now surfaces through the JSON error contract
instead of a traceback.
21 new tests, each verified to fail against the previous code.
…review) The docs promised more than the code delivered, and explained a failure mode wrongly: - SKILL.md said "every comment ... comes back untouched" of custom/config.toml. Comments inside the [modules.bmad-loop] table do not survive -- that table is rewritten wholesale, which IS the anti-zombie guarantee. State the contract that actually holds: every unrelated table, every comment outside our table, and the file's line endings and mode. - SKILL.md explained toml_validated: false as meaning only that tomllib was unavailable. A parse failure took the same path, so a corrupt config read as a Python-version footnote. The edit is now verified before the write, so the field is always true on success and a failure exits non-zero having written nothing. - Record why that matters: BMAD's resolver treats an unparseable custom layer as an empty one, so a corrupt file silently drops the user's own overrides too. - Note the Python 3.11+ requirement on the TOML branch, the two errors only the user can clear (an unparseable existing file, an older interpreter), that a non-zero exit always means nothing was committed, and the new columns_dropped field.
Review findings addressed —
|
Two self-review fixes on the previous commit: - Stacked pytest skipif markers are ALL evaluated, so the separate os.geteuid() marker on the catalog-truncation test would raise AttributeError at collection time on Windows -- breaking the whole module there, not just that test. One marker with a short-circuiting `or` instead. - _skip_string consumed unbounded quotes after a multi-line string's closing delimiter (and carried an always-true guard). TOML allows at most two, so a value like `"""ends with a quote""""` now closes where TOML says it does.
…-layout-258 # Conflicts: # CHANGELOG.md # README.md
1 participant
Problem
BMAD-METHOD v6.10 consolidated onto TOML. Its resolver (
_bmad/scripts/resolve_config.py)reads exactly four files —
config.toml,config.user.toml,custom/config.toml,custom/config.user.toml— and neverconfig.yaml./bmad-helpreads exactly onecatalog,
_bmad/_config/bmad-help.csv, the installer's assembled concatenation of theper-module
_bmad/<mod>/module-help.csvfiles./bmad-loop-setup's two merge scripts still wrote the pre-6.10 root_bmad/config.yamland
_bmad/module-help.csv. Neither is read by anything on v6.10, and neither is even trackedin
_bmad/_config/files-manifest.csv. Net effect: on a v6.10 project the module wasregistered nowhere and its skills never appeared in
/bmad-help.Two claims in the issue were checked and corrected (details in the
validation comment):
uv tool install+bmad-loop initproject —manifest.yamllists onlycore+bmm,there are 0
bmad-looprows anywhere, andinitnever touches_bmad/. A no-op wouldleave the module permanently invisible, so the fix has to register into the new layout.
validatereports BMAD config OK from the TOML layout." No —cli.py→bmadconfig.pyreads
_bmad/bmm/config.yaml, the per-module YAML, which v6.10.1 still generates andmanifest-tracks. The runtime is unaffected by this issue (that migration is bmadconfig: read the four-layer central TOML as primary, legacy bmm/config.yaml as fallback #154).
Design — detect the layout, register into what the project actually reads
Layout is TOML iff
<bmad-dir>/config.tomlexists (purePath.exists(), no parsing, noimports). Both scripts gained a required
--bmad-dirand carry their own copy ofdetect_layout/detect_orphans— they ship standalone into user projects, so a sharedimport module would break them.
TOML branch (v6.10+) writes three things and nothing else:
_bmad/custom/config.toml[modules.bmad-loop]— surgical text edit of that table's span only_bmad/bmad-loop/module-help.csv_bmad/_config/bmad-help.csvLegacy YAML branch (pre-6.10) is byte-for-byte today's behavior, still driven by
--config-path/--user-config-path/--target. Each branch fails loudly with thelayout-specific argument it is missing.
Why no core values on the custom layer
custom/config.tomlis the layer BMAD documents as "never touched by the installer" — whichis exactly why it is the right home for our module table, and exactly why
user_name,output_folderand friends must not go there. The custom layer wins over the installerlayer, so a value pinned there would silently override every future installer answer. Core
config stays installer-owned;
core_updatedis alwaysfalseanduser_keysalways empty onthis branch, by design. Since
assets/module.yamldeclares zero prompt variables, there isnothing to collect on this layout at all.
Writing TOML without a writer
There is no stdlib TOML writer, and
custom/config.tomlis a human-authored, comment-bearingfile — round-tripping it through a parser + emitter would destroy it. So the table is
hand-emitted and spliced in via
upsert_module_table(); everything outside the replaced spancomes back byte-identical, and the result is validated by re-parsing with
tomllib(lazyimport;
toml_validated: false+ a stderr note when unavailable). No new dependency wasadded —
tomli-wwas deliberately avoided because it would deepen #259.Orphans: report, never delete
Inert pre-6.10 leftovers directly under
_bmad/(config.yaml,config.user.yaml,module-help.csv) are reported asorphans_detected—[{"path", "has_module_entries"}]—and never deleted, per the #64 never-delete-live-config doctrine.
legacy_configs_deleted/
legacy_csvs_deletedstay[]. The skill now tells the user these are safe to remove byhand and that bmad-loop will not do it for them.
Caveat: the installer regenerates the catalog
_config/bmad-help.csvis generated, not hash-tracked infiles-manifest.csv— merging intoit desyncs no manifest, but a BMAD installer re-run regenerates it and drops our rows.
Expected, and now documented in the skill's Confirm step: re-run
/bmad-loop-setupto restorethem.
Commits
e03d579—merge-config.py:--bmad-dir,detect_layout,emit_module_table,upsert_module_table,detect_orphans, TOML branch (+8 tests)606f76a—merge-help-csv.py: own layout/orphan copies, per-module CSV + catalog merge,stale
HEADERconstant fix (after/before→preceded-by/followed-by) (+8 tests)fff4c6b— docs:SKILL.mdtwo-layout rewrite,docs/setup-guide.md(what setup does +uninstall),
README.md,data/skills/README.md, CHANGELOGSKILL.md's two script command lines were stale after the first two commits (neither passedthe now-required
--bmad-dir); the docs commit closes that seam. The commands now carry thelegacy-only arguments too, so one command line works on both layouts — they are simply
ignored on the TOML branch.
One drive-by content fix:
assets/module-help.csv'soutputscell forbmad-loop-setupreadconfig.yaml entries, which is wrong on v6.10. It is now the layout-neutralmodule config + help entries. No test asserted the old string.End-to-end proof
A throwaway v6.10 fixture (
config.toml+config.user.toml+ a comment-bearingcustom/config.tomlcarrying an unrelated[agents.bmad-agent-pm]table + a realistic_config/bmad-help.csv), both scripts run against it twice, then BMAD's own unmodifiedresolve_config.pyused as the oracle.(a) BMAD's resolver reads our registration back:
The pre-existing comments and the unrelated
[agents.bmad-agent-pm]table survived verbatim:(b) Exactly one block of our rows in the catalog after two runs (2 pre-existing rows kept):
Run 1 reported
table_replaced: false/rows_removed: 0, rows_added: 2, total_rows: 4;run 2 reported
table_replaced: true/rows_removed: 2, rows_added: 2, total_rows: 4.toml_validated: trueon both.(c) No legacy files created:
(d) Orphans reported, never deleted — a second fixture pre-seeded with pre-6.10 leftovers:
{ "layout": "toml", "orphans_detected": [ { "path": ".../_bmad/config.yaml", "has_module_entries": true }, { "path": ".../_bmad/config.user.yaml", "has_module_entries": false }, { "path": ".../_bmad/module-help.csv", "has_module_entries": true } ], "legacy_configs_deleted": [], "core_updated": false }All three files survived byte-identical (
config.yamlstillbauto:| version: 0.4.0|bmad-loop:| version: 0.8.0|).Verification
16 new tests in
tests/test_setup_cleanup_scripts.pybehind a_v610_bmadfixture cover bothscripts' TOML branches: custom-table write, comment/unrelated-table preservation (append and
replace-with-a-neighbour-below), no YAML written, no core values leaked, upsert idempotence
including a version bump, per-module CSV, catalog merge + idempotence, catalog created when
absent, root CSV left alone, orphans reported-not-deleted, and both legacy YAML branches
unchanged plus their missing-argument errors.
Note:
test_merge_help_toml_strips_pre_rename_rowsasserts the opposite of what its namesuggests, on purpose — the anti-zombie filter keys on source module codes
(
BMAD Loop Skills), so leftover pre-renameBMAD Automator Skillsrows survive the merge.That is now documented as a manual-delete step in the skill's bauto-migration section, extended
to cover
_bmad/_config/bmad-help.csv.Out of scope (both remain open)
python3invocation prefix vsuv runfor PEP 723 headers. Theprefix is left exactly as-is here; bundling it was explicitly declined.
_bmad/bmm/config.yamlread inbmadconfig.py. That per-moduleYAML is still live and installer-written at v6.10.1, so the orchestrator runtime is
unaffected by this PR. Untouched.
Closes #258