From 59fafa9357d7f295348b54f4125d9819506de21c Mon Sep 17 00:00:00 2001 From: t Date: Tue, 22 Sep 2026 14:45:42 -0700 Subject: [PATCH 1/5] Resolve artifact paths from BMAD's central TOML config (#769, #154) load_paths now reads the four central TOML layers (_bmad/config.toml, config.user.toml, custom/config.toml, custom/config.user.toml) merged as BMAD-METHOD v6.12.0's config_utils.structural_merge does for anything a path key can observe, and resolves each key the way render_skill's _resolve_short_config does: every scalar match in the merged tree, more than one refused as ambiguous. A TOML value wins over the legacy _bmad/bmm/config.yaml for every key; the YAML fills only keys the TOML lacks, and with no TOML layer the YAML is read exactly as before. Malformed or undecodable layers, blank or non-string values and ambiguous keys raise BmadConfigError naming file and key rather than falling back. --- CHANGELOG.md | 2 + docs/FEATURES.md | 1 + src/bmad_loop/bmadconfig.py | 281 ++++++++++++++++++++++--- tests/conftest.py | 47 +++++ tests/test_bmadconfig.py | 406 +++++++++++++++++++++++++++++++++++- tests/test_cli.py | 48 ++++- 6 files changed, 752 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 28c9c570d..2506f2ada 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,8 @@ breaking changes may land in a minor release. `validate` warns (`hooks.relay-stale`) while a backslash registration remains. Paths with spaces remain unsupported under the PowerShell fallback. +- Resolve artifact paths from BMAD's four-layer central `_bmad/config.toml`, falling back to `_bmad/bmm/config.yaml` only for keys the TOML lacks, so a TOML-only BMAD 6.12 install passes `validate` and runs; refuse ambiguous, blank, non-string or malformed TOML values instead of falling back (#769, #154). + - Replace stale installed relay hooks when a project moves between Windows and POSIX. - Report stale or unverifiable Codex hook trust in `validate` and `probe-adapter` diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 6e8dd4569..e028f7a99 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -754,6 +754,7 @@ verdict unverifiable rather than certifying a different launch configuration. - `bmad-loop init` installs the three `bmad-loop-*` skills (`bmad-loop-setup`, `bmad-loop-resolve`, `bmad-loop-sweep`, into `.claude/skills/` and/or `.agents/skills/`), an absolute hook registration for the installed relay, `.bmad-loop/policy.toml`, and a gitignore covering the runs dir, plugin caches, and policy.toml itself (per-machine config). Flags: `--cli` (repeatable), `--no-skills`, `--force-skills`. - `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git (including its version — a host below the **2.34** support floor gets a `git.version` **problem** and exit 1, so validate's verdict cannot disagree with run/sweep/resume's outright refusal), the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary (**probed, not just resolved**: a name that is on `PATH` but fails `--version`, typically a dead WSL/npm shim, adds an `adapter.binary-unrunnable` finding at warning severity — `adapter.binary` itself still reports ok, and validate's exit code is unchanged — [#294](https://github.com/bmad-code-org/bmad-loop/issues/294); only **packaged** profiles are probed — a project overlay's binary is resolved but never launched, so a clone cannot choose which binary this diagnostic launches; resolution still goes through your `PATH`, so what a probed name resolves to is whatever the session launch would itself run), hook registration, and the review skills the installed dev primitive actually invokes (reporting which name it resolved) — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. +- **Where the artifact paths come from** ([#769](https://github.com/bmad-code-org/bmad-loop/issues/769), [#154](https://github.com/bmad-code-org/bmad-loop/issues/154)): `implementation_artifacts`, `planning_artifacts`, `output_folder` and `repo_root` are read from BMAD's central TOML — `_bmad/config.toml`, `_bmad/config.user.toml`, `_bmad/custom/config.toml`, `_bmad/custom/config.user.toml`, each overriding the one before, merged as BMAD's renderer merges them — and from the legacy `_bmad/bmm/config.yaml`. Each key is looked up the way the renderer resolves a short config key: a key found under more than one table (e.g. both `[core]` and `[modules.bmm]`) is refused as ambiguous, naming every location. A TOML value wins over the YAML for every key; the YAML only fills a key the TOML lacks, and with no TOML layer the YAML is read as before. A malformed or non-UTF-8 layer, a blank or non-string value, or an ambiguous key is a `bmad-config` failure, never a silent fallback to the YAML. The two artifact dirs are required; `output_folder` defaults to `{project-root}/_bmad-output` and `repo_root` to the project dir. - The preflight also **names the multiplexer selection reason wherever selection resolves** (`mux.selection`, e.g. `platform default for win32`), not only when a `BMAD_LOOP_MUX_BACKEND`/`[mux] backend` choice forced it. A `fallback` selection is reported as a warning (its own label says no available backend matches this platform); a selection that outright failed is carried by `mux.preflight`, and a detection that failed by `mux.backends-detected` at warning — so a missing `mux.selection` line is normally explained by another finding (the historical unregistered-tmux fallback is the one silent exception; see the `--json` contract note in `documents.py`). On top of that, `host.win32-on-wsl-path` warns when a **native-Windows interpreter is working on a `\\wsl.localhost\...` project** ([#332](https://github.com/bmad-code-org/bmad-loop/issues/332) — see [multiplexer-backends.md](multiplexer-backends.md) for why WSL can hand a bash prompt the Windows build). Both are diagnostics only: neither changes which backend is selected (psmux _is_ correct for a `win32` interpreter) and neither flips validate's exit code. `bmad-loop diagnose` carries the same two facts in its Environment block as `sys.platform` and `win32 on WSL distro path` (`yes`/`no`). - Non-invasive: drives the upstream dev primitive unmodified — there is no fork to keep in sync — and review is just a re-invocation of it on the `done` spec. Your standard BMAD install is never modified. diff --git a/src/bmad_loop/bmadconfig.py b/src/bmad_loop/bmadconfig.py index 803ae01d6..8d756ae00 100644 --- a/src/bmad_loop/bmadconfig.py +++ b/src/bmad_loop/bmadconfig.py @@ -1,7 +1,44 @@ -"""Resolve BMAD artifact paths from _bmad/bmm/config.yaml.""" +"""Resolve BMAD artifact paths from the central TOML config or _bmad/bmm/config.yaml. + +Two sources, one answer per key (#769, #154). BMAD-METHOD v6.12.0's installer +(`tools/installer/core/manifest-generator.js` `writeCentralConfig`) writes the four +path keys' upstream homes into the central TOML — `output_folder` under ``[core]``, +`implementation_artifacts`/`planning_artifacts` under ``[modules.bmm]`` +(`src/core-skills/module.yaml`, `src/bmm-skills/module.yaml`); `repo_root` is +bmad-loop's own key and has no upstream home, so it is found wherever an operator +puts it. The legacy per-module `_bmad/bmm/config.yaml` may or may not be beside it. + +- **Layers**, lowest to highest: `_bmad/config.toml`, `_bmad/config.user.toml`, + `_bmad/custom/config.toml`, `_bmad/custom/config.user.toml` — upstream + `config_utils.load_central_config`. A missing layer is skipped; upstream requires + the base layer, but that is the renderer's gate (`install.py` reports it), not a + path-resolution one. +- **Merge**: a minimal faithful subset of upstream `config_utils.structural_merge` — + tables merge recursively and anything else is replaced by the higher layer. The + subset is exact for the four path keys because array semantics cannot reach + them: the renderer's short-key lookup (`render_skill._find_config_values`) never + descends into an array and never matches an array value, and upstream's array + merge only ever yields an array where both sides were arrays — the same *type* + this merge leaves at that spot. What the subset does not reproduce is upstream + refusing a malformed keyed array (`code`/`id` not a non-empty string); that is a + renderer diagnosis about a table no path key lives in. +- **Lookup**: each key resolves as `render_skill._resolve_short_config` resolves a + ``{{.key}}`` token — every scalar entry of that name anywhere in the merged tree, + and more than one is refused as ambiguous, naming each location and its layer. + #154's "`[modules.bmm]` beats `[core]`" flattening is obsolete and not done. +- **Precedence**: a TOML value wins over the YAML for every key; the YAML fills a + key only when the merged TOML has no entry of that name. Everything wrong with a + present TOML source — an undecodable or malformed layer, a blank or non-string + value, an ambiguous key — raises rather than falling back, because a fallback + would silently run against paths the operator did not choose. +- With no TOML layer at all the YAML is read exactly as it always was. + +`{project-root}` is substituted with the renderer's spelling (a literal replace +with the canonical root) by `_resolve`, for both sources.""" from __future__ import annotations +import tomllib from dataclasses import dataclass, field from pathlib import Path @@ -82,7 +119,7 @@ def worktree_isolation_conflict(paths: ProjectPaths, isolation: str) -> str | No itself is pointed at, never copied. The `MODULE_SKILLS` this wheel bundles are seeded from package data and are unaffected by either root; nothing is seeded from ``project``, which `provision_worktree` is never even passed.) - `load_paths` *requires* `project/_bmad/bmm/config.yaml`, so `_bmad/` is under + `load_paths` *requires* its config under `project/_bmad/`, so `_bmad/` is under `project` by definition and `repo_root/_bmad/` generally does not exist. When the two diverge the preflight therefore approves a surface the isolated run never receives, and the seed-completeness gates go inert rather than fire: an @@ -134,7 +171,8 @@ def worktree_isolation_conflict(paths: ProjectPaths, isolation: str) -> str | No f"directory: worktree provisioning seeds from repo_root ({paths.repo_root}) while " f"init, validate and the run preflight read the project ({paths.project}), so an " "isolated session would get none of the skills the preflight just approved. " - "Remove the `repo_root` key from _bmad/bmm/config.yaml, or set " + "Remove the `repo_root` key from the BMAD config (_bmad/bmm/config.yaml or a " + "_bmad/ TOML layer), or set " '`isolation = "none"` under [scm] in .bmad-loop/policy.toml.' ) @@ -155,8 +193,9 @@ def _canonical(expanded: Path, label: str) -> Path: raise BmadConfigError(_diagnostic_text(message)) from e -def _resolve(raw: str, project: Path) -> Path: - """Expand `{project-root}` and canonicalize, or raise typed. A config string can +def _resolve(raw: str, project: Path, origin: str) -> Path: + """Expand `{project-root}` and canonicalize, or raise typed. `origin` names the + key and file the string came from, for the refusal. A config string can name a UNC share of its own, independent of `--project`, so it refuses on the same terms as the root in `load_paths` (#552). Degrading to the lexical spelling was tried and retired: a spelling the OS cannot canonicalize has an *unknowable* @@ -166,7 +205,158 @@ def _resolve(raw: str, project: Path) -> Path: sends a worktree-isolated run's artifact writes into a worktree-local directory instead of the configured destination. No member enters a snapshot unresolved.""" expanded = Path(raw.replace("{project-root}", str(project))) - return _canonical(expanded, f"configured path {raw!r}") + return _canonical(expanded, f"configured path {raw!r} ({origin})") + + +LEGACY_CONFIG_REL = Path("_bmad") / "bmm" / "config.yaml" +# Lowest to highest — upstream `config_utils.load_central_config` (v6.12.0). +CENTRAL_LAYERS_REL: tuple[Path, ...] = ( + Path("_bmad") / "config.toml", + Path("_bmad") / "config.user.toml", + Path("_bmad") / "custom" / "config.toml", + Path("_bmad") / "custom" / "config.user.toml", +) +_REQUIRED_KEYS = ("implementation_artifacts", "planning_artifacts") +_PATH_KEYS = (*_REQUIRED_KEYS, "repo_root", "output_folder") + + +@dataclass(frozen=True) +class _Leaf: + """A non-table value in the merged central config, with the layer that set it.""" + + value: object + layer: Path + + +def _load_layer(path: Path) -> dict[str, object] | None: + """One central TOML layer, None when absent. Present-but-unusable raises: the + caller must never read an unparseable layer as "no TOML" and fall back.""" + if not path.exists(): + return None + if not path.is_file(): + raise BmadConfigError(_diagnostic_text(f"BMAD config layer is not a file: {path}")) + try: + # tomllib decodes as UTF-8 itself; a bad byte surfaces as UnicodeDecodeError + # (a ValueError), not TOMLDecodeError — both are this layer's fault + with path.open("rb") as stream: + return tomllib.load(stream) + except UnicodeDecodeError as e: + raise BmadConfigError(_diagnostic_text(f"{path} is not valid UTF-8: {e}")) from e + except tomllib.TOMLDecodeError as e: + raise BmadConfigError(_diagnostic_text(f"invalid TOML in {path}: {e}")) from e + except OSError as e: + raise BmadConfigError(_diagnostic_text(f"cannot read {path}: {e}")) from e + + +class _Table(dict[str, object]): + """A table in the merged central config, with every layer that contributed to it + — a table has no single source the way a leaf does.""" + + def __init__(self, entries: dict[str, object], layers: tuple[Path, ...]) -> None: + super().__init__(entries) + self.layers = layers + + +def _overlay(base: dict[str, object], layer: dict[str, object], source: Path) -> dict[str, object]: + """`structural_merge` for everything a path key can observe (module docstring): + tables merge, any other value replaces, and each node remembers its layers.""" + merged = dict(base) + for key, value in layer.items(): + below = merged.get(key) + if isinstance(value, dict): + if isinstance(below, _Table): + merged[key] = _Table(_overlay(below, value, source), (*below.layers, source)) + else: + merged[key] = _Table(_overlay({}, value, source), (source,)) + else: + merged[key] = _Leaf(value, source) + return merged + + +def _load_central(project: Path) -> dict[str, object] | None: + """The merged four-layer central config, or None when no layer exists.""" + merged: dict[str, object] | None = None + for rel in CENTRAL_LAYERS_REL: + layer = _load_layer(project / rel) + if layer is not None: + merged = _overlay(merged or {}, layer, project / rel) + return merged + + +def _find(tree: dict[str, object], key: str, prefix: str = "") -> list[tuple[str, object]]: + """Every entry named `key`, depth-first in document order, as (dotted path, node). + Arrays are opaque, as in `render_skill._find_config_values`.""" + found: list[tuple[str, object]] = [] + for name, node in tree.items(): + dotted = f"{prefix}.{name}" if prefix else name + if name == key: + found.append((dotted, node)) + if isinstance(node, dict): + found.extend(_find(node, key, dotted)) + return found + + +def _central_value(central: dict[str, object], key: str) -> tuple[str, str] | None: + """(value, origin) for `key` the way the renderer resolves a short config token, + or None when the merged config has no entry of that name. Scalars are the + renderer's candidates; an array or table under the name is not one, but it is a + present non-string value, so it refuses rather than letting the YAML fill in.""" + found = _find(central, key) + scalars = [ + (dotted, node) + for dotted, node in found + if isinstance(node, _Leaf) and not isinstance(node.value, list) + ] + if len(scalars) > 1: + where = ", ".join(f"{dotted} ({leaf.layer})" for dotted, leaf in scalars) + raise BmadConfigError( + _diagnostic_text( + f"ambiguous config value `{key}` found at: {where} — the BMAD renderer " + "refuses the same config; keep exactly one entry of that name across " + "the _bmad/ TOML layers" + ) + ) + if scalars: + dotted, leaf = scalars[0] + assert isinstance(leaf, _Leaf) + origin = f"`{dotted}` in {leaf.layer}" + value = leaf.value + if not isinstance(value, str): + raise BmadConfigError( + _diagnostic_text(f"{origin} must be a string, got {type(value).__name__}") + ) + if not value.strip(): + raise BmadConfigError(_diagnostic_text(f"{origin} must not be empty")) + return value, origin + if found: + dotted, node = found[0] + if isinstance(node, _Leaf): + origin = f"`{dotted}` in {node.layer}" + kind = type(node.value).__name__ + else: + assert isinstance(node, _Table) + origin = f"`{dotted}` in {', '.join(str(layer) for layer in node.layers)}" + kind = "table" + raise BmadConfigError(_diagnostic_text(f"{origin} must be a string, got {kind}")) + return None + + +def _load_legacy(config_path: Path) -> dict[object, object]: + """The legacy `_bmad/bmm/config.yaml` mapping, raising typed on any fault.""" + try: + # UnicodeDecodeError is a ValueError, not an OSError, so an undecodable file + # would otherwise escape every caller's `except BmadConfigError` and crash + # them. Same reasoning as `policy.load`. + raw = config_path.read_text(encoding="utf-8") + except UnicodeDecodeError as e: + raise BmadConfigError(f"{config_path} is not valid UTF-8: {e}") from e + try: + doc = yaml.safe_load(raw) or {} + except yaml.YAMLError as e: + raise BmadConfigError(f"invalid YAML in {config_path}: {e}") from e + if not isinstance(doc, dict): + raise BmadConfigError(f"{config_path} must contain a top-level mapping") + return doc def load_paths(project: Path) -> ProjectPaths: @@ -191,35 +381,68 @@ def load_paths(project: Path) -> ProjectPaths: "Run `bmad-loop validate` for what this host is doing." ) raise BmadConfigError(_diagnostic_text(message)) from e - config_path = project / "_bmad" / "bmm" / "config.yaml" - if not config_path.is_file(): - raise BmadConfigError(f"BMAD config not found: {config_path} (is BMAD installed here?)") - try: - # UnicodeDecodeError is a ValueError, not an OSError, so an undecodable file - # would otherwise escape every caller's `except BmadConfigError` and crash - # them. Same reasoning as `policy.load`. - raw = config_path.read_text(encoding="utf-8") - except UnicodeDecodeError as e: - raise BmadConfigError(f"{config_path} is not valid UTF-8: {e}") from e - try: - doc = yaml.safe_load(raw) or {} - except yaml.YAMLError as e: - raise BmadConfigError(f"invalid YAML in {config_path}: {e}") from e - if not isinstance(doc, dict): - raise BmadConfigError(f"{config_path} must contain a top-level mapping") + config_path = project / LEGACY_CONFIG_REL + central = _load_central(project) + if central is None: + if not config_path.is_file(): + layers = ", ".join(str(project / rel) for rel in CENTRAL_LAYERS_REL) + raise BmadConfigError( + f"BMAD config not found: neither the central TOML ({layers}) nor " + f"{config_path} exists (is BMAD installed here?)" + ) + return _paths_from_legacy(project, config_path, _load_legacy(config_path)) + + # Mixed or TOML-only: TOML wins per key; the YAML is read only when some key is + # absent from the merged TOML, and only if it exists. + legacy: dict[object, object] | None = None + + def lookup(key: str) -> tuple[str, str] | None: + nonlocal legacy + hit = _central_value(central, key) + if hit is not None: + return hit + if legacy is None: + legacy = _load_legacy(config_path) if config_path.is_file() else {} + raw = legacy.get(key) + return (str(raw), f"`{key}` in {config_path}") if raw else None + + found = {key: lookup(key) for key in _PATH_KEYS} + for key in _REQUIRED_KEYS: + if found[key] is None: + layers = ", ".join(str(project / rel) for rel in CENTRAL_LAYERS_REL) + raise BmadConfigError( + _diagnostic_text( + f"missing `{key}`: not in the central TOML ({layers}) nor in {config_path}" + ) + ) + return _assemble(project, found) + +def _paths_from_legacy(project: Path, config_path: Path, doc: dict[object, object]) -> ProjectPaths: + """Today's YAML-only behavior, unchanged: a falsy value is an absent key.""" impl = doc.get("implementation_artifacts") plan = doc.get("planning_artifacts") if not impl or not plan: raise BmadConfigError( f"{config_path} missing implementation_artifacts/planning_artifacts keys" ) - repo_root_raw = doc.get("repo_root") - repo_root = _resolve(str(repo_root_raw), project) if repo_root_raw else project - out_raw = doc.get("output_folder") + found: dict[str, tuple[str, str] | None] = {} + for key in _PATH_KEYS: + raw = doc.get(key) + found[key] = (str(raw), f"`{key}` in {config_path}") if raw else None + return _assemble(project, found) + + +def _assemble(project: Path, found: dict[str, tuple[str, str] | None]) -> ProjectPaths: + """Canonicalize every member of the snapshot; `found` maps each key to its + (raw value, origin) or None, and the two required keys are never None here.""" + impl, plan = found["implementation_artifacts"], found["planning_artifacts"] + assert impl is not None and plan is not None + repo_root_hit, out_hit = found["repo_root"], found["output_folder"] + repo_root = _resolve(repo_root_hit[0], project, repo_root_hit[1]) if repo_root_hit else project output_folder = ( - _resolve(str(out_raw), project) - if out_raw + _resolve(out_hit[0], project, out_hit[1]) + if out_hit # the default branch is a bare join off the (canonical) root and takes the # same canonicalize-or-raise treatment as a configured string: an in-tree # junction under the default name misclassifies exactly like a configured one. @@ -227,8 +450,8 @@ def load_paths(project: Path) -> ProjectPaths: ) return ProjectPaths( project=project, - implementation_artifacts=_resolve(str(impl), project), - planning_artifacts=_resolve(str(plan), project), + implementation_artifacts=_resolve(impl[0], project, impl[1]), + planning_artifacts=_resolve(plan[0], project, plan[1]), output_folder=output_folder, repo_root=repo_root, ) diff --git a/tests/conftest.py b/tests/conftest.py index 645bb9537..70195e2f1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1247,6 +1247,53 @@ def install_bmad_config(paths: ProjectPaths) -> None: cfg.write_text(_ARTIFACT_PATH_KEYS) +# The #769 layout: what BMAD-METHOD v6.12.0's installer writes with no legacy +# `_bmad/bmm/config.yaml` beside it. Derived from the source at tag v6.12.0 — +# `tools/installer/core/manifest-generator.js` `writeCentralConfig` (team-scope +# answers to `[core]` / `[modules.]` in config.toml, user-scope ones to +# config.user.toml, `[agents.]` always team) and `ensureCustomConfigStubs` +# (comment-only custom layers); keys and `{project-root}/{value}` results from +# `src/core-skills/module.yaml` and `src/bmm-skills/module.yaml` at their defaults. +# Not captured from a live `bmad setup` — none can run here. +CENTRAL_TEAM_CONFIG = """\ +[core] +project_name = "sandbox" +document_output_language = "English" +output_folder = "{project-root}/_bmad-output" + +[modules.bmm] +planning_artifacts = "{project-root}/_bmad-output/planning-artifacts" +implementation_artifacts = "{project-root}/_bmad-output/implementation-artifacts" +project_knowledge = "{project-root}/docs" + +[agents.bmad-agent-dev] +module = "bmm" +team = "software-development" +name = "Amelia" +title = "Senior Software Engineer" +""" +CENTRAL_USER_CONFIG = """\ +[core] +user_name = "BMad" +communication_language = "English" + +[modules.bmm] +user_skill_level = "intermediate" +""" +CENTRAL_CUSTOM_STUB = "# Team / enterprise overrides for _bmad/config.toml.\n" +CENTRAL_CUSTOM_USER_STUB = "# Personal overrides for _bmad/config.toml.\n" + + +def install_bmad_central_config(paths: ProjectPaths) -> None: + """Write the TOML-only four-layer layout (#769) and no `_bmad/bmm/config.yaml`.""" + bmad = paths.project / "_bmad" + (bmad / "custom").mkdir(parents=True, exist_ok=True) + (bmad / "config.toml").write_text(CENTRAL_TEAM_CONFIG, encoding="utf-8") + (bmad / "config.user.toml").write_text(CENTRAL_USER_CONFIG, encoding="utf-8") + (bmad / "custom" / "config.toml").write_text(CENTRAL_CUSTOM_STUB, encoding="utf-8") + (bmad / "custom" / "config.user.toml").write_text(CENTRAL_CUSTOM_USER_STUB, encoding="utf-8") + + def _write_skill_stubs(skills: Path, catalog: dict) -> None: """Stub every skill in `catalog` (an install.py {skill: marker_files} map) under `skills`. Reading the catalog instead of restating it means a newly required diff --git a/tests/test_bmadconfig.py b/tests/test_bmadconfig.py index 421b4eae7..12c574874 100644 --- a/tests/test_bmadconfig.py +++ b/tests/test_bmadconfig.py @@ -1,7 +1,8 @@ """ProjectPaths.repo_root / rebased and load_paths(repo_root) — the Phase 1 Workspace-seam foundation. repo_root defaults to project (today's behavior); rebased re-roots artifacts onto a worktree-style checkout. Plus -worktree_isolation_conflict, the #414 refusal predicate built on the same pair.""" +worktree_isolation_conflict, the #414 refusal predicate built on the same pair, and +load_paths' two sources: the four-layer central TOML and the legacy YAML (#769).""" from __future__ import annotations @@ -11,8 +12,11 @@ import pytest from conftest import ( + CENTRAL_TEAM_CONFIG, + CENTRAL_USER_CONFIG, NUL_PATH_RESOLVE_FAULTS, UNRESOLVABLE, + install_bmad_central_config, install_bmad_config, refuse_to_resolve, ) @@ -396,3 +400,403 @@ def test_load_paths_refuses_a_degraded_root_beside_canonically_spelled_config_pa # the full prefix: this row pins the ROOT refusing, not a member's shared stem with pytest.raises(bmadconfig.BmadConfigError, match="cannot canonicalize the project root"): bmadconfig.load_paths(root) + + +# ------------- the four-layer central TOML config (#769, #154) ------------- +# +# Fixtures follow BMAD-METHOD v6.12.0 (see `CENTRAL_TEAM_CONFIG` in conftest for the +# installer source they are derived from); resolution follows that tag's +# `src/scripts/config_utils.py` (layers, structural merge) and +# `src/scripts/render_skill.py` `_resolve_short_config` (short-key lookup). + +_LAYERS = [str(rel) for rel in bmadconfig.CENTRAL_LAYERS_REL] +_DEFAULT_IMPL = "_bmad-output/implementation-artifacts" + + +def _write_layer(root: Path, index: int, text: str | bytes) -> Path: + path = root / bmadconfig.CENTRAL_LAYERS_REL[index] + path.parent.mkdir(parents=True, exist_ok=True) + if isinstance(text, bytes): + path.write_bytes(text) + else: + path.write_text(text, encoding="utf-8") + return path + + +def _toml_only(tmp_path: Path) -> Path: + root = tmp_path / "p" + root.mkdir() + _write_layer(root, 0, CENTRAL_TEAM_CONFIG) + _write_layer(root, 1, CENTRAL_USER_CONFIG) + return root + + +def test_load_paths_reads_the_769_toml_only_layout(project) -> None: + """The reported install: `_bmad/config.toml` and friends, no `_bmad/bmm/`.""" + install_bmad_central_config(project) + root = project.project.resolve() + assert not (root / bmadconfig.LEGACY_CONFIG_REL).exists() + + loaded = bmadconfig.load_paths(project.project) + + assert loaded.implementation_artifacts == root / _DEFAULT_IMPL + assert loaded.planning_artifacts == root / "_bmad-output" / "planning-artifacts" + assert loaded.output_folder == root / "_bmad-output" + assert loaded.repo_root == root + + +def test_toml_only_and_yaml_only_installs_resolve_identically(project, tmp_path) -> None: + """The same install answers through either source give the same snapshot.""" + install_bmad_central_config(project) + from_toml = bmadconfig.load_paths(project.project) + (project.project / "_bmad" / "config.toml").unlink() + (project.project / "_bmad" / "config.user.toml").unlink() + (project.project / "_bmad" / "custom" / "config.toml").unlink() + (project.project / "_bmad" / "custom" / "config.user.toml").unlink() + install_bmad_config(project) + assert bmadconfig.load_paths(project.project) == from_toml + + +@pytest.mark.parametrize("upper", [1, 2, 3]) +def test_each_layer_overrides_the_one_below(tmp_path: Path, upper: int) -> None: + root = tmp_path / "p" + root.mkdir() + _write_layer(root, 0, CENTRAL_TEAM_CONFIG) + if upper > 1: + _write_layer( + root, upper - 1, '[modules.bmm]\nimplementation_artifacts = "{project-root}/lower"\n' + ) + _write_layer(root, upper, '[modules.bmm]\nimplementation_artifacts = "{project-root}/upper"\n') + + loaded = bmadconfig.load_paths(root) + + assert loaded.implementation_artifacts == root.resolve() / "upper" + # tables merge rather than replace: the base layer's sibling key survives + assert loaded.planning_artifacts == root.resolve() / "_bmad-output" / "planning-artifacts" + + +def test_a_lower_layer_does_not_beat_a_higher_one(tmp_path: Path) -> None: + """Order, not presence: the base layer's value loses to the custom user layer's + even with the layers between them silent.""" + root = tmp_path / "p" + root.mkdir() + _write_layer(root, 0, CENTRAL_TEAM_CONFIG) + _write_layer(root, 3, '[core]\noutput_folder = "{project-root}/mine"\n') + assert bmadconfig.load_paths(root).output_folder == root.resolve() / "mine" + + +@pytest.mark.parametrize( + ("layer", "text", "expected"), + [ + # the #154 shape: the same key under [core] and [modules.bmm] + ( + 0, + '[core]\nimplementation_artifacts = "{project-root}/core"\n', + "core.implementation_artifacts", + ), + # an override layer adding the key in a section of its own + ( + 2, + '[core]\nimplementation_artifacts = "{project-root}/core"\n', + "core.implementation_artifacts", + ), + # arbitrarily deep, as the renderer searches the whole tree + ( + 3, + '[agents.bmad-agent-dev]\nimplementation_artifacts = "x"\n', + "agents.bmad-agent-dev.implementation_artifacts", + ), + ], +) +def test_a_key_in_two_sections_is_ambiguous( + tmp_path: Path, layer: int, text: str, expected: str +) -> None: + """The renderer refuses a short key matched more than once, and so does this — + no "`[modules.bmm]` beats `[core]`" tie-break (#154's obsolete proposal), and no + falling back to the YAML, which is present here and would resolve cleanly.""" + root = _toml_only(tmp_path) + _write_config(root) # a valid legacy YAML beside it must not rescue the load + base = (root / bmadconfig.CENTRAL_LAYERS_REL[0]).read_text(encoding="utf-8") + if layer == 0: + _write_layer(root, 0, base.replace("[core]\n", text, 1)) + else: + _write_layer(root, layer, text) + + with pytest.raises(bmadconfig.BmadConfigError) as excinfo: + bmadconfig.load_paths(root) + + message = str(excinfo.value) + assert "ambiguous config value `implementation_artifacts` found at:" in message + assert "modules.bmm.implementation_artifacts" in message + assert expected in message + # every location names the layer it came from + assert str(root.resolve() / _LAYERS[0]) in message + assert str(root.resolve() / _LAYERS[layer]) in message + + +def test_the_same_path_in_two_layers_is_an_override_not_an_ambiguity(tmp_path: Path) -> None: + root = _toml_only(tmp_path) + _write_layer(root, 2, '[core]\noutput_folder = "{project-root}/team-out"\n') + assert bmadconfig.load_paths(root).output_folder == root.resolve() / "team-out" + + +@pytest.mark.parametrize("key", ["implementation_artifacts", "planning_artifacts"]) +def test_a_required_key_absent_from_both_sources_fails(tmp_path: Path, key: str) -> None: + root = _toml_only(tmp_path) + kept = CENTRAL_TEAM_CONFIG.replace(f"{key} = ", f"unused_{key} = ") + _write_layer(root, 0, kept) + + with pytest.raises(bmadconfig.BmadConfigError, match=f"missing `{key}`") as excinfo: + bmadconfig.load_paths(root) + assert str(root.resolve() / bmadconfig.LEGACY_CONFIG_REL) in str(excinfo.value) + + +def test_optional_keys_absent_from_both_sources_keep_their_defaults(tmp_path: Path) -> None: + root = tmp_path / "p" + root.mkdir() + _write_layer( + root, + 0, + '[modules.bmm]\nimplementation_artifacts = "{project-root}/i"\n' + 'planning_artifacts = "{project-root}/pl"\n', + ) + loaded = bmadconfig.load_paths(root) + assert loaded.output_folder == root.resolve() / "_bmad-output" + assert loaded.repo_root == root.resolve() + + +@pytest.mark.parametrize( + ("value", "complaint"), + [ + ('""', "must not be empty"), + ('" "', "must not be empty"), + ("42", "must be a string, got int"), + ("true", "must be a string, got bool"), + ('["{project-root}/a"]', "must be a string, got list"), + ('{ path = "{project-root}/a" }', "must be a string, got table"), + ], +) +@pytest.mark.parametrize("key", ["implementation_artifacts", "output_folder", "repo_root"]) +def test_a_blank_or_wrong_typed_toml_value_refuses_instead_of_falling_back( + tmp_path: Path, key: str, value: str, complaint: str +) -> None: + """Present-but-unusable is an error, not an absence: the legacy YAML beside it + carries a perfectly good value for every key and must not be consulted.""" + root = tmp_path / "p" + root.mkdir() + _write_config( + root, + output_folder="{project-root}/yaml-out", + repo_root="{project-root}", + ) + body = ( + CENTRAL_TEAM_CONFIG.replace(f"{key} = ", f"unused_{key} = ") + + f"\n[custom]\n{key} = {value}\n" + ) + _write_layer(root, 0, body) + + with pytest.raises(bmadconfig.BmadConfigError, match=complaint) as excinfo: + bmadconfig.load_paths(root) + message = str(excinfo.value) + assert f"custom.{key}" in message, "the error names the key" + assert str(root.resolve() / _LAYERS[0]) in message, "the error names the file" + + +def test_an_array_of_tables_is_opaque_to_the_lookup(tmp_path: Path) -> None: + """The renderer never descends into arrays, so a same-named key inside an array + of tables is neither a match nor an ambiguity — and neither is it here.""" + root = _toml_only(tmp_path) + _write_layer( + root, + 2, + '[[extras]]\nid = "a"\nimplementation_artifacts = "{project-root}/nope"\n', + ) + loaded = bmadconfig.load_paths(root) + assert loaded.implementation_artifacts == root.resolve() / _DEFAULT_IMPL + + +def test_a_higher_layer_scalar_replaces_a_lower_table(tmp_path: Path) -> None: + """`structural_merge` replaces unless both sides are tables: a scalar `modules` + in an override layer removes `[modules.bmm]` wholesale, so its keys are gone.""" + root = _toml_only(tmp_path) + _write_layer(root, 3, 'modules = "gone"\n') + with pytest.raises(bmadconfig.BmadConfigError, match="missing `implementation_artifacts`"): + bmadconfig.load_paths(root) + + +# --- mixed installs: TOML wins per key, YAML fills only what TOML lacks --- + + +@pytest.mark.parametrize( + "key", ["implementation_artifacts", "planning_artifacts", "output_folder", "repo_root"] +) +def test_mixed_install_toml_value_wins_over_yaml(tmp_path: Path, key: str) -> None: + root = tmp_path / "p" + root.mkdir() + _write_config(root, **{key: "{project-root}/from-yaml"}) + _write_layer(root, 0, CENTRAL_TEAM_CONFIG) + _write_layer(root, 2, f'[custom]\n{key} = "{{project-root}}/from-toml"\n') + if key != "repo_root": + # move the base layer's own entry aside so the custom one is the only match + base = CENTRAL_TEAM_CONFIG.replace(f"{key} = ", f"unused_{key} = ") + _write_layer(root, 0, base) + + loaded = bmadconfig.load_paths(root) + assert getattr(loaded, key) == root.resolve() / "from-toml" + + +@pytest.mark.parametrize( + "key", ["implementation_artifacts", "planning_artifacts", "output_folder", "repo_root"] +) +def test_mixed_install_yaml_fills_a_key_absent_from_toml(tmp_path: Path, key: str) -> None: + root = tmp_path / "p" + root.mkdir() + _write_config(root, **{key: "{project-root}/from-yaml"}) + _write_layer(root, 0, CENTRAL_TEAM_CONFIG.replace(f"{key} = ", f"unused_{key} = ")) + + loaded = bmadconfig.load_paths(root) + assert getattr(loaded, key) == root.resolve() / "from-yaml" + if key != "implementation_artifacts": + # and the keys TOML does carry are still TOML's + assert loaded.implementation_artifacts == root.resolve() / _DEFAULT_IMPL + + +def test_mixed_install_reports_a_yaml_filled_key_by_its_yaml_origin( + tmp_path: Path, monkeypatch +) -> None: + root = tmp_path / "p" + root.mkdir() + target = tmp_path / "elsewhere" + _write_config(root, repo_root=str(target)) + _write_layer(root, 0, CENTRAL_TEAM_CONFIG) + refuse_to_resolve(monkeypatch, target) + + with pytest.raises(bmadconfig.BmadConfigError, match="cannot canonicalize") as excinfo: + bmadconfig.load_paths(root) + assert f"`repo_root` in {root.resolve() / bmadconfig.LEGACY_CONFIG_REL}" in str(excinfo.value) + + +@pytest.mark.parametrize("layer", [0, 1, 2, 3]) +def test_malformed_toml_in_any_layer_refuses_instead_of_falling_back( + tmp_path: Path, layer: int +) -> None: + root = tmp_path / "p" + root.mkdir() + _write_config(root) # a valid legacy YAML must not rescue the load + for index in range(4): + _write_layer(root, index, CENTRAL_TEAM_CONFIG if index == 0 else "") + _write_layer(root, layer, "[core\nbroken = \n") + + with pytest.raises(bmadconfig.BmadConfigError, match="invalid TOML in") as excinfo: + bmadconfig.load_paths(root) + assert str(root.resolve() / _LAYERS[layer]) in str(excinfo.value) + + +@pytest.mark.parametrize("layer", [0, 1, 2, 3]) +def test_undecodable_toml_in_any_layer_refuses_instead_of_falling_back( + tmp_path: Path, layer: int +) -> None: + """tomllib raises UnicodeDecodeError — a ValueError, not TOMLDecodeError — so it + needs its own conversion or it escapes every `except BmadConfigError`.""" + root = tmp_path / "p" + root.mkdir() + _write_config(root) + _write_layer(root, 0, CENTRAL_TEAM_CONFIG) + _write_layer(root, layer, b'[core]\nuser_name = "\xff\xfe"\n') + + with pytest.raises(bmadconfig.BmadConfigError, match="not valid UTF-8") as excinfo: + bmadconfig.load_paths(root) + assert str(root.resolve() / _LAYERS[layer]) in str(excinfo.value) + + +def test_a_layer_that_is_not_a_file_refuses(tmp_path: Path) -> None: + root = _toml_only(tmp_path) + (root / bmadconfig.CENTRAL_LAYERS_REL[2]).mkdir(parents=True) + with pytest.raises(bmadconfig.BmadConfigError, match="not a file"): + bmadconfig.load_paths(root) + + +def test_no_toml_and_no_yaml_names_both_expected_locations(tmp_path: Path) -> None: + root = tmp_path / "p" + root.mkdir() + with pytest.raises(bmadconfig.BmadConfigError, match="BMAD config not found") as excinfo: + bmadconfig.load_paths(root) + message = str(excinfo.value) + assert str(root.resolve() / _LAYERS[0]) in message + assert str(root.resolve() / bmadconfig.LEGACY_CONFIG_REL) in message + + +def test_yaml_only_keeps_its_falsy_means_absent_semantics(tmp_path: Path) -> None: + """With no TOML layer the legacy reading is untouched: a blank YAML value is an + absent key, not the refusal a blank TOML value gets.""" + root = tmp_path / "p" + root.mkdir() + _write_config(root, output_folder="") + assert bmadconfig.load_paths(root).output_folder == root.resolve() / "_bmad-output" + + +# --- the #552 / worktree guarantees hold for a TOML-sourced path --- + + +def test_a_toml_path_that_cannot_canonicalize_refuses_naming_file_and_key( + tmp_path: Path, monkeypatch +) -> None: + root = _toml_only(tmp_path) + target = root.resolve() / "_bmad-output" / "implementation-artifacts" + refuse_to_resolve(monkeypatch, target) + + with pytest.raises( + bmadconfig.BmadConfigError, match="cannot canonicalize the configured path" + ) as excinfo: + bmadconfig.load_paths(root) + message = str(excinfo.value) + assert "`modules.bmm.implementation_artifacts`" in message + assert str(root.resolve() / _LAYERS[0]) in message + + +def test_a_toml_path_escaping_the_project_is_canonical_and_stays_put_on_rebase( + tmp_path: Path, +) -> None: + """`{project-root}/../shared` escapes the tree: it canonicalizes to the shared + directory, and `rebased` leaves it where it is as for any external path.""" + root = _toml_only(tmp_path) + _write_layer( + root, 2, '[modules.bmm]\nimplementation_artifacts = "{project-root}/../shared/impl"\n' + ) + loaded = bmadconfig.load_paths(root) + assert loaded.implementation_artifacts == (tmp_path / "shared" / "impl").resolve() + + wt = tmp_path / "wt" + rebased = loaded.rebased(wt) + assert rebased.implementation_artifacts == (tmp_path / "shared" / "impl").resolve() + assert rebased.planning_artifacts == (wt / "_bmad-output" / "planning-artifacts").resolve() + + +def test_a_symlinked_toml_path_is_classified_by_its_target(tmp_path: Path) -> None: + """Spelled inside the project, pointing outside: canonicalization follows the + link, so `rebased` files it as external rather than per-checkout.""" + root = _toml_only(tmp_path) + outside = tmp_path / "outside" + outside.mkdir() + try: + (root / "linked").symlink_to(outside, target_is_directory=True) + except OSError as e: # Windows without SeCreateSymbolicLink / developer mode + pytest.skip(f"cannot create a symlink here: {e}") + _write_layer(root, 3, '[modules.bmm]\nimplementation_artifacts = "{project-root}/linked"\n') + + loaded = bmadconfig.load_paths(root) + assert loaded.implementation_artifacts == outside.resolve() + assert loaded.rebased(tmp_path / "wt").implementation_artifacts == outside.resolve() + + +def test_rebased_reroots_a_toml_sourced_config(tmp_path: Path) -> None: + root = _toml_only(tmp_path) + loaded = bmadconfig.load_paths(root) + wt = tmp_path / "worktree" + + rebased = loaded.rebased(wt) + + assert rebased.project == rebased.repo_root == wt.resolve() + assert rebased.implementation_artifacts == (wt / _DEFAULT_IMPL).resolve() + assert rebased.output_folder == (wt / "_bmad-output").resolve() + assert rebased.sprint_status == (wt / _DEFAULT_IMPL / "sprint-status.yaml").resolve() diff --git a/tests/test_cli.py b/tests/test_cli.py index 141382c03..cb88ce985 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -29,6 +29,7 @@ fault_read_text, git, ignore_before_commit, + install_bmad_central_config, install_bmad_config, install_build_auto_skill, install_dev_base_skills, @@ -9992,7 +9993,15 @@ def test_validate_stories_folder_known_selector_ok(project): CLAUDE_ONLY_POLICY = '[adapter]\nname = "claude"\nmodel = "opus"\n' -def _make_validate_pass(project, monkeypatch, capsys, *, policy=CLAUDE_ONLY_POLICY, skills=None): +def _make_validate_pass( + project, + monkeypatch, + capsys, + *, + policy=CLAUDE_ONLY_POLICY, + skills=None, + bmad_config=install_bmad_config, +): """Set a project up so every validate gate passes, and pin the gates whose outcome is a property of the *host* rather than of the project: whether the CLI binary is on PATH, whether it actually runs, and whether a multiplexer is @@ -10010,8 +10019,9 @@ def _make_validate_pass(project, monkeypatch, capsys, *, policy=CLAUDE_ONLY_POLI tree) while keeping every other gate green — an rc-0 assertion about one check is worthless if some unrelated gate is what is actually failing. ``skills`` is called with the project root BEFORE the commit, so whatever it lays down is committed and - the worktree-clean gate still passes.""" - install_bmad_config(project) + the worktree-clean gate still passes. ``bmad_config`` lays down the BMAD config + source — the legacy YAML by default, or the central TOML layout (#769).""" + bmad_config(project) _write_policy(project.project, policy) write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) if skills is None: @@ -10125,6 +10135,38 @@ def test_validate_reports_an_undecodable_bmad_config_instead_of_crashing(project assert "not valid UTF-8" in finding["message"] +def test_validate_passes_bmad_config_on_a_toml_only_project(project, capsys, monkeypatch): + """#769: a v6.12 install with only the central `_bmad/config.toml` layers (no + `_bmad/bmm/config.yaml`) used to FAIL `bmad-config` and block every run.""" + _make_validate_pass(project, monkeypatch, capsys, bmad_config=install_bmad_central_config) + assert not (project.project / "_bmad" / "bmm" / "config.yaml").exists() + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys) + finding = next(f for f in doc["findings"] if f["check"] == "bmad-config") + assert finding["severity"] == "ok" + expected = project.project.resolve() / "_bmad-output" / "implementation-artifacts" + assert finding["detail"]["implementation_artifacts"] == str(expected) + assert doc["ok"] is True + + +def test_validate_fails_bmad_config_on_an_ambiguous_toml_key(project, capsys): + """The renderer's refusal, surfaced where validate reports config faults: the + same short key under two tables is a `bmad-config` problem naming both, not a + silent pick of either.""" + _write_policy(project.project, CLAUDE_ONLY_POLICY) + install_bmad_central_config(project) + (project.project / "_bmad" / "custom" / "config.toml").write_text( + '[core]\nimplementation_artifacts = "{project-root}/elsewhere"\n', encoding="utf-8" + ) + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys, rc=1) + finding = next(f for f in doc["findings"] if f["check"] == "bmad-config") + assert finding["severity"] == "problem" + assert "ambiguous config value `implementation_artifacts`" in finding["message"] + assert "modules.bmm.implementation_artifacts" in finding["message"] + assert "core.implementation_artifacts" in finding["message"] + + def test_validate_reports_an_undecodable_profile_overlay_instead_of_crashing(project, capsys): """#473: the third loader, same conversion. `load_profiles` reads each overlay in `.bmad-loop/profiles/` with `read_text(encoding="utf-8")`, so a non-UTF-8 file From 3eb92efb8f351c01dfbc117a5dfa648f957734a8 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 22 Sep 2026 14:58:17 -0700 Subject: [PATCH 2/5] Follow up central TOML config: TUI cache, symlink layers, wording (#769) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - tui.data._project_paths stat-gates on all four central TOML layers plus the legacy config.yaml (via bmadconfig.CENTRAL_LAYERS_REL / LEGACY_CONFIG_REL), so a TOML layer edit invalidates the cached ProjectPaths in a mixed install, and a TOML-only install is cached at all (it was reloaded on every call — a missed cache, not wrong paths). - bmadconfig._load_layer refuses a layer path that is a symlink resolving to no file (dangling or looping) instead of reading it as absent and letting the YAML fill the key; a symlink to a real file still loads. - The code-root-moved warnings in cli and runs name "the BMAD config" and `repo_root`, since the key can now come from a TOML layer. - Rewrap the #769 CHANGELOG entry to the file's ~88-column width. --- CHANGELOG.md | 5 +++- src/bmad_loop/bmadconfig.py | 10 +++++-- src/bmad_loop/cli.py | 4 +-- src/bmad_loop/runs.py | 4 +-- src/bmad_loop/tui/data.py | 20 +++++++------ tests/test_bmadconfig.py | 35 +++++++++++++++++++++++ tests/test_cli.py | 6 ++-- tests/test_runs.py | 4 +-- tests/test_tui_app.py | 2 +- tests/test_tui_data.py | 56 ++++++++++++++++++++++++++++++++++++- 10 files changed, 123 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2506f2ada..aabfe961f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,10 @@ breaking changes may land in a minor release. `validate` warns (`hooks.relay-stale`) while a backslash registration remains. Paths with spaces remain unsupported under the PowerShell fallback. -- Resolve artifact paths from BMAD's four-layer central `_bmad/config.toml`, falling back to `_bmad/bmm/config.yaml` only for keys the TOML lacks, so a TOML-only BMAD 6.12 install passes `validate` and runs; refuse ambiguous, blank, non-string or malformed TOML values instead of falling back (#769, #154). +- Resolve artifact paths from BMAD's four-layer central `_bmad/config.toml`, falling + back to `_bmad/bmm/config.yaml` only for keys the TOML lacks, so a TOML-only BMAD 6.12 + install passes `validate` and runs; refuse ambiguous, blank, non-string or malformed + TOML values instead of falling back (#769, #154). - Replace stale installed relay hooks when a project moves between Windows and POSIX. diff --git a/src/bmad_loop/bmadconfig.py b/src/bmad_loop/bmadconfig.py index 8d756ae00..fbf0d3f9a 100644 --- a/src/bmad_loop/bmadconfig.py +++ b/src/bmad_loop/bmadconfig.py @@ -230,9 +230,15 @@ class _Leaf: def _load_layer(path: Path) -> dict[str, object] | None: """One central TOML layer, None when absent. Present-but-unusable raises: the - caller must never read an unparseable layer as "no TOML" and fall back.""" - if not path.exists(): + caller must never read an unparseable layer as "no TOML" and fall back. A link + counts as present even when `exists()` (which follows it) says otherwise: a + dangling or looping symlink is an entry the operator put there, not an absence.""" + if not path.exists() and not path.is_symlink(): return None + if path.is_symlink() and not path.exists(): + raise BmadConfigError( + _diagnostic_text(f"BMAD config layer is a symlink that resolves to no file: {path}") + ) if not path.is_file(): raise BmadConfigError(_diagnostic_text(f"BMAD config layer is not a file: {path}")) try: diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index a654d54b6..7e9658d9a 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -3048,11 +3048,11 @@ def _prepare_resume_locked(project: Path, run_dir: Path): # tree the run is in from here on; whether the new tree can honor those shas # is the operator's call, and this is the moment they can still make it. print( - f"warning: run {run_dir.name}: the code root in _bmad/bmm/config.yaml has" + f"warning: run {run_dir.name}: the code root in the BMAD config has" " changed since this run started — the resumed engine works in the tree" " configured now, while the baselines, preserve refs and branches this run" " already recorded name objects in the previous one. Restore the previous" - " `repo_root:` value if you did not intend the move.", + " `repo_root` value if you did not intend the move.", file=sys.stderr, ) # Re-stamp: the snapshot must describe the policy THIS process enforces, for diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 0d9655c54..1223734ec 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -4477,10 +4477,10 @@ def restamp_code_root(run_dir: Path, repo_root: Path) -> str | None: if not moved: return None return ( - f"run {run_dir.name}: the code root in _bmad/bmm/config.yaml has changed since " + f"run {run_dir.name}: the code root in the BMAD config has changed since " "this run started — the re-drive works in the tree configured now, while the " "baselines, preserve refs and branches this run already recorded name objects " - "in the previous one. Restore the previous `repo_root:` value if you did not " + "in the previous one. Restore the previous `repo_root` value if you did not " "intend the move." ) diff --git a/src/bmad_loop/tui/data.py b/src/bmad_loop/tui/data.py index 235efd5da..03db4cc8a 100644 --- a/src/bmad_loop/tui/data.py +++ b/src/bmad_loop/tui/data.py @@ -740,8 +740,8 @@ def pending_decision(journal_entries: list[dict[str, Any]]) -> tuple[str, str] | # --------------------------------------------- project-level artifact readers -# project root -> (config.yaml sig, ProjectPaths) -_paths_cache: dict[Path, tuple[_StatSig, bmadconfig.ProjectPaths]] = {} +# project root -> (sigs of every BMAD config source, ProjectPaths) +_paths_cache: dict[Path, tuple[tuple[_StatSig | None, ...], bmadconfig.ProjectPaths]] = {} # sprint-status.yaml path -> (sig or None for missing, parse or None) _sprint_cache: dict[Path, tuple[_StatSig | None, sprintstatus.SprintStatus | None]] = {} # deferred-work.md path -> (sig or None for missing, items or None) @@ -752,25 +752,27 @@ def pending_decision(journal_entries: list[dict[str, Any]]) -> tuple[str, str] | def _project_paths(project: Path) -> bmadconfig.ProjectPaths | None: - """BMAD artifact paths, stat-gated on config.yaml; None when the project - is not initialized (or the config is unreadable).""" + """BMAD artifact paths, stat-gated on every config source `load_paths` reads + (the four central TOML layers and the legacy config.yaml), so an edit to any + of them is seen on the next call; None when the project is not initialized + (or the config is unreadable).""" project = resolve_or_lexical(project) - config_sig = _stat_sig(project / "_bmad" / "bmm" / "config.yaml") + sources = (*bmadconfig.CENTRAL_LAYERS_REL, bmadconfig.LEGACY_CONFIG_REL) + config_sigs = tuple(_stat_sig(project / rel) for rel in sources) cached_paths = _paths_cache.get(project) - if config_sig is not None and cached_paths is not None and cached_paths[0] == config_sig: + if cached_paths is not None and cached_paths[0] == config_sigs: return cached_paths[1] try: paths = bmadconfig.load_paths(project) except (bmadconfig.BmadConfigError, OSError): return None - if config_sig is not None: - _paths_cache[project] = (config_sig, paths) + _paths_cache[project] = (config_sigs, paths) return paths def sprint_overview(project: Path) -> sprintstatus.SprintStatus | None: """Parsed sprint-status.yaml, or None when unavailable (uninitialized - project, missing file, bad YAML). Stat-gated on both config.yaml and the + project, missing file, bad YAML). Stat-gated on both the BMAD config and the sprint file; the same object is returned while the file is unchanged.""" paths = _project_paths(project) if paths is None: diff --git a/tests/test_bmadconfig.py b/tests/test_bmadconfig.py index 12c574874..d06c6df0c 100644 --- a/tests/test_bmadconfig.py +++ b/tests/test_bmadconfig.py @@ -7,6 +7,7 @@ from __future__ import annotations import io +import os import sys from pathlib import Path @@ -716,6 +717,40 @@ def test_a_layer_that_is_not_a_file_refuses(tmp_path: Path) -> None: bmadconfig.load_paths(root) +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +@pytest.mark.parametrize("target", ["missing.toml", "config.toml"], ids=["dangling", "loop"]) +def test_a_layer_symlink_that_resolves_to_no_file_refuses(tmp_path: Path, target: str) -> None: + """`exists()` follows the link and reads a dangling (or self-looping) one as + absent, which would let the YAML fill the key: present-but-unreadable must + refuse, like a directory at the layer path does. + + Ablation: gate on `exists()` alone and the load succeeds off the YAML.""" + root = _toml_only(tmp_path) + _write_config(root) # a valid legacy YAML must not rescue the load + layer = root / bmadconfig.CENTRAL_LAYERS_REL[2] + layer.parent.mkdir(parents=True, exist_ok=True) + layer.symlink_to(target) # relative to custom/: nothing there, or itself + + with pytest.raises(bmadconfig.BmadConfigError, match="resolves to no file") as excinfo: + bmadconfig.load_paths(root) + assert str(root.resolve() / _LAYERS[2]) in str(excinfo.value) + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_a_layer_symlink_to_a_real_file_is_read(tmp_path: Path) -> None: + root = _toml_only(tmp_path) + shared = root / "shared-override.toml" + shared.write_text( + '[modules.bmm]\nimplementation_artifacts = "{project-root}/linked-impl"\n', + encoding="utf-8", + ) + layer = root / bmadconfig.CENTRAL_LAYERS_REL[3] + layer.parent.mkdir(parents=True, exist_ok=True) + layer.symlink_to(shared) + + assert bmadconfig.load_paths(root).implementation_artifacts == root.resolve() / "linked-impl" + + def test_no_toml_and_no_yaml_names_both_expected_locations(tmp_path: Path) -> None: root = tmp_path / "p" root.mkdir() diff --git a/tests/test_cli.py b/tests/test_cli.py index cb88ce985..133e96828 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4041,7 +4041,7 @@ def fake_rearm( assert seen == [moved.resolve()] err = capsys.readouterr().err - assert "the code root in _bmad/bmm/config.yaml has changed" in err + assert "the code root in the BMAD config has changed" in err assert str(moved) not in err # the warning names neither tree, matching resume's @@ -7036,7 +7036,7 @@ def test_resume_restamps_the_code_root_when_the_config_moved(project, monkeypatc # unlanded `restamp_code_root` row, never for this resume's own re-stamp. assert _restamp_records(run_dir) == [] err = capsys.readouterr().err - assert "the code root in _bmad/bmm/config.yaml has changed" in err + assert "the code root in the BMAD config has changed" in err # the warning names neither tree: a journalled scalar, an operator-facing sentence assert str(moved) not in err @@ -7140,7 +7140,7 @@ def test_resume_discharges_the_owed_record_under_the_root_the_marker_names( assert [r["repo"] for r in _restamp_records(run_dir)] == [str(owed)] # The config really did move away from the mirror, so THIS resume is a move too. assert _resume_entry(run_dir)["code_root_changed"] is True - assert "the code root in _bmad/bmm/config.yaml has changed" in capsys.readouterr().err + assert "the code root in the BMAD config has changed" in capsys.readouterr().err persisted = load_state(run_dir) assert persisted.repo_root == str(again.resolve()) assert persisted.code_root_restamp_pending is False diff --git a/tests/test_runs.py b/tests/test_runs.py index e19a3ca2a..9b3e5d336 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -3084,7 +3084,7 @@ def test_restamp_code_root_aims_the_mirror_the_rearm_reads(tmp_path, recorded): assert rewritten is (recorded != "unchanged") if recorded == "moved": assert message is not None - assert "the code root in _bmad/bmm/config.yaml has changed" in message + assert "the code root in the BMAD config has changed" in message # names NEITHER tree, like resume's: the fact is that the run changed # repositories, and the paths are the half that puts arbitrary text on a terminal assert str(now) not in message @@ -3209,7 +3209,7 @@ def records(): moved_message = runs.restamp_code_root(run.run_dir, now) assert moved_message is not None - assert "the code root in _bmad/bmm/config.yaml has changed" in moved_message + assert "the code root in the BMAD config has changed" in moved_message assert [(r["repo"], r["code_root_changed"], r["discharged_owed_move"]) for r in records()] == [ (str(now), True, False) ] diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index 15621ea1d..5d666f5d5 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -5778,7 +5778,7 @@ def fake_rearm(rd, sk, *, isolated_redrive=False, resolution_recorded=False, pro await until(pilot, lambda: calls == ["20260611-100000-aaaa"]) assert seen == [moved.resolve()] - assert any("the code root in _bmad/bmm/config.yaml has changed" in n for n in notes) + assert any("the code root in the BMAD config has changed" in n for n in notes) def test_escalation_rearm_rechecks_liveness_inside_state_lock(project, monkeypatch): diff --git a/tests/test_tui_data.py b/tests/test_tui_data.py index 305db1d87..b71cd065b 100644 --- a/tests/test_tui_data.py +++ b/tests/test_tui_data.py @@ -10,7 +10,12 @@ import time from pathlib import Path -from conftest import install_bmad_config, refuse_to_resolve, write_sprint +from conftest import ( + install_bmad_central_config, + install_bmad_config, + refuse_to_resolve, + write_sprint, +) from bmad_loop import bmadconfig, deferredwork, platform_util, policy from bmad_loop.journal import UNREADABLE_LINE_KIND, Journal, save_state @@ -1107,6 +1112,55 @@ def test_project_paths_uses_one_canonical_cache_key(project): assert alternate_spelling not in data._paths_cache +def _override_implementation_artifacts(project, rel: str) -> None: + (project.project / "_bmad" / "custom" / "config.toml").write_text( + f'[modules.bmm]\nimplementation_artifacts = "{{project-root}}/{rel}"\n', + encoding="utf-8", + ) + + +def test_project_paths_sees_a_toml_layer_edit_in_a_mixed_install(project): + """#769: the TOML layers outrank the legacy YAML the v6.12 installer still writes + beside them, so an override-layer edit must invalidate the cached snapshot. + + Ablation: stat-gate on config.yaml alone and the second call serves the stale + artifact dir from cache. + """ + install_bmad_config(project) + install_bmad_central_config(project) + root = project.project.resolve() + before = data._project_paths(root) + assert before is not None + assert data._project_paths(root) is before + + _override_implementation_artifacts(project, "moved-impl") + + after = data._project_paths(root) + assert after is not None + assert after.implementation_artifacts == root / "moved-impl" + + +def test_project_paths_caches_and_invalidates_a_toml_only_install(project): + """With no config.yaml there is still a source to stat-gate on: the snapshot is + cached (it used to be reloaded on every call) and a layer edit invalidates it.""" + install_bmad_central_config(project) + root = project.project.resolve() + assert not (root / bmadconfig.LEGACY_CONFIG_REL).exists() + + first = data._project_paths(root) + assert first is not None + assert data._paths_cache[root][1] is first + assert data._project_paths(root) is first + + _override_implementation_artifacts(project, "moved-impl") + + second = data._project_paths(root) + assert second is not None + assert second is not first + assert second.implementation_artifacts == root / "moved-impl" + assert data._paths_cache[root][1] is second + + def test_sprint_overview(project): install_bmad_config(project) write_sprint( From db4e58574eb7d42fa20b0f2da372e7b105710855 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 22 Sep 2026 17:26:34 -0700 Subject: [PATCH 3/5] docs(changelog): scope the central-TOML entry to what it resolves (#154, #769) Stable BMAD 6.12 writes both _bmad/config.toml and _bmad/bmm/config.yaml, and a genuinely TOML-only install still fails the skills preflight (#768). The entry now says what the change delivers: paths that live only in the central TOML resolve. --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index aabfe961f..6539a98cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,9 +45,9 @@ breaking changes may land in a minor release. with spaces remain unsupported under the PowerShell fallback. - Resolve artifact paths from BMAD's four-layer central `_bmad/config.toml`, falling - back to `_bmad/bmm/config.yaml` only for keys the TOML lacks, so a TOML-only BMAD 6.12 - install passes `validate` and runs; refuse ambiguous, blank, non-string or malformed - TOML values instead of falling back (#769, #154). + back to `_bmad/bmm/config.yaml` only for keys the TOML lacks, so a project whose paths + live only in the central TOML resolves them; refuse ambiguous, blank, non-string or + malformed TOML values instead of falling back (#154, #769). - Replace stale installed relay hooks when a project moves between Windows and POSIX. From c1ff6c661df86221f8162c17de51b9e35aa68d26 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 22 Sep 2026 19:16:49 -0700 Subject: [PATCH 4/5] fix(bmadconfig): type a central layer's stat failures as config errors (#769) --- src/bmad_loop/bmadconfig.py | 31 ++++++++++++++++++++++++------- tests/test_bmadconfig.py | 23 +++++++++++++++++++++++ 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/bmad_loop/bmadconfig.py b/src/bmad_loop/bmadconfig.py index fbf0d3f9a..c290db746 100644 --- a/src/bmad_loop/bmadconfig.py +++ b/src/bmad_loop/bmadconfig.py @@ -38,6 +38,8 @@ from __future__ import annotations +import errno +import stat import tomllib from dataclasses import dataclass, field from pathlib import Path @@ -232,14 +234,29 @@ def _load_layer(path: Path) -> dict[str, object] | None: """One central TOML layer, None when absent. Present-but-unusable raises: the caller must never read an unparseable layer as "no TOML" and fall back. A link counts as present even when `exists()` (which follows it) says otherwise: a - dangling or looping symlink is an entry the operator put there, not an absence.""" - if not path.exists() and not path.is_symlink(): + dangling or looping symlink is an entry the operator put there, not an absence. + The probes are raw `lstat`/`stat`, not `exists()`: only a missing entry is + absent, and any other failure (an unreadable directory, a dead share) is this + layer's error — `exists()` raises it untyped through 3.13 and reads it as absent + on 3.14+, which would let the YAML fill the key.""" + try: + entry = path.lstat() + except (FileNotFoundError, NotADirectoryError): return None - if path.is_symlink() and not path.exists(): - raise BmadConfigError( - _diagnostic_text(f"BMAD config layer is a symlink that resolves to no file: {path}") - ) - if not path.is_file(): + except OSError as e: + raise BmadConfigError(_diagnostic_text(f"cannot read {path}: {e}")) from e + try: + target = path.stat() + except OSError as e: + # Dangling (ENOENT/ENOTDIR) or looping (ELOOP; WinError 1921) — anything else + # (EACCES on the target's directory) is a read failure, not a missing target. + unresolved = e.errno in (errno.ENOENT, errno.ENOTDIR, errno.ELOOP) + if stat.S_ISLNK(entry.st_mode) and (unresolved or getattr(e, "winerror", None) == 1921): + raise BmadConfigError( + _diagnostic_text(f"BMAD config layer is a symlink that resolves to no file: {path}") + ) from e + raise BmadConfigError(_diagnostic_text(f"cannot read {path}: {e}")) from e + if not stat.S_ISREG(target.st_mode): raise BmadConfigError(_diagnostic_text(f"BMAD config layer is not a file: {path}")) try: # tomllib decodes as UTF-8 itself; a bad byte surfaces as UnicodeDecodeError diff --git a/tests/test_bmadconfig.py b/tests/test_bmadconfig.py index d06c6df0c..a8fc84375 100644 --- a/tests/test_bmadconfig.py +++ b/tests/test_bmadconfig.py @@ -751,6 +751,29 @@ def test_a_layer_symlink_to_a_real_file_is_read(tmp_path: Path) -> None: assert bmadconfig.load_paths(root).implementation_artifacts == root.resolve() / "linked-impl" +@pytest.mark.skipif( + os.name == "nt" or os.geteuid() == 0, reason="POSIX permissions; root bypasses them" +) +def test_a_layer_behind_an_unreadable_directory_refuses_typed(tmp_path: Path) -> None: + """An unreadable layer directory is neither absent nor an untyped crash: through + 3.13 `exists()` raises PermissionError past every `except BmadConfigError`, and + on 3.14+ it reads the layer as absent so the YAML fills the key. + + Ablation: probe with `exists()` again and this raises PermissionError (<=3.13) or + loads off the YAML (3.14+).""" + root = _toml_only(tmp_path) + _write_config(root) # a valid legacy YAML must not rescue the load + custom = root / bmadconfig.CENTRAL_LAYERS_REL[2].parent + custom.mkdir(parents=True) + custom.chmod(0) + try: + with pytest.raises(bmadconfig.BmadConfigError, match="cannot read") as excinfo: + bmadconfig.load_paths(root) + finally: + custom.chmod(0o755) + assert str(root.resolve() / _LAYERS[2]) in str(excinfo.value) + + def test_no_toml_and_no_yaml_names_both_expected_locations(tmp_path: Path) -> None: root = tmp_path / "p" root.mkdir() From f713459fc050abd0f0ae63d66aa0e99e7cfa57f7 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 22 Sep 2026 19:37:28 -0700 Subject: [PATCH 5/5] fix(sweep,bmadconfig,tui): hand triage the resolved ledger; type legacy probe; lstat-aware TUI config cache (#769) --- CHANGELOG.md | 4 ++- src/bmad_loop/bmadconfig.py | 18 +++++++++++-- .../data/skills/bmad-loop-sweep/SKILL.md | 10 +++++-- .../skills/bmad-loop-sweep/migration-mode.md | 4 ++- src/bmad_loop/engine.py | 3 ++- src/bmad_loop/sweep.py | 12 +++++++++ src/bmad_loop/tui/data.py | 18 +++++++++++-- tests/test_bmadconfig.py | 26 +++++++++++++++++++ tests/test_sweep.py | 19 ++++++++++++++ tests/test_tui_data.py | 20 ++++++++++++++ 10 files changed, 125 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6539a98cf..52c47b5a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,7 +47,9 @@ breaking changes may land in a minor release. - Resolve artifact paths from BMAD's four-layer central `_bmad/config.toml`, falling back to `_bmad/bmm/config.yaml` only for keys the TOML lacks, so a project whose paths live only in the central TOML resolves them; refuse ambiguous, blank, non-string or - malformed TOML values instead of falling back (#154, #769). + malformed TOML values instead of falling back; sweep triage reads the ledger the + orchestrator resolved (`BMAD_LOOP_LEDGER`) rather than re-deriving it from the YAML + (#154, #769). - Replace stale installed relay hooks when a project moves between Windows and POSIX. diff --git a/src/bmad_loop/bmadconfig.py b/src/bmad_loop/bmadconfig.py index c290db746..3b28260c9 100644 --- a/src/bmad_loop/bmadconfig.py +++ b/src/bmad_loop/bmadconfig.py @@ -271,6 +271,20 @@ def _load_layer(path: Path) -> dict[str, object] | None: raise BmadConfigError(_diagnostic_text(f"cannot read {path}: {e}")) from e +def _legacy_is_file(path: Path) -> bool: + """`path.is_file()` for the legacy YAML, with its probe failures typed. A missing + entry (or a dangling/looping link) is not a file, as `is_file()` always said; + any other failure — an unreadable `_bmad/bmm`, a dead share — raises, where + `is_file()` raises it untyped through 3.13 and reads it as absent on 3.14+, + misreporting an unreadable fallback as a missing key.""" + try: + return stat.S_ISREG(path.stat().st_mode) + except OSError as e: + if e.errno in (errno.ENOENT, errno.ENOTDIR, errno.ELOOP): + return False + raise BmadConfigError(_diagnostic_text(f"cannot read {path}: {e}")) from e + + class _Table(dict[str, object]): """A table in the merged central config, with every layer that contributed to it — a table has no single source the way a leaf does.""" @@ -407,7 +421,7 @@ def load_paths(project: Path) -> ProjectPaths: config_path = project / LEGACY_CONFIG_REL central = _load_central(project) if central is None: - if not config_path.is_file(): + if not _legacy_is_file(config_path): layers = ", ".join(str(project / rel) for rel in CENTRAL_LAYERS_REL) raise BmadConfigError( f"BMAD config not found: neither the central TOML ({layers}) nor " @@ -425,7 +439,7 @@ def lookup(key: str) -> tuple[str, str] | None: if hit is not None: return hit if legacy is None: - legacy = _load_legacy(config_path) if config_path.is_file() else {} + legacy = _load_legacy(config_path) if _legacy_is_file(config_path) else {} raw = legacy.get(key) return (str(raw), f"`{key}` in {config_path}") if raw else None diff --git a/src/bmad_loop/data/skills/bmad-loop-sweep/SKILL.md b/src/bmad_loop/data/skills/bmad-loop-sweep/SKILL.md index 479888723..fcb4b496b 100644 --- a/src/bmad_loop/data/skills/bmad-loop-sweep/SKILL.md +++ b/src/bmad_loop/data/skills/bmad-loop-sweep/SKILL.md @@ -29,8 +29,14 @@ Steps 1–4 below. ### Step 1: Locate the ledger -Read `{project-root}/_bmad/bmm/config.yaml` to resolve `implementation_artifacts`, -then read `{implementation_artifacts}/deferred-work.md` in full. Open entries +Run: `echo "${BMAD_LOOP_LEDGER:-}"` + +The printed path is the ledger — the orchestrator resolved it from the project's +BMAD config (the central `_bmad/config.toml` layers over the legacy +`_bmad/bmm/config.yaml`), so read that file and never re-derive it; its directory +is `{implementation_artifacts}`. Only if the output is empty, read +`{project-root}/_bmad/bmm/config.yaml` to resolve `implementation_artifacts` and +use `{implementation_artifacts}/deferred-work.md`. Read the ledger in full. Open entries are `### DW-:` blocks whose `status:` line is `open`. If the ledger is missing or unreadable, escalate `CRITICAL` (`type: missing-ledger`) per automation-mode.md and end your turn. diff --git a/src/bmad_loop/data/skills/bmad-loop-sweep/migration-mode.md b/src/bmad_loop/data/skills/bmad-loop-sweep/migration-mode.md index 45228bc6f..672a47030 100644 --- a/src/bmad_loop/data/skills/bmad-loop-sweep/migration-mode.md +++ b/src/bmad_loop/data/skills/bmad-loop-sweep/migration-mode.md @@ -9,7 +9,9 @@ parser. Your job is a one-time rewrite of every legacy item into a canonical `### DW-:` entry, after which the normal triage flow takes over. This is the ONE workflow mode that edits a file: exactly the ledger at -`{implementation_artifacts}/deferred-work.md`. Never any other file, never +`{implementation_artifacts}/deferred-work.md` — the path in `$BMAD_LOOP_LEDGER`, +which the orchestrator resolved and which wins over any path you would derive +yourself. Never any other file, never code, never specs, never sprint-status. Never commit — the orchestrator commits the migrated ledger after validating it. diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 1db102252..893d53240 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -5402,7 +5402,8 @@ def _extra_session_env( ) -> dict[str, str]: """Engine-variant additions to a session's environment. Base: none. StoriesEngine overrides this to export BMAD_LOOP_SPEC_FOLDER for the - adapter's deterministic id-keyed read-back. ``label`` is None for the + adapter's deterministic id-keyed read-back; SweepEngine exports + BMAD_LOOP_LEDGER to its triage sessions. ``label`` is None for the primary dev/review session and set for an injected plugin-workflow session, so a variant can scope its env to primary sessions only.""" return {} diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index fba4c9dcd..e88b5609d 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -4022,6 +4022,18 @@ def _ensure_triage(self, open_now: set[str], cycle: int = 1) -> TriagePlan: "The triage result.json failed deterministic validation:\n- " + "\n- ".join(errors), ) + def _extra_session_env( + self, task: StoryTask, role: str, label: str | None = None + ) -> dict[str, str]: + # Triage and migration sessions read the ledger the orchestrator resolved, + # not one they re-derive: `load_paths` layers the central TOML over the + # legacy YAML, and a skill reading the YAML alone would triage a stale + # ledger — or find none on a TOML-only install (#769). Bundle sessions + # never read the ledger, so they stay byte-identical. + if role != "triage" or label is not None: + return {} + return {"BMAD_LOOP_LEDGER": str(self.workspace.paths.deferred_work)} + def _triage_prompt(self, feedback: Path | None, open_now: set[str] | None = None) -> str: prompt = "/bmad-loop-sweep" if open_now is not None and (self.only_ids is not None or self.min_severity is not None): diff --git a/src/bmad_loop/tui/data.py b/src/bmad_loop/tui/data.py index 03db4cc8a..826cdca09 100644 --- a/src/bmad_loop/tui/data.py +++ b/src/bmad_loop/tui/data.py @@ -741,7 +741,7 @@ def pending_decision(journal_entries: list[dict[str, Any]]) -> tuple[str, str] | # --------------------------------------------- project-level artifact readers # project root -> (sigs of every BMAD config source, ProjectPaths) -_paths_cache: dict[Path, tuple[tuple[_StatSig | None, ...], bmadconfig.ProjectPaths]] = {} +_paths_cache: dict[Path, tuple[tuple[object, ...], bmadconfig.ProjectPaths]] = {} # sprint-status.yaml path -> (sig or None for missing, parse or None) _sprint_cache: dict[Path, tuple[_StatSig | None, sprintstatus.SprintStatus | None]] = {} # deferred-work.md path -> (sig or None for missing, items or None) @@ -751,6 +751,20 @@ def pending_decision(journal_entries: list[dict[str, Any]]) -> tuple[str, str] | _missed_cache: dict[Path, tuple[Any, list]] = {} +def _config_source_sig(path: Path) -> object: + """A config source's cache signature, distinguishing what `_stat_sig` folds into + one `None`: absent, present but failing (an unreadable directory), and a link + whose target is gone or loops. `load_paths` refuses the last two where it reads + the first as "no layer", so appearing at an absent path must invalidate.""" + try: + st = path.lstat() + except (FileNotFoundError, NotADirectoryError): + return None + except OSError as e: + return ("error", e.errno) + return ((st.st_mtime_ns, st.st_size, st.st_ino, st.st_mode), _stat_sig(path)) + + def _project_paths(project: Path) -> bmadconfig.ProjectPaths | None: """BMAD artifact paths, stat-gated on every config source `load_paths` reads (the four central TOML layers and the legacy config.yaml), so an edit to any @@ -758,7 +772,7 @@ def _project_paths(project: Path) -> bmadconfig.ProjectPaths | None: (or the config is unreadable).""" project = resolve_or_lexical(project) sources = (*bmadconfig.CENTRAL_LAYERS_REL, bmadconfig.LEGACY_CONFIG_REL) - config_sigs = tuple(_stat_sig(project / rel) for rel in sources) + config_sigs = tuple(_config_source_sig(project / rel) for rel in sources) cached_paths = _paths_cache.get(project) if cached_paths is not None and cached_paths[0] == config_sigs: return cached_paths[1] diff --git a/tests/test_bmadconfig.py b/tests/test_bmadconfig.py index a8fc84375..31233add3 100644 --- a/tests/test_bmadconfig.py +++ b/tests/test_bmadconfig.py @@ -774,6 +774,32 @@ def test_a_layer_behind_an_unreadable_directory_refuses_typed(tmp_path: Path) -> assert str(root.resolve() / _LAYERS[2]) in str(excinfo.value) +@pytest.mark.skipif( + os.name == "nt" or os.geteuid() == 0, reason="POSIX permissions; root bypasses them" +) +@pytest.mark.parametrize("central", [True, False], ids=["mixed", "yaml-only"]) +def test_an_unreadable_legacy_directory_refuses_typed(tmp_path: Path, central: bool) -> None: + """The legacy YAML probe gets the layers' typing: through 3.13 `is_file()` raises + PermissionError past every `except BmadConfigError`, and on 3.14+ it reads the + YAML as absent, misreporting an unreadable fallback as a missing key. + + Ablation: probe with `config_path.is_file()` again and this raises + PermissionError (<=3.13) or the wrong BmadConfigError (3.14+).""" + root = tmp_path / "p" + root.mkdir() + if central: # a TOML that omits the path keys, so the lookup falls to the YAML + _write_layer(root, 0, b'[core]\nuser_name = "me"\n') + _write_config(root) + legacy_dir = root / bmadconfig.LEGACY_CONFIG_REL.parent + legacy_dir.chmod(0) + try: + with pytest.raises(bmadconfig.BmadConfigError, match="cannot read") as excinfo: + bmadconfig.load_paths(root) + finally: + legacy_dir.chmod(0o755) + assert str(root.resolve() / bmadconfig.LEGACY_CONFIG_REL) in str(excinfo.value) + + def test_no_toml_and_no_yaml_names_both_expected_locations(tmp_path: Path) -> None: root = tmp_path / "p" root.mkdir() diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 2698665ff..18e8532b8 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -32167,3 +32167,22 @@ def record_publish(repo, message, path, **kwargs): assert seen == [(target.resolve(), ledger)] assert ledger.is_symlink() assert target.read_text(encoding="utf-8") == migrated_ledger() + + +def test_triage_session_reads_the_ledger_load_paths_resolved(project): + """#769: the central TOML can move `implementation_artifacts` away from what the + legacy YAML still names. Triage must read the ledger the orchestrator resolved, + so the engine exports it; a bundle (dev) session never reads the ledger and + stays byte-identical. + + Ablation: drop `SweepEngine._extra_session_env` and the env key is absent.""" + write_ledger(project, {"DW-1": "open"}) + plan = triage_result(["DW-1"], skip=[{"id": "DW-1", "reason": "leave it"}]) + engine, adapter = make_sweep(project, [triage_effect(plan)]) + + engine.run() + + assert adapter.sessions[0].env["BMAD_LOOP_LEDGER"] == str(engine.workspace.paths.deferred_work) + task = next(iter(engine.state.tasks.values())) + assert engine._extra_session_env(task, "dev") == {} + assert engine._extra_session_env(task, "triage", label="wf") == {} diff --git a/tests/test_tui_data.py b/tests/test_tui_data.py index b71cd065b..ecbe3800b 100644 --- a/tests/test_tui_data.py +++ b/tests/test_tui_data.py @@ -10,6 +10,7 @@ import time from pathlib import Path +import pytest from conftest import ( install_bmad_central_config, install_bmad_config, @@ -1466,3 +1467,22 @@ def test_story_key_from_task_id_grammar_including_the_generation_suffix(): assert data._story_key_from_task_id("1-1-a-dev-1-g01", "dev") == "1-1-a-dev-1-g01" assert data._story_key_from_task_id("1-1-a-dev-1-g١", "dev") == "1-1-a-dev-1-g١" assert data._story_key_from_task_id("1-1-a-dev-1-g1-extra", "dev") == "1-1-a-dev-1-g1-extra" + + +@pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") +def test_project_paths_invalidates_when_a_dangling_link_appears_at_an_absent_layer(project): + """`_stat_sig` follows links and folds every OSError into None, so a dangling + link created at a layer path that was absent signs exactly like the absence and + the cache served stale paths. `load_paths` refuses that link, so must the TUI. + + Ablation: sign config sources with `_stat_sig` and the stale paths come back.""" + install_bmad_config(project) + install_bmad_central_config(project) + root = project.project.resolve() + layer = root / bmadconfig.CENTRAL_LAYERS_REL[3] + layer.unlink() # an optional layer the operator never wrote + assert data._project_paths(root) is not None + assert root in data._paths_cache + layer.symlink_to("missing.toml") + + assert data._project_paths(root) is None