📚 Pull Request Stack

• branch: main
  • fix(submit): only prompt to publish draft when base transitions to trunk #100 - branch: issue-95 (this PR)

Managed by gh-stack

Claude finished @boneskull's task in 4m 42s —— View job

Code Review

• Gather context and read changed files
• Review internal/config/config.go and internal/config/config_test.go
• Review cmd/submit.go and cmd/submit_internal_test.go
• Review cmd/sync.go, cmd/orphan.go, cmd/unlink.go
• Review new e2e tests (e2e/orphan_test.go, e2e/unlink_test.go)

Overall this is well-structured: the new stackPRBase key follows the same patterns as stackPR, stackParent, and stackForkPoint, cleanup is consistent across all paths, and the unit tests for isTransitionToTrunk are thorough. One correctness issue in the prActionUpdate path needs attention before merging.

Bug: SetPRBase is called before isTransitionToTrunk in the prActionUpdate path

cmd/submit.go lines 466–479

if err := ghClient.UpdatePRBase(d.prNum, parent); err != nil {
    fmt.Println(s.Error("failed"))
    fmt.Printf(...)
} else {
    fmt.Println(s.Success("ok"))
    _ = cfg.SetPRBase(b.Name, parent)   // ← (1) stores trunk NOW
    ...
}
...
maybeMarkPRReady(...)  // ← (2) isTransitionToTrunk reads stackPRBase
                       //      always sees trunk → always returns false

Because SetPRBase(b.Name, trunk) is called at (1) before maybeMarkPRReady at (2), by the time isTransitionToTrunk runs it reads stackPRBase == trunk and always returns false. The publish-draft prompt can never fire in the prActionUpdate path — not even on the first submit after the parent branch is merged.

The practical consequence: in the normal flow (run sync after a parent PR merges, then submit), sync calls SetPRBase(child, trunk) when it retargets, and submit also calls SetPRBase before the check. Either way, isTransitionToTrunk sees trunk and suppresses the prompt. The user must manually mark the PR ready.

Compare with adoptExistingPRDirect (cmd/submit.go:729), which correctly works around the same problem by using the live existingPR.Base.Ref instead of the stored value:

_ = pCtx.cfg.SetPRBase(branch, base)  // stored trunk
// ...
if existingPR.Draft && base == pCtx.trunk && existingPR.Base.Ref != pCtx.trunk {
    // uses live ref, not stored base — correctly avoids the ordering issue
    promptMarkPRReady(...)
}

Fix: move maybeMarkPRReady inside the else block, before SetPRBase — or capture the transition state before updating:

} else {
    fmt.Println(s.Success("ok"))
    maybeMarkPRReady(ghClient, cfg, d.prNum, b.Name, parent, trunk, s) // check BEFORE persisting
    _ = cfg.SetPRBase(b.Name, parent)
    ...
}

Fix this →

Minor: maybeMarkPRReady calls GetPR even when base != trunk

cmd/submit.go:762–779 — maybeMarkPRReady returns early if base != trunk (line 765), which is good. But when base == trunk, it fetches the full PR object via ghClient.GetPR(prNumber) just to check pr.Draft. For stacks with many PRs this is one extra API call per PR per submit. Low priority, but if the prDecision struct already carries the draft status (it does — d.draft for prActionCreate), it could be threaded through for the update path too.

Minor: orphanMergedBranch in cmd/sync.go clears stackPRBase and stackPR, but RemoveParent is the only one checked with a non-nil return

cmd/sync.go:489-495 (the new orphanMergedBranch function) — all removals are best-effort (_ = cfg.Remove...), which is consistent with the other cleanup paths. No issue.

Tests

• TestIsTransitionToTrunk covers all the right cases in isolation.
• TestDeleteMergedBranchClearsPRBase and TestOrphanMergedBranchClearsPRBase correctly verify cleanup.
• There's no test that exercises maybeMarkPRReady via the prActionUpdate path, which is why the ordering bug isn't caught. A test that sets stackPRBase = "feat-parent" and verifies the prompt fires (or doesn't fire after the fix) when UpdatePRBase succeeds would catch this.
• The new e2e tests for orphan and unlink are clean and focused.

TL;DR: The SetPRBase / maybeMarkPRReady ordering in the prActionUpdate path is a correctness bug that silently suppresses the publish-draft prompt. Everything else looks solid.

Claude finished @boneskull's task in 1m 41s —— View job

Code Review

• Gather context and read changed files
• Review cmd/submit.go ordering fix in prActionUpdate
• Review isTransitionToTrunk and maybeMarkPRReady
• Review executePRCreate and adoptExistingPRDirect
• Review cleanup paths (sync.go, orphan.go, unlink.go)
• Review tests (submit_internal_test.go, config_test.go, e2e)

The ordering bug flagged in the previous review is fixed in commit 8d96385. The rest of the changes are solid. Two minor observations below — neither is a blocker.

Fix looks correct

cmd/submit.go:469–481 — maybeMarkPRReady is now called before cfg.SetPRBase(b.Name, parent) inside the else (success) block, which is the correct ordering. GenerateAndPostStackComment was also moved inside the success block, which is a logical improvement.

TestIsTransitionToTrunkOrderingInvariant (cmd/submit_internal_test.go) is a nice addition — it explicitly documents the invariant and creates a regression guard for the specific failure mode.

adoptExistingPRDirect correctly uses the live existingPR.Base.Ref != pCtx.trunk check (rather than the stored value) since the stored base may not exist yet for a freshly adopted PR. The comment explains this clearly.

Minor: isTransitionToTrunk check runs after the GetPR API call

cmd/submit.go:764–781

func maybeMarkPRReady(ghClient *github.Client, cfg *config.Config, prNumber int, branch, base, trunk string, s *style.Style) {
    if base != trunk {
        return
    }
    pr, err := ghClient.GetPR(prNumber)  // API call
    if err != nil || !pr.Draft {
        return
    }
    if !isTransitionToTrunk(cfg, branch, trunk) {  // cheap local check
        return
    }
    promptMarkPRReady(...)
}

When isTransitionToTrunk returns false (stored base is already trunk — the steady-state after the first submit), we still pay the GetPR API call before hitting the cheap local check. Moving isTransitionToTrunk before the GetPR call would skip the network round-trip for every subsequent submit run on a PR already targeting trunk:

if !isTransitionToTrunk(cfg, branch, trunk) {
    return
}
pr, err := ghClient.GetPR(prNumber)
if err != nil || !pr.Draft {
    return
}
promptMarkPRReady(...)

Low priority — the current behavior is correct, it's just one extra API call per PR per submit when stackPRBase == trunk.

Minor: setupTestRepo vs setupTestRepoWithDir duplication

cmd/submit_internal_test.go:323–360 — setupTestRepo and setupTestRepoWithDir share nearly identical logic. The first is a strict subset of the second (the only difference is the initial commit and returning dir). setupTestRepo callers could use setupTestRepoWithDir and discard the returned dir. Small surface-area DRY issue; fine to leave as-is.

TL;DR: The core correctness bug is fixed and the fix is well-tested. The two items above are genuinely minor. Ready to merge from a correctness standpoint.