`70242ad`

S23: review orchestrator (submit/comment/resolve/dismiss/request/required/position-map) + tests

Authored by mfwolffe <wolffemf@dukes.jmu.edu> 5 days ago

SHA: 70242ad1e7a7351d03cf5062f83072ac93595c92
Parents: 54bdb31
Tree: 131462c

7 changed files

Status	File	+
A	`internal/pulls/review/comment.go`	159
A	`internal/pulls/review/position_map.go`	117
A	`internal/pulls/review/request.go`	51
A	`internal/pulls/review/required.go`	171
A	`internal/pulls/review/review.go`	46
A	`internal/pulls/review/review_test.go`	400
A	`internal/pulls/review/submit.go`	127

internal/pulls/review/comment.goadded

 +// SPDX-License-Identifier: AGPL-3.0-or-later
++
 +package review
++
 +import (
 +	"context"
 +	"errors"
 +	"strings"
 +	"time"
++
 +	"github.com/jackc/pgx/v5"
 +	"github.com/jackc/pgx/v5/pgtype"
++
 +	pullsdb "github.com/tenseleyFlow/shithub/internal/pulls/sqlc"
 +	mdrender "github.com/tenseleyFlow/shithub/internal/repos/markdown"
 +)
++
 +// CommentParams describes a single inline comment on a PR. When
 +// `Pending` is true the comment is filed as a draft attached to the
 +// author's pending review (review_id stays NULL); SubmitReview later
 +// flips pending=false and sets review_id.
 +type CommentParams struct {
 +	PRIssueID         int64
 +	AuthorUserID      int64
 +	FilePath          string
 +	Side              string // "left" | "right"
 +	OriginalCommitSHA string
 +	OriginalLine      int32
 +	OriginalPosition  int32
 +	CurrentPosition   int32 // -1 means already outdated (rare on insert)
 +	Body              string
 +	InReplyToID       int64 // 0 means top-level
 +	Pending           bool
 +}
++
 +// AddComment inserts a single inline comment. When InReplyToID is set
 +// the new comment threads under that one (validated to belong to the
 +// same PR). Markdown is rendered to body_html_cached at insert time.
 +func AddComment(ctx context.Context, deps Deps, p CommentParams) (pullsdb.PrReviewComment, error) {
 +	body := strings.TrimSpace(p.Body)
 +	if body == "" {
 +		return pullsdb.PrReviewComment{}, ErrEmptyBody
 +	}
 +	if len(body) > 65535 {
 +		return pullsdb.PrReviewComment{}, ErrBodyTooLong
 +	}
 +	if p.Side != "left" && p.Side != "right" {
 +		p.Side = "right"
 +	}
++
 +	q := pullsdb.New()
++
 +	// Reply validation: parent must exist on the same PR. We don't
 +	// inherit FilePath/Side from the parent — the caller passes the
 +	// correct anchor; the in-reply-to link is purely for threading.
 +	if p.InReplyToID != 0 {
 +		parent, err := q.GetPRReviewComment(ctx, deps.Pool, p.InReplyToID)
 +		if err != nil {
 +			if errors.Is(err, pgx.ErrNoRows) {
 +				return pullsdb.PrReviewComment{}, ErrCommentNotOnPR
 +			}
 +			return pullsdb.PrReviewComment{}, err
 +		}
 +		if parent.PrIssueID != p.PRIssueID {
 +			return pullsdb.PrReviewComment{}, ErrCommentNotOnPR
 +		}
 +		// Inherit anchor from parent so the whole thread renders on
 +		// the same diff line even if the caller forgot to pass it.
 +		p.FilePath = parent.FilePath
 +		p.Side = string(parent.Side)
 +		p.OriginalCommitSHA = parent.OriginalCommitSha
 +		p.OriginalLine = parent.OriginalLine
 +		p.OriginalPosition = parent.OriginalPosition
 +		if parent.CurrentPosition.Valid {
 +			p.CurrentPosition = parent.CurrentPosition.Int32
 +		}
 +	}
++
 +	html, _ := mdrender.RenderHTML([]byte(body))
++
 +	cur := pgtype.Int4{}
 +	if p.CurrentPosition >= 0 {
 +		cur = pgtype.Int4{Int32: p.CurrentPosition, Valid: true}
 +	}
++
 +	row, err := q.CreatePRReviewComment(ctx, deps.Pool, pullsdb.CreatePRReviewCommentParams{
 +		PrIssueID:         p.PRIssueID,
 +		ReviewID:          pgtype.Int8{},
 +		AuthorUserID:      pgtype.Int8{Int64: p.AuthorUserID, Valid: p.AuthorUserID != 0},
 +		FilePath:          p.FilePath,
 +		Side:              pullsdb.PrReviewSide(p.Side),
 +		OriginalCommitSha: p.OriginalCommitSHA,
 +		OriginalLine:      p.OriginalLine,
 +		OriginalPosition:  p.OriginalPosition,
 +		CurrentPosition:   cur,
 +		Body:              body,
 +		BodyHtmlCached:    pgtype.Text{String: html, Valid: html != ""},
 +		InReplyToID:       pgtype.Int8{Int64: p.InReplyToID, Valid: p.InReplyToID != 0},
 +		Pending:           p.Pending,
 +	})
 +	if err != nil {
 +		return pullsdb.PrReviewComment{}, err
 +	}
 +	return row, nil
 +}
++
 +// EditComment updates the body of an existing comment. Caller is
 +// expected to have already enforced "actor == comment.author OR
 +// repo admin" via policy.
 +func EditComment(ctx context.Context, deps Deps, commentID int64, body string) error {
 +	body = strings.TrimSpace(body)
 +	if body == "" {
 +		return ErrEmptyBody
 +	}
 +	if len(body) > 65535 {
 +		return ErrBodyTooLong
 +	}
 +	html, _ := mdrender.RenderHTML([]byte(body))
 +	return pullsdb.New().UpdatePRReviewCommentBody(ctx, deps.Pool, pullsdb.UpdatePRReviewCommentBodyParams{
 +		ID: commentID, Body: body, BodyHtmlCached: pgtype.Text{String: html, Valid: html != ""},
 +	})
 +}
