fix(config): refuse Windows device names and trim aliases across the path-guard family (#480, #637)

CHANGELOG.md

@@ -41,6 +41,23 @@ 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. 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 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.
 
 ### Fixed
 
@@ -72,6 +89,34 @@ 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`. 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
+  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`. 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-<id>` fallback name.
 
 ### Security
 

docs/porting-to-a-new-os.md

@@ -331,6 +331,59 @@ 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`), 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:
+
+- **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 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.
+- **`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

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

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,13 +349,23 @@ 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
     # loop's containment checks, so the whole repo is copied in.
     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:

src/bmad_loop/data/plugins/unity/unity_seed_assets.py

@@ -77,17 +77,67 @@ 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]
     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 ``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 != part.rstrip(" .") and part not in (".", "..") and not root_naming)
+        for part in parts
+    )
+
+
 def _truthy(value: str | None, default: bool) -> bool:
     if value is None or value.strip() == "":
         return default
@@ -182,19 +232,35 @@ 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
     # ".." 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.
     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