fix(triage): keep the policy's group order when the hub stores a pull request's mail (CL-12197)

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

@@ -37,7 +37,7 @@ function decideLandings(rule: DecideRule, automatic: readonly ActionKind[]): Lan
 function admitsFirstTimers(pack: CheckPack, whens: readonly StepWhen[]): boolean {
   return FIRST_TIMERS.some(function admits(kind) {
     const author = { login: "", kind, teams: [] };
-    const subject = { author: { ...author, groups: authorGroupsOf(pack.groups ?? {}, author) }, linkedIssue: false, ci: "none" as const, files: 0, lines: 0 };
+    const subject = { author: { ...author, groups: authorGroupsOf(pack.groups ?? [], author) }, linkedIssue: false, ci: "none" as const, files: 0, lines: 0 };
     return whens.every((when) => whenHolds(when.author && { author: when.author }, subject).matches !== false);
   });
 }

apps/hub/src/manager-state.postgres.test.ts

@@ -3,6 +3,7 @@ import { expect, test } from "bun:test";
 import { randomBytes, randomUUID } from "node:crypto";
 import { sql } from "drizzle-orm";
 import { createDB, runMigrations, schema } from "@intx/db";
+import { authorGroupsOf, readCheckPack, readPolicyFile } from "@corbits/triage-contracts";
 import { closeOutDue, done, endNeverRan, notePlaced, recordEvent, take, type PullRequestRef } from "./manager-state.js";
 import { timings } from "./timings.js";
 import { migrateTriageTables } from "./triage-db.js";
@@ -118,3 +119,18 @@ test("PostgreSQL: a cancel that fails is logged and the reaper still ends the ne
   });
 }, 120_000);
 
+test("PostgreSQL: a pull request's mail keeps the policy's group order, so an author is in the first group the file lists that they match", async () => {
+  const policy = readPolicyFile("rules: []\ngroups:\n  approved: { users: [octocat] }\n  outside: { association: [CONTRIBUTOR] }\n", GITHUB.repo);
+  await withDatabase(async function stored(db) {
+    await recordEvent(db, TENANT, GITHUB, "d1", { type: "webhook", head: "h1", payload: { kind: "pr", headSha: "h1", pack: policy } });
+    await liveRun(db, "run_a");
+    const taken = await take(db, TENANT, GITHUB, "run_a");
+    if (taken.outcome !== "pass") throw new Error(`take answered ${taken.outcome}`);
+    const pack = readCheckPack(taken.events[0].event.payload.pack, GITHUB.repo);
+    const author = { login: "octocat", kind: "CONTRIBUTOR" as const, teams: [] };
+    expect(authorGroupsOf(pack.groups ?? [], author)).toEqual(["approved", "external", "everyone"]);
+    await recordEvent(db, TENANT, GITHUB, "d2", { type: "webhook", head: "h2", payload: { kind: "pr", headSha: "h2", pack: { ...policy, groups: { approved: { users: ["octocat"] }, outside: { association: ["CONTRIBUTOR"] } } } } });
+    const [stored] = await db.execute<{ state: { payload: { pack: unknown } } }>(sql`SELECT state FROM corbits_triage.manager_state WHERE key LIKE ${"%d2"}`);
+    expect(() => readCheckPack(stored.state.payload.pack, GITHUB.repo)).toThrow("stored before Triage kept group order");
+  });
+}, 120_000);

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

@@ -447,7 +447,7 @@ export type CheckPack = {
   /** So tools watching GitHub can react to it; absent, Triage writes no labels. */
   labels?: VerdictLabels;
   /** The groups the policy file defines, in its order, which steps name as `{ group }`. */
-  groups?: Record<string, FileGroup>;
+  groups?: NamedGroup[];
   /** What reading the stored policy left out because it no longer runs; each verdict says so. Never read back. */
   dropped?: string[];
 };
@@ -470,7 +470,6 @@ const ReviewTargetSchema = type({ "+": "reject", to: "'users'", users: Texts })
 
 export const GROUP_NAME_MAX = 64;
 
-/** Starting with a letter keeps a mapping of groups in the order the file wrote them. */
 export const GroupName = type(/^[a-z][a-z0-9]*(?:-[a-z0-9]+)*$/).atMostLength(GROUP_NAME_MAX);
 
 /** A policy file names a group as itself; the policy holds it as `{ group }` once read. */
