MagicAST restructure: Phase 5 — Optional DTO layer for AST/JSON decoupling

[Spelkington]

Parent

Part of the MagicAST restructure milestone. Phase 5 of 5. Marked ready-for-human because the decision to do this phase at all is a HITL judgment call gated on observable diagnostic quality after Phase 2 ships.

What to build

After Phase 2 (#3) lands, the worst of the AST↔JSON coupling is already gone: base classes no longer carry [JsonPolymorphic] lists, and concrete types carry domain-flavored [OracleAbility] / [OracleEffect] attributes instead of JSON-flavored ones. However, individual AST records may still carry per-property JSON attributes:

• [JsonPropertyName("...")]
• [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
• [JsonConverter(...)]

The open question: are the post-Phase-2 test diagnostics good enough as they are?

When an agent's hand-parsed JSON has an unmapped field or an unknown discriminator, the JsonException they see today references JSON-layer vocabulary (property names, types). If that's clear enough for the TDD loop, this phase is unnecessary and should be closed as wontfix.

If diagnostics still leak JSON-internal vocabulary in a way that confuses agents reading red test output, introduce a thin DTO mirror layer:

libs/magic-ast/
├── AST/                    ← pure domain records, ZERO [Json*] attributes
└── Serialization/
    └── Dto/                ← mirrors of each AST record, carrying JSON attributes
        ├── AbilityDto.cs
        ├── EffectDto.cs
        └── ...             ← each Dto has ToDomain() and FromDomain() methods

The test infrastructure serializes via DTO; the parser produces pure AST. Error messages can be wrapped to reference domain types and discriminators instead of JSON property names.

Acceptance criteria — Decision phase (do this first)

• Run the existing TDD loop end-to-end on a sample hand-parsed card with an intentionally-introduced schema gap (e.g., add an unknown \"kind\" discriminator or an unmapped field).
• Capture the actual error message produced.
• Document the decision in the PR description or as a comment on this issue: either (a) good enough — close as wontfix, or (b) introduce DTO layer.

Acceptance criteria — If proceeding (path b)

• All AST records under libs/magic-ast/AST/ contain zero [Json*] attributes (including per-property ones).
• A parallel DTO hierarchy under libs/magic-ast/Serialization/Dto/ (or similar) carries the JSON attributes.
• Each DTO has ToDomain() and FromDomain(domain) methods (or equivalent mapping). No reflection required for mapping unless it's clearly the right call.
• Test infrastructure uses the DTO for round-trip serialization; parser output is compared as pure-domain AST.
• Error messages on schema gap reference domain vocabulary (e.g., "unknown ability kind: 'delayed-trigger'") rather than JSON-internal vocabulary.
• Full ratchet test suite remains green or improves — no regressions in test-baseline.json.

Blocked by

• MagicAST restructure: Phase 2 — JSON polymorphic converter via attribute scan #3 (Phase 2 — must land first; decision criteria depend on observing post-Phase-2 diagnostic quality)
• MagicAST restructure: Phase 4 — Trait composition for Effect #5 (Phase 4 — must land first; trait-bearing effects need to round-trip through whatever serialization layer this phase establishes)

[Spelkington]

Decision: close as wontfix. Phase 2 diagnostics are good enough.

Per the acceptance criteria's decision phase, I ran the experimental schema-gap scenarios against the post-Phase-2 / post-Phase-4 codebase (branch refactor/mast-phase-5-dto-decision). Both common failure modes produce domain-flavored, actionable error messages without requiring a DTO mirror layer.

Experiment

Mutated tools/test/magic-ast/Data/HandParsedCards/LlanowarElves.json (a known-passing fixture) and captured the resulting JsonException messages.

Mutation #1 — unknown discriminator value

Changed "kind": "activated" to "kind": "delayedTrigger" (a discriminator no AST type registers).

Error produced:

System.Text.Json.JsonException : Unknown Ability discriminator 'delayedTrigger'. Known: activated, modal, spell, static, triggered, unparsed.
   at MagicAST.Serialization.PolymorphicReflectionConverter`1.Read(...)

This is the custom error message authored in PolymorphicReflectionConverter.Read (Phase 2). It:

• Names the AST polymorphic base (Ability) — domain vocabulary, not JSON vocabulary.
• Names the offending value ('delayedTrigger').
• Enumerates the full set of valid discriminators.
• Stack trace points directly at the converter for code navigation.

Mutation #2 — unmapped JSON property

Added "madeUpField": 42 to an addMana effect node.

Error produced:

System.Text.Json.JsonException : The JSON property 'madeUpField' could not be mapped to any .NET member contained in type 'MagicAST.AST.Effects.Resource.AddManaEffect'.

This is the built-in .NET JsonUnmappedMemberHandling = Disallow behavior (inherited from JsonSerializerOptions.Strict). It:

• Names the offending property ('madeUpField').
• Names the target AST type fully (MagicAST.AST.Effects.Resource.AddManaEffect) — the namespace path is exactly the relative file path under libs/magic-ast/AST/.
• Action is unambiguous: either add the property to the AST type, or remove it from the JSON.

The only mildly C#-flavored phrase is ".NET member" instead of "AST node field." Not friction enough to justify ~50 mirror classes plus mapper methods.

Why a DTO layer is unnecessary

The original concern was that JSON-internal vocabulary (JsonElement, Utf8JsonReader, property-name escape codes, etc.) would leak into the error messages an agent reads to decide between "fix my JSON" and "extend the AST." That doesn't happen in practice:

• Phase 2's custom polymorphic converter already produces a domain-flavored message for the most common case (unknown discriminator).
• JsonUnmappedMemberHandling = Disallow already produces a clean message for unmapped properties — naming the property and the target type without exposing JSON parser internals.
• Phase 4's trait split means a "missing field" maps cleanly to "this effect type doesn't implement the relevant trait" — implementer just adds the interface, no DTO ceremony needed.

What a DTO layer WOULD have cost

For comparison, had we proceeded:

• ~50 mirror DTO records (one per concrete AST type plus the bases)
• 50 ToDomain() / FromDomain() mapper methods
• Test infrastructure rewrite to round-trip through the DTO layer
• ~1 day of implementation + maintenance cost on every future AST node addition (agents would have to update both the AST record and its DTO mirror)

Per the issue body's guidance ("if the diagnostic quality after Phase 2 is good enough, this phase should be skipped"), I'm closing this issue as wontfix.

Follow-up: if diagnostics ever degrade

If a future change makes the schema-gap diagnostics worse (e.g., a new converter that throws raw JsonException without context), reopen this issue. The cost of introducing a DTO layer later is the same as introducing it now — it's not load-bearing for any other phase.

[Spelkington]

Closed as wontfix per the decision-phase outcome documented above. Post-Phase-2 diagnostics are sufficient — reopen if they degrade.