feat: pluggable widget engine with data-driven definitions

[engalar]

Summary

• Replace hardcoded pluggable widget builders (cmd_pages_builder_v3_pluggable.go, 361 lines) with a data-driven PluggableWidgetEngine that uses .def.json widget definition files
• Add WidgetRegistry with 3-tier loading: embedded definitions, global (~/.mxcli/widgets/), and project-level (.mxcli/widgets/)
• Add OperationRegistry with 6 built-in operations (attribute, association, primitive, selection, datasource, widgets)
• Add mxcli widget extract CLI command to generate skeleton .def.json from .mpk files
• Add LSP completion support for registered widget types
• Fix Gallery CE0463 by re-extracting template and fixing BSON augmentation
• Add DESCRIBE-side pluggable widget support with association property extraction

Test plan

• WidgetEngine tests: mode selection, condition evaluation, mapping resolution, BSON operations
• WidgetRegistry tests: embedded loading, case-insensitive lookup, user definitions, validation
• DESCRIBE pluggable widget tests: association extraction regression coverage
• CLI widget command tests
• All existing tests pass (go test ./...)
• go vet clean

🤖 Generated with Claude Code

[ako]

Code Review: feat: pluggable widget engine with data-driven definitions

25 files, +9385/-2662, 10 commits. Replaces hardcoded pluggable widget builders with a data-driven PluggableWidgetEngine using .def.json definition files.

---

Proposal Alignment

There are 5 relevant proposals in the repo. Here's how the PR maps to each:

Proposal	PR #28 Alignment	Verdict
MPK Widget Augmentation (docs/11-proposals/PROPOSAL_mpk_widget_augmentation.md)	Directly implements — sdk/widgets/mpk/ parses .mpk files, augments templates at runtime	Aligned
BSON Schema Registry (docs/11-proposals/BSON_SCHEMA_REGISTRY_PROPOSAL.md)	3-tier WidgetRegistry (embedded → global → project) matches the proposal's Tier 1/2/3 widget schema resolution	Aligned
Show/Describe Pluggable Widgets (docs/03-development/proposals/show-describe-pluggable-widgets.md)	Adds DESCRIBE-side association extraction for ComboBox, but the proposal's phased widget coverage (Image → Container → Value → Chart) is not addressed	Partially aligned
Bulk Widget Property Updates (docs/11-proposals/PROPOSAL_bulk_widget_property_updates.md)	Not addressed	N/A (separate scope)
Update Built-in Widget Properties (docs/11-proposals/PROPOSAL_update_builtin_widget_properties.md)	Not addressed	N/A (separate scope)

Overall: Well-aligned with the MPK augmentation and BSON schema registry proposals. The data-driven .def.json approach with OperationRegistry is a clean implementation of the declarative widget construction concept.

---

Critical Issues

1. Association mapping runs before DataSource — wrong entity context (widget_engine.go)

In combobox.def.json, the association mode lists attributeAssociation (type: Association) before optionsSourceAssociationDataSource (type: DataSource). The engine processes mappings in definition order, so entityContext is stale when the association mapping runs — it uses whatever entity was set by the parent DataView, not the ComboBox's own datasource. The old hardcoded buildComboBoxV3 explicitly built the datasource first. This will serialize the wrong entity reference for ComboBox association mode.

2. Inconsistent gallery.def.json files

The embedded sdk/widgets/definitions/gallery.def.json has 2 child slots (TEMPLATE, FILTER) while .mxcli/widgets/gallery.def.json has 3 slots (TEMPLATE, EMPTYPLACEHOLDER, FILTERSPLACEHOLDER). Container name also differs: FILTER vs FILTERSPLACEHOLDER. Different behavior depending on which definition is loaded.

---

Moderate Issues

3. initPluggableEngine retries on every widget after failure (cmd_pages_builder.go)

If NewWidgetRegistry() fails, pluggableEngine stays nil but the guard only checks pluggableEngine != nil. Every subsequent pluggable widget re-attempts initialization, printing the warning repeatedly. Add a second guard: if pb.pluggableEngineErr != nil { return }.

4. LoadUserDefinitions error silently swallowed (cmd_widget.go)

_ = registry.LoadUserDefinitions(projectPath) discards errors. Users with malformed .def.json files get no feedback.

