Skip to content

Review: Tier 1 — YAML-First Content Authoring (r1)

This review checks docs/designs/authoring/tier1-yaml-first-authoring.md against the current MAID codebase.

Issues

1) BLOCKER — DataDrivenContentPack.on_load() does not match the actual Pipeline API

  • Section reference: §3.2, lines 317-354
  • What's wrong: The proposed implementation builds Pipeline(phases=self.phase_order(), timeout=self.pipeline_timeout) and then calls await pipeline.run_for_pack(pack=self, data_paths=[...]). That does not work against the current API. Pipeline.run_for_pack() requires either an explicit LoaderContext or a default_context; otherwise it raises ValueError("run_for_pack requires context") (packages/maid-engine/src/maid_engine/loader/pipeline.py:156-177). The same snippet also imports SemanticRule from loader.rules.builtin, but SemanticRule actually lives in loader.rules.__init__ (packages/maid-engine/src/maid_engine/loader/rules/__init__.py:1-28).
  • Suggested fix: Redesign the base class around a real LoaderContext/LoaderConfig construction path, or explicitly require GameEngine.create_data_pipeline() plus pack-specific context mutation. Update the imports and show runnable code, not pseudocode that would fail immediately.

2) HIGH — The design promises hooks/config knobs that its own class implementation never applies

  • Section reference: §3.2, lines 187-193, 258-273, 317-388; Appendix B, lines 3554-3578
  • What's wrong: strict_validation, custom_rules(), and pipeline_context_extras() are documented as first-class extension points, but the proposed on_load() never uses them. It never creates a LoaderConfig(strict_mode=...), never merges custom_rules() into context.semantic_rules, and never applies pipeline_context_extras() anywhere. fail_on_warnings is the only knob actually wired. This is an internal contradiction inside the design doc itself.
  • Suggested fix: Either (a) show the full context-building code that applies these hooks, or (b) remove the hooks from the API until the design specifies exactly where they land in LoaderContext/LoaderConfig.

3) HIGH — The stated data-path support is broader than the proposed implementation

  • Section reference: §3.1, lines 123-127; §3.2, lines 202-221
  • What's wrong: The goals say the base class supports both package-relative data (src/my_pack/data/) and project-root data (data/) layouts. The proposed data_paths property only checks Path(mod.__file__).parent / self.data_dir_name and returns [] otherwise. There is no fallback for project-root data/, workspace roots, installed packages, or explicit override storage beyond subclassing the property.
  • Suggested fix: Specify the exact discovery order and implement it in the design: explicit override → module sibling → project root → configured search paths. If project-root support is not actually intended, remove the claim.

4) BLOCKER — The document standardizes _schema, but the loader only reads _meta.schema

  • Section reference: §4.1, lines 642-650; every schema example in §4.2-§4.10; Appendix A, lines 3455-3463
  • What's wrong: The doc consistently uses top-level _schema: maid:<type>:v1. The current loader does not read _schema at all. PreparePhase._resolve_schema() only checks doc.data.get("_meta").get("schema"), then falls back to top-level keys/directory inference (packages/maid-engine/src/maid_engine/loader/phases/prepare.py:220-267). As written, the document's canonical examples would not behave the way the doc says.
  • Suggested fix: Pick one syntax and align both code and document. Either add _schema support to PreparePhase, or rewrite every example to use _meta: { schema: ... }.

5) BLOCKER — The “standardized YAML schemas” do not match the loader’s actual input shape

  • Section reference: §4.2-§4.4, lines 676-821 and 972-1108; Appendix A, lines 3476-3527
  • What's wrong: The document treats fields like name, short_desc, long_desc, npc, health, item, weapon, etc. as first-class top-level schema fields. The current loader does not normalize those aliases into ECS components. PreparePhase only validates a components mapping (packages/maid-engine/src/maid_engine/loader/phases/prepare.py:139-172,384-431). There is no translation layer from health: to HealthComponent, npc: to NPCComponent, or name: to DescriptionComponent. So the examples are not merely “future ideas”; they are structurally incompatible with the current loader contract.
  • Suggested fix: Either define a normalization/assembly layer that converts the ergonomic YAML shape into component instances, or make the schema examples honest and component-oriented (components: { DescriptionComponent: ..., NPCComponent: ... }).

