From 8503b9304733e05d9a285a014568d85fe2858425 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Fri, 22 May 2026 12:23:57 -0400 Subject: [PATCH] modify: only require submit when changes affect PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, `gh stack modify` always transitioned to `PhasePendingSubmit` after completing on any stack with a remote ID (`s.ID != ""`). This blocked the user from running another `modify` until they ran `gh stack submit`, even when the modifications only touched local branches without PRs. This was overly restrictive. If a user is working at the top of their stack with branches that haven't been pushed or had PRs created yet, restructuring those branches is a purely local operation — there is no remote state to reconcile, and no reason to force a submit before allowing further modifies. ## What changed The condition for entering `PhasePendingSubmit` is now `s.ID != "" && affectsPRs` instead of just `s.ID != ""`. A new `affectsPRs` flag is tracked throughout the apply process. It is set to `true` when any of the following occurs: - A **renamed** branch has a `PullRequest` ref - A **folded** branch (source or target) has a `PullRequest` ref - A **dropped** branch has a `PullRequest` ref - A **rebased** branch (during cascading rebase) has a `PullRequest` ref If none of these conditions are met, the modify state file is cleared immediately — no pending-submit lock, no "run `gh stack submit`" prompt. ## Changes by file **`internal/modify/state.go`** - Added `AffectsPRs bool` field to `StateFile`. This persists the flag across conflict boundaries so that `ContinueApply` knows whether actions applied before the conflict already affected PR branches. **`internal/modify/apply.go`** - `ApplyPlan`: tracks `affectsPRs` through each step (rename, fold, drop, rebase). Saves the flag into conflict state when a conflict occurs. Uses `s.ID != "" && affectsPRs` for the pending-submit decision. - `ContinueApply`: initializes `affectsPRs` from the saved state file, then checks the conflict branch and remaining branches for PRs during the cascading rebase. Uses the same combined condition. - Both functions set `result.NeedsSubmit` / show the "run submit" message only when the flag is true. **`internal/tui/modifyview/types.go`** - Added `NeedsSubmit bool` to `ApplyResult` so the caller can use it for the success message. **`cmd/modify.go`** - `printModifySuccess` now takes its cue from `result.NeedsSubmit` instead of `s.ID != ""`. The "run `gh stack submit`" hint is only shown when PR branches were actually affected. **`internal/modify/apply_test.go`** - Updated `TestApplyPlan_PendingSubmitForRemoteStack` to use branches with PRs and trigger an actual rebase, validating the pending-submit path correctly. - Added `TestApplyPlan_ClearsStateForRemoteStackWithNoPRBranches`: remote stack where no branches have PRs → state is cleared. - Added `TestApplyPlan_PendingSubmitOnlyWhenPRBranchesAffected`: remote stack with a mix of PR and non-PR branches, only the non-PR branch is renamed → state is cleared, `NeedsSubmit` is false. ## Behavior summary | Scenario | Before | After | |---|---|---| | Modify on local stack (no remote ID) | State cleared | State cleared (unchanged) | | Modify on remote stack, PR branches affected | `PhasePendingSubmit` | `PhasePendingSubmit` (unchanged) | | Modify on remote stack, only local branches affected | `PhasePendingSubmit` ❌ | State cleared ✅ | The `CheckStateGuard` function (used by `add`, `push`, `sync`, `unstack`, `rebase`) already did not block on `PhasePendingSubmit`, so those commands are unaffected by this change. --- cmd/modify.go | 6 +- internal/modify/apply.go | 51 +++++++++++-- internal/modify/apply_test.go | 118 +++++++++++++++++++++++++++++-- internal/modify/state.go | 5 ++ internal/tui/modifyview/types.go | 1 + 5 files changed, 166 insertions(+), 15 deletions(-) diff --git a/cmd/modify.go b/cmd/modify.go index 4e1456c..d0d4656 100644 --- a/cmd/modify.go +++ b/cmd/modify.go @@ -149,13 +149,13 @@ func runModify(cfg *config.Config) error { } // Print success summary - printModifySuccess(cfg, applyResult, s.ID != "") + printModifySuccess(cfg, applyResult) return nil } // printModifySuccess prints a summary of what was applied. -func printModifySuccess(cfg *config.Config, result *modifyview.ApplyResult, hasRemoteStack bool) { +func printModifySuccess(cfg *config.Config, result *modifyview.ApplyResult) { if result == nil { return } @@ -178,7 +178,7 @@ func printModifySuccess(cfg *config.Config, result *modifyview.ApplyResult, hasR } cfg.Printf("") - if hasRemoteStack { + if result.NeedsSubmit { cfg.Printf("Run `%s` to push your changes and update the stack of PRs on GitHub", cfg.ColorCyan("gh stack submit")) } diff --git a/internal/modify/apply.go b/internal/modify/apply.go index adcd7e9..674ed35 100644 --- a/internal/modify/apply.go +++ b/internal/modify/apply.go @@ -137,6 +137,8 @@ func ApplyPlan( result := &modifyview.ApplyResult{Success: true} + // Track whether any action affects a branch with a PR. + affectsPRs := false // Collect original refs for rebase --onto, including trunk branchNames := make([]string, 0, len(s.Branches)+1) branchNames = append(branchNames, s.Trunk.Branch) @@ -207,6 +209,9 @@ func ApplyPlan( OldName: oldName, NewName: newName, }) + if n.Ref.PullRequest != nil { + affectsPRs = true + } cfg.Successf("Renamed %s → %s", oldName, newName) } } @@ -255,6 +260,15 @@ func ApplyPlan( baseBranch := s.ActiveBaseBranch(foldBranch) + // Check if fold source or target has a PR + if n.Ref.PullRequest != nil { + affectsPRs = true + } + targetIdx := s.IndexOf(targetBranch) + if targetIdx >= 0 && s.Branches[targetIdx].PullRequest != nil { + affectsPRs = true + } + if n.PendingAction.Type == modifyview.ActionFoldDown { // Fold-down: cherry-pick the folded branch's commits onto the target. commits, err := git.LogRange(baseBranch, foldBranch) @@ -301,6 +315,7 @@ func ApplyPlan( stateFile.RemainingBranches = remaining stateFile.OriginalBranch = currentBranch stateFile.OriginalRefs = originalParentTips + stateFile.AffectsPRs = affectsPRs if saveErr := SaveState(gitDir, stateFile); saveErr != nil { cfg.Warningf("failed to save conflict state: %v", saveErr) } @@ -351,6 +366,7 @@ func ApplyPlan( Branch: dropBranch, PRNumber: n.Ref.PullRequest.Number, }) + affectsPRs = true } s.Branches = append(s.Branches[:dropIdx], s.Branches[dropIdx+1:]...) @@ -466,6 +482,10 @@ func ApplyPlan( conflict.ConflictedFiles = files } + if b.PullRequest != nil { + affectsPRs = true + } + // Save conflict state so --continue can resume remaining := make([]string, 0) for j := i + 1; j < len(s.Branches); j++ { @@ -479,6 +499,7 @@ func ApplyPlan( stateFile.RemainingBranches = remaining stateFile.OriginalBranch = currentBranch stateFile.OriginalRefs = originalParentTips + stateFile.AffectsPRs = affectsPRs if saveErr := SaveState(gitDir, stateFile); saveErr != nil { cfg.Warningf("failed to save conflict state: %v", saveErr) } @@ -492,6 +513,9 @@ func ApplyPlan( } cfg.Successf("Rebased %s onto %s", b.Branch, newBase) + if b.PullRequest != nil { + affectsPRs = true + } result.MovedBranches++ } @@ -506,14 +530,15 @@ func ApplyPlan( // Update base SHAs updateBaseSHAs(s) - // Update state file phase - if s.ID != "" { + // Update state file phase — only require submit when PRs are affected + needsSubmit := s.ID != "" && affectsPRs + result.NeedsSubmit = needsSubmit + if needsSubmit { stateFile.Phase = PhasePendingSubmit if err := SaveState(gitDir, stateFile); err != nil { cfg.Warningf("failed to update modify state: %s", err) } } else { - // No remote stack — clear the state file ClearState(gitDir) } @@ -662,6 +687,14 @@ func ContinueApply( return fmt.Errorf("stack at index %d not found (stack file may have changed)", state.StackIndex) } + // Carry forward whether any prior actions already affected PRs + affectsPRs := state.AffectsPRs + + // Check the conflict branch itself + if idx := s.IndexOf(state.ConflictBranch); idx >= 0 && s.Branches[idx].PullRequest != nil { + affectsPRs = true + } + // Finish the in-progress git operation (rebase or cherry-pick) if state.ConflictType == "cherry_pick" { if err := git.CherryPickContinue(); err != nil { @@ -739,6 +772,7 @@ func ContinueApply( } state.ConflictBranch = branchName state.RemainingBranches = remaining + state.AffectsPRs = affectsPRs _ = SaveState(gitDir, state) cfg.Warningf("Conflict rebasing %s", branchName) @@ -757,8 +791,10 @@ func ContinueApply( } cfg.Successf("Rebased %s onto %s", branchName, newBase) + if b.PullRequest != nil { + affectsPRs = true + } } - // All rebases done — check out the best branch if state.OriginalBranch != "" { targetBranch := resolveCheckoutBranch(state.OriginalBranch, state.Plan, state.Snapshot, s) @@ -771,8 +807,9 @@ func ContinueApply( // Update base SHAs updateBaseSHAs(s) - // Transition to pending_submit or clear - if s.ID != "" { + // Transition to pending_submit only when PRs are affected + needsSubmit := s.ID != "" && affectsPRs + if needsSubmit { state.Phase = PhasePendingSubmit state.ConflictBranch = "" state.RemainingBranches = nil @@ -790,7 +827,7 @@ func ContinueApply( } cfg.Successf("Stack modified successfully") - if state.PriorRemoteStackID != "" { + if needsSubmit { cfg.Printf("") cfg.Printf("Run `%s` to push your changes and update the stack of PRs on GitHub", cfg.ColorCyan("gh stack submit")) diff --git a/internal/modify/apply_test.go b/internal/modify/apply_test.go index b3bf1b9..82a8353 100644 --- a/internal/modify/apply_test.go +++ b/internal/modify/apply_test.go @@ -1262,8 +1262,8 @@ func TestApplyPlan_PendingSubmitForRemoteStack(t *testing.T) { ID: "remote-stack-123", Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ - {Branch: "A"}, - {Branch: "B"}, + {Branch: "A", PullRequest: &stack.PullRequestRef{Number: 1}}, + {Branch: "B", PullRequest: &stack.PullRequestRef{Number: 2}}, }, } @@ -1277,7 +1277,7 @@ func TestApplyPlan_PendingSubmitForRemoteStack(t *testing.T) { } mock := newApplyMock(gitDir, branchSHAs) - mock.IsAncestorFn = func(a, d string) (bool, error) { return true, nil } + mock.IsAncestorFn = func(a, d string) (bool, error) { return false, nil } mock.MergeBaseFn = func(a, b string) (string, error) { if a == "main" && b == "A" { return branchSHAs["main"], nil @@ -1295,16 +1295,19 @@ func TestApplyPlan_PendingSubmitForRemoteStack(t *testing.T) { defer cfg.Out.Close() defer cfg.Err.Close() + // Reverse nodes so position differs → triggers rebase of PR branches nodes := makeNodes(&sf.Stacks[0]) + nodes[0], nodes[1] = nodes[1], nodes[0] - _, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs) + result, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs) require.NoError(t, err) - // Remote stack should transition to "pending_submit" + // Remote stack with PR branches affected should transition to "pending_submit" state, loadErr := LoadState(gitDir) require.NoError(t, loadErr) require.NotNil(t, state) assert.Equal(t, "pending_submit", state.Phase) + assert.True(t, result.NeedsSubmit, "NeedsSubmit should be true when PR branches are affected") } func TestApplyPlan_ClearsStateForLocalStack(t *testing.T) { @@ -1345,6 +1348,111 @@ func TestApplyPlan_ClearsStateForLocalStack(t *testing.T) { assert.False(t, StateExists(gitDir)) } +func TestApplyPlan_ClearsStateForRemoteStackWithNoPRBranches(t *testing.T) { + // Remote stack (has ID) but branches have no PRs — local-only modify + s := stack.Stack{ + ID: "remote-stack-456", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "A"}, + {Branch: "B"}, + }, + } + + gitDir := t.TempDir() + sf := writeTestStackFile(t, gitDir, s) + + branchSHAs := map[string]string{ + "main": "sha-main", + "A": "sha-A", + "B": "sha-B", + } + + mock := newApplyMock(gitDir, branchSHAs) + mock.IsAncestorFn = func(a, d string) (bool, error) { return true, nil } + mock.MergeBaseFn = func(a, b string) (string, error) { + if a == "main" && b == "A" { + return branchSHAs["main"], nil + } + if a == "A" && b == "B" { + return branchSHAs["A"], nil + } + return "merge-base", nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + defer cfg.Out.Close() + defer cfg.Err.Close() + + nodes := makeNodes(&sf.Stacks[0]) + + result, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs) + require.NoError(t, err) + + // Remote stack but no PR branches affected → state should be cleared + assert.False(t, StateExists(gitDir), "state file should be cleared when no PR branches are affected") + assert.False(t, result.NeedsSubmit, "NeedsSubmit should be false when no PR branches are affected") +} + +func TestApplyPlan_PendingSubmitOnlyWhenPRBranchesAffected(t *testing.T) { + // Stack with one PR branch (A) and one local branch (B). + // Only rename the local branch B — PRs should not be affected. + s := stack.Stack{ + ID: "remote-stack-789", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "A", PullRequest: &stack.PullRequestRef{Number: 1}}, + {Branch: "B"}, + }, + } + + gitDir := t.TempDir() + sf := writeTestStackFile(t, gitDir, s) + + branchSHAs := map[string]string{ + "main": "sha-main", + "A": "sha-A", + "B": "sha-B", + } + + mock := newApplyMock(gitDir, branchSHAs) + mock.IsAncestorFn = func(a, d string) (bool, error) { return true, nil } + mock.MergeBaseFn = func(a, b string) (string, error) { + if a == "main" && b == "A" { + return branchSHAs["main"], nil + } + if a == "A" && b == "B" || a == "A" && b == "B-renamed" { + return branchSHAs["A"], nil + } + return "merge-base", nil + } + mock.RenameBranchFn = func(old, newName string) error { return nil } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, _ := config.NewTestConfig() + defer cfg.Out.Close() + defer cfg.Err.Close() + + nodes := makeNodes(&sf.Stacks[0]) + // Rename only the non-PR branch B + nodes[1].PendingAction = &modifyview.PendingAction{ + Type: modifyview.ActionRename, + NewName: "B-renamed", + } + + result, _, err := ApplyPlan(cfg, gitDir, &sf.Stacks[0], sf, nodes, "A", noopUpdateBaseSHAs) + require.NoError(t, err) + + // Only non-PR branch was renamed — should clear state, not pending submit + assert.False(t, StateExists(gitDir), "state file should be cleared when only non-PR branches are renamed") + assert.False(t, result.NeedsSubmit, "NeedsSubmit should be false when only non-PR branches are affected") +} + // ─── resolveCheckoutBranch ────────────────────────────────────────────────── func TestResolveCheckoutBranch_StillInStack(t *testing.T) { diff --git a/internal/modify/state.go b/internal/modify/state.go index d4e243a..2660cba 100644 --- a/internal/modify/state.go +++ b/internal/modify/state.go @@ -39,6 +39,11 @@ type StateFile struct { // Cherry-pick conflict context — which fold was in progress FoldBranch string `json:"fold_branch,omitempty"` // branch being folded FoldTarget string `json:"fold_target,omitempty"` // branch receiving the cherry-pick + + // AffectsPRs records whether any action so far has affected a branch with + // a PR. Persisted across conflict boundaries so ContinueApply can combine + // it with checks on remaining branches. + AffectsPRs bool `json:"affects_prs,omitempty"` } // Snapshot captures the pre-modify state for unwind/recovery. diff --git a/internal/tui/modifyview/types.go b/internal/tui/modifyview/types.go index e3f995d..4598c6b 100644 --- a/internal/tui/modifyview/types.go +++ b/internal/tui/modifyview/types.go @@ -47,6 +47,7 @@ type ApplyResult struct { DroppedPRs []DroppedPR RenamedBranches []RenamedBranch MovedBranches int + NeedsSubmit bool // true when the modify affected branches with PRs Message string }