fix: address all 5 code review findings — RPC backward compat, remove unreachable branches, fix expand normalization

Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/aae1804c-cfe4-4529-af70-e59031349057

Author: Copilot

content/docs/references/data/data-engine.mdx

@@ -68,7 +68,7 @@ Options for DataEngine.aggregate operations
 | :--- | :--- | :--- | :--- |
 | **method** | `string` | ✅ |  |
 | **object** | `string` | ✅ |  |
-| **query** | `Object` | ✅ | QueryAST-aligned options for DataEngine.aggregate operations |
+| **query** | `Object` | ✅ |  |
 
 
 ---
@@ -108,7 +108,7 @@ Options for DataEngine.count operations
 | :--- | :--- | :--- | :--- |
 | **method** | `string` | ✅ |  |
 | **object** | `string` | ✅ |  |
-| **query** | `Object` | optional | QueryAST-aligned options for DataEngine.count operations |
+| **query** | `Object` | optional |  |
 
 
 ---
@@ -137,7 +137,7 @@ Options for DataEngine.delete operations
 | **method** | `string` | ✅ |  |
 | **object** | `string` | ✅ |  |
 | **id** | `string \| number` | optional | ID for single delete, or use where in options |
-| **options** | `Object` | optional | QueryAST-aligned options for DataEngine.delete operations |
+| **options** | `Object` | optional |  |
 
 
 ---
@@ -186,7 +186,7 @@ Reference: [__schema0](./__schema0)
 | :--- | :--- | :--- | :--- |
 | **method** | `string` | ✅ |  |
 | **object** | `string` | ✅ |  |
-| **query** | `Object` | optional | QueryAST-aligned query options for IDataEngine.find() operations |
+| **query** | `Object` | optional |  |
 
 
 ---
@@ -199,7 +199,7 @@ Reference: [__schema0](./__schema0)
 | :--- | :--- | :--- | :--- |
 | **method** | `string` | ✅ |  |
 | **object** | `string` | ✅ |  |
-| **query** | `Object` | optional | QueryAST-aligned query options for IDataEngine.find() operations |
+| **query** | `Object` | optional |  |
 
 
 ---
@@ -268,7 +268,7 @@ This schema accepts one of the following structures:
 | :--- | :--- | :--- | :--- |
 | **method** | `string` | ✅ |  |
 | **object** | `string` | ✅ |  |
-| **query** | `Object` | optional | QueryAST-aligned query options for IDataEngine.find() operations |
+| **query** | `Object` | optional |  |
 
 ---
 
@@ -280,7 +280,7 @@ This schema accepts one of the following structures:
 | :--- | :--- | :--- | :--- |
 | **method** | `string` | ✅ |  |
 | **object** | `string` | ✅ |  |
-| **query** | `Object` | optional | QueryAST-aligned query options for IDataEngine.find() operations |
+| **query** | `Object` | optional |  |
 
 ---
 
@@ -307,7 +307,7 @@ This schema accepts one of the following structures:
 | **object** | `string` | ✅ |  |
 | **data** | `Record<string, any>` | ✅ |  |
 | **id** | `string \| number` | optional | ID for single update, or use where in options |
-| **options** | `Object` | optional | QueryAST-aligned options for DataEngine.update operations |
+| **options** | `Object` | optional |  |
 
 ---
 
@@ -320,7 +320,7 @@ This schema accepts one of the following structures:
 | **method** | `string` | ✅ |  |
 | **object** | `string` | ✅ |  |
 | **id** | `string \| number` | optional | ID for single delete, or use where in options |
-| **options** | `Object` | optional | QueryAST-aligned options for DataEngine.delete operations |
+| **options** | `Object` | optional |  |
 
 ---
 
@@ -332,7 +332,7 @@ This schema accepts one of the following structures:
 | :--- | :--- | :--- | :--- |
 | **method** | `string` | ✅ |  |
 | **object** | `string` | ✅ |  |
-| **query** | `Object` | optional | QueryAST-aligned options for DataEngine.count operations |
+| **query** | `Object` | optional |  |
 
 ---
 
@@ -344,7 +344,7 @@ This schema accepts one of the following structures:
 | :--- | :--- | :--- | :--- |
 | **method** | `string` | ✅ |  |
 | **object** | `string` | ✅ |  |
-| **query** | `Object` | ✅ | QueryAST-aligned options for DataEngine.aggregate operations |
+| **query** | `Object` | ✅ |  |
 
 ---
 
@@ -447,7 +447,7 @@ Options for DataEngine.update operations
 | **object** | `string` | ✅ |  |
 | **data** | `Record<string, any>` | ✅ |  |
 | **id** | `string \| number` | optional | ID for single update, or use where in options |
-| **options** | `Object` | optional | QueryAST-aligned options for DataEngine.update operations |
+| **options** | `Object` | optional |  |
 
 
 ---

packages/objectql/src/engine.ts

