feat(cli,tui): refuse worktree isolation under a repo_root override (#433, 6E) - #445
Conversation
…433, 6E) Provisioning seeds every off-disk surface from repo_root while init, validate and the run preflight probe project, so a project that set both got a green preflight and an isolated session with no dev primitive — no result, and no journal line naming the cause. Five call sites, one message: _reject_isolation_conflict aborts cmd_run, cmd_sweep and _resume_paused_run; _sweep_factory RAISES instead, so the parent journals sweep-auto-failed rather than recording a child sweep that never ran; cmd_validate reports it as policy.isolation-repo-root and keeps going; _warn_preflight_would_abort names it first in the dry-run banner; the TUI toasts it ahead of the clean-tree gate, since "commit or stash first" would send the operator to fix something that is not the problem. _warn_preflight_would_abort now takes ProjectPaths rather than project alone — the conflict is a fact about the two roots' relationship. Its problem-severity filter is main's and stays: the 0.9.x banner had none, and dropping it would print warnings as FAIL lines. Refs #433, #414. The plumbing that retires this is #443.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. WalkthroughThe change rejects ChangesWorktree isolation conflict
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant bmadconfig
participant CommandFlow
CLI->>bmadconfig: Check project paths and isolation
bmadconfig-->>CLI: Return conflict message or None
CLI->>CommandFlow: Reject launch or continue preflight
CommandFlow-->>CLI: Record child-sweep failure or render dry-run result
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
263-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten this entry to match the terse changelog style.
This entry runs 11 lines and explains internal mechanism details (why
repo_root/_bmad/doesn't exist, why the seed-completeness gates go inert). A changelog reader needs the user-facing behavior change, not the internal rationale already covered in theworktree_isolation_conflictdocstring.✏️ Proposed terser entry
-- **`isolation = "worktree"` under a `repo_root` override is refused instead of running empty - (`#414`).** Worktree provisioning seeds every off-disk surface from `repo_root` — skill trees, - `_bmad/` and the `_bmad/custom/` overrides in it, each `seed_files`/`seed_globs` entry — and bakes - the hook relay's absolute path from it, while `init`, `validate` and the run preflight write and - probe those same surfaces under `project`. `load_paths` requires `project/_bmad/bmm/config.yaml`, - so `repo_root/_bmad/` generally does not exist at all. A project that set both got a green - preflight and then an isolated session with no dev primitive, stopping with no result and nothing - journaled naming the cause; the seed-completeness gates went inert rather than fired. `validate` - now reports the pair, `run`/`sweep`/`resume` and the auto-triggered child sweep refuse to start, - the dry-run banner names it first, and the TUI toasts it ahead of its clean-tree gate. Plumbing - `project` through provisioning so both work together is `#443`. +- **Refuse `isolation = "worktree"` combined with a `repo_root` override (`#414`).** The combination + previously produced a green preflight followed by an isolated session with no dev primitive and no + journaled cause. `validate` now reports the conflict; `run`, `sweep`, `resume`, and the + auto-triggered child sweep refuse to start; the dry-run banner names it first; the TUI shows it + before the clean-tree gate. Plumbing `project` through provisioning to support both together is + tracked as `#443`.As per coding guidelines, "Write Unreleased changelog entries tersely, scannably, and imperatively."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 263 - 274, Shorten the changelog entry for the worktree isolation and repo_root conflict to a concise, imperative statement covering the user-facing behavior: the conflicting configuration is detected and refused by validation, run-related commands, preflight, dry-run, and TUI flows. Remove internal provisioning, path, seed, and failure-mechanism details, and retain the issue reference.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 263-274: Shorten the changelog entry for the worktree isolation
and repo_root conflict to a concise, imperative statement covering the
user-facing behavior: the conflicting configuration is detected and refused by
validation, run-related commands, preflight, dry-run, and TUI flows. Remove
internal provisioning, path, seed, and failure-mechanism details, and retain the
issue reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7140f214-960f-4935-87fa-b87a3698b34f
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mddocs/FEATURES.mdsrc/bmad_loop/bmadconfig.pysrc/bmad_loop/checks.pysrc/bmad_loop/cli.pysrc/bmad_loop/tui/app.pytests/test_bmadconfig.pytests/test_cli.pytests/test_tui_app.py
The mechanism it spelled out is already verbatim in worktree_isolation_conflict's docstring; the entry keeps the behavior change and all five surfaces.
There was a problem hiding this comment.
pbean has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Taking this one — applied in Worth saying why, since I declined the same-shaped finding on #441 two hours ago and do not want that to read as inconsistency. There I measured the section: the entries sat at the 64th and 14th percentile of the 42 This proposal is 6 lines, comfortably inside that distribution. So both versions were in-band, which stopped it being a measurable question and made it a judgement one — and on the merits you are right. What your version drops is the mechanism (why Kept everything user-facing: the behaviour change, all five refusal surfaces, and the #443 pointer for the plumbing that retires this. No other findings this round. |
Sub-phase 6E of the 0.9.1 forward-port (#433): ports the #414 refusal, not the plumbing.
Worktree provisioning reads
paths.repo_rootfor every surface it seeds off disk — the upstream skill trees,_bmad/and the_bmad/custom/overrides inside it, eachseed_files/seed_globsentry — and bakes the hook relay's absolute path from it into the worktree's hook config.init,validateand the run preflight write and probe those same surfaces underpaths.project.bmadconfig.load_pathsrequiresproject/_bmad/bmm/config.yaml, so_bmad/is underprojectby definition andrepo_root/_bmad/generally does not exist at all.The two are the same path by default, so the split is invisible — until
_bmad/bmm/config.yamlsetsrepo_root, which is the documented monorepo knob. Then the preflight approves a surface the isolated run never receives, and the seed-completeness gates go inert rather than fire: the session dispatches into a worktree with no dev primitive, and stops with no result and nothing journaled naming the cause.Why a refusal and not the fix
Deliberate scope call, carried over from the 0.9.1 line. Shape (1) — plumb
projectthrough provisioning — rewrites every disk read inprovision_worktree, which is character-for-character the diff of two later sub-phases, and "which root wins" is a separate decision per seeded surface: the relay only exists underproject, but operator-configuredseed_filesmay legitimately name a path outside it and inside the git root.ProjectPaths.rebasedencodesproject == repo_rootbesides, so it cannot express the divergent case at all.That work is filed as #443, which names every line this PR adds as something it deletes.
worktree_isolation_conflict's docstring says so in the source, so a later reader does not mistake the refusal for the intended end state.What lands
bmadconfig.worktree_isolation_conflict(paths, isolation) -> str | Noneis the sole producer of the text, so the five consumers cannot drift:cmd_run,cmd_sweep,_resume_paused_run_reject_isolation_conflict→ rc 1_sweep_factory.factoryRuntimeErrorcmd_validatereport.fail("policy.isolation-repo-root", …), keeps going_warn_preflight_would_abortproblems.insert(0, …)— named firsttui/app.py::_guardedOrdering is load-bearing in three places and each is pinned by a test:
cmd_run/cmd_sweepand in the TUI. This refusal 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. The fixtures leave the tree genuinely dirty so a wrongly-ordered gate speaks instead._sweep_factoryraises rather than returns. By the time the engine calls the factory it has latched the trigger and journaledsweep-auto-trigger, and it reads a plain return as success — a bare decline would be recorded assweep-auto-finished, i.e. a child sweep that ran and finished when none was ever launched. Raising lands on thesweep-auto-failed+ notify path an unparseablepolicy.tomlalready takes.validatereports rather than aborts, so its other gates still run._resume_paused_rungets it too: resume re-reads both files off disk, so a run started before the override was added must not finish its remaining stories through provisioning the preflight would now refuse. It is refused before therun-resumejournal entry, so the journal does not record a resume that never happened.Main-side adaptations (this is a re-implementation, not a cherry-pick)
_warn_preflight_would_abortkeeps main'sseverity == "problem"filter. The 0.9.x version had none. Main additionally carriestest_dry_run_banner_stays_silent_for_a_warning_only_finding, whose docstring calls the filter "a deliberate correction, not a port artefact" — so only the parameter changed (project: Path→ProjectPaths, at all three call sites) and the insert was added. Dropping the filter would have printed warnings asFAIL:lines._require_base_skillsgates on" became narrower than the behaviour once the banner also reported the isolation conflict — which_require_base_skillsdoes not check. Rewritten to name both gates it mirrors, and to say which refusals are deliberately not in the banner (dirty tree, queue, run id)._findings_by_checkdoes not exist on main; its equivalent is_validate_findings._make_validate_passtakespolicy=, notpolicy_text=._render_findingswas inlined in one TUI-render test on main and is now a helper.except (BmadConfigError, PolicyError, OSError)fall-through in the TUI guard depends on both loaders convertingUnicodeDecodeError(aValueError, not anOSError) into their own typed error. Verified present on main atpolicy.py:684andbmadconfig.py:125— they landed in 6C — and an ablation row proves thePolicyErrorleg is reachable, not merely present.install.pyciting these guards. Main has none of those helpers yet, so it has no home here; it is recorded on Plumbpaths.projectthrough worktree provisioning sorepo_root != projectis supported, and retire the refusal #443.Verification
main's 3953 + the 11 tests added here. The 4 reds are pre-existingtest_module_skills_synclocal skill-drift on the dev box and do not occur on a clean checkout.uv run pyright0 errors, 0 warnings, 0 informations — zero delta frommain.checks.pyis in the strict list and gained an id only, never logic.trunk check --all --no-fix— 242 files, no issues.Ablation matrix — every row against the whole suite
Collected total constant at 3964 on all 11 runs (no baseline drift); the full
src/digest was re-verified pristine after every row.cmd_runrefusaltest_start_refuses_…[run]— fails onassert REFUSAL in err, having got "git worktree is not clean": the later gate speaks, which is exactly the ordering claimcmd_sweeprefusaltest_start_refuses_…[sweep]— no cross-bleed between the two params_resume_paused_runrefusaltest_resume_refuses_…— failsengine constructed, i.e. resume proceeded to build_sweep_factoryraisetest_auto_sweep_refuses_…—DID NOT RAISE RuntimeErrorcmd_validatefindingtest_validate_refuses_…— rc 0 where 1 was assertedtest_dry_run_banner_names_the_isolation_refusal_first— banner still prints, so this pins the isolation line, not banner silence_guardedrefusaltest_worktree_isolation_…_blocks_launchexcepttuple narrowed to dropPolicyErrortest_unreadable_policy_falls_through_…— uncaughtPolicyErrortakes the app down.resolve()on both sides of the path comparetest_worktree_isolation_conflict_compares_normalized_pathsVALIDATE_CHECKSidtest_validate_refuses_…— the registry assert bites, so a future id cannot skip registrationEvery row reddens exactly one test, inside the edit's blast radius, and no row reddened nothing.
Row H is the one that matters most for a refusal gate: a false positive here is worse than the bug it forecloses, since it would refuse an ordinary isolated project whose
repo_rootmerely spells the same directory a different way.load_pathsresolves both sides, but nothing obliges a hand-builtProjectPaths— or a future caller — to have done so.Refs #433, #414. The plumbing is tracked as #443.
Summary by CodeRabbit
Bug Fixes
Documentation