Skip to content

feat(submit): register stacks on GitHub after submit - #22

Merged
amustafa merged 5 commits into
mainfrom
gh-stacks-api
Aug 1, 2026
Merged

feat(submit): register stacks on GitHub after submit#22
amustafa merged 5 commits into
mainfrom
gh-stacks-api

Conversation

@amustafa

@amustafa amustafa commented Jul 31, 2026

Copy link
Copy Markdown
Owner

Integrates with GitHub stacked pull requests, which entered public preview on 2026-07-30.

Why this is small

GitHub's stack is a first-class server-side object, not just a UI convention over chained base branches. Creating one requires that each PR's base ref equals the previous PR's head ref — otherwise the API returns 422.

stackr already guarantees exactly that: ghCreatePR passes --base b.ParentBranchName. So registering a stack is one API call after submit, not a restructuring.

What's here

  • internal/engine/ghstack.goghCreateStack / ghGetStack / ghAddToStack / ghUnstack, over gh api --input -. JSON on stdin keeps array ordering exact and avoids gh's field-flag syntax; gh expands the {owner}/{repo} placeholders itself, so stackr never resolves the remote. Pinned to X-GitHub-Api-Version: 2026-03-10.

  • linearSegments — stackr's graph is a tree; a GitHub stack is strictly linear and a PR may belong to only one stack. The two don't map one-to-one, so forks are cut and each child starts a fresh run based on the fork point:

    main <- a <- b <- c        segments: [a b]  [c]  [d]
                  \- d
    

    [c] and [d] are single PRs, so they stay unregistered until they grow (GitHub requires two or more PRs per stack). Their base refs already point at b either way. The trade-off — GitHub can't show a relationship between [a b] and what sits above b — is documented on the function.

  • syncGitHubStacks — runs on every submit path, scoped to what was actually pushed. Best-effort by design, matching ghMergedHeadBranches: the PRs already exist by the time it runs, so a repo without the preview, an offline machine, or an older gh warns rather than failing the submit. Dry runs contribute nothing.

  • store.BranchPR.StackNumber — persists the stack so a later submit extends it instead of duplicating. cloneBranchPR does a struct copy, so merge.go carries it for free.

Deliberately left open

resolveDivergedStack handles the case where the recorded stack's top PR isn't in the segment at all — after sr reorder/sr move/sr fold, or a manual regroup in the web UI. It currently returns an error, with the trade-off written up: rebuilding (unstack + create) mirrors local but destroys remote intent and isn't atomic; warning and leaving it never destroys anything but lets GitHub drift.

Testing

7 tests over linearSegments: linear stacks, fork-cutting, forks that extend upward, partial submits, and an unsubmitted sibling correctly not counting as a fork.

Not verified against live GitHub — endpoint shapes come from the docs. This PR's own submit is the first real exercise.

Summary by CodeRabbit

  • New Features

    • Added GitHub Stack integration for creating, retrieving, extending, and dissolving stacks.
    • Automatically synchronizes eligible pull request stacks after successful submissions.
    • Tracks stack membership across linear branch segments.
    • Reconciles existing stacks and adds newly submitted pull requests.
    • Retargets pull requests when local branch relationships change.
  • Bug Fixes

    • Reports synchronization failures without interrupting submissions.
    • Prevents dry runs from triggering synchronization or recording pushed branches.
    • Detects divergent stacks that cannot be safely reconciled.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 671a63aa-daa0-4272-958d-2ea103127445

📥 Commits

Reviewing files that changed from the base of the PR and between 89f73a5 and 4f7b546.

📒 Files selected for processing (1)
  • internal/engine/ghstack_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/engine/ghstack_test.go

📝 Walkthrough

Walkthrough

Adds GitHub Stack REST API integration, linear branch-segment detection, remote stack reconciliation, persisted stack numbers, and synchronization calls across submission flows after successful non-dry-run pushes.

Changes

GitHub Stack Synchronization

