Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion cmd/roborev/compact.go
Original file line number Diff line number Diff line change
Expand Up @@ -322,13 +322,30 @@ func runCompact(cmd *cobra.Command, opts compactOptions) error {
branchFilter := resolveCurrentBranchFilter(
roots.worktreeRoot, opts.branch, opts.allBranches,
)
explicitBranch := opts.branch != ""

// When the branch was auto-resolved, skip the API-level branch
// filter so jobs from merged branches are included.
apiBranch := branchFilter
if !explicitBranch {
apiBranch = ""
}

// Query and limit jobs, excluding non-review types (compact, task)
// to prevent recursive self-compaction loops
allJobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, branchFilter)
allJobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, apiBranch)
if err != nil {
return err
}
if !opts.allBranches {
filterBranch := ""
if explicitBranch {
filterBranch = branchFilter
}
allJobs = filterReachableJobs(
roots.worktreeRoot, filterBranch, allJobs,
)
}
jobs := filterReviewJobs(allJobs)

if len(jobs) == 0 {
Expand Down
25 changes: 6 additions & 19 deletions cmd/roborev/compact_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -407,11 +407,10 @@ func TestWriteCompactMetadata(t *testing.T) {
}

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

if receivedRepo == "" {
require.Condition(t, func() bool {
return false
}, "expected repo param to be sent")
}
if receivedRepo != repo.Dir {
assert.Condition(t, func() bool {
return false
}, "repo: want main repo %q, got %q",
repo.Dir, receivedRepo)
}
if receivedBranch != "wt-branch" {
assert.Condition(t, func() bool {
return false
}, "branch: want worktree branch %q, got %q",
"wt-branch", receivedBranch)
}
// API query should use the main repo path, not the worktree path.
// Branch filtering is handled client-side by filterReachableJobs.
require.NotEmpty(t, receivedRepo, "expected repo param to be sent")
assert.Equal(t, repo.Dir, receivedRepo,
"repo: want main repo path")
}
62 changes: 50 additions & 12 deletions cmd/roborev/fix.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,10 @@ Examples:
effectiveBranch := resolveCurrentBranchFilter(
roots.worktreeRoot, branch, allBranches,
)
return runFixList(cmd, effectiveBranch, newestFirst)
return runFixList(
cmd, effectiveBranch,
allBranches, branch != "", newestFirst,
)
}
opts := fixOptions{
agentName: agentName,
Expand Down Expand Up @@ -435,7 +438,15 @@ func runFixOpen(cmd *cobra.Command, branch string, allBranches, explicitBranch,
seen := make(map[int64]bool)

for {
jobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, branch)
// When the branch was auto-resolved (not passed via --branch),
// skip the API-level branch filter so jobs from merged branches
// are included. filterReachableJobs handles the actual
// filtering via commit-graph reachability.
apiBranch := branch
if !explicitBranch {
apiBranch = ""
}
jobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, apiBranch)
if err != nil {
return err
}
Expand Down Expand Up @@ -538,13 +549,23 @@ func jobReachable(
// Range ref: check whether the end commit is reachable.
if _, end, ok := git.ParseRange(ref); ok {
reachable, err := git.IsAncestor(worktreeRoot, end, "HEAD")
return err != nil || reachable
if err != nil || reachable {
return true
}
// SHA unreachable (commit may have been rebased) —
// fall back to branch matching.
return branchMatch(matchBranch, j.Branch)
}

// SHA ref: check commit graph reachability.
if looksLikeSHA(ref) {
reachable, err := git.IsAncestor(worktreeRoot, ref, "HEAD")
return err != nil || reachable
if err != nil || reachable {
return true
}
// SHA unreachable (commit may have been rebased) —
// fall back to branch matching.
return branchMatch(matchBranch, j.Branch)
}

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

// runFixList prints open jobs with detailed information without running any agent.
func runFixList(cmd *cobra.Command, branch string, newestFirst bool) error {
func runFixList(
cmd *cobra.Command,
branch string,
allBranches, explicitBranch, newestFirst bool,
) error {
if err := ensureDaemon(); err != nil {
return err
}
Expand All @@ -654,15 +679,24 @@ func runFixList(cmd *cobra.Command, branch string, newestFirst bool) error {
return err
}

jobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, branch)
// When the branch was auto-resolved, skip the API-level branch
// filter so jobs from merged branches are included.
apiBranch := branch
if !explicitBranch {
apiBranch = ""
}
jobs, err := queryOpenJobs(ctx, roots.mainRepoRoot, apiBranch)
if err != nil {
return err
}
// When listing a specific branch, filter by reachability/branch.
// When listing all branches (branch==""), skip filtering — the
// user explicitly asked for everything in this repo.
if branch != "" {
jobs = filterReachableJobs(roots.worktreeRoot, branch, jobs)
if !allBranches {
filterBranch := ""
if explicitBranch {
filterBranch = branch
}
jobs = filterReachableJobs(
roots.worktreeRoot, filterBranch, jobs,
)
}

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

// Discover jobs if none provided
if len(jobIDs) == 0 {
jobs, queryErr := queryOpenJobs(ctx, roots.mainRepoRoot, branch)
apiBranch := branch
if !explicitBranch {
apiBranch = ""
}
jobs, queryErr := queryOpenJobs(ctx, roots.mainRepoRoot, apiBranch)
if queryErr != nil {
return queryErr
}
Expand Down
145 changes: 135 additions & 10 deletions cmd/roborev/fix_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1835,7 +1835,7 @@ func TestRunFixList(t *testing.T) {
// serverAddr is patched by daemonFromHandler called inside Build()

out, err := runWithOutput(t, repo.Dir, func(cmd *cobra.Command) error {
return runFixList(cmd, "", false)
return runFixList(cmd, "", true, false, false)
})
require.NoError(t, err, "runFixList")

Expand Down Expand Up @@ -1863,7 +1863,7 @@ func TestRunFixList(t *testing.T) {
Build()

out, err := runWithOutput(t, repo.Dir, func(cmd *cobra.Command) error {
return runFixList(cmd, "", false)
return runFixList(cmd, "", true, false, false)
})
require.NoError(t, err, "runFixList")

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

gotIDs = nil
_, err := runWithOutput(t, repo.Dir, func(cmd *cobra.Command) error {
return runFixList(cmd, "", true)
return runFixList(cmd, "", true, false, true)
})
require.NoError(t, err, "runFixList: %v")

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

Expand Down Expand Up @@ -2793,18 +2793,32 @@ func TestFilterReachableJobs(t *testing.T) {
wantIDs: []int64{1},
},
{
name: "unreachable commit excluded",
name: "unreachable SHA different branch excluded",
jobs: []storage.ReviewJob{
{ID: 2, GitRef: otherSHA},
{ID: 2, GitRef: otherSHA, Branch: "other-branch"},
},
wantIDs: nil,
},
{
name: "mixed reachable and unreachable",
name: "unreachable SHA same branch included (rebase)",
jobs: []storage.ReviewJob{
{ID: 2, GitRef: otherSHA, Branch: defaultBranch},
},
wantIDs: []int64{2},
},
{
name: "unreachable SHA no branch fails open",
jobs: []storage.ReviewJob{
{ID: 1, GitRef: mainSHA},
{ID: 2, GitRef: otherSHA},
},
wantIDs: []int64{2},
},
{
name: "mixed reachable and unreachable different branch",
jobs: []storage.ReviewJob{
{ID: 1, GitRef: mainSHA},
{ID: 2, GitRef: otherSHA, Branch: "other-branch"},
},
wantIDs: []int64{1},
},
{
Expand Down Expand Up @@ -2857,12 +2871,21 @@ func TestFilterReachableJobs(t *testing.T) {
wantIDs: []int64{5},
},
{
name: "range ref with unreachable end excluded",
name: "range ref unreachable end different branch excluded",
jobs: []storage.ReviewJob{
{ID: 5, GitRef: mainSHA + ".." + otherSHA},
{ID: 5, GitRef: mainSHA + ".." + otherSHA,
Branch: "other-branch"},
},
wantIDs: nil,
},
{
name: "range ref unreachable end same branch included (rebase)",
jobs: []storage.ReviewJob{
{ID: 5, GitRef: mainSHA + ".." + otherSHA,
Branch: defaultBranch},
},
wantIDs: []int64{5},
},
{
name: "range ref with bad end fails open",
jobs: []storage.ReviewJob{
Expand Down Expand Up @@ -3141,3 +3164,105 @@ func TestRunFixOpenFiltersUnreachableJobs(t *testing.T) {
assert.NotContains(t, ids, int64(100),
"main-only job should be filtered out")
}

// TestRunFixOpenFindsMergedBranchJobs verifies that roborev fix on the
// main branch discovers jobs whose commits were originally reviewed on
// a feature branch and later merged to main. The API query must NOT
// filter by branch when the branch was auto-resolved (not --branch),
// so that filterReachableJobs can accept the job via commit-graph
// reachability.
func TestRunFixOpenFindsMergedBranchJobs(t *testing.T) {
repo := newTestGitRepo(t)
repo.CommitFile("base.txt", "base", "initial commit")

defaultBranch := strings.TrimSpace(
repo.Run("rev-parse", "--abbrev-ref", "HEAD"),
)

// Create a feature branch and commit
repo.Run("checkout", "-b", "feature/widget")
featureSHA := repo.CommitFile(
"widget.txt", "code", "add widget",
)

// Switch back to main and merge the feature branch
repo.Run("checkout", defaultBranch)
repo.Run("merge", "--no-ff", "feature/widget", "-m", "merge feature")

var processedJobIDs []int64
var mu sync.Mutex
var apiQueries []string

_ = newMockDaemonBuilder(t).
WithHandler("/api/jobs", func(w http.ResponseWriter, r *http.Request) {
q := r.URL.Query()
apiQueries = append(apiQueries, r.URL.RawQuery)
if q.Get("closed") == "false" && q.Get("limit") == "0" {
// Return job created on the feature branch
writeJSON(w, map[string]any{
"jobs": []storage.ReviewJob{
{
ID: 300,
Status: storage.JobStatusDone,
Agent: "test",
GitRef: featureSHA,
Branch: "feature/widget",
},
},
"has_more": false,
})
} else if q.Get("id") != "" {
var id int64
fmt.Sscanf(q.Get("id"), "%d", &id)
mu.Lock()
processedJobIDs = append(processedJobIDs, id)
mu.Unlock()
writeJSON(w, map[string]any{
"jobs": []storage.ReviewJob{
{
ID: id,
Status: storage.JobStatusDone,
Agent: "test",
},
},
"has_more": false,
})
}
}).
WithHandler("/api/review", func(w http.ResponseWriter, r *http.Request) {
writeJSON(w, storage.Review{Output: "findings"})
}).
WithHandler("/api/comment", func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusCreated)
}).
WithHandler("/api/review/close", func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
}).
WithHandler("/api/enqueue", func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
}).
Build()

// Run from main with auto-resolved branch (explicitBranch=false).
// The feature SHA is reachable from HEAD via the merge commit.
_, runErr := runWithOutput(t, repo.Dir, func(cmd *cobra.Command) error {
return runFixOpen(
cmd, defaultBranch, false, false, false,
fixOptions{agentName: "test", reasoning: "fast"},
)
})
require.NoError(t, runErr, "runFixOpen")

// The API query should NOT include a branch filter
require.NotEmpty(t, apiQueries,
"expected at least one API query")
assert.NotContains(t, apiQueries[0], "branch=",
"auto-resolved branch should not filter API query")

// The merged feature job should be processed
mu.Lock()
ids := processedJobIDs
mu.Unlock()
assert.Contains(t, ids, int64(300),
"merged feature branch job should be found via commit-graph")
}
Loading