Skip to content

Commit 45987ff

Browse files
wesmclaude
andauthored
fix: find open jobs when running fix from main branch (#624)
## Summary - **Skip API-level branch filter when branch is auto-resolved.** `queryOpenJobs` was sending `branch=main&branch_include_empty=true` to the server, which excluded jobs originally created on feature branches whose commits were later merged to main. When no `--branch` flag is passed, the API query now omits the branch filter and relies on `filterReachableJobs` for commit-graph reachability. Applied to `runFixOpen`, `runFixList`, `runFixBatch`, and `runCompact`. - **Fall back to branch matching when a SHA is unreachable.** `filterReachableJobs` used `git merge-base --is-ancestor` to check whether a job's commit SHA is reachable from HEAD. When a commit has been amended, squashed, or rebased, the original SHA is no longer an ancestor of HEAD — but the dangling object still exists, so git returns exit code 1 (not ancestor) rather than 128 (unknown object). This caused `filterReachableJobs` to silently drop the job. Now when a SHA is unreachable, it falls back to branch matching: if the job's branch matches the current branch, the job is included. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 4c7ad82 commit 45987ff

4 files changed

Lines changed: 209 additions & 42 deletions

File tree

cmd/roborev/compact.go

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -322,13 +322,30 @@ func runCompact(cmd *cobra.Command, opts compactOptions) error {
322322
branchFilter := resolveCurrentBranchFilter(
323323
roots.worktreeRoot, opts.branch, opts.allBranches,
324324
)
325+
explicitBranch := opts.branch != ""
326+
327+
// When the branch was auto-resolved, skip the API-level branch
328+
// filter so jobs from merged branches are included.
329+
apiBranch := branchFilter
330+
if !explicitBranch {
331+
apiBranch = ""
332+
}
325333

326334
// Query and limit jobs, excluding non-review types (compact, task)
327335
// to prevent recursive self-compaction loops
328-
allJobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, branchFilter)
336+
allJobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, apiBranch)
329337
if err != nil {
330338
return err
331339
}
340+
if !opts.allBranches {
341+
filterBranch := ""
342+
if explicitBranch {
343+
filterBranch = branchFilter
344+
}
345+
allJobs = filterReachableJobs(
346+
roots.worktreeRoot, filterBranch, allJobs,
347+
)
348+
}
332349
jobs := filterReviewJobs(allJobs)
333350

