fix(rest): import-runner builds the canonical QueryAST through a typed findData envelope

.changeset/import-runner-canonical-query-ast.md

@@ -0,0 +1,13 @@
+---
+"@objectstack/rest": minor
+---
+
+`import-runner.ts` builds its three server-side `findData` requests in the CANONICAL QueryAST, and the helper that carried them is typed against the declared contract instead of `any`.
+
+`FindDataRequestSchema` declares `query: QuerySchema.optional()`, and `QuerySchema` declares `where` / `limit` / `offset` / `fields` / `orderBy` / `expand` — it declares neither `$filter` nor `$top`. The normalizer's own table calls those two "the wire-only spellings no schema declares". The reference resolver, the duplicate probe and the id recheck each built a literal in that undeclared dialect, and nothing reddened because the helper they went through took `query: any`: the literals were type-checked by nothing at all, so the undeclared keys cost no diagnostic. Reverting one of them to `$filter` now costs `TS2353 … '$filter' does not exist in type 'QueryInput'`; on the pre-change file the identical revert cost zero errors.
+
+- **The three literals.** `$filter` → `where`, `$top` → `limit`, plus the `object` the declared query requires. No behaviour change on the two `rest-server.ts` call paths (`POST /data/:object/import` and the async import-job worker), which hand `runImport` the real `ObjectStackProtocolImplementation`: that normalizer folds `$filter` onto `where` and `$top` onto `limit` by the spec's own `RPC_QUERY_ALIAS_SLOTS`, moving the value verbatim, so both dialects reach `engine.find` as the same option bag.
+- **The erasure vehicle.** `findArgsBase` now takes a `FindDataRequest` rather than a bare `any` query, so the request-level `object` is compiled too and the `object: ''` placeholder every caller had to override is gone. This is the durable half: rewriting the literals while leaving the parameter `any` would leave the next author in this file with no diagnostic at all.
+- **The pin.** `rest-server-canonical-query-ast.test.ts` censuses the PACKAGE rather than one file. `import-runner.ts` has no HTTP door — every query in it is server-built — so its census rejects a wire spelling anywhere in the file, not only inside a `query:` slot. That whole-file rule is the one that finds this class: these three literals were arguments to a helper and were never in a `query:` slot to begin with.
+
+⚠️ Implementor-visible: `ImportProtocolLike` is exported, its `findData(args: any)` never declared which dialect the runner sends, and the runner now sends the canonical one. An implementation that reads `args.query.$filter` / `args.query.$top` directly — rather than through the protocol normalizer — receives `undefined` and must be updated to read `where` / `limit`.

.changeset/plugin-auth-admin-import-canonical-query.md

@@ -0,0 +1,12 @@
+---
+"@objectstack/plugin-auth": patch
+---
+
+`runAdminImportUsers`'s hand-written `ImportProtocolLike` reads the CANONICAL QueryAST (`where` / `limit`) — the payload `@objectstack/rest`'s import runner sends as of this same release — instead of the wire-only `$filter` / `$top`.
+
+`POST /api/v1/auth/admin/import-users` reuses the shared import runner but swaps in an identity-specific protocol, because an identity write is `auth.api.createUser` and not an engine insert. That protocol is hand-written, so it never passes through `ObjectStackProtocolImplementation` — the normalizer that folds `$filter` onto `where` and `$top` onto `limit` for a caller arriving off the HTTP door. It has to read the canonical keys itself.
+
+- **A mismatch here does not produce a missing filter, it produces an unbounded one.** `const where = args?.query?.$filter ?? {}` turns an unread key into an empty filter, and an empty filter constrains nothing: the upsert duplicate probe stops discriminating, `findExisting` matches rows it was given no key for, and an admin import updates the WRONG user. Both halves are measured in `admin-import-users.test.ts` — the email-match case reported `updated: 2` where one of the two rows was new, and the phone-match case sent a probe carrying no `where` at all.
+- **One dialect, and no default behind it.** The two reads are now `args.query.where` and `args.query.limit`, with no `??`. A default here would not be tolerance for an older caller — this handle is fed by the runner, never off the wire — it is precisely the lenient fallback that converts a spelling mismatch into a silent match-everything. A request that arrives without a `query` now costs a loud `TypeError` instead.
+
+⚠️ No published version shipped the mismatch. The runner's rewrite and this adapter land in the same release, and `@objectstack/plugin-auth` depends on `@objectstack/rest` at an exact workspace version, so the two cannot be installed apart. What this entry records is why they move together — and what the same mismatch costs any OTHER hand-written `ImportProtocolLike`, which the `@objectstack/rest` entry calls out for implementors.

