From 271f57ef7a00a65577f08be6aa61a813417b7d56 Mon Sep 17 00:00:00 2001 From: daniel <113305699+devbydaniel@users.noreply.github.com> Date: Thu, 6 Aug 2026 21:30:58 +0200 Subject: [PATCH] fix: reclaim marker-stripped managed hook entries during merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MergeIntoSettings deduplicates by the _agentfiles marker field, but other writers of settings.json — notably Claude Code, which re-serializes hook entries whenever the user changes a setting or accepts a permission — drop unknown fields. Each subsequent apply then treated the stripped entries as user-owned and appended fresh marked copies, duplicating every managed hook once per strip/apply cycle (observed: 12 copies of 3 SessionStart hooks). The merge now also reclaims unmarked entries whose commands all point into an agentfiles hook deploy dir — those are ours by ownership of the scripts, marker or not. The deploy-base constants move to the hooks package so apply and merge share one definition. File-form hooks with commands outside the deploy dirs still rely on the marker alone; their stripped survivors are not reclaimed. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019NuDDqSg1XzYebxp5JKqmo --- internal/apply/apply.go | 12 ++---- internal/hooks/hooks_test.go | 73 ++++++++++++++++++++++++++++++++++++ internal/hooks/merge.go | 56 ++++++++++++++++++++++++++- 3 files changed, 131 insertions(+), 10 deletions(-) diff --git a/internal/apply/apply.go b/internal/apply/apply.go index e43af27..31da110 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -20,15 +20,11 @@ import ( // hookDeployBase is the repo- or home-relative directory under which agentfiles // deploys directory-form hook contents. For user-level applies this becomes // $HOME//; for repo-level it becomes -// //. -// -// hookDeployBaseRepo must NOT live under ".agentfiles/" — that path is the repo -// manifest *file* (".agentfiles"), so ".agentfiles/hooks" collides with it and -// every repo-level apply fails with "open .agentfiles/hooks: not a directory". -// It sits beside the manifest instead, mirroring ".agentfiles.lock". +// //. The values live in the hooks package, +// which also uses them to reclaim marker-stripped managed entries during merge. const ( - hookDeployBaseUser = ".local/share/agentfiles/hooks" - hookDeployBaseRepo = ".agentfiles-hooks" + hookDeployBaseUser = hooks.DeployBaseUser + hookDeployBaseRepo = hooks.DeployBaseRepo ) // isUserLayout reports whether a layout name targets user-level paths diff --git a/internal/hooks/hooks_test.go b/internal/hooks/hooks_test.go index 1b85718..52c4a20 100644 --- a/internal/hooks/hooks_test.go +++ b/internal/hooks/hooks_test.go @@ -417,6 +417,79 @@ func TestMergeIntoSettings_CreatesFile(t *testing.T) { } } +func TestMergeIntoSettings_ReclaimsStrippedMarkers(t *testing.T) { + dir := t.TempDir() + settingsPath := filepath.Join(dir, "settings.json") + + // Simulate a settings file rewritten by another tool (e.g. Claude Code), + // which drops the _agentfiles marker from managed entries: two stripped + // copies of a managed hook sit next to a genuine user hook. + managedCmd := `$HOME/` + DeployBaseUser + `/sync-skills/scripts/sync.sh` + stripped := `{"matcher": "startup", "hooks": [{"type": "command", "command": "` + managedCmd + `"}]}` + userEntry := `{"hooks": [{"type": "command", "command": "$HOME/bin/mine.sh"}]}` + initial := `{"hooks": {"SessionStart": [` + stripped + `, ` + stripped + `, ` + userEntry + `]}}` + if err := os.WriteFile(settingsPath, []byte(initial), 0o644); err != nil { + t.Fatal(err) + } + + managed := map[string]*HookFile{ + "sync-skills": { + Event: "SessionStart", + Matcher: "startup", + Hooks: []json.RawMessage{json.RawMessage(`{"type": "command", "command": "` + managedCmd + `"}`)}, + }, + } + if err := MergeIntoSettings(settingsPath, managed, FormatNested); err != nil { + t.Fatalf("MergeIntoSettings: %v", err) + } + + data, _ := os.ReadFile(settingsPath) + var topLevel map[string]json.RawMessage + json.Unmarshal(data, &topLevel) + var hooks map[string][]json.RawMessage + json.Unmarshal(topLevel["hooks"], &hooks) + + // The stripped duplicates must be reclaimed: one marked managed entry + // plus the untouched user hook. + if len(hooks["SessionStart"]) != 2 { + t.Fatalf("SessionStart hooks = %d, want 2", len(hooks["SessionStart"])) + } + var markers, userKept int + for _, e := range hooks["SessionStart"] { + if extractAgentfilesMarker(e) == "sync-skills" { + markers++ + } else if !hasAgentfilesMarker(e) { + userKept++ + } + } + if markers != 1 { + t.Errorf("marked sync-skills entries = %d, want 1", markers) + } + if userKept != 1 { + t.Errorf("user entries kept = %d, want 1", userKept) + } +} + +func TestIsStrippedManagedEntry(t *testing.T) { + cases := []struct { + name string + entry string + want bool + }{ + {"nested managed", `{"hooks": [{"command": "$HOME/` + DeployBaseUser + `/x/run.sh"}]}`, true}, + {"nested repo-level managed", `{"hooks": [{"command": "` + DeployBaseRepo + `/x/run.sh"}]}`, true}, + {"flat managed", `{"command": "$HOME/` + DeployBaseUser + `/x/run.sh"}`, true}, + {"user hook", `{"hooks": [{"command": "$HOME/bin/mine.sh"}]}`, false}, + {"mixed commands", `{"hooks": [{"command": "$HOME/` + DeployBaseUser + `/x/run.sh"}, {"command": "$HOME/bin/mine.sh"}]}`, false}, + {"no commands", `{"matcher": "startup"}`, false}, + } + for _, tc := range cases { + if got := isStrippedManagedEntry(json.RawMessage(tc.entry)); got != tc.want { + t.Errorf("%s: got %v, want %v", tc.name, got, tc.want) + } + } +} + func TestRemoveManaged(t *testing.T) { dir := t.TempDir() settingsPath := filepath.Join(dir, "settings.json") diff --git a/internal/hooks/merge.go b/internal/hooks/merge.go index 75d417a..75508da 100644 --- a/internal/hooks/merge.go +++ b/internal/hooks/merge.go @@ -6,6 +6,23 @@ import ( "os" "path/filepath" "sort" + "strings" +) + +// DeployBaseUser and DeployBaseRepo are the home- and repo-relative directories +// under which agentfiles deploys directory-form hook contents (used by +// internal/apply). They double as an ownership signal during merging: an entry +// whose commands all point into one of these directories is agentfiles-managed +// even when its _agentfiles marker is missing — other writers of the settings +// file (notably Claude Code) re-serialize hook entries and drop unknown fields. +// +// DeployBaseRepo must NOT live under ".agentfiles/" — that path is the repo +// manifest *file* (".agentfiles"), so ".agentfiles/hooks" collides with it and +// every repo-level apply fails with "open .agentfiles/hooks: not a directory". +// It sits beside the manifest instead, mirroring ".agentfiles.lock". +const ( + DeployBaseUser = ".local/share/agentfiles/hooks" + DeployBaseRepo = ".agentfiles-hooks" ) // MergeIntoSettings merges managed hooks into a settings/hooks JSON file. @@ -40,11 +57,12 @@ func MergeIntoSettings(settingsPath string, managed map[string]*HookFile, format } } - // Remove all previously managed entries (those with _agentfiles field). + // Remove all previously managed entries: those with an _agentfiles field, + // plus marker-stripped survivors whose commands identify them as ours. for event, entries := range existingHooks { var kept []json.RawMessage for _, entry := range entries { - if !hasAgentfilesMarker(entry) { + if !hasAgentfilesMarker(entry) && !isStrippedManagedEntry(entry) { kept = append(kept, entry) } } @@ -210,6 +228,40 @@ func buildNestedEntry(name string, hf *HookFile) (json.RawMessage, error) { return data, nil } +// isStrippedManagedEntry reports whether an unmarked entry is agentfiles-managed +// anyway: it has at least one command, and every command points into an +// agentfiles hook deploy dir. Covers both the nested shape +// ({matcher, hooks: [{command}]}) and the flat Cursor shape ({command, ...}). +// File-form hooks with commands outside the deploy dirs are not reclaimable +// this way; they still rely on the _agentfiles marker. +func isStrippedManagedEntry(entry json.RawMessage) bool { + var obj struct { + Command string `json:"command"` + Hooks []struct { + Command string `json:"command"` + } `json:"hooks"` + } + if err := json.Unmarshal(entry, &obj); err != nil { + return false + } + cmds := make([]string, 0, len(obj.Hooks)+1) + if obj.Command != "" { + cmds = append(cmds, obj.Command) + } + for _, h := range obj.Hooks { + cmds = append(cmds, h.Command) + } + if len(cmds) == 0 { + return false + } + for _, cmd := range cmds { + if !strings.Contains(cmd, DeployBaseUser+"/") && !strings.Contains(cmd, DeployBaseRepo+"/") { + return false + } + } + return true +} + // hasAgentfilesMarker returns true if the JSON entry contains an "_agentfiles" field. func hasAgentfilesMarker(entry json.RawMessage) bool { return extractAgentfilesMarker(entry) != ""