Refactors post-commit hook generation to eliminate code duplication, improve security, and ensure truly silent operation. (#13)

Follow on work from code review

## Changes
- **Factor out `generateHookContent()` helper** - Hook script generation
was duplicated in `init` and `install-hook` commands, risking drift. Now
uses a single shared function.
- **Security: prefer baked path over PATH lookup** - Hook now uses the
absolute path baked at install time first, only falling back to `command
-v roborev` if the baked binary is missing. Prevents PATH injection
attacks from repo-local or malicious binaries.
- **Add stderr redirect for silence** - Hook now redirects stderr
(`2>/dev/null`) so errors from `roborev enqueue --quiet` don't leak to
the terminal.
- **Add comprehensive tests** - `TestGenerateHookContent` verifies:
	- Shebang and RoboRev comment present
	- Baked path assignment comes before PATH fallback (security)
	- Enqueue line has `--quiet`, `2>/dev/null`, and `&` on same line
	- Baked path is properly quoted
## Hook before/after
**Before:**

```sh
#!/bin/sh
# RoboRev post-commit hook - auto-reviews every commit
roborev enqueue --quiet &
```

After:

```
#!/bin/sh
# RoboRev post-commit hook - auto-reviews every commit
ROBOREV="/path/to/roborev"
if [ ! -x "$ROBOREV" ]; then
		ROBOREV=$(command -v roborev 2>/dev/null) || exit 0
		[ ! -x "$ROBOREV" ] && exit 0
fi
"$ROBOREV" enqueue --quiet 2>/dev/null &
```

Test plan
- go test ./... passes
- Run roborev install-hook --force and verify hook content
- Make a commit and verify no output appears
- Test with baked path removed (should fall back to PATH)

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

Author: wesm

cmd/roborev/main.go

@@ -260,22 +260,7 @@ func initCmd() *cobra.Command {
 				return fmt.Errorf("get hooks path: %w", err)
 			}
 			hookPath := filepath.Join(hooksDir, "post-commit")
-
-			// Get full path to roborev executable to avoid PATH issues in hooks
-			roborevPath, err := exec.LookPath("roborev")
-			if err != nil {
-				roborevPath = "roborev" // Fallback to PATH lookup
-			}
-
-			// Create hook with proper quoting and fallback for moved/upgraded binaries
-			hookContent := fmt.Sprintf(`#!/bin/sh
-# RoboRev post-commit hook - auto-reviews every commit
-ROBOREV=%q
-if [ ! -x "$ROBOREV" ]; then
-    ROBOREV=$(command -v roborev) || exit 0
-fi
-"$ROBOREV" enqueue --quiet &
-`, roborevPath)
+			hookContent := generateHookContent()
 
 			// Ensure hooks directory exists
 			if err := os.MkdirAll(hooksDir, 0755); err != nil {
@@ -758,21 +743,7 @@ func installHookCmd() *cobra.Command {
 				return fmt.Errorf("create hooks directory: %w", err)
 			}
 
-			// Get full path to roborev executable to avoid PATH issues in hooks
-			roborevPath, err := exec.LookPath("roborev")
-			if err != nil {
-				roborevPath = "roborev" // Fallback to PATH lookup
-			}
-
-			// Create hook with proper quoting and fallback for moved/upgraded binaries
-			hookContent := fmt.Sprintf(`#!/bin/sh
-# RoboRev post-commit hook - auto-reviews every commit
-ROBOREV=%q
-if [ ! -x "$ROBOREV" ]; then
-    ROBOREV=$(command -v roborev) || exit 0
-fi
-"$ROBOREV" enqueue --quiet &
-`, roborevPath)
+			hookContent := generateHookContent()
 
 			if err := os.WriteFile(hookPath, []byte(hookContent), 0755); err != nil {
 				return fmt.Errorf("write hook: %w", err)
@@ -1007,3 +978,24 @@ func shortRef(ref string) string {
 	}
 	return shortSHA(ref)
 }
+
+// generateHookContent creates the post-commit hook script content.
+// It prefers the baked absolute path for security, falls back to PATH if missing.
+func generateHookContent() string {
+	// Get current roborev absolute path
+	roborevPath, err := exec.LookPath("roborev")
+	if err != nil {
+		roborevPath = "roborev"
+	}
+
+	// Prefer baked path (security), fall back to PATH only if baked is missing
+	return fmt.Sprintf(`#!/bin/sh
+# RoboRev post-commit hook - auto-reviews every commit
+ROBOREV=%q
+if [ ! -x "$ROBOREV" ]; then
+    ROBOREV=$(command -v roborev 2>/dev/null) || exit 0
+    [ ! -x "$ROBOREV" ] && exit 0
+fi
+"$ROBOREV" enqueue --quiet 2>/dev/null &
+`, roborevPath)
+}

cmd/roborev/main_test.go

@@ -453,3 +453,72 @@ func TestInstallHookCmdCreatesHooksDirectory(t *testing.T) {
 		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", func(t *testing.T) {
+		// Must have exact enqueue line with --quiet, stderr redirect, and background
+		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, 2>/dev/null, and & on same line")
+		}
+	})
+
+	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
+			}
+		}
+	})
+}