diff --git a/backend/internal/adapters/scm/github/observer_provider.go b/backend/internal/adapters/scm/github/observer_provider.go index 6d5ae1ddc..fb9aebb10 100644 --- a/backend/internal/adapters/scm/github/observer_provider.go +++ b/backend/internal/adapters/scm/github/observer_provider.go @@ -33,6 +33,8 @@ const ( githubReviewCommentLimitPerThread = 5 // githubReviewThreadMaxPages bounds the explicit older-thread fallback. githubReviewThreadMaxPages = 2 + // githubReviewSummaryLimit bounds submitted decisive reviews used for summary links. + githubReviewSummaryLimit = 20 ) // ParseRepository normalizes a GitHub remote/origin URL into a provider-neutral @@ -156,12 +158,12 @@ func (p *Provider) FetchFailedCheckLogTail(ctx context.Context, repo ports.SCMRe // FetchReviewThreads fetches review threads separately from the fast PR/CI path. func (p *Provider) FetchReviewThreads(ctx context.Context, ref ports.SCMPRRef) (ports.SCMReviewObservation, error) { - latest, decision, pi, err := p.fetchReviewThreadPage(ctx, ref, "") + latest, reviews, decision, pi, err := p.fetchReviewThreadPage(ctx, ref, "", true) if err != nil { return ports.SCMReviewObservation{}, err } if !boolv(pi["hasPreviousPage"]) { - return ports.SCMReviewObservation{Decision: decision, Threads: latest}, nil + return ports.SCMReviewObservation{Decision: decision, Reviews: reviews, Threads: latest}, nil } out := latest startCursor := str(pi["startCursor"]) @@ -175,7 +177,7 @@ func (p *Provider) FetchReviewThreads(ctx context.Context, ref ports.SCMPRRef) ( p.logger.Warn("github scm: review thread page is partial but missing start cursor", "repo", repoFullName(ref.Repo), "pr", ref.Number) } else { - older, _, olderPI, err := p.fetchReviewThreadPage(ctx, ref, startCursor) + older, _, _, olderPI, err := p.fetchReviewThreadPage(ctx, ref, startCursor, false) if err != nil { return ports.SCMReviewObservation{}, err } @@ -190,7 +192,7 @@ func (p *Provider) FetchReviewThreads(ctx context.Context, ref ports.SCMPRRef) ( } } } - return ports.SCMReviewObservation{Decision: decision, Threads: out, Partial: true}, nil + return ports.SCMReviewObservation{Decision: decision, Reviews: reviews, Threads: out, Partial: true}, nil } type restListPull struct { @@ -518,38 +520,76 @@ func mergeabilityObservation(providerMergeable, providerMergeState, ci, review s return out } -func (p *Provider) fetchReviewThreadPage(ctx context.Context, ref ports.SCMPRRef, beforeCursor string) ([]ports.SCMReviewThreadObservation, string, map[string]any, error) { - query := buildReviewThreadsQuery(ref, beforeCursor) +func (p *Provider) fetchReviewThreadPage(ctx context.Context, ref ports.SCMPRRef, beforeCursor string, includeReviews bool) ([]ports.SCMReviewThreadObservation, []ports.SCMReviewSummaryObservation, string, map[string]any, error) { + query := buildReviewThreadsQuery(ref, beforeCursor, includeReviews) data, err := p.client.doGraphQL(ctx, query, nil) if err != nil { - return nil, "", nil, err + return nil, nil, "", nil, err } repoData, _ := data["repo"].(map[string]any) pr, _ := repoData["pullRequest"].(map[string]any) if pr == nil { - return nil, "", nil, fmt.Errorf("%w: pull request not found in review response", ErrNotFound) + return nil, nil, "", nil, fmt.Errorf("%w: pull request not found in review response", ErrNotFound) } decision := string(reviewDecisionFromGraphQL(pr)) + reviewSummaries, _ := pr["reviewSummaries"].(map[string]any) + reviews := make([]ports.SCMReviewSummaryObservation, 0, len(nodes(reviewSummaries["nodes"]))) + for _, review := range nodes(reviewSummaries["nodes"]) { + summary := scmReviewSummaryFromGraphQL(review) + if summary.ID == "" && summary.URL == "" { + continue + } + reviews = append(reviews, summary) + } threads, _ := pr["reviewThreads"].(map[string]any) out := make([]ports.SCMReviewThreadObservation, 0, len(nodes(threads["nodes"]))) for _, th := range nodes(threads["nodes"]) { out = append(out, scmThreadFromGraphQL(th)) } pi, _ := threads["pageInfo"].(map[string]any) - return out, decision, pi, nil + return out, reviews, decision, pi, nil } -func buildReviewThreadsQuery(ref ports.SCMPRRef, beforeCursor string) string { +func buildReviewThreadsQuery(ref ports.SCMPRRef, beforeCursor string, includeReviews bool) string { before := "null" if beforeCursor != "" { before = graphQLString(beforeCursor) } + reviewSelection := "" + if includeReviews { + reviewSelection = fmt.Sprintf(" reviewSummaries: reviews(last:%d, states:[APPROVED,CHANGES_REQUESTED]){ nodes{ id state url submittedAt author{ login __typename } } }", githubReviewSummaryLimit) + } return fmt.Sprintf(`query{ -repo: repository(owner:%s,name:%s){ pullRequest(number:%d){ reviewDecision reviewThreads(last:%d, before:%s){ nodes{ +repo: repository(owner:%s,name:%s){ pullRequest(number:%d){ reviewDecision%s reviewThreads(last:%d, before:%s){ nodes{ id isResolved path line comments(first:%d){ nodes{ id body url author{ login __typename } } } } pageInfo{ hasPreviousPage startCursor } } } } -}`, graphQLString(ref.Repo.Owner), graphQLString(ref.Repo.Name), ref.Number, githubReviewThreadPageSize, before, githubReviewCommentLimitPerThread) +}`, graphQLString(ref.Repo.Owner), graphQLString(ref.Repo.Name), ref.Number, reviewSelection, githubReviewThreadPageSize, before, githubReviewCommentLimitPerThread) +} + +func scmReviewSummaryFromGraphQL(review map[string]any) ports.SCMReviewSummaryObservation { + author, _ := review["author"].(map[string]any) + return ports.SCMReviewSummaryObservation{ + ID: str(review["id"]), + Author: str(author["login"]), + State: string(reviewStateFromGraphQL(review["state"])), + URL: str(review["url"]), + IsBot: isBotAuthor(author), + SubmittedAt: parseGitHubTime(str(review["submittedAt"])), + } +} + +func reviewStateFromGraphQL(state any) domain.ReviewDecision { + switch strings.ToUpper(strings.TrimSpace(str(state))) { + case "APPROVED": + return domain.ReviewApproved + case "CHANGES_REQUESTED": + return domain.ReviewChangesRequest + case "REVIEW_REQUIRED": + return domain.ReviewRequired + default: + return domain.ReviewNone + } } func scmThreadFromGraphQL(th map[string]any) ports.SCMReviewThreadObservation { diff --git a/backend/internal/adapters/scm/github/provider_test.go b/backend/internal/adapters/scm/github/provider_test.go index 405390d89..6763f5d54 100644 --- a/backend/internal/adapters/scm/github/provider_test.go +++ b/backend/internal/adapters/scm/github/provider_test.go @@ -1369,6 +1369,9 @@ func TestFetchReviewThreadsUsesLatestWindowWithoutFallbackWhenOldestResolved(t * if !strings.Contains(string(body), "reviewThreads(last:50, before:null)") { t.Fatalf("review query should fetch latest 50, body=%s", body) } + if !strings.Contains(string(body), "reviews(last:20, states:[APPROVED,CHANGES_REQUESTED])") { + t.Fatalf("review query should fetch decisive review summaries, body=%s", body) + } if !strings.Contains(string(body), "comments(first:5)") { t.Fatalf("review query should cap comments per thread, body=%s", body) } @@ -1376,8 +1379,17 @@ func TestFetchReviewThreadsUsesLatestWindowWithoutFallbackWhenOldestResolved(t * _ = json.NewEncoder(w).Encode(map[string]any{ "data": map[string]any{"repo": map[string]any{"pullRequest": map[string]any{ "reviewDecision": "CHANGES_REQUESTED", + "reviewSummaries": map[string]any{"nodes": []any{map[string]any{ + "id": "review-1", + "state": "CHANGES_REQUESTED", + "url": "https://github.com/o/r/pull/1#pullrequestreview-1", + "submittedAt": "2026-06-15T00:00:00Z", + "author": map[string]any{"login": "alice", "__typename": "User"}, + }}}, "reviewThreads": map[string]any{ - "nodes": []any{map[string]any{"id": "latest-resolved", "path": "main.go", "line": 1, "isResolved": true, "comments": map[string]any{"nodes": []any{}}}}, + "nodes": []any{map[string]any{"id": "latest-resolved", "path": "main.go", "line": 1, "isResolved": true, "comments": map[string]any{"nodes": []any{map[string]any{ + "id": "comment-1", "body": "fix", "url": "https://github.com/o/r/pull/1#discussion_r1", "author": map[string]any{"login": "alice", "__typename": "User"}, + }}}}}, "pageInfo": map[string]any{"hasPreviousPage": true, "startCursor": "latest-start"}, }, }}}, @@ -1397,6 +1409,12 @@ func TestFetchReviewThreadsUsesLatestWindowWithoutFallbackWhenOldestResolved(t * if len(review.Threads) != 1 || review.Threads[0].ID != "latest-resolved" { t.Fatalf("threads = %#v", review.Threads) } + if len(review.Reviews) != 1 || review.Reviews[0].Author != "alice" || review.Reviews[0].URL != "https://github.com/o/r/pull/1#pullrequestreview-1" { + t.Fatalf("reviews = %#v", review.Reviews) + } + if len(review.Threads[0].Comments) != 1 || review.Threads[0].Comments[0].URL != "https://github.com/o/r/pull/1#discussion_r1" { + t.Fatalf("thread comments = %#v", review.Threads[0].Comments) + } } func TestFetchReviewThreadsFetchesOneOlderPageWhenOldestUnresolved(t *testing.T) { diff --git a/backend/internal/domain/pr.go b/backend/internal/domain/pr.go index 89b4a961e..4bf4e5fa6 100644 --- a/backend/internal/domain/pr.go +++ b/backend/internal/domain/pr.go @@ -107,6 +107,16 @@ type PullRequestReviewThread struct { UpdatedAt time.Time } +// PullRequestReview is one submitted provider review for a pull request. +type PullRequestReview struct { + ID string + Author string + State ReviewDecision + URL string + IsBot bool + SubmittedAt time.Time +} + // CIState is the aggregate CI status of a PR. type CIState string diff --git a/backend/internal/httpd/apispec/openapi.yaml b/backend/internal/httpd/apispec/openapi.yaml index 1b76c9285..738a9dcef 100644 --- a/backend/internal/httpd/apispec/openapi.yaml +++ b/backend/internal/httpd/apispec/openapi.yaml @@ -2371,10 +2371,14 @@ components: properties: count: type: integer + isBot: + type: boolean links: items: $ref: '#/components/schemas/SessionPRReviewCommentLink' type: array + reviewUrl: + type: string reviewerId: type: string required: diff --git a/backend/internal/httpd/controllers/dto.go b/backend/internal/httpd/controllers/dto.go index f6d4f5e1b..fca0790d2 100644 --- a/backend/internal/httpd/controllers/dto.go +++ b/backend/internal/httpd/controllers/dto.go @@ -295,6 +295,8 @@ type SessionPRUnresolvedReviewer struct { ReviewerID string `json:"reviewerId"` Count int `json:"count"` Links []SessionPRReviewCommentLink `json:"links"` + ReviewURL string `json:"reviewUrl,omitempty"` + IsBot bool `json:"isBot,omitempty"` } // SessionPRReviewCommentLink points to one unresolved review comment. @@ -366,7 +368,7 @@ func newSessionPRReviewSummary(in sessionsvc.PRReviewSummary) SessionPRReviewSum for _, link := range reviewer.Links { links = append(links, SessionPRReviewCommentLink{URL: link.URL, File: link.File, Line: link.Line}) } - reviewers = append(reviewers, SessionPRUnresolvedReviewer{ReviewerID: reviewer.ReviewerID, Count: reviewer.Count, Links: links}) + reviewers = append(reviewers, SessionPRUnresolvedReviewer{ReviewerID: reviewer.ReviewerID, Count: reviewer.Count, Links: links, ReviewURL: reviewer.ReviewURL, IsBot: reviewer.IsBot}) } return SessionPRReviewSummary{Decision: in.Decision, HasUnresolvedHumanComments: in.HasUnresolvedHumanComments, UnresolvedBy: reviewers} } diff --git a/backend/internal/httpd/controllers/sessions_test.go b/backend/internal/httpd/controllers/sessions_test.go index ee27405bb..ab5455245 100644 --- a/backend/internal/httpd/controllers/sessions_test.go +++ b/backend/internal/httpd/controllers/sessions_test.go @@ -192,6 +192,7 @@ func (f *fakeSessionService) ListPRSummaries(_ context.Context, id domain.Sessio UnresolvedBy: []sessionsvc.PRUnresolvedReviewer{{ ReviewerID: "reviewer-a", Count: 1, + ReviewURL: "https://github.com/aoagents/agent-orchestrator/pull/142#pullrequestreview-1", Links: []sessionsvc.PRReviewCommentLink{{URL: "https://github.com/aoagents/agent-orchestrator/pull/142#discussion_r1", File: "main.go", Line: 12}}, }}, }, @@ -844,6 +845,7 @@ func TestSessionsAPI_PRRoutes(t *testing.T) { UnresolvedBy []struct { ReviewerID string `json:"reviewerId"` Count int `json:"count"` + ReviewURL string `json:"reviewUrl"` Links []struct { URL string `json:"url"` File string `json:"file"` @@ -869,7 +871,7 @@ func TestSessionsAPI_PRRoutes(t *testing.T) { if checks := listed.PRs[0].CI.FailingChecks; len(checks) != 1 || checks[0].Name != "unit" || checks[0].LogTail != "" { t.Fatalf("failing checks = %#v", checks) } - if reviewers := listed.PRs[0].Review.UnresolvedBy; len(reviewers) != 1 || reviewers[0].ReviewerID != "reviewer-a" || reviewers[0].Links[0].Body != "" { + if reviewers := listed.PRs[0].Review.UnresolvedBy; len(reviewers) != 1 || reviewers[0].ReviewerID != "reviewer-a" || reviewers[0].ReviewURL == "" || reviewers[0].Links[0].Body != "" { t.Fatalf("reviewers = %#v", reviewers) } if merge := listed.PRs[0].Mergeability; merge.State != "conflicting" || len(merge.ConflictFiles) != 0 || merge.PRURL == "" { diff --git a/backend/internal/observe/scm/observer.go b/backend/internal/observe/scm/observer.go index 2d7ec0845..1ca0fa10e 100644 --- a/backend/internal/observe/scm/observer.go +++ b/backend/internal/observe/scm/observer.go @@ -53,7 +53,7 @@ type Store interface { UpsertProject(ctx context.Context, row domain.ProjectRecord) error ListPRsBySession(ctx context.Context, sessionID domain.SessionID) ([]domain.PullRequest, error) ListChecks(ctx context.Context, prURL string) ([]domain.PullRequestCheck, error) - WriteSCMObservation(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode) error + WriteSCMObservation(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, reviews []domain.PullRequestReview, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode) error } // Lifecycle is the provider-neutral lifecycle notification sink. @@ -344,8 +344,8 @@ func (o *Observer) Poll(ctx context.Context) error { prRefreshOK[key] = true continue } - finalPR, finalChecks, finalThreads, finalComments := domainFromObservation(subj.session.ID, prepared, local, opts, now) - pr, checks, threads, comments := finalPR, finalChecks, finalThreads, finalComments + finalPR, finalChecks, finalReviews, finalThreads, finalComments := domainFromObservation(subj.session.ID, prepared, local, opts, now) + pr, checks, reviews, threads, comments := finalPR, finalChecks, finalReviews, finalThreads, finalComments // Lifecycle is allowed to run only after the observed facts are durable, // but semantic hashes are the observer's acknowledgement cursor. Keep // changed hashes at their local values until lifecycle succeeds; if the @@ -362,9 +362,9 @@ func (o *Observer) Poll(ctx context.Context) error { if prepared.Changed.Review { pendingOpts.preserveLocalReviewHash = true } - pr, checks, threads, comments = domainFromObservation(subj.session.ID, prepared, local, pendingOpts, now) + pr, checks, reviews, threads, comments = domainFromObservation(subj.session.ID, prepared, local, pendingOpts, now) } - if err := o.store.WriteSCMObservation(ctx, pr, checks, threads, comments, reviewMode); err != nil { + if err := o.store.WriteSCMObservation(ctx, pr, checks, reviews, threads, comments, reviewMode); err != nil { o.logger.Error("scm observer: DB write failed", "session", subj.session.ID, "pr", pr.URL, "err", err) markRepoRefreshFailed(subj.repo) continue @@ -375,7 +375,7 @@ func (o *Observer) Poll(ctx context.Context) error { markRepoRefreshFailed(subj.repo) continue } - if err := o.store.WriteSCMObservation(ctx, finalPR, finalChecks, nil, nil, ports.ReviewWritePreserve); err != nil { + if err := o.store.WriteSCMObservation(ctx, finalPR, finalChecks, nil, nil, nil, ports.ReviewWritePreserve); err != nil { o.logger.Error("scm observer: DB lifecycle acknowledgement failed", "session", subj.session.ID, "pr", finalPR.URL, "err", err) markRepoRefreshFailed(subj.repo) continue @@ -586,7 +586,7 @@ func (o *Observer) discoverNewPRs(ctx context.Context, sessionRepos []sessionRep // reads every PR of the session from the store. Without this write, an // open sibling/child discovered in the same poll would not yet be // durable, and the session could terminate while that PR is still open. - if err := o.store.WriteSCMObservation(ctx, known, nil, nil, nil, ports.ReviewWritePreserve); err != nil { + if err := o.store.WriteSCMObservation(ctx, known, nil, nil, nil, nil, ports.ReviewWritePreserve); err != nil { o.logger.Error("scm observer: persist discovered PR failed", "session", sr.session.ID, "pr", known.URL, "err", err) if markRepoFailed != nil { markRepoFailed(repo) @@ -793,6 +793,7 @@ func (o *Observer) refreshReviews(ctx context.Context, subjects map[string]*subj if review.Decision != "" { obs.Review.Decision = review.Decision } + obs.Review.Reviews = review.Reviews obs.Review.Threads = review.Threads obs.Review.Partial = review.Partial obs.ObservedAt = now @@ -850,7 +851,7 @@ func (o *Observer) prepareForPersistence(obs ports.SCMObservation, local domain. return obs } -func domainFromObservation(sessionID domain.SessionID, obs ports.SCMObservation, local domain.PullRequest, opts persistenceOptions, now time.Time) (domain.PullRequest, []domain.PullRequestCheck, []domain.PullRequestReviewThread, []domain.PullRequestComment) { +func domainFromObservation(sessionID domain.SessionID, obs ports.SCMObservation, local domain.PullRequest, opts persistenceOptions, now time.Time) (domain.PullRequest, []domain.PullRequestCheck, []domain.PullRequestReview, []domain.PullRequestReviewThread, []domain.PullRequestComment) { metadataHash := metadataSemanticHash(obs) if opts.preserveLocalMetadataHash { metadataHash = local.MetadataHash @@ -924,6 +925,17 @@ func domainFromObservation(sessionID domain.SessionID, obs ports.SCMObservation, for _, ch := range obs.CI.Checks { checks = append(checks, domain.PullRequestCheck{Name: ch.Name, CommitHash: obs.CI.HeadSHA, Status: domain.PRCheckStatus(ch.Status), Conclusion: ch.Conclusion, URL: ch.URL, Details: ch.ProviderID, LogTail: ch.LogTail, CreatedAt: now}) } + reviews := make([]domain.PullRequestReview, 0, len(obs.Review.Reviews)) + for _, review := range obs.Review.Reviews { + reviews = append(reviews, domain.PullRequestReview{ + ID: review.ID, + Author: review.Author, + State: domain.ReviewDecision(firstNonEmpty(review.State, string(domain.ReviewNone))), + URL: review.URL, + IsBot: review.IsBot, + SubmittedAt: firstTime(review.SubmittedAt, now), + }) + } threads := make([]domain.PullRequestReviewThread, 0, len(obs.Review.Threads)) commentCount := 0 for _, th := range obs.Review.Threads { @@ -936,7 +948,7 @@ func domainFromObservation(sessionID domain.SessionID, obs ports.SCMObservation, comments = append(comments, domain.PullRequestComment{ThreadID: th.ID, ID: c.ID, Author: c.Author, File: th.Path, Line: th.Line, Body: c.Body, URL: c.URL, Resolved: th.Resolved, IsBot: c.IsBot || th.IsBot, CreatedAt: now}) } } - return pr, checks, threads, comments + return pr, checks, reviews, threads, comments } func observationFromLocal(repo ports.SCMRepo, pr domain.PullRequest, checks []domain.PullRequestCheck) ports.SCMObservation { @@ -1119,10 +1131,11 @@ func ciSemanticHash(ci ports.SCMCIObservation) string { func reviewSemanticHash(review ports.SCMReviewObservation) string { type reviewHashPayload struct { Decision string + Reviews []ports.SCMReviewSummaryObservation Threads []ports.SCMReviewThreadObservation Partial bool `json:",omitempty"` } - return stableHash(reviewHashPayload{Decision: review.Decision, Threads: review.Threads, Partial: review.Partial}) + return stableHash(reviewHashPayload{Decision: review.Decision, Reviews: review.Reviews, Threads: review.Threads, Partial: review.Partial}) } func threadSemanticHash(th ports.SCMReviewThreadObservation) string { diff --git a/backend/internal/observe/scm/observer_test.go b/backend/internal/observe/scm/observer_test.go index 957344dd6..dec26750a 100644 --- a/backend/internal/observe/scm/observer_test.go +++ b/backend/internal/observe/scm/observer_test.go @@ -41,6 +41,7 @@ type fakeStore struct { type fakeWrite struct { pr domain.PullRequest checks []domain.PullRequestCheck + reviews []domain.PullRequestReview comments []domain.PullRequestComment reviewMode ports.ReviewWriteMode } @@ -94,13 +95,13 @@ func (s *fakeStore) ListChecks(_ context.Context, prURL string) ([]domain.PullRe return append([]domain.PullRequestCheck(nil), s.checks[prURL]...), nil } -func (s *fakeStore) WriteSCMObservation(_ context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode) error { +func (s *fakeStore) WriteSCMObservation(_ context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, reviews []domain.PullRequestReview, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode) error { s.mu.Lock() defer s.mu.Unlock() if s.writeErr != nil { return s.writeErr } - s.writes = append(s.writes, fakeWrite{pr: pr, checks: append([]domain.PullRequestCheck(nil), checks...), comments: append([]domain.PullRequestComment(nil), comments...), reviewMode: reviewMode}) + s.writes = append(s.writes, fakeWrite{pr: pr, checks: append([]domain.PullRequestCheck(nil), checks...), reviews: append([]domain.PullRequestReview(nil), reviews...), comments: append([]domain.PullRequestComment(nil), comments...), reviewMode: reviewMode}) return nil } @@ -229,7 +230,7 @@ func testObs(num int) ports.SCMObservation { func knownPR(num int) domain.PullRequest { obs := testObs(num) - pr, _, _, _ := domainFromObservation("p-1", obs, domain.PullRequest{}, persistenceOptions{}, time.Unix(1, 0).UTC()) + pr, _, _, _, _ := domainFromObservation("p-1", obs, domain.PullRequest{}, persistenceOptions{}, time.Unix(1, 0).UTC()) return pr } @@ -731,7 +732,11 @@ func TestPoll_ReviewHashDrivesPersistenceAndLifecycle(t *testing.T) { local.ReviewHash = "old" local.Review = domain.ReviewChangesRequest store.prs["p-1"] = []domain.PullRequest{local} - review := ports.SCMReviewObservation{Decision: string(domain.ReviewChangesRequest), Threads: []ports.SCMReviewThreadObservation{{ID: "t1", Path: "f.go", Line: 2, Comments: []ports.SCMReviewCommentObservation{{ID: "c1", Author: "ann", Body: "fix this"}}}}} + review := ports.SCMReviewObservation{ + Decision: string(domain.ReviewChangesRequest), + Reviews: []ports.SCMReviewSummaryObservation{{ID: "review-1", Author: "ann", State: string(domain.ReviewChangesRequest), URL: "https://github.com/o/r/pull/1#pullrequestreview-1", SubmittedAt: time.Unix(199, 0).UTC()}}, + Threads: []ports.SCMReviewThreadObservation{{ID: "t1", Path: "f.go", Line: 2, Comments: []ports.SCMReviewCommentObservation{{ID: "c1", Author: "ann", Body: "fix this"}}}}, + } provider := &fakeProvider{repoGuards: map[string]ports.SCMGuardResult{prKey(testRepo, 0): {ETag: "repo", NotModified: true}}, observations: map[string]ports.SCMObservation{}, reviews: map[string]ports.SCMReviewObservation{prKey(testRepo, 1): review}} lc := &fakeLifecycle{} obs := newTestObserver(store, provider, lc, time.Unix(200, 0).UTC()) @@ -742,6 +747,9 @@ func TestPoll_ReviewHashDrivesPersistenceAndLifecycle(t *testing.T) { if len(store.writes) == 0 || len(store.writes[0].comments) != 1 { t.Fatalf("review change not persisted: %#v", store.writes) } + if len(store.writes[0].reviews) != 1 || store.writes[0].reviews[0].URL != "https://github.com/o/r/pull/1#pullrequestreview-1" { + t.Fatalf("review summaries not persisted: %#v", store.writes[0].reviews) + } if len(store.writes) != 2 { t.Fatalf("review change with lifecycle should write held-back facts then acknowledgement, got %d writes", len(store.writes)) } diff --git a/backend/internal/ports/outbound.go b/backend/internal/ports/outbound.go index 8f6b75f09..359cd5f8c 100644 --- a/backend/internal/ports/outbound.go +++ b/backend/internal/ports/outbound.go @@ -38,13 +38,13 @@ const ( // whether review facts are preserved, replaced with a complete snapshot, or // merged as a bounded partial window. type SCMWriter interface { - WriteSCMObservation(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ReviewWriteMode) error + WriteSCMObservation(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, reviews []domain.PullRequestReview, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ReviewWriteMode) error } // PRClaimer atomically moves (or creates) a PR row for a target session and // persists the live SCM facts observed for that PR in the same transaction. type PRClaimer interface { - ClaimPR(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ReviewWriteMode, allowActiveTakeover bool) (ClaimOutcome, error) + ClaimPR(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, reviews []domain.PullRequestReview, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ReviewWriteMode, allowActiveTakeover bool) (ClaimOutcome, error) } // ErrPRClaimedByActiveSession is returned by PRClaimer.ClaimPR when takeover is diff --git a/backend/internal/ports/scm_observations.go b/backend/internal/ports/scm_observations.go index 48e84e9bf..36cb5551b 100644 --- a/backend/internal/ports/scm_observations.go +++ b/backend/internal/ports/scm_observations.go @@ -184,6 +184,8 @@ type SCMCheckObservation struct { type SCMReviewObservation struct { // Decision is AO's normalized review decision. Decision string + // Reviews contains submitted review summaries fetched on the slower review cadence. + Reviews []SCMReviewSummaryObservation // Threads contains normalized review threads fetched on the slower review cadence. Threads []SCMReviewThreadObservation // Partial is true when the provider intentionally fetched and persisted a @@ -192,6 +194,22 @@ type SCMReviewObservation struct { Partial bool } +// SCMReviewSummaryObservation is one submitted review with its provider summary URL. +type SCMReviewSummaryObservation struct { + // ID is the provider's stable submitted-review identifier. + ID string + // Author is the provider login/name of the reviewer. + Author string + // State is AO's normalized review decision for this review. + State string + // URL is a provider link to the submitted review summary. + URL string + // IsBot is true when the provider identifies the reviewer as a bot. + IsBot bool + // SubmittedAt is the provider's review submission timestamp. + SubmittedAt time.Time +} + // SCMReviewThreadObservation is a normalized review thread with comments. type SCMReviewThreadObservation struct { // ID is the provider's stable review thread identifier. diff --git a/backend/internal/service/session/claim_pr.go b/backend/internal/service/session/claim_pr.go index 6d6cd92b7..54070e1dc 100644 --- a/backend/internal/service/session/claim_pr.go +++ b/backend/internal/service/session/claim_pr.go @@ -113,8 +113,8 @@ func (s *Service) ClaimPR(ctx context.Context, id domain.SessionID, ref string, return ClaimPRResult{}, err } now := s.clock().UTC() - pr, checks, threads, comments := claimRowsFromSCM(id, obs, now) - outcome, err := s.prClaimer.ClaimPR(ctx, pr, checks, threads, comments, reviewMode, opts.AllowTakeover) + pr, checks, reviews, threads, comments := claimRowsFromSCM(id, obs, now) + outcome, err := s.prClaimer.ClaimPR(ctx, pr, checks, reviews, threads, comments, reviewMode, opts.AllowTakeover) if err != nil { return ClaimPRResult{}, err } @@ -163,6 +163,7 @@ func (s *Service) enrichClaimReviews(ctx context.Context, ref ports.SCMPRRef, ob obs.Review.Decision = review.Decision } obs.Review.Threads = review.Threads + obs.Review.Reviews = review.Reviews obs.Review.Partial = review.Partial if review.Partial { return ports.ReviewWriteMerge, nil @@ -181,7 +182,7 @@ func scmRepoForClaim(provider scmProvider, projectOrigin, prURL string) (ports.S return ports.SCMRepo{Provider: "github", Host: "github.com", Owner: owner, Name: name, Repo: owner + "/" + name}, nil } -func claimRowsFromSCM(sessionID domain.SessionID, obs ports.SCMObservation, now time.Time) (domain.PullRequest, []domain.PullRequestCheck, []domain.PullRequestReviewThread, []domain.PullRequestComment) { +func claimRowsFromSCM(sessionID domain.SessionID, obs ports.SCMObservation, now time.Time) (domain.PullRequest, []domain.PullRequestCheck, []domain.PullRequestReview, []domain.PullRequestReviewThread, []domain.PullRequestComment) { observedAt := obs.ObservedAt if observedAt.IsZero() { observedAt = now @@ -226,6 +227,21 @@ func claimRowsFromSCM(sessionID domain.SessionID, obs ports.SCMObservation, now for _, ch := range obs.CI.Checks { checks = append(checks, domain.PullRequestCheck{Name: ch.Name, CommitHash: obs.CI.HeadSHA, Status: domain.PRCheckStatus(ch.Status), Conclusion: ch.Conclusion, URL: ch.URL, Details: ch.ProviderID, LogTail: ch.LogTail, CreatedAt: now}) } + reviews := make([]domain.PullRequestReview, 0, len(obs.Review.Reviews)) + for _, review := range obs.Review.Reviews { + submittedAt := review.SubmittedAt + if submittedAt.IsZero() { + submittedAt = now + } + reviews = append(reviews, domain.PullRequestReview{ + ID: review.ID, + Author: review.Author, + State: domain.ReviewDecision(firstNonEmpty(review.State, string(domain.ReviewNone))), + URL: review.URL, + IsBot: review.IsBot, + SubmittedAt: submittedAt, + }) + } threads := make([]domain.PullRequestReviewThread, 0, len(obs.Review.Threads)) commentCount := 0 for _, th := range obs.Review.Threads { @@ -238,7 +254,7 @@ func claimRowsFromSCM(sessionID domain.SessionID, obs ports.SCMObservation, now comments = append(comments, domain.PullRequestComment{ThreadID: th.ID, ID: c.ID, Author: c.Author, File: th.Path, Line: th.Line, Body: c.Body, URL: c.URL, Resolved: th.Resolved, IsBot: c.IsBot || th.IsBot, CreatedAt: now}) } } - return pr, checks, threads, comments + return pr, checks, reviews, threads, comments } func sessionmanagerAPIError(code, message string) error { diff --git a/backend/internal/service/session/pr_summary.go b/backend/internal/service/session/pr_summary.go index 1a09fb5f7..ad8250b28 100644 --- a/backend/internal/service/session/pr_summary.go +++ b/backend/internal/service/session/pr_summary.go @@ -62,6 +62,8 @@ type PRUnresolvedReviewer struct { ReviewerID string Count int Links []PRReviewCommentLink + ReviewURL string + IsBot bool } // PRReviewCommentLink points to one unresolved review comment. @@ -107,17 +109,21 @@ func (s *Service) ListPRSummaries(ctx context.Context, id domain.SessionID) ([]P if err != nil { return nil, err } + reviews, err := s.store.ListPRReviews(ctx, pr.URL) + if err != nil { + return nil, err + } comments, err := s.store.ListPRComments(ctx, pr.URL) if err != nil { return nil, err } - out = append(out, summarizePR(pr, checks, threads, comments)) + out = append(out, summarizePR(pr, checks, reviews, threads, comments)) } sortPRSummaries(out) return out, nil } -func summarizePR(pr domain.PullRequest, checks []domain.PullRequestCheck, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment) PRSummary { +func summarizePR(pr domain.PullRequest, checks []domain.PullRequestCheck, reviews []domain.PullRequestReview, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment) PRSummary { return PRSummary{ URL: pr.URL, HTMLURL: firstNonEmpty(pr.HTMLURL, pr.URL), @@ -134,7 +140,7 @@ func summarizePR(pr domain.PullRequest, checks []domain.PullRequestCheck, thread Deletions: pr.Deletions, ChangedFiles: pr.ChangedFiles, CI: summarizeCI(pr, checks), - Review: summarizeReview(pr, comments), + Review: summarizeReview(pr, comments, reviews), Mergeability: summarizeMergeability(pr, threads), UpdatedAt: pr.UpdatedAt, ObservedAt: pr.ObservedAt, @@ -166,7 +172,7 @@ func summarizeCI(pr domain.PullRequest, checks []domain.PullRequestCheck) PRCISu return out } -func summarizeReview(pr domain.PullRequest, comments []domain.PullRequestComment) PRReviewSummary { +func summarizeReview(pr domain.PullRequest, comments []domain.PullRequestComment, reviews []domain.PullRequestReview) PRReviewSummary { out := PRReviewSummary{Decision: reviewOrNone(pr.Review)} if pr.Merged || pr.Closed { return out @@ -174,6 +180,7 @@ func summarizeReview(pr domain.PullRequest, comments []domain.PullRequestComment byReviewer := map[string]int{} order := []string{} links := map[string][]PRReviewCommentLink{} + isBot := map[string]bool{} for _, c := range comments { if c.Resolved || c.IsBot { continue @@ -186,24 +193,74 @@ func summarizeReview(pr domain.PullRequest, comments []domain.PullRequestComment order = append(order, reviewer) } byReviewer[reviewer]++ + isBot[reviewer] = c.IsBot links[reviewer] = append(links[reviewer], PRReviewCommentLink{ URL: c.URL, File: c.File, Line: c.Line, }) } + reviewURLByAuthor := map[string]string{} + for reviewer, review := range latestChangesRequestedReviews(reviews) { + if _, ok := byReviewer[reviewer]; !ok { + order = append(order, reviewer) + } + reviewURLByAuthor[reviewer] = review.URL + isBot[reviewer] = review.IsBot + } sort.Strings(order) for _, reviewer := range order { out.UnresolvedBy = append(out.UnresolvedBy, PRUnresolvedReviewer{ ReviewerID: reviewer, Count: byReviewer[reviewer], Links: links[reviewer], + ReviewURL: reviewURLByAuthor[reviewer], + IsBot: isBot[reviewer], }) } - out.HasUnresolvedHumanComments = len(out.UnresolvedBy) > 0 + for _, reviewer := range out.UnresolvedBy { + if reviewer.Count > 0 && !reviewer.IsBot { + out.HasUnresolvedHumanComments = true + break + } + } return out } +func latestChangesRequestedReviews(reviews []domain.PullRequestReview) map[string]domain.PullRequestReview { + latestByReviewer := map[string]domain.PullRequestReview{} + for _, review := range reviews { + if review.State != domain.ReviewChangesRequest && review.State != domain.ReviewApproved { + continue + } + reviewer := strings.TrimSpace(review.Author) + if reviewer == "" { + reviewer = "unknown" + } + current, ok := latestByReviewer[reviewer] + if !ok || reviewAfter(review, current) { + latestByReviewer[reviewer] = review + } + } + out := map[string]domain.PullRequestReview{} + for reviewer, review := range latestByReviewer { + if review.State == domain.ReviewChangesRequest { + out[reviewer] = review + } + } + return out +} + +func reviewAfter(a, b domain.PullRequestReview) bool { + if a.SubmittedAt.IsZero() || b.SubmittedAt.IsZero() { + return a.SubmittedAt.IsZero() == b.SubmittedAt.IsZero() && a.ID > b.ID + } + if a.SubmittedAt.Equal(b.SubmittedAt) { + return a.ID > b.ID + } + return a.SubmittedAt.After(b.SubmittedAt) +} + func summarizeMergeability(pr domain.PullRequest, _ []domain.PullRequestReviewThread) PRMergeabilitySummary { return PRMergeabilitySummary{ State: mergeabilityOrUnknown(pr.Mergeability), diff --git a/backend/internal/service/session/service.go b/backend/internal/service/session/service.go index 9f092d2e3..740c9e43c 100644 --- a/backend/internal/service/session/service.go +++ b/backend/internal/service/session/service.go @@ -25,6 +25,7 @@ type Store interface { ListPRFactsForSession(ctx context.Context, id domain.SessionID) ([]domain.PRFacts, error) ListPRsBySession(ctx context.Context, sessionID domain.SessionID) ([]domain.PullRequest, error) ListChecks(ctx context.Context, prURL string) ([]domain.PullRequestCheck, error) + ListPRReviews(ctx context.Context, prURL string) ([]domain.PullRequestReview, error) ListPRReviewThreads(ctx context.Context, prURL string) ([]domain.PullRequestReviewThread, error) ListPRComments(ctx context.Context, prURL string) ([]domain.PullRequestComment, error) GetProject(ctx context.Context, id string) (domain.ProjectRecord, bool, error) diff --git a/backend/internal/service/session/service_test.go b/backend/internal/service/session/service_test.go index 84a484990..405e17964 100644 --- a/backend/internal/service/session/service_test.go +++ b/backend/internal/service/session/service_test.go @@ -25,6 +25,7 @@ type fakeStore struct { pr map[domain.SessionID]domain.PRFacts projects map[string]domain.ProjectRecord checks map[string][]domain.PullRequestCheck + reviews map[string][]domain.PullRequestReview threads map[string][]domain.PullRequestReviewThread comments map[string][]domain.PullRequestComment num int @@ -36,6 +37,7 @@ func newFakeStore() *fakeStore { pr: map[domain.SessionID]domain.PRFacts{}, projects: map[string]domain.ProjectRecord{}, checks: map[string][]domain.PullRequestCheck{}, + reviews: map[string][]domain.PullRequestReview{}, threads: map[string][]domain.PullRequestReviewThread{}, comments: map[string][]domain.PullRequestComment{}, } @@ -118,6 +120,10 @@ func (f *fakeStore) ListChecks(_ context.Context, prURL string) ([]domain.PullRe return append([]domain.PullRequestCheck(nil), f.checks[prURL]...), nil } +func (f *fakeStore) ListPRReviews(_ context.Context, prURL string) ([]domain.PullRequestReview, error) { + return append([]domain.PullRequestReview(nil), f.reviews[prURL]...), nil +} + func (f *fakeStore) ListPRReviewThreads(_ context.Context, prURL string) ([]domain.PullRequestReviewThread, error) { return append([]domain.PullRequestReviewThread(nil), f.threads[prURL]...), nil } @@ -622,7 +628,7 @@ type errorFreeClaimOutcome struct { ports.ClaimOutcome } -func (f fakePRClaimer) ClaimPR(context.Context, domain.PullRequest, []domain.PullRequestCheck, []domain.PullRequestReviewThread, []domain.PullRequestComment, ports.ReviewWriteMode, bool) (ports.ClaimOutcome, error) { +func (f fakePRClaimer) ClaimPR(context.Context, domain.PullRequest, []domain.PullRequestCheck, []domain.PullRequestReview, []domain.PullRequestReviewThread, []domain.PullRequestComment, ports.ReviewWriteMode, bool) (ports.ClaimOutcome, error) { return f.out.ClaimOutcome, f.err } @@ -740,6 +746,9 @@ func TestListPRSummariesOmitsRawLogsAndReviewBodies(t *testing.T) { {Name: "unit", Status: domain.PRCheckFailed, Conclusion: "failure", URL: "https://github.com/acme/repo/actions/runs/1", LogTail: "panic: secret"}, {Name: "lint", Status: domain.PRCheckPassed, Conclusion: "success", URL: "https://github.com/acme/repo/actions/runs/2"}, } + stList.reviews[prURL] = []domain.PullRequestReview{ + {ID: "review-1", Author: "reviewer-a", State: domain.ReviewChangesRequest, URL: "https://github.com/acme/repo/pull/7#pullrequestreview-1", SubmittedAt: now.Add(-30 * time.Second)}, + } stList.comments[prURL] = []domain.PullRequestComment{ {Author: "reviewer-a", File: "main.go", Line: 12, Body: "raw body must stay private", URL: "https://github.com/acme/repo/pull/7#discussion_r1"}, {Author: "ci-bot", File: "main.go", Line: 13, Body: "bot body", URL: "https://github.com/acme/repo/pull/7#discussion_r2", IsBot: true}, @@ -765,6 +774,8 @@ func TestListPRSummariesOmitsRawLogsAndReviewBodies(t *testing.T) { } if reviewer := pr.Review.UnresolvedBy[0]; reviewer.ReviewerID != "reviewer-a" || reviewer.Count != 2 || len(reviewer.Links) != 2 { t.Fatalf("reviewer = %+v", reviewer) + } else if reviewer.ReviewURL != "https://github.com/acme/repo/pull/7#pullrequestreview-1" { + t.Fatalf("review url = %q", reviewer.ReviewURL) } if pr.Mergeability.State != domain.MergeConflicting || len(pr.Mergeability.ConflictFiles) != 0 || !containsString(pr.Mergeability.Reasons, "conflicts") { t.Fatalf("mergeability = %+v", pr.Mergeability) diff --git a/backend/internal/storage/sqlite/gen/models.go b/backend/internal/storage/sqlite/gen/models.go index ccc4bf74a..1140f53cc 100644 --- a/backend/internal/storage/sqlite/gen/models.go +++ b/backend/internal/storage/sqlite/gen/models.go @@ -101,6 +101,16 @@ type PRComment struct { IsBot int64 } +type PRReview struct { + PRURL string + ReviewID string + Author string + State string + URL string + IsBot int64 + SubmittedAt time.Time +} + type PRReviewThread struct { PRURL string ThreadID string diff --git a/backend/internal/storage/sqlite/gen/pr_reviews.sql.go b/backend/internal/storage/sqlite/gen/pr_reviews.sql.go new file mode 100644 index 000000000..153595db8 --- /dev/null +++ b/backend/internal/storage/sqlite/gen/pr_reviews.sql.go @@ -0,0 +1,90 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: pr_reviews.sql + +package gen + +import ( + "context" + "time" +) + +const deletePRReviews = `-- name: DeletePRReviews :exec +DELETE FROM pr_reviews WHERE pr_url = ? +` + +func (q *Queries) DeletePRReviews(ctx context.Context, prUrl string) error { + _, err := q.db.ExecContext(ctx, deletePRReviews, prUrl) + return err +} + +const listPRReviews = `-- name: ListPRReviews :many +SELECT pr_url, review_id, author, state, url, is_bot, submitted_at +FROM pr_reviews WHERE pr_url = ? ORDER BY submitted_at, review_id +` + +func (q *Queries) ListPRReviews(ctx context.Context, prUrl string) ([]PRReview, error) { + rows, err := q.db.QueryContext(ctx, listPRReviews, prUrl) + if err != nil { + return nil, err + } + defer rows.Close() + items := []PRReview{} + for rows.Next() { + var i PRReview + if err := rows.Scan( + &i.PRURL, + &i.ReviewID, + &i.Author, + &i.State, + &i.URL, + &i.IsBot, + &i.SubmittedAt, + ); 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 upsertPRReview = `-- name: UpsertPRReview :exec +INSERT INTO pr_reviews (pr_url, review_id, author, state, url, is_bot, submitted_at) +VALUES (?, ?, ?, ?, ?, ?, ?) +ON CONFLICT (pr_url, review_id) DO UPDATE SET + author = excluded.author, + state = excluded.state, + url = excluded.url, + is_bot = excluded.is_bot, + submitted_at = excluded.submitted_at +` + +type UpsertPRReviewParams struct { + PRURL string + ReviewID string + Author string + State string + URL string + IsBot int64 + SubmittedAt time.Time +} + +func (q *Queries) UpsertPRReview(ctx context.Context, arg UpsertPRReviewParams) error { + _, err := q.db.ExecContext(ctx, upsertPRReview, + arg.PRURL, + arg.ReviewID, + arg.Author, + arg.State, + arg.URL, + arg.IsBot, + arg.SubmittedAt, + ) + return err +} diff --git a/backend/internal/storage/sqlite/migrations/0020_pr_reviews.sql b/backend/internal/storage/sqlite/migrations/0020_pr_reviews.sql new file mode 100644 index 000000000..4cbbc6385 --- /dev/null +++ b/backend/internal/storage/sqlite/migrations/0020_pr_reviews.sql @@ -0,0 +1,22 @@ +-- Summary: persist provider review submissions so the UI can link to review summaries. +-- +goose Up +-- +goose StatementBegin +CREATE TABLE pr_reviews ( + pr_url TEXT NOT NULL REFERENCES pr (url) ON DELETE CASCADE, + review_id TEXT NOT NULL, + author TEXT NOT NULL DEFAULT '', + state TEXT NOT NULL DEFAULT 'none' + CHECK (state IN ('none', 'approved', 'changes_requested', 'review_required')), + url TEXT NOT NULL DEFAULT '', + is_bot INTEGER NOT NULL DEFAULT 0, + submitted_at TIMESTAMP NOT NULL, + PRIMARY KEY (pr_url, review_id) +); +CREATE INDEX idx_pr_reviews_lookup ON pr_reviews (pr_url, submitted_at); +-- +goose StatementEnd + +-- +goose Down +-- +goose StatementBegin +DROP INDEX IF EXISTS idx_pr_reviews_lookup; +DROP TABLE IF EXISTS pr_reviews; +-- +goose StatementEnd diff --git a/backend/internal/storage/sqlite/queries/pr_reviews.sql b/backend/internal/storage/sqlite/queries/pr_reviews.sql new file mode 100644 index 000000000..be2353257 --- /dev/null +++ b/backend/internal/storage/sqlite/queries/pr_reviews.sql @@ -0,0 +1,16 @@ +-- name: UpsertPRReview :exec +INSERT INTO pr_reviews (pr_url, review_id, author, state, url, is_bot, submitted_at) +VALUES (?, ?, ?, ?, ?, ?, ?) +ON CONFLICT (pr_url, review_id) DO UPDATE SET + author = excluded.author, + state = excluded.state, + url = excluded.url, + is_bot = excluded.is_bot, + submitted_at = excluded.submitted_at; + +-- name: DeletePRReviews :exec +DELETE FROM pr_reviews WHERE pr_url = ?; + +-- name: ListPRReviews :many +SELECT pr_url, review_id, author, state, url, is_bot, submitted_at +FROM pr_reviews WHERE pr_url = ? ORDER BY submitted_at, review_id; diff --git a/backend/internal/storage/sqlite/store/pr_cdc_test.go b/backend/internal/storage/sqlite/store/pr_cdc_test.go index ff382c6d3..8beb52cdf 100644 --- a/backend/internal/storage/sqlite/store/pr_cdc_test.go +++ b/backend/internal/storage/sqlite/store/pr_cdc_test.go @@ -64,17 +64,17 @@ func TestPRReviewThreadsCDC_EmitsOnInsertAndResolvedTransition(t *testing.T) { now := time.Now().UTC().Truncate(time.Second) pr := domain.PullRequest{URL: "https://example/pr/9", SessionID: rec.ID, Number: 9, UpdatedAt: now} - if err := s.WriteSCMObservation(ctx, pr, nil, []domain.PullRequestReviewThread{{ + if err := s.WriteSCMObservation(ctx, pr, nil, nil, []domain.PullRequestReviewThread{{ ThreadID: "t1", Path: "main.go", Line: 7, IsBot: true, SemanticHash: "v1", UpdatedAt: now, }}, nil, ports.ReviewWriteReplace); err != nil { t.Fatal(err) } - if err := s.WriteSCMObservation(ctx, pr, nil, []domain.PullRequestReviewThread{{ + if err := s.WriteSCMObservation(ctx, pr, nil, nil, []domain.PullRequestReviewThread{{ ThreadID: "t1", Path: "main.go", Line: 8, Resolved: true, IsBot: true, SemanticHash: "v2", UpdatedAt: now.Add(time.Second), }}, nil, ports.ReviewWriteMerge); err != nil { t.Fatal(err) } - if err := s.WriteSCMObservation(ctx, pr, nil, []domain.PullRequestReviewThread{{ + if err := s.WriteSCMObservation(ctx, pr, nil, nil, []domain.PullRequestReviewThread{{ ThreadID: "t1", Path: "main.go", Line: 9, Resolved: true, IsBot: true, SemanticHash: "v3", UpdatedAt: now.Add(2 * time.Second), }}, nil, ports.ReviewWriteMerge); err != nil { t.Fatal(err) @@ -138,7 +138,7 @@ func TestPRReviewThreadsCDC_EmitsResolvedOnReplacePoll(t *testing.T) { // First poll: seed via Replace (no Partial pagination). Same shape as the // real GitHub provider path when the review-thread listing fits in one page. - if err := s.WriteSCMObservation(ctx, pr, nil, []domain.PullRequestReviewThread{{ + if err := s.WriteSCMObservation(ctx, pr, nil, nil, []domain.PullRequestReviewThread{{ ThreadID: "t1", Path: "main.go", Line: 7, IsBot: true, SemanticHash: "v1", UpdatedAt: now, }}, nil, ports.ReviewWriteReplace); err != nil { t.Fatal(err) @@ -148,7 +148,7 @@ func TestPRReviewThreadsCDC_EmitsResolvedOnReplacePoll(t *testing.T) { // On the buggy code this fired the INSERT trigger again (because the // DELETE-all removed the row first) and the UPDATE trigger never saw the // resolved transition. - if err := s.WriteSCMObservation(ctx, pr, nil, []domain.PullRequestReviewThread{{ + if err := s.WriteSCMObservation(ctx, pr, nil, nil, []domain.PullRequestReviewThread{{ ThreadID: "t1", Path: "main.go", Line: 7, Resolved: true, IsBot: true, SemanticHash: "v2", UpdatedAt: now.Add(time.Second), }}, nil, ports.ReviewWriteReplace); err != nil { t.Fatal(err) @@ -197,13 +197,13 @@ func TestPRReviewThreadsReplace_PrunesOrphansWithoutReinserting(t *testing.T) { now := time.Now().UTC().Truncate(time.Second) pr := domain.PullRequest{URL: "https://example/pr/56", SessionID: rec.ID, Number: 56, UpdatedAt: now} - if err := s.WriteSCMObservation(ctx, pr, nil, []domain.PullRequestReviewThread{ + if err := s.WriteSCMObservation(ctx, pr, nil, nil, []domain.PullRequestReviewThread{ {ThreadID: "keep", Path: "a.go", Line: 1, IsBot: true, SemanticHash: "k1", UpdatedAt: now}, {ThreadID: "drop", Path: "b.go", Line: 2, IsBot: true, SemanticHash: "d1", UpdatedAt: now}, }, nil, ports.ReviewWriteReplace); err != nil { t.Fatal(err) } - if err := s.WriteSCMObservation(ctx, pr, nil, []domain.PullRequestReviewThread{ + if err := s.WriteSCMObservation(ctx, pr, nil, nil, []domain.PullRequestReviewThread{ {ThreadID: "keep", Path: "a.go", Line: 1, Resolved: true, IsBot: true, SemanticHash: "k2", UpdatedAt: now.Add(time.Second)}, }, nil, ports.ReviewWriteReplace); err != nil { t.Fatal(err) @@ -283,7 +283,7 @@ func TestClaimPR_CreatesMovesAndGuardsActiveOwner(t *testing.T) { url := "https://github.com/acme/repo/pull/42" pr := domain.PullRequest{URL: url, SessionID: first.ID, Number: 42, CI: domain.CIPassing, Mergeability: domain.MergeMergeable, UpdatedAt: time.Now().UTC()} - out, err := s.ClaimPR(ctx, pr, nil, nil, nil, ports.ReviewWritePreserve, true) + out, err := s.ClaimPR(ctx, pr, nil, nil, nil, nil, ports.ReviewWritePreserve, true) if err != nil { t.Fatalf("initial claim: %v", err) } @@ -296,7 +296,7 @@ func TestClaimPR_CreatesMovesAndGuardsActiveOwner(t *testing.T) { } pr.SessionID = second.ID - if _, err := s.ClaimPR(ctx, pr, nil, nil, nil, ports.ReviewWritePreserve, false); !errors.Is(err, ports.ErrPRClaimedByActiveSession) { + if _, err := s.ClaimPR(ctx, pr, nil, nil, nil, nil, ports.ReviewWritePreserve, false); !errors.Is(err, ports.ErrPRClaimedByActiveSession) { t.Fatalf("no-takeover err = %v, want ErrPRClaimedByActiveSession", err) } got, _, _ = s.GetPR(ctx, url) @@ -304,7 +304,7 @@ func TestClaimPR_CreatesMovesAndGuardsActiveOwner(t *testing.T) { t.Fatalf("active-owner refusal moved row to %s", got.SessionID) } - out, err = s.ClaimPR(ctx, pr, nil, nil, nil, ports.ReviewWritePreserve, true) + out, err = s.ClaimPR(ctx, pr, nil, nil, nil, nil, ports.ReviewWritePreserve, true) if err != nil { t.Fatalf("takeover: %v", err) } @@ -333,7 +333,7 @@ func TestClaimPRCreatedCDCUsesClaimReviewDecision(t *testing.T) { Review: domain.ReviewChangesRequest, UpdatedAt: time.Now().UTC(), } - if _, err := s.ClaimPR(ctx, pr, nil, nil, nil, ports.ReviewWritePreserve, true); err != nil { + if _, err := s.ClaimPR(ctx, pr, nil, nil, nil, nil, ports.ReviewWritePreserve, true); err != nil { t.Fatal(err) } @@ -361,7 +361,7 @@ func TestClaimPR_TakesOverTerminatedOwnerAndEmitsSessionChangedCDC(t *testing.T) second, _ := s.CreateSession(ctx, sampleRecord("mer")) url := "https://github.com/acme/repo/pull/99" pr := domain.PullRequest{URL: url, SessionID: first.ID, Number: 99, CI: domain.CIPassing, UpdatedAt: time.Now().UTC()} - if _, err := s.ClaimPR(ctx, pr, nil, nil, nil, ports.ReviewWritePreserve, true); err != nil { + if _, err := s.ClaimPR(ctx, pr, nil, nil, nil, nil, ports.ReviewWritePreserve, true); err != nil { t.Fatal(err) } first.IsTerminated = true @@ -371,7 +371,7 @@ func TestClaimPR_TakesOverTerminatedOwnerAndEmitsSessionChangedCDC(t *testing.T) } pr.SessionID = second.ID - out, err := s.ClaimPR(ctx, pr, nil, nil, nil, ports.ReviewWritePreserve, false) + out, err := s.ClaimPR(ctx, pr, nil, nil, nil, nil, ports.ReviewWritePreserve, false) if err != nil { t.Fatalf("terminated takeover: %v", err) } diff --git a/backend/internal/storage/sqlite/store/pr_facts_test.go b/backend/internal/storage/sqlite/store/pr_facts_test.go index dd2405acb..a9e201989 100644 --- a/backend/internal/storage/sqlite/store/pr_facts_test.go +++ b/backend/internal/storage/sqlite/store/pr_facts_test.go @@ -28,7 +28,7 @@ func TestListPRFactsForSessionProjectsAllPRsNewestFirst(t *testing.T) { // historical PR. Distinct updated_at so newest-first ordering is observable. write := func(pr domain.PullRequest) { t.Helper() - if err := s.WriteSCMObservation(ctx, pr, nil, nil, nil, ports.ReviewWritePreserve); err != nil { + if err := s.WriteSCMObservation(ctx, pr, nil, nil, nil, nil, ports.ReviewWritePreserve); err != nil { t.Fatalf("write %s: %v", pr.URL, err) } } diff --git a/backend/internal/storage/sqlite/store/pr_store.go b/backend/internal/storage/sqlite/store/pr_store.go index fc8147830..f3ec47f79 100644 --- a/backend/internal/storage/sqlite/store/pr_store.go +++ b/backend/internal/storage/sqlite/store/pr_store.go @@ -36,7 +36,7 @@ var ( // not touch pr_review_threads: those rows are owned by WriteSCMObservation's // slower review-thread refresh path. func (s *Store) WritePR(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, comments []domain.PullRequestComment) error { - return s.writePR(ctx, pr, checks, nil, comments, ports.ReviewWritePreserve, true) + return s.writePR(ctx, pr, checks, nil, nil, comments, ports.ReviewWritePreserve, true) } // WriteSCMObservation persists a provider-neutral SCM observation in one write @@ -44,14 +44,14 @@ func (s *Store) WritePR(ctx context.Context, pr domain.PullRequest, checks []dom // and comments are preserved, replaced, or merged according to reviewMode // because review polling runs at a slower and sometimes intentionally bounded // cadence than metadata/CI polling. -func (s *Store) WriteSCMObservation(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode) error { - return s.writePR(ctx, pr, checks, threads, comments, reviewMode, false) +func (s *Store) WriteSCMObservation(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, reviews []domain.PullRequestReview, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode) error { + return s.writePR(ctx, pr, checks, reviews, threads, comments, reviewMode, false) } // ClaimPR moves (or creates) a PR row to pr.SessionID and applies the live SCM // observation in the same transaction. The session_id update is what fires the // pr_session_changed CDC trigger added in migration 0005. -func (s *Store) ClaimPR(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode, allowActiveTakeover bool) (ports.ClaimOutcome, error) { +func (s *Store) ClaimPR(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, reviews []domain.PullRequestReview, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode, allowActiveTakeover bool) (ports.ClaimOutcome, error) { s.writeMu.Lock() defer s.writeMu.Unlock() var outcome ports.ClaimOutcome @@ -73,20 +73,20 @@ func (s *Store) ClaimPR(ctx context.Context, pr domain.PullRequest, checks []dom }); err != nil { return err } - return writePRRows(ctx, q, pr, checks, threads, comments, reviewMode, false, false) + return writePRRows(ctx, q, pr, checks, reviews, threads, comments, reviewMode, false, false) }) return outcome, err } -func (s *Store) writePR(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode, replaceLegacyComments bool) error { +func (s *Store) writePR(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, reviews []domain.PullRequestReview, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode, replaceLegacyComments bool) error { s.writeMu.Lock() defer s.writeMu.Unlock() return s.inTx(ctx, "write pr observation", func(q *gen.Queries) error { - return writePRRows(ctx, q, pr, checks, threads, comments, reviewMode, replaceLegacyComments, true) + return writePRRows(ctx, q, pr, checks, reviews, threads, comments, reviewMode, replaceLegacyComments, true) }) } -func writePRRows(ctx context.Context, q *gen.Queries, pr domain.PullRequest, checks []domain.PullRequestCheck, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode, replaceLegacyComments, rejectReassignment bool) error { +func writePRRows(ctx context.Context, q *gen.Queries, pr domain.PullRequest, checks []domain.PullRequestCheck, reviews []domain.PullRequestReview, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode, replaceLegacyComments, rejectReassignment bool) error { if rejectReassignment { existing, err := q.GetPR(ctx, pr.URL) if err != nil && !errors.Is(err, sql.ErrNoRows) { @@ -114,11 +114,21 @@ func writePRRows(ctx context.Context, q *gen.Queries, pr domain.PullRequest, che if err := q.DeletePRComments(ctx, pr.URL); err != nil { return err } + if err := q.DeletePRReviews(ctx, pr.URL); err != nil { + return err + } } else if replaceLegacyComments { if err := q.DeleteLegacyPRComments(ctx, pr.URL); err != nil { return err } } + if reviewMode == ports.ReviewWriteReplace || reviewMode == ports.ReviewWriteMerge { + for _, review := range reviews { + if err := q.UpsertPRReview(ctx, genReviewParams(pr.URL, review)); err != nil { + return fmt.Errorf("review %q: %w", review.ID, err) + } + } + } if reviewMode == ports.ReviewWriteReplace || reviewMode == ports.ReviewWriteMerge { for _, th := range threads { if err := q.UpsertPRReviewThread(ctx, genReviewThreadParams(pr.URL, th)); err != nil { @@ -282,6 +292,19 @@ func (s *Store) ListPRReviewThreads(ctx context.Context, prURL string) ([]domain return out, nil } +// ListPRReviews returns a PR's submitted reviews, oldest first. +func (s *Store) ListPRReviews(ctx context.Context, prURL string) ([]domain.PullRequestReview, error) { + rows, err := s.qr.ListPRReviews(ctx, prURL) + if err != nil { + return nil, fmt.Errorf("list pr reviews %s: %w", prURL, err) + } + out := make([]domain.PullRequestReview, 0, len(rows)) + for _, review := range rows { + out = append(out, reviewFromGen(review)) + } + return out, nil +} + // ---- domain <-> gen mapping ---- // prState collapses the PR's bools into the single pr.state column value. @@ -481,6 +504,33 @@ func reviewThreadFromGen(th gen.PRReviewThread) domain.PullRequestReviewThread { } } +func genReviewParams(prURL string, review domain.PullRequestReview) gen.UpsertPRReviewParams { + id := review.ID + if id == "" { + id = review.URL + } + return gen.UpsertPRReviewParams{ + PRURL: prURL, + ReviewID: id, + Author: review.Author, + State: string(reviewOrDefault(review.State)), + URL: review.URL, + IsBot: boolInt(review.IsBot), + SubmittedAt: review.SubmittedAt, + } +} + +func reviewFromGen(review gen.PRReview) domain.PullRequestReview { + return domain.PullRequestReview{ + ID: review.ReviewID, + Author: review.Author, + State: domain.ReviewDecision(review.State), + URL: review.URL, + IsBot: review.IsBot != 0, + SubmittedAt: review.SubmittedAt, + } +} + func boolInt(v bool) int64 { if v { return 1 diff --git a/backend/internal/storage/sqlite/store/store_test.go b/backend/internal/storage/sqlite/store/store_test.go index c6a564773..3ea986817 100644 --- a/backend/internal/storage/sqlite/store/store_test.go +++ b/backend/internal/storage/sqlite/store/store_test.go @@ -412,10 +412,11 @@ func TestWriteSCMObservationPersistsMetadataChecksReviewsAndComments(t *testing. UpdatedAt: now, ObservedAt: now, CIObservedAt: now, ReviewObservedAt: now, } checks := []domain.PullRequestCheck{{Name: "build", CommitHash: "h1", Status: domain.PRCheckFailed, Conclusion: "failure", URL: "ci", Details: "99", LogTail: "boom", CreatedAt: now}} + reviews := []domain.PullRequestReview{{ID: "review-1", Author: "reviewer", State: domain.ReviewChangesRequest, URL: "https://github.com/o/r/pull/1#pullrequestreview-1", SubmittedAt: now}} threads := []domain.PullRequestReviewThread{{ThreadID: "t1", Path: "main.go", Line: 7, SemanticHash: "th", UpdatedAt: now}} comments := []domain.PullRequestComment{{ThreadID: "t1", ID: "c1", Author: "reviewer", File: "main.go", Line: 7, Body: "fix", URL: "comment", CreatedAt: now}} - if err := s.WriteSCMObservation(ctx, pr, checks, threads, comments, ports.ReviewWriteReplace); err != nil { + if err := s.WriteSCMObservation(ctx, pr, checks, reviews, threads, comments, ports.ReviewWriteReplace); err != nil { t.Fatal(err) } got, ok, err := s.GetPR(ctx, pr.URL) @@ -433,6 +434,10 @@ func TestWriteSCMObservationPersistsMetadataChecksReviewsAndComments(t *testing. if len(gotThreads) != 1 || gotThreads[0].ThreadID != "t1" || gotThreads[0].SemanticHash != "th" { t.Fatalf("threads not persisted: %+v", gotThreads) } + gotReviews, _ := s.ListPRReviews(ctx, pr.URL) + if len(gotReviews) != 1 || gotReviews[0].ID != "review-1" || gotReviews[0].URL != "https://github.com/o/r/pull/1#pullrequestreview-1" { + t.Fatalf("reviews not persisted: %+v", gotReviews) + } gotComments, _ := s.ListPRComments(ctx, pr.URL) if len(gotComments) != 1 || gotComments[0].ThreadID != "t1" || gotComments[0].URL != "comment" { t.Fatalf("comments not persisted: %+v", gotComments) @@ -455,7 +460,7 @@ func TestWriteSCMObservationMergeUpdatesFetchedReviewWindow(t *testing.T) { {ThreadID: "older", ID: "older-c1", Author: "ann", Body: "old", CreatedAt: now}, {ThreadID: "latest", ID: "latest-c1", Author: "bob", Body: "before", CreatedAt: now}, } - if err := s.WriteSCMObservation(ctx, pr, nil, initialThreads, initialComments, ports.ReviewWriteReplace); err != nil { + if err := s.WriteSCMObservation(ctx, pr, nil, nil, initialThreads, initialComments, ports.ReviewWriteReplace); err != nil { t.Fatal(err) } @@ -467,7 +472,7 @@ func TestWriteSCMObservationMergeUpdatesFetchedReviewWindow(t *testing.T) { {ThreadID: "latest", ID: "latest-c2", Author: "bob", Body: "after", CreatedAt: now.Add(time.Second)}, {ThreadID: "new", ID: "new-c1", Author: "cat", Body: "new", CreatedAt: now.Add(time.Second)}, } - if err := s.WriteSCMObservation(ctx, pr, nil, mergedThreads, mergedComments, ports.ReviewWriteMerge); err != nil { + if err := s.WriteSCMObservation(ctx, pr, nil, nil, mergedThreads, mergedComments, ports.ReviewWriteMerge); err != nil { t.Fatal(err) } @@ -524,7 +529,7 @@ func TestWritePRPreservesSCMReviewThreads(t *testing.T) { threads := []domain.PullRequestReviewThread{{ThreadID: "t1", Path: "main.go", Line: 7, SemanticHash: "thread-v1", UpdatedAt: now}} comments := []domain.PullRequestComment{{ThreadID: "t1", ID: "c1", Author: "reviewer", Body: "scm", URL: "https://example/comment/c1", CreatedAt: now}} - if err := s.WriteSCMObservation(ctx, pr, nil, threads, comments, ports.ReviewWriteReplace); err != nil { + if err := s.WriteSCMObservation(ctx, pr, nil, nil, threads, comments, ports.ReviewWriteReplace); err != nil { t.Fatal(err) } legacyComments := []domain.PullRequestComment{ @@ -575,7 +580,7 @@ func TestWritePRPreservesSCMReviewThreads(t *testing.T) { mergedThreads := []domain.PullRequestReviewThread{{ThreadID: "t1", Path: "main.go", Line: 8, Resolved: true, SemanticHash: "thread-v2", UpdatedAt: now.Add(2 * time.Second)}} mergedComments := []domain.PullRequestComment{{ThreadID: "t1", ID: "c2", Author: "reviewer", Body: "updated", URL: "https://example/comment/c2", CreatedAt: now.Add(2 * time.Second)}} - if err := s.WriteSCMObservation(ctx, pr, nil, mergedThreads, mergedComments, ports.ReviewWriteMerge); err != nil { + if err := s.WriteSCMObservation(ctx, pr, nil, nil, mergedThreads, mergedComments, ports.ReviewWriteMerge); err != nil { t.Fatal(err) } gotComments, err = s.ListPRComments(ctx, pr.URL) diff --git a/frontend/src/api/schema.ts b/frontend/src/api/schema.ts index 93d2a7395..8a3b89818 100644 --- a/frontend/src/api/schema.ts +++ b/frontend/src/api/schema.ts @@ -839,7 +839,9 @@ export interface components { }; SessionPRUnresolvedReviewer: { count: number; + isBot?: boolean; links: components["schemas"]["SessionPRReviewCommentLink"][]; + reviewUrl?: string; reviewerId: string; }; SessionPreviewResponse: { diff --git a/frontend/src/renderer/__tests__/integration/pr-hydration.test.tsx b/frontend/src/renderer/__tests__/integration/pr-hydration.test.tsx index d151c40b6..4411716be 100644 --- a/frontend/src/renderer/__tests__/integration/pr-hydration.test.tsx +++ b/frontend/src/renderer/__tests__/integration/pr-hydration.test.tsx @@ -73,6 +73,100 @@ function respondWithProjectAndPRs() { }); } +function respondWithAttentionPR() { + getMock.mockImplementation(async (url: string) => { + if (url === "/api/v1/projects") { + return { data: { projects: [{ id: "proj-1", name: "my-app", path: "/repo/my-app" }] }, error: undefined }; + } + if (url === "/api/v1/sessions/sess-1/pr") { + return { + data: { + sessionId: "sess-1", + prs: [ + { + url: "https://github.com/aoagents/ReverbCode/pull/278", + htmlUrl: "https://github.com/aoagents/ReverbCode/pull/278", + number: 278, + title: "fix the bug", + state: "open", + provider: "github", + repo: "aoagents/ReverbCode", + author: "worker", + sourceBranch: "fix/bug", + targetBranch: "main", + headSha: "abc123", + additions: 1, + deletions: 1, + changedFiles: 1, + ci: { state: "passing", failingChecks: [] }, + review: { + decision: "changes_requested", + hasUnresolvedHumanComments: true, + unresolvedBy: [ + { + reviewerId: "reviewer-a", + count: 1, + reviewUrl: "https://github.com/aoagents/ReverbCode/pull/278#pullrequestreview-1", + links: [ + { + url: "https://github.com/aoagents/ReverbCode/pull/278#discussion_r1", + file: "main.go", + line: 12, + }, + ], + }, + ], + }, + mergeability: { + state: "conflicting", + reasons: ["conflicts"], + prUrl: "https://github.com/aoagents/ReverbCode/pull/278", + conflictFiles: [], + }, + updatedAt: "2026-06-10T16:15:04Z", + observedAt: "2026-06-10T16:15:04Z", + ciObservedAt: "2026-06-10T16:15:04Z", + reviewObservedAt: "2026-06-10T16:15:04Z", + }, + ], + }, + error: undefined, + }; + } + if (url === "/api/v1/sessions") { + return { + data: { + sessions: [ + { + id: "sess-1", + projectId: "proj-1", + displayName: "fix the bug", + harness: "claude-code", + status: "changes_requested", + isTerminated: false, + updatedAt: "2026-06-10T16:15:04Z", + prs: [ + { + number: 278, + state: "open", + url: "https://github.com/aoagents/ReverbCode/pull/278", + ci: "passing", + review: "changes_requested", + mergeability: "conflicting", + reviewComments: true, + updatedAt: "2026-06-10T16:15:04Z", + }, + ], + }, + ], + }, + error: undefined, + }; + } + throw new Error(`unexpected GET ${url}`); + }); +} + function renderWithProviders(node: ReactNode) { const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); render({node}); @@ -93,6 +187,20 @@ describe("PR hydration for a normal project (#251)", () => { expect(screen.queryByText("no PR yet")).not.toBeInTheDocument(); }); + it("links Board attention states to PR fallback and merge conflict targets", async () => { + respondWithAttentionPR(); + renderWithProviders(); + + expect(await screen.findByRole("link", { name: "PR" })).toHaveAttribute( + "href", + "https://github.com/aoagents/ReverbCode/pull/278", + ); + expect(screen.getByRole("link", { name: "conflicts" })).toHaveAttribute( + "href", + "https://github.com/aoagents/ReverbCode/pull/278/conflicts", + ); + }); + it("lists every session PR on the PR page instead of being empty", async () => { renderWithProviders(); diff --git a/frontend/src/renderer/components/SessionInspector.tsx b/frontend/src/renderer/components/SessionInspector.tsx index 02c69f690..e7ae9d64d 100644 --- a/frontend/src/renderer/components/SessionInspector.tsx +++ b/frontend/src/renderer/components/SessionInspector.tsx @@ -6,7 +6,7 @@ import { apiClient, apiErrorMessage } from "../lib/api-client"; import { workspaceQueryKey } from "../hooks/useWorkspaceQuery"; import { formatTimeCompact } from "../lib/format-time"; import { useSessionScmSummary, type SessionPRSummary } from "../hooks/useSessionScmSummary"; -import { prStatusRows, sessionPRDisplaySummaries, type PRDisplayTone } from "../lib/pr-display"; +import { prBrowserUrl, prStatusRows, sessionPRDisplaySummaries, type PRDisplayTone } from "../lib/pr-display"; import type { SessionStatus, WorkspaceSession } from "../types/workspace"; import { sortedPRs, workerDisplayStatus } from "../types/workspace"; import { BrowserPanelView } from "./BrowserPanel"; @@ -210,7 +210,7 @@ function PRSummaryCard({ pr }: { pr: SessionPRSummary }) { {pr.state} ) => { + if (event.target !== event.currentTarget) { + return; + } + if (event.key === "Enter" || event.key === " ") { + event.preventDefault(); + onOpen(); + } + }; return ( - + ); } @@ -316,7 +327,7 @@ function BoardPRSummary({ className, pr }: { className?: string; pr: SessionPRSu {diffSummary ? {diffSummary} : null} - + ); } diff --git a/frontend/src/renderer/lib/pr-display.test.ts b/frontend/src/renderer/lib/pr-display.test.ts index fd50f591a..3a904f5ec 100644 --- a/frontend/src/renderer/lib/pr-display.test.ts +++ b/frontend/src/renderer/lib/pr-display.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; import type { SessionPRSummary } from "../hooks/useSessionScmSummary"; -import { prAttentionItems, prDiffSummary, prStatusRows } from "./pr-display"; +import { prAttentionItems, prBrowserUrl, prDiffSummary, prStatusRows } from "./pr-display"; const summary = (overrides: Partial = {}): SessionPRSummary => ({ url: "https://github.com/acme/repo/pull/7", @@ -56,6 +56,19 @@ describe("prDiffSummary", () => { }); }); +describe("prBrowserUrl", () => { + it("normalizes issue-shaped GitHub PR URLs to the pull request page", () => { + expect( + prBrowserUrl( + summary({ + url: "https://github.com/acme/repo/issues/7", + htmlUrl: "https://github.com/acme/repo/issues/7", + }), + ), + ).toBe("https://github.com/acme/repo/pull/7"); + }); +}); + describe("prAttentionItems", () => { it("returns no attention for clean open PRs", () => { expect(prAttentionItems(summary())).toEqual([]); @@ -100,6 +113,150 @@ describe("prAttentionItems", () => { }); }); + it("links failing CI checks to their provider URLs", () => { + const items = prAttentionItems( + summary({ + ci: { + state: "failing", + failingChecks: [ + { name: "unit", status: "failed", conclusion: "failure", url: "https://checks.example/unit" }, + { name: "lint", status: "failed", conclusion: "failure", url: "https://checks.example/lint" }, + { name: "build", status: "failed", conclusion: "failure", url: "https://checks.example/build" }, + { name: "types", status: "failed", conclusion: "failure", url: "https://checks.example/types" }, + ], + }, + }), + ); + + const ciItem = items.find((item) => item.kind === "ci_failing"); + expect(ciItem?.links).toEqual([ + { label: "unit", href: "https://checks.example/unit", title: "failure" }, + { label: "lint", href: "https://checks.example/lint", title: "failure" }, + { label: "build", href: "https://checks.example/build", title: "failure" }, + ]); + expect(ciItem?.overflowLabel).toBe("+1 check"); + }); + + it("prefers the submitted review summary over inline comments", () => { + const items = prAttentionItems( + summary({ + review: { + decision: "changes_requested", + hasUnresolvedHumanComments: true, + unresolvedBy: [ + { + reviewerId: "alice", + count: 2, + reviewUrl: "https://github.com/acme/repo/pull/7#pullrequestreview-1", + links: [ + { url: "https://github.com/acme/repo/pull/7#discussion_r1", file: "main.go", line: 12 }, + { url: "https://github.com/acme/repo/pull/7#discussion_r2", file: "test.go", line: 20 }, + ], + }, + ], + }, + }), + ); + + expect(items.find((item) => item.kind === "review_changes_requested")?.links[0]).toMatchObject({ + label: "alice +1", + href: "https://github.com/acme/repo/pull/7#pullrequestreview-1", + title: "Open requested-changes review from alice", + }); + }); + + it("falls back to the first inline comment when no review summary exists", () => { + const items = prAttentionItems( + summary({ + review: { + decision: "changes_requested", + hasUnresolvedHumanComments: true, + unresolvedBy: [ + { + reviewerId: "alice", + count: 2, + links: [ + { url: "https://github.com/acme/repo/pull/7#discussion_r1", file: "main.go", line: 12 }, + { url: "https://github.com/acme/repo/pull/7#discussion_r2", file: "test.go", line: 20 }, + ], + }, + ], + }, + }), + ); + + expect(items.find((item) => item.kind === "review_changes_requested")?.links[0]).toMatchObject({ + label: "alice +1", + href: "https://github.com/acme/repo/pull/7#discussion_r1", + title: "2 unresolved comments from alice", + }); + }); + + it("falls back to the PR page when review summary and inline comment URLs are missing", () => { + const items = prAttentionItems( + summary({ + url: "https://github.com/acme/repo/issues/7", + htmlUrl: "https://github.com/acme/repo/issues/7", + review: { + decision: "changes_requested", + hasUnresolvedHumanComments: true, + unresolvedBy: [{ reviewerId: "alice", count: 1, links: [] }], + }, + }), + ); + + expect(items.find((item) => item.kind === "review_changes_requested")?.links[0]).toMatchObject({ + label: "alice", + href: "https://github.com/acme/repo/pull/7", + title: "Open pull request for alice", + }); + }); + + it("shows bot reviewers with a bot label", () => { + const items = prAttentionItems( + summary({ + review: { + decision: "changes_requested", + hasUnresolvedHumanComments: false, + unresolvedBy: [ + { + reviewerId: "copilot", + count: 0, + reviewUrl: "https://github.com/acme/repo/pull/7#pullrequestreview-2", + isBot: true, + links: [], + }, + ], + }, + }), + ); + + expect(items.find((item) => item.kind === "review_changes_requested")?.links[0]).toMatchObject({ + label: "copilot bot", + href: "https://github.com/acme/repo/pull/7#pullrequestreview-2", + title: "Open requested-changes review from copilot bot", + }); + }); + + it("links merge conflicts to GitHub's conflict resolution page", () => { + const items = prAttentionItems( + summary({ + url: "https://github.com/acme/repo/issues/7", + htmlUrl: "https://github.com/acme/repo/issues/7", + mergeability: { + state: "conflicting", + reasons: [], + prUrl: "https://github.com/acme/repo/issues/7", + }, + }), + ); + + expect(items.find((item) => item.kind === "merge_conflict")?.links[0]).toMatchObject({ + label: "conflicts", + href: "https://github.com/acme/repo/pull/7/conflicts", + }); + }); + it("suppresses attention once the PR is closed or merged", () => { expect( prAttentionItems( diff --git a/frontend/src/renderer/lib/pr-display.ts b/frontend/src/renderer/lib/pr-display.ts index b533c0d46..7ce2c3f45 100644 --- a/frontend/src/renderer/lib/pr-display.ts +++ b/frontend/src/renderer/lib/pr-display.ts @@ -46,6 +46,10 @@ export function comparePRDisplaySummaries(a: SessionPRSummary, b: SessionPRSumma return prStateRank[a.state] - prStateRank[b.state] || a.number - b.number; } +export function prBrowserUrl(pr: SessionPRSummary): string { + return prBaseUrl(pr) ?? pr.htmlUrl ?? pr.url; +} + export function sessionPRDisplaySummaries( session: WorkspaceSession, summaries: SessionPRSummary[] = [], @@ -175,11 +179,10 @@ export function prAttentionItems(pr: SessionPRSummary): PRAttentionItem[] { } if (pr.review.decision === "changes_requested" || pr.review.hasUnresolvedHumanComments) { const reviewers = pr.review.unresolvedBy.slice(0, 3); - const links = reviewers.map((reviewer) => ({ - label: reviewerLabel(reviewer), - href: reviewer.links.find((link) => link.url)?.url || undefined, - title: `${reviewer.count} unresolved ${pluralize("comment", reviewer.count)}`, - })); + const links = reviewers.map((reviewer) => reviewAttentionLink(pr, reviewer)); + if (links.length === 0 && pr.review.decision === "changes_requested") { + links.push({ label: "PR", href: prBrowserUrl(pr), title: "Open pull request" }); + } items.push({ kind: "review_changes_requested", title: "Address requested changes", @@ -331,18 +334,23 @@ function mergeAttention( fallback: string, tone: PRDisplayTone, ): PRAttentionItem { + const href = + kind === "merge_conflict" ? mergeConflictUrl(pr) : pr.mergeability.prUrl || pr.htmlUrl || pr.url || undefined; const fileLinks = (pr.mergeability.conflictFiles ?? []).slice(0, 3).map((file) => ({ label: file.path, - href: file.url || pr.mergeability.prUrl || undefined, + href: file.url || href, + title: kind === "merge_conflict" ? "Open merge conflicts" : undefined, })); const reasonLinks = - fileLinks.length > 0 + fileLinks.length > 0 || kind === "merge_conflict" ? [] : pr.mergeability.reasons.slice(0, 3).map((reason) => ({ label: mergeReasonLabel(reason), - href: pr.mergeability.prUrl || undefined, + href, })); - const links = fileLinks.length > 0 ? fileLinks : reasonLinks; + const fallbackLink = + kind === "merge_conflict" && href ? [{ label: "conflicts", href, title: "Open merge conflicts" }] : []; + const links = fileLinks.length > 0 ? fileLinks : reasonLinks.length > 0 ? reasonLinks : fallbackLink; return { kind, title, @@ -356,11 +364,78 @@ function mergeAttention( }; } -function reviewerLabel(reviewer: SessionPRSummary["review"]["unresolvedBy"][number]): string { - if (reviewer.count <= 1) { - return reviewer.reviewerId; +function mergeConflictUrl(pr: SessionPRSummary): string | undefined { + return prSubpageUrl(pr, "conflicts") ?? pr.mergeability.prUrl ?? prBrowserUrl(pr); +} + +function prBaseUrl(pr: SessionPRSummary): string | undefined { + return prURL(pr); +} + +function prSubpageUrl(pr: SessionPRSummary, subpage: "conflicts"): string | undefined { + const base = prURL(pr); + return base ? `${base}/${subpage}` : undefined; +} + +function prURL(pr: SessionPRSummary): string | undefined { + const raw = pr.htmlUrl || pr.mergeability.prUrl || pr.url; + if (!raw) { + return undefined; } - return `${reviewer.reviewerId} +${reviewer.count - 1}`; + try { + const url = new URL(raw); + const match = url.pathname.match(/^(\/[^/]+\/[^/]+)\/(?:pull|issues)\/(\d+)(?:\/.*)?$/); + if (!match) { + return undefined; + } + url.pathname = `${match[1]}/pull/${match[2]}`; + url.search = ""; + url.hash = ""; + return url.toString(); + } catch { + return undefined; + } +} + +function reviewerLabel(reviewer: SessionPRSummary["review"]["unresolvedBy"][number]): string { + const name = reviewerDisplayName(reviewer); + if (reviewer.count <= 1) { + return name; + } + return `${name} +${reviewer.count - 1}`; +} + +function reviewerDisplayName(reviewer: SessionPRSummary["review"]["unresolvedBy"][number]): string { + return reviewer.isBot ? `${reviewer.reviewerId} bot` : reviewer.reviewerId; +} + +function reviewAttentionLink( + pr: SessionPRSummary, + reviewer: SessionPRSummary["review"]["unresolvedBy"][number], +): PRAttentionLink { + const inlineURL = reviewer.links.find((link) => link.url)?.url; + if (reviewer.reviewUrl) { + return { + label: reviewerLabel(reviewer), + href: reviewer.reviewUrl, + title: `Open requested-changes review from ${reviewerDisplayName(reviewer)}`, + }; + } + if (inlineURL) { + return { + label: reviewerLabel(reviewer), + href: inlineURL, + title: + reviewer.count > 0 + ? `${reviewer.count} unresolved ${pluralize("comment", reviewer.count)} from ${reviewerDisplayName(reviewer)}` + : `Open review comments from ${reviewerDisplayName(reviewer)}`, + }; + } + return { + label: reviewerLabel(reviewer), + href: prBrowserUrl(pr), + title: `Open pull request for ${reviewerDisplayName(reviewer)}`, + }; } function mergeReasonLabel(reason: string): string {