355f5f4 fix(mr): cancel auto-merge when the enabler can no longer merge, even while checks are pending · ermite

M docs/merge-requests.md +1 -1


@@ -46,7 +46,7 @@ A merge request can merge by itself once its checks passed.
46	46		- The repository setting `allow_auto_merge` is off by default: `ermite repo edit <repo> --allow-auto-merge` (`=false` to turn it off).
47	47		- Enable it with `ermite mr merge <repo> <n> --head <commit> --auto --mode squash` (`--head` is required, as for a plain merge; a head that moved is refused with "branch moved"), or with Merge when ready in the web merge box. It needs `mr:merge` and a server with CI. Re-enabling replaces the previous mode and enabler.
48	48		- The server merges when CI has settled on the head with every run complete and at least one check passed. A failing or unfinished check always blocks, whatever `failing_checks_block` says. The mode must still be allowed.
49		-	- The merge is made as the person who enabled it. Their token is checked again at merge time: it must still be valid and allow `mr:merge` on the repository and the target branch.
	49	+	- The merge is made as the person who enabled it. Their token is checked at every evaluation, even while checks are pending, and again at merge time under the merge lock: it must still be valid and allow `mr:merge` on the repository and the target branch. A token that no longer qualifies cancels the auto-merge.
50	50		- A pending auto-merge shows in `mr show` (`auto_merge`), in the web merge box and as `merge_request_auto_merge_enabled` in `ermite watch`. A merge request merged this way has `auto_merged` true.
51	51		- A failing, unfinished or conflicting head waits with auto-merge still on. A re-run or a target fix can make it pass; pushing a new source head cancels the recorded intent (the reason names the new head).
52	52		- A manual merge supersedes auto-merge and publishes `merge_request_merged` with `auto=false`, without a cancelled event. A manual close clears it and publishes `merge_request_closed` followed by `merge_request_auto_merge_cancelled` with reason `closed`.


@@ -46,7 +46,7 @@ A merge request can merge by itself once its checks passed.
46		- The repository setting `allow_auto_merge` is off by default: `ermite repo edit <repo> --allow-auto-merge` (`=false` to turn it off).	46		- The repository setting `allow_auto_merge` is off by default: `ermite repo edit <repo> --allow-auto-merge` (`=false` to turn it off).
47		- Enable it with `ermite mr merge <repo> <n> --head <commit> --auto --mode squash` (`--head` is required, as for a plain merge; a head that moved is refused with "branch moved"), or with Merge when ready in the web merge box. It needs `mr:merge` and a server with CI. Re-enabling replaces the previous mode and enabler.	47		- Enable it with `ermite mr merge <repo> <n> --head <commit> --auto --mode squash` (`--head` is required, as for a plain merge; a head that moved is refused with "branch moved"), or with Merge when ready in the web merge box. It needs `mr:merge` and a server with CI. Re-enabling replaces the previous mode and enabler.
48		- The server merges when CI has settled on the head with every run complete and at least one check passed. A failing or unfinished check always blocks, whatever `failing_checks_block` says. The mode must still be allowed.	48		- The server merges when CI has settled on the head with every run complete and at least one check passed. A failing or unfinished check always blocks, whatever `failing_checks_block` says. The mode must still be allowed.
49	-	- The merge is made as the person who enabled it. Their token is checked again at merge time: it must still be valid and allow `mr:merge` on the repository and the target branch.	49	+	- The merge is made as the person who enabled it. Their token is checked at every evaluation, even while checks are pending, and again at merge time under the merge lock: it must still be valid and allow `mr:merge` on the repository and the target branch. A token that no longer qualifies cancels the auto-merge.
50		- A pending auto-merge shows in `mr show` (`auto_merge`), in the web merge box and as `merge_request_auto_merge_enabled` in `ermite watch`. A merge request merged this way has `auto_merged` true.	50		- A pending auto-merge shows in `mr show` (`auto_merge`), in the web merge box and as `merge_request_auto_merge_enabled` in `ermite watch`. A merge request merged this way has `auto_merged` true.
51		- A failing, unfinished or conflicting head waits with auto-merge still on. A re-run or a target fix can make it pass; pushing a new source head cancels the recorded intent (the reason names the new head).	51		- A failing, unfinished or conflicting head waits with auto-merge still on. A re-run or a target fix can make it pass; pushing a new source head cancels the recorded intent (the reason names the new head).
52		- A manual merge supersedes auto-merge and publishes `merge_request_merged` with `auto=false`, without a cancelled event. A manual close clears it and publishes `merge_request_closed` followed by `merge_request_auto_merge_cancelled` with reason `closed`.	52		- A manual merge supersedes auto-merge and publishes `merge_request_merged` with `auto=false`, without a cancelled event. A manual close clears it and publishes `merge_request_closed` followed by `merge_request_auto_merge_cancelled` with reason `closed`.