Layer / File(s) Summary
Stack API contract and lifecycle
internal/engine/ghstack.go, internal/store/pr_info.go
Defines GHStack and GHStackPR, authenticated gh api execution, stack lifecycle operations, and the optional BranchPR.StackNumber field.
Branch segmentation and reconciliation
internal/engine/ghstack.go, internal/engine/github.go, internal/engine/ghstack_test.go
Splits submitted graphs into linear segments, reconciles missing, closed, emptied, matching, appendable, and divergent remote stacks, retargets pull-request bases during rebuilds, and tests these cases.
Submission push tracking and synchronization
internal/engine/submit.go
Tracks successfully pushed branches across submission paths before synchronizing GitHub stacks; dry runs are excluded.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant SubmitFlow
  participant pushBranch
  participant syncGitHubStacks
  participant GitHub
  SubmitFlow->>pushBranch: push branch
  pushBranch-->>SubmitFlow: record successful non-dry-run branch
  SubmitFlow->>syncGitHubStacks: synchronize pushed branches
  syncGitHubStacks->>GitHub: reconcile GitHub stacks
  GitHub-->>syncGitHubStacks: stack response or error
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes registering GitHub stacks after submission, which is the main change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch gh-stacks-api

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
internal/engine/ghstack.go (1)

241-283: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

existing silently picks the first non-zero StackNumber in a segment.

If segment membership changes across submits (e.g., after a local fold/reorder), branches within one segment could carry different recorded StackNumbers, but only the first (bottommost) non-zero value is used as existing; any others are discarded without comment. In practice this is caught downstream by reconcileStack's divergence check (a mismatched remote top routes to resolveDivergedStack), so it's not a silent-corruption risk — but the resulting divergence warning won't mention that the local segment itself already disagreed about stack membership, making it harder to diagnose.

