Skip to content

Commit 19d895b

Browse files
feat: support multi-pr review runs
1 parent afe0817 commit 19d895b

16 files changed

Lines changed: 672 additions & 152 deletions

File tree

backend/internal/httpd/apispec/openapi.yaml

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1243,13 +1243,13 @@ paths:
12431243
content:
12441244
application/json:
12451245
schema:
1246-
$ref: '#/components/schemas/ReviewRunResponse'
1246+
$ref: '#/components/schemas/TriggerReviewResponse'
12471247
description: OK
12481248
"201":
12491249
content:
12501250
application/json:
12511251
schema:
1252-
$ref: '#/components/schemas/ReviewRunResponse'
1252+
$ref: '#/components/schemas/TriggerReviewResponse'
12531253
description: Created
12541254
"404":
12551255
content:
@@ -1632,6 +1632,10 @@ components:
16321632
type: object
16331633
ListReviewsResponse:
16341634
properties:
1635+
items:
1636+
items:
1637+
$ref: '#/components/schemas/PRReviewItem'
1638+
type: array
16351639
reviewerHandleId:
16361640
type: string
16371641
reviews:
@@ -1641,6 +1645,7 @@ components:
16411645
required:
16421646
- reviewerHandleId
16431647
- reviews
1648+
- items
16441649
type: object
16451650
ListSessionPRsResponse:
16461651
properties:
@@ -1772,6 +1777,30 @@ components:
17721777
- id
17731778
- projectId
17741779
type: object
1780+
PRReviewItem:
1781+
properties:
1782+
latestRun:
1783+
$ref: '#/components/schemas/ReviewRun'
1784+
prNumber:
1785+
type: integer
1786+
prUrl:
1787+
type: string
1788+
status:
1789+
enum:
1790+
- needs_review
1791+
- running
1792+
- up_to_date
1793+
- changes_requested
1794+
- ineligible
1795+
type: string
1796+
targetSha:
1797+
type: string
1798+
required:
1799+
- prUrl
1800+
- prNumber
1801+
- targetSha
1802+
- status
1803+
type: object
17751804
Project:
17761805
properties:
17771806
agent:
@@ -2403,6 +2432,26 @@ components:
24032432
- body
24042433
- githubReviewId
24052434
type: object
2435+
TriggerReviewResponse:
2436+
properties:
2437+
items:
2438+
items:
2439+
$ref: '#/components/schemas/PRReviewItem'
2440+
type: array
2441+
review:
2442+
$ref: '#/components/schemas/ReviewRun'
2443+
reviewerHandleId:
2444+
type: string
2445+
reviews:
2446+
items:
2447+
$ref: '#/components/schemas/ReviewRun'
2448+
type: array
2449+
required:
2450+
- review
2451+
- reviewerHandleId
2452+
- reviews
2453+
- items
2454+
type: object
24062455
WorkspaceRepo:
24072456
properties:
24082457
name:

