Resolve BMAD artifacts path from config#18
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR centralizes BMAD implementation-artifact path resolution in a new module, updates prompt templates to use a parameterized implementation-artifacts placeholder, refactors commands and core modules to call the new helpers, and extends tests to cover docs-based and legacy artifact locations. ChangesArtifact Path Resolution Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.py`:
- Line 408: The call to implementation_artifacts_dir(get_project_root()) can
raise ValueError and currently bubbles up from the story-file-status flow; wrap
the matches assignment in a try/except that catches ValueError coming from
implementation_artifacts_dir (and get_project_root if applicable) and translate
it into the command's JSON error response instead of letting the exception crash
the process. Specifically, around the line that sets matches =
sorted(implementation_artifacts_dir(get_project_root()).glob(...)), catch
ValueError and emit the same JSON error structure used elsewhere in this command
(or call the existing error helper used by story-file-status), then exit/return
gracefully so the JSON contract is preserved.
In `@skills/bmad-story-automator/src/story_automator/commands/tmux.py`:
- Around line 208-220: The call to
implementation_artifacts_relpath(get_project_root()) inside _render_step_prompt
can raise errors for invalid BMAD path configs and currently may propagate
uncaught; wrap that call in a try/except and convert any path-related exceptions
into a PolicyError so prompt rendering fails with a controlled CLI error (same
error type _build_cmd already handles); update _render_step_prompt to catch
exceptions from implementation_artifacts_relpath (and any OSError/ValueError it
may raise) and re-raise a PolicyError with a clear message mentioning the
artifact path lookup failure.
In
`@skills/bmad-story-automator/src/story_automator/commands/validate_story_creation.py`:
- Around line 15-17: The code eagerly computes project_root =
os.environ.get("PROJECT_ROOT", os.getcwd()) and calls
implementation_artifacts_dir(project_root) into default_artifacts_dir at
import/run time which can raise on invalid BMAD config before action dispatch;
change this to lazy resolution or guarded resolution: remove the immediate call
and defer calling implementation_artifacts_dir until inside the action handlers
that need artifacts (e.g., in the functions handling "check", "prefix", or the
main dispatch), or wrap the call in a try/except that normalizes failures to a
safe default (e.g., None) and only surface errors when an artifact-producing
action runs; update references to default_artifacts_dir/artifacts_dir to obtain
the value at the point of use (call implementation_artifacts_dir(project_root)
there) and ensure the dispatch logic handles a None/invalid artifacts_dir
appropriately.
🪄 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: 2d1ad5ef-abbb-4ccb-9629-760ff846bc40
📒 Files selected for processing (13)
skills/bmad-story-automator/data/prompts/auto.mdskills/bmad-story-automator/data/prompts/create.mdskills/bmad-story-automator/data/prompts/dev.mdskills/bmad-story-automator/data/prompts/review.mdskills/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/tmux.pyskills/bmad-story-automator/src/story_automator/commands/validate_story_creation.pyskills/bmad-story-automator/src/story_automator/core/artifact_paths.pyskills/bmad-story-automator/src/story_automator/core/story_keys.pyskills/bmad-story-automator/src/story_automator/core/success_verifiers.pyskills/bmad-story-automator/src/story_automator/core/utils.pytests/test_success_verifiers.py
bma-d
left a comment
bma-d
left a comment
There was a problem hiding this comment.
Requesting changes. I verified the four path-resolution regressions on PR head 1bb46f1; each has a small repro in the inline comments. The main blocker is current BMAD configs that use {project-root} resolving to a literal folder, which breaks story lookup, success verification, and prompt rendering.
| artifacts = f"{{output_folder}}/{IMPLEMENTATION_ARTIFACTS}" | ||
| if not artifacts: | ||
| return None | ||
| artifacts = artifacts.replace("{output_folder}", output_folder) |
There was a problem hiding this comment.
This only expands {output_folder}, but current BMAD configs commonly use {project-root} in both implementation_artifacts and output_folder. Simple repro: write _bmad/bmm/config.yaml with implementation_artifacts: "{project-root}/_bmad-output/implementation-artifacts", create _bmad-output/implementation-artifacts/1-2-real.md, then call implementation_artifacts_dir(root) or story-file-status 1.2. The resolver returns <repo>/{project-root}/_bmad-output/implementation-artifacts, normalize_story_key falls back to 1-2, and story-file-status reports story file not found.
| return configured | ||
| legacy = root / DEFAULT_OUTPUT_FOLDER / IMPLEMENTATION_ARTIFACTS | ||
| docs_bmad = root / DOCS_BMAD_ARTIFACTS | ||
| if not legacy.exists() and not (root / DEFAULT_OUTPUT_FOLDER / "sprint-status.yaml").is_file(): |
There was a problem hiding this comment.
Directory existence alone blocks the docs fallback. Simple repro: create an empty _bmad-output/implementation-artifacts/ directory, then create docs/bmad/implementation-artifacts/1-2-docs.md and docs/bmad/implementation-artifacts/sprint-status.yaml. implementation_artifacts_dir(root) still returns _bmad-output/implementation-artifacts, so story lookup and success verification miss the active docs artifacts.
| print(count_files(story_id, artifacts_dir)) | ||
| return 0 | ||
| try: | ||
| artifacts_dir = resolve_default_artifacts_dir() |
There was a problem hiding this comment.
count --artifacts-dir still resolves the default config before reading the override. Simple repro: put an invalid config like implementation_artifacts: ../outside/implementation-artifacts, create a valid custom folder with 1-2-custom.md, then run validate-story-creation count 1.2 --artifacts-dir <custom>. Expected count is 1, but the command exits 1 with BMAD config implementation_artifacts must stay within project root. Parse the override before calling resolve_default_artifacts_dir().
| relative = resolved.relative_to(allowed_root) | ||
| except ValueError: | ||
| continue | ||
| return artifacts_root, str(relative) |
There was a problem hiding this comment.
This validates against either the configured artifacts root or the legacy root, but always returns artifacts_root. Simple repro: configure implementation_artifacts: docs/bmad/implementation-artifacts, create _bmad-output/implementation-artifacts/1-2-legacy.md, then call create_story_artifact with glob: _bmad-output/implementation-artifacts/{story_prefix}-*.md. _resolve_artifact_glob accepts the legacy pattern but returns the docs root with 1-2-*.md, so it reports actualMatches: 0 even though the legacy file exists. Return the matched allowed_root or stop accepting legacy globs under a configured docs root.
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/validate_story_creation.py`:
- Around line 123-130: When parsing CLI in validate_story_creation.py for
"count" mode, the loop that sets artifacts_dir from rest is allowing
malformed/missing values and falling back silently; update the parsing so that
when "--artifacts-dir" is present you must (1) verify there is a following token
in rest, (2) ensure that token is not another flag (doesn't start with "-"), and
(3) optionally validate that Path(token) exists/is a directory before assigning
artifacts_dir; if any check fails, print a clear CLI error/usage message and
exit with a non-zero code instead of calling resolve_default_artifacts_dir();
keep references to rest, artifacts_dir, resolve_default_artifacts_dir(), and
count_files() so you modify the same block.
🪄 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: 04932007-778e-42df-8cec-ab7cffdb7eef
📒 Files selected for processing (6)
skills/bmad-story-automator/src/story_automator/commands/orchestrator.pyskills/bmad-story-automator/src/story_automator/commands/tmux.pyskills/bmad-story-automator/src/story_automator/commands/validate_story_creation.pyskills/bmad-story-automator/src/story_automator/core/artifact_paths.pyskills/bmad-story-automator/src/story_automator/core/success_verifiers.pytests/test_success_verifiers.py
|
Thanks for the concrete repros. I pushed
I added regression coverage for each repro in Validation:
|
bma-d
left a comment
bma-d
left a comment
There was a problem hiding this comment.
Requesting changes: configured BMAD artifacts paths still break several existing paths.
Simple repro from latest head 0c0c3856:
# Project config
_bmad/bmm/config.yaml: implementation_artifacts: docs/bmad/implementation-artifacts
# Existing docs-layout files
docs/bmad/implementation-artifacts/1-2-docs.md
docs/bmad/implementation-artifacts/sprint-status.yaml
Results:
control_direct_config_aware_create_story_artifact -> verified=true
validate-story-creation check 1.2 -> valid=false, created_count=0
orchestrator-helper verify-step create 1.2 -> verified=false, actualMatches=0
sprint-compare --sprint _bmad-output/implementation-artifacts/sprint-status.yaml -> {"ok":false,"error":"sprint_not_found"}
sprint-compare --sprint docs/bmad/implementation-artifacts/sprint-status.yaml -> {"ok":true,"incomplete":[],"checked":["1.1"]}
monitoring fallback legacy glob -> empty; docs glob finds docs/bmad/implementation-artifacts/1-2-docs.md
check-blocking 1.1 with epic file only in configured artifacts dir -> {"blocking":true,"reason":"epic_file_not_found"}
Specific remaining issues:
data/orchestration-policy.json:53still hardcodes_bmad-output/implementation-artifacts/{story_prefix}-*.md, so policy-backed create verification ignores the configured docs path even though the direct config-aware verifier succeeds. This is a P1 because a configured docs-layout create run escalates as failed after successfully creating the story.steps-c/step-01b-continue.md:79still passes_bmad-output/implementation-artifacts/sprint-status.yamltosprint-compare, so resume mode fails withsprint_not_foundwhen sprint status lives in the configured docs path.data/monitoring-fallback.md:38still checks story files only under_bmad-output/implementation-artifacts, so fallback recovery misses successfully created docs-layout stories.orchestrator_epic_agents.py:196only searches_bmad-output/implementation-artifacts/epic-*.mdanddocs/epics/epic-*.md; configured-artifacts-only epic files are reported asepic_file_not_found.
Targeted tests still pass (77 passed), but they do not cover these bundled policy/instruction fallback paths.
| return {"verified": False, "reason": "could_not_normalize_key", "input": story_key} | ||
| config = _success_config(contract) | ||
| raw_glob = str(config.get("glob") or "_bmad-output/implementation-artifacts/{story_prefix}-*.md") | ||
| raw_glob = str(config.get("glob") or implementation_artifacts_glob(project_root, "{story_prefix}-*.md")) |
There was a problem hiding this comment.
This fallback is config-aware, but it never runs for the bundled create contract because data/orchestration-policy.json still provides success.config.glob: _bmad-output/implementation-artifacts/{story_prefix}-*.md. In a project with implementation_artifacts: docs/bmad/implementation-artifacts, a direct create_story_artifact(project_root, "1.2") finds docs/bmad/implementation-artifacts/1-2-docs.md, while validate-story-creation check 1.2 and orchestrator-helper verify-step create 1.2 both return zero matches because the policy glob overrides this default.
|
Thanks for the latest review. I pushed
Added regression coverage for both paths. Local verification: |
bma-d
left a comment
bma-d
left a comment
There was a problem hiding this comment.
Requesting changes again. The previous P1 create-verification issue is fixed: data/orchestration-policy.json no longer pins success.config.glob, and the repro now passes for both validate-story-creation check 1.2 and orchestrator-helper verify-step create 1.2 with implementation_artifacts: docs/bmad/implementation-artifacts.
Remaining verified issues are path-consistency gaps in existing flows:
# fixture
_bmad/bmm/config.yaml: implementation_artifacts: docs/bmad/implementation-artifacts
docs/bmad/implementation-artifacts/1-1-docs.md
docs/bmad/implementation-artifacts/1-2-docs.md
docs/bmad/implementation-artifacts/sprint-status.yaml
docs/bmad/implementation-artifacts/epic-1-docs.md
resume_step_hardcoded_sprint_path -> code=1 {"ok":false,"error":"sprint_not_found"}
control_configured_sprint_path -> code=0 {"ok":true,"incomplete":[],"checked":["1.1"]}
monitoring fallback legacy glob -> empty
configured glob -> docs/bmad/implementation-artifacts/1-2-docs.md
check_blocking_configured_epic_only -> {"blocking":true,"reason":"epic_file_not_found"}
control_check_blocking_same_epic_in_legacy_dir -> {"blocking":false,"reason":"no_dependents_found"}
Targeted verification from my last pass: 233 passed, 14 subtests passed. No P0/P1 remains, but these P2s are still real user-visible regressions for configured artifact layouts.
| return legacy | ||
|
|
||
|
|
||
| def sprint_status_path(project_root: str | Path) -> Path: |
There was a problem hiding this comment.
This helper resolves the configured sprint-status.yaml, but the resume workflow still bypasses it. steps-c/step-01b-continue.md:79 invokes sprint-compare --sprint "_bmad-output/implementation-artifacts/sprint-status.yaml", so a project configured with implementation_artifacts: docs/bmad/implementation-artifacts fails resume with {"ok":false,"error":"sprint_not_found"} even though the same command succeeds when pointed at docs/bmad/implementation-artifacts/sprint-status.yaml.
| return artifacts.relative_to(root).as_posix() | ||
|
|
||
|
|
||
| def implementation_artifacts_glob(project_root: str | Path, pattern: str) -> str: |
There was a problem hiding this comment.
The configured artifact glob helper is now available, but the monitoring fallback path still hardcodes the legacy directory. data/monitoring-fallback.md:38 runs ls _bmad-output/implementation-artifacts/{story_prefix}-*.md; in the same docs-layout fixture this returns empty while docs/bmad/implementation-artifacts/1-2-docs.md exists, so fallback recovery can miss a successfully created story.
| from story_automator.core.sprint import sprint_status_epic | ||
| from story_automator.core.story_keys import normalize_story_key | ||
| from story_automator.core.utils import file_exists, get_project_root, iso_now, print_json, read_text, trim_lines, unquote_scalar | ||
| from story_automator.core.utils import file_exists, get_project_root, iso_now, print_json, read_text, strip_inline_yaml_comment, trim_lines, unquote_scalar |
There was a problem hiding this comment.
This module still needs the configured artifact resolver for epic-file lookup. find_epic_file() below only searches _bmad-output/implementation-artifacts/epic-{epic}-*.md and docs/epics/epic-{epic}-*.md; with epic-1-docs.md only under the configured docs/bmad/implementation-artifacts dir, check-blocking 1.1 returns epic_file_not_found and treats the story as blocking. Copying the same file to the legacy artifact dir flips the result to non-blocking.
|
Thanks — I pushed
I also added regression coverage for:
Verification I ran locally: One note from verification: |
2 participants
Summary
_bmad-output/implementation-artifacts.implementation_artifacts,output_folder,docs/bmad/implementation-artifacts, and legacy_bmad-outputlayouts.Test plan
PYTHONPATH="/home/seven/data/coding/projects/others/bmad-automator/skills/bmad-story-automator/src" python -m pytest "/home/seven/data/coding/projects/others/bmad-automator/tests/test_success_verifiers.py" -qenv -u AI_AGENT PYTHONPATH="/home/seven/data/coding/projects/others/bmad-automator/skills/bmad-story-automator/src" python -m pytest "/home/seven/data/coding/projects/others/bmad-automator/tests" -qSummary by CodeRabbit
Refactor
New Features
Bug Fixes
Documentation
Tests