fix: Address code review feedback

- Fix severity handling: treat 'info' as non-blocking like warnings
- Add try/catch for regex pattern validation to handle invalid patterns
- Fix conditional rule validation: validate rules array and use validateRule for nested rules
- Remove function-based conditions for security (now deprecated with warning)
- Add default case for unhandled validation rule types with warning
- Fix field name propagation in validateFields()
- Fix buildCondition to properly map all filter operators (not just '=')
- Remove unused imports from data-protocol.ts
- Fix ObjectPermission union type (remove redundant boolean shape)
- Rename FieldValidationFunction to avoid conflict with data-protocol ValidationFunction
- Export ValidationFunction from data-protocol for AdvancedValidationRule
- Update ValidationFunction doc comment to clarify it's specific to data-protocol
- Make validateBuiltInRule async to support conditional rules with async validators

Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>

Author: Copilot

packages/core/src/query/query-ast.ts

@@ -118,11 +118,64 @@ export class QueryASTBuilder {
 
   private buildCondition(condition: AdvancedFilterCondition): OperatorNode {
     const field = this.buildField(condition.field);
-    const value = this.buildLiteral(condition.value);
 
+    // Map filter operators to comparison operators
+    const operatorMap: Record<string, string> = {
+      'equals': '=',
+      'not_equals': '!=',
+      'greater_than': '>',
+      'greater_than_or_equal': '>=',
+      'less_than': '<',
+      'less_than_or_equal': '<=',
+      'contains': 'contains',
+      'not_contains': 'contains',
+      'starts_with': 'starts_with',
+      'ends_with': 'ends_with',
+      'like': 'like',
+      'ilike': 'ilike',
+      'in': 'in',
+      'not_in': 'not_in',
+      'is_null': 'is_null',
+      'is_not_null': 'is_not_null',
+      'between': 'between',
+    };
+
+    const operator = operatorMap[condition.operator] || '=';
+    
+    // Handle special operators
+    if (operator === 'between' && condition.values && condition.values.length === 2) {
+      return {
+        type: 'operator',
+        operator: 'between' as any,
+        operands: [
+          field,
+          this.buildLiteral(condition.values[0]),
+          this.buildLiteral(condition.values[1])
+        ],
+      };
+    }
+
+    if ((operator === 'in' || operator === 'not_in') && condition.values) {
+      return {
+        type: 'operator',
+        operator: operator as any,
+        operands: [field, ...condition.values.map(v => this.buildLiteral(v))],
+      };
+    }
+
+    if (operator === 'is_null' || operator === 'is_not_null') {
+      return {
+        type: 'operator',
+        operator: operator as any,
+        operands: [field],
+      };
+    }
+
+    // Standard binary operator
+    const value = this.buildLiteral(condition.value);
     return {
       type: 'operator',
-      operator: '=',
+      operator: operator as any,
       operands: [field, value],
     };
   }

packages/core/src/validation/validation-engine.ts

@@ -57,7 +57,7 @@ export class ValidationEngine {
           severity: rule.severity || 'error',
         };
 
-        if (rule.severity === 'warning') {
+        if (rule.severity === 'warning' || rule.severity === 'info') {
           warnings.push(error);
         } else {
           errors.push(error);
@@ -111,11 +111,11 @@ export class ValidationEngine {
   /**
    * Validate built-in rules
    */
-  private validateBuiltInRule(
+  private async validateBuiltInRule(
     value: any,
     rule: AdvancedValidationRule,
     context?: ValidationContext
-  ): string | null {
+  ): Promise<string | null> {
     const { type, params, message } = rule;
 
     switch (type) {
@@ -139,9 +139,13 @@ export class ValidationEngine {
 
       case 'pattern':
         if (typeof value === 'string') {
-          const regex = typeof params === 'string' ? new RegExp(params) : params;
-          if (!regex.test(value)) {
-            return message || 'Invalid format';
+          try {
+            const regex = typeof params === 'string' ? new RegExp(params) : params;
+            if (!regex.test(value)) {
+              return message || 'Invalid format';
+            }
+          } catch (error) {
+            return message || 'Invalid pattern configuration';
           }
         }
         break;
@@ -305,29 +309,41 @@ export class ValidationEngine {
       case 'conditional':
         if (context?.values && params) {
           const { condition, rules } = params;
+          
+          if (!Array.isArray(rules) || rules.length === 0) {
+            break;
+          }
+          
           const conditionMet = this.evaluateCondition(condition, context.values);
 
           if (conditionMet) {
             for (const conditionalRule of rules) {
-              const result = this.validateBuiltInRule(value, conditionalRule, context);
+              const result = await this.validateRule(value, conditionalRule, context);
               if (result) {
                 return result;
               }
             }
           }
         }
         break;
+      
+      default:
+        // Unhandled validation rule type
+        console.warn(`Unsupported validation rule type: ${type}`);
+        return null;
     }
 
     return null;
   }
 
   /**
    * Evaluate a condition
+   * Note: Conditions must be declarative objects, not functions, for security.
    */
   private evaluateCondition(condition: any, values: Record<string, any>): boolean {
     if (typeof condition === 'function') {
-      return condition(values);
+      console.warn('Function-based conditions are deprecated and will be removed. Use declarative conditions instead.');
+      return false; // Security: reject function-based conditions
     }
 
     if (typeof condition === 'object' && condition.field) {
@@ -372,7 +388,11 @@ export class ValidationEngine {
 
     for (const [field, schema] of Object.entries(schemas)) {
       const value = values[field];
-      results[field] = await this.validate(value, schema, context);
+      const schemaWithField: AdvancedValidationSchema = {
+        ...schema,
+        field: schema.field ?? field,
+      };
+      results[field] = await this.validate(value, schemaWithField, context);
     }
 
     return results;

packages/types/src/data-protocol.ts

@@ -17,11 +17,8 @@
  */
 
 // Import existing base types to avoid duplication
-import type { QueryParams } from './data';
-import type { FilterOperator as BaseFilterOperator, FilterCondition as BaseFilterCondition } from './complex';
 import type { SortConfig as BaseSortConfig } from './objectql';
-import type { ValidationRule as BaseValidationRule } from './field-types';
-import type { ValidationError as BaseValidationError } from './data';
+import type { FilterOperator as BaseFilterOperator } from './complex';
 
 /**
  * =============================================================================
@@ -623,7 +620,14 @@ export type ValidationRuleType =
   | 'exists_check';
 
 /**
- * Validation function type from base
+ * Validation function signature used by AdvancedValidationRule in the data protocol.
+ *
+ * This type is defined in this module and may differ from similarly named
+ * validation function types in other packages (e.g., in `field-types`).
+ * 
+ * @param value - The value to validate
+ * @param context - Optional validation context with access to other field values
+ * @returns true if valid, false or error message string if invalid
  */
 export type ValidationFunction = (value: any, context?: ValidationContext) => boolean | string;
 
@@ -681,9 +685,24 @@ export interface AdvancedValidationResult {
 }
 
 /**
- * Validation error (Phase 3.5.5: Improved error messages) - Extended
+ * Validation error (Phase 3.5.5: Improved error messages)
  */
-export interface AdvancedValidationError extends BaseValidationError {
+export interface AdvancedValidationError {
+  /**
+   * Field path
+   */
+  field: string;
+  
+  /**
+   * Error message
+   */
+  message: string;
+  
+  /**
+   * Error code
+   */
+  code?: string;
+  
   /**
    * Rule type that failed
    */

packages/types/src/field-types.ts

@@ -731,12 +731,7 @@ export interface ObjectSchemaMetadata {
   /**
    * Permissions (Phase 3.1.4: Enhanced permissions)
    */
-  permissions?: ObjectPermission | {
-    create?: boolean;
-    read?: boolean;
-    update?: boolean;
-    delete?: boolean;
-  };
+  permissions?: ObjectPermission;
 
   /**
    * Parent object to inherit from (Phase 3.1.2)

packages/types/src/index.ts

@@ -300,7 +300,7 @@ export type {
 export type {
   BaseFieldMetadata,
   VisibilityCondition,
-  ValidationFunction,
+  ValidationFunction as FieldValidationFunction,
   TextFieldMetadata,
   TextareaFieldMetadata,
   MarkdownFieldMetadata,
@@ -390,6 +390,7 @@ export type {
   AdvancedValidationSchema,
   AdvancedValidationRule,
   ValidationRuleType,
+  ValidationFunction,
   AsyncValidationFunction,
   ValidationContext,
   AdvancedValidationResult,