diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 1222e36..be173f8 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1092,12 +1092,15 @@ func statusCmd() *cobra.Command { json.NewDecoder(healthResp.Body).Decode(&health) } - // Display daemon info with uptime + // Display daemon info with uptime and version + daemonLine := "Daemon: running" if health.Uptime != "" { - fmt.Printf("Daemon: running (uptime: %s)\n", health.Uptime) - } else { - fmt.Println("Daemon: running") + daemonLine += fmt.Sprintf(" (uptime: %s)", health.Uptime) + } + if status.Version != "" { + daemonLine += fmt.Sprintf(" [%s]", status.Version) } + fmt.Println(daemonLine) fmt.Printf("Workers: %d/%d active\n", status.ActiveWorkers, status.MaxWorkers) fmt.Printf("Jobs: %d queued, %d running, %d completed, %d failed\n", status.QueuedJobs, status.RunningJobs, status.CompletedJobs, status.FailedJobs) diff --git a/internal/config/config.go b/internal/config/config.go index 23b845c..90bd6fb 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -12,6 +12,13 @@ import ( "github.com/roborev-dev/roborev/internal/git" ) +// HookConfig defines a hook that runs on review events +type HookConfig struct { + Event string `toml:"event"` // "review.failed", "review.completed", "review.*" + Command string `toml:"command"` // shell command with {var} templates + Type string `toml:"type"` // "beads" for built-in, empty for command +} + // Config holds the daemon configuration type Config struct { ServerAddr string `toml:"server_addr"` @@ -56,6 +63,9 @@ type Config struct { // API keys (optional - agents use subscription auth by default) AnthropicAPIKey string `toml:"anthropic_api_key"` + // Hooks configuration + Hooks []HookConfig `toml:"hooks"` + // Sync configuration for PostgreSQL Sync SyncConfig `toml:"sync"` @@ -176,6 +186,9 @@ type RepoConfig struct { FixModelStandard string `toml:"fix_model_standard"` FixModelThorough string `toml:"fix_model_thorough"` + // Hooks configuration (per-repo) + Hooks []HookConfig `toml:"hooks"` + // Analysis settings MaxPromptSize int `toml:"max_prompt_size"` // Max prompt size in bytes before falling back to paths (overrides global default) } diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go new file mode 100644 index 0000000..e3004f4 --- /dev/null +++ b/internal/daemon/hooks.go @@ -0,0 +1,213 @@ +package daemon + +import ( + "fmt" + "log" + "os/exec" + "path/filepath" + "runtime" + "strings" + "sync" + + "github.com/roborev-dev/roborev/internal/config" +) + +// HookRunner listens for broadcaster events and runs configured hooks. +type HookRunner struct { + cfgGetter ConfigGetter + broadcaster Broadcaster + subID int + mu sync.RWMutex + stopCh chan struct{} +} + +// NewHookRunner creates a new HookRunner that subscribes to events from the broadcaster. +func NewHookRunner(cfgGetter ConfigGetter, broadcaster Broadcaster) *HookRunner { + subID, eventCh := broadcaster.Subscribe("") + + hr := &HookRunner{ + cfgGetter: cfgGetter, + broadcaster: broadcaster, + subID: subID, + stopCh: make(chan struct{}), + } + + go hr.listen(eventCh) + + return hr +} + +// listen processes events from the broadcaster and fires matching hooks. +func (hr *HookRunner) listen(eventCh <-chan Event) { + for { + select { + case <-hr.stopCh: + return + case event, ok := <-eventCh: + if !ok { + return + } + hr.handleEvent(event) + } + } +} + +// Stop shuts down the hook runner and unsubscribes from the broadcaster. +func (hr *HookRunner) Stop() { + close(hr.stopCh) + hr.broadcaster.Unsubscribe(hr.subID) +} + +// handleEvent checks all configured hooks against the event and fires matches. +func (hr *HookRunner) handleEvent(event Event) { + // Only handle review events + if !strings.HasPrefix(event.Type, "review.") { + return + } + + cfg := hr.cfgGetter.Config() + if cfg == nil { + return + } + + // Collect hooks: copy global slice to avoid aliasing, then append repo-specific + hooks := append([]config.HookConfig{}, cfg.Hooks...) + + if event.Repo != "" { + if repoCfg, err := config.LoadRepoConfig(event.Repo); err == nil && repoCfg != nil { + hooks = append(hooks, repoCfg.Hooks...) + } + } + + fired := 0 + for _, hook := range hooks { + if !matchEvent(hook.Event, event.Type) { + continue + } + + cmd := resolveCommand(hook, event) + if cmd == "" { + continue + } + + fired++ + // Run async so hooks don't block workers + go runHook(cmd, event.Repo) + } + + if fired > 0 { + log.Printf("Hooks: fired %d hook(s) for %s (job %d)", fired, event.Type, event.JobID) + } +} + +// matchEvent checks if an event type matches a hook's event pattern. +// Supports exact match and "review.*" wildcard. +func matchEvent(pattern, eventType string) bool { + if pattern == eventType { + return true + } + // Support wildcard like "review.*" + if strings.HasSuffix(pattern, ".*") { + prefix := strings.TrimSuffix(pattern, ".*") + return strings.HasPrefix(eventType, prefix+".") + } + return false +} + +// resolveCommand builds the shell command for a hook, handling built-in types +// and template variable interpolation. +func resolveCommand(hook config.HookConfig, event Event) string { + if hook.Type == "beads" { + return beadsCommand(event) + } + return interpolate(hook.Command, event) +} + +// beadsCommand generates a bd create command for the beads built-in hook. +func beadsCommand(event Event) string { + repoName := event.RepoName + if repoName == "" { + repoName = filepath.Base(event.Repo) + } + + shortSHA := event.SHA + if len(shortSHA) > 8 { + shortSHA = shortSHA[:8] + } + + switch event.Type { + case "review.failed": + title := fmt.Sprintf("Review failed for %s (%s): run roborev show %d", repoName, shortSHA, event.JobID) + return fmt.Sprintf("bd create %q -p 1", title) + case "review.completed": + if event.Verdict == "F" { + title := fmt.Sprintf("Review findings for %s (%s): roborev show %d / one-shot fix with roborev fix %d", repoName, shortSHA, event.JobID, event.JobID) + return fmt.Sprintf("bd create %q -p 2", title) + } + return "" // No issue for passing reviews + default: + return "" + } +} + +// interpolate replaces {var} template variables in a command string. +// Values are shell-escaped to prevent injection via event fields. +func interpolate(cmd string, event Event) string { + if cmd == "" { + return "" + } + + r := strings.NewReplacer( + "{job_id}", fmt.Sprintf("%d", event.JobID), + "{repo}", shellEscape(event.Repo), + "{repo_name}", shellEscape(event.RepoName), + "{sha}", shellEscape(event.SHA), + "{agent}", shellEscape(event.Agent), + "{verdict}", shellEscape(event.Verdict), + "{error}", shellEscape(event.Error), + ) + return r.Replace(cmd) +} + +// shellEscape quotes a value for safe interpolation into a shell command. +// On Unix, wraps in single quotes with embedded single quotes escaped. +// On Windows, wraps in double quotes with embedded double quotes escaped. +func shellEscape(s string) string { + if runtime.GOOS == "windows" { + // PowerShell single-quoted strings: only escape is '' for literal '. + if s == "" { + return "''" + } + return "'" + strings.ReplaceAll(s, "'", "''") + "'" + } + if s == "" { + return "''" + } + return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'" +} + +// runHook executes a shell command in the given working directory. +// Errors are logged but never propagated. +func runHook(command, workDir string) { + var cmd *exec.Cmd + if runtime.GOOS == "windows" { + // Use PowerShell for reliable path handling and command execution. + // -NoProfile avoids loading user profiles that could slow or alter execution. + // -Command takes the rest as a PowerShell script string. + cmd = exec.Command("powershell", "-NoProfile", "-Command", command) + } else { + cmd = exec.Command("sh", "-c", command) + } + if workDir != "" { + cmd.Dir = workDir + } + + output, err := cmd.CombinedOutput() + if err != nil { + log.Printf("Hook error (cmd=%q dir=%q): %v\n%s", command, workDir, err, output) + return + } + if len(output) > 0 { + log.Printf("Hook output (cmd=%q): %s", command, output) + } +} diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go new file mode 100644 index 0000000..fd0d374 --- /dev/null +++ b/internal/daemon/hooks_test.go @@ -0,0 +1,747 @@ +package daemon + +import ( + "bytes" + "log" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + "time" + + "github.com/roborev-dev/roborev/internal/config" +) + +// q wraps a string in platform-appropriate shell quoting (matches shellEscape output). +func q(s string) string { + return shellEscape(s) +} + +// noopCmd returns a platform-appropriate no-op shell command. +func noopCmd() string { + if runtime.GOOS == "windows" { + return "Write-Output ok" + } + return "true" +} + +// touchCmd returns a platform-appropriate shell command to create a file. +// Uses forward slashes on Windows to avoid TOML/shell escaping issues. +func touchCmd(path string) string { + if runtime.GOOS == "windows" { + // runHook uses PowerShell on Windows, so use PowerShell commands directly. + // Use forward slashes — PowerShell resolves them correctly. + return "New-Item -ItemType File -Force -Path '" + filepath.ToSlash(path) + "'" + } + return "touch " + path +} + +// pwdCmd returns a platform-appropriate shell command to write the cwd to a file. +func pwdCmd(path string) string { + if runtime.GOOS == "windows" { + return "(Get-Location).Path | Set-Content -NoNewline '" + filepath.ToSlash(path) + "'" + } + return "pwd > " + path +} + +func TestMatchEvent(t *testing.T) { + tests := []struct { + pattern string + eventType string + want bool + }{ + {"review.failed", "review.failed", true}, + {"review.completed", "review.completed", true}, + {"review.failed", "review.completed", false}, + {"review.*", "review.failed", true}, + {"review.*", "review.completed", true}, + {"review.*", "review.started", true}, + {"review.*", "other.event", false}, + {"other.*", "review.failed", false}, + } + + for _, tt := range tests { + got := matchEvent(tt.pattern, tt.eventType) + if got != tt.want { + t.Errorf("matchEvent(%q, %q) = %v, want %v", tt.pattern, tt.eventType, got, tt.want) + } + } +} + +func TestInterpolate(t *testing.T) { + event := Event{ + JobID: 42, + Repo: "/home/user/myrepo", + RepoName: "myrepo", + SHA: "abc123def456", + Agent: "codex", + Verdict: "F", + Error: "agent timeout", + } + + tests := []struct { + cmd string + want string + }{ + { + "echo {job_id} {sha}", + "echo 42 " + q("abc123def456"), + }, + { + "notify --repo {repo_name} --verdict {verdict}", + "notify --repo " + q("myrepo") + " --verdict " + q("F"), + }, + { + "log {error}", + "log " + q("agent timeout"), + }, + { + "", + "", + }, + } + + for _, tt := range tests { + got := interpolate(tt.cmd, event) + if got != tt.want { + t.Errorf("interpolate(%q) = %q, want %q", tt.cmd, got, tt.want) + } + } +} + +func TestInterpolateShellInjection(t *testing.T) { + // Test with payloads that attempt to break out of quoting and execute commands. + // We assert safety properties rather than exact output to avoid testing + // shellEscape against itself. + payloads := []string{ + "'; rm -rf / #", + "`rm -rf /`", + "$(rm -rf /)", + "a; echo pwned", + "a & echo pwned", + "a | cat /etc/passwd", + } + + for _, payload := range payloads { + event := Event{JobID: 1, Repo: "/repo", Error: payload} + got := interpolate("echo {error}", event) + + prefix := "echo " + if !strings.HasPrefix(got, prefix) || len(got) <= len(prefix)+1 { + t.Fatalf("payload %q: unexpected format (too short or wrong prefix): %q", payload, got) + } + val := got[len(prefix):] + + // The value must be fully enclosed in single quotes on all platforms. + if val[0] != '\'' || val[len(val)-1] != '\'' { + t.Errorf("payload %q: not single-quoted: %q", payload, got) + } + + // The payload content must be present inside the quoted region (not dropped). + // Use an unambiguous substring that survives escaping. + substr := payload[:4] // first 4 chars are always safe to check + if !strings.Contains(val, substr) { + t.Errorf("payload %q: escaped value doesn't contain expected substring %q: %q", payload, substr, val) + } + } +} + +func TestInterpolateQuotedPlaceholders(t *testing.T) { + // Placeholders are auto-escaped with single quotes, so users should NOT + // wrap them in additional quotes. This test documents the behavior: + // double-quoting a placeholder produces nested quotes which is valid shell + // but includes the literal single quotes inside the double-quoted string. + event := Event{ + JobID: 1, + Repo: "/repo", + RepoName: "myrepo", + Error: "simple error", + } + + // Unquoted placeholder (recommended) -- clean output + got := interpolate("echo {error}", event) + if want := "echo " + q("simple error"); got != want { + t.Errorf("unquoted placeholder: got %q, want %q", got, want) + } + + // Double-quoted placeholder -- works but includes literal quotes around the value + got = interpolate(`echo "{error}"`, event) + if want := `echo "` + q("simple error") + `"`; got != want { + t.Errorf("double-quoted placeholder: got %q, want %q", got, want) + } + + // Empty value produces empty quoted string + event.Verdict = "" + got = interpolate("echo {verdict}", event) + if want := "echo " + q(""); got != want { + t.Errorf("empty value: got %q, want %q", got, want) + } +} + +func TestShellEscape(t *testing.T) { + var tests []struct { + in string + want string + } + if runtime.GOOS == "windows" { + tests = []struct { + in string + want string + }{ + {"hello", "'hello'"}, + {"", "''"}, + {"it's", "'it''s'"}, + {"a;b", "'a;b'"}, + {`say "hi"`, `'say "hi"'`}, + {"%PATH%", "'%PATH%'"}, + } + } else { + tests = []struct { + in string + want string + }{ + {"hello", "'hello'"}, + {"", "''"}, + {"it's", "'it'\"'\"'s'"}, + {"a;b", "'a;b'"}, + } + } + for _, tt := range tests { + got := shellEscape(tt.in) + if got != tt.want { + t.Errorf("shellEscape(%q) = %q, want %q", tt.in, got, tt.want) + } + } + + // Verify injection payloads are properly enclosed in quotes on all platforms + injections := []string{ + "'; rm -rf /", + "`rm -rf /`", + "$(cat /etc/passwd)", + "a; echo pwned", + } + for _, payload := range injections { + got := shellEscape(payload) + if len(got) < 2 { + t.Fatalf("shellEscape(%q) too short: %q", payload, got) + } + if got[0] != '\'' || got[len(got)-1] != '\'' { + t.Errorf("shellEscape(%q) not single-quoted: %q", payload, got) + } + } +} + +func TestBeadsCommand(t *testing.T) { + event := Event{ + Type: "review.failed", + JobID: 7, + Repo: "/home/user/myrepo", + RepoName: "myrepo", + SHA: "abc123def456", + Agent: "codex", + Error: "timeout", + } + + cmd := beadsCommand(event) + if cmd == "" { + t.Fatal("expected non-empty command for review.failed") + } + if !contains(cmd, "bd create") { + t.Errorf("expected bd create in command, got %q", cmd) + } + if !contains(cmd, "roborev show 7") { + t.Errorf("expected 'roborev show 7' in command, got %q", cmd) + } + + // Completed with pass should return empty + event.Type = "review.completed" + event.Verdict = "P" + cmd = beadsCommand(event) + if cmd != "" { + t.Errorf("expected empty command for passing review, got %q", cmd) + } + + // Completed with fail should return a command + event.Verdict = "F" + cmd = beadsCommand(event) + if cmd == "" { + t.Fatal("expected non-empty command for failing review") + } + if !contains(cmd, "-p 2") { + t.Errorf("expected priority 2 for failing review, got %q", cmd) + } + if !contains(cmd, "roborev fix 7") { + t.Errorf("expected 'roborev fix' hint in failing review command, got %q", cmd) + } +} + +func TestBeadsCommandShortSHA(t *testing.T) { + event := Event{ + Type: "review.failed", + JobID: 1, + Repo: "/repo", + RepoName: "repo", + SHA: "abcdef1234567890", + } + cmd := beadsCommand(event) + if !contains(cmd, "abcdef12") { + t.Errorf("expected truncated SHA in command, got %q", cmd) + } + if contains(cmd, "abcdef1234567890") { + t.Errorf("expected SHA to be truncated, got %q", cmd) + } +} + +func TestResolveCommand(t *testing.T) { + event := Event{ + Type: "review.failed", + JobID: 5, + Repo: "/repo", + SHA: "abc123", + Agent: "codex", + } + + // Custom command + hook := config.HookConfig{ + Event: "review.failed", + Command: "echo {job_id}", + } + cmd := resolveCommand(hook, event) + if cmd != "echo 5" { // job_id is numeric, not shell-escaped + t.Errorf("expected 'echo 5', got %q", cmd) + } + + // Beads type + hook = config.HookConfig{ + Event: "review.failed", + Type: "beads", + } + cmd = resolveCommand(hook, event) + if cmd == "" { + t.Error("expected non-empty beads command") + } +} + +func TestHookRunnerFiresHooks(t *testing.T) { + + tmpDir := t.TempDir() + markerFile := filepath.Join(tmpDir, "hook-fired") + + cfg := &config.Config{ + Hooks: []config.HookConfig{ + { + Event: "review.completed", + Command: touchCmd(markerFile), + }, + }, + } + + broadcaster := NewBroadcaster() + hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) + defer hr.Stop() + + broadcaster.Broadcast(Event{ + Type: "review.completed", + TS: time.Now(), + JobID: 1, + Repo: tmpDir, + RepoName: "test", + SHA: "abc123", + Agent: "test", + Verdict: "P", + }) + + // Wait for async hook execution + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(markerFile); err == nil { + return // success + } + time.Sleep(50 * time.Millisecond) + } + t.Fatal("hook did not fire within timeout") +} + +func TestHookRunnerWorkingDirectory(t *testing.T) { + + tmpDir := t.TempDir() + markerFile := filepath.Join(tmpDir, "pwd-test") + + cfg := &config.Config{ + Hooks: []config.HookConfig{ + { + Event: "review.failed", + Command: pwdCmd(markerFile), + }, + }, + } + + broadcaster := NewBroadcaster() + hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) + defer hr.Stop() + + broadcaster.Broadcast(Event{ + Type: "review.failed", + TS: time.Now(), + JobID: 1, + Repo: tmpDir, + RepoName: "test", + SHA: "abc", + Agent: "test", + Error: "fail", + }) + + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + data, err := os.ReadFile(markerFile) + if err == nil { + got := filepath.Clean(strings.TrimSpace(string(data))) + want := filepath.Clean(tmpDir) + // On Windows, resolve 8.3 short paths to long paths for comparison + if runtime.GOOS == "windows" { + if g, err := filepath.EvalSymlinks(got); err == nil { + got = g + } + if w, err := filepath.EvalSymlinks(want); err == nil { + want = w + } + } + if !strings.EqualFold(got, want) { + t.Errorf("hook ran in %q, want %q", got, want) + } + return + } + time.Sleep(50 * time.Millisecond) + } + t.Fatal("hook did not fire within timeout") +} + +func TestHookRunnerNoMatchDoesNotFire(t *testing.T) { + + tmpDir := t.TempDir() + markerFile := filepath.Join(tmpDir, "should-not-exist") + + cfg := &config.Config{ + Hooks: []config.HookConfig{ + { + Event: "review.completed", + Command: touchCmd(markerFile), + }, + }, + } + + broadcaster := NewBroadcaster() + hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) + defer hr.Stop() + + // Send a failed event - hook is only for completed + broadcaster.Broadcast(Event{ + Type: "review.failed", + TS: time.Now(), + JobID: 1, + Repo: tmpDir, + RepoName: "test", + SHA: "abc", + Agent: "test", + Error: "fail", + }) + + assertFileNotCreated(t, markerFile, 500*time.Millisecond, "hook should not have fired for non-matching event") +} + +func TestHooksSliceNotAliased(t *testing.T) { + + // Verify that repo hooks don't leak into the global config's Hooks slice + tmpDir := t.TempDir() + markerGlobal := filepath.Join(tmpDir, "global-fired") + markerRepo := filepath.Join(tmpDir, "repo-fired") + + globalHooks := []config.HookConfig{ + {Event: "review.failed", Command: touchCmd(markerGlobal)}, + } + cfg := &config.Config{ + Hooks: globalHooks, + } + + // Write a repo config with an additional hook + repoDir := t.TempDir() + writeRepoConfig(t, repoDir, ` +[[hooks]] +event = "review.failed" +command = "`+touchCmd(markerRepo)+`" +`) + + broadcaster := NewBroadcaster() + hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) + defer hr.Stop() + + // Fire event for the repo + broadcaster.Broadcast(Event{ + Type: "review.failed", + TS: time.Now(), + JobID: 1, + Repo: repoDir, + SHA: "abc", + Agent: "test", + Error: "fail", + }) + + // Wait for hooks to run + time.Sleep(1 * time.Second) + + // The global config's Hooks slice must still have exactly 1 element + if len(cfg.Hooks) != 1 { + t.Errorf("global Hooks slice was mutated: len=%d, want 1", len(cfg.Hooks)) + } +} + +func TestHookRunnerGlobalAndRepoHooksBothFire(t *testing.T) { + + // Both global and per-repo hooks should fire for the same event + globalDir := t.TempDir() + repoDir := t.TempDir() + globalMarker := filepath.Join(globalDir, "global-fired") + repoMarker := filepath.Join(repoDir, "repo-fired") + + cfg := &config.Config{ + Hooks: []config.HookConfig{ + {Event: "review.failed", Command: touchCmd(globalMarker)}, + }, + } + + // Write repo config with its own hook + writeRepoConfig(t, repoDir, ` +[[hooks]] +event = "review.failed" +command = "`+touchCmd(repoMarker)+`" +`) + + broadcaster := NewBroadcaster() + hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) + defer hr.Stop() + + broadcaster.Broadcast(Event{ + Type: "review.failed", + TS: time.Now(), + JobID: 1, + Repo: repoDir, + SHA: "abc", + Agent: "test", + Error: "fail", + }) + + deadline := time.Now().Add(5 * time.Second) + globalFired, repoFired := false, false + for time.Now().Before(deadline) { + if _, err := os.Stat(globalMarker); err == nil { + globalFired = true + } + if _, err := os.Stat(repoMarker); err == nil { + repoFired = true + } + if globalFired && repoFired { + return // success + } + time.Sleep(50 * time.Millisecond) + } + if !globalFired { + t.Error("global hook did not fire") + } + if !repoFired { + t.Error("repo hook did not fire") + } +} + +func TestHookRunnerRepoOnlyHooks(t *testing.T) { + + // Repo hooks fire even when there are no global hooks + repoDir := t.TempDir() + markerFile := filepath.Join(repoDir, "repo-only") + + cfg := &config.Config{} // no global hooks + + writeRepoConfig(t, repoDir, ` +[[hooks]] +event = "review.completed" +command = "`+touchCmd(markerFile)+`" +`) + + broadcaster := NewBroadcaster() + hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) + defer hr.Stop() + + broadcaster.Broadcast(Event{ + Type: "review.completed", + TS: time.Now(), + JobID: 1, + Repo: repoDir, + SHA: "abc", + Agent: "test", + Verdict: "P", + }) + + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + if _, err := os.Stat(markerFile); err == nil { + return + } + time.Sleep(50 * time.Millisecond) + } + t.Fatal("repo-only hook did not fire") +} + +func TestHookRunnerRepoHookDoesNotFireForOtherRepo(t *testing.T) { + + // A repo's hooks should not fire for events from a different repo + repoA := t.TempDir() + repoB := t.TempDir() + markerFile := filepath.Join(repoA, "should-not-exist") + + cfg := &config.Config{} // no global hooks + + // Only repoA has hooks + writeRepoConfig(t, repoA, ` +[[hooks]] +event = "review.failed" +command = "`+touchCmd(markerFile)+`" +`) + + broadcaster := NewBroadcaster() + hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) + defer hr.Stop() + + // Fire event for repoB -- repoA's hooks should NOT fire + broadcaster.Broadcast(Event{ + Type: "review.failed", + TS: time.Now(), + JobID: 1, + Repo: repoB, + SHA: "abc", + Agent: "test", + Error: "fail", + }) + + assertFileNotCreated(t, markerFile, 500*time.Millisecond, "repo hook fired for a different repo's event") +} + +func TestHookRunnerStopUnsubscribes(t *testing.T) { + broadcaster := NewBroadcaster() + cfg := &config.Config{} + + before := broadcaster.SubscriberCount() + hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) + afterSub := broadcaster.SubscriberCount() + if afterSub != before+1 { + t.Errorf("expected subscriber count %d after NewHookRunner, got %d", before+1, afterSub) + } + + hr.Stop() + // Give the goroutine a moment to exit + time.Sleep(100 * time.Millisecond) + + afterStop := broadcaster.SubscriberCount() + if afterStop != before { + t.Errorf("expected subscriber count %d after Stop, got %d", before, afterStop) + } +} + +// writeRepoConfig writes a .roborev.toml file into repoDir, failing the test on error. +func writeRepoConfig(t *testing.T, repoDir, content string) { + t.Helper() + if err := os.WriteFile(filepath.Join(repoDir, ".roborev.toml"), []byte(content), 0644); err != nil { + t.Fatalf("failed to write .roborev.toml: %v", err) + } +} + +// assertFileNotCreated polls for the given duration and fails immediately if the file appears. +func assertFileNotCreated(t *testing.T, path string, duration time.Duration, msg string) { + t.Helper() + deadline := time.Now().Add(duration) + for time.Now().Before(deadline) { + if _, err := os.Stat(path); err == nil { + t.Fatal(msg) + } + time.Sleep(50 * time.Millisecond) + } +} + +func TestHandleEventLogsWhenHooksFired(t *testing.T) { + // Capture log output. The "fired N hook(s)" line is written synchronously + // by handleEvent before it spawns async goroutines for runHook. We capture + // the buffer, immediately restore the logger to avoid races with async + // goroutines, then check the captured output. + var buf bytes.Buffer + prevOut := log.Writer() + log.SetOutput(&buf) + + cfg := &config.Config{ + Hooks: []config.HookConfig{ + {Event: "review.completed", Command: noopCmd()}, + {Event: "review.completed", Command: noopCmd()}, + }, + } + + hr := &HookRunner{cfgGetter: NewStaticConfig(cfg)} + hr.handleEvent(Event{ + Type: "review.completed", + JobID: 42, + Repo: t.TempDir(), + SHA: "abc", + Verdict: "P", + }) + + // Restore immediately — async goroutines may still be running + log.SetOutput(prevOut) + + logOutput := buf.String() + if !strings.Contains(logOutput, "fired 2 hook(s)") { + t.Errorf("expected log to contain 'fired 2 hook(s)', got %q", logOutput) + } + if !strings.Contains(logOutput, "review.completed") { + t.Errorf("expected log to contain event type, got %q", logOutput) + } + if !strings.Contains(logOutput, "job 42") { + t.Errorf("expected log to contain job ID, got %q", logOutput) + } +} + +func TestHandleEventNoLogWhenNoHooksMatch(t *testing.T) { + var buf bytes.Buffer + prevOut := log.Writer() + log.SetOutput(&buf) + + cfg := &config.Config{ + Hooks: []config.HookConfig{ + {Event: "review.completed", Command: "echo test"}, + }, + } + + hr := &HookRunner{cfgGetter: NewStaticConfig(cfg)} + hr.handleEvent(Event{ + Type: "review.failed", + JobID: 99, + Repo: t.TempDir(), + SHA: "abc", + }) + + // No hooks matched, so no async goroutines were spawned — safe to read + log.SetOutput(prevOut) + + if strings.Contains(buf.String(), "fired") { + t.Errorf("expected no log output when no hooks match, got %q", buf.String()) + } +} + +func contains(s, substr string) bool { + return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsStr(s, substr)) +} + +func containsStr(s, substr string) bool { + for i := 0; i <= len(s)-len(substr); i++ { + if s[i:i+len(substr)] == substr { + return true + } + } + return false +} diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 8e9b47a..aaa8b5a 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -27,6 +27,7 @@ type Server struct { workerPool *WorkerPool httpServer *http.Server syncWorker *storage.SyncWorker + hookRunner *HookRunner errorLog *ErrorLog startTime time.Time @@ -51,11 +52,15 @@ func NewServer(db *storage.DB, cfg *config.Config, configPath string) *Server { // Create config watcher for hot-reloading configWatcher := NewConfigWatcher(configPath, cfg, broadcaster) + // Create hook runner to fire hooks on review events + hookRunner := NewHookRunner(configWatcher, broadcaster) + s := &Server{ db: db, configWatcher: configWatcher, broadcaster: broadcaster, workerPool: NewWorkerPool(db, configWatcher, cfg.MaxWorkers, broadcaster, errorLog), + hookRunner: hookRunner, errorLog: errorLog, startTime: time.Now(), } @@ -156,6 +161,11 @@ func (s *Server) Stop() error { // Stop worker pool s.workerPool.Stop() + // Stop hook runner + if s.hookRunner != nil { + s.hookRunner.Stop() + } + // Close error log if s.errorLog != nil { s.errorLog.Close() diff --git a/internal/storage/db_test.go b/internal/storage/db_test.go index 9f2737c..02b7f2f 100644 --- a/internal/storage/db_test.go +++ b/internal/storage/db_test.go @@ -568,9 +568,9 @@ func TestJobCounts(t *testing.T) { // Create a job, claim it, and complete it commit, _ := db.GetOrCreateCommit(repo.ID, "done1", "A", "S", time.Now()) job, _ := db.EnqueueJob(repo.ID, commit.ID, "done1", "", "codex", "", "") - _, _ = db.ClaimJob("w1") // Claims oldest queued job (one of queued0-2) - _, _ = db.ClaimJob("w1") // Claims next - _, _ = db.ClaimJob("w1") // Claims next + _, _ = db.ClaimJob("w1") // Claims oldest queued job (one of queued0-2) + _, _ = db.ClaimJob("w1") // Claims next + _, _ = db.ClaimJob("w1") // Claims next claimed, _ := db.ClaimJob("w1") // Should claim "done1" job now if claimed != nil { db.CompleteJob(claimed.ID, "codex", "p", "o") @@ -2518,6 +2518,52 @@ func setJobStatus(t *testing.T, db *DB, jobID int64, status JobStatus) { } } +func TestListJobsVerdictForBranchRangeReview(t *testing.T) { + // Regression test: branch range reviews (commit_id NULL, git_ref contains "..") + // should have their verdict parsed, not be misclassified as task jobs. + db := openTestDB(t) + defer db.Close() + + repo := createRepo(t, db, filepath.Join(t.TempDir(), "range-verdict-repo")) + + job, err := db.EnqueueRangeJob(repo.ID, "abc123..def456", "", "codex", "", "") + if err != nil { + t.Fatalf("EnqueueRangeJob failed: %v", err) + } + + // Claim and complete with findings output + _, err = db.ClaimJob("worker-0") + if err != nil { + t.Fatalf("ClaimJob failed: %v", err) + } + err = db.CompleteJob(job.ID, "codex", "review prompt", "- Medium — Bug in line 42\nSummary: found issues.") + if err != nil { + t.Fatalf("CompleteJob failed: %v", err) + } + + jobs, err := db.ListJobs("", "", 50, 0) + if err != nil { + t.Fatalf("ListJobs failed: %v", err) + } + + var found bool + for _, j := range jobs { + if j.ID == job.ID { + found = true + if j.Verdict == nil { + t.Fatal("expected verdict to be parsed for branch range review, got nil") + } + if *j.Verdict != "F" { + t.Errorf("expected verdict F for review with findings, got %q", *j.Verdict) + } + break + } + } + if !found { + t.Fatal("branch range job not found in ListJobs result") + } +} + // createJobChain creates a repo, commit, and enqueued job, returning all three. func createJobChain(t *testing.T, db *DB, repoPath, sha string) (*Repo, *Commit, *ReviewJob) { t.Helper() diff --git a/internal/storage/models.go b/internal/storage/models.go index 0b06e59..61a17b6 100644 --- a/internal/storage/models.go +++ b/internal/storage/models.go @@ -1,6 +1,9 @@ package storage -import "time" +import ( + "strings" + "time" +) type Repo struct { ID int64 `json:"id"` @@ -31,25 +34,25 @@ const ( ) type ReviewJob struct { - ID int64 `json:"id"` - RepoID int64 `json:"repo_id"` - CommitID *int64 `json:"commit_id,omitempty"` // nil for ranges - GitRef string `json:"git_ref"` // SHA or "start..end" for ranges - Branch string `json:"branch,omitempty"` // Branch name at time of job creation - Agent string `json:"agent"` - Model string `json:"model,omitempty"` // Model to use (for opencode: provider/model format) - Reasoning string `json:"reasoning,omitempty"` // thorough, standard, fast (default: thorough) - Status JobStatus `json:"status"` - EnqueuedAt time.Time `json:"enqueued_at"` - StartedAt *time.Time `json:"started_at,omitempty"` - FinishedAt *time.Time `json:"finished_at,omitempty"` - WorkerID string `json:"worker_id,omitempty"` - Error string `json:"error,omitempty"` - Prompt string `json:"prompt,omitempty"` - RetryCount int `json:"retry_count"` - DiffContent *string `json:"diff_content,omitempty"` // For dirty reviews (uncommitted changes) - Agentic bool `json:"agentic"` // Enable agentic mode (allow file edits) - OutputPrefix string `json:"output_prefix,omitempty"` // Prefix to prepend to review output + ID int64 `json:"id"` + RepoID int64 `json:"repo_id"` + CommitID *int64 `json:"commit_id,omitempty"` // nil for ranges + GitRef string `json:"git_ref"` // SHA or "start..end" for ranges + Branch string `json:"branch,omitempty"` // Branch name at time of job creation + Agent string `json:"agent"` + Model string `json:"model,omitempty"` // Model to use (for opencode: provider/model format) + Reasoning string `json:"reasoning,omitempty"` // thorough, standard, fast (default: thorough) + Status JobStatus `json:"status"` + EnqueuedAt time.Time `json:"enqueued_at"` + StartedAt *time.Time `json:"started_at,omitempty"` + FinishedAt *time.Time `json:"finished_at,omitempty"` + WorkerID string `json:"worker_id,omitempty"` + Error string `json:"error,omitempty"` + Prompt string `json:"prompt,omitempty"` + RetryCount int `json:"retry_count"` + DiffContent *string `json:"diff_content,omitempty"` // For dirty reviews (uncommitted changes) + Agentic bool `json:"agentic"` // Enable agentic mode (allow file edits) + OutputPrefix string `json:"output_prefix,omitempty"` // Prefix to prepend to review output // Sync fields UUID string `json:"uuid,omitempty"` // Globally unique identifier for sync @@ -77,6 +80,9 @@ func (j ReviewJob) IsTaskJob() bool { if j.GitRef == "dirty" { return false // It's a dirty review (even if DiffContent not loaded) } + if strings.Contains(j.GitRef, "..") { + return false // It's a branch/range review + } if j.GitRef == "" { return false // Invalid job state - not a valid task job } @@ -117,14 +123,14 @@ type Response struct { } type DaemonStatus struct { - Version string `json:"version"` - QueuedJobs int `json:"queued_jobs"` - RunningJobs int `json:"running_jobs"` - CompletedJobs int `json:"completed_jobs"` - FailedJobs int `json:"failed_jobs"` - CanceledJobs int `json:"canceled_jobs"` - ActiveWorkers int `json:"active_workers"` - MaxWorkers int `json:"max_workers"` + Version string `json:"version"` + QueuedJobs int `json:"queued_jobs"` + RunningJobs int `json:"running_jobs"` + CompletedJobs int `json:"completed_jobs"` + FailedJobs int `json:"failed_jobs"` + CanceledJobs int `json:"canceled_jobs"` + ActiveWorkers int `json:"active_workers"` + MaxWorkers int `json:"max_workers"` MachineID string `json:"machine_id,omitempty"` // Local machine ID for remote job detection ConfigReloadedAt string `json:"config_reloaded_at,omitempty"` // Last config reload timestamp (RFC3339Nano) ConfigReloadCounter uint64 `json:"config_reload_counter,omitempty"` // Monotonic reload counter (for sub-second detection) diff --git a/internal/storage/models_test.go b/internal/storage/models_test.go new file mode 100644 index 0000000..6689222 --- /dev/null +++ b/internal/storage/models_test.go @@ -0,0 +1,68 @@ +package storage + +import "testing" + +func TestIsTaskJob(t *testing.T) { + tests := []struct { + name string + job ReviewJob + want bool + }{ + { + name: "single commit review", + job: ReviewJob{CommitID: ptr(int64(1)), GitRef: "abc123"}, + want: false, + }, + { + name: "dirty review", + job: ReviewJob{GitRef: "dirty"}, + want: false, + }, + { + name: "dirty review with diff content", + job: ReviewJob{GitRef: "dirty", DiffContent: ptr("diff")}, + want: false, + }, + { + name: "branch range review", + job: ReviewJob{GitRef: "abc123..def456"}, + want: false, + }, + { + name: "triple-dot range review", + job: ReviewJob{GitRef: "main...feature"}, + want: false, + }, + { + name: "task job with label", + job: ReviewJob{GitRef: "run:lint"}, + want: true, + }, + { + name: "task job analyze", + job: ReviewJob{GitRef: "analyze"}, + want: true, + }, + { + name: "task job run", + job: ReviewJob{GitRef: "run"}, + want: true, + }, + { + name: "empty git ref", + job: ReviewJob{GitRef: ""}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.job.IsTaskJob() + if got != tt.want { + t.Errorf("IsTaskJob() = %v, want %v", got, tt.want) + } + }) + } +} + +func ptr[T any](v T) *T { return &v }