diff --git a/acceptance/init_test.go b/acceptance/init_test.go index 07f4fb19..b73a87ac 100644 --- a/acceptance/init_test.go +++ b/acceptance/init_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "slices" "strings" "testing" @@ -570,6 +571,63 @@ func TestInitCreatesHookFiles(t *testing.T) { "expected hooks section in settings.json, got: %s", settingsContent) } +// A repo initialized by the real binary ends up with a Stop hook that actually +// fires chunk validate. Asserting the hooks key merely exists is not enough: a +// repo can carry commit hooks and no Stop hook at all, which is how repos ended +// up looking configured while nothing validated at session end. +func TestInitWritesFiringStopHook(t *testing.T) { + workDir := gitrepo.SetupGitRepo(t, "my-org", "my-repo") + assert.NilError(t, os.WriteFile(filepath.Join(workDir, "go.mod"), []byte("module example.com/m\n"), 0o644)) + + env := testenv.NewTestEnv(t) + env.AnthropicKey = "" + + result := binary.RunCLI(t, []string{"init"}, env, workDir) + assert.Equal(t, result.ExitCode, 0, "stdout: %s\nstderr: %s", result.Stdout, result.Stderr) + + data, err := os.ReadFile(filepath.Join(workDir, ".claude", "settings.json")) + assert.NilError(t, err, "expected .claude/settings.json to exist") + + var parsed map[string]interface{} + assert.NilError(t, json.Unmarshal(data, &parsed)) + hooks, ok := parsed["hooks"].(map[string]interface{}) + assert.Assert(t, ok, "expected a hooks object, got: %s", data) + + // The commit hooks and the Stop hook are written by separate branches of the + // merge, so a repo can have one without the other. Both must be present. + commitCommands := hookCommands(t, hooks["PreToolUse"]) + assert.Assert(t, len(commitCommands) > 0, "expected a PreToolUse commit hook, got: %s", data) + + stopCommands := hookCommands(t, hooks["Stop"]) + assert.Assert(t, slices.Contains(stopCommands, "chunk validate"), + "expected a Stop hook running chunk validate, got: %v", stopCommands) +} + +// hookCommands flattens the hook entry commands across every group of one hook +// type, so a test can assert on what actually runs regardless of grouping. +func hookCommands(t *testing.T, groups interface{}) []string { + t.Helper() + list, _ := groups.([]interface{}) + var commands []string + for _, g := range list { + group, ok := g.(map[string]interface{}) + if !ok { + continue + } + entries, _ := group["hooks"].([]interface{}) + for _, e := range entries { + entry, ok := e.(map[string]interface{}) + if !ok { + continue + } + if cmd, ok := entry["command"].(string); ok { + commands = append(commands, cmd) + } + } + } + return commands +} + func TestInitHookExistingSettingsForceWritesExample(t *testing.T) { workDir := gitrepo.SetupGitRepo(t, "my-org", "my-repo") assert.NilError(t, os.WriteFile(filepath.Join(workDir, "go.mod"), []byte("module example.com/m\n"), 0o644)) @@ -627,10 +685,17 @@ func TestInitHookExistingSettingsWritesExample(t *testing.T) { assert.Assert(t, strings.Contains(string(data), "existing"), "expected original settings.json to be preserved without --force") - // Example should exist - examplePath := filepath.Join(claudeDir, "settings.example.json") - _, err = os.Stat(examplePath) + // Example should exist, and carry the Stop hook — it is what the user is told + // to copy, so an example without it hands them a half-configured repo. + exampleData, err := os.ReadFile(filepath.Join(claudeDir, "settings.example.json")) assert.NilError(t, err, "expected settings.example.json to exist") + + var example map[string]interface{} + assert.NilError(t, json.Unmarshal(exampleData, &example)) + exampleHooks, ok := example["hooks"].(map[string]interface{}) + assert.Assert(t, ok, "expected a hooks object in the example, got: %s", exampleData) + assert.Assert(t, slices.Contains(hookCommands(t, exampleHooks["Stop"]), "chunk validate"), + "expected the example to run chunk validate at session end, got: %s", exampleData) } // --- init never touches CircleCI --- diff --git a/internal/cmd/init_test.go b/internal/cmd/init_test.go index d4145396..82ff4f4c 100644 --- a/internal/cmd/init_test.go +++ b/internal/cmd/init_test.go @@ -18,6 +18,7 @@ import ( "github.com/CircleCI-Public/chunk-cli/internal/config" "github.com/CircleCI-Public/chunk-cli/internal/iostream" + "github.com/CircleCI-Public/chunk-cli/internal/settings" "github.com/CircleCI-Public/chunk-cli/internal/testing/fakes" "github.com/CircleCI-Public/chunk-cli/internal/ui" ) @@ -33,6 +34,30 @@ func testStreams() (iostream.Streams, *bytes.Buffer, *bytes.Buffer) { return iostream.Streams{Out: &out, Err: &errOut}, &out, &errOut } +// mergedHookCommands flattens the hook entry commands across every group of one +// hook type, so a test can assert on what actually runs regardless of grouping. +func mergedHookCommands(groups interface{}) []string { + list, _ := groups.([]interface{}) + var commands []string + for _, g := range list { + group, ok := g.(map[string]interface{}) + if !ok { + continue + } + entries, _ := group["hooks"].([]interface{}) + for _, e := range entries { + entry, ok := e.(map[string]interface{}) + if !ok { + continue + } + if cmd, ok := entry["command"].(string); ok { + commands = append(commands, cmd) + } + } + } + return commands +} + func TestWriteSettingsNewFile(t *testing.T) { dir := t.TempDir() streams, _, errOut := testStreams() @@ -93,8 +118,19 @@ func TestWriteSettingsExistingMergeApplied(t *testing.T) { assert.Assert(t, slices.Contains(allowStrs, "Read")) assert.Assert(t, slices.Contains(allowStrs, "Bash(chunk:*)")) - // Hooks added. - assert.Assert(t, merged["hooks"] != nil) + // Hooks added — both kinds. "hooks != nil" would pass with only the commit + // hooks merged in, which is exactly the state that left repos looking + // configured while nothing validated at session end. + hooks, ok := merged["hooks"].(map[string]interface{}) + assert.Assert(t, ok, "expected a hooks object, got: %s", data) + + commitCommands := mergedHookCommands(hooks["PreToolUse"]) + assert.Assert(t, slices.ContainsFunc(commitCommands, func(c string) bool { + return strings.Contains(c, "go test ./...") + }), "expected the commit hook to run the configured command, got: %v", commitCommands) + + stopCommands := mergedHookCommands(hooks["Stop"]) + assert.DeepEqual(t, stopCommands, []string{settings.StopCommand}) // No example file written. _, statErr := os.Stat(filepath.Join(claudeDir, "settings.example.json")) diff --git a/internal/settings/merge.go b/internal/settings/merge.go index 88c39aae..1d3b5461 100644 --- a/internal/settings/merge.go +++ b/internal/settings/merge.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "slices" "sort" udiff "github.com/aymanbagabas/go-udiff" @@ -25,6 +26,11 @@ const CommitIfFilter = "Bash(git commit*)" // to the current format without leaving a stale duplicate group behind. const legacyCommitMatcher = "Bash(git commit*)" +// StopCommand is the Stop hook command that chunk manages. Merge identifies +// chunk's own Stop entry by this exact string, so it must stay in sync with the +// command written by Build and BuildCodex. +const StopCommand = "chunk validate" + // MergeResult holds the computed merge without performing any I/O. type MergeResult struct { Original []byte // existing settings.json content (re-marshaled for normalized formatting) @@ -70,7 +76,10 @@ func Merge(existing, generated []byte) (*MergeResult, error) { // Union permissions.allow. mergePermissionsAllow(merged, generatedMap) - // Merge hooks.PreToolUse — replace the chunk-managed hook group by matcher. + // Merge hooks.PreToolUse and hooks.Stop — replace the entries chunk owns, keep + // the rest. Without the Stop half a repo that already had a settings.json keeps + // its commit hooks but never gets the Stop hook, so validation stops running + // at session end. mergeHooks(merged, generatedMap) mergedBytes, err := json.MarshalIndent(merged, "", " ") @@ -133,145 +142,206 @@ func mergePermissionsAllow(merged, generated map[string]interface{}) { mergedPerms["allow"] = result } -// mergeHooks replaces the chunk-managed hook group (matched by CommitMatcher) -// within PreToolUse, preserving all other hook types and groups. +// mergeHooks installs chunk's hooks into merged, preserving every hook type, +// group, and entry chunk does not own. +// +// Both hook types chunk writes are owned per entry, not per group. A user may +// have added their own entries to a group that also holds chunk's, and replacing +// the enclosing group would silently delete them. What chunk owns: +// +// - PreToolUse: entries carrying CommitIfFilter, plus every entry of a group +// still on the legacy matcher — older versions wrote that group whole and +// its entries have no "if" to recognise them by. +// - Stop: entries whose command is StopCommand. func mergeHooks(merged, generated map[string]interface{}) { genHooks, ok := generated["hooks"].(map[string]interface{}) if !ok { return } - genPreToolUse, ok := genHooks["PreToolUse"].([]interface{}) - if !ok || len(genPreToolUse) == 0 { + mergeHookType(merged, genHooks, "PreToolUse", ownsCommitEntry, isChunkCommitGroup) + mergeHookType(merged, genHooks, "Stop", ownsStopEntry, nil) +} + +// entryOwner reports whether chunk owns an entry, given the group holding it. +type entryOwner func(group map[string]interface{}, entry interface{}) bool + +// mergeHookType installs chunk's entries for one hook type. Chunk's entries are +// stripped from wherever they sit — collapsing stale duplicates left behind by +// older versions — and the generated ones go back in at the first position they +// held, so a merge over already-merged settings is a no-op. +// +// With nothing of chunk's present, isTargetGroup picks an existing group to write +// into. PreToolUse needs it: chunk's group is identified by tool name, so +// appending a second group on the same matcher would be wrong. Stop groups have +// no matcher, so it passes nil and chunk's own group is appended. +func mergeHookType(merged, genHooks map[string]interface{}, hookType string, owns entryOwner, isTargetGroup func(map[string]interface{}) bool) { + genGroup, genEntries := chunkEntries(genHooks[hookType], owns) + if len(genEntries) == 0 { return } - // Find the chunk-managed group in generated hooks. - var chunkGroup interface{} - for _, g := range genPreToolUse { - group, isMap := g.(map[string]interface{}) - if !isMap { + mergedHooks := hooksMap(merged) + groups, _ := mergedHooks[hookType].([]interface{}) + + // Strip chunk's entries out of every group, noting where the first one sat and + // which groups held nothing else. + targetIdx, insertAt := -1, 0 + emptied := make(map[int]bool) + for i, g := range groups { + group, entries, isGroup := groupEntries(g) + if !isGroup { + continue + } + kept := make([]interface{}, 0, len(entries)) + for _, e := range entries { + if owns(group, e) { + if targetIdx < 0 { + targetIdx, insertAt = i, len(kept) + } + continue + } + kept = append(kept, e) + } + if len(kept) == len(entries) { continue } - if matcher, _ := group["matcher"].(string); matcher == CommitMatcher { - chunkGroup = g - break + group["hooks"] = kept + if len(kept) == 0 { + emptied[i] = true + } + } + + if targetIdx < 0 && isTargetGroup != nil { + for i, g := range groups { + group, entries, isGroup := groupEntries(g) + if isGroup && isTargetGroup(group) { + targetIdx, insertAt = i, len(entries) + break + } } } - if chunkGroup == nil { + if targetIdx < 0 { + mergedHooks[hookType] = append(groups, chunkGroup(genGroup, genEntries)) return } - // Ensure merged has hooks.PreToolUse. - mergedHooks, ok := merged["hooks"].(map[string]interface{}) - if !ok { - mergedHooks = map[string]interface{}{} - merged["hooks"] = mergedHooks + target, entries, _ := groupEntries(groups[targetIdx]) + target["hooks"] = slices.Insert(entries, insertAt, genEntries...) + // Carry over the generated group's own keys — its matcher above all — so a + // group still on the legacy matcher migrates in place. + for k, v := range genGroup { + if k != "hooks" { + target[k] = v + } } + delete(emptied, targetIdx) - mergedPreToolUse, ok := mergedHooks["PreToolUse"].([]interface{}) - if !ok { - mergedPreToolUse = []interface{}{} + kept := make([]interface{}, 0, len(groups)) + for i, g := range groups { + if !emptied[i] { + kept = append(kept, g) + } } + mergedHooks[hookType] = kept +} - // Replace existing group with same matcher (or legacy matcher), or append. - replaced := false - for i, g := range mergedPreToolUse { - group, isMap := g.(map[string]interface{}) - if !isMap { +// chunkEntries returns the generated group holding chunk's entries for one hook +// type, along with those entries. +func chunkEntries(genGroups interface{}, owns entryOwner) (map[string]interface{}, []interface{}) { + list, _ := genGroups.([]interface{}) + for _, g := range list { + group, entries, isGroup := groupEntries(g) + if !isGroup { continue } - matcher, _ := group["matcher"].(string) - if matcher == CommitMatcher || matcher == legacyCommitMatcher { - mergedPreToolUse[i] = chunkGroup - replaced = true - break + owned := make([]interface{}, 0, len(entries)) + for _, e := range entries { + if owns(group, e) { + owned = append(owned, e) + } + } + if len(owned) > 0 { + return group, owned } } - if !replaced { - mergedPreToolUse = append(mergedPreToolUse, chunkGroup) - } - - mergedHooks["PreToolUse"] = mergedPreToolUse + return nil, nil } -// mergeStopHooks replaces the chunk-managed Stop hook group (identified by the -// "chunk validate" command) within Stop, preserving all other Stop groups. -func mergeStopHooks(merged, generated map[string]interface{}) { - genHooks, ok := generated["hooks"].(map[string]interface{}) - if !ok { - return - } - genStop, ok := genHooks["Stop"].([]interface{}) - if !ok || len(genStop) == 0 { - return - } - - // Find the chunk-managed group in generated Stop hooks. - var chunkGroup interface{} - for _, g := range genStop { - if isChunkStopGroup(g) { - chunkGroup = g - break +// chunkGroup builds a fresh hook group from the generated group's own fields and +// the given entries, so the generated map is never aliased into merged settings. +func chunkGroup(gen map[string]interface{}, entries []interface{}) map[string]interface{} { + group := make(map[string]interface{}, len(gen)) + for k, v := range gen { + if k != "hooks" { + group[k] = v } } - if chunkGroup == nil { - return - } + group["hooks"] = entries + return group +} - // Ensure merged has hooks.Stop. - mergedHooks, ok := merged["hooks"].(map[string]interface{}) +// hooksMap returns the "hooks" object in settings, creating it when absent. +// Created lazily: adding an empty hooks object to settings that have none would +// count as a change and prompt the user over nothing. +func hooksMap(settings map[string]interface{}) map[string]interface{} { + hooks, ok := settings["hooks"].(map[string]interface{}) if !ok { - mergedHooks = map[string]interface{}{} - merged["hooks"] = mergedHooks + hooks = map[string]interface{}{} + settings["hooks"] = hooks } + return hooks +} - mergedStop, ok := mergedHooks["Stop"].([]interface{}) - if !ok { - mergedStop = []interface{}{} - } +// isChunkCommitGroup reports whether a PreToolUse group is the one chunk writes +// its commit hooks into, accepting the legacy matcher so older settings migrate +// in place rather than gaining a second group on the same tool. +func isChunkCommitGroup(group map[string]interface{}) bool { + matcher, _ := group["matcher"].(string) + return matcher == CommitMatcher || matcher == legacyCommitMatcher +} - // Replace existing chunk-managed group, or append. - replaced := false - for i, g := range mergedStop { - if isChunkStopGroup(g) { - mergedStop[i] = chunkGroup - replaced = true - break - } +// groupEntries returns a hook group's map and its list of hook entries. +func groupEntries(g interface{}) (map[string]interface{}, []interface{}, bool) { + group, ok := g.(map[string]interface{}) + if !ok { + return nil, nil, false } - if !replaced { - mergedStop = append(mergedStop, chunkGroup) + entries, ok := group["hooks"].([]interface{}) + if !ok { + return nil, nil, false } - - mergedHooks["Stop"] = mergedStop + return group, entries, true } -// isChunkStopGroup reports whether a Stop hook group is chunk-managed, -// identified by containing a hook with command "chunk validate". -func isChunkStopGroup(g interface{}) bool { - group, ok := g.(map[string]interface{}) +// ownsCommitEntry reports whether a PreToolUse entry is one of chunk's commit +// hooks. Entries are tagged with CommitIfFilter; those in a group still on the +// legacy matcher are not, but that whole group was written by chunk. +func ownsCommitEntry(group map[string]interface{}, e interface{}) bool { + if matcher, _ := group["matcher"].(string); matcher == legacyCommitMatcher { + return true + } + entry, ok := e.(map[string]interface{}) if !ok { return false } - hooks, ok := group["hooks"].([]interface{}) + cond, _ := entry["if"].(string) + return cond == CommitIfFilter +} + +// ownsStopEntry reports whether a Stop entry is the one chunk manages, +// identified by its command. +func ownsStopEntry(_ map[string]interface{}, e interface{}) bool { + entry, ok := e.(map[string]interface{}) if !ok { return false } - for _, h := range hooks { - entry, ok := h.(map[string]interface{}) - if !ok { - continue - } - if cmd, _ := entry["command"].(string); cmd == "chunk validate" { - return true - } - } - return false + cmd, _ := entry["command"].(string) + return cmd == StopCommand } // MergeCodex computes the merged .codex/hooks.json from existing and generated bytes. -// It preserves all unknown keys and hook types, replaces the chunk-managed PreToolUse -// group by matcher, and replaces the chunk-managed Stop hook group by command. +// It preserves all unknown keys and hook types, and replaces chunk's own PreToolUse +// and Stop hook entries via the same mergeHooks used for .claude/settings.json. func MergeCodex(existing, generated []byte) (*MergeResult, error) { var existingMap map[string]interface{} if err := json.Unmarshal(existing, &existingMap); err != nil { @@ -289,7 +359,6 @@ func MergeCodex(existing, generated []byte) (*MergeResult, error) { } mergeHooks(existingMap, generatedMap) - mergeStopHooks(existingMap, generatedMap) mergedBytes, err := json.MarshalIndent(existingMap, "", " ") if err != nil { diff --git a/internal/settings/merge_test.go b/internal/settings/merge_test.go index aa6f5602..73e1631c 100644 --- a/internal/settings/merge_test.go +++ b/internal/settings/merge_test.go @@ -171,6 +171,105 @@ func TestMergeHooksMigratesLegacyMatcher(t *testing.T) { assert.Equal(t, entry["if"], "Bash(git commit*)") } +// A user's own Bash hook survives even though it sits in the group chunk writes +// into. The group matcher is the bare tool name, so a team with any non-commit +// Bash hook shares the group with chunk — replacing the group would delete it. +func TestMergePreservesUserEntriesInChunkCommitGroup(t *testing.T) { + existing := []byte(`{ + "hooks": { + "PreToolUse": [ + {"matcher": "Bash", "hooks": [{"type": "command", "command": "audit-log", "timeout": 5}]} + ] + } + }`) + generated := []byte(`{ + "hooks": { + "PreToolUse": [ + {"matcher": "Bash", "hooks": [{"type": "command", "if": "Bash(git commit*)", "command": "task test", "timeout": 60}]} + ] + } + }`) + + result, err := Merge(existing, generated) + assert.NilError(t, err) + assert.Assert(t, result.Changed) + + var merged map[string]interface{} + assert.NilError(t, json.Unmarshal(result.Merged, &merged)) + + hooks := merged["hooks"].(map[string]interface{}) + preToolUse := hooks["PreToolUse"].([]interface{}) + assert.Equal(t, len(preToolUse), 1) + + group := preToolUse[0].(map[string]interface{}) + assert.Equal(t, group["matcher"], CommitMatcher) + entries := group["hooks"].([]interface{}) + assert.Equal(t, len(entries), 2) + + // The user's entry keeps its position and content; chunk's is appended after it. + first := entries[0].(map[string]interface{}) + assert.Equal(t, first["command"], "audit-log") + assert.Equal(t, first["timeout"], float64(5)) + + second := entries[1].(map[string]interface{}) + assert.Equal(t, second["command"], "task test") + assert.Equal(t, second["if"], CommitIfFilter) + + // Re-running init over the merged result must not stack a second copy. + again, err := Merge(result.Merged, generated) + assert.NilError(t, err) + assert.Assert(t, !again.Changed, "expected a second merge to be a no-op, got:\n%s", Diff(again.Original, again.Merged)) +} + +// Duplicate chunk entries left behind by an earlier version collapse to one +// instead of surviving every merge and running validation twice per session. +func TestMergeCollapsesDuplicateChunkEntries(t *testing.T) { + existing := []byte(`{ + "hooks": { + "PreToolUse": [ + {"matcher": "Bash(git commit*)", "hooks": [{"type": "command", "command": "old-cmd", "timeout": 30}]}, + {"matcher": "Bash", "hooks": [{"type": "command", "if": "Bash(git commit*)", "command": "task test", "timeout": 30}]} + ], + "Stop": [ + {"hooks": [ + {"type": "command", "command": "chunk validate", "timeout": 30}, + {"type": "command", "command": "chunk validate", "timeout": 330} + ]} + ] + } + }`) + generated := []byte(`{ + "hooks": { + "PreToolUse": [ + {"matcher": "Bash", "hooks": [{"type": "command", "if": "Bash(git commit*)", "command": "task test", "timeout": 60}]} + ], + "Stop": [{"hooks": [{"type": "command", "command": "chunk validate", "timeout": 600}]}] + } + }`) + + result, err := Merge(existing, generated) + assert.NilError(t, err) + assert.Assert(t, result.Changed) + + var merged map[string]interface{} + assert.NilError(t, json.Unmarshal(result.Merged, &merged)) + hooks := merged["hooks"].(map[string]interface{}) + + // The legacy group and the current one collapse into a single group. + preToolUse := hooks["PreToolUse"].([]interface{}) + assert.Equal(t, len(preToolUse), 1) + commitEntries := preToolUse[0].(map[string]interface{})["hooks"].([]interface{}) + assert.Equal(t, len(commitEntries), 1) + assert.Equal(t, commitEntries[0].(map[string]interface{})["timeout"], float64(60)) + + // Both stale "chunk validate" entries give way to the one generated entry. + stop := hooks["Stop"].([]interface{}) + assert.Equal(t, len(stop), 1) + stopEntries := stop[0].(map[string]interface{})["hooks"].([]interface{}) + assert.Equal(t, len(stopEntries), 1) + assert.Equal(t, stopEntries[0].(map[string]interface{})["timeout"], float64(600)) +} + func TestMergeHooksPreservesDifferentMatcher(t *testing.T) { existing := []byte(`{ "hooks": { @@ -239,6 +338,171 @@ func TestMergePreservesOtherHookTypes(t *testing.T) { assert.Equal(t, len(preToolUse), 1) } +// A repo that already had a settings.json before chunk init ran gets the Stop +// hook added, not just PreToolUse. Without it the repo keeps its commit hooks +// but never validates at session end. +func TestMergeAddsStopHookToExistingSettings(t *testing.T) { + existing := []byte(`{ + "permissions": {"allow": ["Bash(task *)"]}, + "hooks": { + "PreToolUse": [ + {"matcher": "Bash", "hooks": [{"type": "command", "if": "Bash(git commit*)", "command": "task test", "timeout": 300}]} + ] + } + }`) + generated := []byte(`{ + "hooks": { + "PreToolUse": [ + {"matcher": "Bash", "hooks": [{"type": "command", "if": "Bash(git commit*)", "command": "cd . && task test", "timeout": 300}]} + ], + "Stop": [{"hooks": [{"type": "command", "command": "chunk validate", "timeout": 330}]}] + } + }`) + + result, err := Merge(existing, generated) + assert.NilError(t, err) + assert.Assert(t, result.Changed) + + var merged map[string]interface{} + assert.NilError(t, json.Unmarshal(result.Merged, &merged)) + + hooks := merged["hooks"].(map[string]interface{}) + stop, ok := hooks["Stop"].([]interface{}) + assert.Assert(t, ok && len(stop) == 1, "expected a Stop hook group, got: %v", hooks["Stop"]) + + group := stop[0].(map[string]interface{}) + entry := group["hooks"].([]interface{})[0].(map[string]interface{}) + assert.Equal(t, entry["command"], "chunk validate") + assert.Equal(t, entry["timeout"], float64(330)) +} + +// The chunk-managed Stop group is replaced in place so a changed timeout takes +// effect instead of stacking a second "chunk validate" entry. +func TestMergeReplacesStopHook(t *testing.T) { + existing := []byte(`{ + "hooks": { + "Stop": [{"hooks": [{"type": "command", "command": "chunk validate", "timeout": 30}]}] + } + }`) + generated := []byte(`{ + "hooks": { + "Stop": [{"hooks": [{"type": "command", "command": "chunk validate", "timeout": 600}]}] + } + }`) + + result, err := Merge(existing, generated) + assert.NilError(t, err) + // init.go gates the settings.json write on Changed, so a timeout-only + // difference has to register as a change or the new value is never written. + assert.Assert(t, result.Changed) + + var merged map[string]interface{} + assert.NilError(t, json.Unmarshal(result.Merged, &merged)) + + hooks := merged["hooks"].(map[string]interface{}) + stop := hooks["Stop"].([]interface{}) + assert.Equal(t, len(stop), 1) + + group := stop[0].(map[string]interface{}) + entries := group["hooks"].([]interface{}) + assert.Equal(t, len(entries), 1) + entry := entries[0].(map[string]interface{}) + assert.Equal(t, entry["command"], StopCommand) + assert.Equal(t, entry["timeout"], float64(600)) +} + +// A Stop hook the user wrote themselves is left alone; chunk only manages its own. +func TestMergePreservesUserStopHooks(t *testing.T) { + existing := []byte(`{ + "hooks": { + "Stop": [ + {"hooks": [{"type": "command", "command": "notify-send done", "timeout": 5}]} + ] + } + }`) + generated := []byte(`{ + "hooks": { + "Stop": [{"hooks": [{"type": "command", "command": "chunk validate", "timeout": 600}]}] + } + }`) + + result, err := Merge(existing, generated) + assert.NilError(t, err) + assert.Assert(t, result.Changed) + + var merged map[string]interface{} + assert.NilError(t, json.Unmarshal(result.Merged, &merged)) + + hooks := merged["hooks"].(map[string]interface{}) + stop, ok := hooks["Stop"].([]interface{}) + assert.Assert(t, ok && len(stop) == 2, "expected both Stop groups to be present, got: %v", len(stop)) + + // The user's group keeps its position and content untouched. + userEntries := stop[0].(map[string]interface{})["hooks"].([]interface{}) + assert.Equal(t, len(userEntries), 1) + userEntry := userEntries[0].(map[string]interface{}) + assert.Equal(t, userEntry["command"], "notify-send done") + assert.Equal(t, userEntry["timeout"], float64(5)) + + // Chunk's group is appended after it, carrying the generated command and timeout. + chunkEntries := stop[1].(map[string]interface{})["hooks"].([]interface{}) + assert.Equal(t, len(chunkEntries), 1) + chunkEntry := chunkEntries[0].(map[string]interface{}) + assert.Equal(t, chunkEntry["command"], StopCommand) + assert.Equal(t, chunkEntry["timeout"], float64(600)) +} + +// A user entry sharing the group with chunk's own survives the merge. Chunk owns +// the "chunk validate" entry, not the group it happens to sit in, so the entry is +// replaced in place rather than the whole group being overwritten. +func TestMergePreservesUserEntriesInChunkStopGroup(t *testing.T) { + existing := []byte(`{ + "hooks": { + "Stop": [ + {"hooks": [ + {"type": "command", "command": "chunk validate", "timeout": 30}, + {"type": "command", "command": "notify-send done", "timeout": 5} + ]} + ] + } + }`) + generated := []byte(`{ + "hooks": { + "Stop": [{"hooks": [{"type": "command", "command": "chunk validate", "timeout": 600}]}] + } + }`) + + result, err := Merge(existing, generated) + assert.NilError(t, err) + // init.go gates the write on Changed, so the shared-group path has to report + // the timeout update or the new value never reaches disk. + assert.Assert(t, result.Changed) + + var merged map[string]interface{} + assert.NilError(t, json.Unmarshal(result.Merged, &merged)) + + hooks := merged["hooks"].(map[string]interface{}) + stop := hooks["Stop"].([]interface{}) + assert.Equal(t, len(stop), 1) + + entries := stop[0].(map[string]interface{})["hooks"].([]interface{}) + assert.Equal(t, len(entries), 2) + + commands := make([]string, 0, len(entries)) + for _, e := range entries { + entry := e.(map[string]interface{}) + cmd, _ := entry["command"].(string) + commands = append(commands, cmd) + if cmd == StopCommand { + assert.Equal(t, entry["timeout"], float64(600)) + } + } + assert.DeepEqual(t, commands, []string{StopCommand, "notify-send done"}) +} + +// Re-running init over already-merged settings is a no-op. The fixture carries a +// Stop hook as well as PreToolUse so both halves of mergeHooks are covered — +// replacing chunk's entry with an identical one must not register as a change. func TestMergeNoChangeWhenAlreadyMerged(t *testing.T) { settings := []byte(`{ "$schema": "https://json.schemastore.org/claude-code-settings.json", @@ -247,6 +511,30 @@ func TestMergeNoChangeWhenAlreadyMerged(t *testing.T) { "hooks": { "PreToolUse": [ {"matcher": "Bash", "hooks": [{"type": "command", "if": "Bash(git commit*)", "command": "test", "timeout": 60}]} + ], + "Stop": [ + {"hooks": [{"type": "command", "command": "chunk validate", "timeout": 330}]} + ] + } + }`) + + result, err := Merge(settings, settings) + assert.NilError(t, err) + assert.Assert(t, !result.Changed) +} + +// Idempotency holds when chunk's Stop entry shares a group with a user's entry — +// the in-place entry swap must not reorder or duplicate anything. +func TestMergeNoChangeWhenChunkStopEntryHasUserSibling(t *testing.T) { + settings := []byte(`{ + "$schema": "https://json.schemastore.org/claude-code-settings.json", + "permissions": {"allow": ["Bash(chunk:*)"]}, + "hooks": { + "Stop": [ + {"hooks": [ + {"type": "command", "command": "chunk validate", "timeout": 330}, + {"type": "command", "command": "notify-send done", "timeout": 5} + ]} ] } }`) @@ -465,6 +753,49 @@ func TestMergeCodexPreservesUserStopHooks(t *testing.T) { } } +// The Codex path shares mergeHooks, so it keeps user entries that sit in the +// same group as chunk's own entry too. +func TestMergeCodexPreservesUserEntriesInChunkStopGroup(t *testing.T) { + existing := []byte(`{ + "hooks": { + "Stop": [ + {"hooks": [ + {"type": "command", "command": "notify-team", "timeout": 10}, + {"type": "command", "command": "chunk validate", "timeout": 30} + ]} + ] + } + }`) + generated := []byte(`{ + "hooks": { + "Stop": [{"hooks": [{"type": "command", "command": "chunk validate", "timeout": 600}]}] + } + }`) + + result, err := MergeCodex(existing, generated) + assert.NilError(t, err) + assert.Assert(t, result.Changed) + + var merged map[string]interface{} + assert.NilError(t, json.Unmarshal(result.Merged, &merged)) + + hooks := merged["hooks"].(map[string]interface{}) + stop := hooks["Stop"].([]interface{}) + assert.Equal(t, len(stop), 1) + + entries := stop[0].(map[string]interface{})["hooks"].([]interface{}) + assert.Equal(t, len(entries), 2) + + // The user's entry keeps its position and timeout; chunk's is updated in place. + first := entries[0].(map[string]interface{}) + assert.Equal(t, first["command"], "notify-team") + assert.Equal(t, first["timeout"], float64(10)) + + second := entries[1].(map[string]interface{}) + assert.Equal(t, second["command"], StopCommand) + assert.Equal(t, second["timeout"], float64(600)) +} + func TestMergeCodexNoChangeWhenAlreadyMerged(t *testing.T) { data := []byte(`{ "hooks": { diff --git a/internal/settings/settings.go b/internal/settings/settings.go index f4486eaf..17a4038a 100644 --- a/internal/settings/settings.go +++ b/internal/settings/settings.go @@ -91,7 +91,7 @@ func Build(commands []config.Command) ([]byte, error) { Hooks: []hookEntry{ { Type: "command", - Command: "chunk validate", + Command: StopCommand, Timeout: stopTimeout, }, }, @@ -144,7 +144,7 @@ func BuildCodex(commands []config.Command) ([]byte, error) { Hooks: []hookEntry{ { Type: "command", - Command: "chunk validate", + Command: StopCommand, Timeout: stopTimeout, }, },