From 8c86a3efca5c4ee4d72066e08dfb72ca8290d80f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 3 Aug 2026 18:38:35 +0000 Subject: [PATCH] feat(lint): gate a sharing-rule condition the runtime can only skip (#4698) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #4698 asks for a "declared but never read" check. The general form is not a lint question: "is this key read?" is only decidable from authored metadata when the consumer's decision procedure is in hand, and a repo-wide grep for a reader is famously not evidence of absence (#4604, #4914). A sharing rule's `condition` is the case where it IS decidable. Its one runtime consumer, `bootstrapDeclaredSharingRules`, does exactly one thing with the key: `compileCelToFilter(condition, { variables: {} })`. A condition that does not lower is not degraded or partially applied — the rule is SKIPPED, never reaches `sys_sharing_rule`, and grants nothing, with one boot WARN as the only trace. So the new rule does not model the consumer. It calls the consumer's own compiler, from the same package, on the same input, with the same options, and is pinned in both directions against a shared corpus. Two ids, because the two authoring mistakes have different fixes: - `sharing-rule-unlowerable-condition` — outside the pushdown subset (`has(...)`, `size(...)`, arithmetic, ternary, cross-object path). This is the issue's measured instance: `has()` is right in an object validation, which is interpreted, and wrong here, where the condition is compiled. - `sharing-rule-runtime-variable-condition` — reads `current_user.*`. Criteria rules are materialised, so there is no current user at compile time; the fix is RLS, a different mechanism, not a respelling. Both `error`, on all three commands, for the ADR-0078 reason SharingRuleSchema's own docblock states: the whole authorable surface is enforced, and this was the one field where that sentence was not true. CEL syntax stays with `expression-invalid` — one field, one complaint. Measured before shipping: every sharing-rule condition and RLS predicate declared anywhere in this repo lowers cleanly, so nothing that works today goes red. The "before" half of the proof is in the test rather than in prose — the full authoring registry runs over the offending stack and every OTHER rule must stay silent, which keeps holding as rules are added. Deliberately out of scope, filed instead of guessed: - RLS `using`/`check` (#4983) — same class, and ADR-0056 D4 explicitly asks for the gate, but its decision procedure needs `sqlPredicateToCel`, which lives in a runtime `@objectstack/lint` must not import. Hoisting the bridge into `@objectstack/formula` comes first; copying it would fork the predicate. - `validateOrgAxisRedLines` reads sharing-rule keys the spec rejects (#4984). - The tenant-scoped declared unique index (issue instance 2) is a driver-sql / objectql fix; tenancy is kernel-injected, not authored, so lint cannot judge it — the existing rule says so in its own comment. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_018iARDqtrhQgz6fVHDeDkbQ --- ...sharing-rule-unlowerable-condition-gate.md | 58 ++++ packages/lint/src/authoring-rules.ts | 26 ++ packages/lint/src/index.ts | 14 + ...lidate-sharing-rule-enforceability.test.ts | 240 +++++++++++++++++ .../validate-sharing-rule-enforceability.ts | 248 ++++++++++++++++++ 5 files changed, 586 insertions(+) create mode 100644 .changeset/sharing-rule-unlowerable-condition-gate.md create mode 100644 packages/lint/src/validate-sharing-rule-enforceability.test.ts create mode 100644 packages/lint/src/validate-sharing-rule-enforceability.ts diff --git a/.changeset/sharing-rule-unlowerable-condition-gate.md b/.changeset/sharing-rule-unlowerable-condition-gate.md new file mode 100644 index 0000000000..7ddbee239a --- /dev/null +++ b/.changeset/sharing-rule-unlowerable-condition-gate.md @@ -0,0 +1,58 @@ +--- +"@objectstack/lint": minor +--- + +feat(lint): reject a sharing-rule condition the runtime can only skip (#4698) + +#4698 reported the same failure shape three times in one app in one day: a key +that is authored, is schema-valid, reads as meaningful — and is never consumed +by the runtime. Every check verifies that what is declared is *well-formed*, +never that it is *read*. The issue's third measured instance is a sharing rule +whose CEL `condition` uses `has(...)`: the seeder cannot lower it, skips the +rule, and the only signal is one WARN line at boot. The rule exists in +metadata, is absent from `sys_sharing_rule`, and grants nothing. + +**New rules, both `error`, on all three authoring commands:** + +- **`sharing-rule-unlowerable-condition`** — the condition is outside the + pushdown subset: a function call (`has(...)`, `size(...)`), arithmetic, a + ternary, or a cross-object path (`record.account.region`). +- **`sharing-rule-runtime-variable-condition`** — the condition reads + `current_user.*`. Criteria sharing rules are materialised (one static + `criteria_json` per rule, from which grants are written), so there is no + "current user" at compile time. The fix is a different mechanism, not a + different spelling, which is why it has its own id. + +Fix each by rewriting the predicate inside the lowerable subset — `==` `!=` +`>` `<` `>=` `<=`, `in`, `&&` `||` `!`, `== null` / `!= null`, and +`startsWith` / `endsWith` / `contains` over single-column `record.` +paths (ADR-0058 D2). Two specific migrations: `has(record.x)` → `record.x != +null` (`has()` is correct in an object *validation* rule, which is +interpreted, and wrong here, where the condition is compiled); and a related +record's field → denormalise it onto this object (formula/rollup) and test +that column, or share the related object instead. For per-user access, use an +RLS policy (`rowLevelSecurity[].using`), where `current_user.*` *is* resolved. + +**Why this one surface and not "unread keys" in general.** "Is this key read?" +is only a lint question when the answer is computable from the authored +metadata alone, and usually it is not — a repo-wide grep for a reader is not +evidence of absence, and a consumer may live in another package, another repo, +or an uninstalled plugin. A sharing rule's `condition` is the case where the +predicate is exact: its one runtime consumer +(`bootstrapDeclaredSharingRules`) does exactly one thing with the key — +`compileCelToFilter(condition, { variables: {} })` — and a condition that does +not lower means the rule is skipped outright. So the lint calls that same +compiler, from the same package, with the same options, instead of modelling +the consumer; the verdict is identical to the seeder's by construction and is +pinned in both directions by a test over a shared corpus. + +`error` rather than advisory, per the ADR-0078 claim `SharingRuleSchema`'s own +docblock makes ("the whole authorable surface is enforced — nothing here +validates and then silently does nothing"): there is no reading under which an +unlowerable condition does what it says. It fails closed, which is why it was +survivable, not why it was acceptable. Measured before shipping: every +sharing-rule condition declared anywhere in this repo lowers cleanly, so the +gate turns nothing red that works today. + +CEL *syntax* errors are deliberately left to `expression-invalid`, which +already gates this same field with a message written about syntax. diff --git a/packages/lint/src/authoring-rules.ts b/packages/lint/src/authoring-rules.ts index ac6b36d03c..dbf41dde34 100644 --- a/packages/lint/src/authoring-rules.ts +++ b/packages/lint/src/authoring-rules.ts @@ -120,6 +120,7 @@ import { validateSeedStateMachine } from './validate-seed-state-machine.js'; import { validateVisibilityPredicates } from './validate-visibility-predicates.js'; import { validateSecurityPosture } from './validate-security-posture.js'; import { validateOrgAxisRedLines } from './validate-org-axis-red-lines.js'; +import { validateSharingRuleEnforceability } from './validate-sharing-rule-enforceability.js'; import { validateActionLocations } from './validate-action-locations.js'; import { lintFlowPatterns } from './lint-flow-patterns.js'; import { lintLivenessProperties } from './lint-liveness-properties.js'; @@ -795,6 +796,31 @@ export const AUTHORING_RULES: readonly AuthoringRule[] = [ surfaceReason: RUNTIME_NEEDS_FULL_SNAPSHOT, run: (stack) => validateOrgAxisRedLines(stack), }, + // #4698 — the "declared but never read" gate, for the one surface where the + // predicate is EXACT rather than inferred. A sharing rule's `condition` has a + // single runtime consumer (`bootstrapDeclaredSharingRules`) whose only use of + // the key is `compileCelToFilter(condition, { variables: {} })`; a condition + // that does not lower means the rule is SKIPPED at boot, so the grant is + // declared and does not exist. The lint calls that same compiler, from the + // same package, with the same options — the verdict cannot drift from the + // consumer's. Gating for the ADR-0078 reason `SharingRuleSchema`'s own + // docblock states: the whole authorable surface is enforced, and this was the + // one field where that sentence was not yet true. + { + name: 'validateSharingRuleEnforceability', + tier: 'gating', + input: 'parsed', + commands: ALL, + source: 'packages/lint/src/validate-sharing-rule-enforceability.ts', + surfaces: CLI_ONLY, + surfaceReason: + 'P2 (#4463): a sharing rule is not a `flow`, and P1 gates `flow` alone. The rule itself is ' + + 'snapshot-safe — it reads ONLY `stack.sharingRules[].condition` and needs no other collection — ' + + 'so widening it here is a `runtimeTypes: [\'sharing_rule\']` edit once the gate accepts that type, ' + + 'not new wiring. Recorded as pending rather than done, because a rule that has never run at a ' + + 'door should not claim it.', + run: (stack) => validateSharingRuleEnforceability(stack), + }, ]; // ─── Runner ───────────────────────────────────────────────────────── diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index 1ce022c37d..5743f01a25 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -195,6 +195,20 @@ export { } from './validate-org-axis-red-lines.js'; export type { OrgAxisFinding, OrgAxisSeverity } from './validate-org-axis-red-lines.js'; +// #4698 — "a key that nothing reads should not validate clean", for the one +// surface where "is it read?" is decidable: a sharing rule's `condition` is +// read ONLY through `compileCelToFilter`, so the lint calls that same compiler +// rather than modelling the consumer. +export { + validateSharingRuleEnforceability, + SHARING_RULE_UNLOWERABLE_CONDITION, + SHARING_RULE_RUNTIME_VARIABLE_CONDITION, +} from './validate-sharing-rule-enforceability.js'; +export type { + SharingRuleEnforceabilityFinding, + SharingRuleEnforceabilitySeverity, +} from './validate-sharing-rule-enforceability.js'; + export { validateDashboardActionRefs, DASHBOARD_ACTION_TARGET_UNDEFINED, diff --git a/packages/lint/src/validate-sharing-rule-enforceability.test.ts b/packages/lint/src/validate-sharing-rule-enforceability.test.ts new file mode 100644 index 0000000000..386a28acbc --- /dev/null +++ b/packages/lint/src/validate-sharing-rule-enforceability.test.ts @@ -0,0 +1,240 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { compileCelToFilter } from '@objectstack/formula'; + +import { + validateSharingRuleEnforceability, + SHARING_RULE_UNLOWERABLE_CONDITION, + SHARING_RULE_RUNTIME_VARIABLE_CONDITION, +} from './validate-sharing-rule-enforceability.js'; +import { AUTHORING_RULES, runAuthoringRules } from './authoring-rules.js'; + +const ids = (stack: unknown) => validateSharingRuleEnforceability(stack).map((f) => f.rule); + +/** A complete, spec-shaped sharing rule with the condition swapped in. */ +const ruleWith = (condition: unknown) => ({ + sharingRules: [ + { + name: 'high_value_opps', + type: 'criteria', + object: 'opportunity', + accessLevel: 'read', + sharedWith: { type: 'team', value: 'deal_desk' }, + condition, + }, + ], +}); + +// ── Red: declared, schema-valid, and never read ────────────────────── +// +// Every source below parses as CEL and passes `SharingRuleSchema`. The seeder +// still drops the rule on the floor, which is the whole defect (#4698). + +describe('validateSharingRuleEnforceability — the declared-but-never-read cases go RED', () => { + it('flags `has(...)` — the measured instance from the issue (hotcrm#621/#633)', () => { + const findings = validateSharingRuleEnforceability( + ruleWith("has(record.owner_id) && record.stage == 'closed_won'"), + ); + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ + severity: 'error', + rule: SHARING_RULE_UNLOWERABLE_CONDITION, + // The declaration site is named, not just the rule. + path: 'sharingRules[0].condition', + where: 'sharing rule "high_value_opps" on object "opportunity"', + }); + // It must say what actually happens at boot, not merely "unsupported". + expect(findings[0].message).toMatch(/SKIPS the rule at boot/); + expect(findings[0].message).toMatch(/never written to `sys_sharing_rule`/); + // …and prescribe the fix that works on THIS surface. + expect(findings[0].hint).toMatch(/record\.x != null/); + expect(findings[0].hint).toMatch(/INTERPRETED/); + }); + + it.each([ + ['a bare function call', 'size(record.tags) > 0'], + ['arithmetic', 'record.amount * 2 > 100'], + ['a cross-object path', "record.account.region == 'EU'"], + ['a ternary', "record.stage == 'won' ? true : false"], + ])('flags %s', (_label, source) => { + expect(ids(ruleWith(source))).toEqual([SHARING_RULE_UNLOWERABLE_CONDITION]); + }); + + it('gives `current_user.*` its own id — the fix is a different mechanism, not a respelling', () => { + const findings = validateSharingRuleEnforceability(ruleWith('record.owner_id == current_user.id')); + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ + severity: 'error', + rule: SHARING_RULE_RUNTIME_VARIABLE_CONDITION, + path: 'sharingRules[0].condition', + }); + expect(findings[0].message).toMatch(/current_user\.id/); + // Points at the surface where `current_user.*` IS resolved. + expect(findings[0].hint).toMatch(/rowLevelSecurity\[\]\.using/); + }); + + it('reports the parsed tier identically — the envelope `ExpressionInputSchema` produces', () => { + expect(ids(ruleWith({ dialect: 'cel', source: 'size(record.tags) > 0' }))) + .toEqual([SHARING_RULE_UNLOWERABLE_CONDITION]); + }); + + it('names each offending rule separately, with its own index', () => { + const findings = validateSharingRuleEnforceability({ + sharingRules: [ + { name: 'ok', object: 'a', condition: "record.stage == 'won'" }, + { name: 'fn', object: 'b', condition: 'has(record.x)' }, + { name: 'var', object: 'c', condition: 'record.owner == current_user.id' }, + ], + }); + expect(findings.map((f) => [f.rule, f.path])).toEqual([ + [SHARING_RULE_UNLOWERABLE_CONDITION, 'sharingRules[1].condition'], + [SHARING_RULE_RUNTIME_VARIABLE_CONDITION, 'sharingRules[2].condition'], + ]); + }); +}); + +// ── Green: conditions that really are read ─────────────────────────── +// +// A false positive here is worse than the gap the rule closes: it rejects +// security metadata that enforces correctly today and hands the author a +// "correction" that would break it. + +describe('validateSharingRuleEnforceability — conditions the runtime DOES read stay green', () => { + it.each([ + ["record.health == 'red'"], + ["record.health == 'red' && record.budget > 100000"], + ['record.done == false'], + ["record.stage in ['closed_won', 'closed_lost']"], + ['record.closed_at == null'], + ["record.name.startsWith('ACME')"], + ["!(record.stage in ['draft']) || record.amount >= 1000"], + ])('accepts %s', (source) => { + expect(validateSharingRuleEnforceability(ruleWith(source))).toEqual([]); + }); + + it('accepts every sharing-rule condition the bundled examples declare', () => { + // Lifted verbatim from examples/app-showcase/src/security/sharing-rules.ts + // and examples/app-crm. The gate must not turn shipped apps red. + const shipped = [ + "record.health == 'red'", + "record.health == 'red' && record.budget > 100000", + "record.status == 'new'", + 'record.done == false', + ]; + for (const source of shipped) { + expect(validateSharingRuleEnforceability(ruleWith(source))).toEqual([]); + } + }); + + it('leaves CEL SYNTAX errors to validateStackExpressions — no double report', () => { + // Parses nowhere, but this rule stays silent: `expression-invalid` already + // gates the same field with a message written about syntax. + expect(compileCelToFilter('record.stage ==', { variables: {} })).toMatchObject({ reason: 'parse-error' }); + expect(validateSharingRuleEnforceability(ruleWith('record.stage =='))).toEqual([]); + }); + + it('ignores shapes Zod owns rather than inventing a second complaint', () => { + expect(ids(ruleWith(undefined))).toEqual([]); + expect(ids(ruleWith(''))).toEqual([]); + expect(ids(ruleWith(' '))).toEqual([]); + expect(ids(ruleWith(42))).toEqual([]); + expect(ids(ruleWith({ dialect: 'cel' }))).toEqual([]); + }); + + it('is a no-op on a stack that declares no sharing rules', () => { + expect(validateSharingRuleEnforceability({})).toEqual([]); + expect(validateSharingRuleEnforceability(undefined)).toEqual([]); + expect(validateSharingRuleEnforceability({ sharingRules: [] })).toEqual([]); + }); + + it('checks inactive rules too — the seeder compiles the condition regardless of `active`', () => { + // `bootstrapDeclaredSharingRules` carries `active` through to `defineRule`; + // it does not skip the compile. A rule that is off today and unlowerable is + // still a rule that will grant nothing the day someone switches it on. + expect(ids({ sharingRules: [{ name: 'r', object: 'o', active: false, condition: 'has(record.x)' }] })) + .toEqual([SHARING_RULE_UNLOWERABLE_CONDITION]); + }); +}); + +// ── The predicate is the consumer's, not a model of it ─────────────── + +describe('validateSharingRuleEnforceability — the verdict IS the seeder\'s verdict', () => { + const corpus = [ + "record.health == 'red'", + "record.health == 'red' && record.budget > 100000", + 'record.done == false', + "record.stage in ['a', 'b']", + 'record.closed_at != null', + 'has(record.owner_id)', + 'size(record.tags) > 0', + 'record.amount * 2 > 100', + "record.account.region == 'EU'", + 'record.owner_id == current_user.id', + ]; + + it('agrees with `compileCelToFilter({ variables: {} })` on every source, in both directions', () => { + for (const source of corpus) { + // This is exactly the call `bootstrap-declared-sharing-rules.ts` makes. + const seederWouldSeed = compileCelToFilter(source, { variables: {} }).ok; + const lintIsClean = validateSharingRuleEnforceability(ruleWith(source)).length === 0; + expect({ source, lintIsClean }).toEqual({ source, lintIsClean: seederWouldSeed }); + } + }); + + /** + * The "before" half of the proof, kept mechanical rather than asserted in + * prose. #4698's complaint is that the offending stack passes the WHOLE + * toolchain, so it is not enough to show the new rule goes red — it must also + * be shown that nothing else ever did. Running the full author-time registry + * over the fixture and demanding that every OTHER rule stays silent is that + * statement, and unlike a comment it keeps holding: if some future rule grows + * to cover this shape, this test fails and someone has to decide which of the + * two owns it, instead of the stack quietly acquiring a duplicate diagnostic. + */ + it('no OTHER author-time rule sees this — which is exactly why the gate was missing', () => { + const offending = { + objects: [ + { + name: 'opportunity', + label: 'Opportunity', + sharingModel: 'private', + fields: { + name: { type: 'text', label: 'Name' }, + owner_id: { type: 'text', label: 'Owner' }, + stage: { type: 'text', label: 'Stage' }, + }, + }, + ], + sharingRules: [ + { + name: 'closed_won_to_deal_desk', + type: 'criteria', + object: 'opportunity', + accessLevel: 'read', + sharedWith: { type: 'team', value: 'deal_desk' }, + condition: "has(record.owner_id) && record.stage == 'closed_won'", + }, + ], + }; + + const findings = runAuthoringRules('validate', { normalized: offending, parsed: offending }); + expect(findings.map((f) => f.rule)).toEqual([SHARING_RULE_UNLOWERABLE_CONDITION]); + + // And the fixture really did travel through every rule — a registry that + // silently stopped running would satisfy the assertion above vacuously. + expect(AUTHORING_RULES.filter((r) => r.commands.includes('validate')).length).toBeGreaterThan(20); + }); + + it('a match-all filter is unreachable from a lowering condition (so lint need not re-check it)', () => { + // The seeder's SECOND guard is `isMatchAllCriteria(f)`, which lives in + // plugin-sharing — a runtime `@objectstack/lint` must not import. This + // pins the claim in the module docblock that duplicating it would be dead + // code: every condition the compiler lowers yields a concrete predicate. + for (const source of corpus) { + const result = compileCelToFilter(source, { variables: {} }); + if (!result.ok) continue; + expect(Object.keys(result.filter as Record).length).toBeGreaterThan(0); + } + }); +}); diff --git a/packages/lint/src/validate-sharing-rule-enforceability.ts b/packages/lint/src/validate-sharing-rule-enforceability.ts new file mode 100644 index 0000000000..87249a2a07 --- /dev/null +++ b/packages/lint/src/validate-sharing-rule-enforceability.ts @@ -0,0 +1,248 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #4698 — "declared but never read", made decidable for ONE surface. + * + * The issue behind this file reported three shapes that pass `os validate`, + * `os lint`, `tsc` and a full test suite while the runtime never reads them. + * The useful invariant it names is the ledger discipline's: **a key that + * nothing reads should not validate clean.** The hard part is not the + * sentiment, it is the PREDICATE — "is this key read?" is only a lint question + * when the answer is computable from the authored metadata alone. Most of the + * time it is not: a repo-wide grep for a reader is famously not evidence of + * absence (#4604), and a consumer can live in another package, another repo, or + * a plugin nobody has installed yet (#4914). + * + * A sharing rule's `condition` is the case where the predicate is EXACT, and + * that is the whole reason this rule exists and its neighbours are deferred: + * + * The one runtime consumer of a stack-declared `sharingRules[].condition` is + * `bootstrapDeclaredSharingRules` (plugin-sharing), and the ONLY thing it + * does with the key is hand it to `compileCelToFilter(condition, { variables: + * {} })`. A condition that does not lower is not degraded, not partially + * applied and not deferred — the rule is SKIPPED (ADR-0049: never seeded as a + * permissive match-all), so it never reaches `sys_sharing_rule` and grants + * nothing at all. The only trace is one WARN line at boot. + * + * So this rule does not model the consumer, guess at it, or grep for it. It + * calls the consumer's own decision procedure, from the same package + * (`@objectstack/formula`), on the same input, with the same options. The + * verdict is bit-identical to the seeder's by construction — there is no + * heuristic to drift and no false positive that is not also a real skip. + * + * ## Why `error` + * + * `SharingRuleSchema`'s own docblock makes the claim this rule enforces: "The + * whole authorable surface is enforced — nothing here validates and then + * silently does nothing (ADR-0078)." Until now that sentence held for every + * part of the shape EXCEPT the one field carrying the author's intent. There is + * no reading under which an unlowerable condition does what it says: the grant + * does not exist. Per the severity bar `lint-flow-patterns.ts` states — gate + * when no reading of the metadata behaves as written — that is an `error`, not + * a warning. It fails closed (the recipient under-sees rather than over-sees), + * which is why it was survivable, not why it is acceptable. + * + * Measured before shipping: every sharing-rule condition and RLS predicate + * declared anywhere in this repo (examples, platform permission sets) lowers + * cleanly, so the gate turns nothing red that works today. + * + * ## The two ids, and why not one + * + * `compileCelToFilter` fails for three reasons; two of them are authoring + * mistakes with DIFFERENT fixes, so they get different ids rather than one id + * with a branchy message (allowlists and `--json` consumers key on the id): + * + * - `unsupported` → {@link SHARING_RULE_UNLOWERABLE_CONDITION}. The shape is + * outside the pushdown subset: a function call (`has(...)`, `size(...)`), + * arithmetic, a ternary, or a cross-object path (`record.account.region`). + * This is the issue's measured instance — an author following the guidance + * that is correct for object VALIDATIONS (where `has()` is interpreted, not + * lowered) writes silently inert security metadata. + * - `unresolved-variable` → {@link SHARING_RULE_RUNTIME_VARIABLE_CONDITION}. + * The condition reads `current_user.*`. Criteria sharing rules are + * MATERIALIZED — the seeder compiles one static `criteria_json` per rule and + * the evaluator writes `sys_record_share` grants from it — so there is no + * "current user" at compile time and the compiler correctly refuses. The fix + * is a different mechanism (RLS / an ownership-shaped grant), not a + * different spelling, which is exactly why it earns its own id. + * - `parse-error` → deliberately NOT reported here. CEL syntax is + * `validateStackExpressions`' surface and it already gates this same field + * (`stack.sharingRules[].condition`). Reporting it twice, in two + * vocabularies, would make the second report noise — and the first one is + * the better message, because it is written about syntax. + * + * ## What this rule deliberately does NOT do + * + * - **It does not re-implement `isMatchAllCriteria`.** The seeder's second + * guard (skip a condition that lowers to a filter constraining nothing) + * lives in `plugin-sharing`, and `@objectstack/lint` never depends on a + * runtime. Copying it here would fork the one definition of "this predicate + * constrains nothing" that `rule-criteria.ts` exists to be. It is also + * unreachable from this door: every AST the compiler lowers yields a + * concrete field predicate, so a lowering CEL condition cannot produce a + * match-all filter. `matchAllIsUnreachable` in the tests pins that claim + * against the compiler rather than asserting it in prose. + * - **It does not judge RLS `using` / `check`.** Same class, same compiler, + * and ADR-0056 D4 explicitly asks for this gate — but the RLS decision + * procedure is `isSupportedRlsExpression`, which first bridges the legacy + * SQL-ish subset through `sqlPredicateToCel`, and BOTH live in + * `plugin-security`. Judging RLS here means either importing a runtime + * (forbidden) or copying the bridge (forking the predicate — the thing this + * file's opening paragraph refuses to do). Hoisting `sqlPredicateToCel` into + * `@objectstack/formula` first would make it exact; that is a spec/runtime + * move, not a lint change, and is filed separately. + * - **It does not look at flow / hook `condition`s.** Those are INTERPRETED by + * the CEL engine, not lowered to a filter, so the whole language is in scope + * there and non-pushdownability means nothing. Reusing this predicate on + * them would reject working metadata — the false-positive direction that is + * worse than the gap. + */ + +import { compileCelToFilter } from '@objectstack/formula'; + +/** A `condition` outside the pushdown subset — the rule is never seeded. */ +export const SHARING_RULE_UNLOWERABLE_CONDITION = 'sharing-rule-unlowerable-condition'; +/** A `condition` reading `current_user.*` — unresolvable when grants are materialized. */ +export const SHARING_RULE_RUNTIME_VARIABLE_CONDITION = 'sharing-rule-runtime-variable-condition'; + +export type SharingRuleEnforceabilitySeverity = 'error' | 'warning'; + +export interface SharingRuleEnforceabilityFinding { + severity: SharingRuleEnforceabilitySeverity; + /** Diagnostic rule id (`sharing-rule-*`). */ + rule: string; + /** Human-readable location, e.g. `sharing rule "high_value_opps" on object "opportunity"`. */ + where: string; + /** Config path, e.g. `sharingRules[2].condition`. */ + path: string; + /** What is wrong. */ + message: string; + /** How to fix it. */ + hint: string; +} + +type AnyRec = Record; + +/** Coerce a collection (array or name-keyed map) to an array of records. */ +function asArray(v: unknown): AnyRec[] { + if (Array.isArray(v)) return v as AnyRec[]; + if (v && typeof v === 'object') { + return Object.entries(v as AnyRec).map(([name, def]) => ({ name, ...(def as AnyRec) })); + } + return []; +} + +function str(v: unknown): string { + return typeof v === 'string' ? v : ''; +} + +/** + * The `condition` value in the shape the compiler takes. + * + * Both authoring tiers reach this rule: `os lint` runs it on the NORMALIZED + * stack, where a bare string is still a bare string, while `os validate` / + * `os build` run it after `ExpressionInputSchema` has wrapped that string into + * `{ dialect: 'cel', source }`. `compileCelToFilter` accepts either, so the + * verdict does not depend on which tier asked — the property the wiring guard's + * `input: 'parsed'` entries all have to satisfy. + * + * Anything else (a number, an envelope with no `source`) is returned as `null`: + * the shape is Zod's to reject, and inventing a second complaint about it here + * would just be the double-report this file avoids for syntax. + */ +function toCompilerInput(condition: unknown): string | { source?: string } | null { + if (typeof condition === 'string') return condition.trim() ? condition : null; + if (condition && typeof condition === 'object') { + const source = (condition as AnyRec).source; + if (typeof source === 'string' && source.trim()) return { source }; + } + return null; +} + +/** What the author sees quoted back at them. */ +function sourceOf(condition: unknown): string { + const input = toCompilerInput(condition); + if (typeof input === 'string') return input; + return str(input?.source); +} + +const PUSHDOWN_SUBSET = + 'The lowerable subset is: `==` `!=` `>` `<` `>=` `<=`, `in`, `&&` `||` `!`, `== null` / `!= null`, ' + + 'and the string methods `startsWith` / `endsWith` / `contains` — over SINGLE-column `record.` ' + + 'paths (ADR-0058 D2).'; + +/** + * Gate stack-declared sharing rules on the ONE thing their runtime consumer + * does with `condition`: lower it to a `criteria_json` filter. + * + * Pure `(stack) => Finding[]`; tolerates the normalized and the parsed tier. + */ +export function validateSharingRuleEnforceability(stack: unknown): SharingRuleEnforceabilityFinding[] { + const findings: SharingRuleEnforceabilityFinding[] = []; + const cfg = (stack ?? {}) as AnyRec; + + asArray(cfg.sharingRules).forEach((rule, index) => { + const input = toCompilerInput(rule.condition); + if (input === null) return; + + // The seeder's exact call: `compileCelToFilter(r.condition, { variables: {} })` + // in `bootstrap-declared-sharing-rules.ts`. Same function, same options — + // so `ok === false` here means "this rule will be skipped at boot", not + // "this rule looks suspicious". + const result = compileCelToFilter(input, { variables: {} }); + if (result.ok) return; + // Syntax belongs to `validateStackExpressions`, which already gates this + // same field with a message written about syntax. + if (result.reason === 'parse-error') return; + + const name = str(rule.name) || String(index); + const object = str(rule.object); + const where = `sharing rule "${name}"${object ? ` on object "${object}"` : ''}`; + const path = `sharingRules[${index}].condition`; + const source = sourceOf(rule.condition); + const skipped = + 'so `bootstrapDeclaredSharingRules` SKIPS the rule at boot: it is never written to ' + + '`sys_sharing_rule`, no `sys_record_share` grant is ever materialised, and the only signal is one ' + + 'WARN line in the boot log. The rule is declared and grants nothing (ADR-0049: an unlowerable ' + + 'condition is never seeded as a permissive match-all).'; + + if (result.reason === 'unresolved-variable') { + findings.push({ + severity: 'error', + rule: SHARING_RULE_RUNTIME_VARIABLE_CONDITION, + where, + path, + message: + `Sharing-rule condition \`${source}\` reads a runtime variable (${result.detail}), ` + skipped, + hint: + 'A criteria sharing rule is MATERIALISED: the seeder compiles ONE static `criteria_json` per ' + + 'rule and the evaluator writes `sys_record_share` rows from it, so there is no "current user" ' + + 'for the condition to read. Express per-user access with the mechanism that runs per request ' + + 'instead — an RLS policy on a permission set (`rowLevelSecurity[].using`, where ' + + '`current_user.*` IS resolved), or the record-ownership path. Keep this rule for the part of ' + + 'the predicate that is a property of the RECORD (e.g. `record.stage == \'closed_won\'`) and ' + + 'name the audience through `sharedWith`.', + }); + return; + } + + findings.push({ + severity: 'error', + rule: SHARING_RULE_UNLOWERABLE_CONDITION, + where, + path, + message: + `Sharing-rule condition \`${source}\` is outside the pushdown subset the runtime can compile ` + + `(${result.detail}), ` + skipped, + hint: + 'Rewrite the predicate inside the lowerable subset. ' + PUSHDOWN_SUBSET + ' Two traps in ' + + 'particular: (1) `has(record.x)` is correct in an object VALIDATION rule, which is INTERPRETED, ' + + 'and wrong here, where the condition is COMPILED — write the null test as `record.x != null`; ' + + '(2) a related-record path (`record.account.region`) is a join, which the compiler refuses by ' + + 'design (ADR-0055) — denormalise the value onto this object (a formula/rollup field) and test ' + + 'that column, or share the related object instead.', + }); + }); + + return findings; +}