stewards: refresh a lagging playbook through a reviewed pull request (BB-328)

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 <name>` 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:<name>` 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.

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`:

plugins/stewards/github.ts

@@ -134,7 +134,7 @@ export interface GithubWriter {
    */
   branchExists(repo: string, branch: string): Promise<boolean>;
   /** 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<void>;
   /**
@@ -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" });

plugins/stewards/migrations.snapshot.json

@@ -140,5 +140,7 @@
   "d856f2127e2c5788a01c5ca80b216d1dc9be2a67bb273ad9082bbc69a279ce9a",
   "f1636e9715f38e19e3c21cc7d3008c4f8d734ec26e03eb983961acb7d4cc8949",
   "5aeed47ee4b133a8d26b01316c545a6f4c49b3c73fb9511cb4c2b9eae55a31fa",
-  "ded6270fedbb7d853a412b2167f164b3886f38ed975c124d83b40f6d625500ad"
+  "ded6270fedbb7d853a412b2167f164b3886f38ed975c124d83b40f6d625500ad",
+  "91adbc2f755ca9d634c4865855b06a5f3285b36cc7d3cb896b48b5251ca494f7",
+  "b8f990c5286fd23a4272bb1d25b4c9bb4ef61e584d29c61598a42561eee34e09"
 ]

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>): 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();
+  });
+});