From ab3aca3ef4a95be3b15ca2dc3f57df52d6b481a5 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Fri, 30 Jan 2026 19:54:37 -0600 Subject: [PATCH 01/19] Add review hooks system for running commands on review events (#180) Configurable hooks fire shell commands when reviews complete or fail. Supports {var} template interpolation and a built-in "beads" type that auto-creates bd issues for failed reviews. Co-Authored-By: Claude Opus 4.5 --- internal/config/config.go | 13 ++ internal/daemon/hooks.go | 176 ++++++++++++++++++++ internal/daemon/hooks_test.go | 294 ++++++++++++++++++++++++++++++++++ internal/daemon/server.go | 10 ++ 4 files changed, 493 insertions(+) create mode 100644 internal/daemon/hooks.go create mode 100644 internal/daemon/hooks_test.go 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..cc0fb10 --- /dev/null +++ b/internal/daemon/hooks.go @@ -0,0 +1,176 @@ +package daemon + +import ( + "fmt" + "log" + "os/exec" + "path/filepath" + "strings" + "sync" + + "github.com/roborev-dev/roborev/internal/config" +) + +// HookRunner listens for broadcaster events and runs configured hooks. +type HookRunner struct { + cfgGetter ConfigGetter + 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 { + hr := &HookRunner{ + cfgGetter: cfgGetter, + stopCh: make(chan struct{}), + } + + // Subscribe to all events (no repo filter) + _, eventCh := broadcaster.Subscribe("") + + 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. +func (hr *HookRunner) Stop() { + close(hr.stopCh) +} + +// 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: global + repo-specific + hooks := cfg.Hooks + + if event.Repo != "" { + if repoCfg, err := config.LoadRepoConfig(event.Repo); err == nil && repoCfg != nil { + hooks = append(hooks, repoCfg.Hooks...) + } + } + + for _, hook := range hooks { + if !matchEvent(hook.Event, event.Type) { + continue + } + + cmd := resolveCommand(hook, event) + if cmd == "" { + continue + } + + // Run async so hooks don't block workers + go runHook(cmd, event.Repo) + } +} + +// 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): run roborev show %d", repoName, shortSHA, 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. +func interpolate(cmd string, event Event) string { + if cmd == "" { + return "" + } + + r := strings.NewReplacer( + "{job_id}", fmt.Sprintf("%d", event.JobID), + "{repo}", event.Repo, + "{repo_name}", event.RepoName, + "{sha}", event.SHA, + "{agent}", event.Agent, + "{verdict}", event.Verdict, + "{error}", event.Error, + ) + return r.Replace(cmd) +} + +// runHook executes a shell command in the given working directory. +// Errors are logged but never propagated. +func runHook(command, workDir string) { + 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..cbed7db --- /dev/null +++ b/internal/daemon/hooks_test.go @@ -0,0 +1,294 @@ +package daemon + +import ( + "os" + "path/filepath" + "testing" + "time" + + "github.com/roborev-dev/roborev/internal/config" +) + +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 abc123def456", + }, + { + "notify --repo {repo_name} --verdict {verdict}", + "notify --repo myrepo --verdict F", + }, + { + "log {error}", + "log 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 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) + } +} + +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" { + 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: "touch " + 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: "pwd > " + 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(string(data[:len(data)-1])) // trim newline + want := filepath.Clean(tmpDir) + if 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: "touch " + 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", + }) + + time.Sleep(500 * time.Millisecond) + if _, err := os.Stat(markerFile); err == nil { + t.Fatal("hook should not have fired for non-matching event") + } +} + +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() From 9ede30a82345fec5e5fc938ebdbb4a612d2251eb Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Fri, 30 Jan 2026 20:01:38 -0600 Subject: [PATCH 02/19] Fix hooks: slice aliasing, unsubscribe leak, shell injection Address review #3698 findings: - Copy global hooks slice before appending repo hooks to prevent aliasing - Store subscription ID and unsubscribe on Stop to prevent broadcaster leak - Shell-escape interpolated template variables to prevent injection - Add tests for all three fixes Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks.go | 48 +++++++++------ internal/daemon/hooks_test.go | 108 ++++++++++++++++++++++++++++++++-- 2 files changed, 135 insertions(+), 21 deletions(-) diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go index cc0fb10..79ab468 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -13,21 +13,24 @@ import ( // HookRunner listens for broadcaster events and runs configured hooks. type HookRunner struct { - cfgGetter ConfigGetter - mu sync.RWMutex - stopCh chan 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, - stopCh: make(chan struct{}), + cfgGetter: cfgGetter, + broadcaster: broadcaster, + subID: subID, + stopCh: make(chan struct{}), } - // Subscribe to all events (no repo filter) - _, eventCh := broadcaster.Subscribe("") - go hr.listen(eventCh) return hr @@ -48,9 +51,10 @@ func (hr *HookRunner) listen(eventCh <-chan Event) { } } -// Stop shuts down the hook runner. +// 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. @@ -65,8 +69,8 @@ func (hr *HookRunner) handleEvent(event Event) { return } - // Collect hooks: global + repo-specific - hooks := cfg.Hooks + // 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 { @@ -140,6 +144,7 @@ func beadsCommand(event Event) string { } // 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 "" @@ -147,16 +152,25 @@ func interpolate(cmd string, event Event) string { r := strings.NewReplacer( "{job_id}", fmt.Sprintf("%d", event.JobID), - "{repo}", event.Repo, - "{repo_name}", event.RepoName, - "{sha}", event.SHA, - "{agent}", event.Agent, - "{verdict}", event.Verdict, - "{error}", event.Error, + "{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 wraps a value in single quotes, escaping any embedded single quotes. +// This prevents shell injection when values are interpolated into sh -c commands. +func shellEscape(s string) string { + 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) { diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index cbed7db..394d28c 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -50,15 +50,15 @@ func TestInterpolate(t *testing.T) { }{ { "echo {job_id} {sha}", - "echo 42 abc123def456", + "echo 42 'abc123def456'", }, { "notify --repo {repo_name} --verdict {verdict}", - "notify --repo myrepo --verdict F", + "notify --repo 'myrepo' --verdict 'F'", }, { "log {error}", - "log agent timeout", + "log 'agent timeout'", }, { "", @@ -74,6 +74,40 @@ func TestInterpolate(t *testing.T) { } } +func TestInterpolateShellInjection(t *testing.T) { + event := Event{ + JobID: 1, + Repo: "/repo", + Error: "'; rm -rf / #", + } + + got := interpolate("echo {error}", event) + // The value must be wrapped in single quotes with internal quotes escaped. + // It should NOT contain an unquoted semicolon that could break out of quoting. + want := "echo ''\"'\"'; rm -rf / #'" + if got != want { + t.Errorf("interpolate shell injection:\ngot %q\nwant %q", got, want) + } +} + +func TestShellEscape(t *testing.T) { + 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) + } + } +} + func TestBeadsCommand(t *testing.T) { event := Event{ Type: "review.failed", @@ -147,7 +181,7 @@ func TestResolveCommand(t *testing.T) { Command: "echo {job_id}", } cmd := resolveCommand(hook, event) - if cmd != "echo 5" { + if cmd != "echo 5" { // job_id is numeric, not shell-escaped t.Errorf("expected 'echo 5', got %q", cmd) } @@ -280,6 +314,72 @@ func TestHookRunnerNoMatchDoesNotFire(t *testing.T) { } } +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: "touch " + markerGlobal}, + } + cfg := &config.Config{ + Hooks: globalHooks, + } + + // Write a repo config with an additional hook + repoDir := t.TempDir() + os.WriteFile(filepath.Join(repoDir, ".roborev.toml"), []byte(` +[[hooks]] +event = "review.failed" +command = "touch `+markerRepo+`" +`), 0644) + + 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 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) + } +} + func contains(s, substr string) bool { return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsStr(s, substr)) } From 0a23b0fd3b396671152b481da83f2b69724d700c Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Fri, 30 Jan 2026 20:05:59 -0600 Subject: [PATCH 03/19] Add test for quoted placeholders, document auto-escaping in docs Address review #3699: add TestInterpolateQuotedPlaceholders covering double-quoted and empty-value behavior. Update hooks guide to note that placeholders are auto-escaped and should not be manually quoted. Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks_test.go | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 394d28c..4f0b684 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -90,6 +90,38 @@ func TestInterpolateShellInjection(t *testing.T) { } } +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 got != "echo 'simple error'" { + t.Errorf("unquoted placeholder: got %q", got) + } + + // Double-quoted placeholder -- works but includes literal single quotes + got = interpolate(`echo "{error}"`, event) + if got != `echo "'simple error'"` { + t.Errorf("double-quoted placeholder: got %q", got) + } + + // Empty value produces '' + event.Verdict = "" + got = interpolate("echo {verdict}", event) + if got != "echo ''" { + t.Errorf("empty value: got %q", got) + } +} + func TestShellEscape(t *testing.T) { tests := []struct { in string From bd0210bf3b72605ff8562b27309659ff8adc903a Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Fri, 30 Jan 2026 20:56:48 -0600 Subject: [PATCH 04/19] Add tests for global+repo hook resolution and isolation - TestHookRunnerGlobalAndRepoHooksBothFire: both fire for same event - TestHookRunnerRepoOnlyHooks: repo hooks fire with no global hooks - TestHookRunnerRepoHookDoesNotFireForOtherRepo: repo hooks scoped correctly Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks_test.go | 129 ++++++++++++++++++++++++++++++++++ 1 file changed, 129 insertions(+) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 4f0b684..01e17c0 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -391,6 +391,135 @@ command = "touch `+markerRepo+`" } } +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(globalDir, "repo-fired") + + cfg := &config.Config{ + Hooks: []config.HookConfig{ + {Event: "review.failed", Command: "touch " + globalMarker}, + }, + } + + // Write repo config with its own hook + os.WriteFile(filepath.Join(repoDir, ".roborev.toml"), []byte(` +[[hooks]] +event = "review.failed" +command = "touch `+repoMarker+`" +`), 0644) + + 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 + + os.WriteFile(filepath.Join(repoDir, ".roborev.toml"), []byte(` +[[hooks]] +event = "review.completed" +command = "touch `+markerFile+`" +`), 0644) + + 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 + os.WriteFile(filepath.Join(repoA, ".roborev.toml"), []byte(` +[[hooks]] +event = "review.failed" +command = "touch `+markerFile+`" +`), 0644) + + 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", + }) + + time.Sleep(500 * time.Millisecond) + if _, err := os.Stat(markerFile); err == nil { + t.Fatal("repo hook fired for a different repo's event") + } +} + func TestHookRunnerStopUnsubscribes(t *testing.T) { broadcaster := NewBroadcaster() cfg := &config.Config{} From 3479631e8aa99f55a7ff6e290a10cc9f3b35a601 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Fri, 30 Jan 2026 20:59:36 -0600 Subject: [PATCH 05/19] Fix test hygiene: marker path, WriteFile errors, negative assertions Address review #3707: - Move repoMarker to repoDir instead of globalDir (copy-paste fix) - Add writeRepoConfig helper that fatals on os.WriteFile error - Replace fixed sleep in negative tests with polling assertFileNotCreated Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks_test.go | 48 ++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 01e17c0..913d8fe 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -340,10 +340,7 @@ func TestHookRunnerNoMatchDoesNotFire(t *testing.T) { Error: "fail", }) - time.Sleep(500 * time.Millisecond) - if _, err := os.Stat(markerFile); err == nil { - t.Fatal("hook should not have fired for non-matching event") - } + assertFileNotCreated(t, markerFile, 500*time.Millisecond, "hook should not have fired for non-matching event") } func TestHooksSliceNotAliased(t *testing.T) { @@ -361,11 +358,11 @@ func TestHooksSliceNotAliased(t *testing.T) { // Write a repo config with an additional hook repoDir := t.TempDir() - os.WriteFile(filepath.Join(repoDir, ".roborev.toml"), []byte(` + writeRepoConfig(t, repoDir, ` [[hooks]] event = "review.failed" command = "touch `+markerRepo+`" -`), 0644) +`) broadcaster := NewBroadcaster() hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) @@ -396,7 +393,7 @@ func TestHookRunnerGlobalAndRepoHooksBothFire(t *testing.T) { globalDir := t.TempDir() repoDir := t.TempDir() globalMarker := filepath.Join(globalDir, "global-fired") - repoMarker := filepath.Join(globalDir, "repo-fired") + repoMarker := filepath.Join(repoDir, "repo-fired") cfg := &config.Config{ Hooks: []config.HookConfig{ @@ -405,11 +402,11 @@ func TestHookRunnerGlobalAndRepoHooksBothFire(t *testing.T) { } // Write repo config with its own hook - os.WriteFile(filepath.Join(repoDir, ".roborev.toml"), []byte(` + writeRepoConfig(t, repoDir, ` [[hooks]] event = "review.failed" command = "touch `+repoMarker+`" -`), 0644) +`) broadcaster := NewBroadcaster() hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) @@ -454,11 +451,11 @@ func TestHookRunnerRepoOnlyHooks(t *testing.T) { cfg := &config.Config{} // no global hooks - os.WriteFile(filepath.Join(repoDir, ".roborev.toml"), []byte(` + writeRepoConfig(t, repoDir, ` [[hooks]] event = "review.completed" command = "touch `+markerFile+`" -`), 0644) +`) broadcaster := NewBroadcaster() hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) @@ -493,11 +490,11 @@ func TestHookRunnerRepoHookDoesNotFireForOtherRepo(t *testing.T) { cfg := &config.Config{} // no global hooks // Only repoA has hooks - os.WriteFile(filepath.Join(repoA, ".roborev.toml"), []byte(` + writeRepoConfig(t, repoA, ` [[hooks]] event = "review.failed" command = "touch `+markerFile+`" -`), 0644) +`) broadcaster := NewBroadcaster() hr := NewHookRunner(NewStaticConfig(cfg), broadcaster) @@ -514,10 +511,7 @@ command = "touch `+markerFile+`" Error: "fail", }) - time.Sleep(500 * time.Millisecond) - if _, err := os.Stat(markerFile); err == nil { - t.Fatal("repo hook fired for a different repo's event") - } + assertFileNotCreated(t, markerFile, 500*time.Millisecond, "repo hook fired for a different repo's event") } func TestHookRunnerStopUnsubscribes(t *testing.T) { @@ -541,6 +535,26 @@ func TestHookRunnerStopUnsubscribes(t *testing.T) { } } +// 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 contains(s, substr string) bool { return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsStr(s, substr)) } From b1c0593524851c07bb234490f15abee817d25cb2 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Fri, 30 Jan 2026 21:08:27 -0600 Subject: [PATCH 06/19] Fix hooks for Windows: use cmd /C, platform-aware escaping and tests - runHook uses cmd /C on Windows instead of sh -c - shellEscape uses double-quote escaping on Windows - Tests use touchCmd/pwdCmd helpers for cross-platform commands Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks.go | 24 +++++++++++++++++++++--- internal/daemon/hooks_test.go | 35 ++++++++++++++++++++++++++--------- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go index 79ab468..b32f66a 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -5,6 +5,7 @@ import ( "log" "os/exec" "path/filepath" + "runtime" "strings" "sync" @@ -162,19 +163,36 @@ func interpolate(cmd string, event Event) string { return r.Replace(cmd) } -// shellEscape wraps a value in single quotes, escaping any embedded single quotes. -// This prevents shell injection when values are interpolated into sh -c commands. +// 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" { + if s == "" { + return `""` + } + // cmd.exe uses double quotes; escape embedded double quotes by doubling them + return `"` + strings.ReplaceAll(s, `"`, `""`) + `"` + } if s == "" { return "''" } return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'" } +// shellCommand returns the platform-appropriate shell and flag for running a command string. +func shellCommand() (string, string) { + if runtime.GOOS == "windows" { + return "cmd", "/C" + } + return "sh", "-c" +} + // runHook executes a shell command in the given working directory. // Errors are logged but never propagated. func runHook(command, workDir string) { - cmd := exec.Command("sh", "-c", command) + shell, flag := shellCommand() + cmd := exec.Command(shell, flag, command) if workDir != "" { cmd.Dir = workDir } diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 913d8fe..71f63e6 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -3,12 +3,29 @@ package daemon import ( "os" "path/filepath" + "runtime" "testing" "time" "github.com/roborev-dev/roborev/internal/config" ) +// touchCmd returns a platform-appropriate shell command to create a file. +func touchCmd(path string) string { + if runtime.GOOS == "windows" { + return `type nul > "` + 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 `cd > "` + path + `"` + } + return "pwd > " + path +} + func TestMatchEvent(t *testing.T) { tests := []struct { pattern string @@ -236,7 +253,7 @@ func TestHookRunnerFiresHooks(t *testing.T) { Hooks: []config.HookConfig{ { Event: "review.completed", - Command: "touch " + markerFile, + Command: touchCmd(markerFile), }, }, } @@ -275,7 +292,7 @@ func TestHookRunnerWorkingDirectory(t *testing.T) { Hooks: []config.HookConfig{ { Event: "review.failed", - Command: "pwd > " + markerFile, + Command: pwdCmd(markerFile), }, }, } @@ -319,7 +336,7 @@ func TestHookRunnerNoMatchDoesNotFire(t *testing.T) { Hooks: []config.HookConfig{ { Event: "review.completed", - Command: "touch " + markerFile, + Command: touchCmd(markerFile), }, }, } @@ -350,7 +367,7 @@ func TestHooksSliceNotAliased(t *testing.T) { markerRepo := filepath.Join(tmpDir, "repo-fired") globalHooks := []config.HookConfig{ - {Event: "review.failed", Command: "touch " + markerGlobal}, + {Event: "review.failed", Command: touchCmd(markerGlobal)}, } cfg := &config.Config{ Hooks: globalHooks, @@ -361,7 +378,7 @@ func TestHooksSliceNotAliased(t *testing.T) { writeRepoConfig(t, repoDir, ` [[hooks]] event = "review.failed" -command = "touch `+markerRepo+`" +command = "`+touchCmd(markerRepo)+`" `) broadcaster := NewBroadcaster() @@ -397,7 +414,7 @@ func TestHookRunnerGlobalAndRepoHooksBothFire(t *testing.T) { cfg := &config.Config{ Hooks: []config.HookConfig{ - {Event: "review.failed", Command: "touch " + globalMarker}, + {Event: "review.failed", Command: touchCmd(globalMarker)}, }, } @@ -405,7 +422,7 @@ func TestHookRunnerGlobalAndRepoHooksBothFire(t *testing.T) { writeRepoConfig(t, repoDir, ` [[hooks]] event = "review.failed" -command = "touch `+repoMarker+`" +command = "`+touchCmd(repoMarker)+`" `) broadcaster := NewBroadcaster() @@ -454,7 +471,7 @@ func TestHookRunnerRepoOnlyHooks(t *testing.T) { writeRepoConfig(t, repoDir, ` [[hooks]] event = "review.completed" -command = "touch `+markerFile+`" +command = "`+touchCmd(markerFile)+`" `) broadcaster := NewBroadcaster() @@ -493,7 +510,7 @@ func TestHookRunnerRepoHookDoesNotFireForOtherRepo(t *testing.T) { writeRepoConfig(t, repoA, ` [[hooks]] event = "review.failed" -command = "touch `+markerFile+`" +command = "`+touchCmd(markerFile)+`" `) broadcaster := NewBroadcaster() From a410645364fba502ba55a23ec2ed542bef9c0a41 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Fri, 30 Jan 2026 22:06:11 -0600 Subject: [PATCH 07/19] Fix hook tests to use platform-aware quoting expectations Tests now use q() helper (wraps shellEscape) instead of hardcoded single-quote literals, so they pass on both Unix and Windows. Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks_test.go | 61 +++++++++++++++++++++++------------ 1 file changed, 41 insertions(+), 20 deletions(-) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 71f63e6..84da995 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -10,6 +10,11 @@ import ( "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) +} + // touchCmd returns a platform-appropriate shell command to create a file. func touchCmd(path string) string { if runtime.GOOS == "windows" { @@ -67,15 +72,15 @@ func TestInterpolate(t *testing.T) { }{ { "echo {job_id} {sha}", - "echo 42 'abc123def456'", + "echo 42 " + q("abc123def456"), }, { "notify --repo {repo_name} --verdict {verdict}", - "notify --repo 'myrepo' --verdict 'F'", + "notify --repo " + q("myrepo") + " --verdict " + q("F"), }, { "log {error}", - "log 'agent timeout'", + "log " + q("agent timeout"), }, { "", @@ -99,9 +104,8 @@ func TestInterpolateShellInjection(t *testing.T) { } got := interpolate("echo {error}", event) - // The value must be wrapped in single quotes with internal quotes escaped. - // It should NOT contain an unquoted semicolon that could break out of quoting. - want := "echo ''\"'\"'; rm -rf / #'" + // The value must be quoted so the semicolon cannot break out and execute commands. + want := "echo " + q("'; rm -rf / #") if got != want { t.Errorf("interpolate shell injection:\ngot %q\nwant %q", got, want) } @@ -121,33 +125,50 @@ func TestInterpolateQuotedPlaceholders(t *testing.T) { // Unquoted placeholder (recommended) -- clean output got := interpolate("echo {error}", event) - if got != "echo 'simple error'" { - t.Errorf("unquoted placeholder: got %q", got) + 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 single quotes + // Double-quoted placeholder -- works but includes literal quotes around the value got = interpolate(`echo "{error}"`, event) - if got != `echo "'simple error'"` { - t.Errorf("double-quoted placeholder: got %q", got) + if want := `echo "` + q("simple error") + `"`; got != want { + t.Errorf("double-quoted placeholder: got %q, want %q", got, want) } - // Empty value produces '' + // Empty value produces empty quoted string event.Verdict = "" got = interpolate("echo {verdict}", event) - if got != "echo ''" { - t.Errorf("empty value: got %q", got) + if want := "echo " + q(""); got != want { + t.Errorf("empty value: got %q, want %q", got, want) } } func TestShellEscape(t *testing.T) { - tests := []struct { + var tests []struct { in string want string - }{ - {"hello", "'hello'"}, - {"", "''"}, - {"it's", "'it'\"'\"'s'"}, - {"a;b", "'a;b'"}, + } + if runtime.GOOS == "windows" { + tests = []struct { + in string + want string + }{ + {"hello", `"hello"`}, + {"", `""`}, + {"it's", `"it's"`}, + {"a;b", `"a;b"`}, + {`say "hi"`, `"say ""hi"""`}, + } + } 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) From d1364e0ada8f8fb201dc32b6722c83dc798c5e7f Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 08:07:08 -0600 Subject: [PATCH 08/19] Fix tautological injection test to assert safety properties TestInterpolateShellInjection now tests multiple injection payloads and asserts they are properly quote-enclosed rather than comparing against shellEscape output (which was testing the function against itself). Also adds injection payloads to TestShellEscape. Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks_test.go | 69 ++++++++++++++++++++++++++++++----- 1 file changed, 59 insertions(+), 10 deletions(-) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 84da995..ece3605 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "time" @@ -97,17 +98,45 @@ func TestInterpolate(t *testing.T) { } func TestInterpolateShellInjection(t *testing.T) { - event := Event{ - JobID: 1, - Repo: "/repo", - Error: "'; rm -rf / #", - } + // 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) + + // The interpolated command must start with "echo " followed by a quoted value. + // The value must be fully enclosed in quotes (single on Unix, double on Windows). + if runtime.GOOS == "windows" { + // Must be wrapped in double quotes + val := got[len("echo "):] + if val[0] != '"' || val[len(val)-1] != '"' { + t.Errorf("payload %q: not double-quoted: %q", payload, got) + } + } else { + // Must be wrapped in single quotes (possibly with escaped internal quotes) + val := got[len("echo "):] + if val[0] != '\'' || val[len(val)-1] != '\'' { + t.Errorf("payload %q: not single-quoted: %q", payload, got) + } + } - got := interpolate("echo {error}", event) - // The value must be quoted so the semicolon cannot break out and execute commands. - want := "echo " + q("'; rm -rf / #") - if got != want { - t.Errorf("interpolate shell injection:\ngot %q\nwant %q", got, want) + // The raw payload must never appear unquoted in the output + // (i.e., the dangerous characters must be inside the quoted region) + prefix := "echo " + if !strings.HasPrefix(got, prefix) { + t.Errorf("payload %q: unexpected format: %q", payload, got) + continue + } } } @@ -176,6 +205,26 @@ func TestShellEscape(t *testing.T) { 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 runtime.GOOS == "windows" { + if got[0] != '"' || got[len(got)-1] != '"' { + t.Errorf("shellEscape(%q) not double-quoted: %q", payload, got) + } + } else { + if got[0] != '\'' || got[len(got)-1] != '\'' { + t.Errorf("shellEscape(%q) not single-quoted: %q", payload, got) + } + } + } } func TestBeadsCommand(t *testing.T) { From 5486120c1c75f5fa821ade3fdb390a05b3eecd46 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 14:23:28 -0600 Subject: [PATCH 09/19] Show daemon version in status, add fix hint to beads hook Display the daemon version in `roborev status` output so it's easy to tell if the running daemon matches the installed binary. Update the built-in beads hook for failing reviews to include a `roborev fix` hint alongside `roborev show`. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/main.go | 11 +++++++---- internal/daemon/hooks.go | 2 +- internal/daemon/hooks_test.go | 3 +++ 3 files changed, 11 insertions(+), 5 deletions(-) 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/daemon/hooks.go b/internal/daemon/hooks.go index b32f66a..cf93453 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -135,7 +135,7 @@ func beadsCommand(event Event) string { return fmt.Sprintf("bd create %q -p 1", title) case "review.completed": if event.Verdict == "F" { - title := fmt.Sprintf("Review findings for %s (%s): run roborev show %d", repoName, shortSHA, event.JobID) + 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 diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index ece3605..cbc1eff 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -266,6 +266,9 @@ func TestBeadsCommand(t *testing.T) { 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) { From 9b098432417ea3d524540a784ace852d93259562 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 14:39:24 -0600 Subject: [PATCH 10/19] Log hook activity, add fix hint to beads hook message Add a log line when hooks fire so daemon logs show hook activity. Add unit tests for logging behavior (fires/no-match). Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks.go | 6 ++++ internal/daemon/hooks_test.go | 63 +++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go index cf93453..9414bfe 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -79,6 +79,7 @@ func (hr *HookRunner) handleEvent(event Event) { } } + fired := 0 for _, hook := range hooks { if !matchEvent(hook.Event, event.Type) { continue @@ -89,9 +90,14 @@ func (hr *HookRunner) handleEvent(event Event) { 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. diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index cbc1eff..5d84670 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -1,6 +1,8 @@ package daemon import ( + "bytes" + "log" "os" "path/filepath" "runtime" @@ -645,6 +647,67 @@ func assertFileNotCreated(t *testing.T, path string, duration time.Duration, msg } } +func TestHandleEventLogsWhenHooksFired(t *testing.T) { + // Capture log output + var buf bytes.Buffer + log.SetOutput(&buf) + defer log.SetOutput(os.Stderr) + + tmpDir := t.TempDir() + markerFile := filepath.Join(tmpDir, "log-test") + + cfg := &config.Config{ + Hooks: []config.HookConfig{ + {Event: "review.completed", Command: touchCmd(markerFile)}, + {Event: "review.completed", Command: touchCmd(markerFile + "2")}, + }, + } + + hr := &HookRunner{cfgGetter: NewStaticConfig(cfg)} + hr.handleEvent(Event{ + Type: "review.completed", + JobID: 42, + Repo: tmpDir, + SHA: "abc", + Verdict: "P", + }) + + 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 + log.SetOutput(&buf) + defer log.SetOutput(os.Stderr) + + 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", + }) + + 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)) } From a0631f25d593a6d747af8a7380f6606b73391eb5 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 14:52:01 -0600 Subject: [PATCH 11/19] Fix verdict not shown for branch range reviews IsTaskJob() incorrectly classified branch reviews (commit ranges with ".." in git_ref) as task jobs, skipping verdict parsing. Add the range check and unit tests for IsTaskJob. Co-Authored-By: Claude Opus 4.5 --- internal/storage/models.go | 62 +++++++++++++++++--------------- internal/storage/models_test.go | 63 +++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+), 28 deletions(-) create mode 100644 internal/storage/models_test.go 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..64ea2ec --- /dev/null +++ b/internal/storage/models_test.go @@ -0,0 +1,63 @@ +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: "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 } From 21eeb1999e5f2c731b33de7c1401953b0b0df7c0 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 14:56:08 -0600 Subject: [PATCH 12/19] Address review findings: log writer restoration, range verdict test Restore previous log.Writer() instead of hardcoded os.Stderr in hook log tests. Add regression test proving verdict is parsed for branch range reviews (the user-visible bug). Add additional IsTaskJob test cases. Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks_test.go | 6 ++-- internal/storage/db_test.go | 52 +++++++++++++++++++++++++++++++-- internal/storage/models_test.go | 5 ++++ 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 5d84670..1f82e8a 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -650,8 +650,9 @@ func assertFileNotCreated(t *testing.T, path string, duration time.Duration, msg func TestHandleEventLogsWhenHooksFired(t *testing.T) { // Capture log output var buf bytes.Buffer + prevOut := log.Writer() log.SetOutput(&buf) - defer log.SetOutput(os.Stderr) + t.Cleanup(func() { log.SetOutput(prevOut) }) tmpDir := t.TempDir() markerFile := filepath.Join(tmpDir, "log-test") @@ -686,8 +687,9 @@ func TestHandleEventLogsWhenHooksFired(t *testing.T) { func TestHandleEventNoLogWhenNoHooksMatch(t *testing.T) { var buf bytes.Buffer + prevOut := log.Writer() log.SetOutput(&buf) - defer log.SetOutput(os.Stderr) + t.Cleanup(func() { log.SetOutput(prevOut) }) cfg := &config.Config{ Hooks: []config.HookConfig{ 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_test.go b/internal/storage/models_test.go index 64ea2ec..6689222 100644 --- a/internal/storage/models_test.go +++ b/internal/storage/models_test.go @@ -43,6 +43,11 @@ func TestIsTaskJob(t *testing.T) { job: ReviewJob{GitRef: "analyze"}, want: true, }, + { + name: "task job run", + job: ReviewJob{GitRef: "run"}, + want: true, + }, { name: "empty git ref", job: ReviewJob{GitRef: ""}, From 3bb16315308d88708dda31a9d2ca94c206a63c8c Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 15:33:55 -0600 Subject: [PATCH 13/19] Harden hooks: test guards, payload assertions, Windows escaping - Add length/prefix guards before slicing in injection and escape tests to fail cleanly instead of panicking on regressions - Assert payload content is preserved inside the quoted region - Escape % in Windows shellEscape to prevent env variable expansion - Use cmd /D /C on Windows to disable AutoRun registry entries - Refactor shellCommand to return arg slice for multi-flag support Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks.go | 20 ++++++++++++-------- internal/daemon/hooks_test.go | 26 +++++++++++++++----------- 2 files changed, 27 insertions(+), 19 deletions(-) diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go index 9414bfe..429cb68 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -177,8 +177,11 @@ func shellEscape(s string) string { if s == "" { return `""` } - // cmd.exe uses double quotes; escape embedded double quotes by doubling them - return `"` + strings.ReplaceAll(s, `"`, `""`) + `"` + // cmd.exe uses double quotes; escape embedded double quotes by doubling them. + // Also escape % to prevent environment variable expansion. + s = strings.ReplaceAll(s, `"`, `""`) + s = strings.ReplaceAll(s, `%`, `%%`) + return `"` + s + `"` } if s == "" { return "''" @@ -186,19 +189,20 @@ func shellEscape(s string) string { return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'" } -// shellCommand returns the platform-appropriate shell and flag for running a command string. -func shellCommand() (string, string) { +// shellCommand returns the platform-appropriate shell and args for running a command string. +// On Windows, uses /D to disable AutoRun registry entries. +func shellCommand(command string) []string { if runtime.GOOS == "windows" { - return "cmd", "/C" + return []string{"cmd", "/D", "/C", command} } - return "sh", "-c" + return []string{"sh", "-c", command} } // runHook executes a shell command in the given working directory. // Errors are logged but never propagated. func runHook(command, workDir string) { - shell, flag := shellCommand() - cmd := exec.Command(shell, flag, command) + args := shellCommand(command) + cmd := exec.Command(args[0], args[1:]...) if workDir != "" { cmd.Dir = workDir } diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 1f82e8a..65c0ece 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -116,28 +116,28 @@ func TestInterpolateShellInjection(t *testing.T) { event := Event{JobID: 1, Repo: "/repo", Error: payload} got := interpolate("echo {error}", event) - // The interpolated command must start with "echo " followed by a quoted value. + 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 quotes (single on Unix, double on Windows). if runtime.GOOS == "windows" { - // Must be wrapped in double quotes - val := got[len("echo "):] if val[0] != '"' || val[len(val)-1] != '"' { t.Errorf("payload %q: not double-quoted: %q", payload, got) } } else { - // Must be wrapped in single quotes (possibly with escaped internal quotes) - val := got[len("echo "):] if val[0] != '\'' || val[len(val)-1] != '\'' { t.Errorf("payload %q: not single-quoted: %q", payload, got) } } - // The raw payload must never appear unquoted in the output - // (i.e., the dangerous characters must be inside the quoted region) - prefix := "echo " - if !strings.HasPrefix(got, prefix) { - t.Errorf("payload %q: unexpected format: %q", payload, got) - continue + // 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) } } } @@ -189,6 +189,7 @@ func TestShellEscape(t *testing.T) { {"it's", `"it's"`}, {"a;b", `"a;b"`}, {`say "hi"`, `"say ""hi"""`}, + {"%PATH%", `"%%PATH%%"`}, } } else { tests = []struct { @@ -217,6 +218,9 @@ func TestShellEscape(t *testing.T) { } for _, payload := range injections { got := shellEscape(payload) + if len(got) < 2 { + t.Fatalf("shellEscape(%q) too short: %q", payload, got) + } if runtime.GOOS == "windows" { if got[0] != '"' || got[len(got)-1] != '"' { t.Errorf("shellEscape(%q) not double-quoted: %q", payload, got) From 1a63d33f02fb51d73ba5946a5b14205c56045c25 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 16:33:04 -0600 Subject: [PATCH 14/19] Fix data race in hook log tests Restore the global logger immediately after handleEvent returns (before async goroutines finish) instead of via t.Cleanup, which runs after the test function exits. The "fired N hook(s)" log line is written synchronously by handleEvent, so the buffer is safe to read before restoring. Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks_test.go | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 65c0ece..3318820 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -652,19 +652,18 @@ func assertFileNotCreated(t *testing.T, path string, duration time.Duration, msg } func TestHandleEventLogsWhenHooksFired(t *testing.T) { - // Capture log output + // 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) - t.Cleanup(func() { log.SetOutput(prevOut) }) - - tmpDir := t.TempDir() - markerFile := filepath.Join(tmpDir, "log-test") cfg := &config.Config{ Hooks: []config.HookConfig{ - {Event: "review.completed", Command: touchCmd(markerFile)}, - {Event: "review.completed", Command: touchCmd(markerFile + "2")}, + {Event: "review.completed", Command: "true"}, + {Event: "review.completed", Command: "true"}, }, } @@ -672,11 +671,14 @@ func TestHandleEventLogsWhenHooksFired(t *testing.T) { hr.handleEvent(Event{ Type: "review.completed", JobID: 42, - Repo: tmpDir, + 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) @@ -693,7 +695,6 @@ func TestHandleEventNoLogWhenNoHooksMatch(t *testing.T) { var buf bytes.Buffer prevOut := log.Writer() log.SetOutput(&buf) - t.Cleanup(func() { log.SetOutput(prevOut) }) cfg := &config.Config{ Hooks: []config.HookConfig{ @@ -709,6 +710,9 @@ func TestHandleEventNoLogWhenNoHooksMatch(t *testing.T) { 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()) } From 52ec20953cde0bd4d18d820218337dc1f1dbcd41 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 16:48:47 -0600 Subject: [PATCH 15/19] Fix Windows hook tests: use copy nul, add /D to disable AutoRun Replace 'type nul > path' with 'copy nul path' in touchCmd helper since cmd.exe handles quoted paths more reliably with copy. Inline platform shell logic in runHook with /D flag to disable AutoRun registry entries on Windows. Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks.go | 18 +++++++----------- internal/daemon/hooks_test.go | 2 +- 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go index 429cb68..fd77f42 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -189,20 +189,16 @@ func shellEscape(s string) string { return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'" } -// shellCommand returns the platform-appropriate shell and args for running a command string. -// On Windows, uses /D to disable AutoRun registry entries. -func shellCommand(command string) []string { - if runtime.GOOS == "windows" { - return []string{"cmd", "/D", "/C", command} - } - return []string{"sh", "-c", command} -} - // runHook executes a shell command in the given working directory. // Errors are logged but never propagated. func runHook(command, workDir string) { - args := shellCommand(command) - cmd := exec.Command(args[0], args[1:]...) + var cmd *exec.Cmd + if runtime.GOOS == "windows" { + // Use /D to disable AutoRun registry entries for security. + cmd = exec.Command("cmd", "/D", "/C", command) + } else { + cmd = exec.Command("sh", "-c", command) + } if workDir != "" { cmd.Dir = workDir } diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 3318820..9380ab8 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -21,7 +21,7 @@ func q(s string) string { // touchCmd returns a platform-appropriate shell command to create a file. func touchCmd(path string) string { if runtime.GOOS == "windows" { - return `type nul > "` + path + `"` + return `copy nul "` + path + `"` } return "touch " + path } From 39e5b6b994790e3aae5b17e321d7df8371964d45 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 17:01:26 -0600 Subject: [PATCH 16/19] Fix Windows hook tests: use PowerShell for reliable path handling cmd.exe's redirection and copy commands fail with quoted paths containing short names (8.3 format). Switch to PowerShell for touchCmd and pwdCmd helpers which handle all path formats correctly. Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 9380ab8..42fd702 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -21,7 +21,8 @@ func q(s string) string { // touchCmd returns a platform-appropriate shell command to create a file. func touchCmd(path string) string { if runtime.GOOS == "windows" { - return `copy nul "` + path + `"` + // Use PowerShell for reliable path handling on Windows. + return `powershell -NoProfile -Command "New-Item -ItemType File -Force -Path '` + path + `'"` } return "touch " + path } @@ -29,7 +30,7 @@ func touchCmd(path string) string { // pwdCmd returns a platform-appropriate shell command to write the cwd to a file. func pwdCmd(path string) string { if runtime.GOOS == "windows" { - return `cd > "` + path + `"` + return `powershell -NoProfile -Command "(Get-Location).Path | Set-Content '` + path + `'"` } return "pwd > " + path } From af8046d8703fcc8b88270cd76f59dcd1990f974a Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 17:09:50 -0600 Subject: [PATCH 17/19] Skip shell integration tests on Windows cmd.exe's path quoting with 8.3 short names is unreliable for creating files via redirection. Skip tests that spawn shell processes on Windows; the unit tests (matchEvent, interpolate, shellEscape, beadsCommand) cover all hooks logic without needing shell execution. Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 42fd702..acb46d9 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -13,6 +13,17 @@ import ( "github.com/roborev-dev/roborev/internal/config" ) +// skipOnWindows skips tests that exec shell commands via cmd.exe, +// since cmd.exe's quoting of paths with 8.3 short names is unreliable. +// The unit tests (matchEvent, interpolate, shellEscape, beadsCommand) +// cover all hooks logic without spawning shell processes. +func skipOnWindows(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("skipping shell integration test on Windows") + } +} + // q wraps a string in platform-appropriate shell quoting (matches shellEscape output). func q(s string) string { return shellEscape(s) @@ -326,6 +337,7 @@ func TestResolveCommand(t *testing.T) { } func TestHookRunnerFiresHooks(t *testing.T) { + skipOnWindows(t) tmpDir := t.TempDir() markerFile := filepath.Join(tmpDir, "hook-fired") @@ -365,6 +377,7 @@ func TestHookRunnerFiresHooks(t *testing.T) { } func TestHookRunnerWorkingDirectory(t *testing.T) { + skipOnWindows(t) tmpDir := t.TempDir() markerFile := filepath.Join(tmpDir, "pwd-test") @@ -409,6 +422,7 @@ func TestHookRunnerWorkingDirectory(t *testing.T) { } func TestHookRunnerNoMatchDoesNotFire(t *testing.T) { + skipOnWindows(t) tmpDir := t.TempDir() markerFile := filepath.Join(tmpDir, "should-not-exist") @@ -441,6 +455,7 @@ func TestHookRunnerNoMatchDoesNotFire(t *testing.T) { } func TestHooksSliceNotAliased(t *testing.T) { + skipOnWindows(t) // Verify that repo hooks don't leak into the global config's Hooks slice tmpDir := t.TempDir() markerGlobal := filepath.Join(tmpDir, "global-fired") @@ -486,6 +501,7 @@ command = "`+touchCmd(markerRepo)+`" } func TestHookRunnerGlobalAndRepoHooksBothFire(t *testing.T) { + skipOnWindows(t) // Both global and per-repo hooks should fire for the same event globalDir := t.TempDir() repoDir := t.TempDir() @@ -542,6 +558,7 @@ command = "`+touchCmd(repoMarker)+`" } func TestHookRunnerRepoOnlyHooks(t *testing.T) { + skipOnWindows(t) // Repo hooks fire even when there are no global hooks repoDir := t.TempDir() markerFile := filepath.Join(repoDir, "repo-only") @@ -579,6 +596,7 @@ command = "`+touchCmd(markerFile)+`" } func TestHookRunnerRepoHookDoesNotFireForOtherRepo(t *testing.T) { + skipOnWindows(t) // A repo's hooks should not fire for events from a different repo repoA := t.TempDir() repoB := t.TempDir() From e82b517307cbb83a00770e4900482689a3860327 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 17:26:16 -0600 Subject: [PATCH 18/19] Use PowerShell instead of cmd.exe for hooks on Windows cmd.exe's quoting and redirection are unreliable with 8.3 short paths. Switch to PowerShell for hook execution on Windows: handles all path formats correctly, uses single-quote escaping ('' for literal '), and avoids cmd.exe's AutoRun and %variable% expansion issues. This unifies escaping (single quotes on all platforms) and removes the need to skip integration tests on Windows. Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks.go | 15 ++++---- internal/daemon/hooks_test.go | 65 +++++++++++------------------------ 2 files changed, 28 insertions(+), 52 deletions(-) diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go index fd77f42..e3004f4 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -174,14 +174,11 @@ func interpolate(cmd string, event Event) string { // 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 "''" } - // cmd.exe uses double quotes; escape embedded double quotes by doubling them. - // Also escape % to prevent environment variable expansion. - s = strings.ReplaceAll(s, `"`, `""`) - s = strings.ReplaceAll(s, `%`, `%%`) - return `"` + s + `"` + return "'" + strings.ReplaceAll(s, "'", "''") + "'" } if s == "" { return "''" @@ -194,8 +191,10 @@ func shellEscape(s string) string { func runHook(command, workDir string) { var cmd *exec.Cmd if runtime.GOOS == "windows" { - // Use /D to disable AutoRun registry entries for security. - cmd = exec.Command("cmd", "/D", "/C", command) + // 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) } diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index acb46d9..3abac2a 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -13,17 +13,6 @@ import ( "github.com/roborev-dev/roborev/internal/config" ) -// skipOnWindows skips tests that exec shell commands via cmd.exe, -// since cmd.exe's quoting of paths with 8.3 short names is unreliable. -// The unit tests (matchEvent, interpolate, shellEscape, beadsCommand) -// cover all hooks logic without spawning shell processes. -func skipOnWindows(t *testing.T) { - t.Helper() - if runtime.GOOS == "windows" { - t.Skip("skipping shell integration test on Windows") - } -} - // q wraps a string in platform-appropriate shell quoting (matches shellEscape output). func q(s string) string { return shellEscape(s) @@ -32,8 +21,8 @@ func q(s string) string { // touchCmd returns a platform-appropriate shell command to create a file. func touchCmd(path string) string { if runtime.GOOS == "windows" { - // Use PowerShell for reliable path handling on Windows. - return `powershell -NoProfile -Command "New-Item -ItemType File -Force -Path '` + path + `'"` + // runHook uses PowerShell on Windows, so use PowerShell commands directly. + return "New-Item -ItemType File -Force -Path '" + path + "'" } return "touch " + path } @@ -41,7 +30,7 @@ func touchCmd(path string) string { // pwdCmd returns a platform-appropriate shell command to write the cwd to a file. func pwdCmd(path string) string { if runtime.GOOS == "windows" { - return `powershell -NoProfile -Command "(Get-Location).Path | Set-Content '` + path + `'"` + return "(Get-Location).Path | Set-Content '" + path + "'" } return "pwd > " + path } @@ -134,15 +123,9 @@ func TestInterpolateShellInjection(t *testing.T) { } val := got[len(prefix):] - // The value must be fully enclosed in quotes (single on Unix, double on Windows). - if runtime.GOOS == "windows" { - if val[0] != '"' || val[len(val)-1] != '"' { - t.Errorf("payload %q: not double-quoted: %q", payload, got) - } - } else { - if val[0] != '\'' || val[len(val)-1] != '\'' { - t.Errorf("payload %q: not single-quoted: %q", payload, got) - } + // 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). @@ -196,12 +179,12 @@ func TestShellEscape(t *testing.T) { in string want string }{ - {"hello", `"hello"`}, - {"", `""`}, - {"it's", `"it's"`}, - {"a;b", `"a;b"`}, - {`say "hi"`, `"say ""hi"""`}, - {"%PATH%", `"%%PATH%%"`}, + {"hello", "'hello'"}, + {"", "''"}, + {"it's", "'it''s'"}, + {"a;b", "'a;b'"}, + {`say "hi"`, `'say "hi"'`}, + {"%PATH%", "'%PATH%'"}, } } else { tests = []struct { @@ -233,14 +216,8 @@ func TestShellEscape(t *testing.T) { if len(got) < 2 { t.Fatalf("shellEscape(%q) too short: %q", payload, got) } - if runtime.GOOS == "windows" { - if got[0] != '"' || got[len(got)-1] != '"' { - t.Errorf("shellEscape(%q) not double-quoted: %q", payload, got) - } - } else { - if got[0] != '\'' || got[len(got)-1] != '\'' { - t.Errorf("shellEscape(%q) not single-quoted: %q", payload, got) - } + if got[0] != '\'' || got[len(got)-1] != '\'' { + t.Errorf("shellEscape(%q) not single-quoted: %q", payload, got) } } } @@ -337,7 +314,7 @@ func TestResolveCommand(t *testing.T) { } func TestHookRunnerFiresHooks(t *testing.T) { - skipOnWindows(t) + tmpDir := t.TempDir() markerFile := filepath.Join(tmpDir, "hook-fired") @@ -377,7 +354,7 @@ func TestHookRunnerFiresHooks(t *testing.T) { } func TestHookRunnerWorkingDirectory(t *testing.T) { - skipOnWindows(t) + tmpDir := t.TempDir() markerFile := filepath.Join(tmpDir, "pwd-test") @@ -422,7 +399,7 @@ func TestHookRunnerWorkingDirectory(t *testing.T) { } func TestHookRunnerNoMatchDoesNotFire(t *testing.T) { - skipOnWindows(t) + tmpDir := t.TempDir() markerFile := filepath.Join(tmpDir, "should-not-exist") @@ -455,7 +432,7 @@ func TestHookRunnerNoMatchDoesNotFire(t *testing.T) { } func TestHooksSliceNotAliased(t *testing.T) { - skipOnWindows(t) + // Verify that repo hooks don't leak into the global config's Hooks slice tmpDir := t.TempDir() markerGlobal := filepath.Join(tmpDir, "global-fired") @@ -501,7 +478,7 @@ command = "`+touchCmd(markerRepo)+`" } func TestHookRunnerGlobalAndRepoHooksBothFire(t *testing.T) { - skipOnWindows(t) + // Both global and per-repo hooks should fire for the same event globalDir := t.TempDir() repoDir := t.TempDir() @@ -558,7 +535,7 @@ command = "`+touchCmd(repoMarker)+`" } func TestHookRunnerRepoOnlyHooks(t *testing.T) { - skipOnWindows(t) + // Repo hooks fire even when there are no global hooks repoDir := t.TempDir() markerFile := filepath.Join(repoDir, "repo-only") @@ -596,7 +573,7 @@ command = "`+touchCmd(markerFile)+`" } func TestHookRunnerRepoHookDoesNotFireForOtherRepo(t *testing.T) { - skipOnWindows(t) + // A repo's hooks should not fire for events from a different repo repoA := t.TempDir() repoB := t.TempDir() From ecae764586e9f3b5a11c2b2c76300815161312b6 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sat, 31 Jan 2026 17:31:43 -0600 Subject: [PATCH 19/19] Fix Windows hook tests: forward slashes, path resolution, noopCmd - Use forward slashes in touchCmd/pwdCmd paths to avoid TOML backslash escaping issues when embedding commands in .roborev.toml - Resolve 8.3 short paths via EvalSymlinks and use case-insensitive comparison for working directory test - Use -NoNewline in pwdCmd and TrimSpace to handle CRLF - Add noopCmd() helper (Write-Output on Windows, true on Unix) for log capture tests Co-Authored-By: Claude Opus 4.5 --- internal/daemon/hooks_test.go | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 3abac2a..fd0d374 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -18,11 +18,21 @@ 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. - return "New-Item -ItemType File -Force -Path '" + path + "'" + // Use forward slashes — PowerShell resolves them correctly. + return "New-Item -ItemType File -Force -Path '" + filepath.ToSlash(path) + "'" } return "touch " + path } @@ -30,7 +40,7 @@ func touchCmd(path string) string { // 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 '" + path + "'" + return "(Get-Location).Path | Set-Content -NoNewline '" + filepath.ToSlash(path) + "'" } return "pwd > " + path } @@ -386,9 +396,18 @@ func TestHookRunnerWorkingDirectory(t *testing.T) { for time.Now().Before(deadline) { data, err := os.ReadFile(markerFile) if err == nil { - got := filepath.Clean(string(data[:len(data)-1])) // trim newline + got := filepath.Clean(strings.TrimSpace(string(data))) want := filepath.Clean(tmpDir) - if got != want { + // 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 @@ -658,8 +677,8 @@ func TestHandleEventLogsWhenHooksFired(t *testing.T) { cfg := &config.Config{ Hooks: []config.HookConfig{ - {Event: "review.completed", Command: "true"}, - {Event: "review.completed", Command: "true"}, + {Event: "review.completed", Command: noopCmd()}, + {Event: "review.completed", Command: noopCmd()}, }, }