M internal/usecase/automerge.go +8 -0


@@ -228,6 +228,14 @@ func (s *AutoMerge) evaluateMR(ctx context.Context, repo domain.Repo, mr domain.
228	228		if head != intent.HeadID {
229	229		return cancel("the branch moved to " + head.String()[:7])
230	230		}
	231	+	// An enabler that can no longer merge cancels at once, even while CI is
	232	+	// pending: the intent must not stay shown as enabled. This check only
	233	+	// cancels; the merge itself resolves the enabler again under the merge lock.
	234	+	if fresh, valid, err := principalOfPusher(ctx, s.cfg.Tokens, intent.EnabledBy, s.cfg.Clock.Now()); err != nil {
	235	+	return err
	236	+	} else if !valid \|\| !fresh.Allows(domain.ScopeMRMerge, mr.Repo) \|\| !fresh.AllowsRef("refs/heads/"+mr.Target) {
	237	+	return cancel("the principal that enabled auto-merge can no longer merge " + mr.Target)
	238	+	}
231	239		if s.cfg.Activity != nil && !s.cfg.Activity.Settled(ctx, mr.Repo, mr.Number, mr.Source, head) {
232	240		return nil
233	241		}


@@ -228,6 +228,14 @@ func (s *AutoMerge) evaluateMR(ctx context.Context, repo domain.Repo, mr domain.
228	if head != intent.HeadID {	228		if head != intent.HeadID {
229	return cancel("the branch moved to " + head.String()[:7])	229		return cancel("the branch moved to " + head.String()[:7])
230	}	230		}
		231	+	// An enabler that can no longer merge cancels at once, even while CI is
		232	+	// pending: the intent must not stay shown as enabled. This check only
		233	+	// cancels; the merge itself resolves the enabler again under the merge lock.
		234	+	if fresh, valid, err := principalOfPusher(ctx, s.cfg.Tokens, intent.EnabledBy, s.cfg.Clock.Now()); err != nil {
		235	+	return err
		236	+	} else if !valid \|\| !fresh.Allows(domain.ScopeMRMerge, mr.Repo) \|\| !fresh.AllowsRef("refs/heads/"+mr.Target) {
		237	+	return cancel("the principal that enabled auto-merge can no longer merge " + mr.Target)
		238	+	}
231	if s.cfg.Activity != nil && !s.cfg.Activity.Settled(ctx, mr.Repo, mr.Number, mr.Source, head) {	239		if s.cfg.Activity != nil && !s.cfg.Activity.Settled(ctx, mr.Repo, mr.Number, mr.Source, head) {
232	return nil	240		return nil
233	}	241		}

M internal/usecase/automerge_test.go +7 -4


@@ -208,7 +208,7 @@ func TestAutoMergeCancelsWhenEnablerTokenRevokedOrExpired(t *testing.T) {
208	208		enabled.AutoMerge.EnabledBy = domain.CIPusher{Kind: domain.CIPusherToken, TokenID: "gone"}
209	209		require.NoError(t, r.store.MergeRequests().Update(r.ctx, enabled))
210	210		tokens := mocks.NewTokenStore(t)
211		-	checks.EXPECT().CommitChecks(mock.Anything, r.repo, r.head().String()).Return([]domain.CICheck{{Status: domain.CICompleted, Conclusion: domain.CISuccess}}, nil)
	211	+	// The enabler is checked before CI: no check is read.
212	212		if expired {
213	213		tokens.EXPECT().Get(mock.Anything, "gone").Return(domain.Token{ExpiresAt: now.Add(-time.Hour)}, nil)
214	214		} else {

@@ -405,6 +405,10 @@ func TestAutoMergeResolvesEnablerAfterMergeGate(t *testing.T) {
405	405		enabled.AutoMerge.EnabledBy = domain.CIPusher{Kind: domain.CIPusherToken, TokenID: "revoked"}
406	406		require.NoError(t, r.store.MergeRequests().Update(r.ctx, enabled))
407	407		tokens := mocks.NewTokenStore(t)
	408	+	// Valid when the evaluator looks before CI, revoked by the time it holds the
	409	+	// merge lock: the merge must use the fresh answer.
	410	+	validTok := domain.Token{ID: "revoked", Scopes: []domain.Scope{domain.ScopeMRMerge}, Repos: []domain.RepoName{r.repo}}
	411	+	tokens.EXPECT().Get(mock.Anything, "revoked").Return(validTok, nil).Once()
408	412		tokens.EXPECT().Get(mock.Anything, "revoked").Return(domain.Token{}, domain.ErrNotFound).Once()
409	413		reached := make(chan struct{})
410	414		checks.EXPECT().CommitChecks(mock.Anything, r.repo, r.head().String()).Run(func(context.Context, domain.RepoName, string) { close(reached) }).Return([]domain.CICheck{{Status: domain.CICompleted, Conclusion: domain.CISuccess}}, nil).Once()

@@ -414,9 +418,8 @@ func TestAutoMergeResolvesEnablerAfterMergeGate(t *testing.T) {
414	418		require.NoError(t, r.mrs.WithMergeLock(r.ctx, r.repo, func() error {
415	419		go func() { done <- a.Evaluate(r.repo) }()
416	420		<-reached
417		-	// Evaluation has passed its fast-path checks, but cannot resolve the token
418		-	// until this lock is released. It now sees the revoked token, not old rights.
419		-	tokens.AssertNotCalled(t, "Get", mock.Anything, "revoked")
	421	+	// Evaluation has passed its fast-path checks with the then-valid token
	422	+	// and waits for this lock. The token is revoked meanwhile (second Get).
420	423		return nil
421	424		}))
422	425		require.True(t, <-done)

 			enabled.AutoMerge.EnabledBy = domain.CIPusher{Kind: domain.CIPusherToken, TokenID: "gone"}
 			require.NoError(t, r.store.MergeRequests().Update(r.ctx, enabled))
 			tokens := mocks.NewTokenStore(t)
-			checks.EXPECT().CommitChecks(mock.Anything, r.repo, r.head().String()).Return([]domain.CICheck{{Status: domain.CICompleted, Conclusion: domain.CISuccess}}, nil)
+			// The enabler is checked before CI: no check is read.
 			if expired {
 				tokens.EXPECT().Get(mock.Anything, "gone").Return(domain.Token{ExpiresAt: now.Add(-time.Hour)}, nil)
 			} else {
 	enabled.AutoMerge.EnabledBy = domain.CIPusher{Kind: domain.CIPusherToken, TokenID: "revoked"}
 	require.NoError(t, r.store.MergeRequests().Update(r.ctx, enabled))
 	tokens := mocks.NewTokenStore(t)
+	// Valid when the evaluator looks before CI, revoked by the time it holds the
+	// merge lock: the merge must use the fresh answer.
+	validTok := domain.Token{ID: "revoked", Scopes: []domain.Scope{domain.ScopeMRMerge}, Repos: []domain.RepoName{r.repo}}
+	tokens.EXPECT().Get(mock.Anything, "revoked").Return(validTok, nil).Once()
 	tokens.EXPECT().Get(mock.Anything, "revoked").Return(domain.Token{}, domain.ErrNotFound).Once()
 	reached := make(chan struct{})
 	checks.EXPECT().CommitChecks(mock.Anything, r.repo, r.head().String()).Run(func(context.Context, domain.RepoName, string) { close(reached) }).Return([]domain.CICheck{{Status: domain.CICompleted, Conclusion: domain.CISuccess}}, nil).Once()
 	require.NoError(t, r.mrs.WithMergeLock(r.ctx, r.repo, func() error {
 		go func() { done <- a.Evaluate(r.repo) }()
 		<-reached
-		// Evaluation has passed its fast-path checks, but cannot resolve the token
+		// Evaluation has passed its fast-path checks with the then-valid token
-		// until this lock is released. It now sees the revoked token, not old rights.
+		// and waits for this lock. The token is revoked meanwhile (second Get).
-		tokens.AssertNotCalled(t, "Get", mock.Anything, "revoked")
 		return nil
 	}))
 	require.True(t, <-done)