From 1f23f5e5b14bd2480611b09d80c2fb4f5fc31bb2 Mon Sep 17 00:00:00 2001 From: Pawel-N-pl Date: Tue, 28 Jul 2026 17:30:43 +0200 Subject: [PATCH] fix(worktree): re-register the relay over a seeded hook config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/bmad_loop/install.py | 32 ++++++++++++++++++++ src/bmad_loop/worktree_flow.py | 12 ++++++-- tests/test_install.py | 54 ++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index 16884b71..c4c2171a 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -614,6 +614,38 @@ def _managed_hook_in_handlers(handlers) -> bool: return RELAY_MARKER in dumped or PROBE_MARKER in dumped +def strip_relay_hooks(config: dict, dialect: str) -> bool: + """Drop every relay registration from a parsed hook config. True if any went. + + The inverse of :func:`merge_hooks`, for the one caller that needs its own + registration to be authoritative rather than additive: a worktree seeded with + the main repo's hook config (``provision_worktree``). That config already + carries a relay command written for the main repo — `$CLAUDE_PROJECT_DIR`-relative + for the claude dialect, which resolves inside the worktree, where no relay + exists. `merge_hooks` will not replace it, since `_managed_hook_in_handlers` + reports the event as already registered, so the stale command has to go first. + + Only RELAY_MARKER handlers are removed. A probe-capture hook is a deliberate, + temporary registration that no worktree seeding produces, and is left alone. + Empty event lists are dropped; an empty container is left in place for + `merge_hooks` to refill. + """ + container = hook_event_container(config, dialect) + removed = False + for native_event in list(container): + handlers = container.get(native_event) + if not isinstance(handlers, list): + continue + kept = [h for h in handlers if RELAY_MARKER not in json.dumps(h)] + if len(kept) != len(handlers): + removed = True + if kept: + container[native_event] = kept + else: + del container[native_event] + return removed + + def relay_registered(config: dict, dialect: str, events: Iterable[str]) -> bool: """True if the bmad-loop relay is registered for any of `events`.""" container = hook_event_container(config, dialect) diff --git a/src/bmad_loop/worktree_flow.py b/src/bmad_loop/worktree_flow.py index 92c3a380..91b3084e 100644 --- a/src/bmad_loop/worktree_flow.py +++ b/src/bmad_loop/worktree_flow.py @@ -35,6 +35,7 @@ _worktree_local_exclude, merge_hooks, resolve_review_layers, + strip_relay_hooks, ) from .model import Phase from .process_host import get_process_host @@ -270,8 +271,15 @@ def provision_worktree( native: f"{interp} {host.shell_quote(str(relay))} {canonical}" for native, canonical in profile.hooks.events.items() } - config, changed = merge_hooks(config, registrations, profile.hooks.dialect) - if changed: + # A seeded config_path (.claude/settings.json is both a seeded file and the + # hook config) arrives carrying the MAIN repo's relay command, which for the + # claude dialect is $CLAUDE_PROJECT_DIR-relative and resolves to a path that + # does not exist inside the worktree. merge_hooks will not replace an + # already-registered relay, so strip it first and let this registration — + # baked to the main repo's relay, absolute — be authoritative. + stripped = strip_relay_hooks(config, profile.hooks.dialect) + config, merged = merge_hooks(config, registrations, profile.hooks.dialect) + if stripped or merged: config_path.write_text(json.dumps(config, indent=2) + "\n", encoding="utf-8") # Shield exactly the paths we wrote (skill trees + hook configs + seeded diff --git a/tests/test_install.py b/tests/test_install.py index 47a08df8..c62b9343 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -13,10 +13,12 @@ DEV_BASE_SKILLS, MODULE_SKILLS, _copy_traversable, + _register_hooks, install_into, merge_hooks, missing_base_skills, provision_worktree, + strip_relay_hooks, ) @@ -405,6 +407,58 @@ def test_provision_worktree_lays_down_skills_and_hook(tmp_path): assert not (wt / ".bmad-loop").exists() +def test_provision_worktree_rewrites_seeded_relative_hook_to_absolute(tmp_path): + """The main repo's .claude/settings.json carries a $CLAUDE_PROJECT_DIR-relative + relay command. Seeded into a worktree, that variable resolves to the worktree, + where no .bmad-loop/ relay exists — the hook fails, no Stop signal ever fires and + the run stalls. The registration must overwrite the seeded command, which + merge_hooks alone will not do (it treats the event as already registered).""" + wt, repo = tmp_path / "wt", tmp_path / "repo" + repo.mkdir() + claude = get_profile("claude") + assert _register_hooks(repo, claude) == 0 + main_settings = json.loads((repo / claude.hooks.config_path).read_text()) + assert "$CLAUDE_PROJECT_DIR" in main_settings["hooks"]["Stop"][0]["hooks"][0]["command"] + + provision_worktree(wt, [claude], repo, seed_files=[claude.hooks.config_path]) + + stop = json.loads((wt / claude.hooks.config_path).read_text())["hooks"]["Stop"] + assert len(stop) == 1 # replaced, not appended alongside + cmd = stop[0]["hooks"][0]["command"] + assert str(repo / ".bmad-loop" / "bmad_loop_hook.py") in cmd + assert "$CLAUDE_PROJECT_DIR" not in cmd + + +def test_strip_relay_hooks_leaves_foreign_handlers(tmp_path): + """Only bmad relay handlers go. A project's own hooks share the event list and + must survive, and an event that held nothing else is dropped rather than left + as an empty list.""" + config = { + "hooks": { + "Stop": [ + { + "matcher": "", + "hooks": [{"type": "command", "command": "python bmad_loop_hook.py Stop"}], + }, + {"matcher": "", "hooks": [{"type": "command", "command": "make lint"}]}, + ], + "SessionStart": [ + { + "matcher": "", + "hooks": [{"type": "command", "command": "python bmad_loop_hook.py start"}], + } + ], + } + } + assert strip_relay_hooks(config, "claude-settings-json") is True + assert config["hooks"]["Stop"] == [ + {"matcher": "", "hooks": [{"type": "command", "command": "make lint"}]} + ] + assert "SessionStart" not in config["hooks"] + # idempotent: nothing left to remove + assert strip_relay_hooks(config, "claude-settings-json") is False + + def test_provision_worktree_covers_multiple_profiles(tmp_path): """Dev=claude + review=codex provisions both skill trees (.claude/skills and .agents/skills) and both hook configs."""