feat(triage): make labels off by default (CL-12165)

docs/Corbits Triage User Manual/Corbits Triage User Manual.md

@@ -400,7 +400,7 @@ Triage never merges a pull request while a person's review thread on it is open.
 
 ### Labels
 
-Triage keeps a verdict label on every pull request so that other tools, such as a coding agent, can react to it. It puts the verdict's label on the pull request and takes the other two off. The defaults are **Triage: Ready** for Pass, **Triage: Not Ready** for Fail and **Triage: Needs Human Review** for Unsure; the file can rename them. Triage writes the label once when it decides and changes it only when the verdict changes; a new commit takes it off until that commit's verdict, and while Triage waits only its comment shows progress. When it could not read the pull request it shows **Triage: Failed to Run** (see [What the author sees](#what-the-author-sees)). A step that labels each failed rule, shown as **Label each failed rule**, adds a label such as `rule: linked-issue` for every rule that failed. The label of a rule set to `onFail: label`, and the `sensitive: medium` and `risk: medium` labels, is added whatever the verdict, after the verdict's own steps, as an **Add labels** action: on its own where Triage adds labels on its own, otherwise waiting for you on Triage. The same action takes off the label of a rule that now passes, and a level label of sensitive paths or risk that no longer applies. A label a `labels` step in the policy also names is left alone.
+Triage adds no labels unless the policy file sets `labels: true`. With labels off, Triage takes its own verdict labels and **Triage: Failed to Run** off a pull request the first time it writes to it. With `labels: true`, or a map renaming them, Triage puts the verdict's label on the pull request and takes the other two off. The defaults are **Triage: Ready** for Pass, **Triage: Not Ready** for Fail and **Triage: Needs Human Review** for Unsure. Triage writes the label once when it decides and changes it only when the verdict changes; a new commit takes it off until that commit's verdict. When it could not read the pull request it shows **Triage: Failed to Run** (see [What the author sees](#what-the-author-sees)). A step that labels each failed rule, shown as **Label each failed rule**, adds a label such as `rule: linked-issue` for every rule that failed. The label of a rule set to `onFail: label`, and the `sensitive: medium` and `risk: medium` labels, is added whatever the verdict, after the verdict's own steps, as an **Add labels** action: on its own where Triage adds labels on its own, otherwise waiting for you on Triage. The same action takes off the label of a rule that now passes, and a level label of sensitive paths or risk that no longer applies. A label a `labels` step in the policy also names is left alone.
 
 On Bitbucket Data Center labels have no effect; see [Bitbucket Data Center repositories](#bitbucket-data-center-repositories).
 
@@ -547,7 +547,7 @@ The bar at the bottom of the pane always offers the next step first. For a passi
 
 ### What the author sees
 
-Authors do not use the portal. Depending on the repository's actions, they see a comment from Triage on their pull request with the verdict and reasons, review requests, and on GitHub labels such as **Triage: Ready**, **Triage: Not Ready** or **Triage: Needs Human Review** and assignments. On Bitbucket they see the verdict as the **Corbits Triage** build status instead of a label. A new commit takes the verdict label off once, so it never stands on a commit Triage has not checked, and the new verdict puts its label on; while Triage waits, its comment says what it waits for. When Triage could not read the pull request, or the repository's policy, it shows **Triage: Failed to Run** in place of any verdict label, or on Bitbucket a failed **Corbits Triage** build status that says **Triage couldn't run**, and posts nothing else. On GitHub it does this only where Triage adds labels on its own; on Bitbucket it always does. It clears on the next run that can read the pull request, which shows the verdict again. Triage keeps one comment and edits it in place as the pull request changes, so an author who pushes a fix sees the fresh verdict in the same comment. Each change Triage asks to split out names what it is not part of, the pull request's title, such as **Split out unrelated change: `src/b.ts`, not part of `feat: add search`**. When new commits change the verdict, the comment also says what the verdict was, how many commits arrived since and which pull requests were merged into the branch. Only when someone changes the repository's policy and that changes the verdict on the same commit does Triage post a new comment, which says it supersedes the one above.
+Authors do not use the portal. Depending on the repository's actions, they see a comment from Triage on their pull request with the verdict and reasons, review requests, and on GitHub labels such as **Triage: Ready**, **Triage: Not Ready** or **Triage: Needs Human Review** and assignments. On Bitbucket they see the verdict as the **Corbits Triage** build status instead of a label. A new commit takes the verdict label off once, so it never stands on a commit Triage has not checked, and the new verdict puts its label on; while Triage waits, its comment says what it waits for. When Triage could not read the pull request, or the repository's policy, it shows **Triage: Failed to Run** in place of any verdict label, or on Bitbucket a failed **Corbits Triage** build status that says **Triage couldn't run**, and posts nothing else. On GitHub it does this only where the policy file turns labels on and Triage adds labels on its own; on Bitbucket it always does. It clears on the next run that can read the pull request, which shows the verdict again. Triage keeps one comment and edits it in place as the pull request changes, so an author who pushes a fix sees the fresh verdict in the same comment. Each change Triage asks to split out names what it is not part of, the pull request's title, such as **Split out unrelated change: `src/b.ts`, not part of `feat: add search`**. When new commits change the verdict, the comment also says what the verdict was, how many commits arrived since and which pull requests were merged into the branch. Only when someone changes the repository's policy and that changes the verdict on the same commit does Triage post a new comment, which says it supersedes the one above.
 
 ## Settings: Triage, code review and people
 

packages/triage-contracts/src/check-pack.ts

@@ -686,7 +686,7 @@ const STARTER_REASONS = {
 } as const satisfies Partial<Record<CatalogId, string>>;
 
 /**
- * The first policy file for a repository with none: a chosen subset of the recommended rules, with no steps. It keeps
+ * The first policy file for a repository with none: a chosen subset of the recommended rules, with no steps or labels. It keeps
  * the rules that need no model and hold for any repository; a linked issue and forbidden paths are left out, since a
  * repository without an issue tracker, or one that edits `.github/**`, would fail every pull request.
  */
@@ -696,7 +696,8 @@ export function starterPack(repo: string): CheckPack {
   for (const [id, reason] of Object.entries(STARTER_REASONS) as Array<[keyof typeof STARTER_REASONS, string]>) {
     checks[id] = { ...recommended[id]!, reason, evidence: [MAINTAINER_STATED] };
   }
-  return { ...emptyPack(repo), checks, actions: { when: [...POLICY_EVENTS], pass: [], fail: [], unsure: [] } };
+  const { kind, schemaVersion, repo: clean, custom } = emptyPack(repo);
+  return { kind, schemaVersion, repo: clean, checks, custom, actions: { when: [...POLICY_EVENTS], pass: [], fail: [], unsure: [] } };
 }
 
 export function catalogCheckEnabled(pack: CheckPack, id: CatalogId): boolean {
@@ -1161,7 +1162,8 @@ function parseGroups(raw: unknown): Record<string, FileGroup> | undefined {
 }
 
 function parseLabels(raw: unknown): VerdictLabels | undefined {
-  if (raw === undefined || raw === true) return { ...VERDICT_LABELS };
+  if (raw === undefined) return undefined;
+  if (raw === true) return { ...VERDICT_LABELS };
   const row = record(raw, "labels");
   const extra = Object.keys(row).filter((key) => !(VERDICTS as readonly string[]).includes(key));
   if (extra.length) throw new Error(`labels must set only pass, fail or unsure, not ${extra.join(", ")}.`);

packages/triage-contracts/src/do.test.ts

@@ -10,6 +10,7 @@ const FILE = `rules:
     text: CI passes.
     reason: Required.
     evidence: [maintainer-stated]
+labels: true
 actions:
   pass: [{ do: assign, target: { to: author } }]
   unsure: [{ do: close }, { do: nothing }]

packages/triage-contracts/src/policy-export.ts

@@ -111,10 +111,10 @@ function fileActions(pack: CheckPack): Record<string, unknown> | undefined {
   return Object.keys(actions).length ? actions : undefined;
 }
 
-function fileLabels({ labels }: CheckPack): Record<string, string> | undefined {
+function fileLabels({ labels }: CheckPack): Record<string, string> | true | undefined {
   if (!labels) return undefined;
   const changed = VERDICTS.flatMap((verdict) => (labels[verdict] === VERDICT_LABELS[verdict] ? [] : [[verdict, labels[verdict]]]));
-  return changed.length ? Object.fromEntries(changed) : undefined;
+  return changed.length ? Object.fromEntries(changed) : true;
 }
 
 /** The schema 3 file for `pack`, and each thing it holds that the file cannot. */

packages/triage-workflows/src/actions/evaluate.ts

@@ -1,5 +1,5 @@
 import type { EffectContext } from "@intx/workflow";
-import { checkName, FAILED_LABEL, hostKinds, marksWaiting, ownedLabels, repoSourceOf, UNREAD_STATUS, VERDICT_LABELS, type ActionKind, type CheckPack, type RepoSource, type Step, type VerdictLabels } from "@corbits/triage-contracts";
+import { checkName, FAILED_LABEL, hostKinds, marksWaiting, ownedLabels, repoSourceOf, UNREAD_STATUS, type ActionKind, type CheckPack, type RepoSource, type Step, type VerdictLabels } from "@corbits/triage-contracts";
 import { type } from "arktype";
 import { approvalHeldBy, changesWanted, heldApprovals, holdDos, packApproves, stepActions, type ActionInput, type SuggestedAction } from "../logic/actions.js";
 import type { CheckResult, DeterministicResult, PrFacts } from "../logic/checks.js";
@@ -164,9 +164,8 @@ function degradedMark({ repo, number, headSha }: PrFacts, source: RepoSource, au
   return { effectId: `degraded:${repo.replace("/", "--")}:${number}:${headSha}`, repoSource: source, args };
 }
 
-/** Without a policy Triage labels with the default names. */
 function verdictLabelsOf(it: Item): VerdictLabels | undefined {
-  return it.pack ? it.pack.labels : VERDICT_LABELS;
+  return it.pack?.labels;
 }
 
 function degradedOf(it: Item, reason: string): Verdict {

packages/triage-workflows/src/pr-triage.test.ts

@@ -839,15 +839,15 @@ describe("a run that could not read the pull request", () => {
     expect(github.labels).toEqual(["bug", "Triage: Ready"]);
   });
 
-  test("a policy the hub can't read marks each head the mail names failed with why on both hosts, once, on GitHub only with the grant", async () => {
+  test("a policy the hub can't read marks each head the mail names failed with why, on GitHub with no label and the verdict label taken off, on Bitbucket once", async () => {
     const reason = "The policy file has unknown checks: max-size.";
     const mail = { kind: "pr", prNumber: 8, policy: { enabled: true }, policyUnreadable: reason, automatic: ACTION_KINDS, events: ["opened"] };
     const github = fakeGithub({ labels: ["bug", "Triage: Ready"] });
     const { outputs } = await runTriage(github.gh, [], false, hubMail({ ...mail, repo: REPO, prNumber: undefined, items: [{ prNumber: 8, headSha: github.pr.head }] }, "github@acme.test"));
     const [marked] = (outputs["evaluateRules"] as { items: Verdict[] }).items;
     expect(marked).toMatchObject({ repo: REPO, number: 8, degraded: "error", reason: `Policy can't be read: ${reason}`, actions: [] });
     expect(unsettledBy(marked!, github.pr.head)).toEqual({});
-    expect(github.labels).toEqual(["bug", "Triage: Failed to Run"]);
+    expect(github.labels).toEqual(["bug"]);
     const ungranted = fakeGithub({ labels: ["bug"] });
     await runTriage(ungranted.gh, [], false, hubMail({ ...mail, repo: REPO, headSha: ungranted.pr.head, automatic: [] }, "github@acme.test"));
     expect(writes(ungranted.requests)).toEqual([]);

skills/corbits-triage-setup/reference.md

@@ -97,7 +97,7 @@ To add a group to `sensitive-paths`, change the starter's rule. Writing `groups`
 
 ## Actions
 
-Optional. Without `actions`, Triage only posts its comment and verdict label (a build status on Bitbucket Data Center). Each verdict has an ordered list of at most 20 steps; a verdict left out has none.
+Optional. Without `actions`, Triage only posts its comment, and its verdict label where `labels` turns labels on (a build status on Bitbucket Data Center). Each verdict has an ordered list of at most 20 steps; a verdict left out has none.
 
 ```yaml
 actions:
@@ -150,7 +150,7 @@ Triage carries out an action on its own only where the workspace allows that act
 
 ## Labels
 
-Optional. Renames the verdict labels (at most 50 characters each). Defaults:
+Optional. Without `labels`, Triage adds no labels. `labels: true` turns the verdict labels on with their default names; a map renames them (at most 50 characters each), a verdict left out keeping its default:
 
 ```yaml
 labels:

tests/chain/chain.ts

@@ -42,7 +42,7 @@ const ASSIGN_ON_PASS = "actions:\n  pass:\n    - do: assign\n      target:\n
 const REVIEW_ON_PASS = "actions:\n  pass:\n    - do: request-review\n      target:\n        to: users\n        users:\n          - hubot\n";
 
 export function policy(rules: string[], extra = ""): string {
-  return `rules:\n${rules.map((rule) => `${rule}    reason: The maintainers set it.\n    evidence:\n      - maintainer-stated\n`).join("")}${extra}`;
+  return `rules:\n${rules.map((rule) => `${rule}    reason: The maintainers set it.\n    evidence:\n      - maintainer-stated\n`).join("")}labels: true\n${extra}`;
 }
 
 /** The judge finds every change focused, by the first of a choice's options, and no pull request steering it. */

tests/model/rules.postgres.test.ts

@@ -5,8 +5,8 @@ import { errorText } from "@corbits/common";
 import { isTriageComment } from "@corbits/github-reads";
 import { HTML_MARKERS, VERDICT_LABELS, emptyPack } from "@corbits/triage-contracts";
 import { CODE_REVIEW_RUNNING } from "@corbits/triage-workflows/logic/render";
-import { labelled, OWNER, type Write } from "../chain/github.ts";
-import { LETTERS, open, play, settle, walk, type Letter } from "./events.ts";
+import { labelled, OWNER, pullOf, sha, type Write } from "../chain/github.ts";
+import { LETTERS, open, play, settle, walk, type Letter, type Played } from "./events.ts";
 import { broken, verdictKept } from "./rules.ts";
 import { MANAGER_RELEASE, modelStack, REVIEW_RELEASE } from "./stack.ts";
 
@@ -30,13 +30,17 @@ const diesAfterMail = await modelStack(REPO, { pack: PACK, automatic: ["labels",
 const LABELLING: typeof PACK = { ...PACK, actions: { ...PACK.actions, when: ["opened"], pass: [{ do: "labels", target: { from: "list", labels: ["shipit"] } }] } };
 const labelling = await modelStack(`${OWNER}/labelling`, { pack: LABELLING, automatic: ["labels", "comment"] });
 const labellingReviewed = await modelStack(`${OWNER}/labelling-reviewed`, { pack: LABELLING, automatic: ["labels", "comment"], review: "dies-after-mail" });
+const UNLABELLED: typeof PACK = { ...PACK, repo: `${OWNER}/unlabelled` };
+delete UNLABELLED.labels;
+const unlabelled = await modelStack(UNLABELLED.repo, { pack: UNLABELLED, automatic: ["labels", "comment"] });
 setDefaultTimeout(5 * 60_000);
 afterAll(async () => {
   await stack.stop();
   await neverReady.stop();
   await diesAfterMail.stop();
   await labelling.stop();
   await labellingReviewed.stop();
+  await unlabelled.stop();
 });
 
 let pulls = 0;
@@ -164,3 +168,16 @@ test("with code review on, an action on opened runs once when the pass after CI
   await labellingReviewed.drain();
   expect(shipits(labellingReviewed, pull.number)).toEqual(["shipit"]);
 });
+
+test("with labels off, Triage adds no label and takes an earlier verdict label off once", async () => {
+  const number = ++pulls;
+  const pull: Played = { number, heads: [sha(`${number}-0`)], draft: false };
+  await unlabelled.open({ ...pullOf(number, pull.heads[0]!), labels: [VERDICT_LABELS.pass] });
+  await settle(unlabelled, pull);
+  await play(unlabelled, pull, "push");
+  await settle(unlabelled, pull);
+  const writes = unlabelled.writes(number);
+  const removed = writes.filter((write) => write.method === "DELETE" && write.path.includes("/labels/")).map((write) => decodeURIComponent(write.path.split("/labels/")[1]!));
+  expect(labelled(writes)).toEqual([]);
+  expect(removed).toEqual([VERDICT_LABELS.pass]);
+});