diff --git a/backend/internal/adapters/reviewer/claudecode/claudecode.go b/backend/internal/adapters/reviewer/claudecode/claudecode.go new file mode 100644 index 00000000..5573553f --- /dev/null +++ b/backend/internal/adapters/reviewer/claudecode/claudecode.go @@ -0,0 +1,68 @@ +// Package claudecode is the claude-code reviewer adapter. claude-code is a +// prompt-driven agent, so this reviewer feeds AO's review prompt (authored +// centrally and passed in ReviewInvocation.Prompt) to the worker claude-code +// adapter's launch-command construction (binary resolution, flags). The reviewer +// contract stays prompt-agnostic, so a one-shot CLI reviewer (e.g. greptile) can +// ignore the prompt entirely. +package claudecode + +import ( + "context" + + workeragent "github.com/aoagents/agent-orchestrator/backend/internal/adapters/agent/claudecode" + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +// Reviewer is the claude-code code-review adapter. +type Reviewer struct { + agent ports.Agent +} + +// New builds the claude-code reviewer adapter. +func New() *Reviewer { + return &Reviewer{agent: workeragent.New()} +} + +// Harness identifies this reviewer in the reviewer registry. +func (r *Reviewer) Harness() domain.ReviewerHarness { + return domain.ReviewerClaudeCode +} + +var _ ports.Reviewer = (*Reviewer)(nil) + +// ReviewCommand builds a claude-code invocation that reviews the worker's +// checkout for the PR, with the review prompt baked in. +func (r *Reviewer) ReviewCommand(ctx context.Context, inv ports.ReviewInvocation) (ports.ReviewCommandSpec, error) { + argv, err := r.agent.GetLaunchCommand(ctx, ports.LaunchConfig{ + SessionID: inv.ReviewerID, + WorkspacePath: inv.WorkspacePath, + Prompt: inv.Prompt, + SystemPrompt: inv.SystemPrompt, + }) + if err != nil { + return ports.ReviewCommandSpec{}, err + } + return ports.ReviewCommandSpec{Argv: argv}, nil +} + +// PreLaunch runs any reviewer-specific preflight. For Claude Code this records +// the worker checkout as trusted before the headless reviewer pane starts. +func (r *Reviewer) PreLaunch(ctx context.Context, inv ports.ReviewInvocation) error { + pl, ok := r.agent.(interface { + PreLaunch(context.Context, ports.LaunchConfig) error + }) + if !ok { + return nil + } + return pl.PreLaunch(ctx, ports.LaunchConfig{ + SessionID: inv.ReviewerID, + WorkspacePath: inv.WorkspacePath, + }) +} + +// ReviewMessage is the text injected into an already-running reviewer pane to +// review a new commit — AO's central review prompt. +func (r *Reviewer) ReviewMessage(_ context.Context, inv ports.ReviewInvocation) (string, error) { + return inv.Prompt, nil +} diff --git a/backend/internal/adapters/reviewer/registry.go b/backend/internal/adapters/reviewer/registry.go new file mode 100644 index 00000000..b5fbcb7d --- /dev/null +++ b/backend/internal/adapters/reviewer/registry.go @@ -0,0 +1,58 @@ +// Package reviewer is the single source of truth for the code-review adapters +// the daemon ships. It mirrors the worker agent registry but is a separate set: +// adding a reviewer (claude-code today, greptile tomorrow) is one edit here and +// does not widen the worker AgentHarness vocabulary. +package reviewer + +import ( + "fmt" + + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/reviewer/claudecode" + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +// Adapter is a registered reviewer: a ports.Reviewer that names its harness. +type Adapter interface { + ports.Reviewer + Harness() domain.ReviewerHarness +} + +// Constructors returns every reviewer adapter the daemon ships. Add a reviewer +// here (and to domain.AllReviewerHarnesses) to register it. +func Constructors() []Adapter { + return []Adapter{ + claudecode.New(), + } +} + +// Resolver maps a reviewer harness onto its adapter. +type Resolver struct { + reviewers map[domain.ReviewerHarness]ports.Reviewer +} + +var _ ports.ReviewerResolver = (*Resolver)(nil) + +// NewResolver builds a Resolver from the shipped reviewer adapters. It fails if +// two adapters claim the same harness, or if a registered harness is not in the +// domain reviewer vocabulary (the two must stay in sync). +func NewResolver() (*Resolver, error) { + m := make(map[domain.ReviewerHarness]ports.Reviewer) + for _, a := range Constructors() { + h := a.Harness() + if !h.IsKnown() { + return nil, fmt.Errorf("reviewer adapter %q is not in domain.AllReviewerHarnesses", h) + } + if _, dup := m[h]; dup { + return nil, fmt.Errorf("reviewer harness %q is registered twice", h) + } + m[h] = a + } + return &Resolver{reviewers: m}, nil +} + +// Reviewer returns the adapter for a harness, ok=false when none is registered. +func (r *Resolver) Reviewer(harness domain.ReviewerHarness) (ports.Reviewer, bool) { + rv, ok := r.reviewers[harness] + return rv, ok +} diff --git a/backend/internal/adapters/reviewer/registry_test.go b/backend/internal/adapters/reviewer/registry_test.go new file mode 100644 index 00000000..fba7020f --- /dev/null +++ b/backend/internal/adapters/reviewer/registry_test.go @@ -0,0 +1,44 @@ +package reviewer + +import ( + "testing" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// TestRegistryMatchesDomainVocabulary enforces that the shipped reviewer +// adapters and domain.AllReviewerHarnesses stay in sync: every registered +// adapter is a known reviewer harness, and every known harness has an adapter. +func TestRegistryMatchesDomainVocabulary(t *testing.T) { + registered := map[domain.ReviewerHarness]bool{} + for _, a := range Constructors() { + h := a.Harness() + if !h.IsKnown() { + t.Errorf("adapter harness %q is not in domain.AllReviewerHarnesses", h) + } + if registered[h] { + t.Errorf("reviewer harness %q registered twice", h) + } + registered[h] = true + } + for _, h := range domain.AllReviewerHarnesses { + if !registered[h] { + t.Errorf("reviewer harness %q has no registered adapter", h) + } + } +} + +func TestNewResolverResolvesShippedReviewers(t *testing.T) { + resolver, err := NewResolver() + if err != nil { + t.Fatalf("NewResolver: %v", err) + } + for _, h := range domain.AllReviewerHarnesses { + if _, ok := resolver.Reviewer(h); !ok { + t.Errorf("resolver missing reviewer %q", h) + } + } + if _, ok := resolver.Reviewer("nope"); ok { + t.Error("resolver returned an adapter for an unknown harness") + } +} diff --git a/backend/internal/cli/review.go b/backend/internal/cli/review.go new file mode 100644 index 00000000..7bb2f7f5 --- /dev/null +++ b/backend/internal/cli/review.go @@ -0,0 +1,104 @@ +package cli + +import ( + "errors" + "fmt" + "net/url" + "os" + "strings" + "time" + + "github.com/spf13/cobra" +) + +// reviewRun mirrors the daemon's domain.ReviewRun for the CLI client. +type reviewRun struct { + ID string `json:"id"` + SessionID string `json:"sessionId"` + Harness string `json:"harness"` + PRURL string `json:"prUrl"` + TargetSHA string `json:"targetSha"` + Status string `json:"status"` + Verdict string `json:"verdict"` + Body string `json:"body"` + CreatedAt time.Time `json:"createdAt"` +} + +// reviewRunResponse mirrors controllers.ReviewRunResponse. +type reviewRunResponse struct { + Review reviewRun `json:"review"` + ReviewerHandleID string `json:"reviewerHandleId"` +} + +// submitReviewRequest mirrors controllers.SubmitReviewInput. +type submitReviewRequest struct { + RunID string `json:"runId"` + Verdict string `json:"verdict"` + Body string `json:"body"` +} + +type reviewSubmitOptions struct { + session string + runID string + verdict string + body string +} + +func newReviewCommand(ctx *commandContext) *cobra.Command { + cmd := &cobra.Command{ + Use: "review", + Short: "Manage AO code reviews of a worker's PR", + } + cmd.AddCommand(newReviewSubmitCommand(ctx)) + return cmd +} + +func newReviewSubmitCommand(ctx *commandContext) *cobra.Command { + var opts reviewSubmitOptions + cmd := &cobra.Command{ + Use: "submit [worker-session-id]", + Short: "Record a reviewer's result for a worker's PR", + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return ctx.submitReview(cmd, args, opts) + }, + } + cmd.Flags().StringVar(&opts.session, "session", "", "Worker session id (or pass it as the positional argument)") + cmd.Flags().StringVar(&opts.runID, "run", "", "Review run id (required)") + cmd.Flags().StringVar(&opts.verdict, "verdict", "", "Review verdict: approved or changes_requested (required)") + cmd.Flags().StringVar(&opts.body, "body", "", "Path to a Markdown file with the review body") + return cmd +} + +func (c *commandContext) submitReview(cmd *cobra.Command, args []string, opts reviewSubmitOptions) error { + session := strings.TrimSpace(opts.session) + if len(args) == 1 { + session = strings.TrimSpace(args[0]) + } + if session == "" { + return usageError{errors.New("usage: worker session id is required (positional or --session)")} + } + runID := strings.TrimSpace(opts.runID) + if runID == "" { + return usageError{errors.New("usage: --run is required")} + } + verdict := strings.TrimSpace(opts.verdict) + if verdict == "" { + return usageError{errors.New("usage: --verdict is required (approved or changes_requested)")} + } + var body string + if path := strings.TrimSpace(opts.body); path != "" { + raw, err := os.ReadFile(path) + if err != nil { + return usageError{fmt.Errorf("read body file: %w", err)} + } + body = string(raw) + } + path := "sessions/" + url.PathEscape(session) + "/reviews/submit" + var res reviewRunResponse + if err := c.postJSON(cmd.Context(), path, submitReviewRequest{RunID: runID, Verdict: verdict, Body: body}, &res); err != nil { + return err + } + _, err := fmt.Fprintf(cmd.OutOrStdout(), "recorded %s review for %s\n", res.Review.Verdict, session) + return err +} diff --git a/backend/internal/cli/review_test.go b/backend/internal/cli/review_test.go new file mode 100644 index 00000000..29cdee64 --- /dev/null +++ b/backend/internal/cli/review_test.go @@ -0,0 +1,100 @@ +package cli + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" +) + +// reviewCapture records the method/path/body of the request the CLI made. +type reviewCapture struct { + method string + path string + body string +} + +func reviewServer(t *testing.T, status int, respBody string) (*httptest.Server, *reviewCapture) { + t.Helper() + capture := &reviewCapture{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + capture.method = r.Method + capture.path = r.URL.Path + capture.body = string(body) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = io.WriteString(w, respBody) + })) + t.Cleanup(srv.Close) + return srv, capture +} + +func aliveDeps() Deps { return Deps{ProcessAlive: func(int) bool { return true }} } + +func TestReviewSubmitReadsBodyFile(t *testing.T) { + cfg := setConfigEnv(t) + srv, capture := reviewServer(t, http.StatusOK, `{"review":{"verdict":"changes_requested"}}`) + writeRunFileFor(t, cfg, srv) + + bodyFile := filepath.Join(t.TempDir(), "review.md") + if err := os.WriteFile(bodyFile, []byte("please fix"), 0o600); err != nil { + t.Fatal(err) + } + + _, errOut, err := executeCLI(t, aliveDeps(), + "review", "submit", "mer-1", "--run", "run-1", "--verdict", "changes_requested", "--body", bodyFile) + if err != nil { + t.Fatalf("unexpected error: %v\nstderr=%s", err, errOut) + } + if capture.method != http.MethodPost || capture.path != "/api/v1/sessions/mer-1/reviews/submit" { + t.Fatalf("request = %s %s", capture.method, capture.path) + } + var req submitReviewRequest + if err := json.Unmarshal([]byte(capture.body), &req); err != nil { + t.Fatalf("decode body: %v", err) + } + if req.RunID != "run-1" || req.Verdict != "changes_requested" || req.Body != "please fix" { + t.Fatalf("request = %+v", req) + } +} + +func TestReviewSubmitUsesSessionFlag(t *testing.T) { + cfg := setConfigEnv(t) + srv, capture := reviewServer(t, http.StatusOK, `{"review":{"verdict":"approved"}}`) + writeRunFileFor(t, cfg, srv) + + if _, errOut, err := executeCLI(t, aliveDeps(), "review", "submit", "--session", "mer-7", "--run", "run-7", "--verdict", "approved"); err != nil { + t.Fatalf("unexpected error: %v\nstderr=%s", err, errOut) + } + if capture.path != "/api/v1/sessions/mer-7/reviews/submit" { + t.Fatalf("path = %q, want mer-7", capture.path) + } +} + +func TestReviewSubmitMissingVerdictIsUsageError(t *testing.T) { + setConfigEnv(t) + _, _, err := executeCLI(t, aliveDeps(), "review", "submit", "mer-1", "--run", "run-1") + if got := ExitCode(err); got != 2 { + t.Fatalf("exit code = %d, want 2 (usage); err=%v", got, err) + } +} + +func TestReviewSubmitMissingWorkerIsUsageError(t *testing.T) { + setConfigEnv(t) + _, _, err := executeCLI(t, aliveDeps(), "review", "submit", "--run", "run-1", "--verdict", "approved") + if got := ExitCode(err); got != 2 { + t.Fatalf("exit code = %d, want 2 (usage); err=%v", got, err) + } +} + +func TestReviewSubmitMissingRunIsUsageError(t *testing.T) { + setConfigEnv(t) + _, _, err := executeCLI(t, aliveDeps(), "review", "submit", "mer-1", "--verdict", "approved") + if got := ExitCode(err); got != 2 { + t.Fatalf("exit code = %d, want 2 (usage); err=%v", got, err) + } +} diff --git a/backend/internal/cli/root.go b/backend/internal/cli/root.go index 4dec629c..f536459c 100644 --- a/backend/internal/cli/root.go +++ b/backend/internal/cli/root.go @@ -171,6 +171,7 @@ func NewRootCommand(deps Deps) *cobra.Command { root.AddCommand(newProjectCommand(ctx)) root.AddCommand(newSessionCommand(ctx)) root.AddCommand(newOrchestratorCommand(ctx)) + root.AddCommand(newReviewCommand(ctx)) root.AddCommand(newCompletionCommand()) root.AddCommand(newVersionCommand()) diff --git a/backend/internal/daemon/daemon.go b/backend/internal/daemon/daemon.go index f37e42f8..747f5251 100644 --- a/backend/internal/daemon/daemon.go +++ b/backend/internal/daemon/daemon.go @@ -18,7 +18,6 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/runfile" notificationsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/notification" projectsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/project" - reviewsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/review" "github.com/aoagents/agent-orchestrator/backend/internal/storage/sqlite" "github.com/aoagents/agent-orchestrator/backend/internal/terminal" ) @@ -98,7 +97,7 @@ func Run() error { // zellij runtime, a gitworktree workspace, the per-session agent resolver // (AO_AGENT default, validated here), and the agent messenger, then mount it // on the API. - sessionSvc, err := startSession(cfg, runtimeAdapter, store, lcStack.LCM, messenger, log) + sessionSvc, reviewSvc, err := startSession(cfg, runtimeAdapter, store, lcStack.LCM, messenger, log) if err != nil { stop() lcStack.Stop() @@ -111,7 +110,7 @@ func Run() error { srv, err := httpd.NewWithDeps(cfg, log, termMgr, httpd.APIDeps{ Projects: projectsvc.NewWithDeps(projectsvc.Deps{Store: store, Sessions: sessionSvc}), Sessions: sessionSvc, - Reviews: reviewsvc.NewInMemory(), + Reviews: reviewSvc, Notifications: notifier, NotificationStream: notificationHub, CDC: store, diff --git a/backend/internal/daemon/lifecycle_wiring.go b/backend/internal/daemon/lifecycle_wiring.go index d86e4345..a66d3dea 100644 --- a/backend/internal/daemon/lifecycle_wiring.go +++ b/backend/internal/daemon/lifecycle_wiring.go @@ -9,12 +9,16 @@ import ( "github.com/aoagents/agent-orchestrator/backend/internal/adapters" "github.com/aoagents/agent-orchestrator/backend/internal/adapters/agent/activitydispatch" agentregistry "github.com/aoagents/agent-orchestrator/backend/internal/adapters/agent/registry" + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/reviewer" + "github.com/aoagents/agent-orchestrator/backend/internal/adapters/runtime/zellij" "github.com/aoagents/agent-orchestrator/backend/internal/adapters/workspace/gitworktree" "github.com/aoagents/agent-orchestrator/backend/internal/config" "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/lifecycle" "github.com/aoagents/agent-orchestrator/backend/internal/observe/reaper" "github.com/aoagents/agent-orchestrator/backend/internal/ports" + reviewcore "github.com/aoagents/agent-orchestrator/backend/internal/review" + reviewsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/review" sessionsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/session" sessionmanager "github.com/aoagents/agent-orchestrator/backend/internal/session_manager" "github.com/aoagents/agent-orchestrator/backend/internal/storage/sqlite" @@ -58,7 +62,7 @@ func (l *lifecycleStack) Stop() { // over the real zellij runtime, a per-session gitworktree workspace, the shared // store + LCM, the per-session agent resolver (AO_AGENT default), and the // agent messenger. The returned service is mounted at httpd APIDeps.Sessions. -func startSession(cfg config.Config, runtime ports.Runtime, store *sqlite.Store, lcm *lifecycle.Manager, messenger ports.AgentMessenger, log *slog.Logger) (*sessionsvc.Service, error) { +func startSession(cfg config.Config, runtime *zellij.Runtime, store *sqlite.Store, lcm *lifecycle.Manager, messenger ports.AgentMessenger, log *slog.Logger) (*sessionsvc.Service, reviewsvc.Manager, error) { // Resolve the default agent once and share it with both the resolver (which // launches it for an unspecified harness) and the session manager (which // persists it onto the seed row), so the stored harness matches what runs. @@ -68,7 +72,7 @@ func startSession(cfg config.Config, runtime ports.Runtime, store *sqlite.Store, } agents, err := buildAgentResolver(defaultAgent, log) if err != nil { - return nil, err + return nil, nil, err } ws, err := gitworktree.New(gitworktree.Options{ // Per-session worktrees live under the data dir, so a single AO_DATA_DIR @@ -80,7 +84,7 @@ func startSession(cfg config.Config, runtime ports.Runtime, store *sqlite.Store, RepoResolver: projectRepoResolver{store: store}, }) if err != nil { - return nil, fmt.Errorf("session workspace: %w", err) + return nil, nil, fmt.Errorf("session workspace: %w", err) } mgr := sessionmanager.New(sessionmanager.Deps{ Runtime: runtime, @@ -97,7 +101,7 @@ func startSession(cfg config.Config, runtime ports.Runtime, store *sqlite.Store, if err != nil { logSCMProviderDisabled(log, err) } - return sessionsvc.NewWithDeps(sessionsvc.Deps{ + sessionSvc := sessionsvc.NewWithDeps(sessionsvc.Deps{ Manager: mgr, Store: store, PRClaimer: store, @@ -105,7 +109,24 @@ func startSession(cfg config.Config, runtime ports.Runtime, store *sqlite.Store, // no_signal only makes sense for harnesses whose adapters install // activity hooks; the deriver registry is the source of truth for that. SignalCapable: activitydispatch.SupportsHarness, - }), nil + }) + // Triggering a review spawns a reviewer over the worker's worktree, resolved + // from the reviewer registry (distinct from the worker agent set). The + // reviewer posts its review to the PR itself, so the service needs no SCM + // writer. + reviewers, err := reviewer.NewResolver() + if err != nil { + return nil, nil, fmt.Errorf("reviewer resolver: %w", err) + } + reviewEngine := reviewcore.New(reviewcore.Deps{ + Store: store, + Sessions: store, + PRs: store, + Projects: store, + Launcher: reviewcore.NewLauncher(reviewers, runtime), + }) + reviewSvc := reviewsvc.New(reviewEngine) + return sessionSvc, reviewSvc, nil } // runtimeMessageSender is the narrow part of the concrete runtime needed by diff --git a/backend/internal/daemon/wiring_test.go b/backend/internal/daemon/wiring_test.go index df194ad6..36e67344 100644 --- a/backend/internal/daemon/wiring_test.go +++ b/backend/internal/daemon/wiring_test.go @@ -149,13 +149,16 @@ func TestWiring_StartSessionBuildsSessionService(t *testing.T) { runtime := zellij.New(zellij.Options{}) messenger := newSessionMessenger(store, runtime, log) - svc, err := startSession(cfg, runtime, store, lcm, messenger, log) + svc, reviewSvc, err := startSession(cfg, runtime, store, lcm, messenger, log) if err != nil { t.Fatalf("startSession: %v", err) } if svc == nil { t.Fatal("startSession returned nil session service") } + if reviewSvc == nil { + t.Fatal("startSession returned nil review service") + } } type captureRuntimeSender struct { diff --git a/backend/internal/domain/projectconfig.go b/backend/internal/domain/projectconfig.go index ee958b31..840ad34b 100644 --- a/backend/internal/domain/projectconfig.go +++ b/backend/internal/domain/projectconfig.go @@ -36,6 +36,35 @@ type ProjectConfig struct { // Worker and Orchestrator are role-specific harness/agent-config overrides. Worker RoleOverride `json:"worker,omitempty"` Orchestrator RoleOverride `json:"orchestrator,omitempty"` + + // Reviewers names the agent(s) that review a worker's PR when a review is + // triggered. It is configured independently of the Worker override; an empty + // list falls back to the worker's own harness (see ResolveReviewerHarness). + Reviewers []ReviewerConfig `json:"reviewers,omitempty"` +} + +// ReviewerConfig names one reviewer agent by harness. The harness is drawn from +// the reviewer vocabulary (ReviewerHarness), which is distinct from the worker +// AgentHarness set. +type ReviewerConfig struct { + Harness ReviewerHarness `json:"harness"` +} + +// FallbackReviewerHarness is the reviewer used when a project configures none +// and the worker's harness is not itself a supported reviewer. +const FallbackReviewerHarness = ReviewerClaudeCode + +// ResolveReviewerHarness picks the reviewer harness for a worker. A configured +// reviewer wins; otherwise it reuses the worker's own harness when that harness +// is also a supported reviewer, falling back to claude-code. +func (c ProjectConfig) ResolveReviewerHarness(workerHarness AgentHarness) ReviewerHarness { + if len(c.Reviewers) > 0 { + return c.Reviewers[0].Harness + } + if h := ReviewerHarness(workerHarness); h.IsKnown() { + return h + } + return FallbackReviewerHarness } // RoleOverride overrides the harness and/or agent config for a session role. @@ -94,6 +123,11 @@ func (c ProjectConfig) Validate() error { return fmt.Errorf("symlink %q: %w", s, err) } } + for i, rv := range c.Reviewers { + if !rv.Harness.IsKnown() { + return fmt.Errorf("reviewers[%d].harness: unknown harness %q", i, rv.Harness) + } + } return nil } diff --git a/backend/internal/domain/projectconfig_test.go b/backend/internal/domain/projectconfig_test.go index cc080631..58d9c3ba 100644 --- a/backend/internal/domain/projectconfig_test.go +++ b/backend/internal/domain/projectconfig_test.go @@ -23,6 +23,10 @@ func TestProjectConfigValidate(t *testing.T) { {"symlink parent escape", ProjectConfig{Symlinks: []string{"../escape"}}, true}, {"symlink embedded parent", ProjectConfig{Symlinks: []string{"a/../../b"}}, true}, {"symlink bare ..", ProjectConfig{Symlinks: []string{".."}}, true}, + {"good reviewers", ProjectConfig{Reviewers: []ReviewerConfig{{Harness: ReviewerClaudeCode}}}, false}, + {"unknown reviewer harness", ProjectConfig{Reviewers: []ReviewerConfig{{Harness: "nope"}}}, true}, + {"worker harness is not auto a reviewer", ProjectConfig{Reviewers: []ReviewerConfig{{Harness: ReviewerHarness(HarnessCodex)}}}, true}, + {"empty reviewer harness", ProjectConfig{Reviewers: []ReviewerConfig{{Harness: ""}}}, true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -69,6 +73,25 @@ func TestProjectConfigWithDefaults(t *testing.T) { } } +func TestResolveReviewerHarness(t *testing.T) { + // A configured reviewer always wins, regardless of the worker harness. + cfg := ProjectConfig{Reviewers: []ReviewerConfig{{Harness: ReviewerClaudeCode}}} + if got := cfg.ResolveReviewerHarness(HarnessAider); got != ReviewerClaudeCode { + t.Fatalf("configured reviewer = %q, want claude-code", got) + } + + // No reviewer configured: reuse the worker harness when it is also a + // supported reviewer (claude-code is). + if got := (ProjectConfig{}).ResolveReviewerHarness(HarnessClaudeCode); got != ReviewerClaudeCode { + t.Fatalf("default = %q, want reviewer claude-code", got) + } + + // A worker harness that is not a supported reviewer falls back to claude-code. + if got := (ProjectConfig{}).ResolveReviewerHarness(HarnessAider); got != FallbackReviewerHarness { + t.Fatalf("fallback = %q, want %q", got, FallbackReviewerHarness) + } +} + func TestProjectConfigIsZero(t *testing.T) { if !(ProjectConfig{}).IsZero() { t.Fatal("empty config should be zero") diff --git a/backend/internal/domain/review.go b/backend/internal/domain/review.go new file mode 100644 index 00000000..a71556da --- /dev/null +++ b/backend/internal/domain/review.go @@ -0,0 +1,63 @@ +package domain + +import "time" + +// 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 +// live on ReviewRun. +type Review struct { + ID string `json:"id"` + SessionID SessionID `json:"sessionId"` + ProjectID ProjectID `json:"projectId"` + Harness ReviewerHarness `json:"harness"` + PRURL string `json:"prUrl"` + // ReviewerHandleID is the runtime handle of the live reviewer pane, reused + // across passes and exposed so the UI can attach its terminal. + ReviewerHandleID string `json:"reviewerHandleId"` + CreatedAt time.Time `json:"createdAt"` + UpdatedAt time.Time `json:"updatedAt"` +} + +// ReviewRun is one review pass against a worker's PR. +type ReviewRun struct { + ID string `json:"id"` + ReviewID string `json:"reviewId"` + SessionID SessionID `json:"sessionId"` + Harness ReviewerHarness `json:"harness"` + PRURL string `json:"prUrl"` + // TargetSHA is the PR head commit this pass reviewed. + TargetSHA string `json:"targetSha"` + Status ReviewRunStatus `json:"status"` + Verdict ReviewVerdict `json:"verdict"` + // Body is the review text the reviewer submitted. It is recorded for AO's + // own tracking; the reviewer also posts the review to the PR itself. + Body string `json:"body"` + CreatedAt time.Time `json:"createdAt"` +} + +// ReviewRunStatus is the lifecycle state of a single review pass. +type ReviewRunStatus string + +// Review run statuses. +const ( + ReviewRunRunning ReviewRunStatus = "running" + ReviewRunComplete ReviewRunStatus = "complete" + ReviewRunFailed ReviewRunStatus = "failed" +) + +// ReviewVerdict is the outcome a reviewer reports. The empty verdict marks a +// run that has not produced an outcome yet. +type ReviewVerdict string + +// Review verdicts. +const ( + VerdictNone ReviewVerdict = "" + VerdictApproved ReviewVerdict = "approved" + VerdictChangesRequested ReviewVerdict = "changes_requested" +) + +// Valid reports whether v is a verdict a reviewer may submit (the empty verdict +// is a stored default, not a submittable one). +func (v ReviewVerdict) Valid() bool { + return v == VerdictApproved || v == VerdictChangesRequested +} diff --git a/backend/internal/domain/reviewerharness.go b/backend/internal/domain/reviewerharness.go new file mode 100644 index 00000000..760be13e --- /dev/null +++ b/backend/internal/domain/reviewerharness.go @@ -0,0 +1,30 @@ +package domain + +// ReviewerHarness identifies a code-review agent. It is a separate vocabulary +// from AgentHarness on purpose: a reviewer-only tool (e.g. the Greptile CLI) +// must not become a valid worker, and a worker harness does not automatically +// become a valid reviewer. The two sets are maintained independently and only +// happen to share ids where the same tool serves both roles (claude-code). +type ReviewerHarness string + +// Supported reviewer harnesses. Add a reviewer-only tool here (and register its +// adapter) without widening the worker AgentHarness set. +const ( + ReviewerClaudeCode ReviewerHarness = "claude-code" +) + +// AllReviewerHarnesses is the canonical set used to validate a configured +// reviewer harness. +var AllReviewerHarnesses = []ReviewerHarness{ + ReviewerClaudeCode, +} + +// IsKnown reports whether h is one of the supported reviewer harnesses. +func (h ReviewerHarness) IsKnown() bool { + for _, k := range AllReviewerHarnesses { + if h == k { + return true + } + } + return false +} diff --git a/backend/internal/httpd/apispec/openapi.yaml b/backend/internal/httpd/apispec/openapi.yaml index d717a5c9..3e067d4d 100644 --- a/backend/internal/httpd/apispec/openapi.yaml +++ b/backend/internal/httpd/apispec/openapi.yaml @@ -493,95 +493,6 @@ paths: summary: Resolve review threads on a pull request tags: - prs - /api/v1/reviews: - get: - operationId: listReviews - responses: - "200": - content: - application/json: - schema: - $ref: '#/components/schemas/ListReviewsResponse' - description: OK - "501": - content: - application/json: - schema: - $ref: '#/components/schemas/APIError' - description: Not Implemented - summary: List code-review runs - tags: - - reviews - /api/v1/reviews/{id}/send: - post: - operationId: sendReview - parameters: - - description: Review run id. - in: path - name: id - required: true - schema: - description: Review run id. - type: string - responses: - "200": - content: - application/json: - schema: - $ref: '#/components/schemas/ReviewResponse' - description: OK - "404": - content: - application/json: - schema: - $ref: '#/components/schemas/APIError' - description: Not Found - "501": - content: - application/json: - schema: - $ref: '#/components/schemas/APIError' - description: Not Implemented - summary: Send a review run's findings to its PR - tags: - - reviews - /api/v1/reviews/execute: - post: - operationId: executeReview - requestBody: - content: - application/json: - schema: - $ref: '#/components/schemas/ExecuteReviewInput' - required: true - responses: - "201": - content: - application/json: - schema: - $ref: '#/components/schemas/ReviewResponse' - description: Created - "400": - content: - application/json: - schema: - $ref: '#/components/schemas/APIError' - description: Bad Request - "422": - content: - application/json: - schema: - $ref: '#/components/schemas/APIError' - description: Unprocessable Entity - "501": - content: - application/json: - schema: - $ref: '#/components/schemas/APIError' - description: Not Implemented - summary: Start a code-review run for a session's PR - tags: - - reviews /api/v1/sessions: get: operationId: listSessions @@ -990,6 +901,135 @@ paths: summary: Restore a terminated session tags: - sessions + /api/v1/sessions/{sessionId}/reviews: + get: + operationId: listReviews + parameters: + - description: Session identifier, e.g. project-1. + in: path + name: sessionId + required: true + schema: + description: Session identifier, e.g. project-1. + type: string + responses: + "200": + content: + application/json: + schema: + $ref: '#/components/schemas/ListReviewsResponse' + description: OK + "422": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Unprocessable Entity + "501": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Not Implemented + summary: List a worker's code-review runs + tags: + - reviews + /api/v1/sessions/{sessionId}/reviews/submit: + post: + operationId: submitReview + parameters: + - description: Session identifier, e.g. project-1. + in: path + name: sessionId + required: true + schema: + description: Session identifier, e.g. project-1. + type: string + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/SubmitReviewInput' + required: true + responses: + "200": + content: + application/json: + schema: + $ref: '#/components/schemas/ReviewRunResponse' + description: OK + "400": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Bad Request + "404": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Not Found + "422": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Unprocessable Entity + "501": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Not Implemented + summary: Record a reviewer's result for a worker's PR + tags: + - reviews + /api/v1/sessions/{sessionId}/reviews/trigger: + post: + operationId: triggerReview + parameters: + - description: Session identifier, e.g. project-1. + in: path + name: sessionId + required: true + schema: + description: Session identifier, e.g. project-1. + type: string + responses: + "200": + content: + application/json: + schema: + $ref: '#/components/schemas/ReviewRunResponse' + description: OK + "201": + content: + application/json: + schema: + $ref: '#/components/schemas/ReviewRunResponse' + description: Created + "404": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Not Found + "422": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Unprocessable Entity + "501": + content: + application/json: + schema: + $ref: '#/components/schemas/APIError' + description: Not Implemented + summary: Trigger a code review of a worker's PR + tags: + - reviews /api/v1/sessions/{sessionId}/rollback: post: operationId: rollbackSession @@ -1247,13 +1287,12 @@ components: - state - lastActivityAt type: object - ExecuteReviewInput: + DomainReviewerConfig: properties: - sessionId: - description: Session whose PR to review. + harness: type: string required: - - sessionId + - harness type: object KillSessionResponse: properties: @@ -1287,11 +1326,14 @@ components: type: object ListReviewsResponse: properties: + reviewerHandleId: + type: string reviews: items: $ref: '#/components/schemas/ReviewRun' type: array required: + - reviewerHandleId - reviews type: object ListSessionPRsResponse: @@ -1444,6 +1486,10 @@ components: items: type: string type: array + reviewers: + items: + $ref: '#/components/schemas/DomainReviewerConfig' + type: array sessionPrefix: type: string symlinks: @@ -1553,53 +1599,50 @@ components: - sessionId - session type: object - ReviewFinding: + ReviewRun: properties: body: type: string - id: - type: string - line: - type: integer - path: - type: string - severity: - type: string - required: - - id - - path - - line - - severity - - body - type: object - ReviewResponse: - properties: - review: - $ref: '#/components/schemas/ReviewRun' - required: - - review - type: object - ReviewRun: - properties: createdAt: format: date-time type: string - findings: - items: - $ref: '#/components/schemas/ReviewFinding' - type: array + harness: + type: string id: type: string + prUrl: + type: string + reviewId: + type: string sessionId: type: string status: type: string + targetSha: + type: string + verdict: + type: string required: - id + - reviewId - sessionId + - harness + - prUrl + - targetSha - status + - verdict + - body - createdAt - - findings + type: object + ReviewRunResponse: + properties: + review: + $ref: '#/components/schemas/ReviewRun' + reviewerHandleId: + type: string + required: + - review + - reviewerHandleId type: object RoleOverride: properties: @@ -1817,6 +1860,22 @@ components: required: - projectId type: object + SubmitReviewInput: + properties: + body: + description: Review body recorded by AO. Required for changes_requested. + type: string + runId: + description: Review run id being completed. + type: string + verdict: + description: 'Review verdict: approved or changes_requested.' + type: string + required: + - runId + - verdict + - body + type: object WorkspaceRepo: properties: name: diff --git a/backend/internal/httpd/apispec/specgen/build.go b/backend/internal/httpd/apispec/specgen/build.go index 5ef6b87d..2aeca734 100644 --- a/backend/internal/httpd/apispec/specgen/build.go +++ b/backend/internal/httpd/apispec/specgen/build.go @@ -168,11 +168,10 @@ var schemaNames = map[string]string{ "ControllersResolveCommentsResponse": "ResolveCommentsResponse", // httpd/controllers — review wire envelopes "ControllersListReviewsResponse": "ListReviewsResponse", - "ControllersExecuteReviewInput": "ExecuteReviewInput", - "ControllersReviewResponse": "ReviewResponse", - // service/review entities - "ReviewRun": "ReviewRun", - "ReviewFinding": "ReviewFinding", + "ControllersReviewRunResponse": "ReviewRunResponse", + "ControllersSubmitReviewInput": "SubmitReviewInput", + // domain review entities + "DomainReviewRun": "ReviewRun", // service/project entities + DTOs "ProjectProject": "Project", "ProjectSummary": "ProjectSummary", @@ -290,35 +289,42 @@ func notificationOperations() []operation { } } -// reviewOperations declares the /reviews operations. Must stay 1:1 with the -// routes ReviewsController.Register mounts (enforced by the parity test). +// reviewOperations declares the session-scoped /reviews operations. Must stay +// 1:1 with the routes ReviewsController.Register mounts (enforced by the parity +// test). func reviewOperations() []operation { return []operation{ { - method: http.MethodGet, path: "/api/v1/reviews", id: "listReviews", tag: "reviews", - summary: "List code-review runs", + 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{}}, resps: []respUnit{ {http.StatusOK, controllers.ListReviewsResponse{}}, + {http.StatusUnprocessableEntity, envelope.APIError{}}, {http.StatusNotImplemented, envelope.APIError{}}, }, }, { - method: http.MethodPost, path: "/api/v1/reviews/execute", id: "executeReview", tag: "reviews", - summary: "Start a code-review run for a session's PR", - reqBody: controllers.ExecuteReviewInput{}, + 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{}}, resps: []respUnit{ - {http.StatusCreated, controllers.ReviewResponse{}}, - {http.StatusBadRequest, envelope.APIError{}}, + {http.StatusOK, controllers.ReviewRunResponse{}}, + {http.StatusCreated, controllers.ReviewRunResponse{}}, {http.StatusUnprocessableEntity, envelope.APIError{}}, + {http.StatusNotFound, envelope.APIError{}}, {http.StatusNotImplemented, envelope.APIError{}}, }, }, { - method: http.MethodPost, path: "/api/v1/reviews/{id}/send", id: "sendReview", tag: "reviews", - summary: "Send a review run's findings to its PR", - pathParams: []any{controllers.ReviewIDParam{}}, + method: http.MethodPost, path: "/api/v1/sessions/{sessionId}/reviews/submit", id: "submitReview", tag: "reviews", + summary: "Record a reviewer's result for a worker's PR", + pathParams: []any{controllers.SessionIDParam{}}, + reqBody: controllers.SubmitReviewInput{}, resps: []respUnit{ - {http.StatusOK, controllers.ReviewResponse{}}, + {http.StatusOK, controllers.ReviewRunResponse{}}, + {http.StatusBadRequest, envelope.APIError{}}, + {http.StatusUnprocessableEntity, envelope.APIError{}}, {http.StatusNotFound, envelope.APIError{}}, {http.StatusNotImplemented, envelope.APIError{}}, }, diff --git a/backend/internal/httpd/controllers/reviews.go b/backend/internal/httpd/controllers/reviews.go index 5d7df6bd..b9fc3c68 100644 --- a/backend/internal/httpd/controllers/reviews.go +++ b/backend/internal/httpd/controllers/reviews.go @@ -7,88 +7,98 @@ import ( "github.com/go-chi/chi/v5" + "github.com/aoagents/agent-orchestrator/backend/internal/domain" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/apispec" "github.com/aoagents/agent-orchestrator/backend/internal/httpd/envelope" reviewsvc "github.com/aoagents/agent-orchestrator/backend/internal/service/review" ) -// ReviewIDParam is the {id} path parameter on the /reviews/{id} routes. -type ReviewIDParam struct { - ID string `path:"id" description:"Review run id."` -} - -// ListReviewsResponse is the body of GET /api/v1/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 { - Reviews []reviewsvc.Run `json:"reviews"` + ReviewerHandleID string `json:"reviewerHandleId"` + Reviews []domain.ReviewRun `json:"reviews"` } -// ExecuteReviewInput is the body of POST /api/v1/reviews/execute. -type ExecuteReviewInput struct { - SessionID string `json:"sessionId" description:"Session whose PR to review."` +// ReviewRunResponse is the body of trigger (200/201) and submit (200). It +// carries the run plus the reviewer pane handle so the UI can attach a terminal. +type ReviewRunResponse struct { + Review domain.ReviewRun `json:"review"` + ReviewerHandleID string `json:"reviewerHandleId"` } -// ReviewResponse is the { review } body of execute (201) and send (200). -type ReviewResponse struct { - Review reviewsvc.Run `json:"review"` +// 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."` + Verdict string `json:"verdict" description:"Review verdict: approved or changes_requested."` + Body string `json:"body" description:"Review body recorded by AO. Required for changes_requested."` } -// ReviewsController owns the /reviews routes. A nil Svc returns 501. +// ReviewsController owns the session-scoped /reviews routes. A nil Svc returns 501. type ReviewsController struct { Svc reviewsvc.Manager } // Register mounts the review routes on the supplied router. func (c *ReviewsController) Register(r chi.Router) { - r.Get("/reviews", c.list) - r.Post("/reviews/execute", c.execute) - r.Post("/reviews/{id}/send", c.send) + r.Get("/sessions/{sessionId}/reviews", c.list) + r.Post("/sessions/{sessionId}/reviews/trigger", c.trigger) + r.Post("/sessions/{sessionId}/reviews/submit", c.submit) } func (c *ReviewsController) list(w http.ResponseWriter, r *http.Request) { if c.Svc == nil { - apispec.NotImplemented(w, r, "GET", "/api/v1/reviews") + apispec.NotImplemented(w, r, "GET", "/api/v1/sessions/{sessionId}/reviews") return } - runs, err := c.Svc.List(r.Context()) + res, err := c.Svc.List(r.Context(), sessionID(r)) if err != nil { writeReviewError(w, r, err) return } + runs := res.Runs if runs == nil { - runs = []reviewsvc.Run{} + runs = []domain.ReviewRun{} } - envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{Reviews: runs}) + envelope.WriteJSON(w, http.StatusOK, ListReviewsResponse{ReviewerHandleID: res.ReviewerHandleID, Reviews: runs}) } -func (c *ReviewsController) execute(w http.ResponseWriter, r *http.Request) { +func (c *ReviewsController) trigger(w http.ResponseWriter, r *http.Request) { if c.Svc == nil { - apispec.NotImplemented(w, r, "POST", "/api/v1/reviews/execute") - return - } - var in ExecuteReviewInput - if err := json.NewDecoder(r.Body).Decode(&in); err != nil { - envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_BODY", "Invalid request body", nil) + apispec.NotImplemented(w, r, "POST", "/api/v1/sessions/{sessionId}/reviews/trigger") return } - run, err := c.Svc.Execute(r.Context(), in.SessionID) + res, err := c.Svc.Trigger(r.Context(), sessionID(r)) if err != nil { writeReviewError(w, r, err) return } - envelope.WriteJSON(w, http.StatusCreated, ReviewResponse{Review: run}) + // 201 when a new pass was started; 200 when an existing run for the same + // commit was reused. + status := http.StatusOK + if res.Created { + status = http.StatusCreated + } + envelope.WriteJSON(w, status, ReviewRunResponse{Review: res.Run, ReviewerHandleID: res.ReviewerHandleID}) } -func (c *ReviewsController) send(w http.ResponseWriter, r *http.Request) { +func (c *ReviewsController) submit(w http.ResponseWriter, r *http.Request) { if c.Svc == nil { - apispec.NotImplemented(w, r, "POST", "/api/v1/reviews/{id}/send") + apispec.NotImplemented(w, r, "POST", "/api/v1/sessions/{sessionId}/reviews/submit") + return + } + var in SubmitReviewInput + if err := json.NewDecoder(r.Body).Decode(&in); err != nil { + envelope.WriteAPIError(w, r, http.StatusBadRequest, "bad_request", "INVALID_BODY", "Invalid request body", nil) return } - run, err := c.Svc.Send(r.Context(), chi.URLParam(r, "id")) + run, err := c.Svc.Submit(r.Context(), sessionID(r), in.RunID, domain.ReviewVerdict(in.Verdict), in.Body) if err != nil { writeReviewError(w, r, err) return } - envelope.WriteJSON(w, http.StatusOK, ReviewResponse{Review: run}) + envelope.WriteJSON(w, http.StatusOK, ReviewRunResponse{Review: run}) } func writeReviewError(w http.ResponseWriter, r *http.Request, err error) { @@ -96,7 +106,7 @@ func writeReviewError(w http.ResponseWriter, r *http.Request, err error) { case errors.Is(err, reviewsvc.ErrInvalid): envelope.WriteAPIError(w, r, http.StatusUnprocessableEntity, "unprocessable", "REVIEW_INVALID", err.Error(), nil) case errors.Is(err, reviewsvc.ErrNotFound): - envelope.WriteAPIError(w, r, http.StatusNotFound, "not_found", "REVIEW_NOT_FOUND", "Unknown review run", nil) + envelope.WriteAPIError(w, r, http.StatusNotFound, "not_found", "REVIEW_NOT_FOUND", err.Error(), nil) default: envelope.WriteAPIError(w, r, http.StatusInternalServerError, "internal", "REVIEW_OPERATION_FAILED", "Review operation failed", nil) } diff --git a/backend/internal/ports/reviewer.go b/backend/internal/ports/reviewer.go new file mode 100644 index 00000000..a42df4d7 --- /dev/null +++ b/backend/internal/ports/reviewer.go @@ -0,0 +1,62 @@ +package ports + +import ( + "context" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// Reviewer is the contract a code-review adapter satisfies. It is deliberately +// separate from Agent: a reviewer is invoked once over a checkout to review a +// PR, and need not be a prompt-fed interactive agent. A prompt-driven reviewer +// (claude-code) builds its own prompt internally; a one-shot CLI (greptile) +// returns its own argv with no prompt at all. +type Reviewer interface { + // ReviewCommand builds the command (and any extra env) AO should run to + // spawn a fresh reviewer over the worker's checkout for a PR. + ReviewCommand(ctx context.Context, inv ReviewInvocation) (ReviewCommandSpec, error) + // ReviewMessage builds the text AO injects into an already-running reviewer + // pane to ask it to review a new commit. It must be self-contained (carry + // the ids the reviewer needs to submit) since AO passes no environment. + ReviewMessage(ctx context.Context, inv ReviewInvocation) (string, error) +} + +// ReviewInvocation describes one review pass for a reviewer to act on. All ids +// the reviewer needs are passed explicitly here (and embedded in the prompt / +// message), never through environment variables. +type ReviewInvocation struct { + // ReviewerID is a stable id for the reviewer's runtime instance (pane, + // native session id), derived from the worker session. + ReviewerID string + // RunID is the review_run this pass completes; the reviewer passes it to + // `ao review submit`. + RunID string + // WorkerSessionID is the worker whose PR is under review. + WorkerSessionID domain.SessionID + // PRURL is the pull request to review. + PRURL string + // TargetSHA is the PR head commit under review. + TargetSHA string + // WorkspacePath is the worker's checkout the reviewer reads. + WorkspacePath string + // Prompt and SystemPrompt are the review instructions AO authored centrally, + // mirroring the worker's LaunchConfig.Prompt / SystemPrompt split: SystemPrompt + // carries the standing reviewer role, Prompt the per-pass task. A prompt-driven + // adapter (claude-code) feeds them to the agent; a one-shot CLI reviewer may + // ignore them. + Prompt string + SystemPrompt string +} + +// ReviewCommandSpec is how to launch a reviewer: the argv and any extra env the +// adapter needs. AO supplies the workspace and review-tracking env around it. +type ReviewCommandSpec struct { + Argv []string + Env map[string]string +} + +// ReviewerResolver maps a reviewer harness onto its adapter. ok=false means no +// adapter is registered for that harness. +type ReviewerResolver interface { + Reviewer(harness domain.ReviewerHarness) (Reviewer, bool) +} diff --git a/backend/internal/review/launcher.go b/backend/internal/review/launcher.go new file mode 100644 index 00000000..28e312f1 --- /dev/null +++ b/backend/internal/review/launcher.go @@ -0,0 +1,128 @@ +package review + +import ( + "context" + "fmt" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +// Launcher spawns, re-notifies, and probes a reviewer over a worker's worktree. +// It is the side of the engine that talks to the reviewer registry and runtime; +// the engine owns the orchestration and persistence. +type Launcher interface { + // Spawn launches a fresh reviewer and returns the runtime handle id of the + // live pane (stable per worker, reused across passes). + Spawn(ctx context.Context, spec LaunchSpec) (handleID string, err error) + // Notify asks an already-running reviewer pane to review a new commit. + Notify(ctx context.Context, handleID string, spec LaunchSpec) error + // Alive reports whether a reviewer pane is still running. + Alive(ctx context.Context, handleID string) (bool, error) +} + +// LaunchSpec is the engine's request to (re)launch a reviewer for one pass. +type LaunchSpec struct { + RunID string + WorkerID domain.SessionID + Harness domain.ReviewerHarness + WorkspacePath string + PRURL string + TargetSHA string +} + +// reviewerRuntime is the runtime surface the launcher needs: create a pane, +// inject a message into a running pane, and probe liveness. The zellij runtime +// satisfies it. +type reviewerRuntime interface { + Create(ctx context.Context, cfg ports.RuntimeConfig) (ports.RuntimeHandle, error) + IsAlive(ctx context.Context, handle ports.RuntimeHandle) (bool, error) + SendMessage(ctx context.Context, handle ports.RuntimeHandle, message string) error +} + +// agentLauncher resolves a reviewer adapter from the registry and drives the +// runtime. The reviewer reuses the worker's worktree (a fresh session worktree +// would branch off the default branch and so would not contain the PR changes). +type agentLauncher struct { + reviewers ports.ReviewerResolver + runtime reviewerRuntime +} + +type preLaunchReviewer interface { + PreLaunch(ctx context.Context, inv ports.ReviewInvocation) error +} + +// NewLauncher builds the production reviewer launcher. +func NewLauncher(reviewers ports.ReviewerResolver, runtime reviewerRuntime) Launcher { + return &agentLauncher{reviewers: reviewers, runtime: runtime} +} + +// reviewerHandleID is the stable runtime handle for a worker's reviewer pane, so +// one live reviewer is reused across passes. +func reviewerHandleID(workerID domain.SessionID) string { + return "review-" + string(workerID) +} + +func (l *agentLauncher) invocation(spec LaunchSpec) ports.ReviewInvocation { + prompt, systemPrompt := reviewTexts(spec) + return ports.ReviewInvocation{ + ReviewerID: reviewerHandleID(spec.WorkerID), + RunID: spec.RunID, + WorkerSessionID: spec.WorkerID, + PRURL: spec.PRURL, + TargetSHA: spec.TargetSHA, + WorkspacePath: spec.WorkspacePath, + Prompt: prompt, + SystemPrompt: systemPrompt, + } +} + +func (l *agentLauncher) Spawn(ctx context.Context, spec LaunchSpec) (string, error) { + reviewer, ok := l.reviewers.Reviewer(spec.Harness) + if !ok { + return "", fmt.Errorf("no reviewer adapter for harness %q", spec.Harness) + } + handleID := reviewerHandleID(spec.WorkerID) + inv := l.invocation(spec) + if pl, ok := reviewer.(preLaunchReviewer); ok { + if err := pl.PreLaunch(ctx, inv); err != nil { + return "", fmt.Errorf("reviewer pre-launch: %w", err) + } + } + cmd, err := reviewer.ReviewCommand(ctx, inv) + if err != nil { + return "", fmt.Errorf("reviewer command: %w", err) + } + handle, err := l.runtime.Create(ctx, ports.RuntimeConfig{ + SessionID: domain.SessionID(handleID), + WorkspacePath: spec.WorkspacePath, + Argv: cmd.Argv, + Env: cmd.Env, + }) + if err != nil { + return "", fmt.Errorf("reviewer runtime: %w", err) + } + return handle.ID, nil +} + +func (l *agentLauncher) Notify(ctx context.Context, handleID string, spec LaunchSpec) error { + reviewer, ok := l.reviewers.Reviewer(spec.Harness) + if !ok { + return fmt.Errorf("no reviewer adapter for harness %q", spec.Harness) + } + msg, err := reviewer.ReviewMessage(ctx, l.invocation(spec)) + if err != nil { + return fmt.Errorf("reviewer message: %w", err) + } + if err := l.runtime.SendMessage(ctx, ports.RuntimeHandle{ID: handleID}, msg); err != nil { + return fmt.Errorf("notify reviewer: %w", err) + } + return nil +} + +func (l *agentLauncher) Alive(ctx context.Context, handleID string) (bool, error) { + if handleID == "" { + return false, nil + } + return l.runtime.IsAlive(ctx, ports.RuntimeHandle{ID: handleID}) +} diff --git a/backend/internal/review/launcher_test.go b/backend/internal/review/launcher_test.go new file mode 100644 index 00000000..0717841e --- /dev/null +++ b/backend/internal/review/launcher_test.go @@ -0,0 +1,144 @@ +package review + +import ( + "context" + "strings" + "testing" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/ports" +) + +type fakeReviewer struct { + gotInv ports.ReviewInvocation +} + +func (f *fakeReviewer) ReviewCommand(_ context.Context, inv ports.ReviewInvocation) (ports.ReviewCommandSpec, error) { + f.gotInv = inv + return ports.ReviewCommandSpec{Argv: []string{"greptile", "review"}}, nil +} +func (f *fakeReviewer) ReviewMessage(_ context.Context, inv ports.ReviewInvocation) (string, error) { + f.gotInv = inv + return "review run " + inv.RunID, nil +} + +type fakePreLaunchReviewer struct { + fakeReviewer + prelaunched bool + gotPre ports.ReviewInvocation +} + +func (f *fakePreLaunchReviewer) PreLaunch(_ context.Context, inv ports.ReviewInvocation) error { + f.prelaunched = true + f.gotPre = inv + return nil +} + +type fakeReviewerResolver struct { + reviewer ports.Reviewer + ok bool +} + +func (f fakeReviewerResolver) Reviewer(domain.ReviewerHarness) (ports.Reviewer, bool) { + return f.reviewer, f.ok +} + +type fakeRuntime struct { + createCfg ports.RuntimeConfig + sentMsg string + sentTo string + alive bool +} + +func (f *fakeRuntime) Create(_ context.Context, cfg ports.RuntimeConfig) (ports.RuntimeHandle, error) { + f.createCfg = cfg + return ports.RuntimeHandle{ID: string(cfg.SessionID)}, nil +} +func (f *fakeRuntime) IsAlive(_ context.Context, _ ports.RuntimeHandle) (bool, error) { + return f.alive, nil +} +func (f *fakeRuntime) SendMessage(_ context.Context, handle ports.RuntimeHandle, msg string) error { + f.sentTo = handle.ID + f.sentMsg = msg + return nil +} + +func launchSpec() LaunchSpec { + return LaunchSpec{ + RunID: "run-1", WorkerID: "mer-1", Harness: domain.ReviewerClaudeCode, + WorkspacePath: "/ws/mer-1", PRURL: "https://github.com/o/r/pull/1", TargetSHA: "sha1", + } +} + +func TestLauncherSpawnReturnsStableHandle(t *testing.T) { + reviewer := &fakeReviewer{} + rt := &fakeRuntime{} + l := NewLauncher(fakeReviewerResolver{reviewer: reviewer, ok: true}, rt) + + handle, err := l.Spawn(context.Background(), launchSpec()) + if err != nil { + t.Fatalf("Spawn: %v", err) + } + if handle != "review-mer-1" { + t.Fatalf("handle = %q, want review-mer-1", handle) + } + if rt.createCfg.WorkspacePath != "/ws/mer-1" || len(rt.createCfg.Argv) == 0 || rt.createCfg.Argv[0] != "greptile" { + t.Fatalf("create cfg = %+v", rt.createCfg) + } + // No environment is used to carry review identity. + if len(rt.createCfg.Env) != 0 { + t.Fatalf("expected no env, got %v", rt.createCfg.Env) + } + if reviewer.gotInv.RunID != "run-1" || reviewer.gotInv.TargetSHA != "sha1" || reviewer.gotInv.ReviewerID != "review-mer-1" { + t.Fatalf("invocation = %+v", reviewer.gotInv) + } +} + +func TestLauncherSpawnRunsReviewerPreLaunch(t *testing.T) { + reviewer := &fakePreLaunchReviewer{} + rt := &fakeRuntime{} + l := NewLauncher(fakeReviewerResolver{reviewer: reviewer, ok: true}, rt) + + if _, err := l.Spawn(context.Background(), launchSpec()); err != nil { + t.Fatalf("Spawn: %v", err) + } + if !reviewer.prelaunched { + t.Fatal("expected reviewer pre-launch to run") + } + if reviewer.gotPre.ReviewerID != "review-mer-1" || reviewer.gotPre.WorkspacePath != "/ws/mer-1" { + t.Fatalf("prelaunch invocation = %+v", reviewer.gotPre) + } + if rt.createCfg.WorkspacePath == "" { + t.Fatal("runtime should still be created after pre-launch") + } +} + +func TestLauncherNotifySendsMessageToHandle(t *testing.T) { + reviewer := &fakeReviewer{} + rt := &fakeRuntime{} + l := NewLauncher(fakeReviewerResolver{reviewer: reviewer, ok: true}, rt) + + if err := l.Notify(context.Background(), "review-mer-1", launchSpec()); err != nil { + t.Fatalf("Notify: %v", err) + } + if rt.sentTo != "review-mer-1" || !strings.Contains(rt.sentMsg, "run-1") { + t.Fatalf("sent to %q msg %q", rt.sentTo, rt.sentMsg) + } +} + +func TestLauncherAlive(t *testing.T) { + l := NewLauncher(fakeReviewerResolver{ok: true}, &fakeRuntime{alive: true}) + if ok, _ := l.Alive(context.Background(), "review-mer-1"); !ok { + t.Fatal("want alive true") + } + if ok, _ := l.Alive(context.Background(), ""); ok { + t.Fatal("empty handle should not be alive") + } +} + +func TestLauncherSpawnNoAdapter(t *testing.T) { + l := NewLauncher(fakeReviewerResolver{ok: false}, &fakeRuntime{}) + if _, err := l.Spawn(context.Background(), launchSpec()); err == nil || !strings.Contains(err.Error(), "no reviewer adapter") { + t.Fatalf("err = %v, want no-adapter", err) + } +} diff --git a/backend/internal/review/prompt.go b/backend/internal/review/prompt.go new file mode 100644 index 00000000..a054fbfd --- /dev/null +++ b/backend/internal/review/prompt.go @@ -0,0 +1,30 @@ +package review + +import "fmt" + +// reviewTexts returns the user-facing prompt and the system prompt to deliver to +// a reviewer, authored in one place — the reviewer analogue of +// session_manager.buildSpawnTexts. The standing reviewer role lives in the +// system prompt; the per-pass task (which PR/commit, and the exact submit +// command carrying the ids) lives in the prompt, so it is also what AO injects +// into an already-running reviewer to review a new commit. +// +// The texts are self-contained — they carry the ids the reviewer needs to +// submit — so no environment variables are required. +func reviewTexts(spec LaunchSpec) (prompt, systemPrompt string) { + systemPrompt = `## Code reviewer role + +You are an AO code reviewer. You review a single pull request's changes in the current checkout — do not start unrelated work. Inspect what the PR changed by diffing the checkout against the PR's base branch, and review for correctness bugs, missing error handling, security issues, test coverage, and clear deviations from the surrounding code's conventions. Prefer a few high-confidence findings over nitpicks. + +Post your review on the pull request using the available review tooling (request changes if it needs work, approve if it is ready), with inline comments for specific findings. Do not push commits, edit files, or modify the branch — review only.` + + prompt = fmt.Sprintf(`Review pull request %s (head commit %s). + +When done, write your full review to review.md and record the result with AO by running exactly: + + ao review submit --session %s --run %s --verdict --body review.md + +If you cannot post the review on the provider, still run the command above so the result is recorded.`, + spec.PRURL, spec.TargetSHA, spec.WorkerID, spec.RunID) + return prompt, systemPrompt +} diff --git a/backend/internal/review/review.go b/backend/internal/review/review.go new file mode 100644 index 00000000..9664b0a0 --- /dev/null +++ b/backend/internal/review/review.go @@ -0,0 +1,330 @@ +// Package review holds the core code-review logic: triggering a reviewer over a +// worker's worktree, recording review runs, and accepting submitted results. +// +// It is independent of any transport. The daemon's HTTP service +// (internal/service/review) is a thin boundary over this engine today, and the +// same engine can back an in-process CLI trigger later without going through the +// API. Transport-specific concerns (DTOs, error→status mapping) stay in the +// service/controller layers; the orchestration and run-id generation live here. +package review + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/google/uuid" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// ErrInvalid and ErrNotFound let the transport layer map failures to 422/404. +var ( + ErrInvalid = errors.New("review: invalid input") + ErrNotFound = errors.New("review: not found") +) + +// Store is the persistence surface the engine needs. *sqlite.Store satisfies it +// in production; tests use a fake. +type Store interface { + UpsertReview(ctx context.Context, r domain.Review) error + GetReviewBySession(ctx context.Context, id domain.SessionID) (domain.Review, bool, error) + InsertReviewRun(ctx context.Context, r domain.ReviewRun) error + UpdateReviewRunResult(ctx context.Context, id string, status domain.ReviewRunStatus, verdict domain.ReviewVerdict, body string) (bool, error) + GetReviewRun(ctx context.Context, id string) (domain.ReviewRun, bool, error) + GetReviewRunBySessionAndSHA(ctx context.Context, id domain.SessionID, targetSHA string) (domain.ReviewRun, bool, error) + ListReviewRunsBySession(ctx context.Context, id domain.SessionID) ([]domain.ReviewRun, error) +} + +// Sessions resolves the worker session under review. +type Sessions interface { + GetSession(ctx context.Context, id domain.SessionID) (domain.SessionRecord, bool, error) +} + +// PRs resolves the PR a worker owns. +type PRs interface { + ListPRsBySession(ctx context.Context, id domain.SessionID) ([]domain.PullRequest, error) +} + +// Projects resolves the per-project reviewer config. +type Projects interface { + GetProject(ctx context.Context, id string) (domain.ProjectRecord, bool, error) +} + +// Deps wires the engine. +type Deps struct { + Store Store + Sessions Sessions + PRs PRs + Projects Projects + Launcher Launcher + + // Clock and NewID are injectable for deterministic tests. + Clock func() time.Time + NewID func() string +} + +// Engine is the core code-review engine. +type Engine struct { + store Store + sessions Sessions + prs PRs + projects Projects + launcher Launcher + clock func() time.Time + newID func() string +} + +// New wires an Engine from its dependencies, defaulting the clock and id source. +func New(d Deps) *Engine { + clock := d.Clock + if clock == nil { + clock = func() time.Time { return time.Now().UTC() } + } + newID := d.NewID + if newID == nil { + newID = uuid.NewString + } + return &Engine{ + store: d.Store, + sessions: d.Sessions, + prs: d.PRs, + projects: d.Projects, + launcher: d.Launcher, + clock: clock, + newID: newID, + } +} + +// TriggerResult is the outcome of a trigger: the (new or existing) run, the live +// reviewer pane's handle so the UI can attach its terminal, and whether a new +// pass was started (false when an existing run for the same commit was reused). +type TriggerResult struct { + Run domain.ReviewRun + ReviewerHandleID string + Created bool +} + +// SessionReviews is a worker's review state: the live reviewer handle plus its +// recorded passes, newest first. +type SessionReviews struct { + ReviewerHandleID string + Runs []domain.ReviewRun +} + +// Trigger starts (or reuses) a review of a worker's PR at its current head: +// - if a run already exists for this commit, it is returned unchanged; +// - otherwise, if a live reviewer pane exists, it is messaged to review the +// new commit; if not, a fresh reviewer is spawned; +// - only after the reviewer is launched is the run recorded. +func (e *Engine) Trigger(ctx context.Context, workerID domain.SessionID) (TriggerResult, error) { + if workerID == "" { + return TriggerResult{}, fmt.Errorf("%w: worker session id is required", ErrInvalid) + } + worker, ok, err := e.sessions.GetSession(ctx, workerID) + if err != nil { + return TriggerResult{}, err + } + if !ok { + return TriggerResult{}, fmt.Errorf("%w: worker session %q", ErrNotFound, workerID) + } + if worker.IsTerminated { + return TriggerResult{}, fmt.Errorf("%w: worker session %q is terminated", ErrInvalid, workerID) + } + if worker.Metadata.WorkspacePath == "" { + return TriggerResult{}, fmt.Errorf("%w: worker session %q has no workspace to review", ErrInvalid, workerID) + } + + pr, err := e.workerPR(ctx, workerID) + if err != nil { + return TriggerResult{}, err + } + targetSHA := pr.HeadSHA + + review, hasReview, err := e.store.GetReviewBySession(ctx, workerID) + if err != nil { + return TriggerResult{}, err + } + + // Idempotency: a pass already exists for this commit — return it as-is. + if existing, ok, err := e.store.GetReviewRunBySessionAndSHA(ctx, workerID, targetSHA); err != nil { + return TriggerResult{}, err + } else if ok { + return TriggerResult{Run: existing, ReviewerHandleID: review.ReviewerHandleID, Created: false}, nil + } + + harness, err := e.reviewerHarness(ctx, worker) + if err != nil { + return TriggerResult{}, err + } + + now := e.clock() + runID := e.newID() + spec := LaunchSpec{ + RunID: runID, + WorkerID: workerID, + Harness: harness, + WorkspacePath: worker.Metadata.WorkspacePath, + PRURL: pr.URL, + TargetSHA: targetSHA, + } + + // Reuse a live reviewer pane if there is one; otherwise spawn a fresh one. + handleID := "" + if hasReview && review.ReviewerHandleID != "" { + alive, err := e.launcher.Alive(ctx, review.ReviewerHandleID) + if err != nil { + return TriggerResult{}, err + } + if alive { + if err := e.launcher.Notify(ctx, review.ReviewerHandleID, spec); err != nil { + return TriggerResult{}, fmt.Errorf("notify reviewer: %w", err) + } + handleID = review.ReviewerHandleID + } + } + if handleID == "" { + h, err := e.launcher.Spawn(ctx, spec) + if err != nil { + return TriggerResult{}, fmt.Errorf("launch reviewer: %w", err) + } + handleID = h + } + + // The reviewer is running; now record the pass. + review, err = e.upsertReview(ctx, worker, harness, pr.URL, handleID, now) + if err != nil { + return TriggerResult{}, err + } + run := domain.ReviewRun{ + ID: runID, + ReviewID: review.ID, + SessionID: workerID, + Harness: harness, + PRURL: pr.URL, + TargetSHA: targetSHA, + Status: domain.ReviewRunRunning, + Verdict: domain.VerdictNone, + CreatedAt: now, + } + if err := e.store.InsertReviewRun(ctx, run); err != nil { + return TriggerResult{}, err + } + return TriggerResult{Run: run, ReviewerHandleID: handleID, Created: true}, nil +} + +// Submit records the reviewer's result for a specific worker review pass: it +// marks the run complete and stores the verdict and body. AO does not post the +// review — the reviewer agent posts it to the PR itself. +func (e *Engine) Submit(ctx context.Context, workerID domain.SessionID, runID string, verdict domain.ReviewVerdict, body string) (domain.ReviewRun, error) { + if workerID == "" { + return domain.ReviewRun{}, fmt.Errorf("%w: worker session id is required", ErrInvalid) + } + if runID == "" { + return domain.ReviewRun{}, fmt.Errorf("%w: review run id is required", ErrInvalid) + } + if !verdict.Valid() { + return domain.ReviewRun{}, fmt.Errorf("%w: verdict must be %q or %q", ErrInvalid, domain.VerdictApproved, domain.VerdictChangesRequested) + } + if verdict == domain.VerdictChangesRequested && body == "" { + return domain.ReviewRun{}, fmt.Errorf("%w: a changes_requested review requires a body", ErrInvalid) + } + + run, ok, err := e.store.GetReviewRun(ctx, runID) + if err != nil { + return domain.ReviewRun{}, err + } + if !ok { + return domain.ReviewRun{}, fmt.Errorf("%w: review run %q", ErrNotFound, runID) + } + if run.SessionID != workerID { + return domain.ReviewRun{}, fmt.Errorf("%w: review run %q does not belong to worker %q", ErrInvalid, runID, workerID) + } + if run.Status != domain.ReviewRunRunning { + return domain.ReviewRun{}, fmt.Errorf("%w: review run %q is not running", ErrInvalid, runID) + } + + updated, err := e.store.UpdateReviewRunResult(ctx, run.ID, domain.ReviewRunComplete, verdict, body) + if err != nil { + return domain.ReviewRun{}, err + } + if !updated { + return domain.ReviewRun{}, fmt.Errorf("%w: review run %q is not running", ErrInvalid, runID) + } + run.Status = domain.ReviewRunComplete + run.Verdict = verdict + run.Body = body + return run, nil +} + +// List returns a worker's review state: the live reviewer handle and its passes. +func (e *Engine) List(ctx context.Context, workerID domain.SessionID) (SessionReviews, error) { + if workerID == "" { + return SessionReviews{}, fmt.Errorf("%w: worker session id is required", ErrInvalid) + } + runs, err := e.store.ListReviewRunsBySession(ctx, workerID) + if err != nil { + return SessionReviews{}, err + } + var handle string + if review, ok, err := e.store.GetReviewBySession(ctx, workerID); err != nil { + return SessionReviews{}, err + } else if ok { + handle = review.ReviewerHandleID + } + return SessionReviews{ReviewerHandleID: handle, Runs: runs}, nil +} + +func (e *Engine) workerPR(ctx context.Context, workerID domain.SessionID) (domain.PullRequest, error) { + prs, err := e.prs.ListPRsBySession(ctx, workerID) + if err != nil { + return domain.PullRequest{}, err + } + if len(prs) == 0 { + return domain.PullRequest{}, fmt.Errorf("%w: worker %q has no PR to review", ErrInvalid, workerID) + } + return prs[0], nil +} + +// reviewerHarness resolves which harness reviews the worker's PR: a configured +// reviewer wins, otherwise the worker's own harness is reused (falling back to +// claude-code), per domain.ResolveReviewerHarness. +func (e *Engine) reviewerHarness(ctx context.Context, worker domain.SessionRecord) (domain.ReviewerHarness, error) { + var cfg domain.ProjectConfig + if e.projects != nil { + if proj, ok, err := e.projects.GetProject(ctx, string(worker.ProjectID)); err != nil { + return "", err + } else if ok { + cfg = proj.Config + } + } + return cfg.ResolveReviewerHarness(worker.Harness), nil +} + +func (e *Engine) upsertReview(ctx context.Context, worker domain.SessionRecord, harness domain.ReviewerHarness, prURL, handleID string, now time.Time) (domain.Review, error) { + existing, ok, err := e.store.GetReviewBySession(ctx, worker.ID) + if err != nil { + return domain.Review{}, err + } + review := domain.Review{ + ID: e.newID(), + SessionID: worker.ID, + ProjectID: worker.ProjectID, + Harness: harness, + PRURL: prURL, + ReviewerHandleID: handleID, + CreatedAt: now, + UpdatedAt: now, + } + if ok { + // Reuse the existing row's identity and creation time; UpsertReview + // refreshes harness/pr_url/reviewer_handle_id/updated_at. + review.ID = existing.ID + review.CreatedAt = existing.CreatedAt + } + if err := e.store.UpsertReview(ctx, review); err != nil { + return domain.Review{}, err + } + return review, nil +} diff --git a/backend/internal/review/review_test.go b/backend/internal/review/review_test.go new file mode 100644 index 00000000..333bcdba --- /dev/null +++ b/backend/internal/review/review_test.go @@ -0,0 +1,315 @@ +package review + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +// --- fakes --- + +type fakeStore struct { + review *domain.Review + runs []domain.ReviewRun +} + +func (f *fakeStore) UpsertReview(_ context.Context, r domain.Review) error { + cp := r + f.review = &cp + return nil +} +func (f *fakeStore) GetReviewBySession(_ context.Context, _ domain.SessionID) (domain.Review, bool, error) { + if f.review == nil { + return domain.Review{}, false, nil + } + return *f.review, true, nil +} +func (f *fakeStore) InsertReviewRun(_ context.Context, r domain.ReviewRun) error { + f.runs = append(f.runs, r) + return nil +} +func (f *fakeStore) UpdateReviewRunResult(_ context.Context, id string, status domain.ReviewRunStatus, verdict domain.ReviewVerdict, body string) (bool, error) { + for i := range f.runs { + if f.runs[i].ID == id { + if f.runs[i].Status != domain.ReviewRunRunning { + return false, nil + } + f.runs[i].Status = status + f.runs[i].Verdict = verdict + f.runs[i].Body = body + return true, nil + } + } + return false, nil +} +func (f *fakeStore) GetReviewRun(_ context.Context, id string) (domain.ReviewRun, bool, error) { + for _, r := range f.runs { + if r.ID == id { + return r, true, nil + } + } + return domain.ReviewRun{}, false, nil +} +func (f *fakeStore) GetReviewRunBySessionAndSHA(_ context.Context, _ domain.SessionID, sha string) (domain.ReviewRun, bool, error) { + for i := len(f.runs) - 1; i >= 0; i-- { + if f.runs[i].TargetSHA == sha { + return f.runs[i], true, nil + } + } + return domain.ReviewRun{}, false, nil +} +func (f *fakeStore) ListReviewRunsBySession(_ context.Context, _ domain.SessionID) ([]domain.ReviewRun, error) { + return f.runs, nil +} + +type fakeSessions struct { + rec domain.SessionRecord + ok bool +} + +func (f fakeSessions) GetSession(_ context.Context, _ domain.SessionID) (domain.SessionRecord, bool, error) { + return f.rec, f.ok, nil +} + +type fakePRs struct{ prs []domain.PullRequest } + +func (f fakePRs) ListPRsBySession(_ context.Context, _ domain.SessionID) ([]domain.PullRequest, error) { + return f.prs, nil +} + +type fakeProjects struct{ cfg domain.ProjectConfig } + +func (f fakeProjects) GetProject(_ context.Context, id string) (domain.ProjectRecord, bool, error) { + return domain.ProjectRecord{ID: id, Config: f.cfg}, true, nil +} + +type fakeLauncher struct { + handle string + alive bool + spawnErr error + notifyErr error + spawned bool + notified bool + gotSpec LaunchSpec + gotHandle string +} + +func (f *fakeLauncher) Spawn(_ context.Context, spec LaunchSpec) (string, error) { + f.spawned = true + f.gotSpec = spec + if f.spawnErr != nil { + return "", f.spawnErr + } + return f.handle, nil +} +func (f *fakeLauncher) Notify(_ context.Context, handleID string, spec LaunchSpec) error { + f.notified = true + f.gotHandle = handleID + f.gotSpec = spec + return f.notifyErr +} +func (f *fakeLauncher) Alive(_ context.Context, _ string) (bool, error) { return f.alive, nil } + +func liveWorker() domain.SessionRecord { + return domain.SessionRecord{ + ID: "mer-1", + ProjectID: "mer", + Harness: domain.HarnessClaudeCode, + Metadata: domain.SessionMetadata{WorkspacePath: "/ws/mer-1"}, + } +} + +func newEngineForTest(store Store, sessions Sessions, prs PRs, projects Projects, launcher Launcher) *Engine { + ids := 0 + return New(Deps{ + Store: store, Sessions: sessions, PRs: prs, Projects: projects, Launcher: launcher, + Clock: func() time.Time { return time.Unix(0, 0).UTC() }, + NewID: func() string { ids++; return "id-" + string(rune('0'+ids)) }, + }) +} + +func prAt(sha string) fakePRs { + return fakePRs{prs: []domain.PullRequest{{URL: "https://github.com/o/r/pull/1", HeadSHA: sha}}} +} + +// --- tests --- + +func TestTriggerSpawnsNewReviewerAndRecordsRunAfterLaunch(t *testing.T) { + store := &fakeStore{} + launcher := &fakeLauncher{handle: "review-mer-1"} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + res, err := eng.Trigger(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Trigger: %v", err) + } + if !res.Created || res.ReviewerHandleID != "review-mer-1" { + t.Fatalf("result = %+v", res) + } + if !launcher.spawned || launcher.notified { + t.Fatalf("expected spawn (no live reviewer): %+v", launcher) + } + if res.Run.TargetSHA != "sha1" || res.Run.Status != domain.ReviewRunRunning || res.Run.Harness != domain.ReviewerClaudeCode { + t.Fatalf("run = %+v", res.Run) + } + if launcher.gotSpec.RunID != res.Run.ID { + t.Fatalf("launch spec run id %q != run id %q", launcher.gotSpec.RunID, res.Run.ID) + } + if len(store.runs) != 1 || store.review == nil || store.review.ReviewerHandleID != "review-mer-1" { + t.Fatalf("persisted review=%+v runs=%+v", store.review, store.runs) + } +} + +func TestTriggerIsIdempotentForSameCommit(t *testing.T) { + store := &fakeStore{ + review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, + runs: []domain.ReviewRun{{ID: "run-1", SessionID: "mer-1", TargetSHA: "sha1", Status: domain.ReviewRunRunning}}, + } + launcher := &fakeLauncher{} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + res, err := eng.Trigger(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Trigger: %v", err) + } + if res.Created || res.Run.ID != "run-1" || res.ReviewerHandleID != "review-mer-1" { + t.Fatalf("expected reuse of existing run: %+v", res) + } + if launcher.spawned || launcher.notified { + t.Fatalf("should not launch for an already-reviewed commit: %+v", launcher) + } + if len(store.runs) != 1 { + t.Fatalf("should not insert another run: %+v", store.runs) + } +} + +func TestTriggerNotifiesLiveReviewerOnNewCommit(t *testing.T) { + store := &fakeStore{ + review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, + runs: []domain.ReviewRun{{ID: "run-0", SessionID: "mer-1", TargetSHA: "sha0", Status: domain.ReviewRunComplete}}, + } + launcher := &fakeLauncher{alive: true} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + res, err := eng.Trigger(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Trigger: %v", err) + } + if !launcher.notified || launcher.spawned { + t.Fatalf("expected notify on live reviewer: %+v", launcher) + } + if launcher.gotHandle != "review-mer-1" { + t.Fatalf("notify handle = %q", launcher.gotHandle) + } + if !res.Created || res.Run.TargetSHA != "sha1" || len(store.runs) != 2 { + t.Fatalf("expected a new run for sha1: res=%+v runs=%+v", res, store.runs) + } +} + +func TestTriggerSpawnsWhenReviewerDead(t *testing.T) { + store := &fakeStore{ + review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, + runs: []domain.ReviewRun{{ID: "run-0", SessionID: "mer-1", TargetSHA: "sha0", Status: domain.ReviewRunComplete}}, + } + launcher := &fakeLauncher{alive: false, handle: "review-mer-1"} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + if _, err := eng.Trigger(context.Background(), "mer-1"); err != nil { + t.Fatalf("Trigger: %v", err) + } + if !launcher.spawned || launcher.notified { + t.Fatalf("expected spawn when reviewer dead: %+v", launcher) + } +} + +func TestTriggerLaunchFailureRecordsNothing(t *testing.T) { + store := &fakeStore{} + launcher := &fakeLauncher{spawnErr: errors.New("boom")} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, launcher) + + if _, err := eng.Trigger(context.Background(), "mer-1"); err == nil { + t.Fatal("want launch error") + } + if len(store.runs) != 0 || store.review != nil { + t.Fatalf("nothing should be persisted on launch failure: review=%+v runs=%+v", store.review, store.runs) + } +} + +func TestTriggerUsesConfiguredReviewerHarness(t *testing.T) { + store := &fakeStore{} + projects := fakeProjects{cfg: domain.ProjectConfig{Reviewers: []domain.ReviewerConfig{{Harness: domain.ReviewerHarness("greptile")}}}} + launcher := &fakeLauncher{handle: "review-mer-1"} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), projects, launcher) + + res, err := eng.Trigger(context.Background(), "mer-1") + if err != nil { + t.Fatalf("Trigger: %v", err) + } + if res.Run.Harness != domain.ReviewerHarness("greptile") || launcher.gotSpec.Harness != domain.ReviewerHarness("greptile") { + t.Fatalf("harness not used: run=%+v spec=%+v", res.Run, launcher.gotSpec) + } +} + +func TestTriggerRejectsBadWorkerState(t *testing.T) { + t.Run("unknown worker", func(t *testing.T) { + eng := newEngineForTest(&fakeStore{}, fakeSessions{ok: false}, prAt("sha1"), fakeProjects{}, &fakeLauncher{}) + if _, err := eng.Trigger(context.Background(), "mer-1"); !errors.Is(err, ErrNotFound) { + t.Fatalf("err = %v, want ErrNotFound", err) + } + }) + t.Run("no pr", func(t *testing.T) { + eng := newEngineForTest(&fakeStore{}, fakeSessions{rec: liveWorker(), ok: true}, fakePRs{}, fakeProjects{}, &fakeLauncher{}) + if _, err := eng.Trigger(context.Background(), "mer-1"); !errors.Is(err, ErrInvalid) { + t.Fatalf("err = %v, want ErrInvalid", err) + } + }) +} + +func TestSubmitRecordsVerdictAndBody(t *testing.T) { + store := &fakeStore{runs: []domain.ReviewRun{{ID: "run-1", SessionID: "mer-1", Status: domain.ReviewRunRunning}}} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, &fakeLauncher{}) + + run, err := eng.Submit(context.Background(), "mer-1", "run-1", domain.VerdictChangesRequested, "please fix") + if err != nil { + t.Fatalf("Submit: %v", err) + } + if run.Status != domain.ReviewRunComplete || run.Verdict != domain.VerdictChangesRequested || run.Body != "please fix" { + t.Fatalf("run = %+v", run) + } +} + +func TestSubmitValidationAndOwnership(t *testing.T) { + store := &fakeStore{runs: []domain.ReviewRun{{ID: "run-1", SessionID: "other", Status: domain.ReviewRunRunning}}} + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, &fakeLauncher{}) + + if _, err := eng.Submit(context.Background(), "mer-1", "", domain.VerdictApproved, ""); !errors.Is(err, ErrInvalid) { + t.Fatalf("missing run id err = %v", err) + } + if _, err := eng.Submit(context.Background(), "mer-1", "run-1", "garbage", "b"); !errors.Is(err, ErrInvalid) { + t.Fatalf("bad verdict err = %v", err) + } + if _, err := eng.Submit(context.Background(), "mer-1", "missing", domain.VerdictApproved, ""); !errors.Is(err, ErrNotFound) { + t.Fatalf("unknown run err = %v", err) + } + if _, err := eng.Submit(context.Background(), "mer-1", "run-1", domain.VerdictApproved, ""); !errors.Is(err, ErrInvalid) { + t.Fatalf("ownership err = %v", err) + } +} + +func TestListReturnsHandleAndRuns(t *testing.T) { + store := &fakeStore{ + review: &domain.Review{ID: "rev-1", SessionID: "mer-1", ReviewerHandleID: "review-mer-1"}, + runs: []domain.ReviewRun{{ID: "run-1", SessionID: "mer-1", TargetSHA: "sha1"}}, + } + eng := newEngineForTest(store, fakeSessions{rec: liveWorker(), ok: true}, prAt("sha1"), fakeProjects{}, &fakeLauncher{}) + got, err := eng.List(context.Background(), "mer-1") + if err != nil { + t.Fatalf("List: %v", err) + } + if got.ReviewerHandleID != "review-mer-1" || len(got.Runs) != 1 { + t.Fatalf("list = %+v", got) + } +} diff --git a/backend/internal/service/review/review.go b/backend/internal/service/review/review.go index 2e23c6b8..1bfc8e3f 100644 --- a/backend/internal/service/review/review.go +++ b/backend/internal/service/review/review.go @@ -1,108 +1,53 @@ -// Package review is the daemon's code-review surface: review runs against a -// session's PR and the findings they produce. -// -// This is an in-memory implementation. Execution is not yet wired to a real -// review agent — Execute records a pending run so the HTTP surface is live and -// the dashboard renders against real endpoints; agent-backed findings and -// persistence are a follow-up. Mirrors agent-orchestrator's reviews feature -// (packages/web/src/app/api/reviews) on reverbcode's daemon. +// Package review is the daemon's HTTP-facing code-review service boundary. The +// core orchestration lives in internal/review; this layer is the thin contract +// the API controller depends on and delegates to the engine, so the same engine +// can also back a future in-process CLI trigger. package review import ( "context" - "errors" - "fmt" - "sync" - "time" -) -// ErrInvalid and ErrNotFound let the HTTP layer map service failures to 422/404. -var ( - ErrInvalid = errors.New("review: invalid input") - ErrNotFound = errors.New("review: not found") + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + reviewcore "github.com/aoagents/agent-orchestrator/backend/internal/review" ) -// Severity ranks a finding by how much it should block the human; one of -// "info" | "warning" | "error". Kept as a plain string on the wire. -const ( - SeverityInfo = "info" - SeverityWarning = "warning" - SeverityError = "error" +// ErrInvalid and ErrNotFound re-export the engine sentinels so the HTTP +// controller maps service failures to 422/404 without importing the core. +var ( + ErrInvalid = reviewcore.ErrInvalid + ErrNotFound = reviewcore.ErrNotFound ) -// Finding is one review comment produced for a run. -type Finding struct { - ID string `json:"id"` - Path string `json:"path"` - Line int `json:"line"` - Severity string `json:"severity"` - Body string `json:"body"` -} - -// Run is one code-review execution against a session's PR. -type Run struct { - ID string `json:"id"` - SessionID string `json:"sessionId"` - // Status is one of: pending | complete | sent. - Status string `json:"status"` - CreatedAt time.Time `json:"createdAt"` - Findings []Finding `json:"findings"` -} - // Manager is the reviews surface the HTTP controller depends on. type Manager interface { - List(ctx context.Context) ([]Run, error) - Execute(ctx context.Context, sessionID string) (Run, error) - Send(ctx context.Context, id string) (Run, error) + Trigger(ctx context.Context, workerID domain.SessionID) (reviewcore.TriggerResult, error) + Submit(ctx context.Context, workerID domain.SessionID, runID string, verdict domain.ReviewVerdict, body string) (domain.ReviewRun, error) + List(ctx context.Context, workerID domain.SessionID) (reviewcore.SessionReviews, error) } -type memStore struct { - mu sync.Mutex - runs map[string]*Run - seq int +// Service is the API-facing review service. It delegates to the core engine. +type Service struct { + engine *reviewcore.Engine } -// NewInMemory returns an in-memory Manager. Runs do not survive a daemon -// restart. -func NewInMemory() Manager { - return &memStore{runs: map[string]*Run{}} +var _ Manager = (*Service)(nil) + +// New wraps a core review engine as the API-facing service. +func New(engine *reviewcore.Engine) *Service { + return &Service{engine: engine} } -func (s *memStore) List(_ context.Context) ([]Run, error) { - s.mu.Lock() - defer s.mu.Unlock() - out := make([]Run, 0, len(s.runs)) - for _, run := range s.runs { - out = append(out, *run) - } - return out, nil +// Trigger starts (or reuses) a review pass for a worker's PR. +func (s *Service) Trigger(ctx context.Context, workerID domain.SessionID) (reviewcore.TriggerResult, error) { + return s.engine.Trigger(ctx, workerID) } -func (s *memStore) Execute(_ context.Context, sessionID string) (Run, error) { - if sessionID == "" { - return Run{}, fmt.Errorf("%w: sessionId is required", ErrInvalid) - } - s.mu.Lock() - defer s.mu.Unlock() - s.seq++ - run := &Run{ - ID: fmt.Sprintf("rev-%d", s.seq), - SessionID: sessionID, - Status: "pending", - CreatedAt: time.Now().UTC(), - Findings: []Finding{}, - } - s.runs[run.ID] = run - return *run, nil +// Submit records a reviewer's result for a specific worker review pass. +func (s *Service) Submit(ctx context.Context, workerID domain.SessionID, runID string, verdict domain.ReviewVerdict, body string) (domain.ReviewRun, error) { + return s.engine.Submit(ctx, workerID, runID, verdict, body) } -func (s *memStore) Send(_ context.Context, id string) (Run, error) { - s.mu.Lock() - defer s.mu.Unlock() - run, ok := s.runs[id] - if !ok { - return Run{}, fmt.Errorf("%w: review %q", ErrNotFound, id) - } - run.Status = "sent" - return *run, nil +// List returns a worker's review state. +func (s *Service) List(ctx context.Context, workerID domain.SessionID) (reviewcore.SessionReviews, error) { + return s.engine.List(ctx, workerID) } diff --git a/backend/internal/storage/sqlite/gen/models.go b/backend/internal/storage/sqlite/gen/models.go index 26610fe6..589bfed0 100644 --- a/backend/internal/storage/sqlite/gen/models.go +++ b/backend/internal/storage/sqlite/gen/models.go @@ -123,6 +123,30 @@ type Project struct { Kind string } +type Review struct { + ID string + SessionID domain.SessionID + ProjectID domain.ProjectID + Harness domain.ReviewerHarness + PRURL string + ReviewerHandleID string + CreatedAt time.Time + UpdatedAt time.Time +} + +type ReviewRun struct { + ID string + ReviewID string + SessionID domain.SessionID + Harness domain.ReviewerHarness + PRURL string + TargetSha string + Status domain.ReviewRunStatus + Verdict domain.ReviewVerdict + Body string + CreatedAt time.Time +} + type Session struct { ID domain.SessionID ProjectID domain.ProjectID diff --git a/backend/internal/storage/sqlite/gen/review.sql.go b/backend/internal/storage/sqlite/gen/review.sql.go new file mode 100644 index 00000000..c075e2b5 --- /dev/null +++ b/backend/internal/storage/sqlite/gen/review.sql.go @@ -0,0 +1,217 @@ +// Code generated by sqlc. DO NOT EDIT. +// versions: +// sqlc v1.31.1 +// source: review.sql + +package gen + +import ( + "context" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +const getReviewBySession = `-- name: GetReviewBySession :one +SELECT id, session_id, project_id, harness, pr_url, reviewer_handle_id, created_at, updated_at +FROM review WHERE session_id = ? +` + +func (q *Queries) GetReviewBySession(ctx context.Context, sessionID domain.SessionID) (Review, error) { + row := q.db.QueryRowContext(ctx, getReviewBySession, sessionID) + var i Review + err := row.Scan( + &i.ID, + &i.SessionID, + &i.ProjectID, + &i.Harness, + &i.PRURL, + &i.ReviewerHandleID, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} + +const getReviewRun = `-- name: GetReviewRun :one +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at +FROM review_run WHERE id = ? +` + +func (q *Queries) GetReviewRun(ctx context.Context, id string) (ReviewRun, error) { + row := q.db.QueryRowContext(ctx, getReviewRun, id) + var i ReviewRun + err := row.Scan( + &i.ID, + &i.ReviewID, + &i.SessionID, + &i.Harness, + &i.PRURL, + &i.TargetSha, + &i.Status, + &i.Verdict, + &i.Body, + &i.CreatedAt, + ) + return i, err +} + +const getReviewRunBySessionAndSHA = `-- name: GetReviewRunBySessionAndSHA :one +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at +FROM review_run WHERE session_id = ? AND target_sha = ? ORDER BY created_at DESC LIMIT 1 +` + +type GetReviewRunBySessionAndSHAParams struct { + SessionID domain.SessionID + TargetSha string +} + +func (q *Queries) GetReviewRunBySessionAndSHA(ctx context.Context, arg GetReviewRunBySessionAndSHAParams) (ReviewRun, error) { + row := q.db.QueryRowContext(ctx, getReviewRunBySessionAndSHA, arg.SessionID, arg.TargetSha) + var i ReviewRun + err := row.Scan( + &i.ID, + &i.ReviewID, + &i.SessionID, + &i.Harness, + &i.PRURL, + &i.TargetSha, + &i.Status, + &i.Verdict, + &i.Body, + &i.CreatedAt, + ) + return i, err +} + +const insertReviewRun = `-- name: InsertReviewRun :exec +INSERT INTO review_run (id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at) +VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) +` + +type InsertReviewRunParams struct { + ID string + ReviewID string + SessionID domain.SessionID + Harness domain.ReviewerHarness + PRURL string + TargetSha string + Status domain.ReviewRunStatus + Verdict domain.ReviewVerdict + Body string + CreatedAt time.Time +} + +func (q *Queries) InsertReviewRun(ctx context.Context, arg InsertReviewRunParams) error { + _, err := q.db.ExecContext(ctx, insertReviewRun, + arg.ID, + arg.ReviewID, + arg.SessionID, + arg.Harness, + arg.PRURL, + arg.TargetSha, + arg.Status, + arg.Verdict, + arg.Body, + arg.CreatedAt, + ) + return err +} + +const listReviewRunsBySession = `-- name: ListReviewRunsBySession :many +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at +FROM review_run WHERE session_id = ? ORDER BY created_at DESC +` + +func (q *Queries) ListReviewRunsBySession(ctx context.Context, sessionID domain.SessionID) ([]ReviewRun, error) { + rows, err := q.db.QueryContext(ctx, listReviewRunsBySession, sessionID) + if err != nil { + return nil, err + } + defer rows.Close() + items := []ReviewRun{} + for rows.Next() { + var i ReviewRun + if err := rows.Scan( + &i.ID, + &i.ReviewID, + &i.SessionID, + &i.Harness, + &i.PRURL, + &i.TargetSha, + &i.Status, + &i.Verdict, + &i.Body, + &i.CreatedAt, + ); 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 updateReviewRunResult = `-- name: UpdateReviewRunResult :execrows +UPDATE review_run SET status = ?, verdict = ?, body = ? WHERE id = ? AND status = 'running' +` + +type UpdateReviewRunResultParams struct { + Status domain.ReviewRunStatus + Verdict domain.ReviewVerdict + Body string + ID string +} + +func (q *Queries) UpdateReviewRunResult(ctx context.Context, arg UpdateReviewRunResultParams) (int64, error) { + result, err := q.db.ExecContext(ctx, updateReviewRunResult, + arg.Status, + arg.Verdict, + arg.Body, + arg.ID, + ) + if err != nil { + return 0, err + } + return result.RowsAffected() +} + +const upsertReview = `-- name: UpsertReview :exec +INSERT INTO review (id, session_id, project_id, harness, pr_url, reviewer_handle_id, created_at, updated_at) +VALUES (?, ?, ?, ?, ?, ?, ?, ?) +ON CONFLICT (session_id) DO UPDATE SET + harness = excluded.harness, + pr_url = excluded.pr_url, + reviewer_handle_id = excluded.reviewer_handle_id, + updated_at = excluded.updated_at +` + +type UpsertReviewParams struct { + ID string + SessionID domain.SessionID + ProjectID domain.ProjectID + Harness domain.ReviewerHarness + PRURL string + ReviewerHandleID string + CreatedAt time.Time + UpdatedAt time.Time +} + +func (q *Queries) UpsertReview(ctx context.Context, arg UpsertReviewParams) error { + _, err := q.db.ExecContext(ctx, upsertReview, + arg.ID, + arg.SessionID, + arg.ProjectID, + arg.Harness, + arg.PRURL, + arg.ReviewerHandleID, + arg.CreatedAt, + arg.UpdatedAt, + ) + return err +} diff --git a/backend/internal/storage/sqlite/migrations/0012_add_review_tables.sql b/backend/internal/storage/sqlite/migrations/0012_add_review_tables.sql new file mode 100644 index 00000000..88419a3a --- /dev/null +++ b/backend/internal/storage/sqlite/migrations/0012_add_review_tables.sql @@ -0,0 +1,45 @@ +-- Configurable AO code review (issue #192). review holds one row per worker +-- session under review (session_id UNIQUE); a repeat trigger reuses the row. +-- review_run holds the per-pass facts. The reviewer agent posts its review to +-- the PR itself; `ao review submit` records the verdict and body on the run. + +-- +goose Up +-- +goose StatementBegin +CREATE TABLE review ( + id TEXT PRIMARY KEY, + session_id TEXT NOT NULL UNIQUE REFERENCES sessions (id) ON DELETE CASCADE, + project_id TEXT NOT NULL REFERENCES projects (id), + harness TEXT NOT NULL, + pr_url TEXT NOT NULL DEFAULT '', + -- runtime handle id of the live reviewer pane, reused across passes and + -- exposed so the UI can attach its terminal over /mux. + reviewer_handle_id TEXT NOT NULL DEFAULT '', + created_at TIMESTAMP NOT NULL, + updated_at TIMESTAMP NOT NULL +); +-- +goose StatementEnd + +-- +goose StatementBegin +CREATE TABLE review_run ( + id TEXT PRIMARY KEY, + review_id TEXT NOT NULL REFERENCES review (id) ON DELETE CASCADE, + session_id TEXT NOT NULL REFERENCES sessions (id) ON DELETE CASCADE, + harness TEXT NOT NULL, + pr_url TEXT NOT NULL DEFAULT '', + -- the commit the pass reviewed; lets a repeat trigger for the same head + -- short-circuit to the existing run. + target_sha TEXT NOT NULL DEFAULT '', + status TEXT NOT NULL DEFAULT 'running', + verdict TEXT NOT NULL DEFAULT '', + body TEXT NOT NULL DEFAULT '', + created_at TIMESTAMP NOT NULL +); +-- +goose StatementEnd + +-- +goose Down +-- +goose StatementBegin +DROP TABLE review_run; +-- +goose StatementEnd +-- +goose StatementBegin +DROP TABLE review; +-- +goose StatementEnd diff --git a/backend/internal/storage/sqlite/queries/review.sql b/backend/internal/storage/sqlite/queries/review.sql new file mode 100644 index 00000000..1151c946 --- /dev/null +++ b/backend/internal/storage/sqlite/queries/review.sql @@ -0,0 +1,31 @@ +-- name: UpsertReview :exec +INSERT INTO review (id, session_id, project_id, harness, pr_url, reviewer_handle_id, created_at, updated_at) +VALUES (?, ?, ?, ?, ?, ?, ?, ?) +ON CONFLICT (session_id) DO UPDATE SET + harness = excluded.harness, + pr_url = excluded.pr_url, + reviewer_handle_id = excluded.reviewer_handle_id, + updated_at = excluded.updated_at; + +-- name: GetReviewBySession :one +SELECT id, session_id, project_id, harness, pr_url, reviewer_handle_id, created_at, updated_at +FROM review WHERE session_id = ?; + +-- name: InsertReviewRun :exec +INSERT INTO review_run (id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at) +VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?); + +-- name: UpdateReviewRunResult :execrows +UPDATE review_run SET status = ?, verdict = ?, body = ? WHERE id = ? AND status = 'running'; + +-- name: GetReviewRun :one +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at +FROM review_run WHERE id = ?; + +-- name: GetReviewRunBySessionAndSHA :one +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at +FROM review_run WHERE session_id = ? AND target_sha = ? ORDER BY created_at DESC LIMIT 1; + +-- name: ListReviewRunsBySession :many +SELECT id, review_id, session_id, harness, pr_url, target_sha, status, verdict, body, created_at +FROM review_run WHERE session_id = ? ORDER BY created_at DESC; diff --git a/backend/internal/storage/sqlite/store/review_store.go b/backend/internal/storage/sqlite/store/review_store.go new file mode 100644 index 00000000..36dfc9c7 --- /dev/null +++ b/backend/internal/storage/sqlite/store/review_store.go @@ -0,0 +1,141 @@ +package store + +import ( + "context" + "database/sql" + "errors" + "fmt" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" + "github.com/aoagents/agent-orchestrator/backend/internal/storage/sqlite/gen" +) + +// UpsertReview inserts the per-worker review row, or reuses the existing one +// (session_id is unique) by refreshing its harness/pr_url/updated_at. +func (s *Store) UpsertReview(ctx context.Context, r domain.Review) error { + s.writeMu.Lock() + defer s.writeMu.Unlock() + return s.qw.UpsertReview(ctx, gen.UpsertReviewParams{ + ID: r.ID, + SessionID: r.SessionID, + ProjectID: r.ProjectID, + Harness: r.Harness, + PRURL: r.PRURL, + ReviewerHandleID: r.ReviewerHandleID, + CreatedAt: r.CreatedAt, + UpdatedAt: r.UpdatedAt, + }) +} + +// GetReviewBySession returns the review row for a worker session, ok=false if none. +func (s *Store) GetReviewBySession(ctx context.Context, id domain.SessionID) (domain.Review, bool, error) { + row, err := s.qr.GetReviewBySession(ctx, id) + if errors.Is(err, sql.ErrNoRows) { + return domain.Review{}, false, nil + } + if err != nil { + return domain.Review{}, false, fmt.Errorf("get review by session %s: %w", id, err) + } + return reviewFromRow(row), true, nil +} + +// InsertReviewRun records a new review pass. +func (s *Store) InsertReviewRun(ctx context.Context, r domain.ReviewRun) error { + s.writeMu.Lock() + defer s.writeMu.Unlock() + return s.qw.InsertReviewRun(ctx, gen.InsertReviewRunParams{ + ID: r.ID, + ReviewID: r.ReviewID, + SessionID: r.SessionID, + Harness: r.Harness, + PRURL: r.PRURL, + TargetSha: r.TargetSHA, + Status: r.Status, + Verdict: r.Verdict, + Body: r.Body, + CreatedAt: r.CreatedAt, + }) +} + +// UpdateReviewRunResult sets the status/verdict/body of a running review pass. +func (s *Store) UpdateReviewRunResult(ctx context.Context, id string, status domain.ReviewRunStatus, verdict domain.ReviewVerdict, body string) (bool, error) { + s.writeMu.Lock() + defer s.writeMu.Unlock() + n, err := s.qw.UpdateReviewRunResult(ctx, gen.UpdateReviewRunResultParams{ + Status: status, + Verdict: verdict, + Body: body, + ID: id, + }) + if err != nil { + return false, err + } + return n > 0, nil +} + +// GetReviewRun returns one review pass by id. +func (s *Store) GetReviewRun(ctx context.Context, id string) (domain.ReviewRun, bool, error) { + row, err := s.qr.GetReviewRun(ctx, id) + if errors.Is(err, sql.ErrNoRows) { + return domain.ReviewRun{}, false, nil + } + if err != nil { + return domain.ReviewRun{}, false, fmt.Errorf("get review run %s: %w", id, err) + } + return reviewRunFromRow(row), true, nil +} + +// GetReviewRunBySessionAndSHA returns the most recent review pass for a worker +// session at a specific commit, ok=false if none. It lets a repeat trigger for +// the same PR head short-circuit to the existing run. +func (s *Store) GetReviewRunBySessionAndSHA(ctx context.Context, id domain.SessionID, targetSHA string) (domain.ReviewRun, bool, error) { + row, err := s.qr.GetReviewRunBySessionAndSHA(ctx, gen.GetReviewRunBySessionAndSHAParams{SessionID: id, TargetSha: targetSHA}) + if errors.Is(err, sql.ErrNoRows) { + return domain.ReviewRun{}, false, nil + } + if err != nil { + return domain.ReviewRun{}, false, fmt.Errorf("get review run for session %s sha %s: %w", id, targetSHA, err) + } + return reviewRunFromRow(row), true, nil +} + +// ListReviewRunsBySession returns all review passes for a worker session, newest first. +func (s *Store) ListReviewRunsBySession(ctx context.Context, id domain.SessionID) ([]domain.ReviewRun, error) { + rows, err := s.qr.ListReviewRunsBySession(ctx, id) + if err != nil { + return nil, fmt.Errorf("list review runs for session %s: %w", id, err) + } + out := make([]domain.ReviewRun, 0, len(rows)) + for _, row := range rows { + out = append(out, reviewRunFromRow(row)) + } + return out, nil +} + +func reviewFromRow(r gen.Review) domain.Review { + return domain.Review{ + ID: r.ID, + SessionID: r.SessionID, + ProjectID: r.ProjectID, + Harness: r.Harness, + PRURL: r.PRURL, + ReviewerHandleID: r.ReviewerHandleID, + CreatedAt: r.CreatedAt, + UpdatedAt: r.UpdatedAt, + } +} + +func reviewRunFromRow(r gen.ReviewRun) domain.ReviewRun { + return domain.ReviewRun{ + ID: r.ID, + ReviewID: r.ReviewID, + SessionID: r.SessionID, + Harness: r.Harness, + PRURL: r.PRURL, + TargetSHA: r.TargetSha, + Status: r.Status, + Verdict: r.Verdict, + Body: r.Body, + CreatedAt: r.CreatedAt, + } +} diff --git a/backend/internal/storage/sqlite/store/review_store_test.go b/backend/internal/storage/sqlite/store/review_store_test.go new file mode 100644 index 00000000..e6643ac4 --- /dev/null +++ b/backend/internal/storage/sqlite/store/review_store_test.go @@ -0,0 +1,111 @@ +package store_test + +import ( + "context" + "testing" + "time" + + "github.com/aoagents/agent-orchestrator/backend/internal/domain" +) + +func TestReviewUpsertReusesRowAndRunRoundTrip(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + seedProject(t, s, "mer") + rec, err := s.CreateSession(ctx, sampleRecord("mer")) + if err != nil { + t.Fatalf("create session: %v", err) + } + now := time.Now().UTC().Truncate(time.Second) + + // First upsert creates the review row. + if err := s.UpsertReview(ctx, domain.Review{ + ID: "rev-1", SessionID: rec.ID, ProjectID: rec.ProjectID, + Harness: domain.ReviewerClaudeCode, PRURL: "https://example/pr/1", + ReviewerHandleID: "review-mer-1", + CreatedAt: now, UpdatedAt: now, + }); err != nil { + t.Fatalf("upsert review: %v", err) + } + // Second upsert with the same session reuses the row (session_id UNIQUE), + // refreshing harness/pr_url/reviewer_handle_id but keeping the original id. + if err := s.UpsertReview(ctx, domain.Review{ + ID: "rev-2", SessionID: rec.ID, ProjectID: rec.ProjectID, + Harness: domain.ReviewerHarness("greptile"), PRURL: "https://example/pr/2", + ReviewerHandleID: "review-mer-1b", + CreatedAt: now, UpdatedAt: now.Add(time.Second), + }); err != nil { + t.Fatalf("upsert review (reuse): %v", err) + } + got, ok, err := s.GetReviewBySession(ctx, rec.ID) + if err != nil || !ok { + t.Fatalf("get review: ok=%v err=%v", ok, err) + } + if got.ID != "rev-1" { + t.Fatalf("upsert created a new row, want reuse: id=%q", got.ID) + } + if got.Harness != domain.ReviewerHarness("greptile") || got.PRURL != "https://example/pr/2" || got.ReviewerHandleID != "review-mer-1b" { + t.Fatalf("upsert did not refresh fields: %+v", got) + } + + // A run inserts running and updates to complete/changes_requested. + if err := s.InsertReviewRun(ctx, domain.ReviewRun{ + ID: "run-1", ReviewID: got.ID, SessionID: rec.ID, Harness: domain.ReviewerHarness("greptile"), + PRURL: got.PRURL, TargetSHA: "sha1", Status: domain.ReviewRunRunning, Verdict: domain.VerdictNone, + CreatedAt: now, + }); err != nil { + t.Fatalf("insert run: %v", err) + } + if ok, err := s.UpdateReviewRunResult(ctx, "run-1", domain.ReviewRunComplete, domain.VerdictChangesRequested, "please fix"); err != nil { + t.Fatalf("update run: %v", err) + } else if !ok { + t.Fatal("update run: got ok=false") + } + + gotRun, ok, err := s.GetReviewRun(ctx, "run-1") + if err != nil || !ok { + t.Fatalf("get run: ok=%v err=%v", ok, err) + } + if gotRun.ID != "run-1" || gotRun.SessionID != rec.ID || gotRun.TargetSHA != "sha1" { + t.Fatalf("get run = %+v", gotRun) + } + + bySHA, ok, err := s.GetReviewRunBySessionAndSHA(ctx, rec.ID, "sha1") + if err != nil || !ok { + t.Fatalf("by sha: ok=%v err=%v", ok, err) + } + if bySHA.Status != domain.ReviewRunComplete || bySHA.Verdict != domain.VerdictChangesRequested || bySHA.Body != "please fix" { + t.Fatalf("run result not persisted: %+v", bySHA) + } + if _, ok, _ := s.GetReviewRunBySessionAndSHA(ctx, rec.ID, "other"); ok { + t.Fatal("unexpected run for a different sha") + } + + runs, err := s.ListReviewRunsBySession(ctx, rec.ID) + if err != nil { + t.Fatalf("list runs: %v", err) + } + if len(runs) != 1 || runs[0].ID != "run-1" { + t.Fatalf("list runs = %+v", runs) + } + + if ok, err := s.UpdateReviewRunResult(ctx, "run-1", domain.ReviewRunComplete, domain.VerdictApproved, "again"); err != nil { + t.Fatalf("second update: %v", err) + } else if ok { + t.Fatal("second update completed an already-complete run") + } +} + +func TestReviewGettersMissing(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + if _, ok, err := s.GetReviewBySession(ctx, "mer-1"); err != nil || ok { + t.Fatalf("missing review: ok=%v err=%v", ok, err) + } + if _, ok, err := s.GetReviewRunBySessionAndSHA(ctx, "mer-1", "sha1"); err != nil || ok { + t.Fatalf("missing run: ok=%v err=%v", ok, err) + } + if _, ok, err := s.GetReviewRun(ctx, "run-missing"); err != nil || ok { + t.Fatalf("missing run by id: ok=%v err=%v", ok, err) + } +} diff --git a/backend/sqlc.yaml b/backend/sqlc.yaml index 4baabf6b..070b6916 100644 --- a/backend/sqlc.yaml +++ b/backend/sqlc.yaml @@ -108,3 +108,31 @@ sql: go_type: import: "github.com/aoagents/agent-orchestrator/backend/internal/domain" type: "ActivityState" + - column: "review.session_id" + go_type: + import: "github.com/aoagents/agent-orchestrator/backend/internal/domain" + type: "SessionID" + - column: "review.project_id" + go_type: + import: "github.com/aoagents/agent-orchestrator/backend/internal/domain" + type: "ProjectID" + - column: "review.harness" + go_type: + import: "github.com/aoagents/agent-orchestrator/backend/internal/domain" + type: "ReviewerHarness" + - column: "review_run.session_id" + go_type: + import: "github.com/aoagents/agent-orchestrator/backend/internal/domain" + type: "SessionID" + - column: "review_run.harness" + go_type: + import: "github.com/aoagents/agent-orchestrator/backend/internal/domain" + type: "ReviewerHarness" + - column: "review_run.status" + go_type: + import: "github.com/aoagents/agent-orchestrator/backend/internal/domain" + type: "ReviewRunStatus" + - column: "review_run.verdict" + go_type: + import: "github.com/aoagents/agent-orchestrator/backend/internal/domain" + type: "ReviewVerdict" diff --git a/frontend/src/api/schema.ts b/frontend/src/api/schema.ts index 45a3f99f..92a84d15 100644 --- a/frontend/src/api/schema.ts +++ b/frontend/src/api/schema.ts @@ -177,41 +177,43 @@ export interface paths { patch?: never; trace?: never; }; - "/api/v1/reviews": { + "/api/v1/sessions": { parameters: { query?: never; header?: never; path?: never; cookie?: never; }; - /** List code-review runs */ - get: operations["listReviews"]; + /** List sessions */ + get: operations["listSessions"]; put?: never; - post?: never; + /** Spawn a new agent session */ + post: operations["spawnSession"]; delete?: never; options?: never; head?: never; patch?: never; trace?: never; }; - "/api/v1/reviews/{id}/send": { + "/api/v1/sessions/{sessionId}": { parameters: { query?: never; header?: never; path?: never; cookie?: never; }; - get?: never; + /** Fetch one session */ + get: operations["getSession"]; put?: never; - /** Send a review run's findings to its PR */ - post: operations["sendReview"]; + post?: never; delete?: never; options?: never; head?: never; - patch?: never; + /** Rename a session display name */ + patch: operations["renameSession"]; trace?: never; }; - "/api/v1/reviews/execute": { + "/api/v1/sessions/{sessionId}/activity": { parameters: { query?: never; header?: never; @@ -220,51 +222,49 @@ export interface paths { }; get?: never; put?: never; - /** Start a code-review run for a session's PR */ - post: operations["executeReview"]; + /** Report an agent activity-state signal for a session */ + post: operations["setSessionActivity"]; delete?: never; options?: never; head?: never; patch?: never; trace?: never; }; - "/api/v1/sessions": { + "/api/v1/sessions/{sessionId}/kill": { parameters: { query?: never; header?: never; path?: never; cookie?: never; }; - /** List sessions */ - get: operations["listSessions"]; + get?: never; put?: never; - /** Spawn a new agent session */ - post: operations["spawnSession"]; + /** Mark a session terminated and tear down runtime/workspace resources */ + post: operations["killSession"]; delete?: never; options?: never; head?: never; patch?: never; trace?: never; }; - "/api/v1/sessions/{sessionId}": { + "/api/v1/sessions/{sessionId}/pr": { parameters: { query?: never; header?: never; path?: never; cookie?: never; }; - /** Fetch one session */ - get: operations["getSession"]; + /** List pull requests owned by a session */ + get: operations["listSessionPRs"]; put?: never; post?: never; delete?: never; options?: never; head?: never; - /** Rename a session display name */ - patch: operations["renameSession"]; + patch?: never; trace?: never; }; - "/api/v1/sessions/{sessionId}/activity": { + "/api/v1/sessions/{sessionId}/pr/claim": { parameters: { query?: never; header?: never; @@ -273,15 +273,15 @@ export interface paths { }; get?: never; put?: never; - /** Report an agent activity-state signal for a session */ - post: operations["setSessionActivity"]; + /** Claim an existing pull request for a session */ + post: operations["claimSessionPR"]; delete?: never; options?: never; head?: never; patch?: never; trace?: never; }; - "/api/v1/sessions/{sessionId}/kill": { + "/api/v1/sessions/{sessionId}/restore": { parameters: { query?: never; header?: never; @@ -290,23 +290,23 @@ export interface paths { }; get?: never; put?: never; - /** Mark a session terminated and tear down runtime/workspace resources */ - post: operations["killSession"]; + /** Restore a terminated session */ + post: operations["restoreSession"]; delete?: never; options?: never; head?: never; patch?: never; trace?: never; }; - "/api/v1/sessions/{sessionId}/pr": { + "/api/v1/sessions/{sessionId}/reviews": { parameters: { query?: never; header?: never; path?: never; cookie?: never; }; - /** List pull requests owned by a session */ - get: operations["listSessionPRs"]; + /** List a worker's code-review runs */ + get: operations["listReviews"]; put?: never; post?: never; delete?: never; @@ -315,7 +315,7 @@ export interface paths { patch?: never; trace?: never; }; - "/api/v1/sessions/{sessionId}/pr/claim": { + "/api/v1/sessions/{sessionId}/reviews/submit": { parameters: { query?: never; header?: never; @@ -324,15 +324,15 @@ export interface paths { }; get?: never; put?: never; - /** Claim an existing pull request for a session */ - post: operations["claimSessionPR"]; + /** Record a reviewer's result for a worker's PR */ + post: operations["submitReview"]; delete?: never; options?: never; head?: never; patch?: never; trace?: never; }; - "/api/v1/sessions/{sessionId}/restore": { + "/api/v1/sessions/{sessionId}/reviews/trigger": { parameters: { query?: never; header?: never; @@ -341,8 +341,8 @@ export interface paths { }; get?: never; put?: never; - /** Restore a terminated session */ - post: operations["restoreSession"]; + /** Trigger a code review of a worker's PR */ + post: operations["triggerReview"]; delete?: never; options?: never; head?: never; @@ -456,9 +456,8 @@ export interface components { lastActivityAt: string; state: string; }; - ExecuteReviewInput: { - /** @description Session whose PR to review. */ - sessionId: string; + DomainReviewerConfig: { + harness: string; }; KillSessionResponse: { freed?: boolean; @@ -472,6 +471,7 @@ export interface components { projects: components["schemas"]["ProjectSummary"][]; }; ListReviewsResponse: { + reviewerHandleId: string; reviews: components["schemas"]["ReviewRun"][]; }; ListSessionPRsResponse: { @@ -531,6 +531,7 @@ export interface components { }; orchestrator?: components["schemas"]["RoleOverride"]; postCreate?: string[]; + reviewers?: components["schemas"]["DomainReviewerConfig"][]; sessionPrefix?: string; symlinks?: string[]; worker?: components["schemas"]["RoleOverride"]; @@ -573,23 +574,22 @@ export interface components { session: components["schemas"]["Session"]; sessionId: string; }; - ReviewFinding: { - body: string; - id: string; - line: number; - path: string; - severity: string; - }; - ReviewResponse: { - review: components["schemas"]["ReviewRun"]; - }; ReviewRun: { + body: string; /** Format: date-time */ createdAt: string; - findings: components["schemas"]["ReviewFinding"][]; + harness: string; id: string; + prUrl: string; + reviewId: string; sessionId: string; status: string; + targetSha: string; + verdict: string; + }; + ReviewRunResponse: { + review: components["schemas"]["ReviewRun"]; + reviewerHandleId: string; }; RoleOverride: { agent?: string; @@ -672,6 +672,14 @@ export interface components { projectId: string; prompt?: string; }; + SubmitReviewInput: { + /** @description Review body recorded by AO. Required for changes_requested. */ + body: string; + /** @description Review run id being completed. */ + runId: string; + /** @description Review verdict: approved or changes_requested. */ + verdict: string; + }; WorkspaceRepo: { name: string; relativePath: string; @@ -1311,127 +1319,6 @@ export interface operations { }; }; }; - listReviews: { - parameters: { - query?: never; - header?: never; - path?: never; - cookie?: never; - }; - requestBody?: never; - responses: { - /** @description OK */ - 200: { - headers: { - [name: string]: unknown; - }; - content: { - "application/json": components["schemas"]["ListReviewsResponse"]; - }; - }; - /** @description Not Implemented */ - 501: { - headers: { - [name: string]: unknown; - }; - content: { - "application/json": components["schemas"]["APIError"]; - }; - }; - }; - }; - sendReview: { - parameters: { - query?: never; - header?: never; - path: { - /** @description Review run id. */ - id: string; - }; - cookie?: never; - }; - requestBody?: never; - responses: { - /** @description OK */ - 200: { - headers: { - [name: string]: unknown; - }; - content: { - "application/json": components["schemas"]["ReviewResponse"]; - }; - }; - /** @description Not Found */ - 404: { - headers: { - [name: string]: unknown; - }; - content: { - "application/json": components["schemas"]["APIError"]; - }; - }; - /** @description Not Implemented */ - 501: { - headers: { - [name: string]: unknown; - }; - content: { - "application/json": components["schemas"]["APIError"]; - }; - }; - }; - }; - executeReview: { - parameters: { - query?: never; - header?: never; - path?: never; - cookie?: never; - }; - requestBody: { - content: { - "application/json": components["schemas"]["ExecuteReviewInput"]; - }; - }; - responses: { - /** @description Created */ - 201: { - headers: { - [name: string]: unknown; - }; - content: { - "application/json": components["schemas"]["ReviewResponse"]; - }; - }; - /** @description Bad Request */ - 400: { - headers: { - [name: string]: unknown; - }; - content: { - "application/json": components["schemas"]["APIError"]; - }; - }; - /** @description Unprocessable Entity */ - 422: { - headers: { - [name: string]: unknown; - }; - content: { - "application/json": components["schemas"]["APIError"]; - }; - }; - /** @description Not Implemented */ - 501: { - headers: { - [name: string]: unknown; - }; - content: { - "application/json": components["schemas"]["APIError"]; - }; - }; - }; - }; listSessions: { parameters: { query?: { @@ -1928,6 +1815,169 @@ export interface operations { }; }; }; + listReviews: { + parameters: { + query?: never; + header?: never; + path: { + /** @description Session identifier, e.g. project-1. */ + sessionId: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description OK */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ListReviewsResponse"]; + }; + }; + /** @description Unprocessable Entity */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + /** @description Not Implemented */ + 501: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + }; + }; + submitReview: { + parameters: { + query?: never; + header?: never; + path: { + /** @description Session identifier, e.g. project-1. */ + sessionId: string; + }; + cookie?: never; + }; + requestBody: { + content: { + "application/json": components["schemas"]["SubmitReviewInput"]; + }; + }; + responses: { + /** @description OK */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ReviewRunResponse"]; + }; + }; + /** @description Bad Request */ + 400: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + /** @description Not Found */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + /** @description Unprocessable Entity */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + /** @description Not Implemented */ + 501: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + }; + }; + triggerReview: { + parameters: { + query?: never; + header?: never; + path: { + /** @description Session identifier, e.g. project-1. */ + sessionId: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description OK */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ReviewRunResponse"]; + }; + }; + /** @description Created */ + 201: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["ReviewRunResponse"]; + }; + }; + /** @description Not Found */ + 404: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + /** @description Unprocessable Entity */ + 422: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + /** @description Not Implemented */ + 501: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["APIError"]; + }; + }; + }; + }; rollbackSession: { parameters: { query?: never;