fix: reconcile story and sprint status on closeout#2342
fix: reconcile story and sprint status on closeout#2342dickymoore wants to merge 6 commits intobmad-code-org:mainfrom
Conversation
🤖 Augment PR SummarySummary: This PR hardens closeout by requiring story markdown and sprint tracker statuses to reconcile before reporting completion. Changes:
Technical Notes: Drift validation resolves 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
dickymoore
force-pushed
the
fix/closeout-status-reconciliation
branch
from
8859270 to
de8b12d
Compare
📝 WalkthroughWalkthroughAdds a closeout reconciliation test and enforces cross-artifact status reconciliation: code-review now reopens and verifies story markdown Changes
Sequence DiagramsequenceDiagram
participant Runner as Orchestration/Runner
participant CodeReview as Code-Review Step
participant StoryFile as Story Markdown
participant Tracker as Sprint Tracker
participant SprintStatus as Sprint-Status Validator
Runner->>CodeReview: request closeout / set `{new_status}` to done
CodeReview->>StoryFile: save updated Status:
CodeReview->>StoryFile: reopen and read top-level Status:
StoryFile-->>CodeReview: return Status:
CodeReview->>Tracker: read development_status[{story_key}]
Tracker-->>CodeReview: return tracker status
alt statuses match
CodeReview->>Runner: record reconciliation OK, allow done
else drift detected
CodeReview->>Runner: HALT, record blocker, set story to in-progress
end
Note over SprintStatus,StoryFile: separate validation path
SprintStatus->>Tracker: inspect development_status entries
SprintStatus->>StoryFile: open via story_location and compare Status:
alt drift (tracker=review/done && story!=tracker)
SprintStatus-->>Runner: is_valid = false, return drift & reconciliation suggestion
else
SprintStatus-->>Runner: validation OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 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 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 |
dickymoore
force-pushed
the
fix/closeout-status-reconciliation
branch
from
de8b12d to
06013bd
Compare
|
augment review |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md (2)
99-104:⚠️ Potential issue | 🟠 MajorLive-gate blocker can be overwritten by later status logic.
After setting
{new_status} = in-progressfor failed/missing live-gate evidence, the next section can still set{new_status} = donebased only on finding resolution. This breaks the done-gate contract.Suggested logic guard
4. If the command fails or evidence is missing, the review cannot mark `done`; set `{new_status}` = `in-progress` and record the blocker in the story file. + Set `{live_gate_blocked}` = true. - - If all `decision-needed` and `patch` findings were resolved (fixed or dismissed) AND no unresolved HIGH/MEDIUM issues remain: set `{new_status}` = `done`. Update the story file Status section to `done`. + - If all `decision-needed` and `patch` findings were resolved (fixed or dismissed) AND no unresolved HIGH/MEDIUM issues remain AND `{live_gate_blocked}` is not true: set `{new_status}` = `done`. Update the story file Status section to `done`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md` around lines 99 - 104, The current status-setting flow can overwrite a prior live-gate failure (where `{new_status}` was set to `in-progress` for missing/failed live-gate evidence) when the "Determine new status based on review outcome" block later computes `{new_status}` from `decision-needed` and `patch` findings; change the logic so that once a live-gate failure flag is set (e.g., live-gate_failed or similar) you short-circuit the final status decision and keep `{new_status}` = `in-progress` regardless of resolved findings, and ensure the story file Status section is updated accordingly; locate the guard around the "If all `decision-needed` and `patch` findings..." logic and add a check for the live-gate failure flag before allowing `{new_status}` to be set to `done`.
125-139:⚠️ Potential issue | 🟠 MajorReconciliation can report success even when tracker was never reconciled.
Tracker verification is conditional (“if
{sprint_status}exists and{story_key}was found”), but the completion summary always states story/tracker agreement. This can emit a false “reconciled” closeout.Suggested reconciliation contract fix
-If `{sprint_status}` exists and `{story_key}` was found, re-open `{sprint_status}` after saving and verify `development_status[{story_key}]` also equals `{new_status}`. +If `{sprint_status}` exists and `{story_key}` was found, re-open `{sprint_status}` after saving and verify `development_status[{story_key}]` also equals `{new_status}`. +If `{new_status}` is `done` and either `{sprint_status}` is missing or `{story_key}` was not found, HALT with closeout reconciliation failure. - > **Reconciled:** story markdown and sprint tracker agree on `{new_status}` + > **Reconciled:** {{reconciliation_result}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md` around lines 125 - 139, The completion summary can falsely claim "Reconciled" because the verification of `{sprint_status}` vs `development_status[{story_key}]` is conditional; change the logic to set an explicit reconciled boolean only when the verification runs and both artifacts exist and match `{new_status}`, and use that boolean to drive the Completion summary text (or else report "Not reconciled" / "Tracker missing" accordingly). Locate the block that checks `if {sprint_status} exists and {story_key} was found` and the code that emits the Completion summary, introduce a reconciled flag set to true only after confirming `development_status[{story_key}] == {new_status}` and `{sprint_status} == {new_status}`, and update the summary wording to reflect the flag instead of always saying "reconciled".
🤖 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-skills/4-implementation/bmad-code-review/steps/step-04-present.md`:
- Around line 65-70: Update the descriptive sentence to match the actual
rendered options under the "How would you like to handle the <Z> `patch`
findings?" prompt: change "If `{spec_file}` is **not** set, present only options
1 and 3 (omit option 2 — findings were not written to a file)." to indicate the
correct option numbers shown (0 and 1), or alternatively renumber the displayed
list to match "1 and 3"; ensure consistency between the sentence and the list so
the branch that checks `{spec_file}` uses the same option labels as the
user-facing prompt.
In `@src/bmm-skills/4-implementation/bmad-sprint-status/SKILL.md`:
- Around line 293-299: The drift validation for development_status uses
story_location but doesn't define path resolution or failure behavior; update
the validation logic (the code that iterates development_status and reads each
story_location) to resolve relative paths against the repository root (or a
configurable base_dir) and to treat a missing story file or a missing top-level
"Status:" field as a validation failure (add an error entry to drift_entries and
set is_valid = false). Ensure the routine that compares the story's Status: to
the tracker status (for statuses "review" or "done") propagates file I/O and
parsing errors into the same drift_entries output with a clear message and
suggestion to reconcile sprint-status.yaml so validate mode cannot silently pass
on missing artifacts.
---
Outside diff comments:
In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md`:
- Around line 99-104: The current status-setting flow can overwrite a prior
live-gate failure (where `{new_status}` was set to `in-progress` for
missing/failed live-gate evidence) when the "Determine new status based on
review outcome" block later computes `{new_status}` from `decision-needed` and
`patch` findings; change the logic so that once a live-gate failure flag is set
(e.g., live-gate_failed or similar) you short-circuit the final status decision
and keep `{new_status}` = `in-progress` regardless of resolved findings, and
ensure the story file Status section is updated accordingly; locate the guard
around the "If all `decision-needed` and `patch` findings..." logic and add a
check for the live-gate failure flag before allowing `{new_status}` to be set to
`done`.
- Around line 125-139: The completion summary can falsely claim "Reconciled"
because the verification of `{sprint_status}` vs
`development_status[{story_key}]` is conditional; change the logic to set an
explicit reconciled boolean only when the verification runs and both artifacts
exist and match `{new_status}`, and use that boolean to drive the Completion
summary text (or else report "Not reconciled" / "Tracker missing" accordingly).
Locate the block that checks `if {sprint_status} exists and {story_key} was
found` and the code that emits the Completion summary, introduce a reconciled
flag set to true only after confirming `development_status[{story_key}] ==
{new_status}` and `{sprint_status} == {new_status}`, and update the summary
wording to reflect the flag instead of always saying "reconciled".
🪄 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: c882ec42-e28f-4a41-885a-a329d12d8f5a
📒 Files selected for processing (4)
package.jsonsrc/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.mdsrc/bmm-skills/4-implementation/bmad-sprint-status/SKILL.mdtest/test-closeout-reconciliation.js
dickymoore
force-pushed
the
fix/closeout-status-reconciliation
branch
from
06013bd to
999fbea
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md (2)
131-138:⚠️ Potential issue | 🟡 MinorMake reconciliation summary conditional on actual tracker verification.
Line 138 always states story/tracker agreement, but earlier flow allows skipping tracker sync (e.g., missing
{story_key}or{sprint_status}). That can produce a false completion claim.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md` around lines 131 - 138, The "Reconciled: story markdown and sprint tracker agree on `{new_status}`" line is always emitted even when tracker sync was skipped; change the template to only render that reconciled sentence when tracker verification actually ran and succeeded (e.g., check a boolean like `tracker_verified` or presence of `{story_key}` and `{sprint_status}`), otherwise omit or replace with a conditional note (e.g., "Tracker verification skipped" or nothing); update the template/renderer that emits the `Reconciled` line and use the variables `{new_status}`, `{story_key}`, and `{sprint_status}` to determine the conditional.
92-105:⚠️ Potential issue | 🟠 MajorPrevent
{new_status}=donefrom overriding unresolved live-gate blockers.Lines 96-100 correctly force
in-progressfor missing/failed live-gate evidence, but lines 103-104 can still set{new_status}=donewithout explicitly checking live-gate clearance. Add an explicit “live gates cleared (or valid unexpired deferral)” condition to the done path to avoid contradictory outcomes.Suggested guard in status decision
-- If all `decision-needed` and `patch` findings were resolved (fixed or dismissed) AND no unresolved HIGH/MEDIUM issues remain: set `{new_status}` = `done`. Update the story file Status section to `done`. +- If all `decision-needed` and `patch` findings were resolved (fixed or dismissed) AND no unresolved HIGH/MEDIUM issues remain AND live-gate blockers are cleared (or have complete, unexpired product-owner deferral): set `{new_status}` = `done`. Update the story file Status section to `done`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md` around lines 92 - 105, The done-path needs an explicit live-gates clearance check so we don't set {new_status} = done while unresolved live-gate blockers exist; update the "Determine new status based on review outcome" logic to require "live gates cleared (or valid unexpired product-owner deferral)" in addition to resolved decision-needed/patch findings and no unresolved HIGH/MEDIUM issues before setting {new_status} = done, and ensure the earlier live-gate failure branch (the "Live journey release-gate closeout" checks that set {new_status} = in-progress) is treated as a blocking condition in that decision so the two paths cannot contradict each other.
♻️ Duplicate comments (2)
src/bmm-skills/4-implementation/bmad-sprint-status/SKILL.md (1)
293-299:⚠️ Potential issue | 🟠 MajorDefine
story_locationresolution and missing-artifact failure behavior in validate mode.Line 293 says to use
story_location, but it still doesn’t define how relative paths are resolved or how missing/unreadable story files (or missing top-levelStatus:) are handled. That leaves room for silent pass conditions invalidatemode.Suggested text tightening
-<action>For each story entry in development_status, use `story_location` to open the matching story markdown file and compare its top-level `Status:` value with the tracker status when the tracker status is `review` or `done`</action> +<action>For each story entry in development_status where tracker status is `review` or `done`: +- Resolve story path from `story_location` (`{project-root}` base for relative paths). +- Open the matching story markdown file and read top-level `Status:`. +- If file is missing/unreadable or top-level `Status:` is missing, record a drift entry. +- Compare markdown `Status:` with tracker status and record mismatches as drift entries.</action>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-sprint-status/SKILL.md` around lines 293 - 299, The docs/instruction for processing development_status entries lacks a clear definition for resolving story_location and for failure behavior when story files or top-level Status are missing during validate mode; update the spec so that the routine that iterates development_status (use the symbol development_status) resolves story_location as relative to the repository root unless an absolute path is provided, attempts to open the resolved path and treats unreadable/missing files or absent top-level "Status:" as validation errors (not silent passes), and in validate mode return is_valid = false plus an explicit error entry for each missing/unreadable file or missing Status (include the story_location and reason) so drift detection (the check comparing markdown Status with tracker status for review/done) cannot silently skip entries.src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md (1)
65-70:⚠️ Potential issue | 🟡 MinorFix option-number wording in the no-
{spec_file}branch.Line 65 says “present only options 1 and 3,” but the rendered list here is options 1 and 2 (plus optional 0). This can misroute user input.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md` around lines 65 - 70, Update the explanatory sentence that controls the no-{spec_file} branch so the option numbers match the rendered list: change "present only options 1 and 3 (omit option 2 — findings were not written to a file)" to "present only options 1 and 2 (omit option 3 — findings were not written to a file)" and verify the prompt titled "How would you like to handle the <Z> `patch` findings?" plus the numbered options (0, 1, 2) remain consistent with that wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md`:
- Around line 131-138: The "Reconciled: story markdown and sprint tracker agree
on `{new_status}`" line is always emitted even when tracker sync was skipped;
change the template to only render that reconciled sentence when tracker
verification actually ran and succeeded (e.g., check a boolean like
`tracker_verified` or presence of `{story_key}` and `{sprint_status}`),
otherwise omit or replace with a conditional note (e.g., "Tracker verification
skipped" or nothing); update the template/renderer that emits the `Reconciled`
line and use the variables `{new_status}`, `{story_key}`, and `{sprint_status}`
to determine the conditional.
- Around line 92-105: The done-path needs an explicit live-gates clearance check
so we don't set {new_status} = done while unresolved live-gate blockers exist;
update the "Determine new status based on review outcome" logic to require "live
gates cleared (or valid unexpired product-owner deferral)" in addition to
resolved decision-needed/patch findings and no unresolved HIGH/MEDIUM issues
before setting {new_status} = done, and ensure the earlier live-gate failure
branch (the "Live journey release-gate closeout" checks that set {new_status} =
in-progress) is treated as a blocking condition in that decision so the two
paths cannot contradict each other.
---
Duplicate comments:
In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md`:
- Around line 65-70: Update the explanatory sentence that controls the
no-{spec_file} branch so the option numbers match the rendered list: change
"present only options 1 and 3 (omit option 2 — findings were not written to a
file)" to "present only options 1 and 2 (omit option 3 — findings were not
written to a file)" and verify the prompt titled "How would you like to handle
the <Z> `patch` findings?" plus the numbered options (0, 1, 2) remain consistent
with that wording.
In `@src/bmm-skills/4-implementation/bmad-sprint-status/SKILL.md`:
- Around line 293-299: The docs/instruction for processing development_status
entries lacks a clear definition for resolving story_location and for failure
behavior when story files or top-level Status are missing during validate mode;
update the spec so that the routine that iterates development_status (use the
symbol development_status) resolves story_location as relative to the repository
root unless an absolute path is provided, attempts to open the resolved path and
treats unreadable/missing files or absent top-level "Status:" as validation
errors (not silent passes), and in validate mode return is_valid = false plus an
explicit error entry for each missing/unreadable file or missing Status (include
the story_location and reason) so drift detection (the check comparing markdown
Status with tracker status for review/done) cannot silently skip entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56da33be-6d41-4357-843b-6c2c70de0ba9
📒 Files selected for processing (4)
package.jsonsrc/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.mdsrc/bmm-skills/4-implementation/bmad-sprint-status/SKILL.mdtest/test-closeout-reconciliation.js
🚧 Files skipped from review as they are similar to previous changes (2)
- test/test-closeout-reconciliation.js
- package.json
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@augmentcode review |
| #### Determine new status based on review outcome | ||
|
|
||
| - If all `decision-needed` and `patch` findings were resolved (fixed or dismissed) AND no unresolved HIGH/MEDIUM issues remain: set `{new_status}` = `done`. Update the story file Status section to `done`. | ||
| - If the live journey release-gate closeout above found missing evidence, red gates, skipped gates, blocked gates, environment-blocked gates, or incomplete/expired product-owner deferrals: keep `{new_status}` = `in-progress` regardless of resolved findings. Update the story file Status section to `in-progress` and record the blocker in the story file. |
There was a problem hiding this comment.
src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md:89: The first rule treats any "skipped gates" as an automatic blocker, but the next rule allows completing when blockers have a complete/unexpired product-owner deferral; it’s unclear whether deferred skips are still considered “skipped gates.” This ambiguity could cause {new_status} to stay in-progress even when a valid deferral exists (or vice versa).
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| - IF any story has status "review": suggest `/bmad:bmm:workflows:code-review` | ||
| - IF any story has status "in-progress" AND no stories have status "ready-for-dev": recommend staying focused on active story | ||
| - IF any story markdown file top-level `Status:` does not match its `development_status` entry: warn "story/tracker status drift detected" |
There was a problem hiding this comment.
src/bmm-skills/4-implementation/bmad-sprint-status/SKILL.md:151: This risk rule says to warn on any story/tracker drift, but validate-mode enforcement below only checks review/done stories. Consider clarifying the intended scope so users don’t assume validate will fail for drift on backlog/in-progress stories.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| <action>For each story entry in development_status where the tracker status is `review` or `done`:</action> | ||
|
|
||
| - Resolve the story path from `story_location` using `{project-root}` as the base for relative paths. |
There was a problem hiding this comment.
src/bmm-skills/4-implementation/bmad-sprint-status/SKILL.md:295: Path resolution is specified for relative story_location values, but it’s not explicit what to do if story_location (or the resolved story path) is already absolute. Clarifying that absolute paths should be used as-is would help avoid accidentally constructing invalid paths during validate mode.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md (1)
114-116: Reduce repeated leading “If” sentences to satisfy style/lint.Static analysis flagged line ~115 for “Three successive sentences begin with the same word.” You can rephrase one or two sentences in the reconciliation block (e.g., replace one “If” with “Once …” / “Otherwise …” / “Next …”) without changing behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md` around lines 114 - 116, The reconciliation block repeats leading "If" three times; rephrase one or two sentences to avoid three successive sentences starting with "If" without changing logic: locate the paragraph that checks {sprint_status}, {story_key}, development_status[{story_key}] and {new_status} and adjust wording (for example start the second sentence with "Once" or "Next" and the final check with "Otherwise") while preserving the behavior that when {sprint_status} exists and {story_key} was found you re-open {sprint_status}, save and verify development_status[{story_key}] == {new_status} (then set {reconciliation_result} = "story markdown and sprint tracker agree on {new_status}"); if the story file doesn't match {new_status} or the verified sprint status doesn't match, HALT with a closeout reconciliation failure.
🤖 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-skills/4-implementation/bmad-code-review/steps/step-04-present.md`:
- Around line 108-117: The final reconciliation must not treat a missing or
unreadable development_status entry as "tracker verification skipped"; update
the condition that currently reads "If `{sprint_status}` exists and
`{story_key}` was found" to explicitly require that `{sprint_status}` contains a
`development_status[{story_key}]` entry before attempting comparison; if that
entry is absent or unreadable, immediately HALT with a closeout reconciliation
failure and do not set `{reconciliation_result}` to a skipped state; otherwise
verify `development_status[{story_key}]` equals `{new_status}` and set
`{reconciliation_result}` to the agreement message only on match.
---
Nitpick comments:
In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md`:
- Around line 114-116: The reconciliation block repeats leading "If" three
times; rephrase one or two sentences to avoid three successive sentences
starting with "If" without changing logic: locate the paragraph that checks
{sprint_status}, {story_key}, development_status[{story_key}] and {new_status}
and adjust wording (for example start the second sentence with "Once" or "Next"
and the final check with "Otherwise") while preserving the behavior that when
{sprint_status} exists and {story_key} was found you re-open {sprint_status},
save and verify development_status[{story_key}] == {new_status} (then set
{reconciliation_result} = "story markdown and sprint tracker agree on
{new_status}"); if the story file doesn't match {new_status} or the verified
sprint status doesn't match, HALT with a closeout reconciliation failure.
🪄 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: bedbe8b7-f010-4f73-ab95-0895778a5530
📒 Files selected for processing (4)
package.jsonsrc/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.mdsrc/bmm-skills/4-implementation/bmad-sprint-status/SKILL.mdtest/test-closeout-reconciliation.js
✅ Files skipped from review due to trivial changes (1)
- test/test-closeout-reconciliation.js
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/bmm-skills/4-implementation/bmad-sprint-status/SKILL.md
| #### Final reconciliation | ||
|
|
||
| Re-open the story file after saving and verify the top-level `Status:` field equals `{new_status}`. | ||
|
|
||
| Set `{reconciliation_result}` = `story file verified; sprint tracker verification skipped`. | ||
|
|
||
| If `{sprint_status}` exists and `{story_key}` was found, re-open `{sprint_status}` after saving and verify `development_status[{story_key}]` also equals `{new_status}`. If it matches, set `{reconciliation_result}` = `story markdown and sprint tracker agree on {new_status}`. | ||
|
|
||
| If the story file does not match `{new_status}`, or if `{sprint_status}` was verified and `development_status[{story_key}]` does not match `{new_status}`, HALT with a closeout reconciliation failure instead of reporting completion. | ||
|
|
There was a problem hiding this comment.
Harden “Final reconciliation” guards for missing development_status[{story_key}].
Right now, the logic says “If {sprint_status} exists and {story_key} was found…” then verify development_status[{story_key}]. But it doesn’t explicitly cover the case where the story key is “found” in some sense during sync, yet the development_status entry is missing (or unreadable). In that scenario, reconciliation might become ambiguous (e.g., attempted access vs “mismatch”), and the {reconciliation_result} might incorrectly remain “tracker verification skipped”.
Suggestion: change the condition to something like:
- “If
{sprint_status}exists and it contains adevelopment_status[{story_key}]entry, verify it equals{new_status}; if the entry is missing/unreadable, HALT as a closeout reconciliation failure.”
This keeps the HALT-on-drift contract deterministic and prevents missing-key behavior from depending on engine interpretation.
🧰 Tools
🪛 LanguageTool
[style] ~115-~115: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...sprint tracker agree on {new_status}. If the story file does not match {new_sta...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md`
around lines 108 - 117, The final reconciliation must not treat a missing or
unreadable development_status entry as "tracker verification skipped"; update
the condition that currently reads "If `{sprint_status}` exists and
`{story_key}` was found" to explicitly require that `{sprint_status}` contains a
`development_status[{story_key}]` entry before attempting comparison; if that
entry is absent or unreadable, immediately HALT with a closeout reconciliation
failure and do not set `{reconciliation_result}` to a skipped state; otherwise
verify `development_status[{story_key}]` equals `{new_status}` and set
`{reconciliation_result}` to the agreement message only on match.
|
Hey again. I've revised this to align with your review direction and reduce coupling to the soon-to-be-replaced sprint-status path. What changed:
I didn't add replacement test coverage here because this is workflow/prompt behaviour; per your feedback, that should move through evals once the eval path lands. Broader reconciliation should fit the future deterministic story front-matter tracking work rather than adding more sprint-status surface area now. It's now a very tiny PR 😄 |
|
TBH, I'm closing this PR rather than pushing it further as I think the remaining change is too narrow to fully solve #2341, and the broader sprint-status-based reconciliation direction is no longer the right target given the planned move to deterministic story front-matter checks. |
2 participants
Summary
Status:persisted before the existing sprint-status sync continuesTesting
git diff --check upstream/mainnode tools/validate-skills.js --strictnpx markdownlint-cli2 "src/bmm-skills/4-implementation/bmad-code-review/steps/step-04-present.md"Refs #2341