fix(sharing,driver-sql): a rule authored in Setup actually applies — and switching it off actually withdraws access

.changeset/sharing-rule-unknown-sort-and-stale-help.md

@@ -0,0 +1,64 @@
+---
+"@objectstack/driver-sql": patch
+"@objectstack/plugin-sharing": patch
+---
+
+fix(driver-sql,sharing): an unsortable query loses its ORDER BY, not its rows (#3821)
+
+`SqlDriver.find()` already recovered from a SELECT projection naming a column
+the table lacks (retry with `select('*')`, the unknown field is simply absent
+from each row). The identical failure one clause over — an **ORDER BY** column
+the table lacks — fell through to `return []`. Because `count()` is a separate
+statement, the list endpoint answered `HTTP 200` with `records: []` and
+`total: 3`: the rows are there, none are shown, nothing is logged. Same family
+as the `$`-param footgun closed by #2926.
+
+It surfaced through the Console's sharing-rule **recipient picker**, which
+never listed a single candidate. The client mangled `'name asc'` into
+`0 n,1 a,2 m,…` (fixed separately in objectui) and the driver turned that into
+"no users exist", so no sharing rule could be authored from the UI at all.
+
+Rows now outrank their order: the retry ladder drops the projection first (the
+likelier culprit and the cheaper thing to lose), then the sort, then gives up.
+A query that cannot be sorted comes back **unordered instead of empty**. Errors
+that are not about an unknown column still propagate untouched.
+
+**A rule authored in Setup now actually applies — and switching it off actually
+withdraws access.** Writing a `sys_sharing_rule` rebound the per-record hooks,
+which only makes the rule reach records written FROM THEN ON. So an admin who
+created a rule and enabled it saw nothing happen: the recipient's list stayed
+empty until somebody happened to touch each record. The reverse was worse —
+switching a rule OFF, or deleting it, left every grant it had already issued in
+place, and boot backfill only reconciles ACTIVE rules, so those grants outlived
+restarts while the UI displayed the rule as disabled. The reconcile was reachable
+only through `POST /sharing/rules/:id/evaluate`, which the Console never calls.
+
+Each non-system write to `sys_sharing_rule` now also reconciles that rule's
+grants, chained behind the existing rebind: insert/update run the same
+diff-based `evaluateRule` the REST endpoint runs (it purges when the rule is
+inactive), and delete purges directly via the new
+`SharingRuleService.revokeRuleGrants` — `evaluateRule` can't help there because
+the row is already gone (`RULE_NOT_FOUND`), which is also why a rule deleted
+through the plain data API used to orphan its grants. Seeding and package
+bootstrap write with `isSystem` and are skipped; `kernel:bootstrapped` already
+backfills those. Reconciliation is best-effort and never fails the write.
+
+**The dialog's help text was engineering notes, shown to tenant admins.** The
+field descriptions on `sys_sharing_rule` render under each input in Setup, and
+they cited ADR numbers, table and column names (`parent_business_unit_id`,
+`sys_business_unit`), enum machine values the dropdown never shows
+(`business_unit`, `team`), a third-party library (better-auth), and engine
+vocabulary ("evaluation", "lifecycle"). Several were also stale: they still told
+admins to type an id or hand-write a `FilterCondition` after those inputs became
+a record picker and a visual builder. Rewritten for the reader who actually sees
+them — the implementation detail was already in the object's doc comment, which
+is where it stays. `criteria_json`'s LABEL loses its "(FilterCondition JSON)"
+suffix for the same reason, and `active` can finally say what it now does:
+turning it off withdraws the access.
+
+Also refreshes the `sys_sharing_rule` help text in the zh-CN / ja-JP / es-ES
+translation bundles, which still described `recipient_type` in terms of
+`department` (the enum value is `business_unit`) and told admins to enter a
+queue name for `recipient_id` (`queue` was removed in ADR-0078). The es-ES
+option labels for `position` / `unit_and_subordinates` were translated as
+"rol" — corrected to "Puesto" / "Unidad de negocio y subordinados".

packages/plugins/driver-sql/src/sql-driver-unknown-column-recovery.test.ts

@@ -0,0 +1,98 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * objectstack#3821 — `find()` must not turn an unknown column into "no rows".
+ *
+ * A SELECT projection naming a column the table lacks already had a retry
+ * (select `*` and let the missing field simply be absent). An unknown ORDER BY
+ * column did not: it fell through to `return []`, so the page came back empty
+ * while `count()` — a separate statement — still reported the true total. The
+ * UI reads that as "the records vanished". It surfaced through the sharing-rule
+ * recipient picker, whose client mangled `'name asc'` into a list of
+ * one-character column names.
+ *
+ * Rows matter more than their order, so an unsortable query returns the rows
+ * unordered. These tests pin that, and that a genuine SQL error still throws.
+ */
+
+import { describe, it, expect, beforeEach, afterEach } from 'vitest';
+import { SqlDriver } from '../src/index.js';
+
+describe('SqlDriver find() recovers from unknown columns (objectstack#3821)', () => {
+  let driver: SqlDriver;
+
+  beforeEach(async () => {
+    driver = new SqlDriver({
+      client: 'better-sqlite3',
+      connection: { filename: ':memory:' },
+      useNullAsDefault: true,
+    });
+
+    await driver.initObjects([
+      {
+        name: 'member',
+        fields: {
+          name: { type: 'string' },
+          rank: { type: 'integer' },
+        },
+      },
+    ]);
+
+    await driver.create('member', { id: 'm1', name: 'Carol', rank: 3 }, { bypassTenantAudit: true });
+    await driver.create('member', { id: 'm2', name: 'Alice', rank: 1 }, { bypassTenantAudit: true });
+    await driver.create('member', { id: 'm3', name: 'Bob', rank: 2 }, { bypassTenantAudit: true });
+  });
+
+  afterEach(async () => {
+    await driver.disconnect();
+  });
+
+  it('returns the rows unordered when ORDER BY names a column the table lacks', async () => {
+    const rows = await driver.find('member', {
+      orderBy: [{ field: 'nosuchfield', order: 'asc' }],
+    } as any);
+
+    expect(rows.map((r: any) => r.id).sort()).toEqual(['m1', 'm2', 'm3']);
+  });
+
+  it('still sorts when every ORDER BY column exists', async () => {
+    const rows = await driver.find('member', {
+      orderBy: [{ field: 'rank', order: 'asc' }],
+    } as any);
+
+    expect(rows.map((r: any) => r.name)).toEqual(['Alice', 'Bob', 'Carol']);
+  });
+
+  it('drops only the unknown clause when a known and an unknown sort are mixed', async () => {
+    // knex emits both columns in one ORDER BY, so the whole clause has to go;
+    // what must NOT happen is losing the rows.
+    const rows = await driver.find('member', {
+      orderBy: [{ field: 'rank', order: 'asc' }, { field: 'nosuchfield', order: 'desc' }],
+    } as any);
+
+    expect(rows).toHaveLength(3);
+  });
+
+  it('recovers from an unknown SELECT projection and an unknown sort together', async () => {
+    const rows = await driver.find('member', {
+      fields: ['name', 'nosuchfield'],
+      orderBy: [{ field: 'alsomissing', order: 'asc' }],
+    } as any);
+
+    expect(rows).toHaveLength(3);
+    expect(rows.map((r: any) => r.name).sort()).toEqual(['Alice', 'Bob', 'Carol']);
+  });
+
+  it('keeps honouring the WHERE clause while recovering', async () => {
+    const rows = await driver.find('member', {
+      where: { rank: { $gte: 2 } },
+      orderBy: [{ field: 'nosuchfield', order: 'asc' }],
+    } as any);
+
+    expect(rows.map((r: any) => r.id).sort()).toEqual(['m1', 'm3']);
+  });
+
+  it('propagates errors that are not about an unknown column', async () => {
+    await expect(driver.find('no_such_table', {} as any)).rejects.toThrow();
+  });
+});

packages/plugins/driver-sql/src/sql-driver.ts

@@ -803,7 +803,8 @@ export class SqlDriver implements IDataDriver {
   async find(object: string, query: QueryAST, options?: DriverOptions): Promise<any[]> {
     // Build everything EXCEPT the SELECT list, so the unknown-column retry
     // below can rebuild without re-deriving where/order/pagination.
-    const buildBase = () => {
+    const buildBase = (opts?: { withOrderBy?: boolean }) => {
+      const withOrderBy = opts?.withOrderBy !== false;
       const b = this.getBuilder(object, options);
       this.applyTenantScope(b, object, options);
 
@@ -813,7 +814,7 @@ export class SqlDriver implements IDataDriver {
       }
 
       // ORDER BY
-      if (query.orderBy && Array.isArray(query.orderBy)) {
+      if (withOrderBy && query.orderBy && Array.isArray(query.orderBy)) {
         for (const item of query.orderBy) {
           if (item.field) {
             b.orderBy(this.remoteColumn(object, item.field, this.mapSortField(item.field)), item.order || 'asc');
@@ -858,15 +859,31 @@ export class SqlDriver implements IDataDriver {
         // only fires when the object's schema is populated in the registry —
         // this driver backstop holds even when it isn't (notably the cloud
         // multi-tenant runtime, where the projection otherwise zeroes the list).
-        if (query.fields) {
+        //
+        // An unknown ORDER BY column is the same footgun one clause over
+        // (objectstack#3821): a client that sorts by a field the table lacks
+        // used to get an empty page with a non-zero `total` — "the rows are
+        // there but none are shown". Rows matter more than their order, so
+        // drop the sort and return them unordered rather than nothing. Ladder:
+        // projection first (it is the likelier culprit and the cheaper thing
+        // to lose), then the sort, then give up.
+        const retries: Array<() => any> = [];
+        if (query.fields) retries.push(() => buildBase().select('*'));
+        if (query.orderBy && Array.isArray(query.orderBy) && query.orderBy.length > 0) {
+          retries.push(() => buildBase({ withOrderBy: false }).select('*'));
+        }
+        results = [];
+        let recovered = false;
+        for (const retry of retries) {
           try {
-            results = await buildBase().select('*');
+            results = await retry();
+            recovered = true;
+            break;
           } catch {
-            return [];
+            // Try the next, broader fallback.
           }
-        } else {
-          return [];
         }
+        if (!recovered) return [];
       } else {
         throw error;
       }

packages/plugins/plugin-sharing/src/objects/sys-sharing-rule.object.ts

@@ -37,7 +37,7 @@ export const SysSharingRule = ObjectSchema.create({
   // We still recommend `defineSharingRule({...})` for repo-controlled
   // baselines, but admins can safely create/edit/delete from the UI.
   userActions: { create: true, edit: true, delete: true, import: false },
-  description: 'Declarative sharing rule that auto-materialises sys_record_share grants. Authored via defineSharingRule() in code or the Studio criteria builder.',
+  description: 'Grants a group of people access to the records that match a condition.',
   displayNameField: 'name',
   nameField: 'name', // [ADR-0079] canonical primary-title pointer (mirrors deprecated displayNameField)
   titleFormat: '{label}',
@@ -99,7 +99,13 @@ export const SysSharingRule = ObjectSchema.create({
       label: 'Name',
       required: true,
       maxLength: 100,
-      description: 'Unique snake_case rule name',
+      // Field `description`s on this object are ADMIN-FACING help text rendered
+      // under each input in Setup — not notes for the next engineer. They must
+      // not name tables, columns, enum values, ADRs or third-party libraries:
+      // the reader is a tenant admin who sees only the labels in the dropdown
+      // (objectstack#3821). Implementation detail belongs in the doc comments
+      // above, where it already is.
+      description: 'Identifies the rule. Lowercase letters, digits and underscores only.',
       group: 'Identity',
     }),
 
@@ -124,20 +130,20 @@ export const SysSharingRule = ObjectSchema.create({
       // instead of a free-text machine-name input. Falls back to a text input
       // when the `field:object-ref` widget is unavailable.
       widget: 'object-ref',
-      description: 'Short object name (e.g. opportunity, account)',
+      description: 'The object whose records this rule shares.',
       group: 'Target',
     }),
 
     criteria_json: Field.textarea({
-      label: 'Criteria (FilterCondition JSON)',
+      label: 'Criteria',
       required: false,
       // Rendered as a visual criteria builder scoped to the selected object's
       // fields (dependsOn: object_name), storing the same JSON FilterCondition.
       // An "Edit as JSON" fallback keeps hand-authored / advanced filters
       // editable. Falls back to a textarea when the widget is unavailable.
       widget: 'filter-condition',
       dependsOn: ['object_name'],
-      description: 'JSON FilterCondition matched against records of object_name. Empty = match all.',
+      description: 'Which records to share. Leave empty to share every record of the object.',
       group: 'Target',
     }),
 
@@ -150,7 +156,9 @@ export const SysSharingRule = ObjectSchema.create({
         label: 'Recipient Type',
         required: true,
         defaultValue: 'business_unit',
-        description: 'Kind of principal that receives access — expanded to user grants at evaluation time. `business_unit` walks the parent_business_unit_id tree; `team` is flat (better-auth); `position` expands the position\'s holders (positions are flat, ADR-0090 D3); `unit_and_subordinates` expands the named business unit PLUS every descendant unit\'s members via the sys_business_unit tree (ADR-0057 D5).',
+        // The engine detail this used to spell out (which tree is walked, which
+        // expansion is flat, the ADRs behind each) lives in the class doc above.
+        description: 'Who receives access. Picking a team, business unit or position gives access to everyone in it. "Business unit and subordinates" also covers every unit below the one you pick.',
         group: 'Recipient',
       },
     ),
@@ -166,7 +174,7 @@ export const SysSharingRule = ObjectSchema.create({
       // back to a text input when the widget is unavailable.
       widget: 'recipient-picker',
       dependsOn: ['recipient_type'],
-      description: 'business-unit id / team id / position name / user id depending on recipient_type',
+      description: 'The specific user, team, business unit or position that receives access.',
       group: 'Recipient',
     }),
 
@@ -184,7 +192,7 @@ export const SysSharingRule = ObjectSchema.create({
       label: 'Active',
       required: false,
       defaultValue: true,
-      description: 'Only active rules participate in lifecycle evaluation',
+      description: 'Turn off to withdraw the access this rule granted, without deleting the rule.',
       group: 'Lifecycle',
     }),
 
@@ -203,8 +211,7 @@ export const SysSharingRule = ObjectSchema.create({
       readonly: true,
       defaultValue: 'admin',
       description:
-        'Record provenance (unified tri-state, A4 #2920): platform = framework built-in / ' +
-        'package = app/package-declared (boot-seeded) / admin = tenant-created in Setup.',
+        'Where this rule came from: built into the platform, installed with an app, or created here in Setup.',
       options: [
         { value: 'platform', label: 'Platform' },
         { value: 'package', label: 'Package' },
@@ -219,8 +226,7 @@ export const SysSharingRule = ObjectSchema.create({
       readonly: true,
       defaultValue: false,
       description:
-        'Set when an admin edits a package-declared rule; boot seeding will no longer ' +
-        'overwrite the row (deactivations survive redeploys). Meaningless on admin rows.',
+        'Set once you edit a rule that came with an app, so your changes are kept when the app is updated.',
       group: 'System',
     }),