diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 844c4ca4..46416058 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -68,7 +68,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Off by default (`[scm] isolation = "none"` — work in place on the checked-out branch, byte-for-byte the prior behavior). Set `isolation = "worktree"` and each story (and each sweep bundle) runs in its own `git worktree` on a `bmad-loop/[/]` branch cut from the target branch, then merges back **locally** — the main checkout stays free while a run is in flight. - Merge knobs: `merge_strategy` (`ff` / `merge` / `squash`), `target_branch` (default = branch checked out at run start; created if missing — a detached HEAD or unborn repo pauses the run instead of merging onto an unreferenced commit), `branch_per` (`story` or a shared `run` branch; `run` forces `delete_branch = false`), and `delete_branch`. - Failed-unit forensics: a deferred/escalated unit's worktree + branch stay mounted (`keep_failed`, default on) and its full diff is preserved to `run_dir/failed//changes.patch`; `failed_diff_max_mb` caps per-file untracked-file size (oversized skipped with a marker), `failed_diff_unlimited` lifts the cap. -- Config seeding: a worktree checks out _tracked_ files only, so a project's gitignored MCP/CLI configs (`.mcp.json`, `.claude/settings.json`, `.codex/config.toml`, `.gemini/settings.json`) would be missing — an isolated session couldn't reach its MCP server. With `seed_adapter_defaults` (default on) each loaded adapter's own `seed_files` are copied in from the main repo before the session launches; `worktree_seed` adds extra paths. Copy-when-absent, seeded before the hook-merge (a seeded `settings.json` keeps its content and just gains the Stop hook), and shielded from the unit's `git add -A`. +- Config seeding: a worktree checks out _tracked_ files only, so a project's gitignored MCP/CLI configs (`.mcp.json`, `.claude/settings.json`, `.codex/config.toml`, `.gemini/settings.json`) would be missing — an isolated session couldn't reach its MCP server. With `seed_adapter_defaults` (default on) each loaded adapter's own `seed_files` are copied in from the main repo before the session launches; `worktree_seed` adds extra paths. Copy-when-absent at file granularity — a directory entry whose destination already exists (a worktree checkout carries its tracked children) still seeds the children that are missing — seeded before the hook-merge (a seeded `settings.json` keeps its content and just gains the Stop hook), and shielded from the unit's `git add -A`. - Run state never moves into a worktree — `.bmad-loop/` always lives in the main repo; spec paths are persisted relative to the worktree so a kept-failed run stays portable. - Merge-back is serialized; `max_parallel` is a validated knob clamped to `1` until parallel fan-out is built. The `repo_root` key in `_bmad/bmm/config.yaml` (defaults to the project dir) decouples where git/code work happens from where run state lives (monorepos). - `commit_message_template` (`{story_key}` / `{run_id}` substituted) customizes story/bundle commit messages. diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index 073c8f92..cfd25b0a 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -384,23 +384,49 @@ def _register_hooks(project: Path, profile: CLIProfile) -> int: return 0 -def _copy_traversable(src, dst: Path) -> None: +def _copy_traversable(src, dst: Path, *, skip_existing: bool = False) -> bool: """Recursively copy a packaged resource tree to a filesystem path. Walks via the Traversable API (.iterdir/.read_bytes) rather than resolving a filesystem path, so it works even when the package is zip-imported. + + ``skip_existing`` makes the copy no-clobber at FILE granularity: an existing + destination file is left untouched and its siblings are still copied — even + when it stands where the source has a directory (that subtree is skipped + whole rather than mkdir'd over the file). It is + opt-in because the `--force-skills` path (`install_into`) rmtree's the + destination precisely to overwrite it, and a guard baked into this helper + would silently regress that. Only the worktree-seed caller passes it. + + Returns whether anything was actually written, so a caller seeding into an + existing directory can tell a partial seed (something landed) from a total + no-op (every child was already present). Every other call site ignores it. """ if src.is_dir(): + if skip_existing and dst.exists() and not dst.is_dir(): + # a FILE sits where the source has a directory: mkdir would raise + # FileExistsError. Under the no-clobber contract the file wins and + # the whole subtree is skipped, like any existing destination. + return False + # creating a missing directory IS a write: an empty dir seeded into an + # existing tree must count, or the entry is misreported as a total no-op. + copied = not dst.exists() dst.mkdir(parents=True, exist_ok=True) + # `any(...)` would short-circuit and skip the remaining children. for child in src.iterdir(): - _copy_traversable(child, dst / child.name) - elif isinstance(src, Path): + if _copy_traversable(child, dst / child.name, skip_existing=skip_existing): + copied = True + return copied + if skip_existing and dst.exists(): + return False + if isinstance(src, Path): # real filesystem source (worktree seeds, non-zip package data): copy2 # preserves the mode so a seeded vendor/bin/* keeps +x (issue #126) shutil.copy2(src, dst) else: # zip-imported Traversable exposes no stat: content-only copy dst.write_bytes(src.read_bytes()) + return True def _worktree_local_exclude(worktree: Path, patterns: Sequence[str]) -> None: diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index 8592f692..5b999a5b 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -91,9 +91,15 @@ def provision_worktree( to pull its MCP-generated skill tree (gitignored, so absent from the checkout) into a per_worktree Editor's checkout. + A `seed_files` entry naming a DIRECTORY whose destination already exists is + seeded child by child: the children the checkout lacks are copied in, the ones + it carries are left untouched. A worktree checks out tracked files, so such a + dir always exists and the entry would otherwise be a total no-op (issue #230). + Kept safe against the unit's eventual `git add -A` commit: - - skills + seed files are copied only when ABSENT, so a project that commits its - own skill tree (e.g. .agents/) or config keeps it untouched (no diff merged back); + - skills + seed files are copied only when ABSENT — at FILE granularity, so a + project that commits its own skill tree (e.g. .agents/) or config keeps it + untouched (no diff merged back); - the hook points at the MAIN repo's already-installed relay via an absolute path (the relay locates the run dir from $BMAD_LOOP_RUN_DIR, not its own location), so nothing is written into the worktree's .bmad-loop/; @@ -105,10 +111,11 @@ def provision_worktree( also a hook config_path (.claude/settings.json, .gemini/settings.json) keeps its real content and just gets the Stop hook merged in, rather than being created empty. - Returns the `seed_files` entries that existed in the repo but were skipped - because the destination already existed — copy-when-absent turned them into - no-ops. The caller journals them: a user-authored `worktree_seed` entry that - silently copies nothing reads as applied configuration and is not. + Returns the `seed_files` entries that copied NOTHING because everything they + name was already present — copy-when-absent turned them into no-ops. The caller + journals them: a user-authored `worktree_seed` entry that silently copies + nothing reads as applied configuration and is not. A directory entry that + seeded even one child is not reported: it is applied configuration. """ if not profiles and not seed_files and not seed_globs: return [] @@ -120,14 +127,13 @@ def provision_worktree( # project gitignored MCP/CLI configs: copy from the main repo when absent. # Resolve-and-contain guards against an `..`/absolute entry escaping either tree. seeded: list[str] = [] - # Entries that named a real source but whose destination already exists, so - # copy-when-absent made them a no-op. Reported to the caller (this function is - # quiet by contract — it runs under a TUI) because for a DIRECTORY entry the - # no-op is silent and total: a worktree checks out tracked files, so a seed dir - # with any tracked child always exists and the whole entry is skipped, including - # the children that are absent and would clobber nothing. Glob-expanded matches - # are deliberately not reported: a plugin's glob is expected to hit paths the - # checkout already carries, so that skip is routine rather than a misconfiguration. + # Entries that named a real source but copied nothing, because every path they + # name already exists. Reported to the caller (this function is quiet by + # contract — it runs under a TUI) because the no-op is otherwise silent: an + # entry that reads as applied configuration is not. Per-CHILD skips inside a + # directory entry are deliberately not reported — the checkout is expected to + # carry its tracked children, so that is routine rather than a + # misconfiguration, exactly like the glob-expanded matches below. skipped: list[str] = [] for rel in seed_files: src = (repo_root / rel).resolve() @@ -137,7 +143,27 @@ def provision_worktree( if not src.exists(): continue if dst.exists(): - skipped.append(str(rel)) + # File entries keep the classic copy-when-absent skip. A destination + # that is not a directory while the source is (the checkout carries a + # FILE where the seed names a dir) is a type mismatch: recursing would + # try to mkdir over the file, so the entry is skipped whole instead. + if not src.is_dir() or not dst.is_dir(): + skipped.append(str(rel)) + continue + # A DIRECTORY whose destination exists is the case #230 reported: the + # checkout carries some tracked child, so the whole entry used to be a + # no-op — including the gitignored children that are absent and would + # clobber nothing. Recurse instead, copying only what is missing. + if not _copy_traversable(src, dst, skip_existing=True): + # every child was already present: still a total no-op, still + # reported. Only a PARTIAL seed stops being reported. + skipped.append(str(rel)) + continue + # Partially seeded, so the entry must still reach `patterns` below: + # the children we just wrote have to stay out of the unit's + # `git add -A`. Excluding the whole dir is safe — an exclude does not + # untrack the tracked children that were already there. + seeded.append(rel) continue dst.parent.mkdir(parents=True, exist_ok=True) _copy_traversable(src, dst) diff --git a/tests/test_install.py b/tests/test_install.py index 686e2e56..97b1f1a5 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -816,18 +816,93 @@ def test_provision_worktree_reports_seed_skipped_as_noop(tmp_path): assert provision_worktree(wt, [], repo, seed_files=[".mcp.json"]) == [".mcp.json"] -def test_provision_worktree_reports_seed_dir_skipped_whole(tmp_path): - """The case that motivated the report: a worktree checks out tracked files, so a - seed DIRECTORY with any tracked child already exists and the whole entry is - skipped — including children that are absent and would clobber nothing.""" +def test_provision_worktree_seeds_absent_children_of_existing_dir(tmp_path): + """The case that motivated #230: a worktree checks out tracked files, so a seed + DIRECTORY with any tracked child already exists. Its absent children are seeded + anyway (they clobber nothing), and the entry is no longer reported as a no-op.""" wt, repo = tmp_path / "wt", tmp_path / "repo" (repo / "_bmad" / "custom").mkdir(parents=True) # tracked child (repo / "_bmad" / "bmm").mkdir() # gitignored sibling, absent from the checkout (repo / "_bmad" / "bmm" / "config.yaml").write_text("SEED ME", encoding="utf-8") (wt / "_bmad" / "custom").mkdir(parents=True) # what `git worktree add` lays down - assert provision_worktree(wt, [], repo, seed_files=["_bmad"]) == ["_bmad"] - assert not (wt / "_bmad" / "bmm").exists() # documents today's behaviour + assert provision_worktree(wt, [], repo, seed_files=["_bmad"]) == [] + assert (wt / "_bmad" / "bmm" / "config.yaml").read_text() == "SEED ME" + + +def test_provision_worktree_seed_dir_does_not_clobber_existing_children(tmp_path): + """Seeding into an existing dir stays no-clobber at FILE granularity: a child the + checkout carries keeps its content while its absent siblings are copied in.""" + wt, repo = tmp_path / "wt", tmp_path / "repo" + (repo / "cfg").mkdir(parents=True) + (repo / "cfg" / "tracked.yaml").write_text("FROM_REPO", encoding="utf-8") + (repo / "cfg" / "ignored.yaml").write_text("SEED ME", encoding="utf-8") + (repo / "cfg" / "nested").mkdir() + (repo / "cfg" / "nested" / "deep.yaml").write_text("SEED ME TOO", encoding="utf-8") + (wt / "cfg").mkdir(parents=True) + (wt / "cfg" / "tracked.yaml").write_text("IN_WORKTREE", encoding="utf-8") + + assert provision_worktree(wt, [], repo, seed_files=["cfg"]) == [] + assert (wt / "cfg" / "tracked.yaml").read_text() == "IN_WORKTREE" # untouched + assert (wt / "cfg" / "ignored.yaml").read_text() == "SEED ME" + assert (wt / "cfg" / "nested" / "deep.yaml").read_text() == "SEED ME TOO" + + +def test_provision_worktree_reports_seed_dir_with_nothing_to_copy(tmp_path): + """A directory entry whose children ALL already exist copied nothing, so it is + still a silent no-op and still reported — only a partial seed stops being.""" + wt, repo = tmp_path / "wt", tmp_path / "repo" + (repo / "cfg").mkdir(parents=True) + (repo / "cfg" / "tracked.yaml").write_text("FROM_REPO", encoding="utf-8") + (wt / "cfg").mkdir(parents=True) + (wt / "cfg" / "tracked.yaml").write_text("IN_WORKTREE", encoding="utf-8") + + assert provision_worktree(wt, [], repo, seed_files=["cfg"]) == ["cfg"] + assert (wt / "cfg" / "tracked.yaml").read_text() == "IN_WORKTREE" + + +def test_provision_worktree_seed_dir_over_existing_file_is_skipped(tmp_path): + """A directory entry whose destination is a FILE is a type mismatch: recursing + would mkdir over the file. The file wins (no-clobber) and the entry is reported + skipped like any other whose destination already exists.""" + wt, repo = tmp_path / "wt", tmp_path / "repo" + (repo / "cfg").mkdir(parents=True) + (repo / "cfg" / "ignored.yaml").write_text("SEED ME", encoding="utf-8") + wt.mkdir() + (wt / "cfg").write_text("A FILE, NOT A DIR", encoding="utf-8") + + assert provision_worktree(wt, [], repo, seed_files=["cfg"]) == ["cfg"] + assert (wt / "cfg").read_text() == "A FILE, NOT A DIR" # untouched + + +def test_provision_worktree_seed_skips_nested_file_typed_as_dir(tmp_path): + """The same type mismatch one level down: a child that is a dir in the repo but + a FILE in the checkout is skipped whole (never mkdir'd over), while its absent + siblings still seed — so the entry counts as applied.""" + wt, repo = tmp_path / "wt", tmp_path / "repo" + (repo / "cfg" / "sub").mkdir(parents=True) + (repo / "cfg" / "sub" / "deep.yaml").write_text("SEED ME", encoding="utf-8") + (repo / "cfg" / "ignored.yaml").write_text("SEED ME TOO", encoding="utf-8") + (wt / "cfg").mkdir(parents=True) + (wt / "cfg" / "sub").write_text("A FILE, NOT A DIR", encoding="utf-8") + + assert provision_worktree(wt, [], repo, seed_files=["cfg"]) == [] + assert (wt / "cfg" / "sub").read_text() == "A FILE, NOT A DIR" # untouched + assert (wt / "cfg" / "ignored.yaml").read_text() == "SEED ME TOO" + + +def test_provision_worktree_seeds_absent_empty_child_dir(tmp_path): + """Creating a missing EMPTY child directory is a write: the entry modified the + worktree, so it is treated as applied rather than reported as a no-op.""" + wt, repo = tmp_path / "wt", tmp_path / "repo" + (repo / "cfg" / "empty").mkdir(parents=True) + (repo / "cfg" / "tracked.yaml").write_text("FROM_REPO", encoding="utf-8") + (wt / "cfg").mkdir(parents=True) + (wt / "cfg" / "tracked.yaml").write_text("IN_WORKTREE", encoding="utf-8") + + assert provision_worktree(wt, [], repo, seed_files=["cfg"]) == [] + assert (wt / "cfg" / "empty").is_dir() + assert (wt / "cfg" / "tracked.yaml").read_text() == "IN_WORKTREE" # untouched def test_provision_worktree_reports_nothing_when_seeding_succeeds(tmp_path): @@ -883,6 +958,27 @@ def test_provision_worktree_seed_shielded_in_local_exclude(project, tmp_path): assert "/.mcp.json" in exclude.splitlines() +def test_provision_worktree_partial_seed_dir_shielded_in_local_exclude(project, tmp_path): + """A directory entry that only PARTIALLY seeds (its destination already existed) + still gets its exclude pattern written — otherwise the children just seeded would + be staged by the unit's `git add -A`.""" + repo = project.project + (repo / "cfg").mkdir(exist_ok=True) + (repo / "cfg" / "tracked.yaml").write_text("FROM_REPO", encoding="utf-8") + (repo / "cfg" / "ignored.yaml").write_text("SEED ME", encoding="utf-8") + wt = tmp_path / "wt" + verify.worktree_add(repo, wt, "feat", "main") + # what the checkout lays down: the tracked child, so the seed dir already exists + (wt / "cfg").mkdir(parents=True) + (wt / "cfg" / "tracked.yaml").write_text("FROM_REPO", encoding="utf-8") + + provision_worktree(wt, [get_profile("claude")], repo, seed_files=["cfg"]) + + assert (wt / "cfg" / "ignored.yaml").read_text() == "SEED ME" + exclude = (repo / ".git" / "info" / "exclude").read_text(encoding="utf-8") + assert "/cfg" in exclude.splitlines() + + # ----------------------------------------------------------------- hookless profiles @@ -1027,3 +1123,46 @@ def test_copy_traversable_zip_source_copies_content(tmp_path): _copy_traversable(zipfile.Path(zf_path, "pkg/"), dst) assert (dst / "skill" / "SKILL.md").read_text() == "tool" + + +def test_copy_traversable_skip_existing_holds_on_zip_source(tmp_path): + """`skip_existing` guards the zip-import branch too, not just the copy2 one: + an existing destination file survives while its absent sibling is written.""" + zf_path = tmp_path / "pkg.zip" + with zipfile.ZipFile(zf_path, "w") as zf: + zf.writestr("pkg/skill/SKILL.md", "FROM_ZIP") + zf.writestr("pkg/skill/EXTRA.md", "FROM_ZIP") + dst = tmp_path / "out" + (dst / "skill").mkdir(parents=True) + (dst / "skill" / "SKILL.md").write_text("ON_DISK", encoding="utf-8") + + assert _copy_traversable(zipfile.Path(zf_path, "pkg/"), dst, skip_existing=True) is True + + assert (dst / "skill" / "SKILL.md").read_text() == "ON_DISK" # untouched + assert (dst / "skill" / "EXTRA.md").read_text() == "FROM_ZIP" + + +def test_copy_traversable_skip_existing_reports_total_noop(tmp_path): + """Nothing left to copy -> False, which is how a seed entry that copied nothing + is still told apart from one that partially seeded.""" + src, dst = tmp_path / "src", tmp_path / "dst" + (src / "d").mkdir(parents=True) + (src / "d" / "f.txt").write_text("FROM_SRC", encoding="utf-8") + (dst / "d").mkdir(parents=True) + (dst / "d" / "f.txt").write_text("ON_DISK", encoding="utf-8") + + assert _copy_traversable(src, dst, skip_existing=True) is False + assert (dst / "d" / "f.txt").read_text() == "ON_DISK" + + +def test_copy_traversable_skip_existing_never_mkdirs_over_file(tmp_path): + """A destination FILE standing where the source has a directory is left alone: + without the guard, mkdir(exist_ok=True) on the file raises FileExistsError.""" + src, dst = tmp_path / "src", tmp_path / "dst" + (src / "d").mkdir(parents=True) + (src / "d" / "f.txt").write_text("FROM_SRC", encoding="utf-8") + dst.mkdir() + (dst / "d").write_text("A FILE, NOT A DIR", encoding="utf-8") + + assert _copy_traversable(src, dst, skip_existing=True) is False + assert (dst / "d").read_text() == "A FILE, NOT A DIR"