Skip to content

Commit 78ad1f5

Browse files
randleeclaude
andcommitted
fix(sc-gh-stack): unblock adopted-layer reconcile, bound rebases at fork points
Iteration-3 review pass (1 blocker, 2 majors, 8 minors): - convert: adopted-layer freshness also accepts a local tip that already contains every remote commit, so the documented reconcile recipe, re-runs after gh stack submit pushed the branches, and ff-then-rebase sequences all pass while post-adoption remote pushes are still refused (was an unrecoverable GIT.BRANCH_DIVERGED loop / false positive) - convert: when a layer descends from neither the below layer's recorded tip nor its current tip (cut from an older tip of the layer below), the rebase upstream falls back to the merge-base fork point instead of trunk, so the below layer's conflict-resolved commits are never re-replayed as spurious conflicts or duplicates - convert: failed fast-forwards report CONVERT.FF_FAILED with git stderr (worktree-checkout cause visible) instead of CONVERT.REBASE_FAILED - tests: 49 cases — new mutation-verified coverage for adopted-layer divergence + reconcile, post-submit idempotent re-run, fork-point bounding, and rerere-staged zero-unmerged conflicts - docs: empty conflict.files guidance (rerere), CONVERT.FF_FAILED, sanctioned scope of the keep-local troubleshooting path, submit-recovery conflict loop, STACK.INIT_FAILED split onto the failure path, test counts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 3558ad6 commit 78ad1f5

5 files changed

Lines changed: 146 additions & 21 deletions

File tree

packages/sc-gh-stack/CHANGELOG.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,11 @@ Initial package (skill-only, no agents).
1717
a different conversion starts, so dependent branches never replay a lower
1818
layer's commits and stale bookkeeping never leaks between conversions);
1919
stdlib-only, fenced JSON envelopes
20-
- `tests/`: 45 pytest cases — mocked unit tests plus real-git integration tests
21-
(conflict → resume, abort → re-run, dependent layers, trunk-merge
22-
linearisation, stale and diverged remotes, stale-bookkeeping clearing)
20+
- `tests/`: 49 pytest cases — mocked unit tests plus real-git integration tests
21+
(conflict → resume, abort → re-run, dependent layers, fork-point bounds,
22+
trunk-merge linearisation, stale and diverged remotes, adopted-layer
23+
divergence and reconcile, post-submit idempotency, rerere-staged conflict
24+
resumability, stale-bookkeeping clearing)
2325
- upstream `github/gh-stack` references (`commands.md`, `troubleshooting.md`,
2426
`stack-design.md`) carried verbatim for on-demand loading
2527
- `references/installation-and-troubleshooting.md` per guidelines v0.7

