a73x

ad439273

Design merge recording via Patch trailers

a73x   2026-08-09 17:30

Commit message
Design merge recording via Patch trailers

Merged state is derived on every read and stored nowhere, so it depends on
a branch that is expected to be deleted, and reachability cannot see a
squash merge at all. Records it as a PatchMerge event instead.

Detection is scanned at sync time from Patch: trailers, reusing the parser
and never-break-sync behaviour the Issue: trailer scan already has, rather
than from a client hook - hooks are not cloned and only ever help the
machine they are installed on. A commit-msg hook stamps the trailer, which
is the one job it is the right tool for.

Emitting the merge is also the moment --fixes can close its issue, which
could not work reliably while merge itself was underivable.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
docs/superpowers/specs/2026-08-09-merge-recording-design.md
Old New
@@ -0,0 +1,230 @@
1 # Recording Merges — Design
2
3 Date: 2026-08-09
4 Status: Draft. Supersedes issues `9c7cc9ed` and `87246f9f` as a single design.
5 Independent of the `c65ac0e2` chain; buildable now.
6
7 ## Purpose
8
9 `PatchStatus::Merged` is derived on every read and recorded nowhere.
10 `check_auto_merge` (`state.rs:464-486`) resolves the patch's branch and asks
11 whether its head is reachable from the base tip. Both halves fail in ordinary
12 use:
13
14 - **The branch is expected to be deleted.** Deleting a merged branch is routine
15 housekeeping; afterwards `resolve_head` errors, the `let Ok(..) else` returns,
16 and the patch silently reverts to `Open`. Demonstrated: five patches merged
17 this session read `merged` only while the state cache was warm, and reverted on
18 cache clear. There is no `patch merge` command, so the fact cannot be recorded.
19 - **Reachability cannot see a squash.** A squash merge puts a *new* commit with
20 the same tree on the base branch; the patch's own commits are never ancestors
21 of it. `graph_descendant_of` returns false permanently. The same applies to a
22 rebase-merge. Anchoring detection to revision refs (`c65ac0e2`) fixes the first
23 problem and not this one.
24
25 Derivation is the wrong mechanism. A merge must be recorded.
26
27 ## Governing constraint
28
29 **Recording must never be a side effect of a read.**
30
31 This project already has one bug of exactly that shape: `auto_detect_revision`
32 appends a `PatchRevision` during `patch comment` and `patch review`, silently, at
33 a time unrelated to the code change (recorded on `c65ac0e2`). Making
34 `check_auto_merge` append during derivation would rebuild that bug with a wider
35 blast radius — `patch list` would mutate the DAG, read-only clones would break,
36 and two people running it concurrently would both append.
37
38 Detection therefore *suggests*. Only an explicit act records.
39
40 ## Model
41
42 ### The event
43
44 ```rust
45 PatchMerge { commit: String }
46 ```
47
48 `commit` is the commit on the base branch that constitutes the merge — the squash
49 commit, the merge commit, or the rebased tip. Recording which commit landed the
50 patch is worth more than a bare status flag: it is what the UI links to, and it
51 is the only durable evidence of *how* a squashed patch reached the branch.
52
53 Anyone may record. The event is signed and attributed like any other, and
54 `(clock, oid)` ordering makes concurrent recordings converge. Applying a
55 `PatchMerge` to a patch already `Merged` is a no-op, so duplicates are harmless.
56
57 ### Three layers
58
59 Each degrades cleanly into the next, and each is independently useful:
60
61 1. A `commit-msg` hook stamps `Patch: <id>` into commits on a patch branch.
62 2. `sync` scans the base branch for those trailers and emits `PatchMerge`.
63 3. `git-collab patch merge <id>` records it by hand.
64
65 Layer 3 always works and is the only one that must exist for the design to be
66 correct. Layers 1 and 2 remove the need to remember it.
67
68 ## Trailer scanning
69
70 This reuses the mechanism `Issue:` trailers already use. `commit_link.rs` parses
71 git trailers with correct block semantics — final paragraph only, every non-empty
72 line trailer-shaped (`commit_link.rs:28-112`) — and walks history during sync,
73 warning and continuing on every per-commit error so the scan can never break sync
74 (`commit_link.rs:141-149`).
75
76 Generalise it rather than copying it. `parse_issue_trailers` and
77 `match_issue_line` hardcode the token `issue`; both become token-parameterised, so
78 `Patch:` and `Issue:` share one parser and one set of tests. `is_trailer_shaped`
79 is already generic.
80
81 Rejecting values with interior whitespace (`commit_link.rs:128-133`) carries over
82 unchanged and matters more here: `Patch: abc merged by me` must parse to nothing,
83 visibly, rather than silently to `abc`.
84
85 ### What is scanned
86
87 Not "every branch", as `scan_and_link` does. A `Patch:` trailer on some unrelated
88 branch is not a merge. For each open patch the trailer must appear on that
89 patch's own `base_ref`.
90
91 One revwalk per *distinct* base branch, not per patch — with a handful of patches
92 sharing `main`, per-patch walks would be O(patches × history) for no gain.
93 Collect `Patch:` trailers during the walk, then match against open patches.
94
95 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
97 pure cost. With no open patches for a base branch, that base is not walked at all.
98
99 ### Why this survives a squash
100
101 `git merge --squash` concatenates the source commits' messages, so the trailer
102 comes along. Squashing five commits is fine: only the last one's trailer block
103 lands in the final paragraph, and all five carry the same patch id, so one
104 surviving trailer is sufficient. Rebase and cherry-pick preserve messages
105 outright.
106
107 It does not survive a hand-rewritten squash message. That is what layer 3 is for.
108
109 ### Failure handling
110
111 Mirrors the `Issue:` scan exactly, because the reasoning is the same — a scan
112 must never break sync:
113
114 | Condition | Behaviour |
115 |---|---|
116 | Trailer resolves to no patch | Warn on stderr, continue |
117 | Trailer prefix is ambiguous | Warn, continue |
118 | Patch is archived | Warn and skip, as `commit_link.rs:205-211` does for issues |
119 | Patch already merged | Silent no-op |
120 | Commit unreadable | Warn, continue |
121 | Revwalk cannot start | Return `Err`; caller skips the scan for this sync |
122
123 ## Auto-closing the referenced issue
124
125 `--fixes` is documented as "auto-closes on merge" (`cli.rs:309`) and nothing
126 implements it. It could not have worked reliably before this design, because
127 merge itself was not reliably detectable.
128
129 When a `PatchMerge` is emitted, an `IssueClose` for the patch's `fixes` issue is
130 emitted **at the same moment, in the same operation**. Not during derivation —
131 that is the constraint above, and it applies with equal force here.
132
133 Guards:
134
135 - Skip if the issue is already closed, so re-running the scan appends nothing.
136 - Skip and warn if the issue does not resolve or is ambiguous.
137 - Emit the close only alongside a `PatchMerge` that was *actually* emitted, never
138 alongside one that was a no-op because the patch was already merged.
139
140 Closing writes to a different ref than the merge, so the two are not atomic. If
141 the close fails the merge still stands; the next scan sees a merged patch with an
142 open `fixes` issue and retries the close. Make that path explicit rather than
143 letting it fall out of the ordering.
144
145 ## Stamping the trailer
146
147 Something must put `Patch: <id>` into the commit. Three sources, in order of how
148 much they cover:
149
150 **`patch create --stamp`** rewrites the branch's existing commits to carry the
151 trailer. Opt-in, because it rewrites history — but at create time nothing has
152 been reviewed yet, so it is the one safe moment to do it. Without this, a patch
153 made from commits that already exist has no trailers at all and layer 2 never
154 fires for it, which is the common case.
155
156 **A `commit-msg` hook** covers commits written afterward. Installed by
157 `git-collab init` (or an explicit `git-collab hooks install`), it appends
158 `Patch: <id>` when HEAD is on a branch with an open patch.
159
160 Two requirements that are easy to get wrong and unacceptable to get wrong:
161
162 - **Never clobber an existing `commit-msg` hook.** If one is present, chain to it
163 and preserve its exit status, or refuse to install and say so. Silently
164 replacing a user's hook is worse than not installing.
165 - **Idempotent.** A message already carrying the right `Patch:` trailer is left
166 alone. Never append a second.
167
168 The hook is a convenience and must fail open: any error leaves the message
169 untouched and the commit proceeds.
170
171 **Manual.** Writing the trailer by hand always works, which is why the format is
172 a plain git trailer rather than anything requiring tooling.
173
174 Hooks are deliberately *not* the detection mechanism. `.git/hooks/` is not
175 cloned, so a hook only ever helps the machine it is installed on and only for
176 commits made after installation. Sync-time scanning works for everyone who syncs,
177 on any host, and retroactively — including for merges that happened before any of
178 this existed.
179
180 ## Detection demoted to a hint
181
182 Reachability detection stays, and stops deciding anything. Where a patch is
183 reachable from its base tip but has no `PatchMerge`, surface it:
184
185 - `patch list` marks the patch `merged?` rather than `merged`.
186 - `sync` prints a closing line naming the patches that look merged and the
187 command to record them.
188
189 The hint is knowingly partial — it cannot see a squash — so its absence must
190 never be read as "not merged". It never writes.
191
192 ## Interaction with the revision-refs design
193
194 Independent, and it *reduces* what `c65ac0e2` has to do. That spec's "Merge
195 detection needs explicit rework" section exists because `check_auto_merge` reads
196 `base_commit` and silently no-ops on a missing branch. Once merged state is
197 recorded, that code is a hint generator rather than the source of truth, so
198 getting it wrong degrades a suggestion instead of losing history. The rework is
199 still worth doing; it stops being load-bearing.
200
201 ## Testing
202
203 - A patch whose commits are squash-merged onto the base branch, with the trailer
204 surviving, is recorded as merged by `sync`.
205 - The same patch with the squash message rewritten to drop the trailer is *not*
206 auto-recorded, and `patch merge` records it.
207 - Merged state survives deleting the patch's branch, clearing
208 `.git/collab/cache`, and a fresh clone that never had the branch — the case
209 that fails today.
210 - A second `sync` after a recorded merge emits no further events.
211 - Two clones recording a merge concurrently converge to one merged patch on both.
212 - `--fixes` closes the referenced issue exactly once across repeated syncs.
213 - A `PatchMerge` whose `fixes` issue is already closed appends no `IssueClose`.
214 - A `Patch:` trailer naming an unknown, ambiguous, or archived patch warns and
215 leaves sync successful.
216 - `Patch: abc merged by me` parses to nothing.
217 - Trailers on a branch that is not the patch's `base_ref` do not record a merge.
218 - Installing the hook where a `commit-msg` hook already exists either chains to it
219 or refuses; it never overwrites.
220 - The hook run twice on one message produces one trailer.
221 - A patch reachable from its base tip with no `PatchMerge` shows as `merged?` and
222 no event is written by displaying it.
223
224 ## Out of scope
225
226 - Detecting merges on a base branch the local clone does not have.
227 - Un-merging. A `PatchMerge` recorded in error is corrected by closing or by a
228 future reopen event, not by deleting history.
229 - Changing how merges are performed. Merging remains plain git; this records that
230 it happened.