fix(contracts): report a level label in a labels step instead of failing the whole stored pack

apps/hub/src/github/repo-policy.ts

@@ -45,7 +45,7 @@ async function fileResolved(deps: RepoPolicyDeps, tenantId: string, repo: string
   const shared = extendsOf(content);
   if (shared !== undefined) return overSharedFile(deps.policyFile, tenantId, repo, source, content, shared);
   refuseUnknownChecks(content);
-  return { pack: readCheckPack(content, repo) };
+  return { pack: readCheckPack(content, repo, true) };
 }
 
 /** Why triage would refuse a policy file holding `policy`, resolved as a pushed file is; undefined when it would run it. */

packages/triage-contracts/README.md

@@ -11,7 +11,7 @@ and trigger mail. Every app and most packages import it.
 
 A policy sets which rules run on a repository's pull requests and what happens after. Its rules are stored under `checks` (built-in) and `custom`. `GET /api/integrations/pack-schema` returns its JSON Schema as `checkPack`, the part of a policy file that extends another repository's file as `policyOverride`, the repository settings as `repoPolicy`, and `.corbits-triage.yml` as `policyFile`: schema 3, a list of `rules` (each `text`, `check`, `reason` and `evidence`), `actions` (`on` and steps per verdict) and `labels`, which `readPolicyFile` maps onto the stored shape; `checkPolicyFile` answers the same or each problem with its line, and the file's v4 keys as `workflow`. Those keys (`policy-workflow.ts`) are `groups` (each `association`, `teams` or `users`), one key per block, `code-review` (`on`, and optionally `for`, `when: { triage, paths, rules }` and `provider`; a file setting the retired `clean` is refused with a pointer to a step's `when.findings`), with `security` and `deslop` reserved: `checkPolicyFile` reads a file without them and without any `decide` rule with a condition on one, names each as `pending`, and refuses the file as written if the rest is refused too; `patchPolicyFile` refuses a file still holding them unless the batch has `strip-reserved`, which removes them. The other v4 keys are `pace`, `pauseLabel` and `decide` (rules `{ if, do }`, `if` on `all`, `triage`, `event`, `author` and each block's outcome); they are validated and returned as written, and nothing acts on them yet except steps naming a group and, on GitHub, `pauseLabel`, whose label on a pull request stops the hub starting runs for it. A file that sets `extends` holds none of them. Groups are matched first match wins, in the file's order: an author is in at most one of the file's groups, then in each built-in group (`maintainers`, `external`, `first-timers`, `bots`) their author kind puts them in, which are facts matched independently, then in `everyone`, which always matches; a file group with a built-in group's name replaces it. `authorGroupsOf(groups, author)` answers that set, failing closed: it answers none, so no `{ group }` but `everyone` holds, when the author's kind is not known or teams that can't be read would decide their file group. A step's built-in group reads as author kinds, so it still holds when teams can't be read, and is not known only when the author's kind is. The teams read for an author are every team the policy names, in a step or a group (`policyTeams`). A step's `when.author` names groups as the policy holds them, `{ group }` for a file group or `everyone` and a built-in group's kinds otherwise, and holds when the author's set includes it; approving or merging must never rest on `everyone` alone. `{ all: clean }` holds when at least one block ran and every block that ran is clean; skipped blocks never count, so it fails when every block was skipped or none is on. A rule that approves or merges without `triage` matches only Pass (`ruleVerdicts`). Incomplete is never clean: a rule that approves or merges can't name `incomplete`. A file that sets `actions` and no `decide` gets its `decide` from `decideOf`: one rule `{ if: { triage: <verdict> }, do: <its steps> }` per verdict with steps, `do: nothing` for a Pass or Fail with none, and no rule for an Unsure with none, each limited to `event: <actions.on>` when the actions wake on fewer than every event, every step as written; a file that extends another has no `decide`. A `decide` rule can't use `review` yet. A file setting any v4 key is read as schema 3 and needs `rules`. Problems name their line from the YAML parser's node positions (`yaml`, core schema, aliases and the keys `__proto__`, `constructor` and `prototype` refused); schema errors are said in the file's terms by `policy-file-problems.ts`. A file without `rules` or a v4 key is read as a policy or override without `kind`, `schemaVersion` and `repo`. Constraints the schema cannot state are checked when the policy is read: a custom rule's `options` and `failOn` must not repeat and `failOn` must be among `options`, a score's `failBelow` must be one of its levels after the first, and a custom id must not be a built-in rule's or read as a judge question's. A rule that fails any of these rejects the policy with its reason. Custom rules are decision-model questions (`kind: "model"`): `is-true`, `score` or `choose`, or text rules (`kind: "text"`) that fail when any of `fields` (`title`, `description`, `commits`) contains one of `phrases`, or with `failWhen: "not-contains"` when one contains none, ignoring case; a stored rule of the retired `kind: "rule"` rejects the policy. The reader ignores unknown built-in rows. Built-in rule ids renamed earlier (`size`, `issue`, `conflicts`, `ci`, `paths`, `draft`, `review`) are still read and mapped to their new ids by `RENAMED_RULE_IDS`; an old `size` row loads as both size limits. A new custom rule's id is its name as lowercase words joined by hyphens, numbered past any taken id; it never starts with `docs-` or `focused-` or ends in `-file`, `-hunk` or `-slice` and numbers, as judge question ids do.
 
-`actions` holds one `when` (the events it wakes on) and an ordered list of steps per verdict. Each step is `{ do, target?, when? }`; its `when` holds conditions that must all hold for it to run (`whenHolds`): `author` (any of GitHub's `author_association` values `OWNER`, `MEMBER`, `COLLABORATOR`, `CONTRIBUTOR`, `FIRST_TIME_CONTRIBUTOR`, `FIRST_TIMER`, `NONE`, `MANNEQUIN`, or `BOT` for a bot whatever its association, `{ team }`, `{ users }`; a policy naming a group of `AUTHOR_GROUPS`, `maintainers`, `external`, `first-timers` or `bots`, reads it as exactly that group's kinds), `linkedIssue`, `ci` (any of `passing`, `failing`, `pending`, `none`), `files` and `lines` as `{ over?, atMost? }`, and `findings` as `{ above: P0–P3 }` or `{ any: boolean }`, which `whenHolds` leaves to the manager once code review reports and a policy file accepts only with `code-review` on. Every rule row may carry `text`, `reason` and `evidence` (`#44`, `owner/name#44`, https links or `maintainer-stated`). An action runs on its own only when the workspace's `pr-triage` system role holds `invoke` on `triage-do:<kind>:<owner>/<name>`; otherwise it waits for a person holding that grant, and `close` on `unsure` and `merge` on anything but `pass` always wait. `labels` names the label the mirror writes for each verdict; a verdict it leaves out keeps its default (`Triage: Ready`, `Triage: Not Ready`, `Triage: Needs Human Review`). Repository roles were removed, so a step whose target is a role is left out and named in the pack's `dropped`.
+`actions` holds one `when` (the events it wakes on) and an ordered list of steps per verdict. Each step is `{ do, target?, when? }`; its `when` holds conditions that must all hold for it to run (`whenHolds`): `author` (any of GitHub's `author_association` values `OWNER`, `MEMBER`, `COLLABORATOR`, `CONTRIBUTOR`, `FIRST_TIME_CONTRIBUTOR`, `FIRST_TIMER`, `NONE`, `MANNEQUIN`, or `BOT` for a bot whatever its association, `{ team }`, `{ users }`; a policy naming a group of `AUTHOR_GROUPS`, `maintainers`, `external`, `first-timers` or `bots`, reads it as exactly that group's kinds), `linkedIssue`, `ci` (any of `passing`, `failing`, `pending`, `none`), `files` and `lines` as `{ over?, atMost? }`, and `findings` as `{ above: P0–P3 }` or `{ any: boolean }`, which `whenHolds` leaves to the manager once code review reports and a policy file accepts only with `code-review` on. Every rule row may carry `text`, `reason` and `evidence` (`#44`, `owner/name#44`, https links or `maintainer-stated`). An action runs on its own only when the workspace's `pr-triage` system role holds `invoke` on `triage-do:<kind>:<owner>/<name>`; otherwise it waits for a person holding that grant, and `close` on `unsure` and `merge` on anything but `pass` always wait. `labels` names the label the mirror writes for each verdict; a verdict it leaves out keeps its default (`Triage: Ready`, `Triage: Not Ready`, `Triage: Needs Human Review`). Repository roles were removed, so a step whose target is a role is left out and named in the pack's `dropped`. So is a labels step naming a label Triage takes back (a `sensitive: …` or `risk: …` level label): the whole step is left out and named in `dropped` with its position and its other labels, and the rest of the pack loads; a policy file or an override naming one is refused instead.
 
 ```json
 {

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

@@ -230,11 +230,22 @@ describe("check pack contract", () => {
       [{ ...ACTIONS, pass: [{ do: "comment", target: {} }] }, "Pass step 1 target must set exactly body."],
       [{ ...ACTIONS, pass: [{ do: "approve", for: "staff" }] }, "Pass step 1 for must be one of OWNER, MEMBER, COLLABORATOR, CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR, FIRST_TIMER, NONE, MANNEQUIN, BOT."],
       [{ ...ACTIONS, fail: [{ kind: "comment", target: { body: "Hi" } }] }, "Fail step 1 must not set kind."],
-      [{ ...ACTIONS, pass: [{ do: "labels", target: { from: "list", labels: ["Risk: High"] } }] }, "Pass step 1 label Risk: High is one Triage takes back; name another."],
     ];
     for (const [actions, reason] of rejected) expect(() => readCheckPack({ ...emptyPack(REPO), actions })).toThrow(reason);
   });
 
+  test("a labels step holding a level label is left out and named, and the rest of the pack loads", () => {
+    const level = { do: "labels", target: { from: "list", labels: ["Risk: High"] } };
+    const withOthers = { do: "labels", target: { from: "list", labels: ["needs-qa", "Risk: High", "sensitive: MEDIUM", "risk: high"] } };
+    const read = readCheckPack({ ...emptyPack(REPO), actions: { ...ACTIONS, pass: [COMMENT, COMMENT, level, withOthers] } });
+    expect(read.actions.pass).toEqual([COMMENT]);
+    expect(read.dropped).toEqual([
+      "Pass step 3 label Risk: High is one Triage takes back, so the whole step is skipped",
+      "Pass step 4 label Risk: High is one Triage takes back, so the whole step is skipped; its other labels (needs-qa) are not added either",
+    ]);
+    expect(() => readCheckPack({ ...emptyPack(REPO), actions: { ...ACTIONS, pass: [level] } }, REPO, true)).toThrow("Pass step 1 label Risk: High is one Triage takes back; name another.");
+  });
+
   test("the schema accepts what the parser reads, and a rejection says why", () => {
     const accepted = [
       { kind: "corbits.triage.check-pack", schemaVersion: 2, repo: REPO },

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

@@ -1129,15 +1129,23 @@ function parseStep(row: Record<string, unknown>, where: string): Step {
   }
 }
 
-/** Repository roles were removed, so a step for one is left out and named in `dropped`. */
-function parseList(raw: unknown, where: string, dropped: string[]): Step[] {
+/** The first label of a labels step that Triage takes back, which a step naming would flip on and off across runs. */
+function levelLabelOf(step: Step): string | undefined {
+  return step.do === "labels" && step.target.from === "list" ? step.target.labels.find((label) => levelLabelRule(label) !== undefined) : undefined;
+}
+
+/**
+ * Repository roles were removed, so a step for one is left out and named in `dropped`. A step naming a level label is
+ * left out and named the same way, or refused when `refuse` is set, as a policy file is.
+ */
+function parseList(raw: unknown, where: string, dropped: string[], refuse: boolean): Step[] {
   if (!Array.isArray(raw)) throw new Error(`${where} must be a list.`);
   if (raw.length > STEPS_MAX) throw new Error(`${where} must list at most ${STEPS_MAX} steps.`);
   return raw.flatMap(function stepAt(item, i) {
     const at = `${where} ${i + 1}`;
     const row = record(item, at);
     const role = asRecord(row.target)?.to === "role" ? asRecord(row.target)?.role : undefined;
-    if (role === undefined) return [parseStep(row, at)];
+    if (role === undefined) return stepWithoutLevelLabel(parseStep(row, at), at, dropped, refuse);
     dropped.push(`${at} (${String(row.do)} role ${String(role)}) no longer runs`);
     return [];
   });
@@ -1176,17 +1184,26 @@ function parseLabels(raw: unknown): Record<VerdictWord, string> {
   return Object.fromEntries(VERDICTS.map((verdict) => [verdict, row[verdict] === undefined ? VERDICT_LABELS[verdict] : text(row[verdict], `labels ${verdict}`, LABEL_MAX)])) as Record<VerdictWord, string>;
 }
 
+function stepWithoutLevelLabel(step: Step, at: string, dropped: string[], refuse: boolean): Step[] {
+  const level = levelLabelOf(step);
+  if (level === undefined) return [step];
+  if (refuse) throw new Error(`${at} label ${level} is one Triage takes back; name another.`);
+  const others = step.do === "labels" && step.target.from === "list" ? step.target.labels.filter((label) => levelLabelRule(label) === undefined) : [];
+  dropped.push(`${at} label ${level} is one Triage takes back, so the whole step is skipped${others.length > 0 ? `; its other labels (${others.join(", ")}) are not added either` : ""}`);
+  return [];
+}
+
 /** Packs written before verdicts stored an empty list of actions; one that is not empty is no longer read. */
-function parseActions(raw: unknown, dropped: string[]): PolicyActions {
+function parseActions(raw: unknown, dropped: string[], refuse: boolean): PolicyActions {
   if (raw === undefined || (Array.isArray(raw) && raw.length === 0)) return { when: [...POLICY_EVENTS], pass: [], fail: [], unsure: [] };
   if (Array.isArray(raw)) throw new Error("Actions must be { when, pass, fail, unsure }; a list of actions with yes, no, unsure and always branches is no longer read.");
   const row = record(raw, "Actions");
   exactKeys(row, ["when", ...VERDICTS], "Actions");
   return {
     when: parseEvents(row.when, "Actions when"),
-    pass: distinctSteps(parseList(row.pass, "Pass step", dropped)),
-    fail: distinctSteps(parseList(row.fail, "Fail step", dropped)),
-    unsure: distinctSteps(parseList(row.unsure, "Unsure step", dropped)),
+    pass: distinctSteps(parseList(row.pass, "Pass step", dropped, refuse)),
+    fail: distinctSteps(parseList(row.fail, "Fail step", dropped, refuse)),
+    unsure: distinctSteps(parseList(row.unsure, "Unsure step", dropped, refuse)),
   };
 }
 
@@ -1283,8 +1300,8 @@ function withDropped<T extends object>(read: T, dropped: string[]): T & { droppe
   return { ...read, ...(dropped.length > 0 && { dropped }) };
 }
 
-/** Reads a stored pack, throwing an Error whose message says why it is rejected. Version 1 reads as version 2. */
-export function readCheckPack(raw: unknown, expectedRepo?: string): CheckPack {
+/** Reads a stored pack, throwing an Error whose message says why it is rejected. Version 1 reads as version 2. A policy file passes `refuse`, so a step naming a label Triage takes back is refused where a stored pack leaves it out and names it in `dropped`. */
+export function readCheckPack(raw: unknown, expectedRepo?: string, refuse = false): CheckPack {
   const stored = policyObject(raw);
   if (stored.schemaVersion !== LEGACY_SCHEMA_VERSION) currentVersion(stored);
   const dropped: string[] = [];
@@ -1293,7 +1310,7 @@ export function readCheckPack(raw: unknown, expectedRepo?: string): CheckPack {
   const repo = readRepo(body, expectedRepo);
   const checks = readChecks(body.checks);
   const custom = parseCustom(body.custom);
-  const actions = parseActions(body.actions, dropped);
+  const actions = parseActions(body.actions, dropped, refuse);
   const labels = parseLabels(body.labels);
   const groups = parseGroups(body.groups);
   return withDropped(refuseTriageLabels({ kind: CHECK_PACK_KIND, schemaVersion: CHECK_PACK_SCHEMA_VERSION, repo, checks, custom, actions, labels, ...(groups && { groups }) }), dropped);
@@ -1322,7 +1339,7 @@ export function readSharedPolicy(raw: unknown): SharedPolicy {
   const checks = readChecks(body.checks);
   const dropped: string[] = [];
   const custom = parseCustom(body.custom);
-  const actions = parseActions(body.actions, dropped);
+  const actions = parseActions(body.actions, dropped, false);
   const labels = parseLabels(body.labels);
   return withDropped({ kind: CHECK_PACK_KIND, schemaVersion: CHECK_PACK_SCHEMA_VERSION, checks, custom, actions, labels }, dropped);
 }
@@ -1344,7 +1361,7 @@ function parseOverrideActions(row: Partial<Record<"when" | VerdictWord, unknown>
   const actions: Partial<PolicyActions> = {};
   if (row?.when !== undefined) actions.when = parseEvents(row.when, "Actions when");
   for (const verdict of VERDICTS) {
-    if (row?.[verdict] !== undefined) actions[verdict] = distinctSteps(parseList(row[verdict], STEP_WHERE[verdict], []));
+    if (row?.[verdict] !== undefined) actions[verdict] = distinctSteps(parseList(row[verdict], STEP_WHERE[verdict], [], true));
   }
   return actions;
 }
@@ -1443,13 +1460,6 @@ export function refuseTriageLabels(pack: CheckPack): CheckPack {
     const level = levelLabelRule(lower);
     if (lower.startsWith("triage:") || lower.startsWith(RULE_LABEL_PREFIX) || verdictLabels.includes(lower) || (level !== undefined && level !== id)) throw new Error(`Rule ${id} label ${label} is one Triage takes back; name another.`);
   }
-  for (const list of VERDICTS) {
-    for (const [i, step] of pack.actions[list].entries()) {
-      if (step.do !== "labels" || step.target.from !== "list") continue;
-      const level = step.target.labels.find((label) => levelLabelRule(label) !== undefined);
-      if (level !== undefined) throw new Error(`${STEP_WHERE[list]} ${i + 1} label ${level} is one Triage takes back; name another.`);
-    }
-  }
   return pack;
 }
 

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

@@ -113,7 +113,7 @@ function policyOf(file: Record<string, unknown>, repo: string): { policy: Record
   const header = { kind: CHECK_PACK_KIND, schemaVersion: CHECK_PACK_SCHEMA_VERSION } as const;
   const { policy: body, workflow } = readFile(file);
   if (body.extends !== undefined) return { policy: { ...header, ...overrideOf({ ...body, ...header, repo }, repo) }, workflow };
-  const { dropped, ...pack } = readCheckPack({ ...body, ...header, repo }, repo);
+  const { dropped, ...pack } = readCheckPack({ ...body, ...header, repo }, repo, true);
   if (dropped) throw new Error(`The policy file holds what no longer runs: ${refusalNames(dropped)}.`);
   return { policy: pack, workflow: { ...workflow, decide: workflow.decide ?? decideOf(pack.actions) } };
 }

packages/triage-contracts/src/policy-rules.test.ts

@@ -134,6 +134,8 @@ actions:
     expect(checkPolicyFile(`rules:\n${rule}    label: docs\n`, REPO)).toEqual({ ok: false, problems: [{ message: "Rule docs label needs onFail: label.", line: 2 }] });
     expect(checkPolicyFile(`rules:\n${rule}    onFail: label\n    label: "rule: docs"\n`, REPO)).toEqual({ ok: false, problems: [{ message: "Rule docs label rule: docs is one Triage takes back; name another." }] });
     expect(checkPolicyFile(`rules:\n${rule}    onFail: label\n    label: "sensitive: medium"\n`, REPO)).toEqual({ ok: false, problems: [{ message: "Rule docs label sensitive: medium is one Triage takes back; name another." }] });
+    const step = `rules:\n${rule}actions:\n  pass: [{ do: comment, target: { body: Hi } }, { do: labels, target: { from: list, labels: ["Risk: High", needs-qa] } }]\n`;
+    expect(checkPolicyFile(step, REPO)).toEqual({ ok: false, problems: [{ message: "Pass step 2 label Risk: High is one Triage takes back; name another." }] });
   });
 
   test("sensitive-paths reads its groups and risk its levels, both ask unless the file says, survive export, and a bad group name, signal or count is refused", () => {