From 0c85374d689586deec0de7d4913dbbfa36d61d41 Mon Sep 17 00:00:00 2001 From: Rian Stockbower Date: Fri, 31 Jul 2026 19:42:36 -0400 Subject: [PATCH] MON-6650: Re-review stale approvals by default --- docs/review-lifecycle.md | 5 +++++ internal/app/runtime_test.go | 1 + internal/cmd/reviewcmd/reviewcmd.go | 8 ++++---- internal/cmd/reviewcmd/reviewcmd_test.go | 7 ++++--- internal/gateio/gateio.go | 12 ++++++------ internal/gateio/gateio_test.go | 21 +++++++++++++++++++++ 6 files changed, 41 insertions(+), 13 deletions(-) diff --git a/docs/review-lifecycle.md b/docs/review-lifecycle.md index 5d8f665..9094811 100644 --- a/docs/review-lifecycle.md +++ b/docs/review-lifecycle.md @@ -21,6 +21,11 @@ stores its agent ID, broad or scoped assignment, runtime compatibility fields, and latest provider session ID. The explicit `--session` flag never changes the reviewer cohort's PR scope. +Before the pipeline starts, a normal live review checks approvals from the +configured posting identity. It exits early only when an approval is explicitly +bound to the current PR head. An approval bound to an older head, or with no +revision binding, is stale and the normal command reviews the current head. + ## First run 1. Resolve the PR, changed files, discussion, reviewer catalog, and runtime. diff --git a/internal/app/runtime_test.go b/internal/app/runtime_test.go index 00e9daa..2eec60e 100644 --- a/internal/app/runtime_test.go +++ b/internal/app/runtime_test.go @@ -878,6 +878,7 @@ func TestOpenLiveApprovedFastPathDoesNotInitializeAdapter(t *testing.T) { ID: "review-approved", Author: identity, State: gitprovider.ReviewStateApproved, + CommitSHA: pr.Head.SHA, SubmittedAt: time.Now().UTC(), }}); err != nil { t.Fatalf("SetReviews: %v", err) diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index 2ecfcd4..016eecf 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -32,10 +32,10 @@ import ( const reviewLong = `Run an automated pull-request review. -Live review checks local and host state before starting the reviewer loop. By -default, if the posting identity has already approved the PR, cr exits before -any LLM classifier or reviewer work, even if newer commits made that approval -stale. Use --rerun to bypass these local gates and force a new live review. +Live review checks local and host state before starting the reviewer loop. If +the posting identity has already approved the current PR head, cr exits before +any LLM classifier or reviewer work. An approval on an older head is stale, so +the normal command reviews the current head. Session reuse is independent of local review gates. Plain follow-up reviews and --rerun reuse the PR's original reviewer cohort and each reviewer's provider diff --git a/internal/cmd/reviewcmd/reviewcmd_test.go b/internal/cmd/reviewcmd/reviewcmd_test.go index 21f0f48..cb57f0e 100644 --- a/internal/cmd/reviewcmd/reviewcmd_test.go +++ b/internal/cmd/reviewcmd/reviewcmd_test.go @@ -364,7 +364,7 @@ func TestReviewExplicitProfileHostMismatch(t *testing.T) { } } -func TestReviewHelpDocumentsApprovalFastPaths(t *testing.T) { +func TestReviewHelpDocumentsCurrentHeadApprovalFastPath(t *testing.T) { cmd, out := newTestCommand(t, testConfig(), func(context.Context, app.OpenRequest) (app.Runtime, error) { t.Fatal("runtime factory should not be called for help") return app.Runtime{}, nil @@ -375,8 +375,9 @@ func TestReviewHelpDocumentsApprovalFastPaths(t *testing.T) { } text := out.String() for _, want := range []string{ - "already approved the PR", - "--rerun to bypass these local gates", + "already approved the current PR head", + "approval on an older head is stale", + "normal command reviews the current head", "--rerun reuse the PR's original reviewer cohort", "--session scopes only the orchestrator conversation", "--fresh-session", diff --git a/internal/gateio/gateio.go b/internal/gateio/gateio.go index be561b7..ab51fc8 100644 --- a/internal/gateio/gateio.go +++ b/internal/gateio/gateio.go @@ -163,7 +163,7 @@ func Evaluate(ctx context.Context, opts Options, req Request) (Result, error) { var precheckedReviews []gitprovider.Review reviewsLoaded := false if normalLiveFastPathEnabled(req.Flags) { - reviews, earlyExit, err := fetchReviewsUnlessApproved(ctx, opts.Provider, req.PRRef, precheckedReviews, reviewsLoaded, req.PostingIdentity) + reviews, earlyExit, err := fetchReviewsUnlessApproved(ctx, opts.Provider, req.PRRef, precheckedReviews, reviewsLoaded, req.PostingIdentity, req.PR.Head.SHA) if err != nil { return Result{}, err } @@ -194,7 +194,7 @@ func Evaluate(ctx context.Context, opts Options, req Request) (Result, error) { } var host *gateHostState if normalLiveFastPathEnabled(req.Flags) { - reviews, earlyExit, err := fetchReviewsUnlessApproved(ctx, opts.Provider, req.PRRef, precheckedReviews, reviewsLoaded, req.PostingIdentity) + reviews, earlyExit, err := fetchReviewsUnlessApproved(ctx, opts.Provider, req.PRRef, precheckedReviews, reviewsLoaded, req.PostingIdentity, req.PR.Head.SHA) if err != nil { return Result{}, err } @@ -871,7 +871,7 @@ func summarizeStaleCandidate(opts Options, req Request, run ledger.Run, summary }, nil, nil } -func fetchReviewsUnlessApproved(ctx context.Context, provider outbox.LiveProvider, ref gitprovider.PRRef, reviews []gitprovider.Review, loaded bool, posting gitprovider.Identity) ([]gitprovider.Review, *Result, error) { +func fetchReviewsUnlessApproved(ctx context.Context, provider outbox.LiveProvider, ref gitprovider.PRRef, reviews []gitprovider.Review, loaded bool, posting gitprovider.Identity, headSHA string) ([]gitprovider.Review, *Result, error) { if !loaded { var err error reviews, err = provider.ListReviews(ctx, ref) @@ -879,7 +879,7 @@ func fetchReviewsUnlessApproved(ctx context.Context, provider outbox.LiveProvide return nil, nil, err } } - if !activeApprovalByPostingIdentity(reviews, posting) { + if !activeApprovalByPostingIdentity(reviews, posting, headSHA) { return reviews, nil, nil } return nil, &Result{ @@ -989,7 +989,7 @@ func latestCodereviewMarkerAt(host gateHostState, posting gitprovider.Identity) return latest, found } -func activeApprovalByPostingIdentity(reviews []gitprovider.Review, posting gitprovider.Identity) bool { +func activeApprovalByPostingIdentity(reviews []gitprovider.Review, posting gitprovider.Identity, headSHA string) bool { var ( selected gitprovider.Review found bool @@ -1013,7 +1013,7 @@ func activeApprovalByPostingIdentity(reviews []gitprovider.Review, posting gitpr found = true } } - return found && selected.State == gitprovider.ReviewStateApproved + return found && selected.State == gitprovider.ReviewStateApproved && selected.CommitSHA == headSHA } func maybeExecuteApprovalOverride(ctx context.Context, opts Options, req Request, host *gateHostState) (Result, bool, error) { diff --git a/internal/gateio/gateio_test.go b/internal/gateio/gateio_test.go index 7374a06..bca4154 100644 --- a/internal/gateio/gateio_test.go +++ b/internal/gateio/gateio_test.go @@ -256,6 +256,7 @@ func TestEvaluateActivePostingIdentityApprovalExitsBeforeOverrideReads(t *testin ID: "review-approved", Author: fixture.req.PostingIdentity, State: gitprovider.ReviewStateApproved, + CommitSHA: testHeadSHA, SubmittedAt: testNow.Add(-time.Hour), }}) setIssueComments(t, fixture, []gitprovider.IssueComment{{ @@ -277,6 +278,26 @@ func TestEvaluateActivePostingIdentityApprovalExitsBeforeOverrideReads(t *testin } } +func TestEvaluateStalePostingIdentityApprovalReviewsCurrentHead(t *testing.T) { + fixture := newFixture(t) + setReviews(t, fixture, []gitprovider.Review{{ + ID: "review-approved", + Author: fixture.req.PostingIdentity, + State: gitprovider.ReviewStateApproved, + CommitSHA: testOldBase, + SubmittedAt: testNow.Add(-time.Hour), + }}) + + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatalf("Evaluate: %v", err) + } + defer releaseResultLock(t, result) + if result.Status != StatusContinue || result.Decision.Kind != gate.DecisionFresh { + t.Fatalf("Evaluate = %#v, want fresh review for stale approval", result) + } +} + func TestEvaluateRetryPostsIgnoresActiveApprovalAndOverride(t *testing.T) { fixture := newFixture(t) run := fixture.allocateRun(t, "run-retry", testBaseSHA, ledger.PostModeLive)