diff --git a/packages/backend-utils/__tests__/gatekeeper-action.test.ts b/packages/backend-utils/__tests__/gatekeeper-action.test.ts index a4b4f53e2..92bbf6e30 100644 --- a/packages/backend-utils/__tests__/gatekeeper-action.test.ts +++ b/packages/backend-utils/__tests__/gatekeeper-action.test.ts @@ -1,5 +1,82 @@ import { describe, expect, it } from "vitest"; -import { SerialTaskQueue } from "../src/gatekeeper-action"; +import { + InvalidationLog, + SerialTaskQueue, + displayReason, + validateApplyThroughArgs, +} from "../src/gatekeeper-action"; + +type Kv = ConstructorParameters[0]; + +// Minimal in-memory KV matching the slice of the DO storage API the log uses. +function makeKv(): Kv { + const map = new Map(); + return { + put: (k: string, v: unknown) => void map.set(k, v), + delete: (k: string) => void map.delete(k), + list: ({ prefix }: { prefix: string }) => + [...map.entries()].filter(([k]) => k.startsWith(prefix)) as [string, T][], + } as unknown as Kv; +} + +describe("validateApplyThroughArgs", () => { + it("deduplicates in-range vetoes", () => { + expect(validateApplyThroughArgs(5, [2, 2, 5])).toEqual(new Set([2, 5])); + }); + + it("rejects a non-positive or non-integer frontier", () => { + expect(() => validateApplyThroughArgs(0, [])).toThrow("Invalid action ID"); + expect(() => validateApplyThroughArgs(1.5, [])).toThrow("Invalid action ID"); + }); + + it("rejects vetoes above the frontier or out of the integer range", () => { + expect(() => validateApplyThroughArgs(3, [4])).toThrow("Invalid veto action ID"); + expect(() => validateApplyThroughArgs(3, [0])).toThrow("Invalid veto action ID"); + }); +}); + +describe("InvalidationLog", () => { + it("reports only entries attributed to the requested vetoes, ascending", () => { + const log = new InvalidationLog(makeKv()); + log.record(10, 3); + log.record(4, 3); + log.record(7, 5); + + expect(log.attributedTo(new Set([3]))).toEqual([ + { action: 4, invalidatedBy: 3 }, + { action: 10, invalidatedBy: 3 }, + ]); + }); + + it("prunes entries below the pending floor but keeps the current request's vetoes", () => { + const log = new InvalidationLog(makeKv()); + log.record(4, 3); + log.record(7, 5); + + log.prune(new Set([3]), 4); + + expect(log.attributedTo(new Set([3, 5]))).toEqual([ + { action: 4, invalidatedBy: 3 }, + { action: 7, invalidatedBy: 5 }, + ]); + + log.prune(new Set(), Infinity); + + expect(log.attributedTo(new Set([3, 5]))).toEqual([]); + }); +}); + +describe("displayReason", () => { + it("passes through an Error with a message", () => { + const error = new Error("page was deleted upstream"); + expect(displayReason(error, "fallback")).toBe(error); + }); + + it("wraps non-Error throws in the fallback text", () => { + expect(displayReason("oops", "Vendor could not apply this action").message) + .toBe("Vendor could not apply this action: oops"); + }); +}); // This package deliberately avoids the full Node type environment (see node-async-hooks.d.ts). // Tests run under vitest on Node, so type the small `process` surface used here locally. diff --git a/packages/backend-utils/src/gatekeeper-action.ts b/packages/backend-utils/src/gatekeeper-action.ts index 922bbd6a5..45fc3ed36 100644 --- a/packages/backend-utils/src/gatekeeper-action.ts +++ b/packages/backend-utils/src/gatekeeper-action.ts @@ -1,4 +1,9 @@ -// Shared helpers for gatekeepers implementing the `Gatekeeper` action contract. +// Shared helpers for gatekeepers implementing the `Gatekeeper` action contract. They cover the +// obligations every action-queueing gatekeeper repeats: serializing resolution methods, argument +// validation, durable attribution of veto-cascade invalidations (so repeated requests can +// re-report them), and the display-safe `stopped.reason` error. + +type Kv = DurableObjectStorage["kv"]; /** Runs asynchronous operations sequentially in submission order. */ export class SerialTaskQueue { @@ -11,3 +16,73 @@ export class SerialTaskQueue { return result; } } + +/** One `ApplyActionsThroughResult.invalidatedByVeto` entry. */ +export type VetoInvalidation = { action: number, invalidatedBy: number }; + +/** + * Validate an `applyActionsThrough(actionId, vetoes)` request before touching any state: the + * frontier must be a positive integer and every veto must be a positive integer at or below it. + * Returns the deduplicated veto set. + */ +export function validateApplyThroughArgs(actionId: number, vetoes: number[]): Set { + if (!Number.isSafeInteger(actionId) || actionId < 1) throw new TypeError("Invalid action ID."); + const result = new Set(); + for (const veto of vetoes) { + if (!Number.isSafeInteger(veto) || veto < 1 || veto > actionId) { + throw new TypeError("Invalid veto action ID."); + } + result.add(veto); + } + return result; +} + +/** + * Durable record of which veto invalidated which cascade-deleted action, kept in its own KV + * keyspace so a gatekeeper that deletes rejected records can still satisfy the contract's + * requirement that a repeated request re-report invalidations attributable to its vetoes. + */ +export class InvalidationLog { + #kv: Kv; + + constructor(kv: Kv) { + this.#kv = kv; + } + + /** Record that the veto of `vetoedBy` invalidated `invalidatedId`, for later re-reporting. */ + record(invalidatedId: number, vetoedBy: number): void { + this.#kv.put(`invalidation:${invalidatedId}`, vetoedBy); + } + + /** Persisted invalidations attributed to any of the given vetoed action IDs, ascending. */ + attributedTo(vetoes: Set): VetoInvalidation[] { + return [...this.#kv.list({ prefix: "invalidation:" })] + .map(([key, invalidatedBy]) => ({ action: Number(key.slice("invalidation:".length)), invalidatedBy })) + .filter(entry => vetoes.has(entry.invalidatedBy)) + .toSorted((a, b) => a.action - b.action); + } + + /** + * Drop entries no future request can attribute: a veto is only ever re-sent while some action at + * or below it is still undecided, so entries whose veto precedes every remaining undecided + * action (`pendingFloor`, `Infinity` when none remain) are unreachable. `keep` protects the + * current request's vetoes. + */ + prune(keep: Set, pendingFloor: number): void { + for (const [key, vetoedBy] of this.#kv.list({ prefix: "invalidation:" })) { + if (vetoedBy < pendingFloor && !keep.has(vetoedBy)) this.#kv.delete(key); + } + } +} + +/** + * The error a gatekeeper should report in `stopped.reason`. Only its `message` survives the RPC + * hop to the overseer, so it must stand alone as text the user can act on; apply errors already + * carry a specific, display-safe message, exactly as the legacy single-action path surfaced them. + * `fallback` describes the failure generically for non-Error throws with no message of their own. + */ +export function displayReason(error: unknown, fallback: string): Error { + return error instanceof Error && error.message + ? error + : new Error(`${fallback}: ${String(error)}`); +} diff --git a/packages/backend-utils/tsconfig.json b/packages/backend-utils/tsconfig.json index ae8d4640b..3b380fab4 100644 --- a/packages/backend-utils/tsconfig.json +++ b/packages/backend-utils/tsconfig.json @@ -2,7 +2,7 @@ "extends": "../../tsconfig.json", "compilerOptions": { "target": "ES2022", - "lib": ["ES2022"], + "lib": ["ES2023"], "module": "ESNext", "moduleResolution": "bundler", "types": ["@cloudflare/workers-types/experimental", "./src/node-async-hooks.d.ts"] diff --git a/packages/gatekeeper-confluence/__tests__/apply.test.ts b/packages/gatekeeper-confluence/__tests__/apply.test.ts index 7b0385dac..e670ce956 100644 --- a/packages/gatekeeper-confluence/__tests__/apply.test.ts +++ b/packages/gatekeeper-confluence/__tests__/apply.test.ts @@ -1,8 +1,10 @@ import { describe, expect, it } from "vitest"; import { ConfluenceStore, + applyStoredActionsThrough, applyStoredAction, revertStoredAction, + stageAction, type ConfluenceAction, } from "../src/confluence-actions"; import type { ConfluenceApi } from "../src/confluence-api"; @@ -114,6 +116,205 @@ describe("applyStoredAction", () => { }); }); +describe("applyStoredActionsThrough", () => { + it("skips sparse IDs and stops at the first failed action", async () => { + const { api, calls } = makeApi(); + const store = storeWith(api); + const first = stage(store, { type: "addComment", contentId: "first", text: "one" }); + const hole = stage(store, { type: "addComment", contentId: "hole", text: "two" }); + const failed = stage(store, { type: "addComment", contentId: "failed", text: "three" }); + const later = stage(store, { type: "addComment", contentId: "later", text: "four" }); + store.deleteAction(hole); + api.addComment = async (id: string, _storage: string, type: string) => { + calls.addComment.push({ id, type }); + if (id === "failed") throw new Error("safe failure"); + return { id: `comment-${id}` }; + }; + + const result = await applyStoredActionsThrough(store, later, []); + + expect(calls.addComment.map(call => call.id)).toEqual(["first", "failed"]); + expect(store.getAction(first)?.state).toBe("applied"); + expect(store.getAction(failed)?.state).toBe("pending"); + expect(store.getAction(later)?.state).toBe("pending"); + // The specific apply error is passed through so the user can resolve the problem. + expect(result.stopped).toMatchObject({ at: failed, reason: expect.any(Error) }); + expect(result.stopped?.reason.message).toBe("safe failure"); + }); + + it("does not re-apply earlier actions when retried after a stop", async () => { + const { api, calls } = makeApi(); + const store = storeWith(api); + const first = stage(store, { type: "addComment", contentId: "first", text: "one" }); + const flaky = stage(store, { type: "addComment", contentId: "flaky", text: "two" }); + let failOnce = true; + api.addComment = async (id: string, _storage: string, type: string) => { + calls.addComment.push({ id, type }); + if (id === "flaky" && failOnce) { + failOnce = false; + throw new Error("temporarily unavailable"); + } + return { id: `comment-${id}` }; + }; + + const stopped = await applyStoredActionsThrough(store, flaky, []); + const retry = await applyStoredActionsThrough(store, flaky, []); + + expect(stopped.stopped).toMatchObject({ at: flaky }); + expect(retry).toEqual({}); + expect(calls.addComment.map(call => call.id)).toEqual(["first", "flaky", "flaky"]); + expect(store.getAction(first)?.state).toBe("applied"); + expect(store.getAction(flaky)?.state).toBe("applied"); + }); + + it("never applies a staged action whose submission has not completed", async () => { + const { api, calls } = makeApi(); + const store = storeWith(api); + const staged = store.nextActionId(); + store.putAction({ + id: staged, action: { type: "addComment", contentId: "staged", text: "early" }, + state: "staged", submittedAt: staged, + }); + const pending = stage(store, { type: "addComment", contentId: "later", text: "late" }); + + const result = await applyStoredActionsThrough(store, pending, []); + + expect(result).toEqual({}); + expect(calls.addComment.map(call => call.id)).toEqual(["later"]); + expect(store.getAction(staged)?.state).toBe("staged"); + }); + + it("persists transitive invalidations and reports them again on retry", async () => { + const { api, calls } = makeApi(); + const store = storeWith(api); + const root = stage(store, { + type: "createContent", provisionalId: "~root", kind: "page", + parent: { type: "space", spaceKey: "ENG" }, title: "Root", status: "current", + }); + const edit = stage(store, { + type: "setTitle", contentId: "~root", title: "Edited", previousTitle: "Root", + }); + const child = stage(store, { + type: "createContent", provisionalId: "~child", kind: "page", + parent: { type: "page", parentId: "~root", spaceKey: "ENG" }, title: "Child", status: "current", + }); + const childEdit = stage(store, { + type: "addComment", contentId: "~child", text: "Comment", + }); + + const first = await applyStoredActionsThrough(store, root, [root]); + const retry = await applyStoredActionsThrough(store, root, [root]); + + expect(first.invalidatedByVeto).toEqual([ + { action: edit, invalidatedBy: root }, + { action: child, invalidatedBy: root }, + { action: childEdit, invalidatedBy: root }, + ]); + expect(retry.invalidatedByVeto).toEqual(first.invalidatedByVeto); + // Vetoed and invalidated records are deleted so read overlays recompute without them. + expect(store.getAction(root)).toBeUndefined(); + expect(store.getAction(childEdit)).toBeUndefined(); + expect(store.knowsProvisional("~root")).toBe(false); + expect(calls.addComment).toHaveLength(0); + }); + + it("makes the legacy single-action path throw for a cascade-invalidated action", async () => { + const { api } = makeApi(); + const store = storeWith(api); + const root = stage(store, { + type: "createContent", provisionalId: "~root", kind: "page", + parent: { type: "space", spaceKey: "ENG" }, title: "Root", status: "current", + }); + const edit = stage(store, { + type: "setTitle", contentId: "~root", title: "Edited", previousTitle: "Root", + }); + + await applyStoredActionsThrough(store, root, [root]); + + // An un-migrated overseer applying the orphan must see a failure, not a silent success. + await expect(applyStoredAction(store, edit)).rejects.toThrow(`Unknown action: ${edit}`); + }); + + it("ignores a veto of an already-applied action", async () => { + const { api } = makeApi(); + const store = storeWith(api); + const id = stage(store, { type: "addComment", contentId: "page", text: "Comment" }); + await applyStoredActionsThrough(store, id, []); + + const result = await applyStoredActionsThrough(store, id, [id]); + + expect(result).toEqual({}); + expect(store.getAction(id)?.state).toBe("applied"); + }); + + it("rejects an out-of-range veto before changing state", async () => { + const { api, calls } = makeApi(); + const store = storeWith(api); + const id = stage(store, { type: "addComment", contentId: "page", text: "Comment" }); + + await expect(applyStoredActionsThrough(store, id, [id + 1])) + .rejects.toThrow("Invalid veto action ID"); + + expect(store.getAction(id)?.state).toBe("pending"); + expect(calls.addComment).toHaveLength(0); + }); + + it("persists later vetoes before an earlier action fails", async () => { + const { api } = makeApi(); + const store = storeWith(api); + const failed = stage(store, { type: "addComment", contentId: "failed", text: "Comment" }); + const vetoed = stage(store, { + type: "createContent", provisionalId: "~root", kind: "page", + parent: { type: "space", spaceKey: "ENG" }, title: "Root", status: "current", + }); + const invalidated = stage(store, { + type: "setTitle", contentId: "~root", title: "Edited", previousTitle: "Root", + }); + api.addComment = async () => { throw new Error("safe failure"); }; + + const result = await applyStoredActionsThrough(store, invalidated, [vetoed]); + + expect(result.stopped?.at).toBe(failed); + expect(result.invalidatedByVeto).toEqual([{ action: invalidated, invalidatedBy: vetoed }]); + expect(store.getAction(vetoed)).toBeUndefined(); + expect(store.getAction(invalidated)).toBeUndefined(); + }); +}); + +describe("stageAction", () => { + it("keeps the record staged until submitAction completes", async () => { + const { api } = makeApi(); + const store = storeWith(api); + let stateDuringSubmit: string | undefined; + const approvalQueue = { + submitAction: async (id: number) => { + stateDuringSubmit = store.getAction(id)?.state; + }, + } as unknown as Parameters[1]; + + const id = await stageAction(store, approvalQueue, { + type: "addComment", contentId: "page", text: "Comment", + }); + + expect(stateDuringSubmit).toBe("staged"); + expect(store.getAction(id)?.state).toBe("pending"); + }); + + it("rolls the record back when submitAction fails", async () => { + const { api } = makeApi(); + const store = storeWith(api); + const approvalQueue = { + submitAction: async () => { throw new Error("submit failed"); }, + } as unknown as Parameters[1]; + + await expect(stageAction(store, approvalQueue, { + type: "addComment", contentId: "page", text: "Comment", + })).rejects.toThrow("submit failed"); + + expect(store.allActions()).toHaveLength(0); + }); +}); + describe("revertStoredAction", () => { it("marks the action reverted on success", async () => { const { api } = makeApi(); diff --git a/packages/gatekeeper-confluence/package.json b/packages/gatekeeper-confluence/package.json index 9bb0bbb41..015e7eb9e 100644 --- a/packages/gatekeeper-confluence/package.json +++ b/packages/gatekeeper-confluence/package.json @@ -10,6 +10,7 @@ "test:run": "vitest run" }, "dependencies": { + "@gadgets/backend-utils": "workspace:*", "@gadgets/configurator-ui": "workspace:*", "@gadgets/workshop-shared": "workspace:*", "capnweb": "catalog:", diff --git a/packages/gatekeeper-confluence/src/confluence-actions.ts b/packages/gatekeeper-confluence/src/confluence-actions.ts index db6dc8953..5a4866ca1 100644 --- a/packages/gatekeeper-confluence/src/confluence-actions.ts +++ b/packages/gatekeeper-confluence/src/confluence-actions.ts @@ -9,7 +9,18 @@ // without it. import type { RpcStub } from "cloudflare:workers"; -import type { ActionDescription, ApprovalQueue, ObservationDescription } from "@gadgets/workshop-shared/gatekeeper"; +import { + InvalidationLog, + displayReason, + validateApplyThroughArgs, +} from "@gadgets/backend-utils/gatekeeper-action"; +import { createLogger } from "@gadgets/backend-utils/logger"; +import type { + ActionDescription, + ApplyActionsThroughResult, + ApprovalQueue, + ObservationDescription, +} from "@gadgets/workshop-shared/gatekeeper"; import { ConfluenceApi, contentBodyMarkdown, @@ -19,6 +30,15 @@ import { import { markdownToStorage, storageToMarkdown } from "./confluence-markdown"; import type { Comment, ContentSummary, ContentType } from "./types"; +/** Observability fields emitted by Confluence action resolution. */ +type ConfluenceActionLogFields = { actionId: number; vendorId: string }; + +const VENDOR_ID = "confluence"; + +const logger = createLogger({ + component: "gatekeeper.confluence.actions", vendorId: VENDOR_ID, +}); + // --------------------------------------------------------------------------------------------- // Action model @@ -56,7 +76,11 @@ export type ConfluenceAction = export type StoredActionRecord = { id: number; action: ConfluenceAction; - state: "pending" | "applied" | "reverted"; + /** + * "staged" means submitAction() has not completed yet: the record overlays reads like a pending + * one, but applyStoredActionsThrough() must not apply it. + */ + state: "staged" | "pending" | "applied" | "reverted"; submittedAt: number; /** For createContent / addComment: the real content ID assigned on apply. */ createdContentId?: string; @@ -81,9 +105,13 @@ export class ConfluenceStore { #kv: Kv; #api: ConfluenceApi; + /** Durable attribution of veto-cascade invalidations, re-reported on repeated requests. */ + readonly invalidations: InvalidationLog; + constructor(kv: Kv, api: ConfluenceApi) { this.#kv = kv; this.#api = api; + this.invalidations = new InvalidationLog(kv); } get api(): ConfluenceApi { @@ -124,8 +152,9 @@ export class ConfluenceStore { .toSorted((a, b) => a.id - b.id); } + /** Not-yet-applied actions, including staged ones (read overlays must reflect both). */ pendingActions(): StoredActionRecord[] { - return this.allActions().filter(r => r.state === "pending"); + return this.allActions().filter(r => r.state === "pending" || r.state === "staged"); } /** Pending actions targeting a piece of content (addressed by either provisional or real ID). */ @@ -417,18 +446,25 @@ function truncate(text: string, max = 2000): string { // --------------------------------------------------------------------------------------------- // Staging -/** Record a pending action and submit it for approval. Rolls back the record if submit fails. */ +/** Record a staged action and submit it for approval. Rolls back the record if submit fails. */ export async function stageAction( store: ConfluenceStore, approvalQueue: RpcStub, action: ConfluenceAction, ): Promise { const id = store.nextActionId(); - store.putAction({ id, action, state: "pending", submittedAt: Date.now() }); + store.putAction({ id, action, state: "staged", submittedAt: Date.now() }); try { await approvalQueue.submitAction(id, describeAction(action)); } catch (err) { store.deleteAction(id); throw err; } + // Only now may the action be applied: the overseer has accepted it, so a decision frontier can + // legitimately cover it. A concurrent veto cascade may have deleted the record meanwhile. + const record = store.getAction(id); + if (record?.state === "staged") { + record.state = "pending"; + store.putAction(record); + } return id; } @@ -568,43 +604,88 @@ export async function applyStoredAction(store: ConfluenceStore, id: number): Pro await applyAction(store, record); } -export function rejectStoredAction(store: ConfluenceStore, id: number): void | { restart?: boolean } { - const record = store.getAction(id); - if (!record) return; - store.deleteAction(id); - - // Rejecting a creation invalidates any pending actions on that (now-nonexistent) content, - // including child pages created under it, transitively. Cascade-delete and request a restart. - if (record.action.type === "createContent") { - const pending = store.pendingActions(); - const purge = new Set([record.action.provisionalId]); - for (;;) { - let added = false; - for (const r of pending) { - if (r.action.type === "createContent" && r.action.parent.type === "page" && - purge.has(r.action.parent.parentId) && !purge.has(r.action.provisionalId)) { - purge.add(r.action.provisionalId); - added = true; - } +/** + * Rejecting a creation invalidates any pending actions on that (now-nonexistent) content, + * including child pages created under it, transitively. Cascade-delete them all, recording which + * veto invalidated each so a repeated request can re-report the attribution. + */ +function cascadeRejectedCreation(store: ConfluenceStore, record: StoredActionRecord): void { + if (record.action.type !== "createContent") return; + + // Snapshot the pending set once — deleting actions below would otherwise change it under us. + const pending = store.pendingActions(); + const purge = new Set([record.action.provisionalId]); + for (;;) { + let added = false; + for (const candidate of pending) { + if (candidate.action.type === "createContent" && candidate.action.parent.type === "page" && + purge.has(candidate.action.parent.parentId) && !purge.has(candidate.action.provisionalId)) { + purge.add(candidate.action.provisionalId); + added = true; } - if (!added) break; } - let deleted = false; - for (const r of pending) { - const t = actionContentId(r.action); - if (t !== null && purge.has(t)) { - store.deleteAction(r.id); - deleted = true; - } + if (!added) break; + } + + for (const candidate of pending) { + const target = actionContentId(candidate.action); + if (target !== null && purge.has(target)) { + store.deleteAction(candidate.id); + store.invalidations.record(candidate.id, record.id); } - return deleted ? { restart: true } : undefined; } +} + +/** Delete vetoed records and cascade to actions they invalidate. Settled records are left alone. */ +function rejectRecords(store: ConfluenceStore, vetoes: Set): void { + const rejected: StoredActionRecord[] = []; + for (const id of vetoes) { + const record = store.getAction(id); + if (!record || record.state === "applied" || record.state === "reverted") continue; + store.deleteAction(id); + rejected.push(record); + } + for (const record of rejected) cascadeRejectedCreation(store, record); +} + +/** Resolve all stored actions through a Gatekeeper-local action ID. */ +export async function applyStoredActionsThrough( + store: ConfluenceStore, actionId: number, vetoes: number[], +): Promise { + const vetoSet = validateApplyThroughArgs(actionId, vetoes); + rejectRecords(store, vetoSet); + store.invalidations.prune(vetoSet, store.pendingActions()[0]?.id ?? Infinity); + + const invalidatedByVeto = store.invalidations.attributedTo(vetoSet); + const invalidations = invalidatedByVeto.length > 0 ? { invalidatedByVeto } : {}; + for (const record of store.pendingActions()) { + if (record.id > actionId) break; + if (record.state === "staged") continue; // submitAction() has not completed; not coverable yet + try { + await applyAction(store, record); + } catch (error) { + logger.warn("failed to apply action", { + event: "action.apply.failed", + actionId: record.id, + error, + }); + return { + ...invalidations, + stopped: { + at: record.id, + reason: displayReason(error, "Confluence could not apply this action"), + }, + }; + } + } + return invalidations; +} - const target = actionContentId(record.action); - if (target && store.pendingForContent(target).length > 0) return { restart: true }; +export function rejectStoredAction(store: ConfluenceStore, id: number): void { + rejectRecords(store, new Set([id])); } -type RevertResult = void | { message?: string; canRetry?: boolean; restart?: boolean }; +type RevertResult = void | { message?: string; canRetry?: boolean }; export async function revertStoredAction(store: ConfluenceStore, id: number): Promise { const record = store.getAction(id); diff --git a/packages/gatekeeper-confluence/src/confluence.ts b/packages/gatekeeper-confluence/src/confluence.ts index a2a033f01..b98431ff4 100644 --- a/packages/gatekeeper-confluence/src/confluence.ts +++ b/packages/gatekeeper-confluence/src/confluence.ts @@ -15,6 +15,7 @@ // 7. Observer verification — all bindings track independently restricted spaces and content. import { DurableObject, RpcStub, RpcTarget, WorkerEntrypoint } from "cloudflare:workers"; +import { SerialTaskQueue } from "@gadgets/backend-utils/gatekeeper-action"; import { skipRpcValidation, validateRpc } from "capnweb-validate"; import { type AccountDescription, @@ -58,6 +59,7 @@ import { } from "./confluence-api"; import { ConfluenceStore, + applyStoredActionsThrough, applyStoredAction, observation, overlayChildPages, @@ -568,6 +570,7 @@ function makeApi(ctx: { exports: Cloudflare.Env }, props: BaseProps): Confluence @validateRpc() export class ConfluenceSiteGatekeeperImpl extends DurableObject implements Gatekeeper { + #actionResolution = new SerialTaskQueue(); #store() { return new ConfluenceStore(this.ctx.storage.kv, makeApi(this.ctx, this.ctx.props)); } #tracker() { return new ConfluenceObserverTracker(this.ctx.storage.kv, this.ctx.props.cloudId); } @@ -609,14 +612,24 @@ export class ConfluenceSiteGatekeeperImpl extends DurableObject { this.#tracker().removeObserver(id); } - async applyAction(action: number): Promise { await applyStoredAction(this.#store(), action); } - async rejectAction(action: number) { return rejectStoredAction(this.#store(), action); } - async revertAction(action: number) { return await revertStoredAction(this.#store(), action); } + applyActionsThrough(actionId: number, vetoes: number[]) { + return this.#actionResolution.run(() => applyStoredActionsThrough(this.#store(), actionId, vetoes)); + } + applyAction(action: number): Promise { + return this.#actionResolution.run(() => applyStoredAction(this.#store(), action)); + } + rejectAction(action: number) { + return this.#actionResolution.run(() => rejectStoredAction(this.#store(), action)); + } + revertAction(action: number) { + return this.#actionResolution.run(() => revertStoredAction(this.#store(), action)); + } } @validateRpc() export class ConfluenceSpaceGatekeeperImpl extends DurableObject implements Gatekeeper { + #actionResolution = new SerialTaskQueue(); #store() { return new ConfluenceStore(this.ctx.storage.kv, makeApi(this.ctx, this.ctx.props)); } #tracker() { return new ConfluenceObserverTracker(this.ctx.storage.kv, this.ctx.props.cloudId); } @@ -655,14 +668,24 @@ export class ConfluenceSpaceGatekeeperImpl extends DurableObject { this.#tracker().removeObserver(id); } - async applyAction(action: number): Promise { await applyStoredAction(this.#store(), action); } - async rejectAction(action: number) { return rejectStoredAction(this.#store(), action); } - async revertAction(action: number) { return await revertStoredAction(this.#store(), action); } + applyActionsThrough(actionId: number, vetoes: number[]) { + return this.#actionResolution.run(() => applyStoredActionsThrough(this.#store(), actionId, vetoes)); + } + applyAction(action: number): Promise { + return this.#actionResolution.run(() => applyStoredAction(this.#store(), action)); + } + rejectAction(action: number) { + return this.#actionResolution.run(() => rejectStoredAction(this.#store(), action)); + } + revertAction(action: number) { + return this.#actionResolution.run(() => revertStoredAction(this.#store(), action)); + } } @validateRpc() export class ConfluenceContentGatekeeperImpl extends DurableObject implements Gatekeeper { + #actionResolution = new SerialTaskQueue(); #store() { return new ConfluenceStore(this.ctx.storage.kv, makeApi(this.ctx, this.ctx.props)); } #tracker() { return new ConfluenceObserverTracker(this.ctx.storage.kv, this.ctx.props.cloudId); } @@ -702,9 +725,18 @@ export class ConfluenceContentGatekeeperImpl extends DurableObject { this.#tracker().removeObserver(id); } - async applyAction(action: number): Promise { await applyStoredAction(this.#store(), action); } - async rejectAction(action: number) { return rejectStoredAction(this.#store(), action); } - async revertAction(action: number) { return await revertStoredAction(this.#store(), action); } + applyActionsThrough(actionId: number, vetoes: number[]) { + return this.#actionResolution.run(() => applyStoredActionsThrough(this.#store(), actionId, vetoes)); + } + applyAction(action: number): Promise { + return this.#actionResolution.run(() => applyStoredAction(this.#store(), action)); + } + rejectAction(action: number) { + return this.#actionResolution.run(() => rejectStoredAction(this.#store(), action)); + } + revertAction(action: number) { + return this.#actionResolution.run(() => revertStoredAction(this.#store(), action)); + } } function accountFor(ctx: { exports: Cloudflare.Env }, userObjectId: string) { diff --git a/packages/gatekeeper-github/__tests__/github-actions.test.ts b/packages/gatekeeper-github/__tests__/github-actions.test.ts new file mode 100644 index 000000000..8879f8dfe --- /dev/null +++ b/packages/gatekeeper-github/__tests__/github-actions.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, it } from "vitest"; +import { + resolveActionVetoes, + type GitHubAction, + type StoredActionRecord, +} from "../src/github"; + +function action(id: number, fields: Partial): GitHubAction { + return { + approvalId: id, + submittedAt: id, + owner: "cloudflare", + repo: "gadgets", + type: "postComment", + targetKind: "issue", + targetId: "1", + bodyMarkdown: "body", + provisionalCommentId: `~comment${id}`, + ...fields, + } as GitHubAction; +} + +function record(storedAction: GitHubAction, state: StoredActionRecord["state"] = "pending"): + StoredActionRecord { + return { action: storedAction, state }; +} + +describe("resolveActionVetoes", () => { + it("invalidates future resource dependencies and reports them on retry", () => { + const root = record(action(1, { + type: "createIssue", provisionalId: "~issue1", options: { title: "Issue" }, + })); + const edit = record(action(7, { + type: "setTitle", targetKind: "issue", targetId: "~issue1", title: "New", previousTitle: "Old", + })); + const comment = record(action(9, { targetKind: "issue", targetId: "~issue1" })); + const records = [root, edit, comment]; + + const first = resolveActionVetoes(records, new Set([1]), 100); + const retry = resolveActionVetoes(records, new Set([1]), 200); + + expect(first.invalidatedByVeto).toEqual([ + { action: 7, invalidatedBy: 1 }, + { action: 9, invalidatedBy: 1 }, + ]); + expect(retry.invalidatedByVeto).toEqual(first.invalidatedByVeto); + expect(root).toMatchObject({ state: "rejected", rejectedAt: 100 }); + expect(edit).toMatchObject({ state: "rejected", invalidatedByVeto: 1 }); + }); + + it("does not let a veto overwrite an approved action", () => { + const approved = record(action(1, { + type: "createIssue", provisionalId: "~issue1", options: { title: "Issue" }, + }), "approved"); + + const result = resolveActionVetoes([approved], new Set([1]), 100); + + expect(result.invalidatedByVeto).toBeUndefined(); + expect(result.changed).toHaveLength(0); + expect(approved.state).toBe("approved"); + }); + + it("tracks transitive reply invalidation with direct-veto precedence", () => { + const review = record(action(1, { + type: "postReview", + pullId: "1", + provisionalReviewId: "~review1", + review: { + revision: { baseSha: "base", headSha: "head" }, + decision: "comment", + diffComments: [{ + provisionalCommentId: "~review-comment", + target: { path: "file.ts", line: 1, side: "new", subjectType: "line" }, + bodyMarkdown: "review", + }], + }, + })); + const directReply = record(action(2, { + type: "replyToDiffComment", pullId: "1", commentId: "~review-comment", + bodyMarkdown: "reply", provisionalCommentId: "~reply1", + })); + const nestedReply = record(action(3, { + type: "replyToDiffComment", pullId: "1", commentId: "~reply1", + bodyMarkdown: "nested", provisionalCommentId: "~reply2", + })); + + const result = resolveActionVetoes([review, directReply, nestedReply], new Set([1, 2]), 100); + + expect(result.invalidatedByVeto).toEqual([{ action: 3, invalidatedBy: 2 }]); + expect(directReply.state).toBe("rejected"); + expect(directReply.invalidatedByVeto).toBeUndefined(); + expect(nestedReply).toMatchObject({ state: "rejected", invalidatedByVeto: 2 }); + }); +}); diff --git a/packages/gatekeeper-github/__tests__/github-batch.test.ts b/packages/gatekeeper-github/__tests__/github-batch.test.ts new file mode 100644 index 000000000..065909fbe --- /dev/null +++ b/packages/gatekeeper-github/__tests__/github-batch.test.ts @@ -0,0 +1,103 @@ +// DO-level batch-resolution tests: exercise applyActionsThrough against a real +// GitHubGatekeeperImpl facet (real storage, real staging flow), on the veto paths that never +// reach the GitHub API. + +import { env } from "cloudflare:workers"; +import { describe, expect, it } from "vitest"; +import type { ActionDescription } from "@gadgets/workshop-shared/gatekeeper"; +import type { GitHubAction } from "../src/github.js"; +import type { GitHubTestParent } from "./worker.js"; + +const testEnv = env as unknown as { + GITHUB_TEST_PARENT: DurableObjectNamespace; +}; + +let uniqueParent = 0; +function makeParent() { + return testEnv.GITHUB_TEST_PARENT.getByName(`parent-${++uniqueParent}`); +} + +function description(title: string): ActionDescription { + return { title, description: title, implementsRevert: false }; +} + +function createIssue(approvalId: number, provisionalId: string): GitHubAction { + return { + type: "createIssue", + approvalId, + submittedAt: approvalId, + owner: "cloudflare", + repo: "gadgets", + provisionalId, + options: { title: `Issue ${approvalId}` }, + } as GitHubAction; +} + +function postComment(approvalId: number, targetId: string): GitHubAction { + return { + type: "postComment", + approvalId, + submittedAt: approvalId, + owner: "cloudflare", + repo: "gadgets", + targetKind: "issue", + targetId, + bodyMarkdown: `Comment ${approvalId}`, + provisionalCommentId: `~comment${approvalId}`, + } as GitHubAction; +} + +describe("GitHubGatekeeperImpl.applyActionsThrough", () => { + it("cascade-invalidates dependents of a vetoed creation and re-reports on retry", async () => { + const parent = makeParent(); + await parent.submitAction(createIssue(1, "~issue1"), description("Create issue")); + await parent.submitAction(postComment(2, "~issue1"), description("Comment on it")); + + const first = await parent.applyActionsThrough(2, [1]); + const retry = await parent.applyActionsThrough(2, [1]); + + expect(first.invalidatedByVeto).toEqual([{ action: 2, invalidatedBy: 1 }]); + expect(retry.invalidatedByVeto).toEqual(first.invalidatedByVeto); + expect(first.stopped).toBeUndefined(); + }); + + it("makes the legacy single-action path refuse a cascade-invalidated action", async () => { + const parent = makeParent(); + await parent.submitAction(createIssue(1, "~issue1"), description("Create issue")); + await parent.submitAction(postComment(2, "~issue1"), description("Comment on it")); + + await parent.applyActionsThrough(2, [1]); + + // An un-migrated overseer applying the orphan must see a failure, not a silent success. + await expect(parent.applyAction(2)).rejects.toThrow("no longer pending"); + }); + + it("ignores vetoes of unknown actions and returns an empty result", async () => { + const parent = makeParent(); + await parent.submitAction(createIssue(1, "~issue1"), description("Create issue")); + await parent.applyActionsThrough(1, [1]); + + const result = await parent.applyActionsThrough(5, [3, 5]); + + expect(result).toEqual({}); + }); + + it("rejects an out-of-range veto across the RPC boundary", async () => { + const parent = makeParent(); + await parent.submitAction(createIssue(1, "~issue1"), description("Create issue")); + + await expect(parent.applyActionsThrough(1, [2])).rejects.toThrow("Invalid veto action ID"); + }); + + it("keeps the legacy reject cascading, with attribution durably re-reportable", async () => { + const parent = makeParent(); + await parent.submitAction(createIssue(1, "~issue1"), description("Create issue")); + await parent.submitAction(postComment(2, "~issue1"), description("Comment on it")); + + await parent.rejectAction(1); + + // The cascade recorded attribution durably, so a later batch call can still report it. + const result = await parent.applyActionsThrough(2, [1]); + expect(result.invalidatedByVeto).toEqual([{ action: 2, invalidatedBy: 1 }]); + }); +}); diff --git a/packages/gatekeeper-github/__tests__/worker.ts b/packages/gatekeeper-github/__tests__/worker.ts new file mode 100644 index 000000000..fc904b302 --- /dev/null +++ b/packages/gatekeeper-github/__tests__/worker.ts @@ -0,0 +1,58 @@ +import { DurableObject, RpcTarget } from "cloudflare:workers"; +import type { RpcStub } from "cloudflare:workers"; +import type { + ActionDescription, + ApplyActionsThroughResult, + ApprovalQueue, +} from "@gadgets/workshop-shared/gatekeeper"; +import type { GitHubAction, GitHubGatekeeperImpl } from "../src/github.js"; + +export { default } from "../src/github.js"; +// Vitest's ctx.exports analyzer needs the classes named directly, not through a barrel. +export { + GatekeeperVendor, + GatekeeperUserImpl, + GitHubGatekeeperImpl, + GitHubVerifier, + UserAccount, +} from "../src/github.js"; + +// Approval queue that accepts every submission, standing in for the workshop overseer. +class AcceptingApprovalQueue extends RpcTarget { + async submitAction(): Promise {} + async authorizeObservation(): Promise {} +} + +type FacetExports = { + GitHubGatekeeperImpl: DurableObjectClass; +}; + +/** Test-only parent hosting a GitHubGatekeeperImpl facet the way the workshop overseer does. */ +export class GitHubTestParent extends DurableObject { + #gatekeeper() { + const exports = this.ctx.exports as unknown as FacetExports; + // The facet runs without props: the veto/staging paths under test never read ctx.props (only + // the GitHub API paths dereference the user account and repo coordinates). + return this.ctx.facets.get("gatekeeper", () => ({ + class: exports.GitHubGatekeeperImpl, + })); + } + + /** Stage and submit an action, as a session would. */ + async submitAction(action: GitHubAction, description: ActionDescription): Promise { + await this.#gatekeeper().submitActionForApproval( + new AcceptingApprovalQueue() as unknown as RpcStub, action, description); + } + + applyActionsThrough(actionId: number, vetoes: number[]): Promise { + return this.#gatekeeper().applyActionsThrough(actionId, vetoes); + } + + applyAction(actionId: number): Promise { + return this.#gatekeeper().applyAction(actionId); + } + + rejectAction(actionId: number): Promise { + return this.#gatekeeper().rejectAction(actionId); + } +} diff --git a/packages/gatekeeper-github/package.json b/packages/gatekeeper-github/package.json index f427700ae..067f45825 100644 --- a/packages/gatekeeper-github/package.json +++ b/packages/gatekeeper-github/package.json @@ -17,6 +17,7 @@ "capnweb-validate": "catalog:" }, "devDependencies": { + "@cloudflare/vitest-pool-workers": "catalog:", "typescript": "catalog:", "vitest": "catalog:", "wrangler": "catalog:" diff --git a/packages/gatekeeper-github/src/github.ts b/packages/gatekeeper-github/src/github.ts index 8c8bde5e9..073eb3bb3 100644 --- a/packages/gatekeeper-github/src/github.ts +++ b/packages/gatekeeper-github/src/github.ts @@ -1,10 +1,17 @@ import { DurableObject, RpcStub, RpcTarget, WorkerEntrypoint } from "cloudflare:workers"; +import { + SerialTaskQueue, + displayReason, + validateApplyThroughArgs, + type VetoInvalidation, +} from "@gadgets/backend-utils/gatekeeper-action"; import { skipRpcValidation, validateRpc } from "capnweb-validate"; import { ApprovalQueue, stripTrailingSlashes, type ActionDescription, type AccountDescription, + type ApplyActionsThroughResult, type Cursor, type Gatekeeper, type GatekeeperConnectCallback, @@ -141,7 +148,7 @@ type StoredProvisionalResource = { realId?: string; }; -type StoredActionState = "staged" | "pending" | "approved" | "rejected"; +export type StoredActionState = "staged" | "pending" | "approved" | "rejected"; type GitHubRevertInfo = | { @@ -242,7 +249,7 @@ type MergePullRequestAction = BaseAction & { options?: GitHubPullRequestMergeOptions; }; -type GitHubAction = +export type GitHubAction = | CreateIssueAction | CreatePullRequestAction | SetTitleAction @@ -255,14 +262,126 @@ type GitHubAction = | ReplyToDiffCommentAction | MergePullRequestAction; -type StoredActionRecord = { +export type StoredActionRecord = { action: GitHubAction; state: StoredActionState; appliedAt?: number; rejectedAt?: number; + /** Directly-vetoed action whose rejection invalidated this one. Absent on a direct veto. */ + invalidatedByVeto?: number; revertInfo?: GitHubRevertInfo; }; +function dependsOnResource(action: GitHubAction, kind: EntityKind, provisionalId: string): boolean { + switch (action.type) { + case "createIssue": + case "createPullRequest": + return action.provisionalId === provisionalId; + case "setTitle": + case "setBody": + case "addLabels": + case "removeLabels": + case "changeState": + case "postComment": + return action.targetKind === kind && action.targetId === provisionalId; + case "postReview": + case "replyToDiffComment": + case "mergePullRequest": + return kind === "pull" && action.pullId === provisionalId; + } +} + +function replyRoots(action: GitHubAction): string[] { + if (action.type === "postReview") { + return (action.review.diffComments ?? []).map(comment => comment.provisionalCommentId); + } + return action.type === "replyToDiffComment" ? [action.provisionalCommentId] : []; +} + +/** State changes produced by applying a set of vetoes to GitHub action records. */ +export interface ResolvedActionVetoes { + /** Records changed by this resolution attempt. */ + changed: StoredActionRecord[]; + + /** Records directly vetoed by this request, including previously-vetoed roots. */ + directlyRejected: StoredActionRecord[]; + + /** Persisted dependency invalidations attributable to this request's vetoes, ascending. */ + invalidatedByVeto?: VetoInvalidation[]; +} + +/** Apply direct vetoes and GitHub's known resource and reply dependencies to stored records. */ +export function resolveActionVetoes( + records: StoredActionRecord[], vetoes: Set, rejectedAt: number, +): ResolvedActionVetoes { + const byId = new Map(records.map(record => [record.action.approvalId, record])); + const changed = new Map(); + const directlyRejected: StoredActionRecord[] = []; + + for (const id of vetoes) { + const record = byId.get(id); + if (!record || record.state === "approved") continue; + const wasDirect = record.state === "rejected" && record.invalidatedByVeto === undefined; + record.state = "rejected"; + record.rejectedAt ??= rejectedAt; + delete record.invalidatedByVeto; + directlyRejected.push(record); + if (!wasDirect) changed.set(id, record); + } + + const rejectDependency = (record: StoredActionRecord, rootId: number) => { + if (record.state !== "pending" && record.state !== "staged") return false; + record.state = "rejected"; + record.rejectedAt = rejectedAt; + record.invalidatedByVeto = rootId; + changed.set(record.action.approvalId, record); + return true; + }; + + for (const root of directlyRejected) { + const rootId = root.action.approvalId; + if (root.action.type === "createIssue" || root.action.type === "createPullRequest") { + const kind = root.action.type === "createIssue" ? "issue" : "pull"; + for (const candidate of records) { + if (!vetoes.has(candidate.action.approvalId) && + dependsOnResource(candidate.action, kind, root.action.provisionalId)) { + rejectDependency(candidate, rootId); + } + } + } + + const queue = replyRoots(root.action); + const seen = new Set(queue); + while (queue.length > 0) { + const commentId = queue.shift()!; + for (const candidate of records) { + const action = candidate.action; + if (action.type !== "replyToDiffComment" || action.commentId !== commentId || + vetoes.has(action.approvalId)) continue; + if (rejectDependency(candidate, rootId) || + (candidate.state === "rejected" && candidate.invalidatedByVeto === rootId)) { + if (!seen.has(action.provisionalCommentId)) { + seen.add(action.provisionalCommentId); + queue.push(action.provisionalCommentId); + } + } + } + } + } + + const invalidations: VetoInvalidation[] = records + .filter(record => record.state === "rejected" && record.invalidatedByVeto !== undefined && + vetoes.has(record.invalidatedByVeto)) + .map(record => ({ action: record.action.approvalId, invalidatedBy: record.invalidatedByVeto! })) + .toSorted((a, b) => a.action - b.action); + + return { + changed: [...changed.values()], + directlyRejected, + ...(invalidations.length > 0 && { invalidatedByVeto: invalidations }), + }; +} + const NONCE_BYTES = 32; const INITIATION_NONCE_LIFETIME_MS = 10 * 60 * 1000; const OAUTH_NONCE_LIFETIME_MS = 10 * 60 * 1000; @@ -1382,6 +1501,7 @@ export class GitHubVerifier extends WorkerEntrypoint export class GitHubGatekeeperImpl extends DurableObject implements Gatekeeper { + #actionResolution = new SerialTaskQueue(); #pendingActionsCache?: GitHubAction[]; #userAccount() { @@ -1881,6 +2001,17 @@ export class GitHubGatekeeperImpl extends DurableObject(this.#retiredActionRecordKey(approvalId)); } + #listActionRecords(): StoredActionRecord[] { + const records = new Map(); + for (const [, record] of this.ctx.storage.kv.list({ prefix: "retiredAction:" })) { + records.set(record.action.approvalId, record); + } + for (const [, record] of this.ctx.storage.kv.list({ prefix: "action:" })) { + records.set(record.action.approvalId, record); + } + return [...records.values()].toSorted((a, b) => a.action.approvalId - b.action.approvalId); + } + #requireActionRecord(approvalId: number): StoredActionRecord { const record = this.#getActionRecord(approvalId); if (!record) { @@ -1889,8 +2020,11 @@ export class GitHubGatekeeperImpl extends DurableObject(`provisional:${action.provisionalId}`, { kind: action.type === "createIssue" ? "issue" : "pull", @@ -1942,67 +2076,6 @@ export class GitHubGatekeeperImpl extends DurableObject action.type === "replyToDiffComment", - ); - const queue = [...rootCommentIds]; - const seen = new Set(rootCommentIds); - - while (queue.length > 0) { - const current = queue.shift(); - if (!current) { - break; - } - for (const reply of pendingReplies) { - if (reply.commentId === current) { - this.#markActionRejected(reply); - if (!seen.has(reply.provisionalCommentId)) { - seen.add(reply.provisionalCommentId); - queue.push(reply.provisionalCommentId); - } - } - } - } } #getProvisionalResource(id: string): StoredProvisionalResource | undefined { @@ -3251,7 +3324,7 @@ export class GitHubGatekeeperImpl extends DurableObject { + async #applyAction(actionId: number): Promise { const record = this.#requireActionRecord(actionId); if (record.state !== "pending" && record.state !== "staged") { throw new Error(`GitHub action ${actionId} is no longer pending.`); @@ -3440,6 +3513,51 @@ export class GitHubGatekeeperImpl extends DurableObject { + const vetoSet = validateApplyThroughArgs(actionId, vetoes); + const resolution = resolveActionVetoes(this.#listActionRecords(), vetoSet, Date.now()); + for (const record of resolution.changed) { + this.#retireActionRecord(record.action.approvalId, record); + } + for (const record of resolution.directlyRejected) { + const action = record.action; + if (action.type === "createIssue" || action.type === "createPullRequest") { + this.ctx.storage.kv.delete(`provisional:${action.provisionalId}`); + } + } + if (resolution.changed.length > 0) this.#clearCaches(); + + for (const record of this.#listActionRecords()) { + if (record.action.approvalId > actionId) break; + // A "staged" record's submitAction() has not completed; the contract forbids applying it. + if (record.state !== "pending") continue; + try { + await this.#applyAction(record.action.approvalId); + } catch (error) { + logger.warn("failed to apply action", { + event: "action.apply.failed", + actionId: record.action.approvalId, + error, + }); + return { + stopped: { + at: record.action.approvalId, + reason: displayReason(error, "GitHub could not apply this action"), + }, + ...(resolution.invalidatedByVeto && { + invalidatedByVeto: resolution.invalidatedByVeto, + }), + }; + } + } + + return resolution.invalidatedByVeto + ? { invalidatedByVeto: resolution.invalidatedByVeto } + : {}; + } + async #resolveReplyTarget(commentId: string): Promise { const pendingReplies = new Map( this.#listPendingActions() @@ -3478,32 +3596,28 @@ export class GitHubGatekeeperImpl extends DurableObject { + #rejectAction(actionId: number): void | { restart?: boolean } { const record = this.#requireActionRecord(actionId); const action = record.action; if (record.state !== "pending" && record.state !== "staged") { throw new Error(`GitHub action ${actionId} is no longer pending.`); } - this.#markActionRejected(action); + const resolution = resolveActionVetoes(this.#listActionRecords(), new Set([actionId]), Date.now()); + for (const changed of resolution.changed) { + this.#retireActionRecord(changed.action.approvalId, changed); + } if (action.type === "createIssue" || action.type === "createPullRequest") { - this.#rejectActionsForResource(action.type === "createIssue" ? "issue" : "pull", action.provisionalId); this.ctx.storage.kv.delete(`provisional:${action.provisionalId}`); this.#clearCaches(); return { restart: true }; } - if (action.type === "postReview") { - this.#rejectReplyDependencyChain((action.review.diffComments ?? []).map(comment => comment.provisionalCommentId)); - } else if (action.type === "replyToDiffComment") { - this.#rejectReplyDependencyChain([action.provisionalCommentId]); - } - this.#clearCaches(); return; } - async revertAction(actionId: number): Promise { + async #revertAction(actionId: number): Promise { const record = this.#requireActionRecord(actionId); const action = record.action; const revertInfo = record.revertInfo; @@ -3567,6 +3681,22 @@ export class GitHubGatekeeperImpl extends DurableObject this.#applyActionsThrough(actionId, vetoes)); + } + + applyAction(actionId: number): Promise { + return this.#actionResolution.run(() => this.#applyAction(actionId)); + } + + rejectAction(actionId: number): Promise { + return this.#actionResolution.run(() => this.#rejectAction(actionId)); + } + + revertAction(actionId: number) { + return this.#actionResolution.run(() => this.#revertAction(actionId)); + } + async repoMetadata(): Promise { return this.#getRepoMetadata(); } diff --git a/packages/gatekeeper-github/src/observability.ts b/packages/gatekeeper-github/src/observability.ts index 9affdf2fe..0548a8d3d 100644 --- a/packages/gatekeeper-github/src/observability.ts +++ b/packages/gatekeeper-github/src/observability.ts @@ -1,7 +1,7 @@ import { createObservabilityContext } from "@gadgets/backend-utils/observability-context"; /** Observability fields emitted by the GitHub gatekeeper. */ -export type GitHubObservabilityFields = { vendorId: string }; +export type GitHubObservabilityFields = { vendorId: string; actionId?: number }; /** Ambient observability fields for one GitHub gatekeeper operation. */ export const obsContext = createObservabilityContext(); diff --git a/packages/gatekeeper-github/vitest.config.ts b/packages/gatekeeper-github/vitest.config.ts index cb8781017..8a722ee14 100644 --- a/packages/gatekeeper-github/vitest.config.ts +++ b/packages/gatekeeper-github/vitest.config.ts @@ -1,8 +1,29 @@ +import { cloudflareTest } from "@cloudflare/vitest-pool-workers"; +import capnwebValidate from "capnweb-validate/vite"; import { defineConfig } from "vitest/config"; +/** + * Tests run inside workerd so they can exercise modules that import "cloudflare:workers" + * (github.ts), not just the standalone API helpers. + */ export default defineConfig({ + plugins: [ + capnwebValidate(), + cloudflareTest({ + main: "./__tests__/worker.ts", + miniflare: { + compatibilityDate: "2026-02-02", + compatibilityFlags: ["allow_irrevocable_stub_storage", "nodejs_als"], + durableObjects: { + GITHUB_TEST_PARENT: { className: "GitHubTestParent", useSQLite: true }, + GITHUB_GATEKEEPER_IMPL: { className: "GitHubGatekeeperImpl", useSQLite: true }, + }, + }, + }), + ], test: { include: ["__tests__/*.test.ts"], - environment: "node", + // Asserts the pool actually started, rather than trusting a green run to mean workerd. + setupFiles: ["../../test-setup/assert-workerd.ts"], }, }); diff --git a/packages/gatekeeper-linear/src/linear.ts b/packages/gatekeeper-linear/src/linear.ts index 85d36b80e..8b6be76e6 100644 --- a/packages/gatekeeper-linear/src/linear.ts +++ b/packages/gatekeeper-linear/src/linear.ts @@ -1,4 +1,10 @@ import { WorkerEntrypoint, DurableObject, RpcTarget, RpcStub } from "cloudflare:workers"; +import { + InvalidationLog, + SerialTaskQueue, + displayReason, + validateApplyThroughArgs, +} from "@gadgets/backend-utils/gatekeeper-action"; import { skipRpcValidation, validateRpc } from "capnweb-validate"; import { GatekeeperUser, @@ -15,6 +21,7 @@ import { AccountDescription, SupportedResource, ResourceConfiguratorFrame, + type ApplyActionsThroughResult, } from "@gadgets/workshop-shared/gatekeeper"; import type { Cursor, @@ -902,7 +909,11 @@ export class LinearVerifier extends WorkerEntrypoint // --------------------------------------------------------------------------- // Action records — stored in the gatekeeper DO and applied/reverted on approval. -type ActionStatus = "pending" | "applied"; +/** + * "staged" means submitAction() has not completed yet: the record overlays reads like a pending + * one, but applyActionsThrough() must not apply it. + */ +type ActionStatus = "staged" | "pending" | "applied"; // A display-level patch merged into a RawIssue when simulating a pending updateIssue action. // Captured at submit time (with resolved display objects) so reads need no extra lookups. @@ -951,6 +962,13 @@ type LinearGatekeeperImplProps = { export class LinearGatekeeperImpl extends DurableObject implements Gatekeeper { + #actionResolution = new SerialTaskQueue(); + + /** Durable attribution of veto-cascade invalidations, re-reported on repeated requests. */ + #invalidations() { + return new InvalidationLog(this.ctx.storage.kv); + } + // ---- private API access (token never leaves the DO) ---- #account() { @@ -1114,13 +1132,27 @@ export class LinearGatekeeperImpl extends DurableObject { const id = this.#nextCounter("action"); - this.ctx.storage.kv.put(`action:${id}`, { ...action, id, status: "pending" } as StoredAction); + this.ctx.storage.kv.put(`action:${id}`, { ...action, id, status: "staged" } as StoredAction); this.#invalidatePendingActions(); - await approvalQueue.submitAction(id, { - title: description.title, - description: description.body, - implementsRevert: description.implementsRevert, - }); + try { + await approvalQueue.submitAction(id, { + title: description.title, + description: description.body, + implementsRevert: description.implementsRevert, + }); + } catch (err) { + this.ctx.storage.kv.delete(`action:${id}`); + this.#invalidatePendingActions(); + throw err; + } + // Only now may the action be applied: the overseer has accepted it, so a decision frontier can + // legitimately cover it. A concurrent veto cascade may have deleted the record meanwhile. + const record = this.ctx.storage.kv.get(`action:${id}`); + if (record?.status === "staged") { + record.status = "pending"; + this.ctx.storage.kv.put(`action:${id}`, record); + this.#invalidatePendingActions(); + } } // ---- simulation: overlay pending (submitted-but-not-applied) actions onto reads ---- @@ -1131,9 +1163,10 @@ export class LinearGatekeeperImpl extends DurableObject({ prefix: "action:" })] .map(([, value]) => value) - .filter(a => a.status === "pending") + .filter(a => a.status === "pending" || a.status === "staged") .toSorted((a, b) => a.id - b.id); } return this.#pendingActionsCache; @@ -1523,7 +1556,7 @@ export class LinearGatekeeperImpl extends DurableObject { + async #applyAction(actionId: number): Promise { const action = this.ctx.storage.kv.get(`action:${actionId}`); if (!action) throw new Error(`Unknown action: ${actionId}`); @@ -1597,8 +1630,9 @@ export class LinearGatekeeperImpl extends DurableObject { + // Delete a vetoed record and cascade to the actions its rejection invalidates. Settled records + // are left alone. + #rejectRecord(actionId: number): void { const action = this.ctx.storage.kv.get(`action:${actionId}`); + if (!action || action.status === "applied") return; this.ctx.storage.kv.delete(`action:${actionId}`); this.#invalidatePendingActions(); - if (!action) return; // Rejecting a create invalidates every action queued against its provisional id, which can - // never be applied. Drop them all and ask the Overseer to restart the gadget. + // never be applied. if (action.kind === "createIssue") { - this.#cascadeRejectProvisional(action.provisionalId); - return { restart: true }; - } - - // Rejecting a label create invalidates any pending addLabels/removeLabels that referenced its - // provisional id (they could never resolve to a real label). Drop them. No restart needed — - // createLabel returns no handle the gadget holds. - if (action.kind === "createLabel") { - const provisionalLabelId = action.synthetic.id; - for (const dep of this.#pendingActions()) { - if ((dep.kind === "addLabels" || dep.kind === "removeLabels") && - dep.labelIds.includes(provisionalLabelId)) { - this.ctx.storage.kv.delete(`action:${dep.id}`); - } + this.#cascadeRejectProvisional(action.provisionalId, actionId); + } else if (action.kind === "createLabel") { + this.#cascadeRejectLabel(action.synthetic.id, actionId); + } + } + + async #rejectAction(actionId: number): Promise { + this.#rejectRecord(actionId); + } + + async #applyActionsThrough(actionId: number, vetoes: number[]): Promise { + const vetoSet = validateApplyThroughArgs(actionId, vetoes); + for (const veto of vetoSet) this.#rejectRecord(veto); + const log = this.#invalidations(); + log.prune(vetoSet, this.#pendingActions()[0]?.id ?? Infinity); + + const invalidatedByVeto = log.attributedTo(vetoSet); + const invalidations = invalidatedByVeto.length > 0 ? { invalidatedByVeto } : {}; + for (const record of this.#pendingActions()) { + if (record.id > actionId) break; + if (record.status !== "pending") continue; // staged: submitAction() has not completed + try { + await this.#applyAction(record.id); + } catch (error) { + logger.warn("failed to apply action", { event: "action.apply.failed", error }); + return { + ...invalidations, + stopped: { + at: record.id, + reason: displayReason(error, "Linear could not apply this action"), + }, + }; } - this.#invalidatePendingActions(); } + return invalidations; + } + + applyActionsThrough(actionId: number, vetoes: number[]): Promise { + return this.#actionResolution.run(() => this.#applyActionsThrough(actionId, vetoes)); + } + + applyAction(actionId: number): Promise { + return this.#actionResolution.run(() => this.#applyAction(actionId)); + } + + rejectAction(actionId: number): Promise { + return this.#actionResolution.run(() => this.#rejectAction(actionId)); + } + + revertAction(actionId: number): Promise { + return this.#actionResolution.run(() => this.#revertAction(actionId)); } - async revertAction(actionId: number): - Promise { + async #revertAction(actionId: number): + Promise { const action = this.ctx.storage.kv.get(`action:${actionId}`); if (!action) throw new Error(`Unknown action: ${actionId}`); diff --git a/packages/gatekeeper-notion/__tests__/notion-actions.test.ts b/packages/gatekeeper-notion/__tests__/notion-actions.test.ts new file mode 100644 index 000000000..be4e80dc8 --- /dev/null +++ b/packages/gatekeeper-notion/__tests__/notion-actions.test.ts @@ -0,0 +1,106 @@ +import { describe, expect, it } from "vitest"; +import { + NotionStore, + applyStoredActionsThrough, + type NotionAction, +} from "../src/notion-actions"; +import type { NotionApi } from "../src/notion-api"; + +type Kv = ConstructorParameters[0]; + +function makeKv(): Kv { + const map = new Map(); + return { + get: (key: string) => map.get(key) as T | undefined, + put: (key: string, value: unknown) => void map.set(key, value), + delete: (key: string) => void map.delete(key), + list: ({ prefix }: { prefix: string }) => + [...map.entries()].filter(([key]) => key.startsWith(prefix)) as [string, T][], + } as unknown as Kv; +} + +function makeStore() { + const comments: string[] = []; + const api = { + createComment: async ({ parent }: { parent: { page_id: string } }) => { + comments.push(parent.page_id); + if (parent.page_id === "failed") throw new Error("safe failure"); + return {}; + }, + } as unknown as NotionApi; + return { store: new NotionStore(makeKv(), api), comments }; +} + +function stage(store: NotionStore, action: NotionAction): number { + const id = store.nextActionId(); + store.putAction({ id, action, state: "pending", submittedAt: id }); + return id; +} + +describe("applyStoredActionsThrough", () => { + it("skips sparse IDs and stops at the first failed action", async () => { + const { store, comments } = makeStore(); + const first = stage(store, { type: "addComment", pageId: "first", text: "one" }); + const hole = stage(store, { type: "addComment", pageId: "hole", text: "two" }); + const failed = stage(store, { type: "addComment", pageId: "failed", text: "three" }); + const later = stage(store, { type: "addComment", pageId: "later", text: "four" }); + store.deleteAction(hole); + + const result = await applyStoredActionsThrough(store, later, []); + + expect(comments).toEqual(["first", "failed"]); + expect(store.getAction(first)?.state).toBe("applied"); + expect(store.getAction(failed)?.state).toBe("pending"); + expect(store.getAction(later)?.state).toBe("pending"); + expect(result.stopped).toMatchObject({ at: failed, reason: expect.any(Error) }); + }); + + it("persists transitive invalidations and reports them again on retry", async () => { + const { store, comments } = makeStore(); + const root = stage(store, { + type: "createPage", provisionalId: "~root", parent: { kind: "workspace" }, title: "Root", + }); + const child = stage(store, { + type: "createPage", provisionalId: "~child", parent: { kind: "page", pageId: "~root" }, + title: "Child", + }); + const edit = stage(store, { type: "addComment", pageId: "~child", text: "Comment" }); + + const first = await applyStoredActionsThrough(store, root, [root]); + const retry = await applyStoredActionsThrough(store, root, [root]); + + expect(first.invalidatedByVeto).toEqual([ + { action: child, invalidatedBy: root }, + { action: edit, invalidatedBy: root }, + ]); + expect(retry.invalidatedByVeto).toEqual(first.invalidatedByVeto); + // Vetoed and invalidated records are deleted so read overlays recompute without them. + expect(store.getAction(root)).toBeUndefined(); + expect(store.getAction(edit)).toBeUndefined(); + expect(store.knowsProvisional("~root")).toBe(false); + expect(comments).toHaveLength(0); + }); + + it("ignores a veto of an already-applied action", async () => { + const { store, comments } = makeStore(); + const id = stage(store, { type: "addComment", pageId: "page", text: "Comment" }); + await applyStoredActionsThrough(store, id, []); + + const result = await applyStoredActionsThrough(store, id, [id]); + + expect(result).toEqual({}); + expect(store.getAction(id)?.state).toBe("applied"); + expect(comments).toEqual(["page"]); + }); + + it("rejects an out-of-range veto before changing state", async () => { + const { store, comments } = makeStore(); + const id = stage(store, { type: "addComment", pageId: "page", text: "Comment" }); + + await expect(applyStoredActionsThrough(store, id, [id + 1])) + .rejects.toThrow("Invalid veto action ID"); + + expect(store.getAction(id)?.state).toBe("pending"); + expect(comments).toHaveLength(0); + }); +}); diff --git a/packages/gatekeeper-notion/package.json b/packages/gatekeeper-notion/package.json index 8dd05ac74..14f600534 100644 --- a/packages/gatekeeper-notion/package.json +++ b/packages/gatekeeper-notion/package.json @@ -10,6 +10,7 @@ "test:run": "vitest run" }, "dependencies": { + "@gadgets/backend-utils": "workspace:*", "@gadgets/configurator-ui": "workspace:*", "@gadgets/workshop-shared": "workspace:*", "capnweb": "catalog:", diff --git a/packages/gatekeeper-notion/src/notion-actions.ts b/packages/gatekeeper-notion/src/notion-actions.ts index a43248503..05719781d 100644 --- a/packages/gatekeeper-notion/src/notion-actions.ts +++ b/packages/gatekeeper-notion/src/notion-actions.ts @@ -27,7 +27,18 @@ import { type NotionPageResponse, } from "./notion-api"; import type { RpcStub } from "cloudflare:workers"; -import type { ActionDescription, ApprovalQueue, ObservationDescription } from "@gadgets/workshop-shared/gatekeeper"; +import { + InvalidationLog, + displayReason, + validateApplyThroughArgs, +} from "@gadgets/backend-utils/gatekeeper-action"; +import { createLogger } from "@gadgets/backend-utils/logger"; +import type { + ActionDescription, + ApplyActionsThroughResult, + ApprovalQueue, + ObservationDescription, +} from "@gadgets/workshop-shared/gatekeeper"; import type { NotionComment, NotionDatabaseSchema, @@ -40,6 +51,15 @@ import type { NotionUser, } from "./types"; +/** Observability fields emitted by Notion action resolution. */ +type NotionActionLogFields = { actionId: number; vendorId: string }; + +const VENDOR_ID = "notion"; + +const logger = createLogger({ + component: "gatekeeper.notion.actions", vendorId: VENDOR_ID, +}); + // --------------------------------------------------------------------------------------------- // Action model @@ -77,7 +97,11 @@ export type NotionAction = export type StoredActionRecord = { id: number; action: NotionAction; - state: "pending" | "applied" | "reverted"; + /** + * "staged" means submitAction() has not completed yet: the record overlays reads like a pending + * one, but applyStoredActionsThrough() must not apply it. + */ + state: "staged" | "pending" | "applied" | "reverted"; submittedAt: number; /** For appendContent revert: the IDs of the blocks created on apply. */ appendedBlockIds?: string[]; @@ -110,9 +134,13 @@ export class NotionStore { #kv: Kv; #api: NotionApi; + /** Durable attribution of veto-cascade invalidations, re-reported on repeated requests. */ + readonly invalidations: InvalidationLog; + constructor(kv: Kv, api: NotionApi) { this.#kv = kv; this.#api = api; + this.invalidations = new InvalidationLog(kv); } get api(): NotionApi { @@ -153,8 +181,9 @@ export class NotionStore { .toSorted((a, b) => a.id - b.id); } + /** Not-yet-applied actions, including staged ones (read overlays must reflect both). */ pendingActions(): StoredActionRecord[] { - return this.allActions().filter(r => r.state === "pending"); + return this.allActions().filter(r => r.state === "pending" || r.state === "staged"); } /** @@ -820,7 +849,7 @@ export async function applyNotionAction(store: NotionStore, record: StoredAction export async function revertNotionAction( store: NotionStore, record: StoredActionRecord, -): Promise { +): Promise { const api = store.api; const action = record.action; @@ -1004,7 +1033,7 @@ export function buildCreateBody( } /** - * Record a pending action and submit it to the approval queue for later approval. If the submit + * Record a staged action and submit it to the approval queue for later approval. If the submit * fails, the stored record is rolled back so it doesn't pollute simulation. Returns the action ID. */ export async function stageAction( @@ -1013,13 +1042,20 @@ export async function stageAction( action: NotionAction, ): Promise { const id = store.nextActionId(); - store.putAction({ id, action, state: "pending", submittedAt: Date.now() }); + store.putAction({ id, action, state: "staged", submittedAt: Date.now() }); try { await approvalQueue.submitAction(id, describeAction(action)); } catch (err) { store.deleteAction(id); throw err; } + // Only now may the action be applied: the overseer has accepted it, so a decision frontier can + // legitimately cover it. A concurrent veto cascade may have deleted the record meanwhile. + const record = store.getAction(id); + if (record?.state === "staged") { + record.state = "pending"; + store.putAction(record); + } return id; } @@ -1032,57 +1068,95 @@ export async function applyStoredAction(store: NotionStore, id: number): Promise await applyNotionAction(store, record); } -export function rejectStoredAction(store: NotionStore, id: number): void | { restart?: boolean } { - const record = store.getAction(id); - if (!record) return; - store.deleteAction(id); - - // Rejecting a page creation invalidates every pending action that targeted that provisional page - // (they could never be applied — the page won't exist), including sub-pages created under it and - // their edits, transitively. Cascade-delete them all so the overseer never tries to apply an - // orphan, and request a restart since the Gadget already observed simulated state built on them. - if (record.action.type === "createPage") { - // Snapshot the pending set once — deleting actions below would otherwise change it under us. - const pending = store.pendingActions(); - const purge = new Set([record.action.provisionalId]); - // Expand to transitively-nested sub-page creations. Only `createSubPage` nests a creation under - // a provisional page (parent.kind === "page"); database/workspace creates never have a - // provisional parent, so they don't need handling here. - for (;;) { - let added = false; - for (const r of pending) { - if (r.action.type === "createPage" && r.action.parent.kind === "page" && - purge.has(r.action.parent.pageId) && !purge.has(r.action.provisionalId)) { - purge.add(r.action.provisionalId); - added = true; - } +/** + * Rejecting a page creation invalidates every pending action that targeted that provisional page + * (they could never be applied — the page won't exist), including sub-pages created under it and + * their edits, transitively. Cascade-delete them all, recording which veto invalidated each so a + * repeated request can re-report the attribution. + */ +function cascadeRejectedCreation(store: NotionStore, record: StoredActionRecord): void { + if (record.action.type !== "createPage") return; + + // Snapshot the pending set once — deleting actions below would otherwise change it under us. + const pending = store.pendingActions(); + const purge = new Set([record.action.provisionalId]); + // Expand to transitively-nested sub-page creations. Only `createSubPage` nests a creation under + // a provisional page (parent.kind === "page"); database/workspace creates never have a + // provisional parent, so they don't need handling here. + for (;;) { + let added = false; + for (const candidate of pending) { + if (candidate.action.type === "createPage" && candidate.action.parent.kind === "page" && + purge.has(candidate.action.parent.pageId) && !purge.has(candidate.action.provisionalId)) { + purge.add(candidate.action.provisionalId); + added = true; } - if (!added) break; } - let deleted = false; - for (const r of pending) { - const t = actionPageId(r.action); - if (t !== null && purge.has(t)) { - store.deleteAction(r.id); - deleted = true; - } + if (!added) break; + } + + for (const candidate of pending) { + const target = actionPageId(candidate.action); + if (target !== null && purge.has(target)) { + store.deleteAction(candidate.id); + store.invalidations.record(candidate.id, record.id); } - if (deleted) return { restart: true }; - return; } +} - // Rejecting a mid-stack edit leaves the simulated overlay the Gadget already observed - // inconsistent; ask for a restart if other pending actions still target the same page. - const target = actionPageId(record.action); - if (target && store.pendingForPage(target).length > 0) { - return { restart: true }; +/** Delete vetoed records and cascade to actions they invalidate. Settled records are left alone. */ +function rejectRecords(store: NotionStore, vetoes: Set): void { + const rejected: StoredActionRecord[] = []; + for (const id of vetoes) { + const record = store.getAction(id); + if (!record || record.state === "applied" || record.state === "reverted") continue; + store.deleteAction(id); + rejected.push(record); } + for (const record of rejected) cascadeRejectedCreation(store, record); +} + +/** Resolve all stored actions through a Gatekeeper-local action ID. */ +export async function applyStoredActionsThrough( + store: NotionStore, actionId: number, vetoes: number[], +): Promise { + const vetoSet = validateApplyThroughArgs(actionId, vetoes); + rejectRecords(store, vetoSet); + store.invalidations.prune(vetoSet, store.pendingActions()[0]?.id ?? Infinity); + + const invalidatedByVeto = store.invalidations.attributedTo(vetoSet); + const invalidations = invalidatedByVeto.length > 0 ? { invalidatedByVeto } : {}; + for (const record of store.pendingActions()) { + if (record.id > actionId) break; + if (record.state === "staged") continue; // submitAction() has not completed; not coverable yet + try { + await applyNotionAction(store, record); + } catch (error) { + logger.warn("failed to apply action", { + event: "action.apply.failed", + actionId: record.id, + error, + }); + return { + ...invalidations, + stopped: { + at: record.id, + reason: displayReason(error, "Notion could not apply this action"), + }, + }; + } + } + return invalidations; +} + +export function rejectStoredAction(store: NotionStore, id: number): void { + rejectRecords(store, new Set([id])); } export async function revertStoredAction( store: NotionStore, id: number, -): Promise { +): Promise { const record = store.getAction(id); if (!record) throw new Error(`Unknown action: ${id}`); return await revertNotionAction(store, record); diff --git a/packages/gatekeeper-notion/src/notion.ts b/packages/gatekeeper-notion/src/notion.ts index af56af206..ed6f32f53 100644 --- a/packages/gatekeeper-notion/src/notion.ts +++ b/packages/gatekeeper-notion/src/notion.ts @@ -14,6 +14,7 @@ // writes immediately. List simulation has documented limitations (see types.d.ts). import { DurableObject, RpcStub, RpcTarget, WorkerEntrypoint } from "cloudflare:workers"; +import { SerialTaskQueue } from "@gadgets/backend-utils/gatekeeper-action"; import { skipRpcValidation, validateRpc } from "capnweb-validate"; import { stripTrailingSlashes, @@ -48,6 +49,7 @@ import { } from "./notion-api"; import { NotionStore, + applyStoredActionsThrough, applyStoredAction, defaultPropertiesFromSchema, observation, @@ -694,6 +696,7 @@ type NotionItemGatekeeperImplProps = { @validateRpc() export class NotionItemGatekeeperImpl extends DurableObject implements Gatekeeper { + #actionResolution = new SerialTaskQueue(); #api(): NotionApi { const userObjectId = this.ctx.props.userObjectId; const account = () => @@ -778,16 +781,20 @@ export class NotionItemGatekeeperImpl extends DurableObject {} - async applyAction(action: number): Promise { - await applyStoredAction(this.#store(), action); + applyActionsThrough(actionId: number, vetoes: number[]) { + return this.#actionResolution.run(() => applyStoredActionsThrough(this.#store(), actionId, vetoes)); } - async rejectAction(action: number): Promise { - return rejectStoredAction(this.#store(), action); + applyAction(action: number): Promise { + return this.#actionResolution.run(() => applyStoredAction(this.#store(), action)); } - async revertAction(action: number) { - return await revertStoredAction(this.#store(), action); + rejectAction(action: number): Promise { + return this.#actionResolution.run(() => rejectStoredAction(this.#store(), action)); + } + + revertAction(action: number) { + return this.#actionResolution.run(() => revertStoredAction(this.#store(), action)); } } @@ -799,6 +806,7 @@ type NotionWorkspaceGatekeeperImplProps = { export class NotionWorkspaceGatekeeperImpl extends DurableObject implements Gatekeeper { + #actionResolution = new SerialTaskQueue(); #api(): NotionApi { const userObjectId = this.ctx.props.userObjectId; const account = () => @@ -940,16 +948,20 @@ export class NotionWorkspaceGatekeeperImpl this.ctx.storage.kv.delete(this.#observerKey(id)); } - async applyAction(action: number): Promise { - await applyStoredAction(this.#store(), action); + applyActionsThrough(actionId: number, vetoes: number[]) { + return this.#actionResolution.run(() => applyStoredActionsThrough(this.#store(), actionId, vetoes)); + } + + applyAction(action: number): Promise { + return this.#actionResolution.run(() => applyStoredAction(this.#store(), action)); } - async rejectAction(action: number): Promise { - return rejectStoredAction(this.#store(), action); + rejectAction(action: number): Promise { + return this.#actionResolution.run(() => rejectStoredAction(this.#store(), action)); } - async revertAction(action: number) { - return await revertStoredAction(this.#store(), action); + revertAction(action: number) { + return this.#actionResolution.run(() => revertStoredAction(this.#store(), action)); } } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index f572acfe0..3528afa63 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -117,6 +117,9 @@ importers: packages/gatekeeper-confluence: dependencies: + '@gadgets/backend-utils': + specifier: workspace:* + version: link:../backend-utils '@gadgets/configurator-ui': specifier: workspace:* version: link:../configurator-ui @@ -313,6 +316,9 @@ importers: specifier: 'catalog:' version: 0.2.4(capnweb@0.11.1)(esbuild@0.28.1)(rollup@4.62.3)(vite@7.3.6(@types/node@26.1.0)(jiti@2.7.0)(lightningcss@1.33.0)(terser@5.49.2)(yaml@2.9.0)) devDependencies: + '@cloudflare/vitest-pool-workers': + specifier: 'catalog:' + version: 0.20.3(@cloudflare/workers-types@5.20260808.1)(@vitest/runner@4.1.10)(@vitest/snapshot@4.1.10)(vitest@4.1.10) typescript: specifier: 'catalog:' version: 7.0.2 @@ -465,6 +471,9 @@ importers: packages/gatekeeper-notion: dependencies: + '@gadgets/backend-utils': + specifier: workspace:* + version: link:../backend-utils '@gadgets/configurator-ui': specifier: workspace:* version: link:../configurator-ui @@ -1148,7 +1157,7 @@ packages: resolution: {integrity: sha512-ehg3jIkYKulZh+8om/O25vkvSsXXwC+skXmyA87FFx6A/45eqOkZsBltMw/TVteb0mloiGT8oGRTcjRAz66zaA==} '@cloudflare/kumo@2.9.2': - resolution: {integrity: sha512-c3RZBmx0TqxTKAPT4PWyTgVwPcDVW+KrFmf4mKCnwWBe6OIc0vWn+wMhnaARarJz/2kvsx87tMGmNRBsCn7pUA==} + resolution: {integrity: sha512-c3RZBmx0TqxTKAPT4PWyTgVwPcDVW+KrFmf4mKCnwWBe6OIc0vWn+wMhnaARarJz/2kvsx87tMGmNRBsCn7pUA==, tarball: https://registry.npmjs.org/@cloudflare/kumo/-/kumo-2.9.2.tgz} hasBin: true peerDependencies: '@phosphor-icons/react': ^2.1.10 @@ -1163,15 +1172,15 @@ packages: optional: true '@cloudflare/kv-asset-handler@0.5.0': - resolution: {integrity: sha512-jxQYkj8dSIzc0cD6cMMNdOc1UVjqSqu8BZdor5s8cGjW2I8BjODt/kWPVdY+u9zj3ms75Q5qaZgnxUad83+eAg==} + resolution: {integrity: sha512-jxQYkj8dSIzc0cD6cMMNdOc1UVjqSqu8BZdor5s8cGjW2I8BjODt/kWPVdY+u9zj3ms75Q5qaZgnxUad83+eAg==, tarball: https://registry.npmjs.org/@cloudflare/kv-asset-handler/-/kv-asset-handler-0.5.0.tgz} engines: {node: '>=22.0.0'} '@cloudflare/puppeteer@1.3.0': - resolution: {integrity: sha512-NBrJEUnqe082nopLh0eqnTXK4DjwsTsZGzoAcs71NFnBgzWU6Yb/ibUJHveCHV4AyAkM+mE/DChFev5gwaKZEg==} + resolution: {integrity: sha512-NBrJEUnqe082nopLh0eqnTXK4DjwsTsZGzoAcs71NFnBgzWU6Yb/ibUJHveCHV4AyAkM+mE/DChFev5gwaKZEg==, tarball: https://registry.npmjs.org/@cloudflare/puppeteer/-/puppeteer-1.3.0.tgz} engines: {node: '>=18'} '@cloudflare/unenv-preset@2.16.1': - resolution: {integrity: sha512-ECxObrMfyTl5bhQf/lZCXwo5G6xX9IAUo+nDMKK4SZ8m4Jvvxp52vilxyySSWh2YTZz8+HQ07qGH/2rEom1vDw==} + resolution: {integrity: sha512-ECxObrMfyTl5bhQf/lZCXwo5G6xX9IAUo+nDMKK4SZ8m4Jvvxp52vilxyySSWh2YTZz8+HQ07qGH/2rEom1vDw==, tarball: https://registry.npmjs.org/@cloudflare/unenv-preset/-/unenv-preset-2.16.1.tgz} peerDependencies: unenv: 2.0.0-rc.24 workerd: '>1.20260305.0 <2.0.0-0' @@ -1180,44 +1189,44 @@ packages: optional: true '@cloudflare/vitest-pool-workers@0.20.3': - resolution: {integrity: sha512-aCMvM5zQ3MTz8SorSZB6ZxjVK2Gof6UhIc2Z1z9zbX/obucSYwDUkx8A0MUOod4I7clfB0QLarKBjRdIczK3vg==} + resolution: {integrity: sha512-aCMvM5zQ3MTz8SorSZB6ZxjVK2Gof6UhIc2Z1z9zbX/obucSYwDUkx8A0MUOod4I7clfB0QLarKBjRdIczK3vg==, tarball: https://registry.npmjs.org/@cloudflare/vitest-pool-workers/-/vitest-pool-workers-0.20.3.tgz} peerDependencies: '@vitest/runner': ^4.1.0 '@vitest/snapshot': ^4.1.0 vitest: ^4.1.0 '@cloudflare/workerd-darwin-64@1.20260801.1': - resolution: {integrity: sha512-wuJWbXpKvncJi1P0GKS+iYpN5tHdb7JPJJ/+6ZQe8zzovHHVMkLJPNBsgWpqeUhpM3g9qTwEKd2rglNKejuh5A==} + resolution: {integrity: sha512-wuJWbXpKvncJi1P0GKS+iYpN5tHdb7JPJJ/+6ZQe8zzovHHVMkLJPNBsgWpqeUhpM3g9qTwEKd2rglNKejuh5A==, tarball: https://registry.npmjs.org/@cloudflare/workerd-darwin-64/-/workerd-darwin-64-1.20260801.1.tgz} engines: {node: '>=16'} cpu: [x64] os: [darwin] '@cloudflare/workerd-darwin-arm64@1.20260801.1': - resolution: {integrity: sha512-kwoZiTpnhNrF3+APx84Q/oAqvJ3sU9yefGagwm/ASaH/2W19x0vghkW/r4qCoHCK0WW7EPugZ+aXjgPMRtlq1Q==} + resolution: {integrity: sha512-kwoZiTpnhNrF3+APx84Q/oAqvJ3sU9yefGagwm/ASaH/2W19x0vghkW/r4qCoHCK0WW7EPugZ+aXjgPMRtlq1Q==, tarball: https://registry.npmjs.org/@cloudflare/workerd-darwin-arm64/-/workerd-darwin-arm64-1.20260801.1.tgz} engines: {node: '>=16'} cpu: [arm64] os: [darwin] '@cloudflare/workerd-linux-64@1.20260801.1': - resolution: {integrity: sha512-r0vAxCZH+Jih9Unm1yoyiByPNWNgawcKciOHDm5Q37ZVGOkKLsT9AtLe3yLSaul76WrKqtf+JP2n0WW32VBLJg==} + resolution: {integrity: sha512-r0vAxCZH+Jih9Unm1yoyiByPNWNgawcKciOHDm5Q37ZVGOkKLsT9AtLe3yLSaul76WrKqtf+JP2n0WW32VBLJg==, tarball: https://registry.npmjs.org/@cloudflare/workerd-linux-64/-/workerd-linux-64-1.20260801.1.tgz} engines: {node: '>=16'} cpu: [x64] os: [linux] '@cloudflare/workerd-linux-arm64@1.20260801.1': - resolution: {integrity: sha512-zWgpdZtSozvIgzQNmQiDSF8yEOQJUkRAWNsDXXzAAoy+fCn8YUoSibj3mpFSbZRvbUldeBEaW6SCdC2VEMkhNQ==} + resolution: {integrity: sha512-zWgpdZtSozvIgzQNmQiDSF8yEOQJUkRAWNsDXXzAAoy+fCn8YUoSibj3mpFSbZRvbUldeBEaW6SCdC2VEMkhNQ==, tarball: https://registry.npmjs.org/@cloudflare/workerd-linux-arm64/-/workerd-linux-arm64-1.20260801.1.tgz} engines: {node: '>=16'} cpu: [arm64] os: [linux] '@cloudflare/workerd-windows-64@1.20260801.1': - resolution: {integrity: sha512-2oQz+Ksu4ji6e/+ZoYX+tWQEcxAii2p7l+iR8kx48W1llMalaufAsmVxTlk+3/vrM7D3/2c0iK448e0UQTcIMg==} + resolution: {integrity: sha512-2oQz+Ksu4ji6e/+ZoYX+tWQEcxAii2p7l+iR8kx48W1llMalaufAsmVxTlk+3/vrM7D3/2c0iK448e0UQTcIMg==, tarball: https://registry.npmjs.org/@cloudflare/workerd-windows-64/-/workerd-windows-64-1.20260801.1.tgz} engines: {node: '>=16'} cpu: [x64] os: [win32] '@cloudflare/workers-types@5.20260808.1': - resolution: {integrity: sha512-DN7G9SMyeOq031YhQexoExFAK78ms74cFiFF1teDlTK4+LjHgIc5Z8VbvXQtcCIP//o38btxzxW0b9MGPA83CA==} + resolution: {integrity: sha512-DN7G9SMyeOq031YhQexoExFAK78ms74cFiFF1teDlTK4+LjHgIc5Z8VbvXQtcCIP//o38btxzxW0b9MGPA83CA==, tarball: https://registry.npmjs.org/@cloudflare/workers-types/-/workers-types-5.20260808.1.tgz} '@codemirror/autocomplete@6.20.3': resolution: {integrity: sha512-tlosUqb+3BbxCxZdu4tKeRghPFC+QM7q4X5YhKV2eCmPG+1r2F3f4AaSz5sCrFqUtX4Jh20VFTKecl16MgiV9g==}