Skip to content

Commit a31cf1b

Browse files
fix: deep-link PR attention actions (#2200)
* fix: deep-link PR attention actions * chore: format with prettier [skip ci] --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 parent 8241868 commit a31cf1b

28 files changed

Lines changed: 833 additions & 87 deletions

File tree

backend/internal/adapters/scm/github/observer_provider.go

Lines changed: 52 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ const (
3333
githubReviewCommentLimitPerThread = 5
3434
// githubReviewThreadMaxPages bounds the explicit older-thread fallback.
3535
githubReviewThreadMaxPages = 2
36+
// githubReviewSummaryLimit bounds submitted decisive reviews used for summary links.
37+
githubReviewSummaryLimit = 20
3638
)
3739

3840
// 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
156158

157159
// FetchReviewThreads fetches review threads separately from the fast PR/CI path.
158160
func (p *Provider) FetchReviewThreads(ctx context.Context, ref ports.SCMPRRef) (ports.SCMReviewObservation, error) {
159-
latest, decision, pi, err := p.fetchReviewThreadPage(ctx, ref, "")
161+
latest, reviews, decision, pi, err := p.fetchReviewThreadPage(ctx, ref, "", true)
160162
if err != nil {
161163
return ports.SCMReviewObservation{}, err
162164
}
163165
if !boolv(pi["hasPreviousPage"]) {
164-
return ports.SCMReviewObservation{Decision: decision, Threads: latest}, nil
166+
return ports.SCMReviewObservation{Decision: decision, Reviews: reviews, Threads: latest}, nil
165167
}
166168
out := latest
167169
startCursor := str(pi["startCursor"])
@@ -175,7 +177,7 @@ func (p *Provider) FetchReviewThreads(ctx context.Context, ref ports.SCMPRRef) (
175177
p.logger.Warn("github scm: review thread page is partial but missing start cursor",
176178
"repo", repoFullName(ref.Repo), "pr", ref.Number)
177179
} else {
178-
older, _, olderPI, err := p.fetchReviewThreadPage(ctx, ref, startCursor)
180+
older, _, _, olderPI, err := p.fetchReviewThreadPage(ctx, ref, startCursor, false)
179181
if err != nil {
180182
return ports.SCMReviewObservation{}, err
181183
}
@@ -190,7 +192,7 @@ func (p *Provider) FetchReviewThreads(ctx context.Context, ref ports.SCMPRRef) (
190192
}
191193
}
192194
}
193-
return ports.SCMReviewObservation{Decision: decision, Threads: out, Partial: true}, nil
195+
return ports.SCMReviewObservation{Decision: decision, Reviews: reviews, Threads: out, Partial: true}, nil
194196
}
195197