++
 +// Resolve marks the thread (root + replies) resolved. The "thread" is
 +// keyed off the comment id; UI walks `in_reply_to_id` to render
 +// replies, so resolving the root collapses the whole conversation.
 +// Default-collapsed in the Files tab; "Show resolved" toggle reopens.
 +func Resolve(ctx context.Context, deps Deps, actorUserID, commentID int64) error {
 +	q := pullsdb.New()
 +	c, err := q.GetPRReviewComment(ctx, deps.Pool, commentID)
 +	if err != nil {
 +		return err
 +	}
 +	if c.ResolvedAt.Valid {
 +		return ErrAlreadyResolved
 +	}
 +	return q.SetPRReviewCommentResolved(ctx, deps.Pool, pullsdb.SetPRReviewCommentResolvedParams{
 +		ID:               commentID,
 +		ResolvedAt:       pgtype.Timestamptz{Time: time.Now(), Valid: true},
 +		ResolvedByUserID: pgtype.Int8{Int64: actorUserID, Valid: actorUserID != 0},
 +	})
 +}
++
 +// Reopen clears the resolved-at marker on a thread.
 +func Reopen(ctx context.Context, deps Deps, commentID int64) error {
 +	q := pullsdb.New()
 +	c, err := q.GetPRReviewComment(ctx, deps.Pool, commentID)
 +	if err != nil {
 +		return err
 +	}
 +	if !c.ResolvedAt.Valid {
 +		return ErrNotResolved
 +	}
 +	return q.SetPRReviewCommentResolved(ctx, deps.Pool, pullsdb.SetPRReviewCommentResolvedParams{
 +		ID:               commentID,
 +		ResolvedAt:       pgtype.Timestamptz{Valid: false},
 +		ResolvedByUserID: pgtype.Int8{Valid: false},
 +	})
 +}

internal/pulls/review/position_map.goadded

 +// SPDX-License-Identifier: AGPL-3.0-or-later
++
 +package review
++
 +import (
 +	"context"
 +	"fmt"
++
 +	"github.com/jackc/pgx/v5/pgtype"
++
 +	pullsdb "github.com/tenseleyFlow/shithub/internal/pulls/sqlc"
 +	repogit "github.com/tenseleyFlow/shithub/internal/repos/git"
 +)
++
 +// RemapAllForPR re-anchors every non-draft review comment on a PR
 +// against the new (base, head) snapshot. For each comment:
 +//
 +//	1. Re-walk the diff for its file at (newBase..newHead).
 +//	2. If the line at original_line still appears on the chosen side
 +//	   (left = base, right = head), set current_position to its new
 +//	   position index. Otherwise NULL → outdated.
 +//
 +// This is the conservative v1 implementation. The spec calls out
 +// `git blame --porcelain` as a richer mapper for rebase-heavy PRs;
 +// add that when the simple line-presence check proves insufficient.
 +//
 +// The function reads `pr_review_comments` then issues per-row
 +// SetPRReviewCommentCurrentPosition updates. Idempotent: re-running
 +// converges on the same answer.
 +func RemapAllForPR(ctx context.Context, deps Deps, gitDir string, prID int64, newBaseOID, newHeadOID string) error {
 +	if newBaseOID == "" || newHeadOID == "" {
 +		return nil
 +	}
 +	q := pullsdb.New()
 +	rows, err := q.ListNonDraftCommentsForPositionMap(ctx, deps.Pool, prID)
 +	if err != nil {
 +		return fmt.Errorf("position map list: %w", err)
 +	}
 +	if len(rows) == 0 {
 +		return nil
 +	}
++
 +	// Build per-file caches keyed on (file_path, side). Each entry
 +	// stores the line-text → position map for that side of the new
 +	// diff. v1 reads file contents at the relevant snapshot OID and
 +	// computes positions on demand.
 +	type sideKey struct {
 +		path string
 +		side string
 +	}
 +	cache := map[sideKey]map[int]int{} // line# (from original) → new position
++
 +	getMap := func(path, side string) (map[int]int, error) {
 +		k := sideKey{path, side}
 +		if m, ok := cache[k]; ok {
 +			return m, nil
 +		}
 +		// For the v1 mapper we just compute position-by-line via the
 +		// snapshot file content. side=right uses newHeadOID; side=left
 +		// uses newBaseOID.
 +		ref := newHeadOID
 +		if side == "left" {
 +			ref = newBaseOID
 +		}
 +		// Inline cap on file size — we don't bother mapping files >
 +		// 1 MiB; those comments outdate.
 +		const maxBytes = 1 << 20
 +		blob, err := repogit.ReadBlobBytes(ctx, gitDir, ref, path, maxBytes)
 +		if err != nil {
 +			cache[k] = nil // miss → outdate everything in this file
 +			return nil, nil
 +		}
 +		m := buildLineMap(blob)
 +		cache[k] = m
 +		return m, nil
 +	}
++
 +	for _, c := range rows {
 +		m, _ := getMap(c.FilePath, string(c.Side))
 +		var newPos pgtype.Int4
 +		if m != nil {
 +			if pos, ok := m[int(c.OriginalLine)]; ok {
 +				newPos = pgtype.Int4{Int32: int32(pos), Valid: true}
 +			}
 +		}
 +		if err := q.SetPRReviewCommentCurrentPosition(ctx, deps.Pool, pullsdb.SetPRReviewCommentCurrentPositionParams{
 +			ID:              c.ID,
 +			CurrentPosition: newPos,
 +		}); err != nil {
 +			return fmt.Errorf("position map update: %w", err)
 +		}
 +	}
 +	return nil
 +}
++
 +// buildLineMap returns line-number → position-index for a blob. v1
 +// just maps line N to position N — line numbers in the blob *are*
 +// the positions for the simple "line still exists" check. The map
 +// will be replaced with a content-aware mapping when we add the
 +// blame-based variant.
 +func buildLineMap(blob []byte) map[int]int {
 +	out := map[int]int{}
 +	line := 1
 +	pos := 1
 +	for i := 0; i < len(blob); i++ {
 +		if blob[i] == '\n' {
 +			out[line] = pos
 +			line++
 +			pos++
 +		}
 +	}
 +	// Trailing line without newline.
 +	if len(blob) > 0 && blob[len(blob)-1] != '\n' {
 +		out[line] = pos
 +	}
 +	return out
 +}

internal/pulls/review/request.goadded

 +// SPDX-License-Identifier: AGPL-3.0-or-later
++
 +package review
