History

Policies: specificity-aware default position on the server (#1395)

* fix(policies): specificity-aware default position on the server 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. * fix(policies): badge Clear works while the policies list is mid-refresh 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.

Rhys Sullivan committed 4 months ago

Commit d90d8be1 parent 1ca51119 files · +147 −50Browse files

Unified · Split

9 files
  1. .changeset/policy-position-server-default.md +5 −0
  2. packages/core/sdk/src/executor.ts +6 −7
  3. packages/core/sdk/src/policies.test.ts +42 −1
  4. packages/core/sdk/src/policies.ts +41 −0
  5. packages/core/sdk/src/shared.ts +2 −0
  6. packages/react/src/components/tool-detail.tsx +6 −3
  7. packages/react/src/hooks/use-policy-actions.ts +39 −37
  8. packages/react/src/pages/integration-detail.tsx +3 −1
  9. packages/react/src/pages/tools.tsx +3 −1
.changeset/policy-position-server-default.md+5−0
@@ -0,0 +1,5 @@
1+---
2+"executor": patch
3+---
4+
5+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.
packages/core/sdk/src/executor.ts+6−7
@@ -5,8 +5,6 @@ import { memoryAdapter } from "@executor-js/fumadb/adapters/memory"; · 4 unmodified lines
55 import { withQueryContext, type Condition, type ConditionBuilder } from "@executor-js/fumadb/query";
66 import { schema as fumaSchema, type RelationsMap } from "@executor-js/fumadb/schema";
77 import type { AnyColumn } from "@executor-js/fumadb/schema";
8−import { generateKeyBetween } from "fractional-indexing";
9−
108 import {
119 StorageError,
1210 isStorageFailure,
@@ -99,6 +97,7 @@ import { · 86 unmodified lines
9997 comparePolicyRow,
10098 isValidPattern,
10199 matchPattern,
100+ positionForNewPattern,
102101 resolveEffectivePolicy,
103102 rowToToolPolicy,
104103 type CreateToolPolicyInput,
@@ -3407,11 +3406,11 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 3302 unmodified lines
34073406 const existing = yield* core.findMany("tool_policy", {
34083407 where: byOwner(input.owner),
34093408 });
3410− const minPosition = existing
3411− .map((row) => row.position)
3412− .sort()
3413− .at(0);
3414− const position = input.position ?? generateKeyBetween(null, minPosition ?? null);
3409+ // Default placement is specificity-aware (below any more-specific
3410+ // rule), not top-of-list: a client that omits position — the UI when
3411+ // its policy list is stale, the API, an agent tool — must not have its
3412+ // broad rule silently shadow an existing narrow one.
3413+ const position = input.position ?? positionForNewPattern(input.pattern, existing);
34153414 const id = PolicyId.make(
34163415 `pol_${Math.random().toString(36).slice(2)}${Date.now().toString(36)}`,
34173416 );
packages/core/sdk/src/policies.test.ts+42−1
@@ -367,7 +367,7 @@ describe("executor.policies", () => { · 366 unmodified lines
367367 }),
368368 );
369369
370− it.effect("create defaults new rules to the top of the list", () =>
370+ it.effect("create defaults new rules above equally-or-less-specific ones", () =>
371371 Effect.gen(function* () {
372372 const executor = yield* setupExecutor();
373373 const first = yield* executor.policies.create({
@@ -387,6 +387,47 @@ describe("executor.policies", () => { · 13 unmodified lines
387387 }),
388388 );
389389
390+ it.effect("create places a broad rule below an existing narrower one", () =>
391+ Effect.gen(function* () {
392+ const executor = yield* setupExecutor();
393+ // Narrow leaf rule first, then a broad category rule WITHOUT an explicit
394+ // position — the category rule must not shadow the leaf rule.
395+ yield* executor.policies.create({
396+ owner: "org",
397+ pattern: "vercel.*.*.records.create",
398+ action: "block",
399+ });
400+ yield* executor.policies.create({
401+ owner: "org",
402+ pattern: "vercel.*.*.records.*",
403+ action: "require_approval",
404+ });
405+
406+ const rules = yield* executor.policies.list();
407+ expect(rules.map((r) => r.pattern)).toEqual([
408+ "vercel.*.*.records.create",
409+ "vercel.*.*.records.*",
410+ ]);
411+
412+ const rows = rules.map(
413+ (r) =>
414+ ({
415+ id: String(r.id),
416+ owner: r.owner,
417+ subject: "",
418+ pattern: r.pattern,
419+ action: r.action,
420+ position: r.position,
421+ created_at: r.createdAt,
422+ updated_at: r.updatedAt,
423+ }) as ToolPolicyRow,
424+ );
425+ const match = resolveToolPolicy("vercel.org.acct.records.create", rows, () => 0);
426+ expect(match?.action).toBe("block");
427+ expect(match?.pattern).toBe("vercel.*.*.records.create");
428+ }),
429+ );
430+
390431 it.effect("create stores rules at the requested owner", () =>
391432 Effect.gen(function* () {
392433 const executor = yield* setupExecutor();
packages/core/sdk/src/policies.ts+41−0
@@ -10,6 +10,7 @@ · 9 unmodified lines
1010 // ---------------------------------------------------------------------------
1111
1212 import { Match, Schema } from "effect";
13+import { generateKeyBetween } from "fractional-indexing";
1314
1415 import type { ToolPolicyAction, ToolPolicyRow } from "./core-schema";
1516 import { Owner, PolicyId } from "./ids";
@@ -139,6 +140,46 @@ export const comparePolicyRow = ( · 123 unmodified lines
139140 return ia < ib ? -1 : ia > ib ? 1 : 0;
140141 };
141142
143+// Specificity score for ordering. Higher = more specific = should sit at a
144+// lower position-key (higher precedence). New rules are auto-placed below
145+// any more-specific existing rules so a freshly-added group rule never
146+// silently shadows an existing leaf rule.
147+// `*` → 0
148+// `vercel.*` → 2 (1 literal segment, wildcard)
149+// `vercel.dns.*` → 4 (2 literal segments, wildcard)
150+// `vercel.dns` → 5 (2 literal segments, exact — beats same-prefix wildcard)
151+// `vercel.dns.create` → 7 (3 literal segments, exact)
152+export const patternSpecificity = (pattern: string): number => {
153+ if (pattern === "*") return 0;
154+ if (pattern.endsWith(".*")) {
155+ const prefix = pattern.slice(0, -2);
156+ return prefix.split(".").length * 2;
157+ }
158+ return pattern.split(".").length * 2 + 1;
159+};
160+
161+/**
162+ * Position key for a new rule among an owner's existing rules, placed just
163+ * below every existing rule that is MORE specific (and above everything
164+ * equally or less specific). Rows must be the owner's committed rules; order
165+ * doesn't matter, they're sorted here. This is the authoritative default —
166+ * the server applies it when `create` gets no explicit position, so a rule
167+ * written by any client (UI, API, agent tool) cannot shadow a more-specific
168+ * existing rule by racing to the top of the list.
169+ */
170+export const positionForNewPattern = (
171+ pattern: string,
172+ rows: ReadonlyArray<Pick<ToolPolicyRow, "pattern" | "position" | "id">>,
173+): string => {
174+ const committed = [...rows].sort(comparePolicyRow);
175+ const newScore = patternSpecificity(pattern);
176+ let idx = committed.findIndex((r) => patternSpecificity(r.pattern) <= newScore);
177+ if (idx === -1) idx = committed.length; // below every more-specific rule
178+ const prev = idx === 0 ? null : committed[idx - 1]!.position;
179+ const next = idx === committed.length ? null : committed[idx]!.position;
180+ return generateKeyBetween(prev, next);
181+};
182+
142183 const actionRestrictionRank = (action: ToolPolicyAction): number =>
143184 Match.value(action).pipe(
144185 Match.when("block", () => 3),
packages/core/sdk/src/shared.ts+2−0
@@ -88,6 +88,8 @@ export { · 87 unmodified lines
8888 isValidPattern,
8989 effectivePolicyFromSorted,
9090 comparePolicyRow,
91+ patternSpecificity,
92+ positionForNewPattern,
9193 ToolPolicyActionSchema,
9294 type ToolPolicy,
9395 type CreateToolPolicyInput,
packages/react/src/components/tool-detail.tsx+6−3
@@ -171,7 +171,7 @@ export function ToolDetail(props: { · 170 unmodified lines
171171 /** When provided, the policy badge becomes a dropdown trigger that
172172 * applies a user rule to this tool's exact id. */
173173 onSetPolicy?: (pattern: string, action: ToolPolicyAction) => void;
174− onClearPolicy?: (pattern: string) => void;
174+ onClearPolicy?: (pattern: string, policyId?: string) => void;
175175 patternForDisplay?: (displayPattern: string) => string;
176176 /** Run-tab wiring. When `integration` + `runToolName` are provided, a third
177177 * "Run" tab hosts the per-connection tool tester. */
@@ -413,7 +413,7 @@ function PolicyBadgeMenu(props: { · 235 unmodified lines
413413 staticTool?: boolean;
414414 policy?: EffectivePolicy;
415415 onSetPolicy?: (pattern: string, action: ToolPolicyAction) => void;
416− onClearPolicy?: (pattern: string) => void;
416+ onClearPolicy?: (pattern: string, policyId?: string) => void;
417417 patternForDisplay?: (displayPattern: string) => string;
418418 }) {
419419 const interactive = !!props.onSetPolicy;
@@ -487,7 +487,10 @@ function PolicyBadgeMenu(props: { · 67 unmodified lines
487487 <>
488488 <DropdownMenuSeparator />
489489 <DropdownMenuItem
490− onSelect={() => props.onClearPolicy?.(pattern)}
490+ // Pass the resolved rule's id: right after this menu authored
491+ // the rule, the policies list may still be mid-refresh, and a
492+ // pattern-only lookup there would silently no-op the clear.
493+ onSelect={() => props.onClearPolicy?.(pattern, props.policy?.policyId)}
491494 className="text-muted-foreground"
492495 >
493496 Clear
packages/react/src/hooks/use-policy-actions.ts+39−37
@@ -1,8 +1,12 @@
1−import { useCallback, useMemo } from "react";
1+import { useCallback, useMemo, useRef } from "react";
22 import { useAtomSet, useAtomValue } from "@effect/atom-react";
33 import * as AsyncResult from "effect/unstable/reactivity/AsyncResult";
4−import { generateKeyBetween } from "fractional-indexing";
5−import { PolicyId, type Owner, type ToolPolicyAction } from "@executor-js/sdk/shared";
4+import {
5+ PolicyId,
6+ positionForNewPattern,
7+ type Owner,
8+ type ToolPolicyAction,
9+} from "@executor-js/sdk/shared";
610
711 import {
812 createPolicyOptimistic,
@@ -13,31 +17,17 @@ import { · 4 unmodified lines
1317 import { policyWriteKeys } from "../api/reactivity-keys";
1418 import { trackEvent } from "../api/analytics";
1519
16−// Specificity score for ordering. Higher = more specific = should sit at a
17−// lower position-key (higher precedence). New rules are auto-placed below
18−// any more-specific existing rules so a freshly-added group rule never
19−// silently shadows an existing leaf rule.
20−// `*` → 0
21−// `vercel.*` → 2 (1 literal segment, wildcard)
22−// `vercel.dns.*` → 4 (2 literal segments, wildcard)
23−// `vercel.dns` → 5 (2 literal segments, exact — beats same-prefix wildcard)
24−// `vercel.dns.create` → 7 (3 literal segments, exact)
25−const specificity = (pattern: string): number => {
26− if (pattern === "*") return 0;
27− if (pattern.endsWith(".*")) {
28− const prefix = pattern.slice(0, -2);
29− return prefix.split(".").length * 2;
30− }
31− return pattern.split(".").length * 2 + 1;
32−};
33−
3420 export interface PolicyAction {
3521 /** Set the action on a pattern. If a user rule with this exact pattern
3622 * already exists, update it. Otherwise create with auto-placed
3723 * position so more-specific rules keep precedence. */
3824 readonly set: (pattern: string, action: ToolPolicyAction) => Promise<void>;
39− /** Remove the user rule with this exact pattern, if any. No-op if none. */
40− readonly clear: (pattern: string) => Promise<void>;
25+ /** Remove the user rule with this exact pattern, if any. `policyId` is the
26+ * id of the rule the caller believes is active (e.g. from the tool's
27+ * resolved EffectivePolicy); it is the fallback when the local policies
28+ * list is mid-refresh and doesn't contain the rule yet — without it a
29+ * clear in that window silently no-ops. */
30+ readonly clear: (pattern: string, policyId?: string) => Promise<void>;
4131 /** True while a write is in flight. */
4232 readonly busy: boolean;
4333 }
@@ -83,20 +73,23 @@ export const usePolicyActions = (owner: Owner = "org"): PolicyAction => { · 39 unmodified lines
8373
8474 const busy = policies.waiting;
8575
76+ // Server-assigned ids of rules this hook created, by pattern. The local
77+ // policies list is optimistic: right after a create it holds a placeholder
78+ // row with a fake `pending-*` id (and the resolved EffectivePolicy a caller
79+ // hands back can carry that same fake id), so neither is safe to DELETE
80+ // with. The create response is the one authoritative id source in that
81+ // window.
82+ const createdIdByPattern = useRef(new Map<string, string>());
83+
84+ // Specificity-aware placement (below any more-specific rule) via the shared
85+ // sdk helper — the same computation the server applies when no position is
86+ // sent. Computing it here too keeps the optimistic UI's final order stable;
87+ // if this client's list is stale, the server default is the backstop.
8688 const computePosition = useCallback(
8789 (newPattern: string): string | undefined => {
8890 const committed = sorted.filter((r) => r.position !== "");
8991 if (committed.length === 0) return undefined;
90− const newScore = specificity(newPattern);
91− // Walk down the list (most-precedent first); place the new rule
92− // just before the first existing rule whose specificity is <= the
93− // new one. That way more-specific rules stay above us, and we win
94− // against everything equally or less specific.
95− let idx = committed.findIndex((r) => specificity(r.pattern) <= newScore);
96− if (idx === -1) idx = committed.length; // append at bottom
97− const prev = idx === 0 ? null : committed[idx - 1]!.position;
98− const next = idx === committed.length ? null : committed[idx]!.position;
99− return generateKeyBetween(prev, next);
92+ return positionForNewPattern(newPattern, committed);
10093 },
10194 [sorted],
10295 );
@@ -121,24 +114,33 @@ export const usePolicyActions = (owner: Owner = "org"): PolicyAction => { · 18 unmodified lines
121114 return;
122115 }
123116 const position = computePosition(pattern);
124− await doCreate({
117+ const created = await doCreate({
125118 payload:
126119 position === undefined
127120 ? { owner, pattern, action }
128121 : { owner, pattern, action, position },
129122 reactivityKeys: policyWriteKeys,
130123 });
124+ createdIdByPattern.current.set(pattern, String(created.id));
131125 trackEvent("tool_policy_set", { action, pattern_kind: patternKind, owner });
132126 },
133127 [owner, doCreate, doUpdate, findExact, computePosition],
134128 );
135129
136130 const clear = useCallback(
137− async (pattern: string) => {
131+ async (pattern: string, policyId?: string) => {
132+ // Resolve the id to delete, most-authoritative first. The local list
133+ // and the caller's resolved policy can both hold an optimistic
134+ // `pending-*` placeholder id right after a create (before the
135+ // post-commit refetch lands); the create response we recorded is real,
136+ // and a placeholder id must never reach the server.
138137 const existing = findExact(pattern);
139− if (!existing) return;
138+ const isReal = (id: string | undefined): id is string => !!id && !id.startsWith("pending-");
139+ const id = [existing?.id, createdIdByPattern.current.get(pattern), policyId].find(isReal);
140+ if (!id) return;
141+ createdIdByPattern.current.delete(pattern);
140142 await doRemove({
141− params: { policyId: PolicyId.make(existing.id) },
143+ params: { policyId: PolicyId.make(id) },
142144 payload: { owner },
143145 reactivityKeys: policyWriteKeys,
144146 });
packages/react/src/pages/integration-detail.tsx+3−1
@@ -526,7 +526,9 @@ export function IntegrationDetailPage(props: { · 525 unmodified lines
526526 staticTool={selection?.static}
527527 policy={selectedTool.policy}
528528 onSetPolicy={(pattern, action) => void policyActions.set(pattern, action)}
529− onClearPolicy={(pattern) => void policyActions.clear(pattern)}
529+ onClearPolicy={(pattern, policyId) =>
530+ void policyActions.clear(pattern, policyId)
531+ }
530532 {...(!selection?.static && selectedBareName
531533 ? {
532534 integration: slug,
packages/react/src/pages/tools.tsx+3−1
@@ -157,7 +157,9 @@ export function ToolsPage() { · 156 unmodified lines
157157 staticTool={selection?.static}
158158 policy={selectedTool.policy}
159159 onSetPolicy={(pattern, action) => void policyActions.set(pattern, action)}
160− onClearPolicy={(pattern) => void policyActions.clear(pattern)}
160+ onClearPolicy={(pattern, policyId) =>
161+ void policyActions.clear(pattern, policyId)
162+ }
161163 />
162164 ) : (
163165 <ToolDetailEmpty hasTools={summaries.length > 0} />