packages/sc-gh-stack/README.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,9 @@ Stdlib-only Python 3; every run emits one fenced JSON envelope (`success`/`data`
4848
python3 -m pytest packages/sc-gh-stack/tests
4949
```
5050

51-
Unit tests mock `git`/`gh`; one integration test drives real `git` in a temp repo
52-
(conflict → resume → linear chain) with a stubbed `gh` on PATH.
51+
Unit tests mock `git`/`gh`; the integration tests drive real `git` in temp repos
52+
(conflict → resume, abort → re-run, dependent layers, divergence guards) with a
53+
stubbed `gh` on PATH.
5354

5455
## Why stacks
5556

packages/sc-gh-stack/scripts/gh_stack_convert.py

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -103,10 +103,11 @@ def _has_merges(upstream: str, layer_ref: str, cwd: Optional[Path] = None) -> bo
103103
return gs.git_out(["rev-list", "--merges", f"{upstream}..{layer_ref}"], cwd=cwd) != ""
104104

105105

106-
def _fast_forward(branch: str, target: str, cwd: Optional[Path] = None) -> bool:
106+
def _fast_forward(branch: str, target: str, cwd: Optional[Path] = None):
107+
"""Returns the CompletedProcess of the attempted update (caller checks rc/stderr)."""
107108
if gs.git_out(["rev-parse", "--abbrev-ref", "HEAD"], cwd=cwd) == branch:
108-
return gs.git(["merge", "--ff-only", target], cwd=cwd).returncode == 0
109-
return gs.git(["branch", "-f", branch, target], cwd=cwd).returncode == 0
109+
return gs.git(["merge", "--ff-only", target], cwd=cwd)
110+
return gs.git(["branch", "-f", branch, target], cwd=cwd)
110111

111112

112113
def begin_conversion(trunk: str, layers: List[str], cwd: Optional[Path] = None) -> None:
@@ -140,16 +141,22 @@ def _check_remote_freshness(layer: str, remote: str, cwd: Optional[Path]) -> Opt
140141
orig = _orig_tip(layer, cwd=cwd)
141142
adopted = orig is not None and orig != _rev(_head(layer), cwd=cwd)
142143
if adopted:
143-
if gs.is_ancestor(remote_ref, orig, cwd=cwd):
144+
# Safe when the remote gained nothing since adoption (ancestor of the
145+
# recorded tip) OR the local tip already contains every remote commit —
146+
# the post-submit state, and the state after the documented reconcile
147+
# (`git rebase <remote>/<layer>`), both of which must pass.
148+
if gs.is_ancestor(remote_ref, orig, cwd=cwd) \
149+
or gs.is_ancestor(remote_ref, _head(layer), cwd=cwd):
144150
return None
145151
elif gs.is_ancestor(remote_ref, _head(layer), cwd=cwd):
146152
return None
147153
elif gs.is_ancestor(_head(layer), remote_ref, cwd=cwd):
148-
if _fast_forward(layer, remote_ref, cwd=cwd):
154+
ff = _fast_forward(layer, remote_ref, cwd=cwd)
155+
if ff.returncode == 0:
149156
return None
150-
return {"code": "CONVERT.REBASE_FAILED", "exit": EXIT_ERR, "layer": layer,
151-
"message": f"could not fast-forward {layer} to {remote_ref}",
152-
"action": "inspect the branch state and re-run"}
157+
return {"code": "CONVERT.FF_FAILED", "exit": EXIT_ERR, "layer": layer,
158+
"message": f"could not fast-forward {layer} to {remote_ref}: {ff.stderr.strip()}",
159+
"action": "inspect the branch state (is it checked out in another worktree?) and re-run"}
153160
return {"code": "GIT.BRANCH_DIVERGED", "exit": EXIT_INPUT, "layer": layer,
154161
"message": f"{layer} and {remote_ref} have diverged; converting the "
155162
f"local branch would drop the remote's commits on submit",
@@ -193,6 +200,14 @@ def chain(layers: List[str], trunk_ref: str, remote: str, cwd: Optional[Path] =
193200
upstream = orig_below
194201
elif gs.is_ancestor(below, layer_ref, cwd=cwd):
195202
upstream = below
203+
else:
204+
# Layer cut from an OLDER tip of the layer below: bound the
205+
# rebase at the fork point so the below layer's shared commits
206+
# are never re-replayed. Degenerates to the trunk fork point
207+
# (same replay set as trunk_ref) for layers cut from trunk.
208+
fork = gs.git_out(["merge-base", orig_below or below, layer_ref], cwd=cwd)
209+
if fork:
210+
upstream = fork
196211
if _orig_tip(layer, cwd=cwd) is None:
197212
gs.git(["update-ref", _orig_ref(layer), layer_ref], cwd=cwd)
198213

packages/sc-gh-stack/skills/managing-gh-stacks/references/playbook-convert.md

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,10 @@ merge commits — verify no conflict-resolution content from an "evil merge" was
3939
continuing). It stops at the **first** conflict. Every run emits one fenced JSON envelope;
4040
read `success`, `error.code`, and `data`. The sample payloads below show the fields to act
4141
on; `data` also always carries `trunk`, `remote`, and `layers`. Exit 1 with
42-
`CONVERT.REBASE_FAILED` means a rebase or fast-forward failed without leaving a resumable
43-
rebase — read `error.message`, fix that (never re-chain by hand), and re-run; finished layers
44-
are skipped.
42+
`CONVERT.REBASE_FAILED` (a rebase failed without leaving a resumable rebase) or
43+
`CONVERT.FF_FAILED` (a fast-forward failed — often the branch is checked out in another
44+
worktree) — read `error.message`, fix that (never re-chain by hand), and re-run; finished
45+
layers are skipped.
4546

4647
### On exit 3 — `error.code: "CONVERT.CONFLICT"`
4748

@@ -65,7 +66,9 @@ git rebase --continue
6566
python3 .claude/scripts/gh_stack_convert.py main 101 102 103 104 # re-run: finished layers report "skip"
6667
```
6768

68-
Every conflict is attributed to a specific layer. If `git rebase --continue` conflicts again
69+
Every conflict is attributed to a specific layer. If `conflict.files` is empty, rerere has
70+
already staged every resolution — run `git rebase --continue` directly, then re-run the
71+
script. If `git rebase --continue` conflicts again
6972
(a layer with several conflicting commits), repeat resolve + `git add` + `--continue` until
7073
the rebase itself finishes; only then re-run the script — run mid-rebase it refuses with
7174
`GIT.REBASE_IN_PROGRESS`. Loop until `success: true`. rerere records each resolution, so the
@@ -75,9 +78,13 @@ same conflict never needs a second manual resolution when the stack is rebased a
7578

7679
`data.stack_init.action` is `"initialised"` (or `"existing_stack_kept"` if a local stack was
7780
already present — if the reported branches differ from your list, run `gh stack unstack --local`
78-
and re-run the script). On exit 1 with `error.code: "STACK.INIT_FAILED"`, read
79-
`data.stack_init.stderr`, fix the reported problem, and re-run — chained layers are skipped.
80-
Nothing has been pushed yet. Now inspect the stack:
81+
and re-run the script).
82+
83+
On exit 1 with `error.code: "STACK.INIT_FAILED"`, read `data.stack_init.stderr`, fix the
84+
reported problem, and re-run — chained layers are skipped; do not run `view` or `submit`
85+
until the re-run succeeds.
86+
87+
On success: nothing has been pushed yet. Now inspect the stack:
8188

