diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index c529c57e..7455ff05 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -273,25 +273,40 @@ any other tenant or an unknown sender. the claim: the next manager's take finds the run gone and claims. A run left deployed with no sidecar for two minutes is gone too, and its cancellation is requested so it does not start after the claim has moved on. - When the rules need the judge and `release.json` publishes one for the host, - a pass deploys it under `workflow:*` `create` and `read` on exactly the - judge's asset, which the definition declares when its module loads with that - release, mails it the rules output until the judge's one-hour deadline, and - posts nothing. The pass that has its answers runs the verdict. A judge the - release does not publish posts nothing. One that could not be mailed, - reported a failure or ran past its deadline goes on to the verdict and is - not deployed again. When the policy turns code review on, the verdict's - rules let it past, the hub read the head's review request and the release - publishes a code review, a pass deploys it the same way (without one it - declares no code-review asset, and the verdict goes up alone), mails it the - request until the review's one-hour deadline, and still posts nothing while - that review is out. The pass that collects its report mirrors the verdict, - then posts the review through the host's `send_code_review` tool, so they - go up together. A review that could not be mailed, reported a failure or - ran past its deadline leaves the verdict to go up alone. Each reviewer the - manager records names the manager run that deployed it (`deployedBy`). Done - stops each judge or code review this run deployed and no longer waits for, - one on an older head or past its deadline, through the same stop-run route; + When the rules need the judge, the host's own checks have finished (or the + hub waits for them no longer) and `release.json` publishes a judge for the + host, a pass deploys it under `workflow:*` `create` and `read` on exactly + the judge's asset, which the definition declares when its module loads with + that release, mails it the rules output until the judge's one-hour deadline, + and posts nothing. While those checks run, the judge is held and the pass + marks the head waiting, as a pass of deterministic rules does. The pass that + collects the judge's report posts and executes its verdict as the judge made + it, once that verdict names the pass's pull request, head and host; facts + and rules still run, and when they no longer need the judge (a failed hard + rule, a draft or a closed pull request) the pass's own verdict goes up + instead. A decided verdict, one that does not wait and that nothing on the + host can still change, stays on the judge's reviewer on the claim for its + head, so a later pass on that head posts it again, whatever else changed, + and deploys no other judge; a new head gets its own. A verdict the host's + checks could still change, a degraded one or one over 256 KiB still goes up + on the pass that collects it, and a failure or a missed deadline leaves the + machine verdict to go up alone; none of them is kept, so the next event + deploys the judge again. A judge the release does not publish posts nothing. + When the policy turns code review on, the verdict's rules let it past, the + hub read the head's review request and the release publishes a code review, + a pass deploys it the same way (without one it declares no code-review + asset, and the verdict goes up alone), mails it the request until the + review's one-hour deadline, and still posts nothing while that review is + out. The pass that collects its report mirrors the verdict, then posts the + review through the host's `send_code_review` tool, so they go up together. A + review stays on its reviewer for its head like a decided verdict, so a later + pass on that head whose rules still let code review past posts it with the + verdict and deploys no other; a review that could not be mailed, reported a + failure or ran past its deadline leaves the verdict to go up alone and is + deployed again on the next event. Each reviewer the manager records names + the manager run that deployed it (`deployedBy`). Done stops each judge or + code review this run deployed and no longer waits for, one on an older head + or past its deadline, through the same stop-run route; one an earlier manager deployed runs out its own deadline. Stock grants the deployer `read` on a run it deployed, and the vendored Interchange carries `manage` instead (`vendor/interchange/VENDORED.md`). @@ -306,16 +321,17 @@ any other tenant or an unknown sender. `triage-manager:result` and `manage` on `workflow-run:self`, sourced from its creator: it can report a result and stop itself, and reach no other manager route. -- `triage-judge-github` and `triage-judge-bitbucket-datacenter` - (`triageJudge` in `@corbits/triage-judge`): the judge a manager deploys. It - takes mail only from that manager. It runs the model checks the machine - rules produced, then focused's follow-up questions, then evaluate. Stock - runs a step whose dependencies failed, so however the judge ended it records - a `StageResult` of kind `judge` through the reviewer-result route (a failure - as `failed`), including the answers a later pass of the same head replays, - mails the manager a wake-up and stops. It posts nothing to the pull request. - Its definition declares only `create` on `triage-manager:result` and - `manage` on `workflow-run:self`, sourced from its creator. +- `triage-judge-github` and `triage-judge-bitbucket-datacenter` (`triageJudge` + in `@corbits/triage-judge`): the judge a manager deploys. It takes mail only + from that manager. It runs the model checks the machine rules produced, then + focused's follow-up questions, then evaluate. Stock runs a step whose + dependencies failed, so however the judge ended it records a `StageResult` + of kind `judge` through the reviewer-result route: the whole verdict + evaluate made, its comment, labels, Dos and rule results, or a failure as + `failed`. It mails the manager a wake-up and stops. It posts nothing to the + pull request. Its definition declares only `create` on + `triage-manager:result` and `manage` on `workflow-run:self`, sourced from + its creator. - A run sends mail only through an agent step's transport, so the listener, the manager, the judge and the code review mail through one agent whose director makes no model call: it runs its one tool, which waits for a @@ -838,7 +854,7 @@ read it, and passes the rest of the request to stock. own run id, and the manager matches results to its reviewers by run id. Its body is a `StageResult` (`@corbits/triage-contracts`): the reviewer's kind (`code-review` or `judge`), the head it reviewed and its result, a - code review's output or a judge's verdict detail, or `failed` with why it + code review's output or a judge's whole verdict, or `failed` with why it has none. - In a workspace with a host's listener live, pull request, waiting, held-Do approval and backlog mail goes to it instead of a pr-triage worker, diff --git a/packages/triage-contracts/src/hub-responses.ts b/packages/triage-contracts/src/hub-responses.ts index 3eefe6f8..28496a56 100644 --- a/packages/triage-contracts/src/hub-responses.ts +++ b/packages/triage-contracts/src/hub-responses.ts @@ -55,8 +55,6 @@ export const VerdictDetail = type({ "automatic?": "string[]", /** Why the run's mirror posted nothing on the host: `stale`, `waiting`, `no_request` or `no_grants`. */ "notPosted?": "string", - /** Answers a later pass of the same head replays, so the judge is not asked again. */ - "judged?": "Record", "facts?": { "+": "delete", "title?": "string", "author?": "string", "draft?": "boolean", "mergeable?": "boolean | null", "checks?": "string", "reviewAnswered?": "boolean" }, }); export type VerdictDetail = typeof VerdictDetail.infer; diff --git a/packages/triage-contracts/src/stage-result.test.ts b/packages/triage-contracts/src/stage-result.test.ts index 3dc071c3..6fad8186 100644 --- a/packages/triage-contracts/src/stage-result.test.ts +++ b/packages/triage-contracts/src/stage-result.test.ts @@ -3,16 +3,17 @@ import { type } from "arktype"; import { StageResult } from "./stage-result.js"; const REVIEW = { summary: "One bug.", comments: [{ path: "src/a.ts", start: 3, end: 4, title: "Off by one", body: "The loop skips the last item." }], budgetExhausted: false }; -const VERDICT = { verdict: "pass", reasons: ["Every rule passed."] }; +const VERDICT = { repo: "acme/widgets", number: 8, headSha: "h1", verdict: "pass", checks: [], request: { repo: "acme/widgets", number: 8, comment: "Ready." }, automatic: ["labels"], actions: [] }; test("a stage result is read by its kind, and one whose result is not that kind's output is refused", () => { expect(StageResult({ kind: "code-review", head: "h1", result: REVIEW })).toEqual({ kind: "code-review", head: "h1", result: REVIEW }); expect(StageResult({ kind: "judge", head: "h1", result: VERDICT })).toEqual({ kind: "judge", head: "h1", result: VERDICT }); - expect(StageResult({ kind: "judge", head: "h1", result: { ...VERDICT, judged: { k: "v" }, extra: true } })).toEqual({ kind: "judge", head: "h1", result: { ...VERDICT, judged: { k: "v" } } }); + expect(StageResult({ kind: "judge", head: "h1", result: { ...VERDICT, reasons: ["Every rule passed."], mirror: true } })).toEqual({ kind: "judge", head: "h1", result: { ...VERDICT, reasons: ["Every rule passed."], mirror: true } }); expect(StageResult({ kind: "code-review", head: "h1", failed: "the review did not finish" })).toEqual({ kind: "code-review", head: "h1", failed: "the review did not finish" }); for (const malformed of [ { kind: "code-review", head: "h1", result: VERDICT }, - { kind: "judge", head: "h1", result: { verdict: "maybe" } }, + { kind: "judge", head: "h1", result: { ...VERDICT, verdict: "maybe" } }, + { kind: "judge", head: "h1", result: { verdict: "pass", reasons: ["Every rule passed."] } }, { kind: "code-review", result: REVIEW }, { kind: "lint", head: "h1", result: {} }, ]) { diff --git a/packages/triage-contracts/src/stage-result.ts b/packages/triage-contracts/src/stage-result.ts index 8d60a145..9167fcdf 100644 --- a/packages/triage-contracts/src/stage-result.ts +++ b/packages/triage-contracts/src/stage-result.ts @@ -1,6 +1,7 @@ import { type } from "arktype"; +import { VERDICTS } from "./check-pack.js"; import { CodeReviewOutput } from "./code-review.js"; -import { VerdictDetail } from "./hub-responses.js"; +import { REPO_SOURCES } from "./repo-source.js"; /** The grant on a pull request's manager claim and events that the hub's manager routes require. */ export const MANAGER_STATE_RESOURCE = "triage-manager:state"; @@ -12,11 +13,28 @@ export const MANAGER_RESULT_ACTION = "create"; const Reported = type({ head: "string > 0" }); +/** + * A verdict whole, as pr-triage's evaluate made it, so the manager posts and executes it as the judge reported it: the + * fields its mirror and execute read are checked, and every other is kept. + */ +export const JudgeVerdict = type({ + repo: "string", + number: "number", + headSha: "string | null", + "repoSource?": type.enumerated(...REPO_SOURCES), + verdict: type.enumerated(...VERDICTS), + checks: "object[]", + request: { repo: "string", number: "number" }, + automatic: "string[]", + actions: "object[]", +}); +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. */ export const StageResult = Reported.and({ kind: "'code-review'", result: CodeReviewOutput }) - .or(Reported.and({ kind: "'judge'", result: VerdictDetail })) + .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-judge/README.md b/packages/triage-judge/README.md index 0263343e..75f01b83 100644 --- a/packages/triage-judge/README.md +++ b/packages/triage-judge/README.md @@ -2,8 +2,8 @@ The judge a pull request's manager deploys (`triageJudge`, `triage-judge-`). The manager mails it the rules output for one head. It runs those chunks through System One, then focused's follow-up questions, then evaluate, and reports -the verdict — including the answers a later pass can replay — through the reviewer-result route. It mails the manager -a wake-up and stops, whether or not the judge finished. It posts nothing to the pull request. +the whole verdict, which the manager posts as it is, through the reviewer-result route. It mails the manager a wake-up +and stops, whether or not the judge finished. It posts nothing to the pull request. Only the manager that deployed it may mail it. Its definition declares `create` on `triage-manager:result` and `manage` on its own run. diff --git a/packages/triage-judge/src/actions/report.test.ts b/packages/triage-judge/src/actions/report.test.ts index e05bacb7..3a5453eb 100644 --- a/packages/triage-judge/src/actions/report.test.ts +++ b/packages/triage-judge/src/actions/report.test.ts @@ -9,7 +9,7 @@ const MAIL = { headers: { from: "run_manager@acme.test", to: ["run_judge@acme.test"] }, parts: [{ contentType: "text/plain", text: JSON.stringify({ kind: "judge", tenantId: "tnt_acme", number: 8, checkout: CHECKOUT, rules: RULES, reply: REPLY }) }], }; -const VERDICT = { verdict: "pass", reasons: ["Every rule passed."], judged: { k: "v" }, extra: true }; +const VERDICT = { repo: "acme/widgets", number: 8, headSha: "h1", verdict: "pass", reasons: ["Every rule passed."], checks: [], request: { repo: "acme/widgets", number: 8, comment: "Ready." }, automatic: ["labels"], actions: [] }; /** Each effect runs once against a hub that records its calls and records the result. */ function hubContext() { @@ -32,7 +32,7 @@ test("a manager's judge hands over the rules, reports the verdict through the hu expect(await stop(MAIL, ctx, signal)).toEqual({ stopped: true }); const reported = { source: "github", repo: "acme/widgets", number: 8, kind: "judge", head: "h1" }; expect(calls).toEqual([ - { method: "POST", path: "/api/tenants/tnt_acme/manager/reviewer-result", body: { ...reported, result: { verdict: "pass", reasons: ["Every rule passed."], judged: { k: "v" } } } }, + { method: "POST", path: "/api/tenants/tnt_acme/manager/reviewer-result", body: { ...reported, result: VERDICT } }, { method: "POST", path: "/api/tenants/tnt_acme/manager/reviewer-result", body: { ...reported, failed: "the judge did not finish" } }, { method: "DELETE", path: "/api/tenants/tnt_acme/workflows/runs/run_judge" }, ]); diff --git a/packages/triage-judge/src/actions/report.ts b/packages/triage-judge/src/actions/report.ts index f34a16c7..03b0a269 100644 --- a/packages/triage-judge/src/actions/report.ts +++ b/packages/triage-judge/src/actions/report.ts @@ -1,6 +1,6 @@ import type { EffectContext } from "@intx/workflow"; import { type } from "arktype"; -import { REPO_SOURCES, VerdictDetail } from "@corbits/triage-contracts"; +import { JudgeVerdict, REPO_SOURCES } from "@corbits/triage-contracts"; import type { MailInput } from "@corbits/triage-manager/mail"; import { answered, callHub, endRun, mailOf, tenantPath } from "@corbits/triage-manager/actions/hub"; @@ -34,13 +34,13 @@ const NOT_FINISHED = "the judge did not finish"; /** * Reports the verdict to the pull request's manager as a `StageResult` through the hub, or that the judge did not - * finish; the reply then wakes the manager. The verdict keeps the answers a later pass replays. Posts nothing. + * finish; the reply then wakes the manager. The verdict goes whole, so the manager posts it as it was made. Posts nothing. */ 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 } = judgeMailOf(parsed.payload).mail; - const verdict = VerdictDetail(parsed.evaluate?.output); + const verdict = JudgeVerdict(parsed.evaluate?.output); const outcome = parsed.evaluate?.output === undefined || verdict instanceof type.errors ? { failed: NOT_FINISHED } : { result: verdict }; const result = { source: checkout.source, repo: checkout.repo, number, kind: "judge" as const, head: checkout.sha, ...outcome }; const { recorded } = answered(await callHub(ctx, "report", tenantPath(tenantId, "/manager/reviewer-result"), "POST", result), "report", Recorded); diff --git a/packages/triage-judge/src/triage-judge.test.ts b/packages/triage-judge/src/triage-judge.test.ts index e88d4865..f9bc9a1c 100644 --- a/packages/triage-judge/src/triage-judge.test.ts +++ b/packages/triage-judge/src/triage-judge.test.ts @@ -103,7 +103,7 @@ async function runJudge(rulesOutput: { chunks: unknown[] }) { return { terminalStatus, calls, mailed }; } -test("the judge runs the chunks the manager mailed, reports the verdict and stops, posting nothing", async () => { +test("the judge runs the chunks the manager mailed, reports the whole verdict and stops, posting nothing", async () => { const rulesOutput = await ruled(); expect(rulesOutput.chunks.length).toBeGreaterThan(0); const { terminalStatus, calls, mailed } = await runJudge(rulesOutput); @@ -111,9 +111,7 @@ test("the judge runs the chunks the manager mailed, reports the verdict and stop const posted = calls.find((call) => call.method === "POST"); expect(posted).toMatchObject({ path: `/api/tenants/${TENANT}/manager/reviewer-result`, body: { kind: "judge", head: SHA, source: "github", repo: "acme/widgets", number: 8 } }); if (posted === undefined) throw new Error("the judge posted nothing"); - const result = (posted.body as { result?: { verdict?: string; judged?: Record } }).result; - expect(result?.verdict).toBe("pass"); - expect(Object.keys(result?.judged ?? {}).length).toBeGreaterThan(0); + expect((posted.body as { result?: unknown }).result).toMatchObject({ headSha: SHA, verdict: "pass", request: { header: { head: SHA, verdict: "pass" } }, automatic: [], actions: [] }); expect(calls.some((call) => call.path.includes("github.com"))).toBe(false); expect(mailed).toHaveLength(1); expect(calls.at(-1)).toEqual({ method: "DELETE", path: `/api/tenants/${TENANT}/workflows/runs/${RUN}` }); diff --git a/packages/triage-manager/README.md b/packages/triage-manager/README.md index 11f52042..8224028a 100644 --- a/packages/triage-manager/README.md +++ b/packages/triage-manager/README.md @@ -11,13 +11,16 @@ A workspace's listener and a pull request's manager. steps run on the newest mail the hub recorded for the head. A policy of only deterministic rules posts on that pass. One whose rules need the judge, or that asks for code review, posts once, after each of those has reported or given up. The next pass runs at once while an event is pending, or every 30 seconds while it waits for a reviewer. With a - published judge, a pass deploys [`@corbits/triage-judge`](../triage-judge) and mails it the rules output; the pass - that has its answers runs the verdict. With code review, a pass then deploys the review and mails it the head's - request; the pass that collects its report mirrors the verdict and posts the review together. A reviewer that could - not be mailed, failed or ran past its one-hour deadline leaves the verdict to go up alone; a judge the release does - not publish posts nothing. `done` stops a reviewer this run deployed that is on an older head or past its deadline. - The run's last step is its only end: it stops the run once `done` ends the pull request's claim, take is refused - because another run holds it, or the passes fail or run out. + published judge, a pass deploys [`@corbits/triage-judge`](../triage-judge) once the host's checks have finished and + mails it the rules output; while they run the head waits. The pass that collects its report posts the judge's + verdict as the judge made it while the rules still need the judge, and so does every later pass on that head once + the verdict is decided, deploying no other judge. With code review, a pass then deploys the review and mails it the + head's request; the pass that collects its report mirrors the verdict and posts the review together, and a later + pass on that head posts the stored review again without deploying another. A reviewer that could not be mailed, + failed or ran past its one-hour deadline leaves the verdict to go up alone and is deployed again on the next event; + a judge the release does not publish posts nothing. `done` stops a reviewer this run deployed that is on an older + head or past its deadline. The run's last step is its only end: it stops the run once `done` ends the pull + request's claim, take is refused because another run holds it, or the passes fail or run out. What each deploys next, a published workflow's source asset at one commit and the offerings it runs on (`Release`), is read per host from `src/release.json`, which the portal writes into each published tree and the bundle keeps external, diff --git a/packages/triage-manager/src/actions/index.ts b/packages/triage-manager/src/actions/index.ts index 9f124490..2b5abf9f 100644 --- a/packages/triage-manager/src/actions/index.ts +++ b/packages/triage-manager/src/actions/index.ts @@ -1,6 +1,6 @@ -export { evaluate, execute, facts, mirror, rules, waiting } from "@corbits/triage-workflows/actions/index"; +export { execute, facts, mirror, rules, waiting } from "@corbits/triage-workflows/actions/index"; export { executeApprovals } from "./approvals.js"; export { forward } from "./listener.js"; -export { deployJudge, judgeMailed, judgeScope, recall } from "./judge.js"; +export { deployJudge, judgeMailed, judgeScope, verdict } from "./judge.js"; export { done, stop, take } from "./manager.js"; export { deployReview, postReview, reviewMailed, reviewScope } from "./review.js"; diff --git a/packages/triage-manager/src/actions/judge.ts b/packages/triage-manager/src/actions/judge.ts index e608a35f..9e463f22 100644 --- a/packages/triage-manager/src/actions/judge.ts +++ b/packages/triage-manager/src/actions/judge.ts @@ -1,7 +1,10 @@ import type { EffectContext } from "@intx/workflow"; import { type } from "arktype"; -import { asRecord } from "@corbits/common"; -import { repoSourceOf } from "@corbits/triage-contracts"; +import { JudgeVerdict, repoSourceOf } from "@corbits/triage-contracts"; +import { evaluate, type EvaluateOutput } from "@corbits/triage-workflows/actions/evaluate"; +import type { Item } from "@corbits/triage-workflows/logic/item"; +import { hostChecksPending } from "@corbits/triage-workflows/logic/render"; +import { decided } from "@corbits/triage-workflows/logic/settled"; import { Mailed, type MailInput } from "../mail.js"; import { publishedFor } from "../release.js"; import { deploy, managerMailOf, manageMail } from "./hub.js"; @@ -10,61 +13,68 @@ import { Reviewer, TakeOutput } from "./manager.js"; /** How long the manager waits for a judge it deployed, from its deploy to its report. */ export const JUDGE_DEADLINE_MS = 60 * 60_000; -const RecallInput = type({ facts: { output: { items: "object[]", batch: "boolean" } }, take: { output: TakeOutput } }); +const VerdictInput = type({ payload: "unknown", take: { output: TakeOutput }, rules: { output: { needsJudgment: "boolean", items: "object[]", batch: "boolean" } } }); -function judgedRecord(value: unknown): Record | undefined { - if (!value || typeof value !== "object") return undefined; - const judged = (value as { judged?: unknown }).judged; - if (!judged || typeof judged !== "object") return undefined; - const entries = Object.entries(judged).filter((entry): entry is [string, string] => typeof entry[1] === "string"); - return entries.length > 0 ? Object.fromEntries(entries) : undefined; -} - -/** Answers the judge reported for this head, on this pass or stored on its reviewer. */ -function answersOf({ head, judge, reviewers }: TakeOutput): Record | undefined { - return judgedRecord(judge) ?? reviewers.map((reviewer) => reviewer.kind === "judge" && reviewer.head === head ? judgedRecord(reviewer.result) : undefined).find((judged) => judged !== undefined); +/** Whether an item's rules wait on the host's own checks, so its judge is held until they finish. */ +function waitingOnHost(items: readonly object[]): boolean { + return (items as Item[]).some((item) => item.det !== undefined && hostChecksPending(item.det, item.noWait)); } /** - * Puts the judge's stored answers onto the facts items before rules run, so a head the judge already answered is not - * asked again. An item facts could not read is left as it is. With no answers, the facts output is unchanged. + * The verdict the pass posts. When its rules need the judge and the judge reported a verdict for its head, that + * verdict as the judge made it, after checking that its repository, number, head and host, and its comment's + * repository and number, are the pass's. Otherwise pr-triage's evaluate of the rules output, the judge held back while + * the host's checks run, so the head waits for them. */ -export async function recall(input: unknown): Promise<{ items: object[]; batch: boolean }> { - const parsed = RecallInput(input); - if (parsed instanceof type.errors) throw new Error(`recall: ${parsed.summary}`); - const facts = parsed.facts.output; - const judged = answersOf(parsed.take.output); - if (!judged) return facts; - const items = facts.items.map(function attach(item) { - if ("error" in item && typeof item.error === "string") return item; - const prior = "judged" in item && item.judged && typeof item.judged === "object" ? item.judged as Record : {}; - return { ...item, judged: { ...prior, ...judged } }; - }); - return { items, batch: facts.batch }; +export async function verdict(input: unknown, ctx: EffectContext, signal: AbortSignal): Promise { + const parsed = VerdictInput(input); + if (parsed instanceof type.errors) throw new Error(`verdict: ${parsed.summary}`); + const rules = parsed.rules.output; + const judged = parsed.take.output.verdict; + if (rules.needsJudgment && judged) { + const { pullRequest } = managerMailOf(parsed.payload); + const { repo, number, request } = judged; + const matches = repo === pullRequest.repo && number === pullRequest.number && judged.headSha === parsed.take.output.head + && request.repo === pullRequest.repo && request.number === pullRequest.number && repoSourceOf(judged.repoSource) === repoSourceOf(pullRequest.source); + if (!matches) throw new Error(`verdict: the judge's verdict names ${repo}#${number} at ${judged.headSha}, not the pass's pull request and head`); + return judged; + } + if (rules.needsJudgment && waitingOnHost(rules.items)) return evaluate({ ...rules, items: rules.items.map((item) => ({ ...item, judgeSkipped: true })) }, ctx, signal); + return evaluate(rules, ctx, signal); } -const ScopeInput = type({ payload: "unknown", take: { output: TakeOutput }, rules: { output: "unknown" } }); +const ScopeInput = type({ payload: "unknown", take: { output: TakeOutput }, rules: { output: { needsJudgment: "boolean", items: "object[]" } } }); function ownJudge(reviewers: readonly Reviewer[], head: string | undefined): Reviewer[] { return reviewers.filter((reviewer) => reviewer.kind === "judge" && reviewer.head === head); } +/** A verdict an earlier pass stored that the host's checks could still change. */ +function undecided(reviewer: Reviewer, head: string | undefined): boolean { + const verdict = JudgeVerdict(reviewer.result); + return !(verdict instanceof type.errors) && (head === undefined || !decided(verdict, head)); +} + /** - * Whether the pass deploys the judge, and whether it goes on to the verdict. The judge runs once per head, when the - * rules still need it and the release publishes one. A pass that deploys it, and every pass until it reports, posts - * nothing. One that reported, reported a failure, or ran past its deadline goes on to the verdict and is not deployed again. + * Whether the pass deploys the judge, and whether it goes on to the verdict. The judge runs when the rules still need + * it, the release publishes one and the host's checks have finished; while they run, the pass goes on to a verdict that + * waits for them. A pass that deploys it, and every pass until it reports, posts nothing. One that reported, reported a + * failure, or ran past its deadline goes on to the verdict and is not deployed again; one whose stored verdict the + * host's checks could still change is deployed again. */ export async function judgeScope(input: unknown): Promise<{ deploy: boolean; post: boolean }> { const parsed = ScopeInput(input); if (parsed instanceof type.errors) throw new Error(`judgeScope: ${parsed.summary}`); - if (asRecord(parsed.rules.output)?.["needsJudgment"] !== true) return { deploy: false, post: false }; + if (!parsed.rules.output.needsJudgment) return { deploy: false, post: false }; const { head, judge, judgeFailed, reviewers } = parsed.take.output; if (publishedFor(repoSourceOf(managerMailOf(parsed.payload).pullRequest.source), "judge") === undefined) return { deploy: false, post: false }; const pending = ownJudge(reviewers, head); - const overdue = pending.some((reviewer) => reviewer.result === undefined && Date.parse(reviewer.deadline) <= Date.now()); - const settled = judge !== undefined || judgeFailed !== undefined || pending.some((reviewer) => reviewer.result !== undefined); - if (judgeFailed !== undefined || overdue || settled) return { deploy: false, post: true }; - if (pending.some((reviewer) => reviewer.result === undefined)) return { deploy: false, post: false }; + const open = pending.filter((reviewer) => reviewer.result === undefined); + const overdue = open.some((reviewer) => Date.parse(reviewer.deadline) <= Date.now()); + const settled = judge !== undefined || judgeFailed !== undefined || pending.some((reviewer) => reviewer.result !== undefined && !undecided(reviewer, head)); + if (overdue || settled) return { deploy: false, post: true }; + if (open.length > 0) return { deploy: false, post: false }; + if (waitingOnHost(parsed.rules.output.items)) return { deploy: false, post: true }; return { deploy: true, post: false }; } diff --git a/packages/triage-manager/src/actions/manager.ts b/packages/triage-manager/src/actions/manager.ts index 820ff686..ba336ede 100644 --- a/packages/triage-manager/src/actions/manager.ts +++ b/packages/triage-manager/src/actions/manager.ts @@ -1,6 +1,7 @@ import type { EffectContext } from "@intx/workflow"; import { type, type Type } from "arktype"; -import { APPROVE_DO_KIND, CodeReviewOutput, StageResult, VerdictDetail, WAITING_KIND } from "@corbits/triage-contracts"; +import { APPROVE_DO_KIND, CodeReviewOutput, JudgeVerdict, StageResult, WAITING_KIND } from "@corbits/triage-contracts"; +import { decided } from "@corbits/triage-workflows/logic/settled"; import { answered, callHub, endRun, managerMailOf, tenantPath, type ManagerMail } from "./hub.js"; export const Reviewer = type({ kind: "string > 0", head: "string > 0", runId: "string > 0", deployedBy: "string > 0", deadline: "string", "result?": "unknown" }); @@ -16,7 +17,7 @@ const Done = type({ outcome: "'ended'" }).or({ outcome: "'more'", wait: "boolean * its deadline. `approvals` are the held Dos approved since, each run as pr-triage runs one. `triage` when the head is * triaged: `taken` is the newest pull request mail the hub recorded, or the mail that holds the head for CI when that * is the newest for it. `review` or `reviewFailed`, and `judge` or `judgeFailed`, is what one of its own reviewers - * reported on `head`. + * reported on `head` on this pass; `verdict` is the judge's verdict on `head`, reported on this pass or an earlier one. */ export const TakeOutput = type({ run: "boolean", @@ -27,8 +28,9 @@ export const TakeOutput = type({ "head?": "string", "review?": CodeReviewOutput, "reviewFailed?": "string", - "judge?": VerdictDetail, + "judge?": JudgeVerdict, "judgeFailed?": "string", + "verdict?": JudgeVerdict, overdue: "boolean", reviewers: Reviewer.array(), }); @@ -45,6 +47,9 @@ function isOwn(reviewer: Reviewer, kind: string, head: string): boolean { return reviewer.kind === kind && reviewer.head === head; } +/** A judge whose evaluate could not read the pull request reports a verdict that names no head, which decides nothing. */ +const NO_HEAD = "the judge's verdict names no head"; + /** What one of the manager's own reviewers reported on `head`. */ function reportOf(events: (typeof TakenEvent.infer)[], head: string, reviewers: readonly Reviewer[]): Pick { const reported: Pick = {}; @@ -56,11 +61,22 @@ function reportOf(events: (typeof TakenEvent.infer)[], head: string, reviewers: if ("failed" in result) reported.reviewFailed = result.failed; else reported.review = result.result; } else if ("failed" in result) reported.judgeFailed = result.failed; + else if (result.result.headSha === null) reported.judgeFailed = NO_HEAD; else reported.judge = result.result; } return reported; } +/** The decided verdict a judge of `head` reported on an earlier pass, stored on its reviewer. */ +function storedVerdict(reviewers: readonly Reviewer[], head: string): JudgeVerdict | undefined { + for (const reviewer of reviewers) { + if (reviewer.kind !== "judge" || reviewer.head !== head) continue; + const verdict = JudgeVerdict(reviewer.result); + if (!(verdict instanceof type.errors) && decided(verdict, head)) return verdict; + } + return undefined; +} + /** A reviewer of `kind` on `head` whose deadline passed with no report and no result stored. */ function late(reviewers: readonly Reviewer[], head: string, kind: string, reported: boolean): boolean { return !reported && reviewers.some((reviewer) => isOwn(reviewer, kind, head) && reviewer.result === undefined && Date.parse(reviewer.deadline) <= Date.now()); @@ -84,7 +100,8 @@ export async function take(input: unknown, ctx: EffectContext, _signal: AbortSig // A pass of approvals alone, before any pull request mail was recorded, has nothing to triage. const triage = request !== undefined && (forHead !== undefined || report.review !== undefined || report.reviewFailed !== undefined || report.judge !== undefined || report.judgeFailed !== undefined || overdue); const taken = forHead?.payload["kind"] === WAITING_KIND ? forHead.payload : request; - return { run: answer.outcome === "pass" || overdue, approvals, triage, ...(taken && { taken }), head, ...report, overdue, reviewers }; + const verdict = report.judge ?? storedVerdict(reviewers, head); + return { run: answer.outcome === "pass" || overdue, approvals, triage, ...(taken && { taken }), head, ...report, ...(verdict && { verdict }), overdue, reviewers }; } const DoneInput = type({ @@ -113,14 +130,31 @@ function remembered(take: TakeOutput): Reviewer[] { }); } +/** The most one stored result takes of done's body, which the route caps at 1 MiB and holds a judge's and a review's. */ +const MAX_STORED_BYTES = 256 * 1024; + +function storable(reviewer: Reviewer): boolean { + return reviewer.result === undefined || Buffer.byteLength(JSON.stringify(reviewer.result)) <= MAX_STORED_BYTES; +} + +/** A result a later pass on `head` posts as it is: a decided verdict, or a code review. */ +function lasting(reviewer: Reviewer, head: string): boolean { + if (reviewer.kind === "code-review") return !(CodeReviewOutput(reviewer.result) instanceof type.errors); + const verdict = JudgeVerdict(reviewer.result); + return reviewer.kind === "judge" && !(verdict instanceof type.errors) && decided(verdict, head); +} + /** * The reviewers to wait for. A result stays on its reviewer while another of this head is still out, so a judge that - * reported before code review is not lost; once nothing is waiting, the settled ones are dropped. Plus one mailed now. + * reported before code review is not lost. Once nothing is waiting, only the lasting results of the pass's head stay, so + * a later pass on it posts them and deploys no other; a degraded, undecided, failed or late one is dropped, so the next + * event deploys its reviewer again. A result too large to store is dropped. Plus one mailed now. */ function reviewersAfter(take: TakeOutput, mailed: Reviewer | undefined): Reviewer[] { - let current = remembered(take); + const { head } = take; + let current = remembered(take).filter(storable); const waiting = current.some((reviewer) => reviewer.result === undefined) || (mailed !== undefined && mailed.result === undefined); - if (!waiting) current = current.filter((reviewer) => reviewer.result === undefined); + if (!waiting) current = current.filter((reviewer) => reviewer.result === undefined || (head !== undefined && lasting(reviewer, head))); return mailed ? [...current, mailed] : current; } diff --git a/packages/triage-manager/src/actions/review.ts b/packages/triage-manager/src/actions/review.ts index 5a14b024..d001a420 100644 --- a/packages/triage-manager/src/actions/review.ts +++ b/packages/triage-manager/src/actions/review.ts @@ -1,7 +1,7 @@ import type { EffectContext } from "@intx/workflow"; import { type } from "arktype"; import { asRecord } from "@corbits/common"; -import { repoSourceOf, RULE_GATES, rulesGate } from "@corbits/triage-contracts"; +import { CodeReviewOutput, repoSourceOf, RULE_GATES, rulesGate } from "@corbits/triage-contracts"; import { call, hostWrite, toolName } from "@corbits/triage-workflows/logic/host"; import { codeReviewSummary } from "@corbits/triage-workflows/logic/render"; import { Mailed, type MailInput } from "../mail.js"; @@ -20,19 +20,20 @@ const ScopeInput = type({ payload: "unknown", take: { output: TakeOutput }, eval * triage's rules let it past as its `when.rules` says, the hub read the head's review request and the release publishes * a code review for the host. The pass that deploys * it, and every pass before its review is in, posts nothing, so the verdict and the review go up together; one that - * reported a failure or ran past its deadline posts the verdict alone. + * reported a failure or ran past its deadline posts the verdict alone. A later pass on a head whose review settled posts + * again and deploys no other. */ -export async function reviewScope(input: unknown): Promise<{ deploy: boolean; post: boolean }> { +export async function reviewScope(input: unknown): Promise<{ deploy: boolean; post: boolean; reviewed: boolean }> { const parsed = ScopeInput(input); if (parsed instanceof type.errors) throw new Error(`reviewScope: ${parsed.summary}`); const { taken, head, review, reviewFailed, reviewers } = parsed.take.output; const part = CodeReviewPart(taken?.["codeReview"]); const reviewed = !(part instanceof type.errors) && part.on && rulesGate(part.rules, asRecord(parsed.evaluateRules.output)?.["checks"]).open; const published = publishedFor(repoSourceOf(managerMailOf(parsed.payload).pullRequest.source), "codeReview") !== undefined; - const reviewLate = reviewers.some((reviewer) => reviewer.kind === "code-review" && reviewer.head === head && reviewer.result === undefined && Date.parse(reviewer.deadline) <= Date.now()); - if (!reviewed || !published || review || reviewFailed || reviewLate || taken?.["codeReviewRequest"] === undefined) return { deploy: false, post: true }; - const pending = reviewers.some((reviewer) => reviewer.kind === "code-review" && reviewer.head === head); - return { deploy: !pending, post: false }; + const own = reviewers.filter((reviewer) => reviewer.kind === "code-review" && reviewer.head === head); + const settled = own.some((reviewer) => reviewer.result !== undefined || Date.parse(reviewer.deadline) <= Date.now()); + if (!reviewed || !published || review || reviewFailed || settled || taken?.["codeReviewRequest"] === undefined) return { deploy: false, post: true, reviewed }; + return { deploy: own.length === 0, post: false, reviewed }; } const DeployInput = type({ payload: "unknown", take: { output: TakeOutput } }); @@ -69,14 +70,29 @@ export async function reviewMailed(input: unknown): Promise<{ reviewer?: Reviewe return mailed.mailed ? { reviewer: parsed.deployReview.output.reviewer, post: false } : { post: true }; } -const PostInput = type({ payload: "unknown", take: { output: TakeOutput } }); +const PostInput = type({ payload: "unknown", take: { output: TakeOutput }, reviewScope: { output: { reviewed: "boolean" } } }); -/** Posts the code review the pass collected on its head, after the verdict; the host writes nothing for a review already there. */ +/** The review a code review of `head` reported on an earlier pass, stored on its reviewer. */ +function storedReview(reviewers: readonly Reviewer[], head: string): CodeReviewOutput | undefined { + for (const reviewer of reviewers) { + if (reviewer.kind !== "code-review" || reviewer.head !== head) continue; + const review = CodeReviewOutput(reviewer.result); + if (!(review instanceof type.errors)) return review; + } + return undefined; +} + +/** + * Posts the code review reported on its head after the verdict: this pass's, or one an earlier pass stored when this + * pass's rules still let code review past. The host writes nothing for a review already there. + */ export async function postReview(input: unknown, ctx: EffectContext, _signal: AbortSignal): Promise<{ posted: boolean }> { const parsed = PostInput(input); if (parsed instanceof type.errors) throw new Error(`postReview: ${parsed.summary}`); - const { review, head, taken } = parsed.take.output; - if (!review || head === undefined || !taken) return { posted: false }; + const { head, taken, reviewers } = parsed.take.output; + if (head === undefined || !taken) return { posted: false }; + const review = parsed.take.output.review ?? (parsed.reviewScope.output.reviewed ? storedReview(reviewers, head) : undefined); + if (!review) return { posted: false }; const summary = codeReviewSummary(review); if (!summary && review.comments.length === 0) return { posted: false }; const { pullRequest } = managerMailOf(parsed.payload); diff --git a/packages/triage-manager/src/triage-manager.test.ts b/packages/triage-manager/src/triage-manager.test.ts index 017e5e8d..1e66d7f5 100644 --- a/packages/triage-manager/src/triage-manager.test.ts +++ b/packages/triage-manager/src/triage-manager.test.ts @@ -7,7 +7,8 @@ import { runLocal, type ActionHandler, type LoopFn, type OnTriggerPrimitive, typ import { ACTION_KINDS, APPROVE_DO_KIND, emptyPack, WAITING_KIND, WAITING_LABEL, type ActionKind, type CheckPack } from "@corbits/triage-contracts"; import type { Snapshot } from "@corbits/triage-workflows/evals/cases"; import { fixtureGithub, triggerMail } from "@corbits/triage-workflows/evals/serve"; -import { judgeKey } from "@corbits/triage-workflows/logic/quality"; +import { evaluate } from "@corbits/triage-workflows/actions/evaluate"; +import { parts } from "@corbits/triage-workflows/actions/parts"; import { delivered, grantsFor, type Bindings, type Fetch } from "@corbits/triage-workflows/local-credentials"; import { recordingGithub, type Write } from "@corbits/triage-workflows/run-local"; import * as actions from "./actions/index.js"; @@ -144,22 +145,26 @@ function wake(pull: { source: string; repo: string; number: number } = PULL) { return { headers: { from: "run_listener@triage.test", to: [`${RUN}@triage.test`] }, parts: [{ contentType: "text/plain", text: manageMail(TENANT, pull) }] }; } -/** The eval fixture's pull request at `head`, with the base and head repositories a comment's write compares. */ -function githubHost(head = HEAD) { +/** + * The eval fixture's pull request, or `pr`, at `head`, with the base and head repositories a comment's write compares, + * `pull`'s fields over the host's own, and `checkRuns` as the head's CI. + */ +function githubHost(head = HEAD, { pr = PR, pull = {}, checkRuns }: { pr?: Snapshot; pull?: Record; checkRuns?: object[] } = {}) { const writes: Write[] = []; - const fixture = fixtureGithub(PR, {}); + const fixture = fixtureGithub(pr, {}); async function read(url: URL): Promise { + if (checkRuns && url.pathname.endsWith("/check-runs")) return Response.json({ check_runs: checkRuns }); const response = await fixture(url); if (url.pathname !== `/repos/${PR.repo}/pulls/${PR.number}`) return response; - const pull = await response.json(); - return Response.json({ ...pull, head: { ...pull.head, sha: head, repo: { id: 1 } }, base: { ...pull.base, repo: { id: 1 } } }); + const read = await response.json(); + return Response.json({ ...read, ...pull, head: { ...read.head, sha: head, repo: { id: 1 } }, base: { ...read.base, repo: { id: 1 } } }); } return { writes, host: recordingGithub(read, writes) }; } const TAKE = { method: "POST", path: `${TENANT_PATH}/manager/take`, body: PULL }; const DONE = { method: "POST", path: `${TENANT_PATH}/manager/done`, body: { ...PULL, reviewers: [] } }; -const STAGE_1 = ["approvals", "done", "evaluateRules", "executeRules", "facts", "held", "mirrorRules", "paused", "postReview", "posting", "ran", "recall", "reviewScope", "reviewing", "route", "rules", "take", "triaged", "unready", "waiting"]; +const STAGE_1 = ["approvals", "done", "evaluateRules", "executeRules", "facts", "held", "mirrorRules", "paused", "postReview", "posting", "ran", "reviewScope", "reviewing", "route", "rules", "take", "triaged", "unready", "waiting"]; /** Publishes `release` as the release.json the portal writes beside the workflows; the repository's own holds none. */ function publish(release: Record): void { @@ -226,7 +231,7 @@ describe("triage-manager", () => { const { writes, host } = githubHost(); const hub = fakeHub({ takes: [{ ...PASS, request: hubRequest(JUDGED) }], dones: [ENDED] }); const { ran } = await runMail(github, wake(), host, hub.hub); - expect(ran).toEqual(["approvals", "done", "facts", "held", "judgeScope", "judgeWait", "judging", "paused", "ran", "recall", "route", "rules", "take", "triaged", "unready", "waiting"]); + expect(ran).toEqual(["approvals", "done", "facts", "held", "judgeScope", "judgeWait", "judging", "paused", "ran", "route", "rules", "take", "triaged", "unready", "waiting"]); expect(writes).toEqual([]); }); @@ -333,10 +338,32 @@ describe("triage-manager with code review", () => { expect(ran).not.toContain("deployReview"); expect(writes).toContainEqual(READY); expect(writes).toContainEqual(POSTED_REVIEW); - expect(second.calls).toEqual([TAKE, DONE, END]); + expect(second.calls).toEqual([TAKE, { ...DONE, body: { ...PULL, reviewers: [{ ...reviewer(), result: REVIEW }] } }, END]); expect(mailed).toHaveLength(1); }); + test("a later event on the same head deploys no second review and posts the stored review with the verdict", async () => { + const { writes, host } = githubHost(); + const mailed: unknown[] = []; + const later = { ...PASS, request: ON, events: [mailEvent(HEAD, ON, "evt_2")], reviewers: [{ ...reviewer(), result: REVIEW }] }; + const hub = fakeHub({ takes: [later], dones: [ENDED] }); + const { ran } = await runMail(reviewed, wake(), host, hub.hub, mailed); + expect(ran).not.toContain("deployReview"); + expect(writes).toContainEqual(READY); + expect(writes).toContainEqual(POSTED_REVIEW); + expect(mailed).toEqual([]); + expect(hub.calls).toEqual([TAKE, { ...DONE, body: { ...PULL, reviewers: [{ ...reviewer(), result: REVIEW }] } }, END]); + }); + + test("a stored review is not posted once this pass's policy no longer asks for code review", async () => { + const { writes, host } = githubHost(); + const hub = fakeHub({ takes: [{ ...PASS, reviewers: [{ ...reviewer(), result: REVIEW }] }], dones: [ENDED] }); + const { ran } = await runMail(reviewed, wake(), host, hub.hub); + expect(ran).toContain("postReview"); + expect(writes).toContainEqual(READY); + expect(writes).not.toContainEqual(POSTED_REVIEW); + }); + test.each([ ["the review reported a failure", reported({ failed: "the review did not finish" }, ON), [TAKE, DONE, END]], ["the review ran past its deadline, which is stopped,", { ...IDLE, request: ON, reviewers: [reviewer("2000-01-01T00:00:00.000Z")] }, [TAKE, DONE, STOP_REVIEW, END]], @@ -362,7 +389,7 @@ describe("triage-manager with code review", () => { }); }); -/** A decisive answer for one question id, reusable by a later rules run of the same chunk. */ +/** A decisive answer for one question id. */ function answerFor(id: string): string { if (id === "instructed") return JSON.stringify({ id, type: "noul", noul: 0.1 }); if (id === "verdict") return JSON.stringify({ id, type: "choice", choice: "pass", probabilities: { pass: 0.9, fail: 0.05, cant_tell: 0.05 }, confidence: 0.9 }); @@ -370,21 +397,36 @@ function answerFor(id: string): string { return JSON.stringify({ id, type: "choice", choice: "serves_purpose", probabilities: { serves_purpose: 0.9, separate_change: 0.05, cant_tell: 0.05 }, confidence: 0.9 }); } -function judgedFrom(rules: { chunks?: { state: unknown; questions: { id: string }[] }[] }): Record { - const judged: Record = {}; - for (const chunk of rules.chunks ?? []) judged[judgeKey(chunk)] = chunk.questions.map((question) => answerFor(question.id)).join(""); - return judged; +type MailedRules = { items: object[]; batch: boolean; chunks: { questions: { id: string }[] }[] }; + +const JUDGE_COMMENT = "The judge's own words."; + +function answered(chunks: { questions: { id: string }[] }[]) { + return chunks.map((chunk) => ({ reply: JSON.stringify({ judge: chunk.questions.map((question) => answerFor(question.id)).join("") }) })); } -/** A pass on which the manager's judge reported `judged` for this head. */ -function judgeReported(judged: Record, request = hubRequest(JUDGED)) { - return { ...PASS, request, events: [{ key: `evt:github:${PR.repo}#8:${DEPLOYED}`, event: { type: "reviewer-result", head: HEAD, payload: { kind: "judge", result: { verdict: "pass", reasons: ["Every rule passed."], judged } } } }], reviewers: [{ kind: "judge", head: HEAD, runId: DEPLOYED, deployedBy: RUN, deadline: LATER }] }; +/** The verdict the judge reports on `rules`, as its steps make it from decisive answers, worded so no verdict the manager makes matches it. */ +async function judgeVerdict(rules: MailedRules): Promise> { + const signal = new AbortController().signal; + const judged = { ...rules, judge: { output: answered(rules.chunks) } }; + const { chunks } = await parts(judged, {} as never, signal) as { chunks: MailedRules["chunks"] }; + const made = await evaluate({ ...judged, judgeParts: { output: answered(chunks) } }, {} as never, signal) as { request: Record }; + return { ...made, feedback: JUDGE_COMMENT, request: { ...made.request, comment: JUDGE_COMMENT } }; } +const JUDGE = { kind: "judge", head: HEAD, runId: DEPLOYED, deployedBy: RUN, deadline: LATER }; + +/** A pass on which the manager's judge reported `verdict` for this head. */ +function judgeReported(verdict: Record, request = hubRequest(JUDGED)) { + return { ...PASS, request, events: [{ key: `evt:github:${PR.repo}#8:${DEPLOYED}`, event: { type: "reviewer-result", head: HEAD, payload: { kind: "judge", result: verdict } } }], reviewers: [JUDGE] }; +} + +const JUDGE_POSTED = expect.objectContaining({ method: "POST", path: `/repos/${PR.repo}/issues/${PR.number}/comments`, body: { body: expect.stringContaining(JUDGE_COMMENT) } }); + describe("triage-manager with a judge", () => { beforeEach(() => publish({ github: { judge: JUDGE_RELEASE } })); - test("the first pass deploys the judge and mails it the rules, posting nothing; the pass that collects its answers posts the verdict", async () => { + test("the first pass deploys the judge and mails it the rules, posting nothing; the pass that collects its report posts its verdict as reported and keeps it on the head", async () => { const { writes, host } = githubHost(); const mailed: unknown[] = []; const first = fakeHub({ takes: [{ ...PASS, request: hubRequest(JUDGED) }], dones: [ENDED] }); @@ -393,61 +435,153 @@ describe("triage-manager with a judge", () => { expect(first.calls[1]).toMatchObject({ ...DEPLOY, body: { source: { kind: "asset", assetId: "ast_judge", package: { format: "source", commitSha: "c0ffee", packageName: JUDGE_RELEASE.workflow.packageName } }, entry: JUDGE_RELEASE.workflow.entry, sourceOfferingIds: ["off_d"], defaultSourceOfferingId: "off_d" } }); expect(first.calls[2]).toMatchObject({ ...DONE, body: { reviewers: [{ kind: "judge", head: HEAD, runId: DEPLOYED, deployedBy: RUN }] } }); const [mail] = mailed as [{ tenantId: string; to: string; content: string; readyBy: string }]; - const content = JSON.parse(mail.content) as { kind: string; rules: { chunks: { state: unknown; questions: { id: string }[] }[] }; reply: { to: string } }; + const content = JSON.parse(mail.content) as { kind: string; rules: MailedRules; reply: { to: string } }; expect(content.kind).toBe("judge"); expect(content.rules.chunks.length).toBeGreaterThan(0); expect(content.reply.to).toBe(RUN); expect(Date.parse(mail.readyBy)).toBeGreaterThan(Date.now()); - const second = fakeHub({ takes: [judgeReported(judgedFrom(content.rules))], dones: [ENDED] }); - const { ran } = await runMail(github, wake(), host, second.hub, mailed); - expect(ran).toContain("mirrorRules"); + const verdict = await judgeVerdict(content.rules); + const second = fakeHub({ takes: [judgeReported(verdict)], dones: [ENDED] }); + const { ran, outputs } = await runMail(github, wake(), host, second.hub, mailed); expect(ran).not.toContain("deployJudge"); - expect(writes.length).toBeGreaterThan(0); - expect(second.calls[1]).toEqual(DONE); + expect(outputs["evaluateRules"]).toEqual(verdict); + expect(writes).toContainEqual(JUDGE_POSTED); + expect(second.calls[1]).toEqual({ ...DONE, body: { ...PULL, reviewers: [{ ...JUDGE, result: verdict }] } }); expect(mailed).toHaveLength(1); }); + test("a later event on the same head deploys no second judge and posts the judge's verdict, whatever changed on the pull request since", async () => { + const verdict = await judgeVerdict(JSON.parse((await firstJudgeMail()).content).rules); + const { writes, host } = githubHost(HEAD, { pr: { ...PR, title: "Renamed since", body: "", bodyHtml: "" } }); + const later = { ...PASS, request: hubRequest(JUDGED), events: [mailEvent(HEAD, hubRequest(JUDGED), "evt_2")], reviewers: [{ ...JUDGE, result: verdict }] }; + const hub = fakeHub({ takes: [later], dones: [ENDED] }); + const { ran, outputs } = await runMail(github, wake(), host, hub.hub); + expect(ran).not.toContain("deployJudge"); + expect(outputs["evaluateRules"]).toEqual(verdict); + expect(writes).toContainEqual(JUDGE_POSTED); + expect(hub.calls).toEqual([TAKE, { ...DONE, body: { ...PULL, reviewers: [{ ...JUDGE, result: verdict }] } }, END]); + }); + + test("a stored verdict gives way when this pass's rules no longer need the judge, as on a draft", async () => { + const verdict = await judgeVerdict(JSON.parse((await firstJudgeMail()).content).rules); + const { writes, host } = githubHost(HEAD, { pull: { draft: true } }); + const hub = fakeHub({ takes: [{ ...PASS, request: hubRequest(JUDGED), reviewers: [{ ...JUDGE, result: verdict }] }], dones: [ENDED] }); + const { ran, outputs } = await runMail(github, wake(), host, hub.hub); + expect(ran).not.toContain("judgeScope"); + expect(outputs["evaluateRules"]).not.toEqual(verdict); + expect(writes).not.toContainEqual(JUDGE_POSTED); + }); + + test("a judge's verdict that names no head goes up as a failed judge, and the pass still ends", async () => { + const verdict = await judgeVerdict(JSON.parse((await firstJudgeMail()).content).rules); + const { writes, host } = githubHost(); + const hub = fakeHub({ takes: [judgeReported({ ...verdict, headSha: null })], dones: [ENDED] }); + const { terminalStatus, ran } = await runMail(github, wake(), host, hub.hub); + expect(terminalStatus).toBe("completed"); + expect(ran).toContain("mirrorRules"); + expect(writes).not.toContainEqual(JUDGE_POSTED); + expect(hub.calls).toEqual([TAKE, DONE, END]); + }); + test.each([ - ["the judge reported a failure", { ...PASS, request: hubRequest(JUDGED), events: [{ key: `evt:github:${PR.repo}#8:${DEPLOYED}`, event: { type: "reviewer-result", head: HEAD, payload: { kind: "judge", failed: "the judge did not finish" } } }], reviewers: [{ kind: "judge", head: HEAD, runId: DEPLOYED, deployedBy: RUN, deadline: LATER }] }, [TAKE, DONE, END]], - ["the judge ran past its deadline, which is stopped,", { ...PASS, request: hubRequest(JUDGED), reviewers: [{ kind: "judge", head: HEAD, runId: DEPLOYED, deployedBy: RUN, deadline: "2000-01-01T00:00:00.000Z" }] }, [TAKE, DONE, STOP_REVIEW, END]], - ])("when %s the machine verdict goes up and the judge is not deployed again", async (_name, taken, calls) => { + ["an approve skipped until CI passes", { kind: "approve", list: "pass", index: 0, skipped: true, reason: "CI has not passed on this head." }], + ["a merge held for failing CI", { kind: "merge", list: "pass", index: 0, effectId: "do:merge", target: {}, held: true, reason: "CI has failed on this head." }], + ])("a stored verdict with %s is judged again, since CI on the same head can still change it", async (_name, action) => { + const verdict = await judgeVerdict(JSON.parse((await firstJudgeMail()).content).rules); + const { host } = githubHost(); + const hub = fakeHub({ takes: [{ ...PASS, request: hubRequest(JUDGED), reviewers: [{ ...JUDGE, result: { ...verdict, actions: [action] } }] }], dones: [ENDED] }); + expect((await runMail(github, wake(), host, hub.hub)).ran).toContain("deployJudge"); + }); + + test("a judge's verdict for another pull request fails the pass instead of being posted", async () => { + const verdict = await judgeVerdict(JSON.parse((await firstJudgeMail()).content).rules); + const { writes, host } = githubHost(); + const hub = fakeHub({ takes: [judgeReported({ ...verdict, number: 9 })] }); + expect((await runMail(github, wake(), host, hub.hub)).terminalStatus).toBe("failed"); + expect(writes).toEqual([]); + }); + + test("a verdict too large to store is posted and dropped from the claim", async () => { + const verdict = { ...await judgeVerdict(JSON.parse((await firstJudgeMail()).content).rules), padding: "x".repeat(300 * 1024) }; + const { writes, host } = githubHost(); + const hub = fakeHub({ takes: [judgeReported(verdict)], dones: [ENDED] }); + await runMail(github, wake(), host, hub.hub); + expect(writes).toContainEqual(JUDGE_POSTED); + expect(hub.calls[1]).toEqual(DONE); + }); + + test("while CI runs the judge waits and the head is marked waiting; a verdict CI could still change is judged again once CI finished, and its Dos run", async () => { + const pack: CheckPack = { ...JUDGED, checks: { ...JUDGED.checks, "ci-green": { enabled: true } }, actions: { ...JUDGED.actions, pass: [{ do: "comment", target: { body: "Welcome" } }] } }; + const request = hubRequest(pack); + const running = githubHost(HEAD, { checkRuns: [{ name: "ci", status: "in_progress", conclusion: null }] }); + const waiting = fakeHub({ takes: [{ ...PASS, request }], dones: [ENDED] }); + expect((await runMail(github, wake(), running.host, waiting.hub)).ran).not.toContain("deployJudge"); + expect(running.writes).toContainEqual(expect.objectContaining({ path: `/repos/${PR.repo}/issues/${PR.number}/labels`, body: { labels: [WAITING_LABEL] } })); + + const passed = [{ name: "ci", status: "completed", conclusion: "success" }]; + const mailed: unknown[] = []; + const { writes, host } = githubHost(HEAD, { checkRuns: passed }); + const undecided = { ...await judgeVerdict(JSON.parse((await firstJudgeMail(request)).content).rules), waiting: true }; + const again = fakeHub({ takes: [{ ...PASS, request, reviewers: [{ ...JUDGE, result: undecided }] }], dones: [ENDED] }); + expect((await runMail(github, wake(), host, again.hub, mailed)).ran).toContain("deployJudge"); + expect(writes).toEqual([]); + + const verdict = await judgeVerdict(JSON.parse((mailed[0] as { content: string }).content).rules); + const judged = fakeHub({ takes: [judgeReported(verdict, request)], dones: [ENDED] }); + await runMail(github, wake(), host, judged.hub); + expect(writes).toContainEqual(JUDGE_POSTED); + expect(writes).toContainEqual(expect.objectContaining({ path: `/repos/${PR.repo}/issues/${PR.number}/comments`, body: { body: expect.stringContaining("Welcome") } })); + }); + + test.each([ + ["the judge reported a failure", { ...PASS, request: hubRequest(JUDGED), events: [{ key: `evt:github:${PR.repo}#8:${DEPLOYED}`, event: { type: "reviewer-result", head: HEAD, payload: { kind: "judge", failed: "the judge did not finish" } } }], reviewers: [JUDGE] }, [TAKE, DONE, END]], + ["the judge ran past its deadline, which is stopped,", { ...PASS, request: hubRequest(JUDGED), reviewers: [{ ...JUDGE, deadline: "2000-01-01T00:00:00.000Z" }] }, [TAKE, DONE, STOP_REVIEW, END]], + ])("when %s the machine verdict goes up, the judge is not deployed again on that pass and the next event deploys it", async (_name, taken, calls) => { const { writes, host } = githubHost(); const hub = fakeHub({ takes: [taken], dones: [ENDED] }); const { ran } = await runMail(github, wake(), host, hub.hub); expect(ran).toContain("mirrorRules"); expect(ran).not.toContain("deployJudge"); expect(writes.length).toBeGreaterThan(0); + expect(writes).not.toContainEqual(JUDGE_POSTED); expect(hub.calls).toEqual(calls); }); }); +/** The mail a first pass sends the judge it deploys. */ +async function firstJudgeMail(request = hubRequest(JUDGED)): Promise<{ content: string }> { + const mailed: unknown[] = []; + await runMail(github, wake(), githubHost().host, fakeHub({ takes: [{ ...PASS, request }], dones: [ENDED] }).hub, mailed); + return mailed[0] as { content: string }; +} + describe("triage-manager with a judge and code review", () => { beforeEach(() => publish({ github: { judge: JUDGE_RELEASE, codeReview: RELEASE } })); - test("posts the verdict and the review together once both have reported", async () => { + test("posts the judge's verdict and the review together once both have reported", async () => { const { writes, host } = githubHost(); const mailed: unknown[] = []; const request = hubRequest(JUDGED, { ...CODE_REVIEW, ...REVIEW_REQUEST }); const first = fakeHub({ takes: [{ ...PASS, request }], dones: [{ outcome: "more", wait: true }] }); expect((await runMail(github, wake(), host, first.hub, mailed)).ran).toContain("doneJudging"); expect(writes).toEqual([]); - const rules = JSON.parse((mailed[0] as { content: string }).content).rules as { chunks: { state: unknown; questions: { id: string }[] }[] }; + const verdict = await judgeVerdict(JSON.parse((mailed[0] as { content: string }).content).rules); - const second = fakeHub({ takes: [judgeReported(judgedFrom(rules), request)], dones: [{ outcome: "more", wait: true }] }); + const second = fakeHub({ takes: [judgeReported(verdict, request)], dones: [{ outcome: "more", wait: true }] }); const again = await runMail(github, wake(), host, second.hub, mailed); expect(again.ran).toContain("deployReview"); expect(again.ran).not.toContain("mirrorRules"); expect(writes).toEqual([]); - expect(second.calls[2]).toMatchObject({ body: { reviewers: expect.arrayContaining([expect.objectContaining({ kind: "judge", result: expect.anything() }), expect.objectContaining({ kind: "code-review", runId: DEPLOYED })]) } }); + expect(second.calls[2]).toMatchObject({ body: { reviewers: [{ ...JUDGE, result: verdict }, expect.objectContaining({ kind: "code-review", runId: DEPLOYED })] } }); - const judged = judgedFrom(rules); - const third = fakeHub({ takes: [{ ...reported({ result: REVIEW }, request), reviewers: [{ kind: "judge", head: HEAD, runId: DEPLOYED, deployedBy: RUN, deadline: LATER, result: { verdict: "pass", reasons: ["Every rule passed."], judged } }, reviewer()] }], dones: [ENDED] }); + const third = fakeHub({ takes: [{ ...reported({ result: REVIEW }, request), reviewers: [{ ...JUDGE, result: verdict }, reviewer()] }], dones: [ENDED] }); const { ran } = await runMail(github, wake(), host, third.hub, mailed); expect(ran).toContain("postReview"); expect(ran).not.toContain("deployJudge"); + expect(writes).toContainEqual(JUDGE_POSTED); expect(writes).toContainEqual(POSTED_REVIEW); - expect(third.calls[1]).toEqual(DONE); + expect(third.calls[1]).toEqual({ ...DONE, body: { ...PULL, reviewers: [{ ...JUDGE, result: verdict }, { ...reviewer(), result: REVIEW }] } }); }); }); diff --git a/packages/triage-manager/src/triage-manager.ts b/packages/triage-manager/src/triage-manager.ts index 5e94c8a5..13ac130a 100644 --- a/packages/triage-manager/src/triage-manager.ts +++ b/packages/triage-manager/src/triage-manager.ts @@ -50,15 +50,16 @@ function reviewSteps(sources: Sources): Record { // A pass that mailed its review records it as a reviewer; a skipped step is missing from every selector, so this branch has its own done. doneReviewing: hubAction("done", { merge: [MAIL, stepsOf("take", "reviewMailed")] }, ["reviewSent"]), posting: gate({ when: { from: "steps.reviewScope.output.post" }, then: "mirrorRules", else: "done", after: ["reviewing"] }), - postReview: action({ handler: "postReview", input: { merge: [MAIL, stepsOf("take")] }, after: ["mirrorRules"], credentials: sources, effect: { requires: [WRITE_PULL_REQUEST] } }), + postReview: action({ handler: "postReview", input: { merge: [MAIL, stepsOf("take", "reviewScope")] }, after: ["mirrorRules"], credentials: sources, effect: { requires: [WRITE_PULL_REQUEST] } }), }; } /** - * The judge between the rules and the verdict. A pass that deploys it mails the rules output and posts nothing. The - * pass that has its answers runs the verdict, and code review when the policy asks for it, still posting nothing while - * a review is out. A judge the release does not publish posts nothing. One that failed, could not be mailed, or ran - * past its deadline goes on to the verdict and is not deployed again. + * The judge between the rules and the verdict. A pass that deploys it mails the rules output and posts nothing; while + * the host's checks run it is not deployed, and the verdict waits for them. The pass that has the judge's verdict posts + * it, and code review when the policy asks for it, still posting nothing while a review is out. A judge the release + * does not publish posts nothing. One that failed, could not be mailed, or ran past its deadline goes on to the + * machine verdict and is not deployed again on that pass. */ function judgeSteps(): Record { return { @@ -81,7 +82,7 @@ function judgeSteps(): Record { * posts once both have reported or given up. A pass after one that waits for a reviewer first waits `POLL_MS`. */ function passOf(id: string, sources: Sources) { - const { waiting, held, markWaiting, unready, rules, evaluateRules, mirrorRules, executeRules } = triageSteps(sources); + const { waiting, held, markWaiting, unready, rules, mirrorRules, executeRules } = triageSteps(sources); return defineWorkflow({ id: `${id}-pass`, trigger: { type: "manual" }, @@ -98,11 +99,10 @@ function passOf(id: string, sources: Sources) { markWaiting, unready, facts: hostAction("facts", sources, TAKEN, ["unready"]), - recall: action({ handler: "recall", input: { merge: [stepsOf("facts", "take")] }, after: ["facts"] }), - rules: { ...rules, input: { from: "steps.recall.output" }, after: ["recall"] }, + rules, route: gate({ when: { from: "steps.rules.output.needsJudgment" }, then: "judgeScope", else: "evaluateRules", after: ["rules"] }), ...judgeSteps(), - evaluateRules: { ...evaluateRules, after: ["route", "judgeWait", "judgeSent"] }, + evaluateRules: action({ handler: "verdict", input: { merge: [MAIL, stepsOf("take", "rules")] }, after: ["route", "judgeWait", "judgeSent"] }), ...reviewSteps(sources), mirrorRules: { ...mirrorRules, after: ["posting", "reviewSent"] }, // The verdict's automatic Dos run as pr-triage runs them, after the verdict and any code review are up. diff --git a/packages/triage-workflows/src/logic/actions.ts b/packages/triage-workflows/src/logic/actions.ts index 8918d857..5aee3dc0 100644 --- a/packages/triage-workflows/src/logic/actions.ts +++ b/packages/triage-workflows/src/logic/actions.ts @@ -230,9 +230,12 @@ export function evaluateActions(input: ActionInput): SuggestedAction[] { * Why an approval cannot stand on this head, if it cannot: it says the pull request may merge, so the verdict must pass * and CI must have finished green. A head without CI cannot be told apart from one whose CI has not started yet, so it is not approved either. */ +/** Why an approve Do is skipped on a passing verdict whose head's CI has not passed. */ +export const CI_NOT_PASSED = "CI has not passed on this head."; + export function approvalHeldBy(verdict: VerdictWord, ci: PrFacts["checks"]): string | undefined { if (verdict !== "pass") return "The pull request does not pass its policy."; - if (ci !== "success") return "CI has not passed on this head."; + if (ci !== "success") return CI_NOT_PASSED; return undefined; } diff --git a/packages/triage-workflows/src/logic/item.ts b/packages/triage-workflows/src/logic/item.ts index 8d9b597b..c654d1e3 100644 --- a/packages/triage-workflows/src/logic/item.ts +++ b/packages/triage-workflows/src/logic/item.ts @@ -10,7 +10,10 @@ export interface Item { judgeError?: string; /** The quality evaluation exceeds the System One budget, so the judge is not asked and a human decides. */ judgeLimitExceeded?: true; - /** A preview does not run the judge, so the checks it would answer wait on it and a human decides. */ + /** + * A preview, or a manager holding its judge until the host's checks finish, does not run the judge: the checks it would + * answer wait on it, and the head waits for those checks or a human decides. + */ judgeSkipped?: true; judged?: Judged; automatic?: ActionKind[]; diff --git a/packages/triage-workflows/src/logic/render.ts b/packages/triage-workflows/src/logic/render.ts index e2289be7..4a0edff3 100644 --- a/packages/triage-workflows/src/logic/render.ts +++ b/packages/triage-workflows/src/logic/render.ts @@ -535,6 +535,11 @@ function hostPending(c: CheckResult): boolean { return unsettledMachine(c) && c.output.kind === "could-not-check"; } +/** Whether a rule waits on the host's own checks, which the hub waits for unless it waits no longer (`noWait`). */ +export function hostChecksPending(det: Pick, noWait: boolean | undefined): boolean { + return !noWait && det.checks.some(hostPending); +} + /** A labelling rule's result that labels and notes the pull request. */ export function isLabelledFinding(c: CheckResult): boolean { return c.result === "fail" || unsure(c); @@ -745,6 +750,8 @@ export function renderVerdict(input: RenderInput): RenderOutput { const unconfirmed = checks.filter((check) => check.result === "unconfirmed"); const waitsOnHost = !noWait && unconfirmed.length > 0 && unconfirmed.every(hostPending); if (evaluated.instructed) return unsureOf(INSTRUCTED_REASON); + // A judge held back for the host's checks has not decided anything yet, so the head waits for them. + if (judgeSkipped === true && hostChecksPending(det, noWait)) return waitingOf(null); if (!decidesByModel(det)) { if (det.checks.every((c) => c.result === "pass")) return withChecks({ ...render("ready-monitoring"), verdict: "pass", degraded: null, reason: RULES_PASS_REASON }, checks, null, reasons, ctx); if (waitsOnHost) return waitingOf(null); diff --git a/packages/triage-workflows/src/logic/settled.ts b/packages/triage-workflows/src/logic/settled.ts index 320079c2..8d32f511 100644 --- a/packages/triage-workflows/src/logic/settled.ts +++ b/packages/triage-workflows/src/logic/settled.ts @@ -1,6 +1,8 @@ import { isRecord } from "@corbits/common"; +import { ciRefusal } from "@corbits/github-reads"; import { hardFailures, NOT_A_DRAFT } from "@corbits/triage-contracts"; import type { CheckResult } from "./checks.js"; +import { CI_NOT_PASSED } from "./actions.js"; import { QUALITY_EVALUATION_LIMIT_ERROR } from "./quality.js"; import { NEEDS_JUDGE_REASON, NOT_ASKED_REASON } from "./render.js"; @@ -68,3 +70,18 @@ function modelUnanswered(verdict: VerdictFields, checks: Array = new Set([...(["failure", "pending", "cancelled", "none"] as const).map((ci) => `${ciRefusal(ci)}.`), CI_NOT_PASSED]); + +/** + * A verdict made on `headSha` that nothing on the host can still change: it does not wait, settles its head with every + * model check answered, confirmed every machine check, CI included, and holds or skips no Do for the head's CI, which + * a run of CI on the same head can still change. + */ +export function decided(verdict: VerdictFields & { actions?: unknown }, headSha: string): boolean { + const actions = Array.isArray(verdict.actions) ? verdict.actions.filter(isRecord) : []; + return verdict.headSha === headSha && verdict.waiting !== true && decidesDos(verdict) + && !checksOf(verdict).some((check) => check["kind"] === "machine" && check["result"] === "unconfirmed") + && !actions.some((action) => CI_REASONS.has(action["reason"])); +}