Skip to content

Commit 1e9e8fc

Browse files
wesmclaude
andauthored
Fix list and fix returning no results from git worktrees (#249)
Closes #248 ## Summary - **Worktree path mismatch**: CLI commands (`list`, `fix`) used `GetRepoRoot` (`--show-toplevel`) which returns the worktree path, but the daemon stores jobs under `GetMainRepoRoot`. The `repo=` API filter never matched. Now all API query paths resolve through `GetMainRepoRoot`, while local git operations (branch detection, agent working directory) still use the worktree-local path. - **Null branch fallback**: `handleEnqueue` detected `currentBranch` for exclusion checks but never used it as a fallback when `req.Branch` was empty. Jobs from older clients or edge cases were stored with NULL branch, making them invisible to branch-filtered queries. Now the daemon sets `req.Branch = currentBranch` when the client omits it. - **Explicit `--repo` normalization**: `list --repo <worktree-path>` now normalizes the path to the main repo root, so it matches regardless of which checkout the user points to. ## Test plan - [x] `TestListCommand/worktree_sends_main_repo_path_as_repo_param` — list from worktree sends main repo path - [x] `TestListCommand/explicit_--repo_with_worktree_path_normalizes_to_main_repo` — explicit --repo with worktree path - [x] `TestFixWorktreeRepoResolution/runFixList_sends_main_repo_path` — fix --list from worktree - [x] `TestFixWorktreeRepoResolution/runFixUnaddressed_sends_main_repo_path` — fix --unaddressed from worktree - [x] `TestFixWorktreeRepoResolution/runFixBatch_sends_main_repo_path` — fix --batch from worktree - [x] `TestHandleEnqueueBranchFallback` — daemon fills in branch when client omits it - [x] Full test suite passes (`go test ./...`) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 37bb1c0 commit 1e9e8fc

File tree

6 files changed

+260
-6
lines changed

6 files changed

+260
-6
lines changed

cmd/roborev/fix.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,7 @@ func runFixUnaddressed(cmd *cobra.Command, branch string, newestFirst bool, opts
386386
}
387387

388388
repoRoot := workDir
389-
if root, err := git.GetRepoRoot(workDir); err == nil {
389+
if root, err := git.GetMainRepoRoot(workDir); err == nil {
390390
repoRoot = root
391391
}
392392

@@ -477,7 +477,7 @@ func runFixList(cmd *cobra.Command, branch string, newestFirst bool) error {
477477
return fmt.Errorf("get working directory: %w", err)
478478
}
479479
repoRoot := workDir
480-
if root, err := git.GetRepoRoot(workDir); err == nil {
480+
if root, err := git.GetMainRepoRoot(workDir); err == nil {
481481
repoRoot = root
482482
}
483483

@@ -707,10 +707,15 @@ func runFixBatch(cmd *cobra.Command, jobIDs []int64, branch string, newestFirst
707707
if root, err := git.GetRepoRoot(workDir); err == nil {
708708
repoRoot = root
709709
}
710+
// Use main repo root for API queries (daemon stores jobs under main repo path)
711+
apiRepoRoot := repoRoot
712+
if root, err := git.GetMainRepoRoot(workDir); err == nil {
713+
apiRepoRoot = root
714+
}
710715

711716
// Discover jobs if none provided
712717
if len(jobIDs) == 0 {
713-
jobIDs, err = queryUnaddressedJobs(repoRoot, branch)
718+
jobIDs, err = queryUnaddressedJobs(apiRepoRoot, branch)
714719
if err != nil {
715720
return err
716721
}

cmd/roborev/fix_test.go

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1323,3 +1323,100 @@ func TestFixListFlagValidation(t *testing.T) {
13231323
})
13241324
}
13251325
}
1326+
1327+
// setupWorktree creates a main repo with a commit and a worktree, returning
1328+
// the main repo and the worktree directory path.
1329+
func setupWorktree(t *testing.T) (mainRepo *TestGitRepo, worktreeDir string) {
1330+
t.Helper()
1331+
repo := newTestGitRepo(t)
1332+
repo.CommitFile("file.txt", "content", "initial")
1333+
1334+
wtDir := t.TempDir()
1335+
os.Remove(wtDir)
1336+
repo.Run("worktree", "add", "-b", "wt-branch", wtDir)
1337+
return repo, wtDir
1338+
}
1339+
1340+
// setupWorktreeMockDaemon sets up a mock daemon that captures the repo query
1341+
// param from /api/jobs requests, returning empty results.
1342+
func setupWorktreeMockDaemon(t *testing.T) (receivedRepo *string) {
1343+
t.Helper()
1344+
var repo string
1345+
_, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1346+
if r.URL.Path == "/api/jobs" {
1347+
repo = r.URL.Query().Get("repo")
1348+
json.NewEncoder(w).Encode(map[string]interface{}{
1349+
"jobs": []storage.ReviewJob{},
1350+
"has_more": false,
1351+
})
1352+
return
1353+
}
1354+
}))
1355+
t.Cleanup(cleanup)
1356+
return &repo
1357+
}
1358+
1359+
func TestFixWorktreeRepoResolution(t *testing.T) {
1360+
t.Run("runFixList sends main repo path", func(t *testing.T) {
1361+
receivedRepo := setupWorktreeMockDaemon(t)
1362+
repo, worktreeDir := setupWorktree(t)
1363+
chdir(t, worktreeDir)
1364+
1365+
cmd := &cobra.Command{}
1366+
var buf bytes.Buffer
1367+
cmd.SetOut(&buf)
1368+
if err := runFixList(cmd, "", false); err != nil {
1369+
t.Fatalf("runFixList: %v", err)
1370+
}
1371+
1372+
if *receivedRepo == "" {
1373+
t.Fatal("expected repo param to be sent")
1374+
}
1375+
if *receivedRepo != repo.Dir {
1376+
t.Errorf("expected main repo path %q, got %q", repo.Dir, *receivedRepo)
1377+
}
1378+
})
1379+
1380+
t.Run("runFixUnaddressed sends main repo path", func(t *testing.T) {
1381+
receivedRepo := setupWorktreeMockDaemon(t)
1382+
repo, worktreeDir := setupWorktree(t)
1383+
chdir(t, worktreeDir)
1384+
1385+
cmd := &cobra.Command{}
1386+
var buf bytes.Buffer
1387+
cmd.SetOut(&buf)
1388+
opts := fixOptions{quiet: true}
1389+
if err := runFixUnaddressed(cmd, "", false, opts); err != nil {
1390+
t.Fatalf("runFixUnaddressed: %v", err)
1391+
}
1392+
1393+
if *receivedRepo == "" {
1394+
t.Fatal("expected repo param to be sent")
1395+
}
1396+
if *receivedRepo != repo.Dir {
1397+
t.Errorf("expected main repo path %q, got %q", repo.Dir, *receivedRepo)
1398+
}
1399+
})
1400+
1401+
t.Run("runFixBatch sends main repo path", func(t *testing.T) {
1402+
receivedRepo := setupWorktreeMockDaemon(t)
1403+
repo, worktreeDir := setupWorktree(t)
1404+
chdir(t, worktreeDir)
1405+
1406+
cmd := &cobra.Command{}
1407+
var buf bytes.Buffer
1408+
cmd.SetOut(&buf)
1409+
opts := fixOptions{quiet: true}
1410+
// nil jobIDs triggers discovery via queryUnaddressedJobs
1411+
if err := runFixBatch(cmd, nil, "", false, opts); err != nil {
1412+
t.Fatalf("runFixBatch: %v", err)
1413+
}
1414+
1415+
if *receivedRepo == "" {
1416+
t.Fatal("expected repo param to be sent")
1417+
}
1418+
if *receivedRepo != repo.Dir {
1419+
t.Errorf("expected main repo path %q, got %q", repo.Dir, *receivedRepo)
1420+
}
1421+
})
1422+
}

