diff --git a/backend/internal/adapters/reviewer/claudecode/claudecode.go b/backend/internal/adapters/reviewer/claudecode/claudecode.go index 159d67a673..eac0f7a205 100644 --- a/backend/internal/adapters/reviewer/claudecode/claudecode.go +++ b/backend/internal/adapters/reviewer/claudecode/claudecode.go @@ -30,6 +30,7 @@ func (r *Reviewer) Harness() domain.ReviewerHarness { } var _ ports.Reviewer = (*Reviewer)(nil) +var _ ports.ReviewerCanceller = (*Reviewer)(nil) // reviewerAllowedTools is the read-only tool allowlist the reviewer launches // with. The reviewer runs headless (no human to approve prompts) but must stay @@ -105,3 +106,9 @@ func (r *Reviewer) PreLaunch(ctx context.Context, inv ports.ReviewInvocation) er func (r *Reviewer) ReviewMessage(_ context.Context, inv ports.ReviewInvocation) (string, error) { return inv.Prompt, nil } + +// ReviewCancel stops the active Claude Code reviewer turn while preserving the +// terminal pane for inspection. +func (r *Reviewer) ReviewCancel(context.Context) (ports.ReviewCancelSpec, error) { + return ports.ReviewCancelSpec{Mode: ports.ReviewCancelInterrupt, Interrupts: 2}, nil +} diff --git a/backend/internal/adapters/reviewer/codex/codex.go b/backend/internal/adapters/reviewer/codex/codex.go index a166b78c7d..0b9b3bec4a 100644 --- a/backend/internal/adapters/reviewer/codex/codex.go +++ b/backend/internal/adapters/reviewer/codex/codex.go @@ -28,6 +28,7 @@ func (r *Reviewer) Harness() domain.ReviewerHarness { } var _ ports.Reviewer = (*Reviewer)(nil) +var _ ports.ReviewerCanceller = (*Reviewer)(nil) // ReviewCommand launches the reviewer with an enforced read-only filesystem // sandbox. Auto approval lets the headless session request the narrowly needed @@ -65,6 +66,12 @@ func (r *Reviewer) ReviewMessage(_ context.Context, inv ports.ReviewInvocation) return inv.Prompt, nil } +// ReviewCancel stops the active Codex reviewer turn while preserving the +// terminal pane for inspection. +func (r *Reviewer) ReviewCancel(context.Context) (ports.ReviewCancelSpec, error) { + return ports.ReviewCancelSpec{Mode: ports.ReviewCancelInterrupt, Interrupts: 2}, nil +} + func insertBeforePrompt(argv []string, extra ...string) []string { for i, arg := range argv { if arg == "--" { diff --git a/backend/internal/adapters/reviewer/opencode/opencode.go b/backend/internal/adapters/reviewer/opencode/opencode.go index 2ffd3b5a31..6c0055bbc4 100644 --- a/backend/internal/adapters/reviewer/opencode/opencode.go +++ b/backend/internal/adapters/reviewer/opencode/opencode.go @@ -28,6 +28,7 @@ func (r *Reviewer) Harness() domain.ReviewerHarness { } var _ ports.Reviewer = (*Reviewer)(nil) +var _ ports.ReviewerCanceller = (*Reviewer)(nil) // ReviewCommand launches the reviewer with an inline permission policy that // permits inspection and the two reporting commands while denying edits and @@ -54,3 +55,9 @@ func (r *Reviewer) ReviewCommand(ctx context.Context, inv ports.ReviewInvocation func (r *Reviewer) ReviewMessage(_ context.Context, inv ports.ReviewInvocation) (string, error) { return inv.Prompt, nil } + +// ReviewCancel stops the active OpenCode reviewer turn while preserving the +// terminal pane for inspection. +func (r *Reviewer) ReviewCancel(context.Context) (ports.ReviewCancelSpec, error) { + return ports.ReviewCancelSpec{Mode: ports.ReviewCancelInterrupt, Interrupts: 2}, nil +} diff --git a/backend/internal/adapters/reviewer/registry_test.go b/backend/internal/adapters/reviewer/registry_test.go index fba7020ffd..ce8ccc012e 100644 --- a/backend/internal/adapters/reviewer/registry_test.go +++ b/backend/internal/adapters/reviewer/registry_test.go @@ -1,9 +1,11 @@ package reviewer import ( + "context" "testing" "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" ) // TestRegistryMatchesDomainVocabulary enforces that the shipped reviewer @@ -19,6 +21,16 @@ func TestRegistryMatchesDomainVocabulary(t *testing.T) { if registered[h] { t.Errorf("reviewer harness %q registered twice", h) } + canceller, ok := a.(ports.ReviewerCanceller) + if !ok { + t.Errorf("reviewer harness %q does not implement cancellation", h) + } else if spec, err := canceller.ReviewCancel(context.Background()); err != nil { + t.Errorf("reviewer harness %q cancel spec: %v", h, err) + } else if spec.Mode != ports.ReviewCancelInterrupt { + t.Errorf("reviewer harness %q cancel mode = %q, want %q", h, spec.Mode, ports.ReviewCancelInterrupt) + } else if spec.Interrupts != 2 { + t.Errorf("reviewer harness %q cancel interrupts = %d, want 2", h, spec.Interrupts) + } registered[h] = true } for _, h := range domain.AllReviewerHarnesses { diff --git a/backend/internal/adapters/runtime/conpty/client.go b/backend/internal/adapters/runtime/conpty/client.go index f4d21a3b6c..9d3ba390d3 100644 --- a/backend/internal/adapters/runtime/conpty/client.go +++ b/backend/internal/adapters/runtime/conpty/client.go @@ -75,6 +75,20 @@ func clientSendMessage(addr, message string) error { return err } +func clientSendInput(addr, input string) error { + conn, err := dialHost(addr, dialTimeout) + if err != nil { + return err + } + defer func() { _ = conn.Close() }() + frame, err := EncodeMessage(MsgTerminalInput, []byte(input)) + if err != nil { + return err + } + _, err = conn.Write(frame) + return err +} + // clientGetOutput sends MsgGetOutputReq and reads frames until MsgGetOutputRes. // Returns "" on timeout or connection failure (no error), matching the TS. // lines <= 0 is handled by the caller (runtime.go rejects it before calling). diff --git a/backend/internal/adapters/runtime/conpty/runtime.go b/backend/internal/adapters/runtime/conpty/runtime.go index ddde0ad9ff..b4fa1adc63 100644 --- a/backend/internal/adapters/runtime/conpty/runtime.go +++ b/backend/internal/adapters/runtime/conpty/runtime.go @@ -171,6 +171,15 @@ func (r *Runtime) SendMessage(ctx context.Context, handle ports.RuntimeHandle, m return clientSendMessage(sess.addr, message) } +// Interrupt sends Ctrl-C to the PTY without tearing down the terminal host. +func (r *Runtime) Interrupt(ctx context.Context, handle ports.RuntimeHandle) error { + sess := r.resolve(handle.ID) + if sess == nil { + return fmt.Errorf("conpty: session %q not found", handle.ID) + } + return clientSendInput(sess.addr, "\x03") +} + // GetOutput returns the last lines lines from the pty-host ring buffer. func (r *Runtime) GetOutput(ctx context.Context, handle ports.RuntimeHandle, lines int) (string, error) { if lines <= 0 { diff --git a/backend/internal/adapters/runtime/runtimeselect/runtimeselect.go b/backend/internal/adapters/runtime/runtimeselect/runtimeselect.go index 3092590f01..2d658e9f5e 100644 --- a/backend/internal/adapters/runtime/runtimeselect/runtimeselect.go +++ b/backend/internal/adapters/runtime/runtimeselect/runtimeselect.go @@ -19,6 +19,7 @@ import ( type Runtime interface { ports.Runtime // Create, Destroy, IsAlive ports.Attacher + Interrupt(ctx context.Context, handle ports.RuntimeHandle) error SendMessage(ctx context.Context, handle ports.RuntimeHandle, message string) error GetOutput(ctx context.Context, handle ports.RuntimeHandle, lines int) (string, error) } diff --git a/backend/internal/adapters/runtime/tmux/commands.go b/backend/internal/adapters/runtime/tmux/commands.go index 1a55d3633a..2cdfb00624 100644 --- a/backend/internal/adapters/runtime/tmux/commands.go +++ b/backend/internal/adapters/runtime/tmux/commands.go @@ -64,6 +64,12 @@ func sendEnterArgs(id string) []string { return []string{"send-keys", "-t", id, "Enter"} } +// sendInterruptArgs builds args for `tmux send-keys -t C-c` to interrupt +// the foreground process without killing the terminal session. +func sendInterruptArgs(id string) []string { + return []string{"send-keys", "-t", id, "C-c"} +} + // capturePaneArgs builds args for `tmux capture-pane -t -p -S -`. // -p prints to stdout; -S - starts n lines back in history. func capturePaneArgs(id string, lines int) []string { diff --git a/backend/internal/adapters/runtime/tmux/tmux.go b/backend/internal/adapters/runtime/tmux/tmux.go index cb804b6463..6ccc7161c3 100644 --- a/backend/internal/adapters/runtime/tmux/tmux.go +++ b/backend/internal/adapters/runtime/tmux/tmux.go @@ -246,6 +246,19 @@ func (r *Runtime) SendMessage(ctx context.Context, handle ports.RuntimeHandle, m return nil } +// Interrupt sends Ctrl-C to the foreground process without destroying the tmux +// session, keeping the terminal available for inspection and reuse. +func (r *Runtime) Interrupt(ctx context.Context, handle ports.RuntimeHandle) error { + id, err := handleID(handle) + if err != nil { + return err + } + if _, err := r.run(ctx, sendInterruptArgs(id)...); err != nil { + return fmt.Errorf("tmux runtime: interrupt session %s: %w", id, err) + } + return nil +} + // GetOutput returns the last `lines` lines of the session pane's captured // output. func (r *Runtime) GetOutput(ctx context.Context, handle ports.RuntimeHandle, lines int) (string, error) { diff --git a/backend/internal/adapters/runtime/tmux/tmux_test.go b/backend/internal/adapters/runtime/tmux/tmux_test.go index 3569d7dedd..0625ab9421 100644 --- a/backend/internal/adapters/runtime/tmux/tmux_test.go +++ b/backend/internal/adapters/runtime/tmux/tmux_test.go @@ -95,6 +95,9 @@ func TestCommandBuilders(t *testing.T) { if got, want := sendEnterArgs("sess-1"), []string{"send-keys", "-t", "sess-1", "Enter"}; !reflect.DeepEqual(got, want) { t.Fatalf("sendEnterArgs = %#v, want %#v", got, want) } + if got, want := sendInterruptArgs("sess-1"), []string{"send-keys", "-t", "sess-1", "C-c"}; !reflect.DeepEqual(got, want) { + t.Fatalf("sendInterruptArgs = %#v, want %#v", got, want) + } if got, want := capturePaneArgs("sess-1", 10), []string{"capture-pane", "-t", "sess-1", "-p", "-S", "-10"}; !reflect.DeepEqual(got, want) { t.Fatalf("capturePaneArgs = %#v, want %#v", got, want) } @@ -562,6 +565,16 @@ func TestSendMessageEnterSurvivesCallerCancel(t *testing.T) { } } +func TestInterruptSendsCtrlC(t *testing.T) { + r, fr := newTestRuntime(0) + if err := r.Interrupt(context.Background(), ports.RuntimeHandle{ID: "sess-1"}); err != nil { + t.Fatalf("Interrupt: %v", err) + } + if got, want := fr.calls[0].args, sendInterruptArgs("sess-1"); !reflect.DeepEqual(got, want) { + t.Fatalf("interrupt args = %#v, want %#v", got, want) + } +} + // -- GetOutput tests -- func TestGetOutputValidatesLines(t *testing.T) { diff --git a/backend/internal/domain/review.go b/backend/internal/domain/review.go index 749cb6c247..7af2fc37ea 100644 --- a/backend/internal/domain/review.go +++ b/backend/internal/domain/review.go @@ -64,6 +64,7 @@ const ( ReviewRunComplete ReviewRunStatus = "complete" ReviewRunDelivered ReviewRunStatus = "delivered" ReviewRunFailed ReviewRunStatus = "failed" + ReviewRunCancelled ReviewRunStatus = "cancelled" ) // ReviewVerdict is the outcome a reviewer reports. The empty verdict marks a diff --git a/backend/internal/httpd/apispec/openapi.yaml b/backend/internal/httpd/apispec/openapi.yaml index 4d82e7b44e..92ee4bc95a 100644 --- a/backend/internal/httpd/apispec/openapi.yaml +++ b/backend/internal/httpd/apispec/openapi.yaml @@ -1445,6 +1445,45 @@ paths: summary: List a worker's code-review runs tags: - reviews + /api/v1/sessions/{sessionId}/reviews/cancel: + post: + operationId: cancelReview + parameters: + - description: Session identifier, e.g. project-1. + in: path + name: sessionId + required: true + schema: + description: Session identifier, e.g. project-1. + type: string + responses: + "200": + content: + application/json: + schema: + $ref: '#/components/schemas/CancelReviewResponse' + description: OK + "404": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Not Found + "422": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Unprocessable Entity + "501": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Not Implemented + summary: Cancel a running code review + tags: + - reviews /api/v1/sessions/{sessionId}/reviews/submit: post: operationId: submitReview @@ -1730,6 +1769,18 @@ components: - id - label type: object + CancelReviewResponse: + properties: + reviewerHandleId: + type: string + reviews: + items: + $ref: '#/components/schemas/PRReviewState' + type: array + required: + - reviewerHandleId + - reviews + type: object ClaimPRRequest: properties: allowTakeover: diff --git a/backend/internal/httpd/apispec/specgen/build.go b/backend/internal/httpd/apispec/specgen/build.go index 50c418d6fe..c9fca816f7 100644 --- a/backend/internal/httpd/apispec/specgen/build.go +++ b/backend/internal/httpd/apispec/specgen/build.go @@ -195,6 +195,7 @@ var schemaNames = map[string]string{ "ControllersListReviewsResponse": "ListReviewsResponse", "ControllersReviewRunResponse": "ReviewRunResponse", "ControllersTriggerReviewResponse": "TriggerReviewResponse", + "ControllersCancelReviewResponse": "CancelReviewResponse", "ControllersSubmitReviewItem": "SubmitReviewItem", "ControllersSubmitReviewInput": "SubmitReviewInput", // domain review entities @@ -482,6 +483,17 @@ func reviewOperations() []operation { {http.StatusNotImplemented, envelope.APIError{}}, }, }, + { + method: http.MethodPost, path: "/api/v1/sessions/{sessionId}/reviews/cancel", id: "cancelReview", tag: "reviews", + summary: "Cancel a running code review", + pathParams: []any{controllers.SessionIDParam{}}, + resps: []respUnit{ + {http.StatusOK, controllers.CancelReviewResponse{}}, + {http.StatusUnprocessableEntity, envelope.APIError{}}, + {http.StatusNotFound, envelope.APIError{}}, + {http.StatusNotImplemented, envelope.APIError{}}, + }, + }, { method: http.MethodPost, path: "/api/v1/sessions/{sessionId}/reviews/submit", id: "submitReview", tag: "reviews", summary: "Record a reviewer's result for a worker's PR", diff --git a/backend/internal/httpd/controllers/reviews.go b/backend/internal/httpd/controllers/reviews.go index 1790e3c1da..2c9966a257 100644 --- a/backend/internal/httpd/controllers/reviews.go +++ b/backend/internal/httpd/controllers/reviews.go @@ -37,6 +37,13 @@ type TriggerReviewResponse struct { Reviews []reviewcore.PRReviewState `json:"reviews"` } +// CancelReviewResponse is the body of cancel (200). reviews carries the +// PR-scoped review state after running passes have been stopped. +type CancelReviewResponse struct { + ReviewerHandleID string `json:"reviewerHandleId"` + Reviews []reviewcore.PRReviewState `json:"reviews"` +} + // SubmitReviewItem is one review result in a batched submit request. type SubmitReviewItem struct { RunID string `json:"runId" description:"Review run id being completed."` @@ -63,6 +70,7 @@ type ReviewsController struct { func (c *ReviewsController) Register(r chi.Router) { r.Get("/sessions/{sessionId}/reviews", c.list) r.Post("/sessions/{sessionId}/reviews/trigger", c.trigger) + r.Post("/sessions/{sessionId}/reviews/cancel", c.cancel) r.Post("/sessions/{sessionId}/reviews/submit", c.submit) } @@ -109,6 +117,26 @@ func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) { }) } +func (c *ReviewsController) cancel(w http.ResponseWriter, r *http.Request) { + if c.Svc == nil { + apispec.NotImplemented(w, r, "POST", "/api/v1/sessions/{sessionId}/reviews/cancel") + return + } + res, err := c.Svc.Cancel(r.Context(), sessionID(r)) + if err != nil { + writeReviewError(w, r, err) + return + } + reviews := res.Reviews + if reviews == nil { + reviews = []reviewcore.PRReviewState{} + } + envelope.WriteJSON(w, http.StatusOK, CancelReviewResponse{ + ReviewerHandleID: res.ReviewerHandleID, + Reviews: reviews, + }) +} + func (c *ReviewsController) submit(w http.ResponseWriter, r *http.Request) { if c.Svc == nil { apispec.NotImplemented(w, r, "POST", "/api/v1/sessions/{sessionId}/reviews/submit") diff --git a/backend/internal/httpd/controllers/reviews_test.go b/backend/internal/httpd/controllers/reviews_test.go index 123c45420f..d34d122a89 100644 --- a/backend/internal/httpd/controllers/reviews_test.go +++ b/backend/internal/httpd/controllers/reviews_test.go @@ -20,7 +20,9 @@ import ( type fakeReviewService struct { triggerErr error + cancelErr error trigger reviewcore.TriggerResult + cancel reviewcore.CancelResult list reviewcore.SessionReviews submitted []reviewsvc.SubmittedReview } @@ -39,6 +41,13 @@ func (f *fakeReviewService) Submit(context.Context, domain.SessionID, string, do return domain.ReviewRun{}, nil } +func (f *fakeReviewService) Cancel(context.Context, domain.SessionID) (reviewcore.CancelResult, error) { + if f.cancelErr != nil { + return reviewcore.CancelResult{}, f.cancelErr + } + return f.cancel, nil +} + func (f *fakeReviewService) SubmitMany(_ context.Context, _ domain.SessionID, reviews []reviewsvc.SubmittedReview) ([]domain.ReviewRun, error) { f.submitted = append([]reviewsvc.SubmittedReview(nil), reviews...) runs := make([]domain.ReviewRun, 0, len(reviews)) @@ -126,6 +135,26 @@ func TestReviewsTriggerIncludesBatchFields(t *testing.T) { } } +func TestReviewsCancelIncludesReviewStates(t *testing.T) { + srv := newReviewTestServer(t, &fakeReviewService{cancel: reviewcore.CancelResult{ + ReviewerHandleID: "review-mer-1", + Reviews: []reviewcore.PRReviewState{ + {PRURL: "https://github.com/o/r/pull/1", PRNumber: 1, TargetSHA: "sha1", Status: reviewcore.ReviewStateNeedsReview}, + }, + }}) + + body, status, headers := doRequest(t, srv, "POST", "/api/v1/sessions/mer-1/reviews/cancel", "") + assertJSON(t, headers) + if status != http.StatusOK { + t.Fatalf("status = %d body=%s", status, body) + } + for _, want := range []string{`"reviews"`, `"needs_review"`, `"reviewerHandleId":"review-mer-1"`} { + if !strings.Contains(string(body), want) { + t.Fatalf("body missing %s: %s", want, body) + } + } +} + func TestReviewsSubmitAcceptsBatchedReviews(t *testing.T) { svc := &fakeReviewService{} srv := newReviewTestServer(t, svc) diff --git a/backend/internal/ports/reviewer.go b/backend/internal/ports/reviewer.go index 21bb4ffe10..b7dff865e8 100644 --- a/backend/internal/ports/reviewer.go +++ b/backend/internal/ports/reviewer.go @@ -21,6 +21,28 @@ type Reviewer interface { ReviewMessage(ctx context.Context, inv ReviewInvocation) (string, error) } +// ReviewCancelMode names how AO should stop a running reviewer. +type ReviewCancelMode string + +const ( + // ReviewCancelInterrupt sends the terminal interrupt key sequence to the + // reviewer process while preserving the terminal pane. + ReviewCancelInterrupt ReviewCancelMode = "interrupt" +) + +// ReviewCancelSpec is the adapter-selected cancellation behavior for a running +// reviewer. +type ReviewCancelSpec struct { + Mode ReviewCancelMode + Interrupts int +} + +// ReviewerCanceller is implemented by reviewer adapters that explicitly define +// how their running CLI should be cancelled. +type ReviewerCanceller interface { + ReviewCancel(ctx context.Context) (ReviewCancelSpec, error) +} + // ReviewInvocation describes one review pass for a reviewer to act on. All ids // the reviewer needs are passed explicitly here (and embedded in the prompt / // message), never through environment variables. diff --git a/backend/internal/review/launcher.go b/backend/internal/review/launcher.go index d9639f0bd9..636576f872 100644 --- a/backend/internal/review/launcher.go +++ b/backend/internal/review/launcher.go @@ -4,12 +4,15 @@ import ( "context" "fmt" "os" + "time" "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/ports" sessionmanager "github.com/aoagents/agent-orchestrator/backend/internal/session_manager" ) +const cancelInterruptDelay = 150 * time.Millisecond + // Launcher spawns, re-notifies, and probes a reviewer over a worker's worktree. // It is the side of the engine that talks to the reviewer registry and runtime; // the engine owns the orchestration and persistence. @@ -21,6 +24,8 @@ type Launcher interface { Notify(ctx context.Context, handleID string, spec LaunchSpec) error // Alive reports whether a reviewer pane is still running. Alive(ctx context.Context, handleID string) (bool, error) + // Cancel interrupts a running reviewer pane while keeping the terminal alive. + Cancel(ctx context.Context, handleID string, harness domain.ReviewerHarness) error } // LaunchSpec is the engine's request to (re)launch a reviewer for one pass. @@ -40,6 +45,7 @@ type LaunchSpec struct { // satisfies it. type reviewerRuntime interface { Create(ctx context.Context, cfg ports.RuntimeConfig) (ports.RuntimeHandle, error) + Interrupt(ctx context.Context, handle ports.RuntimeHandle) error IsAlive(ctx context.Context, handle ports.RuntimeHandle) (bool, error) SendMessage(ctx context.Context, handle ports.RuntimeHandle, message string) error } @@ -150,3 +156,45 @@ func (l *agentLauncher) Alive(ctx context.Context, handleID string) (bool, error } return l.runtime.IsAlive(ctx, ports.RuntimeHandle{ID: handleID}) } + +func (l *agentLauncher) Cancel(ctx context.Context, handleID string, harness domain.ReviewerHarness) error { + if handleID == "" { + return nil + } + reviewer, ok := l.reviewers.Reviewer(harness) + if !ok { + return fmt.Errorf("no reviewer adapter for harness %q", harness) + } + canceller, ok := reviewer.(ports.ReviewerCanceller) + if !ok { + return fmt.Errorf("reviewer adapter %q does not support cancellation", harness) + } + spec, err := canceller.ReviewCancel(ctx) + if err != nil { + return fmt.Errorf("reviewer cancel: %w", err) + } + switch spec.Mode { + case ports.ReviewCancelInterrupt: + interrupts := spec.Interrupts + if interrupts <= 0 { + interrupts = 1 + } + for i := 0; i < interrupts; i++ { + if err := l.runtime.Interrupt(ctx, ports.RuntimeHandle{ID: handleID}); err != nil { + return err + } + if i < interrupts-1 { + timer := time.NewTimer(cancelInterruptDelay) + select { + case <-ctx.Done(): + timer.Stop() + return ctx.Err() + case <-timer.C: + } + } + } + return nil + default: + return fmt.Errorf("reviewer adapter %q returned unsupported cancel mode %q", harness, spec.Mode) + } +} diff --git a/backend/internal/review/launcher_test.go b/backend/internal/review/launcher_test.go index 0717841ec0..b7529df4b7 100644 --- a/backend/internal/review/launcher_test.go +++ b/backend/internal/review/launcher_test.go @@ -34,6 +34,26 @@ func (f *fakePreLaunchReviewer) PreLaunch(_ context.Context, inv ports.ReviewInv return nil } +type fakeCancellableReviewer struct { + fakeReviewer + cancelled bool + cancelErr error + mode ports.ReviewCancelMode + interrupts int +} + +func (f *fakeCancellableReviewer) ReviewCancel(context.Context) (ports.ReviewCancelSpec, error) { + f.cancelled = true + if f.cancelErr != nil { + return ports.ReviewCancelSpec{}, f.cancelErr + } + mode := f.mode + if mode == "" { + mode = ports.ReviewCancelInterrupt + } + return ports.ReviewCancelSpec{Mode: mode, Interrupts: f.interrupts}, nil +} + type fakeReviewerResolver struct { reviewer ports.Reviewer ok bool @@ -44,10 +64,12 @@ func (f fakeReviewerResolver) Reviewer(domain.ReviewerHarness) (ports.Reviewer, } type fakeRuntime struct { - createCfg ports.RuntimeConfig - sentMsg string - sentTo string - alive bool + createCfg ports.RuntimeConfig + sentMsg string + sentTo string + alive bool + interrupt string + interrupts int } func (f *fakeRuntime) Create(_ context.Context, cfg ports.RuntimeConfig) (ports.RuntimeHandle, error) { @@ -57,6 +79,11 @@ func (f *fakeRuntime) Create(_ context.Context, cfg ports.RuntimeConfig) (ports. func (f *fakeRuntime) IsAlive(_ context.Context, _ ports.RuntimeHandle) (bool, error) { return f.alive, nil } +func (f *fakeRuntime) Interrupt(_ context.Context, handle ports.RuntimeHandle) error { + f.interrupt = handle.ID + f.interrupts++ + return nil +} func (f *fakeRuntime) SendMessage(_ context.Context, handle ports.RuntimeHandle, msg string) error { f.sentTo = handle.ID f.sentMsg = msg @@ -136,6 +163,33 @@ func TestLauncherAlive(t *testing.T) { } } +func TestLauncherCancelUsesReviewerCancelMode(t *testing.T) { + reviewer := &fakeCancellableReviewer{interrupts: 2} + rt := &fakeRuntime{} + l := NewLauncher(fakeReviewerResolver{reviewer: reviewer, ok: true}, rt) + + if err := l.Cancel(context.Background(), "review-mer-1", domain.ReviewerClaudeCode); err != nil { + t.Fatalf("Cancel: %v", err) + } + if !reviewer.cancelled { + t.Fatal("expected reviewer cancel hook to run") + } + if rt.interrupt != "review-mer-1" { + t.Fatalf("interrupt handle = %q, want review-mer-1", rt.interrupt) + } + if rt.interrupts != 2 { + t.Fatalf("interrupt count = %d, want 2", rt.interrupts) + } +} + +func TestLauncherCancelRequiresReviewerSupport(t *testing.T) { + l := NewLauncher(fakeReviewerResolver{reviewer: &fakeReviewer{}, ok: true}, &fakeRuntime{}) + + if err := l.Cancel(context.Background(), "review-mer-1", domain.ReviewerClaudeCode); err == nil || !strings.Contains(err.Error(), "does not support cancellation") { + t.Fatalf("err = %v, want unsupported cancellation", err) + } +} + func TestLauncherSpawnNoAdapter(t *testing.T) { l := NewLauncher(fakeReviewerResolver{ok: false}, &fakeRuntime{}) if _, err := l.Spawn(context.Background(), launchSpec()); err == nil || !strings.Contains(err.Error(), "no reviewer adapter") { diff --git a/backend/internal/review/planner.go b/backend/internal/review/planner.go index a2cbdd6fa5..12c5a9ae2c 100644 --- a/backend/internal/review/planner.go +++ b/backend/internal/review/planner.go @@ -63,7 +63,7 @@ func Plan(prs []domain.PullRequest, runs []domain.ReviewRun) []PRReviewState { review.Status = ReviewStateUpToDate case run.Verdict == domain.VerdictChangesRequested: review.Status = ReviewStateChangesRequested - case run.Status == domain.ReviewRunFailed: + case run.Status == domain.ReviewRunFailed || run.Status == domain.ReviewRunCancelled: review.Status = ReviewStateNeedsReview default: review.Status = ReviewStateNeedsReview diff --git a/backend/internal/review/review.go b/backend/internal/review/review.go index a3fe57d668..d33137f7d8 100644 --- a/backend/internal/review/review.go +++ b/backend/internal/review/review.go @@ -34,11 +34,12 @@ type Store interface { GetReviewBySession(ctx stdctx.Context, id domain.SessionID) (domain.Review, bool, error) InsertReviewRun(ctx stdctx.Context, r domain.ReviewRun) error UpdateReviewRunResult(ctx stdctx.Context, id string, status domain.ReviewRunStatus, verdict domain.ReviewVerdict, body, githubReviewID string) (bool, error) - SupersedeReviewRun(ctx stdctx.Context, id, body string) (bool, error) SupersedeStaleRunningReviewRuns(ctx stdctx.Context, sessionID domain.SessionID, prURL, targetSHA, body string) (int64, error) + CancelRunningReviewRunsBySession(ctx stdctx.Context, sessionID domain.SessionID, body string) (int64, error) GetReviewRun(ctx stdctx.Context, id string) (domain.ReviewRun, bool, error) GetReviewRunBySessionPRAndSHA(ctx stdctx.Context, id domain.SessionID, prURL, targetSHA string) (domain.ReviewRun, bool, error) ListReviewRunsBySession(ctx stdctx.Context, id domain.SessionID) ([]domain.ReviewRun, error) + ListRunningReviewRunsBySession(ctx stdctx.Context, id domain.SessionID) ([]domain.ReviewRun, error) } // Sessions resolves the worker session under review. @@ -147,6 +148,13 @@ type SessionReviews struct { Reviews []PRReviewState } +// CancelResult is the review state after a reviewer pane cancellation. +type CancelResult struct { + ReviewerHandleID string + Reviews []PRReviewState + CancelledRuns []domain.ReviewRun +} + // Trigger starts reviews for every PR on the worker session that needs review. // It reuses running/up-to-date runs, retries failed/current changes-requested // heads, and uses one reviewer pane for every new run in the batch. @@ -211,20 +219,6 @@ func (e *Engine) Trigger(ctx stdctx.Context, workerID domain.SessionID) (Trigger if reviewState.Status != ReviewStateNeedsReview && reviewState.Status != ReviewStateChangesRequested { continue } - if reviewState.LatestRun != nil && reviewState.LatestRun.Status != domain.ReviewRunFailed && reviewState.LatestRun.Status != domain.ReviewRunRunning && reviewState.LatestRun.Verdict == domain.VerdictNone { - superseded, err := e.store.SupersedeReviewRun(ctx, reviewState.LatestRun.ID, "superseded by a new review trigger") - if err != nil { - return TriggerResult{}, err - } - if !superseded { - if latest, ok, err := e.store.GetReviewRun(ctx, reviewState.LatestRun.ID); err != nil { - return TriggerResult{}, err - } else if ok { - reviews = replaceReviewLatestRun(reviews, reviewState.PRURL, reviewState.TargetSHA, latest) - continue - } - } - } if _, err := e.store.SupersedeStaleRunningReviewRuns(ctx, workerID, reviewState.PRURL, reviewState.TargetSHA, "superseded by a review trigger for a newer commit"); err != nil { return TriggerResult{}, err } @@ -374,6 +368,54 @@ func (e *Engine) List(ctx stdctx.Context, workerID domain.SessionID) (SessionRev return SessionReviews{ReviewerHandleID: handle, Runs: runs, Reviews: Plan(prs, runs)}, nil } +// Cancel interrupts the live reviewer pane for a worker and marks running +// review runs as cancelled so they no longer block a fresh trigger. +func (e *Engine) Cancel(ctx stdctx.Context, workerID domain.SessionID) (CancelResult, error) { + if workerID == "" { + return CancelResult{}, fmt.Errorf("%w: worker session id is required", ErrInvalid) + } + review, ok, err := e.store.GetReviewBySession(ctx, workerID) + if err != nil { + return CancelResult{}, err + } + if !ok || review.ReviewerHandleID == "" { + return CancelResult{}, fmt.Errorf("%w: reviewer for worker session %q", ErrNotFound, workerID) + } + running, err := e.store.ListRunningReviewRunsBySession(ctx, workerID) + if err != nil { + return CancelResult{}, err + } + if err := e.launcher.Cancel(ctx, review.ReviewerHandleID, review.Harness); err != nil { + alive, aliveErr := e.launcher.Alive(ctx, review.ReviewerHandleID) + if aliveErr != nil { + return CancelResult{}, err + } + if alive { + return CancelResult{}, err + } + } + if _, err := e.store.CancelRunningReviewRunsBySession(ctx, workerID, "cancelled by user"); err != nil { + return CancelResult{}, err + } + cancelled := make([]domain.ReviewRun, 0, len(running)) + for _, run := range running { + run.Status = domain.ReviewRunCancelled + run.Verdict = domain.VerdictNone + run.Body = "cancelled by user" + run.GithubReviewID = "" + cancelled = append(cancelled, run) + } + prs, err := e.prs.ListPRsBySession(ctx, workerID) + if err != nil { + return CancelResult{}, err + } + runs, err := e.store.ListReviewRunsBySession(ctx, workerID) + if err != nil { + return CancelResult{}, err + } + return CancelResult{ReviewerHandleID: review.ReviewerHandleID, Reviews: Plan(prs, runs), CancelledRuns: cancelled}, nil +} + // reviewerHarness resolves which harness reviews the worker's PR: a configured // reviewer wins, otherwise worker's own harness is reused when it is a // supported reviewer, otherwise fallback to claude-code. diff --git a/backend/internal/review/review_test.go b/backend/internal/review/review_test.go index 455ae690de..afbaaa6ac2 100644 --- a/backend/internal/review/review_test.go +++ b/backend/internal/review/review_test.go @@ -16,8 +16,9 @@ import ( // --- fakes --- type fakeStore struct { - review *domain.Review - runs []domain.ReviewRun + review *domain.Review + runs []domain.ReviewRun + listAllReviewRunHits int // insertErr, when set, makes the next InsertReviewRun model a concurrent // writer that already recorded a run for this commit: it records that // winner (so a follow-up GetReviewRunBySessionAndSHA finds it) and returns @@ -49,7 +50,9 @@ func (f *fakeStore) InsertReviewRun(_ context.Context, r domain.ReviewRun) error existing.TargetSHA == r.TargetSHA && existing.TargetSHA != "" && existing.Status != domain.ReviewRunFailed && - existing.Verdict != domain.VerdictChangesRequested { + existing.Status != domain.ReviewRunCancelled && + (existing.Status == domain.ReviewRunRunning || + (existing.Verdict != domain.VerdictNone && existing.Verdict != domain.VerdictChangesRequested)) { return domain.ErrDuplicateReviewRun } } @@ -71,24 +74,22 @@ func (f *fakeStore) UpdateReviewRunResult(_ context.Context, id string, status d } return false, nil } -func (f *fakeStore) SupersedeReviewRun(_ context.Context, id, body string) (bool, error) { +func (f *fakeStore) SupersedeStaleRunningReviewRuns(_ context.Context, sessionID domain.SessionID, prURL, targetSHA, body string) (int64, error) { + var n int64 for i := range f.runs { - if f.runs[i].ID == id { - if f.runs[i].Verdict != domain.VerdictNone || f.runs[i].Status == domain.ReviewRunFailed { - return false, nil - } + if f.runs[i].SessionID == sessionID && f.runs[i].PRURL == prURL && f.runs[i].TargetSHA != targetSHA && f.runs[i].Status == domain.ReviewRunRunning && f.runs[i].Verdict == domain.VerdictNone { f.runs[i].Status = domain.ReviewRunFailed f.runs[i].Body = body - return true, nil + n++ } } - return false, nil + return n, nil } -func (f *fakeStore) SupersedeStaleRunningReviewRuns(_ context.Context, sessionID domain.SessionID, prURL, targetSHA, body string) (int64, error) { +func (f *fakeStore) CancelRunningReviewRunsBySession(_ context.Context, sessionID domain.SessionID, body string) (int64, error) { var n int64 for i := range f.runs { - if f.runs[i].SessionID == sessionID && f.runs[i].PRURL == prURL && f.runs[i].TargetSHA != targetSHA && f.runs[i].Status == domain.ReviewRunRunning && f.runs[i].Verdict == domain.VerdictNone { - f.runs[i].Status = domain.ReviewRunFailed + if f.runs[i].SessionID == sessionID && f.runs[i].Status == domain.ReviewRunRunning && f.runs[i].Verdict == domain.VerdictNone { + f.runs[i].Status = domain.ReviewRunCancelled f.runs[i].Body = body n++ } @@ -112,8 +113,18 @@ func (f *fakeStore) GetReviewRunBySessionPRAndSHA(_ context.Context, sessionID d return domain.ReviewRun{}, false, nil } func (f *fakeStore) ListReviewRunsBySession(_ context.Context, _ domain.SessionID) ([]domain.ReviewRun, error) { + f.listAllReviewRunHits++ return f.runs, nil } +func (f *fakeStore) ListRunningReviewRunsBySession(_ context.Context, sessionID domain.SessionID) ([]domain.ReviewRun, error) { + out := make([]domain.ReviewRun, 0) + for _, run := range f.runs { + if run.SessionID == sessionID && run.Status == domain.ReviewRunRunning && run.Verdict == domain.VerdictNone { + out = append(out, run) + } + } + return out, nil +} type fakeSessions struct { rec domain.SessionRecord @@ -137,17 +148,22 @@ func (f fakeProjects) GetProject(_ context.Context, id string) (domain.ProjectRe } type fakeLauncher struct { - handle string - alive bool - spawnErr error - notifyErr error - spawned bool - spawnCount int - notified bool - gotSpec LaunchSpec - gotHandle string - specs []LaunchSpec - handles []string + handle string + alive bool + spawnErr error + notifyErr error + spawned bool + spawnCount int + notified bool + cancelled bool + cancelErr error + aliveErr error + gotSpec LaunchSpec + gotHandle string + cancelledHandle string + cancelledHarness domain.ReviewerHarness + specs []LaunchSpec + handles []string } func (f *fakeLauncher) Spawn(_ context.Context, spec LaunchSpec) (string, error) { @@ -169,7 +185,13 @@ func (f *fakeLauncher) Notify(_ context.Context, handleID string, spec LaunchSpe return f.notifyErr } func (f *fakeLauncher) Alive(_ context.Context, _ string) (bool, error) { - return f.alive || f.spawned, nil + return f.alive || f.spawned, f.aliveErr +} +func (f *fakeLauncher) Cancel(_ context.Context, handleID string, harness domain.ReviewerHarness) error { + f.cancelled = true + f.cancelledHandle = handleID + f.cancelledHarness = harness + return f.cancelErr } func liveWorker() domain.SessionRecord { @@ -222,6 +244,93 @@ func TestTriggerSpawnsNewReviewerAndRecordsRunAfterLaunch(t *testing.T) { } } +func TestCancelInterruptsReviewerAndCancelsRunningRuns(t *testing.T) { + store := &fakeStore{ + review: &domain.Review{ID: "rev-1", SessionID: "mer-1", Harness: domain.ReviewerCodex, ReviewerHandleID: "review-mer-1"}, + runs: []domain.ReviewRun{ + {ID: "run-1", ReviewID: "rev-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Status: domain.ReviewRunRunning}, + {ID: "run-2", ReviewID: "rev-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/2", TargetSHA: "sha2", Status: domain.ReviewRunComplete, Verdict: domain.VerdictApproved}, + }, + } + launcher := &fakeLauncher{} + prs := fakePRs{prs: []domain.PullRequest{ + {URL: "https://github.com/o/r/pull/1", Number: 1, HeadSHA: "sha1"}, + {URL: "https://github.com/o/r/pull/2", Number: 2, HeadSHA: "sha2"}, + }} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prs, fakeProjects{}, launcher) + + res, err := eng.Cancel(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Cancel: %v", err) + } + if !launcher.cancelled || launcher.cancelledHandle != "review-mer-1" { + t.Fatalf("launcher cancel = %v handle=%q", launcher.cancelled, launcher.cancelledHandle) + } + if launcher.cancelledHarness != domain.ReviewerCodex { + t.Fatalf("cancel harness = %q, want codex", launcher.cancelledHarness) + } + if len(res.CancelledRuns) != 1 || res.CancelledRuns[0].ID != "run-1" { + t.Fatalf("cancelled runs = %+v", res.CancelledRuns) + } + if store.runs[0].Status != domain.ReviewRunCancelled || !strings.Contains(store.runs[0].Body, "cancelled") { + t.Fatalf("run not marked cancelled: %+v", store.runs[0]) + } + if store.runs[1].Status != domain.ReviewRunComplete { + t.Fatalf("non-running run was changed: %+v", store.runs[1]) + } + if store.listAllReviewRunHits != 1 { + t.Fatalf("full review run list calls = %d, want 1 for final plan refresh only", store.listAllReviewRunHits) + } + if res.Reviews[0].Status == ReviewStateRunning { + t.Fatalf("review state still running: %+v", res.Reviews[0]) + } +} + +func TestCancelMarksRunsCancelledWhenReviewerHandleIsGone(t *testing.T) { + store := &fakeStore{ + review: &domain.Review{ID: "rev-1", SessionID: "mer-1", Harness: domain.ReviewerCodex, ReviewerHandleID: "review-mer-1"}, + runs: []domain.ReviewRun{{ + ID: "run-1", ReviewID: "rev-1", SessionID: "mer-1", + PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Status: domain.ReviewRunRunning, + }}, + } + launcher := &fakeLauncher{cancelErr: errors.New("runtime: session not found")} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + res, err := eng.Cancel(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Cancel: %v", err) + } + if !launcher.cancelled { + t.Fatal("expected launcher cancellation to be attempted") + } + if got := store.runs[0]; got.Status != domain.ReviewRunCancelled { + t.Fatalf("run not marked cancelled after stale handle: %+v", got) + } + if len(res.CancelledRuns) != 1 || res.CancelledRuns[0].ID != "run-1" { + t.Fatalf("cancelled runs = %+v", res.CancelledRuns) + } +} + +func TestCancelKeepsRunsRunningWhenReviewerCancelFailsAndHandleIsAlive(t *testing.T) { + store := &fakeStore{ + review: &domain.Review{ID: "rev-1", SessionID: "mer-1", Harness: domain.ReviewerCodex, ReviewerHandleID: "review-mer-1"}, + runs: []domain.ReviewRun{{ + ID: "run-1", ReviewID: "rev-1", SessionID: "mer-1", + PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Status: domain.ReviewRunRunning, + }}, + } + launcher := &fakeLauncher{alive: true, cancelErr: errors.New("interrupt failed")} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + if _, err := eng.Cancel(context.Background(), "mer-1"); err == nil { + t.Fatal("Cancel err = nil, want interrupt failure") + } + if got := store.runs[0]; got.Status != domain.ReviewRunRunning { + t.Fatalf("run should remain running when reviewer is still alive: %+v", got) + } +} + func TestTriggerConcurrentSameWorkerSpawnsOnce(t *testing.T) { store := &fakeStore{} launcher := &fakeLauncher{handle: "review-mer-1"} @@ -331,26 +440,26 @@ func TestTriggerReusesRunningRowWithNoVerdict(t *testing.T) { } } -func TestTriggerSupersedesNonRunningRowWithNoVerdict(t *testing.T) { +func TestTriggerRetriesTerminalRowWithNoVerdict(t *testing.T) { store := &fakeStore{ review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, - runs: []domain.ReviewRun{{ID: "run-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Status: domain.ReviewRunComplete}}, + runs: []domain.ReviewRun{{ + ID: "run-empty-verdict", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", + Status: domain.ReviewRunComplete, Verdict: domain.VerdictNone, + }}, } - launcher := &fakeLauncher{alive: true, handle: "review-mer-1"} + launcher := &fakeLauncher{handle: "review-mer-2"} eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) res, err := eng.Trigger(context.Background(), "mer-1") if err != nil { t.Fatalf("Trigger: %v", err) } - if !res.Created { - t.Fatalf("expected a fresh pass when prior non-running row has no verdict: %+v", res) - } - if !launcher.notified || launcher.spawned { - t.Fatalf("expected notify on live reviewer pane, not spawn: %+v", launcher) + if !res.Created || res.Run.ID == "run-empty-verdict" { + t.Fatalf("expected retry to create a new run, got %+v", res) } - if stale := store.runs[0]; stale.ID != "run-1" || stale.Status != domain.ReviewRunFailed { - t.Fatalf("expected stale run-1 marked failed, got %+v", stale) + if len(store.runs) != 2 || !launcher.spawned { + t.Fatalf("expected new launch/run after terminal empty-verdict row: launched=%v runs=%+v", launcher.spawned, store.runs) } } @@ -456,6 +565,26 @@ func TestTriggerRetriesAfterFailedRunForSameCommit(t *testing.T) { } } +func TestTriggerRetriesAfterCancelledRunForSameCommit(t *testing.T) { + store := &fakeStore{ + review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, + runs: []domain.ReviewRun{{ID: "run-cancelled", ReviewID: "rev-1", SessionID: "mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", Status: domain.ReviewRunCancelled}}, + } + launcher := &fakeLauncher{handle: "review-mer-1"} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + res, err := eng.Trigger(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Trigger: %v", err) + } + if !res.Created || res.Run.ID == "run-cancelled" { + t.Fatalf("expected retry to create a new run, got %+v", res) + } + if len(store.runs) != 2 || !launcher.spawned { + t.Fatalf("expected new launch/run after cancelled pass: launched=%v runs=%+v", launcher.spawned, store.runs) + } +} + func TestTriggerCreatesRunsForMultipleEligiblePRsWithOneReviewer(t *testing.T) { store := &fakeStore{} launcher := &fakeLauncher{handle: "review-mer-1"} diff --git a/backend/internal/service/review/review.go b/backend/internal/service/review/review.go index 61f09e3b79..bf48cfc071 100644 --- a/backend/internal/service/review/review.go +++ b/backend/internal/service/review/review.go @@ -26,6 +26,7 @@ var ( // Manager is the reviews surface the HTTP controller depends on. type Manager interface { Trigger(ctx context.Context, workerID domain.SessionID) (reviewcore.TriggerResult, error) + Cancel(ctx context.Context, workerID domain.SessionID) (reviewcore.CancelResult, error) Submit(ctx context.Context, workerID domain.SessionID, runID string, verdict domain.ReviewVerdict, body, githubReviewID string) (domain.ReviewRun, error) SubmitMany(ctx context.Context, workerID domain.SessionID, reviews []SubmittedReview) ([]domain.ReviewRun, error) List(ctx context.Context, workerID domain.SessionID) (reviewcore.SessionReviews, error) @@ -87,6 +88,11 @@ func (s *Service) Trigger(ctx context.Context, workerID domain.SessionID) (revie return s.engine.Trigger(ctx, workerID) } +// Cancel stops the live reviewer pane and marks running review passes as failed. +func (s *Service) Cancel(ctx context.Context, workerID domain.SessionID) (reviewcore.CancelResult, error) { + return s.engine.Cancel(ctx, workerID) +} + // SubmittedReview is one review result supplied by the reviewer CLI. type SubmittedReview struct { RunID string diff --git a/backend/internal/storage/sqlite/gen/review.sql.go b/backend/internal/storage/sqlite/gen/review.sql.go index 16a939aee5..f0e7801734 100644 --- a/backend/internal/storage/sqlite/gen/review.sql.go +++ b/backend/internal/storage/sqlite/gen/review.sql.go @@ -13,6 +13,23 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/domain" ) +const cancelRunningReviewRunsBySession = `-- name: CancelRunningReviewRunsBySession :execrows +UPDATE review_run SET status = 'cancelled', body = ? WHERE session_id = ? AND status = 'running' AND verdict = '' +` + +type CancelRunningReviewRunsBySessionParams struct { + Body string + SessionID domain.SessionID +} + +func (q *Queries) CancelRunningReviewRunsBySession(ctx context.Context, arg CancelRunningReviewRunsBySessionParams) (int64, error) { + result, err := q.db.ExecContext(ctx, cancelRunningReviewRunsBySession, arg.Body, arg.SessionID) + if err != nil { + return 0, err + } + return result.RowsAffected() +} + const getReviewBySession = `-- name: GetReviewBySession :one SELECT id, session_id, project_id, harness, pr_url, reviewer_handle_id, created_at, updated_at FROM review WHERE session_id = ? @@ -219,6 +236,48 @@ func (q *Queries) ListReviewRunsBySession(ctx context.Context, sessionID domain. return items, nil } +const listRunningReviewRunsBySession = `-- name: ListRunningReviewRunsBySession :many +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id +FROM review_run WHERE session_id = ? AND status = 'running' AND verdict = '' ORDER BY created_at DESC +` + +func (q *Queries) ListRunningReviewRunsBySession(ctx context.Context, sessionID domain.SessionID) ([]ReviewRun, error) { + rows, err := q.db.QueryContext(ctx, listRunningReviewRunsBySession, sessionID) + if err != nil { + return nil, err + } + defer rows.Close() + items := []ReviewRun{} + for rows.Next() { + var i ReviewRun + if err := rows.Scan( + &i.ID, + &i.ReviewID, + &i.SessionID, + &i.Harness, + &i.PRURL, + &i.TargetSha, + &i.Status, + &i.Verdict, + &i.Body, + &i.CreatedAt, + &i.GithubReviewID, + &i.DeliveredAt, + &i.BatchID, + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const markReviewRunDelivered = `-- name: MarkReviewRunDelivered :execrows UPDATE review_run SET status = 'delivered', delivered_at = ? WHERE id = ? AND status = 'complete' AND delivered_at IS NULL ` @@ -236,23 +295,6 @@ func (q *Queries) MarkReviewRunDelivered(ctx context.Context, arg MarkReviewRunD return result.RowsAffected() } -const supersedeReviewRun = `-- name: SupersedeReviewRun :execrows -UPDATE review_run SET status = 'failed', body = ? WHERE id = ? AND verdict = '' AND status != 'failed' -` - -type SupersedeReviewRunParams struct { - Body string - ID string -} - -func (q *Queries) SupersedeReviewRun(ctx context.Context, arg SupersedeReviewRunParams) (int64, error) { - result, err := q.db.ExecContext(ctx, supersedeReviewRun, arg.Body, arg.ID) - if err != nil { - return 0, err - } - return result.RowsAffected() -} - const supersedeStaleRunningReviewRuns = `-- name: SupersedeStaleRunningReviewRuns :execrows UPDATE review_run SET status = 'failed', body = ? WHERE session_id = ? AND pr_url = ? AND target_sha != ? AND status = 'running' AND verdict = '' ` diff --git a/backend/internal/storage/sqlite/migrations/0023_review_run_retry_cancelled.sql b/backend/internal/storage/sqlite/migrations/0023_review_run_retry_cancelled.sql new file mode 100644 index 0000000000..e0894ab656 --- /dev/null +++ b/backend/internal/storage/sqlite/migrations/0023_review_run_retry_cancelled.sql @@ -0,0 +1,25 @@ +-- Cancelled review runs should be retryable like failed runs. + +-- +goose Up +-- +goose StatementBegin +DROP INDEX idx_review_run_session_pr_sha; +-- +goose StatementEnd + +-- +goose StatementBegin +CREATE UNIQUE INDEX idx_review_run_session_pr_sha + ON review_run (session_id, pr_url, target_sha) + WHERE target_sha != '' + AND status NOT IN ('failed', 'cancelled') + AND (status = 'running' OR verdict NOT IN ('', 'changes_requested')); +-- +goose StatementEnd + +-- +goose Down +-- +goose StatementBegin +DROP INDEX idx_review_run_session_pr_sha; +-- +goose StatementEnd + +-- +goose StatementBegin +CREATE UNIQUE INDEX idx_review_run_session_pr_sha + ON review_run (session_id, pr_url, target_sha) + WHERE target_sha != '' AND status != 'failed' AND verdict != 'changes_requested'; +-- +goose StatementEnd diff --git a/backend/internal/storage/sqlite/queries/review.sql b/backend/internal/storage/sqlite/queries/review.sql index 335f87cdf4..b72b249ce3 100644 --- a/backend/internal/storage/sqlite/queries/review.sql +++ b/backend/internal/storage/sqlite/queries/review.sql @@ -18,12 +18,12 @@ VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?); -- name: UpdateReviewRunResult :execrows UPDATE review_run SET status = ?, verdict = ?, body = ?, github_review_id = ? WHERE id = ? AND status = 'running'; --- name: SupersedeReviewRun :execrows -UPDATE review_run SET status = 'failed', body = ? WHERE id = ? AND verdict = '' AND status != 'failed'; - -- name: SupersedeStaleRunningReviewRuns :execrows UPDATE review_run SET status = 'failed', body = ? WHERE session_id = ? AND pr_url = ? AND target_sha != ? AND status = 'running' AND verdict = ''; +-- name: CancelRunningReviewRunsBySession :execrows +UPDATE review_run SET status = 'cancelled', body = ? WHERE session_id = ? AND status = 'running' AND verdict = ''; + -- name: MarkReviewRunDelivered :execrows UPDATE review_run SET status = 'delivered', delivered_at = ? WHERE id = ? AND status = 'complete' AND delivered_at IS NULL; @@ -39,6 +39,10 @@ FROM review_run WHERE session_id = ? AND pr_url = ? AND target_sha = ? ORDER BY SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id FROM review_run WHERE session_id = ? ORDER BY created_at DESC; +-- name: ListRunningReviewRunsBySession :many +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id +FROM review_run WHERE session_id = ? AND status = 'running' AND verdict = '' ORDER BY created_at DESC; + -- name: ListReviewRunsByBatch :many SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at, github_review_id, delivered_at, batch_id FROM review_run WHERE session_id = ? AND batch_id = ? ORDER BY created_at ASC, id ASC; diff --git a/backend/internal/storage/sqlite/store/review_store.go b/backend/internal/storage/sqlite/store/review_store.go index 9a62925ffa..107a41e3d3 100644 --- a/backend/internal/storage/sqlite/store/review_store.go +++ b/backend/internal/storage/sqlite/store/review_store.go @@ -84,21 +84,6 @@ func (s *Store) UpdateReviewRunResult(ctx context.Context, id string, status dom return n > 0, nil } -// SupersedeReviewRun marks an unverdicted non-failed pass failed so a new pass -// for the same commit can be recorded. -func (s *Store) SupersedeReviewRun(ctx context.Context, id, body string) (bool, error) { - s.writeMu.Lock() - defer s.writeMu.Unlock() - n, err := s.qw.SupersedeReviewRun(ctx, gen.SupersedeReviewRunParams{ - Body: body, - ID: id, - }) - if err != nil { - return false, err - } - return n > 0, nil -} - // SupersedeStaleRunningReviewRuns marks older running unverdicted passes for a // worker failed before starting a review for a newer commit. func (s *Store) SupersedeStaleRunningReviewRuns(ctx context.Context, sessionID domain.SessionID, prURL, targetSHA, body string) (int64, error) { @@ -112,6 +97,17 @@ func (s *Store) SupersedeStaleRunningReviewRuns(ctx context.Context, sessionID d }) } +// CancelRunningReviewRunsBySession marks all currently running review passes +// for a worker cancelled. +func (s *Store) CancelRunningReviewRunsBySession(ctx context.Context, sessionID domain.SessionID, body string) (int64, error) { + s.writeMu.Lock() + defer s.writeMu.Unlock() + return s.qw.CancelRunningReviewRunsBySession(ctx, gen.CancelRunningReviewRunsBySessionParams{ + Body: body, + SessionID: sessionID, + }) +} + // MarkReviewRunDelivered records that lifecycle delivered the worker nudge for // a completed AO-internal review pass. func (s *Store) MarkReviewRunDelivered(ctx context.Context, id string, deliveredAt time.Time) (bool, error) { @@ -167,6 +163,20 @@ func (s *Store) ListReviewRunsBySession(ctx context.Context, id domain.SessionID return out, nil } +// ListRunningReviewRunsBySession returns only currently running unverdicted +// review passes for a worker session, newest first. +func (s *Store) ListRunningReviewRunsBySession(ctx context.Context, id domain.SessionID) ([]domain.ReviewRun, error) { + rows, err := s.qr.ListRunningReviewRunsBySession(ctx, id) + if err != nil { + return nil, fmt.Errorf("list running review runs for session %s: %w", id, err) + } + out := make([]domain.ReviewRun, 0, len(rows)) + for _, row := range rows { + out = append(out, reviewRunFromRow(row)) + } + return out, nil +} + // ListReviewRunsByBatch returns all passes in one trigger-created batch, oldest first. func (s *Store) ListReviewRunsByBatch(ctx context.Context, id domain.SessionID, batchID string) ([]domain.ReviewRun, error) { rows, err := s.qr.ListReviewRunsByBatch(ctx, gen.ListReviewRunsByBatchParams{SessionID: id, BatchID: batchID}) diff --git a/backend/internal/storage/sqlite/store/review_store_test.go b/backend/internal/storage/sqlite/store/review_store_test.go index e8ea62bb38..1441bc9a40 100644 --- a/backend/internal/storage/sqlite/store/review_store_test.go +++ b/backend/internal/storage/sqlite/store/review_store_test.go @@ -103,6 +103,38 @@ func TestInsertReviewRunAllowsRerunAfterChangesRequested(t *testing.T) { } } +func TestInsertReviewRunAllowsRerunAfterTerminalEmptyVerdict(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + seedProject(t, s, "mer") + rec, err := s.CreateSession(ctx, sampleRecord("mer")) + if err != nil { + t.Fatalf("create session: %v", err) + } + now := time.Now().UTC().Truncate(time.Second) + if err := s.UpsertReview(ctx, domain.Review{ + ID: "rev-1", SessionID: rec.ID, ProjectID: rec.ProjectID, + Harness: domain.ReviewerClaudeCode, CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("upsert review: %v", err) + } + run := domain.ReviewRun{ + ID: "run-1", ReviewID: "rev-1", SessionID: rec.ID, Harness: domain.ReviewerClaudeCode, + PRURL: "https://example/pr/1", TargetSHA: "sha1", Status: domain.ReviewRunComplete, Verdict: domain.VerdictNone, CreatedAt: now, + } + if err := s.InsertReviewRun(ctx, run); err != nil { + t.Fatalf("first insert: %v", err) + } + + rerun := run + rerun.ID = "run-2" + rerun.Status = domain.ReviewRunRunning + rerun.CreatedAt = now.Add(time.Second) + if err := s.InsertReviewRun(ctx, rerun); err != nil { + t.Fatalf("rerun after terminal empty-verdict insert: %v", err) + } +} + func TestReviewUpsertReusesRowAndRunRoundTrip(t *testing.T) { s := newTestStore(t) ctx := context.Background() @@ -198,6 +230,60 @@ func TestReviewUpsertReusesRowAndRunRoundTrip(t *testing.T) { } } +func TestCancelRunningReviewRunsBySession(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + seedProject(t, s, "mer") + rec, err := s.CreateSession(ctx, sampleRecord("mer")) + if err != nil { + t.Fatalf("create session: %v", err) + } + now := time.Now().UTC().Truncate(time.Second) + if err := s.UpsertReview(ctx, domain.Review{ + ID: "rev-1", SessionID: rec.ID, ProjectID: rec.ProjectID, + Harness: domain.ReviewerCodex, CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("upsert review: %v", err) + } + for _, run := range []domain.ReviewRun{ + {ID: "run-1", ReviewID: "rev-1", SessionID: rec.ID, Harness: domain.ReviewerCodex, PRURL: "https://example/pr/1", TargetSHA: "sha1", Status: domain.ReviewRunRunning, CreatedAt: now}, + {ID: "run-2", ReviewID: "rev-1", SessionID: rec.ID, Harness: domain.ReviewerCodex, PRURL: "https://example/pr/2", TargetSHA: "sha2", Status: domain.ReviewRunRunning, CreatedAt: now.Add(time.Second)}, + {ID: "run-3", ReviewID: "rev-1", SessionID: rec.ID, Harness: domain.ReviewerCodex, PRURL: "https://example/pr/3", TargetSHA: "sha3", Status: domain.ReviewRunComplete, Verdict: domain.VerdictApproved, CreatedAt: now.Add(2 * time.Second)}, + } { + if err := s.InsertReviewRun(ctx, run); err != nil { + t.Fatalf("insert %s: %v", run.ID, err) + } + } + running, err := s.ListRunningReviewRunsBySession(ctx, rec.ID) + if err != nil { + t.Fatalf("list running: %v", err) + } + if len(running) != 2 || running[0].ID != "run-2" || running[1].ID != "run-1" { + t.Fatalf("running = %+v", running) + } + n, err := s.CancelRunningReviewRunsBySession(ctx, rec.ID, "cancelled by user") + if err != nil { + t.Fatalf("cancel running: %v", err) + } + if n != 2 { + t.Fatalf("cancelled rows = %d, want 2", n) + } + runs, err := s.ListReviewRunsBySession(ctx, rec.ID) + if err != nil { + t.Fatalf("list runs: %v", err) + } + byID := map[string]domain.ReviewRun{} + for _, run := range runs { + byID[run.ID] = run + } + if byID["run-1"].Status != domain.ReviewRunCancelled || byID["run-2"].Status != domain.ReviewRunCancelled { + t.Fatalf("running runs not cancelled: %+v", byID) + } + if byID["run-3"].Status != domain.ReviewRunComplete { + t.Fatalf("complete run changed: %+v", byID["run-3"]) + } +} + func TestReviewGettersMissing(t *testing.T) { s := newTestStore(t) ctx := context.Background() diff --git a/frontend/src/api/schema.ts b/frontend/src/api/schema.ts index c3f875897e..8f7a057888 100644 --- a/frontend/src/api/schema.ts +++ b/frontend/src/api/schema.ts @@ -539,6 +539,23 @@ export interface paths { patch?: never; trace?: never; }; + "/api/v1/sessions/{sessionId}/reviews/cancel": { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + get?: never; + put?: never; + /** Cancel a running code review */ + post: operations["cancelReview"]; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; "/api/v1/sessions/{sessionId}/reviews/submit": { parameters: { query?: never; @@ -657,6 +674,10 @@ export interface components { id: string; label: string; }; + CancelReviewResponse: { + reviewerHandleId: string; + reviews: components["schemas"]["PRReviewState"][]; + }; ClaimPRRequest: { allowTakeover?: null | boolean; pr: string; @@ -2987,6 +3008,56 @@ export interface operations { }; }; }; + cancelReview: { + parameters: { + query?: never; + header?: never; + path: { + /** @description Session identifier, e.g. project-1. */ + sessionId: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description OK */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["CancelReviewResponse"]; + }; + }; + /** @description Not Found */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + /** @description Unprocessable Entity */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + /** @description Not Implemented */ + 501: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + }; + }; submitReview: { parameters: { query?: never; diff --git a/frontend/src/renderer/components/SessionInspector.test.tsx b/frontend/src/renderer/components/SessionInspector.test.tsx index 86709d15f6..8aaae9bd71 100644 --- a/frontend/src/renderer/components/SessionInspector.test.tsx +++ b/frontend/src/renderer/components/SessionInspector.test.tsx @@ -424,19 +424,22 @@ describe("SessionInspector reviews tab", () => { expect(await screen.findByText("claude-code")).toBeInTheDocument(); }); - it("shows eligible and up-to-date PR review rows", async () => { + it("shows eligible and up-to-date open PR review rows", async () => { mockCommonGets([approvedReview], "reviewer-pane", [ reviewState(3, "needs_review", "abc123"), reviewState(4, "up_to_date", "def456"), + reviewState(5, "ineligible", "ghi789"), ]); - renderWithQuery(); + renderWithQuery(); await openReviewsTab(); + expect(screen.getByText("Pull requests")).toBeInTheDocument(); expect(await screen.findByText("Reviewable change 3")).toBeInTheDocument(); expect(screen.getByText("#3")).toBeInTheDocument(); expect(screen.getByText("Reviewable change 4")).toBeInTheDocument(); expect(screen.getByText("#4")).toBeInTheDocument(); + expect(screen.queryByText("Reviewable change 5")).not.toBeInTheDocument(); expect(screen.getAllByText("Not run")).not.toHaveLength(0); expect(screen.getAllByText("Approved")).not.toHaveLength(0); expect(screen.getByRole("button", { name: "Re-run review" })).toBeInTheDocument(); @@ -466,7 +469,7 @@ describe("SessionInspector reviews tab", () => { expect(onOpenReviewerTerminal).not.toHaveBeenCalled(); }); - it("shows one shared terminal action", async () => { + it("cancels the running review instead of allowing rerun", async () => { mockCommonGets([approvedReview], "reviewer-pane", [ reviewState(3, "running", "abc123"), reviewState(4, "up_to_date", "def456"), @@ -478,11 +481,29 @@ describe("SessionInspector reviews tab", () => { ); await openReviewsTab(); - await waitFor(() => expect(screen.getAllByText("Open terminal")).toHaveLength(1)); - expect(screen.getAllByRole("button", { name: /review/i })).toHaveLength(1); - await userEvent.click(screen.getByRole("button", { name: /open terminal/i })); + await waitFor(() => expect(screen.getByRole("button", { name: "Cancel review" })).toBeEnabled()); + expect(screen.queryByRole("button", { name: /re-run review/i })).not.toBeInTheDocument(); + await userEvent.click(screen.getByRole("button", { name: /cancel review/i })); - expect(onOpenReviewerTerminal).toHaveBeenCalledWith({ handleId: "reviewer-pane", harness: "codex" }); + await waitFor(() => { + expect(postMock).toHaveBeenCalledWith("/api/v1/sessions/{sessionId}/reviews/cancel", { + params: { path: { sessionId: "sess-1" } }, + }); + }); + expect(onOpenReviewerTerminal).not.toHaveBeenCalled(); + }); + + it("shows cancelled review runs without marking them failed", async () => { + mockCommonGets([], "reviewer-pane", [ + { ...reviewState(3, "needs_review", "abc123"), latestRun: { ...failedReview, status: "cancelled" } }, + ]); + + renderWithQuery(); + await openReviewsTab(); + + expect(await screen.findAllByText("Cancelled")).toHaveLength(2); + expect(screen.queryByText("Failed")).not.toBeInTheDocument(); + expect(screen.getByRole("button", { name: "Re-run review" })).toBeEnabled(); }); it("shows the reviewer identity and aggregate verdict", async () => { diff --git a/frontend/src/renderer/components/SessionInspector.tsx b/frontend/src/renderer/components/SessionInspector.tsx index 34a16862e8..7530ef8ed9 100644 --- a/frontend/src/renderer/components/SessionInspector.tsx +++ b/frontend/src/renderer/components/SessionInspector.tsx @@ -1,6 +1,6 @@ import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { useState, type ReactNode } from "react"; -import { ArrowUpRight, GitPullRequest, Play, Shield, Terminal } from "lucide-react"; +import { ArrowUpRight, GitPullRequest, Play, Shield, Terminal, X } from "lucide-react"; import type { components } from "../../api/schema"; import { apiClient, apiErrorMessage } from "../lib/api-client"; import { workspaceQueryKey } from "../hooks/useWorkspaceQuery"; @@ -501,6 +501,19 @@ function ReviewsView({ } }, }); + const cancelReview = useMutation({ + mutationFn: async () => { + const { error } = await apiClient.POST("/api/v1/sessions/{sessionId}/reviews/cancel", { + params: { path: { sessionId: session.id } }, + }); + if (error) throw new Error(apiErrorMessage(error, "Unable to cancel review")); + }, + onSuccess: () => { + setReviewNotice(null); + void queryClient.invalidateQueries({ queryKey: ["session-reviews", session.id] }); + void queryClient.invalidateQueries({ queryKey: workspaceQueryKey }); + }, + }); const reviewStates = reviewsQuery.data?.reviews ?? []; return ( @@ -508,10 +521,12 @@ function ReviewsView({
cancelReview.mutate()} onTrigger={() => triggerReview.mutate()} reviewerHandleId={reviewsQuery.data?.reviewerHandleId ?? ""} reviewStates={reviewStates} @@ -605,9 +620,11 @@ function ReviewPanel({ reviewerHandleId, isLoading, isTriggering, + isCancelling, error, notice, onTrigger, + onCancel, onOpenTerminal, }: { session: WorkspaceSession; @@ -616,9 +633,11 @@ function ReviewPanel({ reviewerHandleId: string; isLoading: boolean; isTriggering: boolean; + isCancelling: boolean; error: unknown; notice: string | null; onTrigger: () => void; + onCancel: () => void; onOpenTerminal?: OpenReviewerTerminal; }) { if (sortedPRs(session).length === 0) { @@ -628,16 +647,26 @@ function ReviewPanel({ return

Loading reviews...

; } - const latest = reviewStates.find((review) => review.latestRun)?.latestRun; + const openPRURLs = new Set( + sortedPRs(session) + .filter((pr) => pr.state === "open") + .map((pr) => pr.url), + ); + const openReviewStates = reviewStates.filter((reviewState) => openPRURLs.has(reviewState.prUrl)); + const latest = openReviewStates.find((review) => review.latestRun)?.latestRun; const harness = latest?.harness || config?.reviewers?.[0]?.harness || "claude-code"; const terminalEnabled = Boolean(reviewerHandleId && onOpenTerminal); - const aggregateVerdict = sessionReviewVerdict(reviewStates); - const runAction = reviewSessionRunAction(reviewStates, isTriggering); + const aggregateVerdict = sessionReviewVerdict(openReviewStates); + const reviewRunning = openReviewStates.some((reviewState) => reviewState.status === "running"); + const runAction = reviewSessionRunAction(openReviewStates, isTriggering); + const openReviewerTerminal = () => { + if (!terminalEnabled) return; + onOpenTerminal?.({ handleId: reviewerHandleId, harness }); + }; const runDisabled = isTriggering || - reviewStates.length === 0 || - reviewStates.some((reviewState) => reviewState.status === "running") || - reviewStates.every((reviewState) => reviewState.status === "ineligible"); + openReviewStates.length === 0 || + openReviewStates.every((reviewState) => reviewState.status === "ineligible"); return (
@@ -669,30 +698,32 @@ function ReviewPanel({
- {reviewStates.length === 0 ? ( -

No review state loaded yet.

+ {openReviewStates.length === 0 ? ( +

No open pull requests to review.

) : null} - {reviewStates.map((reviewState) => ( + {openReviewStates.map((reviewState) => ( ))}