feat(app): cap the comments of a merge request at 1000

Past MaxCommentsPerMergeRequest, AddComment fails with ErrResourceExhausted,
which the API maps to resource_exhausted.

commit: b87e47166515aa06e5684d8671d1fd1c643df05b
author: brice · 11d ago · 2026-10-10 17:46 UTC
parent: 6d7a853
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +37 -1

M internal/app/mergerequests.go +10 -1


@@ -25,6 +25,11 @@ const MaxMergeRequestCommits = 100
25	25		// retry is a read and a write of ermite-kv, cheap enough for a bound this high.
26	26		const maxWriteRetries = 32
27	27
	28	+	// MaxCommentsPerMergeRequest is the most comments a merge request holds. Past
	29	+	// it AddComment fails with domain.ErrResourceExhausted (resource_exhausted in
	30	+	// the API). Comments are never deleted, so the limit counts the ones ever added.
	31	+	const MaxCommentsPerMergeRequest = 1000
	32	+
28	33		const mrTokenPrefix = "mp1:"
29	34
30	35		// MergeRequests are the merge request use cases (ADR 0017): review, comment,

@@ -469,7 +474,8 @@ type AddCommentParams struct {
469	474		// mr:write. The text must follow the rules of ADR 0017 and not be blank; a
470	475		// comment on a line must name a line of the current diff of the request (see
471	476		// domain.Diff.HasLine), else the call fails with domain.ErrInvalid. A request
472		-	// that is not open gives domain.ErrPreconditionFailed.
	477	+	// that is not open gives domain.ErrPreconditionFailed, and one that already
	478	+	// holds MaxCommentsPerMergeRequest comments domain.ErrResourceExhausted.
473	479		func (m *MergeRequests) AddComment(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params AddCommentParams) (domain.Comment, error) {
474	480		repo, err := m.writable(ctx, p, name, domain.ScopeMRWrite)
475	481		if err != nil {

@@ -514,6 +520,9 @@ func (m *MergeRequests) AddComment(ctx context.Context, p domain.Principal, name
514	520
515	521		var c domain.Comment
516	522		for range maxWriteRetries {
	523	+	if mr.CommentSeq >= MaxCommentsPerMergeRequest {
	524	+	return domain.Comment{}, fmt.Errorf("%w: !%d already has %d comments", domain.ErrResourceExhausted, number, MaxCommentsPerMergeRequest)
	525	+	}
517	526		c = domain.Comment{
518	527		ID: mr.CommentSeq + 1, Author: authorOf(p), Path: params.Path, Line: params.Line, Side: params.Side,
519	528		CommitID: mr.HeadID, Text: text, CreatedAt: m.clock.Now().UTC(),

 // retry is a read and a write of ermite-kv, cheap enough for a bound this high.
 const maxWriteRetries = 32
+// MaxCommentsPerMergeRequest is the most comments a merge request holds. Past
+// it AddComment fails with domain.ErrResourceExhausted (resource_exhausted in
+// the API). Comments are never deleted, so the limit counts the ones ever added.
+const MaxCommentsPerMergeRequest = 1000
 const mrTokenPrefix = "mp1:"
 // MergeRequests are the merge request use cases (ADR 0017): review, comment,
 // mr:write. The text must follow the rules of ADR 0017 and not be blank; a
 // comment on a line must name a line of the current diff of the request (see
 // domain.Diff.HasLine), else the call fails with domain.ErrInvalid. A request
-// that is not open gives domain.ErrPreconditionFailed.
+// that is not open gives domain.ErrPreconditionFailed, and one that already
+// holds MaxCommentsPerMergeRequest comments domain.ErrResourceExhausted.
 func (m *MergeRequests) AddComment(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params AddCommentParams) (domain.Comment, error) {
 	repo, err := m.writable(ctx, p, name, domain.ScopeMRWrite)
 	if err != nil {
 	var c domain.Comment
 	for range maxWriteRetries {
+		if mr.CommentSeq >= MaxCommentsPerMergeRequest {
+			return domain.Comment{}, fmt.Errorf("%w: !%d already has %d comments", domain.ErrResourceExhausted, number, MaxCommentsPerMergeRequest)
+		}
 		c = domain.Comment{
 			ID: mr.CommentSeq + 1, Author: authorOf(p), Path: params.Path, Line: params.Line, Side: params.Side,
 			CommitID: mr.HeadID, Text: text, CreatedAt: m.clock.Now().UTC(),

M internal/app/mergerequests_integration_test.go +27 -0


@@ -1250,3 +1250,30 @@ func TestMergeRecordsAPush(t *testing.T) {
1250	1250		})
1251	1251		}
1252	1252		}
	1253	+
	1254	+	// GO-008: a merge request holds at most MaxCommentsPerMergeRequest comments.
	1255	+	func TestCommentsPerMergeRequestAreBounded(t *testing.T) {
	1256	+	r := newMRRig(t)
	1257	+	r.seed()
	1258	+	r.feature("feat")
	1259	+	mr := r.open("feat")
	1260	+	for i := range app.MaxCommentsPerMergeRequest {
	1261	+	_, err := r.mrs.AddComment(r.ctx, mrW, r.repo, mr.Number, app.AddCommentParams{Text: fmt.Sprintf("c%d", i)})
	1262	+	require.NoError(t, err, "comment %d", i+1)
	1263	+	}
	1264	+	_, err := r.mrs.AddComment(r.ctx, mrW, r.repo, mr.Number, app.AddCommentParams{Text: "one too many"})
	1265	+	assert.ErrorIs(t, err, domain.ErrResourceExhausted)
	1266	+	d, err := r.mrs.Get(r.ctx, root, r.repo, mr.Number, false)
	1267	+	require.NoError(t, err)
	1268	+	assert.Len(t, d.Comments, app.MaxCommentsPerMergeRequest)
	1269	+
	1270	+	// Resolving still works, and another request has its own budget.
	1271	+	_, err = r.mrs.ResolveComment(r.ctx, mrW, r.repo, mr.Number, 1, true)
	1272	+	require.NoError(t, err)
	1273	+	r.sh("checkout", "-q", "-B", "other", "main")
	1274	+	r.commit(author, "o.txt", "o\n", "other")
	1275	+	r.push("other")
	1276	+	other := r.open("other")
	1277	+	_, err = r.mrs.AddComment(r.ctx, mrW, r.repo, other.Number, app.AddCommentParams{Text: "fine"})
	1278	+	require.NoError(t, err)
	1279	+	}


@@ -1250,3 +1250,30 @@ func TestMergeRecordsAPush(t *testing.T) {
1250	})	1250		})
1251	}	1251		}
1252	}	1252		}
		1253	+
		1254	+	// GO-008: a merge request holds at most MaxCommentsPerMergeRequest comments.
		1255	+	func TestCommentsPerMergeRequestAreBounded(t *testing.T) {
		1256	+	r := newMRRig(t)
		1257	+	r.seed()
		1258	+	r.feature("feat")
		1259	+	mr := r.open("feat")
		1260	+	for i := range app.MaxCommentsPerMergeRequest {
		1261	+	_, err := r.mrs.AddComment(r.ctx, mrW, r.repo, mr.Number, app.AddCommentParams{Text: fmt.Sprintf("c%d", i)})
		1262	+	require.NoError(t, err, "comment %d", i+1)
		1263	+	}
		1264	+	_, err := r.mrs.AddComment(r.ctx, mrW, r.repo, mr.Number, app.AddCommentParams{Text: "one too many"})
		1265	+	assert.ErrorIs(t, err, domain.ErrResourceExhausted)
		1266	+	d, err := r.mrs.Get(r.ctx, root, r.repo, mr.Number, false)
		1267	+	require.NoError(t, err)
		1268	+	assert.Len(t, d.Comments, app.MaxCommentsPerMergeRequest)
		1269	+
		1270	+	// Resolving still works, and another request has its own budget.
		1271	+	_, err = r.mrs.ResolveComment(r.ctx, mrW, r.repo, mr.Number, 1, true)
		1272	+	require.NoError(t, err)
		1273	+	r.sh("checkout", "-q", "-B", "other", "main")
		1274	+	r.commit(author, "o.txt", "o\n", "other")
		1275	+	r.push("other")
		1276	+	other := r.open("other")
		1277	+	_, err = r.mrs.AddComment(r.ctx, mrW, r.repo, other.Number, app.AddCommentParams{Text: "fine"})
		1278	+	require.NoError(t, err)
		1279	+	}