feat(lint): 判红「声明了但运行期只会跳过」的 sharing-rule condition (#4698)

.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.<field>`
+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.

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 ─────────────────────────────────────────────────────────

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,

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<string, unknown>).length).toBeGreaterThan(0);
+    }
+  });
+});