From 18d95d74724a03da5cfd1c58529d59a50bee5e92 Mon Sep 17 00:00:00 2001 From: piekstra Date: Tue, 15 Sep 2026 09:57:52 -0400 Subject: [PATCH 1/4] Fix review reruns after thread responses --- internal/cmd/reviewcmd/reviewcmd.go | 4 +++- internal/gateio/gateio.go | 24 ++++++++++++++------ internal/gateio/gateio_test.go | 35 +++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 8 deletions(-) diff --git a/internal/cmd/reviewcmd/reviewcmd.go b/internal/cmd/reviewcmd/reviewcmd.go index 4949407a..29cb8111 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..51266ad4 100644 --- a/internal/gateio/gateio.go +++ b/internal/gateio/gateio.go @@ -917,7 +917,14 @@ 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 { + latest, found := latestVerdictReviewByPostingIdentity(host.reviews, req.PostingIdentity) + if found && latest.State == gitprovider.ReviewStateCommented { + return gate.PRSummary{State: gate.PRStateFresh} + } + } + return summary } func markerActionRecords(host gateHostState, posting gitprovider.Identity) []markerRecord { @@ -990,6 +997,11 @@ func latestCodereviewMarkerAt(host gateHostState, posting gitprovider.Identity) } func activeApprovalByPostingIdentity(reviews []gitprovider.Review, posting gitprovider.Identity) bool { + selected, found := latestVerdictReviewByPostingIdentity(reviews, posting) + return found && selected.State == gitprovider.ReviewStateApproved +} + +func latestVerdictReviewByPostingIdentity(reviews []gitprovider.Review, posting gitprovider.Identity) (gitprovider.Review, bool) { var ( selected gitprovider.Review found bool @@ -999,21 +1011,19 @@ func activeApprovalByPostingIdentity(reviews []gitprovider.Review, posting gitpr 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) { + (review.SubmittedAt.Equal(selected.SubmittedAt) && string(review.ID) > string(selected.ID)) { selected = review found = true } } - return found && selected.State == gitprovider.ReviewStateApproved + return selected, found } 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..09bc4544 100644 --- a/internal/gateio/gateio_test.go +++ b/internal/gateio/gateio_test.go @@ -277,6 +277,41 @@ 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 TestEvaluateRetryPostsIgnoresActiveApprovalAndOverride(t *testing.T) { fixture := newFixture(t) run := fixture.allocateRun(t, "run-retry", testBaseSHA, ledger.PostModeLive) From 23fb501a0267e1641ff16104f4357d40b0cf8f6f Mon Sep 17 00:00:00 2001 From: piekstra Date: Thu, 1 Oct 2026 17:24:49 -0400 Subject: [PATCH 2/4] fix(review): preserve completed commented verdicts during reply recovery --- internal/gateio/gateio.go | 6 ++++- internal/gateio/gateio_test.go | 48 ++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/internal/gateio/gateio.go b/internal/gateio/gateio.go index 51266ad4..270eb075 100644 --- a/internal/gateio/gateio.go +++ b/internal/gateio/gateio.go @@ -921,7 +921,11 @@ func summarizePRFromHost(host gateHostState, req Request) gate.PRSummary { if summary.State == gate.PRStateCompleteReview { latest, found := latestVerdictReviewByPostingIdentity(host.reviews, req.PostingIdentity) if found && latest.State == gitprovider.ReviewStateCommented { - return gate.PRSummary{State: gate.PRStateFresh} + 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 diff --git a/internal/gateio/gateio_test.go b/internal/gateio/gateio_test.go index 09bc4544..e7751ce2 100644 --- a/internal/gateio/gateio_test.go +++ b/internal/gateio/gateio_test.go @@ -312,6 +312,54 @@ func TestEvaluateNewerCommentedReviewDoesNotUseApprovalFastPath(t *testing.T) { } } +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 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) From 99d5211e66190fe39b74bca5a441f28248d902ae Mon Sep 17 00:00:00 2001 From: piekstra Date: Thu, 1 Oct 2026 17:33:00 -0400 Subject: [PATCH 3/4] fix(review): handle tied verdict timestamps conservatively --- internal/gateio/gateio.go | 37 +++++++++++++++------- internal/gateio/gateio_test.go | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 12 deletions(-) diff --git a/internal/gateio/gateio.go b/internal/gateio/gateio.go index 270eb075..fe096181 100644 --- a/internal/gateio/gateio.go +++ b/internal/gateio/gateio.go @@ -919,8 +919,10 @@ func summarizePRFromHost(host gateHostState, req Request) gate.PRSummary { records := markerActionRecords(host, req.PostingIdentity) summary := classifyMarkers(records, req.PR.Head.SHA, req.PR.Base.SHA) if summary.State == gate.PRStateCompleteReview { - latest, found := latestVerdictReviewByPostingIdentity(host.reviews, req.PostingIdentity) - if found && latest.State == gitprovider.ReviewStateCommented { + 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 { @@ -1001,14 +1003,24 @@ func latestCodereviewMarkerAt(host gateHostState, posting gitprovider.Identity) } func activeApprovalByPostingIdentity(reviews []gitprovider.Review, posting gitprovider.Identity) bool { - selected, found := latestVerdictReviewByPostingIdentity(reviews, posting) - return found && selected.State == gitprovider.ReviewStateApproved + latest := latestVerdictReviewsByPostingIdentity(reviews, posting) + if len(latest) == 0 { + return false + } + for _, review := range latest { + if review.State != gitprovider.ReviewStateApproved { + return false + } + } + return true } -func latestVerdictReviewByPostingIdentity(reviews []gitprovider.Review, posting gitprovider.Identity) (gitprovider.Review, bool) { +// 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) { @@ -1021,13 +1033,14 @@ func latestVerdictReviewByPostingIdentity(reviews []gitprovider.Review, posting default: continue } - if !found || review.SubmittedAt.After(selected.SubmittedAt) || - (review.SubmittedAt.Equal(selected.SubmittedAt) && string(review.ID) > string(selected.ID)) { - 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 selected, found + 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 e7751ce2..ac719952 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" @@ -333,6 +334,36 @@ func TestEvaluateMarkedCommentedVerdictRemainsComplete(t *testing.T) { } } +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{ @@ -529,6 +560,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) From 5ac31f474bcebedd727895ebaba4b9dfc17c6bcb Mon Sep 17 00:00:00 2001 From: piekstra Date: Thu, 1 Oct 2026 17:43:09 -0400 Subject: [PATCH 4/4] test(review): cover completion after reply recovery --- internal/gateio/gateio_test.go | 39 ++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/internal/gateio/gateio_test.go b/internal/gateio/gateio_test.go index ac719952..3301976d 100644 --- a/internal/gateio/gateio_test.go +++ b/internal/gateio/gateio_test.go @@ -334,6 +334,45 @@ func TestEvaluateMarkedCommentedVerdictRemainsComplete(t *testing.T) { } } +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) {