Audit: modelRegistry.get() without ensureTypeLoaded() in YAML repository save() paths
Opened by keeb · 4/12/2026· Shipped 4/13/2026
Summary
Follow-up to #89. During the audit of other modelRegistry.get( call sites across the repo, two save-path call sites still bypass ensureTypeLoaded(), mirroring the same latent bug pattern #89 fixed in validateModelPathReference. Both are in the persistence layer and stamp data.typeVersion from the registry when writing a definition to disk:
src/infrastructure/persistence/yaml_definition_repository.ts:250src/infrastructure/persistence/yaml_evaluated_definition_repository.ts:246
// yaml_definition_repository.ts
async save(type: ModelType, definition: Definition): Promise<void> {
// ...
data.type = type.normalized;
const modelDef = modelRegistry.get(type); // <-- here
data.typeVersion = modelDef?.version ?? data.typeVersion;
// ...
}If save() is ever invoked for a type that has been registered lazily but not yet imported, modelRegistry.get() returns undefined, the ?? data.typeVersion fallback kicks in, and the persisted YAML silently retains whatever typeVersion was on the incoming Definition — which may or may not match the currently-installed type version.
Unlike #89, this does not throw — it silently writes a potentially stale typeVersion to disk, which is arguably worse because downstream reads can't distinguish "author knew what they were doing" from "lazy loading tripped over itself".
Root Cause
Same class of bug as #89: the per-bundle lazy loading introduced in #1063 added the invariant that modelRegistry.has(type) === true does not imply modelRegistry.get(type) !== undefined for lazy types. Callers must await modelRegistry.ensureTypeLoaded(type) first. The execution path and (now, after #89/#1166) the validation path both honor this, but the save path does not.
Audit Results
I walked every non-test modelRegistry.get( call site. For reference, here's the state as of commit 82a9115e:
| Site | Status |
|---|---|
src/libswamp/models/get.ts:77 |
safe — awaits ensureTypeLoaded on the line above |
src/libswamp/models/create.ts:96 |
protected upstream — resolveModelType at create.ts:129 awaits ensureTypeLoaded inside extension_auto_resolver.ts:358 before getModelDef() is called |
src/domain/extensions/extension_auto_resolver.ts:359,367 |
this is resolveModelType itself — the authoritative pattern |
src/domain/workflows/execution_service.ts:719,849 |
practically safe (type is loaded by the time method dispatch reaches report lookup) but relies on implicit ordering — fragile |
src/domain/reports/report_execution_service.ts:299 |
same — runs in execution context where the type is already loaded |
src/domain/models/validation_service.ts:707 |
fixed in #1166 |
src/infrastructure/persistence/yaml_definition_repository.ts:250 |
latent bug — save() can silently persist a stale typeVersion for lazy types |
src/infrastructure/persistence/yaml_evaluated_definition_repository.ts:246 |
latent bug — same pattern |
The two save sites are the only remaining callers that can be reached with a lazy-unloaded type and silently do the wrong thing.
Steps to Reproduce
This is harder to hit than #89 because save() is usually called after swamp model create, which goes through resolveModelType (protected). A constructed repro would require a code path that builds a Definition for a lazy type and calls yaml_definition_repository.save() directly without any intervening step that loads the type. I have not constructed a user-visible repro — filing this as a hardening / latent-bug cleanup rather than a live incident.
Expected Behavior
save() should await modelRegistry.ensureTypeLoaded(type) before reading modelDef.version, mirroring the pattern used in libswamp/models/get.ts:75-77 and now in validation_service.ts:706.
Suggested Fix
One-line await in each save() method:
async save(type: ModelType, definition: Definition): Promise<void> {
// ...
data.type = type.normalized;
await modelRegistry.ensureTypeLoaded(type);
const modelDef = modelRegistry.get(type);
data.typeVersion = modelDef?.version ?? data.typeVersion;
// ...
}Both functions are already async, so no signature change is needed.
Longer-Term Hardening
The root of this bug class is that modelRegistry.get() silently violates the obvious reading of has() for lazy types. Two structural options:
- Make
get()itself call a sync-onlytryLoadIfLazy()that logs a warning when it can't, and documentensureTypeLoadedas an explicit async precondition for callers that need guarantees. This is what #89 effectively did by convention. - Split the API: rename the current
get()togetLoaded()(emphasizing the precondition) and add anasync getAndLoad()helper that composesensureTypeLoaded + get. Makes the contract explicit and grep-able at call sites.
Option 2 is more invasive but would mechanically prevent this class of bug from recurring — any caller using the unsafe API would at least be named for what it's doing.
Environment
- swamp built from main @ 82a9115e (after #1166 landed)
- Related commits: 686365ad (#1063, introduced lazy per-bundle loading), #1166 (fixed validation_service)
Shipped
Click a lifecycle step above to view its details.
Sign in to post a ripple.