From a24b39dab8d1fd0b553c3f4f83be990ef0dfb7eb Mon Sep 17 00:00:00 2001 From: Corey Thomas <125082303+Corey-T1000@users.noreply.github.com> Date: Wed, 15 Apr 2026 15:58:48 -0400 Subject: [PATCH] fix(hooks): merge PreToolUse bundles instead of replacing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit InstallHooksConfig used to assign a single-entry slice to hooks.PreToolUse, silently wiping any existing guard hooks (security scanners, subagent auto-approvers, commit formatters, etc.) whenever serve-claude ran with --install-hooks. Now it preserves existing bundles, dedups any prior SpiceBox bundle (identified by hookURL) to stay idempotent across re-installs, and appends ours last so user guards run first and SpiceBox's /check gets the final say. Adds three regression tests: - _PreservesExistingPreToolUseBundles — existing guard survives install - _DedupesOnReinstall — no duplicate SpiceBox entries on repeated installs - _ReinstallPreservesOtherBundles — guards and single SpiceBox coexist assertHookURL was updated to scan for the hook by URL rather than assuming index 0, since the SpiceBox bundle is no longer guaranteed to be first. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/lifecycle/hookinstall.go | 66 ++++++++++-- internal/lifecycle/hookinstall_test.go | 136 ++++++++++++++++++++----- 2 files changed, 167 insertions(+), 35 deletions(-) diff --git a/internal/lifecycle/hookinstall.go b/internal/lifecycle/hookinstall.go index 0ee403d..6c6d17f 100644 --- a/internal/lifecycle/hookinstall.go +++ b/internal/lifecycle/hookinstall.go @@ -8,7 +8,9 @@ import ( ) // InstallHooksConfig reads an existing Claude Code settings JSON file (or creates one) -// and merges in the SpiceBox PreToolUse HTTP hook configuration. +// and merges in the SpiceBox PreToolUse HTTP hook configuration. Existing hooks in +// the PreToolUse array are preserved; if a prior SpiceBox entry (matched by hookURL) +// is present, it is replaced in-place to avoid duplicates on repeated installs. func InstallHooksConfig(path string, hookURL string) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return fmt.Errorf("create directory: %w", err) @@ -29,18 +31,39 @@ func InstallHooksConfig(path string, hookURL string) error { hooks = make(map[string]any) } - hooks["PreToolUse"] = []map[string]any{ - { - "matcher": "*", - "hooks": []map[string]any{ - { - "type": "http", - "url": hookURL, - "timeout": 5000, - }, + spiceboxBundle := map[string]any{ + "matcher": "*", + "hooks": []map[string]any{ + { + "type": "http", + "url": hookURL, + "timeout": 5000, }, }, } + + // Merge: keep existing PreToolUse bundles, drop any prior SpiceBox bundle + // (identified by our hookURL) to prevent duplicates, then append ours last + // so existing guards run first and SpiceBox gets the final say. + var existingPreToolUse []any + switch v := hooks["PreToolUse"].(type) { + case []any: + existingPreToolUse = v + case []map[string]any: + for _, entry := range v { + existingPreToolUse = append(existingPreToolUse, entry) + } + } + + merged := make([]any, 0, len(existingPreToolUse)+1) + for _, entry := range existingPreToolUse { + if bundleContainsHookURL(entry, hookURL) { + continue + } + merged = append(merged, entry) + } + merged = append(merged, spiceboxBundle) + hooks["PreToolUse"] = merged settings["hooks"] = hooks data, err := json.MarshalIndent(settings, "", " ") @@ -51,6 +74,29 @@ func InstallHooksConfig(path string, hookURL string) error { return os.WriteFile(path, data, 0o644) } +// bundleContainsHookURL returns true if the given PreToolUse bundle entry +// contains an HTTP hook matching hookURL. +func bundleContainsHookURL(entry any, hookURL string) bool { + entryMap, ok := entry.(map[string]any) + if !ok { + return false + } + innerHooks, ok := entryMap["hooks"].([]any) + if !ok { + return false + } + for _, h := range innerHooks { + hMap, ok := h.(map[string]any) + if !ok { + continue + } + if hMap["url"] == hookURL { + return true + } + } + return false +} + // UninstallHooksConfig removes the SpiceBox hook entry (identified by hookURL) // from a Claude Code settings JSON file. Other hooks and settings are preserved. func UninstallHooksConfig(path string, hookURL string) error { diff --git a/internal/lifecycle/hookinstall_test.go b/internal/lifecycle/hookinstall_test.go index dfb8f06..cede4bf 100644 --- a/internal/lifecycle/hookinstall_test.go +++ b/internal/lifecycle/hookinstall_test.go @@ -61,16 +61,17 @@ func TestInstallHooksConfig_MergesWithExistingSettings(t *testing.T) { } } -func TestInstallHooksConfig_OverwritesExistingHooks(t *testing.T) { +func TestInstallHooksConfig_PreservesExistingPreToolUseBundles(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "settings.json") - // Write existing hooks config with a different URL. + // Write existing config with a Fort-style guard hook. This is the + // regression case: previously InstallHooksConfig would clobber this. existing := map[string]any{ "hooks": map[string]any{ "PreToolUse": []map[string]any{ - {"matcher": "*", "hooks": []map[string]any{ - {"type": "command", "command": "echo old"}, + {"matcher": "Bash", "hooks": []map[string]any{ + {"type": "command", "command": "/path/to/guard.sh", "timeout": 5000}, }}, }, }, @@ -84,6 +85,82 @@ func TestInstallHooksConfig_OverwritesExistingHooks(t *testing.T) { got := readJSON(t, path) assertHookURL(t, got, newURL) + + // Existing guard must still be present. + hooks := got["hooks"].(map[string]any) + preToolUse := hooks["PreToolUse"].([]any) + if len(preToolUse) != 2 { + t.Fatalf("PreToolUse has %d entries, want 2 (existing guard + spicebox)", len(preToolUse)) + } + + // The first entry should be the preserved guard. + guard := preToolUse[0].(map[string]any) + if guard["matcher"] != "Bash" { + t.Errorf("first entry matcher = %v, want %q (existing guard should come first)", guard["matcher"], "Bash") + } + guardHooks := guard["hooks"].([]any) + guardHook := guardHooks[0].(map[string]any) + if guardHook["command"] != "/path/to/guard.sh" { + t.Errorf("existing guard command = %v, want %q", guardHook["command"], "/path/to/guard.sh") + } +} + +func TestInstallHooksConfig_DedupesOnReinstall(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "settings.json") + hookURL := "http://localhost:9090/check" + + // Install twice. + if err := InstallHooksConfig(path, hookURL); err != nil { + t.Fatalf("first InstallHooksConfig: %v", err) + } + if err := InstallHooksConfig(path, hookURL); err != nil { + t.Fatalf("second InstallHooksConfig: %v", err) + } + + got := readJSON(t, path) + hooks := got["hooks"].(map[string]any) + preToolUse := hooks["PreToolUse"].([]any) + if len(preToolUse) != 1 { + t.Errorf("PreToolUse has %d entries after reinstall, want 1 (dedup)", len(preToolUse)) + } +} + +func TestInstallHooksConfig_ReinstallPreservesOtherBundles(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "settings.json") + hookURL := "http://localhost:9090/check" + + // First install with an existing guard. + existing := map[string]any{ + "hooks": map[string]any{ + "PreToolUse": []map[string]any{ + {"matcher": "Bash", "hooks": []map[string]any{ + {"type": "command", "command": "/path/to/guard.sh"}, + }}, + }, + }, + } + writeJSON(t, path, existing) + if err := InstallHooksConfig(path, hookURL); err != nil { + t.Fatalf("first InstallHooksConfig: %v", err) + } + + // Reinstall (simulates SpiceBox restart). + if err := InstallHooksConfig(path, hookURL); err != nil { + t.Fatalf("second InstallHooksConfig: %v", err) + } + + got := readJSON(t, path) + hooks := got["hooks"].(map[string]any) + preToolUse := hooks["PreToolUse"].([]any) + if len(preToolUse) != 2 { + t.Fatalf("PreToolUse has %d entries after reinstall, want 2 (guard + single spicebox)", len(preToolUse)) + } + guard := preToolUse[0].(map[string]any) + if guard["matcher"] != "Bash" { + t.Errorf("guard lost after reinstall, got first matcher = %v", guard["matcher"]) + } } func TestInstallHooksConfig_InvalidExistingJSON(t *testing.T) { @@ -321,7 +398,9 @@ func writeJSON(t *testing.T, path string, v any) { } // assertHookURL verifies the parsed settings JSON contains a PreToolUse HTTP -// hook pointing at the expected URL. +// hook pointing at the expected URL. The SpiceBox bundle may now be any entry +// in the array (merge semantics), so scan for a matching URL rather than +// assuming index 0. func assertHookURL(t *testing.T, settings map[string]any, wantURL string) { t.Helper() @@ -339,24 +418,31 @@ func assertHookURL(t *testing.T, settings map[string]any, wantURL string) { t.Fatal("PreToolUse is empty") } - entry := preToolUse[0].(map[string]any) - if entry["matcher"] != "*" { - t.Errorf("matcher = %v, want %q", entry["matcher"], "*") - } - - innerHooks := entry["hooks"].([]any) - if len(innerHooks) == 0 { - t.Fatal("inner hooks array is empty") - } - - hook := innerHooks[0].(map[string]any) - if hook["type"] != "http" { - t.Errorf("hook type = %v, want %q", hook["type"], "http") - } - if hook["url"] != wantURL { - t.Errorf("hook url = %v, want %q", hook["url"], wantURL) - } - if hook["timeout"] != float64(5000) { - t.Errorf("hook timeout = %v, want %v", hook["timeout"], 5000) - } + for _, entry := range preToolUse { + entryMap, ok := entry.(map[string]any) + if !ok { + continue + } + if entryMap["matcher"] != "*" { + continue + } + innerHooks, ok := entryMap["hooks"].([]any) + if !ok || len(innerHooks) == 0 { + continue + } + for _, h := range innerHooks { + hook, ok := h.(map[string]any) + if !ok { + continue + } + if hook["type"] == "http" && hook["url"] == wantURL { + if hook["timeout"] != float64(5000) { + t.Errorf("hook timeout = %v, want %v", hook["timeout"], 5000) + } + return // found — all good + } + } + } + + t.Fatalf("no HTTP hook with url=%q found in PreToolUse: %v", wantURL, preToolUse) }