Refactor TEA policy core integration#28
Open
dickymoore wants to merge 37 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (27)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (8)
WalkthroughAdds TEA workflow support: runtime-policy shape/snapshot handling, TEA run-policy builder and detector, state-document progress table generation and updates, orchestrator helper commands, CLI wiring, templates/docs updates, and many tests and test utilities. ChangesTEA Workflow Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
🤖 Augment PR SummarySummary: This PR refactors TEA (Test Engineering Acceleration) workflow support into reusable core modules and makes the orchestrator policy-driven. Changes:
Technical Notes: Standard workflows remain gated by the existing deterministic suite and smoke checks. TEA opt-in is policy-driven; the orchestrator never silently switches to TEA. The 🤖 Was this summary useful? React with 👍 or 👎 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
skills/bmad-story-automator/src/story_automator/commands/state.py (1)
42-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
configshape before accessing fields and building story ranges.Line 50 assumes
configis a dict, and Lines 62/174 assumestoryRangeis a list. With valid JSON that isn’t an object (or a stringstoryRange), this can crash or silently produce per-character story IDs instead of story keys.Suggested fix
try: config = json.loads(config_json) except json.JSONDecodeError: write_json({"ok": False, "error": "invalid_config_json"}) return 1 + if not isinstance(config, dict): + write_json({"ok": False, "error": "config_must_be_object"}) + return 1 + raw_story_range = config.get("storyRange", []) + if raw_story_range is not None and not isinstance(raw_story_range, list): + write_json({"ok": False, "error": "storyRange_must_be_array"}) + return 1 + story_range = [item for item in (raw_story_range or []) if isinstance(item, str)] @@ - progress_header, progress_divider, progress_rows = progress_table_lines(snapshot["policy"], [item for item in config.get("storyRange", []) if isinstance(item, str)]) + progress_header, progress_divider, progress_rows = progress_table_lines(snapshot["policy"], story_range) @@ - story_range = [item for item in config.get("storyRange", []) if isinstance(item, str)] + # reuse validated story_rangeAlso applies to: 62-63, 174-174
🤖 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` around lines 42 - 50, The code parses config_json into config but doesn't validate its shape; add checks after json.loads to ensure config is a dict (if not, write_json({"ok": False, "error": "invalid_config_shape"}) and return 1) and validate that config.get("storyRange") is a list before using it (otherwise return an error); update places that build story IDs/ranges to only iterate when storyRange is a list and use safe access for epic (keep using epic = str(config.get("epic") or "epic")) so non-object JSON or wrong types won't produce per-character iteration or crashes.skills/bmad-story-automator/steps-c/step-03b-execute-finish.md (1)
114-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve retro-policy gating in the final trigger decision.
Lines 114-117 set
trigger_retro=falsewhenretrois not in policy, but Lines 123-127 can overwrite it back totrue. This re-enables retrospective even when policy excludes it.Suggested fix
policy_sequence=$("{scriptsDir}" orchestrator-helper policy-sequence --state-file "{outputFile}") -if ! echo "$policy_sequence" | jq -e '.ok == true and (.sequence | index("retro"))' >/dev/null; then - trigger_retro=false -fi +policy_has_retro=false +if echo "$policy_sequence" | jq -e '.ok == true and (.sequence | index("retro"))' >/dev/null; then + policy_has_retro=true +fi @@ -if [ "$all_done" = "true" ] && [ "$epic_ok" = "true" ] && [ "$epic_complete" = "true" ]; then +if [ "$policy_has_retro" = "true" ] && [ "$all_done" = "true" ] && [ "$epic_ok" = "true" ] && [ "$epic_complete" = "true" ]; then trigger_retro=true else trigger_retro=false fi🤖 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/steps-c/step-03b-execute-finish.md` around lines 114 - 127, The current logic sets trigger_retro=false when the policy sequence lacks "retro" (policy_sequence) but then unconditionally sets it to true if story/epic checks pass; update the final gating so the policy check is honored: preserve the earlier result (trigger_retro or a new boolean like policy_allows_retro derived from policy_sequence) and include it in the combined conditional alongside all_done, epic_ok, and epic_complete (i.e., require policy_allows_retro to be true as well) so that retrospective is only triggered when both policy and story/epic checks permit it; adjust uses of trigger_retro, policy_sequence, epic_status, all_done, epic_ok, and epic_complete accordingly.
🧹 Nitpick comments (1)
tests/test_runtime_policy.py (1)
429-535: ⚡ Quick winUse shared TEA test helpers instead of duplicating fixture builders.
Lines 429–535 duplicate helper behavior already centralized in
tests/tea_test_support.py. Reusing the shared helpers will reduce drift and keep TEA policy fixtures consistent across suites.Refactor direction
+from tests.tea_test_support import install_tea_skills, tea_steps_override ... def _install_tea_skills(self) -> None: - _write_tea_assets(self.project_root) - for name in ( - "bmad-tea-testarch-atdd", - "bmad-tea-testarch-automate", - "bmad-tea-testarch-test-review", - "bmad-tea-testarch-trace", - ): - skill_dir = self.project_root / ".claude" / "skills" / name - skill_dir.mkdir(parents=True, exist_ok=True) - (skill_dir / "SKILL.md").write_text(f"# {name}\n", encoding="utf-8") - (skill_dir / "workflow.md").write_text(f"# {name}\n", encoding="utf-8") + install_tea_skills(self.project_root, include_nfr=False, canonical=False, write_assets=True) ... - steps = _tea_steps_override(self.project_root) + steps = tea_steps_override(include_nfr=False, canonical=False)🤖 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_runtime_policy.py` around lines 429 - 535, The duplicated TEA fixture builders (_install_tea_skills, _write_tea_assets, _tea_steps_override) should be replaced with the shared helpers from tests/tea_test_support.py: remove these local implementations and import the canonical helpers (e.g. the tea asset writer/installer and steps override factory) from tests.tea_test_support, then call those helper functions in tests instead of reimplementing them so the suite uses the centralized TEA fixtures (reference the local symbols _install_tea_skills, _write_tea_assets, and _tea_steps_override to find and remove the duplicated code).
🤖 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 `@skills/bmad-story-automator/src/story_automator/core/state_document.py`:
- Around line 145-152: The metrics parser currently removes empty cells (values
= [part for part in parts[1:-1] if part]) which shifts columns and then uses
substring matching to detect completion (any(token in values[-1].lower() ...))
causing false positives like "incomplete"; update the logic to preserve empty
cells by assigning values = parts[1:-1] (but still strip whitespace when
checking), detect header separator by checking re.fullmatch(r"-+",
values[0].strip()) on the preserved first_cell, increment total using the
correct last column value, and determine completion by comparing
values[-1].strip().lower() for exact equality against a set like
{"done","complete","completed"} (or membership test) before incrementing
completed.
In `@tests/test_orchestrator_progress.py`:
- Around line 139-142: Update the _md5_hex8 helper to mark MD5 usage as
non-security by calling hashlib.md5(..., usedforsecurity=False) so Ruff S324 is
satisfied; locate the function _md5_hex8 and modify the hashlib.md5 invocation
to pass usedforsecurity=False before encoding and slicing the hexdigest, keeping
the returned value as the first 8 hex chars.
In `@tests/test_runtime_policy.py`:
- Around line 89-101: The test
test_tea_policy_fails_when_required_tea_skill_missing is currently ambiguous
because failure could be due to missing TEA assets rather than the missing
skill; update the test to precreate the required TEA assets (use the same
helpers/setup used elsewhere in tests or create the minimal files under
self.project_root before calling _write_override) so the only remaining failure
is the missing skill, then call load_effective_policy(str(self.project_root))
inside an assertRaisesRegex or similar to check PolicyError and assert the error
message contains the expected text about the missing TEA skill; keep references
to _tea_steps_override, _write_override, load_effective_policy, and PolicyError
when locating where to change the test.
In `@tests/test_tea_state_rendering.py`:
- Around line 229-232: The test currently assumes the desired note is at
payload["notes"][0], which is order-coupled and brittle; update the assertion to
check that at least one element in payload["notes"] contains the substring
"checkpoint-preview is out of scope" (e.g., replace the index-based
self.assertIn on payload["notes"][0] with an existence check such as any(...) or
a loop/collection membership check over payload["notes"]). Ensure you modify the
assertion around the payload variable in tests/test_tea_state_rendering.py so it
scans the whole notes list rather than indexing element 0.
---
Outside diff comments:
In `@skills/bmad-story-automator/src/story_automator/commands/state.py`:
- Around line 42-50: The code parses config_json into config but doesn't
validate its shape; add checks after json.loads to ensure config is a dict (if
not, write_json({"ok": False, "error": "invalid_config_shape"}) and return 1)
and validate that config.get("storyRange") is a list before using it (otherwise
return an error); update places that build story IDs/ranges to only iterate when
storyRange is a list and use safe access for epic (keep using epic =
str(config.get("epic") or "epic")) so non-object JSON or wrong types won't
produce per-character iteration or crashes.
In `@skills/bmad-story-automator/steps-c/step-03b-execute-finish.md`:
- Around line 114-127: The current logic sets trigger_retro=false when the
policy sequence lacks "retro" (policy_sequence) but then unconditionally sets it
to true if story/epic checks pass; update the final gating so the policy check
is honored: preserve the earlier result (trigger_retro or a new boolean like
policy_allows_retro derived from policy_sequence) and include it in the combined
conditional alongside all_done, epic_ok, and epic_complete (i.e., require
policy_allows_retro to be true as well) so that retrospective is only triggered
when both policy and story/epic checks permit it; adjust uses of trigger_retro,
policy_sequence, epic_status, all_done, epic_ok, and epic_complete accordingly.
---
Nitpick comments:
In `@tests/test_runtime_policy.py`:
- Around line 429-535: The duplicated TEA fixture builders (_install_tea_skills,
_write_tea_assets, _tea_steps_override) should be replaced with the shared
helpers from tests/tea_test_support.py: remove these local implementations and
import the canonical helpers (e.g. the tea asset writer/installer and steps
override factory) from tests.tea_test_support, then call those helper functions
in tests instead of reimplementing them so the suite uses the centralized TEA
fixtures (reference the local symbols _install_tea_skills, _write_tea_assets,
and _tea_steps_override to find and remove the duplicated code).
🪄 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: 6639226b-58ba-4e14-95a0-5be4381f5776
📒 Files selected for processing (27)
skills/bmad-story-automator/data/orchestration-policy.jsonskills/bmad-story-automator/data/tea-story-automator/parse/tea_step.jsonskills/bmad-story-automator/data/tea-story-automator/prompts/tea_step.mdskills/bmad-story-automator/src/story_automator/cli.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/core/agent_config.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy.pyskills/bmad-story-automator/src/story_automator/core/state_document.pyskills/bmad-story-automator/src/story_automator/core/tea_policy.pyskills/bmad-story-automator/steps-c/step-01b-continue.mdskills/bmad-story-automator/steps-c/step-02a-preflight-config.mdskills/bmad-story-automator/steps-c/step-03-execute.mdskills/bmad-story-automator/steps-c/step-03a-execute-review.mdskills/bmad-story-automator/steps-c/step-03b-execute-finish.mdskills/bmad-story-automator/templates/state-document.mdskills/bmad-story-automator/workflow.mdtests/tea_test_support.pytests/test_agent_config_model.pytests/test_orchestrator_parse.pytests/test_orchestrator_progress.pytests/test_runtime_policy.pytests/test_state_policy_metadata.pytests/test_tea_detection.pytests/test_tea_policy_flow.pytests/test_tea_state_rendering.py
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 `@skills/bmad-story-automator/src/story_automator/commands/state.py`:
- Around line 50-55: The code currently silently filters non-string items from
raw_story_range; instead validate that config.get("storyRange") when present is
a list of strings: after fetching raw_story_range, if raw_story_range is not
None and not isinstance(raw_story_range, list) return the existing error, and if
it is a list but any(not isinstance(item, str) for item in raw_story_range) call
write_json({"ok": False, "error": "storyRange_must_be_array_of_strings"}) and
return 1; only then set story_range = raw_story_range (or list(raw_story_range))
and continue to ensure_dir(output_folder).
🪄 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: c58b7b79-1876-4653-b9fb-edd8ad11d8b4
📒 Files selected for processing (6)
skills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/core/state_document.pyskills/bmad-story-automator/steps-c/step-03b-execute-finish.mdtests/test_orchestrator_progress.pytests/test_runtime_policy.pytests/test_tea_state_rendering.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/test_orchestrator_progress.py
- skills/bmad-story-automator/steps-c/step-03b-execute-finish.md
- skills/bmad-story-automator/src/story_automator/core/state_document.py
- tests/test_tea_state_rendering.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/run-smoke-policy-invariants.py`:
- Line 24: The subprocess.run call that assigns to completed may hang; add a
timeout argument (e.g., timeout=300) to subprocess.run(cmd, text=True,
capture_output=True, cwd=repo_root, env=env) and wrap the call in a try/except
for subprocess.TimeoutExpired to handle timeouts gracefully (log or print a
clear message and set a non-zero exit/result). Update the code around the
completed assignment and the timeout handling should reference subprocess.run,
cmd, repo_root, env and handle subprocess.TimeoutExpired to avoid indefinitely
stalled CI/smoke jobs.
In `@scripts/run-smoke-resume-matrix.py`:
- Line 23: The subprocess.run call that assigns to completed currently blocks
indefinitely; add a timeout argument to subprocess.run (e.g., timeout=...) and
wrap the call in a try/except for subprocess.TimeoutExpired to handle hung runs;
on timeout, terminate/cleanup if needed, log an error including cmd and timeout
duration, and exit non-zero (or set appropriate return status) so the smoke
pipeline fails fast—update the call site where completed = subprocess.run(cmd,
text=True, capture_output=True, cwd=repo_root, env=env) and add the exception
handling around subprocess.run.
🪄 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: 986138ba-bf08-4dd3-8108-bdd3b1483a50
📒 Files selected for processing (16)
scripts/run-smoke-policy-invariants.pyscripts/run-smoke-resume-matrix.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy.pyskills/bmad-story-automator/src/story_automator/core/tea_policy.pyskills/bmad-story-automator/steps-c/step-03-execute.mdskills/bmad-story-automator/steps-c/step-03a-execute-review.mdskills/bmad-story-automator/steps-c/step-03b-execute-finish.mdtests/test_policy_invariants.pytests/test_progress_invariants.pytests/test_resume_matrix.pytests/test_runtime_policy.pytests/test_tea_detection.pytests/test_tea_policy_flow.pytests/test_tea_state_rendering.py
🚧 Files skipped from review as they are similar to previous changes (6)
- skills/bmad-story-automator/steps-c/step-03b-execute-finish.md
- skills/bmad-story-automator/src/story_automator/commands/state.py
- tests/test_runtime_policy.py
- skills/bmad-story-automator/steps-c/step-03-execute.md
- tests/test_tea_detection.py
- tests/test_tea_state_rendering.py
Contributor
Author
|
Addressed the latest review comments on this branch. Main fixes in this pass:
I also followed the direction of PR 32 in a bounded way by adding standalone local verification for the maintainer repro classes, without widening this PR into the broader deterministic smoke work:
Local verification:
|
Collaborator
|
@augmentcode review |
|
This pull request is abnormally large and would use a significant amount of tokens to review. If you still wish to review it, comment "augment review" and we will review it. |
Collaborator
|
augment review |
|
✅ Action performedReview finished.
|
|
Sorry, Augment failed to review this pull request. |
Collaborator
|
augment review |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
Collaborator
|
@augmentcode review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
skills/bmad-story-automator/src/story_automator/core/workflow_steps.py (1)
46-47: 💤 Low valueUse
TEA_OPTIONAL_STEPSconstant instead of hardcoded tuple.The function hardcodes
("nfr", "retro")whileTEA_OPTIONAL_STEPSis defined on line 40 with the same values. Using the constant avoids drift.♻️ Suggested fix
def selected_optional_steps_from_sequence(sequence: list[str]) -> list[str]: - return [step for step in ("nfr", "retro") if step in sequence] + return [step for step in TEA_OPTIONAL_STEPS if step in sequence]🤖 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/core/workflow_steps.py` around lines 46 - 47, The function selected_optional_steps_from_sequence hardcodes the tuple ("nfr", "retro") in its list comprehension instead of using the TEA_OPTIONAL_STEPS constant that is already defined on line 40 with the same values. Replace the hardcoded tuple with TEA_OPTIONAL_STEPS in the list comprehension to maintain a single source of truth and prevent drift if the constant is updated in the future.skills/bmad-story-automator/src/story_automator/core/tea_policy_assets.py (1)
145-156: 💤 Low valueDuplicated required-files list.
Lines 145-154 duplicate the required files list from
tea_assets_complete_for_base(lines 61-70). Consider extracting to a shared constant or helper to avoid drift if the list changes.🤖 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/core/tea_policy_assets.py` around lines 145 - 156, The required files list in this section (containing the prompts and parse files: atdd.md, test_automate.md, test_review.md, trace.md, and their corresponding .json files) duplicates an identical list that appears in the tea_assets_complete_for_base function. Extract this list into a module-level constant at the top of the file, then replace both occurrences (the one here and the one in tea_assets_complete_for_base) with references to this shared constant to prevent maintenance issues and drift if the list needs to change in the future.
🤖 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
`@skills/bmad-story-automator/src/story_automator/commands/orchestrator_state.py`:
- Around line 98-99: The argument parsing logic for the `--set` flag in the
condition checking `if args[idx] == "--set"` does not validate that the argument
contains an equals sign before splitting. Add a check to verify that `args[idx +
1]` contains the `=` character before calling split, and either raise a clear
error message or skip the invalid argument if the format is incorrect (missing
the equals sign or value).
In `@skills/bmad-story-automator/steps-c/step-01b-continue.md`:
- Line 71: The fenced code block containing the warning message about Stories
being incomplete in sprint-status.yaml is missing a language label, which
violates markdownlint MD040. Add the language label "text" immediately after the
opening three backticks (```) of this fenced code block to resolve the linting
issue.
---
Nitpick comments:
In `@skills/bmad-story-automator/src/story_automator/core/tea_policy_assets.py`:
- Around line 145-156: The required files list in this section (containing the
prompts and parse files: atdd.md, test_automate.md, test_review.md, trace.md,
and their corresponding .json files) duplicates an identical list that appears
in the tea_assets_complete_for_base function. Extract this list into a
module-level constant at the top of the file, then replace both occurrences (the
one here and the one in tea_assets_complete_for_base) with references to this
shared constant to prevent maintenance issues and drift if the list needs to
change in the future.
In `@skills/bmad-story-automator/src/story_automator/core/workflow_steps.py`:
- Around line 46-47: The function selected_optional_steps_from_sequence
hardcodes the tuple ("nfr", "retro") in its list comprehension instead of using
the TEA_OPTIONAL_STEPS constant that is already defined on line 40 with the same
values. Replace the hardcoded tuple with TEA_OPTIONAL_STEPS in the list
comprehension to maintain a single source of truth and prevent drift if the
constant is updated in the future.
🪄 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: 96059c74-083f-4b59-8db3-450f1971b381
📒 Files selected for processing (27)
scripts/run-smoke-policy-invariants.pyscripts/run-smoke-resume-matrix.pyskills/bmad-story-automator/data/orchestration-policy.jsonskills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_state.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/core/policy_state_rendering.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy_support.pyskills/bmad-story-automator/src/story_automator/core/tea_policy.pyskills/bmad-story-automator/src/story_automator/core/tea_policy_assets.pyskills/bmad-story-automator/src/story_automator/core/tea_policy_config.pyskills/bmad-story-automator/src/story_automator/core/workflow_steps.pyskills/bmad-story-automator/steps-c/step-01b-continue.mdskills/bmad-story-automator/steps-c/step-03-execute.mdskills/bmad-story-automator/steps-c/step-03a-execute-review.mdskills/bmad-story-automator/steps-c/step-03b-execute-finish.mdtests/test_orchestrator_parse.pytests/test_policy_invariants.pytests/test_progress_invariants.pytests/test_resume_matrix.pytests/test_runtime_policy.pytests/test_state_policy_metadata.pytests/test_tea_detection.pytests/test_tea_policy_flow.pytests/test_tea_state_rendering.py
💤 Files with no reviewable changes (1)
- skills/bmad-story-automator/data/orchestration-policy.json
🚧 Files skipped from review as they are similar to previous changes (8)
- skills/bmad-story-automator/steps-c/step-03b-execute-finish.md
- skills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.py
- skills/bmad-story-automator/steps-c/step-03-execute.md
- skills/bmad-story-automator/steps-c/step-03a-execute-review.md
- tests/test_runtime_policy.py
- tests/test_state_policy_metadata.py
- tests/test_tea_detection.py
- skills/bmad-story-automator/src/story_automator/commands/state.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (2)
skills/bmad-story-automator/src/story_automator/core/workflow_steps.py (1)
46-47: 💤 Low valueUse
TEA_OPTIONAL_STEPSconstant instead of hardcoded tuple.The function hardcodes
("nfr", "retro")whileTEA_OPTIONAL_STEPSis defined on line 40 with the same values. Using the constant avoids drift.♻️ Suggested fix
def selected_optional_steps_from_sequence(sequence: list[str]) -> list[str]: - return [step for step in ("nfr", "retro") if step in sequence] + return [step for step in TEA_OPTIONAL_STEPS if step in sequence]🤖 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/core/workflow_steps.py` around lines 46 - 47, The function selected_optional_steps_from_sequence hardcodes the tuple ("nfr", "retro") in its list comprehension instead of using the TEA_OPTIONAL_STEPS constant that is already defined on line 40 with the same values. Replace the hardcoded tuple with TEA_OPTIONAL_STEPS in the list comprehension to maintain a single source of truth and prevent drift if the constant is updated in the future.skills/bmad-story-automator/src/story_automator/core/tea_policy_assets.py (1)
145-156: 💤 Low valueDuplicated required-files list.
Lines 145-154 duplicate the required files list from
tea_assets_complete_for_base(lines 61-70). Consider extracting to a shared constant or helper to avoid drift if the list changes.🤖 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/core/tea_policy_assets.py` around lines 145 - 156, The required files list in this section (containing the prompts and parse files: atdd.md, test_automate.md, test_review.md, trace.md, and their corresponding .json files) duplicates an identical list that appears in the tea_assets_complete_for_base function. Extract this list into a module-level constant at the top of the file, then replace both occurrences (the one here and the one in tea_assets_complete_for_base) with references to this shared constant to prevent maintenance issues and drift if the list needs to change in the future.
🤖 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
`@skills/bmad-story-automator/src/story_automator/commands/orchestrator_state.py`:
- Around line 98-99: The argument parsing logic for the `--set` flag in the
condition checking `if args[idx] == "--set"` does not validate that the argument
contains an equals sign before splitting. Add a check to verify that `args[idx +
1]` contains the `=` character before calling split, and either raise a clear
error message or skip the invalid argument if the format is incorrect (missing
the equals sign or value).
In `@skills/bmad-story-automator/steps-c/step-01b-continue.md`:
- Line 71: The fenced code block containing the warning message about Stories
being incomplete in sprint-status.yaml is missing a language label, which
violates markdownlint MD040. Add the language label "text" immediately after the
opening three backticks (```) of this fenced code block to resolve the linting
issue.
---
Nitpick comments:
In `@skills/bmad-story-automator/src/story_automator/core/tea_policy_assets.py`:
- Around line 145-156: The required files list in this section (containing the
prompts and parse files: atdd.md, test_automate.md, test_review.md, trace.md,
and their corresponding .json files) duplicates an identical list that appears
in the tea_assets_complete_for_base function. Extract this list into a
module-level constant at the top of the file, then replace both occurrences (the
one here and the one in tea_assets_complete_for_base) with references to this
shared constant to prevent maintenance issues and drift if the list needs to
change in the future.
In `@skills/bmad-story-automator/src/story_automator/core/workflow_steps.py`:
- Around line 46-47: The function selected_optional_steps_from_sequence
hardcodes the tuple ("nfr", "retro") in its list comprehension instead of using
the TEA_OPTIONAL_STEPS constant that is already defined on line 40 with the same
values. Replace the hardcoded tuple with TEA_OPTIONAL_STEPS in the list
comprehension to maintain a single source of truth and prevent drift if the
constant is updated in the future.
🪄 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: 96059c74-083f-4b59-8db3-450f1971b381
📒 Files selected for processing (27)
scripts/run-smoke-policy-invariants.pyscripts/run-smoke-resume-matrix.pyskills/bmad-story-automator/data/orchestration-policy.jsonskills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.pyskills/bmad-story-automator/src/story_automator/commands/orchestrator_state.pyskills/bmad-story-automator/src/story_automator/commands/state.pyskills/bmad-story-automator/src/story_automator/core/policy_state_rendering.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy.pyskills/bmad-story-automator/src/story_automator/core/runtime_policy_support.pyskills/bmad-story-automator/src/story_automator/core/tea_policy.pyskills/bmad-story-automator/src/story_automator/core/tea_policy_assets.pyskills/bmad-story-automator/src/story_automator/core/tea_policy_config.pyskills/bmad-story-automator/src/story_automator/core/workflow_steps.pyskills/bmad-story-automator/steps-c/step-01b-continue.mdskills/bmad-story-automator/steps-c/step-03-execute.mdskills/bmad-story-automator/steps-c/step-03a-execute-review.mdskills/bmad-story-automator/steps-c/step-03b-execute-finish.mdtests/test_orchestrator_parse.pytests/test_policy_invariants.pytests/test_progress_invariants.pytests/test_resume_matrix.pytests/test_runtime_policy.pytests/test_state_policy_metadata.pytests/test_tea_detection.pytests/test_tea_policy_flow.pytests/test_tea_state_rendering.py
💤 Files with no reviewable changes (1)
- skills/bmad-story-automator/data/orchestration-policy.json
🚧 Files skipped from review as they are similar to previous changes (8)
- skills/bmad-story-automator/steps-c/step-03b-execute-finish.md
- skills/bmad-story-automator/src/story_automator/commands/orchestrator_epic_agents.py
- skills/bmad-story-automator/steps-c/step-03-execute.md
- skills/bmad-story-automator/steps-c/step-03a-execute-review.md
- tests/test_runtime_policy.py
- tests/test_state_policy_metadata.py
- tests/test_tea_detection.py
- skills/bmad-story-automator/src/story_automator/commands/state.py
🛑 Comments failed to post (2)
skills/bmad-story-automator/src/story_automator/commands/orchestrator_state.py (1)
98-99:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing validation for
--setargument format.If
args[idx + 1]doesn't contain=,split("=", 1)returns a single-element list, causingValueErroron unpacking tokey, value.🐛 Proposed fix
if args[idx] == "--set" and idx + 1 < len(args): + if "=" not in args[idx + 1]: + print_json({"ok": False, "error": "invalid_set_argument", "argument": args[idx + 1]}) + return 1 key, value = args[idx + 1].split("=", 1)📝 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.if args[idx] == "--set" and idx + 1 < len(args): if "=" not in args[idx + 1]: print_json({"ok": False, "error": "invalid_set_argument", "argument": args[idx + 1]}) return 1 key, value = args[idx + 1].split("=", 1)🤖 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/orchestrator_state.py` around lines 98 - 99, The argument parsing logic for the `--set` flag in the condition checking `if args[idx] == "--set"` does not validate that the argument contains an equals sign before splitting. Add a check to verify that `args[idx + 1]` contains the `=` character before calling split, and either raise a clear error message or skip the invalid argument if the format is incorrect (missing the equals sign or value).skills/bmad-story-automator/steps-c/step-01b-continue.md (1)
71-71:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language label to the fenced block.
The unlabeled fence will keep tripping markdownlint MD040 for this doc.
Suggested patch
- ``` + ```text **Warning:** Stories {X, Y} are not complete in sprint-status.yaml. [B]atch them first - Add to queue before continuing [S]kip - Continue from current story anyway</details> <!-- suggestion_start --> <details> <summary>📝 Committable suggestion</summary> > ‼️ **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. ```suggestion🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 71-71: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/steps-c/step-01b-continue.md` at line 71, The fenced code block containing the warning message about Stories being incomplete in sprint-status.yaml is missing a language label, which violates markdownlint MD040. Add the language label "text" immediately after the opening three backticks (```) of this fenced code block to resolve the linting issue.Source: Linters/SAST tools
|
Sorry, Augment failed to review this pull request. |
Collaborator
|
augmentcode review |
|
Sorry, Augment failed to review this pull request. |
Collaborator
|
augmentcode review |
|
Sorry, Augment failed to review this pull request. |
Collaborator
|
augment review |
|
Sorry, Augment failed to review this pull request. |
Collaborator
|
@augmentcode review |
|
✅ Action performedReview finished.
|
|
Sorry, Augment failed to review this pull request. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Notes
Validation
PYTHONPATH=skills/bmad-story-automator/src python3 -m unittest discover -s testsnpm run test:smokenpm run pack:dry-runSummary by CodeRabbit
New Features
state-progressupdates for story progress.Improvements
storyRange/ID validation.Tests