fix(install): register every hook as a matcher group, not just SessionStart (#167)

Claude Code expects each hook event to be a list of matcher groups holding a nested
`hooks` array. `Stop` was written as a flat list of entries, on the premise that the
two events genuinely differed in shape — and the comment stating that premise
predicted its own failure mode exactly: the config parses and the hook never fires.
It never fired.

Every event is now written grouped; mergeFlat is gone. mergeMatcher treats an empty
matcher as "the group with no matcher key", found again by that same absence, so
repeated installs stay idempotent and uninstall still removes exactly enola's entry
(comparing group["matcher"] to "" matches no group at all — the value is nil).

An existing installation has to migrate. A flat entry is a map with no `hooks` key;
fed to the group merger unguarded it would have a hooks array grafted onto a command
entry, producing a hybrid that parses and does nothing — the defect surviving its own
fix. enola's own legacy entry is dropped, anyone else's is preserved verbatim, and
uninstall removes both shapes.

The blast radius recorded in DEFECTS_FOUND.md was wrong, and the truth is worse: a
malformed entry invalidates the whole hooks block, so the correctly-shaped
SessionStart beside it did not fire either. `install --hooks` configured nothing at
all — no baseline pinned, no grading — while reporting success. Verified across three
configurations, one variable each; the correction is recorded with the defect.

Covered by TestStopHook_FiresInARealSession (opt-in, ENOLA_E2E=1): installs the hooks
as a user does, pins a baseline, closes a real dependency cycle, ends a headless
session, and asserts the hook both ran and produced the verdict. It fails on the code
it replaces. The unit tests around it assert mechanics only — grouping as an invariant
across every event, idempotency, migration, clean uninstall, foreign entries untouched
— because a unit test can only check the shape against the belief that produced it,
which is how this survived the first time.

Author: dejo1307

pkg/install/hooks.go

@@ -23,9 +23,11 @@ const enolaHookMarker = "enola"
 type hookSpec struct {
 	Event      string
 	Subcommand string
-	// Matcher, when set, means this event takes matcher-GROUPED entries rather than a
-	// flat list. The two shapes are not interchangeable: writing one where the other is
-	// expected produces a config that parses cleanly and never fires.
+	// Matcher narrows an event to particular occasions of it — SessionStart fires on
+	// startup, resume and clear, and enola wants the first two. An empty Matcher means
+	// the event applies unconditionally, NOT that the entry takes a different shape:
+	// every event is written as a list of matcher groups, and a group with no matcher
+	// simply omits the key. See writeHooks.
 	Matcher     string
 	Description string
 }
@@ -73,26 +75,29 @@ func writeHooks(o Options, remove bool) ([]Result, error) {
 			hooks = map[string]any{}
 		}
 
-		// Stop takes a flat list of entries; SessionStart takes matcher-grouped ones.
-		// The events genuinely differ in shape, and getting it wrong is the silent kind
-		// of failure: the config parses and the hook simply never fires.
+		// EVERY event is a list of matcher groups, each holding a nested `hooks` array.
+		// Events differ in whether they carry a matcher, not in their shape.
+		//
+		// This code used to write Stop as a flat list of entries, on the stated premise
+		// that the two events genuinely differed — and the comment above it predicted
+		// its own failure mode exactly: the config parses and the hook simply never
+		// fires. It never fired. `SessionStart` pinned a baseline before editing while
+		// `Stop` silently graded nothing, which is the worst of the three possible
+		// states: everything looks configured and the half that produces the value is
+		// absent. Verified against a real session, both shapes, one variable.
 		for _, h := range installedHooks {
 			entry := map[string]any{
 				"type":    "command",
 				"command": o.hookCommand() + " " + h.Subcommand,
 				"timeout": hookTimeoutSeconds,
 				"source":  enolaHookMarker,
 			}
-			if h.Matcher != "" {
-				hooks[h.Event] = mergeMatcher(hooks[h.Event], h.Matcher, entry, remove)
-			} else {
-				hooks[h.Event] = mergeFlat(hooks[h.Event], entry, remove)
-			}
+			hooks[h.Event] = mergeMatcher(hooks[h.Event], h.Matcher, entry, remove)
 		}
 
-		for _, k := range []string{"Stop", "SessionStart"} {
-			if lst, ok := hooks[k].([]any); ok && len(lst) == 0 {
-				delete(hooks, k)
+		for _, h := range installedHooks {
+			if lst, ok := hooks[h.Event].([]any); ok && len(lst) == 0 {
+				delete(hooks, h.Event)
 			}
 		}
 		if len(hooks) == 0 {
@@ -107,24 +112,14 @@ func writeHooks(o Options, remove bool) ([]Result, error) {
 	return []Result{r}, nil
 }
 
-// mergeFlat adds or removes one entry in a flat hook list, preserving every other entry.
-func mergeFlat(existing any, entry map[string]any, remove bool) any {
-	list, _ := existing.([]any)
-	out := make([]any, 0, len(list)+1)
-	for _, e := range list {
-		if !isEnolaEntry(e) {
-			out = append(out, e)
-		}
-	}
-	if !remove {
-		out = append(out, entry)
-	}
-	return out
-}
-
-// mergeMatcher adds or removes one entry inside a matcher-grouped hook list. An existing
-// group with the same matcher is reused so the user does not accumulate duplicate groups;
-// groups belonging to anything else are left exactly as they were.
+// mergeMatcher adds or removes one entry inside an event's list of matcher groups. An
+// existing group with the same matcher is reused so the user does not accumulate
+// duplicate groups; groups belonging to anything else are left exactly as they were.
+//
+// An empty matcher means the group that carries no matcher key — the event applying
+// unconditionally. Such a group is written without the key and recognised again by the
+// same absence, so a second install updates it in place rather than appending a
+// duplicate, and uninstall still finds it.
 func mergeMatcher(existing any, matcher string, entry map[string]any, remove bool) any {
 	groups, _ := existing.([]any)
 	out := make([]any, 0, len(groups)+1)
@@ -136,14 +131,25 @@ func mergeMatcher(existing any, matcher string, entry map[string]any, remove boo
 			out = append(out, g)
 			continue
 		}
+		// A map with no `hooks` key is a bare command entry from a flat list — the
+		// shape enola wrote for Stop before this was fixed, which parses and never
+		// fires. Ours is dropped, which is how an existing installation migrates.
+		// Anyone else's is preserved verbatim: it is their file, and a hook that does
+		// not fire is still not ours to delete.
+		if _, grouped := group["hooks"]; !grouped {
+			if !isEnolaEntry(group) {
+				out = append(out, g)
+			}
+			continue
+		}
 		inner, _ := group["hooks"].([]any)
 		kept := make([]any, 0, len(inner))
 		for _, e := range inner {
 			if !isEnolaEntry(e) {
 				kept = append(kept, e)
 			}
 		}
-		if !remove && group["matcher"] == matcher {
+		if !remove && matcherOf(group) == matcher {
 			kept = append(kept, entry)
 			placed = true
 		}
@@ -157,14 +163,26 @@ func mergeMatcher(existing any, matcher string, entry map[string]any, remove boo
 	}
 
 	if !remove && !placed {
-		out = append(out, map[string]any{
-			"matcher": matcher,
-			"hooks":   []any{entry},
-		})
+		group := map[string]any{"hooks": []any{entry}}
+		if matcher != "" {
+			group["matcher"] = matcher
+		}
+		out = append(out, group)
 	}
 	return out
 }
 
+// matcherOf returns a group's matcher, treating an absent or non-string value as the
+// empty matcher — the group that applies unconditionally.
+//
+// Comparing group["matcher"] to a string directly cannot see that group: the value is
+// nil, and nil never equals "". Every install would then append another group instead
+// of updating the one already there, and uninstall would leave it behind.
+func matcherOf(group map[string]any) string {
+	s, _ := group["matcher"].(string)
+	return s
+}
+
 // isEnolaEntry reports whether a hook entry is one enola installed. Identified by an
 // explicit marker rather than by matching the command string, so a user who edits the
 // command — adding a flag, pointing at a different binary — still gets a clean uninstall.

pkg/install/hooks_e2e_test.go

@@ -0,0 +1,177 @@
+package install
+
+import (
+	"context"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+	"time"
+)
+
+// This is the check that would have caught the Stop hook never firing, and the only
+// kind that could have.
+//
+// The failure mode is a configuration that parses, reports success, and does
+// nothing. Every cheaper check passed while it was broken: `enola hook stop`
+// produced the right verdict when invoked by hand, the installer wrote the file it
+// meant to write, and a unit test asserted the shape against the same belief that
+// produced it. What settled it was ending a real session and looking at whether the
+// hook ran. So this test installs the hooks the way a user does, ends a session with
+// a real regression present, and asserts the verdict came out — the same discipline
+// TestPublishedExample_StillDemonstratesWhatItClaims applies to examples/cross-repo.
+//
+// It is opt-in because it spawns a real agent session. ENOLA_E2E=1 runs it. The skip
+// names its reason rather than passing silently: a test that quietly does nothing is
+// the thing being tested here.
+func TestStopHook_FiresInARealSession(t *testing.T) {
+	if os.Getenv("ENOLA_E2E") != "1" {
+		t.Skip("set ENOLA_E2E=1 to run: this spawns a real Claude Code session")
+	}
+	if runtime.GOOS == "windows" {
+		t.Skip("the hook wrapper is a POSIX shell script")
+	}
+	claude, err := exec.LookPath("claude")
+	if err != nil {
+		t.Skip("claude not on PATH: the Stop hook cannot be exercised end to end")
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), 8*time.Minute)
+	defer cancel()
+
+	work := t.TempDir()
+	enola := buildEnola(ctx, t, work)
+	repo := writeCyclePendingRepo(t, work)
+
+	// Pin the baseline BEFORE the regression exists — this is the "before" the Stop
+	// hook grades against. Pinned deliberately (no auto-pin marker), so the
+	// SessionStart hook leaves it alone and the session cannot re-baseline the
+	// regression away.
+	runCLI(ctx, t, repo, enola, "baseline", "pin", repo)
+
+	// Close the cycle. b already imports a; now a imports b.
+	writeFile(t, filepath.Join(repo, "a", "a.go"), `package a
+
+import "example.com/e2e/b"
+
+// A now calls back into b, closing a cycle.
+func A() string { return "a" + b.B() }
+`)
+
+	// The hook command is a wrapper around the real binary: it records that it ran
+	// at all, and tees the hook's stdout. Those are two different questions — the
+	// defect was that the hook never ran, not that it answered wrongly — and only
+	// the first is visible from outside the session.
+	log := filepath.Join(work, "hooks")
+	if err := os.MkdirAll(log, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	wrapper := filepath.Join(work, "enola-hook-wrapper")
+	writeFile(t, wrapper, "#!/bin/sh\n"+
+		"printf '%s\\n' \"$*\" >> '"+filepath.Join(log, "fired.log")+"'\n"+
+		"'"+enola+"' \"$@\" 2>> '"+filepath.Join(log, "stderr.log")+"'"+
+		" | tee -a '"+filepath.Join(log, "stdout.log")+"'\n")
+	if err := os.Chmod(wrapper, 0o755); err != nil {
+		t.Fatal(err)
+	}
+
+	if _, err := Install(Options{
+		Scope:       ScopeLocal,
+		RepoDir:     repo,
+		HomeDir:     t.TempDir(),
+		Hooks:       true,
+		HookCommand: wrapper,
+		Targets:     []string{"claude"},
+	}); err != nil {
+		t.Fatalf("Install: %v", err)
+	}
+
+	settings := filepath.Join(repo, ".claude", "settings.json")
+	session := exec.CommandContext(ctx, claude, "-p", "Say OK", "--settings", settings)
+	session.Dir = repo
+	if out, err := session.CombinedOutput(); err != nil {
+		t.Fatalf("claude session failed: %v\n%s", err, out)
+	}
+
+	fired := readIfPresent(filepath.Join(log, "fired.log"))
+	stdout := readIfPresent(filepath.Join(log, "stdout.log"))
+
+	if !strings.Contains(fired, "hook stop") {
+		t.Errorf("the Stop hook never ran.\nhooks that did run:\n%s\nsettings.json:\n%s",
+			fired, readIfPresent(settings))
+	}
+	// Firing is necessary but not sufficient: a hook that runs and stays silent
+	// leaves the session ungraded just as completely.
+	if !strings.Contains(stdout, "structural regression") {
+		t.Errorf("the Stop hook produced no regression verdict for a repo with a real cycle.\n"+
+			"stdout:\n%s\nstderr:\n%s", stdout, readIfPresent(filepath.Join(log, "stderr.log")))
+	}
+	// SessionStart was already correct, and is the control: if neither fired, the
+	// session itself did not run the way this test assumes.
+	if !strings.Contains(fired, "hook session-start") {
+		t.Errorf("SessionStart did not fire either — the session did not run as assumed:\n%s", fired)
+	}
+}
+
+// buildEnola compiles the binary under test. The hook invokes enola as a
+// subprocess, so an e2e run has to exercise a real build rather than this package.
+func buildEnola(ctx context.Context, t *testing.T, work string) string {
+	t.Helper()
+	bin := filepath.Join(work, "enola")
+	cmd := exec.CommandContext(ctx, "go", "build", "-o", bin, "./cmd/enola")
+	cmd.Dir = filepath.Join("..", "..")
+	if out, err := cmd.CombinedOutput(); err != nil {
+		t.Fatalf("building enola: %v\n%s", err, out)
+	}
+	return bin
+}
+
+// writeCyclePendingRepo creates a two-package Go module with a single edge b -> a.
+// Adding the opposite edge closes a cycle, which is what the default policy fails on.
+func writeCyclePendingRepo(t *testing.T, work string) string {
+	t.Helper()
+	repo := filepath.Join(work, "repo")
+	writeFile(t, filepath.Join(repo, "go.mod"), "module example.com/e2e\n\ngo 1.25\n")
+	writeFile(t, filepath.Join(repo, "a", "a.go"), `package a
+
+// A is the leaf.
+func A() string { return "a" }
+`)
+	writeFile(t, filepath.Join(repo, "b", "b.go"), `package b
+
+import "example.com/e2e/a"
+
+// B depends on a.
+func B() string { return a.A() }
+`)
+	return repo
+}
+
+func writeFile(t *testing.T, path, content string) {
+	t.Helper()
+	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(path, []byte(content), 0o644); err != nil {
+		t.Fatal(err)
+	}
+}
+
+func runCLI(ctx context.Context, t *testing.T, dir, bin string, args ...string) {
+	t.Helper()
+	cmd := exec.CommandContext(ctx, bin, args...)
+	cmd.Dir = dir
+	if out, err := cmd.CombinedOutput(); err != nil {
+		t.Fatalf("%s %s: %v\n%s", filepath.Base(bin), strings.Join(args, " "), err, out)
+	}
+}
+
+func readIfPresent(path string) string {
+	b, err := os.ReadFile(path)
+	if err != nil {
+		return "<file not written>"
+	}
+	return string(b)
+}