++
 +import (
 +	"context"
++
 +	"github.com/jackc/pgx/v5/pgtype"
++
 +	pullsdb "github.com/tenseleyFlow/shithub/internal/pulls/sqlc"
 +)
++
 +// RequestParams describes a review-request action.
 +type RequestParams struct {
 +	PRIssueID         int64
 +	RequestedUserID   int64
 +	RequestedByUserID int64
 +}
++
 +// Request creates a pr_review_requests row. Bounded by
 +// MaxReviewersPerPR per the spec pitfalls section.
 +func Request(ctx context.Context, deps Deps, p RequestParams) (pullsdb.PrReviewRequest, error) {
 +	q := pullsdb.New()
 +	count, err := q.CountActivePRReviewRequests(ctx, deps.Pool, p.PRIssueID)
 +	if err != nil {
 +		return pullsdb.PrReviewRequest{}, err
 +	}
 +	if int(count) >= MaxReviewersPerPR {
 +		return pullsdb.PrReviewRequest{}, ErrReviewerLimitReached
 +	}
 +	// Idempotent-ish: if the same reviewer is already pending, don't
 +	// add a duplicate row. Walking the active list is cheaper than
 +	// adding a partial unique index given v1 PRs typically have
 +	// single-digit reviewer counts.
 +	existing, err := q.ListPRReviewRequests(ctx, deps.Pool, p.PRIssueID)
 +	if err != nil {
 +		return pullsdb.PrReviewRequest{}, err
 +	}
 +	for _, e := range existing {
 +		if e.RequestedUserID.Valid && e.RequestedUserID.Int64 == p.RequestedUserID &&
 +			!e.DismissedAt.Valid && !e.SatisfiedByReviewID.Valid {
 +			return pullsdb.PrReviewRequest{}, ErrReviewerAlreadyPending
 +		}
 +	}
 +	return q.CreatePRReviewRequest(ctx, deps.Pool, pullsdb.CreatePRReviewRequestParams{
 +		PrIssueID:         p.PRIssueID,
 +		RequestedUserID:   pgtype.Int8{Int64: p.RequestedUserID, Valid: p.RequestedUserID != 0},
 +		RequestedTeamID:   pgtype.Int8{Valid: false},
 +		RequestedByUserID: pgtype.Int8{Int64: p.RequestedByUserID, Valid: p.RequestedByUserID != 0},
 +	})
 +}

internal/pulls/review/required.goadded

 +// SPDX-License-Identifier: AGPL-3.0-or-later
++
 +package review
++
 +import (
 +	"context"
 +	"path/filepath"
++
 +	"github.com/jackc/pgx/v5/pgtype"
 +	"github.com/jackc/pgx/v5/pgxpool"
++
 +	pullsdb "github.com/tenseleyFlow/shithub/internal/pulls/sqlc"
 +	reposdb "github.com/tenseleyFlow/shithub/internal/repos/sqlc"
 +)
++
 +// GateInputs is the small struct the merge gate cares about.
 +type GateInputs struct {
 +	RepoID    int64
 +	BaseRef   string
 +	PRIssueID int64
 +}
++
 +// GateResult mirrors the spec's mergeable_state vocabulary for
 +// review-side blocks.
 +type GateResult struct {
 +	// Satisfied — the PR meets the review requirements (or none apply).
 +	Satisfied bool
 +	// Reason — short human-readable cause when not satisfied. Used by
 +	// the UI banner ("Approval required (1/1 reviews missing)").
 +	Reason string
 +	// RequiredCount — the rule's required_review_count.
 +	RequiredCount int
 +	// CurrentApprovals — count of "latest review per author" with state
 +	// approve. Always excludes the PR author.
 +	CurrentApprovals int
 +	// HasRequestChanges — at least one undismissed `request_changes`
 +	// review whose author hasn't superseded it with a later approve.
 +	HasRequestChanges bool
 +}
++
 +// Evaluate computes the gate against the matching branch-protection
 +// rule. Returns Satisfied=true when no rule matches the base ref or
 +// when the rule's required_review_count == 0 and no unresolved
 +// request_changes reviews exist.
 +//
 +// "Latest review per author" semantics: when an author submits a
 +// request_changes and later submits an approve, only the approve
 +// counts. When two distinct authors approve and one of them later
 +// submits a comment, both still count for the approval tally — only
 +// approve and request_changes shift the per-author state.
 +func Evaluate(ctx context.Context, pool *pgxpool.Pool, in GateInputs, prAuthorUserID int64) (GateResult, error) {
 +	// Locate the longest-pattern-matching protection rule for this
 +	// base ref. The same matching helper that S20 uses lives in
 +	// internal/repos/protection but we don't import it here to avoid
 +	// a circular dependency on the repos package — duplicate the
 +	// `filepath.Match` longest-match shape inline (cheap).
 +	rules, err := loadProtectionRules(ctx, pool, in.RepoID)
 +	if err != nil {
 +		return GateResult{}, err
 +	}
 +	rule, hasRule := matchRule(rules, in.BaseRef)
++
 +	// If there's no rule, approve count + request_changes still gate
 +	// the merge per spec ("no unresolved request_changes reviews").
 +	q := pullsdb.New()
 +	reviews, err := q.ListUndismissedReviewsForGate(ctx, pool, in.PRIssueID)
 +	if err != nil {
 +		return GateResult{}, err
 +	}
 +	approves, requestChanges := latestPerAuthor(reviews, prAuthorUserID)
++
 +	required := 0
 +	if hasRule {
 +		required = int(rule.RequiredReviewCount)
 +	}
 +	out := GateResult{
 +		RequiredCount:     required,
 +		CurrentApprovals:  approves,
 +		HasRequestChanges: requestChanges,
 +	}
++
 +	switch {
 +	case requestChanges:
 +		out.Reason = "Changes requested by a reviewer."
 +	case approves < required:
 +		out.Reason = "Approval required."
 +	default:
 +		out.Satisfied = true
 +	}
 +	return out, nil
 +}
++
 +// loadProtectionRules reads every rule on the repo. Tiny set in
 +// practice; cheaper than per-request matching at the SQL layer.
 +func loadProtectionRules(ctx context.Context, pool *pgxpool.Pool, repoID int64) ([]reposdb.BranchProtectionRule, error) {
 +	return reposdb.New().ListBranchProtectionRules(ctx, pool, repoID)
 +}
