diff --git a/.changeset/policy-transactional-visibility.md b/.changeset/policy-transactional-visibility.md new file mode 100644 index 0000000000..d238d09683 --- /dev/null +++ b/.changeset/policy-transactional-visibility.md @@ -0,0 +1,5 @@ +--- +"@executor-js/sdk": patch +--- + +Wrap tool-policy create and update in a transaction so concurrent edits can no longer read the same snapshot and commit duplicate positions or overwrite each other. diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index aba6b674c2..51c58b5cd5 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -5396,30 +5396,35 @@ export const createExecutor = ownedKeys(input.owner), catch: (cause) => storageFailureFromUnknown("invalid owner", cause), }); - const existing = yield* core.findMany("tool_policy", { - where: byOwner(input.owner), - }); - // Default placement is specificity-aware (below any more-specific - // rule), not top-of-list: a client that omits position — the UI when - // its policy list is stale, the API, an agent tool — must not have its - // broad rule silently shadow an existing narrow one. - const position = input.position ?? positionForNewPattern(input.pattern, existing); - const id = PolicyId.make( - `pol_${Math.random().toString(36).slice(2)}${Date.now().toString(36)}`, + // Scan → position → insert runs atomically so concurrent creates cannot commit duplicate positions. + return yield* transaction( + Effect.gen(function* () { + const existing = yield* core.findMany("tool_policy", { + where: byOwner(input.owner), + }); + // Default placement is specificity-aware (below any more-specific + // rule), not top-of-list: a client that omits position — the UI + // when its policy list is stale, the API, an agent tool — must + // not have its broad rule silently shadow an existing narrow one. + const position = input.position ?? positionForNewPattern(input.pattern, existing); + const id = PolicyId.make( + `pol_${Math.random().toString(36).slice(2)}${Date.now().toString(36)}`, + ); + const now = new Date(); + const created = yield* core.create("tool_policy", { + tenant: keys.tenant, + owner: keys.owner, + subject: keys.subject, + id: String(id), + pattern: input.pattern, + action: input.action, + position, + created_at: now, + updated_at: now, + }); + return rowToToolPolicy(created); + }), ); - const now = new Date(); - const created = yield* core.create("tool_policy", { - tenant: keys.tenant, - owner: keys.owner, - subject: keys.subject, - id: String(id), - pattern: input.pattern, - action: input.action, - position, - created_at: now, - updated_at: now, - }); - return rowToToolPolicy(created); }); const policiesUpdate = ( @@ -5433,20 +5438,25 @@ export const createExecutor = b.and(byOwner(input.owner)(b), b("id", "=", input.id)); - const existing = yield* core.findFirst("tool_policy", { where }); - if (!existing) { - return yield* new StorageError({ - message: `Tool policy not found: ${input.id}`, - cause: undefined, - }); - } - const set: Record = { updated_at: new Date() }; - if (input.pattern !== undefined) set.pattern = input.pattern; - if (input.action !== undefined) set.action = input.action; - if (input.position !== undefined) set.position = input.position; - yield* core.updateMany("tool_policy", { where, set }); - const updated = yield* core.findFirst("tool_policy", { where }); - return rowToToolPolicy(updated ?? ({ ...existing, ...set } as ToolPolicyRow)); + // Existence check, write, and re-read commit together. + return yield* transaction( + Effect.gen(function* () { + const existing = yield* core.findFirst("tool_policy", { where }); + if (!existing) { + return yield* new StorageError({ + message: `Tool policy not found: ${input.id}`, + cause: undefined, + }); + } + const set: Record = { updated_at: new Date() }; + if (input.pattern !== undefined) set.pattern = input.pattern; + if (input.action !== undefined) set.action = input.action; + if (input.position !== undefined) set.position = input.position; + yield* core.updateMany("tool_policy", { where, set }); + const updated = yield* core.findFirst("tool_policy", { where }); + return rowToToolPolicy(updated ?? ({ ...existing, ...set } as ToolPolicyRow)); + }), + ); }); const policiesRemove = (input: RemoveToolPolicyInput): Effect.Effect => diff --git a/packages/core/sdk/src/policies.test.ts b/packages/core/sdk/src/policies.test.ts index beb9703c49..c05e634842 100644 --- a/packages/core/sdk/src/policies.test.ts +++ b/packages/core/sdk/src/policies.test.ts @@ -428,6 +428,23 @@ describe("executor.policies", () => { }), ); + it.live("concurrent creates of equally specific rules get distinct positions", () => + Effect.gen(function* () { + const executor = yield* setupExecutor(); + yield* Effect.all( + [ + executor.policies.create({ owner: "org", pattern: "vercel.dns.create", action: "block" }), + executor.policies.create({ owner: "org", pattern: "vercel.dns.delete", action: "block" }), + ], + { concurrency: "unbounded" }, + ); + + const rules = yield* executor.policies.list(); + expect(rules).toHaveLength(2); + expect(new Set(rules.map((r) => r.position)).size).toBe(2); + }), + ); + it.effect("create stores rules at the requested owner", () => Effect.gen(function* () { const executor = yield* setupExecutor();