fix: remove output_folder/story_dir aliases, flatten variables sections#1608
Conversation
|
@coderabbitai review |
🤖 Augment PR SummarySummary: Simplifies Phase 4, Quick Flow, and QA workflows by removing redundant path aliases and standardizing on canonical artifact variables.
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis PR restructures Phase 4 and Quick Flow workflows by removing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip We've launched Issue Planner and it is currently in beta. Please try it out and share your feedback on Discord! 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/bmm/workflows/4-implementation/create-story/instructions.xml (1)
194-199:⚠️ Potential issue | 🟠 Major
{{previous_story_num}}is never computed, causing the previous story file lookup to fail.On line 195, the code attempts to load
{{implementation_artifacts}}/{{epic_num}}-{{previous_story_num}}-*.md, but{{previous_story_num}}is never defined or derived anywhere in the workflow. While{{story_num}}is explicitly set (lines 23, 49, 90, 147), there is no corresponding arithmetic operation to compute the previous story number (e.g.,story_num - 1). This leaves the glob pattern unresolved, causing the file load to silently fail and skip the previous story context.Add a computation step before the load action:
<check if="story_num > 1"> + <action>Set {{previous_story_num}} = {{story_num}} - 1</action> <action>Load previous story file: {{implementation_artifacts}}/{{epic_num}}-{{previous_story_num}}-*.md</action>src/bmm/workflows/4-implementation/retrospective/instructions.md (3)
368-377:⚠️ Potential issue | 🟠 MajorInconsistent wildcard escaping in epic file patterns.
Lines 368, 371, and 377 show inconsistent wildcard syntax when searching for epic files:
- Line 368:
epic\*/epic-{{next_epic_num}}.md(escaped)- Line 371:
*epic*/epic-{{next_epic_num}}.md(not escaped)- Line 377:
epic\*.md(escaped)This inconsistency creates ambiguity about whether the backslash is intentional escaping or template syntax. Most filesystem glob APIs would interpret
\*as a literal asterisk character, not a wildcard, causing pattern matching to fail.This appears to be a pre-existing issue not introduced by this PR, but it affects the same category of file discovery logic being modified in this PR.
🔧 Proposed fix: standardize wildcard syntax
-<action>Check if file exists: {planning_artifacts}/epic\*/epic-{{next_epic_num}}.md</action> +<action>Check if file exists: {planning_artifacts}/epic*/epic-{{next_epic_num}}.md</action> <check if="sharded epic file found"> - <action>Load {planning_artifacts}/*epic*/epic-{{next_epic_num}}.md</action> + <action>Load {planning_artifacts}/epic*/epic-{{next_epic_num}}.md</action> <action>Set {{next_epic_source}} = "sharded"</action> </check> **Fallback to whole document:** <check if="sharded epic not found"> -<action>Check if file exists: {planning_artifacts}/epic\*.md</action> +<action>Check if file exists: {planning_artifacts}/epic*.md</action>
165-173:⚠️ Potential issue | 🟠 MajorMissing validation that {{epic_number}} was successfully determined before use.
Step 2 (lines 165-173) reads story files using
{{epic_number}}and{{story_num}}variables, but there's no explicit check that{{epic_number}}was successfully set by Step 1.Step 1 has three conditional paths (priority 1, 2, 3) that each attempt to set
{{epic_number}}, but if all three fail - for instance:
- Priority 1: sprint-status file is corrupted or missing the expected entries
- Priority 2: user provides invalid input or declines to provide epic number
- Priority 3: user fails to confirm detected epic number
Then
{{epic_number}}would be undefined, causing Step 2 to construct invalid file patterns like{implementation_artifacts}/undefined-undefined-*.md.🛡️ Proposed validation gate
Add validation at the start of Step 2:
<step n="2" goal="Deep Story Analysis - Extract Lessons from Implementation"> +<check if="{{epic_number}} is undefined or empty"> + <output> +Bob (Scrum Master): "We couldn't determine which epic to review. Cannot proceed with retrospective." + </output> + <action>HALT</action> +</check> + <output> Bob (Scrum Master): "Before we start the team discussion, let me review all the story records to surface key themes. This'll help us have a richer conversation."
264-264:⚠️ Potential issue | 🟡 MinorAmbiguous file selection when multiple retrospective files exist for same epic.
Line 264 searches for the previous retrospective using pattern
{retrospectives_folder}/epic-{{prev_epic_num}}-retro-*.md, which could match multiple files if the retrospective was run multiple times for the same epic (e.g.,epic-1-retro-2026-01-15.mdandepic-1-retro-2026-02-01.md).The code says "search for previous retrospective" (singular) but provides no guidance on which file to load if multiple exist:
- Most recent (latest date)?
- First alphabetically?
- All files?
- Error on multiple?
This creates non-deterministic behavior where older retrospective data might be loaded instead of the most recent.
📋 Proposed clarification
- <action>Search for previous retrospective using pattern: {retrospectives_folder}/epic-{{prev_epic_num}}-retro-*.md</action> + <action>Search for previous retrospective using pattern: {retrospectives_folder}/epic-{{prev_epic_num}}-retro-*.md (if multiple exist, select most recent by date in filename)</action>Or implement explicit sorting:
<action>Search for previous retrospective using pattern: {retrospectives_folder}/epic-{{prev_epic_num}}-retro-*.md</action> + <action>If multiple files found, sort by filename (date) descending and select first</action>
🤖 Fix all issues with AI agents
In `@src/bmm/workflows/4-implementation/create-story/instructions.xml`:
- Line 195: Replace the double-braced config variable in the Load previous story
action so it uses the single-brace config syntax; specifically update the action
string containing "Load previous story file:
{{implementation_artifacts}}/{{epic_num}}-{{previous_story_num}}-*.md" to use
{implementation_artifacts} while keeping {{epic_num}} and {{previous_story_num}}
unchanged (look for the <action> element with "Load previous story file" in
instructions.xml).
In `@src/bmm/workflows/4-implementation/create-story/workflow.yaml`:
- Around line 20-25: The top-level file references are inconsistent: ux_file
uses a glob while epics_file, prd_file, and architecture_file use exact
filenames, and epics_file ("epics.md") conflicts with
input_file_patterns.epics.whole ("*epic*.md"); decide on one convention and
align them—either change ux_file to an exact filename (ux.md or the canonical UX
filename) to match epics_file/prd_file/architecture_file, or convert
epics_file/prd_file/architecture_file to globs (e.g., "*epic*.md", "*prd*.md",
"*architecture*.md") if globs are intended; specifically update the ux_file and
epics_file values to match the pattern used by input_file_patterns.epics.whole
(or update that pattern to the exact name) so all top-level variables
consistently resolve to the same matching scheme.
In `@src/bmm/workflows/4-implementation/retrospective/instructions.md`:
- Line 83: Add a defensive validation step that ensures the workflow variable
{implementation_artifacts} is defined and non-empty before any use (specifically
before the "Scan {implementation_artifacts} for highest numbered story files"
step and the later reference at line 173); implement a new step (e.g., step
n="0.1" goal="Validate required variables") that checks
"{implementation_artifacts} is undefined or empty", prints a clear error like
"ERROR: {implementation_artifacts} variable is not defined. Check that
workflow.yaml was loaded correctly.", and HALTs the workflow if the check fails
so subsequent steps (scanning or listing) cannot run with a missing variable.
- Line 173: The glob pattern in the action uses an escaped asterisk
`{implementation_artifacts}/{{epic_number}}-{{story_num}}-\*.md`, which matches
a literal '*' instead of wildcard; update the pattern used in the action (the
string containing `{{epic_number}}-{{story_num}}-\\*.md`) to remove the
backslash so it becomes a proper wildcard (e.g.,
`{{epic_number}}-{{story_num}}-*.md`), and scan the same file for other
similarly escaped patterns like `epic\*/epic-{{next_epic_num}}.md` and
`epic\*.md` to fix them consistently.
- Line 173: The instruction is ambiguous about how {{story_num}} is produced;
update the action to explicitly define the iteration source and termination:
either iterate i from 1..{{total_stories}} (where {{total_stories}} was obtained
from sprint_status_file) and skip missing files, or explicitly state that the
agent should glob the {implementation_artifacts} directory for files matching
pattern "{{epic_number}}-*-*.md" and derive {{story_num}} from filenames;
reference and link this behavior to the previously-determined {{total_stories}}
(from sprint_status_file) if using the 1..N approach, and add a rule for
non-contiguous/missing story files (e.g., log and continue). Ensure the updated
action mentions the exact variables {{story_num}}, {{total_stories}},
{implementation_artifacts}, and sprint_status_file so the iteration mechanism is
unambiguous.
- Line 173: The instruction "For each story in epic {{epic_number}}, read the
complete story file from
{implementation_artifacts}/{{epic_number}}-{{story_num}}-*.md" is ambiguous when
the glob matches 0 or multiple files; change it to require exactly one match:
when processing the pattern
{implementation_artifacts}/{{epic_number}}-{{story_num}}-*.md, if exactly one
file is found read it, if zero files or more than one file are found emit a
deterministic error/validation message stating "Expected exactly one story file
for {{epic_number}}-{{story_num}}, found N" and halt (or surface the error to
the caller) so downstream steps never proceed with ambiguous input.
- Around line 83-84: Add explicit error handling around every filesystem
operation referenced in the workflow (e.g., the "Scan
{implementation_artifacts}" step, "Extract epic numbers from story filenames",
the "Read story files" step, "Search for previous retro", and "Load
sprint_status_file") by verifying the directory exists and is readable before
scanning, wrapping file reads/parses in try/catch-style checks that detect
missing files, permission errors, encoding/corruption, and malformed
YAML/Markdown, and by emitting clear, contextual messages and control flow: halt
the workflow with a user-facing error for unrecoverable failures (e.g., missing
{implementation_artifacts} dir or unreadable sprint_status_file) and
skip/continue with a logged warning for per-file read/parse errors (e.g.,
unreadable story file), ensuring each action includes the filename or epic/story
id in the message so downstream steps won't operate on undefined data.
- Around line 83-84: Replace the vague "scan {implementation_artifacts} for
highest numbered story files" with an explicit glob/regex to limit the search to
story filenames; use the concrete pattern already used later for consistency
(e.g. {implementation_artifacts}/{{epic_number}}-{{story_num}}-*.md or a glob
matching epic-X-Y-story-name.md like {implementation_artifacts}/epic-*-*-*.md),
update the instruction text to instruct agents to scan only files matching that
pattern, and ensure the extraction step references the same pattern when pulling
epic and story numbers.
In `@src/bmm/workflows/4-implementation/sprint-status/workflow.yaml`:
- Around line 20-21: Add the missing project_context key to this workflow by
inserting project_context: "**/project-context.md" above sprint_status_file to
match sibling workflows (look for the project_context key used in
sprint-planning/create-story/code-review/dev-story), and keep the existing
sprint_status_file and input_file_patterns.sprint_status.whole definitions as-is
to preserve the current duplication pattern.
In `@src/bmm/workflows/bmad-quick-flow/quick-spec/steps/step-01-understand.md`:
- Around line 79-81: The instruction conflates locations: update the step
content so planning-phase outputs (PRD, architecture, epics, research) are
explicitly checked in `{planning_artifacts}` and implementation-phase items
(stories, tech-specs, implementation notes) are explicitly checked in
`{implementation_artifacts}`; keep the separate checks for
`**/project-context.md` and for "existing stories or specs related to user's
request" but ensure each line maps to the correct directory (use
`{planning_artifacts}` for PRD/architecture/epics/research and
`{implementation_artifacts}` for stories/tech-specs).
🧹 Nitpick comments (8)
src/bmm/workflows/4-implementation/create-story/instructions.xml (1)
59-174: Massive duplication: lines 59–117 and 119–174 are near-identical blocks.The sprint-status loading, backlog-story discovery, key extraction, and epic-status update logic is duplicated verbatim between the "no user input" branch and the fallthrough path. This makes it easy for future changes (like this PR's
story_dir→implementation_artifactsfix) to be applied in one copy but missed in the other. Consider extracting the shared logic into a reusable sub-step or protocol.Pre-existing, not introduced by this PR — flagging for awareness.
src/bmm/workflows/bmad-quick-flow/quick-spec/workflow.md (1)
69-74:project_contextis not resolved here, unlike the YAML-based workflows.Other workflows in this PR (code-review, sprint-planning) define
project_context: "**/project-context.md"as a top-level key. The quick-spec workflow does not resolveproject_context, even thoughstep-01-understand.mdline 80 hardcodes the glob**/project-context.mddirectly in its instructions. This works but creates a maintenance divergence: if the project-context filename or location ever changes, the step file must be updated manually rather than inheriting it from a single config key.Consider adding
project_contextto the resolved variables for consistency.src/bmm/workflows/4-implementation/sprint-planning/workflow.yaml (2)
26-27:story_locationandstory_location_absolutebypass the already-resolved{implementation_artifacts}key.Both lines resolve
{config_source}:implementation_artifactsdirectly from config, duplicating the resolution that already happens at line 10. If the config reference at line 10 is ever changed (e.g., to a different key name or override), these two lines will silently diverge.Use the already-resolved key instead:
Proposed fix
-story_location: "{config_source}:implementation_artifacts" # Relative path for file-system, Future will support URL for Jira/Linear/Trello -story_location_absolute: "{config_source}:implementation_artifacts" # Absolute path for file operations +story_location: "{implementation_artifacts}" # Relative path for file-system, Future will support URL for Jira/Linear/Trello +story_location_absolute: "{implementation_artifacts}" # Absolute path for file operations
33-34: Duplicate# Output configurationsection comment.Lines 33 and 46 both carry the
# Output configurationheading. The first introducesstatus_file; the second introducesdefault_output_file. This reads as two distinct sections with the same name, which is confusing — especially sincedefault_output_fileis just a reference tostatus_file. Consider merging them under a single comment block or renaming one.Also applies to: 46-47
src/bmm/workflows/4-implementation/create-story/checklist.md (2)
52-52:planning_artifactsomitted from the representative variable list.This line calls out
implementation_artifactsandepics_fileas examples, butplanning_artifactsis equally foundational — it's the base path forepics_file,prd_file,architecture_file, andux_fileinworkflow.yaml. Listing it here would make the hint more useful for someone running this checklist in a fresh context.-- **Workflow variables**: From workflow.yaml (implementation_artifacts, epics_file, etc.) +- **Workflow variables**: From workflow.yaml (planning_artifacts, implementation_artifacts, epics_file, etc.)
68-68: Same omission on the procedural step —planning_artifactsmissing.Line 68 lists variables to resolve but omits
planning_artifacts, which is the parent path for most of the input files the subsequent steps reference ({epics_file},{architecture_file},{prd_file}). Including it reinforces that the resolver must expand it before the dependent paths make sense.-5. **Resolve all workflow variables**: implementation_artifacts, epics_file, architecture_file, etc. +5. **Resolve all workflow variables**: planning_artifacts, implementation_artifacts, epics_file, architecture_file, etc.src/bmm/workflows/4-implementation/retrospective/instructions.md (2)
173-173: Path construction assumes {implementation_artifacts} has no trailing slash.Line 173 constructs the path as
{implementation_artifacts}/{{epic_number}}-{{story_num}}-*.mdby directly concatenating with/. If{implementation_artifacts}includes a trailing slash (e.g.,"output/artifacts/"), this produces a double slash:"output/artifacts//1-1-*.md".While most filesystem APIs tolerate double slashes, this creates inconsistency and potential issues with path normalization, comparison, or logging.
The same assumption applies to line 83's use of
{implementation_artifacts}.📝 Recommendation: document path format assumption
Add a note in the workflow.yaml file (or reference documentation) specifying:
<!-- Variable format assumptions: - {implementation_artifacts} must NOT have trailing slash (e.g., "output/artifacts", not "output/artifacts/") - {planning_artifacts} must NOT have trailing slash - {retrospectives_folder} must NOT have trailing slash -->Alternatively, if the workflow engine supports path normalization functions, use them.
83-83: Inconsistent naming convention across artifact path variables.The workflow uses mixed naming conventions for path variables:
{planning_artifacts}(plural, generic artifacts){implementation_artifacts}(plural, generic artifacts){retrospectives_folder}(singular, specific type "folder"){sprint_status_file}(singular, specific type "file")This inconsistency creates conceptual confusion about what each variable represents:
- Do
*_artifactsvariables point to directories or could they be file paths?- Why is retrospectives called "folder" but implementation/planning are "artifacts"?
- Is this a naming convention issue or intentional semantic distinction?
The PR introduced
implementation_artifactsto align withplanning_artifacts, but leftretrospectives_folderwith different naming. This suggests incomplete standardization of the path variable naming scheme.Consider standardizing to one of these patterns:
Option 1: All artifacts (current direction)
{implementation_artifacts}{planning_artifacts}{retrospective_artifacts}(not retrospectives_folder)Option 2: All folders (more explicit)
{implementation_folder}{planning_folder}{retrospectives_folder}(current)Option 3: Use _dir suffix (more explicit than artifacts)
{implementation_dir}{planning_dir}{retrospectives_dir}Regardless, document the naming convention in a central location.
Also applies to: 264-264, 368-368
src/bmm/workflows/4-implementation/create-story/instructions.xml
Outdated
Show resolved
Hide resolved
src/bmm/workflows/4-implementation/retrospective/instructions.md
Outdated
Show resolved
Hide resolved
src/bmm/workflows/bmad-quick-flow/quick-spec/steps/step-01-understand.md
Show resolved
Hide resolved
…ns (bmad-code-org#1602) Drop pointless alias variables (output_folder, story_dir, story_directory) from Phase 4, Quick Flow, and QA workflows. Replace all references with the canonical {implementation_artifacts} or {planning_artifacts} variables. Also flatten unnecessary `variables:` YAML nesting in all affected workflow.yaml files — the workflow engine treats all keys as top-level, so the nesting added complexity with no semantic value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ad variables - Add missing document_output_language and user_skill_level to create-story (referenced in instructions.xml but never declared) - Remove retrospectives_folder alias, replace with canonical implementation_artifacts - Remove unused sprint_status and duplicate validation alias from correct-course - Remove unused date, planning_artifacts, tracking_system from sprint-status Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…iable
- Fix single/double brace mismatch for {implementation_artifacts} in
create-story instructions.xml (F1)
- Remove escaped asterisks in glob patterns in retrospective
instructions.md (F2)
- Eliminate redundant {config_source} re-resolution for story_location
in sprint-planning workflow.yaml (F5)
- Add explicit instruction to discover previous_story_num by scanning
artifacts instead of leaving it undefined (#7)
- Rename output_folder to project_knowledge in document-project
workflows to stop shadowing the canonical core config variable (#13)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The glob pattern can match multiple retrospective files for the same epic (e.g., partial mid-sprint retro and full completion retro). Use plural "retrospectives" to make clear the LLM should load all matches. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alexeyv
force-pushed
the
fix/1602-output-folder-override
branch
from
47d3632 to
98fc7cd
Compare
2 participants
Summary
Closes #1602
output_folder,story_dir,story_directory) from Phase 4, Quick Flow, and QA workflows{implementation_artifacts}/{planning_artifacts}variablesvariables:YAML nesting in all affected workflow.yaml files — the workflow engine treats all keys as top-level, so the nesting added complexity with no semantic valueTest plan
grep -rn 'output_folder'in Phase 4 / Quick Flow / QA workflows — zero resultsgrep -rn 'story_dir'in Phase 4 workflows — zero resultsgrep -rn 'story_directory'in Phase 4 workflows — zero resultsnpm test— all tests passNote
Filed #1607 for broader
output_foldercleanup across phases 1-3, document-project, and other workflows.🤖 Generated with Claude Code