Skip to content

Commit 35ff13a

Browse files
wesmclaude
andauthored
Fix post-commit hook not firing in linked worktrees (#500)
## Summary - Resolve relative `core.hooksPath` values against the main repository root instead of the worktree root, both in `GetHooksPath` (for roborev's internal use) and via `EnsureAbsoluteHooksPath` (to fix the stored git config so git itself dispatches hooks correctly from linked worktrees) - Add JSONL post-commit hook logging to `~/.roborev/post-commit.log` so that silent hook failures leave an audit trail (timestamp, repo, outcome, reason) - Preserve git tilde-expansion paths (`~`, `~/path`, `~user/path`) during normalization - Apply hooks-path normalization in `init`, `install-hook`, and `uninstall-hook` as a fatal error so these commands stop before operating on the wrong directory - Clean up test anti-patterns in `internal/git/git_test.go` 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent b7cac70 commit 35ff13a

File tree

7 files changed

+653
-161
lines changed

7 files changed

+653
-161
lines changed

cmd/roborev/hook.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ func installHookCmd() *cobra.Command {
2222
return fmt.Errorf("not a git repository: %w", err)
2323
}
2424

25+
if err := git.EnsureAbsoluteHooksPath(root); err != nil {
26+
return fmt.Errorf("normalize hooks path: %w", err)
27+
}
2528
hooksDir, err := git.GetHooksPath(root)
2629
if err != nil {
2730
return fmt.Errorf("get hooks path: %w", err)

cmd/roborev/hook_test.go

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"net/http/httptest"
1010
"net/url"
1111
"os"
12+
"os/exec"
1213
"path/filepath"
1314
"runtime"
1415
"strings"
@@ -454,3 +455,116 @@ func TestInitNoDaemonWithAgentCreatesCommentedRepoConfig(t *testing.T) {
454455
}
455456
}
456457
}
458+
459+
func TestInstallHookFromLinkedWorktree(t *testing.T) {
460+
if runtime.GOOS == "windows" {
461+
t.Skip("test uses Unix worktree semantics")
462+
}
463+
464+
// Set up a main repo with a relative core.hooksPath but
465+
// no hooks installed yet.
466+
repo := testutil.NewTestRepoWithCommit(t)
467+
customHooks := filepath.Join(repo.Root, ".githooks")
468+
require.NoError(t, os.MkdirAll(customHooks, 0755))
469+
470+
runGit := func(dir string, args ...string) string {
471+
t.Helper()
472+
cmd := exec.Command("git", args...)
473+
cmd.Dir = dir
474+
cmd.Env = append(os.Environ(),
475+
"GIT_AUTHOR_NAME=Test",
476+
"GIT_AUTHOR_EMAIL=test@test.com",
477+
"GIT_COMMITTER_NAME=Test",
478+
"GIT_COMMITTER_EMAIL=test@test.com",
479+
)
480+
out, err := cmd.CombinedOutput()
481+
require.NoError(t, err, "git %v: %s", args, out)
482+
return strings.TrimSpace(string(out))
483+
}
484+
485+
runGit(repo.Root, "config", "core.hooksPath", ".githooks")
486+
487+
// Create a linked worktree.
488+
wtDir := t.TempDir()
489+
resolved, err := filepath.EvalSymlinks(wtDir)
490+
require.NoError(t, err)
491+
runGit(repo.Root, "worktree", "add", resolved, "-b", "wt")
492+
493+
// Run install-hook from the worktree.
494+
origDir, _ := os.Getwd()
495+
require.NoError(t, os.Chdir(resolved))
496+
t.Cleanup(func() { os.Chdir(origDir) })
497+
498+
installCmd := installHookCmd()
499+
installCmd.SetArgs([]string{})
500+
require.NoError(t, installCmd.Execute())
501+
502+
// The hooks should be installed in the main repo's .githooks.
503+
for _, name := range []string{"post-commit", "post-rewrite"} {
504+
_, err := os.Stat(filepath.Join(customHooks, name))
505+
assert.NoError(t, err,
506+
"%s should exist in shared hooks dir", name)
507+
}
508+
}
509+
510+
func TestUninstallHookFromLinkedWorktree(t *testing.T) {
511+
if runtime.GOOS == "windows" {
512+
t.Skip("test uses Unix worktree semantics")
513+
}
514+
515+
// Set up a main repo with a relative core.hooksPath and
516+
// installed hooks.
517+
repo := testutil.NewTestRepoWithCommit(t)
518+
customHooks := filepath.Join(repo.Root, ".githooks")
519+
require.NoError(t, os.MkdirAll(customHooks, 0755))
520+
521+
runGit := func(dir string, args ...string) string {
522+
t.Helper()
523+
cmd := exec.Command("git", args...)
524+
cmd.Dir = dir
525+
cmd.Env = append(os.Environ(),
526+
"GIT_AUTHOR_NAME=Test",
527+
"GIT_AUTHOR_EMAIL=test@test.com",
528+
"GIT_COMMITTER_NAME=Test",
529+
"GIT_COMMITTER_EMAIL=test@test.com",
530+
)
531+
out, err := cmd.CombinedOutput()
532+
require.NoError(t, err, "git %v: %s", args, out)
533+
return strings.TrimSpace(string(out))
534+
}
535+
536+
// Set relative core.hooksPath, install hooks.
537+
runGit(repo.Root, "config", "core.hooksPath", ".githooks")
538+
for _, name := range []string{"post-commit", "post-rewrite"} {
539+
var content string
540+
if name == "post-commit" {
541+
content = githook.GeneratePostCommit()
542+
} else {
543+
content = githook.GeneratePostRewrite()
544+
}
545+
require.NoError(t, os.WriteFile(
546+
filepath.Join(customHooks, name),
547+
[]byte(content), 0755))
548+
}
549+
550+
// Create a linked worktree.
551+
wtDir := t.TempDir()
552+
resolved, err := filepath.EvalSymlinks(wtDir)
553+
require.NoError(t, err)
554+
runGit(repo.Root, "worktree", "add", resolved, "-b", "wt")
555+
556+
// Run uninstall-hook from the worktree.
557+
origDir, _ := os.Getwd()
558+
require.NoError(t, os.Chdir(resolved))
559+
t.Cleanup(func() { os.Chdir(origDir) })
560+
561+
cmd := uninstallHookCmd()
562+
require.NoError(t, cmd.Execute())
563+
564+
// The hooks in the main repo's .githooks should be removed.
565+
for _, name := range []string{"post-commit", "post-rewrite"} {
566+
_, err := os.Stat(filepath.Join(customHooks, name))
567+
assert.ErrorIs(t, err, fs.ErrNotExist,
568+
"%s should be removed from shared hooks dir", name)
569+
}
570+
}

