Skip to content

fix(worktree): re-register the relay over a seeded hook config - #354

Draft
Pawel-N-pl wants to merge 1 commit into
bmad-code-org:mainfrom
Pawel-N-pl:fix/worktree-hook-absolute-path
Draft

fix(worktree): re-register the relay over a seeded hook config#354
Pawel-N-pl wants to merge 1 commit into
bmad-code-org:mainfrom
Pawel-N-pl:fix/worktree-hook-absolute-path

Conversation

@Pawel-N-pl

Copy link
Copy Markdown
Contributor

What

provision_worktree now strips the relay out of a seeded hook config before re-registering, so
its own registration is authoritative rather than additive.

Why

.claude/settings.json is both a seeded file and the hook config_path, so it arrives in the
worktree carrying the main repo's $CLAUDE_PROJECT_DIR-relative relay command. That variable
resolves to the worktree, where no .bmad-loop/ relay exists. merge_hooks treats the event as
already 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

  • Add install.strip_relay_hooks(config, dialect), the inverse of merge_hooks, built on the
    existing hook_event_container so every dialect shape is covered. Only RELAY_MARKER
    handlers are removed; a probe-capture hook is deliberate and left alone.
  • Call it from provision_worktree before merge_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_hooks preserves a
project's own handlers sharing the event. Full suite: 3458 passed, 35 skipped.

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.
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@Pawel-N-pl, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 44 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: e6d4914a-19ef-4637-857e-b64d5de24691

📥 Commits

Reviewing files that changed from the base of the PR and between 8a3cf91 and 1f23f5e.

📒 Files selected for processing (3)
  • src/bmad_loop/install.py
  • src/bmad_loop/worktree_flow.py
  • tests/test_install.py
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a silent stall in worktree sessions by ensuring the hook config seeded from the main repo doesn't carry a stale $CLAUDE_PROJECT_DIR-relative relay path into the worktree, where no .bmad-loop/ relay exists.

  • Adds strip_relay_hooks(config, dialect) to install.py, the targeted inverse of merge_hooks: it removes only RELAY_MARKER handlers from the parsed hook container (across all dialect shapes via hook_event_container), preserving probe-capture hooks and any foreign handlers, and returns True if anything was removed.
  • Updates provision_worktree in worktree_flow.py to call strip_relay_hooks before merge_hooks, and writes the file when either call made a change; two new tests cover both the end-to-end rewrite scenario and the foreign-handler-preservation invariant.

Confidence Score: 5/5

Safe 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.

Important Files Changed

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)
Loading

Reviews (2): Last reviewed commit: "fix(worktree): re-register the relay ove..." | Re-trigger Greptile

@Pawel-N-pl
Pawel-N-pl force-pushed the fix/worktree-hook-absolute-path branch from 4d22796 to 1f23f5e Compare July 28, 2026 15:39
@Pawel-N-pl
Pawel-N-pl marked this pull request as draft July 28, 2026 16:12
@Pawel-N-pl

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A seeded .claude/settings.json keeps its $CLAUDE_PROJECT_DIR hook, so worktree runs receive zero hook events

1 participant