fix: emit list retrieve for owner-both reverse references

[hjotha]

Closes #380.

Summary

• Treat explicit-owner reverse Reference traversal, including Owner=Both, as a database retrieve with XPath when the parent entity is persistable.
• Keep association-source fallback for missing ownership metadata or non-persistable parent entities.
• Preserve list typing for child-side Reference traversal even when fallback source syntax is used.

Validation

• go test ./mdl/executor -run 'TestAddRetrieveAction_ReverseReference'
• make build
• make test

[github-actions]

AI Code Review

Critical Issues

None found.

Moderate Issues

None found.

Minor Issues

None found.

What Looks Good

• The PR addresses a specific issue (Reverse Reference retrieve with owner-both association is typed as single object #380) regarding reverse reference traversal for Owner=Both associations
• Changes are focused and minimal, touching only the necessary logic in the microflow builder
• Test updates properly reflect the new behavior:
  • Renamed test to match expected outcome (OwnerBothUsesDatabaseSource)
  • Changed expected source type from AssociationRetrieveSource to DatabaseRetrieveSource
  • Updated variable name from "Parent" to "Parents" to correctly reflect list result
  • Added new test case for unknown owner preserving association source behavior
• Variable typing logic correctly handles list vs single object results based on traversal direction
• Maintains backward association-source fallback for edge cases (missing ownership or non-persistable parents)
• Follows existing code patterns and conventions in the executor package
• No changes to MDL syntax, grammar, or AST needed (appropriate for a behavioral fix)
• Test coverage includes both positive and negative cases

Recommendation

Approve the PR. The changes correctly implement the fix for emitting list retrieve for owner-both reverse references while preserving existing fallback behavior. The test coverage is adequate and validates both the new behavior and edge cases. No syntax or architectural concerns were identified.

---

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

[ako]

PR #381 — fix: emit list retrieve for owner-both reverse references

Reviewed against the CLAUDE.md checklist.

---

No Blockers

The core fix is correct. Owner=Both (symmetric association) is still a one-to-many relationship when traversed in reverse (child → parent), so using DatabaseRetrieveSource with an XPath constraint is the right output. The old != AssociationOwnerBoth condition incorrectly routed it to AssociationRetrieveSource.

---

Moderate Issues

1. Missing mdl-examples/bug-tests/ file

The checklist requires a bug-test MDL script for every bug fix that is verifiable in Studio Pro:

"every bug fix should include an MDL test script in mdl-examples/bug-tests/ that reproduces the issue, so the fix can be verified in Studio Pro if applicable"

The fix changes how ChangeAction BSON retrieve actions are built — directly affecting what ends up in the MPR. A short script that creates an entity with a Both-owned association and retrieves via reverse traversal, with a note to run mx check, would be the natural companion.

---

Minor Issues

2. Owner="" behavioural change is implicit in the condition swap

Changing != AssociationOwnerBoth to != "" has a second effect beyond the stated fix: associations where Owner is empty (missing metadata) now fall through to AssociationRetrieveSource instead of DatabaseRetrieveSource. Previously "" != "Both" was true, so empty-owner associations would have entered the database-retrieve path.

The new test TestAddRetrieveAction_ReverseReferenceUnknownOwnerPreservesAssociationSource correctly documents this new behaviour, and the PR summary mentions "missing ownership metadata" as an intended fallback case. The only gap is that the summary doesn't explicitly note this is also a behaviour change from the previous state (not just a preservation of pre-PR-356 behaviour). A one-line note in the PR description or a code comment at the != "" guard (// Owner="" means metadata was unavailable; fall back to association source) would make the intent self-evident for future readers.

---

Positive Notes

• List typing fix in the else branch ("List of " + otherEntity for reverse traversal) is correct and appropriately scoped — it applies only when startVarType == assocInfo.childEntityQN.
• Three test cases cover the three distinct owner states (Both → database source, "" → association source, non-persistable → association source), and the NonPersistableParent test correctly updated its varType assertion to "List of Sample.Parent". ✓
• No grammar, AST, or BSON writer changes needed — the fix is entirely in the routing logic. ✓

---

Summary: No blockers. The fix is correct and test coverage is solid. Two gaps: missing mdl-examples/bug-tests/ file (checklist item), and the implicit Owner="" behaviour change warrants a one-line comment at the guard.