refactor: decouple executor from storage layer

[retran]

Why

After PRs #235–#237, the executor still holds direct references to *mpr.Reader and *mpr.Writer in its struct, and several command files still construct BSON for datagrids, filters, and widget cloning. This PR completes the decoupling: the executor works primarily through the backend.FullBackend abstraction.

This is the architectural pivot — dependency inversion via a BackendFactory pattern replaces the direct MPR reader/writer construction.

What changed (incremental from PR #237)

32 files changed, +1,523/-3,121 (net -1,598 lines)

Executor struct decoupled:

• Removed writer and reader fields from the Executor struct
• Added BackendFactory injection — executor_connect.go rewritten to create backends through the factory instead of constructing mpr.Reader/mpr.Writer directly
• cmd/mxcli/main.go updated to inject the factory

Remaining BSON extracted:

• mdl/backend/mpr/datagrid_builder.go (1,260 lines) — DataGrid2 construction, filter widgets, column building moved from executor to backend
• mdl/backend/mpr/datagrid_builder_test.go — test coverage for datagrid building

Deleted from executor (5 files, ~1,800 lines):

• bson_helpers.go — BSON utility functions
• cmd_pages_builder_input_cloning.go — widget cloning logic
• cmd_pages_builder_input_datagrid.go — datagrid construction
• cmd_pages_builder_input_filters.go — filter widget construction
• cmd_pages_builder_v3_pluggable.go — pluggable widget BSON

Result: The only BSON remaining in the executor is in 3 read-only files (describe, diff) that deserialize for display — no write-path BSON. Reader() remains as an escape hatch for these read-only paths.

Review-driven improvements

• Mock defaults: OpenPageForMutation, OpenWorkflowForMutation, BuildDataGrid2Widget, BuildFilterWidget now return descriptive errors instead of (nil, nil)
• Nil guards: updateWidgetsInContainer checks for nil mutator; ParseMicroflowFromRaw returns stub MDL on nil
• Spec type aliasing: NavigationProfileSpec, NavHomePageSpec, NavMenuItemSpec in sdk/mpr are now type aliases of mdl/types — eliminates silent divergence risk
• Field-count drift assertions: TestFieldCountDrift with 30 assertions (15 struct pairs) catches field additions/removals in manually-copied types
• Disconnect error propagation: execConnect, reconnect, execDisconnect log disconnect errors; Executor.Close() returns disconnect error
• Nil backendFactory guard: reconnect checks for nil factory before attempting reconnection
• Stale comments: "underlying reader" → "backend" in cmd_pages_builder.go
• Unused param: buildDataGrid2Property receiver renamed to _

Stack

PR 4/5 in the shared-types refactoring stack.
Merge order: #232 → #235 → #236 → #237 → this → #239

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Refactors the MDL executor to stop manipulating sdk/mpr/BSON directly and instead operate through backend abstractions (including new mutation interfaces), while migrating various shared value types into mdl/types.

Changes:

• Replaced many sdk/mpr dependencies with mdl/types and backend interface calls across executor and catalog.
• Introduced backend mutation interfaces (PageMutator, WorkflowMutator, widget builder/serialization backends) and updated widget update flow to use them.
• Added extensive MockBackend-based executor tests and adjusted CLI/example wiring to set a backend factory.

Reviewed changes

Copilot reviewed 109 out of 148 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
mdl/executor/cmd_workflows_write.go	Switch workflow UUID/ID generation to mdl/types.
mdl/executor/cmd_workflows_mock_test.go	Add mock-backend tests for workflow show/describe.
mdl/executor/cmd_widgets.go	Replace raw BSON widget updates with PageMutator.
mdl/executor/cmd_settings_mock_test.go	Add mock-backend tests for settings show/describe.
mdl/executor/cmd_security_write.go	Migrate security write types from mpr to types.
mdl/executor/cmd_security_mock_test.go	Add mock-backend tests for security show/describe paths.
mdl/executor/cmd_rest_clients_mock_test.go	Add mock-backend tests for REST clients show/describe.
mdl/executor/cmd_rename.go	Convert rename hit types from mpr to types.
mdl/executor/cmd_published_rest_mock_test.go	Add mock-backend tests for published REST services.
mdl/executor/cmd_pages_mock_test.go	Add mock-backend tests for pages/snippets/layouts list.
mdl/executor/cmd_pages_create_v3.go	Build pages/snippets via backend-backed pageBuilder.
mdl/executor/cmd_pages_builder_v3_pluggable.go	Remove BSON-based pluggable widget builder helpers.
mdl/executor/cmd_pages_builder_v3_layout.go	Swap mpr.GenerateID usages to types.GenerateID.
mdl/executor/cmd_pages_builder_input_filters.go	Remove BSON filter widget construction helpers.
mdl/executor/cmd_pages_builder_input_cloning.go	Remove BSON deep-clone helpers from executor.
mdl/executor/cmd_pages_builder_input.go	Move snippet resolution to backend list APIs.
mdl/executor/cmd_pages_builder.go	Refactor pageBuilder to use backend.FullBackend + mdl/types.
mdl/executor/cmd_odata_mock_test.go	Add mock-backend tests for OData show/describe.
mdl/executor/cmd_odata.go	Replace mpr helpers with types (IDs, EDMX parsing).
mdl/executor/cmd_notconnected_mock_test.go	Add tests for “not connected” behavior across commands.
mdl/executor/cmd_navigation_mock_test.go	Add mock-backend tests for navigation show/describe.
mdl/executor/cmd_navigation.go	Switch navigation document/spec types to mdl/types.
mdl/executor/cmd_modules_mock_test.go	Add mock-backend tests for module listing aggregation.
mdl/executor/cmd_misc_mock_test.go	Add mock-backend test for version output.
mdl/executor/cmd_microflows_mock_test.go	Add mock-backend tests for microflow show/describe.
mdl/executor/cmd_microflows_create.go	Switch IDs to types.GenerateID; use backend for lookups.
mdl/executor/cmd_microflows_builder_workflow.go	Switch microflow IDs to types.GenerateID.
mdl/executor/cmd_microflows_builder_graph.go	Switch graph element IDs to types.GenerateID.
mdl/executor/cmd_microflows_builder_flows.go	Switch sequence flow IDs to types.GenerateID.
mdl/executor/cmd_microflows_builder_control.go	Switch control structure IDs to types.GenerateID.
mdl/executor/cmd_microflows_builder_annotations.go	Switch annotation IDs to types.GenerateID.
mdl/executor/cmd_microflows_builder.go	Generalize microflow builder “reader” to backend interface.
mdl/executor/cmd_mermaid_mock_test.go	Add mock-backend test for mermaid domain model describe.
mdl/executor/cmd_lint.go	Route lint reader via executor accessor.
mdl/executor/cmd_jsonstructures_mock_test.go	Add mock-backend tests for JSON structures show.
mdl/executor/cmd_jsonstructures.go	Migrate JSON structure helpers/types to mdl/types.
mdl/executor/cmd_javascript_actions_mock_test.go	Add mock-backend tests for JavaScript actions.
mdl/executor/cmd_javaactions_mock_test.go	Add mock-backend tests for Java actions.
mdl/executor/cmd_javaactions.go	Migrate Java action creation IDs to types.GenerateID.
mdl/executor/cmd_import_mappings_mock_test.go	Add mock-backend tests for import mappings show.
mdl/executor/cmd_import_mappings.go	Switch JSON element typing to types and use backend for DM lookup.
mdl/executor/cmd_imagecollections_mock_test.go	Add mock-backend tests for image collections.
mdl/executor/cmd_imagecollections.go	Switch image collection typings to mdl/types.
mdl/executor/cmd_fragments_mock_test.go	Add tests for fragment listing behavior.
mdl/executor/cmd_folders.go	Switch folder info typing to mdl/types.
mdl/executor/cmd_features.go	Update comment to reflect “not connected” behavior.
mdl/executor/cmd_export_mappings_mock_test.go	Add mock-backend tests for export mappings show.
mdl/executor/cmd_export_mappings.go	Switch JSON element typing to types and use backend for DM lookup.
mdl/executor/cmd_enumerations_mock_test.go	Refactor + expand mock tests for enumerations show/describe.
mdl/executor/cmd_entities_mock_test.go	Add mock-backend tests for entity listing.
mdl/executor/cmd_entities.go	Switch entity creation IDs to types.GenerateID.
mdl/executor/cmd_diff_local.go	Delegate microflow parsing from raw BSON map to backend.
mdl/executor/cmd_dbconnection_mock_test.go	Add mock-backend tests for database connections show/describe.
mdl/executor/cmd_datatransformer_mock_test.go	Add mock-backend tests for data transformers show/describe.
mdl/executor/cmd_contract.go	Switch EDMX/AsyncAPI parsing and types to mdl/types.
mdl/executor/cmd_constants_mock_test.go	Add mock-backend tests for constants show/describe.
mdl/executor/cmd_catalog.go	Update local executor creation to use backend field.
mdl/executor/cmd_businessevents_mock_test.go	Add mock-backend tests for business event services.
mdl/executor/cmd_businessevents.go	Switch channel ID generation to types.GenerateID.
mdl/executor/cmd_associations_mock_test.go	Add mock-backend tests for associations show.
mdl/executor/cmd_agenteditor_mock_test.go	Add mock-backend tests for agent editor documents.
mdl/catalog/builder_references.go	Use types.NavMenuItem in reference extraction.
mdl/catalog/builder_navigation.go	Use types.NavMenuItem in navigation indexing.
mdl/catalog/builder_contract.go	Switch contract parsing to types.ParseEdmx / types.ParseAsyncAPI.
mdl/catalog/builder.go	Update CatalogReader interface to use mdl/types shared structs.
mdl/bsonutil/bsonutil.go	Add BSON ID conversion utilities without sdk/mpr dependency.
mdl/backend/workflow.go	Replace image backend signatures to use mdl/types.
mdl/backend/security.go	Replace security member access/revocation types to mdl/types.
mdl/backend/navigation.go	Replace navigation backend types/specs to mdl/types.
mdl/backend/mutation.go	Introduce mutation + widget builder/serialization backend interfaces.
mdl/backend/mpr/datagrid_builder_test.go	Move deep-clone BSON tests into mpr backend package, using bsonutil.
mdl/backend/mock/mock_workflow.go	Update mock image backend signatures to mdl/types.
mdl/backend/mock/mock_security.go	Update mock signatures to use mdl/types.
mdl/backend/mock/mock_navigation.go	Update mock signatures to use mdl/types.
mdl/backend/mock/mock_mutation.go	Add mock implementations for new mutation/builder interfaces.
mdl/backend/mock/mock_module.go	Update mock folder list signatures to mdl/types.
mdl/backend/mock/mock_microflow.go	Add mock hook for ParseMicroflowFromRaw.
mdl/backend/mock/mock_mapping.go	Update JSON structure signatures to mdl/types.
mdl/backend/mock/mock_java.go	Update Java/JS action signatures to mdl/types.
mdl/backend/mock/mock_infrastructure.go	Port raw unit/rename/widget type signatures to mdl/types; expand agent editor mocks.
mdl/backend/mock/mock_connection.go	Switch version/project version types to mdl/types.
mdl/backend/mock/backend.go	Extend MockBackend struct for new types + mutation/builder hooks.
mdl/backend/microflow.go	Add backend-level ParseMicroflowFromRaw API.
mdl/backend/mapping.go	Port mapping backend JSON structures API to mdl/types.
mdl/backend/java.go	Port Java backend typed list/read APIs to mdl/types.
mdl/backend/infrastructure.go	Port rename/raw unit/widget type APIs to mdl/types.
mdl/backend/doc.go	Update package docs to reflect mdl/types shared types.
mdl/backend/connection.go	Port connection + folder backend types to mdl/types.
mdl/backend/backend.go	Extend FullBackend with mutation/builder/serialization interfaces.
examples/create_datagrid2_page/main.go	Wire executor to mpr backend via backend factory.
cmd/mxcli/project_tree.go	Switch menu tree node input type to types.NavMenuItem.
cmd/mxcli/main.go	Wire CLI executor to mpr backend via backend factory.

Comments suppressed due to low confidence (1)

mdl/executor/cmd_diff_local.go:503

• microflowBsonToMDL claims it “falls back to a header-only stub if parsing fails”, but it does not handle ParseMicroflowFromRaw returning nil. If the backend cannot parse the raw map (or a mock returns nil), renderMicroflowMDL is likely to panic. Add an explicit nil check and return the intended stub output (or an empty/diagnostic MDL representation) when parsing returns nil.

func microflowBsonToMDL(ctx *ExecContext, raw map[string]any, qualifiedName string) string {
	qn := splitQualifiedName(qualifiedName)
	mf := ctx.Backend.ParseMicroflowFromRaw(raw, model.ID(qn.Name), "")

	entityNames, microflowNames := buildNameLookups(ctx)
	return renderMicroflowMDL(ctx, mf, qn, entityNames, microflowNames, nil)
}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• In mdl/executor/cmd_diff_local.go, the null check for parsed microflow returns a hardcoded string format. While this improves error handling, it might be better to propagate the error up the call stack for consistent error handling. However, given this is in a describe/diff context where output formatting is expected, this approach is acceptable.
• The MprReader() method in mdl/backend/mpr/backend.go provides direct SDK access as an escape hatch. While the comment advises preferring Backend methods, this could potentially be misused. However, this is a reasonable trade-off for gradual migration and certain edge cases (like linter rules mentioned in the comment).

What Looks Good

• Clean dependency inversion: Executor now depends on backend.FullBackend abstraction via BackendFactory instead of direct sdk/mpr imports
• Successful relocation of BSON construction logic: Datagrid2, filter widget, and cloning logic moved from executor to backend (mdl/backend/mpr/datagrid_builder.go)
• Test coverage maintained: Datagrid builder tests moved and renamed appropriately
• Consistent application: Changes applied uniformly across executor command files, examples, and main CLI
• Improved error handling: setWidgetCaptionMut now returns proper error instead of silent failure
• Cleanup: Removed unused BSON helper files (bson_helpers.go, widget cloning/files)
• Proper interface implementation: Mock backend updated with new widget builder methods
• Architectural alignment: Fully implements the dependency inversion pattern described in CLAUDE.md

Recommendation

Approve the PR. This refactor successfully completes the executor decoupling from the storage layer as intended. All changes are focused, consistent with the architectural direction, maintain test coverage, and improve the codebase without introducing functional changes. The PR addresses technical debt while preserving existing functionality through proper abstraction boundaries.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[Copilot]

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[retran]

Thanks for the review.

1. Microflow null check — agreed, the hardcoded string format is intentional here. This is a describe/diff context where we render a stub MDL representation when parsing fails. Propagating an error would suppress the rest of the diff output.

2. MprReader() escape hatch — correct, this is a deliberate transitional API. The comment on the method documents the intent and advises callers to prefer Backend methods. It will be removed once linter rules and remaining edge cases are migrated.

[github-actions]

AI Code Review

What Looks Good

This PR successfully accomplishes its goal of decoupling the executor from the storage layer through dependency inversion. Key strengths:

1. Clean architectural separation: The Executor no longer holds direct references to *mpr.Reader and *mpr.Writer, instead working through the backend.FullBackend abstraction via a BackendFactory pattern.

2. Complete BSON extraction: All write-path BSON construction (DataGrid2, filters, widget cloning) has been moved from the executor to the backend (mdl/backend/mpr/datagrid_builder.go), leaving only read-only BSON in the executor for describe/diff operations.

3. Consistent implementation: Changes are applied uniformly across:

   • Executor struct (executor.go)
   • Connection setup (executor_connect.go)
   • Main CLI entrypoint (main.go)
   • Examples (create_datagrid2_page/main.go)
   • Command files (updated to use ctx.Backend instead of direct reader/writer access)
   • Mock backend (updated with new widget building methods)

4. Test coverage maintained: The datagrid builder test was properly relocated and adapted from the executor test file.

5. Minimal disruption: The refactor focuses purely on dependency inversion without changing behavior - the executor still functions correctly through the backend abstraction.

The PR represents a significant improvement in separation of concerns, making the executor storage-agnostic and easier to test/maintain. All changes support the single stated goal of architectural decoupling.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• In mdl/executor/cmd_diff_local.go, the added nil check for microflow parsing is reasonable but could be improved with more context about why parsing might fail. However, this is a minor defensive addition that doesn't block approval.
• The mock backend now returns errors for unconfigured methods instead of nil, which is good for testability but may require updates in test files that use the mock backend (not visible in the diff).

What Looks Good

• Successfully decouples executor from storage layer by removing writer and reader fields from Executor struct
• Properly implements BackendFactory pattern for dependency injection
• Moves complex BSON construction (DataGrid2, filters, widget cloning) from executor to backend where it belongs
• Maintains test coverage by moving/renaming test files appropriately
• Preserves error handling and adds appropriate error messages in mock implementations
• Updates all call sites consistently (executor, examples, mutation.go, etc.)
• Aligns with architectural goal of dependency inversion via backend abstraction
• Part of a planned refactor stack (PR 4/5 in shared-types refactoring)
• The only BSON remaining in executor is in read-only files (describe, diff) for display purposes
• No MDL syntax changes were introduced, so full-stack consistency checklist for MDL features doesn't apply

Recommendation

Approve this PR. The refactor successfully accomplishes its goal of decoupling the executor from the storage layer through the BackendFactory pattern, moves BSON construction responsibilities to the appropriate backend layer, maintains test coverage, and follows the project's architectural direction. The changes are focused, consistent, and improve the codebase's separation of concerns.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[Copilot]

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (2)

mdl/executor/hierarchy.go:68

• newContainerHierarchyImpl ignores errors from ListUnits and ListFolders. If either call fails (e.g., transient backend error), the hierarchy cache is silently incomplete, which can lead to incorrect module/folder resolution later. Consider returning the error (or at least logging) so callers can fail fast instead of building a partial hierarchy.
  mdl/executor/executor.go:334
• Executor.Close() always returns nil even if backend.Disconnect() fails. Since Disconnect() can return an error, propagate it (or wrap it) so callers can detect close/flush failures.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

AI Code Review

Critical Issues
None found.

Moderate Issues
None found.

Minor Issues
None found.

What Looks Good

• Successfully decouples executor from storage layer via BackendFactory pattern
• Removes ~1,800 lines of BSON logic from executor files
• Moves datagrid/filter/widget cloning logic to appropriate backend location
• Maintains test coverage (datagrid_builder_test.go moved with 91% similarity)
• Clean dependency injection: executor works through backend.FullBackend abstraction
• Preserves error handling patterns using mdlerrors
• Clear architectural pivot as described in PR: "dependency inversion via a BackendFactory pattern replaces the direct MPR reader/writer construction"
• Consistent application across 32 files with net line reduction (-1,598)
• No MDL syntax changes (appropriate for this refactor-focused PR)
• Follows single responsibility principle - each change supports the decoupling goal

Recommendation
Approve - this PR successfully completes the architectural refactor to decouple the executor from direct MPR reader/writer dependencies through proper backend abstraction and dependency injection. The changes are focused, well-tested, and improve separation of concerns without introducing functionality gaps.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[github-actions]

AI Code Review

What Looks Good

This PR successfully executes the architectural pivot to decouple the executor from the storage layer via dependency inversion. The changes are consistent, focused, and maintain backward compatibility where needed:

• Clean dependency injection: The BackendFactory pattern is properly implemented in main.go and examples, replacing direct mpr.Reader/mpr.Writer construction with abstraction-backed backend creation.
• Complete BSON migration: All datagrid, filter, and widget cloning BSON logic has been moved from executor to backend (datagrid_builder.go), leaving only read-only BSON in executor for describe/diff operations.
• Thoughtful escape hatch: The MprReader() method on MprBackend provides a controlled escape for legacy code (like linters) while guiding new code toward the FullBackend interface.
• Consistent interface updates: All call sites were updated to use backend.FullBackend instead of direct SDK types (e.g., in cmd_export_mappings.go, cmd_alter_page.go).
• Improved error handling: Mock backend now returns descriptive errors instead of nil for unimplemented methods, increasing robustness.
• Test coverage maintained: Test files were appropriately moved/renamed (e.g., cmd_pages_builder_input_cloning_test.go → datagrid_builder_test.go).
• Field safety: Added field-count drift tests to prevent silent API mismatches in conversion code.
• Gradual migration path: The refactor preserves existing functionality while enabling future storage backends through the BackendFactory.

Recommendation

Approve. This PR is a well-executed, focused refactor that achieves its stated goal of decoupling the executor from the storage layer. The changes are consistent with the project's architectural direction, maintain test coverage, improve error handling, and provide a clean migration path. No critical issues were identified in the reviewed diff.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[Copilot]

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• In mdl/executor/cmd_diff_local.go, the error handling for failed microflow parsing returns a formatted string but doesn't use the standard error types from mdlerrors. This isn't critical but could be improved for consistency.
• In mdl/backend/mpr/backend.go, the New() function comment says "Call Connect(path) to open a project" but there's no Connect method shown in the diff - this may be elsewhere in the file or the comment could be slightly misleading.

What Looks Good

• Excellent execution of the architectural refactor: completely removed direct *mpr.Reader and *mpr.Writer references from the Executor struct
• Properly implemented BackendFactory pattern for dependency injection
• Successfully migrated all BSON construction logic (DataGrid2, filters, widget cloning) from executor to backend layer
• Maintained test coverage by moving test files along with implementation
• Updated all callers (main.go, examples) to use the new factory pattern
• Preserved error handling and functionality throughout the refactor
• The change enables better separation of concerns and testability
• Only read-only BSON remains in executor (for describe/diff) as intended
• Clean removal of ~1,800 lines of BSON construction code from executor files
• Consistent application of the pattern across all affected files

Recommendation

Approve. This PR successfully completes the executor decoupling refactor as described in the architectural plan. It moves storage-layer-specific BSON construction to the appropriate backend layer while maintaining all functionality through the backend.FullBackend abstraction. The changes are focused, well-tested, and follow the established patterns from previous PRs in the stack. Minor issues noted do not block approval.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[Copilot]

Pull request overview

Copilot reviewed 37 out of 37 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

AI Code Review

What Looks Good

This PR successfully accomplishes its goal of decoupling the executor from the storage layer through dependency inversion. Key strengths:

• Clean architectural separation: The executor now works exclusively through the backend.FullBackend abstraction, with direct MPR reader/writer references removed
• Proper dependency injection: BackendFactory pattern implemented correctly in main.go and examples
• Complete BSON migration: All datagrid, filter, and widget cloning BSON construction moved to mdl/backend/mpr/datagrid_builder.go (1,260 lines) with corresponding test file
• Thoughtful escape hatch: MprReader() method added for legacy cases while encouraging backend abstraction use
• Interface evolution: WidgetBuilderBackend properly extended with new methods (BuildDataGrid2Widget, BuildFilterWidget) and supporting structs
• Consistent refactor: All call sites updated (alter_page, export_mappings, etc.) to use backend abstraction
• Test coverage maintained: Datagrid builder test file relocated and preserved
• Focused scope: Purely an architectural refactor with no mixed concerns
• Error handling preserved: Proper use of mdlerrors throughout moved code

Recommendation

Approve - This PR is a well-executed architectural refactor that successfully decouples the executor from storage layer dependencies. It follows the dependency inversion principle, maintains test coverage, updates all consumers consistently, and prepares the codebase for future shared-types work as described in the PR stack. No blocking issues identified.

---

Automated review via OpenRouter (Nemotron Super 120B) — workflow source

[ako]

Code Review

Overview

This PR completes the executor decoupling that PRs #235–#237 began. Net result: the executor holds no *mpr.Reader or *mpr.Writer fields, five BSON-heavy executor files (~1,800 lines) are deleted, and DataGrid2/filter/cloning logic moves to mdl/backend/mpr/datagrid_builder.go. It also addresses two specific issues raised in the PR #235 review: the NavigationProfileSpec divergence risk and the absence of field-drift detection.

---

Positive Aspects

• BackendFactory pattern is clean. executor_connect.go replaces the old triple-assignment (writer/reader/backend = mprbackend.Wrap(...)) with a straightforward factory call + Connect(). Nil guard on backendFactory is present. Wiring in cmd/mxcli/main.go is correct.
• Spec type aliases close the divergence risk flagged in refactor: extract shared types and utility functions to mdl/types #235. NavigationProfileSpec, NavHomePageSpec, NavMenuItemSpec in sdk/mpr are now proper type aliases (= types.*). The unconvertNavProfileSpec conversion collapses to a pass-through, removing 27 lines of manual field copying.
• TestFieldCountDrift is a good lightweight guard. 30 assertions across 15 struct pairs using reflect.TypeOf(v).NumField() will catch any field addition/removal in manually-converted types and force an explicit update to convert.go.
• Mock defaults are now fail-loud. OpenPageForMutation, OpenWorkflowForMutation, BuildDataGrid2Widget, BuildFilterWidget all return descriptive "MockBackend.X not configured" errors instead of (nil, nil) — prevents silent nil-dereference bugs in tests.
• Reader() escape hatch is handled well. Uses a local readerProvider interface to avoid circular imports, has nil guards, and carries a deprecation comment signalling intent to remove it.

---

Issues & Concerns

1. datagrid_builder_test.go lacks happy-path coverage for the main entry points

The test file covers only the deep-clone helper (three tests: ID regeneration, arrays, nil preservation). There are no tests for:

• BuildDataGrid2Widget() — column assembly, datasource binding, paging, selection
• BuildFilterWidget() — any filter type
• The property builders (buildDataGrid2Property, buildFiltersPlaceholderProperty, applyDataGridPagingProps, applyDataGridSelectionProp)

These are the highest-value paths in the new file and the most likely to have edge cases. The PR moves them from the executor (where they were also untested) to the backend — a good structural move — but doesn't add new coverage in the process.

2. TestFieldCountDrift validates cardinality only, not field names or order

If a field is renamed (e.g., Name → DisplayName) the count stays the same and the test passes while convert.go silently copies the wrong value. This is inherent to the approach but worth noting — it's a partial guard, not a complete one.

3. New WidgetBuilderBackend interface methods are not in the mock by default

BuildDataGrid2Widget and BuildFilterWidget are added to mock_mutation.go with descriptive error defaults (good), but there are no MockDataGrid2Mutator helpers or test fixtures for exercising these methods. Tests that exercise datagrid creation paths in the executor still need integration with a real backend.

---

Checklist Against Project Standards

Item	Status
Executor no longer imports sdk/mpr for write paths	✅ Only 3 read-only display files remain
BackendFactory nil guard	✅ Present in reconnect
Spec type divergence resolved	✅ Aliases in sdk/mpr/writer_navigation.go
Field drift detection	✅ TestFieldCountDrift with 30 assertions
Mock defaults fail-loud	✅ Descriptive error messages
datagrid_builder.go happy-path tests	⚠️ Only cloning helper tested; widget construction untested
Error propagation improvements	✅ Close() returns error, disconnect errors logged

---

Verdict

Approve. The BackendFactory pattern is well-implemented, the spec aliasing fix directly addresses prior review feedback, and the field-drift test is a practical addition. The main gap is that datagrid_builder.go's primary entry points (BuildDataGrid2Widget, BuildFilterWidget) have no unit tests — these moved from executor to backend untested. That's worth a follow-up issue rather than a blocker, since the structural goal of this PR is achieved cleanly.