fix(workflow): use git for dev-story File List#1793
fix(workflow): use git for dev-story File List#1793A386official wants to merge 2 commits intobmad-code-org:mainfrom
Conversation
📝 WalkthroughWalkthroughThe PR updates the dev-story workflow and story creation template to implement git-driven file discovery for File List generation. Instead of relying on agent-driven self-assessment, the workflow now uses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bmm/workflows/4-implementation/dev-story/instructions.xml (1)
298-315:⚠️ Potential issue | 🟠 MajorOrdering issue: git commands run before story file is saved.
The sequence is:
- Line 298: Mark task checkbox
[x](in-memory change)- Lines 299-301: Run git commands and update File List
- Line 315: Save the story file
Git commands at lines 299-300 inspect the working tree, but the story file modifications from marking the task complete haven't been written to disk yet (save happens at line 315). This means:
- The story file won't appear in git output for the current task
- Only on the next task iteration would the previous save be visible
This partially defeats the purpose of capturing "the story file itself" as stated in line 300.
Proposed fix: save before git commands
<check if="ALL validation gates pass AND tests ACTUALLY exist and pass"> <action>ONLY THEN mark the task (and subtasks) checkbox with [x]</action> + <action>Save the story file to ensure in-memory changes are visible to git</action> <action>Run `git diff --name-only` and `git status --porcelain` to discover ALL changed files (including infrastructure side-effects like lock files, the story file itself, and sprint-status.yaml)</action> <action>Update File List section from git output with ALL new, modified, or deleted files (paths relative to repo root)</action> <action>Add completion notes to Dev Agent Record summarizing what was ACTUALLY implemented and tested</action> + <action>Save the story file again to persist File List and completion notes</action> </check> - - ... - - <action>Save the story file</action>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml` around lines 298 - 315, Move the "Save the story file" action to occur immediately after the in-memory task checkbox update (the action that marks the task and subtasks with [x]) and before running the git inspection commands and File List update; in other words, keep the sequence: mark checkbox (in-memory), Save the story file, then run `git diff --name-only` / `git status --porcelain` and Update File List, so the saved story file appears in the git output used by the File List and subsequent validation steps.
🧹 Nitpick comments (4)
src/bmm/workflows/4-implementation/dev-story/instructions.xml (4)
341-341: DoD validation repeats incomplete command set.This is the third location specifying the git commands, and all three are inconsistent with the issue's specification (missing
--cached). This creates maintenance burden and risks drift. Consider defining the command set once and referencing it.Consider extracting the git discovery logic into a reusable block or variable at the top of the workflow (e.g.,
{{git_changed_files_commands}}) to ensure consistency and simplify future updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml` at line 341, The DoD validation repeats an incomplete git command set (missing --cached) in multiple places; fix by centralizing the git discovery commands into a single reusable variable/block (e.g., {{git_changed_files_commands}}) at the top of the workflow and replace all three inline occurrences with that reference, and update the canonical command to include the --cached flag (for example use `git diff --name-only --cached` and `git status --porcelain --cached` or the correct cached variants per repository semantics) so future updates only change the command in one place.
320-322: Potential infinite loop if validation keeps failing after fix attempts.The flow at lines 317-322 either loops back to step 5 (more tasks) or proceeds to step 9 (completion). However, if step 8 validation fails (line 305-308), it says "fix issues first" then potentially HALTs. But if the fix succeeds partially and the agent continues, there's no explicit re-validation loop—the agent might mark incomplete tasks as complete after a fix without re-running the full validation gates.
Consider whether the validation gates should be re-executed after fixes before marking complete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml` around lines 320 - 322, The workflow can prematurely advance to step 9 or loop indefinitely because fixes aren't forced through the validation gate; modify the flow so that after any "fix issues" action (the step that currently halts on failed validation at step 8) the system must re-run the validation step (step 8) before proceeding to the "no tasks remain" check or the goto step="9" completion; implement an explicit transition from the fix path back into the validation action (or from action if="no tasks remain" include a validation_pass condition) and add a retry/attempt limit to the validation loop to prevent infinite retries. Ensure you update the transitions referenced by the action if="no tasks remain", goto step="9", step 8 validation, and step 5 task loop so fixes always re-enter step 8 validation and stop after a bounded number of attempts.
371-371: "File List is incomplete" lacks definition.This HALT condition triggers if the File List is incomplete, but "incomplete" is never defined. Is it:
- Empty?
- Missing files that git reports?
- Containing paths that git doesn't report (stale entries)?
- Incorrectly formatted?
Without clear criteria, agents may inconsistently evaluate completeness, leading to either spurious HALTs or missed issues.
Consider adding explicit criteria:
<action if="File List is missing any path reported by git commands">HALT - Update File List with all changed files</action>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml` at line 371, Update the ambiguous HALT condition on the <action if="File List is incomplete"> element by replacing the vague "incomplete" predicate with explicit, testable criteria: check for an empty file list, any path reported by git status/ls-files/changed-files that is missing from the File List, any extra/stale paths not present in git outputs, and syntactic/format validation for each path; implement the new condition as something like "File List is missing any path reported by git commands or contains malformed paths" so the agent can deterministically compare the File List against git outputs and HALT only when those concrete checks fail.
301-301: No guidance on representing deleted files.
git status --porcelainprefixes deleted files withDandgit diff --name-onlyincludes them. The instruction says "deleted files" should appear in the File List, but there's no format guidance. Should entries be:
- Plain paths:
src/old-file.ts- Annotated:
src/old-file.ts (deleted)- Prefixed:
D src/old-file.tsWithout clarity, agents may inconsistently format deletions, complicating downstream code-review validation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml` at line 301, Clarify the "Update File List" instruction by specifying how to represent deletions: require entries be paths relative to the repo root and annotate deleted files using the git-porcelain prefix "D " (e.g., "D src/old-file.ts"); likewise recommend using "A " for added and "M " for modified if you want consistency with git status --porcelain, and update the action text "Update File List section from git output with ALL new, modified, or deleted files (paths relative to repo root)" to explicitly state the "D " prefix for deletions.
🤖 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/4-implementation/create-story/template.md`:
- Around line 49-52: Update the "File List" section in template.md to (1)
explicitly instruct using three git commands—git status --porcelain, git diff
--name-only, and git diff --cached --name-only—to capture all changed files
including staged ones, (2) add an exclusion rule to omit _bmad/ and
_bmad-output/ from automatic listings, (3) provide a clear list structure
example (e.g., one bullet per path or one path per line) as the placeholder for
file entries, and (4) replace or clarify the self-referential phrase "this story
file" with a clear placeholder like <STORY_FILENAME> so agents know which file
to include.
In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml`:
- Line 301: The step labeled "Update File List section from git output" is
ambiguous; change it to explicitly REPLACE the entire File List section with the
current git output (paths relative to repo root) rather than append or merge,
and update the action text and any implementation of that step (the "Update File
List section from git output" handler) to overwrite the File List content
atomically; also add a concise note in the same handler that this is a full
replace operation to avoid duplicates or lost state.
- Around line 299-301: The workflow currently runs the actions "Run `git diff
--name-only` and `git status --porcelain`" and then updates the "File List"
without checking for failures; modify the implementation around those actions to
check exit codes and stderr for failures (git not found, not a git repo,
detached HEAD, unresolved merge conflicts, permission errors), and on any
non-zero exit or recognized error output emit a HALT action like <action if="git
commands fail or working directory is not a git repository">HALT: "Cannot
determine changed files - git commands failed or not in a git
repository"</action>, ensuring the "Update File List" step only runs when both
git commands succeed and produce valid output.
- Around line 299-301: Update the first <action> (the one that currently
instructs "Run `git diff --name-only` and `git status --porcelain`...") to also
run `git diff --cached --name-only` so staged changes are captured, and modify
the second <action> ("Update File List section from git output...") to apply
filtering that excludes any paths under `_bmad/` and `_bmad-output/` before
populating the File List; ensure the instructions specify merging results from
`git diff --name-only`, `git diff --cached --name-only`, and `git status
--porcelain` then removing entries that match the `_bmad/` or `_bmad-output/`
prefixes (including deletions) so those directories are never auto-added.
- Line 328: The current action "Run `git diff --name-only` and `git status
--porcelain`" is ambiguous and misses the staged/unstaged/untracked
distinctions; change it to run explicit commands to capture staged, unstaged and
untracked files (e.g., `git diff --name-only --cached`, `git diff --name-only`,
and `git ls-files --others --exclude-standard` or use `git status --porcelain=v1
-z`), then define precise matching semantics for the "File List": require exact
set equality between the computed git set and the provided File List, and if
they differ the agent must HALT (the existing HALT for "incomplete" File List)
while emitting a clear mismatch report listing files only-in-git,
only-in-File-List, and suggesting the user update the File List (do not
auto-fix); update the instruction text referencing the original action line and
the File List/HALT behavior so downstream logic can implement the set comparison
and report.
---
Outside diff comments:
In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml`:
- Around line 298-315: Move the "Save the story file" action to occur
immediately after the in-memory task checkbox update (the action that marks the
task and subtasks with [x]) and before running the git inspection commands and
File List update; in other words, keep the sequence: mark checkbox (in-memory),
Save the story file, then run `git diff --name-only` / `git status --porcelain`
and Update File List, so the saved story file appears in the git output used by
the File List and subsequent validation steps.
---
Nitpick comments:
In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml`:
- Line 341: The DoD validation repeats an incomplete git command set (missing
--cached) in multiple places; fix by centralizing the git discovery commands
into a single reusable variable/block (e.g., {{git_changed_files_commands}}) at
the top of the workflow and replace all three inline occurrences with that
reference, and update the canonical command to include the --cached flag (for
example use `git diff --name-only --cached` and `git status --porcelain
--cached` or the correct cached variants per repository semantics) so future
updates only change the command in one place.
- Around line 320-322: The workflow can prematurely advance to step 9 or loop
indefinitely because fixes aren't forced through the validation gate; modify the
flow so that after any "fix issues" action (the step that currently halts on
failed validation at step 8) the system must re-run the validation step (step 8)
before proceeding to the "no tasks remain" check or the goto step="9"
completion; implement an explicit transition from the fix path back into the
validation action (or from action if="no tasks remain" include a validation_pass
condition) and add a retry/attempt limit to the validation loop to prevent
infinite retries. Ensure you update the transitions referenced by the action
if="no tasks remain", goto step="9", step 8 validation, and step 5 task loop so
fixes always re-enter step 8 validation and stop after a bounded number of
attempts.
- Line 371: Update the ambiguous HALT condition on the <action if="File List is
incomplete"> element by replacing the vague "incomplete" predicate with
explicit, testable criteria: check for an empty file list, any path reported by
git status/ls-files/changed-files that is missing from the File List, any
extra/stale paths not present in git outputs, and syntactic/format validation
for each path; implement the new condition as something like "File List is
missing any path reported by git commands or contains malformed paths" so the
agent can deterministically compare the File List against git outputs and HALT
only when those concrete checks fail.
- Line 301: Clarify the "Update File List" instruction by specifying how to
represent deletions: require entries be paths relative to the repo root and
annotate deleted files using the git-porcelain prefix "D " (e.g., "D
src/old-file.ts"); likewise recommend using "A " for added and "M " for modified
if you want consistency with git status --porcelain, and update the action text
"Update File List section from git output with ALL new, modified, or deleted
files (paths relative to repo root)" to explicitly state the "D " prefix for
deletions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/bmm/workflows/4-implementation/create-story/template.mdsrc/bmm/workflows/4-implementation/dev-story/instructions.xml
| ### File List | ||
|
|
||
| <!-- Use `git diff --name-only` and `git status --porcelain` to capture ALL changed files, | ||
| including infrastructure side-effects: lock files, this story file, sprint-status.yaml --> |
There was a problem hiding this comment.
File List section lacks structure and critical details.
Several concerns with this template addition:
-
Missing exclusion guidance: The linked issue explicitly states that
_bmad/and_bmad-output/should be excluded from automatic additions to avoid cluttering the File List with workflow artifacts. This exclusion rule is absent from the comment. -
Incomplete git command set: The issue specifies three commands:
git status --porcelain,git diff --name-only, andgit diff --cached --name-only. The comment omits--cachedfor staged files. -
No list structure: The section ends immediately after the comment with no placeholder or formatting guidance for actual file entries (e.g., bullet list, one path per line).
-
Self-referential ambiguity: "this story file" is meaningless in an unfilled template—the agent won't know the story filename until the template is instantiated.
Proposed improvement
### File List
-<!-- Use `git diff --name-only` and `git status --porcelain` to capture ALL changed files,
- including infrastructure side-effects: lock files, this story file, sprint-status.yaml -->
+<!--
+ Populate using git commands after each task:
+ git diff --name-only
+ git diff --cached --name-only
+ git status --porcelain
+ Include infrastructure side-effects (lock files, sprint-status.yaml, story file).
+ Exclude: _bmad/, _bmad-output/ directories.
+-->
+
+- (list files here, one per line)📝 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.
| ### File List | |
| <!-- Use `git diff --name-only` and `git status --porcelain` to capture ALL changed files, | |
| including infrastructure side-effects: lock files, this story file, sprint-status.yaml --> | |
| ### File List | |
| <!-- | |
| Populate using git commands after each task: | |
| git diff --name-only | |
| git diff --cached --name-only | |
| git status --porcelain | |
| Include infrastructure side-effects (lock files, sprint-status.yaml, story file). | |
| Exclude: _bmad/, _bmad-output/ directories. | |
| --> | |
| - (list files here, one per line) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bmm/workflows/4-implementation/create-story/template.md` around lines 49
- 52, Update the "File List" section in template.md to (1) explicitly instruct
using three git commands—git status --porcelain, git diff --name-only, and git
diff --cached --name-only—to capture all changed files including staged ones,
(2) add an exclusion rule to omit _bmad/ and _bmad-output/ from automatic
listings, (3) provide a clear list structure example (e.g., one bullet per path
or one path per line) as the placeholder for file entries, and (4) replace or
clarify the self-referential phrase "this story file" with a clear placeholder
like <STORY_FILENAME> so agents know which file to include.
| <action>Run `git diff --name-only` and `git status --porcelain` to discover ALL changed files (including infrastructure | ||
| side-effects like lock files, the story file itself, and sprint-status.yaml)</action> | ||
| <action>Update File List section from git output with ALL new, modified, or deleted files (paths relative to repo root)</action> |
There was a problem hiding this comment.
No error handling for git command failures.
What happens if:
- The repository is in a detached HEAD state?
- There are merge conflicts (git status output differs)?
- The working directory is not a git repository?
- Git is not installed or not in PATH?
The workflow assumes git commands succeed but provides no fallback or HALT condition for failures. This could lead to silent failures where the File List remains incomplete.
Suggested addition
<action if="git commands fail or working directory is not a git repository">HALT: "Cannot determine changed files - git commands failed or not in a git repository"</action>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml` around lines
299 - 301, The workflow currently runs the actions "Run `git diff --name-only`
and `git status --porcelain`" and then updates the "File List" without checking
for failures; modify the implementation around those actions to check exit codes
and stderr for failures (git not found, not a git repo, detached HEAD,
unresolved merge conflicts, permission errors), and on any non-zero exit or
recognized error output emit a HALT action like <action if="git commands fail or
working directory is not a git repository">HALT: "Cannot determine changed files
- git commands failed or not in a git repository"</action>, ensuring the "Update
File List" step only runs when both git commands succeed and produce valid
output.
Missing --cached flag and exclusion logic.
The issue explicitly specifies running git diff --cached --name-only alongside the other commands to capture staged files. This is omitted here.
Additionally, the issue states: "Apply the same exclusion used by code-review: exclude _bmad/ and _bmad-output/ from automatic additions." There's no filtering logic here—git output will include these directories, and the agent will blindly add them to the File List, defeating the purpose.
Proposed fix to add cached diff and exclusion
- <action>Run `git diff --name-only` and `git status --porcelain` to discover ALL changed files (including infrastructure
- side-effects like lock files, the story file itself, and sprint-status.yaml)</action>
- <action>Update File List section from git output with ALL new, modified, or deleted files (paths relative to repo root)</action>
+ <action>Run `git diff --name-only`, `git diff --cached --name-only`, and `git status --porcelain` to discover ALL changed files
+ (including infrastructure side-effects like lock files, the story file itself, and sprint-status.yaml)</action>
+ <action>Filter out paths matching `_bmad/` and `_bmad-output/` directories from the git output</action>
+ <action>Update File List section with filtered results: ALL new, modified, or deleted files (paths relative to repo root)</action>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml` around lines
299 - 301, Update the first <action> (the one that currently instructs "Run `git
diff --name-only` and `git status --porcelain`...") to also run `git diff
--cached --name-only` so staged changes are captured, and modify the second
<action> ("Update File List section from git output...") to apply filtering that
excludes any paths under `_bmad/` and `_bmad-output/` before populating the File
List; ensure the instructions specify merging results from `git diff
--name-only`, `git diff --cached --name-only`, and `git status --porcelain` then
removing entries that match the `_bmad/` or `_bmad-output/` prefixes (including
deletions) so those directories are never auto-added.
| <action>Update File List section with ALL new, modified, or deleted files (paths relative to repo root)</action> | ||
| <action>Run `git diff --name-only` and `git status --porcelain` to discover ALL changed files (including infrastructure | ||
| side-effects like lock files, the story file itself, and sprint-status.yaml)</action> | ||
| <action>Update File List section from git output with ALL new, modified, or deleted files (paths relative to repo root)</action> |
There was a problem hiding this comment.
Ambiguous update semantics: replace vs. append.
"Update File List section from git output" doesn't clarify whether this:
- Replaces the entire File List with current git output (loses manually added entries)
- Appends new files to existing list (may create duplicates)
- Merges with deduplication (ideal but not stated)
Since git commands show cumulative changes from the base, option 1 might be correct, but this should be explicit to prevent agent confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml` at line 301,
The step labeled "Update File List section from git output" is ambiguous; change
it to explicitly REPLACE the entire File List section with the current git
output (paths relative to repo root) rather than append or merge, and update the
action text and any implementation of that step (the "Update File List section
from git output" handler) to overwrite the File List content atomically; also
add a concise note in the same handler that this is a full replace operation to
avoid duplicates or lost state.
| <action>Verify ALL tasks and subtasks are marked [x] (re-scan the story document now)</action> | ||
| <action>Run the full regression suite (do not skip)</action> | ||
| <action>Confirm File List includes every changed file</action> | ||
| <action>Run `git diff --name-only` and `git status --porcelain` to get actual changed files; confirm File List matches git output</action> |
There was a problem hiding this comment.
Inconsistent command set and ambiguous matching semantics.
This line mirrors the issues from Step 8 (missing --cached, no exclusion). Additionally, "confirm File List matches git output" is vague. Does this mean:
- Exact match (same set of files)?
- Superset (File List contains at least all git-reported files)?
- Should the agent auto-fix mismatches or just HALT?
The only downstream handling is the HALT at line 371 for "incomplete" File List, but there's no guidance on how to detect or resolve the mismatch.
Suggested clarification
- <action>Run `git diff --name-only` and `git status --porcelain` to get actual changed files; confirm File List matches git output</action>
+ <action>Run `git diff --name-only`, `git diff --cached --name-only`, and `git status --porcelain` to get actual changed files (excluding _bmad/ and _bmad-output/); verify File List contains ALL reported paths and auto-append any missing entries</action>📝 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.
| <action>Run `git diff --name-only` and `git status --porcelain` to get actual changed files; confirm File List matches git output</action> | |
| <action>Run `git diff --name-only`, `git diff --cached --name-only`, and `git status --porcelain` to get actual changed files (excluding _bmad/ and _bmad-output/); verify File List contains ALL reported paths and auto-append any missing entries</action> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bmm/workflows/4-implementation/dev-story/instructions.xml` at line 328,
The current action "Run `git diff --name-only` and `git status --porcelain`" is
ambiguous and misses the staged/unstaged/untracked distinctions; change it to
run explicit commands to capture staged, unstaged and untracked files (e.g.,
`git diff --name-only --cached`, `git diff --name-only`, and `git ls-files
--others --exclude-standard` or use `git status --porcelain=v1 -z`), then define
precise matching semantics for the "File List": require exact set equality
between the computed git set and the provided File List, and if they differ the
agent must HALT (the existing HALT for "incomplete" File List) while emitting a
clear mismatch report listing files only-in-git, only-in-File-List, and
suggesting the user update the File List (do not auto-fix); update the
instruction text referencing the original action line and the File List/HALT
behavior so downstream logic can implement the set comparison and report.
- Move story file save BEFORE git commands so its changes appear in working tree - Add git diff --cached --name-only to capture staged files alongside unstaged - Add _bmad/ and _bmad-output/ exclusion filtering to all git inspection steps - Clarify File List update uses full-replace semantics (not append) - Add list structure placeholder to story template - Apply consistent three-command git set across Step 8, Step 9, and DoD checklist
|
Addressed in the latest push — thanks for the thorough review. Changes made:
|
1 participant
Summary
The dev-story workflow builds its File List from agent memory, which misses infrastructure side-effects like
pnpm-lock.yaml, the story file itself, andsprint-status.yaml. This causes a false-positive MEDIUM finding on every code-review run.The fix replaces agent-driven File List updates (Steps 8 and 9) with
git diff --name-only/git status --porcelain- the same approach the code-review workflow already uses for change discovery.Also adds a hint comment to the story template's File List section so the pattern is visible to agents from the start.
Closes #1789
Test plan