diff --git a/apps/desktop/src/main/main.ts b/apps/desktop/src/main/main.ts index 9e5e89413..9e037b3ab 100644 --- a/apps/desktop/src/main/main.ts +++ b/apps/desktop/src/main/main.ts @@ -78,6 +78,7 @@ import { createAutomationService } from "./services/automations/automationServic import { createAutomationPlannerService } from "./services/automations/automationPlannerService"; import { createAutomationSecretService } from "./services/automations/automationSecretService"; import { createAutomationIngressService } from "./services/automations/automationIngressService"; +import { createReviewService } from "./services/review/reviewService"; import { createGithubPollingService } from "./services/automations/githubPollingService"; import type { AutomationAdeActionRegistry } from "./services/automations/automationService"; import { @@ -2334,6 +2335,23 @@ app.whenReady().then(async () => { onEvent: (event) => emitProjectEvent(projectRoot, IPC.automationsEvent, event), }); + const reviewService = createReviewService({ + db, + logger, + projectId, + projectRoot, + projectDefaultBranch: baseRef, + laneService, + gitService, + agentChatService, + sessionService, + sessionDeltaService, + testService, + issueInventoryService, + prService, + embeddingService, + onEvent: (event) => emitProjectEvent(projectRoot, IPC.reviewEvent, event), + }); const automationIngressService = createAutomationIngressService({ logger, automationService, @@ -3464,6 +3482,7 @@ app.whenReady().then(async () => { queueLandingService, issueInventoryService, prSummaryService, + reviewService, jobEngine, automationService, automationPlannerService, @@ -3572,6 +3591,7 @@ app.whenReady().then(async () => { queueLandingService: null, issueInventoryService: null, prSummaryService: null, + reviewService: null, jobEngine: null, automationService: null, automationPlannerService: null, @@ -3684,6 +3704,11 @@ app.whenReady().then(async () => { } catch { // ignore } + try { + ctx.reviewService?.dispose?.(); + } catch { + // ignore + } try { ctx.usageTrackingService?.dispose(); } catch { diff --git a/apps/desktop/src/main/services/ipc/registerIpc.ts b/apps/desktop/src/main/services/ipc/registerIpc.ts index 46e8c6a35..dafcf12bf 100644 --- a/apps/desktop/src/main/services/ipc/registerIpc.ts +++ b/apps/desktop/src/main/services/ipc/registerIpc.ts @@ -41,6 +41,11 @@ import type { AutomationSaveDraftResult, AutomationSimulateRequest, AutomationSimulateResult, + ReviewLaunchContext, + ReviewListRunsArgs, + ReviewRun, + ReviewRunDetail, + ReviewStartRunArgs, AdeActionRegistryEntry, AddMissionArtifactArgs, AddMissionInterventionArgs, @@ -534,6 +539,7 @@ import type { createPrPollingService } from "../prs/prPollingService"; import type { createQueueLandingService } from "../prs/queueLandingService"; import type { createIssueInventoryService } from "../prs/issueInventoryService"; import type { createPrSummaryService } from "../prs/prSummaryService"; +import type { createReviewService } from "../review/reviewService"; import type { createAgentChatService } from "../chat/agentChatService"; import type { createComputerUseArtifactBrokerService } from "../computerUse/computerUseArtifactBrokerService"; import { buildComputerUseOwnerSnapshot } from "../computerUse/controlPlane"; @@ -631,6 +637,7 @@ export type AppContext = { queueLandingService: ReturnType; issueInventoryService: ReturnType; prSummaryService: ReturnType; + reviewService: ReturnType; jobEngine: ReturnType; automationService: ReturnType; automationPlannerService: ReturnType; @@ -2785,6 +2792,56 @@ export function registerIpc({ return ctx.automationPlannerService.simulate(arg); }); + ipcMain.handle(IPC.reviewListLaunchContext, async (): Promise => { + const ctx = getCtx(); + return ctx.reviewService.listLaunchContext(); + }); + + ipcMain.handle(IPC.reviewListRuns, async (_event, arg: ReviewListRunsArgs = {}): Promise => { + const ctx = getCtx(); + return ctx.reviewService.listRuns(arg); + }); + + ipcMain.handle(IPC.reviewGetRunDetail, async (_event, arg: { runId: string }): Promise => { + const ctx = getCtx(); + return ctx.reviewService.getRunDetail({ runId: arg?.runId ?? "" }); + }); + + ipcMain.handle(IPC.reviewStartRun, async (_event, arg: ReviewStartRunArgs): Promise => { + const ctx = getCtx(); + return ctx.reviewService.startRun(arg); + }); + + ipcMain.handle(IPC.reviewRerun, async (_event, arg: { runId: string }): Promise => { + const ctx = getCtx(); + return ctx.reviewService.rerun(arg?.runId ?? ""); + }); + + ipcMain.handle(IPC.reviewCancelRun, async (_event, arg: { runId: string }) => { + const ctx = getCtx(); + return ctx.reviewService.cancelRun({ runId: arg?.runId ?? "" }); + }); + + ipcMain.handle(IPC.reviewRecordFeedback, async (_event, arg: import("../../../shared/types").ReviewRecordFeedbackArgs) => { + const ctx = getCtx(); + return ctx.reviewService.recordFeedback(arg); + }); + + ipcMain.handle(IPC.reviewListSuppressions, async (_event, arg: import("../../../shared/types").ReviewListSuppressionsArgs | undefined) => { + const ctx = getCtx(); + return ctx.reviewService.listSuppressions(arg ?? {}); + }); + + ipcMain.handle(IPC.reviewDeleteSuppression, async (_event, arg: { suppressionId: string }) => { + const ctx = getCtx(); + return ctx.reviewService.deleteSuppression({ suppressionId: arg?.suppressionId ?? "" }); + }); + + ipcMain.handle(IPC.reviewQualityReport, async () => { + const ctx = getCtx(); + return ctx.reviewService.qualityReport(); + }); + ipcMain.handle(IPC.adeActionsListRegistry, async (): Promise => { const ctx = getCtx(); const services = getAdeActionDomainServices(ctx as unknown as AdeRuntime); diff --git a/apps/desktop/src/main/services/lanes/laneLaunchContext.test.ts b/apps/desktop/src/main/services/lanes/laneLaunchContext.test.ts index 9f631de0b..ecd5d16a5 100644 --- a/apps/desktop/src/main/services/lanes/laneLaunchContext.test.ts +++ b/apps/desktop/src/main/services/lanes/laneLaunchContext.test.ts @@ -156,6 +156,29 @@ describe("resolveLaneLaunchContext", () => { }); }); + describe("happy path: explicit absolute cwd outside worktree", () => { + it("allows an external absolute cwd when the caller opts in", () => { + mocks.statSync.mockReturnValue({ isDirectory: () => true }); + mocks.realpathSync + .mockReturnValueOnce("/real/lane/root") + .mockReturnValueOnce("/real/project/root"); + + const result = resolveLaneLaunchContext({ + laneService: makeLaneService("/projects/my-lane"), + laneId: "lane-1", + requestedCwd: "/real/project/root", + allowExternalCwd: true, + purpose: "start agent", + }); + + expect(result).toEqual({ + laneWorktreePath: "/real/lane/root", + cwd: "/real/project/root", + }); + expect(mocks.resolvePathWithinRoot).not.toHaveBeenCalled(); + }); + }); + describe("error: lane has no worktree configured", () => { it("throws when worktreePath is empty string", () => { expect(() => diff --git a/apps/desktop/src/main/services/lanes/laneLaunchContext.ts b/apps/desktop/src/main/services/lanes/laneLaunchContext.ts index 406a3a37b..7ff5d31c4 100644 --- a/apps/desktop/src/main/services/lanes/laneLaunchContext.ts +++ b/apps/desktop/src/main/services/lanes/laneLaunchContext.ts @@ -29,6 +29,7 @@ export function resolveLaneLaunchContext(args: { laneService: ReturnType; laneId: string; requestedCwd?: string | null; + allowExternalCwd?: boolean; purpose: string; }): LaneLaunchContext { const laneId = String(args.laneId ?? "").trim(); @@ -51,6 +52,16 @@ export function resolveLaneLaunchContext(args: { }; } + if (args.allowExternalCwd === true && path.isAbsolute(requestedCwd)) { + return { + laneWorktreePath: laneRoot, + cwd: ensureDirectoryExists( + path.resolve(requestedCwd), + `Requested cwd '${requestedCwd}' is not an existing directory.`, + ), + }; + } + const requestedTarget = path.isAbsolute(requestedCwd) ? requestedCwd : path.resolve(laneRoot, requestedCwd); diff --git a/apps/desktop/src/main/services/lanes/laneService.ts b/apps/desktop/src/main/services/lanes/laneService.ts index cdfd88e60..0b8cb2ccd 100644 --- a/apps/desktop/src/main/services/lanes/laneService.ts +++ b/apps/desktop/src/main/services/lanes/laneService.ts @@ -2532,6 +2532,11 @@ export function createLaneService({ db.run("delete from pr_pipeline_settings where pr_id in (select id from pull_requests where lane_id = ? and project_id = ?)", [laneId, projectId]); db.run("delete from pr_issue_inventory where pr_id in (select id from pull_requests where lane_id = ? and project_id = ?)", [laneId, projectId]); db.run("delete from pull_requests where lane_id = ? and project_id = ?", [laneId, projectId]); + db.run("delete from review_run_publications where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); + db.run("delete from review_finding_feedback where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); + db.run("delete from review_findings where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); + db.run("delete from review_run_artifacts where run_id in (select id from review_runs where lane_id = ? and project_id = ?)", [laneId, projectId]); + db.run("delete from review_runs where lane_id = ? and project_id = ?", [laneId, projectId]); db.run("delete from session_deltas where lane_id = ?", [laneId]); db.run("delete from terminal_sessions where lane_id = ?", [laneId]); db.run("delete from operations where lane_id = ?", [laneId]); diff --git a/apps/desktop/src/main/services/processes/processService.test.ts b/apps/desktop/src/main/services/processes/processService.test.ts index a38f340c5..c838821d4 100644 --- a/apps/desktop/src/main/services/processes/processService.test.ts +++ b/apps/desktop/src/main/services/processes/processService.test.ts @@ -212,6 +212,7 @@ describe("processService PTY-backed run commands", () => { try { await service.start({ laneId: "lane-env", processId: "print-env" }); expect(ptyService.create).toHaveBeenCalledWith(expect.objectContaining({ + allowNewSessionId: true, env: expect.objectContaining({ PORT: "3001", PORT_RANGE_START: "3001", @@ -227,6 +228,65 @@ describe("processService PTY-backed run commands", () => { } }); + it("allows a managed process to use an explicit absolute cwd outside the lane worktree", async () => { + const projectRoot = fs.mkdtempSync(path.join(os.tmpdir(), "ade-process-absolute-cwd-")); + const laneRoot = path.join(projectRoot, ".ade", "worktrees", "lane-absolute"); + fs.mkdirSync(laneRoot, { recursive: true }); + const dbPath = path.join(projectRoot, "kv.sqlite"); + const projectId = "proj-absolute-cwd"; + const logger = createLogger(); + const db = await openKvDb(dbPath, createLogger()); + const now = "2026-03-24T12:00:00.000Z"; + const { ptyService, sessionService } = createPtyHarness(projectRoot); + + db.run( + "insert into projects(id, root_path, display_name, default_base_ref, created_at, last_opened_at) values (?, ?, ?, ?, ?, ?)", + [projectId, projectRoot, "test", "main", now, now], + ); + db.run( + `insert into lanes( + id, project_id, name, description, lane_type, base_ref, branch_ref, worktree_path, + attached_root_path, is_edit_protected, parent_lane_id, color, icon, tags_json, status, created_at, archived_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + ["lane-absolute", projectId, "Lane Absolute", null, "worktree", "main", "feature/absolute", laneRoot, null, 0, null, null, null, null, "active", now, null], + ); + + const config = makeMinimalConfig([ + { id: "absolute-proc", command: ["scripts/dogfood.sh", "code-review"], cwd: projectRoot }, + ]); + + const service = createProcessService({ + db, + projectId, + logger, + laneService: { + getLaneWorktreePath: () => laneRoot, + list: async () => [makeLaneSummary(laneRoot, "lane-absolute")], + } as any, + projectConfigService: { + get: () => config, + getEffective: () => config.effective, + getExecutableConfig: () => config.effective, + } as any, + sessionService, + ptyService, + broadcastEvent: () => {}, + }); + + try { + const resolvedProjectRoot = fs.realpathSync(projectRoot); + await service.start({ laneId: "lane-absolute", processId: "absolute-proc" }); + expect(ptyService.create).toHaveBeenCalledWith(expect.objectContaining({ + allowExternalCwd: true, + cwd: resolvedProjectRoot, + })); + } finally { + service.disposeAll(); + db.close(); + fs.rmSync(projectRoot, { recursive: true, force: true }); + } + }); + it("includes envPath and envShell in the process.start log entry", async () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "ade-process-startlog-")); const dbPath = path.join(tmpDir, "kv.sqlite"); diff --git a/apps/desktop/src/main/services/processes/processService.ts b/apps/desktop/src/main/services/processes/processService.ts index 0786b6b8b..515597a81 100644 --- a/apps/desktop/src/main/services/processes/processService.ts +++ b/apps/desktop/src/main/services/processes/processService.ts @@ -582,17 +582,32 @@ export function createProcessService({ } const laneRoot = laneService.getLaneWorktreePath(laneId); - const configuredCwd = opts.overlay?.cwd?.trim() ? opts.overlay.cwd : definition.cwd; - const cwdCandidate = path.isAbsolute(configuredCwd) ? configuredCwd : path.join(laneRoot, configuredCwd); + const configuredCwd = opts.overlay?.cwd?.trim() ? opts.overlay.cwd.trim() : definition.cwd.trim(); + // Absolute cwd is intentionally allowed: ProcessDefinition.cwd and LaneOverlayOverrides.cwd + // originate from the trusted project config (same file that already declares the command + // to execute), not from UI/IPC input. Relative paths remain sandboxed to the lane worktree. + const allowExternalCwd = path.isAbsolute(configuredCwd); let cwd: string; - try { - cwd = resolvePathWithinRoot(laneRoot, cwdCandidate); - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - if (message.includes("Path does not exist")) { + if (allowExternalCwd) { + try { + const resolved = path.resolve(configuredCwd); + const stat = fs.statSync(resolved); + if (!stat.isDirectory()) throw new Error("Path is not a directory"); + cwd = fs.realpathSync(resolved); + } catch { throw new Error(`Process '${definition.id}' cwd does not exist: ${configuredCwd}`); } - throw new Error(`Process '${definition.id}' cwd must stay within the lane workspace`); + } else { + const cwdCandidate = path.join(laneRoot, configuredCwd); + try { + cwd = resolvePathWithinRoot(laneRoot, cwdCandidate); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + if (message.includes("Path does not exist")) { + throw new Error(`Process '${definition.id}' cwd does not exist: ${configuredCwd}`); + } + throw new Error(`Process '${definition.id}' cwd must stay within the lane workspace`); + } } const laneRuntimeEnv = (await getLaneRuntimeEnv?.(laneId)) ?? {}; @@ -614,6 +629,8 @@ export function createProcessService({ try { const result = await ptyService.create({ sessionId, + allowNewSessionId: true, + allowExternalCwd, laneId, cwd, cols: 120, diff --git a/apps/desktop/src/main/services/prs/prService.reviewPublication.test.ts b/apps/desktop/src/main/services/prs/prService.reviewPublication.test.ts new file mode 100644 index 000000000..261e1f6f7 --- /dev/null +++ b/apps/desktop/src/main/services/prs/prService.reviewPublication.test.ts @@ -0,0 +1,221 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createPrService } from "./prService"; + +function makeLogger() { + return { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + } as any; +} + +function makeMockDb() { + return { + get: vi.fn((sql: string) => { + if (!sql.includes("from pull_requests")) return null; + return { + id: "pr-80", + lane_id: "lane-1", + project_id: "proj-1", + repo_owner: "test-owner", + repo_name: "test-repo", + github_pr_number: 80, + github_url: "https://github.com/test-owner/test-repo/pull/80", + github_node_id: "PR_kwDOExample", + title: "Review publication", + state: "open", + base_branch: "main", + head_branch: "feature/pr-80", + checks_status: "passing", + review_status: "commented", + additions: 2, + deletions: 0, + last_synced_at: "2026-04-06T10:00:00.000Z", + created_at: "2026-04-06T09:55:00.000Z", + updated_at: "2026-04-06T10:00:00.000Z", + }; + }), + all: vi.fn(() => []), + run: vi.fn(), + } as any; +} + +function makeLaneService() { + return { + list: vi.fn(async () => []), + getLaneBaseAndBranch: vi.fn(), + } as any; +} + +describe("prService.publishReviewPublication", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("posts anchored findings inline and routes unanchored findings into the review summary", async () => { + const apiRequest = vi.fn(async ({ method, path, body }: { method: string; path: string; body?: Record }) => { + if (method === "GET" && path === "/repos/test-owner/test-repo/pulls/80") { + return { + data: { + number: 80, + html_url: "https://github.com/test-owner/test-repo/pull/80", + node_id: "PR_kwDOExample", + title: "Review publication", + state: "open", + draft: false, + merged_at: null, + head: { ref: "feature/pr-80", sha: "def456789012" }, + base: { ref: "main", sha: "abc123456789" }, + additions: 2, + deletions: 0, + }, + }; + } + if (method === "GET" && path === "/repos/test-owner/test-repo/pulls/80/files") { + return { + data: [ + { + filename: "src/review.ts", + status: "modified", + additions: 2, + deletions: 0, + patch: "@@ -10,1 +10,3 @@\n context\n+anchored\n+summary only\n", + previous_filename: null, + }, + ], + }; + } + if (method === "GET" && path === "/repos/test-owner/test-repo/commits/def456789012/status") { + return { data: { state: "success", statuses: [] } }; + } + if (method === "GET" && path === "/repos/test-owner/test-repo/commits/def456789012/check-runs") { + return { data: { check_runs: [] } }; + } + if (method === "GET" && path === "/repos/test-owner/test-repo/pulls/80/reviews") { + return { data: [] }; + } + if (method === "POST" && path === "/repos/test-owner/test-repo/pulls/80/reviews") { + return { + data: { + id: 123, + html_url: "https://github.com/test-owner/test-repo/pull/80#pullrequestreview-123", + }, + }; + } + throw new Error(`Unexpected request: ${method} ${path} ${JSON.stringify(body ?? {})}`); + }); + + const service = createPrService({ + db: makeMockDb(), + logger: makeLogger(), + projectId: "proj-1", + projectRoot: "/tmp/test-project", + laneService: makeLaneService(), + operationService: {} as any, + githubService: { + apiRequest, + getRepoOrThrow: vi.fn(), + getStatus: vi.fn(), + setToken: vi.fn(), + clearToken: vi.fn(), + getTokenOrThrow: vi.fn(() => "ghp_mock"), + } as any, + projectConfigService: { get: vi.fn(() => ({ effective: { ai: {} } })) } as any, + openExternal: vi.fn(async () => undefined), + }); + + const publication = await service.publishReviewPublication({ + runId: "run-1", + destination: { + kind: "github_pr_review", + prId: "pr-80", + repoOwner: "test-owner", + repoName: "test-repo", + prNumber: 80, + githubUrl: "https://github.com/test-owner/test-repo/pull/80", + }, + targetLabel: "PR #80 feature/pr-80 -> main", + summary: "One finding can anchor, one cannot.", + findings: [ + { + id: "finding-inline", + runId: "run-1", + title: "Anchored finding", + severity: "high", + body: "This should post inline.", + confidence: 0.9, + evidence: [], + filePath: "src/review.ts", + line: 11, + anchorState: "anchored", + sourcePass: "adjudicated", + publicationState: "local_only", + originatingPasses: ["diff-risk", "cross-file-impact"], + adjudication: { + score: 8.2, + candidateCount: 2, + mergedFindingIds: ["raw-1", "raw-2"], + rationale: "Merged overlapping findings from diff-risk and cross-file-impact.", + publicationEligible: true, + }, + }, + { + id: "finding-summary", + runId: "run-1", + title: "Summary finding", + severity: "medium", + body: "This should stay in the top-level review body.", + confidence: 0.6, + evidence: [], + filePath: "src/review.ts", + line: 200, + anchorState: "file_only", + sourcePass: "adjudicated", + publicationState: "local_only", + originatingPasses: ["checks-and-tests"], + adjudication: { + score: 5.7, + candidateCount: 1, + mergedFindingIds: ["raw-3"], + rationale: "Accepted because the finding carried concrete evidence and cleared the adjudication threshold.", + publicationEligible: true, + }, + }, + ], + changedFiles: [ + { + filePath: "src/review.ts", + diffPositionsByLine: { 11: 2 }, + }, + ], + }); + + expect(publication.status).toBe("published"); + expect(publication.inlineComments).toEqual([ + expect.objectContaining({ + findingId: "finding-inline", + path: "src/review.ts", + line: 11, + position: 2, + }), + ]); + expect(publication.summaryFindingIds).toEqual(["finding-summary"]); + + const postCall = apiRequest.mock.calls.find( + ([request]: [{ method: string; path: string }]) => request.method === "POST" && request.path.endsWith("/reviews"), + )?.[0]; + expect(postCall?.body).toEqual(expect.objectContaining({ + event: "COMMENT", + commit_id: "def456789012", + comments: [ + expect.objectContaining({ + path: "src/review.ts", + position: 2, + }), + ], + })); + expect(String(postCall?.body?.body ?? "")).toContain("Summary finding"); + expect(String(postCall?.body?.body ?? "")).toContain("Anchored inline comments posted: 1."); + }); +}); diff --git a/apps/desktop/src/main/services/prs/prService.ts b/apps/desktop/src/main/services/prs/prService.ts index 452258ac5..7edc0cdc4 100644 --- a/apps/desktop/src/main/services/prs/prService.ts +++ b/apps/desktop/src/main/services/prs/prService.ts @@ -80,6 +80,7 @@ import type { SetPrLabelsArgs, RequestPrReviewersArgs, SubmitPrReviewArgs, + SubmitPrReviewResult, ClosePrArgs, ReopenPrArgs, RerunPrChecksArgs, @@ -89,6 +90,7 @@ import type { GitHubPrSnapshot, PrDetail, PrFile, + PrReviewSnapshot, PrActionRun, PrActionJob, PrActionStep, @@ -106,6 +108,10 @@ import type { SetPrReviewThreadResolvedResult, ReactToPrCommentArgs, PrReactionContent, + ReviewFinding, + ReviewPublication, + ReviewPublicationDestination, + ReviewPublicationInlineComment, } from "../../../shared/types"; import type { AdeDb } from "../state/kvDb"; import type { Logger } from "../logging/logger"; @@ -269,6 +275,64 @@ function createEmptyIntegrationResolutionState(integrationLaneId: string, update }; } +function buildPublishedReviewCommentBody(finding: ReviewFinding): string { + const sections = [ + `[${finding.severity.toUpperCase()}] ${finding.title}`, + "", + finding.body, + "", + `Confidence: ${Math.round(finding.confidence * 100)}%`, + ]; + const quote = finding.evidence.find((entry) => typeof entry.quote === "string" && entry.quote.trim().length > 0)?.quote?.trim() ?? null; + if (quote) { + sections.push("", "```", quote.slice(0, 1_200), "```"); + } + return sections.join("\n"); +} + +function formatPublishedFindingLocation(finding: ReviewFinding): string { + if (finding.filePath && finding.line != null) return `${finding.filePath}:${finding.line}`; + if (finding.filePath) return finding.filePath; + return "general"; +} + +function buildPublishedReviewSummaryBody(args: { + targetLabel: string; + summary: string | null; + inlineFindings: ReviewFinding[]; + summaryFindings: ReviewFinding[]; +}): string { + const lines = [ + "## ADE review", + "", + `Target: ${args.targetLabel}`, + "", + args.summary?.trim() || (args.inlineFindings.length + args.summaryFindings.length > 0 + ? `ADE found ${args.inlineFindings.length + args.summaryFindings.length} actionable finding(s).` + : "ADE found no actionable findings."), + ]; + + if (args.inlineFindings.length > 0) { + lines.push("", `Anchored inline comments posted: ${args.inlineFindings.length}.`); + } + + if (args.summaryFindings.length > 0) { + lines.push("", "### Findings kept in the summary"); + for (const finding of args.summaryFindings) { + lines.push( + `- [${finding.severity}] ${finding.title} (${formatPublishedFindingLocation(finding)})`, + ` ${finding.body}`, + ); + } + } + + if (args.inlineFindings.length === 0 && args.summaryFindings.length === 0) { + lines.push("", "No actionable findings."); + } + + return lines.join("\n").trim(); +} + async function readIntegrationLaneSnapshot(worktreePath: string): Promise { if (!worktreePath || !fs.existsSync(worktreePath)) return null; const [headRes, statusRes] = await Promise.all([ @@ -1972,6 +2036,85 @@ export function createPrService({ })); }; + const getReviewSnapshot = async (prId: string): Promise => { + const row = requireRow(prId); + const repo = repoFromRow(row); + const [pr, files] = await Promise.all([ + fetchPr(repo, Number(row.github_pr_number)), + getFilesSnapshot(prId), + ]); + + return { + ...rowToSummary(row), + baseBranch: asString(pr?.base?.ref) || row.base_branch, + headBranch: asString(pr?.head?.ref) || row.head_branch, + githubUrl: asString(pr?.html_url) || row.github_url, + githubNodeId: asString(pr?.node_id) || row.github_node_id, + title: asString(pr?.title) || row.title || "", + additions: Number(pr?.additions ?? row.additions ?? 0), + deletions: Number(pr?.deletions ?? row.deletions ?? 0), + baseSha: asString(pr?.base?.sha) || null, + headSha: asString(pr?.head?.sha) || null, + files, + }; + }; + + const submitReviewRequest = async ( + args: SubmitPrReviewArgs, + options: { + commitSha?: string | null; + } = {}, + ): Promise => { + const row = requireRow(args.prId); + const repo = repoFromRow(row); + const inlineComments = Array.isArray(args.comments) + ? args.comments.filter((comment) => { + const path = asString(comment?.path).trim(); + const body = asString(comment?.body).trim(); + const position = Number(comment?.position); + return path.length > 0 && body.length > 0 && Number.isFinite(position) && position > 0; + }) + : []; + + let commitSha = asString(options.commitSha).trim() || null; + if (inlineComments.length > 0 && !commitSha) { + commitSha = (await getReviewSnapshot(args.prId)).headSha; + } + if (inlineComments.length > 0 && !commitSha) { + throw new Error("PR head commit is unavailable for inline review comments."); + } + + const { data } = await githubService.apiRequest({ + method: "POST", + path: `/repos/${repo.owner}/${repo.name}/pulls/${Number(row.github_pr_number)}/reviews`, + body: { + event: args.event, + body: args.body ?? "", + ...(commitSha ? { commit_id: commitSha } : {}), + ...(inlineComments.length > 0 + ? { + comments: inlineComments.map((comment) => ({ + path: comment.path, + body: comment.body, + position: comment.position, + })), + } + : {}), + }, + }); + + markHotRefresh([args.prId]); + await refreshOne(args.prId); + + return { + id: Number.isFinite(Number(data?.id)) ? String(Number(data.id)) : null, + nodeId: asString(data?.node_id) || null, + htmlUrl: asString(data?.html_url) || null, + state: asString(data?.state) || null, + submittedAt: asString(data?.submitted_at) || null, + }; + }; + /** * Aggregate per-commit check runs into the compact `PrCommit.checkStatus` * discriminator ("success" | "failure" | "pending" | "none"). Mirrors @@ -4902,6 +5045,8 @@ export function createPrService({ return laneId ? summaries.filter((pr) => pr.laneId === laneId) : summaries; }, + getReviewSnapshot, + async refresh(args: { prId?: string; prIds?: string[] } = {}): Promise { const requestedPrIds = [ ...(args.prId ? [args.prId] : []), @@ -5720,16 +5865,117 @@ export function createPrService({ await refreshOne(args.prId); }, - async submitReview(args: SubmitPrReviewArgs): Promise { - const row = requireRow(args.prId); - const repo = repoFromRow(row); - await githubService.apiRequest({ - method: "POST", - path: `/repos/${repo.owner}/${repo.name}/pulls/${Number(row.github_pr_number)}/reviews`, - body: { event: args.event, body: args.body ?? "" } + async submitReview(args: SubmitPrReviewArgs): Promise { + return await submitReviewRequest(args); + }, + + async publishReviewPublication(args: { + runId: string; + destination: ReviewPublicationDestination; + targetLabel: string; + summary: string | null; + findings: ReviewFinding[]; + changedFiles: Array<{ filePath: string; diffPositionsByLine: Record }>; + }): Promise { + if (args.destination.kind !== "github_pr_review") { + throw new Error(`Unsupported review publication destination: ${args.destination.kind}`); + } + + const snapshot = await getReviewSnapshot(args.destination.prId); + const requestedAt = nowIso(); + const changedFilesByPath = new Map( + args.changedFiles.map((file) => [file.filePath, file.diffPositionsByLine] as const), + ); + const inlineFindings: ReviewFinding[] = []; + const summaryFindings: ReviewFinding[] = []; + const inlineComments: ReviewPublicationInlineComment[] = []; + + for (const finding of args.findings) { + const linePositions = finding.filePath ? changedFilesByPath.get(finding.filePath) : null; + const diffPosition = finding.line != null && linePositions + ? Number(linePositions[finding.line] ?? NaN) + : Number.NaN; + + if ( + finding.anchorState === "anchored" + && finding.filePath + && finding.line != null + && Number.isFinite(diffPosition) + && diffPosition > 0 + ) { + inlineFindings.push(finding); + inlineComments.push({ + findingId: finding.id, + path: finding.filePath, + line: finding.line, + position: diffPosition, + body: buildPublishedReviewCommentBody(finding), + }); + continue; + } + summaryFindings.push(finding); + } + + const summaryBody = buildPublishedReviewSummaryBody({ + targetLabel: args.targetLabel, + summary: args.summary, + inlineFindings, + summaryFindings, }); - markHotRefresh([args.prId]); - await refreshOne(args.prId); + + try { + const result = await submitReviewRequest( + { + prId: args.destination.prId, + event: "COMMENT", + body: summaryBody, + comments: inlineComments.map((comment) => ({ + path: comment.path, + position: comment.position, + body: comment.body, + })), + }, + { + commitSha: snapshot.headSha, + }, + ); + + const completedAt = nowIso(); + return { + id: randomUUID(), + runId: args.runId, + destination: args.destination, + reviewEvent: "COMMENT", + status: "published", + reviewUrl: result.htmlUrl, + remoteReviewId: result.id || result.nodeId, + summaryBody, + inlineComments, + summaryFindingIds: summaryFindings.map((finding) => finding.id), + errorMessage: null, + createdAt: requestedAt, + updatedAt: completedAt, + completedAt, + }; + } catch (error) { + const completedAt = nowIso(); + return { + id: randomUUID(), + runId: args.runId, + destination: args.destination, + reviewEvent: "COMMENT", + status: "failed", + reviewUrl: null, + remoteReviewId: null, + summaryBody, + inlineComments, + summaryFindingIds: summaryFindings.map((finding) => finding.id), + errorMessage: getErrorMessage(error), + createdAt: requestedAt, + updatedAt: completedAt, + completedAt, + }; + } }, async closePr(args: ClosePrArgs): Promise { diff --git a/apps/desktop/src/main/services/pty/ptyService.test.ts b/apps/desktop/src/main/services/pty/ptyService.test.ts index 57ed6d025..4a5c6878f 100644 --- a/apps/desktop/src/main/services/pty/ptyService.test.ts +++ b/apps/desktop/src/main/services/pty/ptyService.test.ts @@ -541,6 +541,25 @@ describe("ptyService", () => { expect(loadPty).not.toHaveBeenCalled(); }); + it("allows an explicit absolute cwd outside the selected lane when opted in", async () => { + mocks.existsSyncResults.set("/tmp/outside", true); + const { service, loadPty } = createHarness(); + await service.create({ + laneId: "lane-1", + cwd: "/tmp/outside", + allowExternalCwd: true, + title: "External cwd terminal", + cols: 80, + rows: 24, + }); + const spawnCall = loadPty.mock.results[0].value.spawn; + expect(spawnCall).toHaveBeenCalledWith( + expect.any(String), + expect.any(Array), + expect.objectContaining({ cwd: "/tmp/outside" }), + ); + }); + it("rejects a cwd whose realpath hops outside the lane worktree", async () => { const childPath = "/tmp/test-worktree/hop-child"; mocks.existsSyncResults.set(childPath, true); @@ -712,6 +731,46 @@ describe("ptyService", () => { expect(sessionService.create).toHaveBeenCalledTimes(createCallsBeforeResume); }); + it("preserves the strict resume path when a requested session id does not exist", async () => { + const { service } = createHarness(); + + await expect(service.create({ + sessionId: "session-missing", + laneId: "lane-1", + title: "Codex CLI", + cols: 80, + rows: 24, + toolType: "codex", + startupCommand: "codex --no-alt-screen resume thread-existing", + })).rejects.toThrow(/was not found/i); + }); + + it("creates a new tracked session when the caller explicitly pre-assigns a fresh session id", async () => { + const { service, sessionService } = createHarness(); + + const result = await service.create({ + sessionId: "session-process-1", + allowNewSessionId: true, + laneId: "lane-1", + title: "Run process", + cols: 80, + rows: 24, + toolType: "run-shell", + command: "npm", + args: ["run", "dev"], + }); + + expect(result.sessionId).toBe("session-process-1"); + expect(sessionService.create).toHaveBeenCalledWith( + expect.objectContaining({ + sessionId: "session-process-1", + title: "Run process", + toolType: "run-shell", + }), + ); + expect(sessionService.reattach).not.toHaveBeenCalled(); + }); + it("reuses an already-live PTY when resume is requested twice for the same tracked session", async () => { const { service, sessionService, logger } = createHarness(); sessionService.create({ diff --git a/apps/desktop/src/main/services/pty/ptyService.ts b/apps/desktop/src/main/services/pty/ptyService.ts index 2b3af7710..74623c3de 100644 --- a/apps/desktop/src/main/services/pty/ptyService.ts +++ b/apps/desktop/src/main/services/pty/ptyService.ts @@ -1002,15 +1002,22 @@ export function createPtyService({ laneService, laneId, requestedCwd: args.cwd, + allowExternalCwd: args.allowExternalCwd === true, purpose: "start a terminal session", }); const { laneWorktreePath: worktreePath, cwd } = launchContext; const { cols, rows } = clampDims(args.cols, args.rows); const requestedSessionId = typeof args.sessionId === "string" ? args.sessionId.trim() : ""; + const allowNewSessionId = args.allowNewSessionId === true; + const isResumeAttempt = + typeof args.startupCommand === "string" && args.startupCommand.trim().length > 0; const existingSession = requestedSessionId.length ? sessionService.get(requestedSessionId) : null; + if (requestedSessionId.length && !existingSession && isResumeAttempt && !allowNewSessionId) { + throw new Error(`Terminal session '${requestedSessionId}' was not found.`); + } if (existingSession && existingSession.laneId !== laneId) { throw new Error(`Terminal session '${requestedSessionId}' belongs to lane '${existingSession.laneId}', not '${laneId}'.`); } diff --git a/apps/desktop/src/main/services/review/reviewContextBuilder.test.ts b/apps/desktop/src/main/services/review/reviewContextBuilder.test.ts new file mode 100644 index 000000000..24b74119b --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewContextBuilder.test.ts @@ -0,0 +1,540 @@ +import path from "node:path"; +import { createRequire } from "node:module"; +import initSqlJs from "sql.js"; +import type { Database, SqlJsStatic } from "sql.js"; +import { beforeAll, describe, expect, it, vi } from "vitest"; +import { createReviewContextBuilder } from "./reviewContextBuilder"; + +type SqlValue = string | number | null | Uint8Array; +type AdeDb = { + run: (sql: string, params?: SqlValue[]) => void; + get: = Record>(sql: string, params?: SqlValue[]) => T | null; + all: = Record>(sql: string, params?: SqlValue[]) => T[]; +}; + +function mapExecRows(rows: { columns: string[]; values: unknown[][] }[]): Record[] { + const first = rows[0]; + if (!first) return []; + return first.values.map((row) => { + const out: Record = {}; + first.columns.forEach((column, index) => { + out[column] = row[index]; + }); + return out; + }); +} + +let SQL: SqlJsStatic; + +beforeAll(async () => { + const require = createRequire(import.meta.url); + const wasmPath = require.resolve("sql.js/dist/sql-wasm.wasm"); + const wasmDir = path.dirname(wasmPath); + SQL = await initSqlJs({ + locateFile: (file) => path.join(wasmDir, file), + }); +}); + +function createInMemoryAdeDb(): { db: AdeDb; raw: Database } { + const raw = new SQL.Database(); + raw.run(` + create table lanes( + id text primary key, + project_id text not null, + mission_id text + ); + `); + raw.run(` + create table missions( + id text primary key, + project_id text not null, + title text, + prompt text, + status text, + outcome_summary text, + updated_at text + ); + `); + raw.run(` + create table orchestrator_worker_digests( + id text primary key, + project_id text not null, + mission_id text not null, + run_id text not null, + step_id text not null, + attempt_id text not null, + lane_id text, + session_id text, + step_key text, + status text not null, + summary text not null, + files_changed_json text, + tests_run_json text, + warnings_json text, + tokens_json text, + cost_usd real, + suggested_next_actions_json text, + created_at text not null + ); + `); + raw.run(` + create table pull_requests( + id text primary key, + project_id text not null, + lane_id text not null, + repo_owner text not null, + repo_name text not null, + github_pr_number integer not null, + github_url text not null, + title text, + state text not null, + updated_at text not null + ); + `); + raw.run(` + create table review_runs( + id text primary key, + project_id text not null, + lane_id text not null, + summary text, + status text not null, + finding_count integer not null default 0, + created_at text not null + ); + `); + raw.run(` + create table review_findings( + id text primary key, + run_id text not null, + file_path text + ); + `); + raw.run(` + create table review_run_publications( + id text primary key, + run_id text not null + ); + `); + + const run = (sql: string, params: SqlValue[] = []) => raw.run(sql, params); + const all = = Record>(sql: string, params: SqlValue[] = []): T[] => + mapExecRows(raw.exec(sql, params)) as T[]; + const get = = Record>(sql: string, params: SqlValue[] = []): T | null => + all(sql, params)[0] ?? null; + return { raw, db: { run, all, get } }; +} + +function makeRun() { + return { + id: "run-current", + projectId: "project-1", + laneId: "lane-review", + target: { mode: "lane_diff", laneId: "lane-review" }, + config: { + compareAgainst: { kind: "default_branch" }, + selectionMode: "full_diff", + dirtyOnly: false, + modelId: "openai/gpt-5.4-codex", + reasoningEffort: "medium", + budgets: { + maxFiles: 60, + maxDiffChars: 180_000, + maxPromptChars: 220_000, + maxFindings: 12, + maxFindingsPerPass: 6, + maxPublishedFindings: 6, + }, + publishBehavior: "local_only", + }, + targetLabel: "lane-review vs main", + compareTarget: null, + status: "running", + summary: null, + errorMessage: null, + findingCount: 0, + severitySummary: { critical: 0, high: 0, medium: 0, low: 0, info: 0 }, + chatSessionId: null, + createdAt: "2026-04-06T10:00:00.000Z", + startedAt: "2026-04-06T10:00:00.000Z", + endedAt: null, + updatedAt: "2026-04-06T10:00:00.000Z", + } as const; +} + +function makeMaterialized(changedPaths: string[]) { + return { + targetLabel: "lane-review vs main", + compareTarget: null, + publicationTarget: null, + fullPatchText: "diff --git a/file b/file", + changedFiles: changedPaths.map((filePath) => ({ + filePath, + excerpt: `@@ -1 +1 @@\n+${filePath}`, + lineNumbers: [1], + diffPositionsByLine: { 1: 1 }, + })), + artifacts: [], + }; +} + +describe("reviewContextBuilder", () => { + it("builds a bounded compact packet from ADE-native provenance, rules, and validation signals", async () => { + const { db } = createInMemoryAdeDb(); + db.run("insert into lanes(id, project_id, mission_id) values (?, ?, ?)", ["lane-review", "project-1", "mission-1"]); + db.run( + "insert into missions(id, project_id, title, prompt, status, outcome_summary, updated_at) values (?, ?, ?, ?, ?, ?, ?)", + [ + "mission-1", + "project-1", + "Keep preload and renderer aligned", + "This is a very long mission prompt that should be clipped in the compact provenance packet because raw prompt bloat is not review-safe and should never be copied wholesale into prompts or artifacts.", + "running", + "Workers are still converging on the bridge rollout.", + "2026-04-06T09:59:00.000Z", + ], + ); + for (let index = 0; index < 4; index += 1) { + db.run( + ` + insert into orchestrator_worker_digests( + id, project_id, mission_id, run_id, step_id, attempt_id, lane_id, session_id, step_key, status, + summary, files_changed_json, tests_run_json, warnings_json, tokens_json, cost_usd, suggested_next_actions_json, created_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `, + [ + `digest-${index}`, + "project-1", + "mission-1", + "orchestrator-run-1", + `step-${index}`, + `attempt-${index}`, + "lane-review", + `session-${index}`, + `step-${index}`, + index === 0 ? "failed" : "succeeded", + `Worker digest ${index} repeated text ${"x".repeat(80)}`, + JSON.stringify(["apps/desktop/src/preload/reviewBridge.ts"]), + JSON.stringify({ passed: 0, failed: 1, skipped: 0, summary: "bridge unit failed" }), + JSON.stringify(["warning one", "warning two"]), + null, + null, + JSON.stringify(["fix the bridge"]), + `2026-04-06T10:0${index}:00.000Z`, + ], + ); + } + db.run( + ` + insert into pull_requests( + id, project_id, lane_id, repo_owner, repo_name, github_pr_number, github_url, title, state, updated_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `, + [ + "pr-1", + "project-1", + "lane-review", + "ade-dev", + "ade", + 26, + "https://github.com/ade-dev/ade/pull/26", + "Bridge rollout", + "open", + "2026-04-06T10:05:00.000Z", + ], + ); + db.run( + "insert into review_runs(id, project_id, lane_id, summary, status, finding_count, created_at) values (?, ?, ?, ?, ?, ?, ?)", + ["run-prior", "project-1", "lane-review", "Prior ADE review on the same bridge path", "completed", 1, "2026-04-05T15:00:00.000Z"], + ); + db.run("insert into review_findings(id, run_id, file_path) values (?, ?, ?)", [ + "finding-prior", + "run-prior", + "apps/desktop/src/preload/reviewBridge.ts", + ]); + db.run("insert into review_run_publications(id, run_id) values (?, ?)", ["publication-prior", "run-prior"]); + + const builder = createReviewContextBuilder({ + db: db as any, + projectId: "project-1", + logger: { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + } as any, + laneService: { + getStateSnapshot: vi.fn().mockReturnValue({ + laneId: "lane-review", + agentSummary: { summary: "Recent ADE chat distilled the bridge rollout intent." }, + missionSummary: { summary: "Finish the preload bridge rollout cleanly." }, + updatedAt: "2026-04-06T09:58:00.000Z", + }), + } as any, + sessionDeltaService: { + listRecentLaneSessionDeltas: vi.fn().mockReturnValue([ + { + sessionId: "delta-1", + laneId: "lane-review", + startedAt: "2026-04-06T09:40:00.000Z", + endedAt: "2026-04-06T09:50:00.000Z", + headShaStart: null, + headShaEnd: null, + filesChanged: 1, + insertions: 10, + deletions: 2, + touchedFiles: ["apps/desktop/src/preload/reviewBridge.ts"], + failureLines: ["AssertionError: bridge mismatch", "Traceback: renderer bridge failed"], + computedAt: "2026-04-06T09:50:10.000Z", + }, + { + sessionId: "delta-2", + laneId: "lane-review", + startedAt: "2026-04-06T09:20:00.000Z", + endedAt: "2026-04-06T09:25:00.000Z", + headShaStart: null, + headShaEnd: null, + filesChanged: 2, + insertions: 5, + deletions: 1, + touchedFiles: ["apps/desktop/src/shared/ipc.ts"], + failureLines: ["Error: shared IPC shape drifted"], + computedAt: "2026-04-06T09:25:10.000Z", + }, + { + sessionId: "delta-3", + laneId: "lane-review", + startedAt: "2026-04-06T09:00:00.000Z", + endedAt: "2026-04-06T09:05:00.000Z", + headShaStart: null, + headShaEnd: null, + filesChanged: 1, + insertions: 3, + deletions: 0, + touchedFiles: ["apps/desktop/src/renderer/review/ReviewPanel.tsx"], + failureLines: ["Error: renderer still expects old bridge"], + computedAt: "2026-04-06T09:05:10.000Z", + }, + { + sessionId: "delta-4", + laneId: "lane-review", + startedAt: "2026-04-06T08:00:00.000Z", + endedAt: "2026-04-06T08:05:00.000Z", + headShaStart: null, + headShaEnd: null, + filesChanged: 1, + insertions: 1, + deletions: 1, + touchedFiles: ["unrelated.ts"], + failureLines: ["Error: unrelated"], + computedAt: "2026-04-06T08:05:10.000Z", + }, + ]), + } as any, + testService: { + listSuites: vi.fn().mockReturnValue([{ id: "unit" }, { id: "lint" }, { id: "e2e" }]), + listRuns: vi.fn().mockReturnValue([ + { + id: "test-run-1", + suiteId: "unit", + suiteName: "Unit", + laneId: "lane-review", + status: "failed", + exitCode: 1, + durationMs: 1200, + startedAt: "2026-04-06T10:00:00.000Z", + endedAt: "2026-04-06T10:02:00.000Z", + logPath: "/tmp/test-run-1.log", + }, + ]), + getLogTail: vi.fn().mockReturnValue( + `${"noise ".repeat(200)}\nAssertionError: bridge mismatch repeated ${"y".repeat(200)}\n`, + ), + } as any, + issueInventoryService: { + getInventory: vi.fn().mockReturnValue({ + prId: "pr-1", + items: Array.from({ length: 6 }, (_, index) => ({ + id: `inventory-${index}`, + prId: "pr-1", + source: "human", + type: "review_thread", + externalId: `thread-${index}`, + state: "new", + round: 1, + filePath: index === 0 ? "apps/desktop/src/preload/reviewBridge.ts" : "apps/desktop/src/renderer/review/ReviewPanel.tsx", + line: 20 + index, + severity: "major", + headline: `Review feedback ${index}`, + body: `Feedback body ${index}`, + author: "reviewer", + url: null, + dismissReason: null, + agentSessionId: null, + createdAt: "2026-04-06T10:01:00.000Z", + updatedAt: `2026-04-06T10:0${index}:00.000Z`, + })), + convergence: { currentRound: 1 }, + runtime: { currentRound: 1 }, + }), + } as any, + prService: { + getChecks: vi.fn().mockResolvedValue([ + { + name: "unit-tests", + status: "completed", + conclusion: "failure", + detailsUrl: "https://ci.example/unit-tests", + startedAt: "2026-04-06T10:00:00.000Z", + completedAt: "2026-04-06T10:03:00.000Z", + }, + { + name: "lint", + status: "completed", + conclusion: "success", + detailsUrl: null, + startedAt: "2026-04-06T10:04:00.000Z", + completedAt: "2026-04-06T10:05:00.000Z", + }, + ]), + getReviewSnapshot: vi.fn().mockResolvedValue({ + id: "pr-1", + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 26, + githubUrl: "https://github.com/ade-dev/ade/pull/26", + baseBranch: "main", + headBranch: "feature/bridge", + baseSha: "abc123", + headSha: "def456", + files: [{ filename: "apps/desktop/src/preload/reviewBridge.ts" }], + }), + } as any, + }); + + const packet = await builder.buildContext({ + run: makeRun() as any, + materialized: makeMaterialized([ + "apps/desktop/src/preload/reviewBridge.ts", + "apps/desktop/src/shared/ipc.ts", + ]) as any, + }); + + expect(packet.provenance.payload.workerDigests).toHaveLength(3); + expect(packet.provenance.payload.sessionDeltas).toHaveLength(3); + expect(packet.validation.payload.issueInventory).toHaveLength(5); + expect(packet.validation.payload.signals.length).toBeLessThanOrEqual(5); + expect(packet.provenance.payload.missions[0]?.intentSummary?.length ?? 0).toBeLessThanOrEqual(220); + expect(packet.validation.payload.testRuns[0]?.logExcerpt?.length ?? 0).toBeLessThanOrEqual(220); + expect(packet.validation.prompt).not.toContain("noise noise noise noise noise noise"); + expect(packet.rules.metadata.matchedRuleIds).toContain("preload-bridge"); + expect(packet.rules.metadata.matchedRuleIds).toContain("shared-contract"); + }); + + it("emits late-stage signals for validation failures, reviewer feedback, and prior review overlap", async () => { + const { db } = createInMemoryAdeDb(); + db.run("insert into lanes(id, project_id, mission_id) values (?, ?, ?)", ["lane-review", "project-1", null]); + db.run( + ` + insert into pull_requests( + id, project_id, lane_id, repo_owner, repo_name, github_pr_number, github_url, title, state, updated_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + `, + ["pr-1", "project-1", "lane-review", "ade-dev", "ade", 26, "https://github.com/ade-dev/ade/pull/26", "Bridge rollout", "open", "2026-04-06T10:05:00.000Z"], + ); + db.run( + "insert into review_runs(id, project_id, lane_id, summary, status, finding_count, created_at) values (?, ?, ?, ?, ?, ?, ?)", + ["run-prior", "project-1", "lane-review", "Prior review flagged the bridge", "completed", 1, "2026-04-05T15:00:00.000Z"], + ); + db.run("insert into review_findings(id, run_id, file_path) values (?, ?, ?)", [ + "finding-prior", + "run-prior", + "apps/desktop/src/preload/reviewBridge.ts", + ]); + + const builder = createReviewContextBuilder({ + db: db as any, + projectId: "project-1", + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() } as any, + laneService: { + getStateSnapshot: vi.fn().mockReturnValue(null), + } as any, + sessionDeltaService: { + listRecentLaneSessionDeltas: vi.fn().mockReturnValue([ + { + sessionId: "delta-1", + laneId: "lane-review", + startedAt: "2026-04-06T09:40:00.000Z", + endedAt: "2026-04-06T09:50:00.000Z", + headShaStart: null, + headShaEnd: null, + filesChanged: 1, + insertions: 1, + deletions: 0, + touchedFiles: ["apps/desktop/src/preload/reviewBridge.ts"], + failureLines: ["AssertionError: bridge mismatch"], + computedAt: "2026-04-06T09:50:10.000Z", + }, + ]), + } as any, + testService: { + listSuites: vi.fn().mockReturnValue([]), + listRuns: vi.fn().mockReturnValue([]), + getLogTail: vi.fn().mockReturnValue(""), + } as any, + issueInventoryService: { + getInventory: vi.fn().mockReturnValue({ + prId: "pr-1", + items: [ + { + id: "inventory-1", + prId: "pr-1", + source: "human", + type: "review_thread", + externalId: "thread-1", + state: "new", + round: 1, + filePath: "apps/desktop/src/preload/reviewBridge.ts", + line: 10, + severity: "major", + headline: "Reviewer says the preload bridge still drifts", + body: null, + author: "reviewer", + url: null, + dismissReason: null, + agentSessionId: null, + createdAt: "2026-04-06T10:01:00.000Z", + updatedAt: "2026-04-06T10:02:00.000Z", + }, + ], + convergence: { currentRound: 1 }, + runtime: { currentRound: 1 }, + }), + } as any, + prService: { + getChecks: vi.fn().mockResolvedValue([]), + getReviewSnapshot: vi.fn().mockResolvedValue({ + id: "pr-1", + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 26, + githubUrl: "https://github.com/ade-dev/ade/pull/26", + baseBranch: "main", + headBranch: "feature/bridge", + baseSha: "abc123", + headSha: "def456", + files: [{ filename: "apps/desktop/src/preload/reviewBridge.ts" }], + }), + } as any, + }); + + const packet = await builder.buildContext({ + run: makeRun() as any, + materialized: makeMaterialized(["apps/desktop/src/preload/reviewBridge.ts"]) as any, + }); + + const kinds = packet.provenance.payload.lateStageSignals.map((signal) => signal.kind); + expect(kinds).toContain("validation_failure_followed_by_edits"); + expect(kinds).toContain("review_feedback_followed_by_edits"); + expect(kinds).toContain("prior_review_overlap"); + }); +}); diff --git a/apps/desktop/src/main/services/review/reviewContextBuilder.ts b/apps/desktop/src/main/services/review/reviewContextBuilder.ts new file mode 100644 index 000000000..524ded6df --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewContextBuilder.ts @@ -0,0 +1,848 @@ +import type { + IssueInventorySnapshot, + PrCheck, + PrReviewSnapshot, + ReviewRun, + SessionDeltaSummary, +} from "../../../shared/types"; +import type { Logger } from "../logging/logger"; +import type { createLaneService } from "../lanes/laneService"; +import type { createPrService } from "../prs/prService"; +import type { createIssueInventoryService } from "../prs/issueInventoryService"; +import { parseWorkerDigestRow } from "../orchestrator/workerTracking"; +import type { createSessionDeltaService } from "../sessions/sessionDeltaService"; +import type { AdeDb } from "../state/kvDb"; +import type { createTestService } from "../tests/testService"; +import { getErrorMessage } from "../shared/utils"; +import type { ReviewMaterializedTarget } from "./reviewTargetMaterializer"; +import { + matchReviewRuleOverlays, + type MatchedReviewRuleOverlay, +} from "./reviewRuleRegistry"; + +const MISSION_LIMIT = 1; +const WORKER_DIGEST_LIMIT = 3; +const SESSION_DELTA_LIMIT = 3; +const PRIOR_REVIEW_LIMIT = 2; +const VALIDATION_SIGNAL_LIMIT = 5; +const ISSUE_INVENTORY_LIMIT = 5; +const MAX_TEXT_FIELD = 220; +const MAX_PROMPT_SECTION = 6_000; + +type LinkedPrRow = { + id: string; + title: string | null; + state: string; + github_url: string; + repo_owner: string; + repo_name: string; + github_pr_number: number; + updated_at: string; +}; + +type MissionRow = { + mission_id: string | null; + title: string | null; + prompt: string | null; + status: string | null; + outcome_summary: string | null; + updated_at: string | null; +}; + +type PriorReviewRow = { + id: string; + status: string; + summary: string | null; + finding_count: number; + created_at: string; + publication_count: number; +}; + +type WorkerDigestRow = { + id: string; + mission_id: string; + run_id: string; + step_id: string; + attempt_id: string; + lane_id: string | null; + session_id: string | null; + step_key: string | null; + status: string; + summary: string; + files_changed_json: string | null; + tests_run_json: string | null; + warnings_json: string | null; + tokens_json: string | null; + cost_usd: number | null; + suggested_next_actions_json: string | null; + created_at: string; +}; + +export type ReviewContextProvenancePayload = { + changedPaths: string[]; + laneSnapshot: { + updatedAt: string | null; + agentSummary: string | null; + missionSummary: string | null; + } | null; + missions: Array<{ + id: string; + title: string; + status: string | null; + outcomeSummary: string | null; + intentSummary: string | null; + updatedAt: string | null; + }>; + workerDigests: Array<{ + id: string; + stepKey: string | null; + status: string; + summary: string; + filesChanged: string[]; + testsSummary: string | null; + warnings: string[]; + createdAt: string; + }>; + sessionDeltas: Array<{ + sessionId: string; + startedAt: string; + endedAt: string | null; + filesChanged: number; + touchedFiles: string[]; + failureLines: string[]; + computedAt: string | null; + }>; + priorReviews: Array<{ + runId: string; + status: string; + summary: string | null; + findingCount: number; + publicationCount: number; + overlappingPaths: string[]; + createdAt: string; + }>; + lateStageSignals: Array<{ + kind: "validation_failure_followed_by_edits" | "review_feedback_followed_by_edits" | "prior_review_overlap"; + summary: string; + filePaths: string[]; + source: string; + occurredAt: string | null; + }>; +}; + +export type ReviewContextRulesPayload = { + changedPaths: string[]; + overlays: Array<{ + id: MatchedReviewRuleOverlay["id"]; + label: string; + description: string; + matchedPaths: string[]; + rolloutExpectations: string[]; + coveredFamilies: Array<{ id: string; label: string }>; + missingFamilies: Array<{ id: string; label: string }>; + adjudicationPolicy: MatchedReviewRuleOverlay["adjudicationPolicy"]; + }>; +}; + +export type ReviewContextValidationPayload = { + linkedPr: { + prId: string; + title: string | null; + state: string; + repo: string; + githubUrl: string; + updatedAt: string; + } | null; + reviewSnapshot: { + baseBranch: string | null; + headBranch: string | null; + baseSha: string | null; + headSha: string | null; + fileCount: number; + } | null; + checks: Array<{ + name: string; + status: string; + conclusion: string | null; + detailsUrl: string | null; + startedAt: string | null; + completedAt: string | null; + }>; + suites: string[]; + testRuns: Array<{ + runId: string; + suiteId: string; + suiteName: string; + status: string; + exitCode: number | null; + startedAt: string; + endedAt: string | null; + logExcerpt: string | null; + }>; + issueInventory: Array<{ + id: string; + source: string; + type: string; + state: string; + round: number; + headline: string; + body: string | null; + filePath: string | null; + line: number | null; + updatedAt: string; + }>; + sessionFailures: Array<{ + sessionId: string; + touchedFiles: string[]; + failureLines: string[]; + computedAt: string | null; + }>; + signals: Array<{ + kind: "pr_check_failure" | "test_run_failure" | "review_feedback" | "session_failure"; + summary: string; + filePaths: string[]; + sourceId: string; + }>; +}; + +export type ReviewContextSection> = { + summary: string; + prompt: string; + payload: TPayload; + metadata: Record; +}; + +export type ReviewContextPacket = { + matchedRuleOverlays: MatchedReviewRuleOverlay[]; + provenance: ReviewContextSection; + rules: ReviewContextSection; + validation: ReviewContextSection; +}; + +function clipText(value: string | null | undefined, maxChars: number = MAX_TEXT_FIELD): string | null { + const normalized = String(value ?? "").replace(/\s+/g, " ").trim(); + if (!normalized) return null; + if (normalized.length <= maxChars) return normalized; + return `${normalized.slice(0, Math.max(0, maxChars - 3)).trimEnd()}...`; +} + +function compactList(values: Array, limit: number): string[] { + const seen = new Set(); + const compacted: string[] = []; + for (const value of values) { + const clipped = clipText(value); + if (!clipped || seen.has(clipped)) continue; + seen.add(clipped); + compacted.push(clipped); + if (compacted.length >= limit) break; + } + return compacted; +} + +function summarizeRecord(record: Record | null): string | null { + if (!record) return null; + const directKeys = [ + "summary", + "headline", + "title", + "goal", + "currentTask", + "statusSummary", + "latestMessage", + "intent", + "mission", + ]; + const directValues = directKeys + .map((key) => record[key]) + .filter((value): value is string => typeof value === "string"); + const clippedDirect = compactList(directValues, 2); + if (clippedDirect.length > 0) return clippedDirect.join(" | "); + + const nestedValues = Object.values(record) + .flatMap((value) => { + if (typeof value === "string") return [value]; + if (Array.isArray(value)) { + return value.filter((entry): entry is string => typeof entry === "string"); + } + return []; + }); + const clippedNested = compactList(nestedValues, 2); + return clippedNested.length > 0 ? clippedNested.join(" | ") : null; +} + +function overlapsChangedPaths(candidatePaths: Array, changedPaths: string[]): string[] { + const changedSet = new Set(changedPaths); + return candidatePaths + .map((value) => value?.trim() ?? "") + .filter((value) => value.length > 0 && changedSet.has(value)) + .slice(0, 5); +} + +function extractFailureExcerpt(rawTail: string): string | null { + const lines = rawTail + .split(/\r?\n/) + .map((line) => line.replace(/\s+/g, " ").trim()) + .filter(Boolean) + .filter((line) => /\b(error|failed|failure|exception|fatal|traceback)\b/i.test(line)); + return compactList(lines, 3).join(" | ") || null; +} + +function truncatePromptSection(value: string): string { + if (value.length <= MAX_PROMPT_SECTION) return value; + return `${value.slice(0, MAX_PROMPT_SECTION)}\n...(truncated)...\n`; +} + +export function createReviewContextBuilder({ + db, + projectId, + logger, + laneService, + sessionDeltaService, + testService, + issueInventoryService, + prService, +}: { + db: AdeDb; + projectId: string; + logger: Logger; + laneService: Pick, "getStateSnapshot">; + sessionDeltaService: Pick, "listRecentLaneSessionDeltas">; + testService: Pick, "listRuns" | "getLogTail" | "listSuites">; + issueInventoryService: Pick, "getInventory">; + prService?: Pick, "getChecks" | "getReviewSnapshot">; +}) { + function getLinkedPrRow(run: ReviewRun): LinkedPrRow | null { + if (run.target.mode === "pr") { + return db.get( + ` + select id, title, state, github_url, repo_owner, repo_name, github_pr_number, updated_at + from pull_requests + where id = ? + and project_id = ? + limit 1 + `, + [run.target.prId, projectId], + ); + } + return db.get( + ` + select id, title, state, github_url, repo_owner, repo_name, github_pr_number, updated_at + from pull_requests + where project_id = ? + and lane_id = ? + and state in ('open', 'draft') + order by updated_at desc + limit 1 + `, + [projectId, run.laneId], + ); + } + + function getMissionRow(laneId: string): MissionRow | null { + return db.get( + ` + select + l.mission_id as mission_id, + m.title as title, + m.prompt as prompt, + m.status as status, + m.outcome_summary as outcome_summary, + m.updated_at as updated_at + from lanes l + left join missions m on m.id = l.mission_id + where l.id = ? + and l.project_id = ? + limit 1 + `, + [laneId, projectId], + ); + } + + function listWorkerDigests(missionId: string | null, laneId: string): ReviewContextProvenancePayload["workerDigests"] { + if (!missionId?.trim()) return []; + const rows = db.all( + ` + select * + from orchestrator_worker_digests + where mission_id = ? + and project_id = ? + and (lane_id = ? or lane_id is null) + order by created_at desc + limit ? + `, + [missionId, projectId, laneId, WORKER_DIGEST_LIMIT], + ); + return rows.map((row) => { + const digest = parseWorkerDigestRow(row); + return { + id: digest.id, + stepKey: digest.stepKey, + status: digest.status, + summary: clipText(digest.summary) ?? "No summary", + filesChanged: digest.filesChanged.slice(0, 4), + testsSummary: clipText(digest.testsRun?.summary ?? null), + warnings: compactList(digest.warnings, 2), + createdAt: digest.createdAt, + }; + }); + } + + function listPriorReviews(runId: string, laneId: string, changedPaths: string[]): ReviewContextProvenancePayload["priorReviews"] { + const rows = db.all( + ` + select + r.id, + r.status, + r.summary, + r.finding_count, + r.created_at, + ( + select count(1) + from review_run_publications p + where p.run_id = r.id + ) as publication_count + from review_runs r + where r.project_id = ? + and r.lane_id = ? + and r.id != ? + order by r.created_at desc + limit ? + `, + [projectId, laneId, runId, PRIOR_REVIEW_LIMIT], + ); + return rows.map((row) => { + const findingRows = db.all<{ file_path: string | null }>( + ` + select file_path + from review_findings + where run_id = ? + and file_path is not null + order by file_path asc + limit 16 + `, + [row.id], + ); + return { + runId: row.id, + status: row.status, + summary: clipText(row.summary), + findingCount: Number(row.finding_count ?? 0), + publicationCount: Number(row.publication_count ?? 0), + overlappingPaths: overlapsChangedPaths(findingRows.map((entry) => entry.file_path), changedPaths), + createdAt: row.created_at, + }; + }); + } + + async function buildValidationPayload(args: { + laneId: string; + changedPaths: string[]; + linkedPr: LinkedPrRow | null; + sessionDeltas: ReviewContextProvenancePayload["sessionDeltas"]; + }): Promise { + let reviewSnapshot: PrReviewSnapshot | null = null; + let checks: PrCheck[] = []; + let inventory: IssueInventorySnapshot | null = null; + + if (args.linkedPr?.id) { + if (prService) { + reviewSnapshot = await prService.getReviewSnapshot(args.linkedPr.id).catch((error) => { + logger.debug("review.context_builder.review_snapshot_unavailable", { + prId: args.linkedPr?.id, + error: getErrorMessage(error), + }); + return null; + }); + checks = await prService.getChecks(args.linkedPr.id).catch((error) => { + logger.debug("review.context_builder.pr_checks_unavailable", { + prId: args.linkedPr?.id, + error: getErrorMessage(error), + }); + return []; + }); + } + try { + inventory = issueInventoryService.getInventory(args.linkedPr.id); + } catch (error) { + logger.debug("review.context_builder.issue_inventory_unavailable", { + prId: args.linkedPr?.id, + error: getErrorMessage(error), + }); + } + } + + const suites = testService.listSuites().map((suite) => suite.id).slice(0, VALIDATION_SIGNAL_LIMIT); + const testRuns = testService.listRuns({ laneId: args.laneId, limit: VALIDATION_SIGNAL_LIMIT }); + const normalizedTestRuns = testRuns.map((run) => ({ + runId: run.id, + suiteId: run.suiteId, + suiteName: clipText(run.suiteName, 80) ?? run.suiteId, + status: run.status, + exitCode: run.exitCode, + startedAt: run.startedAt, + endedAt: run.endedAt, + logExcerpt: (run.status === "failed" || run.status === "timed_out") + ? clipText(extractFailureExcerpt(testService.getLogTail({ runId: run.id, maxBytes: 12_000 })), 260) + : null, + })); + const normalizedChecks = checks.map((check) => ({ + name: clipText(check.name, 100) ?? "unnamed check", + status: check.status, + conclusion: check.conclusion, + detailsUrl: check.detailsUrl, + startedAt: check.startedAt, + completedAt: check.completedAt, + })); + const unresolvedInventoryItems = (inventory?.items ?? []) + .filter((item) => item.state !== "fixed" && item.state !== "dismissed") + .sort((left, right) => right.updatedAt.localeCompare(left.updatedAt)) + .slice(0, ISSUE_INVENTORY_LIMIT) + .map((item) => ({ + id: item.id, + source: item.source, + type: item.type, + state: item.state, + round: item.round, + headline: clipText(item.headline) ?? item.id, + body: clipText(item.body), + filePath: item.filePath, + line: item.line, + updatedAt: item.updatedAt, + })); + const sessionFailures = args.sessionDeltas + .filter((delta) => delta.failureLines.length > 0) + .slice(0, SESSION_DELTA_LIMIT) + .map((delta) => ({ + sessionId: delta.sessionId, + touchedFiles: delta.touchedFiles.slice(0, 5), + failureLines: compactList(delta.failureLines, 3), + computedAt: delta.computedAt, + })); + + const signals: ReviewContextValidationPayload["signals"] = []; + for (const check of normalizedChecks) { + if (signals.length >= VALIDATION_SIGNAL_LIMIT) break; + const failed = check.conclusion === "failure" || (check.status === "completed" && check.conclusion !== "success"); + if (!failed) continue; + signals.push({ + kind: "pr_check_failure", + summary: clipText(`${check.name}: ${check.status}${check.conclusion ? ` / ${check.conclusion}` : ""}${check.detailsUrl ? ` (${check.detailsUrl})` : ""}`, 260) ?? check.name, + filePaths: [], + sourceId: check.name, + }); + } + for (const run of normalizedTestRuns) { + if (signals.length >= VALIDATION_SIGNAL_LIMIT) break; + if (run.status !== "failed" && run.status !== "timed_out") continue; + signals.push({ + kind: "test_run_failure", + summary: clipText(`${run.suiteName}: ${run.status}${run.logExcerpt ? ` — ${run.logExcerpt}` : ""}`, 260) ?? run.suiteName, + filePaths: [], + sourceId: run.runId, + }); + } + for (const item of unresolvedInventoryItems) { + if (signals.length >= VALIDATION_SIGNAL_LIMIT) break; + signals.push({ + kind: "review_feedback", + summary: clipText(`${item.headline}${item.filePath ? ` (${item.filePath}${item.line ? `:${item.line}` : ""})` : ""}`, 260) ?? item.id, + filePaths: item.filePath ? [item.filePath] : [], + sourceId: item.id, + }); + } + for (const delta of sessionFailures) { + if (signals.length >= VALIDATION_SIGNAL_LIMIT) break; + signals.push({ + kind: "session_failure", + summary: clipText(delta.failureLines.join(" | "), 260) ?? delta.sessionId, + filePaths: overlapsChangedPaths(delta.touchedFiles, args.changedPaths), + sourceId: delta.sessionId, + }); + } + + return { + linkedPr: args.linkedPr ? { + prId: args.linkedPr.id, + title: clipText(args.linkedPr.title), + state: args.linkedPr.state, + repo: `${args.linkedPr.repo_owner}/${args.linkedPr.repo_name}`, + githubUrl: args.linkedPr.github_url, + updatedAt: args.linkedPr.updated_at, + } : null, + reviewSnapshot: reviewSnapshot ? { + baseBranch: reviewSnapshot.baseBranch ?? null, + headBranch: reviewSnapshot.headBranch ?? null, + baseSha: reviewSnapshot.baseSha ?? null, + headSha: reviewSnapshot.headSha ?? null, + fileCount: reviewSnapshot.files.length, + } : null, + checks: normalizedChecks, + suites, + testRuns: normalizedTestRuns, + issueInventory: unresolvedInventoryItems, + sessionFailures, + signals, + }; + } + + function buildProvenancePayload(args: { + materialized: ReviewMaterializedTarget; + laneSnapshot: ReturnType, "getStateSnapshot">["getStateSnapshot"]>; + missionRow: MissionRow | null; + workerDigests: ReviewContextProvenancePayload["workerDigests"]; + sessionDeltas: SessionDeltaSummary[]; + priorReviews: ReviewContextProvenancePayload["priorReviews"]; + validation: ReviewContextValidationPayload; + }): ReviewContextProvenancePayload { + const changedPaths = args.materialized.changedFiles.map((file) => file.filePath); + const normalizedSessionDeltas = args.sessionDeltas.slice(0, SESSION_DELTA_LIMIT).map((delta) => ({ + sessionId: delta.sessionId, + startedAt: delta.startedAt, + endedAt: delta.endedAt, + filesChanged: delta.filesChanged, + touchedFiles: delta.touchedFiles.slice(0, 5), + failureLines: compactList(delta.failureLines, 3), + computedAt: delta.computedAt, + })); + const missions = args.missionRow?.mission_id && MISSION_LIMIT > 0 ? [{ + id: args.missionRow.mission_id, + title: clipText(args.missionRow.title, 120) ?? args.missionRow.mission_id, + status: args.missionRow.status, + outcomeSummary: clipText(args.missionRow.outcome_summary), + intentSummary: clipText(args.missionRow.prompt), + updatedAt: args.missionRow.updated_at, + }] : []; + const lateStageSignals: ReviewContextProvenancePayload["lateStageSignals"] = []; + for (const delta of normalizedSessionDeltas) { + const overlappingPaths = overlapsChangedPaths(delta.touchedFiles, changedPaths); + if (overlappingPaths.length === 0 || delta.failureLines.length === 0) continue; + lateStageSignals.push({ + kind: "validation_failure_followed_by_edits", + summary: clipText(`Recent lane validation failed before edits touched ${overlappingPaths.join(", ")}: ${delta.failureLines.join(" | ")}`, 260) ?? delta.sessionId, + filePaths: overlappingPaths, + source: delta.sessionId, + occurredAt: delta.computedAt ?? delta.endedAt, + }); + } + for (const item of args.validation.issueInventory) { + if (lateStageSignals.length >= VALIDATION_SIGNAL_LIMIT) break; + const overlappingPaths = overlapsChangedPaths([item.filePath], changedPaths); + if (overlappingPaths.length === 0) continue; + lateStageSignals.push({ + kind: "review_feedback_followed_by_edits", + summary: clipText(`Open reviewer or check feedback still targets ${overlappingPaths.join(", ")}: ${item.headline}`, 260) ?? item.id, + filePaths: overlappingPaths, + source: item.id, + occurredAt: item.updatedAt, + }); + } + for (const review of args.priorReviews) { + if (lateStageSignals.length >= VALIDATION_SIGNAL_LIMIT) break; + if (review.overlappingPaths.length === 0) continue; + lateStageSignals.push({ + kind: "prior_review_overlap", + summary: clipText(`A prior ADE review already flagged ${review.overlappingPaths.join(", ")} (${review.findingCount} finding${review.findingCount === 1 ? "" : "s"}).`, 260) ?? review.runId, + filePaths: review.overlappingPaths, + source: review.runId, + occurredAt: review.createdAt, + }); + } + + return { + changedPaths, + laneSnapshot: args.laneSnapshot ? { + updatedAt: args.laneSnapshot.updatedAt, + agentSummary: clipText(summarizeRecord(args.laneSnapshot.agentSummary)), + missionSummary: clipText(summarizeRecord(args.laneSnapshot.missionSummary)), + } : null, + missions, + workerDigests: args.workerDigests, + sessionDeltas: normalizedSessionDeltas, + priorReviews: args.priorReviews, + lateStageSignals: lateStageSignals.slice(0, VALIDATION_SIGNAL_LIMIT), + }; + } + + function buildProvenancePrompt(payload: ReviewContextProvenancePayload): string { + const lines: string[] = []; + if (payload.laneSnapshot?.agentSummary) { + lines.push(`- Lane agent summary: ${payload.laneSnapshot.agentSummary}`); + } + if (payload.laneSnapshot?.missionSummary) { + lines.push(`- Lane mission summary: ${payload.laneSnapshot.missionSummary}`); + } + for (const mission of payload.missions) { + lines.push(`- Mission: ${mission.title}${mission.status ? ` [${mission.status}]` : ""}${mission.outcomeSummary ? ` — ${mission.outcomeSummary}` : mission.intentSummary ? ` — ${mission.intentSummary}` : ""}`); + } + for (const digest of payload.workerDigests) { + lines.push(`- Worker digest: ${digest.stepKey ?? "worker"} ${digest.status} — ${digest.summary}`); + } + for (const delta of payload.sessionDeltas) { + if (delta.failureLines.length > 0) { + lines.push(`- Session delta: ${delta.failureLines.join(" | ")}`); + } + } + for (const review of payload.priorReviews) { + lines.push(`- Prior ADE review: ${review.summary ?? "No summary"}${review.overlappingPaths.length > 0 ? ` (overlaps ${review.overlappingPaths.join(", ")})` : ""}`); + } + for (const signal of payload.lateStageSignals) { + lines.push(`- Late-stage signal: ${signal.summary}`); + } + return truncatePromptSection(lines.length > 0 ? lines.join("\n") : "- No ADE provenance or intent context was available."); + } + + function buildRulesPrompt(overlays: MatchedReviewRuleOverlay[]): string { + if (overlays.length === 0) { + return "- No ADE repo/path-specific rule overlay matched the changed paths."; + } + const lines = overlays.map((overlay) => { + const coverage = overlay.missingFamilies.length > 0 + ? `missing companion coverage: ${overlay.missingFamilies.map((family) => family.label).join(", ")}` + : "companion families touched in this diff"; + return `- ${overlay.label}: matched ${overlay.matchedPaths.join(", ")}; ${coverage}; rollout expectations: ${overlay.rolloutExpectations.join(" ")}`; + }); + return truncatePromptSection(lines.join("\n")); + } + + function buildValidationPrompt(payload: ReviewContextValidationPayload): string { + const lines: string[] = []; + if (payload.linkedPr) { + lines.push(`- Linked PR: ${payload.linkedPr.repo} #${payload.linkedPr.prId}${payload.linkedPr.title ? ` — ${payload.linkedPr.title}` : ""}`); + } + for (const signal of payload.signals) { + lines.push(`- Validation signal: ${signal.summary}`); + } + if (payload.signals.length === 0 && payload.testRuns.length > 0) { + const latestRuns = payload.testRuns.slice(0, 2).map((run) => `${run.suiteName}: ${run.status}`).join(" | "); + lines.push(`- Recent test runs: ${latestRuns}`); + } + return truncatePromptSection(lines.length > 0 ? lines.join("\n") : "- No prior ADE validation signals were available."); + } + + function buildProvenanceSummary(payload: ReviewContextProvenancePayload): string { + const parts = [ + payload.missions.length > 0 ? `${payload.missions.length} mission` : null, + payload.workerDigests.length > 0 ? `${payload.workerDigests.length} worker digest${payload.workerDigests.length === 1 ? "" : "s"}` : null, + payload.sessionDeltas.length > 0 ? `${payload.sessionDeltas.length} session delta${payload.sessionDeltas.length === 1 ? "" : "s"}` : null, + payload.priorReviews.length > 0 ? `${payload.priorReviews.length} prior review${payload.priorReviews.length === 1 ? "" : "s"}` : null, + payload.lateStageSignals.length > 0 ? `${payload.lateStageSignals.length} late-stage signal${payload.lateStageSignals.length === 1 ? "" : "s"}` : null, + ].filter((value): value is string => Boolean(value)); + return parts.length > 0 ? parts.join(", ") : "No ADE provenance context"; + } + + function buildRulesSummary(overlays: MatchedReviewRuleOverlay[]): string { + if (overlays.length === 0) return "No rule overlays matched"; + return `${overlays.length} rule overlay${overlays.length === 1 ? "" : "s"} matched`; + } + + function buildValidationSummary(payload: ReviewContextValidationPayload): string { + const parts = [ + payload.signals.length > 0 ? `${payload.signals.length} validation signal${payload.signals.length === 1 ? "" : "s"}` : null, + payload.checks.length > 0 ? `${payload.checks.length} check${payload.checks.length === 1 ? "" : "s"}` : null, + payload.testRuns.length > 0 ? `${payload.testRuns.length} test run${payload.testRuns.length === 1 ? "" : "s"}` : null, + payload.issueInventory.length > 0 ? `${payload.issueInventory.length} inventory item${payload.issueInventory.length === 1 ? "" : "s"}` : null, + ].filter((value): value is string => Boolean(value)); + return parts.length > 0 ? parts.join(", ") : "No validation signals"; + } + + return { + async buildContext(args: { + run: ReviewRun; + materialized: ReviewMaterializedTarget; + }): Promise { + const changedPaths = args.materialized.changedFiles.map((file) => file.filePath); + const laneSnapshot = laneService.getStateSnapshot(args.run.laneId); + const missionRow = getMissionRow(args.run.laneId); + const sessionDeltas = sessionDeltaService.listRecentLaneSessionDeltas(args.run.laneId, SESSION_DELTA_LIMIT); + const workerDigests = listWorkerDigests(missionRow?.mission_id ?? null, args.run.laneId); + const priorReviews = listPriorReviews(args.run.id, args.run.laneId, changedPaths); + const linkedPr = getLinkedPrRow(args.run); + const matchedRules = matchReviewRuleOverlays(changedPaths); + const validationPayload = await buildValidationPayload({ + laneId: args.run.laneId, + changedPaths, + linkedPr, + sessionDeltas: sessionDeltas.map((delta) => ({ + sessionId: delta.sessionId, + startedAt: delta.startedAt, + endedAt: delta.endedAt, + filesChanged: delta.filesChanged, + touchedFiles: delta.touchedFiles, + failureLines: delta.failureLines, + computedAt: delta.computedAt, + })), + }); + const provenancePayload = buildProvenancePayload({ + materialized: args.materialized, + laneSnapshot, + missionRow, + workerDigests, + sessionDeltas, + priorReviews, + validation: validationPayload, + }); + const rulesPayload: ReviewContextRulesPayload = { + changedPaths, + overlays: matchedRules.map((overlay) => ({ + id: overlay.id, + label: overlay.label, + description: overlay.description, + matchedPaths: overlay.matchedPaths, + rolloutExpectations: overlay.rolloutExpectations, + coveredFamilies: overlay.coveredFamilies, + missingFamilies: overlay.missingFamilies, + adjudicationPolicy: overlay.adjudicationPolicy, + })), + }; + + return { + matchedRuleOverlays: matchedRules, + provenance: { + summary: buildProvenanceSummary(provenancePayload), + prompt: buildProvenancePrompt(provenancePayload), + payload: provenancePayload, + metadata: { + summary: buildProvenanceSummary(provenancePayload), + provenanceCount: + provenancePayload.missions.length + + provenancePayload.workerDigests.length + + provenancePayload.sessionDeltas.length + + provenancePayload.priorReviews.length + + provenancePayload.lateStageSignals.length, + missionCount: provenancePayload.missions.length, + workerDigestCount: provenancePayload.workerDigests.length, + sessionDeltaCount: provenancePayload.sessionDeltas.length, + priorReviewCount: provenancePayload.priorReviews.length, + lateStageSignalCount: provenancePayload.lateStageSignals.length, + }, + }, + rules: { + summary: buildRulesSummary(matchedRules), + prompt: buildRulesPrompt(matchedRules), + payload: rulesPayload, + metadata: { + summary: buildRulesSummary(matchedRules), + matchedRuleCount: matchedRules.length, + ruleCount: matchedRules.length, + pathCount: changedPaths.length, + matchedRuleIds: matchedRules.map((overlay) => overlay.id), + }, + }, + validation: { + summary: buildValidationSummary(validationPayload), + prompt: buildValidationPrompt(validationPayload), + payload: validationPayload, + metadata: { + summary: buildValidationSummary(validationPayload), + signalCount: validationPayload.signals.length, + checkCount: validationPayload.checks.length, + testRunCount: validationPayload.testRuns.length, + issueCount: validationPayload.issueInventory.length, + sessionFailureCount: validationPayload.sessionFailures.length, + suiteCount: validationPayload.suites.length, + }, + }, + }; + }, + }; +} diff --git a/apps/desktop/src/main/services/review/reviewDiffContext.test.ts b/apps/desktop/src/main/services/review/reviewDiffContext.test.ts new file mode 100644 index 000000000..259ce09c5 --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewDiffContext.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it } from "vitest"; +import { buildDiffContextForFinding } from "./reviewDiffContext"; + +const SAMPLE_PATCH = `diff --git a/src/auth.ts b/src/auth.ts +--- a/src/auth.ts ++++ b/src/auth.ts +@@ -10,6 +10,10 @@ + function validateToken(token: string) { + if (!token) return false; +- return token === SECRET; ++ if (token.length < 8) return false; ++ return verifySignature(token); ++} ++function verifySignature(token: string) { ++ return cryptoCompare(token, SECRET); + }`; + +describe("buildDiffContextForFinding", () => { + it("returns null when file path is missing", () => { + expect(buildDiffContextForFinding({ filePath: null, anchoredLine: 12, patches: [] })).toBeNull(); + }); + + it("returns null when no patch matches the file", () => { + const result = buildDiffContextForFinding({ + filePath: "src/other.ts", + anchoredLine: 10, + patches: [{ filePath: "src/auth.ts", excerpt: SAMPLE_PATCH }], + }); + expect(result).toBeNull(); + }); + + it("highlights the anchored line and slices window around it", () => { + const result = buildDiffContextForFinding({ + filePath: "src/auth.ts", + anchoredLine: 13, + patches: [{ filePath: "src/auth.ts", excerpt: SAMPLE_PATCH }], + }); + expect(result).not.toBeNull(); + expect(result!.filePath).toBe("src/auth.ts"); + expect(result!.anchoredLine).toBe(13); + const highlighted = result!.lines.filter((line) => line.highlighted); + expect(highlighted.length).toBe(1); + expect(highlighted[0]?.line).toBe(13); + expect(result!.lines.some((line) => line.kind === "add")).toBe(true); + }); + + it("shows full first hunk (no highlight) when anchor falls outside every hunk", () => { + const result = buildDiffContextForFinding({ + filePath: "src/auth.ts", + anchoredLine: 5000, + patches: [{ filePath: "src/auth.ts", excerpt: SAMPLE_PATCH }], + }); + expect(result).not.toBeNull(); + expect(result!.anchoredLine).toBeNull(); + expect(result!.lines.some((line) => line.highlighted)).toBe(false); + expect(result!.lines.some((line) => line.kind === "add" || line.kind === "context")).toBe(true); + }); + + it("falls back to the first hunk when no anchor is provided", () => { + const result = buildDiffContextForFinding({ + filePath: "src/auth.ts", + anchoredLine: null, + patches: [{ filePath: "src/auth.ts", excerpt: SAMPLE_PATCH }], + }); + expect(result).not.toBeNull(); + expect(result!.lines.length).toBeGreaterThan(0); + expect(result!.lines.find((line) => line.highlighted)).toBeUndefined(); + }); + + it("normalizes CRLF so no line text carries a trailing carriage return", () => { + const crlfPatch = SAMPLE_PATCH.replace(/\n/g, "\r\n"); + const result = buildDiffContextForFinding({ + filePath: "src/auth.ts", + anchoredLine: 13, + patches: [{ filePath: "src/auth.ts", excerpt: crlfPatch }], + }); + expect(result).not.toBeNull(); + for (const line of result!.lines) { + expect(line.text.includes("\r")).toBe(false); + } + }); +}); diff --git a/apps/desktop/src/main/services/review/reviewDiffContext.ts b/apps/desktop/src/main/services/review/reviewDiffContext.ts new file mode 100644 index 000000000..792464853 --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewDiffContext.ts @@ -0,0 +1,124 @@ +import type { ReviewDiffContext } from "../../../shared/types"; + +type FilePatchSource = { + filePath: string; + excerpt: string; +}; + +const CONTEXT_RADIUS = 8; + +function extractFileHunks(patchText: string): string[] { + if (!patchText) return []; + // Normalize CRLF so render doesn't show a stray `\r` glyph on every line. + const lines = patchText.replace(/\r\n/g, "\n").split("\n"); + const hunks: string[][] = []; + let current: string[] = []; + for (const line of lines) { + if (line.startsWith("@@")) { + if (current.length > 0) hunks.push(current); + current = [line]; + continue; + } + if (current.length > 0) current.push(line); + } + if (current.length > 0) hunks.push(current); + return hunks.map((hunk) => hunk.join("\n")); +} + +type ParsedHunk = { + newStart: number; + newLength: number; + lines: ReviewDiffContext["lines"]; +}; + +function parseHunk(hunk: string): ParsedHunk | null { + const rawLines = hunk.split("\n"); + const header = rawLines.shift(); + if (!header) return null; + const match = header.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/); + if (!match) return null; + const newStart = Number(match[1] ?? "0"); + const newLength = Number(match[2] ?? "1"); + + const output: ReviewDiffContext["lines"] = [ + { line: null, kind: "meta", text: header, highlighted: false }, + ]; + let runningLine = newStart; + for (const raw of rawLines) { + if (raw.length === 0) continue; + if (raw.startsWith("\\")) { + output.push({ line: null, kind: "meta", text: raw, highlighted: false }); + continue; + } + const ch = raw[0]!; + const text = raw.slice(1); + if (ch === "+") { + output.push({ line: runningLine, kind: "add", text, highlighted: false }); + runningLine += 1; + } else if (ch === "-") { + output.push({ line: null, kind: "del", text, highlighted: false }); + } else { + output.push({ line: runningLine, kind: "context", text, highlighted: false }); + runningLine += 1; + } + } + return { newStart, newLength, lines: output }; +} + +function sliceAroundAnchor(parsed: ParsedHunk, anchor: number): ParsedHunk { + const windowStart = Math.max(parsed.newStart, anchor - CONTEXT_RADIUS); + const windowEnd = anchor + CONTEXT_RADIUS; + const filtered: ReviewDiffContext["lines"] = []; + for (const entry of parsed.lines) { + if (entry.kind === "meta") { + filtered.push(entry); + continue; + } + if (entry.line == null) { + filtered.push(entry); + continue; + } + if (entry.line >= windowStart && entry.line <= windowEnd) { + filtered.push({ ...entry, highlighted: entry.line === anchor }); + } + } + return { ...parsed, lines: filtered }; +} + +export function buildDiffContextForFinding(args: { + filePath: string | null; + anchoredLine: number | null; + patches: FilePatchSource[]; +}): ReviewDiffContext | null { + if (!args.filePath) return null; + const patch = args.patches.find((entry) => entry.filePath === args.filePath); + if (!patch || !patch.excerpt) return null; + const hunks = extractFileHunks(patch.excerpt).map(parseHunk).filter((h): h is ParsedHunk => h != null); + if (hunks.length === 0) return null; + + let chosen: ParsedHunk | null = null; + let anchorInHunk = false; + if (args.anchoredLine != null) { + chosen = + hunks.find((hunk) => args.anchoredLine! >= hunk.newStart && args.anchoredLine! < hunk.newStart + hunk.newLength) ?? + null; + anchorInHunk = chosen != null; + } + if (!chosen) chosen = hunks[0] ?? null; + if (!chosen) return null; + + const sliced = anchorInHunk && args.anchoredLine != null + ? sliceAroundAnchor(chosen, args.anchoredLine) + : chosen; + const lineNumbers = sliced.lines.filter((entry) => entry.line != null).map((entry) => entry.line!); + const startLine = lineNumbers.length ? Math.min(...lineNumbers) : chosen.newStart; + const endLine = lineNumbers.length ? Math.max(...lineNumbers) : chosen.newStart + chosen.newLength - 1; + + return { + filePath: args.filePath, + startLine, + endLine, + anchoredLine: anchorInHunk ? args.anchoredLine : null, + lines: sliced.lines, + }; +} diff --git a/apps/desktop/src/main/services/review/reviewRuleRegistry.ts b/apps/desktop/src/main/services/review/reviewRuleRegistry.ts new file mode 100644 index 000000000..ecb380f5f --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewRuleRegistry.ts @@ -0,0 +1,255 @@ +import type { ReviewPassKey } from "../../../shared/types"; + +export type ReviewRuleCompanionFamily = { + id: string; + label: string; + pathPatterns: string[]; +}; + +export type ReviewRuleAdjudicationPolicy = { + evidenceMode: "normal" | "cross_boundary"; + requireDualPathConsideration?: boolean; +}; + +export type ReviewRuleOverlayDefinition = { + id: "renderer-surface" | "preload-bridge" | "shared-contract" | "mcp-dual-path"; + label: string; + description: string; + pathPatterns: string[]; + rolloutExpectations: string[]; + companionFamilies: ReviewRuleCompanionFamily[]; + promptGuidance: Partial>; + adjudicationPolicy: ReviewRuleAdjudicationPolicy; +}; + +export type MatchedReviewRuleOverlay = ReviewRuleOverlayDefinition & { + matchedPaths: string[]; + coveredFamilies: Array>; + missingFamilies: Array>; +}; + +function matchesPathPattern(filePath: string, pattern: string): boolean { + if (!pattern.trim()) return false; + if (pattern.endsWith("/**")) { + return filePath.startsWith(pattern.slice(0, -3)); + } + return filePath === pattern; +} + +function matchesAnyPattern(filePath: string, patterns: string[]): boolean { + return patterns.some((pattern) => matchesPathPattern(filePath, pattern)); +} + +const REVIEW_RULE_OVERLAYS: ReviewRuleOverlayDefinition[] = [ + { + id: "renderer-surface", + label: "Renderer surface", + description: "Renderer-facing changes should preserve visible flows, edge states, and the shared contracts they consume.", + pathPatterns: ["apps/desktop/src/renderer/**"], + rolloutExpectations: [ + "Check user-visible empty, loading, and error states.", + "Confirm renderer changes still match the shared data shape they consume.", + ], + companionFamilies: [ + { + id: "renderer", + label: "renderer surface", + pathPatterns: ["apps/desktop/src/renderer/**"], + }, + ], + promptGuidance: { + "diff-risk": [ + "Treat renderer changes as user-visible behavior changes, not just presentation edits.", + "Look for broken loading, empty, error, and optimistic states.", + ], + "cross-file-impact": [ + "Check whether renderer assumptions still match the shared types and IPC payloads it consumes.", + ], + }, + adjudicationPolicy: { + evidenceMode: "normal", + }, + }, + { + id: "preload-bridge", + label: "Preload bridge", + description: "Preload bridge changes must keep preload exports, IPC contracts, and renderer consumers aligned.", + pathPatterns: [ + "apps/desktop/src/preload/**", + "apps/desktop/src/preload/global.d.ts", + "apps/desktop/src/shared/ipc.ts", + ], + rolloutExpectations: [ + "Keep preload exports, `global.d.ts`, IPC contracts, and renderer call sites in sync.", + "Treat bridge mismatches as rollout gaps even when the changed file compiles in isolation.", + ], + companionFamilies: [ + { + id: "preload", + label: "preload bridge", + pathPatterns: [ + "apps/desktop/src/preload/**", + "apps/desktop/src/preload/global.d.ts", + ], + }, + { + id: "shared-ipc", + label: "shared IPC contract", + pathPatterns: ["apps/desktop/src/shared/ipc.ts"], + }, + { + id: "renderer-consumer", + label: "renderer consumer", + pathPatterns: ["apps/desktop/src/renderer/**"], + }, + ], + promptGuidance: { + "diff-risk": [ + "Treat preload or IPC edits as bridge changes that can silently break renderer access.", + ], + "cross-file-impact": [ + "Explicitly check preload exports, `global.d.ts`, shared IPC contracts, and renderer consumers together.", + ], + "checks-and-tests": [ + "Prefer validation evidence that proves the bridge stayed aligned across preload and renderer boundaries.", + ], + }, + adjudicationPolicy: { + evidenceMode: "cross_boundary", + }, + }, + { + id: "shared-contract", + label: "Shared contract", + description: "Shared contract and type changes should be reviewed as interface rollouts across their desktop consumers.", + pathPatterns: ["apps/desktop/src/shared/**"], + rolloutExpectations: [ + "Check whether shared contract changes were rolled out to preload, main, and renderer consumers.", + "Prefer concrete interface mismatches over speculative API concerns.", + ], + companionFamilies: [ + { + id: "shared", + label: "shared contract", + pathPatterns: ["apps/desktop/src/shared/**"], + }, + { + id: "preload-consumer", + label: "preload consumer", + pathPatterns: [ + "apps/desktop/src/preload/**", + "apps/desktop/src/preload/global.d.ts", + ], + }, + { + id: "renderer-consumer", + label: "renderer consumer", + pathPatterns: ["apps/desktop/src/renderer/**"], + }, + ], + promptGuidance: { + "diff-risk": [ + "Treat shared type or contract changes as interface changes that can break downstream callers.", + ], + "cross-file-impact": [ + "Look for missing rollout across preload, renderer, and other shared-contract consumers.", + ], + "checks-and-tests": [ + "Use validation evidence to confirm consumer updates actually landed for shared contract changes.", + ], + }, + adjudicationPolicy: { + evidenceMode: "cross_boundary", + }, + }, + { + id: "mcp-dual-path", + label: "MCP dual path", + description: "ADE MCP changes should keep the headless server path and the desktop socket-backed proxy path in sync.", + pathPatterns: [ + "apps/mcp-server/**", + "apps/desktop/src/main/adeMcpProxy.ts", + "apps/desktop/src/main/adeMcpProxyUtils.ts", + "apps/desktop/src/main/services/runtime/adeMcpLaunch.ts", + ], + rolloutExpectations: [ + "Check both headless MCP mode and the desktop socket-backed launch/proxy path.", + "Treat one-sided MCP changes as incomplete rollout unless the other path is intentionally unaffected and the diff proves it.", + ], + companionFamilies: [ + { + id: "mcp-server", + label: "headless MCP server", + pathPatterns: ["apps/mcp-server/**"], + }, + { + id: "desktop-mcp", + label: "desktop MCP proxy/runtime", + pathPatterns: [ + "apps/desktop/src/main/adeMcpProxy.ts", + "apps/desktop/src/main/adeMcpProxyUtils.ts", + "apps/desktop/src/main/services/runtime/adeMcpLaunch.ts", + ], + }, + ], + promptGuidance: { + "diff-risk": [ + "Treat MCP edits as transport or protocol changes that can break ADE-native tool execution.", + ], + "cross-file-impact": [ + "Before concluding there is no issue, explicitly consider both headless MCP behavior and the desktop socket-backed proxy path.", + ], + "checks-and-tests": [ + "Prefer evidence from existing MCP-adjacent validation instead of generic 'add tests' guidance.", + ], + }, + adjudicationPolicy: { + evidenceMode: "cross_boundary", + requireDualPathConsideration: true, + }, + }, +]; + +export function getReviewRuleOverlayDefinitions(): ReviewRuleOverlayDefinition[] { + return REVIEW_RULE_OVERLAYS.map((overlay) => ({ + ...overlay, + pathPatterns: [...overlay.pathPatterns], + rolloutExpectations: [...overlay.rolloutExpectations], + companionFamilies: overlay.companionFamilies.map((family) => ({ + ...family, + pathPatterns: [...family.pathPatterns], + })), + promptGuidance: Object.fromEntries( + Object.entries(overlay.promptGuidance).map(([passKey, guidance]) => [passKey, [...(guidance ?? [])]]), + ) as ReviewRuleOverlayDefinition["promptGuidance"], + })); +} + +export function matchReviewRuleOverlays(changedPaths: string[]): MatchedReviewRuleOverlay[] { + return getReviewRuleOverlayDefinitions().flatMap((overlay) => { + const matchedPaths = changedPaths.filter((filePath) => matchesAnyPattern(filePath, overlay.pathPatterns)); + if (matchedPaths.length === 0) return []; + const coveredFamilies = overlay.companionFamilies + .filter((family) => changedPaths.some((filePath) => matchesAnyPattern(filePath, family.pathPatterns))) + .map((family) => ({ id: family.id, label: family.label })); + const missingFamilies = overlay.companionFamilies + .filter((family) => !coveredFamilies.some((covered) => covered.id === family.id)) + .map((family) => ({ id: family.id, label: family.label })); + return [{ + ...overlay, + matchedPaths, + coveredFamilies, + missingFamilies, + }]; + }); +} + +export function overlayMatchesPath(overlay: Pick, filePath: string | null | undefined): boolean { + if (!filePath?.trim()) return false; + return matchesAnyPattern(filePath, overlay.pathPatterns); +} + +export function collectRulePromptGuidance(overlays: MatchedReviewRuleOverlay[], passKey: ReviewPassKey): string[] { + const guidance = overlays.flatMap((overlay) => overlay.promptGuidance[passKey] ?? []); + return Array.from(new Set(guidance.map((line) => line.trim()).filter(Boolean))); +} diff --git a/apps/desktop/src/main/services/review/reviewService.test.ts b/apps/desktop/src/main/services/review/reviewService.test.ts new file mode 100644 index 000000000..ac7ef3d75 --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewService.test.ts @@ -0,0 +1,1107 @@ +import path from "node:path"; +import { createRequire } from "node:module"; +import initSqlJs from "sql.js"; +import type { Database, SqlJsStatic } from "sql.js"; +import { beforeAll, beforeEach, describe, expect, it, vi } from "vitest"; +import type { ReviewPublicationDestination, ReviewRunConfig } from "../../../shared/types"; + +const mockMaterializer = vi.hoisted(() => ({ + materialize: vi.fn(), +})); + +const mockContextBuilder = vi.hoisted(() => ({ + buildContext: vi.fn(), +})); + +vi.mock("./reviewTargetMaterializer", () => ({ + createReviewTargetMaterializer: () => ({ + materialize: (...args: unknown[]) => mockMaterializer.materialize(...args), + }), +})); + +vi.mock("./reviewContextBuilder", () => ({ + createReviewContextBuilder: () => ({ + buildContext: (...args: unknown[]) => mockContextBuilder.buildContext(...args), + }), +})); + +import { createReviewService } from "./reviewService"; + +type SqlValue = string | number | null | Uint8Array; +type AdeDb = { + run: (sql: string, params?: SqlValue[]) => void; + get: = Record>(sql: string, params?: SqlValue[]) => T | null; + all: = Record>(sql: string, params?: SqlValue[]) => T[]; +}; + +function mapExecRows(rows: { columns: string[]; values: unknown[][] }[]): Record[] { + const first = rows[0]; + if (!first) return []; + return first.values.map((row) => { + const out: Record = {}; + first.columns.forEach((column, index) => { + out[column] = row[index]; + }); + return out; + }); +} + +let SQL: SqlJsStatic; + +beforeAll(async () => { + const require = createRequire(import.meta.url); + const wasmPath = require.resolve("sql.js/dist/sql-wasm.wasm"); + const wasmDir = path.dirname(wasmPath); + SQL = await initSqlJs({ + locateFile: (file) => path.join(wasmDir, file), + }); +}); + +function createInMemoryAdeDb(): { db: AdeDb; raw: Database } { + const raw = new SQL.Database(); + raw.run(` + create table review_runs( + id text primary key, + project_id text not null, + lane_id text not null, + target_json text not null, + config_json text not null, + target_label text not null, + compare_target_json text, + status text not null, + summary text, + error_message text, + finding_count integer not null default 0, + severity_summary_json text, + chat_session_id text, + created_at text not null, + started_at text not null, + ended_at text, + updated_at text not null + ) + `); + raw.run(` + create table review_findings( + id text primary key, + run_id text not null, + title text not null, + severity text not null, + finding_class text, + body text not null, + confidence real not null default 0.5, + evidence_json text, + file_path text, + line integer, + anchor_state text not null, + source_pass text not null, + publication_state text not null, + originating_passes_json text, + adjudication_json text, + diff_context_json text, + suppression_match_json text + ) + `); + raw.run(` + create table review_finding_feedback( + id text primary key, + finding_id text not null, + run_id text not null, + project_id text not null, + kind text not null, + reason text, + note text, + snooze_until text, + created_at text not null + ) + `); + raw.run(` + create table review_suppressions( + id text primary key, + project_id text not null, + scope text not null, + repo_key text, + path_pattern text, + title text not null, + title_norm text not null, + finding_class text, + severity text, + reason text, + note text, + embedding_json text, + source_finding_id text, + hit_count integer not null default 0, + created_at text not null, + last_matched_at text + ) + `); + raw.run(` + create table review_run_artifacts( + id text primary key, + run_id text not null, + artifact_type text not null, + title text not null, + mime_type text not null, + content_text text, + metadata_json text, + created_at text not null + ) + `); + raw.run(` + create table review_run_publications( + id text primary key, + run_id text not null, + destination_json text not null, + review_event text not null, + status text not null, + review_url text, + remote_review_id text, + summary_body text not null, + inline_comments_json text not null default '[]', + summary_finding_ids_json text not null default '[]', + error_message text, + created_at text not null, + updated_at text not null, + completed_at text + ) + `); + + const run = (sql: string, params: SqlValue[] = []) => raw.run(sql, params); + const all = = Record>(sql: string, params: SqlValue[] = []): T[] => + mapExecRows(raw.exec(sql, params)) as T[]; + const get = = Record>(sql: string, params: SqlValue[] = []): T | null => + all(sql, params)[0] ?? null; + + return { raw, db: { run, all, get } }; +} + +async function waitFor(fn: () => T | Promise, predicate: (value: T) => boolean, timeoutMs = 3000): Promise { + const started = Date.now(); + while (Date.now() - started < timeoutMs) { + const value = await fn(); + if (predicate(value)) return value; + await new Promise((resolve) => setTimeout(resolve, 25)); + } + throw new Error("Timed out waiting for review service state"); +} + +function makeConfig(overrides: Partial = {}): ReviewRunConfig { + return { + compareAgainst: { kind: "default_branch" }, + selectionMode: "full_diff", + dirtyOnly: false, + modelId: "openai/gpt-5.4-codex", + reasoningEffort: "medium", + budgets: { + maxFiles: 60, + maxDiffChars: 180_000, + maxPromptChars: 220_000, + maxFindings: 12, + maxFindingsPerPass: 6, + maxPublishedFindings: 6, + }, + publishBehavior: "local_only", + ...overrides, + }; +} + +function makeChangedFile(overrides: Partial<{ + filePath: string; + excerpt: string; + lineNumbers: number[]; + diffPositionsByLine: Record; +}> = {}) { + return { + filePath: "src/review.ts", + excerpt: "@@ -1,2 +1,4 @@\n context\n+return null;\n+missing fallback\n", + lineNumbers: [2, 3], + diffPositionsByLine: { 2: 1, 3: 2 }, + ...overrides, + }; +} + +function makeMaterializedTarget(overrides: Partial<{ + targetLabel: string; + publicationTarget: ReviewPublicationDestination | null; + fullPatchText: string; + changedFiles: ReturnType[]; +}> = {}) { + const fullPatchText = overrides.fullPatchText + ?? "diff --git a/src/review.ts b/src/review.ts\n@@ -1,2 +1,4 @@\n context\n+return null;\n+missing fallback\n"; + const changedFiles = overrides.changedFiles ?? [makeChangedFile()]; + return { + targetLabel: overrides.targetLabel ?? "feature/review-tab vs main", + compareTarget: { + kind: "default_branch", + label: "main", + ref: "main", + laneId: null, + branchRef: "main", + }, + publicationTarget: overrides.publicationTarget ?? null, + fullPatchText, + changedFiles, + artifacts: [ + { + artifactType: "diff_bundle", + title: "Diff bundle", + mimeType: "text/plain", + contentText: fullPatchText, + metadata: null, + }, + ], + }; +} + +function makeFinding(overrides: Partial> = {}): Record { + return { + title: "Missing fallback", + severity: "high", + body: "The branch returns null without the previous fallback path.", + confidence: 0.84, + filePath: "src/review.ts", + line: 2, + evidence: [ + { + summary: "The diff adds a direct null return.", + filePath: "src/review.ts", + line: 2, + quote: "+return null;", + }, + ], + ...overrides, + }; +} + +function makeOutput(summary: string, findings: Array>): string { + return JSON.stringify({ summary, findings }); +} + +function makeContextPacket(overrides: Partial> = {}) { + return { + matchedRuleOverlays: [], + provenance: { + summary: "1 mission, 1 late-stage signal", + prompt: "- Mission: keep renderer and preload behavior aligned.\n- Late-stage signal: recent validation failed on src/review.ts", + payload: { + changedPaths: ["src/review.ts"], + laneSnapshot: { + updatedAt: "2026-04-06T09:58:00.000Z", + agentSummary: "Recent ADE chat focused on restoring the review fallback path.", + missionSummary: "Finish the review engine rollout cleanly.", + }, + missions: [ + { + id: "mission-1", + title: "Restore review fallback behavior", + status: "running", + outcomeSummary: null, + intentSummary: "Keep fallback behavior and ship the review engine safely.", + updatedAt: "2026-04-06T09:59:00.000Z", + }, + ], + workerDigests: [ + { + id: "digest-1", + stepKey: "implement", + status: "succeeded", + summary: "Updated the review engine behavior.", + filesChanged: ["src/review.ts"], + testsSummary: "1 failed", + warnings: [], + createdAt: "2026-04-06T10:00:00.000Z", + }, + ], + sessionDeltas: [ + { + sessionId: "session-delta-1", + startedAt: "2026-04-06T09:55:00.000Z", + endedAt: "2026-04-06T09:57:00.000Z", + filesChanged: 1, + touchedFiles: ["src/review.ts"], + failureLines: ["AssertionError: fallback missing"], + computedAt: "2026-04-06T09:57:10.000Z", + }, + ], + priorReviews: [ + { + runId: "prior-run-1", + status: "completed", + summary: "Earlier ADE review flagged the same fallback path.", + findingCount: 1, + publicationCount: 0, + overlappingPaths: ["src/review.ts"], + createdAt: "2026-04-05T16:00:00.000Z", + }, + ], + lateStageSignals: [ + { + kind: "validation_failure_followed_by_edits", + summary: "Recent validation failed on src/review.ts before this edit.", + filePaths: ["src/review.ts"], + source: "session-delta-1", + occurredAt: "2026-04-06T09:57:10.000Z", + }, + ], + }, + metadata: { + summary: "1 mission, 1 worker digest, 1 session delta, 1 late-stage signal", + provenanceCount: 4, + missionCount: 1, + workerDigestCount: 1, + sessionDeltaCount: 1, + priorReviewCount: 1, + lateStageSignalCount: 1, + }, + }, + rules: { + summary: "1 rule overlay matched", + prompt: "- Preload bridge: matched src/review.ts; missing companion coverage: renderer consumer", + payload: { + changedPaths: ["src/review.ts"], + overlays: [], + }, + metadata: { + summary: "1 rule overlay matched", + matchedRuleCount: 1, + ruleCount: 1, + pathCount: 1, + matchedRuleIds: ["renderer-surface"], + }, + }, + validation: { + summary: "2 validation signals, 1 check, 1 test run", + prompt: "- Validation signal: unit-tests failure\n- Validation signal: fallback assertion failed", + payload: { + linkedPr: null, + reviewSnapshot: null, + checks: [ + { + name: "unit-tests", + status: "completed", + conclusion: "failure", + detailsUrl: "https://ci.example/unit-tests", + startedAt: "2026-04-06T10:00:00.000Z", + completedAt: "2026-04-06T10:03:00.000Z", + }, + ], + suites: ["unit"], + testRuns: [ + { + runId: "test-run-1", + suiteId: "unit", + suiteName: "Unit", + status: "failed", + exitCode: 1, + startedAt: "2026-04-06T10:00:00.000Z", + endedAt: "2026-04-06T10:02:00.000Z", + logExcerpt: "AssertionError: fallback missing", + }, + ], + issueInventory: [], + sessionFailures: [ + { + sessionId: "session-delta-1", + touchedFiles: ["src/review.ts"], + failureLines: ["AssertionError: fallback missing"], + computedAt: "2026-04-06T09:57:10.000Z", + }, + ], + signals: [ + { + kind: "pr_check_failure", + summary: "unit-tests: completed / failure", + filePaths: [], + sourceId: "unit-tests", + }, + { + kind: "session_failure", + summary: "AssertionError: fallback missing", + filePaths: ["src/review.ts"], + sourceId: "session-delta-1", + }, + ], + }, + metadata: { + summary: "2 validation signals, 1 check, 1 test run", + signalCount: 2, + checkCount: 1, + testRunCount: 1, + issueCount: 0, + sessionFailureCount: 1, + suiteCount: 1, + }, + }, + ...overrides, + }; +} + +function buildPublicationResult(args: { + runId: string; + destination: ReviewPublicationDestination; + findings: Array<{ id: string; filePath: string | null; line: number | null }>; +}) { + const firstInline = args.findings.find((finding) => finding.filePath && finding.line != null) ?? null; + return { + id: `publication-${args.runId}`, + runId: args.runId, + destination: args.destination, + reviewEvent: "COMMENT" as const, + status: "published" as const, + reviewUrl: "https://github.com/ade-dev/ade/pull/80#pullrequestreview-1", + remoteReviewId: "1", + summaryBody: "Summary body", + inlineComments: firstInline ? [{ + findingId: firstInline.id, + path: firstInline.filePath ?? "src/review.ts", + line: firstInline.line ?? 1, + position: 2, + body: "Inline comment", + }] : [], + summaryFindingIds: args.findings.filter((finding) => !firstInline || finding.id !== firstInline.id).map((finding) => finding.id), + errorMessage: null, + createdAt: "2026-04-06T10:00:00.000Z", + updatedAt: "2026-04-06T10:00:02.000Z", + completedAt: "2026-04-06T10:00:02.000Z", + }; +} + +function createHarness(args: { + outputs: string[]; + targetLabel?: string; + publicationTarget?: ReviewPublicationDestination | null; + config?: Partial; + target?: { mode: "lane_diff" | "pr" | "commit_range" | "working_tree"; laneId: string; prId?: string; baseCommit?: string; headCommit?: string }; +}) { + const { db, raw } = createInMemoryAdeDb(); + let sessionCount = 0; + const queuedOutputs = [...args.outputs]; + const publishReviewPublication = vi.fn(async (input: any) => buildPublicationResult({ + runId: input.runId, + destination: input.destination, + findings: input.findings, + })); + + mockMaterializer.materialize.mockResolvedValue(makeMaterializedTarget({ + targetLabel: args.targetLabel, + publicationTarget: args.publicationTarget ?? null, + })); + mockContextBuilder.buildContext.mockResolvedValue(makeContextPacket()); + + const runSessionTurn = vi.fn(async () => { + const outputText = queuedOutputs.shift(); + if (!outputText) throw new Error("No mock review output left."); + return { + sessionId: `session-review-${sessionCount}`, + provider: "codex", + model: "gpt-5.4-codex", + modelId: "openai/gpt-5.4-codex", + outputText, + }; + }); + + const service = createReviewService({ + db: db as any, + logger: { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + } as any, + projectId: "project-1", + projectRoot: "/tmp/ade", + projectDefaultBranch: "main", + laneService: { + getLaneBaseAndBranch: vi.fn().mockReturnValue({ + baseRef: "main", + branchRef: "feature/review-tab", + worktreePath: "/tmp/ade/lane", + laneType: "worktree", + }), + getStateSnapshot: vi.fn().mockReturnValue(null), + list: vi.fn(async () => []), + } as any, + gitService: { + listRecentCommits: vi.fn(async () => []), + } as any, + agentChatService: { + createSession: vi.fn(async () => { + sessionCount += 1; + return { + id: `session-review-${sessionCount}`, + laneId: "lane-review", + provider: "codex", + model: "gpt-5.4-codex", + modelId: "openai/gpt-5.4-codex", + }; + }), + getSessionSummary: vi.fn(async (sessionId: string) => ({ + sessionId, + laneId: "lane-review", + provider: "codex", + model: "gpt-5.4-codex", + modelId: "openai/gpt-5.4-codex", + title: "Review transcript", + surface: "automation", + status: "idle", + startedAt: "2026-04-05T12:00:00.000Z", + endedAt: null, + lastActivityAt: "2026-04-05T12:05:00.000Z", + lastOutputPreview: "Review output", + summary: "Saved review transcript", + })), + runSessionTurn, + } as any, + sessionService: { + updateMeta: vi.fn(), + } as any, + sessionDeltaService: { + listRecentLaneSessionDeltas: vi.fn().mockReturnValue([]), + } as any, + testService: { + listRuns: vi.fn().mockReturnValue([]), + getLogTail: vi.fn().mockReturnValue(""), + listSuites: vi.fn().mockReturnValue([]), + } as any, + issueInventoryService: { + getInventory: vi.fn().mockReturnValue({ prId: "pr-80", items: [], convergence: {}, runtime: {} }), + } as any, + prService: args.publicationTarget ? { + getReviewSnapshot: vi.fn(), + getChecks: vi.fn(async () => [ + { + name: "unit-tests", + status: "completed", + conclusion: "failure", + detailsUrl: "https://ci.example/unit-tests", + startedAt: "2026-04-06T10:00:00.000Z", + completedAt: "2026-04-06T10:03:00.000Z", + }, + ]), + publishReviewPublication, + } as any : undefined, + }); + + const target = args.target?.mode === "pr" + ? { mode: "pr" as const, laneId: args.target.laneId, prId: args.target.prId ?? "pr-80" } + : args.target?.mode === "commit_range" + ? { + mode: "commit_range" as const, + laneId: args.target.laneId, + baseCommit: args.target.baseCommit ?? "abc123456789", + headCommit: args.target.headCommit ?? "def456789012", + } + : args.target?.mode === "working_tree" + ? { mode: "working_tree" as const, laneId: args.target.laneId } + : { mode: "lane_diff" as const, laneId: args.target?.laneId ?? "lane-review" }; + + return { + raw, + service, + runSessionTurn, + publishReviewPublication, + start: (config?: Partial) => service.startRun({ + target, + config: makeConfig({ + ...(args.config ?? {}), + ...(config ?? {}), + }), + }), + }; +} + +describe("reviewService", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("merges overlapping multi-pass findings and persists the pass-level artifact trail", async () => { + const harness = createHarness({ + outputs: [ + makeOutput("Direct diff risk.", [ + makeFinding(), + ]), + makeOutput("Cross-file corroboration.", [ + makeFinding({ + title: "Fallback path removed", + body: "The previous fallback is gone, so downstream callers can now receive null.", + confidence: 0.79, + }), + ]), + makeOutput("No extra check findings.", []), + ], + }); + + const run = await harness.start(); + await waitFor( + () => harness.service.listRuns(), + (runs) => runs[0]?.status === "completed", + ); + + const detail = await harness.service.getRunDetail({ runId: run.id }); + expect(detail?.findingCount).toBe(1); + expect(detail?.findings[0]?.sourcePass).toBe("adjudicated"); + expect(detail?.findings[0]?.originatingPasses).toEqual(["diff-risk", "cross-file-impact"]); + expect(detail?.findings[0]?.adjudication?.mergedFindingIds).toHaveLength(2); + expect(detail?.artifacts.filter((artifact) => artifact.artifactType === "pass_prompt")).toHaveLength(3); + expect(detail?.artifacts.filter((artifact) => artifact.artifactType === "pass_output")).toHaveLength(3); + expect(detail?.artifacts.filter((artifact) => artifact.artifactType === "pass_findings")).toHaveLength(3); + expect(detail?.artifacts.some((artifact) => artifact.artifactType === "adjudication_result")).toBe(true); + expect(detail?.artifacts.some((artifact) => artifact.artifactType === "merged_findings")).toBe(true); + + const persistedFindings = mapExecRows(harness.raw.exec("select source_pass, originating_passes_json, adjudication_json from review_findings")); + expect(String(persistedFindings[0]?.source_pass)).toBe("adjudicated"); + expect(String(persistedFindings[0]?.originating_passes_json)).toContain("diff-risk"); + expect(String(persistedFindings[0]?.adjudication_json)).toContain("publicationEligible"); + }); + + it("persists provenance, rules, and validation artifacts and keeps renderer findings on the normal evidence path", async () => { + const harness = createHarness({ + outputs: [ + makeOutput("Renderer regression.", [ + makeFinding({ + title: "Fallback intent drift", + findingClass: "intent_drift", + }), + ]), + makeOutput("No cross-file issues.", []), + makeOutput("No checks issues.", []), + ], + }); + mockContextBuilder.buildContext.mockResolvedValueOnce(makeContextPacket({ + matchedRuleOverlays: [ + { + id: "renderer-surface", + label: "Renderer surface", + description: "Renderer rule", + pathPatterns: ["src/review.ts"], + rolloutExpectations: ["Check user-visible fallback behavior."], + companionFamilies: [{ id: "renderer", label: "renderer", pathPatterns: ["src/review.ts"] }], + promptGuidance: { "diff-risk": ["Treat this as a visible renderer flow."] }, + adjudicationPolicy: { evidenceMode: "normal" }, + matchedPaths: ["src/review.ts"], + coveredFamilies: [{ id: "renderer", label: "renderer" }], + missingFamilies: [], + }, + ], + rules: { + summary: "1 rule overlay matched", + prompt: "- Renderer surface: check user-visible fallback behavior.", + payload: { + changedPaths: ["src/review.ts"], + overlays: [{ + id: "renderer-surface", + label: "Renderer surface", + description: "Renderer rule", + matchedPaths: ["src/review.ts"], + rolloutExpectations: ["Check user-visible fallback behavior."], + coveredFamilies: [{ id: "renderer", label: "renderer" }], + missingFamilies: [], + adjudicationPolicy: { evidenceMode: "normal" }, + }], + }, + metadata: { + summary: "1 rule overlay matched", + matchedRuleCount: 1, + ruleCount: 1, + pathCount: 1, + matchedRuleIds: ["renderer-surface"], + }, + }, + })); + + const run = await harness.start(); + await waitFor(() => harness.service.listRuns(), (runs) => runs[0]?.status === "completed"); + + const detail = await harness.service.getRunDetail({ runId: run.id }); + expect(detail?.findings).toHaveLength(1); + expect(detail?.findings[0]?.findingClass).toBe("intent_drift"); + expect(detail?.artifacts.some((artifact) => artifact.artifactType === "provenance_brief")).toBe(true); + expect(detail?.artifacts.some((artifact) => artifact.artifactType === "rule_overlays")).toBe(true); + expect(detail?.artifacts.some((artifact) => artifact.artifactType === "validation_signals")).toBe(true); + + const harnessArtifact = detail?.artifacts.find((artifact) => artifact.artifactType === "prompt"); + expect(harnessArtifact?.metadata).toMatchObject({ + matchedRuleCount: 1, + provenanceCount: 4, + validationSignalCount: 2, + matchedRuleIds: ["renderer-surface"], + }); + + const firstPassPrompt = detail?.artifacts.find((artifact) => artifact.artifactType === "pass_prompt"); + expect(firstPassPrompt?.contentText).toContain("provenance_brief artifact id"); + expect(firstPassPrompt?.contentText).toContain("rule_overlays artifact id"); + + const persisted = mapExecRows(harness.raw.exec("select finding_class from review_findings")); + expect(String(persisted[0]?.finding_class)).toBe("intent_drift"); + }); + + it("rejects strict preload/shared findings without cross-boundary or provenance-backed evidence", async () => { + const harness = createHarness({ + outputs: [ + makeOutput("Bridge mismatch.", [ + makeFinding({ + title: "Bridge rollout incomplete", + findingClass: "incomplete_rollout", + evidence: [ + { + summary: "The preload diff changes the exposed method name.", + filePath: "src/review.ts", + line: 2, + quote: "+exposeReviewV2()", + }, + ], + }), + ]), + makeOutput("No cross-file issues.", []), + makeOutput("No checks issues.", []), + ], + }); + mockContextBuilder.buildContext.mockResolvedValueOnce(makeContextPacket({ + provenance: { + summary: "No ADE provenance context", + prompt: "- No ADE provenance or intent context was available.", + payload: { + changedPaths: ["src/review.ts"], + laneSnapshot: null, + missions: [], + workerDigests: [], + sessionDeltas: [], + priorReviews: [], + lateStageSignals: [], + }, + metadata: { + summary: "No ADE provenance context", + provenanceCount: 0, + missionCount: 0, + workerDigestCount: 0, + sessionDeltaCount: 0, + priorReviewCount: 0, + lateStageSignalCount: 0, + }, + }, + matchedRuleOverlays: [ + { + id: "preload-bridge", + label: "Preload bridge", + description: "Strict bridge rule", + pathPatterns: ["src/review.ts"], + rolloutExpectations: ["Keep bridge and consumer updates aligned."], + companionFamilies: [ + { id: "preload", label: "preload", pathPatterns: ["src/review.ts"] }, + { id: "renderer", label: "renderer", pathPatterns: ["src/renderer.ts"] }, + ], + promptGuidance: { "cross-file-impact": ["Check both sides of the bridge."] }, + adjudicationPolicy: { evidenceMode: "cross_boundary" }, + matchedPaths: ["src/review.ts"], + coveredFamilies: [{ id: "preload", label: "preload" }], + missingFamilies: [{ id: "renderer", label: "renderer" }], + }, + ], + rules: { + summary: "1 rule overlay matched", + prompt: "- Preload bridge: missing companion coverage: renderer", + payload: { + changedPaths: ["src/review.ts"], + overlays: [{ + id: "preload-bridge", + label: "Preload bridge", + description: "Strict bridge rule", + matchedPaths: ["src/review.ts"], + rolloutExpectations: ["Keep bridge and consumer updates aligned."], + coveredFamilies: [{ id: "preload", label: "preload" }], + missingFamilies: [{ id: "renderer", label: "renderer" }], + adjudicationPolicy: { evidenceMode: "cross_boundary" }, + }], + }, + metadata: { + summary: "1 rule overlay matched", + matchedRuleCount: 1, + ruleCount: 1, + pathCount: 1, + matchedRuleIds: ["preload-bridge"], + }, + }, + validation: { + summary: "No validation signals", + prompt: "- No prior ADE validation signals were available.", + payload: { + linkedPr: null, + reviewSnapshot: null, + checks: [], + suites: [], + testRuns: [], + issueInventory: [], + sessionFailures: [], + signals: [], + }, + metadata: { + summary: "No validation signals", + signalCount: 0, + checkCount: 0, + testRunCount: 0, + issueCount: 0, + sessionFailureCount: 0, + suiteCount: 0, + }, + }, + })); + + const run = await harness.start(); + await waitFor(() => harness.service.listRuns(), (runs) => runs[0]?.status === "completed"); + + const detail = await harness.service.getRunDetail({ runId: run.id }); + expect(detail?.findings).toEqual([]); + const adjudicationArtifact = detail?.artifacts.find((artifact) => artifact.artifactType === "adjudication_result"); + expect(adjudicationArtifact?.contentText).toContain("rule_policy"); + }); + + it("cites validation and provenance artifacts when late-stage signals back the finding", async () => { + const harness = createHarness({ + outputs: [ + makeOutput("No diff-risk issues.", []), + makeOutput("No cross-file issues.", []), + makeOutput("Validation-backed regression.", [ + makeFinding({ + title: "Fallback broke after the failed validation loop", + filePath: "src/review.ts", + line: 2, + evidence: [ + { + summary: "The diff still returns null directly.", + filePath: "src/review.ts", + line: 2, + quote: "+return null;", + }, + ], + }), + ]), + ], + }); + mockContextBuilder.buildContext.mockResolvedValueOnce(makeContextPacket({ + matchedRuleOverlays: [ + { + id: "renderer-surface", + label: "Renderer surface", + description: "Renderer rule", + pathPatterns: ["src/review.ts"], + rolloutExpectations: ["Check the fallback behavior."], + companionFamilies: [{ id: "renderer", label: "renderer", pathPatterns: ["src/review.ts"] }], + promptGuidance: {}, + adjudicationPolicy: { evidenceMode: "normal" }, + matchedPaths: ["src/review.ts"], + coveredFamilies: [{ id: "renderer", label: "renderer" }], + missingFamilies: [], + }, + ], + rules: { + summary: "1 rule overlay matched", + prompt: "- Renderer surface: check fallback behavior.", + payload: { + changedPaths: ["src/review.ts"], + overlays: [{ + id: "renderer-surface", + label: "Renderer surface", + description: "Renderer rule", + matchedPaths: ["src/review.ts"], + rolloutExpectations: ["Check the fallback behavior."], + coveredFamilies: [{ id: "renderer", label: "renderer" }], + missingFamilies: [], + adjudicationPolicy: { evidenceMode: "normal" }, + }], + }, + metadata: { + summary: "1 rule overlay matched", + matchedRuleCount: 1, + ruleCount: 1, + pathCount: 1, + matchedRuleIds: ["renderer-surface"], + }, + }, + })); + + const run = await harness.start(); + await waitFor(() => harness.service.listRuns(), (runs) => runs[0]?.status === "completed"); + + const detail = await harness.service.getRunDetail({ runId: run.id }); + expect(detail?.findings).toHaveLength(1); + expect(detail?.findings[0]?.findingClass).toBe("late_stage_regression"); + expect(detail?.findings[0]?.evidence.some((entry) => entry.artifactId && entry.artifactId.length > 0)).toBe(true); + const artifactKinds = new Set(detail?.findings[0]?.evidence.map((entry) => entry.artifactId).filter(Boolean)); + expect(artifactKinds.size).toBeGreaterThanOrEqual(2); + }); + + it("uses late-stage regression when overlapping passes disagree on ADE-native class", async () => { + const harness = createHarness({ + outputs: [ + makeOutput("Intent drift.", [ + makeFinding({ + title: "Fallback intent drift", + findingClass: "intent_drift", + }), + ]), + makeOutput("Late-stage corroboration.", [ + makeFinding({ + title: "Fallback intent drift", + findingClass: "late_stage_regression", + body: "The same fallback regression reappeared after the failed validation loop.", + confidence: 0.79, + }), + ]), + makeOutput("No checks issues.", []), + ], + }); + + const run = await harness.start(); + await waitFor(() => harness.service.listRuns(), (runs) => runs[0]?.status === "completed"); + + const detail = await harness.service.getRunDetail({ runId: run.id }); + expect(detail?.findings).toHaveLength(1); + expect(detail?.findings[0]?.findingClass).toBe("late_stage_regression"); + }); + + it("filters weak findings that do not carry concrete evidence", async () => { + const harness = createHarness({ + outputs: [ + makeOutput("Weak comment.", [ + makeFinding({ + title: "Maybe rename this helper", + severity: "low", + body: "This name could be clearer for future readers.", + confidence: 0.31, + filePath: null, + line: null, + evidence: [], + }), + ]), + makeOutput("No cross-file issues.", []), + makeOutput("No checks issues.", []), + ], + }); + + const run = await harness.start(); + await waitFor( + () => harness.service.listRuns(), + (runs) => runs[0]?.status === "completed", + ); + + const detail = await harness.service.getRunDetail({ runId: run.id }); + expect(detail?.findings).toEqual([]); + expect(detail?.summary).toContain("filtered out during adjudication"); + + const adjudicationArtifact = detail?.artifacts.find((artifact) => artifact.artifactType === "adjudication_result"); + expect(adjudicationArtifact?.contentText).toContain("low_evidence"); + }); + + it("applies run and publication budgets and only publishes adjudicated findings", async () => { + const destination: ReviewPublicationDestination = { + kind: "github_pr_review", + prId: "pr-80", + repoOwner: "ade-dev", + repoName: "ade", + prNumber: 80, + githubUrl: "https://github.com/ade-dev/ade/pull/80", + }; + const harness = createHarness({ + publicationTarget: destination, + targetLabel: "PR #80 feature/pr-80 -> main", + target: { mode: "pr", laneId: "lane-review", prId: "pr-80" }, + config: { + publishBehavior: "auto_publish", + budgets: { + maxFiles: 60, + maxDiffChars: 180_000, + maxPromptChars: 220_000, + maxFindings: 2, + maxFindingsPerPass: 4, + maxPublishedFindings: 1, + }, + }, + outputs: [ + makeOutput("Diff-risk findings.", [ + makeFinding({ title: "Null fallback removed", filePath: "src/review.ts", line: 2 }), + makeFinding({ title: "Summary-only risk", severity: "medium", filePath: "src/review.ts", line: 200, evidence: [{ summary: "The diff changes behavior without a regression test.", filePath: "src/review.ts", line: 200, quote: "+missing test coverage" }] }), + ]), + makeOutput("Cross-file overlap.", [ + makeFinding({ title: "Fallback path removed", body: "Downstream callers now receive null.", filePath: "src/review.ts", line: 2, confidence: 0.76 }), + makeFinding({ + title: "Dispatch invariant removed", + severity: "high", + body: "The worker dispatch path now skips the invariant check before enqueueing work.", + filePath: "src/worker.ts", + line: 10, + evidence: [{ summary: "The patch drops the invariant guard in the worker path.", filePath: "src/worker.ts", line: 10, quote: "+dispatchWithoutInvariant()" }], + }), + ]), + makeOutput("Checks and tests.", [ + makeFinding({ title: "Missing regression coverage", severity: "medium", filePath: "src/review.ts", line: 200, evidence: [{ summary: "A failing unit check suggests the diff lacks the previous safety net.", filePath: "src/review.ts", line: 200, quote: "unit-tests: completed / failure" }] }), + ]), + ], + }); + + const run = await harness.start(); + await waitFor( + () => harness.service.listRuns(), + (runs) => runs[0]?.status === "completed", + ); + + expect(harness.publishReviewPublication).toHaveBeenCalledTimes(1); + const publicationArgs = harness.publishReviewPublication.mock.calls[0]?.[0]; + expect(publicationArgs.findings).toHaveLength(1); + expect(publicationArgs.findings[0]?.sourcePass).toBe("adjudicated"); + + const detail = await harness.service.getRunDetail({ runId: run.id }); + expect(detail?.findings).toHaveLength(2); + expect(detail?.findings.filter((finding) => finding.publicationState === "published")).toHaveLength(1); + expect(detail?.publications).toHaveLength(1); + expect(detail?.artifacts.some((artifact) => artifact.artifactType === "publication_request")).toBe(true); + }); + + it("reruns a saved multi-pass review through the same shared engine", async () => { + const harness = createHarness({ + outputs: [ + makeOutput("First pass run.", [makeFinding()]), + makeOutput("Cross-file overlap.", [makeFinding({ title: "Fallback path removed", confidence: 0.71 })]), + makeOutput("Checks clear.", []), + makeOutput("Second run diff-risk.", [makeFinding()]), + makeOutput("Second run cross-file.", [makeFinding({ title: "Fallback path removed", confidence: 0.71 })]), + makeOutput("Second run checks.", []), + ], + target: { + mode: "commit_range", + laneId: "lane-review", + baseCommit: "abc123456789", + headCommit: "def456789012", + }, + config: { + selectionMode: "selected_commits", + reasoningEffort: "high", + }, + }); + + const first = await harness.start(); + await waitFor( + () => harness.service.listRuns(), + (runs) => runs.some((run) => run.id === first.id && run.status === "completed"), + ); + + const rerun = await harness.service.rerun(first.id); + await waitFor( + () => harness.service.listRuns(), + (runs) => runs.some((run) => run.id === rerun.id && run.status === "completed"), + ); + + expect(harness.runSessionTurn).toHaveBeenCalledTimes(6); + const rerunDetail = await harness.service.getRunDetail({ runId: rerun.id }); + expect(rerunDetail?.artifacts.filter((artifact) => artifact.artifactType === "pass_prompt")).toHaveLength(3); + + const rerunRecord = (await harness.service.listRuns()).find((entry) => entry.id === rerun.id); + expect(rerunRecord?.target).toEqual({ + mode: "commit_range", + laneId: "lane-review", + baseCommit: "abc123456789", + headCommit: "def456789012", + }); + expect(rerunRecord?.config.selectionMode).toBe("selected_commits"); + expect(rerunRecord?.config.reasoningEffort).toBe("high"); + }); +}); diff --git a/apps/desktop/src/main/services/review/reviewService.ts b/apps/desktop/src/main/services/review/reviewService.ts new file mode 100644 index 000000000..b9a1c9686 --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewService.ts @@ -0,0 +1,2479 @@ +import { randomUUID } from "node:crypto"; +import type { + ReviewArtifactType, + ReviewDiffContext, + ReviewDismissReason, + ReviewEventPayload, + ReviewEvidence, + ReviewFeedbackKind, + ReviewFeedbackRecord, + ReviewFinding, + ReviewFindingAdjudication, + ReviewFindingClass, + ReviewFindingSuppressionMatch, + ReviewListSuppressionsArgs, + ReviewPublication, + ReviewPublicationDestination, + ReviewPublicationInlineComment, + ReviewPublicationState, + ReviewQualityReport, + ReviewRecordFeedbackArgs, + ReviewResolvedCompareTarget, + ReviewRun, + ReviewRunArtifact, + ReviewRunConfig, + ReviewRunDetail, + ReviewRunStatus, + ReviewPassKey, + ReviewSeverity, + ReviewSeveritySummary, + ReviewSourcePass, + ReviewStartRunArgs, + ReviewSuppression, + ReviewSuppressionScope, + ReviewTarget, + ReviewListRunsArgs, + ReviewLaunchContext, + ReviewLaunchLane, + ReviewLaunchCommit, +} from "../../../shared/types"; +import { getDefaultModelDescriptor, getModelById, resolveChatProviderForDescriptor } from "../../../shared/modelRegistry"; +import type { Logger } from "../logging/logger"; +import type { AdeDb } from "../state/kvDb"; +import { getErrorMessage, isRecord, nowIso, safeJsonParse } from "../shared/utils"; +import type { createLaneService } from "../lanes/laneService"; +import type { createGitOperationsService } from "../git/gitOperationsService"; +import type { createAgentChatService } from "../chat/agentChatService"; +import type { createSessionService } from "../sessions/sessionService"; +import type { createSessionDeltaService } from "../sessions/sessionDeltaService"; +import type { createPrService } from "../prs/prService"; +import type { createIssueInventoryService } from "../prs/issueInventoryService"; +import type { createTestService } from "../tests/testService"; +import { createReviewTargetMaterializer } from "./reviewTargetMaterializer"; +import { createReviewContextBuilder, type ReviewContextPacket } from "./reviewContextBuilder"; +import { + collectRulePromptGuidance, + overlayMatchesPath, + type MatchedReviewRuleOverlay, +} from "./reviewRuleRegistry"; +import { createReviewSuppressionService, type ReviewSuppressionService } from "./reviewSuppressionService"; +import { buildDiffContextForFinding } from "./reviewDiffContext"; +import { buildToolBackedEvidence } from "./reviewToolEvidence"; +import type { EmbeddingService } from "../memory/embeddingService"; + +type ReviewRunRow = { + id: string; + project_id: string; + lane_id: string; + target_json: string; + config_json: string; + target_label: string; + compare_target_json: string | null; + status: string; + summary: string | null; + error_message: string | null; + finding_count: number; + severity_summary_json: string | null; + chat_session_id: string | null; + created_at: string; + started_at: string; + ended_at: string | null; + updated_at: string; +}; + +type ReviewFindingRow = { + id: string; + run_id: string; + title: string; + severity: string; + finding_class: string | null; + body: string; + confidence: number; + evidence_json: string | null; + file_path: string | null; + line: number | null; + anchor_state: string; + source_pass: string; + publication_state: string; + originating_passes_json: string | null; + adjudication_json: string | null; + diff_context_json: string | null; + suppression_match_json: string | null; +}; + +type ReviewFindingFeedbackRow = { + id: string; + finding_id: string; + run_id: string; + project_id: string; + kind: string; + reason: string | null; + note: string | null; + snooze_until: string | null; + created_at: string; +}; + +type ReviewRunArtifactRow = { + id: string; + run_id: string; + artifact_type: string; + title: string; + mime_type: string; + content_text: string | null; + metadata_json: string | null; + created_at: string; +}; + +type ReviewRunPublicationRow = { + id: string; + run_id: string; + destination_json: string; + review_event: string; + status: string; + review_url: string | null; + remote_review_id: string | null; + summary_body: string; + inline_comments_json: string; + summary_finding_ids_json: string; + error_message: string | null; + created_at: string; + updated_at: string; + completed_at: string | null; +}; + +const REVIEW_MODEL_FALLBACK_ID = "openai/gpt-5.4-codex"; + +function resolveBuiltinReviewModelId(): string { + const candidates = [ + getDefaultModelDescriptor("codex")?.id ?? null, + getDefaultModelDescriptor("opencode")?.id ?? null, + REVIEW_MODEL_FALLBACK_ID, + getDefaultModelDescriptor("claude")?.id ?? null, + getDefaultModelDescriptor("cursor")?.id ?? null, + ].filter((modelId): modelId is string => Boolean(modelId?.trim())); + + for (const modelId of candidates) { + const descriptor = getModelById(modelId); + if (descriptor) return descriptor.id; + } + + return REVIEW_MODEL_FALLBACK_ID; +} + +const DEFAULT_REVIEW_MODEL_ID = resolveBuiltinReviewModelId(); + +const DEFAULT_BUDGETS: ReviewRunConfig["budgets"] = { + maxFiles: 60, + maxDiffChars: 180_000, + maxPromptChars: 220_000, + maxFindings: 12, + maxFindingsPerPass: 6, + maxPublishedFindings: 6, +}; + +const REVIEW_PASS_ORDER: ReviewPassKey[] = [ + "diff-risk", + "cross-file-impact", + "checks-and-tests", +]; + +const SEVERITY_SCORE: Record = { + critical: 5, + high: 4, + medium: 3, + low: 2, + info: 1, +}; + +type MaterializedChangedFile = { + filePath: string; + excerpt: string; + lineNumbers: number[]; + diffPositionsByLine: Record; +}; + +type PassDefinition = { + key: ReviewPassKey; + label: string; + focus: string; + extraInstructions: string[]; +}; + +type PassCandidateFinding = { + id: string; + runId: string; + passKey: ReviewPassKey; + title: string; + severity: ReviewSeverity; + findingClass: ReviewFindingClass | null; + body: string; + confidence: number; + evidence: ReviewEvidence[]; + filePath: string | null; + line: number | null; + anchorState: ReviewFinding["anchorState"]; + evidenceScore: number; + lowSignal: boolean; + score: number; +}; + +type ReviewContextArtifactIds = { + provenanceArtifactId: string; + rulesArtifactId: string; + validationArtifactId: string; +}; + +type PassExecutionResult = { + pass: PassDefinition; + summary: string | null; + candidates: PassCandidateFinding[]; + promptArtifactId: string; + outputArtifactId: string; + findingsArtifactId: string; + budgetTrimmedCount: number; +}; + +type AdjudicationRejectedFinding = { + candidateIds: string[]; + passKeys: ReviewPassKey[]; + title: string; + reason: "low_evidence" | "low_signal" | "duplicate" | "budget" | "rule_policy"; + detail: string; + score: number; +}; + +type AdjudicationOutcome = { + summary: string; + findings: ReviewFinding[]; + rejected: AdjudicationRejectedFinding[]; + publicationEligibleCount: number; + totalCandidateCount: number; +}; + +function defaultSeveritySummary(): ReviewSeveritySummary { + return { + critical: 0, + high: 0, + medium: 0, + low: 0, + info: 0, + }; +} + +function clampNumber(value: number, min: number, max: number): number { + return Math.max(min, Math.min(max, value)); +} + +function truncateText(value: string, maxChars: number): string { + if (value.length <= maxChars) return value; + return `${value.slice(0, maxChars)}\n...(truncated)...\n`; +} + +function cleanLine(value: string): string { + return value.replace(/\s+/g, " ").trim(); +} + +function normalizeSeverity(value: unknown): ReviewSeverity { + const raw = typeof value === "string" ? value.trim().toLowerCase() : ""; + if (raw === "critical") return "critical"; + if (raw === "high" || raw === "major") return "high"; + if (raw === "medium" || raw === "moderate") return "medium"; + if (raw === "low" || raw === "minor") return "low"; + return "info"; +} + +function normalizeConfidence(value: unknown): number { + if (typeof value === "number" && Number.isFinite(value)) { + return clampNumber(value, 0, 1); + } + if (typeof value === "string") { + const raw = value.trim().toLowerCase(); + if (raw === "high") return 0.85; + if (raw === "medium") return 0.65; + if (raw === "low") return 0.35; + const parsed = Number(raw); + if (Number.isFinite(parsed)) return clampNumber(parsed, 0, 1); + } + return 0.5; +} + +function normalizeFindingClass(value: unknown): ReviewFindingClass | null { + const raw = typeof value === "string" ? value.trim().toLowerCase().replace(/[\s-]+/g, "_") : ""; + if (raw === "intent_drift") return "intent_drift"; + if (raw === "incomplete_rollout") return "incomplete_rollout"; + if (raw === "late_stage_regression") return "late_stage_regression"; + return null; +} + +const FINDING_CLASS_PRIORITY: ReviewFindingClass[] = [ + "late_stage_regression", + "incomplete_rollout", + "intent_drift", +]; + +function mergeFindingClass(classes: Array): ReviewFindingClass | null { + for (const findingClass of FINDING_CLASS_PRIORITY) { + if (classes.some((candidate) => candidate === findingClass)) { + return findingClass; + } + } + return null; +} + +function normalizeBudgetConfig(budgets?: Partial | null): ReviewRunConfig["budgets"] { + return { + maxFiles: clampNumber(Number(budgets?.maxFiles ?? DEFAULT_BUDGETS.maxFiles), 1, 500), + maxDiffChars: clampNumber(Number(budgets?.maxDiffChars ?? DEFAULT_BUDGETS.maxDiffChars), 4_000, 1_000_000), + maxPromptChars: clampNumber(Number(budgets?.maxPromptChars ?? DEFAULT_BUDGETS.maxPromptChars), 4_000, 1_000_000), + maxFindings: clampNumber(Number(budgets?.maxFindings ?? DEFAULT_BUDGETS.maxFindings), 1, 50), + maxFindingsPerPass: clampNumber( + Number(budgets?.maxFindingsPerPass ?? DEFAULT_BUDGETS.maxFindingsPerPass ?? DEFAULT_BUDGETS.maxFindings), + 1, + 50, + ), + maxPublishedFindings: clampNumber( + Number(budgets?.maxPublishedFindings ?? DEFAULT_BUDGETS.maxPublishedFindings ?? DEFAULT_BUDGETS.maxFindings), + 1, + 50, + ), + }; +} + +function extractJsonObject(raw: string): Record | null { + const candidates: string[] = []; + const trimmed = raw.trim(); + if (trimmed.length) candidates.push(trimmed); + const fencedMatch = raw.match(/```(?:json)?\s*([\s\S]+?)```/i); + if (fencedMatch?.[1]) candidates.push(fencedMatch[1].trim()); + + const firstBrace = raw.indexOf("{"); + if (firstBrace >= 0) { + let depth = 0; + let inString = false; + let escaped = false; + for (let index = firstBrace; index < raw.length; index += 1) { + const char = raw[index] ?? ""; + if (escaped) { + escaped = false; + continue; + } + if (char === "\\") { + escaped = true; + continue; + } + if (char === "\"") { + inString = !inString; + continue; + } + if (inString) continue; + if (char === "{") depth += 1; + if (char === "}") { + depth -= 1; + if (depth === 0) { + candidates.push(raw.slice(firstBrace, index + 1)); + break; + } + } + } + } + + for (const candidate of candidates) { + try { + const parsed = JSON.parse(candidate); + if (isRecord(parsed)) return parsed; + } catch { + // Try the next candidate. + } + } + return null; +} + +function resolveTargetLaneId(target: ReviewTarget): string { + return target.laneId; +} + +function mapLaunchLane(lane: Awaited["list"]>>[number]): ReviewLaunchLane { + return { + id: lane.id, + name: lane.name, + laneType: lane.laneType, + branchRef: lane.branchRef, + baseRef: lane.baseRef, + color: lane.color ?? null, + }; +} + +function mapLaunchCommit(commit: Awaited["listRecentCommits"]>>[number]): ReviewLaunchCommit { + return { + sha: commit.sha, + shortSha: commit.shortSha, + subject: commit.subject, + authoredAt: commit.authoredAt, + pushed: commit.pushed, + }; +} + +function serializeSeveritySummary(summary: ReviewSeveritySummary): string { + return JSON.stringify(summary); +} + +const REVIEW_PASSES: PassDefinition[] = [ + { + key: "diff-risk", + label: "Diff risk", + focus: "changed-file correctness, regressions, edge cases, migrations, and unsafe behavior directly visible in the diff", + extraInstructions: [ + "Stay anchored to the changed code and changed lines first.", + "Prioritize regressions, broken invariants, unsafe defaults, and risky data handling.", + ], + }, + { + key: "cross-file-impact", + label: "Cross-file impact", + focus: "impacted call sites, shared contracts, dependent code paths, and regressions likely to surface outside the touched files", + extraInstructions: [ + "Follow interfaces, configuration, and likely callers beyond the edited files.", + "Reject speculative concerns unless the diff gives a concrete reason to believe a wider breakage is likely.", + ], + }, + { + key: "checks-and-tests", + label: "Checks and tests", + focus: "failing checks, validation gaps, and the strongest missing-test risks that would allow a regression to slip through", + extraInstructions: [ + "Prefer concrete missing-test risks over generic 'add more tests' advice.", + "Use check/test context when present, but do not invent failures that were not supplied.", + ], + }, +]; + +function buildChangedFilesSummary(changedFiles: Array<{ filePath: string }>): string { + const changedFilesSummary = changedFiles.length > 0 + ? changedFiles.map((entry) => `- ${entry.filePath}`).join("\n") + : "- No changed files were detected."; + return changedFilesSummary; +} + +function buildContextArtifactHints(args: { + artifactIds: ReviewContextArtifactIds; + includeValidation: boolean; +}): string[] { + const lines = [ + `- provenance_brief artifact id: ${args.artifactIds.provenanceArtifactId}`, + `- rule_overlays artifact id: ${args.artifactIds.rulesArtifactId}`, + ]; + if (args.includeValidation) { + lines.push(`- validation_signals artifact id: ${args.artifactIds.validationArtifactId}`); + } + return lines; +} + +function buildPassPrompt(args: { + run: ReviewRun; + pass: PassDefinition; + diffText: string; + changedFiles: Array<{ filePath: string }>; + context: ReviewContextPacket; + contextArtifactIds: ReviewContextArtifactIds; +}): string { + const changedFilesSummary = buildChangedFilesSummary(args.changedFiles); + const includeValidation = args.pass.key === "checks-and-tests"; + const ruleGuidance = collectRulePromptGuidance(args.context.matchedRuleOverlays, args.pass.key); + + return [ + "You are ADE's local code reviewer.", + "Review only the provided local diff bundle.", + `This pass is ${args.pass.label.toLowerCase()} and it focuses on ${args.pass.focus}.`, + "Prioritize correctness, regressions, security, data loss, race conditions, risky migrations, and missing tests.", + "Do not suggest style-only nits or speculative rewrites.", + "Every finding must include concrete evidence from the diff bundle or supplied review context.", + `Return strict JSON only with this exact top-level shape: {"summary": string, "findings": Finding[]}.`, + "Each Finding must be an object with:", + '- "title": short issue title', + '- "severity": one of "critical", "high", "medium", "low", "info"', + '- "findingClass": optional, one of "intent_drift", "incomplete_rollout", "late_stage_regression", or null', + '- "body": concise explanation of the risk and why it matters', + '- "confidence": number between 0 and 1', + '- "filePath": changed file path when known, otherwise null', + '- "line": line number when known, otherwise null', + '- "evidence": array of objects with {"summary": string, "quote": string|null, "filePath": string|null, "line": number|null, "artifactId": string|null}', + `Return at most ${args.run.config.budgets.maxFindingsPerPass ?? args.run.config.budgets.maxFindings} findings.`, + "If there are no real issues, return an empty findings array and explain that in summary.", + "", + `Pass key: ${args.pass.key}`, + `Review target: ${args.run.targetLabel}`, + `Selection mode: ${args.run.config.selectionMode}`, + `Publish behavior: ${args.run.config.publishBehavior}`, + "", + "Pass guidance:", + ...args.pass.extraInstructions.map((instruction) => `- ${instruction}`), + ...(ruleGuidance.length > 0 ? ["", "Matched rule guidance:", ...ruleGuidance.map((instruction) => `- ${instruction}`)] : []), + "", + "Changed files:", + changedFilesSummary, + "", + "Context artifact ids you may cite in evidence when relevant:", + ...buildContextArtifactHints({ + artifactIds: args.contextArtifactIds, + includeValidation, + }), + "", + "ADE provenance and intent context:", + args.context.provenance.prompt, + "", + "Repo/path rule overlays:", + args.context.rules.prompt, + "", + "Checks and validation context:", + includeValidation ? args.context.validation.prompt : "- Full validation evidence is reserved for the checks-and-tests pass.", + "", + "Diff bundle:", + truncateText(args.diffText, args.run.config.budgets.maxPromptChars), + ].join("\n"); +} + +function parseEvidence(value: unknown): ReviewEvidence[] { + if (!Array.isArray(value)) return []; + return value.flatMap((entry) => { + if (!isRecord(entry)) return []; + const summary = cleanLine(String(entry.summary ?? "")); + if (!summary) return []; + const rawKind = typeof entry.kind === "string" ? entry.kind.trim().toLowerCase() : ""; + const kind: ReviewEvidence["kind"] = + rawKind === "artifact" + ? "artifact" + : rawKind === "file_snapshot" + ? "file_snapshot" + : rawKind === "diff_hunk" + ? "diff_hunk" + : "quote"; + return [{ + kind, + summary, + filePath: typeof entry.filePath === "string" ? entry.filePath.trim() || null : null, + line: typeof entry.line === "number" && Number.isInteger(entry.line) && entry.line > 0 ? entry.line : null, + quote: typeof entry.quote === "string" ? entry.quote.trim() || null : null, + artifactId: typeof entry.artifactId === "string" ? entry.artifactId.trim() || null : null, + }]; + }); +} + +function computeAnchorState(args: { + filePath: string | null; + line: number | null; + changedFilesByPath: Map }>; +}): "anchored" | "file_only" | "missing" { + if (!args.filePath) return "missing"; + const match = args.changedFilesByPath.get(args.filePath); + if (!match) return "missing"; + if (args.line == null) return "file_only"; + return match.lineNumbers.has(args.line) ? "anchored" : "file_only"; +} + +function hasConcreteEvidence(evidence: ReviewEvidence[]): boolean { + return evidence.some((entry) => { + if (entry.kind === "artifact") return false; + return Boolean( + (typeof entry.quote === "string" && entry.quote.trim().length > 0) + || (entry.filePath && entry.line != null) + || (entry.filePath && entry.kind === "diff_hunk"), + ); + }); +} + +function scoreEvidence(evidence: ReviewEvidence[]): number { + if (evidence.length === 0) return 0; + const quoteCount = evidence.filter((entry) => typeof entry.quote === "string" && entry.quote.trim().length > 0).length; + const anchoredCount = evidence.filter((entry) => Boolean(entry.filePath) && entry.line != null).length; + const artifactCount = evidence.filter((entry) => entry.kind === "artifact" && entry.artifactId).length; + return clampNumber( + 0.18 + + Math.min(quoteCount, 3) * 0.18 + + Math.min(anchoredCount, 2) * 0.12 + + Math.min(artifactCount, 2) * 0.08, + 0, + 1, + ); +} + +function isLowSignalFinding(args: { + title: string; + body: string; + severity: ReviewSeverity; + confidence: number; + evidenceScore: number; +}): boolean { + const text = `${args.title} ${args.body}`.toLowerCase(); + const nitPatterns = [ + /\bnit\b/, + /\bnitpick\b/, + /\bstyle\b/, + /\bformat(?:ting)?\b/, + /\bwhitespace\b/, + /\brename\b/, + /\bnaming\b/, + /\bcomment\b/, + /\btypo\b/, + /\bdocs?\b/, + ]; + const looksNitpicky = nitPatterns.some((pattern) => pattern.test(text)); + return looksNitpicky && args.severity !== "critical" && args.severity !== "high" && args.confidence < 0.8 && args.evidenceScore < 0.7; +} + +function buildCandidateScore(args: { + severity: ReviewSeverity; + confidence: number; + evidenceScore: number; + passCount?: number; +}): number { + const corroborationBonus = Math.max(0, (args.passCount ?? 1) - 1) * 0.15; + return Number((SEVERITY_SCORE[args.severity] + args.confidence * 2 + args.evidenceScore * 1.5 + corroborationBonus).toFixed(4)); +} + +function tokenizeFindingText(value: string): string[] { + return cleanLine(value) + .toLowerCase() + .split(/[^a-z0-9]+/g) + .filter((token) => token.length >= 3) + .filter((token) => !new Set([ + "this", + "that", + "with", + "from", + "into", + "when", + "then", + "there", + "return", + "returns", + "added", + "change", + "changes", + "issue", + "risk", + "will", + "could", + "should", + "missing", + "tests", + "test", + "check", + "checks", + "code", + "file", + "files", + ]).has(token)); +} + +function similarityScore(left: string, right: string): number { + const leftTokens = tokenizeFindingText(left); + const rightTokens = tokenizeFindingText(right); + if (leftTokens.length === 0 || rightTokens.length === 0) return 0; + const leftSet = new Set(leftTokens); + const rightSet = new Set(rightTokens); + const intersection = Array.from(leftSet).filter((token) => rightSet.has(token)).length; + const union = new Set([...leftSet, ...rightSet]).size; + return union > 0 ? intersection / union : 0; +} + +function hasOverlappingEvidence(left: PassCandidateFinding, right: PassCandidateFinding): boolean { + for (const leftEntry of left.evidence) { + for (const rightEntry of right.evidence) { + if (leftEntry.filePath && leftEntry.filePath === rightEntry.filePath && leftEntry.line != null && leftEntry.line === rightEntry.line) { + return true; + } + if (leftEntry.quote && rightEntry.quote && cleanLine(leftEntry.quote) === cleanLine(rightEntry.quote)) { + return true; + } + } + } + return false; +} + +function findingsOverlap(left: PassCandidateFinding, right: PassCandidateFinding): boolean { + const sameFile = left.filePath && right.filePath && left.filePath === right.filePath; + const lineDistance = left.line != null && right.line != null ? Math.abs(left.line - right.line) : null; + const titleSimilarity = similarityScore(left.title, right.title); + const bodySimilarity = similarityScore(left.body, right.body); + const similarText = Math.max(titleSimilarity, bodySimilarity, similarityScore(`${left.title} ${left.body}`, `${right.title} ${right.body}`)); + if (sameFile && lineDistance != null && lineDistance <= 3 && similarText >= 0.22) return true; + if (sameFile && similarText >= 0.35) return true; + if (hasOverlappingEvidence(left, right) && similarText >= 0.18) return true; + return !left.filePath && !right.filePath && similarText >= 0.65; +} + +function dedupeEvidenceEntries(evidence: ReviewEvidence[]): ReviewEvidence[] { + const seen = new Set(); + const deduped: ReviewEvidence[] = []; + for (const entry of evidence) { + const key = JSON.stringify([ + entry.kind, + entry.summary, + entry.filePath, + entry.line, + entry.quote, + entry.artifactId, + ]); + if (seen.has(key)) continue; + seen.add(key); + deduped.push(entry); + } + return deduped; +} + +function normalizeParsedFindings(args: { + runId: string; + passKey: ReviewPassKey; + parsed: Record | null; + changedFilesByPath: Map }>; +}): { summary: string | null; findings: PassCandidateFinding[] } { + const findingsRaw = Array.isArray(args.parsed?.findings) ? args.parsed?.findings : []; + const findings = findingsRaw.flatMap((entry) => { + if (!isRecord(entry)) return []; + const title = cleanLine(String(entry.title ?? "")); + const body = cleanLine(String(entry.body ?? "")); + if (!title || !body) return []; + const filePath = typeof entry.filePath === "string" ? entry.filePath.trim() || null : null; + const line = typeof entry.line === "number" && Number.isInteger(entry.line) && entry.line > 0 ? entry.line : null; + const computedEvidence = parseEvidence(entry.evidence); + const fallbackFile = filePath ? args.changedFilesByPath.get(filePath) : null; + const evidence: ReviewEvidence[] = computedEvidence.length > 0 + ? computedEvidence + : fallbackFile + ? [{ + kind: "diff_hunk" as const, + summary: `Relevant diff context from ${filePath}`, + filePath, + line, + quote: fallbackFile.excerpt || null, + artifactId: null, + }] + : []; + const anchorState = computeAnchorState({ + filePath, + line, + changedFilesByPath: args.changedFilesByPath, + }); + const evidenceScore = scoreEvidence(evidence); + const confidence = normalizeConfidence(entry.confidence); + const finding: PassCandidateFinding = { + id: randomUUID(), + runId: args.runId, + passKey: args.passKey, + title, + severity: normalizeSeverity(entry.severity), + findingClass: normalizeFindingClass(entry.findingClass), + body, + confidence, + evidence, + filePath, + line, + anchorState, + evidenceScore, + lowSignal: isLowSignalFinding({ + title, + body, + severity: normalizeSeverity(entry.severity), + confidence, + evidenceScore, + }), + score: buildCandidateScore({ + severity: normalizeSeverity(entry.severity), + confidence, + evidenceScore, + }), + }; + return [finding]; + }); + + const summary = typeof args.parsed?.summary === "string" ? cleanLine(args.parsed.summary) : null; + return { summary, findings }; +} + +function summarizeAdjudication(args: { + keptFindings: ReviewFinding[]; + rejected: AdjudicationRejectedFinding[]; + totalCandidateCount: number; +}): string { + if (args.keptFindings.length === 0) { + if (args.totalCandidateCount === 0) { + return "Multi-pass review completed with no actionable findings."; + } + return "Multi-pass review completed, but every candidate was filtered out during adjudication."; + } + + const corroboratedCount = args.keptFindings.filter((finding) => (finding.originatingPasses?.length ?? 0) > 1).length; + const publicationEligibleCount = args.keptFindings.filter((finding) => finding.adjudication?.publicationEligible).length; + return [ + `Multi-pass review kept ${args.keptFindings.length} high-signal finding(s) from ${args.totalCandidateCount} candidate(s).`, + corroboratedCount > 0 ? `${corroboratedCount} finding(s) were corroborated by multiple passes.` : null, + publicationEligibleCount > 0 ? `${publicationEligibleCount} finding(s) cleared the publication threshold.` : null, + args.rejected.length > 0 ? `${args.rejected.length} candidate(s) were filtered during adjudication.` : null, + ].filter((line): line is string => Boolean(line)).join(" "); +} + +function selectPreferredAnchor(findings: PassCandidateFinding[]): Pick { + const ranked = [...findings].sort((left, right) => { + const anchorDelta = (left.anchorState === "anchored" ? 2 : left.anchorState === "file_only" ? 1 : 0) + - (right.anchorState === "anchored" ? 2 : right.anchorState === "file_only" ? 1 : 0); + if (anchorDelta !== 0) return -anchorDelta; + if (left.filePath && !right.filePath) return -1; + if (!left.filePath && right.filePath) return 1; + return (left.line ?? Number.MAX_SAFE_INTEGER) - (right.line ?? Number.MAX_SAFE_INTEGER); + }); + const preferred = ranked[0]; + return { + filePath: preferred?.filePath ?? null, + line: preferred?.line ?? null, + anchorState: preferred?.anchorState ?? "missing", + }; +} + +function combineConfidence(findings: PassCandidateFinding[]): number { + const combined = findings.reduce((product, finding) => product * (1 - clampNumber(finding.confidence, 0, 1)), 1); + return clampNumber(1 - combined, 0, 0.99); +} + +function groupPassCandidates(candidates: PassCandidateFinding[]): PassCandidateFinding[][] { + const remaining = [...candidates].sort((left, right) => right.score - left.score); + const groups: PassCandidateFinding[][] = []; + while (remaining.length > 0) { + const seed = remaining.shift(); + if (!seed) continue; + const group = [seed]; + let index = 0; + while (index < remaining.length) { + const candidate = remaining[index]; + if (candidate && group.some((entry) => findingsOverlap(entry, candidate))) { + group.push(candidate); + remaining.splice(index, 1); + continue; + } + index += 1; + } + groups.push(group); + } + return groups; +} + +function getCandidatePathSet(group: PassCandidateFinding[]): string[] { + return Array.from(new Set( + group.flatMap((candidate) => [ + candidate.filePath, + ...candidate.evidence.map((entry) => entry.filePath), + ]).filter((value): value is string => Boolean(value?.trim())), + )); +} + +function countConcreteAnchorFiles(evidence: ReviewEvidence[]): number { + return new Set( + evidence + .filter((entry) => entry.kind !== "artifact") + .filter((entry) => Boolean(entry.filePath) && (entry.line != null || (entry.quote?.trim() ?? "").length > 0 || entry.kind === "diff_hunk")) + .map((entry) => entry.filePath as string), + ).size; +} + +function hasArtifactEvidence(evidence: ReviewEvidence[], artifactIds: string[]): boolean { + const allowed = new Set(artifactIds); + return evidence.some((entry) => entry.kind === "artifact" && entry.artifactId && allowed.has(entry.artifactId)); +} + +function buildContextArtifactEvidence(args: { + group: PassCandidateFinding[]; + context: ReviewContextPacket; + artifactIds: ReviewContextArtifactIds; + relevantOverlays: MatchedReviewRuleOverlay[]; +}): ReviewEvidence[] { + const pathSet = new Set(getCandidatePathSet(args.group)); + const evidence: ReviewEvidence[] = []; + if (args.relevantOverlays.length > 0) { + evidence.push({ + kind: "artifact", + summary: `Matched rule overlays: ${args.relevantOverlays.map((overlay) => overlay.id).join(", ")}`, + filePath: null, + line: null, + quote: null, + artifactId: args.artifactIds.rulesArtifactId, + }); + } + const lateStageMatches = args.context.provenance.payload.lateStageSignals.filter((signal) => + signal.filePaths.some((filePath) => pathSet.has(filePath)), + ); + if (lateStageMatches.length > 0) { + evidence.push({ + kind: "artifact", + summary: `Late-stage ADE signals overlap this area: ${lateStageMatches.map((signal) => signal.summary).join(" | ")}`, + filePath: null, + line: null, + quote: null, + artifactId: args.artifactIds.provenanceArtifactId, + }); + } + const includesChecksPass = args.group.some((candidate) => candidate.passKey === "checks-and-tests"); + if (includesChecksPass && args.context.validation.payload.signals.length > 0) { + evidence.push({ + kind: "artifact", + summary: `Validation signals: ${args.context.validation.payload.signals.slice(0, 2).map((signal) => signal.summary).join(" | ")}`, + filePath: null, + line: null, + quote: null, + artifactId: args.artifactIds.validationArtifactId, + }); + } + return evidence; +} + +function inferFindingClass(args: { + group: PassCandidateFinding[]; + context: ReviewContextPacket; + relevantOverlays: MatchedReviewRuleOverlay[]; +}): ReviewFindingClass | null { + const explicitClass = mergeFindingClass(args.group.map((candidate) => candidate.findingClass)); + if (explicitClass) return explicitClass; + const pathSet = new Set(getCandidatePathSet(args.group)); + const hasLateStageSignal = args.context.provenance.payload.lateStageSignals.some((signal) => + signal.filePaths.some((filePath) => pathSet.has(filePath)), + ); + if (hasLateStageSignal) return "late_stage_regression"; + const hasStrictMissingRollout = args.relevantOverlays.some((overlay) => + overlay.adjudicationPolicy.evidenceMode === "cross_boundary" && overlay.missingFamilies.length > 0, + ); + if (hasStrictMissingRollout) return "incomplete_rollout"; + const hasIntentContext = Boolean( + args.context.provenance.payload.missions.length > 0 + || args.context.provenance.payload.laneSnapshot?.agentSummary + || args.context.provenance.payload.laneSnapshot?.missionSummary, + ); + const wordingSuggestsDrift = args.group.some((candidate) => + /\b(expected|intent|should|instead|missing|omits?|drift)\b/i.test(`${candidate.title} ${candidate.body}`), + ); + if (hasIntentContext && wordingSuggestsDrift) { + return "intent_drift"; + } + return null; +} + +function evaluateRuleEvidencePolicy(args: { + evidence: ReviewEvidence[]; + relevantOverlays: MatchedReviewRuleOverlay[]; + artifactIds: ReviewContextArtifactIds; +}): { ok: boolean; detail: string | null } { + const strictOverlays = args.relevantOverlays.filter((overlay) => overlay.adjudicationPolicy.evidenceMode === "cross_boundary"); + if (strictOverlays.length === 0) { + return { ok: true, detail: null }; + } + const concreteAnchorFiles = countConcreteAnchorFiles(args.evidence); + const hasSupportArtifact = hasArtifactEvidence(args.evidence, [ + args.artifactIds.provenanceArtifactId, + args.artifactIds.validationArtifactId, + ]); + if (concreteAnchorFiles >= 2 || (concreteAnchorFiles >= 1 && hasSupportArtifact)) { + return { ok: true, detail: null }; + } + return { + ok: false, + detail: `Rule overlays ${strictOverlays.map((overlay) => overlay.id).join(", ")} require either two concrete file anchors or one concrete anchor plus provenance/validation artifact support.`, + }; +} + +function adjudicatePassFindings(args: { + runId: string; + passResults: PassExecutionResult[]; + budgets: ReviewRunConfig["budgets"]; + context: ReviewContextPacket; + artifactIds: ReviewContextArtifactIds; +}): AdjudicationOutcome { + const allCandidates = args.passResults.flatMap((result) => result.candidates); + const groupedCandidates = groupPassCandidates(allCandidates); + const findings: ReviewFinding[] = []; + const rejected: AdjudicationRejectedFinding[] = []; + + for (const group of groupedCandidates) { + const passes = Array.from(new Set(group.map((candidate) => candidate.passKey))).sort( + (left, right) => REVIEW_PASS_ORDER.indexOf(left) - REVIEW_PASS_ORDER.indexOf(right), + ); + const bestCandidate = [...group].sort((left, right) => right.score - left.score)[0]; + if (!bestCandidate) continue; + const candidatePaths = getCandidatePathSet(group); + const relevantOverlays = args.context.matchedRuleOverlays.filter((overlay) => + candidatePaths.some((filePath) => overlayMatchesPath(overlay, filePath)), + ); + const mergedEvidence = dedupeEvidenceEntries([ + ...group.flatMap((candidate) => candidate.evidence), + ...args.passResults + .filter((result) => passes.includes(result.pass.key)) + .map((result) => ({ + kind: "artifact" as const, + summary: `Raw ${result.pass.key} pass output`, + filePath: null, + line: null, + quote: null, + artifactId: result.findingsArtifactId, + })), + ...buildContextArtifactEvidence({ + group, + context: args.context, + artifactIds: args.artifactIds, + relevantOverlays, + }), + ]).slice(0, 10); + const evidenceScore = Math.max(bestCandidate.evidenceScore, scoreEvidence(mergedEvidence)); + const lowSignal = group.every((candidate) => candidate.lowSignal); + const findingClass = inferFindingClass({ + group, + context: args.context, + relevantOverlays, + }); + const score = buildCandidateScore({ + severity: group + .map((candidate) => candidate.severity) + .sort((left, right) => SEVERITY_SCORE[right] - SEVERITY_SCORE[left])[0] ?? bestCandidate.severity, + confidence: combineConfidence(group), + evidenceScore, + passCount: passes.length, + }); + + if (!hasConcreteEvidence(mergedEvidence)) { + rejected.push({ + candidateIds: group.map((candidate) => candidate.id), + passKeys: passes, + title: bestCandidate.title, + reason: "low_evidence", + detail: "The finding did not retain enough concrete evidence after adjudication.", + score, + }); + continue; + } + + if (lowSignal && passes.length < 2) { + rejected.push({ + candidateIds: group.map((candidate) => candidate.id), + passKeys: passes, + title: bestCandidate.title, + reason: "low_signal", + detail: "The finding looked nitpicky and was not corroborated by another pass.", + score, + }); + continue; + } + + const rulePolicy = evaluateRuleEvidencePolicy({ + evidence: mergedEvidence, + relevantOverlays, + artifactIds: args.artifactIds, + }); + if (!rulePolicy.ok) { + rejected.push({ + candidateIds: group.map((candidate) => candidate.id), + passKeys: passes, + title: bestCandidate.title, + reason: "rule_policy", + detail: rulePolicy.detail ?? "The finding did not satisfy the matched repo/path rule evidence policy.", + score, + }); + continue; + } + + const preferredAnchor = selectPreferredAnchor(group); + const confidence = combineConfidence(group); + const severity = group + .map((candidate) => candidate.severity) + .sort((left, right) => SEVERITY_SCORE[right] - SEVERITY_SCORE[left])[0] ?? bestCandidate.severity; + const publicationEligible = evidenceScore >= 0.55 && confidence >= 0.45 && severity !== "info"; + const adjudication: ReviewFindingAdjudication = { + score, + candidateCount: group.length, + mergedFindingIds: group.map((candidate) => candidate.id), + rationale: [ + passes.length > 1 + ? `Merged overlapping findings from ${passes.join(", ")} with shared evidence.` + : "Accepted because the finding carried concrete evidence and cleared the adjudication threshold.", + relevantOverlays.length > 0 + ? `Matched rule overlays: ${relevantOverlays.map((overlay) => overlay.id).join(", ")}.` + : null, + findingClass ? `Primary ADE-native class: ${findingClass}.` : null, + ].filter((value): value is string => Boolean(value)).join(" "), + publicationEligible, + }; + + findings.push({ + id: randomUUID(), + runId: args.runId, + title: bestCandidate.title, + severity, + findingClass, + body: passes.length > 1 + ? `${bestCandidate.body} This risk was corroborated by ${passes.join(", ")}.` + : bestCandidate.body, + confidence, + evidence: mergedEvidence, + filePath: preferredAnchor.filePath, + line: preferredAnchor.line, + anchorState: preferredAnchor.anchorState, + sourcePass: "adjudicated" as ReviewSourcePass, + publicationState: "local_only" as ReviewPublicationState, + originatingPasses: passes, + adjudication, + }); + } + + const keptFindings = findings + .sort((left, right) => (right.adjudication?.score ?? 0) - (left.adjudication?.score ?? 0)) + .slice(0, args.budgets.maxFindings); + const keptIds = new Set(keptFindings.map((finding) => finding.id)); + for (const finding of findings) { + if (keptIds.has(finding.id)) continue; + rejected.push({ + candidateIds: finding.adjudication?.mergedFindingIds ?? [], + passKeys: finding.originatingPasses ?? [], + title: finding.title, + reason: "budget", + detail: "The finding cleared adjudication but was trimmed by the run-level budget.", + score: finding.adjudication?.score ?? 0, + }); + } + + const publicationEligibleCount = keptFindings.filter((finding) => finding.adjudication?.publicationEligible).length; + return { + summary: summarizeAdjudication({ + keptFindings, + rejected, + totalCandidateCount: allCandidates.length, + }), + findings: keptFindings, + rejected, + publicationEligibleCount, + totalCandidateCount: allCandidates.length, + }; +} + +function deriveRepoKey(target: ReviewPublicationDestination | null, fallbackProjectId: string): string { + if (target && target.kind === "github_pr_review") { + if (target.repoOwner && target.repoName) { + return `${target.repoOwner}/${target.repoName}`.toLowerCase(); + } + } + return `project:${fallbackProjectId}`; +} + +function tallySeveritySummary(findings: ReviewFinding[]): ReviewSeveritySummary { + const summary = defaultSeveritySummary(); + for (const finding of findings) { + summary[finding.severity] += 1; + } + return summary; +} + +function mapRunRow(row: ReviewRunRow): ReviewRun { + const config = safeJsonParse(row.config_json, { + compareAgainst: { kind: "default_branch" }, + selectionMode: "full_diff", + dirtyOnly: false, + modelId: DEFAULT_REVIEW_MODEL_ID, + reasoningEffort: null, + budgets: DEFAULT_BUDGETS, + publishBehavior: "local_only", + }); + return { + id: row.id, + projectId: row.project_id, + laneId: row.lane_id, + target: safeJsonParse(row.target_json, { + mode: "working_tree", + laneId: row.lane_id, + }), + config: { + ...config, + budgets: normalizeBudgetConfig(config.budgets), + }, + targetLabel: row.target_label, + compareTarget: safeJsonParse(row.compare_target_json, null), + status: (row.status as ReviewRunStatus) ?? "failed", + summary: row.summary, + errorMessage: row.error_message, + findingCount: Number(row.finding_count ?? 0), + severitySummary: safeJsonParse(row.severity_summary_json, defaultSeveritySummary()), + chatSessionId: row.chat_session_id, + createdAt: row.created_at, + startedAt: row.started_at, + endedAt: row.ended_at, + updatedAt: row.updated_at, + }; +} + +function mapFindingRow(row: ReviewFindingRow): ReviewFinding { + return { + id: row.id, + runId: row.run_id, + title: row.title, + severity: normalizeSeverity(row.severity), + findingClass: normalizeFindingClass(row.finding_class), + body: row.body, + confidence: clampNumber(Number(row.confidence ?? 0.5), 0, 1), + evidence: safeJsonParse(row.evidence_json, []), + filePath: row.file_path, + line: typeof row.line === "number" ? row.line : null, + anchorState: (row.anchor_state as ReviewFinding["anchorState"]) ?? "missing", + sourcePass: (row.source_pass as ReviewSourcePass) ?? "single_pass", + publicationState: (row.publication_state as ReviewPublicationState) ?? "local_only", + originatingPasses: safeJsonParse(row.originating_passes_json, []), + adjudication: safeJsonParse(row.adjudication_json, null), + diffContext: safeJsonParse(row.diff_context_json, null), + suppressionMatch: safeJsonParse(row.suppression_match_json, null), + }; +} + +function mapFeedbackRow(row: ReviewFindingFeedbackRow): ReviewFeedbackRecord { + return { + id: row.id, + findingId: row.finding_id, + runId: row.run_id, + kind: (row.kind as ReviewFeedbackKind) ?? "acknowledge", + reason: (row.reason as ReviewDismissReason | null) ?? null, + note: row.note, + snoozeUntil: row.snooze_until, + createdAt: row.created_at, + }; +} + +function mapArtifactRow(row: ReviewRunArtifactRow): ReviewRunArtifact { + return { + id: row.id, + runId: row.run_id, + artifactType: (row.artifact_type as ReviewArtifactType) ?? "diff_bundle", + title: row.title, + mimeType: row.mime_type, + contentText: row.content_text, + metadata: safeJsonParse | null>(row.metadata_json, null), + createdAt: row.created_at, + }; +} + +function mapPublicationRow(row: ReviewRunPublicationRow): ReviewPublication { + return { + id: row.id, + runId: row.run_id, + destination: safeJsonParse(row.destination_json, { + kind: "github_pr_review", + prId: "", + repoOwner: "", + repoName: "", + prNumber: 0, + githubUrl: null, + }), + reviewEvent: row.review_event as ReviewPublication["reviewEvent"], + status: row.status === "published" ? "published" : "failed", + reviewUrl: row.review_url, + remoteReviewId: row.remote_review_id, + summaryBody: row.summary_body, + inlineComments: safeJsonParse(row.inline_comments_json, []), + summaryFindingIds: safeJsonParse(row.summary_finding_ids_json, []), + errorMessage: row.error_message, + createdAt: row.created_at, + updatedAt: row.updated_at, + completedAt: row.completed_at, + }; +} + +export function createReviewService({ + db, + logger, + projectId, + projectRoot, + projectDefaultBranch, + laneService, + gitService, + agentChatService, + sessionService, + sessionDeltaService, + testService, + issueInventoryService, + prService, + embeddingService, + onEvent, +}: { + db: AdeDb; + logger: Logger; + projectId: string; + projectRoot: string; + projectDefaultBranch: string | null; + laneService: Pick, "getLaneBaseAndBranch" | "getStateSnapshot" | "list">; + gitService: Pick, "listRecentCommits">; + agentChatService: Pick, "createSession" | "getSessionSummary" | "runSessionTurn">; + sessionService: Pick, "updateMeta">; + sessionDeltaService: Pick, "listRecentLaneSessionDeltas">; + testService: Pick, "listRuns" | "getLogTail" | "listSuites">; + issueInventoryService: Pick, "getInventory">; + prService?: Pick, "getReviewSnapshot" | "getChecks" | "publishReviewPublication">; + embeddingService?: Pick | null; + onEvent?: (event: ReviewEventPayload) => void; +}) { + const materializer = createReviewTargetMaterializer({ laneService, prService }); + const contextBuilder = createReviewContextBuilder({ + db, + projectId, + logger, + laneService, + sessionDeltaService, + testService, + issueInventoryService, + prService, + }); + const suppressionService: ReviewSuppressionService = createReviewSuppressionService({ + db, + logger, + projectId, + embeddingService: embeddingService ?? null, + }); + const activeRuns = new Set(); + const cancelledRuns = new Set(); + let disposed = false; + const configuredDefaultModelId = + getDefaultModelDescriptor("codex")?.id + ?? getDefaultModelDescriptor("opencode")?.id + ?? REVIEW_MODEL_FALLBACK_ID; + const defaultReviewModelId = getModelById(configuredDefaultModelId)?.id ?? DEFAULT_REVIEW_MODEL_ID; + + if (defaultReviewModelId !== configuredDefaultModelId) { + logger.warn("review.default_model_fallback_selected", { + requestedModelId: configuredDefaultModelId, + resolvedModelId: defaultReviewModelId, + }); + } + + function assertNotDisposed(): void { + if (disposed) { + throw new Error("Review service is disposed."); + } + } + + function emit(event: ReviewEventPayload): void { + if (disposed) return; + onEvent?.(event); + } + + function getRunRow(runId: string): ReviewRunRow | null { + return db.get( + "select * from review_runs where id = ? and project_id = ? limit 1", + [runId, projectId], + ); + } + + function updateRun(runId: string, patch: Partial<{ + target_label: string; + compare_target_json: string | null; + status: ReviewRunStatus; + summary: string | null; + error_message: string | null; + finding_count: number; + severity_summary_json: string; + chat_session_id: string | null; + ended_at: string | null; + updated_at: string; + }>): void { + const sets: string[] = []; + const params: Array = []; + for (const [key, value] of Object.entries(patch)) { + sets.push(`${key} = ?`); + params.push(value ?? null); + } + if (sets.length === 0) return; + params.push(runId, projectId); + db.run(`update review_runs set ${sets.join(", ")} where id = ? and project_id = ?`, params); + } + + function insertRun(run: ReviewRun): void { + db.run( + `insert into review_runs ( + id, + project_id, + lane_id, + target_json, + config_json, + target_label, + compare_target_json, + status, + summary, + error_message, + finding_count, + severity_summary_json, + chat_session_id, + created_at, + started_at, + ended_at, + updated_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + [ + run.id, + run.projectId, + run.laneId, + JSON.stringify(run.target), + JSON.stringify(run.config), + run.targetLabel, + run.compareTarget ? JSON.stringify(run.compareTarget) : null, + run.status, + run.summary, + run.errorMessage, + run.findingCount, + serializeSeveritySummary(run.severitySummary), + run.chatSessionId, + run.createdAt, + run.startedAt, + run.endedAt, + run.updatedAt, + ], + ); + } + + function insertArtifact(runId: string, artifact: Omit): ReviewRunArtifact { + const record: ReviewRunArtifact = { + id: randomUUID(), + runId, + artifactType: artifact.artifactType, + title: artifact.title, + mimeType: artifact.mimeType, + contentText: artifact.contentText, + metadata: artifact.metadata ?? null, + createdAt: nowIso(), + }; + db.run( + `insert into review_run_artifacts ( + id, + run_id, + artifact_type, + title, + mime_type, + content_text, + metadata_json, + created_at + ) values (?, ?, ?, ?, ?, ?, ?, ?)`, + [ + record.id, + record.runId, + record.artifactType, + record.title, + record.mimeType, + record.contentText, + record.metadata ? JSON.stringify(record.metadata) : null, + record.createdAt, + ], + ); + return record; + } + + function insertPublication(publication: ReviewPublication): void { + db.run( + `insert into review_run_publications ( + id, + run_id, + destination_json, + review_event, + status, + review_url, + remote_review_id, + summary_body, + inline_comments_json, + summary_finding_ids_json, + error_message, + created_at, + updated_at, + completed_at + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + [ + publication.id, + publication.runId, + JSON.stringify(publication.destination), + publication.reviewEvent, + publication.status, + publication.reviewUrl, + publication.remoteReviewId, + publication.summaryBody, + JSON.stringify(publication.inlineComments), + JSON.stringify(publication.summaryFindingIds), + publication.errorMessage, + publication.createdAt, + publication.updatedAt, + publication.completedAt, + ], + ); + } + + function insertFinding(finding: ReviewFinding): void { + db.run( + `insert into review_findings ( + id, + run_id, + title, + severity, + finding_class, + body, + confidence, + evidence_json, + file_path, + line, + anchor_state, + source_pass, + publication_state, + originating_passes_json, + adjudication_json, + diff_context_json, + suppression_match_json + ) values (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, + [ + finding.id, + finding.runId, + finding.title, + finding.severity, + finding.findingClass ?? null, + finding.body, + finding.confidence, + JSON.stringify(finding.evidence), + finding.filePath, + finding.line, + finding.anchorState, + finding.sourcePass, + finding.publicationState, + JSON.stringify(finding.originatingPasses ?? []), + finding.adjudication ? JSON.stringify(finding.adjudication) : null, + finding.diffContext ? JSON.stringify(finding.diffContext) : null, + finding.suppressionMatch ? JSON.stringify(finding.suppressionMatch) : null, + ], + ); + } + + function updateFindingPublicationState(runId: string, findingId: string, publicationState: ReviewPublicationState): void { + db.run( + "update review_findings set publication_state = ? where id = ? and run_id = ?", + [publicationState, findingId, runId], + ); + } + + async function listLaunchContext(): Promise { + assertNotDisposed(); + const lanes = await laneService.list(); + const laneSummaries = lanes.map(mapLaunchLane); + const recentCommitsByLane = Object.fromEntries(await Promise.all( + laneSummaries.map(async (lane) => { + const commits = await gitService.listRecentCommits({ laneId: lane.id, limit: 20 }); + return [lane.id, commits.map(mapLaunchCommit)] as const; + }), + )); + return { + defaultLaneId: laneSummaries[0]?.id ?? null, + defaultBranchName: projectDefaultBranch ?? laneSummaries.find((lane) => lane.laneType === "primary")?.branchRef ?? null, + lanes: laneSummaries, + recentCommitsByLane, + recommendedModelId: defaultReviewModelId, + }; + } + + function resolveConfig(target: ReviewTarget, partial?: Partial | null): ReviewRunConfig { + return { + compareAgainst: partial?.compareAgainst ?? { kind: "default_branch" }, + selectionMode: partial?.selectionMode + ?? (target.mode === "commit_range" + ? "selected_commits" + : target.mode === "working_tree" + ? "dirty_only" + : "full_diff"), + dirtyOnly: partial?.dirtyOnly ?? target.mode === "working_tree", + modelId: partial?.modelId?.trim() || defaultReviewModelId, + reasoningEffort: partial?.reasoningEffort?.trim() || null, + budgets: normalizeBudgetConfig(partial?.budgets), + publishBehavior: target.mode === "pr" && partial?.publishBehavior === "auto_publish" + ? "auto_publish" + : "local_only", + }; + } + + async function publishRun(args: { + runId: string; + targetLabel: string; + summary: string | null; + config: ReviewRunConfig; + findings: ReviewFinding[]; + publicationTarget: ReviewPublicationDestination | null; + changedFiles: Array<{ filePath: string; diffPositionsByLine: Record }>; + }): Promise { + if (args.config.publishBehavior !== "auto_publish" || !args.publicationTarget || !prService) { + return null; + } + + const publishableFindings = [...args.findings] + .filter((finding) => finding.sourcePass === "adjudicated" && finding.adjudication?.publicationEligible) + .sort((left, right) => (right.adjudication?.score ?? 0) - (left.adjudication?.score ?? 0)) + .slice(0, args.config.budgets.maxPublishedFindings ?? args.config.budgets.maxFindings); + + insertArtifact(args.runId, { + artifactType: "publication_request", + title: "Review publication request", + mimeType: "application/json", + contentText: JSON.stringify({ + destination: args.publicationTarget, + targetLabel: args.targetLabel, + summary: args.summary, + findingIds: publishableFindings.map((finding) => finding.id), + changedFiles: args.changedFiles, + }, null, 2), + metadata: { + publishBehavior: args.config.publishBehavior, + findingCount: publishableFindings.length, + skipped: publishableFindings.length === 0, + }, + }); + + if (publishableFindings.length === 0) { + return null; + } + + const publication = await prService.publishReviewPublication({ + runId: args.runId, + destination: args.publicationTarget, + targetLabel: args.targetLabel, + summary: args.summary, + findings: publishableFindings, + changedFiles: args.changedFiles, + }); + insertPublication(publication); + insertArtifact(args.runId, { + artifactType: "publication_result", + title: "Review publication result", + mimeType: "application/json", + contentText: JSON.stringify(publication, null, 2), + metadata: { + status: publication.status, + destinationKind: publication.destination.kind, + }, + }); + + if (publication.status === "published") { + const publishedFindingIds = new Set([ + ...publication.inlineComments.map((comment) => comment.findingId), + ...publication.summaryFindingIds, + ]); + for (const finding of publishableFindings) { + if (!publishedFindingIds.has(finding.id)) continue; + updateFindingPublicationState(args.runId, finding.id, "published"); + } + } + + return publication; + } + + async function executePass(args: { + runId: string; + run: ReviewRun; + sessionId: string; + sessionTitle: string; + descriptorId: string; + pass: PassDefinition; + diffText: string; + changedFiles: MaterializedChangedFile[]; + changedFilesByPath: Map }>; + context: ReviewContextPacket; + contextArtifactIds: ReviewContextArtifactIds; + }): Promise { + const prompt = buildPassPrompt({ + run: args.run, + pass: args.pass, + diffText: args.diffText, + changedFiles: args.changedFiles, + context: args.context, + contextArtifactIds: args.contextArtifactIds, + }); + const promptArtifact = insertArtifact(args.runId, { + artifactType: "pass_prompt", + title: `${args.pass.label} prompt`, + mimeType: "text/plain", + contentText: prompt, + metadata: { + passKey: args.pass.key, + modelId: args.descriptorId, + reasoningEffort: args.run.config.reasoningEffort, + matchedRuleIds: args.context.rules.metadata.matchedRuleIds ?? [], + }, + }); + const result = await agentChatService.runSessionTurn({ + sessionId: args.sessionId, + text: prompt, + displayText: `${args.sessionTitle} · ${args.pass.label}`, + reasoningEffort: args.run.config.reasoningEffort, + timeoutMs: 15 * 60 * 1000, + }); + const outputArtifact = insertArtifact(args.runId, { + artifactType: "pass_output", + title: `${args.pass.label} output`, + mimeType: "application/json", + contentText: result.outputText, + metadata: { + passKey: args.pass.key, + provider: result.provider, + model: result.model, + modelId: result.modelId ?? args.descriptorId, + }, + }); + const parsed = extractJsonObject(result.outputText); + const normalized = normalizeParsedFindings({ + runId: args.runId, + passKey: args.pass.key, + parsed, + changedFilesByPath: args.changedFilesByPath, + }); + const candidates = [...normalized.findings] + .sort((left, right) => right.score - left.score) + .slice(0, args.run.config.budgets.maxFindingsPerPass ?? args.run.config.budgets.maxFindings); + const findingsArtifact = insertArtifact(args.runId, { + artifactType: "pass_findings", + title: `${args.pass.label} findings`, + mimeType: "application/json", + contentText: JSON.stringify({ + passKey: args.pass.key, + summary: normalized.summary, + totalParsedCount: normalized.findings.length, + keptCount: candidates.length, + budgetTrimmedCount: Math.max(0, normalized.findings.length - candidates.length), + candidates, + }, null, 2), + metadata: { + passKey: args.pass.key, + summary: normalized.summary, + totalParsedCount: normalized.findings.length, + keptCount: candidates.length, + budgetTrimmedCount: Math.max(0, normalized.findings.length - candidates.length), + }, + }); + return { + pass: args.pass, + summary: normalized.summary, + candidates, + promptArtifactId: promptArtifact.id, + outputArtifactId: outputArtifact.id, + findingsArtifactId: findingsArtifact.id, + budgetTrimmedCount: Math.max(0, normalized.findings.length - candidates.length), + }; + } + + async function executeRun(runId: string): Promise { + if (disposed || activeRuns.has(runId)) return; + activeRuns.add(runId); + try { + if (disposed) return; + const row = getRunRow(runId); + if (!row) return; + const run = mapRunRow(row); + if (disposed) return; + if (cancelledRuns.has(runId)) { + cancelledRuns.delete(runId); + const endedAt = nowIso(); + updateRun(runId, { + status: "cancelled", + summary: "Run cancelled before execution began.", + error_message: null, + ended_at: endedAt, + updated_at: endedAt, + }); + emit({ type: "run-completed", runId, laneId: run.laneId, status: "cancelled" }); + emit({ type: "runs-updated", runId, laneId: run.laneId, status: "cancelled" }); + return; + } + updateRun(runId, { + status: "running", + updated_at: nowIso(), + }); + emit({ type: "runs-updated", runId, laneId: run.laneId, status: "running" }); + + const materialized = await materializer.materialize({ + target: run.target, + config: run.config, + }); + if (disposed) return; + + updateRun(runId, { + target_label: materialized.targetLabel, + compare_target_json: materialized.compareTarget ? JSON.stringify(materialized.compareTarget) : null, + updated_at: nowIso(), + }); + + for (const artifact of materialized.artifacts) { + if (disposed) return; + insertArtifact(runId, artifact); + } + + if (disposed) return; + if (!materialized.fullPatchText.trim()) { + const endedAt = nowIso(); + updateRun(runId, { + status: "completed", + summary: "No changes to review.", + error_message: null, + finding_count: 0, + severity_summary_json: serializeSeveritySummary(defaultSeveritySummary()), + ended_at: endedAt, + updated_at: endedAt, + }); + emit({ type: "run-completed", runId, laneId: run.laneId, status: "completed" }); + emit({ type: "runs-updated", runId, laneId: run.laneId, status: "completed" }); + return; + } + + const descriptor = getModelById(run.config.modelId); + if (!descriptor) { + throw new Error(`Unknown review model '${run.config.modelId}'.`); + } + const { provider, model } = resolveChatProviderForDescriptor(descriptor); + const session = await agentChatService.createSession({ + laneId: run.laneId, + provider, + model, + modelId: descriptor.id, + reasoningEffort: run.config.reasoningEffort, + permissionMode: "plan", + sessionProfile: "workflow", + surface: "automation", + }); + if (disposed) return; + const sessionTitle = `Review: ${materialized.targetLabel}`; + sessionService.updateMeta({ + sessionId: session.id, + title: sessionTitle, + }); + updateRun(runId, { + chat_session_id: session.id, + updated_at: nowIso(), + }); + const effectiveRun: ReviewRun = { + ...run, + targetLabel: materialized.targetLabel, + compareTarget: materialized.compareTarget, + }; + const diffText = truncateText(materialized.fullPatchText, effectiveRun.config.budgets.maxDiffChars); + const changedFiles = materialized.changedFiles.slice(0, effectiveRun.config.budgets.maxFiles); + const reviewContext = await contextBuilder.buildContext({ + run: effectiveRun, + materialized: { + ...materialized, + changedFiles, + }, + }); + const provenanceArtifact = insertArtifact(runId, { + artifactType: "provenance_brief", + title: "Provenance brief", + mimeType: "application/json", + contentText: JSON.stringify(reviewContext.provenance.payload, null, 2), + metadata: reviewContext.provenance.metadata, + }); + const rulesArtifact = insertArtifact(runId, { + artifactType: "rule_overlays", + title: "Rule overlays", + mimeType: "application/json", + contentText: JSON.stringify(reviewContext.rules.payload, null, 2), + metadata: reviewContext.rules.metadata, + }); + const validationArtifact = insertArtifact(runId, { + artifactType: "validation_signals", + title: "Validation signals", + mimeType: "application/json", + contentText: JSON.stringify(reviewContext.validation.payload, null, 2), + metadata: reviewContext.validation.metadata, + }); + const contextArtifactIds: ReviewContextArtifactIds = { + provenanceArtifactId: provenanceArtifact.id, + rulesArtifactId: rulesArtifact.id, + validationArtifactId: validationArtifact.id, + }; + insertArtifact(runId, { + artifactType: "prompt", + title: "Review harness plan", + mimeType: "application/json", + contentText: JSON.stringify({ + targetLabel: materialized.targetLabel, + passKeys: REVIEW_PASSES.map((pass) => pass.key), + budgets: effectiveRun.config.budgets, + changedFiles: changedFiles.map((entry) => entry.filePath), + context: { + provenanceSummary: reviewContext.provenance.summary, + rulesSummary: reviewContext.rules.summary, + validationSummary: reviewContext.validation.summary, + matchedRuleIds: reviewContext.rules.metadata.matchedRuleIds ?? [], + contextArtifactIds, + }, + }, null, 2), + metadata: { + modelId: descriptor.id, + reasoningEffort: effectiveRun.config.reasoningEffort, + passCount: REVIEW_PASSES.length, + matchedRuleCount: reviewContext.rules.metadata.matchedRuleCount ?? 0, + matchedRuleIds: reviewContext.rules.metadata.matchedRuleIds ?? [], + provenanceCount: reviewContext.provenance.metadata.provenanceCount ?? 0, + validationSignalCount: reviewContext.validation.metadata.signalCount ?? 0, + }, + }); + + const changedFilesByPath = new Map(changedFiles.map((entry) => [ + entry.filePath, + { + excerpt: entry.excerpt, + lineNumbers: new Set(entry.lineNumbers), + diffPositionsByLine: entry.diffPositionsByLine, + }, + ])); + const passResults: PassExecutionResult[] = []; + for (const pass of REVIEW_PASSES) { + if (disposed) return; + if (cancelledRuns.has(runId)) { + cancelledRuns.delete(runId); + const endedAt = nowIso(); + updateRun(runId, { + status: "cancelled", + summary: "Run cancelled during review passes.", + error_message: null, + ended_at: endedAt, + updated_at: endedAt, + }); + emit({ type: "run-completed", runId, laneId: run.laneId, status: "cancelled" }); + emit({ type: "runs-updated", runId, laneId: run.laneId, status: "cancelled" }); + return; + } + const passResult = await executePass({ + runId, + run: effectiveRun, + sessionId: session.id, + sessionTitle, + descriptorId: descriptor.id, + pass, + diffText, + changedFiles, + changedFilesByPath, + context: reviewContext, + contextArtifactIds, + }); + passResults.push(passResult); + } + + if (disposed) return; + const adjudication = adjudicatePassFindings({ + runId, + passResults, + budgets: effectiveRun.config.budgets, + context: reviewContext, + artifactIds: contextArtifactIds, + }); + insertArtifact(runId, { + artifactType: "adjudication_result", + title: "Review adjudication", + mimeType: "application/json", + contentText: JSON.stringify({ + summary: adjudication.summary, + totalCandidateCount: adjudication.totalCandidateCount, + publicationEligibleCount: adjudication.publicationEligibleCount, + rejected: adjudication.rejected, + passSummaries: passResults.map((result) => ({ + passKey: result.pass.key, + summary: result.summary, + keptCount: result.candidates.length, + budgetTrimmedCount: result.budgetTrimmedCount, + findingsArtifactId: result.findingsArtifactId, + })), + }, null, 2), + metadata: { + acceptedCount: adjudication.findings.length, + rejectedCount: adjudication.rejected.length, + publicationEligibleCount: adjudication.publicationEligibleCount, + }, + }); + insertArtifact(runId, { + artifactType: "merged_findings", + title: "Merged review findings", + mimeType: "application/json", + contentText: JSON.stringify({ + summary: adjudication.summary, + findings: adjudication.findings, + }, null, 2), + metadata: { + findingCount: adjudication.findings.length, + publicationEligibleCount: adjudication.publicationEligibleCount, + }, + }); + insertArtifact(runId, { + artifactType: "review_output", + title: "Adjudicated review output", + mimeType: "application/json", + contentText: JSON.stringify({ + summary: adjudication.summary, + findings: adjudication.findings, + }, null, 2), + metadata: { + stage: "adjudicated", + findingCount: adjudication.findings.length, + }, + }); + + const repoKey = deriveRepoKey(materialized.publicationTarget, projectId); + const enrichedFindings: ReviewFinding[] = []; + for (const finding of adjudication.findings) { + if (disposed) return; + const diffContext = buildDiffContextForFinding({ + filePath: finding.filePath, + anchoredLine: finding.line, + patches: materialized.changedFiles.map((entry) => ({ filePath: entry.filePath, excerpt: entry.excerpt })), + }); + const toolEvidence = buildToolBackedEvidence({ + finding, + validation: reviewContext.validation.payload, + artifactIdByKey: { validation_signals: contextArtifactIds.validationArtifactId }, + }); + const suppressionMatch = await suppressionService.match({ finding, repoKey }).catch((error) => { + logger.warn("review.suppression.match_failed", { + findingId: finding.id, + error: error instanceof Error ? error.message : String(error), + }); + return null; + }); + enrichedFindings.push({ + ...finding, + evidence: toolEvidence.length > 0 ? [...finding.evidence, ...toolEvidence] : finding.evidence, + diffContext, + suppressionMatch, + }); + } + const findings = enrichedFindings; + for (const finding of findings) { + if (disposed) return; + insertFinding(finding); + } + if (disposed) return; + if (cancelledRuns.has(runId)) { + cancelledRuns.delete(runId); + const endedAt = nowIso(); + updateRun(runId, { + status: "cancelled", + summary: "Run cancelled before publication.", + error_message: null, + finding_count: findings.length, + severity_summary_json: serializeSeveritySummary(tallySeveritySummary(findings)), + ended_at: endedAt, + updated_at: endedAt, + }); + emit({ type: "run-completed", runId, laneId: run.laneId, status: "cancelled" }); + emit({ type: "runs-updated", runId, laneId: run.laneId, status: "cancelled" }); + return; + } + const publishableFindings = findings.filter((finding) => finding.suppressionMatch == null); + const suppressedCount = findings.length - publishableFindings.length; + if (suppressedCount > 0) { + insertArtifact(runId, { + artifactType: "tool_evidence", + title: "Suppression filter summary", + mimeType: "application/json", + contentText: JSON.stringify({ + suppressedCount, + suppressedFindingIds: findings + .filter((finding) => finding.suppressionMatch != null) + .map((finding) => finding.id), + }, null, 2), + metadata: { suppressedCount }, + }); + } + await publishRun({ + runId, + targetLabel: materialized.targetLabel, + summary: adjudication.summary, + config: effectiveRun.config, + findings: publishableFindings, + publicationTarget: materialized.publicationTarget, + changedFiles: materialized.changedFiles.map((entry) => ({ + filePath: entry.filePath, + diffPositionsByLine: entry.diffPositionsByLine, + })), + }); + if (disposed) return; + const severitySummary = tallySeveritySummary(findings); + const endedAt = nowIso(); + const cancelledDuringPublish = cancelledRuns.has(runId); + if (cancelledDuringPublish) cancelledRuns.delete(runId); + updateRun(runId, { + status: cancelledDuringPublish ? "cancelled" : "completed", + summary: adjudication.summary, + error_message: null, + finding_count: findings.length, + severity_summary_json: serializeSeveritySummary(severitySummary), + ended_at: endedAt, + updated_at: endedAt, + }); + const finalStatus = cancelledDuringPublish ? "cancelled" : "completed"; + emit({ type: "run-completed", runId, laneId: run.laneId, status: finalStatus }); + emit({ type: "runs-updated", runId, laneId: run.laneId, status: finalStatus }); + } catch (error) { + if (disposed) return; + const endedAt = nowIso(); + updateRun(runId, { + status: "failed", + error_message: getErrorMessage(error), + ended_at: endedAt, + updated_at: endedAt, + }); + logger.warn("review.run_failed", { + runId, + projectRoot, + error: getErrorMessage(error), + }); + const row = getRunRow(runId); + emit({ + type: "run-completed", + runId, + laneId: row?.lane_id ?? "", + status: "failed", + }); + emit({ type: "runs-updated", runId, laneId: row?.lane_id ?? "", status: "failed" }); + } finally { + activeRuns.delete(runId); + cancelledRuns.delete(runId); + } + } + + async function startRun(args: ReviewStartRunArgs): Promise { + assertNotDisposed(); + const laneId = resolveTargetLaneId(args.target); + laneService.getLaneBaseAndBranch(laneId); + if (args.target.mode === "pr" && !prService) { + throw new Error("PR-backed review runs are not available in this workspace."); + } + const config = resolveConfig(args.target, args.config); + const startedAt = nowIso(); + const run: ReviewRun = { + id: randomUUID(), + projectId, + laneId, + target: args.target, + config, + targetLabel: args.target.mode === "commit_range" + ? `${laneId} ${args.target.baseCommit.slice(0, 7)}..${args.target.headCommit.slice(0, 7)}` + : args.target.mode === "pr" + ? `PR ${args.target.prId}` + : args.target.mode === "working_tree" + ? `${laneId} working tree` + : `${laneId} review`, + compareTarget: null, + status: "queued", + summary: null, + errorMessage: null, + findingCount: 0, + severitySummary: defaultSeveritySummary(), + chatSessionId: null, + createdAt: startedAt, + startedAt, + endedAt: null, + updatedAt: startedAt, + }; + insertRun(run); + emit({ type: "run-started", runId: run.id, laneId }); + emit({ type: "runs-updated", runId: run.id, laneId, status: "queued" }); + void executeRun(run.id); + return run; + } + + async function rerun(runId: string): Promise { + assertNotDisposed(); + const row = getRunRow(runId); + if (!row) throw new Error(`Review run '${runId}' was not found.`); + const existing = mapRunRow(row); + return startRun({ + target: existing.target, + config: existing.config, + }); + } + + async function listRuns(args: ReviewListRunsArgs = {}): Promise { + assertNotDisposed(); + const limit = Math.max(1, Math.min(200, Math.floor(args.limit ?? 50))); + const sql = [ + "select * from review_runs where project_id = ?", + args.laneId ? "and lane_id = ?" : "", + args.status && args.status !== "all" ? "and status = ?" : "", + "order by created_at desc limit ?", + ].join(" "); + const params: Array = [projectId]; + if (args.laneId) params.push(args.laneId); + if (args.status && args.status !== "all") params.push(args.status); + params.push(limit); + return db.all(sql, params).map(mapRunRow); + } + + async function getRunDetail(args: { runId: string }): Promise { + assertNotDisposed(); + const row = getRunRow(args.runId); + if (!row) return null; + const run = mapRunRow(row); + const rawFindings = db.all( + `select * from review_findings + where run_id = ? + order by + case severity + when 'critical' then 0 + when 'high' then 1 + when 'medium' then 2 + when 'low' then 3 + else 4 + end asc, + coalesce(file_path, '') asc, + coalesce(line, 2147483647) asc, + title asc`, + [args.runId], + ).map(mapFindingRow); + const feedbackByFinding = new Map(); + const feedbackRows = db.all( + "select * from review_finding_feedback where run_id = ? order by created_at asc", + [args.runId], + ); + for (const feedbackRow of feedbackRows) { + const record = mapFeedbackRow(feedbackRow); + feedbackByFinding.set(record.findingId, record); + } + const findings = rawFindings.map((finding) => ({ + ...finding, + feedback: feedbackByFinding.get(finding.id) ?? null, + })); + const artifacts = db.all( + "select * from review_run_artifacts where run_id = ? order by created_at asc", + [args.runId], + ).map(mapArtifactRow); + const publications = db.all( + "select * from review_run_publications where run_id = ? order by created_at asc", + [args.runId], + ).map(mapPublicationRow); + const chatSession = run.chatSessionId + ? await agentChatService.getSessionSummary(run.chatSessionId).catch(() => null) + : null; + return { + ...run, + findings, + artifacts, + publications, + chatSession, + }; + } + + async function cancelRun(args: { runId: string }): Promise { + assertNotDisposed(); + const row = getRunRow(args.runId); + if (!row) return null; + if (row.status === "completed" || row.status === "failed" || row.status === "cancelled") { + return mapRunRow(row); + } + cancelledRuns.add(args.runId); + const endedAt = nowIso(); + updateRun(args.runId, { + status: "cancelled", + summary: row.summary ?? "Cancellation requested; finishing current pass.", + ended_at: endedAt, + updated_at: endedAt, + }); + const refreshed = getRunRow(args.runId); + if (refreshed) { + emit({ type: "runs-updated", runId: args.runId, laneId: refreshed.lane_id, status: "cancelled" }); + } + return refreshed ? mapRunRow(refreshed) : null; + } + + async function recordFeedback(args: ReviewRecordFeedbackArgs): Promise { + assertNotDisposed(); + const findingRow = db.get( + `select rf.*, rr.project_id as project_id + from review_findings rf + join review_runs rr on rr.id = rf.run_id + where rf.id = ? limit 1`, + [args.findingId], + ); + if (!findingRow) throw new Error(`Finding '${args.findingId}' was not found.`); + if (findingRow.project_id !== projectId) { + throw new Error("Cannot record feedback for a finding outside this project."); + } + const snoozeUntil = args.snoozeDurationMs && args.snoozeDurationMs > 0 + ? new Date(Date.now() + Math.min(args.snoozeDurationMs, 1000 * 60 * 60 * 24 * 365)).toISOString() + : null; + const record: ReviewFeedbackRecord = { + id: `rfb_${randomUUID()}`, + findingId: args.findingId, + runId: findingRow.run_id, + kind: args.kind, + reason: args.reason ?? null, + note: args.note ?? null, + snoozeUntil, + createdAt: nowIso(), + }; + db.run( + `insert into review_finding_feedback ( + id, finding_id, run_id, project_id, kind, reason, note, snooze_until, created_at + ) values (?,?,?,?,?,?,?,?,?)`, + [ + record.id, + record.findingId, + record.runId, + projectId, + record.kind, + record.reason, + record.note, + record.snoozeUntil, + record.createdAt, + ], + ); + + if (args.kind === "suppress") { + const requestedScope: ReviewSuppressionScope = args.suppression?.scope ?? "repo"; + const requestedPathPattern = args.suppression?.pathPattern + ?? (requestedScope === "path" ? findingRow.file_path : null); + const hasUsablePathPattern = typeof requestedPathPattern === "string" && requestedPathPattern.trim().length > 0; + const scope: ReviewSuppressionScope = requestedScope === "path" && !hasUsablePathPattern + ? "repo" + : requestedScope; + const pathPattern = scope === "path" ? requestedPathPattern : null; + const finding = mapFindingRow(findingRow); + const publicationRow = db.get( + "select * from review_run_publications where run_id = ? order by created_at desc limit 1", + [findingRow.run_id], + ); + const destination = publicationRow ? mapPublicationRow(publicationRow).destination : null; + const repoKey = deriveRepoKey(destination, projectId); + await suppressionService + .create({ + scope, + title: finding.title, + repoKey: scope === "global" ? null : repoKey, + pathPattern, + findingClass: finding.findingClass ?? null, + severity: finding.severity, + reason: args.reason ?? null, + note: args.note ?? null, + sourceFindingId: finding.id, + seedText: `${finding.title}\n${finding.body}`, + }) + .catch((error) => { + logger.warn("review.suppression.create_failed", { + findingId: finding.id, + error: error instanceof Error ? error.message : String(error), + }); + }); + emit({ type: "suppressions-updated" }); + } + + emit({ type: "feedback-updated", findingId: args.findingId, runId: findingRow.run_id }); + return record; + } + + async function listSuppressions(args: ReviewListSuppressionsArgs = {}): Promise { + assertNotDisposed(); + return suppressionService.list({ limit: args.limit ?? null, scope: args.scope ?? null }); + } + + async function deleteSuppression(args: { suppressionId: string }): Promise { + assertNotDisposed(); + const removed = suppressionService.remove(args.suppressionId); + if (removed) emit({ type: "suppressions-updated" }); + return removed; + } + + async function qualityReport(): Promise { + assertNotDisposed(); + const totalRunsRow = db.get<{ n: number }>( + "select count(*) as n from review_runs where project_id = ?", + [projectId], + ); + const totalFindingsRow = db.get<{ n: number }>( + `select count(*) as n from review_findings rf + join review_runs rr on rr.id = rf.run_id + where rr.project_id = ?`, + [projectId], + ); + const publishedRow = db.get<{ n: number }>( + `select count(*) as n from review_findings rf + join review_runs rr on rr.id = rf.run_id + where rr.project_id = ? and rf.publication_state = 'published'`, + [projectId], + ); + // Count each finding once, using only its latest feedback entry. Without + // the row_number() filter, a user who toggled feedback (e.g. acknowledge + // then dismiss) would be counted twice and noiseRate could exceed 1.0. + const feedbackCounts = db.all<{ kind: string; n: number }>( + `with latest as ( + select finding_id, kind, + row_number() over (partition by finding_id order by created_at desc) as rn + from review_finding_feedback + where project_id = ? + ) + select kind, count(*) as n from latest where rn = 1 group by kind`, + [projectId], + ); + const kindMap = new Map(feedbackCounts.map((row) => [row.kind, Number(row.n ?? 0)])); + const byClassRows = db.all<{ finding_class: string | null; total: number; addressed: number }>( + `with latest_only as ( + select finding_id, kind from ( + select finding_id, kind, + row_number() over (partition by finding_id order by created_at desc) as rn + from review_finding_feedback + where project_id = ? + ) where rn = 1 + ) + select rf.finding_class as finding_class, + count(*) as total, + sum(case when fb.kind = 'acknowledge' then 1 else 0 end) as addressed + from review_findings rf + join review_runs rr on rr.id = rf.run_id + left join latest_only fb on fb.finding_id = rf.id + where rr.project_id = ? + group by rf.finding_class + order by total desc + limit 20`, + [projectId, projectId], + ); + const recentFeedback = db.all( + "select * from review_finding_feedback where project_id = ? order by created_at desc limit 20", + [projectId], + ).map(mapFeedbackRow); + + const totalFindings = Number(totalFindingsRow?.n ?? 0); + const dismissedCount = kindMap.get("dismiss") ?? 0; + const suppressedCount = kindMap.get("suppress") ?? 0; + const addressedCount = kindMap.get("acknowledge") ?? 0; + const snoozedCount = kindMap.get("snooze") ?? 0; + const noiseRate = totalFindings > 0 + ? Math.max(0, Math.min(1, Number(((dismissedCount + suppressedCount) / totalFindings).toFixed(3)))) + : 0; + + return { + projectId, + totalRuns: Number(totalRunsRow?.n ?? 0), + totalFindings, + addressedCount, + dismissedCount, + snoozedCount, + suppressedCount, + publishedCount: Number(publishedRow?.n ?? 0), + noiseRate, + recentFeedback, + byClass: byClassRows.map((row) => ({ + findingClass: (row.finding_class as ReviewFindingClass | null) ?? "uncategorized", + total: Number(row.total ?? 0), + addressed: Number(row.addressed ?? 0), + })), + }; + } + + return { + listLaunchContext, + startRun, + rerun, + cancelRun, + listRuns, + getRunDetail, + recordFeedback, + listSuppressions, + deleteSuppression, + qualityReport, + dispose() { + disposed = true; + activeRuns.clear(); + cancelledRuns.clear(); + }, + }; +} diff --git a/apps/desktop/src/main/services/review/reviewSuppressionService.test.ts b/apps/desktop/src/main/services/review/reviewSuppressionService.test.ts new file mode 100644 index 000000000..f7e1bad18 --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewSuppressionService.test.ts @@ -0,0 +1,336 @@ +import path from "node:path"; +import { createRequire } from "node:module"; +import initSqlJs from "sql.js"; +import type { Database, SqlJsStatic } from "sql.js"; +import { beforeAll, beforeEach, describe, expect, it } from "vitest"; +import { createReviewSuppressionService } from "./reviewSuppressionService"; + +type SqlValue = string | number | null | Uint8Array; +type AdeDb = { + run: (sql: string, params?: SqlValue[]) => void; + get: = Record>(sql: string, params?: SqlValue[]) => T | null; + all: = Record>(sql: string, params?: SqlValue[]) => T[]; +}; + +function mapExecRows(rows: { columns: string[]; values: unknown[][] }[]): Record[] { + const first = rows[0]; + if (!first) return []; + return first.values.map((row) => { + const out: Record = {}; + first.columns.forEach((column, index) => { + out[column] = row[index]; + }); + return out; + }); +} + +let SQL: SqlJsStatic; + +beforeAll(async () => { + const require = createRequire(import.meta.url); + const wasmPath = require.resolve("sql.js/dist/sql-wasm.wasm"); + const wasmDir = path.dirname(wasmPath); + SQL = await initSqlJs({ + locateFile: (file) => path.join(wasmDir, file), + }); +}); + +function createDb(): { raw: Database; db: AdeDb } { + const raw = new SQL.Database(); + raw.run(` + create table review_suppressions( + id text primary key, + project_id text not null, + scope text not null, + repo_key text, + path_pattern text, + title text not null, + title_norm text not null, + finding_class text, + severity text, + reason text, + note text, + embedding_json text, + source_finding_id text, + hit_count integer not null default 0, + created_at text not null, + last_matched_at text + ) + `); + const db: AdeDb = { + run: (sql, params = []) => raw.run(sql, params), + all: >(sql: string, params: SqlValue[] = []) => + mapExecRows(raw.exec(sql, params)) as T[], + get: >(sql: string, params: SqlValue[] = []) => + (mapExecRows(raw.exec(sql, params))[0] ?? null) as T | null, + }; + return { raw, db }; +} + +const logger = { + info: () => undefined, + warn: () => undefined, + error: () => undefined, + debug: () => undefined, + trace: () => undefined, + child: () => logger, +} as unknown as Parameters[0]["logger"]; + +describe("reviewSuppressionService", () => { + let dbHandle: ReturnType; + beforeEach(() => { + dbHandle = createDb(); + }); + + it("creates a suppression and lists it back", async () => { + const svc = createReviewSuppressionService({ + db: dbHandle.db as unknown as import("../state/kvDb").AdeDb, + logger, + projectId: "proj-1", + }); + await svc.create({ + scope: "repo", + title: "prefer async/await over raw promises", + repoKey: "arul28/ade", + reason: "style_only", + }); + const list = svc.list(); + expect(list.length).toBe(1); + expect(list[0]?.title).toContain("async/await"); + expect(list[0]?.scope).toBe("repo"); + }); + + it("matches a near-duplicate finding by title tokens", async () => { + const svc = createReviewSuppressionService({ + db: dbHandle.db as unknown as import("../state/kvDb").AdeDb, + logger, + projectId: "proj-1", + }); + await svc.create({ + scope: "repo", + title: "Prefer async await over raw promise chains", + repoKey: "arul28/ade", + reason: "low_value_noise", + }); + const hit = await svc.match({ + finding: { + title: "Prefer async await instead of raw promise chains", + body: "...", + filePath: "src/foo.ts", + findingClass: null, + severity: "low", + }, + repoKey: "arul28/ade", + }); + expect(hit).not.toBeNull(); + expect(hit!.similarity).toBeGreaterThanOrEqual(0.55); + expect(hit!.scope).toBe("repo"); + }); + + it("does not match unrelated findings", async () => { + const svc = createReviewSuppressionService({ + db: dbHandle.db as unknown as import("../state/kvDb").AdeDb, + logger, + projectId: "proj-1", + }); + await svc.create({ + scope: "repo", + title: "Inline styles instead of Tailwind classes", + repoKey: "arul28/ade", + reason: "style_only", + }); + const hit = await svc.match({ + finding: { + title: "Race condition in auth middleware", + body: "Concurrent requests can corrupt the session store", + filePath: "src/auth/middleware.ts", + findingClass: null, + severity: "high", + }, + repoKey: "arul28/ade", + }); + expect(hit).toBeNull(); + }); + + it("respects path-scoped patterns", async () => { + const svc = createReviewSuppressionService({ + db: dbHandle.db as unknown as import("../state/kvDb").AdeDb, + logger, + projectId: "proj-1", + }); + await svc.create({ + scope: "path", + title: "Magic number usage — prefer named constant", + pathPattern: "src/math/**", + reason: "low_value_noise", + }); + const hit = await svc.match({ + finding: { + title: "magic number usage, prefer named constant", + body: "", + filePath: "src/math/util.ts", + findingClass: null, + severity: "low", + }, + repoKey: "whatever", + }); + expect(hit?.scope).toBe("path"); + + const miss = await svc.match({ + finding: { + title: "magic number usage, prefer named constant", + body: "", + filePath: "src/other.ts", + findingClass: null, + severity: "low", + }, + repoKey: "whatever", + }); + expect(miss).toBeNull(); + }); + + it("path glob `src/**/*.ts` matches both shallow and nested files", async () => { + const svc = createReviewSuppressionService({ + db: dbHandle.db as unknown as import("../state/kvDb").AdeDb, + logger, + projectId: "proj-1", + }); + await svc.create({ + scope: "path", + title: "Prefer named export over default in TS module", + pathPattern: "src/**/*.ts", + reason: "style_only", + }); + + const shallow = await svc.match({ + finding: { + title: "prefer named export over default in ts module", + body: "", + filePath: "src/foo.ts", + findingClass: null, + severity: "low", + }, + repoKey: "r", + }); + expect(shallow).not.toBeNull(); + + const nested = await svc.match({ + finding: { + title: "prefer named export over default in ts module", + body: "", + filePath: "src/a/b/c/foo.ts", + findingClass: null, + severity: "low", + }, + repoKey: "r", + }); + expect(nested).not.toBeNull(); + + const wrongExt = await svc.match({ + finding: { + title: "prefer named export over default in ts module", + body: "", + filePath: "src/foo.tsx", + findingClass: null, + severity: "low", + }, + repoKey: "r", + }); + expect(wrongExt).toBeNull(); + }); + + it("path glob `?` matches exactly one non-slash character", async () => { + const svc = createReviewSuppressionService({ + db: dbHandle.db as unknown as import("../state/kvDb").AdeDb, + logger, + projectId: "proj-1", + }); + await svc.create({ + scope: "path", + title: "Numbered helper lint", + pathPattern: "src/helper?.ts", + reason: "style_only", + }); + + const single = await svc.match({ + finding: { + title: "numbered helper lint", + body: "", + filePath: "src/helper1.ts", + findingClass: null, + severity: "low", + }, + repoKey: "r", + }); + expect(single).not.toBeNull(); + + const twoChars = await svc.match({ + finding: { + title: "numbered helper lint", + body: "", + filePath: "src/helper12.ts", + findingClass: null, + severity: "low", + }, + repoKey: "r", + }); + expect(twoChars).toBeNull(); + + const slashed = await svc.match({ + finding: { + title: "numbered helper lint", + body: "", + filePath: "src/helper/.ts", + findingClass: null, + severity: "low", + }, + repoKey: "r", + }); + expect(slashed).toBeNull(); + }); + + it("title-match fallback fires when embeddings are weak/missing but tokens overlap", async () => { + // No embeddingService is provided, so both sides fall back to the + // title-token (Jaccard) path with the TITLE_SIM_THRESHOLD bar (0.55). + // The pre-fix logic applied the embedding threshold here and rejected. + const svc = createReviewSuppressionService({ + db: dbHandle.db as unknown as import("../state/kvDb").AdeDb, + logger, + projectId: "proj-1", + }); + await svc.create({ + scope: "repo", + title: "Prefer async await over raw promise chains", + repoKey: "arul28/ade", + reason: "style_only", + }); + const hit = await svc.match({ + finding: { + title: "Prefer async await over raw promise chains", + body: "", + filePath: "src/anywhere.ts", + findingClass: null, + severity: "low", + }, + repoKey: "arul28/ade", + }); + expect(hit).not.toBeNull(); + // Identical titles → Jaccard = 1.0, comfortably above TITLE_SIM_THRESHOLD. + expect(hit!.similarity).toBeGreaterThanOrEqual(0.55); + }); + + it("removes a suppression", async () => { + const svc = createReviewSuppressionService({ + db: dbHandle.db as unknown as import("../state/kvDb").AdeDb, + logger, + projectId: "proj-1", + }); + const created = await svc.create({ + scope: "global", + title: "dead code removal nit", + reason: "low_value_noise", + }); + expect(svc.remove(created.id)).toBe(true); + expect(svc.list()).toEqual([]); + }); +}); diff --git a/apps/desktop/src/main/services/review/reviewSuppressionService.ts b/apps/desktop/src/main/services/review/reviewSuppressionService.ts new file mode 100644 index 000000000..3f531d365 --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewSuppressionService.ts @@ -0,0 +1,331 @@ +import { randomUUID } from "node:crypto"; +import type { + ReviewDismissReason, + ReviewFinding, + ReviewFindingSuppressionMatch, + ReviewSuppression, + ReviewSuppressionScope, +} from "../../../shared/types"; +import type { Logger } from "../logging/logger"; +import type { AdeDb } from "../state/kvDb"; +import { nowIso, safeJsonParse } from "../shared/utils"; +import type { EmbeddingService } from "../memory/embeddingService"; + +const TITLE_SIM_THRESHOLD = 0.55; +const EMBEDDING_SIM_THRESHOLD = 0.78; +const PATH_GLOB_CACHE_MAX = 256; +const PATH_GLOB_CACHE = new Map(); + +function globToRegExp(pattern: string): RegExp { + const cached = PATH_GLOB_CACHE.get(pattern); + if (cached) return cached; + let source = ""; + let i = 0; + while (i < pattern.length) { + const ch = pattern[i]; + if (ch === "*") { + if (pattern[i + 1] === "*") { + // `**/` means "zero or more path segments" — needs to match both + // `src/foo.ts` and `src/a/b/foo.ts` against `src/**/*.ts`. + // Swallow the trailing slash and make the whole segment optional. + if (pattern[i + 2] === "/") { + source += "(?:.*/)?"; + i += 3; + } else { + source += ".*"; + i += 2; + } + } else { + source += "[^/]*"; + i += 1; + } + continue; + } + if (ch === "?") { + source += "[^/]"; + i += 1; + continue; + } + if ("/.+()|^$[]{}\\".includes(ch)) { + source += `\\${ch}`; + } else { + source += ch; + } + i += 1; + } + const re = new RegExp(`^${source}$`); + if (PATH_GLOB_CACHE.size >= PATH_GLOB_CACHE_MAX) { + const oldest = PATH_GLOB_CACHE.keys().next().value; + if (oldest !== undefined) PATH_GLOB_CACHE.delete(oldest); + } + PATH_GLOB_CACHE.set(pattern, re); + return re; +} + +function normalizeTitle(title: string): string { + return title + .toLowerCase() + .replace(/[^a-z0-9]+/g, " ") + .trim(); +} + +function jaccard(left: Set, right: Set): number { + if (left.size === 0 && right.size === 0) return 0; + let intersection = 0; + for (const token of left) if (right.has(token)) intersection += 1; + const union = left.size + right.size - intersection; + return union === 0 ? 0 : intersection / union; +} + +function tokenize(value: string): Set { + const tokens = normalizeTitle(value).split(" ").filter((token) => token.length >= 3); + return new Set(tokens); +} + +function cosine(a: number[] | null, b: number[] | null): number { + if (!a || !b || a.length === 0 || a.length !== b.length) return 0; + let dot = 0; + let an = 0; + let bn = 0; + for (let i = 0; i < a.length; i += 1) { + dot += a[i]! * b[i]!; + an += a[i]! * a[i]!; + bn += b[i]! * b[i]!; + } + if (an === 0 || bn === 0) return 0; + return dot / (Math.sqrt(an) * Math.sqrt(bn)); +} + +function pathMatchesScope( + candidatePath: string | null, + scope: ReviewSuppressionScope, + pattern: string | null, +): boolean { + if (scope === "global") return true; + if (scope === "repo") return true; + if (scope === "path") { + if (!pattern) return false; + if (!candidatePath) return false; + return globToRegExp(pattern).test(candidatePath); + } + return true; +} + +type ReviewSuppressionRow = { + id: string; + project_id: string; + scope: string; + repo_key: string | null; + path_pattern: string | null; + title: string; + title_norm: string; + finding_class: string | null; + severity: string | null; + reason: string | null; + note: string | null; + embedding_json: string | null; + source_finding_id: string | null; + hit_count: number; + created_at: string; + last_matched_at: string | null; +}; + +function mapRow(row: ReviewSuppressionRow): ReviewSuppression { + return { + id: row.id, + scope: (row.scope as ReviewSuppressionScope) ?? "repo", + repoKey: row.repo_key, + pathPattern: row.path_pattern, + title: row.title, + findingClass: (row.finding_class as ReviewSuppression["findingClass"]) ?? null, + severity: (row.severity as ReviewSuppression["severity"]) ?? null, + reason: (row.reason as ReviewDismissReason | null) ?? null, + note: row.note, + embedding: safeJsonParse(row.embedding_json, null), + sourceFindingId: row.source_finding_id, + hitCount: Number(row.hit_count ?? 0), + createdAt: row.created_at, + lastMatchedAt: row.last_matched_at, + }; +} + +export type ReviewSuppressionService = ReturnType; + +export function createReviewSuppressionService({ + db, + logger, + projectId, + embeddingService, +}: { + db: AdeDb; + logger: Logger; + projectId: string; + embeddingService?: Pick | null; +}) { + async function tryEmbed(text: string): Promise { + if (!embeddingService || !text.trim()) return null; + try { + const vector = await embeddingService.embed(text); + return Array.from(vector); + } catch (error) { + logger.warn("review.suppression.embed_failed", { + error: error instanceof Error ? error.message : String(error), + }); + return null; + } + } + + async function create(args: { + scope: ReviewSuppressionScope; + title: string; + repoKey?: string | null; + pathPattern?: string | null; + findingClass?: ReviewSuppression["findingClass"]; + severity?: ReviewSuppression["severity"]; + reason?: ReviewDismissReason | null; + note?: string | null; + sourceFindingId?: string | null; + seedText?: string | null; + }): Promise { + const embeddingText = args.seedText ?? args.title; + const embedding = await tryEmbed(embeddingText); + const row: ReviewSuppression = { + id: `rsup_${randomUUID()}`, + scope: args.scope, + repoKey: args.repoKey ?? null, + pathPattern: args.pathPattern ?? null, + title: args.title, + findingClass: args.findingClass ?? null, + severity: args.severity ?? null, + reason: args.reason ?? null, + note: args.note ?? null, + embedding, + sourceFindingId: args.sourceFindingId ?? null, + hitCount: 0, + createdAt: nowIso(), + lastMatchedAt: null, + }; + db.run( + `insert into review_suppressions ( + id, project_id, scope, repo_key, path_pattern, title, title_norm, + finding_class, severity, reason, note, embedding_json, source_finding_id, + hit_count, created_at, last_matched_at + ) values (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)`, + [ + row.id, + projectId, + row.scope, + row.repoKey, + row.pathPattern, + row.title, + normalizeTitle(row.title), + row.findingClass, + row.severity, + row.reason, + row.note, + embedding ? JSON.stringify(embedding) : null, + row.sourceFindingId, + 0, + row.createdAt, + null, + ], + ); + return row; + } + + function list(args: { + limit?: number | null; + scope?: ReviewSuppressionScope | null; + } = {}): ReviewSuppression[] { + const limit = Math.max(1, Math.min(500, args.limit ?? 200)); + const where: string[] = ["project_id = ?"]; + const params: Array = [projectId]; + if (args.scope) { + where.push("scope = ?"); + params.push(args.scope); + } + const rows = db.all( + `select * from review_suppressions where ${where.join(" and ")} order by created_at desc limit ${limit}`, + params, + ); + return rows.map(mapRow); + } + + function remove(suppressionId: string): boolean { + const row = db.get<{ id: string }>( + "select id from review_suppressions where id = ? and project_id = ? limit 1", + [suppressionId, projectId], + ); + if (!row) return false; + db.run("delete from review_suppressions where id = ?", [suppressionId]); + return true; + } + + function recordHit(suppressionId: string): void { + db.run( + "update review_suppressions set hit_count = hit_count + 1, last_matched_at = ? where id = ?", + [nowIso(), suppressionId], + ); + } + + async function match(args: { + finding: Pick; + repoKey?: string | null; + }): Promise { + const rows = db.all( + "select * from review_suppressions where project_id = ? order by created_at desc limit 500", + [projectId], + ); + if (rows.length === 0) return null; + + const findingText = `${args.finding.title} ${args.finding.body}`.trim(); + const candidateEmbedding = await tryEmbed(findingText); + const findingTokens = tokenize(args.finding.title); + + let best: { row: ReviewSuppression; similarity: number } | null = null; + for (const raw of rows) { + const suppression = mapRow(raw); + if (!pathMatchesScope(args.finding.filePath, suppression.scope, suppression.pathPattern)) continue; + if (suppression.scope === "repo" && suppression.repoKey && args.repoKey && suppression.repoKey !== args.repoKey) { + continue; + } + if (suppression.findingClass && args.finding.findingClass && suppression.findingClass !== args.finding.findingClass) { + continue; + } + + // Score each candidate against its strongest signal: + // - if both sides have embeddings, trust cosine with the embedding bar + // - otherwise fall back to Jaccard-over-title-tokens with a lower bar + // Previously we mixed the two (jaccard could overwrite cosine but then + // get compared to the embedding threshold), which rejected real + // title-matches when embeddings happened to be weak. + const haveBothEmbeddings = Boolean(candidateEmbedding && suppression.embedding); + const cosineScore = haveBothEmbeddings + ? cosine(candidateEmbedding, suppression.embedding) + : 0; + if (haveBothEmbeddings && cosineScore >= EMBEDDING_SIM_THRESHOLD) { + const score = cosineScore; + if (!best || score > best.similarity) best = { row: suppression, similarity: score }; + continue; + } + const titleScore = jaccard(findingTokens, tokenize(suppression.title)); + if (titleScore < TITLE_SIM_THRESHOLD) continue; + const score = Math.max(cosineScore, titleScore); + + if (!best || score > best.similarity) { + best = { row: suppression, similarity: score }; + } + } + + if (!best) return null; + recordHit(best.row.id); + return { + suppressionId: best.row.id, + similarity: Number(best.similarity.toFixed(4)), + reason: best.row.reason, + scope: best.row.scope, + }; + } + + return { create, list, remove, match }; +} diff --git a/apps/desktop/src/main/services/review/reviewTargetMaterializer.test.ts b/apps/desktop/src/main/services/review/reviewTargetMaterializer.test.ts new file mode 100644 index 000000000..8941e708d --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewTargetMaterializer.test.ts @@ -0,0 +1,258 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { ReviewRunConfig } from "../../../shared/types"; + +const mockGit = vi.hoisted(() => ({ + runGit: vi.fn(), + runGitOrThrow: vi.fn(), +})); + +vi.mock("../git/git", () => ({ + runGit: (...args: unknown[]) => mockGit.runGit(...args), + runGitOrThrow: (...args: unknown[]) => mockGit.runGitOrThrow(...args), +})); + +import { createReviewTargetMaterializer } from "./reviewTargetMaterializer"; + +function makeConfig(overrides: Partial = {}): ReviewRunConfig { + return { + compareAgainst: { kind: "default_branch" }, + selectionMode: "full_diff", + dirtyOnly: false, + modelId: "openai/gpt-5.4-codex", + reasoningEffort: "medium", + budgets: { + maxFiles: 60, + maxDiffChars: 180_000, + maxPromptChars: 220_000, + maxFindings: 12, + }, + publishBehavior: "local_only", + ...overrides, + }; +} + +describe("reviewTargetMaterializer", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("materializes lane vs default branch full diff", async () => { + const laneService = { + getLaneBaseAndBranch: vi.fn().mockImplementation((laneId: string) => ({ + baseRef: "main", + branchRef: laneId === "lane-review" ? "feature/review-tab" : "bugfix/review-engine", + worktreePath: "/tmp/lane-review", + laneType: "worktree", + })), + list: vi.fn(), + } as any; + mockGit.runGitOrThrow + .mockResolvedValueOnce("merge-base-sha\n") + .mockResolvedValueOnce("diff --git a/src/review.ts b/src/review.ts\n@@ -10,2 +10,3 @@\n line\n+new line\n") + .mockResolvedValueOnce("M\tsrc/review.ts\n"); + + const materializer = createReviewTargetMaterializer({ laneService }); + const result = await materializer.materialize({ + target: { mode: "lane_diff", laneId: "lane-review" }, + config: makeConfig(), + }); + + expect(mockGit.runGitOrThrow).toHaveBeenCalledWith( + ["merge-base", "main", "feature/review-tab"], + expect.objectContaining({ cwd: "/tmp/lane-review" }), + ); + expect(result.targetLabel).toBe("feature/review-tab vs main"); + expect(result.compareTarget).toEqual({ + kind: "default_branch", + label: "main", + ref: "main", + laneId: null, + branchRef: "main", + }); + expect(result.changedFiles[0]?.filePath).toBe("src/review.ts"); + }); + + it("materializes lane vs another lane", async () => { + const laneService = { + getLaneBaseAndBranch: vi.fn().mockImplementation((laneId: string) => ({ + baseRef: "main", + branchRef: laneId === "lane-review" ? "feature/review-tab" : "bugfix/review-engine", + worktreePath: "/tmp/lane-review", + laneType: "worktree", + })), + list: vi.fn(), + } as any; + mockGit.runGitOrThrow + .mockResolvedValueOnce("merge-base-sha\n") + .mockResolvedValueOnce("diff --git a/src/engine.ts b/src/engine.ts\n@@ -20,2 +20,3 @@\n line\n+new line\n") + .mockResolvedValueOnce("M\tsrc/engine.ts\n"); + + const materializer = createReviewTargetMaterializer({ laneService }); + const result = await materializer.materialize({ + target: { mode: "lane_diff", laneId: "lane-review" }, + config: makeConfig({ + compareAgainst: { kind: "lane", laneId: "lane-bugfix" }, + }), + }); + + expect(mockGit.runGitOrThrow).toHaveBeenCalledWith( + ["merge-base", "bugfix/review-engine", "feature/review-tab"], + expect.objectContaining({ cwd: "/tmp/lane-review" }), + ); + expect(result.compareTarget).toEqual({ + kind: "lane", + label: "bugfix/review-engine", + ref: "bugfix/review-engine", + laneId: "lane-bugfix", + branchRef: "bugfix/review-engine", + }); + }); + + it("materializes a selected commit range", async () => { + const laneService = { + getLaneBaseAndBranch: vi.fn().mockReturnValue({ + baseRef: "main", + branchRef: "feature/review-tab", + worktreePath: "/tmp/lane-review", + laneType: "worktree", + }), + list: vi.fn(), + } as any; + mockGit.runGitOrThrow + .mockResolvedValueOnce("diff --git a/src/commit.ts b/src/commit.ts\n@@ -1,1 +1,2 @@\n+change\n") + .mockResolvedValueOnce("M\tsrc/commit.ts\n"); + + const materializer = createReviewTargetMaterializer({ laneService }); + const result = await materializer.materialize({ + target: { + mode: "commit_range", + laneId: "lane-review", + baseCommit: "abc123456789", + headCommit: "def456789012", + }, + config: makeConfig({ + selectionMode: "selected_commits", + }), + }); + + expect(mockGit.runGitOrThrow).toHaveBeenCalledWith( + ["diff", "--no-color", "--find-renames", "abc123456789..def456789012"], + expect.objectContaining({ cwd: "/tmp/lane-review" }), + ); + expect(result.targetLabel).toContain("abc1234..def4567"); + }); + + it("materializes staged, unstaged, and untracked working tree changes", async () => { + const worktreePath = fs.mkdtempSync(path.join(os.tmpdir(), "ade-review-working-tree-")); + try { + fs.mkdirSync(path.join(worktreePath, "src"), { recursive: true }); + fs.writeFileSync(path.join(worktreePath, "src", "new-file.ts"), "export const untracked = true;\n", "utf8"); + + const laneService = { + getLaneBaseAndBranch: vi.fn().mockReturnValue({ + baseRef: "main", + branchRef: "feature/review-tab", + worktreePath, + laneType: "worktree", + }), + list: vi.fn(), + } as any; + mockGit.runGitOrThrow + .mockResolvedValueOnce("diff --git a/src/staged.ts b/src/staged.ts\n@@ -1,1 +1,2 @@\n+staged change\n") + .mockResolvedValueOnce("diff --git a/src/unstaged.ts b/src/unstaged.ts\n@@ -1,1 +1,2 @@\n+unstaged change\n") + .mockResolvedValueOnce("M src/staged.ts\n M src/unstaged.ts\n?? src/new-file.ts\n"); + + const materializer = createReviewTargetMaterializer({ laneService }); + const result = await materializer.materialize({ + target: { mode: "working_tree", laneId: "lane-review" }, + config: makeConfig({ + selectionMode: "dirty_only", + dirtyOnly: true, + }), + }); + + expect(result.fullPatchText).toContain("## Staged changes"); + expect(result.fullPatchText).toContain("## Unstaged changes"); + expect(result.fullPatchText).toContain("## Untracked files"); + expect(result.artifacts.some((artifact) => artifact.artifactType === "untracked_snapshot")).toBe(true); + expect(result.changedFiles).toContainEqual(expect.objectContaining({ + filePath: "src/new-file.ts", + lineNumbers: [1], + })); + expect(result.changedFiles.find((file) => file.filePath === "src/new-file.ts")?.excerpt).toContain("new file mode 100644"); + } finally { + fs.rmSync(worktreePath, { recursive: true, force: true }); + } + }); + + it("materializes a PR target and prepares GitHub publication metadata", async () => { + const laneService = { + getLaneBaseAndBranch: vi.fn().mockReturnValue({ + baseRef: "main", + branchRef: "feature/pr-80", + worktreePath: "/tmp/lane-review", + laneType: "worktree", + }), + list: vi.fn(), + } as any; + const prService = { + getReviewSnapshot: vi.fn(async () => ({ + id: "pr-80", + laneId: "lane-review", + projectId: "project-1", + repoOwner: "ade-dev", + repoName: "ade", + githubPrNumber: 80, + githubUrl: "https://github.com/ade-dev/ade/pull/80", + githubNodeId: "PR_kwDOExample", + title: "Review publication", + state: "open", + baseBranch: "main", + headBranch: "feature/pr-80", + checksStatus: "passing", + reviewStatus: "commented", + additions: 2, + deletions: 0, + lastSyncedAt: "2026-04-06T10:00:00.000Z", + createdAt: "2026-04-06T09:55:00.000Z", + updatedAt: "2026-04-06T10:00:00.000Z", + baseSha: "abc123456789", + headSha: "def456789012", + files: [ + { + filename: "src/review.ts", + status: "modified", + additions: 2, + deletions: 0, + patch: "@@ -10,1 +10,3 @@\n context\n+anchored\n+summary only\n", + previousFilename: null, + }, + ], + })), + } as any; + mockGit.runGitOrThrow.mockResolvedValueOnce( + "diff --git a/src/review.ts b/src/review.ts\n@@ -10,1 +10,3 @@\n context\n+anchored\n+summary only\n", + ); + + const materializer = createReviewTargetMaterializer({ laneService, prService }); + const result = await materializer.materialize({ + target: { mode: "pr", laneId: "lane-review", prId: "pr-80" }, + config: makeConfig({ publishBehavior: "auto_publish" }), + }); + + expect(prService.getReviewSnapshot).toHaveBeenCalledWith("pr-80"); + expect(result.targetLabel).toBe("PR #80 feature/pr-80 -> main"); + expect(result.publicationTarget).toEqual({ + kind: "github_pr_review", + prId: "pr-80", + repoOwner: "ade-dev", + repoName: "ade", + prNumber: 80, + githubUrl: "https://github.com/ade-dev/ade/pull/80", + }); + expect(result.changedFiles[0]?.diffPositionsByLine).toEqual({ 10: 1, 11: 2, 12: 3 }); + }); +}); diff --git a/apps/desktop/src/main/services/review/reviewTargetMaterializer.ts b/apps/desktop/src/main/services/review/reviewTargetMaterializer.ts new file mode 100644 index 000000000..13a9cfd6d --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewTargetMaterializer.ts @@ -0,0 +1,489 @@ +import fs from "node:fs"; +import path from "node:path"; +import type { + PrFile, + PrReviewSnapshot, + ReviewPublicationDestination, + ReviewResolvedCompareTarget, + ReviewRunArtifact, + ReviewRunConfig, + ReviewTarget, +} from "../../../shared/types"; +import type { createLaneService } from "../lanes/laneService"; +import type { createPrService } from "../prs/prService"; +import { runGit, runGitOrThrow } from "../git/git"; + +type ReviewMaterializedFile = { + filePath: string; + excerpt: string; + lineNumbers: number[]; + diffPositionsByLine: Record; +}; + +export type ReviewMaterializedTarget = { + targetLabel: string; + compareTarget: ReviewResolvedCompareTarget | null; + publicationTarget: ReviewPublicationDestination | null; + fullPatchText: string; + changedFiles: ReviewMaterializedFile[]; + artifacts: Array>; +}; + +type LaneInfo = ReturnType["getLaneBaseAndBranch"]>; + +function normalizeBranchRef(ref: string): string { + return ref.replace(/^refs\/heads\//, "").replace(/^refs\/remotes\//, ""); +} + +function truncateText(value: string, maxChars: number): string { + if (value.length <= maxChars) return value; + return `${value.slice(0, maxChars)}\n...(truncated)...\n`; +} + +function readTextFileSafe(absPath: string, maxBytes: number): { exists: boolean; text: string; isBinary: boolean } { + try { + const stat = fs.statSync(absPath); + if (!stat.isFile()) return { exists: false, text: "", isBinary: false }; + const fd = fs.openSync(absPath, "r"); + try { + const bytesToRead = Math.max(1, Math.min(stat.size, maxBytes)); + const buffer = Buffer.alloc(bytesToRead); + const bytesRead = fs.readSync(fd, buffer, 0, bytesToRead, 0); + const slice = buffer.subarray(0, bytesRead); + if (slice.includes(0)) { + return { exists: true, text: "", isBinary: true }; + } + const text = slice.toString("utf8"); + return { exists: true, text: truncateText(text, maxBytes), isBinary: false }; + } finally { + fs.closeSync(fd); + } + } catch { + return { exists: false, text: "", isBinary: false }; + } +} + +function parseNameStatus(stdout: string): string[] { + return stdout + .split(/\r?\n/) + .map((line) => line.trim()) + .filter(Boolean) + .map((line) => { + const parts = line.split("\t").filter(Boolean); + if (parts.length >= 3 && /^R\d+$/i.test(parts[0] ?? "")) { + return parts[2] ?? ""; + } + return parts.at(-1) ?? ""; + }) + .filter(Boolean); +} + +function parseDiffFiles(patchText: string, fallbackPaths: string[]): ReviewMaterializedFile[] { + const byPath = new Map; diffPositionsByLine: Map }>(); + for (const fallbackPath of fallbackPaths) { + if (!byPath.has(fallbackPath)) { + byPath.set(fallbackPath, { + lines: [], + lineNumbers: new Set(), + diffPositionsByLine: new Map(), + }); + } + } + + const lines = patchText.split(/\r?\n/); + let currentPath: string | null = null; + let currentNewLine: number | null = null; + let currentDiffPosition = 0; + + const ensureEntry = (filePath: string) => { + const existing = byPath.get(filePath); + if (existing) return existing; + const created = { + lines: [] as string[], + lineNumbers: new Set(), + diffPositionsByLine: new Map(), + }; + byPath.set(filePath, created); + return created; + }; + + for (const line of lines) { + const diffMatch = line.match(/^diff --git a\/(.+?) b\/(.+)$/); + if (diffMatch) { + const oldPath = diffMatch[1] ?? ""; + const newPath = diffMatch[2] ?? ""; + currentPath = newPath === "/dev/null" ? oldPath : newPath; + currentNewLine = null; + currentDiffPosition = 0; + continue; + } + if (!currentPath) continue; + const entry = ensureEntry(currentPath); + entry.lines.push(line); + + const hunkMatch = line.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/); + if (hunkMatch) { + currentNewLine = Number(hunkMatch[1] ?? "0"); + // GitHub: "the line just below the '@@' line is position 1". The first hunk header + // anchors position 0 (not counted); later '@@' headers each consume one position. + if (currentDiffPosition > 0) currentDiffPosition += 1; + continue; + } + if (currentNewLine == null) continue; + if (line.length === 0) continue; + if (line.startsWith("\\")) continue; + + currentDiffPosition += 1; + + if (line.startsWith("+") && !line.startsWith("+++")) { + entry.lineNumbers.add(currentNewLine); + entry.diffPositionsByLine.set(currentNewLine, currentDiffPosition); + currentNewLine += 1; + continue; + } + if (line.startsWith("-") && !line.startsWith("---")) { + continue; + } + entry.lineNumbers.add(currentNewLine); + entry.diffPositionsByLine.set(currentNewLine, currentDiffPosition); + currentNewLine += 1; + } + + return Array.from(byPath.entries()).map(([filePath, entry]) => ({ + filePath, + excerpt: truncateText(entry.lines.join("\n").trim(), 4_000), + lineNumbers: Array.from(entry.lineNumbers).sort((a, b) => a - b), + diffPositionsByLine: Object.fromEntries(entry.diffPositionsByLine.entries()), + })); +} + +async function resolveDefaultCompareTarget(lane: LaneInfo): Promise { + const branchRef = normalizeBranchRef(lane.branchRef); + const baseRef = normalizeBranchRef(lane.baseRef); + if (lane.laneType === "primary") { + const upstreamRef = `${branchRef}@{upstream}`; + const upstreamRes = await runGit(["rev-parse", "--verify", upstreamRef], { + cwd: lane.worktreePath, + timeoutMs: 5_000, + }); + if (upstreamRes.exitCode === 0 && upstreamRes.stdout.trim()) { + return { + kind: "default_branch", + label: upstreamRef, + ref: upstreamRef, + laneId: null, + branchRef: upstreamRef, + }; + } + const originRef = `origin/${branchRef}`; + const originRes = await runGit(["rev-parse", "--verify", originRef], { + cwd: lane.worktreePath, + timeoutMs: 5_000, + }); + if (originRes.exitCode === 0 && originRes.stdout.trim()) { + return { + kind: "default_branch", + label: originRef, + ref: originRef, + laneId: null, + branchRef: originRef, + }; + } + } + return { + kind: "default_branch", + label: baseRef, + ref: baseRef, + laneId: null, + branchRef: baseRef, + }; +} + +function buildDiffArtifact( + contentText: string, + metadata: Record | null = null, +): Omit { + return { + artifactType: "diff_bundle", + title: "Diff bundle", + mimeType: "text/plain", + contentText, + metadata, + }; +} + +function buildUntrackedFileDiff(filePath: string, contentText: string): string { + const normalized = contentText.replace(/\r\n/g, "\n"); + const hasTrailingNewline = normalized.endsWith("\n"); + const rawLines = normalized.length > 0 ? normalized.split("\n") : []; + const lines = hasTrailingNewline ? rawLines.slice(0, -1) : rawLines; + const hunkSize = lines.length; + return [ + `diff --git a/${filePath} b/${filePath}`, + "new file mode 100644", + "--- /dev/null", + `+++ b/${filePath}`, + `@@ -0,0 +1,${hunkSize} @@`, + ...lines.map((line) => `+${line}`), + ].join("\n"); +} + +function buildPrFileDiff(file: PrFile): string { + const diffPath = file.previousFilename ?? file.filename; + const oldPath = file.status === "added" ? "/dev/null" : `a/${file.previousFilename ?? file.filename}`; + const newPath = file.status === "removed" ? "/dev/null" : `b/${file.filename}`; + const patchBody = file.patch?.trim() ?? ""; + return [ + `diff --git a/${diffPath} b/${file.filename}`, + `--- ${oldPath}`, + `+++ ${newPath}`, + patchBody, + ].join("\n"); +} + +function buildPrFullPatch(files: PrFile[]): string { + return files.map(buildPrFileDiff).join("\n\n").trim(); +} + +function buildPrPublicationTarget(summary: Pick): ReviewPublicationDestination { + return { + kind: "github_pr_review", + prId: summary.id, + repoOwner: summary.repoOwner, + repoName: summary.repoName, + prNumber: summary.githubPrNumber, + githubUrl: summary.githubUrl, + }; +} + +export function createReviewTargetMaterializer({ + laneService, + prService, +}: { + laneService: Pick, "getLaneBaseAndBranch" | "list">; + prService?: Pick, "getReviewSnapshot">; +}) { + async function materializeLaneDiff(args: { + target: Extract; + config: ReviewRunConfig; + }): Promise { + const lane = laneService.getLaneBaseAndBranch(args.target.laneId); + const sourceRef = normalizeBranchRef(lane.branchRef); + let compareTarget = await resolveDefaultCompareTarget(lane); + if (args.config.compareAgainst.kind === "lane") { + const compareLane = laneService.getLaneBaseAndBranch(args.config.compareAgainst.laneId); + compareTarget = { + kind: "lane", + label: normalizeBranchRef(compareLane.branchRef), + ref: normalizeBranchRef(compareLane.branchRef), + laneId: args.config.compareAgainst.laneId, + branchRef: normalizeBranchRef(compareLane.branchRef), + }; + } + + const mergeBase = await runGitOrThrow(["merge-base", compareTarget.ref ?? "HEAD", sourceRef], { + cwd: lane.worktreePath, + timeoutMs: 10_000, + }).then((stdout) => stdout.trim()); + const patchText = await runGitOrThrow(["diff", "--no-color", "--find-renames", `${mergeBase}..${sourceRef}`], { + cwd: lane.worktreePath, + timeoutMs: 30_000, + maxOutputBytes: 8 * 1024 * 1024, + }); + const nameStatus = await runGitOrThrow(["diff", "--name-status", "--find-renames", `${mergeBase}..${sourceRef}`], { + cwd: lane.worktreePath, + timeoutMs: 15_000, + maxOutputBytes: 2 * 1024 * 1024, + }); + const changedFiles = parseDiffFiles(patchText, parseNameStatus(nameStatus)); + + return { + targetLabel: `${sourceRef} vs ${compareTarget.label}`, + compareTarget, + publicationTarget: null, + fullPatchText: patchText, + changedFiles, + artifacts: [buildDiffArtifact(patchText)], + }; + } + + async function materializeCommitRange(args: { + target: Extract; + }): Promise { + const lane = laneService.getLaneBaseAndBranch(args.target.laneId); + const range = `${args.target.baseCommit}..${args.target.headCommit}`; + const patchText = await runGitOrThrow(["diff", "--no-color", "--find-renames", range], { + cwd: lane.worktreePath, + timeoutMs: 30_000, + maxOutputBytes: 8 * 1024 * 1024, + }); + const nameStatus = await runGitOrThrow(["diff", "--name-status", "--find-renames", range], { + cwd: lane.worktreePath, + timeoutMs: 15_000, + maxOutputBytes: 2 * 1024 * 1024, + }); + const changedFiles = parseDiffFiles(patchText, parseNameStatus(nameStatus)); + + return { + targetLabel: `${normalizeBranchRef(lane.branchRef)} ${args.target.baseCommit.slice(0, 7)}..${args.target.headCommit.slice(0, 7)}`, + compareTarget: null, + publicationTarget: null, + fullPatchText: patchText, + changedFiles, + artifacts: [buildDiffArtifact(patchText)], + }; + } + + async function materializeWorkingTree(args: { + target: Extract; + }): Promise { + const lane = laneService.getLaneBaseAndBranch(args.target.laneId); + const branchRef = normalizeBranchRef(lane.branchRef); + const stagedPatch = await runGitOrThrow(["diff", "--cached", "--no-color", "--find-renames"], { + cwd: lane.worktreePath, + timeoutMs: 30_000, + maxOutputBytes: 8 * 1024 * 1024, + }); + const unstagedPatch = await runGitOrThrow(["diff", "--no-color", "--find-renames"], { + cwd: lane.worktreePath, + timeoutMs: 30_000, + maxOutputBytes: 8 * 1024 * 1024, + }); + const statusResult = await runGitOrThrow(["status", "--porcelain=v1"], { + cwd: lane.worktreePath, + timeoutMs: 10_000, + maxOutputBytes: 2 * 1024 * 1024, + }); + + const untrackedArtifacts: Array> = []; + const untrackedPatchSections: string[] = []; + const fallbackPaths = new Set(); + const statusLines = statusResult + .split(/\r?\n/) + .map((line) => line.trimEnd()) + .filter(Boolean); + for (const line of statusLines) { + const code = line.slice(0, 2); + const rawPath = line.slice(3).trim(); + if (!rawPath) continue; + const normalizedPath = rawPath.includes("->") + ? rawPath.split("->").at(-1)?.trim() ?? rawPath + : rawPath; + fallbackPaths.add(normalizedPath); + if (code === "??") { + const absPath = path.join(lane.worktreePath, normalizedPath); + const file = readTextFileSafe(absPath, 128_000); + if (file.exists && !file.isBinary) { + const contentText = file.text; + untrackedPatchSections.push(buildUntrackedFileDiff(normalizedPath, contentText)); + untrackedArtifacts.push({ + artifactType: "untracked_snapshot", + title: `Untracked: ${normalizedPath}`, + mimeType: "text/plain", + contentText, + metadata: { filePath: normalizedPath }, + }); + } + } + } + + const sections: string[] = []; + if (stagedPatch.trim()) { + sections.push(`## Staged changes\n${stagedPatch.trim()}`); + } + if (unstagedPatch.trim()) { + sections.push(`## Unstaged changes\n${unstagedPatch.trim()}`); + } + if (untrackedPatchSections.length > 0) { + sections.push(`## Untracked files\n${untrackedPatchSections.join("\n\n")}`); + } + const fullPatchText = sections.join("\n\n").trim(); + const changedFiles = parseDiffFiles(fullPatchText, Array.from(fallbackPaths)); + + return { + targetLabel: `${branchRef} working tree`, + compareTarget: null, + publicationTarget: null, + fullPatchText, + changedFiles, + artifacts: [buildDiffArtifact(fullPatchText), ...untrackedArtifacts], + }; + } + + async function materializePullRequest(args: { + target: Extract; + }): Promise { + if (!prService) { + throw new Error("PR review target is not available in this workspace."); + } + + const lane = laneService.getLaneBaseAndBranch(args.target.laneId); + const snapshot = await prService.getReviewSnapshot(args.target.prId); + const range = snapshot.baseSha && snapshot.headSha ? `${snapshot.baseSha}..${snapshot.headSha}` : null; + + let patchText = ""; + if (range) { + try { + patchText = await runGitOrThrow(["diff", "--no-color", "--find-renames", range], { + cwd: lane.worktreePath, + timeoutMs: 30_000, + maxOutputBytes: 8 * 1024 * 1024, + }); + } catch { + patchText = ""; + } + } + if (!patchText.trim()) { + patchText = buildPrFullPatch(snapshot.files); + } + + const changedFiles = parseDiffFiles( + patchText, + snapshot.files.map((file) => file.filename), + ); + const compareTarget: ReviewResolvedCompareTarget = { + kind: "default_branch", + label: snapshot.baseBranch, + ref: snapshot.baseSha ?? snapshot.baseBranch, + laneId: null, + branchRef: snapshot.baseBranch, + }; + + return { + targetLabel: `PR #${snapshot.githubPrNumber} ${snapshot.headBranch} -> ${snapshot.baseBranch}`, + compareTarget, + publicationTarget: buildPrPublicationTarget(snapshot), + fullPatchText: patchText, + changedFiles, + artifacts: [ + buildDiffArtifact(patchText, { + targetMode: "pr", + prId: snapshot.id, + githubPrNumber: snapshot.githubPrNumber, + repoOwner: snapshot.repoOwner, + repoName: snapshot.repoName, + baseSha: snapshot.baseSha, + headSha: snapshot.headSha, + }), + ], + }; + } + + return { + async materialize(args: { target: ReviewTarget; config: ReviewRunConfig }): Promise { + if (args.target.mode === "lane_diff") { + return materializeLaneDiff({ + target: args.target, + config: args.config, + }); + } + if (args.target.mode === "commit_range") { + return materializeCommitRange({ target: args.target }); + } + if (args.target.mode === "pr") { + return materializePullRequest({ target: args.target }); + } + return materializeWorkingTree({ target: args.target }); + }, + }; +} diff --git a/apps/desktop/src/main/services/review/reviewToolEvidence.test.ts b/apps/desktop/src/main/services/review/reviewToolEvidence.test.ts new file mode 100644 index 000000000..b533e9156 --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewToolEvidence.test.ts @@ -0,0 +1,146 @@ +import { describe, expect, it } from "vitest"; +import type { ReviewContextValidationPayload } from "./reviewContextBuilder"; +import { buildToolBackedEvidence } from "./reviewToolEvidence"; + +function emptyPayload( + overrides: Partial = {}, +): ReviewContextValidationPayload { + return { + linkedPr: null, + reviewSnapshot: null, + checks: [], + suites: [], + testRuns: [], + issueInventory: [], + sessionFailures: [], + signals: [], + ...overrides, + }; +} + +describe("buildToolBackedEvidence", () => { + it("returns no evidence when validation payload is null", () => { + const out = buildToolBackedEvidence({ + finding: { filePath: "src/api.ts", title: "x", body: "", line: 1 }, + validation: null, + }); + expect(out).toEqual([]); + }); + + it("maps a validation signal when the file path overlaps", () => { + const out = buildToolBackedEvidence({ + finding: { filePath: "src/api.ts", title: "Handler drops errors", body: "...", line: 20 }, + validation: emptyPayload({ + signals: [ + { + kind: "test_run_failure", + summary: "api integration test failing in src/api.ts", + filePaths: ["src/api.ts"], + sourceId: "suite:integration", + }, + ], + }), + }); + expect(out.length).toBe(1); + expect(out[0]?.kind).toBe("tool_signal"); + expect(out[0]?.toolSignal?.kind).toBe("test"); + expect(out[0]?.toolSignal?.status).toBe("fail"); + }); + + it("caps evidence at three entries", () => { + const signals = Array.from({ length: 6 }, (_, i) => ({ + kind: "pr_check_failure" as const, + summary: `check ${i} error in src/api.ts`, + filePaths: ["src/api.ts"], + sourceId: `check-${i}`, + })); + const out = buildToolBackedEvidence({ + finding: { filePath: "src/api.ts", title: "Issue", body: "", line: 1 }, + validation: emptyPayload({ signals }), + }); + expect(out.length).toBeLessThanOrEqual(3); + }); + + it("includes a failing CI check that mentions the title keywords", () => { + const out = buildToolBackedEvidence({ + finding: { filePath: null, title: "Typecheck regression in shared types", body: "", line: null }, + validation: emptyPayload({ + checks: [ + { + name: "typecheck", + status: "completed", + conclusion: "failure", + detailsUrl: "https://ci/run/1", + startedAt: null, + completedAt: null, + }, + ], + }), + }); + expect(out.length).toBe(1); + expect(out[0]?.toolSignal?.kind).toBe("typecheck"); + }); + + it("does not attach a test-run signal whose only token overlap is a substring (test vs latest)", () => { + // Finding body mentions "latest" and "tested" but never the word "test" + // on its own. A substring match would have (incorrectly) attached this. + const out = buildToolBackedEvidence({ + finding: { + filePath: null, + title: "Use the latest client", + body: "We have tested this carefully.", + line: null, + }, + validation: emptyPayload({ + signals: [ + { + kind: "test_run_failure", + summary: "test suite failing", + filePaths: [], + sourceId: "suite:unit", + }, + ], + }), + }); + expect(out).toEqual([]); + }); + + it("single-word summary (e.g. 'typecheck') needs only one token hit to qualify", () => { + const out = buildToolBackedEvidence({ + finding: { filePath: null, title: "typecheck fails in shared types", body: "", line: null }, + validation: emptyPayload({ + signals: [ + { + kind: "pr_check_failure", + summary: "typecheck", + filePaths: [], + sourceId: "check:typecheck", + }, + ], + }), + }); + expect(out.length).toBe(1); + expect(out[0]?.toolSignal?.kind).toBe("ci_check"); + }); + + it("does not double-count duplicate tokens in the summary", () => { + // Summary repeats "handler" three times; title mentions "handler" once. + // Under the pre-dedup logic, three hits would satisfy the "required: 2" + // threshold even against a single real occurrence. With dedup, tokens + // are only counted once — so we still need a second distinct token match. + const out = buildToolBackedEvidence({ + finding: { filePath: null, title: "Handler drops errors", body: "", line: null }, + validation: emptyPayload({ + signals: [ + { + kind: "test_run_failure", + summary: "handler handler handler", + filePaths: [], + sourceId: "suite:x", + }, + ], + }), + }); + expect(out).toEqual([]); + }); +}); diff --git a/apps/desktop/src/main/services/review/reviewToolEvidence.ts b/apps/desktop/src/main/services/review/reviewToolEvidence.ts new file mode 100644 index 000000000..9a1b1a8f0 --- /dev/null +++ b/apps/desktop/src/main/services/review/reviewToolEvidence.ts @@ -0,0 +1,156 @@ +import type { + ReviewEvidence, + ReviewFinding, + ReviewToolSignalKind, +} from "../../../shared/types"; +import type { ReviewContextValidationPayload } from "./reviewContextBuilder"; + +type ValidationSignalSource = ReviewContextValidationPayload["signals"][number]; + +const STATUS_BY_KIND: Record = { + typecheck: "fail", + test: "fail", + lint: "fail", + build: "fail", + ci_check: "fail", + validation: "warn", +}; + +function classifyCheck(name: string): ReviewToolSignalKind { + const lower = name.toLowerCase(); + if (/(typecheck|tsc|types|type-check)/.test(lower)) return "typecheck"; + if (/(test|vitest|jest|spec)/.test(lower)) return "test"; + if (/(lint|eslint|stylelint|prettier|tsfmt)/.test(lower)) return "lint"; + if (/(build|bundle|compile|webpack|vite|rollup|tsup)/.test(lower)) return "build"; + return "ci_check"; +} + +function signalKindFromPayloadSignal(signal: ValidationSignalSource): ReviewToolSignalKind { + switch (signal.kind) { + case "pr_check_failure": + return "ci_check"; + case "test_run_failure": + return "test"; + case "review_feedback": + return "validation"; + case "session_failure": + return "validation"; + default: + return "validation"; + } +} + +function pathMatchesFinding(paths: string[], findingPath: string | null): boolean { + if (!findingPath || paths.length === 0) return false; + const normalized = findingPath.replace(/^\.+\//, ""); + return paths.some((candidate) => { + const normalizedCandidate = candidate.replace(/^\.+\//, ""); + return normalizedCandidate === normalized + || normalizedCandidate.endsWith(`/${normalized}`) + || normalized.endsWith(`/${normalizedCandidate}`); + }); +} + +function escapeRegex(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +function titleMatchesSignal(findingTitle: string, findingBody: string, summary: string): boolean { + const haystack = `${findingTitle} ${findingBody}`.toLowerCase(); + const tokens = summary + .toLowerCase() + .split(/[^a-z0-9]+/) + .filter((token) => token.length >= 4); + if (tokens.length === 0) return false; + const required = Math.min(2, tokens.length); + let hits = 0; + const seen = new Set(); + for (const token of tokens) { + if (seen.has(token)) continue; + seen.add(token); + // Word-boundary match so "test" doesn't hit "latest" or "tested". + if (new RegExp(`\\b${escapeRegex(token)}\\b`).test(haystack)) hits += 1; + if (hits >= required) return true; + } + return false; +} + +export function buildToolBackedEvidence(args: { + finding: Pick; + validation: ReviewContextValidationPayload | null; + artifactIdByKey?: Partial>; +}): ReviewEvidence[] { + if (!args.validation) return []; + const out: ReviewEvidence[] = []; + const artifactId = args.artifactIdByKey?.validation_signals ?? null; + + for (const signal of args.validation.signals) { + const matches = pathMatchesFinding(signal.filePaths, args.finding.filePath); + const titleHit = !matches && titleMatchesSignal(args.finding.title, args.finding.body, signal.summary); + if (!matches && !titleHit) continue; + const kind: ReviewToolSignalKind = signalKindFromPayloadSignal(signal); + out.push({ + kind: "tool_signal", + summary: signal.summary, + filePath: signal.filePaths[0] ?? null, + line: null, + quote: null, + artifactId, + toolSignal: { + kind, + source: signal.sourceId, + status: STATUS_BY_KIND[kind], + detail: signal.summary, + }, + }); + if (out.length >= 3) return out; + } + + for (const check of args.validation.checks) { + if (check.conclusion && check.conclusion !== "failure" && check.conclusion !== "action_required") continue; + const kind = classifyCheck(check.name); + const detail = `${check.name} (${check.status}${check.conclusion ? ` / ${check.conclusion}` : ""})`; + const matchesTitle = titleMatchesSignal(args.finding.title, args.finding.body, check.name); + if (!matchesTitle) continue; + out.push({ + kind: "tool_signal", + summary: `CI check ${detail}`, + filePath: null, + line: null, + quote: null, + artifactId, + toolSignal: { + kind, + source: check.detailsUrl ?? check.name, + status: check.conclusion === "failure" ? "fail" : "warn", + detail, + }, + }); + if (out.length >= 3) return out; + } + + for (const testRun of args.validation.testRuns) { + if (testRun.status !== "failed" && testRun.status !== "error") continue; + const hitLog = testRun.logExcerpt + ? titleMatchesSignal(args.finding.title, args.finding.body, testRun.logExcerpt) + : false; + if (!hitLog) continue; + out.push({ + kind: "tool_signal", + summary: `Test run ${testRun.suiteName} — ${testRun.status}${testRun.exitCode != null ? ` (exit ${testRun.exitCode})` : ""}`, + filePath: null, + line: null, + quote: testRun.logExcerpt, + artifactId, + toolSignal: { + kind: "test", + source: `test-run:${testRun.runId}`, + status: "fail", + detail: testRun.logExcerpt ?? null, + }, + }); + if (out.length >= 3) return out; + } + + return out; +} diff --git a/apps/desktop/src/main/services/state/kvDb.ts b/apps/desktop/src/main/services/state/kvDb.ts index 6e254f378..61c0a4349 100644 --- a/apps/desktop/src/main/services/state/kvDb.ts +++ b/apps/desktop/src/main/services/state/kvDb.ts @@ -3108,6 +3108,140 @@ function migrate(db: MigrationDb) { db.run("create index if not exists idx_budget_usage_records_week on budget_usage_records(week_key)"); db.run("create index if not exists idx_budget_usage_records_provider_week on budget_usage_records(provider, week_key)"); + // Local review history for Review tab runs. + db.run(` + create table if not exists review_runs ( + id text primary key, + project_id text not null, + lane_id text not null, + target_json text not null, + config_json text not null, + target_label text not null, + compare_target_json text, + status text not null, + summary text, + error_message text, + finding_count integer not null default 0, + severity_summary_json text, + chat_session_id text, + created_at text not null, + started_at text not null, + ended_at text, + updated_at text not null, + foreign key(project_id) references projects(id), + foreign key(lane_id) references lanes(id) + ) + `); + db.run("create index if not exists idx_review_runs_project_created on review_runs(project_id, created_at desc)"); + db.run("create index if not exists idx_review_runs_lane_created on review_runs(lane_id, created_at desc)"); + db.run("create index if not exists idx_review_runs_project_status on review_runs(project_id, status)"); + + db.run(` + create table if not exists review_findings ( + id text primary key, + run_id text not null, + title text not null, + severity text not null, + finding_class text, + body text not null, + confidence real not null default 0.5, + evidence_json text, + file_path text, + line integer, + anchor_state text not null, + source_pass text not null, + publication_state text not null, + originating_passes_json text, + adjudication_json text, + foreign key(run_id) references review_runs(id) on delete cascade + ) + `); + db.run("create index if not exists idx_review_findings_run on review_findings(run_id)"); + db.run("create index if not exists idx_review_findings_run_file on review_findings(run_id, file_path, line)"); + + db.run(` + create table if not exists review_run_publications ( + id text primary key, + run_id text not null, + destination_json text not null, + review_event text not null, + status text not null, + review_url text, + remote_review_id text, + summary_body text not null, + inline_comments_json text not null default '[]', + summary_finding_ids_json text not null default '[]', + error_message text, + created_at text not null, + updated_at text not null, + completed_at text, + foreign key(run_id) references review_runs(id) on delete cascade + ) + `); + db.run("create index if not exists idx_review_run_publications_run on review_run_publications(run_id, created_at)"); + + db.run(` + create table if not exists review_run_artifacts ( + id text primary key, + run_id text not null, + artifact_type text not null, + title text not null, + mime_type text not null, + content_text text, + metadata_json text, + created_at text not null, + foreign key(run_id) references review_runs(id) on delete cascade + ) + `); + db.run("create index if not exists idx_review_run_artifacts_run on review_run_artifacts(run_id, created_at)"); + try { db.run("alter table review_findings add column finding_class text"); } catch {} + try { db.run("alter table review_findings add column originating_passes_json text"); } catch {} + try { db.run("alter table review_findings add column adjudication_json text"); } catch {} + try { db.run("alter table review_findings add column diff_context_json text"); } catch {} + try { db.run("alter table review_findings add column suppression_match_json text"); } catch {} + + // Per-finding feedback — powers the learning loop. + db.run(` + create table if not exists review_finding_feedback ( + id text primary key, + finding_id text not null, + run_id text not null, + project_id text not null, + kind text not null, + reason text, + note text, + snooze_until text, + created_at text not null, + foreign key(finding_id) references review_findings(id) on delete cascade + ) + `); + db.run("create index if not exists idx_review_feedback_finding on review_finding_feedback(finding_id)"); + db.run("create index if not exists idx_review_feedback_project_created on review_finding_feedback(project_id, created_at desc)"); + + // Durable suppressions — Greptile-style learned filter. + db.run(` + create table if not exists review_suppressions ( + id text primary key, + project_id text not null, + scope text not null, + repo_key text, + path_pattern text, + title text not null, + title_norm text not null, + finding_class text, + severity text, + reason text, + note text, + embedding_json text, + source_finding_id text, + hit_count integer not null default 0, + created_at text not null, + last_matched_at text + ) + `); + db.run("create index if not exists idx_review_suppressions_project on review_suppressions(project_id, created_at desc)"); + db.run("create index if not exists idx_review_suppressions_repo on review_suppressions(project_id, repo_key)"); + // PR convergence loop: issue inventory tracking db.run(` create table if not exists pr_issue_inventory ( diff --git a/apps/desktop/src/preload/global.d.ts b/apps/desktop/src/preload/global.d.ts index 5a880a661..e4788d169 100644 --- a/apps/desktop/src/preload/global.d.ts +++ b/apps/desktop/src/preload/global.d.ts @@ -114,6 +114,12 @@ import type { AutomationSaveDraftResult, AutomationSimulateRequest, AutomationSimulateResult, + ReviewEventPayload, + ReviewLaunchContext, + ReviewListRunsArgs, + ReviewRun, + ReviewRunDetail, + ReviewStartRunArgs, AdeActionRegistryEntry, UsageSnapshot, BudgetCheckResult, @@ -772,6 +778,23 @@ declare global { ) => Promise; onEvent: (cb: (ev: AutomationsEventPayload) => void) => () => void; }; + review: { + listLaunchContext: () => Promise; + listRuns: (args?: ReviewListRunsArgs) => Promise; + getRunDetail: (runId: string) => Promise; + startRun: (args: ReviewStartRunArgs) => Promise; + rerun: (runId: string) => Promise; + cancelRun: (runId: string) => Promise; + recordFeedback: ( + args: import("../shared/types").ReviewRecordFeedbackArgs, + ) => Promise; + listSuppressions: ( + args?: import("../shared/types").ReviewListSuppressionsArgs, + ) => Promise; + deleteSuppression: (suppressionId: string) => Promise; + qualityReport: () => Promise; + onEvent: (cb: (ev: ReviewEventPayload) => void) => () => void; + }; actions: { listRegistry: () => Promise; }; @@ -1461,7 +1484,7 @@ declare global { updateBody: (args: UpdatePrBodyArgs) => Promise; setLabels: (args: SetPrLabelsArgs) => Promise; requestReviewers: (args: RequestPrReviewersArgs) => Promise; - submitReview: (args: SubmitPrReviewArgs) => Promise; + submitReview: (args: SubmitPrReviewArgs) => Promise; close: (args: ClosePrArgs) => Promise; reopen: (args: ReopenPrArgs) => Promise; rerunChecks: (args: RerunPrChecksArgs) => Promise; diff --git a/apps/desktop/src/preload/preload.test.ts b/apps/desktop/src/preload/preload.test.ts index 0c3a87db2..a33756404 100644 --- a/apps/desktop/src/preload/preload.test.ts +++ b/apps/desktop/src/preload/preload.test.ts @@ -76,4 +76,52 @@ describe("preload OAuth bridge", () => { unsubscribe(); expect(removeListener).toHaveBeenCalledWith(IPC.lanesOAuthEvent, listener); }); + + it("exposes review IPC methods and cleans up listeners", async () => { + const invoke = vi.fn(async () => undefined); + const on = vi.fn(); + const removeListener = vi.fn(); + const exposeInMainWorld = vi.fn((name: string, value: unknown) => { + (globalThis as any).__bridgeName = name; + (globalThis as any).__adeBridge = value; + }); + + vi.doMock("electron", () => ({ + contextBridge: { exposeInMainWorld }, + ipcRenderer: { invoke, on, removeListener }, + webFrame: { + getZoomLevel: vi.fn(() => 0), + setZoomLevel: vi.fn(), + getZoomFactor: vi.fn(() => 1), + }, + })); + + await import("./preload"); + + const bridge = (globalThis as any).__adeBridge; + expect(bridge.review).toBeTruthy(); + await bridge.review.listLaunchContext(); + await bridge.review.listRuns({ laneId: "lane-1", limit: 5 }); + await bridge.review.getRunDetail("run-1"); + await bridge.review.startRun({ target: { mode: "lane_diff", laneId: "lane-1" } }); + await bridge.review.rerun("run-1"); + + expect(invoke).toHaveBeenCalledWith(IPC.reviewListLaunchContext); + expect(invoke).toHaveBeenCalledWith(IPC.reviewListRuns, { laneId: "lane-1", limit: 5 }); + expect(invoke).toHaveBeenCalledWith(IPC.reviewGetRunDetail, { runId: "run-1" }); + expect(invoke).toHaveBeenCalledWith(IPC.reviewStartRun, { target: { mode: "lane_diff", laneId: "lane-1" } }); + expect(invoke).toHaveBeenCalledWith(IPC.reviewRerun, { runId: "run-1" }); + + const callback = vi.fn(); + const unsubscribe = bridge.review.onEvent(callback); + expect(on).toHaveBeenCalledWith(IPC.reviewEvent, expect.any(Function)); + + const listener = on.mock.calls.at(-1)?.[1]; + expect(typeof listener).toBe("function"); + listener({}, { type: "runs-updated", runId: "run-1", status: "completed" }); + expect(callback).toHaveBeenCalledWith({ type: "runs-updated", runId: "run-1", status: "completed" }); + + unsubscribe(); + expect(removeListener).toHaveBeenCalledWith(IPC.reviewEvent, listener); + }); }); diff --git a/apps/desktop/src/preload/preload.ts b/apps/desktop/src/preload/preload.ts index 6e6e0835b..288508bd5 100644 --- a/apps/desktop/src/preload/preload.ts +++ b/apps/desktop/src/preload/preload.ts @@ -35,6 +35,12 @@ import type { AutomationSaveDraftResult, AutomationSimulateRequest, AutomationSimulateResult, + ReviewEventPayload, + ReviewLaunchContext, + ReviewListRunsArgs, + ReviewRun, + ReviewRunDetail, + ReviewStartRunArgs, AdeActionRegistryEntry, AdeCliInstallResult, AdeCliStatus, @@ -216,6 +222,7 @@ import type { SetPrLabelsArgs, RequestPrReviewersArgs, SubmitPrReviewArgs, + SubmitPrReviewResult, ClosePrArgs, ReopenPrArgs, RerunPrChecksArgs, @@ -898,6 +905,37 @@ contextBridge.exposeInMainWorld("ade", { return () => ipcRenderer.removeListener(IPC.automationsEvent, listener); }, }, + review: { + listLaunchContext: async (): Promise => + ipcRenderer.invoke(IPC.reviewListLaunchContext), + listRuns: async (args: ReviewListRunsArgs = {}): Promise => + ipcRenderer.invoke(IPC.reviewListRuns, args), + getRunDetail: async (runId: string): Promise => + ipcRenderer.invoke(IPC.reviewGetRunDetail, { runId }), + startRun: async (args: ReviewStartRunArgs): Promise => + ipcRenderer.invoke(IPC.reviewStartRun, args), + rerun: async (runId: string): Promise => + ipcRenderer.invoke(IPC.reviewRerun, { runId }), + cancelRun: async (runId: string): Promise => + ipcRenderer.invoke(IPC.reviewCancelRun, { runId }), + recordFeedback: async ( + args: import("../shared/types").ReviewRecordFeedbackArgs, + ): Promise => + ipcRenderer.invoke(IPC.reviewRecordFeedback, args), + listSuppressions: async ( + args: import("../shared/types").ReviewListSuppressionsArgs = {}, + ): Promise => + ipcRenderer.invoke(IPC.reviewListSuppressions, args), + deleteSuppression: async (suppressionId: string): Promise => + ipcRenderer.invoke(IPC.reviewDeleteSuppression, { suppressionId }), + qualityReport: async (): Promise => + ipcRenderer.invoke(IPC.reviewQualityReport), + onEvent: (cb: (ev: ReviewEventPayload) => void) => { + const listener = (_event: Electron.IpcRendererEvent, payload: ReviewEventPayload) => cb(payload); + ipcRenderer.on(IPC.reviewEvent, listener); + return () => ipcRenderer.removeListener(IPC.reviewEvent, listener); + }, + }, actions: { listRegistry: async (): Promise => ipcRenderer.invoke(IPC.adeActionsListRegistry), @@ -2148,29 +2186,17 @@ contextBridge.exposeInMainWorld("ade", { args: ReplyToPrReviewThreadArgs, ): Promise => ipcRenderer.invoke(IPC.prsReplyToReviewThread, args), - resolveReviewThread: async ( - args: ResolvePrReviewThreadArgs, - ): Promise => ipcRenderer.invoke(IPC.prsResolveReviewThread, args), - updateTitle: async (args: UpdatePrTitleArgs): Promise => - ipcRenderer.invoke(IPC.prsUpdateTitle, args), - updateBody: async (args: UpdatePrBodyArgs): Promise => - ipcRenderer.invoke(IPC.prsUpdateBody, args), - setLabels: async (args: SetPrLabelsArgs): Promise => - ipcRenderer.invoke(IPC.prsSetLabels, args), - requestReviewers: async (args: RequestPrReviewersArgs): Promise => - ipcRenderer.invoke(IPC.prsRequestReviewers, args), - submitReview: async (args: SubmitPrReviewArgs): Promise => - ipcRenderer.invoke(IPC.prsSubmitReview, args), - close: async (args: ClosePrArgs): Promise => - ipcRenderer.invoke(IPC.prsClose, args), - reopen: async (args: ReopenPrArgs): Promise => - ipcRenderer.invoke(IPC.prsReopen, args), - rerunChecks: async (args: RerunPrChecksArgs): Promise => - ipcRenderer.invoke(IPC.prsRerunChecks, args), - aiReviewSummary: async ( - args: AiReviewSummaryArgs, - ): Promise => - ipcRenderer.invoke(IPC.prsAiReviewSummary, args), + resolveReviewThread: async (args: ResolvePrReviewThreadArgs): Promise => + ipcRenderer.invoke(IPC.prsResolveReviewThread, args), + updateTitle: async (args: UpdatePrTitleArgs): Promise => ipcRenderer.invoke(IPC.prsUpdateTitle, args), + updateBody: async (args: UpdatePrBodyArgs): Promise => ipcRenderer.invoke(IPC.prsUpdateBody, args), + setLabels: async (args: SetPrLabelsArgs): Promise => ipcRenderer.invoke(IPC.prsSetLabels, args), + requestReviewers: async (args: RequestPrReviewersArgs): Promise => ipcRenderer.invoke(IPC.prsRequestReviewers, args), + submitReview: async (args: SubmitPrReviewArgs): Promise => ipcRenderer.invoke(IPC.prsSubmitReview, args), + close: async (args: ClosePrArgs): Promise => ipcRenderer.invoke(IPC.prsClose, args), + reopen: async (args: ReopenPrArgs): Promise => ipcRenderer.invoke(IPC.prsReopen, args), + rerunChecks: async (args: RerunPrChecksArgs): Promise => ipcRenderer.invoke(IPC.prsRerunChecks, args), + aiReviewSummary: async (args: AiReviewSummaryArgs): Promise => ipcRenderer.invoke(IPC.prsAiReviewSummary, args), issueInventorySync: async (prId: string): Promise => ipcRenderer.invoke(IPC.prsIssueInventorySync, { prId }), issueInventoryGet: async (prId: string): Promise => diff --git a/apps/desktop/src/renderer/browserMock.ts b/apps/desktop/src/renderer/browserMock.ts index d1b1408bf..797d97514 100644 --- a/apps/desktop/src/renderer/browserMock.ts +++ b/apps/desktop/src/renderer/browserMock.ts @@ -2029,6 +2029,249 @@ if (typeof window !== "undefined" && !(window as any).ade) { }), onEvent: noop, }, + review: { + listLaunchContext: resolved({ + defaultLaneId: MOCK_LANES[1]?.id ?? MOCK_LANES[0]?.id ?? null, + defaultBranchName: "main", + lanes: MOCK_LANES.map((lane) => ({ + id: lane.id, + name: lane.name, + laneType: lane.laneType, + branchRef: lane.branchRef, + baseRef: lane.baseRef, + color: lane.color ?? null, + })), + recentCommitsByLane: Object.fromEntries( + MOCK_LANES.map((lane) => [lane.id, [ + { + sha: "abc1234567890", + shortSha: "abc1234", + subject: `Recent work on ${lane.name}`, + authoredAt: now, + pushed: false, + }, + { + sha: "def4567890123", + shortSha: "def4567", + subject: `Follow-up fix on ${lane.name}`, + authoredAt: yesterday, + pushed: true, + }, + ]]), + ), + recommendedModelId: DEFAULT_BROWSER_MOCK_CODEX_MODEL, + }), + listRuns: resolvedArg([ + { + id: "review-run-1", + projectId: MOCK_PROJECT.id, + laneId: MOCK_LANES[1]?.id ?? "lane-auth", + target: { mode: "lane_diff", laneId: MOCK_LANES[1]?.id ?? "lane-auth" }, + config: { + compareAgainst: { kind: "default_branch" }, + selectionMode: "full_diff", + dirtyOnly: false, + modelId: DEFAULT_BROWSER_MOCK_CODEX_MODEL, + reasoningEffort: "medium", + budgets: { maxFiles: 60, maxDiffChars: 180000, maxPromptChars: 220000, maxFindings: 12 }, + publishBehavior: "local_only", + }, + targetLabel: "feature/auth-flow vs main", + compareTarget: { kind: "default_branch", label: "main", ref: "main", laneId: null, branchRef: "main" }, + status: "completed", + summary: "Found two actionable risks in the auth flow changes.", + errorMessage: null, + findingCount: 2, + severitySummary: { critical: 0, high: 1, medium: 1, low: 0, info: 0 }, + chatSessionId: "chat-review-1", + createdAt: yesterday, + startedAt: yesterday, + endedAt: now, + updatedAt: now, + }, + ]), + getRunDetail: resolvedArg({ + id: "review-run-1", + projectId: MOCK_PROJECT.id, + laneId: MOCK_LANES[1]?.id ?? "lane-auth", + target: { mode: "lane_diff", laneId: MOCK_LANES[1]?.id ?? "lane-auth" }, + config: { + compareAgainst: { kind: "default_branch" }, + selectionMode: "full_diff", + dirtyOnly: false, + modelId: DEFAULT_BROWSER_MOCK_CODEX_MODEL, + reasoningEffort: "medium", + budgets: { maxFiles: 60, maxDiffChars: 180000, maxPromptChars: 220000, maxFindings: 12 }, + publishBehavior: "local_only", + }, + targetLabel: "feature/auth-flow vs main", + compareTarget: { kind: "default_branch", label: "main", ref: "main", laneId: null, branchRef: "main" }, + status: "completed", + summary: "Found two actionable risks in the auth flow changes.", + errorMessage: null, + findingCount: 2, + severitySummary: { critical: 0, high: 1, medium: 1, low: 0, info: 0 }, + chatSessionId: "chat-review-1", + createdAt: yesterday, + startedAt: yesterday, + endedAt: now, + updatedAt: now, + findings: [ + { + id: "finding-1", + runId: "review-run-1", + title: "Missing rollback when PKCE token exchange fails", + severity: "high", + body: "The new auth path persists session state before the token exchange completes, which can leave the lane in a partially authenticated state after a failed callback.", + confidence: 0.83, + evidence: [ + { + kind: "diff_hunk", + summary: "Session write happens before token exchange success is confirmed.", + filePath: "src/auth/oauth.ts", + line: 128, + quote: "saveSession(session);", + artifactId: null, + }, + ], + filePath: "src/auth/oauth.ts", + line: 128, + anchorState: "anchored", + sourcePass: "single_pass", + publicationState: "local_only", + }, + { + id: "finding-2", + runId: "review-run-1", + title: "Callback route still lacks regression coverage", + severity: "medium", + body: "The diff updates the callback branching logic but does not add coverage for the rejected-code path, so the new behavior can regress without detection.", + confidence: 0.68, + evidence: [], + filePath: "src/auth/oauth.test.ts", + line: null, + anchorState: "file_only", + sourcePass: "single_pass", + publicationState: "local_only", + }, + ], + artifacts: [ + { + id: "artifact-review-diff-1", + runId: "review-run-1", + artifactType: "diff_bundle", + title: "Diff bundle", + mimeType: "text/plain", + contentText: "diff --git a/src/auth/oauth.ts b/src/auth/oauth.ts\n@@ ...", + metadata: null, + createdAt: now, + }, + ], + publications: [], + chatSession: { + sessionId: "chat-review-1", + laneId: MOCK_LANES[1]?.id ?? "lane-auth", + provider: "codex", + model: "GPT-5.4 Codex", + modelId: DEFAULT_BROWSER_MOCK_CODEX_MODEL, + title: "Review: feature/auth-flow vs main", + surface: "automation", + automationId: null, + automationRunId: null, + status: "idle", + startedAt: yesterday, + endedAt: now, + lastActivityAt: now, + lastOutputPreview: "Found two actionable risks in the auth flow changes.", + summary: "Saved review transcript for local diff review.", + }, + }), + startRun: resolvedArg({ + id: "review-run-queued", + projectId: MOCK_PROJECT.id, + laneId: MOCK_LANES[1]?.id ?? "lane-auth", + target: { mode: "lane_diff", laneId: MOCK_LANES[1]?.id ?? "lane-auth" }, + config: { + compareAgainst: { kind: "default_branch" }, + selectionMode: "full_diff", + dirtyOnly: false, + modelId: DEFAULT_BROWSER_MOCK_CODEX_MODEL, + reasoningEffort: "medium", + budgets: { maxFiles: 60, maxDiffChars: 180000, maxPromptChars: 220000, maxFindings: 12 }, + publishBehavior: "local_only", + }, + targetLabel: "feature/auth-flow review", + compareTarget: null, + status: "queued", + summary: null, + errorMessage: null, + findingCount: 0, + severitySummary: { critical: 0, high: 0, medium: 0, low: 0, info: 0 }, + chatSessionId: null, + createdAt: now, + startedAt: now, + endedAt: null, + updatedAt: now, + }), + rerun: resolvedArg({ + id: "review-run-rerun", + projectId: MOCK_PROJECT.id, + laneId: MOCK_LANES[1]?.id ?? "lane-auth", + target: { mode: "lane_diff", laneId: MOCK_LANES[1]?.id ?? "lane-auth" }, + config: { + compareAgainst: { kind: "default_branch" }, + selectionMode: "full_diff", + dirtyOnly: false, + modelId: DEFAULT_BROWSER_MOCK_CODEX_MODEL, + reasoningEffort: "medium", + budgets: { maxFiles: 60, maxDiffChars: 180000, maxPromptChars: 220000, maxFindings: 12 }, + publishBehavior: "local_only", + }, + targetLabel: "feature/auth-flow review", + compareTarget: null, + status: "queued", + summary: null, + errorMessage: null, + findingCount: 0, + severitySummary: { critical: 0, high: 0, medium: 0, low: 0, info: 0 }, + chatSessionId: null, + createdAt: now, + startedAt: now, + endedAt: null, + updatedAt: now, + }), + cancelRun: resolvedArg(null), + recordFeedback: resolvedArg({ + id: "rfb_mock", + findingId: "mock-finding", + runId: "review-run-1", + kind: "acknowledge" as const, + reason: null, + note: null, + snoozeUntil: null, + createdAt: now, + }), + listSuppressions: resolvedArg([]), + deleteSuppression: resolvedArg(true), + qualityReport: resolved({ + projectId: MOCK_PROJECT.id, + totalRuns: 3, + totalFindings: 14, + addressedCount: 6, + dismissedCount: 3, + snoozedCount: 1, + suppressedCount: 2, + publishedCount: 5, + noiseRate: 0.35, + recentFeedback: [], + byClass: [ + { findingClass: "intent_drift" as const, total: 4, addressed: 2 }, + { findingClass: "incomplete_rollout" as const, total: 5, addressed: 3 }, + { findingClass: "late_stage_regression" as const, total: 2, addressed: 1 }, + ], + }), + onEvent: noop, + }, actions: { listRegistry: resolved([]), }, @@ -3025,7 +3268,13 @@ if (typeof window !== "undefined" && !(window as any).ade) { updateBody: resolvedArg(undefined), setLabels: resolvedArg(undefined), requestReviewers: resolvedArg(undefined), - submitReview: resolvedArg(undefined), + submitReview: resolvedArg({ + id: "pr-review-1", + nodeId: "PRR_mock_1", + htmlUrl: "https://github.com/mock/repo/pull/1#pullrequestreview-1", + state: "COMMENTED", + submittedAt: now, + }), close: resolvedArg(undefined), reopen: resolvedArg(undefined), rerunChecks: resolvedArg(undefined), diff --git a/apps/desktop/src/renderer/components/app/App.tsx b/apps/desktop/src/renderer/components/app/App.tsx index c657413bd..f518d2d52 100644 --- a/apps/desktop/src/renderer/components/app/App.tsx +++ b/apps/desktop/src/renderer/components/app/App.tsx @@ -32,6 +32,9 @@ const TerminalsPage = React.lazy(() => const PRsPage = React.lazy(() => import("../prs/PRsPage").then((m) => ({ default: m.PRsPage })) ); +const ReviewPage = React.lazy(() => + import("../review/ReviewPage").then((m) => ({ default: m.ReviewPage })) +); const HistoryPage = React.lazy(() => import("../history/HistoryPage").then((m) => ({ default: m.HistoryPage })) ); @@ -220,6 +223,7 @@ export function App() { )} /> )} /> )} /> + )} /> )} /> )} /> )} /> diff --git a/apps/desktop/src/renderer/components/app/TabNav.test.tsx b/apps/desktop/src/renderer/components/app/TabNav.test.tsx new file mode 100644 index 000000000..3451b3962 --- /dev/null +++ b/apps/desktop/src/renderer/components/app/TabNav.test.tsx @@ -0,0 +1,61 @@ +/* @vitest-environment jsdom */ + +import React from "react"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { render, screen } from "@testing-library/react"; +import { MemoryRouter } from "react-router-dom"; +import { TabNav } from "./TabNav"; +import { useAppStore } from "../../state/appStore"; + +function resetStore() { + useAppStore.setState({ + project: { rootPath: "/Users/arul/ADE", name: "ADE" } as any, + projectHydrated: true, + showWelcome: false, + selectedLaneId: "lane-1", + runLaneId: null, + focusedSessionId: null, + lanes: [], + laneInspectorTabs: {}, + terminalAttention: { + runningCount: 0, + activeCount: 0, + needsAttentionCount: 0, + indicator: "none", + byLaneId: {}, + }, + workViewByProject: {}, + laneWorkViewByScope: {}, + }); +} + +describe("TabNav", () => { + const originalAde = globalThis.window.ade; + + beforeEach(() => { + resetStore(); + globalThis.window.ade = { + app: { + revealPath: async () => undefined, + getInfo: async () => ({ isPackaged: false }) as any, + }, + } as any; + }); + + afterEach(() => { + globalThis.window.ade = originalAde; + }); + + it("places Review directly below PRs in the sidebar", () => { + render( + + + , + ); + + const prs = screen.getByRole("link", { name: "PRs" }); + const review = screen.getByRole("link", { name: "Review" }); + expect(prs.nextElementSibling).toBe(review); + }); +}); + diff --git a/apps/desktop/src/renderer/components/app/TabNav.tsx b/apps/desktop/src/renderer/components/app/TabNav.tsx index 39f07bd91..8fc208b1f 100644 --- a/apps/desktop/src/renderer/components/app/TabNav.tsx +++ b/apps/desktop/src/renderer/components/app/TabNav.tsx @@ -7,6 +7,7 @@ import { Terminal, Graph, GitPullRequest, + MagnifyingGlass, ClockCounterClockwise, Robot, Strategy, @@ -26,6 +27,7 @@ const mainItems = [ { to: "/project", label: "Run", icon: PlayCircle }, { to: "/graph", label: "Graph", icon: Graph }, { to: "/prs", label: "PRs", icon: GitPullRequest }, + { to: "/review", label: "Review", icon: MagnifyingGlass }, { to: "/history", label: "History", icon: ClockCounterClockwise }, { to: "/automations", label: "Automations", icon: Robot }, { to: "/missions", label: "Missions", icon: Strategy }, diff --git a/apps/desktop/src/renderer/components/review/ReviewFindingCard.tsx b/apps/desktop/src/renderer/components/review/ReviewFindingCard.tsx new file mode 100644 index 000000000..f2076da41 --- /dev/null +++ b/apps/desktop/src/renderer/components/review/ReviewFindingCard.tsx @@ -0,0 +1,668 @@ +import React from "react"; +import { + ArrowSquareOut, + BellSimpleSlash, + CaretDown, + CaretRight, + CheckCircle, + FileText, + MagnifyingGlass, + Prohibit, + Shield, + ShieldCheck, + X, +} from "@phosphor-icons/react"; +import { Button } from "../ui/Button"; +import { Chip } from "../ui/Chip"; +import { cn } from "../ui/cn"; +import type { + ReviewDiffContext, + ReviewDismissReason, + ReviewEvidence, + ReviewFeedbackKind, + ReviewFeedbackRecord, + ReviewFinding, + ReviewFindingClass, + ReviewFindingSuppressionMatch, + ReviewSuppressionScope, +} from "./reviewTypes"; + +export type FindingActionRequest = { + finding: ReviewFinding; + kind: ReviewFeedbackKind; + reason?: ReviewDismissReason | null; + note?: string | null; + snoozeDurationMs?: number | null; + suppression?: { scope: ReviewSuppressionScope; pathPattern?: string | null } | null; +}; + +type ReviewFindingCardProps = { + finding: ReviewFinding; + onRequestAction: (request: FindingActionRequest) => Promise | void; + onOpenInFiles?: (finding: ReviewFinding) => void; + onOpenInEditor?: (finding: ReviewFinding) => void; + disabled?: boolean; +}; + +const FINDING_CLASS_DESCRIPTION: Record = { + intent_drift: "Implementation may diverge from the stated goal or prompt for this lane.", + incomplete_rollout: "Only part of a cross-surface change landed — check paired files.", + late_stage_regression: "A risky change appeared after a failed validation or late fix cycle.", +}; + +const PASS_LABEL: Record = { + "diff-risk": "Diff risk", + "cross-file-impact": "Cross-file", + "checks-and-tests": "Tests + CI", +}; + +function toSeverityTone(severity: string): string { + const n = severity.toLowerCase(); + if (n.includes("crit")) return "border-red-400/30 bg-red-400/[0.12] text-red-200"; + if (n.includes("high")) return "border-orange-400/30 bg-orange-400/[0.12] text-orange-200"; + if (n.includes("medium")) return "border-amber-400/30 bg-amber-400/[0.12] text-amber-200"; + if (n.includes("low")) return "border-sky-400/30 bg-sky-400/[0.12] text-sky-200"; + return "border-zinc-400/25 bg-zinc-400/[0.10] text-zinc-200"; +} + +function toFindingClassTone(value: ReviewFindingClass | null | undefined): string { + if (value === "intent_drift") return "border-fuchsia-400/30 bg-fuchsia-400/[0.12] text-fuchsia-200"; + if (value === "incomplete_rollout") return "border-cyan-400/30 bg-cyan-400/[0.12] text-cyan-200"; + if (value === "late_stage_regression") return "border-rose-400/30 bg-rose-400/[0.12] text-rose-200"; + return "border-zinc-400/25 bg-zinc-400/[0.10] text-zinc-200"; +} + +function toFindingClassLabel(value: ReviewFindingClass | null | undefined): string { + if (!value) return "general"; + return value.replaceAll("_", " "); +} + +function formatConfidence(value: number | string): string { + if (typeof value === "number") { + if (value <= 1) return `${Math.round(value * 100)}%`; + return `${Math.round(value)}%`; + } + return value; +} + +function describeSuppression(match: ReviewFindingSuppressionMatch | null | undefined): string | null { + if (!match) return null; + const pct = Math.round((match.similarity ?? 0) * 100); + const reasonLabel = match.reason ? `(${match.reason.replaceAll("_", " ")})` : ""; + return `Filtered by ${match.scope} suppression · ${pct}% match ${reasonLabel}`.trim(); +} + +function describeFeedback(record: ReviewFeedbackRecord | null | undefined): { label: string; tone: string } | null { + if (!record) return null; + switch (record.kind) { + case "acknowledge": + return { label: "Acknowledged", tone: "border-emerald-400/30 bg-emerald-400/[0.10] text-emerald-200" }; + case "dismiss": + return { + label: `Dismissed${record.reason ? ` · ${record.reason.replaceAll("_", " ")}` : ""}`, + tone: "border-zinc-500/25 bg-zinc-500/[0.12] text-zinc-200", + }; + case "snooze": { + const until = record.snoozeUntil ? new Date(record.snoozeUntil).toLocaleDateString() : "later"; + return { label: `Snoozed until ${until}`, tone: "border-amber-400/30 bg-amber-400/[0.10] text-amber-200" }; + } + case "suppress": + return { + label: `Suppressed${record.reason ? ` · ${record.reason.replaceAll("_", " ")}` : ""}`, + tone: "border-violet-400/30 bg-violet-400/[0.10] text-violet-200", + }; + default: + return null; + } +} + +function DiffContextBlock({ context }: { context: ReviewDiffContext | null | undefined }) { + if (!context || context.lines.length === 0) { + return ( +
+ No inline diff excerpt available for this finding. +
+ ); + } + return ( +
+
+ {context.filePath} + + L{context.startLine}–{context.endLine} + {context.anchoredLine ? ` · focus L${context.anchoredLine}` : ""} + +
+ 
+        {context.lines.map((line, idx) => {
+          const base = "flex gap-2 px-3";
+          const lineColor = line.kind === "add"
+            ? "bg-emerald-400/[0.08] text-emerald-100"
+            : line.kind === "del"
+              ? "bg-red-400/[0.08] text-red-100"
+              : line.kind === "meta"
+                ? "bg-white/[0.02] text-[#6E7F92] italic"
+                : line.highlighted
+                  ? "bg-amber-400/[0.12] text-amber-100"
+                  : "text-[#CBD5E1]";
+          const marker = line.kind === "add" ? "+" : line.kind === "del" ? "-" : line.kind === "meta" ? "@" : " ";
+          return (
+            
+              {line.line ?? ""}
+              {marker}
+              {line.text || " "}
+            
+          );
+        })}
+
+
+ ); +} + +function ToolSignalBlock({ evidence }: { evidence: ReviewEvidence[] }) { + const toolSignals = evidence.filter((entry) => entry.kind === "tool_signal" && entry.toolSignal); + if (toolSignals.length === 0) return null; + return ( +
+ {toolSignals.map((entry, idx) => { + const sig = entry.toolSignal!; + const tone = sig.status === "fail" + ? "border-red-400/30 bg-red-400/[0.10] text-red-200" + : sig.status === "warn" + ? "border-amber-400/30 bg-amber-400/[0.10] text-amber-200" + : "border-sky-400/25 bg-sky-400/[0.08] text-sky-200"; + return ( +
+
+ {sig.kind.replaceAll("_", " ")} + {entry.summary} +
+ {entry.quote ? ( + 
+                {entry.quote}
+
+ ) : null} + {sig.source && !entry.quote ? ( +
source · {sig.source}
+ ) : null} +
+ ); + })} +
+ ); +} + +type DismissModalProps = { + open: boolean; + initialKind: Exclude; + finding: ReviewFinding; + onClose: () => void; + onSubmit: (args: { + kind: Exclude; + reason: ReviewDismissReason; + note: string; + snoozeDurationMs: number | null; + suppressionScope?: ReviewSuppressionScope; + }) => Promise | void; +}; + +const DISMISS_REASONS: Array<{ value: ReviewDismissReason; label: string; hint: string }> = [ + { value: "not_a_bug", label: "Not a bug", hint: "The flagged behavior is intentional or already correct." }, + { value: "out_of_scope", label: "Out of scope", hint: "True but belongs in a different PR or project." }, + { value: "style_only", label: "Style only", hint: "Stylistic nit handled elsewhere (linter, formatter)." }, + { value: "duplicate", label: "Duplicate", hint: "Same issue already flagged by another finding or tool." }, + { value: "wont_fix", label: "Won't fix", hint: "Known limitation we are deliberately not addressing." }, + { value: "low_value_noise", label: "Low-value noise", hint: "Generic warning that adds cost but rarely catches bugs." }, + { value: "other", label: "Other (explain)", hint: "Free-form reason in the note field below." }, +]; + +function DismissModal({ open, initialKind, finding, onClose, onSubmit }: DismissModalProps) { + const [kind, setKind] = React.useState>(initialKind); + const [reason, setReason] = React.useState("low_value_noise"); + const [note, setNote] = React.useState(""); + const [snoozeDays, setSnoozeDays] = React.useState(7); + const [suppressionScope, setSuppressionScope] = React.useState("repo"); + const [submitting, setSubmitting] = React.useState(false); + const closeButtonRef = React.useRef(null); + + React.useEffect(() => { + if (open) { + setKind(initialKind); + setReason("low_value_noise"); + setNote(""); + setSnoozeDays(7); + setSuppressionScope("repo"); + setSubmitting(false); + } + }, [open, initialKind]); + + React.useEffect(() => { + if (!open) return; + const onKey = (event: KeyboardEvent) => { + if (event.key === "Escape") { + event.stopPropagation(); + onClose(); + } + }; + window.addEventListener("keydown", onKey); + // Move focus into the dialog so keyboard users are oriented. + queueMicrotask(() => closeButtonRef.current?.focus()); + return () => window.removeEventListener("keydown", onKey); + }, [open, onClose]); + + if (!open) return null; + + const handleSubmit = async () => { + setSubmitting(true); + try { + await onSubmit({ + kind, + reason, + note: note.trim(), + snoozeDurationMs: kind === "snooze" ? snoozeDays * 24 * 60 * 60 * 1000 : null, + suppressionScope: kind === "suppress" ? suppressionScope : undefined, + }); + } finally { + setSubmitting(false); + } + }; + + const kindOptions: Array<{ value: Exclude; label: string; icon: React.ReactNode }> = [ + { value: "dismiss", label: "Dismiss this finding", icon: }, + { value: "snooze", label: "Snooze for a while", icon: }, + { value: "suppress", label: "Suppress similar findings", icon: }, + ]; + + const submitLabel = submitting + ? "Saving…" + : kind === "suppress" + ? suppressionScope === "path" + ? "Suppress for this path" + : suppressionScope === "global" + ? "Suppress everywhere" + : "Suppress in this repo" + : kind === "snooze" + ? `Snooze ${snoozeDays} day${snoozeDays === 1 ? "" : "s"}` + : "Dismiss"; + + return ( +
+
e.stopPropagation()} + > +
+
+
Feedback
+
{finding.title}
+
+ +
+
+
+ {kindOptions.map((opt) => ( + + ))} +
+ + {kind === "snooze" ? ( +
+ +
+ setSnoozeDays(Math.max(1, Math.min(180, Number(e.target.value) || 1)))} + className="w-20 rounded-md border border-white/[0.08] bg-black/30 px-2 py-1 text-sm text-[#F5FAFF]" + /> + days +
+
+ ) : null} + + {kind === "suppress" ? ( +
+ +
+ {(["repo", "path", "global"] as const).map((scope) => ( + + ))} +
+

+ Future runs skip findings semantically similar to this one within the chosen scope. You can remove suppressions later from the Learnings panel. +

+
+ ) : null} + +
+ +
+ {DISMISS_REASONS.map((opt) => ( + + ))} +
+
+ +
+ +