fix(triage): strip Triage's labels when a pull request closes before its verdict (CL-12164)

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

@@ -362,7 +362,7 @@ Triage reads a pull request's title, description, commit messages and file names
 
 Rules set to `onFail: label` take no part in any of these steps: they only label and note the pull request. A policy whose only Judgement rules only label is decided as one with none.
 
-Draft pull requests are not decided at all, and a closed pull request gets no verdict. When a pull request is merged or closed before Triage decides its last commit, Triage's comment on it says **Merged before Triage finished** or **Closed before Triage finished** with that commit's short id, and on GitHub Triage's labels come off it. A verdict Triage already gave on the last commit stays, with its labels. Where Triage labels pull requests but does not comment, only its waiting labels come off. On Bitbucket Data Center the commit's Corbits Triage build status stays as it was.
+Draft pull requests are not decided at all, and a closed pull request gets no verdict. When a pull request is merged or closed before Triage decides its last commit, Triage's comment on it says **Merged before Triage finished** or **Closed before Triage finished** with that commit's short id, and on GitHub Triage's labels come off it. Where Triage may post on GitHub but does not comment, only its old, retired labels come off. A verdict Triage already gave on the last commit stays, with its labels. On Bitbucket Data Center the commit's Corbits Triage build status stays as it was.
 
 ### Actions
 

packages/github-tool/src/github.ts

