GitDesktop / Tickets / #142 feat(pulls,github,gitlab): add stacked pull request support

Anonymous - 2026-08-04

assigned_to: theBGuy
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-04

Originally posted by: cloudflare-workers-and-pages[bot]

Deploying gitdesktop with Cloudflare Pages

Latest commit: 87fcd0d

Status: ✅ Deploy successful!

Preview URL: https://110dd70e.gitdesktop.pages.dev

Branch Preview URL: https://feat-stacked-pull-requests.gitdesktop.pages.dev

View logs

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-04

Originally posted by: theBGuy

Reviewer context — deliberate calls with their evidence, numbered for reference. Verification state up front: the wave was adversarially spec-reviewed pre-PR (12 findings, all fixed or ruled on the record), then shadow-reviewed again pre-open (11 more, folded in as 1d71552), and live-dogfooded end-to-end against a real 3-PR stack in a scratch repo: badges, stack section, arrow/palette navigation, the disclosure dialog, and an actual merge-async merge that atomically landed all three PRs (server-verified). cargo 945/945, clippy -D warnings clean, pnpm build + site build green.

The merge-async response parser pins GitHub's LIVE shapes, which differ from GitHub's own gh-stack docs. Probed 2026-08-04: the PUT ack is {"status": "pending", "details": {"uuid": …}} — top-level status (docs say state), uuid nested under details. Unit test merge_async_parses_the_live_response_shapes pins the probe JSON. Please don't "correct" the parser toward the documentation.

Legacy merges hard-fail on stacked PRs — probed live: gh pr merge errors ("must be merged using the asynchronous merge REST API") and REST PUT …/merge returns 403. That's why gh_pr_merge probes stack membership before every GitHub merge. The probe is fail-open to the legacy path deliberately: a probe outage must not break normal merges, and GitHub's own 403 is the backstop if the PR was actually stacked.

The merge disclosure is gated on stack-id provenance (isNativeStack), not on the detected provider. GitLab-inferred stacks carry ids minted mr-<iid> (forge/gitlab.rs); GitHub-native ids are a stack number's string (github/pr.rs) — a u64 string can never begin mr-, so the discriminator is total over both producers. A provider check fails in both directions while the forge probe is pending/failed: raw provider silently drops the disclosure on GitHub (under-disclosure of a multi-PR merge), a "default to GitHub" key invents a cascade promise on GitLab. The id travels with the data and is correct on both paths.

GitLab never gets the cascade disclosure because GitLab never cascades — its stacked MRs retarget the next MR after a merge; each merge lands one MR. The dialog on a GitLab chain is byte-identical to before this PR.

If the member-list fetch fails on a stacked GitHub PR, the dialog degrades to a count-free notice ("merges every still-open pull request below it") with a "Merge stack" confirm label — never to the ordinary single-PR dialog. Position/size come from the PR payload and membership from a second endpoint; when only the second fails we still know a cascade is coming, and under-disclosure is the failure direction this feature exists to prevent.

One shared predicate (pull_stack_ref, full number+position+size triple) decides "is stacked" for both the detail view and the merge path. A partial stack payload therefore fails toward the legacy merge (where GitHub 403s if it really was stacked) — never toward a silent cascade behind an ordinary dialog.

GitLab stacks are inferred — GitLab exposes no stack object. Linear chains over open MRs only; any ambiguity (a source branch owned by two open MRs, or a branching child) unmarks the entire connected component, not just the bad link — tested with the backport shape that used to re-root the chain above it (infer_mr_stacks_unmarks_the_whole_component_not_just_the_bad_link). Strictly conservative: a chain sharing a component with an unrelated ambiguous branch goes unmarked rather than guessed. Consequence of open-list inference: merged GitLab layers leave the chain (positions re-derive), while GitHub keeps merged members listed — that asymmetry is documented on the types.

Auto-merge is deliberately NOT gated for stacked PRs. The only reachable auto-merge arm today is GitLab's (mrAutoMerge), where stacking is our heuristic and merging is per-MR — disabling would remove a real capability for no real constraint. A GitHub-side gate belongs with a merge-queue arm (merge_action: "merge_queue"), parked in Phase 2.

