stewards: move roles parsing, the ready gate and escalation routing into modules (BB-280)

plugins/stewards/approvals.test.ts

@@ -6,7 +6,8 @@
 // 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, 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";
 
@@ -143,3 +144,41 @@ 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);
+    }
+  });
+
+  it("keeps the user's approval detection in approvals.ts", () => {
+    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);
+  });
+});

plugins/stewards/approvals.ts

@@ -7,15 +7,29 @@
 //
 // 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 { approvalMarkerLine, ESCALATED_APPROVAL_REFUSAL, prGate, PRODUCT_APPROVAL_REFUSAL, reviewWorkerThread, type PrGate } from "./gate.js";
+import { type BigDecision, decideFor, decideForPull, decideForRefusal, explainDecision, formatDecisionListing, type DecideMark } from "./decisions.js";
+import type { ApprovalMode } from "./config.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, 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));
 
+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
@@ -33,7 +47,17 @@ export type ApprovalsContext = {
   sendTo: (threadId: string, text: string) => Promise<"sent" | "gone" | "failed" | "unavailable">;
   wakeProject: (projectId: string, reason: string, opts: { digestDue: boolean; notes?: string[] }) => Promise<void>;
   pullChangedFiles: (entity: EntitySnapshot) => Promise<string[] | null>;
+  /** The raw `client/turn/requested` rows for a thread, newest first. */
+  turnRequests: (threadId: string) => Promise<ReadonlyArray<{ createdAt: number; data: unknown }>>;
+  /** Whether the plugin itself sent this text as a wake, recorded under its hash (BB-178). */
+  pluginSent: (text: string) => Promise<boolean>;
+  /** 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<readonly BigDecision[]>;
+  /** The project's decide list, or null when the project file could not be read (BB-280). */
+  loadDecisionsOrNull: (projectId: string) => Promise<readonly BigDecision[] | null>;
   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 +144,128 @@ 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 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
+ * the user's, recorded by quoting their words (BB-280; this acceptance was in
+ * 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: 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<boolean> {
+  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;
+}
+
+/**
+ * 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, (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) =>
+      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
+ * item for the project steward's lane (BB-51, BB-280).
+ */
+export const placeReportedPull = async (ctx: ApprovalsContext, pull: EntitySnapshot, decisions: readonly BigDecision[]): Promise<DecideMark> => 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<ProjectPullDecision> {
+  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<void> {
+  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

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", () => {