Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions backend/internal/domain/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import (
)

// ErrDuplicateReviewRun is returned by InsertReviewRun when a run already exists
// for the same worker session and target commit (the partial unique index from
// migration 0013). It lets the review engine fall back to the recorded run
// instead of surfacing a raw storage error after a reviewer may have launched.
var ErrDuplicateReviewRun = errors.New("domain: review run already exists for session and target sha")
// for the same worker session, PR, and target commit. It lets the review engine
// fall back to the recorded run instead of surfacing a raw storage error after a
// reviewer may have launched.
var ErrDuplicateReviewRun = errors.New("domain: review run already exists for session, PR, and target sha")

// Review is the per-worker code-review record: one row per worker session
// (SessionID is unique). A repeat trigger reuses this row; the per-pass facts
Expand Down
40 changes: 40 additions & 0 deletions backend/internal/httpd/apispec/openapi.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -912,6 +912,14 @@ paths:
schema:
description: Session identifier, e.g. project-1.
type: string
- description: Tracked PR URL to list review runs for. Omit to list all PR review
targets for the session.
in: query
name: prUrl
schema:
description: Tracked PR URL to list review runs for. Omit to list all PR
review targets for the session.
type: string
responses:
"200":
content:
Expand Down Expand Up @@ -996,6 +1004,11 @@ paths:
schema:
description: Session identifier, e.g. project-1.
type: string
requestBody:
content:
application/json:
schema:
$ref: '#/components/schemas/TriggerReviewInput'
responses:
"200":
content:
Expand Down Expand Up @@ -1332,9 +1345,14 @@ components:
items:
$ref: '#/components/schemas/ReviewRun'
type: array
targets:
items:
$ref: '#/components/schemas/ReviewTargetResponse'
type: array
required:
- reviewerHandleId
- reviews
- targets
type: object
ListSessionPRsResponse:
properties:
Expand Down Expand Up @@ -1644,6 +1662,21 @@ components:
- review
- reviewerHandleId
type: object
ReviewTargetResponse:
properties:
prUrl:
type: string
reviewerHandleId:
type: string
reviews:
items:
$ref: '#/components/schemas/ReviewRun'
type: array
required:
- prUrl
- reviewerHandleId
- reviews
type: object
RoleOverride:
properties:
agent:
Expand Down Expand Up @@ -1876,6 +1909,13 @@ components:
- verdict
- body
type: object
TriggerReviewInput:
properties:
prUrl:
description: Tracked PR URL to review. Required when the session owns multiple
PRs.
type: string
type: object
WorkspaceRepo:
properties:
name:
Expand Down
24 changes: 17 additions & 7 deletions backend/internal/httpd/apispec/specgen/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,11 @@ func Build() ([]byte, error) {
// AddReqStructure leaves requestBody.required absent, which
// OpenAPI reads as optional. These bodies are mandatory, so force
// it — otherwise validators/generators treat the body as skippable.
oc.AddReqStructure(op.reqBody, openapi.WithCustomize(markRequestBodyRequired))
if op.optionalReqBody {
oc.AddReqStructure(op.reqBody)
} else {
oc.AddReqStructure(op.reqBody, openapi.WithCustomize(markRequestBodyRequired))
}
}
for _, resp := range op.resps {
opts := []openapi.ContentOption{openapi.WithHTTPStatus(resp.status)}
Expand Down Expand Up @@ -167,9 +171,12 @@ var schemaNames = map[string]string{
"ControllersResolveCommentsRequest": "ResolveCommentsRequest",
"ControllersResolveCommentsResponse": "ResolveCommentsResponse",
// httpd/controllers — review wire envelopes
"ControllersListReviewsResponse": "ListReviewsResponse",
"ControllersReviewRunResponse": "ReviewRunResponse",
"ControllersSubmitReviewInput": "SubmitReviewInput",
"ControllersListReviewsResponse": "ListReviewsResponse",
"ControllersListReviewsQuery": "ListReviewsQuery",
"ControllersReviewRunResponse": "ReviewRunResponse",
"ControllersReviewTargetResponse": "ReviewTargetResponse",
"ControllersTriggerReviewInput": "TriggerReviewInput",
"ControllersSubmitReviewInput": "SubmitReviewInput",
// domain review entities
"DomainReviewRun": "ReviewRun",
// service/project entities + DTOs
Expand Down Expand Up @@ -248,6 +255,7 @@ type operation struct {
tag string
pathParams []any // path/query param containers (e.g. ProjectIDParam)
reqBody any // JSON request body struct, nil when the op takes none
optionalReqBody bool
resps []respUnit
contentTypes map[int]string // optional non-JSON response content types by status
}
Expand Down Expand Up @@ -297,7 +305,7 @@ func reviewOperations() []operation {
{
method: http.MethodGet, path: "/api/v1/sessions/{sessionId}/reviews", id: "listReviews", tag: "reviews",
summary: "List a worker's code-review runs",
pathParams: []any{controllers.SessionIDParam{}},
pathParams: []any{controllers.SessionIDParam{}, controllers.ListReviewsQuery{}},
resps: []respUnit{
{http.StatusOK, controllers.ListReviewsResponse{}},
{http.StatusUnprocessableEntity, envelope.APIError{}},
Expand All @@ -306,8 +314,10 @@ func reviewOperations() []operation {
},
{
method: http.MethodPost, path: "/api/v1/sessions/{sessionId}/reviews/trigger", id: "triggerReview", tag: "reviews",
summary: "Trigger a code review of a worker's PR",
pathParams: []any{controllers.SessionIDParam{}},
summary: "Trigger a code review of a worker's PR",
pathParams: []any{controllers.SessionIDParam{}},
reqBody: controllers.TriggerReviewInput{},
optionalReqBody: true,
resps: []respUnit{
{http.StatusOK, controllers.ReviewRunResponse{}},
{http.StatusCreated, controllers.ReviewRunResponse{}},
Expand Down
52 changes: 47 additions & 5 deletions backend/internal/httpd/controllers/reviews.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package controllers
import (
"encoding/json"
"errors"
"io"
"net/http"

"github.com/go-chi/chi/v5"
Expand All @@ -13,12 +14,26 @@ import (
reviewsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/review"
)

// ListReviewsQuery selects one PR's review state. Omit prUrl to list every PR
// review target for the session.
type ListReviewsQuery struct {
PRURL string `query:"prUrl,omitempty" description:"Tracked PR URL to list review runs for. Omit to list all PR review targets for the session."`
}

// ReviewTargetResponse is one PR's review state within a worker session.
type ReviewTargetResponse struct {
PRURL string `json:"prUrl"`
ReviewerHandleID string `json:"reviewerHandleId"`
Reviews []domain.ReviewRun `json:"reviews"`
}

// ListReviewsResponse is the body of GET /api/v1/sessions/{sessionId}/reviews.
// reviewerHandleId is the live reviewer pane's runtime handle, for the UI to
// attach its terminal over /mux (empty when no reviewer has run).
type ListReviewsResponse struct {
ReviewerHandleID string `json:"reviewerHandleId"`
Reviews []domain.ReviewRun `json:"reviews"`
ReviewerHandleID string `json:"reviewerHandleId"`
Reviews []domain.ReviewRun `json:"reviews"`
Targets []ReviewTargetResponse `json:"targets"`
}

// ReviewRunResponse is the body of trigger (200/201) and submit (200). It
Expand All @@ -28,6 +43,12 @@ type ReviewRunResponse struct {
ReviewerHandleID string `json:"reviewerHandleId"`
}

// TriggerReviewInput is the optional body of
// POST /api/v1/sessions/{sessionId}/reviews/trigger.
type TriggerReviewInput struct {
PRURL string `json:"prUrl,omitempty" description:"Tracked PR URL to review. Required when the session owns multiple PRs."`
}

// SubmitReviewInput is the body of POST /api/v1/sessions/{sessionId}/reviews/submit.
type SubmitReviewInput struct {
RunID string `json:"runId" description:"Review run id being completed."`
Expand All @@ -52,7 +73,7 @@ func (c *ReviewsController) list(w http.ResponseWriter, r *http.Request) {
apispec.NotImplemented(w, r, "GET", "/api/v1/sessions/{sessionId}/reviews")
return
}
res, err := c.Svc.List(r.Context(), sessionID(r))
res, err := c.Svc.List(r.Context(), sessionID(r), r.URL.Query().Get("prUrl"))
if err != nil {
writeReviewError(w, r, err)
return
Expand All @@ -61,15 +82,20 @@ func (c *ReviewsController) list(w http.ResponseWriter, r *http.Request) {
if runs == nil {
runs = []domain.ReviewRun{}
}
envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: runs})
envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: runs, Targets: reviewTargetsResponse(res.Targets)})
}

func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) {
if c.Svc == nil {
apispec.NotImplemented(w, r, "POST", "/api/v1/sessions/{sessionId}/reviews/trigger")
return
}
res, err := c.Svc.Trigger(r.Context(), sessionID(r))
var in TriggerReviewInput
if err := json.NewDecoder(r.Body).Decode(&in); err != nil && !errors.Is(err, io.EOF) {
envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_BODY", "Invalid request body", nil)
return
}
res, err := c.Svc.Trigger(r.Context(), sessionID(r), in.PRURL)
if err != nil {
writeReviewError(w, r, err)
return
Expand All @@ -83,6 +109,22 @@ func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) {
envelope.WriteJSON(w, status, ReviewRunResponse{Review: res.Run, ReviewerHandleID: res.ReviewerHandleID})
}

func reviewTargetsResponse(targets []reviewsvc.ReviewTarget) []ReviewTargetResponse {
out := make([]ReviewTargetResponse, 0, len(targets))
for _, target := range targets {
runs := target.Runs
if runs == nil {
runs = []domain.ReviewRun{}
}
out = append(out, ReviewTargetResponse{
PRURL: target.PRURL,
ReviewerHandleID: target.ReviewerHandleID,
Reviews: runs,
})
}
return out
}

func (c *ReviewsController) submit(w http.ResponseWriter, r *http.Request) {
if c.Svc == nil {
apispec.NotImplemented(w, r, "POST", "/api/v1/sessions/{sessionId}/reviews/submit")
Expand Down
104 changes: 104 additions & 0 deletions backend/internal/httpd/controllers/reviews_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
package controllers

import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"strings"
"testing"

"github.com/go-chi/chi/v5"

"github.com/aoagents/agent-orchestrator/backend/internal/domain"
reviewcore "github.com/aoagents/agent-orchestrator/backend/internal/review"
)

type fakeReviewsService struct {
listWorker domain.SessionID
listPRURL string
triggerWorker domain.SessionID
triggerPRURL string
}

func (f *fakeReviewsService) Trigger(_ context.Context, workerID domain.SessionID, prURL string) (reviewcore.TriggerResult, error) {
f.triggerWorker = workerID
f.triggerPRURL = prURL
return reviewcore.TriggerResult{
Run: domain.ReviewRun{ID: "run-1", SessionID: workerID, PRURL: prURL},
ReviewerHandleID: "reviewer-1",
Created: true,
}, nil
}

func (f *fakeReviewsService) Submit(_ context.Context, workerID domain.SessionID, runID string, verdict domain.ReviewVerdict, body string) (domain.ReviewRun, error) {
return domain.ReviewRun{ID: runID, SessionID: workerID, Verdict: verdict, Body: body}, nil
}

func (f *fakeReviewsService) List(_ context.Context, workerID domain.SessionID, prURL string) (reviewcore.SessionReviews, error) {
f.listWorker = workerID
f.listPRURL = prURL
return reviewcore.SessionReviews{
ReviewerHandleID: "reviewer-1",
Runs: []domain.ReviewRun{{ID: "run-1", SessionID: workerID, PRURL: prURL}},
Targets: []reviewcore.ReviewTarget{{PRURL: prURL, ReviewerHandleID: "reviewer-1", Runs: []domain.ReviewRun{{ID: "run-1", SessionID: workerID, PRURL: prURL}}}},
}, nil
}

func reviewTestRouter(svc *fakeReviewsService) http.Handler {
r := chi.NewRouter()
(&ReviewsController{Svc: svc}).Register(r)
return r
}

func TestReviewsListPassesPRURLQuery(t *testing.T) {
svc := &fakeReviewsService{}
req := httptest.NewRequest(http.MethodGet, "/sessions/mer-1/reviews?prUrl=https%3A%2F%2Fgithub.com%2Fo%2Fr%2Fpull%2F1", nil)
rec := httptest.NewRecorder()

reviewTestRouter(svc).ServeHTTP(rec, req)

if rec.Code != http.StatusOK {
t.Fatalf("status = %d body=%s", rec.Code, rec.Body.String())
}
if svc.listWorker != "mer-1" || svc.listPRURL != "https://github.com/o/r/pull/1" {
t.Fatalf("list args worker=%q pr=%q", svc.listWorker, svc.listPRURL)
}
var out ListReviewsResponse
if err := json.Unmarshal(rec.Body.Bytes(), &out); err != nil {
t.Fatalf("decode response: %v", err)
}
if len(out.Targets) != 1 || out.Targets[0].PRURL != "https://github.com/o/r/pull/1" {
t.Fatalf("targets = %+v", out.Targets)
}
}

func TestReviewsTriggerAcceptsOptionalPRURLBody(t *testing.T) {
svc := &fakeReviewsService{}
req := httptest.NewRequest(http.MethodPost, "/sessions/mer-1/reviews/trigger", strings.NewReader(`{"prUrl":"https://github.com/o/r/pull/1"}`))
rec := httptest.NewRecorder()

reviewTestRouter(svc).ServeHTTP(rec, req)

if rec.Code != http.StatusCreated {
t.Fatalf("status = %d body=%s", rec.Code, rec.Body.String())
}
if svc.triggerWorker != "mer-1" || svc.triggerPRURL != "https://github.com/o/r/pull/1" {
t.Fatalf("trigger args worker=%q pr=%q", svc.triggerWorker, svc.triggerPRURL)
}
}

func TestReviewsTriggerAcceptsEmptyBody(t *testing.T) {
svc := &fakeReviewsService{}
req := httptest.NewRequest(http.MethodPost, "/sessions/mer-1/reviews/trigger", nil)
rec := httptest.NewRecorder()

reviewTestRouter(svc).ServeHTTP(rec, req)

if rec.Code != http.StatusCreated {
t.Fatalf("status = %d body=%s", rec.Code, rec.Body.String())
}
if svc.triggerWorker != "mer-1" || svc.triggerPRURL != "" {
t.Fatalf("trigger args worker=%q pr=%q", svc.triggerWorker, svc.triggerPRURL)
}
}
Loading
Loading