Skip to content

Tier 1 YAML-First Authoring Review — Round 2

Reviewer stance: fresh review, no prior context assumed
Document reviewed: docs/designs/authoring/tier1-yaml-first-authoring.md
Codebase verified against: /home/qworg/Development/MAID/packages/

Verdict

NOT SHIP

The document is improved in a few areas (@ref: syntax, World API caveats, and the explicit note that reconciliation is not implemented yet), but it still contains multiple serious mismatches with the actual MAID codebase. The biggest remaining problems are that the doc presents a non-existent DataDrivenContentPack implementation as if it were real, and its author-friendly YAML examples still do not match the actual loader path that exists today.

Quick status against requested checks

Check Result Notes
1. DataDrivenContentPack.on_load() constructs a real LoaderContext FAIL Snippet is not implementable as written; the class/module do not exist in repo.
2. All YAML examples use _meta.schema FAIL Two _schema examples remain.
3. Assembly Layer vs component-centric examples FAIL Doc describes a first-class Assembly Layer, but real pipeline still consumes component-centric components: payloads.
4. Entity types scoped to room/npc/item/template for v1 FAIL Doc still treats monster/quest/zone/spell/skill/shop as quasi-standard in several places.
5. Hooks actually wired FAIL Documented hooks are not implemented in code.
6. Component field names match real models PARTIAL FAIL Some fields match; several examples still do not.
7. @ref: syntax matches resolver PASS / PARTIAL Syntax matches; dependency-enforcement claim does not.
8. Rule suppression scoped to what exists FAIL Global skip_rules exists; _suppress is still documented like a real YAML feature.
9. Persistence reconciliation honestly described PASS / PARTIAL Caveat that reconciliation is not implemented is honest; supporting component definitions are still inaccurate.
10. World API calls use real methods PASS The examples I checked use real methods and correctly warn that name/tag helpers do not exist.
11. Source of Truth section present and coherent PARTIAL PASS Present and directionally coherent, but concrete component definitions are still inaccurate.

Issues

BLOCKER — §4.2, §4.3, §4.4, §12.4, §13.2: DataDrivenContentPack is presented as real implementation, but no such implementation exists

What's wrong

The document presents DataDrivenContentPack as if it is an implemented base class with working on_load() behavior, working hooks, and loader integration. It also claims ContentPackLoader can auto-generate a DataDrivenContentPack when a pack has manifest.toml but no pack.py.

That is not true in the codebase:

  • there is no maid_engine.plugins.data_driven module,
  • there is no DataDrivenContentPack class,
  • ContentPackLoader._load_from_directory() still expects pack.py and only loads a real ContentPack implementation.

This is not a minor wording problem: the document describes current mechanics that simply do not exist.

Section references

  • Doc: tier1-yaml-first-authoring.md:280-579
  • Doc: tier1-yaml-first-authoring.md:807-817
  • Code: packages/maid-engine/src/maid_engine/plugins/protocol.py:30-220
  • Code: packages/maid-engine/src/maid_engine/plugins/loader.py:229-255

Suggested fix

Pick one:

  1. Implement it for real (maid_engine/plugins/data_driven.py, loader support, tests), then keep the current tone.
  2. Recast the whole section as proposed design, not current behavior. Use future tense everywhere and remove statements that imply the repo already has this feature.
  3. Remove the zero-Python auto-generation example until ContentPackLoader actually supports it.

BLOCKER — §5, §6.2, §6.3, §6.4: the documented “Assembly Layer” is not wired into the real pipeline, so author-friendly YAML examples would not load as described

What's wrong

The doc says PreparePhase has an Assembly Layer that converts top-level author-friendly fields like name, short_desc, health, npc, extended, location, etc. into the component-centric components: structure.

The actual code does not do that:

  • PreparePhase still reads expanded.get("components", {}) directly.
  • InstantiatePhase instantiates whatever is already in definition.components.
  • EntityAssembler exists, but it is not invoked by PreparePhase or InstantiatePhase.
  • EntityTypeConfig does not carry the documented assembly_rule hook.

