diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 4397247b..cba118f8 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()) @@ -310,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 { @@ -1185,7 +1189,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] @@ -1290,6 +1294,154 @@ func (e *exitError) Error() string { return fmt.Sprintf("exit code %d", e.code) } +func waitCmd() *cobra.Command { + var ( + shaFlag string + forceJobID bool + quiet bool + ) + + 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 completed with verdict PASS + 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`, + 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 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") + } + + // 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 + + if shaFlag != "" { + ref = shaFlag + } 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 || id <= 0 { + return fmt.Errorf("invalid job ID: %s", arg) + } + jobID = id + } else { + // Try to resolve as git ref first (handles numeric SHAs like "123456") + 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) + } + } + } + } else { + ref = "HEAD" + } + + // Validate git ref before contacting daemon + var sha string + if ref != "" { + repoRoot, _ := git.GetRepoRoot(".") + 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, _ = git.GetRepoRoot(".") + } + 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 + } + + addr := getDaemonAddr() + 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) + 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: 1} + } + 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)") + + return cmd +} + func statusCmd() *cobra.Command { return &cobra.Command{ Use: "status", diff --git a/cmd/roborev/wait_test.go b/cmd/roborev/wait_test.go new file mode 100644 index 00000000..447f5fe2 --- /dev/null +++ b/cmd/roborev/wait_test.go @@ -0,0 +1,415 @@ +package main + +import ( + "bytes" + "encoding/json" + "net/http" + "net/url" + "os" + "strings" + "testing" + + "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) + } +} + +// waitEnv holds common test fixtures for wait command tests. +type waitEnv struct { + repo *TestGitRepo + sha string +} + +// 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) + sha := repo.CommitFile("file.txt", "content", "initial commit") + _, cleanup := setupMockDaemon(t, handler) + t.Cleanup(cleanup) + chdir(t, repo.Dir) + return &waitEnv{repo: repo, sha: sha} +} + +// noopHandler returns an HTTP handler that does nothing. +func noopHandler() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) +} + +// 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.SetOut(&out) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs(args) + err = cmd.Execute() + return out.String(), 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 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)) + + t.Run("No job for SHA", func(t *testing.T) { + stdout, err := runWait(t, "--sha", "HEAD") + requireExitCode(t, err, 1) + if !strings.Contains(stdout, "No job found") { + t.Errorf("expected 'No job found' message, got: %q", stdout) + } + }) + + t.Run("Quiet mode suppresses output", func(t *testing.T) { + stdout, err := runWait(t, "--sha", "HEAD", "--quiet") + requireExitCode(t, err, 1) + 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)) + + _, err := runWait(t, "--job", "99999") + requireExitCode(t, err, 1) +} + +func TestWaitJobIDPollingNon200Response(t *testing.T) { + setupFastPolling(t) + 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 + } + })) + + _, err := runWait(t, "--job", "42") + 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) + newWaitEnv(t, waitMockHandler( + &storage.ReviewJob{ID: 1, Agent: "test", Status: "done"}, + &storage.Review{ID: 1, JobID: 1, Agent: "test", Output: "No issues found."}, + )) + + _, err := runWait(t, "--sha", "HEAD", "--quiet") + if err != nil { + t.Errorf("expected exit 0 for passing review, got error: %v", err) + } +} + +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"}, + )) + + _, err := runWait(t, "--sha", "HEAD", "--quiet") + requireExitCode(t, err, 1) +} + +func TestWaitPositionalArgAsGitRef(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."}, + )) + + // Pass HEAD as a positional arg (not --sha flag) to exercise the + // git-ref-first resolution path. + _, err := runWait(t, "HEAD", "--quiet") + 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" { + 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}, + "has_more": false, + }) + return + } + if r.URL.Path == "/api/review" { + w.WriteHeader(http.StatusOK) + json.NewEncoder(w).Encode(storage.Review{ID: 1, JobID: 42, Agent: "test", Output: "No issues found."}) + return + } + })) + t.Cleanup(cleanup) + chdir(t, repo.Dir) + + _, err := runWait(t, "42", "--quiet") + if err != nil { + t.Errorf("expected exit 0 for numeric arg as job ID, got: %v", err) + } + // 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) + } +} + +func TestWaitReviewFetchErrorIsPlainError(t *testing.T) { + setupFastPolling(t) + + // 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, 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}, + "has_more": false, + }) + return + } + if r.URL.Path == "/api/review" { + w.WriteHeader(http.StatusNotFound) + return + } + })) + + _, err := runWait(t, "--sha", "HEAD", "--quiet") + if err == nil { + t.Fatal("expected error when review not found") + } + // 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") + } +} + +func TestWaitLookupNon200Response(t *testing.T) { + setupFastPolling(t) + 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 + } + })) + + _, err := runWait(t, "--sha", "HEAD") + if err == nil { + t.Fatal("expected error for non-200 response") + } + if !strings.Contains(err.Error(), "500 Internal Server Error") { + t.Errorf("expected error to contain status line, got: %v", err) + } +} + +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") + + 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 + } + 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 + } + })) + t.Cleanup(cleanup) + + chdir(t, worktreeDir) + + _, err := runWait(t, "--sha", "HEAD", "--quiet") + 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) + } +}