From 8f777da599328cab2a062bc8c76f9dfec741735d Mon Sep 17 00:00:00 2001 From: Darren Haas <361714+darrenhaas@users.noreply.github.com> Date: Wed, 4 Mar 2026 16:03:47 -0800 Subject: [PATCH 01/10] Add multi-repo workspace support for non-git parent directories When running roborev from a parent directory containing multiple git repos (e.g., a monorepo workspace), commands now gracefully handle not being in a git repo: `list` shows jobs across all child repos via repo_prefix filter, `show` suggests using job IDs, and `review` lists available child repos with --repo hints. Includes LIKE wildcard escaping for path safety. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/list.go | 15 +++++- cmd/roborev/review.go | 29 ++++++++++++ cmd/roborev/show.go | 10 ++-- internal/daemon/server.go | 13 +++++- internal/daemon/server_jobs_test.go | 68 ++++++++++++++++++++++++++++ internal/daemon/server_repos_test.go | 45 ++++++++++++++++++ internal/storage/jobs.go | 22 +++++++++ internal/storage/repos.go | 29 ++++++++++++ 8 files changed, 224 insertions(+), 7 deletions(-) diff --git a/cmd/roborev/list.go b/cmd/roborev/list.go index a6763ed6..a35a0d2f 100644 --- a/cmd/roborev/list.go +++ b/cmd/roborev/list.go @@ -7,6 +7,7 @@ import ( "net/http" "net/url" "os" + "path/filepath" "strconv" "strings" "text/tabwriter" @@ -75,12 +76,22 @@ Examples: branch = git.GetCurrentBranch(localRepoPath) } + // Workspace mode: not in a git repo and no --repo specified + var repoPrefix string + if repoPath == "" && localRepoPath == "" { + if abs, err := filepath.Abs("."); err == nil { + repoPrefix = abs + } + } + // Build query URL params := url.Values{} - if repoPath != "" { + if repoPrefix != "" { + params.Set("repo_prefix", repoPrefix) + } else if repoPath != "" { params.Set("repo", repoPath) } - if branch != "" { + if branch != "" && repoPrefix == "" { params.Set("branch", branch) params.Set("branch_include_empty", "true") } diff --git a/cmd/roborev/review.go b/cmd/roborev/review.go index cb52766b..0bb91542 100644 --- a/cmd/roborev/review.go +++ b/cmd/roborev/review.go @@ -7,6 +7,8 @@ import ( "fmt" "io" "net/http" + "os" + "path/filepath" "github.com/roborev-dev/roborev/internal/agent" "github.com/roborev-dev/roborev/internal/config" @@ -80,6 +82,14 @@ Examples: if quiet { return nil // Not a repo - silent exit for hooks } + // Scan for child git repos to give a helpful hint + if children := findChildGitRepos(repoPath); len(children) > 0 { + msg := "not in a git repository; use --repo to specify one:" + for _, name := range children { + msg += "\n roborev review --repo " + name + } + return fmt.Errorf("%s", msg) + } return fmt.Errorf("not a git repository: %w", err) } @@ -411,3 +421,22 @@ func runLocalReview(cmd *cobra.Command, repoPath, gitRef, diffContent, agentName } return nil } + +// findChildGitRepos returns the names of immediate child directories that are git repos. +func findChildGitRepos(dir string) []string { + entries, err := os.ReadDir(dir) + if err != nil { + return nil + } + var repos []string + for _, e := range entries { + if !e.IsDir() || e.Name()[0] == '.' { + continue + } + gitDir := filepath.Join(dir, e.Name(), ".git") + if info, err := os.Stat(gitDir); err == nil && info.IsDir() { + repos = append(repos, e.Name()) + } + } + return repos +} diff --git a/cmd/roborev/show.go b/cmd/roborev/show.go index abeea2ac..1806fe96 100644 --- a/cmd/roborev/show.go +++ b/cmd/roborev/show.go @@ -54,10 +54,12 @@ Examples: } // Default to HEAD sha := "HEAD" - if root, err := git.GetRepoRoot("."); err == nil { - if resolved, err := git.ResolveSHA(root, sha); err == nil { - sha = resolved - } + root, rootErr := git.GetRepoRoot(".") + if rootErr != nil { + return fmt.Errorf("not in a git repository; use a job ID instead (e.g., roborev show 42)") + } + if resolved, err := git.ResolveSHA(root, sha); err == nil { + sha = resolved } queryURL = addr + "/api/review?sha=" + sha displayRef = git.ShortSHA(sha) diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 6ab18457..58c77a14 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -815,6 +815,7 @@ func (s *Server) handleListJobs(w http.ResponseWriter, r *http.Request) { status := r.URL.Query().Get("status") repo := r.URL.Query().Get("repo") gitRef := r.URL.Query().Get("git_ref") + repoPrefix := r.URL.Query().Get("repo_prefix") // Parse limit from query, default to 50, 0 means no limit // Clamp to valid range: 0 (unlimited) or 1-10000 @@ -870,6 +871,9 @@ func (s *Server) handleListJobs(w http.ResponseWriter, r *http.Request) { if exJobType := r.URL.Query().Get("exclude_job_type"); exJobType != "" { listOpts = append(listOpts, storage.WithExcludeJobType(exJobType)) } + if repoPrefix != "" && repo == "" { + listOpts = append(listOpts, storage.WithRepoPrefix(repoPrefix)) + } jobs, err := s.db.ListJobs(status, repo, fetchLimit, offset, listOpts...) if err != nil { @@ -893,6 +897,9 @@ func (s *Server) handleListJobs(w http.ResponseWriter, r *http.Request) { statsOpts = append(statsOpts, storage.WithBranch(branch)) } } + if repoPrefix != "" && repo == "" { + statsOpts = append(statsOpts, storage.WithRepoPrefix(repoPrefix)) + } stats, statsErr := s.db.CountJobStats(repo, statsOpts...) if statsErr != nil { log.Printf("Warning: failed to count job stats: %v", statsErr) @@ -918,7 +925,11 @@ func (s *Server) handleListRepos(w http.ResponseWriter, r *http.Request) { var totalCount int var err error - if branch != "" { + prefix := r.URL.Query().Get("prefix") + + if prefix != "" { + repos, totalCount, err = s.db.ListReposWithReviewCountsByPrefix(prefix) + } else if branch != "" { repos, totalCount, err = s.db.ListReposWithReviewCountsByBranch(branch) } else { repos, totalCount, err = s.db.ListReposWithReviewCounts() diff --git a/internal/daemon/server_jobs_test.go b/internal/daemon/server_jobs_test.go index a2ad0bbb..a3e18be3 100644 --- a/internal/daemon/server_jobs_test.go +++ b/internal/daemon/server_jobs_test.go @@ -1301,3 +1301,71 @@ func TestHandleListJobsJobTypeFilter(t *testing.T) { } }) } + +func TestHandleListJobsRepoPrefixFilter(t *testing.T) { + server, db, tmpDir := newTestServer(t) + + // Create repos under a "workspace" parent and one outside it + workspace := filepath.Join(tmpDir, "workspace") + seedRepoWithJobs(t, db, filepath.Join(workspace, "repo-a"), 3, "repoA") + seedRepoWithJobs(t, db, filepath.Join(workspace, "repo-b"), 2, "repoB") + seedRepoWithJobs(t, db, filepath.Join(tmpDir, "outside-repo"), 1, "outside") + + t.Run("repo_prefix returns only child repos", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/api/jobs?repo_prefix="+url.QueryEscape(workspace), nil) + w := httptest.NewRecorder() + server.handleListJobs(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var resp struct { + Jobs []storage.ReviewJob `json:"jobs"` + Stats storage.JobStats `json:"stats"` + } + testutil.DecodeJSON(t, w, &resp) + + if len(resp.Jobs) != 5 { + t.Errorf("Expected 5 jobs under workspace prefix, got %d", len(resp.Jobs)) + } + for _, j := range resp.Jobs { + if !strings.HasPrefix(j.RepoPath, workspace+"/") { + t.Errorf("Job repo_path %q does not start with workspace prefix", j.RepoPath) + } + } + }) + + t.Run("repo_prefix does not match parent directory itself", func(t *testing.T) { + // A repo AT the workspace path shouldn't match (must be a child) + seedRepoWithJobs(t, db, workspace, 1, "exact") + req := httptest.NewRequest(http.MethodGet, "/api/jobs?repo_prefix="+url.QueryEscape(workspace), nil) + w := httptest.NewRecorder() + server.handleListJobs(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var resp struct { + Jobs []storage.ReviewJob `json:"jobs"` + } + testutil.DecodeJSON(t, w, &resp) + + // Should still be 5 (not 6) - the exact workspace path match is excluded + if len(resp.Jobs) != 5 { + t.Errorf("Expected 5 jobs (excluding exact path match), got %d", len(resp.Jobs)) + } + }) + + t.Run("repo param takes precedence over repo_prefix", func(t *testing.T) { + exactRepo := filepath.Join(workspace, "repo-a") + req := httptest.NewRequest(http.MethodGet, "/api/jobs?repo="+url.QueryEscape(exactRepo)+"&repo_prefix="+url.QueryEscape(workspace), nil) + w := httptest.NewRecorder() + server.handleListJobs(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var resp struct { + Jobs []storage.ReviewJob `json:"jobs"` + } + testutil.DecodeJSON(t, w, &resp) + + if len(resp.Jobs) != 3 { + t.Errorf("Expected 3 jobs for exact repo (repo takes precedence), got %d", len(resp.Jobs)) + } + }) +} diff --git a/internal/daemon/server_repos_test.go b/internal/daemon/server_repos_test.go index 19b86bce..8483c471 100644 --- a/internal/daemon/server_repos_test.go +++ b/internal/daemon/server_repos_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "net/url" "os/exec" "path/filepath" "testing" @@ -154,6 +155,50 @@ func TestHandleListReposWithBranchFilter(t *testing.T) { }) } +func TestHandleListReposWithPrefixFilter(t *testing.T) { + server, db, tmpDir := newTestServer(t) + + // Create repos under a "workspace" parent and one outside it + workspace := filepath.Join(tmpDir, "workspace") + seedRepoWithJobs(t, db, filepath.Join(workspace, "repo1"), 3, "repo1") + seedRepoWithJobs(t, db, filepath.Join(workspace, "repo2"), 2, "repo2") + seedRepoWithJobs(t, db, filepath.Join(tmpDir, "other-repo"), 1, "other") + + type reposResponse struct { + Repos []struct{ Name string } `json:"repos"` + TotalCount int `json:"total_count"` + } + + t.Run("prefix returns only child repos", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/api/repos?prefix="+url.QueryEscape(workspace), nil) + w := httptest.NewRecorder() + server.handleListRepos(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var response reposResponse + testutil.DecodeJSON(t, w, &response) + if len(response.Repos) != 2 { + t.Errorf("Expected 2 repos under workspace, got %d", len(response.Repos)) + } + if response.TotalCount != 5 { + t.Errorf("Expected total_count 5, got %d", response.TotalCount) + } + }) + + t.Run("prefix excludes non-matching repos", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/api/repos?prefix="+url.QueryEscape(filepath.Join(tmpDir, "nonexistent")), nil) + w := httptest.NewRecorder() + server.handleListRepos(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var response reposResponse + testutil.DecodeJSON(t, w, &response) + if len(response.Repos) != 0 { + t.Errorf("Expected 0 repos for nonexistent prefix, got %d", len(response.Repos)) + } + }) +} + func TestHandleListBranches(t *testing.T) { server, db, tmpDir := newTestServer(t) diff --git a/internal/storage/jobs.go b/internal/storage/jobs.go index 70b8321d..82b0757d 100644 --- a/internal/storage/jobs.go +++ b/internal/storage/jobs.go @@ -1340,6 +1340,7 @@ type listJobsOptions struct { closed *bool jobType string excludeJobType string + repoPrefix string } // WithGitRef filters jobs by git ref. @@ -1376,6 +1377,19 @@ func WithExcludeJobType(jobType string) ListJobsOption { return func(o *listJobsOptions) { o.excludeJobType = jobType } } +// WithRepoPrefix filters jobs to repos whose root_path starts with the given prefix. +func WithRepoPrefix(prefix string) ListJobsOption { + return func(o *listJobsOptions) { o.repoPrefix = escapeLike(prefix) } +} + +// escapeLike escapes SQL LIKE wildcards (% and _) in a literal string. +func escapeLike(s string) string { + s = strings.ReplaceAll(s, "\\", "\\\\") + s = strings.ReplaceAll(s, "%", "\\%") + s = strings.ReplaceAll(s, "_", "\\_") + return s +} + // ListJobs returns jobs with optional status, repo, branch, and closed filters. func (db *DB) ListJobs(statusFilter string, repoFilter string, limit, offset int, opts ...ListJobsOption) ([]ReviewJob, error) { query := ` @@ -1404,6 +1418,10 @@ func (db *DB) ListJobs(statusFilter string, repoFilter string, limit, offset int for _, opt := range opts { opt(&o) } + if repoFilter == "" && o.repoPrefix != "" { + conditions = append(conditions, "r.root_path LIKE ? || '/%' ESCAPE '\\'") + args = append(args, o.repoPrefix) + } if o.gitRef != "" { conditions = append(conditions, "j.git_ref = ?") args = append(args, o.gitRef) @@ -1571,6 +1589,10 @@ func (db *DB) CountJobStats(repoFilter string, opts ...ListJobsOption) (JobStats for _, opt := range opts { opt(&o) } + if repoFilter == "" && o.repoPrefix != "" { + conditions = append(conditions, "r.root_path LIKE ? || '/%' ESCAPE '\\'") + args = append(args, o.repoPrefix) + } if o.branch != "" { if o.branchIncludeEmpty { conditions = append(conditions, "(j.branch = ? OR j.branch = '' OR j.branch IS NULL)") diff --git a/internal/storage/repos.go b/internal/storage/repos.go index 72962e92..64a8fba4 100644 --- a/internal/storage/repos.go +++ b/internal/storage/repos.go @@ -139,6 +139,35 @@ func (db *DB) ListReposWithReviewCounts() ([]RepoWithCount, int, error) { return repos, totalCount, rows.Err() } +// ListReposWithReviewCountsByPrefix returns repos whose root_path starts with the given prefix, with their job counts +func (db *DB) ListReposWithReviewCountsByPrefix(prefix string) ([]RepoWithCount, int, error) { + escaped := escapeLike(prefix) + rows, err := db.Query(` + SELECT r.name, r.root_path, COUNT(rj.id) as job_count + FROM repos r + LEFT JOIN review_jobs rj ON rj.repo_id = r.id + WHERE r.root_path LIKE ? || '/%' ESCAPE '\' + GROUP BY r.id, r.name, r.root_path + ORDER BY r.name + `, escaped) + if err != nil { + return nil, 0, err + } + defer rows.Close() + + var repos []RepoWithCount + totalCount := 0 + for rows.Next() { + var rc RepoWithCount + if err := rows.Scan(&rc.Name, &rc.RootPath, &rc.Count); err != nil { + return nil, 0, err + } + repos = append(repos, rc) + totalCount += rc.Count + } + return repos, totalCount, rows.Err() +} + // ListReposWithReviewCountsByBranch returns repos filtered by branch with their job counts // If branch is empty, returns all repos. Use "(none)" to filter for jobs without a branch. func (db *DB) ListReposWithReviewCountsByBranch(branch string) ([]RepoWithCount, int, error) { From 4d3c46f947030719fbe638d5724d77548650dfce Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 4 Mar 2026 20:36:07 -0600 Subject: [PATCH 02/10] Fix SQL ESCAPE character conflict with Windows paths Change escapeLike to use '!' instead of '\' as the ESCAPE character in LIKE clauses. The backslash escape conflicted with Windows path separators stored in root_path, causing prefix filters to fail on Windows. Update all three LIKE ESCAPE clauses (ListJobs, CountJobStats, ListReposWithReviewCountsByPrefix). Also fix review hint paths to show full absolute paths instead of bare repo names, and use strings.Builder to satisfy the modernize linter. Add TestEscapeLike and TestPrefixFilterWithSpecialChars to verify correct escaping of !, %, and _ characters. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/review.go | 10 +++-- internal/storage/db_filter_test.go | 70 ++++++++++++++++++++++++++++++ internal/storage/jobs.go | 12 ++--- internal/storage/repos.go | 2 +- 4 files changed, 85 insertions(+), 9 deletions(-) diff --git a/cmd/roborev/review.go b/cmd/roborev/review.go index 0bb91542..f262e501 100644 --- a/cmd/roborev/review.go +++ b/cmd/roborev/review.go @@ -9,6 +9,7 @@ import ( "net/http" "os" "path/filepath" + "strings" "github.com/roborev-dev/roborev/internal/agent" "github.com/roborev-dev/roborev/internal/config" @@ -84,11 +85,14 @@ Examples: } // Scan for child git repos to give a helpful hint if children := findChildGitRepos(repoPath); len(children) > 0 { - msg := "not in a git repository; use --repo to specify one:" + absDir, _ := filepath.Abs(repoPath) + var b strings.Builder + b.WriteString("not in a git repository; use --repo to specify one:") for _, name := range children { - msg += "\n roborev review --repo " + name + b.WriteString("\n roborev review --repo ") + b.WriteString(filepath.Join(absDir, name)) } - return fmt.Errorf("%s", msg) + return fmt.Errorf("%s", b.String()) } return fmt.Errorf("not a git repository: %w", err) } diff --git a/internal/storage/db_filter_test.go b/internal/storage/db_filter_test.go index cc5f6552..a5288dd5 100644 --- a/internal/storage/db_filter_test.go +++ b/internal/storage/db_filter_test.go @@ -917,3 +917,73 @@ func TestListJobsWithJobTypeFilter(t *testing.T) { }) } } + +func TestEscapeLike(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"plain", "plain"}, + {"100%", "100!%"}, + {"under_score", "under!_score"}, + {"has!bang", "has!!bang"}, + {`C:\Users\foo`, `C:\Users\foo`}, + {"combo!_%", "combo!!!_!%"}, + } + for _, tt := range tests { + got := escapeLike(tt.input) + if got != tt.want { + t.Errorf( + "escapeLike(%q) = %q, want %q", + tt.input, got, tt.want, + ) + } + } +} + +func TestPrefixFilterWithSpecialChars(t *testing.T) { + db := openTestDB(t) + defer db.Close() + + // Create repos with LIKE-special chars in path + createRepo(t, db, "/tmp/workspace/repo_one") + createRepo(t, db, "/tmp/workspace/repo%two") + createRepo(t, db, "/tmp/other/repo") + + // Add a job to each repo + repo1, _ := db.GetRepoByPath("/tmp/workspace/repo_one") + commit1 := createCommit(t, db, repo1.ID, "sha1") + enqueueJob(t, db, repo1.ID, commit1.ID, "sha1") + + repo2, _ := db.GetRepoByPath("/tmp/workspace/repo%two") + commit2 := createCommit(t, db, repo2.ID, "sha2") + enqueueJob(t, db, repo2.ID, commit2.ID, "sha2") + + repo3, _ := db.GetRepoByPath("/tmp/other/repo") + commit3 := createCommit(t, db, repo3.ID, "sha3") + enqueueJob(t, db, repo3.ID, commit3.ID, "sha3") + + t.Run("prefix with underscore matches correctly", func(t *testing.T) { + jobs, err := db.ListJobs( + "", "", 50, 0, WithRepoPrefix("/tmp/workspace"), + ) + if err != nil { + t.Fatalf("ListJobs failed: %v", err) + } + if len(jobs) != 2 { + t.Errorf("Expected 2 jobs under /tmp/workspace, got %d", len(jobs)) + } + }) + + t.Run("prefix filter excludes non-matching", func(t *testing.T) { + jobs, err := db.ListJobs( + "", "", 50, 0, WithRepoPrefix("/tmp/other"), + ) + if err != nil { + t.Fatalf("ListJobs failed: %v", err) + } + if len(jobs) != 1 { + t.Errorf("Expected 1 job under /tmp/other, got %d", len(jobs)) + } + }) +} diff --git a/internal/storage/jobs.go b/internal/storage/jobs.go index 82b0757d..b1731416 100644 --- a/internal/storage/jobs.go +++ b/internal/storage/jobs.go @@ -1383,10 +1383,12 @@ func WithRepoPrefix(prefix string) ListJobsOption { } // escapeLike escapes SQL LIKE wildcards (% and _) in a literal string. +// Uses '!' as the ESCAPE character to avoid conflicts with backslashes +// in Windows paths stored in root_path. func escapeLike(s string) string { - s = strings.ReplaceAll(s, "\\", "\\\\") - s = strings.ReplaceAll(s, "%", "\\%") - s = strings.ReplaceAll(s, "_", "\\_") + s = strings.ReplaceAll(s, "!", "!!") + s = strings.ReplaceAll(s, "%", "!%") + s = strings.ReplaceAll(s, "_", "!_") return s } @@ -1419,7 +1421,7 @@ func (db *DB) ListJobs(statusFilter string, repoFilter string, limit, offset int opt(&o) } if repoFilter == "" && o.repoPrefix != "" { - conditions = append(conditions, "r.root_path LIKE ? || '/%' ESCAPE '\\'") + conditions = append(conditions, "r.root_path LIKE ? || '/%' ESCAPE '!'") args = append(args, o.repoPrefix) } if o.gitRef != "" { @@ -1590,7 +1592,7 @@ func (db *DB) CountJobStats(repoFilter string, opts ...ListJobsOption) (JobStats opt(&o) } if repoFilter == "" && o.repoPrefix != "" { - conditions = append(conditions, "r.root_path LIKE ? || '/%' ESCAPE '\\'") + conditions = append(conditions, "r.root_path LIKE ? || '/%' ESCAPE '!'") args = append(args, o.repoPrefix) } if o.branch != "" { diff --git a/internal/storage/repos.go b/internal/storage/repos.go index 64a8fba4..0ccd2aba 100644 --- a/internal/storage/repos.go +++ b/internal/storage/repos.go @@ -146,7 +146,7 @@ func (db *DB) ListReposWithReviewCountsByPrefix(prefix string) ([]RepoWithCount, SELECT r.name, r.root_path, COUNT(rj.id) as job_count FROM repos r LEFT JOIN review_jobs rj ON rj.repo_id = r.id - WHERE r.root_path LIKE ? || '/%' ESCAPE '\' + WHERE r.root_path LIKE ? || '/%' ESCAPE '!' GROUP BY r.id, r.name, r.root_path ORDER BY r.name `, escaped) From 9f9ae89e92cf6c4caf965520f6df8cf2a77e4558 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 4 Mar 2026 20:38:49 -0600 Subject: [PATCH 03/10] Refactor repos query to composable options with path normalization Replace three near-identical ListReposWithReviewCounts* functions with a single ListReposWithReviewCounts(opts ...ListReposOption) using composable WithRepoPathPrefix and WithRepoBranch options. This matches the existing ListJobsOption pattern in jobs.go and enables combined prefix+branch filtering that was previously impossible (the if/else chain in handleListRepos made them mutually exclusive). Add filepath.Clean normalization for repo_prefix and prefix query params in handleListJobs and handleListRepos to handle trailing slashes and ".." segments. Add tests for combined prefix+branch filtering, trailing-slash normalization, and dot-dot path normalization. Co-Authored-By: Claude Opus 4.6 --- internal/daemon/server.go | 24 ++--- internal/daemon/server_jobs_test.go | 34 +++++++ internal/daemon/server_repos_test.go | 55 ++++++++++++ internal/storage/db_filter_test.go | 94 ++++++++++++++++++-- internal/storage/repos.go | 128 +++++++++++++-------------- 5 files changed, 248 insertions(+), 87 deletions(-) diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 58c77a14..598eb3c5 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -10,6 +10,7 @@ import ( "log" "net/http" "os" + "path/filepath" "strconv" "strings" "sync" @@ -816,6 +817,9 @@ func (s *Server) handleListJobs(w http.ResponseWriter, r *http.Request) { repo := r.URL.Query().Get("repo") gitRef := r.URL.Query().Get("git_ref") repoPrefix := r.URL.Query().Get("repo_prefix") + if repoPrefix != "" { + repoPrefix = filepath.Clean(repoPrefix) + } // Parse limit from query, default to 50, 0 means no limit // Clamp to valid range: 0 (unlimited) or 1-10000 @@ -918,22 +922,20 @@ func (s *Server) handleListRepos(w http.ResponseWriter, r *http.Request) { return } - // Optional branch filter branch := r.URL.Query().Get("branch") - - var repos []storage.RepoWithCount - var totalCount int - var err error - prefix := r.URL.Query().Get("prefix") + if prefix != "" { + prefix = filepath.Clean(prefix) + } + var repoOpts []storage.ListReposOption if prefix != "" { - repos, totalCount, err = s.db.ListReposWithReviewCountsByPrefix(prefix) - } else if branch != "" { - repos, totalCount, err = s.db.ListReposWithReviewCountsByBranch(branch) - } else { - repos, totalCount, err = s.db.ListReposWithReviewCounts() + repoOpts = append(repoOpts, storage.WithRepoPathPrefix(prefix)) + } + if branch != "" { + repoOpts = append(repoOpts, storage.WithRepoBranch(branch)) } + repos, totalCount, err := s.db.ListReposWithReviewCounts(repoOpts...) if err != nil { writeError(w, http.StatusInternalServerError, fmt.Sprintf("list repos: %v", err)) return diff --git a/internal/daemon/server_jobs_test.go b/internal/daemon/server_jobs_test.go index a3e18be3..e9353486 100644 --- a/internal/daemon/server_jobs_test.go +++ b/internal/daemon/server_jobs_test.go @@ -1368,4 +1368,38 @@ func TestHandleListJobsRepoPrefixFilter(t *testing.T) { t.Errorf("Expected 3 jobs for exact repo (repo takes precedence), got %d", len(resp.Jobs)) } }) + + t.Run("repo_prefix trailing slash is normalized", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/api/jobs?repo_prefix="+url.QueryEscape(workspace+"/"), nil) + w := httptest.NewRecorder() + server.handleListJobs(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var resp struct { + Jobs []storage.ReviewJob `json:"jobs"` + } + testutil.DecodeJSON(t, w, &resp) + + if len(resp.Jobs) != 5 { + t.Errorf("Expected 5 jobs with trailing-slash prefix, got %d", len(resp.Jobs)) + } + }) + + t.Run("repo_prefix with dot-dot is normalized", func(t *testing.T) { + // workspace/../workspace should normalize to workspace + dotdotPrefix := workspace + "/../" + filepath.Base(workspace) + req := httptest.NewRequest(http.MethodGet, "/api/jobs?repo_prefix="+url.QueryEscape(dotdotPrefix), nil) + w := httptest.NewRecorder() + server.handleListJobs(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var resp struct { + Jobs []storage.ReviewJob `json:"jobs"` + } + testutil.DecodeJSON(t, w, &resp) + + if len(resp.Jobs) != 5 { + t.Errorf("Expected 5 jobs with dot-dot prefix, got %d", len(resp.Jobs)) + } + }) } diff --git a/internal/daemon/server_repos_test.go b/internal/daemon/server_repos_test.go index 8483c471..82ea667d 100644 --- a/internal/daemon/server_repos_test.go +++ b/internal/daemon/server_repos_test.go @@ -197,6 +197,61 @@ func TestHandleListReposWithPrefixFilter(t *testing.T) { t.Errorf("Expected 0 repos for nonexistent prefix, got %d", len(response.Repos)) } }) + + // Set branches on workspace repos: repo1 jobs 1,2=main, 3=feature; repo2 jobs 4,5=main + setJobBranch(t, db, 1, "main") + setJobBranch(t, db, 2, "main") + setJobBranch(t, db, 3, "feature") + setJobBranch(t, db, 4, "main") + setJobBranch(t, db, 5, "main") + + t.Run("prefix + branch combined filter", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, + "/api/repos?prefix="+url.QueryEscape(workspace)+"&branch=main", nil) + w := httptest.NewRecorder() + server.handleListRepos(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var response reposResponse + testutil.DecodeJSON(t, w, &response) + if len(response.Repos) != 2 { + t.Errorf("Expected 2 repos with prefix+branch=main, got %d", len(response.Repos)) + } + if response.TotalCount != 4 { + t.Errorf("Expected total_count 4, got %d", response.TotalCount) + } + }) + + t.Run("prefix + feature branch narrows results", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, + "/api/repos?prefix="+url.QueryEscape(workspace)+"&branch=feature", nil) + w := httptest.NewRecorder() + server.handleListRepos(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var response reposResponse + testutil.DecodeJSON(t, w, &response) + if len(response.Repos) != 1 { + t.Errorf("Expected 1 repo with prefix+branch=feature, got %d", len(response.Repos)) + } + if response.TotalCount != 1 { + t.Errorf("Expected total_count 1, got %d", response.TotalCount) + } + }) + + t.Run("prefix with trailing slash is normalized", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, + "/api/repos?prefix="+url.QueryEscape(workspace+"/"), nil) + w := httptest.NewRecorder() + server.handleListRepos(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var response reposResponse + testutil.DecodeJSON(t, w, &response) + if len(response.Repos) != 2 { + t.Errorf("Expected 2 repos with trailing-slash prefix, got %d", len(response.Repos)) + } + }) } func TestHandleListBranches(t *testing.T) { diff --git a/internal/storage/db_filter_test.go b/internal/storage/db_filter_test.go index a5288dd5..d6ba6158 100644 --- a/internal/storage/db_filter_test.go +++ b/internal/storage/db_filter_test.go @@ -682,9 +682,9 @@ func TestListReposWithReviewCountsByBranch(t *testing.T) { setJobBranch(t, db, job2.ID, "feature") t.Run("filter by main branch", func(t *testing.T) { - repos, totalCount, err := db.ListReposWithReviewCountsByBranch("main") + repos, totalCount, err := db.ListReposWithReviewCounts(WithRepoBranch("main")) if err != nil { - t.Fatalf("ListReposWithReviewCountsByBranch failed: %v", err) + t.Fatalf("ListReposWithReviewCounts(branch=main) failed: %v", err) } if len(repos) != 2 { t.Errorf("Expected 2 repos with main branch, got %d", len(repos)) @@ -695,9 +695,9 @@ func TestListReposWithReviewCountsByBranch(t *testing.T) { }) t.Run("filter by feature branch", func(t *testing.T) { - repos, totalCount, err := db.ListReposWithReviewCountsByBranch("feature") + repos, totalCount, err := db.ListReposWithReviewCounts(WithRepoBranch("feature")) if err != nil { - t.Fatalf("ListReposWithReviewCountsByBranch failed: %v", err) + t.Fatalf("ListReposWithReviewCounts(branch=feature) failed: %v", err) } if len(repos) != 1 { t.Errorf("Expected 1 repo with feature branch, got %d", len(repos)) @@ -712,9 +712,9 @@ func TestListReposWithReviewCountsByBranch(t *testing.T) { commit4 := createCommit(t, db, repo1.ID, "jkl012") enqueueJob(t, db, repo1.ID, commit4.ID, "jkl012") - repos, totalCount, err := db.ListReposWithReviewCountsByBranch("(none)") + repos, totalCount, err := db.ListReposWithReviewCounts(WithRepoBranch("(none)")) if err != nil { - t.Fatalf("ListReposWithReviewCountsByBranch failed: %v", err) + t.Fatalf("ListReposWithReviewCounts(branch=(none)) failed: %v", err) } if len(repos) != 1 { t.Errorf("Expected 1 repo with (none) branch, got %d", len(repos)) @@ -725,9 +725,9 @@ func TestListReposWithReviewCountsByBranch(t *testing.T) { }) t.Run("empty filter returns all", func(t *testing.T) { - repos, _, err := db.ListReposWithReviewCountsByBranch("") + repos, _, err := db.ListReposWithReviewCounts() if err != nil { - t.Fatalf("ListReposWithReviewCountsByBranch failed: %v", err) + t.Fatalf("ListReposWithReviewCounts() failed: %v", err) } if len(repos) != 2 { t.Errorf("Expected 2 repos, got %d", len(repos)) @@ -987,3 +987,81 @@ func TestPrefixFilterWithSpecialChars(t *testing.T) { } }) } + +func TestListReposWithCombinedPrefixAndBranch(t *testing.T) { + db := openTestDB(t) + defer db.Close() + + // Create repos under workspace and one outside + r1 := createRepo(t, db, "/ws/repo-a") + r2 := createRepo(t, db, "/ws/repo-b") + r3 := createRepo(t, db, "/other/repo-c") + + // repo-a: 2 jobs on main, 1 on feature + c1 := createCommit(t, db, r1.ID, "a1") + c2 := createCommit(t, db, r1.ID, "a2") + c3 := createCommit(t, db, r1.ID, "a3") + j1 := enqueueJob(t, db, r1.ID, c1.ID, "a1") + j2 := enqueueJob(t, db, r1.ID, c2.ID, "a2") + j3 := enqueueJob(t, db, r1.ID, c3.ID, "a3") + setJobBranch(t, db, j1.ID, "main") + setJobBranch(t, db, j2.ID, "main") + setJobBranch(t, db, j3.ID, "feature") + + // repo-b: 1 job on main + c4 := createCommit(t, db, r2.ID, "b1") + j4 := enqueueJob(t, db, r2.ID, c4.ID, "b1") + setJobBranch(t, db, j4.ID, "main") + + // repo-c (outside workspace): 1 job on main + c5 := createCommit(t, db, r3.ID, "c1") + j5 := enqueueJob(t, db, r3.ID, c5.ID, "c1") + setJobBranch(t, db, j5.ID, "main") + + t.Run("prefix + branch filters both", func(t *testing.T) { + repos, total, err := db.ListReposWithReviewCounts( + WithRepoPathPrefix("/ws"), + WithRepoBranch("main"), + ) + if err != nil { + t.Fatalf("ListReposWithReviewCounts failed: %v", err) + } + if len(repos) != 2 { + t.Errorf("Expected 2 repos under /ws with main, got %d", len(repos)) + } + if total != 3 { + t.Errorf("Expected total 3 (2+1), got %d", total) + } + }) + + t.Run("prefix + feature branch", func(t *testing.T) { + repos, total, err := db.ListReposWithReviewCounts( + WithRepoPathPrefix("/ws"), + WithRepoBranch("feature"), + ) + if err != nil { + t.Fatalf("ListReposWithReviewCounts failed: %v", err) + } + if len(repos) != 1 { + t.Errorf("Expected 1 repo, got %d", len(repos)) + } + if total != 1 { + t.Errorf("Expected total 1, got %d", total) + } + }) + + t.Run("prefix only returns all branches", func(t *testing.T) { + repos, total, err := db.ListReposWithReviewCounts( + WithRepoPathPrefix("/ws"), + ) + if err != nil { + t.Fatalf("ListReposWithReviewCounts failed: %v", err) + } + if len(repos) != 2 { + t.Errorf("Expected 2 repos under /ws, got %d", len(repos)) + } + if total != 4 { + t.Errorf("Expected total 4 (3+1), got %d", total) + } + }) +} diff --git a/internal/storage/repos.go b/internal/storage/repos.go index 0ccd2aba..0590ed20 100644 --- a/internal/storage/repos.go +++ b/internal/storage/repos.go @@ -111,89 +111,81 @@ type RepoWithCount struct { Count int `json:"count"` } -// ListReposWithReviewCounts returns all repos with their total job counts -func (db *DB) ListReposWithReviewCounts() ([]RepoWithCount, int, error) { - // Query repos with their job counts (includes queued/running, not just completed reviews) - rows, err := db.Query(` - SELECT r.name, r.root_path, COUNT(rj.id) as job_count - FROM repos r - LEFT JOIN review_jobs rj ON rj.repo_id = r.id - GROUP BY r.id, r.name, r.root_path - ORDER BY r.name - `) - if err != nil { - return nil, 0, err +// ListReposOption configures filtering for ListReposWithReviewCounts. +type ListReposOption func(*listReposOptions) + +type listReposOptions struct { + prefix string + branch string +} + +// WithRepoPathPrefix filters repos whose root_path starts with the given prefix. +func WithRepoPathPrefix(prefix string) ListReposOption { + return func(o *listReposOptions) { o.prefix = prefix } +} + +// WithRepoBranch filters repos to those having jobs on the given branch. +// Use "(none)" to filter for jobs without a branch. +func WithRepoBranch(branch string) ListReposOption { + return func(o *listReposOptions) { o.branch = branch } +} + +// ListReposWithReviewCounts returns repos with their total job counts. +// Options can filter by path prefix, branch, or both. +func (db *DB) ListReposWithReviewCounts(opts ...ListReposOption) ([]RepoWithCount, int, error) { + var o listReposOptions + for _, opt := range opts { + opt(&o) } - defer rows.Close() - var repos []RepoWithCount - totalCount := 0 - for rows.Next() { - var rc RepoWithCount - if err := rows.Scan(&rc.Name, &rc.RootPath, &rc.Count); err != nil { - return nil, 0, err - } - repos = append(repos, rc) - totalCount += rc.Count + // Branch filtering requires INNER JOIN + HAVING to exclude repos + // with no matching jobs. Without branch filter, LEFT JOIN shows all repos. + joinType := "LEFT" + if o.branch != "" { + joinType = "INNER" } - return repos, totalCount, rows.Err() -} -// ListReposWithReviewCountsByPrefix returns repos whose root_path starts with the given prefix, with their job counts -func (db *DB) ListReposWithReviewCountsByPrefix(prefix string) ([]RepoWithCount, int, error) { - escaped := escapeLike(prefix) - rows, err := db.Query(` + query := fmt.Sprintf(` SELECT r.name, r.root_path, COUNT(rj.id) as job_count FROM repos r - LEFT JOIN review_jobs rj ON rj.repo_id = r.id - WHERE r.root_path LIKE ? || '/%' ESCAPE '!' - GROUP BY r.id, r.name, r.root_path - ORDER BY r.name - `, escaped) - if err != nil { - return nil, 0, err + %s JOIN review_jobs rj ON rj.repo_id = r.id + `, joinType) + + var args []any + var conditions []string + + if o.prefix != "" { + conditions = append( + conditions, + "r.root_path LIKE ? || '/%' ESCAPE '!'", + ) + args = append(args, escapeLike(o.prefix)) } - defer rows.Close() - var repos []RepoWithCount - totalCount := 0 - for rows.Next() { - var rc RepoWithCount - if err := rows.Scan(&rc.Name, &rc.RootPath, &rc.Count); err != nil { - return nil, 0, err + if o.branch != "" { + branchFilter := o.branch + if o.branch == "(none)" { + branchFilter = "" } - repos = append(repos, rc) - totalCount += rc.Count + conditions = append( + conditions, "COALESCE(rj.branch, '') = ?", + ) + args = append(args, branchFilter) } - return repos, totalCount, rows.Err() -} - -// ListReposWithReviewCountsByBranch returns repos filtered by branch with their job counts -// If branch is empty, returns all repos. Use "(none)" to filter for jobs without a branch. -func (db *DB) ListReposWithReviewCountsByBranch(branch string) ([]RepoWithCount, int, error) { - var rows *sql.Rows - var err error - if branch == "" { - // No filter - return all repos - return db.ListReposWithReviewCounts() + if len(conditions) > 0 { + query += " WHERE " + strings.Join(conditions, " AND ") } - // Filter by branch (handle "(none)" as NULL/empty branch) - branchFilter := branch - if branch == "(none)" { - branchFilter = "" + query += ` + GROUP BY r.id, r.name, r.root_path + ` + if o.branch != "" { + query += " HAVING job_count > 0" } + query += " ORDER BY r.name" - rows, err = db.Query(` - SELECT r.name, r.root_path, COUNT(rj.id) as job_count - FROM repos r - INNER JOIN review_jobs rj ON rj.repo_id = r.id - WHERE COALESCE(rj.branch, '') = ? - GROUP BY r.id, r.name, r.root_path - HAVING job_count > 0 - ORDER BY r.name - `, branchFilter) + rows, err := db.Query(query, args...) if err != nil { return nil, 0, err } From 446e58595ba4a12e38541e8f077b3915a796a329 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 4 Mar 2026 20:39:16 -0600 Subject: [PATCH 04/10] Fix --branch suppression in workspace mode When running from a non-git parent directory (workspace mode), the auto-detected branch was correctly suppressed since it makes no sense to filter by one repo's branch across a workspace. However, an explicit --branch flag was also being dropped. Check cmd.Flags().Changed to honor explicitly passed --branch values in workspace mode. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/roborev/list.go b/cmd/roborev/list.go index a35a0d2f..c75e1654 100644 --- a/cmd/roborev/list.go +++ b/cmd/roborev/list.go @@ -91,7 +91,7 @@ Examples: } else if repoPath != "" { params.Set("repo", repoPath) } - if branch != "" && repoPrefix == "" { + if branch != "" && (repoPrefix == "" || cmd.Flags().Changed("branch")) { params.Set("branch", branch) params.Set("branch_include_empty", "true") } From eddfabb8f5cd9961586dce515f7418fba3ac5edd Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 4 Mar 2026 20:40:05 -0600 Subject: [PATCH 05/10] Fix findChildGitRepos .git file detection and hint paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit findChildGitRepos checked info.IsDir() for .git, which missed git worktrees and submodules where .git is a file pointing to the actual git directory. Remove the IsDir() check — os.Stat succeeding is sufficient to confirm the entry is a git repo. The hint path fix (showing full absolute paths instead of bare repo names) was included in the earlier ESCAPE commit since it touched the same code. This commit adds the .git file detection fix and tests for both behaviors. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/review.go | 2 +- cmd/roborev/review_test.go | 75 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/cmd/roborev/review.go b/cmd/roborev/review.go index f262e501..96065261 100644 --- a/cmd/roborev/review.go +++ b/cmd/roborev/review.go @@ -438,7 +438,7 @@ func findChildGitRepos(dir string) []string { continue } gitDir := filepath.Join(dir, e.Name(), ".git") - if info, err := os.Stat(gitDir); err == nil && info.IsDir() { + if _, err := os.Stat(gitDir); err == nil { repos = append(repos, e.Name()) } } diff --git a/cmd/roborev/review_test.go b/cmd/roborev/review_test.go index 66eced7d..8b46372d 100644 --- a/cmd/roborev/review_test.go +++ b/cmd/roborev/review_test.go @@ -548,3 +548,78 @@ func TestReviewInvalidArgsNoSideEffects(t *testing.T) { } } } + +func TestFindChildGitRepos(t *testing.T) { + parent := t.TempDir() + + // Create a regular git repo (directory .git) + regularRepo := filepath.Join(parent, "regular") + os.Mkdir(regularRepo, 0755) + os.Mkdir(filepath.Join(regularRepo, ".git"), 0755) + + // Create a worktree-style repo (.git is a file) + worktreeRepo := filepath.Join(parent, "worktree") + os.Mkdir(worktreeRepo, 0755) + os.WriteFile( + filepath.Join(worktreeRepo, ".git"), + []byte("gitdir: /some/main/.git/worktrees/wt"), + 0644, + ) + + // Create a non-repo directory (no .git at all) + plainDir := filepath.Join(parent, "plain") + os.Mkdir(plainDir, 0755) + + // Create a hidden directory (should be skipped) + hiddenDir := filepath.Join(parent, ".hidden") + os.Mkdir(hiddenDir, 0755) + os.Mkdir(filepath.Join(hiddenDir, ".git"), 0755) + + repos := findChildGitRepos(parent) + + if len(repos) != 2 { + t.Fatalf("Expected 2 repos, got %d: %v", len(repos), repos) + } + + found := make(map[string]bool) + for _, r := range repos { + found[r] = true + } + if !found["regular"] { + t.Error("Expected 'regular' in results") + } + if !found["worktree"] { + t.Error("Expected 'worktree' in results (has .git file)") + } + if found["plain"] { + t.Error("'plain' should not be in results") + } + if found[".hidden"] { + t.Error("'.hidden' should not be in results") + } +} + +func TestFindChildGitReposHintPaths(t *testing.T) { + parent := t.TempDir() + + // Create a regular git repo + repoDir := filepath.Join(parent, "my-repo") + os.Mkdir(repoDir, 0755) + os.Mkdir(filepath.Join(repoDir, ".git"), 0755) + + // Run the review command against the parent dir (not a git repo) + // to verify the hint message contains full paths + _, _, err := executeReviewCmd("--repo", parent) + if err == nil { + t.Fatal("Expected error for non-git directory") + } + + errMsg := err.Error() + expectedPath := filepath.Join(parent, "my-repo") + if !strings.Contains(errMsg, expectedPath) { + t.Errorf( + "Hint should contain full path %q, got: %s", + expectedPath, errMsg, + ) + } +} From 6724463f1c7a33763fecf7db96caf6d1c3e83e1c Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 4 Mar 2026 20:46:41 -0600 Subject: [PATCH 06/10] Add test coverage for review findings #7920, #7922, #7923 #7920: Add CountJobStats and ListReposWithReviewCounts prefix tests with special chars (!, %, _) and backslash-in-path test verifying no SQL errors after the ESCAPE char change. #7922: Add dot-dot normalization test for /api/repos prefix endpoint. #7923: Add workspace-mode integration tests verifying auto-detected branch is suppressed and explicit --branch is honored. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/list_test.go | 28 ++++++++++++ internal/daemon/server_repos_test.go | 15 +++++++ internal/storage/db_filter_test.go | 65 ++++++++++++++++++++++++++++ 3 files changed, 108 insertions(+) diff --git a/cmd/roborev/list_test.go b/cmd/roborev/list_test.go index 4cfcfb99..49901421 100644 --- a/cmd/roborev/list_test.go +++ b/cmd/roborev/list_test.go @@ -240,6 +240,34 @@ func TestListCommand(t *testing.T) { } }, }, + { + name: "workspace mode suppresses auto-detected branch", + repoSetup: func(t *testing.T) repoSetupResult { + // Create a non-git parent dir (workspace mode) + parent := t.TempDir() + return repoSetupResult{ + workingDir: parent, + repo: nil, + extraArgs: nil, + } + }, + handler: jobsHandler([]storage.ReviewJob{}, false), + wantQuery: []string{"repo_prefix="}, + notWantQuery: []string{"branch="}, + }, + { + name: "workspace mode honors explicit --branch", + repoSetup: func(t *testing.T) repoSetupResult { + parent := t.TempDir() + return repoSetupResult{ + workingDir: parent, + repo: nil, + extraArgs: []string{"--branch", "main"}, + } + }, + handler: jobsHandler([]storage.ReviewJob{}, false), + wantQuery: []string{"repo_prefix=", "branch=main", "branch_include_empty=true"}, + }, } for _, tc := range tests { diff --git a/internal/daemon/server_repos_test.go b/internal/daemon/server_repos_test.go index 82ea667d..e289ea62 100644 --- a/internal/daemon/server_repos_test.go +++ b/internal/daemon/server_repos_test.go @@ -252,6 +252,21 @@ func TestHandleListReposWithPrefixFilter(t *testing.T) { t.Errorf("Expected 2 repos with trailing-slash prefix, got %d", len(response.Repos)) } }) + + t.Run("prefix with dot-dot is normalized", func(t *testing.T) { + dotdotPrefix := workspace + "/../" + filepath.Base(workspace) + req := httptest.NewRequest(http.MethodGet, + "/api/repos?prefix="+url.QueryEscape(dotdotPrefix), nil) + w := httptest.NewRecorder() + server.handleListRepos(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var response reposResponse + testutil.DecodeJSON(t, w, &response) + if len(response.Repos) != 2 { + t.Errorf("Expected 2 repos with dot-dot prefix, got %d", len(response.Repos)) + } + }) } func TestHandleListBranches(t *testing.T) { diff --git a/internal/storage/db_filter_test.go b/internal/storage/db_filter_test.go index d6ba6158..9d8c36a2 100644 --- a/internal/storage/db_filter_test.go +++ b/internal/storage/db_filter_test.go @@ -986,6 +986,71 @@ func TestPrefixFilterWithSpecialChars(t *testing.T) { t.Errorf("Expected 1 job under /tmp/other, got %d", len(jobs)) } }) + + t.Run("CountJobStats with special-char prefix", func(t *testing.T) { + stats, err := db.CountJobStats( + "", WithRepoPrefix("/tmp/workspace"), + ) + if err != nil { + t.Fatalf("CountJobStats failed: %v", err) + } + if stats.Done+stats.Open != 0 { + t.Errorf( + "Expected 0 done stats (jobs are queued), got done=%d open=%d", + stats.Done, stats.Open, + ) + } + }) + + t.Run("ListReposWithReviewCounts with special-char prefix", func(t *testing.T) { + repos, total, err := db.ListReposWithReviewCounts( + WithRepoPathPrefix("/tmp/workspace"), + ) + if err != nil { + t.Fatalf("ListReposWithReviewCounts failed: %v", err) + } + if len(repos) != 2 { + t.Errorf("Expected 2 repos, got %d", len(repos)) + } + if total != 2 { + t.Errorf("Expected total 2, got %d", total) + } + }) + + t.Run("backslash in path does not cause SQL error", func(t *testing.T) { + // Verify that backslashes in paths are passed through correctly + // to LIKE (no longer escaped as ESCAPE char). The LIKE pattern + // uses '/' as separator so Windows-native paths with '\' won't + // match the prefix filter — on Windows, filepath.ToSlash should + // normalize paths before storage. This test ensures no SQL errors. + createRepo(t, db, `C:\Users\dev\workspace\project-a`) + rA, _ := db.GetRepoByPath(`C:\Users\dev\workspace\project-a`) + cA := createCommit(t, db, rA.ID, "win-a") + enqueueJob(t, db, rA.ID, cA.ID, "win-a") + + // Should not error — the old '\' ESCAPE char would have + // caused invalid escape sequences with Windows paths. + _, err := db.ListJobs( + "", "", 50, 0, WithRepoPrefix(`C:\Users\dev\workspace`), + ) + if err != nil { + t.Fatalf("ListJobs with backslash prefix should not error: %v", err) + } + + _, err = db.CountJobStats( + "", WithRepoPrefix(`C:\Users\dev\workspace`), + ) + if err != nil { + t.Fatalf("CountJobStats with backslash prefix should not error: %v", err) + } + + _, _, err = db.ListReposWithReviewCounts( + WithRepoPathPrefix(`C:\Users\dev\workspace`), + ) + if err != nil { + t.Fatalf("ListReposWithReviewCounts with backslash prefix should not error: %v", err) + } + }) } func TestListReposWithCombinedPrefixAndBranch(t *testing.T) { From bef28e3dce3a8a8ff6ae382cd075694ebcfe46d7 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 4 Mar 2026 20:50:27 -0600 Subject: [PATCH 07/10] Fix CountJobStats prefix test to verify filter correctness Complete seeded jobs before asserting CountJobStats so the test validates actual prefix-filter behavior (2 done under prefix, not 3) instead of vacuously passing with all-zero stats from queued jobs. Co-Authored-By: Claude Opus 4.6 --- internal/storage/db_filter_test.go | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/internal/storage/db_filter_test.go b/internal/storage/db_filter_test.go index 9d8c36a2..f90b901c 100644 --- a/internal/storage/db_filter_test.go +++ b/internal/storage/db_filter_test.go @@ -987,6 +987,16 @@ func TestPrefixFilterWithSpecialChars(t *testing.T) { } }) + // Complete workspace jobs so CountJobStats has done counts to verify. + // repo1 (repo_one) job -> done, repo2 (repo%two) job -> done, + // repo3 (other) job -> done (outside prefix). + for range 3 { + claimed := claimJob(t, db, "w1") + if err := db.CompleteJob(claimed.ID, "codex", "p", "o"); err != nil { + t.Fatalf("CompleteJob failed: %v", err) + } + } + t.Run("CountJobStats with special-char prefix", func(t *testing.T) { stats, err := db.CountJobStats( "", WithRepoPrefix("/tmp/workspace"), @@ -994,11 +1004,13 @@ func TestPrefixFilterWithSpecialChars(t *testing.T) { if err != nil { t.Fatalf("CountJobStats failed: %v", err) } - if stats.Done+stats.Open != 0 { - t.Errorf( - "Expected 0 done stats (jobs are queued), got done=%d open=%d", - stats.Done, stats.Open, - ) + // 2 done jobs under /tmp/workspace (repo_one + repo%two), + // not 3 (repo3 is under /tmp/other). + if stats.Done != 2 { + t.Errorf("Expected 2 done under prefix, got %d", stats.Done) + } + if stats.Open != 2 { + t.Errorf("Expected 2 open under prefix, got %d", stats.Open) } }) From b0d57d567711e8e4f225b2a676f4a207aa07fb8d Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 4 Mar 2026 21:04:44 -0600 Subject: [PATCH 08/10] Normalize prefix filters to forward slashes for Windows compatibility Add filepath.ToSlash after filepath.Clean on repo_prefix and prefix query params in handleListJobs and handleListRepos, and on the client-side prefix in list.go. The SQL LIKE clause uses '/%' as the separator, so backslash-separated Windows paths from filepath.Abs or filepath.Clean would fail to match. Add tests for roborev show outside a git repo: default invocation returns a guidance error, explicit --job ID still works. Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/list.go | 2 +- cmd/roborev/show_test.go | 33 +++++++++++++++++++++++++++++++++ internal/daemon/server.go | 4 ++-- 3 files changed, 36 insertions(+), 3 deletions(-) diff --git a/cmd/roborev/list.go b/cmd/roborev/list.go index c75e1654..6f0313ed 100644 --- a/cmd/roborev/list.go +++ b/cmd/roborev/list.go @@ -80,7 +80,7 @@ Examples: var repoPrefix string if repoPath == "" && localRepoPath == "" { if abs, err := filepath.Abs("."); err == nil { - repoPrefix = abs + repoPrefix = filepath.ToSlash(abs) } } diff --git a/cmd/roborev/show_test.go b/cmd/roborev/show_test.go index c31dff97..5cffa31c 100644 --- a/cmd/roborev/show_test.go +++ b/cmd/roborev/show_test.go @@ -141,6 +141,39 @@ func TestShowOutputFormat(t *testing.T) { } } +func TestShowOutsideGitRepo(t *testing.T) { + t.Run("no args outside git repo returns guidance error", func(t *testing.T) { + nonGitDir := t.TempDir() + chdir(t, nonGitDir) + + mockReviewDaemon(t, storage.Review{}) + + cmd := showCmd() + cmd.SetArgs([]string{}) + err := cmd.Execute() + assertErrorContains(t, err, "not in a git repository") + assertErrorContains(t, err, "job ID") + }) + + t.Run("job ID outside git repo still works", func(t *testing.T) { + nonGitDir := t.TempDir() + chdir(t, nonGitDir) + + getQuery := mockReviewDaemon(t, storage.Review{ + ID: 1, JobID: 42, Output: "LGTM", Agent: "test", + }) + + output := runShowCmd(t, "--job", "42") + q := getQuery() + if !strings.Contains(q, "job_id=42") { + t.Errorf("expected job_id=42 in query, got: %s", q) + } + if !strings.Contains(output, "LGTM") { + t.Errorf("expected review output in result, got: %s", output) + } + }) +} + func TestShowJSONOutput(t *testing.T) { repo := newTestGitRepo(t) repo.CommitFile("file.txt", "content", "initial commit") diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 598eb3c5..632714bf 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -818,7 +818,7 @@ func (s *Server) handleListJobs(w http.ResponseWriter, r *http.Request) { gitRef := r.URL.Query().Get("git_ref") repoPrefix := r.URL.Query().Get("repo_prefix") if repoPrefix != "" { - repoPrefix = filepath.Clean(repoPrefix) + repoPrefix = filepath.ToSlash(filepath.Clean(repoPrefix)) } // Parse limit from query, default to 50, 0 means no limit @@ -925,7 +925,7 @@ func (s *Server) handleListRepos(w http.ResponseWriter, r *http.Request) { branch := r.URL.Query().Get("branch") prefix := r.URL.Query().Get("prefix") if prefix != "" { - prefix = filepath.Clean(prefix) + prefix = filepath.ToSlash(filepath.Clean(prefix)) } var repoOpts []storage.ListReposOption From cf7bd8092cb904ad374efa30f9a0d5d5409b6a4b Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 4 Mar 2026 21:16:04 -0600 Subject: [PATCH 09/10] Add slash normalization tests for prefix filter handlers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add TestHandleListJobsSlashNormalization and TestHandleListReposSlashNormalization verifying that forward-slash prefixes (the output of filepath.ToSlash) correctly match stored repo paths through the handler pipeline. Note: filepath.ToSlash is already platform-conditional by design — it replaces os.PathSeparator with '/', which is a no-op on POSIX (where os.PathSeparator is already '/'). Backslashes in POSIX path segments are valid filename characters and are not transformed. Co-Authored-By: Claude Opus 4.6 --- internal/daemon/server_jobs_test.go | 30 ++++++++++++++++++++++++ internal/daemon/server_repos_test.go | 35 ++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/internal/daemon/server_jobs_test.go b/internal/daemon/server_jobs_test.go index e9353486..72ae20ea 100644 --- a/internal/daemon/server_jobs_test.go +++ b/internal/daemon/server_jobs_test.go @@ -1403,3 +1403,33 @@ func TestHandleListJobsRepoPrefixFilter(t *testing.T) { } }) } + +func TestHandleListJobsSlashNormalization(t *testing.T) { + server, db, tmpDir := newTestServer(t) + + // Store repos with forward-slash paths (matching ToSlash output) + ws := tmpDir + "/slash-ws" + seedRepoWithJobs(t, db, ws+"/repo-a", 2, "sa") + seedRepoWithJobs(t, db, ws+"/repo-b", 1, "sb") + seedRepoWithJobs(t, db, tmpDir+"/other-c", 1, "sc") + + t.Run("forward-slash prefix matches stored paths", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, + "/api/jobs?repo_prefix="+url.QueryEscape(ws), nil) + w := httptest.NewRecorder() + server.handleListJobs(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var resp struct { + Jobs []storage.ReviewJob `json:"jobs"` + } + testutil.DecodeJSON(t, w, &resp) + + if len(resp.Jobs) != 3 { + t.Errorf( + "Expected 3 jobs with forward-slash prefix, got %d", + len(resp.Jobs), + ) + } + }) +} diff --git a/internal/daemon/server_repos_test.go b/internal/daemon/server_repos_test.go index e289ea62..3fd30045 100644 --- a/internal/daemon/server_repos_test.go +++ b/internal/daemon/server_repos_test.go @@ -269,6 +269,41 @@ func TestHandleListReposWithPrefixFilter(t *testing.T) { }) } +func TestHandleListReposSlashNormalization(t *testing.T) { + server, db, tmpDir := newTestServer(t) + + // Store repos with forward-slash paths (matching ToSlash output) + ws := tmpDir + "/slash-ws" + seedRepoWithJobs(t, db, ws+"/repo-x", 2, "rx") + seedRepoWithJobs(t, db, ws+"/repo-y", 1, "ry") + seedRepoWithJobs(t, db, tmpDir+"/other-z", 1, "rz") + + type reposResponse struct { + Repos []struct{ Name string } `json:"repos"` + TotalCount int `json:"total_count"` + } + + t.Run("forward-slash prefix matches stored paths", func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, + "/api/repos?prefix="+url.QueryEscape(ws), nil) + w := httptest.NewRecorder() + server.handleListRepos(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var response reposResponse + testutil.DecodeJSON(t, w, &response) + if len(response.Repos) != 2 { + t.Errorf( + "Expected 2 repos with forward-slash prefix, got %d", + len(response.Repos), + ) + } + if response.TotalCount != 3 { + t.Errorf("Expected total_count 3, got %d", response.TotalCount) + } + }) +} + func TestHandleListBranches(t *testing.T) { server, db, tmpDir := newTestServer(t) From 660ce1da5d1c46c92b7491b1ef2cca76851cb2a8 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Wed, 4 Mar 2026 21:22:44 -0600 Subject: [PATCH 10/10] Normalize repo paths to forward slashes at storage boundary GetOrCreateRepo and GetRepoByPath now apply filepath.ToSlash after filepath.Abs so root_path is always stored with forward slashes, matching the LIKE '/%' pattern used by prefix filters. This fixes workspace prefix filtering on Windows where filepath.Abs returns backslash-separated paths. Strip trailing slashes from prefix options (WithRepoPrefix, WithRepoPathPrefix) so root-prefix "/" doesn't produce the double-slash LIKE pattern "//%". A root prefix trims to empty string, disabling the filter (all repos match), which is correct. Strengthen slash normalization test assertions to verify repo identities (names, paths) not just counts. Add TestRootPrefixMatchesAllRepos covering the "/" edge case. Co-Authored-By: Claude Opus 4.6 --- internal/daemon/server_jobs_test.go | 8 ++++++ internal/daemon/server_repos_test.go | 18 ++++++++++-- internal/storage/db_filter_test.go | 41 ++++++++++++++++++++++++++++ internal/storage/jobs.go | 6 +++- internal/storage/repos.go | 9 ++++-- 5 files changed, 77 insertions(+), 5 deletions(-) diff --git a/internal/daemon/server_jobs_test.go b/internal/daemon/server_jobs_test.go index 72ae20ea..85bfce17 100644 --- a/internal/daemon/server_jobs_test.go +++ b/internal/daemon/server_jobs_test.go @@ -1431,5 +1431,13 @@ func TestHandleListJobsSlashNormalization(t *testing.T) { len(resp.Jobs), ) } + for _, j := range resp.Jobs { + if !strings.HasPrefix(j.RepoPath, ws+"/") { + t.Errorf( + "Job %d repo_path %q should be under %s", + j.ID, j.RepoPath, ws, + ) + } + } }) } diff --git a/internal/daemon/server_repos_test.go b/internal/daemon/server_repos_test.go index 3fd30045..8742be35 100644 --- a/internal/daemon/server_repos_test.go +++ b/internal/daemon/server_repos_test.go @@ -278,9 +278,13 @@ func TestHandleListReposSlashNormalization(t *testing.T) { seedRepoWithJobs(t, db, ws+"/repo-y", 1, "ry") seedRepoWithJobs(t, db, tmpDir+"/other-z", 1, "rz") + type repoEntry struct { + Name string `json:"name"` + RootPath string `json:"root_path"` + } type reposResponse struct { - Repos []struct{ Name string } `json:"repos"` - TotalCount int `json:"total_count"` + Repos []repoEntry `json:"repos"` + TotalCount int `json:"total_count"` } t.Run("forward-slash prefix matches stored paths", func(t *testing.T) { @@ -301,6 +305,16 @@ func TestHandleListReposSlashNormalization(t *testing.T) { if response.TotalCount != 3 { t.Errorf("Expected total_count 3, got %d", response.TotalCount) } + names := make(map[string]bool) + for _, r := range response.Repos { + names[r.Name] = true + } + if !names["repo-x"] || !names["repo-y"] { + t.Errorf("Expected repo-x and repo-y, got %v", response.Repos) + } + if names["other-z"] { + t.Error("other-z should not be in prefix-filtered results") + } }) } diff --git a/internal/storage/db_filter_test.go b/internal/storage/db_filter_test.go index f90b901c..a8b2fc2f 100644 --- a/internal/storage/db_filter_test.go +++ b/internal/storage/db_filter_test.go @@ -1065,6 +1065,47 @@ func TestPrefixFilterWithSpecialChars(t *testing.T) { }) } +func TestRootPrefixMatchesAllRepos(t *testing.T) { + db := openTestDB(t) + defer db.Close() + + createRepo(t, db, "/a/repo1") + createRepo(t, db, "/b/repo2") + + r1, _ := db.GetRepoByPath("/a/repo1") + c1 := createCommit(t, db, r1.ID, "r1") + enqueueJob(t, db, r1.ID, c1.ID, "r1") + + r2, _ := db.GetRepoByPath("/b/repo2") + c2 := createCommit(t, db, r2.ID, "r2") + enqueueJob(t, db, r2.ID, c2.ID, "r2") + + t.Run("root prefix returns all repos via ListJobs", func(t *testing.T) { + jobs, err := db.ListJobs("", "", 50, 0, WithRepoPrefix("/")) + if err != nil { + t.Fatalf("ListJobs failed: %v", err) + } + if len(jobs) != 2 { + t.Errorf("Expected 2 jobs with root prefix, got %d", len(jobs)) + } + }) + + t.Run("root prefix returns all repos via ListReposWithReviewCounts", func(t *testing.T) { + repos, total, err := db.ListReposWithReviewCounts( + WithRepoPathPrefix("/"), + ) + if err != nil { + t.Fatalf("ListReposWithReviewCounts failed: %v", err) + } + if len(repos) != 2 { + t.Errorf("Expected 2 repos with root prefix, got %d", len(repos)) + } + if total != 2 { + t.Errorf("Expected total 2, got %d", total) + } + }) +} + func TestListReposWithCombinedPrefixAndBranch(t *testing.T) { db := openTestDB(t) defer db.Close() diff --git a/internal/storage/jobs.go b/internal/storage/jobs.go index b1731416..78dd7582 100644 --- a/internal/storage/jobs.go +++ b/internal/storage/jobs.go @@ -1379,7 +1379,11 @@ func WithExcludeJobType(jobType string) ListJobsOption { // WithRepoPrefix filters jobs to repos whose root_path starts with the given prefix. func WithRepoPrefix(prefix string) ListJobsOption { - return func(o *listJobsOptions) { o.repoPrefix = escapeLike(prefix) } + return func(o *listJobsOptions) { + // Trim trailing slash so LIKE "prefix/%" doesn't become "prefix//%". + // Root prefix "/" trims to "" which disables the filter (all repos match). + o.repoPrefix = escapeLike(strings.TrimRight(prefix, "/")) + } } // escapeLike escapes SQL LIKE wildcards (% and _) in a literal string. diff --git a/internal/storage/repos.go b/internal/storage/repos.go index 0590ed20..801291f3 100644 --- a/internal/storage/repos.go +++ b/internal/storage/repos.go @@ -13,11 +13,13 @@ import ( // GetOrCreateRepo finds or creates a repo by its root path. // If identity is provided, it will be stored; otherwise the identity field remains NULL. func (db *DB) GetOrCreateRepo(rootPath string, identity ...string) (*Repo, error) { - // Normalize path + // Normalize path to forward slashes for consistent storage + // across platforms (LIKE queries use '/' as separator). absPath, err := filepath.Abs(rootPath) if err != nil { return nil, err } + absPath = filepath.ToSlash(absPath) // Extract optional identity var repoIdentity string @@ -92,6 +94,7 @@ func (db *DB) GetRepoByPath(rootPath string) (*Repo, error) { if err != nil { return nil, err } + absPath = filepath.ToSlash(absPath) var repo Repo var createdAt string @@ -121,7 +124,9 @@ type listReposOptions struct { // WithRepoPathPrefix filters repos whose root_path starts with the given prefix. func WithRepoPathPrefix(prefix string) ListReposOption { - return func(o *listReposOptions) { o.prefix = prefix } + return func(o *listReposOptions) { + o.prefix = strings.TrimRight(prefix, "/") + } } // WithRepoBranch filters repos to those having jobs on the given branch.