Skip to content

fix: resolve bare change members against inherited attributes#452

Open
hjotha wants to merge 5 commits intomendixlabs:mainfrom
hjotha:submit/change-member-inherited-attribute
Open

fix: resolve bare change members against inherited attributes#452
hjotha wants to merge 5 commits intomendixlabs:mainfrom
hjotha:submit/change-member-inherited-attribute

Conversation

@hjotha
Copy link
Copy Markdown
Contributor

@hjotha hjotha commented Apr 30, 2026

Closes #451.

Summary

  • resolve bare change-object members against the entity generalization chain before falling back to the concrete entity
  • preserve the declaring entity qualified name for inherited attributes
  • add a synthetic regression test for a child entity changing an attribute declared by its parent

Validation

  • make build
  • make test
  • targeted roundtrip with mx check for an inherited bare attribute case

@ako ako added this to the v0.8.0 milestone Apr 30, 2026
@retran retran mentioned this pull request May 1, 2026
Draft
@hjothamendix
fix: stabilize integration CI baseline
9ebb2d7 
Symptom: all open PRs against main failed the shared build-and-test job in
make test-integration, even when their local build/test/lint validation passed.
The failures reproduced on origin/main, so they were baseline CI instability
rather than PR-specific regressions.

Root cause: TestWatcherDebounce could allow stale timer callbacks to send an
extra message under slow scheduling, nanoflow integration fixtures used MDL
syntax that no longer matched the grammar, and the doctype mx-check harness did
not classify known page/nanoflow showcase consistency errors as expected
limitations.

Fix: guard watcher debounce callbacks with a generation counter, tighten the
watcher burst test, update nanoflow fixtures to current MDL syntax, and extend
the known consistency-error allowlist for showcase-only limitations.

Tests: make build
Tests: go test ./cmd/mxcli/tui -run TestWatcherDebounce -count=20 -v
Tests: ./bin/mxcli check mdl-examples/doctype-tests/02b-nanoflow-examples.mdl
Tests: go test -tags integration -count=1 -timeout 30m ./mdl/executor -run 'TestRoundtripNanoflow_(Loop|EnumParameter|Annotations)|TestMxCheck_DoctypeScripts/02b-nanoflow-examples.mdl|TestMxCheck_DoctypeScripts/03-page-examples.mdl' -v
Tests: make test
Tests: make lint-go
Tests: make test-integration
@hjothamendix
fix: resolve bare change members against inherited attributes
ca7c6f0 
Symptom: a change-object member assignment to an inherited attribute could roundtrip into an attribute qualified on the concrete child entity, causing mx check to report that the selected attribute no longer exists.

Root cause: resolveMemberChange qualified bare attributes with the variable's concrete entity only and did not walk the entity generalization chain.

Fix: when entity metadata is available, resolve bare attribute names against the entity and its ancestors and write the declaring entity qualified name.