As a result, the doc's primary author-facing YAML examples are not backed by the current loader path.

Section references

  • Doc: tier1-yaml-first-authoring.md:824-1023
  • Code: packages/maid-engine/src/maid_engine/loader/phases/prepare.py:139-172
  • Code: packages/maid-engine/src/maid_engine/loader/phases/instantiate.py:33-69
  • Code: packages/maid-engine/src/maid_engine/loader/assembler.py:15-80
  • Code: packages/maid-engine/src/maid_engine/loader/models.py:139-145

Suggested fix

Either:

  • wire a real assembly step into PreparePhase and/or InstantiatePhase, including explicit field mapping support and tests, or
  • rewrite the examples to use the actual current shape (components:) and move the Assembly Layer to a future-work section.

HIGH — §4.2: the “real LoaderContext” fix is still wrong as written (LoaderContext is slotted; _pack_extras assignment would fail)

What's wrong

The doc explicitly claims the revised on_load() now builds a real LoaderContext, then shows:

context._pack_extras = self.pipeline_context_extras()

But LoaderContext is declared as @dataclass(slots=True). Adding arbitrary attributes not declared in the dataclass is not allowed. That assignment would raise AttributeError.

So even the "fixed" snippet is not valid against the real LoaderContext type.

Section references

  • Doc: tier1-yaml-first-authoring.md:394-400, 445-477, especially 475
  • Code: packages/maid-engine/src/maid_engine/loader/models.py:156-174

Suggested fix

Add an explicit field to LoaderContext (e.g. pack_extras: dict[str, Any] = field(default_factory=dict)) or stop claiming ad-hoc attributes can be attached to it.

HIGH — §4.2: built-in semantic rules would be double-registered in the documented DataDrivenContentPack implementation

What's wrong

The doc's _build_loader_context() sets:

  • all_rules = list(BUILTIN_RULES) + self.custom_rules()
  • context.semantic_rules = all_rules

But PreparePhase already does:

rule_set = [*BUILTIN_RULES, *context.semantic_rules]

That means MAID-S001 and MAID-S005 would run twice, producing duplicate diagnostics.

Section references

  • Doc: tier1-yaml-first-authoring.md:461-472
  • Code: packages/maid-engine/src/maid_engine/loader/phases/prepare.py:41-43
  • Code: packages/maid-engine/src/maid_engine/loader/rules/builtin.py:10-66

Suggested fix

Only put custom rules into context.semantic_rules, or change PreparePhase so it does not prepend built-ins again.

HIGH — §6.1, §6.5-§6.10, §7.3: v1 scope is still inconsistent; the doc keeps treating non-core entity types as if they are standard

What's wrong

The doc correctly says the built-in schemas are the four core types: room, npc, item, template. But later sections still present monster/quest/zone/spell/skill/shop as if they are part of the standard schema surface, including a CLI schema export example that outputs ten schemas.

The actual code only ships four standard entity type configs.

This leaves the reader with two conflicting stories: "v1 is four core types" and "the engine exports many more standard types".

Section references

  • Doc: tier1-yaml-first-authoring.md:1044-1055
  • Doc: tier1-yaml-first-authoring.md:2181-2204
  • Doc: tier1-yaml-first-authoring.md:2468-2510, 2646-2682
  • Code: packages/maid-engine/src/maid_engine/loader/entity_types.py:7-44

Suggested fix

Split the document clearly into:

  • engine-provided v1 core: room / npc / item / template
  • pack-provided examples: monster / quest / zone / spell / skill / shop

Do not show non-core schemas in "out of the box" export examples unless that export is explicitly scoped to a pack that registered them.

HIGH — §6.13 and §7.3: _schema examples still remain

What's wrong

The doc now says schema lives under _meta.schema, and PreparePhase._resolve_schema() indeed reads _meta.schema. But two examples still use _schema:

  1. migration adapter writes data["_schema"] = "maid:room:v2"
  2. JSON Schema export example exposes a top-level "_schema" property

