diff --git a/src/bmad_loop/data/plugins/unity/unity_setup.py b/src/bmad_loop/data/plugins/unity/unity_setup.py index d69a5c4..0677a5e 100644 --- a/src/bmad_loop/data/plugins/unity/unity_setup.py +++ b/src/bmad_loop/data/plugins/unity/unity_setup.py @@ -304,7 +304,7 @@ def _local_url(worktree: Path) -> str | None: return override.strip() cfg = worktree / ".mcp.json" try: - data = json.loads(cfg.read_text()) + data = json.loads(cfg.read_text(encoding="utf-8-sig")) servers = data.get("mcpServers", {}) entry = servers.get(_MCP_SERVER_NAME) or next(iter(servers.values()), {}) url = entry.get("url") @@ -425,10 +425,10 @@ def _verify_agent_isolation(agent: str, worktree: Path, url: str) -> bool: port = url.rsplit(":", 1)[-1].strip("/") cfg = worktree / rel try: - text = cfg.read_text() - except OSError: + text = cfg.read_text(encoding="utf-8-sig") + except (OSError, UnicodeDecodeError): print( - f"unity_setup: agent {agent!r} MCP config {rel} missing after setup; " + f"unity_setup: agent {agent!r} MCP config {rel} missing or unreadable after setup; " "cannot guarantee per-worktree isolation", file=sys.stderr, ) diff --git a/src/bmad_loop/data/plugins/unity/unity_teardown.py b/src/bmad_loop/data/plugins/unity/unity_teardown.py index 0f06393..fa45ba7 100644 --- a/src/bmad_loop/data/plugins/unity/unity_teardown.py +++ b/src/bmad_loop/data/plugins/unity/unity_teardown.py @@ -176,11 +176,20 @@ def _force_kill_lingering(worktree: Path) -> int: break time.sleep(0.5) for pid in pids: - if host.is_alive(pid) and ident[pid] is not None and host.identity(pid) == ident[pid]: - try: - host.force_kill(pid) - except OSError: - pass + if not host.is_alive(pid): + continue + if ident[pid] is None or host.identity(pid) != ident[pid]: + print( + f"unity_teardown: pid {pid} survived terminate but its identity is " + "missing or changed since teardown began; skipping force-kill to " + "avoid hitting a reused pid", + file=sys.stderr, + ) + continue + try: + host.force_kill(pid) + except OSError: + pass return len(pids) diff --git a/src/bmad_loop/decisions.py b/src/bmad_loop/decisions.py index c2617c7..8b54e25 100644 --- a/src/bmad_loop/decisions.py +++ b/src/bmad_loop/decisions.py @@ -27,6 +27,7 @@ from pathlib import Path from . import bmadconfig, deferredwork, runs, verify +from .platform_util import atomic_replace from .sweep import Decision, DecisionOption, validate_triage STORE_REL = Path(".bmad-loop") / "decisions.json" @@ -58,7 +59,7 @@ def _write_store(project: Path, data: dict) -> None: path.parent.mkdir(parents=True, exist_ok=True) tmp = path.with_suffix(".tmp") tmp.write_text(json.dumps(data, indent=2, sort_keys=True), encoding="utf-8") - tmp.replace(path) + atomic_replace(tmp, path) def record_pre_answer(project: Path, dw_id: str, option: DecisionOption, *, date: str) -> None: diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 1a8dc9d..564a711 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -39,6 +39,7 @@ SessionRecord, StoryTask, ) +from .platform_util import safe_segment from .plugins import HookBus, HookContext, PluginRegistry from .policy import Policy from .runs import kill_session @@ -134,6 +135,15 @@ def _setup_mcp_agent_id(profile_name: str) -> str: return _SETUP_MCP_AGENT_IDS.get(profile_name, profile_name) +def _session_task_id(story_key: str, part: str, seq: int) -> str: + """Single composition point for session task ids. Sanitize the whole + composition, not the parts: two individually capped parts can still compose + past a Windows filename segment limit, and ``safe_segment``'s digest suffix + differs between the two orders. ``_resumable_session``'s resume match must + be byte-identical to what ``_run_session`` stored, so both MUST call this.""" + return safe_segment(f"{story_key}-{part}-{seq}") + + class Engine: # The engine that installed the process-wide stop handlers; nested # auto-sweep runs (same process) see it set and let RunStopped propagate up. @@ -1175,7 +1185,7 @@ def _resumable_session(self, task: StoryTask) -> tuple[str, SessionResult] | Non role, seq = "review", task.review_cycle else: return None - task_id = f"{task.story_key}-{role}-{seq}" + task_id = _session_task_id(task.story_key, role, seq) for record in reversed(task.sessions): if record.task_id != task_id: continue @@ -1992,7 +2002,7 @@ def _run_session( ) -> SessionResult: # ``label`` names a non-standard session (a plugin-provided workflow) so # its task_id stays distinct from the role's own dev/review attempts. - task_id = f"{task.story_key}-{label or role}-{seq}" + task_id = _session_task_id(task.story_key, label if label else role, seq) adapter = self.adapters[role] cfg = self.policy.adapter.resolved(role) env = { @@ -2167,7 +2177,7 @@ def _reset_spec_for_repair(self, task: StoryTask) -> None: def _write_feedback(self, task: StoryTask, reason: str) -> Path: """Persist a verification failure where the next session can read it — deterministic evidence must reach the LLM, not just the journal.""" - path = self.run_dir / "feedback" / f"{task.story_key}-{len(task.sessions)}.md" + path = self.run_dir / "feedback" / f"{safe_segment(task.story_key)}-{len(task.sessions)}.md" path.parent.mkdir(parents=True, exist_ok=True) path.write_text( f"# Verification feedback: {task.story_key}\n\n" @@ -2329,7 +2339,7 @@ def _stash_deferred_artifacts(self, task: StoryTask) -> None: spec_path = Path(task.spec_file) if not spec_path.is_file(): return - dest = self.run_dir / "deferred" / task.story_key + dest = self.run_dir / "deferred" / safe_segment(task.story_key) dest.mkdir(parents=True, exist_ok=True) shutil.move(str(spec_path), str(dest / spec_path.name)) self.journal.append( diff --git a/src/bmad_loop/journal.py b/src/bmad_loop/journal.py index 0ef7b22..837f7a9 100644 --- a/src/bmad_loop/journal.py +++ b/src/bmad_loop/journal.py @@ -3,12 +3,12 @@ from __future__ import annotations import json -import os import time from pathlib import Path from typing import Any from .model import RunState +from .platform_util import atomic_replace STATE_FILE = "state.json" JOURNAL_FILE = "journal.jsonl" @@ -63,7 +63,7 @@ def save_state(run_dir: Path, state: RunState) -> None: target = run_dir / STATE_FILE tmp = target.with_suffix(".json.tmp") tmp.write_text(json.dumps(state.to_dict(), indent=2), encoding="utf-8") - os.replace(tmp, target) + atomic_replace(tmp, target) def load_state(run_dir: Path) -> RunState: diff --git a/src/bmad_loop/platform_util.py b/src/bmad_loop/platform_util.py index e4e687c..eecc39c 100644 --- a/src/bmad_loop/platform_util.py +++ b/src/bmad_loop/platform_util.py @@ -5,17 +5,47 @@ delegate to it. ``detach_kwargs`` stays a real implementation — it is spawn configuration, not a process-lifecycle primitive, so it does not belong on the host. On Linux/macOS — and WSL, which *is* Linux — these preserve today's exact -behavior; the Windows branch degrades gracefully and is not yet exercised. +behavior. The win32 file-replace and path-segment helpers below (``atomic_replace``, +``safe_segment``) are exercised by the platform tests; the pid kill/liveness +Windows branch degrades gracefully and is not yet exercised. """ from __future__ import annotations +import hashlib +import os +import random +import re import subprocess import sys +import time from pathlib import Path, PurePosixPath, PureWindowsPath from .process_host import get_process_host +# Windows-only: os.replace (MoveFileExW) fails with ERROR_ACCESS_DENIED (5) or +# ERROR_SHARING_VIOLATION (32) when a concurrent reader holds a handle on the +# target — Python's open() grants no FILE_SHARE_DELETE, so renaming over the open +# file is denied. Readers hold their handle briefly, so a jittered backoff clears +# it; an anti-virus / indexer touch can hold longer, hence the ~5 s worst case. +# POSIX rename-over-open never raises this, so the retry stays win32-gated. +_REPLACE_ATTEMPTS = 12 +_REPLACE_BASE_S = 0.02 +_REPLACE_CAP_S = 0.7 + +# Reserved on Windows regardless of extension: CON.txt is as illegal as CON. The +# COM0/LPT0 and superscript (COM¹/COM²/COM³) forms are reserved by the same rule, +# as are the console device names CONIN$/CONOUT$. +_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 "¹²³"} +) +_ILLEGAL_SEGMENT_CHARS = re.compile(r'[<>:"/\\|?*\x00-\x1f]') +_MAX_SEGMENT = 120 # keep segment (incl. any collision suffix) well under the 255 limit + def terminate_pid(pid: int) -> None: """Politely terminate ``pid``. Back-compat shim over @@ -68,3 +98,63 @@ def has_parent_ref(value: str | Path) -> bool: the two for a complete "must stay inside the project" guard.""" text = str(value) return ".." in PurePosixPath(text).parts or ".." in PureWindowsPath(text).parts + + +def atomic_replace(tmp: Path, target: Path) -> None: + """``os.replace(tmp, target)``, retried on the transient Windows sharing + violation a concurrent reader of ``target`` triggers (WinError 5/32). Gated to + win32 so a real POSIX EACCES/EPERM surfaces immediately instead of after a + pointless backoff. Worst-case total wait is ~5 s of jittered exponential + backoff before the final failure propagates.""" + for attempt in range(_REPLACE_ATTEMPTS): + try: + os.replace(tmp, target) + return + except OSError as exc: + last = attempt == _REPLACE_ATTEMPTS - 1 + # a retryable rename-over-open denial, not a genuine permission fault + winerror = getattr(exc, "winerror", None) + retryable = isinstance(exc, PermissionError) or winerror in (5, 32) + # portability: only Windows raises this on rename-over-open; elsewhere a + # permission error is real and must surface at once. + if sys.platform != "win32" or last or not retryable: + raise + delay = min(_REPLACE_CAP_S, _REPLACE_BASE_S * 2**attempt) + time.sleep(delay + random.uniform(0, _REPLACE_BASE_S)) # nosec B311 - retry jitter + + +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.""" + stem = seg.split(".", 1)[0].rstrip(" ") + return stem.upper() in _RESERVED_BASENAMES + + +def safe_segment(name: str) -> str: + """Coerce ``name`` into a single Windows-legal path segment, returning legal + input unchanged (identity for clean keys — the common case, e.g. a story key + like ``3-2-digest-delivery``). + + Replaces the reserved characters ``<>:"/\\|?*`` and control chars with ``_``, + strips trailing dots and spaces (Windows silently drops them), caps the length, + and defuses the reserved device basenames (CON, PRN, AUX, NUL, COM0-9, LPT0-9 + and their superscript ¹²³ forms — case-insensitive, with or without an + extension). Whenever anything is changed a short digest of the raw input is + appended, giving practical (probabilistic, not absolute) collision resistance + between distinct raw names: clean-key identity is the stronger contract, so a + 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] + if _is_reserved_basename(cleaned): + cleaned = "_" + cleaned + if not cleaned: + cleaned = "_" + if cleaned == name: + return name # already a legal segment — keep it byte-identical + digest = hashlib.sha1( + name.encode("utf-8", "surrogatepass"), + usedforsecurity=False, # collision-resistance suffix, not a credential + ).hexdigest() + suffix = "-" + digest[:8] + return cleaned[: _MAX_SEGMENT - len(suffix)] + suffix diff --git a/src/bmad_loop/resolve.py b/src/bmad_loop/resolve.py index e428e8d..64ee7df 100644 --- a/src/bmad_loop/resolve.py +++ b/src/bmad_loop/resolve.py @@ -22,12 +22,13 @@ from .adapters.base import SessionSpec from .model import RunState +from .platform_util import safe_segment RESOLVE_DIR = "resolve" def _story_dir(run_dir: Path, story_key: str) -> Path: - return run_dir / RESOLVE_DIR / story_key + return run_dir / RESOLVE_DIR / safe_segment(story_key) def context_path(run_dir: Path, story_key: str) -> Path: @@ -173,7 +174,7 @@ def run_session(adapter, project: Path, run_dir: Path, story_key: str, *, model: resolution marker. The context file must already be written (build_context). """ spec = SessionSpec( - task_id=f"{story_key}-resolve-1", + task_id=f"{safe_segment(story_key)}-resolve-1", role="dev", prompt=f"/bmad-loop-resolve {story_key}", cwd=project, diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 3c3d83a..0000140 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -14,6 +14,7 @@ from .adapters.multiplexer import get_multiplexer from .journal import STATE_FILE, Journal, load_state, save_state from .model import PAUSE_ESCALATION, Phase +from .platform_util import atomic_replace from .process_host import get_process_host RUNS_DIR = Path(".bmad-loop") / "runs" @@ -357,15 +358,15 @@ def delete_run(run_dir: Path) -> None: def archive_run(project: Path, run_dir: Path) -> Path: """Compress a run dir into .bmad-loop/archive/.tar.gz and remove the - original. The tarball is written to a temp path then os.replace'd into place - so a partial archive never appears. Callers enforce the live guard.""" + original. The tarball is written to a temp path then atomically replaced into + place so a partial archive never appears. Callers enforce the live guard.""" archive_dir = project / ARCHIVE_DIR archive_dir.mkdir(parents=True, exist_ok=True) dest = archive_dir / f"{run_dir.name}.tar.gz" tmp = dest.with_suffix(".tar.gz.tmp") with tarfile.open(tmp, "w:gz") as tar: tar.add(run_dir, arcname=run_dir.name) - os.replace(tmp, dest) + atomic_replace(tmp, dest) shutil.rmtree(run_dir) return dest diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index c3f4767..7b69597 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -22,6 +22,7 @@ from .engine import Engine from .escalation import critical_escalations from .model import Phase, StoryTask +from .platform_util import atomic_replace from .statemachine import advance from .workspace import discard_worktree @@ -793,7 +794,7 @@ def _decisions_phase(self, plan: TriagePlan) -> tuple[dict[str, dict[str, str]], if seeded: tmp = decisions_path.with_suffix(".tmp") tmp.write_text(json.dumps(answers, indent=2), encoding="utf-8") - tmp.replace(decisions_path) + atomic_replace(tmp, decisions_path) pending = [d for d in plan.decisions if d.id not in answers] answered_interactively = False if not self.prompting: @@ -831,7 +832,7 @@ def _decisions_phase(self, plan: TriagePlan) -> tuple[dict[str, dict[str, str]], } tmp = decisions_path.with_suffix(".tmp") tmp.write_text(json.dumps(answers, indent=2), encoding="utf-8") - tmp.replace(decisions_path) + atomic_replace(tmp, decisions_path) self.journal.append( "decision-answered", dw_id=decision.id, diff --git a/src/bmad_loop/tui/settings.py b/src/bmad_loop/tui/settings.py index c17e849..69def89 100644 --- a/src/bmad_loop/tui/settings.py +++ b/src/bmad_loop/tui/settings.py @@ -9,13 +9,13 @@ from __future__ import annotations -import os from pathlib import Path from typing import Any import tomlkit from .. import policy as policy_mod +from ..platform_util import atomic_replace STAGES = ("dev", "review", "triage") @@ -98,4 +98,4 @@ def save(self, path: Path) -> None: path.parent.mkdir(parents=True, exist_ok=True) tmp = path.with_suffix(".toml.tmp") tmp.write_text(self.dumps(), encoding="utf-8") - os.replace(tmp, path) + atomic_replace(tmp, path) diff --git a/src/bmad_loop/workspace.py b/src/bmad_loop/workspace.py index c711ecf..ec6d6d8 100644 --- a/src/bmad_loop/workspace.py +++ b/src/bmad_loop/workspace.py @@ -21,6 +21,7 @@ from . import verify from .bmadconfig import ProjectPaths +from .platform_util import safe_segment # Per-unit worktrees live under the run dir (.bmad-loop/runs//worktrees/), # which `bmad-loop init` already gitignores — so unit checkouts never show up as @@ -85,7 +86,7 @@ def open_unit_workspace( keeps the commits earlier units already landed on it. """ branch = unit_branch_name(run_id, unit_key, branch_per) - wt = (unit_worktrees_dir(run_dir) / unit_key).resolve() + wt = (unit_worktrees_dir(run_dir) / safe_segment(unit_key)).resolve() wt.parent.mkdir(parents=True, exist_ok=True) if verify.branch_exists(repo_root, branch): verify.worktree_add(repo_root, wt, branch, create=False) @@ -134,7 +135,7 @@ def close_unit_workspace( except verify.GitError: diff = "" if diff: - patch = run_dir / "failed" / unit_key / "changes.patch" + patch = run_dir / "failed" / safe_segment(unit_key) / "changes.patch" patch.parent.mkdir(parents=True, exist_ok=True) patch.write_text(diff, encoding="utf-8") if keep_failed: diff --git a/tests/test_engine.py b/tests/test_engine.py index 1d16e3b..57e9a42 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -16,6 +16,7 @@ write_sprint, ) +from bmad_loop import platform_util from bmad_loop.adapters.base import SessionResult from bmad_loop.adapters.mock import MockAdapter from bmad_loop.engine import Engine, RunPaused, RunStopped @@ -487,6 +488,49 @@ def test_run_session_persists_result_json_only_for_resumable_roles(project): assert by_id["1-1-a-tea-trace-1"] is None # labeled → None +def test_run_session_labeled_task_id_capped_as_a_whole(project): + """Two individually legal parts (story_key, plugin label) can compose past + the Windows filename segment cap; the task_id is sanitized as one segment.""" + long_key = "k" * 110 + write_sprint(project, {long_key: "ready-for-dev"}) + engine, _ = make_engine(project, [SessionResult(status="completed")]) + task = StoryTask(story_key=long_key, epic=1) + engine.state.tasks[task.story_key] = task + engine._save() + + 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 + + +@pytest.mark.parametrize("key", ["6-4:cli?list", "k" * 130]) +def test_resumable_session_matches_sanitized_task_id(project, key): + """Resume matching must compose the task_id byte-identically to what + _run_session stored. A story key that sanitization actually changes (dirty + chars, or a clean key whose composed id overflows the segment cap) would + otherwise never match, and _finish_inflight would fall through to the + destructive resume-restart instead of consuming the recorded result.""" + write_sprint(project, {key: "ready-for-dev"}) + engine, _ = make_engine( + project, [SessionResult(status="completed", result_json={"status": "done"})] + ) + task = StoryTask(story_key=key, epic=1) + engine.state.tasks[task.story_key] = task + engine._save() + + engine._run_session(task, role="dev", prompt="p", seq=1) + # the post-save crash window: session recorded, phase still mid-dev + task.phase = Phase.DEV_RUNNING + task.attempt = 1 + + resumable = engine._resumable_session(task) + assert resumable is not None + role, result = resumable + assert role == "dev" + assert result.result_json == {"status": "done"} + + def test_token_budget_discounts_cache_reads(project): """Raw totals dominated by cache reads must not trip the budget; the weighted total (cache reads at 0.1x) is what's checked.""" diff --git a/tests/test_engine_plugin.py b/tests/test_engine_plugin.py index 9a477cf..8e811cb 100644 --- a/tests/test_engine_plugin.py +++ b/tests/test_engine_plugin.py @@ -421,7 +421,7 @@ def test_unity_teardown_sweep_skips_force_kill_when_terminate_suffices(tmp_path, assert host.force_killed == [] -def test_unity_teardown_sweep_skips_force_kill_on_identity_mismatch(tmp_path, monkeypatch): +def test_unity_teardown_sweep_skips_force_kill_on_identity_mismatch(tmp_path, monkeypatch, capsys): """A survivor whose identity changed between snapshot and escalation (pid reused) is never force-killed — guarding against SIGKILL landing on an unrelated process.""" mod = _load_unity_teardown() @@ -433,6 +433,7 @@ def test_unity_teardown_sweep_skips_force_kill_on_identity_mismatch(tmp_path, mo assert mod._force_kill_lingering(tmp_path) == 1 assert host.terminated == [_FAKE_PID] assert host.force_killed == [] # identity no longer matches → refused + assert "skipping force-kill" in capsys.readouterr().err def test_unity_ready_grace_explicit_override(monkeypatch): diff --git a/tests/test_journal.py b/tests/test_journal.py new file mode 100644 index 0000000..ed14c9d --- /dev/null +++ b/tests/test_journal.py @@ -0,0 +1,34 @@ +"""State persistence: the atomic write must survive the transient Windows +sharing violation (WinError 5) a concurrent TUI reader triggers. The retry +lives in platform_util.atomic_replace (unit-tested there); this proves +save_state still rides it end to end.""" + +from __future__ import annotations + +import os + +from bmad_loop import platform_util +from bmad_loop.journal import load_state, save_state +from bmad_loop.model import RunState + + +def test_save_state_retries_transient_sharing_violation(tmp_path, monkeypatch): + """On win32, os.replace denied by a concurrent reader is retried, not fatal.""" + monkeypatch.setattr(platform_util.sys, "platform", "win32") + monkeypatch.setattr(platform_util.time, "sleep", lambda _s: None) # no real backoff + + real_replace = os.replace + calls = {"n": 0} + + def flaky_replace(src, dst): + calls["n"] += 1 + if calls["n"] < 3: # first two collide, third lands + raise PermissionError(5, "Access is denied") + real_replace(src, dst) + + monkeypatch.setattr(platform_util.os, "replace", flaky_replace) + + save_state(tmp_path, RunState(run_id="r1", project="p", started_at="2026-07-06T21:00:00")) + + assert calls["n"] == 3 + assert load_state(tmp_path).run_id == "r1" diff --git a/tests/test_platform_util.py b/tests/test_platform_util.py index b5b9e43..48d3032 100644 --- a/tests/test_platform_util.py +++ b/tests/test_platform_util.py @@ -66,3 +66,134 @@ def test_has_parent_ref_detects_escapes(value): def test_has_parent_ref_ignores_non_segments(value): # `..hidden` / `a..b` contain the substring but not a `..` path segment. assert platform_util.has_parent_ref(value) is False + + +# ---------------------------------------------------------------- atomic_replace + + +def _flaky_replace(fail_times: int, real=os.replace): + """os.replace that raises a sharing violation the first ``fail_times`` calls.""" + calls = {"n": 0} + + def replace(src, dst): + calls["n"] += 1 + if calls["n"] <= fail_times: + raise PermissionError(5, "Access is denied") + real(src, dst) + + return replace, calls + + +def test_atomic_replace_retries_then_succeeds(tmp_path, monkeypatch): + monkeypatch.setattr(platform_util.sys, "platform", "win32") + sleeps: list[float] = [] + monkeypatch.setattr(platform_util.time, "sleep", lambda s: sleeps.append(s)) + + replace, calls = _flaky_replace(2) + monkeypatch.setattr(platform_util.os, "replace", replace) + + src = tmp_path / "s.tmp" + src.write_text("x", encoding="utf-8") + dst = tmp_path / "d.json" + platform_util.atomic_replace(src, dst) + + assert calls["n"] == 3 + assert len(sleeps) == 2 # one backoff before each retry + assert dst.read_text(encoding="utf-8") == "x" + + +def test_atomic_replace_permanent_failure_reraises(tmp_path, monkeypatch): + monkeypatch.setattr(platform_util.sys, "platform", "win32") + monkeypatch.setattr(platform_util.time, "sleep", lambda _s: None) + + def always_denied(src, dst): + raise PermissionError(5, "Access is denied") + + monkeypatch.setattr(platform_util.os, "replace", always_denied) + + with pytest.raises(PermissionError): + platform_util.atomic_replace(tmp_path / "s", tmp_path / "d") + + +def test_atomic_replace_no_retry_on_posix(tmp_path, monkeypatch): + monkeypatch.setattr(platform_util.sys, "platform", "linux") + sleeps: list[float] = [] + monkeypatch.setattr(platform_util.time, "sleep", lambda s: sleeps.append(s)) + + def denied(src, dst): + raise PermissionError(13, "Permission denied") + + monkeypatch.setattr(platform_util.os, "replace", denied) + + with pytest.raises(PermissionError): + platform_util.atomic_replace(tmp_path / "s", tmp_path / "d") + assert sleeps == [] # zero backoff — a real POSIX error surfaces at once + + +# ------------------------------------------------------------------ safe_segment + + +def _is_legal_segment(seg: str) -> bool: + return ( + bool(seg) + 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) + ) + + +@pytest.mark.parametrize( + "value", ["3-2-digest-delivery", "epic1_story2", "a.b.c", "plain", "console"] +) +def test_safe_segment_identity_for_clean_input(value): + # a legal segment (incl. the non-reserved 'console') is returned byte-identical + assert platform_util.safe_segment(value) == value + + +@pytest.mark.parametrize( + "value, base", + [ + ('ac:"d/e\\f|g?h*i', "a_b_c__d_e_f_g_h_i"), # every illegal char -> _ (`:"` = two) + ("with\ttab", "with_tab"), # control char + ("x.", "x"), # trailing dot stripped + ("y ", "y"), # trailing space stripped + ("CON", "_CON"), # reserved basename + ("nul", "_nul"), # case-insensitive + ("COM1.txt", "_COM1.txt"), # reserved even with extension + ("LPT9", "_LPT9"), + ("COM0", "_COM0"), # COM0/LPT0 are reserved too + ("CON .txt", "_CON .txt"), # reserved stem with a trailing space before the extension + ("CONIN$", "_CONIN$"), # console device names are reserved ($ is otherwise legal) + ("conout$.log", "_conout$.log"), # case-insensitive, with extension + ], +) +def test_safe_segment_coerces_and_suffixes_changed_input(value, base): + out = platform_util.safe_segment(value) + assert out != value + assert out.startswith(base + "-") # sanitized base + collision-suffix digest + assert _is_legal_segment(out) + + +def test_safe_segment_distinct_dirty_keys_never_collide(): + # same sanitized base but different raw input must not share a segment (would + # otherwise cross-wire two stories' task dirs / logs / feedback files) + a = platform_util.safe_segment("a:b") + b = platform_util.safe_segment("a?b") + assert a.startswith("a_b-") and b.startswith("a_b-") + assert a != b + + +def test_safe_segment_caps_length(): + out = platform_util.safe_segment("x" * 500) + assert len(out) <= platform_util._MAX_SEGMENT + assert _is_legal_segment(out) + + +def test_dirty_story_key_segment_is_creatable(tmp_path): + # the sanitized segment a consumer builds a dir from must be creatable on this OS + from bmad_loop import resolve + + d = resolve._story_dir(tmp_path, 'a:c."') + d.mkdir(parents=True) + assert d.is_dir() diff --git a/tests/test_portability_guard.py b/tests/test_portability_guard.py index ee03640..3c6268c 100644 --- a/tests/test_portability_guard.py +++ b/tests/test_portability_guard.py @@ -55,6 +55,13 @@ "process_host.py", } +# Broader than the signal-0 probe: *any* `os.kill(` — a real signal send is just as +# destructive-on-Windows as the probe form. Only the ProcessHost may call it directly; +# everything else routes through the seam (terminate / force_kill / is_alive). +OS_KILL_ALLOW = { + "process_host.py", +} + # The two sanctioned `shell=True` spots: operator-authored command strings whose # cmd/PowerShell port is an explicit out-of-scope follow-up. SHELL_ALLOW = { @@ -158,6 +165,17 @@ def line_at(lineno: int) -> str: ): findings.append(("killprobe", rel, node.lineno, line_at(node.lineno))) + # os.kill(...) in any form — every signal send maps to a destructive + # TerminateProcess on Windows, so confine the call to the ProcessHost. + if ( + isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "kill" + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "os" + ): + findings.append(("oskill", rel, node.lineno, line_at(node.lineno))) + # start_new_session=True as a call kwarg if ( isinstance(node, ast.keyword) @@ -245,6 +263,20 @@ def test_pid_existence_probe_only_in_liveness_helpers(): assert not bad, "os.kill(pid, 0) outside liveness helpers:\n" + "\n".join(bad) +def test_os_kill_only_in_process_host(): + """Any reachable ``os.kill`` maps to a destructive TerminateProcess on Windows — + confine it to ``process_host.py``. Detects the literal ``os.kill(`` form only; + import aliases and assigned aliases are deliberately not tracked — this is a + review tripwire, not a sandbox. Other call sites route through the ProcessHost + seam (``terminate`` / ``force_kill`` / ``is_alive``).""" + offenders = [(rel, ln, txt) for _, rel, ln, txt in _of("oskill") if rel not in OS_KILL_ALLOW] + assert ( + not offenders + ), "os.kill( outside process_host.py — route it through the ProcessHost seam:\n" + "\n".join( + f" {rel}:{ln}: {txt.strip()}" for rel, ln, txt in offenders + ) + + def test_start_new_session_only_in_detach_helpers(): """``start_new_session=True`` is POSIX-only — confine it to the detach helpers (which branch on ``sys.platform``), each line carrying a `# portability:` ack.""" diff --git a/tests/test_resolve.py b/tests/test_resolve.py index fe9d601..7e4b023 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -15,6 +15,7 @@ SessionRecord, StoryTask, ) +from bmad_loop.platform_util import safe_segment SPEC = """\ --- @@ -176,6 +177,20 @@ def test_build_context_restore_supported_signal(tmp_path): assert json.loads(path.read_text(encoding="utf-8"))["restore_supported"] is False +def test_build_context_sanitizes_dirty_story_key(tmp_path): + """A story key with Windows-illegal chars lands in a sanitized directory, + while the key itself stays raw inside the context payload (it is data).""" + run_dir, state, _ = _escalated_run(tmp_path) + dirty = "6-4:cli?list" + seg = safe_segment(dirty) + assert seg != dirty + path = resolve.build_context(state, run_dir, dirty) + assert path.parent.name == seg + ctx = json.loads(path.read_text(encoding="utf-8")) + assert ctx["story_key"] == dirty + assert ctx["resolution_path"].endswith(f"resolve/{seg}/resolution.json") + + # ----------------------------------------------------------- rearm_escalation