tenseleyflow/shithub / a3cf168

Browse files

S23: wire required-review gate into Mergeability + Merge (belt-and-braces row-lock recheck)

Authored by mfwolffe <wolffemf@dukes.jmu.edu>
SHA
a3cf16876c859ef3f16bea5ff1502c196696b60c
Parents
70242ad
Tree
891c8f7

2 changed files

StatusFile+-
M internal/pulls/merge.go 17 0
M internal/pulls/pulls.go 55 5
internal/pulls/merge.gomodified
@@ -14,6 +14,7 @@ import (
1414
1515
 	"github.com/tenseleyFlow/shithub/internal/issues"
1616
 	issuesdb "github.com/tenseleyFlow/shithub/internal/issues/sqlc"
17
+	"github.com/tenseleyFlow/shithub/internal/pulls/review"
1718
 	pullsdb "github.com/tenseleyFlow/shithub/internal/pulls/sqlc"
1819
 	repogit "github.com/tenseleyFlow/shithub/internal/repos/git"
1920
 	usersdb "github.com/tenseleyFlow/shithub/internal/users/sqlc"
@@ -84,6 +85,22 @@ func Merge(ctx context.Context, deps Deps, p MergeParams) error {
8485
 		return ErrMergeBlocked
8586
 	}
8687
88
+	// Belt-and-braces: re-evaluate the review gate inside the row
89
+	// lock so a `request_changes` submitted between the last
90
+	// Mergeability tick and this merge attempt is still honored.
91
+	// Spec pitfall: required-review bypass via direct API.
92
+	gate, err := review.Evaluate(ctx, deps.Pool, review.GateInputs{
93
+		RepoID:    issue.RepoID,
94
+		BaseRef:   pr.BaseRef,
95
+		PRIssueID: p.PRID,
96
+	}, int64FromPg(issue.AuthorUserID))
97
+	if err != nil {
98
+		return fmt.Errorf("review gate: %w", err)
99
+	}
100
+	if !gate.Satisfied {
101
+		return ErrMergeBlocked
102
+	}
103
+
87104
 	// Identity for the new commit. Author email rules per spec:
88105
 	//   merge       — author = committer = merger
89106
 	//   squash      — author = PR author; committer = merger
internal/pulls/pulls.gomodified
@@ -29,6 +29,7 @@ import (
2929
3030
 	"github.com/tenseleyFlow/shithub/internal/issues"
3131
 	issuesdb "github.com/tenseleyFlow/shithub/internal/issues/sqlc"
32
+	"github.com/tenseleyFlow/shithub/internal/pulls/review"
3233
 	pullsdb "github.com/tenseleyFlow/shithub/internal/pulls/sqlc"
3334
 	repogit "github.com/tenseleyFlow/shithub/internal/repos/git"
3435
 	mdrender "github.com/tenseleyFlow/shithub/internal/repos/markdown"
@@ -245,6 +246,17 @@ func Synchronize(ctx context.Context, deps Deps, gitDir string, prID int64) erro
245246
 	if err := refreshCommitsAndFiles(ctx, deps, gitDir, prID, baseOID, headOID); err != nil {
246247
 		return err
247248
 	}
249
+	// Re-anchor review comments against the new snapshot. Comments
250
+	// whose original line still exists keep their thread; the rest
251
+	// outdate (current_position=NULL) and surface in the "Show
252
+	// outdated" toggle of the Files tab.
253
+	if err := review.RemapAllForPR(ctx, review.Deps{Pool: deps.Pool, Logger: deps.Logger}, gitDir, prID, baseOID, headOID); err != nil {
254
+		// Best-effort: a position-map miss shouldn't block the sync
255
+		// pipeline. Log + continue.
256
+		if deps.Logger != nil {
257
+			deps.Logger.WarnContext(ctx, "pulls: position remap", "error", err, "pr_id", prID)
258
+		}
259
+	}
248260
 	// Reset mergeability to unknown so the next mergeability tick
249261
 	// recomputes against the fresh snapshot.
250262
 	if err := q.SetPullRequestMergeability(ctx, deps.Pool, pullsdb.SetPullRequestMergeabilityParams{
@@ -267,6 +279,17 @@ func Synchronize(ctx context.Context, deps Deps, gitDir string, prID int64) erro
267279
 }
268280
269281
 // Mergeability runs the merge-tree probe and persists the result.
282
+// Order of state checks (highest priority first):
283
+//
284
+//	dirty   — git merge-tree reports conflicts
285
+//	behind  — head has no commits ahead of base
286
+//	blocked — required reviews missing OR an undismissed
287
+//	          request_changes review exists (S23 gate)
288
+//	clean   — merge-tree clean and review gate satisfied
289
+//
290
+// `blocked` is set by the S23 review evaluator; when no protection
291
+// rule applies and no request_changes review exists, the gate is a
292
+// no-op and we fall through to clean.
270293
 func Mergeability(ctx context.Context, deps Deps, gitDir string, prID int64) error {
271294
 	q := pullsdb.New()
272295
 	pr, err := q.GetPullRequestByIssueID(ctx, deps.Pool, prID)
@@ -277,8 +300,7 @@ func Mergeability(ctx context.Context, deps Deps, gitDir string, prID int64) err
277300
 		return nil // synchronize hasn't run yet; nothing to probe
278301
 	}
279302
 	// Behind: head has no commits ahead of base.
280
-	gitDirCtx := ctx
281
-	commits, err := repogit.CommitsBetweenDetail(gitDirCtx, gitDir, pr.BaseOid, pr.HeadOid, 1)
303
+	commits, err := repogit.CommitsBetweenDetail(ctx, gitDir, pr.BaseOid, pr.HeadOid, 1)
282304
 	if err != nil && !errors.Is(err, repogit.ErrRefNotFound) {
283305
 		return err
284306
 	}
@@ -289,14 +311,34 @@ func Mergeability(ctx context.Context, deps Deps, gitDir string, prID int64) err
289311
 			MergeableState: pullsdb.PrMergeableStateBehind,
290312
 		})
291313
 	}
