fix: address all PR #28 code review issues (critical, moderate, minor)

Critical: fix ComboBox association mapping order (datasource before
association), align gallery.def.json embedded/user definitions.
Moderate: cache engine init errors, surface LoadUserDefinitions errors,
fix fallback "first wins" semantics.
Minor: validate operation names at load time, fix test condition string,
extract TEMPLATE constant, add MPK zip-bomb protection.
Update design doc and templates README to reflect changes.

Author: engalar

docs/plans/2026-03-25-pluggable-widget-engine-design.md

@@ -1,7 +1,7 @@
 # Pluggable Widget Engine: 声明式 Widget 构建系统
 
 **Date**: 2026-03-25
-**Status**: Design (research only)
+**Status**: Implemented
 
 ## Problem
 
@@ -56,18 +56,9 @@
   "templateFile": "combobox.json",
   "defaultEditable": "Always",
 
-  "modes": {
-    "default": {
-      "description": "Enumeration mode",
-      "propertyMappings": [
-        {
-          "propertyKey": "attributeEnumeration",
-          "source": "Attribute",
-          "operation": "attribute"
-        }
-      ]
-    },
-    "association": {
+  "modes": [
+    {
+      "name": "association",
       "condition": "hasDataSource",
       "description": "Association mode with DataSource",
       "propertyMappings": [
@@ -76,24 +67,35 @@
           "value": "association",
           "operation": "primitive"
         },
-        {
-          "propertyKey": "attributeAssociation",
-          "source": "Attribute",
-          "operation": "association"
-        },
         {
           "propertyKey": "optionsSourceAssociationDataSource",
           "source": "DataSource",
           "operation": "datasource"
         },
+        {
+          "propertyKey": "attributeAssociation",
+          "source": "Attribute",
+          "operation": "association"
+        },
         {
           "propertyKey": "optionsSourceAssociationCaptionAttribute",
           "source": "CaptionAttribute",
           "operation": "attribute"
         }
       ]
+    },
+    {
+      "name": "default",
+      "description": "Enumeration mode",
+      "propertyMappings": [
+        {
+          "propertyKey": "attributeEnumeration",
+          "source": "Attribute",
+          "operation": "attribute"
+        }
+      ]
     }
-  }
+  ]
 }
 ```
 
@@ -105,33 +107,26 @@ Gallery (with child slots):
   "mdlName": "GALLERY",
   "templateFile": "gallery.json",
   "defaultEditable": "Always",
-  "defaultSelection": "Single",
 
   "propertyMappings": [
-    {
-      "propertyKey": "datasource",
-      "source": "DataSource",
-      "operation": "datasource"
-    },
-    {
-      "propertyKey": "itemSelection",
-      "source": "Selection",
-      "operation": "primitive",
-      "default": "Single"
-    }
+    {"propertyKey": "advanced", "value": "false", "operation": "primitive"},
+    {"propertyKey": "datasource", "source": "DataSource", "operation": "datasource"},
+    {"propertyKey": "itemSelection", "source": "Selection", "operation": "selection"},
+    {"propertyKey": "itemSelectionMode", "value": "clear", "operation": "primitive"},
+    {"propertyKey": "desktopItems", "value": "1", "operation": "primitive"},
+    {"propertyKey": "tabletItems", "value": "1", "operation": "primitive"},
+    {"propertyKey": "phoneItems", "value": "1", "operation": "primitive"},
+    {"propertyKey": "pageSize", "value": "20", "operation": "primitive"},
+    {"propertyKey": "pagination", "value": "buttons", "operation": "primitive"},
+    {"propertyKey": "pagingPosition", "value": "below", "operation": "primitive"},
+    {"propertyKey": "showEmptyPlaceholder", "value": "none", "operation": "primitive"},
+    {"propertyKey": "onClickTrigger", "value": "single", "operation": "primitive"}
   ],
 
   "childSlots": [
-    {
-      "propertyKey": "content",
-      "mdlContainer": "TEMPLATE",
-      "operation": "widgets"
-    },
-    {
-      "propertyKey": "filtersPlaceholder",
-      "mdlContainer": "FILTER",
-      "operation": "widgets"
-    }
+    {"propertyKey": "content", "mdlContainer": "TEMPLATE", "operation": "widgets"},
+    {"propertyKey": "emptyPlaceholder", "mdlContainer": "EMPTYPLACEHOLDER", "operation": "widgets"},
+    {"propertyKey": "filtersPlaceholder", "mdlContainer": "FILTERSPLACEHOLDER", "operation": "widgets"}
   ]
 }
 ```