++
 +// matchRule duplicates the longest-pattern-wins algorithm from
 +// internal/repos/protection so this package doesn't pull that import
 +// graph. Same behaviour: longest pattern (alphabetical tiebreaker)
 +// wins, no rule means no match.
 +func matchRule(rules []reposdb.BranchProtectionRule, branch string) (reposdb.BranchProtectionRule, bool) {
 +	var best reposdb.BranchProtectionRule
 +	bestLen := -1
 +	for _, r := range rules {
 +		ok, _ := filepath.Match(r.Pattern, branch)
 +		if !ok {
 +			continue
 +		}
 +		if len(r.Pattern) > bestLen ||
 +			(len(r.Pattern) == bestLen && r.Pattern < best.Pattern) {
 +			best = r
 +			bestLen = len(r.Pattern)
 +		}
 +	}
 +	return best, bestLen >= 0
 +}
++
 +// latestPerAuthor reduces reviews to a per-author "winning" state.
 +// approve and request_changes update the author's tally; comment-state
 +// reviews are ignored (they don't shift the gate state per spec).
 +//
 +// Author of the PR is excluded from the approval count.
 +func latestPerAuthor(reviews []pullsdb.ListUndismissedReviewsForGateRow, prAuthorUserID int64) (approves int, requestChanges bool) {
 +	// Reviews are ordered by (author_user_id, submitted_at) so the
 +	// last entry per author is the latest decision. Track the per-
 +	// author winner inline.
 +	var (
 +		curAuthor int64 = 0
 +		curState  pullsdb.PrReviewState
 +		curValid  bool
 +	)
 +	flush := func() {
 +		if !curValid || curAuthor == prAuthorUserID || curAuthor == 0 {
 +			return
 +		}
 +		switch curState {
 +		case pullsdb.PrReviewStateApprove:
 +			approves++
 +		case pullsdb.PrReviewStateRequestChanges:
 +			requestChanges = true
 +		}
 +	}
 +	for _, r := range reviews {
 +		auth := int64FromPg(r.AuthorUserID)
 +		if auth != curAuthor {
 +			flush()
 +			curAuthor = auth
 +			curState = r.State
 +			curValid = true
 +			continue
 +		}
 +		// Same author — newer row wins, but only approve / request_changes
 +		// shift the author's state. A trailing `comment` doesn't reset
 +		// the prior `approve`.
 +		switch r.State {
 +		case pullsdb.PrReviewStateApprove, pullsdb.PrReviewStateRequestChanges:
 +			curState = r.State
 +		}
 +	}
 +	flush()
 +	return approves, requestChanges
 +}
++
 +func int64FromPg(p pgtype.Int8) int64 {
 +	if !p.Valid {
 +		return 0
 +	}
 +	return p.Int64
 +}

internal/pulls/review/review.goadded

 +// SPDX-License-Identifier: AGPL-3.0-or-later
++
 +// Package review owns PR-review orchestration: inline comments,
 +// review submission with attached pending comments, dismissal,
 +// reviewer requests, thread resolution, and the required-reviews
 +// gate consulted by the merge handler.
 +//
 +// Diff-position anchoring model (matches GitHub's contract): each
 +// comment captures (file_path, side, original_commit_sha,
 +// original_line, original_position). The PR synchronize pipeline
 +// re-walks the diff against the new head and updates each comment's
 +// current_position; comments whose anchor line no longer exists go
 +// to current_position=NULL ("outdated"). The position mapping
 +// helper here is invoked from `pulls.Synchronize`.
 +package review
++
 +import (
 +	"errors"
 +	"log/slog"
++
 +	"github.com/jackc/pgx/v5/pgxpool"
 +)
++
 +// Deps wires this package into the runtime.
 +type Deps struct {
 +	Pool   *pgxpool.Pool
 +	Logger *slog.Logger
 +}
++
 +// Errors surfaced to handlers.
 +var (
 +	ErrEmptyBody              = errors.New("review: comment body is required")
 +	ErrBodyTooLong            = errors.New("review: body too long")
 +	ErrAuthorCannotApprove    = errors.New("review: author cannot approve their own PR")
 +	ErrInvalidState           = errors.New("review: state must be comment, approve, or request_changes")
 +	ErrCommentNotOnPR         = errors.New("review: comment does not belong to this PR")
 +	ErrAlreadyResolved        = errors.New("review: thread already resolved")
 +	ErrNotResolved            = errors.New("review: thread is not resolved")
 +	ErrReviewerLimitReached   = errors.New("review: 20 reviewers max per PR")
 +	ErrReviewerAlreadyPending = errors.New("review: reviewer already requested")
 +	ErrReviewNotFound         = errors.New("review: review not found")
 +)
++
 +// MaxReviewersPerPR caps active review requests per PR. Matches the
 +// spec pitfall section.
 +const MaxReviewersPerPR = 20

internal/pulls/review/review_test.goadded

 +// SPDX-License-Identifier: AGPL-3.0-or-later
++
 +package review_test
++
 +import (
 +	"context"
 +	"io"
 +	"log/slog"
 +	"os"
 +	"os/exec"
 +	"path/filepath"
 +	"strings"
 +	"testing"
++
 +	"github.com/jackc/pgx/v5/pgtype"
 +	"github.com/jackc/pgx/v5/pgxpool"
++
 +	issuesdb "github.com/tenseleyFlow/shithub/internal/issues/sqlc"
 +	"github.com/tenseleyFlow/shithub/internal/pulls"
 +	"github.com/tenseleyFlow/shithub/internal/pulls/review"
 +	pullsdb "github.com/tenseleyFlow/shithub/internal/pulls/sqlc"
 +	reposdb "github.com/tenseleyFlow/shithub/internal/repos/sqlc"
 +	"github.com/tenseleyFlow/shithub/internal/testing/dbtest"
 +	usersdb "github.com/tenseleyFlow/shithub/internal/users/sqlc"
 +)
++
 +const fixtureHash = "$argon2id$v=19$m=16384,t=1,p=1$" +
 +	"AAAAAAAAAAAAAAAA$" +
 +	"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
++
 +func gitCmd(args ...string) *exec.Cmd {
 +	//nolint:gosec
 +	return exec.Command("git", args...)
 +}