Consider detecting and surfacing this case explicitly for a clearer warning.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/engine/ghstack.go` around lines 241 - 283, Detect conflicting
non-zero StackNumber values while iterating branches in syncGitHubStacks, rather
than silently retaining only the first value in existing. Surface a clear
warning that the local segment has inconsistent stack membership, including the
affected branch sequence, and preserve the current reconciliation flow for
segments without conflicting values.
internal/engine/ghstack_test.go (1)

1-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

LGTM! Segmentation tests are thorough and correctly cover linear runs, forks, partial submits, and unsubmitted siblings.

Separately, worth noting reconcileStack/resolveDivergedStack (the merged-PR suffix matching and divergence detection) have no test coverage since ghStackAPI shells out directly to gh. Extracting it behind a small function-variable seam would let that logic be unit tested without a real gh binary.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/engine/ghstack_test.go` around lines 1 - 141, Add a small
function-variable seam around the ghstack API’s external gh invocation, then use
it from reconcileStack and resolveDivergedStack instead of shelling out
directly. Add unit tests through the seam covering merged-PR suffix matching and
divergence detection, without requiring a real gh binary.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/engine/ghstack.go`:
- Around line 150-155: Explicitly suppress the unused-symbol lint warning for
ghUnstack while leaving its current implementation unchanged. Use the
repository’s established Go lint-suppression convention directly on ghUnstack,
since resolveDivergedStack is not yet wired to call it.
- Around line 24-44: Update reconcileStack to treat an existing remote GHStack
as dissolved when either remote.Open is false or remote.PullRequests is empty,
and follow the existing dissolved-stack restart path instead of POSTing to
/stacks/{number}/add. Preserve current reconciliation behavior for open stacks
containing pull requests.

---

Nitpick comments:
In `@internal/engine/ghstack_test.go`:
- Around line 1-141: Add a small function-variable seam around the ghstack API’s
external gh invocation, then use it from reconcileStack and resolveDivergedStack
instead of shelling out directly. Add unit tests through the seam covering
merged-PR suffix matching and divergence detection, without requiring a real gh
binary.

In `@internal/engine/ghstack.go`:
- Around line 241-283: Detect conflicting non-zero StackNumber values while
iterating branches in syncGitHubStacks, rather than silently retaining only the
first value in existing. Surface a clear warning that the local segment has
inconsistent stack membership, including the affected branch sequence, and
preserve the current reconciliation flow for segments without conflicting
values.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8f59722e-e1ed-4fa4-a72f-75125c55cf8f

📥 Commits

Reviewing files that changed from the base of the PR and between be90128 and c009e14.

📒 Files selected for processing (4)
  • internal/engine/ghstack.go
  • internal/engine/ghstack_test.go
  • internal/engine/submit.go
  • internal/store/pr_info.go

Comment thread internal/engine/ghstack.go
Comment thread internal/engine/ghstack.go
amustafa added a commit that referenced this pull request Jul 31, 2026
A stack whose PRs have all merged is not deleted — it stays queryable
with open:false and an emptied member list. The nil check only caught
an explicit 404, so a finished stack fell through to POST /add against
a closed stack instead of starting a fresh one.

Extracts stackNeedsRebuild and prsAboveTop as pure functions so the
reconciliation decision is testable without the network, and names
GHStackPR instead of an inline anonymous struct.

Found by CodeRabbit on #22.
amustafa added a commit that referenced this pull request Jul 31, 2026
A stack whose PRs have all merged is not deleted — it stays queryable
with open:false and an emptied member list. The nil check only caught
an explicit 404, so a finished stack fell through to POST /add against
a closed stack instead of starting a fresh one.

Extracts stackNeedsRebuild and prsAboveTop as pure functions so the
reconciliation decision is testable without the network, and names
GHStackPR instead of an inline anonymous struct.

Found by CodeRabbit on #22.
amustafa added a commit that referenced this pull request Jul 31, 2026
A stack whose PRs have all merged is not deleted — it stays queryable
with open:false and an emptied member list. The nil check only caught
an explicit 404, so a finished stack fell through to POST /add against
a closed stack instead of starting a fresh one.

Extracts stackNeedsRebuild and prsAboveTop as pure functions so the
reconciliation decision is testable without the network, and names
GHStackPR instead of an inline anonymous struct.

Found by CodeRabbit on #22.
@amustafa
amustafa changed the base branch from main to am/sync-worktree-cleanup July 31, 2026 03:28
amustafa added a commit that referenced this pull request Jul 31, 2026
…nserted below the current bottom

Two bugs compounded to leave a restacked PR pointed at the wrong base
indefinitely, confirmed against the actual amustafa/stackr repo (PR #22
stayed based on main after am/sync-worktree-cleanup was inserted below it):

1. reconcileStack/prsAboveTop only ever checked whether remote's *top* PR
   appeared somewhere in the local segment, then appended whatever sat above
   it. It never verified the full remote sequence actually lined up, so a PR
   inserted *below* the stack's current bottom (e.g. a restack onto a new
   base branch) was indistinguishable from "nothing changed" — the old top
   was still there, so reconcileStack returned early and never touched
   GitHub. Replaced with classifyAgainstRemote, which locates remote's whole
   PR list as a contiguous run and reports what's below and above it
   separately.

2. Nothing ever synced an existing PR's base ref to GitHub after the local
   parent changed — pushBranch only force-pushed commits and updated
   stackr's own local cache. Added ghUpdatePRBase (a raw REST PATCH; `gh pr
   edit --base` fails outright via an unrelated GraphQL project-cards
   deprecation) and call it from pushBranch whenever the recorded base
   drifts from the local parent.

Those two don't compose for free: GitHub refuses to retarget a PR's base
while it's grouped into a stack, and refuses to create a stack containing a
PR that's already grouped into another one. So when classifyAgainstRemote's
newBelow PRs are still open (a real insertion, not just GitHub having
already dropped a merged one), reconcileStack now unstacks the old group,
retries the base retarget for every PR in the chain, and only then rebuilds.
Base automatically changed from am/sync-worktree-cleanup to main July 31, 2026 03:48
amustafa added 3 commits July 30, 2026 23:49
Adds the GitHub stacked-PRs REST integration (public preview,
X-GitHub-Api-Version 2026-03-10). stackr already creates each PR with
--base set to its parent branch, which is exactly the chain GitHub
validates, so registering a stack is one API call after submit.

- ghstack.go: create/get/add/unstack via 'gh api --input -'
- linearSegments: decompose stackr's branch tree into the maximal
  linear runs GitHub's model requires, cutting at every fork so each
  child starts a fresh stack based on the fork point
- syncGitHubStacks: best-effort, runs on every submit path, scoped to
  what was actually pushed; a repo without the preview warns instead
  of failing a submit whose PRs already exist
- BranchPR.StackNumber persists the stack so later submits extend it

The reconciliation policy for a stack that has diverged remotely is
left as a documented TODO in resolveDivergedStack.
A stack whose PRs have all merged is not deleted — it stays queryable
with open:false and an emptied member list. The nil check only caught
an explicit 404, so a finished stack fell through to POST /add against
a closed stack instead of starting a fresh one.

Extracts stackNeedsRebuild and prsAboveTop as pure functions so the
reconciliation decision is testable without the network, and names
GHStackPR instead of an inline anonymous struct.

Found by CodeRabbit on #22.
…nserted below the current bottom

Two bugs compounded to leave a restacked PR pointed at the wrong base
indefinitely, confirmed against the actual amustafa/stackr repo (PR #22
stayed based on main after am/sync-worktree-cleanup was inserted below it):

1. reconcileStack/prsAboveTop only ever checked whether remote's *top* PR
   appeared somewhere in the local segment, then appended whatever sat above
   it. It never verified the full remote sequence actually lined up, so a PR
   inserted *below* the stack's current bottom (e.g. a restack onto a new
   base branch) was indistinguishable from "nothing changed" — the old top
   was still there, so reconcileStack returned early and never touched
   GitHub. Replaced with classifyAgainstRemote, which locates remote's whole
   PR list as a contiguous run and reports what's below and above it
   separately.

2. Nothing ever synced an existing PR's base ref to GitHub after the local
   parent changed — pushBranch only force-pushed commits and updated
   stackr's own local cache. Added ghUpdatePRBase (a raw REST PATCH; `gh pr
   edit --base` fails outright via an unrelated GraphQL project-cards
   deprecation) and call it from pushBranch whenever the recorded base
   drifts from the local parent.

Those two don't compose for free: GitHub refuses to retarget a PR's base
while it's grouped into a stack, and refuses to create a stack containing a
PR that's already grouped into another one. So when classifyAgainstRemote's
newBelow PRs are still open (a real insertion, not just GitHub having
already dropped a merged one), reconcileStack now unstacks the old group,
retries the base retarget for every PR in the chain, and only then rebuilds.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (5)
internal/engine/ghstack.go (5)

224-231: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Bound the extension loop with claimed.

The inner loop follows child links without checking claimed. The segmentation logic is correct for a tree, but a cycle in the persisted graph makes this loop append forever and grow segment without bound. The graph is read from a state file, so a corrupted parent/child pair is reachable. Add the guard to keep the loop bounded.

🛡️ Proposed guard
 		for {
 			kids := childrenInSet(segment[len(segment)-1])
-			if len(kids) != 1 {
+			if len(kids) != 1 || claimed[kids[0]] {
 				break
 			}
 			segment = append(segment, kids[0])
 			claimed[kids[0]] = true
 		}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/engine/ghstack.go` around lines 224 - 231, Update the inner
