Commit `f8f4f38f31`

f8f4f38f310dd34f689e38911772719bccde59c4

parent: 3b4f05eb52

Verified · cmc ci/build: success ci/test: success ci/vuln: success

cmc <hello@cleberg.net> · 2026-09-04 16:54 UTC

tests: the review loop, driven end to end by three accounts

Approvals, CODEOWNERS, review threads and notifications each had a test.
Nothing put them together, so the loop a repository actually runs — draft,
ready, thread, approval, resolve, merge — had never executed once.

An author, an owner named in CODEOWNERS, and a third collaborator who
owns nothing. The third is what isolates the CODEOWNERS gate: their
approval satisfies require-approvals and leaves the owner requirement
unmet, so the refusal can only come from CODEOWNERS. All three settings
are on at once, which no single-feature test did.

This is not the second human #139 asks for, and the difference matters:
the CODEOWNERS bug was a gate that never fired, and a test written from
the same understanding as the code would have asserted the same wrong
thing. What it closes is the narrower gap that the features had never met.

Writing it found two things.

`mr ready` notified a thread's participants, which at that moment is the
author alone — the actor, excluded — so it reached nobody. It notifies
the repository's targets now, the way opening one does. That corrects the
choice made in the commit below this one.

And there is no way to ask a particular person for a review at all: the
queue is computed from involvement, and a collaborator with write access
who has not yet touched the thread gets nothing until they comment. Filed
as #145. The test asserts the current behaviour and fails if it changes,
so the issue and the code cannot drift apart silently.

Closes #139

Layout: unified · split

e2e/reviewloop_test.go added +205

