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

[pbean]

Closes #104

The bug

--run-id was the reported hole, but exploring it widened the finding — the positional run refs are worse:

$ bmad-loop delete --project ./proj ../../victim
run victim deleted          # ← rmtree'd a directory outside the runs dir

resolve_run_dir's exact branch recomposes project / RUNS_DIR / ref from the raw string, so any outside directory that happens to hold a state.json is a valid target for delete / stop / archive / resume / status. Verified against main before and after the fix.

Meanwhile the hidden --run-id flag on run/sweep reached three sinks unchecked — a directory name, a multiplexer session name (bmad-loop-<id>), and a git ref component (bmad-loop/<id>/<unit>). Both commands build the run dir inline (project / RUNS_DIR / run_id), not via run_dir_for, so a guard inside run_dir_for alone would have missed the primary sinks.

The shape

A run id has exactly one legitimate producer — runs.new_run_id(), format \d{8}-\d{6}-[0-9a-f]{4}. So reject, don't sanitize: coercing an id would leave the caller unable to name the run they just started, breaking the id↔path↔session bijection the CLI depends on.

• runs.RUN_ID_RE + is_valid_run_id() — [A-Za-z0-9][A-Za-z0-9_-]*, capped at MAX_SEGMENT. Identity for every new_run_id() output. The charset excludes by construction: path separators and .., the Windows illegal set <>:"|?* plus trailing dots/spaces, . and : (which mangle a tmux session name), and all whitespace/control chars. A safe_segment identity check adds the one rule the regex can't express — reserved Windows device basenames (CON, NUL, COM1…).
• cmd_run / cmd_sweep reject an invalid supplied --run-id at the boundary — before the preflight side effects (_reconcile_stale tears down worktrees) and before the id touches a path.
• resolve_run_dir skips the exact branch when the ref is absolute, has a .., or carries a separator, falling through to partial matching — which can only ever yield a name list_run_dirs enumerated, so it cannot escape. Gated on the escape shape, not on is_valid_run_id, so a run dir created by an older version (or by hand) stays addressable.
• prunable_sessions skips session names whose stripped id is invalid — a session name is untrusted input, and bmad-loop-../../x would otherwise steer run_dir_for (and prune_sessions' kill) at a path outside the runs dir.
• _ctl_window_candidates (added on final review) applies the same gate to the one other outside-world reverse mapping — a <kind>-<id> control-session window name, matched by a fully permissive (.+). Unguarded, run-../../x steered the liveness read — and an untagged window's run-dir ownership fallback — at a path outside the runs dir. Blast radius is reads only (no path write, no arbitrary kill), guarded for class-consistency with prunable_sessions; legit windows are unaffected since the TUI is the only creator and names them from enumerated run ids.

platform_util._MAX_SEGMENT → public MAX_SEGMENT: a run id is a filesystem segment, and the two caps must not drift. Mechanical rename; tests already reached for the private name.

Compatibility

• The TUI is the only --run-id caller and its only producer is runs.new_run_id() — never rejected.
• Partial refs (aaaa, aa, 025-a1b2) and exact-id lookups are unaffected; existing pins test_resolve_run_dir_exact_and_partial, test_resolve_run_dir_exact_wins_over_ambiguity and test_run_honors_preassigned_run_id_and_writes_pid stay green untouched.
• Run ids already on disk keep resolving — the exact branch is gated on the escape shape, not on the new charset.

Tests

• test_is_valid_run_id_is_identity_for_generated_ids — pins that a generated id survives both sanitizers byte-for-byte (safe_segment for the directory, safe_ref_segment for the branch), which is why is_valid_run_id needs no ref check of its own.
• test_is_valid_run_id_accepts / _rejects — 33-case negative matrix (traversal, both separator flavours, drive-absolute and drive-relative C:rel, tmux metachars, git-ref metachars, control chars, trailing dot/space, reserved basenames, over-cap).
• test_start_rejects_invalid_run_id — parametrized over run × sweep × 12 payloads. Asserts the stderr message, not just rc == 1: an unguarded run would return 1 here anyway (dirty tree / missing base skills), so rc alone would false-pass.
• test_resolve_run_dir_never_escapes_the_runs_dir — plants a state.json exactly where the un-gated exact branch would land. Stated as containment rather than no-match because a\b is one legal directory name on POSIX (inside the runs dir, so it legitimately resolves) and a nested path on Windows (where it must not).
• test_resolve_run_dir_absolute_ref_never_escapes — /-join discards the project prefix entirely, so an absolute ref escapes without needing a ...
• test_prunable_sessions_skips_invalid_run_ids.
• test_prune_ctl_windows_skips_invalid_run_ids — three hostile window rows: tagged traversal, tagged bad-charset, and an untagged traversal pointed at a planted outside state.json (which would otherwise claim the window as ours). All three legs verified to fail against the unguarded function.

Each new guard test was checked to fail against the unpatched functions (the sole exception is the a\b leg, a Windows-only escape — it passes on POSIX by construction, and Windows CI covers it).

Full suite: 2021 passed, 1 skipped. Full trunk check: clean.

Summary by CodeRabbit

• Bug Fixes
  • Run IDs used for CLI and TUI run/sweep operations are now strictly validated and rejected early with a clear error.
  • Invalid identifiers (empty, malformed, overlong, reserved device forms, or containing path/traversal/separators) no longer drive directory/session/ref resolution.
  • Run directory resolution is hardened to prevent escaping the designated runs area; partial-ref behavior is preserved for enumerated runs.
  • Session/window pruning now skips candidates with invalid run-id tails.
• Documentation
  • Updated the changelog with the stricter run ID validation and path-safety behavior.

[coderabbitai]

Walkthrough

Run IDs now follow a constrained format and length limit. CLI run and sweep reject invalid IDs before setup, while directory resolution and session pruning ignore unsafe references. Tests cover validation, traversal, absolute paths, and side-effect prevention.

Changes

Run ID safety

Layer / File(s)	Summary
Run ID contract and segment limit src/bmad_loop/platform_util.py, src/bmad_loop/runs.py, tests/test_engine.py, tests/test_platform_util.py, tests/test_runs.py	Defines RUN_ID_RE, exposes MAX_SEGMENT, validates generated IDs, and updates segment-limit coverage.
Unsafe reference resolution and pruning src/bmad_loop/runs.py, src/bmad_loop/tui/launch.py, tests/test_runs.py, tests/test_tui_launch.py	Bypasses exact recomposition for unsafe references and skips invalid session- or window-derived run IDs during pruning.
CLI rejection and validation coverage src/bmad_loop/cli.py, tests/test_cli.py, CHANGELOG.md	Rejects invalid --run-id values in run and sweep before setup and documents the behavior with CLI coverage.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant runs.is_valid_run_id
  participant RunSetup
  participant stderr
  CLI->>runs.is_valid_run_id: validate --run-id
  runs.is_valid_run_id-->>CLI: valid or invalid
  alt invalid
    CLI->>stderr: print error
    CLI-->>CLI: return 1
  else valid
    CLI->>RunSetup: continue run or sweep initialization
  end

Loading

Poem

I’m a rabbit guarding paths tonight,
No sneaky dots can hop from sight.
Clean IDs bound through every run,
Safe little burrows, one by one.
The CLI thumps: “Invalid? You’re done!”

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The PR implements #104 by rejecting invalid --run-id values, blocking path/session escapes, and preserving valid/generated IDs.
Out of Scope Changes check	✅ Passed	The changes stay focused on run-id validation, path containment, and related tests/documentation; no unrelated work stands out.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	Clearly summarizes the main change: run IDs are validated and path-escaping run refs are blocked.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/validate-run-id-104

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}