From d045b6d51c00f125ac0e8b1839e2f7f8614b1e70 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 16 Aug 2026 14:32:22 -0700 Subject: [PATCH 1/6] fix(worktree): guard provisioning path resolution (#556) Observation probes journal coarse unknowns when roots cannot resolve. Per-entry provisioning refuses only the uncertain explicit seed, glob match, or upstream skill while continuing healthy siblings. Provisioning-root uncertainty raises GitError before writes and escalates before result probes or dispatch. Mount uncertainty becomes an ordinary GitError so only the unit defers. Ablation evidence (2026-08-16): removing the provisioning-root translation made T1 fail 2/2 on raw OSError; removing the caller catch made T2 fail on the escaping GitError. Removing the explicit, glob, and upstream-skill guards made T3 fail 2/2 and T4 fail 2/2 plus 1/1 on raw OSError. Removing the arbitrary-seed and module-skill observation fallbacks made T5 fail 2/2 and 1/1 on raw OSError. Removing the traversal guard made T6 fail on the suppressed raw OSError, while forcing suppression in the repair arm made T6 fail with DID NOT RAISE. Removing the mount translation made T7 fail on raw OSError. Each source was restored with cp from an out-of-repo backup, cmp-verified byte-identical, and its named test rerun green. --- src/bmad_loop/install.py | 8 +- src/bmad_loop/workspace.py | 8 +- src/bmad_loop/worktree_flow.py | 83 +++++++++++---- tests/test_install.py | 178 +++++++++++++++++++++++++++++++++ tests/test_worktree_flow.py | 124 ++++++++++++++++++++++- 5 files changed, 380 insertions(+), 21 deletions(-) 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/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..ef1fd0d3 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 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_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={}) From c05b7b82e5529a088a48360e82b3a38cb4a97d7a Mon Sep 17 00:00:00 2001 From: t Date: Sun, 16 Aug 2026 14:44:31 -0700 Subject: [PATCH 2/6] fix(verify): guard observational path resolution (#557) Fail closed for agent-reported spec containment when either the reported path or any trusted root cannot resolve. Deliberately fail open or empty for observations: keep uncertain paths eligible for commit, omit uncertain dev excludes, and return empty stories excludes. Ablation evidence (2026-08-16): restoring the old bare repo resolve and ValueError-only path_ignored guard made `uv run pytest tests/test_verify.py::test_path_ignored_is_false_when_resolution_is_uncertain -q` fail 2 rows with the scoped OSError at the repo and candidate. Restoring `paths.project.resolve()` made `uv run pytest tests/test_verify.py::test_verify_dev_exclude_relpaths_omits_only_an_uncertain_candidate -q` fail once at the canonical project snapshot; separately narrowing the candidate arm to ValueError made the same command fail once at the refused spec. Moving the reported-spec resolve outside the centralized guard made `uv run pytest tests/test_verify.py::test_spec_within_roots_refuses_uncertain_reported_path -q` fail 2 rows with OSError and RuntimeError; moving trusted-root resolution outside made `uv run pytest tests/test_verify.py::test_spec_within_roots_refuses_uncertain_trusted_root -q` fail all 8 root/error rows. Narrowing `_stories_relpaths` back to ValueError made `uv run pytest tests/test_verify.py::test_stories_relpaths_is_empty_when_resolution_is_uncertain -q` fail both project and candidate rows. Each site was restored from the external cp backup, cmp-verified byte-identical, and its named test rerun green before the next ablation. --- src/bmad_loop/verify.py | 31 ++++++++------ tests/test_verify.py | 95 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+), 14 deletions(-) diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 93fd0699..616f3a9c 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -646,10 +646,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 @@ -1591,12 +1591,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 +1609,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 +2016,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}") diff --git a/tests/test_verify.py b/tests/test_verify.py index 86ccb3c6..a3bb12e9 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -13,6 +13,7 @@ fault_read_text, git, make_git_noisy, + refuse_to_resolve, spec_path, write_spec, write_sprint, @@ -1458,6 +1459,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. @@ -2538,6 +2551,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. @@ -3275,6 +3308,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 +3539,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 From af8e3c91730d250de2070194e3c01a3ec78ab64f Mon Sep 17 00:00:00 2001 From: t Date: Sun, 16 Aug 2026 15:05:03 -0700 Subject: [PATCH 3/6] fix(recovery): preflight path resolution before repair (#557) Rollback uncertainty now pauses before destructive git after a complete confined cleanup-plan preflight, with the underlying filesystem fault retained as the typed cause. Collision RuntimeError preserves the environmental keep-branch escalation, and collision prune roots are resolved before deletion. Exact-commit root uncertainty is typed while one uncertain candidate is omitted and healthy siblings retain order. Ablations (2026-08-16): moving rollback preflight below reset failed T1 because stash/reset were observed; restoring post-reset prune resolution failed T2 with the injected OSError; removing the RecoveryFlow catch failed T3 with uncaught RollbackPreflightError; moving collision parent and repo-root resolution below unlink failed both T4 rows with FileNotFoundError after deletion; removing RuntimeError from the merge catch made T5 crash, while removing it from environmental classification made T5 emit dirty-tree guidance; removing exact-commit root translation failed T6 with raw OSError, and narrowing per-path omission to ValueError failed T6 before the healthy commit. Each source was restored with cp, byte-checked with cmp, and its named test reconfirmed green. Verification: 506 passed, 23 skipped across the four required test files; pyright reported 0 errors and 0 warnings; trunk fmt/check reported no issues on all touched paths. --- src/bmad_loop/recovery_flow.py | 27 ++++-- src/bmad_loop/verify.py | 124 +++++++++++++++++++++---- src/bmad_loop/worktree_flow.py | 12 +-- tests/test_engine_worktree.py | 17 ++-- tests/test_recovery_flow.py | 41 +++++++- tests/test_verify.py | 165 +++++++++++++++++++++++++++++++++ tests/test_verify_worktree.py | 28 +++++- 7 files changed, 371 insertions(+), 43 deletions(-) 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/verify.py b/src/bmad_loop/verify.py index 616f3a9c..60b054a2 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], @@ -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 @@ -2550,9 +2631,16 @@ 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.""" rels: list[str] = [] - repo_root = repo.resolve() + 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 @@ -2560,7 +2648,7 @@ 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 ValueError: + except (OSError, RuntimeError, ValueError): continue missing = [r for r in rels if not ((repo_root / r).exists() or (repo_root / r).is_symlink())] if missing: diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index ef1fd0d3..fccea263 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -1516,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..143f92c3 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -3025,25 +3025,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_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_verify.py b/tests/test_verify.py index a3bb12e9..750b2331 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -10,6 +10,7 @@ _FAIL, _OK, MISSING_TOOL_CMD, + UNRESOLVABLE, fault_read_text, git, make_git_noisy, @@ -1719,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 @@ -2963,6 +3076,58 @@ 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 + + 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 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 From 3e1416d79679d699acdafa5918dd7e61d39baf84 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 16 Aug 2026 15:21:16 -0700 Subject: [PATCH 4/6] fix(tui): survive project path resolution faults (#558) The dashboard now starts and polls with unavailable panes under project-root resolution failure. One stable ProjectPaths spelling keys path caches and pending-decision reads, removing the transient second resolve while healthy behavior remains unchanged. Ablations (2026-08-16): - Restoring BmadLoopApp's bare project.resolve() failed the Pilot startup test with the stubbed provider OSError. - Restoring _project_paths' bare project.resolve() failed the recovery test with the stubbed provider OSError. - Keying _paths_cache by the pre-canonical spelling failed the healthy alias test by producing a second ProjectPaths snapshot. - Restoring pending_missed_decisions' second resolve failed the canonical-root test with the stubbed provider OSError. Each source was restored byte-identical from an external cp backup and its named tests returned green before the next ablation. --- src/bmad_loop/tui/app.py | 3 +- src/bmad_loop/tui/data.py | 5 +-- tests/test_tui_app.py | 32 ++++++++++++++++- tests/test_tui_data.py | 76 +++++++++++++++++++++++++++++++++++++-- 4 files changed, 110 insertions(+), 6 deletions(-) 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/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( From acf0c145f042e7400636fff0c277a5f5da5e2349 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 16 Aug 2026 15:33:17 -0700 Subject: [PATCH 5/6] docs(changelog): document resolve guard fixes (#556) Representative ablations (2026-08-16): A1 removed the provisioning-root translation and failed 2/2 typed/no-write rows on raw OSError (1 caller row passed), then separately removed the caller escalation and failed its 1/1 row on an escaping GitError (2 typed rows passed). A2 removed spec_within_roots' fail-closed resolution guard and failed all 10 reported-path/trusted-root rows on OSError or RuntimeError. A3 restored the TUI's second project.resolve() and failed its 1/1 transient-refusal row on the stubbed OSError. Each source was restored with cp, cmp-verified byte-identical, and its named tests rerun green (A1 3 passed; A2 10 passed; A3 1 passed). Full gates: uv run pytest -q -n logical: 5758 passed, 50 skipped; uv run pyright: 0 errors, 0 warnings, 0 informations; trunk check --all: 257 files checked, no issues; git diff --check: clean. --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) 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 From 2053fb5badb43b29f0184586ad1eab5972f34cd4 Mon Sep 17 00:00:00 2001 From: t Date: Sun, 16 Aug 2026 15:58:07 -0700 Subject: [PATCH 6/6] fix(review): address resolve guard findings (#556) --- src/bmad_loop/engine.py | 13 ++++++++++--- src/bmad_loop/verify.py | 16 ++++++++++++++-- tests/test_engine_worktree.py | 35 +++++++++++++++++++++++++++++++++++ tests/test_verify.py | 31 +++++++++++++++++++++++++++++++ 4 files changed, 90 insertions(+), 5 deletions(-) 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/verify.py b/src/bmad_loop/verify.py index 60b054a2..b213e2da 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -2633,8 +2633,10 @@ def commit_paths(repo: Path, message: str, paths: list[Path]) -> str | None: was never committed). A missing-but-TRACKED path stays in: that is a 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.""" + contract for healthy siblings. If no usable operand survives that uncertainty, + the call raises instead of reporting a successful no-op.""" rels: list[str] = [] + resolution_fault: tuple[Path, OSError | RuntimeError] | None = None try: repo_root = repo.resolve() except (OSError, RuntimeError) as e: @@ -2648,7 +2650,11 @@ 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, ValueError): + 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())] if missing: @@ -2658,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/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index 143f92c3..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, []) diff --git a/tests/test_verify.py b/tests/test_verify.py index 750b2331..0999b1c1 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -3128,6 +3128,37 @@ def test_commit_paths_omits_only_an_uncertain_candidate(project, monkeypatch): 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