From f3e45959e678d359623d7fd4d1404def2f34b673 Mon Sep 17 00:00:00 2001 From: Dhruv Sharma Date: Tue, 19 May 2026 11:47:57 +0530 Subject: [PATCH 1/2] Add orchestrator-driven code review board (#1871) * feat: add orchestrator-driven code review board * feat: wire review findings back to workers * feat(web): send review feedback to workers * fix(core): mark stale review runs outdated * fix: restore reviewer flow after main merge * Fix review lock lint failure * Guard concurrent review executions --------- Co-authored-by: Madhav Kumar --- .../cli/__tests__/commands/review.test.ts | 372 ++++++ .../cli/__tests__/commands/status.test.ts | 85 +- packages/cli/__tests__/program.test.ts | 4 + packages/cli/src/commands/review.ts | 294 +++++ packages/cli/src/commands/status.ts | 103 +- packages/cli/src/program.ts | 2 + .../src/__tests__/code-review-manager.test.ts | 738 +++++++++++ .../src/__tests__/code-review-store.test.ts | 101 ++ .../core/src/__tests__/global-config.test.ts | 51 + .../__tests__/migration-storage-v2.test.ts | 2 + .../src/__tests__/plugin-integration.test.ts | 2 +- packages/core/src/code-review-manager.ts | 1050 +++++++++++++++ packages/core/src/code-review-store.ts | 504 ++++++++ packages/core/src/global-config.ts | 82 +- packages/core/src/index.ts | 48 + packages/core/src/paths.ts | 5 + packages/core/src/prompts/orchestrator.md | 29 + packages/core/vitest.config.ts | 11 + packages/web/e2e/review-board-flows.md | 110 ++ packages/web/e2e/review-board.e2e.ts | 871 +++++++++++++ packages/web/package.json | 1 + packages/web/src/__tests__/api-routes.test.ts | 19 +- .../__tests__/project-detail-route.test.ts | 31 + packages/web/src/__tests__/review-api.test.ts | 328 +++++ .../web/src/app/api/reviews/execute/route.ts | 63 + .../web/src/app/api/reviews/findings/route.ts | 53 + packages/web/src/app/api/reviews/route.ts | 87 ++ .../web/src/app/api/reviews/send/route.ts | 56 + packages/web/src/app/api/sessions/route.ts | 6 +- packages/web/src/app/globals.css | 548 ++++++++ packages/web/src/app/review/page.tsx | 37 + packages/web/src/app/reviews/page.tsx | 11 + .../web/src/app/sessions/[id]/page.test.tsx | 7 +- packages/web/src/components/AttentionZone.tsx | 23 +- packages/web/src/components/Dashboard.tsx | 43 +- .../web/src/components/ProjectSidebar.tsx | 27 +- .../web/src/components/ReviewDashboard.tsx | 1150 +++++++++++++++++ packages/web/src/components/SessionCard.tsx | 158 ++- .../Dashboard.projectOverview.test.tsx | 24 + .../__tests__/ReviewDashboard.test.tsx | 188 +++ .../__tests__/SessionCard.coverage.test.tsx | 13 +- .../__tests__/SessionDetail.desktop.test.tsx | 54 + .../src/lib/__tests__/review-types.test.ts | 21 + packages/web/src/lib/__tests__/types.test.ts | 45 + packages/web/src/lib/review-page-data.ts | 178 +++ packages/web/src/lib/review-types.ts | 77 ++ packages/web/src/lib/routes.ts | 8 + packages/web/src/lib/types.ts | 36 +- 48 files changed, 7637 insertions(+), 119 deletions(-) create mode 100644 packages/cli/__tests__/commands/review.test.ts create mode 100644 packages/cli/src/commands/review.ts create mode 100644 packages/core/src/__tests__/code-review-manager.test.ts create mode 100644 packages/core/src/__tests__/code-review-store.test.ts create mode 100644 packages/core/src/code-review-manager.ts create mode 100644 packages/core/src/code-review-store.ts create mode 100644 packages/web/e2e/review-board-flows.md create mode 100644 packages/web/e2e/review-board.e2e.ts create mode 100644 packages/web/src/__tests__/review-api.test.ts create mode 100644 packages/web/src/app/api/reviews/execute/route.ts create mode 100644 packages/web/src/app/api/reviews/findings/route.ts create mode 100644 packages/web/src/app/api/reviews/route.ts create mode 100644 packages/web/src/app/api/reviews/send/route.ts create mode 100644 packages/web/src/app/review/page.tsx create mode 100644 packages/web/src/app/reviews/page.tsx create mode 100644 packages/web/src/components/ReviewDashboard.tsx create mode 100644 packages/web/src/components/__tests__/ReviewDashboard.test.tsx create mode 100644 packages/web/src/lib/__tests__/review-types.test.ts create mode 100644 packages/web/src/lib/review-page-data.ts create mode 100644 packages/web/src/lib/review-types.ts diff --git a/packages/cli/__tests__/commands/review.test.ts b/packages/cli/__tests__/commands/review.test.ts new file mode 100644 index 000000000..19d0b7950 --- /dev/null +++ b/packages/cli/__tests__/commands/review.test.ts @@ -0,0 +1,372 @@ +import { execFileSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + createActivitySignal, + createInitialCanonicalLifecycle, + createCodeReviewStore, + type OrchestratorConfig, + type Session, + type SessionManager, +} from "@aoagents/ao-core"; +import type * as AoCore from "@aoagents/ao-core"; + +const { mockConfigRef, mockSessionManager, reviewStoreRootRef } = vi.hoisted(() => ({ + mockConfigRef: { current: null as OrchestratorConfig | null }, + mockSessionManager: { + get: vi.fn(), + list: vi.fn(), + spawn: vi.fn(), + spawnOrchestrator: vi.fn(), + ensureOrchestrator: vi.fn(), + restore: vi.fn(), + kill: vi.fn(), + cleanup: vi.fn(), + send: vi.fn(), + claimPR: vi.fn(), + }, + reviewStoreRootRef: { current: "" }, +})); + +vi.mock("@aoagents/ao-core", async (importOriginal) => { + const actual = await importOriginal(); + const createIsolatedStore = (projectId: string, options: AoCore.CodeReviewStoreOptions = {}) => + actual.createCodeReviewStore(projectId, { + ...options, + storeDir: options.storeDir ?? `${reviewStoreRootRef.current}/${projectId}`, + }); + + return { + ...actual, + createCodeReviewStore: createIsolatedStore, + triggerCodeReviewForSession: ( + options: AoCore.TriggerCodeReviewOptions, + input: AoCore.TriggerCodeReviewInput, + ) => + actual.triggerCodeReviewForSession( + { + ...options, + storeFactory: options.storeFactory ?? createIsolatedStore, + }, + input, + ), + executeCodeReviewRun: ( + options: AoCore.ExecuteCodeReviewRunOptions, + input: AoCore.ExecuteCodeReviewRunInput, + ) => + actual.executeCodeReviewRun( + { + ...options, + storeFactory: options.storeFactory ?? createIsolatedStore, + }, + input, + ), + sendCodeReviewFindingsToAgent: ( + options: AoCore.SendCodeReviewFindingsOptions, + input: AoCore.SendCodeReviewFindingsInput, + ) => + actual.sendCodeReviewFindingsToAgent( + { + ...options, + storeFactory: options.storeFactory ?? createIsolatedStore, + }, + input, + ), + loadConfig: () => mockConfigRef.current, + }; +}); + +vi.mock("../../src/lib/create-session-manager.js", () => ({ + getSessionManager: async (): Promise => mockSessionManager as SessionManager, +})); + +function makeSession(overrides: Partial = {}): Session { + const lifecycle = createInitialCanonicalLifecycle("worker", new Date("2026-05-10T10:00:00.000Z")); + lifecycle.session.state = "idle"; + lifecycle.session.reason = "awaiting_external_review"; + lifecycle.pr.state = "open"; + lifecycle.pr.reason = "review_pending"; + lifecycle.pr.number = 7; + lifecycle.pr.url = "https://github.com/acme/app/pull/7"; + lifecycle.runtime.state = "alive"; + lifecycle.runtime.reason = "process_running"; + + return { + id: "app-1", + projectId: "app", + status: "review_pending", + activity: "idle", + activitySignal: createActivitySignal("valid", { + activity: "idle", + timestamp: new Date("2026-05-10T10:00:00.000Z"), + source: "native", + }), + lifecycle, + branch: "feat/todos", + issueId: null, + pr: { + number: 7, + url: "https://github.com/acme/app/pull/7", + title: "feat: todos", + owner: "acme", + repo: "app", + branch: "feat/todos", + baseBranch: "main", + isDraft: false, + }, + workspacePath: null, + runtimeHandle: { id: "tmux-app-1", runtimeName: "tmux", data: {} }, + agentInfo: null, + createdAt: new Date("2026-05-10T09:00:00.000Z"), + lastActivityAt: new Date("2026-05-10T10:00:00.000Z"), + metadata: {}, + ...overrides, + }; +} + +function createGitRepo(path: string): void { + mkdirSync(path, { recursive: true }); + execFileSync("git", ["init", "-b", "main"], { cwd: path }); + writeFileSync(join(path, "README.md"), "# App\n"); + execFileSync("git", ["add", "README.md"], { cwd: path }); + execFileSync("git", ["commit", "-m", "initial"], { + cwd: path, + env: { + ...process.env, + GIT_AUTHOR_NAME: "AO Test", + GIT_AUTHOR_EMAIL: "ao@example.com", + GIT_COMMITTER_NAME: "AO Test", + GIT_COMMITTER_EMAIL: "ao@example.com", + }, + }); +} + +let tmpDir: string; +let originalHome: string | undefined; +let consoleLogSpy: ReturnType; + +import { Command } from "commander"; +import { registerReview } from "../../src/commands/review.js"; + +let program: Command; + +beforeEach(() => { + tmpDir = mkdtempSync(join(tmpdir(), "ao-cli-review-test-")); + reviewStoreRootRef.current = join(tmpDir, "review-store"); + originalHome = process.env["HOME"]; + process.env["HOME"] = tmpDir; + + mockConfigRef.current = { + configPath: join(tmpDir, "agent-orchestrator.yaml"), + readyThresholdMs: 300_000, + defaults: { runtime: "tmux", agent: "codex", workspace: "worktree", notifiers: [] }, + projects: { + app: { + name: "App", + path: join(tmpDir, "app"), + defaultBranch: "main", + sessionPrefix: "app", + }, + docs: { + name: "Docs", + path: join(tmpDir, "docs"), + defaultBranch: "main", + sessionPrefix: "docs", + }, + }, + notifiers: {}, + notificationRouting: { urgent: [], action: [], warning: [], info: [] }, + reactions: {}, + }; + + const appPath = join(tmpDir, "app"); + createGitRepo(appPath); + createGitRepo(join(tmpDir, "docs")); + + mockSessionManager.get.mockReset(); + mockSessionManager.get.mockResolvedValue(makeSession({ workspacePath: appPath })); + mockSessionManager.list.mockReset(); + mockSessionManager.send.mockReset(); + + createCodeReviewStore("app").deleteAll(); + createCodeReviewStore("docs").deleteAll(); + + program = new Command(); + program.exitOverride(); + registerReview(program); + consoleLogSpy = vi.spyOn(console, "log").mockImplementation(() => {}); + vi.spyOn(console, "error").mockImplementation(() => {}); + vi.spyOn(process, "exit").mockImplementation((code) => { + throw new Error(`process.exit(${code})`); + }); +}); + +afterEach(() => { + if (originalHome === undefined) { + delete process.env["HOME"]; + } else { + process.env["HOME"] = originalHome; + } + rmSync(tmpDir, { recursive: true, force: true }); + vi.restoreAllMocks(); +}); + +describe("review command", () => { + it("requests and lists review runs through the CLI", async () => { + await program.parseAsync(["node", "test", "review", "run", "app-1", "--json"]); + + const runPayload = JSON.parse(String(consoleLogSpy.mock.calls.at(-1)?.[0])) as { + run: { linkedSessionId: string; reviewerSessionId: string; status: string }; + }; + expect(runPayload.run).toMatchObject({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "queued", + }); + + consoleLogSpy.mockClear(); + await program.parseAsync(["node", "test", "review", "list", "app", "--json"]); + + const listPayload = JSON.parse(String(consoleLogSpy.mock.calls.at(-1)?.[0])) as { + runs: Array<{ linkedSessionId: string; reviewerSessionId: string }>; + }; + expect(listPayload.runs).toHaveLength(1); + expect(listPayload.runs[0]).toMatchObject({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + }); + }); + + it("rejects unknown review run statuses", async () => { + await expect( + program.parseAsync(["node", "test", "review", "run", "app-1", "--status", "bogus"]), + ).rejects.toThrow("process.exit(1)"); + }); + + it("can request and execute a review run directly", async () => { + await program.parseAsync([ + "node", + "test", + "review", + "run", + "app-1", + "--execute", + "--command", + `printf '%s\\n' '{"findings":[{"severity":"warning","title":"CLI finding","body":"Detected in CLI."}]}'`, + "--json", + ]); + + const payload = JSON.parse(String(consoleLogSpy.mock.calls.at(-1)?.[0])) as { + run: { status: string; findingCount: number; openFindingCount: number }; + }; + expect(payload.run).toMatchObject({ + status: "needs_triage", + findingCount: 1, + openFindingCount: 1, + }); + }); + + it("executes the oldest queued review run across all projects when no run is specified", async () => { + const appStore = createCodeReviewStore("app"); + const docsStore = createCodeReviewStore("docs"); + const appPath = join(tmpDir, "app"); + const docsPath = join(tmpDir, "docs"); + mockSessionManager.get.mockImplementation(async (sessionId: string) => { + if (sessionId === "docs-1") { + return makeSession({ id: "docs-1", projectId: "docs", workspacePath: docsPath }); + } + if (sessionId === "app-1") { + return makeSession({ id: "app-1", projectId: "app", workspacePath: appPath }); + } + return null; + }); + const older = docsStore.createRun( + { + linkedSessionId: "docs-1", + reviewerSessionId: "docs-rev-1", + status: "queued", + }, + new Date("2026-05-10T10:00:00.000Z"), + ); + const newer = appStore.createRun( + { + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "queued", + }, + new Date("2026-05-10T11:00:00.000Z"), + ); + + await program.parseAsync([ + "node", + "test", + "review", + "execute", + "--command", + `printf '%s\\n' '{"findings":[]}'`, + "--json", + ]); + + const payload = JSON.parse(String(consoleLogSpy.mock.calls.at(-1)?.[0])) as { + run: { id: string; reviewerSessionId: string; status: string }; + }; + expect(payload.run).toMatchObject({ + id: older.id, + reviewerSessionId: "docs-rev-1", + status: "clean", + }); + expect(createCodeReviewStore("app").getRun(newer.id)?.status).toBe("queued"); + }); + + it("sends open review findings to the linked coding worker", async () => { + const store = createCodeReviewStore("app"); + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "needs_triage", + prNumber: 7, + }); + store.createFinding({ + runId: run.id, + linkedSessionId: "app-1", + severity: "warning", + title: "CLI finding", + body: "Detected in CLI.", + filePath: "src/app.ts", + startLine: 12, + }); + + await program.parseAsync([ + "node", + "test", + "review", + "send", + "app-rev-1", + "--project", + "app", + "--json", + ]); + + const payload = JSON.parse(String(consoleLogSpy.mock.calls.at(-1)?.[0])) as { + run: { status: string; openFindingCount: number; sentFindingCount: number }; + sentFindingCount: number; + message: string; + }; + expect(payload).toMatchObject({ + sentFindingCount: 1, + run: { + status: "waiting_update", + openFindingCount: 0, + sentFindingCount: 1, + }, + }); + expect(payload.message).toContain("AO reviewer app-rev-1 found 1 open issue"); + expect(payload.message).toContain("Location: src/app.ts:12"); + expect(mockSessionManager.send).toHaveBeenCalledWith( + "app-1", + expect.stringContaining("[warning] CLI finding"), + ); + }); +}); diff --git a/packages/cli/__tests__/commands/status.test.ts b/packages/cli/__tests__/commands/status.test.ts index e9af2024a..7e7ab90b4 100644 --- a/packages/cli/__tests__/commands/status.test.ts +++ b/packages/cli/__tests__/commands/status.test.ts @@ -16,8 +16,10 @@ import { type ActivityState, createInitialCanonicalLifecycle, createActivitySignal, + createCodeReviewStore, sessionFromMetadata, } from "@aoagents/ao-core"; +import type * as AoCore from "@aoagents/ao-core"; const { mockTmux, @@ -32,6 +34,7 @@ const { mockSessionManager, mockGetPluginRegistry, sessionsDirRef, + reviewStoreRootRef, } = vi.hoisted(() => ({ mockTmux: vi.fn(), mockGit: vi.fn(), @@ -54,6 +57,7 @@ const { }, mockGetPluginRegistry: vi.fn(), sessionsDirRef: { current: "" }, + reviewStoreRootRef: { current: "" }, })); vi.mock("../../src/lib/shell.js", () => ({ @@ -76,10 +80,17 @@ vi.mock("../../src/lib/shell.js", () => ({ })); vi.mock("@aoagents/ao-core", async (importOriginal) => { - // eslint-disable-next-line @typescript-eslint/consistent-type-imports - const actual = await importOriginal(); + const actual = await importOriginal(); return { ...actual, + createCodeReviewStore: ( + projectId: string, + options: AoCore.CodeReviewStoreOptions = {}, + ) => + actual.createCodeReviewStore(projectId, { + ...options, + storeDir: options.storeDir ?? `${reviewStoreRootRef.current}/${projectId}`, + }), loadConfig: () => mockConfigRef.current, }; }); @@ -211,6 +222,7 @@ vi.mock("../../src/lib/create-session-manager.js", () => ({ let tmpDir: string; let sessionsDir: string; +let originalHome: string | undefined; import { Command } from "commander"; import { registerStatus } from "../../src/commands/status.js"; @@ -223,6 +235,8 @@ let processOnceSpy: ReturnType | undefined; beforeEach(() => { tmpDir = mkdtempSync(join(tmpdir(), "ao-status-test-")); + originalHome = process.env["HOME"]; + process.env["HOME"] = tmpDir; const configPath = join(tmpDir, "agent-orchestrator.yaml"); writeFileSync(configPath, "projects: {}"); @@ -256,6 +270,7 @@ beforeEach(() => { sessionsDir = join(tmpDir, "sessions"); mkdirSync(sessionsDir, { recursive: true }); sessionsDirRef.current = sessionsDir; + reviewStoreRootRef.current = join(tmpDir, "code-reviews"); program = new Command(); program.exitOverride(); @@ -296,6 +311,11 @@ beforeEach(() => { }); afterEach(() => { + if (originalHome === undefined) { + delete process.env["HOME"]; + } else { + process.env["HOME"] = originalHome; + } setIntervalSpy?.mockRestore(); setIntervalSpy = undefined; clearIntervalSpy?.mockRestore(); @@ -326,6 +346,67 @@ describe("status command", () => { expect(output).toContain("no active sessions"); }); + it("shows AO-local reviewer runs separately from coding sessions", async () => { + const store = createCodeReviewStore("my-app"); + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "needs_triage", + summary: "Review run", + }); + store.createFinding({ + runId: run.id, + linkedSessionId: "app-1", + severity: "warning", + title: "Review finding", + body: "Finding body", + }); + + await program.parseAsync(["node", "test", "status"]); + + const output = consoleSpy.mock.calls.map((c) => c[0]).join("\n"); + expect(output).toContain("Reviews:"); + expect(output).toContain("app-rev-1"); + expect(output).toContain("needs_triage"); + expect(output).toContain("app-1"); + expect(output).toContain("1 open finding"); + }); + + it("includes reviewer runs in JSON status output", async () => { + const store = createCodeReviewStore("my-app"); + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "needs_triage", + }); + store.createFinding({ + runId: run.id, + linkedSessionId: "app-1", + severity: "warning", + title: "Review finding", + body: "Finding body", + }); + + await program.parseAsync(["node", "test", "status", "--json"]); + + const jsonCalls = consoleSpy.mock.calls.map((c) => c[0]).join(""); + const parsed = JSON.parse(jsonCalls) as { + reviews: Array<{ reviewerSessionId: string; linkedSessionId: string; openFindingCount: number }>; + meta: { reviewRunCount: number; activeReviewRunCount: number; openReviewFindingCount: number }; + }; + expect(parsed.reviews).toHaveLength(1); + expect(parsed.reviews[0]).toMatchObject({ + reviewerSessionId: "app-rev-1", + linkedSessionId: "app-1", + openFindingCount: 1, + }); + expect(parsed.meta).toMatchObject({ + reviewRunCount: 1, + activeReviewRunCount: 1, + openReviewFindingCount: 1, + }); + }); + it("displays sessions from tmux with metadata", async () => { // Create metadata files writeFileSync( diff --git a/packages/cli/__tests__/program.test.ts b/packages/cli/__tests__/program.test.ts index e854a2b03..5af2ac638 100644 --- a/packages/cli/__tests__/program.test.ts +++ b/packages/cli/__tests__/program.test.ts @@ -10,4 +10,8 @@ describe("createProgram", () => { it("registers the project command", () => { expect(createProgram().commands.some((command) => command.name() === "project")).toBe(true); }); + + it("registers the review command", () => { + expect(createProgram().commands.some((command) => command.name() === "review")).toBe(true); + }); }); diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts new file mode 100644 index 000000000..488b24032 --- /dev/null +++ b/packages/cli/src/commands/review.ts @@ -0,0 +1,294 @@ +import chalk from "chalk"; +import type { Command } from "commander"; +import { + createShellCodeReviewRunner, + createCodeReviewStore, + executeCodeReviewRun, + loadConfig, + sendCodeReviewFindingsToAgent, + SessionNotFoundError, + triggerCodeReviewForSession, + type CodeReviewRunStatus, + type CodeReviewRunSummary, +} from "@aoagents/ao-core"; +import { getSessionManager } from "../lib/create-session-manager.js"; + +const RUN_STATUSES: ReadonlySet = new Set([ + "queued", + "preparing", + "running", + "needs_triage", + "sent_to_agent", + "waiting_update", + "clean", + "outdated", + "failed", + "cancelled", +]); + +function parseRunStatus(value: string | undefined): CodeReviewRunStatus | undefined { + if (!value) return undefined; + if (RUN_STATUSES.has(value as CodeReviewRunStatus)) { + return value as CodeReviewRunStatus; + } + throw new Error(`Unknown review status: ${value}`); +} + +function printRun(run: CodeReviewRunSummary): void { + const findings = + run.openFindingCount === 1 ? "1 open finding" : `${run.openFindingCount} open findings`; + const parts = [ + chalk.green(run.reviewerSessionId), + chalk.dim(run.id), + run.status, + chalk.cyan(run.linkedSessionId), + findings, + ]; + + if (run.prNumber) { + parts.push(chalk.blue(`PR #${run.prNumber}`)); + } + + console.log(parts.join(" ")); +} + +function printSendResult(run: CodeReviewRunSummary, sentFindingCount: number): void { + const findings = sentFindingCount === 1 ? "1 finding" : `${sentFindingCount} findings`; + console.log(chalk.green(`Sent ${findings} to ${chalk.cyan(run.linkedSessionId)}:`)); + printRun(run); +} + +function getRunProjectId( + projectIds: string[], + runId: string, +): { projectId: string; run: CodeReviewRunSummary } | null { + for (const projectId of projectIds) { + const run = createCodeReviewStore(projectId) + .listRunSummaries() + .find((entry) => entry.id === runId || entry.reviewerSessionId === runId); + if (run) return { projectId, run }; + } + return null; +} + +function getNextQueuedRun( + projectIds: string[], +): { projectId: string; run: CodeReviewRunSummary } | null { + return ( + projectIds + .flatMap((projectId) => + createCodeReviewStore(projectId) + .listRunSummaries({ status: "queued" }) + .map((run) => ({ projectId, run })), + ) + .sort( + (a, b) => + a.run.createdAt.localeCompare(b.run.createdAt) || + a.projectId.localeCompare(b.projectId) || + a.run.id.localeCompare(b.run.id), + )[0] ?? null + ); +} + +export function registerReview(program: Command): void { + const review = program.command("review").description("Manage AO-local reviewer runs"); + + review + .command("run") + .description("Request a reviewer run for a worker session") + .argument("", "Worker session ID") + .option("--summary ", "Summary to store on the review run") + .option("--status ", "Initial run status (defaults to queued)") + .option("--execute", "Execute the review run immediately") + .option("--command ", "Shell command to execute as the reviewer") + .option("--json", "Output as JSON") + .action( + async ( + sessionId: string, + opts: { + summary?: string; + status?: string; + execute?: boolean; + command?: string; + json?: boolean; + }, + ) => { + try { + const config = loadConfig(); + const sessionManager = await getSessionManager(config); + let run = await triggerCodeReviewForSession( + { config, sessionManager }, + { + sessionId, + requestedBy: "cli", + status: parseRunStatus(opts.status), + summary: opts.summary, + }, + ); + + if (opts.execute || opts.command) { + run = await executeCodeReviewRun( + { + config, + sessionManager, + ...(opts.command ? { runReviewer: createShellCodeReviewRunner(opts.command) } : {}), + }, + { projectId: run.projectId, runId: run.id }, + ); + } + + if (opts.json) { + console.log(JSON.stringify({ run }, null, 2)); + return; + } + + console.log( + chalk.green( + opts.execute || opts.command ? "Review run executed:" : "Review run requested:", + ), + ); + printRun(run); + } catch (error) { + if (error instanceof SessionNotFoundError) { + console.error(chalk.red(error.message)); + process.exit(1); + } + console.error(chalk.red(error instanceof Error ? error.message : String(error))); + process.exit(1); + } + }, + ); + + review + .command("execute") + .description("Execute a queued AO-local reviewer run") + .argument("[project]", "Project ID (searches all projects if omitted)") + .option("--run ", "Review run ID or reviewer session ID") + .option("--command ", "Shell command to execute as the reviewer") + .option("--force", "Execute even if the run is not queued") + .option("--json", "Output as JSON") + .action( + async ( + projectId: string | undefined, + opts: { run?: string; command?: string; force?: boolean; json?: boolean }, + ) => { + try { + const config = loadConfig(); + if (projectId && !config.projects[projectId]) { + throw new Error(`Unknown project: ${projectId}`); + } + + const projectIds = projectId ? [projectId] : Object.keys(config.projects); + const target = opts.run + ? getRunProjectId(projectIds, opts.run) + : getNextQueuedRun(projectIds); + if (!target) { + throw new Error( + opts.run ? `Review run not found: ${opts.run}` : "No queued review runs found.", + ); + } + + const sessionManager = await getSessionManager(config); + const run = await executeCodeReviewRun( + { + config, + sessionManager, + force: opts.force, + ...(opts.command ? { runReviewer: createShellCodeReviewRunner(opts.command) } : {}), + }, + { projectId: target.projectId, runId: target.run.id }, + ); + + if (opts.json) { + console.log(JSON.stringify({ run }, null, 2)); + return; + } + + console.log(chalk.green("Review run executed:")); + printRun(run); + } catch (error) { + if (error instanceof SessionNotFoundError) { + console.error(chalk.red(error.message)); + process.exit(1); + } + console.error(chalk.red(error instanceof Error ? error.message : String(error))); + process.exit(1); + } + }, + ); + + review + .command("send") + .description("Send open AO-local review findings to the linked coding worker") + .argument("", "Review run ID or reviewer session ID") + .option("-p, --project ", "Project ID (searches all projects if omitted)") + .option("--json", "Output as JSON") + .action(async (runRef: string, opts: { project?: string; json?: boolean }) => { + try { + const config = loadConfig(); + if (opts.project && !config.projects[opts.project]) { + throw new Error(`Unknown project: ${opts.project}`); + } + + const projectIds = opts.project ? [opts.project] : Object.keys(config.projects); + const target = getRunProjectId(projectIds, runRef); + if (!target) { + throw new Error(`Review run not found: ${runRef}`); + } + + const sessionManager = await getSessionManager(config); + const result = await sendCodeReviewFindingsToAgent( + { config, sessionManager }, + { projectId: target.projectId, runId: target.run.id }, + ); + + if (opts.json) { + console.log(JSON.stringify(result, null, 2)); + return; + } + + printSendResult(result.run, result.sentFindingCount); + } catch (error) { + if (error instanceof SessionNotFoundError) { + console.error(chalk.red(error.message)); + process.exit(1); + } + console.error(chalk.red(error instanceof Error ? error.message : String(error))); + process.exit(1); + } + }); + + review + .command("list") + .description("List AO-local reviewer runs") + .argument("[project]", "Project ID (lists all projects if omitted)") + .option("--json", "Output as JSON") + .action(async (projectId: string | undefined, opts: { json?: boolean }) => { + try { + const config = loadConfig(); + if (projectId && !config.projects[projectId]) { + throw new Error(`Unknown project: ${projectId}`); + } + + const projectIds = projectId ? [projectId] : Object.keys(config.projects); + const runs = projectIds.flatMap((id) => createCodeReviewStore(id).listRunSummaries()); + + if (opts.json) { + console.log(JSON.stringify({ runs }, null, 2)); + return; + } + + if (runs.length === 0) { + console.log(chalk.dim("No review runs found.")); + return; + } + + for (const run of runs) { + printRun(run); + } + } catch (error) { + console.error(chalk.red(error instanceof Error ? error.message : String(error))); + process.exit(1); + } + }); +} diff --git a/packages/cli/src/commands/status.ts b/packages/cli/src/commands/status.ts index 16e063bcc..72b3327f9 100644 --- a/packages/cli/src/commands/status.ts +++ b/packages/cli/src/commands/status.ts @@ -19,6 +19,9 @@ import { loadConfig, getProjectSessionsDir, readAgentReportAuditTrailAsync, + createCodeReviewStore, + type CodeReviewRunStatus, + type CodeReviewRunSummary, } from "@aoagents/ao-core"; import { git, getTmuxSessions, getTmuxActivity } from "../lib/shell.js"; import { @@ -61,6 +64,24 @@ interface StatusOptions { reports?: string; } +interface ProjectReviewStatus { + projectId: string; + runs: CodeReviewRunSummary[]; + runCount: number; + activeRunCount: number; + openFindingCount: number; +} + +const REVIEW_ATTENTION_STATUSES: ReadonlySet = new Set([ + "queued", + "preparing", + "running", + "needs_triage", + "sent_to_agent", + "waiting_update", + "failed", +]); + /** Parse --reports value: "full" → Infinity, positive integer → N, undefined → 0 (off). */ function parseReportsLimit(value: string | undefined): number { if (value === undefined) return 0; @@ -89,6 +110,21 @@ function maybeClearScreen(): void { } } +function gatherProjectReviewStatus(projectId: string): ProjectReviewStatus { + const runs = createCodeReviewStore(projectId).listRunSummaries(); + const activeRunCount = runs.filter( + (run) => REVIEW_ATTENTION_STATUSES.has(run.status) || run.openFindingCount > 0, + ).length; + const openFindingCount = runs.reduce((sum, run) => sum + run.openFindingCount, 0); + return { + projectId, + runs, + runCount: runs.length, + activeRunCount, + openFindingCount, + }; +} + async function gatherSessionInfo( session: Session, agent: Agent, @@ -306,6 +342,44 @@ function printOrchestratorRow(info: SessionInfo): void { printReportRows(info.reports, " "); } +function printReviewStatus(summary: ProjectReviewStatus): void { + if (summary.runCount === 0) return; + + const findings = + summary.openFindingCount === 1 + ? "1 open finding" + : `${summary.openFindingCount} open findings`; + const active = + summary.activeRunCount === 1 ? "1 active" : `${summary.activeRunCount} active`; + + console.log( + ` ${chalk.magenta("Reviews:")} ${summary.runCount} run${summary.runCount !== 1 ? "s" : ""} · ${active} · ${findings}`, + ); + + const visibleRuns = summary.runs + .filter((run) => REVIEW_ATTENTION_STATUSES.has(run.status) || run.openFindingCount > 0) + .slice(0, 5); + + for (const run of visibleRuns) { + const findingText = + run.openFindingCount === 1 + ? "1 open finding" + : `${run.openFindingCount} open findings`; + console.log( + ` ${chalk.green(run.reviewerSessionId)} ${chalk.dim(run.status)} → ${chalk.cyan(run.linkedSessionId)} ${chalk.dim(findingText)}`, + ); + } + + const hiddenCount = summary.runs.length - visibleRuns.length; + if (hiddenCount > 0) { + console.log( + chalk.dim( + ` ${hiddenCount} more review run${hiddenCount !== 1 ? "s" : ""}. Use \`ao review list ${summary.projectId}\`.`, + ), + ); + } +} + export function registerStatus(program: Command): void { program .command("status") @@ -406,8 +480,12 @@ export function registerStatus(program: Command): void { // Show projects that have no sessions too (if not filtered) const projectIds = opts.project ? [opts.project] : Object.keys(config.projects); const jsonOutput: SessionInfo[] = []; + const reviewOutput: CodeReviewRunSummary[] = []; let totalWorkers = 0; let totalOrchestrators = 0; + let totalReviewRuns = 0; + let totalActiveReviewRuns = 0; + let totalOpenReviewFindings = 0; for (const projectId of projectIds) { const projectConfig = config.projects[projectId]; @@ -416,6 +494,11 @@ export function registerStatus(program: Command): void { const projectSessions = (byProject.get(projectId) ?? []).sort((a, b) => a.id.localeCompare(b.id), ); + const reviewStatus = gatherProjectReviewStatus(projectId); + reviewOutput.push(...reviewStatus.runs); + totalReviewRuns += reviewStatus.runCount; + totalActiveReviewRuns += reviewStatus.activeRunCount; + totalOpenReviewFindings += reviewStatus.openFindingCount; // Resolve agent and SCM for this project via the shared registry const agentName = projectConfig.agent ?? config.defaults.agent; @@ -429,6 +512,7 @@ export function registerStatus(program: Command): void { if (projectSessions.length === 0) { if (!opts.json) { console.log(chalk.dim(" (no active sessions)")); + printReviewStatus(reviewStatus); console.log(); } continue; @@ -462,6 +546,7 @@ export function registerStatus(program: Command): void { if (workers.length === 0) { console.log(chalk.dim(" (no active sessions)")); + printReviewStatus(reviewStatus); console.log(); continue; } @@ -470,13 +555,23 @@ export function registerStatus(program: Command): void { for (const info of workers) { printSessionRow(info); } + printReviewStatus(reviewStatus); console.log(); } if (opts.json) { console.log( JSON.stringify( - { data: jsonOutput, meta: { hiddenTerminatedCount } }, + { + data: jsonOutput, + reviews: reviewOutput, + meta: { + hiddenTerminatedCount, + reviewRunCount: totalReviewRuns, + activeReviewRunCount: totalActiveReviewRuns, + openReviewFindingCount: totalOpenReviewFindings, + }, + }, null, 2, ), @@ -487,6 +582,12 @@ export function registerStatus(program: Command): void { ` ${totalWorkers} active session${totalWorkers !== 1 ? "s" : ""} across ${projectIds.length} project${projectIds.length !== 1 ? "s" : ""}` + (totalOrchestrators > 0 ? ` · ${totalOrchestrators} orchestrator${totalOrchestrators !== 1 ? "s" : ""}` + : "") + + (totalReviewRuns > 0 + ? ` · ${totalReviewRuns} review run${totalReviewRuns !== 1 ? "s" : ""}` + + (totalOpenReviewFindings > 0 + ? ` · ${totalOpenReviewFindings} open finding${totalOpenReviewFindings !== 1 ? "s" : ""}` + : "") : ""), ), ); diff --git a/packages/cli/src/program.ts b/packages/cli/src/program.ts index 71cb76136..6e0519a88 100644 --- a/packages/cli/src/program.ts +++ b/packages/cli/src/program.ts @@ -5,6 +5,7 @@ import { registerSession } from "./commands/session.js"; import { registerSend } from "./commands/send.js"; import { registerAcknowledge, registerReport } from "./commands/report.js"; import { registerReviewCheck } from "./commands/review-check.js"; +import { registerReview } from "./commands/review.js"; import { registerDashboard } from "./commands/dashboard.js"; import { registerOpen } from "./commands/open.js"; import { registerStart, registerStop } from "./commands/start.js"; @@ -39,6 +40,7 @@ export function createProgram(): Command { registerAcknowledge(program); registerReport(program); registerReviewCheck(program); + registerReview(program); registerDashboard(program); registerOpen(program); registerVerify(program); diff --git a/packages/core/src/__tests__/code-review-manager.test.ts b/packages/core/src/__tests__/code-review-manager.test.ts new file mode 100644 index 000000000..8c9e4802a --- /dev/null +++ b/packages/core/src/__tests__/code-review-manager.test.ts @@ -0,0 +1,738 @@ +import { randomUUID } from "node:crypto"; +import { execFileSync } from "node:child_process"; +import { existsSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { createActivitySignal } from "../activity-signal.js"; +import { createCodeReviewStore, type CodeReviewStore } from "../code-review-store.js"; +import { + buildCodexCodeReviewArgs, + CodeReviewNoOpenFindingsError, + executeCodeReviewRun, + markOutdatedCodeReviewRunsForSession, + parseReviewerOutput, + prepareGitReviewerWorkspace, + sendCodeReviewFindingsToAgent, + triggerCodeReviewForSession, +} from "../code-review-manager.js"; +import { createInitialCanonicalLifecycle } from "../lifecycle-state.js"; +import { + SessionNotFoundError, + type OrchestratorConfig, + type Session, + type SessionManager, +} from "../types.js"; + +let storeDir: string; +let store: CodeReviewStore; + +const config: OrchestratorConfig = { + configPath: "/tmp/ao/agent-orchestrator.yaml", + readyThresholdMs: 300_000, + defaults: { runtime: "tmux", agent: "codex", workspace: "worktree", notifiers: [] }, + projects: { + app: { + name: "App", + path: "/tmp/app", + defaultBranch: "main", + sessionPrefix: "app", + }, + }, + notifiers: {}, + notificationRouting: { urgent: [], action: [], warning: [], info: [] }, + reactions: {}, +}; + +function makeSession(overrides: Partial & { id?: string } = {}): Session { + const lifecycle = createInitialCanonicalLifecycle("worker", new Date("2026-05-10T10:00:00.000Z")); + lifecycle.session.state = "idle"; + lifecycle.session.reason = "awaiting_external_review"; + lifecycle.pr.state = "open"; + lifecycle.pr.reason = "review_pending"; + lifecycle.pr.number = 7; + lifecycle.pr.url = "https://github.com/acme/app/pull/7"; + lifecycle.runtime.state = "alive"; + lifecycle.runtime.reason = "process_running"; + + return { + id: "app-1", + projectId: "app", + status: "review_pending", + activity: "idle", + activitySignal: createActivitySignal("valid", { + activity: "idle", + timestamp: new Date("2026-05-10T10:00:00.000Z"), + source: "native", + }), + lifecycle, + branch: "feat/todos", + issueId: null, + pr: { + number: 7, + url: "https://github.com/acme/app/pull/7", + title: "feat: todos", + owner: "acme", + repo: "app", + branch: "feat/todos", + baseBranch: "main", + isDraft: false, + }, + workspacePath: "/tmp/app-worktree", + runtimeHandle: { id: "tmux-app-1", runtimeName: "tmux", data: {} }, + agentInfo: null, + createdAt: new Date("2026-05-10T09:00:00.000Z"), + lastActivityAt: new Date("2026-05-10T10:00:00.000Z"), + metadata: {}, + ...overrides, + }; +} + +function makeSessionManager( + session: Session | null, + overrides: Partial = {}, +): SessionManager { + const manager: SessionManager = { + get: async (sessionId: string) => (session?.id === sessionId ? session : null), + list: async () => (session ? [session] : []), + spawn: async () => { + throw new Error("not implemented"); + }, + spawnOrchestrator: async () => { + throw new Error("not implemented"); + }, + ensureOrchestrator: async () => { + throw new Error("not implemented"); + }, + relaunchOrchestrator: async () => { + throw new Error("not implemented"); + }, + restore: async () => { + throw new Error("not implemented"); + }, + kill: async () => ({ cleaned: false, alreadyTerminated: false }), + cleanup: async () => ({ killed: [], skipped: [], errors: [] }), + send: async () => {}, + claimPR: async () => { + throw new Error("not implemented"); + }, + }; + + return Object.assign(manager, overrides); +} + +beforeEach(() => { + storeDir = join(tmpdir(), `ao-test-code-review-manager-${randomUUID()}`); + mkdirSync(storeDir, { recursive: true }); + store = createCodeReviewStore("app", { storeDir }); +}); + +afterEach(() => { + rmSync(storeDir, { recursive: true, force: true }); +}); + +describe("triggerCodeReviewForSession", () => { + it("creates a queued review run linked to the worker session", async () => { + const session = makeSession(); + const run = await triggerCodeReviewForSession( + { + config, + sessionManager: makeSessionManager(session), + storeFactory: () => store, + resolveTargetSha: async () => "abc123", + now: new Date("2026-05-10T11:00:00.000Z"), + }, + { sessionId: "app-1", requestedBy: "cli" }, + ); + + expect(run).toMatchObject({ + projectId: "app", + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "queued", + targetSha: "abc123", + prNumber: 7, + prUrl: "https://github.com/acme/app/pull/7", + summary: "Review requested from CLI for app-1.", + findingCount: 0, + }); + expect(store.listRuns()).toHaveLength(1); + }); + + it("allocates reviewer ids from all prior project review runs", async () => { + store.createRun({ linkedSessionId: "app-1", reviewerSessionId: "app-rev-1" }); + store.createRun({ linkedSessionId: "app-2", reviewerSessionId: "app-rev-7" }); + + const run = await triggerCodeReviewForSession( + { + config, + sessionManager: makeSessionManager(makeSession({ id: "app-3" })), + storeFactory: () => store, + resolveTargetSha: async () => undefined, + }, + { sessionId: "app-3", requestedBy: "web" }, + ); + + expect(run.reviewerSessionId).toBe("app-rev-8"); + }); + + it("allocates unique reviewer ids for concurrent review requests", async () => { + let resolveGate: (() => void) | undefined; + const gate = new Promise((resolve) => { + resolveGate = resolve; + }); + let shaLookups = 0; + const options = { + config, + sessionManager: makeSessionManager(makeSession()), + storeFactory: () => store, + resolveTargetSha: async () => { + shaLookups++; + if (shaLookups === 2) { + resolveGate?.(); + } + await gate; + return "abc123"; + }, + }; + + const [first, second] = await Promise.all([ + triggerCodeReviewForSession(options, { sessionId: "app-1", requestedBy: "web" }), + triggerCodeReviewForSession(options, { sessionId: "app-1", requestedBy: "web" }), + ]); + + expect(new Set([first.reviewerSessionId, second.reviewerSessionId]).size).toBe(2); + expect( + store + .listRuns() + .map((run) => run.reviewerSessionId) + .sort(), + ).toEqual(["app-rev-1", "app-rev-2"]); + }); + + it("marks previous review runs for older worker SHAs as outdated", async () => { + const oldTriage = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "needs_triage", + targetSha: "old-sha", + }); + const oldClean = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-2", + status: "clean", + targetSha: "old-sha", + }); + const failed = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-3", + status: "failed", + targetSha: "old-sha", + }); + const otherWorker = store.createRun({ + linkedSessionId: "app-2", + reviewerSessionId: "app-rev-4", + status: "needs_triage", + targetSha: "old-sha", + }); + + const run = await triggerCodeReviewForSession( + { + config, + sessionManager: makeSessionManager(makeSession()), + storeFactory: () => store, + resolveTargetSha: async () => "new-sha", + now: new Date("2026-05-10T12:00:00.000Z"), + }, + { sessionId: "app-1", requestedBy: "web" }, + ); + + expect(run.reviewerSessionId).toBe("app-rev-5"); + expect(store.getRun(oldTriage.id)?.status).toBe("outdated"); + expect(store.getRun(oldClean.id)?.status).toBe("outdated"); + expect(store.getRun(failed.id)?.status).toBe("failed"); + expect(store.getRun(otherWorker.id)?.status).toBe("needs_triage"); + }); + + it("marks stale review runs outdated when the worker HEAD changes", async () => { + const oldWaiting = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "waiting_update", + targetSha: "old-sha", + }); + const currentQueued = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-2", + status: "queued", + targetSha: "new-sha", + }); + const failed = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-3", + status: "failed", + targetSha: "old-sha", + }); + + const updatedCount = await markOutdatedCodeReviewRunsForSession({ + store, + session: makeSession(), + resolveTargetSha: async () => "new-sha", + now: new Date("2026-05-10T12:00:00.000Z"), + }); + + expect(updatedCount).toBe(1); + expect(store.getRun(oldWaiting.id)?.status).toBe("outdated"); + expect(store.getRun(currentQueued.id)?.status).toBe("queued"); + expect(store.getRun(failed.id)?.status).toBe("failed"); + }); + + it("rejects missing and orchestrator sessions", async () => { + await expect( + triggerCodeReviewForSession( + { config, sessionManager: makeSessionManager(null), storeFactory: () => store }, + { sessionId: "app-404" }, + ), + ).rejects.toBeInstanceOf(SessionNotFoundError); + + await expect( + triggerCodeReviewForSession( + { + config, + sessionManager: makeSessionManager( + makeSession({ id: "app-orchestrator", metadata: { role: "orchestrator" } }), + ), + storeFactory: () => store, + }, + { sessionId: "app-orchestrator" }, + ), + ).rejects.toThrow(/Cannot request code review for orchestrator session/); + }); +}); + +describe("executeCodeReviewRun", () => { + it("runs a reviewer in an isolated workspace and persists findings", async () => { + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "queued", + targetSha: "abc123", + }); + + const summary = await executeCodeReviewRun( + { + config, + sessionManager: makeSessionManager(makeSession()), + storeFactory: () => store, + prepareWorkspace: async ({ run }) => `/tmp/reviews/${run.reviewerSessionId}`, + runReviewer: async ({ workspacePath }) => ({ + summary: `Reviewed ${workspacePath}`, + findings: [ + { + severity: "error", + title: "Broken save path", + body: "The save handler drops failed writes.", + filePath: "src/save.ts", + startLine: 12, + confidence: 0.9, + }, + ], + }), + }, + { projectId: "app", runId: run.id }, + ); + + expect(summary).toMatchObject({ + status: "needs_triage", + reviewerWorkspacePath: "/tmp/reviews/app-rev-1", + findingCount: 1, + openFindingCount: 1, + summary: "Reviewed /tmp/reviews/app-rev-1", + }); + expect(store.listFindings({ runId: run.id })[0]).toMatchObject({ + severity: "error", + title: "Broken save path", + filePath: "src/save.ts", + startLine: 12, + }); + }); + + it("falls back to the project default branch when the session PR base branch is empty", async () => { + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "queued", + targetSha: "abc123", + }); + let observedBaseRef: string | undefined; + const session = makeSession({ + pr: { + ...makeSession().pr!, + baseBranch: "", + }, + }); + + const summary = await executeCodeReviewRun( + { + config, + sessionManager: makeSessionManager(session), + storeFactory: () => store, + prepareWorkspace: async () => "/tmp/reviews/app-rev-1", + runReviewer: async ({ baseRef }) => { + observedBaseRef = baseRef; + return { findings: [] }; + }, + }, + { projectId: "app", runId: run.id }, + ); + + expect(observedBaseRef).toBe("main"); + expect(summary.status).toBe("clean"); + }); + + it("allows only one concurrent execution to claim the same queued review run", async () => { + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "queued", + targetSha: "abc123", + }); + let resolveSessionLookup: (() => void) | undefined; + const sessionLookupGate = new Promise((resolve) => { + resolveSessionLookup = resolve; + }); + let sessionLookups = 0; + let prepareCalls = 0; + const sessionManager = makeSessionManager(makeSession(), { + get: async () => { + sessionLookups++; + if (sessionLookups === 2) { + resolveSessionLookup?.(); + } + await sessionLookupGate; + return makeSession(); + }, + }); + + const first = executeCodeReviewRun( + { + config, + sessionManager, + storeFactory: () => store, + prepareWorkspace: async () => { + prepareCalls++; + return "/tmp/reviews/app-rev-1"; + }, + runReviewer: async () => ({ findings: [] }), + }, + { projectId: "app", runId: run.id }, + ); + while (sessionLookups === 0) { + await new Promise((resolve) => setTimeout(resolve, 1)); + } + const second = executeCodeReviewRun( + { + config, + sessionManager, + storeFactory: () => store, + prepareWorkspace: async () => { + prepareCalls++; + return "/tmp/reviews/app-rev-1"; + }, + runReviewer: async () => ({ findings: [] }), + }, + { projectId: "app", runId: run.id }, + ); + await Promise.resolve(); + resolveSessionLookup?.(); + + const results = await Promise.allSettled([first, second]); + + expect(results.filter((result) => result.status === "fulfilled")).toHaveLength(1); + expect(results.filter((result) => result.status === "rejected")).toHaveLength(1); + expect(prepareCalls).toBe(1); + expect(store.getRun(run.id)?.status).toBe("clean"); + }); + + it("marks clean reviews clean and records failed reviewer executions", async () => { + const cleanRun = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "queued", + }); + + const cleanSummary = await executeCodeReviewRun( + { + config, + sessionManager: makeSessionManager(makeSession()), + storeFactory: () => store, + prepareWorkspace: async () => "/tmp/reviews/app-rev-1", + runReviewer: async () => ({ rawOutput: '{"findings":[]}' }), + }, + { projectId: "app", runId: cleanRun.id }, + ); + expect(cleanSummary.status).toBe("clean"); + expect(cleanSummary.findingCount).toBe(0); + + const failedRun = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-2", + status: "queued", + }); + const failedSummary = await executeCodeReviewRun( + { + config, + sessionManager: makeSessionManager(makeSession()), + storeFactory: () => store, + prepareWorkspace: async () => "/tmp/reviews/app-rev-2", + runReviewer: async () => { + throw new Error("review command crashed"); + }, + }, + { projectId: "app", runId: failedRun.id }, + ); + expect(failedSummary.status).toBe("failed"); + expect(failedSummary.terminationReason).toBe("review command crashed"); + }); +}); + +describe("sendCodeReviewFindingsToAgent", () => { + it("sends open review findings to the linked coding worker and marks them sent", async () => { + const session = makeSession(); + const sentMessages: Array<{ sessionId: string; message: string }> = []; + const run = store.createRun({ + linkedSessionId: session.id, + reviewerSessionId: "app-rev-1", + status: "needs_triage", + targetSha: "abc123", + prNumber: 7, + prUrl: "https://github.com/acme/app/pull/7", + }); + const first = store.createFinding({ + runId: run.id, + linkedSessionId: session.id, + severity: "error", + title: "Broken save path", + body: "The save handler drops failed writes.", + filePath: "src/save.ts", + startLine: 12, + confidence: 0.9, + }); + const second = store.createFinding({ + runId: run.id, + linkedSessionId: session.id, + severity: "warning", + title: "Missing retry", + body: "The request fails permanently on transient network errors.", + filePath: "src/api.ts", + startLine: 4, + endLine: 8, + }); + const dismissed = store.createFinding({ + runId: run.id, + linkedSessionId: session.id, + severity: "info", + title: "Dismissed nit", + body: "This should not be sent.", + status: "dismissed", + }); + + const result = await sendCodeReviewFindingsToAgent( + { + config, + sessionManager: makeSessionManager(session, { + send: async (sessionId, message) => { + sentMessages.push({ sessionId, message }); + }, + }), + storeFactory: () => store, + now: () => new Date("2026-05-10T12:00:00.000Z"), + }, + { projectId: "app", runId: run.id }, + ); + + expect(sentMessages).toHaveLength(1); + expect(sentMessages[0]?.sessionId).toBe("app-1"); + expect(sentMessages[0]?.message).toContain("AO reviewer app-rev-1 found 2 open issues"); + expect(sentMessages[0]?.message).toContain("Review run:"); + expect(sentMessages[0]?.message).toContain("[error] Broken save path"); + expect(sentMessages[0]?.message).toContain("Location: src/save.ts:12"); + expect(sentMessages[0]?.message).toContain("[warning] Missing retry"); + expect(sentMessages[0]?.message).toContain("Location: src/api.ts:4-8"); + expect(sentMessages[0]?.message).not.toContain("Dismissed nit"); + expect(result).toMatchObject({ + sentFindingCount: 2, + run: { + status: "waiting_update", + openFindingCount: 0, + sentFindingCount: 2, + dismissedFindingCount: 1, + }, + }); + expect(store.getFinding(first.id)).toMatchObject({ + status: "sent_to_agent", + sentToAgentAt: "2026-05-10T12:00:00.000Z", + }); + expect(store.getFinding(second.id)?.status).toBe("sent_to_agent"); + expect(store.getFinding(dismissed.id)?.status).toBe("dismissed"); + }); + + it("does not send or mutate when there are no open findings", async () => { + const session = makeSession(); + const run = store.createRun({ + linkedSessionId: session.id, + reviewerSessionId: "app-rev-1", + status: "clean", + }); + store.createFinding({ + runId: run.id, + linkedSessionId: session.id, + severity: "warning", + title: "Already sent", + body: "Do not resend this.", + status: "sent_to_agent", + }); + const sentMessages: string[] = []; + + await expect( + sendCodeReviewFindingsToAgent( + { + config, + sessionManager: makeSessionManager(session, { + send: async (_sessionId, message) => { + sentMessages.push(message); + }, + }), + storeFactory: () => store, + }, + { projectId: "app", runId: run.id }, + ), + ).rejects.toBeInstanceOf(CodeReviewNoOpenFindingsError); + + expect(sentMessages).toEqual([]); + expect(store.getRun(run.id)?.status).toBe("clean"); + }); +}); + +describe("runCodexCodeReview", () => { + it("uses generic codex exec instead of the review subcommand base/prompt combination", () => { + const args = buildCodexCodeReviewArgs("/tmp/review-output.json", "Return JSON only."); + + expect(args).toEqual([ + "exec", + "--sandbox", + "read-only", + "--output-last-message", + "/tmp/review-output.json", + "Return JSON only.", + ]); + expect(args).not.toContain("review"); + expect(args).not.toContain("--base"); + }); +}); + +describe("prepareGitReviewerWorkspace", () => { + it("prunes stale git worktree metadata when the reviewer workspace directory is gone", async () => { + const tmpHome = join(tmpdir(), `ao-test-review-worktree-${randomUUID()}`); + const originalHome = process.env["HOME"]; + process.env["HOME"] = tmpHome; + + try { + const repoPath = join(tmpHome, "repo"); + mkdirSync(repoPath, { recursive: true }); + execFileSync("git", ["init", "-b", "main"], { cwd: repoPath }); + writeFileSync(join(repoPath, "README.md"), "# App\n"); + execFileSync("git", ["add", "README.md"], { cwd: repoPath }); + execFileSync("git", ["commit", "-m", "initial"], { + cwd: repoPath, + env: { + ...process.env, + GIT_AUTHOR_NAME: "AO Test", + GIT_AUTHOR_EMAIL: "ao@example.com", + GIT_COMMITTER_NAME: "AO Test", + GIT_COMMITTER_EMAIL: "ao@example.com", + }, + }); + + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-stale", + status: "queued", + }); + const project = { ...config.projects.app!, path: repoPath }; + const workspaceRoot = join( + tmpHome, + ".agent-orchestrator", + "projects", + "app", + "code-reviews", + "workspaces", + ); + const workspacePath = join(workspaceRoot, run.reviewerSessionId); + mkdirSync(workspaceRoot, { recursive: true }); + execFileSync("git", ["worktree", "add", "--detach", workspacePath, "HEAD"], { + cwd: repoPath, + }); + rmSync(workspacePath, { recursive: true, force: true }); + + const preparedPath = await prepareGitReviewerWorkspace({ + projectId: "app", + project, + session: makeSession({ workspacePath: repoPath }), + run, + }); + + expect(preparedPath).toBe(workspacePath); + expect(existsSync(workspacePath)).toBe(true); + } finally { + if (originalHome === undefined) { + delete process.env["HOME"]; + } else { + process.env["HOME"] = originalHome; + } + rmSync(tmpHome, { recursive: true, force: true }); + } + }); +}); + +describe("parseReviewerOutput", () => { + it("parses JSON findings and falls back to a single reviewer-output finding", () => { + expect( + parseReviewerOutput( + JSON.stringify({ + findings: [{ severity: "warning", title: "Risk", body: "A concrete issue." }], + }), + ), + ).toMatchObject([{ severity: "warning", title: "Risk", body: "A concrete issue." }]); + expect(parseReviewerOutput("No findings.")).toEqual([]); + expect(parseReviewerOutput("Unexpected reviewer text")).toMatchObject([ + { severity: "warning", title: "Reviewer output", body: "Unexpected reviewer text" }, + ]); + }); + + it("does not drop structured findings whose text mentions no findings", () => { + expect( + parseReviewerOutput( + JSON.stringify({ + findings: [ + { + severity: "warning", + title: "No findings banner is stale", + body: "The UI still says no findings even when the reviewer found one.", + filePath: "src/review.ts", + startLine: 42, + }, + ], + }), + ), + ).toMatchObject([ + { + severity: "warning", + title: "No findings banner is stale", + body: "The UI still says no findings even when the reviewer found one.", + filePath: "src/review.ts", + startLine: 42, + }, + ]); + }); +}); diff --git a/packages/core/src/__tests__/code-review-store.test.ts b/packages/core/src/__tests__/code-review-store.test.ts new file mode 100644 index 000000000..2f7e7c422 --- /dev/null +++ b/packages/core/src/__tests__/code-review-store.test.ts @@ -0,0 +1,101 @@ +import { randomUUID } from "node:crypto"; +import { mkdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { createCodeReviewStore, type CodeReviewStore } from "../code-review-store.js"; + +let storeDir: string; +let store: CodeReviewStore; + +beforeEach(() => { + storeDir = join(tmpdir(), `ao-test-code-review-store-${randomUUID()}`); + mkdirSync(storeDir, { recursive: true }); + store = createCodeReviewStore("app", { storeDir }); +}); + +afterEach(() => { + rmSync(storeDir, { recursive: true, force: true }); +}); + +describe("CodeReviewStore", () => { + it("creates runs and lists summaries with finding counts", () => { + const run = store.createRun( + { + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "needs_triage", + targetSha: "abc123", + prNumber: 42, + }, + new Date("2026-05-10T10:00:00.000Z"), + ); + + store.createFinding( + { + runId: run.id, + linkedSessionId: "app-1", + severity: "error", + title: "Missing guard", + body: "The handler reads a nullable value without checking it.", + filePath: "src/auth.ts", + startLine: 12, + fingerprint: "auth-guard", + }, + new Date("2026-05-10T10:01:00.000Z"), + ); + const dismissed = store.createFinding( + { + runId: run.id, + linkedSessionId: "app-1", + severity: "info", + title: "Naming nit", + body: "Consider a clearer name.", + }, + new Date("2026-05-10T10:02:00.000Z"), + ); + store.updateFinding(dismissed.id, { status: "dismissed", dismissedBy: "operator" }); + + const summaries = store.listRunSummaries({ linkedSessionId: "app-1" }); + expect(summaries).toHaveLength(1); + expect(summaries[0]).toMatchObject({ + id: run.id, + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "needs_triage", + findingCount: 2, + openFindingCount: 1, + dismissedFindingCount: 1, + }); + }); + + it("filters runs and findings independently", () => { + const first = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "clean", + }); + const second = store.createRun({ + linkedSessionId: "app-2", + reviewerSessionId: "app-rev-2", + status: "failed", + }); + store.createFinding({ + runId: second.id, + linkedSessionId: "app-2", + severity: "warning", + title: "Race condition", + body: "This update can be lost.", + }); + + expect(store.listRuns({ status: "clean" }).map((run) => run.id)).toEqual([first.id]); + expect(store.listRuns({ linkedSessionId: "app-2" }).map((run) => run.id)).toEqual([second.id]); + expect(store.listFindings({ linkedSessionId: "app-1" })).toEqual([]); + expect(store.listFindings({ linkedSessionId: "app-2" })).toHaveLength(1); + }); + + it("rejects path traversal ids", () => { + expect(() => store.getRun("../bad")).toThrow(/Unsafe review run id/); + expect(() => store.getFinding("../bad")).toThrow(/Unsafe review finding id/); + }); +}); diff --git a/packages/core/src/__tests__/global-config.test.ts b/packages/core/src/__tests__/global-config.test.ts index 25f4a755a..2e7c51628 100644 --- a/packages/core/src/__tests__/global-config.test.ts +++ b/packages/core/src/__tests__/global-config.test.ts @@ -351,6 +351,57 @@ describe("global-config storage identity", () => { }); }); + it("preserves wrapped config defaults when repairing local behavior", () => { + const repoPath = createRepo("wrapped-local-defaults", "https://github.com/OpenAI/demo.git"); + const projectId = registerProjectInGlobalConfig( + "wrapped-local-defaults", + "Wrapped Local Defaults", + repoPath, + { defaultBranch: "main" }, + configPath, + ); + writeFileSync( + join(repoPath, "agent-orchestrator.yaml"), + [ + "defaults:", + " agent: codex", + " runtime: tmux", + " workspace: worktree", + " orchestrator:", + " agent: codex", + " worker:", + " agent: opencode", + "projects:", + " wrapped-local-defaults:", + ` path: ${repoPath}`, + " name: Wrapped Local Defaults", + "", + ].join("\n"), + ); + + expect(resolveProjectIdentity(projectId, loadGlobalConfig(configPath)!, configPath)).toMatchObject({ + resolveError: expect.stringContaining("wrapped projects: format"), + }); + + repairWrappedLocalProjectConfig(projectId, repoPath); + + const repaired = parseYaml(readFileSync(join(repoPath, "agent-orchestrator.yaml"), "utf-8")); + expect(repaired).toEqual({ + agent: "codex", + runtime: "tmux", + workspace: "worktree", + orchestrator: { agent: "codex" }, + worker: { agent: "opencode" }, + }); + expect(resolveProjectIdentity(projectId, loadGlobalConfig(configPath)!, configPath)).toMatchObject({ + agent: "codex", + runtime: "tmux", + workspace: "worktree", + orchestrator: { agent: "codex" }, + worker: { agent: "opencode" }, + }); + }); + it("repairs wrapped local .yml configs without creating a .yaml sibling", () => { const repoPath = createRepo("wrapped-local-yml", "https://github.com/OpenAI/demo.git"); const configPathYml = join(repoPath, "agent-orchestrator.yml"); diff --git a/packages/core/src/__tests__/migration-storage-v2.test.ts b/packages/core/src/__tests__/migration-storage-v2.test.ts index 3d7cc4b09..915431ce9 100644 --- a/packages/core/src/__tests__/migration-storage-v2.test.ts +++ b/packages/core/src/__tests__/migration-storage-v2.test.ts @@ -12,6 +12,8 @@ import { } from "../migration/storage-v2.js"; import { readMetadata } from "../metadata.js"; +vi.setConfig({ testTimeout: 20_000 }); + function createTempDir(): string { const dir = join( tmpdir(), diff --git a/packages/core/src/__tests__/plugin-integration.test.ts b/packages/core/src/__tests__/plugin-integration.test.ts index b8e736c9c..04e38c852 100644 --- a/packages/core/src/__tests__/plugin-integration.test.ts +++ b/packages/core/src/__tests__/plugin-integration.test.ts @@ -25,7 +25,7 @@ vi.mock("node:child_process", () => { const execFile = Object.assign(vi.fn(), { [Symbol.for("nodejs.util.promisify.custom")]: ghMock, }); - return { execFile }; + return { execFile, exec: vi.fn() }; }); // --------------------------------------------------------------------------- diff --git a/packages/core/src/code-review-manager.ts b/packages/core/src/code-review-manager.ts new file mode 100644 index 000000000..756cc3d1c --- /dev/null +++ b/packages/core/src/code-review-manager.ts @@ -0,0 +1,1050 @@ +import { + closeSync, + existsSync, + mkdirSync, + openSync, + readFileSync, + rmSync, + statSync, + unlinkSync, + writeFileSync, +} from "node:fs"; +import { join } from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; +import { promisify } from "node:util"; +import { + type CodeReviewFinding, + type CodeReviewSeverity, + createCodeReviewStore, + type CodeReviewRun, + type CodeReviewRunStatus, + type CodeReviewRunSummary, + type CodeReviewStore, +} from "./code-review-store.js"; +import { getProjectCodeReviewsDir } from "./paths.js"; +import { + isOrchestratorSession, + SessionNotFoundError, + type OrchestratorConfig, + type ProjectConfig, + type Session, + type SessionManager, +} from "./types.js"; +import { getShell, isWindows, killProcessTree } from "./platform.js"; + +const REVIEW_COMMAND_TIMEOUT_MS = 10 * 60_000; +const REVIEW_COMMAND_MAX_BUFFER = 8 * 1024 * 1024; +const REVIEW_RUN_CREATION_LOCK_FILE = ".create-run.lock"; +const REVIEW_RUN_EXECUTION_LOCK_PREFIX = ".execute-run-"; +const REVIEW_RUN_CREATION_LOCK_WAIT_MS = 5_000; +const REVIEW_RUN_CREATION_LOCK_STALE_MS = 30_000; +const REVIEW_LOCK_ID_PATTERN = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; + +async function execFileAsync( + file: string, + args: string[], + options: { + cwd?: string; + timeout?: number; + maxBuffer?: number; + env?: NodeJS.ProcessEnv; + shell?: boolean | string; + windowsHide?: boolean; + } = {}, +): Promise<{ stdout: string; stderr: string }> { + const { execFile } = await import("node:child_process"); + return promisify(execFile)(file, args, { windowsHide: true, ...options }); +} + +async function execFileWithClosedStdin( + file: string, + args: string[], + options: { + cwd?: string; + timeout?: number; + maxBuffer?: number; + env?: NodeJS.ProcessEnv; + shell?: boolean | string; + windowsHide?: boolean; + } = {}, +): Promise<{ stdout: string; stderr: string }> { + const { spawn } = await import("node:child_process"); + + return new Promise((resolve, reject) => { + const child = spawn(file, args, { + cwd: options.cwd, + env: options.env, + shell: options.shell, + windowsHide: options.windowsHide ?? true, + detached: !isWindows(), + stdio: ["ignore", "pipe", "pipe"], + }); + const maxBuffer = options.maxBuffer ?? REVIEW_COMMAND_MAX_BUFFER; + let stdout = ""; + let stderr = ""; + let settled = false; + + const finish = (callback: () => void) => { + if (settled) return; + settled = true; + if (timer) clearTimeout(timer); + callback(); + }; + + const terminateChild = () => { + if (child.pid !== undefined) { + return killProcessTree(child.pid).catch(() => undefined); + } + child.kill("SIGTERM"); + return Promise.resolve(); + }; + + const fail = (message: string, code?: number | null, signal?: NodeJS.Signals | null) => { + const error = new Error(message) as Error & { + code?: number | null; + signal?: NodeJS.Signals | null; + stdout?: string; + stderr?: string; + }; + error.code = code; + error.signal = signal; + error.stdout = stdout; + error.stderr = stderr; + reject(error); + }; + + const timer = + options.timeout && options.timeout > 0 + ? setTimeout(() => { + void terminateChild().finally(() => { + finish(() => fail(`Command timed out after ${options.timeout}ms`, null, "SIGTERM")); + }); + }, options.timeout) + : null; + + const append = (kind: "stdout" | "stderr", chunk: Buffer) => { + const next = chunk.toString(); + if (kind === "stdout") stdout += next; + else stderr += next; + + if (Buffer.byteLength(stdout) + Buffer.byteLength(stderr) <= maxBuffer) return; + void terminateChild(); + finish(() => fail(`Command output exceeded maxBuffer ${maxBuffer}`)); + }; + + child.stdout?.on("data", (chunk: Buffer) => append("stdout", chunk)); + child.stderr?.on("data", (chunk: Buffer) => append("stderr", chunk)); + child.once("error", (error) => finish(() => reject(error))); + child.once("close", (code, signal) => { + finish(() => { + if (code === 0) { + resolve({ stdout, stderr }); + return; + } + fail(`Command failed with code ${code ?? signal ?? "unknown"}`, code, signal); + }); + }); + }); +} + +export type CodeReviewRequestSource = "cli" | "web" | "system"; + +export interface TriggerCodeReviewInput { + sessionId: string; + requestedBy?: CodeReviewRequestSource; + status?: CodeReviewRunStatus; + summary?: string; +} + +export interface TriggerCodeReviewOptions { + config: OrchestratorConfig; + sessionManager: SessionManager; + storeFactory?: (projectId: string) => CodeReviewStore; + resolveTargetSha?: (session: Session) => Promise; + now?: Date; +} + +export interface CodeReviewRunnerFinding { + severity?: CodeReviewSeverity; + title?: string; + body?: string; + filePath?: string; + startLine?: number; + endLine?: number; + category?: string; + confidence?: number; + fingerprint?: string; +} + +export interface CodeReviewRunnerResult { + findings?: CodeReviewRunnerFinding[]; + summary?: string; + rawOutput?: string; +} + +export interface CodeReviewRunnerContext { + config: OrchestratorConfig; + project: ProjectConfig; + session: Session; + run: CodeReviewRun; + workspacePath: string; + baseRef: string; +} + +export type CodeReviewRunner = ( + context: CodeReviewRunnerContext, +) => Promise; + +export type PrepareCodeReviewWorkspace = (context: { + projectId: string; + project: ProjectConfig; + session: Session; + run: CodeReviewRun; +}) => Promise; + +export interface ExecuteCodeReviewRunOptions { + config: OrchestratorConfig; + sessionManager: SessionManager; + storeFactory?: (projectId: string) => CodeReviewStore; + prepareWorkspace?: PrepareCodeReviewWorkspace; + runReviewer?: CodeReviewRunner; + now?: () => Date; + force?: boolean; +} + +export interface ExecuteCodeReviewRunInput { + projectId: string; + runId: string; +} + +export interface SendCodeReviewFindingsOptions { + config: OrchestratorConfig; + sessionManager: SessionManager; + storeFactory?: (projectId: string) => CodeReviewStore; + now?: () => Date; +} + +export interface SendCodeReviewFindingsInput { + projectId: string; + runId: string; +} + +export interface SendCodeReviewFindingsResult { + run: CodeReviewRunSummary; + sentFindingCount: number; + message: string; +} + +export interface MarkOutdatedCodeReviewRunsInput { + store: CodeReviewStore; + session: Session; + resolveTargetSha?: (session: Session) => Promise; + now?: Date; +} + +export class CodeReviewRunNotFoundError extends Error { + constructor(runId: string) { + super(`Code review run not found: ${runId}`); + this.name = "CodeReviewRunNotFoundError"; + } +} + +export class CodeReviewRunNotExecutableError extends Error { + readonly runId: string; + readonly reviewerSessionId: string; + readonly status: CodeReviewRunStatus; + + constructor(run: CodeReviewRun) { + super(`Code review run ${run.reviewerSessionId} is ${run.status}, not queued`); + this.name = "CodeReviewRunNotExecutableError"; + this.runId = run.id; + this.reviewerSessionId = run.reviewerSessionId; + this.status = run.status; + } +} + +export class CodeReviewInvalidSessionError extends Error { + constructor(message: string) { + super(message); + this.name = "CodeReviewInvalidSessionError"; + } +} + +export class CodeReviewNoOpenFindingsError extends Error { + readonly runId: string; + readonly reviewerSessionId: string; + + constructor(run: CodeReviewRun) { + super(`No open review findings to send for ${run.reviewerSessionId}.`); + this.name = "CodeReviewNoOpenFindingsError"; + this.runId = run.id; + this.reviewerSessionId = run.reviewerSessionId; + } +} + +function escapeRegex(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +function parsePrNumber(url: string | undefined): number | undefined { + if (!url) return undefined; + const match = url.match(/\/pull\/(\d+)(?:\D*$|$)/); + if (!match) return undefined; + const parsed = Number.parseInt(match[1], 10); + return Number.isNaN(parsed) ? undefined : parsed; +} + +function truncate(value: string, maxLength: number): string { + return value.length > maxLength ? `${value.slice(0, maxLength - 1)}…` : value; +} + +function formatFindingLocation(finding: CodeReviewFinding): string | null { + if (!finding.filePath) return null; + if (finding.startLine === undefined) return finding.filePath; + if (finding.endLine !== undefined && finding.endLine !== finding.startLine) { + return `${finding.filePath}:${finding.startLine}-${finding.endLine}`; + } + return `${finding.filePath}:${finding.startLine}`; +} + +function formatFindingForAgent(finding: CodeReviewFinding, index: number): string { + const lines = [`${index}. [${finding.severity}] ${finding.title}`]; + const location = formatFindingLocation(finding); + if (location) lines.push(` Location: ${location}`); + if (finding.confidence !== undefined) lines.push(` Confidence: ${finding.confidence}`); + lines.push(" Details:"); + lines.push( + ...finding.body + .split(/\r?\n/) + .map((line) => ` ${line}`) + .filter((line, lineIndex, allLines) => line.trim() || lineIndex < allLines.length - 1), + ); + return lines.join("\n"); +} + +function parseFiniteNumber(value: unknown): number | undefined { + return typeof value === "number" && Number.isFinite(value) ? value : undefined; +} + +function parseSeverity(value: unknown): CodeReviewSeverity { + switch (value) { + case "error": + case "warning": + case "info": + return value; + default: + return "warning"; + } +} + +function asRecord(value: unknown): Record | null { + return typeof value === "object" && value !== null && !Array.isArray(value) + ? (value as Record) + : null; +} + +function stripMarkdownJsonFence(value: string): string { + const trimmed = value.trim(); + const match = trimmed.match(/^```(?:json)?\s*([\s\S]*?)\s*```$/i); + return match?.[1]?.trim() ?? trimmed; +} + +function tryParseJsonCandidate(value: string): unknown | null { + const candidates = [stripMarkdownJsonFence(value)]; + const fenced = value.match(/```(?:json)?\s*([\s\S]*?)\s*```/i); + if (fenced?.[1]) candidates.push(fenced[1].trim()); + + for (const line of value + .split(/\r?\n/) + .map((entry) => entry.trim()) + .filter(Boolean) + .reverse()) { + if (line.startsWith("{") || line.startsWith("[")) { + candidates.push(line); + } + } + + for (const candidate of candidates) { + try { + return JSON.parse(candidate) as unknown; + } catch { + // Keep trying looser candidates. + } + } + + return null; +} + +function normalizeFinding(value: unknown, fallbackIndex: number): CodeReviewRunnerFinding | null { + const record = asRecord(value); + if (!record) return null; + + const body = + typeof record["body"] === "string" + ? record["body"].trim() + : typeof record["message"] === "string" + ? record["message"].trim() + : ""; + if (!body) return null; + + const title = + typeof record["title"] === "string" && record["title"].trim() + ? record["title"].trim() + : `Review finding ${fallbackIndex}`; + + const filePath = + typeof record["filePath"] === "string" + ? record["filePath"] + : typeof record["path"] === "string" + ? record["path"] + : undefined; + + return { + severity: parseSeverity(record["severity"]), + title: truncate(title, 160), + body: truncate(body, 12_000), + filePath, + startLine: parseFiniteNumber(record["startLine"] ?? record["line"]), + endLine: parseFiniteNumber(record["endLine"]), + category: typeof record["category"] === "string" ? record["category"] : undefined, + confidence: parseFiniteNumber(record["confidence"]), + fingerprint: typeof record["fingerprint"] === "string" ? record["fingerprint"] : undefined, + }; +} + +export function parseReviewerOutput(output: string): CodeReviewRunnerFinding[] { + const trimmed = output.trim(); + if (!trimmed) return []; + + const parsed = tryParseJsonCandidate(trimmed); + const parsedRecord = asRecord(parsed); + const rawFindings = Array.isArray(parsed) + ? parsed + : Array.isArray(parsedRecord?.["findings"]) + ? parsedRecord["findings"] + : null; + + if (rawFindings) { + return rawFindings + .map((finding, index) => normalizeFinding(finding, index + 1)) + .filter((finding): finding is CodeReviewRunnerFinding => finding !== null); + } + + if (/^no findings?\.?$/i.test(trimmed)) return []; + + return [ + { + severity: "warning", + title: "Reviewer output", + body: truncate(trimmed, 12_000), + }, + ]; +} + +function allocateReviewerSessionId(existingRuns: CodeReviewRun[], sessionPrefix: string): string { + let max = 0; + const pattern = new RegExp(`^${escapeRegex(sessionPrefix)}-rev-(\\d+)$`); + + for (const run of existingRuns) { + const match = run.reviewerSessionId.match(pattern); + if (!match) continue; + const parsed = Number.parseInt(match[1], 10); + if (!Number.isNaN(parsed) && parsed > max) { + max = parsed; + } + } + + return `${sessionPrefix}-rev-${max + 1}`; +} + +function isFsErrorWithCode(error: unknown, code: string): boolean { + return ( + typeof error === "object" && + error !== null && + "code" in error && + (error as { code?: unknown }).code === code + ); +} + +function assertSafeReviewLockId(id: string, label: string): void { + if (!id || id === "." || id === ".." || !REVIEW_LOCK_ID_PATTERN.test(id)) { + throw new Error(`Unsafe ${label}: "${id}"`); + } +} + +async function acquireCodeReviewStoreLock({ + store, + lockFileName, + label, +}: { + store: CodeReviewStore; + lockFileName: string; + label: string; +}): Promise<() => void> { + mkdirSync(store.storeDir, { recursive: true }); + const lockPath = join(store.storeDir, lockFileName); + const startedAt = Date.now(); + + while (true) { + try { + const fd = openSync(lockPath, "wx"); + writeFileSync(fd, `${process.pid}\n${new Date().toISOString()}\n`); + let released = false; + return () => { + if (released) return; + released = true; + try { + closeSync(fd); + } catch { + // The descriptor may already be closed if process cleanup raced with release. + } + try { + unlinkSync(lockPath); + } catch { + // Another process may already have cleaned up a stale lock. + } + }; + } catch (error) { + if (!isFsErrorWithCode(error, "EEXIST")) { + throw error; + } + + try { + const lockAgeMs = Date.now() - statSync(lockPath).mtimeMs; + if (lockAgeMs > REVIEW_RUN_CREATION_LOCK_STALE_MS) { + unlinkSync(lockPath); + continue; + } + } catch (staleError) { + if (isFsErrorWithCode(staleError, "ENOENT")) { + continue; + } + } + + if (Date.now() - startedAt > REVIEW_RUN_CREATION_LOCK_WAIT_MS) { + throw new Error(`Timed out waiting for ${label} lock`, { cause: error }); + } + await delay(25); + } + } +} + +async function withReviewRunCreationLock( + store: CodeReviewStore, + callback: () => T | Promise, +): Promise { + const release = await acquireCodeReviewStoreLock({ + store, + lockFileName: REVIEW_RUN_CREATION_LOCK_FILE, + label: "code review run creation", + }); + try { + return await callback(); + } finally { + release(); + } +} + +async function withReviewRunExecutionLock( + store: CodeReviewStore, + runId: string, + callback: () => T | Promise, +): Promise { + assertSafeReviewLockId(runId, "review run id"); + const release = await acquireCodeReviewStoreLock({ + store, + lockFileName: `${REVIEW_RUN_EXECUTION_LOCK_PREFIX}${runId}.lock`, + label: `code review run ${runId} execution`, + }); + try { + return await callback(); + } finally { + release(); + } +} + +const SUPERSEDABLE_RUN_STATUSES: ReadonlySet = new Set([ + "queued", + "needs_triage", + "sent_to_agent", + "waiting_update", + "clean", +]); + +function markSupersededReviewRuns({ + store, + existingRuns, + linkedSessionId, + targetSha, + now, +}: { + store: CodeReviewStore; + existingRuns: CodeReviewRun[]; + linkedSessionId: string; + targetSha: string | undefined; + now: Date; +}): number { + if (!targetSha) return 0; + + let updatedCount = 0; + + for (const run of existingRuns) { + if (run.linkedSessionId !== linkedSessionId) continue; + if (!run.targetSha || run.targetSha === targetSha) continue; + if (!SUPERSEDABLE_RUN_STATUSES.has(run.status)) continue; + store.updateRun(run.id, { status: "outdated" }, now); + updatedCount++; + } + + return updatedCount; +} + +async function resolveGitHeadSha(session: Session): Promise { + const cwd = session.workspacePath; + if (!cwd) return undefined; + + try { + const { stdout } = await execFileAsync("git", ["rev-parse", "HEAD"], { + cwd, + timeout: 5_000, + }); + const sha = stdout.trim(); + return sha.length > 0 ? sha : undefined; + } catch { + return undefined; + } +} + +async function git(cwd: string, args: string[], timeout = 30_000): Promise { + const { stdout } = await execFileAsync("git", args, { cwd, timeout }); + return stdout.trim(); +} + +async function resolveWorkspaceHead( + workspacePath: string | null | undefined, +): Promise { + if (!workspacePath) return undefined; + try { + return await git(workspacePath, ["rev-parse", "HEAD"], 5_000); + } catch { + return undefined; + } +} + +async function removeReviewerWorktree(repoPath: string, workspacePath: string): Promise { + if (!existsSync(workspacePath)) { + try { + await git(repoPath, ["worktree", "prune"]); + } catch { + // Best-effort cleanup of stale git metadata before adding the worktree again. + } + return; + } + + try { + await git(repoPath, ["worktree", "remove", "--force", workspacePath]); + return; + } catch { + try { + await git(repoPath, ["worktree", "prune"]); + } catch { + // Best-effort before falling back to directory removal. + } + rmSync(workspacePath, { recursive: true, force: true }); + } +} + +export async function markOutdatedCodeReviewRunsForSession({ + store, + session, + resolveTargetSha = resolveGitHeadSha, + now = new Date(), +}: MarkOutdatedCodeReviewRunsInput): Promise { + const targetSha = await resolveTargetSha(session); + return markSupersededReviewRuns({ + store, + existingRuns: store.listRuns({ linkedSessionId: session.id }), + linkedSessionId: session.id, + targetSha, + now, + }); +} + +export async function prepareGitReviewerWorkspace({ + projectId, + project, + session, + run, +}: { + projectId: string; + project: ProjectConfig; + session: Session; + run: CodeReviewRun; +}): Promise { + const workspaceRoot = join(getProjectCodeReviewsDir(projectId), "workspaces"); + const workspacePath = join(workspaceRoot, run.reviewerSessionId); + mkdirSync(workspaceRoot, { recursive: true }); + await removeReviewerWorktree(project.path, workspacePath); + + const ref = run.targetSha ?? (await resolveWorkspaceHead(session.workspacePath)) ?? "HEAD"; + await git(project.path, ["worktree", "add", "--detach", workspacePath, ref], 60_000); + return workspacePath; +} + +function buildDefaultReviewPrompt(context: CodeReviewRunnerContext): string { + return [ + "You are an AO reviewer agent. Review this repository snapshot for concrete bugs only.", + "Do not modify files. Do not publish comments anywhere.", + `Review the changes against base ref "${context.baseRef}". Start with: git diff --merge-base ${context.baseRef} HEAD -- .`, + "If that diff command fails, inspect git status/log and compare this detached reviewer workspace to the base ref using read-only commands.", + `Linked coding worker: ${context.session.id}`, + `Reviewer run: ${context.run.reviewerSessionId}`, + `Base ref: ${context.baseRef}`, + "Return only JSON using this schema:", + '{"findings":[{"severity":"warning|error|info","title":"short title","body":"specific issue and fix","filePath":"optional/path","startLine":1,"endLine":1,"confidence":0.8}]}', + 'If there are no concrete bugs, return {"findings":[]}.', + ].join("\n"); +} + +async function readOutputFile(path: string): Promise { + if (!existsSync(path)) return null; + try { + return readFileSync(path, "utf-8"); + } catch { + return null; + } +} + +export function createShellCodeReviewRunner(command: string): CodeReviewRunner { + return async (context) => { + const shell = getShell(); + const { stdout, stderr } = await execFileAsync(shell.cmd, shell.args(command), { + cwd: context.workspacePath, + timeout: REVIEW_COMMAND_TIMEOUT_MS, + maxBuffer: REVIEW_COMMAND_MAX_BUFFER, + env: process.env, + }); + return { rawOutput: stdout.trim() || stderr.trim() }; + }; +} + +export function buildCodexCodeReviewArgs(outputFile: string, prompt: string): string[] { + return ["exec", "--sandbox", "read-only", "--output-last-message", outputFile, prompt]; +} + +export async function runCodexCodeReview( + context: CodeReviewRunnerContext, +): Promise { + const outputFile = join(context.workspacePath, ".ao-code-review-output.json"); + const prompt = buildDefaultReviewPrompt(context); + const args = buildCodexCodeReviewArgs(outputFile, prompt); + + try { + const { stdout, stderr } = await execFileWithClosedStdin("codex", args, { + cwd: context.workspacePath, + timeout: REVIEW_COMMAND_TIMEOUT_MS, + maxBuffer: REVIEW_COMMAND_MAX_BUFFER, + env: process.env, + shell: isWindows(), + }); + const outputFileContents = await readOutputFile(outputFile); + const rawOutput = outputFileContents ?? (stdout.trim() || stderr.trim()); + return { rawOutput }; + } catch (error) { + const details = + error instanceof Error && "stderr" in error && typeof error.stderr === "string" + ? error.stderr.trim() + : error instanceof Error + ? error.message + : String(error); + throw new Error(`Codex review failed: ${details}`, { cause: error }); + } +} + +function defaultReviewSummary(session: Session, source: CodeReviewRequestSource): string { + const sourceLabel = source === "cli" ? "CLI" : source === "web" ? "dashboard" : "automation"; + return `Review requested from ${sourceLabel} for ${session.id}.`; +} + +export function formatCodeReviewFindingsForAgent({ + run, + findings, + session, +}: { + run: CodeReviewRun; + findings: CodeReviewFinding[]; + session: Session; +}): string { + const prLabel = run.prNumber + ? `PR #${run.prNumber}${run.prUrl ? ` (${run.prUrl})` : ""}` + : run.prUrl + ? `PR ${run.prUrl}` + : "the current PR"; + const targetLabel = run.targetSha ? `\nTarget SHA reviewed: ${run.targetSha}` : ""; + + return [ + `AO reviewer ${run.reviewerSessionId} found ${findings.length} open issue${ + findings.length === 1 ? "" : "s" + } for ${prLabel}.`, + `Linked coding worker: ${session.id}`, + `Review run: ${run.id}${targetLabel}`, + "", + "Please address each finding below. Verify each issue against the current source before editing, then update the PR branch and push your fixes.", + "When you start working on these, report `ao report addressing-reviews`. When the fixes are ready for another review, report `ao report ready-for-review`.", + "", + "Findings:", + findings.map((finding, index) => formatFindingForAgent(finding, index + 1)).join("\n\n"), + ].join("\n"); +} + +export async function triggerCodeReviewForSession( + { + config, + sessionManager, + storeFactory = createCodeReviewStore, + resolveTargetSha = resolveGitHeadSha, + now = new Date(), + }: TriggerCodeReviewOptions, + input: TriggerCodeReviewInput, +): Promise { + const session = await sessionManager.get(input.sessionId); + if (!session) { + throw new SessionNotFoundError(input.sessionId); + } + + const project = config.projects[session.projectId]; + if (!project) { + throw new CodeReviewInvalidSessionError( + `Unknown project for session ${session.id}: ${session.projectId}`, + ); + } + + const sessionPrefix = project.sessionPrefix ?? session.projectId; + const allSessionPrefixes = Object.entries(config.projects).map( + ([projectId, projectConfig]) => projectConfig.sessionPrefix ?? projectId, + ); + if (isOrchestratorSession(session, sessionPrefix, allSessionPrefixes)) { + throw new CodeReviewInvalidSessionError( + `Cannot request code review for orchestrator session: ${session.id}`, + ); + } + + const store = storeFactory(session.projectId); + const prUrl = session.pr?.url ?? session.metadata["pr"]; + const prNumber = session.pr?.number ?? parsePrNumber(prUrl); + const targetSha = await resolveTargetSha(session); + const requestedBy = input.requestedBy ?? "system"; + + return withReviewRunCreationLock(store, () => { + const existingRuns = store.listRuns(); + const reviewerSessionId = allocateReviewerSessionId(existingRuns, sessionPrefix); + + markSupersededReviewRuns({ + store, + existingRuns, + linkedSessionId: session.id, + targetSha, + now, + }); + + const run = store.createRun( + { + linkedSessionId: session.id, + reviewerSessionId, + status: input.status ?? "queued", + targetSha, + prNumber, + prUrl, + summary: input.summary ?? defaultReviewSummary(session, requestedBy), + }, + now, + ); + + return { + ...run, + findingCount: 0, + openFindingCount: 0, + dismissedFindingCount: 0, + sentFindingCount: 0, + resolvedFindingCount: 0, + }; + }); +} + +function summarizeRun(store: CodeReviewStore, runId: string): CodeReviewRunSummary { + const run = store.listRunSummaries().find((entry) => entry.id === runId); + if (!run) { + throw new CodeReviewRunNotFoundError(runId); + } + return run; +} + +function getExecutableRun(store: CodeReviewStore, runId: string, force: boolean): CodeReviewRun { + const run = store.getRun(runId); + if (!run) { + throw new CodeReviewRunNotFoundError(runId); + } + + if (run.status === "preparing" || run.status === "running") { + throw new CodeReviewRunNotExecutableError(run); + } + + if (!force && !["queued", "failed"].includes(run.status)) { + throw new CodeReviewRunNotExecutableError(run); + } + + return run; +} + +export async function executeCodeReviewRun( + { + config, + sessionManager, + storeFactory = createCodeReviewStore, + prepareWorkspace = prepareGitReviewerWorkspace, + runReviewer = runCodexCodeReview, + now = () => new Date(), + force = false, + }: ExecuteCodeReviewRunOptions, + { projectId, runId }: ExecuteCodeReviewRunInput, +): Promise { + const project = config.projects[projectId]; + if (!project) { + throw new Error(`Unknown project: ${projectId}`); + } + + const store = storeFactory(projectId); + const claimed = await withReviewRunExecutionLock(store, runId, async () => { + const executableRun = getExecutableRun(store, runId, force); + const session = await sessionManager.get(executableRun.linkedSessionId); + if (!session) { + throw new SessionNotFoundError(executableRun.linkedSessionId); + } + + const startedAt = now(); + const run = store.updateRun( + executableRun.id, + { + status: "preparing", + startedAt: executableRun.startedAt ?? startedAt.toISOString(), + completedAt: undefined, + terminationReason: undefined, + }, + startedAt, + ); + + return { run, session }; + }); + let run = claimed.run; + const session = claimed.session; + + try { + const workspacePath = await prepareWorkspace({ projectId, project, session, run }); + run = store.updateRun( + run.id, + { status: "running", reviewerWorkspacePath: workspacePath }, + now(), + ); + const baseRef = session.pr?.baseBranch?.trim() || project.defaultBranch; + const result = await runReviewer({ config, project, session, run, workspacePath, baseRef }); + const findings = result.findings ?? parseReviewerOutput(result.rawOutput ?? ""); + + for (const finding of findings) { + store.createFinding( + { + runId: run.id, + linkedSessionId: run.linkedSessionId, + severity: finding.severity ?? "warning", + title: finding.title?.trim() || "Review finding", + body: finding.body?.trim() || "Reviewer reported an issue without details.", + filePath: finding.filePath, + startLine: finding.startLine, + endLine: finding.endLine, + category: finding.category, + confidence: finding.confidence, + fingerprint: finding.fingerprint, + }, + now(), + ); + } + + const completedAt = now(); + store.updateRun( + run.id, + { + status: findings.length > 0 ? "needs_triage" : "clean", + completedAt: completedAt.toISOString(), + summary: result.summary ?? run.summary, + terminationReason: undefined, + }, + completedAt, + ); + } catch (error) { + const completedAt = now(); + store.updateRun( + run.id, + { + status: "failed", + completedAt: completedAt.toISOString(), + terminationReason: error instanceof Error ? error.message : String(error), + }, + completedAt, + ); + } + + return summarizeRun(store, run.id); +} + +export async function sendCodeReviewFindingsToAgent( + { + config, + sessionManager, + storeFactory = createCodeReviewStore, + now = () => new Date(), + }: SendCodeReviewFindingsOptions, + { projectId, runId }: SendCodeReviewFindingsInput, +): Promise { + const project = config.projects[projectId]; + if (!project) { + throw new Error(`Unknown project: ${projectId}`); + } + + const store = storeFactory(projectId); + const run = store.getRun(runId); + if (!run) { + throw new CodeReviewRunNotFoundError(runId); + } + + const session = await sessionManager.get(run.linkedSessionId); + if (!session) { + throw new SessionNotFoundError(run.linkedSessionId); + } + + const findings = store.listFindings({ runId: run.id, status: "open" }); + if (findings.length === 0) { + throw new CodeReviewNoOpenFindingsError(run); + } + + const message = formatCodeReviewFindingsForAgent({ run, findings, session }); + await sessionManager.send(session.id, message); + + const sentAt = now(); + for (const finding of findings) { + store.updateFinding( + finding.id, + { + status: "sent_to_agent", + sentToAgentAt: sentAt.toISOString(), + }, + sentAt, + ); + } + + store.updateRun(run.id, { status: "waiting_update" }, sentAt); + + return { + run: summarizeRun(store, run.id), + sentFindingCount: findings.length, + message, + }; +} diff --git a/packages/core/src/code-review-store.ts b/packages/core/src/code-review-store.ts new file mode 100644 index 000000000..5f8875314 --- /dev/null +++ b/packages/core/src/code-review-store.ts @@ -0,0 +1,504 @@ +import { randomUUID } from "node:crypto"; +import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { atomicWriteFileSync } from "./atomic-write.js"; +import { getProjectCodeReviewsDir } from "./paths.js"; + +export type CodeReviewRunStatus = + | "queued" + | "preparing" + | "running" + | "needs_triage" + | "sent_to_agent" + | "waiting_update" + | "clean" + | "outdated" + | "failed" + | "cancelled"; + +export type CodeReviewFindingStatus = "open" | "dismissed" | "sent_to_agent" | "resolved"; + +export type CodeReviewSeverity = "info" | "warning" | "error"; + +export interface CodeReviewRun { + id: string; + projectId: string; + linkedSessionId: string; + reviewerSessionId: string; + status: CodeReviewRunStatus; + createdAt: string; + updatedAt: string; + startedAt?: string; + completedAt?: string; + targetSha?: string; + baseSha?: string; + prNumber?: number; + prUrl?: string; + reviewerWorkspacePath?: string; + summary?: string; + terminationReason?: string; +} + +export interface CodeReviewFinding { + id: string; + projectId: string; + runId: string; + linkedSessionId: string; + status: CodeReviewFindingStatus; + severity: CodeReviewSeverity; + title: string; + body: string; + filePath?: string; + startLine?: number; + endLine?: number; + category?: string; + confidence?: number; + fingerprint?: string; + createdAt: string; + updatedAt: string; + dismissedAt?: string; + dismissedBy?: string; + sentToAgentAt?: string; +} + +export interface CodeReviewRunSummary extends CodeReviewRun { + findingCount: number; + openFindingCount: number; + dismissedFindingCount: number; + sentFindingCount: number; + resolvedFindingCount: number; +} + +export interface CodeReviewStoreOptions { + /** Override storage dir for tests. Defaults to the project code review store path. */ + storeDir?: string; +} + +export interface ListCodeReviewRunsFilter { + linkedSessionId?: string; + status?: CodeReviewRunStatus; +} + +export interface ListCodeReviewFindingsFilter { + runId?: string; + linkedSessionId?: string; + status?: CodeReviewFindingStatus; +} + +export interface CreateCodeReviewRunInput { + linkedSessionId: string; + reviewerSessionId: string; + status?: CodeReviewRunStatus; + targetSha?: string; + baseSha?: string; + prNumber?: number; + prUrl?: string; + reviewerWorkspacePath?: string; + summary?: string; +} + +export interface CreateCodeReviewFindingInput { + runId: string; + linkedSessionId: string; + severity: CodeReviewSeverity; + title: string; + body: string; + status?: CodeReviewFindingStatus; + filePath?: string; + startLine?: number; + endLine?: number; + category?: string; + confidence?: number; + fingerprint?: string; +} + +const REVIEW_ID_PATTERN = /^[a-zA-Z0-9][a-zA-Z0-9._-]*$/; + +function assertSafeReviewId(id: string, label: string): void { + if (!id || id === "." || id === ".." || !REVIEW_ID_PATTERN.test(id)) { + throw new Error(`Unsafe ${label}: "${id}"`); + } +} + +function normalizeIsoTimestamp(value: unknown): string | undefined { + if (typeof value !== "string") return undefined; + const parsed = Date.parse(value); + return Number.isNaN(parsed) ? undefined : new Date(parsed).toISOString(); +} + +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function parseNumber(value: unknown): number | undefined { + return typeof value === "number" && Number.isFinite(value) ? value : undefined; +} + +function parseOptionalString(value: unknown): string | undefined { + return typeof value === "string" && value.trim().length > 0 ? value : undefined; +} + +function parseRunStatus(value: unknown): CodeReviewRunStatus { + switch (value) { + case "queued": + case "preparing": + case "running": + case "needs_triage": + case "sent_to_agent": + case "waiting_update": + case "clean": + case "outdated": + case "failed": + case "cancelled": + return value; + default: + return "queued"; + } +} + +function parseFindingStatus(value: unknown): CodeReviewFindingStatus { + switch (value) { + case "dismissed": + case "sent_to_agent": + case "resolved": + return value; + case "open": + default: + return "open"; + } +} + +function parseSeverity(value: unknown): CodeReviewSeverity { + switch (value) { + case "error": + case "warning": + case "info": + return value; + default: + return "warning"; + } +} + +function readJsonFile(path: string): unknown | null { + try { + return JSON.parse(readFileSync(path, "utf-8")) as unknown; + } catch { + return null; + } +} + +function writeJsonFile(path: string, value: unknown): void { + atomicWriteFileSync(path, `${JSON.stringify(value, null, 2)}\n`); +} + +function removeUndefined>(value: T): T { + return Object.fromEntries( + Object.entries(value).filter(([, entryValue]) => entryValue !== undefined), + ) as T; +} + +function compareUpdatedDesc( + a: { updatedAt: string; createdAt: string; id: string }, + b: { updatedAt: string; createdAt: string; id: string }, +): number { + return ( + Date.parse(b.updatedAt) - Date.parse(a.updatedAt) || + Date.parse(b.createdAt) - Date.parse(a.createdAt) || + a.id.localeCompare(b.id) + ); +} + +function parseRun(projectId: string, value: unknown): CodeReviewRun | null { + if (!isRecord(value)) return null; + const id = parseOptionalString(value["id"]); + const linkedSessionId = parseOptionalString(value["linkedSessionId"]); + const reviewerSessionId = parseOptionalString(value["reviewerSessionId"]); + const createdAt = normalizeIsoTimestamp(value["createdAt"]); + const updatedAt = normalizeIsoTimestamp(value["updatedAt"]) ?? createdAt; + if (!id || !linkedSessionId || !reviewerSessionId || !createdAt || !updatedAt) return null; + + return removeUndefined({ + id, + projectId: parseOptionalString(value["projectId"]) ?? projectId, + linkedSessionId, + reviewerSessionId, + status: parseRunStatus(value["status"]), + createdAt, + updatedAt, + startedAt: normalizeIsoTimestamp(value["startedAt"]), + completedAt: normalizeIsoTimestamp(value["completedAt"]), + targetSha: parseOptionalString(value["targetSha"]), + baseSha: parseOptionalString(value["baseSha"]), + prNumber: parseNumber(value["prNumber"]), + prUrl: parseOptionalString(value["prUrl"]), + reviewerWorkspacePath: parseOptionalString(value["reviewerWorkspacePath"]), + summary: parseOptionalString(value["summary"]), + terminationReason: parseOptionalString(value["terminationReason"]), + }); +} + +function parseFinding(projectId: string, value: unknown): CodeReviewFinding | null { + if (!isRecord(value)) return null; + const id = parseOptionalString(value["id"]); + const runId = parseOptionalString(value["runId"]); + const linkedSessionId = parseOptionalString(value["linkedSessionId"]); + const title = parseOptionalString(value["title"]); + const body = parseOptionalString(value["body"]); + const createdAt = normalizeIsoTimestamp(value["createdAt"]); + const updatedAt = normalizeIsoTimestamp(value["updatedAt"]) ?? createdAt; + if (!id || !runId || !linkedSessionId || !title || !body || !createdAt || !updatedAt) { + return null; + } + + return removeUndefined({ + id, + projectId: parseOptionalString(value["projectId"]) ?? projectId, + runId, + linkedSessionId, + status: parseFindingStatus(value["status"]), + severity: parseSeverity(value["severity"]), + title, + body, + filePath: parseOptionalString(value["filePath"]), + startLine: parseNumber(value["startLine"]), + endLine: parseNumber(value["endLine"]), + category: parseOptionalString(value["category"]), + confidence: parseNumber(value["confidence"]), + fingerprint: parseOptionalString(value["fingerprint"]), + createdAt, + updatedAt, + dismissedAt: normalizeIsoTimestamp(value["dismissedAt"]), + dismissedBy: parseOptionalString(value["dismissedBy"]), + sentToAgentAt: normalizeIsoTimestamp(value["sentToAgentAt"]), + }); +} + +export class CodeReviewStore { + readonly projectId: string; + readonly storeDir: string; + + constructor(projectId: string, options: CodeReviewStoreOptions = {}) { + this.projectId = projectId; + this.storeDir = options.storeDir ?? getProjectCodeReviewsDir(projectId); + } + + get runsDir(): string { + return join(this.storeDir, "runs"); + } + + get findingsDir(): string { + return join(this.storeDir, "findings"); + } + + ensure(): void { + mkdirSync(this.runsDir, { recursive: true }); + mkdirSync(this.findingsDir, { recursive: true }); + } + + listRuns(filter: ListCodeReviewRunsFilter = {}): CodeReviewRun[] { + return this.readAllRuns() + .filter((run) => !filter.linkedSessionId || run.linkedSessionId === filter.linkedSessionId) + .filter((run) => !filter.status || run.status === filter.status) + .sort(compareUpdatedDesc); + } + + listRunSummaries(filter: ListCodeReviewRunsFilter = {}): CodeReviewRunSummary[] { + const findings = this.listFindings(); + const countsByRun = new Map>(); + + for (const finding of findings) { + const counts = countsByRun.get(finding.runId) ?? { + findingCount: 0, + openFindingCount: 0, + dismissedFindingCount: 0, + sentFindingCount: 0, + resolvedFindingCount: 0, + }; + counts.findingCount++; + if (finding.status === "open") counts.openFindingCount++; + if (finding.status === "dismissed") counts.dismissedFindingCount++; + if (finding.status === "sent_to_agent") counts.sentFindingCount++; + if (finding.status === "resolved") counts.resolvedFindingCount++; + countsByRun.set(finding.runId, counts); + } + + return this.listRuns(filter).map((run) => ({ + ...run, + ...(countsByRun.get(run.id) ?? { + findingCount: 0, + openFindingCount: 0, + dismissedFindingCount: 0, + sentFindingCount: 0, + resolvedFindingCount: 0, + }), + })); + } + + getRun(runId: string): CodeReviewRun | null { + assertSafeReviewId(runId, "review run id"); + return parseRun(this.projectId, readJsonFile(this.runPath(runId))); + } + + createRun(input: CreateCodeReviewRunInput, now = new Date()): CodeReviewRun { + const id = `review-run-${randomUUID()}`; + const timestamp = now.toISOString(); + const run: CodeReviewRun = removeUndefined({ + id, + projectId: this.projectId, + linkedSessionId: input.linkedSessionId, + reviewerSessionId: input.reviewerSessionId, + status: input.status ?? "queued", + createdAt: timestamp, + updatedAt: timestamp, + startedAt: input.status === "running" ? timestamp : undefined, + targetSha: input.targetSha, + baseSha: input.baseSha, + prNumber: input.prNumber, + prUrl: input.prUrl, + reviewerWorkspacePath: input.reviewerWorkspacePath, + summary: input.summary, + }); + this.writeRun(run); + return run; + } + + updateRun( + runId: string, + patch: Partial>, + now = new Date(), + ): CodeReviewRun { + const existing = this.getRun(runId); + if (!existing) { + throw new Error(`Code review run not found: ${runId}`); + } + const next = removeUndefined({ + ...existing, + ...patch, + id: existing.id, + projectId: existing.projectId, + createdAt: existing.createdAt, + updatedAt: now.toISOString(), + }); + this.writeRun(next); + return next; + } + + listFindings(filter: ListCodeReviewFindingsFilter = {}): CodeReviewFinding[] { + return this.readAllFindings() + .filter((finding) => !filter.runId || finding.runId === filter.runId) + .filter( + (finding) => !filter.linkedSessionId || finding.linkedSessionId === filter.linkedSessionId, + ) + .filter((finding) => !filter.status || finding.status === filter.status) + .sort(compareUpdatedDesc); + } + + getFinding(findingId: string): CodeReviewFinding | null { + assertSafeReviewId(findingId, "review finding id"); + return parseFinding(this.projectId, readJsonFile(this.findingPath(findingId))); + } + + createFinding(input: CreateCodeReviewFindingInput, now = new Date()): CodeReviewFinding { + if (!this.getRun(input.runId)) { + throw new Error(`Code review run not found: ${input.runId}`); + } + const id = `review-finding-${randomUUID()}`; + const timestamp = now.toISOString(); + const finding: CodeReviewFinding = removeUndefined({ + id, + projectId: this.projectId, + runId: input.runId, + linkedSessionId: input.linkedSessionId, + status: input.status ?? "open", + severity: input.severity, + title: input.title, + body: input.body, + filePath: input.filePath, + startLine: input.startLine, + endLine: input.endLine, + category: input.category, + confidence: input.confidence, + fingerprint: input.fingerprint, + createdAt: timestamp, + updatedAt: timestamp, + }); + this.writeFinding(finding); + return finding; + } + + updateFinding( + findingId: string, + patch: Partial< + Omit + >, + now = new Date(), + ): CodeReviewFinding { + const existing = this.getFinding(findingId); + if (!existing) { + throw new Error(`Code review finding not found: ${findingId}`); + } + const next = removeUndefined({ + ...existing, + ...patch, + id: existing.id, + projectId: existing.projectId, + runId: existing.runId, + linkedSessionId: existing.linkedSessionId, + createdAt: existing.createdAt, + updatedAt: now.toISOString(), + }); + this.writeFinding(next); + return next; + } + + deleteAll(): void { + rmSync(this.storeDir, { recursive: true, force: true }); + } + + private runPath(runId: string): string { + assertSafeReviewId(runId, "review run id"); + return join(this.runsDir, `${runId}.json`); + } + + private findingPath(findingId: string): string { + assertSafeReviewId(findingId, "review finding id"); + return join(this.findingsDir, `${findingId}.json`); + } + + private writeRun(run: CodeReviewRun): void { + this.ensure(); + writeJsonFile(this.runPath(run.id), run); + } + + private writeFinding(finding: CodeReviewFinding): void { + this.ensure(); + writeJsonFile(this.findingPath(finding.id), finding); + } + + private readAllRuns(): CodeReviewRun[] { + return this.readAllJsonFiles(this.runsDir) + .map((value) => parseRun(this.projectId, value)) + .filter((run): run is CodeReviewRun => run !== null); + } + + private readAllFindings(): CodeReviewFinding[] { + return this.readAllJsonFiles(this.findingsDir) + .map((value) => parseFinding(this.projectId, value)) + .filter((finding): finding is CodeReviewFinding => finding !== null); + } + + private readAllJsonFiles(dir: string): unknown[] { + if (!existsSync(dir)) return []; + return readdirSync(dir, { withFileTypes: true }) + .filter((entry) => entry.isFile() && entry.name.endsWith(".json")) + .map((entry) => readJsonFile(join(dir, entry.name))) + .filter((value) => value !== null); + } +} + +export function createCodeReviewStore( + projectId: string, + options: CodeReviewStoreOptions = {}, +): CodeReviewStore { + return new CodeReviewStore(projectId, options); +} diff --git a/packages/core/src/global-config.ts b/packages/core/src/global-config.ts index 6ad925953..fd9678d5b 100644 --- a/packages/core/src/global-config.ts +++ b/packages/core/src/global-config.ts @@ -472,6 +472,69 @@ export function writeLocalProjectConfig( return configPath; } +function isRecord(value: unknown): value is Record { + return value !== null && value !== undefined && typeof value === "object" && !Array.isArray(value); +} + +function mergeRoleBehavior( + defaults: Record, + project: Record, + key: "orchestrator" | "worker", +): Record | undefined { + const defaultRole = isRecord(defaults[key]) ? defaults[key] : undefined; + const projectRole = isRecord(project[key]) ? project[key] : undefined; + const merged = { + ...(defaultRole ?? {}), + ...(projectRole ?? {}), + }; + return Object.keys(merged).length > 0 ? merged : undefined; +} + +function buildRepairedLocalProjectConfig( + parsed: Record, + project: Record, +): Record { + const defaults = isRecord(parsed["defaults"]) ? parsed["defaults"] : {}; + const defaultBehavior: Record = {}; + for (const key of ["runtime", "agent", "workspace"] as const) { + if (defaults[key] !== null && defaults[key] !== undefined) { + defaultBehavior[key] = defaults[key]; + } + } + + const { + name: _name, + path: _path, + sessionPrefix: _sessionPrefix, + projectId: _projectId, + source: _source, + registeredAt: _registeredAt, + displayName: _displayName, + orchestrator: _orchestrator, + worker: _worker, + ...projectBehavior + } = project; + void _name; + void _path; + void _sessionPrefix; + void _projectId; + void _source; + void _registeredAt; + void _displayName; + void _orchestrator; + void _worker; + + const behavior = { + ...defaultBehavior, + ...projectBehavior, + }; + const orchestrator = mergeRoleBehavior(defaults, project, "orchestrator"); + const worker = mergeRoleBehavior(defaults, project, "worker"); + if (orchestrator) behavior["orchestrator"] = orchestrator; + if (worker) behavior["worker"] = worker; + return behavior; +} + export function repairWrappedLocalProjectConfig(projectId: string, projectPath: string): void { const localConfigResult = loadLocalProjectConfigDetailed(projectPath); if (localConfigResult.kind !== "old-format" || !localConfigResult.path) { @@ -501,24 +564,7 @@ export function repairWrappedLocalProjectConfig(projectId: string, projectPath: ); } - const { - name: _name, - path: _path, - sessionPrefix: _sessionPrefix, - projectId: _projectId, - source: _source, - registeredAt: _registeredAt, - displayName: _displayName, - ...behaviorFields - } = project; - void _name; - void _path; - void _sessionPrefix; - void _projectId; - void _source; - void _registeredAt; - void _displayName; - + const behaviorFields = buildRepairedLocalProjectConfig(parsed, project); writeLocalProjectConfig(projectPath, behaviorFields, configPath); } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 6cd385068..047bf10d5 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -43,6 +43,53 @@ export { export { createInitialCanonicalLifecycle, deriveLegacyStatus } from "./lifecycle-state.js"; export { sessionFromMetadata } from "./utils/session-from-metadata.js"; +// AO-local code review store +export { CodeReviewStore, createCodeReviewStore } from "./code-review-store.js"; +export type { + CodeReviewFinding, + CodeReviewFindingStatus, + CodeReviewRun, + CodeReviewRunStatus, + CodeReviewRunSummary, + CodeReviewSeverity, + CodeReviewStoreOptions, + CreateCodeReviewFindingInput, + CreateCodeReviewRunInput, + ListCodeReviewFindingsFilter, + ListCodeReviewRunsFilter, +} from "./code-review-store.js"; +export { + CodeReviewInvalidSessionError, + CodeReviewNoOpenFindingsError, + CodeReviewRunNotExecutableError, + CodeReviewRunNotFoundError, + createShellCodeReviewRunner, + executeCodeReviewRun, + formatCodeReviewFindingsForAgent, + markOutdatedCodeReviewRunsForSession, + parseReviewerOutput, + prepareGitReviewerWorkspace, + runCodexCodeReview, + sendCodeReviewFindingsToAgent, + triggerCodeReviewForSession, +} from "./code-review-manager.js"; +export type { + CodeReviewRunner, + CodeReviewRunnerContext, + CodeReviewRunnerFinding, + CodeReviewRunnerResult, + CodeReviewRequestSource, + ExecuteCodeReviewRunInput, + ExecuteCodeReviewRunOptions, + MarkOutdatedCodeReviewRunsInput, + PrepareCodeReviewWorkspace, + SendCodeReviewFindingsInput, + SendCodeReviewFindingsOptions, + SendCodeReviewFindingsResult, + TriggerCodeReviewInput, + TriggerCodeReviewOptions, +} from "./code-review-manager.js"; + // Lifecycle transitions — centralized transition boundary (#137) export { applyLifecycleDecision, @@ -246,6 +293,7 @@ export { getProjectDir, getProjectSessionsDir, getProjectWorktreesDir, + getProjectCodeReviewsDir, getProjectFeedbackReportsDir, getOrchestratorPath, getSessionPath, diff --git a/packages/core/src/paths.ts b/packages/core/src/paths.ts index 1485eeadc..40fdc6ce7 100644 --- a/packages/core/src/paths.ts +++ b/packages/core/src/paths.ts @@ -125,6 +125,11 @@ export function getProjectWorktreesDir(projectId: string): string { return join(getProjectDir(projectId), "worktrees"); } +/** Get the AO-local code review store directory for a project. */ +export function getProjectCodeReviewsDir(projectId: string): string { + return join(getProjectDir(projectId), "code-reviews"); +} + /** Get the feedback reports directory for a project (V2 layout). */ export function getProjectFeedbackReportsDir(projectId: string): string { return join(getProjectDir(projectId), "feedback-reports"); diff --git a/packages/core/src/prompts/orchestrator.md b/packages/core/src/prompts/orchestrator.md index 08ca7ec02..2551bc287 100644 --- a/packages/core/src/prompts/orchestrator.md +++ b/packages/core/src/prompts/orchestrator.md @@ -39,6 +39,12 @@ ao spawn --prompt "Refactor the auth module to use JWT" # List sessions ao session ls -p {{projectId}} +# List AO-local reviewer runs +ao review list {{projectId}} + +# Send completed AO-local review findings back to the linked coding worker +ao review send {{projectSessionPrefix}}-rev-1 -p {{projectId}} + # Send message to a session ao send {{projectSessionPrefix}}-1 "Your message here" @@ -64,6 +70,10 @@ ao open {{projectId}}{{REPO_CONFIGURED_SECTION_END}} - `ao spawn [issue] [--prompt ]{{REPO_CONFIGURED_SECTION_START}} [--claim-pr ]{{REPO_CONFIGURED_SECTION_END}}`: Spawn a worker session{{REPO_CONFIGURED_SECTION_START}}; use issue ID or --prompt for freeform tasks{{REPO_CONFIGURED_SECTION_END}}{{REPO_NOT_CONFIGURED_SECTION_START}} with --prompt for freeform tasks{{REPO_NOT_CONFIGURED_SECTION_END}} {{REPO_CONFIGURED_SECTION_START}}- `ao batch-spawn `: Spawn multiple sessions in parallel (project auto-detected) {{REPO_CONFIGURED_SECTION_END}}- `ao session ls [-p project]`: List all sessions (optionally filter by project) +- `ao review list [project]`: List AO-local reviewer runs. These are review agents/runs, not coding worker sessions. +- `ao review run [--execute]`: Request a reviewer run for a coding worker session. +- `ao review execute [project] [--run ]`: Execute a queued reviewer run. +- `ao review send [-p project]`: Send open AO-local findings from a completed reviewer run to its linked coding worker, then mark the run as waiting for worker updates. {{REPO_CONFIGURED_SECTION_START}}- `ao session claim-pr [session]`: Attach an existing PR to a worker session {{REPO_CONFIGURED_SECTION_END}}- `ao session attach `: Attach to a session's terminal (a tmux window on Unix; a ConPTY pty-host on Windows) - `ao session kill `: Kill a specific session @@ -96,6 +106,7 @@ ao spawn --prompt "Add rate limiting to the /api/upload endpoint" Use `ao status` to see: - Current session status (working, pr_open, review_pending, etc.) +- AO-local reviewer run summary and open finding counts {{REPO_CONFIGURED_SECTION_START}}- PR state (open/merged/closed) - CI status (passing/failing/pending) - Review decision (approved/changes_requested/pending) @@ -111,6 +122,24 @@ ao status --reports full # full audit trail per session Reach for this when an inferred status disagrees with what the worker said, when deciding whether to send a follow-up instruction vs. wait, or when triaging a session that looks stuck. +Reviewer runs are intentionally separate from coding worker sessions. A reviewer run has its own workspace and context, and does not appear in `ao session ls` as a coding session. Use `ao status` for the summary and `ao review list {{projectId}}` for the detailed reviewer-run list. + +When a reviewer run has open findings, do not manually summarize them from memory. Use `ao review send -p {{projectId}}` to hand the stored findings back to the linked coding worker through AO. After sending, monitor the worker and request a new review once it reports the fixes are ready. + +### AO-Local Review Loop + +When the user asks you to review a worker, review a PR, or keep reviewing until clean, handle the loop internally: + +1. Inspect current state with `ao status` and identify the coding worker session. +2. Request and execute the reviewer run with `ao review run --execute`. +3. If the run is clean, report that the work is AO-review clean. +4. If the run has open findings, send the stored findings to the linked coding worker with `ao review send -p {{projectId}}`. +5. Monitor the coding worker with `ao status` and wait for it to push fixes or report `ready-for-review`. +6. Re-run `ao review run --execute` after the worker updates. +7. Continue until the review is clean, the worker is stuck, the user asks you to stop, or the configured review round limit is reached. + +Do not ask the user to manually run review commands for routine review/fix iterations. Treat review commands as orchestration internals, the same way worker spawning and `ao send` are orchestration internals. + ### Explicit Agent Reports Worker agents self-declare their workflow phase using `ao acknowledge` and `ao report ` (started, working, waiting, needs-input, fixing-ci, addressing-reviews, pr-created, draft-pr-created, ready-for-review, completed). These reports are persisted alongside the canonical lifecycle and may inform lifecycle inference, but do not replace runtime/activity/SCM-derived truth. diff --git a/packages/core/vitest.config.ts b/packages/core/vitest.config.ts index 9654a19dc..ec85fd4ef 100644 --- a/packages/core/vitest.config.ts +++ b/packages/core/vitest.config.ts @@ -6,6 +6,17 @@ import { defineConfig } from "vitest/config"; const __dirname = dirname(fileURLToPath(import.meta.url)); export default defineConfig({ + resolve: { + alias: [ + { find: /^@aoagents\/ao-core$/, replacement: resolve(__dirname, "src/index.ts") }, + { + find: /^@aoagents\/ao-core\/scm-webhook-utils$/, + replacement: resolve(__dirname, "src/scm-webhook-utils.ts"), + }, + { find: /^@aoagents\/ao-core\/types$/, replacement: resolve(__dirname, "src/types.ts") }, + { find: /^@aoagents\/ao-core\/utils$/, replacement: resolve(__dirname, "src/utils.ts") }, + ], + }, plugins: [ { name: "raw-markdown", diff --git a/packages/web/e2e/review-board-flows.md b/packages/web/e2e/review-board-flows.md new file mode 100644 index 000000000..a8b4cc706 --- /dev/null +++ b/packages/web/e2e/review-board-flows.md @@ -0,0 +1,110 @@ +# Review Board E2E Flows + +These flows cover the reviewer-agent UI as an orchestrator-owned surface, not as a second +command center. The project orchestrator is the entry point for creating and running reviews. +The review board observes, inspects, and navigates reviewer work. Reviewer runs remain linked to +a coding worker, but they must not reuse the worker's terminal context. + +## Flow 1: Enter through the project orchestrator + +1. Open the project coding dashboard at `/projects/:projectId`. +2. Open the header `Orchestrator` action. +3. Confirm the app lands on `/projects/:projectId/sessions/:orchestratorId`. +4. Confirm this is the project orchestrator session, not a worker or reviewer session. + +## Flow 2: Coding to Reviews navigation stays available + +1. Open the project coding dashboard at `/projects/:projectId`. +2. Confirm the shared header shows `Coding` as the active workspace mode. +3. Click `Reviews`. +4. Confirm the browser lands on `/review?project=:projectId`. +5. Confirm `Reviews` is now active and `Coding` links back to `/projects/:projectId`. + +## Flow 3: Orchestrator requests reviews + +1. Start with a worker card that is ready for review. +2. From the orchestrator flow, issue the AO review command for that worker. +3. Confirm a queued review run appears on the review board. +4. Confirm the review run is linked to the coding worker and displays worker metadata. +5. Confirm no reviewer coding session metadata is created. + +## Flow 4: Orchestrator executes multiple queued reviewer runs + +1. Create at least two queued review runs. +2. From the orchestrator flow, issue two reviewer execution commands without waiting between them. +3. Confirm both cards can be observed in a reviewing state. +4. Confirm completed runs move to either `Triage` when findings exist or `Clean` when no findings exist. + +## Flow 5: Inspect findings + +1. Let the orchestrator execute a run whose reviewer result contains a finding. +2. Confirm the run lands in `Triage`. +3. Click the `view` finding action or the card `details` action. +4. Confirm the details panel lists severity, title, location, body, open count, total count, and worker actions. +5. Close the drawer with the close button or Escape. + +## Flow 6: Worker and orchestrator links + +1. From a review card, click `Worker`. +2. Confirm the app navigates to the coding dashboard focused on the linked worker. +3. Return to the review board. +4. Confirm the header `Orchestrator` action opens or restores the same project orchestrator used by the coding dashboard. + +## Flow 7: Failure and retry + +1. Execute a queued run with a reviewer command that fails. +2. Confirm the card moves to `Failed`. +3. From the orchestrator flow, issue a retry command for the same run. +4. Confirm the retry can execute the same run again with `force: true`. + +## Flow 8: Feedback availability + +1. Show a review run linked to a worker with no live runtime. +2. Confirm the card shows the worker runtime state instead of a `Feedback` action. +3. Show a review run linked to a live worker. +4. Confirm `Feedback` sends open review findings to the linked worker. +5. Confirm the app opens the worker terminal section after the send succeeds. + +## Flow 9: CLI and UI share the same review store + +1. Request a review through the orchestrator AO command path. +2. Confirm the run appears in `/review?project=:projectId` without refreshing any mocked data. +3. Execute that run through the orchestrator AO command path and a deterministic local reviewer command. +4. Confirm the UI reflects the persisted result after a reload. + +## Flow 10: Clean review result + +1. Let the orchestrator execute a reviewer command that returns `{"findings":[]}`. +2. Confirm the run moves to `Clean`. +3. Confirm the card reports `0 findings`. +4. Open details and confirm it reports no captured findings. + +## Flow 11: Reviewer isolation from coding sessions + +1. Execute a reviewer run. +2. Confirm the reviewer has a snapshot workspace under `code-reviews/workspaces/:reviewerSessionId`. +3. Confirm there is no coding session metadata file for `:reviewerSessionId`. +4. Confirm the linked worker card and terminal route remain the coding worker, not the reviewer. + +## Flow 12: New worker commit supersedes old review runs + +1. Complete a review for the worker's current `HEAD`. +2. Commit a new change in the worker repository. +3. From the orchestrator flow, request a new review for the same worker. +4. Confirm older review runs for the previous `HEAD` move to `Outdated`. +5. Confirm the new review remains actionable in `Queued`. + +## Flow 13: Same orchestrator across modes + +1. Open the coding dashboard and capture the header `Orchestrator` link. +2. Open the review board and capture the header `Orchestrator` link. +3. Confirm both links point to the same project orchestrator session. +4. Confirm the review board does not offer `Spawn Orchestrator` when one already exists. + +## Flow 14: Send reviewer findings back to worker + +1. Complete a review run with open findings. +2. From the review board, click `Feedback` for that review run. +3. Confirm AO sends the stored finding details to the linked coding worker. +4. Confirm the review run moves to `Waiting`. +5. Confirm open findings become sent findings and are no longer counted as open. diff --git a/packages/web/e2e/review-board.e2e.ts b/packages/web/e2e/review-board.e2e.ts new file mode 100644 index 000000000..f4e966ece --- /dev/null +++ b/packages/web/e2e/review-board.e2e.ts @@ -0,0 +1,871 @@ +import assert from "node:assert/strict"; +import { execFileSync, spawn, type ChildProcess } from "node:child_process"; +import { existsSync, mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { request } from "node:http"; +import { createServer } from "node:net"; +import { tmpdir } from "node:os"; +import { basename, dirname, join, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; +import { chromium, type Locator, type Page } from "playwright"; +import { createCodeReviewStore } from "../../core/src/code-review-store.ts"; +import { + createInitialCanonicalLifecycle, + deriveLegacyStatus, +} from "../../core/src/lifecycle-state.ts"; +import { writeMetadata } from "../../core/src/metadata.ts"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const WEB_DIR = resolve(__dirname, ".."); +const REPO_ROOT = resolve(WEB_DIR, "../.."); +const PROJECT_ID = "todo-app"; +const SESSION_ID = "todo-1"; +const ORCHESTRATOR_ID = "todo-orchestrator"; +const PR_URL = "https://github.com/acme/todo-app/pull/1"; + +interface Fixture { + rootDir: string; + homeDir: string; + projectDir: string; + globalConfigPath: string; + localConfigPath: string; + tmuxSessionPrefix: string; + tmuxSessions: string[]; +} + +interface ServerHandle { + baseUrl: string; + stop: () => Promise; +} + +function shellJson(value: unknown): string { + return JSON.stringify(value).replaceAll("'", "'\"'\"'"); +} + +function buildStaticReviewCommand(findings: unknown[], delayMs = 0): string { + const payload = { findings }; + return `node -e 'setTimeout(() => console.log(${shellJson(JSON.stringify(payload))}), ${delayMs})'`; +} + +function buildFindingReviewCommand(delayMs: number): string { + return buildStaticReviewCommand( + [ + { + severity: "warning", + title: "E2E reviewer finding", + body: "The e2e review command created this finding.", + filePath: "README.md", + startLine: 1, + confidence: 0.9, + }, + ], + delayMs, + ); +} + +function git(cwd: string, args: string[]): void { + execFileSync("git", args, { + cwd, + stdio: "ignore", + env: { + ...process.env, + GIT_AUTHOR_NAME: "AO E2E", + GIT_AUTHOR_EMAIL: "ao-e2e@example.com", + GIT_COMMITTER_NAME: "AO E2E", + GIT_COMMITTER_EMAIL: "ao-e2e@example.com", + }, + }); +} + +function startCodexBackedTmux(sessionName: string, cwd: string): void { + execFileSync( + "tmux", + [ + "new-session", + "-d", + "-s", + sessionName, + "-c", + cwd, + "exec codex --no-alt-screen --sandbox danger-full-access --ask-for-approval never", + ], + { stdio: "ignore" }, + ); +} + +function killTmuxSession(sessionName: string): void { + try { + execFileSync("tmux", ["kill-session", "-t", sessionName], { stdio: "ignore" }); + } catch { + // Best effort cleanup for local e2e fixtures. + } +} + +function listTmuxSessions(): string[] { + try { + return execFileSync("tmux", ["list-sessions", "-F", "#{session_name}"], { + encoding: "utf-8", + stdio: ["ignore", "pipe", "ignore"], + }) + .split(/\r?\n/) + .map((sessionName) => sessionName.trim()) + .filter(Boolean); + } catch { + return []; + } +} + +function killReviewBoardTmuxSessions(fixture: Fixture): void { + const sessionNames = new Set([ + ...fixture.tmuxSessions, + ...listTmuxSessions().filter((sessionName) => + sessionName.startsWith(fixture.tmuxSessionPrefix), + ), + ]); + + for (const sessionName of sessionNames) { + killTmuxSession(sessionName); + } +} + +function installFixtureSignalCleanup(fixture: Fixture): void { + process.once("exit", () => killReviewBoardTmuxSessions(fixture)); + for (const signal of ["SIGINT", "SIGTERM"] as const) { + process.once(signal, () => { + killReviewBoardTmuxSessions(fixture); + process.exit(signal === "SIGINT" ? 130 : 143); + }); + } +} + +function captureTmuxPane(sessionName: string): string { + try { + return execFileSync("tmux", ["capture-pane", "-p", "-t", sessionName], { + encoding: "utf-8", + }); + } catch { + return ""; + } +} + +async function waitForTmuxText( + sessionName: string, + pattern: RegExp, + label: string, + timeoutMs = 45_000, +): Promise { + const deadline = Date.now() + timeoutMs; + let lastCapture = ""; + + while (Date.now() < deadline) { + lastCapture = captureTmuxPane(sessionName); + if (pattern.test(lastCapture)) return; + await new Promise((resolveWait) => setTimeout(resolveWait, 500)); + } + + throw new Error(`Expected tmux text: ${label}\n${lastCapture}`); +} + +async function getFreePort(): Promise { + return new Promise((resolvePromise, reject) => { + const server = createServer(); + server.unref(); + server.on("error", reject); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + if (!address || typeof address === "string") { + server.close(() => reject(new Error("Could not allocate a TCP port"))); + return; + } + const port = address.port; + server.close(() => resolvePromise(port)); + }); + }); +} + +function probe(url: URL): Promise { + return new Promise((resolveProbe) => { + const req = request( + { + hostname: url.hostname, + port: url.port, + path: url.pathname, + method: "HEAD", + timeout: 2_000, + }, + (res) => { + res.resume(); + resolveProbe(true); + }, + ); + req.on("error", () => resolveProbe(false)); + req.on("timeout", () => { + req.destroy(); + resolveProbe(false); + }); + req.end(); + }); +} + +async function waitForServer(baseUrl: string, timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs; + const url = new URL("/projects/todo-app", baseUrl); + while (Date.now() < deadline) { + if (await probe(url)) return; + await new Promise((resolveWait) => setTimeout(resolveWait, 500)); + } + throw new Error(`Next dev server did not respond at ${baseUrl} within ${timeoutMs}ms`); +} + +async function startWebServer(fixture: Fixture): Promise { + const port = await getFreePort(); + const baseUrl = `http://127.0.0.1:${port}`; + const output: string[] = []; + const child: ChildProcess = spawn("pnpm", ["dev:next"], { + cwd: WEB_DIR, + stdio: ["ignore", "pipe", "pipe"], + env: { + ...process.env, + HOME: fixture.homeDir, + AO_GLOBAL_CONFIG: fixture.globalConfigPath, + AO_CONFIG_PATH: fixture.localConfigPath, + AO_CODE_REVIEW_COMMAND: buildFindingReviewCommand(1_000), + PORT: String(port), + NEXT_TELEMETRY_DISABLED: "1", + AO_NO_UPDATE_NOTIFIER: "1", + AGENT_ORCHESTRATOR_CI: "1", + }, + }); + + const collect = (chunk: Buffer) => { + output.push(chunk.toString()); + if (output.length > 80) output.splice(0, output.length - 80); + }; + child.stdout?.on("data", collect); + child.stderr?.on("data", collect); + + try { + await waitForServer(baseUrl, 45_000); + } catch (error) { + child.kill("SIGTERM"); + throw new Error( + `${error instanceof Error ? error.message : String(error)}\n${output.join("").trim()}`, + { cause: error }, + ); + } + + return { + baseUrl, + stop: async () => { + if (child.exitCode !== null) return; + child.kill("SIGTERM"); + await new Promise((resolveStop) => { + const timer = setTimeout(() => { + child.kill("SIGKILL"); + resolveStop(); + }, 5_000); + child.once("exit", () => { + clearTimeout(timer); + resolveStop(); + }); + }); + }, + }; +} + +function createFixture(): Fixture { + const rootDir = mkdtempSync(join(tmpdir(), "ao-review-board-e2e-")); + const homeDir = join(rootDir, "home"); + const projectDir = join(rootDir, "todo-app"); + const globalConfigPath = join(homeDir, ".agent-orchestrator", "config.yaml"); + const localConfigPath = join(projectDir, "agent-orchestrator.yaml"); + const sessionsDir = join(homeDir, ".agent-orchestrator", "projects", PROJECT_ID, "sessions"); + const tmuxSuffix = basename(rootDir).replace(/[^a-zA-Z0-9_-]/g, "-"); + const tmuxSessionPrefix = `${tmuxSuffix}-`; + const workerTmuxName = `${tmuxSuffix}-worker`; + const orchestratorTmuxName = `${tmuxSuffix}-orchestrator`; + + mkdirSync(dirname(globalConfigPath), { recursive: true }); + mkdirSync(projectDir, { recursive: true }); + mkdirSync(sessionsDir, { recursive: true }); + + writeFileSync(join(projectDir, "README.md"), "# Todo App\n"); + writeFileSync(localConfigPath, "agent: codex\nruntime: tmux\nworkspace: worktree\n"); + git(projectDir, ["init", "-b", "main"]); + git(projectDir, ["add", "."]); + git(projectDir, ["commit", "-m", "initial"]); + startCodexBackedTmux(workerTmuxName, projectDir); + startCodexBackedTmux(orchestratorTmuxName, REPO_ROOT); + + writeFileSync( + globalConfigPath, + [ + "defaults:", + " runtime: tmux", + " agent: codex", + " workspace: worktree", + " notifiers: []", + "notifiers: {}", + "projects:", + ` ${PROJECT_ID}:`, + ` path: ${JSON.stringify(projectDir)}`, + " displayName: Todo App", + " defaultBranch: main", + " sessionPrefix: todo", + "", + ].join("\n"), + ); + + process.env["HOME"] = homeDir; + process.env["AO_GLOBAL_CONFIG"] = globalConfigPath; + process.env["AO_CONFIG_PATH"] = localConfigPath; + + const createdAt = new Date("2026-05-13T10:00:00.000Z"); + const workerLifecycle = createInitialCanonicalLifecycle("worker", createdAt); + workerLifecycle.session.state = "idle"; + workerLifecycle.session.reason = "awaiting_external_review"; + workerLifecycle.session.startedAt = createdAt.toISOString(); + workerLifecycle.session.lastTransitionAt = createdAt.toISOString(); + workerLifecycle.pr.state = "open"; + workerLifecycle.pr.reason = "review_pending"; + workerLifecycle.pr.number = 1; + workerLifecycle.pr.url = PR_URL; + workerLifecycle.pr.lastObservedAt = createdAt.toISOString(); + workerLifecycle.runtime.state = "alive"; + workerLifecycle.runtime.reason = "process_running"; + workerLifecycle.runtime.lastObservedAt = createdAt.toISOString(); + workerLifecycle.runtime.handle = { id: workerTmuxName, runtimeName: "tmux", data: {} }; + workerLifecycle.runtime.tmuxName = workerTmuxName; + + writeMetadata(sessionsDir, SESSION_ID, { + worktree: projectDir, + branch: "main", + status: deriveLegacyStatus(workerLifecycle), + lifecycle: workerLifecycle, + project: PROJECT_ID, + pr: PR_URL, + displayName: "E2E todo review target", + agent: "codex", + createdAt: createdAt.toISOString(), + tmuxName: workerTmuxName, + runtimeHandle: { id: workerTmuxName, runtimeName: "tmux", data: {} }, + }); + + const orchestratorLifecycle = createInitialCanonicalLifecycle("orchestrator", createdAt); + orchestratorLifecycle.session.state = "working"; + orchestratorLifecycle.session.reason = "task_in_progress"; + orchestratorLifecycle.session.startedAt = createdAt.toISOString(); + orchestratorLifecycle.session.lastTransitionAt = createdAt.toISOString(); + orchestratorLifecycle.runtime.state = "alive"; + orchestratorLifecycle.runtime.reason = "process_running"; + orchestratorLifecycle.runtime.lastObservedAt = createdAt.toISOString(); + orchestratorLifecycle.runtime.handle = { + id: orchestratorTmuxName, + runtimeName: "tmux", + data: {}, + }; + orchestratorLifecycle.runtime.tmuxName = orchestratorTmuxName; + + writeMetadata(sessionsDir, ORCHESTRATOR_ID, { + worktree: join(rootDir, "orchestrator-worktree"), + branch: "orchestrator/todo-orchestrator", + status: deriveLegacyStatus(orchestratorLifecycle), + lifecycle: orchestratorLifecycle, + role: "orchestrator", + project: PROJECT_ID, + displayName: "# Todo App Orchestrator", + agent: "codex", + createdAt: createdAt.toISOString(), + tmuxName: orchestratorTmuxName, + runtimeHandle: { id: orchestratorTmuxName, runtimeName: "tmux", data: {} }, + }); + + const store = createCodeReviewStore(PROJECT_ID); + store.deleteAll(); + store.createRun({ + linkedSessionId: SESSION_ID, + reviewerSessionId: "todo-rev-failed", + status: "failed", + prNumber: 1, + prUrl: PR_URL, + summary: "Seeded failed run for retry coverage.", + }); + + return { + rootDir, + homeDir, + projectDir, + globalConfigPath, + localConfigPath, + tmuxSessionPrefix, + tmuxSessions: [workerTmuxName, orchestratorTmuxName], + }; +} + +function reviewCard(page: Page, reviewerSessionId: string): Locator { + return page.locator(`[data-reviewer-session-id="${reviewerSessionId}"]`); +} + +function projectAoDir(fixture: Fixture): string { + return join(fixture.homeDir, ".agent-orchestrator", "projects", PROJECT_ID); +} + +function runAoCli(fixture: Fixture, args: string[]): string { + return execFileSync("pnpm", ["--dir", join(REPO_ROOT, "packages/cli"), "dev", ...args], { + cwd: REPO_ROOT, + encoding: "utf-8", + env: { + ...process.env, + HOME: fixture.homeDir, + AO_GLOBAL_CONFIG: fixture.globalConfigPath, + AO_CONFIG_PATH: fixture.localConfigPath, + AO_NO_UPDATE_NOTIFIER: "1", + AGENT_ORCHESTRATOR_CI: "1", + }, + }); +} + +function runAoCliAsync(fixture: Fixture, args: string[]): Promise { + return new Promise((resolveRun, rejectRun) => { + const child = spawn("pnpm", ["--dir", join(REPO_ROOT, "packages/cli"), "dev", ...args], { + cwd: REPO_ROOT, + stdio: ["ignore", "pipe", "pipe"], + env: { + ...process.env, + HOME: fixture.homeDir, + AO_GLOBAL_CONFIG: fixture.globalConfigPath, + AO_CONFIG_PATH: fixture.localConfigPath, + AO_NO_UPDATE_NOTIFIER: "1", + AGENT_ORCHESTRATOR_CI: "1", + }, + }); + let stdout = ""; + let stderr = ""; + child.stdout?.on("data", (chunk: Buffer) => { + stdout += chunk.toString(); + }); + child.stderr?.on("data", (chunk: Buffer) => { + stderr += chunk.toString(); + }); + child.once("error", rejectRun); + child.once("exit", (code) => { + if (code === 0) { + resolveRun(stdout); + return; + } + rejectRun(new Error(`ao ${args.join(" ")} failed with ${code}\n${stdout}\n${stderr}`)); + }); + }); +} + +function orchestratorReviewRun(fixture: Fixture, args: string[]): unknown { + return parseJsonCommandOutput(runAoCli(fixture, ["review", "run", SESSION_ID, ...args])); +} + +function orchestratorReviewExecute(fixture: Fixture, args: string[]): unknown { + return parseJsonCommandOutput(runAoCli(fixture, ["review", "execute", PROJECT_ID, ...args])); +} + +async function orchestratorReviewExecuteAsync( + fixture: Fixture, + args: string[], +): Promise { + return parseJsonCommandOutput( + await runAoCliAsync(fixture, ["review", "execute", PROJECT_ID, ...args]), + ); +} + +function parseJsonCommandOutput(output: string): unknown { + const lines = output.split(/\r?\n/); + for (let index = lines.length - 1; index >= 0; index--) { + const line = lines[index]?.trim(); + if (!line || (!line.startsWith("{") && !line.startsWith("["))) continue; + try { + return JSON.parse(lines.slice(index).join("\n")); + } catch { + // Keep scanning for the actual JSON payload. pnpm can echo JSON-looking command args. + } + } + + throw new Error(`Expected JSON command output, received: ${output}`); +} + +async function expectVisible(locator: Locator, label: string): Promise { + try { + await locator.waitFor({ state: "visible", timeout: 15_000 }); + } catch (error) { + throw new Error( + `Expected visible: ${label}\n${error instanceof Error ? error.message : error}`, + { cause: error }, + ); + } +} + +async function clickWorkspaceMode(page: Page, name: "Coding" | "Reviews"): Promise { + await page + .getByRole("navigation", { name: "Workspace mode" }) + .getByRole("link", { name, exact: true }) + .click(); +} + +async function step(name: string, run: () => Promise): Promise { + process.stdout.write(`\n- ${name}\n`); + await run(); +} + +async function main(): Promise { + const fixture = createFixture(); + installFixtureSignalCleanup(fixture); + const server = await startWebServer(fixture); + const browser = await chromium.launch({ headless: true }); + const page = await browser.newPage({ viewport: { width: 1440, height: 900 } }); + + try { + await step("enter the project through its orchestrator", async () => { + await page.goto(`${server.baseUrl}/projects/${PROJECT_ID}/sessions/${ORCHESTRATOR_ID}`, { + waitUntil: "networkidle", + }); + await expectVisible(page.getByText("# Todo App Orchestrator").first(), "orchestrator title"); + }); + + await step("navigate between coding and review modes from the shared header", async () => { + await page.goto(`${server.baseUrl}/projects/${PROJECT_ID}`, { + waitUntil: "networkidle", + }); + const nav = page.getByRole("navigation", { name: "Workspace mode" }); + await expectVisible(nav.getByRole("link", { name: "Coding", exact: true }), "Coding tab"); + assert.equal( + await nav.getByRole("link", { name: "Coding", exact: true }).getAttribute("aria-current"), + "page", + ); + + await clickWorkspaceMode(page, "Reviews"); + await page.waitForURL(`**/review?project=${PROJECT_ID}`); + const reviewNav = page.getByRole("navigation", { name: "Workspace mode" }); + assert.equal( + await reviewNav + .getByRole("link", { name: "Reviews", exact: true }) + .getAttribute("aria-current"), + "page", + ); + assert.equal( + await reviewNav.getByRole("link", { name: "Coding", exact: true }).getAttribute("href"), + `/projects/${PROJECT_ID}`, + ); + }); + + await step("confirm coding and review modes use the same project orchestrator", async () => { + await page.goto(`${server.baseUrl}/projects/${PROJECT_ID}`, { + waitUntil: "networkidle", + }); + const codingOrchestrator = page.getByRole("link", { name: "Orchestrator" }).first(); + await expectVisible(codingOrchestrator, "coding orchestrator link"); + const codingHref = await codingOrchestrator.getAttribute("href"); + assert.equal(codingHref, `/projects/${PROJECT_ID}/sessions/${ORCHESTRATOR_ID}`); + + await clickWorkspaceMode(page, "Reviews"); + await page.waitForURL(`**/review?project=${PROJECT_ID}`); + const reviewOrchestrator = page.getByRole("link", { name: "Open project orchestrator" }); + await expectVisible(reviewOrchestrator, "review orchestrator link"); + assert.equal(await reviewOrchestrator.getAttribute("href"), codingHref); + assert.equal( + await page.getByRole("button", { name: "Spawn Orchestrator" }).count(), + 0, + "review board should reuse the existing orchestrator instead of spawning another", + ); + }); + + await step("orchestrator requests the first review", async () => { + const requested = orchestratorReviewRun(fixture, [ + "--summary", + "Orchestrator requested initial E2E review", + "--json", + ]) as { run: { reviewerSessionId: string; status: string } }; + assert.equal(requested.run.status, "queued"); + assert.equal(requested.run.reviewerSessionId, "todo-rev-1"); + + await page.goto(`${server.baseUrl}/review?project=${PROJECT_ID}`, { + waitUntil: "networkidle", + }); + await page.waitForURL(`**/review?project=${PROJECT_ID}`); + await expectVisible(reviewCard(page, "todo-rev-1"), "todo-rev-1 queued review card"); + await expectVisible(reviewCard(page, "todo-rev-failed"), "seeded failed retry card"); + }); + + await step("orchestrator requests another review", async () => { + const requested = orchestratorReviewRun(fixture, [ + "--summary", + "Orchestrator requested parallel E2E review", + "--json", + ]) as { run: { reviewerSessionId: string; status: string } }; + assert.equal(requested.run.status, "queued"); + assert.equal(requested.run.reviewerSessionId, "todo-rev-2"); + + await page.goto(`${server.baseUrl}/review?project=${PROJECT_ID}`, { + waitUntil: "networkidle", + }); + await expectVisible(reviewCard(page, "todo-rev-2"), "todo-rev-2 queued review card"); + }); + + await step("orchestrator runs two queued reviewer runs concurrently", async () => { + const startedAt = Date.now(); + const firstExecution = orchestratorReviewExecuteAsync(fixture, [ + "--run", + "todo-rev-1", + "--command", + buildFindingReviewCommand(8_000), + "--json", + ]); + const secondExecution = orchestratorReviewExecuteAsync(fixture, [ + "--run", + "todo-rev-2", + "--command", + buildFindingReviewCommand(8_000), + "--json", + ]); + + const [firstResult, secondResult] = (await Promise.all([ + firstExecution, + secondExecution, + ])) as [ + { run: { reviewerSessionId: string; status: string } }, + { run: { reviewerSessionId: string; status: string } }, + ]; + assert.equal(firstResult.run.status, "needs_triage"); + assert.equal(secondResult.run.status, "needs_triage"); + assert.ok( + Date.now() - startedAt < 14_000, + "reviewer executions should run concurrently, not serially", + ); + + await page.goto(`${server.baseUrl}/review?project=${PROJECT_ID}`, { + waitUntil: "networkidle", + }); + await expectVisible( + page.locator('[data-reviewer-session-id="todo-rev-1"][data-review-status="needs_triage"]'), + "first card in triage", + ); + await expectVisible( + page.locator('[data-reviewer-session-id="todo-rev-2"][data-review-status="needs_triage"]'), + "second card in triage", + ); + }); + + await step("confirm reviewer workspaces stay isolated from coding sessions", async () => { + const aoProjectDir = projectAoDir(fixture); + assert.equal( + existsSync(join(aoProjectDir, "code-reviews", "workspaces", "todo-rev-1")), + true, + "executed reviewer run should have a snapshot workspace", + ); + assert.equal( + existsSync(join(aoProjectDir, "sessions", "todo-rev-1.json")), + false, + "reviewer run must not create coding session metadata", + ); + await expectVisible( + reviewCard(page, "todo-rev-1").getByRole("link", { name: "Worker" }), + "worker link still points at coding worker", + ); + }); + + await step("open findings details from a triage review card", async () => { + const first = reviewCard(page, "todo-rev-1"); + await first.getByRole("button", { name: "view" }).click(); + const dialog = page.getByRole("dialog", { name: /E2E todo review target/i }); + await expectVisible(dialog, "review details dialog"); + await expectVisible(dialog.getByText("E2E reviewer finding"), "finding title"); + await expectVisible(dialog.getByText("README.md:1"), "finding location"); + await expectVisible( + dialog.getByText("The e2e review command created this finding."), + "finding body", + ); + await expectVisible(dialog.getByRole("link", { name: "Open terminal" }), "terminal link"); + await dialog.getByLabel("Close review details").click(); + }); + + await step("review board sends review findings back to the linked worker", async () => { + await reviewCard(page, "todo-rev-1").getByRole("button", { name: "Feedback" }).click(); + await page.waitForURL( + `**/projects/${PROJECT_ID}/sessions/${SESSION_ID}#session-terminal-section`, + ); + + await waitForTmuxText( + fixture.tmuxSessions[0] ?? "", + /E2E reviewer finding/, + "worker receives AO-local review finding", + ); + + const store = createCodeReviewStore(PROJECT_ID); + const sentRun = store + .listRunSummaries() + .find((run) => run.reviewerSessionId === "todo-rev-1"); + assert.ok(sentRun, "sent review run should still exist"); + assert.equal(sentRun.status, "waiting_update"); + assert.equal(sentRun.openFindingCount, 0); + assert.equal(sentRun.sentFindingCount, 1); + + const sentFindings = store.listFindings({ runId: sentRun.id }); + assert.equal(sentFindings.length, 1); + assert.equal(sentFindings[0]?.status, "sent_to_agent"); + assert.ok(sentFindings[0]?.sentToAgentAt, "sent finding should record handoff time"); + + await page.goto(`${server.baseUrl}/review?project=${PROJECT_ID}`, { + waitUntil: "networkidle", + }); + const sentCard = reviewCard(page, "todo-rev-1"); + await expectVisible( + page.locator('[data-reviewer-session-id="todo-rev-1"][data-review-status="waiting_update"]'), + "sent review moved to waiting", + ); + await expectVisible( + sentCard.getByText(/waiting update · 1 finding · 1 sent/i), + "sent truth line", + ); + assert.equal( + await sentCard.getByRole("button", { name: /open finding/i }).count(), + 0, + "sent findings should not still be counted as open", + ); + }); + + await step("jump from review card back to the linked coding worker", async () => { + await reviewCard(page, "todo-rev-1").getByRole("link", { name: "Worker" }).click(); + await page.waitForURL(`**/projects/${PROJECT_ID}?session=${SESSION_ID}`); + assert.equal(new URL(page.url()).searchParams.get("session"), SESSION_ID); + }); + + await step("orchestrator marks older review runs outdated after a new worker commit", async () => { + writeFileSync(join(fixture.projectDir, "todo.txt"), "new worker commit\n"); + git(fixture.projectDir, ["add", "todo.txt"]); + git(fixture.projectDir, ["commit", "-m", "worker update"]); + + const requested = orchestratorReviewRun(fixture, [ + "--summary", + "Orchestrator requested review after worker update", + "--json", + ]) as { run: { reviewerSessionId: string; status: string } }; + assert.equal(requested.run.status, "queued"); + assert.equal(requested.run.reviewerSessionId, "todo-rev-3"); + + await page.goto(`${server.baseUrl}/review?project=${PROJECT_ID}`, { + waitUntil: "networkidle", + }); + await expectVisible(reviewCard(page, "todo-rev-3"), "todo-rev-3 queued review card"); + await expectVisible( + page.locator('[data-reviewer-session-id="todo-rev-1"][data-review-status="outdated"]'), + "older triage review marked outdated", + ); + await expectVisible( + page.locator('[data-reviewer-session-id="todo-rev-2"][data-review-status="outdated"]'), + "second older triage review marked outdated", + ); + }); + + await step("orchestrator runs a clean review and the UI observes it", async () => { + const requested = orchestratorReviewRun(fixture, [ + "--summary", + "Orchestrator requested clean review", + "--json", + ]) as { run: { id: string; reviewerSessionId: string; status: string } }; + assert.equal(requested.run.status, "queued"); + + const executed = orchestratorReviewExecute(fixture, [ + "--run", + requested.run.reviewerSessionId, + "--command", + buildStaticReviewCommand([]), + "--json", + ]) as { run: { reviewerSessionId: string; status: string; findingCount: number } }; + assert.equal(executed.run.status, "clean"); + assert.equal(executed.run.findingCount, 0); + + await page.goto(`${server.baseUrl}/review?project=${PROJECT_ID}`, { + waitUntil: "networkidle", + }); + const cleanCard = reviewCard(page, requested.run.reviewerSessionId); + await expectVisible( + cleanCard.locator('[data-review-status="clean"]').or(cleanCard), + "CLI clean review card", + ); + await expectVisible( + page.locator( + `[data-reviewer-session-id="${requested.run.reviewerSessionId}"][data-review-status="clean"]`, + ), + "CLI clean review in clean column", + ); + await expectVisible(cleanCard.getByText(/clean · 0 findings/i), "clean truth line"); + await cleanCard.getByRole("button", { name: "details" }).click(); + const dialog = page.getByRole("dialog", { name: /E2E todo review target/i }); + await expectVisible(dialog.getByText("No findings captured for this run."), "clean details"); + await dialog.getByLabel("Close review details").click(); + }); + + await step("orchestrator retries a failed reviewer run", async () => { + const failed = reviewCard(page, "todo-rev-failed"); + await page.goto(`${server.baseUrl}/review?project=${PROJECT_ID}`, { + waitUntil: "networkidle", + }); + await expectVisible(failed, "failed review card"); + const retryPayload = orchestratorReviewExecute(fixture, [ + "--run", + "todo-rev-failed", + "--force", + "--command", + buildFindingReviewCommand(0), + "--json", + ]) as { run: { status?: string; terminationReason?: string } }; + assert.notEqual( + retryPayload.run.status, + "failed", + retryPayload.run.terminationReason ?? "retry should not fail", + ); + await page.goto(`${server.baseUrl}/review?project=${PROJECT_ID}`, { + waitUntil: "networkidle", + }); + await expectVisible( + page.locator( + '[data-reviewer-session-id="todo-rev-failed"][data-review-status="needs_triage"]', + ), + "failed card moved to triage after retry", + ); + }); + + await step("confirm review cards expose worker and orchestrator affordances", async () => { + await page.goto(`${server.baseUrl}/review?project=${PROJECT_ID}`, { + waitUntil: "domcontentloaded", + }); + const retried = reviewCard(page, "todo-rev-failed"); + await expectVisible(retried.getByRole("button", { name: "Feedback" }), "feedback button"); + const orchestrator = page.getByRole("link", { name: "Open project orchestrator" }); + await expectVisible(orchestrator, "project orchestrator link"); + assert.equal( + await orchestrator.getAttribute("href"), + `/projects/${PROJECT_ID}/sessions/${ORCHESTRATOR_ID}`, + ); + }); + + process.stdout.write("\nReview board e2e flows passed.\n"); + } finally { + try { + await browser.close(); + } catch { + // Best effort cleanup; tmux sessions and fixture files still need cleanup. + } + try { + await server.stop(); + } catch { + // Best effort cleanup; tmux sessions and fixture files still need cleanup. + } + killReviewBoardTmuxSessions(fixture); + if (process.env["AO_E2E_KEEP_ARTIFACTS"] !== "1") { + rmSync(fixture.rootDir, { recursive: true, force: true }); + } else { + process.stdout.write(`\nKept e2e fixture at ${fixture.rootDir}\n`); + } + } +} + +main().catch((error: unknown) => { + console.error(error); + process.exit(1); +}); diff --git a/packages/web/package.json b/packages/web/package.json index 83a59ace5..191e07ee9 100644 --- a/packages/web/package.json +++ b/packages/web/package.json @@ -36,6 +36,7 @@ "dev:optimized": "rimraf .next dist-server && next build && tsc -p tsconfig.server.json && node dist-server/start-all.js", "typecheck": "tsc --noEmit", "test": "vitest run", + "test:e2e:review": "tsx e2e/review-board.e2e.ts", "test:watch": "vitest", "clean": "node scripts/guard-production-artifact-clean.mjs && rimraf .next dist-server", "screenshot": "tsx e2e/screenshot.ts", diff --git a/packages/web/src/__tests__/api-routes.test.ts b/packages/web/src/__tests__/api-routes.test.ts index 0a32bf0dc..3bb2eb496 100644 --- a/packages/web/src/__tests__/api-routes.test.ts +++ b/packages/web/src/__tests__/api-routes.test.ts @@ -674,29 +674,18 @@ describe("API Routes", () => { const enrichSpy = vi .spyOn(serialize, "enrichSessionPR") - .mockResolvedValueOnce(true) - .mockResolvedValueOnce(false) - .mockResolvedValueOnce(true); + .mockReturnValueOnce(true) + .mockReturnValueOnce(false); const res = await sessionsGET(makeRequest("http://localhost:3000/api/sessions")); expect(res.status).toBe(200); - expect(enrichSpy).toHaveBeenCalledTimes(3); + expect(enrichSpy).toHaveBeenCalledTimes(2); expect(enrichSpy.mock.calls[0]).toEqual([ expect.objectContaining({ id: "worker-live" }), - expect.anything(), - sessionsWithPRs[0]!.pr, ]); expect(enrichSpy.mock.calls[1]).toEqual([ expect.objectContaining({ id: "worker-killed" }), - expect.anything(), - sessionsWithPRs[1]!.pr, - { cacheOnly: true }, - ]); - expect(enrichSpy.mock.calls[2]).toEqual([ - expect.objectContaining({ id: "worker-killed" }), - expect.anything(), - sessionsWithPRs[1]!.pr, ]); metadataSpy.mockRestore(); @@ -751,8 +740,6 @@ describe("API Routes", () => { expect(enrichSpy).toHaveBeenCalledTimes(1); expect(enrichSpy.mock.calls[0]).toEqual([ expect.objectContaining({ id: "worker-open-pr" }), - expect.anything(), - sessionWithOpenPR[0]!.pr, ]); metadataSpy.mockRestore(); diff --git a/packages/web/src/__tests__/project-detail-route.test.ts b/packages/web/src/__tests__/project-detail-route.test.ts index 297917714..3f9fd67c0 100644 --- a/packages/web/src/__tests__/project-detail-route.test.ts +++ b/packages/web/src/__tests__/project-detail-route.test.ts @@ -458,6 +458,37 @@ describe("/api/projects/[id]", () => { expect(readFileSync(path.join(repoDir, "agent-orchestrator.yaml"), "utf-8")).toContain("agent: codex"); }); + it("POST repair preserves wrapped defaults so the project can start with its intended agent", async () => { + const repoDir = path.join(tempRoot, "broken-defaults"); + mkdirSync(repoDir, { recursive: true }); + writeFileSync( + path.join(repoDir, "agent-orchestrator.yaml"), + [ + "defaults:", + " agent: codex", + " runtime: tmux", + " workspace: worktree", + "projects:", + " broken-defaults:", + ` path: ${repoDir}`, + " name: Broken Defaults", + "", + ].join("\n"), + ); + const effectiveId = registerProjectInGlobalConfig("broken-defaults", "Broken Defaults", repoDir); + + const { POST } = await import("@/app/api/projects/[id]/route"); + const response = await POST(makeRequest("POST", undefined, effectiveId), { + params: Promise.resolve({ id: effectiveId }), + }); + + expect(response.status).toBe(200); + const localYaml = readFileSync(path.join(repoDir, "agent-orchestrator.yaml"), "utf-8"); + expect(localYaml).toContain("agent: codex"); + expect(localYaml).toContain("runtime: tmux"); + expect(localYaml).toContain("workspace: worktree"); + }); + it("POST repairs wrapped local .yml configs in place", async () => { const repoDir = path.join(tempRoot, "broken-yml"); mkdirSync(repoDir, { recursive: true }); diff --git a/packages/web/src/__tests__/review-api.test.ts b/packages/web/src/__tests__/review-api.test.ts new file mode 100644 index 000000000..1852ee1e2 --- /dev/null +++ b/packages/web/src/__tests__/review-api.test.ts @@ -0,0 +1,328 @@ +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { NextRequest } from "next/server"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + createActivitySignal, + createInitialCanonicalLifecycle, + createCodeReviewStore, + type OrchestratorConfig, + type Session, + type SessionManager, +} from "@aoagents/ao-core"; + +const { mockConfig, mockSessionManager } = vi.hoisted(() => ({ + mockConfig: { + configPath: "/tmp/ao/agent-orchestrator.yaml", + readyThresholdMs: 300_000, + defaults: { runtime: "tmux", agent: "codex", workspace: "worktree", notifiers: [] }, + projects: { + app: { + name: "App", + path: "/tmp/app", + defaultBranch: "main", + sessionPrefix: "app", + }, + }, + notifiers: {}, + notificationRouting: { urgent: [], action: [], warning: [], info: [] }, + reactions: {}, + } satisfies OrchestratorConfig, + mockSessionManager: { + get: vi.fn(), + list: vi.fn(), + spawn: vi.fn(), + spawnOrchestrator: vi.fn(), + ensureOrchestrator: vi.fn(), + relaunchOrchestrator: vi.fn(), + restore: vi.fn(), + kill: vi.fn(), + cleanup: vi.fn(), + send: vi.fn(), + claimPR: vi.fn(), + } satisfies SessionManager, +})); + +vi.mock("@/lib/services", () => ({ + getServices: vi.fn(async () => ({ + config: mockConfig, + sessionManager: mockSessionManager, + })), +})); + +function makeRequest(url: string, init?: RequestInit): NextRequest { + return new NextRequest( + new URL(url, "http://localhost:3000"), + init as ConstructorParameters[1], + ); +} + +function makeSession(overrides: Partial = {}): Session { + const lifecycle = createInitialCanonicalLifecycle("worker", new Date("2026-05-10T10:00:00.000Z")); + lifecycle.session.state = "idle"; + lifecycle.session.reason = "awaiting_external_review"; + lifecycle.pr.state = "open"; + lifecycle.pr.reason = "review_pending"; + lifecycle.pr.number = 7; + lifecycle.pr.url = "https://github.com/acme/app/pull/7"; + lifecycle.runtime.state = "alive"; + lifecycle.runtime.reason = "process_running"; + + return { + id: "app-1", + projectId: "app", + status: "review_pending", + activity: "idle", + activitySignal: createActivitySignal("valid", { + activity: "idle", + timestamp: new Date("2026-05-10T10:00:00.000Z"), + source: "native", + }), + lifecycle, + branch: "feat/todos", + issueId: null, + pr: { + number: 7, + url: "https://github.com/acme/app/pull/7", + title: "feat: todos", + owner: "acme", + repo: "app", + branch: "feat/todos", + baseBranch: "main", + isDraft: false, + }, + workspacePath: null, + runtimeHandle: { id: "tmux-app-1", runtimeName: "tmux", data: {} }, + agentInfo: null, + createdAt: new Date("2026-05-10T09:00:00.000Z"), + lastActivityAt: new Date("2026-05-10T10:00:00.000Z"), + metadata: {}, + ...overrides, + }; +} + +import { POST } from "@/app/api/reviews/route"; +import { POST as POST_EXECUTE } from "@/app/api/reviews/execute/route"; +import { GET as GET_FINDINGS } from "@/app/api/reviews/findings/route"; +import { POST as POST_SEND } from "@/app/api/reviews/send/route"; + +let tmpHome: string; +let originalHome: string | undefined; + +beforeEach(() => { + tmpHome = mkdtempSync(join(tmpdir(), "ao-web-review-api-")); + originalHome = process.env["HOME"]; + process.env["HOME"] = tmpHome; + createCodeReviewStore("app").deleteAll(); + mockSessionManager.get.mockReset(); + mockSessionManager.get.mockResolvedValue(makeSession()); +}); + +afterEach(() => { + if (originalHome === undefined) { + delete process.env["HOME"]; + } else { + process.env["HOME"] = originalHome; + } + rmSync(tmpHome, { recursive: true, force: true }); + vi.clearAllMocks(); +}); + +describe("POST /api/reviews", () => { + it("requests a review run for a worker session", async () => { + const response = await POST( + makeRequest("/api/reviews", { + method: "POST", + body: JSON.stringify({ sessionId: "app-1" }), + }), + ); + + expect(response.status).toBe(201); + const payload = (await response.json()) as { + run: { linkedSessionId: string; reviewerSessionId: string; status: string }; + }; + expect(payload.run).toMatchObject({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "queued", + }); + expect(createCodeReviewStore("app").listRuns()).toHaveLength(1); + }); + + it("returns 400 when a session belongs to an unknown project", async () => { + mockSessionManager.get.mockResolvedValueOnce(makeSession({ projectId: "missing-project" })); + + const response = await POST( + makeRequest("/api/reviews", { + method: "POST", + body: JSON.stringify({ sessionId: "app-1" }), + }), + ); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toMatchObject({ + error: "Unknown project for session app-1: missing-project", + }); + }); + + it("returns 400 when a review is requested for an orchestrator session", async () => { + mockSessionManager.get.mockResolvedValueOnce( + makeSession({ id: "app-orchestrator", metadata: { role: "orchestrator" } }), + ); + + const response = await POST( + makeRequest("/api/reviews", { + method: "POST", + body: JSON.stringify({ sessionId: "app-orchestrator" }), + }), + ); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toMatchObject({ + error: "Cannot request code review for orchestrator session: app-orchestrator", + }); + }); +}); + +describe("GET /api/reviews/findings", () => { + it("returns stored findings for a review run", async () => { + const store = createCodeReviewStore("app"); + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + }); + store.createFinding({ + runId: run.id, + linkedSessionId: "app-1", + severity: "warning", + title: "Missing empty state", + body: "The todo list should render a clear empty state.", + filePath: "src/App.tsx", + startLine: 12, + }); + + const response = await GET_FINDINGS( + makeRequest(`/api/reviews/findings?projectId=app&runId=${run.id}`), + ); + + expect(response.status).toBe(200); + const payload = (await response.json()) as { + run: { id: string }; + findings: Array<{ title: string; filePath: string }>; + }; + expect(payload.run.id).toBe(run.id); + expect(payload.findings).toEqual([ + expect.objectContaining({ + title: "Missing empty state", + filePath: "src/App.tsx", + }), + ]); + }); +}); + +describe("POST /api/reviews/send", () => { + it("sends open findings to the linked worker session", async () => { + const store = createCodeReviewStore("app"); + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "needs_triage", + prNumber: 7, + }); + const finding = store.createFinding({ + runId: run.id, + linkedSessionId: "app-1", + severity: "warning", + title: "Missing empty state", + body: "The todo list should render a clear empty state.", + filePath: "src/App.tsx", + startLine: 12, + }); + + const response = await POST_SEND( + makeRequest("/api/reviews/send", { + method: "POST", + body: JSON.stringify({ projectId: "app", runId: run.id }), + }), + ); + + expect(response.status).toBe(200); + const payload = (await response.json()) as { + run: { status: string; sentFindingCount: number; openFindingCount: number }; + sentFindingCount: number; + message: string; + }; + expect(payload.sentFindingCount).toBe(1); + expect(payload.run).toMatchObject({ + status: "waiting_update", + sentFindingCount: 1, + openFindingCount: 0, + }); + expect(payload.message).toContain("Missing empty state"); + expect(mockSessionManager.send).toHaveBeenCalledWith( + "app-1", + expect.stringContaining("Missing empty state"), + ); + expect(store.getFinding(finding.id)).toMatchObject({ status: "sent_to_agent" }); + }); + + it("returns 409 when there are no open findings to send", async () => { + const store = createCodeReviewStore("app"); + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "clean", + }); + + const response = await POST_SEND( + makeRequest("/api/reviews/send", { + method: "POST", + body: JSON.stringify({ projectId: "app", runId: run.id }), + }), + ); + + expect(response.status).toBe(409); + await expect(response.json()).resolves.toMatchObject({ + error: "No open review findings to send for app-rev-1.", + }); + expect(mockSessionManager.send).not.toHaveBeenCalled(); + }); +}); + +describe("POST /api/reviews/execute", () => { + it("returns 404 when the review run does not exist", async () => { + const response = await POST_EXECUTE( + makeRequest("/api/reviews/execute", { + method: "POST", + body: JSON.stringify({ projectId: "app", runId: "review-run-missing" }), + }), + ); + + expect(response.status).toBe(404); + await expect(response.json()).resolves.toMatchObject({ + error: "Code review run not found: review-run-missing", + }); + }); + + it("returns 409 when the review run is not executable", async () => { + const store = createCodeReviewStore("app"); + const run = store.createRun({ + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "running", + }); + + const response = await POST_EXECUTE( + makeRequest("/api/reviews/execute", { + method: "POST", + body: JSON.stringify({ projectId: "app", runId: run.id }), + }), + ); + + expect(response.status).toBe(409); + await expect(response.json()).resolves.toMatchObject({ + error: "Code review run app-rev-1 is running, not queued", + }); + }); +}); diff --git a/packages/web/src/app/api/reviews/execute/route.ts b/packages/web/src/app/api/reviews/execute/route.ts new file mode 100644 index 000000000..df9c7cfd0 --- /dev/null +++ b/packages/web/src/app/api/reviews/execute/route.ts @@ -0,0 +1,63 @@ +import { + CodeReviewRunNotExecutableError, + CodeReviewRunNotFoundError, + createShellCodeReviewRunner, + executeCodeReviewRun, + SessionNotFoundError, +} from "@aoagents/ao-core"; +import { getCorrelationId, jsonWithCorrelation } from "@/lib/observability"; +import { getServices } from "@/lib/services"; +import { validateConfiguredProject, validateIdentifier } from "@/lib/validation"; + +export async function POST(request: Request) { + const correlationId = getCorrelationId(request); + const body = (await request.json().catch(() => null)) as Record | null; + if (!body) { + return jsonWithCorrelation({ error: "Invalid JSON body" }, { status: 400 }, correlationId); + } + + const projectIdErr = validateIdentifier(body.projectId, "projectId"); + if (projectIdErr) { + return jsonWithCorrelation({ error: projectIdErr }, { status: 400 }, correlationId); + } + + const runIdErr = validateIdentifier(body.runId, "runId"); + if (runIdErr) { + return jsonWithCorrelation({ error: runIdErr }, { status: 400 }, correlationId); + } + + try { + const { config, sessionManager } = await getServices(); + const projectId = String(body.projectId); + const configuredProjectErr = validateConfiguredProject(config.projects, projectId); + if (configuredProjectErr) { + return jsonWithCorrelation({ error: configuredProjectErr }, { status: 404 }, correlationId); + } + + const command = process.env["AO_CODE_REVIEW_COMMAND"]; + const run = await executeCodeReviewRun( + { + config, + sessionManager, + force: body.force === true, + ...(command ? { runReviewer: createShellCodeReviewRunner(command) } : {}), + }, + { projectId, runId: String(body.runId) }, + ); + + return jsonWithCorrelation({ run }, { status: 200 }, correlationId); + } catch (error) { + if (error instanceof SessionNotFoundError) { + return jsonWithCorrelation({ error: error.message }, { status: 404 }, correlationId); + } + if (error instanceof CodeReviewRunNotFoundError) { + return jsonWithCorrelation({ error: error.message }, { status: 404 }, correlationId); + } + if (error instanceof CodeReviewRunNotExecutableError) { + return jsonWithCorrelation({ error: error.message }, { status: 409 }, correlationId); + } + + const message = error instanceof Error ? error.message : "Failed to execute review"; + return jsonWithCorrelation({ error: message }, { status: 500 }, correlationId); + } +} diff --git a/packages/web/src/app/api/reviews/findings/route.ts b/packages/web/src/app/api/reviews/findings/route.ts new file mode 100644 index 000000000..0d0e1050f --- /dev/null +++ b/packages/web/src/app/api/reviews/findings/route.ts @@ -0,0 +1,53 @@ +import { createCodeReviewStore } from "@aoagents/ao-core"; +import { getCorrelationId, jsonWithCorrelation } from "@/lib/observability"; +import { getServices } from "@/lib/services"; +import { validateConfiguredProject, validateIdentifier } from "@/lib/validation"; + +export async function GET(request: Request) { + const correlationId = getCorrelationId(request); + const { searchParams } = new URL(request.url); + const projectId = searchParams.get("projectId"); + const runId = searchParams.get("runId"); + + const projectIdErr = validateIdentifier(projectId, "projectId"); + if (projectIdErr) { + return jsonWithCorrelation({ error: projectIdErr }, { status: 400 }, correlationId); + } + + const runIdErr = validateIdentifier(runId, "runId"); + if (runIdErr) { + return jsonWithCorrelation({ error: runIdErr }, { status: 400 }, correlationId); + } + + try { + const { config } = await getServices(); + const safeProjectId = String(projectId); + const safeRunId = String(runId); + const configuredProjectErr = validateConfiguredProject(config.projects, safeProjectId); + if (configuredProjectErr) { + return jsonWithCorrelation({ error: configuredProjectErr }, { status: 404 }, correlationId); + } + + const store = createCodeReviewStore(safeProjectId); + const run = store.getRun(safeRunId); + if (!run) { + return jsonWithCorrelation( + { error: `Review run not found: ${safeRunId}` }, + { status: 404 }, + correlationId, + ); + } + + return jsonWithCorrelation( + { + run, + findings: store.listFindings({ runId: safeRunId }), + }, + { status: 200 }, + correlationId, + ); + } catch (error) { + const message = error instanceof Error ? error.message : "Failed to load review findings"; + return jsonWithCorrelation({ error: message }, { status: 500 }, correlationId); + } +} diff --git a/packages/web/src/app/api/reviews/route.ts b/packages/web/src/app/api/reviews/route.ts new file mode 100644 index 000000000..e5592be41 --- /dev/null +++ b/packages/web/src/app/api/reviews/route.ts @@ -0,0 +1,87 @@ +import { + CodeReviewInvalidSessionError, + SessionNotFoundError, + triggerCodeReviewForSession, +} from "@aoagents/ao-core"; +import { getReviewPageData, resolveReviewProjectFilter } from "@/lib/review-page-data"; +import { getCorrelationId, jsonWithCorrelation } from "@/lib/observability"; +import { getServices } from "@/lib/services"; +import { stripControlChars, validateIdentifier, validateString } from "@/lib/validation"; + +const MAX_REVIEW_SUMMARY_LENGTH = 2_000; + +export async function GET(request: Request) { + const correlationId = getCorrelationId(request); + const { searchParams } = new URL(request.url); + const projectFilter = resolveReviewProjectFilter(searchParams.get("project") ?? undefined); + const pageData = await getReviewPageData(projectFilter); + + if (pageData.dashboardLoadError) { + return jsonWithCorrelation( + { + error: pageData.dashboardLoadError, + runs: pageData.runs, + }, + { status: 500 }, + correlationId, + ); + } + + return jsonWithCorrelation( + { + runs: pageData.runs, + workerOptions: pageData.workerOptions, + orchestrators: pageData.orchestrators, + projectName: pageData.projectName, + selectedProjectId: pageData.selectedProjectId ?? null, + }, + { status: 200 }, + correlationId, + ); +} + +export async function POST(request: Request) { + const correlationId = getCorrelationId(request); + const body = (await request.json().catch(() => null)) as Record | null; + if (!body) { + return jsonWithCorrelation({ error: "Invalid JSON body" }, { status: 400 }, correlationId); + } + + const sessionIdErr = validateIdentifier(body.sessionId, "sessionId"); + if (sessionIdErr) { + return jsonWithCorrelation({ error: sessionIdErr }, { status: 400 }, correlationId); + } + + let summary: string | undefined; + if (body.summary !== undefined) { + const summaryErr = validateString(body.summary, "summary", MAX_REVIEW_SUMMARY_LENGTH); + if (summaryErr) { + return jsonWithCorrelation({ error: summaryErr }, { status: 400 }, correlationId); + } + summary = stripControlChars(String(body.summary)); + } + + try { + const { config, sessionManager } = await getServices(); + const run = await triggerCodeReviewForSession( + { config, sessionManager }, + { + sessionId: String(body.sessionId), + requestedBy: "web", + summary, + }, + ); + + return jsonWithCorrelation({ run }, { status: 201 }, correlationId); + } catch (error) { + if (error instanceof SessionNotFoundError) { + return jsonWithCorrelation({ error: error.message }, { status: 404 }, correlationId); + } + if (error instanceof CodeReviewInvalidSessionError) { + return jsonWithCorrelation({ error: error.message }, { status: 400 }, correlationId); + } + + const message = error instanceof Error ? error.message : "Failed to request review"; + return jsonWithCorrelation({ error: message }, { status: 500 }, correlationId); + } +} diff --git a/packages/web/src/app/api/reviews/send/route.ts b/packages/web/src/app/api/reviews/send/route.ts new file mode 100644 index 000000000..5d87f1cce --- /dev/null +++ b/packages/web/src/app/api/reviews/send/route.ts @@ -0,0 +1,56 @@ +import { + CodeReviewNoOpenFindingsError, + CodeReviewRunNotFoundError, + sendCodeReviewFindingsToAgent, + SessionNotFoundError, +} from "@aoagents/ao-core"; +import { getCorrelationId, jsonWithCorrelation } from "@/lib/observability"; +import { getServices } from "@/lib/services"; +import { validateConfiguredProject, validateIdentifier } from "@/lib/validation"; + +export async function POST(request: Request) { + const correlationId = getCorrelationId(request); + const body = (await request.json().catch(() => null)) as Record | null; + if (!body) { + return jsonWithCorrelation({ error: "Invalid JSON body" }, { status: 400 }, correlationId); + } + + const projectIdErr = validateIdentifier(body.projectId, "projectId"); + if (projectIdErr) { + return jsonWithCorrelation({ error: projectIdErr }, { status: 400 }, correlationId); + } + + const runIdErr = validateIdentifier(body.runId, "runId"); + if (runIdErr) { + return jsonWithCorrelation({ error: runIdErr }, { status: 400 }, correlationId); + } + + try { + const { config, sessionManager } = await getServices(); + const projectId = String(body.projectId); + const configuredProjectErr = validateConfiguredProject(config.projects, projectId); + if (configuredProjectErr) { + return jsonWithCorrelation({ error: configuredProjectErr }, { status: 404 }, correlationId); + } + + const result = await sendCodeReviewFindingsToAgent( + { config, sessionManager }, + { projectId, runId: String(body.runId) }, + ); + + return jsonWithCorrelation(result, { status: 200 }, correlationId); + } catch (error) { + if (error instanceof SessionNotFoundError) { + return jsonWithCorrelation({ error: error.message }, { status: 404 }, correlationId); + } + if (error instanceof CodeReviewRunNotFoundError) { + return jsonWithCorrelation({ error: error.message }, { status: 404 }, correlationId); + } + if (error instanceof CodeReviewNoOpenFindingsError) { + return jsonWithCorrelation({ error: error.message }, { status: 409 }, correlationId); + } + + const message = error instanceof Error ? error.message : "Failed to send review findings"; + return jsonWithCorrelation({ error: message }, { status: 500 }, correlationId); + } +} diff --git a/packages/web/src/app/api/sessions/route.ts b/packages/web/src/app/api/sessions/route.ts index 539740f16..c9645bafd 100644 --- a/packages/web/src/app/api/sessions/route.ts +++ b/packages/web/src/app/api/sessions/route.ts @@ -10,7 +10,7 @@ import { import { getCorrelationId, jsonWithCorrelation, recordApiObservation } from "@/lib/observability"; import { filterProjectSessions } from "@/lib/project-utils"; import { settlesWithin } from "@/lib/async-utils"; -import { isDashboardSessionTerminal, type DashboardOrchestratorLink } from "@/lib/types"; +import { type DashboardOrchestratorLink } from "@/lib/types"; const METADATA_ENRICH_TIMEOUT_MS = 3_000; @@ -136,8 +136,8 @@ export async function GET(request: Request) { let dashboardSessions = workerSessions.map(sessionToDashboard); if (activeOnly) { - const activeIndices = dashboardSessions - .map((session, index) => (!isDashboardSessionTerminal(session) ? index : -1)) + const activeIndices = workerSessions + .map((session, index) => (!isTerminalSession(session) ? index : -1)) .filter((index) => index !== -1); workerSessions = activeIndices.map((index) => workerSessions[index]); dashboardSessions = activeIndices.map((index) => dashboardSessions[index]); diff --git a/packages/web/src/app/globals.css b/packages/web/src/app/globals.css index e47aa6308..1ae049ef9 100644 --- a/packages/web/src/app/globals.css +++ b/packages/web/src/app/globals.css @@ -1219,6 +1219,41 @@ html.light .xterm .xterm-viewport:hover::-webkit-scrollbar-thumb:active { gap: 6px; } +.workspace-mode-switch { + display: inline-flex; + align-items: center; + gap: 2px; + margin-left: 8px; + border: 1px solid var(--color-border-subtle); + border-radius: 5px; + background: var(--color-bg-primary); + padding: 2px; +} + +.workspace-mode-switch__item { + display: inline-flex; + align-items: center; + height: 22px; + padding: 0 8px; + border-radius: 3px; + color: var(--color-text-muted); + text-decoration: none; + font-size: 11px; + font-weight: 500; +} + +.workspace-mode-switch__item:hover { + background: var(--color-bg-hover); + color: var(--color-text-primary); + text-decoration: none; +} + +.workspace-mode-switch__item--active { + background: var(--color-bg-surface); + color: var(--color-text-primary); + box-shadow: inset 0 0 0 1px var(--color-border-subtle); +} + .dashboard-app-btn { display: inline-flex; align-items: center; @@ -2954,6 +2989,13 @@ html.light .xterm .xterm-viewport:hover::-webkit-scrollbar-thumb:active { padding: 5px 10px 7px; } +.session-card__footer-actions { + display: inline-flex; + align-items: center; + gap: 6px; + flex-shrink: 0; +} + .card__status { flex: 1; min-width: 0; @@ -3031,6 +3073,27 @@ html.light .xterm .xterm-viewport:hover::-webkit-scrollbar-thumb:active { color: var(--color-accent-amber); } +.session-card__review-control { + font-size: 10.5px; + font-family: var(--font-sans); + font-weight: 600; + padding: 2px 8px 2px 6px; + border-radius: 4px; + border: 1px solid color-mix(in srgb, var(--color-accent) 22%, transparent); + background: color-mix(in srgb, var(--color-accent) 5%, transparent); + color: var(--color-accent); +} + +.session-card__review-control:hover:not(:disabled) { + background: color-mix(in srgb, var(--color-accent) 10%, transparent); + border-color: color-mix(in srgb, var(--color-accent) 36%, transparent); +} + +.session-card__review-control:disabled { + cursor: wait; + opacity: 0.72; +} + .btn--danger { width: 26px; height: 26px; @@ -5253,6 +5316,491 @@ html.light .xterm .xterm-viewport:hover::-webkit-scrollbar-thumb:active { border-radius: 0; } +/* ── Review workbench ─────────────────────────────────────────────── */ + +.review-dashboard-main { + overflow-y: auto; + padding: 18px 18px 20px; +} + +.review-main-header { + display: flex; + align-items: flex-start; + justify-content: space-between; + gap: 18px; + margin-bottom: 16px; + border-bottom: 1px solid var(--color-border-subtle); + padding-bottom: 14px; +} + +.review-kanban-board { + grid-template-columns: repeat(var(--kanban-column-count), minmax(250px, 1fr)); + min-width: 1750px; +} + +.review-kanban-column { + min-height: 420px; +} + +.review-column-hint { + margin-top: 5px; + min-height: 28px; + font-size: 10.5px; + line-height: 1.35; + color: var(--color-text-tertiary); +} + +.review-new-menu { + position: relative; +} + +.review-new-menu__popover { + position: absolute; + top: calc(100% + 8px); + right: 0; + z-index: 30; + width: min(320px, calc(100vw - 48px)); + max-height: 360px; + overflow-y: auto; + border: 1px solid var(--color-border-default); + border-radius: 6px; + background: var(--color-bg-surface); + box-shadow: 0 16px 36px rgb(0 0 0 / 0.26); + padding: 5px; +} + +.review-new-menu__item { + display: flex; + width: 100%; + min-width: 0; + flex-direction: column; + gap: 3px; + border-radius: 4px; + padding: 8px; + text-align: left; + color: var(--color-text-secondary); +} + +.review-new-menu__item:hover:not(:disabled) { + background: var(--color-bg-hover); + color: var(--color-text-primary); +} + +.review-new-menu__item:disabled { + cursor: wait; + opacity: 0.65; +} + +.review-new-menu__item-title, +.review-new-menu__item-meta { + min-width: 0; + max-width: 100%; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.review-new-menu__item-title { + font-size: 12px; + font-weight: 600; +} + +.review-new-menu__item-meta { + font-family: var(--font-mono); + font-size: 10px; + color: var(--color-text-muted); +} + +.review-column-dot[data-review-column="queued"] { + background: var(--color-text-tertiary); +} + +.review-column-dot[data-review-column="reviewing"] { + background: var(--color-status-review); +} + +.review-column-dot[data-review-column="triage"] { + background: var(--color-status-attention); +} + +.review-column-dot[data-review-column="waiting"] { + background: var(--color-accent); +} + +.review-column-dot[data-review-column="clean"] { + background: var(--color-status-ready); +} + +.review-column-dot[data-review-column="failed"] { + background: var(--color-status-error); +} + +.review-column-dot[data-review-column="outdated"] { + background: var(--color-text-muted); +} + +.review-card { + min-height: 166px; + border-left-color: var(--color-border-default); +} + +.review-card[data-review-status="running"] { + border-left-color: var(--color-status-review); +} + +.review-card[data-review-status="needs_triage"] { + border-left-color: var(--color-status-attention); +} + +.review-card[data-review-status="sent_to_agent"], +.review-card[data-review-status="waiting_update"] { + border-left-color: var(--color-accent); +} + +.review-card[data-review-status="clean"] { + border-left-color: var(--color-status-ready); +} + +.review-card[data-review-status="failed"], +.review-card[data-review-status="cancelled"] { + border-left-color: var(--color-status-error); +} + +.review-card[data-review-status="outdated"] { + border-left-color: var(--color-text-muted); + opacity: 0.78; +} + +.review-card .card__id { + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; +} + +.review-card .card__alerts { + margin-top: auto; +} + +.review-card .session-card__footer { + margin-top: 0; +} + +.review-card .session-card__footer-actions { + gap: 5px; +} + +.review-card .session-card__footer-actions .session-card__control { + height: 24px; + padding-right: 7px; + padding-left: 7px; +} + +.review-card__disabled-control { + cursor: default; + opacity: 0.58; +} + +.review-card__disabled-control:hover { + border-color: var(--color-border-default); + background: var(--color-bg-elevated); + color: var(--color-text-secondary); +} + +.review-card__finding-alert .alert-row__text button { + display: block; + width: 100%; + min-width: 0; + overflow: hidden; + color: inherit; + font: inherit; + text-align: left; + text-overflow: ellipsis; + white-space: nowrap; +} + +.review-card__finding-alert .alert-row__text button:hover { + color: var(--color-text-primary); + text-decoration: underline; + text-underline-offset: 2px; +} + +.review-detail-backdrop { + position: fixed; + inset: 0; + z-index: 80; + background: rgba(0, 0, 0, 0.42); +} + +.review-detail-panel { + position: fixed; + top: 0; + right: 0; + bottom: 0; + z-index: 81; + display: flex; + width: min(440px, calc(100vw - 18px)); + flex-direction: column; + border-left: 1px solid var(--color-border-default); + background: var(--color-bg-elevated); + box-shadow: -18px 0 42px rgba(0, 0, 0, 0.28); +} + +.review-detail-panel__header { + display: flex; + align-items: flex-start; + gap: 16px; + justify-content: space-between; + border-bottom: 1px solid var(--color-border-subtle); + padding: 18px 18px 14px; +} + +.review-detail-panel__eyebrow { + font-family: var(--font-mono); + font-size: 10px; + letter-spacing: 0.04em; + color: var(--color-text-muted); +} + +.review-detail-panel__title { + margin-top: 4px; + font-size: 16px; + font-weight: 600; + line-height: 1.35; + color: var(--color-text-primary); +} + +.review-detail-panel__close { + display: inline-flex; + width: 28px; + height: 28px; + align-items: center; + justify-content: center; + border: 1px solid var(--color-border-default); + border-radius: 4px; + color: var(--color-text-secondary); + font-size: 18px; + line-height: 1; +} + +.review-detail-panel__close:hover { + border-color: var(--color-border-strong); + color: var(--color-text-primary); +} + +.review-detail-panel__meta, +.review-detail-panel__actions, +.review-detail-panel__summary { + display: flex; + flex-wrap: wrap; + gap: 6px; + padding: 12px 18px 0; +} + +.review-detail-panel__meta span, +.review-detail-panel__actions a { + display: inline-flex; + align-items: center; + border: 1px solid var(--color-border-subtle); + border-radius: 4px; + background: var(--color-bg-subtle); + padding: 3px 7px; + font-size: 10.5px; + color: var(--color-text-secondary); +} + +.review-detail-panel__actions a { + border-color: color-mix(in srgb, var(--color-accent) 22%, transparent); + background: color-mix(in srgb, var(--color-accent) 5%, transparent); + color: var(--color-accent); + font-weight: 600; + text-decoration: none; +} + +.review-detail-panel__actions a:hover { + border-color: color-mix(in srgb, var(--color-accent) 36%, transparent); + background: color-mix(in srgb, var(--color-accent) 10%, transparent); +} + +.review-detail-panel__notice { + margin: 12px 18px 0; + border: 1px solid color-mix(in srgb, var(--color-status-attention) 30%, transparent); + border-radius: 5px; + background: color-mix(in srgb, var(--color-status-attention) 8%, transparent); + padding: 9px 10px; + color: var(--color-text-secondary); + font-size: 11.5px; + line-height: 1.45; +} + +.review-detail-panel__summary { + display: grid; + grid-template-columns: repeat(3, minmax(0, 1fr)); + padding-top: 16px; +} + +.review-detail-panel__summary-item { + border: 1px solid var(--color-border-subtle); + border-radius: 5px; + background: var(--color-bg-subtle); + padding: 8px; +} + +.review-detail-panel__summary-item span { + display: block; + font-size: 9.5px; + letter-spacing: 0.05em; + color: var(--color-text-muted); + text-transform: uppercase; +} + +.review-detail-panel__summary-item strong { + display: block; + margin-top: 4px; + overflow: hidden; + color: var(--color-text-primary); + font-size: 13px; + font-weight: 600; + text-overflow: ellipsis; + white-space: nowrap; +} + +.review-detail-panel__content { + display: flex; + min-height: 0; + flex: 1; + flex-direction: column; + gap: 10px; + overflow-y: auto; + padding: 16px 18px 18px; +} + +.review-detail-panel__empty, +.review-detail-panel__error { + border: 1px dashed var(--color-border-default); + border-radius: 6px; + padding: 18px; + color: var(--color-text-secondary); + font-size: 12px; + line-height: 1.45; +} + +.review-detail-panel__error { + border-color: color-mix(in srgb, var(--color-status-error) 35%, transparent); + color: var(--color-status-error); +} + +.review-detail-finding { + border: 1px solid var(--color-border-subtle); + border-left: 3px solid var(--color-text-muted); + border-radius: 6px; + background: var(--card-bg); + padding: 11px 12px 12px; +} + +.review-detail-finding[data-severity="warning"] { + border-left-color: var(--color-status-attention); +} + +.review-detail-finding[data-severity="error"] { + border-left-color: var(--color-status-error); +} + +.review-detail-finding[data-severity="info"] { + border-left-color: var(--color-accent); +} + +.review-detail-finding__header { + display: flex; + gap: 6px; + margin-bottom: 8px; +} + +.review-detail-finding__header span { + border: 1px solid var(--color-border-subtle); + border-radius: 3px; + padding: 2px 6px; + font-family: var(--font-mono); + font-size: 9.5px; + color: var(--color-text-muted); + text-transform: lowercase; +} + +.review-detail-finding h3 { + color: var(--color-text-primary); + font-size: 12.5px; + font-weight: 600; + line-height: 1.35; +} + +.review-detail-finding code { + display: inline-block; + max-width: 100%; + overflow: hidden; + margin-top: 7px; + border: 1px solid var(--color-border-subtle); + border-radius: 3px; + background: var(--color-bg-subtle); + padding: 2px 5px; + color: var(--color-text-secondary); + font-size: 10px; + text-overflow: ellipsis; + white-space: nowrap; +} + +.review-detail-finding p { + margin-top: 9px; + color: var(--color-text-secondary); + font-size: 11.5px; + line-height: 1.5; +} + +.review-empty-state { + margin: 36px auto 0; + max-width: 460px; + border: 1px dashed var(--color-border-default); + border-radius: 7px; + padding: 28px; + text-align: center; + color: var(--color-text-secondary); +} + +.review-empty-state__title { + font-size: 15px; + font-weight: 600; + color: var(--color-text-primary); +} + +.review-empty-state__body { + margin-top: 7px; + font-size: 12px; + line-height: 1.55; + color: var(--color-text-muted); +} + +.review-empty-state__link { + margin-top: 14px; + display: inline-flex; + border: 1px solid var(--color-border-default); + border-radius: 5px; + padding: 7px 10px; + font-size: 12px; + font-weight: 500; + color: var(--color-text-secondary); + text-decoration: none; +} + +.review-empty-state__link:hover { + background: var(--color-bg-hover); + color: var(--color-text-primary); + text-decoration: none; +} + +@media (max-width: 767px) { + .review-kanban-board { + min-width: 0; + } +} + /* ── Done / Terminated collapsible bar ────────────────────────────────── */ .done-bar__toggle { diff --git a/packages/web/src/app/review/page.tsx b/packages/web/src/app/review/page.tsx new file mode 100644 index 000000000..e1b7af9d6 --- /dev/null +++ b/packages/web/src/app/review/page.tsx @@ -0,0 +1,37 @@ +import type { Metadata } from "next"; +import { ReviewDashboard } from "@/components/ReviewDashboard"; +import { + getReviewPageData, + getReviewProjectName, + resolveReviewProjectFilter, +} from "@/lib/review-page-data"; + +export const dynamic = "force-dynamic"; + +export async function generateMetadata(props: { + searchParams: Promise<{ project?: string }>; +}): Promise { + const searchParams = await props.searchParams; + const projectFilter = resolveReviewProjectFilter(searchParams.project); + const projectName = getReviewProjectName(projectFilter); + return { title: { absolute: `ao | ${projectName} Reviews` } }; +} + +export default async function ReviewRoute(props: { searchParams: Promise<{ project?: string }> }) { + const searchParams = await props.searchParams; + const projectFilter = resolveReviewProjectFilter(searchParams.project); + const pageData = await getReviewPageData(projectFilter); + + return ( + + ); +} diff --git a/packages/web/src/app/reviews/page.tsx b/packages/web/src/app/reviews/page.tsx new file mode 100644 index 000000000..090a5b64d --- /dev/null +++ b/packages/web/src/app/reviews/page.tsx @@ -0,0 +1,11 @@ +import { redirect } from "next/navigation"; + +export const dynamic = "force-dynamic"; + +export default async function ReviewsAliasRoute(props: { + searchParams: Promise<{ project?: string }>; +}) { + const searchParams = await props.searchParams; + const suffix = searchParams.project ? `?project=${encodeURIComponent(searchParams.project)}` : ""; + redirect(`/review${suffix}`); +} diff --git a/packages/web/src/app/sessions/[id]/page.test.tsx b/packages/web/src/app/sessions/[id]/page.test.tsx index 6fa1990e9..b7e7b87d1 100644 --- a/packages/web/src/app/sessions/[id]/page.test.tsx +++ b/packages/web/src/app/sessions/[id]/page.test.tsx @@ -291,17 +291,12 @@ describe("SessionPage project polling", () => { const { default: SessionPage } = await import("./page"); - render( - - - , - ); + render(); await flushAsyncWork(); expect(screen.getByText("Session not found")).toBeInTheDocument(); expect(screen.getByRole("button", { name: "Toggle sidebar" })).toBeInTheDocument(); expect(screen.queryByTestId("session-detail")).not.toBeInTheDocument(); - expect(screen.getByTestId("route-error")).toHaveTextContent("NEXT_NOT_FOUND"); }); it("renders an inline error state instead of throwing the route away", async () => { diff --git a/packages/web/src/components/AttentionZone.tsx b/packages/web/src/components/AttentionZone.tsx index e68d4ddaf..422bedd23 100644 --- a/packages/web/src/components/AttentionZone.tsx +++ b/packages/web/src/components/AttentionZone.tsx @@ -1,11 +1,7 @@ "use client"; import { memo, useEffect, useState } from "react"; -import { - type DashboardSession, - type AttentionLevel, - isPRMergeReady, -} from "@/lib/types"; +import { type DashboardSession, type AttentionLevel, isPRMergeReady } from "@/lib/types"; import { SessionCard } from "./SessionCard"; import { getSessionTitle } from "@/lib/format"; import { projectSessionPath } from "@/lib/routes"; @@ -17,6 +13,7 @@ interface AttentionZoneProps { onKill?: (sessionId: string) => void; onMerge?: (prNumber: number) => void; onRestore?: (sessionId: string) => void; + onReview?: (sessionId: string) => Promise | void; /** Accordion mode: whether this section is collapsed (mobile only) */ collapsed?: boolean; /** Accordion mode: called when the header is tapped to toggle */ @@ -82,6 +79,7 @@ function AttentionZoneView({ onKill, onMerge, onRestore, + onReview, collapsed, onToggle, compactMobile, @@ -121,7 +119,9 @@ function AttentionZoneView({ {config.label} {sessions.length} - +
@@ -143,6 +143,7 @@ function AttentionZoneView({ onKill={onKill} onMerge={onMerge} onRestore={onRestore} + onReview={onReview} /> ), )} @@ -187,6 +188,7 @@ function AttentionZoneView({ onKill={onKill} onMerge={onMerge} onRestore={onRestore} + onReview={onReview} /> ))}
@@ -205,6 +207,7 @@ function areAttentionZonePropsEqual(prev: AttentionZoneProps, next: AttentionZon prev.onKill === next.onKill && prev.onMerge === next.onMerge && prev.onRestore === next.onRestore && + prev.onReview === next.onReview && prev.compactMobile === next.compactMobile && prev.onPreview === next.onPreview && prev.resetKey === next.resetKey && @@ -239,11 +242,7 @@ function MobileSessionRow({ aria-label={`Open ${getSessionTitle(session)}`} >
-
@@ -253,7 +252,7 @@ function MobileSessionRow({
diff --git a/packages/web/src/components/Dashboard.tsx b/packages/web/src/components/Dashboard.tsx index ff646762a..1c9ee86f5 100644 --- a/packages/web/src/components/Dashboard.tsx +++ b/packages/web/src/components/Dashboard.tsx @@ -27,7 +27,7 @@ import { CopyDebugBundleButton } from "./CopyDebugBundleButton"; import { SidebarContext, useSidebarContext } from "./workspace/SidebarContext"; import { ProjectSidebar } from "./ProjectSidebar"; import { isOrchestratorSession } from "@aoagents/ao-core/types"; -import { projectDashboardPath, projectSessionPath } from "@/lib/routes"; +import { projectDashboardPath, projectReviewPath, projectSessionPath } from "@/lib/routes"; import { BottomSheet } from "./BottomSheet"; interface DashboardProps { @@ -244,6 +244,8 @@ function DashboardInner({ sessionsRef.current = sessions; const allProjectsView = projects.length > 1 && projectId === undefined; + const codingHref = projectId ? projectDashboardPath(projectId) : "/?project=all"; + const reviewHref = projectReviewPath(projectId); const currentProjectOrchestrator = useMemo( () => projectId @@ -463,6 +465,32 @@ function DashboardInner({ [showToast], ); + const handleRequestReview = useCallback( + async (sessionId: string) => { + try { + const res = await fetch("/api/reviews", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ sessionId }), + }); + const data = (await res.json().catch(() => null)) as { error?: string } | null; + if (!res.ok) { + throw new Error(data?.error ?? "Failed to request review"); + } + + const session = sessionsRef.current.find((entry) => entry.id === sessionId); + showToast("Review run requested", "success"); + routerRef.current.push(projectReviewPath(session?.projectId ?? projectId)); + } catch (error) { + const message = error instanceof Error ? error.message : "Failed to request review"; + console.error(`Failed to request review for ${sessionId}:`, error); + showToast(`Review failed: ${message}`, "error"); + throw error; + } + }, + [projectId, showToast], + ); + const handleSpawnOrchestrator = async (project: ProjectInfo) => { setSpawningProjectIds((current) => current.includes(project.id) ? current : [...current, project.id], @@ -589,6 +617,18 @@ function DashboardInner({
{headerProjectLabel} +
{!allProjectsView && projectSessions.length > 0 ? (
@@ -751,6 +791,7 @@ function DashboardInner({ onKill={handleKill} onMerge={handleMerge} onRestore={handleRestore} + onReview={handleRequestReview} compactMobile={isMobile} collapsed={isMobile && collapsedZones.has(level)} onToggle={isMobile ? handleZoneToggle : undefined} diff --git a/packages/web/src/components/ProjectSidebar.tsx b/packages/web/src/components/ProjectSidebar.tsx index c6b29a7f6..24c21691f 100644 --- a/packages/web/src/components/ProjectSidebar.tsx +++ b/packages/web/src/components/ProjectSidebar.tsx @@ -9,7 +9,7 @@ import { getAttentionLevel, type DashboardSession } from "@/lib/types"; import { isOrchestratorSession } from "@aoagents/ao-core/types"; import { getSessionTitle, humanizeBranch } from "@/lib/format"; import { usePopoverClamp } from "@/hooks/usePopoverClamp"; -import { projectDashboardPath, projectSessionPath } from "@/lib/routes"; +import { projectDashboardPath, projectReviewPath, projectSessionPath } from "@/lib/routes"; import { ThemeToggle } from "./ThemeToggle"; import { AddProjectModal } from "./AddProjectModal"; import { ProjectSettingsModal } from "./ProjectSettingsModal"; @@ -846,6 +846,31 @@ function ProjectSidebarInner({ ) : null} + {!isDegraded ? ( + { + e.stopPropagation(); + onMobileClose?.(); + }} + className="project-sidebar__proj-action" + aria-label={`Open ${project.name} reviews`} + title="Reviews" + > + + + + + + ) : null} + {!isDegraded && orchestratorLink && ( = { + queued: "Review work requested but not executing yet.", + reviewing: "A reviewer is reading a snapshot.", + triage: "Findings need a human decision.", + waiting: "Feedback is with the coding worker.", + clean: "No open AO findings remain.", + failed: "Reviewer runs that need retry or inspection.", + outdated: "Runs superseded by newer worker commits.", +}; + +const SUPERSEDABLE_REVIEW_STATUSES = new Set([ + "queued", + "needs_triage", + "sent_to_agent", + "waiting_update", + "clean", +]); + +function formatRelativeTime(value: string): string { + const timestamp = Date.parse(value); + if (Number.isNaN(timestamp)) return value; + const diffMs = Date.now() - timestamp; + const diffMin = Math.floor(diffMs / 60_000); + if (diffMin < 1) return "just now"; + if (diffMin < 60) return `${diffMin}m ago`; + const diffHours = Math.floor(diffMin / 60); + if (diffHours < 24) return `${diffHours}h ago`; + return `${Math.floor(diffHours / 24)}d ago`; +} + +function formatStatus(value: string): string { + return value.replaceAll("_", " "); +} + +function formatFindingLocation(finding: CodeReviewFinding): string | null { + if (!finding.filePath) return null; + if (finding.startLine === undefined) return finding.filePath; + if (finding.endLine !== undefined && finding.endLine !== finding.startLine) { + return `${finding.filePath}:${finding.startLine}-${finding.endLine}`; + } + return `${finding.filePath}:${finding.startLine}`; +} + +function pluralize(count: number, singular: string, plural = `${singular}s`): string { + return `${count} ${count === 1 ? singular : plural}`; +} + +function canSendFeedbackToWorker(run: DashboardReviewRun): boolean { + if (!run.workerHasRuntime) return false; + if (run.workerActivity === "exited") return false; + return run.workerRuntimeState !== "missing" && run.workerRuntimeState !== "exited"; +} + +function getWorkerAvailabilityLabel(run: DashboardReviewRun): string { + if (!run.workerHasRuntime) return "no runtime"; + if (run.workerActivity === "exited") return "exited"; + if (run.workerRuntimeState === "missing") return "runtime missing"; + if (run.workerRuntimeState === "exited") return "runtime exited"; + return run.workerActivity ?? run.workerStatus ?? "worker"; +} + +function mergeOrchestrators( + current: DashboardOrchestratorLink[], + incoming: DashboardOrchestratorLink[], +): DashboardOrchestratorLink[] { + const merged = new Map(current.map((orchestrator) => [orchestrator.projectId, orchestrator])); + for (const orchestrator of incoming) { + merged.set(orchestrator.projectId, orchestrator); + } + return Array.from(merged.values()); +} + +function markSupersededReviewRuns( + current: DashboardReviewRun[], + nextRun: DashboardReviewRun, +): DashboardReviewRun[] { + if (!nextRun.targetSha) return current; + + return current.map((run) => { + if (run.linkedSessionId !== nextRun.linkedSessionId) return run; + if (run.id === nextRun.id) return run; + if (!run.targetSha || run.targetSha === nextRun.targetSha) return run; + if (!SUPERSEDABLE_REVIEW_STATUSES.has(run.status)) return run; + return { ...run, status: "outdated" }; + }); +} + +function ReviewDashboardInner({ + runs = EMPTY_RUNS, + sidebarSessions = EMPTY_SESSIONS, + orchestrators = EMPTY_ORCHESTRATORS, + workerOptions = EMPTY_WORKERS, + projectId, + projectName, + projects, + dashboardLoadError, +}: ReviewDashboardProps) { + const { showToast } = useToast(); + const router = useRouter(); + const menuRef = useRef(null); + const [reviewRuns, setReviewRuns] = useState(runs); + const [activeOrchestrators, setActiveOrchestrators] = + useState(orchestrators); + const [requestingSessionId, setRequestingSessionId] = useState(null); + const [executingRunIds, setExecutingRunIds] = useState>(() => new Set()); + const [sendingRunIds, setSendingRunIds] = useState>(() => new Set()); + const [restoringOrchestratorId, setRestoringOrchestratorId] = useState(null); + const [newReviewMenuOpen, setNewReviewMenuOpen] = useState(false); + const [reviewDetails, setReviewDetails] = useState(null); + const [sidebarCollapsed, setSidebarCollapsed] = useState(false); + const [mobileMenuOpen, setMobileMenuOpen] = useState(false); + const isMobile = useMediaQuery(MOBILE_BREAKPOINT); + + useEffect(() => { + setReviewRuns(runs); + }, [runs]); + + useEffect(() => { + setActiveOrchestrators((current) => mergeOrchestrators(current, orchestrators)); + }, [orchestrators]); + + useEffect(() => { + if (!newReviewMenuOpen) return; + const handlePointer = (event: MouseEvent) => { + if (menuRef.current && !menuRef.current.contains(event.target as Node)) { + setNewReviewMenuOpen(false); + } + }; + const handleKey = (event: KeyboardEvent) => { + if (event.key === "Escape") { + setNewReviewMenuOpen(false); + } + }; + document.addEventListener("mousedown", handlePointer); + document.addEventListener("keydown", handleKey); + return () => { + document.removeEventListener("mousedown", handlePointer); + document.removeEventListener("keydown", handleKey); + }; + }, [newReviewMenuOpen]); + + useEffect(() => { + if (!reviewDetails) return; + const handleKey = (event: KeyboardEvent) => { + if (event.key === "Escape") { + setReviewDetails(null); + } + }; + document.addEventListener("keydown", handleKey); + return () => document.removeEventListener("keydown", handleKey); + }, [reviewDetails]); + + const grouped = useMemo(() => { + const columns: Record = { + queued: [], + reviewing: [], + triage: [], + waiting: [], + clean: [], + failed: [], + outdated: [], + }; + for (const run of reviewRuns) { + columns[getReviewBoardColumn(run)].push(run); + } + return columns; + }, [reviewRuns]); + + const allProjectsView = !projectId; + const openFindingCount = reviewRuns.reduce((sum, run) => sum + run.openFindingCount, 0); + const activeRunCount = reviewRuns.filter((run) => + ["queued", "preparing", "running", "needs_triage", "sent_to_agent", "waiting_update"].includes( + run.status, + ), + ).length; + const currentProjectOrchestrator = projectId + ? (activeOrchestrators.find((orchestrator) => orchestrator.projectId === projectId) ?? null) + : null; + const orchestratorHref = currentProjectOrchestrator + ? projectSessionPath(currentProjectOrchestrator.projectId, currentProjectOrchestrator.id) + : null; + const visibleWorkerOptions = projectId + ? workerOptions.filter((worker) => worker.projectId === projectId) + : workerOptions; + const codingHref = projectId ? projectDashboardPath(projectId) : "/?project=all"; + const reviewHref = projectReviewPath(projectId); + const headerProjectLabel = projectName ?? (allProjectsView ? "All projects" : "Reviews"); + + const handleToggleSidebar = () => { + if (isMobile) { + setMobileMenuOpen((current) => !current); + } else { + setSidebarCollapsed((current) => !current); + } + }; + + const handleRequestReview = async (worker: ReviewWorkerOption) => { + if (requestingSessionId) return; + setRequestingSessionId(worker.id); + try { + const response = await fetch("/api/reviews", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ sessionId: worker.id }), + }); + const data = (await response.json().catch(() => null)) as { + run?: DashboardReviewRun; + error?: string; + } | null; + if (!response.ok || !data?.run) { + throw new Error(data?.error ?? "Failed to request review"); + } + + const nextRun: DashboardReviewRun = { + ...data.run, + projectName: worker.projectName, + workerTitle: worker.title, + workerBranch: worker.branch, + workerPrUrl: worker.prUrl ?? data.run.prUrl ?? null, + workerStatus: worker.status, + workerActivity: worker.activity, + workerRuntimeState: worker.runtimeState, + workerHasRuntime: worker.hasRuntime, + }; + setReviewRuns((current) => [ + nextRun, + ...markSupersededReviewRuns( + current.filter((run) => run.id !== nextRun.id), + nextRun, + ), + ]); + setNewReviewMenuOpen(false); + showToast("Review run requested", "success"); + } catch (error) { + const message = error instanceof Error ? error.message : "Failed to request review"; + showToast(`Review failed: ${message}`, "error"); + } finally { + setRequestingSessionId(null); + } + }; + + const handleExecuteRun = async (run: DashboardReviewRun) => { + if (executingRunIds.has(run.id)) return; + setExecutingRunIds((current) => { + const next = new Set(current); + next.add(run.id); + return next; + }); + setReviewRuns((current) => + current.map((entry) => (entry.id === run.id ? { ...entry, status: "running" } : entry)), + ); + try { + const response = await fetch("/api/reviews/execute", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + projectId: run.projectId, + runId: run.id, + force: run.status === "failed", + }), + }); + const data = (await response.json().catch(() => null)) as { + run?: DashboardReviewRun; + error?: string; + } | null; + if (!response.ok || !data?.run) { + throw new Error(data?.error ?? "Failed to execute review"); + } + + setReviewRuns((current) => + current.map((entry) => + entry.id === run.id + ? { + ...entry, + ...data.run, + projectName: entry.projectName, + workerTitle: entry.workerTitle, + workerBranch: entry.workerBranch, + workerPrUrl: entry.workerPrUrl, + workerStatus: entry.workerStatus, + workerActivity: entry.workerActivity, + workerRuntimeState: entry.workerRuntimeState, + workerHasRuntime: entry.workerHasRuntime, + } + : entry, + ), + ); + if (data.run.status === "failed") { + showToast( + `Review failed: ${data.run.terminationReason ?? "Reviewer execution failed"}`, + "error", + ); + return; + } + showToast( + data.run.openFindingCount > 0 ? "Review findings ready" : "Review completed clean", + "success", + ); + } catch (error) { + const message = error instanceof Error ? error.message : "Failed to execute review"; + setReviewRuns((current) => + current.map((entry) => (entry.id === run.id ? { ...entry, status: "failed" } : entry)), + ); + showToast(`Review failed: ${message}`, "error"); + } finally { + setExecutingRunIds((current) => { + const next = new Set(current); + next.delete(run.id); + return next; + }); + } + }; + + const mergeRunUpdate = (run: DashboardReviewRun, nextRun: DashboardReviewRun) => ({ + ...run, + ...nextRun, + projectName: run.projectName, + workerTitle: run.workerTitle, + workerBranch: run.workerBranch, + workerPrUrl: run.workerPrUrl, + workerStatus: run.workerStatus, + workerActivity: run.workerActivity, + workerRuntimeState: run.workerRuntimeState, + workerHasRuntime: run.workerHasRuntime, + }); + + const handleSendFeedback = async (run: DashboardReviewRun) => { + if (sendingRunIds.has(run.id) || run.openFindingCount === 0) return; + setSendingRunIds((current) => { + const next = new Set(current); + next.add(run.id); + return next; + }); + try { + const response = await fetch("/api/reviews/send", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ projectId: run.projectId, runId: run.id }), + }); + const data = (await response.json().catch(() => null)) as { + run?: DashboardReviewRun; + sentFindingCount?: number; + error?: string; + } | null; + if (!response.ok || !data?.run) { + throw new Error(data?.error ?? "Failed to send review findings"); + } + + setReviewRuns((current) => + current.map((entry) => + entry.id === run.id ? mergeRunUpdate(entry, data.run as DashboardReviewRun) : entry, + ), + ); + setReviewDetails((current) => { + if (!current || current.run.id !== run.id) return current; + const sentAt = new Date().toISOString(); + return { + ...current, + run: mergeRunUpdate(current.run, data.run as DashboardReviewRun), + findings: current.findings.map((finding) => + finding.status === "open" + ? { ...finding, status: "sent_to_agent", sentToAgentAt: sentAt } + : finding, + ), + }; + }); + showToast( + `Sent ${pluralize(data.sentFindingCount ?? 0, "finding")} to ${run.linkedSessionId}`, + "success", + ); + router.push( + projectSessionHashPath(run.projectId, run.linkedSessionId, "#session-terminal-section"), + ); + } catch (error) { + const message = error instanceof Error ? error.message : "Failed to send review findings"; + showToast(`Feedback failed: ${message}`, "error"); + } finally { + setSendingRunIds((current) => { + const next = new Set(current); + next.delete(run.id); + return next; + }); + } + }; + + const handleRestoreOrchestrator = async (orchestrator: DashboardOrchestratorLink) => { + if (restoringOrchestratorId) return; + setRestoringOrchestratorId(orchestrator.id); + try { + const response = await fetch(`/api/sessions/${encodeURIComponent(orchestrator.id)}/restore`, { + method: "POST", + }); + const data = (await response.json().catch(() => null)) as { + error?: string; + session?: DashboardSession; + } | null; + if (!response.ok) { + throw new Error(data?.error ?? "Failed to restore orchestrator"); + } + + setActiveOrchestrators((current) => + current.map((entry) => + entry.id === orchestrator.id + ? { + ...entry, + status: data?.session?.status ?? entry.status, + activity: data?.session?.activity ?? entry.activity, + runtimeState: data?.session?.lifecycle?.runtimeState ?? "alive", + hasRuntime: true, + isTerminal: false, + isRestorable: false, + } + : entry, + ), + ); + showToast("Orchestrator restored", "success"); + router.push(projectSessionPath(orchestrator.projectId, orchestrator.id)); + } catch (error) { + const message = error instanceof Error ? error.message : "Failed to restore orchestrator"; + showToast(`Restore failed: ${message}`, "error"); + } finally { + setRestoringOrchestratorId(null); + } + }; + + const handleOpenReviewDetails = async (run: DashboardReviewRun) => { + setReviewDetails({ run, findings: [], loading: true, error: null }); + try { + const params = new URLSearchParams({ projectId: run.projectId, runId: run.id }); + const response = await fetch(`/api/reviews/findings?${params.toString()}`); + const data = (await response.json().catch(() => null)) as { + findings?: CodeReviewFinding[]; + error?: string; + } | null; + if (!response.ok || !data?.findings) { + throw new Error(data?.error ?? "Failed to load review findings"); + } + + setReviewDetails((current) => + current?.run.id === run.id + ? { ...current, findings: data.findings ?? [], loading: false, error: null } + : current, + ); + } catch (error) { + const message = error instanceof Error ? error.message : "Failed to load review findings"; + setReviewDetails((current) => + current?.run.id === run.id + ? { ...current, findings: [], loading: false, error: message } + : current, + ); + } + }; + + return ( + +
+
+ +
+
+
+ +
+
+ setSidebarCollapsed((current) => !current)} + onMobileClose={() => setMobileMenuOpen(false)} + /> +
+ {mobileMenuOpen ? ( +
setMobileMenuOpen(false)} /> + ) : null} + +
+
+
+

+ {projectName ? `${projectName} Reviews` : "Reviews"} +

+

+ AO-local reviewer runs, findings, and worker handoffs + {allProjectsView ? " across all projects" : " for this project"}. +

+
+
+ + + +
+
+ + {dashboardLoadError ? ( +
+ {dashboardLoadError} +
+ ) : null} + + {reviewRuns.length === 0 ? ( +
+
No review runs yet
+

+ Reviewer runs will appear here after a worker is ready for review or after a + manual review is requested. +

+ + Back to coding dashboard + +
+ ) : ( +
+
+ {REVIEW_BOARD_COLUMNS.map((column) => ( + + ))} +
+
+ )} +
+
+ {reviewDetails ? ( + setReviewDetails(null)} + onOpenWorker={() => setReviewDetails(null)} + isSending={sendingRunIds.has(reviewDetails.run.id)} + onSendFeedback={handleSendFeedback} + /> + ) : null} +
+ + ); +} + +export function ReviewDashboard(props: ReviewDashboardProps) { + return ( + + + + ); +} + +function ReviewMetric({ label, value, meta }: { label: string; value: number; meta: string }) { + return ( +
+ {value} + {label} + {meta} +
+ ); +} + +function ReviewColumn({ + column, + runs, + allProjectsView, + executingRunIds, + sendingRunIds, + onOpenDetails, + onExecute, + onSendFeedback, +}: { + column: ReviewBoardColumn; + runs: DashboardReviewRun[]; + allProjectsView: boolean; + executingRunIds: Set; + sendingRunIds: Set; + onOpenDetails: (run: DashboardReviewRun) => void; + onExecute: (run: DashboardReviewRun) => void; + onSendFeedback: (run: DashboardReviewRun) => void; +}) { + return ( +
+
+
+
+ {REVIEW_COLUMN_LABELS[column]} + {runs.length} +
+

{COLUMN_HINTS[column]}

+
+ +
+ {runs.length > 0 ? ( +
+ {runs.map((run) => ( + + ))} +
+ ) : null} +
+
+ ); +} + +function ReviewCard({ + run, + allProjectsView, + isExecuting, + isSending, + onOpenDetails, + onExecute, + onSendFeedback, +}: { + run: DashboardReviewRun; + allProjectsView: boolean; + isExecuting: boolean; + isSending: boolean; + onOpenDetails: (run: DashboardReviewRun) => void; + onExecute: (run: DashboardReviewRun) => void; + onSendFeedback: (run: DashboardReviewRun) => void; +}) { + const workerHref = projectDashboardSessionPath(run.projectId, run.linkedSessionId); + const title = run.workerTitle ?? run.linkedSessionId; + const status = formatStatus(run.status); + const totalFindingLabel = pluralize(run.findingCount, "finding"); + const secondaryText = + run.summary ?? + (run.status === "clean" + ? "Reviewer completed without open AO findings." + : `Review requested for ${run.linkedSessionId}.`); + const truthLine = `${status} · ${totalFindingLabel}${ + run.dismissedFindingCount > 0 ? ` · ${pluralize(run.dismissedFindingCount, "dismissed")}` : "" + }${run.sentFindingCount > 0 ? ` · ${pluralize(run.sentFindingCount, "sent")}` : ""} · worker ${getWorkerAvailabilityLabel(run)}`; + const canExecute = isExecuting || run.status === "queued" || run.status === "failed"; + const feedbackAvailable = canSendFeedbackToWorker(run); + const dotClass = + run.status === "running" || run.status === "preparing" + ? "card__adot--working" + : run.status === "clean" + ? "card__adot--ready" + : run.status === "needs_triage" || run.status === "failed" || run.status === "cancelled" + ? "card__adot--waiting" + : run.status === "sent_to_agent" || run.status === "waiting_update" + ? "card__adot--ready" + : "card__adot--idle"; + + return ( +
+
+ + + {allProjectsView ? `${run.projectName} · ` : ""} + {run.reviewerSessionId} + +
+ +
+ +
+
+

{title}

+
+ + + +
+

{secondaryText}

+
+ +
+

+ {truthLine} +

+
+ + {run.openFindingCount > 0 ? ( +
+
+ + + + + +
+
+ ) : null} + +
+ + {status} · updated {formatRelativeTime(run.updatedAt)} + +
+ {canExecute ? ( + + ) : null} + + Worker + + {feedbackAvailable ? ( + + ) : ( + + {getWorkerAvailabilityLabel(run)} + + )} +
+
+
+
+ ); +} + +function ReviewDetailsDrawer({ + state, + onClose, + onOpenWorker, + isSending, + onSendFeedback, +}: { + state: ReviewDetailsState; + onClose: () => void; + onOpenWorker: () => void; + isSending: boolean; + onSendFeedback: (run: DashboardReviewRun) => void; +}) { + const { run, findings, loading, error } = state; + const workerHref = projectDashboardSessionPath(run.projectId, run.linkedSessionId); + const feedbackHref = projectSessionHashPath( + run.projectId, + run.linkedSessionId, + "#session-terminal-section", + ); + const openFindings = findings.filter((finding) => finding.status === "open"); + const feedbackAvailable = canSendFeedbackToWorker(run); + + return ( + <> +
+ + + ); +} diff --git a/packages/web/src/components/SessionCard.tsx b/packages/web/src/components/SessionCard.tsx index 063dfda70..9132075fb 100644 --- a/packages/web/src/components/SessionCard.tsx +++ b/packages/web/src/components/SessionCard.tsx @@ -33,6 +33,7 @@ interface SessionCardProps { onKill?: (sessionId: string) => void; onMerge?: (prNumber: number) => void; onRestore?: (sessionId: string) => void; + onReview?: (sessionId: string) => Promise | void; } /** @@ -126,12 +127,20 @@ function getDoneStatusInfo(session: DashboardSession): { }; } -function SessionCardView({ session, onSend, onKill, onMerge, onRestore }: SessionCardProps) { +function SessionCardView({ + session, + onSend, + onKill, + onMerge, + onRestore, + onReview, +}: SessionCardProps) { const [expanded, setExpanded] = useState(false); const [sendingAction, setSendingAction] = useState(null); const [failedAction, setFailedAction] = useState(null); const [sendingQuickReply, setSendingQuickReply] = useState(null); const [sentQuickReply, setSentQuickReply] = useState(null); + const [requestingReview, setRequestingReview] = useState(false); const [killConfirming, setKillConfirming] = useState(false); const [replyText, setReplyText] = useState(""); const actionTimerRef = useRef | null>(null); @@ -259,6 +268,18 @@ function SessionCardView({ session, onSend, onKill, onMerge, onRestore }: Sessio onKill?.(session.id); }; + const handleReviewClick = async (e: React.MouseEvent) => { + e.stopPropagation(); + if (requestingReview || !onReview) return; + + setRequestingReview(true); + try { + await Promise.resolve(onReview(session.id)); + } finally { + setRequestingReview(false); + } + }; + /* ── Done card variant ──────────────────────────────────────────── */ if (isDone) { const statusInfo = getDoneStatusInfo(session); @@ -827,44 +848,15 @@ function SessionCardView({ session, onSend, onKill, onMerge, onRestore }: Sessio {isReadyToMerge && pr ? ( - - ) : ( - !isTerminal && ( - + ) : null} + +
+ ) : ( + !isTerminal && ( +
+ {onReview ? ( + + ) : null} + +
) )}
@@ -892,7 +959,8 @@ function areSessionCardPropsEqual(prev: SessionCardProps, next: SessionCardProps prev.onSend === next.onSend && prev.onKill === next.onKill && prev.onMerge === next.onMerge && - prev.onRestore === next.onRestore + prev.onRestore === next.onRestore && + prev.onReview === next.onReview ); } @@ -918,8 +986,8 @@ interface Alert { type: "ci" | "changes" | "review" | "conflict" | "comment"; icon: React.ReactNode; label: string; - url: string; count?: number; + url: string; notified?: boolean; actionLabel?: string; actionMessage?: string; diff --git a/packages/web/src/components/__tests__/Dashboard.projectOverview.test.tsx b/packages/web/src/components/__tests__/Dashboard.projectOverview.test.tsx index 731b9487e..27f8cbb48 100644 --- a/packages/web/src/components/__tests__/Dashboard.projectOverview.test.tsx +++ b/packages/web/src/components/__tests__/Dashboard.projectOverview.test.tsx @@ -100,6 +100,30 @@ describe("Dashboard project overview cards", () => { ); }); + it("renders the same Coding/Reviews switch in project-scoped dashboard headers", () => { + render( + , + ); + + expect(screen.getByRole("link", { name: "Coding" })).toHaveAttribute( + "href", + "/projects/my-app", + ); + expect(screen.getByRole("link", { name: "Coding" })).toHaveAttribute( + "aria-current", + "page", + ); + expect(screen.getByRole("link", { name: "Reviews" })).toHaveAttribute( + "href", + "/review?project=my-app", + ); + }); + it("renders a header spawn action when the project has no orchestrator yet", () => { render( ({ + useRouter: () => ({ push: vi.fn(), refresh: vi.fn() }), + usePathname: () => "/review", +})); + +vi.mock("next-themes", () => ({ + useTheme: () => ({ + resolvedTheme: "light", + setTheme: vi.fn(), + }), +})); + +function makeRun(overrides: Partial): DashboardReviewRun { + return { + id: "review-run-1", + projectId: "my-app", + projectName: "My App", + linkedSessionId: "app-1", + reviewerSessionId: "app-rev-1", + status: "needs_triage", + createdAt: "2026-05-10T10:00:00.000Z", + updatedAt: "2026-05-10T10:01:00.000Z", + findingCount: 2, + openFindingCount: 1, + dismissedFindingCount: 1, + sentFindingCount: 0, + resolvedFindingCount: 0, + workerTitle: "Add todo filters", + workerBranch: "feat/todo-filters", + workerPrUrl: "https://github.com/acme/todo/pull/7", + workerStatus: "review_pending", + workerActivity: "idle", + workerRuntimeState: "alive", + workerHasRuntime: true, + ...overrides, + }; +} + +describe("ReviewDashboard", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("renders review runs in review-specific columns", () => { + render( + , + ); + + expect(screen.getByRole("heading", { name: "My App Reviews" })).toBeInTheDocument(); + expect(screen.getByText("Triage")).toBeInTheDocument(); + expect(screen.getByText("Reviewing")).toBeInTheDocument(); + expect(screen.getByText("Add todo filters")).toBeInTheDocument(); + expect(screen.getByText("Persist completed todos")).toBeInTheDocument(); + expect(screen.getByRole("button", { name: /1 open finding/i })).toBeInTheDocument(); + const workerLinks = screen.getAllByRole("link", { name: "Worker" }); + expect( + workerLinks.some((link) => link.getAttribute("href") === "/projects/my-app?session=app-1"), + ).toBe(true); + }); + + it("renders an empty state when there are no review runs", () => { + render( + , + ); + + expect(screen.getByText("No review runs yet")).toBeInTheDocument(); + expect(screen.getByRole("link", { name: "Back to coding dashboard" })).toHaveAttribute( + "href", + "/projects/my-app", + ); + }); + + it("can execute multiple queued review runs without a board-wide lock", async () => { + const fetchMock = vi.fn( + () => + new Promise(() => { + // Keep requests pending so the test can assert simultaneous in-flight runs. + }), + ); + vi.stubGlobal("fetch", fetchMock); + + render( + , + ); + + const runButtons = screen.getAllByRole("button", { name: "Run" }); + fireEvent.click(runButtons[0]); + await screen.findByRole("button", { name: "Running" }); + + fireEvent.click(runButtons[1]); + + await waitFor(() => expect(fetchMock).toHaveBeenCalledTimes(2)); + }); + + it("surfaces a completed failed review run as a failure", async () => { + const fetchMock = vi.fn(async () => + Response.json({ + run: makeRun({ + id: "review-run-1", + reviewerSessionId: "app-rev-1", + status: "failed", + findingCount: 0, + openFindingCount: 0, + dismissedFindingCount: 0, + terminationReason: "Codex review failed: invalid arguments", + }), + }), + ); + vi.stubGlobal("fetch", fetchMock); + + render( + , + ); + + fireEvent.click(screen.getByRole("button", { name: "Run" })); + + expect( + await screen.findByText(/Review failed: Codex review failed: invalid arguments/i), + ).toBeInTheDocument(); + expect(screen.queryByText("Review completed clean")).not.toBeInTheDocument(); + }); +}); diff --git a/packages/web/src/components/__tests__/SessionCard.coverage.test.tsx b/packages/web/src/components/__tests__/SessionCard.coverage.test.tsx index 5ff28639a..680106c70 100644 --- a/packages/web/src/components/__tests__/SessionCard.coverage.test.tsx +++ b/packages/web/src/components/__tests__/SessionCard.coverage.test.tsx @@ -1,4 +1,4 @@ -import { act, fireEvent, render, screen } from "@testing-library/react"; +import { act, fireEvent, render, screen, waitFor } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { SessionCard } from "../SessionCard"; import { makePR, makeSession } from "../../__tests__/helpers"; @@ -135,4 +135,15 @@ describe("SessionCard diff coverage", () => { "kanban-card-enter", ); }); + + it("requests a review from active worker cards", async () => { + const onReview = vi.fn(async () => {}); + render(); + + fireEvent.click(screen.getByRole("button", { name: "Request review" })); + + await waitFor(() => { + expect(onReview).toHaveBeenCalledWith("reviewable-1"); + }); + }); }); diff --git a/packages/web/src/components/__tests__/SessionDetail.desktop.test.tsx b/packages/web/src/components/__tests__/SessionDetail.desktop.test.tsx index d92e80e6a..7f310bb56 100644 --- a/packages/web/src/components/__tests__/SessionDetail.desktop.test.tsx +++ b/packages/web/src/components/__tests__/SessionDetail.desktop.test.tsx @@ -297,6 +297,60 @@ describe("SessionDetail desktop layout", () => { ); }); + it("does not open a blank terminal when activity exited but lifecycle still reports alive", () => { + render( + , + ); + + expect(screen.getByRole("region", { name: "Session ended summary" })).toBeInTheDocument(); + expect(screen.getByText("Terminal ended")).toBeInTheDocument(); + expect(screen.queryByTestId("direct-terminal")).not.toBeInTheDocument(); + expect( + within(screen.getByRole("banner")).getByRole("button", { name: "Restore" }), + ).toBeInTheDocument(); + }); + it("shows restore for restorable orchestrator sessions", () => { render( { + return { status }; +} + +describe("getReviewBoardColumn", () => { + it("maps reviewer run statuses into review board columns", () => { + expect(getReviewBoardColumn(makeRun("queued"))).toBe("queued"); + expect(getReviewBoardColumn(makeRun("preparing"))).toBe("queued"); + expect(getReviewBoardColumn(makeRun("running"))).toBe("reviewing"); + expect(getReviewBoardColumn(makeRun("needs_triage"))).toBe("triage"); + expect(getReviewBoardColumn(makeRun("sent_to_agent"))).toBe("waiting"); + expect(getReviewBoardColumn(makeRun("waiting_update"))).toBe("waiting"); + expect(getReviewBoardColumn(makeRun("clean"))).toBe("clean"); + expect(getReviewBoardColumn(makeRun("failed"))).toBe("failed"); + expect(getReviewBoardColumn(makeRun("cancelled"))).toBe("failed"); + expect(getReviewBoardColumn(makeRun("outdated"))).toBe("outdated"); + }); +}); diff --git a/packages/web/src/lib/__tests__/types.test.ts b/packages/web/src/lib/__tests__/types.test.ts index 04b6036a1..350eee852 100644 --- a/packages/web/src/lib/__tests__/types.test.ts +++ b/packages/web/src/lib/__tests__/types.test.ts @@ -12,7 +12,9 @@ import { TERMINAL_ACTIVITIES, NON_RESTORABLE_STATUSES, isDashboardSessionDone, + isDashboardRuntimeEnded, isDashboardSessionRestorable, + isDashboardSessionTerminal, type DashboardSession, type DashboardPR, } from "../types"; @@ -264,6 +266,49 @@ describe("getAttentionLevel", () => { }); describe("restore affordances", () => { + it("treats exited activity as terminal even when lifecycle runtime is still alive", () => { + const session = createSession({ + status: "review_pending", + activity: "exited", + lifecycle: { + sessionState: "idle", + sessionReason: "awaiting_external_review", + prState: "open", + prReason: "review_pending", + runtimeState: "alive", + runtimeReason: "process_running", + session: { + state: "idle", + reason: "awaiting_external_review", + label: "idle", + reasonLabel: "awaiting external review", + }, + pr: { + state: "open", + reason: "review_pending", + label: "open", + reasonLabel: "review pending", + }, + runtime: { + state: "alive", + reason: "process_running", + label: "alive", + reasonLabel: "process running", + }, + legacyStatus: "review_pending", + evidence: null, + detectingAttempts: 0, + detectingEscalatedAt: null, + summary: "Waiting for review", + guidance: null, + }, + }); + + expect(isDashboardSessionTerminal(session)).toBe(true); + expect(isDashboardRuntimeEnded(session)).toBe(true); + expect(isDashboardSessionRestorable(session)).toBe(true); + }); + it("should not mark a running merged session as restorable (runtime still alive)", () => { const session = createSession({ status: "merged", diff --git a/packages/web/src/lib/review-page-data.ts b/packages/web/src/lib/review-page-data.ts new file mode 100644 index 000000000..1260d84c2 --- /dev/null +++ b/packages/web/src/lib/review-page-data.ts @@ -0,0 +1,178 @@ +import "server-only"; + +import { + createCodeReviewStore, + isOrchestratorSession, + isRestorable, + isTerminalSession, + markOutdatedCodeReviewRunsForSession, +} from "@aoagents/ao-core"; +import { getServices } from "@/lib/services"; +import { + getAllProjects, + getPrimaryProjectId, + getProjectName, + type ProjectInfo, +} from "@/lib/project-name"; +import type { DashboardReviewRun, ReviewWorkerOption } from "@/lib/review-types"; +import { listDashboardOrchestrators, sessionToDashboard } from "@/lib/serialize"; +import type { DashboardOrchestratorLink, DashboardSession } from "@/lib/types"; + +interface ReviewPageData { + runs: DashboardReviewRun[]; + sidebarSessions: DashboardSession[]; + orchestrators: DashboardOrchestratorLink[]; + workerOptions: ReviewWorkerOption[]; + projectName: string; + projects: ProjectInfo[]; + selectedProjectId?: string; + dashboardLoadError?: string; +} + +function formatReviewLoadError(err: unknown): string { + if (err instanceof Error && err.message.trim()) { + return err.message.split(/\r?\n/)[0]?.trim() || "Failed to load review data."; + } + return "Failed to load review data."; +} + +export function getReviewProjectName(projectFilter: string | undefined): string { + if (projectFilter === "all") return "All Projects"; + const projects = getAllProjects(); + if (projectFilter) { + const selectedProject = projects.find((project) => project.id === projectFilter); + if (selectedProject) return selectedProject.name; + } + return getProjectName(); +} + +export function resolveReviewProjectFilter(project?: string): string { + if (project === "all") return "all"; + const projects = getAllProjects(); + if (project && projects.some((entry) => entry.id === project)) { + return project; + } + return getPrimaryProjectId(); +} + +export async function getReviewPageData(project?: string): Promise { + const projectFilter = resolveReviewProjectFilter(project); + const pageData: ReviewPageData = { + runs: [], + sidebarSessions: [], + orchestrators: [], + workerOptions: [], + projectName: getReviewProjectName(projectFilter), + projects: getAllProjects(), + selectedProjectId: projectFilter === "all" ? undefined : projectFilter, + }; + + try { + const { config, sessionManager } = await getServices(); + const projectIds = + projectFilter === "all" + ? Object.keys(config.projects) + : config.projects[projectFilter] + ? [projectFilter] + : []; + const allSessions = await sessionManager.listCached(); + const visibleSessions = allSessions.filter((session) => projectIds.includes(session.projectId)); + const allSessionPrefixes = Object.entries(config.projects).map( + ([projectId, project]) => project.sessionPrefix ?? projectId, + ); + const workerSessionsById = new Map( + visibleSessions + .filter( + (session) => + !isOrchestratorSession( + session, + config.projects[session.projectId]?.sessionPrefix ?? session.projectId, + allSessionPrefixes, + ), + ) + .map((session) => [session.id, session]), + ); + const workerSessions = [...workerSessionsById.values()]; + + pageData.sidebarSessions = visibleSessions.map(sessionToDashboard); + const visibleSessionsById = new Map(visibleSessions.map((session) => [session.id, session])); + pageData.orchestrators = listDashboardOrchestrators(visibleSessions, config.projects).map( + (orchestrator) => { + const session = visibleSessionsById.get(orchestrator.id); + return { + ...orchestrator, + status: session?.status ?? null, + activity: session?.activity ?? null, + runtimeState: session?.lifecycle.runtime.state ?? null, + hasRuntime: session?.runtimeHandle !== null && session?.runtimeHandle !== undefined, + isTerminal: session ? isTerminalSession(session) : false, + isRestorable: session ? isRestorable(session) : false, + }; + }, + ); + pageData.workerOptions = workerSessions.map((session) => { + const project = config.projects[session.projectId]; + const title = + session.metadata["displayName"] ?? + session.metadata["issueTitle"] ?? + session.metadata["pinnedSummary"] ?? + session.agentInfo?.summary ?? + session.branch ?? + session.id; + return { + id: session.id, + projectId: session.projectId, + projectName: project?.name ?? session.projectId, + title, + branch: session.branch ?? null, + status: session.status, + activity: session.activity ?? null, + runtimeState: session.lifecycle.runtime.state, + hasRuntime: session.runtimeHandle !== null && session.runtimeHandle !== undefined, + prNumber: session.pr?.number ?? null, + prUrl: session.pr?.url ?? null, + }; + }); + + const runs: DashboardReviewRun[] = []; + + for (const projectId of projectIds) { + const project = config.projects[projectId]; + if (!project) continue; + const store = createCodeReviewStore(projectId); + const projectWorkers = workerSessions.filter((session) => session.projectId === projectId); + + for (const worker of projectWorkers) { + await markOutdatedCodeReviewRunsForSession({ store, session: worker }); + } + + runs.push( + ...store.listRunSummaries().map((run) => { + const worker = workerSessionsById.get(run.linkedSessionId); + return { + ...run, + projectName: project.name, + workerTitle: + worker?.metadata["displayName"] ?? + worker?.metadata["issueTitle"] ?? + worker?.metadata["pinnedSummary"] ?? + worker?.agentInfo?.summary ?? + null, + workerBranch: worker?.branch ?? null, + workerPrUrl: worker?.pr?.url ?? run.prUrl ?? null, + workerStatus: worker?.status ?? null, + workerActivity: worker?.activity ?? null, + workerRuntimeState: worker?.lifecycle.runtime.state ?? null, + workerHasRuntime: worker?.runtimeHandle !== null && worker?.runtimeHandle !== undefined, + }; + }), + ); + } + + pageData.runs = runs; + } catch (err) { + pageData.dashboardLoadError = formatReviewLoadError(err); + } + + return pageData; +} diff --git a/packages/web/src/lib/review-types.ts b/packages/web/src/lib/review-types.ts new file mode 100644 index 000000000..64af3640e --- /dev/null +++ b/packages/web/src/lib/review-types.ts @@ -0,0 +1,77 @@ +import type { CodeReviewRunSummary } from "@aoagents/ao-core"; + +export type ReviewBoardColumn = + | "queued" + | "reviewing" + | "triage" + | "waiting" + | "clean" + | "failed" + | "outdated"; + +export interface DashboardReviewRun extends CodeReviewRunSummary { + projectName: string; + workerTitle: string | null; + workerBranch: string | null; + workerPrUrl: string | null; + workerStatus: string | null; + workerActivity: string | null; + workerRuntimeState: string | null; + workerHasRuntime: boolean; +} + +export interface ReviewWorkerOption { + id: string; + projectId: string; + projectName: string; + title: string; + branch: string | null; + status: string; + activity: string | null; + runtimeState: string | null; + hasRuntime: boolean; + prNumber: number | null; + prUrl: string | null; +} + +export const REVIEW_BOARD_COLUMNS: ReviewBoardColumn[] = [ + "queued", + "reviewing", + "triage", + "waiting", + "clean", + "failed", + "outdated", +]; + +export const REVIEW_COLUMN_LABELS: Record = { + queued: "Queued", + reviewing: "Reviewing", + triage: "Triage", + waiting: "Waiting", + clean: "Clean", + failed: "Failed", + outdated: "Outdated", +}; + +export function getReviewBoardColumn(run: Pick): ReviewBoardColumn { + switch (run.status) { + case "queued": + case "preparing": + return "queued"; + case "running": + return "reviewing"; + case "needs_triage": + return "triage"; + case "sent_to_agent": + case "waiting_update": + return "waiting"; + case "clean": + return "clean"; + case "failed": + case "cancelled": + return "failed"; + case "outdated": + return "outdated"; + } +} diff --git a/packages/web/src/lib/routes.ts b/packages/web/src/lib/routes.ts index 22862ea99..44bfb8e35 100644 --- a/packages/web/src/lib/routes.ts +++ b/packages/web/src/lib/routes.ts @@ -2,6 +2,14 @@ export function projectDashboardPath(projectId: string): string { return `/projects/${encodeURIComponent(projectId)}`; } +export function projectDashboardSessionPath(projectId: string, sessionId: string): string { + return `${projectDashboardPath(projectId)}?session=${encodeURIComponent(sessionId)}`; +} + +export function projectReviewPath(projectId: string | undefined): string { + return projectId ? `/review?project=${encodeURIComponent(projectId)}` : "/review?project=all"; +} + export function projectSessionPath(projectId: string, sessionId: string): string { return `${projectDashboardPath(projectId)}/sessions/${encodeURIComponent(sessionId)}`; } diff --git a/packages/web/src/lib/types.ts b/packages/web/src/lib/types.ts index 85f93d225..82f7daad2 100644 --- a/packages/web/src/lib/types.ts +++ b/packages/web/src/lib/types.ts @@ -250,6 +250,12 @@ export interface DashboardOrchestratorLink { id: string; projectId: string; projectName: string; + status?: string | null; + activity?: string | null; + runtimeState?: string | null; + hasRuntime?: boolean; + isTerminal?: boolean; + isRestorable?: boolean; } /** @@ -403,30 +409,31 @@ export function isDashboardSessionDone(session: DashboardSession): boolean { return session.pr?.state === "merged"; } +function hasTerminalActivity(session: DashboardSession): boolean { + return session.activity !== null && TERMINAL_ACTIVITIES.has(session.activity); +} + export function isDashboardSessionTerminal(session: DashboardSession): boolean { if (session.lifecycle) { return ( isDashboardSessionDone(session) || session.lifecycle.runtimeState === "missing" || - session.lifecycle.runtimeState === "exited" + session.lifecycle.runtimeState === "exited" || + hasTerminalActivity(session) ); } - return ( - TERMINAL_STATUSES.has(session.status) || - (session.activity !== null && TERMINAL_ACTIVITIES.has(session.activity)) - ); + return TERMINAL_STATUSES.has(session.status) || hasTerminalActivity(session); } export function isDashboardRuntimeEnded(session: DashboardSession): boolean { if (session.lifecycle) { return ( - session.lifecycle.runtimeState === "missing" || session.lifecycle.runtimeState === "exited" + session.lifecycle.runtimeState === "missing" || + session.lifecycle.runtimeState === "exited" || + hasTerminalActivity(session) ); } - return ( - TERMINAL_STATUSES.has(session.status) || - (session.activity !== null && TERMINAL_ACTIVITIES.has(session.activity)) - ); + return TERMINAL_STATUSES.has(session.status) || hasTerminalActivity(session); } export function isDashboardSessionRestorable(session: DashboardSession): boolean { @@ -435,8 +442,13 @@ export function isDashboardSessionRestorable(session: DashboardSession): boolean session.lifecycle.sessionState === "done" || isDashboardSessionTerminated(session) || session.lifecycle.runtimeState === "missing" || - session.lifecycle.runtimeState === "exited"; - return terminalByCoreTruth && !NON_RESTORABLE_STATUSES.has(session.status); + session.lifecycle.runtimeState === "exited" || + hasTerminalActivity(session); + return ( + terminalByCoreTruth && + !NON_RESTORABLE_STATUSES.has(session.status) && + session.status !== "merged" + ); } return isDashboardSessionTerminal(session) && !NON_RESTORABLE_STATUSES.has(session.status); } From ee3fb5d33ae7c00c1737aa0807ea90519ba9773f Mon Sep 17 00:00:00 2001 From: Harshit Singh Bhandari Date: Tue, 19 May 2026 12:08:35 +0530 Subject: [PATCH 2/2] fix(web): make Done/Terminated dashboard section scrollable (#1923) (#1925) * fix(web): make Done/Terminated dashboard section scrollable (#1923) The collapsible Done/Terminated cards grid had no max-height or overflow handling, so it grew unbounded and was clipped by the ancestor's overflow:hidden once enough terminated sessions accumulated. The regression came from the Warm Terminal design refresh (#927) which added `flex: 1` to `.kanban-board-wrap`, defeating the body-level `overflow-y: auto` that previously allowed natural page scroll past the done section. Mirror the existing `.kanban-column-body` pattern by giving the cards grid its own internal scroll container and matching thin scrollbar styling. Each scrolling region owns its own scroll; the Warm Terminal flex layout is preserved. * style(web): document done-bar__cards max-height magic number Addresses greptile P2 review feedback on PR #1925. The 320px in calc(100vh - 320px) is the assumed chrome above the cards (dashboard header + subhead + body padding + done-bar toggle and margins). Adding a comment makes the implicit ancestor-height contract explicit, and points to the kanban-board rule that uses the same viewport-anchored pattern. --- .changeset/fix-done-bar-scroll.md | 5 +++++ packages/web/src/app/globals.css | 15 +++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 .changeset/fix-done-bar-scroll.md diff --git a/.changeset/fix-done-bar-scroll.md b/.changeset/fix-done-bar-scroll.md new file mode 100644 index 000000000..edac04f39 --- /dev/null +++ b/.changeset/fix-done-bar-scroll.md @@ -0,0 +1,5 @@ +--- +"@aoagents/ao-web": patch +--- + +Fix Done/Terminated section on the dashboard not scrolling. The expanded cards grid now has an internal scroll container (mirroring the kanban column body pattern), so older terminated sessions are reachable when many accumulate. diff --git a/packages/web/src/app/globals.css b/packages/web/src/app/globals.css index 1ae049ef9..d85a5c6c0 100644 --- a/packages/web/src/app/globals.css +++ b/packages/web/src/app/globals.css @@ -5858,6 +5858,21 @@ html.light .xterm .xterm-viewport:hover::-webkit-scrollbar-thumb:active { grid-template-columns: repeat(auto-fill, minmax(240px, 1fr)); gap: 8px; margin-top: 12px; + /* 320px ≈ dashboard header + subhead + body padding + done-bar toggle/margins. + Mirrors the .kanban-board viewport-anchored height pattern (line 5107). */ + max-height: calc(100vh - 320px); + overflow-y: auto; +} + +.done-bar__cards::-webkit-scrollbar { + width: 4px; +} +.done-bar__cards::-webkit-scrollbar-track { + background: transparent; +} +.done-bar__cards::-webkit-scrollbar-thumb { + background: var(--color-scrollbar); + border-radius: 0; } /* ── Done card (compact grid card) ──────────────────────────────────── */