334351
if len(jobs) == 0 {

cmd/roborev/compact_test.go

Lines changed: 6 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -407,11 +407,10 @@ func TestWriteCompactMetadata(t *testing.T) {
407407
}
408408

409409
func TestCompactWorktreeBranchResolution(t *testing.T) {
410-
var receivedRepo, receivedBranch string
410+
var receivedRepo string
411411
_ = newMockDaemonBuilder(t).
412412
WithHandler("/api/jobs", func(w http.ResponseWriter, r *http.Request) {
413413
receivedRepo = r.URL.Query().Get("repo")
414-
receivedBranch = r.URL.Query().Get("branch")
415414
writeJSON(w, map[string]any{
416415
"jobs": []storage.ReviewJob{},
417416
"has_more": false,
@@ -429,21 +428,9 @@ func TestCompactWorktreeBranchResolution(t *testing.T) {
429428
opts := compactOptions{quiet: true}
430429
_ = runCompact(cmd, opts)
431430

432-
if receivedRepo == "" {
433-
require.Condition(t, func() bool {
434-
return false
435-
}, "expected repo param to be sent")
436-
}
437-
if receivedRepo != repo.Dir {
438-
assert.Condition(t, func() bool {
439-
return false
440-
}, "repo: want main repo %q, got %q",
441-
repo.Dir, receivedRepo)
442-
}
443-
if receivedBranch != "wt-branch" {
444-
assert.Condition(t, func() bool {
445-
return false
446-
}, "branch: want worktree branch %q, got %q",
447-
"wt-branch", receivedBranch)
448-
}
431+
// API query should use the main repo path, not the worktree path.
432+
// Branch filtering is handled client-side by filterReachableJobs.
433+
require.NotEmpty(t, receivedRepo, "expected repo param to be sent")
434+
assert.Equal(t, repo.Dir, receivedRepo,
435+
"repo: want main repo path")
449436
}

cmd/roborev/fix.go

Lines changed: 50 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,10 @@ Examples:
115115
effectiveBranch := resolveCurrentBranchFilter(
116116
roots.worktreeRoot, branch, allBranches,
117117
)
118-
return runFixList(cmd, effectiveBranch, newestFirst)
118+
return runFixList(
119+
cmd, effectiveBranch,
120+
allBranches, branch != "", newestFirst,
121+
)
119122
}
120123
opts := fixOptions{
121124
agentName: agentName,
@@ -435,7 +438,15 @@ func runFixOpen(cmd *cobra.Command, branch string, allBranches, explicitBranch,
435438
seen := make(map[int64]bool)
436439

437440
for {
438-
jobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, branch)
441+
// When the branch was auto-resolved (not passed via --branch),
442+
// skip the API-level branch filter so jobs from merged branches
443+
// are included. filterReachableJobs handles the actual
444+
// filtering via commit-graph reachability.
445+
apiBranch := branch
446+
if !explicitBranch {
447+
apiBranch = ""
448+
}
449+
jobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, apiBranch)
439450
if err != nil {
440451
return err
441452
}
@@ -538,13 +549,23 @@ func jobReachable(
538549
// Range ref: check whether the end commit is reachable.
539550
if _, end, ok := git.ParseRange(ref); ok {
540551
reachable, err := git.IsAncestor(worktreeRoot, end, "HEAD")
541-
return err != nil || reachable
552+
if err != nil || reachable {
553+
return true
554+
}
555+
// SHA unreachable (commit may have been rebased) —
556+
// fall back to branch matching.
557+
return branchMatch(matchBranch, j.Branch)
542558
}
543559

544560
// SHA ref: check commit graph reachability.
545561
if looksLikeSHA(ref) {
546562
reachable, err := git.IsAncestor(worktreeRoot, ref, "HEAD")
547-
return err != nil || reachable
563+
if err != nil || reachable {
564+
return true
565+
}
566+
// SHA unreachable (commit may have been rebased) —
567+
// fall back to branch matching.
568+
return branchMatch(matchBranch, j.Branch)
548569
}
549570

550571
// Non-SHA ref (empty, "dirty", task labels like "run"/"analyze"):
@@ -640,7 +661,11 @@ func queryOpenJobIDs(
640661
}
641662

642663
// runFixList prints open jobs with detailed information without running any agent.
643-
func runFixList(cmd *cobra.Command, branch string, newestFirst bool) error {
664+
func runFixList(
665+
cmd *cobra.Command,
666+
branch string,
667+
allBranches, explicitBranch, newestFirst bool,
668+
) error {
644669
if err := ensureDaemon(); err != nil {
645670
return err
646671
}
@@ -654,15 +679,24 @@ func runFixList(cmd *cobra.Command, branch string, newestFirst bool) error {
654679
return err
655680
}
656681

657-
jobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, branch)
682+
// When the branch was auto-resolved, skip the API-level branch
683+
// filter so jobs from merged branches are included.
684+
apiBranch := branch
685+
if !explicitBranch {
686+
apiBranch = ""
687+
}
688+
jobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, apiBranch)
658689
if err != nil {
659690
return err
660691
}
661-
// When listing a specific branch, filter by reachability/branch.
662-
// When listing all branches (branch==""), skip filtering — the
663-
// user explicitly asked for everything in this repo.
664-
if branch != "" {
665-
jobs = filterReachableJobs(roots.worktreeRoot, branch, jobs)
692+
if !allBranches {
693+
filterBranch := ""
694+
if explicitBranch {
695+
filterBranch = branch
696+
}
697+
jobs = filterReachableJobs(
698+
roots.worktreeRoot, filterBranch, jobs,
699+
)
666700
}
667701

668702
jobIDs := make([]int64, len(jobs))
@@ -941,7 +975,11 @@ func runFixBatch(cmd *cobra.Command, jobIDs []int64, branch string, allBranches,
941975

942976
// Discover jobs if none provided
943977
if len(jobIDs) == 0 {
944-
jobs, queryErr := queryOpenJobs(ctx, roots.mainRepoRoot, branch)
978+
apiBranch := branch
979+
if !explicitBranch {
980+
apiBranch = ""
981+
}
982+
jobs, queryErr := queryOpenJobs(ctx, roots.mainRepoRoot, apiBranch)
945983
if queryErr != nil {
946984
return queryErr
947985
}

cmd/roborev/fix_test.go

Lines changed: 135 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1835,7 +1835,7 @@ func TestRunFixList(t *testing.T) {
18351835
// serverAddr is patched by daemonFromHandler called inside Build()
18361836

18371837
out, err := runWithOutput(t, repo.Dir, func(cmd *cobra.Command) error {
1838-
return runFixList(cmd, "", false)
1838+
return runFixList(cmd, "", true, false, false)
18391839
})
18401840
require.NoError(t, err, "runFixList")
18411841

@@ -1863,7 +1863,7 @@ func TestRunFixList(t *testing.T) {
18631863
Build()
18641864

18651865
out, err := runWithOutput(t, repo.Dir, func(cmd *cobra.Command) error {
1866-
return runFixList(cmd, "", false)
1866+
return runFixList(cmd, "", true, false, false)
18671867
})
18681868
require.NoError(t, err, "runFixList")
18691869

@@ -1904,7 +1904,7 @@ func TestRunFixList(t *testing.T) {
19041904

19051905
gotIDs = nil
19061906
_, err := runWithOutput(t, repo.Dir, func(cmd *cobra.Command) error {
1907-
return runFixList(cmd, "", true)
1907+
return runFixList(cmd, "", true, false, true)
19081908
})
19091909
require.NoError(t, err, "runFixList: %v")
19101910

@@ -1981,7 +1981,7 @@ func TestFixWorktreeRepoResolution(t *testing.T) {
19811981
cmd := &cobra.Command{}
19821982
var buf bytes.Buffer
19831983
cmd.SetOut(&buf)
1984-
if err := runFixList(cmd, "", false); err != nil {
1984+
if err := runFixList(cmd, "", true, false, false); err != nil {
19851985
require.NoError(t, err, "runFixList: %v")
19861986
}
19871987

@@ -2793,18 +2793,32 @@ func TestFilterReachableJobs(t *testing.T) {
27932793
wantIDs: []int64{1},
27942794
},
27952795
{
2796-
name: "unreachable commit excluded",
2796+
name: "unreachable SHA different branch excluded",
27972797
jobs: []storage.ReviewJob{
2798-
{ID: 2, GitRef: otherSHA},
2798+
{ID: 2, GitRef: otherSHA, Branch: "other-branch"},
27992799
},
28002800
wantIDs: nil,
28012801
},
28022802
{
2803-
name: "mixed reachable and unreachable",
2803+
name: "unreachable SHA same branch included (rebase)",
2804+
jobs: []storage.ReviewJob{
2805+
{ID: 2, GitRef: otherSHA, Branch: defaultBranch},
2806+
},
2807+
wantIDs: []int64{2},
2808+
},
2809+
{
2810+
name: "unreachable SHA no branch fails open",
28042811
jobs: []storage.ReviewJob{
2805-
{ID: 1, GitRef: mainSHA},
28062812
{ID: 2, GitRef: otherSHA},
28072813
},
2814+
wantIDs: []int64{2},
2815+
},
2816+
{
2817+
name: "mixed reachable and unreachable different branch",
2818+
jobs: []storage.ReviewJob{
2819+
{ID: 1, GitRef: mainSHA},
2820+
{ID: 2, GitRef: otherSHA, Branch: "other-branch"},
2821+
},
28082822
wantIDs: []int64{1},
28092823
},
28102824
{
@@ -2857,12 +2871,21 @@ func TestFilterReachableJobs(t *testing.T) {
28572871
wantIDs: []int64{5},
28582872
},
28592873
{
2860-
name: "range ref with unreachable end excluded",
2874+
name: "range ref unreachable end different branch excluded",
28612875
jobs: []storage.ReviewJob{
2862-
{ID: 5, GitRef: mainSHA + ".." + otherSHA},
2876+
{ID: 5, GitRef: mainSHA + ".." + otherSHA,
2877+
Branch: "other-branch"},
28632878
},
28642879
wantIDs: nil,
28652880
},
2881+
{
2882+
name: "range ref unreachable end same branch included (rebase)",
2883+
jobs: []storage.ReviewJob{
2884+
{ID: 5, GitRef: mainSHA + ".." + otherSHA,
2885+
Branch: defaultBranch},
2886+
},
2887+
wantIDs: []int64{5},
2888+
},
28662889
{
28672890
name: "range ref with bad end fails open",
28682891
jobs: []storage.ReviewJob{
@@ -3141,3 +3164,105 @@ func TestRunFixOpenFiltersUnreachableJobs(t *testing.T) {
31413164
assert.NotContains(t, ids, int64(100),
31423165
"main-only job should be filtered out")
31433166
}
3167+
3168+
// TestRunFixOpenFindsMergedBranchJobs verifies that roborev fix on the
3169+
// main branch discovers jobs whose commits were originally reviewed on
3170+
// a feature branch and later merged to main. The API query must NOT
3171+
// filter by branch when the branch was auto-resolved (not --branch),
3172+
// so that filterReachableJobs can accept the job via commit-graph
3173+
// reachability.
3174+
func TestRunFixOpenFindsMergedBranchJobs(t *testing.T) {
3175+
repo := newTestGitRepo(t)
3176+
repo.CommitFile("base.txt", "base", "initial commit")
3177+
3178+
defaultBranch := strings.TrimSpace(
3179+
repo.Run("rev-parse", "--abbrev-ref", "HEAD"),
3180+
)
3181+
3182+
// Create a feature branch and commit
3183+
repo.Run("checkout", "-b", "feature/widget")
3184+
featureSHA := repo.CommitFile(
3185+
"widget.txt", "code", "add widget",
3186+
)
3187+
3188+
// Switch back to main and merge the feature branch
3189+
repo.Run("checkout", defaultBranch)
3190+
repo.Run("merge", "--no-ff", "feature/widget", "-m", "merge feature")
3191+
3192+
var processedJobIDs []int64
3193+
var mu sync.Mutex
3194+
var apiQueries []string
3195+
3196+
_ = newMockDaemonBuilder(t).
3197+
WithHandler("/api/jobs", func(w http.ResponseWriter, r *http.Request) {
3198+
q := r.URL.Query()
3199+
apiQueries = append(apiQueries, r.URL.RawQuery)
3200+
if q.Get("closed") == "false" && q.Get("limit") == "0" {
3201+
// Return job created on the feature branch
3202+
writeJSON(w, map[string]any{
3203+
"jobs": []storage.ReviewJob{
3204+
{
3205+
ID: 300,
3206+
Status: storage.JobStatusDone,
3207+
Agent: "test",
3208+
GitRef: featureSHA,
3209+
Branch: "feature/widget",
3210+
},
3211+
},
3212+
"has_more": false,
3213+
})
3214+
} else if q.Get("id") != "" {
3215+
var id int64
3216+
fmt.Sscanf(q.Get("id"), "%d", &id)
3217+
mu.Lock()
3218+
processedJobIDs = append(processedJobIDs, id)
3219+
mu.Unlock()
3220+
writeJSON(w, map[string]any{
3221+
"jobs": []storage.ReviewJob{
3222+
{
3223+
ID: id,
3224+
Status: storage.JobStatusDone,
3225+
Agent: "test",
3226+
},
3227+
},
3228+
"has_more": false,
3229+
})
3230+
}
3231+
}).
3232+
WithHandler("/api/review", func(w http.ResponseWriter, r *http.Request) {
3233+
writeJSON(w, storage.Review{Output: "findings"})
3234+
}).
3235+
WithHandler("/api/comment", func(w http.ResponseWriter, r *http.Request) {
3236+
w.WriteHeader(http.StatusCreated)
3237+
}).
3238+
WithHandler("/api/review/close", func(w http.ResponseWriter, r *http.Request) {
3239+
w.WriteHeader(http.StatusOK)
3240+
}).
3241+
WithHandler("/api/enqueue", func(w http.ResponseWriter, r *http.Request) {
3242+
w.WriteHeader(http.StatusOK)
3243+
}).
3244+
Build()
3245+
3246+
// Run from main with auto-resolved branch (explicitBranch=false).
3247+
// The feature SHA is reachable from HEAD via the merge commit.
3248+
_, runErr := runWithOutput(t, repo.Dir, func(cmd *cobra.Command) error {
3249+
return runFixOpen(
3250+
cmd, defaultBranch, false, false, false,
3251+
fixOptions{agentName: "test", reasoning: "fast"},
3252+
)
3253+
})
3254+
require.NoError(t, runErr, "runFixOpen")
3255+
3256+
// The API query should NOT include a branch filter
3257+
require.NotEmpty(t, apiQueries,
3258+
"expected at least one API query")
3259+
assert.NotContains(t, apiQueries[0], "branch=",
3260+
"auto-resolved branch should not filter API query")
3261+
3262+
// The merged feature job should be processed
3263+
mu.Lock()
3264+
ids := processedJobIDs
3265+
mu.Unlock()
3266+
assert.Contains(t, ids, int64(300),
3267+
"merged feature branch job should be found via commit-graph")
3268+
}

0 commit comments

Comments
 (0)