From 66b8603e9ee57ba78dd798e8bf68e76c8b0a8fea Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Sun, 15 Nov 2026 10:29:57 +0000 Subject: [PATCH 1/3] stewards: refresh a lagging playbook through a reviewed pull request (BB-328) When the nightly check finds a playbook whose template stamp lags the shipped template, queue one refresh per playbook and start a short-lived worker to merge the template into the project copy, keeping the project's own additions, update the stamp, and publish. The plugin opens the reviewed pull request, classifies the diff for the body, and the project steward reads the diff and decides with steward_update { refreshAction }. The row is the queue: one open refresh per playbook, a declined or failed refresh is not retried until the template changes, and the stamp is only updated by the merged pull request. --- plugins/stewards/DESIGN.md | 29 ++ plugins/stewards/README.md | 20 ++ plugins/stewards/migrations.snapshot.json | 4 +- plugins/stewards/refresh.test.ts | 178 +++++++++++++ plugins/stewards/refresh.ts | 235 ++++++++++++++++ plugins/stewards/server.test.ts | 95 +++++++ plugins/stewards/server.ts | 311 +++++++++++++++++++++- plugins/stewards/store.test.ts | 12 +- plugins/stewards/store.ts | 179 ++++++++++++- plugins/stewards/wake.ts | 31 ++- 10 files changed, 1082 insertions(+), 12 deletions(-) create mode 100644 plugins/stewards/refresh.test.ts create mode 100644 plugins/stewards/refresh.ts diff --git a/plugins/stewards/DESIGN.md b/plugins/stewards/DESIGN.md index 8e637cf4..b538deeb 100644 --- a/plugins/stewards/DESIGN.md +++ b/plugins/stewards/DESIGN.md @@ -669,6 +669,13 @@ At the project file's time, 01:00 US Eastern by default: 3. The project steward's pass follows, over the fresh status records; its reply is kept as the morning digest. +Before the wake, the same pass queues a refresh for every stamped playbook +that lags its shipped template and is not already open or deferred (BB-328 +Part 2). The queue is one row per playbook in `playbook_refreshes`; each is +handed to a short-lived worker, and the project steward is woken when the +reviewed pull request is ready. The queue and its guardrails are described +under "A lagging playbook is refreshed" above. + All of it must finish before 03:00, when bb-gate's quiet window restarts servers to apply staged updates and would kill a running turn. @@ -1030,6 +1037,28 @@ deployment: # where a merge is confirmed live (BB-53); omi `bb stewards template ` prints the current shipped text, and an unknown name lists the valid names and exits non-zero. A refresh is a reviewed change, never a mechanical overwrite; Part 2 queues it. +- **A lagging playbook is refreshed through a reviewed pull request + (BB-328 Part 2).** When the nightly pass finds a stamped playbook whose + hash differs from its template's, it queues one refresh per playbook + (`playbook_refreshes`) and starts a short-lived worker thread with the + project copy, the current template text and the stamp's template name. The + worker merges the template into the project copy keeping the project's own + additions, updates the stamp, and publishes with + `workspace_publish_guidance`. The plugin reads the published branch, opens + the pull request on the primary repo, and classifies the diff for the + body: *purely additive* means only insertions and the front matter + untouched apart from the stamp, and a change to `models`, `may`, a gate or + `requeue-default` is never additive. The classification is the plugin's + read, not the worker's claim; the project steward reads the diff in the + `## Playbooks to refresh` block and decides with + `steward_update { refreshAction: { id, action } }` — `merge` for a purely + additive, project-lane change, `forward` to file the owner's Review + PR/Drop item, `drop` to close it and decline. The guardrails are the row's: + at most one refresh is open per playbook, a declined or failed refresh is + not retried until the template changes again, and the stamp is updated only + by the merged pull request — which is what stops the playbook being queued. + A server on an image that predates the stamp key runs no queue at all and + leaves the playbook alone until it restarts. - **A phase may be keyed on a label too (BB-248).** A `label:` key in `phases` matches a present label on the entity, ignoring case, and wins over the state-keyed phase; with no matching label the state decides as before. diff --git a/plugins/stewards/README.md b/plugins/stewards/README.md index 2e63a9c2..3a047805 100644 --- a/plugins/stewards/README.md +++ b/plugins/stewards/README.md @@ -689,6 +689,26 @@ deduplicated by fingerprint so an unchanged finding is said once and again only when it changes (or clears and returns). `check` and the nightly pass are read-only and need no new credential. +**Refreshing the drift (BB-328 Part 2).** When the nightly pass finds a +stamped playbook whose hash lags its template, it queues one refresh per +playbook and starts a short-lived worker thread with the project copy, the +current template text and the stamp's template name. The worker merges the +template into the project copy keeping the project's own additions, updates +the stamp, and publishes with `workspace_publish_guidance`; the plugin opens +the pull request, classifies the diff (purely additive means only insertions +and the front matter untouched apart from the stamp; a change to `models`, +`may`, a gate or `requeue-default` is never additive) and lists the removals +and rewrites in the body. That classification is only the plugin's read of +the diff: the project steward reads it in the `## Playbooks to refresh` block +and decides with `steward_update { refreshAction: { id, action } }` — `merge` +for a purely additive, project-lane change, `forward` to file the owner's +Review PR/Drop item, `drop` to close it and decline. At most one refresh is +open per playbook, a declined or failed refresh is not retried until the +template changes again, and the stamp is only updated by the merged pull +request, which is what stops the playbook being queued. A server on an image +that predates the stamp key does not run the queue and leaves the playbook +alone until it restarts. + `roles.json` (BB-105, version 2 in BB-163) assigns what each person's server runs. Version 2 names each role once under `roles` and lists its holders under `holders`: diff --git a/plugins/stewards/migrations.snapshot.json b/plugins/stewards/migrations.snapshot.json index 87ba322a..7f0fcb8f 100644 --- a/plugins/stewards/migrations.snapshot.json +++ b/plugins/stewards/migrations.snapshot.json @@ -140,5 +140,7 @@ "d856f2127e2c5788a01c5ca80b216d1dc9be2a67bb273ad9082bbc69a279ce9a", "f1636e9715f38e19e3c21cc7d3008c4f8d734ec26e03eb983961acb7d4cc8949", "5aeed47ee4b133a8d26b01316c545a6f4c49b3c73fb9511cb4c2b9eae55a31fa", - "ded6270fedbb7d853a412b2167f164b3886f38ed975c124d83b40f6d625500ad" + "ded6270fedbb7d853a412b2167f164b3886f38ed975c124d83b40f6d625500ad", + "91adbc2f755ca9d634c4865855b06a5f3285b36cc7d3cb896b48b5251ca494f7", + "b8f990c5286fd23a4272bb1d25b4c9bb4ef61e584d29c61598a42561eee34e09" ] diff --git a/plugins/stewards/refresh.test.ts b/plugins/stewards/refresh.test.ts new file mode 100644 index 00000000..f44e3883 --- /dev/null +++ b/plugins/stewards/refresh.test.ts @@ -0,0 +1,178 @@ +// BB-328 Part 2: the pure half of the playbook refresh. Which stamped +// playbooks lag, whether one may be queued (one open per playbook, no retry +// after a decline until the template changes), how a published diff is +// classified, and the pull request body. +import { describe, expect, it } from "vitest"; +import { classifyRefresh, hasTemplateStamp, laggingPlaybooks, refreshPrBody, refreshQueueDecision } from "./refresh.js"; +import type { PlaybookRefresh } from "./store.js"; + +const refresh = (over: Partial): PlaybookRefresh => ({ + id: "rfr_1", + projectId: "p1", + file: "stewards/playbooks/ops-watch.md", + playbook: "ops-watch", + templateName: "ops-watch", + fromHash: "old", + targetHash: "new", + state: "queued", + threadId: null, + branch: null, + pr: null, + classification: null, + removals: [], + questionId: null, + note: null, + createdAt: 1, + updatedAt: 1, + resolvedAt: null, + ...over, +}); + +const candidate = { name: "ops-watch", templateName: "ops-watch", fromHash: "old", toHash: "new" }; + +describe("laggingPlaybooks", () => { + it("reports a stamped playbook whose hash moved, and nothing else", () => { + const hashes = new Map([["ops-watch", "new"]]); + const lags = laggingPlaybooks( + [ + { name: "ops-watch", template: { name: "ops-watch", hash: "old" } }, + { name: "current", template: { name: "ops-watch", hash: "new" } }, + { name: "unstamped", template: null }, + { name: "gone", template: { name: "gone", hash: "old" } }, + ], + hashes, + ); + expect(lags).toEqual([{ name: "ops-watch", templateName: "ops-watch", fromHash: "old", toHash: "new" }]); + }); + + it("reports a playbook once however many load paths reached it", () => { + const lags = laggingPlaybooks( + [ + { name: "ops-watch", template: { name: "ops-watch", hash: "old" } }, + { name: "ops-watch", template: { name: "ops-watch", hash: "old" } }, + ], + new Map([["ops-watch", "new"]]), + ); + expect(lags).toHaveLength(1); + }); +}); + +describe("refreshQueueDecision", () => { + it("queues when the playbook has no refresh yet", () => { + expect(refreshQueueDecision([], candidate)).toEqual({ queue: true }); + }); + + it("does not queue while a refresh is open (one open per playbook)", () => { + for (const state of ["queued", "spawned", "pr_open"] as const) { + expect(refreshQueueDecision([refresh({ state })], candidate)).toEqual({ queue: false, reason: "open" }); + } + }); + + it("does not retry a declined refresh until the template changes", () => { + const declined = [refresh({ state: "declined", targetHash: "new" })]; + expect(refreshQueueDecision(declined, candidate)).toEqual({ queue: false, reason: "declined" }); + // The template moved again: a new target hash is a new refresh. + expect(refreshQueueDecision(declined, { ...candidate, toHash: "newer" })).toEqual({ queue: true }); + }); + + it("does not retry a failed refresh until the template changes", () => { + const failed = [refresh({ state: "failed", targetHash: "new" })]; + expect(refreshQueueDecision(failed, candidate)).toEqual({ queue: false, reason: "failed" }); + expect(refreshQueueDecision(failed, { ...candidate, toHash: "newer" })).toEqual({ queue: true }); + }); + + it("does not re-queue a target a merged refresh already updated", () => { + const merged = [refresh({ state: "merged", targetHash: "new" })]; + expect(refreshQueueDecision(merged, candidate)).toEqual({ queue: false, reason: "merged" }); + expect(refreshQueueDecision(merged, { ...candidate, toHash: "newer" })).toEqual({ queue: true }); + }); + + it("reads the newest refresh, so an older decline does not block a newer target", () => { + const prior = [refresh({ id: "rfr_old", state: "declined", targetHash: "old" }), refresh({ id: "rfr_new", state: "merged", targetHash: "new" })]; + expect(refreshQueueDecision(prior, candidate)).toEqual({ queue: false, reason: "merged" }); + }); +}); + +describe("classifyRefresh", () => { + const before = ["---", "name: ops-watch", "kind: ops", "models:", " worker: claude/opus", "---", "", "## New", "", "Existing rule.", ""].join("\n"); + + it("calls an insertion-only body change purely additive", () => { + const after = before.replace("Existing rule.", "Existing rule.\n\nNew template rule."); + expect(classifyRefresh(before, after)).toEqual({ classification: "additive", removals: [], rewrites: [], reasons: [] }); + }); + + it("treats the stamp change alone as additive", () => { + const after = before.replace("kind: ops", "kind: ops\ntemplate: ops-watch@new"); + expect(classifyRefresh(before, after).classification).toBe("additive"); + }); + + it("is not additive when a body line is removed", () => { + const after = before.replace("Existing rule.\n", ""); + const c = classifyRefresh(before, after); + expect(c.classification).toBe("not-additive"); + expect(c.removals).toContain("Existing rule."); + }); + + it("is not additive when a body line is rewritten", () => { + const after = before.replace("Existing rule.", "Existing rule, changed."); + const c = classifyRefresh(before, after); + expect(c.classification).toBe("not-additive"); + expect(c.rewrites).toEqual([{ before: "Existing rule.", after: "Existing rule, changed." }]); + }); + + it("is never additive when models, may, a gate or requeue-default changes", () => { + const base = ["---", "name: ops-watch", "kind: ops", "models: claude/opus", "may: [comment]", "requeue-default: 1h", "---", "", "Body.", ""].join("\n"); + for (const [key, value] of [ + ["models", "claude/sonnet"], + ["may", "[comment, edit]"], + ["requeue-default", "2h"], + ["ready-gate", "on"], + ] as const) { + const after = base.includes(`${key}:`) ? base.replace(new RegExp(`^${key}:.*$`, "mu"), `${key}: ${value}`) : base.replace("kind: ops", `kind: ops\n${key}: ${value}`); + const c = classifyRefresh(base, after); + expect(c.classification, key).toBe("not-additive"); + expect(c.reasons.join(" "), key).toContain(key); + } + }); + + it("is not additive when any other front-matter key changes", () => { + const after = before.replace("kind: ops", "kind: ops, ops"); + expect(classifyRefresh(before, after).classification).toBe("not-additive"); + }); +}); + +describe("refreshPrBody", () => { + it("names the template, the stamp move and the removals", () => { + const body = refreshPrBody( + { playbook: "ops-watch", templateName: "ops-watch", fromHash: "old", targetHash: "new" }, + { classification: "not-additive", removals: ["Dropped rule."], rewrites: [{ before: "Old.", after: "New." }], reasons: [] }, + "thr_refresh", + ); + expect(body).toContain("stewards/playbooks/ops-watch.md"); + expect(body).toContain("`old` → `new`"); + expect(body).toContain("Not purely additive"); + expect(body).toContain("Dropped rule."); + expect(body).toContain("`Old.` → `New.`"); + expect(body).toContain("thr_refresh"); + }); + + it("keeps the refresh thread's own summary as its claim", () => { + const body = refreshPrBody( + { playbook: "ops-watch", templateName: "ops-watch", fromHash: "old", targetHash: "new" }, + { classification: "additive", removals: [], rewrites: [], reasons: [] }, + "thr_refresh", + "Purely additive: I added the new paragraph.", + ); + expect(body).toContain("its claim, not the decision"); + expect(body).toContain("Purely additive: I added the new paragraph."); + }); +}); + +describe("hasTemplateStamp", () => { + it("finds the stamp the refresh writes, and not another hash or name", () => { + const text = "---\nname: ops-watch\ntemplate: ops-watch@abc123\n---\n\nBody.\n"; + expect(hasTemplateStamp(text, "ops-watch", "abc123")).toBe(true); + expect(hasTemplateStamp(text, "ops-watch", "other")).toBe(false); + expect(hasTemplateStamp(text, "other", "abc123")).toBe(false); + }); +}); diff --git a/plugins/stewards/refresh.ts b/plugins/stewards/refresh.ts new file mode 100644 index 00000000..30c026ac --- /dev/null +++ b/plugins/stewards/refresh.ts @@ -0,0 +1,235 @@ +// The pure half of the playbook refresh (BB-328 Part 2): which stamped +// playbooks lag their shipped template, whether a queued refresh may be +// started, how the diff a refresh publishes is classified, and the pull +// request body the plugin opens from it. Nothing here touches the store or a +// plugin, so every decision is a unit test; server.ts applies what it returns. +import type { PlaybookRefresh } from "./store.js"; + +/** A playbook whose stamp hash differs from its current shipped template. */ +export type LaggingPlaybook = { + /** The playbook's front-matter `name`. */ + name: string; + /** The shipped template it was copied from. */ + templateName: string; + /** The stamp hash the project copy carries. */ + fromHash: string; + /** The current shipped template's hash, which the refresh targets. */ + toHash: string; +}; + +/** + * The stamped playbooks whose template hash has moved. A playbook with no + * stamp (unknown origin) and one whose template no longer exists are left + * alone: there is no template to merge, so they get the `check` warning but no + * refresh. One entry per playbook, however many load paths reached it. + */ +export function laggingPlaybooks( + playbooks: Iterable<{ name: string; template: { name: string; hash: string } | null }>, + templateHashes: ReadonlyMap, +): LaggingPlaybook[] { + const out: LaggingPlaybook[] = []; + const seen = new Set(); + for (const p of playbooks) { + if (p.template === null) continue; + if (seen.has(p.name)) continue; + seen.add(p.name); + const current = templateHashes.get(p.template.name); + if (current === undefined || current === p.template.hash) continue; + out.push({ name: p.name, templateName: p.template.name, fromHash: p.template.hash, toHash: current }); + } + return out; +} + +/** The states in which a refresh is still in flight for its playbook. */ +export const OPEN_REFRESH_STATES: readonly PlaybookRefresh["state"][] = ["queued", "spawned", "pr_open"]; + +/** Whether a playbook may be queued for a refresh, and why not when it may not. */ +export type RefreshQueueDecision = { queue: true } | { queue: false; reason: "open" | "declined" | "failed" | "merged" }; + +/** + * Whether to queue a refresh for `candidate` given every refresh already + * recorded for the playbook, newest last. At most one refresh is open per + * playbook, and a closed refresh is not retried until the template changes: a + * declined, failed or already-merged refresh whose target hash is still the + * current one blocks a new queue, while one that targeted an older hash does + * not. The merged block is the stamp-only-on-merge rule read from the queue + * side: the merged pull request is what updated the stamp, so a still-lagging + * read is stale and must not start a second refresh. + */ +export function refreshQueueDecision(prior: readonly PlaybookRefresh[], candidate: LaggingPlaybook): RefreshQueueDecision { + const forPlaybook = prior.filter((r) => r.playbook === candidate.name); + if (forPlaybook.some((r) => OPEN_REFRESH_STATES.includes(r.state))) return { queue: false, reason: "open" }; + const latest = forPlaybook.at(-1); + if (latest !== undefined && latest.targetHash === candidate.toHash && !OPEN_REFRESH_STATES.includes(latest.state)) { + return { queue: false, reason: latest.state === "merged" ? "merged" : latest.state === "failed" ? "failed" : "declined" }; + } + return { queue: true }; +} + +/** How a refresh's published diff reads: additive, or not, and what it moved. */ +export type RefreshClassification = { + classification: "additive" | "not-additive"; + /** Lines the merge removed or rewrote, one each, for the pull request body. */ + removals: string[]; + /** A rewrite as `before → after`. */ + rewrites: Array<{ before: string; after: string }>; + /** Why it is not purely additive: the front-matter keys that changed, if any. */ + reasons: string[]; +}; + +/** The front matter keys a change to which is never purely additive (BB-328). */ +const NEVER_ADDITIVE_KEYS = new Set(["models", "may", "requeue-default"]); + +const splitFrontMatter = (text: string): { front: string | null; body: string } => { + const m = /^---\r?\n([\s\S]*?)\r?\n---\r?\n?/u.exec(text); + if (m === null) return { front: null, body: text }; + return { front: m[1]!, body: text.slice(m[0].length) }; +}; + +/** The front-matter lines keyed by their key; a line that is not `key: value` is skipped. */ +const frontMatterLines = (front: string): Map => { + const out = new Map(); + for (const line of front.split("\n")) { + const m = /^([A-Za-z0-9_-]+):(.*)$/u.exec(line); + if (m !== null) out.set(m[1]!, line); + } + return out; +}; + +/** A line-level diff operation, from a longest-common-subsequence walk. */ +type DiffOp = { type: "equal" | "delete" | "insert"; line: string }; + +/** The LCS line diff of two texts, in order. */ +function diffOps(before: string[], after: string[]): DiffOp[] { + const n = before.length; + const m = after.length; + // lcs[i][j] is the length of the LCS of before[i:] and after[j:]. + const lcs: number[][] = Array.from({ length: n + 1 }, () => new Array(m + 1).fill(0)); + for (let i = n - 1; i >= 0; i -= 1) { + for (let j = m - 1; j >= 0; j -= 1) { + lcs[i]![j] = before[i] === after[j] ? lcs[i + 1]![j + 1]! + 1 : Math.max(lcs[i + 1]![j]!, lcs[i]![j + 1]!); + } + } + const ops: DiffOp[] = []; + let i = 0; + let j = 0; + while (i < n && j < m) { + if (before[i] === after[j]) { + ops.push({ type: "equal", line: before[i]! }); + i += 1; + j += 1; + } else if (lcs[i + 1]![j]! >= lcs[i]![j + 1]!) { + ops.push({ type: "delete", line: before[i]! }); + i += 1; + } else { + ops.push({ type: "insert", line: after[j]! }); + j += 1; + } + } + for (; i < n; i += 1) ops.push({ type: "delete", line: before[i]! }); + for (; j < m; j += 1) ops.push({ type: "insert", line: after[j]! }); + return ops; +} + +/** The removals and rewrites in a diff, pairing a delete with an adjacent insert as a rewrite. */ +function movedLines(ops: DiffOp[]): { removals: string[]; rewrites: Array<{ before: string; after: string }> } { + const removals: string[] = []; + const rewrites: Array<{ before: string; after: string }> = []; + let i = 0; + while (i < ops.length) { + if (ops[i]!.type === "equal") { + i += 1; + continue; + } + const dels: string[] = []; + const ins: string[] = []; + while (i < ops.length && ops[i]!.type !== "equal") { + if (ops[i]!.type === "delete") dels.push(ops[i]!.line); + else ins.push(ops[i]!.line); + i += 1; + } + const paired = Math.min(dels.length, ins.length); + for (let k = 0; k < paired; k += 1) rewrites.push({ before: dels[k]!, after: ins[k]! }); + for (let k = paired; k < dels.length; k += 1) removals.push(dels[k]!); + } + return { removals, rewrites }; +} + +/** + * Classify a refresh's published playbook against the project copy it replaces. + * Purely additive means only insertions and the front matter untouched apart + * from the `template:` stamp: a removed or rewritten body line, or any other + * front-matter change (and a change to `models`, `may`, `requeue-default` or a + * gate in particular), is not additive. + */ +export function classifyRefresh(before: string, after: string): RefreshClassification { + const b = splitFrontMatter(before); + const a = splitFrontMatter(after); + const reasons: string[] = []; + if (b.front === null || a.front === null) { + if (b.front !== a.front) reasons.push("the front matter is missing or added"); + } else { + const bf = frontMatterLines(b.front); + const af = frontMatterLines(a.front); + for (const key of new Set([...bf.keys(), ...af.keys()])) { + if (key === "template") continue; + if (bf.get(key) !== af.get(key)) { + const never = NEVER_ADDITIVE_KEYS.has(key) || key.includes("gate"); + reasons.push(never ? `\`${key}\` changed, and a change to it is never additive` : `front matter \`${key}\` changed`); + } + } + } + const { removals, rewrites } = movedLines(diffOps(b.body.split("\n"), a.body.split("\n"))); + const substantiveRemovals = removals.filter((l) => l.trim() !== ""); + const substantiveRewrites = rewrites.filter((r) => r.before.trim() !== "" || r.after.trim() !== ""); + const additive = reasons.length === 0 && substantiveRemovals.length === 0 && substantiveRewrites.length === 0; + return { + classification: additive ? "additive" : "not-additive", + removals: substantiveRemovals, + rewrites: substantiveRewrites, + reasons, + }; +} + +/** + * The pull request body the plugin opens for a refresh. The classification is + * the plugin's own read of the diff; the refresh thread's own summary is kept + * beside it as its claim. The project steward reads the diff and decides, and + * the removals or rewrites are listed so that decision starts from what + * actually moved. + */ +export function refreshPrBody( + refresh: Pick, + c: RefreshClassification, + threadId: string, + claim?: string | null, +): string { + const path = `stewards/playbooks/${refresh.playbook}.md`; + const lines = [ + `Refreshes \`${path}\` from template \`${refresh.templateName}\` (stamp \`${refresh.fromHash}\` → \`${refresh.targetHash}\`).`, + "", + "The refresh worker merged the current shipped template into the project copy, keeping the project's own additions, and updated the stamp. This is a reviewed change, never a mechanical overwrite.", + "", + "## Classification", + c.classification === "additive" ? "**Purely additive** — only insertions, and the front matter is untouched apart from the stamp." : "**Not purely additive** — the merge removes or rewrites something the project copy carried.", + ]; + if (claim !== undefined && claim !== null && claim.trim() !== "") lines.push("", "The refresh thread's own summary (its claim, not the decision):", "", "```", claim.trim(), "```"); + if (c.reasons.length > 0) lines.push("", "Front matter:", ...c.reasons.map((r) => `- ${r}`)); + if (c.removals.length > 0) lines.push("", "Removed lines:", ...c.removals.map((l) => `- \`${l}\``)); + if (c.rewrites.length > 0) { + lines.push("", "Rewritten lines:"); + for (const r of c.rewrites) lines.push(`- \`${r.before}\` → \`${r.after}\``); + } + if (c.removals.length === 0 && c.rewrites.length === 0 && c.reasons.length === 0) lines.push("", "Nothing was removed or rewritten."); + lines.push("", `_— from bb thread ${threadId}_`); + return lines.join("\n"); +} + +/** + * Whether a playbook text carries the given `template: @` stamp, + * so a merge never lands a copy whose stamp the refresh did not update. + */ +export function hasTemplateStamp(text: string, name: string, hash: string): boolean { + const escaped = name.replace(/[.*+?^${}()|[\]\\]/gu, "\\$&"); + return new RegExp(`^template:\\s*${escaped}@${hash}\\s*$`, "mu").test(text); +} diff --git a/plugins/stewards/server.test.ts b/plugins/stewards/server.test.ts index 54d74ba8..6df73462 100644 --- a/plugins/stewards/server.test.ts +++ b/plugins/stewards/server.test.ts @@ -17026,3 +17026,98 @@ describe("template stamps and the nightly live check (BB-328)", () => { expect(t.hooks.store.checkFindings("p1").some((f) => f.text.includes("ENG has no state Todo"))).toBe(true); }); }); + +// BB-328 Part 2: the nightly check queues a refresh for a lagging playbook, the +// plugin spawns a short-lived worker and opens the reviewed pull request, and +// the project steward reads the diff and decides. The queue is one per +// playbook, a declined refresh is not retried until the template changes, and +// the stamp is updated only by the merged refresh. +describe("playbook refreshes (BB-328 Part 2)", () => { + const laggingCopy = () => LINEAR_TICKETS.replace("name: linear-tickets", "name: linear-tickets\ntemplate: linear-tickets@deadbeef"); + + /** A setup whose project copy of linear-tickets carries an old stamp. */ + const withLaggingPlaybook = async () => { + const t = await setup({ timing: { debounceMs: 1, nightlyStaggerMs: 0, configTtlMs: 0 } }); + t.files[playbookPath("linear-tickets")] = laggingCopy(); + return t; + }; + + /** Simulate the worker publishing its merged copy, then finishing its turn. */ + const publishAndFinish = async (t: Awaited>) => { + const refresh = t.hooks.store.playbookRefreshes("p1")[0]!; + const tid = refresh.threadId!; + const branch = `guidance-${tid}`; + const merged = laggingCopy().replace("template: linear-tickets@deadbeef", `template: linear-tickets@${templateHash(LINEAR_TICKETS)}`) + "\nA new template rule.\n"; + t.githubFileContents.set(`acme/bb#${branch}:.bb/${playbookPath("linear-tickets")}`, merged); + await t.idle(tid, "additive"); + return t.hooks.store.playbookRefreshes("p1")[0]!; + }; + + it("queues one refresh per lagging playbook and spawns a short-lived worker", async () => { + const t = await withLaggingPlaybook(); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + const rows = t.hooks.store.playbookRefreshes("p1"); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ playbook: "linear-tickets", templateName: "linear-tickets", fromHash: "deadbeef", targetHash: templateHash(LINEAR_TICKETS), state: "spawned" }); + expect(rows[0]!.threadId).not.toBeNull(); + const spawn = t.spawns.find((s) => (s.pluginMetadata as { role?: string } | undefined)?.role === "refresh"); + expect(spawn).toBeDefined(); + // The fixed brief carries the project copy, the template name and the hash. + expect(String(spawn!.prompt)).toContain("template: linear-tickets@deadbeef"); + expect(String(spawn!.prompt)).toContain("current template hash is"); + }); + + it("keeps one open refresh per playbook: a second nightly pass queues nothing", async () => { + const t = await withLaggingPlaybook(); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + expect(t.hooks.store.playbookRefreshes("p1")).toHaveLength(1); + }); + + it("opens the reviewed pull request when the worker publishes, and leaves the stamp alone until it merges", async () => { + const t = await withLaggingPlaybook(); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + const refresh = await publishAndFinish(t); + expect(refresh.state).toBe("pr_open"); + expect(refresh.classification).toBe("additive"); + expect(refresh.pr).toMatch(/^https:\/\/github\.com\/acme\/bb\/pull\/\d+$/u); + expect(t.githubPulls).toHaveLength(1); + expect(t.githubPulls[0]!.body).toContain("Purely additive"); + // Stamp-only-on-merge: the plugin never rewrites the project copy. + expect(t.files[playbookPath("linear-tickets")]).toContain("template: linear-tickets@deadbeef"); + }); + + it("marks a refresh merged only when the project steward merges it, and then never re-queues that target", async () => { + const t = await withLaggingPlaybook(); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + const refresh = await publishAndFinish(t); + const steward = t.hooks.store.project("p1").threadId!; + const reply = await t.tool("steward_update", { refreshAction: { id: refresh.id, action: "merge" } }, steward); + expect(reply.error).toBe(false); + expect(t.hooks.store.playbookRefreshes("p1")[0]!.state).toBe("merged"); + expect(t.githubMerges).toHaveLength(1); + // The fixture still shows the old stamp, but a merged target is not retried. + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + expect(t.hooks.store.playbookRefreshes("p1")).toHaveLength(1); + }); + + it("does not retry a declined refresh until the template changes", async () => { + const t = await withLaggingPlaybook(); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + const refresh = await publishAndFinish(t); + const steward = t.hooks.store.project("p1").threadId!; + const dropped = await t.tool("steward_update", { refreshAction: { id: refresh.id, action: "drop" } }, steward); + expect(dropped.error).toBe(false); + expect(t.hooks.store.playbookRefreshes("p1")[0]!.state).toBe("declined"); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + expect(t.hooks.store.playbookRefreshes("p1")).toHaveLength(1); + }); +}); diff --git a/plugins/stewards/server.ts b/plugins/stewards/server.ts index 40b70b97..b9aa8040 100644 --- a/plugins/stewards/server.ts +++ b/plugins/stewards/server.ts @@ -128,8 +128,9 @@ import { } from "./source-contract.js"; import { DEPLOY_STATES, allDeployed, applyDeployment, applyDeploymentRead, applyWorkflowRun, awaitingOnRepo, deploymentLines, deploymentReportSchema, isFinalDeploymentState, matchesTarget, matchesWorkflow, mergedPulls, pickWatch, recordMerged, reportDeployment, targetKeyOf, timeOut, timeoutsDue } from "./deploy.js"; import { evaluateCarries, liveAtMergeVerdict, matchCarriesRule, noRuleNoted, type CarriesReader } from "./carries.js"; -import { ACTORS, MIGRATIONS, ESCALATED_NEED_KINDS, NEEDS_KINDS, RESOLUTION_KINDS, createStore, needChoices, newId, type Assumption, type Binding, type DepsDecision, type DivergenceRow, type Escalation, type Fingerprint, type StandupNoteRow, type StandupRow, type StandupTranscriptEntry, type Lesson, type Need, type Observed, type Question, type ReadyState, type Store, type WaitDivergenceRow, type WakeRetry, type Worker } from "./store.js"; +import { ACTORS, MIGRATIONS, ESCALATED_NEED_KINDS, NEEDS_KINDS, RESOLUTION_KINDS, createStore, needChoices, newId, type Assumption, type Binding, type DepsDecision, type DivergenceRow, type Escalation, type Fingerprint, type PlaybookRefresh, type StandupNoteRow, type StandupRow, type StandupTranscriptEntry, type Lesson, type Need, type Observed, type Question, type ReadyState, type Store, type WaitDivergenceRow, type WakeRetry, type Worker } from "./store.js"; import { hasLessonAnchor, landLessonInstruction, lessonAnchor, lessonLandingMark, planLessonTransitions, planValidations, retiredLessonAnchor, withLessonAnchor } from "./lessons.js"; +import { classifyRefresh, hasTemplateStamp, laggingPlaybooks, refreshPrBody, refreshQueueDecision, type LaggingPlaybook } from "./refresh.js"; import { AFTER_THE_FACT_RATIONALE_WHY, REPEAT_RATIONALE_WHY, ROUTINE_RATIONALE_WHY, clusterKey, routineRationaleWanted } from "./rationale.js"; import { countingDatabase, countingGithub, createReadCounts, type ReadCounts } from "./read-counts.js"; import { GUIDANCE_UNIT_PREFIX, GUIDANCE_UNCITED_DAYS, escalationWeeks, parseGuidanceUnits, uncitedGuidanceUnits, type GuidanceUnit } from "./guidance.js"; @@ -1342,6 +1343,40 @@ export function requesterPrompt(b: Pick, need: Pick, projectCopy: string, template: string): string { + return [ + `You are a one-task playbook-refresh thread started by the stewards plugin for the playbook ${refresh.playbook}. Do only this; do not explore the workspace beyond the files named, do not open or merge a pull request, and do not ask a question.`, + "", + `The project copy \`.bb/stewards/playbooks/${refresh.playbook}.md\` was copied from the shipped template \`${refresh.templateName}\` and has drifted: its stamp is \`${refresh.fromHash}\`, and the current template hash is \`${refresh.targetHash}\`.`, + "", + "Merge the current shipped template into the project copy, keeping the project's own additions. A project addition that contradicts the template stays; note the contradiction in your final reply. Never drop a project-specific rule to match the template.", + "", + "The current project copy:", + "", + "```md", + projectCopy.replace(/```/gu, "'''"), + "```", + "", + `The current shipped template \`${refresh.templateName}\`:`, + "", + "```md", + template.replace(/```/gu, "'''"), + "```", + "", + "Steps:", + `1. Edit \`.bb/stewards/playbooks/${refresh.playbook}.md\` so it is the current template merged with the project's own additions, and set its front-matter stamp line to \`template: ${refresh.templateName}@${refresh.targetHash}\`.`, + "2. Publish the `.bb` change with `workspace_publish_guidance` (it pushes the `guidance-` branch). Do not open a pull request; the plugin does that.", + "3. Reply with one short summary: whether the merge is purely additive (only insertions, and no front-matter change beyond the stamp) or not, and every template rule you dropped or rewrote because the project's own text contradicts it.", + ].join("\n"); +} + export const STEWARDS_SETTINGS = { paused: { type: "boolean", @@ -7335,7 +7370,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const questions = store.openQuestions(projectId).map((question) => ({ question, about: question.bindingId === null ? null : store.binding(question.bindingId) })); const needs = open.flatMap((b) => store.openNeeds(b.id).map((need) => ({ need, binding: b }))); const assumptions = store.openAssumptionsForProject(projectId).map((assumption) => ({ assumption, binding: assumption.bindingId === null ? null : (store.binding(assumption.bindingId) ?? null) })); - const text = formatProjectWake({ reason, bindings, routed, withUser, problems: config.problems, digestDue: opts.digestDue, notes: opts.notes ?? [], bigDecisions: config.file.bigDecisions, standingRules: config.file.standingRules, questions, needs, assumptions, lessons: store.allLessons(projectId), guidance: await wakeGuidance(projectId, null), lessonRules: config.file.lessons, timeZone: config.file.nightly.timezone, label: config.file.label, ...(records === undefined ? {} : { records }) }); + const text = formatProjectWake({ reason, bindings, routed, withUser, problems: config.problems, digestDue: opts.digestDue, notes: opts.notes ?? [], bigDecisions: config.file.bigDecisions, standingRules: config.file.standingRules, questions, needs, assumptions, lessons: store.allLessons(projectId), refreshes: store.playbookRefreshes(projectId), guidance: await wakeGuidance(projectId, null), lessonRules: config.file.lessons, timeZone: config.file.nightly.timezone, label: config.file.label, ...(records === undefined ? {} : { records }) }); const delivered = { ids: routed.filter((r) => r.escalation.deliveredAt === null).map((r) => r.escalation.id), at: 0 }; delivered.at = store.markDelivered(delivered.ids); // A wake can be the thing that pushes the transcript over the limit. @@ -8889,6 +8924,239 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P return x; } + // ---- playbook refreshes (BB-328 Part 2) ----------------------------------------------------- + // + // The nightly check reports a stamped playbook whose hash lags its shipped + // template. The plugin queues one refresh per playbook, spawns a short-lived + // worker to merge the template into the project copy and publish, and opens + // a reviewed pull request from the published branch. The plugin classifies + // the diff for the pull request body; the project steward reads the diff and + // decides (`steward_update { refreshAction }`). The stamp is only updated by + // the merged pull request, so the playbook stops being queued only then. + + /** The playbooks a project loaded, deduplicated by name, for the lag read. */ + function loadedPlaybooks(config: ProjectConfig): Playbook[] { + const out: Playbook[] = []; + const seen = new Set(); + for (const p of [...Object.values(config.playbooks), config.depsAlerts, config.depsPrs, ...config.watchPlaybooks.values()]) { + if (p === undefined || p === null || seen.has(p.name)) continue; + seen.add(p.name); + out.push(p); + } + return out; + } + + /** Queue and start a refresh for every playbook that lags and may be queued. */ + async function queuePlaybookRefreshes(config: ProjectConfig): Promise { + const lags = laggingPlaybooks(loadedPlaybooks(config), templateHashes()); + if (lags.length === 0) return; + const prior = store.playbookRefreshes(config.projectId); + for (const lag of lags) { + const decision = refreshQueueDecision(prior, lag); + if (!decision.queue) continue; + const refresh = store.addPlaybookRefresh({ + projectId: config.projectId, + file: playbookPath(lag.name), + playbook: lag.name, + templateName: lag.templateName, + fromHash: lag.fromHash, + targetHash: lag.toHash, + }); + prior.push(refresh); + await spawnPlaybookRefresh(config, refresh); + } + } + + /** Spawn the fixed-prompt worker for a queued refresh, or mark it failed. */ + async function spawnPlaybookRefresh(config: ProjectConfig, refresh: PlaybookRefresh): Promise { + const template = Object.hasOwn(PLAYBOOK_TEMPLATES, refresh.templateName) ? PLAYBOOK_TEMPLATES[refresh.templateName]! : null; + const projectCopy = (await config.source.readFiles([refresh.file]).catch(() => new Map())).get(refresh.file) ?? null; + if (template === null || projectCopy === null) { + store.setPlaybookRefresh(refresh.id, { + state: "failed", + note: template === null ? `the template ${refresh.templateName} no longer exists` : `the project copy ${refresh.file} is unreadable`, + }); + return; + } + const prompt = refreshPrompt(refresh, projectCopy, template); + await recordPluginSend(prompt); + try { + const thread = await bb.sdk.threads.spawn({ + projectId: config.projectId, + environment: freshEnvironment(config.source, await projectHost(config.projectId)), + prompt, + title: `Refresh ${refresh.playbook} playbook`, + ...modelArgs(config.file.projectSteward.model ?? null), + pluginMetadata: { role: "refresh", refreshId: refresh.id }, + }); + store.setPlaybookRefresh(refresh.id, { state: "spawned", threadId: thread.id, branch: lessonBranchFor(thread.id) }); + bb.log.info(`stewards: started a playbook-refresh thread for ${refresh.playbook} (${thread.id})`); + } catch (error) { + store.setPlaybookRefresh(refresh.id, { state: "failed", note: `could not start the refresh thread: ${errorMessage(error)}` }); + } + } + + /** A refresh that failed before a pull request: record it and tell the project steward once. */ + async function failPlaybookRefresh(config: ProjectConfig | null, refresh: PlaybookRefresh, note: string): Promise { + store.setPlaybookRefresh(refresh.id, { state: "failed", note }); + if (config === null) return; + await wakeProject(config.projectId, `playbook refresh ${refresh.playbook} could not start`, { + digestDue: false, + notes: [`The refresh of \`${refresh.file}\` from template \`${refresh.templateName}\` failed: ${note}. The playbook keeps its old stamp and is not retried until the template changes.`], + }); + } + + /** The refresh worker finished its turn: read its published branch and open the reviewed pull request. */ + async function finishPlaybookRefresh(threadId: string, claim: string | null): Promise { + const refresh = store.playbookRefreshByThread(threadId); + if (refresh === null || refresh.state !== "spawned") return; + const config = await loadConfig(refresh.projectId); + const branch = refresh.branch ?? lessonBranchFor(threadId); + if (config === null) { + await failPlaybookRefresh(null, refresh, "the project configuration is unreadable"); + return; + } + const primary = await lessonRepo(refresh.projectId); + if ("error" in primary) { + await failPlaybookRefresh(config, refresh, primary.error); + return; + } + const path = `.bb/${refresh.file}`; + const head = branch.replace(/^refs\/heads\//u, ""); + let published: string | null = null; + if (github.fileContent !== undefined) { + try { + published = await github.fileContent(primary.repo, path, head); + } catch (error) { + bb.log.warn(`stewards: could not read the refresh branch ${head}: ${errorMessage(error)}`); + } + } + if (published === null) { + await failPlaybookRefresh(config, refresh, `the refresh branch ${head} was not published`); + return; + } + // The stamp-only-on-merge rule: the merged copy must carry the target + // stamp, or the merge would land a copy the queue still reads as lagging. + if (!hasTemplateStamp(published, refresh.templateName, refresh.targetHash)) { + await failPlaybookRefresh(config, refresh, `the published copy does not carry the stamp ${refresh.templateName}@${refresh.targetHash}`); + return; + } + const before = (await config.source.readFiles([refresh.file]).catch(() => new Map())).get(refresh.file) ?? null; + if (before === null) { + await failPlaybookRefresh(config, refresh, `the project copy ${refresh.file} is unreadable`); + return; + } + const classified = classifyRefresh(before, published); + const base = await github.defaultBranch(primary.repo).catch((error: unknown) => { + bb.log.warn(`stewards: could not read ${primary.repo}'s default branch: ${errorMessage(error)}`); + return null; + }); + if (base === null) { + await failPlaybookRefresh(config, refresh, `could not read ${primary.repo}'s default branch`); + return; + } + try { + const created = await github.createPull(primary.repo, { head, base, title: `Refresh ${refresh.playbook} playbook`, body: refreshPrBody(refresh, classified, threadId, claim) }); + const removals = [...classified.removals, ...classified.rewrites.map((r) => `${r.before} → ${r.after}`)]; + store.setPlaybookRefresh(refresh.id, { state: "pr_open", pr: created.url, branch, classification: classified.classification, removals }); + await wakeProject(config.projectId, `playbook refresh ${refresh.playbook} is ready to review`, { + digestDue: false, + notes: [ + `Refresh ${refresh.id} for \`${refresh.file}\` is open at ${created.url} (${classified.classification === "additive" ? "purely additive" : "not purely additive"}). Read the diff and decide: \`steward_update { refreshAction: { id: "${refresh.id}", action: "merge" } }\` merges a purely additive, project-lane change; \`action: "forward"\` files the owner's Review PR/Drop item; \`action: "drop"\` closes it and declines it (not retried until the template changes).`, + ], + }); + } catch (error) { + await failPlaybookRefresh(config, refresh, errorMessage(error)); + } + } + + /** The refresh worker failed: record it, so it is not retried until the template changes. */ + async function failPlaybookRefreshThread(threadId: string, note: string): Promise { + const refresh = store.playbookRefreshByThread(threadId); + if (refresh === null || refresh.state !== "spawned") return; + await failPlaybookRefresh(await loadConfig(refresh.projectId), refresh, note); + } + + /** Merge, forward to the owner, or drop a refresh the project steward decided. */ + async function actOnPlaybookRefresh(refresh: PlaybookRefresh, action: "merge" | "forward" | "drop"): Promise { + if (action === "merge") { + if (refresh.state !== "pr_open" || refresh.pr === null) return "there is no open pull request to merge"; + const ref = parsePullRef(refresh.pr); + if (ref === null) return `the pull request ${refresh.pr} cannot be read`; + const primary = await lessonRepo(refresh.projectId); + if ("error" in primary) return primary.error; + // The narrow guard: the pull request changes this playbook and only it. + if (github.files !== undefined) { + const files = await github.files(primary.repo, ref.number).catch(() => null); + if (files === null) return "the pull request's changed files cannot be read"; + const bad = files.filter((p) => p !== `.bb/${refresh.file}` && p !== refresh.file); + if (bad.length > 0) return `the pull request changes files other than ${refresh.file}: ${bad.slice(0, 5).join(", ")}`; + } + try { + const { sha } = await github.squashMerge(primary.repo, ref.number, `Refresh ${refresh.playbook} playbook`, "Merged by the project steward after reading the refresh diff."); + store.setPlaybookRefresh(refresh.id, { state: "merged", note: `merged at ${sha ?? "?"}` }); + if (refresh.questionId !== null) { + const q = store.question(refresh.questionId); + if (q !== null && q.status === "open") store.answer(refresh.questionId, "The refresh pull request merged.", "plugin"); + } + return `merged at ${sha ?? refresh.pr}`; + } catch (error) { + store.setPlaybookRefresh(refresh.id, { note: errorMessage(error) }); + return `pull request refused: ${errorMessage(error)}`; + } + } + if (action === "drop") { + if (refresh.pr !== null) { + const ref = parsePullRef(refresh.pr); + if (ref !== null) await github.closePull(ref.repo, ref.number).catch((error: unknown) => bb.log.warn(`stewards: could not close ${refresh.pr}: ${errorMessage(error)}`)); + } + store.setPlaybookRefresh(refresh.id, { state: "declined", note: "Dropped by the project steward." }); + if (refresh.questionId !== null) { + const q = store.question(refresh.questionId); + if (q !== null && q.status === "open") store.answer(refresh.questionId, "Dropped.", "project"); + } + return "dropped; not retried until the template changes"; + } + // forward: the owner reviews the pull request; the plugin observes the merge. + if (refresh.state !== "pr_open") return "there is no open pull request to forward"; + if (refresh.questionId !== null) return `already with the owner (${refresh.questionId})`; + const question = store.ask({ + projectId: refresh.projectId, + bindingId: null, + escalationId: null, + question: `Refresh the ${refresh.playbook} playbook from template ${refresh.templateName}?`, + context: `The refresh pull request ${refresh.pr ?? "(none)"} is ${refresh.classification === "additive" ? "purely additive" : "not purely additive"}${refresh.removals.length === 0 ? "" : `; it removes or rewrites ${refresh.removals.length} line(s)`}. Review the diff at the pull request and merge it, or drop it.`, + choices: ["Review PR", "Drop"], + url: refresh.pr, + data: { refreshId: refresh.id, file: refresh.file }, + }); + store.setPlaybookRefresh(refresh.id, { questionId: question.id, note: "forwarded to the owner" }); + return `forwarded to the owner as ${question.id}`; + } + + /** A refresh pull request merged or closed: move the row without the project steward acting. */ + async function reconcilePlaybookRefreshes(config: ProjectConfig): Promise { + for (const refresh of store.playbookRefreshes(config.projectId)) { + if (refresh.state !== "pr_open" || refresh.pr === null) continue; + const ref = parsePullRef(refresh.pr); + if (ref === null) continue; + try { + const pull = await snapshot("github", `${ref.repo}#${ref.number}`); + if (pull?.pull?.merged === true) { + store.setPlaybookRefresh(refresh.id, { state: "merged", note: "The pull request merged." }); + if (refresh.questionId !== null) { + const q = store.question(refresh.questionId); + if (q !== null && q.status === "open") store.answer(refresh.questionId, "The refresh pull request merged.", "plugin"); + } + } else if (pull !== null && (pull.state.toLowerCase() === "closed" || pull.state.toLowerCase() === "declined")) { + store.setPlaybookRefresh(refresh.id, { state: "declined", note: "The pull request closed without merging." }); + } + } catch (error) { + bb.log.warn(`stewards: could not read refresh ${refresh.id}'s pull request ${refresh.pr}: ${errorMessage(error)}`); + } + } + } + // ---- thread events --------------------------------------------------------------------------- // // Workers are started by stewards with `--parent-self @@ -9014,6 +9282,14 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P })); bb.events.on("thread.idle", store.coalescingTool(async ({ thread, lastAssistantText }) => { + // BB-328 Part 2: a playbook-refresh worker finished its turn. Read the + // branch it published, classify the diff and open the reviewed pull + // request; the project steward reads the diff and decides. + const refreshThread = store.playbookRefreshByThread(thread.id); + if (refreshThread !== null && refreshThread.state === "spawned") { + await finishPlaybookRefresh(thread.id, lastAssistantText); + return; + } const worker = store.worker(thread.id); if (worker !== null) { if (worker.status === "stopped") return; @@ -9262,6 +9538,13 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P })); bb.events.on("thread.failed", store.coalescingTool(async ({ thread, error }) => { + // BB-328 Part 2: a refresh worker that failed is recorded, so it is not + // retried until the template changes again. + const refreshThread = store.playbookRefreshByThread(thread.id); + if (refreshThread !== null && refreshThread.state === "spawned") { + await failPlaybookRefreshThread(thread.id, error ?? "the refresh worker failed"); + return; + } const worker = store.worker(thread.id); if (worker === null || !workerInFlight(worker)) return; // BB-255: turn.failed is dispatched right after this event and decides @@ -12144,6 +12427,13 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P }) .optional() .describe("Project steward only: finish a pending lesson after applying and publishing it, retire a retired one, or drop it."), + refreshAction: z + .object({ + id: z.string().trim().min(1).max(100).describe("The refresh id (rfr_…) from the `## Playbooks to refresh` wake block."), + action: z.enum(["merge", "forward", "drop"]).describe("merge: squash-merge the refresh pull request (a purely additive, project-lane change). forward: file the owner's Review PR/Drop item. drop: close the pull request and decline the refresh, which is not retried until the template changes."), + }) + .optional() + .describe("Project steward only: decide a playbook refresh after reading its diff (BB-328 Part 2). The plugin's `purely additive` label is only its read of the diff; the project steward decides."), }), execute: store.coalescingTool(async (input, { threadId }) => { try { @@ -12208,8 +12498,8 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P if (projectOf === null && (input.ask !== undefined || resolve?.startsWith("qst_") === true)) { return toolError(new Error("Only the project steward keeps questions to the user; a steward escalates with steward_report.")); } - if (projectOf === null && (input.lesson !== undefined || input.lessonAction !== undefined)) { - return toolError(new Error("Only the project steward records and completes lessons; a steward escalates with steward_report.")); + if (projectOf === null && (input.lesson !== undefined || input.lessonAction !== undefined || input.refreshAction !== undefined)) { + return toolError(new Error("Only the project steward records and completes lessons and decides playbook refreshes; a steward escalates with steward_report.")); } if (input.lesson?.supersedes !== undefined) { const older = store.lesson(input.lesson.supersedes); @@ -12230,6 +12520,12 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const outcome = await completeLesson(lesson, input.lessonAction.action); return `${lesson.id}: ${outcome}.`; } + if (input.refreshAction !== undefined) { + const refresh = store.playbookRefresh(input.refreshAction.id); + if (refresh === null || refresh.projectId !== projectOf) return toolError(new Error(`No playbook refresh ${input.refreshAction.id} in this project.`)); + const outcome = await actOnPlaybookRefresh(refresh, input.refreshAction.action); + return `${refresh.id}: ${outcome}.`; + } /** Record the lesson this call carries, with the binding and escalation it came from. */ const recordLesson = (bindingId: string | null, escalationId: string | null): string => { const projectThread = store.project(projectOf!).threadId; @@ -13074,6 +13370,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P await keepProjectSteward(config.projectId); await syncOpsBindings(config); await reconcileLessons(config); + await reconcilePlaybookRefreshes(config); applyLessonTransitions(config); applyRationaleClustering(config); // BB-325: close any open item left on an ended binding, once. @@ -13165,6 +13462,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // role does not pay for the extra snapshots. const adoption = (await settings.get()).paused ? { adopted: 0, unassigned: [] } : await adoptionPass(config, { collectUnassigned: config.role?.projectSteward === true }); await reconcileLessons(config); + await reconcilePlaybookRefreshes(config); applyLessonTransitions(config); applyValidations(config); applyRationaleClustering(config); @@ -13212,6 +13510,11 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const diff = freshCheckFindings(current, store.checkFindings(config.projectId)); freshFindings = diff.fresh; nextFindings = diff.next; + // BB-328 Part 2: queue a refresh for every playbook that lags its + // shipped template and is not already open or deferred. The + // worker is short-lived; the project steward is woken when the + // reviewed pull request is ready. + await queuePlaybookRefreshes(config); } let digestSent = false; await wakeProject(config.projectId, `nightly pass ${night}`, { diff --git a/plugins/stewards/store.test.ts b/plugins/stewards/store.test.ts index 0f5d608a..4e99f688 100644 --- a/plugins/stewards/store.test.ts +++ b/plugins/stewards/store.test.ts @@ -1,7 +1,7 @@ import Database from "better-sqlite3"; import { describe, expect, it } from "vitest"; import { z } from "zod"; -import { ACTOR_MIGRATIONS, ALERTMANAGER_LOOK_MIGRATIONS, BACKLOG_LOOK_MIGRATIONS, BACKLOG_SNAPSHOT_MIGRATIONS, CHECK_FINDING_MIGRATIONS, HUDDLE_MIGRATIONS, NON_ACTION_MIGRATIONS, TYPED_WAIT_MIGRATIONS, WAIT_DIVERGENCE_MIGRATIONS, APPROVAL_MARKER_MIGRATIONS, ASSESSMENT_MIGRATIONS, ASSUMPTION_MIGRATIONS, BASE_MIGRATIONS, CHAIN_MIGRATIONS, DATED_WAIT_MIGRATIONS, DEPS_DECISION_MIGRATIONS, ENDED_READ_MIGRATIONS, ESCALATION_ROUTE_MIGRATIONS, GUIDANCE_MIGRATIONS, IDLE_MIGRATIONS, LESSON_COUNTER_MIGRATIONS, LESSON_PR_MIGRATIONS, LESSON_TIER_MIGRATIONS, MERGE_BEHIND_MIGRATIONS, MIGRATIONS, PULL_REQUESTER_MIGRATIONS, PULL_WATCH_MIGRATIONS, RATIONALE_MIGRATIONS, RETARGET_PULLS_MIGRATIONS, REVIEWER_MODEL_MIGRATIONS, REVIEWER_REQUEST_MIGRATIONS, SERVER_SETTINGS_MIGRATIONS, STACK_MIGRATIONS, STACKED_PULLS_MIGRATIONS, VALIDATION_MIGRATIONS, WAKE_RETRY_MIGRATIONS, WAKE_SOURCE_MIGRATIONS, WORKER_CHECK_MIGRATIONS, STANDUP_MIGRATIONS, STANDUP_BINDING_MIGRATIONS, STANDUP_DELEGATION_MIGRATIONS, STANDUP_COMPLETED_MIGRATIONS, WORKER_STOP_REASON_MIGRATIONS, WORKER_PARK_MIGRATIONS, createStore, lessonScope, statusRecordSchema, type Binding } from "./store.js"; +import { ACTOR_MIGRATIONS, ALERTMANAGER_LOOK_MIGRATIONS, BACKLOG_LOOK_MIGRATIONS, BACKLOG_SNAPSHOT_MIGRATIONS, CHECK_FINDING_MIGRATIONS, HUDDLE_MIGRATIONS, NON_ACTION_MIGRATIONS, PLAYBOOK_REFRESH_MIGRATIONS, TYPED_WAIT_MIGRATIONS, WAIT_DIVERGENCE_MIGRATIONS, APPROVAL_MARKER_MIGRATIONS, ASSESSMENT_MIGRATIONS, ASSUMPTION_MIGRATIONS, BASE_MIGRATIONS, CHAIN_MIGRATIONS, DATED_WAIT_MIGRATIONS, DEPS_DECISION_MIGRATIONS, ENDED_READ_MIGRATIONS, ESCALATION_ROUTE_MIGRATIONS, GUIDANCE_MIGRATIONS, IDLE_MIGRATIONS, LESSON_COUNTER_MIGRATIONS, LESSON_PR_MIGRATIONS, LESSON_TIER_MIGRATIONS, MERGE_BEHIND_MIGRATIONS, MIGRATIONS, PULL_REQUESTER_MIGRATIONS, PULL_WATCH_MIGRATIONS, RATIONALE_MIGRATIONS, RETARGET_PULLS_MIGRATIONS, REVIEWER_MODEL_MIGRATIONS, REVIEWER_REQUEST_MIGRATIONS, SERVER_SETTINGS_MIGRATIONS, STACK_MIGRATIONS, STACKED_PULLS_MIGRATIONS, VALIDATION_MIGRATIONS, WAKE_RETRY_MIGRATIONS, WAKE_SOURCE_MIGRATIONS, WORKER_CHECK_MIGRATIONS, STANDUP_MIGRATIONS, STANDUP_BINDING_MIGRATIONS, STANDUP_DELEGATION_MIGRATIONS, STANDUP_COMPLETED_MIGRATIONS, WORKER_STOP_REASON_MIGRATIONS, WORKER_PARK_MIGRATIONS, createStore, lessonScope, statusRecordSchema, type Binding } from "./store.js"; /** MIGRATIONS without the groups appended after the ones these order tests assert. */ const beforeActorMigrations = MIGRATIONS.slice(0, MIGRATIONS.indexOf(ACTOR_MIGRATIONS[0]!)); @@ -746,8 +746,8 @@ describe("the actor rename migration (BB-97)", () => { // and BB-328 appended the check-findings table after that. BB-334's // snapshot column followed, BB-333's typed wait column after that, and // BB-335's non-action column after that. BB-333 PR 2's wait-divergence - // table is the newest. - const groups = [WAIT_DIVERGENCE_MIGRATIONS, NON_ACTION_MIGRATIONS, TYPED_WAIT_MIGRATIONS, BACKLOG_SNAPSHOT_MIGRATIONS, CHECK_FINDING_MIGRATIONS, BACKLOG_LOOK_MIGRATIONS, WORKER_PARK_MIGRATIONS, REVIEWER_REQUEST_MIGRATIONS, PULL_REQUESTER_MIGRATIONS, ALERTMANAGER_LOOK_MIGRATIONS, STANDUP_COMPLETED_MIGRATIONS, STANDUP_DELEGATION_MIGRATIONS, STANDUP_BINDING_MIGRATIONS, STANDUP_MIGRATIONS, HUDDLE_MIGRATIONS, ENDED_READ_MIGRATIONS, ASSESSMENT_MIGRATIONS, STACKED_PULLS_MIGRATIONS, RETARGET_PULLS_MIGRATIONS, MERGE_BEHIND_MIGRATIONS, WAKE_RETRY_MIGRATIONS, DATED_WAIT_MIGRATIONS, ASSUMPTION_MIGRATIONS, WORKER_STOP_REASON_MIGRATIONS, DEPS_DECISION_MIGRATIONS, ESCALATION_ROUTE_MIGRATIONS, STACK_MIGRATIONS, PULL_WATCH_MIGRATIONS, SERVER_SETTINGS_MIGRATIONS, WAKE_SOURCE_MIGRATIONS, REVIEWER_MODEL_MIGRATIONS, LESSON_PR_MIGRATIONS, ACTOR_MIGRATIONS, APPROVAL_MARKER_MIGRATIONS]; + // table followed, and BB-328 Part 2's playbook-refresh table is the newest. + const groups = [PLAYBOOK_REFRESH_MIGRATIONS, WAIT_DIVERGENCE_MIGRATIONS, NON_ACTION_MIGRATIONS, TYPED_WAIT_MIGRATIONS, BACKLOG_SNAPSHOT_MIGRATIONS, CHECK_FINDING_MIGRATIONS, BACKLOG_LOOK_MIGRATIONS, WORKER_PARK_MIGRATIONS, REVIEWER_REQUEST_MIGRATIONS, PULL_REQUESTER_MIGRATIONS, ALERTMANAGER_LOOK_MIGRATIONS, STANDUP_COMPLETED_MIGRATIONS, STANDUP_DELEGATION_MIGRATIONS, STANDUP_BINDING_MIGRATIONS, STANDUP_MIGRATIONS, HUDDLE_MIGRATIONS, ENDED_READ_MIGRATIONS, ASSESSMENT_MIGRATIONS, STACKED_PULLS_MIGRATIONS, RETARGET_PULLS_MIGRATIONS, MERGE_BEHIND_MIGRATIONS, WAKE_RETRY_MIGRATIONS, DATED_WAIT_MIGRATIONS, ASSUMPTION_MIGRATIONS, WORKER_STOP_REASON_MIGRATIONS, DEPS_DECISION_MIGRATIONS, ESCALATION_ROUTE_MIGRATIONS, STACK_MIGRATIONS, PULL_WATCH_MIGRATIONS, SERVER_SETTINGS_MIGRATIONS, WAKE_SOURCE_MIGRATIONS, REVIEWER_MODEL_MIGRATIONS, LESSON_PR_MIGRATIONS, ACTOR_MIGRATIONS, APPROVAL_MARKER_MIGRATIONS]; let end = MIGRATIONS.length; for (const group of groups) { expect(MIGRATIONS.slice(end - group.length, end)).toEqual([...group]); @@ -847,8 +847,10 @@ describe("the escalation route rename migration (BB-209)", () => { // BB-328's check-findings table after that, BB-334's backlog snapshot // column after that, and BB-333's typed wait column after that. BB-335's // non-action column is last but for BB-333 PR 2's wait-divergence table. - const withoutWaitDivergence = MIGRATIONS.slice(0, -WAIT_DIVERGENCE_MIGRATIONS.length); - expect(MIGRATIONS.slice(-WAIT_DIVERGENCE_MIGRATIONS.length)).toEqual([...WAIT_DIVERGENCE_MIGRATIONS]); + const withoutPlaybookRefresh = MIGRATIONS.slice(0, -PLAYBOOK_REFRESH_MIGRATIONS.length); + expect(MIGRATIONS.slice(-PLAYBOOK_REFRESH_MIGRATIONS.length)).toEqual([...PLAYBOOK_REFRESH_MIGRATIONS]); + const withoutWaitDivergence = withoutPlaybookRefresh.slice(0, -WAIT_DIVERGENCE_MIGRATIONS.length); + expect(withoutPlaybookRefresh.slice(-WAIT_DIVERGENCE_MIGRATIONS.length)).toEqual([...WAIT_DIVERGENCE_MIGRATIONS]); const withoutNonAction = withoutWaitDivergence.slice(0, -NON_ACTION_MIGRATIONS.length); expect(withoutWaitDivergence.slice(-NON_ACTION_MIGRATIONS.length)).toEqual([...NON_ACTION_MIGRATIONS]); const withoutTypedWait = withoutNonAction.slice(0, -TYPED_WAIT_MIGRATIONS.length); diff --git a/plugins/stewards/store.ts b/plugins/stewards/store.ts index 91a6742e..42414cb9 100644 --- a/plugins/stewards/store.ts +++ b/plugins/stewards/store.ts @@ -792,7 +792,36 @@ export const WAIT_DIVERGENCE_MIGRATIONS = [ )`, ]; -export const MIGRATIONS = [...BASE_MIGRATIONS, ...CHAIN_MIGRATIONS, ...LESSON_COUNTER_MIGRATIONS, ...WORKER_CHECK_MIGRATIONS, ...DECLINED_PR_MIGRATIONS, ...DEPLOYMENT_MIGRATIONS, ...FINISHED_AND_DIGEST_MIGRATIONS, ...LESSON_TIER_MIGRATIONS, ...RATIONALE_MIGRATIONS, ...VALIDATION_MIGRATIONS, ...GUIDANCE_MIGRATIONS, ...IDLE_MIGRATIONS, ...APPROVAL_MARKER_MIGRATIONS, ...ACTOR_MIGRATIONS, ...LESSON_PR_MIGRATIONS, ...REVIEWER_MODEL_MIGRATIONS, ...WAKE_SOURCE_MIGRATIONS, ...SERVER_SETTINGS_MIGRATIONS, ...PULL_WATCH_MIGRATIONS, ...STACK_MIGRATIONS, ...ESCALATION_ROUTE_MIGRATIONS, ...DEPS_DECISION_MIGRATIONS, ...WORKER_STOP_REASON_MIGRATIONS, ...ASSUMPTION_MIGRATIONS, ...DATED_WAIT_MIGRATIONS, ...WAKE_RETRY_MIGRATIONS, ...MERGE_BEHIND_MIGRATIONS, ...RETARGET_PULLS_MIGRATIONS, ...STACKED_PULLS_MIGRATIONS, ...ASSESSMENT_MIGRATIONS, ...ENDED_READ_MIGRATIONS, ...HUDDLE_MIGRATIONS, ...STANDUP_MIGRATIONS, ...STANDUP_BINDING_MIGRATIONS, ...STANDUP_DELEGATION_MIGRATIONS, ...STANDUP_COMPLETED_MIGRATIONS, ...ALERTMANAGER_LOOK_MIGRATIONS, ...PULL_REQUESTER_MIGRATIONS, ...REVIEWER_REQUEST_MIGRATIONS, ...WORKER_PARK_MIGRATIONS, ...BACKLOG_LOOK_MIGRATIONS, ...CHECK_FINDING_MIGRATIONS, ...BACKLOG_SNAPSHOT_MIGRATIONS, ...TYPED_WAIT_MIGRATIONS, ...NON_ACTION_MIGRATIONS, ...WAIT_DIVERGENCE_MIGRATIONS]; +// BB-328 Part 2: one queued refresh of a project playbook that lags its +// shipped template, so the nightly pass queues each playbook once, a declined +// or failed refresh is not retried until the template changes, and the stamp +// is updated only by a merged refresh. A new table appended after every group +// already shipped, so no recorded index moves. +export const PLAYBOOK_REFRESH_MIGRATIONS = [ + `CREATE TABLE IF NOT EXISTS playbook_refreshes ( + id TEXT PRIMARY KEY, + project_id TEXT NOT NULL, + file TEXT NOT NULL, + playbook TEXT NOT NULL, + template_name TEXT NOT NULL, + from_hash TEXT NOT NULL, + target_hash TEXT NOT NULL, + state TEXT NOT NULL DEFAULT 'queued', + thread_id TEXT, + branch TEXT, + pr TEXT, + classification TEXT, + removals TEXT NOT NULL DEFAULT '[]', + question_id TEXT, + note TEXT, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + resolved_at INTEGER + )`, + `CREATE INDEX IF NOT EXISTS playbook_refreshes_project ON playbook_refreshes (project_id, playbook)`, +]; + +export const MIGRATIONS = [...BASE_MIGRATIONS, ...CHAIN_MIGRATIONS, ...LESSON_COUNTER_MIGRATIONS, ...WORKER_CHECK_MIGRATIONS, ...DECLINED_PR_MIGRATIONS, ...DEPLOYMENT_MIGRATIONS, ...FINISHED_AND_DIGEST_MIGRATIONS, ...LESSON_TIER_MIGRATIONS, ...RATIONALE_MIGRATIONS, ...VALIDATION_MIGRATIONS, ...GUIDANCE_MIGRATIONS, ...IDLE_MIGRATIONS, ...APPROVAL_MARKER_MIGRATIONS, ...ACTOR_MIGRATIONS, ...LESSON_PR_MIGRATIONS, ...REVIEWER_MODEL_MIGRATIONS, ...WAKE_SOURCE_MIGRATIONS, ...SERVER_SETTINGS_MIGRATIONS, ...PULL_WATCH_MIGRATIONS, ...STACK_MIGRATIONS, ...ESCALATION_ROUTE_MIGRATIONS, ...DEPS_DECISION_MIGRATIONS, ...WORKER_STOP_REASON_MIGRATIONS, ...ASSUMPTION_MIGRATIONS, ...DATED_WAIT_MIGRATIONS, ...WAKE_RETRY_MIGRATIONS, ...MERGE_BEHIND_MIGRATIONS, ...RETARGET_PULLS_MIGRATIONS, ...STACKED_PULLS_MIGRATIONS, ...ASSESSMENT_MIGRATIONS, ...ENDED_READ_MIGRATIONS, ...HUDDLE_MIGRATIONS, ...STANDUP_MIGRATIONS, ...STANDUP_BINDING_MIGRATIONS, ...STANDUP_DELEGATION_MIGRATIONS, ...STANDUP_COMPLETED_MIGRATIONS, ...ALERTMANAGER_LOOK_MIGRATIONS, ...PULL_REQUESTER_MIGRATIONS, ...REVIEWER_REQUEST_MIGRATIONS, ...WORKER_PARK_MIGRATIONS, ...BACKLOG_LOOK_MIGRATIONS, ...CHECK_FINDING_MIGRATIONS, ...BACKLOG_SNAPSHOT_MIGRATIONS, ...TYPED_WAIT_MIGRATIONS, ...NON_ACTION_MIGRATIONS, ...WAIT_DIVERGENCE_MIGRATIONS, ...PLAYBOOK_REFRESH_MIGRATIONS]; // ---- values kept as JSON ------------------------------------------------------------ @@ -1607,6 +1636,52 @@ export type Lesson = { resolvedAt: number | null; }; +/** + * The lifecycle of a playbook refresh (BB-328 Part 2): `queued` when the + * nightly pass found the lag, `spawned` when its worker thread started, + * `pr_open` when the plugin opened the reviewed pull request, and `merged`, + * `declined` or `failed` once it is over. The stamp is only updated by the + * merged pull request, which is what stops the playbook being queued again. + */ +export const REFRESH_STATES = ["queued", "spawned", "pr_open", "merged", "declined", "failed"] as const; +export type RefreshState = (typeof REFRESH_STATES)[number]; +/** A refresh state read from a row, failing closed to `queued` for an unknown value. */ +export const refreshState = (value: unknown): RefreshState => (REFRESH_STATES.includes(value as RefreshState) ? (value as RefreshState) : "queued"); +export type RefreshClassificationKind = "additive" | "not-additive"; +/** One queued refresh of a project playbook that lags its shipped template. */ +export type PlaybookRefresh = { + id: string; + projectId: string; + /** The playbook file, repo-relative, e.g. `stewards/playbooks/ops-watch.md`. */ + file: string; + /** The playbook's front-matter `name`. */ + playbook: string; + /** The shipped template it is refreshed from. */ + templateName: string; + /** The stamp hash the project copy carried when the refresh was queued. */ + fromHash: string; + /** The current template hash the refresh targets; the stamp is set to it on merge. */ + targetHash: string; + state: RefreshState; + /** The refresh worker thread, once spawned. */ + threadId: string | null; + /** The `guidance-` branch the worker published on. */ + branch: string | null; + /** The pull request the plugin opened from that branch. */ + pr: string | null; + /** The plugin's own classification of the published diff. */ + classification: RefreshClassificationKind | null; + /** Removed or rewritten lines, one each, from the classification. */ + removals: string[]; + /** The owner question the project steward filed when it forwarded, or null. */ + questionId: string | null; + /** The last refusal or explanation, for the wake. */ + note: string | null; + createdAt: number; + updatedAt: number; + resolvedAt: number | null; +}; + export type WorkerStatus = "running" | "finished" | "failed" | "stopped"; /** * Why a worker was stopped (BB-242). `archived` is the plugin's own archive @@ -2117,6 +2192,20 @@ export function createStore(db: Database.Database, now: () => number, options: S const setLessonTierStmt = db.prepare("UPDATE lessons SET tier = ?, tier_at = ?, updated_at = ? WHERE id = ?"); const setLessonClusterStmt = db.prepare("UPDATE lessons SET cluster_key = ?, updated_at = ? WHERE id = ?"); const setLessonSupersededStmt = db.prepare("UPDATE lessons SET superseded_by = ?, updated_at = ? WHERE id = ?"); + // BB-328 Part 2: the playbook-refresh rows. A refresh is read by project and + // by its worker thread; every field is rewritten in one update so a caller + // cannot leave the row half moved. Prepared lazily, like every statement on + // a newer table, so a legacy store opened without the migration still starts + // (migrations.test.ts). + const insertPlaybookRefresh = lazy<[Record], unknown>( + "INSERT INTO playbook_refreshes (id, project_id, file, playbook, template_name, from_hash, target_hash, state, created_at, updated_at) VALUES (@id, @project_id, @file, @playbook, @template_name, @from_hash, @target_hash, 'queued', @created_at, @updated_at)", + ); + const getPlaybookRefresh = lazy<[string], Record>("SELECT * FROM playbook_refreshes WHERE id = ?"); + const playbookRefreshesFor = lazy<[string], Record>("SELECT * FROM playbook_refreshes WHERE project_id = ? ORDER BY created_at"); + const playbookRefreshByThreadStmt = lazy<[string], Record>("SELECT * FROM playbook_refreshes WHERE thread_id = ? ORDER BY created_at DESC LIMIT 1"); + const updatePlaybookRefreshStmt = lazy<[Record], unknown>( + "UPDATE playbook_refreshes SET state = @state, thread_id = @thread_id, branch = @branch, pr = @pr, classification = @classification, removals = @removals, question_id = @question_id, note = @note, updated_at = @updated_at, resolved_at = @resolved_at WHERE id = @id", + ); const lessonsByTierStmt = db.prepare<[string, string], Record>("SELECT * FROM lessons WHERE project_id = ? AND tier = ? ORDER BY created_at"); const setEscalationCitationsStmt = db.prepare("UPDATE escalations SET cited_lessons = ? WHERE id = ?"); const markEscalationCorrectedStmt = db.prepare("UPDATE escalations SET corrections_counted = 1 WHERE id = ?"); @@ -2360,6 +2449,27 @@ export function createStore(db: Database.Database, now: () => number, options: S resolvedAt: r.resolved_at === null || r.resolved_at === undefined ? null : Number(r.resolved_at), }); + const toPlaybookRefresh = (r: Record): PlaybookRefresh => ({ + id: String(r.id), + projectId: String(r.project_id), + file: String(r.file), + playbook: String(r.playbook), + templateName: String(r.template_name), + fromHash: String(r.from_hash), + targetHash: String(r.target_hash), + state: refreshState(r.state), + threadId: r.thread_id === null || r.thread_id === undefined ? null : String(r.thread_id), + branch: r.branch === null || r.branch === undefined ? null : String(r.branch), + pr: r.pr === null || r.pr === undefined ? null : String(r.pr), + classification: r.classification === "additive" || r.classification === "not-additive" ? r.classification : null, + removals: json(z.array(z.string()), r.removals === null || r.removals === undefined ? null : String(r.removals), []), + questionId: r.question_id === null || r.question_id === undefined ? null : String(r.question_id), + note: r.note === null || r.note === undefined ? null : String(r.note), + createdAt: Number(r.created_at), + updatedAt: Number(r.updated_at), + resolvedAt: r.resolved_at === null || r.resolved_at === undefined ? null : Number(r.resolved_at), + }); + const onProjectChange = options.onProjectChange; /** The project a binding belongs to, for a mutation that only has the binding. */ @@ -3381,6 +3491,73 @@ export function createStore(db: Database.Database, now: () => number, options: S resolveLessonStmt.run("dropped", null, null, note, at, at, id); setLessonTierStmt.run("retired", at, at, id); }, + + // playbook refreshes (BB-328 Part 2): the nightly pass queues one per + // lagging playbook, a worker thread produces the merged copy, and the + // plugin opens a reviewed pull request. The row is the queue: it is the + // one place the open/deferred state and the stamp's target live. + /** Queue a refresh of a lagging playbook. */ + addPlaybookRefresh(fields: { projectId: string; file: string; playbook: string; templateName: string; fromHash: string; targetHash: string }): PlaybookRefresh { + const id = newId("rfr"); + const at = now(); + insertPlaybookRefresh().run({ + id, + project_id: fields.projectId, + file: fields.file, + playbook: fields.playbook, + template_name: fields.templateName, + from_hash: fields.fromHash, + target_hash: fields.targetHash, + created_at: at, + updated_at: at, + }); + projectChanged(fields.projectId); + return toPlaybookRefresh(getPlaybookRefresh().get(id)!); + }, + playbookRefresh(id: string): PlaybookRefresh | null { + const r = getPlaybookRefresh().get(id); + return r === undefined ? null : toPlaybookRefresh(r); + }, + /** A project's refreshes, oldest first, or one playbook's. */ + playbookRefreshes(projectId: string, playbook?: string): PlaybookRefresh[] { + const rows = playbookRefreshesFor().all(projectId).map(toPlaybookRefresh); + return playbook === undefined ? rows : rows.filter((r) => r.playbook === playbook); + }, + /** The refresh a worker thread belongs to, or null. */ + playbookRefreshByThread(threadId: string): PlaybookRefresh | null { + const r = playbookRefreshByThreadStmt().get(threadId); + return r === undefined ? null : toPlaybookRefresh(r); + }, + /** + * Move a refresh along. Only the named fields change; a terminal state + * stamps `resolved_at` once, so a later read of the resolved time is the + * moment it actually ended. A merged refresh is what makes the playbook + * stop lagging; nothing else writes the stamp. + */ + setPlaybookRefresh( + id: string, + patch: Partial>, + ): PlaybookRefresh | null { + const row = getPlaybookRefresh().get(id); + if (row === undefined) return null; + const state = patch.state ?? refreshState(row.state); + const at = now(); + updatePlaybookRefreshStmt().run({ + id, + state, + thread_id: patch.threadId !== undefined ? patch.threadId : (row.thread_id as string | null), + branch: patch.branch !== undefined ? patch.branch : (row.branch as string | null), + pr: patch.pr !== undefined ? patch.pr : (row.pr as string | null), + classification: patch.classification !== undefined ? patch.classification : (row.classification as string | null), + removals: patch.removals !== undefined ? JSON.stringify(patch.removals) : String(row.removals), + question_id: patch.questionId !== undefined ? patch.questionId : (row.question_id as string | null), + note: patch.note !== undefined ? patch.note : (row.note as string | null), + updated_at: at, + resolved_at: state === "queued" || state === "spawned" || state === "pr_open" ? (row.resolved_at as number | null) : ((row.resolved_at as number | null) ?? at), + }); + projectChanged(String(row.project_id)); + return toPlaybookRefresh(getPlaybookRefresh().get(id)!); + }, /** * Record the lessons one decision cited (BB-35 step 2). A lesson's `hits` * counts distinct bindings: a second citation from the same binding diff --git a/plugins/stewards/wake.ts b/plugins/stewards/wake.ts index 1c67ad44..74fc91b5 100644 --- a/plugins/stewards/wake.ts +++ b/plugins/stewards/wake.ts @@ -22,7 +22,7 @@ import { reportedItem, type GateState } from "./ready-gate.js"; import { currentStoredGate, type StoredGate } from "./assessed-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"; -import { needChoices, ESCALATED_NEED_KINDS, RESOLUTION_KINDS, STEWARD_NEED_KINDS, type Assumption, type Binding, type DepsDecision, type Escalation, type Lesson, type Need, type Observed, type Question, type Worker } from "./store.js"; +import { needChoices, ESCALATED_NEED_KINDS, RESOLUTION_KINDS, STEWARD_NEED_KINDS, type Assumption, type Binding, type DepsDecision, type Escalation, type Lesson, type Need, type Observed, type PlaybookRefresh, type Question, type Worker } from "./store.js"; import type { CheckFinding } from "./template-stamp.js"; /** A wake longer than this is cut in its snapshot, never in what changed or what to do. */ @@ -1413,6 +1413,32 @@ function projectLessonsLines(lessons: Lesson[], rules: LessonRules | undefined): return out; } +/** + * The `## Playbooks to refresh` block (BB-328 Part 2): the playbook refreshes + * whose pull request is ready for the project steward to read and decide, plus + * the ones still being produced. The plugin's classification is only its read + * of the diff; the project steward reads the diff and decides. + */ +function projectRefreshesLines(refreshes: PlaybookRefresh[]): string[] { + const ready = refreshes.filter((r) => r.state === "pr_open"); + const pending = refreshes.filter((r) => r.state === "queued" || r.state === "spawned"); + if (ready.length === 0 && pending.length === 0) return []; + const out: string[] = ["", "## Playbooks to refresh", ""]; + if (ready.length > 0) { + out.push( + "The nightly check found these project playbooks lag their shipped template, and the plugin opened a refresh pull request. The refresh worker merged the template into the project copy and updated the stamp. Read the diff and decide; the classification is only the plugin's read of the diff.", + ); + for (const r of ready) { + out.push( + `- ${r.id} (${r.playbook}, ${r.file}): ${r.classification === "additive" ? "purely additive" : "not purely additive"} — ${r.pr ?? "(no pull request)"}${r.removals.length === 0 ? "" : `\n Removes or rewrites: ${r.removals.map((l) => JSON.stringify(l)).join(", ")}`}\n Merge a purely additive, project-lane change with \`steward_update { refreshAction: { id: "${r.id}", action: "merge" } }\`; send anything else to the owner with \`{ id: "${r.id}", action: "forward" }\`; drop it with \`{ id: "${r.id}", action: "drop" }\`.`, + ); + } + } + if (pending.length > 0) out.push("", `In progress (no decision yet): ${pending.map((r) => `${r.id} (${r.playbook}, ${r.state})`).join("; ")}.`); + out.push(""); + return out; +} + /** The decide-list listing a plugin-filed `approve-pr` need carries, or null (BB-135). */ function needListing(n: Need): string | null { return typeof n.data.decideListing === "string" && n.data.decideListing !== "" ? n.data.decideListing : null; @@ -1545,6 +1571,8 @@ export function formatProjectWake(input: { assumptions?: Array<{ assumption: Assumption; binding: Binding | null }>; /** Pending lessons: general rules waiting to be landed in `.bb`. */ lessons?: Lesson[]; + /** BB-328 Part 2: playbook refreshes, for the project steward to read and decide. */ + refreshes?: PlaybookRefresh[]; /** The hand-written guidance units this wake renders, each with the `gu_` id a steward cites (BB-288). */ guidance?: GuidanceUnit[]; /** The project's lesson rules, for the trial cap and the wake headings. */ @@ -1653,6 +1681,7 @@ export function formatProjectWake(input: { } const lessons = input.lessons ?? []; out.push(...projectLessonsLines(lessons, input.lessonRules)); + out.push(...projectRefreshesLines(input.refreshes ?? [])); const head = out.splice(0); out.push(`## Bindings (${input.bindings.length} open)`, ""); const allOpen = input.bindings.map((x) => x.binding); From 608e5e15ed808f7d1f802a6918eb5802f840930b Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Sun, 15 Nov 2026 10:43:37 +0000 Subject: [PATCH 2/3] stewards: address review round 1 on the playbook refresh (BB-328) - merge is refused unless the decide list places the playbook project and the stored classification is additive, so a removal, a rewrite or a contradiction can only go to the owner - the owner's Drop on the page closes the pull request and declines the refresh, and the next nightly pass does not re-queue it - the worker brief ends with a fixed Contradictions: line; a missing line or a listed rule makes the refresh not additive and routes it to the owner - the brief fences the template byte-for-byte, so a template's own fenced block is not corrupted --- plugins/stewards/DESIGN.md | 17 ++++--- plugins/stewards/README.md | 10 ++-- plugins/stewards/refresh.test.ts | 21 +++++++- plugins/stewards/refresh.ts | 40 +++++++++++++++ plugins/stewards/server.test.ts | 85 ++++++++++++++++++++++++++++++-- plugins/stewards/server.ts | 63 ++++++++++++++++++----- plugins/stewards/store.ts | 3 ++ 7 files changed, 212 insertions(+), 27 deletions(-) diff --git a/plugins/stewards/DESIGN.md b/plugins/stewards/DESIGN.md index b538deeb..16a43f77 100644 --- a/plugins/stewards/DESIGN.md +++ b/plugins/stewards/DESIGN.md @@ -1048,12 +1048,17 @@ deployment: # where a merge is confirmed live (BB-53); omi the pull request on the primary repo, and classifies the diff for the body: *purely additive* means only insertions and the front matter untouched apart from the stamp, and a change to `models`, `may`, a gate or - `requeue-default` is never additive. The classification is the plugin's - read, not the worker's claim; the project steward reads the diff in the - `## Playbooks to refresh` block and decides with - `steward_update { refreshAction: { id, action } }` — `merge` for a purely - additive, project-lane change, `forward` to file the owner's Review - PR/Drop item, `drop` to close it and decline. The guardrails are the row's: + `requeue-default` is never additive; a project rule that contradicts the + template is never additive either, whatever the line diff looks like. The + classification is the plugin's read, not the worker's claim; the project + steward reads the diff in the `## Playbooks to refresh` block and decides + with `steward_update { refreshAction: { id, action } }` — `merge` for a + purely additive, project-lane change, `forward` to file the owner's Review + PR/Drop item, `drop` to close it and decline. `merge` is refused unless the + decide list places the playbook `project` and the stored classification is + `additive`, so a removal, a rewrite or a contradiction can only go to the + owner; the owner's Drop on the page closes the pull request and declines + it. The guardrails are the row's: at most one refresh is open per playbook, a declined or failed refresh is not retried until the template changes again, and the stamp is updated only by the merged pull request — which is what stops the playbook being queued. diff --git a/plugins/stewards/README.md b/plugins/stewards/README.md index 3a047805..ea5b975e 100644 --- a/plugins/stewards/README.md +++ b/plugins/stewards/README.md @@ -697,12 +697,16 @@ template into the project copy keeping the project's own additions, updates the stamp, and publishes with `workspace_publish_guidance`; the plugin opens the pull request, classifies the diff (purely additive means only insertions and the front matter untouched apart from the stamp; a change to `models`, -`may`, a gate or `requeue-default` is never additive) and lists the removals -and rewrites in the body. That classification is only the plugin's read of +`may`, a gate or `requeue-default` is never additive, and neither is a project +rule that contradicts the template) and lists the removals and rewrites in +the body. That classification is only the plugin's read of the diff: the project steward reads it in the `## Playbooks to refresh` block and decides with `steward_update { refreshAction: { id, action } }` — `merge` for a purely additive, project-lane change, `forward` to file the owner's -Review PR/Drop item, `drop` to close it and decline. At most one refresh is +Review PR/Drop item, `drop` to close it and decline. `merge` is refused unless +the decide list places the playbook `project` and the stored classification is +`additive`, so a removal, a rewrite or a contradiction can only go to the +owner; the owner's Drop on the page closes the pull request and declines it. At most one refresh is open per playbook, a declined or failed refresh is not retried until the template changes again, and the stamp is only updated by the merged pull request, which is what stops the playbook being queued. A server on an image diff --git a/plugins/stewards/refresh.test.ts b/plugins/stewards/refresh.test.ts index f44e3883..6c3bb48a 100644 --- a/plugins/stewards/refresh.test.ts +++ b/plugins/stewards/refresh.test.ts @@ -3,7 +3,7 @@ // after a decline until the template changes), how a published diff is // classified, and the pull request body. import { describe, expect, it } from "vitest"; -import { classifyRefresh, hasTemplateStamp, laggingPlaybooks, refreshPrBody, refreshQueueDecision } from "./refresh.js"; +import { classifyRefresh, fencedBlock, hasTemplateStamp, laggingPlaybooks, parseContradictions, refreshPrBody, refreshQueueDecision } from "./refresh.js"; import type { PlaybookRefresh } from "./store.js"; const refresh = (over: Partial): PlaybookRefresh => ({ @@ -176,3 +176,22 @@ describe("hasTemplateStamp", () => { expect(hasTemplateStamp(text, "other", "abc123")).toBe(false); }); }); + +describe("parseContradictions", () => { + it("reads `none`, a list, and a missing line (fail closed)", () => { + expect(parseContradictions("Did the merge.\nContradictions: none")).toEqual({ listed: true, rules: [] }); + expect(parseContradictions("Contradictions:\n- project says A, template says B\n- project says C")).toEqual({ listed: true, rules: ["project says A, template says B", "project says C"] }); + expect(parseContradictions("Contradictions: project says A")).toEqual({ listed: true, rules: ["project says A"] }); + expect(parseContradictions("Did the merge.")).toEqual({ listed: false, rules: [] }); + expect(parseContradictions(null)).toEqual({ listed: false, rules: [] }); + }); +}); + +describe("fencedBlock", () => { + it("keeps a template's own fence byte-for-byte", () => { + const text = "Before\n```bb-review\nmarker\n```\nAfter"; + const wrapped = fencedBlock(text); + expect(wrapped).toContain(text); + expect(wrapped.startsWith("````")).toBe(true); + }); +}); diff --git a/plugins/stewards/refresh.ts b/plugins/stewards/refresh.ts index 30c026ac..24df54c5 100644 --- a/plugins/stewards/refresh.ts +++ b/plugins/stewards/refresh.ts @@ -80,6 +80,44 @@ export type RefreshClassification = { /** The front matter keys a change to which is never purely additive (BB-328). */ const NEVER_ADDITIVE_KEYS = new Set(["models", "may", "requeue-default"]); +/** + * Wrap a block of text in a Markdown fence longer than any backtick run it + * contains, so the text is shown byte-for-byte: a template that itself contains + * a ``` block (the `bb-review` marker) is not corrupted in the brief. + */ +export function fencedBlock(text: string): string { + const runs = [...text.matchAll(/`+/gu)].map((m) => m[0]!.length); + const fence = "`".repeat(Math.max(3, ...runs.map((n) => n + 1))); + return `${fence}\n${text}\n${fence}`; +} + +/** The fixed line a refresh thread ends its reply with. */ +export const CONTRADICTIONS_PREFIX = "Contradictions:"; + +/** + * Read the `Contradictions:` line from a refresh thread's reply. `listed` is + * false when the reply carries no such line, which the caller treats as not + * additive: a worker that does not say it found none is not trusted to have + * found none. `none` is the only value that means the project's text did not + * contradict the template. + */ +export function parseContradictions(claim: string | null): { listed: boolean; rules: string[] } { + if (claim === null) return { listed: false, rules: [] }; + const lines = claim.split(/\r?\n/u); + const at = lines.findIndex((l) => l.trim().toLowerCase().startsWith(CONTRADICTIONS_PREFIX.toLowerCase())); + if (at === -1) return { listed: false, rules: [] }; + const rules: string[] = []; + const first = lines[at]!.slice(lines[at]!.indexOf(":") + 1).trim(); + if (first !== "" && first.toLowerCase() !== "none") rules.push(first); + for (let i = at + 1; i < lines.length; i += 1) { + const line = lines[i]!.trim(); + if (line === "") break; + if (line.startsWith("-")) rules.push(line.replace(/^-\s*/u, "")); + else break; + } + return { listed: true, rules }; +} + const splitFrontMatter = (text: string): { front: string | null; body: string } => { const m = /^---\r?\n([\s\S]*?)\r?\n---\r?\n?/u.exec(text); if (m === null) return { front: null, body: text }; @@ -203,6 +241,7 @@ export function refreshPrBody( c: RefreshClassification, threadId: string, claim?: string | null, + contradictions: string[] = [], ): string { const path = `stewards/playbooks/${refresh.playbook}.md`; const lines = [ @@ -214,6 +253,7 @@ export function refreshPrBody( c.classification === "additive" ? "**Purely additive** — only insertions, and the front matter is untouched apart from the stamp." : "**Not purely additive** — the merge removes or rewrites something the project copy carried.", ]; if (claim !== undefined && claim !== null && claim.trim() !== "") lines.push("", "The refresh thread's own summary (its claim, not the decision):", "", "```", claim.trim(), "```"); + if (contradictions.length > 0) lines.push("", "## Contradictions", "", "The project's own text contradicts the new template, so a merge means choosing a rule. The owner decides:", ...contradictions.map((r) => `- ${r}`)); if (c.reasons.length > 0) lines.push("", "Front matter:", ...c.reasons.map((r) => `- ${r}`)); if (c.removals.length > 0) lines.push("", "Removed lines:", ...c.removals.map((l) => `- \`${l}\``)); if (c.rewrites.length > 0) { diff --git a/plugins/stewards/server.test.ts b/plugins/stewards/server.test.ts index 6df73462..f5e72933 100644 --- a/plugins/stewards/server.test.ts +++ b/plugins/stewards/server.test.ts @@ -8,7 +8,7 @@ import { comment, crashingPod, githubIssue, issue, parkedWorker, pull, warningEv import { kubectlRefusal, type KubectlResult } from "./kubectl.js"; import { retiredLessonAnchor, withLessonAnchor } from "./lessons.js"; import { guidanceUnitId } from "./guidance.js"; -import { needActions, parseDotenvValue, secretVariable } from "./server.js"; +import { needActions, parseDotenvValue, refreshPrompt, secretVariable } from "./server.js"; import { SOURCE_PLUGIN_IDS, type EntitySnapshot } from "./source-contract.js"; import { MIGRATIONS, ESCALATED_NEED_KINDS, NEEDS_KINDS, STEWARD_NEED_KINDS, type Binding } from "./store.js"; import { GITHUB_ISSUES, GITHUB_PRS, GITHUB_REVIEWS, LINEAR_TICKETS, OPS_WATCH, DEPS_ALERTS, DEPS_PRS, PLAYBOOK_TEMPLATES, projectFileTemplate, stampedTemplate } from "./templates.js"; @@ -17036,20 +17036,25 @@ describe("playbook refreshes (BB-328 Part 2)", () => { const laggingCopy = () => LINEAR_TICKETS.replace("name: linear-tickets", "name: linear-tickets\ntemplate: linear-tickets@deadbeef"); /** A setup whose project copy of linear-tickets carries an old stamp. */ - const withLaggingPlaybook = async () => { + const withLaggingPlaybook = async (opts: { userPlaybook?: boolean } = {}) => { const t = await setup({ timing: { debounceMs: 1, nightlyStaggerMs: 0, configTtlMs: 0 } }); t.files[playbookPath("linear-tickets")] = laggingCopy(); + // The real project file marks playbooks `project`; mirror it so the refresh + // is the project steward's to merge. A test may instead make it the user's. + const project = JSON.parse(t.files[PROJECT_FILE]!) as { bigDecisions: unknown[] }; + project.bigDecisions = opts.userPlaybook === true ? [] : [...project.bigDecisions, BB280_PLAYBOOK_ENTRY]; + t.files[PROJECT_FILE] = JSON.stringify(project, null, 2); return t; }; /** Simulate the worker publishing its merged copy, then finishing its turn. */ - const publishAndFinish = async (t: Awaited>) => { + const publishAndFinish = async (t: Awaited>, claim = "Purely additive: I added the new paragraph.\nContradictions: none") => { const refresh = t.hooks.store.playbookRefreshes("p1")[0]!; const tid = refresh.threadId!; const branch = `guidance-${tid}`; const merged = laggingCopy().replace("template: linear-tickets@deadbeef", `template: linear-tickets@${templateHash(LINEAR_TICKETS)}`) + "\nA new template rule.\n"; t.githubFileContents.set(`acme/bb#${branch}:.bb/${playbookPath("linear-tickets")}`, merged); - await t.idle(tid, "additive"); + await t.idle(tid, claim); return t.hooks.store.playbookRefreshes("p1")[0]!; }; @@ -17120,4 +17125,76 @@ describe("playbook refreshes (BB-328 Part 2)", () => { await t.flush(); expect(t.hooks.store.playbookRefreshes("p1")).toHaveLength(1); }); + + const client = { experimental_caller: { kind: "client" } } as const; + + it("refuses to merge a refresh the plugin classified not purely additive, sending it to the owner", async () => { + const t = await withLaggingPlaybook(); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + // A contradiction makes it not additive, whatever the line diff looked like. + const refresh = await publishAndFinish(t, "Contradictions:\n- The project's rule conflicts with the template's."); + expect(refresh.classification).toBe("not-additive"); + const steward = t.hooks.store.project("p1").threadId!; + const refused = await t.tool("steward_update", { refreshAction: { id: refresh.id, action: "merge" } }, steward); + expect(refused.error).toBe(false); + expect(refused.text).toMatch(/not purely additive/u); + expect(t.githubMerges).toHaveLength(0); + expect(t.hooks.store.playbookRefreshes("p1")[0]!.state).toBe("pr_open"); + }); + + it("refuses to merge a refresh the decide list makes the user's", async () => { + const t = await withLaggingPlaybook({ userPlaybook: true }); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + const refresh = await publishAndFinish(t); + expect(refresh.classification).toBe("additive"); + const steward = t.hooks.store.project("p1").threadId!; + const refused = await t.tool("steward_update", { refreshAction: { id: refresh.id, action: "merge" } }, steward); + expect(refused.error).toBe(false); + expect(refused.text).toMatch(/user's to approve/u); + expect(t.githubMerges).toHaveLength(0); + }); + + it("files the owner's item on forward, and the owner's Drop declines it", async () => { + const t = await withLaggingPlaybook(); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + const refresh = await publishAndFinish(t); + const steward = t.hooks.store.project("p1").threadId!; + await t.tool("steward_update", { refreshAction: { id: refresh.id, action: "forward" } }, steward); + const forwarded = t.hooks.store.playbookRefresh(refresh.id)!; + expect(forwarded.questionId).not.toBeNull(); + const q = t.hooks.store.question(forwarded.questionId!)!; + expect(q.data.refreshId).toBe(refresh.id); + // The owner picks Drop on the Stewards page. + const answered = await t.harness.callRpc("stewardsAnswerQuestion", { questionId: forwarded.questionId!, choice: "Drop" }, client); + expect(answered).toMatchObject({ ok: true }); + await t.flush(); + expect(t.hooks.store.playbookRefresh(refresh.id)!.state).toBe("declined"); + expect(t.githubClosedPulls).toHaveLength(1); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + expect(t.hooks.store.playbookRefreshes("p1")).toHaveLength(1); + }); + + it("marks a refresh declined when its pull request is closed on GitHub without merging", async () => { + const t = await withLaggingPlaybook(); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + const refresh = await publishAndFinish(t); + const ref = /\/pull\/(\d+)$/u.exec(refresh.pr!)!; + t.entities.set(`acme/bb#${ref[1]}`, pull({ id: `acme/bb#${ref[1]}`, key: `acme/bb#${ref[1]}`, unit: "acme/bb", state: "closed" }, { merged: false })); + await t.hooks.nightlyPass("p1", "manual"); + await t.flush(); + expect(t.hooks.store.playbookRefreshes("p1")[0]!.state).toBe("declined"); + }); + + it("shows the shipped template byte-for-byte, fenced, in the worker brief", async () => { + const brief = refreshPrompt({ playbook: "github-reviews", templateName: "github-reviews", fromHash: "old", targetHash: "new" }, "---\nname: github-reviews\n---\n", GITHUB_REVIEWS); + // The template's own ```-fenced marker block survives byte-for-byte. + expect(GITHUB_REVIEWS).toContain("\n```\n