diff --git a/cmd/roborev/list.go b/cmd/roborev/list.go index a6763ed6..6f0313ed 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 = filepath.ToSlash(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 == "" || cmd.Flags().Changed("branch")) { params.Set("branch", branch) params.Set("branch_include_empty", "true") } 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/cmd/roborev/review.go b/cmd/roborev/review.go index cb52766b..96065261 100644 --- a/cmd/roborev/review.go +++ b/cmd/roborev/review.go @@ -7,6 +7,9 @@ import ( "fmt" "io" "net/http" + "os" + "path/filepath" + "strings" "github.com/roborev-dev/roborev/internal/agent" "github.com/roborev-dev/roborev/internal/config" @@ -80,6 +83,17 @@ 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 { + absDir, _ := filepath.Abs(repoPath) + var b strings.Builder + b.WriteString("not in a git repository; use --repo to specify one:") + for _, name := range children { + b.WriteString("\n roborev review --repo ") + b.WriteString(filepath.Join(absDir, name)) + } + return fmt.Errorf("%s", b.String()) + } return fmt.Errorf("not a git repository: %w", err) } @@ -411,3 +425,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 _, err := os.Stat(gitDir); err == nil { + repos = append(repos, e.Name()) + } + } + return repos +} 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, + ) + } +} 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/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 6ab18457..632714bf 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" @@ -815,6 +816,10 @@ 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") + if repoPrefix != "" { + repoPrefix = filepath.ToSlash(filepath.Clean(repoPrefix)) + } // Parse limit from query, default to 50, 0 means no limit // Clamp to valid range: 0 (unlimited) or 1-10000 @@ -870,6 +875,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 +901,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) @@ -911,18 +922,20 @@ func (s *Server) handleListRepos(w http.ResponseWriter, r *http.Request) { return } - // Optional branch filter branch := r.URL.Query().Get("branch") + prefix := r.URL.Query().Get("prefix") + if prefix != "" { + prefix = filepath.ToSlash(filepath.Clean(prefix)) + } - var repos []storage.RepoWithCount - var totalCount int - var err error - + var repoOpts []storage.ListReposOption + if prefix != "" { + repoOpts = append(repoOpts, storage.WithRepoPathPrefix(prefix)) + } if branch != "" { - repos, totalCount, err = s.db.ListReposWithReviewCountsByBranch(branch) - } else { - repos, totalCount, err = s.db.ListReposWithReviewCounts() + 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 a2ad0bbb..85bfce17 100644 --- a/internal/daemon/server_jobs_test.go +++ b/internal/daemon/server_jobs_test.go @@ -1301,3 +1301,143 @@ 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)) + } + }) + + 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)) + } + }) +} + +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), + ) + } + 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 19b86bce..8742be35 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,169 @@ 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)) + } + }) + + // 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)) + } + }) + + 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 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 repoEntry struct { + Name string `json:"name"` + RootPath string `json:"root_path"` + } + type reposResponse struct { + Repos []repoEntry `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) + } + 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") + } + }) +} + func TestHandleListBranches(t *testing.T) { server, db, tmpDir := newTestServer(t) diff --git a/internal/storage/db_filter_test.go b/internal/storage/db_filter_test.go index cc5f6552..a8b2fc2f 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)) @@ -917,3 +917,269 @@ 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)) + } + }) + + // 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"), + ) + if err != nil { + t.Fatalf("CountJobStats failed: %v", err) + } + // 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) + } + }) + + 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 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() + + // 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/jobs.go b/internal/storage/jobs.go index 70b8321d..78dd7582 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,25 @@ 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) { + // 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. +// 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, "_", "!_") + 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 +1424,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 +1595,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..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 @@ -111,60 +114,83 @@ 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 - } - defer rows.Close() +// ListReposOption configures filtering for ListReposWithReviewCounts. +type ListReposOption func(*listReposOptions) - 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 +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 = strings.TrimRight(prefix, "/") } - 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 +// 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 } +} - if branch == "" { - // No filter - return all repos - return db.ListReposWithReviewCounts() +// 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) } - // Filter by branch (handle "(none)" as NULL/empty branch) - branchFilter := branch - if branch == "(none)" { - branchFilter = "" + // 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" } - rows, err = db.Query(` + query := fmt.Sprintf(` 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, '') = ? + %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)) + } + + if o.branch != "" { + branchFilter := o.branch + if o.branch == "(none)" { + branchFilter = "" + } + conditions = append( + conditions, "COALESCE(rj.branch, '') = ?", + ) + args = append(args, branchFilter) + } + + if len(conditions) > 0 { + query += " WHERE " + strings.Join(conditions, " AND ") + } + + query += ` GROUP BY r.id, r.name, r.root_path - HAVING job_count > 0 - ORDER BY r.name - `, branchFilter) + ` + if o.branch != "" { + query += " HAVING job_count > 0" + } + query += " ORDER BY r.name" + + rows, err := db.Query(query, args...) if err != nil { return nil, 0, err }