diff --git a/plugins/github-watcher/server.test.ts b/plugins/github-watcher/server.test.ts index fac37330..d3cc2ea9 100644 --- a/plugins/github-watcher/server.test.ts +++ b/plugins/github-watcher/server.test.ts @@ -870,6 +870,46 @@ describe("stewards' source contract", () => { await service.done; }); + // BB-286: a reconnect re-check rings stewards for every repo whose + // deployment doorbells it holds, so a deployment doorbell dropped while + // the subscriber was away is re-read rather than waiting for the sweep. + it("rings stewards for each deployment interest repo on the connected sweep (BB-286)", async () => { + const w = await setup(WITH_HOOKS); + const service = w.harness.runService("watches"); + await tick(); + w.hooks.last().send(w.hooks.hello("c_1")); + await tick(); + expect(await w.harness.callRpc("sourceSetInterest", { units: [], entities: [], dependabot: [], deployment: ["acme/bb-gate"] }, STEWARDS)).toEqual({ ok: true }); + await tick(); + // A new connection re-checks; the deployment interest is rung once. + w.hooks.last().send(w.hooks.hello("c_2")); + await tick(3000); + expect(w.rpcs()).toEqual([{ pluginId: "stewards", method: "stewardsRing", input: { source: "github", id: "acme/bb-gate" } }]); + service.controller.abort(); + await service.done; + }); + + // BB-286: the deployment queue holds the latest object per deployment id, + // so two statuses inside the debounce hand stewards the newest, not the + // first. + it("hands stewards the newest of two deployment statuses inside the debounce (BB-286)", async () => { + const w = await setup(WITH_HOOKS); + const service = w.harness.runService("watches"); + await tick(); + w.hooks.last().send(w.hooks.hello("c_1")); + await tick(); + expect(await w.harness.callRpc("sourceSetInterest", { units: [], entities: [], dependabot: [], deployment: ["acme/bb-gate"] }, STEWARDS)).toEqual({ ok: true }); + await tick(); + const inProgress = { id: 7, environment: "production", ref: "main", sha: "abc123", state: "in_progress", url: "https://github.com/acme/bb-gate/actions/runs/9" }; + const success = { ...inProgress, state: "success" }; + w.hooks.last().send(`event: doorbell\ndata: ${JSON.stringify({ source: "github", kind: "deployment", id: "Acme/bb-gate#7", unit: "Acme/bb-gate", deployment: inProgress })}\n\n`); + w.hooks.last().send(`event: doorbell\ndata: ${JSON.stringify({ source: "github", kind: "deployment", id: "Acme/bb-gate#7", unit: "Acme/bb-gate", deployment: success })}\n\n`); + await tick(3000); + expect(w.rpcs()).toEqual([{ pluginId: "stewards", method: "stewardsRing", input: { source: "github", id: "Acme/bb-gate#7", deployment: success } }]); + service.controller.abort(); + await service.done; + }); + it("leases what stewards asked for after a restart, beside the watches", async () => { const w = await setup({ ...WITH_HOOKS, kv: { "stewards/interest": { units: ["acme/gadgets"], entities: ["acme/widgets#12"] } } }); await w.tool({ ref: "Acme/Widgets#12" }); diff --git a/plugins/github-watcher/server.ts b/plugins/github-watcher/server.ts index 89e43865..3aef82aa 100644 --- a/plugins/github-watcher/server.ts +++ b/plugins/github-watcher/server.ts @@ -323,6 +323,13 @@ export function createGitHubPlugin(deps: GitHubPluginDeps): (bb: BbPluginApi) => const keys = watches.keys(); if (keys.length > 0) bb.log.info(`checking ${keys.length} watched GitHub pull request${keys.length === 1 ? "" : "s"} or issue${keys.length === 1 ? "" : "s"} (${reason})`); await checks.sweep(keys); + // BB-286: a reconnect re-check must also ring stewards for every repo + // whose deployment doorbells it holds. A deployment doorbell dropped + // while this server was away is otherwise only found by the half-hourly + // sweep; the ring makes stewards re-read that repo's deployments now. + if (reason === "connected") { + for (const repo of interest.deployment) rings.ring(repo); + } } // ---- what stewards wants to hear about ------------------------------------ diff --git a/plugins/stewards/deploy.test.ts b/plugins/stewards/deploy.test.ts index 8d9acc70..14c8a13b 100644 --- a/plugins/stewards/deploy.test.ts +++ b/plugins/stewards/deploy.test.ts @@ -255,6 +255,23 @@ describe("recording merged pull requests", () => { expect(recorded["bitcomplete/bb-gate#20::bb-gate"]).toMatchObject({ state: "merged", mergeSha: "stacked1", deadlineAt: NOW + liveAtMergeTarget.timeout }); }); + // BB-286: a watch recorded under an older playbook is re-matched against + // the current one on every read, so a rule that is now live-at-merge + // settles it with no deployment read. The first-record contract above still + // holds when the rule has not changed. + it("re-matches a pre-existing merged watch when the playbook's rule becomes live-at-merge (BB-286)", () => { + const refTarget: DeploymentTarget = { name: "bb-gate", source: "github", repo: "bitcomplete/bb-gate", environment: "production", carries: [{ when: { repo: "acme/web" }, strategy: { kind: "ref-contains-merge" } }], timeout: 3_600_000, workflows: [] }; + const liveTarget: DeploymentTarget = { ...refTarget, carries: [{ when: { repo: "acme/web", paths: [".bb/**"] }, strategy: { kind: "live-at-merge" } }] }; + const mergedPull = { entity: pull({ key: "acme/web#7" }, { merged: true, mergeCommit: "aaaaaaa1", files: [".bb/stewards/project.json"] }), pulls: [] }; + const before = recordMerged({}, mergedPull, [refTarget], NOW); + expect(before["acme/web#7::bb-gate"]).toMatchObject({ state: "merged", deadlineAt: NOW + refTarget.timeout }); + const after = recordMerged(before, mergedPull, [liveTarget], NOW + 1000); + expect(after["acme/web#7::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/deploy.ts b/plugins/stewards/deploy.ts index 2dd58a0a..74d64edd 100644 --- a/plugins/stewards/deploy.ts +++ b/plugins/stewards/deploy.ts @@ -19,6 +19,13 @@ import type { Observed } from "./store.js"; export const DEPLOY_STATES = ["merged", "deploying", "deployed", "failed"] as const; export type DeployState = (typeof DEPLOY_STATES)[number]; +/** + * Whether a deployment's own last status is final (BB-286). A non-final + * status (`in_progress`, `created`, `queued`, the empty state) has not + * applied the image yet, so a watch that last read one keeps being polled. + */ +export const isFinalDeploymentState = (state: string | null | undefined): boolean => state === "success" || state === "failure" || state === "error"; + export const deploymentWatchSchema = z.object({ /** The pull request's key, e.g. "acme/web#12"; the map's key as well. */ key: z.string(), @@ -113,6 +120,13 @@ function liveVerdict(target: DeploymentTarget, prRepo: string, files: readonly s * the source now knows one and the stored one differs (a watch recorded * before BB-132 holds the pull request head, which a squash merge rewrites). * + * An existing watch is also re-matched against the playbook's current rule + * on every read (BB-286): a watch recorded under an older playbook keeps the + * old rule otherwise. When the rule that now matches its stored `prRepo` and + * `files` is `live-at-merge` and the merge is on the default branch, the + * watch settles `deployed` with the live note and no deadline, so no + * deployment is read for it. A stacked merge stays awaiting. + * * A target whose structured `carries` has no rule for the merge is not * applicable and is skipped; a watch already stored for it is dropped, so it * cannot block closing the ticket. When no target applies at all, every @@ -195,6 +209,22 @@ export function recordMerged( next[id] = { ...existing, state: reevaluate ? "deploying" : existing.state, mergeSha: merge, note }; changed = true; } + // BB-286: re-match the watch against the playbook's current rule. + // The stored `prRepo` and `files` decide applicability, as they do + // above. When the matching rule is now `live-at-merge` and the merge + // is on the default branch, it is live and settles with the live note + // and no deadline, without any deployment read. A stacked merge + // (`onDefault` false) keeps awaiting its own merge to reach `main`. + const current = next[id] ?? existing; + if (onDefault && current.state !== "deployed" && current.state !== "failed" && typeof target.carries !== "string") { + const context = contextFor(target); + const matched = matchCarriesRule(target.carries, context.prRepo, context.files); + const live = matched === null ? null : liveAtMergeVerdict(matched); + if (live !== null) { + next[id] = { ...current, state: "deployed", at: now, deadlineAt: null, note: live.note }; + changed = true; + } + } continue; } // BB-260: a `live-at-merge` rule names paths nothing builds or applies, diff --git a/plugins/stewards/reconcile.test.ts b/plugins/stewards/reconcile.test.ts index 2aa096a0..463060a0 100644 --- a/plugins/stewards/reconcile.test.ts +++ b/plugins/stewards/reconcile.test.ts @@ -1068,6 +1068,14 @@ describe("idle in an active stage (BB-83)", () => { expect(idleInActiveStage(binding({ requeueUntil: at }), merged, at + 1)).toBe(true); }); + // BB-285: a gate-met own pull request is the plugin's to act on, so it does + // not suppress the idle wake the way an open one does. + it("does not suppress the idle wake for a gate-met own pull request", () => { + const open = { entity: issue({ state: "In Review" }), pulls: [pull({ state: "open" })] }; + expect(idleInActiveStage(binding(), open)).toBe(false); + expect(idleInActiveStage(binding({ gateMetPulls: new Set(["acme/web#7"]) }), open)).toBe(true); + }); + it("starts a new idle period when an own pull request closes", () => { const before = { entity: issue({ state: "In Review" }), pulls: [pull({ state: "open" })] }; expect(ownPullClosed(before, merged, own)).toBe(true); diff --git a/plugins/stewards/reconcile.ts b/plugins/stewards/reconcile.ts index 3f4054fd..197f63c2 100644 --- a/plugins/stewards/reconcile.ts +++ b/plugins/stewards/reconcile.ts @@ -670,6 +670,12 @@ export function unheardWait(status: Binding["status"], open: readonly Escalation export type IdleBinding = Pick & { workers: readonly Pick[]; ownPulls: ReadonlySet; + /** + * Lowercased keys of the binding's own pull requests whose gate is met + * (BB-285). A gate-met own pull request is not something to wait on: the + * plugin acts on it, so it does not suppress the idle wake. + */ + gateMetPulls?: ReadonlySet; escalations: readonly Pick[]; needs: readonly unknown[]; }; @@ -697,7 +703,7 @@ export function idleInActiveStage(b: IdleBinding, observed: Observed, now: numbe // "nothing is being waited on" before then. if (b.requeueUntil !== null && b.requeueUntil > now) return false; if (b.workers.some((w) => w.status === "running")) return false; - if ([observed.entity, ...observed.pulls].some((e) => pullOpen(e) && ownPull(e.key, e.url, b.ownPulls))) return false; + if ([observed.entity, ...observed.pulls].some((e) => pullOpen(e) && ownPull(e.key, e.url, b.ownPulls) && !(b.gateMetPulls?.has(e.key.toLowerCase()) ?? false))) return false; if (Object.values(b.deployments).some((w) => w.state === "merged" || w.state === "deploying")) return false; if (b.escalations.some((x) => x.status === "open" && x.route === "user")) return false; if (b.needs.length > 0) return false; diff --git a/plugins/stewards/server.test.ts b/plugins/stewards/server.test.ts index 39d481c2..b68445b6 100644 --- a/plugins/stewards/server.test.ts +++ b/plugins/stewards/server.test.ts @@ -12121,6 +12121,105 @@ describe("deployment watching (BB-53)", () => { expect(t.hooks.store.find("ENG-12")!.pending.some((n) => n.quiet === true && /created/.test(n.text))).toBe(true); }); + // BB-286: a non-final doorbell used to note quietly and stop; a dropped + // final doorbell then waited for the half-hourly sweep. The doorbell path + // now schedules a re-read, so the final status is caught within the + // debounce. + it("re-reads after a non-final deployment doorbell and settles on the final status (BB-286)", async () => { + const t = await setup(); + merged(t); + await t.ring("linear", "uuid-12"); + // The doorbell is in_progress, but the deployment source already has the + // success: the scheduled re-read settles the watch with no second doorbell. + t.sourceDeployments.set("acme/bb-gate production", [deployment()]); + await doorbell(t, "acme/bb-gate#7", deployment({ state: "in_progress" })); + await t.flush(); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "deploying", deploymentState: "success", deploymentId: 7 }); + }); + + // BB-286: the re-read repeats while the last read was non-final. The first + // re-read still sees in_progress; the poll settles it when it turns success + // later, rather than waiting for the half-hourly sweep. + it("keeps polling while the deployment is non-final and settles when it turns final (BB-286)", async () => { + const t = await setup({ timing: { debounceMs: 1, nightlyStaggerMs: 0, deploymentPollMs: 20 } }); + merged(t); + await t.ring("linear", "uuid-12"); + // The source is still in_progress for the doorbell and the first re-read. + t.sourceDeployments.set("acme/bb-gate production", [deployment({ state: "in_progress" })]); + await doorbell(t, "acme/bb-gate#7", deployment({ state: "in_progress" })); + await t.flush(); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "merged", deploymentState: "in_progress" }); + // The final status lands later; the poll re-reads and settles it. + t.sourceDeployments.set("acme/bb-gate production", [deployment({ state: "success" })]); + await new Promise((r) => setTimeout(r, 30)); + await t.flush(); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "deploying", deploymentState: "success", deploymentId: 7 }); + }); + + // BB-286: the same re-read on the sweep read path. + it("re-reads after a non-final deployment status found on the sweep (BB-286)", async () => { + const t = await setup(); + merged(t); + await t.ring("linear", "uuid-12"); + t.sourceDeployments.set("acme/bb-gate production", [deployment({ state: "in_progress" })]); + await t.hooks.sweep(); + // The sweep noted in_progress and scheduled a re-read; the source has the + // final status by the time it runs. + t.sourceDeployments.set("acme/bb-gate production", [deployment()]); + await t.flush(); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "deploying", deploymentState: "success", deploymentId: 7 }); + }); + + // BB-286: the deploy debounce keys on the deployment id and state, so two + // statuses inside the window both run and the newest wins. A first-wins + // debounce kept only the in_progress closure and dropped the success. + it("keeps the newest of two deployment statuses inside the debounce (BB-286)", async () => { + const t = await setup(); + merged(t); + await t.ring("linear", "uuid-12"); + const send = (state: string) => + t.harness.callRpc("stewardsRing", { source: "github", id: "acme/bb-gate#7", deployment: deployment({ state }) }, { experimental_caller: { kind: "plugin", pluginId: "github-watcher" } }); + // Both inside the same debounce window, before either timer fires. + await Promise.all([send("in_progress"), send("success")]); + await t.flush(); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "deploying", deploymentState: "success", deploymentId: 7 }); + }); + + // BB-286: a watch recorded under an older rule is re-matched against the + // playbook on every read. When the rule is now live-at-merge it settles with + // the live note and no deadline, and the steward hears "Deployed at merge". + it("settles a pre-existing merged watch when the playbook's rule becomes live-at-merge (BB-286)", async () => { + const t = await setup(); + t.files[playbookPath("linear-tickets")] = structuredPlaybook(" - when: { repo: acme/web }\n strategy: { kind: ref-contains-merge }"); + t.entities.set("uuid-12", issue({ id: "uuid-12", key: "ENG-12", state: "In Progress", 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", files: [".bb/stewards/project.json"] })); + await t.ring("linear", "uuid-12"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "merged" }); + const steward = t.spawns[1]!.id; + // The rule becomes live-at-merge; the next read settles it without any + // deployment read. Forget the cached config so the new playbook is read. + t.files[playbookPath("linear-tickets")] = structuredPlaybook(' - when: { repo: acme/web, paths: [.bb/**] }\n strategy: { kind: live-at-merge }'); + t.hooks.forgetConfigs(); + await t.ring("linear", "uuid-12"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "deployed", deadlineAt: null, note: "live-at-merge: nothing deploys .bb/**" }); + const wake = t.sends.filter((s) => s.threadId === steward).at(-1)!.text; + expect(wake).toMatch(/Deployed at merge: acme\/web#7/u); + }); + + // BB-286: github-watcher's reconnect sweep rings stewards once per repo with + // a deployment interest, and the repo ring re-reads that repo's deployments + // (BB-59) rather than waiting for the half-hourly sweep. + it("re-reads deployments for a repo when github-watcher rings it on reconnect (BB-286)", async () => { + const t = await setup(); + merged(t); + await t.ring("linear", "uuid-12"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "merged" }); + // The final deployment already exists at the source when the ring arrives. + t.sourceDeployments.set("acme/bb-gate production", [deployment()]); + await t.ring("github", "acme/bb-gate"); + expect(t.hooks.store.find("ENG-12")!.deployments["acme/web#7"]).toMatchObject({ state: "deploying", deploymentId: 7, deploymentState: "success" }); + }); + // 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 () => { @@ -14536,3 +14635,118 @@ describe("retrying a failed wake (BB-255)", () => { expect(t.retries).toHaveLength(2); }); }); + +// BB-285: an open pull request is not a wake source (a `pr`/`review` binding's +// own, or a ticket's once its gate is met), and a gate-met pull request nobody +// acts on, or a retarget wake nobody answers, raises one `stuck` per episode. +describe("a gate-met pull request nobody acts on (BB-285)", () => { + const head = "aaaaaaa1"; + const projectMarker = comment({ id: "m1", author: "trotterdylan", body: ``, signedBy: "thr_reviewer", createdAt: "2026-10-14T16:00:00Z" }); + const owner = comment({ id: "oa", author: "jane", body: "LGTM", signedBy: null, createdAt: "2026-10-14T17:00:00Z" }); + const noReview = comment({ id: "r1", author: "jane", body: ``, signedBy: "thr_reviewer", createdAt: "2026-10-14T15:00:00Z" }); + + it("marks a pr binding's free-text wait no-wake-source while its own pull request is open (#294)", async () => { + const t = await setup(); + t.entities.set("acme/web#7", pull({ labels: ["bb-managed"], author: "jane" })); + await t.ring("github", "acme/web#7"); + const b = t.hooks.store.find("acme/web#7")!; + expect(b.kind).toBe("pr"); + const report = await t.tool("steward_report", { phase: "open", summary: "Waiting.", nextAction: "Wait", actor: "other", waitingOn: "the dompurify fix on main", requeueAfter: "1m" }, b.threadId!); + expect(report.error).toBe(false); + expect(t.hooks.store.find("acme/web#7")!.noWakeSource).toBe(true); + }); + + it("marks a ticket's free-text wait no-wake-source when its own pull request is gate-met", async () => { + const t = await setup(); + t.entities.set("uuid-12", issue({ linkedPulls: [{ key: "acme/web#7", url: "https://github.com/acme/web/pull/7" }] })); + t.entities.set("acme/web#7", pull({ comments: [noReview] }, { head: "bb/eng-12-retry", checks: { state: "success", failing: [] } })); + await t.ring("linear", "uuid-12"); + const b = t.hooks.store.find("ENG-12")!; + const report = await t.tool("steward_report", { phase: "In Progress", summary: "Waiting.", nextAction: "Wait", actor: "other", waitingOn: "the vendor's reply", requeueAfter: "1m" }, b.threadId!); + expect(report.error).toBe(false); + expect(t.hooks.store.find("ENG-12")!.noWakeSource).toBe(true); + }); + + it("raises one stuck for a gate-met pull request nobody acts on after the window, and clears it when the gate moves", async () => { + const t = await setup(); + t.entities.set("acme/web#7", pull({ comments: [noReview] }, { checks: { state: "success", failing: [] } })); + await t.ring("github", "acme/web#7"); + const b = t.hooks.store.find("acme/web#7")!; + expect(b.kind).toBe("review"); + const stuckOf = () => t.hooks.store.openEscalations(b.id).filter((x) => x.kind === "stuck" && x.data.pullStuck === "gate-met"); + expect(stuckOf()).toHaveLength(0); + t.advance(2 * 60 * 60_000); + await t.ring("github", "acme/web#7"); + expect(stuckOf()).toHaveLength(1); + expect(stuckOf()[0]!.question).toContain("acme/web#7"); + // The same state on another sweep re-raises nothing. + await t.ring("github", "acme/web#7"); + expect(stuckOf()).toHaveLength(1); + // A new round requests changes: the gate is no longer met, so it clears. + const changed = comment({ id: "r2", author: "jane", body: ``, signedBy: "thr_reviewer", createdAt: "2026-10-14T18:00:00Z" }); + t.entities.set("acme/web#7", pull({ comments: [noReview, changed] }, { checks: { state: "success", failing: [] } })); + await t.ring("github", "acme/web#7"); + expect(stuckOf()).toHaveLength(0); + }); + + it("raises one stuck for a retarget nobody answers after the window, and clears it when retargeted", async () => { + const t = await setup(); + t.entities.set("uuid-12", issue({ linkedPulls: [{ key: "acme/web#7", url: "https://github.com/acme/web/pull/7" }] })); + t.entities.set("acme/web#8", pull({ id: "acme/web#8", key: "acme/web#8", url: "https://github.com/acme/web/pull/8", title: "Base", state: "merged" }, { merged: true, mergeCommit: "b".repeat(40), head: "bb/bb-256-parent", base: "main" })); + t.entities.set("acme/web#7", pull({ comments: [projectMarker, owner] }, { head: "bb/eng-12-retry", base: "bb/bb-256-parent", checks: { state: "success", failing: [] } })); + await t.ring("linear", "uuid-12"); + await t.flush(); + const b = t.hooks.store.find("ENG-12")!; + expect(b.retargetPulls).toEqual(["acme/web#8"]); + const stuckOf = () => t.hooks.store.openEscalations(b.id).filter((x) => x.kind === "stuck" && x.data.pullStuck === "retarget"); + expect(stuckOf()).toHaveLength(0); + t.advance(5 * 60 * 60_000); + await t.ring("linear", "uuid-12"); + await t.flush(); + expect(stuckOf()).toHaveLength(1); + expect(stuckOf()[0]!.question).toContain("acme/web#7"); + // The same state on another sweep re-raises nothing. + await t.ring("linear", "uuid-12"); + await t.flush(); + expect(stuckOf()).toHaveLength(1); + // Retargeted to the default branch: the state ends and it clears. + t.entities.set("acme/web#7", pull({ comments: [projectMarker, owner] }, { head: "bb/eng-12-retry", base: "main", checks: { state: "success", failing: [] } })); + await t.ring("linear", "uuid-12"); + await t.flush(); + expect(stuckOf()).toHaveLength(0); + }); + + it("does not raise the retarget stuck on the look that first sends the wake", async () => { + const t = await setup(); + t.entities.set("uuid-12", issue({ linkedPulls: [{ key: "acme/web#7", url: "https://github.com/acme/web/pull/7" }] })); + // The base is open, so the binding is held and quiet for hours. + t.entities.set("acme/web#8", pull({ id: "acme/web#8", key: "acme/web#8", url: "https://github.com/acme/web/pull/8", title: "Base" }, { head: "bb/bb-256-parent", base: "main" })); + t.entities.set("acme/web#7", pull({ comments: [projectMarker, owner] }, { head: "bb/eng-12-retry", base: "bb/bb-256-parent", checks: { state: "success", failing: [] } })); + await t.ring("linear", "uuid-12"); + await t.flush(); + const b = t.hooks.store.find("ENG-12")!; + t.advance(5 * 60 * 60_000); + // The base merges: this look sends the retarget wake and must not also + // raise a stall for the wake it just sent. + t.entities.set("acme/web#8", pull({ id: "acme/web#8", key: "acme/web#8", url: "https://github.com/acme/web/pull/8", title: "Base", state: "merged" }, { merged: true, mergeCommit: "b".repeat(40), head: "bb/bb-256-parent", base: "main" })); + await t.ring("linear", "uuid-12"); + await t.flush(); + expect(t.hooks.store.find("ENG-12")!.retargetPulls).toEqual(["acme/web#8"]); + expect(t.hooks.store.openEscalations(b.id).filter((x) => x.kind === "stuck" && x.data.pullStuck === "retarget")).toHaveLength(0); + }); + + it("counts a worker-reported own pull request as a wake source when its branch does not name the ticket", async () => { + const t = await setup(); + t.entities.set("uuid-12", issue({ linkedPulls: [{ key: "acme/web#7", url: "https://github.com/acme/web/pull/7" }] })); + t.entities.set("acme/web#7", pull({ title: "A fix", comments: [] }, { head: "feature/unrelated", checks: { state: "pending", failing: [] } })); + await t.ring("linear", "uuid-12"); + const b = t.hooks.store.find("ENG-12")!; + // A finished worker's result names the pull request, so it is the ticket's + // own even though neither its branch nor its title names the ticket. + await t.created("thr_w1", b.threadId!, "ENG-12: Implement", { branchName: "feature/other" }); + t.hooks.store.setWorker("thr_w1", "finished", "Opened https://github.com/acme/web/pull/7 for ENG-12."); + const report = await t.tool("steward_report", { phase: "In Progress", summary: "Waiting.", nextAction: "Wait", actor: "other", waitingOn: "the reviewer's reply", requeueAfter: "1m" }, b.threadId!); + expect(report.error).toBe(false); + expect(t.hooks.store.find("ENG-12")!.noWakeSource).toBe(false); + }); +}); diff --git a/plugins/stewards/server.ts b/plugins/stewards/server.ts index 0f1edd6d..e4102669 100644 --- a/plugins/stewards/server.ts +++ b/plugins/stewards/server.ts @@ -96,7 +96,7 @@ import { nightlyDue, stillNight } from "./nightly.js"; import { classify, hasFingerprintLine, lookChanges, opsTargetKey, parseOpsTargetKey, ticketResolved, type DependabotTarget, type FingerprintRow, type K8sTarget, type OpsLook, type OpsTarget, type SentryTarget, type Sighting, type TicketState } from "./ops.js"; import { EVENTS_PER_LOOK, NO_TOKEN, RECENT_PER_LOOK, SENTRY_API_URL, TOKEN_SCOPES, createSentryReader, emptySentryLook, issueInOrg, recentLine, sentryChanges, sentryResolveTargets, sentrySightings, sentrySummary, type SentryFinding, type SentryLook, type SentryReader } from "./sentry.js"; import { advanced, answerDiffers, anyWorkerInFlight, asksForOpenApproval, commentDiffersFromRecommendation, compactDue, deploymentSettled, diffObserved, endingDeadline, followingThrough, gateKnownUnmet, hasLabel, IDLE_WAKE_TEXT, idleInActiveStage, isCommentNews, isOwnStageChange, lifecycle, mechanicalStatus, namesUserAsBlocker, newHumanComments, newestCommentAt, openUserFacingItems, ownChecksFinished, ownPullClosed, ownPullMerged, phaseAndStage, productGateOptions, runningWorkerReason, newestEditComment, specDiff, specHash, specText, stuckDue, unheardWait, workerInFlight, workerReportedTerminal, type Change, type MechanicalEvent, type OpenUserItem } from "./reconcile.js"; -import { classifyApproval, latestProjectApprovalMarker, latestReviewMarker, prGate } from "./gate.js"; +import { classifyApproval, latestProjectApprovalMarker, latestReviewMarker, prGate, reviewMarker } from "./gate.js"; import { approveOwnerReviewedPull, approvePrPageAction, declineOwnerReviewedPull, fileForwardedApprovePrNeed, isProjectSteward, isUserThread, markerKey, normalizeText, placeReportedPull, specApprovedByProject, syncPullApproval, userApprovedOnEntity, userSaid, type ApprovalsContext } from "./approvals.js"; import { deliverEscalation, nextRoute, routedTo, type EscalationsContext } from "./escalations.js"; import { approvalWanted, briefGate, gateState, resolveChecklistItem, syncApprovalRequest, type ReadyGateContext } from "./ready-gate.js"; @@ -117,7 +117,7 @@ import { type Source, type WorkflowRun, } from "./source-contract.js"; -import { DEPLOY_STATES, allDeployed, applyDeployment, applyDeploymentRead, applyWorkflowRun, awaitingOnRepo, deploymentLines, deploymentReportSchema, matchesTarget, matchesWorkflow, mergedPulls, pickWatch, recordMerged, reportDeployment, targetKeyOf, timeOut, timeoutsDue } from "./deploy.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 { ACTORS, MIGRATIONS, ESCALATED_NEED_KINDS, NEEDS_KINDS, RESOLUTION_KINDS, createStore, needChoices, type Assumption, type Binding, type DepsDecision, type Escalation, type Fingerprint, type Lesson, type Need, type Observed, type Question, type ReadyState, type Store, type WakeRetry, type Worker } from "./store.js"; import { hasLessonAnchor, lessonAnchor, planLessonTransitions, planValidations, retiredLessonAnchor, withLessonAnchor } from "./lessons.js"; @@ -127,7 +127,7 @@ import { createGhWriter, parsePullRef, type BranchProtection, type GithubWriter, import { allPullsMerged, atWork, conflictsOf, finished, normalizeTouches, openPull, overlapping, touchesOverlap, type Conflict } from "./schedule.js"; import { AUTOMATIC_NAVIGATION_PROVIDER, AUTOMATIC_THREAD_LIST_PROVIDER, BUILTIN_NAVIGATION_ID, BUILTIN_THREAD_LIST_ID, NAVIGATION_PROVIDER_KEY, THREAD_LIST_PROVIDER_KEY } from "./sidebar-provider.js"; import { waitingOnNamesWorker } from "./sidebar.js"; -import { advanceStall, hasLiveWakeSource, parseUntil, type WakeSourceInput } from "./wake-source.js"; +import { advanceStall, hasLiveWakeSource, parseUntil, pullWatchCounts, type WakeSourceInput } from "./wake-source.js"; import { PROJECT_CHANGED } from "./channels.js"; import { LINEAR_TICKETS, PLAYBOOK_TEMPLATES, projectFileTemplate } from "./templates.js"; import type { DeploymentTarget } from "./config.js"; @@ -178,6 +178,12 @@ export const ONBOARDING_INTRO = `Set up stewards for this project. Follow the on export interface Timing { /** Rings for one entity within this long are one look. */ debounceMs: number; + /** + * While an awaiting watch's last deployment read is non-final, re-read it + * this often (BB-286), bounded by the watch's deadline. Within the five + * minutes AC1 names for the re-read after a non-final status. + */ + deploymentPollMs: number; /** How long a project's configuration is reused before `.bb` is read again. */ configTtlMs: number; /** The mechanical sweep: adoption backstop, stuck backstop, a look at every binding. */ @@ -202,6 +208,7 @@ export interface Timing { export const DEFAULT_TIMING: Timing = { debounceMs: 3000, + deploymentPollMs: 4 * 60_000, configTtlMs: 5 * 60_000, sweepMs: 30 * 60_000, requeueTickMs: 60_000, @@ -1124,6 +1131,18 @@ export const STEWARDS_SETTINGS = { description: "After a steward's or the project steward's turn, and before a wake that would push it over, its context is compacted when it holds more than this. Every wake is self-contained, so little is lost. 0 leaves compaction to the provider.", default: 120_000, }, + gateMetStuckMinutes: { + type: "number", + label: "Gate-met, nobody acting (minutes)", + description: "A pull request whose lane gate is met, with no worker running and no open item, raises a `stuck` escalation once it has sat this long (BB-285). 0 turns the check off.", + default: 60, + }, + retargetStuckMinutes: { + type: "number", + label: "Retarget unanswered (minutes)", + description: "A pull request still based on a merged branch this long after the plugin woke its steward to retarget it raises a `stuck` escalation (BB-285). 0 turns the check off.", + default: 240, + }, sentryToken: { type: "string", secret: true, @@ -1385,20 +1404,29 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P return next; } - function debounce(key: string, fn: () => Promise): void { + function debounce(key: string, fn: () => Promise, delayMs = timing.debounceMs): void { if (disposed || timers.has(key)) return; timers.set( key, setTimeout(() => { timers.delete(key); void fn(); - }, timing.debounceMs), + }, delayMs), ); } /** Look at a binding again, soon, once however often this is called meanwhile. */ const scheduleReconcile = (bindingId: string) => debounce(`b:${bindingId}`, () => serial(`b:${bindingId}`, () => reconcileBinding(bindingId, null))); + /** + * Re-read a binding whose last deployment read was non-final, at the + * minutes-scale poll interval (BB-286), so a dropped final doorbell is + * caught within minutes rather than waiting for the half-hourly sweep. + * The watch's deadline ends the retries: the reconcile times it out. The + * poll is silent; only a settled or failed watch wakes the steward. + */ + const scheduleDeploymentPoll = (bindingId: string) => debounce(`deploypoll:${bindingId}`, () => serial(`b:${bindingId}`, () => reconcileBinding(bindingId, null)), timing.deploymentPollMs); + // ---- configuration ---------------------------------------------------------------- const configs = new Map(); @@ -2320,22 +2348,33 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P } /** - * Whether the plugin watches a pull request for this binding (BB-154): a - * ticket with an open linked one, or an open `pr`/`review` binding whose - * entity is the pull request. A closed or merged pull request is not a - * source; github will not ring for it again. + * Whether the plugin watches a pull request for this binding (BB-154, + * BB-285): a `pr`/`review` binding's own pull request is never a wake + * source, and a ticket's open own pull request counts only while its gate + * is not met. A closed or merged pull request is not a source; github will + * not ring for it again. A gate-met pull request is the plugin's to act on + * (merge it, file the Merge item, or raise its own `stuck`), so waiting on + * it would mask every stall backstop (BB-154) and the idle wake (BB-83). */ - function pullWatch(b: Binding): boolean { - if (b.kind === "ticket") return leasedPulls(b).length > 0; - if (b.kind === "pr" || b.kind === "review") { - const entity = b.lastSnapshot?.entity ?? null; - return entity !== null && !["closed", "merged"].includes(lc(entity.state)); - } - return false; + function pullWatch(b: Binding, ownPulls: ReadonlySet): boolean { + if (b.kind !== "ticket") return false; + const observed = b.lastSnapshot; + if (observed === null) return false; + const pulls = observedPulls(b, observed); + // Only the ticket's own open pull requests (BB-239): a linked pull + // request that merely mentions the ticket is context, not a wait. The + // own set is the report path's, the same one the gate/merge loop uses. + const open = leasedPulls(b) + .map((key) => pulls.find((p) => lc(p.key) === lc(key))) + .filter((p): p is EntitySnapshot => p !== undefined && p.pull !== null && p.pull.merged !== true && lc(p.state) !== "closed") + .filter((p) => pullBelongsToTicket(b, p, ownPulls)); + if (open.length === 0) return false; + const gateMet = open.every((p) => prGate(p, { projectApprovals: projectApprovalsFor(b), ...productGateOptions(b, p) }).gateMet); + return pullWatchCounts(b.kind, open.length, gateMet); } /** The structured sources that will wake a binding, as the report path reads them (BB-154). */ - function liveWakeSourceFor(b: Binding): boolean { + function liveWakeSourceFor(b: Binding, ownPulls: ReadonlySet): boolean { const sources: WakeSourceInput = { hold: b.hold, holdBehind: b.holdBehind, @@ -2345,7 +2384,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P runningWorkers: store.workers(b.id).filter((w) => w.status === "running").length, openEscalations: store.openEscalations(b.id).length, openNeeds: store.openNeeds(b.id).length, - pullRequestWatch: pullWatch(b), + pullRequestWatch: pullWatch(b, ownPulls), }; return hasLiveWakeSource(sources); } @@ -3761,7 +3800,9 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // A live-at-merge watch (BB-260) is already `deployed` when it is // recorded: nothing builds or applies its paths, so it has no // deployment to await and the steward is woken with the verdict now. - const liveNow = fresh.filter((k) => deployments[k]?.state === "deployed"); + // BB-286: an existing watch re-matched to a live rule settles in + // `recordMerged` and is announced the same way. + const liveNow = Object.keys(deployments).filter((k) => deployments[k]?.state === "deployed" && beforeDeploy[k]?.state !== "deployed"); const awaiting = fresh.filter((k) => deployments[k]?.state !== "deployed"); if (awaiting.length > 0) deploymentChanges.push({ text: `Awaiting deployment: ${awaiting.join(", ")} via ${deployTargets.map((t) => `${t.repo} ${t.environment}`).join(", ")}`, actionable: true }); if (liveNow.length > 0) deploymentChanges.push({ text: `Deployed at merge: ${liveNow.map((k) => `${k} — ${deployments[k]?.note ?? "live at merge"}`).join("; ")}`, actionable: true }); @@ -3790,6 +3831,9 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P deployments = applied.deployments; b = store.patch(b.id, { deployments }); b = store.note(b.id, line, true); + // BB-286: a non-final status may be the last doorbell delivered; + // re-read within the debounce so a dropped final one is caught. + scheduleReconcile(b.id); continue; } if (failed) { @@ -3845,6 +3889,11 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P deployments = timeOut(deployments, late, now()); deploymentChanges.push({ text: `No matching deployment for ${labels.join(", ")}${within}${first === undefined ? "" : `; check the run: https://github.com/${first.repo}/actions`}`, actionable: true }); } + // BB-286: while an awaiting watch's last deployment read was + // non-final, keep re-reading at the poll interval, so a dropped final + // doorbell is caught within minutes. A settled or failed watch ends + // the retries; the watch's deadline times out the wait. + if (Object.values(deployments).some((w) => (w.state === "merged" || w.state === "deploying") && !isFinalDeploymentState(w.deploymentState))) scheduleDeploymentPoll(b.id); if (deployments !== b.deployments) b = store.patch(b.id, { deployments }); } const awaitingDeploy = deployTargets.length > 0 && Object.values(deployments).some((w) => w.state === "merged" || w.state === "deploying"); @@ -3912,6 +3961,16 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // diff only quiets them while one is, and the idle rule (BB-83) needs the // set for a binding whose worker has finished its turn. const ownPulls = await ownPullKeys(b, workers, observed); + // BB-285: which of the binding's own pull requests are gate-met. A + // gate-met own pull request is the plugin's to act on, so it does not + // suppress the idle wake (BB-83) the way an open one otherwise does. + const gateMetPulls = new Set(); + for (const e of [observed.entity, ...observed.pulls]) { + if (e.pull === null) continue; + const own = ownPulls.has(lc(e.key)) || (e.url !== null && e.url !== "" && ownPulls.has(lc(e.url))); + if (!own) continue; + if (prGate(e, { projectApprovals: projectApprovalsFor(b), ...productGateOptions(b, e) }).gateMet) gateMetPulls.add(e.key.toLowerCase()); + } // BB-203: an open need or `user` escalation keeps the worker's own pull // request activity loud, so the steward can reconcile the user's item. const userWaiting = store.openNeeds(b.id).length > 0 || store.openEscalations(b.id).some((x) => x.status === "open" && x.route === "user"); @@ -3932,7 +3991,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // the last wake; a worker finish asks for the look at once, without the // grace. An idle wake ends the period until one of those happens again. const idle = idleInActiveStage( - { stage, ending: b.ending, hold: b.hold, deployments, workers, ownPulls, escalations: store.openEscalations(b.id), needs: store.openNeeds(b.id), requeueUntil: b.requeueUntil }, + { stage, ending: b.ending, hold: b.hold, deployments, workers, ownPulls, gateMetPulls, escalations: store.openEscalations(b.id), needs: store.openNeeds(b.id), requeueUntil: b.requeueUntil }, observed, now(), ); @@ -5464,9 +5523,12 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const line = `Deployment ${deployment.id} (${state}) for ${repo} ${target.environment}${deployment.url === null ? "" : ` — ${deployment.url}`}${typeof target.carries === "string" ? `; the playbook's rule: ${target.carries}` : ""}`; // A deployment that has not reached a final status has not // applied the image yet, so no watch can move: a quiet note - // records it and no reconcile is scheduled (BB-204). + // records it. A re-read is still scheduled (BB-286), so a + // dropped final doorbell costs a debounce, not the sweep. if (!final) { b = store.note(b.id, line, true); + scheduleReconcile(b.id); + scheduleDeploymentPoll(b.id); continue; } const before = b.deployments; @@ -5553,6 +5615,21 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const bound = store.byEntity(source, id); if (bound !== null) scheduleReconcile(bound.id); if (source === "github") { + // BB-286: a repository ring (no `#`) comes from github-watcher's + // reconnect sweep. Re-read every open binding whose playbook names a + // deployment target for the repo, so a deployment doorbell dropped + // while the subscriber was away is caught by the reconnect (BB-59) + // rather than only by the half-hourly sweep. + if (bound === null && !id.includes("#")) { + for (const config of await stewardedProjects()) { + for (const b of store.open(config.projectId)) { + const playbook = bindingPlaybook(config, b); + if (playbook === null || playbook === undefined) continue; + if (playbook.deployments.some((t) => lc(t.repo) === lc(id)) && awaitingOnRepo(b.deployments, id).length > 0) scheduleReconcile(b.id); + } + } + return; + } // A Dependabot alert doorbell names the alert, not the repository's // ops binding; wake that repository's dependency watch. An id with a // github binding is an issue or pull request, not an alert. @@ -6779,6 +6856,14 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const config = await loadConfig(b.projectId); const decisions = config?.file.bigDecisions ?? []; const ownerLogin = config?.githubLogin ?? null; + // BB-285: the two stall windows, in milliseconds. 0 turns a check off. + const stuckSettings = await settings.get(); + const gateMetStuckMs = Math.max(0, stuckSettings.gateMetStuckMinutes) * 60_000; + const retargetStuckMs = Math.max(0, stuckSettings.retargetStuckMinutes) * 60_000; + // BB-285: the pull requests in a stuck-eligible state this look, so a + // `stuck` episode that has ended is cleared after the loop and a later + // recurrence raises afresh. + const pullStuckState = new Map(); // BB-239: the pull requests a worker of this binding opened or reported. // A ticket's other linked pull requests (GitHub cross-references) only // mention it and must not file gate items here. @@ -6815,6 +6900,10 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // (BB-264), so the retarget wake is once per base pull request. const retargetPulls = [...b.retargetPulls]; const retargetBefore = new Set(retargetPulls.map((k) => lc(k))); + // The retarget wake sent before this look (BB-285): the retarget `stuck` + // is judged on this set, never on the one the loop mutates, so the look + // that first sends the wake cannot also raise the stall. + const retargetSentBefore = new Set(retargetBefore); for (const pull of observedPulls(b, observed)) { const url = pull.url ?? pull.key; if (ownPulls !== null && !pullBelongsToTicket(b, pull, ownPulls)) { @@ -6849,6 +6938,9 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P !playbook.ownerMerges && (b.kind === "review" || (b.kind === "pr" && pull.author !== null && ownerLogin !== null && lc(pull.author) !== lc(ownerLogin))); if (g.gateMet) { + // BB-285: whether this look merged the pull request, so the + // gate-met-with-nobody-acting check below does not race its own merge. + let mergedThisLook = false; // A steward's own merge item (a refused merge) is left alone while a // worker still runs; the plugin files its own only when none does. // BB-256: a standing merge hold is not a stuck loop, so the plugin @@ -6891,6 +6983,20 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P retargetPulls.push(mergedBase.key); retargetBefore.add(lc(mergedBase.key)); } + // BB-285: a retarget wake nobody answered. Once the wake has been + // sent (recorded in retargetPulls) and the pull request is still + // based on the merged branch after the window, escalate `stuck`. + if (retargetStuckMs > 0) { + pullStuckState.set(lc(pull.key), "retarget"); + // Only raise, never resolve here: raising the escalation wakes + // the steward, which moves `wokeAt`, and resolving on a moved + // anchor would clear it in a loop. The post-loop sweep clears it + // once the pull request is retargeted or the state otherwise ends. + if (retargetSentBefore.has(lc(mergedBase.key)) && pullStuckEscalation(b, pull, "retarget") === null) { + const since = store.binding(b.id)?.wokeAt ?? b.wokeAt; + if (since !== null && now() - since >= retargetStuckMs) await raisePullStuck(b, pull, "retarget", `its base ${mergedBase.key} merged and it is still based on that branch after ${formatDuration(retargetStuckMs)}`, changes, null); + } + } } else if (store.openNeeds(b.id).some((n) => n.kind === "merge" && n.url === url && n.filedBy !== "plugin")) { // A steward-filed Merge item is a hand-off to a person (a worker // whose permission system refused the merge, or a refused plugin @@ -6909,6 +7015,7 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P if (mergeRefusalFor(b, g) === null) { const result = await mergePullRequest(b, ref, pull, "by the plugin"); if (result.ok) { + mergedThisLook = true; // The note goes into this look's changes as well as the // binding, so the wake that clears `pending` still carries // it; a merge found by the next look reads the same as a @@ -6948,6 +7055,23 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P }); } } + // BB-285: a gate-met pull request nobody acts on. No worker is + // running and no item is open, so the plugin is neither merging it + // nor waiting on a person; after the window it is stuck. A retarget + // episode is criterion 4's, not this one. + if (mergedBase === null && !mergeHeld && !mergedThisLook && gateMetStuckMs > 0) { + const unattended = runningWorkerReason(store.workers(b.id)) === null && store.openNeeds(b.id).length === 0; + if (unattended) { + pullStuckState.set(lc(pull.key), "gate-met"); + const since = gateMetSince(pull, g); + const open = pullStuckEscalation(b, pull, "gate-met"); + if (since !== null && now() - since >= gateMetStuckMs) { + if (open === null) await raisePullStuck(b, pull, "gate-met", `the gate is met (${g.lane ?? "lane unknown"}), no worker is running and no item is open after ${formatDuration(gateMetStuckMs)}`, changes, g); + } else if (open !== null) { + store.resolve(open.id, "The pull request is no longer gate-met with nobody acting."); + } + } + } } else { store.closeNeedByKey(b.id, "merge", url, `The gate is not met: ${g.missing ?? "unknown"}.`); } @@ -6974,6 +7098,14 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P const text = gatePushText(b, item); if (text !== null) await pushNeed(b, { id: item.id, ...text, threadId: b.threadId }); } + // BB-285: a `stuck` episode that has ended is cleared, so a later + // recurrence is a new episode and raises afresh. + for (const x of store.openEscalations(b.id)) { + const kind = x.data.pullStuck; + if (kind !== "gate-met" && kind !== "retarget") continue; + if (pullStuckState.get(lc(String(x.data.pull ?? ""))) === kind) continue; + store.resolve(x.id, kind === "retarget" ? "The pull request is no longer based on a merged branch." : "The pull request is no longer gate-met with nobody acting."); + } return events; } @@ -7928,6 +8060,67 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P await deliverEscalation(escalationsCtx, b, x); } + /** The open `stuck` escalation the plugin raised for a pull request episode (BB-285), if any. */ + function pullStuckEscalation(b: Binding, pull: EntitySnapshot, kind: "gate-met" | "retarget"): Escalation | null { + return store.openEscalations(b.id).find((x) => x.kind === "stuck" && x.data.pullStuck === kind && lc(String(x.data.pull ?? "")) === lc(pull.key)) ?? null; + } + + /** + * When a pull request's gate became met (BB-285): the newest of the + * reviewer's approving marker and the effective approval (the owner's + * comment or review, or the project steward's recorded marker). Null when + * neither time can be read. Checks are not timed, so a gate that became + * met only when checks finished is dated from the approval; the window is + * a backstop, not an exact clock. + */ + function gateMetSince(pull: EntitySnapshot, g: ReturnType): number | null { + let at = 0; + for (const c of pull.comments) { + if (reviewMarker(c.body) === null) continue; + const t = Date.parse(c.createdAt); + if (!Number.isNaN(t)) at = Math.max(at, t); + } + const approvalAt = g.decision === "project" ? (g.projectApprovalId === null ? null : (pull.comments.find((c) => c.id === g.projectApprovalId)?.createdAt ?? null)) : g.ownerApprovalAt; + if (approvalAt !== null) { + const t = Date.parse(approvalAt); + if (!Number.isNaN(t)) at = Math.max(at, t); + } + return at === 0 ? null : at; + } + + /** + * Raise one `stuck` escalation for a pull request episode (BB-285): a + * gate-met pull request nobody acts on, or a retarget wake nobody answers. + * The episode marker (`data.pullStuck` and `data.pull`) keeps it to one + * per episode: a sweep that finds the same state again sees the open + * escalation and raises nothing, and the sweep that finds the state gone + * resolves it, so a later recurrence is a new episode. + */ + async function raisePullStuck(b: Binding, pull: EntitySnapshot, kind: "gate-met" | "retarget", why: string, changes: Change[], g: ReturnType | null): Promise { + const note = kind === "retarget" ? `Stuck: ${pull.key} is still based on a merged branch after the retarget window.` : `Stuck: ${pull.key} is gate-met with nobody acting.`; + const question = kind === "retarget" ? `Retarget ${pull.key} went unanswered: ${why}.` : `${pull.key} is gate-met with nobody acting: ${why}.`; + const context = + kind === "retarget" + ? "The plugin woke the steward to rebase the pull request onto the default branch and retarget it, and it is still based on a merged branch. Nothing the plugin tracks will retarget it." + : `The lane gate is met (${g?.lane ?? "lane unknown"}), no worker is running and no item is open, so the plugin is neither merging it nor waiting on a person.`; + store.note(b.id, note); + changes.push({ text: note, actionable: true }); + const x = store.escalate({ + bindingId: b.id, + kind: "stuck", + question, + context, + meanwhile: "Waiting: nothing the plugin tracks will move it.", + route: nextRoute(escalationsCtx, b, null), + recommendation: null, + url: pull.url ?? pull.key, + headSha: g?.headSha ?? null, + data: { pullStuck: kind, pull: lc(pull.key) }, + projectId: b.projectId, + }); + await deliverEscalation(escalationsCtx, b, x); + } + /** * The one merge path the Stewards page's Merge button and `bb stewards * merge` share: recompute the gate with the project's approval records @@ -8000,7 +8193,10 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P } if (deployment !== undefined) { if (source !== "github") throw new Error("only the github-watcher plugin sends deployment doorbells"); - debounce(`deploy:${lc(id)}`, () => serial(`deploy:${lc(id)}`, () => onDeploymentDoorbell(id, deployment))); + // BB-286: key the debounce on the deployment's state too, so an + // in_progress followed by a success inside the window does not + // discard the newer closure (first-wins kept only the first). + debounce(`deploy:${lc(id)}:${deployment.state}`, () => serial(`deploy:${lc(id)}`, () => onDeploymentDoorbell(id, deployment))); return { ok: true }; } debounce(`ring:${source}:${lc(id)}`, () => serial(`ring:${source}:${lc(id)}`, () => onRing(source, id))); @@ -9421,7 +9617,10 @@ export function createStewardsPlugin(deps: StewardsDeps): (bb: BbPluginApi) => P // set of sources (an escalation this turn raised, a need it filed, a // gate item syncNeeds filed, a worker it started). const current = store.binding(b.id) ?? b; - const liveSource = liveWakeSourceFor(current); + // The same own-pull set the gate/merge loop uses (BB-52 rule 0), so a + // ticket's own pull request counts as a source or not by one rule. + const currentOwnPulls = current.kind === "ticket" && current.lastSnapshot !== null ? await ownPullKeys(current, store.workers(current.id), current.lastSnapshot) : new Set(); + const liveSource = liveWakeSourceFor(current, currentOwnPulls); // BB-247: a dated wait is the plugin's own wake source while the date // is ahead, so it is not marked waiting with no source (and is listed // as a dated wait in the digest instead). diff --git a/plugins/stewards/wake-source.test.ts b/plugins/stewards/wake-source.test.ts index 87bccf0d..b03568da 100644 --- a/plugins/stewards/wake-source.test.ts +++ b/plugins/stewards/wake-source.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { NO_STALL, STALL_WAKE_THRESHOLD, advanceStall, hasLiveWakeSource, parseUntil, type WakeSourceInput } from "./wake-source.js"; +import { NO_STALL, STALL_WAKE_THRESHOLD, advanceStall, hasLiveWakeSource, parseUntil, pullWatchCounts, type WakeSourceInput } from "./wake-source.js"; const noSources: WakeSourceInput = { hold: null, @@ -52,6 +52,20 @@ describe("a binding's live wake sources (BB-154)", () => { expect(hasLiveWakeSource(watch({ openNeeds: 1 }))).toBe(true); expect(hasLiveWakeSource(watch({ pullRequestWatch: true }))).toBe(true); }); + + // BB-285: an open pull request is not a wake source for a `pr`/`review` + // binding, and a ticket's open own pull request counts only while its gate + // is not met. + it("does not count a pr or review binding's own open pull request", () => { + expect(pullWatchCounts("pr", 1, false)).toBe(false); + expect(pullWatchCounts("review", 1, false)).toBe(false); + }); + + it("counts a ticket's open own pull request only while its gate is not met", () => { + expect(pullWatchCounts("ticket", 1, false)).toBe(true); + expect(pullWatchCounts("ticket", 1, true)).toBe(false); + expect(pullWatchCounts("ticket", 0, false)).toBe(false); + }); }); describe("the requeue-only stall counter (BB-154)", () => { diff --git a/plugins/stewards/wake-source.ts b/plugins/stewards/wake-source.ts index 23713e79..944e2c3b 100644 --- a/plugins/stewards/wake-source.ts +++ b/plugins/stewards/wake-source.ts @@ -8,6 +8,7 @@ // Pure, so the report path, the store's derived phrase and the tests all read // the same rules. The threshold is one plugin constant, never a project.json // key. +import type { BindingKind } from "./config.js"; import type { Deployments } from "./deploy.js"; /** @@ -58,6 +59,22 @@ export function hasLiveWakeSource(s: WakeSourceInput): boolean { ); } +/** + * Whether an open pull request is a wake source for a binding (BB-285). A + * `pr` or `review` binding's own pull request is not: the binding is the pull + * request, so its open state masks every stall backstop (BB-154) and the + * BB-83 idle wake. A ticket binding's open own pull request counts only while + * the gate is not met: once the gate is met the plugin acts on the pull + * request (merges it, files the Merge item, or raises its own `stuck`), so the + * open pull request is not a wait. `openOwnPulls` is the number of the + * binding's open pull requests and `gateMet` whether every one of them is + * gate-met. + */ +export function pullWatchCounts(kind: BindingKind, openOwnPulls: number, gateMet: boolean): boolean { + if (kind !== "ticket") return false; + return openOwnPulls > 0 && !gateMet; +} + /** * Parse a report's `until` (BB-247): a `YYYY-MM-DD` date read as 00:00 UTC, * or an ISO datetime. Returns the epoch ms, or null when the value is not a