++
 +type fx struct {
 +	pool        *pgxpool.Pool
 +	pullsDeps   pulls.Deps
 +	reviewDeps  review.Deps
 +	authorID    int64
 +	reviewerID  int64
 +	otherID     int64
 +	repoID      int64
 +	gitDir      string
 +}
++
 +func setup(t *testing.T) fx {
 +	t.Helper()
 +	pool := dbtest.NewTestDB(t)
 +	ctx := context.Background()
++
 +	uq := usersdb.New()
 +	mkUser := func(name string) usersdb.User {
 +		u, err := uq.CreateUser(ctx, pool, usersdb.CreateUserParams{
 +			Username: name, DisplayName: strings.ToTitle(name), PasswordHash: fixtureHash,
 +		})
 +		if err != nil {
 +			t.Fatalf("CreateUser %s: %v", name, err)
 +		}
 +		em, err := uq.CreateUserEmail(ctx, pool, usersdb.CreateUserEmailParams{
 +			UserID: u.ID, Email: name + "@example.com", IsPrimary: true, Verified: true,
 +		})
 +		if err != nil {
 +			t.Fatalf("CreateUserEmail: %v", err)
 +		}
 +		if err := uq.LinkUserPrimaryEmail(ctx, pool, usersdb.LinkUserPrimaryEmailParams{
 +			ID: u.ID, PrimaryEmailID: pgtype.Int8{Int64: em.ID, Valid: true},
 +		}); err != nil {
 +			t.Fatalf("LinkUserPrimaryEmail: %v", err)
 +		}
 +		return u
 +	}
 +	author := mkUser("alice")
 +	reviewer := mkUser("bob")
 +	other := mkUser("carol")
++
 +	rq := reposdb.New()
 +	repo, err := rq.CreateRepo(ctx, pool, reposdb.CreateRepoParams{
 +		OwnerUserID:   pgtype.Int8{Int64: author.ID, Valid: true},
 +		Name:          "demo",
 +		DefaultBranch: "trunk",
 +		Visibility:    reposdb.RepoVisibilityPublic,
 +	})
 +	if err != nil {
 +		t.Fatalf("CreateRepo: %v", err)
 +	}
 +	if err := issuesdb.New().EnsureRepoIssueCounter(ctx, pool, repo.ID); err != nil {
 +		t.Fatalf("EnsureRepoIssueCounter: %v", err)
 +	}
++
 +	root := t.TempDir()
 +	gitDir := filepath.Join(root, "demo.git")
 +	if out, err := gitCmd("init", "--bare", "-b", "trunk", gitDir).CombinedOutput(); err != nil {
 +		t.Fatalf("git init --bare: %v (%s)", err, out)
 +	}
++
 +	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
 +	return fx{
 +		pool: pool,
 +		pullsDeps:  pulls.Deps{Pool: pool, Logger: logger},
 +		reviewDeps: review.Deps{Pool: pool, Logger: logger},
 +		authorID:   author.ID,
 +		reviewerID: reviewer.ID,
 +		otherID:    other.ID,
 +		repoID:     repo.ID,
 +		gitDir:     gitDir,
 +	}
 +}
++
 +func commitOnBranch(t *testing.T, gitDir, branch, msg, file, contents string) string {
 +	t.Helper()
 +	wt := t.TempDir()
 +	addArgs := []string{"-C", gitDir, "worktree", "add"}
 +	if _, err := gitCmd("-C", gitDir, "show-ref", "--verify", "refs/heads/"+branch).CombinedOutput(); err != nil {
 +		addArgs = append(addArgs, "-b", branch, wt)
 +	} else {
 +		addArgs = append(addArgs, wt, branch)
 +	}
 +	if out, err := gitCmd(addArgs...).CombinedOutput(); err != nil {
 +		t.Fatalf("worktree add %s: %v (%s)", branch, err, out)
 +	}
 +	defer func() {
 +		_ = gitCmd("-C", gitDir, "worktree", "remove", "--force", wt).Run()
 +	}()
 +	if err := os.WriteFile(filepath.Join(wt, file), []byte(contents), 0o644); err != nil { //nolint:gosec
 +		t.Fatalf("write %s: %v", file, err)
 +	}
 +	for _, args := range [][]string{
 +		{"-C", wt, "config", "user.name", "Alice"},
 +		{"-C", wt, "config", "user.email", "alice@example.com"},
 +		{"-C", wt, "add", "."},
 +		{"-C", wt, "commit", "-m", msg},
 +	} {
 +		if out, err := gitCmd(args...).CombinedOutput(); err != nil {
 +			t.Fatalf("%v: %v (%s)", args, err, out)
 +		}
 +	}
 +	out, err := gitCmd("-C", wt, "rev-parse", "HEAD").Output()
 +	if err != nil {
 +		t.Fatalf("rev-parse HEAD: %v", err)
 +	}
 +	return strings.TrimSpace(string(out))
 +}
++
 +// openPR opens a same-repo PR and runs Mergeability so the state is
 +// fresh.
 +func (f fx) openPR(t *testing.T, base, head string) pullsdb.PullRequest {
 +	t.Helper()
 +	res, err := pulls.Create(context.Background(), f.pullsDeps, pulls.CreateParams{
 +		RepoID: f.repoID, AuthorUserID: f.authorID,
 +		Title: "test PR", BaseRef: base, HeadRef: head, GitDir: f.gitDir,
 +	})
 +	if err != nil {
 +		t.Fatalf("openPR: %v", err)
 +	}
 +	if err := pulls.Mergeability(context.Background(), f.pullsDeps, f.gitDir, res.PullRequest.IssueID); err != nil {
 +		t.Fatalf("Mergeability: %v", err)
 +	}
 +	return res.PullRequest
 +}
++
 +func TestSubmit_AuthorCannotApprove(t *testing.T) {
 +	f := setup(t)
 +	commitOnBranch(t, f.gitDir, "trunk", "init", "README.md", "hi\n")
 +	commitOnBranch(t, f.gitDir, "feature", "add", "x.txt", "x\n")
 +	pr := f.openPR(t, "trunk", "feature")
++
 +	_, err := review.Submit(context.Background(), f.reviewDeps, review.SubmitParams{
 +		PRIssueID: pr.IssueID, AuthorUserID: f.authorID,
 +		State: "approve", PRAuthorUserID: f.authorID,
 +	})
 +	if err == nil {
 +		t.Fatalf("expected ErrAuthorCannotApprove, got nil")
 +	}
 +}