extension loop in the segment-building logic to stop when the next child is
already present in claimed, before appending it. Preserve the existing
single-child condition and mark each newly appended child in claimed so cycles
in the persisted graph cannot grow segment indefinitely.

273-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Route the warning through the same output policy as the success line.

The success message at Line 286 respects quiet. This warning does not. The same applies to the warnings at Line 427 in reconcileStack. If quiet exists to keep scripted submits clean, these lines break that contract; if warnings are intentionally always visible, the success line is the inconsistent one. Pick one policy and apply it to both.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/engine/ghstack.go` around lines 273 - 277, Align the warning output
in the stack sync flow with the existing quiet-aware success output, using the
same output policy for both. Apply that policy consistently to the warning
messages in the surrounding sync logic and the warnings in reconcileStack,
preserving the current message content and error handling.

350-367: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

A single failed PR lookup aborts reconciliation.

anyPROpen returns on the first error. If one PR in newBelow is inaccessible or deleted, gh api returns 404, reconcileStack propagates the error, and syncGitHubStacks prints a warning without recording anything. A PR that GitHub cannot find is not open, so the safer reading is to skip it and keep checking.

The calls are also sequential with a 15s timeout each. newBelow is normally short, so this is acceptable today, but consider a note or a bound if segments can grow.

♻️ Proposed change
 	for _, n := range prNumbers {
 		out, err := ghStackAPI("GET", fmt.Sprintf("repos/{owner}/{repo}/pulls/%d", n), nil)
 		if err != nil {
-			return false, err
+			// A PR we cannot read is not an open PR blocking the rebuild.
+			if strings.Contains(err.Error(), "404") || strings.Contains(err.Error(), "Not Found") {
+				continue
+			}
+			return false, err
 		}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/engine/ghstack.go` around lines 350 - 367, Update anyPROpen to skip
inaccessible or deleted PR lookups instead of returning the first ghStackAPI
error, treating those PRs as not open and continuing through prNumbers. Preserve
JSON parsing errors and successful open-state detection, and consider
documenting or bounding the sequential lookup behavior if segment sizes may
grow.

457-470: 📐 Maintainability & Code Quality | 🔵 Trivial

resolveDivergedStack returns an error on every submit for a diverged stack.

