fix(sharing,runtime): a sort passed straight to the engine never ordered anything (#4346) (#4370)

Sweeping every in-repo engine call site that still spoke a deprecated alias
turned up three that were not cosmetic.

#4346 made the engine fold `filter`→`where` and `top`→`limit` on all six
methods. The remaining four pairs in RPC_QUERY_ALIAS_SLOTS (select, sort,
skip, populate) fold at the RPC/wire layer only — their values need shape
lowering that belongs to those layers — and a DIRECT engine.find() never
crosses that layer. Three call sites passed `sort` there, so it rode onto the
AST untouched, every driver's `Array.isArray(query.orderBy)` guard declined to
emit an ORDER BY, and the read returned an ordinary-looking, arbitrarily
ordered result:

  share-link-routes.ts        shared AI conversation messages, created_at asc
  runtime/domains/share-links.ts   the same route, runtime-domain copy
  share-link-service.ts       listLinks: "the 200 most recent" share links

Each pairs the dropped sort with a limit — the "latest N" shape whose failure
#4226 spelled out, one layer below the normalizer #4226 fixed. listLinks had
no test at all, which is why it went unnoticed; it is pinned now on the option
bag the engine RECEIVES, not on row order, because the failure is that the key
never becomes `orderBy` and a double honouring either spelling passes either
way. Verified the pin fails against the pre-fix line before keeping it.

The other 27 sites are strict no-ops since #4346 folds `filter`: approvals 5,
auth 2, reports 6, sharing 11, webhooks 2, plus a spec doc example teaching
`filters` (a wire-only alias the engine does not fold at all, so the example
taught a call that matches every row). Renaming them stops the framework
depending on a spelling it asks users to migrate off. Service-level `filter`
PARAMETERS — each service's own public API — are deliberately untouched.

One test double had to move with them: approver-org-scope's fake engine read
`opts.filter`, so it kept passing only because both sides shared the
deprecated dialect. That is the mechanism that let the whole class persist.

Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: os-zhuang

.changeset/engine-callsites-canonical-keys.md

@@ -0,0 +1,52 @@
+---
+"@objectstack/plugin-sharing": patch
+"@objectstack/runtime": patch
+"@objectstack/plugin-approvals": patch
+"@objectstack/plugin-auth": patch
+"@objectstack/plugin-reports": patch
+"@objectstack/plugin-webhooks": patch
+"@objectstack/spec": patch
+---
+
+fix(sharing,runtime): a `sort` passed straight to the engine never ordered anything; migrate every in-repo engine call to canonical QueryAST keys (#4346)
+
+Two changes with different weights, from one sweep of every in-repo engine
+call site that still speaks a deprecated alias.
+
+**The bug — three dropped sorts.** #4346 made the engine fold `filter`→`where`
+and `top`→`limit` on all six methods. The other four pairs in
+`RPC_QUERY_ALIAS_SLOTS` (`select`, `sort`, `skip`, `populate`) are folded at
+the RPC/wire layer only — their values need shape lowering that belongs to
+those layers — and a **direct `engine.find()` never crosses that layer**. Three
+call sites passed `sort` there, so it rode onto the AST untouched, every
+driver's `Array.isArray(query.orderBy)` guard declined to emit an ORDER BY, and
+the query returned an ordinary-looking, arbitrarily-ordered result:
+
+| call site | asked for | actually got |
+|---|---|---|
+| `share-link-routes.ts` | shared AI conversation messages, `created_at asc` | messages in arbitrary order |
+| `runtime/domains/share-links.ts` | same route, runtime-domain copy | same |
+| `share-link-service.ts` `listLinks` | the 200 most recent share links | an arbitrary 200 |
+
+All three combine the dropped sort with a `limit` — the "latest N" shape whose
+failure #4226 spelled out: an unapplied sort returns rows in arbitrary order,
+which `limit` then slices into an arbitrary page. #4226 fixed that in the wire
+normalizer; these calls sit one layer below it. `listLinks` had no test at all,
+which is why it went unnoticed. Now pinned — on the option bag the engine
+receives, not on row order, because the failure is that the key never becomes
+`orderBy` and a fake engine honouring either spelling would pass either way.
+
+**The cleanup — 27 no-op renames.** Every remaining in-repo engine call passing
+`filter` now passes `where` (approvals 5, auth 2, reports 6, sharing 11,
+webhooks 2, plus the one `filters` in a spec doc example). These are strict
+no-ops since #4346 folds the alias — the point is that the framework stops
+depending on a spelling it asks users to migrate off, which is a prerequisite
+for ever retiring the aliases. Service-level `filter` PARAMETERS (each
+service's own public API, e.g. `listRequests(filter)`) are deliberately
+untouched — those are not engine option bags.
+
+Two of the renamed calls were live victims of the #4346 bug rather than
+cosmetic: `auth-manager`'s `stampIdentitySource` read the table's first row via
+`findOne({filter})` and counted the whole table via `count({filter})`, so a
+federated sign-in never stamped `source: 'idp_provisioned'`. #4346 already
+corrected the behaviour; this makes the call say what it means.

packages/plugins/plugin-approvals/src/approval-service.ts

@@ -1053,7 +1053,7 @@ export class ApprovalService implements IApprovalService {
     let rows: any[] = [];
     try {
       rows = await this.engine.find('sys_team_member', {
-        filter: { team_id: teamId },
+        where: { team_id: teamId },
         fields: ['user_id'],
         limit: 10000,
         context: SYSTEM_CTX,
@@ -1096,7 +1096,7 @@ export class ApprovalService implements IApprovalService {
     // Seed sanity check: skip if dept doesn't exist or is inactive within tenant.
     try {
       const seed = await this.engine.find('sys_business_unit', {
-        filter: this.businessUnitOrgScope({ id: businessUnitId }, organizationId),
+        where: this.businessUnitOrgScope({ id: businessUnitId }, organizationId),
         fields: ['id', 'active'],
         limit: 1,
         context: SYSTEM_CTX,
@@ -1125,7 +1125,7 @@ export class ApprovalService implements IApprovalService {
     let rows: any[] = [];
     try {
       rows = await this.engine.find('sys_business_unit_member', {
-        filter: { business_unit_id: { $in: Array.from(seen) } },
+        where: { business_unit_id: { $in: Array.from(seen) } },
         fields: ['user_id'],
         limit: 10000,
         context: SYSTEM_CTX,
@@ -1184,7 +1184,7 @@ export class ApprovalService implements IApprovalService {
   private async lookupManager(userId: string): Promise<string | null> {
     try {
       const rows = await this.engine.find('sys_user', {
-        filter: { id: userId }, fields: ['id', 'manager_id'], limit: 1, context: SYSTEM_CTX,
+        where: { id: userId }, fields: ['id', 'manager_id'], limit: 1, context: SYSTEM_CTX,
       } as any);
       const row: any = Array.isArray(rows) ? rows[0] : null;
       return row?.manager_id ? String(row.manager_id) : null;
@@ -1240,7 +1240,7 @@ export class ApprovalService implements IApprovalService {
     let rows: any[] = [];
     try {
       rows = await this.engine.find('sys_approval_delegation', {
-        filter: { delegator_id: delegatorId },
+        where: { delegator_id: delegatorId },
         fields: ['id', 'delegator_id', 'delegate_id', 'valid_from', 'valid_until', 'reason', 'organization_id'],
         limit: 50,
         context: SYSTEM_CTX,

packages/plugins/plugin-approvals/src/approver-org-scope.test.ts

@@ -35,7 +35,11 @@ function makeDeps(over: Partial<ApproverOrgScopeDeps> & { members?: Array<{ user
   const members = over.members ?? [];
   const engine = {
     find: vi.fn(async (object: string, opts: any) => {
-      const f = opts?.filter ?? {};
+      // Reads the CANONICAL key, like the engine's own post-fold AST. This
+      // double used to read `opts.filter`, which is why it kept passing while
+      // production spoke the deprecated spelling: both sides shared one
+      // dialect, so nothing forced the migration (#4346).
+      const f = opts?.where ?? {};
       if (object === 'sys_organization') {
         const rows = Object.values(ORGS).filter((o) =>
           (f.id === undefined || o.id === f.id) && (f.slug === undefined || o.slug === f.slug));
@@ -151,7 +155,7 @@ describe('resolveApproverDirectoryOrg — the guards', () => {
     const deps = makeDeps();
     (deps.engine.find as any) = vi.fn(async (object: string, opts: any) => {
       if (object !== 'sys_organization') return [];
-      const id = opts?.filter?.id;
+      const id = opts?.where?.id;
       // a → b → a
       if (id === 'a') return [{ id: 'a', slug: 'a', parent_organization_id: 'b' }];
       if (id === 'b') return [{ id: 'b', slug: 'b', parent_organization_id: 'a' }];

packages/plugins/plugin-approvals/src/approver-org-scope.ts

@@ -62,7 +62,7 @@ async function findOrg(
 ): Promise<any | null> {
   try {
     const rows = await engine.find('sys_organization', {
-      filter: where,
+      where,
       fields: ['id', 'slug', 'parent_organization_id'],
       limit: 1,
       context: SYSTEM_CTX,
@@ -230,7 +230,7 @@ export async function filterApproversWhoCanRead(
   let members: any[] = [];
   try {
     members = await deps.engine.find('sys_member', {
-      filter: { organization_id: requestOrg, user_id: { $in: userIds } },
+      where: { organization_id: requestOrg, user_id: { $in: userIds } },
       fields: ['user_id'],
       limit: 10000,
       context: SYSTEM_CTX,

packages/plugins/plugin-auth/src/auth-manager.ts

@@ -3481,7 +3481,7 @@ export class AuthManager {
         // Gained a local password → env-native. Only write if currently
         // managed (avoids a no-op history row on every local signup).
         const u = await engine.findOne('sys_user', {
-          filter: { id: userId }, fields: ['id', 'source'], context: SYSTEM_CTX,
+          where: { id: userId }, fields: ['id', 'source'], context: SYSTEM_CTX,
         } as any);
         if (u && u.source === 'idp_provisioned') {
           await engine.update('sys_user', { id: userId, source: 'env_native' }, { context: SYSTEM_CTX } as any);
@@ -3491,7 +3491,7 @@ export class AuthManager {
 
       // Federated link → managed, unless a local credential already exists.
       const credentialCount = await engine.count('sys_account', {
-        filter: { user_id: userId, provider_id: 'credential' },
+        where: { user_id: userId, provider_id: 'credential' },
         context: SYSTEM_CTX,
       } as any);
       if (typeof credentialCount === 'number' && credentialCount > 0) return;

packages/plugins/plugin-reports/src/report-service.ts

@@ -307,7 +307,7 @@ export class ReportService implements IReportService {
   /** Raw metadata read of a saved report by id (no authz — callers gate). */
   private async loadReportRow(reportId: string): Promise<any | null> {
     const rows = await this.engine.find('sys_saved_report', {
-      filter: { id: reportId }, limit: 1, context: SYSTEM_CTX,
+      where: { id: reportId }, limit: 1, context: SYSTEM_CTX,
     });
     return Array.isArray(rows) && rows[0] ? rows[0] : null;
   }
@@ -374,7 +374,7 @@ export class ReportService implements IReportService {
       f.owner_id = context.userId;
     }
     const rows = await this.engine.find('sys_saved_report', {
-      filter: f, limit: 500, orderBy: [{ field: 'updated_at', order: 'desc' }], context: SYSTEM_CTX,
+      where: f, limit: 500, orderBy: [{ field: 'updated_at', order: 'desc' }], context: SYSTEM_CTX,
     });
     return Array.isArray(rows) ? rows.map(rowFromSaved) : [];
   }
@@ -397,7 +397,7 @@ export class ReportService implements IReportService {
     }
     // Cascade — drop attached schedules first.
     const schedules = await this.engine.find('sys_report_schedule', {
-      filter: { report_id: reportId }, limit: 500, context: SYSTEM_CTX,
+      where: { report_id: reportId }, limit: 500, context: SYSTEM_CTX,
     });
     for (const s of (schedules ?? [])) {
       await this.engine.delete('sys_report_schedule', { where: { id: (s as any).id }, context: SYSTEM_CTX });
@@ -437,7 +437,7 @@ export class ReportService implements IReportService {
     const q = report.query ?? {};
     const limit = Math.min(q.limit ?? DEFAULT_LIMIT, this.maxRows);
     const rows = await this.engine.find(report.object_name, {
-      filter: q.filter,
+      where: q.filter,
       fields: q.fields,
       orderBy: q.orderBy,
       limit,
@@ -544,7 +544,7 @@ export class ReportService implements IReportService {
     const f: any = {};
     if (filter?.reportId) f.report_id = filter.reportId;
     const rows = await this.engine.find('sys_report_schedule', {
-      filter: f, limit: 500, orderBy: [{ field: 'next_run_at', order: 'asc' }], context: SYSTEM_CTX,
+      where: f, limit: 500, orderBy: [{ field: 'next_run_at', order: 'asc' }], context: SYSTEM_CTX,
     });
     return Array.isArray(rows) ? rows.map(rowFromSchedule) : [];
   }
@@ -554,7 +554,7 @@ export class ReportService implements IReportService {
   async dispatchDue(now?: Date): Promise<{ fired: number; failed: number; skipped: number }> {
     const ts = (now ?? this.clock.now()).toISOString();
     const due = await this.engine.find('sys_report_schedule', {
-      filter: { active: true },
+      where: { active: true },
       limit: 200,
       context: SYSTEM_CTX,
     });

packages/plugins/plugin-sharing/src/share-link-routes.ts

@@ -301,7 +301,7 @@ export function registerShareLinkRoutes(
       const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const;
       const rows = await engine.find('ai_messages', {
         where: { conversation_id: resolved.link.record_id },
-        sort: [{ field: 'created_at', order: 'asc' }],
+        orderBy: [{ field: 'created_at', order: 'asc' }],
         limit: 500,
         context: SYSTEM_CTX,
       } as any);

packages/plugins/plugin-sharing/src/share-link-service.test.ts

@@ -89,6 +89,33 @@ describe('ShareLinkService', () => {
     expect(engine._tables.sys_share_link).toHaveLength(1);
   });
 
+  // [#4346 follow-up] `listLinks` asked for "the 200 most recent links" with a
+  // `sort` key. The ENGINE folds only `where`/`filter` and `limit`/`top`
+  // (`RPC_QUERY_ALIAS_SLOTS`); `sort`→`orderBy` is folded at the RPC/wire layer,
+  // which a direct `engine.find()` never crosses. So `sort` rode onto the AST
+  // untouched, every driver's `Array.isArray(query.orderBy)` guard declined to
+  // emit an ORDER BY, and the "most recent 200" was an ARBITRARY 200 — with a
+  // perfectly ordinary-looking result over it (the #4226 failure mode, one
+  // layer below the normalizer #4226 fixed).
+  //
+  // Asserted on the OPTION BAG rather than on row order, because the failure is
+  // that the key never becomes `orderBy` — a fake engine that sorts by either
+  // spelling would pass while the real one drops it.
+  it('listLinks asks the engine for its ordering under the canonical key', async () => {
+    const seen: any[] = [];
+    const recording = {
+      ...engine,
+      async find(object: string, options?: any) { seen.push(options); return engine.find(object, options); },
+    };
+    const svc = new ShareLinkService({ engine: recording as any });
+    await svc.listLinks({}, { isSystem: true });
+
+    expect(seen).toHaveLength(1);
+    expect(seen[0].orderBy, 'a `sort` key here is silently dropped by every driver')
+      .toEqual([{ field: 'created_at', order: 'desc' }]);
+    expect('sort' in seen[0], 'the deprecated spelling must not ride along').toBe(false);
+  });
+
   it('rejects objects that did not opt in', async () => {
     await expect(
       service.createLink(

packages/plugins/plugin-sharing/src/share-link-service.ts

@@ -358,7 +358,7 @@ export class ShareLinkService implements IShareLinkService {
     const rows = await this.engine.find('sys_share_link', {
       where,
       limit: 200,
-      sort: [{ field: 'created_at', order: 'desc' }],
+      orderBy: [{ field: 'created_at', order: 'desc' }],
       context: context.isSystem ? SYSTEM_CTX : context,
     } as any);
     return Array.isArray(rows) ? (rows as ShareLink[]) : [];

packages/plugins/plugin-sharing/src/sharing-rule-provenance.ts

@@ -57,7 +57,7 @@ export function bindRuleProvenanceStamp(engine: MinimalEngine, logger?: MinimalL
         // current row ourselves (system ctx: this is a provenance check, not
         // an authorization decision).
         const rows = await engine.find('sys_sharing_rule', {
-          filter: { id },
+          where: { id },
           fields: ['id', 'managed_by', 'customized'],
           limit: 1,
           context: SYSTEM_CTX,