fix(story-automator): reconcile story File List against git before review#41
fix(story-automator): reconcile story File List against git before review#41Pawel-N-pl wants to merge 3 commits into
Conversation
WalkthroughThis PR adds a ChangesReconcile-Story Feature
Sequence DiagramsequenceDiagram
participant User
participant ReconcileCmd
participant GitScanner
participant MarkdownParser
participant FileWriter
User->>ReconcileCmd: invoke reconcile-story --repo --story [--write]
ReconcileCmd->>GitScanner: get changed files from git status
GitScanner-->>ReconcileCmd: sorted paths (filtered)
ReconcileCmd->>FileWriter: read story markdown file
FileWriter-->>ReconcileCmd: file content
ReconcileCmd->>MarkdownParser: locate ### File List bounds
MarkdownParser-->>ReconcileCmd: section start/end
ReconcileCmd->>MarkdownParser: reconcile bullet list to git files
MarkdownParser-->>ReconcileCmd: updated markdown content
alt --write flag
ReconcileCmd->>FileWriter: write updated file
FileWriter-->>ReconcileCmd: wrote=true
else no --write
ReconcileCmd-->>User: wrote=false, in_sync=false
end
ReconcileCmd-->>User: JSON response with git_files, missing_from_story, stale_in_story
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
🤖 Augment PR SummarySummary: Adds deterministic reconciliation of a story’s Changes:
Technical Notes: The command outputs a compact JSON payload (e.g., 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
skills/bmad-story-automator/src/story_automator/commands/basic.py (2)
223-242: 💤 Low valueConsider adding defensive validation for git output format.
While git porcelain format is well-defined (2 status chars + space + path), adding a length check before
line[3:]would make the code more robust against unexpected git output or future format changes.🛡️ Optional defensive check
for line in status.output.splitlines(): if not line.strip(): continue + if len(line) < 3: + continue path = line[3:] # strip the 2-char XY status code + separator space🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/src/story_automator/commands/basic.py` around lines 223 - 242, In _git_changed_files, add defensive validation of each status.output line before slicing with line[3:]: check the line length (e.g., at least 3 characters) and that the third character is a space (or otherwise validate the two-char XY status + separator) and skip/log any malformed lines instead of slicing; ensure the rest of the logic (handling " -> ", stripping quotes, and filtering with FILE_LIST_EXCLUDE_PREFIXES) only runs on validated lines and preserve returning sorted(files) or None on run_cmd failure.
245-258: 💤 Low valuePotential edge case: indented headings in code blocks.
The
.lstrip()on line 255 could match indented content in code blocks that starts with#. While markdown headings typically start at column 0, a code block with an indented line like# Commentwould match after lstrip.This is unlikely to cause issues in practice since the story markdown format is controlled, but a more strict check (
lines[idx].startswith("#")without lstrip) would be more robust.♻️ More strict heading detection
end = len(lines) for idx in range(start + 1, len(lines)): - if lines[idx].lstrip().startswith("#"): + if lines[idx].startswith("#"): end = idx break return start, end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/bmad-story-automator/src/story_automator/commands/basic.py` around lines 245 - 258, The heading-detection in _file_list_bounds currently uses lines[idx].lstrip().startswith("#") which will match indented lines (e.g., inside code blocks); change this to check the raw line start (lines[idx].startswith("#")) so only column-0 headings terminate the file-list block; update the condition in _file_list_bounds accordingly and ensure any unit tests or callers expecting the previous behavior still pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@skills/bmad-story-automator/src/story_automator/commands/basic.py`:
- Around line 223-242: In _git_changed_files, add defensive validation of each
status.output line before slicing with line[3:]: check the line length (e.g., at
least 3 characters) and that the third character is a space (or otherwise
validate the two-char XY status + separator) and skip/log any malformed lines
instead of slicing; ensure the rest of the logic (handling " -> ", stripping
quotes, and filtering with FILE_LIST_EXCLUDE_PREFIXES) only runs on validated
lines and preserve returning sorted(files) or None on run_cmd failure.
- Around line 245-258: The heading-detection in _file_list_bounds currently uses
lines[idx].lstrip().startswith("#") which will match indented lines (e.g.,
inside code blocks); change this to check the raw line start
(lines[idx].startswith("#")) so only column-0 headings terminate the file-list
block; update the condition in _file_list_bounds accordingly and ensure any unit
tests or callers expecting the previous behavior still pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b6b41cf-e1e9-4f4d-b4bf-b044f6a63fd5
📒 Files selected for processing (5)
skills/bmad-story-automator-review/instructions.xmlskills/bmad-story-automator/src/story_automator/cli.pyskills/bmad-story-automator/src/story_automator/commands/basic.pyskills/bmad-story-automator/steps-c/step-03-execute.mdtests/test_reconcile_story.py
…usion in reconcile-story
|
Thanks — addressed all review feedback in 38aeb16.
|
|
Thanks @coderabbitai — both nitpicks were already resolved in
No further changes needed here. |
|
Tip For best results, initiate chat on the files or code changes.
Appreciate the thorough summary. 🐇 |
…close The reconcile gate was wired into step-03-execute (right after dev-story, before the `auto` step). But the automator runs `auto` (qa-generate-e2e-tests) AFTER dev and BEFORE review, and that step adds test files — so a File List reconciled at dev-close goes stale the moment `auto` runs, and the committed story record drifts again. Observed on a live Epic 4 build-cycle run: the dev-close gate held for the first story but the record drifted on later ones once `auto` added tests. Move the reconcile to step-03b-execute-finish, immediately before commit-story — the last point the File List can change (after `auto` and the review loop). Revert the review instructions comment: review now runs before the gate, so it is the drift detector again, not defence-in-depth. Companion PR (test counts) gets the same done-close treatment.
|
Repositioned the reconcile gate from Why: the automator runs Done-close (after Companion PR #42 (test counts) gets the same done-close treatment — rebased onto this and wired next to the File List reconcile. Surfaced as action item AI-4.1 in a downstream retrospective; happy to share the write-up if useful. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_reconcile_story.py`:
- Line 144: The test assertion in tests/test_reconcile_story.py uses a hardcoded
forward-slash string check
(payload["story_file"].endswith("docs/bmad/implementation-artifacts/1-2-example.md"))
which fails on Windows; replace it with a Path-based, platform-independent check
such as using
pathlib.Path(payload["story_file"]).match("**/docs/bmad/implementation-artifacts/1-2-example.md")
or converting to a POSIX path with
pathlib.Path(payload["story_file"]).as_posix().endswith("docs/bmad/implementation-artifacts/1-2-example.md")
so the comparison works across OSes and update imports if needed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82f4af73-441d-49f3-957f-4c61e04c4d70
📒 Files selected for processing (3)
skills/bmad-story-automator/src/story_automator/commands/basic.pyskills/bmad-story-automator/steps-c/step-03b-execute-finish.mdtests/test_reconcile_story.py
|
|
||
| payload = self._run("--write") | ||
| self.assertEqual(payload["git_files"], ["src/a.py"]) | ||
| self.assertTrue(payload["story_file"].endswith("docs/bmad/implementation-artifacts/1-2-example.md")) |
There was a problem hiding this comment.
Platform portability: path separator assumption.
The assertion uses a hardcoded forward-slash path, but str(Path(...)) on Windows produces backslashes. This test would fail on Windows.
🔧 Proposed fix for cross-platform compatibility
- self.assertTrue(payload["story_file"].endswith("docs/bmad/implementation-artifacts/1-2-example.md"))
+ # Normalize separators for cross-platform comparison
+ story_file_normalized = payload["story_file"].replace("\\", "/")
+ self.assertTrue(story_file_normalized.endswith("docs/bmad/implementation-artifacts/1-2-example.md"))Alternatively, use Path-based matching:
- self.assertTrue(payload["story_file"].endswith("docs/bmad/implementation-artifacts/1-2-example.md"))
+ self.assertTrue(Path(payload["story_file"]).match("*/docs/bmad/implementation-artifacts/1-2-example.md"))📝 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.
| self.assertTrue(payload["story_file"].endswith("docs/bmad/implementation-artifacts/1-2-example.md")) | |
| # Normalize separators for cross-platform comparison | |
| story_file_normalized = payload["story_file"].replace("\\", "/") | |
| self.assertTrue(story_file_normalized.endswith("docs/bmad/implementation-artifacts/1-2-example.md")) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_reconcile_story.py` at line 144, The test assertion in
tests/test_reconcile_story.py uses a hardcoded forward-slash string check
(payload["story_file"].endswith("docs/bmad/implementation-artifacts/1-2-example.md"))
which fails on Windows; replace it with a Path-based, platform-independent check
such as using
pathlib.Path(payload["story_file"]).match("**/docs/bmad/implementation-artifacts/1-2-example.md")
or converting to a POSIX path with
pathlib.Path(payload["story_file"]).as_posix().endswith("docs/bmad/implementation-artifacts/1-2-example.md")
so the comparison works across OSes and update imports if needed.
bma-d
left a comment
bma-d
left a comment
There was a problem hiding this comment.
Requesting changes for the commit path below. There is also an existing open thread on tests/test_reconcile_story.py:144 about path separator portability that should remain unresolved until fixed.
| echo "- **[$(date -u +%Y-%m-%dT%H:%M:%SZ)]** WARNING: File List reconcile failed: $(printf '%s' "$reconcile" | jq -c '.error // .')" >> "{outputFile}" | ||
| fi | ||
|
|
||
| commit=$("{scriptsDir}" commit-story --repo "{project-root}" --story {story_id} --title "{title}") |
There was a problem hiding this comment.
This currently treats reconcile failure as a warning and still runs commit-story immediately afterward. That means the story record can be committed even when the final File List sync failed, which undercuts this block's stated purpose of syncing immediately before the commit snapshot. Suggested fix: make reconcile success a prerequisite for commit-story; on failure, log the warning and stop or escalate before this command runs.
fix(story-automator): address maintainer review on bmad-code-org#38/bmad-code-org#39/bmad-code-org#41/bmad-code-org#42
3 participants
What
Add a
reconcile-storysubcommand, run at the end of the dev step (--write)before status flips to review, so the story File List is computed from git
instead of transcribed by the LLM.
Why
Doc-drift is the #1 recurring MEDIUM review finding across three epics: the dev
step writes the File List from memory, while the ground truth (
git status) isonly checked at review — one step too late. Three "be more careful" guidelines
(AI-1 → AI-2.1 → AI-3.1) failed because none changed who computes the value.
Fix
reconcile-story --repo --story [--write]: diffsgit status --porcelain --untracked-files=all -z(NUL-separated, verbatim paths) against the story### File List;--writerewrites that section deterministically.cmd_commit_story) and the resolved story key(
normalize_story_key+implementation_artifacts_dir).(
_bmad-output/or a configureddocs/bmad/...) plus agent/IDE config;.gitignore already filters deps/build junk.
step-03-execute.md(checks reconcile succeeded before logging); thereview git cross-check becomes defence-in-depth.
Tests
tests/test_reconcile_story.py— 10 cases (drift detection, idempotent write,tooling/artifact exclusion incl. configured
docs/bmad/..., missing-sectionappend, rename→destination, non-ASCII paths, parenthesised filenames, key
disambiguation, last-section EOF idempotency).
npm run verifygreen (421 tests).Scope
File List only. Test-count drift is the other half, tracked in #40.
Summary by CodeRabbit
New Features
reconcile-storycommand to detect and resolve differences between story file lists and actual git changes, with optional--writeflag to update files.Tests
Chores