diff --git a/CHANGELOG.md b/CHANGELOG.md index e7c7971b..d33a7144 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,23 @@ breaking changes may land in a minor release. ### Fixed +- **Path-resolution refusals no longer tear down worktree runs mid-provisioning (#556).** + Observation probes report coarse unknown entries, each refused seed is skipped independently, + provisioning-root uncertainty raises a typed escalation and pauses before any write or session, + and mount uncertainty defers only the affected unit. Required upstream-skill absence still + escalates; this boundary does not include `ProjectPaths` or confined cleanup resolution. + +- **Rollback path uncertainty now triggers a pre-destructive rollback pause (#557).** Trusted + spec containment fails closed, observational exclude and story derivation use explicit + fail-open or empty fallbacks, collision uncertainty keeps and escalates the branch, and exact + commits omit only an uncertain candidate after a trusted root is established. This is limited + to the audited verify and recovery boundaries, not every path-resolution call. + +- **The dashboard now survives project-root resolution refusal with unavailable panes (#558).** + Startup and polling retain empty or unavailable views through one stable lexical/canonical + cache spelling. This preserves the dashboard around a dead provider; it does not recover the + provider or make it operational. + - **Advancing a sprint board now preserves every authored line ending (#576).** A valid UTF-8 board keeps each CRLF, LF, bare-CR or mixed per-line terminator while the requested story, conditional parent-epic and optional `last_updated` values still change. Previously the diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index bc6a1506..74b4d672 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -5287,12 +5287,19 @@ def _carry_isolated_ledger_writes(self, task: StoryTask) -> None: self._carry_board_advance(task) def _harvest_carry_commit_may_degrade(self, ledger: Path) -> bool: - """Whether a carry may remain uncommitted because git cannot own its path.""" + """Whether a carry may remain uncommitted because git cannot own its path. + + Only a path proven external may degrade. Resolution uncertainty must keep + the durable commit-pending latch set rather than guess that git cannot own + a possibly tracked ledger and silently disable its retry. + """ repo = self.paths.repo_root try: rel = ledger.resolve().relative_to(repo.resolve()).as_posix() - except (OSError, RuntimeError, ValueError): - return True # external or unresolvable ledgers are advisory artifacts + except (OSError, RuntimeError): + return False + except ValueError: + return True # a proven external ledger is an advisory artifact if verify.path_tracked(repo, rel): return False return rel not in verify.untracked_files(repo) diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index e0052319..7decd614 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -1300,7 +1300,13 @@ def _walk_traversable_files( an unreadable source directory never leaves an empty destination behind. """ if _is_dir(src): - real = str(src.resolve()) if isinstance(src, Path) else None + try: + real = str(src.resolve()) if isinstance(src, Path) else None + except (OSError, RuntimeError): + if not _suppress_errors: + raise + yield rel, src + return if real is not None and real in _seen: return if _should_descend is not None and not _should_descend(rel, src): diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index 30726fe3..0a44cb0b 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -230,15 +230,26 @@ def safe_reset(self, task: StoryTask, *, preserve: tuple[str, ...] = ()) -> None pause. The BMAD artifact folders are always kept from untracked deletion; ``preserve`` (set only on a resolved re-drive) additionally keeps their *tracked* content alive through the reset, so a just-corrected spec is not - reverted. Sweep passes no ``preserve`` — it wants the broken ledger gone.""" + reverted. Sweep passes no ``preserve`` — it wants the broken ledger gone. + A cleanup-preflight refusal is journaled and routed through the injected + pause before the re-drive can continue.""" workspace = self._workspace_get() - verify.safe_rollback( - workspace.root, - task.baseline_commit or "", - baseline_untracked=task.baseline_untracked, - keep=(".bmad-loop", *self.protected_relpaths()), - preserve=preserve, - ) + try: + verify.safe_rollback( + workspace.root, + task.baseline_commit or "", + baseline_untracked=task.baseline_untracked, + keep=(".bmad-loop", *self.protected_relpaths()), + preserve=preserve, + ) + except verify.RollbackPreflightError as e: + self.journal.append("rollback-reset-failed", story_key=task.story_key, error=str(e)) + self._pause( + f"automatic rollback for {task.story_key} could not safely start: {e}. " + "Fix the underlying filesystem fault, then resume the run.", + task.story_key, + cause=e, + ) def restore_patch(self, task: StoryTask) -> None: """Re-apply the latched intent-gap patch (BMAD-METHOD #2564) onto the diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index 22d98505..c47f02c9 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -35,6 +35,7 @@ PAUSE_STORY_GATE, RunState, ) +from ..platform_util import resolve_or_lexical from ..policy import POLICY_FILE from ..process_host import ProcessHostError from ..runs import RUNS_DIR, RearmError, StopRunError @@ -144,7 +145,7 @@ class BmadLoopApp(App[None]): def __init__(self, project: Path): super().__init__() - self.project = project.resolve() + self.project = resolve_or_lexical(project) self.sub_title = str(self.project) self._dashboard = DashboardScreen(self.project) diff --git a/src/bmad_loop/tui/data.py b/src/bmad_loop/tui/data.py index 4fe75fac..92b15ec3 100644 --- a/src/bmad_loop/tui/data.py +++ b/src/bmad_loop/tui/data.py @@ -31,6 +31,7 @@ from ..gates import ATTENTION_FILE from ..journal import JOURNAL_FILE, LOGS_DIR, STATE_FILE, load_state from ..model import RunState +from ..platform_util import resolve_or_lexical from ..process_host import ProcessHostError from ..runs import ( STOP_REQUEST_FILE, @@ -811,7 +812,7 @@ def pending_decision(journal_entries: list[dict[str, Any]]) -> tuple[str, str] | def _project_paths(project: Path) -> bmadconfig.ProjectPaths | None: """BMAD artifact paths, stat-gated on config.yaml; None when the project is not initialized (or the config is unreadable).""" - project = project.resolve() + project = resolve_or_lexical(project) config_sig = _stat_sig(project / "_bmad" / "bmm" / "config.yaml") cached_paths = _paths_cache.get(project) if config_sig is not None and cached_paths is not None and cached_paths[0] == config_sig: @@ -946,7 +947,7 @@ def pending_missed_decisions(project: Path) -> list: paths = _project_paths(project) if paths is None: return [] - project = project.resolve() + project = paths.project sig = ( _stat_sig(paths.deferred_work), _stat_sig(decisions.store_path(project)), diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 93fd0699..b213e2da 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -88,6 +88,10 @@ class GitSpawnError(GitError): (#343). The underlying errno stays reachable via ``exc.__cause__.errno``.""" +class RollbackPreflightError(GitError): + """Rollback cleanup paths could not be proven safe before mutation.""" + + @overload def _run_git( cmd: list[str], @@ -646,10 +650,10 @@ def path_ignored(repo: Path, path: Path) -> bool: Raises GitError like every other probe in this module. Its caller degrades by treating the path as NOT ignored, which keeps it in the commit — the behavior before this function existed, and the direction that cannot lose a write.""" - repo_root = repo.resolve() try: + repo_root = repo.resolve() rel = Path(path).resolve().relative_to(repo_root).as_posix() - except ValueError: + except (OSError, RuntimeError, ValueError): return False # `./` disarms pathspec magic on a rel beginning with `:` — see the docstring; # it is not decoration. `rel` is already posix-separated and relative, so the @@ -974,6 +978,66 @@ def ref_exists(repo: Path, refname: str) -> bool: return rc == 0 +@dataclass(frozen=True) +class _RollbackCleanupTarget: + """One canonical, confined untracked path and its canonical prune bounds.""" + + path: Path + prune_start: Path + prune_stop: Path + + +@dataclass(frozen=True) +class _RollbackCleanupPlan: + """Canonical cleanup inputs computed before rollback mutates the checkout.""" + + repo_root: Path | None + keep_roots: tuple[Path, ...] + targets: tuple[_RollbackCleanupTarget, ...] + + +def _rollback_cleanup_plan( + repo: Path, + *, + baseline_untracked: list[str] | None, + keep: tuple[str, ...], +) -> _RollbackCleanupPlan: + """Resolve every later cleanup operand before the rollback mutation boundary.""" + if baseline_untracked is None: + return _RollbackCleanupPlan(repo_root=None, keep_roots=(), targets=()) + + created = untracked_files(repo) - set(baseline_untracked) + try: + repo_root = repo.resolve() + keep_roots = tuple((repo_root / rel).resolve() for rel in keep) + targets: list[_RollbackCleanupTarget] = [] + for rel in sorted(created): + path = (repo_root / rel).resolve() + # A created path reached through a symlinked parent can canonicalize + # outside the checkout. Never turn that uncertainty into an external + # deletion; the rollback cleanup is confined to descendants of root. + if path == repo_root or not path.is_relative_to(repo_root): + continue + if any(path == root or path.is_relative_to(root) for root in keep_roots): + continue + targets.append( + _RollbackCleanupTarget( + path=path, + prune_start=path.parent, + prune_stop=repo_root, + ) + ) + except (OSError, RuntimeError) as e: + raise RollbackPreflightError( + f"cannot preflight rollback cleanup paths safely in {repo}: {e}" + ) from e + return _RollbackCleanupPlan( + repo_root=repo_root, + keep_roots=keep_roots, + targets=tuple(targets), + ) + + def safe_rollback( repo: Path, baseline: str, @@ -1014,10 +1078,21 @@ def safe_rollback( would skip the restore and be lost. Instead we read policy.toml's on-disk content before the reset and write it straight back after — independent of the snapshot, covering both the uncommitted and committed cases. + + Before either stash creation or reset, every path needed by the later + untracked cleanup is canonicalized into a confined plan, including its prune + bounds. Resolution uncertainty raises ``RollbackPreflightError`` with the + filesystem fault as its cause, so the caller can pause while the tree is + untouched; the post-reset cleanup consumes the plan without resolving again. """ # policy.toml: capture on-disk content now, restore unconditionally below. policy_path = repo / POLICY_FILE_REL policy_content = policy_path.read_bytes() if policy_path.is_file() else None + cleanup = _rollback_cleanup_plan( + repo, + baseline_untracked=baseline_untracked, + keep=keep, + ) rc, out, detail = _git_out(repo, "stash", "create") # A failed `stash create` silently empties `snapshot`, which disables the whole @@ -1082,25 +1157,17 @@ def safe_rollback( # `.bmad-loop/policy.toml`, so honouring a link planted there would aim # a host-side write at a path of that session's choosing. atomic_write_bytes(policy_path, policy_content, follow_symlinks=False) - if baseline_untracked is None: - return # no snapshot to diff against: never delete untracked files - created = untracked_files(repo) - set(baseline_untracked) - repo = repo.resolve() - keep_roots = [(repo / k).resolve() for k in keep] - for rel in sorted(created): - path = (repo / rel).resolve() - if any(path == root or path.is_relative_to(root) for root in keep_roots): - continue + for target in cleanup.targets: try: - path.unlink(missing_ok=True) + target.path.unlink(missing_ok=True) except OSError: continue - _prune_empty_parents(path.parent, repo) + _prune_empty_parents(target.prune_start, target.prune_stop) def _prune_empty_parents(start: Path, repo: Path) -> None: - """Remove now-empty directories from `start` up to (not including) `repo`.""" - d = start.resolve() + """Prune canonical parents supplied by the pre-mutation cleanup plan.""" + d = start while d != repo and d.is_relative_to(repo): try: d.rmdir() # succeeds only when empty @@ -1328,7 +1395,21 @@ def clean_incoming_collisions( tolerated = [p for p in stray if dirty[p].startswith("??")] if tolerated and on_tolerated is not None: on_tolerated(tolerated) + # Resolve every untracked cleanup parent before deleting or checking out any + # path. A later resolution fault must not leave an earlier collision cleaned + # and the checkout only partly reconciled. repo_res = repo.resolve() + prune_starts: dict[str, Path] = {} + for path, xy in sorted(dirty.items()): + if path not in incoming or not xy.startswith("??"): + continue + parent = (repo / path).parent.resolve() + if parent != repo_res and not parent.is_relative_to(repo_res): + raise OSError( + f"refusing to clean incoming collision outside repository {repo_res}: " + f"{repo / path}" + ) + prune_starts[path] = parent cleaned: list[str] = [] for path, xy in sorted(dirty.items()): if path not in incoming: @@ -1336,8 +1417,8 @@ def clean_incoming_collisions( if xy.startswith("??"): # untracked: delete it, then prune emptied dirs fp = repo / path fp.unlink(missing_ok=True) - parent = fp.parent - while parent.resolve() != repo_res and parent.is_dir() and not any(parent.iterdir()): + parent = prune_starts[path] + while parent != repo_res and parent.is_dir() and not any(parent.iterdir()): parent.rmdir() parent = parent.parent else: # tracked-modified: restore to the target's committed version @@ -1591,12 +1672,12 @@ def verify_dev_exclude_relpaths( if restore_patch: candidates.append(resolve_restore_path(restore_patch, paths.project)) out: list[str] = [] - project = paths.project.resolve() + project = paths.project for path in candidates: try: rel = path.resolve().relative_to(project).as_posix() - except ValueError: - continue # outside the project tree; nothing to exclude here + except (OSError, RuntimeError, ValueError): + continue # outside or uncertain; nothing safe to exclude here if rel and rel != ".": out.append(rel) return tuple(out) @@ -1609,14 +1690,17 @@ def spec_within_roots(spec_path: Path, paths: ProjectPaths) -> bool: these roots, so a surprising path can never be silently rewritten. Artifact dirs configured outside ``project`` are roots too, so a legitimately out-of-project spec is still allowed.""" - sp = spec_path.resolve() - roots = ( - paths.project, - paths.output_folder, - paths.implementation_artifacts, - paths.planning_artifacts, - ) - return any(sp == r.resolve() or sp.is_relative_to(r.resolve()) for r in roots) + try: + sp = spec_path.resolve() + roots = ( + paths.project, + paths.output_folder, + paths.implementation_artifacts, + paths.planning_artifacts, + ) + return any(sp == r.resolve() or sp.is_relative_to(r.resolve()) for r in roots) + except (OSError, RuntimeError): + return False def resolve_spec_path(spec_file: str, paths: ProjectPaths) -> Path: @@ -2013,7 +2097,7 @@ def _stories_relpaths(project: Path, spec_folder: Path) -> tuple[str, ...]: try: rel = spec_folder.resolve().relative_to(project.resolve()).as_posix() - except ValueError: + except (OSError, RuntimeError, ValueError): return () base = "" if rel == "." else f"{rel}/" return (f"{base}{STORIES_SUBDIR}", f"{base}{STORIES_FILENAME}") @@ -2547,9 +2631,18 @@ def commit_paths(repo: Path, message: str, paths: list[Path]) -> str | None: optional path would otherwise sink the whole commit (a swallowed `GitError` in `confirm` silently losing the spec+board commit over a park record that was never committed). A missing-but-TRACKED path stays in: that is a - deletion to stage.""" + deletion to stage. An uncertain repo root raises before staging; uncertainty + in one candidate omits only that candidate, preserving the partial-path + contract for healthy siblings. If no usable operand survives that uncertainty, + the call raises instead of reporting a successful no-op.""" rels: list[str] = [] - repo_root = repo.resolve() + resolution_fault: tuple[Path, OSError | RuntimeError] | None = None + try: + repo_root = repo.resolve() + except (OSError, RuntimeError) as e: + raise GitError( + f"cannot resolve repository root for exact commit safely ({repo}): {e}" + ) from e for p in paths: try: # `.as_posix()`, not `str()`: every rel here becomes a git pathspec and is @@ -2557,6 +2650,10 @@ def commit_paths(repo: Path, message: str, paths: list[Path]) -> str | None: # every platform. `str()` yields backslashes on Windows, which git reads as # wildmatch ESCAPES rather than separators. rels.append(Path(p).resolve().relative_to(repo_root).as_posix()) + except (OSError, RuntimeError) as e: + if resolution_fault is None: + resolution_fault = (Path(p), e) + continue except ValueError: continue missing = [r for r in rels if not ((repo_root / r).exists() or (repo_root / r).is_symlink())] @@ -2567,6 +2664,12 @@ def commit_paths(repo: Path, message: str, paths: list[Path]) -> str | None: tracked = {t for t in out.split("\0") if t} rels = [r for r in rels if r not in missing or r in tracked] if not rels: + if resolution_fault is not None: + failed_path, error = resolution_fault + raise GitError( + "no exact commit operand remains after path resolution failed " + f"for {failed_path}: {error}" + ) from error return None # Every operand is forced LITERAL: git reads a positional operand as a PATHSPEC, # and `implementation_artifacts` reaches here verbatim out of the operator's diff --git a/src/bmad_loop/workspace.py b/src/bmad_loop/workspace.py index ba531678..6712c743 100644 --- a/src/bmad_loop/workspace.py +++ b/src/bmad_loop/workspace.py @@ -118,7 +118,13 @@ 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) / safe_segment(unit_key)).resolve() + unresolved_wt = unit_worktrees_dir(run_dir) / safe_segment(unit_key) + try: + wt = unresolved_wt.resolve() + except (OSError, RuntimeError) as e: + raise verify.GitError( + f"cannot resolve worktree mount path for {unit_key} ({unresolved_wt}): {e}" + ) from e wt.parent.mkdir(parents=True, exist_ok=True) if verify.branch_exists(repo_root, branch): verify.worktree_add(repo_root, wt, branch, create=False) diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index 2c726dc3..fccea263 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -373,8 +373,21 @@ def worktree_seed_undelivered( source escape, a symlinked destination, or destination escape proves the seed was refused. This report is informational and is never an escalation gate. """ - worktree = worktree.resolve() - repo_root = repo_root.resolve() + unresolved_repo_root = repo_root + try: + worktree = worktree.resolve() + repo_root = repo_root.resolve() + except (OSError, RuntimeError): + # Observation only: root uncertainty cannot prove delivery, but it must + # not turn an informational journal probe into a run-wide failure. + rels = [str(rel) for rel in seed_files] + for pattern in seed_globs: + try: + matches = sorted(unresolved_repo_root.glob(pattern)) + except (OSError, RuntimeError): + continue + rels.extend(match.relative_to(unresolved_repo_root).as_posix() for match in matches) + return list(dict.fromkeys(rels)) rels = [str(rel) for rel in seed_files] for pattern in seed_globs: rels.extend( @@ -477,7 +490,17 @@ def module_skills_seed_undelivered( """ if skills_root is None: skills_root = resources.files("bmad_loop.data").joinpath("skills") - worktree = worktree.resolve() + try: + worktree = worktree.resolve() + except (OSError, RuntimeError): + # This is a journal-only observation. Root uncertainty means every + # bundled skill the wheel actually carries is coarsely undelivered. + return [ + f"{tree}/{skill}" + for tree in dict.fromkeys(trees) + for skill in MODULE_SKILLS + if _is_file(skills_root.joinpath(skill)) or _is_dir(skills_root.joinpath(skill)) + ] def contained(target: Path) -> bool: try: @@ -592,8 +615,16 @@ def provision_worktree( """ if not profiles and not seed_files and not seed_globs and not _is_dir(repo_root / BMAD_DIR): return [] - worktree = worktree.resolve() - repo_root = repo_root.resolve() + unresolved_worktree = worktree + unresolved_repo_root = repo_root + try: + worktree = worktree.resolve() + repo_root = repo_root.resolve() + except (OSError, RuntimeError) as e: + raise verify.GitError( + "cannot resolve worktree provisioning roots safely " + f"(worktree={unresolved_worktree}, repo_root={unresolved_repo_root}): {e}" + ) from e relay = repo_root / HOOK_SCRIPT_REL skills_root = resources.files("bmad_loop.data").joinpath("skills") @@ -609,9 +640,12 @@ def provision_worktree( # misconfiguration, exactly like the glob-expanded matches below. skipped: list[str] = [] for rel in seed_files: - src = (repo_root / rel).resolve() raw = worktree / rel - dst = raw.resolve() + try: + src = (repo_root / rel).resolve() + dst = raw.resolve() + except (OSError, RuntimeError): + continue if not src.is_relative_to(repo_root) or not dst.is_relative_to(worktree): continue if not (_is_file(src) or _is_dir(src)): @@ -672,9 +706,12 @@ def provision_worktree( for pattern in seed_globs: for match in sorted(repo_root.glob(pattern)): rel = match.relative_to(repo_root) - src = match.resolve() raw = worktree / rel - dst = raw.resolve() + try: + src = match.resolve() + dst = raw.resolve() + except (OSError, RuntimeError): + continue if not src.is_relative_to(repo_root) or not dst.is_relative_to(worktree): continue if not (_is_file(src) or _is_dir(src)) or _occupied(dst): @@ -745,7 +782,10 @@ def provision_worktree( # never there. for skill in _worktree_skill_copy_candidates(repo_root, tree): dst = tree_dir / skill - src = (repo_root / tree / skill).resolve() + try: + src = (repo_root / tree / skill).resolve() + except (OSError, RuntimeError): + continue if not src.is_relative_to(repo_root) or not _is_dir(src): continue _copy_traversable( @@ -1219,14 +1259,21 @@ def run_isolated(self, task: StoryTask, drive: Callable[[StoryTask], None]) -> N seeds.extend(self._registry.seed_files()) seed_files = list(dict.fromkeys(seeds)) # dedupe, preserve order seed_globs = self._registry.seed_globs() - skipped_seeds = provision_worktree( - unit.path, - profiles, - self.paths.repo_root, - seed_files=seed_files, - seed_globs=seed_globs, - on_degraded=lambda msg: self._exclude_degraded(task.story_key, msg), - ) + try: + skipped_seeds = provision_worktree( + unit.path, + profiles, + self.paths.repo_root, + seed_files=seed_files, + seed_globs=seed_globs, + on_degraded=lambda msg: self._exclude_degraded(task.story_key, msg), + ) + except verify.GitError as e: + reason = ( + f"cannot safely provision the worktree for {task.story_key} because " + f"a provisioning root could not be resolved: {e}" + ) + self.escalate_unit(task, reason) # always raises RunPaused if skipped_seeds: # A seed entry whose destination already exists is a no-op. Harmless for # a file the checkout legitimately carries, but a directory entry is @@ -1469,12 +1516,12 @@ def merge_local( paths=paths, ), ) - except (verify.GitError, OSError) as e: - # OSError joins GitError because clean_incoming_collisions mutates the - # checkout directly (unlink/iterdir/rmdir) — a non-spawn FS fault the - # #343 chokepoint cannot translate. Crashing here would strand a DONE - # unit mid-merge; the keep-branch escalation is the point of this guard. - if isinstance(e, (verify.GitSpawnError, OSError)): + except (verify.GitError, OSError, RuntimeError) as e: + # OSError/RuntimeError join GitError because clean_incoming_collisions + # mutates the checkout directly (resolve/unlink/iterdir/rmdir) — non-spawn + # FS faults the #343 chokepoint cannot translate. Crashing here would + # strand a DONE unit mid-merge; keep-branch escalation is this boundary. + if isinstance(e, (verify.GitSpawnError, OSError, RuntimeError)): # environment fault (spawn failure or direct-FS error) — there may # be no stray files at all, so no "clean them" guidance: the inner # error is the diagnosis. diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index 325723be..0bff4fbc 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -24,6 +24,7 @@ git, ignore_before_commit, install_build_auto_skill, + refuse_to_resolve, set_sprint, write_ledger, write_spec, @@ -1016,6 +1017,40 @@ def commit_fails(*args, **kwargs): assert load_state(engine.run_dir).tasks[task.story_key].harvest_carry_commit_pending +def test_uncertain_harvest_ledger_keeps_its_pending_commit(project, monkeypatch): + """Resolution uncertainty cannot turn a tracked carry into advisory success.""" + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + project.deferred_work.write_text("# Deferred Work\n", encoding="utf-8") + commit_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, _ = make_engine(project, []) + record = _harvest_record() + deferredwork.append_entry( + project.deferred_work, + title=record["title"], + origin=record["origin"], + location=record["location"], + source_spec=record["source_spec"], + reason=record["reason"], + severity=record["severity"], + ) + task = StoryTask( + story_key="1-1-a", + epic=1, + harvested_deferrals=[record], + harvest_carry_commit_pending=True, + ) + engine.state.tasks[task.story_key] = task + engine._save() + refuse_to_resolve(monkeypatch, project.deferred_work) + + with pytest.raises(verify.GitError, match="no exact commit operand remains"): + engine._carry_harvested_deferrals(task) + + assert load_state(engine.run_dir).tasks[task.story_key].harvest_carry_commit_pending + assert "harvest-carried" not in journal_kinds(engine) + assert "harvest-carry-uncommitted" not in journal_kinds(engine) + + def test_untracked_nonignored_harvest_carry_commit_failure_propagates(project, monkeypatch): """An ordinary new ledger is committable, so its git failure is fatal too.""" engine, _ = make_engine(project, []) @@ -3025,25 +3060,26 @@ def test_merge_tolerates_untracked_stray_in_main_checkout(project): "make_exc", [ lambda: OSError(13, "Permission denied"), + lambda: RuntimeError("Permission denied while resolving collision cleanup"), lambda: verify.GitSpawnError("git status failed to spawn: Permission denied"), ], - ids=["fs-oserror", "git-spawn"], + ids=["fs-oserror", "fs-runtimeerror", "git-spawn"], ) def test_merge_env_fault_during_target_clean_keeps_branch_and_escalates( project, monkeypatch, make_exc ): """#343: `clean_incoming_collisions` mutates the checkout directly - (unlink/rmdir), so a non-spawn FS fault arrives as a plain OSError no - chokepoint can translate — and its git reads can raise a typed - GitSpawnError. The guard must treat both like any other reconcile + (resolve/unlink/rmdir), so non-spawn FS faults arrive as plain OSError or + RuntimeError values no chokepoint can translate — and its git reads can raise + a typed GitSpawnError. The guard must treat all three like any other reconcile failure: keep the branch and escalate rather than crash a DONE unit mid-merge — and the escalation must name the environment fault, not claim stray uncommitted files that may not exist. - Ablation targets: narrow the guard in `merge_local` back to - `verify.GitError` and the fs-oserror case fails — the OSError crashes the - run. Revert the reason branch to the unconditional stray-files text and - both cases fail on the message assertions.""" + Ablation targets: remove `RuntimeError` only from `merge_local`'s catch and the + fs-runtimeerror row fails because the run crashes. Keep that catch but remove + `RuntimeError` only from its environmental `isinstance` arm and the same row + fails on stray-dirt guidance; the underlying-fault guidance is required.""" commit_sprint(project, {"1-1-a": "ready-for-dev"}) engine, _ = make_engine( project, diff --git a/tests/test_install.py b/tests/test_install.py index bebccc39..63e3e3fd 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -20,6 +20,7 @@ git, install_build_auto_skill, install_dev_shim, + refuse_to_resolve, ) import bmad_loop.install as install_mod @@ -3274,6 +3275,85 @@ def test_provision_worktree_seed_rejects_escaping_path(tmp_path): assert not wt.exists() # nothing copied, no dirs created +@pytest.mark.parametrize("refused_root", ["worktree", "repo"]) +def test_provision_worktree_root_resolution_fault_is_typed_and_precedes_writes( + tmp_path, monkeypatch, refused_root +): + """Provisioning cannot write against roots whose identity is uncertain. + + Ablation: delete the provisioning-root translation and this raises raw + ``OSError`` instead of typed ``GitError`` before the seed or hook config write. + """ + repo, wt = tmp_path / "repo", tmp_path / "wt" + repo.mkdir() + (repo / "seed.json").write_text("FROM_REPO\n", encoding="utf-8") + profile = get_profile("claude") + refused = wt if refused_root == "worktree" else repo + refuse_to_resolve(monkeypatch, refused) + + with pytest.raises(verify.GitError) as excinfo: + provision_worktree(wt, [profile], repo, seed_files=["seed.json"]) + + assert "provisioning roots" in str(excinfo.value) + assert isinstance(excinfo.value.__cause__, OSError) + assert not (wt / "seed.json").exists() + assert not (wt / profile.hooks.config_path).exists() + + +@pytest.mark.parametrize("refused_side", ["source", "destination"]) +def test_provision_worktree_refuses_one_explicit_seed_but_copies_healthy_sibling( + tmp_path, monkeypatch, refused_side +): + """Resolution uncertainty is scoped to one explicit seed entry. + + Ablation: delete the explicit-entry resolution guard and the provider fault + aborts provisioning before the healthy sibling can be copied. + """ + repo, wt = tmp_path / "repo", tmp_path / "wt" + repo.mkdir() + (repo / "refused.json").write_text("REFUSED\n", encoding="utf-8") + (repo / "healthy.json").write_text("HEALTHY\n", encoding="utf-8") + refused = repo / "refused.json" if refused_side == "source" else wt / "refused.json" + refuse_to_resolve(monkeypatch, refused) + + provision_worktree( + wt, + [], + repo, + seed_files=["refused.json", "healthy.json"], + ) + + assert not (wt / "refused.json").exists() + assert (wt / "healthy.json").read_text(encoding="utf-8") == "HEALTHY\n" + + +@pytest.mark.parametrize("refused_side", ["source", "destination"]) +def test_provision_worktree_refuses_one_glob_match_but_copies_healthy_sibling( + tmp_path, monkeypatch, refused_side +): + """One uncertain glob match cannot abort the rest of a stable expansion. + + Ablation: delete the glob-entry resolution guard and the provider fault aborts + provisioning before the healthy match can be copied. + """ + repo, wt = tmp_path / "repo", tmp_path / "wt" + matches = repo / "plugins" + matches.mkdir(parents=True) + (matches / "a-refused.json").write_text("REFUSED\n", encoding="utf-8") + (matches / "z-healthy.json").write_text("HEALTHY\n", encoding="utf-8") + refused = ( + matches / "a-refused.json" + if refused_side == "source" + else wt / "plugins" / "a-refused.json" + ) + refuse_to_resolve(monkeypatch, refused) + + provision_worktree(wt, [], repo, seed_globs=["plugins/*.json"]) + + assert not (wt / "plugins" / "a-refused.json").exists() + assert (wt / "plugins" / "z-healthy.json").read_text(encoding="utf-8") == "HEALTHY\n" + + @pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") def test_seed_files_refuses_a_dangling_destination_leaf_without_excluding_it(tmp_path, monkeypatch): import bmad_loop.worktree_flow as worktree_flow @@ -7109,6 +7189,31 @@ def test_walk_keeps_sibling_symlinks_to_one_shared_tree(tmp_path): ] +def test_walk_resolution_refusal_obeys_the_existing_suppression_split(tmp_path, monkeypatch): + """Cycle-key uncertainty is a named leaf only for observation/copy walks. + + Ablation: delete the cycle-key resolution guard and the suppressing walk raises + instead of yielding ``refused``; forcing suppression in both arms makes the + repair walk fail to re-raise the original provider ``OSError``. + """ + from bmad_loop.install import _walk_traversable_files + + root = tmp_path / "tree" + refused = root / "refused" + refused.mkdir(parents=True) + (refused / "hidden.md").write_text("hidden\n", encoding="utf-8") + (root / "sibling.md").write_text("sibling\n", encoding="utf-8") + refuse_to_resolve(monkeypatch, refused) + + walked = dict(_walk_traversable_files(root, _suppress_errors=True)) + assert sorted(walked) == ["refused", "sibling.md"] + assert walked["refused"] == refused + + with pytest.raises(OSError) as excinfo: + list(_walk_traversable_files(root, _suppress_errors=False)) + assert "stubbed: the provider is registered but not serving" in str(excinfo.value) + + @pytest.mark.skipif(os.name == "nt", reason="POSIX symlinks") def test_guarded_copy_treats_a_dangling_destination_leaf_as_occupied(tmp_path): repo, wt = tmp_path / "repo", tmp_path / "wt" @@ -7472,6 +7577,35 @@ def test_base_skills_seed_incomplete_ignores_inactive_catalog_symlink(tmp_path): assert base_skills_seed_incomplete(wt, repo, [tree]) == [] +def test_provision_worktree_refuses_one_upstream_skill_and_preserves_required_result_gate( + tmp_path, monkeypatch +): + """An uncertain upstream source is skipped while healthy skills still copy. + + Ablation: delete the upstream-skill resolution guard and the provider fault + aborts provisioning instead of reaching the required-skill result re-probe. + """ + wt, repo = tmp_path / "wt", tmp_path / "repo" + tree = ".claude/skills" + required_review = "bmad-review" + _install_dev_auto( + repo, + tree, + skill=DEV_PRIMITIVE_NEW, + customize="[workflow]\n" + _layer("blind", required_review), + ) + _install_skills(repo, tree, {required_review: ()}) + refuse_to_resolve(monkeypatch, repo / tree / required_review) + + skipped = provision_worktree(wt, [get_profile("claude")], repo) + + missing_rel = f"{tree}/{required_review}" + assert (wt / tree / DEV_PRIMITIVE_NEW / "SKILL.md").is_file() + assert not (wt / tree / required_review).exists() + assert missing_rel in skipped + assert base_skills_seed_incomplete(wt, repo, [tree]) == [missing_rel] + + def test_advisory_review_skill_is_copied_best_effort_but_not_fatal(tmp_path): """Conditional review skills remain copy candidates, never hard requirements.""" wt, repo = tmp_path / "wt", tmp_path / "repo" @@ -7663,6 +7797,30 @@ def test_worktree_seed_undelivered_names_an_escaped_source_despite_stale_destina assert worktree_seed_undelivered(wt, repo, seed_files=[".mcp.json"]) == [".mcp.json"] +@pytest.mark.parametrize("refused_root", ["worktree", "repo"]) +def test_worktree_seed_undelivered_reports_coarse_names_when_a_root_is_unresolvable( + tmp_path, monkeypatch, refused_root +): + """The journal-only probe reports uncertainty without becoming a run failure. + + Ablation: delete the root-resolution guard and this raises the scoped provider + fault instead of returning the configured and safely enumerable coarse names. + """ + repo, wt = tmp_path / "repo", tmp_path / "wt" + (repo / "plugins").mkdir(parents=True) + (repo / "plugins" / "b.json").write_text("{}\n", encoding="utf-8") + (repo / "plugins" / "a.json").write_text("{}\n", encoding="utf-8") + refused = wt if refused_root == "worktree" else repo + refuse_to_resolve(monkeypatch, refused) + + assert worktree_seed_undelivered( + wt, + repo, + seed_files=["configured.json", "configured.json"], + seed_globs=["plugins/*.json"], + ) == ["configured.json", "plugins/a.json", "plugins/b.json"] + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") @pytest.mark.parametrize("escaped_kind", ["file", "directory"]) def test_worktree_seed_undelivered_rejects_stale_nested_escaped_source(tmp_path, escaped_kind): @@ -7788,6 +7946,26 @@ def test_module_skills_seed_undelivered_reports_only_missing_content(tmp_path): ] +def test_module_skills_seed_undelivered_reports_coarse_names_when_root_is_unresolvable( + tmp_path, monkeypatch +): + """The journal-only wheel probe names existing skills in stable tree order. + + Ablation: delete the worktree-root guard and this raises the provider fault + instead of reporting coarse uncertainty for the bundled skill surface. + """ + wt = tmp_path / "wt" + refuse_to_resolve(monkeypatch, wt) + + assert module_skills_seed_undelivered( + wt, [_MODULE_SKILL_TREE, ".agents/skills", _MODULE_SKILL_TREE] + ) == [ + f"{tree}/{skill}" + for tree in (_MODULE_SKILL_TREE, ".agents/skills") + for skill in MODULE_SKILLS + ] + + def test_module_skills_seed_undelivered_answers_through_a_zip_source(tmp_path): """A zip-imported wheel is a Traversable with no filesystem path, so the probe must enumerate it through the shared walk rather than `rglob`. Skills the source diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index e579922f..459e50da 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -12,7 +12,7 @@ from types import SimpleNamespace import pytest -from conftest import git +from conftest import git, refuse_to_resolve from bmad_loop import verify from bmad_loop.gates import ATTENTION_FILE @@ -104,7 +104,7 @@ def _escalate(task, reason) -> None: raise _Pause(reason, task.story_key) def _pause(reason, story_key="", *, cause=None): - calls.pauses.append((reason, story_key)) + calls.pauses.append((reason, story_key, cause)) raise _Pause(reason, story_key) flow = RecoveryFlow( @@ -903,6 +903,43 @@ def test_safe_reset_reverts_tracked_and_keeps_baseline(project): assert rev_parse_head(repo) == task.baseline_commit +def test_safe_reset_preflight_failure_journals_and_pauses_redrive(project, monkeypatch): + """A typed cleanup-preflight refusal is journaled and passed to the injected + pause as its exact cause; the resolved re-drive stops before post-rollback or + any destructive reset can run. + + Ablation target: delete the `except RollbackPreflightError` journal/pause block + and this test fails on the uncaught typed error; delete only `_pause` and it + fails because `post_rollback` continues and the expected pause is absent. + """ + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws) + task = _task(repo) + created = repo / "uncertain" / "created.txt" + created.parent.mkdir() + created.write_text("run-created\n") + (repo / "src.txt").write_text("tracked attempt\n") + refuse_to_resolve(monkeypatch, created) + monkeypatch.setattr(flow, "preserve_attempt_commits", lambda *args, **kwargs: None) + monkeypatch.setattr(flow, "preserve_attempt_worktree", lambda *args, **kwargs: None) + + with pytest.raises(_Pause): + flow.rollback_or_pause(task, cause="resolved") + + failure = flow.journal.fields("rollback-reset-failed") + assert "preflight rollback cleanup" in failure["error"] + assert len(flow.calls.pauses) == 1 + reason, story_key, cause = flow.calls.pauses[0] + assert story_key == task.story_key + assert isinstance(cause, verify.RollbackPreflightError) + assert cause is not None and cause.__cause__ is not None + assert str(cause) in reason + assert flow.calls.emits == ["pre_rollback"] # no post-reset re-drive continuation + assert (repo / "src.txt").read_text() == "tracked attempt\n" + assert created.read_text() == "run-created\n" + + # --------------------------------------------------------------- prune diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index b1bb0377..713a9933 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -15,7 +15,13 @@ from pathlib import Path import pytest -from conftest import git, install_bmad_config, make_validate_document, write_sprint +from conftest import ( + git, + install_bmad_config, + make_validate_document, + refuse_to_resolve, + write_sprint, +) from rich.console import Console from rich.text import Text from textual.events import MouseMove @@ -207,6 +213,30 @@ async def test_empty_project_shows_hint(project): assert "no runs found" in header +async def test_dashboard_survives_project_root_resolve_refusal(project, monkeypatch): + """The app mounts and completes a poll while the project root is unavailable. + + INVERSE ablation: restore bare ``project.resolve()`` in ``BmadLoopApp.__init__`` + and construction raises the stubbed WinError 64 before Textual can start. + """ + applied_polls = 0 + apply_snapshot = DashboardScreen._apply + + def track_poll(self, snapshot): + nonlocal applied_polls + apply_snapshot(self, snapshot) + applied_polls += 1 + + monkeypatch.setattr(DashboardScreen, "_apply", track_poll) + refuse_to_resolve(monkeypatch, project.project) + app = BmadLoopApp(project.project) + + async with app.run_test() as pilot: + await until(pilot, lambda: isinstance(app.screen, DashboardScreen)) + await until(pilot, lambda: applied_polls > 0) + assert dashboard(app).is_running + + async def test_run_table_populates_and_selects_newest(project): root = project.project make_run(root, "20260611-100000-aaaa", finished=True) diff --git a/tests/test_tui_data.py b/tests/test_tui_data.py index 7cb9bfcb..bd4e4a70 100644 --- a/tests/test_tui_data.py +++ b/tests/test_tui_data.py @@ -10,9 +10,9 @@ from pathlib import Path import pytest -from conftest import install_bmad_config, write_sprint +from conftest import install_bmad_config, refuse_to_resolve, write_sprint -from bmad_loop import deferredwork, policy +from bmad_loop import bmadconfig, deferredwork, policy from bmad_loop.adapters import tmux_base from bmad_loop.journal import Journal, save_state from bmad_loop.model import RunState @@ -84,6 +84,32 @@ def test_pending_missed_decisions_reads_and_caches(project, monkeypatch): assert data.pending_missed_decisions(project.project) is pending +def test_pending_missed_decisions_uses_loaded_project_root(project, monkeypatch): + """The canonical root from ProjectPaths is both the reader and cache key. + + INVERSE ablation: restore the second ``project.resolve()`` in + ``pending_missed_decisions`` and this test raises the stubbed WinError 64 + instead of returning the cached decision from the already-loaded root. + """ + from conftest import write_ledger + + install_bmad_config(project) + write_ledger(project, {"DW-1": "open"}) + run_dir = make_run(project.project, "20260101-000000-aaaa") + _write_triage_decision(run_dir) + paths = bmadconfig.load_paths(project.project) + original_spelling = project.project / "unresolved-alias" / ".." + monkeypatch.setattr(data, "_project_paths", lambda _project: paths) + refuse_to_resolve(monkeypatch, original_spelling) + + pending = data.pending_missed_decisions(original_spelling) + + assert [decision.id for decision in pending] == ["DW-1"] + assert data.pending_missed_decisions(original_spelling) is pending + assert paths.project in data._missed_cache + assert original_spelling not in data._missed_cache + + def test_pending_missed_decisions_empty_for_uninitialized(tmp_path): assert data.pending_missed_decisions(tmp_path) == [] @@ -953,6 +979,52 @@ def test_pending_decision_missing_fields(): # ------------------------------------------------------------ sprint overview +def test_project_paths_degrades_and_recovers_from_root_resolve_refusal(project, monkeypatch): + """A dead provider yields unavailable readers without poisoning recovery. + + INVERSE ablation: restore bare ``project.resolve()`` in ``_project_paths`` + and this test raises the stubbed WinError 64 on its first observation rather + than returning empty panes and recovering after the provider is healthy. + """ + install_bmad_config(project) + write_sprint(project, {"1-1-a": "ready-for-dev"}) + root = project.project + + with monkeypatch.context() as refusal: + refuse_to_resolve(refusal, root) + assert data._project_paths(root) is None + assert data.sprint_overview(root) is None + assert data.deferred_entries(root) is None + assert data.pending_missed_decisions(root) == [] + assert root not in data._paths_cache + + paths = data._project_paths(root) + assert paths is not None + assert paths.project == root + assert data._paths_cache[root][1] is paths + assert data.sprint_overview(root) is not None + + +def test_project_paths_uses_one_canonical_cache_key(project): + """Healthy aliases share one ProjectPaths snapshot under the canonical root. + + INVERSE ablation: key ``_paths_cache`` with the pre-canonical spelling while + loading from the stable root and this test finds the ``..`` spelling as a + second cache key instead of reusing the canonical entry. + """ + install_bmad_config(project) + root = project.project.resolve() + alternate_spelling = root / ".." / root.name + + paths = data._project_paths(alternate_spelling) + + assert paths is not None + assert paths.project == root + assert data._project_paths(root) is paths + assert root in data._paths_cache + assert alternate_spelling not in data._paths_cache + + def test_sprint_overview(project): install_bmad_config(project) write_sprint( diff --git a/tests/test_verify.py b/tests/test_verify.py index 86ccb3c6..0999b1c1 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -10,9 +10,11 @@ _FAIL, _OK, MISSING_TOOL_CMD, + UNRESOLVABLE, fault_read_text, git, make_git_noisy, + refuse_to_resolve, spec_path, write_spec, write_sprint, @@ -1458,6 +1460,18 @@ def test_verify_dev_stories_spec_only_change_outside_artifacts_is_not_work(proje assert out2.ok +@pytest.mark.parametrize("refused", ["project", "spec_folder"]) +def test_stories_relpaths_is_empty_when_resolution_is_uncertain(project, monkeypatch, refused): + """An uncertain observation supplies no story excludes. Ablation target: narrow + `_stories_relpaths`' resolution guard back to `ValueError`, and either refusal + row raises instead of returning the documented empty tuple.""" + spec_folder = project.planning_artifacts / "epic-a" + target = project.project if refused == "project" else spec_folder + refuse_to_resolve(monkeypatch, target) + + assert verify._stories_relpaths(project.project, spec_folder) == () + + def test_verify_dev_stories_plan_halt_expects_ready_for_dev(project): # plan-halt leg: the spec is at ready-for-dev (the plan), not done, and there # is NO code change — proof-of-work is skipped and the plan spec is recorded. @@ -1706,6 +1720,118 @@ def test_safe_rollback_prunes_emptied_dirs(project): assert not (repo / "tmpdir").exists() # emptied parent dirs pruned +def test_safe_rollback_resolution_failure_precedes_every_mutation(project, monkeypatch): + """A cleanup target that cannot be canonicalized fails typed before stash, + reset, unlink, or directory pruning, preserving both the checkout and the + original path fault as the diagnostic cause. + + Ablation target: move `_rollback_cleanup_plan` below `stash create` or + `reset --hard`, and this test fails on the recorded destructive git call before + the injected resolution failure; narrow its exception translation and the + `GitError`/cause assertions fail instead. + """ + repo = project.project + baseline = verify.rev_parse_head(repo) + snap = sorted(verify.untracked_files(repo)) + created = repo / "uncertain" / "created.txt" + created.parent.mkdir() + created.write_text("run-created\n") + (repo / "src.txt").write_text("tracked attempt\n") + refuse_to_resolve(monkeypatch, created) + + git_calls: list[tuple[str, ...]] = [] + removals: list[tuple[str, Path]] = [] + real_git = verify._git + real_git_out = verify._git_out + real_unlink = Path.unlink + real_rmdir = Path.rmdir + + def spy_git(r, *args): + git_calls.append(args) + return real_git(r, *args) + + def spy_git_out(r, *args, env=None): + git_calls.append(args) + return real_git_out(r, *args, env=env) + + def spy_unlink(self, *args, **kwargs): + removals.append(("unlink", self)) + return real_unlink(self, *args, **kwargs) + + def spy_rmdir(self, *args, **kwargs): + removals.append(("rmdir", self)) + return real_rmdir(self, *args, **kwargs) + + monkeypatch.setattr(verify, "_git", spy_git) + monkeypatch.setattr(verify, "_git_out", spy_git_out) + monkeypatch.setattr(Path, "unlink", spy_unlink) + monkeypatch.setattr(Path, "rmdir", spy_rmdir) + + with pytest.raises(verify.GitError, match="preflight rollback cleanup") as caught: + verify.safe_rollback(repo, baseline, baseline_untracked=snap) + + assert isinstance(caught.value.__cause__, OSError) + assert UNRESOLVABLE in str(caught.value.__cause__) + assert not any( + args[:2] in {("stash", "create"), ("reset", "--hard")} for args in git_calls + ) # the read-only untracked probe ran, but no git mutation crossed the boundary + assert removals == [] # no unlink/rmdir ran on an uncertain plan + assert created.read_text() == "run-created\n" + assert (repo / "src.txt").read_text() == "tracked attempt\n" + + +def test_safe_rollback_consumes_only_the_precomputed_confined_plan(project, tmp_path, monkeypatch): + """A healthy plan removes and prunes a confined created path, preserves a kept + path and an external symlink target, and performs no second resolution after + `reset --hard` crosses the mutation boundary. + + INVERSE ablation: restore the post-reset `repo.resolve()`/per-target resolve + loop or make `_prune_empty_parents` resolve its start again, and the exact-path + refusal installed by the reset spy makes this test fail before the planned + cleanup is consumed. + """ + repo = project.project + baseline = verify.rev_parse_head(repo) + snap = sorted(verify.untracked_files(repo)) + created = repo / "tmpdir" / "sub" / "created.txt" + created.parent.mkdir(parents=True) + created.write_text("remove me\n") + kept = repo / ".bmad-loop" / "keep.txt" + kept.parent.mkdir() + kept.write_text("keep me\n") + outside = tmp_path / "outside.txt" + outside.write_text("operator data\n") + external_link = repo / "external-link.txt" + external_link.symlink_to(outside) + (repo / "src.txt").write_text("tracked attempt\n") + + real_git = verify._git + + def refuse_after_reset(r, *args): + result = real_git(r, *args) + if args[:2] == ("reset", "--hard"): + refuse_to_resolve( + monkeypatch, + repo, + repo / ".bmad-loop", + created, + created.parent, + external_link, + ) + return result + + monkeypatch.setattr(verify, "_git", refuse_after_reset) + + verify.safe_rollback(repo, baseline, baseline_untracked=snap) + + assert (repo / "src.txt").read_text() == "original\n" + assert not created.exists() + assert not (repo / "tmpdir").exists() # precomputed parents were pruned + assert kept.read_text() == "keep me\n" + assert external_link.is_symlink() # external canonical targets are never removed + assert outside.read_text() == "operator data\n" + + def test_safe_rollback_preserves_tracked_artifact(project): """`preserve` keeps a *tracked* artifact edit (the resolve workflow's corrected spec) alive through the hard reset, while a tracked source edit is still @@ -2538,6 +2664,26 @@ def test_path_ignored_is_false_outside_the_repo(project, tmp_path): assert not verify.path_ignored(project.project, tmp_path / "elsewhere" / "board.yaml") +@pytest.mark.parametrize("refused", ["repo", "candidate"]) +def test_path_ignored_is_false_when_resolution_is_uncertain(project, monkeypatch, refused): + """Resolution uncertainty keeps the path eligible for the exact commit and never + fabricates a lexical git operand. Ablation target: move the repo resolve outside + the guard or narrow it back to `ValueError`, and the matching row raises instead + of returning False before the empty git-call assertion.""" + repo = project.project + candidate = repo / "board.yaml" + refuse_to_resolve(monkeypatch, repo if refused == "repo" else candidate) + git_calls = [] + + def spy_git(*args, **kwargs): + git_calls.append((args, kwargs)) + + monkeypatch.setattr(verify, "_run_git", spy_git) + + assert verify.path_ignored(repo, candidate) is False + assert git_calls == [] + + def test_path_ignored_raises_on_git_failure(project): """Raises GitError like every other probe here. Its caller degrades by keeping the path IN the commit list — uncertainty must not silently drop a write. @@ -2930,6 +3076,89 @@ def test_commit_paths_commits_only_listed(project): assert "src.txt" not in status +def test_commit_paths_repo_resolution_failure_precedes_staging(project, monkeypatch): + """An uncertain repository root is a typed exact-write failure before any + candidate can be staged; it is never converted into an empty successful commit. + + Ablation target: remove the repo-root translation guard and this test fails on + the raw OSError instead of `GitError`; replace it with a lexical/empty fallback + and the no-staging/no-success assertions fail. + """ + repo = project.project + target = repo / "src.txt" + target.write_text("exact write\n") + refuse_to_resolve(monkeypatch, repo) + git_calls: list[tuple[str, ...]] = [] + real_git = verify._git + + def spy_git(r, *args): + git_calls.append(args) + return real_git(r, *args) + + monkeypatch.setattr(verify, "_git", spy_git) + + with pytest.raises(verify.GitError, match="repository root for exact commit") as caught: + verify.commit_paths(repo, "chore: exact", [target]) + + assert isinstance(caught.value.__cause__, OSError) + assert not any(args[:1] == ("add",) for args in git_calls) + assert target.read_text() == "exact write\n" + + +def test_commit_paths_omits_only_an_uncertain_candidate(project, monkeypatch): + """Per-path uncertainty drops that candidate while a healthy sibling retains + its input order and is the only path staged and committed. + + Ablation target: narrow the candidate guard back to `ValueError` and this test + fails on the injected OSError before the healthy sibling can be committed. + """ + repo = project.project + uncertain = repo / "src.txt" + uncertain.write_text("operator edit\n") + healthy = repo / "healthy.txt" + healthy.write_text("commit me\n") + refuse_to_resolve(monkeypatch, uncertain) + + sha = verify.commit_paths(repo, "chore: healthy only", [uncertain, healthy]) + + assert sha is not None + assert git(repo, "show", "--format=", "--name-only", sha).splitlines() == ["healthy.txt"] + status = git(repo, "status", "--porcelain") + assert "src.txt" in status # uncertain path stayed uncommitted + assert "healthy.txt" not in status + + +@pytest.mark.parametrize("error_type", [OSError, RuntimeError]) +def test_commit_paths_raises_when_no_operand_survives_resolution(project, monkeypatch, error_type): + """A sole uncertain candidate is a typed exact-write failure, not a no-op. + + The harvested-deferral carry clears its durable commit-pending latch after a + successful return, so returning ``None`` here would permanently suppress the + retry even though the ledger could still be dirty. Ablation target: remove + the no-operands resolution guard and both rows return ``None`` instead of + raising before staging. + """ + repo = project.project + uncertain = repo / "src.txt" + uncertain.write_text("uncommitted exact write\n") + _refuse_resolution_as(monkeypatch, uncertain, error_type) + git_calls: list[tuple[str, ...]] = [] + real_git = verify._git + + def spy_git(r, *args): + git_calls.append(args) + return real_git(r, *args) + + monkeypatch.setattr(verify, "_git", spy_git) + + with pytest.raises(verify.GitError, match="no exact commit operand remains") as caught: + verify.commit_paths(repo, "chore: exact", [uncertain]) + + assert isinstance(caught.value.__cause__, error_type) + assert not any(args[:1] == ("add",) for args in git_calls) + assert uncertain.read_text() == "uncommitted exact write\n" + + def test_commit_paths_noop_when_unchanged(project): assert verify.commit_paths(project.project, "noop", [project.project / "src.txt"]) is None # a path outside the repo is ignored, not an error @@ -3275,6 +3504,24 @@ def test_verify_dev_exclude_relpaths_includes_latched_restore_patch(project): assert rel not in verify.verify_dev_exclude_relpaths(project, sp) +def test_verify_dev_exclude_relpaths_omits_only_an_uncertain_candidate(project, monkeypatch): + """A refused exclude is dropped while healthy siblings retain order and normalized + relpaths, using the canonical project snapshot without resolving it again. + + ABLATION A1: restore `paths.project.resolve()` and this test raises on the scoped + project-root refusal before producing excludes. ABLATION A2: narrow the candidate + guard back to `ValueError` and it raises on the refused spec instead of omitting it. + """ + refused_spec = spec_path(project, "1-1-a") + patch = project.implementation_artifacts / "attempt.patch" + refuse_to_resolve(monkeypatch, project.project, refused_spec) + + assert verify.verify_dev_exclude_relpaths(project, refused_spec, str(patch)) == ( + "_bmad-output/implementation-artifacts/sprint-status.yaml", + "_bmad-output/implementation-artifacts/attempt.patch", + ) + + def test_verify_dev_latched_restore_patch_is_not_proof_of_work(project): """T4 (patch-restore x #79): the latched patch file is untracked halt residue under the protected artifact dirs — it survives every reset, so counting it @@ -3488,6 +3735,50 @@ def test_spec_within_roots(project, tmp_path): assert verify.spec_within_roots(Path("/etc/passwd"), project) is False +def _refuse_resolution_as(monkeypatch, target: Path, error_type: type[Exception]) -> None: + if error_type is OSError: + refuse_to_resolve(monkeypatch, target) + return + real_resolve = Path.resolve + + def stub(self, strict: bool = False): + if str(self) == str(target): + raise error_type("injected resolution uncertainty") + return real_resolve(self, strict=strict) + + monkeypatch.setattr(Path, "resolve", stub) + + +@pytest.mark.parametrize("error_type", [OSError, RuntimeError]) +def test_spec_within_roots_refuses_uncertain_reported_path( + project, tmp_path, monkeypatch, error_type +): + """A session-reported spec is untrusted when it cannot be resolved. Ablation + target: move the reported-path resolve above the centralized guard and each + error row raises instead of returning the fail-closed False.""" + reported = tmp_path / "reported" / "spec.md" + _refuse_resolution_as(monkeypatch, reported, error_type) + + assert verify.spec_within_roots(reported, project) is False + + +@pytest.mark.parametrize("error_type", [OSError, RuntimeError]) +@pytest.mark.parametrize( + "root_name", + ["project", "output_folder", "implementation_artifacts", "planning_artifacts"], +) +def test_spec_within_roots_refuses_uncertain_trusted_root( + project, tmp_path, monkeypatch, error_type, root_name +): + """Every trusted root must resolve before containment can be trusted. Ablation + target: move trusted-root resolution outside the centralized guard and the + corresponding root/error row raises instead of returning fail-closed False.""" + reported = tmp_path / "outside" / "spec.md" + _refuse_resolution_as(monkeypatch, getattr(project, root_name), error_type) + + assert verify.spec_within_roots(reported, project) is False + + def test_commits_above_empty_at_baseline(project): """HEAD sitting at baseline has no attempt commits to preserve.""" repo = project.project diff --git a/tests/test_verify_worktree.py b/tests/test_verify_worktree.py index a9117df0..5fcd1f22 100644 --- a/tests/test_verify_worktree.py +++ b/tests/test_verify_worktree.py @@ -6,7 +6,7 @@ """ import pytest -from conftest import git, make_git_noisy +from conftest import git, make_git_noisy, refuse_to_resolve from bmad_loop import verify @@ -537,6 +537,32 @@ def test_clean_incoming_collisions_prunes_emptied_dirs(project, tmp_path): assert not (repo / "Assets").exists() # emptied dirs pruned back to root +@pytest.mark.parametrize("refused", ["repo-root", "prune-parent"]) +def test_clean_incoming_collisions_resolution_fault_precedes_deletion( + project, tmp_path, monkeypatch, refused +): + """Repo-root and prune-parent uncertainty propagate as direct filesystem + failures before the incoming untracked path is unlinked. + + Ablation target: move the prune-parent resolve back below `fp.unlink`, and the + `prune-parent` row fails because the injected fault arrives after the leak was + deleted; move repo-root resolution below cleanup and the `repo-root` row fails + for the same destructive-first reason. + """ + repo = project.project + _branch_with(repo, tmp_path, adds={"Assets/Tests/Leak.cs": "branch\n"}) + leak = repo / "Assets" / "Tests" / "Leak.cs" + leak.parent.mkdir(parents=True, exist_ok=True) + leak.write_text("editor leaked\n") + refuse_to_resolve(monkeypatch, repo if refused == "repo-root" else leak.parent) + + with pytest.raises(OSError): + verify.clean_incoming_collisions(repo, "main", "feat") + + assert leak.read_text() == "editor leaked\n" # uncertain cleanup never ran + assert leak.parent.is_dir() # nor did its prune chain start + + def test_clean_incoming_collisions_prune_keeps_dir_holding_a_stray(project, tmp_path): """The directory-prune half of #460's tolerance. A passing `..._tolerates_untracked_stray` does not imply this one: that stray sits at the diff --git a/tests/test_worktree_flow.py b/tests/test_worktree_flow.py index beb82fe4..b2ee8ad3 100644 --- a/tests/test_worktree_flow.py +++ b/tests/test_worktree_flow.py @@ -12,7 +12,7 @@ from types import SimpleNamespace import pytest -from conftest import git +from conftest import git, refuse_to_resolve from bmad_loop import verify from bmad_loop.bmadconfig import ProjectPaths @@ -20,6 +20,12 @@ from bmad_loop.install import provision_worktree as install_provision_worktree from bmad_loop.model import Phase, StoryTask from bmad_loop.policy import GatesPolicy, LimitsPolicy, NotifyPolicy, Policy, ScmPolicy +from bmad_loop.workspace import ( + UnitWorkspace, + Workspace, + open_unit_workspace, + unit_worktrees_dir, +) from bmad_loop.worktree_flow import WorktreeFlow, _setup_mcp_agent_id, provision_worktree QUIET = NotifyPolicy(desktop=False, file=True) @@ -439,6 +445,56 @@ def boom(*a, **k): assert not any(e.startswith("unit-") for e in flow.journal.events()) +def test_mount_resolution_fault_is_typed_and_defers_only_the_unit(tmp_path, monkeypatch): + """An uncertain mount is an ordinary per-unit open failure, not a spawn fault. + + Ablation: delete the mount-resolution translation and the raw provider fault + escapes ``run_isolated`` instead of reaching DEFERRED/worktree-open-failed. + """ + repo = tmp_path / "repo" + repo.mkdir() + paths = ProjectPaths( + project=repo, + implementation_artifacts=repo / "_bmad-output/implementation-artifacts", + planning_artifacts=repo / "_bmad-output/planning-artifacts", + ) + mount = unit_worktrees_dir(tmp_path) / "1-1" + refuse_to_resolve(monkeypatch, mount) + + with pytest.raises(verify.GitError) as excinfo: + open_unit_workspace(repo, paths, "run-1", "1-1", "main", "story", tmp_path) + assert "worktree mount path" in str(excinfo.value) + assert isinstance(excinfo.value.__cause__, OSError) + + state = SimpleNamespace( + target_branch="main", + run_id="run-1", + source="sprint", + tasks={}, + crashed=False, + ) + flow = _make_flow( + tmp_path, + paths=paths, + state=state, + open_unit_workspace=open_unit_workspace, + ) + task = StoryTask(story_key="1-1", epic=1) + drove = [] + + flow.run_isolated(task, lambda candidate: drove.append(candidate)) + + assert task.phase == Phase.DEFERRED + assert task.defer_reason.startswith("could not open worktree") + assert "worktree mount path" in task.defer_reason + assert flow.journal.events() == ["worktree-open-failed"] + assert flow.calls.saves == 1 + assert flow.calls.pauses == [] # ordinary GitError, never machine-wide spawn pause + assert drove == [] + assert state.crashed is False + assert not mount.exists() + + def test_run_isolated_spawn_fault_pauses_instead_of_deferring(tmp_path): """#343: a spawn fault is machine-wide, not this unit's — deferring would march the whole queue into DEFERRED one notification at a time and end the @@ -464,6 +520,72 @@ def boom(*a, **k): assert drove == [] # drive body never ran +def test_run_isolated_escalates_provisioning_root_failure_before_result_probes( + tmp_path, monkeypatch +): + """An opened worktree stays mounted when repair cannot identify its roots. + + Ablation: delete the provisioning ``GitError`` catch in ``run_isolated`` and + this escapes without marking ESCALATED, notifying, saving, or pausing. + """ + import bmad_loop.worktree_flow as worktree_flow + + repo, wt = tmp_path / "repo", tmp_path / "wt" + repo.mkdir() + wt.mkdir() + paths = ProjectPaths( + project=repo, + implementation_artifacts=repo / "_bmad-output/implementation-artifacts", + planning_artifacts=repo / "_bmad-output/planning-artifacts", + ) + unit = UnitWorkspace( + workspace=Workspace(root=wt, paths=paths.rebased(wt)), + repo_root=repo, + branch="bmad-loop/run-1/1-1", + path=wt, + baseline="abc123", + ) + cause = OSError(0, "provider unavailable", None, 64) + + def provisioning_root_failure(*_args, **_kwargs): + raise verify.GitError("cannot resolve worktree provisioning roots safely") from cause + + monkeypatch.setattr(worktree_flow, "provision_worktree", provisioning_root_failure) + for probe in ( + "worktree_seed_undelivered", + "module_skills_seed_undelivered", + "base_skills_seed_incomplete", + ): + monkeypatch.setattr( + worktree_flow, + probe, + lambda *_args, _probe=probe, **_kwargs: pytest.fail( + f"result probe {_probe} ran after provisioning failed" + ), + ) + state = SimpleNamespace(target_branch="main", run_id="run-1", source="sprint", tasks={}) + flow = _make_flow( + tmp_path, + paths=paths, + state=state, + open_unit_workspace=lambda *_args, **_kwargs: unit, + ) + task = StoryTask(story_key="1-1", epic=1) + drove = [] + + with pytest.raises(_Pause) as excinfo: + flow.run_isolated(task, lambda candidate: drove.append(candidate)) + + assert task.phase == Phase.ESCALATED + assert "provisioning root could not be resolved" in excinfo.value.reason + assert flow.journal.events() == ["worktree-opened", "story-escalated"] + assert flow.calls.saves == 1 + assert flow.calls.pauses == [(excinfo.value.reason, "1-1")] + assert drove == [] + assert task.worktree_path == str(wt) + assert wt.is_dir() # retained for inspection; no integration/teardown ran + + def test_escalate_unit_marks_escalated_notifies_and_pauses(tmp_path): flow = _make_flow( tmp_path, state=SimpleNamespace(target_branch="main", run_id="run-9", tasks={})