From 675ce7c71829cb297a77635e3fee66467e743c55 Mon Sep 17 00:00:00 2001 From: Pawel-N-pl Date: Sun, 7 Jun 2026 14:49:44 +0200 Subject: [PATCH 1/2] fix(story-automator): compute story test counts from JUnit truth Part of #40 --- .../data/orchestration-policy.json | 4 + .../bmad-story-automator/data/prompts/dev.md | 4 + .../src/story_automator/cli.py | 3 + .../src/story_automator/commands/basic.py | 173 +++++++++++-- .../src/story_automator/core/junit.py | 52 ++++ .../story_automator/core/runtime_policy.py | 30 ++- .../steps-c/step-03-execute.md | 3 + .../steps-c/step-03b-execute-finish.md | 22 +- tests/test_test_counts.py | 241 ++++++++++++++++++ 9 files changed, 510 insertions(+), 22 deletions(-) create mode 100644 skills/bmad-story-automator/src/story_automator/core/junit.py create mode 100644 tests/test_test_counts.py diff --git a/skills/bmad-story-automator/data/orchestration-policy.json b/skills/bmad-story-automator/data/orchestration-policy.json index a2ad41d6..f6b5a127 100644 --- a/skills/bmad-story-automator/data/orchestration-policy.json +++ b/skills/bmad-story-automator/data/orchestration-policy.json @@ -14,6 +14,10 @@ "arrays": "replace" } }, + "test": { + "command": "", + "junitPath": "" + }, "workflow": { "sequence": ["create", "dev", "auto", "review", "retro"], "repeat": { diff --git a/skills/bmad-story-automator/data/prompts/dev.md b/skills/bmad-story-automator/data/prompts/dev.md index a808029f..d425e38e 100644 --- a/skills/bmad-story-automator/data/prompts/dev.md +++ b/skills/bmad-story-automator/data/prompts/dev.md @@ -2,3 +2,7 @@ Execute the BMAD dev-story workflow for story {{story_id}}. {{skill_line}}{{workflow_line}}{{instructions_line}}{{checklist_line}}Story file: `{{implementation_artifacts}}/{{story_prefix}}-*.md` Implement all tasks marked [ ]. Run tests. Update checkboxes. +Emit a JUnit XML report from the test run if the runner supports it, and always +print the full test summary (total / failures / errors / skipped). Do NOT +transcribe test counts into the story prose — the orchestrator records them in a +machine-owned section from the JUnit report. diff --git a/skills/bmad-story-automator/src/story_automator/cli.py b/skills/bmad-story-automator/src/story_automator/cli.py index c66920c9..60abf270 100644 --- a/skills/bmad-story-automator/src/story_automator/cli.py +++ b/skills/bmad-story-automator/src/story_automator/cli.py @@ -12,6 +12,7 @@ cmd_list_sessions, cmd_reconcile_story, cmd_stop_hook, + cmd_test_counts, ) from .commands.orchestrator import cmd_orchestrator_helper from .commands.state import cmd_build_state_doc, cmd_sprint_compare, cmd_state_metrics, cmd_validate_state @@ -42,6 +43,7 @@ def main(argv: list[str] | None = None) -> int: "build-state-doc": cmd_build_state_doc, "commit-story": cmd_commit_story, "reconcile-story": cmd_reconcile_story, + "test-counts": cmd_test_counts, "parse-epic": _cmd_parse_epic, "parse-story": _cmd_parse_story, "parse-story-range": _cmd_parse_story_range, @@ -79,6 +81,7 @@ def _usage(stream: object) -> None: "build-state-doc", "commit-story", "reconcile-story", + "test-counts", "parse-epic", "parse-story", "parse-story-range", diff --git a/skills/bmad-story-automator/src/story_automator/commands/basic.py b/skills/bmad-story-automator/src/story_automator/commands/basic.py index 75d352af..5a334e55 100644 --- a/skills/bmad-story-automator/src/story_automator/commands/basic.py +++ b/skills/bmad-story-automator/src/story_automator/commands/basic.py @@ -8,10 +8,13 @@ from pathlib import Path from ..core.artifact_paths import implementation_artifacts_dir, implementation_artifacts_relpath +from ..core.junit import parse_junit from ..core.runtime_layout import active_marker_path, runtime_provider +from ..core.runtime_policy import PolicyError, load_policy_unresolved, test_config from ..core.stop_hooks import HookConfigError, ensure_stop_hook from ..core.story_keys import normalize_story_key from ..core.utils import ( + ensure_dir, get_project_slug, run_cmd, write_json, @@ -247,10 +250,11 @@ def _git_changed_files(repo: str, extra_excludes: tuple[str, ...] = ()) -> list[ return sorted(files) -def _file_list_bounds(lines: list[str]) -> tuple[int, int] | None: +def _section_bounds(lines: list[str], heading: str) -> tuple[int, int] | None: + target = heading.strip().lower() start = None for idx, line in enumerate(lines): - if line.strip().lower() == "### file list": + if line.strip().lower() == target: start = idx break if start is None: @@ -263,6 +267,10 @@ def _file_list_bounds(lines: list[str]) -> tuple[int, int] | None: return start, end +def _file_list_bounds(lines: list[str]) -> tuple[int, int] | None: + return _section_bounds(lines, "### File List") + + # Known dev-record status annotations to strip when parsing a hand-written File # List — a closed set, so we never mangle a real filename that ends in ")". _FILE_LIST_ANNOTATIONS = ("(new)", "(modified)", "(deleted)", "(added)", "(renamed)", "(updated)") @@ -305,6 +313,23 @@ def _reconcile_section(text: str, git_files: list[str]) -> tuple[str, list[str]] return "\n".join(new_lines) + suffix, current +# Map a story id to its artifact file (resolved key first, prefix glob fallback). +# Returns (story_file, None) on success or (None, error_payload) so callers can +# emit the error verbatim and bail. +def _resolve_story_file(repo: str, story: str) -> tuple[Path | None, dict | None]: + norm = normalize_story_key(repo, story) + if norm is None: + return None, {"ok": False, "error": "story_key_invalid", "input": story} + artifacts = implementation_artifacts_dir(repo) + exact = artifacts / f"{norm.key}.md" + if norm.key and exact.is_file(): # disambiguate via the resolved key before falling back to prefix glob + return exact, None + matches = sorted(artifacts.glob(f"{norm.prefix}-*.md")) + if not matches: + return None, {"ok": False, "error": "story_file_not_found", "prefix": norm.prefix} + return matches[0], None + + def cmd_reconcile_story(args: list[str]) -> int: repo = "" story = "" @@ -322,20 +347,10 @@ def cmd_reconcile_story(args: list[str]) -> int: if not Path(repo).is_dir(): write_json({"ok": False, "error": "repo_not_found"}) return 1 - norm = normalize_story_key(repo, story) - if norm is None: - write_json({"ok": False, "error": "story_key_invalid", "input": story}) + story_file, err = _resolve_story_file(repo, story) + if story_file is None: + write_json(err) return 1 - artifacts = implementation_artifacts_dir(repo) - exact = artifacts / f"{norm.key}.md" - if norm.key and exact.is_file(): # disambiguate via the resolved key before falling back to prefix glob - story_file = exact - else: - matches = sorted(artifacts.glob(f"{norm.prefix}-*.md")) - if not matches: - write_json({"ok": False, "error": "story_file_not_found", "prefix": norm.prefix}) - return 1 - story_file = matches[0] # Exclude the resolved artifacts dir (may be _bmad-output/ OR docs/bmad/...) so the # story file and its siblings never pollute the reconciled File List. git_files = _git_changed_files(repo, (implementation_artifacts_relpath(repo) + "/",)) @@ -365,6 +380,134 @@ def cmd_reconcile_story(args: list[str]) -> int: return 0 +TEST_COUNTS_HEADING = "### Test Counts" + + +def _render_test_counts(counts: dict) -> list[str]: + body = [ + f"- Tests: {counts['tests']}", + f"- Failures: {counts['failures']}", + f"- Errors: {counts['errors']}", + f"- Skipped: {counts['skipped']}", + ] + if counts.get("assertions") is not None: # PHPUnit-only; omit the line entirely otherwise + body.append(f"- Assertions: {counts['assertions']}") + return body + + +# Rewrite `heading`'s body deterministically (leaving other sections untouched), +# appending the section at EOF when absent. Mirrors the File List reconcile so a +# re-run with identical counts is a byte-for-byte no-op. +def _replace_or_append_section(text: str, heading: str, body: list[str]) -> str: + lines = text.splitlines() + suffix = "\n" if text.endswith("\n") else "" + bounds = _section_bounds(lines, heading) + if bounds is None: + tail = ["", heading, "", *body] + new_lines = [*lines, *tail] if lines else [heading, "", *body] + return "\n".join(new_lines) + suffix + start, end = bounds + rest = lines[end:] + block = ["", *body] + if rest: # blank separator only when another section follows + block.append("") + new_lines = [*lines[: start + 1], *block, *rest] + return "\n".join(new_lines) + suffix + + +def cmd_test_counts(args: list[str]) -> int: + if args and args[0] in {"--help", "-h"}: + print("Usage: test-counts --repo PATH --story KEY [--since EPOCH] [--write]") + return 0 + repo = "" + story = "" + since: float | None = None + do_write = False + idx = 0 + while idx < len(args): + arg = args[idx] + if arg == "--repo" and idx + 1 < len(args): + repo = args[idx + 1] + idx += 2 + elif arg == "--story" and idx + 1 < len(args): + story = args[idx + 1] + idx += 2 + elif arg == "--since" and idx + 1 < len(args): + try: + since = float(args[idx + 1]) + except ValueError: + since = None + idx += 2 + elif arg == "--write": + do_write = True + idx += 1 + else: + idx += 1 + if not repo or not story: + write_json({"ok": False, "error": "missing_args"}) + return 1 + if not Path(repo).is_dir(): + write_json({"ok": False, "error": "repo_not_found"}) + return 1 + story_file, err = _resolve_story_file(repo, story) + if story_file is None: + write_json(err) + return 1 + try: + cfg = test_config(load_policy_unresolved(repo)) + except PolicyError: + write_json({"ok": False, "error": "policy_invalid"}) + return 1 + junit_rel = cfg["junitPath"] + command = cfg["command"] + if not junit_rel: # Tier 3: nothing configured — File List reconcile still ran independently + write_json({"ok": True, "skipped": True, "reason": "test_not_configured", "test_counts": None, "wrote": False}) + return 0 + junit_path = Path(repo) / junit_rel.replace("{story}", story_file.stem) + fresh = junit_path.is_file() and (since is None or junit_path.stat().st_mtime >= since) + rerun_exit: int | None = None + if fresh: # Tier 1: trust the artifact emitted by this dev run + source = "capture" + elif command: # Tier 2: deterministic floor — re-run and parse what it emits + resolved = command.replace("{junit}", str(junit_path)).replace("{story}", story_file.stem) + ensure_dir(junit_path.parent) + rerun_exit = run_cmd("bash", "-c", resolved, cwd=repo).exit_code # non-zero is expected when tests fail + if not junit_path.is_file(): + write_json( + {"ok": True, "skipped": True, "reason": "test_artifact_not_emitted", "command_exit": rerun_exit, "test_counts": None, "wrote": False} + ) + return 0 + source = "rerun" + else: # Tier 3: stale/missing artifact and no runner to fall back on + reason = "test_artifact_stale" if junit_path.is_file() else "test_artifact_missing" + write_json({"ok": True, "skipped": True, "reason": reason, "test_counts": None, "wrote": False}) + return 0 + try: + counts = parse_junit(junit_path) + except ValueError: + write_json({"ok": False, "error": "junit_parse_failed", "junit_path": str(junit_path)}) + return 1 + text = story_file.read_text(encoding="utf-8") + new_text = _replace_or_append_section(text, TEST_COUNTS_HEADING, _render_test_counts(counts)) + wrote = False + if do_write and new_text != text: + story_file.write_text(new_text, encoding="utf-8") + wrote = True + payload = { + "ok": True, + "skipped": False, + "story_file": str(story_file), + "source": source, + "junit_path": str(junit_path), + "test_counts": counts, + "wrote": wrote, + } + if rerun_exit is not None: + payload["command_exit"] = rerun_exit + write_json(payload) + return 0 + + def cmd_list_sessions(args: list[str]) -> int: if args and args[0] in {"--help", "-h"}: print("Usage: list-sessions --slug SLUG") diff --git a/skills/bmad-story-automator/src/story_automator/core/junit.py b/skills/bmad-story-automator/src/story_automator/core/junit.py new file mode 100644 index 00000000..3179f6b5 --- /dev/null +++ b/skills/bmad-story-automator/src/story_automator/core/junit.py @@ -0,0 +1,52 @@ +from __future__ import annotations + +import xml.etree.ElementTree as ET +from pathlib import Path +from typing import Any + +# Universal across phpunit/pytest/jest/gotestsum/etc. `assertions` is PHPUnit-only, +# so it is reported separately and stays nullable when no suite carries it. +_COUNT_ATTRS = ("tests", "failures", "errors", "skipped") + + +# Aggregate {tests, failures, errors, skipped, assertions|None} from a JUnit +# report. Reads only standard testsuite/testsuites attrs (the project decides HOW +# its runner emits them); raises ValueError on unreadable/non-JUnit XML so the +# caller can degrade cleanly. +def parse_junit(path: str | Path) -> dict[str, Any]: + try: + root = ET.parse(str(path)).getroot() + except (ET.ParseError, OSError) as exc: + raise ValueError(f"junit parse failed: {path}") from exc + if root.tag == "testsuite": + suites = [root] + elif root.tag == "testsuites": + # Sum DIRECT children only: phpunit nests groups whose parent + # already carries subtree totals, so a recursive sum would double-count. + # A bare aggregate-only (no children) is read directly. + suites = root.findall("testsuite") + if not suites and root.get("tests") is not None: + suites = [root] + else: + raise ValueError(f"not a junit report (root <{root.tag}>): {path}") + counts: dict[str, Any] = {key: 0 for key in _COUNT_ATTRS} + assertions = 0 + has_assertions = False + for suite in suites: + for key in _COUNT_ATTRS: + counts[key] += _int_attr(suite.get(key)) + raw = suite.get("assertions") + if raw is not None: + has_assertions = True + assertions += _int_attr(raw) + counts["assertions"] = assertions if has_assertions else None + return counts + + +def _int_attr(value: str | None) -> int: + if value is None: + return 0 + try: + return int(value) + except (TypeError, ValueError): + return 0 diff --git a/skills/bmad-story-automator/src/story_automator/core/runtime_policy.py b/skills/bmad-story-automator/src/story_automator/core/runtime_policy.py index a0cd393e..ee8c7421 100644 --- a/skills/bmad-story-automator/src/story_automator/core/runtime_policy.py +++ b/skills/bmad-story-automator/src/story_automator/core/runtime_policy.py @@ -9,7 +9,7 @@ from .runtime_layout import active_marker_path, bundled_story_skill_root, resolve_portable_path, resolve_skill_dir from .utils import ensure_dir, get_project_root, iso_now, md5_hex8, read_text, write_atomic -VALID_TOP_LEVEL_KEYS = {"version", "snapshot", "runtime", "workflow", "steps"} +VALID_TOP_LEVEL_KEYS = {"version", "snapshot", "runtime", "workflow", "steps", "test"} VALID_STEP_NAMES = {"create", "dev", "auto", "review", "retro"} VALID_VERIFIERS = {"create_story_artifact", "session_exit", "review_completion", "epic_complete"} VALID_ASSET_NAMES = {"skill", "workflow", "instructions", "checklist", "template"} @@ -48,6 +48,22 @@ def load_effective_policy(project_root: str | None = None, *, resolve_assets: bo return policy +# Effective policy (bundled + override, deep-merged and validated) WITHOUT +# resolving step assets/success contracts: reading a project-wide block like +# `test` must not require sibling skills to be installed, which full resolution +# would otherwise demand. +def load_policy_unresolved(project_root: str | None = None) -> dict[str, Any]: + root = Path(project_root or get_project_root()).resolve() + bundle_root = bundled_skill_root(root) + bundled = _read_json(bundle_root / "data" / "orchestration-policy.json") + override_path = root / "_bmad" / "bmm" / "story-automator.policy.json" + override = _read_json(override_path) if override_path.is_file() else {} + policy = _deep_merge(bundled, override) + _apply_legacy_env(policy) + _validate_policy_shape(policy) + return policy + + def load_runtime_policy( project_root: str | None = None, state_file: str | Path | None = None, @@ -208,6 +224,14 @@ def parser_runtime_config(policy: dict[str, Any]) -> dict[str, object]: return {"provider": provider, "model": model, "timeoutSeconds": timeout} +def test_config(policy: dict[str, Any]) -> dict[str, str]: + test = _expect_optional_dict(policy, "test") + return { + "command": str(test.get("command") or "").strip(), + "junitPath": str(test.get("junitPath") or "").strip(), + } + + def bundled_skill_root(project_root: str | Path | None = None) -> Path: root = Path(project_root or get_project_root()).resolve() try: @@ -290,6 +314,10 @@ def _validate_policy_shape(policy: dict[str, Any]) -> None: runtime = _expect_optional_dict(policy, "runtime") _expect_optional_nested_dict(runtime, "merge", "runtime") parser_runtime_config(policy) + test = _expect_optional_dict(policy, "test") + for key in ("command", "junitPath"): + if key in test and not isinstance(test.get(key), str): + raise PolicyError(f"test.{key} must be a string") workflow = _expect_optional_dict(policy, "workflow") repeat = _expect_optional_nested_dict(workflow, "repeat", "workflow") review = _expect_optional_nested_dict(repeat, "review", "workflow.repeat") diff --git a/skills/bmad-story-automator/steps-c/step-03-execute.md b/skills/bmad-story-automator/steps-c/step-03-execute.md index b7df134b..fcc614d5 100644 --- a/skills/bmad-story-automator/steps-c/step-03-execute.md +++ b/skills/bmad-story-automator/steps-c/step-03-execute.md @@ -167,6 +167,9 @@ if should_apply_primary_model "$current_agent"; then else built_cmd=$("$scripts" tmux-wrapper build-cmd dev {story_id} --agent "$current_agent" --state-file "$state_file") fi +# Mark dev start so test-counts can tell a JUnit artifact from THIS run (Tier-1 +# capture) apart from a stale one left by an earlier story. +dev_started=$(date -u +%s) session=$("$scripts" tmux-wrapper spawn dev {epic} {story_id} \ --agent "$current_agent" \ --command "$built_cmd") diff --git a/skills/bmad-story-automator/steps-c/step-03b-execute-finish.md b/skills/bmad-story-automator/steps-c/step-03b-execute-finish.md index 0dc9bb25..b82fc301 100644 --- a/skills/bmad-story-automator/steps-c/step-03b-execute-finish.md +++ b/skills/bmad-story-automator/steps-c/step-03b-execute-finish.md @@ -15,15 +15,15 @@ outputFile: '{output_folder}/story-automator/orchestration-{epic_id}-{timestamp} ## Story Loop (Continue from Step 3) -### E. Reconcile File List + Git Commit +### E. Reconcile Story Record + Git Commit **Required:** Commit after every story (do not skip). -First, sync the story's File List to git truth. This is the **last** point the File List -can change: the `auto` step (§C) and the review loop (§D) have finished adding/removing -files, and the commit below is about to snapshot the record. Reconciling here — not at -dev-close — is what actually ends doc-drift (a dev-close reconcile goes stale the moment -`auto` adds tests). +First, sync the story's record to machine truth — both the File List (vs git) and the test +counts (vs JUnit). This is the **last** point either can change: the `auto` step (§C) and the +review loop (§D) have finished adding/removing files and tests, and the commit below is about +to snapshot the record. Reconciling here — not at dev-close — is what actually ends doc-drift +(a dev-close reconcile goes stale the moment `auto` adds tests). ```bash # Done-close gate: reconcile File List against git truth before the commit snapshots it. @@ -34,6 +34,16 @@ else echo "- **[$(date -u +%Y-%m-%dT%H:%M:%SZ)]** WARNING: File List reconcile failed: $(printf '%s' "$reconcile" | jq -c '.error // .')" >> "{outputFile}" fi +# Sync test counts from JUnit truth (machine-computed; ends count drift). --since "$dev_started" +# (set in step-03 §B) trusts the artifact left by the latest run for THIS story — after `auto` +# re-ran the suite — or re-runs the configured command as a deterministic floor, else skips. +test_counts=$("{scriptsDir}" test-counts --repo "{project-root}" --story {story_id} --since "$dev_started" --write) +if [ "$(printf '%s' "$test_counts" | jq -r '.ok')" = "true" ]; then + echo "- **[$(date -u +%Y-%m-%dT%H:%M:%SZ)]** Test counts: $(printf '%s' "$test_counts" | jq -c '{source, skipped, reason, test_counts, wrote}')" >> "{outputFile}" +else + echo "- **[$(date -u +%Y-%m-%dT%H:%M:%SZ)]** WARNING: test-counts failed: $(printf '%s' "$test_counts" | jq -c '.error // .')" >> "{outputFile}" +fi + commit=$("{scriptsDir}" commit-story --repo "{project-root}" --story {story_id} --title "{title}") ok=$(echo "$commit" | jq -r '.ok') ``` diff --git a/tests/test_test_counts.py b/tests/test_test_counts.py new file mode 100644 index 00000000..b2cfe18c --- /dev/null +++ b/tests/test_test_counts.py @@ -0,0 +1,241 @@ +from __future__ import annotations + +import io +import json +import os +import tempfile +import time +import unittest +from contextlib import redirect_stdout +from pathlib import Path + +import story_automator +from story_automator.commands.basic import cmd_test_counts +from story_automator.core.junit import parse_junit + +# Pin the bundled policy to THIS checkout so the override merge is hermetic even +# when bmad-story-automator is also installed under ~/.claude/skills. +SKILL_DIR = Path(story_automator.__file__).resolve().parents[2] + + +class ParseJunitTests(unittest.TestCase): + """JUnit parsing is stack-agnostic: only the universal testsuite attributes + are read, summed over direct children to avoid phpunit's nested double-count.""" + + def setUp(self) -> None: + self.tmp = tempfile.TemporaryDirectory() + self.dir = Path(self.tmp.name) + + def tearDown(self) -> None: + self.tmp.cleanup() + + def _xml(self, body: str) -> Path: + path = self.dir / "junit.xml" + path.write_text(body, encoding="utf-8") + return path + + def test_single_testsuite_root(self) -> None: + path = self._xml('') + self.assertEqual( + parse_junit(path), + {"tests": 5, "failures": 1, "errors": 0, "skipped": 2, "assertions": 42}, + ) + + def test_testsuites_sums_direct_children(self) -> None: + path = self._xml( + '' + '' + '' + '' + ) + counts = parse_junit(path) + self.assertEqual((counts["tests"], counts["failures"], counts["skipped"]), (5, 1, 1)) + self.assertIsNone(counts["assertions"]) # no suite carries it -> nullable + + def test_nested_suites_not_double_counted(self) -> None: + # phpunit nests a child suite under a parent that already holds subtree + # totals; summing direct children only must report 10, not 20. + path = self._xml( + '' + '' + '' + '' + '' + ) + counts = parse_junit(path) + self.assertEqual(counts["tests"], 10) + self.assertEqual(counts["assertions"], 30) + + def test_aggregate_only_testsuites(self) -> None: + path = self._xml('') + self.assertEqual(parse_junit(path)["tests"], 7) + + def test_corrupt_xml_raises(self) -> None: + with self.assertRaises(ValueError): + parse_junit(self._xml(' None: + with self.assertRaises(ValueError): + parse_junit(self._xml('')) + + +class TestCountsCommandTests(unittest.TestCase): + """test-counts computes story test counts from JUnit truth and owns a single + `### Test Counts` block — the second half of the doc-drift fix (issue #40).""" + + SUITE = '' + + def setUp(self) -> None: + self._prev_skills_root = os.environ.get("BMAD_SKILLS_ROOT") + os.environ["BMAD_SKILLS_ROOT"] = str(SKILL_DIR) + self.tmp = tempfile.TemporaryDirectory() + self.repo = Path(self.tmp.name) + self.artifacts = self.repo / "_bmad-output" / "implementation-artifacts" + self.artifacts.mkdir(parents=True) + self.story = self.artifacts / "1-2-example.md" + self.story.write_text( + "# Story 1.2\n\n## Dev Agent Record\n\n### File List\n\n- src/a.py\n", + encoding="utf-8", + ) + + def tearDown(self) -> None: + if self._prev_skills_root is None: + os.environ.pop("BMAD_SKILLS_ROOT", None) + else: + os.environ["BMAD_SKILLS_ROOT"] = self._prev_skills_root + self.tmp.cleanup() + + def _policy(self, **test_block: str) -> None: + path = self.repo / "_bmad" / "bmm" / "story-automator.policy.json" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps({"test": test_block}), encoding="utf-8") + + def _artifact(self, rel: str, body: str = SUITE, age_seconds: float = 0.0) -> Path: + path = self.repo / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(body, encoding="utf-8") + if age_seconds: + stamp = time.time() - age_seconds + os.utime(path, (stamp, stamp)) + return path + + def _invoke(self, *extra: str, story: str = "1-2") -> tuple[int, dict]: + out = io.StringIO() + with redirect_stdout(out): + code = cmd_test_counts(["--repo", str(self.repo), "--story", story, *extra]) + return code, json.loads(out.getvalue()) + + def test_tier3_skip_when_unconfigured(self) -> None: + # Bundled default has empty test.* -> nothing to compute, File List + # reconcile already ran independently so a skip is fine. + code, payload = self._invoke("--write") + self.assertEqual(code, 0) + self.assertTrue(payload["skipped"]) + self.assertEqual(payload["reason"], "test_not_configured") + self.assertIsNone(payload["test_counts"]) + + def test_tier1_capture_from_fresh_artifact(self) -> None: + self._policy(junitPath="reports/junit.xml") + self._artifact("reports/junit.xml") + code, payload = self._invoke("--write") + self.assertEqual(code, 0) + self.assertEqual(payload["source"], "capture") + self.assertEqual(payload["test_counts"], {"tests": 4, "failures": 0, "errors": 0, "skipped": 1, "assertions": 20}) + self.assertTrue(payload["wrote"]) + text = self.story.read_text(encoding="utf-8") + self.assertIn("### Test Counts", text) + self.assertIn("- Tests: 4", text) + self.assertIn("- Skipped: 1", text) + self.assertIn("- Assertions: 20", text) + self.assertIn("- src/a.py", text) # File List untouched + + def test_assertions_line_omitted_when_absent(self) -> None: + self._policy(junitPath="reports/junit.xml") + self._artifact("reports/junit.xml", '') + _, payload = self._invoke("--write") + self.assertIsNone(payload["test_counts"]["assertions"]) + self.assertNotIn("Assertions", self.story.read_text(encoding="utf-8")) + + def test_story_placeholder_in_junit_path(self) -> None: + self._policy(junitPath="reports/{story}.xml") + self._artifact("reports/1-2-example.xml") + _, payload = self._invoke() + self.assertTrue(payload["junit_path"].endswith("reports/1-2-example.xml")) + self.assertEqual(payload["test_counts"]["tests"], 4) + + def test_since_gates_stale_artifact_to_skip(self) -> None: + self._policy(junitPath="reports/junit.xml") # no command -> no floor + self._artifact("reports/junit.xml", age_seconds=600) + _, payload = self._invoke("--since", str(time.time())) + self.assertTrue(payload["skipped"]) + self.assertEqual(payload["reason"], "test_artifact_stale") + + def test_tier3_missing_artifact_no_command(self) -> None: + self._policy(junitPath="reports/junit.xml") + _, payload = self._invoke() + self.assertTrue(payload["skipped"]) + self.assertEqual(payload["reason"], "test_artifact_missing") + + def test_tier2_rerun_emits_and_parses(self) -> None: + self._policy( + junitPath="reports/junit.xml", + command="printf '' > {junit}", + ) + # No artifact on disk -> must fall to the re-run floor. + code, payload = self._invoke("--write") + self.assertEqual(code, 0) + self.assertEqual(payload["source"], "rerun") + self.assertEqual(payload["test_counts"], {"tests": 3, "failures": 1, "errors": 0, "skipped": 0, "assertions": None}) + self.assertEqual(payload["command_exit"], 0) + self.assertIn("- Tests: 3", self.story.read_text(encoding="utf-8")) + + def test_stale_artifact_reruns_when_command_set(self) -> None: + self._policy( + junitPath="reports/junit.xml", + command="printf '' > {junit}", + ) + self._artifact("reports/junit.xml", age_seconds=600) # stale capture + _, payload = self._invoke("--since", str(time.time())) + self.assertEqual(payload["source"], "rerun") + self.assertEqual(payload["test_counts"]["tests"], 9) + + def test_rerun_skips_when_artifact_not_emitted(self) -> None: + self._policy(junitPath="reports/junit.xml", command="true") # runs, writes nothing + _, payload = self._invoke() + self.assertTrue(payload["skipped"]) + self.assertEqual(payload["reason"], "test_artifact_not_emitted") + self.assertEqual(payload["command_exit"], 0) + + def test_write_is_idempotent(self) -> None: + self._policy(junitPath="reports/junit.xml") + self._artifact("reports/junit.xml") + self.assertTrue(self._invoke("--write")[1]["wrote"]) + first = self.story.read_text(encoding="utf-8") + code, payload = self._invoke("--write") + self.assertEqual(code, 0) + self.assertFalse(payload["wrote"]) # counts unchanged -> no churn + self.assertEqual(self.story.read_text(encoding="utf-8"), first) + + def test_corrupt_artifact_is_an_error(self) -> None: + self._policy(junitPath="reports/junit.xml") + self._artifact("reports/junit.xml", " None: + path = self.repo / "_bmad" / "bmm" / "story-automator.policy.json" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps({"test": {"command": 123}}), encoding="utf-8") # non-string + code, payload = self._invoke() + self.assertEqual(code, 1) + self.assertEqual(payload["error"], "policy_invalid") + + def test_story_not_found(self) -> None: + code, payload = self._invoke(story="9-9") + self.assertEqual(code, 1) + self.assertEqual(payload["error"], "story_file_not_found") + + +if __name__ == "__main__": + unittest.main() From 7b5006698bb015ab207c1aa9d0512f65ecdc6a36 Mon Sep 17 00:00:00 2001 From: Pawel-N-pl Date: Sun, 7 Jun 2026 14:59:52 +0200 Subject: [PATCH 2/2] fix(story-automator): validate --since and shell-quote rerun substitutions Address Augment review on #42: - reject non-numeric --since instead of silently dropping the staleness gate (a stale artifact could otherwise pass as a fresh capture and re-drift) - shlex.quote the {junit}/{story} substitutions so paths with spaces or shell metacharacters don't break the Tier-2 rerun Part of #40 --- .../src/story_automator/commands/basic.py | 10 ++++++++-- tests/test_test_counts.py | 20 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/skills/bmad-story-automator/src/story_automator/commands/basic.py b/skills/bmad-story-automator/src/story_automator/commands/basic.py index 5a334e55..6e73a128 100644 --- a/skills/bmad-story-automator/src/story_automator/commands/basic.py +++ b/skills/bmad-story-automator/src/story_automator/commands/basic.py @@ -433,10 +433,14 @@ def cmd_test_counts(args: list[str]) -> int: story = args[idx + 1] idx += 2 elif arg == "--since" and idx + 1 < len(args): + # --since is always machine-supplied (date +%s); a non-numeric value + # is a contract break. Fail loud rather than silently dropping the + # staleness gate, which would let a stale artifact pass as fresh. try: since = float(args[idx + 1]) except ValueError: - since = None + write_json({"ok": False, "error": "since_invalid", "value": args[idx + 1]}) + return 1 idx += 2 elif arg == "--write": do_write = True @@ -469,7 +473,9 @@ def cmd_test_counts(args: list[str]) -> int: if fresh: # Tier 1: trust the artifact emitted by this dev run source = "capture" elif command: # Tier 2: deterministic floor — re-run and parse what it emits - resolved = command.replace("{junit}", str(junit_path)).replace("{story}", story_file.stem) + # Shell-quote substitutions: the placeholders must be left UNquoted in the + # command template (paths with spaces/metacharacters would break bash -c). + resolved = command.replace("{junit}", shlex.quote(str(junit_path))).replace("{story}", shlex.quote(story_file.stem)) ensure_dir(junit_path.parent) rerun_exit = run_cmd("bash", "-c", resolved, cwd=repo).exit_code # non-zero is expected when tests fail if not junit_path.is_file(): diff --git a/tests/test_test_counts.py b/tests/test_test_counts.py index b2cfe18c..8d52ecc4 100644 --- a/tests/test_test_counts.py +++ b/tests/test_test_counts.py @@ -231,6 +231,26 @@ def test_invalid_policy_test_block(self) -> None: self.assertEqual(code, 1) self.assertEqual(payload["error"], "policy_invalid") + def test_invalid_since_is_an_error(self) -> None: + # A non-numeric --since must fail loud, not silently drop the staleness + # gate (which would let a stale artifact pass as a fresh capture). + self._policy(junitPath="reports/junit.xml") + self._artifact("reports/junit.xml") + code, payload = self._invoke("--since", "not-a-number") + self.assertEqual(code, 1) + self.assertEqual(payload["error"], "since_invalid") + + def test_rerun_handles_spaces_in_junit_path(self) -> None: + # Placeholders are shell-quoted, so a junit path with a space still works. + self._policy( + junitPath="re ports/junit.xml", + command="printf '' > {junit}", + ) + code, payload = self._invoke() + self.assertEqual(code, 0) + self.assertEqual(payload["source"], "rerun") + self.assertEqual(payload["test_counts"]["tests"], 5) + def test_story_not_found(self) -> None: code, payload = self._invoke(story="9-9") self.assertEqual(code, 1)