From 0e4814367dfa52620e0e3fb2f4f8cbb5ac935072 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 29 Jul 2026 09:18:24 -0700 Subject: [PATCH 1/5] feat(validate): warn on legacy shared-exclude pollution (#384) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #384 fix stops the worktree git-add shield from writing the repository-wide `.git/info/exclude`, but nothing removes what older versions already wrote there. Those lines name paths projects legitimately track (`.claude/skills`, `.claude/settings.json`), so in an already-provisioned repo every NEW file under them stays invisible to `git status` and `git add -A` in the operator's own checkout — the tracked files keep diffing normally, which is what made it invisible. `bmad-loop validate` now reports that residue under `git.exclude-legacy-pollution`, naming the file, the exact lines, the symptom and the manual remedy. Warn only, never repair. `.git/info/exclude` is hand-editable and its lines carry no authorship: a project that wants `/.claude/skills` ignored writes the identical line, and after the fact the two are indistinguishable. So the candidate set is rebuilt exactly as `provision_worktree` builds it and only byte-identical lines count — no prefix, substring or containment matching, which would flag an operator's own rule and tell them to delete it. Both negative tests were verified by ablation (widening the match to containment fails them); the absent-exclude test was verified against removing the OSError catch. Detection is silent on every fault — not a repo, git missing or too old, an unreadable exclude — because a repo whose exclude cannot be read is not thereby a repo with a problem to report. --- CHANGELOG.md | 6 +++ docs/FEATURES.md | 1 + src/bmad_loop/checks.py | 1 + src/bmad_loop/cli.py | 26 +++++++++- src/bmad_loop/install.py | 82 +++++++++++++++++++++++++++++++ tests/test_cli.py | 72 +++++++++++++++++++++++++++ tests/test_install.py | 103 +++++++++++++++++++++++++++++++++++++++ 7 files changed, 290 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 06df8972..f09f8a0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,12 @@ breaking changes may land in a minor release. ### Added +- **`validate` warns when an old bmad-loop left shield patterns in `.git/info/exclude` (#384).** + The writer is fixed, but nothing removes the lines already there — and they hide every new file + under paths like `.claude/skills` from `git add -A` in that checkout. `git.exclude-legacy-pollution` + names the file and the exact lines. Warn only, never auto-removed: the same line could be the + project's own, so only exact matches are reported and deleting them stays a manual call. + - **A park travels with its story's commit, so `bmad-loop confirm` works from any clone (#356).** Each parked story now writes one committed JSON record to `.bmad-loop/operator/.json`, inside the story's own commit window, so the record rides the park's commit — through the worktree diff --git a/docs/FEATURES.md b/docs/FEATURES.md index c9d9ce2c..ba03c613 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -183,6 +183,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - `bmad-loop init` installs the three `bmad-loop-*` skills (`bmad-loop-setup`, `bmad-loop-resolve`, `bmad-loop-sweep`, into `.claude/skills/` and/or `.agents/skills/`), the hook relay, `.bmad-loop/policy.toml`, and a gitignore covering the runs dir, plugin caches, and policy.toml itself (per-machine config). Flags: `--cli` (repeatable), `--no-skills`, `--force-skills`. - `bmad-loop validate` preflights every prerequisite: BMAD config, sprint-status, git, the selected terminal-multiplexer backend (listing all detected when more than one is registered), CLI binary, hook registration, and the review skills the installed `bmad-dev-auto` actually invokes — derived from its `customize.toml` review layers (or from `step-04-review.md` on releases that name reviewers inline), so both the merged `bmad-review` topology and the standalone-hunter one validate, and configured layers naming an uninstalled skill are caught — plus its `customize.toml`. +- `validate` also warns (`git.exclude-legacy-pollution`) when the repository-wide `.git/info/exclude` still carries git-add shield patterns a pre-#384 bmad-loop appended to it. Those lines name paths projects legitimately track, so every **new** file under them stays invisible to `git status` and `git add -A` in that checkout until they are removed. The warning names the file and the exact lines and stops there — it never edits the file, because a line like `/.claude/skills` is indistinguishable from one the project wrote for itself, and only exact matches against the shield's own pattern set are reported. - Non-invasive: drives the upstream `bmad-dev-auto` skill unmodified — there is no fork to keep in sync — and review is just a re-invocation of it on the `done` spec. Your standard BMAD install is never modified. ### Command reference diff --git a/src/bmad_loop/checks.py b/src/bmad_loop/checks.py index bcc8a5e1..552904e6 100644 --- a/src/bmad_loop/checks.py +++ b/src/bmad_loop/checks.py @@ -56,6 +56,7 @@ "queue.stories-manifest", "git.worktree-clean", "git.probe", + "git.exclude-legacy-pollution", "hooks.config-parse", "hooks.registered", "mux.backend", diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 1afd313f..6a854c7d 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -185,7 +185,7 @@ def _reconcile_stale(project: Path, paths: bmadconfig.ProjectPaths, pol) -> None def cmd_validate(args: argparse.Namespace) -> int: - from .install import relay_registered + from .install import legacy_exclude_pollution, relay_registered project = _project(args) report = ValidationReport() @@ -267,6 +267,30 @@ def cmd_validate(args: argparse.Namespace) -> int: except verify.GitError as e: report.fail("git.probe", f"git check failed: {e}") + # #384: an older bmad-loop wrote the worktree git-add shield's patterns into the + # repository-wide .git/info/exclude. The writer is fixed and never touches that + # file again, but nothing removes the lines it already wrote — and they name + # paths projects TRACK, so new files under them stay unstageable in this + # checkout until someone deletes them. Warn only, never repair: the file is + # hand-editable and its lines carry no authorship (see the helper's docstring). + seed_rels: list[str] = [] + if pol is not None: + if pol.scm.seed_adapter_defaults: + for profile in profiles: + seed_rels.extend(profile.seed_files) + seed_rels.extend(pol.scm.worktree_seed) + pollution = legacy_exclude_pollution(project, profiles, seed_rels) + if pollution is not None: + report.warn( + "git.exclude-legacy-pollution", + f"{pollution.path} carries git-add shield patterns written by an older " + f"bmad-loop: {', '.join(pollution.lines)} — new files under those paths never " + f"appear in `git status` or `git add -A` in this checkout, however long ago the " + f"run that wrote them finished. bmad-loop no longer writes this file (#384); " + f"review those lines and delete them by hand if your project tracks those paths.", + {"exclude": str(pollution.path), "lines": pollution.lines}, + ) + report.extend(_platform_preflight()) # #231: notify.desktop defaults to true but only fires when a platform notifier diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index 4024075c..fffff037 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -1012,6 +1012,88 @@ def _worktree_local_exclude(worktree: Path, patterns: Sequence[str]) -> str | No return None +class LegacyExcludePollution(NamedTuple): + """Where the repository-wide exclude is, and which shield patterns it carries. + + Returned only when `lines` is non-empty, so a caller that has one of these has + something to report and a file to name in the report. + """ + + path: Path + lines: list[str] + + +def legacy_exclude_pollution( + project: Path, profiles: Sequence[CLIProfile], seed_rels: Sequence[str] +) -> LegacyExcludePollution | None: + """Git-add shield patterns an OLDER bmad-loop left in `.git/info/exclude`. + + Read-only detection for the residue of #384. Until that fix, the worktree + shield appended its patterns to the repository-wide exclude — shared with the + main checkout, permanent, and unversioned. `_worktree_local_exclude` no longer + writes that file at all, but nothing removes what it already wrote, and the + patterns name paths projects legitimately TRACK (`.claude/skills`, + `.claude/settings.json`). So in a repo provisioned by an older version the + tracked files keep diffing normally while every NEW file under those paths + stays invisible to `git status` and `git add -A` — the reporter lost 51 files + and three whole skills that way, across two commits, with nothing in either + diff able to reveal the cause. + + Detection only, never repair, and that is a decision rather than an omission: + `.git/info/exclude` is a hand-editable file whose lines carry no authorship. A + project that genuinely wants `/.claude/skills` ignored writes the identical + line, and there is no way to tell the two apart after the fact — so removing + one is a change only the operator can be sure is right. `validate` warns and + names the lines; deleting them is theirs. + + EXACT matches only, for the same reason. The candidate set is rebuilt the way + `provision_worktree` builds it, and a line counts only if it is byte-identical + to a pattern that set contains. No prefix, substring or path-containment + matching: `/.claude/skills-of-my-own` and a bare `.claude/skills` are the + operator's lines, not ours, and a preflight that flagged them would be telling + someone to delete their own gitignore rules. + + `--git-path info/exclude` names git's ONE shared exclude from either vantage: + a main checkout answers relative (`.git/info/exclude`), a linked worktree + answers with the MAIN repo's absolute path (verified, git 2.55) — which is the + same file, and the file #384 is about. Nothing here reads the private + per-worktree exclude the current shield writes; that one is supposed to carry + these patterns. + + Silent on every fault — not a repo, git missing or too old, an unreadable or + undecodable exclude — because this is preflight observation, and a repo whose + exclude cannot be read is not thereby a repo with a problem to report. + """ + # Mirror provision_worktree's pattern build (worktree_flow.py). Kept in step by + # construction rather than by a shared constant: the shield's set is derived + # per-run from what it actually wrote, and this one is a superset guess about + # runs that are long over. + candidates = {f"/{p.skill_tree}" for p in profiles} + candidates |= {f"/{p.hooks.config_path}" for p in profiles if not p.hookless} + candidates |= {f"/{rel}" for rel in seed_rels} + # Unconditional, unlike the writer's `if customize_seeded`: whether some past + # run seeded _bmad/custom is not knowable from here, and the pattern is ours + # either way. + candidates.add(f"/{CUSTOMIZE_DIR.as_posix()}") + try: + answered = _shield_git(project, "rev-parse", "--git-path", "info/exclude") + if answered.returncode != 0: + return None # not a repo, or a git too old to answer + # fsdecode for the same reason _worktree_local_exclude uses it: a non-UTF-8 + # repo path must round-trip back to the filesystem rather than fault. + exclude = Path(os.fsdecode(answered.stdout).strip()) + if not exclude.is_absolute(): + exclude = project / exclude + present = exclude.read_text(encoding="utf-8").splitlines() + except (OSError, UnicodeError, subprocess.SubprocessError): + return None + # Set intersection IS the exact-match rule — no normalization, no strip(): a + # leading space is a meaningful part of a gitignore pattern, so a line that + # carries one is not a line this code wrote. + hits = sorted(candidates.intersection(present)) + return LegacyExcludePollution(exclude, hits) if hits else None + + def _copy_skills(project: Path, trees: Sequence[str], force: bool) -> bool: """Install the bundled bmad-loop-* skills into each project skill tree. diff --git a/tests/test_cli.py b/tests/test_cli.py index c1d2a3e4..21550b23 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4162,6 +4162,78 @@ def test_dry_run_stories_relativizes_absolute_folder(project, capsys): assert f"Spec folder: {abs_folder}" not in out # not the raw absolute path +# ------------- #384: legacy shared-exclude pollution is a validate warning ----- + + +def _pollute_exclude(project, *lines: str): + """Append `lines` to the repository-wide exclude, the way a pre-#384 run did. + + Called AFTER the fixture's own commit: these patterns name paths the fixture + tracks, and an exclude in place earlier would change what `git add -A` stages + — which is the very bug, and would make the fixture, not the gate, the thing + under test.""" + exclude = project.project / ".git" / "info" / "exclude" + exclude.parent.mkdir(parents=True, exist_ok=True) + with exclude.open("a", encoding="utf-8") as fh: + fh.write("".join(f"{line}\n" for line in lines)) + return exclude + + +def test_validate_warns_on_legacy_shared_exclude_pollution(project, capsys, monkeypatch): + """#384's residue: lines an older bmad-loop left in `.git/info/exclude` hide + every NEW file under those paths from this checkout's `git add -A`, forever. + validate names the file and the exact lines.""" + _make_validate_pass(project, monkeypatch, capsys) + exclude = _pollute_exclude(project, "/.claude/skills", "/_bmad/custom") + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys) + + warned = [f for f in doc["findings"] if f["check"] == "git.exclude-legacy-pollution"] + assert len(warned) == 1, "the pollution gate fires exactly once" + assert warned[0]["severity"] == "warning" + assert warned[0]["detail"]["lines"] == ["/.claude/skills", "/_bmad/custom"] + assert warned[0]["detail"]["exclude"] == str(exclude) + # a warning is not a verdict: the operator is told, not blocked + assert doc["ok"] is True + + +def test_validate_pollution_warning_prints_the_lines_in_text_mode(project, capsys, monkeypatch): + """The remedy is manual, so the human-readable output has to carry enough to + act on: which file, which lines, and what the symptom looks like.""" + _make_validate_pass(project, monkeypatch, capsys) + _pollute_exclude(project, "/.claude/skills") + + cli.main(["validate", "--project", str(project.project)]) + + text = _validate_output(capsys) + assert "/.claude/skills" in text + assert "info/exclude" in text + assert "git add -a" in text # the symptom, lowercased by _validate_output + assert "delete them by hand" in text + + +def test_validate_does_not_flag_an_operator_authored_exclude_line(project, capsys, monkeypatch): + """A line for any path that is not a shield pattern is the operator's own, and + telling them to delete it would be wrong. Exact match only — never substring + or containment (ablation: widen the match and this fails).""" + _make_validate_pass(project, monkeypatch, capsys) + _pollute_exclude(project, "/scratch", "/.claude/skills-mine", ".claude/skills") + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys) + + assert not [f for f in doc["findings"] if f["check"] == "git.exclude-legacy-pollution"] + + +def test_validate_silent_when_the_exclude_file_is_absent(project, capsys, monkeypatch): + """A repo never provisioned by an old bmad-loop gets no warning at all.""" + _make_validate_pass(project, monkeypatch, capsys) + (project.project / ".git" / "info" / "exclude").unlink(missing_ok=True) + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys) + + assert not [f for f in doc["findings"] if f["check"] == "git.exclude-legacy-pollution"] + + # --------------- `bmad-loop mux`: backend listing + persisted choice (issue #87) ---- diff --git a/tests/test_install.py b/tests/test_install.py index 54b09e58..390330a1 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -21,6 +21,7 @@ _copy_traversable, _worktree_local_exclude, install_into, + legacy_exclude_pollution, merge_hooks, missing_base_skills, provision_worktree, @@ -2378,3 +2379,105 @@ def test_copy_traversable_skip_existing_never_mkdirs_over_file(tmp_path): assert _copy_traversable(src, dst, skip_existing=True) is False assert (dst / "d").read_text() == "A FILE, NOT A DIR" + + +# ------------- #384 follow-through: legacy shared-exclude detection ------------ + + +def _repo_with_exclude(tmp_path, *lines: str) -> Path: + """A git repo whose repository-wide exclude carries exactly `lines`.""" + root = tmp_path / "repo" + root.mkdir() + git(root, "init", "-q") + exclude = root / ".git" / "info" / "exclude" + exclude.parent.mkdir(parents=True, exist_ok=True) + exclude.write_text("".join(f"{line}\n" for line in lines), encoding="utf-8") + return root + + +def test_legacy_exclude_pollution_finds_the_shield_patterns(tmp_path): + """The residue #384 is about: patterns an older bmad-loop appended to the + repository-wide exclude, which nothing removes.""" + repo = _repo_with_exclude(tmp_path, "/.claude/settings.json", "/.claude/skills") + claude = get_profile("claude") + + found = legacy_exclude_pollution(repo, [claude], []) + + assert found is not None + assert found.lines == ["/.claude/settings.json", "/.claude/skills"] + # the path is reported so the warning can name the file to hand-edit + assert found.path == repo / ".git" / "info" / "exclude" + + +def test_legacy_exclude_pollution_covers_seed_rels_and_customize_dir(tmp_path): + """The candidate set mirrors provision_worktree's: seeded rels and + `_bmad/custom` are shield patterns too, not just the skill tree.""" + repo = _repo_with_exclude(tmp_path, "/_bmad/custom", "/vendor/creds.json") + + found = legacy_exclude_pollution(repo, [], ["vendor/creds.json"]) + + assert found is not None + assert found.lines == ["/_bmad/custom", "/vendor/creds.json"] + + +def test_legacy_exclude_pollution_matches_exactly_never_by_containment(tmp_path): + """Authorship is ambiguous, so only a byte-identical line counts. Every line + here CONTAINS or is contained by `/.claude/skills`, and none of them is ours — + flagging any would be telling the operator to delete their own rule.""" + repo = _repo_with_exclude( + tmp_path, + ".claude/skills", # no leading slash: a different git pattern + "/.claude/skills/local", # narrower: their own subdir rule + "/.claude/skills-mine", # a sibling path that merely shares the prefix + " /.claude/skills", # leading space IS part of a gitignore pattern + "!/.claude/skills", # a negation, i.e. the opposite instruction + ) + + assert legacy_exclude_pollution(repo, [get_profile("claude")], []) is None + + +def test_legacy_exclude_pollution_silent_without_an_exclude_file(tmp_path): + """A repo that has never been provisioned has nothing to report.""" + root = tmp_path / "clean" + root.mkdir() + git(root, "init", "-q") + (root / ".git" / "info" / "exclude").unlink(missing_ok=True) + + assert legacy_exclude_pollution(root, [get_profile("claude")], []) is None + + +def test_legacy_exclude_pollution_silent_outside_a_repo(tmp_path): + """Preflight observation degrades: a plain directory is not a finding. + + Asserted against a populated pattern set, so a None here is a real refusal and + not an empty candidate set trivially matching nothing. Two branches can + produce it — git's non-zero rc and the OSError catch — and this pins neither + individually; the contract it holds is the one validate needs, that running + outside a repo yields no finding and no exception.""" + plain = tmp_path / "not-a-repo" + plain.mkdir() + (plain / "exclude-lookalike").write_text("/.claude/skills\n", encoding="utf-8") + + assert legacy_exclude_pollution(plain, [get_profile("claude")], []) is None + + +def test_legacy_exclude_pollution_ignores_the_private_worktree_exclude(tmp_path): + """The current shield's own file must never be reported: it is SUPPOSED to + carry these patterns, and it dies with its worktree. + + `--git-path info/exclude` answers with the shared file from a linked worktree + too, so the private one is not even looked at — the assertion is that a + worktree whose private exclude is full of patterns still reports nothing.""" + repo = _repo_with_exclude(tmp_path) # shared exclude: empty + (repo / "f.txt").write_text("x\n", encoding="utf-8") + git(repo, "config", "user.email", "t@t") + git(repo, "config", "user.name", "t") + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "init") + wt = tmp_path / "wt" + git(repo, "worktree", "add", "-q", str(wt), "-b", "wtb") + claude = get_profile("claude") + reason = _worktree_local_exclude(wt, [f"/{claude.skill_tree}"]) + assert reason is None, reason # the private shield really was written + + assert legacy_exclude_pollution(wt, [claude], []) is None From 2d1880fad491e5c693ef501dab15af3b8c15d8b4 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 29 Jul 2026 09:23:40 -0700 Subject: [PATCH 2/5] fix(validate): never flag a bare "/" as a shield pattern (#384) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scm.worktree_seed` is user-authored and unvalidated, so an empty entry renders as the candidate "/". Profiles cannot produce it — `skill_tree` and `seed_files` entries are validated non-empty, and the writer skips hookless config_paths for exactly this reason — but policy can. A bare "/" is a line an operator could legitimately have written, and this gate's output tells them to delete what it names, so a false match there is the one that does harm. Ablation-verified: dropping the guard fails the test. --- src/bmad_loop/install.py | 8 ++++++++ tests/test_install.py | 16 +++++++++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index fffff037..7416f58e 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -1075,6 +1075,14 @@ def legacy_exclude_pollution( # run seeded _bmad/custom is not knowable from here, and the pattern is ours # either way. candidates.add(f"/{CUSTOMIZE_DIR.as_posix()}") + # A bare "/" is never a shield pattern, and as a candidate it is the one that + # could do harm: it is a real (if odd) line for an operator to have written, + # and this function's output tells them to delete what it names. Profiles + # cannot produce it — `skill_tree` and `seed_files` entries are validated + # non-empty, and the writer skips hookless config_paths for exactly this + # reason — but `scm.worktree_seed` is user-authored and unvalidated, so an + # empty entry there renders as "/" here. + candidates.discard("/") try: answered = _shield_git(project, "rev-parse", "--git-path", "info/exclude") if answered.returncode != 0: diff --git a/tests/test_install.py b/tests/test_install.py index 390330a1..263e0bf6 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -2387,7 +2387,7 @@ def test_copy_traversable_skip_existing_never_mkdirs_over_file(tmp_path): def _repo_with_exclude(tmp_path, *lines: str) -> Path: """A git repo whose repository-wide exclude carries exactly `lines`.""" root = tmp_path / "repo" - root.mkdir() + root.mkdir(parents=True) git(root, "init", "-q") exclude = root / ".git" / "info" / "exclude" exclude.parent.mkdir(parents=True, exist_ok=True) @@ -2481,3 +2481,17 @@ def test_legacy_exclude_pollution_ignores_the_private_worktree_exclude(tmp_path) assert reason is None, reason # the private shield really was written assert legacy_exclude_pollution(wt, [claude], []) is None + + +def test_legacy_exclude_pollution_never_flags_a_bare_slash(tmp_path): + """`scm.worktree_seed` is user-authored and unvalidated, so an empty entry + renders as the candidate "/". That must not match: a bare "/" is a line an + operator could legitimately have, and this function's output tells them to + delete what it names.""" + repo = _repo_with_exclude(tmp_path, "/") + + assert legacy_exclude_pollution(repo, [], [""]) is None + # the guard is scoped to "/" alone — a real seed rel alongside it still reports + repo2 = _repo_with_exclude(tmp_path / "b", "/", "/vendor/creds.json") + found = legacy_exclude_pollution(repo2, [], ["", "vendor/creds.json"]) + assert found is not None and found.lines == ["/vendor/creds.json"] From b3dc7e7d130f847a9849d08e868d821d85b7ca2a Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 29 Jul 2026 09:33:51 -0700 Subject: [PATCH 3/5] fix(policy): validate scm.worktree_seed entries at load (#384) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while guarding the read side of the exclude gate. Three sources feed one seed list into `provision_worktree`'s copy loop, and two of them already reject non-project-relative entries with the same predicate — `adapters/profile.py` for a profile's `seed_files`, and `plugins/manifest.py::_check_relative_paths` for a plugin manifest's. `scm.worktree_seed` arrived unchecked. The empty entry is the one that does damage rather than merely no-op. `""` makes that loop resolve `src` to the repo ROOT and `dst` to the worktree; both pass its `is_relative_to` containment checks, so `_copy_traversable` copies the entire project into the worktree — gitignored and untracked files included — and the rel then renders as the exclude pattern "/", which git-excludes the whole worktree so the unit's `git add -A` stages nothing and the story's work is never committed. Measured, not inferred. Absolute and `..` entries were already contained by those same checks and silently skipped; they are rejected here too, for consistency with the sibling sources and because a silently inert seed entry reads as applied configuration when it is not. Ablation-verified: removing the check fails all five new cases while the existing valid-config test keeps passing. --- CHANGELOG.md | 7 +++++++ src/bmad_loop/install.py | 18 +++++++++++------- src/bmad_loop/policy.py | 21 ++++++++++++++++++++- tests/test_install.py | 12 ++++++++---- tests/test_policy.py | 35 +++++++++++++++++++++++++++++++++++ 5 files changed, 81 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f09f8a0a..d6b04f51 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -266,6 +266,13 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Fixed +- **`scm.worktree_seed` entries are validated at policy load (#384).** An empty entry made worktree + provisioning resolve its copy source to the repo **root**, copying the whole project — gitignored + and untracked files included — into the worktree, then render as the exclude pattern `/`, which hid + the entire worktree from the unit's `git add -A` so nothing was ever committed. Entries must be + project-relative, the same rule adapter profiles and plugin manifests already apply to their own + seed lists; `worktree_seed` was the only unchecked source. + - **The worktree git-add shield no longer hides new files in your own checkout (#384).** Worktree provisioning shielded the tool files it writes (`.claude/skills`, the per-CLI hook config, seeded configs) by appending to `.git/info/exclude` — a file shared with the main checkout and every diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index 7416f58e..2e129bd6 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -1075,13 +1075,17 @@ def legacy_exclude_pollution( # run seeded _bmad/custom is not knowable from here, and the pattern is ours # either way. candidates.add(f"/{CUSTOMIZE_DIR.as_posix()}") - # A bare "/" is never a shield pattern, and as a candidate it is the one that - # could do harm: it is a real (if odd) line for an operator to have written, - # and this function's output tells them to delete what it names. Profiles - # cannot produce it — `skill_tree` and `seed_files` entries are validated - # non-empty, and the writer skips hookless config_paths for exactly this - # reason — but `scm.worktree_seed` is user-authored and unvalidated, so an - # empty entry there renders as "/" here. + # A bare "/" is never a shield pattern, and it is the candidate that could do + # harm: it is a real (if odd) line for an operator to have written, and this + # function's output tells them to delete what it names. + # + # No validated caller can produce it any more — every seed source is checked + # at its own boundary (`skill_tree` and profile `seed_files` in + # adapters/profile.py, plugin seeds in plugins/manifest.py, and + # `scm.worktree_seed` in policy.py, which was the hole), and the writer skips + # hookless config_paths for this same reason. This stays as the helper's own + # contract: it takes `seed_rels` from its caller, and a detector that tells + # someone to delete a line should not depend on validation happening upstream. candidates.discard("/") try: answered = _shield_git(project, "rev-parse", "--git-path", "info/exclude") diff --git a/src/bmad_loop/policy.py b/src/bmad_loop/policy.py index edb5aed4..a25727e3 100644 --- a/src/bmad_loop/policy.py +++ b/src/bmad_loop/policy.py @@ -19,7 +19,7 @@ from pathlib import Path from typing import Any -from .platform_util import atomic_replace +from .platform_util import atomic_replace, has_parent_ref, is_absolute_path POLICY_FILE = Path(".bmad-loop") / "policy.toml" @@ -957,6 +957,25 @@ def loads(text: str, plugin_schemas: dict[str, Any] | None = None) -> Policy: ) if scm.failed_diff_max_mb < 1: raise PolicyError(f"scm.failed_diff_max_mb must be >= 1: got {scm.failed_diff_max_mb}") + # The same rule the other two seed sources already apply to their own entries + # (adapters/profile.py `seed_files`, plugins/manifest.py `_check_relative_paths`). + # All three feed one list into provision_worktree's seed loop, and this was the + # only one arriving unvalidated. + # + # The empty entry is why this is a guard rather than a tidy-up: `""` makes that + # loop resolve src to the repo ROOT and dst to the worktree, both of which pass + # its `is_relative_to` containment checks — so it copies the entire repo into + # the worktree, gitignored and untracked files included, and then renders as the + # exclude pattern "/", which git-excludes the whole worktree so the unit's + # `git add -A` stages nothing at all. Absolute and `..` entries are already + # contained by those same checks (silently skipped); they are rejected here for + # consistency with the sibling sources, and because a silently-inert seed entry + # reads as applied configuration when it is not. + for seed in scm.worktree_seed: + if not seed or is_absolute_path(seed) or has_parent_ref(seed): + raise PolicyError( + f"scm.worktree_seed entries must be project-relative paths: got {seed!r}" + ) cleanup = CleanupPolicy( run_retention=int(cleanup_d.get("run_retention", CleanupPolicy.run_retention)), retention_days=int(cleanup_d.get("retention_days", CleanupPolicy.retention_days)), diff --git a/tests/test_install.py b/tests/test_install.py index 263e0bf6..a81c01b8 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -2484,10 +2484,14 @@ def test_legacy_exclude_pollution_ignores_the_private_worktree_exclude(tmp_path) def test_legacy_exclude_pollution_never_flags_a_bare_slash(tmp_path): - """`scm.worktree_seed` is user-authored and unvalidated, so an empty entry - renders as the candidate "/". That must not match: a bare "/" is a line an - operator could legitimately have, and this function's output tells them to - delete what it names.""" + """An empty seed rel renders as the candidate "/", which must never match: a + bare "/" is a line an operator could legitimately have, and this function's + output tells them to delete what it names. + + Policy now rejects an empty `scm.worktree_seed` entry at load, so no validated + caller reaches this. The guard is the helper's own contract — it takes + `seed_rels` from its caller, and a detector that instructs deletion should not + depend on validation having happened upstream.""" repo = _repo_with_exclude(tmp_path, "/") assert legacy_exclude_pollution(repo, [], [""]) is None diff --git a/tests/test_policy.py b/tests/test_policy.py index c2b0805b..1fc998e5 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -618,6 +618,41 @@ def test_scm_worktree_seed_settings(tmp_path): assert pol.scm.worktree_seed == (".mcp.json", ".envrc") +@pytest.mark.parametrize( + "entry", + [ + "", # the harmful one — see below + "/etc/passwd", # POSIX-absolute + "C:\\secrets", # Windows drive-absolute (rejected on POSIX too) + "../../etc", # climbs out without being absolute + ], +) +def test_scm_worktree_seed_rejects_non_project_relative_entries(tmp_path, entry): + """`worktree_seed` was the only one of the three seed sources feeding + provision_worktree that arrived unvalidated — profiles and plugin manifests + both already apply this rule to their own entries. + + The empty entry is the one that does damage rather than merely no-op: it makes + the seed loop resolve src to the repo ROOT and dst to the worktree, both of + which pass its containment checks, so the whole repo is copied in and the + rendered pattern "/" then git-excludes the entire worktree.""" + p = tmp_path / "policy.toml" + p.write_text(f"[scm]\nworktree_seed = [{entry!r}]\n".replace("'", '"')) + + with pytest.raises(policy.PolicyError, match="worktree_seed entries must be project-relative"): + policy.load(p) + + +def test_scm_worktree_seed_rejects_a_bad_entry_beside_good_ones(tmp_path): + """Every entry is checked, not just the first: a valid leading entry must not + let a later empty one through.""" + p = tmp_path / "policy.toml" + p.write_text('[scm]\nworktree_seed = [".mcp.json", "", ".envrc"]\n') + + with pytest.raises(policy.PolicyError, match="got ''"): + policy.load(p) + + def test_scm_override(tmp_path): p = tmp_path / "policy.toml" p.write_text( From 28082d80211e07d6c1e6703192099fdd6d5ec89c Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 29 Jul 2026 09:36:29 -0700 Subject: [PATCH 4/5] test(validate): assert the exclude path, not a POSIX-shaped substring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `assert "info/exclude" in text` encoded a POSIX separator, so on Windows the message rendered `...\.git\info\exclude` and the substring was never present — the test failed on both Windows jobs while passing everywhere else. Assert the real path instead, which is stricter as well as portable: it pins the exact file the warning names rather than a fragment of it. --- tests/test_cli.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/test_cli.py b/tests/test_cli.py index 21550b23..10eb8859 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -4201,13 +4201,15 @@ def test_validate_pollution_warning_prints_the_lines_in_text_mode(project, capsy """The remedy is manual, so the human-readable output has to carry enough to act on: which file, which lines, and what the symptom looks like.""" _make_validate_pass(project, monkeypatch, capsys) - _pollute_exclude(project, "/.claude/skills") + exclude = _pollute_exclude(project, "/.claude/skills") cli.main(["validate", "--project", str(project.project)]) text = _validate_output(capsys) assert "/.claude/skills" in text - assert "info/exclude" in text + # the file, by its real path rather than a POSIX-shaped substring: on Windows + # this renders with backslashes, so asserting "info/exclude" passed nowhere + assert str(exclude).lower() in text assert "git add -a" in text # the symptom, lowercased by _validate_output assert "delete them by hand" in text From 0124783563f8d4d10135871f97971262d22906cd Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 29 Jul 2026 12:25:43 -0700 Subject: [PATCH 5/5] fix(validate): re-derive the legacy detector's git guard (#389) `legacy_exclude_pollution` shared `_shield_git` with the shield, so routing that through the chokepoint moves its faults too: a timeout and a spawn failure now arrive as `GitError` and `GitSpawnError`, neither of which the old `subprocess.SubprocessError`/`OSError` tuple catches. A warning-only preflight detector has no business propagating either. --- src/bmad_loop/install.py | 7 +++++-- tests/test_install.py | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index 13b70a49..9c22b061 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -1140,7 +1140,7 @@ def legacy_exclude_pollution( # someone to delete a line should not depend on validation happening upstream. candidates.discard("/") try: - answered = _shield_git(project, "rev-parse", "--git-path", "info/exclude") + answered = git_bytes(project, "rev-parse", "--git-path", "info/exclude") if answered.returncode != 0: return None # not a repo, or a git too old to answer # fsdecode for the same reason _worktree_local_exclude uses it: a non-UTF-8 @@ -1149,7 +1149,10 @@ def legacy_exclude_pollution( if not exclude.is_absolute(): exclude = project / exclude present = exclude.read_text(encoding="utf-8").splitlines() - except (OSError, UnicodeError, subprocess.SubprocessError): + # GitError for the chokepoint's two raised faults (a timeout, and a spawn + # failure as GitSpawnError) — this is a `validate` warning, and a git that + # cannot answer is not a finding. OSError and UnicodeError stay for the read. + except (GitError, OSError, UnicodeError): return None # Set intersection IS the exact-match rule — no normalization, no strip(): a # leading space is a meaningful part of a gitignore pattern, so a line that diff --git a/tests/test_install.py b/tests/test_install.py index 90049d97..cd09d715 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -2749,6 +2749,25 @@ def test_legacy_exclude_pollution_silent_outside_a_repo(tmp_path): assert legacy_exclude_pollution(plain, [get_profile("claude")], []) is None +def test_legacy_exclude_pollution_silent_when_git_never_answers(project, monkeypatch): + """The sibling above pins git ANSWERING "not a repo" (a non-zero rc). This pins + the other shape — git never answering at all, which the chokepoint raises as + `GitError` (a timeout) or its `GitSpawnError` subclass. + + A warning-only preflight detector has no business propagating either: nothing + it could report is worth failing `bmad-loop validate` over. + + Ablation: drop `GitError` from this helper's except tuple and this fails — the + fault escapes into validate.""" + + def unanswerable(repo, *args): + raise verify.GitSpawnError(f"git {args[0]} failed to spawn in {repo}") + + monkeypatch.setattr(install_mod, "git_bytes", unanswerable) + + assert legacy_exclude_pollution(project.project, [get_profile("claude")], []) is None + + def test_legacy_exclude_pollution_ignores_the_private_worktree_exclude(tmp_path): """The current shield's own file must never be reported: it is SUPPOSED to carry these patterns, and it dies with its worktree.