Every stack read is fail-open and off the critical path: the list join rides tokio::join! with gh pr list; the detail view adds one concurrent call always and a second only when stacked; any failure (GHES has no stacks API — its docs 404 — plus network/parse) yields stack: null and the UI renders exactly as before. Closed-state lists skip the join; past 100 open stacks the surplus goes unmarked (one page, no pagination on a decoration).

The merge path adds one serialized REST probe per GitHub merge — deliberate: merging is rare and heavyweight, and correctness of the cascade decision precedes it (see 2/6).

Scope box (Phase 2, recorded home in the repo's planning docs): no stack create/add/unstack from the app, no reorder (GitHub exposes NO reorder API — it's CLI-local), no forge_pr_edit base retargeting, no merge-queue merge_action. This wave is awareness + navigation + correct merging.

Known accepted costs, on the record: (a) GitLab detail views fetch the open-MR list to derive the chain (concurrent with the existing calls, capped at one page); (b) StackSection rows are individual tab stops, matching the app's existing list idiom (no roving tabindex); (c) the marketing site gets a capability line, not a FeatureRow — deliberate weight call; (d) merged probe PRs [#20]–25 in the scratch fixture repo are test artifacts of the live verification.

Posted by GitDesktop — automated agent comment, verify before acting on it.

Related

Tickets: ~~#20~~
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-04

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Adds stacked-PR awareness across GitHub (native stacks API + async merge) and GitLab (chain inference over open MRs), with list badges, a Stack section, palette navigation, and a scope-disclosing merge dialog. The shape is sound — the inference algorithm is careful and well-tested, the wire shapes are pinned, and the docs surfaces are covered — but there are three correctness gaps and a latency regression I'd want addressed before merge.

Correctness

should-fix — src-tauri/src/github/pr.rs, gh_pr_stack vs gh_pr_merge: a failed stack probe on the detail path is indistinguishable from "unstacked", so the merge can cascade with no disclosure at all. gh_pr_stack returns unstacked = (None, Vec::new()) both when run_gh_raw errors, when out.code != 0, and when the PR genuinely has no stack object. Concrete case: gh pr view succeeds while the concurrent gh api repos/{slug}/pulls/{number} hits a transient 5xx/secondary rate limit/GH_TIMEOUT — PrDetails.stack is null, so StackSection renders nothing and stackMergeDisclosure returns null (ordinary single-PR dialog), and react-query caches that response. The user clicks Merge, gh_pr_merge re-probes independently via gh_pr_is_stacked, that call succeeds, and gh_pr_merge_async lands every open PR below — exactly the under-disclosure the feature exists to prevent. The reviewer notes record the partial-payload case (note 6) and the member-list failure (note 5); this asymmetric-hop case isn't covered by either, and the guard they describe isn't in the code path. Fix: make gh_pr_stack tri-state — return e.g. Option<bool> /* known */ alongside, or a small enum Stacked(PrStackInfo) | Unstacked | Unknown — and carry it as a new PrDetails field (stack_unknown: bool). Knock-ons if you take that route: set it on all three PrDetails construction sites (github/pr.rs gh_pr_view, forge/gitlab.rs view_pr, forge/bitbucket.rs view_pr — the latter two false), add stackUnknown: boolean to PrDetails in src/lib/git/types.ts, and have stackMergeDisclosure emit the existing count-free notice + "Merge stack" label when stackUnknown && provider is GitHub (reusing the branch it already has for an empty member list, whose comment should then say it covers both an empty member list and an unknown membership).

should-fix — src-tauri/src/github/pr.rs, classify_merge_async + gh_pr_merge: "enqueued" maps to MergeAsyncOutcome::Done, gh_pr_merge_async returns Ok(()), and gh_pr_merge then falls straight into if delete_branch { gh_delete_remote_head_branch(...) }. Concrete case: a stacked PR whose base branch requires a merge queue — GitHub enqueues rather than merging, we report success and delete the PR's remote head ref, which closes the still-queued PR and removes it from the queue. PrMergeOutcome's doc ("The PR did merge") and the "Merged #N" toast also over-claim for this state. The comment on MergeAsyncOutcome records the decision to treat enqueued as a terminal success, but not the branch-deletion knock-on. Fix: add an Enqueued variant to MergeAsyncOutcome, have gh_pr_merge_async return the terminal outcome (AppResult<MergeAsyncOutcome>) instead of (), and in gh_pr_merge skip gh_delete_remote_head_branch for Enqueued, returning a PrMergeOutcome whose cleanup_warning says the PR was added to the merge queue and the branch was left in place — which also means widening PrMergeOutcome's doc comment, currently written as "the PR did merge; cleanup_warning carries a caveat when cleanup failed".

