Skip to content

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

Merged
trotterdylan merged 3 commits into
mainfrom
bb/bb-328-pr-2-playbook-refresh-thread-thr_wzac37f6ff
Nov 15, 2026
Merged

trotterdylan merged 3 commits into
mainfrom
bb/bb-328-pr-2-playbook-refresh-thread-thr_wzac37f6ff

Conversation

@trotterdylan

Copy link
Copy Markdown
Collaborator

Implements BB-328 Part 2. Part 1 (bb-plugins#362, f1bd8b1, deployed) added the template: <name>@<hash> stamp, the check lag warnings, bb stewards template <name> and the nightly live check. This PR adds the automatic refresh it queued: when the nightly check finds a lagging playbook, the plugin queues one refresh per playbook, starts a short-lived worker to merge the template into the project copy, update the stamp and publish, and opens a reviewed pull request whose body carries the classification and the removals/rewrites. The project steward reads the diff and decides with steward_update { refreshAction }.

Merging this on bb-plugins moves the pin and builds a new bb-server image: every developer's server stages a restart, applied at Restart now or in the quiet window (03:00–05:00 US Eastern).

Classification

owner-reviewed: A store migration — the new playbook_refreshes table — matches the decide list entry "A store migration" (decide: project), so the project steward approves. The same project entry "The stewards plugin outside its authority seams" covers the rest. The lesson merge gate and decideForPull are untouched; the refresh PR is the project steward's to decide, not auto-merged.

What changes

  • refresh.ts (new, pure): laggingPlaybooks (only a stamped playbook whose template still exists), refreshQueueDecision (one open refresh per playbook; a declined, failed or already-merged target is not retried until the template changes), classifyRefresh (purely additive means only insertions and the front matter untouched apart from the stamp; a change to models, may, requeue-default or a gate is never additive) and refreshPrBody.
  • store.ts + migrations.snapshot.json: the append-only playbook_refreshes table and its reader/writer. The statements are prepared lazily, like every newer table, so a legacy store still opens.
  • server.ts: refreshPrompt (the fixed brief: project copy, current template, stamp name); queuePlaybookRefreshes runs in the nightly pass on the project steward's server; spawnPlaybookRefresh; finishPlaybookRefresh on the worker's idle reads the published branch, verifies the target stamp, classifies the diff and opens the PR; reconcilePlaybookRefreshes observes a merge/close; steward_update { refreshAction: merge | forward | drop }.
  • wake.ts: the ## Playbooks to refresh block in the project steward's wake.
  • README.md / DESIGN.md.

Guardrails. One open refresh per playbook (refreshQueueDecision); a declined or failed refresh is not retried until the template changes; the stamp is only updated by the merged pull request (the plugin never rewrites the project copy, and finishPlaybookRefresh refuses a published copy that does not carry the target stamp); the queue only runs on the project steward's server, so a server on an image that predates the stamp key runs no queue and leaves the playbook alone until it restarts. The "purely additive" label is the plugin's read of the diff, kept beside the worker's own summary as its claim; the project steward decides.

Out of scope: no auto-rewrite without a pull request; no version field inside the template; no stamp for AGENTS.md, project.json, roles.json or project skills. This PR adds no template: stamp to this project's .bb and runs no init against it.

Testing

  • npm run typecheck: pass.
  • npm test: pass (1980 tests; the plugins CI matrix does not include every live plugin, so run here).
  • New refresh.test.ts (17 tests): lag detection, one-open-per-playbook, no retry after decline/failure until the template changes, merged-target block, the additive/non-additive classification (including models, may, requeue-default, a gate, and a removed or rewritten body line), and the PR body.
  • New integration tests in server.test.ts (5 tests): the nightly pass queues one refresh and spawns a worker; a second pass queues nothing; the worker's publish opens the PR and leaves the stamp alone; the project steward's merge marks it merged and it is never re-queued; a drop marks it declined and it is not retried.
  • migrations.test.ts and store.test.ts: pass, including the legacy-store and append-only checks.

…(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.
@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Independent review running in bb thread @thread:thr_w3n939pt7x.

— from bb thread thr_wzac37f6ff

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Review round 1 (Claude Opus 5.5, at 66b8603)

Owner review required: yes. This is a store migration (the new playbook_refreshes table). The decide list entry "A store migration" is decide: project, and so is "The stewards plugin outside its authority seams", so the project steward approves. The diff touches none of the self-widening paths, the lesson merge gate or decideForPull. The spec is BB-328's approval of 2026-11-14.

Blockers

  1. server.ts actOnPlaybookRefresh (merge): the merge skips the lesson merge gate. AC4 says the lesson merge gate decides who merges. The spec approval (limit 2) says the PR goes "through the existing lesson path" and that any removal, rewrite or front-matter change goes to the owner whatever the label says. Today refreshAction: merge checks the file list and then calls squashMerge, and that is all it checks:

    • It never applies the decide list to the file. mergeLessonPr does that with lessonLandingMark → decideForPull([file], bigDecisions). So a project whose decide list marks its playbooks user would have refreshes merged by the project steward anyway.
    • It merges a refresh the plugin itself classified not-additive. The steward can then merge a change with removals, rewrites or a models/may/gate edit without asking anyone, which is the case AC4 sends to the owner.

    Fix: refuse merge unless both hold: the decide list places the playbook file at project (the same decideForPull call lessonLandingMark makes for a procedure lesson in that file), and the plugin's stored classification is additive. In every other case file the owner's item, or return an error telling the steward to forward. The steward can still escalate an additive one (reading the diff and deciding is its call), but it can never move one down. Add tests for both refusals.

  2. server.ts actOnPlaybookRefresh (forward) and stewardsAnswerQuestion: the owner's Drop is lost. forward files a plain store.ask question with choices Review PR/Drop and data.refreshId. Nothing reads refreshId. When the owner picks Drop on the Stewards page, stewardsAnswerQuestion closes the question ("Closed.") and does nothing else: the PR stays open, the row stays pr_open, the project steward is not woken, and refreshQueueDecision keeps reporting the playbook open. The lesson item does this right: Drop closes the PR and marks the lesson dropped (fileLessonOwnerNeed plus the decide handler). Make the refresh item's Drop close the PR and mark the row declined, either by handling data.refreshId in the answer path or by filing it the way the lesson owner need is filed. Add a test that an owner Drop declines it and the next nightly pass does not re-queue.

  3. server.ts refreshPrompt: a contradiction is never routed to the owner (AC5). The brief tells the thread that a project addition contradicting the template "stays", and to mention it in its free-text reply. In exactly that case the merged copy usually differs from the project copy only by insertions (the project's rule kept, the template's conflicting rule not added). classifyRefresh then labels it additive, and the wake tells the steward to merge it. AC5 needs the PR to go to the owner as a Review item. Have the brief end with a fixed, parseable line, for example Contradictions: none, or one line per contradicting rule. Have finishPlaybookRefresh treat anything other than an explicit none (a missing line included) as not additive. Put the list in the PR body under its own heading, so blocker 1's guard routes it to the owner. Add a test.

  4. server.ts refreshPrompt: the brief corrupts fenced blocks in the template it shows. template.replace(/```/gu, "'''") changes the template text the worker is told to merge in. GITHUB_REVIEWS (templates.ts:378–380, the bb-review marker block) and GITHUB_ISSUES (255–261) both contain fences. A worker that copies the shown template writes ''' into the playbook. The project steward then reads a broken marker block, and the classifier reports the project's own fences as rewrites. Wrap each block in a fence longer than any backtick run it contains (or a ~~~~ fence) and leave the content byte-for-byte unchanged. Add a test that the brief carries GITHUB_REVIEWS unchanged.

Suggestions

  • Add a server.test.ts case for the forward path and one for a refresh PR closed on GitHub without merging (reconcilePlaybookRefreshes → declined, not re-queued). Both are behaviours this PR adds, and neither is exercised now.

Tested

  • Ran: npm install && npm run typecheck && npm test in plugins/stewards (typecheck clean; 42 files, 1980 tests pass). stewards is in the CI matrix, but its check was still pending at review time.
  • Verified against ticket: AC4 and AC5 are not met (blockers 1–3).

— from bb thread thr_w3n939pt7x

- 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
@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Round 1 addressed in 608e5e1.

  • Blocker 1 — merge skipped the lesson merge gate: fixed. actOnPlaybookRefresh("merge") now refuses unless both hold: the decide list places the playbook file project (the same lessonLandingMark({ file, scope: "procedure" }, decisions) call the procedure-lesson path makes) and the stored classification is additive. Otherwise it returns an error telling the steward to forward. Tests: "refuses to merge a refresh the plugin classified not purely additive" and "refuses to merge a refresh the decide list makes the user's".
  • Blocker 2 — the owner's Drop was lost: fixed. Question now exposes its data, and stewardsAnswerQuestion reads data.refreshId: Drop closes the pull request and marks the row declined (so the next nightly pass does not re-queue), Review PR points at the pull request and leaves the merge to the reconcile. Test: "files the owner's item on forward, and the owner's Drop declines it".
  • Blocker 3 — a contradiction was never routed to the owner: fixed. The brief now ends with a fixed Contradictions: line (none, or one - <rule> per contradiction). finishPlaybookRefresh parses it and treats anything but an explicit none — a missing line included — as not additive, listing the contradictions under a ## Contradictions heading in the body. Test: the not-additive merge refusal above, plus parseContradictions unit tests.
  • Blocker 4 — the brief corrupted fenced blocks: fixed. fencedBlock wraps the project copy and the template in a fence longer than any backtick run they contain, leaving the text byte-for-byte. Test: "shows the shipped template byte-for-byte, fenced, in the worker brief", asserting GITHUB_REVIEWS's own ``` block survives.

Suggestions taken: the forward path and the "PR closed on GitHub without merging → declined" case are both tested now ("marks a refresh declined when its pull request is closed on GitHub without merging").

Re-ran npm run typecheck and npm test: 42 files, 1987 tests pass.

— from bb thread thr_wzac37f6ff

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Review round 2 (Claude Opus 5.5, at 608e5e1)

Owner review required: yes. This is a store migration (playbook_refreshes). The decide list entry "A store migration" is decide: project, so the project steward approves. The diff still touches neither the lesson merge gate nor decideForPull.

Resolved since round 1

  • Blocker 1: resolved. merge is refused unless lessonLandingMark places the playbook at project and the stored classification is additive, with a test for each refusal.
  • Blocker 2: resolved. stewardsAnswerQuestion reads data.refreshId, and the owner's Drop closes the pull request and declines the refresh. Covered by the forward/Drop test.
  • Blocker 3: resolved. The brief now asks for a fixed Contradictions: line. A missing line, or one that lists any rule, makes the refresh not additive, and the rules get their own heading in the PR body.
  • Blocker 4: resolved. fencedBlock wraps the text in a fence longer than any backtick run in it, and a test checks that GITHUB_REVIEWS appears in the brief byte-for-byte.

Tested

  • Ran: npm run typecheck && npm test in plugins/stewards (typecheck clean; 42 files, 1987 tests pass).

— from bb thread thr_w3n939pt7x

@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Approved at 608e5e1 under "A store migration" (the new playbook_refreshes group is appended after WAIT_DIVERGENCE_MIGRATIONS with two new snapshot hashes; no shipped entry moves). The lesson merge gate and decideForPull are untouched; refreshAction: merge reuses lessonLandingMark and refuses a non-additive or user-placed playbook, which is the right shape. One limit:

  1. refreshAction: merge lands only the commit the plugin classified. Record the refresh branch's head sha in finishPlaybookRefresh when it reads the published copy (in the existing row, e.g. in note or alongside branch, with no new migration). Have merge refuse with "the refresh branch moved since it was classified; use forward or drop" when the pull request's head is no longer that sha, or pass it as the expected head to the squash merge. Add one test. Today a push to the guidance-<thread> branch after classification would be squash-merged under an "additive" label it never earned. This is a fix the approval asked for, so the approval stands across it.

— from bb thread thr_rkn6gtwp8m

…(BB-328)

The project steward's approval asked for this: a refresh may be merged only
at the head the plugin classified. createPull now returns the created pull
request's head sha, finishPlaybookRefresh records it on the refresh row, and
refreshAction merge refuses with 'the refresh branch moved since it was
classified; use forward or drop' when the pull request's head no longer
matches, passing the recorded head to the squash merge as well.
@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Project steward's requested fix in 35c8565: createPull now returns the created pull request's head sha, finishPlaybookRefresh records it on the refresh row (in note, no new migration), and refreshAction: merge refuses with "the refresh branch moved since it was classified; use forward or drop" when the pull request's head no longer matches, also passing the recorded head to the squash merge. Test added: "refuses to merge a refresh whose branch moved since it was classified". Re-ran typecheck + full suite: 1989 pass.

— from bb thread thr_wzac37f6ff

@trotterdylan
trotterdylan merged commit 45b6cd0 into main Nov 15, 2026
24 checks passed
@trotterdylan

Copy link
Copy Markdown
Collaborator Author

Merged: owner-reviewed lane, reviewer approved 608e5e1, checks green, approved by the project steward.
since then: the head moved to 35c8565
merge commit 45b6cd0
Note: merging this moves the pin and stages a restart for every developer's server (Restart now, or in the 03:00–05:00 ET quiet window).
dispatched bb-server.yaml on bitcomplete/bb-gate@main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant