mirror of
https://github.com/bmad-code-org/BMAD-METHOD.git
synced 2026-08-30 17:14:59 +08:00
feat(quick-dev): materialize review layers into invocation blocks
Reconcile #2550 with render-time [workflow] resolution. Main made review layers configurable as [[workflow.review_layers]] arrays of tables and had the LLM resolve them during activation; this branch resolves the [workflow] block in render.py instead, so activation-time resolution no longer exists and the layer refs must be materialized at render time. Rather than inlining the layer tables as data plus interpretation rules, render.py now knows this skill's customization schema outright and renders review_layers/oneshot_review_layers as direct invocation blocks: disabled layers (empty instruction) drop out, each active layer becomes a #### section holding its instruction verbatim, zero active layers renders the HALT instruction, and runtime placeholders like {diff_output} pass through. The only judgment left to the LLM is the optional `when` condition, which renders as a run-time guard line. The step-04/step-oneshot review intros collapse to a single execute-in- parallel imperative. Smoke test covers default rendering, replace-by-id, disable-by-empty-instruction, when-guards, and the all-disabled HALT.
This commit is contained in:
Alex Verkhovsky
@@ -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,
|
||||
)
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
|
||||
@@ -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(', ')}`);
|
||||
|
||||
Reference in New Issue
Block a user