@@ -192,6 +187,19 @@ type ChildSlotMapping struct {
 }
 ```
 
+### Critical: Property Mapping Order Dependency
+
+**The engine processes `propertyMappings` in array order.** Some operations depend on side effects of earlier ones:
+
+- `datasource` sets `pageBuilder.entityContext` as a side effect
+- `association` reads `pageBuilder.entityContext` to resolve the target entity
+
+Therefore, in any mode that uses both, **`datasource` must come before `association`** in the mappings array. Getting this wrong produces silently incorrect BSON (wrong entity reference).
+
+### Operation Validation
+
+Operation names in `.def.json` files are validated at load time against the 6 known operations: `attribute`, `association`, `primitive`, `selection`, `datasource`, `widgets`. Invalid operation names produce an error when `NewWidgetRegistry()` or `LoadUserDefinitions()` runs, rather than failing silently at build time.
+
 ### Mode Selection Conditions
 
 Built-in conditions (extensible):
@@ -201,7 +209,7 @@ Built-in conditions (extensible):
 | `hasDataSource` | `w.GetDataSource() != nil` |
 | `hasAttribute` | `w.GetAttribute() != ""` |
 | `hasProp:X` | `w.GetStringProp("X") != ""` |
-| (none) | `"default"` mode always selected |
+| (none) | Fallback — first no-condition mode wins if multiple exist |
 
 ### Engine Flow
 
@@ -317,3 +325,6 @@ mxcli widget extract --mpk path/to/widget.mpk
 | Template version drift | Existing `augment.go` handles .mpk sync, works unchanged |
 | Performance regression | Template loading is already cached; engine adds minimal overhead |
 | User-provided templates may be invalid | Validate on load: check type+object sections exist, PropertyKey coverage |
+| MPK zip-bomb attack | `ParseMPK` enforces per-file (50MB) and total (200MB) extraction limits |
+| Invalid operation names in .def.json | Validated at load time, not build time — immediate feedback |
+| Engine init failure retried on every widget | Init error cached; subsequent widgets skip immediately |

mdl/executor/cmd_pages_builder.go

@@ -50,7 +50,7 @@ type pageBuilder struct {
 
 // initPluggableEngine lazily initializes the pluggable widget engine.
 func (pb *pageBuilder) initPluggableEngine() {
-	if pb.pluggableEngine != nil {
+	if pb.pluggableEngine != nil || pb.pluggableEngineErr != nil {
 		return
 	}
 	registry, err := NewWidgetRegistry()
@@ -60,7 +60,9 @@ func (pb *pageBuilder) initPluggableEngine() {
 		return
 	}
 	if pb.reader != nil {
-		_ = registry.LoadUserDefinitions(pb.reader.Path())
+		if loadErr := registry.LoadUserDefinitions(pb.reader.Path()); loadErr != nil {
+			fmt.Fprintf(os.Stderr, "warning: loading user widget definitions: %v\n", loadErr)
+		}
 	}
 	pb.widgetRegistry = registry
 	pb.pluggableEngine = NewPluggableWidgetEngine(NewOperationRegistry(), pb)

mdl/executor/widget_engine.go

@@ -14,6 +14,9 @@ import (
 	"go.mongodb.org/mongo-driver/bson"
 )
 
+// defaultSlotContainer is the MDLContainer name that receives default (non-containerized) child widgets.
+const defaultSlotContainer = "TEMPLATE"
+
 // =============================================================================
 // Pluggable Widget Engine — Core Types and Operation Registry
 // =============================================================================
@@ -297,8 +300,10 @@ func (e *PluggableWidgetEngine) selectMappings(def *WidgetDefinition, w *ast.Wid
 	for i := range def.Modes {
 		mode := &def.Modes[i]
 		if mode.Condition == "" {
-			// No condition = default fallback (use last one if multiple)
-			fallback = mode
+			// No condition = default fallback (use first one if multiple)
+			if fallback == nil {
+				fallback = mode
+			}
 			continue
 		}
 		if e.evaluateCondition(mode.Condition, w) {
@@ -447,7 +452,7 @@ func (e *PluggableWidgetEngine) applyChildSlots(slots []ChildSlotMapping, w *ast
 	for _, slot := range slots {
 		childBSONs := slotWidgets[slot.PropertyKey]
 		// If no explicit container children, use default widgets for the first slot
-		if len(childBSONs) == 0 && len(defaultWidgets) > 0 && slot.MDLContainer == "TEMPLATE" {
+		if len(childBSONs) == 0 && len(defaultWidgets) > 0 && slot.MDLContainer == defaultSlotContainer {
 			childBSONs = defaultWidgets
 			defaultWidgets = nil // consume once
 		}

mdl/executor/widget_engine_test.go

@@ -29,7 +29,7 @@ func TestWidgetDefinitionJSONRoundTrip(t *testing.T) {
 		Modes: []WidgetMode{
 			{
 				Name:        "association",
-				Condition:   "DataSource != nil",
+				Condition:   "hasDataSource",
 				Description: "Association-based ComboBox with datasource",
 				PropertyMappings: []PropertyMapping{
 					{PropertyKey: "attributeAssociation", Source: "Attribute", Operation: "association"},
@@ -87,8 +87,8 @@ func TestWidgetDefinitionJSONRoundTrip(t *testing.T) {
 	if assocMode.Name != "association" {
 		t.Errorf("Mode name: got %q, want %q", assocMode.Name, "association")
 	}
-	if assocMode.Condition != "DataSource != nil" {
-		t.Errorf("Mode condition: got %q, want %q", assocMode.Condition, "DataSource != nil")
+	if assocMode.Condition != "hasDataSource" {
+		t.Errorf("Mode condition: got %q, want %q", assocMode.Condition, "hasDataSource")
 	}
 	if len(assocMode.PropertyMappings) != 2 {
 		t.Errorf("Mode PropertyMappings count: got %d, want 2", len(assocMode.PropertyMappings))

mdl/executor/widget_registry.go

@@ -45,6 +45,10 @@ func NewWidgetRegistry() (*WidgetRegistry, error) {
 			return nil, fmt.Errorf("parse definition %s: %w", entry.Name(), err)
 		}
 
+		if err := validateDefinitionOperations(&def, entry.Name()); err != nil {
+			return nil, err
+		}
+
 		reg.byMDLName[strings.ToUpper(def.MDLName)] = &def
 		reg.byWidgetID[def.WidgetID] = &def
 	}
@@ -136,8 +140,49 @@ func (r *WidgetRegistry) loadDefinitionsFromDir(dir string) error {
 			return fmt.Errorf("invalid definition %s: widgetId and mdlName are required", entry.Name())
 		}
 
+		if err := validateDefinitionOperations(&def, entry.Name()); err != nil {
+			return err
+		}
+
 		r.byMDLName[strings.ToUpper(def.MDLName)] = &def
 		r.byWidgetID[def.WidgetID] = &def
 	}
 	return nil
 }
+
+// validOperations is the set of recognized operation names for property and child-slot mappings.
+var validOperations = map[string]bool{
+	"attribute":   true,
+	"association": true,
+	"primitive":   true,
+	"selection":   true,
+	"datasource":  true,
+	"widgets":     true,
+}
+
+// validateDefinitionOperations checks that all operation names in a definition are recognized.
+func validateDefinitionOperations(def *WidgetDefinition, source string) error {
+	for _, m := range def.PropertyMappings {
+		if !validOperations[m.Operation] {
+			return fmt.Errorf("%s: unknown operation %q in propertyMappings for key %q", source, m.Operation, m.PropertyKey)
+		}
+	}
+	for _, s := range def.ChildSlots {
+		if !validOperations[s.Operation] {
+			return fmt.Errorf("%s: unknown operation %q in childSlots for key %q", source, s.Operation, s.PropertyKey)
+		}
+	}
+	for _, mode := range def.Modes {
+		for _, m := range mode.PropertyMappings {
+			if !validOperations[m.Operation] {
+				return fmt.Errorf("%s: unknown operation %q in mode %q propertyMappings for key %q", source, m.Operation, mode.Name, m.PropertyKey)
+			}
+		}
+		for _, s := range mode.ChildSlots {
+			if !validOperations[s.Operation] {
+				return fmt.Errorf("%s: unknown operation %q in mode %q childSlots for key %q", source, s.Operation, mode.Name, s.PropertyKey)
+			}
+		}
+	}
+	return nil
+}

mdl/executor/widget_registry_test.go

@@ -239,8 +239,8 @@ func TestRegistryGalleryChildSlots(t *testing.T) {
 		t.Fatal("GALLERY not found")
 	}
 
-	if len(def.ChildSlots) != 2 {
-		t.Fatalf("childSlots count = %d, want 2", len(def.ChildSlots))
+	if len(def.ChildSlots) != 3 {
+		t.Fatalf("childSlots count = %d, want 3", len(def.ChildSlots))
 	}
 
 	// Verify slot mappings
@@ -257,11 +257,19 @@ func TestRegistryGalleryChildSlots(t *testing.T) {
 		t.Errorf("TEMPLATE slot propertyKey = %q, want content", contentSlot.PropertyKey)
 	}
 
-	filterSlot, ok := slotsByContainer["FILTER"]
+	emptySlot, ok := slotsByContainer["EMPTYPLACEHOLDER"]
 	if !ok {
-		t.Fatal("FILTER slot not found")
+		t.Fatal("EMPTYPLACEHOLDER slot not found")
+	}
+	if emptySlot.PropertyKey != "emptyPlaceholder" {
+		t.Errorf("EMPTYPLACEHOLDER slot propertyKey = %q, want emptyPlaceholder", emptySlot.PropertyKey)
+	}
+
+	filterSlot, ok := slotsByContainer["FILTERSPLACEHOLDER"]
+	if !ok {
+		t.Fatal("FILTERSPLACEHOLDER slot not found")
 	}
 	if filterSlot.PropertyKey != "filtersPlaceholder" {
-		t.Errorf("FILTER slot propertyKey = %q, want filtersPlaceholder", filterSlot.PropertyKey)
+		t.Errorf("FILTERSPLACEHOLDER slot propertyKey = %q, want filtersPlaceholder", filterSlot.PropertyKey)
 	}
 }

sdk/widgets/definitions/combobox.def.json

@@ -10,8 +10,8 @@
       "description": "Association mode",
       "propertyMappings": [
         {"propertyKey": "optionsSourceType", "value": "association", "operation": "primitive"},
-        {"propertyKey": "attributeAssociation", "source": "Attribute", "operation": "association"},
         {"propertyKey": "optionsSourceAssociationDataSource", "source": "DataSource", "operation": "datasource"},
+        {"propertyKey": "attributeAssociation", "source": "Attribute", "operation": "association"},
         {"propertyKey": "optionsSourceAssociationCaptionAttribute", "source": "CaptionAttribute", "operation": "attribute"}
       ]
     },