diff --git a/.changeset/sharing-seeder-skip-reason.md b/.changeset/sharing-seeder-skip-reason.md new file mode 100644 index 0000000000..589bb80887 --- /dev/null +++ b/.changeset/sharing-seeder-skip-reason.md @@ -0,0 +1,5 @@ +--- +'@objectstack/plugin-sharing': patch +--- + +The sharing-rule seeder's skip WARN now names WHY a declared rule's CEL `condition` did not translate: `compileCelToFilter`'s `reason` (the aggregatable category) and `detail` (the concrete refused shape, variable path, or parse bound) are carried into the log meta instead of being collapsed to `null` one line before the log that needed them. `celToFilter` keeps its published `Record | null` signature and delegates to the new `celToFilterOutcome` sibling (the `plugin-security` rls-compiler shape from #13942, one seam over). Skip semantics are unchanged — an unlowerable or match-all condition is still never seeded as a permissive match-all rule (ADR-0049). diff --git a/packages/plugins/plugin-sharing/src/bootstrap-declared-sharing-rules.ts b/packages/plugins/plugin-sharing/src/bootstrap-declared-sharing-rules.ts index 4078d303b4..d93189485f 100644 --- a/packages/plugins/plugin-sharing/src/bootstrap-declared-sharing-rules.ts +++ b/packages/plugins/plugin-sharing/src/bootstrap-declared-sharing-rules.ts @@ -51,6 +51,7 @@ import type { SharingRuleService } from './sharing-rule-service.js'; import type { SharingRuleRecipientType, ShareAccessLevel } from '@objectstack/spec/contracts'; import { compileCelToFilter } from '@objectstack/formula'; +import type { CelFilterFailReason } from '@objectstack/formula'; import { isMatchAllCriteria } from './rule-criteria.js'; const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; @@ -129,8 +130,50 @@ function mapRecipientType(t: unknown): SharingRuleRecipientType | null { * never seeding a permissive match-all (ADR-0049). */ export function celToFilter(cel: unknown): Record | null { + return celToFilterOutcome(cel).filter; +} + +/** + * Why a declared rule's `condition` produced no criteria — the compiler's OWN + * answer, carried instead of discarded. [#13943] + * + * `compileCelToFilter` already returns `{ reason, detail }` on every refusal; + * `celToFilter` used to consume `!ok` and collapse the rest to `null` one line + * before the only WARN that could surface it — so an operator whose declared + * rule was silently not granting got the fact ("skipped") and the source text + * back, but not WHICH shape the compiler refused or why. The extra member is + * this FILE's own drop (the ADR-0049 match-all guard at the call site), which + * the compiler reports as a success — same skip, same silence, so it joins the + * same vocabulary rather than staying unnamed (the `empty-membership` + * precedent in `plugin-security/src/rls-compiler.ts`, #13639). + */ +type SharingSkipReason = CelFilterFailReason | 'match-all-criteria'; + +/** A skipped rule's cause: the compiler's `reason` (the aggregatable category) plus its human `detail` (the concrete fault). */ +interface SharingSkipCause { + reason: SharingSkipReason; + detail: string; +} + +/** {@link celToFilterOutcome}'s answer: the filter, or why there is none. */ +type CelToFilterOutcome = + | { filter: Record; cause?: undefined } + | { filter: null; cause: SharingSkipCause }; + +/** + * [#13943] {@link celToFilter}'s answer WITH the reason it refused. + * + * Same compile, same decision, same returned filter — the only difference is + * that the compiler's `{ reason, detail }` survives to the caller instead of + * being collapsed into `null` at the `!result.ok` line. `celToFilter` stays + * exactly as published (`Record | null`) and delegates here — the + * `compileExpressionOutcome` shape from `plugin-security/src/rls-compiler.ts` + * (#13942), one seam over. + */ +export function celToFilterOutcome(cel: unknown): CelToFilterOutcome { const result = compileCelToFilter(cel as string | { source?: string }, { variables: {} }); - return result.ok ? (result.filter as Record) : null; + if (!result.ok) return { filter: null, cause: { reason: result.reason, detail: result.detail } }; + return { filter: result.filter as Record }; } /** @@ -197,12 +240,29 @@ export async function bootstrapDeclaredSharingRules( // schema requires `condition`, so reaching here means a hand-crafted // `{ dialect, source: '' }` envelope or a stale pre-built package, and // neither earns a match-all. - const f = celToFilter(r.condition); - if (!f || isMatchAllCriteria(f)) { - logger?.warn?.('[sharing-rule] skipped (missing or untranslatable CEL condition — never seeded as match-all) [experimental]', { rule: r.name, condition: r.condition }); + const outcome = celToFilterOutcome(r.condition); + if (outcome.filter === null || isMatchAllCriteria(outcome.filter)) { + // [#13943] The skip keeps its REASON. `reason` + `detail` are what the + // compiler already computed — the shape it refused, the variable path, + // the parse bound that was overrun — and discarding them here is what + // left an operator with a skipped rule, its source text, and no why. + // The skip decision itself is byte-identical to before (ADR-0049: an + // unlowerable condition is never seeded as a permissive match-all). + const cause: SharingSkipCause = outcome.filter === null + ? outcome.cause + : { + // The compiler answered `ok`, so there is no compiler detail to + // carry — this drop is THIS file's match-all guard, and it names + // itself rather than being reported as untranslatable. + reason: 'match-all-criteria', + detail: + `the condition lowered to ${JSON.stringify(outcome.filter)}, which constrains nothing — ` + + 'seeding it would share every record of the object (ADR-0049)', + }; + logger?.warn?.('[sharing-rule] skipped (missing or untranslatable CEL condition — never seeded as match-all) [experimental]', { rule: r.name, condition: r.condition, reason: cause.reason, detail: cause.detail }); skipped += 1; continue; } - const criteria: Record = f; + const criteria: Record = outcome.filter; try { await ruleService.defineRule({ name: r.name, diff --git a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts index 948acc3e57..ca1216c836 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule.test.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule.test.ts @@ -13,7 +13,7 @@ import { BusinessUnitGraphService } from './business-unit-graph.js'; // ruling can be pinned in the same suite as the positive half: the filter // belongs to the sharing CALL SITE, never to the expansion helper. import { PositionGraphService } from './position-graph.js'; -import { celToFilter } from './bootstrap-declared-sharing-rules.js'; +import { celToFilter, celToFilterOutcome, bootstrapDeclaredSharingRules } from './bootstrap-declared-sharing-rules.js'; import { isMatchAllCriteria } from './rule-criteria.js'; import { bindRuleCriteriaGuard } from './rule-hooks.js'; @@ -617,6 +617,113 @@ describe('#1887 — compound sharing condition compiled + enforced (ADR-0058 D3) }); }); +// --------------------------------------------------------------------------- +// #13943 — the seeder's skip WARN carries the compiler's reason AND detail +// +// `compileCelToFilter` returns `{ ok: false, reason, detail }` on every +// refusal; `celToFilter` used to collapse the whole thing to `null` one line +// before the only WARN that could surface it, so the operator learned THAT +// the condition did not translate but never WHY. `celToFilterOutcome` is the +// sibling that keeps the cause (`compileExpressionOutcome` in +// plugin-security's rls-compiler, #13942, is the same shape one seam over); +// `celToFilter` stays exactly as published (`Record | null`) and delegates. +// The skip DECISION is unchanged either way — an unlowerable condition is +// never seeded as a permissive match-all (ADR-0049). +// --------------------------------------------------------------------------- +describe('#13943 — sharing-rule seeder skip WARN names the compiler’s reason and detail', () => { + const SKIP_WARN = + '[sharing-rule] skipped (missing or untranslatable CEL condition — never seeded as match-all) [experimental]'; + + /** Registry-backed seeder harness: declared rules in, defineRule + log lines out. */ + function seedHarness(declared: any[]) { + const engine = { _registry: { listItems: (type: string) => (type === 'sharing_rule' ? declared : []) } }; + const defineRule = vi.fn(async (input: any) => ({ id: `id_${input.name}` })); + const warns: Array<{ msg: string; meta: any }> = []; + const logger = { + warn: (msg: string, meta?: any) => { warns.push({ msg, meta }); }, + info: () => {}, + }; + return { engine, ruleService: { defineRule } as any, logger, warns, defineRule }; + } + + const RULE_BASE = { + object: 'opportunity', + sharedWith: { type: 'user', value: 'alice' }, + accessLevel: 'read', + }; + + it('celToFilterOutcome carries the compiler’s { reason, detail } instead of collapsing to null', () => { + // Refusal: a function call is not pushdown-able — the cause survives. + const refused = celToFilterOutcome('size(record.tags) > 0'); + expect(refused.filter).toBeNull(); + expect(refused.cause?.reason).toBe('unsupported'); + expect(refused.cause?.detail).toEqual(expect.any(String)); + expect(refused.cause?.detail.length).toBeGreaterThan(0); + // Missing / empty condition: the compiler's own empty-expression refusal. + const missing = celToFilterOutcome(undefined); + expect(missing).toEqual({ filter: null, cause: { reason: 'parse-error', detail: 'empty expression' } }); + // Success: same filter celToFilter returns, and no cause at all. + const ok = celToFilterOutcome('record.amount >= 100000'); + expect(ok.filter).toEqual({ amount: { $gte: 100000 } }); + expect(ok.cause).toBeUndefined(); + // The published wrapper delegates: byte-identical answers on both paths. + expect(celToFilter('size(record.tags) > 0')).toBeNull(); + expect(celToFilter('record.amount >= 100000')).toEqual(ok.filter); + }); + + it('an untranslatable condition is skipped WITH the why: reason + detail land in the WARN meta', async () => { + const { engine, ruleService, logger, warns, defineRule } = seedHarness([ + { ...RULE_BASE, name: 'r_unsupported', condition: 'size(record.tags) > 0' }, + ]); + const res = await bootstrapDeclaredSharingRules(ruleService, null, engine, logger); + expect(res).toEqual({ seeded: 0, skipped: 1 }); + expect(defineRule).not.toHaveBeenCalled(); + const skips = warns.filter((w) => w.msg === SKIP_WARN); + expect(skips).toHaveLength(1); + // The fact (rule, condition) is still there; the why (reason, detail) now is too. + expect(skips[0].meta).toMatchObject({ + rule: 'r_unsupported', + condition: 'size(record.tags) > 0', + reason: 'unsupported', + }); + expect(skips[0].meta.detail).toEqual(expect.any(String)); + expect(skips[0].meta.detail.length).toBeGreaterThan(0); + }); + + it('NEGATIVE: a rule whose condition lowers cleanly emits no skip line and seeds as before', async () => { + const { engine, ruleService, logger, warns, defineRule } = seedHarness([ + { ...RULE_BASE, name: 'r_clean', condition: 'record.amount >= 100000' }, + ]); + const res = await bootstrapDeclaredSharingRules(ruleService, null, engine, logger); + expect(res).toEqual({ seeded: 1, skipped: 0 }); + // No new log line of any kind on the clean path — not just "no skip WARN". + expect(warns).toHaveLength(0); + expect(defineRule).toHaveBeenCalledTimes(1); + expect(defineRule.mock.calls[0][0]).toMatchObject({ + name: 'r_clean', + criteria: { amount: { $gte: 100000 } }, + }); + }); + + it('NEGATIVE: a MISSING condition takes the path it takes today — skipped via the same WARN, never seeded', async () => { + const { engine, ruleService, logger, warns, defineRule } = seedHarness([ + { ...RULE_BASE, name: 'r_missing' /* no condition at all */ }, + ]); + const res = await bootstrapDeclaredSharingRules(ruleService, null, engine, logger); + expect(res).toEqual({ seeded: 0, skipped: 1 }); + expect(defineRule).not.toHaveBeenCalled(); + const skips = warns.filter((w) => w.msg === SKIP_WARN); + expect(skips).toHaveLength(1); + // Same branch, same message string as before; the cause names the + // compiler's own empty-expression refusal rather than being absent. + expect(skips[0].meta).toMatchObject({ + rule: 'r_missing', + reason: 'parse-error', + detail: 'empty expression', + }); + }); +}); + // --------------------------------------------------------------------------- // #3896 — a rule with no criteria must share NOTHING, never everything //