diff --git a/.changeset/tool-policy-owner-segment.md b/.changeset/tool-policy-owner-segment.md new file mode 100644 index 000000000..ca2615de9 --- /dev/null +++ b/.changeset/tool-policy-owner-segment.md @@ -0,0 +1,5 @@ +--- +"@executor-js/sdk": patch +--- + +Tool policy patterns written in the documented `integration.connection.tool` form (no owner segment) now match as intended instead of silently never firing. `policies.create`/`policies.update` backfill the missing owner segment automatically. diff --git a/packages/core/sdk/src/core-tools.ts b/packages/core/sdk/src/core-tools.ts index 461e04d8a..290711bfc 100644 --- a/packages/core/sdk/src/core-tools.ts +++ b/packages/core/sdk/src/core-tools.ts @@ -998,7 +998,7 @@ export const coreToolsPlugin = definePlugin((options: CoreToolsPluginOptions = { tool({ name: "policies.create", description: - "Create a tool policy. `pattern` matches a tool address tail (`integration.connection.tool`, `integration.*`, `*`); `action` is approve/require_approval/block. `owner` is org (workspace guardrail) or user (personal).", + "Create a tool policy. `pattern` matches a tool address (`integration.connection.tool`, `integration.*`, `*`); the owner segment is optional and defaults to every owner when omitted. `action` is approve/require_approval/block. `owner` is org (workspace guardrail) or user (personal).", inputSchema: PolicyCreateInputStd, outputSchema: PolicyOutputStd, // A policy decides which tools run without confirmation, so creating diff --git a/packages/core/sdk/src/executor.test.ts b/packages/core/sdk/src/executor.test.ts index fb2835a52..33c606ae5 100644 --- a/packages/core/sdk/src/executor.test.ts +++ b/packages/core/sdk/src/executor.test.ts @@ -1416,6 +1416,30 @@ describe("speculative read abandonment", () => { }), ); + it.effect( + "a policy pattern written in the documented pre-owner shape still blocks the tool (#2047)", + () => + Effect.gen(function* () { + const executor = yield* makeTestExecutor({ plugins: [demoPlugin] as const }); + yield* seedRunConnection(executor); + // Public docs and the settings UI describe patterns as + // `integration.connection.tool` — one segment short of the matcher's + // `integration.owner.connection.tool`. Written as-is this pattern can + // never match; `policiesCreate` backfills the missing owner segment + // so a block set through the documented shape still takes effect. + yield* executor.policies.create({ + owner: "org", + pattern: `${INTEG}.${CONN}.run`, + action: "block", + }); + + const result = yield* Effect.result(executor.execute(addr("run"), {})); + expect(Result.isFailure(result)).toBe(true); + if (!Result.isFailure(result)) return; + expect(Predicate.isTagged("ToolBlockedError")(result.failure)).toBe(true); + }), + ); + it.effect("failing speculative reads neither mask the branch error nor unhandled-reject", () => Effect.gen(function* () { const faults = makeReadFaults(); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index fdbc9b767..2dfcd9df6 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -157,6 +157,7 @@ import { isUnboundedDynamicToolScope, isValidPattern, matchPattern, + normalizePolicyPattern, positionForNewPattern, resolveEffectivePolicy, rowToToolPolicy, @@ -6047,9 +6048,12 @@ export const createExecutor = row.owner === "user" ? 0 : 1; - // Tool policies gate by tool identity (`.`), independent of - // which connection serves it; the org/user split is handled by owner-scoped - // policy rows + ownerRank, not the match pattern. + // Dynamic (connection-backed) tools are keyed by their full four-segment + // address, `...` — the owner + // segment is load-bearing: a connection's (integration, name) is only + // unique per owner, so two different connections can share a name. + // Patterns written without an owner segment are backfilled by + // `normalizePolicyPattern` at create/update time, not here. const normalizedPolicyId = (tool: Tool): string => tool.static ? String(tool.address) @@ -6089,6 +6093,10 @@ export const createExecutor = ownedKeys(input.owner), catch: (cause) => storageFailureFromUnknown("invalid owner", cause), }); + // Patterns written in the documented (pre-owner) 3-segment shape + // are one segment short of the matcher's 4-segment dynamic tool + // id and can never match — see `normalizePolicyPattern`. + const pattern = normalizePolicyPattern(input.pattern); const existing = yield* core.findMany("tool_policy", { where: byOwner(input.owner), }); @@ -6096,7 +6104,7 @@ export const createExecutor = = { updated_at: new Date() }; - if (input.pattern !== undefined) set.pattern = input.pattern; + if (input.pattern !== undefined) set.pattern = normalizePolicyPattern(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 }); diff --git a/packages/core/sdk/src/policies.test.ts b/packages/core/sdk/src/policies.test.ts index 98af2a65e..f5d078aa0 100644 --- a/packages/core/sdk/src/policies.test.ts +++ b/packages/core/sdk/src/policies.test.ts @@ -20,6 +20,7 @@ import { effectivePolicyFromSorted, isValidPattern, matchPattern, + normalizePolicyPattern, resolveToolPolicy, } from "./policies"; import { definePlugin, tool } from "./plugin"; @@ -159,6 +160,41 @@ describe("dynamicToolScopeForPattern", () => { }); }); +describe("normalizePolicyPattern", () => { + it("backfills the owner segment on the documented pre-owner shape (#2047)", () => { + // The public docs and UI describe patterns as `integration.connection.tool`, + // one segment short of the matcher's `integration.owner.connection.tool`. + // Written as-is these can never match — see `matchPattern`'s mid-segment + // wildcard tests above. + expect(normalizePolicyPattern("vercel.dns.create")).toBe("vercel.*.dns.create"); + expect(normalizePolicyPattern("vercel.dns.*")).toBe("vercel.*.dns.*"); + }); + + it("leaves the universal pattern and already-unbounded plugin-wide patterns alone", () => { + expect(normalizePolicyPattern("*")).toBe("*"); + expect(normalizePolicyPattern("vercel.*")).toBe("vercel.*"); + }); + + it("leaves patterns that already carry an owner segment alone", () => { + expect(normalizePolicyPattern("vercel.org.dns.create")).toBe("vercel.org.dns.create"); + expect(normalizePolicyPattern("vercel.*.dns.*")).toBe("vercel.*.dns.*"); + expect(normalizePolicyPattern("github.*.*.repos.list")).toBe("github.*.*.repos.list"); + }); + + it("leaves patterns rooted at a static namespace alone — there is no owner to backfill", () => { + expect(normalizePolicyPattern("executor.desktopSettings.openSettings")).toBe( + "executor.desktopSettings.openSettings", + ); + }); + + it("produces a pattern that actually matches the dynamic tool it was written for", () => { + // Dynamic tool id: integration.owner.connection.tool + const toolId = "vercel.org.dns.create"; + expect(matchPattern("vercel.dns.create", toolId)).toBe(false); // the bug + expect(matchPattern(normalizePolicyPattern("vercel.dns.create"), toolId)).toBe(true); // fixed + }); +}); + describe("resolveToolPolicy", () => { // v2: policy rows carry `owner` (org|user) instead of a scope id. const ROW = ( diff --git a/packages/core/sdk/src/policies.ts b/packages/core/sdk/src/policies.ts index b9e1f1eca..d2eab5793 100644 --- a/packages/core/sdk/src/policies.ts +++ b/packages/core/sdk/src/policies.ts @@ -120,6 +120,32 @@ export const isValidPattern = (pattern: string): boolean => { return true; }; +// --------------------------------------------------------------------------- +// Owner-segment backfill — the public docs and UI describe patterns in the +// pre-owner 3-segment shape `integration.connection.tool`. The matcher keys +// dynamic tools by the full 4-segment `integration.owner.connection.tool` +// (see `normalizedPolicyId` in executor.ts), so a pattern written in the +// documented shape is one segment short and can never match anything +// (#2047). `policiesCreate`/`policiesUpdate` run every pattern through this +// first, backfilling the owner segment the writer never named. Left +// untouched: `*`, `integration.*` (already unbounded — no owner to insert), +// and patterns rooted at a static namespace (no owner segment at all). +// +// Duplicated from `migration-spec.ts`'s `DEFAULT_STATIC_NAMESPACES` rather +// than imported: this module is part of the public SDK surface (browser +// bundles included), and `migration-spec.ts` pulls in `node:crypto`. +// --------------------------------------------------------------------------- + +const STATIC_TOOL_NAMESPACES: readonly string[] = ["executor", "openapi"]; + +export const normalizePolicyPattern = (pattern: string): string => { + if (pattern === "*") return pattern; + const segments = pattern.split("."); + if (segments.length !== 3) return pattern; + if (STATIC_TOOL_NAMESPACES.includes(segments[0]!)) return pattern; + return `${segments[0]}.*.${segments[1]}.${segments[2]}`; +}; + // --------------------------------------------------------------------------- // Dynamic-tool scope — the (integration, owner, connection) prefix a pattern // can reach. Lets a policy source that is an allowlist (a toolkit) narrow the