diff --git a/apps/hub/src/code-review-runs.postgres.test.ts b/apps/hub/src/code-review-runs.postgres.test.ts index ddddf0c15..7b5e3a693 100644 --- a/apps/hub/src/code-review-runs.postgres.test.ts +++ b/apps/hub/src/code-review-runs.postgres.test.ts @@ -3,12 +3,15 @@ import { expect, test } from "bun:test"; import { randomBytes } from "node:crypto"; import { sql } from "drizzle-orm"; import { createDB, runMigrations, schema } from "@intx/db"; -import type { WorkflowRunEvent } from "@intx/hub-sessions"; import { diffHash, type PullRequestToReview } from "./code-review-feed.js"; import { createCodeReviewRuns } from "./code-review-runs.js"; +import { recordEvent, type PullRequestRef } from "./manager-state.js"; +import { migrateTriageTables } from "./triage-db.js"; const TEST_DB = { host: "localhost", port: 5432, user: "postgres", password: "postgres", database: "interchange" }; const TENANT = "tnt_reviews"; +const GITHUB: PullRequestRef = { source: "github", repo: "acme/widgets", number: 7 }; +const BITBUCKET: PullRequestRef = { source: "bitbucket-datacenter", repo: "acme/widgets", number: 7 }; const RECORD = { name: "acme/widgets", connected: true, codeReview: true }; const DIFF = "diff --git a/x b/x\nindex 1111111..2222222 100644\n--- a/x\n+++ b/x\n@@ -1 +1 @@\n-a\n+b\n"; @@ -17,14 +20,6 @@ const BASE_EDITED = DIFF.replace("1111111..2222222", "3333333..4444444"); // GitHub's diff rebuilt from the file list, which has no blob ids. const REBUILT = DIFF.replace("index 1111111..2222222 100644\n", ""); -function request(number: number, sha: string, hashed?: string) { - return { kind: "code-review", number, title: "Fix", description: "", diff: "diff --git a/x b/x\n", checkout: { source: "github", repo: "acme/widgets", sha }, ...(hashed !== undefined && { diffHash: diffHash(hashed) }) }; -} - -function started(number: number, sha: string, hashed?: string): WorkflowRunEvent { - return { seq: 0, type: "RunStarted", ts: "2026-10-17T00:00:00.000Z", body: { trigger: { payload: request(number, sha, hashed) } } } as unknown as WorkflowRunEvent; -} - const REVIEW = { summary: "Adds total(), which skips the first element.", comments: [{ path: "src/total.ts", start: 3, end: 3, title: "total() skips the first element", body: "The loop starts at 1.", priority: 1, quoted: "for (let i = 1; i < xs.length; i++) sum += xs[i];", unverified: true as const }], @@ -33,27 +28,18 @@ const REVIEW = { const OUTPUT = { ...REVIEW, comments: REVIEW.comments.map((comment) => ({ ...comment, agreement: { finders: 1, reports: 1 } })) }; -const LOGS: Record = { - run_old: [ - started(7, "sha1"), - { seq: 1, type: "StepCompleted", body: { stepId: "review", output: { ref: `inline:${JSON.stringify(OUTPUT)}` } } } as unknown as WorkflowRunEvent, - { seq: 2, type: "RunCompleted", body: {} } as unknown as WorkflowRunEvent, - ], - run_other: [started(8, "sha9")], - run_new: [ - started(7, "sha2"), - { seq: 1, type: "StepFailed", body: { stepId: "findDiff", attempt: 1, error: { message: "reactor error: Director exception: reader: inference failed: 529 overloaded" }, retriesExhausted: false } } as unknown as WorkflowRunEvent, - { seq: 2, type: "StepFailed", body: { stepId: "findDiff", attempt: 3, error: { message: "reactor error: Director exception: reader: no findings list in the answer" }, retriesExhausted: true } } as unknown as WorkflowRunEvent, - { seq: 3, type: "StepFailed", body: { stepId: "candidates", attempt: 1, error: { message: "missing field findDiff in project selector source" }, retriesExhausted: true } } as unknown as WorkflowRunEvent, - ], - run_hashed: [ - started(7, "sha3", DIFF), - { seq: 1, type: "StepCompleted", body: { stepId: "review", output: { ref: `inline:${JSON.stringify(OUTPUT)}` } } } as unknown as WorkflowRunEvent, - { seq: 2, type: "RunCompleted", body: {} } as unknown as WorkflowRunEvent, - ], -}; +const LATER = new Date(Date.now() + 60 * 60_000).toISOString(); +const EARLIER = new Date(Date.now() - 60_000).toISOString(); + +function reviewer(runId: string, head: string, fields: Record = {}) { + return { kind: "code-review", head, runId, deployedBy: "run_manager", deadline: LATER, ...fields }; +} -test("PostgreSQL: a pull request's latest code review comes from the newest run that names it, and stands for a head with its diff", async () => { +function eventKey(runId: string): string { + return `evt:github:acme/widgets#7:${runId}`; +} + +test("PostgreSQL: a pull request's latest code review is what its manager recorded: one still out, what a review reported, or one it gave up on", async () => { const database = `corbits_reviews_${randomBytes(6).toString("hex")}`; const admin = createDB(TEST_DB); await admin.db.execute(sql.raw(`create database ${database}`)); @@ -63,27 +49,25 @@ test("PostgreSQL: a pull request's latest code review comes from the newest run await runMigrations(config, { schema: "public" }); handle = createDB(config); const db = handle.db; + await migrateTriageTables(db, "public"); await db.insert(schema.tenant).values({ id: TENANT, name: "Reviews", slug: "reviews", domain: "reviews.test", config: { corbitsTriage: { repos: [RECORD] } } }); - await db.insert(schema.workflowDefinition).values({ id: "wfd_review", tenantId: TENANT, name: "pr-code-review-github", status: "stopped" }); - await db.insert(schema.workflowRun).values({ id: "wfr_anchor", definitionId: "wfd_review", anchorRunId: "wfr_anchor", tenantId: TENANT, status: "deployed" }); - const at = (minute: number) => new Date(`2026-10-17T00:0${minute}:00.000Z`); - await db.insert(schema.workflowRun).values([ - { id: "run_old", definitionId: "wfd_review", anchorRunId: "wfr_anchor", tenantId: TENANT, status: "completed", createdAt: at(1) }, - { id: "run_other", definitionId: "wfd_review", anchorRunId: "wfr_anchor", tenantId: TENANT, status: "running", createdAt: at(2) }, - ]); - const reads: string[] = []; - // A rebase onto base changes to other files: the diff, blob ids included, is as it was. - let head: PullRequestToReview = { title: "Fix", description: "", headSha: "sha4", draft: false, open: true, diff: DIFF }; + await db.insert(schema.workflowDefinition).values({ id: "wfd_review", tenantId: TENANT, name: "pr-code-review-reviewer-github" }); + /** A code review the manager deployed now, after every result recorded so far. */ + async function deployed(runId: string): Promise { + const createdAt = new Date(); + await db.insert(schema.workflowRun).values({ id: runId, definitionId: "wfd_review", anchorRunId: runId, tenantId: TENANT, status: "running", createdAt }); + return createdAt; + } + async function claim(reviewers: unknown[], taken: string[]) { + const since = new Date(0).toISOString(); + const state = { status: "live", claimId: "claim-1", claimedAt: since, runId: "run_manager", reviewers, history: [{ id: "pass-1", head: "sha1", events: taken, takenAt: since, takes: 1, runId: "run_manager", doneAt: since }], starts: 1 }; + await db.execute(sql`INSERT INTO corbits_triage.manager_state (tenant_id, key, kind, state) VALUES (${TENANT}, 'mgr:github:acme/widgets#7', 'claim', ${JSON.stringify(state)}::jsonb) ON CONFLICT (tenant_id, key) DO UPDATE SET state = excluded.state`); + } + let head: PullRequestToReview = { title: "Fix", description: "", headSha: "sha5", draft: false, open: true, diff: DIFF }; let hostReads = 0; let hostDown = false; const runs = createCodeReviewRuns({ db, - runReader: { - async readRunEvents(_repoId, _ref, runId) { - reads.push(runId); - return LOGS[runId] ?? []; - }, - }, async read() { hostReads++; if (hostDown) throw new Error("github GET /repos/acme/widgets/pulls/7 -> 502"); @@ -91,40 +75,50 @@ test("PostgreSQL: a pull request's latest code review comes from the newest run }, log() {}, }); - async function reviewsOf(number: number) { - return runs.latest(TENANT, { repo: "acme/widgets", number }); + async function reviewsOf(pr: PullRequestRef, asked?: string) { + return runs.latest(TENANT, { repo: pr.repo, source: pr.source, number: pr.number, ...(asked && { head: asked }) }); } - const finished = { status: "completed" as const, headSha: "sha1", startedAt: at(1).toISOString(), summary: REVIEW.summary, comments: REVIEW.comments, budgetExhausted: true }; - expect(await reviewsOf(7)).toEqual({ review: finished, completed: finished, carriedTo: null }); - expect(await reviewsOf(8)).toEqual({ review: { status: "running", headSha: "sha9", startedAt: at(2).toISOString() }, completed: null, carriedTo: null }); - expect(await reviewsOf(9)).toEqual({ review: null, completed: null, carriedTo: null }); + expect(await reviewsOf(GITHUB)).toEqual({ review: null, completed: null, carriedTo: null }); + + await recordEvent(db, TENANT, GITHUB, "run_old", { type: "reviewer-result", head: "sha1", payload: { kind: "code-review", result: OUTPUT } }); + await recordEvent(db, TENANT, GITHUB, "run_stray", { type: "reviewer-result", head: "sha9", payload: { kind: "code-review", result: OUTPUT } }); + await recordEvent(db, TENANT, GITHUB, "run_new", { type: "reviewer-result", head: "sha2", payload: { kind: "code-review", failed: "the review did not finish" } }); + await claim([reviewer("run_new", "sha2")], [eventKey("run_old")]); + const finished = { status: "completed" as const, headSha: "sha1", startedAt: expect.any(String), ...REVIEW }; + expect(await reviewsOf(GITHUB)).toEqual({ review: { status: "failed", headSha: "sha2", startedAt: expect.any(String) }, completed: finished, carriedTo: null }); + expect(await reviewsOf(BITBUCKET)).toEqual({ review: null, completed: null, carriedTo: null }); + + const late = await deployed("run_late"); + await claim([reviewer("run_late", "sha3", { deadline: EARLIER, result: { failed: "overdue" } })], [eventKey("run_old"), eventKey("run_new")]); + expect(await reviewsOf(GITHUB)).toEqual({ review: { status: "failed", headSha: "sha3", startedAt: late.toISOString() }, completed: finished, carriedTo: null }); - await db.insert(schema.workflowRun).values({ id: "run_new", definitionId: "wfd_review", anchorRunId: "wfr_anchor", tenantId: TENANT, status: "failed", createdAt: at(3) }); - expect(await reviewsOf(7)).toEqual({ review: { status: "failed", headSha: "sha2", startedAt: at(3).toISOString(), reason: "The diff reviewer's answer couldn't be read." }, completed: finished, carriedTo: null }); - expect(reads.filter((runId) => runId === "run_old")).toHaveLength(1); + const live = await deployed("run_live"); + await claim([reviewer("run_live", "sha4")], [eventKey("run_old"), eventKey("run_new")]); + expect(await reviewsOf(GITHUB)).toEqual({ review: { status: "running", headSha: "sha4", startedAt: live.toISOString() }, completed: finished, carriedTo: null }); - await db.insert(schema.workflowRun).values({ id: "run_hashed", definitionId: "wfd_review", anchorRunId: "wfr_anchor", tenantId: TENANT, status: "completed", createdAt: at(4) }); - const hashed = { ...finished, headSha: "sha3", startedAt: at(4).toISOString(), diffHash: diffHash(DIFF) }; + // A finished review names its diff's hash, so a later head with the same diff takes it over. + await recordEvent(db, TENANT, GITHUB, "run_live", { type: "reviewer-result", head: "sha4", payload: { kind: "code-review", result: OUTPUT, diffHash: diffHash(DIFF) } }); + const hashed = { ...finished, headSha: "sha4", diffHash: diffHash(DIFF) }; expect(await runs.carries(TENANT, RECORD, 7, head)).toBe(true); - expect(await runs.latest(TENANT, { repo: "acme/widgets", number: 7, head: "sha4" })).toEqual({ review: hashed, completed: hashed, carriedTo: "sha4" }); + expect(await reviewsOf(GITHUB, "sha5")).toEqual({ review: hashed, completed: hashed, carriedTo: "sha5" }); expect(hostReads).toBe(0); - expect(await runs.latest(TENANT, { repo: "acme/widgets", number: 7, head: "sha3" })).toMatchObject({ carriedTo: null }); + expect(await reviewsOf(GITHUB, "sha4")).toMatchObject({ carriedTo: null }); - expect(await runs.carries(TENANT, RECORD, 7, { ...head, headSha: "sha5", diff: BASE_EDITED })).toBe(false); - expect(await runs.carries(TENANT, RECORD, 7, { ...head, headSha: "sha6", diff: REBUILT })).toBe(false); - expect(await runs.carries(TENANT, RECORD, 7, { ...head, headSha: "sha7", diff: `${DIFF}+c\n` })).toBe(false); + expect(await runs.carries(TENANT, RECORD, 7, { ...head, headSha: "sha6", diff: BASE_EDITED })).toBe(false); + expect(await runs.carries(TENANT, RECORD, 7, { ...head, headSha: "sha7", diff: REBUILT })).toBe(false); + expect(await runs.carries(TENANT, RECORD, 7, { ...head, headSha: "sha8", diff: `${DIFF}+c\n` })).toBe(false); - head = { ...head, headSha: "sha8" }; - expect(await runs.latest(TENANT, { repo: "acme/widgets", number: 7, head: "sha8" })).toMatchObject({ carriedTo: "sha8" }); - expect(await runs.latest(TENANT, { repo: "acme/widgets", number: 7, head: "sha9" })).toMatchObject({ carriedTo: null }); + head = { ...head, headSha: "sha10" }; + expect(await reviewsOf(GITHUB, "sha10")).toMatchObject({ carriedTo: "sha10" }); + expect(await reviewsOf(GITHUB, "sha11")).toMatchObject({ carriedTo: null }); expect(hostReads).toBe(2); // A read that fails, or finds another head, is not tried again on every poll. hostDown = true; - expect(await runs.latest(TENANT, { repo: "acme/widgets", number: 7, head: "sha10" })).toMatchObject({ carriedTo: null }); - expect(await runs.latest(TENANT, { repo: "acme/widgets", number: 7, head: "sha10" })).toMatchObject({ carriedTo: null }); - expect(await runs.latest(TENANT, { repo: "acme/widgets", number: 7, head: "sha9" })).toMatchObject({ carriedTo: null }); + expect(await reviewsOf(GITHUB, "sha12")).toMatchObject({ carriedTo: null }); + expect(await reviewsOf(GITHUB, "sha12")).toMatchObject({ carriedTo: null }); + expect(await reviewsOf(GITHUB, "sha11")).toMatchObject({ carriedTo: null }); expect(hostReads).toBe(3); } finally { await handle?.close(); diff --git a/apps/hub/src/code-review-runs.ts b/apps/hub/src/code-review-runs.ts index 9588176bf..bd8c743ea 100644 --- a/apps/hub/src/code-review-runs.ts +++ b/apps/hub/src/code-review-runs.ts @@ -1,23 +1,15 @@ -// A pull request's latest code review, read from the committed run logs of its host's pr-code-review deployments; no -// table holds reviews. A head whose diff is the one the latest review read, as after a rebase that changed no line of -// it, takes that review over instead of being reviewed again. -import { and, desc, eq, inArray, ne } from "drizzle-orm"; +// A pull request's latest code review, as its manager recorded it: one still out, what one reported, or one the manager +// gave up on. A head whose diff is the one the latest review read, as after a rebase that changed no line of it, takes +// that review over instead of being reviewed again. +import { eq } from "drizzle-orm"; import { type } from "arktype"; import { schema, type DB } from "@intx/db"; -import { formatRunAddress } from "@intx/types"; -import { classifyTerminalEvent, WORKFLOW_RUN_REF, workflowRunRepoIdForAddress, type WorkflowRunEvent, type WorkflowRunReader } from "@intx/hub-sessions"; -import { failureReason } from "@corbits/pr-code-review/failures"; -import { CodeReviewComment, CodeReviewOutput, DiffCut, REPO_SOURCES, repoSourceOf, triggerRequestOf, type RepoSource } from "@corbits/triage-contracts"; +import { CodeReviewComment, DiffCut, REPO_SOURCES, repoSourceOf, StageResult, type RepoSource } from "@corbits/triage-contracts"; import { diffHash, type PullRequestToReview } from "./code-review-feed.js"; -import { CODE_REVIEW_WORKFLOWS } from "./github/source-workflow.js"; import { repoRecords, sourceOf, triageNs, type RepoRecord } from "./github/tenant-config.js"; -import { inlineOutput } from "./github/triage-runs.js"; import type { Member } from "./integrations/app.js"; +import { codeReviews, type CodeReviewRecord } from "./manager-state.js"; -/** How many of a workspace's newest code review runs are looked through for a pull request's latest. */ -const RECENT_RUNS = 200; -/** Finished runs read once and kept, as their logs never change again. */ -const KEPT_RUNS = 2_000; /** Heads compared with the review before them; only a comparison made is kept, never a read that failed. */ const KEPT_CARRIES = 2_000; /** A head whose diff could not be read is not read again for the portal sooner than this. */ @@ -38,8 +30,7 @@ const CompletedRun = type({ "cut?": DiffCut, "diffHash?": "string", }); -/** A failed run's `reason` says in fixed words why it failed; the run log keeps the detail. */ -const ReviewRun = CompletedRun.or({ status: "'running' | 'failed'", headSha: "string", startedAt: "string", "reason?": "string" }); +const ReviewRun = CompletedRun.or({ status: "'running' | 'failed'", headSha: "string", startedAt: "string" }); export type CompletedReview = typeof CompletedRun.infer; /** * The pull request's newest review, and its newest finished one, which is the same unless a newer one is running or @@ -48,47 +39,23 @@ export type CompletedReview = typeof CompletedRun.infer; export const CodeReviewFound = type({ review: ReviewRun.or("null"), completed: CompletedRun.or("null"), carriedTo: "string | null" }); type Found = Omit; -type Run = { source: RepoSource; repo: string; number: number; review: typeof ReviewRun.infer }; - -const REQUEST = type({ kind: "'code-review'", number: "number", checkout: { source: Source, repo: "string", sha: "string" }, "cut?": DiffCut, "diffHash?": "string" }); - -function isTerminal(event: WorkflowRunEvent): boolean { - return classifyTerminalEvent(event.type).terminal; -} - -/** The run's request and outcome; undefined for a run that names no pull request, such as one an eval started. */ -function runOf(events: readonly WorkflowRunEvent[], status: string, createdAt: Date): Run | undefined { - const started = events.find((event) => event.type === "RunStarted"); - const request = REQUEST(triggerRequestOf((started?.body["trigger"] as { payload?: unknown } | undefined)?.payload)); - if (request instanceof type.errors) return undefined; - const run = { source: request.checkout.source, repo: request.checkout.repo, number: request.number }; - const base = { headSha: request.checkout.sha, startedAt: createdAt.toISOString() }; - const output = CodeReviewOutput(inlineOutput(events.findLast((event) => event.type === "StepCompleted" && event.body["stepId"] === "review"))); - if (!(output instanceof type.errors)) { - const comments = output.comments.map(({ path, start, end, title, body, priority, quoted, unverified }) => ({ path, start, end, title, body, ...(priority !== undefined && { priority }), ...(quoted !== undefined && { quoted }), ...(unverified && { unverified }) })); - return { ...run, review: { ...base, status: "completed", summary: output.summary, comments, budgetExhausted: output.budgetExhausted, ...(output.cut && { cut: output.cut }), ...(request.diffHash && { diffHash: request.diffHash }) } }; - } - const ended = !schema.isLiveWorkflowRunStatus(status) || events.some(isTerminal); - if (!ended) return { ...run, review: { ...base, status: "running" } }; - const reason = reasonOf(events); - return { ...run, review: { ...base, status: "failed", ...(reason && { reason }) } }; -} - -const StepFailed = type({ stepId: "string", error: { message: "string" }, retriesExhausted: "true" }); - -/** Why the first step that failed for good did; the steps after it fail only for want of its output. */ -function reasonOf(events: readonly WorkflowRunEvent[]): string | undefined { - for (const event of events) { - if (event.type !== "StepFailed") continue; - const failed = StepFailed(event.body); - if (!(failed instanceof type.errors)) return failureReason(failed.stepId, failed.error.message); - } - return undefined; +type Review = typeof ReviewRun.infer; + +/** A code review as the portal shows it. */ +function reviewOf(record: CodeReviewRecord): Review { + const base = { headSha: record.head, startedAt: record.at }; + if (record.state === "running") return { ...base, status: "running" }; + if (record.state === "failed") return { ...base, status: "failed" }; + const reported = StageResult.assert({ ...record.payload, head: record.head }); + if ("failed" in reported) return { ...base, status: "failed" }; + if (reported.kind !== "code-review") throw new Error(`a code review result names ${reported.kind}`); + const { summary, budgetExhausted, cut } = reported.result; + const comments = reported.result.comments.map(({ path, start, end, title, body, priority, quoted, unverified }) => ({ path, start, end, title, body, ...(priority !== undefined && { priority }), ...(quoted !== undefined && { quoted }), ...(unverified && { unverified }) })); + return { ...base, status: "completed", summary, comments, budgetExhausted, ...(cut && { cut }), ...(reported.diffHash && { diffHash: reported.diffHash }) }; } type CodeReviewRunsDeps = { db: DB["db"]; - runReader: Pick; read: (tenantId: string, record: RepoRecord, number: number) => Promise; log: (entry: Record) => void; }; @@ -106,7 +73,6 @@ function carriable({ review, completed }: Found, headSha: string): CompletedRevi } export function createCodeReviewRuns(deps: CodeReviewRunsDeps) { - const finished = new Map(); const compared = new Map(); const unread = new Map(); @@ -114,43 +80,15 @@ export function createCodeReviewRuns(deps: CodeReviewRunsDeps) { return `${tenantId}\n${source}\n${repo}\n${number}\n${from}\n${to}`; } - async function recentRuns(tenantId: string) { - const { workflowRun, workflowDefinition } = schema; - return deps.db - .select({ id: workflowRun.id, anchorRunId: workflowRun.anchorRunId, status: workflowRun.status, createdAt: workflowRun.createdAt }) - .from(workflowRun) - .innerJoin(workflowDefinition, eq(workflowRun.definitionId, workflowDefinition.id)) - .where(and(eq(workflowRun.tenantId, tenantId), inArray(workflowDefinition.name, CODE_REVIEW_WORKFLOWS.map((workflow) => workflow.id)), ne(workflowRun.id, workflowRun.anchorRunId))) - .orderBy(desc(workflowRun.createdAt), desc(workflowRun.id)) - .limit(RECENT_RUNS); - } - - async function readRun(row: { id: string; anchorRunId: string; status: string; createdAt: Date }, domain: string): Promise { - if (finished.has(row.id)) return finished.get(row.id); - const repoId = workflowRunRepoIdForAddress(formatRunAddress(row.anchorRunId, domain)); - const events = await deps.runReader.readRunEvents(repoId, WORKFLOW_RUN_REF, row.id); - const run = runOf(events, row.status, row.createdAt); - // Only a log that has ended is kept: one whose status is ahead of its log is read again. - if (events.some(isTerminal)) keep(finished, row.id, run, KEPT_RUNS); - return run; - } - async function tenantOf(tenantId: string) { const tenant = await deps.db.query.tenant.findFirst({ where: eq(schema.tenant.id, tenantId), columns: { domain: true, config: true } }); if (!tenant) throw new Error(`Tenant ${tenantId} not found`); return tenant; } - async function reviewsOf(tenantId: string, domain: string, source: RepoSource, repo: string, number: number): Promise { - let review: Run["review"] | null = null; - for (const row of await recentRuns(tenantId)) { - if (row.anchorRunId === null) continue; - const run = await readRun({ ...row, anchorRunId: row.anchorRunId }, domain); - if (!run || run.source !== source || run.repo !== repo || run.number !== number) continue; - review ??= run.review; - if (run.review.status === "completed") return { review, completed: run.review }; - } - return { review, completed: null }; + async function reviewsOf(tenantId: string, source: RepoSource, repo: string, number: number): Promise { + const reviews = (await codeReviews(deps.db, tenantId, { source, repo, number })).map(reviewOf); + return { review: reviews[0] ?? null, completed: reviews.find((review) => review.status === "completed") ?? null }; } function sameDiff(tenantId: string, record: RepoRecord, number: number, completed: CompletedReview, head: Pick): boolean { @@ -190,15 +128,14 @@ export function createCodeReviewRuns(deps: CodeReviewRunsDeps) { async function latest(tenantId: string, query: typeof CodeReviewQuery.infer): Promise { const tenant = await tenantOf(tenantId); - const reviews = await reviewsOf(tenantId, tenant.domain, repoSourceOf(query.source), query.repo, query.number); + const reviews = await reviewsOf(tenantId, repoSourceOf(query.source), query.repo, query.number); return { ...reviews, carriedTo: await carriedTo(tenantId, tenant.config, query, reviews) }; } /** Whether `pull`'s head takes the newest review over rather than being reviewed; false when that can't be told. */ async function carries(tenantId: string, record: RepoRecord, number: number, pull: PullRequestToReview): Promise { try { - const { domain } = await tenantOf(tenantId); - const completed = carriable(await reviewsOf(tenantId, domain, sourceOf(record), record.name, number), pull.headSha); + const completed = carriable(await reviewsOf(tenantId, sourceOf(record), record.name, number), pull.headSha); return completed !== undefined && sameDiff(tenantId, record, number, completed, pull); } catch (err) { deps.log({ level: "warn", msg: "code_review_carry_unread", tenantId, repo: record.name, pr: number, error: String(err) }); diff --git a/apps/hub/src/manager-state.ts b/apps/hub/src/manager-state.ts index 39040206b..920343b22 100644 --- a/apps/hub/src/manager-state.ts +++ b/apps/hub/src/manager-state.ts @@ -6,6 +6,7 @@ import { and, desc, eq, inArray, isNull, like, sql } from "drizzle-orm"; import { jsonb, primaryKey, text } from "drizzle-orm/pg-core"; import { type } from "arktype"; import { schema, type DB } from "@intx/db"; +import { asRecord } from "@corbits/common"; import { APPROVE_DO_KIND, TRIAGE_LISTENERS, type RepoSource } from "@corbits/triage-contracts"; import type { Tx } from "./github/admin-github-grant.js"; import { resolveLiveDeployment, type LiveDeployment } from "./github/deployment.js"; @@ -262,6 +263,46 @@ async function newestRequest(tx: Tx, tenantId: string, pr: PullRequestRef, head? return row && eventOf(row.key, row.state).payload; } +/** A code review the pull request's manager deployed: still out, what it reported, or that the manager gave up on it. */ +export type CodeReviewRecord = { runId: string; head: string; at: string } & ({ state: "running" } | { state: "reported"; payload: Record } | { state: "failed"; failed: string }); + +/** + * The pull request's code reviews, newest first: each result a reviewer reported that a pass took or that names a + * reviewer of the claim, and each reviewer the claim records without one, running until its deadline or failed as the + * manager recorded it. + */ +export async function codeReviews(db: DB["db"], tenantId: string, pr: PullRequestRef): Promise { + const key = claimKey(pr); + const [row] = await db.select({ state: managerState.state }).from(managerState) + .where(and(eq(managerState.tenantId, tenantId), eq(managerState.key, key), eq(managerState.kind, "claim"))); + if (!row) return []; + const claim = claimOf(key, row.state); + const reviewers = claim.reviewers.filter((reviewer) => reviewer.kind === "code-review"); + const taken = new Set([...claim.history.flatMap((pass) => pass.events), ...(claim.pass ? claim.pass.events : [])]); + const named = new Set(reviewers.map((reviewer) => reviewer.runId)); + const prefix = eventKey(pr, ""); + const events = await db.select({ key: managerState.key, state: managerState.state }).from(managerState) + .where(and(eq(managerState.tenantId, tenantId), sql`${managerState.kind} = 'event'`, like(managerState.key, `${eventPrefix(pr)}%`), sql`${managerState.state}->>'type' = 'reviewer-result'`, sql`${managerState.state}->'payload'->>'kind' = 'code-review'`)); + const results = eventsOf(events).filter((result) => taken.has(result.key) || named.has(result.key.slice(prefix.length))); + const records: CodeReviewRecord[] = results.map((result) => ({ runId: result.key.slice(prefix.length), head: result.event.head, at: result.event.receivedAt, state: "reported", payload: result.event.payload })); + const reported = new Set(records.map((record) => record.runId)); + const pending = reviewers.filter((reviewer) => !reported.has(reviewer.runId)); + if (pending.length > 0) { + const runs = await db.select({ id: schema.workflowRun.id, createdAt: schema.workflowRun.createdAt }).from(schema.workflowRun) + .where(inArray(schema.workflowRun.id, pending.map((reviewer) => reviewer.runId))); + for (const reviewer of pending) { + // A reviewer's run outlives its claim record; one the store no longer holds has nothing to show. + const run = runs.find((candidate) => candidate.id === reviewer.runId); + if (!run) continue; + const failed = asRecord(reviewer.result)?.["failed"]; + const base = { runId: reviewer.runId, head: reviewer.head, at: run.createdAt.toISOString() }; + if (typeof failed === "string") records.push({ ...base, state: "failed", failed }); + else if (reviewer.result === undefined && claim.status === "live" && Date.parse(reviewer.deadline) > Date.now()) records.push({ ...base, state: "running" }); + } + } + return records.sort((a, b) => b.at.localeCompare(a.at)); +} + /** * Claims the pull request for `runId` when no live run holds it, then answers the open pass again, or a new one on the * newest head over every pending event no other transaction holds. Reviewer results for another head are consumed and diff --git a/apps/hub/src/server.ts b/apps/hub/src/server.ts index c583b72ba..d86ee65ea 100644 --- a/apps/hub/src/server.ts +++ b/apps/hub/src/server.ts @@ -410,7 +410,7 @@ const githubManifest = createGithubManifestIntegration({ const tenantBitbucket = createTenantBitbucket({ ...credentialDeps, fetchImpl: fetch, egressAllowlist: bitbucketEgressAllowlist(env) }); const tenantGithub = createTenantGithub({ ...credentialDeps, githubApiOrigin: githubOrigin }); const pullRequests = createPullRequestReader({ tenantGithub, tenantBitbucket }); -const codeReviewRuns = createCodeReviewRuns({ db: composition.db, runReader: composition.runReader, read: pullRequests.withDiff, log: logJson }); +const codeReviewRuns = createCodeReviewRuns({ db: composition.db, read: pullRequests.withDiff, log: logJson }); const repoReads = createRepoReads(now); const githubPrActions = createGithubPrActions({ ...credentialDeps, diff --git a/apps/web/src/components/CodeReviewThreads.tsx b/apps/web/src/components/CodeReviewThreads.tsx index 904a1a7e8..b8d2b5b7c 100644 --- a/apps/web/src/components/CodeReviewThreads.tsx +++ b/apps/web/src/components/CodeReviewThreads.tsx @@ -127,7 +127,6 @@ export function CodeReviewThreads({ item }: { item: Reviewed }) { {again.error ?

