diff --git a/.roborev.toml b/.roborev.toml index db1a0041..c93b38ff 100644 --- a/.roborev.toml +++ b/.roborev.toml @@ -19,4 +19,12 @@ The TUI talks to the daemon over loopback HTTP. Do not flag thundering-herd or rate-limiting concerns for local loopback requests unless the number of concurrent requests scales with untrusted input or external data. A bounded number of requests proportional to the user's tracked repos is acceptable. + +Git hooks (pre-commit, commit-msg, post-checkout, etc.) are the user's own scripts +running on their own machine. Hook stderr forwarded to agents or printed to the +terminal is local-only data that does not cross a trust boundary. Do not flag: +- "prompt injection" from hook output passed to agent prompts +- "secret exposure" from hook stderr being logged or forwarded to the model +- "unbounded capture" of git command stderr (git stderr is practically bounded) +These are the same class of local-data-in-local-tool issues covered above. """ diff --git a/cmd/roborev/refine.go b/cmd/roborev/refine.go index c0b243b0..f1ea1560 100644 --- a/cmd/roborev/refine.go +++ b/cmd/roborev/refine.go @@ -4,6 +4,7 @@ import ( "bufio" "bytes" "context" + "errors" "fmt" "io" "io/fs" @@ -466,7 +467,7 @@ func runRefine(agentName, modelStr, reasoningStr string, maxIterations int, quie cleanupWorktree() commitMsg := fmt.Sprintf("Address review findings (job %d)\n\n%s", currentFailedReview.JobID, summarizeAgentOutput(output)) - newCommit, err := git.CreateCommit(repoPath, commitMsg) + newCommit, err := commitWithHookRetry(repoPath, commitMsg, addressAgent, quiet) if err != nil { return fmt.Errorf("failed to commit changes: %w", err) } @@ -618,8 +619,9 @@ func createTempWorktree(repoPath string) (string, func(), error) { return "", nil, err } - // Create the worktree (without --recurse-submodules for compatibility with older git) - cmd := exec.Command("git", "-C", repoPath, "worktree", "add", "--detach", worktreeDir, "HEAD") + // Create the worktree (without --recurse-submodules for compatibility with older git). + // Suppress hooks via core.hooksPath= — user hooks shouldn't run in internal worktrees. + cmd := exec.Command("git", "-C", repoPath, "-c", "core.hooksPath="+os.DevNull, "worktree", "add", "--detach", worktreeDir, "HEAD") if out, err := cmd.CombinedOutput(); err != nil { os.RemoveAll(worktreeDir) return "", nil, fmt.Errorf("git worktree add: %w: %s", err, out) @@ -772,6 +774,125 @@ func applyWorktreeChanges(repoPath, worktreePath string) error { return nil } +// commitWithHookRetry attempts git.CreateCommit and, on failure, +// runs the agent to fix whatever the hook complained about. Only +// retries when a hook (pre-commit, commit-msg, etc.) caused the +// failure — other commit failures (missing identity, empty commit, +// lockfile) are returned immediately. Retries up to 3 total attempts. +func commitWithHookRetry( + repoPath, commitMsg string, + fixAgent agent.Agent, + quiet bool, +) (string, error) { + const maxAttempts = 3 + + expectedHead, err := git.ResolveSHA(repoPath, "HEAD") + if err != nil { + return "", fmt.Errorf("cannot determine HEAD: %w", err) + } + expectedBranch := git.GetCurrentBranch(repoPath) + + for attempt := 1; attempt <= maxAttempts; attempt++ { + sha, err := git.CreateCommit(repoPath, commitMsg) + if err == nil { + return sha, nil + } + + // Only retry when a hook positively caused the failure. + // Add-phase errors, non-hook commit errors, and commit + // failures without hooks are returned immediately. + var commitErr *git.CommitError + if !errors.As(err, &commitErr) || !commitErr.HookFailed { + return "", err + } + + if attempt == maxAttempts { + return "", fmt.Errorf( + "hook failed after %d attempts: %w", + maxAttempts, err, + ) + } + + hookErr := err.Error() + if !quiet { + fmt.Printf( + "Hook failed (attempt %d/%d), "+ + "running agent to fix:\n%s\n", + attempt, maxAttempts, hookErr, + ) + } + + if err := verifyRepoState( + repoPath, expectedHead, expectedBranch, + ); err != nil { + return "", fmt.Errorf( + "aborting hook retry: %w", err, + ) + } + + fixPrompt := fmt.Sprintf( + "A git hook rejected this commit with the "+ + "following error output. Fix the issues so "+ + "the commit can succeed.\n\n%s", + hookErr, + ) + + var agentOutput io.Writer + if quiet { + agentOutput = io.Discard + } else { + agentOutput = os.Stdout + } + + fixCtx, cancel := context.WithTimeout( + context.Background(), 5*time.Minute, + ) + _, agentErr := fixAgent.Review( + fixCtx, repoPath, "HEAD", fixPrompt, agentOutput, + ) + cancel() + + if agentErr != nil { + return "", fmt.Errorf( + "agent failed to fix hook issues: %w", agentErr, + ) + } + + if err := verifyRepoState( + repoPath, expectedHead, expectedBranch, + ); err != nil { + return "", fmt.Errorf( + "agent changed repo state during hook fix: %w", + err, + ) + } + } + + // unreachable, but satisfies the compiler + return "", fmt.Errorf("commit retry loop exited unexpectedly") +} + +// verifyRepoState checks that HEAD and current branch match expected +// values. Returns an error describing the drift if they don't. +func verifyRepoState( + repoPath, expectedHead, expectedBranch string, +) error { + currentHead, err := git.ResolveSHA(repoPath, "HEAD") + if err != nil { + return fmt.Errorf("cannot verify HEAD: %w", err) + } + currentBranch := git.GetCurrentBranch(repoPath) + if currentHead != expectedHead || + currentBranch != expectedBranch { + return fmt.Errorf( + "HEAD was %s on %s, now %s on %s", + shortSHA(expectedHead), expectedBranch, + shortSHA(currentHead), currentBranch, + ) + } + return nil +} + func selectRefineAgent(resolvedAgent string, reasoningLevel agent.ReasoningLevel, model string) (agent.Agent, error) { if resolvedAgent == "codex" && agent.IsAvailable("codex") { baseAgent, err := agent.Get("codex") diff --git a/cmd/roborev/refine_test.go b/cmd/roborev/refine_test.go index d9326d4a..09a474ca 100644 --- a/cmd/roborev/refine_test.go +++ b/cmd/roborev/refine_test.go @@ -34,6 +34,20 @@ func createMockExecutable(t *testing.T, dir, name string, exitCode int) string { return path } +// installGitHook writes a shell script as the named git hook +// (e.g. "pre-commit", "post-checkout") and makes it executable. +func installGitHook(t *testing.T, repoDir, name, script string) { + t.Helper() + hooksDir := filepath.Join(repoDir, ".git", "hooks") + if err := os.MkdirAll(hooksDir, 0755); err != nil { + t.Fatal(err) + } + hookPath := filepath.Join(hooksDir, name) + if err := os.WriteFile(hookPath, []byte(script), 0755); err != nil { + t.Fatal(err) + } +} + // gitCommitFile writes a file, stages it, commits, and returns the new HEAD SHA. func gitCommitFile(t *testing.T, repoDir string, runGit func(...string) string, filename, content, msg string) string { t.Helper() @@ -1080,6 +1094,208 @@ func TestWorktreeCleanupBetweenIterations(t *testing.T) { } } +func TestCreateTempWorktreeIgnoresHooks(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + repoDir, _, runGit := setupTestGitRepo(t) + + installGitHook(t, repoDir, "post-checkout", "#!/bin/sh\nexit 1\n") + + // Verify the hook is active (a normal worktree add would fail) + failDir := t.TempDir() + cmd := exec.Command("git", "-C", repoDir, "worktree", "add", "--detach", failDir, "HEAD") + if out, err := cmd.CombinedOutput(); err == nil { + // Clean up the worktree before failing + exec.Command("git", "-C", repoDir, "worktree", "remove", "--force", failDir).Run() + // Some git versions don't fail on post-checkout hook errors. + // In that case, verify our approach still succeeds. + _ = out + } + + // createTempWorktree should succeed because it suppresses hooks + worktreePath, cleanup, err := createTempWorktree(repoDir) + if err != nil { + t.Fatalf("createTempWorktree should succeed with failing hook: %v", err) + } + defer cleanup() + + // Verify the worktree directory exists and has the file from the repo + if _, err := os.Stat(worktreePath); err != nil { + t.Fatalf("worktree directory should exist: %v", err) + } + + baseFile := filepath.Join(worktreePath, "base.txt") + content, err := os.ReadFile(baseFile) + if err != nil { + t.Fatalf("expected base.txt in worktree: %v", err) + } + if string(content) != "base" { + t.Errorf("expected content 'base', got %q", string(content)) + } + + _ = runGit // used by setupTestGitRepo +} + +func TestCommitWithHookRetrySucceeds(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + repoDir, _, runGit := setupTestGitRepo(t) + + // Install a pre-commit hook that fails on the first 2 calls and + // succeeds on the 3rd+. The hook runs twice before a retry: once + // by git commit, once by the hook probe. A counter file tracks calls. + installGitHook(t, repoDir, "pre-commit", `#!/bin/sh +COUNT_FILE=".git/hook-count" +COUNT=0 +if [ -f "$COUNT_FILE" ]; then + COUNT=$(cat "$COUNT_FILE") +fi +COUNT=$((COUNT + 1)) +echo "$COUNT" > "$COUNT_FILE" +if [ "$COUNT" -le 2 ]; then + echo "lint error: trailing whitespace" >&2 + exit 1 +fi +exit 0 +`) + + // Make a file change to commit + if err := os.WriteFile(filepath.Join(repoDir, "new.txt"), []byte("hello"), 0644); err != nil { + t.Fatal(err) + } + + testAgent := agent.NewTestAgent() + sha, err := commitWithHookRetry(repoDir, "test commit", testAgent, true) + if err != nil { + t.Fatalf("commitWithHookRetry should succeed: %v", err) + } + + if sha == "" { + t.Fatal("expected non-empty SHA") + } + + // Verify the commit exists + commitSHA := runGit("rev-parse", "HEAD") + if commitSHA != sha { + t.Errorf("expected HEAD=%s, got %s", sha, commitSHA) + } +} + +func TestCommitWithHookRetryExhausted(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + repoDir, _, _ := setupTestGitRepo(t) + + installGitHook(t, repoDir, "pre-commit", + "#!/bin/sh\necho 'always fails' >&2\nexit 1\n") + + // Make a file change + if err := os.WriteFile(filepath.Join(repoDir, "new.txt"), []byte("hello"), 0644); err != nil { + t.Fatal(err) + } + + testAgent := agent.NewTestAgent() + _, err := commitWithHookRetry(repoDir, "test commit", testAgent, true) + if err == nil { + t.Fatal("expected error after exhausting retries") + } + if !strings.Contains(err.Error(), "after 3 attempts") { + t.Errorf("expected error mentioning '3 attempts', got: %v", err) + } +} + +func TestCommitWithHookRetrySkipsNonHookError(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + repoDir, _, _ := setupTestGitRepo(t) + + // No pre-commit hook installed. Commit with no changes will fail + // for a non-hook reason ("nothing to commit"). + testAgent := agent.NewTestAgent() + _, err := commitWithHookRetry(repoDir, "empty commit", testAgent, true) + if err == nil { + t.Fatal("expected error for empty commit without hook") + } + + // Should return the raw git error, not a hook-retry error + if strings.Contains(err.Error(), "pre-commit hook failed") { + t.Errorf("non-hook error should not be reported as hook failure, got: %v", err) + } + if strings.Contains(err.Error(), "after 3 attempts") { + t.Errorf("non-hook error should not trigger retries, got: %v", err) + } +} + +func TestCommitWithHookRetrySkipsAddPhaseError(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + repoDir, _, _ := setupTestGitRepo(t) + + installGitHook(t, repoDir, "pre-commit", "#!/bin/sh\nexit 0\n") + + // Make a change so there's something to commit + if err := os.WriteFile(filepath.Join(repoDir, "new.txt"), []byte("hello"), 0644); err != nil { + t.Fatal(err) + } + + // Create index.lock to make git add fail (non-hook failure) + lockFile := filepath.Join(repoDir, ".git", "index.lock") + if err := os.WriteFile(lockFile, []byte(""), 0644); err != nil { + t.Fatal(err) + } + defer os.Remove(lockFile) + + testAgent := agent.NewTestAgent() + _, err := commitWithHookRetry(repoDir, "test commit", testAgent, true) + if err == nil { + t.Fatal("expected error with index.lock present") + } + + // Should NOT retry despite hook being present (add-phase failure) + if strings.Contains(err.Error(), "pre-commit hook failed") { + t.Errorf("add-phase error should not be reported as hook failure, got: %v", err) + } + if strings.Contains(err.Error(), "after 3 attempts") { + t.Errorf("add-phase error should not trigger retries, got: %v", err) + } +} + +func TestCommitWithHookRetrySkipsCommitPhaseNonHookError(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not available") + } + + repoDir, _, _ := setupTestGitRepo(t) + + installGitHook(t, repoDir, "pre-commit", "#!/bin/sh\nexit 0\n") + + // No changes to commit — "nothing to commit" is a commit-phase + // failure, but the hook passes, so HookFailed should be false. + testAgent := agent.NewTestAgent() + _, err := commitWithHookRetry(repoDir, "empty commit", testAgent, true) + if err == nil { + t.Fatal("expected error for empty commit") + } + + // Should NOT retry despite hook being present (hook is passing) + if strings.Contains(err.Error(), "pre-commit hook failed") { + t.Errorf("non-hook commit error should not be reported as hook failure, got: %v", err) + } + if strings.Contains(err.Error(), "after 3 attempts") { + t.Errorf("non-hook commit error should not trigger retries, got: %v", err) + } +} + func TestResolveReasoningWithFast(t *testing.T) { tests := []struct { name string diff --git a/internal/git/git.go b/internal/git/git.go index d93b05bd..e91d6b2e 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -686,6 +686,20 @@ func GetHooksPath(repoPath string) (string, error) { return hooksPath, nil } +// isHookCausingFailure checks whether a commit failure was caused +// by a hook (pre-commit, commit-msg, etc.) by probing if a hookless +// commit would succeed. Runs "git commit --dry-run --no-verify" +// which validates the commit is viable (staged changes, identity) +// without executing any hooks. If the dry-run passes, a hook must +// have caused the failure. +func isHookCausingFailure(repoPath string) bool { + cmd := exec.Command( + "git", "-C", repoPath, "commit", + "--dry-run", "--no-verify", "-m", "probe", + ) + return cmd.Run() == nil +} + // GetDefaultBranch detects the default branch (from origin/HEAD, or main/master locally) func GetDefaultBranch(repoPath string) (string, error) { // Prefer origin/HEAD as the authoritative source for the default branch @@ -744,6 +758,27 @@ func GetCommitsSince(repoPath, mergeBase string) ([]string, error) { return GetRangeCommits(repoPath, rangeRef) } +// CommitError represents a failure during CreateCommit. +// Phase distinguishes "add" failures (lockfile, permissions) from +// "commit" failures (hooks, empty commit, identity issues). +// HookFailed is set by probing whether a hookless commit would +// succeed — true means a hook (pre-commit, commit-msg, etc.) +// caused the failure. +type CommitError struct { + Phase string // "add" or "commit" + HookFailed bool // true when a hook caused the failure + Stderr string + Err error +} + +func (e *CommitError) Error() string { + return fmt.Sprintf("git %s: %v: %s", e.Phase, e.Err, e.Stderr) +} + +func (e *CommitError) Unwrap() error { + return e.Err +} + // CreateCommit stages all changes and creates a commit with the given message // Returns the SHA of the new commit func CreateCommit(repoPath, message string) (string, error) { @@ -753,7 +788,9 @@ func CreateCommit(repoPath, message string) (string, error) { var stderr bytes.Buffer cmd.Stderr = &stderr if err := cmd.Run(); err != nil { - return "", fmt.Errorf("git add: %w: %s", err, stderr.String()) + return "", &CommitError{ + Phase: "add", Stderr: stderr.String(), Err: err, + } } // Create commit @@ -762,7 +799,12 @@ func CreateCommit(repoPath, message string) (string, error) { stderr.Reset() cmd.Stderr = &stderr if err := cmd.Run(); err != nil { - return "", fmt.Errorf("git commit: %w: %s", err, stderr.String()) + return "", &CommitError{ + Phase: "commit", + HookFailed: isHookCausingFailure(repoPath), + Stderr: stderr.String(), + Err: err, + } } // Get the SHA of the new commit diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 61cbc9bb..b87e2661 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -1,6 +1,7 @@ package git import ( + "errors" "os" "os/exec" "path/filepath" @@ -99,6 +100,20 @@ func (r *TestRepo) AddWorktree(branchName string) *TestRepo { return &TestRepo{T: r.T, Dir: wtDir} } +// InstallHook writes a shell script as the named git hook +// (e.g. "pre-commit") and makes it executable. +func (r *TestRepo) InstallHook(name, script string) { + r.T.Helper() + hooksDir := filepath.Join(r.Dir, ".git", "hooks") + if err := os.MkdirAll(hooksDir, 0755); err != nil { + r.T.Fatal(err) + } + hookPath := filepath.Join(hooksDir, name) + if err := os.WriteFile(hookPath, []byte(script), 0755); err != nil { + r.T.Fatal(err) + } +} + func runGit(t *testing.T, dir string, args ...string) string { t.Helper() cmd := exec.Command("git", args...) @@ -1266,6 +1281,93 @@ func TestGetRangeFilesChanged(t *testing.T) { }) } +func TestCreateCommitPreCommitHookOutput(t *testing.T) { + repo := NewTestRepo(t) + repo.CommitFile("initial.txt", "initial", "initial commit") + + repo.InstallHook("pre-commit", + "#!/bin/sh\necho 'error: trailing whitespace on line 42' >&2\nexit 1\n") + + // Make a change so there's something to commit + repo.WriteFile("new.txt", "content") + repo.Run("add", "new.txt") + + _, err := CreateCommit(repo.Dir, "should fail") + if err == nil { + t.Fatal("expected CreateCommit to fail with pre-commit hook") + } + + // The error should contain the hook's stderr output + if !strings.Contains(err.Error(), "trailing whitespace on line 42") { + t.Errorf( + "expected error to contain hook output, got: %v", err, + ) + } + + // HookFailed should be true since the hook caused the failure + var commitErr *CommitError + if !errors.As(err, &commitErr) { + t.Fatal("expected CommitError type") + } + if !commitErr.HookFailed { + t.Error("expected HookFailed=true for pre-commit hook rejection") + } +} + +func TestCommitErrorHookFailedFalseWhenNothingToCommit(t *testing.T) { + repo := NewTestRepo(t) + repo.CommitFile("initial.txt", "initial", "initial commit") + + // Install a passing pre-commit hook. The commit should still fail + // because there are no staged changes ("nothing to commit"). + // The dry-run probe (--no-verify --dry-run) also fails for the + // same reason, so HookFailed must be false. + repo.InstallHook("pre-commit", "#!/bin/sh\nexit 0\n") + + // No staged changes — commit fails for non-hook reason + _, err := CreateCommit(repo.Dir, "empty commit") + if err == nil { + t.Fatal("expected CreateCommit to fail") + } + + var commitErr *CommitError + if !errors.As(err, &commitErr) { + t.Fatal("expected CommitError type") + } + + // Dry-run without hooks also fails, so HookFailed must be false + if commitErr.HookFailed { + t.Error("HookFailed should be false when commit fails for non-hook reasons") + } +} + +func TestCommitErrorHookFailedCommitMsgHook(t *testing.T) { + repo := NewTestRepo(t) + repo.CommitFile("initial.txt", "initial", "initial commit") + + // Install a commit-msg hook that rejects. The dry-run probe + // bypasses all hooks (--no-verify), so it should succeed and + // HookFailed should be true. + repo.InstallHook("commit-msg", + "#!/bin/sh\necho 'bad commit message format' >&2\nexit 1\n") + + repo.WriteFile("new.txt", "content") + repo.Run("add", "new.txt") + + _, err := CreateCommit(repo.Dir, "should fail") + if err == nil { + t.Fatal("expected CreateCommit to fail with commit-msg hook") + } + + var commitErr *CommitError + if !errors.As(err, &commitErr) { + t.Fatal("expected CommitError type") + } + if !commitErr.HookFailed { + t.Error("expected HookFailed=true for commit-msg hook rejection") + } +} + func TestIsAncestor(t *testing.T) { repo := NewTestRepo(t) repo.Run("symbolic-ref", "HEAD", "refs/heads/main") diff --git a/scripts/changelog.sh b/scripts/changelog.sh index 6565c288..4c3c63fb 100755 --- a/scripts/changelog.sh +++ b/scripts/changelog.sh @@ -45,7 +45,8 @@ echo "Using $AGENT to generate changelog..." >&2 TMPFILE=$(mktemp) PROMPTFILE=$(mktemp) -trap 'rm -f "$TMPFILE" "$PROMPTFILE"' EXIT +ERRFILE=$(mktemp) +trap 'rm -f "$TMPFILE" "$PROMPTFILE" "$ERRFILE"' EXIT cat > "$PROMPTFILE" </dev/null < "$PROMPTFILE" + CODEX_RUST_LOG="${CHANGELOG_CODEX_RUST_LOG:-${RUST_LOG:-error,codex_core::rollout::list=off}}" + set +e + RUST_LOG="$CODEX_RUST_LOG" codex exec --json --skip-git-repo-check --sandbox read-only -c reasoning_effort=high -o "$TMPFILE" - >/dev/null < "$PROMPTFILE" 2>"$ERRFILE" + AGENT_EXIT=$? + set -e ;; claude) - claude --print < "$PROMPTFILE" > "$TMPFILE" + set +e + claude --print < "$PROMPTFILE" > "$TMPFILE" 2>"$ERRFILE" + AGENT_EXIT=$? + set -e ;; *) echo "Error: unknown CHANGELOG_AGENT '$AGENT' (expected 'codex' or 'claude')" >&2 @@ -88,4 +97,23 @@ case "$AGENT" in ;; esac +if [ "$AGENT_EXIT" -ne 0 ] || [ ! -s "$TMPFILE" ]; then + echo "Error: $AGENT failed to generate changelog." >&2 + if [ "${CHANGELOG_DEBUG:-0}" = "1" ]; then + cat "$ERRFILE" >&2 + else + FILTERED_ERR=$(grep -E -v 'rollout path for thread|failed to record rollout items: failed to queue rollout items: channel closed|^mcp startup: no servers$|^WARNING: proceeding, even though we could not update PATH:' "$ERRFILE" || true) + if [ -n "$FILTERED_ERR" ]; then + echo "$FILTERED_ERR" >&2 + else + echo "Set CHANGELOG_DEBUG=1 to print full agent logs." >&2 + fi + fi + exit 1 +fi + +if [ "${CHANGELOG_DEBUG:-0}" = "1" ] && [ -s "$ERRFILE" ]; then + cat "$ERRFILE" >&2 +fi + cat "$TMPFILE"