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 a46951250..1c264254a 100644 --- a/docs/Corbits Triage User Manual/Corbits Triage User Manual.md +++ b/docs/Corbits Triage User Manual/Corbits Triage User Manual.md @@ -40,7 +40,7 @@ Code review reads each new commit of a pull request for bugs: mistakes in the ch - **Turning it on.** A repository's [policy file](#writing-the-file) turns it on: it then runs after triage, for the groups in `for` and on the verdicts and paths in `when`. A pull request they leave out, or one whose failed rules hold the review back, is not reviewed, and its **Code review** section says **Not reviewed:** and why, such as **Not reviewed: the author is not in the maintainers group**. It needs the workspace's Anthropic API key, saved under [Settings → Code review](#code-review-2). - **What it costs.** Each commit it reviews is billed to your Anthropic account. It reviews a pull request when it is opened and after every push while it is not a draft, and **Review again** is one more review. A push that leaves the change exactly as it was reviewed, such as a rebase onto a newer base that changed none of the files it touches, is not reviewed again: the new commit keeps the last review, and the section says **Carried over from** the commit it was made for. **Review again** reviews it anyway. -- **Approve and merge.** With code review on, Triage approves or merges a pull request on its own only once the review of its latest commit is clean: it found nothing, or, with `clean: { below: }` in the policy file, nothing at that severity or above (P0 is critical, P1 high, P2 medium, P3 low). A review with such a finding, one that failed or ran past its hour, one that read only part of the change, and a review that never ran because the rules failed or had not settled leave approve and merge waiting for you on Triage, with the reason beside them, and Triage takes back an approval it gave an earlier commit. A check the file turns on that can't run yet, such as `security`, holds approve and merge until it can. A pull request the policy file chose not to review, by its author, its verdict or its changed files, is approved as if code review were off. +- **Approve and merge.** With code review on, Triage approves or merges a pull request on its own only once the review of its latest commit is clean: it found nothing, or, with `clean: { below: }` in the policy file, nothing at that severity or above (P0 is critical, P1 high, P2 medium, P3 low). A review with such a finding fails a pull request that would otherwise pass: its comment says how many problems the review found, the actions you set for Fail run, and Triage takes back an approval it gave an earlier commit. A pull request that is Unsure or already fails keeps its verdict with the review's problems added to its reasons, and approve and merge wait for you. Either way, Triage's comment asks the author to fix the review's problems. A review that failed or ran past its hour, one that read only part of the change, and a review that never ran because the rules failed or had not settled leave approve and merge waiting for you in the Inbox, with the reason beside them, and Triage takes back an approval it gave an earlier commit. A check the file turns on that can't run yet, such as `security`, holds approve and merge until it can. A pull request the policy file chose not to review, by its author, its verdict or its changed files, is approved as if code review were off. - **Where you see it.** In Triage's reading pane, under **Code review**: each bug with how severe it is, its file and line, and the code it points at. **Review again** reviews the latest commit once more. A very large change is read up to a size limit, and the review says how much of it was read and lists the files it did not review in full (a long list starts collapsed), or says some files were not reviewed when it cannot tell which. A review that stopped before it finished reading the repository says **Stopped early: the reviewer reached its limit for reading the repository.** - **Sending it to GitHub.** On a GitHub pull request, **Send to GitHub** posts the review as one review from the Triage App: each finding on a changed line is a comment on that line, and the rest are listed in the review. A finding whose lines run past the changed ones sits on its first changed lines and names the others. A review that stopped early carries the same line in the posted review, and a review of a cut change lists the files it did not review in full. A review too long for GitHub lists what fits and says how many more are in the portal. It is sent once per commit; the section then says **Sent to GitHub**, and sending it again only tells you it is already there. A review carried over to a newer commit is sent on that commit, unless it was already sent on the earlier one, where its comments stay on the same lines. A review of a newer commit is sent as a new review, and the earlier one stays. - **Sending it to Bitbucket.** On a Bitbucket Data Center pull request, **Send to Bitbucket** posts each finding on a changed line as a comment on that line, then one comment with the summary and the rest. A finding whose lines run past the changed ones sits on its first changed line and names the others. It is sent once per commit: sending it again posts only what is missing, then says **Sent to Bitbucket**. It needs the Comment permission for the repository. @@ -202,7 +202,7 @@ decide: Triage checks these keys when it reads the file. When the `code-review` block is on, Triage runs that review. `provider` is optional, so a file that names another provider, or names none, still runs it. It reviews each pull request's code after triage, for the groups in `for` and on the verdicts and paths in `when`. By default it reviews only once the rules in your file that fail a pull request and need no model have passed. CI counts only when `ci-green` is on. Triage's built-in checks, such as a draft or a requested change, don't hold the review back. A pull request that fails one gets its Fail with those reasons and no review, and one whose CI is still running is reviewed once CI passes. Write `when: { rules: any }` to review whatever those rules say. When triage could not read the pull request, code review waits instead of running. A check's `when: { paths: [...] }` limits it to pull requests where a changed file matches one of the globs. A glob starting with `!` leaves out the files it matches, so the `code-review` example above skips pull requests that change only Markdown docs or lockfiles (`*.lock`, which covers `bun.lock`, plus `package-lock.json` and `pnpm-lock.yaml`). A renamed file counts under its old and new path. When Triage can't read every changed file, the pull request is reviewed, never skipped. An author whose group Triage can't tell is left out by a `for` that does not name `everyone`. A file that turns code review on needs the workspace's Anthropic key ([Settings → Code review](#code-review-2)) and a [place to run](#where-code-review-runs). `security` and `deslop` are accepted and run no check. Triage does not carry out `decide`: a file that turns `code-review`, `security` or `deslop` on, or writes its own `decide`, leaves approve and merge for you on Triage. On GitHub, a pull request carrying the `pauseLabel` label starts no triage or check run while it has that label, and removing the label triages it again. On GitHub, while a pull request in a repository with a `pauseLabel` can't be read, it isn't triaged and is tried again later, never run as if unpaused. Through a long GitHub outage it is retried a limited number of times, then triaged on its next event. `pace` is accepted and Triage does not use it. -An action's `when: { author: [...] }`, a check's `for` and a `decide` rule's `author` can name a group. An author is in the first of your groups they match, in the order you wrote them, and in each built-in group (`maintainers`, `external`, `first-timers`, `bots`) their role puts them in. Everyone is in `everyone`. The built-in groups go by the author's role alone, so they still work when Triage can't read teams; only when it can't tell the author's role are they unknown too. For a group you define, Triage can't tell whether the author is in it when it can't tell their role, or when teams it can't read would decide which of your groups is theirs. An action for a group Triage can't tell about doesn't run. `everyone` always matches. Never let Approve or Merge rest on `everyone` alone. In `decide`, `{ all: clean }` means at least one check ran and every check that ran came back clean; checks that were skipped don't count, so when every check was skipped it doesn't match. A rule that approves or merges without naming `triage` only matches a Pass. A check with `clean: { below: medium }` is still clean when its only findings are less severe than medium. A review that couldn't read the whole change is incomplete, and incomplete is never clean. A file that uses `extends` can't hold these keys. Every key is listed in the setup skill's [reference](../../skills/corbits-triage-setup/reference.md#blocks-and-decide). +An action's `when: { author: [...] }`, a check's `for` and a `decide` rule's `author` can name a group. An author is in the first of your groups they match, in the order you wrote them, and in each built-in group (`maintainers`, `external`, `first-timers`, `bots`) their role puts them in. Everyone is in `everyone`. The built-in groups go by the author's role alone, so they still work when Triage can't read teams; only when it can't tell the author's role are they unknown too. For a group you define, Triage can't tell whether the author is in it when it can't tell their role, or when teams it can't read would decide which of your groups is theirs. An action for a group Triage can't tell about doesn't run. `everyone` always matches. Never let Approve or Merge rest on `everyone` alone. In `decide`, `{ all: clean }` means at least one check ran and every check that ran came back clean; checks that were skipped don't count, so when every check was skipped it doesn't match. A rule that approves or merges without naming `triage` only matches a Pass. A check with `clean: { below: medium }` is still clean when its only findings are less severe than medium; without `clean`, every finding counts. When code review finds problems that count on a pull request that would otherwise pass, it fails, its comment says how many, and the actions you set for Fail run; an Unsure or failing pull request keeps its verdict with the review's problems added. The next push is checked and reviewed again. A review that couldn't read the whole change is incomplete, and incomplete is never clean. A file that uses `extends` can't hold these keys. Every key is listed in the setup skill's [reference](../../skills/corbits-triage-setup/reference.md#blocks-and-decide). If the file has a mistake, each problem names its line and what the value must be, such as "Line 12: `security.when.triage` must be pass, fail or unsure, or a list of them." A misspelt key says which one you likely meant, such as "`code-reveiw` is not a key; did you mean `code-review`?" diff --git a/packages/triage-manager/src/actions/review-clean.ts b/packages/triage-manager/src/actions/review-clean.ts index 19dd4dae9..19085fbe0 100644 --- a/packages/triage-manager/src/actions/review-clean.ts +++ b/packages/triage-manager/src/actions/review-clean.ts @@ -1,5 +1,9 @@ import { type } from "arktype"; -import { BLOCK_KINDS, blockCleanSchema, CodeReviewOutput, RULE_GATES, rulesGate, SEVERITIES, type ActionKind } from "@corbits/triage-contracts"; +import { BLOCK_KINDS, blockCleanSchema, CodeReviewOutput, RULE_GATES, rulesGate, SEVERITIES, VERDICTS, type ActionKind } from "@corbits/triage-contracts"; +import { verdictWithFailure } from "@corbits/triage-workflows/actions/evaluate"; +import type { CheckResult } from "@corbits/triage-workflows/logic/checks"; +import type { Item } from "@corbits/triage-workflows/logic/item"; +import type { Verdict } from "@corbits/triage-workflows/logic/render"; import { storedReview, TakeOutput } from "./manager.js"; const CodeReviewPart = type({ on: "boolean", rules: type.enumerated(...RULE_GATES), "clean?": blockCleanSchema }); @@ -18,13 +22,15 @@ function severityAt({ priority = 0 }: Finding): number { return Math.min(Math.max(SEVERITIES.length - 1 - priority, 0), SEVERITIES.length - 1); } -/** Why the review is not clean: none reported, it read only part of the change, or a finding is at or above `below`. */ -function uncleanBy(review: CodeReviewOutput | undefined, below: (typeof SEVERITIES)[number] | undefined): string | undefined { - if (!review) return "Code review did not report on this head."; - if (review.budgetExhausted || review.cut) return "Code review did not read the whole change on this head."; +type Unclean = { held: string } | { found: number }; + +/** Why the review is not clean: none reported, or it read only part of the change; or how many findings are at or above `below`. */ +function uncleanBy(review: CodeReviewOutput | undefined, below: (typeof SEVERITIES)[number] | undefined): Unclean | undefined { + if (!review) return { held: "Code review did not report on this head." }; + if (review.budgetExhausted || review.cut) return { held: "Code review did not read the whole change on this head." }; const floor = below === undefined ? 0 : SEVERITIES.indexOf(below); - if (review.comments.some((finding) => severityAt(finding) >= floor)) return "Code review found problems on this head."; - return undefined; + const found = review.comments.filter((finding) => severityAt(finding) >= floor).length; + return found > 0 ? { found } : undefined; } /** A Do already held keeps its own reason, such as waiting for CI. */ @@ -35,34 +41,85 @@ function hold(action: A, reason: string): A { } /** - * Why approve and merge wait while the policy turns a check on. A check that can't run yet holds them until it can. + * Why the pass's verdict is not cleared while the policy turns a check on. Problems code review found on the head come + * first, so another check that can't run yet never hides them; such a check holds approve and merge until it can. * Code review must have run on the head and be clean as its block's `clean` says; one the policy's `for` or `when` left * out counts as off, and one held back by failed or unsettled rules did not run. A review that failed or ran past its * deadline reported nothing. */ -function heldBy({ taken, review, reviewers, head }: TakeOutput, checks: unknown, scope: Scope): string | undefined { +function uncleared({ taken, review, reviewers, head }: TakeOutput, checks: unknown, scope: Scope): Unclean | undefined { const part = BlocksPart(taken); if (part instanceof type.errors) return undefined; const { blocks, codeReview } = part; const waiting = blocks.filter((kind) => kind !== "code-review"); - if (waiting.length > 0) return `The policy's ${waiting.join(" and ")} check can't run yet.`; const leftOut = scope.skipped !== undefined && rulesGate(codeReview.rules, checks).open; - if (!codeReview.on || leftOut) return undefined; - if (!scope.reviewed) return "Code review did not run on this head."; - return uncleanBy(review ?? (head === undefined ? undefined : storedReview(reviewers, head)), codeReview.clean?.below); + const reviewing = codeReview.on && !leftOut; + const reported = review ?? (head === undefined ? undefined : storedReview(reviewers, head)); + const unclean = reviewing && scope.reviewed ? uncleanBy(reported, codeReview.clean?.below) : undefined; + if (unclean && "found" in unclean) return unclean; + if (waiting.length > 0) return { held: `The policy's ${waiting.join(" and ")} check can't run yet.` }; + if (!reviewing) return undefined; + if (!scope.reviewed) return { held: "Code review did not run on this head." }; + return unclean; +} + +/** The verdict fields cleared reads, the rest kept as evaluate made them. */ +const ClearedVerdict = type({ + number: "number", headSha: "string | null", verdict: type.enumerated(...VERDICTS), reason: "string", reasons: "string[]", feedback: "string", checks: "object[]", actions: "object[]", + degraded: "string | null", "approvable?": "boolean", "closed?": "true", +}).as(); + +const ClearedInput = type({ + take: { output: TakeOutput }, + rules: { output: { items: type("object[]").as() } }, + evaluateRules: { output: ClearedVerdict }, + reviewScope: { output: Scope }, +}); + +/** A code review with findings on the head is a failed check of its own. */ +function reviewFailure(found: number): CheckResult { + const reason = `Code review found ${found} ${found === 1 ? "problem" : "problems"} on this head.`; + return { check: "code-review", kind: "machine", result: "fail", reason, evidence: [], output: { kind: "values", values: { findings: found } } }; +} + +/** + * The verdict `found`, the review's problems, leave: evaluate's with the review among its checks and reasons, a Pass + * turned to Fail with the policy's fail Dos, rebuilt from the rules item it was made from: the one at its head, or its + * pull request's alone for a degraded verdict that names no head. One that cannot be made, or with no such item, such + * as a judge's verdict on a head the rules read past, leaves the verdict as the rules made it. + */ +function reviewed(items: Item[], verdict: Verdict, found: number): Verdict { + const item = items.find(({ facts }) => facts.number === verdict.number && (verdict.headSha === null || facts.headSha === verdict.headSha)); + if (!item) return verdict; + try { + return verdictWithFailure(item, verdict, reviewFailure(found)); + } catch { + // An action has no logger; the verdict's approve and merge are still held for the review's problems. + return verdict; + } } -const ClearedInput = type({ take: { output: TakeOutput }, evaluateRules: { output: { checks: "unknown", actions: "object[]", "approvable?": "boolean" } }, reviewScope: { output: Scope } }); +/** `verdict` with its approve and merge Dos held for a person, saying why, and not approvable. */ +function held(verdict: Verdict, reason: string): Verdict { + return { ...verdict, actions: verdict.actions.map((action) => hold(action, reason)), ...(verdict.approvable !== undefined && { approvable: false }) }; +} /** - * The pass's verdict with its approve and merge Dos held for a person, saying why, while the policy's checks are not - * clean on its head. Such a head is not approvable, so an approval Triage gave an earlier head is taken back. + * The pass's verdict once the policy's checks are in. A verdict whose code review found problems on its head names + * them, waiting, degraded or decided, and a Pass among them fails with the policy's fail Dos. A closed pull request has + * no verdict to name them in, a take that names no head has no review of one, and a verdict on another head was not + * reviewed, so each is only held. While a check is not clean, approve and merge are held for a person, saying why, and + * the head is not approvable, so an approval Triage gave an earlier head is taken back. */ -export async function cleared(input: unknown): Promise> { +export async function cleared(input: unknown): Promise { const parsed = ClearedInput(input); if (parsed instanceof type.errors) throw new Error(`cleared: ${parsed.summary}`); const verdict = parsed.evaluateRules.output; - const reason = heldBy(parsed.take.output, verdict.checks, parsed.reviewScope.output); - if (reason === undefined) return verdict; - return { ...verdict, actions: verdict.actions.map((action) => hold(action, reason)), ...(verdict.approvable !== undefined && { approvable: false }) }; + const { head } = parsed.take.output; + const unclean = uncleared(parsed.take.output, verdict.checks, parsed.reviewScope.output); + if (unclean === undefined) return verdict; + if (!("found" in unclean)) return held(verdict, unclean.held); + const reason = reviewFailure(unclean.found).reason; + if (verdict.closed || head === undefined || (verdict.headSha !== null && verdict.headSha !== head)) return held(verdict, reason); + return held(reviewed(parsed.rules.output.items, verdict, unclean.found), reason); } diff --git a/packages/triage-manager/src/triage-manager.test.ts b/packages/triage-manager/src/triage-manager.test.ts index 19de9be9b..fc063756d 100644 --- a/packages/triage-manager/src/triage-manager.test.ts +++ b/packages/triage-manager/src/triage-manager.test.ts @@ -196,6 +196,7 @@ function publish(release: Record): void { afterEach(() => publish({})); const READY = { method: "POST", path: `/repos/${PR.repo}/issues/${PR.number}/labels`, body: { labels: ["Triage: Ready"] } }; +const NOT_READY = { ...READY, body: { labels: ["Triage: Not Ready"] } }; const POSTED_REVIEW = expect.objectContaining({ method: "POST", path: `/repos/${PR.repo}/pulls/${PR.number}/reviews` }); /** A Do the verdict on the fixture's head holds for approval, and the hub's mail approving it. */ @@ -374,7 +375,7 @@ describe("triage-manager with code review", () => { const { ran } = await runMail(reviewed, wake(), host, second.hub, mailed); expect(ran).toContain("postReview"); expect(ran).not.toContain("deployReview"); - expect(writes).toContainEqual(READY); + expect(writes).toContainEqual(NOT_READY); expect(writes).toContainEqual(POSTED_REVIEW); expect(second.calls).toEqual([TAKE, { ...DONE, body: expect.objectContaining({ ...PULL, reviewers: [{ ...reviewer(), result: REVIEW }] }) }, END]); expect(mailed).toHaveLength(1); @@ -387,7 +388,7 @@ describe("triage-manager with code 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(NOT_READY); expect(writes).toContainEqual(POSTED_REVIEW); expect(mailed).toEqual([]); expect(hub.calls).toEqual([TAKE, { ...DONE, body: expect.objectContaining({ ...PULL, reviewers: [{ ...reviewer(), result: REVIEW }] }) }, END]); @@ -483,7 +484,6 @@ describe("triage-manager with code review", () => { } test.each([ ["a clean review", CLEAN, CODE_REVIEW, undefined], - ["a review with a finding", { result: REVIEW }, CODE_REVIEW, "Code review found problems on this head."], ["a review that failed", { failed: "the review did not finish" }, CODE_REVIEW, "Code review did not report on this head."], ["a pull request `when.paths` leaves out of code review", undefined, { ...CODE_REVIEW, codeReview: { ...CODE_REVIEW.codeReview, paths: ["!**/*.ts"] } }, undefined], ["only a check that can't run yet", undefined, { codeReview: { ...CODE_REVIEW.codeReview, on: false }, blocks: ["security"] }, "The policy's security check can't run yet."], @@ -495,7 +495,7 @@ describe("triage-manager with code review", () => { expect(approved(writes)).toBe(reason === undefined); }); - test("a head whose review has a P0 finding holds approve and is not approvable, so the approval of the clean head before it is taken back", async () => { + test("a head whose review has a P0 finding fails and is not approvable, so the approval of the clean head before it is taken back", async () => { const clean = await approvingPass((request) => reported(CLEAN, request)); expect(approved(clean.writes)).toBe(true); const NEWER = "0e0e".repeat(10); @@ -506,7 +506,7 @@ describe("triage-manager with code review", () => { return { ...report, pass: { ...report.pass, head: NEWER }, events: report.events.map(({ key, event }) => ({ key, event: { ...event, head: NEWER } })), reviewers: [{ ...reviewer(), head: NEWER }] }; } const { writes, outputs } = await approvingPass(newer, CODE_REVIEW, NEWER); - expect(outputs["cleared"]).toMatchObject({ headSha: NEWER, approvable: false, actions: [{ kind: "approve", held: true, reason: "Code review found problems on this head." }] }); + expect(outputs["cleared"]).toMatchObject({ headSha: NEWER, verdict: "fail", approvable: false, actions: [] }); expect(approved(writes)).toBe(false); }); @@ -522,10 +522,125 @@ describe("triage-manager with code review", () => { const APPROVE_DO = { list: "pass", index: 0, kind: "approve", effectId: "do:approve", target: {}, reason: "every rule passed" }; const CI_FAILED = [{ check: "ci-green", kind: "machine", result: "fail" }]; const finding = (priority?: number) => ({ ...REVIEW, comments: [{ ...REVIEW.comments[0]!, ...(priority !== undefined && { priority }) }] }); + const FAILED = { verdict: "fail", reason: "Code review found 1 problem on this head.", approvable: false, actions: [{ list: "fail", kind: "request-changes" }] }; + const FAILING: CheckPack = { ...RULES, actions: { ...RULES.actions, pass: [{ do: "approve" }], fail: [{ do: "request-changes" }] } }; + test.each([ + ["a review with one P2 finding fails the verdict, runs the policy's fail Dos and is not approvable", finding(2), {}, FAILED], + ["a finding at `clean.below` fails the verdict", finding(1), { clean: { below: "high" } }, FAILED], + ["a finding with no priority counts at any `clean.below`", finding(), { clean: { below: "critical" } }, FAILED], + ["a clean review leaves the verdict as the rules made it", CLEAN.result, {}, { verdict: "pass", approvable: true, actions: [{ list: "pass", kind: "approve" }] }], + ])("%s", async (_name, review, codeReview, cleared) => { + const { writes, host } = githubHost(HEAD, { checkRuns: [{ name: "ci", status: "completed", conclusion: "success" }] }); + const request = hubRequest(FAILING, { ...CODE_REVIEW, codeReview: { ...CODE_REVIEW.codeReview, ...codeReview }, ...REVIEW_REQUEST }); + const hub = fakeHub({ takes: [reported({ result: review }, request)], dones: [ENDED] }); + const { outputs } = await runMail(reviewed, wake(), host, hub.hub); + if (cleared.verdict === "fail") expect(outputs["evaluateRules"]).toMatchObject({ verdict: "pass", approvable: true }); + expect(outputs["cleared"]).toMatchObject(cleared); + expect(approved(writes)).toBe(cleared.verdict === "pass"); + expect(writes.some((write) => (write.body as { event?: string } | undefined)?.event === "REQUEST_CHANGES")).toBe(cleared.verdict === "fail"); + }); + + const REVIEW_REASON = "Code review found 1 problem on this head."; + const CI_REASON = "required checks are failing: ci"; + test.each([ + ["a Fail its rules made keeps its verdict and adds the review's reason and check", FAILING, "fail", `required checks are failing; ${REVIEW_REASON}`, true], + ["an Unsure stays Unsure with the review's problems added, approve and merge held for a maintainer", { ...FAILING, checks: { "ci-green": { enabled: true, onFail: "ask" } } } as CheckPack, "unsure", `${CI_REASON}; ${REVIEW_REASON}`, false], + ])("%s", async (_name, pack, verdict, reason, changes) => { + const { writes, host } = githubHost(HEAD, { checkRuns: [{ name: "ci", status: "completed", conclusion: "failure" }] }); + const hub = fakeHub({ takes: [reported({ result: finding(2) }, hubRequest(pack, { ...CODE_REVIEW, ...REVIEW_REQUEST }))], dones: [ENDED] }); + const { outputs } = await runMail(reviewed, wake(), host, hub.hub); + const cleared = outputs["cleared"] as { verdict: string; reason: string; reasons: string[]; feedback: string; checks: Record[] }; + expect(cleared.verdict).toBe(verdict); + expect(cleared.reasons).toEqual([CI_REASON, REVIEW_REASON]); + expect(cleared.checks.at(-1)).toEqual({ check: "code-review", kind: "machine", result: "fail", reason: REVIEW_REASON, evidence: [], output: { kind: "values", values: { findings: 1 } } }); + expect(cleared.reason).toBe(reason); + expect(cleared.feedback).toContain(`- ${REVIEW_REASON}`); + expect(cleared.feedback).toContain("- required checks are failing: `ci`"); + expect(writes.some((write) => (write.body as { event?: string } | undefined)?.event === "REQUEST_CHANGES")).toBe(changes); + expect(approved(writes)).toBe(false); + }); + + /** The rules output and the verdict of a pass whose review was clean, for `cleared` to be run on by hand. */ + async function cleanPass(pack = FAILING) { + const { host } = githubHost(HEAD, { checkRuns: [{ name: "ci", status: "completed", conclusion: "success" }] }); + const hub = fakeHub({ takes: [reported(CLEAN, hubRequest(pack, { ...CODE_REVIEW, ...REVIEW_REQUEST }))], dones: [ENDED] }); + const { outputs } = await runMail(reviewed, wake(), host, hub.hub); + return { rules: outputs["rules"] as { items: Record[] }, verdict: outputs["evaluateRules"] as Record }; + } + + function takenWith(blocks: string[]) { + return { run: true, approvals: [], triage: true, overdue: false, reviewers: [], head: HEAD, review: finding(2), taken: { blocks, ...CODE_REVIEW } }; + } + + test.each([ + ["an Unsure waiting on CI", { verdict: "unsure", waiting: true, reason: "CI is still running", feedback: "", actor: "system", nextAction: "Wait for checks to finish" }], + ["an Unsure the decision model could not make", { verdict: "unsure", degraded: "inference-outage", reason: "decision model unavailable", feedback: "", actor: "maintainer", nextAction: "Review" }], + ])("%s keeps its state and gains the review's check, reason, next step and comment, approve and merge held", async (_name, unsure) => { + const { rules, verdict } = await cleanPass(); + const made = await actions.cleared({ take: { output: takenWith(["code-review"]) }, rules: { output: rules }, evaluateRules: { output: { ...verdict, ...unsure } }, reviewScope: { output: { reviewed: true } } }) as Record & { reasons: string[]; checks: Record[] }; + expect(made).toMatchObject({ ...unsure, reason: `${unsure.reason}; ${REVIEW_REASON}`, nextAction: `${unsure.nextAction}; the author fixes: ${REVIEW_REASON}`, approvable: false, feedback: `**Triage: Unsure**\n\n@octocat, please address the following:\n- ${REVIEW_REASON}` }); + expect(made.reasons.at(-1)).toBe(REVIEW_REASON); + expect(made.checks.at(-1)).toMatchObject({ check: "code-review", result: "fail", output: { kind: "values", values: { findings: 1 } } }); + expect(made).toMatchObject({ mirror: true, request: { comment: expect.stringContaining(`- ${REVIEW_REASON}`) } }); + }); + + test("a degraded verdict that names no head gains the review's problems and is still not posted", async () => { + const { rules, verdict } = await cleanPass(); + const degraded = { ...verdict, verdict: "unsure", headSha: null, degraded: "error", mirror: false, reason: "render failed", feedback: "" }; + const made = await actions.cleared({ take: { output: takenWith(["code-review"]) }, rules: { output: rules }, evaluateRules: { output: degraded }, reviewScope: { output: { reviewed: true } } }) as Record & { reasons: string[] }; + expect(made).toMatchObject({ verdict: "unsure", headSha: null, degraded: "error", mirror: false, reason: `render failed; ${REVIEW_REASON}` }); + expect(made.reasons.at(-1)).toBe(REVIEW_REASON); + }); + + test("a Pass a policy that never approves turns to Fail stays without an approvable", async () => { + const { rules, verdict } = await cleanPass({ ...FAILING, actions: { ...FAILING.actions, pass: [] } }); + expect(verdict).not.toHaveProperty("approvable"); + const made = await actions.cleared({ take: { output: takenWith(["code-review"]) }, rules: { output: rules }, evaluateRules: { output: verdict }, reviewScope: { output: { reviewed: true } } }); + expect(made).toMatchObject({ verdict: "fail", reason: REVIEW_REASON }); + expect(made).not.toHaveProperty("approvable"); + }); + + test("a review's problems on a take that names no head hold approve and merge on the verdict as the rules made it", async () => { + const { rules, verdict } = await cleanPass(); + const { head: _head, ...headless } = takenWith(["code-review"]); + const made = await actions.cleared({ take: { output: headless }, rules: { output: rules }, evaluateRules: { output: verdict }, reviewScope: { output: { reviewed: true } } }); + expect(made).toEqual({ ...verdict, approvable: false, actions: [{ ...(verdict["actions"] as object[])[0], held: true, reason: REVIEW_REASON }] }); + }); + + test("a degraded verdict that names no head, with no rules item for its pull request, is held rather than failing the step", async () => { + const { verdict } = await cleanPass(); + const degraded = { ...verdict, verdict: "unsure", headSha: null, degraded: "error", mirror: false, reason: "render failed", feedback: "" }; + const elsewhere = { items: [{ facts: { number: PR.number + 1, headSha: HEAD } }] }; + const made = await actions.cleared({ take: { output: takenWith(["code-review"]) }, rules: { output: elsewhere }, evaluateRules: { output: degraded }, reviewScope: { output: { reviewed: true } } }); + expect(made).toMatchObject({ verdict: "unsure", headSha: null, reason: "render failed", approvable: false }); + }); + + test("a review's problems fail a Pass while another check the policy turns on can't run yet", async () => { + const { rules, verdict } = await cleanPass(); + const made = await actions.cleared({ take: { output: takenWith(["code-review", "security"]) }, rules: { output: rules }, evaluateRules: { output: verdict }, reviewScope: { output: { reviewed: true } } }); + expect(made).toMatchObject(FAILED); + }); + + const PASSED = { number: PR.number, headSha: HEAD, verdict: "pass", reason: "every rule passed", reasons: [], feedback: "", degraded: null, checks: [], actions: [APPROVE_DO] }; + const NEWER = "0e0e".repeat(10); + test.each([ + ["a rules item the verdict cannot be made from", PASSED, [{ facts: { number: PR.number, headSha: HEAD } }]], + ["no rules item at the verdict's head, such as a judge's verdict on a head the rules read past", PASSED, [{ facts: { number: PR.number, headSha: NEWER } }]], + ])("a review's problems with %s leave the verdict as the rules made it, approve held", async (_name, verdict, items) => { + const made = await actions.cleared({ take: { output: takenWith(["code-review"]) }, rules: { output: { items } }, evaluateRules: { output: verdict }, reviewScope: { output: { reviewed: true } } }); + expect(made).toEqual({ ...verdict, actions: [{ ...APPROVE_DO, held: true, reason: REVIEW_REASON }] }); + }); + + test("a review's problems on a verdict the rules made for a newer head than the review's hold approve and merge, the verdict as the rules made it", async () => { + const { rules, verdict } = await cleanPass(); + const items = rules.items.map((item) => ({ ...item, facts: { ...(item["facts"] as object), headSha: NEWER } })); + const newer = { ...verdict, headSha: NEWER }; + const made = await actions.cleared({ take: { output: takenWith(["code-review"]) }, rules: { output: { items } }, evaluateRules: { output: newer }, reviewScope: { output: { reviewed: true } } }); + expect(made).toMatchObject({ verdict: "pass", headSha: NEWER, approvable: false, actions: [{ kind: "approve", held: true, reason: REVIEW_REASON }] }); + }); + test.each([ - ["a finding at `clean.below` holds", finding(1), { clean: { below: "high" } }, { reviewed: true }, "Code review found problems on this head."], ["a finding below `clean.below` does not", finding(2), { clean: { below: "high" } }, { reviewed: true }, undefined], - ["a finding with no priority holds", finding(), { clean: { below: "critical" } }, { reviewed: true }, "Code review found problems on this head."], ["a review cut for size holds", { ...CLEAN.result, cut: { lines: 1, of: 2 } }, {}, { reviewed: true }, "Code review did not read the whole change on this head."], ["a review that stopped early holds", { ...CLEAN.result, budgetExhausted: true }, {}, { reviewed: true }, "Code review did not read the whole change on this head."], ["a review skipped for failed rules holds", undefined, { rules: "pass" }, { reviewed: false, skipped: "rules failed: CI green" }, "Code review did not run on this head."], @@ -533,14 +648,14 @@ describe("triage-manager with code review", () => { ])("cleared: %s", async (_name, review, codeReview, scope, reason) => { const checks = scope.skipped?.startsWith("rules failed") ? CI_FAILED : []; const take = { run: true, approvals: [], triage: true, overdue: false, reviewers: [], head: HEAD, ...(review && { review }), taken: { blocks: ["code-review"], codeReview: { ...CODE_REVIEW.codeReview, ...codeReview } } }; - const { actions: [approve] } = await actions.cleared({ take: { output: take }, evaluateRules: { output: { checks, actions: [APPROVE_DO] } }, reviewScope: { output: scope } }) as { actions: Record[] }; + const { actions: [approve] } = await actions.cleared({ take: { output: take }, rules: { output: { items: [] } }, evaluateRules: { output: { ...PASSED, checks } }, reviewScope: { output: scope } }) as { actions: Record[] }; expect(approve).toEqual(reason === undefined ? APPROVE_DO : { ...APPROVE_DO, held: true, reason }); }); test("cleared keeps the reason of a Do already held, such as for CI", async () => { const waiting = { ...APPROVE_DO, kind: "merge", held: true, reason: "Waiting for checks to finish." }; - const take = { run: true, approvals: [], triage: true, overdue: false, reviewers: [], head: HEAD, review: REVIEW, taken: { blocks: ["code-review"], ...CODE_REVIEW } }; - const { actions: [merge] } = await actions.cleared({ take: { output: take }, evaluateRules: { output: { checks: [], actions: [waiting] } }, reviewScope: { output: { reviewed: true } } }) as { actions: Record[] }; + const take = { run: true, approvals: [], triage: true, overdue: false, reviewers: [], head: HEAD, review: { ...REVIEW, cut: { lines: 1, of: 2 } }, taken: { blocks: ["code-review"], ...CODE_REVIEW } }; + const { actions: [merge] } = await actions.cleared({ take: { output: take }, rules: { output: { items: [] } }, evaluateRules: { output: { ...PASSED, actions: [waiting] } }, reviewScope: { output: { reviewed: true } } }) as { actions: Record[] }; expect(merge).toEqual(waiting); }); @@ -734,7 +849,7 @@ async function firstJudgeMail(request = hubRequest(JUDGED)): Promise<{ content: describe("triage-manager with a judge and code review", () => { beforeEach(() => publish({ github: { judge: JUDGE_RELEASE, codeReview: RELEASE } })); - test("posts the judge's verdict and the review together once both have reported", async () => { + test("posts the judge's verdict and the review together once both have reported, a finding failing the verdict and keeping the judge's words", async () => { const { writes, host } = githubHost(); const mailed: unknown[] = []; const request = hubRequest(JUDGED, { ...CODE_REVIEW, ...REVIEW_REQUEST }); @@ -755,6 +870,8 @@ describe("triage-manager with a judge and code review", () => { expect(ran).toContain("postReview"); expect(ran).not.toContain("deployJudge"); expect(writes).toContainEqual(JUDGE_POSTED); + expect(writes).toContainEqual(expect.objectContaining({ path: `/repos/${PR.repo}/issues/${PR.number}/comments`, body: { body: expect.stringContaining("**Triage: Fail**\n\n@octocat, please address the following:\n- Code review found 1 problem on this head.") } })); + expect(writes).toContainEqual(NOT_READY); expect(writes).toContainEqual(POSTED_REVIEW); expect(third.calls[1]).toEqual({ ...DONE, body: expect.objectContaining({ ...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 203d4a1d3..56c3e9e20 100644 --- a/packages/triage-manager/src/triage-manager.ts +++ b/packages/triage-manager/src/triage-manager.ts @@ -25,7 +25,7 @@ const MAIL = { project: { from: "trigger.payload" }, fields: ["payload"] } as co /** The newest pull request mail take read back from the hub, which pr-triage's steps trust as the hub's. */ const TAKEN = { project: { from: "steps.take.output" }, fields: ["taken"] } as const; -/** The verdict the pass posts and runs: evaluateRules' with approve and merge held while the policy's checks are not clean. */ +/** The verdict the pass posts and runs: evaluateRules', failed by a code review that found problems, or with approve and merge held while the policy's checks are not clean. */ const CLEARED = { from: "steps.cleared.output" } as const; function stepsOf(...ids: string[]) { @@ -46,7 +46,7 @@ function reviewSteps(sources: Sources): Record { return { reviewScope: action({ handler: "reviewScope", input: { merge: [MAIL, stepsOf("take", "rules", "evaluateRules")] }, after: ["evaluateRules"] }), // After reviewScope, so a review the policy left out is told from one that is due; the verdict goes up from here. - cleared: action({ handler: "cleared", input: stepsOf("take", "evaluateRules", "reviewScope"), after: ["reviewScope"] }), + cleared: action({ handler: "cleared", input: stepsOf("take", "rules", "evaluateRules", "reviewScope"), after: ["reviewScope"] }), reviewing: gate({ when: { from: "steps.reviewScope.output.deploy" }, then: "deployReview", else: "posting", after: ["cleared"] }), deployReview: hubAction("deployReview", { merge: [MAIL, stepsOf("take")] }, ["reviewing"]), mailReview: step({ agent: mailAgentOf(mailDirector, judgeAgent.inference, { tolerate: true }), input: { from: "steps.deployReview.output.mail" }, after: ["deployReview"] }), diff --git a/packages/triage-workflows/src/actions/evaluate.ts b/packages/triage-workflows/src/actions/evaluate.ts index 6bc6b354b..cfc242cc9 100644 --- a/packages/triage-workflows/src/actions/evaluate.ts +++ b/packages/triage-workflows/src/actions/evaluate.ts @@ -8,7 +8,7 @@ import { errorText } from "@corbits/common"; import { unrunnable } from "../logic/execute-do.js"; import type { Item } from "../logic/item.js"; import { askedQuestionIds, focusedPartRequests, INSTRUCTED_QUESTION, prepareQualityEvaluation, verdictRequest, VERDICT_CHOICES, VERDICT_QUESTION, type FocusedParts, type JudgeRequest, type PolicyRule, type QualityEvaluation } from "../logic/quality.js"; -import { choiceOver, decidesByModel, degradedVerdict, evaluateModel, parseAnswers, separateSlices, renderVerdict, splitLabelled, summarize, toMirrorRequest, type BacklogSummary, type ParsedAnswers, type RenderOutput, type Verdict, type WaitingMark } from "../logic/render.js"; +import { choiceOver, decidesByModel, degradedVerdict, evaluateModel, parseAnswers, separateSlices, renderVerdict, splitLabelled, summarize, toMirrorRequest, withFailure, type BacklogSummary, type ParsedAnswers, type RenderOutput, type Verdict, type WaitingMark } from "../logic/render.js"; import { stepJson } from "../logic/step-json.js"; import { waitingMark } from "./waiting.js"; @@ -143,6 +143,12 @@ function withDos(feedback: string, actions: SuggestedAction[]): string { return lines.length ? `${feedback}\n\n${lines.join("\n")}` : feedback; } +/** `feedback` without the Dos `withDos` closed it with. */ +function withoutDos(feedback: string, actions: SuggestedAction[]): string { + const dos = withDos(" ", actions).slice(1); + return dos && feedback.endsWith(dos) ? feedback.slice(0, -dos.length) : feedback; +} + /** * A run that could not read the pull request says so where its verdict would stand: the failed label in place of any * verdict label on GitHub, a failed build status on Bitbucket Data Center. None without a known head, so a newer head is @@ -188,6 +194,47 @@ export function suggestedDos(it: Item, rendered: Rendered, steps: readonly Step[ return holdDos(heldApprovals(suggested, approvalHeldBy(rendered.verdict, it.facts.checks), it.facts.checks), it.automatic); } +/** The verdict on a rendered item: its labels, its Dos, and the comment and labels its mirror writes. */ +function verdictFrom(it: Item, rendered: RenderOutput): Verdict { + const actionInput = actionInputOf(it, rendered); + const wanted = actionInput && changesWanted(actionInput); + const held = approvalHeldBy(rendered.verdict, it.facts.checks); + const automatic = it.automatic; + const actions = suggestedDos(it, rendered, it.pack?.actions[rendered.verdict] ?? []); + const feedback = it.blocksOn ? rendered.feedback : withDos(rendered.feedback, actions); + // What reading the policy left out is said on every verdict, so the policy can be fixed. + const reasons = [...rendered.reasons, ...(it.pack?.dropped ?? [])]; + const labels = it.pack && !rendered.waiting ? [it.pack.labels[rendered.verdict]] : rendered.labels; + const owned = it.pack ? ownedLabels(it.pack.labels) : undefined; + const mark = rendered.waiting && waitingMark(repoSourceOf(it.repoSource), it.facts, automatic, it.pack?.labels ?? VERDICT_LABELS, it.facts.checks !== "none", it.appLogin); + const verdict = { + ...stamp(it, it.facts.headSha), ...rendered, labels, reasons, feedback, automatic, actions, + ...(it.pack && packApproves(it.pack) && { approvable: held === undefined }), ...(wanted !== undefined && { changesWanted: wanted }), + ...(mark && { mark }), ...(it.facts.state === "closed" && { closed: true as const }), + }; + return { ...verdict, request: toMirrorRequest(verdict, owned, it.source) }; +} + +/** + * `verdict` on `it` as evaluate makes it once `failed`, a check made after it such as code review, found problems. + * What the verdict made of its rendering is made again, as the policy makes it for the new verdict. A degraded verdict + * that names no head keeps what it marks on the pull request in its verdict's place, and is still not posted. + */ +export function verdictWithFailure(it: Item, verdict: Verdict, failed: CheckResult): Verdict { + const { author, reviewers } = it.facts; + if (verdict.headSha === null) { + const made = withFailure(verdict, failed, { author, det: it.det, reviewers }); + return { ...verdict, ...made, request: { ...verdict.request, comment: made.feedback } }; + } + // verdictFrom sets these only when the new verdict has them, so the old ones must not carry over. + const { actions, approvable: _approvable, changesWanted: _changesWanted, mark: _mark, closed: _closed, ...rendered } = verdict; + const dropped = it.pack?.dropped; + // verdictFrom closes the reasons with what reading the policy left out, and does so again. + const reasons = dropped ? rendered.reasons.slice(0, rendered.reasons.length - dropped.length) : rendered.reasons; + const feedback = withoutDos(rendered.feedback, actions); + return verdictFrom(it, withFailure({ ...rendered, feedback, reasons }, failed, { author, det: it.det, reviewers })); +} + /** Takes the item's own chunk replies off the front of `replies` and `partReplies`. */ function verdictOf(it: Item, replies: unknown[], partReplies: unknown[]): Verdict { if (it.error !== undefined) return degradedOf(it, it.error); @@ -216,23 +263,7 @@ function verdictOf(it: Item, replies: unknown[], partReplies: unknown[]): Verdic judgeLimitExceeded: it.judgeLimitExceeded, judgeSkipped: it.judgeSkipped, reviewers: it.facts.reviewers, choice: answered, ...(choice && "judgeError" in choice && { choiceRefused: choice.judgeError }), ...(it.noWait && { noWait: it.noWait }), }); - const actionInput = actionInputOf(it, rendered); - const wanted = actionInput && changesWanted(actionInput); - const held = approvalHeldBy(rendered.verdict, it.facts.checks); - const automatic = it.automatic; - const actions = suggestedDos(it, rendered, it.pack?.actions[rendered.verdict] ?? []); - const feedback = it.blocksOn ? rendered.feedback : withDos(rendered.feedback, actions); - // What reading the policy left out is said on every verdict, so the policy can be fixed. - const reasons = [...rendered.reasons, ...(it.pack?.dropped ?? [])]; - const labels = it.pack && !rendered.waiting ? [it.pack.labels[rendered.verdict]] : rendered.labels; - const owned = it.pack ? ownedLabels(it.pack.labels) : undefined; - const mark = rendered.waiting && waitingMark(repoSourceOf(it.repoSource), it.facts, automatic, it.pack?.labels ?? VERDICT_LABELS, it.facts.checks !== "none", it.appLogin); - const verdict = { - ...stamp(it, it.facts.headSha), ...rendered, labels, reasons, feedback, automatic, actions, - ...(it.pack && packApproves(it.pack) && { approvable: held === undefined }), ...(wanted !== undefined && { changesWanted: wanted }), - ...(mark && { mark }), ...(it.facts.state === "closed" && { closed: true as const }), - }; - return { ...verdict, request: toMirrorRequest(verdict, owned, it.source) }; + return verdictFrom(it, rendered); } catch (e) { return degradedOf(it, `render failed: ${errorText(e)}`); } diff --git a/packages/triage-workflows/src/logic/render.test.ts b/packages/triage-workflows/src/logic/render.test.ts index 73346fa06..b1a31a845 100644 --- a/packages/triage-workflows/src/logic/render.test.ts +++ b/packages/triage-workflows/src/logic/render.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; import { classificationSources, emptyPack, type CheckPack } from "@corbits/triage-contracts"; -import { MAX_MIRROR_COMMENT_BYTES, parseAnswers, renderVerdict } from "./render.js"; +import { MAX_MIRROR_COMMENT_BYTES, parseAnswers, renderVerdict, withFailure } from "./render.js"; import { QUALITY_EVALUATION_LIMIT_ERROR, type FocusedPlan, type QualityPlan } from "./quality.js"; import { REVIEW_ANSWERED_REASON, type DeterministicResult } from "./checks.js"; @@ -459,3 +459,29 @@ describe("requested changes rendering", () => { expect(verdict.feedback).toBe("**Triage: Fail**\n\n@octocat, please address the following:\n- merge conflicts with base"); }); }); + +describe("a check failed after the verdict", () => { + const failed = { check: "code-review", kind: "machine", result: "fail", reason: "Code review found 1 problem on this head.", evidence: [], output: { kind: "values", values: { findings: 1 } } } as const; + const sources = { quality: [], custom: [], rules: [] }; + const asked = "**Triage: Fail**\n\n@octocat, please address the following:\n- Code review found 1 problem on this head."; + + const passing = { state: "ready-monitoring", reason: "all deterministic rules passed", findings: [], checks: [], needsJudgment: true, sources } as DeterministicResult; + const requested = { state: "needs-author-update", reason: "a reviewer requested changes", findings: [], checks: [{ check: "no-changes-requested", kind: "machine", result: "fail", reason: "a reviewer requested changes", evidence: ["changes requested by @maya"], output: { kind: "values", values: {} } }], needsJudgment: false, sources } as DeterministicResult; + + test.each([ + ["a Pass that posted no comment fails and posts one", passing, "Code review found 1 problem on this head.", "Fix: Code review found 1 problem on this head."], + ["a Fail whose only reason is a maintainer's requested changes posts one", requested, "a reviewer requested changes; Code review found 1 problem on this head.", "Address requested changes from @maya; the author fixes: Code review found 1 problem on this head."], + ])("%s asking the author to fix it", (_name, det, reason, nextAction) => { + const rendered = { ...renderVerdict({ repoSource: "github", author: "octocat", repo: "acme/widgets", headSha: "abc123", det }), feedback: "" }; + const made = withFailure(rendered, failed, { author: "octocat", det, reviewers: [] }); + expect(made).toMatchObject({ verdict: "fail", reason, feedback: asked, actor: "author", nextAction }); + }); + + test.each([ + ["a next step keeps its actor and names the author for the fix", "Review", { actor: "maintainer", nextAction: "Review; the author fixes: Code review found 1 problem on this head." }], + ["no next step makes the fix the author's", "", { actor: "author", nextAction: "Fix: Code review found 1 problem on this head." }], + ])("an Unsure with %s", (_name, nextAction, next) => { + const rendered = { ...renderVerdict({ repoSource: "github", author: "octocat", repo: "acme/widgets", headSha: "abc123", det: passing }), verdict: "unsure" as const, actor: "maintainer" as const, nextAction }; + expect(withFailure(rendered, failed, { author: "octocat", det: passing, reviewers: [] })).toMatchObject({ verdict: "unsure", ...next }); + }); +}); diff --git a/packages/triage-workflows/src/logic/render.ts b/packages/triage-workflows/src/logic/render.ts index 47b9b0893..476725b7a 100644 --- a/packages/triage-workflows/src/logic/render.ts +++ b/packages/triage-workflows/src/logic/render.ts @@ -642,6 +642,9 @@ function skippedLine(c: CheckResult, sources: NonNullable): string[] { /** What a passing comment tells the author when nothing else is listed. */ const NOTHING_TO_CHANGE = "No changes needed from you. "; +/** What a comment says when it lists nothing for the author or a maintainer. */ +const MAINTAINER_REVIEWS = "A maintainer will review this pull request."; + /** * Public: the verdict, what the author must fix and what is not yet confirmed; never a score or model reasoning. A * maintainer's requested changes are already in front of the author, so they are left out, and a comment holding @@ -684,7 +687,7 @@ function authorComment(ctx: RenderContext, checks: CheckResult[], verdict: Verdi } if (!lines.length) { if (failing.some((c) => c.check === NO_CHANGES_REQUESTED)) return ""; - lines.push(`${verdict === "pass" ? NOTHING_TO_CHANGE : ""}A maintainer will review this pull request.`); + lines.push(`${verdict === "pass" ? NOTHING_TO_CHANGE : ""}${MAINTAINER_REVIEWS}`); } if (ctx.stack?.onto !== undefined && !holdsOthersChanges(ctx.stack)) lines.push("", `Stacked on #${ctx.stack.onto}: only this pull request's own changes were checked.`); const skipped = sources ? checks.flatMap((c) => skippedLine(c, sources)) : []; @@ -737,6 +740,48 @@ function withChecks(rendered: VerdictBase, checks: CheckResult[], score: number const VERDICT_STATES: Record = { pass: "ready-monitoring", fail: "needs-author-update", unsure: "awaiting-review" }; +/** A verdict's comment lines below its title. */ +function commentBody({ feedback, verdict }: Pick): string[] { + if (!feedback) return []; + const [title, ...body] = feedback.split("\n"); + if (title !== commentTitle(verdict)) return [title!, ...body]; + return body[0] === "" ? body.slice(1) : body; +} + +/** `previous`, a verdict's comment without its title, under `title` and asking its author to fix `reason` too. */ +function withFinding(title: string, previous: string[], author: string, reason: string): string[] { + const lead = `@${author}, please address the following:`; + const at = previous.indexOf(lead); + if (at === -1) return [title, "", lead, `- ${reason}`, ...(previous.length ? ["", ...previous] : [])]; + return [title, "", ...previous.slice(0, at + 1), `- ${reason}`, ...previous.slice(at + 1)]; +} + +/** A comment without the line saying it lists nothing, and the blank lines that line leaves at its top. */ +function listingSomething(lines: string[]): string[] { + const listed = lines.filter((line) => line !== MAINTAINER_REVIEWS && line !== `${NOTHING_TO_CHANGE}${MAINTAINER_REVIEWS}`); + const first = listed.findIndex((line) => line !== ""); + return first === -1 ? [] : listed.slice(first); +} + +/** + * A decided verdict once `failed`, a check made after it such as code review, found problems, `failed` among its checks + * and reasons. A Pass turns to Fail. An Unsure or a Fail keeps its verdict and its next step's actor, its reason and next + * step going on to name `failed` and that the author fixes it. Whatever the verdict, its comment asks the author to fix + * `failed`, below what it said before, so a verdict that posted no comment now posts one. + */ +export function withFailure(rendered: RenderOutput, failed: CheckResult, { author, det, reviewers }: { author: string; det: DeterministicResult; reviewers: string[] }): RenderOutput { + const reasons = [...rendered.reasons, failed.reason]; + const checks = [...rendered.checks, failed]; + const previous = commentBody(rendered); + if (rendered.verdict !== "pass") { + const feedback = bounded(withFinding(commentTitle(rendered.verdict), previous, author, failed.reason)); + const next = rendered.nextAction ? { nextAction: `${rendered.nextAction}; the author fixes: ${failed.reason}` } : nextStep([failed], det.sources, reviewers, rendered.verdict); + return { ...rendered, reason: `${rendered.reason}; ${failed.reason}`, ...next, reasons, checks, feedback }; + } + const feedback = bounded(withFinding(commentTitle("fail"), listingSomething(previous), author, failed.reason)); + return { ...rendered, ...render(VERDICT_STATES.fail), verdict: "fail", reason: failed.reason, ...nextStep(checks, det.sources, reviewers, "fail"), labels: [VERDICT_LABELS.fail], reasons, checks, feedback }; +} + /** Pass or fail only when that choice clears the bar; anything else is unsure. */ function decide(choice: Record): VerdictWord { if (choice.pass >= VERDICT_DECIDES) return "pass"; diff --git a/skills/corbits-triage-setup/reference.md b/skills/corbits-triage-setup/reference.md index 31f19a6c7..e3dac00d1 100644 --- a/skills/corbits-triage-setup/reference.md +++ b/skills/corbits-triage-setup/reference.md @@ -141,7 +141,7 @@ decide: - `on`: `true` or `false` (required). - `for`: list of groups; default everyone. - `when`: `triage` (`pass`, `fail`, `unsure` or a list of them), `rules` (`code-review` only) and `paths` (globs; any changed file matching is enough). A glob starting with `!` leaves out the files it matches; with only `!` globs, every other file matches. When the user asks to turn on `code-review`, suggest skipping pull requests that change only Markdown docs or lockfiles (`*.lock`, which covers `bun.lock`, plus `package-lock.json` and `pnpm-lock.yaml`), as in the example. `rules` (`code-review` only): `pass` (the default) runs the review only once every rule in the file with `onFail: fail` that needs no model has passed, CI only when `ci-green` is one of them, and Triage's built-in checks (draft, changes requested) never hold it back; a failed one skips it, and while CI is still running it waits. `any` runs it whatever those rules say. - - `clean`: `below` (`low`, `medium`, `high` or `critical`); findings below it don't count. `minConfidence` and `coverage` are refused. + - `clean`: `below` (`low`, `medium`, `high` or `critical`); findings below it don't count, and without it every finding counts. A code review with findings that count turns a Pass into Fail, and the `fail` actions run; an Unsure or Fail keeps its verdict, with the review's problems among its reasons and approve and merge held. `minConfidence` and `coverage` are refused. - `provider`: `anthropic`, `openai`, `xai`, `google`, `opencode` or `custom`. Every block that is on and names one must name the same. - `pace`: `debounce`, `maxWait` (durations of at least 1, such as `30s`, `2m`, `24h`, `7d`), `perHead` (whole number ≥ 1), `cap: { runs, per }`, `backoff: { pushes, within, pause }`. - `pauseLabel`: a label of at most 50 characters.