address review feedback: error handling and index validation

- resolveOriginalRefs now returns (map, error) instead of silently
  swallowing RevParseMap failures; sync warns and skips rebase, rebase
  aborts with a clear error
- cascadeRebase uses a new Err field on the result struct to distinguish
  fatal errors (e.g. checkout failure) from recoverable conflicts;
  callers no longer enter conflict-recovery flow for non-conflict errors
- continueRebase validates that remaining branch indices are contiguous
  in stack order, erroring out if the stack was reordered between
  conflict and --continue

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: skarim

cmd/rebase.go

@@ -166,7 +166,10 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
 	// Sync PR state before rebase so we can detect merged PRs.
 	_ = syncStackPRs(cfg, s)
 
-	originalRefs := resolveOriginalRefs(s)
+	originalRefs, err := resolveOriginalRefs(s)
+	if err != nil {
+		return fmt.Errorf("resolving branch refs: %w", err)
+	}
 
 	// Get --onto state from merged/queued branches below the rebase range.
 	// Ensures that when --upstack excludes skipped branches, we still check
@@ -193,6 +196,11 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error {
 		OntoOldBase:  ontoOldBase,
 	})
 
+	if rebaseResult.Err != nil {
+		cfg.Errorf("%v", rebaseResult.Err)
+		return ErrSilent
+	}
+
 	if rebaseResult.Conflicted {
 		cfg.Warningf("Rebasing %s onto %s — conflict", rebaseResult.ConflictBranch, rebaseResult.ConflictBase)
 
@@ -308,17 +316,19 @@ func continueRebase(cfg *config.Config, gitDir string) error {
 
 	// Rebase remaining branches using the shared cascade helper.
 	if len(state.RemainingBranches) > 0 {
-		// Validate all remaining branches still exist in the stack and
-		// build the BranchRef slice with correct absolute indices.
+		// Validate all remaining branches still exist in the stack,
+		// are in contiguous ascending order, and build the BranchRef slice.
 		remainingRefs := make([]stack.BranchRef, 0, len(state.RemainingBranches))
 		startAbsIdx := -1
-		for _, name := range state.RemainingBranches {
+		for i, name := range state.RemainingBranches {
 			idx := s.IndexOf(name)
 			if idx < 0 {
 				return fmt.Errorf("branch %q from saved rebase state is no longer in the stack — the stack may have been modified since the rebase started; consider aborting with --abort", name)
 			}
 			if startAbsIdx < 0 {
 				startAbsIdx = idx
+			} else if idx != startAbsIdx+i {
+				return fmt.Errorf("branch %q is at stack index %d, expected %d — the stack may have been reordered since the rebase started; consider aborting with --abort", name, idx, startAbsIdx+i)
 			}
 			remainingRefs = append(remainingRefs, s.Branches[idx])
 		}
@@ -333,6 +343,11 @@ func continueRebase(cfg *config.Config, gitDir string) error {
 			OntoOldBase:  state.OntoOldBase,
 		})
 
+		if result.Err != nil {
+			cfg.Errorf("%v", result.Err)
+			return ErrSilent
+		}
+
 		if result.Conflicted {
 			cfg.Warningf("Rebasing %s onto %s — conflict", result.ConflictBranch, result.ConflictBase)
 

cmd/sync.go

@@ -110,37 +110,47 @@ func runSync(cfg *config.Config, opts *syncOptions) error {
 		// Sync PR state to detect merged PRs before rebasing.
 		_ = syncStackPRs(cfg, s)
 
-		originalRefs := resolveOriginalRefs(s)
-
-		result := cascadeRebase(cascadeRebaseOpts{
-			Cfg:          cfg,
-			Stack:        s,
-			Branches:     s.Branches,
-			StartAbsIdx:  0,
-			OriginalRefs: originalRefs,
-		})
-
-		if result.Conflicted {
-			// Abort and restore everything — sync is non-interactive.
-			if git.IsRebaseInProgress() {
-				_ = git.RebaseAbort()
+		originalRefs, err := resolveOriginalRefs(s)
+		if err != nil {
+			cfg.Warningf("Could not resolve branch SHAs — skipping rebase: %v", err)
+		} else {
+			result := cascadeRebase(cascadeRebaseOpts{
+				Cfg:          cfg,
+				Stack:        s,
+				Branches:     s.Branches,
+				StartAbsIdx:  0,
+				OriginalRefs: originalRefs,
+			})
+
+			if result.Err != nil {
+				cfg.Errorf("%v", result.Err)
+				_ = git.CheckoutBranch(currentBranch)
+				stack.SaveNonBlocking(gitDir, sf)
+				return ErrSilent
+			}
+
+			if result.Conflicted {
+				// Abort and restore everything — sync is non-interactive.
+				if git.IsRebaseInProgress() {
+					_ = git.RebaseAbort()
+				}
+				restoreErrors := restoreBranches(originalRefs)
+				_ = git.CheckoutBranch(currentBranch)
+
+				cfg.Errorf("Conflict detected rebasing %s onto %s", result.ConflictBranch, result.ConflictBase)
+				reportRestoreStatus(cfg, restoreErrors)
+				cfg.Printf("  Run `%s` to resolve conflicts interactively.",
+					cfg.ColorCyan("gh stack rebase"))
+
+				// Persist refreshed PR state even on conflict, then bail out
+				// before pushing or reporting success.
+				stack.SaveNonBlocking(gitDir, sf)
+				return ErrConflict
 			}
-			restoreErrors := restoreBranches(originalRefs)
-			_ = git.CheckoutBranch(currentBranch)
-
-			cfg.Errorf("Conflict detected rebasing %s onto %s", result.ConflictBranch, result.ConflictBase)
-			reportRestoreStatus(cfg, restoreErrors)
-			cfg.Printf("  Run `%s` to resolve conflicts interactively.",
-				cfg.ColorCyan("gh stack rebase"))
-
-			// Persist refreshed PR state even on conflict, then bail out
-			// before pushing or reporting success.
-			stack.SaveNonBlocking(gitDir, sf)
-			return ErrConflict
-		}
 
-		if result.Rebased {
-			rebased = true
+			if result.Rebased {
+				rebased = true
+			}
 		}
 		_ = git.CheckoutBranch(currentBranch)
 	}

cmd/utils.go

@@ -615,28 +615,28 @@ func fastForwardBranches(cfg *config.Config, s *stack.Stack, remote, currentBran
 // branches in the stack. Merged branches that no longer exist locally are
 // backfilled from the stack metadata. This map is used as the "before" state
 // for cascade rebases and conflict recovery.
-func resolveOriginalRefs(s *stack.Stack) map[string]string {
+func resolveOriginalRefs(s *stack.Stack) (map[string]string, error) {
 	branchNames := make([]string, 0, len(s.Branches))
 	for _, b := range s.Branches {
 		if b.IsMerged() && !git.BranchExists(b.Branch) {
 			continue
 		}
 		branchNames = append(branchNames, b.Branch)
 	}
-	originalRefs, _ := git.RevParseMap(branchNames)
+	originalRefs, err := git.RevParseMap(branchNames)
+	if err != nil {
+		return nil, fmt.Errorf("resolving branch SHAs: %w", err)
+	}
 
 	// Backfill merged branches that were deleted locally.
 	for _, b := range s.Branches {
 		if b.IsMerged() && !git.BranchExists(b.Branch) {
 			if b.Head != "" {
-				if originalRefs == nil {
-					originalRefs = make(map[string]string)
-				}
 				originalRefs[b.Branch] = b.Head
 			}
 		}
 	}
-	return originalRefs
+	return originalRefs, nil
 }
 
 // fastForwardTrunk fast-forwards the trunk branch to match its remote tracking
@@ -700,7 +700,8 @@ type cascadeRebaseOpts struct {
 // cascadeRebaseResult describes the outcome of a cascade rebase.
 type cascadeRebaseResult struct {
 	Rebased        bool     // at least one branch was successfully rebased
-	Conflicted     bool     // a conflict was detected
+	Conflicted     bool     // a rebase conflict was detected (recoverable via --continue)
+	Err            error    // a fatal error occurred (not recoverable via --continue)
 	ConflictIdx    int      // absolute index in Stack.Branches of the conflicting branch
 	ConflictBranch string   // name of the conflicting branch
 	ConflictBase   string   // base branch we were rebasing onto
@@ -789,17 +790,9 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult {
 				rebaseErr = git.RebaseOnto(base, originalRefs[base], br.Branch)
 			} else {
 				if err := git.CheckoutBranch(br.Branch); err != nil {
-					remaining := make([]string, 0, len(opts.Branches)-i-1)
-					for j := i + 1; j < len(opts.Branches); j++ {
-						remaining = append(remaining, opts.Branches[j].Branch)
-					}
 					return cascadeRebaseResult{
-						Rebased:        result.Rebased,
-						Conflicted:     true,
-						ConflictIdx:    absIdx,
-						ConflictBranch: br.Branch,
-						ConflictBase:   base,
-						Remaining:      remaining,
+						Rebased: result.Rebased,
+						Err:     fmt.Errorf("checking out %s: %w", br.Branch, err),
 					}
 				}
 				rebaseErr = git.Rebase(base)