Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions plugins/stewards/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
30 changes: 30 additions & 0 deletions plugins/stewards/reconcile.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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("<!-- bb-review round=2 sha=bbbbbbb2 verdict=approved lane=owner-reviewed -->\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", () => {
Expand Down Expand Up @@ -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: "<!-- bb-review round=1 sha=abc verdict=approved lane=owner-reviewed -->\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);
Expand Down
18 changes: 17 additions & 1 deletion plugins/stewards/reconcile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down Expand Up @@ -399,17 +401,31 @@ 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();
if (viewer === null) return { approved: false, at: null };
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;
}
Expand Down
15 changes: 15 additions & 0 deletions plugins/stewards/server.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: `<!-- bb-review round=1 sha=${head} verdict=approved lane=owner-reviewed -->\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();
Expand Down
Loading