6) HIGH — Even where schemas overlap conceptually, many example field names do not match real models/components

  • Section reference: §4.2-§4.4, especially lines 729-776, 981-1108
  • What's wrong: Several examples use fields that do not line up with actual component/model definitions:
  • ExtendedRoomComponent examples use extended.time_descriptions, season_descriptions, details, and atmosphere; the actual model uses ExtendedDescriptions.time_variants, season_variants, random_details/conditional_details, and atmosphere_text (packages/maid-stdlib/src/maid_stdlib/components/extended_room.py:727-752,891-927). The example also uses day, but the enum values are MORNING, NOON, AFTERNOON, etc. (extended_room.py:32-121).
  • WeaponComponent example uses damage_min, damage_max, weapon_class, two_handed, critical_multiplier; the actual component exposes damage_dice, weapon_type, is_two_handed, crit_chance, crit_multiplier (packages/maid-classic-rpg/src/maid_classic_rpg/components/items.py:16-30).
  • ArmorComponent example uses defense_bonus; the actual component uses armor_value (items.py:32-43).
  • ItemComponent example uses stackable and equip_slot; the actual stdlib component uses max_stack and wear_slots (packages/maid-stdlib/src/maid_stdlib/components/core.py:440-463).
  • Suggested fix: Generate schema docs from real Pydantic/component definitions, or specify an explicit adapter layer with a compatibility table and tests for every alias.

7) BLOCKER — The doc promises entity types the loader cannot currently represent

  • Section reference: §4.5-§4.10, lines 1111-1526; §8.4, lines 2696-2732
  • What's wrong: The design treats monster, quest, zone, spell, skill, and shop as standardized Tier 1 YAML types. The actual loader only ships room, npc, item, and template entity configs (packages/maid-engine/src/maid_engine/loader/entity_types.py:7-44). There are no built-in entity type configs for those additional types, and no loaded content pack in the repository implements DataLoaderPack to contribute them (packages/maid-engine/src/maid_engine/loader/protocols.py:13-27; search shows no production pack implementing get_entity_type_configs() / get_semantic_rules() / get_data_paths()).
  • Suggested fix: Narrow the design scope to the four supported types for v1, or add a concrete plan for how each new type is modeled, registered, validated, persisted, and loaded by specific packs.

8) HIGH — “Atomic transactions” are overstated; reference registration is not rolled back cleanly

  • Section reference: Executive Summary, lines 33-37; §11.2, lines 3168-3170
  • What's wrong: The document repeatedly leans on atomic load/reload behavior. Current staging is only partially atomic. InstantiatePhase uses LoadTransaction, but PreparePhase registers symbolic refs directly into the global ReferenceRegistry before instantiation (packages/maid-engine/src/maid_engine/loader/phases/prepare.py:191-194). LoadTransaction.track_reference() exists but is never used (packages/maid-engine/src/maid_engine/loader/staging.py:34-36). If later phases fail, entity creation rolls back, but those early registry writes are not transactionally tied to the staged load.
  • Suggested fix: Stage reference registrations inside the transaction, or create a per-run registry snapshot and publish it only on commit. Do not promise atomic swaps until symbolic refs are actually part of the transaction boundary.

9) HIGH — Persistence compatibility is claimed, but the current instantiate path does not do any reconciliation

  • Section reference: §8.6, lines 2749-2758
  • What's wrong: The document says InstantiatePhase checks DataProvenanceComponent, compares definition_hash, skips unchanged entities, and updates/recreates changed ones. None of that exists. InstantiatePhase.execute() hardcodes resolve_load_action(EntityLoadState.NEW, ...), never inspects existing entities, never reads DataProvenanceComponent, and simply creates new staged entities (packages/maid-engine/src/maid_engine/loader/phases/instantiate.py:21-89). The reconciliation enums live in loader.models, but the algorithm is not wired into instantiation (packages/maid-engine/src/maid_engine/loader/models.py:176-206).
  • Suggested fix: Treat persistence reconciliation as a prerequisite design section, not a solved problem. Specify lookup strategy, hash comparison, stale-instance handling, quarantine behavior, and how persisted mutable state survives definition refreshes.

10) HIGH — Cross-pack @ref: syntax and resolution behavior do not match the real resolver

  • Section reference: §10.7-§10.8, lines 3059-3105; Appendix A, lines 3529-3535
  • What's wrong: The doc standardizes @ref:pack:stdlib:item/healing_potion and describes deferred second-pass resolution plus global fallback. The current resolver does none of that. It supports @ref:uuid:..., @ref:type/id, and @ref:<pack_name>:type/id (packages/maid-engine/src/maid_engine/loader/phases/resolve_refs.py:154-217). There is no reserved pack: prefix, no deferred unresolved queue, no second pass after all packs load, and no global cross-pack fallback when strict mode is off.
  • Suggested fix: Align the syntax with the real resolver or explicitly redesign the resolver and dependency-loading order. If deferred cross-pack resolution is required, specify the lifecycle and failure semantics in detail.

