fix: add remaining ensureLoaded() calls missed in #1056#1059
Conversation
VaultService.fromRepository() accesses vaultTypeRegistry without loading it first, causing all extension vault tests to fail with "Unsupported vault type". The model validate command similarly calls createModelValidateDeps() without loading the model registry. Add vaultTypeRegistry.ensureLoaded() at the top of fromRepository() and modelRegistry.ensureLoaded() in the model validate command handler. Fixes 6 remaining regression failures from lazy extension loading.
There was a problem hiding this comment.
Code Review
Clean, minimal fix that adds the missing ensureLoaded() calls to resolve the remaining 6 regression failures from #1050.
Blocking Issues
None.
Suggestions
- The
modelRegistryimport inmodel_validate.tsbypassessrc/libswamp/mod.ts(importing directly from../../domain/models/model.ts). This is a pre-existing pattern across many CLI commands (model_method_run.ts,type_describe.ts,type_search.ts, etc.) — not introduced by this PR — but worth noting as tech debt to address holistically.
DDD review: Both changes are well-placed. VaultService.fromRepository() correctly ensures its type registry is loaded within the domain service factory, and the CLI command handler follows the established application-layer pattern of loading registries before invoking domain logic.
LGTM — ship it.
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
None.
Low
vault_service.ts:55—ensureLoaded()sits outside the try/catch block (lines 57–107).
The try/catch gracefully degrades when vault configs are missing or invalid (logging warnings/debug instead of crashing). IfensureLoaded()throws (e.g., corrupt extension manifest, filesystem permission error during extension discovery), the error propagates uncaught fromfromRepository(), which could crash the CLI instead of degrading gracefully. In practice this is unlikely sinceensureLoaded()failures would indicate a fundamentally broken extension setup where crashing is arguably the right behavior — and this placement is consistent with every otherensureLoaded()call in the codebase. Noting for completeness only.
Verdict
PASS — Minimal, correct fix. Both ensureLoaded() calls are idempotent (guard clause + cached promise), follow the established pattern used across all other CLI commands and domain services, and the import of modelRegistry from ../../domain/models/model.ts is consistent with how every other CLI command imports registry singletons. The fix addresses the documented regression from #1050.
Summary
vaultTypeRegistry.ensureLoaded()at the top ofVaultService.fromRepository()— fixes 5 extension vault integration test failures where user-defined vault types weren't found because the registry was emptymodelRegistry.ensureLoaded()in themodel validatecommand handler beforecreateModelValidateDeps()— fixes 1 extension model test failure wheremodel validatecouldn't find extension model typesThese are the remaining 6 regression failures from the lazy extension loading change (#1050), following the initial fix in #1056 which resolved 4 of 10.
Test Plan
extension_vault_test.ts(5 tests): vault lifecycle, model method run with vault secrets, workflow run with vault secrets, built-in and user-defined vaults coexist, two instances of same typeextension_model_test.ts(1 test): model validate step🤖 Generated with Claude Code