Skip to main content
← Back to list
01Issue
BugShippedSwamp CLI
Assigneesstack72

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:250
  • src/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 bugsave() 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:

  1. Make get() itself call a sync-only tryLoadIfLazy() that logs a warning when it can't, and document ensureTypeLoaded as an explicit async precondition for callers that need guarantees. This is what #89 effectively did by convention.
  2. Split the API: rename the current get() to getLoaded() (emphasizing the precondition) and add an async getAndLoad() helper that composes ensureTypeLoaded + 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)
02Bog Flow
OPENTRIAGEDIN PROGRESSSHIPPED+ 1 MOREASSIGNED+ 2 MOREREVIEW+ 3 MOREPR_MERGEDSHIPPED

Shipped

4/13/2026, 5:36:22 PM

Click a lifecycle step above to view its details.

03Sludge Pulse
stack72 assigned stack724/13/2026, 4:38:03 PM

Sign in to post a ripple.