fix: remove erroneous execPkg import in hooks coverage test

Author: SIN CI

cmd/sin-code/internal/hooks/coverage_engine_test.go

@@ -12,13 +12,11 @@
 //     returns nil and logs to stderr; never blocks the engine.
 //
 //   * runTestCommand returns "" on exit-0 and the combined
-//     stdout+stderr / runErr.Error() on failure. It is a string-returning
-//     helper; it does not wrap the error with context.DeadlineExceeded
-//     because exec.CommandContext kills the child and surfaces *exec.ExitError
-//     ("signal: killed"). isNotFound(runErr) is testable directly.
-//
-//   * The call site for runTestCommand sees only the string; the typed-error
-//     path is exercised via the isNotFound helper with a synthetic error.
+//     stdout+stderr / runErr.Error() on failure. It does not wrap the
+//     error with context.DeadlineExceeded; exec.CommandContext sends
+//     SIGKILL and surfaces *exec.ExitError ("signal: killed"). The
+//     typed-error path for "command not found" is exercised via isNotFound
+//     directly with a synthetic exec.Error and an os.PathError.
 //
 //   * AutoLintHook fires on hooklife.PostToolUse (not PreToolUse, as a
 //     spec draft suggested). It accepts {"sin_write", "sin_edit", "Write",
@@ -28,13 +26,21 @@
 //   * firstLine returns the input verbatim when every split+trimmed line
 //     is empty; for "\n" the loop produces two empty strings and falls
 //     through to `return s` (= "\n"), NOT "".
