fix(cmd): only push stack branches when submit will touch PRs

[boneskull]

Closes #79.

Summary

gh stack submit previously force-pushed every branch in scope before the PR phase, so a branch could land on origin even when the user skipped creating a PR (e.g. ESC on the title prompt).

What changed

• Plan first: For each branch (unless --push-only), we now resolve the PR action up front—existing PR update, adopt from GitHub, new PR after prompt, or skip (--update-only / user skip).
• Push second: We only push branches that are not plain skips. If a branch is skipped but a descendant will open or update a PR that uses it as base, we still push that ancestor (pushAnyway) so GitHub has a valid base ref.
• PR phase: executePRDecisions runs the API work from the precomputed plan; new PRs use executePRCreate with the already-chosen title/body.
• Dry-run: Still avoids GitHub; new branches are treated as “would create PR” so push lines match the old behavior. --update-only dry-run still prints skip lines for branches without PRs.
• Docs: Submit command long help now describes selective push.

Merged with main after #88 (SubmitOptions, CascadeOptions, prContext). doSubmitPushAndPR takes SubmitOptions; continue passes the same struct.

Review follow-ups (from #89 (comment))

• Single GitHub client: One NewClient when not dry-run and not --push-only; passed through to executePRDecisions (no second client in the PR phase).
• Push loop: decisionByName map instead of scanning decisions per branch.
• pushAnyway semantics: Only set when the ancestor decision is prActionSkip (matches the field comment; harmless before but clearer).

Tests

• go test ./cmd/...
• TestApplyMustPushForSkippedAncestors extended: child prActionUpdate / prActionAdopt, three-level chain, non-skip ancestor unchanged.

E2E tests in this repo were not run here (environment may use default master trunk while tests expect main).

  

[claude]

Claude finished @boneskull's task in 2m 52s —— View job

---

Code Review

• Gather context and read changed files
• Review cmd/submit.go changes
• Review cmd/submit_internal_test.go changes
• Check for bugs, correctness issues, and edge cases
• Post final review

---

The core idea—plan PR actions first, then selectively push—is sound and cleanly solves the original issue (branches landing on origin when the user ESCs the title prompt). The refactor is well-structured and the logic is correct. A few things worth addressing:

Moderate: GitHub client created twice

doSubmitPushAndPR (line 244) creates a github.NewClient() and passes it to planPRDecisions. Then executePRDecisions (line 390) calls github.NewClient() again. The first client is effectively discarded after planning. Consider passing the client from doSubmitPushAndPR through to executePRDecisions to avoid the double initialization (and any side effects if the token/config changes between calls).

// doSubmitPushAndPR could pass the client:
return executePRDecisions(g, cfg, root, decisions, ghClient, dryRun, openWeb, s)

Fix this →

Minor: O(n²) linear scan in push loop

doSubmitPushAndPR (lines 257–263) does a linear scan over decisions for each branch in branches. For typical stacks this is fine, but a map lookup (similar to the one already built inside applyMustPushForSkippedAncestors) would be cleaner and consistent. The map could be built once before the push loop.

Minor: pushAnyway set on non-skipped ancestors

applyMustPushForSkippedAncestors walks all ancestors of any non-skip decision and sets pushAnyway = true—including ancestors whose action is already prActionUpdate or prActionCreate. The field's own doc comment says it's meaningful "when action is prActionSkip", so setting it on non-skip decisions violates that contract (harmlessly, since shouldPush is already true for those). Consider guarding the assignment:

// in applyMustPushForSkippedAncestors
for cur := d.node.Parent; cur != nil; cur = cur.Parent {
    if x := byName[cur.Name]; x != nil && x.action == prActionSkip {
        x.pushAnyway = true
    }
}

Fix this →

Tests: missing coverage for prActionUpdate/prActionAdopt and multi-level chains

TestApplyMustPushForSkippedAncestors only tests prActionCreate as the non-skip descendant. A skipped parent should also have pushAnyway set when its child has prActionUpdate or prActionAdopt (the condition is d.action != prActionSkip, so both are covered by the code, but not by tests). Also worth adding a 3-level chain (A→B→C, A and B skipped, C gets PR) to verify both ancestors are marked.

---

What's good

• The plan-then-push separation cleanly eliminates the original bug with no new failure modes.
• ErrPRSkipped removal simplifies the API; the prDecision struct is a better abstraction.
• The --yes and dry-run paths are handled correctly in planning (no interactive calls, no GitHub lookups in dry-run).
• The adoptExistingPR fallback in executePRCreate correctly handles the race where a PR is created between planning and execution.
• The ancestor propagation logic in applyMustPushForSkippedAncestors correctly handles the case where main (trunk) is not in the decisions map.

[claude]

Claude finished @cursor[bot]'s task in 2m 30s —— View job

---

Code Review

• Gather context
• Read cmd/submit.go changes
• Read cmd/submit_internal_test.go changes
• Review correctness, edge cases, and test coverage
• Post final review

---

All four issues raised in the previous review have been addressed. Here's the follow-up assessment:

Previous issues — resolved

Double GitHub client creation — Fixed. executePRDecisions now accepts planningClient *github.Client (line 406) and the call site at line 312 passes the already-created client through. The fallback creation inside executePRDecisions (lines 415–421) is correctly limited to the edge case where the caller couldn't obtain a client (which can't happen on the non-dry-run path given the current call sites, but is safe defensive code).