backend/internal/httpd/apispec/specgen/build.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -181,11 +181,13 @@ var schemaNames = map[string]string{
181181
"ControllersResolveCommentsRequest": "ResolveCommentsRequest",
182182
"ControllersResolveCommentsResponse": "ResolveCommentsResponse",
183183
// httpd/controllers — review wire envelopes
184-
"ControllersListReviewsResponse": "ListReviewsResponse",
185-
"ControllersReviewRunResponse": "ReviewRunResponse",
186-
"ControllersSubmitReviewInput": "SubmitReviewInput",
184+
"ControllersListReviewsResponse": "ListReviewsResponse",
185+
"ControllersReviewRunResponse": "ReviewRunResponse",
186+
"ControllersTriggerReviewResponse": "TriggerReviewResponse",
187+
"ControllersSubmitReviewInput": "SubmitReviewInput",
187188
// domain review entities
188-
"DomainReviewRun": "ReviewRun",
189+
"DomainReviewRun": "ReviewRun",
190+
"ReviewPRReviewItem": "PRReviewItem",
189191
// service/project entities + DTOs
190192
"ProjectProject": "Project",
191193
"ProjectSummary": "ProjectSummary",
@@ -345,8 +347,8 @@ func reviewOperations() []operation {
345347
summary: "Trigger a code review of a worker's PR",
346348
pathParams: []any{controllers.SessionIDParam{}},
347349
resps: []respUnit{
348-
{http.StatusOK, controllers.ReviewRunResponse{}},
349-
{http.StatusCreated, controllers.ReviewRunResponse{}},
350+
{http.StatusOK, controllers.TriggerReviewResponse{}},
351+
{http.StatusCreated, controllers.TriggerReviewResponse{}},
350352
{http.StatusUnprocessableEntity, envelope.APIError{}},
351353
{http.StatusNotFound, envelope.APIError{}},
352354
{http.StatusNotImplemented, envelope.APIError{}},

backend/internal/httpd/controllers/reviews.go

Lines changed: 35 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,24 +10,36 @@ import (
1010
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
1111
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec"
1212
"github.com/aoagents/agent-orchestrator/backend/internal/httpd/envelope"
13+
reviewcore "github.com/aoagents/agent-orchestrator/backend/internal/review"
1314
reviewsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/review"
1415
)
1516

1617
// ListReviewsResponse is the body of GET /api/v1/sessions/{sessionId}/reviews.
1718
// reviewerHandleId is the live reviewer pane's runtime handle, for the UI to
1819
// attach its terminal over /mux (empty when no reviewer has run).
1920
type ListReviewsResponse struct {
20-
ReviewerHandleID string `json:"reviewerHandleId"`
21-
Reviews []domain.ReviewRun `json:"reviews"`
21+
ReviewerHandleID string `json:"reviewerHandleId"`
22+
Reviews []domain.ReviewRun `json:"reviews"`
23+
Items []reviewcore.PRReviewItem `json:"items"`
2224
}
2325

24-
// ReviewRunResponse is the body of trigger (200/201) and submit (200). It
25-
// carries the run plus the reviewer pane handle so the UI can attach a terminal.
26+
// ReviewRunResponse is the body of submit (200). It carries the run plus the
27+
// reviewer pane handle so the UI can attach a terminal.
2628
type ReviewRunResponse struct {
2729
Review domain.ReviewRun `json:"review"`
2830
ReviewerHandleID string `json:"reviewerHandleId"`
2931
}
3032

33+
// TriggerReviewResponse is the body of trigger (200/201). review and
34+
// reviewerHandleId are legacy compatibility fields; reviews carries the
35+
// PR-scoped batch of newly created runs and items carries per-PR review state.
36+
type TriggerReviewResponse struct {
37+
Review domain.ReviewRun `json:"review"`
38+
ReviewerHandleID string `json:"reviewerHandleId"`
39+
Reviews []domain.ReviewRun `json:"reviews"`
40+
Items []reviewcore.PRReviewItem `json:"items"`
41+
}
42+
3143
// SubmitReviewInput is the body of POST /api/v1/sessions/{sessionId}/reviews/submit.
3244
type SubmitReviewInput struct {
3345
RunID string `json:"runId" description:"Review run id being completed."`
@@ -62,7 +74,11 @@ func (c *ReviewsController) list(w http.ResponseWriter, r *http.Request) {
6274
if runs == nil {
6375
runs = []domain.ReviewRun{}
6476
}
65-
envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: runs})
77+
items := res.Items
78+
if items == nil {
79+
items = []reviewcore.PRReviewItem{}
80+
}
81+
envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: runs, Items: items})
6682
}
6783

