refactor: define mutation backend interfaces

[retran]

Why

The executor currently manipulates BSON documents directly to alter pages and workflows — constructing widget trees, updating properties, managing workflow activities. This logic is scattered across cmd_alter_page.go (1721 lines) and cmd_alter_workflow.go (887 lines), tightly coupling the executor to sdk/mpr internals.

This PR defines the mutation contract — interfaces that the executor will call through instead of manipulating BSON directly.

What changed (incremental from PR #235)

17 files changed, +485/-146

New files:

• mdl/backend/mutation.go (181 lines) — defines PageMutator and WorkflowMutator interfaces covering all mutation operations: widget property ops, widget tree ops (add/drop/move), variable ops, layout ops, pluggable widget ops, activity management, and Save
• mdl/backend/mock/mock_mutation.go (64 lines) — mock implementations for testing
• mdl/bsonutil/bsonutil.go (33 lines) — BSON ID conversion utilities (IDToBsonBinary, BsonBinaryToID, NewIDBsonBinary) extracted from sdk/mpr/utils.go into a CGO-free package

Interface wiring:

• backend.go and mpr/backend.go extended to expose the new mutation interfaces
• Executor files (cmd_alter_page.go, cmd_alter_workflow.go, cmd_pages_builder_input*.go) updated to call through the new interfaces — preparing for the implementation in PR refactor: implement mutation backends and migrate handlers #237

Bug fix:

• GenerateDeterministicID now sets UUID v4 version and variant bits on hash bytes before formatting

Design: PageMutator and WorkflowMutator are session-scoped — created per mutation operation, accumulate changes, and commit with Save(). This matches the existing pattern of loading a unit, modifying it, and writing it back.

Stack

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

[github-actions]

AI Code Review

Critical Issues

• Incorrect conversion function signatures in mdl/backend/mpr/convert.go: Multiple conversion functions have incorrect input types, using types.* instead of mpr.* for parameters. This will cause type mismatches when called from backend.go since the MPR reader returns mpr.* types, not types.* types.

  Specifically affected functions:

  • convertFolderInfoSlice (should take []*mpr.FolderInfo, not []*types.FolderInfo)
  • convertUnitInfoSlice (should take []*mpr.UnitInfo, not []*types.UnitInfo)
  • convertRawUnitSlice (should take []*mpr.RawUnit, not []*types.RawUnit)
  • convertJavaActionSlice (should take []*mpr.JavaAction, not []*types.JavaAction)
  • convertJavaScriptActionSlice (should take []*mpr.JavaScriptAction, not []*types.JavaScriptAction)
  • convertJavaScriptActionPtr (should take *mpr.JavaScriptAction, not *types.JavaScriptAction)
  • convertNavDocSlice (should take []*mpr.NavigationDocument, not []*types.NavigationDocument)
  • convertNavDocPtr (should take *mpr.NavigationDocument, not *types.NavigationDocument)
  • convertJsonStructureSlice (should take []*mpr.JsonStructure, not []*types.JsonStructure)
  • convertJsonStructurePtr (should take *mpr.JsonStructure, not *types.JsonStructure)
  • convertImageCollectionSlice (should take []*mpr.ImageCollection, not []*types.ImageCollection)

  This will cause compilation errors or runtime panics when these functions are invoked.

Moderate Issues

• None identified beyond the conversion function issue.

Minor Issues

• The PluggablePropertyContext struct in mutation.go has many fields which could benefit from better organization, but this appears necessary for the pluggable widget property operations.
• Some panic messages in MprBackend implementations (e.g., "MprBackend.OpenPageForMutation not yet implemented") could be more specific about which backend method is unimplemented.

What Looks Good

• Clean, well-documented interface definitions in mutation.go for PageMutator, WorkflowMutator, and related backend interfaces.
• Consistent replacement of sdk/mpr dependencies with mdl/types across backend files.
• Proper mock implementations that forward to function pointers for testability.
• The bsonutil/bsonutil.go file provides a clean abstraction for BSON conversion without dragging in sdk/mpr dependencies.
• Overall approach of extracting

---

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

[Copilot]

Pull request overview

This PR advances the “shared-types refactoring” stack by moving several previously sdk/mpr-owned value types into mdl/types, introducing WASM-safe BSON ID helpers, and defining mutation-focused backend interfaces so the executor can mutate pages/workflows through an abstraction layer (rather than manipulating BSON directly).

Changes:

• Add mdl/types shared structs + ID/hash utilities and update sdk/mpr/mdl/ callers to use them.
• Add mdl/bsonutil (WASM-safe) for UUID↔BSON Binary conversion and migrate BSON-writing call sites to it.
• Define mutation backend contracts (PageMutator, WorkflowMutator, serialization backends) and extend mock backends + add mock-based executor tests.

Reviewed changes

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

Show a summary per file

File	Description
sdk/mpr/writer_imagecollection_test.go	Update tests to use types.ImageCollection.
sdk/mpr/writer_imagecollection.go	Writer now accepts *types.ImageCollection for image collection creation/serialization.
sdk/mpr/writer_core.go	Delegate UUID/blob conversion and ID generation to mdl/types.
sdk/mpr/utils.go	Delegate ID/UUID/hash helpers to mdl/types; reimplemented IDToBsonBinary.
sdk/mpr/reader.go	Delegate blobToUUID to types.BlobToUUID.
sdk/mpr/parser_misc.go	Parser returns mdl/types equivalents for Java/JS actions, navigation, image collections, JSON structures.
sdk/mpr/asyncapi.go	Replace AsyncAPI structs with type aliases to mdl/types and delegate parsing.
mdl/visitor/visitor_agenteditor.go	Remove stray blank line.
mdl/types/navigation.go	New navigation document/profile/menu types for backend-agnostic use.
mdl/types/mapping.go	New JSON structure + image collection types for backend-agnostic use.
mdl/types/java.go	New lightweight Java/JavaScript action descriptors.
mdl/types/infrastructure.go	New backend-agnostic infrastructure types (folders, units, raw units, rename hits, etc.).
mdl/types/id.go	New shared ID/UUID conversion + hashing utilities.
mdl/types/doc.go	Package documentation for mdl/types.
mdl/types/asyncapi.go	AsyncAPI document types + parser moved into mdl/types.
mdl/linter/rules/page_navigation_security.go	Switch linter navigation menu traversal to types.NavMenuItem.
mdl/executor/widget_templates.go	Switch ID generation to types.GenerateID.
mdl/executor/widget_operations.go	Switch $ID generation to bsonutil + types.GenerateID.
mdl/executor/widget_engine_test.go	Switch BlobToUUID usage to types.
mdl/executor/widget_engine.go	Switch template ID generator + widget IDs to types.GenerateID.
mdl/executor/helpers.go	Use types.FolderInfo and types.GenerateID in folder helpers.
mdl/executor/executor.go	Cache now stores types.UnitInfo / types.FolderInfo.
mdl/executor/cmd_workflows_write.go	Switch workflow ID generation to types.GenerateID.
mdl/executor/cmd_workflows_mock_test.go	Add mock-based workflow show/describe tests.
mdl/executor/cmd_settings_mock_test.go	Add mock-based settings show/describe tests.
mdl/executor/cmd_security_write.go	Switch member access/revocation structs to types.
mdl/executor/cmd_security_mock_test.go	Add mock-based security show/describe tests.
mdl/executor/cmd_rest_clients_mock_test.go	Add mock-based consumed REST client tests.
mdl/executor/cmd_rename.go	Switch rename hit types to types.RenameHit.
mdl/executor/cmd_published_rest_mock_test.go	Add mock-based published REST tests.
mdl/executor/cmd_pages_mock_test.go	Add mock-based pages/snippets/layouts list tests.
mdl/executor/cmd_pages_builder_v3_pluggable.go	Use bsonutil.NewIDBsonBinary() for regenerated $IDs; switch ID generator to types.
mdl/executor/cmd_pages_builder_v3_layout.go	Switch layout widget IDs to types.GenerateID.
mdl/executor/cmd_pages_builder_input_filters.go	Switch widget-template ID generation to types and $ID handling to bsonutil.
mdl/executor/cmd_pages_builder_input_cloning_test.go	Update cloning tests to use bsonutil.NewIDBsonBinary().
mdl/executor/cmd_pages_builder_input_cloning.go	Switch regenerated $IDs and binary-ID parsing to bsonutil/types.
mdl/executor/cmd_pages_builder_input.go	Switch blob-to-UUID conversion to types and $ID creation to bsonutil.
mdl/executor/cmd_pages_builder.go	Switch folder cache element types to types.FolderInfo.
mdl/executor/cmd_odata_mock_test.go	Add mock-based OData client/service tests.
mdl/executor/cmd_odata.go	Switch ID generation to types and EDMX parsing to types.ParseEdmx.
mdl/executor/cmd_notconnected_mock_test.go	Add centralized not-connected guard tests using a disconnected mock backend.
mdl/executor/cmd_navigation_mock_test.go	Add mock-based navigation show/describe tests using mdl/types navigation structs.
mdl/executor/cmd_navigation.go	Switch navigation spec/document types to mdl/types.
mdl/executor/cmd_modules_mock_test.go	Add mock-based module listing test using types.UnitInfo.
mdl/executor/cmd_misc_mock_test.go	Add mock-based version output test using types.ProjectVersion.
mdl/executor/cmd_microflows_mock_test.go	Add mock-based microflow/nanoflow show/describe tests.
mdl/executor/cmd_microflows_create.go	Switch microflow IDs to types.GenerateID.
mdl/executor/cmd_microflows_builder_workflow.go	Switch microflow workflow-action builder IDs to types.GenerateID.
mdl/executor/cmd_microflows_builder_graph.go	Switch flow graph object 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/flows to types.GenerateID.
mdl/executor/cmd_mermaid_mock_test.go	Add mock-based mermaid domain model describe test.
mdl/executor/cmd_jsonstructures_mock_test.go	Add mock-based JSON structure show test using types.JsonStructure.
mdl/executor/cmd_jsonstructures.go	Switch JSON structure types + JSON helpers to mdl/types.
mdl/executor/cmd_javascript_actions_mock_test.go	Add mock-based JavaScript action show/describe tests using types.JavaScriptAction.
mdl/executor/cmd_javaactions_mock_test.go	Add mock-based Java action show/describe tests.
mdl/executor/cmd_javaactions.go	Switch Java action ID generation to types.GenerateID.
mdl/executor/cmd_import_mappings_mock_test.go	Add mock-based import mapping show test.
mdl/executor/cmd_import_mappings.go	Switch JSON-structure element map types to types.JsonElement.
mdl/executor/cmd_imagecollections_mock_test.go	Add mock-based image collection show/describe tests using types.ImageCollection.
mdl/executor/cmd_imagecollections.go	Switch image collection types to mdl/types.
mdl/executor/cmd_fragments_mock_test.go	Add mock-based fragments show tests.
mdl/executor/cmd_folders.go	Switch folder list argument type to types.FolderInfo.
mdl/executor/cmd_export_mappings_mock_test.go	Add mock-based export mapping show test.
mdl/executor/cmd_export_mappings.go	Switch JSON-structure element map types to types.JsonElement.
mdl/executor/cmd_enumerations_mock_test.go	Refactor and expand enumeration mock tests to use shared mock helpers.
mdl/executor/cmd_entities_mock_test.go	Add mock-based entity list tests.
mdl/executor/cmd_entities.go	Switch entity creation IDs (attrs, rules, indexes) to types.GenerateID.
mdl/executor/cmd_dbconnection_mock_test.go	Add mock-based DB connection show/describe tests.
mdl/executor/cmd_datatransformer_mock_test.go	Add mock-based data transformer list/describe tests.
mdl/executor/cmd_constants_mock_test.go	Add mock-based constants show/describe tests.
mdl/executor/cmd_businessevents_mock_test.go	Add mock-based business event service tests.
mdl/executor/cmd_businessevents.go	Switch generated channel names to use types.GenerateID.
mdl/executor/cmd_associations_mock_test.go	Add mock-based associations list tests.
mdl/executor/cmd_alter_workflow.go	Switch $ID generation from mpr to bsonutil.
mdl/executor/cmd_alter_page.go	Switch blob-to-UUID and $ID generation to types/bsonutil.
mdl/executor/cmd_agenteditor_write.go	Formatting/alignment in Agent creation struct literal.
mdl/executor/cmd_agenteditor_models.go	Formatting/alignment in Agent Editor model creation struct literal.
mdl/catalog/builder_references.go	Switch menu item reference extraction to types.NavMenuItem.
mdl/catalog/builder_navigation.go	Switch catalog navigation counting/insertion to types.NavMenuItem.
mdl/catalog/builder_contract.go	Switch EDMX/AsyncAPI parsing to mdl/types.
mdl/catalog/builder.go	Update CatalogReader interface to use mdl/types value types.
mdl/bsonutil/bsonutil.go	New BSON-aware ID utilities (UUID↔BSON Binary + new ID).
mdl/backend/workflow.go	Backend interfaces updated to use mdl/types (e.g., image collections).
mdl/backend/security.go	Backend security interfaces updated to use mdl/types member access structs.
mdl/backend/navigation.go	Backend navigation interfaces updated to use mdl/types navigation structs/specs.
mdl/backend/mutation.go	New mutation + serialization backend interfaces (page/workflow mutators).
mdl/backend/mock/mock_workflow.go	Mock backend updated for types.ImageCollection.
mdl/backend/mock/mock_security.go	Mock backend updated for types.EntityAccessRevocation.
mdl/backend/mock/mock_navigation.go	Mock backend updated for types.NavigationDocument/spec.
mdl/backend/mock/mock_mutation.go	Add stub/mock implementations for new mutation + serialization interfaces.
mdl/backend/mock/mock_module.go	Mock folder listing updated to types.FolderInfo.
mdl/backend/mock/mock_mapping.go	Mock mapping/JSON structure APIs updated to types.JsonStructure.
mdl/backend/mock/mock_java.go	Mock Java/JS action APIs updated to types.JavaAction/types.JavaScriptAction.
mdl/backend/mock/mock_infrastructure.go	Mock infra APIs updated to types (rename hits, raw units, units, widget types) + add agent editor write hooks.
mdl/backend/mock/mock_connection.go	Mock connection/version APIs updated to types equivalents.
mdl/backend/mock/backend.go	MockBackend fields updated for types-based signatures + new mutation/serialization hooks.
mdl/backend/mapping.go	MappingBackend now uses types.JsonStructure.
mdl/backend/java.go	JavaBackend now uses types.JavaAction/types.JavaScriptAction.
mdl/backend/infrastructure.go	Infrastructure interfaces now use types (rename hits, raw units, units, widget type).
mdl/backend/doc.go	Update backend package docs to reflect mdl/types migration.
mdl/backend/connection.go	ConnectionBackend now uses types.MPRVersion and types.ProjectVersion; folder listing uses types.FolderInfo.
mdl/backend/backend.go	FullBackend now includes mutation + serialization backend interfaces.
internal/marketplace/types.go	Minor comment formatting cleanup.
cmd/mxcli/project_tree.go	Project tree menu rendering updated to consume types.NavMenuItem.

---

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

[github-actions]

AI Code Review

Review Summary

This PR defines mutation backend interfaces to decouple the executor from direct BSON manipulation, preparing for implementation in PR #237. It's part of a shared-types refactoring stack.

Critical Issues

• Potential syntax error in bsonutil.go: The visible diff shows func BsonBinaryToID(bin primitive.Binary) str which appears incomplete/truncated (should likely be string return type). However, since the file is reported as 33 lines total and we only see ~26 lines in the visible diff, this is likely just a display issue from diff truncation.

Moderate Issues

• Placeholder implementations: The new mutation methods in MprBackend (OpenPageForMutation, OpenWorkflowForMutation, etc.) currently just panic with "not yet implemented". This is expected as the actual implementation will come in PR refactor: implement mutation backends and migrate handlers #237, but worth noting.
• Missing bsonutil tests: While the mdl/bsonutil/ package is new, no corresponding test file is visible in the diff (though tests may exist outside the shown diff).

Minor Issues

• Minor comment formatting inconsistencies (e.g., missing spaces in some comments)
• The PR appropriately scopes itself to interface definition only, with implementation deferred to PR refactor: implement mutation backends and migrate handlers #237

What Looks Good

• Clean separation of concerns: Well-defined interfaces (PageMutator, WorkflowMutator) with clear contracts
• Proper decoupling: Consistent replacement of sdk/mpr types with mdl/types across backend files
• Comprehensive mocks: Thorough mock implementations in mdl/backend/mock/ following established patterns
• Good documentation: Interface methods include clear explanations of purpose and usage
• Consistent application: Refactor applied uniformly across all relevant backend interfaces
• Appropriate error handling: Interface methods return error where expected for mutation operations

Recommendation

Approve the PR. The interface definitions are well-designed and follow project conventions. The placeholder panics are appropriate preparation for PR #237, and any minor issues (like the potentially truncated function in bsonutil.go) are likely display artifacts rather than actual code defects. The PR successfully achieves its goal of defining the mutation contract without blocking current functionality.

---

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

[github-actions]

AI Code Review

Critical Issues

• Silent data corruption risk in ID conversion: The IDToBsonBinary function in mdl/bsonutil/bsonutil.go has a dangerous fallback that generates a new random ID when the input ID is invalid:

  blob := types.UUIDToBlob(id)
  if blob == nil || len(blob) != 16 {
      blob = types.UUIDToBlob(types.GenerateID()) // Generates new random ID!
  }

  If this function is used when updating existing elements (rather than creating new ones), it could silently corrupt data by replacing a specific element's ID with a random one. This violates the principle of "no silent side effects" from the security checklist.

Moderate Issues

None identified.

Minor Issues

• Missing interface tests: While mock implementations are provided for PageMutator and WorkflowMutator, there are no explicit tests verifying the interface contracts. Tests should ideally accompany new interfaces, though understandable since the actual implementation comes in PR refactor: implement mutation backends and migrate handlers #237.
• Inconsistent naming in conversion functions: In mdl/backend/mpr/convert.go, functions like convertFolderInfoSlice use []*types.FolderInfo as input parameter type, which is technically correct since mpr.X is a type alias for types.X, but using mpr.X in the signature would improve clarity for conversion functions.

What Looks Good

• Clean separation of concerns: The PR successfully defines mutation interfaces (PageMutator, WorkflowMutator) that decouple the executor from direct BSON manipulation, addressing the core issue of scattered logic in cmd_alter_page.go and cmd_alter_workflow.go.
• Proper dependency management: Consistent migration to mdl/types across backend files eliminates direct sdk/mpr dependencies in interfaces, aligning with the shared-types refactoring goal.
• Thoughtful interface design:
  • Session-scoped mutators with explicit Save() method match existing load-modify-save patterns
  • Comprehensive coverage of widget operations (properties, tree manipulation, variables, layout, pluggable widgets)
  • Parallel workflow mutation interface for activity/outcome/path/boundary event operations
  • Well-structured PluggablePropertyContext that carries operation-specific values cleanly
• Testing preparation: Mock implementations enable testing without requiring real backend connections.
• Incremental approach: Properly prepares executor files for upcoming implementation in PR refactor: implement mutation backends and migrate handlers #237 while maintaining backward compatibility during transition.
• Documentation: Clear interface comments explain usage patterns and error handling expectations.

Recommendation

Approve with notes - The PR successfully establishes the mutation contract foundation for the shared-types refactoring. The silent data corruption risk in IDToBsonBinary requires attention before this utility is used in element update contexts (rather than creation contexts), but given the PR's focus on interface definition and the upcoming implementation PR, this can be addressed in the implementation phase or a follow-up. The minor issues do not block approval as they align with the PR's preparatory nature in the refactoring stack.

---

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

[retran]

Re: AI Review (comment 1) — conversion function signatures:

False positive. sdk/mpr/reader_types.go defines type aliases using = (e.g., type FolderInfo = types.FolderInfo). This means []*types.FolderInfo and []*mpr.FolderInfo are the same type — no conversion needed, no compilation error. The signatures are correct.

[retran]

Re: AI Review (comment 2) — truncated function signature and placeholder panics:

The truncated str is a diff display artifact — BsonBinaryToID returns string and compiles fine. Placeholder panics in MprBackend are intentional — implementation comes in PR #237.

[retran]

Re: AI Review (comment 3) — IDToBsonBinary silent fallback:

The fallback-to-random-ID behavior is inherited from the original sdk/mpr/writer_core.go:idToBsonBinary — not introduced by this PR. It exists because MPR writers must always produce a valid binary ID field; an invalid input ID (which should never happen in practice) gets a new random ID rather than crashing the writer mid-document. Changing this behavior is out of scope for this refactoring stack.

The type alias naming suggestion (using mpr.X in conversion signatures) is noted but would be misleading — these functions live in mdl/backend/mpr/convert.go and operate on mdl/types values. Using mpr.* would imply a dependency on sdk/mpr that does not exist.

[retran]

Re: AI Reviews — fixes applied in 26b2478:

1. IDToBsonBinary silent fallback — Fixed. Now panics with a clear message on invalid UUID input instead of silently generating a random ID. Invalid IDs at this layer are always programming errors.

2. Conversion function signatures — Fixed. Input params now use mpr.* aliases (the source package) while return types use types.* (the target package), making the conversion direction explicit.

3. Interface tests — The compile-time check var _ backend.FullBackend = (*MockBackend)(nil) already validates the interface contract. Runtime mock-wiring tests would be low-value boilerplate.

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• In mdl/backend/mpr/backend.go, the new mutation interface methods (OpenPageForMutation, OpenWorkflowForMutation, etc.) panic with "not yet implemented". While appropriate for work-in-progress, consider adding a TODO comment linking to PR refactor: implement mutation backends and migrate handlers #237 for traceability.
• The mdl/bsonutil/bsonutil.go file extracts UUID conversion utilities but the PR description doesn't mention if corresponding tests were added (though the convert_test.go shows testing patterns are being followed).
• In cmd/mxcli/project_tree.go, the import of github.com/mendixlabs/mxcli/mdl/types is added but could potentially be consolidated with other type imports if they exist elsewhere in the file (though current diff shows it's the only types import needed).

What Looks Good

• The PR successfully defines clear, focused interfaces (PageMutator, WorkflowMutator, WidgetSerializationBackend) that encapsulate mutation operations previously scattered across executor files.
• Consistent application of the shared-types pattern: replacing sdk/mpr dependencies with mdl/types across all backend interfaces, mocks, and conversions.
• Proper separation of concerns: interfaces define the contract, mocks enable testing, conversions handle type translation, and executors will use the interfaces (prepared for PR refactor: implement mutation backends and migrate handlers #237).
• The bug fix for GenerateDeterministicID setting UUID v4 version and variant bits addresses a correctness issue that could lead to invalid UUIDs.
• Test coverage for the conversion layer (convert_test.go) verifies field preservation and type alias consistency, ensuring the refactoring doesn't break data integrity.
• All changes align with the stated goal of preparing for PR refactor: implement mutation backends and migrate handlers #237's implementation while maintaining backward compatibility through the abstraction layer.
• The refactor touches many files but maintains a single, coherent purpose: defining the mutation backend contract as part of the shared-types stack.

Recommendation

Approve. The PR is a well-scoped, high-quality refactoring that consistently implements the shared-types pattern, prepares cleanly for subsequent implementation work, includes necessary test coverage, and fixes a legitimate bug. No blocking issues were identified.

---

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

[Copilot]

Pull request overview

Copilot reviewed 123 out of 126 changed files in this pull request and generated 5 comments.

---

💡 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

• Missing test for new bsonutil package: The PR introduces mdl/bsonutil/bsonutil.go with new utility functions (IDToBsonBinary, BsonBinaryToID, NewIDBsonBinary), but no corresponding test file is visible in the provided diff. While the diff is truncated, the absence of any test file for this new package in the visible changes is concerning. Unit tests should validate UUID-to-BSON conversion correctness, especially given the bug fix mentioned in the PR description regarding UUID version/variant bits.

Minor Issues

None found.

What Looks Good

• Clean separation of concerns: The mutation interfaces (PageMutator, WorkflowMutator) properly encapsulate page/workflow mutation operations, reducing direct BSON manipulation in executors.
• Consistent type extraction: Backend interfaces now consistently use mdl/types instead of sdk/mpr types, reducing dependencies and preparing for the shared-types refactoring stack.
• Mock support: Comprehensive mock implementations (mock_mutation.go) enable testing without real backend dependencies.
• Bug fix addressed: The UUID generation fix (setting v4 version/variant bits) addresses a potential correctness issue in ID generation.
• Future-proof design: Interfaces are session-scoped with explicit Save(), matching existing unit modification patterns and preparing for PR refactor: implement mutation backends and migrate handlers #237 implementation.
• Conversion layer tested: mdl/backend/mpr/convert_test.go provides solid validation for type conversions between sdk/mpr and mdl/types.

Recommendation

Request changes until a test file for mdl/bsonutil/bsonutil.go is added (e.g., mdl/bsonutil/bsonutil_test.go). The test should verify:

• Valid UUID conversion to/from BSON binary
• Proper UUID v4 variant/version bit handling (tying into the mentioned bug fix)
• Error handling for invalid UUID inputs (though the function panics, tests should validate this behavior)

Once tests are added, the PR is ready for approval as it cleanly advances the shared-types refactoring goal without modifying MDL syntax or violating design guidelines. The scoped changes align with the PR's stated purpose of preparing for PR #237's implementation.

---

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

[github-actions]

AI Code Review

Critical Issues

• None found

Moderate Issues

• Placeholder panics: The MprBackend implementations contain panic() statements for unimplemented methods (e.g., OpenPageForMutation, SerializeWidget). While understandable as placeholders for PR refactor: implement mutation backends and migrate handlers #237, these could cause runtime panics if accidentally invoked before PR refactor: implement mutation backends and migrate handlers #237 merges. Consider using return nil, errors.New("not implemented") instead for safer failure mode.
• Redundant fields in PluggablePropertyContext: The struct contains both AttributePath (string) and AttributePaths ([]string) which may cause confusion about when to use each. Clearer naming or documentation would help distinguish their purposes.

Minor Issues

• Inconsistent comment spacing: Some comments have extra spaces (e.g., // - WidgetBackend in mock_infrastructure.go).
• Verbose type aliases: The conversion functions in convert.go perform deep copies despite type aliases being identical (proven in convert_test.go). While safe, this adds unnecessary overhead; direct assignment would suffice.

What Looks Good

• Clean separation of concerns: The mutation interfaces properly abstract BSON manipulation, preparing executor files for cleaner implementation in PR refactor: implement mutation backends and migrate handlers #237.
• Comprehensive interface coverage: PageMutator and WorkflowMutator cover all necessary operations (widget tree, properties, variables, layouts, activities, etc.).
• Session-scoped design: Mutators accumulate changes and commit via Save(), matching existing load-modify-save patterns.
• Bug fix: GenerateDeterministicID now correctly sets UUID v4 version/variant bits.
• Type decoupling: Consistent replacement of sdk/mpr types with mdl/types across backends, mocks, and converters reduces coupling.
• Thorough mocking: Mock implementations for all new interfaces enable testing without real backend.
• CGO-free utilities: BSON ID conversion moved to bsonutil.go avoids CGO dependencies.
• Test coverage: Conversion tests verify type safety and field preservation.
• Stack readiness: PR correctly positions itself as step 2/5 in the shared-types refactoring stack.

Recommendation

Approve
This PR successfully defines the mutation contract interfaces as intended. It reduces executor coupling to BSON internals, prepares for PR #237's implementation, and includes appropriate safeguards (mocks, bug fix). The moderate issues are minor and do not block merging, especially given the placeholder nature of unimplemented methods in the stack. The refactoring aligns with project goals and checklist requirements.

---

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

[retran]

Re: AI Reviews (comments 5 & 6) — fixes applied in 957a1e7 and 741e689:

1. Missing bsonutil tests — Added bsonutil_test.go in 741e689 with 6 tests: valid UUID conversion, panic on invalid UUID, panic on empty string, roundtrip, UUID v4 format validation, and uniqueness.

2. Typed consts for stringly-typed enums — Added ContainerKind, InsertPosition, PluggablePropertyOp typed string constants in 957a1e7. Interface methods now use typed parameters.

3. TODO comments in panic stubs — Added // TODO: implement in PR #237 to all 6 placeholder panics in 957a1e7.

4. Placeholder panics vs return error — Intentional. These are interface stubs for PR refactor: implement mutation backends and migrate handlers #237. A panic is the correct failure mode: it signals "this code path should never execute yet" rather than silently returning an error that callers might mishandle.

5. PluggablePropertyContext redundant fields — AttributePath (single) and AttributePaths (slice) serve different Mendix widget property operations (attribute vs attributeObjects). The naming matches the Mendix property model.

6. Verbose type aliases deep copy — Intentional. Field-by-field copy ensures correctness if types diverge in the future. The overhead is negligible for this use case.

7. Inconsistent comment spacing — Could not reproduce; no extra-spaced comments found in mock_infrastructure.go.

[github-actions]

AI Code Review

What Looks Good

• The PR successfully defines clean mutation interfaces (PageMutator, WorkflowMutator) that encapsulate fine-grained operations on pages and workflows
• It properly extracts shared types to mdl/types to decouple the backend from sdk/mpr dependencies
• The session-scoped design (accumulate changes, commit with Save()) matches existing patterns in the codebase
• Interface segregation is appropriate - PageMutator handles widget/ tree/ variable/ layout/ pluggable widget operations; WorkflowMutator handles properties, activities, outcomes, paths, branches, boundary events
• Mock implementations are provided for testing
• BSON ID conversion utilities are properly extracted to a CGO-free package
• Many backend files were updated to use the new types package, reducing sdk/mpr coupling
• The TODO markers in mpr/backend.go correctly indicate pending implementation in PR refactor: implement mutation backends and migrate handlers #237
• The UUID bug fix (setting proper v4 version/variant bits) addresses a correctness issue

Moderate Issues

Test Coverage Gaps

• New packages lack dedicated test files:
  • mdl/backend/mutation.go (interfaces) - no test file visible
  • mdl/bsonutil/bsonutil.go (BSON utilities) - no test file visible
• While the PR updates executor files to use the new interfaces (which should be covered by existing tests), the new packages themselves should have unit tests
• The bug fix to GenerateDeterministicID should have a test verifying UUID v4 compliance, but none is visible in the diff

Recommendation

Approve with the understanding that test coverage for the new packages and bug fix should be addressed in subsequent work (likely PR #237 where the interfaces are implemented). The refactor is well-scoped, follows the shared-types stack plan, and prepares cleanly for the implementation phase. The missing unit tests are a moderate issue but don't block approval since:

1. This is refactoring existing functionality (not adding new features)
2. Executor files using the new interfaces should be covered by existing tests
3. The PR is explicitly preparatory for implementation in PR refactor: implement mutation backends and migrate handlers #237
4. The architecture improvement is significant and correct

The PR successfully advances the shared-types refactoring goal with minimal risk.

---

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

[Copilot]

Pull request overview

Copilot reviewed 124 out of 127 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

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

• Missing test coverage for new interfaces: While mock implementations are provided, there are no visible unit tests for the new mutation.go interfaces or bsonutil.go functions in the diff. Unit tests should be added for these new packages.

• Incomplete backend wiring: The mpr/backend.go file shows panic("not yet implemented") for the new mutation interfaces, which is correct for this PR in the stack, but means the functionality isn't yet usable. This is expected since implementation comes in PR refactor: implement mutation backends and migrate handlers #237.

What Looks Good

1. Clean separation of concerns: The PR successfully defines clear interfaces (PageMutator, WorkflowMutator) that abstract away BSON manipulation details from the executor.

2. Proper abstraction layer: By introducing the WidgetSerializationBackend and mutation interfaces, the executor will no longer need to know about BSON internals.

3. Mock implementations: Good to see mock implementations for testing included.

4. Conversion layer: The mprbackend/convert.go file provides a clean way to convert between sdk/mpr types and mdl/types, keeping the backend package free of sdk/mpr dependencies as intended.

5. Bug fix: The UUID fix for GenerateDeterministicID is a valuable improvement.

6. Preparation for implementation: The PR correctly leaves the actual implementation as panic("not yet implemented") with TODO comments pointing to PR refactor: implement mutation backends and migrate handlers #237, which is the right approach for a stack.

7. Consistent naming and patterns: The interfaces follow established patterns in the codebase.

Recommendation

Approve. This PR is a clean refactor that successfully defines the mutation contract interfaces, prepares the codebase for the upcoming implementation in PR #237, includes appropriate mock implementations for testing, fixes a UUID generation bug, and maintains proper separation of concerns. The minor issue about missing unit tests is acceptable since this is preparation work and tests will likely be added when the implementation arrives in PR #237.

---

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

[retran]

Re: AI Review (comment 7) — test coverage gaps already addressed:

1. mdl/bsonutil/bsonutil.go — bsonutil_test.go added in 741e689 with 6 tests (valid UUID, panic on invalid, panic on empty, roundtrip, UUID v4 format, uniqueness).

2. GenerateDeterministicID UUID v4 compliance — TestGenerateDeterministicID_V4Bits added in f07d6fb, verifies version nibble 4 and variant nibble [89ab] across 5 seeds.

3. mdl/backend/mutation.go — contains only interface definitions and type constants, no executable logic to test.