That directly contradicts the doc's own corrected convention.

Section references

  • Doc: tier1-yaml-first-authoring.md:1031-1041
  • Doc: tier1-yaml-first-authoring.md:2131-2136
  • Doc: tier1-yaml-first-authoring.md:2215-2218
  • Code: packages/maid-engine/src/maid_engine/loader/phases/prepare.py:224-237

Suggested fix

Replace both remaining _schema examples with _meta.schema, or explicitly label them as legacy-compat examples if that is the intent.

HIGH — §6.2, §6.3, Appendix A: several author-facing field shapes still do not match the real component models

What's wrong

A spot-check found some fields that are correct (DescriptionComponent.name, HealthComponent.current, HealthComponent.maximum, NPCComponent.behavior_type, DialogueComponent.personality), but multiple examples are still wrong in ways that matter to authors:

  1. Extended room details use condition: singular, but RoomDetail uses conditions:.
  2. Autonomy schedule is shown as a plain list of {time, action, target} entries, but ScheduleComponent actually expects blocks with start_hour, end_hour, activity, location, priority, and optional conditions.
  3. Needs are shown as flat scalars (hunger: 0.3, etc.), but NeedsComponent expects a needs mapping of NeedCategory -> Need objects.
  4. Goals are shown as a simple list under goals, but GoalsComponent actually stores active_goals, completed_goals, and failed_goals, with Goal objects using fields like category, description, priority, etc.
  5. Combat examples use fractional critical_chance values like 0.05 / 0.1, but the real CombatComponent.critical_chance is an int.

These are not cosmetic mismatches; they materially affect whether examples correspond to real model shapes.

Section references

  • Doc: tier1-yaml-first-authoring.md:1198-1202
  • Doc: tier1-yaml-first-authoring.md:1343-1389
  • Doc: tier1-yaml-first-authoring.md:1579-1585
  • Doc: tier1-yaml-first-authoring.md:4013-4024
  • Code: packages/maid-stdlib/src/maid_stdlib/components/extended_room.py:473-488, 742-752
  • Code: packages/maid-stdlib/src/maid_stdlib/models/npc/autonomy.py:131-137, 265-279, 322-329
  • Code: packages/maid-stdlib/src/maid_stdlib/components/combat.py:13-28

Suggested fix

Either:

  • make these examples match the actual component models, or
  • explicitly mark them as future assembly-layer syntax and do not present them as if they already correspond to real component fields.

HIGH — §8.4 and Appendix A: rule suppression is still overstated relative to what exists

What's wrong

The code currently supports a global LoaderConfig.skip_rules set. It does not support per-entity _suppress YAML directives or _meta.yaml per-file rule toggles.

The doc does mention those as future enhancements in one note, but Appendix A still documents _suppress as a real top-level field, which makes the current capability boundary unclear.

There is a second problem: the examples use rule IDs like MAID-K001 and MAID-C001 as if they already exist in the current codebase, but built-ins today are only MAID-S001 and MAID-S005.

Section references

  • Doc: tier1-yaml-first-authoring.md:2609-2633
  • Doc: tier1-yaml-first-authoring.md:3997
  • Code: packages/maid-engine/src/maid_engine/loader/models.py:133-145
  • Code: packages/maid-engine/src/maid_engine/loader/phases/prepare.py:197-199
  • Code: packages/maid-engine/src/maid_engine/loader/rules/builtin.py:10-66

Suggested fix

  • Move _suppress out of the current top-level field reference until it exists.
  • Use currently real rule IDs in current-capability examples.
  • Put future rule configuration in a clearly marked future-work subsection.

MEDIUM — §3.2 and §3.4: Source-of-Truth section is present, but the concrete component definitions are still inaccurate

What's wrong

The section is directionally better now: it correctly says YAML is canonical and explicitly acknowledges that real reconciliation is not implemented yet. That part is honest.