packages/plugins/plugin-auth/src/admin-import-users.ts

@@ -349,10 +349,23 @@ export async function runAdminImportUsers(
   }
 
   const protocol: ImportProtocolLike = {
-    // findExisting path: `{ $filter, $top }` against sys_user.
+    // [#16638] findExisting path: the CANONICAL QueryAST against sys_user.
+    // `runImport` hands this handle a `FindDataRequest`, and the `QuerySchema`
+    // that request declares carries `where` / `limit` — it declares neither
+    // `$filter` nor `$top`. Those two are wire-only spellings the protocol
+    // normalizer folds for a caller off the HTTP door; this protocol is
+    // hand-written and never passes through that normalizer, so it must read
+    // the canonical keys itself.
+    //
+    // Two dialects would be the lenient `??` alias Prime Directive #12 forbids,
+    // and the default is not a harmless belt either: a `where` that falls back
+    // to `{}` stops constraining anything, so the duplicate probe matches rows
+    // it was given no key for and the upsert updates the WRONG user. One
+    // dialect, read straight — a request that arrives without a `query` is a
+    // caller defect and costs a loud TypeError, not a silent match-everything.
     async findData(args: any) {
-      const where = args?.query?.$filter ?? {};
-      const limit = args?.query?.$top ?? 2;
+      const where = args.query.where;
+      const limit = args.query.limit;
       return engine.find(args.object, { where, limit, context: SYSTEM_CTX } as any);
     },
 

packages/rest/src/import-runner-bulk.test.ts

@@ -148,8 +148,9 @@ describe('runImport — bulk create batching (framework#2678)', () => {
     }));
     const updateData = vi.fn(async (args: { id: string }) => ({ id: args.id }));
     // Row 1 ('existing') matches an existing record → update; the rest are creates.