@@ -0,0 +1,205 @@
	1	package e2e
	2
	3	import (
	4	"encoding/json"
	5	"fmt"
	6	"os"
	7	"path/filepath"
	8	"strings"
	9	"testing"
	10	)
	11
	12	// TestTwoAccountReviewLoop drives the collaboration features end to end as
	13	// two distinct accounts: the author never approves their own work, and the
	14	// reviewer sees each step arrive.
	15	//
	16	// This is not the second human #139 asks for, and it is worth being exact
	17	// about why. The CODEOWNERS bug (#99) was a gate that never fired; a test
	18	// written from the same understanding as the code would have asserted the
	19	// same wrong thing. What this does close is the narrower gap that nothing
	20	// exercised the loop end to end at all — every feature had its own test
	21	// and none of them met.
	22	func TestTwoAccountReviewLoop(t *testing.T) {
	23	inst := startInstance(t)
	24	authorKey := inst.newKey(t, "author")
	25	reviewerKey := inst.newKey(t, "reviewer")
	26	// The author merges, and a merge commit carries their identity, so
	27	// the account needs a verified address. The first merge below can
	28	// fast-forward and would not have needed one; the second cannot.
	29	inst.admin(t, "admin", "user", "create", "author", "--key", authorKey+".pub",
	30	"--email", "author@example.test", "--verified")
	31	inst.admin(t, "admin", "user", "create", "reviewer", "--key", reviewerKey+".pub")
	32	// A third account with write access who owns nothing, so an approval
	33	// from them satisfies the count and leaves CODEOWNERS unsatisfied.
	34	helperKey := inst.newKey(t, "helper")
	35	inst.admin(t, "admin", "user", "create", "helper", "--key", helperKey+".pub")
	36
	37	if _, errOut, code := inst.ssh(t, authorKey, "", "repo", "create", "author/lib"); code != 0 {
	38	t.Fatalf("repo create: %s", errOut)
	39	}
	40	for _, who := range []string{"reviewer", "helper"} {
	41	if _, _, code := inst.ssh(t, authorKey, "", "repo", "access", "grant", "author/lib", who, "write"); code != 0 {
	42	t.Fatalf("grant %s failed", who)
	43	}
	44	}
	45	// Every gate at once, which is how a repository that means it is set
	46	// up, and which no single-feature test covers together.
	47	for _, s := range [][]string{
	48	{"repo", "settings", "require-approvals", "author/lib", "1"},
	49	{"repo", "settings", "require-resolved", "author/lib", "on"},
	50	{"repo", "settings", "require-codeowners", "author/lib", "on"},
	51	} {
	52	if _, errOut, code := inst.ssh(t, authorKey, "", s...); code != 0 {
	53	t.Fatalf("%v: %s", s, errOut)
	54	}
	55	}
	56
	57	env := inst.gitEnv(authorKey)
	58	work := t.TempDir()
	59	mustGit(t, work, env, "clone", inst.sshURL("author/lib"), "w")
	60	dir := filepath.Join(work, "w")
	61	os.WriteFile(filepath.Join(dir, "CODEOWNERS"), []byte("*.go @reviewer\n"), 0o644)
	62	os.WriteFile(filepath.Join(dir, "lib.go"), []byte("package lib\n"), 0o644)
	63	mustGit(t, dir, env, "checkout", "-q", "-b", "main")
	64	mustGit(t, dir, env, "add", ".")
	65	mustGit(t, dir, env, "commit", "-q", "-m", "base")
	66	mustGit(t, dir, env, "push", "-q", "origin", "main")
	67
	68	mustGit(t, dir, env, "checkout", "-q", "-b", "feat")
	69	os.WriteFile(filepath.Join(dir, "lib.go"), []byte("package lib\n\nvar V = 1\n"), 0o644)
	70	mustGit(t, dir, env, "add", ".")
	71	mustGit(t, dir, env, "commit", "-q", "-m", "add V")
	72	mustGit(t, dir, env, "push", "-q", "origin", "feat")
	73
	74	// Opened as a draft: the reviewer is not asked yet.
	75	if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "create", "author/lib",
	76	"--source", "feat", "--target", "main", "--title", "'add V'", "--draft"); code != 0 {
	77	t.Fatalf("mr create: %s", errOut)
	78	}
	79	if q := reviewQueue(t, inst, reviewerKey); len(q) != 0 {
	80	t.Fatalf("a draft is waiting on the reviewer: %v", q)
	81	}
	82
	83	// Ready. Now it is theirs, and it reaches their inbox.
	84	if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "ready", "author/lib", "1"); code != 0 {
	85	t.Fatalf("mr ready: %s", errOut)
	86	}
	87	if q := reviewQueue(t, inst, reviewerKey); len(q) != 1 \|\| q[0] != 1 {
	88	t.Fatalf("review queue = %v, want !1", q)
	89	}
	90	// The queue is how a reviewer finds out, and the only way: there is
	91	// no "request review from <user>", so nothing is pushed to someone
	92	// who is neither an owner nor already in the thread. Writing this
	93	// test is what surfaced that — see #145.
	94	if got := inbox(t, inst, reviewerKey); strings.Contains(got, "ready for review") {
	95	t.Fatalf("a reviewer is notified after all; #145 and this comment are stale:\n%s", got)
	96	}
	97
	98	// The author cannot approve their own work past the gate.
	99	if _, _, code := inst.ssh(t, authorKey, "", "mr", "review", "author/lib", "1", "--approve"); code != 0 {
	100	t.Fatal("self review refused outright; it should be recorded and not counted")
	101	}
	102	_, errOut, code := inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "1")
	103	if code != 4 \|\| !strings.Contains(errOut, "requires 1 fresh approval") {
	104	t.Fatalf("author's own approval counted: exit %d, %s", code, errOut)
	105	}
	106
	107	// A review thread from the reviewer blocks on require-resolved.
	108	out, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "diff-comment", "author/lib", "1",
	109	"--path", "lib.go", "--line", "3", "--message", "'name it better'", "--json")
	110	if code != 0 {
	111	t.Fatalf("diff-comment: %s", errOut)
	112	}
	113	var thread struct {
	114	Data struct {
	115	Thread int64 `json:"thread"`
	116	} `json:"data"`
	117	}
	118	json.Unmarshal([]byte(out), &thread)
	119	if !strings.Contains(inbox(t, inst, authorKey), "commented on lib.go") {
	120	t.Fatalf("author not told about the review thread:\n%s", inbox(t, inst, authorKey))
	121	}
	122
	123	if _, _, code := inst.ssh(t, reviewerKey, "", "mr", "review", "author/lib", "1", "--approve"); code != 0 {
	124	t.Fatal("reviewer approve failed")
	125	}
	126	_, errOut, code = inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "1")
	127	if code != 4 \|\| !strings.Contains(errOut, "threads resolved") {
	128	t.Fatalf("open thread did not block the merge: exit %d, %s", code, errOut)
	129	}
	130
	131	// Resolve, and every gate is satisfied at once: an owner approved, the
	132	// count is met, the thread is closed.
	133	if _, errOut, code := inst.ssh(t, reviewerKey, "", "mr", "resolve", "author/lib", "1",
	134	fmt.Sprint(thread.Data.Thread)); code != 0 {
	135	t.Fatalf("resolve: %s", errOut)
	136	}
	137	if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "1"); code != 0 {
	138	t.Fatalf("fully reviewed MR refused: %s", errOut)
	139	}
	140	if !strings.Contains(inbox(t, inst, reviewerKey), "merged !1") {
	141	t.Fatalf("reviewer not told about the merge:\n%s", inbox(t, inst, reviewerKey))
	142	}
	143
	144	// A CODEOWNERS gate with no owner approval still refuses, on a second
	145	// MR, so the pass above was the approval and not the gate being off.
	146	mustGit(t, dir, env, "checkout", "-q", "-b", "feat2", "main")
	147	os.WriteFile(filepath.Join(dir, "other.go"), []byte("package lib\n"), 0o644)
	148	mustGit(t, dir, env, "add", ".")
	149	mustGit(t, dir, env, "commit", "-q", "-m", "another")
	150	mustGit(t, dir, env, "push", "-q", "origin", "feat2")
	151	if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "create", "author/lib",
	152	"--source", "feat2", "--target", "main", "--title", "'another'"); code != 0 {
	153	t.Fatalf("mr create: %s", errOut)
	154	}
	155	// helper's approval meets require-approvals but owns none of the
	156	// changed files, so what refuses this merge is CODEOWNERS alone —
	157	// the gate that used to be reachable only through the approval count
	158	// (#99), now on its own toggle (#142).
	159	if _, _, code := inst.ssh(t, helperKey, "", "mr", "review", "author/lib", "2", "--approve"); code != 0 {
	160	t.Fatal("helper approve failed")
	161	}
	162	_, errOut, code = inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "2")
	163	if code != 4 \|\| !strings.Contains(errOut, "CODEOWNERS") {
	164	t.Fatalf("CODEOWNERS gate did not fire on the second MR: exit %d, %s", code, errOut)
	165	}
	166	// And the owner's approval clears it.
	167	if _, _, code := inst.ssh(t, reviewerKey, "", "mr", "review", "author/lib", "2", "--approve"); code != 0 {
	168	t.Fatal("reviewer approve failed")
	169	}
	170	if _, errOut, code := inst.ssh(t, authorKey, "", "mr", "merge", "author/lib", "2"); code != 0 {
	171	t.Fatalf("owner-approved MR refused: %s", errOut)
	172	}
	173	}
	174
	175	// reviewQueue returns the MR numbers waiting on this account.
	176	func reviewQueue(t testing.T, inst instance, key string) []int64 {
	177	t.Helper()
	178	out, errOut, code := inst.ssh(t, key, "", "dashboard", "--json")
	179	if code != 0 {
	180	t.Fatalf("dashboard: %s", errOut)
	181	}
	182	var env struct {
	183	Data struct {
	184	Reviews []struct {
	185	Number int64 `json:"number"`
	186	} `json:"review_queue"`
	187	} `json:"data"`
	188	}
	189	json.Unmarshal([]byte(out), &env)
	190	var ns []int64
	191	for _, r := range env.Data.Reviews {
	192	ns = append(ns, r.Number)
	193	}
	194	return ns
	195	}
	196
	197	// inbox returns this account's notifications as raw JSON.
	198	func inbox(t testing.T, inst instance, key string) string {
	199	t.Helper()
	200	out, errOut, code := inst.ssh(t, key, "", "notifications", "list", "--all", "--json")
	201	if code != 0 {
	202	t.Fatalf("notifications list: %s", errOut)
	203	}
	204	return out
	205	}