8289
```bash
8390
gh stack view --json
@@ -117,8 +124,15 @@ gh stack rebase --upstack # propagate through the layers above
117124
gh stack submit --auto
118125
```
119126

127+
If that `git rebase <remote>/<rejected-layer>` itself conflicts, resolve + `git add` +
128+
`git rebase --continue` exactly as in the conversion loop (rerere replays earlier
129+
resolutions); never `--abort` into a force-push. Duplicated lower-layer commits are dropped
130+
by the following `gh stack rebase --upstack`.
131+
120132
Never resolve a rejected push with `git push --force`, and never pick "keep the local
121-
version" from a divergence prompt — the remote-only commits are someone's work.
133+
version" when branch **content** has diverged (a rejected push) — the remote-only commits
134+
are someone's work. The keep-local path in `troubleshooting.md` applies only to
135+
stack-**grouping** divergence, where no commits differ.
122136
Exit **9** means stacked PRs are not enabled on the repository — stop and tell the user,
123137
reporting which layers (if any) were already pushed per the submit output; the local branches
124138
remain chained and the stack tracked — do not attempt to undo that without the user.

packages/sc-gh-stack/tests/test_convert.py

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,33 @@ def fake_git(args, cwd=None):
115115
assert env["data"]["failure"]["layer"] == "a"
116116
assert "boom" in env["data"]["failure"]["message"]
117117

118+
def test_rerere_staged_conflict_still_resumable(self, monkeypatch):
119+
"""rebase stops nonzero, rebase-in-progress, but rerere.autoUpdate staged
120+
every resolution (zero unmerged paths): must classify as a resumable
121+
conflict, never CONVERT.REBASE_FAILED."""
122+
state = {"rebasing": False}
123+
124+
def fake_git(args, cwd=None):
125+
if args[0] == "rebase":
126+
state["rebasing"] = True
127+
return cp(1, "", "stopped; resolutions staged by rerere")
128+
return cp(0)
129+
130+
monkeypatch.setattr(gs, "rebase_in_progress", lambda cwd=None: state["rebasing"])
131+
monkeypatch.setattr(gs, "working_tree_clean", lambda cwd=None: True)
132+
monkeypatch.setattr(gs, "remotes", lambda cwd=None: ["origin"])
133+
monkeypatch.setattr(gs, "git", fake_git)
134+
monkeypatch.setattr(gs, "git_out", lambda args, cwd=None: "")
135+
monkeypatch.setattr(gs, "local_branch_exists", lambda b, cwd=None: True)
136+
monkeypatch.setattr(gs, "remote_branch_exists",
137+
lambda remote, branch, cwd=None: branch == "main")
138+
monkeypatch.setattr(gs, "is_ancestor", lambda a, b, cwd=None: False)
139+
monkeypatch.setattr(gs, "conflicted_files", lambda cwd=None: [])
140+
code, env = cv.convert("main", ["a", "b"])
141+
assert code == cv.EXIT_CONFLICT
142+
assert env["error"]["code"] == "CONVERT.CONFLICT"
143+
assert env["data"]["conflict"] == {"layer": "a", "onto": "origin/main", "files": []}
144+
118145
def test_init_stack_keeps_existing_stack(self):
119146
with patch.object(gs, "git", return_value=cp(0)), patch.object(gs, "gh", return_value=cp(0, "{}")):
120147
assert cv.init_stack("main", ["a", "b"])["action"] == "existing_stack_kept"
@@ -362,6 +389,72 @@ def test_abort_then_rerun_converges(self, repo):
362389
code, env = cv.convert("main", ["pr1", "pr2", "pr3"], cwd=repo)
363390
assert code == cv.EXIT_OK, env
364391

