From a832c0726c34550c474142ecbae7a7d6d8f76ad1 Mon Sep 17 00:00:00 2001 From: Dylan Trotter Date: Sat, 14 Nov 2026 23:19:25 +0000 Subject: [PATCH] stewards: re-match the deployment rule for every non-terminal watch (BB-337) --- plugins/stewards/deploy.test.ts | 17 +++ plugins/stewards/server.test.ts | 204 ++++++++++++++++++++++++++++++++ plugins/stewards/server.ts | 49 ++++++-- 3 files changed, 262 insertions(+), 8 deletions(-) diff --git a/plugins/stewards/deploy.test.ts b/plugins/stewards/deploy.test.ts index 0c1a9665..21e4623d 100644 --- a/plugins/stewards/deploy.test.ts +++ b/plugins/stewards/deploy.test.ts @@ -294,6 +294,23 @@ describe("recording merged pull requests", () => { expect(awaitingOnRepo(after, "bitcomplete/bb-gate")).toEqual([]); }); + // BB-337: the bb-gate#106 watch was recorded under an older playbook whose + // rule put `.bb/**` under `ref-contains-merge`. The playbook now has + // `live-at-merge` for it, so the next read settles it `deployed` with the + // live note, no deadline and no deployment read. + it("re-matches the bb-gate#106 watch from ref-contains-merge to live-at-merge (BB-337)", () => { + const refTarget: DeploymentTarget = { name: "bb-gate", source: "github", repo: "bitcomplete/bb-gate", environment: "production", carries: [{ when: { repo: "bitcomplete/bb-gate", paths: [".bb/**"] }, strategy: { kind: "ref-contains-merge" } }], timeout: 3_600_000, workflows: [] }; + const liveTarget: DeploymentTarget = { ...refTarget, carries: [{ when: { repo: "bitcomplete/bb-gate", paths: [".bb/**"] }, strategy: { kind: "live-at-merge" } }] }; + const bb106 = { entity: pull({ key: "bitcomplete/bb-gate#106", unit: "bitcomplete/bb-gate" }, { merged: true, mergeCommit: "dc8fb02", files: [".bb/AGENTS.md", ".bb/stewards/playbooks/linear-tickets.md"] }), pulls: [] }; + const before = recordMerged({}, bb106, [refTarget], NOW); + expect(before["bitcomplete/bb-gate#106::bb-gate"]).toMatchObject({ state: "merged", deadlineAt: NOW + refTarget.timeout }); + const after = recordMerged(before, bb106, [liveTarget], NOW + 1000); + expect(after["bitcomplete/bb-gate#106::bb-gate"]).toMatchObject({ state: "deployed", deadlineAt: null, note: "live-at-merge: nothing deploys .bb/**" }); + // No deployment read is needed: nothing times out and nothing awaits. + expect(timeoutsDue(after, NOW + liveTarget.timeout)).toEqual([]); + expect(awaitingOnRepo(after, "bitcomplete/bb-gate")).toEqual([]); + }); + it("does not record an open pull request, and reads a ticket's linked merged ones", () => { expect(recordMerged({}, observed({ entity: pull({ key: "acme/web#7" }, { merged: false }) }), [target], NOW)).toEqual({}); const linked = recordMerged({}, observed({ entity: pull({ key: "acme/web#7", kind: "pull" }, { merged: false }), pulls: [pull({ key: "acme/web#8" }, { merged: true })] }), [target], NOW); diff --git a/plugins/stewards/server.test.ts b/plugins/stewards/server.test.ts index 96b850c6..d145354c 100644 --- a/plugins/stewards/server.test.ts +++ b/plugins/stewards/server.test.ts @@ -12465,6 +12465,40 @@ describe("deployment watching (BB-53)", () => { }; const deployment = (over: Record = {}) => ({ id: 7, environment: "production", ref: "main", sha: "bbbbbbb2", state: "success", url: "https://github.com/acme/bb-gate/actions/runs/9", ...over }); const githubInterest = (t: Awaited>) => t.rpcCalls.filter((c) => c.method === "sourceSetInterest" && c.pluginId === "github-watcher").at(-1)!.input as { deployment: string[] }; + /** A deployment watch fixture (BB-337), with the fields a re-match reads. */ + const watchFixture = (now: number, over: Record = {}) => ({ + key: "acme/web#7", + state: "merged", + mergeSha: "aaaaaaa1", + target: "", + mergedAt: now, + prRepo: "acme/web", + files: [] as string[], + repo: "acme/bb-gate", + environment: "production", + deploymentId: null, + deploymentState: null, + ref: null, + sha: null, + url: null, + at: now, + deadlineAt: now + 3_600_000, + note: null, + workflowRuns: [] as string[], + ...over, + }); + /** A ticket binding with no linked pull, seeded with one deployment watch (BB-337). */ + const seededWatch = async (t: Awaited>, watch: Record, prevPull?: ReturnType) => { + t.threads.set("thr_steward", { status: "idle", archivedAt: null }); + t.entities.set("uuid-12", issue({ id: "uuid-12", key: "ENG-12", state: "In Progress", stateType: "started" })); + const b = t.hooks.store.create({ projectId: "p1", source: "linear", entityId: "uuid-12", entityKey: "ENG-12", url: null, title: "ENG-12", kind: "ticket", playbook: "linear-tickets", playbookVersion: "v1", parentBindingId: null, phase: "In Progress", stage: "build" }); + t.hooks.store.patch(b.id, { threadId: "thr_steward" }); + t.hooks.store.patch(b.id, { deployments: { "acme/web#7": watch as never } }); + // The last snapshot keeps the watch's pull after a later read drops it, so + // the re-match can still name its base (BB-337). + if (prevPull !== undefined) t.hooks.store.patch(b.id, { lastSnapshot: { entity: t.entities.get("uuid-12")!, pulls: [prevPull], at: t.now() } as never }); + return b; + }; it("records a merge as awaiting deployment, leases the target repo, and shows the state in the wake", async () => { const t = await setup(); @@ -12744,6 +12778,176 @@ describe("deployment watching (BB-53)", () => { expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "deploying", deploymentId: 7, deploymentState: "success" }); }); + // BB-337: the re-match runs for a `merged` watch whose pull is no longer in + // the look's snapshot, using the watch's own stored `prRepo` and `files`. + // BB-286 covered the observed case; this is the gap. + it("re-matches a merged watch whose pull has left the snapshot (BB-337)", async () => { + const t = await setup(); + t.files[playbookPath("linear-tickets")] = structuredPlaybook(' - when: { repo: acme/web, paths: [.bb/**] }\n strategy: { kind: live-at-merge }'); + t.hooks.forgetConfigs(); + await seededWatch(t, watchFixture(t.now(), { files: [".bb/stewards/project.json"] }), pull({ key: "acme/web#7" }, { merged: true, mergeCommit: "aaaaaaa1", base: "main", files: [".bb/stewards/project.json"] })); + await t.hooks.reconcilePass("p1"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "deployed", deadlineAt: null, note: "live-at-merge: nothing deploys .bb/**" }); + // The verdict is announced, not only stored. + expect(t.sends.filter((s) => s.threadId === "thr_steward").at(-1)!.text).toMatch(/Deployed at merge: acme\/web#7/u); + }); + + // BB-337 review round 1: a stacked merge whose pull has left the snapshot + // must not be settled from an unknown base. Its last snapshot names the + // non-default base, so it stays awaiting. + it("leaves an unobserved stacked merged watch awaiting (BB-337)", async () => { + const t = await setup(); + t.files[playbookPath("linear-tickets")] = structuredPlaybook(' - when: { repo: acme/web, paths: [.bb/**] }\n strategy: { kind: live-at-merge }'); + t.hooks.forgetConfigs(); + await seededWatch(t, watchFixture(t.now(), { files: [".bb/AGENTS.md"] }), pull({ key: "acme/web#7" }, { merged: true, mergeCommit: "aaaaaaa1", base: "feature-base", files: [".bb/AGENTS.md"] })); + await t.hooks.reconcilePass("p1"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "merged" }); + }); + + // BB-337: a free-text `carries` is the steward's; the re-match never touches + // a watch under one. + it("leaves a free-text carries watch with the steward (BB-337)", async () => { + const t = await setup(); + t.files[playbookPath("linear-tickets")] = deploymentPlaybook(); + t.hooks.forgetConfigs(); + await seededWatch(t, watchFixture(t.now(), { files: [".bb/stewards/project.json"] })); + await t.hooks.reconcilePass("p1"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "merged", note: null }); + }); + + // BB-337: a `merged` watch with a non-final deployment's ref must not be + // settled by it; only a live-at-merge rule settles a `merged` watch in the + // re-match, and the deployment read moves it to `deploying` when one lands. + it("does not settle a merged watch from a non-final deployment's ref (BB-337)", async () => { + const t = await setup(); + t.files[playbookPath("linear-tickets")] = structuredPlaybook(' - when: { repo: acme/web }\n strategy: { kind: ref-contains-merge }'); + t.hooks.forgetConfigs(); + await seededWatch(t, watchFixture(t.now(), { ref: "main", deploymentId: 7, deploymentState: "in_progress" })); + // If the re-match compared this ref, it would read identical and settle. + t.sourceCompares.set("acme/bb-gate aaaaaaa1 main", "identical"); + await t.hooks.reconcilePass("p1"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "merged", deploymentState: "in_progress" }); + }); + + // BB-337: the re-match covers a multi-target keyed watch and a legacy + // pull-only key, from their stored `prRepo` and `files`. + it("re-matches multi-target and legacy watch keys (BB-337)", async () => { + const t = await setup(); + t.files[playbookPath("linear-tickets")] = LINEAR_TICKETS.replace( + "requeue-default: 2d", + [ + "requeue-default: 2d", + "deployment:", + " - name: bb-gate", + " source: github", + " repo: acme/bb-gate", + " environment: production", + " carries:", + " - when: { repo: acme/web, paths: [.bb/**] }", + " strategy: { kind: live-at-merge }", + " timeout: 1h", + " - name: devbox", + " source: github", + " repo: bitcomplete/devbox", + " environment: production", + " carries:", + " - when: { repo: acme/web }", + " strategy: { kind: ref-contains-merge }", + " timeout: 1h", + ].join("\n"), + ); + t.hooks.forgetConfigs(); + t.threads.set("thr_steward", { status: "idle", archivedAt: null }); + t.entities.set("uuid-12", issue({ id: "uuid-12", key: "ENG-12", state: "In Progress", stateType: "started" })); + const b = t.hooks.store.create({ projectId: "p1", source: "linear", entityId: "uuid-12", entityKey: "ENG-12", url: null, title: "ENG-12", kind: "ticket", playbook: "linear-tickets", playbookVersion: "v1", parentBindingId: null, phase: "In Progress", stage: "build" }); + t.hooks.store.patch(b.id, { threadId: "thr_steward" }); + t.hooks.store.patch(b.id, { + deployments: { + // A keyed watch on the live target, a legacy pull-only one on it, and + // a keyed watch on the target whose rule still needs a deployment. + "acme/web#7::bb-gate": watchFixture(t.now(), { target: "bb-gate", repo: "acme/bb-gate", files: [".bb/AGENTS.md"] }) as never, + "acme/web#7": watchFixture(t.now(), { target: "", repo: "acme/bb-gate", files: [".bb/AGENTS.md"] }) as never, + "acme/web#7::devbox": watchFixture(t.now(), { target: "devbox", repo: "bitcomplete/devbox", files: [".bb/AGENTS.md"] }) as never, + }, + }); + // The last snapshot keeps the pull, so the re-match can name its base. + t.hooks.store.patch(b.id, { lastSnapshot: { entity: t.entities.get("uuid-12")!, pulls: [pull({ key: "acme/web#7" }, { merged: true, mergeCommit: "aaaaaaa1", base: "main", files: [".bb/AGENTS.md"] })], at: t.now() } as never }); + await t.hooks.reconcilePass("p1"); + const d = t.hooks.store.find("ENG-12")!.deployments; + expect(d["acme/web#7::bb-gate"]).toMatchObject({ state: "deployed", note: "live-at-merge: nothing deploys .bb/**" }); + expect(d["acme/web#7"]).toMatchObject({ state: "deployed", note: "live-at-merge: nothing deploys .bb/**" }); + expect(d["acme/web#7::devbox"]).toMatchObject({ state: "merged" }); + }); + + // BB-337: a terminal watch is never touched by the re-match. + it("never re-matches a terminal watch (BB-337)", async () => { + const t = await setup(); + t.files[playbookPath("linear-tickets")] = structuredPlaybook(' - when: { repo: acme/web, paths: [.bb/**] }\n strategy: { kind: live-at-merge }'); + t.hooks.forgetConfigs(); + const b = await seededWatch(t, watchFixture(t.now(), { files: [".bb/stewards/project.json"] })); + for (const state of ["deployed", "failed"] as const) { + t.hooks.store.patch(b.id, { deployments: { "acme/web#7": watchFixture(t.now(), { state, files: [".bb/stewards/project.json"], note: "settled" }) as never } }); + await t.hooks.reconcilePass("p1"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state, note: "settled" }); + } + }); + + // BB-337: a stacked merge is not on the default branch, so the re-match + // leaves it awaiting rather than settling it live. + it("leaves a stacked merged watch awaiting (BB-337)", async () => { + const t = await setup(); + t.files[playbookPath("linear-tickets")] = structuredPlaybook(' - when: { repo: acme/web, paths: [.bb/**] }\n strategy: { kind: live-at-merge }'); + t.hooks.forgetConfigs(); + t.threads.set("thr_steward", { status: "idle", archivedAt: null }); + t.entities.set("uuid-12", issue({ id: "uuid-12", key: "ENG-12", state: "In Progress", stateType: "started", linkedPulls: [{ url: "https://github.com/acme/web/pull/7", key: "acme/web#7" }] })); + t.entities.set("acme/web#7", pull({ key: "acme/web#7" }, { merged: true, mergeCommit: "aaaaaaa1", base: "feature-base", files: [".bb/AGENTS.md"] })); + const b = t.hooks.store.create({ projectId: "p1", source: "linear", entityId: "uuid-12", entityKey: "ENG-12", url: null, title: "ENG-12", kind: "ticket", playbook: "linear-tickets", playbookVersion: "v1", parentBindingId: null, phase: "In Progress", stage: "build" }); + t.hooks.store.patch(b.id, { threadId: "thr_steward" }); + t.hooks.store.patch(b.id, { deployments: { "acme/web#7": watchFixture(t.now(), { files: [".bb/AGENTS.md"] }) as never } }); + await t.hooks.reconcilePass("p1"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "merged" }); + }); + + // BB-337: a reconnect reads each open target once, with the source's own + // cap, and a steady-state look adds no source call once the watch settled. + it("reads each open target once on a reconnect and no source call in steady state (BB-337)", async () => { + const t = await setup(); + merged(t); + t.files[playbookPath("linear-tickets")] = structuredPlaybook(' - when: { repo: acme/web }\n strategy: { kind: ref-contains-merge }'); + t.hooks.forgetConfigs(); + await t.ring("linear", "uuid-12"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "merged" }); + const reads = () => t.rpcCalls.filter((c) => c.method === "sourceDeployments").length; + // The final deployment already exists at the source when the reconnect rings. + t.sourceDeployments.set("acme/bb-gate production", [deployment()]); + t.sourceCompares.set("acme/bb-gate aaaaaaa1 main", "identical"); + const before = reads(); + await t.ring("github", "acme/bb-gate"); + expect(reads() - before).toBe(1); + const read = t.rpcCalls.filter((c) => c.method === "sourceDeployments").at(-1)!; + expect(read.input).toMatchObject({ repo: "acme/bb-gate", environment: "production", limit: 10 }); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "deployed" }); + // Steady state: the watch is settled, so the next look reads nothing. + const settled = reads(); + await t.hooks.reconcilePass("p1"); + expect(reads()).toBe(settled); + }); + + // BB-337: the first reconcile after a plugin start reads each open target + // once, the same bounded read the reconnect triggers. `sweep()` is what a + // start runs. + it("reads each open target once on the first look after a start (BB-337)", async () => { + const t = await setup(); + t.files[playbookPath("linear-tickets")] = structuredPlaybook(' - when: { repo: acme/web }\n strategy: { kind: ref-contains-merge }'); + t.hooks.forgetConfigs(); + await seededWatch(t, watchFixture(t.now())); + const reads = () => t.rpcCalls.filter((c) => c.method === "sourceDeployments").length; + const before = reads(); + await t.hooks.sweep(); + await t.flush(); + expect(reads() - before).toBe(1); + }); + // BB-204: a success whose image is behind the merge leaves the watch where // it was and does not wake the steward. it("keeps a success whose image is behind the merge quiet (BB-204)", async () => { diff --git a/plugins/stewards/server.ts b/plugins/stewards/server.ts index f18be7cf..3cf1e2f0 100644 --- a/plugins/stewards/server.ts +++ b/plugins/stewards/server.ts @@ -127,7 +127,7 @@ import { type WorkflowRun, } 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, matchCarriesRule, noRuleNoted, type CarriesReader } from "./carries.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 WakeRetry, type Worker } from "./store.js"; import { hasLessonAnchor, landLessonInstruction, lessonAnchor, lessonLandingMark, planLessonTransitions, planValidations, retiredLessonAnchor, withLessonAnchor } from "./lessons.js"; import { AFTER_THE_FACT_RATIONALE_WHY, REPEAT_RATIONALE_WHY, ROUTINE_RATIONALE_WHY, clusterKey, routineRationaleWanted } from "./rationale.js"; @@ -4903,7 +4903,10 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P let isDefaultBase: ((prRepo: string, base: string | null) => boolean) | undefined; if (wantsLive) { const branches = new Map(); - for (const p of mergedPulls(observed)) { + // BB-337: a watch whose pull has left the current snapshot keeps its + // base in the last snapshot, so the default-branch check reads that + // repo too. The read is once per repo, as before. + for (const p of [...mergedPulls(observed), ...(prev === null ? [] : mergedPulls(prev))]) { if (branches.has(p.unit)) continue; branches.set(p.unit, await github.defaultBranch(p.unit).catch(() => null)); } @@ -4969,17 +4972,46 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P b = store.note(b.id, [line, ...evaluated.notes].join("; "), !evaluated.wake); if (evaluated.wake) bb.log.info(`stewards: deployment ${applied.deployment.id} (${state}) on ${target.repo} ${target.environment} concerns ${applied.changed.join(", ")} (read)`); } - // BB-132/BB-204: a watch left `deploying` by an earlier pass is - // re-evaluated, so a transient read failure is retried, a playbook - // that adds a rule settles a no-rule watch, and a re-resolved merge - // commit is picked up. A watch the read just evaluated is skipped, and - // a missing rule already noted does not wake again. + // BB-132/BB-204/BB-337: a watch left `merged` or `deploying` by an + // earlier pass is re-evaluated, so a transient read failure is + // retried, a playbook that adds a rule settles a no-rule watch, a + // re-resolved merge commit is picked up, and a rule that became + // `live-at-merge` settles a watch with no deployment to compare. The + // rule is matched from the watch's own stored `prRepo` and `files`, + // so a watch whose pull is no longer in the snapshot is re-matched + // too. A watch the read just evaluated is skipped, a free-text rule + // stays with the steward, a terminal watch is never touched, and a + // missing rule already noted does not wake again. const reader = carriesReader(); + const liveSettled: string[] = []; for (const [id, watch] of Object.entries(deployments)) { - if (watch.state !== "deploying") continue; + if (watch.state !== "merged" && watch.state !== "deploying") continue; if (evaluatedIds.has(id)) continue; const target = deployTargets.find((t) => (watch.target === "" ? matchesTarget(t, watch.repo, watch.environment) : targetKeyOf(t) === watch.target)); if (target === undefined || typeof target.carries === "string") continue; + // BB-337: a `merged` watch is re-matched against the rule from its + // stored `prRepo` and `files`. A `live-at-merge` rule settles it + // `deployed` with the live note and no deadline, without a read, + // exactly as `recordMerged` does for a pull still in the snapshot. + // A non-live rule has no final deployment to compare here, so the + // watch is left as it was (the deployment read below moves it to + // `deploying` when one settles). A merge whose known base is not the + // default is stacked and stays awaiting, as `recordMerged` leaves it. + if (watch.state === "merged") { + const matched = matchCarriesRule(target.carries, watch.prRepo, watch.files); + const live = matched === null ? null : liveAtMergeVerdict(matched); + if (live === null) continue; + const knownPull = mergedPulls(observed).find((p) => lc(p.key) === lc(watch.key)) ?? (prev === null ? undefined : mergedPulls(prev).find((p) => lc(p.key) === lc(watch.key))); + // BB-260: a base the read could not name is not treated as default. + // A pull in neither the snapshot nor the last one leaves the watch + // awaiting rather than settling it on an unknown base. + if (knownPull === undefined || isDefaultBase === undefined || !isDefaultBase(watch.prRepo, knownPull.pull?.base ?? null)) continue; + deployments = { ...deployments, [id]: { ...watch, state: "deployed", at: now(), deadlineAt: null, note: live.note } }; + b = store.patch(b.id, { deployments }); + liveSettled.push(`${watch.key}${target.name === undefined ? "" : ` (${target.name})`} — ${live.note}`); + bb.log.info(`stewards: carries deployed for ${watch.key}: ${live.note}`); + continue; + } const verdict = await evaluateCarries(target, { merge: watch.mergeSha, prRepo: watch.prRepo, files: watch.files, ref: watch.ref }, reader); if (verdict === null) continue; const label = `${watch.key}${target.name === undefined ? "" : ` (${target.name})`}`; @@ -4997,6 +5029,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P b = store.note(b.id, `Carries not decided for ${label}: ${verdict.note}`, false); } } + if (liveSettled.length > 0) deploymentChanges.push({ text: `Deployed at merge: ${liveSettled.join("; ")}`, actionable: true }); const late = timeoutsDue(deployments, now()); if (late.length > 0) { const labels = late.map((id) => (deployments[id] === undefined ? id : `${deployments[id]!.key}${deployments[id]!.target === "" ? "" : ` (${deployments[id]!.target})`}`));