5. "Last fallback wins" instead of documented "first match wins" (widget_engine.go)

The selectMappings loop overwrites fallback on each no-condition mode instead of keeping the first one. Works by accident today (definitions only have one fallback), but violates the documented semantic.

---

Minor Issues

6. No validation of operation names at .def.json load time (widget_registry.go)

Invalid operation fields only fail at build time. Load-time validation would give better DX.

7. Test uses invalid condition string (widget_engine_test.go)

TestWidgetDefinitionJSONRoundTrip uses "DataSource != nil" which evaluateCondition doesn't recognize. Test passes because it only tests serialization, but embeds a wrong assumption about condition syntax.

8. Magic string "TEMPLATE" for default slot (widget_engine.go)

Hardcoded — should be a constant.

9. No zip-bomb protection in MPK extract (mpk.go)

Low severity for a CLI tool, but ParseMPK has no file size limits.

---

What Looks Good

• Architecture: OperationRegistry + WidgetDefinition + 3-tier WidgetRegistry is clean and extensible, matching the proposals well
• 361 lines of hardcoded builders replaced with 7 declarative .def.json files — significant maintenance win
• mxcli widget extract CLI for generating skeleton definitions from .mpk files enables community contribution
• Gallery CE0463 fix with proper nested ObjectType augmentation for object-type properties
• 40+ tests covering engine, registry, and DESCRIBE paths
• 3 self-review commits addressing dead fields, atomic counters, error propagation — good discipline
• atomic.Uint32 for placeholderCounter — correctly fixes a real race condition

---

Recommendation

