diff --git a/plugins/stewards/README.md b/plugins/stewards/README.md index 868ea2c..7a54d34 100644 --- a/plugins/stewards/README.md +++ b/plugins/stewards/README.md @@ -293,6 +293,9 @@ bb. [DESIGN.md](DESIGN.md) is the design; this is how it is built and run. each entry `decide: project | user`): the project steward approves a `project` entry (recorded in the store, bound to the pull request's head sha, never a GitHub comment or review) and forwards a `user` one to you. + An unsigned comment or review whose first line carries a `bb-review` or + `bb-approval` marker is a thread's, never the owner's approval, whether or + not it is signed (BB-110). A missing or unknown mark, a change to the list, `bigDecisions`, `prGate` or the approval code is always yours. - **Lessons** are a project steward's general judgements, made permanent in diff --git a/plugins/stewards/reconcile.test.ts b/plugins/stewards/reconcile.test.ts index 94e74f8..d17e951 100644 --- a/plugins/stewards/reconcile.test.ts +++ b/plugins/stewards/reconcile.test.ts @@ -562,6 +562,27 @@ describe("the pull request gate (build-tickets)", () => { const signed = pull({ comments: [marker(head, "approved", "owner-reviewed"), comment({ author: "jane", body: "LGTM", signedBy: "thr_w" })] }, green); expect(ownerApproval(signed).approved).toBe(false); }); + + it("never reads an unsigned bb-review marker comment as the owner's approval (BB-110)", () => { + // The reviewer posts as the connected user; when it leaves its signature + // off, the marker still makes the comment a thread's, not the owner's. + const unsignedMarker = (body: string) => comment({ id: "rev", author: "jane", body, signedBy: null, createdAt: "2026-10-14T17:00:00Z" }); + const marked = pull({ comments: [marker(head, "approved", "owner-reviewed"), unsignedMarker("\n\nLGTM, approved.")] }, green); + expect(ownerApproval(marked).approved).toBe(false); + // The same approving body without the marker is the owner's approval. + const unmarked = pull({ comments: [marker(head, "approved", "owner-reviewed"), unsignedMarker("LGTM, approved.")] }, green); + expect(ownerApproval(unmarked)).toMatchObject({ approved: true, at: "2026-10-14T17:00:00Z" }); + }); + + it("never reads an unsigned bb-approval marker comment as the owner's approval (BB-110)", () => { + const typed = comment({ id: "typed", author: "jane", body: approvalMarkerLine({ sha: head, entry: null, verdict: "approved" }), signedBy: null, createdAt: "2026-10-14T17:00:00Z" }); + expect(ownerApproval(pull({ comments: [marker(head, "approved", "owner-reviewed"), typed] }, green)).approved).toBe(false); + // It is still read by the project-approval path when the plugin recorded its id. + const ap = approval(head, "A cross-repo contract", "approved", "IC_recorded"); + const recorded = pull({ comments: [marker(head, "approved", "owner-reviewed"), { ...ap, author: "jane", signedBy: null }] }, green); + expect(ownerApproval(recorded).approved).toBe(false); + expect(prGate(recorded, { decision: "project", projectApprovals: [record(ap.id)] })).toMatchObject({ projectApproved: true, gateMet: true }); + }); }); describe("a blockedOn naming the user", () => { @@ -593,6 +614,15 @@ describe("a correction from a comment", () => { expect(commentDiffersFromRecommendation(comment({ author: "jane", body: "No, do it the other way.", signedBy: "thr_w" }), "jane", "Do it the recommended way.")).toBe(false); }); + it("never counts a thread's marker comment as the owner's correction, signed or not (BB-110)", () => { + const marked = comment({ author: "jane", body: "\n\nNo, do it the other way.", signedBy: null }); + expect(commentDiffersFromRecommendation(marked, "jane", "Do it the recommended way.")).toBe(false); + const approvalMarked = comment({ author: "jane", body: `${approvalMarkerLine({ sha: "abc", entry: null, verdict: "approved" })}\n\nNo, do it the other way.`, signedBy: null }); + expect(commentDiffersFromRecommendation(approvalMarked, "jane", "Do it the recommended way.")).toBe(false); + // The same words without the marker still correct. + expect(commentDiffersFromRecommendation(comment({ author: "jane", body: "No, do it the other way.", signedBy: null }), "jane", "Do it the recommended way.")).toBe(true); + }); + it("treats an empty side as no difference", () => { expect(answerDiffers("", "Do it.")).toBe(false); expect(answerDiffers("Do it.", "")).toBe(false); diff --git a/plugins/stewards/reconcile.ts b/plugins/stewards/reconcile.ts index 319d719..9aaf926 100644 --- a/plugins/stewards/reconcile.ts +++ b/plugins/stewards/reconcile.ts @@ -162,6 +162,8 @@ export function answerDiffers(answer: string, recommendation: string): boolean { export function commentDiffersFromRecommendation(c: SnapshotComment, viewer: string | null, recommendation: string | null): boolean { if (viewer === null || recommendation === null || c.signedBy !== null) return false; if (c.author.toLowerCase() !== viewer.toLowerCase()) return false; + // A marker comment is a thread's, signed or not: never the owner's words (BB-110). + if (isMarkerComment(c.body)) return false; return answerDiffers(c.body, recommendation); } @@ -399,10 +401,23 @@ export function diffObserved(before: Observed, after: Observed, ctx: ObservedDif // ---- the merge gate ------------------------------------------------------------------- +/** + * Whether a comment body opens with a thread's machine-readable marker on its + * first line: a reviewer's `bb-review` round or the project steward's + * `bb-approval` decision. A marker comment is a thread's by construction, so + * it is never the owner's own words, signed or not (BB-110). + */ +export function isMarkerComment(body: string): boolean { + return reviewMarker(body) !== null || projectApprovalMarker(body) !== null; +} + /** * Whether the owner has approved a pull request: an unsigned approving comment * by the connected user, or an APPROVED review from them. A signed comment is - * a thread's (the reviewer's marker, a worker's), never the owner's. + * a thread's (the reviewer's marker, a worker's), never the owner's. So is an + * unsigned comment that carries a `bb-review` or `bb-approval` marker: the + * reviewer posts as the connected user, and its marker is a thread's whatever + * the signature line says (BB-110). */ export function ownerApproval(entity: EntitySnapshot): { approved: boolean; at: string | null } { const viewer = entity.viewer === null ? null : entity.viewer.toLowerCase(); @@ -410,6 +425,7 @@ export function ownerApproval(entity: EntitySnapshot): { approved: boolean; at: let at: string | null = null; for (const c of entity.comments) { if (c.signedBy !== null || c.author.toLowerCase() !== viewer) continue; + if (isMarkerComment(c.body)) continue; if (classifyApproval(c.body) !== "approve") continue; if (at === null || c.createdAt > at) at = c.createdAt; } diff --git a/plugins/stewards/server.test.ts b/plugins/stewards/server.test.ts index 17ed780..ad279b7 100644 --- a/plugins/stewards/server.test.ts +++ b/plugins/stewards/server.test.ts @@ -3993,6 +3993,21 @@ describe("needs you items", () => { expect((await board()).needs.find((n) => n.kind === "merge")).toMatchObject({ url: "https://github.com/acme/web/pull/7", entityUrl: "https://linear.app/acme/issue/ENG-12" }); }); + it("files approve-pr and the user's approval escalation for an unsigned reviewer marker (BB-110)", async () => { + // The reviewer posts as the connected user. When it leaves the signature + // off, its marker comment still carries the marker, so it is a thread's, + // not the owner's approval: the plugin must still file the page item and + // the approval escalation routed to the user. + const unsigned = comment({ id: "m-unsigned", author: "jane", body: `\n\nLGTM, approved.`, signedBy: null, createdAt: "2026-10-14T16:00:00Z" }); + const t = await withPull([unsigned]); + const b = t.binding(); + const need = t.hooks.store.openNeeds(b.id).find((n) => n.kind === "approve-pr")!; + expect(need).toMatchObject({ what: "Approve acme/web#7 on GitHub", filedBy: "plugin" }); + const esc = t.hooks.store.openEscalations(b.id).find((x) => x.kind === "approval" && x.resolutionKind === "approve-decline")!; + expect(esc).toMatchObject({ route: "me" }); + expect(t.hooks.store.openNeeds(b.id).some((n) => n.kind === "merge")).toBe(false); + }); + it("refuses an approve-pr the gate cannot hold, names the missing step and files nothing (BB-21)", async () => { const t = await withPull([]); const b = t.binding();