The TODO(implement) leaves this path permanently failing. syncGitHubStacks downgrades the error to a warning, so submits still succeed, but the user sees the same message on every submit and has no way to clear it except editing the state file. The PR description acknowledges this.

Do you want me to open a tracking issue for the reconciliation policy, or draft the rebuild variant that clears the recorded StackNumber when ghCreateStack fails after ghUnstack?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/engine/ghstack.go` around lines 457 - 470, The unimplemented
resolveDivergedStack path must reconcile divergent stacks instead of returning
an error on every submit. Implement the chosen reconciliation policy in
resolveDivergedStack, including clearing the recorded stack number if unstacking
succeeds but ghCreateStack fails, and preserve correct state for successful
rebuilds; use the existing submit flow and GHStack helpers rather than leaving
the TODO error path.

96-102: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Include the exit error and make status detection explicit.

The error message contains only stderr. Two consequences:

  1. If gh writes nothing to stderr, the message ends with failed: and no cause remains.
  2. ghGetStack decides "stack no longer exists" by substring matching 404 or Not Found on this message (Line 127). That couples control flow to gh's human-readable stderr text.

Wrap the underlying error and return a typed status so callers do not parse prose.

♻️ Proposed change
+// ghAPIError carries the failed invocation so callers can branch on status
+// instead of matching stderr text.
+type ghAPIError struct {
+	Method, Path string
+	Stderr       string
+	Err          error
+}
+
+func (e *ghAPIError) Error() string {
+	return fmt.Sprintf("gh api %s %s failed: %s", e.Method, e.Path, e.Stderr)
+}
+
+func (e *ghAPIError) Unwrap() error { return e.Err }
+
+func (e *ghAPIError) notFound() bool {
+	return strings.Contains(e.Stderr, "HTTP 404")
+}
 	if err := cmd.Run(); err != nil {
 		if ctx.Err() != nil {
 			return nil, fmt.Errorf("gh api %s timed out after %s", path, ghTimeout)
 		}
-		return nil, fmt.Errorf("gh api %s %s failed: %s", method, path, strings.TrimSpace(stderr.String()))
+		return nil, &ghAPIError{
+			Method: method, Path: path,
+			Stderr: strings.TrimSpace(stderr.String()),
+			Err:    err,
+		}
 	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/engine/ghstack.go` around lines 96 - 102, Update the command