196198
type restListPull struct {
@@ -518,38 +520,76 @@ func mergeabilityObservation(providerMergeable, providerMergeState, ci, review s
518520
return out
519521
}
520522

521-
func (p *Provider) fetchReviewThreadPage(ctx context.Context, ref ports.SCMPRRef, beforeCursor string) ([]ports.SCMReviewThreadObservation, string, map[string]any, error) {
522-
query := buildReviewThreadsQuery(ref, beforeCursor)
523+
func (p *Provider) fetchReviewThreadPage(ctx context.Context, ref ports.SCMPRRef, beforeCursor string, includeReviews bool) ([]ports.SCMReviewThreadObservation, []ports.SCMReviewSummaryObservation, string, map[string]any, error) {
524+
query := buildReviewThreadsQuery(ref, beforeCursor, includeReviews)
523525
data, err := p.client.doGraphQL(ctx, query, nil)
524526
if err != nil {
525-
return nil, "", nil, err
527+
return nil, nil, "", nil, err
526528
}
527529
repoData, _ := data["repo"].(map[string]any)
528530
pr, _ := repoData["pullRequest"].(map[string]any)
529531
if pr == nil {
530-
return nil, "", nil, fmt.Errorf("%w: pull request not found in review response", ErrNotFound)
532+
return nil, nil, "", nil, fmt.Errorf("%w: pull request not found in review response", ErrNotFound)
531533
}
532534
decision := string(reviewDecisionFromGraphQL(pr))
535+
reviewSummaries, _ := pr["reviewSummaries"].(map[string]any)
536+
reviews := make([]ports.SCMReviewSummaryObservation, 0, len(nodes(reviewSummaries["nodes"])))
537+
for _, review := range nodes(reviewSummaries["nodes"]) {
538+
summary := scmReviewSummaryFromGraphQL(review)
539+
if summary.ID == "" && summary.URL == "" {
540+
continue
541+
}
542+
reviews = append(reviews, summary)
543+
}
533544
threads, _ := pr["reviewThreads"].(map[string]any)
534545
out := make([]ports.SCMReviewThreadObservation, 0, len(nodes(threads["nodes"])))
535546
for _, th := range nodes(threads["nodes"]) {
536547
out = append(out, scmThreadFromGraphQL(th))
537548
}
538549
pi, _ := threads["pageInfo"].(map[string]any)
539-
return out, decision, pi, nil
550+
return out, reviews, decision, pi, nil
540551
}
541552

542-
func buildReviewThreadsQuery(ref ports.SCMPRRef, beforeCursor string) string {
553+
func buildReviewThreadsQuery(ref ports.SCMPRRef, beforeCursor string, includeReviews bool) string {
543554
before := "null"
544555
if beforeCursor != "" {
545556
before = graphQLString(beforeCursor)
546557
}
558+
reviewSelection := ""
559+
if includeReviews {
560+
reviewSelection = fmt.Sprintf(" reviewSummaries: reviews(last:%d, states:[APPROVED,CHANGES_REQUESTED]){ nodes{ id state url submittedAt author{ login __typename } } }", githubReviewSummaryLimit)
561+
}
547562
return fmt.Sprintf(`query{
548-
repo: repository(owner:%s,name:%s){ pullRequest(number:%d){ reviewDecision reviewThreads(last:%d, before:%s){ nodes{
563+
repo: repository(owner:%s,name:%s){ pullRequest(number:%d){ reviewDecision%s reviewThreads(last:%d, before:%s){ nodes{
549564
id isResolved path line
550565
comments(first:%d){ nodes{ id body url author{ login __typename } } }
551566
} pageInfo{ hasPreviousPage startCursor } } } }
552-
}`, graphQLString(ref.Repo.Owner), graphQLString(ref.Repo.Name), ref.Number, githubReviewThreadPageSize, before, githubReviewCommentLimitPerThread)
567+
}`, graphQLString(ref.Repo.Owner), graphQLString(ref.Repo.Name), ref.Number, reviewSelection, githubReviewThreadPageSize, before, githubReviewCommentLimitPerThread)
568+
}
569+
570+
func scmReviewSummaryFromGraphQL(review map[string]any) ports.SCMReviewSummaryObservation {
571+
author, _ := review["author"].(map[string]any)
572+
return ports.SCMReviewSummaryObservation{
573+
ID: str(review["id"]),
574+
Author: str(author["login"]),
575+
State: string(reviewStateFromGraphQL(review["state"])),
576+
URL: str(review["url"]),
577+
IsBot: isBotAuthor(author),
578+
SubmittedAt: parseGitHubTime(str(review["submittedAt"])),
579+
}
580+
}
581+
582+
func reviewStateFromGraphQL(state any) domain.ReviewDecision {
583+
switch strings.ToUpper(strings.TrimSpace(str(state))) {
584+
case "APPROVED":
585+
return domain.ReviewApproved
586+
case "CHANGES_REQUESTED":
587+
return domain.ReviewChangesRequest
588+
case "REVIEW_REQUIRED":
589+
return domain.ReviewRequired
590+
default:
591+
return domain.ReviewNone
592+
}
553593
}
554594

555595
func scmThreadFromGraphQL(th map[string]any) ports.SCMReviewThreadObservation {

backend/internal/adapters/scm/github/provider_test.go

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1369,15 +1369,27 @@ func TestFetchReviewThreadsUsesLatestWindowWithoutFallbackWhenOldestResolved(t *
13691369
if !strings.Contains(string(body), "reviewThreads(last:50, before:null)") {
13701370
t.Fatalf("review query should fetch latest 50, body=%s", body)
13711371
}
1372+
if !strings.Contains(string(body), "reviews(last:20, states:[APPROVED,CHANGES_REQUESTED])") {
1373+
t.Fatalf("review query should fetch decisive review summaries, body=%s", body)
1374+
}
13721375
if !strings.Contains(string(body), "comments(first:5)") {
13731376
t.Fatalf("review query should cap comments per thread, body=%s", body)
13741377
}
13751378
w.Header().Set("Content-Type", "application/json")
13761379
_ = json.NewEncoder(w).Encode(map[string]any{
13771380
"data": map[string]any{"repo": map[string]any{"pullRequest": map[string]any{
13781381
"reviewDecision": "CHANGES_REQUESTED",
1382+
"reviewSummaries": map[string]any{"nodes": []any{map[string]any{
1383+
"id": "review-1",
1384+
"state": "CHANGES_REQUESTED",
1385+
"url": "https://github.com/o/r/pull/1#pullrequestreview-1",
1386+
"submittedAt": "2026-06-15T00:00:00Z",
1387+
"author": map[string]any{"login": "alice", "__typename": "User"},
1388+
}}},
13791389
"reviewThreads": map[string]any{
1380-
"nodes": []any{map[string]any{"id": "latest-resolved", "path": "main.go", "line": 1, "isResolved": true, "comments": map[string]any{"nodes": []any{}}}},
1390+
"nodes": []any{map[string]any{"id": "latest-resolved", "path": "main.go", "line": 1, "isResolved": true, "comments": map[string]any{"nodes": []any{map[string]any{
1391+
"id": "comment-1", "body": "fix", "url": "https://github.com/o/r/pull/1#discussion_r1", "author": map[string]any{"login": "alice", "__typename": "User"},
1392+
}}}}},
13811393
"pageInfo": map[string]any{"hasPreviousPage": true, "startCursor": "latest-start"},
13821394
},
13831395
}}},
@@ -1397,6 +1409,12 @@ func TestFetchReviewThreadsUsesLatestWindowWithoutFallbackWhenOldestResolved(t *
13971409
if len(review.Threads) != 1 || review.Threads[0].ID != "latest-resolved" {
13981410
t.Fatalf("threads = %#v", review.Threads)
13991411
}
1412+
if len(review.Reviews) != 1 || review.Reviews[0].Author != "alice" || review.Reviews[0].URL != "https://github.com/o/r/pull/1#pullrequestreview-1" {
1413+
t.Fatalf("reviews = %#v", review.Reviews)
1414+
}
1415+
if len(review.Threads[0].Comments) != 1 || review.Threads[0].Comments[0].URL != "https://github.com/o/r/pull/1#discussion_r1" {
1416+
t.Fatalf("thread comments = %#v", review.Threads[0].Comments)
1417+
}
14001418
}
14011419

14021420
func TestFetchReviewThreadsFetchesOneOlderPageWhenOldestUnresolved(t *testing.T) {

backend/internal/domain/pr.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,16 @@ type PullRequestReviewThread struct {
107107
UpdatedAt time.Time
108108
}
109109

110+
// PullRequestReview is one submitted provider review for a pull request.
111+
type PullRequestReview struct {
112+
ID string
113+
Author string
114+
State ReviewDecision
115+
URL string
116+
IsBot bool
117+
SubmittedAt time.Time
118+
}
119+
110120
// CIState is the aggregate CI status of a PR.
111121
type CIState string
112122

backend/internal/httpd/apispec/openapi.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2371,10 +2371,14 @@ components:
23712371
properties:
23722372
count:
23732373
type: integer
2374+
isBot:
2375+
type: boolean
23742376
links:
23752377
items:
23762378
$ref: '#/components/schemas/SessionPRReviewCommentLink'
23772379
type: array
2380+
reviewUrl:
2381+
type: string
23782382
reviewerId:
23792383
type: string
23802384
required:

backend/internal/httpd/controllers/dto.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,8 @@ type SessionPRUnresolvedReviewer struct {
295295
ReviewerID string `json:"reviewerId"`
296296
Count int `json:"count"`
297297
Links []SessionPRReviewCommentLink `json:"links"`
298+
ReviewURL string `json:"reviewUrl,omitempty"`
299+
IsBot bool `json:"isBot,omitempty"`
298300
}
299301

300302
// SessionPRReviewCommentLink points to one unresolved review comment.
@@ -366,7 +368,7 @@ func newSessionPRReviewSummary(in sessionsvc.PRReviewSummary) SessionPRReviewSum
366368
for _, link := range reviewer.Links {
367369
links = append(links, SessionPRReviewCommentLink{URL: link.URL, File: link.File, Line: link.Line})
368370
}
369-
reviewers = append(reviewers, SessionPRUnresolvedReviewer{ReviewerID: reviewer.ReviewerID, Count: reviewer.Count, Links: links})
371+
reviewers = append(reviewers, SessionPRUnresolvedReviewer{ReviewerID: reviewer.ReviewerID, Count: reviewer.Count, Links: links, ReviewURL: reviewer.ReviewURL, IsBot: reviewer.IsBot})
370372
}
371373
return SessionPRReviewSummary{Decision: in.Decision, HasUnresolvedHumanComments: in.HasUnresolvedHumanComments, UnresolvedBy: reviewers}
372374
}

backend/internal/httpd/controllers/sessions_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,7 @@ func (f *fakeSessionService) ListPRSummaries(_ context.Context, id domain.Sessio
192192
UnresolvedBy: []sessionsvc.PRUnresolvedReviewer{{
193193
ReviewerID: "reviewer-a",
194194
Count: 1,
195+
ReviewURL: "https://github.com/aoagents/agent-orchestrator/pull/142#pullrequestreview-1",
195196
Links: []sessionsvc.PRReviewCommentLink{{URL: "https://github.com/aoagents/agent-orchestrator/pull/142#discussion_r1", File: "main.go", Line: 12}},
196197
}},
197198
},
@@ -844,6 +845,7 @@ func TestSessionsAPI_PRRoutes(t *testing.T) {
844845
UnresolvedBy []struct {
845846
ReviewerID string `json:"reviewerId"`
846847
Count int `json:"count"`
848+
ReviewURL string `json:"reviewUrl"`
847849
Links []struct {
848850
URL string `json:"url"`
849851
File string `json:"file"`
@@ -869,7 +871,7 @@ func TestSessionsAPI_PRRoutes(t *testing.T) {
869871
if checks := listed.PRs[0].CI.FailingChecks; len(checks) != 1 || checks[0].Name != "unit" || checks[0].LogTail != "" {
870872
t.Fatalf("failing checks = %#v", checks)
871873
}
872-
if reviewers := listed.PRs[0].Review.UnresolvedBy; len(reviewers) != 1 || reviewers[0].ReviewerID != "reviewer-a" || reviewers[0].Links[0].Body != "" {
874+
if reviewers := listed.PRs[0].Review.UnresolvedBy; len(reviewers) != 1 || reviewers[0].ReviewerID != "reviewer-a" || reviewers[0].ReviewURL == "" || reviewers[0].Links[0].Body != "" {
873875
t.Fatalf("reviewers = %#v", reviewers)
874876
}
875877
if merge := listed.PRs[0].Mergeability; merge.State != "conflicting" || len(merge.ConflictFiles) != 0 || merge.PRURL == "" {

backend/internal/observe/scm/observer.go

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ type Store interface {
5353
UpsertProject(ctx context.Context, row domain.ProjectRecord) error
5454
ListPRsBySession(ctx context.Context, sessionID domain.SessionID) ([]domain.PullRequest, error)
5555
ListChecks(ctx context.Context, prURL string) ([]domain.PullRequestCheck, error)
56-
WriteSCMObservation(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode) error
56+
WriteSCMObservation(ctx context.Context, pr domain.PullRequest, checks []domain.PullRequestCheck, reviews []domain.PullRequestReview, threads []domain.PullRequestReviewThread, comments []domain.PullRequestComment, reviewMode ports.ReviewWriteMode) error
5757
}
5858

5959
// Lifecycle is the provider-neutral lifecycle notification sink.
@@ -344,8 +344,8 @@ func (o *Observer) Poll(ctx context.Context) error {
344344
prRefreshOK[key] = true
345345
continue
346346
}
347-
finalPR, finalChecks, finalThreads, finalComments := domainFromObservation(subj.session.ID, prepared, local, opts, now)
348-
pr, checks, threads, comments := finalPR, finalChecks, finalThreads, finalComments
347+
finalPR, finalChecks, finalReviews, finalThreads, finalComments := domainFromObservation(subj.session.ID, prepared, local, opts, now)
348+
pr, checks, reviews, threads, comments := finalPR, finalChecks, finalReviews, finalThreads, finalComments
349349
// Lifecycle is allowed to run only after the observed facts are durable,
350350
// but semantic hashes are the observer's acknowledgement cursor. Keep
351351
// changed hashes at their local values until lifecycle succeeds; if the
@@ -362,9 +362,9 @@ func (o *Observer) Poll(ctx context.Context) error {
362362
if prepared.Changed.Review {
363363
pendingOpts.preserveLocalReviewHash = true
364364
}
365-
pr, checks, threads, comments = domainFromObservation(subj.session.ID, prepared, local, pendingOpts, now)
365+
pr, checks, reviews, threads, comments = domainFromObservation(subj.session.ID, prepared, local, pendingOpts, now)
366366
}
367-
if err := o.store.WriteSCMObservation(ctx, pr, checks, threads, comments, reviewMode); err != nil {
367+
if err := o.store.WriteSCMObservation(ctx, pr, checks, reviews, threads, comments, reviewMode); err != nil {
368368
o.logger.Error("scm observer: DB write failed", "session", subj.session.ID, "pr", pr.URL, "err", err)
369369
markRepoRefreshFailed(subj.repo)
370370
continue
@@ -375,7 +375,7 @@ func (o *Observer) Poll(ctx context.Context) error {
375375
markRepoRefreshFailed(subj.repo)
376376
continue
377377
}
378-
if err := o.store.WriteSCMObservation(ctx, finalPR, finalChecks, nil, nil, ports.ReviewWritePreserve); err != nil {
378+
if err := o.store.WriteSCMObservation(ctx, finalPR, finalChecks, nil, nil, nil, ports.ReviewWritePreserve); err != nil {
379379
o.logger.Error("scm observer: DB lifecycle acknowledgement failed", "session", subj.session.ID, "pr", finalPR.URL, "err", err)
380380
markRepoRefreshFailed(subj.repo)
381381
continue
@@ -586,7 +586,7 @@ func (o *Observer) discoverNewPRs(ctx context.Context, sessionRepos []sessionRep
586586
// reads every PR of the session from the store. Without this write, an
587587
// open sibling/child discovered in the same poll would not yet be
588588
// durable, and the session could terminate while that PR is still open.
589-
if err := o.store.WriteSCMObservation(ctx, known, nil, nil, nil, ports.ReviewWritePreserve); err != nil {
589+
if err := o.store.WriteSCMObservation(ctx, known, nil, nil, nil, nil, ports.ReviewWritePreserve); err != nil {
590590
o.logger.Error("scm observer: persist discovered PR failed", "session", sr.session.ID, "pr", known.URL, "err", err)
591591
if markRepoFailed != nil {
592592
markRepoFailed(repo)
@@ -793,6 +793,7 @@ func (o *Observer) refreshReviews(ctx context.Context, subjects map[string]*subj
793793
if review.Decision != "" {
794794
obs.Review.Decision = review.Decision
795795
}
796+
obs.Review.Reviews = review.Reviews
796797
obs.Review.Threads = review.Threads
797798
obs.Review.Partial = review.Partial
798799
obs.ObservedAt = now
@@ -850,7 +851,7 @@ func (o *Observer) prepareForPersistence(obs ports.SCMObservation, local domain.
850851
return obs
851852
}
852853

853-
func domainFromObservation(sessionID domain.SessionID, obs ports.SCMObservation, local domain.PullRequest, opts persistenceOptions, now time.Time) (domain.PullRequest, []domain.PullRequestCheck, []domain.PullRequestReviewThread, []domain.PullRequestComment) {
854+
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) {
854855
metadataHash := metadataSemanticHash(obs)
855856
if opts.preserveLocalMetadataHash {
856857
metadataHash = local.MetadataHash
@@ -924,6 +925,17 @@ func domainFromObservation(sessionID domain.SessionID, obs ports.SCMObservation,
924925
for _, ch := range obs.CI.Checks {
925926
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})
926927
}
928+
reviews := make([]domain.PullRequestReview, 0, len(obs.Review.Reviews))
929+
for _, review := range obs.Review.Reviews {
930+
reviews = append(reviews, domain.PullRequestReview{
931+
ID: review.ID,
932+
Author: review.Author,
933+
State: domain.ReviewDecision(firstNonEmpty(review.State, string(domain.ReviewNone))),
934+
URL: review.URL,
935+
IsBot: review.IsBot,
936+
SubmittedAt: firstTime(review.SubmittedAt, now),
937+
})
938+
}
927939
threads := make([]domain.PullRequestReviewThread, 0, len(obs.Review.Threads))
928940
commentCount := 0
929941
for _, th := range obs.Review.Threads {
@@ -936,7 +948,7 @@ func domainFromObservation(sessionID domain.SessionID, obs ports.SCMObservation,
936948
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})
937949
}
938950
}
939-
return pr, checks, threads, comments
951+
return pr, checks, reviews, threads, comments
940952
}
941953

942954
func observationFromLocal(repo ports.SCMRepo, pr domain.PullRequest, checks []domain.PullRequestCheck) ports.SCMObservation {
@@ -1119,10 +1131,11 @@ func ciSemanticHash(ci ports.SCMCIObservation) string {
11191131
func reviewSemanticHash(review ports.SCMReviewObservation) string {
11201132
type reviewHashPayload struct {
11211133
Decision string
1134+
Reviews []ports.SCMReviewSummaryObservation
11221135
Threads []ports.SCMReviewThreadObservation
11231136
Partial bool `json:",omitempty"`
11241137
}
1125-
return stableHash(reviewHashPayload{Decision: review.Decision, Threads: review.Threads, Partial: review.Partial})
1138+
return stableHash(reviewHashPayload{Decision: review.Decision, Reviews: review.Reviews, Threads: review.Threads, Partial: review.Partial})
11261139
}
11271140

11281141
func threadSemanticHash(th ports.SCMReviewThreadObservation) string {

0 commit comments

Comments
 (0)