Tests: added a synthetic inherited-attribute resolver test, ran make build, make test, and a targeted roundtrip with mx check.
@hjothamendix
Merge remote-tracking branch 'fork/submit/stabilize-integration-ci' i…
2054769 
…nto HEAD
@hjotha hjotha force-pushed the submit/change-member-inherited-attribute branch from 27cf265 to 2054769 Compare May 2, 2026 10:04
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 2, 2026

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

  • Unrelated changes in PR: The PR includes modifications to watcher.go/watcher_test.go (debounce improvement) and simplification of 02b-nanoflow-examples.mdl that are not mentioned in the issue description (Change-object writer qualifies inherited bare attributes on the child entity #451). While these are harmless and potentially beneficial, they deviate from the single-change principle. However, they are trivial and do not interfere with the core fix.
  • Test file naming: The new test cmd_microflows_change_inherited_attribute_test.go uses snake_case inconsistently with surrounding test files (which use camelCase, e.g., cmd_microflows_builder_actions.go). This is a minor style inconsistency but doesn't affect functionality.

What Looks Good

  • Targeted fix: The core change correctly implements inheritance-aware bare attribute resolution in change objects via resolveAttributeInEntityHierarchy, with cycle detection and proper fallback.
  • Comprehensive test: The new unit test explicitly verifies that bare attributes in change objects resolve to the declaring entity's qualified name for inherited attributes, covering the regression scenario.
  • Clean implementation: The helper method is well-encapsulated, handles edge cases (nil/empty inputs, cycles), and integrates cleanly into the existing resolution flow.
  • No syntax changes: Since this is a behavioral fix (not new MDL syntax), full-stack consistency checks are not applicable, avoiding unnecessary churn.
  • Test updates: Roundtrip test adjustments appropriately ignore pre-existing CE errors unrelated to this fix.

Recommendation

Approve the PR. The minor issues (unrelated changes, test naming) are trivial and do not compromise the fix's correctness or safety. The core change addresses the issue precisely, includes robust testing, and maintains code quality. Consider splitting unrelated changes into a separate PR in future work for stricter atomicity, but it's not blocking here.

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

@hjotha
Copy link
Copy Markdown
Contributor Author

hjotha commented May 2, 2026

CI stabilization note: merged #412 into this branch so it uses the same integration-CI baseline as the rest of the active PR set.

@hjothamendix
fix: parse explicit Java action void returns as void
62008a7 
Symptom: CI failed in TestMxCheck_DoctypeScripts/empty_java_action_argument.mdl because a Java action declared as RETURNS Void was written as an entity return type named .void, and Studio Pro reported CE1613.

Root cause: the generic data-type visitor treats bare qualified names as entity/enumeration references. Java action return types reused that generic path, so the explicit Void spelling became a qualified name instead of ast.TypeVoid.

Fix: add a Java-action return-type wrapper that maps unqualified Void to ast.TypeVoid while leaving generic data-type parsing unchanged for parameters and attributes.

Tests: added visitor coverage for explicit RETURNS Void; verified mxcli check for the doctype fixture and the targeted integration subtest that failed in GitHub Actions.
@hjothamendix
Merge remote-tracking branch 'fork/submit/stabilize-integration-ci' i…
8f2a8d3 
…nto HEAD
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 2, 2026

AI Code Review

Critical Issues

None found.

Moderate Issues

  • Scope & atomicity: The PR includes three distinct changes:

    1. Fix for bare change-member resolution (core feature)
    2. TUI watcher debounce improvement (unrelated infrastructure change)
    3. Nanoflow doctype test rewrite (unrelated test update)

    The PR description only mentions the bare change-member fix. Per checklist: "Each PR is scoped to a single feature or concern — if the description needs 'and' between unrelated items, split it." These should be separate PRs.

Minor Issues

  • Test coverage: The nanoflow doctype test file (02b-nanoflow-examples.mdl) was reduced from 86 lines to 22 lines. While the new file covers key concepts, it removes many specific examples (validation, loops, messaging, etc.) that were previously tested. The roundtrip tests were updated accordingly, but we should verify the removed examples aren't needed for regression coverage.
  • Code quality: The new resolveAttributeInEntityHierarchy function lacks documentation comments explaining its purpose and return values.

What Looks Good

  • The core fix correctly walks the generalization chain to resolve bare attributes in change objects, preserving the declaring entity's qualified name.
  • The synthetic test (cmd_microflows_change_inherited_attribute_test.go) adequately verifies the inherited attribute scenario.
  • The TUI watcher debounce improvement is a valid robustness enhancement for file watching.
  • Java action return type handling improvements are unrelated but correct.

Recommendation

Request changes to split this PR into:

  1. Core fix: Bare change-member resolution fix + synthetic test
  2. Infrastructure: TUI watcher debounce improvement (separate PR)
  3. Test update: Nanoflow doctype test rewrite (separate PR, with justification for example removal)

If the nanoflow test changes are deemed necessary for the core fix (unlikely), at least update the PR description to explain all changes and split the TUI watcher change into its own PR. The core fix itself is ready to merge once scope issues are resolved.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.8.0

Development

Successfully merging this pull request may close these issues.

Change-object writer qualifies inherited bare attributes on the child entity

3 participants

@hjotha @ako @hjothamendix