fix(runs): validate run ids and stop a run ref escaping the runs dir

CHANGELOG.md

@@ -46,6 +46,17 @@ breaking changes may land in a minor release.
 
 ### Fixed
 
+- **Run ids are validated, so a run ref can no longer escape the runs directory.** A positional ref
+  (`delete`, `stop`, `archive`, `resume`, `status`) was recomposed into a path raw, so
+  `bmad-loop delete ../../x` deleted any outside directory holding a `state.json`; the hidden
+  `--run-id` flag on `run`/`sweep` reached a directory name, a multiplexer session name and a git ref
+  unchecked. A supplied id must now match `[A-Za-z0-9][A-Za-z0-9_-]*` (≤ 120 chars, no reserved
+  Windows device name) — rejected, never sanitized, so ids stay bijective with paths and sessions —
+  and a ref that is absolute, climbs with `..`, or carries a separator skips the exact-match branch,
+  falling through to partial matching over enumerated run dirs only. Ids recovered from the outside
+  world — a `bmad-loop-<id>` session name, a `<kind>-<id>` control-session window name — pass the
+  same validator before they steer a path. Partial refs unaffected (#104).
+
 - **An abandoned patch-restore no longer smuggles its files into the corrected story's commit.**
   Re-arming a story whose previous re-drive had already applied a restore patch snapshotted that
   patch's new (untracked) files as _pre-existing_, so every later rollback preserved them and

src/bmad_loop/cli.py

@@ -32,6 +32,7 @@
 from .engine import Engine
 from .journal import Journal, load_state, save_state
 from .model import RunState
+from .platform_util import MAX_SEGMENT
 from .process_host import ProcessHostError
 from .runs import RUNS_DIR
 from .stories_engine import StoriesEngine
@@ -48,6 +49,21 @@ def _policy_path(project: Path) -> Path:
     return project / POLICY_FILE
 
 
+def _reject_bad_run_id(run_id: str | None) -> int | None:
+    """Guard the hidden ``--run-id`` flag before the id becomes a directory name, a
+    multiplexer session name and a git ref component. Rejects rather than sanitizes
+    (see ``runs.is_valid_run_id``): a coerced id would no longer name the run the
+    caller asked for. Returns 1 to abort, None to proceed."""
+    if run_id is not None and not runs.is_valid_run_id(run_id):
+        print(
+            f"error: invalid --run-id {run_id!r} — expected {runs.RUN_ID_RE.pattern} "
+            f"(at most {MAX_SEGMENT} characters, not a reserved device name)",
+            file=sys.stderr,
+        )
+        return 1
+    return None
+
+
 def _reconcile_stale(project: Path, paths: bmadconfig.ProjectPaths, pol) -> None:
     """Tear down worktrees leaked by a prior run that stopped mid-flight, before
     starting a new run/sweep — the clean-finish GC never reached them. Gated on
@@ -360,6 +376,8 @@ def _validate_stories_queue(
 
 
 def cmd_run(args: argparse.Namespace) -> int:
+    if (rc := _reject_bad_run_id(args.run_id)) is not None:
+        return rc
     project = _project(args)
     paths = bmadconfig.load_paths(project)
     pol = policy_mod.load(_policy_path(project))
@@ -646,6 +664,8 @@ def factory(trigger: str) -> None:
 
 
 def cmd_sweep(args: argparse.Namespace) -> int:
+    if (rc := _reject_bad_run_id(args.run_id)) is not None:
+        return rc
     project = _project(args)
     paths = bmadconfig.load_paths(project)
     pol = policy_mod.load(_policy_path(project))

src/bmad_loop/platform_util.py

@@ -51,7 +51,7 @@
     | {f"LPT{s}" for s in "¹²³"}
 )
 _ILLEGAL_SEGMENT_CHARS = re.compile(r'[<>:"/\\|?*\x00-\x1f]')
-_MAX_SEGMENT = 120  # keep segment (incl. any collision suffix) well under the 255 limit
+MAX_SEGMENT = 120  # keep segment (incl. any collision suffix) well under the 255 limit
 
 # git-check-ref-format(1) rejects these anywhere in a ref component: ASCII control
 # chars and space (\x00-\x20), DEL, and `~ ^ : ? * [ \`. `/` is added because it
@@ -183,15 +183,15 @@ def safe_segment(name: str) -> str:
     clean name that happens to look like a sanitized-plus-digest name passes
     through verbatim, and case-insensitive NTFS collisions between clean names
     remain the caller's concern. Never raises."""
-    cleaned = _ILLEGAL_SEGMENT_CHARS.sub("_", name).rstrip(". ")[:_MAX_SEGMENT]
+    cleaned = _ILLEGAL_SEGMENT_CHARS.sub("_", name).rstrip(". ")[:MAX_SEGMENT]
     if _is_reserved_basename(cleaned):
         cleaned = "_" + cleaned
     if not cleaned:
         cleaned = "_"
     if cleaned == name:
         return name  # already a legal segment — keep it byte-identical
     suffix = _digest_suffix(name)
-    return cleaned[: _MAX_SEGMENT - len(suffix)] + suffix
+    return cleaned[: MAX_SEGMENT - len(suffix)] + suffix
 
 
 def _is_clean_ref_segment(seg: str) -> bool:
@@ -202,7 +202,7 @@ def _is_clean_ref_segment(seg: str) -> bool:
     directory built from the same key."""
     return (
         bool(seg)
-        and len(seg) <= _MAX_SEGMENT
+        and len(seg) <= MAX_SEGMENT
         and not _ILLEGAL_REF_CHARS.search(seg)
         and ".." not in seg
         and "@{" not in seg
@@ -241,4 +241,4 @@ def safe_ref_segment(name: str) -> str:
     if not cleaned or cleaned == "@":
         cleaned = "_"
     suffix = _digest_suffix(name)
-    return cleaned[: _MAX_SEGMENT - len(suffix)] + suffix
+    return cleaned[: MAX_SEGMENT - len(suffix)] + suffix

src/bmad_loop/runs.py

@@ -4,6 +4,7 @@
 
 import math
 import os
+import re
 import secrets
 import shutil
 import tarfile
@@ -14,7 +15,13 @@
 from .adapters.multiplexer import get_multiplexer
 from .journal import STATE_FILE, Journal, load_state, save_state
 from .model import PAUSE_ESCALATION, Phase, RunState, StoryTask
-from .platform_util import atomic_replace
+from .platform_util import (
+    MAX_SEGMENT,
+    atomic_replace,
+    has_parent_ref,
+    is_absolute_path,
+    safe_segment,
+)
 from .process_host import get_process_host
 
 RUNS_DIR = Path(".bmad-loop") / "runs"
@@ -39,6 +46,36 @@ def new_run_id() -> str:
     return time.strftime("%Y%m%d-%H%M%S") + "-" + secrets.token_hex(2)
 
 
+# A run id is a lookup key with exactly one legitimate producer (new_run_id), and it
+# lands in three positions at once: a directory name under RUNS_DIR, a multiplexer
+# session name (bmad-loop-<id>), and a git ref component (bmad-loop/<id>/<unit>).
+# So an id supplied from outside is *rejected*, never sanitized — coercing it would
+# break the id<->path<->session bijection the CLI relies on to find a run again.
+#
+# The charset is a superset of every new_run_id() output and excludes, by
+# construction: path separators and `..` (traversal), `<>:"|?*` plus trailing dots
+# and spaces (Windows), `.` and `:` (multiplexer session-name mangling), and all
+# whitespace/control characters. It is also identity under safe_ref_segment, so the
+# unit branch a run produces reads back verbatim — hence no ref check below.
+RUN_ID_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9_-]*")
+
+
+def is_valid_run_id(value: str) -> bool:
+    """True when ``value`` is a run id we would have produced ourselves — the guard
+    every externally-supplied ``--run-id`` and every id recomposed from the outside
+    world (a foreign multiplexer session name) must pass before it touches a path.
+
+    The length cap is ``platform_util.MAX_SEGMENT``: a run id is a directory name.
+    The ``safe_segment`` identity check adds the one rule ``RUN_ID_RE`` cannot
+    express — the reserved Windows device basenames (``CON``, ``NUL``, ``COM1``…),
+    which are legal-looking ids that no filesystem will accept as a directory."""
+    return (
+        bool(RUN_ID_RE.fullmatch(value))
+        and len(value) <= MAX_SEGMENT
+        and safe_segment(value) == value
+    )
+
+
 def list_run_dirs(project: Path) -> list[Path]:
     """All run dirs containing a state.json, oldest first (run ids sort
     chronologically)."""
@@ -100,14 +137,30 @@ def short_ref(run_id: str) -> str:
     return run_id.rsplit("-", 1)[-1]
 
 
+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
+
+
 def resolve_run_dir(project: Path, ref: str) -> Path:
     """Full or partial run id -> its run dir. An exact id wins outright;
     otherwise a partial matches when the trailing segment starts with `ref` or
     the full id ends with `ref` (run ids are date-prefixed, so the tail is what
-    distinguishes them). Raises RunRefError on no match / ambiguity."""
-    exact = run_dir_for(project, ref)
-    if is_run(exact):
-        return exact
+    distinguishes them). Raises RunRefError on no match / ambiguity.
+
+    The exact branch recomposes a path from the raw ref, so it is skipped for any
+    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."""
+    if not _is_path_escape(ref):
+        exact = run_dir_for(project, ref)
+        if is_run(exact):
+            return exact
     matches = [
         d
         for d in list_run_dirs(project)
@@ -253,6 +306,8 @@ def prunable_sessions(project: Path) -> tuple[list[str], list[str], set[str]]:
         if name == CTL_SESSION or not name.startswith(_SESSION_PREFIX):
             continue
         run_id = name[len(_SESSION_PREFIX) :]
+        if not is_valid_run_id(run_id):
+            continue  # a foreign/mangled session name must not steer a run-dir path
         run_dir = run_dir_for(project, run_id)
         tag = tags.get(name, "")
         if tag:

src/bmad_loop/tui/launch.py

@@ -203,6 +203,8 @@ def _ctl_window_candidates(project: Path) -> list[tuple[str, str]]:
         m = _CTL_WINDOW_RE.match(name)
         if m is None:
             continue  # not a run window (e.g. the session's initial shell)
+        if not runs.is_valid_run_id(m.group(1)):
+            continue  # a foreign/mangled window name must not steer a run-dir path
         run_dir = runs.run_dir_for(project, m.group(1))
         if tag:
             if tag != mine:

tests/test_cli.py

@@ -603,6 +603,45 @@ def test_run_honors_preassigned_run_id_and_writes_pid(project, monkeypatch):
     assert (run_dir / "engine.pid").read_text().split()[0] == str(os.getpid())
 
 
+_BAD_RUN_IDS = [
+    "../../escape",  # traversal out of the runs dir
+    "..",
+    "/etc/passwd",  # posix absolute
+    "C:\\windows",  # windows drive-absolute
+    "a/b",  # path separators
+    "a\\b",
+    "a.b",  # dot/colon mangle a multiplexer session name
+    "a:b",
+    "-lead",  # leading dash
+    "a b",  # whitespace
+    "",  # empty
+    "CON",  # reserved windows device basename
+]
+
+
+@pytest.mark.parametrize("bad", _BAD_RUN_IDS)
+@pytest.mark.parametrize("command", ["run", "sweep"])
+def test_start_rejects_invalid_run_id(project, monkeypatch, capsys, command, bad):
+    """The hidden --run-id flag is a lookup key that becomes a directory name, a
+    multiplexer session name and a git ref. Reject at the boundary — before any
+    directory is created, and before the preflight side effects run."""
+    install_bmad_config(project)
+    monkeypatch.setattr(cli, "Engine", _StubEngine)
+    monkeypatch.setattr(cli, "SweepEngine", _StubEngine)
+    monkeypatch.setattr(cli, "_make_adapters", lambda *a, **k: {r: None for r in cli.ROLES})
+
+    # `--run-id=<bad>` (not a separate argv token) so argparse doesn't first reject
+    # a leading-dash value as an unknown option — the guard is what must reject it.
+    assert cli.main([command, "--project", str(project.project), f"--run-id={bad}"]) == 1
+    err = capsys.readouterr().err
+    # the stderr message is the real pin: an unguarded run/sweep would also return 1
+    # here (dirty tree / missing base skills), just for the wrong reason.
+    assert "invalid --run-id" in err and repr(bad) in err
+    # rejected before the id reached a path: no run dir, inside or outside the runs dir
+    assert not (project.project / ".bmad-loop" / "runs").exists()
+    assert not (project.project / "escape").exists()
+
+
 def test_run_aborts_when_base_skills_missing(project, monkeypatch, capsys):
     """The orchestrator depends on the non-bundled upstream skills (bmad-dev-auto
     + the review hunters); a run must fail loudly at preflight (not stall mid-run)

tests/test_engine.py

@@ -502,7 +502,7 @@ def test_run_session_labeled_task_id_capped_as_a_whole(project):
     engine._run_session(task, role="dev", prompt="p", seq=1, label="l" * 110)
 
     (record,) = task.sessions
-    assert len(record.task_id) <= platform_util._MAX_SEGMENT
+    assert len(record.task_id) <= platform_util.MAX_SEGMENT
 
 
 @pytest.mark.parametrize("key", ["6-4:cli?list", "k" * 130])

tests/test_platform_util.py

@@ -189,7 +189,7 @@ def test_retrying_unlink_propagates_missing_file(tmp_path):
 def _is_legal_segment(seg: str) -> bool:
     return (
         bool(seg)
-        and len(seg) <= platform_util._MAX_SEGMENT
+        and len(seg) <= platform_util.MAX_SEGMENT
         and not platform_util._ILLEGAL_SEGMENT_CHARS.search(seg)
         and not seg.endswith((" ", "."))
         and not platform_util._is_reserved_basename(seg)
@@ -239,7 +239,7 @@ def test_safe_segment_distinct_dirty_keys_never_collide():
 
 def test_safe_segment_caps_length():
     out = platform_util.safe_segment("x" * 500)
-    assert len(out) <= platform_util._MAX_SEGMENT
+    assert len(out) <= platform_util.MAX_SEGMENT
     assert _is_legal_segment(out)
 
 
@@ -356,7 +356,7 @@ def test_safe_ref_segment_distinct_dirty_keys_never_collide():
 
 
 def test_safe_ref_segment_caps_length():
-    assert len(platform_util.safe_ref_segment("x" * 500)) <= platform_util._MAX_SEGMENT
+    assert len(platform_util.safe_ref_segment("x" * 500)) <= platform_util.MAX_SEGMENT
 
 
 @pytest.mark.parametrize("value", _REF_CORPUS)