Commit `c16661f662`

c16661f662ebc809f6f6a55b834aac9a5fbff699

parent: c1f3c195a2

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-29 03:49 UTC

control: a deploy-key push or an unchecked pusher dequeues a queued merge

A deploy key cannot merge whoever registered it, so its push is judged
by its scope, not the registering account. A pusher that cannot be
looked up fails closed.

Ref #289

Layout: unified · split

internal/control/mergequeue.go +25 −9

 }
 // QueuedMergePushed is post-receive's call for a queued merge request
-// whose source branch pusherID just pushed or deleted. A push from an
+// whose source branch was pushed by pusherID with a key of scope. A push
-// account that cannot write to the target dequeues it: otherwise the
+// the target's writers did not make dequeues it, since otherwise the
-// queuer's authority would merge commits someone else chose. A push from
+// queuer's authority would merge commits someone else chose: a deploy
-// one that can keeps it queued, and the new head has to pass on its own.
+// key (which can never merge, whoever registered it) or an account that
-func QueuedMergePushed(st *store.Store, cfg config.Config, mrID, pusherID int64) {
+// cannot write to the target. A push that cannot be checked dequeues
+// too. A push from a writer keeps it queued, and the new head has to
+// pass on its own.
+func QueuedMergePushed(st *store.Store, cfg config.Config, mrID, pusherID int64, scope string) {
 	mergeQueueMu.Lock()
 	defer mergeQueueMu.Unlock()
 	mr, err := st.MRByID(mrID)
-	if err != nil || mr.QueuedAt == "" {
+	if err != nil {
+		slog.Error("merge queue: push", "mr", mrID, "err", err)
+		st.DequeueMerge(mrID)
+		return
+	}
+	if mr.QueuedAt == "" {
+		return
+	}
+	if policy.IsDeployScope(scope) {
+		dequeueWithReason(st, mr, "a deploy key pushed, and a deploy key cannot merge")
 		return
 	}
+	const unchecked = "could not check who pushed"
 	repo, err := st.RepoByID(mr.RepoID)
 	if err != nil {
-		queueInternalError(st, mr.ID, err)
+		slog.Error("merge queue: push", "mr", mrID, "err", err)
+		dequeueWithReason(st, mr, unchecked)
 		return
 	}
 	pusher, err := st.UserByID(pusherID)
 	if err != nil {
-		queueInternalError(st, mr.ID, err)
+		slog.Error("merge queue: push", "mr", mrID, "err", err)
+		dequeueWithReason(st, mr, unchecked)
 		return
 	}
 	grant, err := st.AccessRole(repo.ID, pusher.ID)
 	if err != nil {
-		queueInternalError(st, mr.ID, err)
+		slog.Error("merge queue: push", "mr", mrID, "err", err)
+		dequeueWithReason(st, mr, unchecked)
 		return
 	}
 	if !policy.CanWrite(pusher, repo, grant) {

internal/hookd/hookd.go +1 −1

	@@ -368,7 +368,7 @@ func (s *Server) postReceive(req Request) {
368	// A queued merge stays queued across a push by someone who can	368	// A queued merge stays queued across a push by someone who can
369	// merge it, and the new head has to pass the gates on its own.	369	// merge it, and the new head has to pass the gates on its own.
370	if mr.QueuedAt != "" {	370	if mr.QueuedAt != "" {
371	control.QueuedMergePushed(s.st, s.cfg, mr.ID, req.UserID)	371	control.QueuedMergePushed(s.st, s.cfg, mr.ID, req.UserID, req.Scope)
372	}	372	}
373	}	373	}
374	}	374	}

internal/hookd/mergequeue_test.go +80

	@@ -2,6 +2,7 @@ package hookd
2		2
3	import (	3	import (
4	"bytes"	4	"bytes"
		5	"fmt"
5	"os"	6	"os"
6	"path/filepath"	7	"path/filepath"
7	"strings"	8	"strings"
	@@ -204,3 +205,82 @@ func mustMR(t testing.T, st store.Store, repoID, n int64) store.MR {
204	}	205	}
205	return mr	206	return mr
206	}	207	}
		208
		209	// A push the queue cannot attribute to a writer dequeues: one from a
		210	// write deploy key, though the account that registered it can write, and
		211	// one whose pusher cannot be looked up.
		212	func TestPostReceiveDequeuesUncheckedPush(t *testing.T) {
		213	for _, tc := range []struct {
		214	name string
		215	user func(alice int64) int64
		216	scope func(repoID int64) string
		217	reason string
		218	}{
		219	{"deploy key", func(a int64) int64 { return a }, func(id int64) string { return fmt.Sprintf("deploy:%d:rw", id) },
		220	"a deploy key pushed, and a deploy key cannot merge"},
		221	{"unknown pusher", func(int64) int64 { return 9999 }, func(int64) string { return "full" },
		222	"could not check who pushed"},
		223	} {
		224	t.Run(tc.name, func(t *testing.T) {
		225	st, err := store.Open(":memory:")
		226	if err != nil {
		227	t.Fatal(err)
		228	}
		229	t.Cleanup(func() { st.Close() })
		230	if err := st.MigrateUp(); err != nil {
		231	t.Fatal(err)
		232	}
		233	alice, _ := st.CreateUser("alice", false)
		234	repoID, _ := st.CreateRepo("user", alice, "app", "public")
		235	st.UpdateRepoSettings(repoID, func(s *store.RepoSettings) { s.RequireApprovals = 1 })
		236	repo, _ := st.RepoByID(repoID)
		237	root := t.TempDir()
		238	f := &shapeFixture{t: t, st: st, repo: repo, uid: alice, root: root, src: filepath.Join(root, "src")}
		239	f.dir = control.RepoDir(root, repo.OwnerName, repo.Name)
		240	cfg := config.Config{}
		241	cfg.Server.Root = root
		242	srv := &Server{cfg: cfg, st: st}
		243	os.MkdirAll(f.src, 0o755)
		244	f.git(root, "init", "-q", "-b", "main", "src")
		245	f.write("README", "x\n")
		246	f.git(f.src, "add", ".")
		247	f.git(f.src, "commit", "-q", "-m", "base")
		248	f.git(f.src, "checkout", "-q", "-b", "feature")
		249	f.write("feature.txt", "y\n")
		250	f.git(f.src, "add", ".")
		251	f.git(f.src, "commit", "-q", "-m", "change")
		252	head := f.sha("HEAD")
		253	os.MkdirAll(filepath.Dir(f.dir), 0o755)
		254	f.git(root, "init", "-q", "--bare", f.dir)
		255	f.sync()
		256	f.git(f.dir, "update-ref", "refs/merge-requests/1/head", head)
		257	if _, err := st.CreateMR(repo.ID, alice, repo.ID, "feature", "main", "t", "", head, "md", false); err != nil {
		258	t.Fatal(err)
		259	}
		260	var out, errOut bytes.Buffer
		261	c := &control.Ctx{User: store.User{ID: alice, Username: "alice"}, Scope: "full", Store: st, Cfg: cfg, Stdout: &out, Stderr: &errOut}
		262	if code := control.Dispatch(c, []string{"mr", "merge", repo.Path(), "1", "--when-ready"}); code != protocol.ExitOK {
		263	t.Fatalf("queue: exit %d, %s", code, errOut.String())
		264	}
		265
		266	f.write("feature.txt", "z\n")
		267	f.git(f.src, "commit", "-q", "-am", "more")
		268	pushed := f.sha("HEAD")
		269	f.sync()
		270	srv.postReceive(Request{RepoID: repo.ID, UserID: tc.user(alice), Scope: tc.scope(repo.ID),
		271	Updates: []policy.RefUpdate{{Ref: "refs/heads/feature", Old: head, New: pushed}}})
		272
		273	mr := mustMR(t, st, repo.ID, 1)
		274	if mr.QueuedAt != "" \|\| mr.State != "open" {
		275	t.Fatalf("!1 = state %s queued_at %q, want open and dequeued", mr.State, mr.QueuedAt)
		276	}
		277	cs, _ := st.ListMRComments(mr.ID)
		278	for _, c := range cs {
		279	if c.Kind == "system" && strings.Contains(c.Body, tc.reason) {
		280	return
		281	}
		282	}
		283	t.Fatalf("timeline does not say %q: %+v", tc.reason, cs)
		284	})
		285	}
		286	}

Commit c16661f662

Commit `c16661f662`