Skip to content

Commit 46c6e4b

Browse files
wesmclaude
andauthored
Fix roborev fix discovering unrelated reviews in worktrees (#501)
## Summary - After fetching candidate jobs from the daemon API, `roborev fix --open`, `--list`, and `--batch` now filter jobs to only those relevant to the current worktree - Single-commit SHA refs are checked via `git merge-base --is-ancestor` against the worktree HEAD - Range refs parse the end commit and check its reachability - Dirty/empty refs compare the job's stored branch against the current worktree branch - All paths fail open on git errors or missing metadata to avoid silently dropping work - Adds `TestFilterReachableJobs` (14 subtests) and `TestRunFixOpenFiltersUnreachableJobs` integration test 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 35ff13a commit 46c6e4b

File tree

3 files changed

+514
-12
lines changed

3 files changed

+514
-12
lines changed

cmd/roborev/fix.go

Lines changed: 139 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,6 +82,15 @@ Examples:
8282
`,
8383
Args: cobra.ArbitraryArgs,
8484
RunE: func(cmd *cobra.Command, args []string) error {
85+
// Migrate stale relative core.hooksPath to absolute
86+
// so linked worktrees resolve hooks correctly. Best-
87+
// effort: runs from a CLI path the user invokes
88+
// directly, unlike the post-commit hook which can't
89+
// self-heal when hooks are already misresolved.
90+
if root, err := git.GetRepoRoot("."); err == nil {
91+
_ = git.EnsureAbsoluteHooksPath(root)
92+
}
93+
8594
// Support deprecated --unaddressed as alias for --open
8695
if unaddressed {
8796
open = true
@@ -433,24 +442,29 @@ func runFixOpen(cmd *cobra.Command, branch string, newestFirst bool, opts fixOpt
433442
return fmt.Errorf("get working directory: %w", err)
434443
}
435444

436-
repoRoot := workDir
445+
worktreeRoot := workDir
446+
if root, err := git.GetRepoRoot(workDir); err == nil {
447+
worktreeRoot = root
448+
}
449+
apiRepoRoot := worktreeRoot
437450
if root, err := git.GetMainRepoRoot(workDir); err == nil {
438-
repoRoot = root
451+
apiRepoRoot = root
439452
}
440453

441454
seen := make(map[int64]bool)
442455

443456
for {
444-
jobIDs, err := queryOpenJobIDs(ctx, repoRoot, branch)
457+
jobs, err := queryOpenJobs(ctx, apiRepoRoot, branch)
445458
if err != nil {
446459
return err
447460
}
461+
jobs = filterReachableJobs(worktreeRoot, "", jobs)
448462

449463
// Filter out jobs we've already processed
450464
var newIDs []int64
451-
for _, id := range jobIDs {
452-
if !seen[id] {
453-
newIDs = append(newIDs, id)
465+
for _, j := range jobs {
466+
if !seen[j.ID] {
467+
newIDs = append(newIDs, j.ID)
454468
}
455469
}
456470

@@ -482,6 +496,99 @@ func runFixOpen(cmd *cobra.Command, branch string, newestFirst bool, opts fixOpt
482496
}
483497
}
484498

499+
// filterReachableJobs returns only those jobs relevant to the
500+
// current worktree. SHA and range refs are checked via the commit
501+
// graph; non-SHA refs (dirty, empty, task labels) fall back to
502+
// branch matching. branchOverride is the explicit --branch value
503+
// for non-mutating flows (e.g. --list); when set, all job types
504+
// use branch matching, so cross-branch listing works for SHA/range
505+
// jobs too. Mutating flows (--open, --batch) must pass "" so that
506+
// fixes are never applied to the wrong checkout. On git errors the
507+
// job is kept (fail open) to avoid silently dropping work.
508+
func filterReachableJobs(
509+
worktreeRoot, branchOverride string,
510+
jobs []storage.ReviewJob,
511+
) []storage.ReviewJob {
512+
matchBranch := branchOverride
513+
if matchBranch == "" {
514+
matchBranch = git.GetCurrentBranch(worktreeRoot)
515+
}
516+
var filtered []storage.ReviewJob
517+
for _, j := range jobs {
518+
if jobReachable(
519+
worktreeRoot, matchBranch, branchOverride != "", j,
520+
) {
521+
filtered = append(filtered, j)
522+
}
523+
}
524+
return filtered
525+
}
526+
527+
// jobReachable decides whether a single job belongs to the current
528+
// worktree. When branchOnly is true (explicit --branch in a
529+
// non-mutating flow), all job types match by Branch field so
530+
// cross-branch listing works. Otherwise SHA and range refs are
531+
// checked via the commit graph, and non-SHA refs fall back to
532+
// branch matching.
533+
func jobReachable(
534+
worktreeRoot, matchBranch string,
535+
branchOnly bool, j storage.ReviewJob,
536+
) bool {
537+
ref := j.GitRef
538+
539+
// When an explicit branch was requested (non-mutating listing),
540+
// match all job types by their stored Branch field.
541+
if branchOnly {
542+
return branchMatch(matchBranch, j.Branch)
543+
}
544+
545+
// Range ref: check whether the end commit is reachable.
546+
if _, end, ok := git.ParseRange(ref); ok {
547+
reachable, err := git.IsAncestor(worktreeRoot, end, "HEAD")
548+
return err != nil || reachable
549+
}
550+
551+
// SHA ref: check commit graph reachability.
552+
if looksLikeSHA(ref) {
553+
reachable, err := git.IsAncestor(worktreeRoot, ref, "HEAD")
554+
return err != nil || reachable
555+
}
556+
557+
// Non-SHA ref (empty, "dirty", task labels like "run"/"analyze"):
558+
// match by branch when possible.
559+
return branchMatch(matchBranch, j.Branch)
560+
}
561+
562+
// branchMatch returns true when a job's branch is compatible with
563+
// the match branch. When both are known they must be equal. When
564+
// the job has no branch, fail open (include it). When the match
565+
// branch is unknown (detached HEAD), exclude jobs that do have a
566+
// branch to avoid cross-worktree leaks in mutating flows.
567+
func branchMatch(matchBranch, jobBranch string) bool {
568+
if matchBranch == "" {
569+
return jobBranch == ""
570+
}
571+
if jobBranch == "" {
572+
return true
573+
}
574+
return jobBranch == matchBranch
575+
}
576+
577+
// looksLikeSHA returns true if s looks like a hex commit SHA (7-40
578+
// hex characters). This avoids calling git merge-base on task labels
579+
// and other non-commit refs.
580+
func looksLikeSHA(s string) bool {
581+
if len(s) < 7 || len(s) > 40 {
582+
return false
583+
}
584+
for _, c := range []byte(s) {
585+
if (c < '0' || c > '9') && (c < 'a' || c > 'f') {
586+
return false
587+
}
588+
}
589+
return true
590+
}
591+
485592
func queryOpenJobs(
486593
ctx context.Context,
487594
repoRoot, branch string,
@@ -553,15 +660,30 @@ func runFixList(cmd *cobra.Command, branch string, newestFirst bool) error {
553660
if err != nil {
554661
return fmt.Errorf("get working directory: %w", err)
555662
}
556-
repoRoot := workDir
663+
worktreeRoot := workDir
664+
if root, err := git.GetRepoRoot(workDir); err == nil {
665+
worktreeRoot = root
666+
}
667+
apiRepoRoot := worktreeRoot
557668
if root, err := git.GetMainRepoRoot(workDir); err == nil {
558-
repoRoot = root
669+
apiRepoRoot = root
559670
}
560671

561-
jobIDs, err := queryOpenJobIDs(ctx, repoRoot, branch)
672+
jobs, err := queryOpenJobs(ctx, apiRepoRoot, branch)
562673
if err != nil {
563674
return err
564675
}
676+
// When listing a specific branch, filter by reachability/branch.
677+
// When listing all branches (branch==""), skip filtering — the
678+
// user explicitly asked for everything in this repo.
679+
if branch != "" {
680+
jobs = filterReachableJobs(worktreeRoot, branch, jobs)
681+
}
682+
683+
jobIDs := make([]int64, len(jobs))
684+
for i, j := range jobs {
685+
jobIDs[i] = j.ID
686+
}
565687

566688
if !newestFirst {
567689
for i, j := 0, len(jobIDs)-1; i < j; i, j = i+1, j-1 {
@@ -837,9 +959,14 @@ func runFixBatch(cmd *cobra.Command, jobIDs []int64, branch string, newestFirst
837959

838960
// Discover jobs if none provided
839961
if len(jobIDs) == 0 {
840-
jobIDs, err = queryOpenJobIDs(ctx, apiRepoRoot, branch)
841-
if err != nil {
842-
return err
962+
jobs, queryErr := queryOpenJobs(ctx, apiRepoRoot, branch)
963+
if queryErr != nil {
964+
return queryErr
965+
}
966+
jobs = filterReachableJobs(repoRoot, "", jobs)
967+
jobIDs = make([]int64, len(jobs))
968+
for i, j := range jobs {
969+
jobIDs[i] = j.ID
843970
}
844971
if !newestFirst {
845972
for i, j := 0, len(jobIDs)-1; i < j; i, j = i+1, j-1 {

0 commit comments

Comments
 (0)