@@ -68,7 +68,8 @@ export type CloseOutInput = { repo: string; number: number; onHead: string; owne
 
 /**
  * A pull request that merged or closed before its head's verdict: where `comment` is set, its comment says so in place
- * of progress, and the `owned` labels come off with it. A verdict already shown on the head stays, as do its labels.
+ * of progress. The `owned` labels come off whether or not `comment` is set, unless the comment shows a verdict on the
+ * head, which stays with its labels.
  */
 export async function closeOut(gh: GithubFetch, i: CloseOutInput, login: string) {
   const { repo, number } = i;

packages/github-tool/src/sidecar-bundle.ts

@@ -177,7 +177,7 @@ const MIRROR_FIELDS = {
   fixedHead: str,
   onHead: { ...str, description: "Write nothing once the pull request has moved past this head" },
   keepVerdict: { ...obj({ verdict: str, policy: POLICY_REF }), description: "The pass's verdict and policy: a comment that already shows them for onHead is kept rather than written over" },
-  closedOut: { ...obj({ merged: { type: "boolean" }, comment: { type: "boolean" } }), description: "Only on a merged or closed pull request, with onHead: with comment, the triage comment says it merged or closed before Triage finished, and owned labels come off, unless the comment shows a verdict on onHead" },
+  closedOut: { ...obj({ merged: { type: "boolean" }, comment: { type: "boolean" } }), description: "Only on a merged or closed pull request, with onHead: with comment, the triage comment says it merged or closed before Triage finished; owned labels come off whether or not comment is set, unless the comment shows a verdict on onHead" },
 };
 const MIRROR_REQUIRED = ["repo", "number", "labels"];
 const MIRROR_INPUT = obj({ ...MIRROR_FIELDS, appLogin: APP_LOGIN }, [...MIRROR_REQUIRED, "appLogin"]);

packages/triage-contracts/src/presets.ts

@@ -11,7 +11,7 @@ const OWNERS: Record<WorkflowState, string> = {
 };
 
 // Labels earlier versions set; owning them clears them from a pull request the next time it is triaged.
-const RETIRED_LABELS = [
+export const RETIRED_LABELS: readonly string[] = [
   "Triage: Needs changes",
   "Triage: Needs review",
   "Triage: Ready to merge",

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

@@ -1,5 +1,5 @@
 import type { EffectContext } from "@intx/workflow";
-import { ownedLabels, parseCheckPack, repoSourceOf, trustedRequestOf, WAITING_STATUS, type Awaits, type KeptVerdict, type RepoSource, type VerdictLabels } from "@corbits/triage-contracts";
+import { ownedLabels, parseCheckPack, postingKinds, RETIRED_LABELS, repoSourceOf, trustedRequestOf, WAITING_STATUS, type Awaits, type KeptVerdict, type RepoSource, type VerdictLabels } from "@corbits/triage-contracts";
 import { progressComment, type WaitingMark } from "../logic/render.js";
 
 type Head = { repo: string; number: number; headSha: string };
@@ -22,18 +22,22 @@ export function waitingMark(source: RepoSource, { repo, number, headSha }: Head,
 }
 
 /**
- * What a pull request that merged or closed on `headSha` before its verdict shows where triage comments: its comment
- * says Triage did not finish, and Triage's labels come off where it labels. Without its comment a verdict on the head
- * cannot be told from a stale one, so nothing is written.
+ * What a pull request that merged or closed on `headSha` before its verdict shows: where triage comments, its comment
+ * says Triage did not finish, and on GitHub Triage's labels come off. Without its comment a verdict on the head cannot be
+ * told from a stale one, so only the retired labels come off, where the workspace lets it post.
  */
 export function closedMark(source: RepoSource, { repo, number, headSha, merged }: Head & { merged: boolean }, automatic: readonly unknown[], labels: VerdictLabels | undefined, appLogin: string | undefined): WaitingMark | undefined {
   const comment = automatic.includes("comment");
-  if (!comment) return undefined;
-  const owned = source === "github" && automatic.includes("labels") && labels ? ownedLabels(labels) : [];
+  if (!comment && (source !== "github" || !postingKinds(source, labels).some((kind) => automatic.includes(kind)))) return undefined;
+  const owned = source === "github" ? closedOwned(comment, labels) : [];
   const args = { repo, number, onHead: headSha, labels: [], owned, closedOut: { merged, comment }, ...(appLogin && { appLogin }) };
   return { effectId: `closed:${repo.replace("/", "--")}:${number}:${headSha}`, repoSource: source, args };
 }
 
+function closedOwned(comment: boolean, labels: VerdictLabels | undefined): readonly string[] {
+  return comment ? ownedLabels(labels) : RETIRED_LABELS;
+}
+
 /**
  * The first pass on a GitHub head takes the earlier verdict's label off before facts; a later event on the same head
  * writes no label, and the head's own verdict puts its label on.

tests/model/rules.postgres.test.ts

@@ -33,6 +33,8 @@ const labellingReviewed = await modelStack(`${OWNER}/labelling-reviewed`, { pack
 const UNLABELLED: typeof PACK = { ...PACK, repo: `${OWNER}/unlabelled` };
 delete UNLABELLED.labels;
 const unlabelled = await modelStack(UNLABELLED.repo, { pack: UNLABELLED, automatic: ["labels", "comment"] });
+const labelsOnly = await modelStack(`${OWNER}/labels-only`, { pack: { ...PACK, repo: `${OWNER}/labels-only` }, automatic: ["labels"] });
+const ungranted = await modelStack(`${OWNER}/ungranted`, { pack: { ...PACK, repo: `${OWNER}/ungranted` }, automatic: [] });
 const MERGING: typeof PACK = { ...PACK, repo: `${OWNER}/merging`, actions: { ...PACK.actions, fail: [{ do: "merge" }] } };
 const merging = await modelStack(MERGING.repo, { pack: MERGING, automatic: ["labels", "comment", "merge"] });
 setDefaultTimeout(5 * 60_000);
@@ -43,6 +45,8 @@ afterAll(async () => {
   await labelling.stop();
   await labellingReviewed.stop();
   await unlabelled.stop();
+  await labelsOnly.stop();
+  await ungranted.stop();
   await merging.stop();
 });
 
@@ -185,6 +189,39 @@ test("with labels off, Triage adds no label and takes an earlier verdict label o
   expect(removed).toEqual([VERDICT_LABELS.pass]);
 });
 
+test("without the comment, a pull request closed before its verdict loses Triage's labels", async () => {
+  const number = ++pulls;
+  const pull: Played = { number, heads: [sha(`${number}-0`)], draft: false };
+  await labelsOnly.open({ ...pullOf(number, pull.heads[0]!), labels: ["Triage: Waiting for CI"] });
+  await labelsOnly.drain();
+  await play(labelsOnly, pull, "close");
+  await settle(labelsOnly, pull);
+  const writes = labelsOnly.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(["Triage: Waiting for CI"]);
+});
+
+test("without the comment, a pull request closed after its verdict keeps the verdict label", async () => {
+  const pull = await open(labelsOnly, ++pulls);
+  await settle(labelsOnly, pull);
+  const [verdict] = labelled(labelsOnly.writes(pull.number));
+  await play(labelsOnly, pull, "close");
+  await settle(labelsOnly, pull);
+  expect(verdict).toBeDefined();
+  expect(labelsOnly.github.pull(labelsOnly.repo, pull.number).labels).toContain(verdict!);
+});
+
+test("without any posting grant, a pull request closed before its verdict is not written to", async () => {
+  const number = ++pulls;
+  const pull: Played = { number, heads: [sha(`${number}-0`)], draft: false };
+  await ungranted.open({ ...pullOf(number, pull.heads[0]!), labels: ["Triage: Waiting for CI"] });
+  await ungranted.drain();
+  await play(ungranted, pull, "close");
+  await settle(ungranted, pull);
+  expect(ungranted.writes(number)).toEqual([]);
+});
+
 test("a merge waits while a reviewer requests changes", async () => {
   const number = ++pulls;
   const pull: Played = { number, heads: [sha(`${number}-0`)], draft: false };