feat: unify bmad-build onto the shared snapshot renderer with a customizable defer sink - #2657
Conversation
Greptile SummaryThis PR unifies
Confidence Score: 4/5Safe to merge. The renderer consolidation is well-contained: the shared renderer already served bmad-build-auto, the new defer key has enforced non-empty semantics validated by tests, and the retired render.py is fully replaced with matching behavior. The change is large in line count but low in behavioral risk — the shared renderer was already proven for bmad-build-auto, and the new defer key adds a customization seam rather than changing default behavior. The test suite covers defer override routing, empty-defer HALT, SKILL.md command dispatch, immutability, and the orchestrator contract. The two observations are purely about test ergonomics and do not affect the correctness of the production skill or its defaults. Files Needing Attention: test/test-build-auto-renderer.js could benefit from a failure accumulation structure matching test-build-auto-contract.js; test/test-build-auto-contract.js may need additional locale checks if other language builds of build-auto.md exist.
|
| Filename | Overview |
|---|---|
| src/bmm-skills/4-implementation/bmad-build/render.py | Deleted: ~420-line skill-local renderer replaced by the shared render_skill.py |
| src/bmm-skills/4-implementation/bmad-build/SKILL.md | Invocation updated to use shared renderer with explicit --project-root and --skill flags; on-success prose clarified |
| src/bmm-skills/4-implementation/bmad-build/customize.toml | Adds single-line defer key with ledger default; updates open_spec comment to remove empty-string disable path; adds {skill-root} note for review layers |
| src/bmm-skills/4-implementation/bmad-build-auto/customize.toml | Adds multi-line defer key with spec frontmatter default; adds non-empty validation note and cross-skill placeholder warnings |
| src/bmm-skills/4-implementation/bmad-build-auto/step-04-review.md | Defer action bullet slimmed to field spec only; recording instruction moved to a dedicated ### Defer recording section that splices {workflow.defer} at column 0 |
| src/bmm-skills/4-implementation/bmad-build/step-04-review.md | defer action bullet now delegates to {workflow.defer} token placed inside the list item; {deferred_work_file} defined in prose as compile-time fallback reference |
| test/test-build-auto-renderer.js | Parameterized to cover both bmad-build and bmad-build-auto; adds 10+ new tests for defer override routing, empty-defer HALT, and SKILL.md command dispatch; test() helper catches failures to stderr without accumulating them for an end-of-run summary |
| test/test-build-auto-contract.js | New contract test suite pinning the orchestrator-facing deferred: frontmatter contract; validates YAML shape round-trips, forbids deferred-work.md references, checks Chinese i18n doc |
| test/test-build-renderer.js | Deleted: retired skill-local renderer test replaced by the parameterized shared suite in test-build-auto-renderer.js |
| package.json | Removes test-build-renderer.js from the test:renderer script |
Sequence Diagram
sequenceDiagram
participant A as Agent
participant R as render_skill.py
participant C as customize.toml
participant S as Step file (rendered)
participant Sink as Deferral Sink
A->>R: uv run render_skill.py --project-root --skill
R->>C: read workflow.defer default / override
R->>S: "splice {workflow.defer} token into recording instruction"
R-->>A: stdout: read and follow /snapshot/workflow.md
Note over A,S: Runtime execution
A->>S: read step-0N.md (call site)
S-->>A: "produce entry {summary, evidence, ...}"
A->>S: follow recording instruction (from defer token)
alt default sink (bmad-build)
A->>Sink: append entry to deferred-work.md
else default sink (bmad-build-auto)
A->>Sink: update deferred: list in spec frontmatter
else custom override
A->>Sink: write to alternate sink (Jira / SQLite / other file)
end
Comments Outside Diff (2)
-
test/test-build-auto-renderer.js, line 24-33 (link)Test failure details lost in renderer suite
The
test()helper here logs failures toconsole.errorand relies on the finalpassed/totalcount as the only summary. Unliketest-build-auto-contract.js, it has nofailuresarray, so a developer looking at CI output has to scroll through all output to find the first failing assertion rather than seeing a re-listed failure block. This is minor in a 29-test suite today but will become friction as the suite grows with morebmad-build-specific cases.Prompt To Fix With AI
This is a comment left during a code review. Path: test/test-build-auto-renderer.js Line: 24-33 Comment: **Test failure details lost in renderer suite** The `test()` helper here logs failures to `console.error` and relies on the final `passed/total` count as the only summary. Unlike `test-build-auto-contract.js`, it has no `failures` array, so a developer looking at CI output has to scroll through all output to find the first failing assertion rather than seeing a re-listed failure block. This is minor in a 29-test suite today but will become friction as the suite grows with more `bmad-build`-specific cases. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
-
test/test-build-auto-contract.js, line 165-169 (link)The contract test pins the English and Chinese reference docs against the new "default sink" qualifier language. If other localized
build-auto.mdfiles exist (e.g.docs/fr/reference/build-auto.md,docs/vi-vn/reference/build-auto.md), they are not validated here, so a future i18n update could silently ship stale orchestrator guidance. Worth adding at least a filesystem glob check that either finds no other locale files or validates each one it finds.Prompt To Fix With AI
This is a comment left during a code review. Path: test/test-build-auto-contract.js Line: 165-169 Comment: **i18n coverage limited to Chinese only** The contract test pins the English and Chinese reference docs against the new "default sink" qualifier language. If other localized `build-auto.md` files exist (e.g. `docs/fr/reference/build-auto.md`, `docs/vi-vn/reference/build-auto.md`), they are not validated here, so a future i18n update could silently ship stale orchestrator guidance. Worth adding at least a filesystem glob check that either finds no other locale files or validates each one it finds. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
### Issue 1
test/test-build-auto-renderer.js:24-33
**Test failure details lost in renderer suite**
The `test()` helper here logs failures to `console.error` and relies on the final `passed/total` count as the only summary. Unlike `test-build-auto-contract.js`, it has no `failures` array, so a developer looking at CI output has to scroll through all output to find the first failing assertion rather than seeing a re-listed failure block. This is minor in a 29-test suite today but will become friction as the suite grows with more `bmad-build`-specific cases.
### Issue 2
test/test-build-auto-contract.js:165-169
**i18n coverage limited to Chinese only**
The contract test pins the English and Chinese reference docs against the new "default sink" qualifier language. If other localized `build-auto.md` files exist (e.g. `docs/fr/reference/build-auto.md`, `docs/vi-vn/reference/build-auto.md`), they are not validated here, so a future i18n update could silently ship stale orchestrator guidance. Worth adding at least a filesystem glob check that either finds no other locale files or validates each one it finds.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "chore(build): align install tests and do..." | Re-trigger Greptile
📝 WalkthroughWalkthroughThe build skills now use shared snapshot rendering and configurable ChangesBuild renderer and deferral flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant BuildCommand
participant render_skill.py
participant RenderedWorkflow
BuildCommand->>render_skill.py: Render selected skill
render_skill.py-->>BuildCommand: Return absolute workflow path
BuildCommand->>RenderedWorkflow: Execute workflow
sequenceDiagram
participant ReviewStep
participant workflow.defer
participant DeferredSink
participant Spec
ReviewStep->>workflow.defer: Send finding fields
workflow.defer->>DeferredSink: Write configured destination
DeferredSink-->>workflow.defer: Return success or failure
workflow.defer->>Spec: Record fallback entry when needed
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
![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
🧹 Nitpick comments (2)
test/test-build-auto-contract.js (1)
55-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
workflowTableandshippedAutoDeferhelpers across both test suites. Both files carry byte-identical copies and both comments ask a maintainer to keep the copies in step by hand. The root cause is the absence of a shared test helper module.
test/test-build-auto-contract.js#L55-L74: moveworkflowTableandshippedAutoDeferinto a new shared module undertest/and import them here.test/test-build-auto-renderer.js#L178-L199: delete the local copies and import the same shared helpers, then drop the "keep the two in step" comments.🤖 Prompt for 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. In `@test/test-build-auto-contract.js` around lines 55 - 74, Move the duplicated workflowTable and shippedAutoDefer helpers into a shared module under test/. In test/test-build-auto-contract.js lines 55-74, remove the local definitions and import the shared helpers; do the same in test/test-build-auto-renderer.js lines 178-199, also removing the comments about keeping copies synchronized.test/test-build-auto-renderer.js (1)
169-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope
shippedDeferto the[workflow]table.
shippedAutoDeferslices the[workflow]table before matching, and the comment on Line 178 states why: only a key inside that table resolves{workflow.defer}.shippedDefermatches^defer = "(.+)"$against the whole file. If adeferkey is later added under another table, this helper can return a value that cannot render, and the test would still pass.Reuse the existing
workflowTablehelper for both.♻️ Proposed change
function shippedDefer() { - const shipped = fs.readFileSync(path.join(SKILLS_SRC, 'bmad-build', 'customize.toml'), 'utf8'); - const match = shipped.match(/^defer = "(.+)"$/m); - assert(match, 'customize.toml ships no single-line defer default'); + const label = 'bmad-build customize.toml'; + const table = workflowTable(fs.readFileSync(path.join(SKILLS_SRC, 'bmad-build', 'customize.toml'), 'utf8'), label); + const match = table.match(/^defer = "(.+)"$/m); + assert(match, `${label} ships no single-line defer default in [workflow]`); const value = match[1];🤖 Prompt for 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. In `@test/test-build-auto-renderer.js` around lines 169 - 176, Update shippedDefer to pass the customize.toml contents through the existing workflowTable helper before matching the defer entry, matching the scoping already used by shippedAutoDefer. Keep the existing assertions and placeholder validation unchanged.
🤖 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/bmm-skills/4-implementation/bmad-build/step-04-review.md`:
- Line 53: Remove all leading indentation before the {workflow.defer} token so
it starts at column 0 in both
src/bmm-skills/4-implementation/bmad-build/step-04-review.md:53-53 and
src/bmm-skills/4-implementation/bmad-build/step-oneshot.md:39-39; apply the same
formatting change at both defer sites.
---
Nitpick comments:
In `@test/test-build-auto-contract.js`:
- Around line 55-74: Move the duplicated workflowTable and shippedAutoDefer
helpers into a shared module under test/. In test/test-build-auto-contract.js
lines 55-74, remove the local definitions and import the shared helpers; do the
same in test/test-build-auto-renderer.js lines 178-199, also removing the
comments about keeping copies synchronized.
In `@test/test-build-auto-renderer.js`:
- Around line 169-176: Update shippedDefer to pass the customize.toml contents
through the existing workflowTable helper before matching the defer entry,
matching the scoping already used by shippedAutoDefer. Keep the existing
assertions and placeholder validation unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 73b72196-c981-4923-a005-5d0135e35cc4
📒 Files selected for processing (20)
docs/how-to/quick-fixes.mddocs/reference/build-auto.mdpackage.jsonsrc/bmm-skills/4-implementation/bmad-build-auto/customize.tomlsrc/bmm-skills/4-implementation/bmad-build-auto/step-04-review.mdsrc/bmm-skills/4-implementation/bmad-build/SKILL.mdsrc/bmm-skills/4-implementation/bmad-build/customize.tomlsrc/bmm-skills/4-implementation/bmad-build/render.pysrc/bmm-skills/4-implementation/bmad-build/step-01-clarify-and-route.mdsrc/bmm-skills/4-implementation/bmad-build/step-02-plan.mdsrc/bmm-skills/4-implementation/bmad-build/step-03-implement.mdsrc/bmm-skills/4-implementation/bmad-build/step-04-review.mdsrc/bmm-skills/4-implementation/bmad-build/step-05-present.mdsrc/bmm-skills/4-implementation/bmad-build/step-oneshot.mdsrc/bmm-skills/4-implementation/bmad-build/sync-sprint-status.mdsrc/bmm-skills/4-implementation/bmad-build/workflow.mdtest/test-build-auto-contract.jstest/test-build-auto-renderer.jstest/test-build-renderer.jstest/test-installation-components.js
💤 Files with no reviewable changes (2)
- src/bmm-skills/4-implementation/bmad-build/render.py
- test/test-build-renderer.js
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@coderabbitai review |
✅ Action performedReview finished.
|
b5fcfe2 to
e1c1079
Compare
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
e1c1079 to
8fc755d
Compare
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
beec7be to
7045e11
Compare
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@coderabbitai review |
✅ Action performedReview finished.
|
7045e11 to
5f46e9f
Compare
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/bmm-skills/ship/bmad-build-auto/customize.toml (1)
115-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
{skill-root}resolves to the rendered snapshot directory.
bmad-build/customize.tomladds a note (lines 145-148 in that file) explaining that{skill-root}inside a review-layer instruction resolves to the rendered snapshot directory, and that overrides must point only at.mdfiles shipped inside the skill directory. This file's review layers use the same{skill-root}/review-prompts/*.mdpattern but carry no equivalent note.Add the same clarification here so an override author does not point
{skill-root}at a file the snapshot renderer never publishes.📝 Proposed addition
# Review layers for the review step. `instruction` is the layer's whole # execution recipe — subagents by default, but an override may run anything # (e.g. an external reviewer via bash). {diff_output} is substituted at run # time. `when` (optional) gates a layer; empty `instruction` disables it. +# +# Inside a review-layer instruction, {skill-root} resolves to this skill's +# rendered snapshot directory. Point it only at .md files shipped inside the +# skill directory, since those are the files published into the snapshot. [[workflow.review_layers]]🤖 Prompt for 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. In `@src/bmm-skills/ship/bmad-build-auto/customize.toml` around lines 115 - 183, Add a clarification comment adjacent to the review-layer instructions in workflow.review_layers explaining that {skill-root} resolves to the rendered snapshot directory and override paths must target only .md files shipped within the skill directory. Keep the existing review-layer behavior and instruction strings unchanged.
🤖 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 `@test/test-build-auto-renderer.js`:
- Around line 490-530: Update the snapshot assertions around renderRoot, the
markdown reference matching, and artifacts to use platform-native path
construction and matching instead of assuming leading `/` or literal `/`
separators. Normalize or construct expected paths consistently with path.sep so
Windows drive-prefixed and backslash paths are accepted while preserving the
existing containment, existence, and artifact checks.
---
Nitpick comments:
In `@src/bmm-skills/ship/bmad-build-auto/customize.toml`:
- Around line 115-183: Add a clarification comment adjacent to the review-layer
instructions in workflow.review_layers explaining that {skill-root} resolves to
the rendered snapshot directory and override paths must target only .md files
shipped within the skill directory. Keep the existing review-layer behavior and
instruction strings unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b2f4b9a-5795-4968-86e1-06ae7120eaae
📒 Files selected for processing (19)
docs/how-to/quick-fixes.mddocs/reference/build-auto.mdpackage.jsonsrc/bmm-skills/ship/bmad-build-auto/customize.tomlsrc/bmm-skills/ship/bmad-build-auto/step-04-review.mdsrc/bmm-skills/ship/bmad-build/SKILL.mdsrc/bmm-skills/ship/bmad-build/customize.tomlsrc/bmm-skills/ship/bmad-build/step-01-clarify-and-route.mdsrc/bmm-skills/ship/bmad-build/step-02-plan.mdsrc/bmm-skills/ship/bmad-build/step-03-implement.mdsrc/bmm-skills/ship/bmad-build/step-04-review.mdsrc/bmm-skills/ship/bmad-build/step-05-present.mdsrc/bmm-skills/ship/bmad-build/step-oneshot.mdsrc/bmm-skills/ship/bmad-build/sync-sprint-status.mdsrc/bmm-skills/ship/bmad-build/workflow.mdtest/test-build-auto-contract.jstest/test-build-auto-renderer.jstest/test-build-renderer.jstest/test-installation-components.js
💤 Files with no reviewable changes (1)
- test/test-build-renderer.js
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- docs/how-to/quick-fixes.md
- docs/reference/build-auto.md
- test/test-build-auto-contract.js
| const renderRoot = path.join(fs.realpathSync(build.project), '_bmad', 'render'); | ||
| const referenced = new Set(); | ||
| for (const match of markdown.matchAll(/`(\/[^`]+\.md)`/g)) { | ||
| const target = match[1]; | ||
| if (!target.startsWith(`${renderRoot}${path.sep}`)) continue; | ||
| assert(target.startsWith(`${dir}${path.sep}`), `cross-generation reference: ${target}`); | ||
| assert(fs.existsSync(target), `snapshot reference does not resolve: ${target}`); | ||
| referenced.add(path.relative(dir, target)); | ||
| } | ||
| // Every published step must be reachable, which also keeps the loop above non-vacuous. | ||
| for (const name of Object.keys(bytesByName(dir))) { | ||
| if (!/^(?:step-|sync-sprint-status)/.test(name)) continue; | ||
| assert(referenced.has(name), `published step is unreachable from the snapshot: ${name}`); | ||
| } | ||
|
|
||
| const prompt = path.join(dir, 'review-prompts', 'edge-case-hunter.md'); | ||
| assert(fs.existsSync(prompt), 'review prompt was not published into the snapshot'); | ||
| assert(markdown.includes(prompt), 'snapshot reviewer path missing'); | ||
|
|
||
| const review = fs.readFileSync(path.join(dir, 'step-04-review.md'), 'utf8'); | ||
| for (const heading of [ | ||
| '#### Blind Hunter (`blind-hunter`)', | ||
| '#### Edge Case Hunter (`edge-case-hunter`)', | ||
| '#### Verification Gap Reviewer (`verification-gap`)', | ||
| ]) { | ||
| assert(review.includes(heading), `default review layer missing: ${heading}`); | ||
| } | ||
| assert(review.includes('{diff_output}'), 'runtime placeholder was removed from review layers'); | ||
|
|
||
| const oneshot = fs.readFileSync(path.join(dir, 'step-oneshot.md'), 'utf8'); | ||
| assert(oneshot.includes('#### Blind Hunter (`blind-hunter`)'), 'oneshot review layer block missing'); | ||
|
|
||
| // The spec editor handoff must reach both terminal routes (#2652). | ||
| const present = fs.readFileSync(path.join(dir, 'step-05-present.md'), 'utf8'); | ||
| assert(present.includes('code -r'), 'open_spec default missing from step-05-present.md'); | ||
| assert(oneshot.includes('code -r'), 'open_spec default missing from step-oneshot.md'); | ||
| assert(/^Offer to push\b/m.test(present), 'standalone "Offer to push" line was lost'); | ||
|
|
||
| const artifacts = `${fs.realpathSync(build.project)}/implementation`; | ||
| assert(markdown.includes(`${artifacts}/sprint-status.yaml`), 'sprint-status path was not baked absolute'); | ||
| assert(markdown.includes(`${artifacts}/deferred-work.md`), 'deferred-work path was not baked absolute'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use platform-native paths in the bmad-build snapshot assertions.
The reference regex only matches paths that start with /. Windows snapshot paths use a drive prefix and backslashes. The artifact assertions also force / after the project root. These assertions fail on Windows even when the renderer is correct.
Proposed fix
- for (const match of markdown.matchAll(/`(\/[^`]+\.md)`/g)) {
+ for (const match of markdown.matchAll(/`([^`]+\.md)`/g)) {
const target = match[1];
- if (!target.startsWith(`${renderRoot}${path.sep}`)) continue;
+ if (!path.isAbsolute(target) || !target.startsWith(`${renderRoot}${path.sep}`)) continue;
assert(target.startsWith(`${dir}${path.sep}`), `cross-generation reference: ${target}`);
assert(fs.existsSync(target), `snapshot reference does not resolve: ${target}`);
referenced.add(path.relative(dir, target));
}
@@
- const artifacts = `${fs.realpathSync(build.project)}/implementation`;
- assert(markdown.includes(`${artifacts}/sprint-status.yaml`), 'sprint-status path was not baked absolute');
- assert(markdown.includes(`${artifacts}/deferred-work.md`), 'deferred-work path was not baked absolute');
+ const artifacts = path.join(fs.realpathSync(build.project), 'implementation');
+ assert(markdown.includes(path.join(artifacts, 'sprint-status.yaml')), 'sprint-status path was not baked absolute');
+ assert(markdown.includes(path.join(artifacts, 'deferred-work.md')), 'deferred-work path was not baked absolute');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const renderRoot = path.join(fs.realpathSync(build.project), '_bmad', 'render'); | |
| const referenced = new Set(); | |
| for (const match of markdown.matchAll(/`(\/[^`]+\.md)`/g)) { | |
| const target = match[1]; | |
| if (!target.startsWith(`${renderRoot}${path.sep}`)) continue; | |
| assert(target.startsWith(`${dir}${path.sep}`), `cross-generation reference: ${target}`); | |
| assert(fs.existsSync(target), `snapshot reference does not resolve: ${target}`); | |
| referenced.add(path.relative(dir, target)); | |
| } | |
| // Every published step must be reachable, which also keeps the loop above non-vacuous. | |
| for (const name of Object.keys(bytesByName(dir))) { | |
| if (!/^(?:step-|sync-sprint-status)/.test(name)) continue; | |
| assert(referenced.has(name), `published step is unreachable from the snapshot: ${name}`); | |
| } | |
| const prompt = path.join(dir, 'review-prompts', 'edge-case-hunter.md'); | |
| assert(fs.existsSync(prompt), 'review prompt was not published into the snapshot'); | |
| assert(markdown.includes(prompt), 'snapshot reviewer path missing'); | |
| const review = fs.readFileSync(path.join(dir, 'step-04-review.md'), 'utf8'); | |
| for (const heading of [ | |
| '#### Blind Hunter (`blind-hunter`)', | |
| '#### Edge Case Hunter (`edge-case-hunter`)', | |
| '#### Verification Gap Reviewer (`verification-gap`)', | |
| ]) { | |
| assert(review.includes(heading), `default review layer missing: ${heading}`); | |
| } | |
| assert(review.includes('{diff_output}'), 'runtime placeholder was removed from review layers'); | |
| const oneshot = fs.readFileSync(path.join(dir, 'step-oneshot.md'), 'utf8'); | |
| assert(oneshot.includes('#### Blind Hunter (`blind-hunter`)'), 'oneshot review layer block missing'); | |
| // The spec editor handoff must reach both terminal routes (#2652). | |
| const present = fs.readFileSync(path.join(dir, 'step-05-present.md'), 'utf8'); | |
| assert(present.includes('code -r'), 'open_spec default missing from step-05-present.md'); | |
| assert(oneshot.includes('code -r'), 'open_spec default missing from step-oneshot.md'); | |
| assert(/^Offer to push\b/m.test(present), 'standalone "Offer to push" line was lost'); | |
| const artifacts = `${fs.realpathSync(build.project)}/implementation`; | |
| assert(markdown.includes(`${artifacts}/sprint-status.yaml`), 'sprint-status path was not baked absolute'); | |
| assert(markdown.includes(`${artifacts}/deferred-work.md`), 'deferred-work path was not baked absolute'); | |
| const renderRoot = path.join(fs.realpathSync(build.project), '_bmad', 'render'); | |
| const referenced = new Set(); | |
| for (const match of markdown.matchAll(/`([^`]+\.md)`/g)) { | |
| const target = match[1]; | |
| if (!path.isAbsolute(target) || !target.startsWith(`${renderRoot}${path.sep}`)) continue; | |
| assert(target.startsWith(`${dir}${path.sep}`), `cross-generation reference: ${target}`); | |
| assert(fs.existsSync(target), `snapshot reference does not resolve: ${target}`); | |
| referenced.add(path.relative(dir, target)); | |
| } | |
| // Every published step must be reachable, which also keeps the loop above non-vacuous. | |
| for (const name of Object.keys(bytesByName(dir))) { | |
| if (!/^(?:step-|sync-sprint-status)/.test(name)) continue; | |
| assert(referenced.has(name), `published step is unreachable from the snapshot: ${name}`); | |
| } | |
| const prompt = path.join(dir, 'review-prompts', 'edge-case-hunter.md'); | |
| assert(fs.existsSync(prompt), 'review prompt was not published into the snapshot'); | |
| assert(markdown.includes(prompt), 'snapshot reviewer path missing'); | |
| const review = fs.readFileSync(path.join(dir, 'step-04-review.md'), 'utf8'); | |
| for (const heading of [ | |
| '#### Blind Hunter (`blind-hunter`)', | |
| '#### Edge Case Hunter (`edge-case-hunter`)', | |
| '#### Verification Gap Reviewer (`verification-gap`)', | |
| ]) { | |
| assert(review.includes(heading), `default review layer missing: ${heading}`); | |
| } | |
| assert(review.includes('{diff_output}'), 'runtime placeholder was removed from review layers'); | |
| const oneshot = fs.readFileSync(path.join(dir, 'step-oneshot.md'), 'utf8'); | |
| assert(oneshot.includes('#### Blind Hunter (`blind-hunter`)'), 'oneshot review layer block missing'); | |
| // The spec editor handoff must reach both terminal routes (`#2652`). | |
| const present = fs.readFileSync(path.join(dir, 'step-05-present.md'), 'utf8'); | |
| assert(present.includes('code -r'), 'open_spec default missing from step-05-present.md'); | |
| assert(oneshot.includes('code -r'), 'open_spec default missing from step-oneshot.md'); | |
| assert(/^Offer to push\b/m.test(present), 'standalone "Offer to push" line was lost'); | |
| const artifacts = path.join(fs.realpathSync(build.project), 'implementation'); | |
| assert(markdown.includes(path.join(artifacts, 'sprint-status.yaml')), 'sprint-status path was not baked absolute'); | |
| assert(markdown.includes(path.join(artifacts, 'deferred-work.md')), 'deferred-work path was not baked absolute'); |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 508-508: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(dir, 'step-04-review.md'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 518-518: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(dir, 'step-oneshot.md'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 522-522: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(dir, 'step-05-present.md'), 'utf8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
🤖 Prompt for 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.
In `@test/test-build-auto-renderer.js` around lines 490 - 530, Update the snapshot
assertions around renderRoot, the markdown reference matching, and artifacts to
use platform-native path construction and matching instead of assuming leading
`/` or literal `/` separators. Normalize or construct expected paths
consistently with path.sep so Windows drive-prefixed and backslash paths are
accepted while preserving the existing containment, existence, and artifact
checks.
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
c7bad19 to
b05a563
Compare
There was a problem hiding this comment.
alexeyv has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
1 participant
What
bmad-build now renders through the shared snapshot renderer (
_bmad/scripts/render_skill.py) — its skill-localrender.pyis retired — and deferred-work recording in both build skills becomes a[workflow] defercustomization key.Why
Two renderers meant double maintenance, and the strictly more robust one (immutable content-addressed generations, atomic publish, strict validation) was only used by bmad-build-auto. The deferral sink was hard-wired: bmad-build to a
deferred-work.mdledger via renderer-derived variables, build-auto to spec frontmatter. Thedeferkey keeps each default but lets installs reroute deferrals (another file, SQLite, Jira) without editing step files.How
[[bmad-snapshot:]]tokensdeferto both skills'customize.toml— call sites keep producing entries, the key owns where they are recordedtest/test-build-renderer.js; the shared suite is parameterized per skill (29 tests), the contract test pins the relocated default, Suite 49 covers the installed bmad-build surfaceTesting
Full
npm run qualitygreen at every commit; adversarial/edge-case/verification-gap/intent-alignment review each commit with all patches applied; key assertions mutation-tested.🤖 Generated with Claude Code