Commit `04c4dc7d48`

04c4dc7d4890d13d42e396a1a9bc92d5bc55a4c5

parent: c23b28d133

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-06 20:24 UTC

Implementation plan for MR 4: merge request lifecycle

Layout: unified · split

docs/superpowers/plans/2026-09-06-mr04-mr-lifecycle.md added +409

		@@ -0,0 +1,409 @@
	1	# MR 4: Merge request lifecycle Implementation Plan
	2
	3	> For agentic workers: REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
	4
	5	Goal: Close four parity rows at once — `draft, ready`, `retarget`, `request a review`, and `stacked merge requests`.
	6
	7	Architecture: All four are already on the wire and none needs a server change. `mr show` and `mr list` carry `draft`, `review_requests`, `stacked_on` and `stacked` as `omitempty` fields the iOS models simply do not decode yet. So this is decoding plus four write actions on a screen that already has a write path.
	8
	9	Tech Stack: Swift 6, default `MainActor` isolation, iOS 26.5, SwiftUI, Swift Testing, `@Observable`.
	10
	11	Spec: `docs/superpowers/specs/2026-09-06-ios-parity-design.md`
	12
	13	## Global Constraints
	14
	15	- Swift 6 language mode, default `MainActor` isolation. Wire models are `nonisolated struct`s.
	16	- Swift Testing only — never XCTest.
	17	- `gitbayTests` is hermetic and offline; network goes through `StubProtocol`.
	18	- New files under `gitbay/` and `gitbayTests/` need no `project.pbxproj` edit.
	19	- The label model is `IssueLabel`, never `Label`.
	20	- Never mention Claude, LLMs or AI in commits, comments, or the merge request. No `Co-Authored-By` trailer.
	21	- Never commit to `main`.
	22
	23	The commands, verbatim from the registry:
	24
	25	```
	26	mr draft <owner/name> <n>
	27	mr ready <owner/name> <n>
	28	mr retarget <owner/name> <n> <branch>
	29	mr review request <owner/name> <n> [--add <user>]... [--remove <user>]...
	30	mr create <target owner/name> --source [owner/name:]<branch> --target <branch>
	31	--title <t> [--body <b> \| --file -] [--format md\|org] [--draft]
	32	repo refs <owner/name>
	33	```
	34
	35	Note `mr review request` is a three-word path — `["mr", "review", "request", repo, n]`. The app already sends `["mr", "review", verdict, ...]` for approvals, so getting this wrong silently posts a review instead of requesting one.
	36
	37	The wire fields, from `internal/control/mr.go:337-357` — all `omitempty`, all currently undecoded by iOS:
	38
	39	```go
	40	Draft bool `json:"draft,omitempty"`
	41	ReviewRequests []string `json:"review_requests,omitempty"`
	42	StackedOn *stackRef `json:"stacked_on,omitempty"` // {number, title}
	43	Stacked []stackRef `json:"stacked,omitempty"` // [{number, title}]
	44	```
	45
	46	`mr list` rows carry `draft` and `stacked_on`; `mr show` carries all four.
	47
	48	Build and test command:
	49
	50	```bash
	51	xcodebuild -project gitbay.xcodeproj -scheme gitbay \
	52	-destination 'platform=iOS Simulator,name=iPhone 17' \
	53	test -only-testing:gitbayTests
	54	```
	55
	56	Baseline: 258 passed, 0 failed, 1 skipped (`LiveInstanceTests` — expected).
	57
	58	---
	59
	60	## File Structure
	61
	62	\| File \| Responsibility \|
	63	\|------\|----------------\|
	64	\| `gitbay/MRs/MRModels.swift` (modify) \| Decode `draft`, `review_requests`, `stacked_on`, `stacked` \|
	65	\| `gitbay/MRs/MRDetailViewModel.swift` (modify) \| `setDraft`, `retarget`, `requestReview`, `removeReviewRequest`, branch list \|
	66	\| `gitbay/MRs/MRCreateViewModel.swift` (modify) \| `--draft` on create \|
	67	\| `gitbayTests/MRLifecycleTests.swift` (create) \| Every test in this plan \|
	68	\| `gitbay/Views/MRs/MRView.swift` (modify) \| Draft badge, stack section, reviewer rows, the three actions \|
	69	\| `gitbay/Views/MRs/MRListView.swift` (modify) \| Draft badge, "stacked on" hint \|
	70
	71	---
	72
	73	### Task 1: Decode the four fields
	74
	75	Files: Modify `gitbay/MRs/MRModels.swift`; test in `gitbayTests/MRLifecycleTests.swift`
	76
	77	Interfaces produced:
	78	- `MergeRequest.draft: Bool` (defaulting false when absent) and `MergeRequest.stackedOn: StackRef?`
	79	- `MRDetail.draft: Bool`, `.reviewRequests: [String]`, `.stackedOn: StackRef?`, `.stacked: [StackRef]`
	80	- `nonisolated struct StackRef: Decodable, Sendable, Hashable, Identifiable` — `number: Int64`, `title: String`, `id` is `number`
	81
	82	The subtlety this task exists to get right. All four keys are `omitempty`, so absence is the common case: a non-draft merge request has no `draft` key at all, not `"draft": false`. `Bool` is not `Optional`, so a plain `let draft: Bool` fails to decode the ordinary merge request. Either declare them optional and expose non-optional accessors, or write `init(from:)` with `decodeIfPresent` and a default. The tests below pin the absent case for every field, because that is what production sends most of the time.
	83
	84	`draft` is a flag, not a fifth state: `state` stays `"open"`. A draft merge request is open but not asking — it does not merge and does not appear in a review queue. `isOpen` must keep meaning what it means.
	85
	86	- [ ] Step 1: Write the failing tests
	87
	88	Create `gitbayTests/MRLifecycleTests.swift`:
	89
	90	```swift
	91	import Foundation
	92	import Testing
	93	@testable import gitbay
	94
	95	private func decodeDetail(_ json: String) throws -> MRDetail {
	96	let decoder = JSONDecoder()
	97	decoder.dateDecodingStrategy = .iso8601
	98	return try decoder.decode(MRDetail.self, from: Data(json.utf8))
	99	}
	100
	101	private func decodeRow(_ json: String) throws -> MergeRequest {
	102	let decoder = JSONDecoder()
	103	decoder.dateDecodingStrategy = .iso8601
	104	return try decoder.decode(MergeRequest.self, from: Data(json.utf8))
	105	}
	106
	107	private let plainDetail = """
	108	{"number":7,"title":"a change","state":"open","author":"cmc",\
	109	"source":"feat","target_ref":"main","head_sha":"abc123",\
	110	"created_at":"2026-09-01T00:00:00Z"}
	111	"""
	112
	113	struct MRLifecycleDecodingTests {
	114
	115	/// Every one of these keys is omitempty. The ordinary merge request
	116	/// carries none of them, so absence must decode, not throw.
	117	@Test func anOrdinaryMergeRequestHasNoneOfTheNewKeys() throws {
	118	let mr = try decodeDetail(plainDetail)
	119	#expect(mr.draft == false)
	120	#expect(mr.reviewRequests.isEmpty)
	121	#expect(mr.stackedOn == nil)
	122	#expect(mr.stacked.isEmpty)
	123	}
	124
	125	@Test func aDraftDecodesAndStaysOpen() throws {
	126	let mr = try decodeDetail("""
	127	{"number":7,"title":"a change","state":"open","draft":true,"author":"cmc",\
	128	"source":"feat","target_ref":"main","head_sha":"abc123",\
	129	"created_at":"2026-09-01T00:00:00Z"}
	130	""")
	131	#expect(mr.draft)
	132	// Draft is a flag, not a fifth state.
	133	#expect(mr.state == "open")
	134	#expect(mr.isOpen)
	135	}
	136
	137	@Test func reviewRequestsDecode() throws {
	138	let mr = try decodeDetail("""
	139	{"number":7,"title":"a change","state":"open","author":"cmc",\
	140	"review_requests":["rae","sam"],\
	141	"source":"feat","target_ref":"main","head_sha":"abc123",\
	142	"created_at":"2026-09-01T00:00:00Z"}
	143	""")
	144	#expect(mr.reviewRequests == ["rae", "sam"])
	145	}
	146
	147	@Test func bothHalvesOfAStackDecode() throws {
	148	let mr = try decodeDetail("""
	149	{"number":7,"title":"a change","state":"open","author":"cmc",\
	150	"stacked_on":{"number":6,"title":"the one below"},\
	151	"stacked":[{"number":8,"title":"one above"},{"number":9,"title":"another"}],\
	152	"source":"feat","target_ref":"main","head_sha":"abc123",\
	153	"created_at":"2026-09-01T00:00:00Z"}
	154	""")
	155	#expect(mr.stackedOn?.number == 6)
	156	#expect(mr.stackedOn?.title == "the one below")
	157	#expect(mr.stacked.map(\.number) == [8, 9])
	158	}
	159
	160	@Test func aListRowCarriesDraftAndStackedOn() throws {
	161	let row = try decodeRow("""
	162	{"number":7,"title":"a change","state":"open","draft":true,"author":"cmc",\
	163	"stacked_on":{"number":6,"title":"below"},\
	164	"source":"feat","target_ref":"main","head_sha":"abc123",\
	165	"created_at":"2026-09-01T00:00:00Z"}
	166	""")
	167	#expect(row.draft)
	168	#expect(row.stackedOn?.number == 6)
	169	}
	170
	171	@Test func aPlainListRowHasNeither() throws {
	172	let row = try decodeRow("""
	173	{"number":7,"title":"a change","state":"open","author":"cmc",\
	174	"source":"feat","target_ref":"main","head_sha":"abc123",\
	175	"created_at":"2026-09-01T00:00:00Z"}
	176	""")
	177	#expect(row.draft == false)
	178	#expect(row.stackedOn == nil)
	179	}
	180	}
	181	```
	182
	183	- [ ] Step 2: Run to verify failure.
	184	- [ ] Step 3: Implement. Add `StackRef` to `MRModels.swift`, add the four fields to `MRDetail` and the two to `MergeRequest`, and extend both `CodingKeys`. Use whichever of the two approaches above is cleaner, but the non-optional accessors (`draft: Bool`, `reviewRequests: [String]`, `stacked: [StackRef]`) are what Tasks 2 and 3 consume — do not push optionality onto callers.
	185	- [ ] Step 4: Run the tests. Expect PASS and the full suite green.
	186	- [ ] Step 5: Commit — `git commit -m "Decode draft, review requests and the merge request stack"`
	187
	188	---
	189
	190	### Task 2: The four write actions
	191
	192	Files: Modify `gitbay/MRs/MRDetailViewModel.swift` and `gitbay/MRs/MRCreateViewModel.swift`; test in `gitbayTests/MRLifecycleTests.swift`
	193
	194	Interfaces produced:
	195	- `MRDetailViewModel.setDraft(_ draft: Bool) async` — `mr draft` when true, `mr ready` when false
	196	- `MRDetailViewModel.retarget(to branch: String) async`
	197	- `MRDetailViewModel.requestReview(from user: String) async` / `.removeReviewRequest(_ user: String) async`
	198	- `MRDetailViewModel.branches: [String]` and `func loadBranches() async` — from `repo refs`, for the retarget picker
	199	- `MRCreateViewModel` gains a `draft: Bool` input that appends `--draft`
	200
	201	Follow the existing `perform(argv:stdin:)` in `MRDetailViewModel` — it already sets `working`, clears `actionError`, runs, and reloads. Do not restructure it.
	202
	203	`mr review request` is a three-word path. The argv is `["mr", "review", "request", repoPath, number, "--add", user]`. The existing approve path sends `["mr", "review", verdict, ...]`, so a two-word mistake here posts a review rather than requesting one — silently, with a 200. The tests below assert the exact argv for this reason.
	204
	205	`repo refs` returns `{branches: [{name, sha}], tags: [...]}`. `RepoRefs` and `RepoRef` already exist and `RefsViewModel.load` already reads this command — reuse both models rather than writing a second pair.
	206
	207	- [ ] Step 1: Write the failing tests — append to `gitbayTests/MRLifecycleTests.swift`:
	208
	209	```swift
	210	@MainActor
	211	struct MRLifecycleActionTests {
	212
	213	private func loaded() async throws -> (MRDetailViewModel, StubProtocol.Box) {
	214	let box = StubProtocol.box()
	215	let client = GitbayClient(
	216	instance: try GitbayInstance(url: "https://gitbay.org"),
	217	token: "test-token",
	218	session: box.session()
	219	)
	220	box.enqueue(.init(status: 200, json: """
	221	{"protocol_version":1,"data":\(plainDetail),"exit_code":0}
	222	"""))
	223	let model = MRDetailViewModel(client: client, repoPath: "krz/gitbay", number: 7)
	224	await model.load()
	225	return (model, box)
	226	}
	227
	228	private func argvOf(_ seen: StubProtocol.Seen) throws -> [String] {
	229	let body = try #require(try JSONSerialization.jsonObject(with: seen.body) as? [String: Any])
	230	return try #require(body["argv"] as? [String])
	231	}
	232
	233	private func ok(_ box: StubProtocol.Box) {
	234	box.enqueue(.init(status: 200, json: """
	235	{"protocol_version":1,"exit_code":0}
	236	"""))
	237	box.enqueue(.init(status: 200, json: """
	238	{"protocol_version":1,"data":\(plainDetail),"exit_code":0}
	239	"""))
	240	}
	241
	242	@Test func markingDraftAndReadyAreDifferentCommands() async throws {
	243	let (model, box) = try await loaded()
	244	ok(box)
	245	await model.setDraft(true)
	246	var write = try #require(box.seen.first { $0.method == "POST" })
	247	#expect(try argvOf(write) == ["mr", "draft", "krz/gitbay", "7"])
	248
	249	let (model2, box2) = try await loaded()
	250	ok(box2)
	251	await model2.setDraft(false)
	252	write = try #require(box2.seen.first { $0.method == "POST" })
	253	#expect(try argvOf(write) == ["mr", "ready", "krz/gitbay", "7"])
	254	}
	255
	256	@Test func retargetPassesTheBranchPositionally() async throws {
	257	let (model, box) = try await loaded()
	258	ok(box)
	259	await model.retarget(to: "release")
	260	let write = try #require(box.seen.first { $0.method == "POST" })
	261	#expect(try argvOf(write) == ["mr", "retarget", "krz/gitbay", "7", "release"])
	262	}
	263
	264	/// `mr review request` is a THREE-word path. Two words posts a
	265	/// review instead — silently, with a 200.
	266	@Test func requestingAReviewUsesTheThreeWordPath() async throws {
	267	let (model, box) = try await loaded()
	268	ok(box)
	269	await model.requestReview(from: "rae")
	270	let write = try #require(box.seen.first { $0.method == "POST" })
	271	#expect(try argvOf(write)
	272	== ["mr", "review", "request", "krz/gitbay", "7", "--add", "rae"])
	273	}
	274
	275	@Test func removingAReviewRequestUsesRemoveNotAdd() async throws {
	276	let (model, box) = try await loaded()
	277	ok(box)
	278	await model.removeReviewRequest("rae")
	279	let write = try #require(box.seen.first { $0.method == "POST" })
	280	#expect(try argvOf(write)
	281	== ["mr", "review", "request", "krz/gitbay", "7", "--remove", "rae"])
	282	}
	283
	284	@Test func branchesComeFromRepoRefs() async throws {
	285	let (model, box) = try await loaded()
	286	box.enqueue(.init(status: 200, json: """
	287	{"protocol_version":1,"data":{"branches":[{"name":"main","sha":"a"},\
	288	{"name":"release","sha":"b"}],"tags":[]},"exit_code":0}
	289	"""))
	290	await model.loadBranches()
	291	#expect(model.branches == ["main", "release"])
	292	}
	293
	294	@Test func aRefusalSurfacesAndDoesNotReload() async throws {
	295	let (model, box) = try await loaded()
	296	box.enqueue(.init(status: 200, json: """
	297	{"protocol_version":1,"error":"cannot retarget a merged merge request",\
	298	"exit_code":1}
	299	"""))
	300	await model.retarget(to: "release")
	301	#expect(model.actionError?.isEmpty == false)
	302	#expect(model.working == false)
	303	}
	304	}
	305
	306	@MainActor
	307	struct MRCreateDraftTests {
	308
	309	@Test func creatingAsADraftAppendsTheFlag() async throws {
	310	let box = StubProtocol.box()
	311	let client = GitbayClient(
	312	instance: try GitbayInstance(url: "https://gitbay.org"),
	313	token: "test-token",
	314	session: box.session()
	315	)
	316	box.enqueue(.init(status: 200, json: """
	317	{"protocol_version":1,"data":{"number":7},"exit_code":0}
	318	"""))
	319	let model = MRCreateViewModel(client: client, repoPath: "krz/gitbay")
	320	model.draft = true
	321	_ = await model.create(source: "feat", target: "main", title: "a change", body: "")
	322
	323	let write = try #require(box.seen.first { $0.method == "POST" })
	324	let body = try #require(
	325	try JSONSerialization.jsonObject(with: write.body) as? [String: Any]
	326	)
	327	let argv = try #require(body["argv"] as? [String])
	328	#expect(argv.contains("--draft"))
	329	}
	330
	331	@Test func creatingWithoutDraftOmitsTheFlag() async throws {
	332	let box = StubProtocol.box()
	333	let client = GitbayClient(
	334	instance: try GitbayInstance(url: "https://gitbay.org"),
	335	token: "test-token",
	336	session: box.session()
	337	)
	338	box.enqueue(.init(status: 200, json: """
	339	{"protocol_version":1,"data":{"number":7},"exit_code":0}
	340	"""))
	341	let model = MRCreateViewModel(client: client, repoPath: "krz/gitbay")
	342	_ = await model.create(source: "feat", target: "main", title: "a change", body: "")
	343
	344	let write = try #require(box.seen.first { $0.method == "POST" })
	345	let body = try #require(
	346	try JSONSerialization.jsonObject(with: write.body) as? [String: Any]
	347	)
	348	#expect((body["argv"] as? [String])?.contains("--draft") == false)
	349	}
	350	}
	351	```
	352
	353	`MRCreateViewModel.create(source:target:title:body:)` is the existing signature and the tests above match it. Add `draft` as a settable property rather than a fifth parameter, so the call sites do not all change.
	354
	355	- [ ] Step 2: Run to verify failure.
	356	- [ ] Step 3: Implement the five methods and the `draft` input.
	357	- [ ] Step 4: Run the tests.
	358	- [ ] Step 5: Commit — `git commit -m "Draft, retarget and review requests on a merge request"`
	359
	360	---
	361
	362	### Task 3: The UI
	363
	364	Files: Modify `gitbay/Views/MRs/MRView.swift` and `gitbay/Views/MRs/MRListView.swift`
	365
	366	No unit tests — UI. Verification is the build plus the suite staying green.
	367
	368	On the list row: a `GBChip("draft", .secondary)` when `mr.draft`, and a quiet "stacked on !N" line when `mr.stackedOn != nil`. Match the row's existing density; do not add a line to every row.
	369
	370	On the detail screen:
	371	- Draft badge in the header beside the state.
	372	- A Stack section when `stackedOn != nil \|\| !stacked.isEmpty`, listing both directions — what this is stacked on, and what is stacked on it — each row a `NavigationLink(value: MRRoute.mr(repo:number:))`. Say which direction each is; "stacked on !6" and "!8 is stacked on this" read differently and the user needs to know which.
	373	- A Reviewers section listing `reviewRequests`, each removable, with a field to add one. Reuse the add/remove shape `IssueView` already uses for labels and assignees rather than inventing another.
	374	- Actions, placed with the existing merge/close controls: Mark as draft / Mark as ready (whichever the current state implies), and Retarget, which presents a branch picker fed by `loadBranches()`.
	375
	376	Two things the UI must say, because the server enforces them and a silent refusal is worse than a warning:
	377	1. Retargeting stales the reviews — an approval was of the diff against the old branch. Say so in the confirmation.
	378	2. A squash or rebase merge is refused while anything is stacked on this merge request, since it would rewrite the commits the stack builds on. The merge sheet offers Squash, Fast-forward and Rebase at `MRView.swift:63-65`; when `!stacked.isEmpty`, offer only Fast-forward.
	379
	380	A draft merge request does not merge. When `mr.draft`, the merge control should say so rather than presenting a button the server will refuse.
	381
	382	- [ ] Step 1: List row — draft chip and stacked hint
	383	- [ ] Step 2: Detail — draft badge, Stack section, Reviewers section
	384	- [ ] Step 3: Detail — draft/ready and retarget actions, with the two warnings above
	385	- [ ] Step 4: Build and run the full suite. Expect no drop from 258 + Tasks 1-2's additions.
	386	- [ ] Step 5: Commit — `git commit -m "Draft, stack and reviewers on the merge request screen"`
	387
	388	---
	389
	390	### Task 4: Flip four parity rows and open the merge request
	391
	392	- [ ] Step 1: In the Merge requests table, set `draft, ready`, `retarget`, `request a review` and `stacked merge requests` to `yes` for iOS. Touch no other row — in particular leave `choose body markup`, which is MR 11.
	393
	394	Land it via a worktree off `origin/main` in `krz/gitbay`, merged `--strategy ff`. That repo requires signed commits, so squash and rebase merges are refused, and its working tree usually holds unrelated work — never switch its branch.
	395
	396	- [ ] Step 2: Run the full suite; record the real number.
	397	- [ ] Step 3: Open the merge request.
	398
	399	---
	400
	401	## Notes for whoever executes this
	402
	403	`mr review request` is three words. `["mr", "review", "request", ...]`. The approve path is `["mr", "review", verdict, ...]`. A two-word slip posts a review and returns 200, so nothing surfaces — which is exactly why two tests assert the full argv.
	404
	405	Absence is the common case for all four new fields. They are `omitempty`. Most merge requests carry none of them, so the decoding must default rather than require. Every test above that pins an absent field is pinning the ordinary case, not an edge case.
	406
	407	Draft is a flag, not a state. `state` stays `"open"`. Do not add a `.draft` case to any state enum, and do not let `isOpen` change meaning — every `state = 'open'` rule on the server still means what it did.
	408
	409	Do not offer a merge strategy the server will refuse. Squash and rebase are refused while anything is stacked on the merge request.

Commit 04c4dc7d48

Commit `04c4dc7d48`