fix(worktree): stop shielding tracked files; seed every hook config (#392, #471) - #478
Conversation
…ator (#392) `path_tracked` answers True for a tracked directory prefix as well as a tracked file, and that is load-bearing there — `cmd_validate`'s render-tracked warning probes a directory. The worktree shield needs the opposite of that boolean: the two cases want opposite treatment, so it cannot be served by widening the sibling. Measured, git 2.55.0, with the shield's own private-exclude + worktree-scoped `core.excludesFile` shape: - a pattern naming a tracked regular FILE shields nothing (`git add -A` stages a modification to it anyway — ignore rules apply only to untracked paths) and its only effect is to make the file answer `ls-files -ci --exclude-standard`; - the same pattern over a tracked DIRECTORY genuinely hides new children. `-z` and the bytes accessor rather than the sibling's text call: this reads the output's TEXT, so `core.quotePath` becomes its problem. NUL output is unquoted and `os.fsencode` keeps a locale-undecodable POSIX name comparable (#377). Ablation-verified twice, and the second one only after the first test ablated GREEN. Asserting that an ABSENT metachar path does not read as tracked passes without `:(literal)`, because a glob match answers with the NEIGHBOUR'S name and the set comparison already refuses it. The direction that needs the literal pathspec is the opposite: a metachar path that IS tracked, whose tracked neighbour the bare glob also returns, making a genuine tracked file read False.
…392, #471) Two failures of one blindness: `provision_worktree` built its seed list and its shield pattern set from two unreconciled sources, and neither consulted the index. #392 (external report, production escalation) — the shield wrote a pattern for every path it covers, including a hook config the project TRACKS. Over a tracked file that pattern shields nothing; its only effect is the tracked-and-ignored state repo-hygiene gates reject, so a shield that exists to keep the orchestrator's files OUT of a story commit blocked one instead. Patterns naming a tracked file are now dropped as inert. A tracked DIRECTORY keeps its pattern: measured, that one really does hide new children, so dropping it would leak seeded skills into the commit. Its tracked children still answer `ls-files -ci` and no pattern shape avoids that — `dir/*`, `dir/**` and a trailing negation all measured identical to `dir`, since gitignore cannot re-include under an excluded parent, while the one shape that cleared the report leaked a new file. The tradeoff favors the shield; recorded at the code. This is #384's second half. Its reporter proposed it as their option 3 ("skip any pattern whose path already contains tracked files ... costs nothing"); PR #385 landed option 1 and this half was dropped rather than rejected, which is how a second reporter hit it in production. #471 — `hooks.config_path` was in the shield set and never in the seed set, so whether it got seeded depended on a profile naming the path twice: claude's `seed_files` carries its own `config_path`, codex's does not, and a codex stage therefore ran without the project's hook config. Derived from every non-hookless profile's resolved `config_path` so a new profile cannot regress the same way. Uncertainty KEEPS the pattern and journals the wide shield: a too-wide shield is a cosmetic hygiene complaint, dropping one on a fault leaks work into a commit. Not-a-repo stays silent behind the same `rev-parse --absolute-git-dir` gate the shield already skips on — many callers provision plain directories, and a degrade per call would train operators past the one that matters (caught by an existing test, which is why the gate is there). 6/6 ablations bite. The seed test ablated GREEN written with claude — whose `config_path` is already a `seed_file`, so it could not tell the fix from the bug. The coincidence #471 reports is what makes claude useless as its fixture; codex is the only faithful one.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe change adds exact tracked-file detection, filters inert file shields while retaining directory protection, journals Git inspection failures, and seeds resolved hook configuration paths for non-hookless profiles in isolated worktrees. ChangesWorktree isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Engine
participant IsolatedProvisioning
participant provision_worktree
participant Git
Engine->>IsolatedProvisioning: start worktree provisioning
IsolatedProvisioning->>provision_worktree: resolved hook configuration seed paths
provision_worktree->>Git: inspect shielded paths
Git-->>provision_worktree: tracked-file status or failure
provision_worktree-->>IsolatedProvisioning: filtered shield and provisioned worktree
IsolatedProvisioning-->>Engine: completed worktree
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@CHANGELOG.md`:
- Around line 155-171: Rewrite both Unreleased changelog entries as terse,
imperative release notes describing the worktree git-add shield fix and the
codex hook-config seeding fix. Remove the incident narrative and implementation
detail while preserving issue references `#392` and `#471`.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a4aeb34c-875f-4065-b25a-b917a06616c3
📒 Files selected for processing (7)
CHANGELOG.mddocs/FEATURES.mdsrc/bmad_loop/verify.pysrc/bmad_loop/worktree_flow.pytests/test_engine_worktree.pytests/test_install.pytests/test_verify.py
CodeRabbit (PR #478) flagged both Unreleased entries as narrative rather than terse. Partial accept: the incident detail and the design tradeoff go, but the suggested one-line rewrite drops what a user actually needs to know — a shielded tracked DIRECTORY's children still answer `git ls-files -ci --exclude-standard`, because no pattern shape avoids that. That caveat stays; the reasoning behind it lives at the code.
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.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bcff535ccd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| - Failed-unit forensics: a deferred/escalated unit's worktree + branch stay mounted (`keep_failed`, default on) and its full diff is preserved to `run_dir/failed/<unit>/changes.patch`; `failed_diff_max_mb` caps per-file untracked-file size (oversized skipped with a marker), `failed_diff_unlimited` lifts the cap. | ||
| - Config seeding: a worktree checks out _tracked_ files only, so a project's gitignored MCP/CLI configs (`.mcp.json`, `.claude/settings.json`, `.codex/config.toml`, `.gemini/settings.json`) would be missing — an isolated session couldn't reach its MCP server. With `seed_adapter_defaults` (default on) each loaded adapter's own `seed_files` are copied in from the main repo before the session launches; `worktree_seed` adds extra paths. Copy-when-absent 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). Provisioned tool files (skill trees, hook config, seeded configs) go into a private `.git/worktrees/<id>/info/exclude`, activated by a worktree-scoped `core.excludesFile` that shadows rather than concatenates with your own — so yours is copied into the private one **byte for byte**, the shield covers that unit only, `git worktree remove` takes it away, and the shared, permanent `.git/info/exclude` is never written. An explicitly empty `core.excludesFile` is honored literally — no excludes file at all — rather than read as unset, so git's XDG fallback is not consulted and there is nothing to copy. It then **proves it applies or stands down**: bmad-loop asks git which excludes file it actually resolves, and anything else — ambient command-scope config (`git -c`, `GIT_CONFIG_PARAMETERS`, `GIT_CONFIG_COUNT`), an unreadable excludes file, an unanswerable probe — skips the shield with a journaled and notified reason. Two runs against one repository serialize on an exclusive lock, leaving a zero-length `.git/bmad-loop-shield.lock` — never in the working tree, so nothing your `git add -A` can see; on Windows the wait gives up after ~10s and the shield is skipped naming the lock. Caveats: it needs **git 2.20 or newer** (older git skips the shield, and the repo-format flag is deliberately not written), and enabling it sets `extensions.worktreeConfig` — a **permanent** repo-format flag, rolled back wherever it could be left set without a working shield, but surviving in two cases the reason distinguishes: a sibling worktree still depends on it, or the rollback could not be made at all. Where it cannot be set safely at all (`core.bare = true` or `core.worktree` in the shared config) the shield is skipped instead. Lines an older bmad-loop wrote into `.git/info/exclude` are **not** removed for you — delete them by hand. | ||
| - 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, together with every non-hookless adapter's resolved hook `config_path` — which is gitignored for codex, so without it a codex stage registered no hooks and stalled to timeout (#471); `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. |
There was a problem hiding this comment.
When the hook config is not seeded, provision_worktree still creates profile.hooks.config_path, builds registrations for every configured event, and writes the merged config at worktree_flow.py:632-673; therefore the Codex stage does register bmad-loop's Stop hook. The commit description itself says the reported mechanism is unverified, so documenting “registered no hooks” in the behavior reference gives operators a false diagnosis and overstates what seeding fixes. Describe the verified preservation of the project's existing hook configuration instead, leaving any trust-related explanation explicitly unconfirmed.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — fixed in e3387e7, and it reached further than the line you flagged.
Measured on this branch, confirming your read: the hook step at worktree_flow.py:634-673 runs unconditionally for every non-hookless profile, and merge_hooks({}, registrations, "codex-hooks-json") on an absent config returns changed=True, so the file is created and bmad-loop's own Stop hook registers whether or not the path was seeded.
So the doc gave operators a false diagnosis, as you said. What an unseeded worktree actually loses is the project's hook configuration — the session runs against a file holding the relay registrations alone. That is now what both sites claim.
Two notes beyond the finding:
- The same false claim was in the
worktree_flow.py:953comment, which is not in your diff view. Corrected there too, with the verified/unverified split recorded at the code per this repo's "durable facts belong in docstrings" rule. - Agreed on leaving the trust explanation explicitly unconfirmed. worktree isolation breaks codex stages: .codex/hooks.json is not seeded, so the session registers no hooks and stalls #471's own stated mechanism — that
.codex/hooks.jsonis absent from the worktree — is false at that line; the stall is only consistent with the CLI declining hooks from a config it has not trusted (codex.toml'sfirst_run_note). The comment now says so and says nothing rests on it. The seed is correct either way, which is why no behavior changed.
4273 tests green, pyright clean, trunk check --all clean.
Codex (PR #478) flagged the FEATURES.md claim that an unseeded codex stage 'registered no hooks'. Measured on this branch: provision_worktree's hook step runs unconditionally for every non-hookless profile, and merge_hooks on an absent config returns changed=True, so bmad-loop's own Stop hook is registered whether or not the path was seeded. What an unseeded worktree actually loses is the PROJECT's hook configuration — the session runs against a file holding the relay registrations alone. That is the verified justification; the seed is right either way. #471's own stated mechanism (the file is absent from the worktree) is false at that line, and the stall is only consistent with the CLI declining hooks from a config it has not trusted — unconfirmed, and nothing rests on it. The same false claim was in the worktree_flow.py comment Codex did not see; both are corrected. No behavior change.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@docs/FEATURES.md`:
- Line 78: Update the “Config seeding” documentation to remove the unverified
claim that missing codex config_path caused the session to use a file containing
only bmad-loop relay registrations. Describe only the verified behavior: seeding
preserves the project’s hook configuration and does not determine relay-hook
registration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a32246df-33cd-4dc8-8020-d8cfc84e979e
📒 Files selected for processing (3)
CHANGELOG.mddocs/FEATURES.mdsrc/bmad_loop/worktree_flow.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bmad_loop/worktree_flow.py
- CHANGELOG.md
…gistration (#471) CodeRabbit read the corrected line as still carrying an unverified claim. Its premise is wrong — what the PR leaves unconfirmed is the TRUST mechanism behind #471's stall, not this. That an unseeded worktree runs against a file holding the relay registrations alone is measured: with codex's real profile (config_path .codex/hooks.json, absent from its seed_files), merge_hooks({}, registrations, 'codex-hooks-json') returns changed=True and a config whose only content is those registrations. But the clarifying half of the suggestion is worth having, and is what Codex asked for: the line said what the session runs against without ruling out the old false diagnosis. Now stated outright.
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.
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
1 participant
Two failures of one blindness.
provision_worktreebuilds its seed list (worktree_flow.py:932-948) and its shield pattern set (:611-619) from two unreconciled sources, and neither consults the index.hooks.config_pathwas in the second and never the first.claude is unaffected only by coincidence:
.claude/settings.jsonis both aseed_filesentry and itsconfig_path. codex hasseed_files = [".codex/config.toml"]andconfig_path = ".codex/hooks.json".The #392 half
The shield wrote a pattern for every path it covers, including paths projects legitimately track — the comment above the render site already conceded that. Over a tracked file the pattern shields nothing. Git consults ignore rules only for untracked paths, so
git add -Astages a modification to it regardless; its one effect is to make the file answergit ls-files -ci --exclude-standard, which is what the reporter'sbun run check:repo-hygienerejected.Measured on git 2.55.0 with the shield's real shape (private exclude + worktree-scoped
core.excludesFile), tracked.codex/hooks.json, pattern present:ls-files -ci --exclude-standardin the worktreemainls-files -ci --exclude-standardin the main checkoutgit add -ASo tracked files are free to drop, and tracked directories are not.
The tradeoff, stated plainly
A tracked directory's pattern stays, so its tracked children still read as ignored. No pattern shape avoids that — measured:
-ciclean?dir(current)dir/*dir/**dir+!dir/childdir/*+ negationsgitignore cannot re-include anything under an excluded parent, and the one shape that cleared the report broke the shield. The shield is worth more than the report; the reasoning is recorded at the code so a later round does not "finish" it.
This is #384's second half
#384's reporter proposed exactly this as their option 3 — "skip any pattern whose path already contains tracked files… costs nothing and removes the surprising case entirely." PR #385 landed option 1 (scope + lifetime) and option 3 was dropped rather than rejected, which is how a second external reporter hit it in production. The measurement proving the pattern inert was made later, applied to a read-side helper, and lost when that PR was closed unmerged.
The #471 half
The hook config is now seeded from every non-hookless profile's resolved
config_pathrather than restated per profile, so a new profile cannot regress the same way. The seed loop skips an occupied destination, so a project that tracks the path keeps its checked-out copy..codex/hooks.json";worktree_flow.py:589-604creates and writes it unconditionally. The likely real discriminator is hook trust (codex's ownfirst_run_notewarns untrusted hooks silently never fire) — with the seed,merge_hooksfinds the relay already registered, returnschanged=False, and leaves the project's byte-identical file alone. The seed is correct either way; the mechanism needs measuring with a real codex before anyone builds on it.Failure handling
rev-parse --absolute-git-dirgate_worktree_local_excludealready skips on. Many callers provision plain directories, and a degrade per call would train operators past the one that matters. An existing test caught this during development.Tests
8 new. 6/6 ablations bite — and two of them only after the tests they targeted ablated GREEN:
test_path_tracked_file_reads_a_metachar_name_past_its_glob_neighbour— first written as the sibling's assertion (an absent metachar path must not read tracked). That passes without:(literal), because a glob match answers with the neighbour's name and the set comparison already refuses it. The direction that needs the literal pathspec is the opposite one.test_hook_config_is_seeded_for_every_non_hookless_profile— first written with claude, whoseconfig_pathis already aseed_file, so it could not tell the fix from the bug. The coincidence worktree isolation breaks codex stages: .codex/hooks.json is not seeded, so the session registers no hooks and stalls #471 reports is what makes claude useless as its own fixture; codex is the only faithful one.Suite 4273 green, pyright clean,
trunk checkclean.Issues
Closes #471.
Addresses #392 — the reported case (a tracked hook config file) is fixed and their requested probe is now asserted in both checkouts. Left open on purpose: their ask was "never exclude tracked paths", and the tracked-directory half is a deliberate non-fix above. I have asked the reporter whether that distinction bites them before closing it.
Summary by CodeRabbit
Bug Fixes
Documentation