392+
def test_adopted_layer_remote_advance_refused(self, repo):
393+
"""After a layer was rebased by this conversion, a commit someone pushes
394+
to its remote must still be detected (via the recorded pre-rebase tip),
395+
even though plain ancestry against the rewritten branch means nothing."""
396+
old_tip = _sh(repo, "git", "rev-parse", "pr3").strip()
397+
code, env = cv.convert("main", ["pr1", "pr3"], cwd=repo)
398+
assert code == cv.EXIT_OK, env
399+
# Collaborator pushes to origin/pr3, building on the pre-conversion tip.
400+
_sh(repo, "git", "checkout", "-qb", "collab", old_tip)
401+
_commit(repo, "collab.txt", "c\n", "collab work")
402+
_sh(repo, "git", "push", "-q", "origin", "collab:pr3")
403+
_sh(repo, "git", "checkout", "-q", "main")
404+
code, env = cv.convert("main", ["pr1", "pr3"], cwd=repo)
405+
assert code == cv.EXIT_INPUT
406+
assert env["error"]["code"] == "GIT.BRANCH_DIVERGED"
407+
assert "pr3" in env["error"]["message"]
408+
409+
# The error's own reconcile recipe must clear the guard on re-run.
410+
_sh(repo, "git", "checkout", "-q", "pr3")
411+
_sh(repo, "git", "rebase", "-q", "origin/pr3")
412+
_sh(repo, "git", "checkout", "-q", "main")
413+
code, env = cv.convert("main", ["pr1", "pr3"], cwd=repo)
414+
assert code == cv.EXIT_OK, env
415+
assert "collab work" in _sh(repo, "git", "log", "--format=%s", "origin/main..pr3")
416+
417+
def test_rerun_after_submit_push_is_idempotent(self, repo):
418+
code, env = cv.convert("main", ["pr1", "pr3"], cwd=repo)
419+
assert code == cv.EXIT_OK, env
420+
# Simulate `gh stack submit` pushing the rebased branches.
421+
_sh(repo, "git", "push", "-q", "-f", "origin", "pr1", "pr3")
422+
code, env = cv.convert("main", ["pr1", "pr3"], cwd=repo)
423+
assert code == cv.EXIT_OK, env
424+
assert all(c["action"] == "skip" for c in env["data"]["chained"])
425+
426+
def test_layer_cut_from_older_tip_of_below_not_duplicated(self, repo):
427+
# dep is branched from pr1's current tip (contains the shared.txt
428+
# commit), then pr1 gains one MORE commit — the everyday "lower layer
429+
# kept moving after the upper layer was cut" shape.
430+
_sh(repo, "git", "checkout", "-qb", "dep", "pr1")
431+
_commit(repo, "dep.txt", "dep\n", "dep work")
432+
_sh(repo, "git", "push", "-q", "origin", "dep")
433+
_sh(repo, "git", "checkout", "-q", "pr1")
434+
_commit(repo, "l1b.txt", "more\n", "pr1 more work")
435+
_sh(repo, "git", "push", "-q", "-f", "origin", "pr1")
436+
# Trunk moves, conflicting with pr1's shared-file edit.
437+
_sh(repo, "git", "checkout", "-q", "main")
438+
_commit(repo, "shared.txt", "trunk\n", "trunk shared")
439+
_sh(repo, "git", "push", "-q", "origin", "main")
440+
441+
code, env = cv.convert("main", ["pr1", "dep"], cwd=repo)
442+
assert code == cv.EXIT_CONFLICT
443+
assert env["data"]["conflict"]["layer"] == "pr1"
444+
(repo / "shared.txt").write_text("resolved\n")
445+
_sh(repo, "git", "add", "shared.txt")
446+
_sh(repo, "git", "rebase", "--continue")
447+
448+
# Re-run: dep descends from NEITHER pr1's recorded pre-rebase tip
449+
# (which includes the later commit) NOR its current tip, so the
450+
# upstream bound must fall back to the fork point. Only dep's own
451+
# commit replays — pr1's conflict-resolved shared commit (whose patch
452+
# changed) is never re-replayed, so no spurious conflict, no duplicate.
453+
code, env = cv.convert("main", ["pr1", "dep"], cwd=repo)
454+
assert code == cv.EXIT_OK, env
455+
assert _sh(repo, "git", "rev-list", "--count", "pr1..dep").strip() == "1"
456+
assert (repo / "shared.txt").read_text() == "resolved\n"
457+
365458
def test_new_conversion_clears_stale_orig_refs(self, repo):
366459
code, env = cv.convert("main", ["pr1", "pr3"], cwd=repo)
367460
assert code == cv.EXIT_OK, env

0 commit comments

Comments
 (0)