refactor(quick-flow): separate one-shot into self-contained step file#1899
refactor(quick-flow): separate one-shot into self-contained step file#1899alexeyv wants to merge 1 commit intobmad-code-org:mainfrom
Conversation
One-shot and plan-code-review shared steps 3-5 which depend on spec files one-shot never creates. Give one-shot its own step-oneshot.md with implement/review/classify/commit/HITL. Clean all one-shot refs from steps 3-4. Restructure step-01 as explicit self-contained branches with no dead variables or indirect routing.
🤖 Augment PR SummarySummary: Refactors the Quick Dev New Preview workflow to make the “one-shot” execution path self-contained and simplify plan-code-review steps. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| - Active specs (`ready-for-dev`, `in-progress`, `in-review`) in `{implementation_artifacts}`? → List them and HALT. Ask user which to resume (or `[N]` for new). | ||
| - If `ready-for-dev` or `in-progress` selected: Set `spec_file`, set `execution_mode = "plan-code-review"`, skip to step 3. | ||
| - If `in-review` selected: Set `spec_file`, set `execution_mode = "plan-code-review"`, skip to step 4. | ||
| - If `ready-for-dev` or `in-progress` selected: Set `spec_file`. **EARLY EXIT** → `{installed_path}/steps/step-03-implement.md` |
There was a problem hiding this comment.
{installed_path} is referenced for EARLY EXIT targets, but this workflow’s workflow.md appears to use relative ./steps/... paths and doesn’t define installed_path, so these links may not resolve at runtime. Consider ensuring installed_path is always set for this workflow (or using a consistent linking scheme) to avoid breaking routing.
Severity: medium
Other Locations
src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-01-clarify-and-route.md:26src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-01-clarify-and-route.md:45src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-01-clarify-and-route.md:53
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
📝 WalkthroughWalkthroughThe PR modifies workflow specification steps for the bmad-quick-flow to introduce EARLY EXIT semantics for conditional routing, unify one-shot and plan-code-review execution paths, add an Acceptance auditor role to review processes, and introduce a new self-contained one-shot workflow step with dedicated implementation, review, and classification stages. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClarifyRoute as Clarify & Route (step-01)
participant OneShot as One-Shot (step-oneshot)
participant PlanReview as Plan-Code-Review Flow
participant Implement as Implement (step-03)
participant Review as Review (step-04)
participant Classify as Classify
participant Commit as Commit
User->>ClarifyRoute: Start workflow with artifact
alt Ready-for-dev or In-Progress
ClarifyRoute->>Implement: EARLY EXIT with spec_file set
Implement->>Review: Hand to sub-agent
Review->>Classify: Analyze findings
Classify->>Commit: Route patch/defer/reject
Commit->>User: Complete (if VCS dirty)
else In-Review or One-Shot Route
ClarifyRoute->>OneShot: EARLY EXIT to one-shot workflow
OneShot->>OneShot: Implement locally
OneShot->>OneShot: Review via adversarial task
OneShot->>OneShot: Classify findings
OneShot->>OneShot: Commit if VCS available
OneShot->>User: CHECKPOINT & summary
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-04-review.md (3)
53-53:⚠️ Potential issue | 🔴 CriticalBroken relative path in NEXT section.
Line 53 references
./steps/step-05-present.mdbut this file is already in thesteps/directory. The path should be./step-05-present.md(nosteps/prefix).🔧 Proposed fix
-Read fully and follow `./steps/step-05-present.md` +Read fully and follow `./step-05-present.md`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-04-review.md` at line 53, Fix the broken relative path in the NEXT instruction inside step-04-review.md by replacing the incorrect string "./steps/step-05-present.md" with the correct relative path "./step-05-present.md" so the link resolves from within the steps/ directory; update the occurrence in the text that currently reads "Read fully and follow `./steps/step-05-present.md`" to use "Read fully and follow `./step-05-present.md`".
32-32:⚠️ Potential issue | 🟠 MajorMissing error handling: spec_file context field might not exist.
Line 32 says the Acceptance auditor "Must also read the docs listed in
{spec_file}frontmattercontext." What if the spec_file has nocontextfield? What if it's empty? The instruction provides no error handling or fallback. The auditor subagent will either crash or skip context docs silently, producing incomplete review.🛡️ Proposed fix
-- **Acceptance auditor** — receives `{diff_output}`, `{spec_file}`, and read access to the project. Must also read the docs listed in `{spec_file}` frontmatter `context`. Checks for violations of acceptance criteria, rules, and principles from the spec and context docs. +- **Acceptance auditor** — receives `{diff_output}`, `{spec_file}`, and read access to the project. Read the `{spec_file}` frontmatter `context` field if it exists; if missing or empty, proceed with spec and project access only. Checks for violations of acceptance criteria, rules, and principles from the spec and context docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-04-review.md` at line 32, The Acceptance auditor step currently assumes `{spec_file}` frontmatter contains a `context` field; add defensive handling in the Acceptance auditor logic to check whether `{spec_file}.frontmatter.context` exists and is a non-empty list/string, and if not, log a warning and proceed with an empty context (or a defined default) rather than crashing or silently skipping; update any references to "Must also read the docs listed in `{spec_file}` frontmatter `context`" to describe this behavior and ensure the auditor uses a safe accessor when loading context docs (e.g., validate `context` before iterating/reading).
32-32:⚠️ Potential issue | 🟡 MinorUndisclosed addition: Acceptance auditor role not in PR description.
Line 32 introduces a third reviewer role—"Acceptance auditor"—that does not appear in the PR description or AI summary. The PR summary mentions "Adversarial review" generically and the AI summary says "adds an Acceptance auditor role to review processes," but this is buried as a side note. This is a significant workflow change (expanding from 2 to 3 reviewers) that should have been highlighted in the PR objectives and test plan.
The AI summary does mention the acceptance auditor but the PR description does not. This addition changes the review SLA (3 subagents instead of 2) and introduces a new dependency on spec context docs. It should be called out explicitly in the test plan: "Verify that acceptance auditor receives spec_file and context docs correctly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-04-review.md` at line 32, The PR adds a new reviewer role "Acceptance auditor" in step-04-review.md but the PR description, AI summary, and test plan do not explicitly document this workflow change; update the PR description and AI summary to explicitly state the addition of the "Acceptance auditor" role and the impact on review SLA (3 reviewers instead of 2), and update the test plan to include a test case verifying the acceptance auditor receives `{diff_output}`, `{spec_file}`, and the frontmatter `context` docs (e.g., add "Verify that Acceptance auditor receives spec_file and context docs correctly" to the test plan), and ensure any references to "Adversarial review" are clarified to list all three roles including "Acceptance auditor."
🧹 Nitpick comments (2)
src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-03-implement.md (1)
21-23: Qualifier removed but still accurate: "Baseline" is plan-code-review only.The heading was changed from "Baseline (plan-code-review only)" to "Baseline" (line 21). However, the qualifier was accurate—this step IS plan-code-review only now (one-shot has been extracted to step-oneshot.md). Removing the qualifier doesn't change the fact, and the instruction on line 23 still says "Capture
baseline_commit" which is only relevant for plan-code-review (per learnings, one-shot relies on session knowledge, not baseline_commit).This is a stylistic question: should headings signal their scope, or should the context (step file name, step description) be sufficient? If the workflow maintainers prefer explicit scope markers, restore the qualifier. Otherwise, this is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-03-implement.md` around lines 21 - 23, The "Baseline" heading lost its qualifier but the step is still plan-code-review only; restore the qualifier in the heading text so it reads "Baseline (plan-code-review only)" to make scope explicit, and ensure the description under that heading (the line mentioning Capture `baseline_commit`) remains unchanged; update the heading string in step-03-implement.md where the heading currently reads "Baseline" so reviewers clearly see this step applies to plan-code-review only.src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-01-clarify-and-route.md (1)
25-26: Inconsistent resume paths: step-03 and step-04 are now plan-code-review only.Lines 25-26 route to step-03-implement.md for
ready-for-devorin-progressspecs, and step-04-review.md forin-reviewspecs. But per the PR changes, step-03 and step-04 are now plan-code-review only (one-shot has been extracted). This is correct for plan-code-review specs but creates a conceptual mismatch: if a spec exists with status, it was created via plan-code-review, so routing to step-03/04 is valid. However, the comment should clarify this to avoid confusion when reading the ARTIFACT SCAN in isolation.📝 Optional clarification
- Active specs (`ready-for-dev`, `in-progress`, `in-review`) in `{implementation_artifacts}`? → List them and HALT. Ask user which to resume (or `[N]` for new). - - If `ready-for-dev` or `in-progress` selected: Set `spec_file`. **EARLY EXIT** → `{installed_path}/steps/step-03-implement.md` - - If `in-review` selected: Set `spec_file`. **EARLY EXIT** → `{installed_path}/steps/step-04-review.md` + - If `ready-for-dev` or `in-progress` selected: Set `spec_file`. **EARLY EXIT** → `{installed_path}/steps/step-03-implement.md` (plan-code-review path) + - If `in-review` selected: Set `spec_file`. **EARLY EXIT** → `{installed_path}/steps/step-04-review.md` (plan-code-review path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-01-clarify-and-route.md` around lines 25 - 26, Summary: Clarify routing comment to explain why `ready-for-dev`/`in-progress` go to step-03-implement.md and `in-review` goes to step-04-review.md even though those steps are now plan-code-review-only. Update the text in step-01-clarify-and-route.md to state that specs with statuses `ready-for-dev`, `in-progress`, or `in-review` must have been created via the plan-code-review flow, so routing to step-03-implement.md and step-04-review.md is intentional; explicitly note these are early-exit paths for plan-code-review-created artifacts to avoid confusion when reading the ARTIFACT SCAN in isolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-01-clarify-and-route.md`:
- Around line 25-26: The workflow is missing the installed_path path variable
referenced by step-01-clarify-and-route.md; open the workflow.md for the
bmad-quick-dev-new-preview flow and in the Paths section add an installed_path
variable named installed_path that points to the flow directory (the same
pattern used in other workflows) so that templates like
`{installed_path}/steps/step-03-implement.md` and
`{installed_path}/steps/step-04-review.md` resolve correctly; ensure the
variable name is exactly installed_path and its value matches the flow's folder
path.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-03-implement.md`:
- Line 3: Update the frontmatter "description" string so it doesn't claim the
review is always done "via sub-agent"; either make it neutral or mention both
modes (e.g., "Plan-code-review implementation via sub-agent or fallback to
direct implementation") or similar. Locate the description field (the line
currently: description: 'Plan-code-review implementation via sub-agent. Local
only.') and align it with the fallback sentence "If no sub-agents are available,
implement directly." so the description accurately reflects both modes.
- Line 29: The instruction "Hand `{spec_file}` to a sub-agent/task and let it
implement" is too vague; update step-03-implement.md to either declare a named
implementation task and its contract in the frontmatter (e.g., add a field like
implementation_task: ImplementFromSpec and define the contract: inputs:
spec_file path/string, outputs: changed_files list and patch/diff) and replace
that sentence with an explicit handoff call (invoke ImplementFromSpec with
{spec_file} and expect a manifest of changed_files and a completion status), or
explicitly state that implementation will be done in-process (no subagent) and
describe the outputs (changed_files list for step-04). Ensure references to
{spec_file}, ImplementFromSpec (or chosen task name), changed_files, and step-04
diff construction are included.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-04-review.md`:
- Line 22: The current fallback "use best effort to determine what changed" for
reading `{baseline_commit}` from `{spec_file}` when `{baseline_commit}` is
missing or set to `NO_VCS` is underspecified and fragile; change the behavior so
that when `{baseline_commit}` is absent or `NO_VCS` the step halts with a clear
error asking the pipeline to provide a baseline or require step-03 to record
changed files into `{spec_file}` frontmatter as a fallback, and ensure
`{diff_output}` is only constructed from a trusted source (either a valid
`{baseline_commit}` or the explicit changed-files list written by step-03),
updating any code that reads `{spec_file}` and the logic around generating
`{diff_output}` to enforce this invariant.
- Line 43: The current rule that any single "intent_gap" or "bad_spec" finding
triggers a full loopback is too aggressive; update the process to first
deduplicate and consolidate findings, then evaluate whether the issue is
systemic by applying a threshold or severity weighting (e.g., require >=2
distinct "intent_gap" or "bad_spec" findings or at least one high-severity
finding) before triggering the loopback and incrementing {specLoopIteration};
ensure consolidated findings are used when deciding to process a patch normally
versus escalate after {specLoopIteration} exceeds 5.
- Line 49: Update the guidance for the step that currently reads "If version
control is available and the tree is dirty, commit." to specify a required
commit message format (e.g., Conventional Commits + include spec/PR reference
and step id), pre-commit failure handling (run linters/tests, capture hook
errors, surface them to the user and abort the automated flow), remote-conflict
checks (git fetch and ensure local branch is up-to-date or prompt to
rebase/merge before committing), and failure recovery instructions (how to
stash/reset changes or revert the commit and notify the operator). Reference the
exact sentence "If version control is available and the tree is dirty, commit."
and add short examples of expected commit message structure and concise steps
for handling/preventing hook or merge failures.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-oneshot.md`:
- Line 14: The document contains a contradiction between the sentence "No push.
No remote ops." and the later phrase "Offer to push and/or create a pull
request."; decide which behavior is correct and make the text consistent: either
remove or reword the later offer ("Offer to push and/or create a pull request.")
to match the no-remote policy, or change the initial statement ("No push. No
remote ops.") to allow optional pushes (e.g., "No push by default; offer
optional push/PR") and update any related instructions or examples in
step-oneshot.md so both lines convey the same allowed remote behavior.
- Line 24: Add a deterministic, collision-resistant naming pattern for the
review prompt file written to {implementation_artifacts} when no subagents are
available: include unique identifiers such as the workflow run id or a generated
UUID, a UTC timestamp, the task name ({adversarial_review_task}) and a clear
extension like .review_prompt (e.g.,
{workflowRunId}_{adversarial_review_task}_{timestamp}.review_prompt); ensure any
code that writes or reads these files uses the same composition logic and
document the pattern so humans can find and correlate files from concurrent or
past one-shot sessions.
- Line 36: Update the one-shot step text to explicitly require Conventional
Commits format "type(scope): subject" and define how to derive each piece:
determine "type" via a small mapping from intent keywords (e.g.,
"add"/"create"→feat, "fix"/"repair"→fix, "docs"→docs,
"refactor"/"cleanup"→refactor, otherwise chore); set "scope" to the target
component if available in session memory (or omit it if not); build the
"subject" as a concise imperative summary derived from the session memory intent
summary (fallback to a short autogenerated description), and if session memory
is absent do not create a commit; update the sentence that currently says
"conventional message derived from the intent" to include this exact format and
mapping so implementers know where to get the intent (session memory) and how to
form the commit message.
- Line 31: The defer classification references a variable deferred_work_file but
never specifies an action to append findings; update the INSTRUCTIONS section
and the "defer" line so agents explicitly perform the append: change the "Append
to `{deferred_work_file}`" note into a concrete instruction (e.g., "Write/Append
deferred finding to `{deferred_work_file}`") and ensure the step logic that
handles classifications (the defer branch in this step or any function that
processes classifications) calls the append/write operation to
deferred_work_file so the variable is actually used.
- Around line 28-32: Update the "Deduplicate all review findings" classification
in step-oneshot.md to match the five-category model used in step-04-review.md:
intent_gap, bad_spec, patch, defer, reject; keep patch = auto-fix, defer =
append to {deferred_work_file}, reject = drop, and implement intent_gap and
bad_spec as HALT-and-escalate actions (stop one-shot processing and flag/route
the finding for manual intervention/escalation) so one-shot can surface
root-cause issues rather than only auto-patching or deferring them.
- Line 5: Update the adversarial_review_task value in step-oneshot.md to point
to the new native skill location: replace the incorrect path
'{project-root}/_bmad/core/tasks/review-adversarial-general.xml' with
'src/core/tasks/bmad-review-adversarial-general/workflow.md' so the
adversarial_review_task key references the new directory and workflow.md file
(look for the adversarial_review_task line in this file and change the string
accordingly).
---
Outside diff comments:
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-04-review.md`:
- Line 53: Fix the broken relative path in the NEXT instruction inside
step-04-review.md by replacing the incorrect string "./steps/step-05-present.md"
with the correct relative path "./step-05-present.md" so the link resolves from
within the steps/ directory; update the occurrence in the text that currently
reads "Read fully and follow `./steps/step-05-present.md`" to use "Read fully
and follow `./step-05-present.md`".
- Line 32: The Acceptance auditor step currently assumes `{spec_file}`
frontmatter contains a `context` field; add defensive handling in the Acceptance
auditor logic to check whether `{spec_file}.frontmatter.context` exists and is a
non-empty list/string, and if not, log a warning and proceed with an empty
context (or a defined default) rather than crashing or silently skipping; update
any references to "Must also read the docs listed in `{spec_file}` frontmatter
`context`" to describe this behavior and ensure the auditor uses a safe accessor
when loading context docs (e.g., validate `context` before iterating/reading).
- Line 32: The PR adds a new reviewer role "Acceptance auditor" in
step-04-review.md but the PR description, AI summary, and test plan do not
explicitly document this workflow change; update the PR description and AI
summary to explicitly state the addition of the "Acceptance auditor" role and
the impact on review SLA (3 reviewers instead of 2), and update the test plan to
include a test case verifying the acceptance auditor receives `{diff_output}`,
`{spec_file}`, and the frontmatter `context` docs (e.g., add "Verify that
Acceptance auditor receives spec_file and context docs correctly" to the test
plan), and ensure any references to "Adversarial review" are clarified to list
all three roles including "Acceptance auditor."
---
Nitpick comments:
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-01-clarify-and-route.md`:
- Around line 25-26: Summary: Clarify routing comment to explain why
`ready-for-dev`/`in-progress` go to step-03-implement.md and `in-review` goes to
step-04-review.md even though those steps are now plan-code-review-only. Update
the text in step-01-clarify-and-route.md to state that specs with statuses
`ready-for-dev`, `in-progress`, or `in-review` must have been created via the
plan-code-review flow, so routing to step-03-implement.md and step-04-review.md
is intentional; explicitly note these are early-exit paths for
plan-code-review-created artifacts to avoid confusion when reading the ARTIFACT
SCAN in isolation.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-03-implement.md`:
- Around line 21-23: The "Baseline" heading lost its qualifier but the step is
still plan-code-review only; restore the qualifier in the heading text so it
reads "Baseline (plan-code-review only)" to make scope explicit, and ensure the
description under that heading (the line mentioning Capture `baseline_commit`)
remains unchanged; update the heading string in step-03-implement.md where the
heading currently reads "Baseline" so reviewers clearly see this step applies to
plan-code-review only.
🪄 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
Run ID: 3a6ef04e-d291-425d-964f-f9d9ba6cb483
📒 Files selected for processing (4)
src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-01-clarify-and-route.mdsrc/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-03-implement.mdsrc/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-04-review.mdsrc/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-oneshot.md
| - If `ready-for-dev` or `in-progress` selected: Set `spec_file`. **EARLY EXIT** → `{installed_path}/steps/step-03-implement.md` | ||
| - If `in-review` selected: Set `spec_file`. **EARLY EXIT** → `{installed_path}/steps/step-04-review.md` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that installed_path is defined in the parent workflow.md
rg -n "installed_path" src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/workflow.mdRepository: bmad-code-org/BMAD-METHOD
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Check the actual step file content
cat -n src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-01-clarify-and-route.md | head -60
# Also check if other steps in this workflow declare installed_path
echo "=== Checking all steps in this workflow ==="
for file in src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-*.md; do
echo "File: $file"
rg -c "installed_path" "$file" || echo " Not found"
doneRepository: bmad-code-org/BMAD-METHOD
Length of output: 5168
🏁 Script executed:
#!/bin/bash
# Check the workflow.md file structure
echo "=== Workflow.md content ==="
cat -n src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/workflow.md | head -100Repository: bmad-code-org/BMAD-METHOD
Length of output: 4244
🏁 Script executed:
#!/bin/bash
# Search for how installed_path might be passed to steps (look for variable passing patterns)
rg -n "installed_path" src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/Repository: bmad-code-org/BMAD-METHOD
Length of output: 824
🏁 Script executed:
#!/bin/bash
# Search for installed_path across entire codebase to understand its definition and usage
rg -n "installed_path" src/bmm/workflows/ | head -30
# Check how it's defined in similar workflows
echo "=== Quick-dev workflow (for comparison) ==="
rg -n "installed_path" src/bmm/workflows/bmad-quick-flow/quick-dev/
# Search in tools/cli for how installed_path might be injected
echo "=== Looking for installed_path in tools/cli ==="
rg -n "installed_path" tools/cli/ --type md | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 4644
🏁 Script executed:
#!/bin/bash
# Check other skill-format workflows to see if they declare installed_path
echo "=== Checking other workflows for installed_path pattern ==="
fd "workflow.md" src/bmm/workflows/ --exec rg -l "installed_path" {} \;
# Also check if installed_path might be injected as a runtime variable
echo "=== Checking manifest or generator files ==="
rg -n "installed_path" tools/cli/installers/ --type js --type md | head -20Repository: bmad-code-org/BMAD-METHOD
Length of output: 1068
Add missing installed_path variable to workflow.md frontmatter.
The step file (step-01-clarify-and-route.md) routes to other steps using {installed_path} at lines 25, 26, 45, and 53, but this variable is not declared in either the step file's frontmatter or the workflow.md frontmatter. All comparable workflows (qa-generate-e2e-tests, quick-dev, create-story, document-project, etc.) define installed_path in their Paths section. Without this variable, the routing instructions will fail to resolve.
Add to src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/workflow.md in the Paths section (after line 81):
- `installed_path` = `{project-root}/_bmad/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview`
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-01-clarify-and-route.md`
around lines 25 - 26, The workflow is missing the installed_path path variable
referenced by step-01-clarify-and-route.md; open the workflow.md for the
bmad-quick-dev-new-preview flow and in the Paths section add an installed_path
variable named installed_path that points to the flow directory (the same
pattern used in other workflows) so that templates like
`{installed_path}/steps/step-03-implement.md` and
`{installed_path}/steps/step-04-review.md` resolve correctly; ensure the
variable name is exactly installed_path and its value matches the flow's folder
path.
| --- | ||
| name: 'step-03-implement' | ||
| description: 'Execute implementation directly or via sub-agent. Local only.' | ||
| description: 'Plan-code-review implementation via sub-agent. Local only.' |
There was a problem hiding this comment.
Misleading description: "via sub-agent" is not always true.
The description says "Plan-code-review implementation via sub-agent" but line 29 provides a fallback: "If no sub-agents are available, implement directly." The description should reflect both modes or avoid claiming "via sub-agent" as the sole mechanism.
✏️ Proposed fix
-description: 'Plan-code-review implementation via sub-agent. Local only.'
+description: 'Plan-code-review implementation (sub-agent or direct). Local only.'📝 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.
| description: 'Plan-code-review implementation via sub-agent. Local only.' | |
| description: 'Plan-code-review implementation (sub-agent or direct). Local only.' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-03-implement.md`
at line 3, Update the frontmatter "description" string so it doesn't claim the
review is always done "via sub-agent"; either make it neutral or mention both
modes (e.g., "Plan-code-review implementation via sub-agent or fallback to
direct implementation") or similar. Locate the description field (the line
currently: description: 'Plan-code-review implementation via sub-agent. Local
only.') and align it with the fallback sentence "If no sub-agents are available,
implement directly." so the description accurately reflects both modes.
| `execution_mode = "one-shot"` or no sub-agents/tasks available: implement the intent. | ||
|
|
||
| Otherwise (`execution_mode = "plan-code-review"`): hand `{spec_file}` to a sub-agent/task and let it implement. | ||
| Hand `{spec_file}` to a sub-agent/task and let it implement. If no sub-agents are available, implement directly. |
There was a problem hiding this comment.
Vague subagent handoff: no task specified, no contract defined.
Line 29 says "Hand {spec_file} to a sub-agent/task and let it implement" but provides no details:
- Which task/skill should be invoked?
- What is the expected contract (inputs, outputs)?
- How does the parent agent know when implementation is complete?
- Which files were changed (needed for step-04 diff construction)?
The old one-shot mode (per learnings) relied on session knowledge of which files were modified. If implementation is delegated to a subagent, the parent loses that context unless the subagent returns a manifest of changed files.
🛠️ Recommended fix
Option 1: Declare an implementation task in frontmatter and specify the contract:
name: 'step-03-implement'
description: 'Plan-code-review implementation (sub-agent or direct). Local only.'
+implementation_task: '{project-root}/_bmad/core/tasks/bmad-implement-from-spec/workflow.md'Then in line 29:
-Hand `{spec_file}` to a sub-agent/task and let it implement. If no sub-agents are available, implement directly.
+Invoke `{implementation_task}` in a subagent with `{spec_file}` as input. The subagent must return a list of all files modified, created, or deleted. If no sub-agents are available, implement directly and track the changed files yourself.Option 2: If no standard implementation task exists, implement directly and note this as a future enhancement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-03-implement.md`
at line 29, The instruction "Hand `{spec_file}` to a sub-agent/task and let it
implement" is too vague; update step-03-implement.md to either declare a named
implementation task and its contract in the frontmatter (e.g., add a field like
implementation_task: ImplementFromSpec and define the contract: inputs:
spec_file path/string, outputs: changed_files list and patch/diff) and replace
that sentence with an explicit handoff call (invoke ImplementFromSpec with
{spec_file} and expect a manifest of changed_files and a completion status), or
explicitly state that implementation will be done in-process (no subagent) and
describe the outputs (changed_files list for step-04). Ensure references to
{spec_file}, ImplementFromSpec (or chosen task name), changed_files, and step-04
diff construction are included.
| ### Construct Diff (plan-code-review only) | ||
| ### Construct Diff | ||
|
|
||
| Read `{baseline_commit}` from `{spec_file}` frontmatter. If `{baseline_commit}` is missing or `NO_VCS`, use best effort to determine what changed. Otherwise, construct `{diff_output}` covering all changes — tracked and untracked — since `{baseline_commit}`. |
There was a problem hiding this comment.
Vague and fragile fallback: "best effort to determine what changed."
Line 22 says "If {baseline_commit} is missing or NO_VCS, use best effort to determine what changed." This is dangerously unspecified:
- Check
git status? But we're in NO_VCS mode. - Query the agent's session memory? Not reliable if step-03 delegated to a subagent.
- Scan the filesystem for recent modifications? Picks up unrelated changes.
In practice, this will either fail silently (empty diff) or include irrelevant files, producing false review findings.
⚠️ Recommended fix
Option 1: If baseline_commit is missing and NO_VCS, HALT with error:
-Read `{baseline_commit}` from `{spec_file}` frontmatter. If `{baseline_commit}` is missing or `NO_VCS`, use best effort to determine what changed. Otherwise, construct `{diff_output}` covering all changes — tracked and untracked — since `{baseline_commit}`.
+Read `{baseline_commit}` from `{spec_file}` frontmatter. If `{baseline_commit}` is missing, HALT with error: "baseline_commit not set in spec frontmatter. Cannot construct diff." If `{baseline_commit}` is `NO_VCS`, HALT and ask the human to provide a list of changed files or confirm all files in the working directory should be reviewed. Otherwise, construct `{diff_output}` covering all changes — tracked and untracked — since `{baseline_commit}`.Option 2: Require step-03 to track and write changed files to spec frontmatter as a fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-04-review.md`
at line 22, The current fallback "use best effort to determine what changed" for
reading `{baseline_commit}` from `{spec_file}` when `{baseline_commit}` is
missing or set to `NO_VCS` is underspecified and fragile; change the behavior so
that when `{baseline_commit}` is absent or `NO_VCS` the step halts with a clear
error asking the pipeline to provide a baseline or require step-03 to record
changed files into `{spec_file}` frontmatter as a fallback, and ensure
`{diff_output}` is only constructed from a trusted source (either a valid
`{baseline_commit}` or the explicit changed-files list written by step-03),
updating any code that reads `{spec_file}` and the logic around generating
`{diff_output}` to enforce this invariant.
| - **defer** — pre-existing issue not caused by this story, surfaced incidentally by the review. Collect for later focused attention. | ||
| - **reject** — noise. Drop silently. When unsure between defer and reject, prefer reject — only defer findings you are confident are real. | ||
| 3. Process findings in cascading order. If intent_gap or bad_spec findings exist, they trigger a loopback — lower findings are moot since code will be re-derived. If neither exists, process patch and defer normally. Increment `{specLoopIteration}` on each loopback. If it exceeds 5, HALT and escalate to the human. On any loopback, re-evaluate routing — if scope has grown beyond one-shot, escalate `execution_mode` to plan-code-review. | ||
| 3. Process findings in cascading order. If intent_gap or bad_spec findings exist, they trigger a loopback — lower findings are moot since code will be re-derived. If neither exists, process patch and defer normally. Increment `{specLoopIteration}` on each loopback. If it exceeds 5, HALT and escalate to the human. |
There was a problem hiding this comment.
Extreme loopback trigger: one finding causes full revert?
Line 43 says "If intent_gap or bad_spec findings exist, they trigger a loopback." Does this mean a SINGLE finding in either category triggers a full code revert and re-derivation? That seems disproportionate. What if the Blind hunter flags one minor bad_spec issue but the other two reviewers find nothing? The entire implementation is discarded and re-derived. Consider whether there should be a threshold (e.g., "2 or more intent_gap/bad_spec findings") or severity weighting.
Add guidance to deduplicate and consolidate findings first, then assess whether the root cause is systemic or isolated:
-3. Process findings in cascading order. If intent_gap or bad_spec findings exist, they trigger a loopback — lower findings are moot since code will be re-derived. If neither exists, process patch and defer normally. Increment `{specLoopIteration}` on each loopback. If it exceeds 5, HALT and escalate to the human.
+3. Process findings in cascading order. After deduplication, assess intent_gap and bad_spec findings: if they represent systemic issues (multiple findings or findings affecting core logic), trigger a loopback — lower findings are moot since code will be re-derived. If they are isolated edge cases, consider treating them as patch-level fixes. Increment `{specLoopIteration}` on each loopback. If it exceeds 5, HALT and escalate to the human.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-04-review.md`
at line 43, The current rule that any single "intent_gap" or "bad_spec" finding
triggers a full loopback is too aggressive; update the process to first
deduplicate and consolidate findings, then evaluate whether the issue is
systemic by applying a threshold or severity weighting (e.g., require >=2
distinct "intent_gap" or "bad_spec" findings or at least one high-severity
finding) before triggering the loopback and incrementing {specLoopIteration};
ensure consolidated findings are used when deciding to process a patch normally
versus escalate after {specLoopIteration} exceeds 5.
| ## RULES | ||
|
|
||
| - YOU MUST ALWAYS SPEAK OUTPUT in your Agent communication style with the config `{communication_language}` | ||
| - No push. No remote ops. |
There was a problem hiding this comment.
Contradiction: Line 40 offers push, but line 14 prohibits it.
Line 14 states "No push. No remote ops." but line 40 says "Offer to push and/or create a pull request." This is a direct contradiction that will confuse agents and users.
🔧 Proposed fix
-Offer to push and/or create a pull request. HALT and wait for human input.
+HALT. Inform the human that changes are committed locally. Remote operations (push, PR creation) are outside this workflow's scope.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-oneshot.md`
at line 14, The document contains a contradiction between the sentence "No push.
No remote ops." and the later phrase "Offer to push and/or create a pull
request."; decide which behavior is correct and make the text consistent: either
remove or reword the later offer ("Offer to push and/or create a pull request.")
to match the no-remote policy, or change the initial statement ("No push. No
remote ops.") to allow optional pushes (e.g., "No push by default; offer
optional push/PR") and update any related instructions or examples in
step-oneshot.md so both lines convey the same allowed remote behavior.
|
|
||
| ### Review | ||
|
|
||
| Invoke `{adversarial_review_task}` in a subagent with the changed files. The subagent gets NO conversation context — to avoid anchoring bias. If no sub-agents are available, write the changed files to a review prompt file in `{implementation_artifacts}` and HALT. Ask the human to run the review in a separate session and paste back the findings. |
There was a problem hiding this comment.
Missing specification: review prompt file naming pattern.
When subagents are unavailable, the instruction says "write the changed files to a review prompt file in {implementation_artifacts}" but provides no naming pattern. How will the human identify which file to run? Consider scenarios where multiple one-shot sessions run concurrently or the filesystem contains artifacts from previous runs.
📋 Proposed fix
-Invoke `{adversarial_review_task}` in a subagent with the changed files. The subagent gets NO conversation context — to avoid anchoring bias. If no sub-agents are available, write the changed files to a review prompt file in `{implementation_artifacts}` and HALT. Ask the human to run the review in a separate session and paste back the findings.
+Invoke `{adversarial_review_task}` in a subagent with the changed files. The subagent gets NO conversation context — to avoid anchoring bias. If no sub-agents are available, write the changed files to `{implementation_artifacts}/oneshot-review-prompt-{timestamp}.md` and HALT. Display the full path to the human and ask them to run the review in a separate session (ideally a different LLM) and paste back the findings.📝 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.
| Invoke `{adversarial_review_task}` in a subagent with the changed files. The subagent gets NO conversation context — to avoid anchoring bias. If no sub-agents are available, write the changed files to a review prompt file in `{implementation_artifacts}` and HALT. Ask the human to run the review in a separate session and paste back the findings. | |
| Invoke `{adversarial_review_task}` in a subagent with the changed files. The subagent gets NO conversation context — to avoid anchoring bias. If no sub-agents are available, write the changed files to `{implementation_artifacts}/oneshot-review-prompt-{timestamp}.md` and HALT. Display the full path to the human and ask them to run the review in a separate session (ideally a different LLM) and paste back the findings. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-oneshot.md`
at line 24, Add a deterministic, collision-resistant naming pattern for the
review prompt file written to {implementation_artifacts} when no subagents are
available: include unique identifiers such as the workflow run id or a generated
UUID, a UTC timestamp, the task name ({adversarial_review_task}) and a clear
extension like .review_prompt (e.g.,
{workflowRunId}_{adversarial_review_task}_{timestamp}.review_prompt); ensure any
code that writes or reads these files uses the same composition logic and
document the pattern so humans can find and correlate files from concurrent or
past one-shot sessions.
| Deduplicate all review findings. Three categories only: | ||
|
|
||
| - **patch** — trivially fixable. Auto-fix immediately. | ||
| - **defer** — pre-existing issue not caused by this change. Append to `{deferred_work_file}`. | ||
| - **reject** — noise. Drop silently. |
There was a problem hiding this comment.
Oversimplified classification: only 3 categories vs 5 in standard review.
The one-shot classification uses only patch/defer/reject, while step-04-review.md uses five categories: intent_gap, bad_spec, patch, defer, reject. Even "zero blast radius" changes can expose intent gaps or spec defects. Without intent_gap and bad_spec categories, one-shot has no mechanism to loop back and fix root causes—it will auto-patch symptoms or defer real problems.
🛠️ Recommended refactor
Add intent_gap and bad_spec categories with HALT-and-escalate behavior:
Deduplicate all review findings. Three categories only:
+- **intent_gap** — the clarified intent was incomplete. HALT and escalate to human.
+- **bad_spec** — the one-shot assessment was wrong (not actually zero blast radius). HALT and escalate to human to route via plan-code-review instead.
- **patch** — trivially fixable. Auto-fix immediately.
- **defer** — pre-existing issue not caused by this change. Append to `{deferred_work_file}`.
- **reject** — noise. Drop silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-oneshot.md`
around lines 28 - 32, Update the "Deduplicate all review findings"
classification in step-oneshot.md to match the five-category model used in
step-04-review.md: intent_gap, bad_spec, patch, defer, reject; keep patch =
auto-fix, defer = append to {deferred_work_file}, reject = drop, and implement
intent_gap and bad_spec as HALT-and-escalate actions (stop one-shot processing
and flag/route the finding for manual intervention/escalation) so one-shot can
surface root-cause issues rather than only auto-patching or deferring them.
| Deduplicate all review findings. Three categories only: | ||
|
|
||
| - **patch** — trivially fixable. Auto-fix immediately. | ||
| - **defer** — pre-existing issue not caused by this change. Append to `{deferred_work_file}`. |
There was a problem hiding this comment.
Unused variable: deferred_work_file declared but append instruction never executed.
Line 6 declares deferred_work_file and line 31 says "Append to {deferred_work_file}" in the defer classification, but there is no explicit instruction in the INSTRUCTIONS section to write deferred findings to that file. The step says "Append to" without an action verb like "Write" or "Save", making it unclear whether the agent should actually perform the append or if this is just a label.
✏️ Proposed fix
- **patch** — trivially fixable. Auto-fix immediately.
-- **defer** — pre-existing issue not caused by this change. Append to `{deferred_work_file}`.
+- **defer** — pre-existing issue not caused by this change. Write each deferred finding as a bullet item in `{deferred_work_file}`, creating the file if it doesn't exist.
- **reject** — noise. Drop silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-oneshot.md`
at line 31, The defer classification references a variable deferred_work_file
but never specifies an action to append findings; update the INSTRUCTIONS
section and the "defer" line so agents explicitly perform the append: change the
"Append to `{deferred_work_file}`" note into a concrete instruction (e.g.,
"Write/Append deferred finding to `{deferred_work_file}`") and ensure the step
logic that handles classifications (the defer branch in this step or any
function that processes classifications) calls the append/write operation to
deferred_work_file so the variable is actually used.
|
|
||
| ### Commit | ||
|
|
||
| If version control is available and the tree is dirty, create a local commit with a conventional message derived from the intent. If VCS is unavailable, skip. |
There was a problem hiding this comment.
Missing guidance: commit message format.
Line 36 says "create a local commit with a conventional message derived from the intent" but provides no format specification. What is "conventional" here? Conventional Commits format (type(scope): subject)? If so, how is the type determined? Where is the intent stored for derivation—there's no spec_file in the one-shot path, only session memory.
📝 Proposed fix
-If version control is available and the tree is dirty, create a local commit with a conventional message derived from the intent. If VCS is unavailable, skip.
+If version control is available and the tree is dirty, create a local commit. Use Conventional Commits format: `type(scope): subject`. Derive type (feat/fix/refactor/etc.) and subject from the clarified intent captured in Step 1. If VCS is unavailable, skip.📝 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.
| If version control is available and the tree is dirty, create a local commit with a conventional message derived from the intent. If VCS is unavailable, skip. | |
| If version control is available and the tree is dirty, create a local commit. Use Conventional Commits format: `type(scope): subject`. Derive type (feat/fix/refactor/etc.) and subject from the clarified intent captured in Step 1. If VCS is unavailable, skip. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/bmm/workflows/bmad-quick-flow/bmad-quick-dev-new-preview/steps/step-oneshot.md`
at line 36, Update the one-shot step text to explicitly require Conventional
Commits format "type(scope): subject" and define how to derive each piece:
determine "type" via a small mapping from intent keywords (e.g.,
"add"/"create"→feat, "fix"/"repair"→fix, "docs"→docs,
"refactor"/"cleanup"→refactor, otherwise chore); set "scope" to the target
component if available in session memory (or omit it if not); build the
"subject" as a concise imperative summary derived from the session memory intent
summary (fallback to a short autogenerated description), and if session memory
is absent do not create a commit; update the sentence that currently says
"conventional message derived from the intent" to include this exact format and
mapping so implementers know where to get the intent (session memory) and how to
form the commit message.
|
Superseded by new PR from org repo branch. |
1 participant
Summary
Reworked version of the original PR, updated for current
main(step files moved fromsteps/subdirectory to workflow root, content drift).step-oneshot.md— self-contained one-shot step with implement/review/classify/commit/presentexecution_modeand one-shot conditional branches from steps 3–5spec_filegeneration to plan-code-review path only (one-shot never needs it)What changed vs the original PR
steps/subdirectory)./relative paths instead of{installed_path}/steps/step-05-present.md(original PR missed this)Test plan
🤖 Generated with Claude Code