From b33d139914781d394ada66c9fa9ad9a5b041ad65 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 18 Feb 2026 13:57:13 -0600 Subject: [PATCH 1/6] Consolidate hook system into internal/githook, fix early exit bug Move all hook install/upgrade/uninstall logic from cmd/roborev/main.go and internal/daemon/server.go into a new internal/githook package, eliminating ~300 lines of duplicated code. Fix #298: hooks embedded in existing scripts (e.g. husky) now use a function wrapper inserted after the shebang instead of appending at the end, so they run before any `exit 0` in the existing hook. Key changes: - New internal/githook package with Install, InstallAll, Uninstall, NeedsUpgrade, Missing, and content generators - Embeddable snippets use _roborev_hook()/_roborev_remap() function wrappers with `return 0` instead of `exit 0` - embedSnippet() inserts after shebang, not at end of file - Bump version markers to v3/v2 to trigger upgrades - Auto-install missing/outdated hooks from `roborev review` (non-quiet) - Uninstall handles v3 function-wrapped format (} and return 0) - Integration test for husky-style early exit hook Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/hook_integration_test.go | 126 ++- cmd/roborev/hook_test.go | 1015 +-------------------- cmd/roborev/main.go | 467 +--------- internal/daemon/server.go | 50 +- internal/githook/githook.go | 459 ++++++++++ internal/githook/githook_test.go | 1258 ++++++++++++++++++++++++++ 6 files changed, 1881 insertions(+), 1494 deletions(-) create mode 100644 internal/githook/githook.go create mode 100644 internal/githook/githook_test.go diff --git a/cmd/roborev/hook_integration_test.go b/cmd/roborev/hook_integration_test.go index a4313ddf..7c8bd06d 100644 --- a/cmd/roborev/hook_integration_test.go +++ b/cmd/roborev/hook_integration_test.go @@ -4,10 +4,12 @@ package main import ( "os" + "path/filepath" "runtime" "strings" "testing" + "github.com/roborev-dev/roborev/internal/githook" "github.com/roborev-dev/roborev/internal/testutil" ) @@ -16,7 +18,6 @@ func TestInitCmdCreatesHooksDirectory(t *testing.T) { t.Skip("test uses shell script stub, skipping on Windows") } - // Override HOME to prevent reading real daemon.json tmpHome := t.TempDir() origHome := os.Getenv("HOME") os.Setenv("HOME", tmpHome) @@ -25,37 +26,29 @@ func TestInitCmdCreatesHooksDirectory(t *testing.T) { repo := testutil.NewTestRepo(t) repo.RemoveHooksDir() - // Verify hooks directory doesn't exist if _, err := os.Stat(repo.HooksDir); !os.IsNotExist(err) { t.Fatal("hooks directory should not exist before test") } - // Create a fake roborev binary in PATH defer testutil.MockBinaryInPath(t, "roborev", "#!/bin/sh\nexit 0\n")() - defer repo.Chdir()() - // Build init command initCommand := initCmd() initCommand.SetArgs([]string{"--agent", "test"}) err := initCommand.Execute() - // Should succeed (not fail with "no such file or directory") if err != nil { t.Fatalf("init command failed: %v", err) } - // Verify hooks directory was created if _, err := os.Stat(repo.HooksDir); os.IsNotExist(err) { t.Error("hooks directory was not created") } - // Verify hook file was created if _, err := os.Stat(repo.HookPath); os.IsNotExist(err) { t.Error("post-commit hook was not created") } - // Verify hook is executable info, err := os.Stat(repo.HookPath) if err != nil { t.Fatal(err) @@ -77,8 +70,15 @@ func TestInitCmdUpgradesOutdatedHook(t *testing.T) { repo := testutil.NewTestRepo(t) - // Write a realistic old-style hook (no version marker, has &, includes if/fi block) - oldHook := "#!/bin/sh\n# roborev post-commit hook - auto-reviews every commit\nROBOREV=\"/usr/local/bin/roborev\"\nif [ ! -x \"$ROBOREV\" ]; then\n ROBOREV=$(command -v roborev 2>/dev/null)\n [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ] && exit 0\nfi\n\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" + // Write a realistic old-style hook (v2 format) + oldHook := "#!/bin/sh\n" + + "# roborev post-commit hook v2 - auto-reviews every commit\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "if [ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=$(command -v roborev 2>/dev/null)\n" + + " [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ] && exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" repo.WriteHook(oldHook) defer testutil.MockBinaryInPath(t, "roborev", "#!/bin/sh\nexit 0\n")() @@ -97,18 +97,14 @@ func TestInitCmdUpgradesOutdatedHook(t *testing.T) { } contentStr := string(content) - if !strings.Contains(contentStr, "hook v2") { - t.Error("upgraded hook should contain version marker") - } - if strings.Contains(contentStr, "2>/dev/null &") { - t.Error("upgraded hook should not have backgrounded enqueue") + if !strings.Contains(contentStr, githook.PostCommitVersionMarker) { + t.Errorf("upgraded hook should contain v3 marker, got:\n%s", contentStr) } - if !strings.Contains(contentStr, "2>/dev/null\n") { - t.Error("upgraded hook should still have enqueue line (without &)") + if strings.Contains(contentStr, "hook v2") { + t.Error("upgraded hook should not contain old v2 marker") } - // Verify the if/fi block is preserved intact (no stray fi) - if !strings.Contains(contentStr, "if [ ! -x") { - t.Error("upgraded hook should preserve the if block") + if !strings.Contains(contentStr, "enqueue --quiet") { + t.Error("upgraded hook should have enqueue line") } } @@ -124,7 +120,16 @@ func TestInitCmdPreservesOtherHooksOnUpgrade(t *testing.T) { repo := testutil.NewTestRepo(t) - oldHook := "#!/bin/sh\necho 'my custom hook'\n# roborev post-commit hook - auto-reviews every commit\nROBOREV=\"/usr/local/bin/roborev\"\nif [ ! -x \"$ROBOREV\" ]; then\n ROBOREV=$(command -v roborev 2>/dev/null)\n [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ] && exit 0\nfi\n\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" + // Mixed hook: user content + old v2 roborev snippet + oldHook := "#!/bin/sh\n" + + "echo 'my custom hook'\n" + + "# roborev post-commit hook v2 - auto-reviews every commit\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "if [ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=$(command -v roborev 2>/dev/null)\n" + + " [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ] && exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" repo.WriteHook(oldHook) defer testutil.MockBinaryInPath(t, "roborev", "#!/bin/sh\nexit 0\n")() @@ -146,10 +151,79 @@ func TestInitCmdPreservesOtherHooksOnUpgrade(t *testing.T) { if !strings.Contains(contentStr, "echo 'my custom hook'") { t.Error("upgrade should preserve non-roborev lines") } - if !strings.Contains(contentStr, "hook v2") { - t.Error("upgrade should contain version marker") + if !strings.Contains(contentStr, githook.PostCommitVersionMarker) { + t.Errorf("upgrade should contain v3 marker, got:\n%s", contentStr) + } + if strings.Contains(contentStr, "hook v2") { + t.Error("upgrade should remove old v2 marker") + } +} + +func TestInitCmdEarlyExitHookStillRunsRoborev(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test uses shell script stub, skipping on Windows") + } + + tmpHome := t.TempDir() + origHome := os.Getenv("HOME") + os.Setenv("HOME", tmpHome) + defer os.Setenv("HOME", origHome) + + repo := testutil.NewTestRepo(t) + + // Husky-style hook with exit 0 at the end + huskyHook := "#!/bin/sh\n" + + ". \"$(dirname \"$0\")/_/husky.sh\"\n" + + "npx lint-staged\n" + + "exit 0\n" + repo.WriteHook(huskyHook) + + defer testutil.MockBinaryInPath(t, "roborev", "#!/bin/sh\nexit 0\n")() + defer repo.Chdir()() + + initCommand := initCmd() + initCommand.SetArgs([]string{"--agent", "test"}) + err := initCommand.Execute() + if err != nil { + t.Fatalf("init command failed: %v", err) + } + + content, err := os.ReadFile(repo.HookPath) + if err != nil { + t.Fatalf("Failed to read hook: %v", err) + } + + contentStr := string(content) + + // Roborev snippet should appear before exit 0 + snippetIdx := strings.Index(contentStr, "_roborev_hook") + exitIdx := strings.Index(contentStr, "exit 0") + if snippetIdx < 0 { + t.Fatal("roborev snippet should be present") + } + if exitIdx < 0 { + t.Fatal("exit 0 should be preserved") + } + if snippetIdx > exitIdx { + t.Error("roborev snippet should appear before exit 0") + } + + // All original content should be preserved + if !strings.Contains(contentStr, "husky.sh") { + t.Error("husky.sh reference should be preserved") + } + if !strings.Contains(contentStr, "npx lint-staged") { + t.Error("lint-staged command should be preserved") + } + + // Post-rewrite hook should also be installed + prContent, err := os.ReadFile( + filepath.Join(repo.HooksDir, "post-rewrite"), + ) + if err != nil { + t.Fatalf("post-rewrite hook should be created: %v", err) } - if strings.Contains(contentStr, "2>/dev/null &") { - t.Error("upgrade should remove trailing &") + if !strings.Contains(string(prContent), githook.PostRewriteVersionMarker) { + t.Error("post-rewrite hook should have version marker") } } diff --git a/cmd/roborev/hook_test.go b/cmd/roborev/hook_test.go index 2731c68d..3a48c1e1 100644 --- a/cmd/roborev/hook_test.go +++ b/cmd/roborev/hook_test.go @@ -3,7 +3,6 @@ package main import ( "errors" "fmt" - "io/fs" "net" "net/http" "net/http/httptest" @@ -14,6 +13,7 @@ import ( "strings" "testing" + "github.com/roborev-dev/roborev/internal/githook" "github.com/roborev-dev/roborev/internal/testutil" ) @@ -28,7 +28,6 @@ func TestUninstallHookCmd(t *testing.T) { t.Fatalf("uninstall-hook failed: %v", err) } - // Hook should still not exist if _, err := os.Stat(repo.HookPath); !os.IsNotExist(err) { t.Error("Hook file should not exist") } @@ -46,7 +45,6 @@ func TestUninstallHookCmd(t *testing.T) { t.Fatalf("uninstall-hook failed: %v", err) } - // Hook should be unchanged content, err := os.ReadFile(repo.HookPath) if err != nil { t.Fatalf("Failed to read hook: %v", err) @@ -58,7 +56,7 @@ func TestUninstallHookCmd(t *testing.T) { t.Run("hook with roborev only - removes file", func(t *testing.T) { repo := testutil.NewTestRepo(t) - repo.WriteHook(generateHookContent()) + repo.WriteHook(githook.GeneratePostCommit()) defer repo.Chdir()() cmd := uninstallHookCmd() @@ -67,7 +65,6 @@ func TestUninstallHookCmd(t *testing.T) { t.Fatalf("uninstall-hook failed: %v", err) } - // Hook should be removed entirely if _, err := os.Stat(repo.HookPath); !os.IsNotExist(err) { t.Error("Hook file should have been removed") } @@ -76,7 +73,7 @@ func TestUninstallHookCmd(t *testing.T) { t.Run("hook with roborev and other commands - preserves others", func(t *testing.T) { repo := testutil.NewTestRepo(t) mixed := "#!/bin/sh\necho 'before'\necho 'after'\n" + - generateHookContent() + githook.GeneratePostCommit() repo.WriteHook(mixed) defer repo.Chdir()() @@ -86,7 +83,6 @@ func TestUninstallHookCmd(t *testing.T) { t.Fatalf("uninstall-hook failed: %v", err) } - // Hook should exist with roborev snippet removed content, err := os.ReadFile(repo.HookPath) if err != nil { t.Fatalf("Failed to read hook: %v", err) @@ -106,12 +102,11 @@ func TestUninstallHookCmd(t *testing.T) { t.Run("also removes post-rewrite hook", func(t *testing.T) { repo := testutil.NewTestRepo(t) - repo.WriteHook(generateHookContent()) - // Also install post-rewrite hook + repo.WriteHook(githook.GeneratePostCommit()) prPath := filepath.Join(repo.HooksDir, "post-rewrite") os.WriteFile( prPath, - []byte(generatePostRewriteHookContent()), + []byte(githook.GeneratePostRewrite()), 0755, ) defer repo.Chdir()() @@ -132,12 +127,11 @@ func TestUninstallHookCmd(t *testing.T) { t.Run("removes post-rewrite even without post-commit", func(t *testing.T) { repo := testutil.NewTestRepo(t) - // Only install post-rewrite, no post-commit prPath := filepath.Join(repo.HooksDir, "post-rewrite") os.MkdirAll(repo.HooksDir, 0755) os.WriteFile( prPath, - []byte(generatePostRewriteHookContent()), + []byte(githook.GeneratePostRewrite()), 0755, ) defer repo.Chdir()() @@ -162,14 +156,12 @@ func TestInstallHookCmdCreatesHooksDirectory(t *testing.T) { repo := testutil.NewTestRepo(t) repo.RemoveHooksDir() - // Verify hooks directory doesn't exist if _, err := os.Stat(repo.HooksDir); !os.IsNotExist(err) { t.Fatal("hooks directory should not exist before test") } defer repo.Chdir()() - // Run install-hook command installCmd := installHookCmd() installCmd.SetArgs([]string{}) err := installCmd.Execute() @@ -178,198 +170,42 @@ func TestInstallHookCmdCreatesHooksDirectory(t *testing.T) { t.Fatalf("install-hook command failed: %v", err) } - // Verify hooks directory was created if _, err := os.Stat(repo.HooksDir); os.IsNotExist(err) { t.Error("hooks directory was not created") } - // Verify hook file was created if _, err := os.Stat(repo.HookPath); os.IsNotExist(err) { t.Error("post-commit hook was not created") } } -func TestGenerateHookContent(t *testing.T) { - content := generateHookContent() - lines := strings.Split(content, "\n") - - t.Run("has shebang", func(t *testing.T) { - if !strings.HasPrefix(content, "#!/bin/sh\n") { - t.Error("hook should start with #!/bin/sh") - } - }) - - t.Run("has roborev comment", func(t *testing.T) { - if !strings.Contains(content, "# roborev") { - t.Error("hook should contain roborev comment for detection") - } - }) - - t.Run("baked path comes first", func(t *testing.T) { - // Security: baked path should be set before any PATH lookup - bakedIdx := -1 - pathIdx := -1 - for i, line := range lines { - if strings.HasPrefix(line, "ROBOREV=") && !strings.Contains(line, "command -v") { - bakedIdx = i - } - if strings.Contains(line, "command -v roborev") { - pathIdx = i - } - } - if bakedIdx == -1 { - t.Error("hook should have baked ROBOREV= assignment") - } - if pathIdx == -1 { - t.Error("hook should have PATH fallback via command -v") - } - if bakedIdx > pathIdx { - t.Error("baked path should come before PATH lookup for security") - } - }) - - t.Run("enqueue line has quiet and stderr redirect without background", func(t *testing.T) { - found := false - for _, line := range lines { - if strings.Contains(line, "enqueue --quiet") && - strings.Contains(line, "2>/dev/null") && - !strings.HasSuffix(strings.TrimSpace(line), "&") { - found = true - break - } - } - if !found { - t.Error("hook should have enqueue line with --quiet and 2>/dev/null but no trailing &") - } - }) - - t.Run("has version marker", func(t *testing.T) { - if !strings.Contains(content, "hook v2") { - t.Error("hook should contain version marker 'hook v2'") - } - }) - - t.Run("baked path is quoted", func(t *testing.T) { - // The baked path should be properly quoted to handle spaces - for _, line := range lines { - if strings.HasPrefix(line, "ROBOREV=") && !strings.Contains(line, "command -v") { - // Should be ROBOREV="/path/to/roborev" or ROBOREV="roborev" - if !strings.Contains(line, `ROBOREV="`) { - t.Errorf("baked path should be quoted, got: %s", line) - } - break - } - } - }) - - t.Run("baked path is absolute", func(t *testing.T) { - // os.Executable() returns absolute path; verify it's baked correctly - for _, line := range lines { - if strings.HasPrefix(line, "ROBOREV=") && !strings.Contains(line, "command -v") { - // Extract the path from ROBOREV="/path/here" - start := strings.Index(line, `"`) - end := strings.LastIndex(line, `"`) - if start != -1 && end > start { - path := line[start+1 : end] - if !filepath.IsAbs(path) { - t.Errorf("baked path should be absolute, got: %s", path) - } - } - break - } - } - }) -} - -func TestHookNeedsUpgrade(t *testing.T) { - t.Run("outdated hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\n# roborev post-commit hook\nroborev enqueue\n") - if !hookNeedsUpgrade(repo.Root, "post-commit", hookVersionMarker) { - t.Error("should detect outdated hook") - } - }) - - t.Run("current hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\n# roborev post-commit hook v2 - auto-reviews every commit\nroborev enqueue\n") - if hookNeedsUpgrade(repo.Root, "post-commit", hookVersionMarker) { - t.Error("should not flag current hook as outdated") - } - }) - - t.Run("no hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if hookNeedsUpgrade(repo.Root, "post-commit", hookVersionMarker) { - t.Error("should not flag missing hook as outdated") - } - }) - - t.Run("non-roborev hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\necho hello\n") - if hookNeedsUpgrade(repo.Root, "post-commit", hookVersionMarker) { - t.Error("should not flag non-roborev hook as outdated") - } - }) - - t.Run("post-rewrite outdated", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - hooksDir := filepath.Join(repo.Root, ".git", "hooks") - os.MkdirAll(hooksDir, 0755) - os.WriteFile(filepath.Join(hooksDir, "post-rewrite"), - []byte("#!/bin/sh\n# roborev hook\nroborev remap\n"), 0755) - if !hookNeedsUpgrade(repo.Root, "post-rewrite", postRewriteHookVersionMarker) { - t.Error("should detect outdated post-rewrite hook") - } - }) - - t.Run("post-rewrite current", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - hooksDir := filepath.Join(repo.Root, ".git", "hooks") - os.MkdirAll(hooksDir, 0755) - os.WriteFile(filepath.Join(hooksDir, "post-rewrite"), - []byte("#!/bin/sh\n# roborev post-rewrite hook v1\nroborev remap\n"), 0755) - if hookNeedsUpgrade(repo.Root, "post-rewrite", postRewriteHookVersionMarker) { - t.Error("should not flag current post-rewrite hook") - } - }) -} +func TestInstallHookCmdCreatesPostRewriteHook(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test checks Unix exec bits, skipping on Windows") + } -func TestHookMissing(t *testing.T) { - t.Run("missing post-rewrite with roborev post-commit", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\n# roborev post-commit hook v2\nroborev enqueue\n") - if !hookMissing(repo.Root, "post-rewrite") { - t.Error("should detect missing post-rewrite when post-commit has roborev") - } - }) + repo := testutil.NewTestRepo(t) + repo.RemoveHooksDir() + defer repo.Chdir()() - t.Run("no post-commit hook at all", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if hookMissing(repo.Root, "post-rewrite") { - t.Error("should not warn when post-commit is not installed") - } - }) + installCmd := installHookCmd() + installCmd.SetArgs([]string{}) + if err := installCmd.Execute(); err != nil { + t.Fatalf("install-hook failed: %v", err) + } - t.Run("post-rewrite exists with roborev", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\n# roborev post-commit hook v2\nroborev enqueue\n") - hooksDir := filepath.Join(repo.Root, ".git", "hooks") - os.WriteFile(filepath.Join(hooksDir, "post-rewrite"), - []byte("#!/bin/sh\n# roborev post-rewrite hook v1\nroborev remap\n"), 0755) - if hookMissing(repo.Root, "post-rewrite") { - t.Error("should not warn when post-rewrite has roborev content") - } - }) + prHookPath := filepath.Join(repo.HooksDir, "post-rewrite") + content, err := os.ReadFile(prHookPath) + if err != nil { + t.Fatalf("post-rewrite hook not created: %v", err) + } - t.Run("non-roborev post-commit", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - repo.WriteHook("#!/bin/sh\necho hello\n") - if hookMissing(repo.Root, "post-rewrite") { - t.Error("should not warn when post-commit is not roborev") - } - }) + if !strings.Contains(string(content), "remap --quiet") { + t.Error("post-rewrite hook should contain 'remap --quiet'") + } + if !strings.Contains(string(content), githook.PostRewriteVersionMarker) { + t.Error("post-rewrite hook should contain version marker") + } } func TestIsTransportError(t *testing.T) { @@ -383,7 +219,6 @@ func TestIsTransportError(t *testing.T) { }) t.Run("url.Error without OpError is not transport error", func(t *testing.T) { - // e.g. malformed URL, TLS config error err := &url.Error{Op: "Post", URL: "http://127.0.0.1:7373", Err: errors.New("some non-transport error")} if isTransportError(err) { t.Error("expected url.Error without net.OpError to NOT be transport error") @@ -421,7 +256,6 @@ func TestRegisterRepoError(t *testing.T) { t.Errorf("unexpected error message: %s", err.Error()) } - // Verify it satisfies the error interface and is distinguishable var regErr *registerRepoError if !errors.As(err, ®Err) { t.Error("expected errors.As to match registerRepoError") @@ -433,7 +267,6 @@ func TestRegisterRepoError(t *testing.T) { // initNoDaemonSetup prepares the environment for init --no-daemon tests: // isolated HOME, fake roborev binary, and chdir to a test repo. -// Returns the repo path and a cleanup function. func initNoDaemonSetup(t *testing.T) string { t.Helper() @@ -455,9 +288,8 @@ func TestInitNoDaemon_ConnectionError(t *testing.T) { } initNoDaemonSetup(t) - // Point to a port that's definitely not listening oldAddr := serverAddr - serverAddr = "http://127.0.0.1:1" // port 1 won't have a daemon + serverAddr = "http://127.0.0.1:1" defer func() { serverAddr = oldAddr }() output := captureStdout(t, func() { @@ -483,7 +315,6 @@ func TestInitNoDaemon_ServerError(t *testing.T) { } initNoDaemonSetup(t) - // Spin up a test server that returns 500 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(500) _, _ = w.Write([]byte("database locked")) @@ -520,7 +351,6 @@ func TestInitNoDaemon_Success(t *testing.T) { } initNoDaemonSetup(t) - // Spin up a test server that returns 200 ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) })) @@ -547,780 +377,18 @@ func TestInitNoDaemon_Success(t *testing.T) { } } -func TestInstallHookCmdCreatesPostRewriteHook(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("test checks Unix exec bits, skipping on Windows") - } - - repo := testutil.NewTestRepo(t) - repo.RemoveHooksDir() - defer repo.Chdir()() - - installCmd := installHookCmd() - installCmd.SetArgs([]string{}) - if err := installCmd.Execute(); err != nil { - t.Fatalf("install-hook failed: %v", err) - } - - prHookPath := filepath.Join(repo.HooksDir, "post-rewrite") - content, err := os.ReadFile(prHookPath) - if err != nil { - t.Fatalf("post-rewrite hook not created: %v", err) - } - - if !strings.Contains(string(content), "remap --quiet") { - t.Error("post-rewrite hook should contain 'remap --quiet'") - } - if !strings.Contains(string(content), postRewriteHookVersionMarker) { - t.Error("post-rewrite hook should contain version marker") - } -} - -func TestInstallOrUpgradeHook(t *testing.T) { - t.Run("appends to existing non-roborev hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - existing := "#!/bin/sh\necho 'custom logic'\n" - if err := os.WriteFile(hookPath, []byte(existing), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err != nil { - t.Fatalf("installOrUpgradeHook: %v", err) - } - - content, _ := os.ReadFile(hookPath) - contentStr := string(content) - if !strings.Contains(contentStr, "echo 'custom logic'") { - t.Error("original content should be preserved") - } - if !strings.Contains(contentStr, hookVersionMarker) { - t.Error("roborev snippet should be appended") - } - }) - - t.Run("skips current version", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - current := generatePostRewriteHookContent() - if err := os.WriteFile(hookPath, []byte(current), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-rewrite", - postRewriteHookVersionMarker, - generatePostRewriteHookContent, false, - ) - if err != nil { - t.Fatalf("installOrUpgradeHook: %v", err) - } - - content, _ := os.ReadFile(hookPath) - if string(content) != current { - t.Error("current hook should not be modified") - } - }) - - t.Run("upgrades outdated roborev hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - // Simulates an older generated hook (has marker but no version) - outdated := "#!/bin/sh\n" + - "# roborev post-commit hook\n" + - "ROBOREV=\"/usr/local/bin/roborev\"\n" + - "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" - if err := os.WriteFile(hookPath, []byte(outdated), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err != nil { - t.Fatalf("installOrUpgradeHook: %v", err) - } - - content, _ := os.ReadFile(hookPath) - contentStr := string(content) - if !strings.Contains(contentStr, hookVersionMarker) { - t.Error("should have new version marker") - } - // Old marker should be gone - if strings.Contains(contentStr, "# roborev post-commit hook\n") { - t.Error("old marker should be removed") - } - }) - - t.Run("upgrades mixed hook preserving user content", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - mixed := "#!/bin/sh\necho 'user code'\n# roborev post-rewrite hook\nROBOREV=\"/usr/bin/roborev\"\n\"$ROBOREV\" remap --quiet 2>/dev/null\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-rewrite", - postRewriteHookVersionMarker, - generatePostRewriteHookContent, false, - ) - if err != nil { - t.Fatalf("installOrUpgradeHook: %v", err) - } - - content, _ := os.ReadFile(hookPath) - contentStr := string(content) - if !strings.Contains(contentStr, "echo 'user code'") { - t.Error("user content should be preserved") - } - if !strings.Contains(contentStr, postRewriteHookVersionMarker) { - t.Error("should have new version marker") - } - }) - - t.Run("appends to hooks with various shell shebangs", func(t *testing.T) { - shebangs := []string{ - "#!/bin/sh", "#!/usr/bin/env sh", - "#!/bin/bash", "#!/usr/bin/env bash", - "#!/bin/zsh", "#!/usr/bin/env zsh", - "#!/bin/ksh", "#!/usr/bin/env ksh", - "#!/bin/dash", "#!/usr/bin/env dash", - } - for _, shebang := range shebangs { - t.Run(shebang, func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - existing := shebang + "\necho 'custom'\n" - if err := os.WriteFile(hookPath, []byte(existing), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err != nil { - t.Fatalf("should append to %s hook: %v", shebang, err) - } - - content, _ := os.ReadFile(hookPath) - if !strings.Contains(string(content), "echo 'custom'") { - t.Error("original content should be preserved") - } - if !strings.Contains(string(content), hookVersionMarker) { - t.Error("roborev content should be appended") - } - }) - } - }) - - t.Run("refuses to append to non-shell hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - pythonHook := "#!/usr/bin/env python3\nprint('hello')\n" - if err := os.WriteFile(hookPath, []byte(pythonHook), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err == nil { - t.Fatal("expected error for non-shell hook") - } - if !strings.Contains(err.Error(), "non-shell interpreter") { - t.Errorf("unexpected error: %v", err) - } - - // Verify hook was not modified - content, _ := os.ReadFile(hookPath) - if string(content) != pythonHook { - t.Errorf("hook should be unchanged, got:\n%s", content) - } - }) - - t.Run("upgrade returns error on re-read failure", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - // Outdated hook triggers the upgrade path. - outdated := "#!/bin/sh\n" + - "# roborev post-commit hook\n" + - "ROBOREV=\"/usr/local/bin/roborev\"\n" + - "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" - if err := os.WriteFile( - hookPath, []byte(outdated), 0755, - ); err != nil { - t.Fatal(err) - } - - // Inject a non-ENOENT error on re-read after cleanup. - origReadFile := hookReadFile - hookReadFile = func(string) ([]byte, error) { - return nil, fs.ErrPermission - } - t.Cleanup(func() { hookReadFile = origReadFile }) - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err == nil { - t.Fatal("expected error from re-read failure") - } - if !strings.Contains(err.Error(), "re-read") { - t.Errorf("error should mention re-read, got: %v", err) - } - if !errors.Is(err, fs.ErrPermission) { - t.Errorf( - "error should wrap ErrPermission, got: %v", err, - ) - } - }) - - t.Run("refuses upgrade of non-shell hook mentioning roborev", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - // Non-shell hook that mentions roborev but has no version marker. - pythonHook := "#!/usr/bin/env python3\n# reviewed by roborev\nprint('hello')\n" - if err := os.WriteFile(hookPath, []byte(pythonHook), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, false, - ) - if err == nil { - t.Fatal("expected error for non-shell hook in upgrade path") - } - if !strings.Contains(err.Error(), "non-shell interpreter") { - t.Errorf("unexpected error: %v", err) - } - - // Hook must not be modified. - content, _ := os.ReadFile(hookPath) - if string(content) != pythonHook { - t.Errorf("hook should be unchanged, got:\n%s", content) - } - }) - - t.Run("force overwrites existing hook", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - existing := "#!/bin/sh\necho 'custom'\n" - if err := os.WriteFile(hookPath, []byte(existing), 0755); err != nil { - t.Fatal(err) - } - - err := installOrUpgradeHook( - repo.HooksDir, "post-commit", - hookVersionMarker, generateHookContent, true, - ) - if err != nil { - t.Fatalf("installOrUpgradeHook: %v", err) - } - - content, _ := os.ReadFile(hookPath) - contentStr := string(content) - if strings.Contains(contentStr, "echo 'custom'") { - t.Error("force should overwrite, not append") - } - if !strings.Contains(contentStr, hookVersionMarker) { - t.Error("should have roborev content") - } - }) -} - -func TestRemoveRoborevFromHook(t *testing.T) { - t.Run("generated hook is deleted entirely", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - hookContent := generatePostRewriteHookContent() - if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - if _, err := os.Stat(hookPath); !os.IsNotExist(err) { - t.Error("generated hook should have been deleted entirely") - } - }) - - t.Run("mixed hook preserves non-roborev content", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - mixed := "#!/bin/sh\necho 'custom logic'\n" + generatePostRewriteHookContent() - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if strings.Contains(strings.ToLower(contentStr), "roborev") { - t.Errorf("roborev content should be removed, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "echo 'custom logic'") { - t.Error("custom content should be preserved") - } - // Verify no orphaned fi - if strings.Contains(contentStr, "\nfi\n") || strings.HasSuffix(strings.TrimSpace(contentStr), "fi") { - t.Errorf("should not have orphaned fi, got:\n%s", contentStr) - } - }) - - t.Run("custom line mentioning roborev before snippet", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // Custom line mentions roborev but isn't the generated marker - mixed := "#!/bin/sh\necho 'notify roborev team'\n" + - generatePostRewriteHookContent() - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - // The custom line should be preserved - if !strings.Contains(contentStr, "notify roborev team") { - t.Error("custom line mentioning roborev should be preserved") - } - // The generated snippet should be removed - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("custom if-block after snippet is preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // Snippet followed by user's own if-block - mixed := "#!/bin/sh\n" + - generatePostRewriteHookContent() + - "if [ -f .notify ]; then\n" + - " echo 'send notification'\n" + - "fi\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - // User's if-block must survive - if !strings.Contains(contentStr, "send notification") { - t.Errorf("user if-block should be preserved, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "if [ -f .notify ]") { - t.Errorf("user if-statement should be preserved, got:\n%s", contentStr) - } - // No orphaned fi - lines := strings.Split(contentStr, "\n") - ifCount, fiCount := 0, 0 - for _, line := range lines { - trimmed := strings.TrimSpace(line) - if strings.HasPrefix(trimmed, "if ") { - ifCount++ - } - if trimmed == "fi" { - fiCount++ - } - } - if ifCount != fiCount { - t.Errorf("if/fi mismatch: %d if vs %d fi in:\n%s", - ifCount, fiCount, contentStr) - } - // Generated snippet should be gone - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("custom comment starting with # roborev is preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // User comment starts with "# roborev" but isn't a generated marker - mixed := "#!/bin/sh\n# roborev notes: this hook was customized\necho 'custom'\n" + - generatePostRewriteHookContent() - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if !strings.Contains(contentStr, "roborev notes") { - t.Error("custom comment should be preserved") - } - if !strings.Contains(contentStr, "echo 'custom'") { - t.Error("custom echo should be preserved") - } - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("post-snippet user line mentioning roborev is preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // Snippet followed by user logic that mentions roborev - mixed := "#!/bin/sh\n" + - generatePostRewriteHookContent() + - "echo 'roborev hook finished'\n" + - "logger 'roborev done'\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if !strings.Contains(contentStr, "roborev hook finished") { - t.Errorf("user line mentioning roborev should be preserved, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "roborev done") { - t.Errorf("user logger line should be preserved, got:\n%s", contentStr) - } - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("user $ROBOREV line after snippet is preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // User has their own "$ROBOREV" line (e.g. version check) after snippet - mixed := "#!/bin/sh\n" + - generatePostRewriteHookContent() + - "\"$ROBOREV\" --version\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if !strings.Contains(contentStr, "\"$ROBOREV\" --version") { - t.Errorf("user $ROBOREV line should be preserved, got:\n%s", contentStr) - } - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("user $ROBOREV enqueue/remap lines after snippet are preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // User added their own enqueue/remap invocations with different flags - mixed := "#!/bin/sh\n" + - generatePostRewriteHookContent() + - "\"$ROBOREV\" enqueue --dry-run\n" + - "\"$ROBOREV\" remap --verbose\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if !strings.Contains(contentStr, "enqueue --dry-run") { - t.Errorf("user enqueue line should be preserved, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "remap --verbose") { - t.Errorf("user remap line should be preserved, got:\n%s", contentStr) - } - if strings.Contains(contentStr, "remap --quiet") { - t.Errorf("generated snippet should be removed, got:\n%s", contentStr) - } - }) - - t.Run("user --quietly/--quiet-mode lines are preserved", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - // User lines that start with --quiet but aren't the exact generated form - mixed := "#!/bin/sh\n" + - generatePostRewriteHookContent() + - "\"$ROBOREV\" enqueue --quietly\n" + - "\"$ROBOREV\" remap --quiet-mode\n" - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if !strings.Contains(contentStr, "enqueue --quietly") { - t.Errorf("user --quietly line should be preserved, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "remap --quiet-mode") { - t.Errorf("user --quiet-mode line should be preserved, got:\n%s", contentStr) - } - }) - - t.Run("v0 hook (plain roborev invocation) is removed", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - v0Hook := "#!/bin/sh\n" + - "# RoboRev post-commit hook - auto-reviews every commit\n" + - "roborev enqueue --sha HEAD 2>/dev/null &\n" - if err := os.WriteFile(hookPath, []byte(v0Hook), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - if _, err := os.Stat(hookPath); !os.IsNotExist(err) { - content, _ := os.ReadFile(hookPath) - t.Errorf("v0 hook should be deleted entirely, got:\n%s", content) - } - }) - - t.Run("v0.5 hook (early variable format) is removed", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - v05Hook := "#!/bin/sh\n" + - "# RoboRev post-commit hook - auto-reviews every commit\n" + - "ROBOREV=\"/usr/local/bin/roborev\"\n" + - "if [ ! -x \"$ROBOREV\" ]; then\n" + - " ROBOREV=$(command -v roborev) || exit 0\n" + - "fi\n" + - "\"$ROBOREV\" enqueue --quiet &\n" - if err := os.WriteFile(hookPath, []byte(v05Hook), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - if _, err := os.Stat(hookPath); !os.IsNotExist(err) { - content, _ := os.ReadFile(hookPath) - t.Errorf("v0.5 hook should be deleted entirely, got:\n%s", content) - } - }) - - t.Run("v1 hook (PATH-first format) is removed", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - v1Hook := "#!/bin/sh\n" + - "# RoboRev post-commit hook - auto-reviews every commit\n" + - "ROBOREV=$(command -v roborev 2>/dev/null)\n" + - "if [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ]; then\n" + - " ROBOREV=\"/usr/local/bin/roborev\"\n" + - " [ ! -x \"$ROBOREV\" ] && exit 0\n" + - "fi\n" + - "\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" - if err := os.WriteFile(hookPath, []byte(v1Hook), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - if _, err := os.Stat(hookPath); !os.IsNotExist(err) { - content, _ := os.ReadFile(hookPath) - t.Errorf("v1 hook should be deleted entirely, got:\n%s", content) - } - }) - - t.Run("v1 mixed hook removes only roborev block", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-commit") - v1Block := "# RoboRev post-commit hook - auto-reviews every commit\n" + - "ROBOREV=$(command -v roborev 2>/dev/null)\n" + - "if [ -z \"$ROBOREV\" ] || [ ! -x \"$ROBOREV\" ]; then\n" + - " ROBOREV=\"/usr/local/bin/roborev\"\n" + - " [ ! -x \"$ROBOREV\" ] && exit 0\n" + - "fi\n" + - "\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" - mixed := "#!/bin/sh\necho 'custom'\n" + v1Block - if err := os.WriteFile(hookPath, []byte(mixed), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - contentStr := string(content) - if strings.Contains(strings.ToLower(contentStr), "roborev") { - t.Errorf("roborev content should be removed, got:\n%s", contentStr) - } - if !strings.Contains(contentStr, "echo 'custom'") { - t.Error("custom content should be preserved") - } - }) - - t.Run("no-op if hook has no roborev content", func(t *testing.T) { - repo := testutil.NewTestRepo(t) - if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { - t.Fatal(err) - } - hookPath := filepath.Join(repo.HooksDir, "post-rewrite") - hookContent := "#!/bin/sh\necho 'unrelated'\n" - if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil { - t.Fatal(err) - } - - if err := removeRoborevFromHook(hookPath); err != nil { - t.Fatalf("removeRoborevFromHook: %v", err) - } - - content, err := os.ReadFile(hookPath) - if err != nil { - t.Fatalf("hook should still exist: %v", err) - } - if string(content) != hookContent { - t.Errorf("hook should be unchanged, got:\n%s", content) - } - }) -} - func TestInitInstallsPostRewriteHookOnUpgrade(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("test uses shell script stub, skipping on Windows") } root := initNoDaemonSetup(t) - // Pre-install a current post-commit hook so init takes the - // "already installed" goto path + // Pre-install a current post-commit hook so init sees it hooksDir := filepath.Join(root, ".git", "hooks") if err := os.MkdirAll(hooksDir, 0755); err != nil { t.Fatal(err) } - hookContent := generateHookContent() + hookContent := githook.GeneratePostCommit() if err := os.WriteFile(filepath.Join(hooksDir, "post-commit"), []byte(hookContent), 0755); err != nil { t.Fatal(err) } @@ -1340,29 +408,12 @@ func TestInitInstallsPostRewriteHookOnUpgrade(t *testing.T) { _ = cmd.Execute() }) - // Verify post-rewrite hook was installed despite post-commit - // taking the "already installed" path prHookPath := filepath.Join(hooksDir, "post-rewrite") content, err := os.ReadFile(prHookPath) if err != nil { - t.Fatalf("post-rewrite hook should be installed even when "+ - "post-commit is already current: %v", err) + t.Fatalf("post-rewrite hook should be installed: %v", err) } if !strings.Contains(string(content), "remap --quiet") { t.Error("post-rewrite hook should contain 'remap --quiet'") } } - -func TestGeneratePostRewriteHookContent(t *testing.T) { - content := generatePostRewriteHookContent() - - if !strings.HasPrefix(content, "#!/bin/sh\n") { - t.Error("hook should start with #!/bin/sh") - } - if !strings.Contains(content, postRewriteHookVersionMarker) { - t.Error("hook should contain version marker") - } - if !strings.Contains(content, "remap --quiet") { - t.Error("hook should call remap --quiet") - } -} diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 198f0d26..d4a8dbe2 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -28,6 +28,7 @@ import ( "github.com/roborev-dev/roborev/internal/config" "github.com/roborev-dev/roborev/internal/daemon" "github.com/roborev-dev/roborev/internal/git" + "github.com/roborev-dev/roborev/internal/githook" "github.com/roborev-dev/roborev/internal/prompt" "github.com/roborev-dev/roborev/internal/skills" "github.com/roborev-dev/roborev/internal/storage" @@ -451,64 +452,18 @@ func initCmd() *cobra.Command { } } - // 4. Install post-commit hook + // 4. Install hooks (post-commit + post-rewrite) hooksDir, err := git.GetHooksPath(root) if err != nil { return fmt.Errorf("get hooks path: %w", err) } - hookPath := filepath.Join(hooksDir, "post-commit") - hookContent := generateHookContent() - - // Ensure hooks directory exists if err := os.MkdirAll(hooksDir, 0755); err != nil { return fmt.Errorf("create hooks directory: %w", err) } - - // Check for existing hook - if existing, err := os.ReadFile(hookPath); err == nil { - existingStr := string(existing) - if !strings.Contains(strings.ToLower(existingStr), "roborev") { - if !isShellHook(existingStr) { - fmt.Printf(" Warning: %s uses a non-shell interpreter, skipping roborev hook\n", hookPath) - goto startDaemon - } - // Append to existing hook - hookContent = existingStr + "\n" + hookContent - } else if strings.Contains(existingStr, hookVersionMarker) { - fmt.Println(" Hook already installed") - goto startDaemon - } else { - // Upgrade: try patching in place first (preserves hook structure) - // v1 → v2: remove trailing & from enqueue line, add version marker - upgraded := existingStr - upgraded = strings.Replace(upgraded, "2>/dev/null &", "2>/dev/null", 1) - upgraded = strings.Replace(upgraded, "post-commit hook -", "post-commit hook v2 -", 1) - if !strings.Contains(upgraded, hookVersionMarker) { - // Comment was edited — just append a marker so we don't re-upgrade - if !strings.HasSuffix(upgraded, "\n") { - upgraded += "\n" - } - upgraded += "# " + hookVersionMarker + "\n" - } - if err := os.WriteFile(hookPath, []byte(upgraded), 0755); err != nil { - return fmt.Errorf("upgrade hook: %w", err) - } - fmt.Println(" Upgraded post-commit hook") - goto startDaemon - } + if err := githook.InstallAll(hooksDir, false); err != nil { + return fmt.Errorf("install hooks: %w", err) } - if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil { - return fmt.Errorf("install hook: %w", err) - } - fmt.Printf(" Installed post-commit hook\n") - - startDaemon: - // 4b. Install post-rewrite hook (for rebase review preservation) - // Runs on all paths (fresh install, upgrade, already-installed) - // so existing users get the new hook on next `roborev init`. - installPostRewriteHook(hooksDir) - // 5. Start daemon (or just register if --no-daemon) var initIncomplete bool if noDaemon { @@ -851,6 +806,12 @@ Examples: return nil // Intentional skip, exit 0 } + // Auto-install/upgrade hooks when running from CLI + // (not when called from a hook via --quiet) + if !quiet { + autoInstallHooks(root) + } + // Ensure daemon is running (skip for --local mode) if !local { if err := ensureDaemon(); err != nil { @@ -1577,12 +1538,12 @@ func statusCmd() *cobra.Command { // Check for outdated hooks in current repo if root, err := git.GetRepoRoot("."); err == nil { - if hookNeedsUpgrade(root, "post-commit", hookVersionMarker) { + if githook.NeedsUpgrade(root, "post-commit", githook.PostCommitVersionMarker) { fmt.Println() fmt.Println("Warning: post-commit hook is outdated -- run 'roborev init' to upgrade") } - if hookNeedsUpgrade(root, "post-rewrite", postRewriteHookVersionMarker) || - hookMissing(root, "post-rewrite") { + if githook.NeedsUpgrade(root, "post-rewrite", githook.PostRewriteVersionMarker) || + githook.Missing(root, "post-rewrite") { fmt.Println() fmt.Println("Warning: post-rewrite hook is missing or outdated -- run 'roborev init' to install") } @@ -2355,22 +2316,7 @@ func installHookCmd() *cobra.Command { return fmt.Errorf("create hooks directory: %w", err) } - if err := installOrUpgradeHook( - hooksDir, "post-commit", - hookVersionMarker, generateHookContent, force, - ); err != nil { - return err - } - - if err := installOrUpgradeHook( - hooksDir, "post-rewrite", - postRewriteHookVersionMarker, - generatePostRewriteHookContent, force, - ); err != nil { - return err - } - - return nil + return githook.InstallAll(hooksDir, force) }, } @@ -2379,73 +2325,6 @@ func installHookCmd() *cobra.Command { return cmd } -// installOrUpgradeHook handles the append/upgrade/skip logic for a -// single hook file, following the design doc's per-path behavior: -// - No existing hook: write fresh -// - Existing without roborev: append -// - Existing with current version: skip -// - Existing with old version: upgrade (remove old, append new) -// - --force: overwrite unconditionally -// -// hookReadFile is used to re-read the hook file after cleanup during -// upgrade. Replaceable in tests to simulate read failures. -var hookReadFile = os.ReadFile - -func installOrUpgradeHook( - hooksDir, hookName, versionMarker string, - generate func() string, force bool, -) error { - hookPath := filepath.Join(hooksDir, hookName) - hookContent := generate() - - existing, err := os.ReadFile(hookPath) - if err == nil && !force { - existingStr := string(existing) - if !strings.Contains(strings.ToLower(existingStr), "roborev") { - if !isShellHook(existingStr) { - return fmt.Errorf( - "%s hook uses a non-shell interpreter; "+ - "add the roborev snippet manually or use --force to overwrite", - hookName) - } - // No roborev content — append - hookContent = existingStr + "\n" + hookContent - } else if strings.Contains(existingStr, versionMarker) { - fmt.Printf("%s hook already installed (current)\n", hookName) - return nil - } else { - // Upgrade: remove old snippet, append new one - if !isShellHook(existingStr) { - return fmt.Errorf( - "%s hook uses a non-shell interpreter; "+ - "add the roborev snippet manually "+ - "or use --force to overwrite", - hookName) - } - if rmErr := removeRoborevFromHook(hookPath); rmErr != nil { - return fmt.Errorf("upgrade %s: %w", hookName, rmErr) - } - updated, readErr := hookReadFile(hookPath) - if readErr != nil && !os.IsNotExist(readErr) { - return fmt.Errorf("re-read %s after cleanup: %w", hookName, readErr) - } - if readErr == nil { - remaining := string(updated) - if remaining != "" && !strings.HasSuffix(remaining, "\n") { - remaining += "\n" - } - hookContent = remaining + hookContent - } - } - } - - if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil { - return fmt.Errorf("write %s hook: %w", hookName, err) - } - fmt.Printf("Installed %s hook at %s\n", hookName, hookPath) - return nil -} - func uninstallHookCmd() *cobra.Command { return &cobra.Command{ Use: "uninstall-hook", @@ -2464,7 +2343,7 @@ func uninstallHookCmd() *cobra.Command { for _, hookName := range []string{ "post-commit", "post-rewrite", } { - if err := removeRoborevFromHook( + if err := githook.Uninstall( filepath.Join(hooksDir, hookName), ); err != nil { return err @@ -2476,169 +2355,6 @@ func uninstallHookCmd() *cobra.Command { } } -// removeRoborevFromHook removes the roborev block from a hook file, -// or deletes it entirely if nothing else remains. Uses block-based -// removal: drops all lines from the first roborev comment marker -// through the end of that contiguous block (since roborev appends -// its snippet as a self-contained block at the end). -// isShellHook returns true if the hook content starts with a -// POSIX-compatible shell shebang (sh, bash, zsh, ksh, dash). -// Used to avoid appending shell snippets to non-shell hooks. -func isShellHook(content string) bool { - first, _, _ := strings.Cut(content, "\n") - first = strings.TrimSpace(first) - for _, sh := range []string{"sh", "bash", "zsh", "ksh", "dash"} { - if strings.HasPrefix(first, "#!/bin/"+sh) || - strings.HasPrefix(first, "#!/usr/bin/env "+sh) { - return true - } - } - return false -} - -// isRoborevMarker returns true if the line is a generated roborev hook -// marker comment. Only matches the known generated forms: -// -// # roborev post-commit hook ... -// # roborev post-rewrite hook ... -func isRoborevMarker(line string) bool { - trimmed := strings.TrimSpace(strings.ToLower(line)) - return strings.HasPrefix(trimmed, "# roborev post-commit hook") || - strings.HasPrefix(trimmed, "# roborev post-rewrite hook") -} - -// hasCommandPrefix checks if line starts with prefix and the prefix -// is followed by end-of-string, whitespace, or a shell operator -// (e.g. redirection). This prevents "enqueue --quiet" from matching -// "enqueue --quietly". -func hasCommandPrefix(line, prefix string) bool { - if !strings.HasPrefix(line, prefix) { - return false - } - if len(line) == len(prefix) { - return true - } - next := line[len(prefix)] - return next == ' ' || next == '\t' || next == '>' || - next == '|' || next == '&' || next == ';' -} - -// isRoborevSnippetLine returns true if the line is part of a -// generated roborev hook snippet (current or legacy versions). -func isRoborevSnippetLine(line string) bool { - trimmed := strings.TrimSpace(line) - if trimmed == "" { - return false - } - return strings.HasPrefix(trimmed, "ROBOREV=") || - strings.HasPrefix(trimmed, "ROBOREV=$(") || - hasCommandPrefix(trimmed, "\"$ROBOREV\" enqueue --quiet") || - hasCommandPrefix(trimmed, "\"$ROBOREV\" remap --quiet") || - hasCommandPrefix(trimmed, "roborev enqueue") || - hasCommandPrefix(trimmed, "roborev remap") || - strings.HasPrefix(trimmed, "if [ ! -x \"$ROBOREV\"") || - strings.HasPrefix(trimmed, "if [ -z \"$ROBOREV\"") || - strings.HasPrefix(trimmed, "[ -z \"$ROBOREV\"") || - strings.HasPrefix(trimmed, "[ ! -x \"$ROBOREV\"") -} - -func removeRoborevFromHook(hookPath string) error { - content, err := os.ReadFile(hookPath) - if os.IsNotExist(err) { - return nil - } - if err != nil { - return fmt.Errorf("read %s: %w", filepath.Base(hookPath), err) - } - - hookStr := string(content) - if !strings.Contains(strings.ToLower(hookStr), "roborev") { - return nil - } - - lines := strings.Split(hookStr, "\n") - - // Find the start: anchor on the generated marker comment line - // (e.g. "# roborev post-commit hook v2 - ..."), not just any - // line that mentions roborev. - blockStart := -1 - for i, line := range lines { - if isRoborevMarker(line) { - blockStart = i - break - } - } - if blockStart < 0 { - return nil - } - - // Find the end: scan forward from the marker, consuming only - // lines that are part of the generated snippet. Stop at the - // first line that isn't a snippet line AND isn't "fi" closing - // a snippet's if-block. - blockEnd := blockStart - inIfBlock := false - for i := blockStart + 1; i < len(lines); i++ { - trimmed := strings.TrimSpace(lines[i]) - if trimmed == "" { - // Blank lines between snippet lines are consumed; a - // blank line after the snippet ends the block. - if i+1 < len(lines) && isRoborevSnippetLine(lines[i+1]) { - blockEnd = i - continue - } - break - } - if isRoborevSnippetLine(trimmed) { - blockEnd = i - if strings.HasPrefix(trimmed, "if ") { - inIfBlock = true - } - continue - } - // "fi" only belongs to the block if we saw an "if" inside it - if trimmed == "fi" && inIfBlock { - blockEnd = i - inIfBlock = false - continue - } - break - } - - // Keep everything before and after the block - remaining := make([]string, 0, len(lines)) - remaining = append(remaining, lines[:blockStart]...) - remaining = append(remaining, lines[blockEnd+1:]...) - - // Check if anything meaningful remains - hasContent := false - for _, line := range remaining { - trimmed := strings.TrimSpace(line) - if trimmed != "" && !strings.HasPrefix(trimmed, "#!") { - hasContent = true - break - } - } - - hookName := filepath.Base(hookPath) - if hasContent { - newContent := strings.Join(remaining, "\n") - if !strings.HasSuffix(newContent, "\n") { - newContent += "\n" - } - if err := os.WriteFile(hookPath, []byte(newContent), 0755); err != nil { - return fmt.Errorf("write %s: %w", hookName, err) - } - fmt.Printf("Removed roborev from %s\n", hookName) - } else { - if err := os.Remove(hookPath); err != nil { - return fmt.Errorf("remove %s: %w", hookName, err) - } - fmt.Printf("Removed %s hook\n", hookName) - } - return nil -} - func skillsCmd() *cobra.Command { cmd := &cobra.Command{ Use: "skills", @@ -3404,151 +3120,22 @@ func resolveReasoningWithFast(reasoning string, fast bool, reasoningExplicitlySe return reasoning } -// hookVersionMarker is the string that identifies the current hook version. -// Bump this when the hook template changes to trigger upgrade warnings. -const hookVersionMarker = "post-commit hook v2" - -const postRewriteHookVersionMarker = "post-rewrite hook v1" - -// hookNeedsUpgrade checks whether a repo's named hook contains roborev -// but is outdated (missing the given version marker). -func hookNeedsUpgrade(repoPath, hookName, versionMarker string) bool { - hooksDir, err := git.GetHooksPath(repoPath) - if err != nil { - return false - } - content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) - if err != nil { - return false - } - s := string(content) - return strings.Contains(strings.ToLower(s), "roborev") && - !strings.Contains(s, versionMarker) -} - -// hookMissing checks whether a repo has roborev installed (post-commit -// hook present) but is missing the named hook entirely. -func hookMissing(repoPath, hookName string) bool { +// autoInstallHooks installs or upgrades hooks when running +// from the CLI (not from hooks themselves). +func autoInstallHooks(repoPath string) { hooksDir, err := git.GetHooksPath(repoPath) if err != nil { - return false - } - // Only warn if roborev is installed (post-commit hook exists) - pcContent, err := os.ReadFile(filepath.Join(hooksDir, "post-commit")) - if err != nil { - return false - } - if !strings.Contains(strings.ToLower(string(pcContent)), "roborev") { - return false - } - // Check if the target hook is missing or has no roborev content - content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) - if err != nil { - return true // hook file doesn't exist - } - return !strings.Contains(strings.ToLower(string(content)), "roborev") -} - -func generateHookContent() string { - // Get path to the currently running binary (not just first in PATH) - roborevPath, err := os.Executable() - if err == nil { - // Resolve symlinks to get the real path - if resolved, err := filepath.EvalSymlinks(roborevPath); err == nil { - roborevPath = resolved - } - } else { - // Fallback to PATH lookup if os.Executable fails (shouldn't happen) - roborevPath, _ = exec.LookPath("roborev") - if roborevPath == "" { - roborevPath = "roborev" - } - } - - // Prefer baked path (security), fall back to PATH only if baked is missing - return fmt.Sprintf(`#!/bin/sh -# roborev post-commit hook v2 - auto-reviews every commit -ROBOREV=%q -if [ ! -x "$ROBOREV" ]; then - ROBOREV=$(command -v roborev 2>/dev/null) - [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && exit 0 -fi -"$ROBOREV" enqueue --quiet 2>/dev/null -`, roborevPath) -} - -func installPostRewriteHook(hooksDir string) { - hookPath := filepath.Join(hooksDir, "post-rewrite") - hookContent := generatePostRewriteHookContent() - - if existing, err := os.ReadFile(hookPath); err == nil { - existingStr := string(existing) - if !strings.Contains(strings.ToLower(existingStr), "roborev") { - if !isShellHook(existingStr) { - fmt.Printf(" Warning: %s uses a non-shell interpreter, skipping\n", hookPath) - return - } - // No roborev content — append to existing hook - hookContent = existingStr + "\n" + hookContent - } else if strings.Contains(existingStr, postRewriteHookVersionMarker) { - fmt.Println(" Post-rewrite hook already installed") - return - } else { - // Upgrade: remove old roborev snippet, append new one. - // This preserves user content around the snippet. - if !isShellHook(existingStr) { - fmt.Printf(" Warning: %s uses a non-shell interpreter, skipping\n", hookPath) - return - } - if rmErr := removeRoborevFromHook(hookPath); rmErr != nil { - fmt.Printf(" Warning: %v\n", rmErr) - return - } - updated, readErr := os.ReadFile(hookPath) - if readErr != nil && !os.IsNotExist(readErr) { - fmt.Printf(" Warning: re-read %s after cleanup: %v\n", - hookPath, readErr) - return - } - if readErr == nil { - remaining := string(updated) - if remaining != "" && !strings.HasSuffix(remaining, "\n") { - remaining += "\n" - } - hookContent = remaining + hookContent - } - // If the file was deleted (snippet-only), hookContent - // is already the fresh generated content. - } - } - - if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil { - fmt.Printf(" Warning: could not install post-rewrite hook: %v\n", err) return } - fmt.Printf(" Installed post-rewrite hook\n") -} - -func generatePostRewriteHookContent() string { - roborevPath, err := os.Executable() - if err == nil { - if resolved, err := filepath.EvalSymlinks(roborevPath); err == nil { - roborevPath = resolved - } - } else { - roborevPath, _ = exec.LookPath("roborev") - if roborevPath == "" { - roborevPath = "roborev" + for _, name := range []string{"post-commit", "post-rewrite"} { + marker := githook.VersionMarker(name) + if githook.NeedsUpgrade(repoPath, name, marker) || + githook.Missing(repoPath, name) { + if err := githook.Install(hooksDir, name, false); err != nil { + fmt.Fprintf(os.Stderr, + "Warning: auto-install %s hook: %v\n", + name, err) + } } } - - return fmt.Sprintf(`#!/bin/sh -# roborev post-rewrite hook v1 - remaps reviews after rebase/amend -ROBOREV=%q -if [ ! -x "$ROBOREV" ]; then - ROBOREV=$(command -v roborev 2>/dev/null) - [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && exit 0 -fi -"$ROBOREV" remap --quiet 2>/dev/null -`, roborevPath) } diff --git a/internal/daemon/server.go b/internal/daemon/server.go index bfc3c271..bd3c91f4 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -8,8 +8,6 @@ import ( "fmt" "log" "net/http" - "os" - "path/filepath" "strings" "sync" "time" @@ -17,6 +15,7 @@ import ( "github.com/roborev-dev/roborev/internal/agent" "github.com/roborev-dev/roborev/internal/config" "github.com/roborev-dev/roborev/internal/git" + "github.com/roborev-dev/roborev/internal/githook" "github.com/roborev-dev/roborev/internal/storage" "github.com/roborev-dev/roborev/internal/version" ) @@ -143,11 +142,11 @@ func (s *Server) Start(ctx context.Context) error { if s.ciPoller == nil { if repos, err := s.db.ListRepos(); err == nil { for _, repo := range repos { - if hookNeedsUpgrade(repo.RootPath, "post-commit", hookVersionMarker) { + if githook.NeedsUpgrade(repo.RootPath, "post-commit", githook.PostCommitVersionMarker) { log.Printf("Warning: outdated post-commit hook in %s -- run 'roborev init' to upgrade", repo.RootPath) } - if hookNeedsUpgrade(repo.RootPath, "post-rewrite", postRewriteHookVersionMarker) || - hookMissing(repo.RootPath, "post-rewrite") { + if githook.NeedsUpgrade(repo.RootPath, "post-rewrite", githook.PostRewriteVersionMarker) || + githook.Missing(repo.RootPath, "post-rewrite") { log.Printf("Warning: missing or outdated post-rewrite hook in %s -- run 'roborev init' to install", repo.RootPath) } } @@ -164,47 +163,6 @@ func (s *Server) Start(ctx context.Context) error { return nil } -// hookVersionMarker identifies the current hook version. -const hookVersionMarker = "post-commit hook v2" -const postRewriteHookVersionMarker = "post-rewrite hook v1" - -// hookNeedsUpgrade checks whether a repo's named hook contains roborev -// but is outdated (missing the given version marker). -func hookNeedsUpgrade(repoPath, hookName, versionMarker string) bool { - hooksDir, err := git.GetHooksPath(repoPath) - if err != nil { - return false - } - content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) - if err != nil { - return false - } - s := string(content) - return strings.Contains(strings.ToLower(s), "roborev") && - !strings.Contains(s, versionMarker) -} - -// hookMissing checks whether a repo has roborev installed (post-commit -// hook present) but is missing the named hook entirely. -func hookMissing(repoPath, hookName string) bool { - hooksDir, err := git.GetHooksPath(repoPath) - if err != nil { - return false - } - pcContent, err := os.ReadFile(filepath.Join(hooksDir, "post-commit")) - if err != nil { - return false - } - if !strings.Contains(strings.ToLower(string(pcContent)), "roborev") { - return false - } - content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) - if err != nil { - return true - } - return !strings.Contains(strings.ToLower(string(content)), "roborev") -} - // Stop gracefully shuts down the server func (s *Server) Stop() error { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) diff --git a/internal/githook/githook.go b/internal/githook/githook.go new file mode 100644 index 00000000..2a181638 --- /dev/null +++ b/internal/githook/githook.go @@ -0,0 +1,459 @@ +// Package githook manages roborev's git hook installation, +// upgrade, and removal. It supports both standalone hooks +// (fresh installs) and embedded snippets that coexist with +// existing hook scripts. +package githook + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + + "github.com/roborev-dev/roborev/internal/git" +) + +// Version markers identify the current hook template version. +// Bump these when the hook template changes to trigger +// upgrade warnings and auto-upgrades. +const PostCommitVersionMarker = "post-commit hook v3" +const PostRewriteVersionMarker = "post-rewrite hook v2" + +// VersionMarker returns the current version marker for a hook. +func VersionMarker(hookName string) string { + switch hookName { + case "post-commit": + return PostCommitVersionMarker + case "post-rewrite": + return PostRewriteVersionMarker + default: + return "" + } +} + +// ReadFile is the function used to re-read a hook file after +// cleanup during upgrade. Replaceable in tests to simulate +// read failures. +var ReadFile = os.ReadFile + +// NeedsUpgrade checks whether a repo's named hook contains +// roborev but is outdated (missing the given version marker). +func NeedsUpgrade(repoPath, hookName, versionMarker string) bool { + hooksDir, err := git.GetHooksPath(repoPath) + if err != nil { + return false + } + content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) + if err != nil { + return false + } + s := string(content) + return strings.Contains(strings.ToLower(s), "roborev") && + !strings.Contains(s, versionMarker) +} + +// Missing checks whether a repo has roborev installed +// (post-commit hook present) but is missing the named hook. +func Missing(repoPath, hookName string) bool { + hooksDir, err := git.GetHooksPath(repoPath) + if err != nil { + return false + } + pcContent, err := os.ReadFile( + filepath.Join(hooksDir, "post-commit"), + ) + if err != nil { + return false + } + if !strings.Contains( + strings.ToLower(string(pcContent)), "roborev", + ) { + return false + } + content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) + if err != nil { + return true + } + return !strings.Contains( + strings.ToLower(string(content)), "roborev", + ) +} + +// resolveRoborevPath returns the absolute path to the running +// roborev binary, falling back to a PATH lookup. +func resolveRoborevPath() string { + roborevPath, err := os.Executable() + if err == nil { + if resolved, err := filepath.EvalSymlinks(roborevPath); err == nil { + roborevPath = resolved + } + return roborevPath + } + roborevPath, _ = exec.LookPath("roborev") + if roborevPath == "" { + roborevPath = "roborev" + } + return roborevPath +} + +// GeneratePostCommit returns a standalone post-commit hook +// (with shebang, suitable for fresh installs). +func GeneratePostCommit() string { + return fmt.Sprintf(`#!/bin/sh +# roborev %s - auto-reviews every commit +ROBOREV=%q +if [ ! -x "$ROBOREV" ]; then + ROBOREV=$(command -v roborev 2>/dev/null) + [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && exit 0 +fi +"$ROBOREV" enqueue --quiet 2>/dev/null +`, PostCommitVersionMarker, resolveRoborevPath()) +} + +// GeneratePostRewrite returns a standalone post-rewrite hook +// (with shebang, suitable for fresh installs). +func GeneratePostRewrite() string { + return fmt.Sprintf(`#!/bin/sh +# roborev %s - remaps reviews after rebase/amend +ROBOREV=%q +if [ ! -x "$ROBOREV" ]; then + ROBOREV=$(command -v roborev 2>/dev/null) + [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && exit 0 +fi +"$ROBOREV" remap --quiet 2>/dev/null +`, PostRewriteVersionMarker, resolveRoborevPath()) +} + +// generateEmbeddablePostCommit returns a function-wrapped +// snippet without shebang, for embedding in existing hooks. +// Uses return instead of exit so it doesn't terminate the +// parent script. +func generateEmbeddablePostCommit() string { + return fmt.Sprintf(`# roborev %s - auto-reviews every commit +_roborev_hook() { +ROBOREV=%q +if [ ! -x "$ROBOREV" ]; then + ROBOREV=$(command -v roborev 2>/dev/null) + [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && return 0 +fi +"$ROBOREV" enqueue --quiet 2>/dev/null +} +_roborev_hook +`, PostCommitVersionMarker, resolveRoborevPath()) +} + +// generateEmbeddablePostRewrite returns a function-wrapped +// snippet without shebang, for embedding in existing hooks. +func generateEmbeddablePostRewrite() string { + return fmt.Sprintf(`# roborev %s - remaps reviews after rebase/amend +_roborev_remap() { +ROBOREV=%q +if [ ! -x "$ROBOREV" ]; then + ROBOREV=$(command -v roborev 2>/dev/null) + [ -z "$ROBOREV" ] || [ ! -x "$ROBOREV" ] && return 0 +fi +"$ROBOREV" remap --quiet 2>/dev/null +} +_roborev_remap +`, PostRewriteVersionMarker, resolveRoborevPath()) +} + +// generateContent returns the standalone hook content for the +// given hook name. +func generateContent(hookName string) string { + switch hookName { + case "post-commit": + return GeneratePostCommit() + case "post-rewrite": + return GeneratePostRewrite() + default: + return "" + } +} + +// generateEmbeddable returns the embeddable snippet for the +// given hook name. +func generateEmbeddable(hookName string) string { + switch hookName { + case "post-commit": + return generateEmbeddablePostCommit() + case "post-rewrite": + return generateEmbeddablePostRewrite() + default: + return "" + } +} + +// embedSnippet inserts snippet after the shebang line of +// existing, so it runs before any possible exit in the +// original script. If there is no shebang, the snippet is +// prepended. +func embedSnippet(existing, snippet string) string { + lines := strings.SplitAfter(existing, "\n") + if len(lines) > 0 && + strings.HasPrefix(strings.TrimSpace(lines[0]), "#!") { + return lines[0] + snippet + strings.Join(lines[1:], "") + } + return snippet + existing +} + +// Install installs or upgrades a single hook. It handles: +// - No existing hook: write standalone content +// - Existing without roborev: embed snippet after shebang +// - Existing with current version: skip (no-op) +// - Existing with old version: remove old, embed new +// - force=true: overwrite unconditionally +func Install(hooksDir, hookName string, force bool) error { + hookPath := filepath.Join(hooksDir, hookName) + versionMarker := VersionMarker(hookName) + hookContent := generateContent(hookName) + + existing, err := os.ReadFile(hookPath) + if err == nil && !force { + existingStr := string(existing) + if !strings.Contains( + strings.ToLower(existingStr), "roborev", + ) { + if !isShellHook(existingStr) { + return fmt.Errorf( + "%s hook uses a non-shell interpreter; "+ + "add the roborev snippet manually "+ + "or use --force to overwrite", + hookName) + } + hookContent = embedSnippet( + existingStr, + generateEmbeddable(hookName), + ) + } else if strings.Contains(existingStr, versionMarker) { + fmt.Printf( + "%s hook already installed (current)\n", + hookName, + ) + return nil + } else { + // Upgrade: remove old snippet, embed new one + if !isShellHook(existingStr) { + return fmt.Errorf( + "%s hook uses a non-shell interpreter; "+ + "add the roborev snippet manually "+ + "or use --force to overwrite", + hookName) + } + if rmErr := Uninstall(hookPath); rmErr != nil { + return fmt.Errorf( + "upgrade %s: %w", hookName, rmErr, + ) + } + updated, readErr := ReadFile(hookPath) + if readErr != nil && !os.IsNotExist(readErr) { + return fmt.Errorf( + "re-read %s after cleanup: %w", + hookName, readErr, + ) + } + if readErr == nil { + remaining := string(updated) + hookContent = embedSnippet( + remaining, + generateEmbeddable(hookName), + ) + } + // If file was deleted (snippet-only), hookContent + // is the fresh standalone content. + } + } + + if err := os.WriteFile( + hookPath, []byte(hookContent), 0755, + ); err != nil { + return fmt.Errorf("write %s hook: %w", hookName, err) + } + fmt.Printf("Installed %s hook at %s\n", hookName, hookPath) + return nil +} + +// InstallAll installs both post-commit and post-rewrite hooks. +func InstallAll(hooksDir string, force bool) error { + for _, name := range []string{"post-commit", "post-rewrite"} { + if err := Install(hooksDir, name, force); err != nil { + return err + } + } + return nil +} + +// Uninstall removes the roborev block from a hook file, or +// deletes it entirely if nothing else remains. +func Uninstall(hookPath string) error { + content, err := os.ReadFile(hookPath) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return fmt.Errorf( + "read %s: %w", filepath.Base(hookPath), err, + ) + } + + hookStr := string(content) + if !strings.Contains(strings.ToLower(hookStr), "roborev") { + return nil + } + + lines := strings.Split(hookStr, "\n") + + blockStart := -1 + for i, line := range lines { + if isRoborevMarker(line) { + blockStart = i + break + } + } + if blockStart < 0 { + return nil + } + + blockEnd := blockStart + inIfBlock := false + inFuncBlock := false + for i := blockStart + 1; i < len(lines); i++ { + trimmed := strings.TrimSpace(lines[i]) + if trimmed == "" { + if i+1 < len(lines) && + isRoborevSnippetLine(lines[i+1]) { + blockEnd = i + continue + } + break + } + if isRoborevSnippetLine(trimmed) { + blockEnd = i + if strings.HasPrefix(trimmed, "if ") { + inIfBlock = true + } + if strings.HasSuffix(trimmed, "() {") { + inFuncBlock = true + } + continue + } + if trimmed == "fi" && inIfBlock { + blockEnd = i + inIfBlock = false + continue + } + if trimmed == "}" && inFuncBlock { + blockEnd = i + inFuncBlock = false + continue + } + break + } + + remaining := make([]string, 0, len(lines)) + remaining = append(remaining, lines[:blockStart]...) + remaining = append(remaining, lines[blockEnd+1:]...) + + hasContent := false + for _, line := range remaining { + trimmed := strings.TrimSpace(line) + if trimmed != "" && !strings.HasPrefix(trimmed, "#!") { + hasContent = true + break + } + } + + hookName := filepath.Base(hookPath) + if hasContent { + newContent := strings.Join(remaining, "\n") + if !strings.HasSuffix(newContent, "\n") { + newContent += "\n" + } + if err := os.WriteFile( + hookPath, []byte(newContent), 0755, + ); err != nil { + return fmt.Errorf("write %s: %w", hookName, err) + } + fmt.Printf("Removed roborev from %s\n", hookName) + } else { + if err := os.Remove(hookPath); err != nil { + return fmt.Errorf("remove %s: %w", hookName, err) + } + fmt.Printf("Removed %s hook\n", hookName) + } + return nil +} + +// isShellHook returns true if the hook content starts with a +// POSIX-compatible shell shebang. +func isShellHook(content string) bool { + first, _, _ := strings.Cut(content, "\n") + first = strings.TrimSpace(first) + for _, sh := range []string{ + "sh", "bash", "zsh", "ksh", "dash", + } { + if strings.HasPrefix(first, "#!/bin/"+sh) || + strings.HasPrefix(first, "#!/usr/bin/env "+sh) { + return true + } + } + return false +} + +// isRoborevMarker returns true if the line is a generated +// roborev hook marker comment. +func isRoborevMarker(line string) bool { + trimmed := strings.TrimSpace(strings.ToLower(line)) + return strings.HasPrefix( + trimmed, "# roborev post-commit hook", + ) || strings.HasPrefix( + trimmed, "# roborev post-rewrite hook", + ) +} + +// hasCommandPrefix checks if line starts with prefix and the +// prefix is followed by end-of-string, whitespace, or a shell +// operator. Prevents "enqueue --quiet" from matching +// "enqueue --quietly". +func hasCommandPrefix(line, prefix string) bool { + if !strings.HasPrefix(line, prefix) { + return false + } + if len(line) == len(prefix) { + return true + } + next := line[len(prefix)] + return next == ' ' || next == '\t' || next == '>' || + next == '|' || next == '&' || next == ';' +} + +// isRoborevSnippetLine returns true if the line is part of a +// generated roborev hook snippet (current or legacy versions). +func isRoborevSnippetLine(line string) bool { + trimmed := strings.TrimSpace(line) + if trimmed == "" { + return false + } + return strings.HasPrefix(trimmed, "ROBOREV=") || + strings.HasPrefix(trimmed, "ROBOREV=$(") || + hasCommandPrefix( + trimmed, "\"$ROBOREV\" enqueue --quiet", + ) || + hasCommandPrefix( + trimmed, "\"$ROBOREV\" remap --quiet", + ) || + hasCommandPrefix(trimmed, "roborev enqueue") || + hasCommandPrefix(trimmed, "roborev remap") || + strings.HasPrefix( + trimmed, "if [ ! -x \"$ROBOREV\"", + ) || + strings.HasPrefix( + trimmed, "if [ -z \"$ROBOREV\"", + ) || + strings.HasPrefix(trimmed, "[ -z \"$ROBOREV\"") || + strings.HasPrefix(trimmed, "[ ! -x \"$ROBOREV\"") || + trimmed == "return 0" || + strings.HasPrefix(trimmed, "_roborev_hook") || + strings.HasPrefix(trimmed, "_roborev_remap") +} diff --git a/internal/githook/githook_test.go b/internal/githook/githook_test.go new file mode 100644 index 00000000..24bf9402 --- /dev/null +++ b/internal/githook/githook_test.go @@ -0,0 +1,1258 @@ +package githook + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/roborev-dev/roborev/internal/testutil" +) + +func TestGeneratePostCommit(t *testing.T) { + content := GeneratePostCommit() + lines := strings.Split(content, "\n") + + t.Run("has shebang", func(t *testing.T) { + if !strings.HasPrefix(content, "#!/bin/sh\n") { + t.Error("hook should start with #!/bin/sh") + } + }) + + t.Run("has roborev comment", func(t *testing.T) { + if !strings.Contains(content, "# roborev") { + t.Error("hook should contain roborev comment") + } + }) + + t.Run("baked path comes first", func(t *testing.T) { + bakedIdx := -1 + pathIdx := -1 + for i, line := range lines { + if strings.HasPrefix(line, "ROBOREV=") && + !strings.Contains(line, "command -v") { + bakedIdx = i + } + if strings.Contains(line, "command -v roborev") { + pathIdx = i + } + } + if bakedIdx == -1 { + t.Error("hook should have baked ROBOREV= assignment") + } + if pathIdx == -1 { + t.Error("hook should have PATH fallback") + } + if bakedIdx > pathIdx { + t.Error("baked path should come before PATH lookup") + } + }) + + t.Run("enqueue line without background", func(t *testing.T) { + found := false + for _, line := range lines { + if strings.Contains(line, "enqueue --quiet") && + strings.Contains(line, "2>/dev/null") && + !strings.HasSuffix( + strings.TrimSpace(line), "&", + ) { + found = true + break + } + } + if !found { + t.Error("hook should have enqueue with --quiet " + + "and 2>/dev/null but no trailing &") + } + }) + + t.Run("has version marker", func(t *testing.T) { + if !strings.Contains(content, PostCommitVersionMarker) { + t.Errorf( + "hook should contain %q", + PostCommitVersionMarker, + ) + } + }) + + t.Run("baked path is quoted", func(t *testing.T) { + for _, line := range lines { + if strings.HasPrefix(line, "ROBOREV=") && + !strings.Contains(line, "command -v") { + if !strings.Contains(line, `ROBOREV="`) { + t.Errorf( + "baked path should be quoted: %s", + line, + ) + } + break + } + } + }) + + t.Run("baked path is absolute", func(t *testing.T) { + for _, line := range lines { + if strings.HasPrefix(line, "ROBOREV=") && + !strings.Contains(line, "command -v") { + start := strings.Index(line, `"`) + end := strings.LastIndex(line, `"`) + if start != -1 && end > start { + path := line[start+1 : end] + if !filepath.IsAbs(path) { + t.Errorf( + "baked path should be absolute: %s", + path, + ) + } + } + break + } + } + }) +} + +func TestGeneratePostRewrite(t *testing.T) { + content := GeneratePostRewrite() + + if !strings.HasPrefix(content, "#!/bin/sh\n") { + t.Error("hook should start with #!/bin/sh") + } + if !strings.Contains(content, PostRewriteVersionMarker) { + t.Error("hook should contain version marker") + } + if !strings.Contains(content, "remap --quiet") { + t.Error("hook should call remap --quiet") + } +} + +func TestGenerateEmbeddablePostCommit(t *testing.T) { + content := generateEmbeddablePostCommit() + + if strings.HasPrefix(content, "#!") { + t.Error("embeddable should not have shebang") + } + if !strings.Contains(content, "_roborev_hook() {") { + t.Error("embeddable should use function wrapper") + } + if !strings.Contains(content, "return 0") { + t.Error("embeddable should use return, not exit") + } + if strings.Contains(content, "exit 0") { + t.Error("embeddable must not use exit 0") + } + if !strings.Contains(content, PostCommitVersionMarker) { + t.Error("embeddable should contain version marker") + } + // Ends with function call + lines := strings.Split( + strings.TrimRight(content, "\n"), "\n", + ) + last := strings.TrimSpace(lines[len(lines)-1]) + if last != "_roborev_hook" { + t.Errorf( + "embeddable should end with function call, got: %s", + last, + ) + } +} + +func TestGenerateEmbeddablePostRewrite(t *testing.T) { + content := generateEmbeddablePostRewrite() + + if strings.HasPrefix(content, "#!") { + t.Error("embeddable should not have shebang") + } + if !strings.Contains(content, "_roborev_remap() {") { + t.Error("embeddable should use function wrapper") + } + if !strings.Contains(content, "return 0") { + t.Error("embeddable should use return, not exit") + } + if strings.Contains(content, "exit 0") { + t.Error("embeddable must not use exit 0") + } + if !strings.Contains(content, PostRewriteVersionMarker) { + t.Error("embeddable should contain version marker") + } +} + +func TestEmbedSnippet(t *testing.T) { + t.Run("inserts after shebang", func(t *testing.T) { + existing := "#!/bin/sh\necho 'user code'\nexit 0\n" + snippet := "# roborev snippet\n_roborev_hook\n" + result := embedSnippet(existing, snippet) + if !strings.HasPrefix(result, "#!/bin/sh\n") { + t.Error("should preserve shebang") + } + shebangEnd := strings.Index(result, "\n") + 1 + afterShebang := result[shebangEnd:] + if !strings.HasPrefix(afterShebang, "# roborev snippet") { + t.Errorf( + "snippet should come right after shebang, got:\n%s", + result, + ) + } + if !strings.Contains(result, "echo 'user code'") { + t.Error("user code should be preserved") + } + }) + + t.Run("snippet before exit 0", func(t *testing.T) { + existing := "#!/bin/sh\nexit 0\n" + snippet := "SNIPPET\n" + result := embedSnippet(existing, snippet) + snippetIdx := strings.Index(result, "SNIPPET") + exitIdx := strings.Index(result, "exit 0") + if snippetIdx > exitIdx { + t.Error("snippet should appear before exit 0") + } + }) + + t.Run("no shebang prepends", func(t *testing.T) { + existing := "echo 'no shebang'\n" + snippet := "SNIPPET\n" + result := embedSnippet(existing, snippet) + if !strings.HasPrefix(result, "SNIPPET\n") { + t.Errorf( + "snippet should be prepended, got:\n%s", + result, + ) + } + }) +} + +func TestNeedsUpgrade(t *testing.T) { + t.Run("outdated hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook( + "#!/bin/sh\n# roborev post-commit hook\n" + + "roborev enqueue\n", + ) + if !NeedsUpgrade( + repo.Root, "post-commit", PostCommitVersionMarker, + ) { + t.Error("should detect outdated hook") + } + }) + + t.Run("current hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook( + "#!/bin/sh\n# roborev " + + PostCommitVersionMarker + + "\nroborev enqueue\n", + ) + if NeedsUpgrade( + repo.Root, "post-commit", PostCommitVersionMarker, + ) { + t.Error("should not flag current hook") + } + }) + + t.Run("no hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if NeedsUpgrade( + repo.Root, "post-commit", PostCommitVersionMarker, + ) { + t.Error("should not flag missing hook") + } + }) + + t.Run("non-roborev hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook("#!/bin/sh\necho hello\n") + if NeedsUpgrade( + repo.Root, "post-commit", PostCommitVersionMarker, + ) { + t.Error("should not flag non-roborev hook") + } + }) + + t.Run("post-rewrite outdated", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + hooksDir := filepath.Join(repo.Root, ".git", "hooks") + os.MkdirAll(hooksDir, 0755) + os.WriteFile( + filepath.Join(hooksDir, "post-rewrite"), + []byte("#!/bin/sh\n# roborev hook\n"+ + "roborev remap\n"), + 0755, + ) + if !NeedsUpgrade( + repo.Root, "post-rewrite", + PostRewriteVersionMarker, + ) { + t.Error("should detect outdated post-rewrite hook") + } + }) + + t.Run("post-rewrite current", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + hooksDir := filepath.Join(repo.Root, ".git", "hooks") + os.MkdirAll(hooksDir, 0755) + os.WriteFile( + filepath.Join(hooksDir, "post-rewrite"), + []byte("#!/bin/sh\n# roborev "+ + PostRewriteVersionMarker+ + "\nroborev remap\n"), + 0755, + ) + if NeedsUpgrade( + repo.Root, "post-rewrite", + PostRewriteVersionMarker, + ) { + t.Error("should not flag current post-rewrite hook") + } + }) +} + +func TestMissing(t *testing.T) { + t.Run("missing post-rewrite with roborev post-commit", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook( + "#!/bin/sh\n# roborev " + + PostCommitVersionMarker + "\n" + + "roborev enqueue\n", + ) + if !Missing(repo.Root, "post-rewrite") { + t.Error("should detect missing post-rewrite") + } + }, + ) + + t.Run("no post-commit hook at all", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if Missing(repo.Root, "post-rewrite") { + t.Error("should not warn without post-commit") + } + }) + + t.Run("post-rewrite exists with roborev", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook( + "#!/bin/sh\n# roborev " + + PostCommitVersionMarker + "\n" + + "roborev enqueue\n", + ) + hooksDir := filepath.Join(repo.Root, ".git", "hooks") + os.WriteFile( + filepath.Join(hooksDir, "post-rewrite"), + []byte("#!/bin/sh\n# roborev "+ + PostRewriteVersionMarker+ + "\nroborev remap\n"), + 0755, + ) + if Missing(repo.Root, "post-rewrite") { + t.Error("should not warn when present") + } + }) + + t.Run("non-roborev post-commit", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook("#!/bin/sh\necho hello\n") + if Missing(repo.Root, "post-rewrite") { + t.Error("should not warn for non-roborev") + } + }) +} + +func TestInstall(t *testing.T) { + t.Run("fresh install creates standalone hook", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile( + filepath.Join(repo.HooksDir, "post-commit"), + ) + contentStr := string(content) + if !strings.HasPrefix(contentStr, "#!/bin/sh\n") { + t.Error("fresh install should have shebang") + } + if !strings.Contains( + contentStr, PostCommitVersionMarker, + ) { + t.Error("should have version marker") + } + }, + ) + + t.Run("embeds after shebang in existing hook", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + existing := "#!/bin/sh\necho 'custom'\n" + os.WriteFile(hookPath, []byte(existing), 0755) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if !strings.Contains( + contentStr, "echo 'custom'", + ) { + t.Error("original content should be preserved") + } + if !strings.Contains( + contentStr, PostCommitVersionMarker, + ) { + t.Error("should have roborev snippet") + } + }, + ) + + t.Run("function wrapper uses return not exit", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + existing := "#!/bin/sh\necho 'custom'\n" + os.WriteFile(hookPath, []byte(existing), 0755) + + Install(repo.HooksDir, "post-commit", false) + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if strings.Contains(contentStr, "exit 0") { + t.Error( + "embedded snippet must use return, not exit", + ) + } + if !strings.Contains(contentStr, "return 0") { + t.Error("embedded snippet should use return 0") + } + if !strings.Contains( + contentStr, "_roborev_hook() {", + ) { + t.Error("should use function wrapper") + } + }, + ) + + t.Run("early exit does not block snippet", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + // Husky-style hook with exit 0 at the end + husky := "#!/bin/sh\n" + + ". \"$(dirname \"$0\")/_/husky.sh\"\n" + + "npx lint-staged\n" + + "exit 0\n" + os.WriteFile(hookPath, []byte(husky), 0755) + + Install(repo.HooksDir, "post-commit", false) + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + snippetIdx := strings.Index( + contentStr, "_roborev_hook", + ) + exitIdx := strings.Index(contentStr, "exit 0") + if snippetIdx < 0 { + t.Fatal("snippet should be present") + } + if exitIdx < 0 { + t.Fatal("exit 0 should be preserved") + } + if snippetIdx > exitIdx { + t.Error( + "roborev snippet should appear before exit 0", + ) + } + }) + + t.Run("skips current version", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + current := GeneratePostRewrite() + os.WriteFile(hookPath, []byte(current), 0755) + + err := Install( + repo.HooksDir, "post-rewrite", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + if string(content) != current { + t.Error("current hook should not be modified") + } + }) + + t.Run("upgrades outdated hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + outdated := "#!/bin/sh\n" + + "# roborev post-commit hook\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" + os.WriteFile(hookPath, []byte(outdated), 0755) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if !strings.Contains( + contentStr, PostCommitVersionMarker, + ) { + t.Error("should have new version marker") + } + if strings.Contains( + contentStr, "# roborev post-commit hook\n", + ) { + t.Error("old marker should be removed") + } + }) + + t.Run("upgrade from v2 to v3", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v2Hook := "#!/bin/sh\n" + + "# roborev post-commit hook v2 - " + + "auto-reviews every commit\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "if [ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=$(command -v roborev " + + "2>/dev/null)\n" + + " [ -z \"$ROBOREV\" ] || " + + "[ ! -x \"$ROBOREV\" ] && exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" + os.WriteFile(hookPath, []byte(v2Hook), 0755) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if !strings.Contains( + contentStr, PostCommitVersionMarker, + ) { + t.Error("should have v3 marker") + } + if strings.Contains(contentStr, "hook v2") { + t.Error("v2 marker should be removed") + } + }) + + t.Run("upgrades mixed hook preserving user content", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + mixed := "#!/bin/sh\necho 'user code'\n" + + "# roborev post-rewrite hook\n" + + "ROBOREV=\"/usr/bin/roborev\"\n" + + "\"$ROBOREV\" remap --quiet 2>/dev/null\n" + os.WriteFile(hookPath, []byte(mixed), 0755) + + err := Install( + repo.HooksDir, "post-rewrite", false, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if !strings.Contains( + contentStr, "echo 'user code'", + ) { + t.Error("user content should be preserved") + } + if !strings.Contains( + contentStr, PostRewriteVersionMarker, + ) { + t.Error("should have new version marker") + } + }, + ) + + t.Run("appends to hooks with various shell shebangs", + func(t *testing.T) { + shebangs := []string{ + "#!/bin/sh", "#!/usr/bin/env sh", + "#!/bin/bash", "#!/usr/bin/env bash", + "#!/bin/zsh", "#!/usr/bin/env zsh", + "#!/bin/ksh", "#!/usr/bin/env ksh", + "#!/bin/dash", "#!/usr/bin/env dash", + } + for _, shebang := range shebangs { + t.Run(shebang, func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + existing := shebang + "\necho 'custom'\n" + os.WriteFile( + hookPath, + []byte(existing), + 0755, + ) + + err := Install( + repo.HooksDir, + "post-commit", false, + ) + if err != nil { + t.Fatalf( + "should append to %s: %v", + shebang, err, + ) + } + + content, _ := os.ReadFile(hookPath) + cs := string(content) + if !strings.Contains( + cs, "echo 'custom'", + ) { + t.Error("original should be preserved") + } + if !strings.Contains( + cs, PostCommitVersionMarker, + ) { + t.Error("roborev should be appended") + } + }) + } + }, + ) + + t.Run("refuses non-shell hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + python := "#!/usr/bin/env python3\nprint('hello')\n" + os.WriteFile(hookPath, []byte(python), 0755) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err == nil { + t.Fatal("expected error for non-shell hook") + } + if !strings.Contains( + err.Error(), "non-shell interpreter", + ) { + t.Errorf("unexpected error: %v", err) + } + + content, _ := os.ReadFile(hookPath) + if string(content) != python { + t.Error("hook should be unchanged") + } + }) + + t.Run("upgrade returns error on re-read failure", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + outdated := "#!/bin/sh\n" + + "# roborev post-commit hook\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null\n" + os.WriteFile(hookPath, []byte(outdated), 0755) + + origReadFile := ReadFile + ReadFile = func(string) ([]byte, error) { + return nil, fs.ErrPermission + } + t.Cleanup(func() { ReadFile = origReadFile }) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err == nil { + t.Fatal("expected error from re-read failure") + } + if !strings.Contains(err.Error(), "re-read") { + t.Errorf( + "error should mention re-read: %v", err, + ) + } + if !errors.Is(err, fs.ErrPermission) { + t.Errorf( + "should wrap ErrPermission: %v", err, + ) + } + }, + ) + + t.Run("refuses upgrade of non-shell hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + python := "#!/usr/bin/env python3\n" + + "# reviewed by roborev\nprint('hello')\n" + os.WriteFile(hookPath, []byte(python), 0755) + + err := Install( + repo.HooksDir, "post-commit", false, + ) + if err == nil { + t.Fatal( + "expected error for non-shell upgrade", + ) + } + if !strings.Contains( + err.Error(), "non-shell interpreter", + ) { + t.Errorf("unexpected error: %v", err) + } + + content, _ := os.ReadFile(hookPath) + if string(content) != python { + t.Error("hook should be unchanged") + } + }) + + t.Run("force overwrites existing hook", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + existing := "#!/bin/sh\necho 'custom'\n" + os.WriteFile(hookPath, []byte(existing), 0755) + + err := Install( + repo.HooksDir, "post-commit", true, + ) + if err != nil { + t.Fatalf("Install: %v", err) + } + + content, _ := os.ReadFile(hookPath) + contentStr := string(content) + if strings.Contains(contentStr, "echo 'custom'") { + t.Error("force should overwrite, not append") + } + if !strings.Contains( + contentStr, PostCommitVersionMarker, + ) { + t.Error("should have roborev content") + } + }) +} + +func TestInstallAll(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test checks Unix exec bits") + } + + repo := testutil.NewTestRepo(t) + repo.RemoveHooksDir() + + if err := os.MkdirAll(repo.HooksDir, 0755); err != nil { + t.Fatal(err) + } + + if err := InstallAll(repo.HooksDir, false); err != nil { + t.Fatalf("InstallAll: %v", err) + } + + for _, name := range []string{"post-commit", "post-rewrite"} { + path := filepath.Join(repo.HooksDir, name) + content, err := os.ReadFile(path) + if err != nil { + t.Errorf("%s hook not created: %v", name, err) + continue + } + if !strings.Contains( + string(content), VersionMarker(name), + ) { + t.Errorf( + "%s should contain version marker", name, + ) + } + } +} + +func TestUninstall(t *testing.T) { + t.Run("generated hook is deleted entirely", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + os.WriteFile( + hookPath, + []byte(GeneratePostRewrite()), + 0755, + ) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + if _, err := os.Stat(hookPath); !os.IsNotExist(err) { + t.Error("should be deleted entirely") + } + }, + ) + + t.Run("mixed hook preserves non-roborev content", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + mixed := "#!/bin/sh\necho 'custom logic'\n" + + GeneratePostRewrite() + os.WriteFile(hookPath, []byte(mixed), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook should still exist: %v", err) + } + cs := string(content) + if strings.Contains( + strings.ToLower(cs), "roborev", + ) { + t.Errorf( + "roborev content should be removed:\n%s", + cs, + ) + } + if !strings.Contains(cs, "echo 'custom logic'") { + t.Error("custom content should be preserved") + } + if strings.Contains(cs, "\nfi\n") || + strings.HasSuffix( + strings.TrimSpace(cs), "fi", + ) { + t.Errorf("should not have orphaned fi:\n%s", cs) + } + }, + ) + + t.Run("v3 function wrapper removed", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + // Simulate a v3 embedded hook + v3Content := "#!/bin/sh\n" + + generateEmbeddablePostCommit() + + "echo 'user code after'\n" + os.WriteFile(hookPath, []byte(v3Content), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook should still exist: %v", err) + } + cs := string(content) + if strings.Contains(cs, "_roborev_hook") { + t.Errorf( + "function wrapper should be removed:\n%s", cs, + ) + } + if strings.Contains(cs, "return 0") { + t.Errorf( + "return 0 should be removed:\n%s", cs, + ) + } + if !strings.Contains(cs, "echo 'user code after'") { + t.Error("user content should be preserved") + } + }) + + t.Run("v3 mixed hook preserves user content", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v3Mixed := "#!/bin/sh\n" + + generateEmbeddablePostCommit() + + "echo 'before'\necho 'after'\n" + os.WriteFile(hookPath, []byte(v3Mixed), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook should exist: %v", err) + } + cs := string(content) + if strings.Contains( + strings.ToLower(cs), "roborev", + ) { + t.Errorf("roborev removed:\n%s", cs) + } + if !strings.Contains(cs, "echo 'before'") { + t.Error("should preserve 'before'") + } + if !strings.Contains(cs, "echo 'after'") { + t.Error("should preserve 'after'") + } + }, + ) + + t.Run("v0 hook removed", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v0 := "#!/bin/sh\n" + + "# RoboRev post-commit hook - " + + "auto-reviews every commit\n" + + "roborev enqueue --sha HEAD 2>/dev/null &\n" + os.WriteFile(hookPath, []byte(v0), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + if _, err := os.Stat(hookPath); !os.IsNotExist(err) { + content, _ := os.ReadFile(hookPath) + t.Errorf( + "v0 hook should be deleted:\n%s", content, + ) + } + }) + + t.Run("v0.5 hook removed", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v05 := "#!/bin/sh\n" + + "# RoboRev post-commit hook - " + + "auto-reviews every commit\n" + + "ROBOREV=\"/usr/local/bin/roborev\"\n" + + "if [ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=$(command -v roborev) || exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet &\n" + os.WriteFile(hookPath, []byte(v05), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + if _, err := os.Stat(hookPath); !os.IsNotExist(err) { + content, _ := os.ReadFile(hookPath) + t.Errorf( + "v0.5 hook should be deleted:\n%s", content, + ) + } + }) + + t.Run("v1 hook removed", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v1 := "#!/bin/sh\n" + + "# RoboRev post-commit hook - " + + "auto-reviews every commit\n" + + "ROBOREV=$(command -v roborev 2>/dev/null)\n" + + "if [ -z \"$ROBOREV\" ] || " + + "[ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=\"/usr/local/bin/roborev\"\n" + + " [ ! -x \"$ROBOREV\" ] && exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" + os.WriteFile(hookPath, []byte(v1), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + if _, err := os.Stat(hookPath); !os.IsNotExist(err) { + content, _ := os.ReadFile(hookPath) + t.Errorf( + "v1 hook should be deleted:\n%s", content, + ) + } + }) + + t.Run("v1 mixed hook removes only roborev block", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-commit", + ) + v1Block := "# RoboRev post-commit hook - " + + "auto-reviews every commit\n" + + "ROBOREV=$(command -v roborev 2>/dev/null)\n" + + "if [ -z \"$ROBOREV\" ] || " + + "[ ! -x \"$ROBOREV\" ]; then\n" + + " ROBOREV=\"/usr/local/bin/roborev\"\n" + + " [ ! -x \"$ROBOREV\" ] && exit 0\n" + + "fi\n" + + "\"$ROBOREV\" enqueue --quiet 2>/dev/null &\n" + mixed := "#!/bin/sh\necho 'custom'\n" + v1Block + os.WriteFile(hookPath, []byte(mixed), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook should exist: %v", err) + } + cs := string(content) + if strings.Contains( + strings.ToLower(cs), "roborev", + ) { + t.Errorf("roborev removed:\n%s", cs) + } + if !strings.Contains(cs, "echo 'custom'") { + t.Error("custom content should be preserved") + } + }, + ) + + t.Run("no-op for no roborev content", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + hookContent := "#!/bin/sh\necho 'unrelated'\n" + os.WriteFile(hookPath, []byte(hookContent), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, _ := os.ReadFile(hookPath) + if string(content) != hookContent { + t.Error("hook should be unchanged") + } + }) + + t.Run("custom if-block after snippet preserved", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if err := os.MkdirAll( + repo.HooksDir, 0755, + ); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join( + repo.HooksDir, "post-rewrite", + ) + mixed := "#!/bin/sh\n" + + GeneratePostRewrite() + + "if [ -f .notify ]; then\n" + + " echo 'send notification'\n" + + "fi\n" + os.WriteFile(hookPath, []byte(mixed), 0755) + + if err := Uninstall(hookPath); err != nil { + t.Fatalf("Uninstall: %v", err) + } + + content, err := os.ReadFile(hookPath) + if err != nil { + t.Fatalf("hook should exist: %v", err) + } + cs := string(content) + if !strings.Contains(cs, "send notification") { + t.Error("user if-block should be preserved") + } + if strings.Contains(cs, "remap --quiet") { + t.Errorf("snippet should be removed:\n%s", cs) + } + // Balanced if/fi + lines := strings.Split(cs, "\n") + ifCount, fiCount := 0, 0 + for _, l := range lines { + tr := strings.TrimSpace(l) + if strings.HasPrefix(tr, "if ") { + ifCount++ + } + if tr == "fi" { + fiCount++ + } + } + if ifCount != fiCount { + t.Errorf( + "if/fi mismatch: %d vs %d in:\n%s", + ifCount, fiCount, cs, + ) + } + }, + ) + + t.Run("missing file is no-op", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "nonexistent") + if err := Uninstall(path); err != nil { + t.Errorf("should be no-op: %v", err) + } + }) +} + +func TestVersionMarker(t *testing.T) { + if m := VersionMarker("post-commit"); m != PostCommitVersionMarker { + t.Errorf("got %q, want %q", m, PostCommitVersionMarker) + } + if m := VersionMarker("post-rewrite"); m != PostRewriteVersionMarker { + t.Errorf("got %q, want %q", m, PostRewriteVersionMarker) + } + if m := VersionMarker("unknown"); m != "" { + t.Errorf("unknown should return empty, got %q", m) + } +} From 9aab4aebe346be5258b27f0017b08ae087750dbf Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 18 Feb 2026 14:22:58 -0600 Subject: [PATCH 2/6] Fix review findings: init regression, auto-install gap, embedSnippet edge case 1. initCmd now warns and continues (instead of failing) when a non-shell hook exists, so daemon setup still completes. 2. autoInstallHooks uses new NotInstalled() instead of Missing() so a completely absent post-commit hook is also auto-installed. Missing() only detects absent dependent hooks (post-rewrite) when post-commit already has roborev content. 3. embedSnippet normalizes the shebang line to end with a newline before concatenating the snippet, preventing corrupted output like "#!/bin/sh# roborev ..." when the input lacks a trailing newline. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 6 +++-- internal/githook/githook.go | 24 ++++++++++++++++++- internal/githook/githook_test.go | 40 ++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index d4a8dbe2..7f5457c9 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -461,7 +461,9 @@ func initCmd() *cobra.Command { return fmt.Errorf("create hooks directory: %w", err) } if err := githook.InstallAll(hooksDir, false); err != nil { - return fmt.Errorf("install hooks: %w", err) + // Non-shell hooks produce an error but shouldn't + // abort init — warn and continue to daemon setup. + fmt.Printf(" Warning: %v\n", err) } // 5. Start daemon (or just register if --no-daemon) @@ -3130,7 +3132,7 @@ func autoInstallHooks(repoPath string) { for _, name := range []string{"post-commit", "post-rewrite"} { marker := githook.VersionMarker(name) if githook.NeedsUpgrade(repoPath, name, marker) || - githook.Missing(repoPath, name) { + githook.NotInstalled(repoPath, name) { if err := githook.Install(hooksDir, name, false); err != nil { fmt.Fprintf(os.Stderr, "Warning: auto-install %s hook: %v\n", diff --git a/internal/githook/githook.go b/internal/githook/githook.go index 2a181638..0126f030 100644 --- a/internal/githook/githook.go +++ b/internal/githook/githook.go @@ -53,6 +53,24 @@ func NeedsUpgrade(repoPath, hookName, versionMarker string) bool { !strings.Contains(s, versionMarker) } +// NotInstalled checks whether the named hook file is absent +// or does not contain any roborev content. +func NotInstalled(repoPath, hookName string) bool { + hooksDir, err := git.GetHooksPath(repoPath) + if err != nil { + return false + } + content, err := os.ReadFile( + filepath.Join(hooksDir, hookName), + ) + if err != nil { + return true + } + return !strings.Contains( + strings.ToLower(string(content)), "roborev", + ) +} + // Missing checks whether a repo has roborev installed // (post-commit hook present) but is missing the named hook. func Missing(repoPath, hookName string) bool { @@ -193,7 +211,11 @@ func embedSnippet(existing, snippet string) string { lines := strings.SplitAfter(existing, "\n") if len(lines) > 0 && strings.HasPrefix(strings.TrimSpace(lines[0]), "#!") { - return lines[0] + snippet + strings.Join(lines[1:], "") + shebang := lines[0] + if !strings.HasSuffix(shebang, "\n") { + shebang += "\n" + } + return shebang + snippet + strings.Join(lines[1:], "") } return snippet + existing } diff --git a/internal/githook/githook_test.go b/internal/githook/githook_test.go index 24bf9402..c846f0f9 100644 --- a/internal/githook/githook_test.go +++ b/internal/githook/githook_test.go @@ -222,6 +222,21 @@ func TestEmbedSnippet(t *testing.T) { ) } }) + + t.Run("shebang without trailing newline", func(t *testing.T) { + existing := "#!/bin/sh" + snippet := "SNIPPET\n" + result := embedSnippet(existing, snippet) + if !strings.HasPrefix(result, "#!/bin/sh\n") { + t.Errorf( + "shebang should get trailing newline, got:\n%q", + result, + ) + } + if !strings.Contains(result, "SNIPPET") { + t.Error("snippet should be present") + } + }) } func TestNeedsUpgrade(t *testing.T) { @@ -309,6 +324,31 @@ func TestNeedsUpgrade(t *testing.T) { }) } +func TestNotInstalled(t *testing.T) { + t.Run("hook file absent", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + if !NotInstalled(repo.Root, "post-commit") { + t.Error("absent hook should be not installed") + } + }) + + t.Run("hook without roborev", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook("#!/bin/sh\necho hello\n") + if !NotInstalled(repo.Root, "post-commit") { + t.Error("non-roborev hook should be not installed") + } + }) + + t.Run("hook with roborev", func(t *testing.T) { + repo := testutil.NewTestRepo(t) + repo.WriteHook(GeneratePostCommit()) + if NotInstalled(repo.Root, "post-commit") { + t.Error("roborev hook should be installed") + } + }) +} + func TestMissing(t *testing.T) { t.Run("missing post-rewrite with roborev post-commit", func(t *testing.T) { From 363683aa5054187009917d2ee5c0e881bf330d3b Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 18 Feb 2026 14:30:37 -0600 Subject: [PATCH 3/6] Fix init error handling and NotInstalled read-error semantics 1. initCmd now only downgrades ErrNonShellHook to a warning; real install failures (permission, I/O) still abort init. Install() wraps the sentinel ErrNonShellHook so callers can use errors.Is() to distinguish it. 2. NotInstalled returns true only for os.IsNotExist errors; other read failures (permission, I/O) return false to avoid masking problems and triggering repeated auto-install attempts. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 8 +++++--- internal/githook/githook.go | 21 ++++++++++++--------- internal/githook/githook_test.go | 32 ++++++++++++++++++++++++-------- 3 files changed, 41 insertions(+), 20 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 7f5457c9..6e471dce 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -461,9 +461,11 @@ func initCmd() *cobra.Command { return fmt.Errorf("create hooks directory: %w", err) } if err := githook.InstallAll(hooksDir, false); err != nil { - // Non-shell hooks produce an error but shouldn't - // abort init — warn and continue to daemon setup. - fmt.Printf(" Warning: %v\n", err) + if errors.Is(err, githook.ErrNonShellHook) { + fmt.Printf(" Warning: %v\n", err) + } else { + return fmt.Errorf("install hooks: %w", err) + } } // 5. Start daemon (or just register if --no-daemon) diff --git a/internal/githook/githook.go b/internal/githook/githook.go index 0126f030..5b9791f3 100644 --- a/internal/githook/githook.go +++ b/internal/githook/githook.go @@ -5,6 +5,7 @@ package githook import ( + "errors" "fmt" "os" "os/exec" @@ -14,6 +15,10 @@ import ( "github.com/roborev-dev/roborev/internal/git" ) +// ErrNonShellHook is returned when a hook uses a non-shell +// interpreter and cannot be safely modified. +var ErrNonShellHook = errors.New("non-shell interpreter") + // Version markers identify the current hook template version. // Bump these when the hook template changes to trigger // upgrade warnings and auto-upgrades. @@ -64,7 +69,7 @@ func NotInstalled(repoPath, hookName string) bool { filepath.Join(hooksDir, hookName), ) if err != nil { - return true + return os.IsNotExist(err) } return !strings.Contains( strings.ToLower(string(content)), "roborev", @@ -239,10 +244,9 @@ func Install(hooksDir, hookName string, force bool) error { ) { if !isShellHook(existingStr) { return fmt.Errorf( - "%s hook uses a non-shell interpreter; "+ - "add the roborev snippet manually "+ - "or use --force to overwrite", - hookName) + "%s hook: %w; add the roborev snippet "+ + "manually or use --force to overwrite", + hookName, ErrNonShellHook) } hookContent = embedSnippet( existingStr, @@ -258,10 +262,9 @@ func Install(hooksDir, hookName string, force bool) error { // Upgrade: remove old snippet, embed new one if !isShellHook(existingStr) { return fmt.Errorf( - "%s hook uses a non-shell interpreter; "+ - "add the roborev snippet manually "+ - "or use --force to overwrite", - hookName) + "%s hook: %w; add the roborev snippet "+ + "manually or use --force to overwrite", + hookName, ErrNonShellHook) } if rmErr := Uninstall(hookPath); rmErr != nil { return fmt.Errorf( diff --git a/internal/githook/githook_test.go b/internal/githook/githook_test.go index c846f0f9..1bb0be2b 100644 --- a/internal/githook/githook_test.go +++ b/internal/githook/githook_test.go @@ -347,6 +347,24 @@ func TestNotInstalled(t *testing.T) { t.Error("roborev hook should be installed") } }) + + t.Run("non-ENOENT read error returns false", + func(t *testing.T) { + repo := testutil.NewTestRepo(t) + // Create a directory where the hook file would be. + // Reading a directory is a non-ENOENT I/O error. + hookPath := filepath.Join( + repo.Root, ".git", "hooks", "post-commit", + ) + os.MkdirAll(hookPath, 0755) + if NotInstalled(repo.Root, "post-commit") { + t.Error( + "non-ENOENT error should not report " + + "as not installed", + ) + } + }, + ) } func TestMissing(t *testing.T) { @@ -758,10 +776,8 @@ func TestInstall(t *testing.T) { if err == nil { t.Fatal("expected error for non-shell hook") } - if !strings.Contains( - err.Error(), "non-shell interpreter", - ) { - t.Errorf("unexpected error: %v", err) + if !errors.Is(err, ErrNonShellHook) { + t.Errorf("should wrap ErrNonShellHook: %v", err) } content, _ := os.ReadFile(hookPath) @@ -834,10 +850,10 @@ func TestInstall(t *testing.T) { "expected error for non-shell upgrade", ) } - if !strings.Contains( - err.Error(), "non-shell interpreter", - ) { - t.Errorf("unexpected error: %v", err) + if !errors.Is(err, ErrNonShellHook) { + t.Errorf( + "should wrap ErrNonShellHook: %v", err, + ) } content, _ := os.ReadFile(hookPath) From ad8a7bc794b74038d2ae1c44dfa6218820c65c45 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 18 Feb 2026 15:03:33 -0600 Subject: [PATCH 4/6] Fix InstallAll error aggregation and add init branch tests InstallAll now uses errors.Join to attempt all hooks instead of stopping at first error. autoInstallHooks silently suppresses ErrNonShellHook warnings. Added TestInitCmdWarnsOnNonShellHook and TestInitCmdFailsOnHookWriteError for init branch coverage. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/hook_test.go | 73 +++++++++++++++++++++++++++++++++++++ cmd/roborev/main.go | 10 +++-- internal/githook/githook.go | 6 ++- 3 files changed, 84 insertions(+), 5 deletions(-) diff --git a/cmd/roborev/hook_test.go b/cmd/roborev/hook_test.go index 3a48c1e1..57d31c9d 100644 --- a/cmd/roborev/hook_test.go +++ b/cmd/roborev/hook_test.go @@ -417,3 +417,76 @@ func TestInitInstallsPostRewriteHookOnUpgrade(t *testing.T) { t.Error("post-rewrite hook should contain 'remap --quiet'") } } + +func TestInitCmdWarnsOnNonShellHook(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test uses shell script stub, skipping on Windows") + } + root := initNoDaemonSetup(t) + + // Install a non-shell post-commit hook + hooksDir := filepath.Join(root, ".git", "hooks") + os.MkdirAll(hooksDir, 0755) + os.WriteFile( + filepath.Join(hooksDir, "post-commit"), + []byte("#!/usr/bin/env python3\nprint('hello')\n"), + 0755, + ) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + })) + defer ts.Close() + + oldAddr := serverAddr + serverAddr = ts.URL + defer func() { serverAddr = oldAddr }() + + output := captureStdout(t, func() { + cmd := initCmd() + cmd.SetArgs([]string{"--no-daemon"}) + _ = cmd.Execute() + }) + + // Should warn about non-shell hook but still succeed + if !strings.Contains(output, "non-shell interpreter") { + t.Errorf("expected non-shell warning, got:\n%s", output) + } + if !strings.Contains(output, "Ready!") { + t.Errorf("init should still succeed with Ready!, got:\n%s", output) + } +} + +func TestInitCmdFailsOnHookWriteError(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test uses shell script stub, skipping on Windows") + } + root := initNoDaemonSetup(t) + + // Make hooks directory unwritable to trigger a real error + hooksDir := filepath.Join(root, ".git", "hooks") + os.MkdirAll(hooksDir, 0755) + os.Chmod(hooksDir, 0555) + t.Cleanup(func() { os.Chmod(hooksDir, 0755) }) + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + })) + defer ts.Close() + + oldAddr := serverAddr + serverAddr = ts.URL + defer func() { serverAddr = oldAddr }() + + cmd := initCmd() + cmd.SetArgs([]string{"--no-daemon"}) + err := cmd.Execute() + + // Should fail on permission error, not warn-and-continue + if err == nil { + t.Fatal("expected error for unwritable hooks directory") + } + if !strings.Contains(err.Error(), "install hooks") { + t.Errorf("error should mention install hooks: %v", err) + } +} diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 6e471dce..0bf0cc65 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -3136,9 +3136,13 @@ func autoInstallHooks(repoPath string) { if githook.NeedsUpgrade(repoPath, name, marker) || githook.NotInstalled(repoPath, name) { if err := githook.Install(hooksDir, name, false); err != nil { - fmt.Fprintf(os.Stderr, - "Warning: auto-install %s hook: %v\n", - name, err) + // Non-shell hooks are a persistent condition; + // don't warn on every invocation. + if !errors.Is(err, githook.ErrNonShellHook) { + fmt.Fprintf(os.Stderr, + "Warning: auto-install %s hook: %v\n", + name, err) + } } } } diff --git a/internal/githook/githook.go b/internal/githook/githook.go index 5b9791f3..09860689 100644 --- a/internal/githook/githook.go +++ b/internal/githook/githook.go @@ -300,13 +300,15 @@ func Install(hooksDir, hookName string, force bool) error { } // InstallAll installs both post-commit and post-rewrite hooks. +// It attempts all hooks and returns a joined error if any fail. func InstallAll(hooksDir string, force bool) error { + var errs []error for _, name := range []string{"post-commit", "post-rewrite"} { if err := Install(hooksDir, name, force); err != nil { - return err + errs = append(errs, err) } } - return nil + return errors.Join(errs...) } // Uninstall removes the roborev block from a hook file, or From 62baeb1fb0ca3f7024f9a9a3215dbc35e11a0824 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 18 Feb 2026 15:06:48 -0600 Subject: [PATCH 5/6] Fix uninstall/review conflict, validation ordering, and init error masking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - autoInstallHooks uses Missing (not NotInstalled) so explicit uninstall-hook is respected — review only upgrades outdated hooks and installs companion hooks when roborev is already present. - Move autoInstallHooks after CLI argument validation so invalid flag combinations don't cause hook side effects. - Add HasRealErrors helper to distinguish joined errors containing only ErrNonShellHook from those with real failures. init now correctly fails when a non-shell warning is joined with an actual write error. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 41 +++++++++++++------------ internal/githook/githook.go | 20 +++++++++++++ internal/githook/githook_test.go | 51 ++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 19 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 0bf0cc65..4b6b68eb 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -461,11 +461,10 @@ func initCmd() *cobra.Command { return fmt.Errorf("create hooks directory: %w", err) } if err := githook.InstallAll(hooksDir, false); err != nil { - if errors.Is(err, githook.ErrNonShellHook) { - fmt.Printf(" Warning: %v\n", err) - } else { + if githook.HasRealErrors(err) { return fmt.Errorf("install hooks: %w", err) } + fmt.Printf(" Warning: %v\n", err) } // 5. Start daemon (or just register if --no-daemon) @@ -810,19 +809,6 @@ Examples: return nil // Intentional skip, exit 0 } - // Auto-install/upgrade hooks when running from CLI - // (not when called from a hook via --quiet) - if !quiet { - autoInstallHooks(root) - } - - // Ensure daemon is running (skip for --local mode) - if !local { - if err := ensureDaemon(); err != nil { - return err // Return error (quiet mode silences output, not exit code) - } - } - // Validate mutually exclusive options if branch != "" && dirty { return fmt.Errorf("cannot use --branch with --dirty") @@ -845,6 +831,21 @@ Examples: return fmt.Errorf("invalid --type %q (valid: security, design)", reviewType) } + // Auto-install/upgrade hooks when running from CLI + // (not when called from a hook via --quiet). + // Runs after validation so invalid args don't + // cause side effects. + if !quiet { + autoInstallHooks(root) + } + + // Ensure daemon is running (skip for --local mode) + if !local { + if err := ensureDaemon(); err != nil { + return err // Return error (quiet mode silences output, not exit code) + } + } + var gitRef string var diffContent string @@ -3124,8 +3125,10 @@ func resolveReasoningWithFast(reasoning string, fast bool, reasoningExplicitlySe return reasoning } -// autoInstallHooks installs or upgrades hooks when running -// from the CLI (not from hooks themselves). +// autoInstallHooks upgrades outdated hooks and installs +// companion hooks (e.g. post-rewrite when post-commit +// exists). It does NOT install hooks from scratch so that +// explicit uninstall-hook is respected. func autoInstallHooks(repoPath string) { hooksDir, err := git.GetHooksPath(repoPath) if err != nil { @@ -3134,7 +3137,7 @@ func autoInstallHooks(repoPath string) { for _, name := range []string{"post-commit", "post-rewrite"} { marker := githook.VersionMarker(name) if githook.NeedsUpgrade(repoPath, name, marker) || - githook.NotInstalled(repoPath, name) { + githook.Missing(repoPath, name) { if err := githook.Install(hooksDir, name, false); err != nil { // Non-shell hooks are a persistent condition; // don't warn on every invocation. diff --git a/internal/githook/githook.go b/internal/githook/githook.go index 09860689..79b8f266 100644 --- a/internal/githook/githook.go +++ b/internal/githook/githook.go @@ -19,6 +19,26 @@ import ( // interpreter and cannot be safely modified. var ErrNonShellHook = errors.New("non-shell interpreter") +// HasRealErrors returns true if err contains any error that +// is not ErrNonShellHook. Use this instead of errors.Is when +// checking joined errors that may contain both non-shell +// warnings and real failures. +func HasRealErrors(err error) bool { + if err == nil { + return false + } + type unwrapper interface{ Unwrap() []error } + if joined, ok := err.(unwrapper); ok { + for _, e := range joined.Unwrap() { + if !errors.Is(e, ErrNonShellHook) { + return true + } + } + return false + } + return !errors.Is(err, ErrNonShellHook) +} + // Version markers identify the current hook template version. // Bump these when the hook template changes to trigger // upgrade warnings and auto-upgrades. diff --git a/internal/githook/githook_test.go b/internal/githook/githook_test.go index 1bb0be2b..32b14a61 100644 --- a/internal/githook/githook_test.go +++ b/internal/githook/githook_test.go @@ -2,6 +2,7 @@ package githook import ( "errors" + "fmt" "io/fs" "os" "path/filepath" @@ -1312,3 +1313,53 @@ func TestVersionMarker(t *testing.T) { t.Errorf("unknown should return empty, got %q", m) } } + +func TestHasRealErrors(t *testing.T) { + realErr := errors.New("permission denied") + + t.Run("nil", func(t *testing.T) { + if HasRealErrors(nil) { + t.Error("nil should return false") + } + }) + + t.Run("only non-shell", func(t *testing.T) { + err := fmt.Errorf("hook: %w", ErrNonShellHook) + if HasRealErrors(err) { + t.Error("single ErrNonShellHook should return false") + } + }) + + t.Run("only real", func(t *testing.T) { + if !HasRealErrors(realErr) { + t.Error("real error should return true") + } + }) + + t.Run("joined all non-shell", func(t *testing.T) { + err := errors.Join( + fmt.Errorf("a: %w", ErrNonShellHook), + fmt.Errorf("b: %w", ErrNonShellHook), + ) + if HasRealErrors(err) { + t.Error("joined non-shell only should return false") + } + }) + + t.Run("joined mixed", func(t *testing.T) { + err := errors.Join( + fmt.Errorf("a: %w", ErrNonShellHook), + realErr, + ) + if !HasRealErrors(err) { + t.Error("joined with real error should return true") + } + }) + + t.Run("joined all real", func(t *testing.T) { + err := errors.Join(realErr, errors.New("disk full")) + if !HasRealErrors(err) { + t.Error("joined real errors should return true") + } + }) +} From 4995ad319e771b966543c47898ddc7d46358a28e Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 18 Feb 2026 15:17:36 -0600 Subject: [PATCH 6/6] Fix Missing read-error semantics and add coverage for review findings - Missing returns false on non-ENOENT read errors (e.g. permission denied, is-a-directory) instead of treating them as missing. - Add TestInitCmdFailsOnMixedHookErrors: non-shell post-commit + unwritable post-rewrite produces a fatal error, not a warning. - Add TestReviewInvalidArgsNoSideEffects: invalid flag combos (--branch --dirty) produce no hook writes or daemon contact. - Add TestMissing non-ENOENT subtest for the read-error edge case. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/hook_test.go | 44 ++++++++++++++++++++++++++++++++ cmd/roborev/review_test.go | 35 +++++++++++++++++++++++++ internal/githook/githook.go | 2 +- internal/githook/githook_test.go | 25 ++++++++++++++++++ 4 files changed, 105 insertions(+), 1 deletion(-) diff --git a/cmd/roborev/hook_test.go b/cmd/roborev/hook_test.go index 57d31c9d..c9a26ddd 100644 --- a/cmd/roborev/hook_test.go +++ b/cmd/roborev/hook_test.go @@ -490,3 +490,47 @@ func TestInitCmdFailsOnHookWriteError(t *testing.T) { t.Errorf("error should mention install hooks: %v", err) } } + +func TestInitCmdFailsOnMixedHookErrors(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("test uses shell script stub, skipping on Windows") + } + root := initNoDaemonSetup(t) + + // Non-shell post-commit (produces ErrNonShellHook) + + // unwritable post-rewrite (produces real write error). + // InstallAll joins both; init must treat as fatal. + hooksDir := filepath.Join(root, ".git", "hooks") + os.MkdirAll(hooksDir, 0755) + os.WriteFile( + filepath.Join(hooksDir, "post-commit"), + []byte("#!/usr/bin/env python3\nprint('hello')\n"), + 0755, + ) + // Create post-rewrite as a directory so writing it fails. + os.MkdirAll( + filepath.Join(hooksDir, "post-rewrite"), 0755, + ) + + ts := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + }), + ) + defer ts.Close() + + oldAddr := serverAddr + serverAddr = ts.URL + defer func() { serverAddr = oldAddr }() + + cmd := initCmd() + cmd.SetArgs([]string{"--no-daemon"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error when mixed errors from InstallAll") + } + if !strings.Contains(err.Error(), "install hooks") { + t.Errorf("error should mention install hooks: %v", err) + } +} diff --git a/cmd/roborev/review_test.go b/cmd/roborev/review_test.go index e7120f70..ccc4b84c 100644 --- a/cmd/roborev/review_test.go +++ b/cmd/roborev/review_test.go @@ -6,6 +6,8 @@ import ( "bytes" "encoding/json" "net/http" + "os" + "path/filepath" "strings" "testing" "time" @@ -687,3 +689,36 @@ func TestReviewFastFlag(t *testing.T) { } }) } + +func TestReviewInvalidArgsNoSideEffects(t *testing.T) { + _, cleanup := setupMockDaemon(t, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Error("daemon should not be contacted on invalid args") + w.WriteHeader(http.StatusOK) + }), + ) + defer cleanup() + + repo := newTestGitRepo(t) + hooksDir := filepath.Join(repo.Dir, ".git", "hooks") + + cmd := reviewCmd() + cmd.SetArgs([]string{ + "--repo", repo.Dir, "--branch", "--dirty", + }) + err := cmd.Execute() + if err == nil { + t.Fatal("expected error for --branch with --dirty") + } + + // Hooks directory should have no roborev-generated files. + for _, name := range []string{"post-commit", "post-rewrite"} { + if _, statErr := os.Stat( + filepath.Join(hooksDir, name), + ); statErr == nil { + t.Errorf( + "%s should not exist after invalid args", name, + ) + } + } +} diff --git a/internal/githook/githook.go b/internal/githook/githook.go index 79b8f266..bf4a6ab6 100644 --- a/internal/githook/githook.go +++ b/internal/githook/githook.go @@ -116,7 +116,7 @@ func Missing(repoPath, hookName string) bool { } content, err := os.ReadFile(filepath.Join(hooksDir, hookName)) if err != nil { - return true + return os.IsNotExist(err) } return !strings.Contains( strings.ToLower(string(content)), "roborev", diff --git a/internal/githook/githook_test.go b/internal/githook/githook_test.go index 32b14a61..de955cff 100644 --- a/internal/githook/githook_test.go +++ b/internal/githook/githook_test.go @@ -417,6 +417,31 @@ func TestMissing(t *testing.T) { t.Error("should not warn for non-roborev") } }) + + t.Run("non-ENOENT read error returns false", + func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("permission test unreliable on Windows") + } + repo := testutil.NewTestRepo(t) + repo.WriteHook( + "#!/bin/sh\n# roborev " + + PostCommitVersionMarker + "\n" + + "roborev enqueue\n", + ) + // Create post-rewrite as a directory so ReadFile + // fails with a non-ENOENT error. + prPath := filepath.Join( + repo.Root, ".git", "hooks", "post-rewrite", + ) + os.MkdirAll(prPath, 0755) + if Missing(repo.Root, "post-rewrite") { + t.Error( + "non-ENOENT error should return false", + ) + } + }, + ) } func TestInstall(t *testing.T) {