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

Ref: main   Size: 13.7 KiB   History

# Recording Merges — Design

Date: 2026-08-09
Status: Draft. Supersedes issues `9c7cc9ed` and `87246f9f` as a single design.
Independent of the `c65ac0e2` chain; buildable now.

## Purpose

`PatchStatus::Merged` is derived on every read and recorded nowhere.
`check_auto_merge` (`state.rs:464-486`) resolves the patch's branch and asks
whether its head is reachable from the base tip. Both halves fail in ordinary
use:

- **The branch is expected to be deleted.** Deleting a merged branch is routine
  housekeeping; afterwards `resolve_head` errors, the `let Ok(..) else` returns,
  and the patch silently reverts to `Open`. Demonstrated: five patches merged
  this session read `merged` only while the state cache was warm, and reverted on
  cache clear. There is no `patch merge` command, so the fact cannot be recorded.
- **Reachability cannot see a squash.** A squash merge puts a *new* commit with
  the same tree on the base branch; the patch's own commits are never ancestors
  of it. `graph_descendant_of` returns false permanently. The same applies to a
  rebase-merge. Anchoring detection to revision refs (`c65ac0e2`) fixes the first
  problem and not this one.

Derivation is the wrong mechanism. A merge must be recorded.

## Governing constraint

**Recording must never be a side effect of a read.**

This project already has one bug of exactly that shape: `auto_detect_revision`
appends a `PatchRevision` during `patch comment` and `patch review`, silently, at
a time unrelated to the code change (recorded on `c65ac0e2`). Making
`check_auto_merge` append during derivation would rebuild that bug with a wider
blast radius — `patch list` would mutate the DAG, read-only clones would break,
and two people running it concurrently would both append.

Detection therefore *suggests*. Only an explicit act records.

## Model

### The event

```rust
PatchMerge { commit: String }
```

`commit` is the commit on the base branch that constitutes the merge — the squash
commit, the merge commit, or the rebased tip. Recording which commit landed the
patch is worth more than a bare status flag: it is what the UI links to, and it
is the only durable evidence of *how* a squashed patch reached the branch.

Anyone may record. The event is signed and attributed like any other, and
`(clock, oid)` ordering makes concurrent recordings converge. Applying a
`PatchMerge` to a patch already `Merged` is a no-op, so duplicates are harmless.

### Three layers

Each degrades cleanly into the next, and each is independently useful:

1. A `commit-msg` hook stamps `Patch: <id>` into commits on a patch branch.
2. `sync` scans the base branch for those trailers and emits `PatchMerge`.
3. `git-collab patch merge <id>` records it by hand.

Layer 3 always works and is the only one that must exist for the design to be
correct. Layers 1 and 2 remove the need to remember it.

## Trailer scanning

This reuses the mechanism `Issue:` trailers already use. `commit_link.rs` parses
git trailers with correct block semantics — final paragraph only, every non-empty
line trailer-shaped (`commit_link.rs:28-112`) — and walks history during sync,
warning and continuing on every per-commit error so the scan can never break sync
(`commit_link.rs:141-149`).

Generalise it rather than copying it. `parse_issue_trailers` and
`match_issue_line` hardcode the token `issue`; both become token-parameterised, so
`Patch:` and `Issue:` share one parser and one set of tests. `is_trailer_shaped`
is already generic.

Rejecting values with interior whitespace (`commit_link.rs:128-133`) carries over
unchanged and matters more here: `Patch: abc merged by me` must parse to nothing,
visibly, rather than silently to `abc`.

### What is scanned

Not "every branch", as `scan_and_link` does. A `Patch:` trailer on some unrelated
branch is not a merge. For each open patch the trailer must appear on that
patch's own `base_ref`.

One revwalk per *distinct* base branch, not per patch — with a handful of patches
sharing `main`, per-patch walks would be O(patches × history) for no gain.
Collect `Patch:` trailers during the walk, then match against open patches.

The walk is bounded: stop at the oldest `base_commit` among open patches with
that base. Everything older cannot merge a currently-open patch, so walking it is
pure cost. With no open patches for a base branch, that base is not walked at all.