++
 +func TestSubmit_AttachesPendingComments(t *testing.T) {
 +	f := setup(t)
 +	ctx := context.Background()
 +	commitOnBranch(t, f.gitDir, "trunk", "init", "README.md", "hi\n")
 +	commitOnBranch(t, f.gitDir, "feature", "add", "x.txt", "x\n")
 +	pr := f.openPR(t, "trunk", "feature")
++
 +	// Two pending draft comments by the reviewer.
 +	if _, err := review.AddComment(ctx, f.reviewDeps, review.CommentParams{
 +		PRIssueID: pr.IssueID, AuthorUserID: f.reviewerID,
 +		FilePath: "x.txt", Side: "right", OriginalCommitSHA: pr.HeadOid,
 +		OriginalLine: 1, OriginalPosition: 1, CurrentPosition: 1,
 +		Body: "first draft", Pending: true,
 +	}); err != nil {
 +		t.Fatalf("AddComment 1: %v", err)
 +	}
 +	if _, err := review.AddComment(ctx, f.reviewDeps, review.CommentParams{
 +		PRIssueID: pr.IssueID, AuthorUserID: f.reviewerID,
 +		FilePath: "x.txt", Side: "right", OriginalCommitSHA: pr.HeadOid,
 +		OriginalLine: 1, OriginalPosition: 1, CurrentPosition: 1,
 +		Body: "second draft", Pending: true,
 +	}); err != nil {
 +		t.Fatalf("AddComment 2: %v", err)
 +	}
++
 +	rv, err := review.Submit(ctx, f.reviewDeps, review.SubmitParams{
 +		PRIssueID: pr.IssueID, AuthorUserID: f.reviewerID,
 +		State: "comment", Body: "review body", PRAuthorUserID: f.authorID,
 +	})
 +	if err != nil {
 +		t.Fatalf("Submit: %v", err)
 +	}
++
 +	// All pending comments should now have review_id = rv.ID and pending=false.
 +	cs, _ := pullsdb.New().ListPRReviewComments(ctx, f.pool, pr.IssueID)
 +	for _, c := range cs {
 +		if c.Pending {
 +			t.Errorf("comment %d still pending after submit", c.ID)
 +		}
 +		if !c.ReviewID.Valid || c.ReviewID.Int64 != rv.ID {
 +			t.Errorf("comment %d review_id=%v, want %d", c.ID, c.ReviewID, rv.ID)
 +		}
 +	}
 +}
++
 +func TestRequiredReviews_BlocksThenUnblocks(t *testing.T) {
 +	f := setup(t)
 +	ctx := context.Background()
 +	commitOnBranch(t, f.gitDir, "trunk", "init", "README.md", "hi\n")
 +	commitOnBranch(t, f.gitDir, "feature", "add", "x.txt", "x\n")
 +	pr := f.openPR(t, "trunk", "feature")
++
 +	// Add a protection rule on trunk requiring 1 approval.
 +	rq := reposdb.New()
 +	id, err := rq.UpsertBranchProtectionRule(ctx, f.pool, reposdb.UpsertBranchProtectionRuleParams{
 +		RepoID: f.repoID, Pattern: "trunk",
 +		PreventForcePush: true, PreventDeletion: true, RequirePrForPush: false,
 +		AllowedPusherUserIds: []int64{},
 +		CreatedByUserID:      pgtype.Int8{Valid: false},
 +	})
 +	if err != nil {
 +		t.Fatalf("UpsertBranchProtectionRule: %v", err)
 +	}
 +	if err := rq.UpdateBranchProtectionReviewSettings(ctx, f.pool, reposdb.UpdateBranchProtectionReviewSettingsParams{
 +		ID: id, RequiredReviewCount: 1,
 +	}); err != nil {
 +		t.Fatalf("UpdateBranchProtectionReviewSettings: %v", err)
 +	}
++
 +	// Re-tick mergeability — should be blocked now.
 +	if err := pulls.Mergeability(ctx, f.pullsDeps, f.gitDir, pr.IssueID); err != nil {
 +		t.Fatalf("Mergeability: %v", err)
 +	}
 +	got, _ := pullsdb.New().GetPullRequestByIssueID(ctx, f.pool, pr.IssueID)
 +	if got.MergeableState != pullsdb.PrMergeableStateBlocked {
 +		t.Errorf("after rule: state=%s, want blocked", got.MergeableState)
 +	}
++
 +	// Reviewer approves.
 +	if _, err := review.Submit(ctx, f.reviewDeps, review.SubmitParams{
 +		PRIssueID: pr.IssueID, AuthorUserID: f.reviewerID,
 +		State: "approve", PRAuthorUserID: f.authorID,
 +	}); err != nil {
 +		t.Fatalf("approve: %v", err)
 +	}
 +	if err := pulls.Mergeability(ctx, f.pullsDeps, f.gitDir, pr.IssueID); err != nil {
 +		t.Fatalf("Mergeability after approve: %v", err)
 +	}
 +	got, _ = pullsdb.New().GetPullRequestByIssueID(ctx, f.pool, pr.IssueID)
 +	if got.MergeableState != pullsdb.PrMergeableStateClean {
 +		t.Errorf("after approve: state=%s, want clean", got.MergeableState)
 +	}
++
 +	// Merge proceeds.
 +	if err := pulls.Merge(ctx, f.pullsDeps, pulls.MergeParams{
 +		PRID: pr.IssueID, ActorUserID: f.authorID, GitDir: f.gitDir, Method: "merge",
 +	}); err != nil {
 +		t.Fatalf("Merge: %v", err)
 +	}
 +}
