fix(manager): fail the verdict when code review finds problems

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: <severity> }` 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: <severity> }` 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`?"
 

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<A extends object>(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<Verdict>();
+
+const ClearedInput = type({
+  take: { output: TakeOutput },
+  rules: { output: { items: type("object[]").as<Item[]>() } },
+  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<Record<string, unknown>> {
+export async function cleared(input: unknown): Promise<Verdict> {
   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);
 }