The bound must be an **ancestor of every** recorded base, and "oldest" is only
well defined when the bases are linearly ordered — which is the normal case but
not guaranteed. The obvious way to compute it is a trap: libgit2's
`git_merge_base_many` (git2's `merge_base_many`, and plain `git merge-base A B C`)
is *not* the octopus base. It computes `merge_base(oids[0], merge(oids[1..]))`, so
its result depends on argument order and is an ancestor of only *some* of the
inputs. On a linear chain A-B-C-D-E, `git merge-base A B D` is A but
`git merge-base B A D` is B — verified. Bounding with B hides A, and a merge
commit sitting between them is never seen: the scan then records nothing, silently,
for exactly the case layer 2 exists to serve. Fold binary `merge_base` over the
bases (or use octopus, which git2 0.19 does not expose). Tests must cover a
bound over **three** bases. Two is not enough: for two inputs
`git_merge_base_many` degenerates to the symmetric binary `merge_base` and is
correct, so no two-base test — unit or end to end — can tell the two
implementations apart.

### Why this survives a squash

`git merge --squash` concatenates the source commits' messages, so the trailer
comes along. Squashing five commits is fine: only the last one's trailer block
lands in the final paragraph, and all five carry the same patch id, so one
surviving trailer is sufficient. Rebase and cherry-pick preserve messages
outright.

It does not survive a hand-rewritten squash message. That is what layer 3 is for.

### Failure handling

Mirrors the `Issue:` scan exactly, because the reasoning is the same — a scan
must never break sync:

| Condition | Behaviour |
|---|---|
| Trailer resolves to no patch | Warn on stderr, continue |
| Trailer prefix is ambiguous | Warn, continue |
| Patch is archived | Warn and skip, as `commit_link.rs:205-211` does for issues |
| Patch already merged | Silent no-op |
| Commit unreadable | Warn, continue |
| Revwalk cannot start | Return `Err`; caller skips the scan for this sync |

The archived-patch row is **conditional on a walk happening at all**, and the
bounding rule above can prevent one. If an archived patch is the only patch on
its base branch, that base is not walked, so no trailer is read and no warning is
emitted. The optimisation is right and the warning is a courtesy, so the
optimisation wins — but the table overstates the guarantee and the tests should
encode the conditional behaviour rather than the absolute.

## Auto-closing the referenced issue

`--fixes` is documented as "auto-closes on merge" (`cli.rs:309`) and nothing
implements it. It could not have worked reliably before this design, because
merge itself was not reliably detectable.

The close is driven by an **invariant**, not by an emission: *a merged patch's
`fixes` issue is closed*. Anything that establishes or observes the merge works
to restore it.

An earlier draft specified this the other way — "emit the close only alongside a
`PatchMerge` that was actually emitted, never alongside one that was a no-op" —
and that is self-defeating. Closing writes to a different ref than the merge, so
the two are not atomic; if the close fails, the merge still stands, and on the
next pass the merge is *necessarily* a no-op. The retry the same section called
for was therefore unreachable by its own rule.

Stated as an invariant, both cases fall out:

- A freshly recorded merge closes its issue inline, in the same operation.
- A patch already merged with its `fixes` issue still open has the close retried.

Either way it is a write performed by an operation the user invoked — never
during derivation. That constraint is unchanged and applies with equal force.

### The invariant needs a reopen clause

Stated bare, the invariant is too strong. `issue reopen` sets the issue back to
open; the next scan sees a merged patch with an open `fixes` issue, cannot tell
that apart from a half-failure, and closes it again — every sync, forever, and
silently, since the retry pass does not warn. That makes `issue reopen`
unusable for any issue a merged patch names, which is a worse bug than the one
the retry fixes. Reopening a merged patch's issue is ordinary: the fix landed
and turned out to be wrong or incomplete.

The DAG already holds the distinguishing fact, so no new state is needed:

- A genuine half-failure has **no `IssueClose` attributable to this patch**. The
  close never happened.
- A deliberate reopen has one, followed by a later `IssueReopen`.

So the invariant is: *a merged patch's `fixes` issue is closed **unless it has
been closed for this patch once already and since reopened***. The retry
restores a close that never happened; it never overrides a human decision made
after one did.

Guards:

- Skip if the issue is already closed, so repeated scans append nothing.
- Skip if this patch's close already happened and was reopened — see above.
- Skip and warn if the issue does not resolve or is ambiguous.
- Both paths must be idempotent, since the retry pass runs on every scan.

## Stamping the trailer

Something must put `Patch: <id>` into the commit. Three sources, in order of how
much they cover:

**`patch create --stamp`** rewrites the branch's existing commits to carry the
trailer. Opt-in, because it rewrites history — but at create time nothing has
been reviewed yet, so it is the one safe moment to do it. Without this, a patch
made from commits that already exist has no trailers at all and layer 2 never
fires for it, which is the common case.

**A `commit-msg` hook** covers commits written afterward. Installed by
`git-collab init` (or an explicit `git-collab hooks install`), it appends
`Patch: <id>` when HEAD is on a branch with an open patch.

Two requirements that are easy to get wrong and unacceptable to get wrong:

- **Never clobber an existing `commit-msg` hook.** If one is present, chain to it
  and preserve its exit status, or refuse to install and say so. Silently
  replacing a user's hook is worse than not installing.
- **Idempotent.** A message already carrying the right `Patch:` trailer is left
  alone. Never append a second.

The hook is a convenience and must fail open: any error leaves the message
untouched and the commit proceeds.

**Manual.** Writing the trailer by hand always works, which is why the format is
a plain git trailer rather than anything requiring tooling.

Hooks are deliberately *not* the detection mechanism. `.git/hooks/` is not
cloned, so a hook only ever helps the machine it is installed on and only for
commits made after installation. Sync-time scanning works for everyone who syncs,
on any host, and retroactively — including for merges that happened before any of
this existed.

## Detection demoted to a hint

Reachability detection stays, and stops deciding anything. Where a patch is
reachable from its base tip but has no `PatchMerge`, surface it:

- `patch list` marks the patch `merged?` rather than `merged`.
- `sync` prints a closing line naming the patches that look merged and the
  command to record them.

The hint is knowingly partial — it cannot see a squash — so its absence must
never be read as "not merged". It never writes.

## Interaction with the revision-refs design

Independent, and it *reduces* what `c65ac0e2` has to do. That spec's "Merge
detection needs explicit rework" section exists because `check_auto_merge` reads
`base_commit` and silently no-ops on a missing branch. Once merged state is
recorded, that code is a hint generator rather than the source of truth, so
getting it wrong degrades a suggestion instead of losing history. The rework is
still worth doing; it stops being load-bearing.

## Testing

- A patch whose commits are squash-merged onto the base branch, with the trailer
  surviving, is recorded as merged by `sync`.
- The same patch with the squash message rewritten to drop the trailer is *not*
  auto-recorded, and `patch merge` records it.
- Merged state survives deleting the patch's branch, clearing
  `.git/collab/cache`, and a fresh clone that never had the branch — the case
  that fails today.
- A second `sync` after a recorded merge emits no further events.
- Two clones recording a merge concurrently converge to one merged patch on both.
- `--fixes` closes the referenced issue exactly once across repeated syncs.
- A `PatchMerge` whose `fixes` issue is already closed appends no `IssueClose`.
- An issue closed by a merge and then deliberately reopened stays open across
  repeated syncs.
- A merge recorded with the close half-failed is closed by the next scan.
- Three open patches on one base with distinct bases, the oldest one merged by a
  commit below the other two's bases, is still recorded — the multi-base bound
  case. Three, not two; see above.
- A `Patch:` trailer naming an unknown, ambiguous, or archived patch warns and
  leaves sync successful.
- `Patch: abc merged by me` parses to nothing.
- Trailers on a branch that is not the patch's `base_ref` do not record a merge.
- Installing the hook where a `commit-msg` hook already exists either chains to it
  or refuses; it never overwrites.
- The hook run twice on one message produces one trailer.
- A patch reachable from its base tip with no `PatchMerge` shows as `merged?` and
  no event is written by displaying it.

## Out of scope

- Detecting merges on a base branch the local clone does not have.
- Un-merging. A `PatchMerge` recorded in error is corrected by closing or by a
  future reopen event, not by deleting history.
- Changing how merges are performed. Merging remains plain git; this records that
  it happened.