-    const findData = vi.fn(async (args: { query?: { $filter?: { name?: string } } }) =>
-      (args.query?.$filter?.name === 'existing' ? [{ id: 'existing_id', name: 'existing' }] : []));
+    // [#16638] Reads the CANONICAL `where` the runner sends, not `$filter`.
+    const findData = vi.fn(async (args: { query: { where: { name?: string } } }) =>
+      (args.query.where.name === 'existing' ? [{ id: 'existing_id', name: 'existing' }] : []));
     const p: ImportProtocolLike = { findData, createData: vi.fn(), updateData, createManyData };
 
     const summary = await runImport({

packages/rest/src/import-runner-idempotency.test.ts

@@ -49,8 +49,21 @@ function makeProtocol(opts: { firstCall?: 'throw' | 'shortReturn' } = {}) {
     store.push(rec);
     return rec;
   });
-  const findData = vi.fn(async (args: { query?: { $filter?: Record<string, any> } }) => {
-    const filter = args.query?.$filter ?? {};
+  // [#16638] Reads the CANONICAL `where` the runner sends. ⛔ The `?? {}` this
+  // replaces is what made this whole file pass VACUOUSLY once the runner moved
+  // to `where`: with `$filter` undefined every recheck degraded to `{}`, which
+  // constrains nothing, so the recheck matched the entire store and the
+  // no-duplicate assertions below held without the probe discriminating at all.
+  // Reading `where` straight means an absent filter throws instead.
+  /**
+   * [#16638] Every filter this double actually APPLIED. Pinning the payload
+   * alone would still pass over a double that read the wrong key and defaulted
+   * to `{}` — which is the vacuity being closed here, so both are recorded.
+   */
+  const appliedFilters: Array<Record<string, any>> = [];
+  const findData = vi.fn(async (args: { query: { where: Record<string, any>; limit?: number } }) => {
+    const filter = args.query.where;
+    appliedFilters.push(filter);
     // Supports equality and { $in: [...] } — the id recheck (framework#3173)
     // queries by pre-assigned id $in, like the real SQL driver does.
     return store.filter((row) => Object.entries(filter).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); 
@@ -59,12 +72,45 @@ function makeProtocol(opts: { firstCall?: 'throw' | 'shortReturn' } = {}) {
     }));
   });
   const p: ImportProtocolLike = { findData, createData, updateData: vi.fn(), createManyData };
-  return { p, store, createManyData, createData };
+  return { p, store, createManyData, createData, findData, appliedFilters };
+}
+
+/** One recorded `findData` probe, as the double above receives it. */
+type FindProbe = { query: { where: Record<string, any>; limit?: number } };
+
+/**
+ * ⭐ [#16638] Every probe the runner sends must NARROW — the assertion this
+ * file was missing, and the reason it stayed GREEN through a payload rewrite
+ * that broke it. While the double read `args.query.$filter`, a runner sending
+ * `where` left that read `undefined`, the `?? {}` default turned it into an
+ * empty filter, and an empty filter constrains NOTHING: every recheck matched
+ * the entire store, so each `store` / `created` expectation below held without
+ * the probe discriminating between one row and any other. Passing was not
+ * evidence. `{}` is the shape that has to be refused, so it is asserted
+ * against directly.
+ *
+ * `Object.keys` on an ABSENT `where` throws rather than reporting zero keys,
+ * and that is deliberate: a spelling drift must be loud here, not degrade into
+ * a probe that matches everything.
+ */
+function expectEveryProbeNarrowed(
+  calls: ReadonlyArray<readonly [FindProbe]>,
+  appliedFilters: ReadonlyArray<Record<string, any>>,
+): void {
+  expect(calls.length).toBeGreaterThan(0);
+  for (const [args] of calls) {
+    expect(Object.keys(args.query.where)).not.toHaveLength(0);
+  }
+  // The payload half is the drift alarm; this is the vacuity half. The filter
+  // the double APPLIED must be the one it was handed — an equality a `?? {}`
+  // default breaks even while the runner's payload stays perfectly canonical.
+  expect(appliedFilters).toEqual(calls.map(([args]) => args.query.where));
+  for (const filter of appliedFilters) expect(Object.keys(filter)).not.toHaveLength(0);
 }
 
 describe('runImport — idempotent retry with natural keys (framework#3149)', () => {
   it('upsert+matchFields: a transient retry after commit does not duplicate rows', async () => {
-    const { p, store, createManyData } = makeProtocol({ firstCall: 'throw' });
+    const { p, store, createManyData, findData, appliedFilters } = makeProtocol({ firstCall: 'throw' });
 
     const summary = await runImport({
       ...baseOpts, p, writeMode: 'upsert', matchFields: ['name'],
@@ -79,6 +125,10 @@ describe('runImport — idempotent retry with natural keys (framework#3149)', ()
     expect(store).toHaveLength(2); // no duplicates
     expect(summary.created).toBe(2);
     expect(summary.errors).toBe(0);
+    // ⭐ [#16638] …and every probe that produced those numbers actually
+    // constrained something. The natural-key probes carry the match field.
+    expectEveryProbeNarrowed(findData.mock.calls, appliedFilters);
+    expect(findData.mock.calls.map(([a]) => Object.keys(a.query.where))).toContainEqual(['name']);
   });
 
   it('upsert+matchFields: a short createManyData return degrades and still does not duplicate', async () => {
@@ -97,7 +147,7 @@ describe('runImport — idempotent retry with natural keys (framework#3149)', ()
   });
 
   it('pure insert (no matchFields): pre-assigned ids make the retry exactly-once too (#3173)', async () => {
-    const { p, store, createManyData } = makeProtocol({ firstCall: 'throw' });
+    const { p, store, createManyData, findData, appliedFilters } = makeProtocol({ firstCall: 'throw' });
 
     const summary = await runImport({
       ...baseOpts, p, writeMode: 'insert', matchFields: [],
@@ -110,6 +160,18 @@ describe('runImport — idempotent retry with natural keys (framework#3149)', ()
     expect(store).toHaveLength(2);
     expect(summary.created).toBe(2);
     expect(summary.errors).toBe(0);
+
+    // ⭐ [#16638] The recheck is the whole mechanism of #3173, so pin the
+    // payload it was handed rather than only the outcome: `id: { $in: [...] }`
+    // over exactly the ids the runner pre-assigned, bounded to that many rows.
+    // Read `where` / `limit`, the keys `FindDataRequest` declares — a drift
+    // back to `$filter` / `$top` reddens here before it reaches an implementor.
+    expectEveryProbeNarrowed(findData.mock.calls, appliedFilters);
+    const probes = findData.mock.calls.map(([a]) => a.query);
+    expect(probes).toHaveLength(1);
+    expect(Object.keys(probes[0].where)).toEqual(['id']);
+    expect([...probes[0].where.id.$in].sort()).toEqual(store.map((r) => r.id).sort());
+    expect(probes[0].limit).toBe(store.length);
   });
 
   it('pure insert: legitimate duplicate rows survive the retry intact (each copy has its own id) (#3173)', async () => {

packages/rest/src/import-runner-selfref.test.ts

@@ -42,8 +42,10 @@ function makeProtocol(seed: Array<Record<string, any>> = []) {
       return rec;
     }),
   }));
-  const findData = vi.fn(async (args: { query?: { $filter?: Record<string, any> } }) => {
-    const filter = args.query?.$filter ?? {};
+  // [#16638] Reads the CANONICAL `where` the runner sends. ⛔ No `?? {}`: an
+  // absent filter must throw here, never degrade into a match-everything probe.
+  const findData = vi.fn(async (args: { query: { where: Record<string, any> } }) => {
+    const filter = args.query.where;
     return store.filter((row) => Object.entries(filter).every(([k, v]) => { if (k.startsWith('$')) throw new Error(`fake driver: unsupported operator ${k}`); return row[k] === v; }));
   });
   const p: ImportProtocolLike = { findData, createData: vi.fn(), updateData: vi.fn(), createManyData };

packages/rest/src/import-runner.ts

@@ -4,7 +4,7 @@ import { randomUUID } from 'node:crypto';
 import { coerceRow, type RefResolver, type RefMatch } from './import-coerce.js';
 import type { ExportFieldMeta } from './export-format.js';
 import type { ValidationMessageTranslator } from '@objectstack/spec/system';
-import type { ValidateDataIssue, ValidateDataRequest, ValidateDataResponse } from '@objectstack/spec/api';
+import type { FindDataRequest, ValidateDataIssue, ValidateDataRequest, ValidateDataResponse } from '@objectstack/spec/api';
 import { bulkWrite, withTransientRetry, defaultIsTransientError, type BulkWriteRowResult } from '@objectstack/core';
 import { isUniqueViolationError, uniqueViolationColumn } from '@objectstack/types';
 import { isEngineDuplicateRecordEnvelope } from './error-response.js';
@@ -357,9 +357,29 @@ export function runImport(opts: RunImportOptions): Promise<ImportRunSummary> {
       : Array.isArray(r?.data) ? r.data
         : Array.isArray(r?.rows) ? r.rows
           : Array.isArray(r) ? r : [];
-  const findArgsBase = (query: any) => ({
-    object: '',
-    query,
+  // [#16638] The server-scoped envelope for this file's `findData` calls, and
+  // the reason its parameter is TYPED. It used to be `query: any`, so the three
+  // call sites below were type-checked by nothing at all and their undeclared
+  // `$filter` / `$top` wire spellings cost no diagnostic — the same erasure
+  // #16337 found on `loadImportJob`'s `p: any` handle in `rest-server.ts`,
+  // whose signpost prescribes exactly this rewrite. Compiled against
+  // `FindDataRequest` every member is now held to the contract
+  // `FindDataRequestSchema` declares (`QuerySchema`: `where` / `limit` /
+  // `offset` / `fields` / `orderBy` / `expand`), so a wire alias is a compile
+  // error at the call site instead of a payload no schema has seen. It also
+  // retires the `object: ''` placeholder every caller had to override.
+  //
+  // ⚠️ The rewrite is a SPELLING change only: `@objectstack/metadata-protocol`
+  // folds `$filter`→`where` and `$top`→`limit` by the spec's own
+  // `RPC_QUERY_ALIAS_SLOTS` with the value moved verbatim, so all three calls
+  // reach `engine.find` with the same option bag as before —
+  // `rest-server-canonical-query-ast.test.ts` §3 measures that pair by pair.
+  //
+  // ⛔ Server-built means server-built: the wire aliases stay accepted at the
+  // HTTP door for CALLERS. Declaring them there is #16066's spec half and is
+  // not this file's business.
+  const findArgsBase = (request: FindDataRequest) => ({
+    ...request,
     ...(environmentId ? { environmentId } : {}),
     ...(context ? { context } : {}),
   });
@@ -385,10 +405,10 @@ export function runImport(opts: RunImportOptions): Promise<ImportRunSummary> {
       let match: RefMatch = {};
       for (const f of candidates) {
         try {
-          const r = await p.findData({
-            ...findArgsBase({ $filter: { [f]: display }, $top: 2 }),
+          const r = await p.findData(findArgsBase({
             object: referenceObject,
-          });
+            query: { object: referenceObject, where: { [f]: display }, limit: 2 },
+          }));
           const recs = findRows(r);
           if (recs.length === 0) continue;
           if (recs.length > 1) { match = { ambiguous: true, matchedField: f }; break; }
@@ -428,7 +448,10 @@ export function runImport(opts: RunImportOptions): Promise<ImportRunSummary> {
       if (v === undefined || v === null || v === '') return 'blank';
       filter[f] = v;
     }
-    const r = await p.findData({ ...findArgsBase({ $filter: filter, $top: 2 }), object: objectName });
+    const r = await p.findData(findArgsBase({
+      object: objectName,
+      query: { object: objectName, where: filter, limit: 2 },
+    }));
     const recs = findRows(r);
     if (recs.length === 0) return 'none';
     if (recs.length > 1) return 'ambiguous';
@@ -548,10 +571,10 @@ export function runImport(opts: RunImportOptions): Promise<ImportRunSummary> {
   const recheckByIds = async (chunk: Array<Record<string, any>>): Promise<Map<string, any>> => {
     const ids = chunk.map((r) => r.id).filter((v) => v != null && v !== '');
     if (ids.length === 0) return new Map();
-    const r = await p.findData({
-      ...findArgsBase({ $filter: { id: { $in: ids } }, $top: ids.length }),
+    const r = await p.findData(findArgsBase({
       object: objectName,
-    });
+      query: { object: objectName, where: { id: { $in: ids } }, limit: ids.length },
+    }));
     return new Map(findRows(r).map((rec: any) => [String(rec.id), rec]));
   };
   const flushPendingCreates = async (): Promise<void> => {