f3f0d57b

Record merges as events instead of deriving them on read

a73x 2026-08-10 07:30

Commit message

Record merges as events instead of deriving them on read

`PatchStatus::Merged` was derived on every read, by asking whether the
patch's head was reachable from its base tip. Both halves fail in
ordinary use: deleting the merged branch is routine housekeeping and
sent the patch back to Open the moment the state cache went cold, and
reachability cannot see a squash merge at all -- a squash puts a new
commit with the same tree on the base branch, so the patch's own commits
are never ancestors of it.

Merged state is now an event. `Action::PatchMerge` gains the commit that
landed the patch, which is what the UI links to and the only durable
evidence of how a squashed patch reached the branch.

Two of the design's three layers land here; stamping (the commit-msg
hook and `patch create --stamp`) is a separate patch, and this is
correct without it -- the trailer can be written by hand.

- `git-collab patch merge <id> [--commit <rev>] [--no-close]` records a
  merge by hand. Always works, and is the only layer that must exist for
  the design to be correct.
- `sync` scans each open patch's own base branch for `Patch:` trailers
  and records the merges they name. One revwalk per distinct base
  branch, bounded by the merge-base of the open patches' recorded bases
  -- an ancestor of every one of them, so it can never hide a commit we
  needed, and the oldest of them whenever they are linearly ordered.
  Warn-and-continue on every per-commit and per-patch error, exactly as
  the `Issue:` scan does, so a scan can never break sync.
- `--fixes` closes its issue in the same operation that emits the merge.

The trailer parser is now token-parameterised and lives in `trailer`, so
`Issue:` and `Patch:` share one parser and one set of tests. Rejecting
interior whitespace carries over and matters more here: `Patch: abc
merged by me` parses to nothing, visibly, rather than silently to `abc`.

Recording is never a side effect of a read. Reachability detection stays
as a hint that decides nothing and writes nothing: `patch list` and
`patch show` render it `merged?`, and `sync` closes by naming the
patches that look merged and the command that records them.

The close writes to a different ref than the merge, so the two are not
atomic. If the close does not land, the merge stands, and a later scan
retries it -- which is why the close is driven by the invariant "a
merged patch's fixes issue is closed" rather than by the emission alone.
A rule that only fired alongside a freshly-emitted `PatchMerge` could
never retry, because on the retry pass the merge itself is a no-op.
Idempotent throughout: an already-closed issue is left alone, so
repeated syncs append nothing.

Cache format bumped to v5: a v4 entry can hold a `merged` status derived
from reachability, which no event backs and nothing recomputes any more.

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

README.md

