From e3143ba8e99d65c587094bbaead03dedd2827769 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Fri, 10 Jul 2026 01:10:24 -0700 Subject: [PATCH 1/2] fix(policies): specificity-aware default position on the server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The UI computed a specificity-aware position for new rules client-side, but the server defaulted a positionless create to the TOP of the owner's list. When the client's policy list was stale (e.g. the optimistic atom hadn't refreshed between two quick writes), the UI sent no usable position and the top-of-list default let a broad category rule shadow a narrower leaf rule written moments earlier — surfaced by the policies-ui scenario failing against the production selfhost image. Move the specificity placement into the sdk (positionForNewPattern) and make it the server-side default for create; the React hook now reuses the same helper so the optimistic order matches what the server commits. --- .changeset/policy-position-server-default.md | 5 +++ packages/core/sdk/src/executor.ts | 13 +++--- packages/core/sdk/src/policies.test.ts | 43 ++++++++++++++++++- packages/core/sdk/src/policies.ts | 41 ++++++++++++++++++ packages/core/sdk/src/shared.ts | 2 + .../react/src/hooks/use-policy-actions.ts | 41 +++++------------- 6 files changed, 107 insertions(+), 38 deletions(-) create mode 100644 .changeset/policy-position-server-default.md 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/hooks/use-policy-actions.ts b/packages/react/src/hooks/use-policy-actions.ts index 0ab3375f4..2b6384b0c 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 { 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,24 +17,6 @@ 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 @@ -83,20 +69,15 @@ export const usePolicyActions = (owner: Owner = "org"): PolicyAction => { const busy = policies.waiting; + // 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], ); From f953c5b18a82b1adeacec924555e0ccbf42ba4a9 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Fri, 10 Jul 2026 01:34:05 -0700 Subject: [PATCH 2/2] fix(policies): badge Clear works while the policies list is mid-refresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The detail badge's Clear resolved the rule to remove by pattern-matching the local policies list. Right after the badge authors a rule, the post-commit refetch may not have landed, so the lookup missed and the clear silently no-oped — the second race the policies-ui scenario hit on the production image. Thread the resolved EffectivePolicy's policyId through as a fallback for exactly that window. --- packages/react/src/components/tool-detail.tsx | 9 +++-- .../react/src/hooks/use-policy-actions.ts | 35 +++++++++++++++---- .../react/src/pages/integration-detail.tsx | 4 ++- packages/react/src/pages/tools.tsx | 4 ++- 4 files changed, 40 insertions(+), 12 deletions(-) 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 2b6384b0c..c7d594698 100644 --- a/packages/react/src/hooks/use-policy-actions.ts +++ b/packages/react/src/hooks/use-policy-actions.ts @@ -1,4 +1,4 @@ -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 { @@ -22,8 +22,12 @@ export interface PolicyAction { * 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; } @@ -69,6 +73,14 @@ 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; @@ -102,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} />