should-fix — src-tauri/src/forge/gitlab.rs, list_prs (lines ~474–494): the inference set is API-truncated before apply_mr_stacks runs, so a limit can produce wrong positions, not just missing ones. per_page = limit.map_or(100, |n| n.clamp(1, 100)) is sent to GitLab, then apply_mr_stacks(&mut prs) runs over whatever came back — so the comment two lines below ("Inferred before truncation, so a limit narrows what's shown without distorting the chains") is contradicted by the code above it. Concrete case: the MCP list_pull_requests tool forwards limit; with limit: 5 on a project whose stack bottom is older than the 5 newest MRs, that bottom is absent from sources, so the next MR up gets zero parent candidates and is emitted as a genuine chain bottom — the row reports {"position": 1, "size": 2} for an MR that is really 3 of 4, and read_forge.rs's newly added "position 1 = bottom and merges first" tells the agent to believe it. (GitHub's arm is unaffected: gh_open_stack_memberships always uses per_page=100 and takes position/size from the server.) Fix: when state == "open", always request per_page=100 and let the existing prs.truncate(n) — which already runs after apply_mr_stacks — narrow the result; then reword the list_prs comment to say the open set is always fetched at a full page so a limit only narrows what's returned, and leave apply_mr_stacks's existing caveat (which then correctly scopes to >100 open MRs).

Performance

should-fix — the supplementary stack reads are joined, not raced, so they gate the primary payload; three sites, and the comments at each claim the opposite. src-tauri/src/github/pr.rs gh_pr_list (~line 1450) says stacks ride "alongside the list rather than adding a gh spawn + round-trip to the list's critical path", but tokio::join! returns only when both arms finish — a hanging repos/{slug}/stacks call blocks the whole PR list for up to GH_TIMEOUT (30s) even though gh pr list came back. Same at gh_pr_view (~line 2460), where the joined gh_pr_stack makes up to two sequential 30s calls, so the detail view can be blocked ~60s. Worst is src-tauri/src/forge/gitlab.rs view_pr (~line 1040), which joins the /changes call with list_prs(repo_path, "open", None) at GLAB_NETWORK_TIMEOUT — up to 120s of added blocking on an MR detail whose own data is ready. Fix: wrap each supplementary arm in tokio::time::timeout with a short budget (a few seconds) and fall back to the existing empty/None value on elapse — gh_open_stack_memberships, gh_pr_stack, and the list_prs arm inside GitLab's view_pr are all already infallible-by-contract, so the fallback needs no new error handling. Then reword the three comments (and reviewer note 9's "off the critical path" framing) to say the probe is bounded rather than off the path. If you'd rather keep the full budget for the list, the repo already has the right idiom: hydrate row badges through a separate command the way forge_pr_list_ci / usePrListCi hydrates row CI in PullRequestsPanel.tsx.

Edge cases

nit — src/features/pulls/StackSection.tsx, memberPresentation default arm: a GitHub member with neither merged_at nor state maps through unwrap_or_default() to "" in stack_members_from, so the row renders a neutral circle with no word — the only status signal disappears. Fall back to word: state.trim() || "unknown".

nit — src/features/pulls/StackSection.tsx, the header Stack · {stack.position} of {rows.length}: stack_members_from drops a member whose number is absent (p.number? inside filter_map) while positions still come from the pre-filter enumerate index, so the header can read "3 of 2" and stackMergeBelow's count in the confirm label can undercount. Deriving the denominator from Math.max(rows.length, stack.position) (or keeping stack.size when it exceeds rows.length) closes it.

Readability

nit — src/features/pulls/RemotePrView.tsx: isSelectedPr (line 439) duplicates the identical derivation inlined at lines 342–343 in the pending-section effect; hoist the const above that effect and use it in both.

Tests