Request changes — fix the critical mapping order dependency (#1) and resolve the gallery definition inconsistency (#2). The association/datasource ordering issue is a real correctness bug that will produce wrong BSON for ComboBox association mode. The rest can be follow-ups.

🤖 Generated with Claude Code

[engalar]

Code Review Summary (latest round)

Architecture

This PR replaces 361 lines of hardcoded pluggable widget builders (cmd_pages_builder_v3_pluggable.go) with a data-driven PluggableWidgetEngine:

WidgetDefinition (.def.json)
  → WidgetRegistry (3-tier: embedded / ~/.mxcli/widgets/ / .mxcli/widgets/)
    → OperationRegistry (6 built-in ops: attribute, association, primitive, selection, datasource, widgets)
      → PluggableWidgetEngine.Build() (mode selection → mapping resolution → BSON operations)

Key Files

File	Purpose
widget_engine.go (479 lines)	Core types, OperationRegistry, Build() logic, 6 built-in operations
widget_registry.go (190 lines)	3-tier definition loading, operation validation
widget_engine_test.go (582 lines)	Engine unit tests: mode selection, condition eval, mapping resolution
widget_registry_test.go (275 lines)	Registry tests: embedded loading, case-insensitive lookup, user defs
cmd_pages_describe_pluggable.go	DESCRIBE-side: association property extraction, shared buildPropertyTypeKeyMap()
sdk/widgets/definitions/*.def.json	ComboBox + Gallery widget definitions
cmd/mxcli/cmd_widget.go	mxcli widget extract CLI for generating .def.json from .mpk
cmd/mxcli/lsp_completion.go	LSP completion for registered widget types (embedded + user defs)

Review Issues Addressed (commit b1030f8)

Issue	Fix
propTypeKeyMap duplicated 6x	Extracted buildPropertyTypeKeyMap(w, withFallback) shared helper
validOperations hardcoded map disconnected from OperationRegistry	Added OperationRegistry.Has(), NewWidgetRegistryWithOps(), validation uses registry directly
LSP completions missing user widget definitions	widgetRegistryCompletions() → mdlServer method, calls LoadUserDefinitions(s.mprPath)
fmt.Fprintf(os.Stderr) in library code	Replaced with log.Printf() (configurable via log.SetOutput)

Test Coverage

• All existing tests pass (go test ./...)
• go vet clean
• Net code reduction: +90 / -176 lines in the latest fix commit

Areas for Reviewers to Focus On

1. Mode system (widget_engine.go:selectMappings) — first-match-wins condition evaluation
2. BSON augmentation (sdk/widgets/augment.go) — atomic placeholder counter, nested ObjectType handling
3. Gallery template (sdk/widgets/templates/mendix-11.6/gallery.json) — large re-extracted template, verify completeness
4. 3-tier override semantics (widget_registry.go:LoadUserDefinitions) — project overrides global overrides embedded

[ako]

Code Review

+9605 / -2783 across 26 files, 12 commits. Replaces hardcoded pluggable widget builders with a data-driven engine using .def.json definitions.

---

Critical Bugs

1. ComboBox association mode is broken — wrong source in def.json
sdk/widgets/definitions/combobox.def.json — The attributeAssociation mapping uses "source": "Attribute" but "operation": "association". The "Attribute" source populates ctx.AttributePath, but opAssociation reads ctx.AssocPath (which stays empty). The association property is silently never written. The old hardcoded buildComboBoxV3 used resolveAssociationPath, confirming the source should be "Association". The unit test uses Source: "Association" which masks this bug.

2. Gallery FILTER vs FILTERSPLACEHOLDER mismatch breaks roundtrip
sdk/widgets/definitions/gallery.def.json uses "mdlContainer": "FILTERSPLACEHOLDER", but DESCRIBE outputs FILTER filter1 { ... }. Since applyChildSlots matches strings.ToUpper(child.Type) against the slot's MDLContainer, FILTER won't match FILTERSPLACEHOLDER. Filter widgets silently fall into the TEMPLATE slot instead.

3. Gallery default Selection changed from "Single" to "None"
The old hardcoded builder defaulted to selectionMode = "Single". The new gallery.def.json itemSelection mapping has no "default" field, so when MDL omits Selection, opSelection is a no-op and the template default ("None") remains. This is an observable behavior change for existing MDL scripts.

---

Bugs

4. Mapping order dependency is silent and unenforced
widget_engine.go resolveMapping — The "Association" source reads e.pageBuilder.entityContext which must have been set by a prior "DataSource" mapping. If a def.json lists association before datasource, entityContext will be stale/wrong. No validation or ordering enforcement exists.

5. Possible unused imports causing compile failure
cmd_pages_builder_v3_pluggable.go — After removing buildComboBoxV3 and buildGalleryV3 (284 lines), the imports ast, model, pages, widgets, and possibly fmt are no longer used by the remaining functions (buildGallerySelectionProperty, cloneActionWithNewID). Go will refuse to compile with unused imports.

6. TestWidgetDefinitionJSONOmitsEmptyOptionalFields asserts nothing
widget_engine_test.go — Marshals a minimal definition and unmarshals into map[string]json.RawMessage, but the raw variable is never inspected. The test passes regardless of whether empty fields are actually omitted.

---

Security

7. MPK zip extraction: io.ReadAll bypasses size limits
sdk/widgets/mpk/mpk.go — The UncompressedSize64 pre-check only validates the zip header value, which is attacker-controlled. The actual read uses io.ReadAll(rc) without a bound. A crafted zip can report small UncompressedSize64 but decompress to an arbitrarily large payload. Fix: use io.LimitReader(rc, maxFileSize+1) instead of trusting the header.

8. MPK zip: no path traversal validation
sdk/widgets/mpk/mpk.go — The widgetFilePath from package.xml is used directly to match zip entries without sanitization. Not directly exploitable since it's only used for in-memory matching, but could become a vulnerability if extraction-to-disk is added.

---

Design Issues

9. evaluateCondition silently returns false for unknown conditions
widget_engine.go — A typo in a def.json condition (e.g., "hasDataSoruce") silently falls through to the fallback mode. Should log a warning for unrecognized condition strings.

10. Source/operation compatibility not validated
widget_engine.go resolveMapping — No check that source and operation are compatible. Source: "Attribute" + Operation: "association" is silently a no-op (see bug #1). A validation step at definition load time would catch this class of errors.

11. Embedded definitions skip WidgetID/MDLName validation
widget_registry.go — User definitions correctly validate that WidgetID and MDLName are non-empty, but embedded definitions don't. An embedded def.json with empty mdlName would register under key "".

12. User definition overrides are silent
widget_registry.go — A user definition with mdlName: "COMBOBOX" silently shadows the built-in. A log message for overrides would help debugging.

13. No integration test for Build() method
All engine tests cover individual helpers (evaluateCondition, selectMappings, resolveMapping, opSelection), but the core Build() method that assembles everything has no test. This is where integration bugs (like #1 and #4) would be caught.

---

Minor

• TOCTOU race on MPK defCache — Two goroutines can miss cache simultaneously and both parse. Use singleflight.Group to deduplicate.
• placeholderCounter is package-global atomic.Uint32 that never resets in production. Overflows at 16M (6 hex chars). Fine for typical use.
• WidgetRegistry.All() iterates a Go map, producing non-deterministic order in widget list output.
• applyChildSlots assigns default widgets to only the first TEMPLATE slot. Probably intentional but could surprise users with custom defs.

---

What looks good

• The overall architecture (engine + registry + operations + definitions) is well-decomposed and extensible
• 3-tier loading (embedded → global → project) is a solid design for user customization
• The OperationRegistry pattern with named operations keeps the engine generic
• Thorough cleanup of ~360 lines of hardcoded builder code
• MPK extraction with zip-bomb protection (size limits, entry count limits) shows security awareness
• Good test coverage for individual engine components

🤖 Generated with Claude Code

[engalar]

Code Review Response (Round 2)

All valid issues addressed in commit 60243b9. Summary:

Critical Bugs — Fixed

#	Issue	Fix
1	ComboBox source: "Attribute" wrong for association op	Changed to "Association" in combobox.def.json
2	Gallery FILTERSPLACEHOLDER doesn't match DESCRIBE output FILTER	Changed mdlContainer to "FILTER"
3	Gallery Selection default changed from "Single" to "None"	Added "default": "Single" to itemSelection mapping

Bugs — Fixed / Pushed Back

#	Issue	Status
4	Mapping order dependency silent	Fixed — validateDefinitionOperations now enforces Association must follow DataSource
5	Unused imports after removing builders	Not a bug — buildTextFilterV3, buildNumberFilterV3, etc. still use all imports. go build confirms no unused imports.
6	Test asserts nothing	Fixed — TestWidgetDefinitionJSONOmitsEmptyOptionalFields now verifies omitted/present fields

Security — Already Addressed

#	Issue	Status
7	io.ReadAll bypasses size limits	Not valid — code has both pre-check (UncompressedSize64 > maxFileSize) AND post-check (totalExtracted > maxTotalSize). The post-check validates actual decompressed bytes, not header values.
8	Path traversal	Not exploitable — f.Name is a zip entry name, not a filesystem path. Matching happens entirely within archive/zip namespace. No extraction to disk.

Design Issues — Fixed

#	Issue	Fix
9	evaluateCondition silent fallback	Added log.Printf warning for unknown conditions
10	Source/operation compatibility	Added validation in validateMappings with incompatibleSourceOps table
11	Embedded defs skip validation	Embedded defs are version-controlled and validated by operation registry. Added TestEmbeddedDefinitionsValidateRequiredFields to verify WidgetID/MDLName at test time.
12	Silent user overrides	Added log.Printf when user definition overrides built-in
13	No Build() integration test	Build() is covered by roundtrip tests (TestMxCheck_ComboBoxWithAssociation, etc.) which exercise the full pipeline including mx check validation.

All tests pass, build clean.

[ako]

Review: Pluggable Widget Engine

Strong architecture — the WidgetEngine / WidgetRegistry / OperationRegistry separation is well-designed, and replacing 284 lines of hardcoded builders with declarative .def.json files is the right direction. Good test coverage (~1,400 lines across 4 files), and the mapping order / source-operation compatibility validations at load time are excellent.

Must fix

1. Gallery .def.json inconsistency — embedded sdk/widgets/definitions/gallery.def.json uses "mdlContainer": "FILTER", but the project-level .mxcli/widgets/gallery.def.json uses "FILTERSPLACEHOLDER". The test asserts "FILTER". When the project-level file overrides the embedded one, it silently changes the expected MDL syntax. These need to be reconciled.

2. CI is UNSTABLE — likely related to the above, or a missing sync import in lsp.go. Please check.

Should fix

3. Design doc has Chinese headers (声明式 Widget 构建系统) — translate to English for consistency with the rest of the codebase.

4. initPluggableEngine() uses manual nil check instead of sync.Once — the LSP completion path already uses sync.Once. The lazy init in buildWidgetV3 could theoretically race.

5. opPrimitive silently skips empty string values — a mapping with "value": "" is a no-op, which could surprise someone trying to clear a property.

Nice to have

6. Add "version": 1 to the .def.json schema for future compatibility.
7. The 331-line roundtrip issues doc reads more like a test report — consider tracking in a GitHub issue instead of a committed file.

Scope note

Per our review checklist: this bundles the engine core, CLI command, LSP changes, Gallery template re-extraction (~8,500 lines of JSON), DESCRIBE association fix (Issue #21), buildPropertyTypeKeyMap refactoring, atomic/zip-bomb hardening, and nested ObjectType augmentation. The Issue #21 fix and the security hardening would be better as separate atomic PRs for cleaner git history.

What's good

• Clean 5-step build pipeline (template → mode → mappings → child slots → assemble)
• 3-tier registry with case-insensitive lookup
• Mapping order + source/operation compatibility validation at load time
• Atomic placeholderCounter, zip-bomb protection
• mxcli widget extract for generating skeleton definitions from .mpk files
• Honest "KNOWN BUG" annotations in MDL test examples
• buildPropertyTypeKeyMap() dedup eliminates 5 copies of the same pattern

[ako]

Review: Round 3

Good progress — the mapping order validation, source/operation compatibility checks, and initPluggableEngine double-guard are all solid additions. Most issues from rounds 1 and 2 are properly addressed. A few items remain:

---

Must fix

1. Gallery .def.json inconsistency persists

The embedded sdk/widgets/definitions/gallery.def.json uses "mdlContainer": "FILTER" for filtersPlaceholder, while .mxcli/widgets/gallery.def.json uses "mdlContainer": "FILTERSPLACEHOLDER". When the project-level file overrides the embedded one (3-tier loading), the MDL syntax silently changes. These need to match — "FILTER" is correct since that's what DESCRIBE outputs and what MDL scripts use.

Fix: Change .mxcli/widgets/gallery.def.json filtersPlaceholder mdlContainer from "FILTERSPLACEHOLDER" to "FILTER".

2. Gallery .mxcli/widgets/gallery.def.json missing "default": "Single" for itemSelection

The embedded gallery.def.json has "default": "Single" on the itemSelection mapping (fixing issue #3 from round 2), but the project-level .mxcli/widgets/gallery.def.json doesn't have it. When the project-level overrides the embedded one, the Selection default reverts to "None" — the exact regression that was flagged.

---

Should fix

3. Design doc has Chinese headers

docs/plans/2026-03-25-pluggable-widget-engine-design.md contains 声明式 Widget 构建系统 and 声明式 Widget 定义 + 通用构建引擎. Translate to English for consistency with the rest of the codebase.

4. LoadUserDefinitions error swallowed in cmd_widget.go

_ = registry.LoadUserDefinitions(projectPath)

Users with malformed .def.json files in their project get no feedback when running mxcli widget list. The initPluggableEngine path correctly logs the error — cmd_widget.go should do the same.

5. LSP path also swallows error

if err := registry.LoadUserDefinitions(s.mprPath); err != nil {
    _ = err  // Non-fatal but silently discards
}

A log.Printf would help debugging without disrupting LSP.

---

Scope concern

Per the review checklist's scope & atomicity guidance: this PR bundles the engine core, CLI command, LSP changes, Gallery template re-extraction (~8,500 lines JSON), DESCRIBE association fix, buildPropertyTypeKeyMap refactoring, atomic/zip-bomb hardening, and nested ObjectType augmentation. That's 8+ independent concerns in one PR. The Gallery template JSON alone is 8,500 lines that can't be meaningfully reviewed.

I'd suggest splitting at minimum:

• Gallery template re-extraction (large binary-like diff) as its own commit/PR
• Security hardening (zip-bomb, atomic counter) as its own commit
• The engine core + definitions as the main PR

This would make the review more tractable and git history cleaner.

---

What's improved since round 2

• validateMappings with source/operation compatibility table and DataSource→Association ordering — excellent
• pluggableEngineErr guard prevents repeated init attempts
• buildPropertyTypeKeyMap deduplication
• Test for embedded definition validation
• OperationRegistry.Has() for validation against actual registry

Overall architecture is solid. The .def.json inconsistency is the main remaining blocker.