diff --git a/apps/hub/src/github/policy-read.ts b/apps/hub/src/github/policy-read.ts index 447b8fe0..7639b9ee 100644 --- a/apps/hub/src/github/policy-read.ts +++ b/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); }); } diff --git a/apps/hub/src/manager-state.postgres.test.ts b/apps/hub/src/manager-state.postgres.test.ts index 8fa1fd78..f3d978fd 100644 --- a/apps/hub/src/manager-state.postgres.test.ts +++ b/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); diff --git a/packages/triage-contracts/src/check-pack.ts b/packages/triage-contracts/src/check-pack.ts index 3a45ae35..b2eac6e6 100644 --- a/packages/triage-contracts/src/check-pack.ts +++ b/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; + 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 | undefined { +/** The policy file's groups as a pack lists them, in the order the file wrote them. */ +export function namedGroups(groups: Readonly>): 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])]; } diff --git a/packages/triage-contracts/src/policy-export.ts b/packages/triage-contracts/src/policy-export.ts index faf4e349..f10b74d3 100644 --- a/packages/triage-contracts/src/policy-export.ts +++ b/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 }; } diff --git a/packages/triage-contracts/src/policy-rules.ts b/packages/triage-contracts/src/policy-rules.ts index 2e1cc043..c087d4de 100644 --- a/packages/triage-contracts/src/policy-rules.ts +++ b/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): { policy: Record { 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,11 +252,11 @@ 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 { return { login: "octocat", kind: "CONTRIBUTOR", teams: [], ...over }; } @@ -264,14 +264,14 @@ describe("author groups", () => { 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", () => { diff --git a/packages/triage-contracts/src/step-when.ts b/packages/triage-contracts/src/step-when.ts index 7ff7ee3c..a5a05483 100644 --- a/packages/triage-contracts/src/step-when.ts +++ b/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>, 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]; } diff --git a/packages/triage-manager/src/actions/review.ts b/packages/triage-manager/src/actions/review.ts index 6c31da13..60734088 100644 --- a/packages/triage-manager/src/actions/review.ts +++ b/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}`; diff --git a/packages/triage-workflows/src/logic/actions.ts b/packages/triage-workflows/src/logic/actions.ts index 408fff54..3d7d673f 100644 --- a/packages/triage-workflows/src/logic/actions.ts +++ b/packages/triage-workflows/src/logic/actions.ts @@ -204,7 +204,7 @@ const CI: Record = { 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,