diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e56649d..e80bb3fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -260,6 +260,13 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Fixed +- **Refuse `isolation = "worktree"` combined with a `repo_root` override (#414).** The pair + previously produced a green preflight and then an isolated session with no dev primitive, no + result, and nothing journaled naming the cause. `validate` now reports it; `run`, `sweep`, + `resume` and the auto-triggered child sweep refuse to start; the dry-run banner names it first; + the TUI toasts it ahead of its clean-tree gate. Plumbing `project` through provisioning so both + work together is #443. + - **A configured path carrying `[`, `]`, `*` or `?` no longer makes git act on the wrong files (#423).** `implementation_artifacts` reaches git verbatim out of the operator's `_bmad/bmm/config.yaml`, and git reads a positional operand as a _pathspec_, not a path — so such diff --git a/README.md b/README.md index 73885a3e..90b21dad 100644 --- a/README.md +++ b/README.md @@ -477,7 +477,7 @@ Merge-back is always **serialized** — `max_parallel` is a validated knob clamp The settings editor with the [scm] section expanded: isolation, branch_per, merge_strategy, the seed-adapter-configs switch, and the extra-worktree-seed-files field.

-For a monorepo or any layout where the git root differs from the project dir, set an optional `repo_root` key in `_bmad/bmm/config.yaml` — it decouples where git/code work happens from where run state lives (defaults to the project dir). +For a monorepo or any layout where the git root differs from the project dir, set an optional `repo_root` key in `_bmad/bmm/config.yaml` — it decouples where git/code work happens from where run state lives (defaults to the project dir). It is **not compatible with `isolation = "worktree"`**: provisioning seeds a worktree from `repo_root` while the preflight probes `project`, so `validate` reports the pair and `run`/`sweep`/`resume` refuse to start. Use one or the other — plumbing both through provisioning is tracked as #443. ### Plugins diff --git a/docs/FEATURES.md b/docs/FEATURES.md index cc652938..1048aa52 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -78,7 +78,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - 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` — in a private exclude scoped to that worktree alone (see below), never repo-wide. - The git-add shield is scoped to the worktree and expires with it (#384). The provisioned tool files (skill trees, the per-CLI hook config, seeded configs) are excluded through a private `.git/worktrees//info/exclude`, activated by a worktree-scoped `core.excludesFile` — so the shield applies to that unit only, and `git worktree remove` deletes it along with the worktree. Storing that key is not the same as git reading it: config supplied through the environment (`GIT_CONFIG_COUNT`, `GIT_CONFIG_PARAMETERS`, or a `git -c` above us) is **command** scope, which outranks **worktree** scope, so an ambient `core.excludesFile` of your own would leave a shield that reported success and never applied. After activating, bmad-loop asks git which excludes file it actually resolves; if that is not the one just written, the shield is skipped with a journaled reason rather than reported as working. The repository-wide `.git/info/exclude` is never written: it is shared with your own checkout and permanent, so shielding through it made every **new** file under a tracked tool dir (`.claude/skills`, `.claude/settings.json`) silently invisible to your `git add -A`, long after the run. Because that key shadows your own `core.excludesFile` (git reads it from the most specific scope and never concatenates), your excludes file is copied into the private one **byte for byte** when it is created — any encoding, since exclude patterns are paths and POSIX paths are arbitrary bytes — and any path, read NUL-terminated so leading or trailing whitespace in the filename survives. An excludes file that exists but **cannot be read**, or a **git that will not say which file applies** (a timeout, a failed spawn, or any answer to that one query other than a definite "there is no such key"), skips the shield with a journaled and notified reason instead of shadowing patterns it could not copy: not knowing whether there is anything to copy has the same standing as knowing there is and failing to read it. Only a definite **absent** answer is a silent no-op. The same rule governs the two `rev-parse` probes that identify the repository: only a failure of the first is the expected silent skip (you handed it a plain directory, or git is missing) — once git has answered that one, a fault on the second is journaled rather than swallowed. An **explicitly empty** `core.excludesFile` is not an unset one: git reads that as "no excludes file at all" and does **not** fall back to `$XDG_CONFIG_HOME/git/ignore`, so neither does the shield — patterns you deliberately switched off stay off instead of being copied into the private file and re-applied inside the worktree. Two runs against one repository are **serialized**: the probe → enable → activate → rollback sequence is taken under an exclusive lock, and a failed activation never rolls the repo-format flag back while another worktree's `config.worktree` still depends on it — otherwise one run's failure would silently switch off a sibling run's live shield. Three residues are worth knowing about: enabling this needs `extensions.worktreeConfig`, a permanent repo-format flag bmad-loop sets once and never removes — written at the last possible moment, so a degrade **above** it never leaves your repo marked for a shield that did not apply, and wherever it could be left set without a working shield — the enable failing, the activation failing, or the activation succeeding without taking effect — the flag is rolled back. It outlives a failed shield only where that rollback was declined because a sibling worktree depends on the flag, or could not be made at all, and the reason says which — the lock leaves a zero-length `.git/bmad-loop-shield.lock` behind (inside `.git`, so never in your working tree and never stageable), and lines an older bmad-loop already wrote into `.git/info/exclude` are **not** removed for you — delete them by hand. Where the flag cannot be set safely (`core.bare = true` or `core.worktree` in the shared config, which git requires you move first), the shield is skipped with a journaled reason rather than widened back — and a git that cannot **answer** those two questions is treated the same way rather than as a "no", since reading "git failed" as "that key is unset" is what would open the gate. It also needs **git 2.20 or newer** — the release that added both the flag and `git config --worktree`; on anything older (or a git that will not report its version) the shield is skipped the same way, and the repo-format flag is deliberately _not_ written, since git that old refuses a repository carrying it. - 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). +- 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) — but it is **mutually exclusive with `isolation = "worktree"`**, which seeds from `repo_root` while the preflight probes `project`; `validate` reports the pair and the run/sweep/resume preflight refuses it (#414; #443 lifts the restriction). - `commit_message_template` (`{story_key}` / `{run_id}` substituted) customizes story/bundle commit messages. ### Plugins (extensibility) diff --git a/src/bmad_loop/bmadconfig.py b/src/bmad_loop/bmadconfig.py index 16fe21af..3c914af7 100644 --- a/src/bmad_loop/bmadconfig.py +++ b/src/bmad_loop/bmadconfig.py @@ -63,6 +63,55 @@ def rebase(p: Path) -> Path: ) +def worktree_isolation_conflict(paths: ProjectPaths, isolation: str) -> str | None: + """The refusal message for ``isolation = "worktree"`` under a `repo_root` + override, or None when the combination is supported (#414). + + Worktree provisioning reads ``repo_root`` for every surface it seeds *off disk* + — the upstream skill trees, `_bmad/` and the `_bmad/custom/` overrides inside + it, and each `seed_files`/`seed_globs` entry — and bakes the absolute hook-relay + path from it into the worktree's hook config, while `init`, `validate` and the + run preflight write and probe those same surfaces under ``project``. (The relay + itself is pointed at, never copied. The `MODULE_SKILLS` this wheel bundles are + seeded from package data and are unaffected by either root; nothing is seeded + from ``project``, which `provision_worktree` is never even passed.) + `load_paths` *requires* `project/_bmad/bmm/config.yaml`, so `_bmad/` is under + `project` by definition and `repo_root/_bmad/` generally does not exist. When + the two diverge the preflight therefore approves a surface the isolated run + never receives, and the seed-completeness gates go inert rather than fire: an + isolated session dispatches into a worktree with no dev primitive and no + renderer, and stops with no result and nothing journaled naming the cause. + + **This function exists to be deleted.** The real fix is #443 — plumb ``project`` + through provisioning for the non-git reads — and landing it removes this + function, all five of its call sites, the `policy.isolation-repo-root` id and + both doc sentences. It is a refusal rather than the fix because "which root + wins" is a separate decision per seeded surface (the relay only exists under + `project`; operator-configured `seed_files` may legitimately name a path outside + it), and `ProjectPaths.rebased` encodes `project == repo_root` besides. So the + message names only remediations that exist today. Both are named because either + alone is sufficient and which one is right is the operator's call: the override + buys a decoupled git root, the isolation mode buys per-unit worktrees, and until + #443 lands the orchestrator cannot give both. + + Sole producer of the text, shared by `cmd_validate`, the run/sweep preflight, + the dry-run honesty banner and the TUI's pre-launch guard, so the four cannot + drift. Compares resolved paths: `load_paths` resolves both sides, but a + hand-built :class:`ProjectPaths` (tests) need not have.""" + if isolation != "worktree": + return None + if paths.repo_root.resolve() == paths.project.resolve(): + return None + return ( + 'isolation = "worktree" is not supported when repo_root differs from the project ' + f"directory: worktree provisioning seeds from repo_root ({paths.repo_root}) while " + f"init, validate and the run preflight read the project ({paths.project}), so an " + "isolated session would get none of the skills the preflight just approved. " + "Remove the `repo_root` key from _bmad/bmm/config.yaml, or set " + '`isolation = "none"` under [scm] in .bmad-loop/policy.toml.' + ) + + def _resolve(raw: str, project: Path) -> Path: return Path(raw.replace("{project-root}", str(project))).resolve() diff --git a/src/bmad_loop/checks.py b/src/bmad_loop/checks.py index 6e7d3356..43da777c 100644 --- a/src/bmad_loop/checks.py +++ b/src/bmad_loop/checks.py @@ -47,6 +47,7 @@ "bmad-config", "policy", "policy.model-qualified", + "policy.isolation-repo-root", "adapter.profile", "adapter.binary", "adapter.hookless", diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 67fe42cb..b72cfb9a 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -169,6 +169,29 @@ def _reject_bad_run_id(run_id: str | None) -> int | None: return None +def _reject_isolation_conflict(paths: bmadconfig.ProjectPaths, pol) -> int | None: + """Refuse `isolation = "worktree"` under a `repo_root` override (#414). Returns + 1 to abort, None to proceed — the `_reject_bad_run_id` shape. + + Called from the three :class:`~engine.Engine` construction sites that return an + rc to a human: `cmd_run`, `cmd_sweep`, and `_resume_paused_run` — the shared + helper behind both `resume` and `resolve`'s re-arm. The fourth such site, the + auto-triggered child sweep in `_sweep_factory`, shares the refusal but not this + disposition: it has no rc channel, so it raises (see the comment there). + Keyed on Engine construction rather than on "loads policy.toml", which is a + wider set that does not all provision — `_configure_mux` reads the file on + every command and builds nothing; `cmd_validate` and `cmd_clean` load it and + never mount a worktree. + + `validate` deliberately does not call this — it reports rather than aborts, so + it renders the same message as a Finding and keeps running its other gates.""" + conflict = bmadconfig.worktree_isolation_conflict(paths, pol.scm.isolation) + if conflict is None: + return None + print(conflict, file=sys.stderr) + return 1 + + def _reconcile_stale(project: Path, paths: bmadconfig.ProjectPaths, pol) -> None: """Tear down worktrees leaked by a prior run that stopped mid-flight, before starting a new run/sweep — the clean-finish GC never reached them. Gated on @@ -230,6 +253,20 @@ def cmd_validate(args: argparse.Namespace) -> int: except policy_mod.PolicyError as e: report.fail("policy", str(e)) + # #414: the one configuration where every gate below reports on a surface the + # isolated run will never see. Reported only when it fires — there is no `ok` + # twin, because the supported case is "no such conflict", which would print a + # line about a coupling most projects have never configured either half of. + # Needs both halves loaded; either failing already has its own finding above. + if paths is not None and pol is not None: + conflict = bmadconfig.worktree_isolation_conflict(paths, pol.scm.isolation) + if conflict is not None: + report.fail( + "policy.isolation-repo-root", + conflict, + {"repo_root": str(paths.repo_root), "project": str(paths.project)}, + ) + # Built exactly the way run/sweep's real preflight builds it, so validate's # verdict and their abort cannot disagree. Deliberately NOT `[p.skill_tree for p # in profiles]`: that carries triage's tree, and every skills check below asks a @@ -565,7 +602,9 @@ def _dev_skill_for_role(pol, project: Path, role: str) -> str: return install.dev_primitive_or_default(project, tree) -def _warn_preflight_would_abort(project: Path, pol, *, require_stories: bool = False) -> None: +def _warn_preflight_would_abort( + paths: bmadconfig.ProjectPaths, pol, *, require_stories: bool = False +) -> None: """Dry-run honesty banner: say so when the real command would refuse to run. ``--dry-run`` returns before `_require_base_skills` (cmd_run/cmd_sweep), so a @@ -575,20 +614,34 @@ def _warn_preflight_would_abort(project: Path, pol, *, require_stories: bool = F ``/bmad-dev-auto`` reads fine and would HALT an unattended session on the shim's interactive migration gate. - Reads the same finding list `_require_base_skills` gates on, so the two cannot - disagree about what "runnable" means. + Mirrors both of the refusals the dry-run's early return skips past, and only + those: the finding list `_require_base_skills` gates on, and the #414 isolation + conflict `_reject_isolation_conflict` refuses ahead of it. Reading the same + sources as the gates themselves is what keeps the preview from disagreeing with + the real command about what "runnable" means. Severity-filtered to `problem` + for that same reason — `_require_base_skills` ignores warnings, so reporting one + as a `FAIL:` line would promise an abort that never comes. The dirty-tree, queue + and run-id gates are deliberately not part of this banner. + + Takes the whole :class:`~bmadconfig.ProjectPaths` rather than `project` alone + because the #414 refusal is a fact about the two roots' relationship; every + other probe here still reads `paths.project`, which is what `init` wrote and + what a session's own root resolution will re-derive. The exit code deliberately stays 0. A dry-run is a diagnostic — refusing to print the schedule would withhold the very thing the operator asked for, and every existing caller reads rc 0 as "the preview rendered", not as "the project is ready". The banner goes to stderr so stdout stays the preview.""" - trees = _skill_trees(project, pol) + trees = _skill_trees(paths.project, pol) problems = [ p.message - for p in install.missing_base_skills(project, trees) - + (install.missing_stories_support(project, trees) if require_stories else []) + for p in install.missing_base_skills(paths.project, trees) + + (install.missing_stories_support(paths.project, trees) if require_stories else []) if p.severity == "problem" ] + conflict = bmadconfig.worktree_isolation_conflict(paths, pol.scm.isolation) + if conflict is not None: + problems.insert(0, conflict) if not problems: return print( @@ -1006,6 +1059,14 @@ def cmd_run(args: argparse.Namespace) -> int: if args.dry_run: return _dry_run(paths, pol, args, stories_on, spec_folder) + # First of the configuration refusals (`_reject_bad_run_id` and the two loaders + # above can abort earlier), and deliberately before the queue and worktree-clean + # gates: this one says the configuration cannot run at all, so making the + # operator clear a dirty tree or fix a story key first would only delay the + # same abort. + if (rc := _reject_isolation_conflict(paths, pol)) is not None: + return rc + if stories_on: problem = _validate_stories_folder(paths, spec_folder, selector=args.story) if problem: @@ -1091,7 +1152,7 @@ def _dry_run( if stories_on: return _dry_run_stories(paths, pol, args, spec_folder) - _warn_preflight_would_abort(paths.project, pol) + _warn_preflight_would_abort(paths, pol) def render(role: str, prompt: str) -> str: return _render_invocation(pol, paths.project, role, prompt) @@ -1135,7 +1196,7 @@ def _dry_run_stories( ) -> int: """Print the linear stories-mode schedule (list order, checkpoints, live on-disk state) — no topo waves, one story per line, spawns nothing.""" - _warn_preflight_would_abort(paths.project, pol, require_stories=True) + _warn_preflight_would_abort(paths, pol, require_stories=True) folder = stories_mod.resolve_spec_folder(paths.project, spec_folder) # The real dispatch always uses the project-relative folder (the engine # relativizes it); render the identical string here so dry-run and run agree. @@ -1235,6 +1296,18 @@ def _sweep_factory(project: Path, paths: bmadconfig.ProjectPaths): def factory(trigger: str) -> None: pol = policy_mod.load(_policy_path(project)) + # Raise rather than return the rc the other three sites return. By the time + # the engine calls this it has already latched the trigger and journaled + # `sweep-auto-trigger`, and it reads a plain return as success — so a bare + # decline would be recorded as `sweep-auto-finished`, which `engine.py` + # defines as "a clean completion from the parent's perspective": a child + # sweep that ran and finished when none was ever launched. Raising lands on + # the same `sweep-auto-failed` + notify path the `load` above already takes + # on an unparseable policy.toml, which is the same kind of event — the + # config on disk changed under a run that had already started. + conflict = bmadconfig.worktree_isolation_conflict(paths, pol.scm.isolation) + if conflict is not None: + raise RuntimeError(conflict) _start_sweep( project, paths, @@ -1258,6 +1331,9 @@ def cmd_sweep(args: argparse.Namespace) -> int: if args.dry_run: return _sweep_dry_run(paths, pol) + if (rc := _reject_isolation_conflict(paths, pol)) is not None: + return rc + if not verify.worktree_clean(paths.repo_root): print("git worktree is not clean — commit or stash first", file=sys.stderr) return 1 @@ -1284,7 +1360,7 @@ def cmd_sweep(args: argparse.Namespace) -> int: def _sweep_dry_run(paths: bmadconfig.ProjectPaths, pol) -> int: # Before the no-ledger early return below: a broken install is worth saying so # about whether or not there is anything to sweep. - _warn_preflight_would_abort(paths.project, pol) + _warn_preflight_would_abort(paths, pol) ledger = paths.deferred_work if not ledger.is_file(): print(f"no deferred-work ledger at {ledger}") @@ -1320,6 +1396,12 @@ def _resume_paused_run(project: Path, run_dir: Path) -> int: print(f"run {run_dir.name} already finished", file=sys.stderr) return 1 pol = policy_mod.load(_policy_path(project)) + # Resume re-reads config.yaml and policy.toml from disk, so it is a second + # entrypoint into the same engine and gets the same refusal — a run started + # before the override was added must not finish its remaining stories through + # provisioning the preflight would now refuse. + if (rc := _reject_isolation_conflict(paths, pol)) is not None: + return rc if not _require_base_skills(project, pol, require_stories=state.source == "stories"): return 1 journal = Journal(run_dir) diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index 33c5fdbd..8829018a 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -178,8 +178,31 @@ def _mux_guarded(self, probe: Callable[[], _T]) -> tuple[bool, _T | None]: return False, None def _guarded(self, go: Callable[[], None]) -> None: - """Pre-launch guard mirroring the CLI: clean worktree required, plus a - confirm when another engine is already live.""" + """Pre-launch guard mirroring the CLI: the #414 isolation/repo_root conflict + refused first, then a clean worktree required, plus a confirm when another + engine is already live.""" + # The detached CLI refuses this combination too, and it is the authority — + # this only turns a pane that dies immediately into a toast. Ordered ahead of + # the clean-tree gate for the same reason `cmd_run` orders it ahead: this one + # says the configuration cannot run at all, so answering "commit or stash + # first" would send the operator to fix something that is not the problem. + # + # An unreadable config or policy falls through to launch rather than + # blocking: the guard cannot tell "no conflict" from "could not look", so it + # defers to the CLI, which reads the same two files and fails loudly on + # whichever one it cannot parse. Both loaders convert an undecodable file + # into their own typed error, so the two named here are the whole surface; + # a raw `UnicodeDecodeError` would be a ValueError and escape. + try: + conflict = bmadconfig.worktree_isolation_conflict( + bmadconfig.load_paths(self.project), + policy.load(self.project / POLICY_FILE).scm.isolation, + ) + except (bmadconfig.BmadConfigError, policy.PolicyError, OSError): + conflict = None + if conflict is not None: + self.notify(conflict, severity="error") + return try: if not verify.worktree_clean(self.project): self.notify( diff --git a/tests/test_bmadconfig.py b/tests/test_bmadconfig.py index 59db0c3c..e56251f0 100644 --- a/tests/test_bmadconfig.py +++ b/tests/test_bmadconfig.py @@ -1,6 +1,7 @@ """ProjectPaths.repo_root / rebased and load_paths(repo_root) — the Phase 1 Workspace-seam foundation. repo_root defaults to project (today's behavior); -rebased re-roots artifacts onto a worktree-style checkout.""" +rebased re-roots artifacts onto a worktree-style checkout. Plus +worktree_isolation_conflict, the #414 refusal predicate built on the same pair.""" from __future__ import annotations @@ -104,3 +105,19 @@ def test_workspace_default_uses_repo_root(tmp_path: Path) -> None: ws = Workspace.default(paths) assert ws.root == tmp_path / "repo" assert ws.paths is paths + + +def test_worktree_isolation_conflict_compares_normalized_paths(tmp_path: Path) -> None: + """A refusal gate's false positives are worse than the bug it forecloses (#414): + this one would refuse an ordinary isolated project whose `repo_root` merely spells + the same directory a different way. `load_paths` resolves both sides, but nothing + obliges a hand-built ProjectPaths — or a future caller — to have done so.""" + (tmp_path / "p").mkdir() + paths = ProjectPaths( + project=tmp_path / "p", + implementation_artifacts=tmp_path / "p" / "impl", + planning_artifacts=tmp_path / "p" / "plan", + repo_root=tmp_path / "p" / ".." / "p", + ) + assert paths.repo_root != paths.project, "the two spellings really are different" + assert bmadconfig.worktree_isolation_conflict(paths, "worktree") is None diff --git a/tests/test_cli.py b/tests/test_cli.py index 8a1ab48d..8f45722f 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -5631,3 +5631,193 @@ def test_validate_does_not_gate_a_triage_only_skill_tree(project, capsys, monkey for f in doc["findings"] if f["check"].startswith("skills.") and (f["detail"] or {}).get("tree") == ".agents/skills" ] + + +# ---- #414: worktree isolation is refused under a repo_root override ---------- + +ISOLATION_WORKTREE_POLICY = ( + '[adapter]\nname = "claude"\nmodel = "opus"\n\n[scm]\nisolation = "worktree"\n' +) +NO_ISOLATION_POLICY = '[adapter]\nname = "claude"\nmodel = "opus"\n\n[scm]\nisolation = "none"\n' +REFUSAL = 'isolation = "worktree" is not supported' + + +def _override_repo_root(paths, rel="git-root"): + """Point `repo_root` away from the project — #414's monorepo layout, minus the + monorepo. The target has no `_bmad/`, which is the shape the issue reports, but + it DOES exist: `cmd_run`/`cmd_sweep` probe `verify.worktree_clean(repo_root)`, + and against a missing dir that raises `GitError` instead of answering, which + would make every "the isolation gate spoke first" assertion below unfalsifiable + — the later gate would crash rather than print the message it is asserted not to + print.""" + (paths.project / rel).mkdir(exist_ok=True) + cfg = paths.project / "_bmad" / "bmm" / "config.yaml" + cfg.write_text(cfg.read_text() + f"repo_root: '{{project-root}}/{rel}'\n", encoding="utf-8") + + +def _render_findings(doc) -> str: + """Draw a document through the TUI renderer (#210) and return the text. The + `{'` assertion is that file's nested-dict tell: a renderer that str()s a detail + shape it did not model prints a Python repr.""" + from rich.console import Console + + from bmad_loop.tui import widgets + + console = Console(width=96) + with console.capture() as capture: + console.print(widgets.validate_findings(doc, details=True)) + rendered = capture.get() + assert "{'" not in rendered + return rendered + + +def test_validate_refuses_worktree_isolation_under_a_repo_root_override( + project, monkeypatch, capsys +): + """#414: provisioning seeds every non-git surface from `repo_root` while every + gate validate runs probes `project`, so a split pair makes validate approve a + surface the isolated run never receives. A `problem`, so the rc flips — and the + fixture is committed first, so the rc-1 is this gate and not a dirty tree.""" + _make_validate_pass(project, monkeypatch, capsys, policy=ISOLATION_WORKTREE_POLICY) + _override_repo_root(project) + git(project.project, "commit", "-qam", "repo_root override") + + doc = machine_json(["validate", "--project", str(project.project), "--json"], capsys, rc=1) + finding = {f["check"]: f for f in doc["findings"]}["policy.isolation-repo-root"] + assert finding["severity"] == "problem" + # Both remediations named, and only remediations that exist on this line: fix-1 + # (plumbing `project` through provisioning) is a main-line option, so the message + # must not gesture at a flag or a version that would make the pair work. + assert "Remove the `repo_root` key" in finding["message"] + assert '`isolation = "none"`' in finding["message"] + assert finding["detail"] == { + "repo_root": str(project.project / "git-root"), + "project": str(project.project), + } + _render_findings(doc) # the detail shape draws in the TUI modal + + +@pytest.mark.parametrize( + "override,policy_text", + [(True, NO_ISOLATION_POLICY), (False, ISOLATION_WORKTREE_POLICY)], + ids=["override-without-worktree", "worktree-without-override"], +) +def test_validate_isolation_gate_needs_both_halves( + project, monkeypatch, capsys, override, policy_text +): + """Either half alone is a configuration this release supports and documents: a + `repo_root` override under `isolation = "none"` is the monorepo knob README's + "Worktree isolation" section documents, and + worktree isolation without an override is the ordinary isolated setup. The + gate stays silent on both, and validate still passes — the green rc is the second + witness, since a gate that fired would take the whole verdict with it.""" + from bmad_loop import bmadconfig + + _make_validate_pass(project, monkeypatch, capsys, policy=policy_text) + if override: + _override_repo_root(project) + git(project.project, "commit", "-qam", "repo_root override") + + # The fixture can express the failing value: each leg really does carry exactly + # one half of it, so neither silence is the silence of a setup that never landed. + loaded = bmadconfig.load_paths(project.project) + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + assert (loaded.repo_root != loaded.project) is override + assert (pol.scm.isolation == "worktree") is not override + + assert "policy.isolation-repo-root" not in _validate_findings(project, capsys) + + +def _split_root_project(project, *, policy_text=ISOLATION_WORKTREE_POLICY): + install_bmad_config(project) + _override_repo_root(project) + _write_policy(project.project, policy_text) + write_sprint(project, {"1-1-a": "ready-for-dev"}) + # `worktree_clean` scopes `git status` to `-- .` inside the dir it is handed, so + # only dirt UNDER repo_root can make the next gate speak. Without this the + # ordering assertion below passes no matter where the gate sits. + (project.project / "git-root" / "dirty.txt").write_text("uncommitted\n", encoding="utf-8") + + +@pytest.mark.parametrize("command", ["run", "sweep"]) +def test_start_refuses_worktree_isolation_under_a_repo_root_override( + project, monkeypatch, capsys, command +): + """The refusal validate reports is also the one the real command makes, and it is + the FIRST one: `repo_root` is left genuinely dirty, so a gate ordered after + `worktree_clean` would answer "commit or stash first" instead — a message that + sends the operator to fix something that is not the problem. Both halves of that + are load-bearing: the probe reads `repo_root`, not `project`, so dirtying the + project would prove nothing.""" + _split_root_project(project) + monkeypatch.setattr(cli, "Engine", _StubEngine) + monkeypatch.setattr(cli, "_make_adapters", lambda *a, **k: {r: None for r in cli.ROLES}) + + assert cli.main([command, "--project", str(project.project)]) == 1 + err = capsys.readouterr().err + assert REFUSAL in err + assert "not clean" not in err + + +def test_resume_refuses_worktree_isolation_under_a_repo_root_override(project, monkeypatch, capsys): + """Resume re-reads config.yaml and policy.toml off disk, so it is a second + entrypoint into the same provisioning: a run whose config grew the override + mid-flight must not finish its remaining stories through worktrees the preflight + would now refuse. Refused before the `run-resume` entry, so the journal does not + record a resume that never happened.""" + run_dir = _paused_run_for_resume(project, monkeypatch) + _write_policy(project.project, RESUME_POLICY + '\n[scm]\nisolation = "worktree"\n') + _override_repo_root(project) + monkeypatch.setattr(cli, "Engine", lambda **kw: pytest.fail("engine constructed")) + + assert cli._resume_paused_run(project.project, run_dir) == 1 + assert REFUSAL in capsys.readouterr().err + assert _resume_entries(run_dir) == [] + + +def test_auto_sweep_refuses_worktree_isolation_under_a_repo_root_override(project, monkeypatch): + """The child sweep an engine auto-triggers is the one caller that reloads + policy.toml while reusing the parent's already-loaded paths — so it is the only + way a mid-run flip to `isolation = "worktree"` reaches provisioning under a split + the parent's own start was allowed not to check. + + It RAISES rather than returning, which is the whole point: `_maybe_auto_sweep` + has already journaled `sweep-auto-trigger` and latched the trigger by the time it + calls the factory, and it reads a plain return as success — so a quiet decline is + recorded as `sweep-auto-finished`, a child sweep that ran and finished when none + was launched. Raising lands on the `sweep-auto-failed` + notify path instead, + which is the same one an unparseable policy.toml already takes, and the parent + run is still unaffected (`_maybe_auto_sweep` swallows it).""" + from bmad_loop import bmadconfig + + _split_root_project(project) + started = [] + monkeypatch.setattr(cli, "_start_sweep", lambda *a, **kw: started.append(kw) or 0) + + factory = cli._sweep_factory(project.project, bmadconfig.load_paths(project.project)) + with pytest.raises(RuntimeError, match=REFUSAL): + factory("epic-boundary") + assert started == [] + + +def test_dry_run_banner_names_the_isolation_refusal_first(project, capsys): + """The preview keeps rc 0 and still renders the schedule, but the banner has to + name every refusal the dry-run's early return skips past — and in the order the + real command makes them. (Not every refusal there is: the dirty-tree, queue and + run-id gates are not part of this banner.) This + project is short of base skills too, so the ordering is observable: the isolation + refusal aborts before `_require_base_skills`, so it heads the list.""" + import dataclasses + + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + _write_policy(project.project, ISOLATION_WORKTREE_POLICY) + pol = policy_mod.load(project.project / ".bmad-loop" / "policy.toml") + paths = dataclasses.replace(project, repo_root=project.project / "git-root") + args = argparse.Namespace(epic=None, story=None, max_stories=None) + + assert cli._dry_run(paths, pol, args) == 0 + out, err = capsys.readouterr() + fails = [line for line in err.splitlines() if line.startswith(" FAIL:")] + assert REFUSAL in fails[0] + assert len(fails) > 1, "the base-skill problems the banner already reported" + assert "1-1-a" in out # the schedule itself still rendered diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index 8fa1ccc1..0087c91e 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -14,7 +14,7 @@ from pathlib import Path import pytest -from conftest import install_bmad_config, make_validate_document, write_sprint +from conftest import git, install_bmad_config, make_validate_document, write_sprint from rich.console import Console from rich.text import Text from textual.events import MouseMove @@ -32,7 +32,7 @@ TabbedContent, ) -from bmad_loop import documents +from bmad_loop import bmadconfig, documents from bmad_loop import policy as policy_mod from bmad_loop.adapters.multiplexer import MultiplexerError from bmad_loop.journal import Journal, save_state @@ -1547,6 +1547,78 @@ async def test_dirty_worktree_blocks_launch(project, monkeypatch): assert not calls +def _split_root_tui_project(project): + """The #414 pair, written where the guard reads them. Deliberately left + UNCOMMITTED: the guard is ordered ahead of the clean-tree gate exactly as + `cmd_run` orders it, and a committed fixture could not tell the two orders + apart.""" + install_bmad_config(project) + cfg = project.project / "_bmad" / "bmm" / "config.yaml" + cfg.write_text(cfg.read_text() + "repo_root: '{project-root}/git-root'\n", encoding="utf-8") + (project.project / ".bmad-loop").mkdir(parents=True, exist_ok=True) + (project.project / ".bmad-loop" / "policy.toml").write_text( + '[adapter]\nname = "claude"\n\n[scm]\nisolation = "worktree"\n', encoding="utf-8" + ) + + +async def test_worktree_isolation_under_a_repo_root_override_blocks_launch(project, monkeypatch): + """#414: the TUI launches a detached CLI, and that CLI refuses this combination + itself — this guard exists so the operator gets a toast instead of a pane that + dies immediately. Asserted against the sole producer of the text rather than a + literal, so a reworded message cannot drift this test away from the CLI's.""" + calls = [] + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "start_run_detached", lambda *a, **kw: calls.append(a)) + _split_root_tui_project(project) + expected = bmadconfig.worktree_isolation_conflict( + bmadconfig.load_paths(project.project), "worktree" + ) + assert expected is not None, "the fixture really does carry the conflicting pair" + + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await until(pilot, lambda: isinstance(app.screen, DashboardScreen)) + await pilot.press("r") + await until(pilot, lambda: isinstance(app.screen, StartRunModal)) + await pilot.click(await ready(pilot, "#ok")) + await until(pilot, lambda: expected in notifications(app)) + # The tree is dirty, so this also pins the ORDER: the clean-tree gate would + # otherwise have spoken first and sent the operator to commit something + # that is not the problem. + assert not any("not clean" in m for m in notifications(app)) + assert not calls + + +async def test_unreadable_policy_falls_through_the_isolation_guard(project, monkeypatch): + """The guard's deliberate blind spot, and the one branch where a wrong `except` + tuple silently disables it. It cannot tell "no conflict" from "could not look", + so it defers to the detached CLI, which reads the same two files and fails + loudly on whichever it cannot parse. The bytes here are undecodable rather than + merely malformed: `read_text` raises `UnicodeDecodeError`, which is a ValueError + and NOT an OSError, so it escapes the obvious tuple and would take the TUI down + instead of launching. + + Committed, unlike the sibling above: this one asserts the launch actually + HAPPENS, so the clean-tree gate downstream has to be satisfied or it would + block for an unrelated reason and the fall-through would go unwitnessed.""" + calls = [] + monkeypatch.setattr(launch, "mux_available", lambda: True) + monkeypatch.setattr(launch, "start_run_detached", lambda *a, **kw: calls.append(a)) + _split_root_tui_project(project) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "split roots") + (project.project / ".bmad-loop" / "policy.toml").write_bytes(b'[scm]\nisolation = "\xff\xfe"\n') + + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await until(pilot, lambda: isinstance(app.screen, DashboardScreen)) + await pilot.press("r") + await until(pilot, lambda: isinstance(app.screen, StartRunModal)) + await pilot.click(await ready(pilot, "#ok")) + await until(pilot, lambda: calls) + assert not any("isolation" in m for m in notifications(app)) + + async def test_live_run_asks_for_confirmation(project, monkeypatch): calls = [] monkeypatch.setattr(launch, "mux_available", lambda: True)