Commit `eee4509235`

eee4509235cd13d014936206ad7092e00289786e

parent: b56876ccbf

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

cmc <hello@cleberg.net> · 2026-09-03 16:47 UTC

mr: CODEOWNERS gates on its own, not only under require-approvals

The CODEOWNERS block sat inside the require_approvals branch of
reviewGates, so a repository with an owners file and default settings
had no owner gating. Reviews are now read for every merge, the count
and request-changes checks stay behind require_approvals, and the
owners check runs whenever the target branch carries the file.

TestCodeownersWithoutRequiredApprovals: with default settings, an
owned change is refused until an owner approves and an unowned change
merges.

Closes #99

Layout: unified · split

e2e/approvals_test.go +56

	@@ -136,3 +136,59 @@ func TestMergeRequirements(t *testing.T) {
136	t.Fatalf("stale approval counted: exit %d, %s", code, errOut)	136	t.Fatalf("stale approval counted: exit %d, %s", code, errOut)
137	}	137	}
138	}	138	}
		139
		140	// A CODEOWNERS file gates on its own. It used to be read only inside the
		141	// require-approvals branch, so a repository with owners and the default
		142	// settings had no owner gating at all (#99).
		143	func TestCodeownersWithoutRequiredApprovals(t *testing.T) {
		144	inst := startInstance(t)
		145	aliceKey := inst.newKey(t, "alice")
		146	carolKey := inst.newKey(t, "carol")
		147	inst.admin(t, "admin", "user", "create", "alice", "--key", aliceKey+".pub", "--email", "alice@example.test", "--verified")
		148	inst.admin(t, "admin", "user", "create", "carol", "--key", carolKey+".pub")
		149	if _, errOut, code := inst.ssh(t, aliceKey, "", "repo", "create", "alice/svc"); code != 0 {
		150	t.Fatalf("repo create: %s", errOut)
		151	}
		152	if _, _, code := inst.ssh(t, aliceKey, "", "repo", "access", "grant", "alice/svc", "carol", "write"); code != 0 {
		153	t.Fatal("grant failed")
		154	}
		155	work := t.TempDir()
		156	env := inst.gitEnv(aliceKey)
		157	mustGit(t, work, env, "clone", inst.sshURL("alice/svc"), "w")
		158	dir := filepath.Join(work, "w")
		159	os.WriteFile(filepath.Join(dir, "CODEOWNERS"), []byte("*.go @carol\n"), 0o644)
		160	os.WriteFile(filepath.Join(dir, "svc.go"), []byte("package svc\n"), 0o644)
		161	os.WriteFile(filepath.Join(dir, "README"), []byte("svc\n"), 0o644)
		162	mustGit(t, dir, env, "checkout", "-q", "-b", "main")
		163	mustGit(t, dir, env, "add", ".")
		164	mustGit(t, dir, env, "commit", "-q", "-m", "base")
		165	mustGit(t, dir, env, "push", "-q", "origin", "main")
		166
		167	// One MR touches an owned file, one does not.
		168	for i, f := range []string{"svc.go", "README"} {
		169	mustGit(t, dir, env, "checkout", "-q", "-b", fmt.Sprintf("feat%d", i), "main")
		170	os.WriteFile(filepath.Join(dir, f), []byte("changed\n"), 0o644)
		171	mustGit(t, dir, env, "add", ".")
		172	mustGit(t, dir, env, "commit", "-q", "-m", "change")
		173	mustGit(t, dir, env, "push", "-q", "origin", fmt.Sprintf("feat%d", i))
		174	if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "create", "alice/svc",
		175	"--source", fmt.Sprintf("feat%d", i), "--target", "main", "--title", "'change'"); code != 0 {
		176	t.Fatalf("mr create: %s", errOut)
		177	}
		178	}
		179
		180	// Default settings, no approvals: the owned file is gated, the other is not.
		181	_, errOut, code := inst.ssh(t, aliceKey, "", "mr", "merge", "alice/svc", "1")
		182	if code != 4 \|\| !strings.Contains(errOut, "CODEOWNERS") \|\| !strings.Contains(errOut, "carol") {
		183	t.Fatalf("codeowners gate with require-approvals off: exit %d, %s", code, errOut)
		184	}
		185	if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "merge", "alice/svc", "2"); code != 0 {
		186	t.Fatalf("unowned change refused: %s", errOut)
		187	}
		188	if _, _, code := inst.ssh(t, carolKey, "", "mr", "review", "alice/svc", "1", "--approve"); code != 0 {
		189	t.Fatal("carol review failed")
		190	}
		191	if _, errOut, code := inst.ssh(t, aliceKey, "", "mr", "merge", "alice/svc", "1"); code != 0 {
		192	t.Fatalf("owner-approved merge refused: %s", errOut)
		193	}
		194	}

