fix: add ensureTypeLoaded() before registry .get() calls for lazy types#1116
fix: add ensureTypeLoaded() before registry .get() calls for lazy types#1116
Conversation
PR #1089 introduced lazy per-bundle loading for vault, datastore, driver, and report registries. The get() method only returns fully-loaded types, returning undefined for lazy entries — but many consumer code paths call get() without first calling ensureTypeLoaded() to promote lazy entries. This caused extension types like @swamp/s3-datastore and @swamp/aws-sm to be treated as unknown, breaking 11 UATs across vault and datastore operations. The fix adds await registry.ensureTypeLoaded(type) before every .get() call across all four registries, matching the pattern already used by resolveModelType() in extension_auto_resolver.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
CLI UX Review
Blocking
None.
Suggestions
None.
Verdict
PASS — This PR is a pure internal bug fix. No flags, help text, error messages, JSON output shapes, or log-mode output were changed. The only user-visible effect is that commands involving extension vaults, datastores, drivers, and reports now work correctly instead of failing with "Unknown vault type" / "Unknown datastore type" errors. The getReport interface change from sync to async is an internal contract update with no observable UX difference.
There was a problem hiding this comment.
Code Review
Clean, well-scoped bug fix. The pattern of adding ensureTypeLoaded() before .get() is correct and consistent with the existing model registry approach in extension_auto_resolver.ts. All call sites are covered.
Blocking Issues
None.
Suggestions
-
getVaultTypeInfoinlibswamp/vaults/create.ts:83— This synchronous.get()call relies onresolveExtensionVaultType(line 127) being called first to ensure the type is loaded. The ordering is correct today, but the dependency is implicit. Consider makinggetVaultTypeInfoasync with its ownensureTypeLoadedfor defensiveness, matching the pattern used forgetReport. Not blocking since the current call ordering is safe. -
Pre-existing import boundary violation in
src/cli/resolve_datastore.ts:44—maybeAutoUpdateDatastoreExtensionis imported from../libswamp/extensions/datastore_auto_update.tsinstead of../libswamp/mod.ts(which does export it). Not introduced by this PR.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None found.
Medium
None found.
Low
-
src/libswamp/reports/search.ts:191-199— Sequential awaits in label filter loop. The conversion from.filter()to afor...ofloop withawait deps.getReport()makes report lookups sequential. If there are many candidates with distinct report names, this does N sequential loads instead of parallel. In practice this is negligible becauseensureTypeLoadedreturns immediately for already-loaded types (all reports likely share a small set of types), so only the first call per type incurs actual work. Not a real issue. -
src/libswamp/vaults/create.ts:83—getVaultTypeInfois synchronous and callsvaultTypeRegistry.get()withoutensureTypeLoaded. This looks potentially missed, but it's actually safe:resolveExtensionVaultType(line 77-81) is always called first in thevaultCreategenerator (line 127), and that function now callsensureTypeLoaded. By the timegetVaultTypeInfois called at line 130, the type is already promoted. The ordering dependency is implicit rather than enforced by the type system, but given the generator's sequential flow, this works correctly.
Verdict
PASS — This is a straightforward, mechanical fix applied consistently across all four registries. Every get() call site either has a preceding ensureTypeLoaded or is in a context where the type was already promoted (loader internals, or sequenced after a prior ensureTypeLoaded in the same flow). The async conversion of ReportDescribeDeps.getReport and ReportSearchDeps.getReport is correct, and the label filter refactor from .filter() to an imperative loop properly handles the new async signature. The ensureTypeLoaded method is safe to call with non-existent types (returns silently), so there are no new error paths introduced.
Adds 10 tests verifying the lazy registry promotion contract introduced by PR #1089 and fixed by PR #1116: Category 1 (8 tests, fresh instances): Documents the registry invariant that get() returns undefined for lazy entries and ensureTypeLoaded() must be called first to promote them. Covers all four registries (vault, datastore, driver, report). Category 2 (2 tests, global singletons): Verifies that resolveVaultType() and resolveDatastoreType() promote lazy entries before returning, so callers can safely use get() afterwards. These tests would have caught the PR #1089 regression — they fail without the ensureTypeLoaded() calls added in PR #1116. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Summary
PR #1089 introduced lazy per-bundle loading for vault, datastore, driver, and report registries. This caused a regression:
get()only returns fully-loaded types (fromthis.types), returningundefinedfor lazy entries (inthis.lazyTypes) — but many consumer code paths callget()without first callingensureTypeLoaded()to promote the lazy entry.This broke 11 UATs:
Unknown datastore type "@swamp/s3-datastore"/ same for GCSUnknown vault type: @test/mock-vault— vault create, vault put, and extension vault lifecycle all failRoot cause
The registries have two maps:
types(fully loaded) andlazyTypes(indexed but not imported). After PR #1089:has()returnstruefor both maps — masking the problem in conditional checksget()only checkstypes— returningundefinedfor lazy entriesensureLoaded()only populateslazyTypes(builds the index), it does NOT promote entries totypesensureTypeLoaded(type)is required to import the specific bundle and move the entry fromlazyTypestotypesThe model registry was already correct —
resolveModelType()inextension_auto_resolver.tscallsawait modelRegistry.ensureTypeLoaded(type)beforeget(). The vault, datastore, driver, and report registries needed the same treatment.Changes
Added
await registry.ensureTypeLoaded(type)before every.get()call across all four registries (15 files, 11 code paths):extension_auto_resolver.tsresolveVaultType()ensureTypeLoadedbeforehas()checkextension_auto_resolver.tsresolveDatastoreType()vault_service.tsfromRepository()ensureTypeLoadedbeforeregisterVault()libswamp/vaults/create.tsresolveExtensionVaultTypecallbackensureTypeLoadedbeforehas()checklibswamp/datastores/sync.tscreateDatastoreSyncDeps()ensureTypeLoadedbefore.get()libswamp/datastores/status.tsverifyHealthcallbackensureTypeLoadedbefore.get()libswamp/datastores/setup.tsdatastoreSetupExtension()ensureTypeLoadedbefore.get()cli/repo_context.tsresolveCustomProvider()ensureTypeLoadedalongside existingensureLoaded()cli/resolve_datastore.tsparseDatastoreEnvVarandresolveDatastoreConfigensureTypeLoadedbefore each.get()method_execution_service.tsensureTypeLoadedbefore.get()reports/describe.ts+search.tsgetReportcallbackensureTypeLoadedbefore.get(); updated interface, callers, CLI commands, and testsWhy
ensureLoaded()alone is insufficientAfter PR #1089,
ensureLoaded()callsbuildIndex()which only registers lazy entries viaregisterLazy(). It does not import bundles or populatethis.types. OnlyensureTypeLoaded(type)triggers thetypeLoaderto import the specific bundle and callpromoteFromLazy(), moving the entry fromlazyTypestotypeswhereget()can find it.Test Plan
deno check— all types passdeno lint— cleandeno fmt --check— cleandeno run test— 4174 tests pass, 0 failuresswamp repo initin /tmp directoryswamp extension pull @swamp/aws-sm— installed extension vaultswamp vault create @swamp/aws-sm my-aws-vault --config '{"region":"us-east-1"}'— succeeded (would have failed with "Unknown vault type" before fix)swamp extension pull @swamp/s3-datastore— installed extension datastoreswamp datastore setup extension @swamp/s3-datastore --config '{"bucket":"test","region":"us-east-1"}'— reached S3 access check (would have failed with "not registered or has no provider" before fix)🤖 Generated with Claude Code