Old		New
@@ -47,9 +47,24 @@ carries the patch and every revision it ever had. Contributing needs no `git pus
47	write access to `refs/heads/*`, and rebasing your branch cannot strip an earlier	47	write access to `refs/heads/*`, and rebasing your branch cannot strip an earlier
48	revision of the objects a reviewer is looking at.	48	revision of the objects a reviewer is looking at.
49		49
50	Merging is not a command. When a patch's commits become reachable from the base	50	Merge however you already merge — merging stays plain git. That a patch merged
51	branch, `git-collab` notices and marks the patch merged. Merge however you	51	is recorded, though, rather than guessed at on every read: a merged branch is
52	already merge.	52	routinely deleted, and no amount of reachability can see a squash, whose commit
		53	shares nothing with the patch's own.
		54
		55	Put `Patch: <id>` in the commit that lands the patch and `git-collab sync` finds
		56	it on the patch's base branch and records the merge — which works retroactively,
		57	on any machine, and survives a squash, because `git merge --squash` carries the
		58	message along. Or record it by hand:
		59
		60	```console
		61	$ git-collab patch merge a1b2c3d4
		62	```
		63
		64	Either way, a patch created with `--fixes` closes the issue it fixes at the same
		65	moment. A patch whose commits are simply reachable from its base tip is shown as
		66	`merged?` — a hint, and one that can only ever be a hint, so it decides nothing
		67	and writes nothing.
53		68
54	## Install	69	## Install
55		70
@@ -98,7 +113,7 @@ than type.
98	\| \| \|	113	\| \| \|
99	\|---\|---\|	114	\|---\|---\|
100	\| `issue` \| open, list, show, comment, edit, label, assign, close \|	115	\| `issue` \| open, list, show, comment, edit, label, assign, close \|
101	\| `patch` \| create, list, show, diff, comment, review, revise, log, checkout, close \|	116	\| `patch` \| create, list, show, diff, comment, review, revise, log, checkout, merge, close \|
102	\| `sync` \| fetch, reconcile and push collab refs \|	117	\| `sync` \| fetch, reconcile and push collab refs \|
103	\| `status` \| project overview \|	118	\| `status` \| project overview \|
104	\| `dashboard` \| interactive TUI \|	119	\| `dashboard` \| interactive TUI \|

src/cache.rs

Old		New
@@ -22,8 +22,12 @@ fn sanitize_ref_name(ref_name: &str) -> String {
22	/// single optional value. v4: `PatchState` gained a `labels` field; a	22	/// single optional value. v4: `PatchState` gained a `labels` field; a
23	/// stale v3 entry would deserialize fine (the field is `#[serde(default)]`)	23	/// stale v3 entry would deserialize fine (the field is `#[serde(default)]`)
24	/// but silently omit any labels folded in since, so it must be rejected by	24	/// but silently omit any labels folded in since, so it must be rejected by
25	/// version rather than relying on that default to catch it.	25	/// version rather than relying on that default to catch it. v5: `PatchStatus`
26	const CACHE_FORMAT_VERSION: u32 = 4;	26	/// is folded from the DAG alone. A v4 entry could hold `merged` derived from
		27	/// reachability against a base branch — a verdict no event backs and which
		28	/// nothing recomputes any more, so it would be served as recorded fact
		29	/// forever. The shape is unchanged, so only the version can reject it.
		30	const CACHE_FORMAT_VERSION: u32 = 5;
27		31
28	/// Cache entry stored on disk: the tip OID at cache time + serialized state.	32	/// Cache entry stored on disk: the tip OID at cache time + serialized state.
29	#[derive(serde::Serialize, serde::Deserialize)]	33	#[derive(serde::Serialize, serde::Deserialize)]

src/cli.rs

Old		New
@@ -467,6 +467,22 @@ pub enum PatchCmd {
467	/// Label to remove	467	/// Label to remove
468	label: String,	468	label: String,
469	},	469	},
		470	/// Record that a patch was merged into its base branch
		471	///
		472	/// Merging stays plain git; this records that it happened. Use it whenever
		473	/// the merge is not visible to `sync`'s `Patch:` trailer scan — a squash
		474	/// whose message was rewritten, a merge made before the trailer existed, or
		475	/// a base branch this clone does not have.
		476	Merge {
		477	/// Patch ID (prefix match)
		478	id: String,
		479	/// The commit that landed the patch (default: the base branch tip)
		480	#[arg(long)]
		481	commit: Option<String>,
		482	/// Record the merge without closing the issue named by --fixes
		483	#[arg(long)]
		484	no_close: bool,
		485	},
470	/// Close a patch	486	/// Close a patch
471	Close {	487	Close {
472	/// Patch ID (prefix match)	488	/// Patch ID (prefix match)
@@ -513,6 +529,7 @@ impl Commands {
513	\| PatchCmd::Label { .. }	529	\| PatchCmd::Label { .. }
514	\| PatchCmd::Unlabel { .. }	530	\| PatchCmd::Unlabel { .. }
515	\| PatchCmd::Close { .. }	531	\| PatchCmd::Close { .. }
		532	\| PatchCmd::Merge { .. }
516	),	533	),
517	_ => false,	534	_ => false,
518	}	535	}

src/commit_link.rs

Old		New
@@ -27,111 +27,10 @@ pub fn collect_linked_shas(repo: &Repository, issue_ref: &str) -> Result<HashSet
27		27
28	/// Parse `Issue:` trailers from a commit message.	28	/// Parse `Issue:` trailers from a commit message.
29	///	29	///
30	/// Returns the list of trailer values in order of appearance. Follows git's	30	/// A thin naming of the shared parser in [`crate::trailer`], which `Patch:`
31	/// own trailer-block semantics: only the final paragraph is considered, and	31	/// trailers use too.
32	/// every non-empty line in it must be trailer-shaped (a `token: value`
33	/// line) for the paragraph to qualify. Any prose line in the final paragraph
34	/// disqualifies the whole paragraph — this prevents false positives like
35	/// `"Thanks Bob.\nIssue: abc"` in commit bodies.
36	///
37	/// The key match is `(?i)issue`; the value must be a single non-whitespace
38	/// token followed by optional trailing whitespace and end-of-line. Values
39	/// like `abc fixes thing` are rejected so that loose commentary never
40	/// becomes a silent issue-prefix lookup that warns every sync forever.
41	pub fn parse_issue_trailers(message: &str) -> Vec<String> {	32	pub fn parse_issue_trailers(message: &str) -> Vec<String> {
42	// 1. Split into paragraphs (blank-line separated), preserving order.	33	crate::trailer::parse_trailers(message, crate::trailer::ISSUE_TOKEN)
43	// Trim trailing whitespace from each line for the trailer-shape check,
44	// but keep enough structure to recognize blank lines.
45	let lines: Vec<&str> = message.lines().collect();
46
47	// 2. Find the last paragraph: the longest tail slice that contains at
48	// least one non-empty line and has no blank line before its first
49	// non-empty line in the tail.
50	//
51	// Walking from the end: skip trailing blank/whitespace-only lines,
52	// then collect lines until we hit a blank line.
53	let mut end = lines.len();
54	while end > 0 && lines[end - 1].trim().is_empty() {
55	end -= 1;
56	}
57	if end == 0 {
58	return Vec::new();
59	}
60	let mut start = end;
61	while start > 0 && !lines[start - 1].trim().is_empty() {
62	start -= 1;
63	}
64	let paragraph = &lines[start..end];
65
66	// 3. Validate every non-empty line in the paragraph is trailer-shaped.
67	for line in paragraph {
68	if line.trim().is_empty() {
69	continue;
70	}
71	if !is_trailer_shaped(line) {
72	return Vec::new();
73	}
74	}
75
76	// 4. Extract `Issue:` values.
77	let mut out = Vec::new();
78	for line in paragraph {
79	if let Some(value) = match_issue_line(line) {
80	out.push(value);
81	}
82	}
83	out
84	}
85
86	/// Returns true if a line looks like a git trailer: `<token>: <value>`, where
87	/// token starts with a letter and consists of `[A-Za-z0-9-]`, and value is at
88	/// least one non-whitespace character.
89	fn is_trailer_shaped(line: &str) -> bool {
90	let trimmed = line.trim_start();
91	let Some(colon_pos) = trimmed.find(':') else {
92	return false;
93	};
94	// Use trim_end() so that `ISSUE : abc` is recognized as the token `ISSUE`
95	// — matching what `match_issue_line` does. Without this, the space before
96	// the colon would disqualify the line and make the whole paragraph fail
97	// the trailer-shape check.
98	let token = trimmed[..colon_pos].trim_end();
99	if token.is_empty() {
100	return false;
101	}
102	let mut chars = token.chars();
103	let first = chars.next().unwrap();
104	if !first.is_ascii_alphabetic() {
105	return false;
106	}
107	if !chars.all(\|c\| c.is_ascii_alphanumeric() \|\| c == '-') {
108	return false;
109	}
110	let value = trimmed[colon_pos + 1..].trim();
111	!value.is_empty()
112	}
113
114	/// If `line` is an `Issue: <token>` trailer with exactly one non-whitespace
115	/// token in its value, returns the token. Otherwise returns None.
116	fn match_issue_line(line: &str) -> Option<String> {
117	let trimmed = line.trim_start();
118	let colon_pos = trimmed.find(':')?;
119	let key = trimmed[..colon_pos].trim_end();
120	if !key.eq_ignore_ascii_case("issue") {
121	return None;
122	}
123	let value_region = &trimmed[colon_pos + 1..];
124	let value = value_region.trim();
125	if value.is_empty() {
126	return None;
127	}
128	// Reject values with interior whitespace: `abc fixes thing` must not
129	// parse to `abc` silently — it must parse to nothing so the user sees
130	// that their commentary is being ignored.
131	if value.split_whitespace().count() != 1 {
132	return None;
133	}
134	Some(value.to_string())
135	}	34	}
136		35
137	const ACTIVE_ISSUE_PREFIX: &str = "refs/collab/issues/";	36	const ACTIVE_ISSUE_PREFIX: &str = "refs/collab/issues/";

src/dag.rs

Old		New
@@ -337,7 +337,7 @@ fn commit_message(action: &Action) -> String {
337	format!("patch: inline comment on {}:{}", file, line)	337	format!("patch: inline comment on {}:{}", file, line)
338	}	338	}
339	Action::PatchClose { .. } => "patch: close".to_string(),	339	Action::PatchClose { .. } => "patch: close".to_string(),
340	Action::PatchMerge => "patch: merge".to_string(),	340	Action::PatchMerge { .. } => "patch: merge".to_string(),
341	Action::Merge => "collab: merge".to_string(),	341	Action::Merge => "collab: merge".to_string(),
342	}	342	}
343	}	343	}

src/event.rs

Old		New
@@ -136,8 +136,24 @@ pub enum Action {
136	},	136	},
137	#[serde(rename = "patch.close")]	137	#[serde(rename = "patch.close")]
138	PatchClose { reason: Option<String> },	138	PatchClose { reason: Option<String> },
		139	/// A patch landed on its base branch. Recorded, never derived: reachability
		140	/// cannot see a squash merge and cannot survive the merged branch being
		141	/// deleted, so merged state has to be an event like any other.
		142	///
		143	/// `commit` is the commit on the base branch that constitutes the merge —
		144	/// the squash commit, the merge commit, or the rebased tip. It is the only
		145	/// durable evidence of how a squashed patch reached the branch.
		146	///
		147	/// Events written before the field existed carry no `commit`, so it
		148	/// defaults to empty and an empty one is skipped on the way out: signatures
		149	/// are verified by re-serializing, and writing back a field the signer
		150	/// never wrote would invalidate every one of them. An empty `commit` means
		151	/// "not recorded", never "the null OID".
139	#[serde(rename = "patch.merge")]	152	#[serde(rename = "patch.merge")]
140	PatchMerge,	153	PatchMerge {
		154	#[serde(default, skip_serializing_if = "String::is_empty")]
		155	commit: String,
		156	},
141	#[serde(rename = "collab.merge")]	157	#[serde(rename = "collab.merge")]
142	Merge,	158	Merge,
143	}	159	}

src/lib.rs

Old		New
@@ -8,6 +8,7 @@ pub mod event;
8	pub mod identity;	8	pub mod identity;
9	pub mod issue;	9	pub mod issue;
10	pub mod log;	10	pub mod log;
		11	pub mod merge_scan;
11	pub mod patch;	12	pub mod patch;
12	pub mod release;	13	pub mod release;
13	pub mod signing;	14	pub mod signing;
@@ -15,6 +16,7 @@ pub mod state;
15	pub mod status;	16	pub mod status;
16	pub mod sync;	17	pub mod sync;
17	pub mod sync_lock;	18	pub mod sync_lock;
		19	pub mod trailer;
18	pub mod trust;	20	pub mod trust;
19	pub mod tui;	21	pub mod tui;
20		22
@@ -351,28 +353,19 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
351	println!("{}", output);	353	println!("{}", output);
352	return Ok(());	354	return Ok(());
353	}	355	}
354	let entries = patch::list(repo, all, archived, limit, offset, sort, &label)?;	356	// One renderer, not two: this used to be a copy of
355	if entries.is_empty() {	357	// `list_to_writer`'s body, and the copies drifted apart every
356	println!("No patches found.");	358	// time either grew a column.
357	} else {	359	patch::list_to_writer(
358	for e in &entries {	360	repo,
359	let p = &e.patch;	361	all,
360	let labels = cli::label_suffix(&p.labels);	362	archived,
361	let stale = match p.staleness(repo) {	363	limit,
362	Ok((_, behind)) if behind > 0 => format!(" [behind {}]", behind),	364	offset,
363	Ok(_) => String::new(),	365	sort,
364	Err(_) => String::new(),	366	&label,
365	};	367	&mut std::io::stdout(),
366	let unread = match e.unread {	368	)?;
367	Some(n) if n > 0 => format!(" ({} new)", n),
368	_ => String::new(),
369	};
370	println!(
371	"{:.8} {:6} {}{} (by {}){}{}",
372	p.id, p.status, p.title, labels, p.author.name, stale, unread
373	);
374	}
375	}
376	Ok(())	369	Ok(())
377	}	370	}
378	PatchCmd::Show { id, json, revision } => {	371	PatchCmd::Show { id, json, revision } => {
@@ -389,7 +382,13 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
389	}	382	}
390	_ => String::new(),	383	_ => String::new(),
391	};	384	};
392	println!("Patch {} [{}{}] (r{})", &p.id[..8], p.status, status_detail, rev_count);	385	println!(
		386	"Patch {} [{}{}] (r{})",
		387	&p.id[..8],
		388	p.status_display(repo),
		389	status_detail,
		390	rev_count
		391	);
393	println!("Title: {}", p.title);	392	println!("Title: {}", p.title);
394	println!("Author: {} <{}>", p.author.name, p.author.email);	393	println!("Author: {} <{}>", p.author.name, p.author.email);
395	match p.resolve_head(repo) {	394	match p.resolve_head(repo) {
@@ -571,6 +570,26 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
571	println!("Label '{}' removed.", label);	570	println!("Label '{}' removed.", label);
572	Ok(())	571	Ok(())
573	}	572	}
		573	PatchCmd::Merge {
		574	id,
		575	commit,
		576	no_close,
		577	} => {
		578	let report = patch::merge(repo, &id, commit.as_deref(), !no_close)?;
		579	match report.outcome {
		580	merge_scan::MergeOutcome::Recorded => println!(
		581	"Recorded patch {:.8} as merged in {:.8}",
		582	report.id, report.commit
		583	),
		584	merge_scan::MergeOutcome::AlreadyMerged => {
		585	println!("Patch {:.8} is already recorded as merged.", report.id)
		586	}
		587	}
		588	if report.close == merge_scan::CloseOutcome::Closed {
		589	println!("Closed the issue it fixes.");
		590	}
		591	Ok(())
		592	}
574	PatchCmd::Close { id, reason } => {	593	PatchCmd::Close { id, reason } => {
575	patch::close(repo, &id, reason.as_deref())?;	594	patch::close(repo, &id, reason.as_deref())?;
576	println!("Patch closed.");	595	println!("Patch closed.");

src/log.rs

Old		New
@@ -128,7 +128,7 @@ fn action_type_name(action: &Action) -> String {
128	Action::PatchComment { .. } => "PatchComment".to_string(),	128	Action::PatchComment { .. } => "PatchComment".to_string(),
129	Action::PatchInlineComment { .. } => "PatchInlineComment".to_string(),	129	Action::PatchInlineComment { .. } => "PatchInlineComment".to_string(),
130	Action::PatchClose { .. } => "PatchClose".to_string(),	130	Action::PatchClose { .. } => "PatchClose".to_string(),
131	Action::PatchMerge => "PatchMerge".to_string(),	131	Action::PatchMerge { .. } => "PatchMerge".to_string(),
132	Action::Merge => "Merge".to_string(),	132	Action::Merge => "Merge".to_string(),
133	}	133	}
134	}	134	}
@@ -175,7 +175,13 @@ fn action_summary(action: &Action) -> String {
175	Some(r) => format!("close: {}", r),	175	Some(r) => format!("close: {}", r),
176	None => "close".to_string(),	176	None => "close".to_string(),
177	},	177	},
178	Action::PatchMerge => "merge".to_string(),	178	Action::PatchMerge { commit } => {
		179	if commit.is_empty() {
		180	"merge".to_string()
		181	} else {
		182	format!("merge {}", &commit[..commit.len().min(7)])
		183	}
		184	}
179	Action::Merge => "dag merge".to_string(),	185	Action::Merge => "dag merge".to_string(),
180	}	186	}
181	}	187	}

src/merge_scan.rs

@@ @@ -0,0 +1,433 @@ @@
+//! Record merges as events, from `Patch:` trailers on a patch's base branch.
+//!
+//! See: docs/superpowers/specs/2026-08-09-merge-recording-design.md
+//!
+//! `PatchStatus::Merged` used to be derived on every read, by asking whether
+//! the patch's head was reachable from its base tip. That fails in ordinary
+//! use: the merged branch is routinely deleted, and reachability cannot see a
+//! squash merge at all. So a merge is recorded, three ways that degrade into
+//! each other:
+//!
+//! 1. a `commit-msg` hook stamps `Patch: <id>` onto commits (not implemented);
+//! 2. `sync` scans the base branch for those trailers — [`scan_and_record_merges`];
+//! 3. `git-collab patch merge <id>` records it by hand — [`crate::patch::merge`].
+//!
+//! 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.
+//!
+//! **Recording is never a side effect of a read.** Nothing in this module runs
+//! on a display path; [`crate::state::PatchState::looks_merged`] is the only
+//! thing a read consults, and it writes nothing.
+use std::collections::{HashMap, HashSet};
+use git2::{Oid, Repository, Sort};
+use crate::dag;
+use crate::error::Error;
+use crate::event::{Action, Author, Event};
+use crate::state::{self, IssueStatus, PatchState, PatchStatus};
+use crate::trailer;
+const ARCHIVED_PATCH_PREFIX: &str = "refs/collab/archive/patches/";
+/// What recording a merge on one patch did.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum MergeOutcome {
+    /// A `PatchMerge` was appended.
+    Recorded,
+    /// The patch was already merged, so nothing was appended.
+    AlreadyMerged,
+}
+/// What reconciling a patch's `--fixes` issue did.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum CloseOutcome {
+    /// An `IssueClose` was appended.
+    Closed,
+    /// The patch has no `fixes`, or the issue was already closed.
+    NothingToDo,
+    /// The issue could not be resolved, or the close failed. Warned about;
+    /// the merge still stands and the next scan retries.
+    Failed,
+}
+/// Append a `PatchMerge` naming the commit that landed the patch, unless the
+/// patch is already merged.
+///
+/// Applying a `PatchMerge` to a patch that is already `Merged` is a no-op in
+/// the fold, so a duplicate would be harmless — but appending one anyway would
+/// make every sync grow the DAG forever, so the check is here rather than left
+/// to the fold.
+pub fn record_merge(
+    repo: &Repository,
+    events_ref: &str,
+    patch: &PatchState,
+    commit: Oid,
+    author: &Author,
+    sk: &ed25519_dalek::SigningKey,
+) -> Result<MergeOutcome, Error> {
+    if patch.status == PatchStatus::Merged {
+        return Ok(MergeOutcome::AlreadyMerged);
+    }
+    let event = Event {
+        timestamp: chrono::Utc::now().to_rfc3339(),
+        author: author.clone(),
+        action: Action::PatchMerge {
+            commit: commit.to_string(),
+        },
+        clock: 0,
+    };
+    dag::append_event(repo, events_ref, &event, sk)?;
+    Ok(MergeOutcome::Recorded)
+}
+/// Close the issue a patch was created to fix, if it is still open.
+///
+/// `--fixes` is documented as "auto-closes on merge" and this is where that
+/// happens: in the same operation that records the merge, never during
+/// derivation.
+///
+/// The two writes go to different refs, so they are not atomic. If this half
+/// fails the merge still stands, and the next scan — which sees a merged patch
+/// with an open `fixes` issue — retries it. That is why this is driven by the
+/// invariant "a merged patch's `fixes` issue is closed" rather than by the
+/// merge emission alone: a rule that only fired alongside a freshly-emitted
+/// `PatchMerge` could never retry, because on the retry pass the merge is a
+/// no-op. Being idempotent is what makes it safe to run every time: an already
+/// closed issue is left alone, so repeated syncs append nothing.
+pub fn close_fixed_issue(repo: &Repository, patch: &PatchState) -> CloseOutcome {
+    close_fixed_issue_inner(repo, patch, true)
+}
+/// `warn_unresolvable` is false on the retry pass. An issue that does not
+/// resolve — deleted, or an ambiguous prefix — will not resolve on the next
+/// sync either, and the attempt made alongside the merge already said so; going
+/// on to warn about it on every sync forever is noise nobody can act on. A
+/// failure to *append* still warns either way, because that one is worth
+/// retrying and worth hearing about.
+fn close_fixed_issue_inner(
+    repo: &Repository,
+    patch: &PatchState,
+    warn_unresolvable: bool,
+) -> CloseOutcome {
+    let Some(fixes) = patch.fixes.as_deref() else {
+        return CloseOutcome::NothingToDo;
+    };
+    let (issue_ref, issue_id) = match state::resolve_issue_ref(repo, fixes) {
+        Ok(v) => v,
+        Err(e) => {
+            // resolve_issue_ref distinguishes "no issue found" from "ambiguous
+            // prefix" in its message.
+            if warn_unresolvable {
+                eprintln!(
+                    "warning: patch {:.8}: cannot close fixed issue {:.8}: {}",
+                    patch.id, fixes, e
+                );
+            }
+            return CloseOutcome::Failed;
+        }
+    };
+    let issue = match state::IssueState::from_ref(repo, &issue_ref, &issue_id) {
+        Ok(i) => i,
+        Err(e) => {
+            if warn_unresolvable {
+                eprintln!(
+                    "warning: patch {:.8}: cannot read fixed issue {:.8}: {}",
+                    patch.id, fixes, e
+                );
+            }
+            return CloseOutcome::Failed;
+        }
+    };
+    if issue.status != IssueStatus::Open {
+        return CloseOutcome::NothingToDo;
+    }
+    let reason = format!("merged in patch {:.8}", patch.id);
+    if let Err(e) = crate::issue::close(repo, &issue_id, Some(&reason)) {
+        eprintln!(
+            "warning: patch {:.8}: failed to close fixed issue {:.8}: {} — \
+             the merge stands; the next scan will retry the close",
+            patch.id, fixes, e
+        );
+        return CloseOutcome::Failed;
+    }
+    CloseOutcome::Closed
+}
+/// A `Patch:` trailer found during the walk, and the commit that carried it.
+struct FoundTrailer {
+    /// The patch id prefix, verbatim from the trailer.
+    prefix: String,
+    /// The commit on the base branch that constitutes the merge.
+    commit: Oid,
+}
+/// Walk each base branch that has open patches, collect `Patch:` trailers, and
+/// emit a `PatchMerge` for every patch whose trailer appears on *its own*
+/// base branch.
+///
+/// **Never breaks sync.** Per-commit and per-patch errors are logged as
+/// one-line stderr warnings and iteration continues, mirroring the `Issue:`
+/// scan exactly. The only errors that propagate are "couldn't even start"
+/// failures; callers treat a returned `Err` as "skip the merge scan for this
+/// sync" and proceed.
+///
+/// Returns the number of `PatchMerge` events actually emitted.
+pub fn scan_and_record_merges(
+    repo: &Repository,
+    author: &Author,
+    sk: &ed25519_dalek::SigningKey,
+) -> Result<usize, Error> {
+    // Only open patches are candidates, and a trailer only counts on the
+    // patch's own base branch — a `Patch:` trailer on some unrelated branch is
+    // not a merge. So group the open patches by base branch and walk each base
+    // once, rather than walking every branch as the `Issue:` scan does, or
+    // walking once per patch.
+    let all = state::list_patches(repo)?;
+    let mut by_base: HashMap<String, Vec<PatchState>> = HashMap::new();
+    for patch in &all {
+        if patch.status == PatchStatus::Open {
+            by_base
+                .entry(patch.base_ref.clone())
+                .or_default()
+                .push(patch.clone());
+        }
+    }
+    let mut emitted = 0usize;
+    // Iterate in a stable order so warnings do not shuffle between runs.
+    let mut bases: Vec<&String> = by_base.keys().collect();
+    bases.sort();
+    for base in bases {
+        let patches = &by_base[base];
+        emitted += scan_one_base(repo, base, patches, author, sk);
+    }
+    retry_pending_closes(repo, &all);
+    Ok(emitted)
+}
+/// Close the `fixes` issue of any patch that was *already* merged when this
+/// scan started.
+///
+/// This is the explicit retry the non-atomicity demands. A merge and the close
+/// of the issue it fixes write to different refs, so the pair can half-succeed;
+/// when it does, the merge stands and the issue is left open with nothing to
+/// finish the job. Recording the merge again is not an option — the second
+/// `PatchMerge` would be a no-op, so hanging the close off a freshly-emitted
+/// merge could never retry.
+///
+/// So the rule is the invariant, not the emission: a merged patch's `fixes`
+/// issue is closed. Cheap, because it only looks at patches that are merged
+/// *and* carry a `fixes`; idempotent, because an already-closed issue is left
+/// alone, which is what keeps repeated syncs from appending anything.
+///
+/// Patches merged during this very scan already had their close attempted
+/// inline, in the same operation that emitted the merge. This pass is for the
+/// ones merged by some earlier operation — including by `patch merge
+/// --no-close`, and including merges recorded on another machine and arriving
+/// by sync.
+fn retry_pending_closes(repo: &Repository, patches: &[PatchState]) {
+    for patch in patches {
+        if patch.status == PatchStatus::Merged && patch.fixes.is_some() {
+            close_fixed_issue_inner(repo, patch, false);
+        }
+    }
+}
+/// Scan one base branch. Returns the number of merges recorded. Absorbs every
+/// error itself, because a base branch that cannot be walked must not stop the
+/// other base branches, let alone the sync.
+fn scan_one_base(
+    repo: &Repository,
+    base: &str,
+    patches: &[PatchState],
+    author: &Author,
+    sk: &ed25519_dalek::SigningKey,
+) -> usize {
+    let base_ref = format!("refs/heads/{}", base);
+    let Ok(base_tip) = repo.refname_to_id(&base_ref) else {
+        // The clone does not have this base branch. Detecting merges on a base
+        // branch we do not have is out of scope, and it is not an error: a
+        // contributor legitimately syncs without every base branch checked out.
+        return 0;
+    };
+    let trailers = match collect_trailers(repo, base_tip, patches) {
+        Ok(t) => t,
+        Err(e) => {
+            eprintln!(
+                "warning: cannot scan '{}' for Patch: trailers: {} — skipping",
+                base, e
+            );
+            return 0;
+        }
+    };
+    let mut emitted = 0usize;
+    for found in trailers {
+        match record_from_trailer(repo, base, &found, author, sk) {
+            Ok(MergeOutcome::Recorded) => emitted += 1,
+            Ok(MergeOutcome::AlreadyMerged) => {}
+            Err(()) => {}
+        }
+    }
+    emitted
+}
+/// Walk `base_tip` back to the oldest base among `patches`, collecting
+/// `Patch:` trailers.
+///
+/// The bound matters: everything older than the oldest open patch's base
+/// cannot have merged a currently-open patch, so walking it is pure cost. The
+/// merge-base of all the recorded bases is an ancestor of every one of them, so
+/// hiding it can never hide a commit we needed — and when the bases are
+/// linearly ordered, which is the normal case, it *is* the oldest of them.
+///
+/// The first trailer seen for a given prefix wins. The walk is newest-first, so
+/// that is the newest commit carrying it: the rebased tip, or the squash commit.
+fn collect_trailers(
+    repo: &Repository,
+    base_tip: Oid,
+    patches: &[PatchState],
+) -> Result<Vec<FoundTrailer>, Error> {
+    let mut revwalk = repo.revwalk()?;
+    revwalk.set_sorting(Sort::TOPOLOGICAL)?;
+    revwalk.push(base_tip)?;
+    let bases: Vec<Oid> = patches
+        .iter()
+        .filter_map(|p| p.effective_base(repo))
+        .collect();
+    if !bases.is_empty() {
+        if let Ok(bound) = repo.merge_base_many(&bases) {
+            // A failure to hide is not fatal — it only means a longer walk.
+            let _ = revwalk.hide(bound);
+        }
+    }
+    let mut found = Vec::new();
+    let mut seen_prefix: HashSet<String> = HashSet::new();
+    for oid_result in revwalk {
+        let oid = match oid_result {
+            Ok(o) => o,
+            Err(e) => {
+                eprintln!("warning: revwalk error, stopping merge scan: {}", e);
+                break;
+            }
+        };
+        let commit = match repo.find_commit(oid) {
+            Ok(c) => c,
+            Err(e) => {
+                eprintln!("warning: cannot load commit {}: {}", oid, e);
+                continue;
+            }
+        };
+        let message = commit.message().unwrap_or("");
+        for prefix in trailer::parse_trailers(message, trailer::PATCH_TOKEN) {
+            if seen_prefix.insert(prefix.clone()) {
+                found.push(FoundTrailer { prefix, commit: oid });
+            }
+        }
+    }
+    Ok(found)
+}
+/// Resolve one trailer and record the merge it names. `Err(())` means the
+/// trailer was warned about and skipped; it never propagates.
+fn record_from_trailer(
+    repo: &Repository,
+    base: &str,
+    found: &FoundTrailer,
+    author: &Author,
+    sk: &ed25519_dalek::SigningKey,
+) -> Result<MergeOutcome, ()> {
+    let (events_ref, id) = match state::resolve_patch_ref(repo, &found.prefix) {
+        Ok(v) => v,
+        Err(e) => {
+            // resolve_patch_ref's message already distinguishes "no patch
+            // found" from "ambiguous prefix".
+            eprintln!(
+                "warning: commit {:.8}: Patch: {} — {}, skipping",
+                found.commit, found.prefix, e
+            );
+            return Err(());
+        }
+    };
+    if events_ref.starts_with(ARCHIVED_PATCH_PREFIX) {
+        eprintln!(
+            "warning: commit {:.8}: Patch: {} — patch is archived, skipping",
+            found.commit, found.prefix
+        );
+        return Err(());
+    }
+    let patch = match PatchState::from_ref(repo, &events_ref, &id) {
+        Ok(p) => p,
+        Err(e) => {
+            eprintln!(
+                "warning: commit {:.8}: Patch: {} — cannot read patch: {}, skipping",
+                found.commit, found.prefix, e
+            );
+            return Err(());
+        }
+    };
+    // A trailer only records a merge on the patch's *own* base branch. The
+    // walk was seeded from one branch, but the trailer may name a patch based
+    // on another, and landing on the wrong branch is not landing.
+    if patch.base_ref != base {
+        return Err(());
+    }
+    // A patch someone deliberately closed is not resurrected by a trailer.
+    if patch.status == PatchStatus::Closed {
+        return Err(());
+    }
+    let outcome = match record_merge(repo, &events_ref, &patch, found.commit, author, sk) {
+        Ok(o) => o,
+        Err(e) => {
+            eprintln!(
+                "warning: commit {:.8}: failed to record merge of patch {:.8}: {}",
+                found.commit, id, e
+            );
+            return Err(());
+        }
+    };
+    // Whether or not the merge was freshly emitted, the `fixes` issue must end
+    // up closed — see `close_fixed_issue` for why the retry has to be able to
+    // run on a pass where the merge itself was a no-op.
+    close_fixed_issue(repo, &patch);
+    Ok(outcome)
+}
+/// Open patches whose head is reachable from their base tip but which carry no
+/// `PatchMerge`. Returned so `sync` can name them; nothing here writes.
+pub fn merge_hints(repo: &Repository) -> Result<Vec<PatchState>, Error> {
+    Ok(state::list_patches(repo)?
+        .into_iter()
+        .filter(|p| p.looks_merged(repo))
+        .collect())
+}
+/// Print the closing hint `sync` shows for patches that look merged but are
+/// recorded nowhere. Knowingly partial — it cannot see a squash — so it is
+/// phrased as a suggestion.
+pub fn print_merge_hints(repo: &Repository) {
+    let Ok(hints) = merge_hints(repo) else { return };
+    if hints.is_empty() {
+        return;
+    }
+    println!(
+        "\n{} patch(es) look merged but are not recorded:",
+        hints.len()
+    );
+    for p in &hints {
+        println!("  {:.8}  {}", p.id, p.title);
+    }
+    println!("Record one with: git-collab patch merge <id>");
+}

src/patch.rs

@@ @@ -160,10 +160,18 @@ pub fn list_to_writer( @@
+            Some(n) if n > 0 => format!(" ({} new)", n),
+            _ => String::new(),
+        };
+        // `merged?` — not `merged` — for a patch that only looks merged.
+        // Reachability is a hint here, never the recorded status.
+        writeln!(
+            writer,
+            "{:.8}  {:7}  {}{}  (by {}){}{}",
+            p.id,
+            p.status_display(repo),
+            p.title,
+            labels,
+            p.author.name,
+            stale,
+            unread
+        .ok();
+    Ok(())
+/// What `patch merge` did, so the caller can say so.
+pub struct MergeReport {
+    pub id: String,
+    pub outcome: crate::merge_scan::MergeOutcome,
+    pub close: crate::merge_scan::CloseOutcome,
+    /// The commit recorded as having landed the patch.
+    pub commit: Oid,
+}
+/// Record that a patch landed on its base branch — layer 3 of merge recording,
+/// the one that always works.
+///
+/// `commit` names the commit that constitutes the merge; with none given it is
+/// the current tip of the patch's base branch, which is what it is immediately
+/// after merging. This is the fallback for everything the trailer scan cannot
+/// see: a squash whose message was rewritten by hand, a merge made before any
+/// of this existed, a base branch this clone does not have the history of.
+///
+/// `close_fixes` is on by default: `--fixes` is documented as auto-closing on
+/// merge, and this is one of the two places that happens. Pass `false` to
+/// record a merge that does not resolve the issue — a partial fix — or when the
+/// issue should be closed by hand. The two writes go to different refs and are
+/// not atomic; if the close does not land, the merge still stands and the next
+/// `sync` retries it.
+pub fn merge(
+    repo: &Repository,
+    id_prefix: &str,
+    commit: Option<&str>,
+    close_fixes: bool,
+) -> Result<MergeReport, crate::error::Error> {
+    let (events_ref, id) = state::resolve_patch_ref(repo, id_prefix)?;
+    let patch = PatchState::from_ref(repo, &events_ref, &id)?;
+    let commit_oid = match commit {
+        Some(rev) => repo
+            .revparse_single(rev)
+            .map_err(|e| Error::Cmd(format!("cannot resolve '{}': {}", rev, e)))?
+            .id(),
+        None => {
+            let base_ref = format!("refs/heads/{}", patch.base_ref);
+            repo.refname_to_id(&base_ref).map_err(|e| {
+                Error::Cmd(format!(
+                    "base branch '{}' not found: {} — pass --commit to name the commit that landed the patch",
+                    patch.base_ref, e
+                ))
+            })?
+        }
+    };
+    let author = get_author(repo)?;
+    let sk = signing::load_signing_key(&signing::signing_key_dir()?)?;
+    let outcome = crate::merge_scan::record_merge(
+        repo,
+        &events_ref,
+        &patch,
+        commit_oid,
+        &author,
+        &sk,
+    )?;
+    // Reconcile the `fixes` issue whether or not the merge was freshly
+    // emitted, so a close that failed the first time is retried rather than
+    // stranded. Idempotent: an already-closed issue is left alone.
+    let close = if close_fixes {
+        crate::merge_scan::close_fixed_issue(repo, &patch)
+    } else {
+        crate::merge_scan::CloseOutcome::NothingToDo
+    };
+    Ok(MergeReport {
+        id,
+        outcome,
+        close,
+        commit: commit_oid,
+    })
+}
+pub fn close(
+    repo: &Repository,
+    id_prefix: &str,

src/state.rs

Old		New
@@ -542,7 +542,7 @@ impl PatchState {
542	/// unknown base cannot be recovered by recomputing a merge-base against the	542	/// unknown base cannot be recovered by recomputing a merge-base against the
543	/// tip as it stands now — once the patch is merged that yields the head	543	/// tip as it stands now — once the patch is merged that yields the head
544	/// itself, which looks like a base that never moved.	544	/// itself, which looks like a base that never moved.
545	fn effective_base(&self, repo: &Repository) -> Option<Oid> {	545	pub fn effective_base(&self, repo: &Repository) -> Option<Oid> {
546	let end = self.latest_usable_index(repo)?;	546	let end = self.latest_usable_index(repo)?;
547	self.revisions[..=end]	547	self.revisions[..=end]
548	.iter()	548	.iter()
@@ -561,17 +561,31 @@ impl PatchState {
561	Ok((ahead, behind))	561	Ok((ahead, behind))
562	}	562	}
563		563
564	/// Auto-detect merge: if the patch is still Open and its head is	564	/// Whether this patch looks merged: it is still Open, and its head is
565	/// reachable from the base branch tip, the user merged it outside of	565	/// reachable from the tip of its base branch.
566	/// git-collab. We compare the current base tip to where the latest
567	/// revision stood — if the base has moved to include the patch head, the
568	/// patch is merged.
569	///	566	///
570	/// Anchoring this to the latest revision rather than to creation matters	567	/// This is a hint and nothing more. It used to decide the patch's status on
571	/// twice over. It survives a rebase, which moves the base out from under	568	/// every read, which was wrong in both directions:
572	/// an earlier revision; and it no longer routes through	569	///
573	/// `refs/heads/<branch>`, which used to make detection no-op silently	570	/// - It cannot see a squash or a rebase-merge. Those put a new commit
574	/// whenever the source branch had been deleted or was never pushed.	571	/// with the same tree on the base branch, so the patch's own commits are
		572	/// never ancestors of it and `graph_descendant_of` is false forever.
		573	/// - Anything it decided was recorded nowhere, so it had to be re-derived
		574	/// every time, from a base branch the clone might not have.
		575	///
		576	/// So its absence must never be read as "not merged", and it never writes:
		577	/// a merge is recorded by `Action::PatchMerge`, emitted only by an explicit
		578	/// act (`patch merge`, or the `Patch:` trailer scan during sync). Making
		579	/// this method append during derivation would rebuild the phantom-revision
		580	/// bug with a wider blast radius — `patch list` would mutate the DAG,
		581	/// read-only clones would break, and two people running it concurrently
		582	/// would both append.
		583	///
		584	/// Anchoring to the latest revision rather than to creation matters twice
		585	/// over. It survives a rebase, which moves the base out from under an
		586	/// earlier revision; and it does not route through `refs/heads/<branch>`,
		587	/// which used to make detection no-op silently whenever the source branch
		588	/// had been deleted or was never pushed.
575	///	589	///
576	/// `base_moved` exists only to spare the degenerate patch whose recorded	590	/// `base_moved` exists only to spare the degenerate patch whose recorded
577	/// base is its own head — one created on the base branch itself. With a	591	/// base is its own head — one created on the base branch itself. With a
@@ -580,25 +594,24 @@ impl PatchState {
580	/// The base therefore has to be the one actually recorded, never one	594	/// The base therefore has to be the one actually recorded, never one
581	/// recomputed against the tip as it stands now: in the exact fast-forward	595	/// recomputed against the tip as it stands now: in the exact fast-forward
582	/// case a recomputed merge-base is the head itself, so a merged patch looks	596	/// case a recomputed merge-base is the head itself, so a merged patch looks
583	/// unmoved and stays Open forever. That is the commonest merge for a	597	/// unmoved. That is the commonest merge for a single-commit patch.
584	/// single-commit patch. `effective_base` searches back for the newest	598	/// `effective_base` searches back for the newest recorded base rather than
585	/// recorded base rather than reading only the newest revision, which is	599	/// reading only the newest revision, which is what keeps migrated patches —
586	/// what keeps migrated patches — base on the create, none on the revisions	600	/// base on the create, none on the revisions after it — out of that trap.
587	/// after it — out of that trap.
588	///	601	///
589	/// A patch with no recorded base anywhere predates the field entirely.	602	/// A patch with no recorded base anywhere predates the field entirely.
590	/// Nothing distinguishes it from the degenerate case, so it keeps the	603	/// Nothing distinguishes it from the degenerate case, so it is not guessed
591	/// behaviour it has always had rather than being guessed at.	604	/// at.
592	fn check_auto_merge(&mut self, repo: &Repository) {	605	pub fn looks_merged(&self, repo: &Repository) -> bool {
593	if self.status != PatchStatus::Open {	606	if self.status != PatchStatus::Open {
594	return;	607	return false;
595	}	608	}
596	let Ok(patch_head) = self.resolve_head(repo) else {	609	let Ok(patch_head) = self.resolve_head(repo) else {
597	return;	610	return false;
598	};	611	};
599	let base_ref = format!("refs/heads/{}", self.base_ref);	612	let base_ref = format!("refs/heads/{}", self.base_ref);
600	let Ok(base_tip) = repo.refname_to_id(&base_ref) else {	613	let Ok(base_tip) = repo.refname_to_id(&base_ref) else {
601	return;	614	return false;
602	};	615	};
603	let base_moved = match self.effective_base(repo) {	616	let base_moved = match self.effective_base(repo) {
604	Some(base) => base != base_tip,	617	Some(base) => base != base_tip,
@@ -608,8 +621,18 @@ impl PatchState {
608	\|\| repo	621	\|\| repo
609	.graph_descendant_of(base_tip, patch_head)	622	.graph_descendant_of(base_tip, patch_head)
610	.unwrap_or(false);	623	.unwrap_or(false);
611	if base_moved && reachable {	624	base_moved && reachable
612	self.status = PatchStatus::Merged;	625	}
		626
		627	/// What to print where the status goes: the recorded status, or `merged?`
		628	/// for a patch that only looks merged. The question mark is the whole
		629	/// point — the hint is knowingly partial, so it must never be mistaken for
		630	/// the recorded fact.
		631	pub fn status_display(&self, repo: &Repository) -> String {
		632	if self.looks_merged(repo) {
		633	"merged?".to_string()
		634	} else {
		635	self.status.to_string()
613	}	636	}
614	}	637	}
615		638
@@ -618,17 +641,10 @@ impl PatchState {
618	ref_name: &str,	641	ref_name: &str,
619	id: &str,	642	id: &str,
620	) -> Result<Self, crate::error::Error> {	643	) -> Result<Self, crate::error::Error> {
621	// Check cache first	644	// Check cache first. Nothing is recomputed on the way out: status is
622	if let Some(mut cached) = cache::get_cached_state::<PatchState>(repo, ref_name) {	645	// now folded from the DAG alone, so a cache entry keyed on the DAG tip
623	// The cache may return a stale Open status if the patch was merged	646	// is complete by construction.
624	// outside of git-collab since the DAG tip hasn't changed.	647	if let Some(cached) = cache::get_cached_state::<PatchState>(repo, ref_name) {
625	cached.check_auto_merge(repo);
626	if cached.status == PatchStatus::Merged {
627	// Update the cache with the corrected status
628	if let Ok(tip) = repo.refname_to_id(ref_name) {
629	cache::set_cached_state(repo, ref_name, tip, &cached);
630	}
631	}
632	return Ok(cached);	648	return Ok(cached);
633	}	649	}
634		650
@@ -831,7 +847,7 @@ impl PatchState {
831	}	847	}
832	}	848	}
833	}	849	}
834	Action::PatchMerge => {	850	Action::PatchMerge { .. } => {
835	if let Some(ref mut s) = state {	851	if let Some(ref mut s) = state {
836	let key = (event.clock, oid.to_string());	852	let key = (event.clock, oid.to_string());
837	if status_key.as_ref().is_none_or(\|k\| key >= *k) {	853	if status_key.as_ref().is_none_or(\|k\| key >= *k) {
@@ -846,7 +862,6 @@ impl PatchState {
846		862
847	if let Some(ref mut s) = state {	863	if let Some(ref mut s) = state {
848	s.last_updated = latest.map(\|(_, raw)\| raw).unwrap_or_default();	864	s.last_updated = latest.map(\|(_, raw)\| raw).unwrap_or_default();
849	s.check_auto_merge(repo);
850	}	865	}
851	state.ok_or_else(\|\| git2::Error::from_str("no PatchCreate event found in DAG").into())	866	state.ok_or_else(\|\| git2::Error::from_str("no PatchCreate event found in DAG").into())
852	}	867	}

src/sync.rs

Old		New
@@ -484,6 +484,17 @@ pub fn sync(repo: &Repository, remote_name: &str) -> Result<(), Error> {
484	Err(e) => eprintln!("warning: commit link scan failed: {}", e),	484	Err(e) => eprintln!("warning: commit link scan failed: {}", e),
485	}	485	}
486		486
		487	// Step 2.6: Scan each open patch's own base branch for Patch: trailers and
		488	// record the merges they name. Same contract as the link scan above: it
		489	// absorbs its own per-commit and per-patch errors and can never break sync.
		490	// This runs before the push so anything it records travels in the same
		491	// sync rather than waiting for the next one.
		492	match crate::merge_scan::scan_and_record_merges(&repo, &author, &sk) {
		493	Ok(n) if n > 0 => println!("Recorded {} merged patch(es).", n),
		494	Ok(_) => {}
		495	Err(e) => eprintln!("warning: merge scan failed: {}", e),
		496	}
		497
487	// Step 3: Push collab refs individually	498	// Step 3: Push collab refs individually
488	println!("Pushing to '{}'...", remote_name);	499	println!("Pushing to '{}'...", remote_name);
489	let refs_to_push = collect_push_refs(&repo)?;	500	let refs_to_push = collect_push_refs(&repo)?;
@@ -530,6 +541,11 @@ pub fn sync(repo: &Repository, remote_name: &str) -> Result<(), Error> {
530	cleanup_sync_refs(&repo)?;	541	cleanup_sync_refs(&repo)?;
531		542
532	println!("Sync complete for '{}'.", remote_name);	543	println!("Sync complete for '{}'.", remote_name);
		544
		545	// A closing hint, not a decision: patches whose head is reachable from
		546	// their base tip but which carry no `PatchMerge`. Reachability cannot see a
		547	// squash, so its silence proves nothing — and it writes nothing.
		548	crate::merge_scan::print_merge_hints(&repo);
533	Ok(())	549	Ok(())
534	}	550	}
535		551

src/trailer.rs

@@ @@ -0,0 +1,129 @@ @@
+//! Git trailer parsing, shared by everything that reads a `<Token>: <value>`
+//! line out of a commit message.
+//!
+//! Two features use it: `Issue:` trailers link commits to issues
+//! (`commit_link`), and `Patch:` trailers record that a patch landed on its
+//! base branch (`merge_scan`). They differ only in the token, so they share one
+//! parser and one set of tests (`tests/trailer_test.rs`) — a fix to the block
+//! semantics or the value rules cannot land for one and miss the other.
+//!
+//! See: docs/superpowers/specs/2026-04-12-commit-issue-link-design.md
+//! See: docs/superpowers/specs/2026-08-09-merge-recording-design.md
+/// Parse `<token>: <value>` trailers out of a commit message.
+///
+/// Returns the values in order of appearance. Follows git's own trailer-block
+/// semantics: only the final paragraph is considered, and *every* non-empty
+/// line in it must be trailer-shaped (a `token: value` line) for the paragraph
+/// to qualify. Any prose line in the final paragraph disqualifies the whole
+/// paragraph — this prevents false positives like `"Thanks Bob.\nIssue: abc"`
+/// in commit bodies.
+///
+/// The key match is case-insensitive. The value must be a single
+/// non-whitespace token followed by optional trailing whitespace and
+/// end-of-line; values like `abc fixes thing` are rejected so that loose
+/// commentary never becomes a silent prefix lookup that warns every sync
+/// forever.
+pub fn parse_trailers(message: &str, token: &str) -> Vec<String> {
+    // 1. Split into paragraphs (blank-line separated), preserving order.
+    //    Trim trailing whitespace from each line for the trailer-shape check,
+    //    but keep enough structure to recognize blank lines.
+    let lines: Vec<&str> = message.lines().collect();
+    // 2. Find the last paragraph: the longest tail slice that contains at
+    //    least one non-empty line and has no blank line *before* its first
+    //    non-empty line in the tail.
+    //
+    //    Walking from the end: skip trailing blank/whitespace-only lines,
+    //    then collect lines until we hit a blank line.
+    let mut end = lines.len();
+    while end > 0 && lines[end - 1].trim().is_empty() {
+        end -= 1;
+    }
+    if end == 0 {
+        return Vec::new();
+    }
+    let mut start = end;
+    while start > 0 && !lines[start - 1].trim().is_empty() {
+        start -= 1;
+    }
+    let paragraph = &lines[start..end];
+    // 3. Validate every non-empty line in the paragraph is trailer-shaped.
+    for line in paragraph {
+        if line.trim().is_empty() {
+            continue;
+        }
+        if !is_trailer_shaped(line) {
+            return Vec::new();
+        }
+    }
+    // 4. Extract the values whose key is `token`.
+    let mut out = Vec::new();
+    for line in paragraph {
+        if let Some(value) = match_trailer_line(line, token) {
+            out.push(value);
+        }
+    }
+    out
+}
+/// Returns true if a line looks like a git trailer: `<token>: <value>`, where
+/// token starts with a letter and consists of `[A-Za-z0-9-]`, and value is at
+/// least one non-whitespace character.
+fn is_trailer_shaped(line: &str) -> bool {
+    let trimmed = line.trim_start();
+    let Some(colon_pos) = trimmed.find(':') else {
+        return false;
+    };
+    // Use trim_end() so that `ISSUE : abc` is recognized as the token `ISSUE`
+    // — matching what `match_trailer_line` does. Without this, the space before
+    // the colon would disqualify the line and make the whole paragraph fail
+    // the trailer-shape check.
+    let token = trimmed[..colon_pos].trim_end();
+    if token.is_empty() {
+        return false;
+    }
+    let mut chars = token.chars();
+    let first = chars.next().unwrap();
+    if !first.is_ascii_alphabetic() {
+        return false;
+    }
+    if !chars.all(|c| c.is_ascii_alphanumeric() || c == '-') {
+        return false;
+    }
+    let value = trimmed[colon_pos + 1..].trim();
+    !value.is_empty()
+}
+/// If `line` is a `<token>: <value>` trailer with exactly one non-whitespace
+/// token in its value, returns the value. Otherwise returns None.
+fn match_trailer_line(line: &str, token: &str) -> Option<String> {
+    let trimmed = line.trim_start();
+    let colon_pos = trimmed.find(':')?;
+    let key = trimmed[..colon_pos].trim_end();
+    if !key.eq_ignore_ascii_case(token) {
+        return None;
+    }
+    let value_region = &trimmed[colon_pos + 1..];
+    let value = value_region.trim();
+    if value.is_empty() {
+        return None;
+    }
+    // Reject values with interior whitespace: `abc fixes thing` must not
+    // parse to `abc` silently — it must parse to nothing so the user sees
+    // that their commentary is being ignored. This matters more for `Patch:`
+    // than for `Issue:`, because there the silent reading would record a
+    // merge under an id the author never wrote on its own.
+    if value.split_whitespace().count() != 1 {
+        return None;
+    }
+    Some(value.to_string())
+}
+/// The token `Issue:` trailers use.
+pub const ISSUE_TOKEN: &str = "issue";
+/// The token `Patch:` trailers use.
+pub const PATCH_TOKEN: &str = "patch";

src/tui/widgets.rs

Old		New
@@ -21,7 +21,7 @@ pub(crate) fn action_type_label(action: &Action) -> &str {
21	Action::PatchComment { .. } => "Patch Comment",	21	Action::PatchComment { .. } => "Patch Comment",
22	Action::PatchInlineComment { .. } => "Inline Comment",	22	Action::PatchInlineComment { .. } => "Inline Comment",
23	Action::PatchClose { .. } => "Patch Close",	23	Action::PatchClose { .. } => "Patch Close",
24	Action::PatchMerge => "Patch Merge",	24	Action::PatchMerge { .. } => "Patch Merge",
25	Action::Merge => "Merge",	25	Action::Merge => "Merge",
26	Action::IssueEdit { .. } => "Issue Edit",	26	Action::IssueEdit { .. } => "Issue Edit",
27	Action::IssueLabel { .. } => "Issue Label",	27	Action::IssueLabel { .. } => "Issue Label",
@@ -129,7 +129,12 @@ pub(crate) fn format_event_detail(oid: &Oid, event: &crate::event::Event) -> Str
129	Action::IssueCommitLink { commit } => {	129	Action::IssueCommitLink { commit } => {
130	detail.push_str(&format!("\nCommit: {}\n", commit));	130	detail.push_str(&format!("\nCommit: {}\n", commit));
131	}	131	}
132	Action::IssueReopen \| Action::PatchMerge \| Action::Merge => {}	132	Action::PatchMerge { commit } => {
		133	if !commit.is_empty() {
		134	detail.push_str(&format!("\nMerged as: {}\n", commit));
		135	}
		136	}
		137	Action::IssueReopen \| Action::Merge => {}
133	}	138	}
134		139
135	detail	140	detail

tests/adversarial_test.rs

Old		New
@@ -553,7 +553,9 @@ fn arb_action() -> impl Strategy<Value = Action> {
553	base: None,	553	base: None,
554	}),	554	}),
555	(".*",).prop_map(\|(body,)\| Action::PatchComment { body }),	555	(".*",).prop_map(\|(body,)\| Action::PatchComment { body }),
556	Just(Action::PatchMerge),	556	Just(Action::PatchMerge {
		557	commit: "0000000000000000000000000000000000000000".to_string(),
		558	}),
557	Just(Action::Merge),	559	Just(Action::Merge),
558	]	560	]
559	}	561	}

tests/cli_test.rs

Old		New
@@ -733,12 +733,10 @@ fn test_patch_list_label_all_flag_isolated_from_archived() {
733	// archived" patch (archiving happens only inside `close`), so that half	733	// archived" patch (archiving happens only inside `close`), so that half
734	// of the pair genuinely cannot be isolated under current semantics.	734	// of the pair genuinely cannot be isolated under current semantics.
735	//	735	//
736	// But the other half can: a patch auto-detected as Merged (its branch	736	// But the other half can: a patch recorded as merged by `patch merge`
737	// fast-forwarded into base outside git-collab, which	737	// becomes non-open without ever being archived -- archiving is something
738	// `PatchState::check_auto_merge` picks up on read) becomes non-open	738	// only `close` does. That gives a labeled, non-open, still-active patch,
739	// without ever being archived -- archiving is something only `close`	739	// which isolates --all: visible with --all alone, hidden without it, and
740	// does. That gives a labeled, non-open, still-active patch, which
741	// isolates --all: visible with --all alone, hidden without it, and
742	// --archived is irrelevant either way since it was never archived.	740	// --archived is irrelevant either way since it was never archived.
743	let repo = TestRepo::new("Alice", "alice@example.com");	741	let repo = TestRepo::new("Alice", "alice@example.com");
744		742
@@ -752,9 +750,10 @@ fn test_patch_list_label_all_flag_isolated_from_archived() {
752	.to_string();	750	.to_string();
753	repo.run_ok(&["patch", "label", &id, "bug"]);	751	repo.run_ok(&["patch", "label", &id, "bug"]);
754		752
755	// Simulate `git merge merge-me` on main, outside of git-collab.	753	// Merge with plain git, then record it.
756	repo.git(&["checkout", "main"]);	754	repo.git(&["checkout", "main"]);
757	repo.git(&["merge", "--ff-only", "merge-me"]);	755	repo.git(&["merge", "--ff-only", "merge-me"]);
		756	repo.run_ok(&["patch", "merge", &id]);
758		757
759	let out = repo.run_ok(&["patch", "list", "--label", "bug"]);	758	let out = repo.run_ok(&["patch", "list", "--label", "bug"]);
760	assert!(!out.contains("Merged labeled"));	759	assert!(!out.contains("Merged labeled"));
@@ -960,7 +959,7 @@ fn test_patch_close() {
960	}	959	}
961		960
962	#[test]	961	#[test]
963	fn test_patch_auto_detect_merge_on_git_merge() {	962	fn test_git_merge_shows_a_hint_and_patch_merge_records_it() {
964	let repo = TestRepo::new("Alice", "alice@example.com");	963	let repo = TestRepo::new("Alice", "alice@example.com");
965		964
966	// Create a feature branch ahead of main	965	// Create a feature branch ahead of main
@@ -970,14 +969,19 @@ fn test_patch_auto_detect_merge_on_git_merge() {
970		969
971	// Create patch pointing at the feature branch	970	// Create patch pointing at the feature branch
972	let out = repo.run_ok(&["patch", "create", "-t", "Add feature", "-B", "feature"]);	971	let out = repo.run_ok(&["patch", "create", "-t", "Add feature", "-B", "feature"]);
973	let id = out.trim().strip_prefix("Created patch ").unwrap();	972	let id = out.trim().strip_prefix("Created patch ").unwrap().to_string();
974		973
975	// Merge via git directly	974	// Merge via git directly
976	repo.git(&["merge", "feature"]);	975	repo.git(&["merge", "feature"]);
977		976
978	// Patch should auto-detect as merged	977	// Reachability makes the patch look merged, with a question mark: it
979	let out = repo.run_ok(&["patch", "show", id]);	978	// cannot see a squash, so it never gets to decide.
980	assert!(out.contains("[merged]"));	979	let out = repo.run_ok(&["patch", "show", &id]);
		980	assert!(out.contains("[merged?]"), "{}", out);
		981
		982	repo.run_ok(&["patch", "merge", &id]);
		983	let out = repo.run_ok(&["patch", "show", &id]);
		984	assert!(out.contains("[merged]") && !out.contains("[merged?]"), "{}", out);
981	}	985	}
982		986
983	#[test]	987	#[test]
@@ -1283,11 +1287,13 @@ fn test_full_patch_review_cycle() {
1283	// Approve	1287	// Approve
1284	repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM now"]);	1288	repo.run_ok(&["patch", "review", &id, "-v", "approve", "-b", "LGTM now"]);
1285		1289
1286	// Merge via git	1290	// Merge via git, then record it. Merging stays plain git; git-collab
		1291	// records that it happened rather than deriving it.
1287	repo.git(&["checkout", "main"]);	1292	repo.git(&["checkout", "main"]);
1288	repo.git(&["merge", "feature"]);	1293	repo.git(&["merge", "feature"]);
		1294	repo.run_ok(&["patch", "merge", &id]);
1289		1295
1290	// Verify final state — auto-detected as merged	1296	// Verify final state
1291	let out = repo.run_ok(&["patch", "show", &id]);	1297	let out = repo.run_ok(&["patch", "show", &id]);
1292	assert!(out.contains("[merged]"));	1298	assert!(out.contains("[merged]"));
1293	assert!(out.contains("Added documentation"));	1299	assert!(out.contains("Added documentation"));

tests/collab_test.rs

Old		New
@@ -1023,14 +1023,15 @@ fn test_resolve_head_with_oid_string() {
1023	}	1023	}
1024		1024
1025	// ---------------------------------------------------------------------------	1025	// ---------------------------------------------------------------------------
1026	// Phase 5: US3 — Merge auto-detection	1026	// Phase 5: US3 — Merge detection, demoted to a hint
1027	// ---------------------------------------------------------------------------	1027	// ---------------------------------------------------------------------------
1028		1028
1029	#[test]	1029	#[test]
1030	fn test_auto_detect_merged_patch_via_git_merge() {	1030	fn test_reachability_is_a_hint_and_does_not_set_the_status() {
1031	// When a user merges the patch branch into the base branch manually	1031	// Merging the patch branch into the base branch with plain git makes the
1032	// (using git merge), PatchState should auto-detect that the patch	1032	// patch look merged, and that is all it does. Merged state is recorded
1033	// is merged without needing `patch merge`.	1033	// by an event, never derived: reachability cannot see a squash, so its
		1034	// verdict is a suggestion in one direction and silence in the other.
1034	let cfg = ScopedTestConfig::new();	1035	let cfg = ScopedTestConfig::new();
1035	cfg.ensure_signing_key();	1036	cfg.ensure_signing_key();
1036	let tmp = TempDir::new().unwrap();	1037	let tmp = TempDir::new().unwrap();
@@ -1055,12 +1056,29 @@ fn test_auto_detect_merged_patch_via_git_merge() {
1055	repo.reference("refs/heads/main", feat_tip, true, "manual merge")	1056	repo.reference("refs/heads/main", feat_tip, true, "manual merge")
1056	.unwrap();	1057	.unwrap();
1057		1058
1058	// Now PatchState should auto-detect that it's merged	1059	// The patch now looks merged, and is still Open until someone records it.
1059	let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap();	1060	let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap();
		1061	assert!(state.looks_merged(&repo), "the hint should fire");
1060	assert_eq!(	1062	assert_eq!(
1061	state.status,	1063	state.status,
1062	PatchStatus::Merged,	1064	PatchStatus::Open,
1063	"should auto-detect merge"	1065	"the hint must not become the status"
		1066	);
		1067	assert_eq!(state.status_display(&repo), "merged?");
		1068
		1069	// Recording it is what makes it Merged — and reading it never did.
		1070	let tip_before = repo.refname_to_id(&ref_name).unwrap();
		1071	assert_eq!(
		1072	repo.refname_to_id(&ref_name).unwrap(),
		1073	tip_before,
		1074	"deriving state must not append to the DAG"
		1075	);
		1076	patch::merge(&repo, &id, None, true).unwrap();
		1077	let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap();
		1078	assert_eq!(state.status, PatchStatus::Merged);
		1079	assert!(
		1080	!state.looks_merged(&repo),
		1081	"a recorded merge is a fact, not a hint"
1064	);	1082	);
1065	}	1083	}
1066		1084
@@ -1094,10 +1112,12 @@ fn test_auto_detect_merged_patch_deleted_branch() {
1094	}	1112	}
1095		1113
1096	#[test]	1114	#[test]
1097	fn test_cache_does_not_defeat_auto_detect_merge() {	1115	fn test_a_cache_hit_neither_hides_nor_writes_the_merge_hint() {
1098	// Regression: from_ref() returned cached Open status even after the	1116	// The cache used to have to be corrected on read, because status was
1099	// patch branch was merged into main, because the cache hit bypassed	1117	// derived from the base branch rather than folded from the DAG — a cache
1100	// the auto-detect merge logic that only ran in from_ref_uncached().	1118	// entry keyed on the DAG tip could not see main move. Now status comes from
		1119	// the DAG alone, so a cache hit is complete by construction, and the hint
		1120	// is computed fresh at display time without touching the cache or the DAG.
1101	let cfg = ScopedTestConfig::new();	1121	let cfg = ScopedTestConfig::new();
1102	cfg.ensure_signing_key();	1122	cfg.ensure_signing_key();
1103	let tmp = TempDir::new().unwrap();	1123	let tmp = TempDir::new().unwrap();
@@ -1122,12 +1142,19 @@ fn test_cache_does_not_defeat_auto_detect_merge() {
1122	repo.reference("refs/heads/main", feat_tip, true, "manual merge")	1142	repo.reference("refs/heads/main", feat_tip, true, "manual merge")
1123	.unwrap();	1143	.unwrap();
1124		1144
1125	// Second call: cache hit (DAG tip unchanged), but should still detect merge	1145	// Second call: cache hit (DAG tip unchanged). The status is still the
		1146	// recorded one, and the hint is available all the same.
		1147	let tip_before = repo.refname_to_id(&ref_name).unwrap();
1126	let state2 = PatchState::from_ref(&repo, &ref_name, &id).unwrap();	1148	let state2 = PatchState::from_ref(&repo, &ref_name, &id).unwrap();
		1149	assert_eq!(state2.status, PatchStatus::Open);
		1150	assert!(
		1151	state2.looks_merged(&repo),
		1152	"the hint is computed from the repo, not read out of the cache"
		1153	);
1127	assert_eq!(	1154	assert_eq!(
1128	state2.status,	1155	repo.refname_to_id(&ref_name).unwrap(),
1129	PatchStatus::Merged,	1156	tip_before,
1130	"cached from_ref should detect merge"	1157	"a cached read must not append to the DAG"
1131	);	1158	);
1132	}	1159	}
1133		1160

tests/commit_link_test.rs

Old		New
@@ -186,99 +186,17 @@ fn issue_state_dedups_commit_links_by_sha_keeping_earliest() {
186		186
187	use git_collab::commit_link::parse_issue_trailers;	187	use git_collab::commit_link::parse_issue_trailers;
188		188
189	#[test]	189	// The trailer parser itself lives in `git_collab::trailer` and is shared with
190	fn parser_no_trailer_block() {	190	// `Patch:` trailers; its cases are in `tests/trailer_test.rs`, run against both
191	assert_eq!(parse_issue_trailers("Just a plain commit"), Vec::<String>::new());	191	// tokens. All that is left to pin here is which token this entry point picks —
192	}	192	// the one thing the shared tests cannot see.
193
194	#[test]
195	fn parser_empty_message() {
196	assert_eq!(parse_issue_trailers(""), Vec::<String>::new());
197	}
198		193
199	#[test]	194	#[test]
200	fn parser_single_trailer_in_pure_block() {	195	fn parse_issue_trailers_reads_issue_and_not_patch() {
201	let msg = "Fix thing\n\nSome context in the body.\n\nIssue: abc";	196	let msg = "subject\n\nIssue: abc\nPatch: def";
202	assert_eq!(parse_issue_trailers(msg), vec!["abc".to_string()]);	197	assert_eq!(parse_issue_trailers(msg), vec!["abc".to_string()]);
203	}	198	}
204		199
205	#[test]
206	fn parser_case_variants() {
207	let msg1 = "subject\n\nissue: abc";
208	let msg2 = "subject\n\nISSUE : abc";
209	let msg3 = "subject\n\n Issue: abc ";
210	assert_eq!(parse_issue_trailers(msg1), vec!["abc".to_string()]);
211	assert_eq!(parse_issue_trailers(msg2), vec!["abc".to_string()]);
212	assert_eq!(parse_issue_trailers(msg3), vec!["abc".to_string()]);
213	}
214
215	#[test]
216	fn parser_two_trailers_in_pure_block() {
217	let msg = "subject\n\nIssue: abc\nIssue: def";
218	assert_eq!(parse_issue_trailers(msg), vec!["abc".to_string(), "def".to_string()]);
219	}
220
221	#[test]
222	fn parser_issue_in_body_but_not_final_paragraph() {
223	let msg = "subject\n\nIssue: abc\n\nSigned-off-by: alice <a@example.com>";
224	// The final paragraph is the signed-off-by block, not the issue line.
225	// It's a valid trailer block (Signed-off-by is trailer-shaped), but it
226	// contains no Issue: key, so we extract nothing.
227	assert_eq!(parse_issue_trailers(msg), Vec::<String>::new());
228	}
229
230	#[test]
231	fn parser_wrong_key() {
232	let msg = "subject\n\nIssues: abc";
233	assert_eq!(parse_issue_trailers(msg), Vec::<String>::new());
234	}
235
236	#[test]
237	fn parser_prose_mention() {
238	let msg = "subject\n\nthis fixes issue abc in the body";
239	assert_eq!(parse_issue_trailers(msg), Vec::<String>::new());
240	}
241
242	#[test]
243	fn parser_single_paragraph_whole_message_is_trailer_block() {
244	let msg = "Issue: abc";
245	assert_eq!(parse_issue_trailers(msg), vec!["abc".to_string()]);
246	}
247
248	#[test]
249	fn parser_mixed_final_paragraph_rejects_all() {
250	let msg = "subject\n\nThanks to Bob for the catch.\nIssue: a3f9";
251	// Final paragraph has a prose line, so it's not a trailer block and we
252	// extract nothing. This is the "false positive in prose" guard.
253	assert_eq!(parse_issue_trailers(msg), Vec::<String>::new());
254	}
255
256	#[test]
257	fn parser_trailing_whitespace_paragraph_does_not_shadow_trailer_block() {
258	// The final paragraph is empty/whitespace, so the walk should fall back
259	// to the previous non-empty paragraph, which is a valid trailer block.
260	let msg = "subject\n\nIssue: abc\n\n \n";
261	assert_eq!(parse_issue_trailers(msg), vec!["abc".to_string()]);
262	}
263
264	#[test]
265	fn parser_pure_block_with_mixed_keys() {
266	let msg = "subject\n\nSigned-off-by: alice <a@example.com>\nIssue: abc";
267	assert_eq!(parse_issue_trailers(msg), vec!["abc".to_string()]);
268	}
269
270	#[test]
271	fn parser_rejects_value_with_trailing_garbage() {
272	let msg = "subject\n\nIssue: abc fixes thing";
273	assert_eq!(parse_issue_trailers(msg), Vec::<String>::new());
274	}
275
276	#[test]
277	fn parser_rejects_empty_value() {
278	let msg = "subject\n\nIssue: ";
279	assert_eq!(parse_issue_trailers(msg), Vec::<String>::new());
280	}
281
282	use git_collab::commit_link::collect_linked_shas;	200	use git_collab::commit_link::collect_linked_shas;
283		201
284	#[test]	202	#[test]

tests/crdt_test.rs

Old		New
@@ -362,7 +362,9 @@ fn concurrent_patch_close_merge_higher_clock_wins() {
362	let merge = Event {	362	let merge = Event {
363	timestamp: "2026-01-03T00:00:00Z".to_string(),	363	timestamp: "2026-01-03T00:00:00Z".to_string(),
364	author: bob(),	364	author: bob(),
365	action: Action::PatchMerge,	365	action: Action::PatchMerge {
		366	commit: String::new(),
		367	},
366	clock: 0,	368	clock: 0,
367	};	369	};
368	dag::append_event(&repo, remote_ref, &merge, &sk).unwrap();	370	dag::append_event(&repo, remote_ref, &merge, &sk).unwrap();

tests/merge_recording_test.rs

@@ @@ -0,0 +1,637 @@ @@
+//! Merges are recorded as events, not derived on read.
+//!
+//! See: docs/superpowers/specs/2026-08-09-merge-recording-design.md
+//!
+//! Layer 3 (`patch merge`) and layer 2 (sync scanning `Patch:` trailers) are
+//! covered here, along with the demotion of reachability detection to a hint
+//! that never writes. Layer 1 (stamping the trailer — the `commit-msg` hook
+//! and `patch create --stamp`) is deliberately not implemented, so every test
+//! below writes the trailer by hand, which is exactly what a user without the
+//! hook does.
+mod common;
+use std::process::Command;
+use serde_json::Value;
+use tempfile::TempDir;
+use common::TestRepo;
+// ---------------------------------------------------------------------------
+// Harness
+// ---------------------------------------------------------------------------
+/// A `TestRepo` with a local bare `origin` it can sync against. Never a real
+/// network remote. The returned `TempDir` owns the bare repo — keep it alive.
+fn repo_with_origin() -> (TestRepo, TempDir) {
+    let bare = TempDir::new().unwrap();
+    let status = Command::new("git")
+        .args(["init", "--bare", "-b", "main"])
+        .arg(bare.path())
+        .status()
+        .unwrap();
+    assert!(status.success());
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    repo.git(&["remote", "add", "origin", bare.path().to_str().unwrap()]);
+    repo.git(&["push", "-u", "origin", "main"]);
+    repo.run_ok(&["init"]);
+    (repo, bare)
+}
+fn show_json(repo: &TestRepo, id: &str) -> Value {
+    serde_json::from_str(&repo.run_ok(&["patch", "show", id, "--json"])).unwrap()
+}
+fn issue_json(repo: &TestRepo, id: &str) -> Value {
+    serde_json::from_str(&repo.run_ok(&["issue", "show", id, "--json"])).unwrap()
+}
+/// Create a branch with one commit whose message carries `Patch: <id>`, and a
+/// patch for it. Returns the patch's short id. The trailer has to be written
+/// before the patch exists to know the id, so this creates the patch first and
+/// then amends the trailer in — which is what `patch create --stamp` (layer 1,
+/// out of scope) would do for the user.
+fn patch_with_trailer(repo: &TestRepo, branch: &str, file: &str) -> String {
+    repo.git(&["checkout", "-b", branch]);
+    repo.commit_file(file, "content", &format!("work on {}", branch));
+    let out = repo.run_ok(&["patch", "create", "-t", branch, "-B", branch]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    stamp_head(repo, &short);
+    repo.run_ok(&["patch", "revise", &short, "-b", "stamped"]);
+    repo.git(&["checkout", "main"]);
+    short
+}
+/// Amend HEAD's message to carry a `Patch:` trailer.
+fn stamp_head(repo: &TestRepo, patch_id: &str) {
+    let message = repo.git(&["log", "-1", "--format=%B"]);
+    let stamped = format!("{}\n\nPatch: {}\n", message.trim_end(), patch_id);
+    repo.git(&["commit", "--amend", "-m", &stamped]);
+}
+/// Squash-merge `branch` into the current branch, keeping the source commit's
+/// message (and therefore its trailer) verbatim.
+fn squash_merge_keeping_message(repo: &TestRepo, branch: &str) {
+    let message = repo.git(&["log", "-1", "--format=%B", branch]);
+    repo.git(&["merge", "--squash", branch]);
+    repo.git(&["commit", "-m", message.trim_end()]);
+}
+fn count_events(repo: &TestRepo, ref_name: &str) -> usize {
+    repo.git(&["rev-list", "--count", ref_name])
+        .trim()
+        .parse()
+        .unwrap()
+}
+fn patch_events_ref(repo: &TestRepo, short: &str) -> String {
+    let out = repo.git(&["for-each-ref", "--format=%(refname)", "refs/collab/patches/"]);
+    out.lines()
+        .find(|r| r.contains(short) && r.ends_with("/events"))
+        .unwrap_or_else(|| panic!("no events ref for {} in {}", short, out))
+        .to_string()
+}
+// ---------------------------------------------------------------------------
+// The event
+// ---------------------------------------------------------------------------
+#[test]
+fn patch_merge_event_carries_the_commit_that_landed_it() {
+    use git_collab::event::{Action, Author, Event};
+    let event = Event {
+        timestamp: "2026-08-09T12:00:00Z".to_string(),
+        author: Author {
+            name: "Alice".to_string(),
+            email: "alice@example.com".to_string(),
+        },
+        action: Action::PatchMerge {
+            commit: "4b2e1cd0123456789012345678901234567890ab".to_string(),
+        },
+        clock: 4,
+    };
+    let json = serde_json::to_string(&event).unwrap();
+    assert!(json.contains("\"type\":\"patch.merge\""), "{}", json);
+    assert!(
+        json.contains("\"commit\":\"4b2e1cd0123456789012345678901234567890ab\""),
+        "{}",
+        json
+    );
+    let parsed: Event = serde_json::from_str(&json).unwrap();
+    match parsed.action {
+        Action::PatchMerge { commit } => {
+            assert_eq!(commit, "4b2e1cd0123456789012345678901234567890ab")
+        }
+        other => panic!("expected PatchMerge, got {:?}", other),
+    }
+}
+#[test]
+fn a_patch_merge_written_before_the_commit_field_existed_still_reads() {
+    // Signatures are checked by re-serializing, so a legacy `patch.merge` must
+    // both deserialize and round-trip back to the bytes its signer produced.
+    use git_collab::event::{Action, Event};
+    let raw = r#"{"timestamp":"2026-01-01T00:00:00Z","author":{"name":"A","email":"a@b.c"},"action":{"type":"patch.merge"},"clock":2}"#;
+    let parsed: Event = serde_json::from_str(raw).unwrap();
+    match &parsed.action {
+        Action::PatchMerge { commit } => assert_eq!(commit, ""),
+        other => panic!("expected PatchMerge, got {:?}", other),
+    }
+    let round_tripped = serde_json::to_string(&parsed).unwrap();
+    assert!(
+        !round_tripped.contains("\"commit\""),
+        "an empty commit must not be written back, or legacy signatures break: {}",
+        round_tripped
+    );
+}
+// ---------------------------------------------------------------------------
+// Layer 3: `patch merge`
+// ---------------------------------------------------------------------------
+#[test]
+fn patch_merge_records_the_merge() {
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the patch");
+    let out = repo.run_ok(&["patch", "create", "-t", "Feature", "-B", "feat"]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    repo.git(&["checkout", "main"]);
+    repo.git(&["merge", "--ff-only", "feat"]);
+    repo.run_ok(&["patch", "merge", &short]);
+    let json = show_json(&repo, &short);
+    assert_eq!(json["status"], "merged");
+}
+#[test]
+fn patch_merge_records_the_base_tip_as_the_landing_commit() {
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the patch");
+    let out = repo.run_ok(&["patch", "create", "-t", "Feature", "-B", "feat"]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    repo.git(&["checkout", "main"]);
+    squash_merge_keeping_message(&repo, "feat");
+    let landed = repo.git(&["rev-parse", "main"]).trim().to_string();
+    repo.run_ok(&["patch", "merge", &short]);
+    let log = repo.run_ok(&["log"]);
+    assert!(
+        log.contains(&landed[..7]),
+        "the recorded merge commit should be the base tip {}: {}",
+        &landed[..7],
+        log
+    );
+}
+#[test]
+fn merged_state_survives_branch_deletion_and_a_cold_cache() {
+    // The case that fails without recording: deleting the merged branch used
+    // to send the patch back to Open the moment the state cache was cleared.
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the patch");
+    let out = repo.run_ok(&["patch", "create", "-t", "Feature", "-B", "feat"]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    repo.git(&["checkout", "main"]);
+    repo.git(&["merge", "--ff-only", "feat"]);
+    repo.run_ok(&["patch", "merge", &short]);
+    repo.git(&["branch", "-D", "feat"]);
+    let cache = repo.dir.path().join(".git").join("collab").join("cache");
+    if cache.exists() {
+        std::fs::remove_dir_all(&cache).unwrap();
+    }
+    assert_eq!(show_json(&repo, &short)["status"], "merged");
+}
+#[test]
+fn patch_merge_twice_records_nothing_the_second_time() {
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the patch");
+    let out = repo.run_ok(&["patch", "create", "-t", "Feature", "-B", "feat"]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    repo.git(&["checkout", "main"]);
+    repo.git(&["merge", "--ff-only", "feat"]);
+    repo.run_ok(&["patch", "merge", &short]);
+    let events_ref = patch_events_ref(&repo, &short);
+    let after_first = count_events(&repo, &events_ref);
+    let out = repo.run_ok(&["patch", "merge", &short]);
+    assert!(
+        out.contains("already"),
+        "a second merge should say so, got: {}",
+        out
+    );
+    assert_eq!(
+        count_events(&repo, &events_ref),
+        after_first,
+        "recording a merge twice must append nothing the second time"
+    );
+}
+#[test]
+fn patch_merge_closes_the_fixed_issue() {
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    let issue = repo.issue_open("Broken thing");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the fix");
+    let out = repo.run_ok(&[
+        "patch", "create", "-t", "The fix", "-B", "feat", "--fixes", &issue,
+    ]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    repo.git(&["checkout", "main"]);
+    repo.git(&["merge", "--ff-only", "feat"]);
+    repo.run_ok(&["patch", "merge", &short]);
+    assert_eq!(issue_json(&repo, &issue)["status"], "closed");
+}
+// ---------------------------------------------------------------------------
+// Layer 2: sync scans `Patch:` trailers on the patch's own base branch
+// ---------------------------------------------------------------------------
+#[test]
+fn sync_records_a_squash_merge_whose_trailer_survived() {
+    let (repo, _bare) = repo_with_origin();
+    let short = patch_with_trailer(&repo, "feat", "a.txt");
+    squash_merge_keeping_message(&repo, "feat");
+    assert_eq!(
+        show_json(&repo, &short)["status"],
+        "open",
+        "precondition: reachability cannot see a squash"
+    );
+    repo.run_ok(&["sync"]);
+    assert_eq!(show_json(&repo, &short)["status"], "merged");
+}
+#[test]
+fn sync_does_not_record_a_squash_whose_message_was_rewritten() {
+    let (repo, _bare) = repo_with_origin();
+    let short = patch_with_trailer(&repo, "feat", "a.txt");
+    repo.git(&["merge", "--squash", "feat"]);
+    repo.git(&["commit", "-m", "a message with no trailer at all"]);
+    repo.run_ok(&["sync"]);
+    assert_eq!(show_json(&repo, &short)["status"], "open");
+    // Layer 3 is what covers this.
+    repo.run_ok(&["patch", "merge", &short]);
+    assert_eq!(show_json(&repo, &short)["status"], "merged");
+}
+#[test]
+fn a_second_sync_after_a_recorded_merge_emits_nothing() {
+    let (repo, _bare) = repo_with_origin();
+    let short = patch_with_trailer(&repo, "feat", "a.txt");
+    squash_merge_keeping_message(&repo, "feat");
+    repo.run_ok(&["sync"]);
+    let events_ref = patch_events_ref(&repo, &short);
+    let after_first = count_events(&repo, &events_ref);
+    repo.run_ok(&["sync"]);
+    assert_eq!(
+        count_events(&repo, &events_ref),
+        after_first,
+        "the trailer is still in history; the second scan must emit nothing"
+    );
+}
+#[test]
+fn a_trailer_on_a_branch_that_is_not_the_patches_base_does_not_record_a_merge() {
+    let (repo, _bare) = repo_with_origin();
+    let short = patch_with_trailer(&repo, "feat", "a.txt");
+    // Land the patch on a *different* branch. `main` — the patch's base — never
+    // sees the trailer.
+    repo.git(&["checkout", "-b", "someone-elses-branch"]);
+    squash_merge_keeping_message(&repo, "feat");
+    repo.git(&["checkout", "main"]);
+    repo.run_ok(&["sync"]);
+    assert_eq!(show_json(&repo, &short)["status"], "open");
+}
+#[test]
+fn an_unknown_patch_id_in_a_trailer_warns_and_leaves_sync_successful() {
+    let (repo, _bare) = repo_with_origin();
+    // A base branch with no open patches is not walked at all, so there has to
+    // be something for the walk to be for.
+    let open = patch_with_trailer(&repo, "feat", "a.txt");
+    repo.commit_file(
+        "x.txt",
+        "x",
+        "land something\n\nPatch: ffffffffffffffffffffffff",
+    );
+    let out = repo.run(&["sync"]);
+    assert!(out.status.success(), "sync must not fail on a bad trailer");
+    let stderr = String::from_utf8(out.stderr).unwrap();
+    assert!(
+        stderr.contains("ffffffff"),
+        "expected a warning naming the unresolvable id, got: {}",
+        stderr
+    );
+    assert_eq!(show_json(&repo, &open)["status"], "open");
+}
+/// Give the patch whose events ref is `events_ref` a second ref under a
+/// near-identical id, so any 8-character prefix of the real id matches two
+/// patches. Random ids practically never collide, so ambiguity has to be
+/// constructed.
+fn duplicate_patch_ref_with_colliding_id(repo: &TestRepo, events_ref: &str) -> String {
+    let id = events_ref
+        .strip_prefix("refs/collab/patches/")
+        .and_then(|r| r.strip_suffix("/events"))
+        .unwrap();
+    // Flip the last hex digit: everything up to it still matches.
+    let last = id.chars().last().unwrap();
+    let replacement = if last == '0' { '1' } else { '0' };
+    let twin: String = id[..id.len() - 1].chars().chain([replacement]).collect();
+    let tip = repo.git(&["rev-parse", events_ref]).trim().to_string();
+    repo.git(&[
+        "update-ref",
+        &format!("refs/collab/patches/{}/events", twin),
+        &tip,
+    ]);
+    twin
+}
+#[test]
+fn an_ambiguous_patch_prefix_in_a_trailer_warns_and_leaves_sync_successful() {
+    let (repo, _bare) = repo_with_origin();
+    let short = patch_with_trailer(&repo, "feat", "a.txt");
+    let events_ref = patch_events_ref(&repo, &short);
+    duplicate_patch_ref_with_colliding_id(&repo, &events_ref);
+    repo.commit_file("x.txt", "x", &format!("land something\n\nPatch: {}", short));
+    let out = repo.run(&["sync"]);
+    assert!(
+        out.status.success(),
+        "sync must not fail on an ambiguous trailer"
+    );
+    let stderr = String::from_utf8(out.stderr).unwrap();
+    assert!(
+        stderr.contains("ambiguous"),
+        "expected an ambiguity warning, got: {}",
+        stderr
+    );
+    assert_eq!(count_events(&repo, &events_ref), 2, "nothing was recorded");
+}
+#[test]
+fn a_trailer_naming_an_archived_patch_warns_and_leaves_sync_successful() {
+    let (repo, _bare) = repo_with_origin();
+    // One patch stays open so `main` is walked at all; the other is archived.
+    let _open = patch_with_trailer(&repo, "still-open", "b.txt");
+    let short = patch_with_trailer(&repo, "feat", "a.txt");
+    // `patch close` archives the ref.
+    repo.run_ok(&["patch", "close", &short]);
+    squash_merge_keeping_message(&repo, "feat");
+    let out = repo.run(&["sync"]);
+    assert!(
+        out.status.success(),
+        "sync must not fail on an archived patch"
+    );
+    let stderr = String::from_utf8(out.stderr).unwrap();
+    assert!(
+        stderr.contains("archived"),
+        "expected an archived-patch warning, got: {}",
+        stderr
+    );
+    assert_eq!(show_json(&repo, &short)["status"], "closed");
+}
+// ---------------------------------------------------------------------------
+// `--fixes` closes its issue in the same operation that records the merge
+// ---------------------------------------------------------------------------
+#[test]
+fn fixes_closes_the_issue_exactly_once_across_repeated_syncs() {
+    let (repo, _bare) = repo_with_origin();
+    let issue = repo.issue_open("Broken thing");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the fix");
+    let out = repo.run_ok(&[
+        "patch", "create", "-t", "The fix", "-B", "feat", "--fixes", &issue,
+    ]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    stamp_head(&repo, &short);
+    repo.run_ok(&["patch", "revise", &short, "-b", "stamped"]);
+    repo.git(&["checkout", "main"]);
+    squash_merge_keeping_message(&repo, "feat");
+    repo.run_ok(&["sync"]);
+    assert_eq!(issue_json(&repo, &issue)["status"], "closed");
+    let issue_ref = repo
+        .git(&[
+            "for-each-ref",
+            "--format=%(refname)",
+            "refs/collab/archive/issues/",
+        ])
+        .lines()
+        .find(|r| r.contains(&issue))
+        .unwrap()
+        .to_string();
+    let after_first = count_events(&repo, &issue_ref);
+    repo.run_ok(&["sync"]);
+    repo.run_ok(&["sync"]);
+    assert_eq!(
+        count_events(&repo, &issue_ref),
+        after_first,
+        "repeated syncs must not append a second IssueClose"
+    );
+}
+#[test]
+fn a_fixes_issue_that_is_already_closed_gets_no_further_close() {
+    let (repo, _bare) = repo_with_origin();
+    let issue = repo.issue_open("Broken thing");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the fix");
+    let out = repo.run_ok(&[
+        "patch", "create", "-t", "The fix", "-B", "feat", "--fixes", &issue,
+    ]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    stamp_head(&repo, &short);
+    repo.run_ok(&["patch", "revise", &short, "-b", "stamped"]);
+    repo.git(&["checkout", "main"]);
+    repo.issue_close(&issue);
+    let issue_ref = repo
+        .git(&[
+            "for-each-ref",
+            "--format=%(refname)",
+            "refs/collab/archive/issues/",
+        ])
+        .lines()
+        .find(|r| r.contains(&issue))
+        .unwrap()
+        .to_string();
+    let before = count_events(&repo, &issue_ref);
+    squash_merge_keeping_message(&repo, "feat");
+    repo.run_ok(&["sync"]);
+    assert_eq!(show_json(&repo, &short)["status"], "merged");
+    assert_eq!(
+        count_events(&repo, &issue_ref),
+        before,
+        "the merge is recorded, but an already-closed issue gets no IssueClose"
+    );
+}
+#[test]
+fn a_close_that_did_not_happen_with_the_merge_is_retried_by_the_next_scan() {
+    // The merge and the close write to different refs, so they are not atomic.
+    // If the close does not land, the merge still stands — and the next scan,
+    // seeing a merged patch with an open `fixes` issue, must retry rather than
+    // leave the issue open forever. Simulated here by recording the merge with
+    // `patch merge --no-close`, which is precisely the "merge landed, close did
+    // not" state.
+    let (repo, _bare) = repo_with_origin();
+    let issue = repo.issue_open("Broken thing");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the fix");
+    let out = repo.run_ok(&[
+        "patch", "create", "-t", "The fix", "-B", "feat", "--fixes", &issue,
+    ]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    stamp_head(&repo, &short);
+    repo.run_ok(&["patch", "revise", &short, "-b", "stamped"]);
+    repo.git(&["checkout", "main"]);
+    squash_merge_keeping_message(&repo, "feat");
+    repo.run_ok(&["patch", "merge", &short, "--no-close"]);
+    assert_eq!(show_json(&repo, &short)["status"], "merged");
+    assert_eq!(
+        issue_json(&repo, &issue)["status"],
+        "open",
+        "precondition: the merge landed and the close did not"
+    );
+    repo.run_ok(&["sync"]);
+    assert_eq!(
+        issue_json(&repo, &issue)["status"],
+        "closed",
+        "the next scan must retry the close it could not make atomic"
+    );
+}
+// ---------------------------------------------------------------------------
+// Reachability is a hint, and never writes
+// ---------------------------------------------------------------------------
+#[test]
+fn a_reachable_patch_with_no_merge_event_shows_as_merged_with_a_question_mark() {
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the patch");
+    let out = repo.run_ok(&["patch", "create", "-t", "Feature", "-B", "feat"]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    repo.git(&["checkout", "main"]);
+    repo.git(&["merge", "--ff-only", "feat"]);
+    let list = repo.run_ok(&["patch", "list"]);
+    assert!(
+        list.contains("merged?"),
+        "a reachable patch with no recorded merge is a hint, not a fact: {}",
+        list
+    );
+    assert_eq!(
+        show_json(&repo, &short)["status"],
+        "open",
+        "the hint must not become the status"
+    );
+}
+#[test]
+fn displaying_a_reachable_patch_writes_no_event() {
+    // The governing constraint: recording is never a side effect of a read.
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the patch");
+    let out = repo.run_ok(&["patch", "create", "-t", "Feature", "-B", "feat"]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    repo.git(&["checkout", "main"]);
+    repo.git(&["merge", "--ff-only", "feat"]);
+    let events_ref = patch_events_ref(&repo, &short);
+    let before = count_events(&repo, &events_ref);
+    let tip_before = repo.git(&["rev-parse", &events_ref]).trim().to_string();
+    repo.run_ok(&["patch", "list"]);
+    repo.run_ok(&["patch", "show", &short]);
+    repo.run_ok(&["patch", "show", &short, "--json"]);
+    repo.run_ok(&["patch", "log", &short]);
+    assert_eq!(count_events(&repo, &events_ref), before);
+    assert_eq!(
+        repo.git(&["rev-parse", &events_ref]).trim(),
+        tip_before,
+        "reading a patch must not move its ref"
+    );
+}
+#[test]
+fn sync_names_the_patches_that_look_merged_but_are_not_recorded() {
+    let (repo, _bare) = repo_with_origin();
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the patch");
+    let out = repo.run_ok(&["patch", "create", "-t", "Feature", "-B", "feat"]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    repo.git(&["checkout", "main"]);
+    repo.git(&["merge", "--ff-only", "feat"]);
+    let out = repo.run_ok(&["sync"]);
+    assert!(
+        out.contains(&short) && out.contains("patch merge"),
+        "sync should name the patch and the command that records it: {}",
+        out
+    );
+}
+#[test]
+fn a_recorded_merge_is_not_reported_as_a_hint() {
+    let repo = TestRepo::new("Alice", "alice@example.com");
+    repo.git(&["checkout", "-b", "feat"]);
+    repo.commit_file("a.txt", "x", "the patch");
+    let out = repo.run_ok(&["patch", "create", "-t", "Feature", "-B", "feat"]);
+    let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
+    repo.git(&["checkout", "main"]);
+    repo.git(&["merge", "--ff-only", "feat"]);
+    repo.run_ok(&["patch", "merge", &short]);
+    let list = repo.run_ok(&["patch", "list", "--all"]);
+    assert!(list.contains("merged"), "{}", list);
+    assert!(
+        !list.contains("merged?"),
+        "a recorded merge is a fact, not a hint: {}",
+        list
+    );
+}

tests/revision_refs_test.rs

Old		New
@@ -59,6 +59,16 @@ fn show_json(repo: &TestRepo, id: &str) -> serde_json::Value {
59	serde_json::from_str(&out).unwrap()	59	serde_json::from_str(&out).unwrap()
60	}	60	}
61		61
		62	/// Whether reachability detection fires for this patch.
		63	///
		64	/// It is a hint, not a status: a merge is recorded by `Action::PatchMerge`, and
		65	/// detection only ever suggests — it cannot see a squash, so its silence
		66	/// proves nothing, and it never writes. `patch show` renders the suggestion as
		67	/// `[merged?]`, which is what these tests read.
		68	fn looks_merged(repo: &TestRepo, id: &str) -> bool {
		69	repo.run_ok(&["patch", "show", id]).contains("[merged?")
		70	}
		71
62	/// Create a patch on a fresh branch holding one commit. Returns (short id, full	72	/// Create a patch on a fresh branch holding one commit. Returns (short id, full
63	/// id, tip commit).	73	/// id, tip commit).
64	fn patch_on_branch(repo: &TestRepo, branch: &str, file: &str) -> (String, String, String) {	74	fn patch_on_branch(repo: &TestRepo, branch: &str, file: &str) -> (String, String, String) {
@@ -334,51 +344,77 @@ fn two_branches_with_generated_names_get_their_own_patches() {
334	}	344	}
335		345
336	// ===========================================================================	346	// ===========================================================================
337	// Merge detection, anchored to the latest revision's base	347	// The merge hint, anchored to the latest revision's base
		348	//
		349	// These exercise reachability detection, which no longer decides the status —
		350	// it only raises `merged?`. The discrimination it has to get right is the same
		351	// either way, so the assertions moved from the status to the hint.
338	// ===========================================================================	352	// ===========================================================================
339		353
340	#[test]	354	#[test]
341	fn merging_the_base_branch_forward_marks_the_patch_merged() {	355	fn merging_the_base_branch_forward_makes_the_patch_look_merged() {
342	let repo = TestRepo::new("Alice", "alice@example.com");	356	let repo = TestRepo::new("Alice", "alice@example.com");
343	let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");	357	let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
344		358
345	repo.git(&["checkout", "main"]);	359	repo.git(&["checkout", "main"]);
346	repo.git(&["merge", "--ff-only", "feat"]);	360	repo.git(&["merge", "--ff-only", "feat"]);
347		361
348	assert_eq!(show_json(&repo, &short)["status"], "merged");	362	assert!(looks_merged(&repo, &short));
		363	assert_eq!(
		364	show_json(&repo, &short)["status"], "open",
		365	"the hint is not the status"
		366	);
349	}	367	}
350		368
351	#[test]	369	#[test]
352	fn an_unmerged_patch_stays_open() {	370	fn an_unmerged_patch_raises_no_hint() {
353	// Guards the failure mode where dropping `base_commit` leaves `base_moved`	371	// Guards the failure mode where dropping `base_commit` leaves `base_moved`
354	// permanently false — or permanently true — and merge detection silently	372	// permanently false — or permanently true — and the hint silently stops
355	// stops telling the truth.	373	// telling the truth.
356	let repo = TestRepo::new("Alice", "alice@example.com");	374	let repo = TestRepo::new("Alice", "alice@example.com");
357	let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");	375	let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
358		376
359	repo.git(&["checkout", "main"]);	377	repo.git(&["checkout", "main"]);
360	repo.commit_file("unrelated.txt", "x", "main moves on its own");	378	repo.commit_file("unrelated.txt", "x", "main moves on its own");
361		379
		380	assert!(!looks_merged(&repo, &short));
362	assert_eq!(show_json(&repo, &short)["status"], "open");	381	assert_eq!(show_json(&repo, &short)["status"], "open");
363	}	382	}
364		383
365	#[test]	384	#[test]
366	fn merge_detection_survives_deleting_the_source_branch() {	385	fn the_merge_hint_survives_deleting_the_source_branch() {
367	// Merge detection used to go through `refs/heads/<branch>` and no-op	386	// The hint used to go through `refs/heads/<branch>` and no-op silently when
368	// silently when it was absent, so deleting the branch after merging left	387	// it was absent, so deleting the branch after merging left the patch with
369	// the patch Open forever with no diagnostic.	388	// no diagnostic at all.
		389	let repo = TestRepo::new("Alice", "alice@example.com");
		390	let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
		391
		392	repo.git(&["checkout", "main"]);
		393	repo.git(&["merge", "--ff-only", "feat"]);
		394	repo.git(&["branch", "-D", "feat"]);
		395
		396	assert!(looks_merged(&repo, &short));
		397	}
		398
		399	#[test]
		400	fn a_recorded_merge_survives_deleting_the_source_branch() {
		401	// The hint above is a convenience. This is the fact: once recorded, merged
		402	// state does not depend on the branch, or on the base branch, existing at
		403	// all — which is the case that used to revert to Open the moment the state
		404	// cache went cold.
370	let repo = TestRepo::new("Alice", "alice@example.com");	405	let repo = TestRepo::new("Alice", "alice@example.com");
371	let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");	406	let (short, _id, _tip) = patch_on_branch(&repo, "feat", "a.txt");
372		407
373	repo.git(&["checkout", "main"]);	408	repo.git(&["checkout", "main"]);
374	repo.git(&["merge", "--ff-only", "feat"]);	409	repo.git(&["merge", "--ff-only", "feat"]);
		410	repo.run_ok(&["patch", "merge", &short]);
375	repo.git(&["branch", "-D", "feat"]);	411	repo.git(&["branch", "-D", "feat"]);
376		412
377	assert_eq!(show_json(&repo, &short)["status"], "merged");	413	assert_eq!(show_json(&repo, &short)["status"], "merged");
378	}	414	}
379		415
380	#[test]	416	#[test]
381	fn merge_detection_reads_the_latest_revisions_base_not_the_first() {	417	fn the_merge_hint_reads_the_latest_revisions_base_not_the_first() {
382	// After a rebase the patch's base moves. Detection must compare against	418	// After a rebase the patch's base moves. Detection must compare against
383	// where the latest revision stood, not where revision 1 did.	419	// where the latest revision stood, not where revision 1 did.
384	let repo = TestRepo::new("Alice", "alice@example.com");	420	let repo = TestRepo::new("Alice", "alice@example.com");
@@ -390,14 +426,11 @@ fn merge_detection_reads_the_latest_revisions_base_not_the_first() {
390	repo.git(&["rebase", "main"]);	426	repo.git(&["rebase", "main"]);
391	repo.run_ok(&["patch", "revise", &short, "-b", "rebased"]);	427	repo.run_ok(&["patch", "revise", &short, "-b", "rebased"]);
392		428
393	assert_eq!(	429	assert!(!looks_merged(&repo, &short), "rebasing is not merging");
394	show_json(&repo, &short)["status"], "open",
395	"rebasing is not merging"
396	);
397		430
398	repo.git(&["checkout", "main"]);	431	repo.git(&["checkout", "main"]);
399	repo.git(&["merge", "--ff-only", "feat"]);	432	repo.git(&["merge", "--ff-only", "feat"]);
400	assert_eq!(show_json(&repo, &short)["status"], "merged");	433	assert!(looks_merged(&repo, &short));
401	}	434	}
402		435
403	fn tree_of(repo: &TestRepo, commit: &str) -> String {	436	fn tree_of(repo: &TestRepo, commit: &str) -> String {
@@ -465,9 +498,9 @@ fn a_migrated_patch_merged_by_exact_fast_forward_is_detected() {
465	// The latest revision predates `base`, which is the normal shape of a	498	// The latest revision predates `base`, which is the normal shape of a
466	// migrated patch. In the exact fast-forward case — main fast-forwarded onto	499	// migrated patch. In the exact fast-forward case — main fast-forwarded onto
467	// the patch head, so base tip == head — recomputing a merge-base yields the	500	// the patch head, so base tip == head — recomputing a merge-base yields the
468	// head itself, so the base looks like it never moved and the merged patch	501	// head itself, so the base looks like it never moved and the merge goes
469	// stays Open forever. That is the commonest merge for a single-commit	502	// unnoticed. That is the commonest merge for a single-commit patch, so it
470	// patch, so it has to come from the base that was actually recorded.	503	// has to come from the base that was actually recorded.
471	let repo = TestRepo::new("Alice", "alice@example.com");	504	let repo = TestRepo::new("Alice", "alice@example.com");
472	let base = repo.git(&["rev-parse", "main"]).trim().to_string();	505	let base = repo.git(&["rev-parse", "main"]).trim().to_string();
473	repo.git(&["checkout", "-b", "feat"]);	506	repo.git(&["checkout", "-b", "feat"]);
@@ -484,7 +517,7 @@ fn a_migrated_patch_merged_by_exact_fast_forward_is_detected() {
484	"precondition: base tip and patch head are the same commit"	517	"precondition: base tip and patch head are the same commit"
485	);	518	);
486		519
487	assert_eq!(show_json(&repo, &id[..8])["status"], "merged");	520	assert!(looks_merged(&repo, &id[..8]));
488	}	521	}
489		522
490	#[test]	523	#[test]
@@ -509,7 +542,8 @@ fn a_patch_created_on_the_base_branch_is_not_reported_merged() {
509	// The degenerate case `base_moved` exists for: the recorded base IS the	542	// The degenerate case `base_moved` exists for: the recorded base IS the
510	// patch's own head, so the head is trivially reachable from the base tip	543	// patch's own head, so the head is trivially reachable from the base tip
511	// without anything having been merged. Guards against widening the	544	// without anything having been merged. Guards against widening the
512	// unknown-base handling until it swallows this.	545	// unknown-base handling until it swallows this. It stays a hint either
		546	// way, but a hint that fires on every patch is worse than none.
513	let repo = TestRepo::new("Alice", "alice@example.com");	547	let repo = TestRepo::new("Alice", "alice@example.com");
514	let tip = repo.git(&["rev-parse", "main"]).trim().to_string();	548	let tip = repo.git(&["rev-parse", "main"]).trim().to_string();
515	repo.git(&["branch", "feat"]);	549	repo.git(&["branch", "feat"]);
@@ -541,6 +575,7 @@ fn a_patch_created_on_the_base_branch_is_not_reported_merged() {
541	.unwrap();	575	.unwrap();
542	drop(git_repo);	576	drop(git_repo);
543		577
		578	assert!(!looks_merged(&repo, &id[..8]));
544	assert_eq!(show_json(&repo, &id[..8])["status"], "open");	579	assert_eq!(show_json(&repo, &id[..8])["status"], "open");
545	}	580	}
546		581
@@ -604,11 +639,9 @@ fn merge_detection_reads_the_base_of_the_revision_it_resolved_the_head_from() {
604		639
605	repo.git(&["merge", "--ff-only", "feat"]);	640	repo.git(&["merge", "--ff-only", "feat"]);
606		641
607	let json = show_json(&repo, &id[..8]);	642	assert!(
608	assert_eq!(	643	looks_merged(&repo, &id[..8]),
609	json["status"], "merged",	644	"head came from r1, so the base must come from r1 too"
610	"head came from r1, so the base must come from r1 too: {}",
611	json
612	);	645	);
613	}	646	}
614		647

tests/server_behavior_test.rs

Old		New
@@ -267,7 +267,7 @@ fn patches_list_defaults_to_open_and_offers_closed_and_merged_filters() {
267	"content for merged patch",	267	"content for merged patch",
268	"add file for merged patch",	268	"add file for merged patch",
269	);	269	);
270	harness.work_repo().run_ok(&[	270	let merged_out = harness.work_repo().run_ok(&[
271	"patch",	271	"patch",
272	"create",	272	"create",
273	"-t",	273	"-t",
@@ -275,15 +275,19 @@ fn patches_list_defaults_to_open_and_offers_closed_and_merged_filters() {
275	"-B",	275	"-B",
276	"feature-merged",	276	"feature-merged",
277	]);	277	]);
		278	let merged_id = merged_out
		279	.trim()
		280	.strip_prefix("Created patch ")
		281	.unwrap()
		282	.to_string();
278	harness.work_repo().git(&["checkout", "main"]);	283	harness.work_repo().git(&["checkout", "main"]);
279	harness.work_repo().git(&["merge", "feature-merged"]);	284	harness.work_repo().git(&["merge", "feature-merged"]);
		285	// Merged state is recorded, so it travels with the collab refs. The server
		286	// does not have to re-derive it — which it could not do for a squash, and
		287	// which used to make this test push `feature-merged` so the server had a
		288	// branch to resolve.
		289	harness.work_repo().run_ok(&["patch", "merge", &merged_id]);
280		290
281	// Auto-merge detection resolves the patch's source branch on the server
282	// side too (`refs/heads/feature-merged`), so it needs to be pushed
283	// alongside the fast-forwarded main.
284	harness
285	.work_repo()
286	.git(&["push", "origin", "feature-merged"]);
287	harness.push_head();	291	harness.push_head();
288	harness.push_collab_refs();	292	harness.push_collab_refs();
289		293

tests/status_test.rs

Old		New
@@ -25,7 +25,9 @@ fn merge_patch(repo: &git2::Repository, ref_name: &str, author: &git_collab::eve
25	let event = git_collab::event::Event {	25	let event = git_collab::event::Event {
26	timestamp: common::now(),	26	timestamp: common::now(),
27	author: author.clone(),	27	author: author.clone(),
28	action: git_collab::event::Action::PatchMerge,	28	action: git_collab::event::Action::PatchMerge {
		29	commit: String::new(),
		30	},
29	clock: 0,	31	clock: 0,
30	};	32	};
31	git_collab::dag::append_event(repo, ref_name, &event, &sk).unwrap();	33	git_collab::dag::append_event(repo, ref_name, &event, &sk).unwrap();

tests/trailer_test.rs

@@ @@ -0,0 +1,157 @@ @@
+//! One set of tests for the one git-trailer parser.
+//!
+//! `Issue:` (commit linking) and `Patch:` (merge recording) share
+//! `trailer::parse_trailers`; every case below is run against both tokens so a
+//! change that fixes one and breaks the other cannot pass.
+use git_collab::trailer::parse_trailers;
+/// Run one parser case against both tokens that use it. `message` is written
+/// with `{}` where the token goes, and `expected` is the list of values the
+/// parser must return.
+fn both_tokens(message_template: &str, expected: &[&str]) {
+    for token in ["issue", "patch"] {
+        // Capitalized in the message, lowercase as the token argument: the
+        // match is case-insensitive.
+        let capitalized = format!("{}{}", token[..1].to_uppercase(), &token[1..]);
+        let message = message_template.replace("{}", &capitalized);
+        let got = parse_trailers(&message, token);
+        assert_eq!(
+            got,
+            expected.iter().map(|s| s.to_string()).collect::<Vec<_>>(),
+            "token {} on message {:?}",
+            token,
+            message
+        );
+    }
+}
+#[test]
+fn no_trailer_block() {
+    both_tokens("Just a plain commit", &[]);
+}
+#[test]
+fn empty_message() {
+    both_tokens("", &[]);
+}
+#[test]
+fn single_trailer_in_pure_block() {
+    both_tokens("Fix thing\n\nSome context in the body.\n\n{}: abc", &["abc"]);
+}
+#[test]
+fn case_and_spacing_variants() {
+    for token in ["issue", "patch"] {
+        let upper = token.to_uppercase();
+        assert_eq!(
+            parse_trailers(&format!("subject\n\n{}: abc", token), token),
+            vec!["abc".to_string()]
+        );
+        assert_eq!(
+            parse_trailers(&format!("subject\n\n{} : abc", upper), token),
+            vec!["abc".to_string()]
+        );
+        assert_eq!(
+            parse_trailers(&format!("subject\n\n  {}:  abc  ", upper), token),
+            vec!["abc".to_string()]
+        );
+    }
+}
+#[test]
+fn two_trailers_in_pure_block() {
+    both_tokens("subject\n\n{}: abc\n{}: def", &["abc", "def"]);
+}
+#[test]
+fn trailer_present_but_not_in_final_paragraph() {
+    // The final paragraph is the signed-off-by block. It is a valid trailer
+    // block, but it carries no matching key, so nothing is extracted.
+    both_tokens(
+        "subject\n\n{}: abc\n\nSigned-off-by: alice <a@example.com>",
+        &[],
+    );
+}
+#[test]
+fn wrong_key() {
+    for token in ["issue", "patch"] {
+        // `Issues:` / `Patchs:` — trailer-shaped, but not our token.
+        let msg = format!("subject\n\n{}s: abc", token);
+        assert_eq!(parse_trailers(&msg, token), Vec::<String>::new());
+    }
+}
+#[test]
+fn prose_mention() {
+    for token in ["issue", "patch"] {
+        let msg = format!("subject\n\nthis fixes {} abc in the body", token);
+        assert_eq!(parse_trailers(&msg, token), Vec::<String>::new());
+    }
+}
+#[test]
+fn single_paragraph_whole_message_is_trailer_block() {
+    both_tokens("{}: abc", &["abc"]);
+}
+#[test]
+fn mixed_final_paragraph_rejects_all() {
+    // A prose line in the final paragraph disqualifies the whole paragraph.
+    both_tokens("subject\n\nThanks to Bob for the catch.\n{}: a3f9", &[]);
+}
+#[test]
+fn trailing_whitespace_paragraph_does_not_shadow_trailer_block() {
+    both_tokens("subject\n\n{}: abc\n\n   \n", &["abc"]);
+}
+#[test]
+fn pure_block_with_mixed_keys() {
+    both_tokens(
+        "subject\n\nSigned-off-by: alice <a@example.com>\n{}: abc",
+        &["abc"],
+    );
+}
+#[test]
+fn rejects_value_with_interior_whitespace() {
+    // `Patch: abc merged by me` must parse to *nothing*, not silently to
+    // `abc`. Silently truncating would record a merge the author did not
+    // name; parsing to nothing makes the ignored commentary visible.
+    both_tokens("subject\n\n{}: abc merged by me", &[]);
+    both_tokens("subject\n\n{}: abc fixes thing", &[]);
+}
+#[test]
+fn rejects_empty_value() {
+    both_tokens("subject\n\n{}:   ", &[]);
+}
+#[test]
+fn a_trailer_for_the_other_token_is_not_matched() {
+    // The two tokens genuinely select: an `Issue:` line is not a `Patch:` one.
+    let msg = "subject\n\nIssue: abc\nPatch: def";
+    assert_eq!(parse_trailers(msg, "issue"), vec!["abc".to_string()]);
+    assert_eq!(parse_trailers(msg, "patch"), vec!["def".to_string()]);
+}
+#[test]
+fn squashed_message_keeps_only_the_final_trailer_block() {
+    // `git merge --squash` concatenates the source messages. Only the last
+    // commit's trailer block is the final paragraph — which is fine, because
+    // every commit of a patch carries the same id.
+    let msg = "\
+Squashed commit of the following:
+first commit
+Patch: aaaa1111
+second commit
+Patch: aaaa1111";
+    assert_eq!(parse_trailers(msg, "patch"), vec!["aaaa1111".to_string()]);
+}