diff --git a/src/bmm-skills/4-implementation/bmad-quick-dev/render.py b/src/bmm-skills/4-implementation/bmad-quick-dev/render.py index b5faed752..267ae8ef9 100644 --- a/src/bmm-skills/4-implementation/bmad-quick-dev/render.py +++ b/src/bmm-skills/4-implementation/bmad-quick-dev/render.py @@ -215,12 +215,59 @@ def _scalar_str(value): return str(value) -def _render_workflow_value(value): - """Format a resolved [workflow] value for inline substitution. Lists render - as markdown bullets (empty -> '_None._'); scalars render verbatim. Each list - item uses the same scalar formatting so booleans stay consistent. Entries are - emitted as-is so runtime placeholders like {project-root} survive for the LLM - to resolve.""" +# [workflow] keys holding review layers ([[workflow.review_layers]] tables with +# id/name/instruction/when fields). This renderer knows this skill's +# customization schema outright — layer semantics are materialized here, not +# interpreted by the LLM at run time. +_REVIEW_LAYER_KEYS = ("review_layers", "oneshot_review_layers") + + +def _render_review_layers(layers): + """Materialize review layers into direct invocation blocks. A layer with an + empty or missing instruction is disabled (that is how an override turns off + a default layer) and drops out entirely. A `when` condition is the one part + that stays with the LLM: it renders as a run-time guard line. No active + layers renders as the HALT instruction the workflow would otherwise have to + derive from an empty list.""" + active = [ + layer + for layer in layers + if isinstance(layer, dict) and _scalar_str(layer.get("instruction")).strip() + ] + if not active: + return ( + "No review layers are active. HALT with status `blocked` and " + "blocking condition `no active review layers`." + ) + blocks = [] + for layer in active: + title = ( + _scalar_str(layer.get("name")).strip() + or _scalar_str(layer.get("id")).strip() + or "Review layer" + ) + lines = [f"#### {title}", ""] + when = _scalar_str(layer.get("when")).strip() + if when: + lines.append( + "Run this layer only if the following holds in the " + f"current context: `{when}`" + ) + lines.append("") + lines.append(_scalar_str(layer.get("instruction")).strip("\n")) + blocks.append("\n".join(lines)) + return "\n\n".join(blocks) + + +def _render_workflow_value(key, value): + """Format a resolved [workflow] value for inline substitution. Review-layer + keys materialize as invocation blocks; other lists render as markdown + bullets (empty -> '_None._'); scalars render verbatim. Each list item uses + the same scalar formatting so booleans stay consistent. Entries are emitted + as-is so runtime placeholders like {project-root} or {diff_output} survive + for the LLM to resolve.""" + if key in _REVIEW_LAYER_KEYS and isinstance(value, list): + return _render_review_layers(value) if isinstance(value, list): if not value: return "_None._" @@ -235,7 +282,7 @@ def render_workflow(content, workflow): elsewhere are untouched.""" return re.sub( r"\{workflow\.(\w+)\}", - lambda m: _render_workflow_value(workflow.get(m.group(1))), + lambda m: _render_workflow_value(m.group(1), workflow.get(m.group(1))), content, ) diff --git a/src/bmm-skills/4-implementation/bmad-quick-dev/step-04-review.md b/src/bmm-skills/4-implementation/bmad-quick-dev/step-04-review.md index c49ae8c3c..3a735cd83 100644 --- a/src/bmm-skills/4-implementation/bmad-quick-dev/step-04-review.md +++ b/src/bmm-skills/4-implementation/bmad-quick-dev/step-04-review.md @@ -18,11 +18,9 @@ Do NOT `git add` anything — this is read-only inspection. ### Review -The review layers are `{workflow.review_layers}`, resolved during activation. +Execute these review layers in parallel wherever their execution methods allow: substitute the runtime placeholders (e.g. `{diff_output}`) into each layer's instruction, then follow it verbatim. -Skip every layer whose `instruction` is empty or missing — that is how an override disables a default layer — and every layer whose `when` condition (if present) does not hold in the current context. If no layers remain, HALT with status `blocked` and blocking condition `no active review layers`. - -Execute all remaining layers in parallel wherever their execution methods allow: substitute the runtime placeholders (e.g. `{diff_output}`) into each layer's `instruction`, then follow it verbatim. +{workflow.review_layers} If a layer's instruction requires subagents and none are available, generate one review prompt file per such layer in `{{.implementation_artifacts}}` and HALT. Ask the human to run each in a separate session (ideally a different LLM) and paste back the findings. diff --git a/src/bmm-skills/4-implementation/bmad-quick-dev/step-oneshot.md b/src/bmm-skills/4-implementation/bmad-quick-dev/step-oneshot.md index 21bbc1bdd..eb078796c 100644 --- a/src/bmm-skills/4-implementation/bmad-quick-dev/step-oneshot.md +++ b/src/bmm-skills/4-implementation/bmad-quick-dev/step-oneshot.md @@ -17,9 +17,9 @@ Implement the clarified intent directly. ### Review -The review layers for this route are `{workflow.oneshot_review_layers}`, resolved during activation. +Execute these review layers in parallel wherever their execution methods allow, following each layer's instruction verbatim after substituting any runtime placeholders: -Skip every layer whose `instruction` is empty or missing, and every layer whose `when` condition (if present) does not hold. If no layers remain, HALT with status `blocked` and blocking condition `no active review layers`. Execute all remaining layers in parallel wherever their execution methods allow, following each layer's `instruction` verbatim after substituting any runtime placeholders. +{workflow.oneshot_review_layers} If a layer's instruction requires subagents and none are available, generate one review prompt file per such layer in `{{.implementation_artifacts}}` and HALT. Ask the human to run each in a separate session and paste back the findings. diff --git a/test/test-quick-dev-renderer.js b/test/test-quick-dev-renderer.js index 22746fc70..89452ec90 100644 --- a/test/test-quick-dev-renderer.js +++ b/test/test-quick-dev-renderer.js @@ -9,7 +9,12 @@ * 3. [workflow] customization is self-resolved and inlined: prepend bullet, * persistent_facts append (base kept), empty list -> _None._, on_complete * scalar baked into step-05/step-oneshot. - * 4. No {workflow.*} placeholder or resolve_customization.py call survives + * 4. Review layers materialize as direct invocation blocks: default layers + * become #### sections in step-04, an override replacing a layer by id + * wins, an empty-instruction override drops its layer, a `when` renders + * as a run-time guard, runtime placeholders like {diff_output} survive, + * and disabling every layer renders the HALT instruction. + * 5. No {workflow.*} placeholder or resolve_customization.py call survives * in any rendered file. * * Usage: node test/test-quick-dev-renderer.js @@ -114,6 +119,16 @@ try { 'activation_steps_prepend = ["TEST_PREPEND_STEP"]', 'persistent_facts = ["TEST_EXTRA_FACT"]', 'on_complete = "TEST_ON_COMPLETE_INSTRUCTION"', + '', + '[[workflow.review_layers]]', + 'id = "edge-case-hunter"', + 'name = "Replaced Layer"', + 'when = "TEST_WHEN_CONDITION"', + 'instruction = "TEST_REPLACED_LAYER_INSTRUCTION"', + '', + '[[workflow.review_layers]]', + 'id = "verification-gap"', + 'instruction = ""', ].join('\n'), 'utf-8', ); @@ -195,6 +210,72 @@ try { } }); + test('review layers materialize as invocation blocks in step-04', () => { + const content = readRendered('step-04-review.md'); + assert(content.includes('#### Blind Hunter'), 'default review layer not rendered as a #### invocation block'); + assert(!content.includes('- id:'), 'layer table data leaked into the rendered output'); + assert(content.includes('{diff_output}'), 'runtime {diff_output} placeholder did not survive rendering'); + }); + + test('review layer override replaces the matching default by id', () => { + const content = readRendered('step-04-review.md'); + assert(content.includes('#### Replaced Layer'), 'override layer name not used as block title'); + assert(content.includes('TEST_REPLACED_LAYER_INSTRUCTION'), 'override layer instruction not inlined'); + assert(!content.includes('bmad-review-edge-case-hunter'), 'replaced default layer instruction still present'); + assert(content.includes('bmad-review-adversarial-general'), 'untouched default layer dropped by keyed merge'); + }); + + test('empty-instruction override drops its layer entirely', () => { + const content = readRendered('step-04-review.md'); + assert(!content.includes('verification-gap'), 'disabled layer id still present in rendered output'); + assert(!content.includes('Verification Gap Reviewer'), 'disabled layer name still present in rendered output'); + }); + + test('when condition renders as a run-time guard line', () => { + const content = readRendered('step-04-review.md'); + assert( + content.includes('Run this layer only if the following holds in the current context: `TEST_WHEN_CONDITION`'), + 'when condition not rendered as a guard line', + ); + }); + + test('disabling every layer renders the HALT instruction', () => { + // Second render pass: replace the override file so every default layer + // (and the oneshot route's only layer) is disabled, then re-render. + fs.writeFileSync( + path.join(tmpDir, '_bmad', 'custom', 'bmad-quick-dev.user.toml'), + [ + '[workflow]', + '', + '[[workflow.review_layers]]', + 'id = "blind-hunter"', + 'instruction = ""', + '', + '[[workflow.review_layers]]', + 'id = "edge-case-hunter"', + 'instruction = ""', + '', + '[[workflow.review_layers]]', + 'id = "verification-gap"', + 'instruction = ""', + '', + '[[workflow.oneshot_review_layers]]', + 'id = "blind-hunter"', + 'instruction = ""', + ].join('\n'), + 'utf-8', + ); + const rerun = spawnSync('python3', [path.join(skillDst, 'render.py')], { + cwd: skillDst, + encoding: 'utf-8', + }); + assert(rerun.status === 0, `re-render exit code ${rerun.status}\nstderr: ${rerun.stderr}`); + const halt = 'No review layers are active. HALT with status `blocked` and blocking condition `no active review layers`.'; + for (const file of ['step-04-review.md', 'step-oneshot.md']) { + assert(readRendered(file).includes(halt), `HALT instruction missing from ${file}`); + } + }); + test('no {workflow.*} placeholder survives in any rendered file', () => { const leaks = renderedMdFiles().filter((f) => readRendered(f).includes('{workflow.')); assert(leaks.length === 0, `{workflow.*} leaked in: ${leaks.join(', ')}`);