11) HIGH — Rule suppression/configuration is documented, but unsupported by the implementation

  • Section reference: §6.4-§6.5, lines 2154-2232; Appendix A, line 3488
  • What's wrong: The design documents per-pack/per-file rules: config in _meta.yaml, per-entity _suppress, severity overrides, and a rich --list-rules CLI. None of that is implemented. The loader only has a global LoaderConfig.skip_rules set (packages/maid-engine/src/maid_engine/loader/models.py:119-135), and PreparePhase only checks if rule.id in context.config.skip_rules (packages/maid-engine/src/maid_engine/loader/phases/prepare.py:197-208). The existing CLI just prints the two built-in rules (packages/maid-engine/src/maid_engine/cli/app.py:2827-2840).
  • Suggested fix: Either specify the actual plumbing for _meta.rules, _suppress, and severity overrides, or move this section to “future work.” Right now the document reads as if these features already fit the current architecture, but they do not.

12) MEDIUM — The rule implementation example is written against APIs that do not exist

  • Section reference: §6.3, lines 2066-2150
  • What's wrong: The sample BidirectionalExitRule calls context.get_entity_by_id(target_id) and refers to entity.source_line. LoaderContext exposes no get_entity_by_id() helper (packages/maid-engine/src/maid_engine/loader/models.py:156-174), and EntityDefinition has line, not source_line (models.py:64-78). This makes the example misleading for anyone trying to implement a real rule.
  • Suggested fix: Rewrite the sample against actual structures: use context.entity_definitions, context.reference_registry, and entity.line. If helper APIs are desirable, add them explicitly to the design before using them in examples.

13) MEDIUM — Multiple examples rely on world/engine APIs that are not present

  • Section reference: §3.3 advanced pack, lines 572-586; §8.3 after-example, lines 2678-2688
  • What's wrong: The examples use world.get_entities_by_tag("creature"), world.get_entity_by_name("Elder Miriam"), and engine.world.component_registry.register(...). The current World API does not provide get_entities_by_tag() or get_entity_by_name() (packages/maid-engine/src/maid_engine/core/world.py:84-260,408-...), and the component registry is on GameEngine.component_registry, not World (packages/maid-engine/src/maid_engine/core/engine.py:344-346). These examples will push implementers toward nonexistent interfaces.
  • Suggested fix: Update examples to use real APIs, or explicitly add the missing convenience APIs to the design and implementation plan.

14) MEDIUM — The “helpful errors” promise is stronger than the current source-map implementation supports

  • Section reference: §2.5, lines 99-114; §11.1, lines 3125-3133
  • What's wrong: The design promises file path, line number, field path, suggestions, and near-entity diff output. Current source maps are only a shallow “top-level key → line” map built with a regex (packages/maid-engine/src/maid_engine/loader/phases/parse.py:196-203). Many loader errors use line=1 or field_path=None (for example unresolved refs in resolve_refs.py:177-186 and component validation in prepare.py:417-425). That means the builder experience described here is materially ahead of the actual diagnostic substrate.
  • Suggested fix: Either tone down the guarantees or add a real nested source-map model before treating this UX as part of the Tier 1 contract.

15) MEDIUM — CLI/watch sections ignore the repo’s current runtime model

  • Section reference: §11.1-§11.4, lines 3111-3235
  • What's wrong: The proposed commands assume access to “currently loaded engine state” and live reload of a running server. Current maid data commands are largely offline helpers that build a fresh engine instance (packages/maid-engine/src/maid_engine/cli/app.py:3036-3150), and there is no data diff, data watch, data export, or data init command. The watcher note is also inaccurate: the repo depends on watchfiles, not watchdog (packages/maid-engine/pyproject.toml:61-62).
  • Suggested fix: Decide whether these commands are offline-authoring tools or remote admin operations against a running engine. If they need live state, define the transport/API and auth story. Also align the dependency note with the actual watcher stack.

16) LOW — The rule-count math contradicts itself

  • Section reference: §6.2 and §6.5, lines 2002-2005 and 2232
  • What's wrong: The section headline says “25 rules,” but the detailed listing totals 29 rules (10 + 7 + 4 + 4 + 4 = 29), and the CLI example explicitly says “Total: 29 rules.” This is a small doc-quality problem, but it erodes confidence in a design that already asks readers to trust lots of enumerated IDs.
  • Suggested fix: Fix the counts everywhere and keep the section title, tables, and example output in sync.

Overall assessment

The document has a solid product direction, but it currently reads as if the ergonomic YAML authoring layer is a thin wrapper over the existing loader. It is not. The biggest missing piece is a normalization layer between builder-friendly YAML and the loader’s current component-centric internals, plus real persistence reconciliation and a runnable DataDrivenContentPack integration path.