internal/control/mr.go +66 −67

	@@ -1145,37 +1145,35 @@ func runMRMerge(c *Ctx, args []string) int {
1145	}	1145	}
1146		1146
1147	// reviewGates enforces require_approvals (fresh, non-author, latest review	1147	// reviewGates enforces require_approvals (fresh, non-author, latest review
1148	// per reviewer; a fresh request-changes blocks), CODEOWNERS coverage, and	1148	// per reviewer; a fresh request-changes blocks), CODEOWNERS coverage
		1149	// whenever the target branch carries a CODEOWNERS file, and
1149	// require_resolved. Returns -1 to proceed.	1150	// require_resolved. Returns -1 to proceed.
1150	func (c *Ctx) reviewGates(repo store.Repo, mr store.MR, dir, targetSHA, headSHA string) int {	1151	func (c *Ctx) reviewGates(repo store.Repo, mr store.MR, dir, targetSHA, headSHA string) int {
1151	set := repo.Settings	1152	set := repo.Settings
1152	if set.RequireApprovals == 0 && !set.RequireResolved {	1153	reviews, err := c.Store.ListMRReviews(mr.ID)
1153	return -1	1154	if err != nil {
		1155	return c.fail(protocol.ExitFailure, "%v", err)
1154	}	1156	}
1155		1157	// Latest fresh review per reviewer decides their stance.
1156	if set.RequireApprovals > 0 {	1158	latest := map[string]string{}
1157	reviews, err := c.Store.ListMRReviews(mr.ID)	1159	for _, r := range reviews {
1158	if err != nil {	1160	if r.Stale \|\| r.Reviewer == mr.Author {
1159	return c.fail(protocol.ExitFailure, "%v", err)	1161	continue
1160	}
1161	// Latest fresh review per reviewer decides their stance.
1162	latest := map[string]string{}
1163	for _, r := range reviews {
1164	if r.Stale \|\| r.Reviewer == mr.Author {
1165	continue
1166	}
1167	latest[r.Reviewer] = r.Verdict
1168	}	1162	}
1169	var approvers []string	1163	latest[r.Reviewer] = r.Verdict
1170	var blockers []string	1164	}
1171	for who, verdict := range latest {	1165	var approvers []string
1172	switch verdict {	1166	var blockers []string
1173	case "approve":	1167	for who, verdict := range latest {
1174	approvers = append(approvers, who)	1168	switch verdict {
1175	case "request_changes":	1169	case "approve":
1176	blockers = append(blockers, who)	1170	approvers = append(approvers, who)
1177	}	1171	case "request_changes":
		1172	blockers = append(blockers, who)
1178	}	1173	}
		1174	}
		1175
		1176	if set.RequireApprovals > 0 {
1179	if len(blockers) > 0 {	1177	if len(blockers) > 0 {
1180	slices.Sort(blockers)	1178	slices.Sort(blockers)
1181	return c.fail(protocol.ExitDenied,	1179	return c.fail(protocol.ExitDenied,
	@@ -1185,57 +1183,58 @@ func (c *Ctx) reviewGates(repo store.Repo, mr store.MR, dir, targetSHA, headSHA
1185	return c.fail(protocol.ExitDenied,	1183	return c.fail(protocol.ExitDenied,
1186	"%s requires %d fresh approval(s); !%d has %d", repo.Path(), set.RequireApprovals, mr.Number, len(approvers))	1184	"%s requires %d fresh approval(s); !%d has %d", repo.Path(), set.RequireApprovals, mr.Number, len(approvers))
1187	}	1185	}
		1186	}
1188		1187
1189	// CODEOWNERS: every owned changed file needs an approval from one	1188	// CODEOWNERS: every owned changed file needs an approval from one of
1190	// of its owners.	1189	// its owners. The file's presence is the opt-in; it does not wait on
1191	content, err := gitutil.ReadBlob(dir, "refs/heads/"+mr.TargetRef, "CODEOWNERS", 1<<20)	1190	// require_approvals (#99).
		1191	content, err := gitutil.ReadBlob(dir, "refs/heads/"+mr.TargetRef, "CODEOWNERS", 1<<20)
		1192	if err != nil {
		1193	content, err = gitutil.ReadBlob(dir, "refs/heads/"+mr.TargetRef, ".gitbay/CODEOWNERS", 1<<20)
		1194	}
		1195	if err == nil && len(content) > 0 {
		1196	rules := policy.ParseCodeowners(string(content))
		1197	base, err := gitutil.MergeBase(dir, targetSHA, headSHA)
1192	if err != nil {	1198	if err != nil {
1193	content, err = gitutil.ReadBlob(dir, "refs/heads/"+mr.TargetRef, ".gitbay/CODEOWNERS", 1<<20)	1199	return c.fail(protocol.ExitFailure, "%v", err)
1194	}	1200	}
1195	if err == nil && len(content) > 0 {	1201	files, err := gitutil.DiffFiles(dir, base, headSHA)
1196	rules := policy.ParseCodeowners(string(content))	1202	if err != nil {
1197	base, err := gitutil.MergeBase(dir, targetSHA, headSHA)	1203	return c.fail(protocol.ExitFailure, "%v", err)
1198	if err != nil {	1204	}
1199	return c.fail(protocol.ExitFailure, "%v", err)	1205	approved := map[string]bool{}
1200	}	1206	for _, a := range approvers {
1201	files, err := gitutil.DiffFiles(dir, base, headSHA)	1207	approved[a] = true
1202	if err != nil {	1208	}
1203	return c.fail(protocol.ExitFailure, "%v", err)	1209	missing := map[string][]string{} // owner-set key -> example paths
1204	}	1210	for _, f := range files {
1205	approved := map[string]bool{}	1211	owners := policy.OwnersFor(rules, f)
1206	for _, a := range approvers {	1212	if owners == nil {
1207	approved[a] = true	1213	continue
1208	}	1214	}
1209	missing := map[string][]string{} // owner-set key -> example paths	1215	ok := false
1210	for _, f := range files {	1216	for _, o := range owners {
1211	owners := policy.OwnersFor(rules, f)	1217	if approved[o] {
1212	if owners == nil {	1218	ok = true
1213	continue	1219	break
1214	}
1215	ok := false
1216	for _, o := range owners {
1217	if approved[o] {
1218	ok = true
1219	break
1220	}
1221	}
1222	if !ok {
1223	key := strings.Join(owners, ",")
1224	if len(missing[key]) < 3 {
1225	missing[key] = append(missing[key], f)
1226	}
1227	}	1220	}
1228	}	1221	}
1229	if len(missing) > 0 {	1222	if !ok {
1230	var parts []string	1223	key := strings.Join(owners, ",")
1231	for owners, paths := range missing {	1224	if len(missing[key]) < 3 {
1232	parts = append(parts, fmt.Sprintf("%s (owned by %s)", strings.Join(paths, ", "), owners))	1225	missing[key] = append(missing[key], f)
1233	}	1226	}
1234	slices.Sort(parts)
1235	return c.fail(protocol.ExitDenied,
1236	"CODEOWNERS approval missing for: %s", strings.Join(parts, "; "))
1237	}	1227	}
1238	}	1228	}
		1229	if len(missing) > 0 {
		1230	var parts []string
		1231	for owners, paths := range missing {
		1232	parts = append(parts, fmt.Sprintf("%s (owned by %s)", strings.Join(paths, ", "), owners))
		1233	}
		1234	slices.Sort(parts)
		1235	return c.fail(protocol.ExitDenied,
		1236	"CODEOWNERS approval missing for: %s", strings.Join(parts, "; "))
		1237	}
1239	}	1238	}
1240		1239
1241	if set.RequireResolved {	1240	if set.RequireResolved {

Commit eee4509235

Commit `eee4509235`