6884
func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) {
@@ -81,7 +97,20 @@ func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) {
8197
if res.Created {
8298
status = http.StatusCreated
8399
}
84-
envelope.WriteJSON(w, status, ReviewRunResponse{Review: res.Run, ReviewerHandleID: res.ReviewerHandleID})
100+
reviews := res.CreatedRuns
101+
if reviews == nil {
102+
reviews = []domain.ReviewRun{}
103+
}
104+
items := res.Items
105+
if items == nil {
106+
items = []reviewcore.PRReviewItem{}
107+
}
108+
envelope.WriteJSON(w, status, TriggerReviewResponse{
109+
Review: res.Run,
110+
ReviewerHandleID: res.ReviewerHandleID,
111+
Reviews: reviews,
112+
Items: items,
113+
})
85114
}
86115

87116
func (c *ReviewsController) submit(w http.ResponseWriter, r *http.Request) {

backend/internal/httpd/controllers/reviews_test.go

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,17 @@ import (
2020

2121
type fakeReviewService struct {
2222
triggerErr error
23+
trigger reviewcore.TriggerResult
24+
list reviewcore.SessionReviews
2325
}
2426

2527
func (f *fakeReviewService) Trigger(context.Context, domain.SessionID) (reviewcore.TriggerResult, error) {
2628
if f.triggerErr != nil {
2729
return reviewcore.TriggerResult{}, f.triggerErr
2830
}
31+
if f.trigger.ReviewerHandleID != "" || f.trigger.Run.ID != "" || f.trigger.Items != nil || f.trigger.CreatedRuns != nil {
32+
return f.trigger, nil
33+
}
2934
return reviewcore.TriggerResult{Run: domain.ReviewRun{ID: "run-1"}, Created: true}, nil
3035
}
3136

@@ -34,7 +39,7 @@ func (f *fakeReviewService) Submit(context.Context, domain.SessionID, string, do
3439
}
3540

3641
func (f *fakeReviewService) List(context.Context, domain.SessionID) (reviewcore.SessionReviews, error) {
37-
return reviewcore.SessionReviews{}, nil
42+
return f.list, nil
3843
}
3944

4045
func newReviewTestServer(t *testing.T, svc reviewsvc.Manager) *httptest.Server {
@@ -59,3 +64,46 @@ func TestReviewsTrigger_MissingReviewerBinaryReturns422WithCause(t *testing.T) {
5964
t.Fatalf("message = %q, want reviewer binary cause", got.Message)
6065
}
6166
}
67+
68+
func TestReviewsListIncludesItems(t *testing.T) {
69+
srv := newReviewTestServer(t, &fakeReviewService{list: reviewcore.SessionReviews{
70+
ReviewerHandleID: "review-mer-1",
71+
Runs: []domain.ReviewRun{{ID: "run-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1"}},
72+
Items: []reviewcore.PRReviewItem{{PRURL: "https://github.com/o/r/pull/1", PRNumber: 1, TargetSHA: "sha1", Status: reviewcore.ItemUpToDate}},
73+
}})
74+
75+
body, status, headers := doRequest(t, srv, "GET", "/api/v1/sessions/mer-1/reviews", "")
76+
assertJSON(t, headers)
77+
if status != http.StatusOK {
78+
t.Fatalf("status = %d body=%s", status, body)
79+
}
80+
if !strings.Contains(string(body), `"items"`) || !strings.Contains(string(body), `"up_to_date"`) || !strings.Contains(string(body), `"reviewerHandleId":"review-mer-1"`) {
81+
t.Fatalf("body missing review items/handle: %s", body)
82+
}
83+
}
84+
85+
func TestReviewsTriggerIncludesBatchFields(t *testing.T) {
86+
run1 := domain.ReviewRun{ID: "run-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1"}
87+
run2 := domain.ReviewRun{ID: "run-2", PRURL: "https://github.com/o/r/pull/2", TargetSHA: "sha2"}
88+
srv := newReviewTestServer(t, &fakeReviewService{trigger: reviewcore.TriggerResult{
89+
Run: run1,
90+
ReviewerHandleID: "review-mer-1",
91+
Created: true,
92+
CreatedRuns: []domain.ReviewRun{run1, run2},
93+
Items: []reviewcore.PRReviewItem{
94+
{PRURL: run1.PRURL, PRNumber: 1, TargetSHA: run1.TargetSHA, Status: reviewcore.ItemRunning, LatestRun: &run1},
95+
{PRURL: run2.PRURL, PRNumber: 2, TargetSHA: run2.TargetSHA, Status: reviewcore.ItemRunning, LatestRun: &run2},
96+
},
97+
}})
98+
99+
body, status, headers := doRequest(t, srv, "POST", "/api/v1/sessions/mer-1/reviews/trigger", "")
100+
assertJSON(t, headers)
101+
if status != http.StatusCreated {
102+
t.Fatalf("status = %d body=%s", status, body)
103+
}
104+
for _, want := range []string{`"reviews"`, `"items"`, `"run-1"`, `"run-2"`, `"reviewerHandleId":"review-mer-1"`} {
105+
if !strings.Contains(string(body), want) {
106+
t.Fatalf("body missing %s: %s", want, body)
107+
}
108+
}
109+
}

backend/internal/lifecycle/manager_test.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -543,8 +543,10 @@ func TestApplyReviewResultSendsAndDedupsThroughPRSignature(t *testing.T) {
543543
t.Fatalf("outcome/messages = %q/%v, want sent once", outcome, msg.msgs)
544544
}
545545
got := msg.msgs[0]
546-
if !strings.Contains(got, "[AO reviewer]") || !strings.Contains(got, "fix the bug") || !strings.Contains(got, "98[2J765") {
547-
t.Fatalf("AO review nudge missing label/body/review id: %q", got)
546+
for _, want := range []string{"[AO reviewer]", "PR: " + result.PRURL, "Verdict: changes_requested", "Review body:\nfix the bug", "GitHub review: 98[2J765"} {
547+
if !strings.Contains(got, want) {
548+
t.Fatalf("AO review nudge missing %q: %q", want, got)
549+
}
548550
}
549551
if strings.Contains(got, "\x1b") {
550552
t.Fatalf("AO review nudge should sanitize control bytes: %q", got)

backend/internal/lifecycle/reactions.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -154,13 +154,14 @@ func (m *Manager) ApplyReviewResult(ctx context.Context, workerID domain.Session
154154
if m.messenger == nil {
155155
return ReviewDeliveryNoop, nil
156156
}
157-
msg := "[AO reviewer] AO's internal code reviewer requested changes on your PR. Review the feedback below and address it."
157+
msg := fmt.Sprintf("[AO reviewer] AO's internal code reviewer submitted a review.\n\nPR: %s\nVerdict: %s", domain.SanitizeControlChars(r.PRURL), domain.SanitizeControlChars(string(r.Verdict)))
158158
if r.GithubReviewID != "" {
159159
safeReviewID := domain.SanitizeControlChars(r.GithubReviewID)
160-
msg += fmt.Sprintf(" This feedback is GitHub review %s. Once you have addressed it, reply on that review referencing id %s with how you addressed it, then resolve the review comment threads you addressed.", safeReviewID, safeReviewID)
160+
msg += fmt.Sprintf("\nGitHub review: %s", safeReviewID)
161+
msg += fmt.Sprintf("\n\nOnce you have addressed it, reply on GitHub review %s with how you addressed it, then resolve the review comment threads you addressed.", safeReviewID)
161162
}
162163
if r.Body != "" {
163-
msg += "\n\n" + domain.SanitizeControlChars(r.Body)
164+
msg += "\n\nReview body:\n" + domain.SanitizeControlChars(r.Body)
164165
}
165166
key := "review:" + r.PRURL + ":ao:" + r.RunID
166167
sig := strings.Join([]string{r.TargetSHA, r.RunID, r.GithubReviewID, r.Body}, "\x00")

backend/internal/review/planner.go

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
package review
2+
3+
import (
4+
"sort"
5+
6+
"github.com/aoagents/agent-orchestrator/backend/internal/domain"
7+
)
8+
9+
// ItemStatus is the per-PR review planning state.
10+
type ItemStatus string
11+
12+
const (
13+
// ItemNeedsReview means an eligible PR has no current AO approval or running pass.
14+
ItemNeedsReview ItemStatus = "needs_review"
15+
// ItemRunning means a review run is already active for the PR's current head.
16+
ItemRunning ItemStatus = "running"
17+
// ItemUpToDate means AO approved the PR's current head.
18+
ItemUpToDate ItemStatus = "up_to_date"
19+
// ItemChangesRequested means AO requested changes on the PR's current head.
20+
ItemChangesRequested ItemStatus = "changes_requested"
21+
// ItemIneligible means the PR is draft, closed, merged, or missing required facts.
22+
ItemIneligible ItemStatus = "ineligible"
23+
)
24+
25+
// PRReviewItem is one PR-scoped review decision for a worker session.
26+
type PRReviewItem struct {
27+
PRURL string `json:"prUrl"`
28+
PRNumber int `json:"prNumber"`
29+
TargetSHA string `json:"targetSha"`
30+
Status ItemStatus `json:"status" enum:"needs_review,running,up_to_date,changes_requested,ineligible"`
31+
LatestRun *domain.ReviewRun `json:"latestRun,omitempty"`
32+
}
33+
34+
// Plan computes per-PR review work from the currently observed PRs and existing
35+
// review runs. It is pure so the trigger path and API list path share exactly
36+
// the same eligibility/status rules.
37+
func Plan(prs []domain.PullRequest, runs []domain.ReviewRun) []PRReviewItem {
38+
latest := latestRunsByPRAndSHA(runs)
39+
items := make([]PRReviewItem, 0, len(prs))
40+
for _, pr := range prs {
41+
item := PRReviewItem{
42+
PRURL: pr.URL,
43+
PRNumber: pr.Number,
44+
TargetSHA: pr.HeadSHA,
45+
Status: ItemNeedsReview,
46+
}
47+
if pr.URL == "" || pr.HeadSHA == "" || pr.Draft || pr.Merged || pr.Closed {
48+
item.Status = ItemIneligible
49+
if run, ok := latest[item.PRURL+"\x00"+item.TargetSHA]; ok {
50+
item.LatestRun = &run
51+
}
52+
items = append(items, item)
53+
continue
54+
}
55+
if run, ok := latest[item.PRURL+"\x00"+item.TargetSHA]; ok {
56+
item.LatestRun = &run
57+
switch {
58+
case run.Status == domain.ReviewRunRunning:
59+
item.Status = ItemRunning
60+
case run.Verdict == domain.VerdictApproved:
61+
item.Status = ItemUpToDate
62+
case run.Verdict == domain.VerdictChangesRequested:
63+
item.Status = ItemChangesRequested
64+
case run.Status == domain.ReviewRunFailed:
65+
item.Status = ItemNeedsReview
66+
default:
67+
item.Status = ItemNeedsReview
68+
}
69+
}
70+
items = append(items, item)
71+
}
72+
sort.SliceStable(items, func(i, j int) bool {
73+
if items[i].PRNumber != items[j].PRNumber {
74+
return items[i].PRNumber < items[j].PRNumber
75+
}
76+
return items[i].PRURL < items[j].PRURL
77+
})
78+
return items
79+
}
80+
81+
func latestRunsByPRAndSHA(runs []domain.ReviewRun) map[string]domain.ReviewRun {
82+
latest := make(map[string]domain.ReviewRun)
83+
for _, run := range runs {
84+
if run.PRURL == "" || run.TargetSHA == "" {
85+
continue
86+
}
87+
key := run.PRURL + "\x00" + run.TargetSHA
88+
if existing, ok := latest[key]; !ok || run.CreatedAt.After(existing.CreatedAt) {
89+
latest[key] = run
90+
}
91+
}
92+
return latest
93+
}

0 commit comments

Comments
 (0)