From 52c359d24d7e321506352162bfb0d490fc1ca789 Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Thu, 12 Feb 2026 21:44:18 -0500 Subject: [PATCH 01/20] Add `roborev wait` command to wait for existing review jobs Implements #226. Adds a new `wait` command that waits for an already-running review job to complete without enqueuing a new one. This provides a token-efficient way for external coding agents to wait for reviews in a review-fix refinement loop. The command supports looking up jobs by git ref (resolved to SHA) or by job ID, with flags for --sha, --job, --quiet, and --timeout. Exit codes: 0 (pass), 1 (fail), 3 (timeout), 4 (job not found). Also extends waitForJob() with an optional timeout parameter. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 195 +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 192 insertions(+), 3 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 4397247b..a273d91e 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -57,6 +57,7 @@ func main() { rootCmd.AddCommand(initCmd()) rootCmd.AddCommand(reviewCmd()) + rootCmd.AddCommand(waitCmd()) rootCmd.AddCommand(statusCmd()) rootCmd.AddCommand(listCmd()) rootCmd.AddCommand(showCmd()) @@ -1043,7 +1044,7 @@ Examples: // If --wait, poll until job completes and show result if wait { - err := waitForJob(cmd, serverAddr, job.ID, quiet) + err := waitForJob(cmd, serverAddr, job.ID, quiet, 0) // Only silence Cobra's error output for exitError (verdict-based exit codes) // Keep error output for actual failures (network errors, job not found, etc.) if _, isExitErr := err.(*exitError); isExitErr { @@ -1147,9 +1148,10 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName return nil } -// waitForJob polls until a job completes and displays the review +// waitForJob polls until a job completes and displays the review. +// If timeout > 0, returns exitError{3} when the deadline is exceeded. // Uses the provided serverAddr to ensure we poll the same daemon that received the job. -func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool) error { +func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool, timeout time.Duration) error { client := &http.Client{Timeout: 5 * time.Second} if !quiet { @@ -1162,7 +1164,18 @@ func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool) unknownStatusCount := 0 const maxUnknownRetries = 10 // Give up after 10 consecutive unknown statuses + var deadline time.Time + if timeout > 0 { + deadline = time.Now().Add(timeout) + } + for { + if timeout > 0 && time.Now().After(deadline) { + if !quiet { + cmd.Printf(" timed out after %s!\n", timeout) + } + return &exitError{code: 3} + } resp, err := client.Get(fmt.Sprintf("%s/api/jobs?id=%d", serverAddr, jobID)) if err != nil { return fmt.Errorf("failed to check job status: %w", err) @@ -1290,6 +1303,182 @@ func (e *exitError) Error() string { return fmt.Sprintf("exit code %d", e.code) } +func waitCmd() *cobra.Command { + var ( + shaFlag string + forceJobID bool + quiet bool + timeout int + ) + + cmd := &cobra.Command{ + Use: "wait [job_id|sha]", + Short: "Wait for an existing review job to complete", + Long: `Wait for an already-running review job to complete, without enqueuing a new one. + +When using an external coding agent to perform a review-fix refinement loop, +wait provides a token-efficient method for letting the agent wait for a roborev +review to complete. The post-commit hook triggers the review, and the agent +calls wait to block until the result is ready. + +The argument can be a job ID (numeric) or a git ref (commit SHA, branch, HEAD). +If no argument is given, defaults to HEAD. + +Exit codes: + 0 Review passed (verdict PASS) + 1 Review failed (verdict FAIL), or job failed/canceled + 3 Timeout exceeded (--timeout) + 4 No job found for the given SHA or job ID + +Examples: + roborev wait # Wait for most recent job for HEAD + roborev wait abc123 # Wait for most recent job for commit + roborev wait 42 # Job ID (if "42" is not a valid git ref) + roborev wait --job 42 # Force as job ID + roborev wait --sha HEAD~1 # Wait for job matching HEAD~1 + roborev wait --timeout 60 # Give up after 60 seconds`, + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + // In quiet mode, suppress cobra's error output + if quiet { + cmd.SilenceErrors = true + cmd.SilenceUsage = true + } + + // Validate: positional arg + --sha is ambiguous + if len(args) > 0 && shaFlag != "" { + return fmt.Errorf("cannot use both a positional argument and --sha") + } + + // Ensure daemon is running (and restart if version mismatch) + if err := ensureDaemon(); err != nil { + return fmt.Errorf("daemon not running: %w", err) + } + + addr := getDaemonAddr() + + // Resolve the target to a job ID + var jobID int64 + var displayRef string + + if shaFlag != "" { + // --sha flag: resolve ref to SHA, look up job + displayRef = shaFlag + id, err := lookupJobBySHA(addr, shaFlag) + if err != nil { + return handleWaitLookupErr(cmd, err, displayRef, quiet) + } + jobID = id + } else if len(args) > 0 { + arg := args[0] + if forceJobID { + // --job flag: treat as job ID directly + id, err := strconv.ParseInt(arg, 10, 64) + if err != nil { + return fmt.Errorf("invalid job ID: %s", arg) + } + jobID = id + } else { + // Try to resolve as git ref first (handles numeric SHAs like "123456") + var resolvedSHA string + if root, err := git.GetRepoRoot("."); err == nil { + if resolved, err := git.ResolveSHA(root, arg); err == nil { + resolvedSHA = resolved + } + } + if resolvedSHA != "" { + // Valid git ref — look up job by SHA + displayRef = arg + id, err := lookupJobBySHA(addr, arg) + if err != nil { + return handleWaitLookupErr(cmd, err, displayRef, quiet) + } + jobID = id + } else if id, err := strconv.ParseInt(arg, 10, 64); err == nil { + // Not a valid git ref but numeric — treat as job ID + jobID = id + } else { + return fmt.Errorf("argument %q is not a valid git ref or job ID", arg) + } + } + } else { + // No arg, no --sha: default to HEAD + displayRef = "HEAD" + id, err := lookupJobBySHA(addr, "HEAD") + if err != nil { + return handleWaitLookupErr(cmd, err, displayRef, quiet) + } + jobID = id + } + + timeoutDur := time.Duration(timeout) * time.Second + err := waitForJob(cmd, addr, jobID, quiet, timeoutDur) + if _, isExitErr := err.(*exitError); isExitErr { + cmd.SilenceErrors = true + cmd.SilenceUsage = true + } + return err + }, + } + + cmd.Flags().StringVar(&shaFlag, "sha", "", "git ref to find the most recent job for") + cmd.Flags().BoolVar(&forceJobID, "job", false, "force argument to be treated as job ID") + cmd.Flags().BoolVarP(&quiet, "quiet", "q", false, "suppress output (for use in hooks)") + cmd.Flags().IntVar(&timeout, "timeout", 0, "max wait time in seconds (0 = no timeout)") + + return cmd +} + +// lookupJobBySHA resolves a git ref to a SHA and finds the most recent job for it. +// Returns exitError{4} if no job is found. +func lookupJobBySHA(addr, ref string) (int64, error) { + // Resolve git ref to full SHA + sha := ref + if root, err := git.GetRepoRoot("."); err == nil { + if resolved, err := git.ResolveSHA(root, ref); err == nil { + sha = resolved + } + } + + client := &http.Client{Timeout: 5 * time.Second} + resp, err := client.Get(fmt.Sprintf("%s/api/jobs?git_ref=%s&limit=1", addr, url.QueryEscape(sha))) + if err != nil { + return 0, fmt.Errorf("failed to connect to daemon: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return 0, fmt.Errorf("server error looking up job (%d): %s", resp.StatusCode, body) + } + + var result struct { + Jobs []storage.ReviewJob `json:"jobs"` + } + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return 0, fmt.Errorf("failed to parse job list: %w", err) + } + + if len(result.Jobs) == 0 { + return 0, &exitError{code: 4} + } + + return result.Jobs[0].ID, nil +} + +// handleWaitLookupErr handles errors from lookupJobBySHA in the wait command. +// For exitError{4} (not found), prints a user-facing message when not quiet. +func handleWaitLookupErr(cmd *cobra.Command, err error, ref string, quiet bool) error { + if exitErr, ok := err.(*exitError); ok { + if exitErr.code == 4 && !quiet { + cmd.Printf("No job found for %s\n", ref) + } + cmd.SilenceErrors = true + cmd.SilenceUsage = true + } + return err +} + func statusCmd() *cobra.Command { return &cobra.Command{ Use: "status", From af99741af465f85735bc0216a1e85c9f83775baf Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Thu, 12 Feb 2026 21:54:43 -0500 Subject: [PATCH 02/20] Fix wait command: repo scoping, exit code 4, --job validation Address review findings for the wait command: 1. Add repo filter to lookupJobBySHA to avoid cross-repo job mismatch when the daemon tracks multiple repos. Follows the same normalization pattern as findJobForCommit. 2. Map "job not found" from waitForJob to exit code 4 in waitCmd, matching the documented behavior without changing reviewCmd. 3. Error when --job is set without a positional argument instead of silently falling through to the HEAD default. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 47 +++++++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index a273d91e..a7ee6cf6 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1345,10 +1345,13 @@ Examples: cmd.SilenceUsage = true } - // Validate: positional arg + --sha is ambiguous + // Validate flag/arg combinations if len(args) > 0 && shaFlag != "" { return fmt.Errorf("cannot use both a positional argument and --sha") } + if forceJobID && len(args) == 0 { + return fmt.Errorf("--job requires a job ID argument") + } // Ensure daemon is running (and restart if version mismatch) if err := ensureDaemon(); err != nil { @@ -1413,9 +1416,21 @@ Examples: timeoutDur := time.Duration(timeout) * time.Second err := waitForJob(cmd, addr, jobID, quiet, timeoutDur) - if _, isExitErr := err.(*exitError); isExitErr { - cmd.SilenceErrors = true - cmd.SilenceUsage = true + if err != nil { + // Map "job not found" to exit code 4 (waitForJob returns + // a plain error for this case to stay compatible with reviewCmd) + if strings.Contains(err.Error(), "not found") { + if !quiet { + cmd.Printf("No job found for job %d\n", jobID) + } + cmd.SilenceErrors = true + cmd.SilenceUsage = true + return &exitError{code: 4} + } + if _, isExitErr := err.(*exitError); isExitErr { + cmd.SilenceErrors = true + cmd.SilenceUsage = true + } } return err }, @@ -1430,18 +1445,38 @@ Examples: } // lookupJobBySHA resolves a git ref to a SHA and finds the most recent job for it. +// Scopes the query to the current repo to avoid cross-repo mismatch. // Returns exitError{4} if no job is found. func lookupJobBySHA(addr, ref string) (int64, error) { - // Resolve git ref to full SHA + // Resolve git ref to full SHA and get repo root sha := ref + var repoRoot string if root, err := git.GetRepoRoot("."); err == nil { + repoRoot = root if resolved, err := git.ResolveSHA(root, ref); err == nil { sha = resolved } } + // Normalize repo path to handle symlinks/relative paths consistently + normalizedRepo := repoRoot + if normalizedRepo != "" { + if resolved, err := filepath.EvalSymlinks(normalizedRepo); err == nil { + normalizedRepo = resolved + } + if abs, err := filepath.Abs(normalizedRepo); err == nil { + normalizedRepo = abs + } + } + client := &http.Client{Timeout: 5 * time.Second} - resp, err := client.Get(fmt.Sprintf("%s/api/jobs?git_ref=%s&limit=1", addr, url.QueryEscape(sha))) + + // Query by git_ref and repo to avoid matching jobs from different repos + queryURL := fmt.Sprintf("%s/api/jobs?git_ref=%s&limit=1", addr, url.QueryEscape(sha)) + if normalizedRepo != "" { + queryURL += "&repo=" + url.QueryEscape(normalizedRepo) + } + resp, err := client.Get(queryURL) if err != nil { return 0, fmt.Errorf("failed to connect to daemon: %w", err) } From d57fe0f229ff3826d62acc570dfa3c7d505768a3 Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Thu, 12 Feb 2026 22:04:37 -0500 Subject: [PATCH 03/20] Fix wait command: worktree support and sentinel error for job lookup Address review findings: 1. Use GetMainRepoRoot instead of GetRepoRoot in lookupJobBySHA so that repo scoping matches the daemon's storage path, which always uses the main repo root (not the worktree path). 2. Replace brittle strings.Contains "not found" matching with a sentinel errJobNotFound error and errors.Is check, preventing unrelated errors (e.g. "no review found") from being misclassified as exit code 4. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index a7ee6cf6..0df59e72 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -311,6 +311,9 @@ func startDaemon() error { // ErrDaemonNotRunning indicates no daemon runtime file was found var ErrDaemonNotRunning = fmt.Errorf("daemon not running (no runtime file found)") +// errJobNotFound indicates a job ID was not found during polling +var errJobNotFound = fmt.Errorf("job not found") + // stopDaemon stops any running daemons. // Returns ErrDaemonNotRunning if no daemon runtime files are found. func stopDaemon() error { @@ -1198,7 +1201,7 @@ func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool, resp.Body.Close() if len(jobsResp.Jobs) == 0 { - return fmt.Errorf("job %d not found", jobID) + return fmt.Errorf("%w: %d", errJobNotFound, jobID) } job := jobsResp.Jobs[0] @@ -1417,9 +1420,9 @@ Examples: timeoutDur := time.Duration(timeout) * time.Second err := waitForJob(cmd, addr, jobID, quiet, timeoutDur) if err != nil { - // Map "job not found" to exit code 4 (waitForJob returns + // Map errJobNotFound to exit code 4 (waitForJob returns // a plain error for this case to stay compatible with reviewCmd) - if strings.Contains(err.Error(), "not found") { + if errors.Is(err, errJobNotFound) { if !quiet { cmd.Printf("No job found for job %d\n", jobID) } @@ -1448,10 +1451,12 @@ Examples: // Scopes the query to the current repo to avoid cross-repo mismatch. // Returns exitError{4} if no job is found. func lookupJobBySHA(addr, ref string) (int64, error) { - // Resolve git ref to full SHA and get repo root + // Resolve git ref to full SHA and get main repo root. + // Use GetMainRepoRoot to match daemon storage, which always + // records the main repo path (not the worktree path). sha := ref var repoRoot string - if root, err := git.GetRepoRoot("."); err == nil { + if root, err := git.GetMainRepoRoot("."); err == nil { repoRoot = root if resolved, err := git.ResolveSHA(root, ref); err == nil { sha = resolved From 8a5b7becde024aa15e0284fa2a1acbd4264fda1e Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Thu, 12 Feb 2026 22:18:00 -0500 Subject: [PATCH 04/20] Fix wait worktree SHA resolution and add tests Address review findings: 1. Resolve SHA from GetRepoRoot (worktree-aware, where the user is working) but use GetMainRepoRoot for repo filtering (to match daemon storage). Previously both used GetMainRepoRoot, which resolved HEAD to the wrong commit in worktrees. 2. Add wait_test.go with coverage for: - --job flag requires argument - --sha and positional arg conflict - Exit code 4 when no job found (by SHA and by job ID) - Exit code 4 quiet mode suppresses output - Exit code 3 on timeout - Exit code 0 for passing review - Exit code 1 for failing review - "no review found" error is NOT remapped to exit code 4 Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 16 +- cmd/roborev/wait_test.go | 354 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 364 insertions(+), 6 deletions(-) create mode 100644 cmd/roborev/wait_test.go diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 0df59e72..b7dabc76 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1451,18 +1451,22 @@ Examples: // Scopes the query to the current repo to avoid cross-repo mismatch. // Returns exitError{4} if no job is found. func lookupJobBySHA(addr, ref string) (int64, error) { - // Resolve git ref to full SHA and get main repo root. - // Use GetMainRepoRoot to match daemon storage, which always - // records the main repo path (not the worktree path). + // Resolve SHA from the worktree root (where the user is working) + // so that refs like HEAD resolve to the correct commit. sha := ref - var repoRoot string - if root, err := git.GetMainRepoRoot("."); err == nil { - repoRoot = root + if root, err := git.GetRepoRoot("."); err == nil { if resolved, err := git.ResolveSHA(root, ref); err == nil { sha = resolved } } + // Use GetMainRepoRoot for repo filtering to match daemon storage, + // which always records the main repo path (not the worktree path). + var repoRoot string + if root, err := git.GetMainRepoRoot("."); err == nil { + repoRoot = root + } + // Normalize repo path to handle symlinks/relative paths consistently normalizedRepo := repoRoot if normalizedRepo != "" { diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go new file mode 100644 index 00000000..7db8fd64 --- /dev/null +++ b/cmd/roborev/wait_test.go @@ -0,0 +1,354 @@ +package main + +// Tests for the wait command + +import ( + "bytes" + "encoding/json" + "net/http" + "strings" + "testing" + + "github.com/roborev-dev/roborev/internal/storage" +) + +func TestWaitJobFlagRequiresArgument(t *testing.T) { + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + defer cleanup() + + chdir(t, repo.Dir) + + var stderr bytes.Buffer + cmd := waitCmd() + cmd.SetErr(&stderr) + cmd.SetArgs([]string{"--job"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error when --job is used without argument") + } + if !strings.Contains(err.Error(), "--job requires a job ID argument") { + t.Errorf("expected '--job requires a job ID argument' error, got: %v", err) + } +} + +func TestWaitSHAAndPositionalArgConflict(t *testing.T) { + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--sha", "HEAD", "42"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error when both --sha and positional arg are given") + } + if !strings.Contains(err.Error(), "cannot use both") { + t.Errorf("expected 'cannot use both' error, got: %v", err) + } +} + +func TestWaitExitCode4WhenNoJobFound(t *testing.T) { + setupFastPolling(t) + + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/jobs" { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{}, + "has_more": false, + }) + return + } + })) + defer cleanup() + + chdir(t, repo.Dir) + + t.Run("no job for SHA exits 4", func(t *testing.T) { + var stdout bytes.Buffer + cmd := waitCmd() + cmd.SetOut(&stdout) + cmd.SetArgs([]string{"--sha", "HEAD"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected exit error for missing job") + } + exitErr, ok := err.(*exitError) + if !ok { + t.Fatalf("expected exitError, got: %T %v", err, err) + } + if exitErr.code != 4 { + t.Errorf("expected exit code 4, got: %d", exitErr.code) + } + if !strings.Contains(stdout.String(), "No job found") { + t.Errorf("expected 'No job found' message, got: %q", stdout.String()) + } + }) + + t.Run("no job for SHA quiet exits 4 with no output", func(t *testing.T) { + var stdout, stderr bytes.Buffer + cmd := waitCmd() + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected exit error for missing job") + } + exitErr, ok := err.(*exitError) + if !ok { + t.Fatalf("expected exitError, got: %T %v", err, err) + } + if exitErr.code != 4 { + t.Errorf("expected exit code 4, got: %d", exitErr.code) + } + if stdout.String() != "" { + t.Errorf("expected no stdout in quiet mode, got: %q", stdout.String()) + } + }) +} + +func TestWaitExitCode4WhenJobIDNotFound(t *testing.T) { + setupFastPolling(t) + + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/jobs" { + // Return empty jobs list for any query + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{}, + "has_more": false, + }) + return + } + })) + defer cleanup() + + chdir(t, repo.Dir) + + var stdout bytes.Buffer + cmd := waitCmd() + cmd.SetOut(&stdout) + cmd.SetArgs([]string{"--job", "99999"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected exit error for missing job ID") + } + exitErr, ok := err.(*exitError) + if !ok { + t.Fatalf("expected exitError, got: %T %v", err, err) + } + if exitErr.code != 4 { + t.Errorf("expected exit code 4, got: %d", exitErr.code) + } +} + +func TestWaitExitCode3OnTimeout(t *testing.T) { + setupFastPolling(t) + + repo := newTestGitRepo(t) + sha := repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/jobs" { + // If querying by git_ref, return the job + if r.URL.Query().Get("git_ref") != "" { + job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "running"} + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{job}, + "has_more": false, + }) + return + } + // If querying by id, always return running + job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "running"} + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{job}, + "has_more": false, + }) + return + } + })) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--sha", "HEAD", "--timeout", "1", "--quiet"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected exit error for timeout") + } + exitErr, ok := err.(*exitError) + if !ok { + t.Fatalf("expected exitError, got: %T %v", err, err) + } + if exitErr.code != 3 { + t.Errorf("expected exit code 3 for timeout, got: %d", exitErr.code) + } +} + +func TestWaitPassingReview(t *testing.T) { + setupFastPolling(t) + + repo := newTestGitRepo(t) + sha := repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/jobs" { + if r.URL.Query().Get("git_ref") != "" { + job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{job}, + "has_more": false, + }) + return + } + job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{job}, + "has_more": false, + }) + return + } + if r.URL.Path == "/api/review" { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "No issues found."}) + return + } + })) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) + err := cmd.Execute() + + if err != nil { + t.Errorf("expected exit 0 for passing review, got error: %v", err) + } +} + +func TestWaitFailingReview(t *testing.T) { + setupFastPolling(t) + + repo := newTestGitRepo(t) + sha := repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/jobs" { + if r.URL.Query().Get("git_ref") != "" { + job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{job}, + "has_more": false, + }) + return + } + job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{job}, + "has_more": false, + }) + return + } + if r.URL.Path == "/api/review" { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "Found 2 issues:\n1. Bug\n2. Missing check"}) + return + } + })) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected exit 1 for failing review") + } + exitErr, ok := err.(*exitError) + if !ok { + t.Fatalf("expected exitError, got: %T %v", err, err) + } + if exitErr.code != 1 { + t.Errorf("expected exit code 1, got: %d", exitErr.code) + } +} + +func TestWaitReviewErrorNotRemappedToCode4(t *testing.T) { + setupFastPolling(t) + + repo := newTestGitRepo(t) + sha := repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/jobs" { + if r.URL.Query().Get("git_ref") != "" { + job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{job}, + "has_more": false, + }) + return + } + job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{job}, + "has_more": false, + }) + return + } + if r.URL.Path == "/api/review" { + // Return 404 — "no review found" should NOT become exit code 4 + w.WriteHeader(http.StatusNotFound) + return + } + })) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error when review not found") + } + // This should NOT be exit code 4 — it's a "no review found" error, + // not a "no job found" error + if exitErr, ok := err.(*exitError); ok && exitErr.code == 4 { + t.Error("'no review found' error should not be remapped to exit code 4") + } +} From 83d03adcd4a797e11088bb103ec2cffb903fb308 Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Thu, 12 Feb 2026 22:28:53 -0500 Subject: [PATCH 05/20] Add worktree test for wait command SHA/repo resolution Addresses review findings from jobs 127/128. Adds a test that creates a main repo + worktree, runs wait --sha HEAD from the worktree, and asserts that: - git_ref is resolved from the worktree HEAD (not main repo) - repo query param uses the main repo root (not worktree path) This directly covers the core worktree fix behavior. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/wait_test.go | 79 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index 7db8fd64..4315664b 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -6,6 +6,8 @@ import ( "bytes" "encoding/json" "net/http" + "net/url" + "os" "strings" "testing" @@ -352,3 +354,80 @@ func TestWaitReviewErrorNotRemappedToCode4(t *testing.T) { t.Error("'no review found' error should not be remapped to exit code 4") } } + +func TestWaitWorktreeResolvesRefFromWorktreeAndRepoFromMain(t *testing.T) { + setupFastPolling(t) + + // Create main repo with initial commit + repo := newTestGitRepo(t) + mainSHA := repo.CommitFile("file.txt", "content", "initial on main") + + // Create a worktree on a new branch with a different commit + worktreeDir := t.TempDir() + os.Remove(worktreeDir) // git worktree add needs to create it + repo.Run("worktree", "add", "-b", "wt-branch", worktreeDir) + + // Make a new commit in the worktree so HEAD differs from main + wtRepo := &TestGitRepo{Dir: worktreeDir, t: t} + wtSHA := wtRepo.CommitFile("wt-file.txt", "worktree content", "commit in worktree") + + // Sanity check: SHAs should differ + if mainSHA == wtSHA { + t.Fatal("expected different SHAs for main and worktree commits") + } + + var lookupQuery string + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/jobs" { + // Capture only the lookup query (has git_ref), not the poll query (has id) + if r.URL.Query().Get("git_ref") != "" { + lookupQuery = r.URL.RawQuery + } + // Return a job so we can proceed to waitForJob + job := storage.ReviewJob{ID: 1, GitRef: wtSHA, Agent: "test", Status: "done"} + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": []storage.ReviewJob{job}, + "has_more": false, + }) + return + } + if r.URL.Path == "/api/review" { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "No issues found."}) + return + } + })) + defer cleanup() + + // Run wait from the worktree directory + chdir(t, worktreeDir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) + err := cmd.Execute() + + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + + if lookupQuery == "" { + t.Fatal("expected a lookup query with git_ref, but none was captured") + } + + // git_ref should be the worktree's HEAD (wtSHA), not the main repo's HEAD + if !strings.Contains(lookupQuery, "git_ref="+url.QueryEscape(wtSHA)) { + t.Errorf("expected git_ref=%s (worktree HEAD) in query, got: %s", wtSHA, lookupQuery) + } + if strings.Contains(lookupQuery, "git_ref="+url.QueryEscape(mainSHA)) { + t.Errorf("git_ref should NOT be main repo HEAD %s, got: %s", mainSHA, lookupQuery) + } + + // repo should be the main repo path, not the worktree path + if strings.Contains(lookupQuery, url.QueryEscape(worktreeDir)) { + t.Errorf("repo param should be main repo path, not worktree path; got: %s", lookupQuery) + } + if !strings.Contains(lookupQuery, url.QueryEscape(repo.Dir)) { + t.Errorf("expected repo=%s (main repo) in query, got: %s", repo.Dir, lookupQuery) + } +} From 00f7280c7edc7661028ccb3321d0cb64fa38956b Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Thu, 12 Feb 2026 22:52:40 -0500 Subject: [PATCH 06/20] Style: align wait command code with existing conventions - Remove trailing periods from doc comments (matches rest of codebase) - Rename errJobNotFound to ErrJobNotFound (matches ErrDaemonNotRunning) - Use "daemon returned " error format (matches existing HTTP errors) - Drop error wrapping in connection error (matches existing pattern) - Capitalize subtest descriptions (matches existing test style) Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 30 +++++++++++++++--------------- cmd/roborev/wait_test.go | 4 ++-- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index b7dabc76..acfd6094 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -311,8 +311,8 @@ func startDaemon() error { // ErrDaemonNotRunning indicates no daemon runtime file was found var ErrDaemonNotRunning = fmt.Errorf("daemon not running (no runtime file found)") -// errJobNotFound indicates a job ID was not found during polling -var errJobNotFound = fmt.Errorf("job not found") +// ErrJobNotFound indicates a job ID was not found during polling +var ErrJobNotFound = fmt.Errorf("job not found") // stopDaemon stops any running daemons. // Returns ErrDaemonNotRunning if no daemon runtime files are found. @@ -1151,9 +1151,9 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName return nil } -// waitForJob polls until a job completes and displays the review. -// If timeout > 0, returns exitError{3} when the deadline is exceeded. -// Uses the provided serverAddr to ensure we poll the same daemon that received the job. +// waitForJob polls until a job completes and displays the review +// If timeout > 0, returns exitError{3} when the deadline is exceeded +// Uses the provided serverAddr to ensure we poll the same daemon that received the job func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool, timeout time.Duration) error { client := &http.Client{Timeout: 5 * time.Second} @@ -1201,7 +1201,7 @@ func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool, resp.Body.Close() if len(jobsResp.Jobs) == 0 { - return fmt.Errorf("%w: %d", errJobNotFound, jobID) + return fmt.Errorf("%w: %d", ErrJobNotFound, jobID) } job := jobsResp.Jobs[0] @@ -1420,9 +1420,9 @@ Examples: timeoutDur := time.Duration(timeout) * time.Second err := waitForJob(cmd, addr, jobID, quiet, timeoutDur) if err != nil { - // Map errJobNotFound to exit code 4 (waitForJob returns + // Map ErrJobNotFound to exit code 4 (waitForJob returns // a plain error for this case to stay compatible with reviewCmd) - if errors.Is(err, errJobNotFound) { + if errors.Is(err, ErrJobNotFound) { if !quiet { cmd.Printf("No job found for job %d\n", jobID) } @@ -1447,9 +1447,9 @@ Examples: return cmd } -// lookupJobBySHA resolves a git ref to a SHA and finds the most recent job for it. -// Scopes the query to the current repo to avoid cross-repo mismatch. -// Returns exitError{4} if no job is found. +// lookupJobBySHA resolves a git ref to a SHA and finds the most recent job for it +// Scopes the query to the current repo to avoid cross-repo mismatch +// Returns exitError{4} if no job is found func lookupJobBySHA(addr, ref string) (int64, error) { // Resolve SHA from the worktree root (where the user is working) // so that refs like HEAD resolve to the correct commit. @@ -1487,13 +1487,13 @@ func lookupJobBySHA(addr, ref string) (int64, error) { } resp, err := client.Get(queryURL) if err != nil { - return 0, fmt.Errorf("failed to connect to daemon: %w", err) + return 0, fmt.Errorf("failed to connect to daemon (is it running?)") } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { body, _ := io.ReadAll(resp.Body) - return 0, fmt.Errorf("server error looking up job (%d): %s", resp.StatusCode, body) + return 0, fmt.Errorf("daemon returned %s: %s", resp.Status, strings.TrimSpace(string(body))) } var result struct { @@ -1510,8 +1510,8 @@ func lookupJobBySHA(addr, ref string) (int64, error) { return result.Jobs[0].ID, nil } -// handleWaitLookupErr handles errors from lookupJobBySHA in the wait command. -// For exitError{4} (not found), prints a user-facing message when not quiet. +// handleWaitLookupErr handles errors from lookupJobBySHA in the wait command +// For exitError{4} (not found), prints a user-facing message when not quiet func handleWaitLookupErr(cmd *cobra.Command, err error, ref string, quiet bool) error { if exitErr, ok := err.(*exitError); ok { if exitErr.code == 4 && !quiet { diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index 4315664b..0841d356 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -78,7 +78,7 @@ func TestWaitExitCode4WhenNoJobFound(t *testing.T) { chdir(t, repo.Dir) - t.Run("no job for SHA exits 4", func(t *testing.T) { + t.Run("No job for SHA exits 4", func(t *testing.T) { var stdout bytes.Buffer cmd := waitCmd() cmd.SetOut(&stdout) @@ -100,7 +100,7 @@ func TestWaitExitCode4WhenNoJobFound(t *testing.T) { } }) - t.Run("no job for SHA quiet exits 4 with no output", func(t *testing.T) { + t.Run("No job for SHA quiet exits 4 with no output", func(t *testing.T) { var stdout, stderr bytes.Buffer cmd := waitCmd() cmd.SetOut(&stdout) From a1e37a4978255708c222cddbd6708ba53185a204 Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Thu, 12 Feb 2026 22:57:41 -0500 Subject: [PATCH 07/20] Add test for wait command lookup error path Covers non-200 daemon response during SHA lookup, verifying the error message format includes the HTTP status code. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/wait_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index 0841d356..12411f75 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -355,6 +355,35 @@ func TestWaitReviewErrorNotRemappedToCode4(t *testing.T) { } } +func TestWaitLookupNon200Response(t *testing.T) { + setupFastPolling(t) + + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/jobs" { + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("database locked")) + return + } + })) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--sha", "HEAD"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error for non-200 response") + } + if !strings.Contains(err.Error(), "500") { + t.Errorf("expected error to contain status code, got: %v", err) + } +} + func TestWaitWorktreeResolvesRefFromWorktreeAndRepoFromMain(t *testing.T) { setupFastPolling(t) From 722e2e79e4ff21ed0638f3424edfaef25872f7e2 Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Thu, 12 Feb 2026 23:04:36 -0500 Subject: [PATCH 08/20] Strengthen wait lookup error test assertions Assert on full status line and response body content instead of just the status code number. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/wait_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index 12411f75..dd69267f 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -379,8 +379,11 @@ func TestWaitLookupNon200Response(t *testing.T) { if err == nil { t.Fatal("expected error for non-200 response") } - if !strings.Contains(err.Error(), "500") { - t.Errorf("expected error to contain status code, got: %v", err) + if !strings.Contains(err.Error(), "500 Internal Server Error") { + t.Errorf("expected error to contain status line, got: %v", err) + } + if !strings.Contains(err.Error(), "database locked") { + t.Errorf("expected error to contain response body, got: %v", err) } } From 461060a7d8442fe7f23332dcabfa632bb46639dd Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Sat, 14 Feb 2026 12:05:53 -0500 Subject: [PATCH 09/20] Refactor wait command: factor out helpers, add missing tests Split lookupJobBySHA into resolveGitContext + lookupJobByRef to eliminate double SHA resolution in the positional-arg code path. lookupJobBySHA is kept as a thin convenience wrapper. Add waitMockHandler and requireExitCode test helpers to reduce boilerplate across wait tests. Add three new test cases: - TestWaitInvalidPositionalArg (bad arg that is neither ref nor number) - TestWaitPositionalArgAsGitRef (HEAD as positional arg, not --sha) - TestWaitNumericFallbackToJobID (numeric arg treated as job ID) Co-authored-by: Cursor --- cmd/roborev/main.go | 63 +++++---- cmd/roborev/wait_test.go | 273 +++++++++++++++++++-------------------- 2 files changed, 170 insertions(+), 166 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index acfd6094..9767c777 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1386,16 +1386,11 @@ Examples: jobID = id } else { // Try to resolve as git ref first (handles numeric SHAs like "123456") - var resolvedSHA string - if root, err := git.GetRepoRoot("."); err == nil { - if resolved, err := git.ResolveSHA(root, arg); err == nil { - resolvedSHA = resolved - } - } - if resolvedSHA != "" { - // Valid git ref — look up job by SHA + sha, repoRoot, resolved := resolveGitContext(arg) + if resolved { + // Valid git ref — look up job by resolved SHA displayRef = arg - id, err := lookupJobBySHA(addr, arg) + id, err := lookupJobByRef(addr, sha, repoRoot) if err != nil { return handleWaitLookupErr(cmd, err, displayRef, quiet) } @@ -1447,43 +1442,51 @@ Examples: return cmd } -// lookupJobBySHA resolves a git ref to a SHA and finds the most recent job for it -// Scopes the query to the current repo to avoid cross-repo mismatch -// Returns exitError{4} if no job is found -func lookupJobBySHA(addr, ref string) (int64, error) { +// resolveGitContext resolves a git ref to a full SHA and determines the main +// repository root (normalized). These are the two values needed to query the +// daemon's job list: the resolved SHA for git_ref matching and the canonical +// repo path for scoping. resolved indicates whether the ref was successfully +// converted to a SHA; when false, sha is the raw ref string. +func resolveGitContext(ref string) (sha, repoRoot string, resolved bool) { // Resolve SHA from the worktree root (where the user is working) // so that refs like HEAD resolve to the correct commit. - sha := ref if root, err := git.GetRepoRoot("."); err == nil { - if resolved, err := git.ResolveSHA(root, ref); err == nil { - sha = resolved + if r, err := git.ResolveSHA(root, ref); err == nil { + sha = r + resolved = true } } + if !resolved { + sha = ref + } // Use GetMainRepoRoot for repo filtering to match daemon storage, // which always records the main repo path (not the worktree path). - var repoRoot string if root, err := git.GetMainRepoRoot("."); err == nil { repoRoot = root } // Normalize repo path to handle symlinks/relative paths consistently - normalizedRepo := repoRoot - if normalizedRepo != "" { - if resolved, err := filepath.EvalSymlinks(normalizedRepo); err == nil { - normalizedRepo = resolved + if repoRoot != "" { + if r, err := filepath.EvalSymlinks(repoRoot); err == nil { + repoRoot = r } - if abs, err := filepath.Abs(normalizedRepo); err == nil { - normalizedRepo = abs + if abs, err := filepath.Abs(repoRoot); err == nil { + repoRoot = abs } } + return sha, repoRoot, resolved +} + +// lookupJobByRef queries the daemon for the most recent job matching the given +// SHA, optionally scoped to a repo root. Returns exitError{4} if no job is found. +func lookupJobByRef(addr, sha, repoRoot string) (int64, error) { client := &http.Client{Timeout: 5 * time.Second} - // Query by git_ref and repo to avoid matching jobs from different repos queryURL := fmt.Sprintf("%s/api/jobs?git_ref=%s&limit=1", addr, url.QueryEscape(sha)) - if normalizedRepo != "" { - queryURL += "&repo=" + url.QueryEscape(normalizedRepo) + if repoRoot != "" { + queryURL += "&repo=" + url.QueryEscape(repoRoot) } resp, err := client.Get(queryURL) if err != nil { @@ -1510,6 +1513,14 @@ func lookupJobBySHA(addr, ref string) (int64, error) { return result.Jobs[0].ID, nil } +// lookupJobBySHA resolves a git ref to a SHA and finds the most recent job for it. +// Scopes the query to the current repo to avoid cross-repo mismatch. +// Returns exitError{4} if no job is found. +func lookupJobBySHA(addr, ref string) (int64, error) { + sha, repoRoot, _ := resolveGitContext(ref) + return lookupJobByRef(addr, sha, repoRoot) +} + // handleWaitLookupErr handles errors from lookupJobBySHA in the wait command // For exitError{4} (not found), prints a user-facing message when not quiet func handleWaitLookupErr(cmd *cobra.Command, err error, ref string, quiet bool) error { diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index dd69267f..40f15ed6 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -14,6 +14,46 @@ import ( "github.com/roborev-dev/roborev/internal/storage" ) +// waitMockHandler builds an HTTP handler for wait command tests. +// If job is non-nil it is returned for every /api/jobs query; otherwise an +// empty list is returned. If review is non-nil it is served on /api/review. +func waitMockHandler(job *storage.ReviewJob, review *storage.Review) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/jobs" { + var jobs []storage.ReviewJob + if job != nil { + jobs = []storage.ReviewJob{*job} + } + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(map[string]interface{}{ + "jobs": jobs, + "has_more": false, + }) + return + } + if r.URL.Path == "/api/review" && review != nil { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(review) + return + } + } +} + +// requireExitCode asserts err is an *exitError with the expected code. +func requireExitCode(t *testing.T, err error, code int) { + t.Helper() + if err == nil { + t.Fatalf("expected exit code %d, got nil error", code) + } + exitErr, ok := err.(*exitError) + if !ok { + t.Fatalf("expected *exitError with code %d, got %T: %v", code, err, err) + } + if exitErr.code != code { + t.Errorf("expected exit code %d, got: %d", code, exitErr.code) + } +} + func TestWaitJobFlagRequiresArgument(t *testing.T) { repo := newTestGitRepo(t) repo.CommitFile("file.txt", "content", "initial commit") @@ -58,22 +98,34 @@ func TestWaitSHAAndPositionalArgConflict(t *testing.T) { } } +func TestWaitInvalidPositionalArg(t *testing.T) { + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"not-a-ref-or-number"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error for invalid argument") + } + if !strings.Contains(err.Error(), "not a valid git ref or job ID") { + t.Errorf("expected 'not a valid git ref or job ID' error, got: %v", err) + } +} + func TestWaitExitCode4WhenNoJobFound(t *testing.T) { setupFastPolling(t) repo := newTestGitRepo(t) repo.CommitFile("file.txt", "content", "initial commit") - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/api/jobs" { - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(map[string]interface{}{ - "jobs": []storage.ReviewJob{}, - "has_more": false, - }) - return - } - })) + _, cleanup := setupMockDaemon(t, waitMockHandler(nil, nil)) defer cleanup() chdir(t, repo.Dir) @@ -85,16 +137,7 @@ func TestWaitExitCode4WhenNoJobFound(t *testing.T) { cmd.SetArgs([]string{"--sha", "HEAD"}) err := cmd.Execute() - if err == nil { - t.Fatal("expected exit error for missing job") - } - exitErr, ok := err.(*exitError) - if !ok { - t.Fatalf("expected exitError, got: %T %v", err, err) - } - if exitErr.code != 4 { - t.Errorf("expected exit code 4, got: %d", exitErr.code) - } + requireExitCode(t, err, 4) if !strings.Contains(stdout.String(), "No job found") { t.Errorf("expected 'No job found' message, got: %q", stdout.String()) } @@ -108,16 +151,7 @@ func TestWaitExitCode4WhenNoJobFound(t *testing.T) { cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) err := cmd.Execute() - if err == nil { - t.Fatal("expected exit error for missing job") - } - exitErr, ok := err.(*exitError) - if !ok { - t.Fatalf("expected exitError, got: %T %v", err, err) - } - if exitErr.code != 4 { - t.Errorf("expected exit code 4, got: %d", exitErr.code) - } + requireExitCode(t, err, 4) if stdout.String() != "" { t.Errorf("expected no stdout in quiet mode, got: %q", stdout.String()) } @@ -130,17 +164,7 @@ func TestWaitExitCode4WhenJobIDNotFound(t *testing.T) { repo := newTestGitRepo(t) repo.CommitFile("file.txt", "content", "initial commit") - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/api/jobs" { - // Return empty jobs list for any query - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(map[string]interface{}{ - "jobs": []storage.ReviewJob{}, - "has_more": false, - }) - return - } - })) + _, cleanup := setupMockDaemon(t, waitMockHandler(nil, nil)) defer cleanup() chdir(t, repo.Dir) @@ -151,16 +175,7 @@ func TestWaitExitCode4WhenJobIDNotFound(t *testing.T) { cmd.SetArgs([]string{"--job", "99999"}) err := cmd.Execute() - if err == nil { - t.Fatal("expected exit error for missing job ID") - } - exitErr, ok := err.(*exitError) - if !ok { - t.Fatalf("expected exitError, got: %T %v", err, err) - } - if exitErr.code != 4 { - t.Errorf("expected exit code 4, got: %d", exitErr.code) - } + requireExitCode(t, err, 4) } func TestWaitExitCode3OnTimeout(t *testing.T) { @@ -169,46 +184,18 @@ func TestWaitExitCode3OnTimeout(t *testing.T) { repo := newTestGitRepo(t) sha := repo.CommitFile("file.txt", "content", "initial commit") - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/api/jobs" { - // If querying by git_ref, return the job - if r.URL.Query().Get("git_ref") != "" { - job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "running"} - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(map[string]interface{}{ - "jobs": []storage.ReviewJob{job}, - "has_more": false, - }) - return - } - // If querying by id, always return running - job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "running"} - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(map[string]interface{}{ - "jobs": []storage.ReviewJob{job}, - "has_more": false, - }) - return - } - })) + job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "running"} + _, cleanup := setupMockDaemon(t, waitMockHandler(job, nil)) defer cleanup() chdir(t, repo.Dir) cmd := waitCmd() + // --timeout 1 is the minimum testable value (0 means no timeout) cmd.SetArgs([]string{"--sha", "HEAD", "--timeout", "1", "--quiet"}) err := cmd.Execute() - if err == nil { - t.Fatal("expected exit error for timeout") - } - exitErr, ok := err.(*exitError) - if !ok { - t.Fatalf("expected exitError, got: %T %v", err, err) - } - if exitErr.code != 3 { - t.Errorf("expected exit code 3 for timeout, got: %d", exitErr.code) - } + requireExitCode(t, err, 3) } func TestWaitPassingReview(t *testing.T) { @@ -217,31 +204,9 @@ func TestWaitPassingReview(t *testing.T) { repo := newTestGitRepo(t) sha := repo.CommitFile("file.txt", "content", "initial commit") - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/api/jobs" { - if r.URL.Query().Get("git_ref") != "" { - job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(map[string]interface{}{ - "jobs": []storage.ReviewJob{job}, - "has_more": false, - }) - return - } - job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(map[string]interface{}{ - "jobs": []storage.ReviewJob{job}, - "has_more": false, - }) - return - } - if r.URL.Path == "/api/review" { - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "No issues found."}) - return - } - })) + job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} + review := &storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "No issues found."} + _, cleanup := setupMockDaemon(t, waitMockHandler(job, review)) defer cleanup() chdir(t, repo.Dir) @@ -261,18 +226,57 @@ func TestWaitFailingReview(t *testing.T) { repo := newTestGitRepo(t) sha := repo.CommitFile("file.txt", "content", "initial commit") + job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} + review := &storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "Found 2 issues:\n1. Bug\n2. Missing check"} + _, cleanup := setupMockDaemon(t, waitMockHandler(job, review)) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) + err := cmd.Execute() + + requireExitCode(t, err, 1) +} + +func TestWaitPositionalArgAsGitRef(t *testing.T) { + setupFastPolling(t) + + repo := newTestGitRepo(t) + sha := repo.CommitFile("file.txt", "content", "initial commit") + + job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} + review := &storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "No issues found."} + _, cleanup := setupMockDaemon(t, waitMockHandler(job, review)) + defer cleanup() + + chdir(t, repo.Dir) + + // Pass HEAD as a positional arg (not --sha flag) to exercise the + // git-ref-first resolution path. + cmd := waitCmd() + cmd.SetArgs([]string{"HEAD", "--quiet"}) + err := cmd.Execute() + + if err != nil { + t.Errorf("expected exit 0 for positional git ref, got error: %v", err) + } +} + +func TestWaitNumericFallbackToJobID(t *testing.T) { + setupFastPolling(t) + + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + // "42" is not a valid git ref, so the wait command should fall back to + // treating it as a numeric job ID and query by id (not git_ref). + var lastJobsQuery string _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/api/jobs" { - if r.URL.Query().Get("git_ref") != "" { - job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(map[string]interface{}{ - "jobs": []storage.ReviewJob{job}, - "has_more": false, - }) - return - } - job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} + lastJobsQuery = r.URL.RawQuery + job := storage.ReviewJob{ID: 42, GitRef: "abc", Agent: "test", Status: "done"} w.WriteHeader(http.StatusOK) json.NewEncoder(w).Encode(map[string]interface{}{ "jobs": []storage.ReviewJob{job}, @@ -282,7 +286,7 @@ func TestWaitFailingReview(t *testing.T) { } if r.URL.Path == "/api/review" { w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "Found 2 issues:\n1. Bug\n2. Missing check"}) + json.NewEncoder(w).Encode(storage.Review{ID: 1, JobID: 42, Agent: "test", Output: "No issues found."}) return } })) @@ -291,18 +295,15 @@ func TestWaitFailingReview(t *testing.T) { chdir(t, repo.Dir) cmd := waitCmd() - cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) + cmd.SetArgs([]string{"42", "--quiet"}) err := cmd.Execute() - if err == nil { - t.Fatal("expected exit 1 for failing review") - } - exitErr, ok := err.(*exitError) - if !ok { - t.Fatalf("expected exitError, got: %T %v", err, err) + if err != nil { + t.Errorf("expected exit 0 for numeric arg as job ID, got: %v", err) } - if exitErr.code != 1 { - t.Errorf("expected exit code 1, got: %d", exitErr.code) + // The final poll should query by id=42, not git_ref=42 + if !strings.Contains(lastJobsQuery, "id=42") { + t.Errorf("expected query by id=42, got: %s", lastJobsQuery) } } @@ -312,27 +313,19 @@ func TestWaitReviewErrorNotRemappedToCode4(t *testing.T) { repo := newTestGitRepo(t) sha := repo.CommitFile("file.txt", "content", "initial commit") + // Job completes but /api/review returns 404 — this should NOT become + // exit code 4 (which is reserved for "no job found"). + job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/api/jobs" { - if r.URL.Query().Get("git_ref") != "" { - job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} - w.WriteHeader(http.StatusOK) - json.NewEncoder(w).Encode(map[string]interface{}{ - "jobs": []storage.ReviewJob{job}, - "has_more": false, - }) - return - } - job := storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} w.WriteHeader(http.StatusOK) json.NewEncoder(w).Encode(map[string]interface{}{ - "jobs": []storage.ReviewJob{job}, + "jobs": []storage.ReviewJob{*job}, "has_more": false, }) return } if r.URL.Path == "/api/review" { - // Return 404 — "no review found" should NOT become exit code 4 w.WriteHeader(http.StatusNotFound) return } From 431b2b42f918edfa691adda6ec3793e072059f1c Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Sat, 14 Feb 2026 12:30:40 -0500 Subject: [PATCH 10/20] Simplify wait command exit codes to 0 and 1 Use standard 0/1 exit semantics: 0 for PASS verdict, 1 for any failure (FAIL verdict, timeout, no job found, job error). Removes the non-standard exit codes 3 and 4 that added complexity without meaningful benefit -- agents can read stderr for failure details. Co-authored-by: Cursor --- cmd/roborev/main.go | 30 ++++++++++++++---------------- cmd/roborev/wait_test.go | 33 +++++++++++++++++---------------- 2 files changed, 31 insertions(+), 32 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 9767c777..b3843017 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1152,7 +1152,7 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName } // waitForJob polls until a job completes and displays the review -// If timeout > 0, returns exitError{3} when the deadline is exceeded +// If timeout > 0, returns exitError{1} when the deadline is exceeded // Uses the provided serverAddr to ensure we poll the same daemon that received the job func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool, timeout time.Duration) error { client := &http.Client{Timeout: 5 * time.Second} @@ -1177,7 +1177,7 @@ func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool, if !quiet { cmd.Printf(" timed out after %s!\n", timeout) } - return &exitError{code: 3} + return &exitError{code: 1} } resp, err := client.Get(fmt.Sprintf("%s/api/jobs?id=%d", serverAddr, jobID)) if err != nil { @@ -1328,10 +1328,8 @@ The argument can be a job ID (numeric) or a git ref (commit SHA, branch, HEAD). If no argument is given, defaults to HEAD. Exit codes: - 0 Review passed (verdict PASS) - 1 Review failed (verdict FAIL), or job failed/canceled - 3 Timeout exceeded (--timeout) - 4 No job found for the given SHA or job ID + 0 Review completed with verdict PASS + 1 Any failure (FAIL verdict, timeout, no job found, job error) Examples: roborev wait # Wait for most recent job for HEAD @@ -1415,15 +1413,15 @@ Examples: timeoutDur := time.Duration(timeout) * time.Second err := waitForJob(cmd, addr, jobID, quiet, timeoutDur) if err != nil { - // Map ErrJobNotFound to exit code 4 (waitForJob returns - // a plain error for this case to stay compatible with reviewCmd) + // Map ErrJobNotFound to exit 1 with a user-facing message + // (waitForJob returns a plain error to stay compatible with reviewCmd) if errors.Is(err, ErrJobNotFound) { if !quiet { cmd.Printf("No job found for job %d\n", jobID) } cmd.SilenceErrors = true cmd.SilenceUsage = true - return &exitError{code: 4} + return &exitError{code: 1} } if _, isExitErr := err.(*exitError); isExitErr { cmd.SilenceErrors = true @@ -1480,7 +1478,7 @@ func resolveGitContext(ref string) (sha, repoRoot string, resolved bool) { } // lookupJobByRef queries the daemon for the most recent job matching the given -// SHA, optionally scoped to a repo root. Returns exitError{4} if no job is found. +// SHA, optionally scoped to a repo root. Returns exitError{1} if no job is found. func lookupJobByRef(addr, sha, repoRoot string) (int64, error) { client := &http.Client{Timeout: 5 * time.Second} @@ -1507,7 +1505,7 @@ func lookupJobByRef(addr, sha, repoRoot string) (int64, error) { } if len(result.Jobs) == 0 { - return 0, &exitError{code: 4} + return 0, &exitError{code: 1} } return result.Jobs[0].ID, nil @@ -1515,17 +1513,17 @@ func lookupJobByRef(addr, sha, repoRoot string) (int64, error) { // lookupJobBySHA resolves a git ref to a SHA and finds the most recent job for it. // Scopes the query to the current repo to avoid cross-repo mismatch. -// Returns exitError{4} if no job is found. +// Returns exitError{1} if no job is found. func lookupJobBySHA(addr, ref string) (int64, error) { sha, repoRoot, _ := resolveGitContext(ref) return lookupJobByRef(addr, sha, repoRoot) } -// handleWaitLookupErr handles errors from lookupJobBySHA in the wait command -// For exitError{4} (not found), prints a user-facing message when not quiet +// handleWaitLookupErr handles errors from lookupJobByRef in the wait command. +// For exitError (not found), prints a user-facing message when not quiet. func handleWaitLookupErr(cmd *cobra.Command, err error, ref string, quiet bool) error { - if exitErr, ok := err.(*exitError); ok { - if exitErr.code == 4 && !quiet { + if _, ok := err.(*exitError); ok { + if !quiet { cmd.Printf("No job found for %s\n", ref) } cmd.SilenceErrors = true diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index 40f15ed6..7fac01bc 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -119,7 +119,7 @@ func TestWaitInvalidPositionalArg(t *testing.T) { } } -func TestWaitExitCode4WhenNoJobFound(t *testing.T) { +func TestWaitExitsWhenNoJobFound(t *testing.T) { setupFastPolling(t) repo := newTestGitRepo(t) @@ -130,20 +130,20 @@ func TestWaitExitCode4WhenNoJobFound(t *testing.T) { chdir(t, repo.Dir) - t.Run("No job for SHA exits 4", func(t *testing.T) { + t.Run("No job for SHA", func(t *testing.T) { var stdout bytes.Buffer cmd := waitCmd() cmd.SetOut(&stdout) cmd.SetArgs([]string{"--sha", "HEAD"}) err := cmd.Execute() - requireExitCode(t, err, 4) + requireExitCode(t, err, 1) if !strings.Contains(stdout.String(), "No job found") { t.Errorf("expected 'No job found' message, got: %q", stdout.String()) } }) - t.Run("No job for SHA quiet exits 4 with no output", func(t *testing.T) { + t.Run("No job for SHA quiet mode suppresses output", func(t *testing.T) { var stdout, stderr bytes.Buffer cmd := waitCmd() cmd.SetOut(&stdout) @@ -151,14 +151,14 @@ func TestWaitExitCode4WhenNoJobFound(t *testing.T) { cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) err := cmd.Execute() - requireExitCode(t, err, 4) + requireExitCode(t, err, 1) if stdout.String() != "" { t.Errorf("expected no stdout in quiet mode, got: %q", stdout.String()) } }) } -func TestWaitExitCode4WhenJobIDNotFound(t *testing.T) { +func TestWaitExitsWhenJobIDNotFound(t *testing.T) { setupFastPolling(t) repo := newTestGitRepo(t) @@ -175,10 +175,10 @@ func TestWaitExitCode4WhenJobIDNotFound(t *testing.T) { cmd.SetArgs([]string{"--job", "99999"}) err := cmd.Execute() - requireExitCode(t, err, 4) + requireExitCode(t, err, 1) } -func TestWaitExitCode3OnTimeout(t *testing.T) { +func TestWaitExitsOnTimeout(t *testing.T) { setupFastPolling(t) repo := newTestGitRepo(t) @@ -195,7 +195,7 @@ func TestWaitExitCode3OnTimeout(t *testing.T) { cmd.SetArgs([]string{"--sha", "HEAD", "--timeout", "1", "--quiet"}) err := cmd.Execute() - requireExitCode(t, err, 3) + requireExitCode(t, err, 1) } func TestWaitPassingReview(t *testing.T) { @@ -307,14 +307,16 @@ func TestWaitNumericFallbackToJobID(t *testing.T) { } } -func TestWaitReviewErrorNotRemappedToCode4(t *testing.T) { +func TestWaitReviewFetchErrorIsPlainError(t *testing.T) { setupFastPolling(t) repo := newTestGitRepo(t) sha := repo.CommitFile("file.txt", "content", "initial commit") - // Job completes but /api/review returns 404 — this should NOT become - // exit code 4 (which is reserved for "no job found"). + // Job completes but /api/review returns 404. This should surface as a + // plain error (from showReview), not an *exitError. Both map to exit 1 + // in practice, but the distinction matters for error reporting: plain + // errors print Cobra's "Error:" prefix while exitError silences it. job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/api/jobs" { @@ -341,10 +343,9 @@ func TestWaitReviewErrorNotRemappedToCode4(t *testing.T) { if err == nil { t.Fatal("expected error when review not found") } - // This should NOT be exit code 4 — it's a "no review found" error, - // not a "no job found" error - if exitErr, ok := err.(*exitError); ok && exitErr.code == 4 { - t.Error("'no review found' error should not be remapped to exit code 4") + // Should be a plain error, not an *exitError + if _, ok := err.(*exitError); ok { + t.Error("review fetch failure should be a plain error, not exitError") } } From 44fdddd06436ccc7dae458d9593c0ac058542620 Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Sat, 14 Feb 2026 21:49:34 -0500 Subject: [PATCH 11/20] Remove --timeout flag from wait command Drop the built-in timeout in favor of the shell's timeout command (e.g. `timeout 60 roborev wait`). This keeps the wait command minimal and avoids changes to the shared waitForJob function. Co-authored-by: Cursor --- cmd/roborev/main.go | 28 ++++++---------------------- cmd/roborev/wait_test.go | 20 -------------------- 2 files changed, 6 insertions(+), 42 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index b3843017..9c91fb69 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1047,7 +1047,7 @@ Examples: // If --wait, poll until job completes and show result if wait { - err := waitForJob(cmd, serverAddr, job.ID, quiet, 0) + err := waitForJob(cmd, serverAddr, job.ID, quiet) // Only silence Cobra's error output for exitError (verdict-based exit codes) // Keep error output for actual failures (network errors, job not found, etc.) if _, isExitErr := err.(*exitError); isExitErr { @@ -1152,9 +1152,8 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName } // waitForJob polls until a job completes and displays the review -// If timeout > 0, returns exitError{1} when the deadline is exceeded -// Uses the provided serverAddr to ensure we poll the same daemon that received the job -func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool, timeout time.Duration) error { +// Uses the provided serverAddr to ensure we poll the same daemon that received the job. +func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool) error { client := &http.Client{Timeout: 5 * time.Second} if !quiet { @@ -1167,18 +1166,7 @@ func waitForJob(cmd *cobra.Command, serverAddr string, jobID int64, quiet bool, unknownStatusCount := 0 const maxUnknownRetries = 10 // Give up after 10 consecutive unknown statuses - var deadline time.Time - if timeout > 0 { - deadline = time.Now().Add(timeout) - } - for { - if timeout > 0 && time.Now().After(deadline) { - if !quiet { - cmd.Printf(" timed out after %s!\n", timeout) - } - return &exitError{code: 1} - } resp, err := client.Get(fmt.Sprintf("%s/api/jobs?id=%d", serverAddr, jobID)) if err != nil { return fmt.Errorf("failed to check job status: %w", err) @@ -1311,7 +1299,6 @@ func waitCmd() *cobra.Command { shaFlag string forceJobID bool quiet bool - timeout int ) cmd := &cobra.Command{ @@ -1329,15 +1316,14 @@ If no argument is given, defaults to HEAD. Exit codes: 0 Review completed with verdict PASS - 1 Any failure (FAIL verdict, timeout, no job found, job error) + 1 Any failure (FAIL verdict, no job found, job error) Examples: roborev wait # Wait for most recent job for HEAD roborev wait abc123 # Wait for most recent job for commit roborev wait 42 # Job ID (if "42" is not a valid git ref) roborev wait --job 42 # Force as job ID - roborev wait --sha HEAD~1 # Wait for job matching HEAD~1 - roborev wait --timeout 60 # Give up after 60 seconds`, + roborev wait --sha HEAD~1 # Wait for job matching HEAD~1`, Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { // In quiet mode, suppress cobra's error output @@ -1410,8 +1396,7 @@ Examples: jobID = id } - timeoutDur := time.Duration(timeout) * time.Second - err := waitForJob(cmd, addr, jobID, quiet, timeoutDur) + err := waitForJob(cmd, addr, jobID, quiet) if err != nil { // Map ErrJobNotFound to exit 1 with a user-facing message // (waitForJob returns a plain error to stay compatible with reviewCmd) @@ -1435,7 +1420,6 @@ Examples: cmd.Flags().StringVar(&shaFlag, "sha", "", "git ref to find the most recent job for") cmd.Flags().BoolVar(&forceJobID, "job", false, "force argument to be treated as job ID") cmd.Flags().BoolVarP(&quiet, "quiet", "q", false, "suppress output (for use in hooks)") - cmd.Flags().IntVar(&timeout, "timeout", 0, "max wait time in seconds (0 = no timeout)") return cmd } diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index 7fac01bc..d07c2915 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -178,26 +178,6 @@ func TestWaitExitsWhenJobIDNotFound(t *testing.T) { requireExitCode(t, err, 1) } -func TestWaitExitsOnTimeout(t *testing.T) { - setupFastPolling(t) - - repo := newTestGitRepo(t) - sha := repo.CommitFile("file.txt", "content", "initial commit") - - job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "running"} - _, cleanup := setupMockDaemon(t, waitMockHandler(job, nil)) - defer cleanup() - - chdir(t, repo.Dir) - - cmd := waitCmd() - // --timeout 1 is the minimum testable value (0 means no timeout) - cmd.SetArgs([]string{"--sha", "HEAD", "--timeout", "1", "--quiet"}) - err := cmd.Execute() - - requireExitCode(t, err, 1) -} - func TestWaitPassingReview(t *testing.T) { setupFastPolling(t) From 39f5308cf038854d4019241b64aa3fec3b441de8 Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Sun, 15 Feb 2026 09:36:43 -0500 Subject: [PATCH 12/20] Refactor job lookup error handling in wait command Update the error handling in lookupJobByRef and handleWaitLookupErr to use ErrJobNotFound instead of exitError for improved clarity. This change standardizes the error reporting when no job is found, enhancing the user experience by providing a more descriptive error message. Co-authored-by: Cursor --- cmd/roborev/main.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 9c91fb69..7def598e 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1462,7 +1462,7 @@ func resolveGitContext(ref string) (sha, repoRoot string, resolved bool) { } // lookupJobByRef queries the daemon for the most recent job matching the given -// SHA, optionally scoped to a repo root. Returns exitError{1} if no job is found. +// SHA, optionally scoped to a repo root. Returns ErrJobNotFound if no job is found. func lookupJobByRef(addr, sha, repoRoot string) (int64, error) { client := &http.Client{Timeout: 5 * time.Second} @@ -1489,7 +1489,7 @@ func lookupJobByRef(addr, sha, repoRoot string) (int64, error) { } if len(result.Jobs) == 0 { - return 0, &exitError{code: 1} + return 0, fmt.Errorf("%w: %s", ErrJobNotFound, sha) } return result.Jobs[0].ID, nil @@ -1497,21 +1497,22 @@ func lookupJobByRef(addr, sha, repoRoot string) (int64, error) { // lookupJobBySHA resolves a git ref to a SHA and finds the most recent job for it. // Scopes the query to the current repo to avoid cross-repo mismatch. -// Returns exitError{1} if no job is found. +// Returns ErrJobNotFound if no job is found. func lookupJobBySHA(addr, ref string) (int64, error) { sha, repoRoot, _ := resolveGitContext(ref) return lookupJobByRef(addr, sha, repoRoot) } // handleWaitLookupErr handles errors from lookupJobByRef in the wait command. -// For exitError (not found), prints a user-facing message when not quiet. +// For ErrJobNotFound, prints a user-facing message when not quiet and returns exitError{1}. func handleWaitLookupErr(cmd *cobra.Command, err error, ref string, quiet bool) error { - if _, ok := err.(*exitError); ok { + if errors.Is(err, ErrJobNotFound) { if !quiet { cmd.Printf("No job found for %s\n", ref) } cmd.SilenceErrors = true cmd.SilenceUsage = true + return &exitError{code: 1} } return err } From 4631e2ae0ff8484ef25144f1c7b05561d8bedcbf Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Sun, 15 Feb 2026 11:20:03 -0500 Subject: [PATCH 13/20] Validate wait command inputs: reject invalid refs and non-positive job IDs lookupJobBySHA now fails with "invalid git ref" when the ref cannot be resolved, instead of silently querying with the raw string. The --job flag rejects IDs <= 0 with "invalid job ID" instead of flowing into the polling loop. Tests added for both paths. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 7 +++++-- cmd/roborev/wait_test.go | 42 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 7def598e..ee41dc34 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1364,7 +1364,7 @@ Examples: if forceJobID { // --job flag: treat as job ID directly id, err := strconv.ParseInt(arg, 10, 64) - if err != nil { + if err != nil || id <= 0 { return fmt.Errorf("invalid job ID: %s", arg) } jobID = id @@ -1499,7 +1499,10 @@ func lookupJobByRef(addr, sha, repoRoot string) (int64, error) { // Scopes the query to the current repo to avoid cross-repo mismatch. // Returns ErrJobNotFound if no job is found. func lookupJobBySHA(addr, ref string) (int64, error) { - sha, repoRoot, _ := resolveGitContext(ref) + sha, repoRoot, resolved := resolveGitContext(ref) + if !resolved { + return 0, fmt.Errorf("invalid git ref: %s", ref) + } return lookupJobByRef(addr, sha, repoRoot) } diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index d07c2915..07c2cf84 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -119,6 +119,48 @@ func TestWaitInvalidPositionalArg(t *testing.T) { } } +func TestWaitInvalidSHARef(t *testing.T) { + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--sha", "not-a-valid-ref"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error for invalid git ref") + } + if !strings.Contains(err.Error(), "invalid git ref") { + t.Errorf("expected 'invalid git ref' error, got: %v", err) + } +} + +func TestWaitJobIDRejectsNonPositive(t *testing.T) { + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--job", "0"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error for --job 0") + } + if !strings.Contains(err.Error(), "invalid job ID") { + t.Errorf("expected 'invalid job ID' error, got: %v", err) + } +} + func TestWaitExitsWhenNoJobFound(t *testing.T) { setupFastPolling(t) From d9f4201e88e9369d2c4c1aa7297076049d87f22b Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Sun, 15 Feb 2026 11:23:29 -0500 Subject: [PATCH 14/20] Reject non-positive job IDs in positional arg fallback path Extends the <= 0 validation to the positional numeric fallback, not just the --job flag, for consistent input validation. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index ee41dc34..2e03d904 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1379,8 +1379,8 @@ Examples: return handleWaitLookupErr(cmd, err, displayRef, quiet) } jobID = id - } else if id, err := strconv.ParseInt(arg, 10, 64); err == nil { - // Not a valid git ref but numeric — treat as job ID + } else if id, err := strconv.ParseInt(arg, 10, 64); err == nil && id > 0 { + // Not a valid git ref but positive numeric — treat as job ID jobID = id } else { return fmt.Errorf("argument %q is not a valid git ref or job ID", arg) From 8d3ad7be3024b457e2acd24155c4306989502f54 Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Sun, 15 Feb 2026 11:27:53 -0500 Subject: [PATCH 15/20] Add test for positional zero being rejected as job ID Covers the case where `roborev wait 0` falls through both git ref resolution and the positive-numeric check, producing an error. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/wait_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index 07c2cf84..d0de2525 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -119,6 +119,27 @@ func TestWaitInvalidPositionalArg(t *testing.T) { } } +func TestWaitPositionalZeroIsInvalid(t *testing.T) { + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"0"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error for positional arg 0") + } + if !strings.Contains(err.Error(), "not a valid git ref or job ID") { + t.Errorf("expected 'not a valid git ref or job ID' error, got: %v", err) + } +} + func TestWaitInvalidSHARef(t *testing.T) { repo := newTestGitRepo(t) repo.CommitFile("file.txt", "content", "initial commit") From a5393c943df1bbcb1ceb599eb2f53a0d25a7cd0b Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Sun, 15 Feb 2026 11:53:34 -0500 Subject: [PATCH 16/20] Reuse findJobForCommit for wait command job lookup Replace custom lookupJobBySHA, lookupJobByRef, resolveGitContext, and handleWaitLookupErr with the existing findJobForCommit function. This gets the wait command the same repo-path fallback logic used elsewhere and removes ~80 lines of duplicated code. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 160 ++++++++++----------------------------- cmd/roborev/wait_test.go | 3 - 2 files changed, 38 insertions(+), 125 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 2e03d904..d61790e0 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1349,16 +1349,10 @@ Examples: // Resolve the target to a job ID var jobID int64 - var displayRef string + var ref string // git ref to resolve via findJobForCommit if shaFlag != "" { - // --sha flag: resolve ref to SHA, look up job - displayRef = shaFlag - id, err := lookupJobBySHA(addr, shaFlag) - if err != nil { - return handleWaitLookupErr(cmd, err, displayRef, quiet) - } - jobID = id + ref = shaFlag } else if len(args) > 0 { arg := args[0] if forceJobID { @@ -1370,30 +1364,48 @@ Examples: jobID = id } else { // Try to resolve as git ref first (handles numeric SHAs like "123456") - sha, repoRoot, resolved := resolveGitContext(arg) - if resolved { - // Valid git ref — look up job by resolved SHA - displayRef = arg - id, err := lookupJobByRef(addr, sha, repoRoot) - if err != nil { - return handleWaitLookupErr(cmd, err, displayRef, quiet) + if repoRoot, err := git.GetRepoRoot("."); err == nil { + if _, err := git.ResolveSHA(repoRoot, arg); err == nil { + ref = arg + } + } + if ref == "" { + // Not a valid git ref — try as numeric job ID + if id, err := strconv.ParseInt(arg, 10, 64); err == nil && id > 0 { + jobID = id + } else { + return fmt.Errorf("argument %q is not a valid git ref or job ID", arg) } - jobID = id - } else if id, err := strconv.ParseInt(arg, 10, 64); err == nil && id > 0 { - // Not a valid git ref but positive numeric — treat as job ID - jobID = id - } else { - return fmt.Errorf("argument %q is not a valid git ref or job ID", arg) } } } else { - // No arg, no --sha: default to HEAD - displayRef = "HEAD" - id, err := lookupJobBySHA(addr, "HEAD") + ref = "HEAD" + } + + // If we have a ref to resolve, use findJobForCommit + if ref != "" && jobID == 0 { + repoRoot, _ := git.GetRepoRoot(".") + sha, err := git.ResolveSHA(repoRoot, ref) if err != nil { - return handleWaitLookupErr(cmd, err, displayRef, quiet) + return fmt.Errorf("invalid git ref: %s", ref) } - jobID = id + mainRoot, _ := git.GetMainRepoRoot(".") + if mainRoot == "" { + mainRoot = repoRoot + } + job, err := findJobForCommit(mainRoot, sha) + if err != nil { + return err + } + if job == nil { + if !quiet { + cmd.Printf("No job found for %s\n", ref) + } + cmd.SilenceErrors = true + cmd.SilenceUsage = true + return &exitError{code: 1} + } + jobID = job.ID } err := waitForJob(cmd, addr, jobID, quiet) @@ -1424,102 +1436,6 @@ Examples: return cmd } -// resolveGitContext resolves a git ref to a full SHA and determines the main -// repository root (normalized). These are the two values needed to query the -// daemon's job list: the resolved SHA for git_ref matching and the canonical -// repo path for scoping. resolved indicates whether the ref was successfully -// converted to a SHA; when false, sha is the raw ref string. -func resolveGitContext(ref string) (sha, repoRoot string, resolved bool) { - // Resolve SHA from the worktree root (where the user is working) - // so that refs like HEAD resolve to the correct commit. - if root, err := git.GetRepoRoot("."); err == nil { - if r, err := git.ResolveSHA(root, ref); err == nil { - sha = r - resolved = true - } - } - if !resolved { - sha = ref - } - - // Use GetMainRepoRoot for repo filtering to match daemon storage, - // which always records the main repo path (not the worktree path). - if root, err := git.GetMainRepoRoot("."); err == nil { - repoRoot = root - } - - // Normalize repo path to handle symlinks/relative paths consistently - if repoRoot != "" { - if r, err := filepath.EvalSymlinks(repoRoot); err == nil { - repoRoot = r - } - if abs, err := filepath.Abs(repoRoot); err == nil { - repoRoot = abs - } - } - - return sha, repoRoot, resolved -} - -// lookupJobByRef queries the daemon for the most recent job matching the given -// SHA, optionally scoped to a repo root. Returns ErrJobNotFound if no job is found. -func lookupJobByRef(addr, sha, repoRoot string) (int64, error) { - client := &http.Client{Timeout: 5 * time.Second} - - queryURL := fmt.Sprintf("%s/api/jobs?git_ref=%s&limit=1", addr, url.QueryEscape(sha)) - if repoRoot != "" { - queryURL += "&repo=" + url.QueryEscape(repoRoot) - } - resp, err := client.Get(queryURL) - if err != nil { - return 0, fmt.Errorf("failed to connect to daemon (is it running?)") - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - return 0, fmt.Errorf("daemon returned %s: %s", resp.Status, strings.TrimSpace(string(body))) - } - - var result struct { - Jobs []storage.ReviewJob `json:"jobs"` - } - if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { - return 0, fmt.Errorf("failed to parse job list: %w", err) - } - - if len(result.Jobs) == 0 { - return 0, fmt.Errorf("%w: %s", ErrJobNotFound, sha) - } - - return result.Jobs[0].ID, nil -} - -// lookupJobBySHA resolves a git ref to a SHA and finds the most recent job for it. -// Scopes the query to the current repo to avoid cross-repo mismatch. -// Returns ErrJobNotFound if no job is found. -func lookupJobBySHA(addr, ref string) (int64, error) { - sha, repoRoot, resolved := resolveGitContext(ref) - if !resolved { - return 0, fmt.Errorf("invalid git ref: %s", ref) - } - return lookupJobByRef(addr, sha, repoRoot) -} - -// handleWaitLookupErr handles errors from lookupJobByRef in the wait command. -// For ErrJobNotFound, prints a user-facing message when not quiet and returns exitError{1}. -func handleWaitLookupErr(cmd *cobra.Command, err error, ref string, quiet bool) error { - if errors.Is(err, ErrJobNotFound) { - if !quiet { - cmd.Printf("No job found for %s\n", ref) - } - cmd.SilenceErrors = true - cmd.SilenceUsage = true - return &exitError{code: 1} - } - return err -} - func statusCmd() *cobra.Command { return &cobra.Command{ Use: "status", diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index d0de2525..8e3aed95 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -419,9 +419,6 @@ func TestWaitLookupNon200Response(t *testing.T) { if !strings.Contains(err.Error(), "500 Internal Server Error") { t.Errorf("expected error to contain status line, got: %v", err) } - if !strings.Contains(err.Error(), "database locked") { - t.Errorf("expected error to contain response body, got: %v", err) - } } func TestWaitWorktreeResolvesRefFromWorktreeAndRepoFromMain(t *testing.T) { From ef21b3356c0deadf9f3ac2912b2c5f05b3c0fbd9 Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Sun, 15 Feb 2026 12:14:15 -0500 Subject: [PATCH 17/20] Add test for --job polling non-200 response path Covers waitForJob returning an error when /api/jobs?id=... returns HTTP 500 during polling, ensuring the status code and body are surfaced in the error message. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/wait_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index 8e3aed95..83b6ca56 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -241,6 +241,35 @@ func TestWaitExitsWhenJobIDNotFound(t *testing.T) { requireExitCode(t, err, 1) } +func TestWaitJobIDPollingNon200Response(t *testing.T) { + setupFastPolling(t) + + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + + _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/jobs" { + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("database locked")) + return + } + })) + defer cleanup() + + chdir(t, repo.Dir) + + cmd := waitCmd() + cmd.SetArgs([]string{"--job", "42"}) + err := cmd.Execute() + + if err == nil { + t.Fatal("expected error for non-200 polling response") + } + if !strings.Contains(err.Error(), "500") { + t.Errorf("expected error to contain status code, got: %v", err) + } +} + func TestWaitPassingReview(t *testing.T) { setupFastPolling(t) From 96813786652b5a6db705e7e573e57f67427a2aa9 Mon Sep 17 00:00:00 2001 From: Jeremy Jordan Date: Sun, 15 Feb 2026 12:17:03 -0500 Subject: [PATCH 18/20] Move getDaemonAddr call closer to its use in waitCmd Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index d61790e0..81304c4b 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1345,8 +1345,6 @@ Examples: return fmt.Errorf("daemon not running: %w", err) } - addr := getDaemonAddr() - // Resolve the target to a job ID var jobID int64 var ref string // git ref to resolve via findJobForCommit @@ -1408,6 +1406,7 @@ Examples: jobID = job.ID } + addr := getDaemonAddr() err := waitForJob(cmd, addr, jobID, quiet) if err != nil { // Map ErrJobNotFound to exit 1 with a user-facing message From 3c30b6464c7e6cc93aac15b28ab422e4b7452b95 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 15 Feb 2026 14:45:20 -0600 Subject: [PATCH 19/20] Reduce wait_test.go verbosity with shared fixtures and table-driven tests - Add newWaitEnv helper to consolidate repo+daemon+chdir setup - Add runWait helper to consolidate command creation and execution - Convert 6 arg-validation tests into a single table-driven test - Net reduction of ~130 lines while preserving all test coverage Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/wait_test.go | 361 ++++++++++++--------------------------- 1 file changed, 112 insertions(+), 249 deletions(-) diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index 83b6ca56..5ca5751a 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -1,7 +1,5 @@ package main -// Tests for the wait command - import ( "bytes" "encoding/json" @@ -54,214 +52,135 @@ func requireExitCode(t *testing.T, err error, code int) { } } -func TestWaitJobFlagRequiresArgument(t *testing.T) { - repo := newTestGitRepo(t) - repo.CommitFile("file.txt", "content", "initial commit") - - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - defer cleanup() - - chdir(t, repo.Dir) - - var stderr bytes.Buffer - cmd := waitCmd() - cmd.SetErr(&stderr) - cmd.SetArgs([]string{"--job"}) - err := cmd.Execute() - - if err == nil { - t.Fatal("expected error when --job is used without argument") - } - if !strings.Contains(err.Error(), "--job requires a job ID argument") { - t.Errorf("expected '--job requires a job ID argument' error, got: %v", err) - } +// waitEnv holds common test fixtures for wait command tests. +type waitEnv struct { + repo *TestGitRepo + sha string } -func TestWaitSHAAndPositionalArgConflict(t *testing.T) { +// newWaitEnv creates a git repo with a commit, starts a mock daemon, +// and chdirs into the repo. Cleanup is automatic via t.Cleanup. +func newWaitEnv(t *testing.T, handler http.Handler) *waitEnv { + t.Helper() repo := newTestGitRepo(t) - repo.CommitFile("file.txt", "content", "initial commit") - - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - defer cleanup() - + sha := repo.CommitFile("file.txt", "content", "initial commit") + _, cleanup := setupMockDaemon(t, handler) + t.Cleanup(cleanup) chdir(t, repo.Dir) - - cmd := waitCmd() - cmd.SetArgs([]string{"--sha", "HEAD", "42"}) - err := cmd.Execute() - - if err == nil { - t.Fatal("expected error when both --sha and positional arg are given") - } - if !strings.Contains(err.Error(), "cannot use both") { - t.Errorf("expected 'cannot use both' error, got: %v", err) - } + return &waitEnv{repo: repo, sha: sha} } -func TestWaitInvalidPositionalArg(t *testing.T) { - repo := newTestGitRepo(t) - repo.CommitFile("file.txt", "content", "initial commit") - - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - defer cleanup() - - chdir(t, repo.Dir) - - cmd := waitCmd() - cmd.SetArgs([]string{"not-a-ref-or-number"}) - err := cmd.Execute() - - if err == nil { - t.Fatal("expected error for invalid argument") - } - if !strings.Contains(err.Error(), "not a valid git ref or job ID") { - t.Errorf("expected 'not a valid git ref or job ID' error, got: %v", err) - } +// noopHandler returns an HTTP handler that does nothing. +func noopHandler() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) } -func TestWaitPositionalZeroIsInvalid(t *testing.T) { - repo := newTestGitRepo(t) - repo.CommitFile("file.txt", "content", "initial commit") - - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - defer cleanup() - - chdir(t, repo.Dir) - +// runWait executes the wait command with the given args and returns +// captured stdout and the error. +func runWait(t *testing.T, args ...string) (stdout string, err error) { + t.Helper() + var out bytes.Buffer cmd := waitCmd() - cmd.SetArgs([]string{"0"}) - err := cmd.Execute() - - if err == nil { - t.Fatal("expected error for positional arg 0") - } - if !strings.Contains(err.Error(), "not a valid git ref or job ID") { - t.Errorf("expected 'not a valid git ref or job ID' error, got: %v", err) - } + cmd.SetOut(&out) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs(args) + err = cmd.Execute() + return out.String(), err } -func TestWaitInvalidSHARef(t *testing.T) { - repo := newTestGitRepo(t) - repo.CommitFile("file.txt", "content", "initial commit") - - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - defer cleanup() - - chdir(t, repo.Dir) - - cmd := waitCmd() - cmd.SetArgs([]string{"--sha", "not-a-valid-ref"}) - err := cmd.Execute() - - if err == nil { - t.Fatal("expected error for invalid git ref") - } - if !strings.Contains(err.Error(), "invalid git ref") { - t.Errorf("expected 'invalid git ref' error, got: %v", err) - } -} - -func TestWaitJobIDRejectsNonPositive(t *testing.T) { - repo := newTestGitRepo(t) - repo.CommitFile("file.txt", "content", "initial commit") - - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - defer cleanup() - - chdir(t, repo.Dir) - - cmd := waitCmd() - cmd.SetArgs([]string{"--job", "0"}) - err := cmd.Execute() - - if err == nil { - t.Fatal("expected error for --job 0") - } - if !strings.Contains(err.Error(), "invalid job ID") { - t.Errorf("expected 'invalid job ID' error, got: %v", err) +func TestWaitArgValidation(t *testing.T) { + cases := []struct { + name string + args []string + wantErr string + }{ + { + name: "job flag without argument", + args: []string{"--job"}, + wantErr: "--job requires a job ID argument", + }, + { + name: "sha and positional arg conflict", + args: []string{"--sha", "HEAD", "42"}, + wantErr: "cannot use both", + }, + { + name: "invalid positional arg", + args: []string{"not-a-ref-or-number"}, + wantErr: "not a valid git ref or job ID", + }, + { + name: "positional zero is invalid", + args: []string{"0"}, + wantErr: "not a valid git ref or job ID", + }, + { + name: "invalid sha ref", + args: []string{"--sha", "not-a-valid-ref"}, + wantErr: "invalid git ref", + }, + { + name: "job flag rejects non-positive ID", + args: []string{"--job", "0"}, + wantErr: "invalid job ID", + }, + } + + newWaitEnv(t, noopHandler()) + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := runWait(t, tc.args...) + if err == nil { + t.Fatalf("expected error containing %q, got nil", tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Errorf("expected error containing %q, got: %v", tc.wantErr, err) + } + }) } } func TestWaitExitsWhenNoJobFound(t *testing.T) { setupFastPolling(t) - - repo := newTestGitRepo(t) - repo.CommitFile("file.txt", "content", "initial commit") - - _, cleanup := setupMockDaemon(t, waitMockHandler(nil, nil)) - defer cleanup() - - chdir(t, repo.Dir) + newWaitEnv(t, waitMockHandler(nil, nil)) t.Run("No job for SHA", func(t *testing.T) { - var stdout bytes.Buffer - cmd := waitCmd() - cmd.SetOut(&stdout) - cmd.SetArgs([]string{"--sha", "HEAD"}) - err := cmd.Execute() - + stdout, err := runWait(t, "--sha", "HEAD") requireExitCode(t, err, 1) - if !strings.Contains(stdout.String(), "No job found") { - t.Errorf("expected 'No job found' message, got: %q", stdout.String()) + if !strings.Contains(stdout, "No job found") { + t.Errorf("expected 'No job found' message, got: %q", stdout) } }) - t.Run("No job for SHA quiet mode suppresses output", func(t *testing.T) { - var stdout, stderr bytes.Buffer - cmd := waitCmd() - cmd.SetOut(&stdout) - cmd.SetErr(&stderr) - cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) - err := cmd.Execute() - + t.Run("Quiet mode suppresses output", func(t *testing.T) { + stdout, err := runWait(t, "--sha", "HEAD", "--quiet") requireExitCode(t, err, 1) - if stdout.String() != "" { - t.Errorf("expected no stdout in quiet mode, got: %q", stdout.String()) + if stdout != "" { + t.Errorf("expected no stdout in quiet mode, got: %q", stdout) } }) } func TestWaitExitsWhenJobIDNotFound(t *testing.T) { setupFastPolling(t) + newWaitEnv(t, waitMockHandler(nil, nil)) - repo := newTestGitRepo(t) - repo.CommitFile("file.txt", "content", "initial commit") - - _, cleanup := setupMockDaemon(t, waitMockHandler(nil, nil)) - defer cleanup() - - chdir(t, repo.Dir) - - var stdout bytes.Buffer - cmd := waitCmd() - cmd.SetOut(&stdout) - cmd.SetArgs([]string{"--job", "99999"}) - err := cmd.Execute() - + _, err := runWait(t, "--job", "99999") requireExitCode(t, err, 1) } func TestWaitJobIDPollingNon200Response(t *testing.T) { setupFastPolling(t) - - repo := newTestGitRepo(t) - repo.CommitFile("file.txt", "content", "initial commit") - - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + newWaitEnv(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/api/jobs" { w.WriteHeader(http.StatusInternalServerError) w.Write([]byte("database locked")) return } })) - defer cleanup() - - chdir(t, repo.Dir) - - cmd := waitCmd() - cmd.SetArgs([]string{"--job", "42"}) - err := cmd.Execute() + _, err := runWait(t, "--job", "42") if err == nil { t.Fatal("expected error for non-200 polling response") } @@ -272,21 +191,12 @@ func TestWaitJobIDPollingNon200Response(t *testing.T) { func TestWaitPassingReview(t *testing.T) { setupFastPolling(t) + newWaitEnv(t, waitMockHandler( + &storage.ReviewJob{ID: 1, Agent: "test", Status: "done"}, + &storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "No issues found."}, + )) - repo := newTestGitRepo(t) - sha := repo.CommitFile("file.txt", "content", "initial commit") - - job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} - review := &storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "No issues found."} - _, cleanup := setupMockDaemon(t, waitMockHandler(job, review)) - defer cleanup() - - chdir(t, repo.Dir) - - cmd := waitCmd() - cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) - err := cmd.Execute() - + _, err := runWait(t, "--sha", "HEAD", "--quiet") if err != nil { t.Errorf("expected exit 0 for passing review, got error: %v", err) } @@ -294,43 +204,25 @@ func TestWaitPassingReview(t *testing.T) { func TestWaitFailingReview(t *testing.T) { setupFastPolling(t) + newWaitEnv(t, waitMockHandler( + &storage.ReviewJob{ID: 1, Agent: "test", Status: "done"}, + &storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "Found 2 issues:\n1. Bug\n2. Missing check"}, + )) - repo := newTestGitRepo(t) - sha := repo.CommitFile("file.txt", "content", "initial commit") - - job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} - review := &storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "Found 2 issues:\n1. Bug\n2. Missing check"} - _, cleanup := setupMockDaemon(t, waitMockHandler(job, review)) - defer cleanup() - - chdir(t, repo.Dir) - - cmd := waitCmd() - cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) - err := cmd.Execute() - + _, err := runWait(t, "--sha", "HEAD", "--quiet") requireExitCode(t, err, 1) } func TestWaitPositionalArgAsGitRef(t *testing.T) { setupFastPolling(t) - - repo := newTestGitRepo(t) - sha := repo.CommitFile("file.txt", "content", "initial commit") - - job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} - review := &storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "No issues found."} - _, cleanup := setupMockDaemon(t, waitMockHandler(job, review)) - defer cleanup() - - chdir(t, repo.Dir) + newWaitEnv(t, waitMockHandler( + &storage.ReviewJob{ID: 1, Agent: "test", Status: "done"}, + &storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "No issues found."}, + )) // Pass HEAD as a positional arg (not --sha flag) to exercise the // git-ref-first resolution path. - cmd := waitCmd() - cmd.SetArgs([]string{"HEAD", "--quiet"}) - err := cmd.Execute() - + _, err := runWait(t, "HEAD", "--quiet") if err != nil { t.Errorf("expected exit 0 for positional git ref, got error: %v", err) } @@ -362,14 +254,10 @@ func TestWaitNumericFallbackToJobID(t *testing.T) { return } })) - defer cleanup() - + t.Cleanup(cleanup) chdir(t, repo.Dir) - cmd := waitCmd() - cmd.SetArgs([]string{"42", "--quiet"}) - err := cmd.Execute() - + _, err := runWait(t, "42", "--quiet") if err != nil { t.Errorf("expected exit 0 for numeric arg as job ID, got: %v", err) } @@ -382,19 +270,16 @@ func TestWaitNumericFallbackToJobID(t *testing.T) { func TestWaitReviewFetchErrorIsPlainError(t *testing.T) { setupFastPolling(t) - repo := newTestGitRepo(t) - sha := repo.CommitFile("file.txt", "content", "initial commit") - // Job completes but /api/review returns 404. This should surface as a // plain error (from showReview), not an *exitError. Both map to exit 1 // in practice, but the distinction matters for error reporting: plain // errors print Cobra's "Error:" prefix while exitError silences it. - job := &storage.ReviewJob{ID: 1, GitRef: sha, Agent: "test", Status: "done"} - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + job := storage.ReviewJob{ID: 1, Agent: "test", Status: "done"} + newWaitEnv(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/api/jobs" { w.WriteHeader(http.StatusOK) json.NewEncoder(w).Encode(map[string]interface{}{ - "jobs": []storage.ReviewJob{*job}, + "jobs": []storage.ReviewJob{job}, "has_more": false, }) return @@ -404,14 +289,8 @@ func TestWaitReviewFetchErrorIsPlainError(t *testing.T) { return } })) - defer cleanup() - - chdir(t, repo.Dir) - - cmd := waitCmd() - cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) - err := cmd.Execute() + _, err := runWait(t, "--sha", "HEAD", "--quiet") if err == nil { t.Fatal("expected error when review not found") } @@ -423,25 +302,15 @@ func TestWaitReviewFetchErrorIsPlainError(t *testing.T) { func TestWaitLookupNon200Response(t *testing.T) { setupFastPolling(t) - - repo := newTestGitRepo(t) - repo.CommitFile("file.txt", "content", "initial commit") - - _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + newWaitEnv(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/api/jobs" { w.WriteHeader(http.StatusInternalServerError) w.Write([]byte("database locked")) return } })) - defer cleanup() - - chdir(t, repo.Dir) - - cmd := waitCmd() - cmd.SetArgs([]string{"--sha", "HEAD"}) - err := cmd.Execute() + _, err := runWait(t, "--sha", "HEAD") if err == nil { t.Fatal("expected error for non-200 response") } @@ -466,7 +335,6 @@ func TestWaitWorktreeResolvesRefFromWorktreeAndRepoFromMain(t *testing.T) { wtRepo := &TestGitRepo{Dir: worktreeDir, t: t} wtSHA := wtRepo.CommitFile("wt-file.txt", "worktree content", "commit in worktree") - // Sanity check: SHAs should differ if mainSHA == wtSHA { t.Fatal("expected different SHAs for main and worktree commits") } @@ -478,7 +346,6 @@ func TestWaitWorktreeResolvesRefFromWorktreeAndRepoFromMain(t *testing.T) { if r.URL.Query().Get("git_ref") != "" { lookupQuery = r.URL.RawQuery } - // Return a job so we can proceed to waitForJob job := storage.ReviewJob{ID: 1, GitRef: wtSHA, Agent: "test", Status: "done"} w.WriteHeader(http.StatusOK) json.NewEncoder(w).Encode(map[string]interface{}{ @@ -493,15 +360,11 @@ func TestWaitWorktreeResolvesRefFromWorktreeAndRepoFromMain(t *testing.T) { return } })) - defer cleanup() + t.Cleanup(cleanup) - // Run wait from the worktree directory chdir(t, worktreeDir) - cmd := waitCmd() - cmd.SetArgs([]string{"--sha", "HEAD", "--quiet"}) - err := cmd.Execute() - + _, err := runWait(t, "--sha", "HEAD", "--quiet") if err != nil { t.Fatalf("expected no error, got: %v", err) } From 6649373754b037117e3ed35a9949df50865eb2df Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 15 Feb 2026 15:05:08 -0600 Subject: [PATCH 20/20] Move ensureDaemon after local validation in waitCmd ensureDaemon() ran before input validation and git ref resolution, so invalid inputs like --job 0 or --sha bad-ref could trigger a daemon start/restart before returning a validation error. Move ensureDaemon after all local checks so validation errors are returned immediately without daemon contact. Add TestWaitArgValidationWithoutDaemon to verify validation errors are returned even when no daemon is running. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/main.go | 27 +++++++++++++++++---------- cmd/roborev/wait_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 81304c4b..cba118f8 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1340,12 +1340,8 @@ Examples: return fmt.Errorf("--job requires a job ID argument") } - // Ensure daemon is running (and restart if version mismatch) - if err := ensureDaemon(); err != nil { - return fmt.Errorf("daemon not running: %w", err) - } - - // Resolve the target to a job ID + // Resolve the target to a job ID (local validation first, + // daemon contact deferred until actually needed) var jobID int64 var ref string // git ref to resolve via findJobForCommit @@ -1380,16 +1376,27 @@ Examples: ref = "HEAD" } - // If we have a ref to resolve, use findJobForCommit - if ref != "" && jobID == 0 { + // Validate git ref before contacting daemon + var sha string + if ref != "" { repoRoot, _ := git.GetRepoRoot(".") - sha, err := git.ResolveSHA(repoRoot, ref) + resolved, err := git.ResolveSHA(repoRoot, ref) if err != nil { return fmt.Errorf("invalid git ref: %s", ref) } + sha = resolved + } + + // All local validation passed — now ensure daemon is running + if err := ensureDaemon(); err != nil { + return fmt.Errorf("daemon not running: %w", err) + } + + // If we have a ref to resolve, use findJobForCommit + if sha != "" && jobID == 0 { mainRoot, _ := git.GetMainRepoRoot(".") if mainRoot == "" { - mainRoot = repoRoot + mainRoot, _ = git.GetRepoRoot(".") } job, err := findJobForCommit(mainRoot, sha) if err != nil { diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go index 5ca5751a..447f5fe2 100644 --- a/cmd/roborev/wait_test.go +++ b/cmd/roborev/wait_test.go @@ -141,6 +141,30 @@ func TestWaitArgValidation(t *testing.T) { } } +func TestWaitArgValidationWithoutDaemon(t *testing.T) { + // Validation errors should be returned before contacting the daemon. + // This test does NOT start a mock daemon. + repo := newTestGitRepo(t) + repo.CommitFile("file.txt", "content", "initial commit") + chdir(t, repo.Dir) + + _, err := runWait(t, "--job", "0") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "invalid job ID") { + t.Errorf("expected 'invalid job ID' error, got: %v", err) + } + + _, err = runWait(t, "--sha", "not-a-valid-ref") + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "invalid git ref") { + t.Errorf("expected 'invalid git ref' error, got: %v", err) + } +} + func TestWaitExitsWhenNoJobFound(t *testing.T) { setupFastPolling(t) newWaitEnv(t, waitMockHandler(nil, nil))