cmd/roborev/init_cmd.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,9 @@ func initCmd() *cobra.Command {
6565
}
6666

6767
// 4. Install hooks (post-commit + post-rewrite)
68+
if err := git.EnsureAbsoluteHooksPath(root); err != nil {
69+
return fmt.Errorf("normalize hooks path: %w", err)
70+
}
6871
hooksDir, err := git.GetHooksPath(root)
6972
if err != nil {
7073
return fmt.Errorf("get hooks path: %w", err)

cmd/roborev/postcommit.go

Lines changed: 80 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,14 @@ package main
33
import (
44
"bytes"
55
"encoding/json"
6+
"fmt"
67
"io"
78
"net/http"
9+
"os"
10+
"path/filepath"
811
"time"
912

13+
"github.com/roborev-dev/roborev/internal/config"
1014
"github.com/roborev-dev/roborev/internal/daemon"
1115
"github.com/roborev-dev/roborev/internal/git"
1216
"github.com/spf13/cobra"
@@ -16,6 +20,9 @@ import (
1620
// ensures hooks never block commits if the daemon stalls.
1721
var hookHTTPClient = &http.Client{Timeout: 3 * time.Second}
1822

23+
// hookLogPath can be overridden in tests.
24+
var hookLogPath = ""
25+
1926
func postCommitCmd() *cobra.Command {
2027
var (
2128
repoPath string
@@ -35,15 +42,20 @@ func postCommitCmd() *cobra.Command {
3542

3643
root, err := git.GetRepoRoot(repoPath)
3744
if err != nil {
38-
return nil // Not a repo — silent exit for hooks
45+
hookLog(repoPath, "skip", "not a git repo")
46+
return nil
3947
}
4048

4149
if git.IsRebaseInProgress(root) {
50+
hookLog(root, "skip", "rebase in progress")
4251
return nil
4352
}
4453

4554
if err := ensureDaemon(); err != nil {
46-
return nil // Can't reach daemon — don't block commit
55+
hookLog(root, "fail", fmt.Sprintf(
56+
"daemon unavailable: %v", err,
57+
))
58+
return nil
4759
}
4860

4961
var gitRef string
@@ -67,11 +79,26 @@ func postCommitCmd() *cobra.Command {
6779
bytes.NewReader(reqBody),
6880
)
6981
if err != nil {
70-
return nil // Network error — don't block commit
82+
hookLog(root, "fail", fmt.Sprintf(
83+
"enqueue request failed: %v", err,
84+
))
85+
return nil
7186
}
7287
defer resp.Body.Close()
73-
_, _ = io.Copy(io.Discard, resp.Body)
7488

89+
body, _ := io.ReadAll(resp.Body)
90+
if resp.StatusCode >= 400 {
91+
hookLog(root, "fail", fmt.Sprintf(
92+
"daemon returned %d: %s",
93+
resp.StatusCode,
94+
truncateBytes(body, 200),
95+
))
96+
return nil
97+
}
98+
99+
hookLog(root, "ok", fmt.Sprintf(
100+
"enqueued ref=%s branch=%s", gitRef, branchName,
101+
))
75102
return nil
76103
},
77104
}
@@ -106,3 +133,52 @@ func enqueueCmd() *cobra.Command {
106133
cmd.Hidden = true
107134
return cmd
108135
}
136+
137+
// hookLog appends a single JSONL entry to the post-commit log.
138+
// Best-effort: errors are silently ignored so the hook never
139+
// blocks a commit.
140+
func hookLog(repo, outcome, message string) {
141+
logPath := hookLogPath
142+
if logPath == "" {
143+
logPath = filepath.Join(
144+
config.DataDir(), "post-commit.log",
145+
)
146+
}
147+
148+
entry := struct {
149+
TS string `json:"ts"`
150+
Repo string `json:"repo"`
151+
Outcome string `json:"outcome"`
152+
Message string `json:"message"`
153+
}{
154+
TS: time.Now().Format(time.RFC3339),
155+
Repo: repo,
156+
Outcome: outcome,
157+
Message: message,
158+
}
159+
data, err := json.Marshal(entry)
160+
if err != nil {
161+
return
162+
}
163+
data = append(data, '\n')
164+
165+
if err := os.MkdirAll(filepath.Dir(logPath), 0700); err != nil {
166+
return
167+
}
168+
f, err := os.OpenFile(
169+
logPath, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0600,
170+
)
171+
if err != nil {
172+
return
173+
}
174+
defer f.Close()
175+
_ = f.Chmod(0600)
176+
_, _ = f.Write(data)
177+
}
178+
179+
func truncateBytes(b []byte, max int) string {
180+
if len(b) <= max {
181+
return string(b)
182+
}
183+
return string(b[:max]) + "..."
184+
}

cmd/roborev/postcommit_test.go

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,106 @@ func TestEnqueueRejectsPositionalArgs(t *testing.T) {
137137

138138
}
139139

140+
func TestPostCommitLogsSuccess(t *testing.T) {
141+
repo, mux := setupTestEnvironment(t)
142+
mockEnqueue(t, mux)
143+
144+
logFile := filepath.Join(t.TempDir(), "post-commit.log")
145+
old := hookLogPath
146+
hookLogPath = logFile
147+
t.Cleanup(func() { hookLogPath = old })
148+
149+
repo.CommitFile("file.txt", "content", "initial commit")
150+
151+
_, _, err := executePostCommitCmd("--repo", repo.Dir)
152+
require.NoError(t, err)
153+
154+
data, err := os.ReadFile(logFile)
155+
require.NoError(t, err)
156+
assert.Contains(t, string(data), `"outcome":"ok"`)
157+
assert.Contains(t, string(data), `"repo"`)
158+
}
159+
160+
func TestPostCommitLogsSkipNotARepo(t *testing.T) {
161+
logFile := filepath.Join(t.TempDir(), "post-commit.log")
162+
old := hookLogPath
163+
hookLogPath = logFile
164+
t.Cleanup(func() { hookLogPath = old })
165+
166+
dir := t.TempDir()
167+
_, _, err := executePostCommitCmd("--repo", dir)
168+
require.NoError(t, err)
169+
170+
data, err := os.ReadFile(logFile)
171+
require.NoError(t, err)
172+
assert.Contains(t, string(data), `"outcome":"skip"`)
173+
assert.Contains(t, string(data), "not a git repo")
174+
}
175+
176+
func TestPostCommitLogsSkipRebase(t *testing.T) {
177+
repo, _ := setupTestEnvironment(t)
178+
repo.CommitFile("file.txt", "content", "initial commit")
179+
180+
logFile := filepath.Join(t.TempDir(), "post-commit.log")
181+
old := hookLogPath
182+
hookLogPath = logFile
183+
t.Cleanup(func() { hookLogPath = old })
184+
185+
// Simulate rebase in progress.
186+
gitDir := strings.TrimSpace(repo.Run("rev-parse", "--git-dir"))
187+
if !filepath.IsAbs(gitDir) {
188+
gitDir = filepath.Join(repo.Dir, gitDir)
189+
}
190+
require.NoError(t, os.MkdirAll(
191+
filepath.Join(gitDir, "rebase-merge"), 0755))
192+
193+
_, _, err := executePostCommitCmd("--repo", repo.Dir)
194+
require.NoError(t, err)
195+
196+
data, err := os.ReadFile(logFile)
197+
require.NoError(t, err)
198+
assert.Contains(t, string(data), `"outcome":"skip"`)
199+
assert.Contains(t, string(data), "rebase in progress")
200+
}
201+
202+
func TestPostCommitLogsDaemonFailure(t *testing.T) {
203+
logFile := filepath.Join(t.TempDir(), "post-commit.log")
204+
old := hookLogPath
205+
hookLogPath = logFile
206+
t.Cleanup(func() { hookLogPath = old })
207+
208+
repo := newTestGitRepo(t)
209+
repo.CommitFile("file.txt", "content", "initial")
210+
211+
// Point serverAddr at nothing so ensureDaemon fails.
212+
patchServerAddr(t, "http://127.0.0.1:1")
213+
214+
_, _, err := executePostCommitCmd("--repo", repo.Dir)
215+
require.NoError(t, err)
216+
217+
data, err := os.ReadFile(logFile)
218+
require.NoError(t, err)
219+
assert.Contains(t, string(data), `"outcome":"fail"`)
220+
assert.Contains(t, string(data), "daemon")
221+
}
222+
223+
func TestPostCommitLogsCreatesParentDir(t *testing.T) {
224+
// Log path under a directory that doesn't exist yet,
225+
// simulating a fresh install where ~/.roborev is absent.
226+
logFile := filepath.Join(t.TempDir(), "subdir", "post-commit.log")
227+
old := hookLogPath
228+
hookLogPath = logFile
229+
t.Cleanup(func() { hookLogPath = old })
230+
231+
dir := t.TempDir()
232+
_, _, err := executePostCommitCmd("--repo", dir)
233+
require.NoError(t, err)
234+
235+
data, err := os.ReadFile(logFile)
236+
require.NoError(t, err, "log file should be created even when parent dir is absent")
237+
assert.Contains(t, string(data), `"outcome":"skip"`)
238+
}
239+
140240
// stallingRoundTripper blocks until the request context is
141241
// cancelled, then returns an error. This simulates a daemon
142242
// that accepts connections but never responds, without needing

0 commit comments

Comments
 (0)