++
 +func TestRequestChanges_BlocksMerge(t *testing.T) {
 +	f := setup(t)
 +	ctx := context.Background()
 +	commitOnBranch(t, f.gitDir, "trunk", "init", "README.md", "hi\n")
 +	commitOnBranch(t, f.gitDir, "feature", "add", "x.txt", "x\n")
 +	pr := f.openPR(t, "trunk", "feature")
++
 +	if _, err := review.Submit(ctx, f.reviewDeps, review.SubmitParams{
 +		PRIssueID: pr.IssueID, AuthorUserID: f.reviewerID,
 +		State: "request_changes", PRAuthorUserID: f.authorID,
 +	}); err != nil {
 +		t.Fatalf("submit request_changes: %v", err)
 +	}
 +	if err := pulls.Mergeability(ctx, f.pullsDeps, f.gitDir, pr.IssueID); err != nil {
 +		t.Fatalf("Mergeability: %v", err)
 +	}
 +	got, _ := pullsdb.New().GetPullRequestByIssueID(ctx, f.pool, pr.IssueID)
 +	if got.MergeableState != pullsdb.PrMergeableStateBlocked {
 +		t.Errorf("after request_changes: state=%s, want blocked", got.MergeableState)
 +	}
 +	// Merge should refuse.
 +	if err := pulls.Merge(ctx, f.pullsDeps, pulls.MergeParams{
 +		PRID: pr.IssueID, ActorUserID: f.authorID, GitDir: f.gitDir, Method: "merge",
 +	}); err == nil {
 +		t.Errorf("Merge should have been blocked, got nil")
 +	}
 +}
++
 +func TestTwoApprovers_UnblockMerge(t *testing.T) {
 +	f := setup(t)
 +	ctx := context.Background()
 +	commitOnBranch(t, f.gitDir, "trunk", "init", "README.md", "hi\n")
 +	commitOnBranch(t, f.gitDir, "feature", "add", "x.txt", "x\n")
 +	pr := f.openPR(t, "trunk", "feature")
++
 +	rq := reposdb.New()
 +	id, _ := rq.UpsertBranchProtectionRule(ctx, f.pool, reposdb.UpsertBranchProtectionRuleParams{
 +		RepoID: f.repoID, Pattern: "trunk",
 +		PreventForcePush: true, PreventDeletion: true, RequirePrForPush: false,
 +		AllowedPusherUserIds: []int64{}, CreatedByUserID: pgtype.Int8{Valid: false},
 +	})
 +	_ = rq.UpdateBranchProtectionReviewSettings(ctx, f.pool, reposdb.UpdateBranchProtectionReviewSettingsParams{
 +		ID: id, RequiredReviewCount: 2,
 +	})
 +	if err := pulls.Mergeability(ctx, f.pullsDeps, f.gitDir, pr.IssueID); err != nil {
 +		t.Fatalf("Mergeability: %v", err)
 +	}
 +	for _, uid := range []int64{f.reviewerID, f.otherID} {
 +		if _, err := review.Submit(ctx, f.reviewDeps, review.SubmitParams{
 +			PRIssueID: pr.IssueID, AuthorUserID: uid,
 +			State: "approve", PRAuthorUserID: f.authorID,
 +		}); err != nil {
 +			t.Fatalf("approve: %v", err)
 +		}
 +	}
 +	if err := pulls.Mergeability(ctx, f.pullsDeps, f.gitDir, pr.IssueID); err != nil {
 +		t.Fatalf("Mergeability: %v", err)
 +	}
 +	got, _ := pullsdb.New().GetPullRequestByIssueID(ctx, f.pool, pr.IssueID)
 +	if got.MergeableState != pullsdb.PrMergeableStateClean {
 +		t.Errorf("two approvers: state=%s, want clean", got.MergeableState)
 +	}
 +}
++
 +func TestResolveAndReopen(t *testing.T) {
 +	f := setup(t)
 +	ctx := context.Background()
 +	commitOnBranch(t, f.gitDir, "trunk", "init", "README.md", "hi\n")
 +	commitOnBranch(t, f.gitDir, "feature", "add", "x.txt", "x\n")
 +	pr := f.openPR(t, "trunk", "feature")
++
 +	c, err := review.AddComment(ctx, f.reviewDeps, review.CommentParams{
 +		PRIssueID: pr.IssueID, AuthorUserID: f.reviewerID,
 +		FilePath: "x.txt", Side: "right", OriginalCommitSHA: pr.HeadOid,
 +		OriginalLine: 1, OriginalPosition: 1, CurrentPosition: 1,
 +		Body: "comment",
 +	})
 +	if err != nil {
 +		t.Fatalf("AddComment: %v", err)
 +	}
 +	if err := review.Resolve(ctx, f.reviewDeps, f.reviewerID, c.ID); err != nil {
 +		t.Fatalf("Resolve: %v", err)
 +	}
 +	if err := review.Resolve(ctx, f.reviewDeps, f.reviewerID, c.ID); err == nil {
 +		t.Errorf("double-resolve should error")
 +	}
 +	if err := review.Reopen(ctx, f.reviewDeps, c.ID); err != nil {
 +		t.Fatalf("Reopen: %v", err)
 +	}
 +	got, _ := pullsdb.New().GetPRReviewComment(ctx, f.pool, c.ID)
 +	if got.ResolvedAt.Valid {
 +		t.Errorf("after reopen: resolved_at still set")
 +	}
 +}
++
 +func TestDismiss_ClearsBlock(t *testing.T) {
 +	f := setup(t)
 +	ctx := context.Background()
 +	commitOnBranch(t, f.gitDir, "trunk", "init", "README.md", "hi\n")
 +	commitOnBranch(t, f.gitDir, "feature", "add", "x.txt", "x\n")
 +	pr := f.openPR(t, "trunk", "feature")
++
 +	rv, err := review.Submit(ctx, f.reviewDeps, review.SubmitParams{
 +		PRIssueID: pr.IssueID, AuthorUserID: f.reviewerID,
 +		State: "request_changes", PRAuthorUserID: f.authorID,
 +	})
 +	if err != nil {
 +		t.Fatalf("submit: %v", err)
 +	}
 +	_ = pulls.Mergeability(ctx, f.pullsDeps, f.gitDir, pr.IssueID)
 +	got, _ := pullsdb.New().GetPullRequestByIssueID(ctx, f.pool, pr.IssueID)
 +	if got.MergeableState != pullsdb.PrMergeableStateBlocked {
 +		t.Fatalf("pre-dismiss: state=%s, want blocked", got.MergeableState)
 +	}
 +	if err := review.Dismiss(ctx, f.reviewDeps, f.authorID, rv.ID, "stale"); err != nil {
 +		t.Fatalf("Dismiss: %v", err)
 +	}
 +	_ = pulls.Mergeability(ctx, f.pullsDeps, f.gitDir, pr.IssueID)
 +	got, _ = pullsdb.New().GetPullRequestByIssueID(ctx, f.pool, pr.IssueID)
 +	if got.MergeableState != pullsdb.PrMergeableStateClean {
 +		t.Errorf("post-dismiss: state=%s, want clean", got.MergeableState)
 +	}
 +}

