From d83153bd86cb1be1ca309075be3a376e5e178ae2 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 26 Oct 2026 11:04:30 -0700 Subject: [PATCH 1/2] fix(hub): show the manager's code review in the portal --- .../hub/src/code-review-runs.postgres.test.ts | 120 +++++------------- apps/hub/src/code-review-runs.ts | 117 +++++------------ apps/hub/src/manager-state.ts | 24 ++++ apps/hub/src/server.ts | 2 +- docs/ARCHITECTURE.md | 14 +- docs/IMPLEMENTATION.md | 2 +- packages/triage-contracts/src/stage-result.ts | 5 +- .../triage-manager/src/actions/report.test.ts | 6 +- packages/triage-manager/src/actions/report.ts | 5 +- 9 files changed, 106 insertions(+), 189 deletions(-) diff --git a/apps/hub/src/code-review-runs.postgres.test.ts b/apps/hub/src/code-review-runs.postgres.test.ts index ddddf0c15..c2e5d261e 100644 --- a/apps/hub/src/code-review-runs.postgres.test.ts +++ b/apps/hub/src/code-review-runs.postgres.test.ts @@ -3,27 +3,18 @@ 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 { diffHash } 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 RECORD = { name: "acme/widgets", connected: true, codeReview: true }; +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"; -// The same change on a base that changed its file outside the context lines: only the blob ids differ. -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.", @@ -33,27 +24,7 @@ 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, - ], -}; - -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 () => { +test("PostgreSQL: a pull request's latest code review is the one its manager still waits on, else the newest a reviewer reported, per host", async () => { const database = `corbits_reviews_${randomBytes(6).toString("hex")}`; const admin = createDB(TEST_DB); await admin.db.execute(sql.raw(`create database ${database}`)); @@ -63,69 +34,44 @@ 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 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 }; - let hostReads = 0; - let hostDown = false; + await migrateTriageTables(db, "public"); + await db.insert(schema.tenant).values({ id: TENANT, name: "Reviews", slug: "reviews", domain: "reviews.test" }); 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"); - return head; + throw new Error("no head is asked about"); }, log() {}, }); - async function reviewsOf(number: number) { - return runs.latest(TENANT, { repo: "acme/widgets", number }); + async function reviewsOf(pr: PullRequestRef) { + return runs.latest(TENANT, { repo: pr.repo, source: pr.source, number: pr.number }); } - 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 }); - 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); + expect(await reviewsOf(GITHUB)).toEqual({ review: null, completed: null, 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) }; - 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(hostReads).toBe(0); - expect(await runs.latest(TENANT, { repo: "acme/widgets", number: 7, head: "sha3" })).toMatchObject({ 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_judge", { type: "reviewer-result", head: "sha1", payload: { kind: "judge", failed: "the judge did not finish" } }); + const finished = { status: "completed" as const, headSha: "sha1", startedAt: expect.any(String), ...REVIEW }; + expect(await reviewsOf(GITHUB)).toEqual({ review: finished, completed: finished, carriedTo: null }); + expect(await reviewsOf(BITBUCKET)).toEqual({ review: null, completed: null, 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); + await recordEvent(db, TENANT, GITHUB, "run_new", { type: "reviewer-result", head: "sha2", payload: { kind: "code-review", failed: "the review did not finish" } }); + expect(await reviewsOf(GITHUB)).toEqual({ review: { status: "failed", headSha: "sha2", startedAt: expect.any(String), reason: "the review did not finish" }, completed: finished, carriedTo: null }); - 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 }); - expect(hostReads).toBe(2); + await db.insert(schema.workflowDefinition).values({ id: "wfd_review", tenantId: TENANT, name: "pr-code-review-reviewer-github" }); + const deployedAt = new Date("2026-10-26T10:00:00.000Z"); + await db.insert(schema.workflowRun).values({ id: "run_live", definitionId: "wfd_review", anchorRunId: "run_live", tenantId: TENANT, status: "running", createdAt: deployedAt }); + const deadline = new Date(Date.now() + 60 * 60_000).toISOString(); + const claim = { status: "live", claimId: "claim-1", claimedAt: deployedAt.toISOString(), runId: "run_manager", reviewers: [{ kind: "code-review", head: "sha3", runId: "run_live", deployedBy: "run_manager", deadline }], history: [], 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(claim)}::jsonb)`); + expect(await reviewsOf(GITHUB)).toEqual({ review: { status: "running", headSha: "sha3", startedAt: deployedAt.toISOString() }, completed: finished, carriedTo: null }); - // 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(hostReads).toBe(3); + // 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: "sha3", payload: { kind: "code-review", result: OUTPUT, diffHash: diffHash(DIFF) } }); + const pull = { title: "Fix", description: "", headSha: "sha4", draft: false, open: true, diff: DIFF }; + expect(await runs.carries(TENANT, RECORD, 7, pull)).toBe(true); + expect(await runs.carries(TENANT, RECORD, 7, { ...pull, diff: `${DIFF}+c\n` })).toBe(false); + expect(await reviewsOf(GITHUB)).toMatchObject({ review: { status: "completed", headSha: "sha3", diffHash: diffHash(DIFF) }, completed: { headSha: "sha3" } }); } finally { await handle?.close(); await admin.db.execute(sql.raw(`drop database if exists ${database} with (force)`)); diff --git a/apps/hub/src/code-review-runs.ts b/apps/hub/src/code-review-runs.ts index 9588176bf..675c7c0ba 100644 --- a/apps/hub/src/code-review-runs.ts +++ b/apps/hub/src/code-review-runs.ts @@ -1,23 +1,16 @@ -// 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: one its manager deployed and still waits on, else the newest result such a code +// review reported. 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 { reviewerResults, reviewersOut } 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,7 +31,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. */ +/** A failed review's `reason` says in fixed words why it did not finish. */ const ReviewRun = CompletedRun.or({ status: "'running' | 'failed'", headSha: "string", startedAt: "string", "reason?": "string" }); export type CompletedReview = typeof CompletedRun.infer; /** @@ -48,47 +41,21 @@ 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 reported review as the portal shows it; a review that did not finish says why in fixed words. */ +function reviewOf(event: { head: string; receivedAt: string; payload: Record }): Review { + const reported = StageResult.assert({ ...event.payload, head: event.head }); + const base = { headSha: event.head, startedAt: event.receivedAt }; + if ("failed" in reported) return { ...base, status: "failed", reason: reported.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,21 @@ 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 pr = { source, repo, number }; + const [results, out] = await Promise.all([reviewerResults(deps.db, tenantId, pr, "code-review"), reviewersOut(deps.db, tenantId, pr, "code-review")]); + const reported = new Set(results.map((result) => result.runId)); + const [running] = out.filter((reviewer) => !reported.has(reviewer.runId)).sort((a, b) => b.deployedAt.getTime() - a.deployedAt.getTime()); + const reviews = results.map(reviewOf); + const completed = reviews.find((review) => review.status === "completed") ?? null; + if (running) return { review: { status: "running", headSha: running.head, startedAt: running.deployedAt.toISOString() }, completed }; + return { review: reviews[0] ?? null, completed }; } function sameDiff(tenantId: string, record: RepoRecord, number: number, completed: CompletedReview, head: Pick): boolean { @@ -190,15 +134,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..4ff8027e1 100644 --- a/apps/hub/src/manager-state.ts +++ b/apps/hub/src/manager-state.ts @@ -262,6 +262,30 @@ async function newestRequest(tx: Tx, tenantId: string, pr: PullRequestRef, head? return row && eventOf(row.key, row.state).payload; } +/** The results reviewers of `kind` reported for the pull request, newest first, each under the reporting run. */ +export async function reviewerResults(db: DB["db"], tenantId: string, pr: PullRequestRef, kind: "code-review" | "judge"): Promise<(ManagerEvent & { runId: string })[]> { + const prefix = eventPrefix(pr); + const rows = await db.select({ key: managerState.key, state: managerState.state }).from(managerState) + .where(and(eq(managerState.tenantId, tenantId), sql`${managerState.kind} = 'event'`, like(managerState.key, `${prefix}%`), sql`${managerState.state}->>'type' = 'reviewer-result'`, sql`${managerState.state}->'payload'->>'kind' = ${kind}`)); + return eventsOf(rows).map((row) => ({ ...row.event, runId: row.key.slice(eventKey(pr, "").length) })).reverse(); +} + +/** The reviewers of `kind` the pull request's live claim still waits on before their deadline, with when each was deployed. */ +export async function reviewersOut(db: DB["db"], tenantId: string, pr: PullRequestRef, kind: "code-review" | "judge"): Promise<{ runId: string; head: string; deployedAt: Date }[]> { + 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"))); + const claim = row && claimOf(key, row.state); + const out = claim?.status === "live" ? claim.reviewers.filter((reviewer) => reviewer.kind === kind && reviewer.result === undefined && Date.parse(reviewer.deadline) > Date.now()) : []; + if (out.length === 0) return []; + const runs = await db.select({ id: schema.workflowRun.id, createdAt: schema.workflowRun.createdAt }).from(schema.workflowRun) + .where(inArray(schema.workflowRun.id, out.map((reviewer) => reviewer.runId))); + return out.flatMap(function deployed(reviewer) { + const run = runs.find((candidate) => candidate.id === reviewer.runId); + return run ? [{ runId: reviewer.runId, head: reviewer.head, deployedAt: run.createdAt }] : []; + }); +} + /** * 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/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 370dbadb2..adbb6369f 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,15 @@ 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 the code review its manager still waits on, shown as running, else the +newest result a manager's code review reported through the reviewer-result +route, read from `manager_state`. A review that did not finish says so in +fixed words; its run log keeps the error itself. 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/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/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 } }; From 828b17a25adadbecb8c97139ebeac571daab1cd7 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Mon, 26 Oct 2026 11:43:47 -0700 Subject: [PATCH 2/2] fix(hub): show a code review the manager gave up on, and only results its passes took --- .../hub/src/code-review-runs.postgres.test.ts | 94 ++++++++++++++----- apps/hub/src/code-review-runs.ts | 34 +++---- apps/hub/src/manager-state.ts | 53 +++++++---- apps/web/src/components/CodeReviewThreads.tsx | 1 - apps/web/src/lib/hub-api.ts | 2 - docs/ARCHITECTURE.md | 9 +- .../Corbits Triage User Manual.md | 2 +- packages/pr-code-review/src/failures.ts | 11 --- 8 files changed, 126 insertions(+), 80 deletions(-) diff --git a/apps/hub/src/code-review-runs.postgres.test.ts b/apps/hub/src/code-review-runs.postgres.test.ts index c2e5d261e..7b5e3a693 100644 --- a/apps/hub/src/code-review-runs.postgres.test.ts +++ b/apps/hub/src/code-review-runs.postgres.test.ts @@ -3,7 +3,7 @@ import { expect, test } from "bun:test"; import { randomBytes } from "node:crypto"; import { sql } from "drizzle-orm"; import { createDB, runMigrations, schema } from "@intx/db"; -import { diffHash } from "./code-review-feed.js"; +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"; @@ -12,9 +12,13 @@ const TEST_DB = { host: "localhost", port: 5432, user: "postgres", password: "po 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"; +// The same change on a base that changed its file outside the context lines: only the blob ids differ. +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", ""); const REVIEW = { summary: "Adds total(), which skips the first element.", @@ -24,7 +28,18 @@ const REVIEW = { const OUTPUT = { ...REVIEW, comments: REVIEW.comments.map((comment) => ({ ...comment, agreement: { finders: 1, reports: 1 } })) }; -test("PostgreSQL: a pull request's latest code review is the one its manager still waits on, else the newest a reviewer reported, per host", async () => { +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 }; +} + +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}`)); @@ -35,43 +50,76 @@ test("PostgreSQL: a pull request's latest code review is the one its manager sti 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" }); + 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-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, async read() { - throw new Error("no head is asked about"); + hostReads++; + if (hostDown) throw new Error("github GET /repos/acme/widgets/pulls/7 -> 502"); + return head; }, log() {}, }); - async function reviewsOf(pr: PullRequestRef) { - return runs.latest(TENANT, { repo: pr.repo, source: pr.source, number: pr.number }); + async function reviewsOf(pr: PullRequestRef, asked?: string) { + return runs.latest(TENANT, { repo: pr.repo, source: pr.source, number: pr.number, ...(asked && { head: asked }) }); } 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_judge", { type: "reviewer-result", head: "sha1", payload: { kind: "judge", failed: "the judge did not finish" } }); + 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: finished, completed: finished, carriedTo: null }); + 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 }); - await recordEvent(db, TENANT, GITHUB, "run_new", { type: "reviewer-result", head: "sha2", payload: { kind: "code-review", failed: "the review did not finish" } }); - expect(await reviewsOf(GITHUB)).toEqual({ review: { status: "failed", headSha: "sha2", startedAt: expect.any(String), reason: "the review did not finish" }, completed: finished, 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.workflowDefinition).values({ id: "wfd_review", tenantId: TENANT, name: "pr-code-review-reviewer-github" }); - const deployedAt = new Date("2026-10-26T10:00:00.000Z"); - await db.insert(schema.workflowRun).values({ id: "run_live", definitionId: "wfd_review", anchorRunId: "run_live", tenantId: TENANT, status: "running", createdAt: deployedAt }); - const deadline = new Date(Date.now() + 60 * 60_000).toISOString(); - const claim = { status: "live", claimId: "claim-1", claimedAt: deployedAt.toISOString(), runId: "run_manager", reviewers: [{ kind: "code-review", head: "sha3", runId: "run_live", deployedBy: "run_manager", deadline }], history: [], 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(claim)}::jsonb)`); - expect(await reviewsOf(GITHUB)).toEqual({ review: { status: "running", headSha: "sha3", startedAt: deployedAt.toISOString() }, completed: finished, carriedTo: null }); + 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 }); // 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: "sha3", payload: { kind: "code-review", result: OUTPUT, diffHash: diffHash(DIFF) } }); - const pull = { title: "Fix", description: "", headSha: "sha4", draft: false, open: true, diff: DIFF }; - expect(await runs.carries(TENANT, RECORD, 7, pull)).toBe(true); - expect(await runs.carries(TENANT, RECORD, 7, { ...pull, diff: `${DIFF}+c\n` })).toBe(false); - expect(await reviewsOf(GITHUB)).toMatchObject({ review: { status: "completed", headSha: "sha3", diffHash: diffHash(DIFF) }, completed: { headSha: "sha3" } }); + 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 reviewsOf(GITHUB, "sha5")).toEqual({ review: hashed, completed: hashed, carriedTo: "sha5" }); + expect(hostReads).toBe(0); + expect(await reviewsOf(GITHUB, "sha4")).toMatchObject({ carriedTo: null }); + + 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: "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 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(); await admin.db.execute(sql.raw(`drop database if exists ${database} with (force)`)); diff --git a/apps/hub/src/code-review-runs.ts b/apps/hub/src/code-review-runs.ts index 675c7c0ba..bd8c743ea 100644 --- a/apps/hub/src/code-review-runs.ts +++ b/apps/hub/src/code-review-runs.ts @@ -1,7 +1,6 @@ -// A pull request's latest code review: one its manager deployed and still waits on, else the newest result such a code -// review reported. 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. +// 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"; @@ -9,7 +8,7 @@ import { CodeReviewComment, DiffCut, REPO_SOURCES, repoSourceOf, StageResult, ty import { diffHash, type PullRequestToReview } from "./code-review-feed.js"; import { repoRecords, sourceOf, triageNs, type RepoRecord } from "./github/tenant-config.js"; import type { Member } from "./integrations/app.js"; -import { reviewerResults, reviewersOut } from "./manager-state.js"; +import { codeReviews, type CodeReviewRecord } from "./manager-state.js"; /** Heads compared with the review before them; only a comparison made is kept, never a read that failed. */ const KEPT_CARRIES = 2_000; @@ -31,8 +30,7 @@ const CompletedRun = type({ "cut?": DiffCut, "diffHash?": "string", }); -/** A failed review's `reason` says in fixed words why it did not finish. */ -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 @@ -43,11 +41,13 @@ type Found = Omit; type Review = typeof ReviewRun.infer; -/** A reported review as the portal shows it; a review that did not finish says why in fixed words. */ -function reviewOf(event: { head: string; receivedAt: string; payload: Record }): Review { - const reported = StageResult.assert({ ...event.payload, head: event.head }); - const base = { headSha: event.head, startedAt: event.receivedAt }; - if ("failed" in reported) return { ...base, status: "failed", reason: reported.failed }; +/** 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 }) })); @@ -87,14 +87,8 @@ export function createCodeReviewRuns(deps: CodeReviewRunsDeps) { } async function reviewsOf(tenantId: string, source: RepoSource, repo: string, number: number): Promise { - const pr = { source, repo, number }; - const [results, out] = await Promise.all([reviewerResults(deps.db, tenantId, pr, "code-review"), reviewersOut(deps.db, tenantId, pr, "code-review")]); - const reported = new Set(results.map((result) => result.runId)); - const [running] = out.filter((reviewer) => !reported.has(reviewer.runId)).sort((a, b) => b.deployedAt.getTime() - a.deployedAt.getTime()); - const reviews = results.map(reviewOf); - const completed = reviews.find((review) => review.status === "completed") ?? null; - if (running) return { review: { status: "running", headSha: running.head, startedAt: running.deployedAt.toISOString() }, completed }; - return { review: reviews[0] ?? null, completed }; + 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 { diff --git a/apps/hub/src/manager-state.ts b/apps/hub/src/manager-state.ts index 4ff8027e1..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,28 +263,44 @@ async function newestRequest(tx: Tx, tenantId: string, pr: PullRequestRef, head? return row && eventOf(row.key, row.state).payload; } -/** The results reviewers of `kind` reported for the pull request, newest first, each under the reporting run. */ -export async function reviewerResults(db: DB["db"], tenantId: string, pr: PullRequestRef, kind: "code-review" | "judge"): Promise<(ManagerEvent & { runId: string })[]> { - const prefix = eventPrefix(pr); - const rows = await db.select({ key: managerState.key, state: managerState.state }).from(managerState) - .where(and(eq(managerState.tenantId, tenantId), sql`${managerState.kind} = 'event'`, like(managerState.key, `${prefix}%`), sql`${managerState.state}->>'type' = 'reviewer-result'`, sql`${managerState.state}->'payload'->>'kind' = ${kind}`)); - return eventsOf(rows).map((row) => ({ ...row.event, runId: row.key.slice(eventKey(pr, "").length) })).reverse(); -} +/** 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 reviewers of `kind` the pull request's live claim still waits on before their deadline, with when each was deployed. */ -export async function reviewersOut(db: DB["db"], tenantId: string, pr: PullRequestRef, kind: "code-review" | "judge"): Promise<{ runId: string; head: string; deployedAt: Date }[]> { +/** + * 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"))); - const claim = row && claimOf(key, row.state); - const out = claim?.status === "live" ? claim.reviewers.filter((reviewer) => reviewer.kind === kind && reviewer.result === undefined && Date.parse(reviewer.deadline) > Date.now()) : []; - if (out.length === 0) return []; - const runs = await db.select({ id: schema.workflowRun.id, createdAt: schema.workflowRun.createdAt }).from(schema.workflowRun) - .where(inArray(schema.workflowRun.id, out.map((reviewer) => reviewer.runId))); - return out.flatMap(function deployed(reviewer) { - const run = runs.find((candidate) => candidate.id === reviewer.runId); - return run ? [{ runId: reviewer.runId, head: reviewer.head, deployedAt: run.createdAt }] : []; - }); + 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)); } /** 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 adbb6369f..0a59f55a1 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -561,10 +561,11 @@ 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 latest review (`GET …/code-review`, `code-review-runs.ts`) -is the code review its manager still waits on, shown as running, else the -newest result a manager's code review reported through the reviewer-result -route, read from `manager_state`. A review that did not finish says so in -fixed words; its run log keeps the error itself. +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"`, 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/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."; -}