fix(worktree): re-register the relay over a seeded hook config - #354
fix(worktree): re-register the relay over a seeded hook config#354Pawel-N-pl wants to merge 1 commit into
Conversation
A worktree seeded with the main repo's .claude/settings.json inherits its $CLAUDE_PROJECT_DIR-relative relay command, which resolves inside the worktree, where no .bmad-loop/ relay exists. merge_hooks will not replace an already-registered relay, so the broken command survived and the session emitted no hook events at all — the run stalled until the session clock ran out. Add install.strip_relay_hooks (the inverse of merge_hooks, built on hook_event_container) and call it from provision_worktree before re-registering, so the absolute main-relay command is authoritative. Probe-capture hooks are deliberately left alone. Claude dialect in practice: codex/gemini expose no $CLAUDE_PROJECT_DIR equivalent and already bake an absolute path.
|
Warning Review limit reached
Next review available in: 44 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. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR fixes a silent stall in worktree sessions by ensuring the hook config seeded from the main repo doesn't carry a stale
Confidence Score: 5/5Safe to merge; the change is narrow and well-tested with an end-to-end test confirmed to fail without the fix. The new strip_relay_hooks function is a straightforward in-place mutation of the hook container dict, using the same hook_event_container abstraction that all other hook operations rely on, so all dialect shapes are covered consistently. The only call site is provision_worktree, which already owned the read/write cycle for the config file. The file-write guard (stripped or merged) is correct and handles all four cases. Two tests exercise the exact regression scenario and the foreign-handler invariant. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/install.py | Adds strip_relay_hooks: correctly uses hook_event_container for dialect abstraction, mutates the container in-place, preserves foreign handlers, and drops empty event lists. |
| src/bmad_loop/worktree_flow.py | Minimal, targeted change: calls strip_relay_hooks before merge_hooks and gates the file write on either returning a change. |
| tests/test_install.py | Two new tests added: end-to-end round-trip and unit test for strip_relay_hooks covering foreign-handler preservation, empty-event-list removal, and idempotency. |
Sequence Diagram
sequenceDiagram
participant PW as provision_worktree
participant FS as config_path (worktree)
participant SR as strip_relay_hooks
participant MH as merge_hooks
PW->>FS: read seeded settings.json
FS-->>PW: config dict with stale relay command
PW->>SR: strip_relay_hooks(config, dialect)
SR-->>PW: "removed=True (stale relay stripped in-place)"
PW->>MH: merge_hooks(config, registrations, dialect)
MH-->>PW: "config dict with absolute relay, merged=True"
PW->>FS: write_text (stripped OR merged)
Reviews (2): Last reviewed commit: "fix(worktree): re-register the relay ove..." | Re-trigger Greptile
4d22796 to
1f23f5e
Compare
|
Moving this to draft — I gather a larger rework is planned, so I'd rather not sit in your review queue against code that's about to change. The branch stays up and CI is green (py3.11–3.14, Windows, trunk, pyright). Happy to rebase it onto whatever lands, or close it if the rework covers this — just say the word. Issue #352 stays open either way, since the defect outlives the patch. |
What
provision_worktreenow strips the relay out of a seeded hook config before re-registering, soits own registration is authoritative rather than additive.
Why
.claude/settings.jsonis both a seeded file and the hookconfig_path, so it arrives in theworktree carrying the main repo's
$CLAUDE_PROJECT_DIR-relative relay command. That variableresolves to the worktree, where no
.bmad-loop/relay exists.merge_hookstreats the event asalready registered and leaves the stale command in place, so the session emits no hook events at
all and the run stalls until the session clock expires — silently, since an idle session looks
the same as a working one.
Fixes #352.
How
install.strip_relay_hooks(config, dialect), the inverse ofmerge_hooks, built on theexisting
hook_event_containerso every dialect shape is covered. OnlyRELAY_MARKERhandlers are removed; a probe-capture hook is deliberate and left alone.
provision_worktreebeforemerge_hooks, writing the file when either changed it.Testing
Two tests, the end-to-end one confirmed to fail with the fix reverted: a seeded config ends up
with the absolute main-relay command and no duplicate handler, and
strip_relay_hookspreserves aproject's own handlers sharing the event. Full suite: 3458 passed, 35 skipped.