diff --git a/.changeset/policy-position-server-default.md b/.changeset/policy-position-server-default.md new file mode 100644 index 000000000..f370ee6c9 --- /dev/null +++ b/.changeset/policy-position-server-default.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +Policy create now defaults a new rule's position below any more-specific existing rule on the server, so a broad rule written without an explicit position (stale UI, API, agent tool) cannot shadow an existing narrower rule. diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 5f470a70e..9309fd8c3 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -5,8 +5,6 @@ import { memoryAdapter } from "@executor-js/fumadb/adapters/memory"; import { withQueryContext, type Condition, type ConditionBuilder } from "@executor-js/fumadb/query"; import { schema as fumaSchema, type RelationsMap } from "@executor-js/fumadb/schema"; import type { AnyColumn } from "@executor-js/fumadb/schema"; -import { generateKeyBetween } from "fractional-indexing"; - import { StorageError, isStorageFailure, @@ -99,6 +97,7 @@ import { comparePolicyRow, isValidPattern, matchPattern, + positionForNewPattern, resolveEffectivePolicy, rowToToolPolicy, type CreateToolPolicyInput, @@ -3407,11 +3406,11 @@ export const createExecutor = row.position) - .sort() - .at(0); - const position = input.position ?? generateKeyBetween(null, minPosition ?? null); + // 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)}`, ); diff --git a/packages/core/sdk/src/policies.test.ts b/packages/core/sdk/src/policies.test.ts index 3571de63a..beb9703c4 100644 --- a/packages/core/sdk/src/policies.test.ts +++ b/packages/core/sdk/src/policies.test.ts @@ -367,7 +367,7 @@ describe("executor.policies", () => { }), ); - it.effect("create defaults new rules to the top of the list", () => + it.effect("create defaults new rules above equally-or-less-specific ones", () => Effect.gen(function* () { const executor = yield* setupExecutor(); const first = yield* executor.policies.create({ @@ -387,6 +387,47 @@ describe("executor.policies", () => { }), ); + it.effect("create places a broad rule below an existing narrower one", () => + Effect.gen(function* () { + const executor = yield* setupExecutor(); + // Narrow leaf rule first, then a broad category rule WITHOUT an explicit + // position — the category rule must not shadow the leaf rule. + yield* executor.policies.create({ + owner: "org", + pattern: "vercel.*.*.records.create", + action: "block", + }); + yield* executor.policies.create({ + owner: "org", + pattern: "vercel.*.*.records.*", + action: "require_approval", + }); + + const rules = yield* executor.policies.list(); + expect(rules.map((r) => r.pattern)).toEqual([ + "vercel.*.*.records.create", + "vercel.*.*.records.*", + ]); + + const rows = rules.map( + (r) => + ({ + id: String(r.id), + owner: r.owner, + subject: "", + pattern: r.pattern, + action: r.action, + position: r.position, + created_at: r.createdAt, + updated_at: r.updatedAt, + }) as ToolPolicyRow, + ); + const match = resolveToolPolicy("vercel.org.acct.records.create", rows, () => 0); + expect(match?.action).toBe("block"); + expect(match?.pattern).toBe("vercel.*.*.records.create"); + }), + ); + it.effect("create stores rules at the requested owner", () => Effect.gen(function* () { const executor = yield* setupExecutor(); diff --git a/packages/core/sdk/src/policies.ts b/packages/core/sdk/src/policies.ts index ff60de512..8620d9c6d 100644 --- a/packages/core/sdk/src/policies.ts +++ b/packages/core/sdk/src/policies.ts @@ -10,6 +10,7 @@ // --------------------------------------------------------------------------- import { Match, Schema } from "effect"; +import { generateKeyBetween } from "fractional-indexing"; import type { ToolPolicyAction, ToolPolicyRow } from "./core-schema"; import { Owner, PolicyId } from "./ids"; @@ -139,6 +140,46 @@ export const comparePolicyRow = ( return ia < ib ? -1 : ia > ib ? 1 : 0; }; +// Specificity score for ordering. Higher = more specific = should sit at a +// lower position-key (higher precedence). New rules are auto-placed below +// any more-specific existing rules so a freshly-added group rule never +// silently shadows an existing leaf rule. +// `*` → 0 +// `vercel.*` → 2 (1 literal segment, wildcard) +// `vercel.dns.*` → 4 (2 literal segments, wildcard) +// `vercel.dns` → 5 (2 literal segments, exact — beats same-prefix wildcard) +// `vercel.dns.create` → 7 (3 literal segments, exact) +export const patternSpecificity = (pattern: string): number => { + if (pattern === "*") return 0; + if (pattern.endsWith(".*")) { + const prefix = pattern.slice(0, -2); + return prefix.split(".").length * 2; + } + return pattern.split(".").length * 2 + 1; +}; + +/** + * Position key for a new rule among an owner's existing rules, placed just + * below every existing rule that is MORE specific (and above everything + * equally or less specific). Rows must be the owner's committed rules; order + * doesn't matter, they're sorted here. This is the authoritative default — + * the server applies it when `create` gets no explicit position, so a rule + * written by any client (UI, API, agent tool) cannot shadow a more-specific + * existing rule by racing to the top of the list. + */ +export const positionForNewPattern = ( + pattern: string, + rows: ReadonlyArray>, +): string => { + const committed = [...rows].sort(comparePolicyRow); + const newScore = patternSpecificity(pattern); + let idx = committed.findIndex((r) => patternSpecificity(r.pattern) <= newScore); + if (idx === -1) idx = committed.length; // below every more-specific rule + const prev = idx === 0 ? null : committed[idx - 1]!.position; + const next = idx === committed.length ? null : committed[idx]!.position; + return generateKeyBetween(prev, next); +}; + const actionRestrictionRank = (action: ToolPolicyAction): number => Match.value(action).pipe( Match.when("block", () => 3), diff --git a/packages/core/sdk/src/shared.ts b/packages/core/sdk/src/shared.ts index 8f69d25a8..e78b56577 100644 --- a/packages/core/sdk/src/shared.ts +++ b/packages/core/sdk/src/shared.ts @@ -88,6 +88,8 @@ export { isValidPattern, effectivePolicyFromSorted, comparePolicyRow, + patternSpecificity, + positionForNewPattern, ToolPolicyActionSchema, type ToolPolicy, type CreateToolPolicyInput, diff --git a/packages/react/src/components/tool-detail.tsx b/packages/react/src/components/tool-detail.tsx index 9daebdcbf..cbd5fd656 100644 --- a/packages/react/src/components/tool-detail.tsx +++ b/packages/react/src/components/tool-detail.tsx @@ -171,7 +171,7 @@ export function ToolDetail(props: { /** When provided, the policy badge becomes a dropdown trigger that * applies a user rule to this tool's exact id. */ onSetPolicy?: (pattern: string, action: ToolPolicyAction) => void; - onClearPolicy?: (pattern: string) => void; + onClearPolicy?: (pattern: string, policyId?: string) => void; patternForDisplay?: (displayPattern: string) => string; /** Run-tab wiring. When `integration` + `runToolName` are provided, a third * "Run" tab hosts the per-connection tool tester. */ @@ -413,7 +413,7 @@ function PolicyBadgeMenu(props: { staticTool?: boolean; policy?: EffectivePolicy; onSetPolicy?: (pattern: string, action: ToolPolicyAction) => void; - onClearPolicy?: (pattern: string) => void; + onClearPolicy?: (pattern: string, policyId?: string) => void; patternForDisplay?: (displayPattern: string) => string; }) { const interactive = !!props.onSetPolicy; @@ -487,7 +487,10 @@ function PolicyBadgeMenu(props: { <> props.onClearPolicy?.(pattern)} + // Pass the resolved rule's id: right after this menu authored + // the rule, the policies list may still be mid-refresh, and a + // pattern-only lookup there would silently no-op the clear. + onSelect={() => props.onClearPolicy?.(pattern, props.policy?.policyId)} className="text-muted-foreground" > Clear diff --git a/packages/react/src/hooks/use-policy-actions.ts b/packages/react/src/hooks/use-policy-actions.ts index 0ab3375f4..c7d594698 100644 --- a/packages/react/src/hooks/use-policy-actions.ts +++ b/packages/react/src/hooks/use-policy-actions.ts @@ -1,8 +1,12 @@ -import { useCallback, useMemo } from "react"; +import { useCallback, useMemo, useRef } from "react"; import { useAtomSet, useAtomValue } from "@effect/atom-react"; import * as AsyncResult from "effect/unstable/reactivity/AsyncResult"; -import { generateKeyBetween } from "fractional-indexing"; -import { PolicyId, type Owner, type ToolPolicyAction } from "@executor-js/sdk/shared"; +import { + PolicyId, + positionForNewPattern, + type Owner, + type ToolPolicyAction, +} from "@executor-js/sdk/shared"; import { createPolicyOptimistic, @@ -13,31 +17,17 @@ import { import { policyWriteKeys } from "../api/reactivity-keys"; import { trackEvent } from "../api/analytics"; -// Specificity score for ordering. Higher = more specific = should sit at a -// lower position-key (higher precedence). New rules are auto-placed below -// any more-specific existing rules so a freshly-added group rule never -// silently shadows an existing leaf rule. -// `*` → 0 -// `vercel.*` → 2 (1 literal segment, wildcard) -// `vercel.dns.*` → 4 (2 literal segments, wildcard) -// `vercel.dns` → 5 (2 literal segments, exact — beats same-prefix wildcard) -// `vercel.dns.create` → 7 (3 literal segments, exact) -const specificity = (pattern: string): number => { - if (pattern === "*") return 0; - if (pattern.endsWith(".*")) { - const prefix = pattern.slice(0, -2); - return prefix.split(".").length * 2; - } - return pattern.split(".").length * 2 + 1; -}; - export interface PolicyAction { /** Set the action on a pattern. If a user rule with this exact pattern * already exists, update it. Otherwise create with auto-placed * position so more-specific rules keep precedence. */ readonly set: (pattern: string, action: ToolPolicyAction) => Promise; - /** Remove the user rule with this exact pattern, if any. No-op if none. */ - readonly clear: (pattern: string) => Promise; + /** Remove the user rule with this exact pattern, if any. `policyId` is the + * id of the rule the caller believes is active (e.g. from the tool's + * resolved EffectivePolicy); it is the fallback when the local policies + * list is mid-refresh and doesn't contain the rule yet — without it a + * clear in that window silently no-ops. */ + readonly clear: (pattern: string, policyId?: string) => Promise; /** True while a write is in flight. */ readonly busy: boolean; } @@ -83,20 +73,23 @@ export const usePolicyActions = (owner: Owner = "org"): PolicyAction => { const busy = policies.waiting; + // Server-assigned ids of rules this hook created, by pattern. The local + // policies list is optimistic: right after a create it holds a placeholder + // row with a fake `pending-*` id (and the resolved EffectivePolicy a caller + // hands back can carry that same fake id), so neither is safe to DELETE + // with. The create response is the one authoritative id source in that + // window. + const createdIdByPattern = useRef(new Map()); + + // Specificity-aware placement (below any more-specific rule) via the shared + // sdk helper — the same computation the server applies when no position is + // sent. Computing it here too keeps the optimistic UI's final order stable; + // if this client's list is stale, the server default is the backstop. const computePosition = useCallback( (newPattern: string): string | undefined => { const committed = sorted.filter((r) => r.position !== ""); if (committed.length === 0) return undefined; - const newScore = specificity(newPattern); - // Walk down the list (most-precedent first); place the new rule - // just before the first existing rule whose specificity is <= the - // new one. That way more-specific rules stay above us, and we win - // against everything equally or less specific. - let idx = committed.findIndex((r) => specificity(r.pattern) <= newScore); - if (idx === -1) idx = committed.length; // append at bottom - const prev = idx === 0 ? null : committed[idx - 1]!.position; - const next = idx === committed.length ? null : committed[idx]!.position; - return generateKeyBetween(prev, next); + return positionForNewPattern(newPattern, committed); }, [sorted], ); @@ -121,24 +114,33 @@ export const usePolicyActions = (owner: Owner = "org"): PolicyAction => { return; } const position = computePosition(pattern); - await doCreate({ + const created = await doCreate({ payload: position === undefined ? { owner, pattern, action } : { owner, pattern, action, position }, reactivityKeys: policyWriteKeys, }); + createdIdByPattern.current.set(pattern, String(created.id)); trackEvent("tool_policy_set", { action, pattern_kind: patternKind, owner }); }, [owner, doCreate, doUpdate, findExact, computePosition], ); const clear = useCallback( - async (pattern: string) => { + async (pattern: string, policyId?: string) => { + // Resolve the id to delete, most-authoritative first. The local list + // and the caller's resolved policy can both hold an optimistic + // `pending-*` placeholder id right after a create (before the + // post-commit refetch lands); the create response we recorded is real, + // and a placeholder id must never reach the server. const existing = findExact(pattern); - if (!existing) return; + const isReal = (id: string | undefined): id is string => !!id && !id.startsWith("pending-"); + const id = [existing?.id, createdIdByPattern.current.get(pattern), policyId].find(isReal); + if (!id) return; + createdIdByPattern.current.delete(pattern); await doRemove({ - params: { policyId: PolicyId.make(existing.id) }, + params: { policyId: PolicyId.make(id) }, payload: { owner }, reactivityKeys: policyWriteKeys, }); diff --git a/packages/react/src/pages/integration-detail.tsx b/packages/react/src/pages/integration-detail.tsx index 9fbfa5c73..c575a2c76 100644 --- a/packages/react/src/pages/integration-detail.tsx +++ b/packages/react/src/pages/integration-detail.tsx @@ -526,7 +526,9 @@ export function IntegrationDetailPage(props: { staticTool={selection?.static} policy={selectedTool.policy} onSetPolicy={(pattern, action) => void policyActions.set(pattern, action)} - onClearPolicy={(pattern) => void policyActions.clear(pattern)} + onClearPolicy={(pattern, policyId) => + void policyActions.clear(pattern, policyId) + } {...(!selection?.static && selectedBareName ? { integration: slug, diff --git a/packages/react/src/pages/tools.tsx b/packages/react/src/pages/tools.tsx index 1dc737749..60e6e0fa9 100644 --- a/packages/react/src/pages/tools.tsx +++ b/packages/react/src/pages/tools.tsx @@ -157,7 +157,9 @@ export function ToolsPage() { staticTool={selection?.static} policy={selectedTool.policy} onSetPolicy={(pattern, action) => void policyActions.set(pattern, action)} - onClearPolicy={(pattern) => void policyActions.clear(pattern)} + onClearPolicy={(pattern, policyId) => + void policyActions.clear(pattern, policyId) + } /> ) : ( 0} />