fix(sprint-compare): resolve descriptive slug keys via existing resolver#36
fix(sprint-compare): resolve descriptive slug keys via existing resolver#36Pawel-N-pl wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR extracts sprint-status parsing into ChangesSprint Status Completion Detection
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 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: Fixes 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| return match.group(1).strip() == "done" | ||
| norm = normalize_story_key(project_root, story_id) | ||
| target = norm.id if norm is not None else story_id | ||
| for key, status in _status_rows(content): |
There was a problem hiding this comment.
sprint_status_done_in_text returns on the first _status_rows() match; if sprint-status.yaml contains both a prefix key (e.g. 1-1:) and a more specific slug key (e.g. 1-1-...:), the answer can depend on file order and diverge from _best_status_match behavior used by sprint_status_get.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Good catch — fixed in 03332c2. sprint_status_done_in_text now delegates to a shared _status_from_content (also used by sprint_status_get), so resolution goes through _best_status_match ranking: a descriptive slug key wins deterministically over a bare prefix key regardless of file order. Added a regression test asserting that precedence in both orderings.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
skills/bmad-story-automator/src/story_automator/commands/state.py (1)
196-196: 💤 Low valueConsider passing
project_rootfor full epic-based ID support.The current code works correctly for numeric story IDs (e.g.,
1.1,1-2), which is the typical use case. However, for epic-based story IDs (e.g.,epic-foo.1,epic-bar-2-description), the normalization logic insprint_status_done_in_textmay need the project root to resolve keys correctly.Since
get_project_root()is already imported, you could pass it to ensure full compatibility:♻️ Proposed enhancement
+project_root = get_project_root() -incomplete = [sid for sid in before if not sprint_status_done_in_text(sprint_text, sid)] +incomplete = [sid for sid in before if not sprint_status_done_in_text(sprint_text, sid, project_root)]🤖 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/state.py` at line 196, The list comprehension building incomplete uses sprint_status_done_in_text(sprint_text, sid) but doesn’t pass project_root, which can break epic-based IDs; update the call in the incomplete assignment to pass get_project_root() (or a resolved project_root variable) so sprint_status_done_in_text(sprint_text, sid, project_root) can normalize epic-based keys correctly; ensure the function signature of sprint_status_done_in_text supports the extra param where defined and import/get_project_root() is used consistently.
🤖 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/state.py`:
- Line 196: The list comprehension building incomplete uses
sprint_status_done_in_text(sprint_text, sid) but doesn’t pass project_root,
which can break epic-based IDs; update the call in the incomplete assignment to
pass get_project_root() (or a resolved project_root variable) so
sprint_status_done_in_text(sprint_text, sid, project_root) can normalize
epic-based keys correctly; ensure the function signature of
sprint_status_done_in_text supports the extra param where defined and
import/get_project_root() is used consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a65e1b12-db7a-44e1-b6eb-05fd4a0e83ad
📒 Files selected for processing (3)
skills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/core/sprint.pytests/test_sprint_compare.py
…olver Address review feedback on #36: - Extract `_status_from_content` shared by `sprint_status_get` and `sprint_status_done_in_text`, so completion checks use `_best_status_match` ranking (descriptive slug wins over a bare prefix key) instead of returning on the first row match -- removes order-dependence (augment). - `cmd_sprint_compare` passes `get_project_root()` so non-numeric epic IDs resolve through the full normalizer (coderabbit). - Add regression test asserting slug-over-prefix precedence regardless of file order.
|
Thanks for the reviews — both points addressed in 03332c2:
|
…onvention The package is effectively docstring-free (core/ and commands/ carry ~0%); keep only a terse inline "why" comment on the public text-based variant, consistent with the existing comment style. No behavior change.
|
@coderabbitai review |
✅ Action performedReview finished.
|
bma-d
left a comment
bma-d
left a comment
There was a problem hiding this comment.
Thanks for the work here. I'm requesting changes on the sprint-compare blocker below because an explicit sprint-status file can be misread when the process root is absent or points at a different project.
| match = re.search(rf"(?m)^\s*{re.escape(story_id)}:\s*(\S+)", sprint_text) | ||
| if not match or match.group(1) != "done": | ||
| incomplete.append(story_id) | ||
| project_root = get_project_root() |
There was a problem hiding this comment.
cmd_sprint_compare accepts explicit --state and --sprint paths, but this now feeds the ambient project root into the text-backed resolver. When that root is absent or points at a different project, a child row like release-3-phase-2-1-title: done can satisfy predecessor release.3, so the predecessor gate reports no incomplete story.
Repro:
tmp=$(mktemp -d)
cat > "$tmp/orchestration.md" <<'EOF'
---
storyRange: ["release.3", "release-3-phase-2.1"]
currentStory: release-3-phase-2.1
---
EOF
cat > "$tmp/sprint-status.yaml" <<'EOF'
development_status:
release-3-phase-2-1-title: done
EOF
env -u PROJECT_ROOT PYTHONDONTWRITEBYTECODE=1 PYTHONPATH=skills/bmad-story-automator/src python3 -m story_automator sprint-compare --state "$tmp/orchestration.md" --sprint "$tmp/sprint-status.yaml"Output:
{"ok":true,"incomplete":[],"checked":["release.3"]}Suggested fix: make the text-backed lookup disambiguate from the supplied sprint content, or derive/reject a project root tied to the explicit sprint path, so unmatched predecessors fail closed.
…olver Address review feedback on bmad-code-org#36: - Extract `_status_from_content` shared by `sprint_status_get` and `sprint_status_done_in_text`, so completion checks use `_best_status_match` ranking (descriptive slug wins over a bare prefix key) instead of returning on the first row match -- removes order-dependence (augment). - `cmd_sprint_compare` passes `get_project_root()` so non-numeric epic IDs resolve through the full normalizer (coderabbit). - Add regression test asserting slug-over-prefix precedence regardless of file order. (cherry picked from commit 03332c2)
3 participants
What
sprint-compareflagged already-donestories asincompleteon Resume(
step-01b-continue), firing a spurious Batch/Skip prompt.Why
cmd_sprint_comparematched dotted IDs (1.1) with a literal^\s*1.1:regex,but
sprint-status.yamluses descriptive slug keys (1-1-host-feasibility-probe:,the BMAD default). No line starts with
1.1:, so every "before currentStory"story was flagged — a pure false positive (no state corruption).
Fix
Route the check through the existing key resolver in
core/sprint.pyinstead of aduplicated, weaker regex. New
sprint_status_done_in_textreuses_exact_status_match/_status_rows/_status_key_matches_story— singlesource of truth. Completes the downstream-helper migration from #23, which fixed
sprint-status get,sprint_status_epic,check-epic-completeandget-epic-storiesbut did not touchcmd_sprint_compare.Tests
tests/test_sprint_compare.py— dotted/dashed/slug resolution, true negatives,and an end-to-end
cmd_sprint_compareregression (slug-keyed status + dottedrange ⇒
incomplete == []).Out of scope: an unrelated
derive-project-slugdegradation is tracked in #35.Summary by CodeRabbit
Improvements
Tests