should-fix — two pure steps of this feature are the only ones without coverage, while every sibling (infer_mr_stacks, pull_stack_ref, stack_members_from, classify_merge_async, the serde shapes) has a test. In src-tauri/src/github/pr.rs, the stacks-list → membership mapping is inlined in the async gh_open_stack_memberships, so the if stack.open != Some(true) { continue } filter is untested — note that it fails closed on a payload where open is simply absent (every stack silently dropped, no badges anywhere), which is precisely the drift a test would catch. Extract fn stack_memberships_from(raw: Vec<serde_json::Value>) -> HashMap<u64, PrStackInfo> and pin it (open stack mapped with 1-based positions and size = member count, dissolved open: false skipped, open absent, one malformed entry not poisoning the rest), adding it to the mod tests use super::{…} list. In src-tauri/src/forge/gitlab.rs, mr_stack_from_rows is pure over &[PrInfo] and untested — a fixture with two chains plus an unstacked MR would pin the id filter, the position sort, and the state lowercasing.

Docs: README, site/src/data/capabilities.ts, src/features/help/content.ts, and the changelog.d/ fragment are all updated, and the skipped site/src/pages/index.astro FeatureRow is a recorded deliberate weight call — no gap here.

Posted by GitDesktop — AI output, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-04

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

No genuine security issues in these changes: the new gh/glab calls pass fixed-position argv arrays (no shell) with only a validated strategy (merge/squash/rebase) and u64 numbers interpolated into repos/…-prefixed endpoints, all new forge JSON is parsed into tolerant typed serde structs, and every new UI surface (StackSection rows, list badge, merge notice) renders forge-supplied strings as JSX text or via el.title / CSS.escaped selectors, with no URL, HTML, or LLM-prompt sink introduced.

Posted by GitDesktop — AI output, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-04

Originally posted by: theBGuy

Dispositions for the AI review's findings — all verified against the tree before acceptance; fixes land in the next push.

Correctness 1 (asymmetric-hop under-disclosure) — fixed, with the fix mechanism corrected. The tri-state shape you suggested shipped: gh_pr_stack distinguishes unknown (probe failed / unparseable body / timeout) from unstacked (readable payload, no stack key), carried as PrDetails.stackUnknown on the wire, mirrored in TS, set false on GitLab/Bitbucket with per-site reasons. One deliberate divergence from the suggested fix: the unknown arm does not reuse the count-free notice + "Merge stack" label — an unknown is not a known stack, and most unknowns are transient failures on ordinary unstacked PRs, so a "Merge stack" button would itself be a false claim. It shows a hedged notice ("Couldn't confirm whether this pull request is part of a stack…") with no confirm-label override. The MCP get_pull_request description gained the matching warning so an agent can't read stack: null + stackUnknown: true as "safe to merge".