Could not review again. {errorText(again.error)}

: null} {send.error ?

Could not send to {hostName(item.source)}. {errorText(send.error)}

: null} - {reviews.review?.status === "failed" && reviews.review.reason ?

{reviews.review.reason}

: null} {completed && carriedTo ?

Carried over from {shortSha(completed.headSha)}: the new commits leave the diff as it was reviewed.

: null} {completed?.summary ?

{completed.summary}

: null} {completed?.cut ? : null} diff --git a/apps/web/src/lib/hub-api.ts b/apps/web/src/lib/hub-api.ts index cc454323e..2a5f5fcb9 100644 --- a/apps/web/src/lib/hub-api.ts +++ b/apps/web/src/lib/hub-api.ts @@ -886,8 +886,6 @@ export type CodeReviewRun = { budgetExhausted?: boolean; /** Set when the diff was cut to its first `lines` of `of` for size, with the files it cut part-way or left out. */ cut?: { lines: number; of: number; unread?: string[] }; - /** Why a failed review did not finish. */ - reason?: string; }; /** diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 370dbadb2..0a59f55a1 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -544,8 +544,8 @@ incomplete. A repository with no config is not linted. A lint that fails is Each request carries `diffHash`, a SHA-256 of the full diff, `index` lines included, so a base change to a file the diff touches, or a binary change, changes it. A diff with no `index` line, as GitHub's rebuilt from the file -list, is sent unhashed. The hash lives in the run log, so carrying over works -after a restart: a new head whose diff hashes the same as the newest review's, +list, is sent unhashed. A manager's code review reports the hash with its +review, so carrying over works after a restart: a new head whose diff hashes the same as the newest review's, when that review finished, is not mailed but takes the review over (`carries` in `code-review-runs.ts`). `GET …/code-review` with `head` answers `carriedTo`, reading that head's diff once; a failed read is not retried for five minutes. @@ -560,13 +560,16 @@ comment with the rest. Each comment's first line is a link-reference marker naming the head (and the finding's index), so a send that failed part-way posts only what is missing, and none once the summary stands on either head. -A pull request's reviews are read from the run logs. A failed run's reason is -a fixed sentence for why its first step that failed for good did -(`failureReason` in `pr-code-review`); the run log keeps the error itself. +A pull request's latest review (`GET …/code-review`, `code-review-runs.ts`) +is read from `manager_state`: the newest of the code reviews its claim still +waits on, shown as running, those the manager gave up on, such as one past its +deadline, and the results its code reviews reported through the +reviewer-result route that a pass took or that name a reviewer of the claim. A +review that did not finish shows as failed; its run log keeps the error. Sending a review to GitHub is a person's write through `POST /api/integrations/github-actions/{tenantId}` (`action: "code-review"`, -the Comment grant): the hub reads the newest finished review from the run logs +the Comment grant): the hub reads the newest finished review the same way and posts it as one `COMMENT` review pinned to its head. The review's marker and `commit_id` are its effect id, so the App's marked review of that head is found before anything is written. diff --git a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md index fe3f58790..a8173357f 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -437,7 +437,7 @@ Clicking a pull request opens it on the right. The pane is built to be read top Some lines come before the button. A pull request the author updated after your review reads **Re-review: @ada updated it**, then what is still open, such as **· still not met: Linked issue** or **· still to decide: Focused**, or **· all rules met** when nothing is. A rule you need to decide reads **Decide: Focused (Triage wasn't sure)** when the model was unsure, **Decide: Linked issue (your policy asks you)** when the policy asks you about it when it fails, or **(see why below)** for both. Once the feedback is posted it is the author's turn, and it reads **Waiting on @ada to fix: Linked issue**; **Reply** stays available if you want to add something. A rule that could not be checked reads **Couldn't check Merge conflicts · use Triage again in More**. After you approve, it says only what is still open or what holds the merge: **You approved · still not met: Linked issue**, **· still to decide: Focused**, **· couldn't check: Merge conflicts**, **· waiting on @ada**, **· can't merge yet: behind its base branch** or **· ready to merge**. The change you requested is no longer listed. While Triage runs, waits for CI, or checks new commits it says so, such as **Rechecking**, and the row says the same. 3. **The problems**, if any, failed rules first. Each one names the rule, then why, such as **Linked issue: Pull request does not reference an issue.**, with the evidence, such as the name of the failing check. A rule you need to decide is marked **Decide**. A pull request with no problems skips straight to the description. -4. **Code review**, when it is on for the repository: what the change does, then each bug it found as a comment from **Code review**, with the commit it reviewed, how severe it is (P0 is the most severe), the file and line, and the code it points at. A bug in a file code review could not read says **Not checked against the file**, here and on GitHub. If the model keeps failing or a reviewer's answer can't be read, the review shows **The last review did not finish** instead of **No findings**, with the reason under it. **Review again** reviews the latest commit once more. A commit that left the change as it was keeps the last review and says **Carried over from** the commit it was made for. On a GitHub pull request, **Send to GitHub** posts the review to the pull request, inline where the lines are part of the change; it then says **Sent to GitHub**. On a Bitbucket Data Center pull request, **Send to Bitbucket** does the same with a comment per finding and one summary comment. Either needs the Comment permission for the repository, and is refused once the pull request has newer commits that did not keep the review: review again, then send. +4. **Code review**, when it is on for the repository: what the change does, then each bug it found as a comment from **Code review**, with the commit it reviewed, how severe it is (P0 is the most severe), the file and line, and the code it points at. A bug in a file code review could not read says **Not checked against the file**, here and on GitHub. If the review fails or runs past its hour, the section shows **The last review did not finish** instead of **No findings**. **Review again** reviews the latest commit once more. A commit that left the change as it was keeps the last review and says **Carried over from** the commit it was made for. On a GitHub pull request, **Send to GitHub** posts the review to the pull request, inline where the lines are part of the change; it then says **Sent to GitHub**. On a Bitbucket Data Center pull request, **Send to Bitbucket** does the same with a comment per finding and one summary comment. Either needs the Comment permission for the repository, and is refused once the pull request has newer commits that did not keep the review: review again, then send. 5. **The description** the author wrote. 6. **Folded rows** you can expand: the linked issues, the changed files (with the diff), and the rules that passed. Evidence for a file cites its changed lines and links them at the exact commit Triage judged. Linked issues are the GitHub issues the title, description or commits mention, and the Linear issues linked in the description, the commits or a comment from one of Linear's bots. Links hidden in the description are ignored. 7. **Commits and conversation** opens the full history and comments. diff --git a/docs/IMPLEMENTATION.md b/docs/IMPLEMENTATION.md index b1702bd5e..c06b0235a 100644 --- a/docs/IMPLEMENTATION.md +++ b/docs/IMPLEMENTATION.md @@ -27,7 +27,7 @@ with the `intx-src` export condition, so every hub command runs with | `src/audit-log.ts` | The audit list, read from what the workspace already recorded. | | `src/invitations/` | Invites as stock invited principals with magic links, and invite email over SMTP. | | `src/workflow-deploy.ts`, `src/workflow-versions.ts` | Deploys the portal's published workflow versions; lists what is deployed. | -| `src/code-review.ts`, `src/code-review-feed.ts`, `src/code-review-runs.ts` | The code review switch; `…/place`, the selected provisioner and each place probed at boot (`dockerPlaces`); `…/run`; the feed that mails each new head; the latest review, read from run logs, and whether a head takes it over. | +| `src/code-review.ts`, `src/code-review-feed.ts`, `src/code-review-runs.ts` | The code review switch; `…/place`, the selected provisioner and each place probed at boot (`dockerPlaces`); `…/run`; the feed that mails each new head; the latest review, read from the manager's state, and whether a head takes it over. | | `src/sidecar/` | Local-process `SidecarProvisioner` (lifecycle, spawn env, encrypted manifests, boot restore, `host-capacity.ts`) and Docker registration (`docker.ts`). | | `src/sidecar-config.ts` | Sidecar adapter manifest (System One model provider). | | `src/bitbucket-datacenter/` | Bitbucket Data Center connection, repository and webhook routes, payload normalizing. | diff --git a/packages/pr-code-review/src/failures.ts b/packages/pr-code-review/src/failures.ts index e9ecc9d6a..e1d84d4d9 100644 --- a/packages/pr-code-review/src/failures.ts +++ b/packages/pr-code-review/src/failures.ts @@ -5,14 +5,3 @@ export const MODEL_ATTEMPTS = 3; export const UNREADABLE = "reader: no findings list in the answer"; export const MODEL_FAILED = "inference failed"; export const NO_CHECKOUT = "reader: no checkout"; - -const FINDERS: Record = { findDiff: "diff reviewer", findRepo: "repository reviewer" }; - -/** Why a review failed, in fixed words for a reader, from the step that failed first and its error. */ -export function failureReason(stepId: string, message: string): string { - if (message.includes(NO_CHECKOUT)) return "The repository checkout wasn't available."; - if (message.includes(MODEL_FAILED)) return `The model call failed after ${MODEL_ATTEMPTS} tries.`; - const finder = FINDERS[stepId]; - if (finder && message.includes(UNREADABLE)) return `The ${finder}'s answer couldn't be read.`; - return "A step of the review failed."; -} diff --git a/packages/triage-contracts/src/stage-result.ts b/packages/triage-contracts/src/stage-result.ts index 9167fcdf4..a04c815d6 100644 --- a/packages/triage-contracts/src/stage-result.ts +++ b/packages/triage-contracts/src/stage-result.ts @@ -32,9 +32,10 @@ export type JudgeVerdict = typeof JudgeVerdict.infer; /** * What a reviewer reports to its pull request's manager: the head it reviewed and its result, or why it has none; the - * hub knows the reporting run from its run token. + * hub knows the reporting run from its run token. A code review names the hash of the diff it read, which a later head + * with the same diff takes the review over by. */ -export const StageResult = Reported.and({ kind: "'code-review'", result: CodeReviewOutput }) +export const StageResult = Reported.and({ kind: "'code-review'", result: CodeReviewOutput, "diffHash?": "string > 0" }) .or(Reported.and({ kind: "'judge'", result: JudgeVerdict })) .or(Reported.and({ kind: "'code-review' | 'judge'", failed: "string > 0", "result?": "never" })); export type StageResult = typeof StageResult.infer; diff --git a/packages/triage-manager/src/actions/report.test.ts b/packages/triage-manager/src/actions/report.test.ts index 7cf6eb90a..3f469187e 100644 --- a/packages/triage-manager/src/actions/report.test.ts +++ b/packages/triage-manager/src/actions/report.test.ts @@ -2,7 +2,7 @@ import { expect, test } from "bun:test"; import type { EffectContext } from "@intx/workflow"; import { report, reviewRequest, stop } from "./report.js"; -const REQUEST = { kind: "code-review", number: 8, title: "Add the flag", description: "", diff: "diff --git a/f b/f", checkout: { source: "github", repo: "acme/widgets", sha: "h1" } }; +const REQUEST = { kind: "code-review", number: 8, title: "Add the flag", description: "", diff: "diff --git a/f b/f", checkout: { source: "github", repo: "acme/widgets", sha: "h1" }, diffHash: "hash1" }; const REPLY = { to: "run_manager", content: "{\"kind\":\"manage\"}" }; const MAIL = { headers: { from: "run_manager@acme.test", to: ["run_review@acme.test"] }, parts: [{ contentType: "text/plain", text: JSON.stringify({ ...REQUEST, tenantId: "tnt_acme", reply: REPLY }) }] }; const REVIEW = { summary: "No bugs.", comments: [], budgetExhausted: false }; @@ -19,7 +19,7 @@ function hubContext() { return { ctx, calls }; } -test("a manager's code review hands its request to the review, reports the review through the hub, replies to the manager and stops, posting nothing", async () => { +test("a manager's code review hands its request to the review, reports the review and its diff's hash through the hub, replies to the manager and stops, posting nothing", async () => { const { ctx, calls } = hubContext(); expect(await reviewRequest(MAIL)).toEqual({ request: { ...REQUEST, tenantId: "tnt_acme", reply: REPLY } }); expect(await report({ payload: MAIL, review: { output: REVIEW } }, ctx, new AbortController().signal)).toEqual({ recorded: true, mail: { tenantId: "tnt_acme", ...REPLY } }); @@ -27,7 +27,7 @@ test("a manager's code review hands its request to the review, reports the revie expect(await stop(MAIL, ctx, new AbortController().signal)).toEqual({ stopped: true }); const reported = { source: "github", repo: "acme/widgets", number: 8, kind: "code-review", head: "h1" }; expect(calls).toEqual([ - { method: "POST", path: "/api/tenants/tnt_acme/manager/reviewer-result", body: { ...reported, result: REVIEW } }, + { method: "POST", path: "/api/tenants/tnt_acme/manager/reviewer-result", body: { ...reported, result: REVIEW, diffHash: "hash1" } }, { method: "POST", path: "/api/tenants/tnt_acme/manager/reviewer-result", body: { ...reported, failed: "the review did not finish" } }, { method: "DELETE", path: "/api/tenants/tnt_acme/workflows/runs/run_review" }, ]); diff --git a/packages/triage-manager/src/actions/report.ts b/packages/triage-manager/src/actions/report.ts index 49b7288a7..0e1dd69cd 100644 --- a/packages/triage-manager/src/actions/report.ts +++ b/packages/triage-manager/src/actions/report.ts @@ -10,6 +10,7 @@ const ReviewMail = type({ tenantId: "string > 0", number: "number.integer > 0", checkout: { source: type.enumerated(...REPO_SOURCES), repo: "string > 0", sha: "string > 0" }, + "diffHash?": "string > 0", reply: { to: "string > 0", content: "string > 0" }, }); @@ -38,9 +39,9 @@ const NOT_FINISHED = "the review did not finish"; export async function report(input: unknown, ctx: EffectContext, _signal: AbortSignal): Promise<{ recorded: boolean; mail: MailInput }> { const parsed = ReportInput(input); if (parsed instanceof type.errors) throw new Error(`report: ${parsed.summary}`); - const { tenantId, number, checkout, reply } = reviewMailOf(parsed.payload).mail; + const { tenantId, number, checkout, diffHash, reply } = reviewMailOf(parsed.payload).mail; const review = CodeReviewOutput(parsed.review?.output); - const outcome = review instanceof type.errors ? { failed: NOT_FINISHED } : { result: review }; + const outcome = review instanceof type.errors ? { failed: NOT_FINISHED } : { result: review, ...(diffHash && { diffHash }) }; const result = { source: checkout.source, repo: checkout.repo, number, kind: "code-review", head: checkout.sha, ...outcome }; const { recorded } = answered(await callHub(ctx, "report", tenantPath(tenantId, "/manager/reviewer-result"), "POST", result), "report", Recorded); return { recorded, mail: { tenantId, ...reply } };