execution error path in ghGetStack’s underlying gh API helper to preserve the
cmd.Run exit error and expose an explicit typed status for callers, rather than
relying on formatted stderr text. Ensure the returned error retains stderr
context while wrapping the underlying error, and update ghGetStack’s “stack no
longer exists” handling to inspect the typed status instead of matching “404” or
“Not Found” in error prose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/engine/ghstack.go`:
- Around line 249-272: Update the segment scan around existing and
reconcileStack to collect all distinct non-zero StackNumber values instead of
retaining only the first one. When a segment spans multiple recorded stacks,
route it through the rebuild path by passing the designated multi-stack sentinel
(existing < 0), and update reconcileStack to dissolve every recorded stack
before creating the unified stack. Preserve the existing behavior for segments
belonging to zero or one recorded stack.
- Around line 291-303: Move the opening paragraph describing reconcileStack from
the comment above stackNeedsRebuild to the comment immediately preceding
reconcileStack. Keep the stackNeedsRebuild-specific explanation attached to
stackNeedsRebuild, and leave the baseByPR note with the reconcileStack
documentation as appropriate.
- Around line 381-383: Update the stackNeedsRebuild branch in the stack handling
flow to dissolve or unstack the existing remote stack before calling
ghCreateStack(prNumbers). Match the cleanup behavior used by the newBelow path,
then preserve creation of the replacement stack and registration flow.

---

Nitpick comments:
In `@internal/engine/ghstack.go`:
- Around line 224-231: Update the inner extension loop in the segment-building
logic to stop when the next child is already present in claimed, before
appending it. Preserve the existing single-child condition and mark each newly
appended child in claimed so cycles in the persisted graph cannot grow segment
indefinitely.
- Around line 273-277: Align the warning output in the stack sync flow with the
existing quiet-aware success output, using the same output policy for both.
Apply that policy consistently to the warning messages in the surrounding sync
logic and the warnings in reconcileStack, preserving the current message content
and error handling.
- Around line 350-367: Update anyPROpen to skip inaccessible or deleted PR
lookups instead of returning the first ghStackAPI error, treating those PRs as
not open and continuing through prNumbers. Preserve JSON parsing errors and
successful open-state detection, and consider documenting or bounding the
sequential lookup behavior if segment sizes may grow.
- Around line 457-470: The unimplemented resolveDivergedStack path must
reconcile divergent stacks instead of returning an error on every submit.
Implement the chosen reconciliation policy in resolveDivergedStack, including
clearing the recorded stack number if unstacking succeeds but ghCreateStack
fails, and preserve correct state for successful rebuilds; use the existing
submit flow and GHStack helpers rather than leaving the TODO error path.
- Around line 96-102: Update the command execution error path in ghGetStack’s
underlying gh API helper to preserve the cmd.Run exit error and expose an
explicit typed status for callers, rather than relying on formatted stderr text.
Ensure the returned error retains stderr context while wrapping the underlying
error, and update ghGetStack’s “stack no longer exists” handling to inspect the
typed status instead of matching “404” or “Not Found” in error prose.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ace0f69c-63a6-4d53-88ff-eccb896f89fa

📥 Commits

Reviewing files that changed from the base of the PR and between 7f5f5b7 and 472a439.

📒 Files selected for processing (5)
  • internal/engine/ghstack.go
  • internal/engine/ghstack_test.go
  • internal/engine/github.go
  • internal/engine/submit.go
  • internal/store/pr_info.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • internal/engine/ghstack_test.go
  • internal/store/pr_info.go
  • internal/engine/submit.go

Comment thread internal/engine/ghstack.go Outdated
Comment thread internal/engine/ghstack.go Outdated
Comment thread internal/engine/ghstack.go
Addresses three CodeRabbit findings on #22 that share one root cause: stackr
created a GitHub stack without first dissolving the stacks its PRs already
belonged to, and GitHub allows a PR in only one stack.

- The segment scan kept only the first non-zero StackNumber, so a segment
  spanning two recorded stacks picked one and failed /add forever.
- The stackNeedsRebuild path created without unstacking, but a closed stack
  retains its members until explicitly unstacked.
- reconcileStack's doc comment was attached to stackNeedsRebuild.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
internal/engine/ghstack_test.go (1)

228-238: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Assert the complete mapSegment result.

TestMapSegment_CollectsEveryRecordedStack checks only len(seg.prNumbers). TestMapSegment_SkipsBranchesWithoutAPR checks only seg.branches. A wrong PR-number mapping can pass both tests.

Assert seg.prNumbers equals []int{42, 43, 44} and []int{42, 44}. Also assert baseByPR[44] == "b" in the first test.

Also applies to: 248-256

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/engine/ghstack_test.go` around lines 228 - 238, Strengthen the
mapSegment assertions in TestMapSegment_CollectsEveryRecordedStack and
TestMapSegment_SkipsBranchesWithoutAPR: compare seg.prNumbers against the exact
expected slices []int{42, 43, 44} and []int{42, 44}, respectively, rather than
checking only their lengths. In the first test, also assert that
seg.baseByPR[44] equals "b".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/engine/ghstack_test.go`:
- Around line 284-298: Tighten isMissingStack so it recognizes only ghStackAPI
errors matching the failed unstack command shape and a 404/Not Found response,
rather than any message containing those strings. Preserve rejection of
unrelated 404 output and existing non-404 cases, and extend TestIsMissingStack
with coverage for an unrelated 404 response.

---

Nitpick comments:
In `@internal/engine/ghstack_test.go`:
- Around line 228-238: Strengthen the mapSegment assertions in
TestMapSegment_CollectsEveryRecordedStack and
TestMapSegment_SkipsBranchesWithoutAPR: compare seg.prNumbers against the exact
expected slices []int{42, 43, 44} and []int{42, 44}, respectively, rather than
checking only their lengths. In the first test, also assert that
seg.baseByPR[44] equals "b".
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6814d6a8-b6e8-453d-a44a-b78c71280977

📥 Commits

Reviewing files that changed from the base of the PR and between 472a439 and 89f73a5.

📒 Files selected for processing (2)
  • internal/engine/ghstack.go
  • internal/engine/ghstack_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/engine/ghstack.go

Comment thread internal/engine/ghstack_test.go
@amustafa
amustafa merged commit 56e1c37 into main Aug 1, 2026
1 check passed
@amustafa
amustafa deleted the gh-stacks-api branch August 1, 2026 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant