fix: ensure lazy types are loaded before reading version in save() paths#1169
fix: ensure lazy types are loaded before reading version in save() paths#1169
Conversation
modelRegistry.get() returns undefined for lazily-registered types that haven't been imported yet. Both YamlDefinitionRepository.save() and YamlEvaluatedDefinitionRepository.save() called get() without first awaiting ensureTypeLoaded(), silently falling back to a potentially stale typeVersion on the incoming Definition. Add the missing ensureTypeLoaded() call in both save methods, matching the established pattern in libswamp/models/get.ts and validation_service.ts. Closes swamp-club#90
There was a problem hiding this comment.
Code Review
Clean, well-scoped bug fix that follows the established ensureTypeLoaded → get pattern used throughout the codebase (e.g., libswamp/models/get.ts:76, validation_service.ts, datastores/sync.ts:71, etc.).
Blocking Issues
None.
Suggestions
- The existing
yaml_definition_repository_test.tsdoesn't cover the lazy-typetypeVersionpersistence path. The PR body mentions filing systeminit/swamp-uat#136 for UAT coverage, which is a reasonable approach — but a unit test asserting thatsave()with a lazy-registered type correctly resolvestypeVersion(rather than falling back to stale data) would make this fix regression-proof at the unit level. Not blocking since the fix follows a well-tested pattern and the UAT tracking is in place.
Notes
- DDD: Infrastructure repositories correctly depend on the domain
modelRegistry— layer direction is sound. - Import boundary: Both files import from
../../domain/models/model.ts(domain internals), which is appropriate for infrastructure code. No libswamp boundary violation. - No
anytypes, license headers present, change is minimal and focused. ensureTypeLoadedacceptsstring | ModelTypeand the call sites passModelType— type-safe.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
None.
Low
None.
Verdict
PASS — This is a clean, minimal 2-line fix that adds await modelRegistry.ensureTypeLoaded(type) before modelRegistry.get(type) in both YamlDefinitionRepository.save() and YamlEvaluatedDefinitionRepository.save(). It follows the established pattern from libswamp/models/get.ts:76 and validation_service.ts:706. The ensureTypeLoaded call is correctly placed after data.type assignment but before modelRegistry.get() and well before atomicWriteTextFile, so a loader failure propagates cleanly without leaving partial writes. Both files already import modelRegistry, the type parameter is ModelType which matches the string | ModelType signature, and the existing ?.version ?? data.typeVersion fallback handles the (now unlikely) undefined case. No new failure modes, no API contract changes, no concurrency concerns beyond the already-deduplicated loader promises.
Summary
await modelRegistry.ensureTypeLoaded(type)beforemodelRegistry.get(type)inYamlDefinitionRepository.save()andYamlEvaluatedDefinitionRepository.save()get()to returnundefined, silently persisting a staletypeVersionto disklibswamp/models/get.ts:76andvalidation_service.ts:706Closes swamp-club#90 (follow-up to swamp-club#89)
Test Plan
deno checkpassesdeno lintpassesdeno fmt --checkpasses🤖 Generated with Claude Code