From 98ad12101cad15881397e7a1a3897f77ff575158 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 25 Aug 2026 15:21:37 -0700 Subject: [PATCH 01/10] feat(platform): add names_win32_alias, the determinism member of the path-guard family (#480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fourth member of the `platform_util` "must be a path inside the project" family. The other three refuse a value that escapes the tree; this one refuses a value that stays inside it and still names a DIFFERENT path on Windows than it does on POSIX — a reserved device basename, or a component whose trailing periods and spaces Win32 trims away. `skill_tree = "NUL"` is project-relative by every measure the other three apply, and on Windows it is a device. Phase 1 of 6. No call site is wired yet; this is the predicate and its truth table only. `_is_reserved_basename` is referenced in place at its definition site with the sanitizers — Python resolves it at call time, and a pure move would obscure the diff. It is a SEGMENT predicate (it splits on the first dot of the whole string, so it answers False for "sub/NUL"), which is why it is applied per component here. `names_tree_root`'s docstring asserted the opposite of this change as settled law — "`\"foo. \"` strips to `\"foo\"`, names a child, and is accepted" — so its tail is amended to name the new sibling as the member that refuses it, on determinism rather than containment grounds. The rest of that docstring is untouched. The `part.strip(" .") != ""` carve-out is load-bearing, not tidiness: it hands `.`, `...`, `" "` and `".. "` back to `names_tree_root` and plain `..` back to `has_parent_ref`, so all four members refuse DISJOINT sets. That is what makes each one separately ablatable, and it mirrors `names_tree_root`'s own `part != ".."` carve-out one function up. SOURCES. The git half of rule 2 is measured on this repo's suite; the Win32 filesystem half is CITED, not measured — this is a Linux box and nothing here calls a Win32 API. Fetched 2026-08-25, not quoted from memory: Microsoft "Naming Files, Paths, and Namespaces" (the reserved list, the NUL.txt equivalence); Microsoft ".NET File path formats on Windows systems" (the trim rule, and the "prior to Windows 11 ... because this no longer applies with Windows 11" statement); Wine's ntdll path conformance tests, `test_RtlGetFullPathName_U` / `test_RtlIsDosDeviceName_U` against `collapse_path` / `RtlIsDosDeviceName_U` (the per-case Windows 11 narrowing markers, and the NUL carve-out); Project Zero's "The Definitive Guide on Win32 to NT Path Conversion" (2016, so the pre-narrowing mechanism). Two corrections the fetch produced, both carried into the docstring: - Microsoft's published list stops at COM1-COM9 / LPT1-LPT9 and omits CONIN$/CONOUT$ entirely. `_RESERVED_BASENAMES` holds COM0/LPT0 and the console pair, so it is a deliberate SUPERSET, not a transcription of the docs. Wine matches CONIN$/CONOUT$ and rejects the `0` forms. The docstring says so rather than implying the set is a claim about Win32. - The trailing period/space STRIPPING rule is not on the file-naming page at all — that page states only a prohibition ("do not end a file or directory name with a space or a period"). The stripping rule is cited to the .NET path-format page, Project Zero, and Wine's `collapse_path`. - The Windows 11 narrowing is corroborated on two independent axes, and bare `NUL` at a leaf still resolves to the device there; `sub/CON` and `NUL.txt` no longer do. The NUL carve-out is stated by no Microsoft page. We refuse the Windows 10 superset on every platform anyway: a config value must not mean different things by OS build. ABLATIONS, run 2026-08-25 against a `cp` backup of src/bmad_loop/platform_util.py (md5 3357b42411b73f683514dd4d84d1021a, restored byte-identical and `md5sum -c` re-verified after each), singly, graded by NAMED test outcome rather than exit code: - A1, drop the `_is_reserved_basename(part)` term -> 12 failed, 19 passed. test_names_win32_alias_catches_reserved_device_names RED on 12 of its 14 rows. Green there: "PRN " and "CON.", which rule 2 catches without help. test_names_win32_alias_catches_the_trailing_trim (5), _accepts_ordinary_paths (6) and _leaves_the_root_and_parent_spellings_to_its_siblings (6) all stayed GREEN. - A2, drop the `part.strip(" .") != ""` carve-out -> 5 failed, 26 passed. test_names_win32_alias_leaves_the_root_and_parent_spellings_to_its_siblings RED on 5 of its 6 rows. Green there: "", which has no components at all. The other three tests all stayed GREEN. Each rule reddens its own rows and only its own rows: that disjointness is the proof, and it is what will redden if someone later "simplifies" the predicate to a bare rstrip test. `trunk fmt` changed nothing (md5 unchanged), so the ablation record grades the file as committed. Full suite 6740 passed / 49 skipped (baseline 6709 + these 31), pyright 0 errors, trunk fmt + trunk check --all clean over 258 files. --- src/bmad_loop/platform_util.py | 102 ++++++++++++++++++++++++++++++++- tests/test_platform_util.py | 81 ++++++++++++++++++++++++++ 2 files changed, 181 insertions(+), 2 deletions(-) diff --git a/src/bmad_loop/platform_util.py b/src/bmad_loop/platform_util.py index 191f2387..da08a714 100644 --- a/src/bmad_loop/platform_util.py +++ b/src/bmad_loop/platform_util.py @@ -150,8 +150,12 @@ def names_tree_root(value: str | Path) -> bool: containing directory there, while both pure flavours keep them as ordinary one-segment names (pathlib never applies that trim — only ``resolve()``, by asking the OS, does). A component made solely of periods and spaces is - therefore root-naming, and that is the whole rule: ``"foo. "`` strips to - ``"foo"``, names a child, and is accepted. + therefore root-naming, and that is where *this* predicate's rule stops: + ``"foo. "`` strips to ``"foo"``, names a child, and is accepted here. It is + still refused, by :func:`names_win32_alias`, on the ground this predicate does + not speak to: the trim leaves ``"foo. "`` inside the tree, so containment has + nothing to object to, but it does not name the same path on Windows as it does + on POSIX, and that determinism rule is the fourth member's. ``".. "`` lands here rather than in :func:`has_parent_ref` because the trailing space stops it matching the ``..`` relative component, so Win32 trims it to @@ -170,6 +174,100 @@ def names_tree_root(value: str | Path) -> bool: return bool(parts) and all(part.strip(" .") == "" and part != ".." for part in parts) +def names_win32_alias(value: str | Path) -> bool: + """True if any component of ``value`` names something other than itself on + Win32 — a reserved device name, or a name whose trailing periods and spaces + Win32 trims away before the path ever reaches the filesystem. + + The fourth member of the "must be a path inside the project" family, and the + only one about *determinism* rather than containment. The other three refuse a + value that leaves the tree; this one refuses a value that stays inside it and + still names a *different* path on Windows than it does on POSIX. + ``skill_tree = "NUL"`` is project-relative by every measure the other three + apply, and on Windows it is a device rather than a directory. Two rules, both + applied per component — both separators are split for the same reason + :func:`names_tree_root` splits both. + + **Rule 1 — reserved device basenames.** ``_RESERVED_BASENAMES`` holds ``CON``, + ``PRN``, ``AUX``, ``NUL``, the console pair ``CONIN$``/``CONOUT$``, ``COM0`` + through ``COM9``, ``LPT0`` through ``LPT9``, and the ISO-8859-1 superscript + ``COM¹``/``COM²``/``COM³`` and ``LPT¹``/``LPT²``/``LPT³`` forms. + :func:`_is_reserved_basename` compares case-insensitively, with or without an + extension, and trims trailing spaces before comparing — ``nul``, ``NUL.txt`` + and ``CON .txt`` all count, and the trim-first ordering is right because Win32 + strips the trailing run *before* it tests for a device (``aux.. ..`` resolves + to AUX). It is a *segment* predicate: it splits on the first dot of the whole + string, so ``_is_reserved_basename("sub/NUL")`` is False. Applying it per + component is what puts ``"sub/NUL"`` in reach at all. + + That set is deliberately a superset of Microsoft's published list, which names + only ``COM1``-``COM9`` and ``LPT1``-``LPT9`` and omits the console pair + entirely — Wine's ``RtlIsDosDeviceName_U`` matches ``CONIN$``/``CONOUT$`` and + rejects the ``0`` forms, so ``COM0``/``LPT0`` are refused here by neither + authority. Over-refusing six spellings nobody wants as a directory name is the + safe direction for a guard; do not read the set as a claim about Win32. + + Windows 11 narrowed the rule the set encodes. Microsoft states the change (in + the .NET path-format documentation, not in the file-naming page, which still + asserts the old model): before Windows 11 a path *beginning* with a legacy + device name was always interpreted as that device, so ``CON.TXT`` meant + ``\\\\.\\CON``; that no longer applies. Wine's conformance data encodes the + same narrowing case by case — ``C:\\con\\con`` carries a Windows 11 alternate + expectation of a literal path, and the extension forms are marked as failing + there — while bare ``NUL`` is left unmarked at every position. Bare ``NUL`` at + a leaf therefore stays a device on Windows 11, as though every existing + directory holds a virtual ``NUL``; ``sub/CON`` and ``NUL.txt`` do not. The + unnarrowed rule — hijack from any position, extension or not — holds on + Windows 10 and earlier. + + We refuse the Windows 10 superset on every platform anyway, deliberately: a + config value must not mean one thing on one OS build and something else on the + next, and a guard that tracked the narrowing would turn a ``seed_files`` entry + into a build-number question. It is the same reasoning that already has the + family refusing ``C:\\secrets`` on POSIX. + + **Rule 2 — the trailing period/space trim.** Win32 removes every trailing + period and space from a path component, so ``".claude/skills."`` creates and + addresses ``.claude/skills`` while the configured string still spells + ``skills.``. The divergence reaches past the filesystem: git's gitignore parser + reads the authored spelling, so a shield pattern rendered from the config + matches ``skills.`` and misses the directory Win32 actually made. + + Rule 2 reads the same trim :func:`names_tree_root` does, split by what the trim + leaves behind. That predicate owns a component made *solely* of periods and + spaces, where the trim leaves nothing and the component names the tree root. + This one owns a component where the trim leaves something, where it names a + *sibling* of what was written. The ``part.strip(" .") != ""`` carve-out is what + draws that line, and it hands plain ``".."`` back to :func:`has_parent_ref` at + the same time. All four members therefore refuse disjoint sets — which is what + lets each be ablated on its own, and mirrors :func:`names_tree_root`'s own + ``part != ".."`` carve-out one function up. + + The git half of rule 2 is measured, on this repo's own suite. **The Win32 + filesystem half is cited, not measured** — this is a Linux box and nothing here + calls a Win32 API. The sources are Microsoft's "Naming Files, Paths, and + Namespaces" for the reserved list and the ``NUL.txt`` equivalence; Microsoft's + ".NET File path formats on Windows systems" for the trim rule and the Windows + 11 statement; Wine's ntdll path conformance tests (``test_RtlGetFullPathName_U`` + and ``test_RtlIsDosDeviceName_U``, against ``collapse_path`` and + ``RtlIsDosDeviceName_U``) for the per-case narrowing and the ``NUL`` carve-out; + and Project Zero's "The Definitive Guide on Win32 to NT Path Conversion" (2016, + so pre-narrowing) for the mechanism. The ``NUL`` carve-out is stated by none of + Microsoft's pages.""" + text = str(value) + # Same both-separator split as `names_tree_root`, and for the same reason: a + # value is judged by the components Win32 would see. + parts = [part for part in text.replace("\\", "/").split("/") if part] + return any( + # `part.strip(" .") != ""` hands a component that is nothing but periods + # and spaces back to `names_tree_root` (and plain `..` to `has_parent_ref`), + # so the four predicates refuse disjoint sets and stay separately ablatable + # — the same carve-out, for the same reason, as `names_tree_root`'s `..`. + _is_reserved_basename(part) or (part.strip(" .") != "" and part != part.rstrip(" .")) + for part in parts + ) + + def is_wsl_unc_path(value: str | Path) -> bool: """True if ``value`` addresses a WSL distro's filesystem through the Windows UNC bridge — ``\\\\wsl.localhost\\\\...`` or its legacy ``\\\\wsl$\\\\...`` diff --git a/tests/test_platform_util.py b/tests/test_platform_util.py index 04b44008..3b17a61f 100644 --- a/tests/test_platform_util.py +++ b/tests/test_platform_util.py @@ -109,6 +109,87 @@ def test_names_tree_root_catches_the_win32_trim_aliases(value): assert platform_util.names_tree_root(value) is True +@pytest.mark.parametrize( + "value", + [ + "NUL", # the bare device, the one form every Windows build still special-cases + "nul", # the match is case-insensitive + "NUL.txt", # an extension does not defuse it (Win10 and earlier) + "PRN ", # trailing spaces are trimmed before the name is compared + "sub/NUL", # non-final component — `_is_reserved_basename` alone answers False here + "sub/NUL.txt", # both narrowings at once + "CONIN$", # the console pair, easy to omit from a hand-written set + "COM1", + "COM0", # COM0/LPT0 are reserved by the same rule as COM1..COM9 + "LPT9", # the last member — the set stops here, `com10` is the tripwire below + "AUX", + "aux.json", # lowercase *and* extension, the shape a config field actually takes + "CON.", # a bare trailing dot on a device name + "sub\\NUL", # backslash separator — judged by the components Win32 would see + ], +) +def test_names_win32_alias_catches_reserved_device_names(value): + # Rule 1, proven per-component and in both separator flavours. The `sub/...` + # rows are the ones `_is_reserved_basename` cannot answer on its own: it splits + # on the first dot of the *whole* string, so it reads `"sub/NUL"` as stem + # `"sub/NUL"` and returns False. Splitting into components first is the whole + # difference. Ablation A1: drop the `_is_reserved_basename(part)` term from + # `names_win32_alias` and this test reddens while + # `test_names_win32_alias_catches_the_trailing_trim` stays green — except the + # `"PRN "` and `"CON."` rows, which rule 2 catches on its own. + assert platform_util.names_win32_alias(value) is True + + +@pytest.mark.parametrize( + "value", + [ + ".claude/skills.", # the #480 item-2 spelling: git matches `skills.`, Win32 creates `skills` + ".claude/skills ", # PR #708 made the space case identical to the dot case + "skills. ", # both at once + "sub./x", # a non-final component — the trim is not a basename-only rule + "a/b ", + ], +) +def test_names_win32_alias_catches_the_trailing_trim(value): + # Rule 2, deliberately holding no row rule 1 also catches, so the two rules + # redden separately: every component here strips to something non-empty and is + # not a reserved name. Ablation A2 (dropping the `part.strip(" .") != ""` + # carve-out) leaves this test entirely green — it only widens rule 2 onto the + # sibling predicates' territory, which is what the disjointness test below pins. + assert platform_util.names_win32_alias(value) is True + + +@pytest.mark.parametrize( + "value", + [ + ".claude/skills", # the shipped default — this predicate must never touch it + "normal/path", + "com10", # NOT reserved: the set stops at COM9 + "nulls", # a device name as a *prefix* is an ordinary name + "auxiliary", + "a.b/c.d", # interior dots are ordinary; only *trailing* ones alias + ], +) +def test_names_win32_alias_accepts_ordinary_paths(value): + # The over-refusal guard. `com10`/`nulls`/`auxiliary` are the false-positive + # tripwires: each would redden if the reserved-name test were loosened from an + # exact stem match to a prefix or substring one. + assert platform_util.names_win32_alias(value) is False + + +@pytest.mark.parametrize("value", ["..", ".", "", "...", " ", ".. "]) +def test_names_win32_alias_leaves_the_root_and_parent_spellings_to_its_siblings(value): + # The disjointness pin. Every row here is refused by `has_parent_ref` (`..`) or + # by `names_tree_root` (the rest), and this predicate must leave them alone so + # the four family members reject disjoint sets and each stays separately + # ablatable. Ablation A2: drop the `part.strip(" .") != ""` carve-out from + # `names_win32_alias` — a bare `part != part.rstrip(" .")` test — and this test + # reddens alone, on every row but `""` (which has no components at all), while + # the three tests above stay green. It is also what reddens if someone + # "simplifies" the predicate to that bare rstrip. + assert platform_util.names_win32_alias(value) is False + + # --------------------------------------------------------------- is_wsl_unc_path From 4d6241fe1f0028cb413fcae4d8085838dd149c17 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 25 Aug 2026 15:32:05 -0700 Subject: [PATCH 02/10] fix(config): refuse Windows device names and trim aliases in policy and profile paths (#480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of 6: wires `names_win32_alias` into the four config-path validation sites that policy.toml and CLI profiles own — `scm.worktree_seed`, `hooks.config_path`, `skill_tree` and `seed_files`. The plugin manifest and the Unity clone are phase 3. Each site keeps its existing project-relative refusal EXACTLY as it was and gains a SECOND arm with its own message. That is not style: "must be project-relative paths" is simply false for `NUL`, which IS project-relative, and for `.claude/skills.`, which is too. The first three predicates are about containment — does this value leave the tree — and the new one is about determinism: does this value name the same path on Windows that it names here. A seed entry spelling a reserved name resolves to a device rather than to the file it spells, and a component ending in a period or space is created trimmed, so the exclude pattern the shield later renders from the configured spelling names a path that does not exist and is inert. One message grammar across all four, so a single substring matches every test: entries must not name a Windows device or end a component in a period or space: got {value!r} Singular fields (`skill_tree`, `hooks.config_path`) drop `entries` and the `: got` tail, matching the messages already beside them. This is a deliberate compatibility break on config that previously loaded. It is a refusal, not a warning: the value means one thing on this platform and another on Windows, and a run that quietly seeds a different file than the one configured is the failure the guard exists to prevent. The comment at the policy site records why the refusal is separate rather than a fourth term in the arm above, and points at the predicate's docstring instead of restating the two rules and their sources. ABLATIONS (2026-08-25, HEAD 98ad1210 + this change, `cp` backup, restored byte-identical with md5 re-verified and re-run green after each). Four arms deleted singly; graded by named test outcome over `pytest tests/test_policy.py tests/test_profile.py`, unablated 472 passed: - A, `policy.py` `scm.worktree_seed`: 7 failed / 465 passed. RED: all six rows of test_scm_worktree_seed_rejects_win32_alias_entries, plus the new `NUL` row of test_scm_worktree_seed_rejects_a_bad_entry_beside_good_ones. GREEN throughout: test_scm_worktree_seed_rejects_non_project_relative_entries (all 8 rows) and every row of test_profile_rejects_win32_alias_paths. - B, `profile.py` `hooks.config_path`: 2 failed / 470 passed. RED: exactly the two config_path rows. GREEN: the skill_tree and seed_files rows, and all of test_policy. - C, `profile.py` `skill_tree`: 2 failed / 470 passed. RED: exactly the two skill_tree rows. GREEN: the config_path and seed_files rows, and all of test_policy. - D, `profile.py` `seed_files`: 2 failed / 470 passed. RED: exactly the two seed_files rows. GREEN: the config_path and skill_tree rows, and all of test_policy. Four ablations, four DISJOINT red sets. That disjointness is the proof that each site is independently guarded rather than sharing one upstream check — a single guard higher up would have reddened all thirteen rows on any one deletion. `trunk fmt` was run before the ablation baseline, so the record grades the files as committed (source md5s unchanged by it). Full suite 6753 passed / 49 skipped (baseline 6740 + these 13), pyright 0 errors, `trunk check --all` clean over 258 files. No version bump; `worktree_flow.py` untouched. --- src/bmad_loop/adapters/profile.py | 22 +++++++++++- src/bmad_loop/policy.py | 14 ++++++++ tests/test_policy.py | 55 +++++++++++++++++++++++++--- tests/test_profile.py | 59 +++++++++++++++++++++++++++++++ 4 files changed, 145 insertions(+), 5 deletions(-) diff --git a/src/bmad_loop/adapters/profile.py b/src/bmad_loop/adapters/profile.py index 6de2af76..1e3eb6dd 100644 --- a/src/bmad_loop/adapters/profile.py +++ b/src/bmad_loop/adapters/profile.py @@ -41,7 +41,12 @@ import regex -from ..platform_util import has_parent_ref, is_absolute_path, names_tree_root +from ..platform_util import ( + has_parent_ref, + is_absolute_path, + names_tree_root, + names_win32_alias, +) from .entrypoints import record_load_error USAGE_PARSERS = {"claude-jsonl", "codex-rollout", "gemini-chat", "copilot-events", "none"} @@ -248,6 +253,11 @@ def fail(msg: str) -> ProfileError: # `names_tree_root("")` is True, so this arm also carries the # "a real dialect must name a config_path at all" case. raise fail("hooks.config_path must be a project-relative path") + if names_win32_alias(hooks.config_path): + raise fail( + "hooks.config_path must not name a Windows device or end a component " + "in a period or space" + ) if not hooks.events: raise fail("hooks.events must map native event names to canonical ones") bad = sorted(set(hooks.events.values()) - CANONICAL_EVENTS) @@ -339,6 +349,11 @@ def fail(msg: str) -> ProfileError: ): raise fail("skill_tree must be a project-relative path") + if names_win32_alias(profile.skill_tree): + raise fail( + "skill_tree must not name a Windows device or end a component in a period or space" + ) + # `names_tree_root` subsumes the emptiness check it replaced. These entries feed # provision_worktree's seed loop, where any spelling of the root ("", ".", "./", # ".\") resolves src to the repo root and dst to the worktree — both pass the @@ -346,6 +361,11 @@ def fail(msg: str) -> ProfileError: for seed in profile.seed_files: if names_tree_root(seed) or is_absolute_path(seed) or has_parent_ref(seed): raise fail(f"seed_files entries must be project-relative paths: got {seed!r}") + if names_win32_alias(seed): + raise fail( + "seed_files entries must not name a Windows device or end a component " + f"in a period or space: got {seed!r}" + ) for pattern in profile.env_fault_patterns: try: diff --git a/src/bmad_loop/policy.py b/src/bmad_loop/policy.py index 1f8e0668..63c10f99 100644 --- a/src/bmad_loop/policy.py +++ b/src/bmad_loop/policy.py @@ -24,6 +24,7 @@ has_parent_ref, is_absolute_path, names_tree_root, + names_win32_alias, ) POLICY_FILE = Path(".bmad-loop") / "policy.toml" @@ -1148,11 +1149,24 @@ def loads(text: str, plugin_schemas: dict[str, Any] | None = None) -> Policy: # skipped); they are rejected here for consistency with the sibling sources, and # because a silently-inert seed entry reads as applied configuration when it is # not. + # + # The second refusal is a SEPARATE arm, not a fourth term in the first, because + # the first one's message is false for what it catches: `NUL` and `cfg. ` are + # project-relative by every measure those three predicates apply. What they are + # not is deterministic — each names a different path on Windows than it does + # here, so the same seed entry copies a different file (or a device) depending + # on where the run happens. `names_win32_alias`'s docstring carries the two + # rules, their sources, and which half of each is measurable on this platform. for seed in scm.worktree_seed: if names_tree_root(seed) or is_absolute_path(seed) or has_parent_ref(seed): raise PolicyError( f"scm.worktree_seed entries must be project-relative paths: got {seed!r}" ) + if names_win32_alias(seed): + raise PolicyError( + "scm.worktree_seed entries must not name a Windows device or end a component " + f"in a period or space: got {seed!r}" + ) cleanup = CleanupPolicy( run_retention=_typed_int( cleanup_d, "cleanup", "run_retention", CleanupPolicy.run_retention diff --git a/tests/test_policy.py b/tests/test_policy.py index 2473aaa2..b3321724 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -1009,6 +1009,45 @@ def test_scm_worktree_seed_rejects_non_project_relative_entries(tmp_path, entry) policy.load(p) +@pytest.mark.parametrize( + "entry", + [ + "NUL", # a device rather than a directory, and project-relative by every other measure + "sub/NUL", # non-final component — `_is_reserved_basename` alone answers False here + "aux.json", # lowercase with an extension: the shape a seed entry actually takes + "PRN ", # trailing spaces are trimmed away before the device name is compared + ".claude/skills.", # the trim rule one component up from the leaf + "cfg ", # …and at the leaf, on a name that is otherwise entirely ordinary + ], +) +def test_scm_worktree_seed_rejects_win32_alias_entries(tmp_path, entry): + """The second refusal at this site, and the one the first cannot make: every row + here IS project-relative, so `names_tree_root`, `is_absolute_path` and + `has_parent_ref` all pass it. What it is not is the same path on both platforms, + which is why it carries its own message instead of a fourth spelling of "must be + project-relative". + + The harm is a seed entry that quietly means something else on Windows. A reserved + name resolves to a device rather than to the file it spells, and a component + ending in a period or space is created trimmed — so the entry the shield later + renders as an exclude pattern names a path that does not exist, the line is inert, + and the surplus it fails to shield is staged by the unit's `git add -A`. Both + halves of that are cited (Microsoft, Wine, Project Zero) rather than measured: + this suite runs on POSIX, where every row here is an ordinary name. + + Ablation: delete the `names_win32_alias(seed)` arm and all six rows fail while + `test_scm_worktree_seed_rejects_non_project_relative_entries` stays green — the + two arms reject disjoint sets, which is what keeps each separately ablatable.""" + p = tmp_path / "policy.toml" + p.write_text(f"[scm]\nworktree_seed = [{entry!r}]\n".replace("'", '"')) + + with pytest.raises( + policy.PolicyError, + match="must not name a Windows device or end a component in a period or space", + ): + policy.load(p) + + @pytest.mark.parametrize( ("value", "match"), [ @@ -1038,13 +1077,21 @@ def test_scm_worktree_seed_rejects_value_shapes_that_are_not_a_list_of_paths( policy.load(p) -def test_scm_worktree_seed_rejects_a_bad_entry_beside_good_ones(tmp_path): +@pytest.mark.parametrize( + ("entry", "match"), + [ + ("", "got ''"), # root-naming: caught by the project-relative arm + ("NUL", "got 'NUL'"), # …and by the win32-alias arm that follows it in the same loop + ], +) +def test_scm_worktree_seed_rejects_a_bad_entry_beside_good_ones(tmp_path, entry, match): """Every entry is checked, not just the first: a valid leading entry must not - let a later empty one through.""" + let a later empty one through. The win32-alias arm sits inside that same loop, so + the row for it pins that it inherits the property rather than re-earning it.""" p = tmp_path / "policy.toml" - p.write_text('[scm]\nworktree_seed = [".mcp.json", "", ".envrc"]\n') + p.write_text(f'[scm]\nworktree_seed = [".mcp.json", "{entry}", ".envrc"]\n') - with pytest.raises(policy.PolicyError, match="got ''"): + with pytest.raises(policy.PolicyError, match=match): policy.load(p) diff --git a/tests/test_profile.py b/tests/test_profile.py index ee104fd7..c1b9b738 100644 --- a/tests/test_profile.py +++ b/tests/test_profile.py @@ -501,6 +501,65 @@ def test_invalid_profiles_rejected(tmp_path, mutation, match): load_profiles(tmp_path) +@pytest.mark.parametrize( + ("mutation", "match"), + [ + # skill_tree — a reserved device name, then the trailing-trim alias. Each of + # the three fields carries the same grammar with its own name in front. + ( + MINIMAL_PROFILE.replace("[hooks]", 'skill_tree = "NUL"\n[hooks]'), + "skill_tree must not name a Windows device", + ), + ( + MINIMAL_PROFILE.replace("[hooks]", 'skill_tree = ".claude/skills."\n[hooks]'), + "skill_tree must not name a Windows device", + ), + # hooks.config_path — the field a real dialect is required to set anyway + ( + MINIMAL_PROFILE.replace( + 'config_path = ".mycli/settings.json"', 'config_path = "aux.json"' + ), + "hooks.config_path must not name a Windows device", + ), + ( + MINIMAL_PROFILE.replace( + 'config_path = ".mycli/settings.json"', + 'config_path = ".mycli/settings.json "', + ), + "hooks.config_path must not name a Windows device", + ), + # seed_files is checked per entry, so both aliases sit beside a good one and a + # non-final component carries the reserved name `_is_reserved_basename` misses + ( + MINIMAL_PROFILE.replace("[hooks]", 'seed_files = [".mcp.json", "sub/NUL"]\n[hooks]'), + "seed_files entries must not name a Windows device", + ), + ( + MINIMAL_PROFILE.replace("[hooks]", 'seed_files = [".mcp.json", "cfg "]\n[hooks]'), + "seed_files entries must not name a Windows device", + ), + ], +) +def test_profile_rejects_win32_alias_paths(tmp_path, mutation, match): + """Every row here is project-relative, so the refusal beside this one passes it — + a profile that names `NUL` or `.claude/skills.` is contained, and still does not + name the same path on Windows as it does here. The harm is a profile that quietly + means something else per platform: a reserved component resolves to a device, and + a component ending in a period or space is created trimmed, so the path the + shield renders from the configured spelling is not the path on disk. Cited to + Microsoft, Wine and Project Zero, not measured — this suite runs on POSIX. + + Ablation: delete any one of the three `names_win32_alias` arms in + `_validate_profile` and exactly that field's two rows fail; the other four stay + green, which is what proves the three sites are independently guarded rather than + sharing one upstream check.""" + profiles_dir = tmp_path / ".bmad-loop" / "profiles" + profiles_dir.mkdir(parents=True) + (profiles_dir / "bad.toml").write_text(mutation) + with pytest.raises(ProfileError, match=match): + load_profiles(tmp_path) + + # every type `tomllib` can yield, plus the numeric spellings that are legal TOML # and hostile to a raw coercion TOML_VALUE_DOMAIN = [ From 4d7bc02a0281826ee87d28702c3d96a57cfa7379 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 25 Aug 2026 15:43:37 -0700 Subject: [PATCH 03/10] fix(plugins): refuse Windows device names and trim aliases in manifest and Unity seed paths (#480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 of 6: wires `names_win32_alias` into the three remaining #480 sites the plugin layer owns — `_check_relative_paths` in plugins/manifest.py (which serves BOTH `seed_files` and `seed_globs`), the `[python] module` guard beside it, and the hand-mirrored guard chain in the deployed `unity_seed_assets.py`. policy.py and adapters/profile.py were phase 2; runs.py is phase 4. Both manifest sites keep their existing project-/plugin-relative refusal EXACTLY as it was and gain a SECOND arm, for the reason phase 2 recorded: "must be project-relative paths" is simply false for `NUL` and `.claude/skills.`, both of which ARE project-relative. The first three predicates are about containment; this one is about determinism — does the value name the same path on Windows that it names here. At `[python] module` the consequence is sharper than at the seed fields: that value is imported rather than copied, so a spelling Win32 resolves elsewhere is what gets exec'd. The message keeps each site's own subject spelling (`{label} entries`, and the TOML-table form `[python] module` from the plugin-relative refusal above it) on the one grammar shared by all seven sites, so a single substring matches every test in the family: must not name a Windows device or end a component in a period or space THE UNITY CLONE. `unity_seed_assets.py` is deployed into a consumer project and is stdlib-only — it cannot import core — so `_names_win32_alias` is a hand-written mirror, joining the existing `_is_absolute` / `_has_parent_ref` / `_names_tree_root` clones, with its own `_RESERVED_BASENAMES` and `_is_reserved_basename`. The file is excluded from pyright and had no guard against drift (#546). The clone WAS diffed against the core predicate character by character before this was committed — mechanically, not by eye: the frozenset, `_is_reserved_basename` and the predicate body were extracted from both files and run through difflib with docstrings and comments stripped. `_is_reserved_basename` and the set are IDENTICAL member for member; the predicate body differs only in the private name, the `str | Path` -> `str` signature (matching its three sibling clones, which are all `str`-only) and the dropped `text = str(value)` line it makes unnecessary. The two rule terms are byte-identical. Unlike the manifest sites, the Unity term joins the EXISTING chain rather than adding a second arm — that site's message ("invalid scene guard dir") is not false for a device name, so there is nothing to keep separate. The message is extended to carry the shared substring. `Assets/BmadLoop/Editor.` is the shape that matters there: the caller `.strip()`s only the whole env var, so a trailing dot and every interior component reach the guard untouched. TESTS. `test_manifest_rejects_win32_alias_seed_paths` is parametrized on the FIELD as well as the value, so the claim that one shared helper guards both fields is asserted rather than assumed. The Unity mirror is pinned behaviorally on the core predicate's own 31-row phase-1 truth table (`_names_win32_alias(x) is names_win32_alias(x)`), loaded by path through the `_load_seeder()` helper this file already uses — there was existing precedent for importing it, so no fragile loader was invented — plus a set-equality test, because the truth table cannot reach every member and a device name silently dropped from the clone would simply be seeded. ABLATIONS (2026-08-25, `cp` backup, one arm deleted at a time, each restored and md5-verified against the baseline before the next). Graded by named test node-id over `pytest tests/test_plugin_loader.py tests/test_unity_scene_guard.py`, unablated 122 passed. Baseline md5s: manifest.py 348971b941b6f3bd17ff8c048410ee34, unity_seed_assets.py bfc5f8478474b67c39baf6a29c79f695 (both restored byte-identical and re-run green). - A, `_check_relative_paths` arm: 10 failed / 112 passed. RED: all five rows of test_manifest_rejects_win32_alias_seed_paths in BOTH the seed_files and the seed_globs parametrization, reddening together. That joint reddening is the point of the field axis — it is what proves the one helper is the guard for both fields. GREEN: all four [python] module rows, all 46 Unity rows. - B, `[python] module` arm: 4 failed / 118 passed. RED: exactly its own four rows. GREEN: all ten seed rows, all 46 Unity rows. - C, the Unity chain term `or _names_win32_alias(guard_dir)`: 1 failed / 121 passed. RED: only test_seed_rejects_a_win32_alias_guard_dir_on_any_platform. The 31 parity rows and the set-mirror test stayed GREEN — correctly, and usefully: the parity tests grade the MIRROR, the behavioral test grades the WIRING, and each fails alone. - D, drop `CONIN$` from the CLONE's set only: 2 failed / 44 passed. RED: the CONIN$ parity row and test_seeder_reserved_basename_set_mirrors_core_member_for_member. This is the anti-vacuity check on the parity suite — a mirror test that could not fail would be worse than none on a file nothing else guards. Four ablations, four disjoint red sets. `trunk fmt` was run before the ablation baseline and changed nothing, so the record grades the files exactly as committed. Full suite 6800 passed / 49 skipped (baseline 6753 + 47: 14 manifest rows, 33 Unity), pyright 0 errors, `trunk check --all` clean over 258 files. No existing test or fixture needed changing, so nothing shipped in a bundled plugin.toml trips the new refusal. No version bump; worktree_flow.py untouched; the pyright exclude for `src/bmad_loop/data` left alone. --- .../data/plugins/unity/unity_seed_assets.py | 61 +++++++++++++- src/bmad_loop/plugins/manifest.py | 28 ++++++- tests/test_plugin_loader.py | 53 ++++++++++++ tests/test_unity_scene_guard.py | 80 +++++++++++++++++++ 4 files changed, 219 insertions(+), 3 deletions(-) diff --git a/src/bmad_loop/data/plugins/unity/unity_seed_assets.py b/src/bmad_loop/data/plugins/unity/unity_seed_assets.py index 7c5160c1..35a9334c 100644 --- a/src/bmad_loop/data/plugins/unity/unity_seed_assets.py +++ b/src/bmad_loop/data/plugins/unity/unity_seed_assets.py @@ -88,6 +88,51 @@ def _names_tree_root(value: str) -> bool: return bool(parts) and all(part.strip(" .") == "" and part != ".." for part in parts) +# Reserved on Windows regardless of extension: CON.txt is as illegal as CON. Mirrors +# ``bmad_loop.platform_util._RESERVED_BASENAMES`` member for member, including its +# deliberate over-refusals: Microsoft's published list names only COM1-COM9 / +# LPT1-LPT9 and omits the console pair, while Wine's ``RtlIsDosDeviceName_U`` matches +# CONIN$/CONOUT$ but rejects the 0 forms, so COM0/LPT0 are backed by neither. Mirror +# the set, not any claim about it. +_RESERVED_BASENAMES = frozenset( + {"CON", "PRN", "AUX", "NUL", "CONIN$", "CONOUT$"} + | {f"COM{i}" for i in range(10)} + | {f"LPT{i}" for i in range(10)} + | {f"COM{s}" for s in "¹²³"} + | {f"LPT{s}" for s in "¹²³"} +) + + +def _is_reserved_basename(seg: str) -> bool: + """True if ``seg``'s basename (before the first dot, trailing spaces trimmed — + ``CON .txt`` counts) is a Windows reserved device name (mirrors + ``bmad_loop.platform_util._is_reserved_basename``).""" + stem = seg.split(".", 1)[0].rstrip(" ") + return stem.upper() in _RESERVED_BASENAMES + + +def _names_win32_alias(value: str) -> bool: + """True if any component of ``value`` names something other than itself on Win32 — + a reserved device name, or a name whose trailing periods and spaces Win32 trims + away before the path ever reaches the filesystem (mirrors + ``bmad_loop.platform_util.names_win32_alias`` — this deployed script is stdlib-only + and cannot import core). + + The fourth member of the family, and the only one about *determinism* rather than + containment: ``"Assets/NUL"`` and ``"Assets/BmadLoop/Editor."`` are both inside the + worktree by every measure the other three apply, and both name a different thing on + Windows than they spell here. The ``part.strip(" .") != ""`` carve-out hands a + component made solely of periods and spaces back to :func:`_names_tree_root` and + plain ``".."`` back to :func:`_has_parent_ref`, so all four refuse disjoint sets. + Core's docstring carries the two rules, their sources, and the Windows 11 narrowing + this deliberately does not track.""" + parts = [part for part in value.replace("\\", "/").split("/") if part] + return any( + _is_reserved_basename(part) or (part.strip(" .") != "" and part != part.rstrip(" .")) + for part in parts + ) + + def _truthy(value: str | None, default: bool) -> bool: if value is None or value.strip() == "": return default @@ -187,14 +232,26 @@ def main() -> int: # The install dir must stay inside the worktree AND name something in it: an # absolute/drive-qualified path would make _install's relative_to() raise, a # ".." segment would let the copy escape the project tree, and a root-naming - # spelling would scatter the payload across the worktree root itself. + # spelling would scatter the payload across the worktree root itself. The fourth + # term is about determinism rather than containment: Win32 trims a component's + # trailing periods and spaces, so "Assets/BmadLoop/Editor." installs into "Editor" + # while the configured string still spells "Editor.", and a component naming a + # reserved device writes to the device instead of the tree. The caller's .strip() + # above removes only a trailing run on the WHOLE value, so neither an interior + # component nor a trailing dot is already handled. if ( not rel.parts or _names_tree_root(guard_dir) or _is_absolute(guard_dir) or _has_parent_ref(guard_dir) + or _names_win32_alias(guard_dir) ): - print(f"unity_seed_assets: invalid scene guard dir {guard_dir!r}", file=sys.stderr) + print( + f"unity_seed_assets: invalid scene guard dir {guard_dir!r}: it must name a " + "path inside the worktree and must not name a Windows device or end a " + "component in a period or space", + file=sys.stderr, + ) return 2 target_dir = worktree / guard_dir diff --git a/src/bmad_loop/plugins/manifest.py b/src/bmad_loop/plugins/manifest.py index f9124df0..d331570e 100644 --- a/src/bmad_loop/plugins/manifest.py +++ b/src/bmad_loop/plugins/manifest.py @@ -16,7 +16,12 @@ import tomllib from typing import Any -from ..platform_util import has_parent_ref, is_absolute_path, names_tree_root +from ..platform_util import ( + has_parent_ref, + is_absolute_path, + names_tree_root, + names_win32_alias, +) from .model import ( SETTING_TYPES, WORKFLOW_ROLES, @@ -56,9 +61,22 @@ def _check_relative_paths(values: tuple[str, ...], label: str, fail) -> None: # `names_tree_root` subsumes the emptiness check it replaced: "", ".", "./" and # ".\" all name the tree rather than anything in it, and a seed entry that names # the tree root makes provision_worktree copy the whole repo into the worktree. + # + # The second refusal is a SEPARATE arm rather than a fourth term in the first, + # because the first one's message is false for what it catches: `NUL` and + # `skills.` ARE project-relative. What they are not is deterministic — each names + # a different path on Windows than the string spells, so the same manifest seeds a + # different file (or a device) depending on where the run happens. One call site + # guards BOTH `seed_files` and `seed_globs`; see `names_win32_alias`'s docstring + # for the two rules and their sources. for value in values: if names_tree_root(value) or is_absolute_path(value) or has_parent_ref(value): raise fail(f"{label} entries must be project-relative paths: got {value!r}") + if names_win32_alias(value): + raise fail( + f"{label} entries must not name a Windows device or end a component " + f"in a period or space: got {value!r}" + ) def _parse_hooks(hooks_d: Any, fail) -> tuple[HookSpec, ...]: @@ -174,6 +192,14 @@ def _parse_python(python_d: Any, fail) -> PythonSpec | None: raise fail("[python] requires a 'module'") if names_tree_root(module) or is_absolute_path(module) or has_parent_ref(module): raise fail(f"[python] module must be a plugin-relative path: got {module!r}") + # Separate arm, same reason as `_check_relative_paths`, and it bites harder here: + # this value is not copied but *imported*, so a module spelled `NUL` or `hooks.` + # resolves on Windows to something other than the file the manifest names. + if names_win32_alias(module): + raise fail( + "[python] module must not name a Windows device or end a component " + f"in a period or space: got {module!r}" + ) return PythonSpec(module=module, cls=str(python_d.get("class", "Plugin")) or "Plugin") diff --git a/tests/test_plugin_loader.py b/tests/test_plugin_loader.py index fb214665..acda51e8 100644 --- a/tests/test_plugin_loader.py +++ b/tests/test_plugin_loader.py @@ -231,6 +231,59 @@ def test_invalid_toml_rejected(tmp_path): load_plugins(tmp_path) +# The one substring every #480 refusal shares, across all seven guarded config +# sites — a single matcher for the whole family. +_WIN32_ALIAS_MATCH = "must not name a Windows device or end a component in a period or space" + + +@pytest.mark.parametrize("key", ["seed_files", "seed_globs"]) +@pytest.mark.parametrize( + "value", + [ + "NUL", # the bare device — project-relative by every measure the arm above applies + "sub/CON", # a non-final component: `_is_reserved_basename` alone reads this False + "aux.json", # lowercase and extensioned, the shape a seed entry actually takes + ".claude/skills.", # rule 2: Win32 creates `skills`, the manifest still spells `skills.` + "a/b ", # the trailing space, identical in shape to the dot since PR #708 + ], +) +def test_manifest_rejects_win32_alias_seed_paths(key, value): + """Both seed fields refuse a value that names a Windows device or ends a + component in a period or space. + + The `key` axis is the assertion, not scenery: `seed_files` and `seed_globs` are + each guarded by one `_check_relative_paths` call, so this proves the single + shared helper covers both rather than assuming it. Ablation: delete that + helper's `names_win32_alias` arm and every row of BOTH parametrizations reddens + together — one helper, two fields — while the `[python] module` test below stays + green. + """ + body = f'[plugin]\nname = "e"\napi_version = 1\n{key} = ["{value}"]\n' + with pytest.raises(PluginError, match=_WIN32_ALIAS_MATCH) as excinfo: + load_manifest(body, "e/plugin.toml", "e") + assert key in str(excinfo.value) # the message names the field it refused + + +@pytest.mark.parametrize( + "value", + [ + "NUL", + "hooks.", # Win32 trims to `hooks`, so the import resolves past the file named + "sub/CON.py", + "pkg /hooks.py", # an interior component, which the caller's .strip() cannot reach + ], +) +def test_manifest_rejects_win32_alias_python_module(value): + """`[python] module` refuses the same family, and it bites harder here than at + the seed fields: this value is imported rather than copied, so a spelling Win32 + resolves elsewhere is what gets exec'd. Its own arm — ablate it and only these + rows redden, while both seed-field parametrizations above stay green.""" + body = f'[plugin]\nname = "e"\napi_version = 1\n[python]\nmodule = "{value}"\n' + with pytest.raises(PluginError, match=_WIN32_ALIAS_MATCH) as excinfo: + load_manifest(body, "e/plugin.toml", "e") + assert "[python] module" in str(excinfo.value) + + # ------------------------------------------------------------ manifest reads diff --git a/tests/test_unity_scene_guard.py b/tests/test_unity_scene_guard.py index 7c2eb4ee..7c9ee593 100644 --- a/tests/test_unity_scene_guard.py +++ b/tests/test_unity_scene_guard.py @@ -17,6 +17,9 @@ import os from pathlib import Path +import pytest + +from bmad_loop import platform_util from bmad_loop.plugins import get_plugin _GUARD_DIR = "Assets/BmadLoop/Editor" @@ -225,6 +228,83 @@ def test_seed_rejects_windows_flavored_escapes_on_any_platform(tmp_path, monkeyp assert not (tmp_path / "Assets" / "BmadLoop").exists() # nothing seeded +def test_seed_rejects_a_win32_alias_guard_dir_on_any_platform(tmp_path, monkeypatch): + """The fourth family member, wired into the same guard chain: a guard dir that + names a Windows device, or whose trailing periods/spaces Win32 trims, installs + somewhere other than the path it spells. Refused on every platform for the same + reason the drive-qualified rows above are — a value must not mean one thing here + and another on Windows. `Editor.` is the shape that matters in practice: nothing + upstream catches it, since the caller `.strip()`s only the whole env var.""" + mod = _load_seeder() + (tmp_path / "Assets").mkdir() + for evil in ("Assets/NUL", "Assets/BmadLoop/Editor.", "Assets/BmadLoop /Editor", "NUL"): + _set_env(monkeypatch, tmp_path, guard_dir=evil) + assert mod.main() == 2, evil + assert not (tmp_path / "Assets" / "BmadLoop").exists() # nothing seeded + + +# --------------------------------------------- the hand-mirrored win32 predicate + +# The phase-1 truth table from `tests/test_platform_util.py`, carried over verbatim: +# rule 1 (reserved device basenames, per component, both separators), rule 2 (the +# trailing period/space trim), the over-refusal tripwires (`com10`, `nulls`, +# `auxiliary`), and the root/parent spellings the predicate must leave to its three +# siblings. Any divergence between core and the clone shows up on one of these rows. +_WIN32_ALIAS_ROWS = ( + "NUL", + "nul", + "NUL.txt", + "PRN ", + "sub/NUL", + "sub/NUL.txt", + "CONIN$", + "COM1", + "COM0", + "LPT9", + "AUX", + "aux.json", + "CON.", + "sub\\NUL", + ".claude/skills.", + ".claude/skills ", + "skills. ", + "sub./x", + "a/b ", + ".claude/skills", + "normal/path", + "com10", + "nulls", + "auxiliary", + "a.b/c.d", + "..", + ".", + "", + "...", + " ", + ".. ", +) + + +@pytest.mark.parametrize("value", _WIN32_ALIAS_ROWS) +def test_seeder_win32_alias_clone_agrees_with_the_core_predicate(value): + """This script is stdlib-only (it is deployed into a consumer project and cannot + import bmad_loop), so `_names_win32_alias` is a hand-written mirror of + `platform_util.names_win32_alias`. The file is also excluded from pyright and has + no other guard against drift (#546) — so the mirror is pinned behaviorally, on + the core predicate's own truth table, rather than trusted to stay in sync.""" + mod = _load_seeder() + assert mod._names_win32_alias(value) is platform_util.names_win32_alias(value) + + +def test_seeder_reserved_basename_set_mirrors_core_member_for_member(): + """The truth table above cannot reach every member of the set, and a device name + dropped from the clone would simply be seeded. Pin the set itself — including the + deliberate over-refusals (`COM0`/`LPT0`, the `CONIN$`/`CONOUT$` pair) that neither + Microsoft's published list nor Wine's `RtlIsDosDeviceName_U` backs on its own.""" + mod = _load_seeder() + assert mod._RESERVED_BASENAMES == platform_util._RESERVED_BASENAMES + + # ------------------------------------------------------------ version parsing From 3c9b0857981ea6a7b9b6b029b0d4d54ba4dc407c Mon Sep 17 00:00:00 2001 From: t Date: Tue, 25 Aug 2026 15:57:29 -0700 Subject: [PATCH 04/10] fix(runs): restore guard-family parity in _is_path_escape and contain the run-dir removals (#480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_is_path_escape` was the only member of the "must be a path inside the project" guard family that omitted `names_tree_root` — the six sibling sites (policy.py, adapters/profile.py x3, plugins/manifest.py x2) all pair the three. Add it, and add a containment check to the two destructive run-dir writes. What the omission actually cost, measured on this repo: * `""` and `"."` join to the runs ROOT exactly — `Path(runs) / ""` and `Path(runs) / "."` both *are* the runs dir — so `resolve_run_dir`'s exact branch handed that root back whenever a `state.json` was lying at it, and `delete_run` removed it with a bare `shutil.rmtree`. * That precondition is worth stating rather than glossing: without a `state.json` at the runs root the exact branch is inert and the refs fall through to partial matching, which can only yield an enumerated child. The issue's own framing is wider than the measurement supports. * The ref is the operator's own argv, so there is no privilege boundary here. This is a footgun, not an escalation, and is described as one. #480 attributes the defect to `".. "` slipping `has_parent_ref` and reaching `.bmad-loop/`. That is refuted on POSIX: `".. "` resolves to an ordinary one-segment directory INSIDE the runs dir, and there is no escape. `"..."`, `".. "` and `" .."` are refused here for the Win32 half of the rule instead — cited to Microsoft/Wine/Project Zero, not measured, because this is a Linux box: the trim of trailing periods and spaces leaves `..` or nothing there. `names_win32_alias` is deliberately NOT added to this guard. It would make a legacy run dir named `NUL` or `run. ` permanently unaddressable, which is the one thing the guard exists to prevent; refusing to *mint* such a name is `is_valid_run_id`'s job and it already applies `safe_segment` identity. Addressability is otherwise unharmed — skipping the exact branch only defers to partial matching, which still enumerates and still matches a legacy dir by its own spelling. `_refuse_uncontained_run_dir` gates `delete_run` and `archive_run` on `run_dir` being a direct child of the runs dir, ahead of the live-session guard and NOT under `force` (an override is the operator accepting a leaked session, never a licence to rmtree elsewhere). It raises `UnconfinedWriteError` — the shape refusal this module already raises in `_project_of_run_dir` — because observation may degrade but a repair write must not: there is no partial `rmtree` to fall back to. Every existing caller was audited; all seven build `run_dir` from the same `project` they pass, so none is newly refused. No `.resolve()` inside the guard on purpose: `resolve()` can raise on a WSL-UNC host. Ablations (2026-08-25, five arms deleted/moved SINGLY, `cp` backup, all restored byte-identical, md5 083e108a3e81b374092c173f0e4c110a re-verified each time; 11 nodes under `-k "root_naming or outside_the_runs_dir"`, graded by named node outcome, not exit code): A drop `names_tree_root` from the chain -> 2 RED / 9 GREEN, red exactly on the `''` and `'.'` rows (DID NOT RAISE) — the two with POSIX reach. `'...'`, `'.. '`, `' ..'` stay green: they have no POSIX reach to lose, which is the measurement, not a gap in the test. B delete the `delete_run` guard -> 3 RED / 8 GREEN, on DID NOT RAISE. B' move it BELOW the `rmtree` -> 3 RED / 8 GREEN, on the canary assertion — the raise assertion passes. This is why the test asserts the directory SURVIVES: a raise after the removal would pass otherwise. C delete the `archive_run` guard -> 3 RED / 8 GREEN, on DID NOT RAISE. C' move it BELOW its `rmtree` -> 3 RED / 8 GREEN, on the canary. Four disjoint red sets across A/B/C plus the two placement mutants. Suite 6811 passed / 49 skipped (6800 + 11 new), pyright 0 errors, trunk fmt + trunk check --all clean (258 files). --- src/bmad_loop/runs.py | 84 ++++++++++++++++++++++++++++++++++++---- tests/test_runs.py | 90 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 7 deletions(-) diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 448ab252..93d7ac41 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -32,6 +32,7 @@ has_parent_ref, is_absolute_path, is_link_like, + names_tree_root, retrying_unlink, safe_segment, ) @@ -560,11 +561,40 @@ def short_ref(run_id: str) -> str: def _is_path_escape(ref: str) -> bool: """True when ``ref`` would steer ``run_dir_for``'s recomposition outside the - runs dir — it is absolute/drive-qualified, climbs with ``..``, or carries a - path separator of either flavour. Sub-check of the run-id charset rather than - `is_valid_run_id` itself: a run dir created by an older version (or by hand) - may bear a name we would no longer mint, and must stay addressable.""" - return is_absolute_path(ref) or has_parent_ref(ref) or "/" in ref or "\\" in ref + runs dir — it is absolute/drive-qualified, climbs with ``..``, names the runs + dir itself rather than anything inside it, or carries a path separator of + either flavour. Sub-check of the run-id charset rather than `is_valid_run_id` + itself: a run dir created by an older version (or by hand) may bear a name we + would no longer mint, and must stay addressable. + + `names_tree_root` restores this site to the three-guard pairing every sibling + already spells (`policy.py`, `adapters/profile.py`, `plugins/manifest.py`); it + was the only member of the family omitting it (#480). It closes the spellings + that recompose to the runs *root* instead of a run in it. ``""`` and ``"."`` + join to it exactly — measured here, both `runs / ""` and `runs / "."` *are* + the runs dir — so a `state.json` lying at that root made the exact branch + below hand `delete_run` the whole runs tree to `rmtree`. ``"..."``, ``".. "`` + and ``" "`` are the Win32 half of the same rule (cited, not measurable on + POSIX): the trim of trailing periods and spaces leaves ``..`` or nothing, so + they name `.bmad-loop/` or the runs dir there while both pure pathlib flavours + keep them as ordinary one-segment names. + + Addressability is unharmed: skipping the exact branch only defers to partial + matching, and a legacy dir named ``"..."`` is still enumerated by + `list_run_dirs` and still matched by its own spelling. + + `names_win32_alias`, the family's fourth member, is deliberately NOT applied + here — it would make a legacy run dir named ``NUL`` or ``run. `` permanently + unaddressable, which is the one thing this guard exists to prevent. Refusing + to *mint* such a name is `is_valid_run_id`'s job, and it already does it with + a `safe_segment` identity check.""" + return ( + is_absolute_path(ref) + or has_parent_ref(ref) + or names_tree_root(ref) + or "/" in ref + or "\\" in ref + ) def resolve_run_dir(project: Path, ref: str) -> Path: @@ -1608,6 +1638,36 @@ def _discard_state_dir(project: Path, run_id: str) -> None: shutil.rmtree(target, ignore_errors=True) +def _refuse_uncontained_run_dir(project: Path, run_dir: Path, action: str) -> None: + """Refuse to remove anything but a direct child of ``project``'s runs dir. + + The containment half of #480, and deliberately independent of how the ref was + spelled: :func:`_is_path_escape` gates the *string* an operator typed, this + gates the *path* the two destructive writes are about to hand `shutil.rmtree`. + Both are wanted. `delete_run` and `archive_run` are module-public and take a + `run_dir` outright, so a caller that composed one by some route other than + :func:`resolve_run_dir` — the TUI's selection, a record read back from disk, a + call site not yet written — never passes the ref guard at all. + + ``run_dir_for`` is the sole builder of these paths, so recomposing one from + the basename and comparing is exactly the "is a direct child" question: the + runs root itself, a nested grandchild, and anything outside the project all + differ from what it returns. Comparing against the rebuild rather than + walking `parents` keeps this tracking `RUNS_DIR` the way + :func:`_project_of_run_dir` does. + + Raises rather than degrading — observation may degrade, a repair write must + not: there is no partial `rmtree` to fall back to, and declining quietly would + report a removal that never happened. :class:`UnconfinedWriteError` is the + shape-refusal this module already raises for the same class of mistake (see + :func:`_project_of_run_dir`), and being an ``OSError`` it lands in the + handling callers already have for a removal that failed.""" + if run_dir_for(project, run_dir.name) != run_dir: + raise UnconfinedWriteError( + f"refusing to {action} {run_dir}: not a run directory under {project / RUNS_DIR}" + ) + + def delete_run(project: Path, run_dir: Path, *, force: bool = False) -> None: """Permanently remove a run directory. Callers enforce the engine-liveness guard; the session guard is enforced here (see :func:`_refuse_live_session`), @@ -1617,7 +1677,12 @@ def delete_run(project: Path, run_dir: Path, *, force: bool = False) -> None: the leak on their own say-so. It deliberately does not kill the session instead — that would be unscoped, and this project cannot prove the session is its own (which is the whole defect). Trading a possible leak of our own session - for a possible kill of someone else's is the wrong direction for an override.""" + for a possible kill of someone else's is the wrong direction for an override. + + The containment guard runs first and is NOT under ``force``: an override is + the operator accepting a leaked session, never a licence to rmtree a path + outside the runs dir.""" + _refuse_uncontained_run_dir(project, run_dir, "delete") if not force: _refuse_live_session(project, run_dir.name, "delete") shutil.rmtree(run_dir) @@ -1638,7 +1703,12 @@ def archive_run(project: Path, run_dir: Path, *, force: bool = False) -> Path: the run's ``events/``: the channel moved out of the tree, and its files are transient completion signals the watcher has already consumed — the recorded decision accepts losing them from the archive. Everything an archive is read - for later (state, journal, tasks, logs) is in the run dir and unaffected.""" + for later (state, journal, tasks, logs) is in the run dir and unaffected. + + Containment (see :func:`_refuse_uncontained_run_dir`) is checked ahead of both, + for the reason the session guard runs early: a refusal must leave no archive + directory and no tarball behind.""" + _refuse_uncontained_run_dir(project, run_dir, "archive") if not force: _refuse_live_session(project, run_dir.name, "archive") archive_dir = project / ARCHIVE_DIR diff --git a/tests/test_runs.py b/tests/test_runs.py index 93f15c05..a6088cb7 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -314,6 +314,32 @@ def test_resolve_run_dir_exact_wins_over_ambiguity(tmp_path): assert runs.resolve_run_dir(tmp_path, "20260620-143025-a1b2") == exact +@pytest.mark.parametrize("ref", ["", ".", "...", ".. ", " .."], ids=repr) +def test_resolve_run_dir_refuses_the_root_naming_refs(tmp_path, ref): + """#480: `_is_path_escape` was the one member of the guard family omitting + `names_tree_root`, so a ref naming the runs *root* still reached the exact + branch. `""` and `"."` both join to that root exactly — measured here, both + `runs / ""` and `runs / "."` *are* the runs dir — so a state.json lying there + made `bmad-loop delete ""` hand the whole runs tree to `shutil.rmtree`. The + trailing dot/space spellings are the Win32 half of the same rule, cited rather + than measurable on POSIX: the trim of trailing periods and spaces leaves `..` + or nothing, so they name `.bmad-loop/` or the runs dir there. + + The planted state.json is the load-bearing part of the fixture. Without it the + exact branch is inert for every row and the test would pass for the wrong + reason. Ablation: drop `names_tree_root` from `_is_path_escape` and the `""` + and `"."` rows redden alone — the other three have no POSIX reach to lose.""" + project = tmp_path / "proj" + _make_run(project, "20260620-143025-a1b2") + _make_run(project, "20260619-101010-a1c9") + runs_root = project / ".bmad-loop" / "runs" + (runs_root / "state.json").write_text("{}") # exactly where "" and "." land + + with pytest.raises(runs.RunRefError): + runs.resolve_run_dir(project, ref) + assert (runs_root / "state.json").is_file() # never consumed as a run + + def test_read_pid_missing_and_garbage(tmp_path): run_dir = _make_run(tmp_path, "r1") assert runs.read_pid(run_dir) is None @@ -2452,6 +2478,42 @@ def test_delete_run_proceeds_when_the_multiplexer_cannot_answer(tmp_path, monkey assert not run_dir.exists() +@pytest.mark.parametrize( + "kind", ["outside-the-project", "the-runs-root-itself", "a-nested-grandchild"] +) +def test_delete_run_refuses_a_run_dir_outside_the_runs_dir(tmp_path, kind): + """#480's containment half, and the reason it is a second guard rather than a + tighter ref check: `delete_run` is module-public and takes a `run_dir` outright, + so a path composed by any route other than `resolve_run_dir` never meets + `_is_path_escape` at all. + + The canary — not the raise — is what grades the guard's PLACEMENT: a guard that + raised *after* `shutil.rmtree` would satisfy `pytest.raises` and still have + destroyed the directory. Ablation: delete the guard and the raise assertion + reddens; move it below the `rmtree` and the canary assertion reddens alone.""" + project = tmp_path / "proj" + _make_run(project, "20260620-143025-a1b2") + runs_root = project / ".bmad-loop" / "runs" + target = { + "outside-the-project": tmp_path / "outside", + "the-runs-root-itself": runs_root, + "a-nested-grandchild": runs_root / "20260620-143025-a1b2" / "nested", + }[kind] + target.mkdir(parents=True, exist_ok=True) + canary = target / "canary.txt" + canary.write_text("survives") + + with pytest.raises(platform_util.UnconfinedWriteError, match="not a run directory under"): + runs.delete_run(project, target) + assert canary.is_file() + + # `force` is the operator accepting a leaked session, never a licence to + # rmtree outside the runs dir — so containment sits above it, not under it. + with pytest.raises(platform_util.UnconfinedWriteError): + runs.delete_run(project, target, force=True) + assert canary.is_file() + + def _escalated_run(tmp_path, spec_text, *, restore_patch_stale=None, git_project=False): """conftest's builder with this module's shape: the spec is written first (so `git_project=True` commits it), and only `(run_dir, spec)` comes back.""" @@ -2899,6 +2961,34 @@ def test_archive_run_refuses_while_the_agent_session_is_live(tmp_path, monkeypat assert not (tmp_path / ".bmad-loop" / "archive").exists() +@pytest.mark.parametrize( + "kind", ["outside-the-project", "the-runs-root-itself", "a-nested-grandchild"] +) +def test_archive_run_refuses_a_run_dir_outside_the_runs_dir(tmp_path, kind): + """Archive carries the same `shutil.rmtree` as delete and needs the same + containment (#480). It is checked ahead of the tarball for the reason the + session guard is: a refusal must leave no archive directory behind. + + Graded like the delete twin — the canary, not the raise, pins the guard above + the `rmtree`.""" + project = tmp_path / "proj" + _make_run(project, "20260611-100000-aaaa") + runs_root = project / ".bmad-loop" / "runs" + target = { + "outside-the-project": tmp_path / "outside", + "the-runs-root-itself": runs_root, + "a-nested-grandchild": runs_root / "20260611-100000-aaaa" / "nested", + }[kind] + target.mkdir(parents=True, exist_ok=True) + canary = target / "canary.txt" + canary.write_text("survives") + + with pytest.raises(platform_util.UnconfinedWriteError, match="not a run directory under"): + runs.archive_run(project, target) + assert canary.is_file() + assert not (project / ".bmad-loop" / "archive").exists() # nothing staged + + def test_archive_run_removes_the_out_of_tree_state_counterpart(tmp_path): """Archive inherits delete's tail — it removes the run dir just the same, so it would leak the same subtree. From 5a784a71252e8c2849ad7ce8677a778bcb86a3ee Mon Sep 17 00:00:00 2001 From: t Date: Tue, 25 Aug 2026 16:05:59 -0700 Subject: [PATCH 05/10] fix(sweep): reject a bundle name that is not a legal path segment (#637) A cycle-1 bundle's name becomes its directory verbatim -- `_write_intent` builds `run_dir/bundles//intent.md` with no sanitizer on the way in. BUNDLE_NAME_RE's charset is `[a-z0-9-]` with a minimum length of 2, so the reserved Windows device basenames (con, nul, aux, prn, com, lpt) all pass it, and Windows matches them case-insensitively, so lowercase is no reprieve. `safe_segment` already classifies every one of them as unsafe; the two families simply never met on this path. Gate at `validate_triage` rather than sanitizing at `_write_intent`, so a bad name fails loudly at triage where the LLM can be re-prompted and `Bundle.name` keeps meaning exactly one string -- sanitizing at use would split the display name from the on-disk name and leave the `dw-` story key carrying the raw one. The test is `safe_segment` identity, not a hand-written device list, which keeps the accepted set in lockstep with the sanitizer that defines it: the identical idiom, for the identical reason, as `runs.is_valid_run_id`. The new check is guarded on `BUNDLE_NAME_RE.match` so a name failing both gates yields one error. Bounds re-measured against the real `safe_segment` rather than trusted: an exhaustive sweep of every RE-legal name of length 2-4 fails identity on exactly the device-name families and nothing else; cycle > 1 is unexposed because the directory is `c-`, which no prefix leaves in the reserved set; and length cannot reach the gate, since RE caps a name at 40 and MAX_SEGMENT is 120. The Win32 filesystem half of the premise is cited, not measured -- this is a Linux box. 19 new nodes in tests/test_sweep.py. Two single ablations, disjoint red sets: deleting the gate reddens the 8 reserved-name rows alone; dropping the `BUNDLE_NAME_RE.match` guard reddens the 4 fails-both-gates rows alone. Those 4 rows exist because a lowercase device name passes the regex, so it can only ever raise one error -- the guard is unobservable without an input that trips both checks. --- src/bmad_loop/sweep.py | 18 ++++++++++- tests/test_sweep.py | 69 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 1c3ed224..51891101 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -21,7 +21,12 @@ from .engine import Engine, RunPaused from .escalation import critical_escalations, env_fault_pause_reason, session_failure_reason from .model import PAUSE_STORY_GATE, Phase, StoryTask -from .platform_util import atomic_write_text, atomic_write_text_confined, neutralize_surrogates +from .platform_util import ( + atomic_write_text, + atomic_write_text_confined, + neutralize_surrogates, + safe_segment, +) from .runs import _project_of_run_dir from .statemachine import advance from .workspace import discard_worktree @@ -196,6 +201,17 @@ def claim(dw_id: str, category: str) -> None: name = str(item.get("name", "")) if not BUNDLE_NAME_RE.match(name): errors.append(f"bundle name {name!r} invalid (want {BUNDLE_NAME_RE.pattern})") + # The one rule BUNDLE_NAME_RE cannot express. A cycle-1 bundle's name IS its + # directory (`_write_intent`), and the reserved Windows device basenames -- + # CON, NUL, AUX, PRN, COM, LPT -- are `[a-z0-9-]`-legal names that no + # Windows filesystem will accept as one (matched case-insensitively, so + # lowercase is no reprieve). Testing `safe_segment` identity rather than a + # hand-written device list keeps this gate in lockstep with the sanitizer + # that defines the set: the identical idiom, for the identical reason, as + # `runs.is_valid_run_id`. Guarded on the match above so one bad name yields + # one error and not two. + if BUNDLE_NAME_RE.match(name) and safe_segment(name) != name: + errors.append(f"bundle name {name!r} is not a legal path segment") if name in names: errors.append(f"duplicate bundle name {name!r}") names.add(name) diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 468552ee..d2c24ff4 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -287,6 +287,75 @@ def test_bundle_key_re_refuses_a_trailing_newline(): assert BUNDLE_KEY_RE.match("dw-c2-foo").group(2) == "c2-foo" +# ------------------------------ reserved Windows device basenames (#637) +# +# The third axis on the same "a bundle name IS a path segment" surface. A +# reserved device basename is `[a-z0-9-]`-legal and at least 2 characters, so +# BUNDLE_NAME_RE accepts every one of them, and a cycle-1 bundle turns the name +# into run_dir/bundles// verbatim -- a directory native Windows will not +# create. The gate is `safe_segment` identity, so the accepted set stays in +# lockstep with the sanitizer instead of a second hand-written device list. + + +@pytest.mark.parametrize( + "name", + ["con", "nul", "aux", "prn", "com1", "com9", "lpt1", "lpt9"], + ids=["con", "nul", "aux", "prn", "com1", "com9", "lpt1", "lpt9"], +) +def test_validate_triage_rejects_reserved_device_bundle_names(name): + """ABLATION: delete the safe_segment identity gate and every row here accepts.""" + rj = triage_result(["DW-1"], bundles=[{"name": name, "dw_ids": ["DW-1"], "intent": "do x"}]) + + plan, errors = validate_triage(rj, {"DW-1"}) + + assert plan is None + # Exactly one error, not merely one that matches: these names pass + # BUNDLE_NAME_RE, so a bare `plan is None` (or an `any(...)` over errors) + # would pass for reasons unrelated to the device name. The count is what + # says the two name gates report a given defect once between them. + assert len(errors) == 1 + assert repr(name) in errors[0] + assert "not a legal path segment" in errors[0] + + +@pytest.mark.parametrize( + "name", + ["CON", "nul.", "aux.txt", "lpt9 "], + ids=["uppercase", "trailing-dot", "extension", "trailing-space"], +) +def test_validate_triage_reports_one_error_when_a_name_fails_both_gates(name): + """ABLATION: drop the `BUNDLE_NAME_RE.match(name) and` guard and each row + double-reports -- two errors for one name. Unlike the reserved-name rows + above, these fail BUNDLE_NAME_RE *and* safe_segment identity, so they are + the only inputs on which that guard can be observed at all.""" + rj = triage_result(["DW-1"], bundles=[{"name": name, "dw_ids": ["DW-1"], "intent": "do x"}]) + + plan, errors = validate_triage(rj, {"DW-1"}) + + assert plan is None + assert len(errors) == 1 + assert repr(name) in errors[0] + assert "invalid" in errors[0] + + +@pytest.mark.parametrize( + "name", + ["com10", "console", "com", "lpt", "aux2", "nul-fix", "a" * 40], + ids=["com10", "console", "com", "lpt", "aux2", "nul-fix", "max-length"], +) +def test_validate_triage_accepts_ordinary_bundle_names(name): + """The over-refusal guard: `com10` and `console` merely start with a device + name, and a 40-character name is BUNDLE_NAME_RE's maximum -- well under + platform_util.MAX_SEGMENT (120), so length never reaches the new gate.""" + rj = triage_result(["DW-1"], bundles=[{"name": name, "dw_ids": ["DW-1"], "intent": "do x"}]) + + plan, errors = validate_triage(rj, {"DW-1"}) + + assert errors == [] + assert plan is not None + assert plan.bundles[0].name == name + + def test_validate_triage_truncates_overlong_bundle_name(): """ABLATION A1: delete direct-bundle normalization and this fails on validation.""" rj = triage_result( From 84f2f6687d185399a0ab244556c711e14d7fc934 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 25 Aug 2026 18:23:01 -0700 Subject: [PATCH 06/10] fix(sweep): gate the decision-option bundle_name on the same rule (#637) `validate_triage` gates bundle names in TWO places, and phase 5 landed only the first. The other is a decision option's `bundle_name`, validated against BUNDLE_NAME_RE alone. It is not inert: a build-effect option's `bundle_name` becomes `Bundle.name` in `_materialize_bundles`, which feeds `_run_bundle` -> `dirname` -> `_write_intent` -- the same cycle-1 directory, by the same mechanism, so `con`/`nul`/`com1` reached it exactly as they did through the `bundles` loop. Two lines of the identical shape as the sibling gate, with the message keeping this site's own subject spelling (`bundle_name`, and the `decision {id} option {key}:` prefix the checks beside it use) on the "is not a legal path segment" grammar shared with the `bundles` loop, so one substring matches both. Guarded on `BUNDLE_NAME_RE.match` for the same one-error-per-name reason -- and, unlike at the sibling site, that guard carries a SECOND duty here: an option's `bundle_name` is optional, and `safe_segment("")` is non-identity, so an unguarded identity test would report an error for every option that names no bundle at all. ABLATIONS (2026-08-25, `cp` backup, arms applied SINGLY, restored byte-identical with md5 b4914b2254f7806e58eb46cb7fe6f10c re-verified each time; 15 nodes under `-k "reserved_device_option or one_option_error or ordinary_option"`, graded by NAMED NODE OUTCOME rather than exit code): A delete the identity gate -> 8 RED / 7 GREEN, red exactly on the reserved-device rows. B drop `BUNDLE_NAME_RE.match(...) and` -> 15 RED, ALL of them. Not the expected 4: the bare drop also loses the absent-name skip, so every filler `keep-open` option reports on its empty `bundle_name`. That is the site difference above, measured rather than reasoned. B' drop only the match TERM, keeping a truthiness skip -> 4 RED / 11 GREEN, red exactly on the fails-both-gates rows. This is the arm that isolates the double-report property, and its red set is disjoint from A's. The fails-both-gates rows (`CON`, `nul.`, `aux.txt`, `lpt9 `) exist for the reason phase 5 recorded: a lowercase device name PASSES BUNDLE_NAME_RE, so it raises exactly one error with or without the guard, and an error-count assertion over such names is green either way. Only an input failing both gates can observe the guard at all. The out-of-band path is deliberately untouched: `_materialize_bundles` also honours `answer["bundle_name"]` from a pre-answer that never passes `validate_triage`. That is wider than this program characterized and no phase has measured it; it is recorded in the PR body as residual work. Suite 6845 passed / 49 skipped, pyright 0 errors, `trunk fmt` left this file byte-identical so the ablation record grades it as committed. --- src/bmad_loop/sweep.py | 12 ++++++ tests/test_sweep.py | 96 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 51891101..a4ac6ec6 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -267,6 +267,18 @@ def claim(dw_id: str, category: str) -> None: bundle_name = str(raw.get("bundle_name", "")) if bundle_name and not BUNDLE_NAME_RE.match(bundle_name): errors.append(f"decision {dw_id} option {key}: bad bundle_name {bundle_name!r}") + # The second site that mints a bundle directory, gated for the reason + # stated at the `bundles` loop above. A build-effect option's + # `bundle_name` becomes `Bundle.name` in `_materialize_bundles`, so it + # reaches `_write_intent`'s cycle-1 directory by the identical path -- + # `BUNDLE_NAME_RE` is no more able to express the rule here than there. + # Guarded on the match above so one bad name yields one error, and on + # nothing else: an absent `bundle_name` fails that match already. + if BUNDLE_NAME_RE.match(bundle_name) and safe_segment(bundle_name) != bundle_name: + errors.append( + f"decision {dw_id} option {key}: bundle_name {bundle_name!r} " + "is not a legal path segment" + ) if effect == "build" and bundle_name: if bundle_name in names: errors.append(f"duplicate bundle name {bundle_name!r}") diff --git a/tests/test_sweep.py b/tests/test_sweep.py index d2c24ff4..5460b4e4 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -356,6 +356,102 @@ def test_validate_triage_accepts_ordinary_bundle_names(name): assert plan.bundles[0].name == name +# ------------------------- the same rule at the decision-option site (#637) +# +# `validate_triage` gates bundle names TWICE. A build-effect option's +# `bundle_name` becomes `Bundle.name` in `_materialize_bundles`, so it reaches +# `_write_intent`'s cycle-1 directory by the same path the `bundles` loop above +# does -- and it was validated against BUNDLE_NAME_RE alone. + + +def _option_bundle_decision(bundle_name): + """One otherwise-clean build decision whose only possible defect is its + option's `bundle_name`. That cleanliness is what lets the tests below assert + an error COUNT: a decision also carries question / >=2 options / + recommendation rules, any of which would add errors of their own.""" + return triage_result( + ["DW-1"], + decisions=[ + { + "id": "DW-1", + "question": "build it?", + "options": [ + { + "key": "1", + "label": "build", + "effect": "build", + "intent": "fix it", + "bundle_name": bundle_name, + }, + {"key": "2", "label": "keep", "effect": "keep-open"}, + ], + "recommendation": "1", + } + ], + ) + + +@pytest.mark.parametrize( + "bundle_name", + ["con", "nul", "aux", "prn", "com1", "com9", "lpt1", "lpt9"], + ids=["con", "nul", "aux", "prn", "com1", "com9", "lpt1", "lpt9"], +) +def test_validate_triage_rejects_reserved_device_option_bundle_names(bundle_name): + """ABLATION: delete the safe_segment identity gate at the decision-option + site and every row here accepts. The `bundles` loop's gate does not reach + this value -- it is a different loop over a different key.""" + rj = _option_bundle_decision(bundle_name) + + plan, errors = validate_triage(rj, {"DW-1"}) + + assert plan is None + # Exactly one error, not merely one that matches: these names pass + # BUNDLE_NAME_RE, so a bare `plan is None` would pass for reasons unrelated + # to the device name. + assert len(errors) == 1 + assert repr(bundle_name) in errors[0] + assert "not a legal path segment" in errors[0] + assert "option 1" in errors[0] + + +@pytest.mark.parametrize( + "bundle_name", + ["CON", "nul.", "aux.txt", "lpt9 "], + ids=["uppercase", "trailing-dot", "extension", "trailing-space"], +) +def test_validate_triage_reports_one_option_error_when_a_name_fails_both_gates(bundle_name): + """ABLATION: drop the `BUNDLE_NAME_RE.match(bundle_name) and` guard and each + row double-reports -- two errors for one name. The rows above cannot show + this: a lowercase device name PASSES BUNDLE_NAME_RE, so it raises exactly one + error with or without the guard. Only an input failing both gates can + observe it at all.""" + rj = _option_bundle_decision(bundle_name) + + plan, errors = validate_triage(rj, {"DW-1"}) + + assert plan is None + assert len(errors) == 1 + assert repr(bundle_name) in errors[0] + assert "bad bundle_name" in errors[0] + + +@pytest.mark.parametrize( + "bundle_name", + ["com10", "console", "nul-fix"], + ids=["com10", "console", "nul-fix"], +) +def test_validate_triage_accepts_ordinary_option_bundle_names(bundle_name): + """The over-refusal guard at this site: `com10` and `console` merely start + with a device name, and the gate must not reach them.""" + rj = _option_bundle_decision(bundle_name) + + plan, errors = validate_triage(rj, {"DW-1"}) + + assert errors == [] + assert plan is not None + assert plan.decisions[0].option("1").bundle_name == bundle_name + + def test_validate_triage_truncates_overlong_bundle_name(): """ABLATION A1: delete direct-bundle normalization and this fails on validation.""" rj = triage_result( From a088db29009ddeb5b5d6a776fb08f890851310c2 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 25 Aug 2026 18:23:30 -0700 Subject: [PATCH 07/10] docs(guards): record the is_absolute anchoring rule and the Win32 path-guard family (#480) #480's item 4 asks six anchoring helpers to swap stdlib `Path.is_absolute()` for `platform_util.is_absolute_path`, on the claim that a root-anchored operand is "joined under paths.project" on Windows. The mechanism is refuted: Windows joins such an operand by keeping the base's DRIVE and discarding the rest, so `/etc/passwd` under `C:\proj` lands at `C:\etc\passwd` -- OUTSIDE the project, not under it. The swap yields the identical result there and LOOSENS `C:foo`, which stdlib resolves to the contained `C:\proj\foo` and the family predicate would pass through. So item 4 ships as a documentation outcome. No swap is performed. recovery_flow.py's `_restore_attempt_owned_spec_bytes` is the one genuine REFUSAL guard in the tree built on stdlib `is_absolute()` -- the category the issue asserts does not exist. It fails CLOSED on Windows (a POSIX-absolute spelling reads as not-absolute there, so it raises) and `is_absolute_path` would answer True and open it. A comment records that, because the next path-guard sweep would otherwise "fix" it. The question at that site is the platform's own: a live path this process is about to `resolve(strict=True)` and write through on the host it is running on -- the opposite of the "must stay inside the project" question the family predicate is built for. Same shape of note, for the same reason, as `runs._state_base`'s. `verify.resolve_spec_path` had no docstring. It gains one stating the RULE its 14 call sites follow rather than an enumeration that would drift -- but NOT the rule the plan proposed. "A repair write pairs it with `spec_within_roots`; an observer does not" is a biconditional, and it is false in both directions: two callers that write nothing (the attempt-binding observations) DO check, and two that write (the post-dev board sync, the sweep bundle's ledger close) do NOT. The rule that actually holds is positive-only and scoped to the spec: a caller that goes on to REWRITE the spec must pair this with `spec_within_roots` first. The two unpaired writers are named and their reason given -- they write to a deterministic orchestrator-owned target, so an out-of-tree spec steers what they write and never where. DOCS. FEATURES.md's seeding and git-add-shield bullets record that the pathological spellings are now refused at config load, so the render site never sees one -- which is the only direction in which the trailing-space case closes, since PR #708's escaping fixed the gitignore parse and still named a path `mkdir` never created. porting-to-a-new-os.md gains a section on the four predicates: `platform_util` was named nowhere under docs/ before this. testing.md's "currently exactly four" conftest fixtures is corrected to five (`_isolate_state_root` was added and the doc was not). CHANGELOG under `## [Unreleased]`: the config refusal under `Changed`, led by the compatibility break; the `_is_path_escape` parity + run-dir containment and the bundle-name gate under `Fixed`. No version bump -- `sync_version.py` owns those and the bump is a separate release PR. The break makes the next release a MINOR. ABLATION for the new recovery_flow test (2026-08-25, `cp` backup, restored byte-identical with md5 b39204d831954e3c4c45902fb85914a2 re-verified, graded by named node outcome): 1 delete `not spec_path.is_absolute()` ALONE -> GREEN. Measured, and it is the finding rather than a hole: on POSIX the `resolve(strict=True)` fixed-point term below subsumes it, because a relative path never equals its own resolve. The term is load-bearing on Windows only, so no POSIX test can protect it from deletion -- the comment is what has to, which is precisely why it was worth writing. 2 delete the WHOLE refusal -> RED, on the CANARY (`assert not spec.parent.exists()`), with the `pytest.raises` assertion still GREEN: the post-mkdir recheck below still fires on the relative parent, so something raises after `mkdir` has already run. Phase 4's raise-vs-canary lesson, reproduced exactly -- a refusal test asserting only the raise would have passed against a guard that had already created the directory. Full suite 6846 passed / 49 skipped (baseline 6830 + 16), pyright 0 errors, `trunk fmt` (which touched only the new doc section) then `trunk check --all` clean over 258 files. `git grep names_win32_alias src/` shows the definition, the Unity clone's definition, and SEVEN call sites -- all wiring landed. --- CHANGELOG.md | 30 +++++++++++++++++++++ docs/FEATURES.md | 4 +-- docs/porting-to-a-new-os.md | 49 ++++++++++++++++++++++++++++++++++ docs/testing.md | 7 ++++- src/bmad_loop/recovery_flow.py | 15 +++++++++++ src/bmad_loop/verify.py | 26 ++++++++++++++++++ tests/test_recovery_flow.py | 39 +++++++++++++++++++++++++-- 7 files changed, 165 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2eee5fff..3cb66092 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,18 @@ breaking changes may land in a minor release. states the rule a native-id backend must follow rather than leaving it to be inferred from psmux's per-seam specifics, and `TerminalMultiplexer.new_parked_window` now says its id is opaque and MAY be qualified, matching `new_window`. Documentation only; no behavior change. +- **A config path component that names a Windows device, or ends in a period or space, is + refused at load** (#480). Values that were accepted before — `skill_tree = "NUL"`, a + `seed_files` entry of `aux.json`, a `worktree_seed` of `.claude/skills.` — now raise at all + seven validation sites: `scm.worktree_seed`, an adapter's `hooks.config_path` / `skill_tree` / + `seed_files`, a plugin's `seed_files` / `seed_globs` and `[python] module`, and the Unity + seeder's guard dir. Such a component names a _different_ path on Windows than the one it + spells — a device rather than a file, or a sibling once Win32 strips the trailing run — so the + file that gets seeded and the exclude pattern rendered from the authored spelling disagree + about which path they mean. The refusal is cross-platform on purpose, matching how the family + already rejects `C:\secrets` on POSIX: a config value must not mean one thing per host. No + shipped profile, bundled `plugin.toml` or default trips it, but this is a compatibility break + on previously-loading config. ### Fixed @@ -72,6 +84,24 @@ breaking changes may land in a minor release. - The git-add shield's rollback report no longer raises through its own stderr decode on Windows (#394). `_shield_undo_extension` is contracted never to raise, but the `fsdecode` of a failing `--unset-all`'s stderr was guarded for `GitError` alone. +- **A run ref that names the runs directory itself is refused, and the two destructive run-dir + writes are contained** (#480). `runs._is_path_escape` was the only member of the seven-site + guard family omitting `names_tree_root`, so `""` and `"."` joined to the runs root exactly, + and `delete_run` removed whatever it was handed with a bare `rmtree`. That reach needed a + `state.json` lying at the runs root — without one those refs fall through to partial matching + and can only name a child — and the ref is the operator's own argv, so this was a footgun + rather than an escalation. `delete_run` and `archive_run` now also refuse a `run_dir` that is + not a direct child of the runs directory, raising `UnconfinedWriteError` ahead of the + live-session guard and not waived by `--force`. +- **A sweep bundle name that is not a legal path segment is refused at triage** (#637), at both + of `validate_triage`'s bundle-name sites — the `bundles` list and a decision option's + `bundle_name`, which becomes `Bundle.name` by way of `_materialize_bundles`. A cycle-1 + bundle's name becomes its directory verbatim, and the reserved device basenames are all + `[a-z0-9-]`-legal, so `BUNDLE_NAME_RE` accepted `con`, `nul` and `com1` while no Windows + filesystem would create the directory — matched case-insensitively, so lowercase was no + reprieve. The test is `safe_segment` identity rather than a second hand-written device list, + which keeps the accepted set in lockstep with the sanitizer that defines it: the same idiom, + for the same reason, as `runs.is_valid_run_id`. ### Security diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 5edc8cb4..72b5925f 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -81,9 +81,9 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Dirt in your MAIN checkout only blocks a unit merge when the merge — **or the run's own post-merge bookkeeping** — could commit it (#460, #618). Two questions, asked per path over the strays that lie outside the branch's incoming set. What the MERGE can commit is what git has **staged**, so the axis is the index column rather than trackedness: an untracked stray and a tracked stray edited in the working tree only are both inert — measured on git 2.55.0 across both topologies and both strategies, rc 0, the edit survives uncommitted, and it is absent from the resulting commit — so both are left exactly where they are and the guard journals `merge-target-tolerated` naming them. Before this, one unrelated `notes.txt` (untracked, #460) or one saved-but-unstaged edit (#618) in the main checkout escalated the first story and paused an unattended run. A **staged** stray still escalates, because `merge --no-ff` refuses it outright and a fast-forwardable `merge --squash` folds it into the story's commit — against a diverged target `--squash` refuses too, with the same error as `--no-ff`, so the fold is a fast-path artifact rather than a squash property. What the RUN can commit is the second question: the post-merge carries stage the sprint board and the deferred-work ledger **by pathspec** (`git add -- :(literal)`), which takes whatever the working tree holds no matter who wrote it, so **any** dirt on one of those two paths escalates whatever its index column says — otherwise an operator's private reopen of a story row rides out under a `chore(sprint-status): carry …` message with the tree left clean and nothing to read the substitution back from. That protection covers artifacts git already **tracks**: an untracked one has no baseline to diverge from, the orchestrator has been reading that exact file as its own all along, and committing it whole is how a non-ignored board first reaches git (#350) — protecting it would halt the first story of every project that has yet to commit its board. Either escalation names the paths and asks you to commit, stash or revert them (it never cleans them for you), and each names its own remedy, since staged work has to be committed or unstaged while dirt on a carried path has to leave the path entirely. `merge-target-tolerated` records what the **guard** decided and is emitted before the merge, so a stray it waved through by path can still clash with the incoming commit by **shape** — a file where the merge needs a directory, or the reverse — and git then refuses at pre-flight over the very path the event called harmless; that run journals a corrective `merge-preflight-refused` beside it, naming the same paths and carrying git's raw text (#623). A `per_worktree` engine Editor leaking _this branch's own_ files into the main checkout is still auto-cleaned and journaled `merge-target-cleaned`, since those are duplicates of content the branch already committed. - A merge-back that fails escalates in one of **five** typed shapes rather than one, plus an honest fallback for anything unclassified (#619). Git declining at **pre-flight** — an untracked file the merge would overwrite, a staged change on an incoming path, a file/directory shape clash, a `merge_strategy = "ff"` target that cannot fast-forward — is not a conflict: nothing was merged, the target checkout is exactly as it was, and there are no markers to find. That escalation now says so and lets git's appended text name the cause and the paths, instead of sending you to resolve a conflict that does not exist; a genuine content conflict keeps the resolve-by-hand wording, and both keep the unit's branch and worktree mounted for manual recovery. The third shape is a `--no-ff` that merged cleanly and was then refused at the **commit** — a `pre-merge-commit` or `commit-msg` hook exiting non-zero, or a `commit.gpgsign` that cannot sign. Nothing conflicted, so it leaves no unmerged stages, but it does leave `MERGE_HEAD`: an index-only reading calls that started merge a pre-flight refusal and sends you to clear a clash that does not exist. bmad-loop aborts it, restores the checkout, and points you at the policy that declined instead. Telling the three apart takes **both** probes — `git ls-files -u` leads and answers content, and `MERGE_HEAD`, read before the abort that erases it, parts a merge that never started from one that started and could not be sealed. Neither alone is enough: a conflicted `merge --squash` writes three unmerged stages and conflict markers while creating no `MERGE_HEAD` at all. The `squash` leg reaches the third shape through a different door: `--squash` itself stops before committing by design, so no hook or signature can refuse the merge invocation — but the leg then seals the staged result with its own plain `git commit`, where commit hooks and `commit.gpgsign` run like anywhere else, and a refusal there is the same third shape. That one is rolled back with `git reset --hard HEAD`, gated on the pre-merge reading having found the tree clean — a checkout already carrying uncommitted work of yours is never reset, and the reset is deliberately whole-tree, since it undoes a merge that SUCCEEDED, whose staged result spans the entire incoming set (the stated ceiling: an edit landing after that clean reading rides the reset); when the rollback is withheld the squash result is left **staged**, and the escalation says so and names clearing it as your first step. Neither the exit code nor the message text can stand in, the same refusal being rc 1 or rc 2 depending on topology, rc 1 also being what a conflict returns, and the message being fully translated. The fourth shape is the one **all three** strategies reach and no index reading can see: git dying part-way through the CHECKOUT. It materializes the incoming files in index order, so a failure partway — measured under a **required** clean/smudge filter that cannot run, on all three strategies — stops with HEAD where it was and the tree already partly rewritten. `--ff-only` is not exempt: it declines the _topology_ question before touching anything, but once the fast-forward is possible it checks the incoming tree out like any other merge, so the "it never starts a merge" premise that used to excuse this leg from checking was simply wrong. That leaves no unmerged stages, no `MERGE_HEAD`, and a tree that reads clean against HEAD (an untracked file is in neither HEAD nor the index), so every probe above calls it a pre-flight refusal and tells you the checkout is untouched — while the residue refuses the NEXT merge as an untracked-overwrite, identically on every resume, over paths nothing named. Residue probes answer it: the untracked set and the dirty-tracked set are each sampled **before** the merge, differenced after, and the deltas intersected with the branch's incoming set — the only paths a merge can write — so the answer is "git wrote this", not "this is here" and not "something changed while the merge ran". The intersection closes the seventh mislabeled state: with attribution read off a repo-wide "the tree is dirty now" boolean, an edit YOU made to an unrelated tracked file while the merge was failing was attributed to git — a genuine pre-flight refusal classified as "failed part-way through checkout", and the repo-wide `reset --hard HEAD` riding on that attribution destroyed your edit (measured on all three strategies). The residue has two axes and they get different answers. An incoming path the target did not already track lands **untracked**, and nothing reaches it — neither `git merge --abort` (which exits 128 here anyway, there being no merge to abort) nor `git reset --hard` touches untracked files — so it is named for you to clear rather than cleaned, since the delta proves git wrote a path, not that the bytes now there are yours or git's. An incoming path the target **did** track is rewritten in place, which a path-scoped `git checkout HEAD -- ` over exactly the attributed paths does undo, so that half is undone for you on every leg — never by a repo-wide reset, whose blast radius would take your own uncommitted work on paths the attribution deliberately left alone — and the escalation asks only for whichever residue actually survived, rather than reciting both; a failed restore names the exact paths and prescribes the same path-scoped command. Two ceilings worth knowing, both per path rather than per tree: a path that was _already_ dirty before the merge stays unattributable — its delta cannot say which bytes are whose — and an edit landing **during** the merge window on a path **inside** the incoming set is indistinguishable from git's own write, so it is restored with it; "cannot tell" and "git did nothing" fail to the same side on purpose, the alternative being a restore over your uncommitted work. The differencing is load-bearing and not tidiness: an absolute reading would reclassify every genuine pre-flight refusal that happens to have an untracked stray in the checkout, which is exactly the stray the guard above deliberately tolerates. Relatedly, a `squash` merge git refuses no longer discards your uncommitted work: `--squash` has no `--abort`, so the recovery is that same path-scoped restore over whatever the deltas attribute to git — your pre-existing dirt is never touched, and a conflict landing on top of it has the conflicted incoming paths restored while your edit stays put. The fifth shape is one of the classifier's own post-merge readings failing after a failed merge — the residue deltas or the incoming set that attributes them, the unmerged-stages reading, or the `MERGE_HEAD` reading: what rested on that reading is then **unverified**, and the escalation says exactly that — every verdict still standing on its own live measurement keeps its class, cleanup not gated on the dead reading still runs, and an unread `MERGE_HEAD` skips the abort it gates and says so rather than repairing on uncertainty — and you are sent to the one reading the run could not take, your own `git status`, instead of a probe failure impersonating a verdict. A reading can die on the far side of success too: a `merge --squash` that succeeded still needs one index reading to tell a no-op replay from a result to commit, and with that reading dead the same unverified escalation fires — nothing committed, nothing reset, any staged result left in place — rather than the doomed `git commit` that used to dress the failure as a commit refusal and roll the tree back on the fiction. The conflict itself is typed from its own measurement too (the unmerged stages), so the final catch-all no longer equates "anything unrecognized" with "content conflict": a failure none of the probes classified escalates saying just that — the run cannot say what state the checkout is in; `git status` and git's own appended text are the sources — rather than prescribing conflict resolution for a state nothing measured. - Failed-unit forensics: a deferred/escalated unit's worktree + branch stay mounted (`keep_failed`, default on) and its full diff is preserved to `run_dir/failed//changes.patch`; `failed_diff_max_mb` caps per-file untracked-file size (oversized skipped with a marker), `failed_diff_unlimited` lifts the cap. -- Config seeding: a worktree checks out _tracked_ files only, so a project's gitignored MCP/CLI configs (`.mcp.json`, `.claude/settings.json`, `.codex/config.toml`, `.gemini/settings.json`) would be missing — an isolated session couldn't reach its MCP server. With `seed_adapter_defaults` (default on) each loaded adapter's own `seed_files` are copied in from the main repo before the session launches, together with every non-hookless adapter's resolved hook `config_path` — which is gitignored for codex, so without it the project's own hook configuration never reached the worktree and the session ran against a file holding bmad-loop's relay registrations alone (#471). Seeding does not decide whether that relay hook registers — the hook-merge writes it either way, with one refusal: a seeded config that is present but **will not parse** stops provisioning outright (`GitError` → CRITICAL escalation, run paused) instead of being read as an empty document. JSON has no partial read, so an unparseable config is evidence of an earlier fault rather than a blank slate — and merging the relay into `{}` would publish a hooks-only file over the operator's allowlist, `env` and MCP entries, erasing that evidence along with them. The bytes are left exactly as found, and the refusal names whichever source actually supplies them rather than the copy it read: an escalated story re-enters the run only through a re-arm, which discards the worktree and provisions a fresh one, so a repair applied to the disposable copy is undone before the next drive. Which source that is comes from the seed bookkeeping rather than from a counterpart's existence — seeding is copy-when-absent, so a config the project TRACKS is skipped as an occupied destination and arrives with the branch checkout, and that lane is told to commit the repair on the target branch while a genuinely seeded one is sent to the main checkout. That bookkeeping is recorded per path actually written rather than per seed entry: a directory entry seeds child by child, so its presence proves that some child landed and never that this config did, and inferring from it would advise committing a gitignored settings file the checkout supplied. The remedy names the re-arm (`bmad-loop resolve --no-interactive`) because `ESCALATED` is terminal: repairing the file alone leaves the story out of the run (#592). Invalid UTF-8 takes the same lane, which before this crashed the engine instead of escalating. `worktree_seed` adds extra paths. Copy-when-absent at file granularity — a directory entry whose destination already exists (a worktree checkout carries its tracked children) still seeds the children that are missing — seeded before the hook-merge (a seeded `settings.json` keeps its content, but its relay entry is replaced rather than kept — the seeded copy names the main repo's `$CLAUDE_PROJECT_DIR`-relative relay, which resolves to the worktree, where no relay exists; left in place it would read as already registered and the session would stall silently, #352; a _tracked_ config gets the same rewrite pinned `skip-worktree` in the worktree's own index, so the machine-specific command never rides `git add -A` into a story commit — while pinned the config is orchestrator-owned, so a story's own edit to it stays session-local), and shielded from the unit's `git add -A` — in a private exclude scoped to that worktree alone (see below), never repo-wide. +- Config seeding: a worktree checks out _tracked_ files only, so a project's gitignored MCP/CLI configs (`.mcp.json`, `.claude/settings.json`, `.codex/config.toml`, `.gemini/settings.json`) would be missing — an isolated session couldn't reach its MCP server. With `seed_adapter_defaults` (default on) each loaded adapter's own `seed_files` are copied in from the main repo before the session launches, together with every non-hookless adapter's resolved hook `config_path` — which is gitignored for codex, so without it the project's own hook configuration never reached the worktree and the session ran against a file holding bmad-loop's relay registrations alone (#471). Seeding does not decide whether that relay hook registers — the hook-merge writes it either way, with one refusal: a seeded config that is present but **will not parse** stops provisioning outright (`GitError` → CRITICAL escalation, run paused) instead of being read as an empty document. JSON has no partial read, so an unparseable config is evidence of an earlier fault rather than a blank slate — and merging the relay into `{}` would publish a hooks-only file over the operator's allowlist, `env` and MCP entries, erasing that evidence along with them. The bytes are left exactly as found, and the refusal names whichever source actually supplies them rather than the copy it read: an escalated story re-enters the run only through a re-arm, which discards the worktree and provisions a fresh one, so a repair applied to the disposable copy is undone before the next drive. Which source that is comes from the seed bookkeeping rather than from a counterpart's existence — seeding is copy-when-absent, so a config the project TRACKS is skipped as an occupied destination and arrives with the branch checkout, and that lane is told to commit the repair on the target branch while a genuinely seeded one is sent to the main checkout. That bookkeeping is recorded per path actually written rather than per seed entry: a directory entry seeds child by child, so its presence proves that some child landed and never that this config did, and inferring from it would advise committing a gitignored settings file the checkout supplied. The remedy names the re-arm (`bmad-loop resolve --no-interactive`) because `ESCALATED` is terminal: repairing the file alone leaves the story out of the run (#592). Invalid UTF-8 takes the same lane, which before this crashed the engine instead of escalating. `worktree_seed` adds extra paths. Every configured seed path is refused at **config load** if any component names a Windows device or ends in a period or space (#480) — `worktree_seed`, an adapter's `seed_files`/`seed_globs`, its hook `config_path` and `skill_tree`, a plugin's `[python] module`, and the Unity seeder's guard dir all take the same refusal, on every platform. Such a component names a _different_ path on Windows than the one it spells: `NUL` and `aux.json` are devices rather than files, and `.claude/skills.` is created as `.claude/skills` because Win32 strips a trailing run of periods and spaces before the path reaches the filesystem — so the copy lands somewhere the operator did not configure, and the exclude pattern the shield later renders from the authored spelling names a path that does not exist. Refusing at load is what keeps the render site below from ever seeing one. It is a compatibility break on config that previously loaded, and a hard refusal rather than a warning for the reason the value is dangerous at all — it means one thing here and another there, and a run that quietly seeds a different file than the one configured is the failure the guard exists to prevent. Copy-when-absent at file granularity — a directory entry whose destination already exists (a worktree checkout carries its tracked children) still seeds the children that are missing — seeded before the hook-merge (a seeded `settings.json` keeps its content, but its relay entry is replaced rather than kept — the seeded copy names the main repo's `$CLAUDE_PROJECT_DIR`-relative relay, which resolves to the worktree, where no relay exists; left in place it would read as already registered and the session would stall silently, #352; a _tracked_ config gets the same rewrite pinned `skip-worktree` in the worktree's own index, so the machine-specific command never rides `git add -A` into a story commit — while pinned the config is orchestrator-owned, so a story's own edit to it stays session-local), and shielded from the unit's `git add -A` — in a private exclude scoped to that worktree alone (see below), never repo-wide. - Seeding is verified **result-side**, by re-probing the worktree on disk rather than trusting the copier's bookkeeping — so a user-authored `worktree_seed` rel can neither forge nor mask a report: `worktree-seed-skipped` (a seed entry that copied nothing), `worktree-seed-dropped` (a repo-carried seed that never arrived), and `worktree-module-skills-dropped` (#464 — a wheel-bundled `bmad-loop-*` skill whose content came up short, e.g. a checkout file squatting the skill's directory, which the per-file no-clobber refuses to replace) are all **informational**, journaled and never a pause. Presence is the whole contract; content is never compared, since no-clobber is per _file_ and a checkout's own divergent fork of a bundled skill is a healthy shape. Only the upstream dev/review skill and renderer completeness probes escalate, because those have a deterministic in-worktree consumer — the session dispatches them inside the worktree and stalls having written nothing — while the bundled operator/triage skills are dispatched at the **main checkout** (`/bmad-loop-sweep` from sweep triage, `bmad-loop resolve`; `bmad-loop-setup` has no session consumer at all), so their absence in a worktree stalls nothing. -- The git-add shield is scoped to the worktree and expires with it (#384). Provisioned tool files (skill trees, hook config, seeded configs) go into a private `.git/worktrees//info/exclude`, activated by a worktree-scoped `core.excludesFile` that shadows rather than concatenates with your own — so yours is copied into the private one **byte for byte**, the shield covers that unit only, `git worktree remove` takes it away, and the shared, permanent `.git/info/exclude` is never written. An explicitly empty `core.excludesFile` is honored literally — no excludes file at all — rather than read as unset, so git's XDG fallback is not consulted and there is nothing to copy. Where that fallback IS consulted, the file it names is asked of git rather than computed in Python — and on **Git for Windows >= 2.46** the file is `%APPDATA%/Git/ignore` whenever that file exists, which the fork prefers over the `$HOME/.config/git/ignore` upstream resolves (#403). Seeding the `$HOME` one there was wrong in both directions: with only the APPDATA file present the seed came back empty and the activation then shadowed the ignore file git really reads, and with both present the worktree over-ignored with patterns git was not applying, so session-created files went missing instead. The gate is the reported version's own `.windows.` fork string plus that floor rather than the platform — Cygwin, MSYS2 and WSL gits run on Windows hardware without the patch — and anything else, an unanswerable `git version` included, keeps the `$HOME` answer. Read from the fork's source through #403, **not measured on a Windows machine**. It then **proves it applies or stands down**: bmad-loop asks git which excludes file it actually resolves **and from which scope** (`git config --show-scope`), so the degrade reason names the winner rather than inferring one — ambient command-scope config (`git -c`, `GIT_CONFIG_PARAMETERS`, `GIT_CONFIG_COUNT`) is told apart from a worktree-scoped write that is not in force at all, which sends you to a different repair (#692). Anything but a confirmed byte-identical match — either of those, an unreadable excludes file, an answer naming no scope, an unanswerable probe — skips the shield with a journaled and notified reason; a match activates it whatever scope supplied the value, since the thing being proved is that git reads the file bmad-loop wrote. Two runs against one repository serialize on an exclusive lock, leaving a zero-length `.git/bmad-loop-shield.lock` — never in the working tree, so nothing your `git add -A` can see; on Windows the wait gives up after ~10s and the shield is skipped naming the lock. Caveats: its gate is the project's **git 2.34** support floor (an older git — or one that cannot say what it is — skips the shield, and the repo-format flag is deliberately not written); that is a _support_ refusal rather than a capability one, since the two things the shield is built out of, `extensions.worktreeConfig` and `git config --worktree`, have existed since git 2.20 — runs are refused outright below the floor, so what this gate still catches on a current host is a git that cannot be spawned, times out, or answers unparseably. Enabling it sets `extensions.worktreeConfig` — a **permanent** repo-format flag, rolled back wherever it could be left set without a working shield, but surviving in two cases the reason distinguishes: a sibling worktree still depends on it, or the rollback could not be made at all. Where it cannot be set safely at all (`core.bare = true` or `core.worktree` in the shared config) the shield is skipped instead. It is skipped the same way where the shared config already carries an explicit `extensions.worktreeConfig` that is not `true` (#396): that line is an operator's own declaration, and enabling over it would rewrite it to `true` permanently with nothing recording what stood there — so the shield stands down and the reason quotes the spelling it found, raw rather than `--type=bool`-normalized, since `false`, `off`, `no` and `0` all read back as `false`. A **valueless** `worktreeConfig` line is not that case: git reads it as `true`, so the flag counts as already carried and the shield proceeds. Lines an older bmad-loop wrote into `.git/info/exclude` are **not** removed for you — delete them by hand. A **file** your project tracks gets no pattern: git applies ignore rules only to untracked paths, so the pattern would shield nothing while making the file read as tracked-and-ignored to `git ls-files -ci --exclude-standard` and to repo-hygiene gates built on it (#392). A tracked **directory** gets no directory pattern either — it is replaced by one pattern per file this provisioning run actually wrote below it (#484). A dir pattern does hide new children, and no pattern shape both hides them and keeps the report clean (`dir/*` with per-child negations clears the report and leaks a new file into the commit), but over a tracked tree it bought new-child coverage alone at the price of a false tracked-and-ignored report for every tracked child — modifications to those stage regardless. The accepted trade: a file the **session** creates under such a directory can be staged, which matches your own decision to track that tree, while everything the orchestrator seeded there keeps a pattern of its own. An **untracked** tool directory is unchanged — it keeps the single `/dir` pattern. If git cannot say what a path is — or a file provisioning wrote below a tracked directory has a name no exclude line can spell (an embedded newline, a trailing carriage return), which would shield nothing and loose an orphan pattern on unrelated files — the pattern is kept in its ORIGINAL shape, dir shape included, and the reason is journaled. Patterns are escaped as they are rendered (#476): a rel holding a trailing space or a wildmatch special (`*`, `?`, `[`, `\`) otherwise names something else, leaving the seeded path unshielded while the broken character class silently hides an unrelated file the unit meant to commit. +- The git-add shield is scoped to the worktree and expires with it (#384). Provisioned tool files (skill trees, hook config, seeded configs) go into a private `.git/worktrees//info/exclude`, activated by a worktree-scoped `core.excludesFile` that shadows rather than concatenates with your own — so yours is copied into the private one **byte for byte**, the shield covers that unit only, `git worktree remove` takes it away, and the shared, permanent `.git/info/exclude` is never written. An explicitly empty `core.excludesFile` is honored literally — no excludes file at all — rather than read as unset, so git's XDG fallback is not consulted and there is nothing to copy. Where that fallback IS consulted, the file it names is asked of git rather than computed in Python — and on **Git for Windows >= 2.46** the file is `%APPDATA%/Git/ignore` whenever that file exists, which the fork prefers over the `$HOME/.config/git/ignore` upstream resolves (#403). Seeding the `$HOME` one there was wrong in both directions: with only the APPDATA file present the seed came back empty and the activation then shadowed the ignore file git really reads, and with both present the worktree over-ignored with patterns git was not applying, so session-created files went missing instead. The gate is the reported version's own `.windows.` fork string plus that floor rather than the platform — Cygwin, MSYS2 and WSL gits run on Windows hardware without the patch — and anything else, an unanswerable `git version` included, keeps the `$HOME` answer. Read from the fork's source through #403, **not measured on a Windows machine**. It then **proves it applies or stands down**: bmad-loop asks git which excludes file it actually resolves **and from which scope** (`git config --show-scope`), so the degrade reason names the winner rather than inferring one — ambient command-scope config (`git -c`, `GIT_CONFIG_PARAMETERS`, `GIT_CONFIG_COUNT`) is told apart from a worktree-scoped write that is not in force at all, which sends you to a different repair (#692). Anything but a confirmed byte-identical match — either of those, an unreadable excludes file, an answer naming no scope, an unanswerable probe — skips the shield with a journaled and notified reason; a match activates it whatever scope supplied the value, since the thing being proved is that git reads the file bmad-loop wrote. Two runs against one repository serialize on an exclusive lock, leaving a zero-length `.git/bmad-loop-shield.lock` — never in the working tree, so nothing your `git add -A` can see; on Windows the wait gives up after ~10s and the shield is skipped naming the lock. Caveats: its gate is the project's **git 2.34** support floor (an older git — or one that cannot say what it is — skips the shield, and the repo-format flag is deliberately not written); that is a _support_ refusal rather than a capability one, since the two things the shield is built out of, `extensions.worktreeConfig` and `git config --worktree`, have existed since git 2.20 — runs are refused outright below the floor, so what this gate still catches on a current host is a git that cannot be spawned, times out, or answers unparseably. Enabling it sets `extensions.worktreeConfig` — a **permanent** repo-format flag, rolled back wherever it could be left set without a working shield, but surviving in two cases the reason distinguishes: a sibling worktree still depends on it, or the rollback could not be made at all. Where it cannot be set safely at all (`core.bare = true` or `core.worktree` in the shared config) the shield is skipped instead. It is skipped the same way where the shared config already carries an explicit `extensions.worktreeConfig` that is not `true` (#396): that line is an operator's own declaration, and enabling over it would rewrite it to `true` permanently with nothing recording what stood there — so the shield stands down and the reason quotes the spelling it found, raw rather than `--type=bool`-normalized, since `false`, `off`, `no` and `0` all read back as `false`. A **valueless** `worktreeConfig` line is not that case: git reads it as `true`, so the flag counts as already carried and the shield proceeds. Lines an older bmad-loop wrote into `.git/info/exclude` are **not** removed for you — delete them by hand. A **file** your project tracks gets no pattern: git applies ignore rules only to untracked paths, so the pattern would shield nothing while making the file read as tracked-and-ignored to `git ls-files -ci --exclude-standard` and to repo-hygiene gates built on it (#392). A tracked **directory** gets no directory pattern either — it is replaced by one pattern per file this provisioning run actually wrote below it (#484). A dir pattern does hide new children, and no pattern shape both hides them and keeps the report clean (`dir/*` with per-child negations clears the report and leaks a new file into the commit), but over a tracked tree it bought new-child coverage alone at the price of a false tracked-and-ignored report for every tracked child — modifications to those stage regardless. The accepted trade: a file the **session** creates under such a directory can be staged, which matches your own decision to track that tree, while everything the orchestrator seeded there keeps a pattern of its own. An **untracked** tool directory is unchanged — it keeps the single `/dir` pattern. If git cannot say what a path is — or a file provisioning wrote below a tracked directory has a name no exclude line can spell (an embedded newline, a trailing carriage return), which would shield nothing and loose an orphan pattern on unrelated files — the pattern is kept in its ORIGINAL shape, dir shape included, and the reason is journaled. Patterns are escaped as they are rendered (#476): a rel holding a trailing space or a wildmatch special (`*`, `?`, `[`, `\`) otherwise names something else, leaving the seeded path unshielded while the broken character class silently hides an unrelated file the unit meant to commit. The spellings that made a **trailing period or space** diverge no longer reach this render site at all: a configured component Win32 would trim, or that names a device, is refused at config load (#480), so the shield never has to reconcile an authored spelling against a directory Win32 created under a different name. That is the only direction in which the trailing-space case closes — escaping it to `/skills\ ` fixes the gitignore parse and still names a path `mkdir` never created, because the trim happens below git rather than inside it, so the dot and the space are one defect with one shape rather than two. - Run state never moves into a worktree — `.bmad-loop/` always lives in the main repo; spec paths are persisted relative to the worktree so a kept-failed run stays portable. - The sprint board is **worktree-canonical for the duration of a story** (#350). A board your project _tracks_ needs nothing special: the story's advance is an ordinary modification of a checked-out file and rides the unit commit through the merge. A **gitignored** board is neither checked out nor delivered by one, so it is seeded into the worktree alongside the deferred-work ledger — the orchestrator writes the board through the worktree and `verify_dev` then reads the file it just wrote, which without the seed is a missing file the run dies on rather than a lost write. Its advance is re-applied to the main checkout's board **after** the merge, journaled `board-advance-carried` — or `board-advance-carry-uncommitted` where `git add` refused the ignored path, which is the ordinary outcome for such a board and not a fault (the status on disk is the value; the commit is best-effort). The carry replays from its record if a crash lands between the merge and its latch, and `sprintstatus.advance` never regresses, so a double application is a no-op. On that replay leg the merge — and with it the pre-flight above — has already happened, so the carry proves its own ownership before committing: it recomputes HEAD's content through `advance` and asks **git** whether the board holds that and nothing else, which a crashed pass's half-written advance does and an operator's edit does not. Sameness is git's own — both sides are hashed through the path's clean filter — so a board spelled CRLF by one host and LF by another still matches, where a byte compare would have to guess which spelling is on disk and would refuse a pristine board on every host that chose the other. **Both** places git holds the path are proved, because the carry's `git add` overwrites both: the working tree it copies into the commit, and the index it stages over — an edit staged and then restored in the working tree exists nowhere afterwards, so proving the working tree alone would authorize destroying it. That proof guards the **commit**, which is one write too late for the story's own row: `advance` would already have replaced an operator's status with the target, leaving exactly the bytes the proof accepts — and skipping the commit saves nothing when the value scheduling reads is the one on disk. So that one row is checked **before** the advance as well, and a status that is neither HEAD's nor this pass's own is refused there with nothing written. Both refusals journal `board-advance-carry-foreign-dirt`, and they refuse at different points with different stakes: the pre-advance row check refuses with **nothing written**, so your status is still on the board; the post-advance proof withholds only the **commit** — the advance is on disk by then — and the dirt it declined to take is still escalated by the next run's merge pre-flight. A board that is **gone** by carry time is refused before either question is asked and journaled `board-advance-carry-failed`, the same record a vanished row gets. Every probe fails closed, and git's own dirt answer decides whether either comparison is asked at all — a board nobody wrote is not their question (#618). Both comparisons apply to a board git **tracks**. A gitignored one has no baseline anywhere in git to be compared against, so a replayed carry can still overwrite a row you edited on it while the host was down; the commit half cannot arise there at all, since `git add` refuses an ignored path outright. Scheduling is what depends on it: `_pick_next` reads the **main** board, so an advance that never came back hands finished work to the next run's dev session. - Only the orchestrator writes the board — the session prompts forbid it, and `sprintstatus.advance` is the sole write path. The consequence under isolation, stated rather than left as a surprise: in a _gitignored_ board a session's edit to any OTHER story's row lives only in the worktree copy and evaporates when the worktree is removed. Only the story's own advance is carried back, because only that one is the orchestrator's own write. diff --git a/docs/porting-to-a-new-os.md b/docs/porting-to-a-new-os.md index a1eef483..496e030f 100644 --- a/docs/porting-to-a-new-os.md +++ b/docs/porting-to-a-new-os.md @@ -331,6 +331,55 @@ byte-identical; the new-OS branch can be best-effort until exercised. --- +## Path predicates — the guard family a port must not relax + +`src/bmad_loop/platform_util.py` carries four predicates that every "this config +value must be a path inside the project" guard is built from. They are +**platform-independent by construction**: each answers the same way on every host, +because a config file must not mean different things depending on where the run +happens. + +- `is_absolute_path(value)` — rooted or drive-qualified in _either_ flavour: + `/etc/passwd`, `C:\x`, `\\server\share`, and the drive-_relative_ `C:foo`. +- `has_parent_ref(value)` — a `..` segment in either flavour. +- `names_tree_root(value)` — names the tree itself rather than anything inside it: + `""`, `"."`, and the all-periods-and-spaces spellings Win32 trims to nothing + (`". "`, `"..."`, `" "`). +- `names_win32_alias(value)` — the determinism member. The other three refuse a value + that _escapes_ the tree; this one refuses a value that stays inside it and still + names a **different** path on Windows: a reserved device basename (`NUL`, + `aux.json`, `CON .txt`), or a component whose trailing periods and spaces Win32 + strips (`.claude/skills.`). + +What a porter needs to know: + +- **Do not make any of them consult `sys.platform`.** A value refused here is refused + everywhere, deliberately — `skill_tree = "NUL"` is rejected on Linux for the same + reason `C:\secrets` is. The alternative turns a `seed_files` entry into a + build-number question, since Windows 11 narrowed the device rule and Windows 10 did + not. +- **They refuse disjoint sets, and that is load-bearing.** `names_tree_root` carves + out `..` for `has_parent_ref`; `names_win32_alias` carves out components that are + nothing but periods and spaces for `names_tree_root`. Those carve-outs are what let + each predicate be ablated on its own — a "simplification" that merges them costs the + suite its ability to say which rule fired. +- **`_is_reserved_basename` is a _segment_ predicate**, blind to `sub/NUL`: it splits + on the first dot of the whole string. Apply it per component, after splitting on + both separators, or it silently answers False. +- **`safe_segment` is not one of them.** It maps `/` to `_`, so it sanitizes a single + name (a run id, a sweep bundle name) and must never be applied to a multi-segment + config path. +- **Their Win32 half is cited, not measured** — CI's legs are Linux and nothing here + calls a Win32 API. A native-Windows port is the first chance to measure it; the + docstrings carry their sources (Microsoft, Wine's ntdll path conformance tests, + Project Zero) so a disagreement can be settled rather than argued. +- `src/bmad_loop/data/plugins/unity/unity_seed_assets.py` re-implements all four by + hand: it is deployed into a consumer project, is stdlib-only, and is excluded from + pyright. Its only drift guard is the parity suite in + `tests/test_unity_scene_guard.py`. Mirror any change there too. + +--- + ## Testing a port Both `get_multiplexer()` and `get_process_host()` are `lru_cache`d, and selection diff --git a/docs/testing.md b/docs/testing.md index c0cd29e1..759e85a5 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -72,7 +72,7 @@ Placement rules: ## Fixtures and helpers `tests/conftest.py` holds **fixtures for lifecycle and isolation only — currently exactly -four** — and everything else is a plain function or constant a test imports by name: +five** — and everything else is a plain function or constant a test imports by name: - `project` — the workhorse: a disposable copy of a session-scoped template repo (`_project_template`), BMAD-shaped artifact dirs plus an initial commit. Never hand-roll a @@ -87,6 +87,11 @@ four** — and everything else is a plain function or constant a test imports by just sandbox ones, shadowing the developer's global gitignore sources (`GIT_CONFIG_GLOBAL`, `XDG_CONFIG_HOME`). Without it, a dev box that globally ignores `.claude/` makes shield tests measure the wrong thing while passing. +- `_isolate_state_root` — **autouse** and function-scoped: points `BMAD_LOOP_STATE_DIR` at a + per-test temp dir. `runs.state_root()` does not merely read the user-scoped state location, + it mkdirs into it, so without this every test that builds an adapter would litter the + developer's (or the runner's) real state directory with one tree per run id, on a path no + fixture cleans up. Everything else is a **plain helper — a function or constant** (`from conftest import write_spec, dev_effect, machine_json, ...`). The rule is deliberate: a fixture is ambient — its consumers are invisible diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index 163c3ac1..5b1aae21 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -206,6 +206,21 @@ def _restore_attempt_owned_spec_bytes(spec_path: Path, snapshot: bytes) -> None: if existing_parent == existing_parent.parent: break existing_parent = existing_parent.parent + # `Path.is_absolute()` on purpose, NOT `platform_util.is_absolute_path` + # (#480 item 4). That family predicate is built for "must stay INSIDE + # the project" config guards, where the answer must not vary by host. + # This is the opposite question: a live path this process is about to + # `resolve(strict=True)` and write through on the host it is running on, + # so the platform's own notion of absolute is the operative one. They + # diverge in the direction that matters -- on Windows a POSIX-absolute + # `/spec.md` reads as NOT absolute, so this REFUSES it and fails CLOSED, + # while `is_absolute_path` answers True and would let it through. The + # swap #480 proposes would loosen the only genuine refusal guard it + # named. Measured on POSIX: the `resolve(strict=True)` fixed-point term + # below already refuses every relative spelling on its own (a relative + # path never equals its own resolve), so on this platform the term + # states the intent rather than carrying it alone -- which is exactly + # why it needs saying here. if ( not spec_path.is_absolute() or not existing_parent.is_dir() diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index f2c5342d..ac2e912c 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -3202,6 +3202,32 @@ def spec_within_roots(spec_path: Path, paths: ProjectPaths) -> bool: def resolve_spec_path(spec_file: str, paths: ProjectPaths) -> Path: + """A session-reported ``spec_file`` as a concrete path: an absolute value passes + through untouched, a relative one is probed against ``paths.project`` and falls + back to ``paths.implementation_artifacts``. + + Neither branch promises the result exists — the fallback is returned unprobed + when the project candidate is not a file — so every caller re-tests + ``.is_file()`` itself. Deliberately does NOT ``.resolve()``: callers needing + symlink and ``..`` normalization get it from :func:`spec_within_roots`, which + resolves both sides itself. + + The rule its call sites follow: a caller that goes on to REWRITE the spec must + pair this with :func:`spec_within_roots` first. The value is session-reported + and this function hands back whatever it spells, so the containment check is + what stands between an untrusted string and a write to it. The frontmatter + reconcile, the marker repair and the repair/review spec resets all pair it; so + do the two attempt-binding observations, which write nothing themselves but + establish the binding ``recovery_flow`` later restores bytes through — the + check belongs at the site conferring the authority, not only at the write. + + The rule is about writes to the SPEC, not writes in general, and two callers sit + outside it deliberately: the post-dev board sync and the sweep bundle's ledger + close each read a ``status:`` from an unchecked path and then write to a + deterministic orchestrator-owned target of their own (the sprint board, the + deferred-work ledger). An out-of-tree spec can influence what those write, never + where. A caller that only reads — the ``--json`` read-model, the dev-verify + gates — pairs it with nothing.""" p = Path(spec_file) if p.is_absolute(): return p diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index 5e4dce60..877c3a7a 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -9,7 +9,7 @@ from __future__ import annotations import sys -from pathlib import Path +from pathlib import Path, PureWindowsPath from types import SimpleNamespace import pytest @@ -19,7 +19,7 @@ from bmad_loop.bmadconfig import ProjectPaths from bmad_loop.gates import ATTENTION_FILE from bmad_loop.model import Phase, StoryTask -from bmad_loop.platform_util import UnconfinedWriteError +from bmad_loop.platform_util import UnconfinedWriteError, is_absolute_path from bmad_loop.policy import GatesPolicy, LimitsPolicy, NotifyPolicy, Policy, ScmPolicy from bmad_loop.recovery_flow import PRESERVE_REF_PROBE_LIMIT, RecoveryFlow from bmad_loop.verify import GitError, rev_parse_head @@ -89,6 +89,41 @@ def test_owned_spec_restore_recreates_missing_canonical_parents(tmp_path): assert spec.read_bytes() == snapshot +def test_attempt_owned_spec_refuses_a_posix_absolute_spec_path(tmp_path, monkeypatch): + """#480 item 4: the one genuine REFUSAL guard in the tree built on stdlib + `is_absolute()`, pinned so a later path-guard sweep does not "fix" it. + + It fails CLOSED on Windows, and `platform_util.is_absolute_path` would open + it: the Windows flavour reads a POSIX-absolute spec path as NOT absolute, so + the guard raises, while the family predicate answers True and would let it + through. That divergence is asserted on the pure flavour, so a POSIX host + measures the claim rather than skipping it. + + Ablation: deleting the whole refusal reddens the raise and the canary + together. Deleting the `not spec_path.is_absolute()` term ALONE leaves this + GREEN on POSIX -- measured, not assumed -- because the `resolve(strict=True)` + fixed-point term below subsumes it here: a relative path never equals its own + resolve. That is the finding rather than a hole in the test. The term is + load-bearing on Windows only, so no POSIX test can protect it from deletion + and the comment at the guard is what has to.""" + # The divergence the proposed swap would introduce, on the flavour that + # decides it -- this is the whole of #480 item 4's mechanism, inverted. + assert PureWindowsPath("/attempt/owned.md").is_absolute() is False + assert is_absolute_path("/attempt/owned.md") is True + + monkeypatch.chdir(tmp_path) + snapshot = b"---\nstatus: ready-for-dev\n---\n\noperator input\n" + spec = Path("attempt") / "owned.md" + + with pytest.raises(RuntimeError, match="became unsafe"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) + + # Canary: the guard sits ABOVE the mkdir and the write, so neither ran. A + # refusal raised after the restore would pass the assertion above alone. + assert not spec.exists() + assert not spec.parent.exists() + + def _make_flow( *, workspace, From d475aaa73fd1b297819070060eb329c8aec5520d Mon Sep 17 00:00:00 2001 From: t Date: Tue, 25 Aug 2026 19:02:42 -0700 Subject: [PATCH 08/10] fix(guards): close the round-1 review gaps in the path-guard family and run containment (#480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1 (CodeRabbit + codex), all six findings validated against the code and fixed: - names_win32_alias: an all-period/space component embedded beside a real one (sub/...) slipped all four family members — names_tree_root demands every component be root-naming and the old carve-out excluded such components unconditionally. Rule 2 is now scoped by the whole value (not root_naming), with . and .. carved out for their owners. Unity clone mirrored; parity and behavioral rows extended. - _refuse_uncontained_run_dir: a run_dir spelled runs/.. has .name == '..' and the direct-child rebuild reproduces it verbatim, so rmtree would resolve it to .bmad-loop itself; refused by name. A run-dir level redirected through a symlink (or win32 junction — is_link_like, not is_symlink) kept the lexical spelling while sending the removal outside the project; refused by an lstat-family walk over the orchestrator-owned levels, stopping short of the operator's own project argument. - resolve_run_dir: an empty ref is a prefix and suffix of every id, so partial matching read it as a wildcard and resolved the sole run of a one-run project; refused outright (no directory can be named '', so no addressability is lost). - unity_seed_assets: the env value was .strip()-ed before validation, so an authored trailing space was silently trimmed instead of refused; .strip() now only detects an unset/blank setting and validation sees the raw value. - test_unity_scene_guard: the behavioral wiring test now records its exact ablation mutation, per the testing doctrine. Every new gate ablation-graded singly with disjoint red sets; the redirect tests' measured failure kinds are recorded in their docstrings. --- CHANGELOG.md | 18 ++- docs/porting-to-a-new-os.md | 18 ++- .../data/plugins/unity/unity_seed_assets.py | 39 ++++-- src/bmad_loop/platform_util.py | 43 ++++-- src/bmad_loop/runs.py | 46 ++++++- tests/test_platform_util.py | 47 +++++-- tests/test_runs.py | 129 +++++++++++++++++- tests/test_unity_scene_guard.py | 44 ++++-- 8 files changed, 316 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cb66092..d84cf40d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,9 +50,13 @@ breaking changes may land in a minor release. spells — a device rather than a file, or a sibling once Win32 strips the trailing run — so the file that gets seeded and the exclude pattern rendered from the authored spelling disagree about which path they mean. The refusal is cross-platform on purpose, matching how the family - already rejects `C:\secrets` on POSIX: a config value must not mean one thing per host. No - shipped profile, bundled `plugin.toml` or default trips it, but this is a compatibility break - on previously-loading config. + already rejects `C:\secrets` on POSIX: a config value must not mean one thing per host. A + component of only periods and spaces embedded beside a real one (`sub/...`) is refused by the + same rule — Win32 empties it and the value addresses `sub` — and the Unity seeder validates + the authored env value rather than a `.strip()`-normalized copy, so an authored trailing + space is refused like at every other site instead of silently trimmed. No shipped profile, + bundled `plugin.toml` or default trips it, but this is a compatibility break on + previously-loading config. ### Fixed @@ -92,7 +96,13 @@ breaking changes may land in a minor release. and can only name a child — and the ref is the operator's own argv, so this was a footgun rather than an escalation. `delete_run` and `archive_run` now also refuse a `run_dir` that is not a direct child of the runs directory, raising `UnconfinedWriteError` ahead of the - live-session guard and not waived by `--force`. + live-session guard and not waived by `--force`. That refusal also covers a `run_dir` spelled + `runs/..` — the one shape the direct-child rebuild reproduces verbatim while `rmtree` would + resolve it to `.bmad-loop` itself — and a run-dir level redirected through a symlink or, on + Windows, an unelevated directory junction, which kept the lexical spelling while sending the + removal outside the project. An empty run ref is refused outright as well: it is a prefix and + a suffix of every id, so partial matching read it as a wildcard and resolved the sole run of + a one-run project. - **A sweep bundle name that is not a legal path segment is refused at triage** (#637), at both of `validate_triage`'s bundle-name sites — the `bundles` list and a decision option's `bundle_name`, which becomes `Bundle.name` by way of `_materialize_bundles`. A cycle-1 diff --git a/docs/porting-to-a-new-os.md b/docs/porting-to-a-new-os.md index 496e030f..2a39a0c0 100644 --- a/docs/porting-to-a-new-os.md +++ b/docs/porting-to-a-new-os.md @@ -348,8 +348,10 @@ happens. - `names_win32_alias(value)` — the determinism member. The other three refuse a value that _escapes_ the tree; this one refuses a value that stays inside it and still names a **different** path on Windows: a reserved device basename (`NUL`, - `aux.json`, `CON .txt`), or a component whose trailing periods and spaces Win32 - strips (`.claude/skills.`). + `aux.json`, `CON .txt`), a component whose trailing periods and spaces Win32 + strips (`.claude/skills.`), or a component of _nothing but_ periods and spaces + sitting beside a real one (`sub/...` — Win32 empties it and the value addresses + `sub`). What a porter needs to know: @@ -358,11 +360,13 @@ What a porter needs to know: reason `C:\secrets` is. The alternative turns a `seed_files` entry into a build-number question, since Windows 11 narrowed the device rule and Windows 10 did not. -- **They refuse disjoint sets, and that is load-bearing.** `names_tree_root` carves - out `..` for `has_parent_ref`; `names_win32_alias` carves out components that are - nothing but periods and spaces for `names_tree_root`. Those carve-outs are what let - each predicate be ablated on its own — a "simplification" that merges them costs the - suite its ability to say which rule fired. +- **They refuse disjoint spelling classes, and that is load-bearing.** + `names_tree_root` carves out `..` for `has_parent_ref`; `names_win32_alias` carves + out a _value_ made entirely of period/space components for `names_tree_root` (a + single such component beside a real one stays its own — it aliases its parent, not + the root) and the `.`/`..` components for their owners. Those carve-outs are what + let each predicate be ablated on its own — a "simplification" that merges them + costs the suite its ability to say which rule fired. - **`_is_reserved_basename` is a _segment_ predicate**, blind to `sub/NUL`: it splits on the first dot of the whole string. Apply it per component, after splitting on both separators, or it silently answers False. diff --git a/src/bmad_loop/data/plugins/unity/unity_seed_assets.py b/src/bmad_loop/data/plugins/unity/unity_seed_assets.py index 35a9334c..25e513a6 100644 --- a/src/bmad_loop/data/plugins/unity/unity_seed_assets.py +++ b/src/bmad_loop/data/plugins/unity/unity_seed_assets.py @@ -77,11 +77,12 @@ def _names_tree_root(value: str) -> bool: The third member of the family, and the one this script was missing. Win32 strips every trailing period and space from a path's final component, so ``"..."`` names the worktree root there while both pure flavours read it as an - ordinary one-segment name. That mattered here: the caller `.strip()`s the env - var, which collapses the *space* spellings into ``"."`` and lets the existing - ``not rel.parts`` check catch them, but leaves the *dot* spellings intact — - and the asset-root probe below would then find the worktree itself, so the - payload landed in the worktree root instead of under ``Assets/``.""" + ordinary one-segment name. That mattered here: the asset-root probe below + would find the worktree itself for such a value, so the payload landed in the + worktree root instead of under ``Assets/``. (The caller once ``.strip()``-ed + the env var before validating, which collapsed the *space* spellings into + ``"."``; validation now sees the authored value, so every spelling lands + here.)""" if PurePosixPath(value) == PurePosixPath(".") or PureWindowsPath(value) == PureWindowsPath("."): return True parts = [part for part in value.replace("\\", "/").split("/") if part] @@ -121,14 +122,18 @@ def _names_win32_alias(value: str) -> bool: The fourth member of the family, and the only one about *determinism* rather than containment: ``"Assets/NUL"`` and ``"Assets/BmadLoop/Editor."`` are both inside the worktree by every measure the other three apply, and both name a different thing on - Windows than they spell here. The ``part.strip(" .") != ""`` carve-out hands a - component made solely of periods and spaces back to :func:`_names_tree_root` and - plain ``".."`` back to :func:`_has_parent_ref`, so all four refuse disjoint sets. - Core's docstring carries the two rules, their sources, and the Windows 11 narrowing - this deliberately does not track.""" + Windows than they spell here. The ``not root_naming`` term hands a value made + entirely of period/space components back to :func:`_names_tree_root` — while still + catching one such component embedded beside a real one (``"Assets/..."``), which is + nobody's root and nobody's parent — and the ``part not in (".", "..")`` carve-out + hands plain ``".."`` back to :func:`_has_parent_ref`, so all four refuse disjoint + spelling classes. Core's docstring carries the two rules, their sources, and the + Windows 11 narrowing this deliberately does not track.""" parts = [part for part in value.replace("\\", "/").split("/") if part] + root_naming = _names_tree_root(value) return any( - _is_reserved_basename(part) or (part.strip(" .") != "" and part != part.rstrip(" .")) + _is_reserved_basename(part) + or (part != part.rstrip(" .") and part not in (".", "..") and not root_naming) for part in parts ) @@ -227,7 +232,13 @@ def main() -> int: return 2 payload_version = _read_version(guard_src) - guard_dir = os.environ.get("BMAD_LOOP_UNITY_SCENE_GUARD_DIR", "").strip() or _DEFAULT_GUARD_DIR + # `.strip()` decides only whether the env var is SET — the authored value is + # what gets validated. Stripping before validation silently trimmed the exact + # spelling the guard below promises to refuse ("Assets/BmadLoop/Editor " was + # trimmed and installed into Editor), and made this the one site of seven whose + # config value was normalized before the family saw it. + raw = os.environ.get("BMAD_LOOP_UNITY_SCENE_GUARD_DIR", "") + guard_dir = raw if raw.strip() else _DEFAULT_GUARD_DIR rel = Path(guard_dir) # The install dir must stay inside the worktree AND name something in it: an # absolute/drive-qualified path would make _install's relative_to() raise, a @@ -236,9 +247,7 @@ def main() -> int: # term is about determinism rather than containment: Win32 trims a component's # trailing periods and spaces, so "Assets/BmadLoop/Editor." installs into "Editor" # while the configured string still spells "Editor.", and a component naming a - # reserved device writes to the device instead of the tree. The caller's .strip() - # above removes only a trailing run on the WHOLE value, so neither an interior - # component nor a trailing dot is already handled. + # reserved device writes to the device instead of the tree. if ( not rel.parts or _names_tree_root(guard_dir) diff --git a/src/bmad_loop/platform_util.py b/src/bmad_loop/platform_util.py index da08a714..63aa4634 100644 --- a/src/bmad_loop/platform_util.py +++ b/src/bmad_loop/platform_util.py @@ -233,14 +233,21 @@ def names_win32_alias(value: str | Path) -> bool: reads the authored spelling, so a shield pattern rendered from the config matches ``skills.`` and misses the directory Win32 actually made. - Rule 2 reads the same trim :func:`names_tree_root` does, split by what the trim - leaves behind. That predicate owns a component made *solely* of periods and - spaces, where the trim leaves nothing and the component names the tree root. - This one owns a component where the trim leaves something, where it names a - *sibling* of what was written. The ``part.strip(" .") != ""`` carve-out is what - draws that line, and it hands plain ``".."`` back to :func:`has_parent_ref` at - the same time. All four members therefore refuse disjoint sets — which is what - lets each be ablated on its own, and mirrors :func:`names_tree_root`'s own + Rule 2 reads the same trim :func:`names_tree_root` does, split by what the + whole *value* amounts to rather than by component. That predicate owns a value + made *entirely* of period/space components, where the trim leaves nothing at + any level and the value names the tree root. This one owns everything else the + trim touches: a component the trim shortens (``"skills. "`` names a sibling of + what was written) and equally a component the trim *empties* when it sits + beside a real one — ``"sub/..."`` is nobody's root and nobody's parent, so it + is an alias and belongs here (it addresses ``sub`` on Windows and a literal + ``...`` directory on POSIX; the first review round caught it slipping all four + members). The ``not root_naming`` term draws that line, and the + ``part not in (".", "..")`` carve-out beside it hands the two spellings that + mean the *same* path on every platform back to their owners — ``"."`` is a + no-op component everywhere, ``".."`` is :func:`has_parent_ref`'s climb. All + four members therefore refuse disjoint spelling classes — which is what lets + each be ablated on its own, and mirrors :func:`names_tree_root`'s own ``part != ".."`` carve-out one function up. The git half of rule 2 is measured, on this repo's own suite. **The Win32 @@ -258,12 +265,22 @@ def names_win32_alias(value: str | Path) -> bool: # Same both-separator split as `names_tree_root`, and for the same reason: a # value is judged by the components Win32 would see. parts = [part for part in text.replace("\\", "/").split("/") if part] + # A value made ENTIRELY of period/space components names the tree root and is + # `names_tree_root`'s to refuse; scoping rule 2 by the WHOLE value rather than + # per component is what keeps the two disjoint while still catching an + # all-period/space component embedded beside a real one (`sub/...`), which is + # nobody's root and nobody's parent. + root_naming = names_tree_root(text) return any( - # `part.strip(" .") != ""` hands a component that is nothing but periods - # and spaces back to `names_tree_root` (and plain `..` to `has_parent_ref`), - # so the four predicates refuse disjoint sets and stay separately ablatable - # — the same carve-out, for the same reason, as `names_tree_root`'s `..`. - _is_reserved_basename(part) or (part.strip(" .") != "" and part != part.rstrip(" .")) + _is_reserved_basename(part) + or ( + part != part.rstrip(" .") + # `.` and `..` spell the same path on every platform — the no-op + # component, and the climb `has_parent_ref` owns — the same + # carve-out, for the same reason, as `names_tree_root`'s `..`. + and part not in (".", "..") + and not root_naming + ) for part in parts ) diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 93d7ac41..2f6c3943 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -607,7 +607,17 @@ def resolve_run_dir(project: Path, ref: str) -> Path: ref that could escape the runs dir (`bmad-loop delete ../../x` would otherwise rmtree an outside directory that happens to hold a state.json). Such a ref falls through to partial matching, which can only ever yield a name - `list_run_dirs` enumerated — and so cannot escape.""" + `list_run_dirs` enumerated — and so cannot escape. + + An EMPTY ref is refused outright rather than deferred: `""` is a prefix and a + suffix of every name, so partial matching reads it as a wildcard — harmlessly + ambiguous with two runs, but silently resolving the sole run of a one-run + project, which handed `bmad-loop delete ""` that run. No addressability is + lost (no directory can be named `""`); every other escape spelling keeps the + partial fallback so a legacy dir named `"..."` stays matchable by its own + spelling.""" + if not ref: + raise RunRefError("empty run ref: it would match every run, never name one") if not _is_path_escape(ref): exact = run_dir_for(project, ref) if is_run(exact): @@ -1654,7 +1664,27 @@ def _refuse_uncontained_run_dir(project: Path, run_dir: Path, action: str) -> No runs root itself, a nested grandchild, and anything outside the project all differ from what it returns. Comparing against the rebuild rather than walking `parents` keeps this tracking `RUNS_DIR` the way - :func:`_project_of_run_dir` does. + :func:`_project_of_run_dir` does. The rebuild has one blind spot the name + check closes: ``.name`` of ``runs / ".."`` is ``".."`` and the rebuild + reproduces it verbatim, so the lexical equality holds while `rmtree` would + resolve it to ``.bmad-loop`` itself. pathlib drops ``"."`` at parse so only + the ``".."`` spelling survives to here; ``"."`` is refused anyway rather than + reasoned about. + + The link walk below the equality check refuses a REDIRECTED spelling of a + contained path: with ``.bmad-loop``, ``runs`` or the run dir itself replaced + by a symlink (or, on Windows, an unelevated ``mklink /J`` junction — why this + is :func:`is_link_like` and not ``is_symlink``), the rebuild is lexically + identical while `rmtree` follows the redirect and removes a tree outside the + project. A planted redirect is this module's live threat class (see the #591 + notes in :func:`archive_run`). The walk stops short of ``project`` — the + operator's own argument, and a project addressed through a symlinked home is + legitimate — and covers only the orchestrator-owned levels under it. It is + check-then-act, not fd-anchored like `journal.py`'s writes: `resolve()` is + banned here (it can raise on a WSL-UNC host — `tests/conftest.py`'s + ``refuse_to_resolve``), `tarfile` cannot take a dir fd at all, and the racer + that could re-plant between check and rmtree is a live session, which the + guard below this one refuses anyway. Raises rather than degrading — observation may degrade, a repair write must not: there is no partial `rmtree` to fall back to, and declining quietly would @@ -1662,10 +1692,20 @@ def _refuse_uncontained_run_dir(project: Path, run_dir: Path, action: str) -> No shape-refusal this module already raises for the same class of mistake (see :func:`_project_of_run_dir`), and being an ``OSError`` it lands in the handling callers already have for a removal that failed.""" - if run_dir_for(project, run_dir.name) != run_dir: + if run_dir.name in (".", "..") or run_dir_for(project, run_dir.name) != run_dir: raise UnconfinedWriteError( f"refusing to {action} {run_dir}: not a run directory under {project / RUNS_DIR}" ) + node = run_dir + while node != project: + if is_link_like(node): + raise UnconfinedWriteError( + f"refusing to {action} {run_dir}: {node} is a symlink or junction" + ) + parent = node.parent + if parent == node: # anchored: never walk past the filesystem root + break + node = parent def delete_run(project: Path, run_dir: Path, *, force: bool = False) -> None: diff --git a/tests/test_platform_util.py b/tests/test_platform_util.py index 3b17a61f..510fde9c 100644 --- a/tests/test_platform_util.py +++ b/tests/test_platform_util.py @@ -153,9 +153,30 @@ def test_names_win32_alias_catches_reserved_device_names(value): def test_names_win32_alias_catches_the_trailing_trim(value): # Rule 2, deliberately holding no row rule 1 also catches, so the two rules # redden separately: every component here strips to something non-empty and is - # not a reserved name. Ablation A2 (dropping the `part.strip(" .") != ""` - # carve-out) leaves this test entirely green — it only widens rule 2 onto the - # sibling predicates' territory, which is what the disjointness test below pins. + # not a reserved name. Dropping either of rule 2's carve-outs leaves this test + # entirely green — they only subtract, widening rule 2 onto the sibling + # predicates' territory, which is what the disjointness test below pins. + assert platform_util.names_win32_alias(value) is True + + +@pytest.mark.parametrize( + "value", + ["sub/...", "sub/.. ", "sub/ ", "a/. ", " /a", "sub\\..."], +) +def test_names_win32_alias_catches_an_all_dot_or_space_component_beside_a_real_one(value): + """The round-1 review gap in rule 2's original carve-out: `names_tree_root` + demands EVERY component be root-naming, `has_parent_ref` wants a literal `..`, + and the old `part.strip(" .") != ""` carve-out excluded an all-period/space + component unconditionally — so `sub/...` passed all four family members while + Win32's trim empties the component and the value addresses `sub` (or, for + `.. `, climbs under the other reading of the trim-vs-`..` ordering — divergent + from the literal POSIX directory either way, so the refusal rests on neither + reading). Scoping the carve-out by the WHOLE value (`not root_naming`) is what + closes this without taking the single-component spellings off + `names_tree_root`'s hands. Ablation: restore the old carve-out — replace + `part not in (".", "..") and not root_naming` with `part.strip(" .") != ""` — + and every row here reddens while the three alias tests above and the sibling + delegation test below stay green.""" assert platform_util.names_win32_alias(value) is True @@ -177,16 +198,18 @@ def test_names_win32_alias_accepts_ordinary_paths(value): assert platform_util.names_win32_alias(value) is False -@pytest.mark.parametrize("value", ["..", ".", "", "...", " ", ".. "]) +@pytest.mark.parametrize("value", ["..", ".", "", "...", " ", ".. ", "a/..", "a/./b"]) def test_names_win32_alias_leaves_the_root_and_parent_spellings_to_its_siblings(value): - # The disjointness pin. Every row here is refused by `has_parent_ref` (`..`) or - # by `names_tree_root` (the rest), and this predicate must leave them alone so - # the four family members reject disjoint sets and each stays separately - # ablatable. Ablation A2: drop the `part.strip(" .") != ""` carve-out from - # `names_win32_alias` — a bare `part != part.rstrip(" .")` test — and this test - # reddens alone, on every row but `""` (which has no components at all), while - # the three tests above stay green. It is also what reddens if someone - # "simplifies" the predicate to that bare rstrip. + # The disjointness pin. Every row here is refused by `has_parent_ref` (the + # `..` spellings), refused by `names_tree_root` (the value-wide dot/space + # ones), or contains a no-op `.` component that names the SAME path on every + # platform — and this predicate must leave them all alone so the four family + # members reject disjoint spelling classes and each stays separately + # ablatable. Two arms, disjoint red sets: A2 — drop the `not root_naming` + # term and the value-wide rows (`...`, ` `, `.. `) redden alone; A3 — drop + # the `part not in (".", "..")` carve-out and the `..`/`.`-component rows + # (`..`, `a/..`, `a/./b`) redden alone. `""` and bare `.` can redden under + # neither: one has no components at all, the other is root-naming whole. assert platform_util.names_win32_alias(value) is False diff --git a/tests/test_runs.py b/tests/test_runs.py index a6088cb7..81145624 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -327,8 +327,10 @@ def test_resolve_run_dir_refuses_the_root_naming_refs(tmp_path, ref): The planted state.json is the load-bearing part of the fixture. Without it the exact branch is inert for every row and the test would pass for the wrong - reason. Ablation: drop `names_tree_root` from `_is_path_escape` and the `""` - and `"."` rows redden alone — the other three have no POSIX reach to lose.""" + reason. Ablation: drop `names_tree_root` from `_is_path_escape` and the `"."` + row reddens alone — the other three have no POSIX reach to lose, and `""` is + refused upstream by `resolve_run_dir`'s empty-ref gate (whose own single-run + grading lives in the test below).""" project = tmp_path / "proj" _make_run(project, "20260620-143025-a1b2") _make_run(project, "20260619-101010-a1c9") @@ -340,6 +342,24 @@ def test_resolve_run_dir_refuses_the_root_naming_refs(tmp_path, ref): assert (runs_root / "state.json").is_file() # never consumed as a run +def test_resolve_run_dir_refuses_an_empty_ref_even_with_a_single_run(tmp_path): + """Round-1 review: `""` is a prefix and a suffix of every name, so partial + matching reads it as a wildcard — the two-run fixture above lands it in the + ambiguity arm, but with exactly ONE run it resolved that run, handing + `bmad-loop delete ""` a run the operator never named. The refusal sits above + partial matching so it cannot depend on how many runs exist, and it costs no + addressability: no directory can be named `""`, unlike the other escape + spellings, which keep the partial fallback so a legacy dir named `"..."` + stays matchable. Ablation: drop the `if not ref` gate from `resolve_run_dir` + and this test reddens alone (the ref resolves) while the two-run test above + stays green on its ambiguity arm.""" + project = tmp_path / "proj" + run = _make_run(project, "20260620-143025-a1b2") + with pytest.raises(runs.RunRefError, match="empty run ref"): + runs.resolve_run_dir(project, "") + assert run.is_dir() + + def test_read_pid_missing_and_garbage(tmp_path): run_dir = _make_run(tmp_path, "r1") assert runs.read_pid(run_dir) is None @@ -2479,7 +2499,13 @@ def test_delete_run_proceeds_when_the_multiplexer_cannot_answer(tmp_path, monkey @pytest.mark.parametrize( - "kind", ["outside-the-project", "the-runs-root-itself", "a-nested-grandchild"] + "kind", + [ + "outside-the-project", + "the-runs-root-itself", + "a-nested-grandchild", + "the-dot-dot-alias", + ], ) def test_delete_run_refuses_a_run_dir_outside_the_runs_dir(tmp_path, kind): """#480's containment half, and the reason it is a second guard rather than a @@ -2487,6 +2513,12 @@ def test_delete_run_refuses_a_run_dir_outside_the_runs_dir(tmp_path, kind): so a path composed by any route other than `resolve_run_dir` never meets `_is_path_escape` at all. + The `..` row is the round-1 review catch — the one spelling the rebuild + equality is blind to: `.name` of `runs / ".."` is `".."` and the rebuild + reproduces it verbatim, so the lexical comparison holds while `rmtree` would + resolve it to `.bmad-loop` itself (its canary lives there). Ablation: drop + the `run_dir.name in (".", "..")` clause and that row reddens alone. + The canary — not the raise — is what grades the guard's PLACEMENT: a guard that raised *after* `shutil.rmtree` would satisfy `pytest.raises` and still have destroyed the directory. Ablation: delete the guard and the raise assertion @@ -2498,6 +2530,7 @@ def test_delete_run_refuses_a_run_dir_outside_the_runs_dir(tmp_path, kind): "outside-the-project": tmp_path / "outside", "the-runs-root-itself": runs_root, "a-nested-grandchild": runs_root / "20260620-143025-a1b2" / "nested", + "the-dot-dot-alias": runs_root / "..", }[kind] target.mkdir(parents=True, exist_ok=True) canary = target / "canary.txt" @@ -2514,6 +2547,69 @@ def test_delete_run_refuses_a_run_dir_outside_the_runs_dir(tmp_path, kind): assert canary.is_file() +def _redirected_project(tmp_path, level, run_name): + """A project whose ``level`` — ``.bmad-loop``, its ``runs`` dir, or the run + itself — is a symlink into an external tree holding a real state.json-bearing + run. The lexical rebuild in `_refuse_uncontained_run_dir` is identical for all + three, which is exactly what the link walk exists to see through. Returns + ``(project, run_dir, external_run, canary)`` — the canary lives in the + redirect TARGET, because that is what a guard trusting the lexical spelling + hands `rmtree`.""" + project = tmp_path / "proj" + external = tmp_path / "external" + if level == "the-run-dir": + (project / ".bmad-loop" / "runs").mkdir(parents=True) + ext_run = external / run_name + link, target = project / ".bmad-loop" / "runs" / run_name, ext_run + elif level == "the-runs-dir": + (project / ".bmad-loop").mkdir(parents=True) + ext_run = external / run_name + link, target = project / ".bmad-loop" / "runs", external + else: # the-state-dir + project.mkdir() + ext_run = external / "runs" / run_name + link, target = project / ".bmad-loop", external + ext_run.mkdir(parents=True) + (ext_run / "state.json").write_text("{}") + canary = ext_run / "canary.txt" + canary.write_text("survives") + link.symlink_to(target) + return project, project / ".bmad-loop" / "runs" / run_name, ext_run, canary + + +@pytest.mark.parametrize("level", ["the-state-dir", "the-runs-dir", "the-run-dir"]) +def test_delete_run_refuses_a_redirected_run_dir(tmp_path, level): + """Round-1 review (codex P1): with an orchestrator-owned level replaced by a + symlink, `run_dir_for(project, run_dir.name)` is lexically identical to + `run_dir`, so the rebuild equality holds while `rmtree` follows the redirect + and removes a tree OUTSIDE the project — a planted redirect being this + module's live threat class (see the #591 notes in `archive_run`). The guard + walks `is_link_like` — not `is_symlink`, which reports False for the + unelevated win32 junction — over every level below `project`, and stops + short of `project` itself: a project addressed through a symlinked home is + the operator's own business. + + Ablation (measured): drop the link walk from `_refuse_uncontained_run_dir` + and every arm reddens on the raise expectation — the state-dir and runs-dir + arms as DID NOT RAISE, because the delete *succeeds* and eats the external + run (the canary assertion never even runs; it is what would catch a guard + moved below the rmtree), and the run-dir arm on the raise TYPE, because + `shutil.rmtree` refuses a symlink argument itself but with a plain OSError + where the containment contract promised UnconfinedWriteError.""" + run_name = "20260620-143025-a1b2" + project, run_dir, ext_run, canary = _redirected_project(tmp_path, level, run_name) + + with pytest.raises(platform_util.UnconfinedWriteError, match="symlink or junction"): + runs.delete_run(project, run_dir) + assert canary.is_file() + assert ext_run.is_dir() + + # containment sits above `force`, exactly as in the lexical test above + with pytest.raises(platform_util.UnconfinedWriteError): + runs.delete_run(project, run_dir, force=True) + assert canary.is_file() + + def _escalated_run(tmp_path, spec_text, *, restore_patch_stale=None, git_project=False): """conftest's builder with this module's shape: the spec is written first (so `git_project=True` commits it), and only `(run_dir, spec)` comes back.""" @@ -2962,7 +3058,13 @@ def test_archive_run_refuses_while_the_agent_session_is_live(tmp_path, monkeypat @pytest.mark.parametrize( - "kind", ["outside-the-project", "the-runs-root-itself", "a-nested-grandchild"] + "kind", + [ + "outside-the-project", + "the-runs-root-itself", + "a-nested-grandchild", + "the-dot-dot-alias", + ], ) def test_archive_run_refuses_a_run_dir_outside_the_runs_dir(tmp_path, kind): """Archive carries the same `shutil.rmtree` as delete and needs the same @@ -2970,7 +3072,8 @@ def test_archive_run_refuses_a_run_dir_outside_the_runs_dir(tmp_path, kind): session guard is: a refusal must leave no archive directory behind. Graded like the delete twin — the canary, not the raise, pins the guard above - the `rmtree`.""" + the `rmtree`; the `..` row grades the delete twin's round-1 name clause from + this write path too.""" project = tmp_path / "proj" _make_run(project, "20260611-100000-aaaa") runs_root = project / ".bmad-loop" / "runs" @@ -2978,6 +3081,7 @@ def test_archive_run_refuses_a_run_dir_outside_the_runs_dir(tmp_path, kind): "outside-the-project": tmp_path / "outside", "the-runs-root-itself": runs_root, "a-nested-grandchild": runs_root / "20260611-100000-aaaa" / "nested", + "the-dot-dot-alias": runs_root / "..", }[kind] target.mkdir(parents=True, exist_ok=True) canary = target / "canary.txt" @@ -2989,6 +3093,21 @@ def test_archive_run_refuses_a_run_dir_outside_the_runs_dir(tmp_path, kind): assert not (project / ".bmad-loop" / "archive").exists() # nothing staged +def test_archive_run_refuses_a_redirected_runs_dir(tmp_path): + """The archive twin of `test_delete_run_refuses_a_redirected_run_dir`, on the + representative middle arm: archive would first TAR the redirect target's + content and then `rmtree` it, so a refusal must come before either. Ablation + (measured): drop the link walk and this reddens as DID NOT RAISE — the + external run is consumed into a tarball and removed.""" + run_name = "20260611-100000-aaaa" + project, run_dir, ext_run, canary = _redirected_project(tmp_path, "the-runs-dir", run_name) + + with pytest.raises(platform_util.UnconfinedWriteError, match="symlink or junction"): + runs.archive_run(project, run_dir) + assert canary.is_file() + assert not (project / ".bmad-loop" / "archive").exists() # nothing staged + + def test_archive_run_removes_the_out_of_tree_state_counterpart(tmp_path): """Archive inherits delete's tail — it removes the run dir just the same, so it would leak the same subtree. diff --git a/tests/test_unity_scene_guard.py b/tests/test_unity_scene_guard.py index 7c9ee593..1465eeb6 100644 --- a/tests/test_unity_scene_guard.py +++ b/tests/test_unity_scene_guard.py @@ -207,12 +207,12 @@ def test_seed_rejects_root_naming_guard_dir(tmp_path, monkeypatch): mod = _load_seeder() worktree = tmp_path / "wt" (worktree / "Assets").mkdir(parents=True) - # `main()` `.strip()`s the env var, so the trailing-SPACE spellings collapse to - # "." and the pre-existing `not rel.parts` arm already caught them. These are - # the ones that survive that strip: on Windows each names the worktree itself, - # and the asset-root probe below the guard would then find the worktree (a real - # directory) and pass, scattering the payload across the worktree root. - for evil in ("...", "....", ". .", ".\\"): + # On Windows each of these names the worktree itself, and the asset-root probe + # below the guard would then find the worktree (a real directory) and pass, + # scattering the payload across the worktree root. `main()` once `.strip()`-ed + # the env var, which collapsed the space spellings into "."; validation now + # sees the authored value, so `. ` reaches `_names_tree_root` intact. + for evil in ("...", "....", ". .", ".\\", ". "): _set_env(monkeypatch, worktree, guard_dir=evil) assert mod.main() == 2, evil assert not (worktree / mod._GUARD_CS).exists() # payload never hit the root @@ -233,14 +233,32 @@ def test_seed_rejects_a_win32_alias_guard_dir_on_any_platform(tmp_path, monkeypa names a Windows device, or whose trailing periods/spaces Win32 trims, installs somewhere other than the path it spells. Refused on every platform for the same reason the drive-qualified rows above are — a value must not mean one thing here - and another on Windows. `Editor.` is the shape that matters in practice: nothing - upstream catches it, since the caller `.strip()`s only the whole env var.""" + and another on Windows. The `Editor ` row is the round-1 review catch: `main()` + once `.strip()`-ed the env var before validating, so the authored trailing + space was silently trimmed and installed into `Editor` instead of being + refused; validation now sees the raw value and only uses the strip to detect + an unset/blank setting. `Assets/...` is the same round's embedded + all-dot-component catch, refused by the widened predicate itself. + + Ablation: remove `_names_win32_alias(guard_dir)` from `main()`'s validation + chain and every row here reddens while the clone-parity rows below stay green + — parity grades the MIRROR, this test grades the WIRING, and they must fail + alone. The `Editor ` row also reddens alone if the caller's `.strip()` is + restored into the value that gets validated.""" mod = _load_seeder() (tmp_path / "Assets").mkdir() - for evil in ("Assets/NUL", "Assets/BmadLoop/Editor.", "Assets/BmadLoop /Editor", "NUL"): + for evil in ( + "Assets/NUL", + "Assets/BmadLoop/Editor.", + "Assets/BmadLoop /Editor", + "NUL", + "Assets/BmadLoop/Editor ", + "Assets/...", + ): _set_env(monkeypatch, tmp_path, guard_dir=evil) assert mod.main() == 2, evil assert not (tmp_path / "Assets" / "BmadLoop").exists() # nothing seeded + assert not (tmp_path / "Assets" / "...").exists() # --------------------------------------------- the hand-mirrored win32 predicate @@ -282,6 +300,14 @@ def test_seed_rejects_a_win32_alias_guard_dir_on_any_platform(tmp_path, monkeypa "...", " ", ".. ", + # the round-1 widening: an all-dot/space component beside a real one is an + # alias; a value made entirely of such components stays `_names_tree_root`'s + "sub/...", + "sub/.. ", + "sub/ ", + "a/. ", + "a/..", + "a/./b", ) From c9e3886cbe5061bb897f4c10cb4076e0d2ffc89d Mon Sep 17 00:00:00 2001 From: t Date: Tue, 25 Aug 2026 19:11:40 -0700 Subject: [PATCH 09/10] test(unity): grade the Assets/... canary through the payload file (#480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The directory spelling was itself Win32-divergent: with nothing seeded, (tmp_path / 'Assets' / '...').exists() is True on Windows because the trim resolves '...' to Assets — the Windows CI legs failing on it is a measured live demonstration of the rule under test. The payload-file spelling grades on both platforms. --- tests/test_unity_scene_guard.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/test_unity_scene_guard.py b/tests/test_unity_scene_guard.py index 1465eeb6..6170545b 100644 --- a/tests/test_unity_scene_guard.py +++ b/tests/test_unity_scene_guard.py @@ -258,7 +258,14 @@ def test_seed_rejects_a_win32_alias_guard_dir_on_any_platform(tmp_path, monkeypa _set_env(monkeypatch, tmp_path, guard_dir=evil) assert mod.main() == 2, evil assert not (tmp_path / "Assets" / "BmadLoop").exists() # nothing seeded - assert not (tmp_path / "Assets" / "...").exists() + # The `Assets/...` canary is checked through the payload file, not the + # directory: `(tmp_path / "Assets" / "...").exists()` is True ON WINDOWS with + # nothing seeded at all — the trim resolves `...` to `Assets` itself. That + # spelling failed the Windows CI legs, a measured live demonstration of the + # rule under test. This one grades on both platforms: had seeding happened, + # POSIX holds a literal `.../SceneAutoSaveGuard.cs` and Windows lands the + # payload in `Assets/` — and this path resolves to whichever one exists. + assert not (tmp_path / "Assets" / "..." / mod._GUARD_CS).exists() # --------------------------------------------- the hand-mirrored win32 predicate From 8b4e45dfb3daeccc66dda859a1af87f34f910e73 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 25 Aug 2026 19:18:03 -0700 Subject: [PATCH 10/10] fix(guards): validate the authored [python] module and gate pre-answer bundle names (#480, #637) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2 (codex), both findings validated and fixed: - manifest._parse_python stripped the module value before the alias arm ran, so an authored trailing space (module = 'hooks.py ') was silently trimmed and accepted instead of refused — the one site of seven whose value the family never saw raw. .strip() now decides only whether a module was given; validation and the stored value use the authored spelling. The other six sites were audited and already validate raw values. - A pre-answer's bundle_name never passes validate_triage (a fresh triage can renumber or drop the option it answered), so the persisted-answer fallback in _materialize_bundles was the one route by which a name failing the two option-site gates still reached _write_intent as a directory. The same two rules now gate that lane, by journaled discard (sweep-bundle-name-discarded) rather than by error — the build decision is honored under the always-legal decision- fallback, mirroring the sweep-bundle-name-normalized repair precedent. This closes the residual the PR body had recorded for follow-up; the body is updated accordingly. Both gates ablation-graded singly: restoring the manifest strip reddens the 'hooks.py ' row alone; dropping the discard gate reddens the new pre-answer test alone. --- CHANGELOG.md | 13 ++++--- src/bmad_loop/plugins/manifest.py | 9 +++-- src/bmad_loop/sweep.py | 18 ++++++++++ tests/test_plugin_loader.py | 18 +++++++--- tests/test_sweep.py | 57 +++++++++++++++++++++++++++++++ 5 files changed, 104 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d84cf40d..dc31b485 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -52,9 +52,10 @@ breaking changes may land in a minor release. about which path they mean. The refusal is cross-platform on purpose, matching how the family already rejects `C:\secrets` on POSIX: a config value must not mean one thing per host. A component of only periods and spaces embedded beside a real one (`sub/...`) is refused by the - same rule — Win32 empties it and the value addresses `sub` — and the Unity seeder validates - the authored env value rather than a `.strip()`-normalized copy, so an authored trailing - space is refused like at every other site instead of silently trimmed. No shipped profile, + same rule — Win32 empties it and the value addresses `sub` — and the Unity seeder and a + plugin's `[python] module` validate the authored value rather than a `.strip()`-normalized + copy, so an authored trailing space is refused like at every other site instead of silently + trimmed. No shipped profile, bundled `plugin.toml` or default trips it, but this is a compatibility break on previously-loading config. @@ -111,7 +112,11 @@ breaking changes may land in a minor release. filesystem would create the directory — matched case-insensitively, so lowercase was no reprieve. The test is `safe_segment` identity rather than a second hand-written device list, which keeps the accepted set in lockstep with the sanitizer that defines it: the same idiom, - for the same reason, as `runs.is_valid_run_id`. + for the same reason, as `runs.is_valid_run_id`. The persisted pre-answer lane takes the same + two rules at `_materialize_bundles` — a `bundle_name` answered out of band against an earlier + triage never passes `validate_triage`, and a fresh triage can renumber the option it named — + applied by journaled discard rather than by error: the build decision is honored under the + always-legal `decision-` fallback name. ### Security diff --git a/src/bmad_loop/plugins/manifest.py b/src/bmad_loop/plugins/manifest.py index d331570e..a6272c75 100644 --- a/src/bmad_loop/plugins/manifest.py +++ b/src/bmad_loop/plugins/manifest.py @@ -187,8 +187,13 @@ def _parse_python(python_d: Any, fail) -> PythonSpec | None: return None if not isinstance(python_d, dict): raise fail("[python] must be a table") - module = str(python_d.get("module", "")).strip() - if not module: + # `.strip()` decides only whether a module was given — the authored value is + # what gets validated and stored. Stripping first silently normalized the + # trailing-space spelling the alias arm below promises to refuse + # (`module = "hooks.py "` was trimmed and accepted), making this the one + # site of seven whose value the family never saw raw. + module = str(python_d.get("module", "")) + if not module.strip(): raise fail("[python] requires a 'module'") if names_tree_root(module) or is_absolute_path(module) or has_parent_ref(module): raise fail(f"[python] module must be a plugin-relative path: got {module!r}") diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index a4ac6ec6..b1c59415 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -1359,6 +1359,24 @@ def _materialize_bundles( bundle_name = (option.bundle_name if option else "") or str( answer.get("bundle_name", "") ) + # A pre-answer's bundle_name never passed `validate_triage` — it was + # answered out of band against an earlier triage, and a fresh one can + # renumber or drop the option it named — so this fallback lane was the + # one route by which a name failing the two option-site gates (#637) + # still reached `_write_intent` as a directory. Gate it with the same + # two rules, but by DISCARD rather than by error: the human's build + # decision is the payload and `decision-` below is the always-legal + # name it falls back to anyway, so the discard is journaled the way + # `_normalize_bundle_names`'s repairs are and the sweep proceeds. + if bundle_name and ( + not BUNDLE_NAME_RE.match(bundle_name) or safe_segment(bundle_name) != bundle_name + ): + self.journal.append( + "sweep-bundle-name-discarded", + decision=decision.id, + original=bundle_name, + ) + bundle_name = "" key = (option.key if option else "") or str(answer.get("key", "")) or "?" name = bundle_name or "decision-" + decision.id.lower() bundles.append( diff --git a/tests/test_plugin_loader.py b/tests/test_plugin_loader.py index acda51e8..4093c420 100644 --- a/tests/test_plugin_loader.py +++ b/tests/test_plugin_loader.py @@ -186,13 +186,15 @@ def test_full_manifest_parses(tmp_path): ('[plugin]\nname = "e"\napi_version = 1\nseed_globs = ["."]\n', "seed_globs"), # absolute python module path ('[plugin]\nname = "e"\napi_version = 1\n[python]\nmodule = "/x.py"\n', "plugin-relative"), - # root-naming python module path. The value is `.strip()`ed before the guard, - # so the trailing-space spellings arrive as "." — but the trailing-DOT ones - # arrive intact, and Win32 trims those to the plugin dir just the same. What - # gets exec'd matters more than what gets copied, hence the whole family. + # root-naming python module path. The guard sees the AUTHORED value (the + # `.strip()` decides only whether a module was given), so the space + # spellings arrive intact alongside the dot ones — Win32 trims each to + # the plugin dir just the same. What gets exec'd matters more than what + # gets copied, hence the whole family. ('[plugin]\nname = "e"\napi_version = 1\n[python]\nmodule = "."\n', "plugin-relative"), ('[plugin]\nname = "e"\napi_version = 1\n[python]\nmodule = "..."\n', "plugin-relative"), ('[plugin]\nname = "e"\napi_version = 1\n[python]\nmodule = ". ."\n', "plugin-relative"), + ('[plugin]\nname = "e"\napi_version = 1\n[python]\nmodule = ". "\n', "plugin-relative"), # hook with no cmd ( '[plugin]\nname = "e"\napi_version = 1\n[hooks.pre_run]\nblocking = true\n', @@ -270,7 +272,13 @@ def test_manifest_rejects_win32_alias_seed_paths(key, value): "NUL", "hooks.", # Win32 trims to `hooks`, so the import resolves past the file named "sub/CON.py", - "pkg /hooks.py", # an interior component, which the caller's .strip() cannot reach + "pkg /hooks.py", # an interior component, out of any whole-string strip's reach + # the round-2 review catch: `_parse_python` once `.strip()`-ed the value + # BEFORE this guard ran, so the authored trailing space was silently + # trimmed and accepted instead of refused — the one site of seven whose + # value the family never saw raw. Ablation: restore that + # strip-before-validate composition and this row reddens alone. + "hooks.py ", ], ) def test_manifest_rejects_win32_alias_python_module(value): diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 5460b4e4..09b9976a 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -2806,6 +2806,63 @@ def test_preanswered_build_materializes_bundle_unattended(project): assert '"decision-preanswers-pruned"' in journal +def test_preanswered_bundle_name_failing_the_segment_gate_is_discarded(project): + """Round-2 review: a pre-answer's `bundle_name` never passes `validate_triage` + — it was answered out of band against an earlier triage, and a fresh one can + renumber or drop the option it named — so it was the one route by which a name + failing the two option-site gates (#637) still reached `_write_intent` as a + directory (`nul` passes BUNDLE_NAME_RE and fails `safe_segment` identity). + `_materialize_bundles` now applies the same two rules to that lane, by + journaled DISCARD rather than by error: the build decision is the payload and + the always-legal `decision-` fallback is what an unnamed answer gets + anyway. Ablation: drop that gate and this reddens — the bundle materializes + as `nul`, so the `decision-dw-1` effects below never match and the discard + event never appears.""" + from bmad_loop import decisions + from bmad_loop.sweep import DecisionOption + + write_ledger(project, {"DW-1": "open"}) + # stored key "9" is NOT one of this triage's option keys, so every field — + # bundle_name included — comes from the stored answer, not a validated option + decisions.record_pre_answer( + project.project, + "DW-1", + DecisionOption( + key="9", label="Widen", effect="build", intent="widen the field", bundle_name="nul" + ), + date="2026-06-12", + ) + plan = triage_result( + ["DW-1"], + decisions=[ + _decision( + "DW-1", + [ + {"key": "1", "label": "Widen", "effect": "build", "intent": "fresh intent"}, + {"key": "2", "label": "Keep", "effect": "keep-open"}, + ], + ) + ], + ) + engine, _ = make_sweep( + project, + [ + triage_effect(plan), + bundle_dev_effect(project, "decision-dw-1", ["DW-1"]), + bundle_review_effect(project, "decision-dw-1"), + ], + prompting=False, + ) + summary = engine.run() + assert not summary.paused + + journal = journal_text(engine) + assert '"sweep-bundle-name-discarded"' in journal + assert '"nul"' in journal # the discard names the spelling it dropped + assert engine.state.tasks["dw-decision-dw-1"].phase == Phase.DONE + assert "dw-nul" not in engine.state.tasks # the raw name minted nothing + + def test_preanswered_keep_open_suppresses_prompt_and_persists(project): """A keep-open pre-answer is adopted (no skip, no re-prompt) and, since the entry stays open, the store keeps it for the next sweep too."""