@@ -488,6 +487,9 @@ export const fileGroupSchema = type({
 });
 export type FileGroup = typeof fileGroupSchema.infer;
 const FileGroupsSchema = type({ "[string]": fileGroupSchema });
+const NamedGroupsSchema = fileGroupSchema.merge({ name: GroupName }).array();
+/** A group the policy file defines, under its name; a pack lists them in the file's order. */
+export type NamedGroup = typeof NamedGroupsSchema.infer[number];
 const Count = type("number.integer").atLeast(0);
 const SizeBoundSchema = type({ "+": "reject", "over?": Count, "atMost?": Count });
 const stepWhenSchema = type({
@@ -586,7 +588,7 @@ export const checkPackSchema = type({
   "custom?": CustomCheckSchema.array().atMostLength(CUSTOM_CHECK_CAP),
   "actions?": PolicyActionsSchema,
   "labels?": type("true").or({ "+": "reject", "pass?": Label, "fail?": Label, "unsure?": Label }),
-  "groups?": FileGroupsSchema,
+  "groups?": NamedGroupsSchema,
 });
 
 const OverrideChecksSchema = type(
@@ -1153,11 +1155,23 @@ function distinctSteps(steps: Step[]): Step[] {
   });
 }
 
-function parseGroups(raw: unknown): Record<string, FileGroup> | undefined {
+/** The policy file's groups as a pack lists them, in the order the file wrote them. */
+export function namedGroups(groups: Readonly<Record<string, FileGroup>>): NamedGroup[] {
+  return Object.entries(groups).map(([name, group]) => ({ name, ...group }));
+}
+
+/** A pack stored before groups were listed holds a mapping whose order storage may have changed, so only one group is safe to read. */
+function parseGroups(raw: unknown): NamedGroup[] | undefined {
   if (raw === undefined) return undefined;
-  const groups = FileGroupsSchema(raw);
+  if (!Array.isArray(raw)) {
+    const mapped = FileGroupsSchema(raw);
+    if (mapped instanceof type.errors) throw new Error(`Groups ${mapped.summary}`);
+    if (Object.keys(mapped).length > 1) throw new Error("This pull request's policy was stored before Triage kept group order. Use Triage again.");
+    return parseGroups(namedGroups(mapped));
+  }
+  const groups = NamedGroupsSchema(raw);
   if (groups instanceof type.errors) throw new Error(`Groups ${groups.summary}`);
-  if (Object.keys(groups).some((name) => !GroupName.allows(name))) throw new Error("Group names must be lowercase words joined by hyphens.");
+  if (new Set(groups.map((group) => group.name)).size !== groups.length) throw new Error("Group names must be unique.");
   return groups;
 }
 
@@ -1385,7 +1399,7 @@ export function ruleLabel(id: string): string {
 /** Every team the policy names, in a step or in a group, whose membership can decide who a step is for. */
 export function policyTeams(pack: CheckPack): string[] {
   const steps = VERDICTS.flatMap((list) => pack.actions[list].flatMap((step) => (step.when?.author ?? []).flatMap((match) => (typeof match === "object" && "team" in match ? [match.team] : []))));
-  const groups = Object.values(pack.groups ?? {}).flatMap((group) => group.teams ?? []);
+  const groups = (pack.groups ?? []).flatMap((group) => group.teams ?? []);
   return [...new Set([...steps, ...groups])];
 }
 

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

@@ -123,7 +123,7 @@ export function exportPolicyFile(pack: CheckPack): PolicyFileExport {
   const rules = [...CATALOG_IDS.flatMap((id) => catalogRule(pack, id, unexpressed)), ...pack.custom.flatMap((row) => customRule(row, unexpressed))];
   const actions = fileActions(pack);
   const labels = fileLabels(pack);
-  const body = { ...(pack.groups && { groups: pack.groups }), rules, ...(actions && { actions }), ...(labels && { labels }) };
+  const body = { ...(pack.groups && { groups: Object.fromEntries(pack.groups.map(({ name, ...group }) => [name, group])) }), rules, ...(actions && { actions }), ...(labels && { labels }) };
   return { text: `# Corbits Triage policy.\n${stringify(body, { lineWidth: 0, aliasDuplicateObjects: false })}`, unexpressed };
 }
 

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

@@ -3,7 +3,7 @@
 // mapping it onto the stored policy's shape, so the stored policy's reader stays the one authority on what a policy may hold.
 import { type } from "arktype";
 import { asRecord } from "@corbits/common";
-import { CATALOG_IDS, CONVENTIONAL_FIELDS, CONVENTIONAL_TYPE, CustomId, defaultOnFail, Evidence, ISSUE_TRACKERS, Label, ON_FAILS, PatternRuleSchema, POLICY_EVENTS, RepoName, RISK_MARKS, RISK_SIGNALS, SIZE_RULES, stepListSchema, Text, TEXT_FIELDS, TEXT_MATCHES, VERDICTS, type CatalogId, type SizeRuleId } from "./check-pack.js";
+import { CATALOG_IDS, CONVENTIONAL_FIELDS, CONVENTIONAL_TYPE, CustomId, defaultOnFail, Evidence, ISSUE_TRACKERS, Label, namedGroups, ON_FAILS, PatternRuleSchema, POLICY_EVENTS, RepoName, RISK_MARKS, RISK_SIGNALS, SIZE_RULES, stepListSchema, Text, TEXT_FIELDS, TEXT_MATCHES, VERDICTS, type CatalogId, type SizeRuleId } from "./check-pack.js";
 import { PATTERN_PRESETS } from "./code-patterns.js";
 import { fileProblems } from "./policy-file-problems.js";
 import { withAuthors, WORKFLOW_KEYS, WORKFLOW_PROPS, workflowProblems, type PathProblem, type PolicyWorkflow } from "./policy-workflow.js";
@@ -190,7 +190,7 @@ export function readRulesFile(raw: Record<string, unknown>): { policy: Record<st
     ...mapRules(rules, onFail),
     ...(actions && { actions }),
     ...(labels && { labels }),
-    ...(workflow.groups && { groups: workflow.groups }),
+    ...(workflow.groups && { groups: namedGroups(workflow.groups) }),
   };
   return { policy, workflow: withDecideAuthors(workflow) };
 }

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

@@ -34,7 +34,7 @@ describe("policy file v4", () => {
   test("every v4 key reads as written beside the stored policy, which holds only the rules, and a step's groups read as who they stand for", () => {
     const checked = checkPolicyFile(V4, REPO);
     if (!checked.ok) throw new Error(JSON.stringify(checked.problems));
-    expect(checked.policy).toEqual({ ...readPolicyFile(RULES, REPO), groups: checked.workflow.groups });
+    expect(checked.policy).toEqual({ ...readPolicyFile(RULES, REPO), groups: [{ name: "maintainers", association: ["OWNER", "MEMBER", "COLLABORATOR"] }, { name: "core", teams: ["acme/core"], users: ["octocat"] }] });
     expect(checked.workflow).toEqual({
       groups: { maintainers: { association: ["OWNER", "MEMBER", "COLLABORATOR"] }, core: { teams: ["acme/core"], users: ["octocat"] } },
       "code-review": { on: true, for: ["maintainers"], provider: "anthropic", profile: "default", when: { triage: "pass", paths: ["!**/*.md", "!**/*.lock"] } },
@@ -252,26 +252,26 @@ describe("policy file v4", () => {
 });
 
 describe("author groups", () => {
-  const groups = {
-    core: { teams: ["acme/core"] },
-    maintainers: { users: ["octocat"] },
-    regulars: { association: ["CONTRIBUTOR" as const], users: ["octocat"] },
-  };
+  const groups = [
+    { name: "core", teams: ["acme/core"] },
+    { name: "maintainers", users: ["octocat"] },
+    { name: "regulars", association: ["CONTRIBUTOR" as const], users: ["octocat"] },
+  ];
   function author(over: Partial<StepAuthor>): StepAuthor {
     return { login: "octocat", kind: "CONTRIBUTOR", teams: [], ...over };
   }
 
   test("the first file group matching wins, built-in groups are facts no file group of that name replaced, and everyone always holds", () => {
     expect(authorGroupsOf(groups, author({}))).toEqual(["maintainers", "external", "everyone"]);
     expect(authorGroupsOf(groups, author({ teams: ["acme/core"], kind: "MEMBER" }))).toEqual(["core", "everyone"]);
-    expect(authorGroupsOf({}, author({ kind: "MEMBER" }))).toEqual(["maintainers", "everyone"]);
+    expect(authorGroupsOf([], author({ kind: "MEMBER" }))).toEqual(["maintainers", "everyone"]);
   });
 
   test("an unknown kind, or teams that can't be read deciding the first group, leaves the author's groups not known", () => {
     expect(authorGroupsOf(groups, author({ kind: undefined }))).toBeUndefined();
     expect(authorGroupsOf(groups, author({ teams: undefined }))).toBeUndefined();
     expect(authorGroupsOf(groups, author({ teamsUnknown: ["acme/core"] }))).toBeUndefined();
-    expect(authorGroupsOf({ maintainers: { users: ["octocat"] }, ...groups }, author({ teams: undefined }))).toEqual(["maintainers", "external", "everyone"]);
+    expect(authorGroupsOf([{ name: "maintainers", users: ["octocat"] }, ...groups], author({ teams: undefined }))).toEqual(["maintainers", "external", "everyone"]);
   });
 
   test("a step for a group holds by the author's groups, everyone always, and is not known without them", () => {

packages/triage-contracts/src/step-when.ts

@@ -1,4 +1,4 @@
-import { AUTHOR_GROUPS, type AuthorKind, type AuthorMatch, type CiState, type FileGroup, type SizeBound, type StepWhen } from "./check-pack.js";
+import { AUTHOR_GROUPS, type AuthorKind, type AuthorMatch, type CiState, type FileGroup, type NamedGroup, type SizeBound, type StepWhen } from "./check-pack.js";
 
 /** The group every author is in; it always matches. */
 export const EVERYONE = "everyone";
@@ -21,18 +21,18 @@ function inGroup(group: FileGroup, author: StepAuthor): boolean | undefined {
  * their kind is in that no file group replaces by name, then `everyone`. Failing closed, it is undefined, so that no
  * group but `everyone` holds, when the author's kind is not known or teams that can't be read would decide their group.
  */
-export function authorGroupsOf(groups: Readonly<Record<string, FileGroup>>, author: StepAuthor): string[] | undefined {
+export function authorGroupsOf(groups: readonly NamedGroup[], author: StepAuthor): string[] | undefined {
   const kind = author.kind;
   if (kind === undefined) return undefined;
   let own: string | undefined;
-  for (const [name, group] of Object.entries(groups)) {
+  for (const group of groups) {
     const held = inGroup(group, author);
     if (held === undefined) return undefined;
     if (!held) continue;
-    own = name;
+    own = group.name;
     break;
   }
-  const builtIn = Object.entries(AUTHOR_GROUPS).filter(([name, kinds]) => !Object.hasOwn(groups, name) && (kinds as readonly AuthorKind[]).includes(kind));
+  const builtIn = Object.entries(AUTHOR_GROUPS).filter(([name, kinds]) => !groups.some((group) => group.name === name) && (kinds as readonly AuthorKind[]).includes(kind));
   return [...(own === undefined ? [] : [own]), ...builtIn.map(([name]) => name), EVERYONE];
 }
 

packages/triage-manager/src/actions/review.ts

@@ -59,7 +59,7 @@ function capitalized(word: string): string {
 /** Why `for` leaves the author out, failing closed when their groups cannot be told. */
 function authorOutside(groups: readonly string[], { facts, pack }: ReadItem): string | undefined {
   if (groups.includes(EVERYONE)) return undefined;
-  const held = authorGroupsOf(pack.groups ?? {}, { login: facts.author, kind: facts.authorKind, teams: facts.authorTeams, teamsUnknown: facts.authorTeamsUnknown });
+  const held = authorGroupsOf(pack.groups ?? [], { login: facts.author, kind: facts.authorKind, teams: facts.authorTeams, teamsUnknown: facts.authorTeamsUnknown });
   const named = `the ${groups.join(" or ")} group`;
   if (held === undefined) return `Triage could not tell whether the author is in ${named}`;
   return held.some((group) => groups.includes(group)) ? undefined : `the author is not in ${named}`;

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

@@ -204,7 +204,7 @@ const CI: Record<PrFacts["checks"], CiState> = { success: "passing", failure: "f
 function stepSubject(facts: PrFacts, pack: CheckPack): StepSubject {
   const author = { login: facts.author, kind: facts.authorKind, teams: facts.authorTeams, teamsUnknown: facts.authorTeamsUnknown };
   return {
-    author: { ...author, groups: authorGroupsOf(pack.groups ?? {}, author) },
+    author: { ...author, groups: authorGroupsOf(pack.groups ?? [], author) },
     linkedIssue: issueReference(facts, "either") !== null,
     ci: CI[facts.checks],
     files: facts.changedFiles,