O(n²) push loop scan — Fixed. decisionByName map is built once before the push loop (lines 278–281) and lookups are O(1).

pushAnyway set on non-skipped ancestors — Fixed. The ancestor walk now guards the assignment with x.action == prActionSkip (line 397), matching the field's documented semantics. The new test non_skipped_ancestor_not_given_pushAnyway_flag (line 384) verifies this.

Missing test cases — All four gaps are closed: prActionUpdate (line 349), prActionAdopt (line 360), 3-level chain (line 371), and the non-skip ancestor case (line 384).

---

New minor issue: unused g *git.Git parameter in executePRCreate

executePRCreate (line 545) accepts g *git.Git as its first parameter, but the function body never references it — every operation goes through pCtx.ghClient or pCtx.cfg. The parameter is propagated down from executePRDecisions (line 499) where g itself comes from the outer scope of doSubmitPushAndPR. This won't cause a compile error in Go (unused parameters are legal), but it's dead code that could mislead a future reader into thinking git operations happen during PR creation.

Fix this →

---

What's good

The plan-then-push split cleanly solves the original problem with no new failure modes. The decisionByName map, the SubmitOptions struct consolidation, and the planningClient thread-through are all clean improvements from the previous iteration. Test coverage is now comprehensive for applyMustPushForSkippedAncestors.

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @boneskull's task in 2m 48s —— View job

---

Code Review

• Gather context
• Read cmd/submit.go changes
• Read cmd/submit_internal_test.go changes
• Review correctness, edge cases, and test coverage
• Post final review

---

All issues raised in the two previous reviews have been addressed. Here's the current-state assessment:

Previous issues — all resolved

Double GitHub client — Fixed. executePRDecisions now accepts planningClient *github.Client (line 406), called with the already-created client at line 312. The fallback path (lines 415–421) for ghClient == nil is unreachable on the non-dry-run path given current call sites, but is harmless defensive code.

O(n²) push loop — Fixed. decisionByName map built once before the push loop (lines 278–281); lookups are O(1).

pushAnyway on non-skipped ancestors — Fixed. The ancestor walk at line 396 now guards with x.action == prActionSkip. Verified by the new non_skipped_ancestor_not_given_pushAnyway_flag test.

Missing test cases — All four gaps closed: prActionUpdate (line 349), prActionAdopt (line 360), 3-level chain (line 371), non-skip ancestor (line 384).

Unused g *git.Git in executePRCreate — Removed. The function signature at line 545 no longer has that parameter.

---

New observation: duplicate skip-message code

executePRDecisions handles prActionSkip / skipReason == "update-only" in two places that print the same string:

// dry-run path (line 451):
if d.skipReason == "update-only" {
    fmt.Printf("Skipping %s %s\n", s.Branch(b.Name), s.Muted("(no existing PR, --update-only)"))
}
continue

// non-dry-run path (line 457):
case "update-only":
    fmt.Printf("Skipping %s %s\n", s.Branch(b.Name), s.Muted("(no existing PR, --update-only)"))

The dry-run if also implicitly handles a skipReason == "user" dry-run case (silent continue), but that case is unreachable — planPRDecisions routes all new-PR branches through planPRDecisionDryRun (which always returns prActionCreate) when dryRun && !updateOnly. The two print paths could be unified by moving the prActionSkip message outside the opts.DryRun guard, but this is a style nit with no behavioral impact.

Fix this →

---

What's good

• continue path correctly reconstructs SubmitOptions (cmd/continue.go:148): no DryRun field (correct — resume always executes), correct passthrough of UpdateOnly, OpenWeb, PushOnly.
• Ancestor walk handles trunk correctly: trunk is not in decisions, so byName[cur.Name] returns nil, and the x != nil guard prevents a nil-pointer panic.
• executePRCreate race handling: the "pull request already exists" fallback to adoptExistingPR correctly handles the window between planning and execution.
• Error handling in PR phase is non-fatal: individual PR failures print warnings and allow subsequent branches to proceed — correct behavior for a best-effort operation.
• Dry-run produces all push lines for all branches: since planPRDecisionDryRun always returns prActionCreate, applyMustPushForSkippedAncestors has nothing to mark, and shouldPush is always true. Matches old behavior correctly.