From 448f30120e6a2609285fd15ea40ded8a645a1d4b Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 5 Nov 2026 15:06:16 -0800 Subject: [PATCH 1/2] 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 | 2 +- apps/hub/src/manager-state.postgres.test.ts | 13 ++++++++++++ packages/triage-contracts/src/check-pack.ts | 21 +++++++++++++------ .../triage-contracts/src/policy-export.ts | 2 +- .../src/policy-workflow.test.ts | 16 +++++++------- packages/triage-contracts/src/step-when.ts | 10 ++++----- packages/triage-manager/src/actions/review.ts | 2 +- .../triage-workflows/src/logic/actions.ts | 2 +- 8 files changed, 45 insertions(+), 23 deletions(-) diff --git a/apps/hub/src/github/policy-read.ts b/apps/hub/src/github/policy-read.ts index 447b8fe06..7639b9eee 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 8fa1fd788..d237381e6 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,15 @@ 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"]); + }); +}, 120_000); diff --git a/packages/triage-contracts/src/check-pack.ts b/packages/triage-contracts/src/check-pack.ts index 3a45ae353..e823c7ef8 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,12 +1155,19 @@ function distinctSteps(steps: Step[]): Step[] { }); } -function parseGroups(raw: unknown): Record | undefined { +/** A policy file, and mail stored before packs listed groups, hold a mapping; a mapping is read in its key order. */ +function parseGroups(raw: unknown): NamedGroup[] | undefined { if (raw === undefined) return undefined; + if (Array.isArray(raw)) { + const groups = NamedGroupsSchema(raw); + if (groups instanceof type.errors) throw new Error(`Groups ${groups.summary}`); + if (new Set(groups.map((group) => group.name)).size !== groups.length) throw new Error("Group names must be unique."); + return groups; + } const groups = FileGroupsSchema(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."); - return groups; + return Object.entries(groups).map(([name, group]) => ({ name, ...group })); } function parseLabels(raw: unknown): VerdictLabels | undefined { @@ -1385,7 +1394,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 faf4e3496..f10b74d3e 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-workflow.test.ts b/packages/triage-contracts/src/policy-workflow.test.ts index 7ae44e5c3..772582a7b 100644 --- a/packages/triage-contracts/src/policy-workflow.test.ts +++ b/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,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 7ff7ee3c0..a5a05483f 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 6c31da138..60734088b 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 408fff54f..3d7d673f9 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, From 3af7bf6a2ede30b797e5fb8ca95df3fc1d59ae1a Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Thu, 5 Nov 2026 15:51:46 -0800 Subject: [PATCH 2/2] fix(triage): refuse stored mail whose groups lost the file's order (CL-12197) --- apps/hub/src/manager-state.postgres.test.ts | 3 +++ packages/triage-contracts/src/check-pack.ts | 23 +++++++++++-------- packages/triage-contracts/src/policy-rules.ts | 4 ++-- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/apps/hub/src/manager-state.postgres.test.ts b/apps/hub/src/manager-state.postgres.test.ts index d237381e6..f3d978fd5 100644 --- a/apps/hub/src/manager-state.postgres.test.ts +++ b/apps/hub/src/manager-state.postgres.test.ts @@ -129,5 +129,8 @@ test("PostgreSQL: a pull request's mail keeps the policy's group order, so an au 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 e823c7ef8..b2eac6e67 100644 --- a/packages/triage-contracts/src/check-pack.ts +++ b/packages/triage-contracts/src/check-pack.ts @@ -1155,19 +1155,24 @@ function distinctSteps(steps: Step[]): Step[] { }); } -/** A policy file, and mail stored before packs listed groups, hold a mapping; a mapping is read in its key order. */ +/** 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; - if (Array.isArray(raw)) { - const groups = NamedGroupsSchema(raw); - if (groups instanceof type.errors) throw new Error(`Groups ${groups.summary}`); - if (new Set(groups.map((group) => group.name)).size !== groups.length) throw new Error("Group names must be unique."); - return groups; + 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 = FileGroupsSchema(raw); + 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."); - return Object.entries(groups).map(([name, group]) => ({ name, ...group })); + if (new Set(groups.map((group) => group.name)).size !== groups.length) throw new Error("Group names must be unique."); + return groups; } function parseLabels(raw: unknown): VerdictLabels | undefined { diff --git a/packages/triage-contracts/src/policy-rules.ts b/packages/triage-contracts/src/policy-rules.ts index 2e1cc043f..c087d4de4 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