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.