However, the concrete component definitions shown in the section still drift from the code:

  • InstanceStateComponent is shown with dirty_components, but the real model uses modified_components, plus last_modified_at and modified_by.
  • DataProvenanceComponent is shown as a simple dataclass with four fields, but the real model also includes definition_id and definition_version.

Since this section is meant to define the ownership/reconciliation model, its concrete component shapes should be exact.

Section references

  • Doc: tier1-yaml-first-authoring.md:157-177
  • Code: packages/maid-engine/src/maid_engine/loader/models.py:209-236
  • Code: packages/maid-engine/src/maid_engine/loader/phases/instantiate.py:60-68

Suggested fix

Update the code snippets to match the actual models exactly, or explicitly label them as simplified pseudocode.

MEDIUM — §12.7: cross-pack @ref: syntax is correct, but the dependency-enforcement claim is not

What's wrong

The syntax examples for @ref:type/id, @ref:pack:type/id, and @ref:uuid:... match the resolver.

But the doc also says: "The referenced pack must be declared as a dependency in manifest.toml." The resolver does not enforce that. It simply looks up names in the shared ReferenceRegistry. If a pack's symbols are registered, they are resolvable regardless of whether the manifest declared that dependency.

Section references

  • Doc: tier1-yaml-first-authoring.md:3559-3585
  • Code: packages/maid-engine/src/maid_engine/loader/phases/resolve_refs.py:194-217
  • Code: packages/maid-engine/src/maid_engine/loader/resolver.py:50-71

Suggested fix

Either implement dependency-aware resolution checks, or weaken the wording to say dependency declaration is a recommended packaging rule / load-order expectation, not a current resolver guarantee.

MEDIUM — §6.2 and Appendix A: grid is still described as a standard room top-level field, but the core room config does not allow it

What's wrong

The room examples and Appendix A still advertise grid as a normal room top-level field. But the real ROOM_CONFIG.allowed_top_level_fields only allows exits and zone.

This is also internally inconsistent with the doc's own schema-show example, which lists allowed room top-level fields as exits, zone.

Section references

  • Doc: tier1-yaml-first-authoring.md:1151-1154
  • Doc: tier1-yaml-first-authoring.md:2337-2340
  • Doc: tier1-yaml-first-authoring.md:4004-4007
  • Code: packages/maid-engine/src/maid_engine/loader/entity_types.py:7-13

Suggested fix

Either add grid to the real room config (and document how it is assembled/validated), or remove it from the v1 core room schema examples and mark it as pack- or world-system-specific extension data.

What looks genuinely fixed / accurate

@ref: syntax itself

The syntax description aligns with the real resolver:

  • @ref:type/id
  • @ref:pack:type/id
  • @ref:uuid:<uuid>

Evidence

  • Doc: tier1-yaml-first-authoring.md:4040-4044
  • Code: packages/maid-engine/src/maid_engine/loader/phases/resolve_refs.py:150-217

World API examples

The document now correctly says that World does not provide get_entity_by_name() or get_entities_by_tag(), and the sample code uses world.get_all_entities() instead. That matches the real World API.

Evidence

  • Doc: tier1-yaml-first-authoring.md:3487-3491
  • Code: packages/maid-engine/src/maid_engine/core/world.py:329-332, 357-457

Reconciliation caveat

The document now openly states that InstantiatePhase does not perform reconciliation and that the described conflict-resolution behavior is a prerequisite/future step. That is honest and aligned with the current implementation, which always takes the NEW path.

Evidence

  • Doc: tier1-yaml-first-authoring.md:213-218
  • Code: packages/maid-engine/src/maid_engine/loader/phases/instantiate.py:33-39

Bottom line

This should not ship yet.

The review-cycle fixes were only partial. Some prior problem areas improved, but the document still over-claims current implementation status in multiple places, and several key author-facing examples still do not line up with the real code. The largest remaining gap is unchanged: the doc is still describing a YAML-first authoring system that the current loader path does not actually implement.