internal/control/mr.go +10 −2

 		fmt.Sprintf(`{"number":%d,"draft":%t}`, mr.Number, draft))
 	// Marking ready is the request for review; going back to draft
 	// withdraws it and is not worth anyone's inbox.
+	//
+	// The targets are the repository's, not the thread's participants.
+	// Until someone comments or reviews, the only participant is the
+	// author, who is the actor and excluded — so notifying participants
+	// here reaches nobody, which is exactly what opening it as a draft
+	// and then marking it ready would do. Opening a merge request tells
+	// the repository; so does saying it is finally asking.
 	if !draft {
-		if parts, err := c.Store.MRParticipants(mr.ID); err == nil {
+		if targets, err := c.Store.RepoNotifyTargets(repo); err == nil {
-			notify(c, parts, notice{repo: repo, kind: "mr",
+			parts, _ := c.Store.MRParticipants(mr.ID)
+			notify(c, append(targets, parts...), notice{repo: repo, kind: "mr",
 				subject: mrSubject(repo, mr.Number, mr.Title),
 				action:  fmt.Sprintf("marked !%d ready for review", mr.Number),
 				path:    fmt.Sprintf("%s/mrs/%d", repo.Path(), mr.Number)})

Commit f8f4f38f31

Commit `f8f4f38f31`