cmd/roborev/list_test.go

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@ package main
33
import (
44
"encoding/json"
55
"net/http"
6+
"net/url"
7+
"os"
68
"strings"
79
"testing"
810
"time"
@@ -311,6 +313,96 @@ func TestListCommand(t *testing.T) {
311313
t.Errorf("expected branch param when --repo is a valid git repo, got: %s", receivedQuery)
312314
}
313315
})
316+
317+
t.Run("worktree sends main repo path as repo param", func(t *testing.T) {
318+
var receivedQuery string
319+
_, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
320+
if r.URL.Path == "/api/jobs" {
321+
receivedQuery = r.URL.RawQuery
322+
json.NewEncoder(w).Encode(map[string]interface{}{
323+
"jobs": []storage.ReviewJob{},
324+
"has_more": false,
325+
})
326+
return
327+
}
328+
}))
329+
t.Cleanup(cleanup)
330+
331+
// Create main repo with initial commit
332+
repo := newTestGitRepo(t)
333+
repo.CommitFile("file.txt", "content", "initial")
334+
335+
// Create a worktree
336+
worktreeDir := t.TempDir()
337+
// Remove the dir so git worktree add can create it
338+
os.Remove(worktreeDir)
339+
repo.Run("worktree", "add", "-b", "wt-branch", worktreeDir)
340+
341+
chdir(t, worktreeDir)
342+
343+
captureStdout(t, func() {
344+
cmd := listCmd()
345+
cmd.SetArgs([]string{})
346+
if err := cmd.Execute(); err != nil {
347+
t.Fatalf("unexpected error: %v", err)
348+
}
349+
})
350+
351+
// The repo= param should be the main repo path, not the worktree path
352+
if strings.Contains(receivedQuery, url.QueryEscape(worktreeDir)) {
353+
t.Errorf("expected main repo path in query, but got worktree path: %s", receivedQuery)
354+
}
355+
if !strings.Contains(receivedQuery, url.QueryEscape(repo.Dir)) {
356+
t.Errorf("expected main repo path %q in query, got: %s", repo.Dir, receivedQuery)
357+
}
358+
// Branch should be the worktree's branch, not the main repo's
359+
if !strings.Contains(receivedQuery, "branch=wt-branch") {
360+
t.Errorf("expected branch=wt-branch in query, got: %s", receivedQuery)
361+
}
362+
})
363+
364+
t.Run("explicit --repo with worktree path normalizes to main repo", func(t *testing.T) {
365+
var receivedQuery string
366+
_, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
367+
if r.URL.Path == "/api/jobs" {
368+
receivedQuery = r.URL.RawQuery
369+
json.NewEncoder(w).Encode(map[string]interface{}{
370+
"jobs": []storage.ReviewJob{},
371+
"has_more": false,
372+
})
373+
return
374+
}
375+
}))
376+
t.Cleanup(cleanup)
377+
378+
// Create main repo with initial commit
379+
repo := newTestGitRepo(t)
380+
repo.CommitFile("file.txt", "content", "initial")
381+
382+
// Create a worktree
383+
worktreeDir := t.TempDir()
384+
os.Remove(worktreeDir)
385+
repo.Run("worktree", "add", "-b", "wt-branch", worktreeDir)
386+
387+
// Don't chdir — pass the worktree path via --repo explicitly
388+
chdir(t, repo.Dir)
389+
390+
captureStdout(t, func() {
391+
cmd := listCmd()
392+
cmd.SetArgs([]string{"--repo", worktreeDir})
393+
if err := cmd.Execute(); err != nil {
394+
t.Fatalf("unexpected error: %v", err)
395+
}
396+
})
397+
398+
// Even with --repo pointing to the worktree, repo= should be the main repo
399+
if strings.Contains(receivedQuery, url.QueryEscape(worktreeDir)) {
400+
t.Errorf("expected main repo path in query, but got worktree path: %s", receivedQuery)
401+
}
402+
if !strings.Contains(receivedQuery, url.QueryEscape(repo.Dir)) {
403+
t.Errorf("expected main repo path %q in query, got: %s", repo.Dir, receivedQuery)
404+
}
405+
})
314406
}
315407