292
-	res, err := repogit.ProbeMerge(gitDirCtx, gitDir, pr.BaseOid, pr.HeadOid)
314
+	res, err := repogit.ProbeMerge(ctx, gitDir, pr.BaseOid, pr.HeadOid)
293315
 	if err != nil {
294316
 		return fmt.Errorf("probe: %w", err)
295317
 	}
318
+	if res.HasConflict {
319
+		return q.SetPullRequestMergeability(ctx, deps.Pool, pullsdb.SetPullRequestMergeabilityParams{
320
+			IssueID:        prID,
321
+			Mergeable:      pgtype.Bool{Bool: false, Valid: true},
322
+			MergeableState: pullsdb.PrMergeableStateDirty,
323
+		})
324
+	}
325
+	// Review gate. Need the issue's repo + author for the eval.
326
+	issue, err := issuesdb.New().GetIssueByID(ctx, deps.Pool, prID)
327
+	if err != nil {
328
+		return fmt.Errorf("load issue: %w", err)
329
+	}
330
+	gate, err := review.Evaluate(ctx, deps.Pool, review.GateInputs{
331
+		RepoID:    issue.RepoID,
332
+		BaseRef:   pr.BaseRef,
333
+		PRIssueID: prID,
334
+	}, int64FromPg(issue.AuthorUserID))
335
+	if err != nil {
336
+		return fmt.Errorf("review gate: %w", err)
337
+	}
296338
 	state := pullsdb.PrMergeableStateClean
297339
 	mergeable := true
298
-	if res.HasConflict {
299
-		state = pullsdb.PrMergeableStateDirty
340
+	if !gate.Satisfied {
341
+		state = pullsdb.PrMergeableStateBlocked
300342
 		mergeable = false
301343
 	}
302344
 	return q.SetPullRequestMergeability(ctx, deps.Pool, pullsdb.SetPullRequestMergeabilityParams{
@@ -306,6 +348,14 @@ func Mergeability(ctx context.Context, deps Deps, gitDir string, prID int64) err
306348
 	})
307349
 }
308350
351
+// int64FromPg unwraps a pgtype.Int8; returns 0 when invalid.
352
+func int64FromPg(p pgtype.Int8) int64 {
353
+	if !p.Valid {
354
+		return 0
355
+	}
356
+	return p.Int64
357
+}
358
+
309359
 // EditPR updates the PR's title + body. Body markdown is re-rendered
310360
 // via the same pipeline issues.Create uses so HTML is consistent.
311361
 func EditPR(ctx context.Context, deps Deps, prID int64, title, body string) error {