9b8f67f4

Correct the merge-recording spec: walk bound and reopen clause

a73x 2026-08-10 07:17

Commit message

Correct the merge-recording spec: walk bound and reopen clause

Two defects found in review of the implementation, both mine.

The walk bound said "stop at the oldest base_commit". That is only well
defined when the bases are linearly ordered, and the obvious way to
compute it is wrong: merge_base_many is not the octopus base, so its
result is order-dependent and an ancestor of only some of the inputs.
Bounding with it can hide the merge commit entirely, silently, for
exactly the case the scan exists to serve.

The --fixes invariant was stated bare, so it could not tell a
half-failed close from a deliberate `issue reopen`, and re-closed the
latter on every sync forever. The DAG already distinguishes them: a
half-failure has no IssueClose for this patch at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

docs/superpowers/specs/2026-08-09-merge-recording-design.md

Old		New
@@ -96,6 +96,19 @@ The walk is bounded: stop at the oldest `base_commit` among open patches with
96	that base. Everything older cannot merge a currently-open patch, so walking it is	96	that base. Everything older cannot merge a currently-open patch, so walking it is
97	pure cost. With no open patches for a base branch, that base is not walked at all.	97	pure cost. With no open patches for a base branch, that base is not walked at all.
98		98
		99	The bound must be an ancestor of every recorded base, and "oldest" is only
		100	well defined when the bases are linearly ordered — which is the normal case but
		101	not guaranteed. The obvious way to compute it is a trap: libgit2's
		102	`git_merge_base_many` (git2's `merge_base_many`, and plain `git merge-base A B C`)
		103	is not the octopus base. It computes `merge_base(oids[0], merge(oids[1..]))`, so
		104	its result depends on argument order and is an ancestor of only some of the
		105	inputs. On a linear chain A-B-C-D-E, `git merge-base A B D` is A but
		106	`git merge-base B A D` is B — verified. Bounding with B hides A, and a merge
		107	commit sitting between them is never seen: the scan then records nothing, silently,
		108	for exactly the case layer 2 exists to serve. Fold binary `merge_base` over the
		109	bases (or use octopus, which git2 0.19 does not expose). Tests must cover a
		110	multi-base bound, since a single-base test cannot distinguish the two.
		111
99	### Why this survives a squash	112	### Why this survives a squash
100		113
101	`git merge --squash` concatenates the source commits' messages, so the trailer	114	`git merge --squash` concatenates the source commits' messages, so the trailer
@@ -152,9 +165,31 @@ Stated as an invariant, both cases fall out:
152	Either way it is a write performed by an operation the user invoked — never	165	Either way it is a write performed by an operation the user invoked — never
153	during derivation. That constraint is unchanged and applies with equal force.	166	during derivation. That constraint is unchanged and applies with equal force.
154		167
		168	### The invariant needs a reopen clause
		169
		170	Stated bare, the invariant is too strong. `issue reopen` sets the issue back to
		171	open; the next scan sees a merged patch with an open `fixes` issue, cannot tell
		172	that apart from a half-failure, and closes it again — every sync, forever, and
		173	silently, since the retry pass does not warn. That makes `issue reopen`
		174	unusable for any issue a merged patch names, which is a worse bug than the one
		175	the retry fixes. Reopening a merged patch's issue is ordinary: the fix landed
		176	and turned out to be wrong or incomplete.
		177
		178	The DAG already holds the distinguishing fact, so no new state is needed:
		179
		180	- A genuine half-failure has no `IssueClose` attributable to this patch. The
		181	close never happened.
		182	- A deliberate reopen has one, followed by a later `IssueReopen`.
		183
		184	So the invariant is: a merged patch's `fixes` issue is closed *unless it has
		185	been closed for this patch once already and since reopened***. The retry
		186	restores a close that never happened; it never overrides a human decision made
		187	after one did.
		188
155	Guards:	189	Guards:
156		190
157	- Skip if the issue is already closed, so repeated scans append nothing.	191	- Skip if the issue is already closed, so repeated scans append nothing.
		192	- Skip if this patch's close already happened and was reopened — see above.
158	- Skip and warn if the issue does not resolve or is ambiguous.	193	- Skip and warn if the issue does not resolve or is ambiguous.
159	- Both paths must be idempotent, since the retry pass runs on every scan.	194	- Both paths must be idempotent, since the retry pass runs on every scan.
160		195
@@ -227,6 +262,11 @@ still worth doing; it stops being load-bearing.
227	- Two clones recording a merge concurrently converge to one merged patch on both.	262	- Two clones recording a merge concurrently converge to one merged patch on both.
228	- `--fixes` closes the referenced issue exactly once across repeated syncs.	263	- `--fixes` closes the referenced issue exactly once across repeated syncs.
229	- A `PatchMerge` whose `fixes` issue is already closed appends no `IssueClose`.	264	- A `PatchMerge` whose `fixes` issue is already closed appends no `IssueClose`.
		265	- An issue closed by a merge and then deliberately reopened stays open across
		266	repeated syncs.
		267	- A merge recorded with the close half-failed is closed by the next scan.
		268	- Two open patches on one base with unrelated bases, one merged by a commit
		269	older than the other's base, is still recorded — the multi-base bound case.
230	- A `Patch:` trailer naming an unknown, ambiguous, or archived patch warns and	270	- A `Patch:` trailer naming an unknown, ambiguous, or archived patch warns and
231	leaves sync successful.	271	leaves sync successful.
232	- `Patch: abc merged by me` parses to nothing.	272	- `Patch: abc merged by me` parses to nothing.