316408
func TestShowJSONOutput(t *testing.T) {

cmd/roborev/main.go

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1457,14 +1457,28 @@ Examples:
14571457
addr := getDaemonAddr()
14581458

14591459
// Auto-resolve repo from cwd when not specified.
1460-
if repoPath == "" {
1460+
// Use worktree root for branch detection, main repo root for API queries
1461+
// (daemon stores jobs under the main repo path).
1462+
localRepoPath := repoPath
1463+
if localRepoPath == "" {
14611464
if root, err := git.GetRepoRoot("."); err == nil {
1465+
localRepoPath = root
1466+
}
1467+
}
1468+
if repoPath == "" {
1469+
if root, err := git.GetMainRepoRoot("."); err == nil {
1470+
repoPath = root
1471+
}
1472+
} else {
1473+
// Normalize explicit --repo to main repo root so worktree
1474+
// paths match the daemon's stored repo path.
1475+
if root, err := git.GetMainRepoRoot(repoPath); err == nil {
14621476
repoPath = root
14631477
}
14641478
}
14651479
// Auto-resolve branch from the target repo when not specified.
1466-
if branch == "" && repoPath != "" {
1467-
branch = git.GetCurrentBranch(repoPath)
1480+
if branch == "" && localRepoPath != "" {
1481+
branch = git.GetCurrentBranch(localRepoPath)
14681482
}
14691483

14701484
// Build query URL

internal/daemon/server.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,11 @@ func (s *Server) handleEnqueue(w http.ResponseWriter, r *http.Request) {
468468
return
469469
}
470470

471+
// Fall back to detected branch when client didn't send one
472+
if req.Branch == "" {
473+
req.Branch = currentBranch
474+
}
475+
471476
// Resolve repo identity for sync
472477
repoIdentity := config.ResolveRepoIdentity(repoRoot, nil)
473478

internal/daemon/server_test.go

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1903,6 +1903,47 @@ func TestHandleEnqueueExcludedBranch(t *testing.T) {
19031903
})
19041904
}
19051905

1906+
func TestHandleEnqueueBranchFallback(t *testing.T) {
1907+
server, db, tmpDir := newTestServer(t)
1908+
1909+
repoDir := filepath.Join(tmpDir, "testrepo")
1910+
testutil.InitTestGitRepo(t, repoDir)
1911+
1912+
// Switch to a named branch
1913+
branchCmd := exec.Command("git", "-C", repoDir, "checkout", "-b", "my-feature")
1914+
if out, err := branchCmd.CombinedOutput(); err != nil {
1915+
t.Fatalf("git checkout failed: %v\n%s", err, out)
1916+
}
1917+
1918+
// Enqueue with empty branch field
1919+
reqData := map[string]string{
1920+
"repo_path": repoDir,
1921+
"git_ref": "HEAD",
1922+
"agent": "test",
1923+
}
1924+
req := testutil.MakeJSONRequest(t, http.MethodPost, "/api/enqueue", reqData)
1925+
w := httptest.NewRecorder()
1926+
server.handleEnqueue(w, req)
1927+
1928+
if w.Code != http.StatusCreated {
1929+
t.Fatalf("expected 201, got %d: %s", w.Code, w.Body.String())
1930+
}
1931+
1932+
var respJob storage.ReviewJob
1933+
if err := json.NewDecoder(w.Body).Decode(&respJob); err != nil {
1934+
t.Fatalf("decode response: %v", err)
1935+
}
1936+
1937+
// Verify the job has the detected branch, not empty
1938+
job, err := db.GetJobByID(respJob.ID)
1939+
if err != nil {
1940+
t.Fatalf("GetJob: %v", err)
1941+
}
1942+
if job.Branch != "my-feature" {
1943+
t.Errorf("expected branch %q, got %q", "my-feature", job.Branch)
1944+
}
1945+
}
1946+
19061947
func TestHandleEnqueueBodySizeLimit(t *testing.T) {
19071948
server, _, tmpDir := newTestServer(t)
19081949

0 commit comments

Comments
 (0)