diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index 00c1a185..4ac585b8 100644 --- a/internal/cmd/reviewcmd/reviewcmd.go +++ b/internal/cmd/reviewcmd/reviewcmd.go @@ -35,7 +35,9 @@ 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. +stale. A newer COMMENTED review from the posting identity supersedes that fast +path so thread-response activity can be followed by a fresh verdict. Use +--rerun to bypass these local gates and force a new live review. 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/gateio/gateio.go b/internal/gateio/gateio.go index be561b76..fe096181 100644 --- a/internal/gateio/gateio.go +++ b/internal/gateio/gateio.go @@ -917,7 +917,20 @@ func readGateHostStateWithReviews(ctx context.Context, provider outbox.LiveProvi func summarizePRFromHost(host gateHostState, req Request) gate.PRSummary { records := markerActionRecords(host, req.PostingIdentity) - return classifyMarkers(records, req.PR.Head.SHA, req.PR.Base.SHA) + summary := classifyMarkers(records, req.PR.Head.SHA, req.PR.Base.SHA) + if summary.State == gate.PRStateCompleteReview { + for _, latest := range latestVerdictReviewsByPostingIdentity(host.reviews, req.PostingIdentity) { + if latest.State != gitprovider.ReviewStateCommented { + continue + } + latestRecords := markerActionRecords(gateHostState{reviews: []gitprovider.Review{latest}}, req.PostingIdentity) + latestSummary := classifyMarkers(latestRecords, req.PR.Head.SHA, req.PR.Base.SHA) + if latestSummary.State != gate.PRStateCompleteReview { + return gate.PRSummary{State: gate.PRStateFresh} + } + } + } + return summary } func markerActionRecords(host gateHostState, posting gitprovider.Identity) []markerRecord { @@ -990,30 +1003,44 @@ func latestCodereviewMarkerAt(host gateHostState, posting gitprovider.Identity) } func activeApprovalByPostingIdentity(reviews []gitprovider.Review, posting gitprovider.Identity) bool { + latest := latestVerdictReviewsByPostingIdentity(reviews, posting) + if len(latest) == 0 { + return false + } + for _, review := range latest { + if review.State != gitprovider.ReviewStateApproved { + return false + } + } + return true +} + +// Review IDs are opaque, and tied timestamps do not establish chronology. +// Keep every latest verdict so both fast paths handle ambiguity conservatively. +func latestVerdictReviewsByPostingIdentity(reviews []gitprovider.Review, posting gitprovider.Identity) []gitprovider.Review { var ( - selected gitprovider.Review - found bool + latest []gitprovider.Review + when time.Time ) for _, review := range reviews { if !review.Author.Same(posting) { continue } switch review.State { - case gitprovider.ReviewStateApproved, gitprovider.ReviewStateChangesRequested: - case gitprovider.ReviewStateCommented, gitprovider.ReviewStateDismissed, gitprovider.ReviewStatePending: + case gitprovider.ReviewStateApproved, gitprovider.ReviewStateChangesRequested, gitprovider.ReviewStateCommented: + case gitprovider.ReviewStateDismissed, gitprovider.ReviewStatePending: continue default: continue } - if !found || review.SubmittedAt.After(selected.SubmittedAt) || - (review.SubmittedAt.Equal(selected.SubmittedAt) && - selected.State == gitprovider.ReviewStateApproved && - review.State == gitprovider.ReviewStateChangesRequested) { - selected = review - found = true + if len(latest) == 0 || review.SubmittedAt.After(when) { + latest = []gitprovider.Review{review} + when = review.SubmittedAt + } else if review.SubmittedAt.Equal(when) { + latest = append(latest, review) } } - return found && selected.State == gitprovider.ReviewStateApproved + return latest } 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 7374a064..3301976d 100644 --- a/internal/gateio/gateio_test.go +++ b/internal/gateio/gateio_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "fmt" "path/filepath" "reflect" "strings" @@ -277,6 +278,158 @@ func TestEvaluateActivePostingIdentityApprovalExitsBeforeOverrideReads(t *testin } } +func TestEvaluateNewerCommentedReviewDoesNotUseApprovalFastPath(t *testing.T) { + fixture := newFixture(t) + submit := mustRenderAction(t, marker.ActionMarker{ + RunID: "run-approved", + ActionID: "submit-1", + Kind: marker.ActionKindSubmitReview, + SHA: testHeadSHA, + BaseSHA: testBaseSHA, + }) + setReviews(t, fixture, []gitprovider.Review{ + { + ID: "review-approved", + Author: fixture.req.PostingIdentity, + Body: submit, + State: gitprovider.ReviewStateApproved, + SubmittedAt: testNow.Add(-time.Minute), + }, + { + ID: "review-commented", + Author: fixture.req.PostingIdentity, + State: gitprovider.ReviewStateCommented, + SubmittedAt: testNow, + }, + }) + + 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 after newer commented review", result) + } +} + +func TestEvaluateMarkedCommentedVerdictRemainsComplete(t *testing.T) { + fixture := newFixture(t) + submit := mustRenderAction(t, marker.ActionMarker{ + RunID: "run-commented", ActionID: "submit-1", Kind: marker.ActionKindSubmitReview, + SHA: testHeadSHA, BaseSHA: testBaseSHA, + }) + setReviews(t, fixture, []gitprovider.Review{{ + ID: "review-commented", Author: fixture.req.PostingIdentity, Body: submit, + State: gitprovider.ReviewStateCommented, SubmittedAt: testNow, + }}) + + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatal(err) + } + defer releaseResultLock(t, result) + if result.Status != StatusEarlyExit || result.Decision.Kind != gate.DecisionEarlyExit { + t.Fatalf("Evaluate = %#v, want completed comment verdict to remain idempotent", result) + } +} + +func TestEvaluateNewerMarkedCommentCompletesReplyRecovery(t *testing.T) { + fixture := newFixture(t) + submit := mustRenderAction(t, marker.ActionMarker{ + RunID: "run-recovered", ActionID: "submit-1", Kind: marker.ActionKindSubmitReview, + SHA: testHeadSHA, BaseSHA: testBaseSHA, + }) + setReviews(t, fixture, []gitprovider.Review{ + {ID: "99", Author: fixture.req.PostingIdentity, + State: gitprovider.ReviewStateCommented, SubmittedAt: testNow.Add(-time.Minute)}, + {ID: "100", Author: fixture.req.PostingIdentity, Body: submit, + State: gitprovider.ReviewStateCommented, SubmittedAt: testNow}, + }) + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatal(err) + } + defer releaseResultLock(t, result) + if result.Status != StatusEarlyExit || result.Decision.Kind != gate.DecisionEarlyExit { + t.Fatalf("Evaluate = %#v, want newer completed verdict to end reply recovery", result) + } +} + +func TestEvaluateTiedApprovalsRemainIdempotent(t *testing.T) { + fixture := newFixture(t) + setReviews(t, fixture, []gitprovider.Review{ + {ID: "99", Author: fixture.req.PostingIdentity, + State: gitprovider.ReviewStateApproved, SubmittedAt: testNow}, + {ID: "100", Author: fixture.req.PostingIdentity, + State: gitprovider.ReviewStateApproved, SubmittedAt: testNow}, + }) + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatal(err) + } + if result.Status != StatusEarlyExit || result.Decision.Kind != gate.DecisionEarlyExit { + t.Fatalf("Evaluate = %#v, want unanimous tied approvals to remain idempotent", result) + } +} + +func TestEvaluateTiedEmptyCommentPreventsMarkedCommentCompletion(t *testing.T) { + for _, reverse := range []bool{false, true} { + t.Run(fmt.Sprintf("reverse=%t", reverse), func(t *testing.T) { + fixture := newFixture(t) + submit := mustRenderAction(t, marker.ActionMarker{ + RunID: "run-commented", ActionID: "submit-1", Kind: marker.ActionKindSubmitReview, + SHA: testHeadSHA, BaseSHA: testBaseSHA, + }) + reviews := []gitprovider.Review{ + {ID: "99", Author: fixture.req.PostingIdentity, Body: submit, + State: gitprovider.ReviewStateCommented, SubmittedAt: testNow}, + {ID: "100", Author: fixture.req.PostingIdentity, + State: gitprovider.ReviewStateCommented, SubmittedAt: testNow}, + } + if reverse { + reviews[0], reviews[1] = reviews[1], reviews[0] + } + setReviews(t, fixture, reviews) + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatal(err) + } + defer releaseResultLock(t, result) + if result.Status != StatusContinue || result.Decision.Kind != gate.DecisionFresh { + t.Fatalf("Evaluate = %#v, want fresh review after an ambiguous empty comment", result) + } + }) + } +} + +func TestEvaluateCommentedVerdictOnOldBaseRequiresFreshReview(t *testing.T) { + fixture := newFixture(t) + current := mustRenderAction(t, marker.ActionMarker{ + RunID: "run-approved", ActionID: "submit-1", Kind: marker.ActionKindSubmitReview, + SHA: testHeadSHA, BaseSHA: testBaseSHA, + }) + stale := mustRenderAction(t, marker.ActionMarker{ + RunID: "run-commented", ActionID: "submit-2", Kind: marker.ActionKindSubmitReview, + SHA: testHeadSHA, BaseSHA: testOldBase, + }) + setReviews(t, fixture, []gitprovider.Review{ + {ID: "review-approved", Author: fixture.req.PostingIdentity, Body: current, + State: gitprovider.ReviewStateApproved, SubmittedAt: testNow.Add(-time.Minute)}, + {ID: "review-commented", Author: fixture.req.PostingIdentity, Body: stale, + State: gitprovider.ReviewStateCommented, SubmittedAt: testNow}, + }) + + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatal(err) + } + defer releaseResultLock(t, result) + if result.Status != StatusContinue || result.Decision.Kind != gate.DecisionFresh { + t.Fatalf("Evaluate = %#v, want fresh review after a stale-base comment verdict", result) + } +} + func TestEvaluateRetryPostsIgnoresActiveApprovalAndOverride(t *testing.T) { fixture := newFixture(t) run := fixture.allocateRun(t, "run-retry", testBaseSHA, ledger.PostModeLive) @@ -446,6 +599,32 @@ func TestEvaluateSameTimestampChangesRequestedPreventsActiveApprovalExit(t *test } } +func TestEvaluateTiedNumericReviewIDsDoNotEstablishApproval(t *testing.T) { + for _, state := range []gitprovider.ReviewState{gitprovider.ReviewStateChangesRequested, gitprovider.ReviewStateCommented} { + for _, reverse := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/reverse=%t", state, reverse), func(t *testing.T) { + fixture := newFixture(t) + reviews := []gitprovider.Review{ + {ID: "99", Author: fixture.req.PostingIdentity, State: gitprovider.ReviewStateApproved, SubmittedAt: testNow}, + {ID: "100", Author: fixture.req.PostingIdentity, State: state, SubmittedAt: testNow}, + } + if reverse { + reviews[0], reviews[1] = reviews[1], reviews[0] + } + setReviews(t, fixture, reviews) + result, err := Evaluate(context.Background(), fixture.opts(), fixture.req) + if err != nil { + t.Fatal(err) + } + defer releaseResultLock(t, result) + if result.Status != StatusContinue || result.Decision.Kind != gate.DecisionFresh { + t.Fatalf("Evaluate = %#v, want fresh review for ambiguous tied verdicts", result) + } + }) + } + } +} + func TestEvaluateAuthorOverrideAfterLatestMarkerApproves(t *testing.T) { fixture := newFixture(t) legacy := fixture.allocateRun(t, "legacy-override", testOldBase, ledger.PostModeLive)