feat: support free microflow annotations#319
Conversation
Symptom: standalone Mendix annotation notes that describe emits immediately before an activity were parsed as activity annotations and became attached to that activity on exec. Root cause: the MDL annotation model only had one annotation text field, so order-sensitive free notes and activity-bound notes were collapsed into the same AST property. Fix: add a FreeAnnotation field, classify annotations before activity metadata as free-floating when later metadata binds the activity, and preserve the free note when building the microflow graph. Tests: added parser and builder coverage for free-versus-attached annotation order, documented the feature with a draft proposal, quick-reference entry, skill guidance, and a doctype fixture checked with mxcli.
AI Code ReviewWe are reviewing a pull request that adds support for free microflow annotations. Let's go through the checklist: Overlap & duplication
MDL syntax designThe PR does not add new MDL syntax (keywords or statement types). It only changes the interpretation of existing
However, note that the PR updates the quick reference to add a line for "Free annotation". This is documentation only. Full-stack consistencySince this is a change in the interpretation of an existing annotation (not a new MDL statement), we don't expect:
We note that the feature does not require:
Looking at the changes, we see that the executor changes are in the builder (for creating/updating microflows). The DESCRIBE command is likely in a different part of the codebase (probably in the However, the PR description says: "Adds first-class support for free microflow annotations so standalone notes can roundtrip without being attached to the following activity." and the validation includes But note: the PR also adds tests in the visitor and executor that check the AST and the built flow graph. We don't see a test that does a full roundtrip (MDL -> model -> MDL) for free annotations. However, the PR does mention that the feature is for roundtrip. Given the checklist, we note that the feature is an existing MDL feature (annotation) with a change in semantics. Therefore, we don't expect new grammar or AST node types (though we do have an AST field addition). The visitor and executor are updated. We don't see LSP or DESCRIBE changes, but note that the DESCRIBE command might be unaffected because it already outputs annotations as However, the PR does not show changes to the reader (which would be in the visitor when reading from the model). But note that the visitor is used for both parsing MDL and for reading from the model? Actually, the visitor in Looking at the structure, the But note: the PR adds a test in Given the checklist, we must check for full-stack consistency for MDL features. Since this is a change in the semantics of an existing feature, we should verify that the DESCRIBE command (which is part of the full stack) works correctly. However, the PR does not show changes to the DESCRIBE command. But note: the feature is about preserving free annotations when going from MDL to model and back to MDL. The DESCRIBE command is the step that goes from model to MDL. We must ensure that when the model has a free annotation (not attached to any activity), the DESCRIBE command outputs it as an We don't see changes in the DESCRIBE command in the diff, but note that the DESCRIBE command might be using the same AST structure when reading from the model. The AST node However, the PR does not show changes to the model-to-AST visitor (which would be in the Therefore, we must check that the BSON reader (in This is a potential gap: the PR only changes the writer (MDL -> model) but not the reader (model -> MDL). However, note that the feature is about roundtripping: we want to preserve free annotations when we do MDL -> model -> MDL. The PR changes the writer so that free annotations are not attached to the next activity. But when reading the model back, we must be able to distinguish a free annotation (standing alone) from an attached one (attached to the next activity). Without changes to the reader, when we read a free annotation from the model, it would be parsed as an attached annotation (because the old logic treated every annotation as attached to the next activity). Then when we write it back as MDL, we would put it as an attached annotation (after any activity-binding metadata) which would change the meaning. Therefore, we must also update the reader to set the However, note that the PR does not show any changes to the reader. This is a critical issue for roundtripping. But wait: the PR description says: "The MDL AST had only one annotation text field, so free notes and activity-bound notes collapsed into the same property during parsing and graph building." and the fix includes:
It does not mention the reader. However, note that the visitor mentioned is the MDL visitor (for parsing MDL). The reader (for BSON) is a different piece. Given the above, we suspect that the reader might not be updated. However, let's look at the diff again: we don't see any changes to Therefore, we must flag that the reader (BSON to AST) is not updated to distinguish free and attached annotations, which breaks the roundtrip. But note: the PR adds a test in However, the PR does say that the feature is for roundtrip and includes a doctype test. The doctype test is only checked for syntax (with Given the checklist, we must check for full-stack consistency. Since the feature requires changes in both the writer and the reader, and we only see the writer, we have a problem. Test coverage
Security & robustness
Scope & atomicity
Code quality
Given the above, we have a Critical Issue regarding the roundtrip: the reader (BSON to AST) is not updated to distinguish free and attached annotations. Without this, when we read a model that contains a free annotation, it will be misinterpreted as an attached annotation, and then when written back as MDL it will be placed incorrectly (as attached) altering the diagram. However, note that the PR might be assuming that the reader already works because the BSON structure for free annotations and attached annotations is different? Let's think: In the Mendix model, a free annotation is a separate object (standalone) while an attached annotation is a property of an activity. Therefore, when reading the model, we should be able to tell by the BSON structure whether an annotation is free (a standalone object) or attached (a field in an activity object). If that is the case, then the reader might already be setting the correct field? But note the PR says: "The MDL AST had only one annotation text field, so free notes and activity-bound notes collapsed into the same property during parsing and graph building." This suggests that the AST did not have a way to distinguish, so the reader must have been putting both kinds of annotations into the same field (AnnotationText). Therefore, we must update the reader to put free annotations in the FreeAnnotation field and attached ones in the AnnotationText field. Since we don't see changes to the reader in the diff, we must flag this as a critical issue. But wait: the PR might be only about the writer? And the reader is already correct? Let me re-read the PR description: "The MDL AST had only one annotation text field, so free notes and activity-bound notes collapsed into the same property during parsing and graph building." This implies that the problem was in the AST (which had only one field) and that the parser (MDL to AST) and the graph builder (AST to model) were collapsing them. The fix is to split the AST field into two and then use the order in the MDL to decide which one to set when parsing MDL. When reading from the model, we would have to look at the BSON to decide: if the annotation is a standalone object, then it's free; if it's a property of an activity, then it's attached. The PR does not show changes to the reader, but note that the reader might already be correctly setting the annotation text to the AnnotationText field (because that's the only field that existed). Now we have two fields, so we must update the reader to set the appropriate field. Without seeing the reader code, we cannot be sure. However, given that the PR does not show any changes to the reader, and the problem was that the AST had only one field, we suspect the reader is not updated. Therefore, we raise a critical issue. But note: the PR adds a test in the visitor that builds the AST from MDL and checks the AnnotationText vs FreeAnnotation fields. This tests the MDL->AST direction. We don't see a test for the model->AST direction. Given the checklist, we must check for full-stack consistency. The feature requires changes in both directions. We only see changes in the MDL->AST and AST->model directions. We are missing the model->AST direction. RecommendationWe must request changes to update the reader (BSON to AST) to correctly set the FreeAnnotation and AnnotationText fields based on whether the annotation is a standalone object or a property of an activity. However, note that the PR might be incomplete in the diff we are seeing? But we are only given the diff in the PR. Alternatively, we might be missing that the reader is in a different part of the code that is not shown in the diff. But the diff provided does not include any changes to Given the information we have, we must flag this as a critical issue. But let's double-check the diff: we see changes in:
No changes to the reader. Therefore, we have: Critical Issues
Moderate Issues
Minor Issues
What Looks Good
RecommendationWe must request changes to update the BSON reader (in However, note that the PR might be relying on the fact that the reader already works because the BSON structure is different and the existing code already puts the annotation text in the correct place? But the PR description says the AST had only one field, implying that the reader was putting both kinds into that one field. Now we have two fields, so we must split them. Without the reader change, when we read a model that has a free annotation, the FreeAnnotation field will be empty and the AnnotationText field will be set (because the old reader put it there). Then when we write the model back as MDL, we will see AnnotationText set and treat it as an attached annotation (because the writer logic: if AnnotationText is set, we treat it as attached unless we see later activity metadata? But note: the writer logic now checks for FreeAnnotation first? Actually, in the visitor when building the AST from MDL, we set FreeAnnotation only if there is no prior activity metadata and there is a later activity annotation. When writing from AST to model, we attach the FreeAnnotation as a standalone and the AnnotationText as attached Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
The PR review questioned whether free microflow annotations were preserved when reading an existing model back through DESCRIBE. That path does not use ActivityAnnotations: parsed Microflows$Annotation objects and AnnotationFlow links are already split by collectFreeAnnotations and buildAnnotationsByTarget. Extract the free-annotation prefixing into a small helper and add a regression test that starts from a model object collection with one standalone note and one attached note. Tests: - go test ./mdl/executor -run 'TestPrependFreeAnnotationLines_ModelAnnotationsStayFree|TestFreeAnnotationBeforePositionStaysUnattached' - make build - make test - make lint-go - make test-integration
|
Addressed the review concern about the model-to-DESCRIBE side of free annotations in follow-up commit Clarification: this path does not use I extracted the free-annotation prefixing into Validation rerun:
|
AI Code ReviewCritical Issues
Moderate Issues
Minor Issues
What Looks Good
RecommendationApprove the PR. The implementation correctly solves the roundtrip issue for free microflow annotations with minimal, focused changes. Minor issues noted do not block merge but should be tracked for future refinement (e.g., supporting multiple free annotations via slice in AST). All checklist items satisfied. Automated review via OpenRouter (Nemotron Super 120B) — workflow source |
2 participants
Part of #332.
Closes #318.
Summary
Adds first-class support for free microflow annotations so standalone notes can roundtrip without being attached to the following activity.
Semantics
An
@annotation 'text'is treated as free when it appears before activity-binding metadata such as@position,@caption,@color,@excluded, or@anchor, and a later annotation binds the activity. Activity-bound annotations remain supported as before.Root cause
The MDL AST had only one annotation text field, so free notes and activity-bound notes collapsed into the same property during parsing and graph building.
Fix
ActivityAnnotations.FreeAnnotation.Tests
Added parser and builder coverage for free-versus-attached annotation order plus a doctype fixture.
Documentation
docs/11-proposals/PROPOSAL_microflow_free_annotation.mddocs/11-proposals/README.mddocs/01-project/MDL_QUICK_REFERENCE.md.claude/skills/mendix/write-microflows.mdmdl-examples/doctype-tests/free_annotation.test.mdlValidation
make buildmake testmake lint-gomake test-integration./bin/mxcli check mdl-examples/doctype-tests/free_annotation.test.mdlAgentic Code Testing
mxcli check.Test plan
FreeAnnotation.