From 7d13c12c69791c7803b45cda871217461a41ff9c Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Tue, 10 Nov 2026 04:11:45 +0000 Subject: [PATCH 1/7] stewards: move roles parsing into roles.ts (BB-280) --- plugins/stewards/config.test.ts | 18 +- plugins/stewards/config.ts | 516 +------------------------------ plugins/stewards/roles.ts | 523 ++++++++++++++++++++++++++++++++ plugins/stewards/server.test.ts | 3 +- plugins/stewards/server.ts | 36 +-- 5 files changed, 558 insertions(+), 538 deletions(-) create mode 100644 plugins/stewards/roles.ts diff --git a/plugins/stewards/config.test.ts b/plugins/stewards/config.test.ts index 13c21104..b5017c2b 100644 --- a/plugins/stewards/config.test.ts +++ b/plugins/stewards/config.test.ts @@ -1,25 +1,30 @@ import { describe, expect, it } from "vitest"; import { - bindingRoles, DEPENDENCY_BOTS, depsTargets, - filingTargetProblems, githubStateLabels, isTerminal, k8sTargets, labelPhaseName, legacyOpsPresent, - legacyRole, modelSpecSchema, opsTeam, parsePlaybook, parseProjectFile, - parseRolesFile, phaseInstructions, phaseOf, playbookLabels, playbookNameFor, playbookStates, + sentryTargets, + ticketWatchId, + ticketWatchProblems, +} from "./config.js"; +import { + bindingRoles, + filingTargetProblems, + legacyRole, + parseRolesFile, projectStewardProblem, pullWatchId, pullWatchProblems, @@ -33,12 +38,9 @@ import { roleSentryTargets, roleUsesOps, roleWarnings, - sentryTargets, - ticketWatchId, - ticketWatchProblems, usesKind, watchProblems, -} from "./config.js"; +} from "./roles.js"; import { DEPS_ALERTS, DEPS_PRS, GITHUB_ISSUES, GITHUB_PRS, GITHUB_REVIEWS, LINEAR_TICKETS, OPS_WATCH, projectFileTemplate } from "./templates.js"; describe("the project file", () => { diff --git a/plugins/stewards/config.ts b/plugins/stewards/config.ts index 0ab72ef9..d37f395e 100644 --- a/plugins/stewards/config.ts +++ b/plugins/stewards/config.ts @@ -14,7 +14,6 @@ import { DEPENDABOT_SEVERITIES, REPO_NAME } from "./source-contract.js"; export const STEWARDS_DIR = "stewards"; export const PROJECT_FILE = `${STEWARDS_DIR}/project.json`; /** The per-person roles: which watches and the project stewardship a GitHub login's server runs. */ -export const ROLES_FILE = `${STEWARDS_DIR}/roles.json`; export const playbookPath = (name: string) => `${STEWARDS_DIR}/playbooks/${name}.md`; export const DEFAULT_LABEL = "bb-managed"; @@ -119,7 +118,7 @@ export const GITHUB_TODO_LABEL = "status:todo"; export const DEPS_ALERTS_PLAYBOOK = "deps-alerts"; export const DEPS_PRS_PLAYBOOK = "deps-prs"; -const playbookName = z.string().regex(/^[a-z0-9][a-z0-9-]{0,62}$/u, "a playbook name: lowercase letters, digits and dashes"); +export const playbookName = z.string().regex(/^[a-z0-9][a-z0-9-]{0,62}$/u, "a playbook name: lowercase letters, digits and dashes"); function validTimezone(tz: string): boolean { try { @@ -175,7 +174,7 @@ export const dependabotRepoSchema = z.string().trim().regex(/^[A-Za-z0-9_.-]+\/[ const dependabotTargetSchema = z.object({ kind: z.literal("dependabot"), repo: dependabotRepoSchema }).strict(); /** A Sentry slug as Sentry spells it in URLs; upper case is folded, since Sentry's are always lower. */ -const sentrySlug = z.string().trim().toLowerCase().regex(SENTRY_SLUG, "a Sentry slug such as acme or web-api"); +export const sentrySlug = z.string().trim().toLowerCase().regex(SENTRY_SLUG, "a Sentry slug such as acme or web-api"); /** A Sentry project whose issues an ops steward watches, read with the plugin's Sentry token. */ const sentryProjectTargetSchema = z.object({ kind: z.literal("sentry-project"), org: sentrySlug, project: sentrySlug }).strict(); @@ -435,7 +434,7 @@ export type ProjectFile = z.infer; export type Parsed = { ok: true; value: T } | { ok: false; error: string }; -function zodMessage(error: z.ZodError): string { +export function zodMessage(error: z.ZodError): string { return error.issues.map((i) => `${i.path.length === 0 ? "" : `${i.path.join(".")}: `}${i.message}`).join("; "); } @@ -450,291 +449,6 @@ export function parseProjectFile(text: string): Parsed { return parsed.success ? { ok: true, value: parsed.data } : { ok: false, error: `${PROJECT_FILE}: ${zodMessage(parsed.error)}` }; } -// ---- roles: per-person watches, keyed by GitHub login ------------------------------ - -/** - * The per-person roles file (BB-105). It is read like the project file — a git - * repo at the canonical checkout's committed HEAD, a plain project directory - * from `.bb` on disk — and each server runs only the entry whose key is its own - * signed-in GitHub login. The watches with no natural - * owner (namespace, Sentry project, Dependabot repo) are assigned here; the - * ticket and review stewards are dispatched by the Linear assignee and the - * GitHub requested reviewer, and need no entry. - */ - -/** A GitHub login, folded to lower case by the parser. GitHub caps logins at 39 characters. */ -const githubLogin = z.string().trim().min(1).max(39).regex(/^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?$/u, "a GitHub login such as jane-doe"); - -const sentryProjectSchema = z.object({ org: sentrySlug, project: sentrySlug }).strict(); - -const roleSentrySchema = z - .object({ - projects: z.array(sentryProjectSchema).max(50).default([]), - /** On, the Sentry look resolves a mapped issue whose ticket is Done (the project file's `ops.sentry.resolveOnDone`). */ - resolveOnDone: z.boolean().default(false), - }) - .strict() - .default({ projects: [], resolveOnDone: false }); - -/** One pull-request watch's match fields: every given field must match, and `paths` means every changed file matches one of its globs. */ -export const pullMatchSchema = z - .object({ - title: z.string().trim().min(1).max(300).optional(), - branch: z.string().trim().min(1).max(300).optional(), - author: z.string().trim().min(1).max(100).optional(), - paths: z.array(z.string().trim().min(1).max(300)).min(1).max(200).optional(), - }) - .strict(); - -/** - * One pull-request watch a role declares (BB-164): PRs in `repo` whose shape - * matches, adopted under `playbook` on the holder's server alone. `match` is - * a glob per field, using the decide list's `*`/`**` dialect. - */ -export const pullWatchSchema = z.object({ repo: dependabotRepoSchema, match: pullMatchSchema.default({}), playbook: playbookName }).strict(); - -/** - * A pull watch as the matcher reads it. `author` is a string on a declared - * watch; the synthesized Dependabot watch names every bot login, so the type - * allows a list there. - */ -export type PullMatch = { title?: string; branch?: string; author?: string | string[]; paths?: string[] }; -export type PullWatch = { repo: string; match: PullMatch; playbook: string }; - -/** - * The stable id of a watch, recorded on the binding it adopts as `adoptedBy` - * and shown by `bb stewards list` (BB-164): `@`. It is what a - * binding reads to know a watch, not a label or review path, adopted it. - */ -export const pullWatchId = (watch: Pick): string => `${watch.playbook}@${watch.repo.toLowerCase()}`; - -/** A role's watches: namespaces, Sentry projects, Dependabot repos, pull watches, and where its tickets file. */ -export const roleOpsSchema = z - .object({ - k8sNamespaces: z.array(z.string().trim().regex(/^[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?$/u, "a namespace name such as bb-gate-production")).max(50).default([]), - /** The kubeconfig context kubectl uses on this person's machine; its current context when left out. */ - context: z.string().trim().min(1).max(200).regex(/^[^\s-][^\s]*$/u, "a kubeconfig context name").optional(), - /** Where this person's ops tickets are filed (the first of linear.teams when left out): a Linear team key or a GitHub owner/repo. */ - team: filingTargetSchema.optional(), - autoAdopt: z.array(z.enum(LINEAR_PRIORITIES)).max(4).default([]), - sentry: roleSentrySchema, - /** Repos whose Dependabot alerts this person watches; `"all"` is every repo in repos.json less github.exclude. */ - dependabot: z.union([z.literal("all"), z.array(dependabotRepoSchema).max(200)]).optional(), - /** Pull requests this role's server adopts by shape, under a playbook, with no label (BB-164). */ - pulls: z.array(pullWatchSchema).max(50).default([]), - }) - .strict(); -export type RoleOps = z.infer; - -/** One person's role: whether their server is the project steward, and the watches it runs. */ -export const roleSchema = z.object({ projectSteward: z.boolean().default(false), ops: roleOpsSchema.optional() }).strict(); -export type Role = z.infer; - -/** The built-in role name for the project steward. It has no `roles` definition. */ -export const PROJECT_STEWARD_ROLE = "projectSteward"; - -/** A named role in a version 2 file. */ -const roleNameSchema = z.string().min(1).max(63).regex(/^[A-Za-z][A-Za-z0-9_-]*$/u, "a role name such as ops"); - -/** - * The normalized roles file. Both versions read into this shape: `roles` is - * the effective role each login runs (the merge of every role it holds), so - * every consumer of `config.role` is unchanged. `holders` keeps the role - * names a login holds for display and validation, and `definitions` keeps - * the named definitions so `check` can report an unknown or unheld one. - */ -export type RolesFile = { - /** 1 for the login-keyed shape, 2 for named role definitions and holders. */ - version: 1 | 2; - /** login -> the effective role: the merge of every role it holds. */ - roles: Record; - /** login -> the role names it holds, in order. Version 1 synthesizes the login itself. */ - holders: Record; - /** role name -> its ops definition. Version 1 synthesizes one per login. `projectSteward` is never a key. */ - definitions: Record; -}; - -const rolesFileV1RawSchema = z.object({ version: z.literal(1), roles: z.record(z.string(), roleSchema) }).strict(); - -/** - * Folds every login to lower case and refuses two keys that name the same - * login ignoring case. Duplicate JSON keys are collapsed by `JSON.parse` - * before this runs; the check catches `Jane` and `jane` in one file. Each - * login becomes one synthesized role named after the login, held by it alone. - */ -const rolesFileV1Schema = rolesFileV1RawSchema.transform((file, ctx): RolesFile => { - const roles: Record = {}; - const holders: Record = {}; - const definitions: Record = {}; - for (const [key, role] of Object.entries(file.roles)) { - const parsed = githubLogin.safeParse(key); - if (!parsed.success) { - ctx.addIssue({ code: "custom", path: ["roles", key], message: `${JSON.stringify(key)} is not a GitHub login` }); - return z.NEVER; - } - const login = parsed.data.toLowerCase(); - if (roles[login] !== undefined) { - ctx.addIssue({ code: "custom", path: ["roles", key], message: `${key} names the same login as another entry, ignoring case` }); - return z.NEVER; - } - holders[login] = [login, ...(role.projectSteward ? [PROJECT_STEWARD_ROLE] : [])]; - if (role.ops !== undefined) definitions[login] = role.ops; - roles[login] = role; - } - return { version: 1, roles, holders, definitions }; -}); - -const rolesFileV2RawSchema = z - .object({ - version: z.literal(2), - /** The named role definitions: today's `roleOpsSchema` body, once per name. */ - roles: z.record(roleNameSchema, roleOpsSchema), - /** login -> the role names it holds. `projectSteward` is a built-in name. */ - holders: z.record(z.string(), z.array(roleNameSchema).min(1).max(50)), - }) - .strict() - .superRefine((file, ctx) => { - if (Object.prototype.hasOwnProperty.call(file.roles, PROJECT_STEWARD_ROLE)) { - ctx.addIssue({ code: "custom", path: ["roles", PROJECT_STEWARD_ROLE], message: `${PROJECT_STEWARD_ROLE} is a built-in role with no definition; remove it from roles` }); - } - }); - -/** - * The merge of a holder's role definitions, in the order the holder lists - * them. List fields are unioned; `context` and `team` take the first defined - * value (a disagreement is a `check` problem, never a runtime change); - * `dependabot: "all"` wins over an explicit list. - */ -function mergeRoleOps(defs: readonly RoleOps[]): RoleOps { - const k8sNamespaces: string[] = []; - const autoAdopt: RoleOps["autoAdopt"] = []; - const projects: RoleOps["sentry"]["projects"] = []; - const dependabot: string[] = []; - const pulls: RoleOps["pulls"] = []; - let dependabotAll = false; - let context: string | undefined; - let team: string | undefined; - let resolveOnDone = false; - for (const def of defs) { - for (const name of def.k8sNamespaces) if (!k8sNamespaces.includes(name)) k8sNamespaces.push(name); - for (const priority of def.autoAdopt) if (!autoAdopt.includes(priority)) autoAdopt.push(priority); - for (const project of def.sentry.projects) if (!projects.some((p) => p.org === project.org && p.project === project.project)) projects.push(project); - if (def.context !== undefined && context === undefined) context = def.context; - if (def.team !== undefined && team === undefined) team = def.team; - if (def.sentry.resolveOnDone) resolveOnDone = true; - if (def.dependabot === "all") dependabotAll = true; - else if (Array.isArray(def.dependabot)) for (const repo of def.dependabot) if (!dependabot.includes(repo)) dependabot.push(repo); - for (const watch of def.pulls) if (!pulls.some((w) => samePullWatch(w, watch))) pulls.push(watch); - } - const ops: RoleOps = { k8sNamespaces, autoAdopt, sentry: { projects, resolveOnDone }, pulls }; - if (context !== undefined) ops.context = context; - if (team !== undefined) ops.team = team; - if (dependabotAll) ops.dependabot = "all"; - else if (dependabot.length > 0) ops.dependabot = dependabot; - return ops; -} - -/** Whether two watches are the same declaration, so a holder of several roles does not double them. */ -function samePullWatch(a: PullWatch, b: PullWatch): boolean { - return a.repo.toLowerCase() === b.repo.toLowerCase() && a.playbook === b.playbook && JSON.stringify(a.match) === JSON.stringify(b.match); -} - -/** The effective role a holder runs: the merge of the roles it holds, and whether one is `projectSteward`. */ -function effectiveRole(names: readonly string[], definitions: Record): Role { - const defs = names.filter((name) => name !== PROJECT_STEWARD_ROLE).map((name) => definitions[name]).filter((def): def is RoleOps => def !== undefined); - return { projectSteward: names.includes(PROJECT_STEWARD_ROLE), ...(defs.length === 0 ? {} : { ops: mergeRoleOps(defs) }) }; -} - -/** Folds holder logins to lower case and refuses two that name the same login ignoring case. */ -const rolesFileV2Schema = rolesFileV2RawSchema.transform((file, ctx): RolesFile => { - const holders: Record = {}; - for (const [key, names] of Object.entries(file.holders)) { - const parsed = githubLogin.safeParse(key); - if (!parsed.success) { - ctx.addIssue({ code: "custom", path: ["holders", key], message: `${JSON.stringify(key)} is not a GitHub login` }); - return z.NEVER; - } - const login = parsed.data.toLowerCase(); - if (holders[login] !== undefined) { - ctx.addIssue({ code: "custom", path: ["holders", key], message: `${key} names the same login as another entry, ignoring case` }); - return z.NEVER; - } - holders[login] = [...new Set(names)]; - } - const roles: Record = {}; - for (const [login, names] of Object.entries(holders)) roles[login] = effectiveRole(names, file.roles); - return { version: 2, roles, holders, definitions: file.roles }; -}); - -export function parseRolesFile(text: string): Parsed { - let raw: unknown; - try { - raw = JSON.parse(text); - } catch (error) { - return { ok: false, error: `${ROLES_FILE} is not JSON: ${error instanceof Error ? error.message : String(error)}` }; - } - const version = typeof raw === "object" && raw !== null ? (raw as { version?: unknown }).version : undefined; - const parsed = (version === 2 ? rolesFileV2Schema : rolesFileV1Schema).safeParse(raw); - return parsed.success ? { ok: true, value: parsed.data } : { ok: false, error: `${ROLES_FILE}: ${zodMessage(parsed.error)}` }; -} - -// ---- the role one server runs -------------------------------------------------------- - -/** The k8s namespaces a role watches, which are read with kubectl. */ -export const roleK8sTargets = (role: Role | null): K8sTarget[] => (role?.ops?.k8sNamespaces ?? []).map((name) => ({ kind: "k8s-namespace", name })); - -/** The Sentry projects a role watches, which are read with the plugin's Sentry token and rung by bb-gate-hooks. */ -export const roleSentryTargets = (role: Role | null): SentryTarget[] => (role?.ops?.sentry?.projects ?? []).map((p) => ({ kind: "sentry-project", org: p.org, project: p.project })); - -/** The namespace and Sentry targets a role watches; the Dependabot watch is separate because it defaults to every repo. */ -export const roleOpsTargets = (role: Role | null): OpsTarget[] => [...roleK8sTargets(role), ...roleSentryTargets(role)]; - -/** - * The Dependabot alert targets a role watches: `"all"` is every repo in the - * project's derived set (`repos.json` less `github.exclude`), an explicit list - * is exactly those, and no `dependabot` key is none. - */ -export function roleDepsTargets(role: Role | null, repos: readonly string[]): DependabotTarget[] { - const dep = role?.ops?.dependabot; - return (dep === "all" ? [...repos] : dep ?? []).map((repo) => ({ kind: "dependabot" as const, repo })); -} - -/** The pull watches a role declares, in the order the file lists them. */ -export const rolePulls = (role: Role | null): PullWatch[] => role?.ops?.pulls ?? []; - -/** - * Every pull watch a role's server runs: the Dependabot watches synthesized - * from `dependabot` first, then the role's declared watches, first match wins - * (BB-164). A synthesized watch names every bot login as its `match.author` - * and runs the deps-prs playbook. - */ -export function rolePullWatches(role: Role | null, repos: readonly string[], depsPrsName: string): PullWatch[] { - const synthesized: PullWatch[] = roleDepsTargets(role, repos).map((t) => ({ repo: t.repo, match: { author: [...DEPENDENCY_BOTS] }, playbook: depsPrsName })); - return [...synthesized, ...rolePulls(role)]; -} - -/** The Linear team a role's ops steward files in (the first of linear.teams when the role names none), with its source. */ -export const roleOpsTeam = (role: Role | null, project: ProjectFile): FilingTarget | null => { - const target = role?.ops?.team ?? project.linear.teams[0] ?? null; - return target === null ? null : filingTargetOf(target); -}; - -/** - * Whether a role watches a namespace or Sentry project, so the ops-watch - * playbook is needed. The Dependabot watch runs the separate deps-alerts - * playbook, so it does not count here. - */ -export const roleUsesOps = (role: Role | null): boolean => roleK8sTargets(role).length > 0 || roleSentryTargets(role).length > 0; - -/** - * Whether a project uses a kind of binding at all: reviews can be turned off, - * and ops needs a target. `role` is the server's resolved role: omitted, the - * legacy `project.ops` is read (a project without a roles.json); null, the - * server has no role and runs no ops. - */ -export const usesKind = (project: ProjectFile, kind: BindingKind, role?: Role | null): boolean => - kind === "review" ? project.reviews.enabled : kind === "ops" ? (role === undefined ? project.ops.targets.length > 0 : roleUsesOps(role)) : true; - /** * The playbook a project file names for a kind. A ticket with no explicit * playbook follows the source the project tracks: `github-issues` when it has @@ -771,7 +485,7 @@ export const opsTeam = (project: ProjectFile): FilingTarget | null => { return target === null ? null : filingTargetOf(target); }; -// ---- migration and the project steward's role problems --------------------------------- +// ---- migration --------------------------------------------------------------------- /** * Whether a project file still carries watches that now belong in roles.json. @@ -780,126 +494,6 @@ export const opsTeam = (project: ProjectFile): FilingTarget | null => { export const legacyOpsPresent = (project: ProjectFile): boolean => project.ops.targets.length > 0 || project.ops.team !== undefined || project.ops.context !== undefined || project.ops.autoAdopt.length > 0 || project.ops.sentry.resolveOnDone; -/** - * A project file's legacy `ops` as a role, so a project with no roles.json runs - * exactly as before: every server is the project steward and runs every watch. - * The Dependabot default (every repo) becomes `"all"`. - */ -export function legacyRole(project: ProjectFile): Role { - const explicit = project.ops.targets.filter((t): t is DependabotTarget => t.kind === "dependabot").map((t) => t.repo); - return { - projectSteward: true, - ops: { - k8sNamespaces: k8sTargets(project).map((t) => t.name), - ...(project.ops.team === undefined ? {} : { team: project.ops.team }), - ...(project.ops.context === undefined ? {} : { context: project.ops.context }), - autoAdopt: [...project.ops.autoAdopt], - sentry: { projects: sentryTargets(project).map((t) => ({ org: t.org, project: t.project })), resolveOnDone: project.ops.sentry.resolveOnDone }, - dependabot: explicit.length > 0 ? explicit : "all", - pulls: [], - }, - }; -} - -/** The words the project steward's problem lines use for one watch. */ -function watchLabel(key: string): string { - if (key.startsWith("k8s-namespace/")) return `the ${key.slice("k8s-namespace/".length)} watch`; - if (key.startsWith("sentry-project/")) return `the Sentry watch for ${key.slice("sentry-project/".length)}`; - return `the Dependabot watch for ${key.slice("dependabot/".length)}`; -} - -/** Every watch a role runs, keyed as `opsTargetKey` keys the same target. */ -function roleWatchKeys(role: Role, repos: readonly string[]): string[] { - return [...roleOpsTargets(role), ...roleDepsTargets(role, repos)].map(opsTargetKey); -} - -/** - * The watches the project expects someone to run: the legacy `ops` targets, - * and the Dependabot default (every repo) while the dependency watch is on. A - * watch in no role is unowned; one in two roles is doubly owned. - */ -export function expectedWatches(project: ProjectFile, repos: readonly string[], depsEnabled: boolean): { key: string; label: string }[] { - const out: { key: string; label: string }[] = []; - const seen = new Set(); - const add = (key: string) => { - if (seen.has(key)) return; - seen.add(key); - out.push({ key, label: watchLabel(key) }); - }; - for (const t of k8sTargets(project)) add(opsTargetKey(t)); - for (const t of sentryTargets(project)) add(opsTargetKey(t)); - const explicit = project.ops.targets.filter((t): t is DependabotTarget => t.kind === "dependabot").map((t) => t.repo); - if (explicit.length > 0) for (const repo of explicit) add(opsTargetKey({ kind: "dependabot", repo })); - else if (depsEnabled) for (const repo of repos) add(opsTargetKey({ kind: "dependabot", repo })); - return out; -} - -/** - * The project steward's problem lines about who owns what: a watch nobody - * runs, and a watch two people run. Credentials are checked on each server, - * never here: a role whose machine lacks kubectl or a Sentry token is that - * server's own problem line. - */ -export function watchProblems(project: ProjectFile, roles: RolesFile, repos: readonly string[], depsEnabled: boolean): string[] { - const problems: string[] = []; - const owners = new Map(); - for (const [login, role] of Object.entries(roles.roles)) { - for (const key of roleWatchKeys(role, repos)) owners.set(key, [...(owners.get(key) ?? []), login]); - } - const expected = expectedWatches(project, repos, depsEnabled); - for (const watch of expected) { - const list = owners.get(watch.key) ?? []; - if (list.length === 0) problems.push(`nobody runs ${watch.label}`); - else if (list.length > 1) problems.push(`two people run ${watch.label}: ${list.join(", ")}`); - } - // A watch two roles name that the project does not otherwise expect: a namespace, say. - for (const [key, list] of owners) { - if (list.length > 1 && !expected.some((w) => w.key === key)) problems.push(`two people run ${watchLabel(key)}: ${list.join(", ")}`); - } - return problems; -} - -/** - * The `check` problems of every declared pull watch (BB-164): its repo must be - * one the project leases (in `repos.json`, not in `github.exclude`), its - * playbook must exist and be kind `pr`, its globs must parse, and no two roles - * with holders may declare a watch on the same repo — an error, not a warning, - * since it is the only place two servers adopting the same pull request can be - * caught. `playbooks` holds every watch playbook that parsed; a missing one was - * already reported where it was read. - */ -export function pullWatchProblems(roles: RolesFile, repos: readonly string[], excluded: readonly string[], playbooks: ReadonlyMap): string[] { - const problems: string[] = []; - const repoOwners = new Map(); - for (const [login, role] of Object.entries(roles.roles)) { - for (const watch of rolePulls(role)) { - const where = `roles.json: the pull watch on ${watch.repo} for ${JSON.stringify(login)}`; - if (excluded.some((r) => r.toLowerCase() === watch.repo.toLowerCase())) problems.push(`${where}: the repo is in github.exclude, so it is not stewarded`); - else if (!repos.some((r) => r.toLowerCase() === watch.repo.toLowerCase())) problems.push(`${where}: the repo is not in repos.json, so the project has no lease for it and gets no doorbells`); - const playbook = playbooks.get(watch.playbook); - if (playbook !== undefined && playbook.kind !== "pr") problems.push(`${where}: the playbook ${JSON.stringify(watch.playbook)} is a ${playbook.kind} playbook, not a pr playbook`); - for (const [field, value] of [ - ["match.title", watch.match.title], - ["match.branch", watch.match.branch], - ["match.author", Array.isArray(watch.match.author) ? undefined : watch.match.author], - ] as const) { - const bad = value === undefined ? null : globError(value); - if (bad !== null) problems.push(`${where}: ${field} is not a glob: ${bad}`); - } - for (const pattern of watch.match.paths ?? []) { - const bad = globError(pattern); - if (bad !== null) problems.push(`${where}: match.paths ${bad}`); - } - const owners = repoOwners.get(watch.repo.toLowerCase()) ?? []; - if (!owners.includes(login)) repoOwners.set(watch.repo.toLowerCase(), [...owners, login]); - } - } - for (const [repo, owners] of repoOwners) { - if (owners.length > 1) problems.push(`two roles with holders declare pull watches on ${repo}: ${owners.join(", ")} — only one server can adopt a pull request, so remove one`); - } - return problems; -} - /** * The `check` problems of every declared ticket watch (BB-246): its playbook * must exist and be kind `ticket`, and its `title` glob must parse. `playbooks` @@ -924,108 +518,6 @@ export function ticketWatchProblems(file: ProjectFile, playbooks: ReadonlyMap { - const out = new Map(); - if (roles === null || login === null) return out; - for (const name of roles.holders[login] ?? []) { - if (name === PROJECT_STEWARD_ROLE) continue; - const definition = roles.definitions[name]; - if (definition === undefined) continue; - const role: Role = { projectSteward: false, ops: definition }; - for (const target of [...roleOpsTargets(role), ...roleDepsTargets(role, repos)]) { - const key = opsTargetKey(target); - const list = out.get(key) ?? []; - if (!list.includes(name)) out.set(key, [...list, name]); - } - // A pull binding is keyed by the watch that adopted it, not a target key. - for (const watch of rolePulls(role)) { - const key = pullWatchId(watch); - const list = out.get(key) ?? []; - if (!list.includes(name)) out.set(key, [...list, name]); - } - } - return out; -} - -/** - * The project steward count problem, or null when one or more holders hold it. - * Two or more holders is normal (BB-230): each server runs only its own login's - * roles, so every developer whose server runs stewards can hold the role. - */ -export function projectStewardProblem(roles: RolesFile): string | null { - const stewards = Object.entries(roles.roles).filter(([, role]) => role.projectSteward).map(([login]) => login); - if (stewards.length === 0) { - return roles.version === 2 - ? `no one holds ${PROJECT_STEWARD_ROLE}: give one holder the ${PROJECT_STEWARD_ROLE} role in roles.json` - : "no one is the project steward: set projectSteward: true on one login in roles.json"; - } - return null; -} - -/** - * The version 2 validation errors a `check` reports: a holder names a role - * that is neither defined nor the built-in `projectSteward`, or a holder's - * roles disagree on `context` or `team`. The runtime never acts on these: the - * effective role is merged deterministically and the same file keeps running. - */ -export function roleProblems(roles: RolesFile): string[] { - if (roles.version !== 2) return []; - const problems: string[] = []; - for (const [login, names] of Object.entries(roles.holders)) { - for (const name of names) { - if (name !== PROJECT_STEWARD_ROLE && roles.definitions[name] === undefined) { - problems.push(`${login} holds an unknown role ${JSON.stringify(name)}: define it under roles, or use ${PROJECT_STEWARD_ROLE}`); - } - } - const defs = names.map((name) => roles.definitions[name]).filter((def): def is RoleOps => def !== undefined); - const contexts = [...new Set(defs.map((def) => def.context).filter((value): value is string => value !== undefined))]; - if (contexts.length > 1) problems.push(`${login} holds roles with conflicting context (${contexts.join(", ")}): a holder's roles must agree on one context`); - const teams = [...new Set(defs.map((def) => def.team).filter((value): value is string => value !== undefined))]; - if (teams.length > 1) problems.push(`${login} holds roles with conflicting team (${teams.join(", ")}): a holder's roles must agree on one team`); - } - return problems; -} - -/** The version 2 warnings a `check` reports: a defined role no holder names. */ -export function roleWarnings(roles: RolesFile): string[] { - if (roles.version !== 2) return []; - const held = new Set(Object.values(roles.holders).flat()); - return Object.keys(roles.definitions) - .filter((name) => !held.has(name)) - .map((name) => `role ${JSON.stringify(name)} has no holder`); -} - -/** - * The problems with a project's filing targets (BB-235): a GitHub `owner/repo` - * that is not in the project's repo set, and a role's Linear team key that is - * not one of `linear.teams`. A project file's team key is checked by the - * schema itself; this adds the repo case the schema cannot see (the repo set - * comes from `repos.json`) and the role cases the schema never checks. - */ -export function filingTargetProblems(project: ProjectFile, roles: RolesFile | null, repos: readonly string[]): string[] { - const out: string[] = []; - const repoProblem = (where: string, target: string) => { - if (!repos.some((r) => r.toLowerCase() === target.toLowerCase())) out.push(`${where}: ${target} is not in repos.json, so a ticket filed there would never be stewarded`); - }; - const teamProblem = (where: string, target: string) => { - if (!project.linear.teams.includes(target)) out.push(`${where}: ${target} is not one of linear.teams, so a ticket filed there would never be stewarded`); - }; - const projectTarget = project.ops.team; - if (projectTarget !== undefined && REPO_NAME.test(projectTarget)) repoProblem(`${PROJECT_FILE} ops.team`, projectTarget); - for (const [login, role] of Object.entries(roles?.roles ?? {})) { - const target = role.ops?.team; - if (target === undefined) continue; - if (REPO_NAME.test(target)) repoProblem(`${ROLES_FILE} ${login}'s ops.team`, target); - else teamProblem(`${ROLES_FILE} ${login}'s ops.team`, target); - } - return out; -} - // ---- playbooks --------------------------------------------------------------------- export const STAGES = ["spec", "build", "review"] as const; diff --git a/plugins/stewards/roles.ts b/plugins/stewards/roles.ts new file mode 100644 index 00000000..ebbc33e2 --- /dev/null +++ b/plugins/stewards/roles.ts @@ -0,0 +1,523 @@ +// The roles file: who stewards are and what each server runs (BB-105). The +// `roles.json` schemas, the version 1 and 2 normalisation, the role a server +// resolves, and the `check` problems about who owns what. This is the roles +// seam, kept in its own module so a change to it is always the owner's. +// +// It reads nothing on its own: the project file's schema primitives and its +// own `check` problems come from config.ts, and the store is passed to the +// functions that need it. +import { z } from "zod"; +import { globError } from "./decisions.js"; +import { opsTargetKey, type DependabotTarget, type K8sTarget, type OpsTarget, type SentryTarget } from "./ops.js"; +import { REPO_NAME } from "./source-contract.js"; +import { DEPENDENCY_BOTS, dependabotRepoSchema, filingTargetSchema, filingTargetOf, k8sTargets, LINEAR_PRIORITIES, playbookName, PROJECT_FILE, sentrySlug, sentryTargets, STEWARDS_DIR, zodMessage, type BindingKind, type FilingTarget, type Parsed, type Playbook, type ProjectFile } from "./config.js"; + +export const ROLES_FILE = `${STEWARDS_DIR}/roles.json`; + +// ---- roles: per-person watches, keyed by GitHub login ------------------------------ + +/** + * The per-person roles file (BB-105). It is read like the project file — a git + * repo at the canonical checkout's committed HEAD, a plain project directory + * from `.bb` on disk — and each server runs only the entry whose key is its own + * signed-in GitHub login. The watches with no natural + * owner (namespace, Sentry project, Dependabot repo) are assigned here; the + * ticket and review stewards are dispatched by the Linear assignee and the + * GitHub requested reviewer, and need no entry. + */ + +/** A GitHub login, folded to lower case by the parser. GitHub caps logins at 39 characters. */ +const githubLogin = z.string().trim().min(1).max(39).regex(/^[A-Za-z0-9](?:[A-Za-z0-9-]*[A-Za-z0-9])?$/u, "a GitHub login such as jane-doe"); + +const sentryProjectSchema = z.object({ org: sentrySlug, project: sentrySlug }).strict(); + +const roleSentrySchema = z + .object({ + projects: z.array(sentryProjectSchema).max(50).default([]), + /** On, the Sentry look resolves a mapped issue whose ticket is Done (the project file's `ops.sentry.resolveOnDone`). */ + resolveOnDone: z.boolean().default(false), + }) + .strict() + .default({ projects: [], resolveOnDone: false }); + +/** One pull-request watch's match fields: every given field must match, and `paths` means every changed file matches one of its globs. */ +export const pullMatchSchema = z + .object({ + title: z.string().trim().min(1).max(300).optional(), + branch: z.string().trim().min(1).max(300).optional(), + author: z.string().trim().min(1).max(100).optional(), + paths: z.array(z.string().trim().min(1).max(300)).min(1).max(200).optional(), + }) + .strict(); + +/** + * One pull-request watch a role declares (BB-164): PRs in `repo` whose shape + * matches, adopted under `playbook` on the holder's server alone. `match` is + * a glob per field, using the decide list's `*`/`**` dialect. + */ +export const pullWatchSchema = z.object({ repo: dependabotRepoSchema, match: pullMatchSchema.default({}), playbook: playbookName }).strict(); + +/** + * A pull watch as the matcher reads it. `author` is a string on a declared + * watch; the synthesized Dependabot watch names every bot login, so the type + * allows a list there. + */ +export type PullMatch = { title?: string; branch?: string; author?: string | string[]; paths?: string[] }; +export type PullWatch = { repo: string; match: PullMatch; playbook: string }; + +/** + * The stable id of a watch, recorded on the binding it adopts as `adoptedBy` + * and shown by `bb stewards list` (BB-164): `@`. It is what a + * binding reads to know a watch, not a label or review path, adopted it. + */ +export const pullWatchId = (watch: Pick): string => `${watch.playbook}@${watch.repo.toLowerCase()}`; + +/** A role's watches: namespaces, Sentry projects, Dependabot repos, pull watches, and where its tickets file. */ +export const roleOpsSchema = z + .object({ + k8sNamespaces: z.array(z.string().trim().regex(/^[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?$/u, "a namespace name such as bb-gate-production")).max(50).default([]), + /** The kubeconfig context kubectl uses on this person's machine; its current context when left out. */ + context: z.string().trim().min(1).max(200).regex(/^[^\s-][^\s]*$/u, "a kubeconfig context name").optional(), + /** Where this person's ops tickets are filed (the first of linear.teams when left out): a Linear team key or a GitHub owner/repo. */ + team: filingTargetSchema.optional(), + autoAdopt: z.array(z.enum(LINEAR_PRIORITIES)).max(4).default([]), + sentry: roleSentrySchema, + /** Repos whose Dependabot alerts this person watches; `"all"` is every repo in repos.json less github.exclude. */ + dependabot: z.union([z.literal("all"), z.array(dependabotRepoSchema).max(200)]).optional(), + /** Pull requests this role's server adopts by shape, under a playbook, with no label (BB-164). */ + pulls: z.array(pullWatchSchema).max(50).default([]), + }) + .strict(); +export type RoleOps = z.infer; + +/** One person's role: whether their server is the project steward, and the watches it runs. */ +export const roleSchema = z.object({ projectSteward: z.boolean().default(false), ops: roleOpsSchema.optional() }).strict(); +export type Role = z.infer; + +/** The built-in role name for the project steward. It has no `roles` definition. */ +export const PROJECT_STEWARD_ROLE = "projectSteward"; + +const roleNameSchema = z.string().min(1).max(63).regex(/^[A-Za-z][A-Za-z0-9_-]*$/u, "a role name such as ops"); + +/** + * The normalized roles file. Both versions read into this shape: `roles` is + * the effective role each login runs (the merge of every role it holds), so + * every consumer of `config.role` is unchanged. `holders` keeps the role + * names a login holds for display and validation, and `definitions` keeps + * the named definitions so `check` can report an unknown or unheld one. + */ +export type RolesFile = { + /** 1 for the login-keyed shape, 2 for named role definitions and holders. */ + version: 1 | 2; + /** login -> the effective role: the merge of every role it holds. */ + roles: Record; + /** login -> the role names it holds, in order. Version 1 synthesizes the login itself. */ + holders: Record; + /** role name -> its ops definition. Version 1 synthesizes one per login. `projectSteward` is never a key. */ + definitions: Record; +}; + +const rolesFileV1RawSchema = z.object({ version: z.literal(1), roles: z.record(z.string(), roleSchema) }).strict(); + +/** + * Folds every login to lower case and refuses two keys that name the same + * login ignoring case. Duplicate JSON keys are collapsed by `JSON.parse` + * before this runs; the check catches `Jane` and `jane` in one file. Each + * login becomes one synthesized role named after the login, held by it alone. + */ +const rolesFileV1Schema = rolesFileV1RawSchema.transform((file, ctx): RolesFile => { + const roles: Record = {}; + const holders: Record = {}; + const definitions: Record = {}; + for (const [key, role] of Object.entries(file.roles)) { + const parsed = githubLogin.safeParse(key); + if (!parsed.success) { + ctx.addIssue({ code: "custom", path: ["roles", key], message: `${JSON.stringify(key)} is not a GitHub login` }); + return z.NEVER; + } + const login = parsed.data.toLowerCase(); + if (roles[login] !== undefined) { + ctx.addIssue({ code: "custom", path: ["roles", key], message: `${key} names the same login as another entry, ignoring case` }); + return z.NEVER; + } + holders[login] = [login, ...(role.projectSteward ? [PROJECT_STEWARD_ROLE] : [])]; + if (role.ops !== undefined) definitions[login] = role.ops; + roles[login] = role; + } + return { version: 1, roles, holders, definitions }; +}); + +const rolesFileV2RawSchema = z + .object({ + version: z.literal(2), + /** The named role definitions: today's `roleOpsSchema` body, once per name. */ + roles: z.record(roleNameSchema, roleOpsSchema), + /** login -> the role names it holds. `projectSteward` is a built-in name. */ + holders: z.record(z.string(), z.array(roleNameSchema).min(1).max(50)), + }) + .strict() + .superRefine((file, ctx) => { + if (Object.prototype.hasOwnProperty.call(file.roles, PROJECT_STEWARD_ROLE)) { + ctx.addIssue({ code: "custom", path: ["roles", PROJECT_STEWARD_ROLE], message: `${PROJECT_STEWARD_ROLE} is a built-in role with no definition; remove it from roles` }); + } + }); + +/** + * The merge of a holder's role definitions, in the order the holder lists + * them. List fields are unioned; `context` and `team` take the first defined + * value (a disagreement is a `check` problem, never a runtime change); + * `dependabot: "all"` wins over an explicit list. + */ +function mergeRoleOps(defs: readonly RoleOps[]): RoleOps { + const k8sNamespaces: string[] = []; + const autoAdopt: RoleOps["autoAdopt"] = []; + const projects: RoleOps["sentry"]["projects"] = []; + const dependabot: string[] = []; + const pulls: RoleOps["pulls"] = []; + let dependabotAll = false; + let context: string | undefined; + let team: string | undefined; + let resolveOnDone = false; + for (const def of defs) { + for (const name of def.k8sNamespaces) if (!k8sNamespaces.includes(name)) k8sNamespaces.push(name); + for (const priority of def.autoAdopt) if (!autoAdopt.includes(priority)) autoAdopt.push(priority); + for (const project of def.sentry.projects) if (!projects.some((p) => p.org === project.org && p.project === project.project)) projects.push(project); + if (def.context !== undefined && context === undefined) context = def.context; + if (def.team !== undefined && team === undefined) team = def.team; + if (def.sentry.resolveOnDone) resolveOnDone = true; + if (def.dependabot === "all") dependabotAll = true; + else if (Array.isArray(def.dependabot)) for (const repo of def.dependabot) if (!dependabot.includes(repo)) dependabot.push(repo); + for (const watch of def.pulls) if (!pulls.some((w) => samePullWatch(w, watch))) pulls.push(watch); + } + const ops: RoleOps = { k8sNamespaces, autoAdopt, sentry: { projects, resolveOnDone }, pulls }; + if (context !== undefined) ops.context = context; + if (team !== undefined) ops.team = team; + if (dependabotAll) ops.dependabot = "all"; + else if (dependabot.length > 0) ops.dependabot = dependabot; + return ops; +} + +/** Whether two watches are the same declaration, so a holder of several roles does not double them. */ +function samePullWatch(a: PullWatch, b: PullWatch): boolean { + return a.repo.toLowerCase() === b.repo.toLowerCase() && a.playbook === b.playbook && JSON.stringify(a.match) === JSON.stringify(b.match); +} + +/** The effective role a holder runs: the merge of the roles it holds, and whether one is `projectSteward`. */ +function effectiveRole(names: readonly string[], definitions: Record): Role { + const defs = names.filter((name) => name !== PROJECT_STEWARD_ROLE).map((name) => definitions[name]).filter((def): def is RoleOps => def !== undefined); + return { projectSteward: names.includes(PROJECT_STEWARD_ROLE), ...(defs.length === 0 ? {} : { ops: mergeRoleOps(defs) }) }; +} + +/** Folds holder logins to lower case and refuses two that name the same login ignoring case. */ +const rolesFileV2Schema = rolesFileV2RawSchema.transform((file, ctx): RolesFile => { + const holders: Record = {}; + for (const [key, names] of Object.entries(file.holders)) { + const parsed = githubLogin.safeParse(key); + if (!parsed.success) { + ctx.addIssue({ code: "custom", path: ["holders", key], message: `${JSON.stringify(key)} is not a GitHub login` }); + return z.NEVER; + } + const login = parsed.data.toLowerCase(); + if (holders[login] !== undefined) { + ctx.addIssue({ code: "custom", path: ["holders", key], message: `${key} names the same login as another entry, ignoring case` }); + return z.NEVER; + } + holders[login] = [...new Set(names)]; + } + const roles: Record = {}; + for (const [login, names] of Object.entries(holders)) roles[login] = effectiveRole(names, file.roles); + return { version: 2, roles, holders, definitions: file.roles }; +}); + +export function parseRolesFile(text: string): Parsed { + let raw: unknown; + try { + raw = JSON.parse(text); + } catch (error) { + return { ok: false, error: `${ROLES_FILE} is not JSON: ${error instanceof Error ? error.message : String(error)}` }; + } + const version = typeof raw === "object" && raw !== null ? (raw as { version?: unknown }).version : undefined; + const parsed = (version === 2 ? rolesFileV2Schema : rolesFileV1Schema).safeParse(raw); + return parsed.success ? { ok: true, value: parsed.data } : { ok: false, error: `${ROLES_FILE}: ${zodMessage(parsed.error)}` }; +} + +// ---- the role one server runs -------------------------------------------------------- + +/** The k8s namespaces a role watches, which are read with kubectl. */ +export const roleK8sTargets = (role: Role | null): K8sTarget[] => (role?.ops?.k8sNamespaces ?? []).map((name) => ({ kind: "k8s-namespace", name })); + +/** The Sentry projects a role watches, which are read with the plugin's Sentry token and rung by bb-gate-hooks. */ +export const roleSentryTargets = (role: Role | null): SentryTarget[] => (role?.ops?.sentry?.projects ?? []).map((p) => ({ kind: "sentry-project", org: p.org, project: p.project })); + +/** The namespace and Sentry targets a role watches; the Dependabot watch is separate because it defaults to every repo. */ +export const roleOpsTargets = (role: Role | null): OpsTarget[] => [...roleK8sTargets(role), ...roleSentryTargets(role)]; + +/** + * The Dependabot alert targets a role watches: `"all"` is every repo in the + * project's derived set (`repos.json` less `github.exclude`), an explicit list + * is exactly those, and no `dependabot` key is none. + */ +export function roleDepsTargets(role: Role | null, repos: readonly string[]): DependabotTarget[] { + const dep = role?.ops?.dependabot; + return (dep === "all" ? [...repos] : dep ?? []).map((repo) => ({ kind: "dependabot" as const, repo })); +} + +/** The pull watches a role declares, in the order the file lists them. */ +export const rolePulls = (role: Role | null): PullWatch[] => role?.ops?.pulls ?? []; + +/** + * Every pull watch a role's server runs: the Dependabot watches synthesized + * from `dependabot` first, then the role's declared watches, first match wins + * (BB-164). A synthesized watch names every bot login as its `match.author` + * and runs the deps-prs playbook. + */ +export function rolePullWatches(role: Role | null, repos: readonly string[], depsPrsName: string): PullWatch[] { + const synthesized: PullWatch[] = roleDepsTargets(role, repos).map((t) => ({ repo: t.repo, match: { author: [...DEPENDENCY_BOTS] }, playbook: depsPrsName })); + return [...synthesized, ...rolePulls(role)]; +} + +/** The Linear team a role's ops steward files in (the first of linear.teams when the role names none), with its source. */ +export const roleOpsTeam = (role: Role | null, project: ProjectFile): FilingTarget | null => { + const target = role?.ops?.team ?? project.linear.teams[0] ?? null; + return target === null ? null : filingTargetOf(target); +}; + +/** + * Whether a role watches a namespace or Sentry project, so the ops-watch + * playbook is needed. The Dependabot watch runs the separate deps-alerts + * playbook, so it does not count here. + */ +export const roleUsesOps = (role: Role | null): boolean => roleK8sTargets(role).length > 0 || roleSentryTargets(role).length > 0; + +/** + * Whether a project uses a kind of binding at all: reviews can be turned off, + * and ops needs a target. `role` is the server's resolved role: omitted, the + * legacy `project.ops` is read (a project without a roles.json); null, the + * server has no role and runs no ops. + */ +export const usesKind = (project: ProjectFile, kind: BindingKind, role?: Role | null): boolean => + kind === "review" ? project.reviews.enabled : kind === "ops" ? (role === undefined ? project.ops.targets.length > 0 : roleUsesOps(role)) : true; + +/** + * A project file's legacy `ops` as a role, so a project with no roles.json runs + * exactly as before: every server is the project steward and runs every watch. + * The Dependabot default (every repo) becomes `"all"`. + */ +export function legacyRole(project: ProjectFile): Role { + const explicit = project.ops.targets.filter((t): t is DependabotTarget => t.kind === "dependabot").map((t) => t.repo); + return { + projectSteward: true, + ops: { + k8sNamespaces: k8sTargets(project).map((t) => t.name), + ...(project.ops.team === undefined ? {} : { team: project.ops.team }), + ...(project.ops.context === undefined ? {} : { context: project.ops.context }), + autoAdopt: [...project.ops.autoAdopt], + sentry: { projects: sentryTargets(project).map((t) => ({ org: t.org, project: t.project })), resolveOnDone: project.ops.sentry.resolveOnDone }, + dependabot: explicit.length > 0 ? explicit : "all", + pulls: [], + }, + }; +} +/** The words the project steward's problem lines use for one watch. */ +function watchLabel(key: string): string { + if (key.startsWith("k8s-namespace/")) return `the ${key.slice("k8s-namespace/".length)} watch`; + if (key.startsWith("sentry-project/")) return `the Sentry watch for ${key.slice("sentry-project/".length)}`; + return `the Dependabot watch for ${key.slice("dependabot/".length)}`; +} +/** Every watch a role runs, keyed as `opsTargetKey` keys the same target. */ +function roleWatchKeys(role: Role, repos: readonly string[]): string[] { + return [...roleOpsTargets(role), ...roleDepsTargets(role, repos)].map(opsTargetKey); +} + +/** + * The watches the project expects someone to run: the legacy `ops` targets, + * and the Dependabot default (every repo) while the dependency watch is on. A + * watch in no role is unowned; one in two roles is doubly owned. + */ +export function expectedWatches(project: ProjectFile, repos: readonly string[], depsEnabled: boolean): { key: string; label: string }[] { + const out: { key: string; label: string }[] = []; + const seen = new Set(); + const add = (key: string) => { + if (seen.has(key)) return; + seen.add(key); + out.push({ key, label: watchLabel(key) }); + }; + for (const t of k8sTargets(project)) add(opsTargetKey(t)); + for (const t of sentryTargets(project)) add(opsTargetKey(t)); + const explicit = project.ops.targets.filter((t): t is DependabotTarget => t.kind === "dependabot").map((t) => t.repo); + if (explicit.length > 0) for (const repo of explicit) add(opsTargetKey({ kind: "dependabot", repo })); + else if (depsEnabled) for (const repo of repos) add(opsTargetKey({ kind: "dependabot", repo })); + return out; +} + +/** + * The project steward's problem lines about who owns what: a watch nobody + * runs, and a watch two people run. Credentials are checked on each server, + * never here: a role whose machine lacks kubectl or a Sentry token is that + * server's own problem line. + */ +export function watchProblems(project: ProjectFile, roles: RolesFile, repos: readonly string[], depsEnabled: boolean): string[] { + const problems: string[] = []; + const owners = new Map(); + for (const [login, role] of Object.entries(roles.roles)) { + for (const key of roleWatchKeys(role, repos)) owners.set(key, [...(owners.get(key) ?? []), login]); + } + const expected = expectedWatches(project, repos, depsEnabled); + for (const watch of expected) { + const list = owners.get(watch.key) ?? []; + if (list.length === 0) problems.push(`nobody runs ${watch.label}`); + else if (list.length > 1) problems.push(`two people run ${watch.label}: ${list.join(", ")}`); + } + // A watch two roles name that the project does not otherwise expect: a namespace, say. + for (const [key, list] of owners) { + if (list.length > 1 && !expected.some((w) => w.key === key)) problems.push(`two people run ${watchLabel(key)}: ${list.join(", ")}`); + } + return problems; +} +/** + * The `check` problems of every declared pull watch (BB-164): its repo must be + * one the project leases (in `repos.json`, not in `github.exclude`), its + * playbook must exist and be kind `pr`, its globs must parse, and no two roles + * with holders may declare a watch on the same repo — an error, not a warning, + * since it is the only place two servers adopting the same pull request can be + * caught. `playbooks` holds every watch playbook that parsed; a missing one was + * already reported where it was read. + */ +export function pullWatchProblems(roles: RolesFile, repos: readonly string[], excluded: readonly string[], playbooks: ReadonlyMap): string[] { + const problems: string[] = []; + const repoOwners = new Map(); + for (const [login, role] of Object.entries(roles.roles)) { + for (const watch of rolePulls(role)) { + const where = `roles.json: the pull watch on ${watch.repo} for ${JSON.stringify(login)}`; + if (excluded.some((r) => r.toLowerCase() === watch.repo.toLowerCase())) problems.push(`${where}: the repo is in github.exclude, so it is not stewarded`); + else if (!repos.some((r) => r.toLowerCase() === watch.repo.toLowerCase())) problems.push(`${where}: the repo is not in repos.json, so the project has no lease for it and gets no doorbells`); + const playbook = playbooks.get(watch.playbook); + if (playbook !== undefined && playbook.kind !== "pr") problems.push(`${where}: the playbook ${JSON.stringify(watch.playbook)} is a ${playbook.kind} playbook, not a pr playbook`); + for (const [field, value] of [ + ["match.title", watch.match.title], + ["match.branch", watch.match.branch], + ["match.author", Array.isArray(watch.match.author) ? undefined : watch.match.author], + ] as const) { + const bad = value === undefined ? null : globError(value); + if (bad !== null) problems.push(`${where}: ${field} is not a glob: ${bad}`); + } + for (const pattern of watch.match.paths ?? []) { + const bad = globError(pattern); + if (bad !== null) problems.push(`${where}: match.paths ${bad}`); + } + const owners = repoOwners.get(watch.repo.toLowerCase()) ?? []; + if (!owners.includes(login)) repoOwners.set(watch.repo.toLowerCase(), [...owners, login]); + } + } + for (const [repo, owners] of repoOwners) { + if (owners.length > 1) problems.push(`two roles with holders declare pull watches on ${repo}: ${owners.join(", ")} — only one server can adopt a pull request, so remove one`); + } + return problems; +} +/** + * The role names whose definitions produce each of a holder's target keys: what + * `bb stewards list` and the board attribute a binding to. A login with no + * roles, or a target no role owns, maps to nothing. + */ +export function bindingRoles(roles: RolesFile | null, login: string | null, repos: readonly string[]): Map { + const out = new Map(); + if (roles === null || login === null) return out; + for (const name of roles.holders[login] ?? []) { + if (name === PROJECT_STEWARD_ROLE) continue; + const definition = roles.definitions[name]; + if (definition === undefined) continue; + const role: Role = { projectSteward: false, ops: definition }; + for (const target of [...roleOpsTargets(role), ...roleDepsTargets(role, repos)]) { + const key = opsTargetKey(target); + const list = out.get(key) ?? []; + if (!list.includes(name)) out.set(key, [...list, name]); + } + // A pull binding is keyed by the watch that adopted it, not a target key. + for (const watch of rolePulls(role)) { + const key = pullWatchId(watch); + const list = out.get(key) ?? []; + if (!list.includes(name)) out.set(key, [...list, name]); + } + } + return out; +} + +/** + * The project steward count problem, or null when one or more holders hold it. + * Two or more holders is normal (BB-230): each server runs only its own login's + * roles, so every developer whose server runs stewards can hold the role. + */ +export function projectStewardProblem(roles: RolesFile): string | null { + const stewards = Object.entries(roles.roles).filter(([, role]) => role.projectSteward).map(([login]) => login); + if (stewards.length === 0) { + return roles.version === 2 + ? `no one holds ${PROJECT_STEWARD_ROLE}: give one holder the ${PROJECT_STEWARD_ROLE} role in roles.json` + : "no one is the project steward: set projectSteward: true on one login in roles.json"; + } + return null; +} + +/** + * The version 2 validation errors a `check` reports: a holder names a role + * that is neither defined nor the built-in `projectSteward`, or a holder's + * roles disagree on `context` or `team`. The runtime never acts on these: the + * effective role is merged deterministically and the same file keeps running. + */ +export function roleProblems(roles: RolesFile): string[] { + if (roles.version !== 2) return []; + const problems: string[] = []; + for (const [login, names] of Object.entries(roles.holders)) { + for (const name of names) { + if (name !== PROJECT_STEWARD_ROLE && roles.definitions[name] === undefined) { + problems.push(`${login} holds an unknown role ${JSON.stringify(name)}: define it under roles, or use ${PROJECT_STEWARD_ROLE}`); + } + } + const defs = names.map((name) => roles.definitions[name]).filter((def): def is RoleOps => def !== undefined); + const contexts = [...new Set(defs.map((def) => def.context).filter((value): value is string => value !== undefined))]; + if (contexts.length > 1) problems.push(`${login} holds roles with conflicting context (${contexts.join(", ")}): a holder's roles must agree on one context`); + const teams = [...new Set(defs.map((def) => def.team).filter((value): value is string => value !== undefined))]; + if (teams.length > 1) problems.push(`${login} holds roles with conflicting team (${teams.join(", ")}): a holder's roles must agree on one team`); + } + return problems; +} + +/** The version 2 warnings a `check` reports: a defined role no holder names. */ +export function roleWarnings(roles: RolesFile): string[] { + if (roles.version !== 2) return []; + const held = new Set(Object.values(roles.holders).flat()); + return Object.keys(roles.definitions) + .filter((name) => !held.has(name)) + .map((name) => `role ${JSON.stringify(name)} has no holder`); +} + +/** + * The problems with a project's filing targets (BB-235): a GitHub `owner/repo` + * that is not in the project's repo set, and a role's Linear team key that is + * not one of `linear.teams`. A project file's team key is checked by the + * schema itself; this adds the repo case the schema cannot see (the repo set + * comes from `repos.json`) and the role cases the schema never checks. +/** + * The problems with a project's filing targets (BB-235): a GitHub `owner/repo` + * that is not in the project's repo set, and a role's Linear team key that is + * not one of `linear.teams`. A project file's team key is checked by the + * schema itself; this adds the repo case the schema cannot see (the repo set + * comes from `repos.json`) and the role cases the schema never checks. + */ +export function filingTargetProblems(project: ProjectFile, roles: RolesFile | null, repos: readonly string[]): string[] { + const out: string[] = []; + const repoProblem = (where: string, target: string) => { + if (!repos.some((r) => r.toLowerCase() === target.toLowerCase())) out.push(`${where}: ${target} is not in repos.json, so a ticket filed there would never be stewarded`); + }; + const teamProblem = (where: string, target: string) => { + if (!project.linear.teams.includes(target)) out.push(`${where}: ${target} is not one of linear.teams, so a ticket filed there would never be stewarded`); + }; + const projectTarget = project.ops.team; + if (projectTarget !== undefined && REPO_NAME.test(projectTarget)) repoProblem(`${PROJECT_FILE} ops.team`, projectTarget); + for (const [login, role] of Object.entries(roles?.roles ?? {})) { + const target = role.ops?.team; + if (target === undefined) continue; + if (REPO_NAME.test(target)) repoProblem(`${ROLES_FILE} ${login}'s ops.team`, target); + else teamProblem(`${ROLES_FILE} ${login}'s ops.team`, target); + } + return out; +} diff --git a/plugins/stewards/server.test.ts b/plugins/stewards/server.test.ts index b636ec53..39d481c2 100644 --- a/plugins/stewards/server.test.ts +++ b/plugins/stewards/server.test.ts @@ -2,7 +2,8 @@ import Database from "better-sqlite3"; import { createFakePluginHost, makePluginAgentConfigurationContext, makeThreadResponse } from "@get-bb/plugin-sdk/testing"; import { describe, expect, it } from "vitest"; import { PROJECT_CHANGED } from "./channels.js"; -import { PROJECT_FILE, ROLES_FILE, playbookPath } from "./config.js"; +import { PROJECT_FILE, playbookPath } from "./config.js"; +import { ROLES_FILE } from "./roles.js"; import { comment, crashingPod, githubIssue, issue, pull, warningEvent } from "./fixtures.js"; import { kubectlRefusal, type KubectlResult } from "./kubectl.js"; import { retiredLessonAnchor, withLessonAnchor } from "./lessons.js"; diff --git a/plugins/stewards/server.ts b/plugins/stewards/server.ts index 31882b1e..d2ba3f2c 100644 --- a/plugins/stewards/server.ts +++ b/plugins/stewards/server.ts @@ -32,28 +32,41 @@ import { ESCALATION_KINDS, KINDS, PROJECT_FILE, - ROLES_FILE, - bindingRoles, depsAlertsPlaybookName, depsPrsPlaybookName, durationSchema, filingTargetOf, - filingTargetProblems, formatDuration, GITHUB_TODO_LABEL, githubStateLabels, isTerminal, legacyOpsPresent, - legacyRole, modelSpecSchema, parsePlaybook, parseProjectFile, - parseRolesFile, phaseOf, playbookLabels, playbookNameFor, playbookPath, playbookStates, + STAGES, + ticketWatchId, + ticketWatchProblems, + type BindingKind, + type EscalationKind, + type FilingTarget, + type LessonRules, + type ModelSpec, + type Playbook, + type ProjectFile, + type TicketWatch, +} from "./config.js"; +import { + ROLES_FILE, + bindingRoles, + filingTargetProblems, + legacyRole, + parseRolesFile, projectStewardProblem, pullWatchId, pullWatchProblems, @@ -67,23 +80,12 @@ import { roleSentryTargets, roleUsesOps, roleWarnings, - STAGES, - ticketWatchId, - ticketWatchProblems, usesKind, watchProblems, - type BindingKind, - type EscalationKind, - type FilingTarget, - type LessonRules, - type ModelSpec, - type Playbook, - type ProjectFile, type PullWatch, type Role as StewardRole, type RolesFile, - type TicketWatch, -} from "./config.js"; +} from "./roles.js"; import { decideFor, decideForPull, decideForRefusal, explainDecision, formatDecisionListing, globMatches, type BigDecision } from "./decisions.js"; import { ADOPTION_CHOICES, adoptionChoices, adoptionHow, adoptionQuestion, alertClockAt, alertFingerprint, batchAlert, deadlineAt, depsLookSchema, dueDate, isBatchable, overdue, priorityFor, priorityLabel, priorityLabelForSeverity, reminderText, repoOfFingerprint, sightingOf, snoozeUntil, type DepsFinding, type DepsLook, type DepsTicket } from "./deps.js"; import { hostContract } from "./host-contract.js"; From 5fb382ffd48ca5517b21ba02efd2ab6f05aa89ce Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Tue, 10 Nov 2026 04:16:16 +0000 Subject: [PATCH 2/7] stewards: move the ready gate and escalation routing into modules (BB-280) --- plugins/stewards/escalations.ts | 56 +++++++++++++ plugins/stewards/ready-gate.ts | 90 ++++++++++++++++++++ plugins/stewards/server.ts | 140 ++++++-------------------------- 3 files changed, 171 insertions(+), 115 deletions(-) create mode 100644 plugins/stewards/escalations.ts create mode 100644 plugins/stewards/ready-gate.ts diff --git a/plugins/stewards/escalations.ts b/plugins/stewards/escalations.ts new file mode 100644 index 00000000..2552228d --- /dev/null +++ b/plugins/stewards/escalations.ts @@ -0,0 +1,56 @@ +// Escalation routing: where an escalation goes next, how it is delivered to +// the binding that decides it, and the escalations routed to a binding from +// its children. This is the routing seam, kept in its own module so a change +// to it is always the owner's. +// +// The functions take an `EscalationsContext` rather than the server's whole +// closure, so nothing here reads the plugin's config or snapshots on its own. +import type { Binding, Escalation, EscalationRoute, Store } from "./store.js"; + +/** + * What the escalation routing needs from the server: the store, the reconcile + * scheduler a delivered escalation wakes, and the project wake a route that + * reaches the project steward uses. Passed in so this module holds no state. + */ +export type EscalationsContext = { + store: Store; + scheduleReconcile: (bindingId: string) => void; + wakeProject: (projectId: string, reason: string, opts: { digestDue: boolean; notes?: string[] }) => Promise; +}; + +/** The escalations routed to `b` from its children, for the wake to list. */ +export function routedTo(ctx: EscalationsContext, b: Binding): Array<{ escalation: Escalation; from: Binding }> { + return ctx.store.children(b.id).flatMap((child) => ctx.store.openEscalations(child.id).filter((x) => x.route === "parent").map((escalation) => ({ escalation, from: child }))); +} + +/** Where an escalation goes next, above where it is. */ +export function nextRoute(ctx: EscalationsContext, b: Binding, from: EscalationRoute | null): EscalationRoute { + if (from === null) { + const parent = b.parentBindingId === null ? null : ctx.store.binding(b.parentBindingId); + return parent !== null && parent.endedAt === null && parent.threadId !== null ? "parent" : "project"; + } + if (from === "steward" || from === "parent") return "project"; + return "user"; +} + +/** + * Deliver an escalation to whatever decides it: a worker's goes to the + * binding's own steward, a child's to its parent, and a route-`project` one + * wakes the project steward. A route-`user` escalation is the project + * steward's to ask in its own thread, so nothing is delivered here. + */ +export async function deliverEscalation(ctx: EscalationsContext, b: Binding, x: Escalation): Promise { + if (x.route === "user") return; + // A worker's escalation is for the binding's own steward. + if (x.route === "steward") { + ctx.store.note(b.id, `Escalation ${x.id} from a worker (${x.kind}): ${x.question}`); + ctx.scheduleReconcile(b.id); + return; + } + if (x.route === "parent" && b.parentBindingId !== null) { + ctx.store.note(b.parentBindingId, `Escalation ${x.id} from ${b.entityKey} (${x.kind}): ${x.question}`); + ctx.scheduleReconcile(b.parentBindingId); + return; + } + await ctx.wakeProject(b.projectId, `escalation from ${b.entityKey}`, { digestDue: false }); +} diff --git a/plugins/stewards/ready-gate.ts b/plugins/stewards/ready-gate.ts new file mode 100644 index 00000000..53877daf --- /dev/null +++ b/plugins/stewards/ready-gate.ts @@ -0,0 +1,90 @@ +// The ready gate's plugin-side plumbing: the gate a brief reads, and the +// approval request the plugin keeps in step with the gate. This is the ready +// gate seam, kept in its own module so a change to it is always the owner's. +// +// The functions take a `ReadyGateContext` rather than the server's whole +// closure, so nothing here reads the plugin's config or snapshots on its own. +import { approvalWanted, gateState, specDiff } from "./reconcile.js"; +import type { EntitySnapshot } from "./source-contract.js"; +import type { Binding, Escalation, Store } from "./store.js"; +import type { Playbook } from "./config.js"; + +/** + * What the ready gate needs from the server: the store, the spec hash and text + * a binding's approval is pinned to (they read the store for the user's own + * spec comments), and the plugin log. Passed in so this module holds no state. + */ +export type ReadyGateContext = { + store: Store; + specHashFor: (b: Pick, entity: EntitySnapshot) => string; + specTextFor: (b: Pick, entity: EntitySnapshot) => string; + log: (message: string) => void; +}; + +/** + * The ready gate for a brief: the live spec hash when the binding has a + * snapshot, so an approval whose spec changed reads as not passed. The wake + * does the same; a brief pinned to the approval's own hash would call a stale + * gate passed on exactly the compacted/non-wake turns it protects. + */ +export const briefGate = (ctx: ReadyGateContext, b: Binding, playbook: Playbook) => + gateState(playbook, b.ready, b.lastSnapshot === null ? (b.ready.approval?.specHash ?? "") : ctx.specHashFor(b, b.lastSnapshot.entity)); + +/** + * Keep the gate's approval request in step with the gate. While the + * checklist is met, the ticket is substantial and the current spec has + * no approval, one `approval` escalation is open with the approver: the + * project steward, or the user with `approval: user`. It is the plugin's, + * so the approver hears whatever the steward wrote, and it closes itself + * once the gate passes or no longer needs an approval. A request the + * approver declined (guidance + resolve) is not made again until the + * spec changes. A Linear-state approval is the steward's to wait for. + * + * Returns a request just filed with the project steward, for the caller + * to wake it, once, with whatever else it has to say. + */ +export async function syncApprovalRequest(ctx: ReadyGateContext, b: Binding, playbook: Playbook): Promise { + const mode = playbook.ready?.approval ?? null; + if (mode === null || typeof mode === "object" || b.lastSnapshot === null) return null; + // `approval: user` is the user's, and is filed as an `approve-spec` need by + // syncNeeds; the escalation below is only for the project steward. + if (mode === "user") return null; + const open = ctx.store.openEscalations(b.id).filter((x) => x.kind === "approval" && x.resolutionKind !== "approve-decline"); + const current = ctx.specHashFor(b, b.lastSnapshot.entity); + const gate = gateState(playbook, b.ready, current); + const wanted = approvalWanted(gate) && b.endedAt === null && b.ending === null && b.ready.declinedSpecHash !== current; + if (!wanted) { + const why = gate.passed ? "The spec is approved." : !gate.checklistMet ? "Withdrawn: the ready checklist is no longer met." : !gate.approvalNeeded ? "Withdrawn: the ticket is not substantial." : "Withdrawn."; + for (const x of open) ctx.store.resolve(x.id, why); + return null; + } + if (open.length > 0) return null; + // BB-251: the request lists the open assumptions it covers, and records + // their ids, so the approval confirms exactly those. An assumption + // recorded after this request was shown is not on the list and stays open. + const coveredAssumptions = ctx.store.openAssumptions(b.id); + const assumptionListing = + coveredAssumptions.length === 0 ? "" : ` It covers ${coveredAssumptions.length} open assumption${coveredAssumptions.length === 1 ? "" : "s"}: ${coveredAssumptions.map((a) => `${a.id} — ${a.text}`).join("; ")}.`; + // BB-249: a re-approval after an edit carries the same diff the user's + // item does, so the project steward reads a change rather than a document. + const prior = b.ready.approval; + const approvedText = prior?.approvedText ?? b.ready.voidedApprovedText; + const reapproval = approvedText !== null ? specDiff(approvedText, ctx.specTextFor(b, b.lastSnapshot.entity)) : null; + const x = ctx.store.escalate({ + bindingId: b.id, + kind: "approval", + question: + `Approve ${b.entityKey}'s spec${b.title === null ? "" : ` (${b.title})`}? The ready checklist is met and the ticket is ${b.ready.substantial === null ? "not yet judged, so treated as substantial" : "substantial"}${b.ready.why === null ? "" : ` (${b.ready.why})`}; ` + + (gate.approvalStale ? "the spec has changed since its last approval." : "the current spec has no approval.") + + assumptionListing, + meanwhile: "Waiting: no implementation workers start until it is approved.", + route: "project", + recommendation: null, + data: { + assumptionIds: coveredAssumptions.map((a) => a.id), + ...(reapproval === null ? {} : { specDiff: reapproval.diff, specChangedLines: reapproval.changedLines }), + }, + }); + ctx.log(`stewards: ${b.entityKey}'s ready gate waits on the project steward; asked for approval (${x.id})`); + return x; +} diff --git a/plugins/stewards/server.ts b/plugins/stewards/server.ts index d2ba3f2c..09d9eba4 100644 --- a/plugins/stewards/server.ts +++ b/plugins/stewards/server.ts @@ -98,6 +98,8 @@ import { EVENTS_PER_LOOK, NO_TOKEN, RECENT_PER_LOOK, SENTRY_API_URL, TOKEN_SCOPE import { advanced, answerDiffers, anyWorkerInFlight, approvalWanted, asksForOpenApproval, classifyApproval, commentDiffersFromRecommendation, compactDue, deploymentSettled, diffObserved, endingDeadline, followingThrough, gateKnownUnmet, gateState, hasLabel, IDLE_WAKE_TEXT, idleInActiveStage, isCommentNews, isOwnStageChange, isSpecComment, latestReviewMarker, lifecycle, mechanicalStatus, namesUserAsBlocker, newHumanComments, newestCommentAt, openUserFacingItems, ownChecksFinished, ownPullClosed, ownPullMerged, phaseAndStage, productGateOptions, resolveChecklistItem, runningWorkerReason, newestEditComment, specDiff, specHash, specText, stuckDue, unheardWait, workerInFlight, workerReportedTerminal, type Change, type MechanicalEvent, type OpenUserItem } from "./reconcile.js"; import { ESCALATED_APPROVAL_REFUSAL, latestProjectApprovalMarker, prGate, PRODUCT_APPROVAL_REFUSAL } from "./gate.js"; import { approvePrPageAction, deliverProjectApproval, fileForwardedApprovePrNeed, markerKey, recordProjectDecision, syncPullApproval, type ApprovalsContext } from "./approvals.js"; +import { deliverEscalation, nextRoute, routedTo, type EscalationsContext } from "./escalations.js"; +import { briefGate, syncApprovalRequest, type ReadyGateContext } from "./ready-gate.js"; import { SOURCE_PLUGIN_IDS, STEWARDS_PLUGIN_ID, @@ -117,7 +119,7 @@ import { } from "./source-contract.js"; import { DEPLOY_STATES, allDeployed, applyDeployment, applyDeploymentRead, applyWorkflowRun, awaitingOnRepo, deploymentLines, deploymentReportSchema, matchesTarget, matchesWorkflow, mergedPulls, pickWatch, recordMerged, reportDeployment, targetKeyOf, timeOut, timeoutsDue } from "./deploy.js"; import { evaluateCarries, matchCarriesRule, noRuleNoted, type CarriesReader } from "./carries.js"; -import { ACTORS, MIGRATIONS, ESCALATED_NEED_KINDS, NEEDS_KINDS, RESOLUTION_KINDS, createStore, needChoices, type Assumption, type Binding, type DepsDecision, type Escalation, type EscalationRoute, type Fingerprint, type Lesson, type Need, type Observed, type Question, type ReadyState, type Store, type WakeRetry, type Worker } from "./store.js"; +import { ACTORS, MIGRATIONS, ESCALATED_NEED_KINDS, NEEDS_KINDS, RESOLUTION_KINDS, createStore, needChoices, type Assumption, type Binding, type DepsDecision, type Escalation, type Fingerprint, type Lesson, type Need, type Observed, type Question, type ReadyState, type Store, type WakeRetry, type Worker } from "./store.js"; import { hasLessonAnchor, lessonAnchor, planLessonTransitions, planValidations, retiredLessonAnchor, withLessonAnchor } from "./lessons.js"; import { AFTER_THE_FACT_RATIONALE_WHY, REPEAT_RATIONALE_WHY, ROUTINE_RATIONALE_WHY, clusterKey, routineRationaleWanted } from "./rationale.js"; import { GUIDANCE_UNIT_PREFIX, GUIDANCE_UNCITED_DAYS, escalationWeeks, parseGuidanceUnits, uncitedGuidanceUnits, type GuidanceUnit } from "./guidance.js"; @@ -3301,20 +3303,13 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P /** The text a binding's approval pins, stored as `approvedText` so BB-249 can diff a later edit against it. */ const specTextFor = (b: Pick, entity: EntitySnapshot): string => specText(entity, b.threadId, isUserThread); - /** - * The ready gate for a brief: the live spec hash when the binding has a - * snapshot, so an approval whose spec changed reads as not passed. The - * wake does the same; a brief pinned to the approval's own hash would call - * a stale gate passed on exactly the compacted/non-wake turns it protects. - */ - const briefGate = (b: Binding, playbook: Playbook) => - gateState(playbook, b.ready, b.lastSnapshot === null ? (b.ready.approval?.specHash ?? "") : specHashFor(b, b.lastSnapshot.entity)); + /** The escalation-routing helpers bound to this server's store and wakes (escalations.ts). */ + const escalationsCtx: EscalationsContext = { store, scheduleReconcile, wakeProject }; - const ownThreads = (b: Binding) => new Set([b.threadId, ...store.workers(b.id).map((w) => w.threadId)].filter((t): t is string => t !== null)); + /** The ready-gate helpers bound to this server's store and spec readers (ready-gate.ts). */ + const readyGateCtx: ReadyGateContext = { store, specHashFor, specTextFor, log: (message) => bb.log.info(message) }; - function routedTo(b: Binding): Array<{ escalation: Escalation; from: Binding }> { - return store.children(b.id).flatMap((child) => store.openEscalations(child.id).filter((x) => x.route === "parent").map((escalation) => ({ escalation, from: child }))); - } + const ownThreads = (b: Binding) => new Set([b.threadId, ...store.workers(b.id).map((w) => w.threadId)].filter((t): t is string => t !== null)); /** A delivered wake, and the respawn the recovery path made, when it made one (BB-103). */ type WakeDelivery = false | { delivered: true; respawned: { from: string | null; to: string } | null }; @@ -3412,7 +3407,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P observed, gate, escalations: store.openEscalations(b.id), - routedToMe: routedTo(b), + routedToMe: routedTo(escalationsCtx, b), workers: store.workers(b.id), workerQuestions, children: store.children(b.id).map((c) => ({ binding: c, openEscalations: store.openEscalations(c.id) })), @@ -3685,14 +3680,14 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P question: `The steward's wake failed after ${attempts}: ${reason}.`, context: "Every retry the plugin made failed the same way, so the steward could not be woken.", meanwhile: "Waiting: the steward cannot be woken until the provider recovers; a later reconcile will try again.", - route: nextRoute(b, null), + route: nextRoute(escalationsCtx, b, null), recommendation: null, projectId: b.projectId, origin: { threadId: args.threadId, bindingId: b.id, chain: [] }, }); // The failed steward is not re-woken, but the escalation's route // (project steward, parent, or the user) is told. - await deliver(b, escalation); + await deliverEscalation(escalationsCtx, b, escalation); } } @@ -4155,7 +4150,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // No turn is coming to report the gate, so look at it here: a gate // left waiting on its approver (a report from before this check, a // request declined, then the spec changed) is asked for now. - const approvalAsked = !b.awaitingReport && (await syncApprovalRequest(b, playbook)) !== null; + const approvalAsked = !b.awaitingReport && (await syncApprovalRequest(readyGateCtx, b, playbook)) !== null; const asked = [approvalAsked ? `approval requested for ${b.entityKey}` : null, scheduling === null ? null : `scheduling requested for ${b.entityKey}`].filter((r) => r !== null); if (asked.length > 0) await wakeProject(b.projectId, asked.join(" and "), { digestDue: false }); return; @@ -6315,91 +6310,6 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P } } - /** Where an escalation goes next, above where it is. */ - function nextRoute(b: Binding, from: EscalationRoute | null): EscalationRoute { - if (from === null) { - const parent = b.parentBindingId === null ? null : store.binding(b.parentBindingId); - return parent !== null && parent.endedAt === null && parent.threadId !== null ? "parent" : "project"; - } - if (from === "steward" || from === "parent") return "project"; - return "user"; - } - - async function deliver(b: Binding, x: Escalation): Promise { - if (x.route === "user") return; - // A worker's escalation is for the binding's own steward. - if (x.route === "steward") { - store.note(b.id, `Escalation ${x.id} from a worker (${x.kind}): ${x.question}`); - scheduleReconcile(b.id); - return; - } - if (x.route === "parent" && b.parentBindingId !== null) { - store.note(b.parentBindingId, `Escalation ${x.id} from ${b.entityKey} (${x.kind}): ${x.question}`); - scheduleReconcile(b.parentBindingId); - return; - } - await wakeProject(b.projectId, `escalation from ${b.entityKey}`, { digestDue: false }); - } - - /** - * Keep the gate's approval request in step with the gate. While the - * checklist is met, the ticket is substantial and the current spec has - * no approval, one `approval` escalation is open with the approver: the - * project steward, or the user with `approval: user`. It is the plugin's, - * so the approver hears whatever the steward wrote, and it closes itself - * once the gate passes or no longer needs an approval. A request the - * approver declined (guidance + resolve) is not made again until the - * spec changes. A Linear-state approval is the steward's to wait for. - * - * Returns a request just filed with the project steward, for the caller - * to wake it, once, with whatever else it has to say. - */ - async function syncApprovalRequest(b: Binding, playbook: Playbook): Promise { - const mode = playbook.ready?.approval ?? null; - if (mode === null || typeof mode === "object" || b.lastSnapshot === null) return null; - // `approval: user` is the user's, and is filed as an `approve-spec` need by - // syncNeeds; the escalation below is only for the project steward. - if (mode === "user") return null; - const open = store.openEscalations(b.id).filter((x) => x.kind === "approval" && x.resolutionKind !== "approve-decline"); - const current = specHashFor(b, b.lastSnapshot.entity); - const gate = gateState(playbook, b.ready, current); - const wanted = approvalWanted(gate) && b.endedAt === null && b.ending === null && b.ready.declinedSpecHash !== current; - if (!wanted) { - const why = gate.passed ? "The spec is approved." : !gate.checklistMet ? "Withdrawn: the ready checklist is no longer met." : !gate.approvalNeeded ? "Withdrawn: the ticket is not substantial." : "Withdrawn."; - for (const x of open) store.resolve(x.id, why); - return null; - } - if (open.length > 0) return null; - // BB-251: the request lists the open assumptions it covers, and records - // their ids, so the approval confirms exactly those. An assumption - // recorded after this request was shown is not on the list and stays open. - const coveredAssumptions = store.openAssumptions(b.id); - const assumptionListing = - coveredAssumptions.length === 0 ? "" : ` It covers ${coveredAssumptions.length} open assumption${coveredAssumptions.length === 1 ? "" : "s"}: ${coveredAssumptions.map((a) => `${a.id} — ${a.text}`).join("; ")}.`; - // BB-249: a re-approval after an edit carries the same diff the user's - // item does, so the project steward reads a change rather than a document. - const prior = b.ready.approval; - const approvedText = prior?.approvedText ?? b.ready.voidedApprovedText; - const reapproval = approvedText !== null ? specDiff(approvedText, specTextFor(b, b.lastSnapshot.entity)) : null; - const x = store.escalate({ - bindingId: b.id, - kind: "approval", - question: - `Approve ${b.entityKey}'s spec${b.title === null ? "" : ` (${b.title})`}? The ready checklist is met and the ticket is ${b.ready.substantial === null ? "not yet judged, so treated as substantial" : "substantial"}${b.ready.why === null ? "" : ` (${b.ready.why})`}; ` + - (gate.approvalStale ? "the spec has changed since its last approval." : "the current spec has no approval.") + - assumptionListing, - meanwhile: "Waiting: no implementation workers start until it is approved.", - route: "project", - recommendation: null, - data: { - assumptionIds: coveredAssumptions.map((a) => a.id), - ...(reapproval === null ? {} : { specDiff: reapproval.diff, specChangedLines: reapproval.changedLines }), - }, - }); - bb.log.info(`stewards: ${b.entityKey}'s ready gate waits on the project steward; asked for approval (${x.id})`); - return x; - } - /** * Keep the scheduling request in step with the world. A ticket whose ready * gate has passed, with no worker running yet, that touches what another @@ -7129,8 +7039,8 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P } async function raise(b: Binding, kind: EscalationKind, question: string, meanwhile: string): Promise { - const x = store.escalate({ bindingId: b.id, kind, question, meanwhile, route: nextRoute(b, null), recommendation: null }); - await deliver(b, x); + const x = store.escalate({ bindingId: b.id, kind, question, meanwhile, route: nextRoute(escalationsCtx, b, null), recommendation: null }); + await deliverEscalation(escalationsCtx, b, x); return x; } @@ -8030,14 +7940,14 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P question: `Could not merge ${pull.key}: ${message}`, context: `The lane gate was met, so the plugin attempted the merge itself (BB-257), and GitHub refused it. Resolve the conflict or the branch protection and push; the plugin retries when the head sha or a gate input changes, or retry once the cause is fixed with \`bb stewards merge ${pull.key}\`. GitHub said: ${message}`, meanwhile: "The plugin does not retry the same gate; the pull request stays open.", - route: nextRoute(b, null), + route: nextRoute(escalationsCtx, b, null), recommendation: null, url: pull.url ?? pull.key, headSha: g.headSha, data: { mergeRefusal: true, mergeKey: key }, projectId: b.projectId, }); - await deliver(b, x); + await deliverEscalation(escalationsCtx, b, x); } /** @@ -8854,7 +8764,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const hostId = cachedProjectHost(b.projectId); const flags = hostId === undefined ? null : [environmentFlags(config?.source ?? null, hostId), workerFlags(workerModel(b, playbook))].filter((f) => f !== "").join(" "); return formatBrief( - { binding: b, playbook, gate: briefGate(b, playbook), workerFlags: flags, escalations: store.openEscalations(b.id), conflicts: conflictsOf(b, store.open(b.projectId)) }, + { binding: b, playbook, gate: briefGate(readyGateCtx, b, playbook), workerFlags: flags, escalations: store.openEscalations(b.id), conflicts: conflictsOf(b, store.open(b.projectId)) }, BRIEF_MAX_CHARS, ); }); @@ -9435,7 +9345,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // Delivered after the status is written, together with any approval // request this report makes due, so the project steward reads the // new status and is woken once. - const raised = input.escalate === undefined ? null : store.escalate({ bindingId: b.id, kind: input.escalate.kind, question: input.escalate.question, context: input.escalate.context ?? null, meanwhile: input.escalate.meanwhile, route: nextRoute(b, null), recommendation: null, projectId: b.projectId, origin: { threadId, bindingId: b.id, chain: [] }, ...(input.escalate.choices === undefined ? {} : { choices: input.escalate.choices }) }); + const raised = input.escalate === undefined ? null : store.escalate({ bindingId: b.id, kind: input.escalate.kind, question: input.escalate.question, context: input.escalate.context ?? null, meanwhile: input.escalate.meanwhile, route: nextRoute(escalationsCtx, b, null), recommendation: null, projectId: b.projectId, origin: { threadId, bindingId: b.id, chain: [] }, ...(input.escalate.choices === undefined ? {} : { choices: input.escalate.choices }) }); const open = store.openEscalations(b.id); const status = { phase: input.phase, @@ -9498,7 +9408,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P if (noteBeyondMaxWait) { store.noteProject(b.projectId, `${b.entityKey} reported a dated wait beyond the playbook's max-wait (${formatDuration(maxWaitMs)}): waiting on ${JSON.stringify(waitingOn ?? "")} until ${iso(untilAt!)}.`); } - const requested = playbook === null ? null : await syncApprovalRequest(b, playbook); + const requested = playbook === null ? null : await syncApprovalRequest(readyGateCtx, b, playbook); const scheduling = playbook === null ? null : await syncScheduleRequest(b, playbook); // An `approve-pr` or `merge` item is filed only when the lane gate // can hold it: the page's Approve and Merge buttons are already @@ -9610,9 +9520,9 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P }); bb.log.info(`stewards: ${b.entityKey} has requeued ${count} times with the same wait; escalated a stall (${stallEscalation.id})`); } - if (raised !== null && !(raised.route === "project" && requests.length > 0)) await deliver(b, raised); + if (raised !== null && !(raised.route === "project" && requests.length > 0)) await deliverEscalation(escalationsCtx, b, raised); if (requests.length > 0) await wakeProject(b.projectId, raised !== null && raised.route === "project" ? `escalation and ${requests.join(" and ")} request from ${b.entityKey}` : `${requests.join(" and ")} requested for ${b.entityKey}`, { digestDue: false }); - if (stallEscalation !== null && requests.length === 0 && !(raised !== null && raised.route === "project")) await deliver(b, stallEscalation); + if (stallEscalation !== null && requests.length === 0 && !(raised !== null && raised.route === "project")) await deliverEscalation(escalationsCtx, b, stallEscalation); // The unread dot means "needs you": a turn that ends with nothing for // the user is marked read, so a "nothing to do yet" look does not // flag the sidebar like a question does. A turn that does need the @@ -10176,7 +10086,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P store.resolve(forwarding.id, `Covered by ${covering.id} (${covering.kind}): ${covering.text}`, threadId); done.push(`resolved ${forwarding.id} as covered by ${covering.id} (${covering.kind}); the ticket steward is woken with it`); } else if (forwarding !== null) { - const route = nextRoute(target, forwarding.route); + const route = nextRoute(escalationsCtx, target, forwarding.route); const recommendation = input.recommendation ?? forwarding.recommendation; const resolutionKind = input.resolution ?? (forwarding.choices.length > 0 ? "choice" : "text"); // A forwarded approval is the user's to decide exactly as @@ -10278,7 +10188,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // BB-213: a forward that reaches the user files the real item, so // the auto-filed `nextAction` item closes rather than sit beside it. if (route === "user") reconcileAutoNeed(target, target.status?.actor ?? "steward"); - await deliver(target, store.escalation(forwarding.id)!); + await deliverEscalation(escalationsCtx, target, store.escalation(forwarding.id)!); if (approvalToUser) done.push(`forwarded ${forwarding.id} to the user as an approve-spec item: they press Approve or Decline on the Stewards page, so do not ask them in prose`); else if (prToUser) done.push(`forwarded ${forwarding.id} to the user as an approve-pr item: they approve on GitHub from the Stewards page`); else if (q !== null) done.push(`forwarded ${forwarding.id} to the user as question ${q.id}: ask them in your reply, then record their answer with guidance + resolve`); @@ -11401,7 +11311,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P if (config === null || playbook === null) throw new PluginCliError(`No playbook for ${b.entityKey}: run bb stewards check.`, { code: "playbook_missing" }); return { exitCode: 0, - stdout: formatBrief({ binding: b, playbook, gate: briefGate(b, playbook), workerFlags: await workerFlagsFor(b, playbook, config.source), escalations: store.openEscalations(b.id), conflicts: conflictsOf(b, store.open(b.projectId)) }), + stdout: formatBrief({ binding: b, playbook, gate: briefGate(readyGateCtx, b, playbook), workerFlags: await workerFlagsFor(b, playbook, config.source), escalations: store.openEscalations(b.id), conflicts: conflictsOf(b, store.open(b.projectId)) }), }; }, }), @@ -11706,7 +11616,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P projectId: b.projectId, origin: { threadId, bindingId: b.id, chain: [] }, }); - await deliver(b, x); + await deliverEscalation(escalationsCtx, b, x); scheduleReconcile(b.id); return { exitCode: 0, stdout: `Escalation ${x.id} is with your steward.` }; }, From 9199cefa59db9091c6b215af7f362f4c4d2e81de Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Tue, 10 Nov 2026 04:29:45 +0000 Subject: [PATCH 3/7] stewards: address review round 1 (BB-280) --- plugins/stewards/config.ts | 3 +-- plugins/stewards/roles.ts | 5 +++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/plugins/stewards/config.ts b/plugins/stewards/config.ts index d37f395e..b4fa7df4 100644 --- a/plugins/stewards/config.ts +++ b/plugins/stewards/config.ts @@ -8,12 +8,11 @@ import { createHash } from "node:crypto"; import { parse as parseYaml } from "yaml"; import { z } from "zod"; import { decideEntrySchema, defaultDecideList, globError } from "./decisions.js"; -import { SENTRY_SLUG, opsTargetKey, type DependabotTarget, type K8sTarget, type OpsTarget, type SentryTarget } from "./ops.js"; +import { SENTRY_SLUG, type DependabotTarget, type K8sTarget, type OpsTarget, type SentryTarget } from "./ops.js"; import { DEPENDABOT_SEVERITIES, REPO_NAME } from "./source-contract.js"; export const STEWARDS_DIR = "stewards"; export const PROJECT_FILE = `${STEWARDS_DIR}/project.json`; -/** The per-person roles: which watches and the project stewardship a GitHub login's server runs. */ export const playbookPath = (name: string) => `${STEWARDS_DIR}/playbooks/${name}.md`; export const DEFAULT_LABEL = "bb-managed"; diff --git a/plugins/stewards/roles.ts b/plugins/stewards/roles.ts index ebbc33e2..c0437b5b 100644 --- a/plugins/stewards/roles.ts +++ b/plugins/stewards/roles.ts @@ -4,14 +4,14 @@ // seam, kept in its own module so a change to it is always the owner's. // // It reads nothing on its own: the project file's schema primitives and its -// own `check` problems come from config.ts, and the store is passed to the -// functions that need it. +// own `check` problems come from config.ts. import { z } from "zod"; import { globError } from "./decisions.js"; import { opsTargetKey, type DependabotTarget, type K8sTarget, type OpsTarget, type SentryTarget } from "./ops.js"; import { REPO_NAME } from "./source-contract.js"; import { DEPENDENCY_BOTS, dependabotRepoSchema, filingTargetSchema, filingTargetOf, k8sTargets, LINEAR_PRIORITIES, playbookName, PROJECT_FILE, sentrySlug, sentryTargets, STEWARDS_DIR, zodMessage, type BindingKind, type FilingTarget, type Parsed, type Playbook, type ProjectFile } from "./config.js"; +/** The per-person roles: which watches and the project stewardship a GitHub login's server runs. */ export const ROLES_FILE = `${STEWARDS_DIR}/roles.json`; // ---- roles: per-person watches, keyed by GitHub login ------------------------------ @@ -97,6 +97,7 @@ export type Role = z.infer; /** The built-in role name for the project steward. It has no `roles` definition. */ export const PROJECT_STEWARD_ROLE = "projectSteward"; +/** A named role in a version 2 file. */ const roleNameSchema = z.string().min(1).max(63).regex(/^[A-Za-z][A-Za-z0-9_-]*$/u, "a role name such as ops"); /** From 8c6c6fd12ab2d2aee34bc62bd9c686faf5a3f457 Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Tue, 10 Nov 2026 04:50:32 +0000 Subject: [PATCH 4/7] stewards: move the approval-recording path into approvals.ts (BB-280) --- plugins/stewards/approvals.test.ts | 11 +++++ plugins/stewards/approvals.ts | 75 +++++++++++++++++++++++++++++- plugins/stewards/server.ts | 58 ++++++----------------- 3 files changed, 98 insertions(+), 46 deletions(-) diff --git a/plugins/stewards/approvals.test.ts b/plugins/stewards/approvals.test.ts index dbd984a2..7bec83b2 100644 --- a/plugins/stewards/approvals.test.ts +++ b/plugins/stewards/approvals.test.ts @@ -7,6 +7,7 @@ // to a parked worker by the same path (`deliverProjectApproval`). import { describe, expect, it, vi } from "vitest"; import { approvePrPageAction, deliverProjectApproval, recordProjectDecision, type ApprovalsContext } from "./approvals.js"; +import { isSelfWidening } from "./decisions.js"; import { comment, parkedWorker, pull } from "./fixtures.js"; import type { Binding, Need } from "./store.js"; @@ -143,3 +144,13 @@ describe("an approval is delivered to a parked worker (BB-279)", () => { expect(sendTo).not.toHaveBeenCalled(); }); }); + +// BB-280: the approve/decline recording path lives in this module, so a change +// to it must always be the user's, whatever the decide list matched. +describe("the approval-recording files are self-widening (BB-280)", () => { + it("classifies approvals.ts and approvals-store.ts as self-widening", () => { + for (const file of ["bb-plugins/plugins/stewards/approvals.ts", "plugins/stewards/approvals.ts", "bb-plugins/plugins/stewards/approvals-store.ts", "plugins/stewards/approvals-store.ts"]) { + expect(isSelfWidening([file]), file).toBe(true); + } + }); +}); diff --git a/plugins/stewards/approvals.ts b/plugins/stewards/approvals.ts index 52365664..17b636c9 100644 --- a/plugins/stewards/approvals.ts +++ b/plugins/stewards/approvals.ts @@ -7,12 +7,12 @@ // // The functions take an `ApprovalsContext` rather than the server's whole // closure, so nothing here reads the plugin's config or snapshots on its own. -import { type BigDecision, explainDecision, formatDecisionListing } from "./decisions.js"; +import { type BigDecision, decideFor, decideForPull, decideForRefusal, explainDecision, formatDecisionListing, type DecideMark } from "./decisions.js"; import { approvalMarkerLine, ESCALATED_APPROVAL_REFUSAL, prGate, PRODUCT_APPROVAL_REFUSAL, reviewWorkerThread, type PrGate } from "./gate.js"; import { parsePullRef, type GithubWriter } from "./github.js"; import { productGateOptions, productReason, workerInFlight, type MechanicalEvent } from "./reconcile.js"; import { signatureLine, type EntitySnapshot, type Source } from "./source-contract.js"; -import type { Binding, Need, Observed, Store } from "./store.js"; +import type { Binding, Escalation, Need, Observed, Store } from "./store.js"; const errorMessage = (error: unknown) => (error instanceof Error ? error.message : String(error)); @@ -34,6 +34,8 @@ export type ApprovalsContext = { wakeProject: (projectId: string, reason: string, opts: { digestDue: boolean; notes?: string[] }) => Promise; pullChangedFiles: (entity: EntitySnapshot) => Promise; loadDecisions: (projectId: string) => Promise; + /** The project's decide list, or null when the project file could not be read (BB-280). */ + loadDecisionsOrNull: (projectId: string) => Promise; observedPulls: (b: Binding, observed: Observed) => EntitySnapshot[]; /** Write the status the plugin observed itself, without a steward turn (BB-203). */ mechanicalStatus: (b: Binding, event: MechanicalEvent) => void; @@ -120,6 +122,75 @@ export async function recordProjectDecision(ctx: ApprovalsContext, b: Binding, p return { commentId }; } +/** Whether `threadId` is the thread of `b`'s project steward (BB-280). */ +export const isProjectSteward = (ctx: ApprovalsContext, threadId: string, b: Binding): boolean => ctx.store.project(b.projectId).threadId === threadId; + +/** + * Whether the project steward's thread may approve `b`'s spec under the + * playbook's `mode`: `project` and the project steward's own thread. `user` is + * the user's, recorded by quoting their words (BB-280; this acceptance was in + * server.ts before). + */ +export const specApprovedByProject = (ctx: ApprovalsContext, threadId: string, b: Binding, mode: "project" | "user"): boolean => mode === "project" && isProjectSteward(ctx, threadId, b); + +/** + * The decide-list placement of a pull request a steward is reporting: + * `decideForPull` on its changed files, so a steward cannot file the user's + * item for the project steward's lane (BB-51, BB-280). + */ +export const placeReportedPull = async (ctx: ApprovalsContext, pull: EntitySnapshot, decisions: readonly BigDecision[]): Promise => decideForPull(await ctx.pullChangedFiles(pull), decisions); + +/** The outcome of the project steward's decision on an owner-reviewed pull request from `steward_update`. */ +export type ProjectPullDecision = { ok: true; done: string } | { ok: false; error: string }; + +/** + * Record the project steward's approval of an owner-reviewed pull request + * (`steward_update { approve: true, entry }`): the isProjectSteward check, the + * reviewer's owner call refusal, the decide-list placement, the marker and the + * delivery. The decide list comes from the context, null when the project file + * could not be read. The whole path was in server.ts before (BB-280). + */ +export async function approveOwnerReviewedPull(ctx: ApprovalsContext, b: Binding, prApproval: Escalation, threadId: string, fields: { entry: string | null; guidance: string | undefined }): Promise { + if (!isProjectSteward(ctx, threadId, b)) return { ok: false, error: "An owner-reviewed pull request is the project steward's to approve; a steward escalates it and waits." }; + const decisions = await ctx.loadDecisionsOrNull(b.projectId); + if (decisions === null) return { ok: false, error: "The project file could not be read, so no project-steward approval can be recorded." }; + const observed = b.lastSnapshot; + const pull = observed === null ? null : [observed.entity, ...observed.pulls].find((e) => (e.url ?? e.key) === prApproval.url) ?? null; + // A reviewer's owner call in the marker is binding: the project steward does + // not decide it, whatever entry the decide list matched, and never posts a + // `bb-approval` on it (BB-192). The product reason (BB-207) binds it the same way. + const approvalGate = pull === null ? null : prGate(pull, { projectApprovals: ctx.store.projectApprovals(b.id), ...productGateOptions(b, pull) }); + if (approvalGate !== null && approvalGate.escalated) return { ok: false, error: approvalGate.reason === "product" ? PRODUCT_APPROVAL_REFUSAL : ESCALATED_APPROVAL_REFUSAL }; + const files = pull === null ? null : await ctx.pullChangedFiles(pull); + const decision = decideFor(files, fields.entry, decisions); + if (decision !== "project") { + const why = decideForRefusal(files, fields.entry, decisions) ?? "the change is not the project steward's to approve"; + return { ok: false, error: `That pull request is the user's to approve: ${why}. Forward it with steward_update { binding: "${b.entityKey}", forward: "${prApproval.id}", resolution: "approve-decline", recommendation } instead.` }; + } + const answer = fields.guidance ?? `Approved as ${JSON.stringify(fields.entry)}.`; + if (pull === null) return { ok: false, error: "The pull request could not be read, so the approval marker cannot be posted." }; + try { + await recordProjectDecision(ctx, b, pull, threadId, { headSha: prApproval.headSha ?? "", entry: fields.entry, verdict: "approved", reasoning: answer }); + } catch (error) { + return { ok: false, error: error instanceof Error ? error.message : String(error) }; + } + ctx.store.addGuidance(b.id, answer, threadId); + ctx.store.answerEscalation(prApproval.id, answer, threadId); + const reached = await deliverProjectApproval(ctx, b, pull, { verdict: "approved", sha: prApproval.headSha ?? "", entry: fields.entry, reasoning: answer }); + return { ok: true, done: `posted the project steward's approval marker on ${prApproval.url} at ${ctx.shortSha(prApproval.headSha)} and ${reached === "worker" ? "told its worker" : "woke the steward"}` }; +} + +/** + * Record the project steward's decline of an owner-reviewed pull request that + * resolves an `approve-decline` escalation: the marker first, then the + * delivery, so a GitHub failure leaves the request open to retry rather than + * recording a decision nobody can see. The path was in server.ts before (BB-280). + */ +export async function declineOwnerReviewedPull(ctx: ApprovalsContext, b: Binding, pull: EntitySnapshot, threadId: string, fields: { headSha: string | null; entry: string | null; reasoning: string }): Promise { + await recordProjectDecision(ctx, b, pull, threadId, { headSha: fields.headSha ?? "", entry: fields.entry, verdict: "declined", reasoning: fields.reasoning }); + await deliverProjectApproval(ctx, b, pull, { verdict: "declined", sha: fields.headSha ?? "", entry: fields.entry, reasoning: fields.reasoning }); +} + /** * Whether a pull request's author is someone other than the server's gh * login: a teammate's, where the owner can give a real GitHub review rather diff --git a/plugins/stewards/server.ts b/plugins/stewards/server.ts index 09d9eba4..5ad3d220 100644 --- a/plugins/stewards/server.ts +++ b/plugins/stewards/server.ts @@ -86,7 +86,7 @@ import { type Role as StewardRole, type RolesFile, } from "./roles.js"; -import { decideFor, decideForPull, decideForRefusal, explainDecision, formatDecisionListing, globMatches, type BigDecision } from "./decisions.js"; +import { explainDecision, formatDecisionListing, globMatches, type BigDecision } from "./decisions.js"; import { ADOPTION_CHOICES, adoptionChoices, adoptionHow, adoptionQuestion, alertClockAt, alertFingerprint, batchAlert, deadlineAt, depsLookSchema, dueDate, isBatchable, overdue, priorityFor, priorityLabel, priorityLabelForSeverity, reminderText, repoOfFingerprint, sightingOf, snoozeUntil, type DepsFinding, type DepsLook, type DepsTicket } from "./deps.js"; import { hostContract } from "./host-contract.js"; import { MULTI_REPO_ENVIRONMENT_PROVIDER_ID, MULTI_REPO_PLUGIN_ID, multiRepoEnvironmentFlags, multiRepoFreshEnvironment, multiRepoSource, nativeSource, projectSourceLabel, sourceRpcMissing, type FreshEnvironment, type NativeGitRead, type ProjectSource } from "./project-source.js"; @@ -96,8 +96,8 @@ import { nightlyDue, stillNight } from "./nightly.js"; import { classify, hasFingerprintLine, lookChanges, opsTargetKey, parseOpsTargetKey, ticketResolved, type DependabotTarget, type FingerprintRow, type K8sTarget, type OpsLook, type OpsTarget, type SentryTarget, type Sighting, type TicketState } from "./ops.js"; import { EVENTS_PER_LOOK, NO_TOKEN, RECENT_PER_LOOK, SENTRY_API_URL, TOKEN_SCOPES, createSentryReader, emptySentryLook, issueInOrg, recentLine, sentryChanges, sentryResolveTargets, sentrySightings, sentrySummary, type SentryFinding, type SentryLook, type SentryReader } from "./sentry.js"; import { advanced, answerDiffers, anyWorkerInFlight, approvalWanted, asksForOpenApproval, classifyApproval, commentDiffersFromRecommendation, compactDue, deploymentSettled, diffObserved, endingDeadline, followingThrough, gateKnownUnmet, gateState, hasLabel, IDLE_WAKE_TEXT, idleInActiveStage, isCommentNews, isOwnStageChange, isSpecComment, latestReviewMarker, lifecycle, mechanicalStatus, namesUserAsBlocker, newHumanComments, newestCommentAt, openUserFacingItems, ownChecksFinished, ownPullClosed, ownPullMerged, phaseAndStage, productGateOptions, resolveChecklistItem, runningWorkerReason, newestEditComment, specDiff, specHash, specText, stuckDue, unheardWait, workerInFlight, workerReportedTerminal, type Change, type MechanicalEvent, type OpenUserItem } from "./reconcile.js"; -import { ESCALATED_APPROVAL_REFUSAL, latestProjectApprovalMarker, prGate, PRODUCT_APPROVAL_REFUSAL } from "./gate.js"; -import { approvePrPageAction, deliverProjectApproval, fileForwardedApprovePrNeed, markerKey, recordProjectDecision, syncPullApproval, type ApprovalsContext } from "./approvals.js"; +import { latestProjectApprovalMarker, prGate } from "./gate.js"; +import { approveOwnerReviewedPull, approvePrPageAction, declineOwnerReviewedPull, fileForwardedApprovePrNeed, isProjectSteward, markerKey, placeReportedPull, specApprovedByProject, syncPullApproval, type ApprovalsContext } from "./approvals.js"; import { deliverEscalation, nextRoute, routedTo, type EscalationsContext } from "./escalations.js"; import { briefGate, syncApprovalRequest, type ReadyGateContext } from "./ready-gate.js"; import { @@ -6550,6 +6550,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P wakeProject, pullChangedFiles, loadDecisions: async (projectId) => (await loadConfig(projectId))?.file.bigDecisions ?? [], + loadDecisionsOrNull: async (projectId) => (await loadConfig(projectId))?.file.bigDecisions ?? null, observedPulls, mechanicalStatus: patchMechanicalStatus, }; @@ -8829,11 +8830,9 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P return b; } - const isProjectSteward = (threadId: string, b: Binding) => store.project(b.projectId).threadId === threadId; - /** Whether `threadId` may steer `target`: its own steward, a steward above it, or its project steward. */ function mayDirect(threadId: string, target: Binding): boolean { - if (target.threadId === threadId || isProjectSteward(threadId, target)) return true; + if (target.threadId === threadId || isProjectSteward(approvalsCtx, threadId, target)) return true; let up = target.parentBindingId === null ? null : store.binding(target.parentBindingId); for (let depth = 0; up !== null && depth < 10; depth++) { if (up.threadId === threadId) return true; @@ -9439,7 +9438,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const pull = b.lastSnapshot === null ? undefined : observedPulls(b, b.lastSnapshot).find((p) => [p.url, p.key, p.id].some((v) => v !== null && lc(v) === wanted)); // The decide list places the pull request, so a steward cannot // file the user's item for the project steward's lane (BB-51). - const decision = pull === undefined ? "user" : decideForPull(await pullChangedFiles(pull), config?.file.bigDecisions ?? []); + const decision = pull === undefined ? "user" : await placeReportedPull(approvalsCtx, pull, config?.file.bigDecisions ?? []); const gate = pull === undefined ? null : prGate(pull, { decision, projectApprovals: projectApprovalsFor(b), ...productGateOptions(b, pull) }); const holdable = gate !== null && (n.kind === "approve-pr" ? gate.needsOwnerApproval : gate.gateMet); if (!holdable) { @@ -9935,36 +9934,9 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // missing, unknown, `user`, or the change widens the list itself. const prApproval = store.openEscalations(target.id).find((x) => x.kind === "approval" && x.resolutionKind === "approve-decline") ?? null; if (input.approve === true && prApproval !== null && input.entry !== undefined) { - if (!isProjectSteward(threadId, target)) return toolError(new Error("An owner-reviewed pull request is the project steward's to approve; a steward escalates it and waits.")); - const config = await loadConfig(target.projectId); - if (config === null) return toolError(new Error("The project file could not be read, so no project-steward approval can be recorded.")); - const observed = target.lastSnapshot; - const pull = observed === null ? null : [observed.entity, ...observed.pulls].find((e) => (e.url ?? e.key) === prApproval.url) ?? null; - // A reviewer's owner call in the marker is binding: the project - // steward does not decide it, whatever entry the decide list - // matched, and never posts a `bb-approval` on it (BB-192). The - // product reason (BB-207) binds it the same way. - const approvalGate = pull === null ? null : prGate(pull, { projectApprovals: projectApprovalsFor(target), ...productGateOptions(target, pull) }); - if (approvalGate !== null && approvalGate.escalated) { - return toolError(new Error(approvalGate.reason === "product" ? PRODUCT_APPROVAL_REFUSAL : ESCALATED_APPROVAL_REFUSAL)); - } - const files = pull === null ? null : await pullChangedFiles(pull); - const decision = decideFor(files, input.entry ?? null, config.file.bigDecisions); - if (decision !== "project") { - const why = decideForRefusal(files, input.entry ?? null, config.file.bigDecisions) ?? "the change is not the project steward's to approve"; - return toolError(new Error(`That pull request is the user's to approve: ${why}. Forward it with steward_update { binding: "${target.entityKey}", forward: "${prApproval.id}", resolution: "approve-decline", recommendation } instead.`)); - } - const answer = guidance ?? `Approved as ${JSON.stringify(input.entry)}.`; - if (pull === null) return toolError(new Error("The pull request could not be read, so the approval marker cannot be posted.")); - try { - await recordProjectDecision(approvalsCtx, target, pull, threadId, { headSha: prApproval.headSha ?? "", entry: input.entry ?? null, verdict: "approved", reasoning: answer }); - } catch (error) { - return toolError(error); - } - store.addGuidance(target.id, answer, threadId); - store.answerEscalation(prApproval.id, answer, threadId); - const reached = await deliverProjectApproval(approvalsCtx, target, pull, { verdict: "approved", sha: prApproval.headSha ?? "", entry: input.entry ?? null, reasoning: answer }); - done.push(`posted the project steward's approval marker on ${prApproval.url} at ${shortSha(prApproval.headSha)} and ${reached === "worker" ? "told its worker" : "woke the steward"}`); + const result = await approveOwnerReviewedPull(approvalsCtx, target, prApproval, threadId, { entry: input.entry, guidance }); + if (!result.ok) return toolError(new Error(result.error)); + done.push(result.done); return `${target.entityKey}: ${done.join("; ")}.`; } if (input.approve === true) { @@ -9975,7 +9947,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P if (typeof mode === "object") return toolError(new Error(`${target.entityKey} is approved by moving it to ${mode.state} in ${target.source === "github" ? "GitHub" : "Linear"}.`)); if (target.lastSnapshot === null) return toolError(new Error("Nothing has been observed for this binding yet.")); const entity = (target.source === "ops" ? null : await snapshot(target.source, target.entityId).catch(() => null)) ?? target.lastSnapshot.entity; - const byProject = mode === "project" && isProjectSteward(threadId, target); + const byProject = specApprovedByProject(approvalsCtx, threadId, target, mode); // Criterion 3: the user's words may be an unsigned comment on the entity itself, not only a message in this thread. const byUser = input.quote !== undefined && ((await userSaid(threadId, input.quote)) || userApprovedOnEntity(entity, target.threadId, input.quote)); if (!byProject && !byUser) { @@ -10011,7 +9983,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P if (!covered && approval === null && resolving.kind === "approval" && resolving.resolutionKind !== "approve-decline" && resolving.status === "open" && target.lastSnapshot !== null && classifyApproval(guidance) === "approve") { const config = await loadConfig(target.projectId); const playbook = config === null ? null : bindingPlaybook(config, target); - if (playbook?.ready?.approval === "project" && isProjectSteward(threadId, target)) approval = { entity: (target.source === "ops" ? null : await snapshot(target.source, target.entityId).catch(() => null)) ?? target.lastSnapshot.entity, byUser: false }; + if (playbook?.ready?.approval === "project" && isProjectSteward(approvalsCtx, threadId, target)) approval = { entity: (target.source === "ops" ? null : await snapshot(target.source, target.entityId).catch(() => null)) ?? target.lastSnapshot.entity, byUser: false }; } } if (input.stewardModel !== undefined && target.threadId !== null) { @@ -10224,13 +10196,11 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const declinePull = isDecline && target.lastSnapshot !== null ? observedPulls(target, target.lastSnapshot).find((p) => (p.url ?? p.key) === resolving.url) ?? null : null; const declineSha = resolving.headSha ?? declinePull?.pull?.headSha ?? null; if (isDecline && declinePull !== null) { - const entry = input.entry ?? null; try { - await recordProjectDecision(approvalsCtx, target, declinePull, threadId, { headSha: declineSha ?? "", entry, verdict: "declined", reasoning: guidance ?? "" }); + await declineOwnerReviewedPull(approvalsCtx, target, declinePull, threadId, { headSha: declineSha, entry: input.entry ?? null, reasoning: guidance ?? "" }); } catch (error) { return toolError(error); } - await deliverProjectApproval(approvalsCtx, target, declinePull, { verdict: "declined", sha: declineSha ?? "", entry, reasoning: guidance ?? "" }); } // Already resolved (the binding ended, a retry) is no error: the answer is added to the record. const outcome = store.answerEscalation(resolving.id, guidance!, threadId); @@ -10265,7 +10235,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P done.push(`recorded the decline's ${objections.length} objection(s) for the steward's checklist`); } } - if (resolving.status === "open") done.push(`resolved ${resolving.id}${isProjectSteward(threadId, target) ? "; if this is a general judgement the playbook should carry, record it with steward_update { lesson: { text, file, scope } }" : ""}`); + if (resolving.status === "open") done.push(`resolved ${resolving.id}${isProjectSteward(approvalsCtx, threadId, target) ? "; if this is a general judgement the playbook should carry, record it with steward_update { lesson: { text, file, scope } }" : ""}`); else if (outcome === "appended") done.push(`recorded the answer on ${resolving.id}, which was already resolved (${resolving.resolution ?? "no answer"})`); else done.push(`${resolving.id} already has this answer`); if (outcome !== "unchanged") notes.push(`Escalation ${resolving.id} decided: ${guidance}`); @@ -10281,7 +10251,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const stackNote = stackKeys.length === 0 ? "" : ` (stacked on ${stackKeys.join(", ")}: lifts itself once ${stackKeys.length === 1 ? "its pull request opens" : "their pull requests open"})`; const what = [input.priority === undefined ? null : `priority ${input.priority}`, input.hold === undefined ? null : input.hold === "" ? "hold lifted" : `on hold: ${input.hold}${behindNote}${stackNote}`].filter((s) => s !== null).join("; "); done.push(what); - notes.push(`Set by your ${isProjectSteward(threadId, target) ? "project" : "parent"} steward: ${what}`); + notes.push(`Set by your ${isProjectSteward(approvalsCtx, threadId, target) ? "project" : "parent"} steward: ${what}`); // A hold answers an open scheduling request. A hold lifted by hand closes it too; the next look asks again if it must. if (input.hold !== undefined) { for (const x of store.openEscalations(target.id).filter((e) => e.kind === "schedule")) store.resolve(x.id, input.hold === "" ? "Hold lifted." : stackKeys.length > 0 ? `Stacked on ${stackKeys.join(", ")}.` : behindKeys.length === 0 ? `Held: ${input.hold}` : `Held behind ${behindKeys.join(", ")}.`); From a3676eb066617fd1120acf50fa13bdf7cdff754a Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Tue, 10 Nov 2026 04:59:16 +0000 Subject: [PATCH 5/7] stewards: route both spec acceptance paths through specApprovedByProject (BB-280) --- plugins/stewards/approvals.ts | 6 ++++-- plugins/stewards/server.ts | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/plugins/stewards/approvals.ts b/plugins/stewards/approvals.ts index 17b636c9..29c1ca85 100644 --- a/plugins/stewards/approvals.ts +++ b/plugins/stewards/approvals.ts @@ -8,6 +8,7 @@ // The functions take an `ApprovalsContext` rather than the server's whole // closure, so nothing here reads the plugin's config or snapshots on its own. import { type BigDecision, decideFor, decideForPull, decideForRefusal, explainDecision, formatDecisionListing, type DecideMark } from "./decisions.js"; +import type { ApprovalMode } from "./config.js"; import { approvalMarkerLine, ESCALATED_APPROVAL_REFUSAL, prGate, PRODUCT_APPROVAL_REFUSAL, reviewWorkerThread, type PrGate } from "./gate.js"; import { parsePullRef, type GithubWriter } from "./github.js"; import { productGateOptions, productReason, workerInFlight, type MechanicalEvent } from "./reconcile.js"; @@ -129,9 +130,10 @@ export const isProjectSteward = (ctx: ApprovalsContext, threadId: string, b: Bin * Whether the project steward's thread may approve `b`'s spec under the * playbook's `mode`: `project` and the project steward's own thread. `user` is * the user's, recorded by quoting their words (BB-280; this acceptance was in - * server.ts before). + * server.ts before). The raw `ready.approval` value is accepted so both spec + * acceptance paths route through here. */ -export const specApprovedByProject = (ctx: ApprovalsContext, threadId: string, b: Binding, mode: "project" | "user"): boolean => mode === "project" && isProjectSteward(ctx, threadId, b); +export const specApprovedByProject = (ctx: ApprovalsContext, threadId: string, b: Binding, mode: ApprovalMode | null | undefined): boolean => mode === "project" && isProjectSteward(ctx, threadId, b); /** * The decide-list placement of a pull request a steward is reporting: diff --git a/plugins/stewards/server.ts b/plugins/stewards/server.ts index 5ad3d220..6d75e581 100644 --- a/plugins/stewards/server.ts +++ b/plugins/stewards/server.ts @@ -9983,7 +9983,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P if (!covered && approval === null && resolving.kind === "approval" && resolving.resolutionKind !== "approve-decline" && resolving.status === "open" && target.lastSnapshot !== null && classifyApproval(guidance) === "approve") { const config = await loadConfig(target.projectId); const playbook = config === null ? null : bindingPlaybook(config, target); - if (playbook?.ready?.approval === "project" && isProjectSteward(approvalsCtx, threadId, target)) approval = { entity: (target.source === "ops" ? null : await snapshot(target.source, target.entityId).catch(() => null)) ?? target.lastSnapshot.entity, byUser: false }; + if (specApprovedByProject(approvalsCtx, threadId, target, playbook?.ready?.approval)) approval = { entity: (target.source === "ops" ? null : await snapshot(target.source, target.entityId).catch(() => null)) ?? target.lastSnapshot.entity, byUser: false }; } } if (input.stewardModel !== undefined && target.threadId !== null) { From b8509f1a7fd6dfe8b6e88f0ed5548fb0a2e7e44e Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Tue, 10 Nov 2026 05:12:19 +0000 Subject: [PATCH 6/7] stewards: move the marker, approval words and ready-gate state into their modules (BB-280) --- plugins/stewards/approvals.test.ts | 7 +- plugins/stewards/approvals.ts | 40 ++++++- plugins/stewards/gate.ts | 120 +++++++++++++++++++- plugins/stewards/ready-gate.ts | 65 ++++++++++- plugins/stewards/reconcile.test.ts | 28 ++++- plugins/stewards/reconcile.ts | 169 +---------------------------- plugins/stewards/server.ts | 74 +++++-------- plugins/stewards/wake.test.ts | 2 +- plugins/stewards/wake.ts | 3 +- 9 files changed, 279 insertions(+), 229 deletions(-) diff --git a/plugins/stewards/approvals.test.ts b/plugins/stewards/approvals.test.ts index 7bec83b2..941cd3c2 100644 --- a/plugins/stewards/approvals.test.ts +++ b/plugins/stewards/approvals.test.ts @@ -6,7 +6,7 @@ // BB-279: an owner's page approval and the project steward's decision deliver // to a parked worker by the same path (`deliverProjectApproval`). import { describe, expect, it, vi } from "vitest"; -import { approvePrPageAction, deliverProjectApproval, recordProjectDecision, type ApprovalsContext } from "./approvals.js"; +import { approvePrPageAction, deliverProjectApproval, recordProjectDecision, userApprovedOnEntity, userSaid, type ApprovalsContext } from "./approvals.js"; import { isSelfWidening } from "./decisions.js"; import { comment, parkedWorker, pull } from "./fixtures.js"; import type { Binding, Need } from "./store.js"; @@ -153,4 +153,9 @@ describe("the approval-recording files are self-widening (BB-280)", () => { expect(isSelfWidening([file]), file).toBe(true); } }); + + it("keeps the user's approval detection in approvals.ts", () => { + expect(typeof userSaid).toBe("function"); + expect(typeof userApprovedOnEntity).toBe("function"); + }); }); diff --git a/plugins/stewards/approvals.ts b/plugins/stewards/approvals.ts index 29c1ca85..c9a8b765 100644 --- a/plugins/stewards/approvals.ts +++ b/plugins/stewards/approvals.ts @@ -9,9 +9,9 @@ // closure, so nothing here reads the plugin's config or snapshots on its own. import { type BigDecision, decideFor, decideForPull, decideForRefusal, explainDecision, formatDecisionListing, type DecideMark } from "./decisions.js"; import type { ApprovalMode } from "./config.js"; -import { approvalMarkerLine, ESCALATED_APPROVAL_REFUSAL, prGate, PRODUCT_APPROVAL_REFUSAL, reviewWorkerThread, type PrGate } from "./gate.js"; +import { approvalMarkerLine, classifyApproval, ESCALATED_APPROVAL_REFUSAL, prGate, productReason, PRODUCT_APPROVAL_REFUSAL, reviewWorkerThread, type PrGate } from "./gate.js"; import { parsePullRef, type GithubWriter } from "./github.js"; -import { productGateOptions, productReason, workerInFlight, type MechanicalEvent } from "./reconcile.js"; +import { isSpecComment, productGateOptions, workerInFlight, type MechanicalEvent } from "./reconcile.js"; import { signatureLine, type EntitySnapshot, type Source } from "./source-contract.js"; import type { Binding, Escalation, Need, Observed, Store } from "./store.js"; @@ -34,6 +34,10 @@ export type ApprovalsContext = { sendTo: (threadId: string, text: string) => Promise<"sent" | "gone" | "failed" | "unavailable">; wakeProject: (projectId: string, reason: string, opts: { digestDue: boolean; notes?: string[] }) => Promise; pullChangedFiles: (entity: EntitySnapshot) => Promise; + /** The user's own recent turn texts in a thread, normalized, newest first, excluding the plugin's own wakes (BB-178). */ + recentUserTurnTexts: (threadId: string) => Promise; + /** Whether a thread is the user's own rather than a steward's, a worker's or the project steward's. */ + isUserThread: (threadId: string) => boolean; loadDecisions: (projectId: string) => Promise; /** The project's decide list, or null when the project file could not be read (BB-280). */ loadDecisionsOrNull: (projectId: string) => Promise; @@ -135,6 +139,38 @@ export const isProjectSteward = (ctx: ApprovalsContext, threadId: string, b: Bin */ export const specApprovedByProject = (ctx: ApprovalsContext, threadId: string, b: Binding, mode: ApprovalMode | null | undefined): boolean => mode === "project" && isProjectSteward(ctx, threadId, b); +/** + * Whether the user said these words in this thread lately, in their own turn + * rather than the plugin's own wake (BB-178). The context supplies the user's + * own recent turns, already filtered to the user and the lookback and + * normalized. The path was in server.ts before (BB-280). + */ +export async function userSaid(ctx: ApprovalsContext, threadId: string, quote: string): Promise { + const want = quote.replace(/\s+/gu, " ").trim().toLowerCase(); + return (await ctx.recentUserTurnTexts(threadId)).some((text) => text.includes(want)); +} + +/** + * Whether the connected user approved in an unsigned comment on the entity, + * after the latest spec comment (criterion 3). `quote` is matched against the + * comment's text, so the steward can quote it back. The path was in server.ts + * before (BB-280). + */ +export function userApprovedOnEntity(ctx: ApprovalsContext, entity: EntitySnapshot | null, stewardThreadId: string | null, quote: string): boolean { + if (entity === null || entity.viewer === null) return false; + const viewer = entity.viewer.toLowerCase(); + const latestSpec = entity.comments.filter((c) => isSpecComment(c, stewardThreadId, ctx.isUserThread)).reduce((max, c) => (c.createdAt > max ? c.createdAt : max), ""); + const want = quote.replace(/\s+/gu, " ").trim().toLowerCase(); + return entity.comments.some( + (c) => + c.signedBy === null && + c.author.toLowerCase() === viewer && + c.createdAt > latestSpec && + classifyApproval(c.body) === "approve" && + c.body.replace(/\s+/gu, " ").toLowerCase().includes(want), + ); +} + /** * The decide-list placement of a pull request a steward is reporting: * `decideForPull` on its changed files, so a steward cannot file the user's diff --git a/plugins/stewards/gate.ts b/plugins/stewards/gate.ts index 583abef5..03e2fe0d 100644 --- a/plugins/stewards/gate.ts +++ b/plugins/stewards/gate.ts @@ -3,12 +3,124 @@ // `bb-approval` marker and its plugin-recorded comment id, all read from one // snapshot. Pure, and kept in its own module because this is the code a change // may not widen by itself: a pull request that touches this file is always the -// owner's, whatever the decide list says (BB-153). It reads the reviewer marker -// and the approval words from reconcile.ts; reconcile.ts reads `isMarkerComment` -// back, the one cycle, which ESM function hoisting makes safe. -import { classifyApproval, latestReviewMarker, productReason, reviewMarker, type ReviewMarker } from "./reconcile.js"; +// owner's, whatever the decide list says (BB-153). It owns the reviewer's +// marker and the approval words, so it reads nothing back from reconcile.ts +// (BB-280). import type { EntitySnapshot, SnapshotComment } from "./source-contract.js"; +// ---- the reviewer's marker and the approval words ----------------------------------- + +/** + * A reviewer's machine-readable verdict, as `pr-reviewer` posts it on the + * first line of a comment. `lane` is null when the marker did not name one + * (the gate then treats the pull request as the owner's). `approver` is the + * optional `approver=owner|project` attribute; it is undefined when the marker + * did not name one, and the gate then reads it as `owner` (fail closed, + * BB-192). `substantial` is the reviewer's diff judgment (BB-207), null when + * the marker did not name one; `ticketApprovedSpec` is whether a ticket the + * pull request references has an approved spec, the ack that spares the owner + * the product ask, false when the marker did not say so (fail closed). + */ +export type ReviewMarker = { round: number; sha: string; verdict: string; lane: string | null; approver?: string; substantial: boolean | null; ticketApprovedSpec: boolean }; + +const REVIEW_MARKER = /^/u; + +/** The verdict a marker without `verdict=` is read as: never `approved`. */ +export const UNREADABLE_VERDICT = "unreadable"; +/** + * The reviewer's marker on a comment, or null for any other comment. Any + * first line that opens a `bb-review` comment is a marker, however it is + * spelled: a round whose keys are missing or misspelled still closes the + * gate, because the alternative is to read the previous round's `approved` + * as the latest word. A missing `verdict=` is read from a bare word in the + * marker (`changes-requested`) and otherwise is `unreadable`; neither is + * `approved`. + */ +export function reviewMarker(body: string): ReviewMarker | null { + const first = body.split("\n", 1)[0]!.trim(); + const m = REVIEW_MARKER.exec(first); + if (m === null) return null; + const keyed = new Map(); + const bare: string[] = []; + for (const token of m[1]!.trim().split(/\s+/u).filter((t) => t !== "")) { + const eq = token.indexOf("="); + if (eq > 0) keyed.set(token.slice(0, eq).toLowerCase(), token.slice(eq + 1)); + else bare.push(token); + } + const round = Number(keyed.get("round") ?? ""); + // BB-207: `substantial=yes|no` is the reviewer's diff judgment, absent is + // unjudged; `spec=yes` says a referenced ticket has an approved spec. + const substantial = keyed.get("substantial"); + return { + round: Number.isInteger(round) && round > 0 ? round : 0, + sha: keyed.get("sha") ?? "", + verdict: keyed.get("verdict") ?? bare[0] ?? UNREADABLE_VERDICT, + lane: keyed.get("lane") ?? null, + approver: keyed.get("approver"), + substantial: substantial === "yes" ? true : substantial === "no" ? false : null, + ticketApprovedSpec: keyed.get("spec") === "yes", + }; +} + +/** The newest reviewer marker in a comment list, by comment time. */ +export function latestReviewMarker(comments: readonly SnapshotComment[]): ReviewMarker | null { + let at = ""; + let latest: ReviewMarker | null = null; + for (const c of comments) { + const m = reviewMarker(c.body); + if (m !== null && c.createdAt >= at) { + at = c.createdAt; + latest = m; + } + } + return latest; +} + +/** + * The product reason a pull request's owner lane carries (BB-207): the review + * binding was judged `substantial` from the diff, and no ticket it references + * has an approved spec. `substantial` is the binding's ready-gate judgment; + * null (unjudged) and false leave every lane exactly as today, which is what + * keeps the change inert until a reviewer reports a verdict. A referenced + * ticket with an approved spec is the existing ack, so the owner is not asked + * again. The reason is a property of the pull request, never of a commit: a + * later push only re-asks when it changed scope or behaviour. + */ +export function productReason(substantial: boolean | null, ticketApprovedSpec: boolean): "product" | null { + return substantial === true && !ticketApprovedSpec ? "product" : null; +} + +// ---- approval words ------------------------------------------------------------------- + +/** + * How a piece of text answers an approval request. Explicit approval words + * approve; explicit refusal words decline; anything else is ambiguous and is + * the project steward's to interpret. The page answer uses all three; a + * project steward's own guidance + resolve is a decision, so anything that + * does not approve declines (as before). + */ +export type ApprovalVerdict = "approve" | "decline" | "ambiguous"; + +const NEGATED_APPROVE = /(?:^|[^\p{L}\p{N}])(?:not|never|cannot|do\s+not|[a-z]+n['’]t)\s+(?:\w+\s+){0,2}?(?:approv\w*|ship|lgtm|go\s+ahead|looks?\s+good|sign\s*off|yes)(?:[^\p{L}\p{N}]|$)/iu; +const REFUSAL = /(?:^|[^\p{L}\p{N}])(?:reject(?:ed)?|declin(?:e|ed)|hold\s+off)(?:[^\p{L}\p{N}]|$)/iu; +const APPROVE_WORDS = /(?:^|[^\p{L}\p{N}])(?:lgtm|approv(?:e|es|ed|al)|ship\s*it|go\s*ahead|looks?\s+good|sign(?:ed)?\s*off|:shipit:|yes|yep|yeah)(?:[^\p{L}\p{N}]|$)/iu; +const DECLINE_WORDS = /(?:^|[^\p{L}\p{N}])(?:no|nope|don'?t|do\s+not|not\s+yet|changes?\s+(?:are\s+)?(?:needed|requested)|needs?\s+changes?)(?:[^\p{L}\p{N}]|$)/iu; + +/** + * A clear refusal first, so "not approved" and "don't ship it" decline rather + * than matching the approval word inside them; then approval words, so + * "LGTM, no concerns" and "ship it, no further changes needed" approve; then + * a bare no or a request for changes declines; anything else is ambiguous. + */ +export function classifyApproval(text: string | null | undefined): ApprovalVerdict { + const flat = (text ?? "").replace(/\s+/gu, " ").trim(); + if (flat === "") return "ambiguous"; + if (NEGATED_APPROVE.test(flat) || REFUSAL.test(flat)) return "decline"; + if (APPROVE_WORDS.test(flat)) return "approve"; + if (DECLINE_WORDS.test(flat)) return "decline"; + return "ambiguous"; +} + // ---- the merge gate ------------------------------------------------------------------- /** diff --git a/plugins/stewards/ready-gate.ts b/plugins/stewards/ready-gate.ts index 53877daf..e928d698 100644 --- a/plugins/stewards/ready-gate.ts +++ b/plugins/stewards/ready-gate.ts @@ -4,11 +4,69 @@ // // The functions take a `ReadyGateContext` rather than the server's whole // closure, so nothing here reads the plugin's config or snapshots on its own. -import { approvalWanted, gateState, specDiff } from "./reconcile.js"; +import type { SpecDiff } from "./reconcile.js"; import type { EntitySnapshot } from "./source-contract.js"; -import type { Binding, Escalation, Store } from "./store.js"; +import type { Binding, Escalation, ReadyState, Store } from "./store.js"; import type { Playbook } from "./config.js"; +// ---- the ready gate ------------------------------------------------------------------ + +export type GateState = { + /** No ready gate in the playbook: nothing to pass. */ + none: boolean; + checklistMet: boolean; + unmet: string[]; + /** Null until the steward has judged it. */ + substantial: boolean | null; + approvalNeeded: boolean; + approved: boolean; + /** An approval exists but the spec has changed since. */ + approvalStale: boolean; + passed: boolean; +}; + +/** Checklist wording as compared: case, runs of spaces and trailing punctuation do not count. */ +const checklistKey = (s: string) => s.toLowerCase().replace(/\s+/gu, " ").replace(/[\s.:;,]+$/u, "").trim(); + +/** What the steward last reported for one of the playbook's checklist items. */ +export const reportedItem = (ready: ReadyState, item: string): ReadyState["checklist"][number] | undefined => + ready.checklist.find((c) => checklistKey(c.item) === checklistKey(item)); + +/** + * The playbook's own wording for a reported checklist entry: by its number + * in the wake (1-based), or by its text as the playbook writes it. Null for + * anything else. Stewards paraphrase long items, so a paraphrase is refused + * rather than guessed at: a guess that misses leaves the gate shut in + * silence, which is how it never passed. + */ +export function resolveChecklistItem(items: readonly string[], reported: string | number): string | null { + const text = String(reported); + const number = /^\s*#?(\d+)\.?\s*$/u.exec(text); + if (number !== null) return items[Number(number[1]) - 1] ?? null; + return items.find((item) => checklistKey(item) === checklistKey(text)) ?? null; +} + +export function gateState(playbook: Playbook, ready: ReadyState, currentSpecHash: string): GateState { + if (playbook.ready === null) return { none: true, checklistMet: true, unmet: [], substantial: null, approvalNeeded: false, approved: false, approvalStale: false, passed: true }; + const items = playbook.ready.checklist; + const unmet = items.filter((item) => reportedItem(ready, item)?.met !== true); + const checklistMet = unmet.length === 0; + // Unjudged counts as substantial: the safe side of the gate. + const approvalNeeded = ready.substantial !== false; + const approved = ready.approval !== null; + const approvalStale = approved && ready.approval!.specHash !== currentSpecHash; + const passed = checklistMet && (!approvalNeeded || (approved && !approvalStale)); + return { none: false, checklistMet, unmet, substantial: ready.substantial, approvalNeeded, approved, approvalStale, passed }; +} + +/** + * Whether the gate waits on its approver: the checklist is met, the ticket + * is (or may be) substantial, and the current spec has no approval. The + * plugin asks the approver itself then, so the gate does not depend on the + * steward remembering to. + */ +export const approvalWanted = (gate: GateState): boolean => !gate.none && gate.checklistMet && gate.approvalNeeded && !gate.passed; + /** * What the ready gate needs from the server: the store, the spec hash and text * a binding's approval is pinned to (they read the store for the user's own @@ -18,6 +76,7 @@ export type ReadyGateContext = { store: Store; specHashFor: (b: Pick, entity: EntitySnapshot) => string; specTextFor: (b: Pick, entity: EntitySnapshot) => string; + specDiff: (before: string, after: string, context?: number) => SpecDiff; log: (message: string) => void; }; @@ -69,7 +128,7 @@ export async function syncApprovalRequest(ctx: ReadyGateContext, b: Binding, pla // item does, so the project steward reads a change rather than a document. const prior = b.ready.approval; const approvedText = prior?.approvedText ?? b.ready.voidedApprovedText; - const reapproval = approvedText !== null ? specDiff(approvedText, ctx.specTextFor(b, b.lastSnapshot.entity)) : null; + const reapproval = approvedText !== null ? ctx.specDiff(approvedText, ctx.specTextFor(b, b.lastSnapshot.entity)) : null; const x = ctx.store.escalate({ bindingId: b.id, kind: "approval", diff --git a/plugins/stewards/reconcile.test.ts b/plugins/stewards/reconcile.test.ts index 196134b1..2aa096a0 100644 --- a/plugins/stewards/reconcile.test.ts +++ b/plugins/stewards/reconcile.test.ts @@ -1,10 +1,34 @@ import { describe, expect, it } from "vitest"; import { parsePlaybook, phaseOf } from "./config.js"; import { comment, issue, pull } from "./fixtures.js"; -import { approvalWanted, answerDiffers, anyWorkerInFlight, asksForOpenApproval, checksPhrase, classifyApproval, commentDiffersFromRecommendation, compactDue, deploymentSettled, diffObserved, endingDeadline, followingThrough, gateKnownUnmet, gateState, idleInActiveStage, isCommentNews, isOwnStageChange, isOwnStageMove, lifecycle, markerLaneChange, mechanicalStatus, namesUserAsBlocker, newestEditComment, ownChecksFinished, ownPullClosed, ownPullMerged, phaseAndStage, productGateOptions, productReason, resolveChecklistItem, reviewMarker, specDiff, specHash, specText, stuckDue, UNREADABLE_VERDICT, unheardWait, workerInFlight, workerReportedTerminal, type IdleBinding } from "./reconcile.js"; -import { approvalMarkerLine, ownerApproval, prGate, projectApprovalMarker, reviewWorkerThread, type ProjectApprovalRecord } from "./gate.js"; +import { answerDiffers, anyWorkerInFlight, asksForOpenApproval, checksPhrase, commentDiffersFromRecommendation, compactDue, deploymentSettled, diffObserved, endingDeadline, followingThrough, gateKnownUnmet, idleInActiveStage, isCommentNews, isOwnStageChange, isOwnStageMove, lifecycle, markerLaneChange, mechanicalStatus, namesUserAsBlocker, newestEditComment, ownChecksFinished, ownPullClosed, ownPullMerged, phaseAndStage, productGateOptions, specDiff, specHash, specText, stuckDue, unheardWait, workerInFlight, workerReportedTerminal, type IdleBinding } from "./reconcile.js"; +import { approvalMarkerLine, classifyApproval, ownerApproval, prGate, productReason, projectApprovalMarker, reviewMarker, reviewWorkerThread, UNREADABLE_VERDICT, type ProjectApprovalRecord } from "./gate.js"; +import { approvalWanted, gateState, resolveChecklistItem } from "./ready-gate.js"; import { readyStateSchema, statusRecordSchema } from "./store.js"; import { GITHUB_ISSUES, GITHUB_REVIEWS, LINEAR_TICKETS } from "./templates.js"; +import * as gateModule from "./gate.js"; +import * as readyGateModule from "./ready-gate.js"; +import * as reconcileModule from "./reconcile.js"; + +// BB-280: each function moved out of reconcile.ts is pinned to its new module. +// A change that moves one back (or drops the export) fails these, so the +// owner-reviewed gate.ts and ready-gate.ts keep the code the decide list's +// self-widening and narrowed entries name. +describe("the moved functions stay in their modules (BB-280)", () => { + it("keeps the reviewer marker and approval words in gate.ts, not reconcile.ts", () => { + for (const name of ["reviewMarker", "latestReviewMarker", "classifyApproval", "productReason"]) { + expect(typeof (gateModule as Record)[name], name).toBe("function"); + expect((reconcileModule as Record)[name], name).toBeUndefined(); + } + }); + + it("keeps the ready gate in ready-gate.ts, not reconcile.ts", () => { + for (const name of ["gateState", "approvalWanted", "reportedItem", "resolveChecklistItem"]) { + expect(typeof (readyGateModule as Record)[name], name).toBe("function"); + expect((reconcileModule as Record)[name], name).toBeUndefined(); + } + }); +}); const tickets = (() => { const p = parsePlaybook(LINEAR_TICKETS, "linear-tickets"); diff --git a/plugins/stewards/reconcile.ts b/plugins/stewards/reconcile.ts index 68c7b10f..3f4054fd 100644 --- a/plugins/stewards/reconcile.ts +++ b/plugins/stewards/reconcile.ts @@ -8,7 +8,8 @@ import { createHash } from "node:crypto"; import { isTerminal, labelPhaseName, phaseOf, type Playbook } from "./config.js"; import type { Deployments } from "./deploy.js"; -import { isMarkerComment } from "./gate.js"; +import { isMarkerComment, latestReviewMarker, reviewMarker } from "./gate.js"; +import { gateState, reportedItem } from "./ready-gate.js"; import type { EntitySnapshot, SnapshotComment } from "./source-contract.js"; import type { Binding, Escalation, Need, Observed, ReadyState, StatusRecord, Worker } from "./store.js"; @@ -47,72 +48,7 @@ export function runningWorkerReason(workers: readonly Pick]*?)-->/u; -/** The verdict a marker without `verdict=` is read as: never `approved`. */ -export const UNREADABLE_VERDICT = "unreadable"; - -/** - * The reviewer's marker on a comment, or null for any other comment. Any - * first line that opens a `bb-review` comment is a marker, however it is - * spelled: a round whose keys are missing or misspelled still closes the - * gate, because the alternative is to read the previous round's `approved` - * as the latest word. A missing `verdict=` is read from a bare word in the - * marker (`changes-requested`) and otherwise is `unreadable`; neither is - * `approved`. - */ -export function reviewMarker(body: string): ReviewMarker | null { - const first = body.split("\n", 1)[0]!.trim(); - const m = REVIEW_MARKER.exec(first); - if (m === null) return null; - const keyed = new Map(); - const bare: string[] = []; - for (const token of m[1]!.trim().split(/\s+/u).filter((t) => t !== "")) { - const eq = token.indexOf("="); - if (eq > 0) keyed.set(token.slice(0, eq).toLowerCase(), token.slice(eq + 1)); - else bare.push(token); - } - const round = Number(keyed.get("round") ?? ""); - // BB-207: `substantial=yes|no` is the reviewer's diff judgment, absent is - // unjudged; `spec=yes` says a referenced ticket has an approved spec. - const substantial = keyed.get("substantial"); - return { - round: Number.isInteger(round) && round > 0 ? round : 0, - sha: keyed.get("sha") ?? "", - verdict: keyed.get("verdict") ?? bare[0] ?? UNREADABLE_VERDICT, - lane: keyed.get("lane") ?? null, - approver: keyed.get("approver"), - substantial: substantial === "yes" ? true : substantial === "no" ? false : null, - ticketApprovedSpec: keyed.get("spec") === "yes", - }; -} - -/** The newest reviewer marker in a comment list, by comment time. */ -export function latestReviewMarker(comments: readonly SnapshotComment[]): ReviewMarker | null { - let at = ""; - let latest: ReviewMarker | null = null; - for (const c of comments) { - const m = reviewMarker(c.body); - if (m !== null && c.createdAt >= at) { - at = c.createdAt; - latest = m; - } - } - return latest; -} /** * A change in the lane a reviewer's rounds are in: the one thing about a @@ -125,19 +61,6 @@ export function markerLaneChange(before: readonly SnapshotComment[], after: read return { from: b.lane, to: a.lane }; } -/** - * The product reason a pull request's owner lane carries (BB-207): the review - * binding was judged `substantial` from the diff, and no ticket it references - * has an approved spec. `substantial` is the binding's ready-gate judgment; - * null (unjudged) and false leave every lane exactly as today, which is what - * keeps the change inert until a reviewer reports a verdict. A referenced - * ticket with an approved spec is the existing ack, so the owner is not asked - * again. The reason is a property of the pull request, never of a commit: a - * later push only re-asks when it changed scope or behaviour. - */ -export function productReason(substantial: boolean | null, ticketApprovedSpec: boolean): "product" | null { - return substantial === true && !ticketApprovedSpec ? "product" : null; -} /** * The product inputs `prGate` reads from a binding (BB-207). A review or PR @@ -645,36 +568,6 @@ export function namesUserAsBlocker(text: string): boolean { return /(?:^|[^\p{L}\p{N}])(?:user|owner|human)(?:[^\p{L}\p{N}]|$)/iu.test(text); } -// ---- approval words ------------------------------------------------------------------- - -/** - * How a piece of text answers an approval request. Explicit approval words - * approve; explicit refusal words decline; anything else is ambiguous and is - * the project steward's to interpret. The page answer uses all three; a - * project steward's own guidance + resolve is a decision, so anything that - * does not approve declines (as before). - */ -export type ApprovalVerdict = "approve" | "decline" | "ambiguous"; - -const NEGATED_APPROVE = /(?:^|[^\p{L}\p{N}])(?:not|never|cannot|do\s+not|[a-z]+n['’]t)\s+(?:\w+\s+){0,2}?(?:approv\w*|ship|lgtm|go\s+ahead|looks?\s+good|sign\s*off|yes)(?:[^\p{L}\p{N}]|$)/iu; -const REFUSAL = /(?:^|[^\p{L}\p{N}])(?:reject(?:ed)?|declin(?:e|ed)|hold\s+off)(?:[^\p{L}\p{N}]|$)/iu; -const APPROVE_WORDS = /(?:^|[^\p{L}\p{N}])(?:lgtm|approv(?:e|es|ed|al)|ship\s*it|go\s*ahead|looks?\s+good|sign(?:ed)?\s*off|:shipit:|yes|yep|yeah)(?:[^\p{L}\p{N}]|$)/iu; -const DECLINE_WORDS = /(?:^|[^\p{L}\p{N}])(?:no|nope|don'?t|do\s+not|not\s+yet|changes?\s+(?:are\s+)?(?:needed|requested)|needs?\s+changes?)(?:[^\p{L}\p{N}]|$)/iu; - -/** - * A clear refusal first, so "not approved" and "don't ship it" decline rather - * than matching the approval word inside them; then approval words, so - * "LGTM, no concerns" and "ship it, no further changes needed" approve; then - * a bare no or a request for changes declines; anything else is ambiguous. - */ -export function classifyApproval(text: string | null | undefined): ApprovalVerdict { - const flat = (text ?? "").replace(/\s+/gu, " ").trim(); - if (flat === "") return "ambiguous"; - if (NEGATED_APPROVE.test(flat) || REFUSAL.test(flat)) return "decline"; - if (APPROVE_WORDS.test(flat)) return "approve"; - if (DECLINE_WORDS.test(flat)) return "decline"; - return "ambiguous"; -} /** * A steward that asks for an approval that is already open. The plugin files @@ -742,56 +635,6 @@ export function compactDue(usedTokens: number | null, limit: number, incomingCha return usedTokens > limit || usedTokens + Math.ceil(incomingChars / CHARS_PER_TOKEN) > limit; } -// ---- the ready gate ------------------------------------------------------------------ - -export type GateState = { - /** No ready gate in the playbook: nothing to pass. */ - none: boolean; - checklistMet: boolean; - unmet: string[]; - /** Null until the steward has judged it. */ - substantial: boolean | null; - approvalNeeded: boolean; - approved: boolean; - /** An approval exists but the spec has changed since. */ - approvalStale: boolean; - passed: boolean; -}; - -/** Checklist wording as compared: case, runs of spaces and trailing punctuation do not count. */ -const checklistKey = (s: string) => s.toLowerCase().replace(/\s+/gu, " ").replace(/[\s.:;,]+$/u, "").trim(); - -/** What the steward last reported for one of the playbook's checklist items. */ -export const reportedItem = (ready: ReadyState, item: string): ReadyState["checklist"][number] | undefined => - ready.checklist.find((c) => checklistKey(c.item) === checklistKey(item)); - -/** - * The playbook's own wording for a reported checklist entry: by its number - * in the wake (1-based), or by its text as the playbook writes it. Null for - * anything else. Stewards paraphrase long items, so a paraphrase is refused - * rather than guessed at: a guess that misses leaves the gate shut in - * silence, which is how it never passed. - */ -export function resolveChecklistItem(items: readonly string[], reported: string | number): string | null { - const text = String(reported); - const number = /^\s*#?(\d+)\.?\s*$/u.exec(text); - if (number !== null) return items[Number(number[1]) - 1] ?? null; - return items.find((item) => checklistKey(item) === checklistKey(text)) ?? null; -} - -export function gateState(playbook: Playbook, ready: ReadyState, currentSpecHash: string): GateState { - if (playbook.ready === null) return { none: true, checklistMet: true, unmet: [], substantial: null, approvalNeeded: false, approved: false, approvalStale: false, passed: true }; - const items = playbook.ready.checklist; - const unmet = items.filter((item) => reportedItem(ready, item)?.met !== true); - const checklistMet = unmet.length === 0; - // Unjudged counts as substantial: the safe side of the gate. - const approvalNeeded = ready.substantial !== false; - const approved = ready.approval !== null; - const approvalStale = approved && ready.approval!.specHash !== currentSpecHash; - const passed = checklistMet && (!approvalNeeded || (approved && !approvalStale)); - return { none: false, checklistMet, unmet, substantial: ready.substantial, approvalNeeded, approved, approvalStale, passed }; -} - /** * Whether the gate is shut on something known rather than merely not yet * reported: approval is needed and the current spec has none (missing, @@ -806,14 +649,6 @@ export function gateKnownUnmet(playbook: Playbook, ready: ReadyState, currentSpe return gate.unmet.some((item) => reportedItem(ready, item)?.met === false); } -/** - * Whether the gate waits on its approver: the checklist is met, the ticket - * is (or may be) substantial, and the current spec has no approval. The - * plugin asks the approver itself then, so the gate does not depend on the - * steward remembering to. - */ -export const approvalWanted = (gate: GateState): boolean => !gate.none && gate.checklistMet && gate.approvalNeeded && !gate.passed; - /** * A status that says it waits on the project steward or the user while * none of its escalations is open: a wait nobody above it hears about. diff --git a/plugins/stewards/server.ts b/plugins/stewards/server.ts index 6d75e581..27b7a21e 100644 --- a/plugins/stewards/server.ts +++ b/plugins/stewards/server.ts @@ -95,11 +95,11 @@ import { HOOKS_URL_ENV, createHooksLoop, type Lease, type Rejection } from "./ho import { nightlyDue, stillNight } from "./nightly.js"; import { classify, hasFingerprintLine, lookChanges, opsTargetKey, parseOpsTargetKey, ticketResolved, type DependabotTarget, type FingerprintRow, type K8sTarget, type OpsLook, type OpsTarget, type SentryTarget, type Sighting, type TicketState } from "./ops.js"; import { EVENTS_PER_LOOK, NO_TOKEN, RECENT_PER_LOOK, SENTRY_API_URL, TOKEN_SCOPES, createSentryReader, emptySentryLook, issueInOrg, recentLine, sentryChanges, sentryResolveTargets, sentrySightings, sentrySummary, type SentryFinding, type SentryLook, type SentryReader } from "./sentry.js"; -import { advanced, answerDiffers, anyWorkerInFlight, approvalWanted, asksForOpenApproval, classifyApproval, commentDiffersFromRecommendation, compactDue, deploymentSettled, diffObserved, endingDeadline, followingThrough, gateKnownUnmet, gateState, hasLabel, IDLE_WAKE_TEXT, idleInActiveStage, isCommentNews, isOwnStageChange, isSpecComment, latestReviewMarker, lifecycle, mechanicalStatus, namesUserAsBlocker, newHumanComments, newestCommentAt, openUserFacingItems, ownChecksFinished, ownPullClosed, ownPullMerged, phaseAndStage, productGateOptions, resolveChecklistItem, runningWorkerReason, newestEditComment, specDiff, specHash, specText, stuckDue, unheardWait, workerInFlight, workerReportedTerminal, type Change, type MechanicalEvent, type OpenUserItem } from "./reconcile.js"; -import { latestProjectApprovalMarker, prGate } from "./gate.js"; -import { approveOwnerReviewedPull, approvePrPageAction, declineOwnerReviewedPull, fileForwardedApprovePrNeed, isProjectSteward, markerKey, placeReportedPull, specApprovedByProject, syncPullApproval, type ApprovalsContext } from "./approvals.js"; +import { advanced, answerDiffers, anyWorkerInFlight, asksForOpenApproval, commentDiffersFromRecommendation, compactDue, deploymentSettled, diffObserved, endingDeadline, followingThrough, gateKnownUnmet, hasLabel, IDLE_WAKE_TEXT, idleInActiveStage, isCommentNews, isOwnStageChange, lifecycle, mechanicalStatus, namesUserAsBlocker, newHumanComments, newestCommentAt, openUserFacingItems, ownChecksFinished, ownPullClosed, ownPullMerged, phaseAndStage, productGateOptions, runningWorkerReason, newestEditComment, specDiff, specHash, specText, stuckDue, unheardWait, workerInFlight, workerReportedTerminal, type Change, type MechanicalEvent, type OpenUserItem } from "./reconcile.js"; +import { classifyApproval, latestProjectApprovalMarker, latestReviewMarker, prGate } from "./gate.js"; +import { approveOwnerReviewedPull, approvePrPageAction, declineOwnerReviewedPull, fileForwardedApprovePrNeed, isProjectSteward, markerKey, placeReportedPull, specApprovedByProject, syncPullApproval, userApprovedOnEntity, userSaid, type ApprovalsContext } from "./approvals.js"; import { deliverEscalation, nextRoute, routedTo, type EscalationsContext } from "./escalations.js"; -import { briefGate, syncApprovalRequest, type ReadyGateContext } from "./ready-gate.js"; +import { approvalWanted, briefGate, gateState, resolveChecklistItem, syncApprovalRequest, type ReadyGateContext } from "./ready-gate.js"; import { SOURCE_PLUGIN_IDS, STEWARDS_PLUGIN_ID, @@ -3307,7 +3307,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const escalationsCtx: EscalationsContext = { store, scheduleReconcile, wakeProject }; /** The ready-gate helpers bound to this server's store and spec readers (ready-gate.ts). */ - const readyGateCtx: ReadyGateContext = { store, specHashFor, specTextFor, log: (message) => bb.log.info(message) }; + const readyGateCtx: ReadyGateContext = { store, specHashFor, specTextFor, specDiff, log: (message) => bb.log.info(message) }; const ownThreads = (b: Binding) => new Set([b.threadId, ...store.workers(b.id).map((w) => w.threadId)].filter((t): t is string => t !== null)); @@ -6549,6 +6549,26 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P sendTo, wakeProject, pullChangedFiles, + recentUserTurnTexts: async (threadId) => { + const out: string[] = []; + const rows = await bb.sdk.threads.events.list({ threadId, types: ["client/turn/requested"], order: "desc", limit: "30" }); + for (const row of rows) { + if (now() - row.createdAt > timing.approvalLookbackMs) break; + // A turn from before this plugin started recording its sends cannot be + // told from the plugin's own wakes, so it is never the owner's (BB-178). + if (row.createdAt < sentSince) continue; + const data = row.data as { initiator?: string; input?: unknown }; + if (data.initiator !== "user") continue; + const text = turnText(data.input); + if (text === null) continue; + // A wake the plugin sent is recorded under its text's hash; it quotes + // the entity verbatim and must never count as the owner's approval. + if ((await bb.storage.kv.get(`sent/${sentHash(text)}`)) !== undefined) continue; + out.push(normalizeText(text)); + } + return out; + }, + isUserThread, loadDecisions: async (projectId) => (await loadConfig(projectId))?.file.bigDecisions ?? [], loadDecisionsOrNull: async (projectId) => (await loadConfig(projectId))?.file.bigDecisions ?? null, observedPulls, @@ -8940,48 +8960,6 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P }); } - /** Whether the user said these words in this thread lately, in their own turn rather than the plugin's own wake (BB-178). */ - async function userSaid(threadId: string, quote: string): Promise { - const want = normalizeText(quote); - const rows = await bb.sdk.threads.events.list({ threadId, types: ["client/turn/requested"], order: "desc", limit: "30" }); - for (const row of rows) { - if (now() - row.createdAt > timing.approvalLookbackMs) break; - // A turn from before this plugin started recording its sends cannot be - // told from the plugin's own wakes, so it is never the owner's (BB-178). - if (row.createdAt < sentSince) continue; - const data = row.data as { initiator?: string; input?: unknown }; - if (data.initiator !== "user") continue; - const text = turnText(data.input); - if (text === null) continue; - if (!normalizeText(text).includes(want)) continue; - // A wake the plugin sent is recorded under its text's hash; it quotes - // the entity verbatim and must never count as the owner's approval. - if ((await bb.storage.kv.get(`sent/${sentHash(text)}`)) !== undefined) continue; - return true; - } - return false; - } - - /** - * Whether the connected user approved in an unsigned comment on the - * entity, after the latest spec comment (criterion 3). `quote` is matched - * against the comment's text, so the steward can quote it back. - */ - function userApprovedOnEntity(entity: EntitySnapshot | null, stewardThreadId: string | null, quote: string): boolean { - if (entity === null || entity.viewer === null) return false; - const viewer = entity.viewer.toLowerCase(); - const latestSpec = entity.comments.filter((c) => isSpecComment(c, stewardThreadId, isUserThread)).reduce((max, c) => (c.createdAt > max ? c.createdAt : max), ""); - const want = quote.replace(/\s+/gu, " ").trim().toLowerCase(); - return entity.comments.some( - (c) => - c.signedBy === null && - c.author.toLowerCase() === viewer && - c.createdAt > latestSpec && - classifyApproval(c.body) === "approve" && - c.body.replace(/\s+/gu, " ").toLowerCase().includes(want), - ); - } - bb.agents.registerTool({ name: "steward_report", description: @@ -9949,7 +9927,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const entity = (target.source === "ops" ? null : await snapshot(target.source, target.entityId).catch(() => null)) ?? target.lastSnapshot.entity; const byProject = specApprovedByProject(approvalsCtx, threadId, target, mode); // Criterion 3: the user's words may be an unsigned comment on the entity itself, not only a message in this thread. - const byUser = input.quote !== undefined && ((await userSaid(threadId, input.quote)) || userApprovedOnEntity(entity, target.threadId, input.quote)); + const byUser = input.quote !== undefined && ((await userSaid(approvalsCtx, threadId, input.quote)) || userApprovedOnEntity(approvalsCtx, entity, target.threadId, input.quote)); if (!byProject && !byUser) { return toolError( new Error( diff --git a/plugins/stewards/wake.test.ts b/plugins/stewards/wake.test.ts index 08ee9350..4421561b 100644 --- a/plugins/stewards/wake.test.ts +++ b/plugins/stewards/wake.test.ts @@ -4,7 +4,7 @@ import type { DepsDecision } from "./store.js"; import { parsePlaybook } from "./config.js"; import { explainDecision, formatDecisionListing } from "./decisions.js"; import { comment, issue, pull } from "./fixtures.js"; -import { gateState } from "./reconcile.js"; +import { gateState } from "./ready-gate.js"; import { MIGRATIONS, RESOLUTION_KINDS, createStore } from "./store.js"; import { DEPS_PRS, GITHUB_PRS, GITHUB_REVIEWS, LINEAR_TICKETS, OPS_WATCH, DEPS_ALERTS, PLAYBOOK_TEMPLATES } from "./templates.js"; import { STANDING_RULES, BRIEF_PREFIX, formatBrief, formatDepsWake, formatOpsWake, formatProjectWake, formatRecords, formatWake, initiativeProgress, initiativeProgressFlag, spawnCommand, workerFinishNote, workerQuestion, type DigestRecords, type WakeInput } from "./wake.js"; diff --git a/plugins/stewards/wake.ts b/plugins/stewards/wake.ts index 704d63bf..3d4deedb 100644 --- a/plugins/stewards/wake.ts +++ b/plugins/stewards/wake.ts @@ -13,7 +13,8 @@ import { FINDING_CLASSES, type FindingClass, type OpsFinding, type OpsLook } fro import type { DepsFinding, DepsLook } from "./deps.js"; import { ADOPTION_CHOICES, adoptionHow, dueDate } from "./deps.js"; import { NO_TOKEN, TOKEN_PAGE, TOKEN_SCOPES, TOKEN_SETTING, type SentryFinding, type SentryLook, type SentryReason } from "./sentry.js"; -import { newHumanComments, productGateOptions, reportedItem, unheardWait, type GateState } from "./reconcile.js"; +import { newHumanComments, productGateOptions, unheardWait } from "./reconcile.js"; +import { reportedItem, type GateState } from "./ready-gate.js"; import { prGate, type ProjectApprovalRecord } from "./gate.js"; import { conflictsOf, openPull, type Conflict } from "./schedule.js"; import { SNAPSHOT_COMMENTS_MAX, type EntitySnapshot, type SessionWake, type SnapshotComment, type WorkflowRun } from "./source-contract.js"; From f34c00f9e35a3e3bc312f32a59de95e6166cd178 Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Tue, 10 Nov 2026 05:20:11 +0000 Subject: [PATCH 7/7] stewards: keep the user-turn filter in approvals.ts (BB-280) --- plugins/stewards/approvals.test.ts | 25 +++++++++++- plugins/stewards/approvals.ts | 51 +++++++++++++++++++++---- plugins/stewards/server.ts | 61 +++++------------------------- 3 files changed, 77 insertions(+), 60 deletions(-) diff --git a/plugins/stewards/approvals.test.ts b/plugins/stewards/approvals.test.ts index 941cd3c2..d8415961 100644 --- a/plugins/stewards/approvals.test.ts +++ b/plugins/stewards/approvals.test.ts @@ -6,7 +6,7 @@ // BB-279: an owner's page approval and the project steward's decision deliver // to a parked worker by the same path (`deliverProjectApproval`). import { describe, expect, it, vi } from "vitest"; -import { approvePrPageAction, deliverProjectApproval, recordProjectDecision, userApprovedOnEntity, userSaid, type ApprovalsContext } from "./approvals.js"; +import { approvePrPageAction, deliverProjectApproval, isUserThread, recordProjectDecision, userApprovedOnEntity, userSaid, type ApprovalsContext } from "./approvals.js"; import { isSelfWidening } from "./decisions.js"; import { comment, parkedWorker, pull } from "./fixtures.js"; import type { Binding, Need } from "./store.js"; @@ -158,4 +158,27 @@ describe("the approval-recording files are self-widening (BB-280)", () => { expect(typeof userSaid).toBe("function"); expect(typeof userApprovedOnEntity).toBe("function"); }); + + it("filters the user's turns in approvals.ts, so the logic cannot move back to server.ts", async () => { + const at = 1_000_000; + const turn = (createdAt: number, initiator: string, text: string) => ({ createdAt, data: { initiator, input: [{ type: "text", text }] } }); + const ctx = (rows: Array<{ createdAt: number; data: unknown }>, sent: string[] = [], sentSince = 0) => + ({ now: () => at, approvalLookbackMs: 60_000, sentSince, turnRequests: async () => rows, pluginSent: async (text: string) => sent.includes(text) }) as unknown as ApprovalsContext; + // A genuine user turn counts; the plugin's own wake, recorded under its hash, does not. + await expect(userSaid(ctx([turn(at, "user", "LGTM"), turn(at, "user", "LGTM wake")], ["LGTM wake"]), "thr_user", "LGTM")).resolves.toBe(true); + await expect(userSaid(ctx([turn(at, "user", "LGTM wake")], ["LGTM wake"]), "thr_user", "LGTM")).resolves.toBe(false); + // A steward's turn, and a turn from before the plugin recorded its sends, do not count. + await expect(userSaid(ctx([turn(at, "steward", "LGTM")]), "thr_user", "LGTM")).resolves.toBe(false); + await expect(userSaid(ctx([turn(at, "user", "LGTM")], [], at + 1), "thr_user", "LGTM")).resolves.toBe(false); + // A turn older than the lookback does not count. + await expect(userSaid(ctx([turn(at - 120_000, "user", "LGTM")]), "thr_user", "LGTM")).resolves.toBe(false); + }); + + it("decides whose thread a signed comment is from the store, not a server closure", () => { + const ctx = (byThread: unknown, worker: unknown, projectByThread: unknown) => ({ store: { byThread: () => byThread, worker: () => worker, projectByThread: () => projectByThread } }) as unknown as ApprovalsContext; + expect(isUserThread(ctx(null, null, null), "thr_user")).toBe(true); + expect(isUserThread(ctx({}, null, null), "thr_user")).toBe(false); + expect(isUserThread(ctx(null, {}, null), "thr_user")).toBe(false); + expect(isUserThread(ctx(null, null, {}), "thr_user")).toBe(false); + }); }); diff --git a/plugins/stewards/approvals.ts b/plugins/stewards/approvals.ts index c9a8b765..3a1cfca3 100644 --- a/plugins/stewards/approvals.ts +++ b/plugins/stewards/approvals.ts @@ -17,6 +17,19 @@ import type { Binding, Escalation, Need, Observed, Store } from "./store.js"; const errorMessage = (error: unknown) => (error instanceof Error ? error.message : String(error)); +export const normalizeText = (text: string) => text.replace(/\s+/gu, " ").trim().toLowerCase(); + +/** The text of a turn's input parts, joined; null when it carries no text (BB-178). */ +function turnText(input: unknown): string | null { + if (!Array.isArray(input)) return null; + const parts = input.flatMap((part) => { + if (typeof part !== "object" || part === null) return []; + const p = part as { type?: unknown; text?: unknown }; + return p.type === "text" && typeof p.text === "string" ? [p.text] : []; + }); + return parts.length === 0 ? null : parts.join("\n"); +} + /** * What the approval functions need from the server: the store and GitHub * writer, the clock, and the few server helpers that read snapshots, deliver @@ -34,10 +47,14 @@ export type ApprovalsContext = { sendTo: (threadId: string, text: string) => Promise<"sent" | "gone" | "failed" | "unavailable">; wakeProject: (projectId: string, reason: string, opts: { digestDue: boolean; notes?: string[] }) => Promise; pullChangedFiles: (entity: EntitySnapshot) => Promise; - /** The user's own recent turn texts in a thread, normalized, newest first, excluding the plugin's own wakes (BB-178). */ - recentUserTurnTexts: (threadId: string) => Promise; - /** Whether a thread is the user's own rather than a steward's, a worker's or the project steward's. */ - isUserThread: (threadId: string) => boolean; + /** The raw `client/turn/requested` rows for a thread, newest first. */ + turnRequests: (threadId: string) => Promise>; + /** Whether the plugin itself sent this text as a wake, recorded under its hash (BB-178). */ + pluginSent: (text: string) => Promise; + /** How far back an approval quote is looked for. */ + approvalLookbackMs: number; + /** The time this plugin started recording its sends; a turn from before it is never the user's (BB-178). */ + sentSince: number; loadDecisions: (projectId: string) => Promise; /** The project's decide list, or null when the project file could not be read (BB-280). */ loadDecisionsOrNull: (projectId: string) => Promise; @@ -130,6 +147,10 @@ export async function recordProjectDecision(ctx: ApprovalsContext, b: Binding, p /** Whether `threadId` is the thread of `b`'s project steward (BB-280). */ export const isProjectSteward = (ctx: ApprovalsContext, threadId: string, b: Binding): boolean => ctx.store.project(b.projectId).threadId === threadId; +/** Whether a thread is the user's own rather than a steward's, a worker's or the project steward's. */ +export const isUserThread = (ctx: ApprovalsContext, threadId: string): boolean => + ctx.store.byThread(threadId) === null && ctx.store.worker(threadId) === null && ctx.store.projectByThread(threadId) === null; + /** * Whether the project steward's thread may approve `b`'s spec under the * playbook's `mode`: `project` and the project steward's own thread. `user` is @@ -146,8 +167,24 @@ export const specApprovedByProject = (ctx: ApprovalsContext, threadId: string, b * normalized. The path was in server.ts before (BB-280). */ export async function userSaid(ctx: ApprovalsContext, threadId: string, quote: string): Promise { - const want = quote.replace(/\s+/gu, " ").trim().toLowerCase(); - return (await ctx.recentUserTurnTexts(threadId)).some((text) => text.includes(want)); + const want = normalizeText(quote); + const rows = await ctx.turnRequests(threadId); + for (const row of rows) { + if (ctx.now() - row.createdAt > ctx.approvalLookbackMs) break; + // A turn from before this plugin started recording its sends cannot be + // told from the plugin's own wakes, so it is never the owner's (BB-178). + if (row.createdAt < ctx.sentSince) continue; + const data = row.data as { initiator?: string; input?: unknown }; + if (data.initiator !== "user") continue; + const text = turnText(data.input); + if (text === null) continue; + if (!normalizeText(text).includes(want)) continue; + // A wake the plugin sent is recorded under its text's hash; it quotes + // the entity verbatim and must never count as the owner's approval. + if (await ctx.pluginSent(text)) continue; + return true; + } + return false; } /** @@ -159,7 +196,7 @@ export async function userSaid(ctx: ApprovalsContext, threadId: string, quote: s export function userApprovedOnEntity(ctx: ApprovalsContext, entity: EntitySnapshot | null, stewardThreadId: string | null, quote: string): boolean { if (entity === null || entity.viewer === null) return false; const viewer = entity.viewer.toLowerCase(); - const latestSpec = entity.comments.filter((c) => isSpecComment(c, stewardThreadId, ctx.isUserThread)).reduce((max, c) => (c.createdAt > max ? c.createdAt : max), ""); + const latestSpec = entity.comments.filter((c) => isSpecComment(c, stewardThreadId, (threadId) => isUserThread(ctx, threadId))).reduce((max, c) => (c.createdAt > max ? c.createdAt : max), ""); const want = quote.replace(/\s+/gu, " ").trim().toLowerCase(); return entity.comments.some( (c) => diff --git a/plugins/stewards/server.ts b/plugins/stewards/server.ts index 27b7a21e..0f1edd6d 100644 --- a/plugins/stewards/server.ts +++ b/plugins/stewards/server.ts @@ -97,7 +97,7 @@ import { classify, hasFingerprintLine, lookChanges, opsTargetKey, parseOpsTarget import { EVENTS_PER_LOOK, NO_TOKEN, RECENT_PER_LOOK, SENTRY_API_URL, TOKEN_SCOPES, createSentryReader, emptySentryLook, issueInOrg, recentLine, sentryChanges, sentryResolveTargets, sentrySightings, sentrySummary, type SentryFinding, type SentryLook, type SentryReader } from "./sentry.js"; import { advanced, answerDiffers, anyWorkerInFlight, asksForOpenApproval, commentDiffersFromRecommendation, compactDue, deploymentSettled, diffObserved, endingDeadline, followingThrough, gateKnownUnmet, hasLabel, IDLE_WAKE_TEXT, idleInActiveStage, isCommentNews, isOwnStageChange, lifecycle, mechanicalStatus, namesUserAsBlocker, newHumanComments, newestCommentAt, openUserFacingItems, ownChecksFinished, ownPullClosed, ownPullMerged, phaseAndStage, productGateOptions, runningWorkerReason, newestEditComment, specDiff, specHash, specText, stuckDue, unheardWait, workerInFlight, workerReportedTerminal, type Change, type MechanicalEvent, type OpenUserItem } from "./reconcile.js"; import { classifyApproval, latestProjectApprovalMarker, latestReviewMarker, prGate } from "./gate.js"; -import { approveOwnerReviewedPull, approvePrPageAction, declineOwnerReviewedPull, fileForwardedApprovePrNeed, isProjectSteward, markerKey, placeReportedPull, specApprovedByProject, syncPullApproval, userApprovedOnEntity, userSaid, type ApprovalsContext } from "./approvals.js"; +import { approveOwnerReviewedPull, approvePrPageAction, declineOwnerReviewedPull, fileForwardedApprovePrNeed, isProjectSteward, isUserThread, markerKey, normalizeText, placeReportedPull, specApprovedByProject, syncPullApproval, userApprovedOnEntity, userSaid, type ApprovalsContext } from "./approvals.js"; import { deliverEscalation, nextRoute, routedTo, type EscalationsContext } from "./escalations.js"; import { approvalWanted, briefGate, gateState, resolveChecklistItem, syncApprovalRequest, type ReadyGateContext } from "./ready-gate.js"; import { @@ -237,9 +237,6 @@ export interface StewardsDeps { const errorMessage = (error: unknown) => (error instanceof Error ? error.message : String(error)); -/** The comparable form of a message: whitespace collapsed, trimmed, lowercased. */ -const normalizeText = (text: string) => text.replace(/\s+/gu, " ").trim().toLowerCase(); - /** * The key a text the plugin sent is recorded under, so the ready gate can tell * the plugin's own wakes from the owner's words (BB-178). Content, not the @@ -247,21 +244,6 @@ const normalizeText = (text: string) => text.replace(/\s+/gu, " ").trim().toLowe */ const sentHash = (text: string) => createHash("sha256").update(normalizeText(text)).digest("hex"); -/** - * The text parts of a `client/turn/requested` event's input, joined; null when - * it carries none. bb stores a turn's input as parts under `data.input`, with - * the words in the `text` parts (BB-178). - */ -function turnText(input: unknown): string | null { - if (!Array.isArray(input)) return null; - const parts = input.flatMap((part) => { - if (typeof part !== "object" || part === null) return []; - const p = part as { type?: unknown; text?: unknown }; - return p.type === "text" && typeof p.text === "string" ? [p.text] : []; - }); - return parts.length === 0 ? null : parts.join("\n"); -} - /** The server setting the footer toggle stores the replaced navigation in (BB-184). */ export const NAVIGATION_RESTORE_KEY = "sidebar.navigationRestore"; @@ -3288,20 +3270,11 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // ---- the reconcile loop ------------------------------------------------------------------ - /** - * A thread the plugin does not know as a steward, a worker or the project - * steward is one the user runs themselves. Its signed comments count as - * the user's own words, including for the spec (criterion 4). - */ - function isUserThread(threadId: string): boolean { - return store.byThread(threadId) === null && store.worker(threadId) === null && store.projectByThread(threadId) === null; - } - /** The spec hash a binding's approval is pinned to, counting the user's own spec comments. */ - const specHashFor = (b: Pick, entity: EntitySnapshot): string => specHash(entity, b.threadId, isUserThread); + const specHashFor = (b: Pick, entity: EntitySnapshot): string => specHash(entity, b.threadId, (threadId) => isUserThread(approvalsCtx, threadId)); /** The text a binding's approval pins, stored as `approvedText` so BB-249 can diff a later edit against it. */ - const specTextFor = (b: Pick, entity: EntitySnapshot): string => specText(entity, b.threadId, isUserThread); + const specTextFor = (b: Pick, entity: EntitySnapshot): string => specText(entity, b.threadId, (threadId) => isUserThread(approvalsCtx, threadId)); /** The escalation-routing helpers bound to this server's store and wakes (escalations.ts). */ const escalationsCtx: EscalationsContext = { store, scheduleReconcile, wakeProject }; @@ -3942,7 +3915,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // BB-203: an open need or `user` escalation keeps the worker's own pull // request activity loud, so the steward can reconcile the user's item. const userWaiting = store.openNeeds(b.id).length > 0 || store.openEscalations(b.id).some((x) => x.status === "open" && x.route === "user"); - const fresh = prev === null ? [] : diffObserved(prev, observed, { own: ownThreads(b), stewardThreadId: b.threadId, approved: ready.approval !== null, cursor: b.commentCursor, workerInFlight, userWaiting, ownPulls, stageOf: (state) => phaseOf(playbook, state)?.stage ?? null, userThread: isUserThread }); + const fresh = prev === null ? [] : diffObserved(prev, observed, { own: ownThreads(b), stewardThreadId: b.threadId, approved: ready.approval !== null, cursor: b.commentCursor, workerInFlight, userWaiting, ownPulls, stageOf: (state) => phaseOf(playbook, state)?.stage ?? null, userThread: (threadId) => isUserThread(approvalsCtx, threadId) }); changes.push(...fresh); // BB-248: a label that changes the phase with no state change is news // on its own; a state move already carries its own actionable line. The @@ -6549,26 +6522,10 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P sendTo, wakeProject, pullChangedFiles, - recentUserTurnTexts: async (threadId) => { - const out: string[] = []; - const rows = await bb.sdk.threads.events.list({ threadId, types: ["client/turn/requested"], order: "desc", limit: "30" }); - for (const row of rows) { - if (now() - row.createdAt > timing.approvalLookbackMs) break; - // A turn from before this plugin started recording its sends cannot be - // told from the plugin's own wakes, so it is never the owner's (BB-178). - if (row.createdAt < sentSince) continue; - const data = row.data as { initiator?: string; input?: unknown }; - if (data.initiator !== "user") continue; - const text = turnText(data.input); - if (text === null) continue; - // A wake the plugin sent is recorded under its text's hash; it quotes - // the entity verbatim and must never count as the owner's approval. - if ((await bb.storage.kv.get(`sent/${sentHash(text)}`)) !== undefined) continue; - out.push(normalizeText(text)); - } - return out; - }, - isUserThread, + turnRequests: (threadId) => bb.sdk.threads.events.list({ threadId, types: ["client/turn/requested"], order: "desc", limit: "30" }), + pluginSent: async (text) => (await bb.storage.kv.get(`sent/${sentHash(text)}`)) !== undefined, + approvalLookbackMs: timing.approvalLookbackMs, + sentSince, loadDecisions: async (projectId) => (await loadConfig(projectId))?.file.bigDecisions ?? [], loadDecisionsOrNull: async (projectId) => (await loadConfig(projectId))?.file.bigDecisions ?? null, observedPulls, @@ -6663,7 +6620,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const approvedText = prior?.approvedText ?? b.ready.voidedApprovedText; const approvedAt = prior?.at ?? b.ready.voidedApprovedAt; const reapproval = approvedText !== null && approvedAt !== null ? specDiff(approvedText, currentText) : null; - const specComment = reapproval === null || approvedAt === null ? null : newestEditComment(entity, b.threadId, isUserThread, approvedAt); + const specComment = reapproval === null || approvedAt === null ? null : newestEditComment(entity, b.threadId, (threadId) => isUserThread(approvalsCtx, threadId), approvedAt); const what = reapproval === null ? `Approve ${b.entityKey}'s spec`