Correctness 2 (enqueued ≠ merged) — fixed exactly as you prescribed (converged with Copilot's inline finding): Enqueued variant, cleanup skipped, PrMergeOutcome doc widened — plus the knock-ons your framing implied: queued travels as a structured wire field, the UI announces a queued state instead of "Merged #N", the MCP merge tool reports action: "queued" / deleted_branch: false, six stale statements of the old "warning == cleanup failed" contract were rewritten across both languages, and the docs gained the merge-queue clause. assert_ne!(enqueued, Merged) pins the regression.

Correctness 3 (GitLab truncation distorts inference) — fixed as prescribed: open-state fetches always take the full page, inference runs, then truncate(limit) narrows the result; both comments reworded, plus a third stale sentence on apply_mr_stacks your finding falsified that the fix round caught. The full-page cost is named as deliberate in the comment so it doesn't get "optimized" back.

Performance (joined, not bounded) — fixed at all three sites you named, with one refinement: the detail path bounds per hop, because a single outer bound would discard an already-known membership when only the member fetch is slow — collapsing a firm "position 3 of 4" disclosure into a weak hedge. And a correction to my own reviewer-context note 9, on the record: "off the critical path" over-claimed — the join removes the serial cost, the bound removes the stall; the comments (and this thread) now say bounded. The fourth stack read (gh_pr_is_stacked at merge time) stays deliberately unbounded: it's a merge-decision input, not a decoration.

Edge nits — both fixed: empty member state renders the word "unknown" (never an icon alone); the header denominator is guarded with Math.max in both arms.

Readability — fixed: isSelectedPr hoisted; the effect's deps narrowed to the boolean.

Tests — fixed as prescribed: stack_memberships_from extracted and pinned — including the open-absent-fails-closed case you called out as the silent-badge-kill drift — and mr_stack_from_rows pinned (id filter, position sort, state lowercasing, both empty cases). Suite: 948 passed, clippy -D warnings clean.

Held on the record (accepted costs, not fixed): PR-list rows carry no unknown-signal (the list offers no merge action, so unknown-ness has no disclosure consequence there); a GHES repo with a transient probe failure can over-hedge (GHES routes through the unrecognized-host path and has no stacks API — rare, and over-disclosure is the recoverable direction).

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-04

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Re-reviewed against the current diff. All four correctness/performance findings and all three nits from the last round are fixed, and the fix hunks themselves are clean apart from one inaccurate MCP field and two cosmetics.

Correctness

should-fix — src-tauri/src/mcp_server/write_forge.rs, merge_pull_request result ("deleted_branch": args.delete_branch && !outcome.queued): the queued case is now truthful, but the other caveat case still isn't. Concrete: delete_branch: true on an ordinary (non-queued) GitHub merge where gh_delete_remote_head_branch fails — head ref protected, or already deleted by GitHub's auto-delete setting — gh_pr_merge returns cleanup_warning: Some("… but the remote branch …"), queued: false, so the tool reports deleted_branch: true while the branch is still there. That's exactly the prose-vs-state split the new queued field was introduced to avoid, and the comment you added right above tells the agent to branch on these fields. Fix: hoist the flag before the warning is moved out —

// false for BOTH caveat causes: a failed deletion and a queued merge that skipped it. let deleted_branch = args.delete_branch && outcome.cleanup_warning.is_none();

— use it for the "deleted_branch" key (it subsumes && !outcome.queued, since the queued path always sets a warning), and reword the adjacent comment to cover both causes rather than only the queued one. It must be computed above the existing if let Some(warning) = outcome.cleanup_warning since that consumes the field.

Readability

nit — src-tauri/src/github/pr.rs, PrMergeOutcome::queued + src/features/pulls/RemotePrView.tsx merge onSuccess: #[serde(default)] is inert on a struct deriving only Serialize, Default, so the key is always emitted; correspondingly outcome.queued ?? false is dead against queued: boolean (non-optional) in src/lib/git/api.ts — drop both and keep the TS type as-is.

nit — src-tauri/src/github/pr.rs, gh_pr_list (~line 1543): the outer comment still says the probe rides alongside "rather than adding a gh spawn + round-trip to the list's critical path", while the inner one concedes it is on the path and merely bounded — trim the outer to the "needs its own endpoint / open rows only" facts and let the inner comment own the latency framing.

Resolved since last review

Asymmetric-hop under-disclosure: gh_pr_stack is now tri-state via PrStackProbe, carried as PrDetails.stack_unknown → stackUnknown, set false with per-site reasons on GitLab/Bitbucket, surfaced as a hedge arm in stackMergeDisclosure and in the MCP get_pull_request description. The three details_with_stack call sites and the tests-module import list were updated with it.

enqueued treated as merged: MergeAsyncOutcome::{Merged,Enqueued} + StackedMergeResult, cleanup skipped and disclosed for Queued, queued on the wire, UI/MCP branch on it, and PrMergeOutcome / forge_pr_merge / api.ts doc comments widened consistently.

GitLab limit distorting inferred positions: open state now always fetches per_page=100 with truncate after inference; both comments and the apply_mr_stacks caveat reworded to match.

Unbounded supplementary stack reads: STACKS_TIMEOUT applied at the list join, per hop in gh_pr_stack, and around GitLab's open-MR arm, with gh_pr_is_stacked deliberately exempt and documented.

memberPresentation empty-state word, the Math.max denominators in both StackSection arms, and the hoisted isSelectedPr are all in place.

Test gaps closed: stack_memberships_from extracted and pinned (including open absent failing closed and one malformed entry not poisoning the rest), mr_stack_from_rows pinned, plus the new merge_outcome_pins_queued_in_both_states and stackUnknown wire-shape assertions.

Verdict: no blocking issues — remaining items are non-blocking; merge when ready

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-04

Originally posted by: theBGuy

Round-2 dispositions — all three accepted and applied; in the next push.

deleted_branch truthfulness — fixed with your prescribed predicate, verified rather than adopted: all four outcome paths traced (queued → warning set, cleanup never attempted; deletion failed → warning set; deletion clean → none; deletion not requested → none, gated by the conjunct), so cleanup_warning.is_none() is the single honest "branch really gone" signal and subsumes !queued. Hoisted above the move, comment covers both caveat causes and names the ordering as load-bearing. One residual on the record: the computation lives in an MCP tool method with no test harness — verified by reading; a unit test here would only assert its own literal (the queued ⇒ warning invariant is established inside an async network fn, outside the repo's pure-logic testing posture), so the invariant is pinned in the comment at the depending site instead of a tautological test.

Inert #[serde(default)] — dropped (confirmed no-op first: Serialize+Default only), and the matching dead ?? false removed on the TS side.

Contradictory outer comment — trimmed; the inner bounded-on-the-path comment owns the latency framing alone.

Gates on the batch: cargo 948/948, clippy -D warnings clean, pnpm build green.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Re-reviewed against the current diff. All three items from the last round are fixed and the fix hunks are clean; one new finding remains, on the agent-facing side of the same merge path.

Documentation & agent contract

should-fix — src-tauri/src/mcp_server/write_forge.rs, the #[tool(description = …)] on merge_pull_request (lines 629–634): the tool's contract still describes a single-PR merge, while this PR makes the underlying call cascade. Concrete: an agent running with --allow-remote-write, told to "merge [#23]" where [#23] is position 3 of a 3-PR GitHub stack, now goes merge_pull_request → forge_pr_merge → gh_pr_merge → gh_pr_is_stacked → gh_pr_merge_async (src-tauri/src/github/pr.rs:652), which lands [#21] and [#22] as well — and the description gave it nothing to check first, while the result JSON names only "pull_request": 23. The same description also omits the new "action": "queued" value that the very next lines can emit (line 663), so an agent keying on action == "merged" reads a queued merge as landed. Both read-tool descriptions in read_forge.rs were updated for stacks in this same diff — this is the sibling that wasn't, and it's the one with the irreversible consequence. Fix: extend the description string, after "A merge is NOT trivially reversible.", with the two facts the code now has — e.g. "On GitHub a STACKED pull request merges bottom-up: merging it also merges every still-open pull request below it in its stack (readstack/stackMembersvia get_pull_request first).actionis \"merged\", or \"queued\" when a merge queue took the stack — queued means NOT yet merged, and the head branch is left in place (deleted_branchfalse)." Keep annotations(read_only_hint = false, destructive_hint = true) as-is; no other site describes this tool (grep found only this one, plus the module-header class note at line 14, which stays accurate).

Copilot's three inline findings (unbounded stacks join, StackSection hiding on a failed member fetch, enqueued treated as merged) are all addressed in the code as it now stands — STACKS_TIMEOUT at the join, the header + "Couldn't load the stack's members" fallback, and Enqueued/StackedMergeResult::Queued as distinct outcomes.

Resolved since last review

deleted_branch truthfulness: now args.delete_branch && outcome.cleanup_warning.is_none(), hoisted above the if let that moves the field, with the comment covering both caveat causes. Verified against all four paths in gh_pr_merge (lines 664–679): queued always sets a warning, a failed deletion sets one, a clean deletion sets none, and "not requested" is gated by the conjunct — so the predicate subsumes !queued. The partial move of outcome.cleanup_warning after reading outcome.queued is fine; no collateral in the hunk.

Inert #[serde(default)] on PrMergeOutcome::queued dropped (the struct derives Serialize, Default only, so serialization and merge_outcome_pins_queued_in_both_states are unaffected), and the matching dead ?? false removed in RemotePrView's merge onSuccess.

The contradictory outer comment on gh_pr_list's stacks join is trimmed; the inner comment owns the bounded-on-the-path framing alone.

Leftover polish (non-blocking)

src-tauri/src/forge/gitlab.rs, infer_mr_stacks tests: the cycle arm (find(|iid| !has_parent.contains(iid)) → continue) is reachable with two open MRs targeting each other's source branch (A: feat-a→feat-b, B: feat-b→feat-a) and nothing pins it — one assert!(infer_mr_stacks(&[(1,"feat-a","feat-b"),(2,"feat-b","feat-a")]).is_empty()); in infer_mr_stacks_finds_nothing_without_a_chain would cover it.

Verdict: no blocking issues — remaining items are non-blocking; merge when ready

Posted by GitDesktop — AI output, verify before acting on it.

Related

Tickets: ~~#21~~
Tickets: ~~#22~~
Tickets: ~~#23~~
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

Round-3 dispositions — both items applied; in the next push.

merge_pull_request tool description — fixed as found (the sibling-miss framing was exactly right: both read-tool descriptions gained stack sentences while the one write tool with the irreversible consequence didn't). The description now carries the cascade fact with a check-first pointer at get_pull_request's stack/stackMembers, and documents action: "queued" as not-yet-merged with the head branch left in place and deleted_branch: false. Every claim in the new text was verified against the code (cascade direction per the live probe; the exact action values from the result JSON; the deleted_branch predicate), not adopted from the suggested draft. One note for the record: MCP descriptions are per-session context an agent pays for — the cascade fact earns its lines as the one irreversible surprise in the surface, but this description grew this wave and is a candidate if the roster is ever token-trimmed.

Cycle-arm assertion — added to infer_mr_stacks_finds_nothing_without_a_chain, and made non-vacuous the measured way: the algorithm was replicated with per-arm instrumentation to confirm the 2-cycle shape is rejected by the no-bottom (cycle) arm specifically — not the ambiguity arm — so the assertion pins the path the finding named.

Gates: cargo 948/948, clippy -D warnings clean, pnpm build green.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Re-reviewed against the current diff. Both round-3 items landed and their hunks are correct: the merge_pull_request description's new claims all check out against the code (action is exactly "queued"/"merged" per write_forge.rs:661; on the queued path gh_pr_merge always sets cleanup_warning, so deleted_branch = args.delete_branch && cleanup_warning.is_none() is necessarily false, and cleanup is skipped so the head branch really is left in place), and the 2-cycle assertion exercises the arm it claims — (1, feat-a→feat-b) and (2, feat-b→feat-a) each get exactly one candidate parent, so nothing is marked ambiguous and the component is rejected by the find(|iid| !has_parent.contains(iid)) no-bottom arm, not the ambiguity arm. Nothing further to raise beyond two stragglers below.

Resolved since last review

write_forge.rs merge_pull_request tool description: now carries the GitHub bottom-up cascade fact with a check-stack/stackMembers-via-get_pull_request pointer, and documents action: "queued" as not-yet-merged with deleted_branch false — matching the result JSON and the deleted_branch predicate at the call site. The annotations(read_only_hint = false, destructive_hint = true) line and the module-header note are untouched, so no collateral.

gitlab.rs infer_mr_stacks_finds_nothing_without_a_chain: the cycle arm is pinned (leftover-polish item from last round).

Leftover polish (non-blocking)

src-tauri/src/github/pr.rs, the test module's use super::{…} block: ragged wrapping (GhPrFile, and PrMergeOutcome, orphaned on short lines) and PrMergeOutcome sorted before PrInfo — a rustfmt pass scoped to that block tidies it without touching the rest of the file.

src/features/pulls/RemotePrView.tsx, pr-stack-next / pr-stack-previous: enablement gates only on !!details.data?.stack, so on the known-stack-but-member-fetch-failed state (stackMembers: []) both commands appear in the palette and silently no-op, since goToStackNeighbor finds no member at position ± 1. Batch with the next push if you want it: fold the neighbor lookup into a small stackNeighbor(delta) helper and pass isSelectedPr && !!stackNeighbor(±1) as the enabled argument to each useHotkeyAction (the effect deps are [id, enabled] and run rides useEffectEvent, so a per-render boolean is safe) — note this also disables "next" at the top of the stack, which replaces the current documented silent clamp, so trim the "Clamped at both ends" comment accordingly.

Verdict: no blocking issues — remaining items are non-blocking; merge when ready

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Round-4 delta is two small hunks — the test-module import block tidy and the stackNeighbor enablement gate — and both are correct. useHotkeyAction is (id, run, enabled) with stableRun = useEffectEvent(run) and effect deps [id, enabled] (src/lib/hotkeys/hotkeys.tsx:73-89), so passing a per-render boolean re-registers only on a flip and run genuinely reads live state; the palette hides actions with no enabled handler (useAvailableActions / CommandPalette.tsx:14-25), so the new gate does what its comment claims. Nothing blocking.

Documentation

nit — src/features/help/content.ts, Stacked pull requests: "Next pull request in stack and Previous pull request in stack, offered from the pull-request view while you have a stacked one open" is now slightly over-broad — with this push's gate (isSelectedPr && !!stackNeighbor(±1)), "Previous" is absent from the palette at position 1 and "Next" at the top, since the palette lists only actions with an enabled handler. Reword to e.g. "…offered from the pull-request view whenever there's another member in that direction" (the rest of the sentence, including the Settings → Keyboard pointer, stays as-is; README's "palette commands for the next and previous PR in the stack" makes no such claim and needs no change).

Resolved since last review

src-tauri/src/github/pr.rs test use super::{…}: the orphaned GhPrFile, / PrMergeOutcome, lines are gone and PrInfo now sorts before PrMergeOutcome — the leftover-polish item from last round.

src/features/pulls/RemotePrView.tsx pr-stack-next / pr-stack-previous: enablement now resolves the actual neighbor via the extracted stackNeighbor, so the known-stack-but-members-missing state (stackMembers: []) and both stack ends no longer offer a silently no-op palette command; the "Clamped at both ends" comment was correctly replaced with one describing the disable, and goToStackNeighbor still re-resolves on activation (accurate — run rides useEffectEvent). No collateral: the only PrMergeOutcome/cleanupWarning consumer is this same view's merge onSuccess, and useMergePr does no optimistic merge-state write that a queued outcome would falsify.

Verdict: no blocking issues — remaining items are non-blocking; merge when ready

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

Final-round disposition: the help-copy nit is accepted and applied with your suggested wording (verified accurate in every enablement state, including the empty-member case where neither command is offered) — it sits in the working tree as a one-line reword and rides whatever the author pushes or merges next; it does not gate the merge.

Convergence summary, for the record: CI green across all 8 checks on 44d907f; all review threads resolved; this round and the two before it returned no blocking findings; the last three deltas were description text, a test assertion, palette enablement, and copy. Rounds by count: 11 → 3 → 1 → polish → one copy nit. The review is polishing, not finding — merge-ready.

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

The only delta since the last review is the one-line help-copy reword, and it matches the shipped enablement gate (isSelectedPr && !!stackNeighbor(±1)): "whenever there's another member in that direction" is true in every state, including the known-stack-with-empty-stackMembers case where stackNeighbor finds nothing and neither command is offered. No collateral — I grepped content.ts and README.md for the old claim and there is no second copy of it anywhere (README line 493 says only "palette commands for the next and previous PR in the stack", which stays accurate).

Resolved since last review

src/features/help/content.ts, Stacked pull requests: the over-broad "while you have a stacked one open" is gone, replaced by "whenever there's another member in that direction"; the Settings → Keyboard pointer and the rest of the paragraph are intact.

Nothing further to raise.

Verdict: no blocking issues — remaining items are non-blocking; merge when ready

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-08-05

Ticket changed by: theBGuy

status: open --> closed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

feat(pulls,github,gitlab): add stacked pull request support

Milestone

Searches

Help

#142 feat(pulls,github,gitlab): add stacked pull request support

GitHub backend

GitLab

UI

MCP and documentation

Related

Discussion

Deploying gitdesktop with Cloudflare Pages

Related

Correctness

Performance

Edge cases

Readability

Tests

Correctness

Readability

Resolved since last review

Documentation & agent contract

Resolved since last review

Leftover polish (non-blocking)

Related

Resolved since last review

Leftover polish (non-blocking)

Documentation

Resolved since last review

Resolved since last review

Latest commit:	`87fcd0d`
Status:	✅ Deploy successful!
Preview URL:	https://110dd70e.gitdesktop.pages.dev
Branch Preview URL:	https://feat-stacked-pull-requests.gitdesktop.pages.dev