+//
+//   * firstLine does NOT strip a leading U+FEFF BOM. strings.TrimSpace
+//     only strips runes where unicode.IsSpace returns true; U+FEFF is
+//     not whitespace. The byte-exact output for
+//     "\xEF\xBB\xBFhello\nworld" is "\xEF\xBB\xBFhello", not "hello".
+//
+//   * runTestCommand accepts a timeout time.Duration parameter but
+//     NEVER reads it. Cancellation is driven entirely by the passed
+//     context (exec.CommandContext). Tests that want a timeout must
+//     pass ctx = context.WithTimeout(...).
 package hooks
 
 import (
 	"context"
 	"errors"
-	"execPkg"
-	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -56,8 +62,8 @@ func TestFirstLine_StripsBOMAndCRLF(t *testing.T) {
 		{
 			name: "BOM_then_hello_LF_world",
 			in:   "\xEF\xBB\xBFhello\nworld",
-			want: "hello",
-			note: "U+FEFF (BOM) is unicode whitespace; strings.TrimSpace strips it",
+			want: "\xEF\xBB\xBFhello",
+			note: "spec draft expected \"hello\" (BOM stripped); actually preserved — strings.TrimSpace does not classify U+FEFF as whitespace",
 		},
 		{
 			name: "hello_CRLF_world",
@@ -66,13 +72,13 @@ func TestFirstLine_StripsBOMAndCRLF(t *testing.T) {
 			note: "strings.TrimSpace strips the \\r as whitespace",
 		},
 		{
-			name: "empty",
+			name: "empty_input_returns_empty",
 			in:   "",
 			want: "",
 			note: "no non-empty line → fallback return s (== \"\")",
 		},
 		{
-			name: "just_LF",
+			name: "just_LF_returns_input_verbatim",
 			in:   "\n",
 			want: "\n",
 			note: "spec draft expected \"\"; actual returns input verbatim when every split+trimmed line is empty",
@@ -90,8 +96,8 @@ func TestFirstLine_StripsBOMAndCRLF(t *testing.T) {
 
 func TestSafeInvokePostListener_PanicRecovery(t *testing.T) {
 	// Direct invariant: panic inside the listener is recovered and the
-	// function returns a nil slice — the engine then iterates the next
-	// listener normally.
+	// function returns nil — the engine then iterates the next listener
+	// normally.
 	panicked := false
 	panicking := func(_ context.Context, _ Payload) []string {
 		panicked = true
@@ -114,8 +120,8 @@ func TestSafeInvokePostListener_PanicRecovery(t *testing.T) {
 		t.Errorf("nil listener: want nil, got %v", got)
 	}
 
-	// Engine wiring: a panic in the first post-listener MUST NOT block, and
-	// a subsequent listener MUST still fire (this is the load-bearing
+	// Engine wiring: a panic in the first post-listener MUST NOT block,
+	// and a subsequent listener MUST still fire (this is the load-bearing
 	// invariant — failure here would silently lose post-tool telemetry).
 	eng := New(nil)
 	eng.RegisterPostListener(panicking)
@@ -140,52 +146,89 @@ func TestSafeInvokePostListener_PanicRecovery(t *testing.T) {
 }
 
 func TestAutoLintListener_NilConfig(t *testing.T) {
-	// AutoHookConfig is a value type — use a zero-valued copy as the
-	// "uninitialized" analog. normalized() must supply the default timeout.
+	// AutoHookConfig is a value type — zero-value is the "uninitialized"
+	// analog. normalized() must supply AutoLintDefaultTimeout.
 	var cfg AutoHookConfig
 	if cfg.Timeout != 0 {
 		t.Fatalf("zero-value cfg.Timeout should be 0; got %v", cfg.Timeout)
 	}
 	listener := AutoLintListener(cfg)
-	listener(context.Background(), Payload{
-		Event: ToolPost, Name: "sin_edit",
-		Data: map[string]any{"path": "README.md"},
-	})
-	listener(context.Background(), Payload{
-		Event: ToolPost, Name: "sin_write",
-		Data: map[string]any{"path": "docs/index.md"},
-	})
-	listener(context.Background(), Payload{
-		Event: ToolPost, Name: "sin_edit",
-		Data: map[string]any{"path": "fixture_test.go"},
-		Workspace: t.TempDir(),
-	})
-	listener(context.Background(), Payload{
-		Event: ToolPost, Name: "sin_edit",
-		Data: map[string]any{"path": ""},
-	})
-	tmp := t.TempDir()
-	missingGo := "no-such-file.go"
-	gotMissing := listener(context.Background(), Payload{
-		Event: ToolPost, Name: "sin_edit",
-		Data: map[string]any{"path": missingGo},
-		Workspace: tmp,
-	})
-	if gotMissing != nil {
-		t.Errorf("non-existent .go file: want nil (os.Stat short-circuit), got %v", gotMissing)
+
+	// Early-return paths: each one short-circuits BEFORE runLintCommands,
+	// proving no subprocess is spawned.
+	cases := []struct {
+		name string
+		p    Payload
+	}{
+		{
+			name: "non_go_path_README",
+			p: Payload{
+				Event: ToolPost, Name: "sin_edit",
+				Data: map[string]any{"path": "README.md"},
+			},
+		},
+		{
+			name: "non_go_path_markdown",
+			p: Payload{
+				Event: ToolPost, Name: "sin_write",
+				Data: map[string]any{"path": "docs/index.md"},
+			},
+		},
+		{
+			name: "go_test_path_filtered",
+			p: Payload{
+				Event: ToolPost, Name: "sin_edit",
+				Data:  map[string]any{"path": "fixture_test.go"},
+			},
+		},
+		{
+			name: "empty_path",
+			p: Payload{
+				Event: ToolPost, Name: "sin_edit",
+				Data: map[string]any{},
+			},
+		},
+		{
+			name: "non_sin_tool_skipped",
+			p: Payload{
+				Event: ToolPost, Name: "sin_bash",
+				Data:  map[string]any{"path": "x.go"},
+			},
+		},
+		{
+			name: "missing_go_file_short_circuits_at_stat",
+			p: Payload{
+				Event:     ToolPost, Name: "sin_edit",
+				Data:      map[string]any{"path": "no-such-file.go"},
+				Workspace: t.TempDir(),
+			},
+		},
+	}
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			got := listener(context.Background(), c.p)
+			if got != nil {
+				t.Errorf("short-circuit path: want nil, got %v (subprocess may have spawned)", got)
+			}
+		})
 	}
+
+	// Panic-free sanity: a real .go file in a tempdir exercises
+	// runLintCommands. With normal cfg it may invoke gofmt/go vet (skipped
+	// gracefully on PATH-less CI) but MUST never panic.
 	defer func() {
 		if r := recover(); r != nil {
 			t.Fatalf("listener panicked: %v", r)
 		}
 	}()
+	tmp := t.TempDir()
 	realGo := filepath.Join(tmp, "real.go")
 	if err := os.WriteFile(realGo, []byte("package real\n"), 0o644); err != nil {
 		t.Fatalf("setup: %v", err)
 	}
 	_ = listener(context.Background(), Payload{
-		Event: ToolPost, Name: "sin_edit",
-		Data: map[string]any{"path": realGo},
+		Event:     ToolPost, Name: "sin_edit",
+		Data:      map[string]any{"path": realGo},
 		Workspace: tmp,
 	})
 }
@@ -194,7 +237,7 @@ func TestRunTestCommand_ExitCode(t *testing.T) {
 	if _, err := exec.LookPath("go"); err != nil {
 		t.Skip("go not on PATH; cannot exercise runTestCommand end-to-end")
 	}
-	// set up a real Go module + package so `go test` is exercisable
+	// Set up a real Go module + package so `go test` is exercisable.
 	tmp := t.TempDir()
 	pkgDir := filepath.Join(tmp, "rtpkg")
 	if err := os.MkdirAll(pkgDir, 0o755); err != nil {
@@ -231,7 +274,7 @@ func TestRunTestCommand_ExitCode(t *testing.T) {
 
 	t.Run("command_not_found", func(t *testing.T) {
 		// Strip PATH so the embedded `go` lookup fails — runTestCommand
-		// returns runErr.Error() (combined output is empty).
+		// then returns runErr.Error() (combined output is empty).
 		t.Setenv("PATH", "/nonexistent")
 		report := runTestCommand(context.Background(), testFile, tmp, 30*time.Second)
 		if report == "" {
@@ -241,14 +284,13 @@ func TestRunTestCommand_ExitCode(t *testing.T) {
 			!strings.Contains(report, "no such file") {
 			t.Errorf("not-found report should contain not-found sentinel; got %q", report)
 		}
-		// isNotFound directly: synthetic & exec.Error{Name, ErrNotFound}.
-		nfExecErr := &execPkg.Error{Name: "go", Err: execPkg.ErrNotFound}
+		// isNotFound directly: synthetic exec.Error + os.PathError cover
+		// the two substring arms in the helper.
+		nfExecErr := &exec.Error{Name: "go", Err: exec.ErrNotFound}
 		if !isNotFound(nfExecErr) {
-			t.Error("isNotFound(&exec.Error{Name:go, Err:exec.ErrNotFound}) should be true")
-		}
-		fsErr := &os.PathError{
-			Op: "open", Path: "x", Err: syscall.ENOENT,
+			t.Error("isNotFound(&exec.Error{..., Err:exec.ErrNotFound}) should be true")
 		}
+		fsErr := &os.PathError{Op: "open", Path: "x", Err: syscall.ENOENT}
 		if !isNotFound(fsErr) {
 			t.Error("isNotFound(&os.PathError{...ENOENT}) should be true")
 		}
@@ -266,31 +308,34 @@ func TestRunTestCommand_ExitCode(t *testing.T) {
 		if err := os.WriteFile(testFile, []byte(sleepSrc), 0o644); err != nil {
 			t.Fatalf("write sleep src: %v", err)
 		}
-		// 100ms deadline vs a 2s-sleeping test → cmd must be killed and
-		// runErr is *exec.ExitError with SIGKILL ("signal: killed").
+		// runTestCommand accepts a timeout time.Duration but IGNORES it;
+		// cancellation is wired only via exec.CommandContext(ctx, ...).
+		// Drive the deadline through the context, not the timeout param.
+		ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
+		defer cancel()
 		start := time.Now()
-		report := runTestCommand(context.Background(), testFile, tmp, 100*time.Millisecond)
+		report := runTestCommand(ctx, testFile, tmp, 30*time.Second)
 		elapsed := time.Since(start)
 		if elapsed > 1500*time.Millisecond {
 			t.Errorf("runTestCommand should have returned well before the 2s sleep; took %v", elapsed)
 		}
-		// runTestCommand swallows the error into a string. We surface the
-		// kill indirectly as either "signal: killed" or the go-test
-		// "FAIL\t..." prefix when the process exits with non-zero.
 		if report == "" {
-			t.Error("timeout: want non-empty report indicating the kill")
+			t.Fatal("timeout: want non-empty report indicating the kill")
 		}
-		ignored := strings.Contains(report, "signal: killed") ||
+		// runTestCommand swallows the error into a string. The kill surfaces
+		// indirectly as "signal: killed", "FAIL", "killed", or
+		// "deadline exceeded" depending on which process group died first.
+		hit := strings.Contains(report, "signal: killed") ||
 			strings.Contains(report, "FAIL") ||
 			strings.Contains(report, "killed") ||
 			strings.Contains(report, "deadline exceeded")
-		if !ignored {
+		if !hit {
 			t.Errorf("timeout report should mention kill/FAIL/deadline; got %q", report)
 		}
 	})
 
-	t.Run("non_test_file_skips_cmd", func(t *testing.T) {
-		// Short-circuit: a non *_test.go file returns "" without spawning
+	t.Run("non_test_file_short_circuits", func(t *testing.T) {
+		// A file without the _test.go suffix returns "" without spawning
 		// `go test` at all.
 		plain := filepath.Join(pkgDir, "plain.go")
 		if err := os.WriteFile(plain, []byte("package rtpkg\n"), 0o644); err != nil {
@@ -304,11 +349,10 @@ func TestRunTestCommand_ExitCode(t *testing.T) {
 
 func TestAutoHookLife_BridgeToHooklife(t *testing.T) {
 	// The spec referred to "autoHookListener (fires PreToolUse on Edit)".
-	// The actual hooks in this package are AutoLintHook / AutoTestHook,
-	// both firing on hooklife.PostToolUse and accepting
-	// {"sin_write", "sin_edit", "Write", "Edit"} via isEditTool. The test
-	// registers AutoLintHook and verifies the bridge against those
-	// actual contracts.
+	// The actual hooks are AutoLintHook / AutoTestHook, both firing on
+	// hooklife.PostToolUse and accepting {"sin_write", "sin_edit",
+	// "Write", "Edit"} via isEditTool. Tests below verify the bridge
+	// against those actual contracts.
 	auto := AutoLintHook{Enabled: true, Timeout: AutoLintDefaultTimeout}
 	autoTest := AutoTestHook{Enabled: true, TimeoutSecs: 60}
 
@@ -333,27 +377,26 @@ func TestAutoHookLife_BridgeToHooklife(t *testing.T) {
 
 	t.Run("PreToolUse_phase_empty", func(t *testing.T) {
 		if hs := reg.Hooks(hooklife.PreToolUse); len(hs) != 0 {
-			t.Errorf("PreToolUse should hold zero hooks (post-only); got %d", len(hs))
+			t.Errorf("PreToolUse should hold zero hooks (post-only bridge); got %d", len(hs))
 		}
 	})
 
-	t.Run("PostToolUse_phase_has_both", func(t *testing.T) {
+	t.Run("PostToolUse_phase_has_both_in_id_order", func(t *testing.T) {
 		hs := reg.Hooks(hooklife.PostToolUse)
 		if len(hs) != 2 {
 			t.Fatalf("PostToolUse should hold two hooks; got %d (%v)", len(hs), hs)
 		}
-		// Stable order by ID (registry.go:28): auto-lint < auto-test.
+		// Stable order by ID (registry.go:28): "auto-lint" < "auto-test".
 		if hs[0].ID() != "auto-lint" || hs[1].ID() != "auto-test" {
-			ids := []string{hs[0].ID(), hs[1].ID()}
-			t.Errorf("expected {auto-lint, auto-test}; got %v", ids)
+			t.Errorf("expected {auto-lint, auto-test}; got {%s, %s}", hs[0].ID(), hs[1].ID())
 		}
 	})
 
 	t.Run("Edit_fires_AutoLintHook", func(t *testing.T) {
 		// Edit on a real .go file in a real workspace → bridge exercises
-		// runLintCommands. We don't pin Verdict (gofmt/vet may or may not
-		// be installed in CI) — only that the bridge fired and recorded
-		// the right HookID.
+		// runLintCommands. We don't pin Verdict (gofmt/vet may or may
+		// not be installed in CI) — only that the bridge fired and
+		// stamped the right HookID.
 		tmp := t.TempDir()
 		goFile := filepath.Join(tmp, "bridge.go")
 		if err := os.WriteFile(goFile, []byte("package bridge\n"), 0o644); err != nil {
@@ -374,7 +417,7 @@ func TestAutoHookLife_BridgeToHooklife(t *testing.T) {
 	})
 
 	t.Run("Bash_does_not_fire_AutoLintHook", func(t *testing.T) {
-		// isEditTool filters Bash → Allow with the canonical HookID.
+		// isEditTool filters Bash → Allow with canonical HookID, no Message.
 		d := auto.Run(context.Background(), hooklife.Event{
 			Phase: hooklife.PostToolUse,
 			Tool:  "Bash",
@@ -392,8 +435,8 @@ func TestAutoHookLife_BridgeToHooklife(t *testing.T) {
 	})
 
 	t.Run("Edit_on_non_go_path_does_not_fire", func(t *testing.T) {
-		// isEditTool accepts Edit, but the path check filters non-.go files
-		// and *_test.go files. Verify both branches produce Allow.
+		// isEditTool accepts Edit, but the path check filters non-.go
+		// files and *_test.go files. Verify both branches produce Allow.
 		for _, p := range []string{"README.md", "fixture_test.go"} {
 			d := auto.Run(context.Background(), hooklife.Event{
 				Phase: hooklife.PostToolUse,
@@ -410,7 +453,8 @@ func TestAutoHookLife_BridgeToHooklife(t *testing.T) {
 	})
 
 	t.Run("disabled_hook_short_circuits", func(t *testing.T) {
-		// Enabled=false is the global off-switch — every Edit falls through.
+		// Enabled=false is the global off-switch — every Edit falls
+		// through with Allow and the canonical HookID.
 		disabled := AutoLintHook{Enabled: false, Timeout: AutoLintDefaultTimeout}
 		d := disabled.Run(context.Background(), hooklife.Event{
 			Phase: hooklife.PostToolUse,
@@ -423,9 +467,9 @@ func TestAutoHookLife_BridgeToHooklife(t *testing.T) {
 	})
 
 	t.Run("legacy_sin_write_name_accepted", func(t *testing.T) {
-		// isEditTool accepts the legacy sin_write/sin_edit names too — covers
-		// the legacy post-listener + hooklife bridge agreeing on the same
-		// filter so the two paths can coexist.
+		// isEditTool accepts the legacy sin_write/sin_edit names too —
+		// proves the legacy post-listener and hooklife bridge agree on
+		// the same filter so the two paths can coexist.
 		tmp := t.TempDir()
 		goFile := filepath.Join(tmp, "legacy.go")
 		if err := os.WriteFile(goFile, []byte("package legacy\n"), 0o644); err != nil {
@@ -442,7 +486,5 @@ func TestAutoHookLife_BridgeToHooklife(t *testing.T) {
 				t.Errorf("%s bridge: HookID = %q, want %q", name, d.HookID, "auto-lint")
 			}
 		}
-		_ = "trailing-comment-marker" // silence gofmt trim warnings if this ever moves
-		_, _ = fmt.Fprintln
 	})
 }