internal/pulls/review/submit.goadded

 +// SPDX-License-Identifier: AGPL-3.0-or-later
++
 +package review
++
 +import (
 +	"context"
 +	"errors"
 +	"fmt"
 +	"strings"
++
 +	"github.com/jackc/pgx/v5"
 +	"github.com/jackc/pgx/v5/pgtype"
++
 +	issuesdb "github.com/tenseleyFlow/shithub/internal/issues/sqlc"
 +	pullsdb "github.com/tenseleyFlow/shithub/internal/pulls/sqlc"
 +	mdrender "github.com/tenseleyFlow/shithub/internal/repos/markdown"
 +)
++
 +// SubmitParams describes the submit-a-review action.
 +type SubmitParams struct {
 +	PRIssueID    int64
 +	AuthorUserID int64
 +	State        string // "comment" | "approve" | "request_changes"
 +	Body         string
 +	// PRAuthorUserID is the issues.author_user_id of the PR — used to
 +	// reject self-approval. Caller passes it in to keep this package
 +	// from cross-importing the pulls orchestrator.
 +	PRAuthorUserID int64
 +}
++
 +// Submit records the review row, attaches every pending draft comment
 +// the author has on this PR to the new review (one tx), and emits a
 +// `reviewed` timeline event with the state. Pending review-request
 +// rows owned by the author are satisfied when state is approve or
 +// request_changes.
 +func Submit(ctx context.Context, deps Deps, p SubmitParams) (pullsdb.PrReview, error) {
 +	if p.State != "comment" && p.State != "approve" && p.State != "request_changes" {
 +		return pullsdb.PrReview{}, ErrInvalidState
 +	}
 +	if p.State == "approve" && p.AuthorUserID != 0 && p.AuthorUserID == p.PRAuthorUserID {
 +		return pullsdb.PrReview{}, ErrAuthorCannotApprove
 +	}
 +	body := strings.TrimSpace(p.Body)
 +	if len(body) > 65535 {
 +		return pullsdb.PrReview{}, ErrBodyTooLong
 +	}
 +	html, _ := mdrender.RenderHTML([]byte(body))
++
 +	tx, err := deps.Pool.Begin(ctx)
 +	if err != nil {
 +		return pullsdb.PrReview{}, err
 +	}
 +	committed := false
 +	defer func() {
 +		if !committed {
 +			_ = tx.Rollback(ctx)
 +		}
 +	}()
++
 +	q := pullsdb.New()
 +	row, err := q.CreatePRReview(ctx, tx, pullsdb.CreatePRReviewParams{
 +		PrIssueID:      p.PRIssueID,
 +		AuthorUserID:   pgtype.Int8{Int64: p.AuthorUserID, Valid: p.AuthorUserID != 0},
 +		State:          pullsdb.PrReviewState(p.State),
 +		Body:           body,
 +		BodyHtmlCached: pgtype.Text{String: html, Valid: html != ""},
 +	})
 +	if err != nil {
 +		return pullsdb.PrReview{}, fmt.Errorf("insert review: %w", err)
 +	}
++
 +	// Attach every pending draft comment authored by this user on this
 +	// PR to the new review.
 +	if err := q.AttachPendingCommentsToReview(ctx, tx, pullsdb.AttachPendingCommentsToReviewParams{
 +		PrIssueID:    p.PRIssueID,
 +		AuthorUserID: pgtype.Int8{Int64: p.AuthorUserID, Valid: p.AuthorUserID != 0},
 +		ReviewID:     pgtype.Int8{Int64: row.ID, Valid: true},
 +	}); err != nil {
 +		return pullsdb.PrReview{}, fmt.Errorf("attach pending: %w", err)
 +	}
++
 +	// Satisfy any pending review request from the author (only on
 +	// approve / request_changes per spec).
 +	if p.State == "approve" || p.State == "request_changes" {
 +		if err := q.SatisfyPRReviewRequest(ctx, tx, pullsdb.SatisfyPRReviewRequestParams{
 +			PrIssueID:           p.PRIssueID,
 +			SatisfiedByReviewID: pgtype.Int8{Int64: row.ID, Valid: true},
 +			RequestedUserID:     pgtype.Int8{Int64: p.AuthorUserID, Valid: p.AuthorUserID != 0},
 +		}); err != nil {
 +			return pullsdb.PrReview{}, fmt.Errorf("satisfy request: %w", err)
 +		}
 +	}
++
 +	// `reviewed` timeline event.
 +	iq := issuesdb.New()
 +	if _, err := iq.InsertIssueEvent(ctx, tx, issuesdb.InsertIssueEventParams{
 +		IssueID:     p.PRIssueID,
 +		ActorUserID: pgtype.Int8{Int64: p.AuthorUserID, Valid: p.AuthorUserID != 0},
 +		Kind:        "reviewed",
 +		Meta:        []byte(fmt.Sprintf(`{"state":%q,"review_id":%d}`, p.State, row.ID)),
 +	}); err != nil {
 +		return pullsdb.PrReview{}, fmt.Errorf("emit event: %w", err)
 +	}
++
 +	if err := tx.Commit(ctx); err != nil {
 +		return pullsdb.PrReview{}, err
 +	}
 +	committed = true
 +	return row, nil
 +}
++
 +// Dismiss flips a review's dismissed_at + reason. Caller has already
 +// gated on policy (typically repo admin only).
 +func Dismiss(ctx context.Context, deps Deps, actorUserID, reviewID int64, reason string) error {
 +	q := pullsdb.New()
 +	if _, err := q.GetPRReviewByID(ctx, deps.Pool, reviewID); err != nil {
 +		if errors.Is(err, pgx.ErrNoRows) {
 +			return ErrReviewNotFound
 +		}
 +		return err
 +	}
 +	return q.DismissPRReview(ctx, deps.Pool, pullsdb.DismissPRReviewParams{
 +		ID:                 reviewID,
 +		DismissedByUserID:  pgtype.Int8{Int64: actorUserID, Valid: actorUserID != 0},
 +		DismissalReason:    strings.TrimSpace(reason),
 +	})
 +}