From 71736dac0aaf615ec3e7032a769f6de5bbd9fbf5 Mon Sep 17 00:00:00 2001 From: blindndangerous <20344049+blindndangerous@users.noreply.github.com> Date: Tue, 2 Jun 2026 07:00:45 -0600 Subject: [PATCH 1/2] fix: preserve hooks during claudio reinstall --- .github/workflows/ci.yml | 2 +- internal/install/coverage_test.go | 199 ++++++++++++++++++++++++++++++ internal/install/hooks.go | 112 ++++++++++++----- 3 files changed, 282 insertions(+), 31 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2dda5bf..4bd694b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,7 +72,7 @@ jobs: # These packages have no cgo dependency, so no ALSA headers are needed. - name: Enforce coverage floor run: | - THRESHOLD=87.0 + THRESHOLD=88.5 status=0 for pkg in ./internal/hooks ./internal/install; do line=$(go test "$pkg" -count=1 -cover) diff --git a/internal/install/coverage_test.go b/internal/install/coverage_test.go index 017784a..8846853 100644 --- a/internal/install/coverage_test.go +++ b/internal/install/coverage_test.go @@ -140,6 +140,33 @@ func TestWriteSettingsFileRoundTrip(t *testing.T) { } } +func TestWriteSettingsFileMarshalError(t *testing.T) { + fsys := afero.NewMemMapFs() + settings := &SettingsMap{"bad": make(chan int)} + if err := WriteSettingsFile(fsys, "/settings.json", settings); err == nil { + t.Error("expected marshal error for unsupported settings value") + } +} + +func TestWriteSettingsFilePreservesExistingPermissions(t *testing.T) { + fsys := afero.NewMemMapFs() + path := "/settings.json" + if err := afero.WriteFile(fsys, path, []byte(`{"old":true}`), 0600); err != nil { + t.Fatal(err) + } + settings := &SettingsMap{"new": true} + if err := WriteSettingsFile(fsys, path, settings); err != nil { + t.Fatal(err) + } + info, err := fsys.Stat(path) + if err != nil { + t.Fatal(err) + } + if got := info.Mode() & os.ModePerm; got != 0600 { + t.Errorf("mode = %v, want 0600", got) + } +} + func TestMergeHookValuesStringFormatExisting(t *testing.T) { // Existing hook in legacy string format (non-claudio) must be preserved // and merged into array form alongside the claudio command. @@ -178,6 +205,129 @@ func TestMergeHookValuesStringFormatExisting(t *testing.T) { } } +func TestMergeHooksRefreshesClaudioWithoutDroppingExistingHooks(t *testing.T) { + existing := &SettingsMap{ + "hooks": map[string]interface{}{ + "PreToolUse": []interface{}{ + map[string]interface{}{ + "matcher": ".*", + "hooks": []interface{}{ + map[string]interface{}{"command": "/usr/bin/logger"}, + }, + }, + map[string]interface{}{ + "matcher": "*", + "hooks": []interface{}{ + map[string]interface{}{"command": "/old/claudio"}, + }, + }, + }, + }, + } + claudioHooks, err := GenerateClaudioHooksForAgent(afero.NewMemMapFs(), "/new/claudio", AgentCodex) + if err != nil { + t.Fatal(err) + } + merged, err := MergeHooksIntoSettings(existing, claudioHooks) + if err != nil { + t.Fatal(err) + } + hooksSection := (*merged)["hooks"].(map[string]interface{}) + arr := hooksSection["PreToolUse"].([]interface{}) + + foundLogger := false + foundOldClaudio := false + foundNewClaudio := false + for _, e := range arr { + cfg := e.(map[string]interface{}) + for _, h := range cfg["hooks"].([]interface{}) { + switch h.(map[string]interface{})["command"] { + case "/usr/bin/logger": + foundLogger = true + case "/old/claudio": + foundOldClaudio = true + case "/new/claudio": + foundNewClaudio = true + } + } + } + if !foundLogger { + t.Error("existing non-claudio hook was dropped") + } + if foundOldClaudio { + t.Error("old claudio hook was not refreshed") + } + if !foundNewClaudio { + t.Error("new claudio hook missing after refresh") + } +} + +func TestRemoveClaudioCommandsPreservesNonClaudioEntries(t *testing.T) { + entries := []interface{}{ + "raw-entry", + map[string]interface{}{"matcher": "*"}, + map[string]interface{}{ + "matcher": "mixed", + "hooks": []interface{}{ + "raw-hook", + map[string]interface{}{"command": 42}, + map[string]interface{}{"command": "/old/claudio"}, + map[string]interface{}{"command": "/usr/bin/logger"}, + }, + }, + map[string]interface{}{ + "matcher": "claudio-only", + "hooks": []interface{}{ + map[string]interface{}{"command": "/old/claudio"}, + }, + }, + } + + filtered := removeClaudioCommands(entries) + if len(filtered) != 3 { + t.Fatalf("filtered entry count = %d, want 3: %#v", len(filtered), filtered) + } + + foundLogger := false + foundOldClaudio := false + foundRawHook := false + foundNumericCommand := false + for _, entry := range filtered { + cfg, ok := entry.(map[string]interface{}) + if !ok { + continue + } + hooksList, ok := cfg["hooks"].([]interface{}) + if !ok { + continue + } + for _, hook := range hooksList { + if hook == "raw-hook" { + foundRawHook = true + continue + } + cmd, ok := hook.(map[string]interface{}) + if !ok { + continue + } + switch cmd["command"] { + case 42: + foundNumericCommand = true + case "/old/claudio": + foundOldClaudio = true + case "/usr/bin/logger": + foundLogger = true + } + } + } + if !foundLogger || !foundRawHook || !foundNumericCommand { + t.Errorf("non-claudio content not preserved: logger=%v raw=%v numeric=%v", foundLogger, foundRawHook, foundNumericCommand) + } + if foundOldClaudio { + t.Error("old claudio command was not removed") + } +} + func TestIsClaudioHookFormats(t *testing.T) { if !IsClaudioHook("/usr/local/bin/claudio") { t.Error("expected string claudio command recognized") @@ -200,6 +350,55 @@ func TestIsClaudioHookFormats(t *testing.T) { } } +func TestIsClaudioHookFindsClaudioInMergedHookArrays(t *testing.T) { + cases := []struct { + name string + arr []interface{} + }{ + { + name: "claudio after existing hook", + arr: []interface{}{ + map[string]interface{}{ + "matcher": ".*", + "hooks": []interface{}{ + map[string]interface{}{"command": "/usr/bin/logger"}, + }, + }, + map[string]interface{}{ + "matcher": "*", + "hooks": []interface{}{ + map[string]interface{}{"command": "/usr/local/bin/claudio"}, + }, + }, + }, + }, + { + name: "claudio before existing hook", + arr: []interface{}{ + map[string]interface{}{ + "matcher": "*", + "hooks": []interface{}{ + map[string]interface{}{"command": `C:\tools\claudio.exe`}, + }, + }, + map[string]interface{}{ + "matcher": ".*", + "hooks": []interface{}{ + map[string]interface{}{"command": "/usr/bin/logger"}, + }, + }, + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if !IsClaudioHook(tc.arr) { + t.Error("expected merged hook array to be recognized when any entry is claudio") + } + }) + } +} + func TestMergeHooksMarshalErrorPropagates(t *testing.T) { // A channel value cannot be JSON-marshaled, forcing deepCopySettings to error. bad := &SettingsMap{"x": make(chan int)} diff --git a/internal/install/hooks.go b/internal/install/hooks.go index 6216673..925c854 100644 --- a/internal/install/hooks.go +++ b/internal/install/hooks.go @@ -6,6 +6,7 @@ import ( "log/slog" "path/filepath" "runtime" + "strings" "claudio.click/internal/fs" "github.com/spf13/afero" @@ -14,7 +15,6 @@ import ( // HooksMap represents the hooks section of Claude Code settings type HooksMap map[string]interface{} - // GenerateClaudioHooks creates the Claude Code hook configuration (backward-compatible default). func GenerateClaudioHooks(filesystem afero.Fs, executablePath string) (interface{}, error) { return GenerateClaudioHooksForAgent(filesystem, executablePath, AgentClaude) @@ -138,15 +138,12 @@ func MergeHooksIntoSettings(existingSettings *SettingsMap, claudioHooks interfac // Then, add/update Claudio hooks with proper merging for hookName, claudioValue := range claudioHooksMap { if existingValue, exists := mergedHooks[hookName]; exists { - if !IsClaudioHook(existingValue) { - // Merge existing non-Claudio hook with Claudio hook - mergedHooks[hookName] = mergeHookValues(existingValue, claudioValue) + mergedHooks[hookName] = mergeHookValues(existingValue, claudioValue) + if IsClaudioHook(existingValue) { + slog.Debug("refreshed existing Claudio hook", "hook_name", hookName) + } else { slog.Info("merged existing non-Claudio hook with Claudio", "hook_name", hookName, "action", "merging") - } else { - // Existing is Claudio - idempotent update - mergedHooks[hookName] = claudioValue - slog.Debug("updated existing Claudio hook", "hook_name", hookName) } } else { // No conflict - add new Claudio hook @@ -190,12 +187,6 @@ func deepCopySettings(original *SettingsMap) (*SettingsMap, error) { func mergeHookValues(existingValue, claudioValue interface{}) interface{} { slog.Debug("merging hook values", "existing_type", fmt.Sprintf("%T", existingValue), "claudio_type", fmt.Sprintf("%T", claudioValue)) - // If existing value is already a Claudio hook, don't duplicate - return Claudio value (idempotent) - if IsClaudioHook(existingValue) { - slog.Debug("existing value is Claudio hook, returning Claudio value (idempotent)") - return claudioValue - } - // Convert Claudio value to array format (it should already be, but be safe) claudioArray, ok := claudioValue.([]interface{}) if !ok { @@ -206,6 +197,10 @@ func mergeHookValues(existingValue, claudioValue interface{}) interface{} { // Convert existing value to array format var existingArray []interface{} if existingStr, ok := existingValue.(string); ok { + if isClaudioCommand(existingStr) { + slog.Debug("replacing existing string Claudio hook") + return claudioValue + } // Convert string hook to array format existingArray = []interface{}{ map[string]interface{}{ @@ -221,7 +216,7 @@ func mergeHookValues(existingValue, claudioValue interface{}) interface{} { slog.Debug("converted existing string hook to array format", "command", existingStr) } else if existingArr, ok := existingValue.([]interface{}); ok { // Already in array format - existingArray = existingArr + existingArray = removeClaudioCommands(existingArr) slog.Debug("existing hook already in array format") } else { slog.Warn("unknown existing hook format, treating as string", "type", fmt.Sprintf("%T", existingValue)) @@ -248,7 +243,7 @@ func mergeHookValues(existingValue, claudioValue interface{}) interface{} { // Add Claudio array elements mergedArray = append(mergedArray, claudioArray...) - slog.Debug("completed hook value merge", + slog.Debug("completed hook value merge", "existing_elements", len(existingArray), "claudio_elements", len(claudioArray), "merged_elements", len(mergedArray)) @@ -256,20 +251,56 @@ func mergeHookValues(existingValue, claudioValue interface{}) interface{} { return mergedArray } -// IsClaudioHook checks if a hook value represents a claudio hook, -// supporting both the old string format and new array format -func IsClaudioHook(hookValue interface{}) bool { - // Helper function to check if command is a claudio executable - isClaudioCommand := func(cmdStr string) bool { - // Strip quotes if present (for Windows compatibility) - if len(cmdStr) >= 2 && cmdStr[0] == '"' && cmdStr[len(cmdStr)-1] == '"' { - cmdStr = cmdStr[1 : len(cmdStr)-1] +func removeClaudioCommands(entries []interface{}) []interface{} { + filtered := make([]interface{}, 0, len(entries)) + for _, entry := range entries { + config, ok := entry.(map[string]interface{}) + if !ok { + filtered = append(filtered, entry) + continue + } + hooksList, ok := config["hooks"].([]interface{}) + if !ok { + filtered = append(filtered, entry) + continue + } + + keptHooks := make([]interface{}, 0, len(hooksList)) + removed := false + for _, hook := range hooksList { + cmd, ok := hook.(map[string]interface{}) + if !ok { + keptHooks = append(keptHooks, hook) + continue + } + cmdStr, ok := cmd["command"].(string) + if ok && isClaudioCommand(cmdStr) { + removed = true + continue + } + keptHooks = append(keptHooks, hook) + } + if len(keptHooks) == 0 { + continue } - baseName := filepath.Base(cmdStr) - // Handle production "claudio" and "claudio.exe" (Windows) and test executables "install.test", "uninstall.test", "cli.test" - return baseName == "claudio" || baseName == "claudio.exe" || baseName == "install.test" || baseName == "uninstall.test" || baseName == "cli.test" + if !removed { + filtered = append(filtered, entry) + continue + } + + configCopy := make(map[string]interface{}, len(config)) + for key, value := range config { + configCopy[key] = value + } + configCopy["hooks"] = keptHooks + filtered = append(filtered, configCopy) } + return filtered +} +// IsClaudioHook checks if a hook value represents a claudio hook, +// supporting both the old string format and new array format +func IsClaudioHook(hookValue interface{}) bool { // Check old string format (backward compatibility) if str, ok := hookValue.(string); ok { return isClaudioCommand(str) @@ -277,11 +308,21 @@ func IsClaudioHook(hookValue interface{}) bool { // Check new array format if arr, ok := hookValue.([]interface{}); ok && len(arr) > 0 { - if config, ok := arr[0].(map[string]interface{}); ok { + for _, item := range arr { + config, ok := item.(map[string]interface{}) + if !ok { + continue + } if hooks, ok := config["hooks"].([]interface{}); ok && len(hooks) > 0 { - if cmd, ok := hooks[0].(map[string]interface{}); ok { + for _, hook := range hooks { + cmd, ok := hook.(map[string]interface{}) + if !ok { + continue + } if cmdStr, ok := cmd["command"].(string); ok { - return isClaudioCommand(cmdStr) + if isClaudioCommand(cmdStr) { + return true + } } } } @@ -291,6 +332,17 @@ func IsClaudioHook(hookValue interface{}) bool { return false } +func isClaudioCommand(cmdStr string) bool { + // Strip quotes if present (for Windows compatibility) + if len(cmdStr) >= 2 && cmdStr[0] == '"' && cmdStr[len(cmdStr)-1] == '"' { + cmdStr = cmdStr[1 : len(cmdStr)-1] + } + cmdStr = strings.ReplaceAll(cmdStr, "\\", "/") + baseName := filepath.Base(cmdStr) + // Handle production "claudio" and "claudio.exe" (Windows) and test executables "install.test", "uninstall.test", "cli.test" + return baseName == "claudio" || baseName == "claudio.exe" || baseName == "install.test" || baseName == "uninstall.test" || baseName == "cli.test" +} + // GetExecutablePath returns the current executable path using filesystem abstraction. // On Windows the result is converted to forward slashes so that the path works // when Claude Code invokes the hook command through bash. From d38a578e00ac6e71493875801320075e5dcb677d Mon Sep 17 00:00:00 2001 From: blindndangerous <20344049+blindndangerous@users.noreply.github.com> Date: Tue, 2 Jun 2026 12:53:46 -0600 Subject: [PATCH 2/2] test: make audio command tests portable --- internal/audio/backend_test.go | 17 ++++++++--- internal/audio/platform_test.go | 29 ++++++++++++------- internal/audio/system_command_backend_test.go | 4 +-- 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/internal/audio/backend_test.go b/internal/audio/backend_test.go index d0933e0..edce11c 100644 --- a/internal/audio/backend_test.go +++ b/internal/audio/backend_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "io" + "runtime" "strings" "testing" "time" @@ -83,6 +84,7 @@ func TestSystemCommandBackend_Interface(t *testing.T) { } func TestSystemCommandBackend_Play(t *testing.T) { + successCommand := successfulNoopCommand() tests := []struct { name string command string @@ -91,15 +93,15 @@ func TestSystemCommandBackend_Play(t *testing.T) { }{ { name: "play file source with nonexistent file", - command: "echo", // Use echo instead of paplay to avoid system dependencies + command: successCommand, source: NewFileSource("/test/sound.wav"), - wantErr: false, // echo will succeed even with nonexistent args + wantErr: false, }, { name: "play reader source", - command: "echo", // Use echo instead of paplay + command: successCommand, source: NewReaderSource(io.NopCloser(strings.NewReader("test")), "wav"), - wantErr: false, // echo will succeed + wantErr: false, }, { name: "invalid command", @@ -127,6 +129,13 @@ func TestSystemCommandBackend_Play(t *testing.T) { } } +func successfulNoopCommand() string { + if runtime.GOOS == "windows" { + return "cmd.exe" + } + return "true" +} + func TestSystemCommandBackend_Lifecycle(t *testing.T) { backend := NewSystemCommandBackend("paplay") diff --git a/internal/audio/platform_test.go b/internal/audio/platform_test.go index 86ef5ae..7ffb974 100644 --- a/internal/audio/platform_test.go +++ b/internal/audio/platform_test.go @@ -1,6 +1,7 @@ package audio import ( + "runtime" "testing" ) @@ -13,19 +14,20 @@ func TestPlatformDetectionInterface(t *testing.T) { } func TestCommandExists(t *testing.T) { + firstExisting, secondExisting := existingCommandFixtures() tests := []struct { name string command string expected bool }{ { - name: "existing command - echo", - command: "echo", + name: "existing command - primary", + command: firstExisting, expected: true, }, { - name: "existing command - ls", - command: "ls", + name: "existing command - secondary", + command: secondExisting, expected: true, }, { @@ -190,15 +192,13 @@ func TestGetPreferredSystemCommand(t *testing.T) { // TestRealSystemIntegration tests against the real system (these may vary by environment) func TestRealSystemIntegration(t *testing.T) { t.Run("real command detection", func(t *testing.T) { - // Test some commands that should exist on most systems - echoExists := CommandExists("echo") - if !echoExists { - t.Error("echo command should exist on most systems") + firstExisting, secondExisting := existingCommandFixtures() + if !CommandExists(firstExisting) { + t.Errorf("%s command should exist on this system", firstExisting) } - lsExists := CommandExists("ls") - if !lsExists { - t.Error("ls command should exist on most Unix-like systems") + if !CommandExists(secondExisting) { + t.Errorf("%s command should exist on this system", secondExisting) } fakeExists := CommandExists("definitely-does-not-exist-12345") @@ -222,3 +222,10 @@ func TestRealSystemIntegration(t *testing.T) { } }) } + +func existingCommandFixtures() (string, string) { + if runtime.GOOS == "windows" { + return "cmd.exe", "where.exe" + } + return "sh", "true" +} diff --git a/internal/audio/system_command_backend_test.go b/internal/audio/system_command_backend_test.go index 1cea861..7b3400b 100644 --- a/internal/audio/system_command_backend_test.go +++ b/internal/audio/system_command_backend_test.go @@ -143,8 +143,8 @@ func TestBuildPlayerArgv_AplayFullVolumeNoWarn(t *testing.T) { } // TestBuildPlayerArgv_UnknownCommandFallsBack confirms the default branch -// passes only filePath. The existing TestSystemCommandBackend_Play uses -// command="echo" -- this guards that test from regressing. +// passes only filePath. TestSystemCommandBackend_Play uses a harmless +// platform command, but echo still exercises the default argv branch here. func TestBuildPlayerArgv_UnknownCommandFallsBack(t *testing.T) { scb := NewSystemCommandBackend("echo") argv := scb.buildPlayerArgv("/tmp/x.wav", 0.5)