@@ -890,9 +890,8 @@ export class ObjectQL implements IDataEngine {
 
      // 1. Extract ID from data or where if it's a single update by ID
      let id = data.id;
-     if (!id && options?.where) {
-         if (typeof options.where === 'string') id = options.where;
-         else if (options.where.id) id = options.where.id;
+     if (!id && options?.where && typeof options.where === 'object' && 'id' in options.where) {
+         id = (options.where as Record<string, unknown>).id;
      }
 
      const opCtx: OperationContext = {
@@ -945,9 +944,8 @@ export class ObjectQL implements IDataEngine {
 
     // Extract ID logic similar to update
     let id: any = undefined;
-    if (options?.where) {
-         if (typeof options.where === 'string') id = options.where;
-         else if (options.where.id) id = options.where.id;
+    if (options?.where && typeof options.where === 'object' && 'id' in options.where) {
+        id = (options.where as Record<string, unknown>).id;
     }
 
     const opCtx: OperationContext = {

packages/objectql/src/protocol-data.test.ts

@@ -78,13 +78,12 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => {
                 query: { populate: ['assignee'], expand: 'project' },
             });
 
-            // populate names take precedence, but the pre-existing expand string
-            // prevents the Record from being created (edge case)
+            // populate names take precedence; the non-object expand string is
+            // cleaned up first, then populate-derived names create the Record.
             const callArgs = mockEngine.find.mock.calls[0][1];
             expect(callArgs.populate).toBeUndefined();
             expect(callArgs.$expand).toBeUndefined();
-            // The expand string blocks Record creation then gets cleaned up as non-object
-            expect(callArgs.expand).toBeUndefined();
+            expect(callArgs.expand).toEqual({ assignee: { object: 'assignee' } });
         });
 
         it('should pass expand Record object through as-is', async () => {

packages/objectql/src/protocol.ts

@@ -357,14 +357,18 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
         }
         delete options.populate;
         delete options.$expand;
+        // Clean up non-object expand (e.g. string) BEFORE the Record conversion
+        // below, so that populate-derived names can create the expand Record even
+        // when a legacy string expand was also present.
+        if (typeof options.expand !== 'object' || options.expand === null) {
+            delete options.expand;
+        }
         // Only set expand if not already an object (advanced usage)
         if (expandNames.length > 0 && !options.expand) {
             options.expand = {} as Record<string, any>;
             for (const rel of expandNames) {
                 options.expand[rel] = { object: rel };
             }
-        } else if (typeof options.expand !== 'object' || options.expand === null) {
-            delete options.expand;
         }
 
         // Boolean fields

packages/spec/src/data/data-engine.test.ts

@@ -457,6 +457,25 @@ describe('DataEngineFindRequestSchema', () => {
 
     expect(request.query?.where).toBeDefined();
   });
+
+  it('should accept find with legacy params (backward compat)', () => {
+    const request = DataEngineFindRequestSchema.parse({
+      method: 'find',
+      object: 'account',
+      query: {
+        filter: { status: 'active' },
+        select: ['id', 'name'],
+        sort: [{ field: 'name', order: 'asc' }],
+        skip: 10,
+        populate: ['owner'],
+      },
+    });
+
+    expect(request.query?.filter).toEqual({ status: 'active' });
+    expect(request.query?.select).toEqual(['id', 'name']);
+    expect(request.query?.skip).toBe(10);
+    expect(request.query?.populate).toEqual(['owner']);
+  });
 });
 
 describe('DataEngineFindOneRequestSchema', () => {
@@ -471,6 +490,19 @@ describe('DataEngineFindOneRequestSchema', () => {
 
     expect(request.method).toBe('findOne');
   });
+
+  it('should accept find one with legacy params (backward compat)', () => {
+    const request = DataEngineFindOneRequestSchema.parse({
+      method: 'findOne',
+      object: 'account',
+      query: {
+        filter: { id: '123' },
+        select: ['id', 'name'],
+      },
+    });
+
+    expect(request.query?.filter).toEqual({ id: '123' });
+  });
 });
 
 describe('DataEngineInsertRequestSchema', () => {
@@ -524,7 +556,7 @@ describe('DataEngineUpdateRequestSchema', () => {
     expect(request.id).toBe('123');
   });
 
-  it('should accept update with filter', () => {
+  it('should accept update with legacy filter (backward compat)', () => {
     const request = DataEngineUpdateRequestSchema.parse({
       method: 'update',
       object: 'account',
@@ -535,6 +567,22 @@ describe('DataEngineUpdateRequestSchema', () => {
       },
     });
 
+    expect(request.options?.filter).toEqual({ category: 'premium' });
+    expect(request.options?.multi).toBe(true);
+  });
+
+  it('should accept update with new where (preferred)', () => {
+    const request = DataEngineUpdateRequestSchema.parse({
+      method: 'update',
+      object: 'account',
+      data: { status: 'active' },
+      options: {
+        where: { category: 'premium' },
+        multi: true,
+      },
+    });
+
+    expect(request.options?.where).toEqual({ category: 'premium' });
     expect(request.options?.multi).toBe(true);
   });
 });
@@ -550,7 +598,7 @@ describe('DataEngineDeleteRequestSchema', () => {
     expect(request.id).toBe('123');
   });
 
-  it('should accept delete with filter', () => {
+  it('should accept delete with legacy filter (backward compat)', () => {
     const request = DataEngineDeleteRequestSchema.parse({
       method: 'delete',
       object: 'account',
@@ -560,6 +608,21 @@ describe('DataEngineDeleteRequestSchema', () => {
       },
     });
 
+    expect(request.options?.filter).toEqual({ status: 'archived' });
+    expect(request.options?.multi).toBe(true);
+  });
+
+  it('should accept delete with new where (preferred)', () => {
+    const request = DataEngineDeleteRequestSchema.parse({
+      method: 'delete',
+      object: 'account',
+      options: {
+        where: { status: 'archived' },
+        multi: true,
+      },
+    });
+
+    expect(request.options?.where).toEqual({ status: 'archived' });
     expect(request.options?.multi).toBe(true);
   });
 });
@@ -574,7 +637,7 @@ describe('DataEngineCountRequestSchema', () => {
     expect(request.method).toBe('count');
   });
 
-  it('should accept count with filter', () => {
+  it('should accept count with where (preferred)', () => {
     const request = DataEngineCountRequestSchema.parse({
       method: 'count',
       object: 'account',
@@ -585,6 +648,18 @@ describe('DataEngineCountRequestSchema', () => {
 
     expect(request.query?.where).toBeDefined();
   });
+
+  it('should accept count with legacy filter (backward compat)', () => {
+    const request = DataEngineCountRequestSchema.parse({
+      method: 'count',
+      object: 'account',
+      query: {
+        filter: { status: 'active' },
+      },
+    });
+
+    expect(request.query?.filter).toEqual({ status: 'active' });
+  });
 });
 
 describe('DataEngineAggregateRequestSchema', () => {

packages/spec/src/data/data-engine.zod.ts

@@ -320,16 +320,47 @@ export const DataEngineContractSchema = z.object({
  * separate microservice or plugin.
  */
 
+/**
+ * RPC backward-compatibility mixin — shared `@deprecated filter` field.
+ * When both `filter` and `where` are present, the protocol/engine ignores
+ * `filter` in favor of `where`; only one should be provided.
+ */
+const RpcLegacyFilterMixin = {
+  /** @deprecated Use `where` */
+  filter: DataEngineFilterSchema.optional(),
+};
+
+/**
+ * RPC query options that accept BOTH new (where/fields/orderBy) and
+ * legacy (filter/select/sort/skip/populate) parameter names.
+ * 
+ * **Precedence:** When both legacy and new keys are present for the same
+ * concern, the protocol normalizer uses the new key (`where` > `filter`,
+ * `fields` > `select`, `orderBy` > `sort`, `offset` > `skip`,
+ * `expand` > `populate`). Callers should not mix vocabularies.
+ */
+const RpcQueryOptionsSchema = EngineQueryOptionsSchema.extend({
+  ...RpcLegacyFilterMixin,
+  /** @deprecated Use `fields` */
+  select: z.array(z.string()).optional(),
+  /** @deprecated Use `orderBy` */
+  sort: DataEngineSortSchema.optional(),
+  /** @deprecated Use `offset` */
+  skip: z.number().int().min(0).optional(),
+  /** @deprecated Use `expand` */
+  populate: z.array(z.string()).optional(),
+});
+
 export const DataEngineFindRequestSchema = z.object({
   method: z.literal('find'),
   object: z.string(),
-  query: EngineQueryOptionsSchema.optional()
+  query: RpcQueryOptionsSchema.optional()
 });
 
 export const DataEngineFindOneRequestSchema = z.object({
   method: z.literal('findOne'),
   object: z.string(),
-  query: EngineQueryOptionsSchema.optional()
+  query: RpcQueryOptionsSchema.optional()
 });
 
 export const DataEngineInsertRequestSchema = z.object({
@@ -344,26 +375,26 @@ export const DataEngineUpdateRequestSchema = z.object({
   object: z.string(),
   data: z.record(z.string(), z.unknown()),
   id: z.union([z.string(), z.number()]).optional().describe('ID for single update, or use where in options'),
-  options: EngineUpdateOptionsSchema.optional()
+  options: EngineUpdateOptionsSchema.extend(RpcLegacyFilterMixin).optional()
 });
 
 export const DataEngineDeleteRequestSchema = z.object({
   method: z.literal('delete'),
   object: z.string(),
   id: z.union([z.string(), z.number()]).optional().describe('ID for single delete, or use where in options'),
-  options: EngineDeleteOptionsSchema.optional()
+  options: EngineDeleteOptionsSchema.extend(RpcLegacyFilterMixin).optional()
 });
 
 export const DataEngineCountRequestSchema = z.object({
   method: z.literal('count'),
   object: z.string(),
-  query: EngineCountOptionsSchema.optional()
+  query: EngineCountOptionsSchema.extend(RpcLegacyFilterMixin).optional()
 });
 
 export const DataEngineAggregateRequestSchema = z.object({
   method: z.literal('aggregate'),
   object: z.string(),
-  query: EngineAggregateOptionsSchema
+  query: EngineAggregateOptionsSchema.extend(RpcLegacyFilterMixin)
 });
 
 /**