diff --git a/plugins/stewards/DESIGN.md b/plugins/stewards/DESIGN.md index 8e637cf4..efcfdc02 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,34 @@ 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; 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; it also refuses a branch pushed after it was classified, so only the + head the plugin read can be merged. 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. + 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..06214a16 100644 --- a/plugins/stewards/README.md +++ b/plugins/stewards/README.md @@ -689,6 +689,32 @@ 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 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. `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; a branch pushed after it was classified is refused too, so only the +head the plugin read can be merged. 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 +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/github.ts b/plugins/stewards/github.ts index d4a27b1a..54bbefd8 100644 --- a/plugins/stewards/github.ts +++ b/plugins/stewards/github.ts @@ -134,7 +134,7 @@ export interface GithubWriter { */ branchExists(repo: string, branch: string): Promise; /** Open a pull request and return its URL and number (BB-108). */ - createPull(repo: string, args: { head: string; base: string; title: string; body: string }): Promise<{ url: string; number: number }>; + createPull(repo: string, args: { head: string; base: string; title: string; body: string }): Promise<{ url: string; number: number; headSha: string | null }>; /** Close a pull request without merging it (BB-108 Drop). */ closePull(repo: string, number: number): Promise; /** @@ -266,9 +266,9 @@ export function createGhWriter(token: TokenReader = createGhTokenReader(), doFet }, async createPull(repo, args) { const response = await request("POST", `/repos/${repo}/pulls`, { head: args.head, base: args.base, title: args.title, body: args.body }); - const body = (await response.json().catch(() => null)) as { html_url?: unknown; number?: unknown } | null; + const body = (await response.json().catch(() => null)) as { html_url?: unknown; number?: unknown; head?: { sha?: unknown } } | null; if (typeof body?.html_url !== "string" || typeof body.number !== "number") throw new GithubApiError("GitHub created the pull request but returned no URL or number."); - return { url: body.html_url, number: body.number }; + return { url: body.html_url, number: body.number, headSha: typeof body.head?.sha === "string" && body.head.sha !== "" ? body.head.sha : null }; }, async closePull(repo, number) { await request("PATCH", `/repos/${repo}/pulls/${number}`, { state: "closed" }); 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..0c9d5541 --- /dev/null +++ b/plugins/stewards/refresh.test.ts @@ -0,0 +1,206 @@ +// 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, classifiedHeadSha, fencedBlock, hasTemplateStamp, laggingPlaybooks, parseContradictions, 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); + }); +}); + +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); + }); +}); + +describe("classifiedHeadSha", () => { + it("reads the head a refresh was classified at, and fails closed otherwise", () => { + expect(classifiedHeadSha("classified at abc1234")).toBe("abc1234"); + expect(classifiedHeadSha("classified at abc1234 — refused: boom")).toBe("abc1234"); + expect(classifiedHeadSha("classified at unreadable")).toBeNull(); + expect(classifiedHeadSha(null)).toBeNull(); + }); +}); diff --git a/plugins/stewards/refresh.ts b/plugins/stewards/refresh.ts new file mode 100644 index 00000000..25364886 --- /dev/null +++ b/plugins/stewards/refresh.ts @@ -0,0 +1,289 @@ +// 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"]); + +/** + * 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 }; + 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, + contradictions: string[] = [], +): 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 (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) { + 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); +} + +/** The prefix `finishPlaybookRefresh` records the classified head sha under in the refresh note. */ +export const CLASSIFIED_AT_PREFIX = "classified at "; + +/** + * The head sha a refresh was classified at, read from its note, or null when + * the note does not name one. A null is fail-closed: `merge` refuses rather + * than squash-merging a head the plugin never read. + */ +export function classifiedHeadSha(note: string | null): string | null { + if (note === null) return null; + const m = /^classified at ([0-9a-f]{7,64})\b/u.exec(note); + return m === null ? null : m[1]!; +} diff --git a/plugins/stewards/server.test.ts b/plugins/stewards/server.test.ts index 54d74ba8..1a81bc26 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"; @@ -17026,3 +17026,193 @@ 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 (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>, 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, claim); + 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); + }); + + 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