CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — This PR is a pure internal performance refactoring with no user-facing UX changes. Help text, flag names, error messages, log-mode output, and JSON-mode output are all unchanged. The only observable effect for users is that commands that don't need extensions (e.g. swamp model help, swamp vault search) are now ~6-7x faster. The rename of the internal commandNeedsExtensions → commandNeedsLoaderSetup correctly reflects the new semantics and is not user-visible.

Code Review

Well-executed performance optimization. The lazy loading pattern is clean — configuring loaders at startup and deferring actual loading to ensureLoaded() in dep factories is the right approach. The benchmark results (7x speedup for commands that don't need extensions) speak for themselves.

DDD Assessment

The setLoader() / ensureLoaded() addition to domain registries is a reasonable trade-off. The registries don't depend on infrastructure — they accept a loader function from the CLI layer (dependency inversion), keeping the domain pure. The lazy-loading concern is expressed as a capability of the registry, not as knowledge of how loading works.

Blocking Issues

None.

Suggestions

1. Duplicated ensureLoaded / setLoader boilerplate — The identical 15-line pattern is copy-pasted across all 5 registries (ModelRegistry, VaultTypeRegistry, DriverTypeRegistry, DatastoreTypeRegistry, ReportRegistry). Consider extracting to a shared LazyLoadable mixin or utility class in a follow-up.

2. Missing unit tests for setLoader / ensureLoaded — This is new core functionality with interesting edge cases (concurrent callers, loader rejection, no-op when no loader set). At least one registry should have tests covering: (a) basic load-on-first-call, (b) concurrent ensureLoaded() deduplication, (c) no-op when no loader configured. The existing 3940 tests validate this indirectly through integration, but targeted unit tests would prevent regressions.

3. report_describe.ts could use createReportDescribeDeps — The PR adds createReportDescribeDeps() to libswamp and exports it from mod.ts, but report_describe.ts still manually constructs deps and calls ensureLoaded() inline. Using the dep factory would be more consistent with the pattern used by other commands (e.g., model_get.ts uses createModelGetDeps).

4. Rejected loader promise is permanently cached — If the loader throws (e.g., transient I/O error), extensionLoadPromise caches the rejection and all subsequent ensureLoaded() calls re-throw. For a CLI tool that exits on error this is fine, but the serve module keeps running — a transient failure on the first WebSocket connection would poison the registry for all subsequent connections. Worth documenting or adding a reset-on-failure path in a follow-up.

Adversarial Review

Critical / High

1. src/cli/mod.ts:593-718 — Deferred extension-loading warnings are silently dropped

   The deferredWarnings array is created at line 593 and the lazy-loader closures (lines 596-610) capture it by reference. However, the warning-emission loop runs inside globalAction at line 718, which executes before any command .action() handler. Since ensureLoaded() is only called from .action() handlers (or dep factories invoked by them), the loaders haven't run yet when warnings are emitted — so the array is empty (or only contains checkForMissingPulledExtensions warnings).

   Breaking scenario: A user has a corrupt or version-mismatched extension installed. Previously, they saw a warning like Failed to load user model @swamp/aws/ec2: .... Now, the loader runs inside the command action, pushes the warning into deferredWarnings, but the emission loop at line 718 already finished. The warning is silently swallowed, and the user gets a confusing downstream error (e.g., "unknown model type") with no hint about the real cause.

   Suggested fix: Either (a) emit warnings after ensureLoaded() calls (e.g., in dep factories or a post-load hook), or (b) have ensureLoaded() log warnings directly via the logger (since logging IS initialized by the time .action() runs), bypassing the deferred-warning mechanism entirely.

2. src/cli/repo_context.ts:78 — Custom extension datastore types not found during requireInitializedRepo

   resolveCustomProvider calls datastoreTypeRegistry.get(config.type) without any ensureLoaded() call. This function is invoked from requireInitializedRepo (line 275), which runs in ~59 command handlers before any ensureLoaded() call in the command body.

   Before this PR, all extensions were eagerly loaded, so custom datastore types were already registered. After this PR, lazy loading means the type won't be found.

   Breaking scenario: User has @swamp/aws/s3 (or another extension datastore) configured in .swamp.yaml. They run swamp model search. requireInitializedRepo calls resolveCustomProvider, which calls datastoreTypeRegistry.get("@swamp/aws/s3") → returns undefined → throws UserError: Datastore type "@swamp/aws/s3" is not registered or has no provider.

   Note: This may be partially mitigated by the auto-resolver in resolveDatastoreConfig (line 251-253) for @-prefixed types, but resolveCustomProvider at line 78 is called separately (e.g., at lines 461, 566, 763) and does not go through auto-resolution. If any of these paths are hit before ensureLoaded(), it breaks.

   Suggested fix: Add await datastoreTypeRegistry.ensureLoaded() in requireInitializedRepo before the resolveCustomProvider call, or add it directly in resolveCustomProvider.

Medium

1. src/cli/completion_types.ts:134 — Shell completions for model types won't include extension types

   ModelTypeType.complete() calls modelRegistry.types() without ensureLoaded(). Shell completion is async-capable via Cliffy, but extensions won't be loaded. This means swamp model create <TAB> won't suggest extension model types. Lower severity since it's a completions UX issue, not a correctness issue.

2. All 5 registries ensureLoaded() — Rejected loader promise is permanently cached

   If the loader throws (e.g., network error during extension loading), extensionLoadPromise is set to the rejected promise but extensionsLoaded stays false. Every subsequent ensureLoaded() call re-awaits the same rejected promise. For a CLI (single invocation per process), this is fine. But in serve mode, where connections create deps per-WebSocket (line 964-973 in connection.ts), a transient failure on the first connection permanently poisons the registry for all future connections until the server restarts.

   Suggested fix: On rejection, clear extensionLoadPromise so the next call retries:

   this.extensionLoadPromise = loader().then(() => {
     this.extensionsLoaded = true;
   }).catch((err) => {
     this.extensionLoadPromise = null;
     throw err;
   });

Low

1. Duplicated ensureLoaded() pattern across 5 registries — The setLoader/ensureLoaded implementation is copy-pasted identically in ModelRegistry, VaultTypeRegistry, DriverTypeRegistry, DatastoreTypeRegistry, and ReportRegistry. Not a correctness issue, but a maintenance concern — a fix to the caching logic (e.g., the rejection fix above) would need to be applied in 5 places.

Verdict

FAIL — The deferred-warnings bug (#1) means extension loading failures are silently swallowed, which will cause confusing downstream errors. The requireInitializedRepo regression (#2) breaks custom extension datastores. Both are fixable without changing the overall lazy-loading architecture.

CLI UX Review

Blocking

None.

Suggestions

None.

Verdict

PASS — This PR is a pure internal performance optimization. No user-facing interface changes: no new flags, no changed help text, no modified output formats, no altered error messages. The only UX change is a significant speedup (6-7x faster for commands that don't need extensions, e.g. swamp model help drops from 4.8s to 0.65s). Warning message format for failed extension loads is preserved — same text, now emitted directly via logger.warn inside command action handlers rather than through the deferred system, which is correct since logging is initialized by the time those run.

Code Review

Well-structured PR that cleanly replaces eager all-or-nothing extension loading with lazy per-registry loading. The setLoader()/ensureLoaded() pattern is sound — configuring loaders at startup and deferring actual loading to command action handlers is a good architectural choice. The concurrent-caller caching via the shared promise is correct, and the loaders' internal try/catch blocks prevent rejected-promise caching issues in practice.

DDD alignment is good: registries remain domain-layer singletons, the lazy loading infrastructure is configured by the CLI orchestration layer, and application services (dep factories in libswamp) trigger loading as needed. The libswamp import boundary is respected — CLI commands import from src/libswamp/mod.ts only.

Blocking Issues

None.

Suggestions

1. Duplicate lazy-loading boilerplate across 5 registries: The setLoader/ensureLoaded/extensionLoader/extensionLoadPromise/extensionsLoaded pattern is copy-pasted identically across ModelRegistry, VaultTypeRegistry, DriverTypeRegistry, DatastoreTypeRegistry, and ReportRegistry. Consider extracting this into a shared mixin or base class to reduce duplication and ensure consistent behavior if the pattern needs to evolve. Not blocking since the current code is correct.

2. No unit tests for setLoader/ensureLoaded: The new lazy-loading mechanism has subtle concurrency semantics (promise caching, no-op when no loader is set, idempotency). While the existing 3940 tests provide integration coverage, dedicated unit tests for the registry lazy-loading behavior would be valuable — especially for edge cases like concurrent callers and the "no loader configured" path.

3. Unused createReportSearchDeps and createReportDescribeDeps factories: These are defined in src/libswamp/reports/search.ts and src/libswamp/reports/describe.ts, exported from src/libswamp/mod.ts, but not imported by any consumer. The CLI commands (report_search.ts, report_describe.ts) build their deps inline instead. If these are intended for the serve layer or future use, a comment noting that would help; otherwise they're dead code.

4. Rejected-promise caching in ensureLoaded(): If a loader's promise rejects, the rejection is permanently cached in extensionLoadPromise — all subsequent ensureLoaded() calls will re-throw the same error with no retry path. This is mitigated today because all loaders wrap their bodies in try/catch, but it's a latent risk if a future loader doesn't follow this pattern. Consider resetting extensionLoadPromise on rejection.

Adversarial Review

Critical / High

None found.

Medium

1. All registries ensureLoaded() — rejected promise cached permanently (src/domain/models/model.ts:692-698, and identical pattern in all 5 registries):
   If loader() rejects, extensionLoadPromise caches the rejected promise while extensionsLoaded remains false. Every subsequent ensureLoaded() call re-awaits the same rejected promise — the registry is permanently broken for the rest of the process with no retry path.

   Breaking example: A transient filesystem error (e.g., permission denied on the pulled extensions directory) during first ensureLoaded() call would cause every subsequent command handler that calls ensureLoaded() to throw. In the serve command, this means the first WebSocket connection's transient failure permanently poisons all future connections.

   Mitigating factor: The loadUser* functions all have broad try { ... } catch { } blocks that swallow all errors, so the loader should never reject in practice. But if someone later adds error propagation in a loader, this will silently become a permanent-failure bug.

   Suggested fix: Clear extensionLoadPromise on rejection so the next caller retries:

   this.extensionLoadPromise = loader().then(() => {
     this.extensionsLoaded = true;
   }).catch((err) => {
     this.extensionLoadPromise = null;
     throw err;
   });

Low

1. datastoreTypeRegistry not in serve Promise.all blocks (src/serve/deps.ts:53-58, src/serve/deps.ts:78-83):
   createWorkflowRunDeps and createModelMethodRunDeps eagerly load 4 of 5 registries (model, vault, driver, report) but not datastoreTypeRegistry. Datastore loading only happens lazily through resolveCustomProvider() in repo_context.ts. This works correctly since custom datastore resolution gates itself, but it's an inconsistency — a workflow step that needs a custom datastore type pays the loading cost at lock-acquisition time rather than at deps-creation time, which could produce surprising latency during execution.

2. deferredWarnings array declared but never populated by extension loaders (src/cli/mod.ts:571):
   The deferredWarnings array is still declared and passed to emitDeferredWarnings() later, but all 5 loadUser* functions now log directly via logger.warn instead of pushing to this array. The array is only populated by checkForMissingPulledExtensions. This is harmless dead-ish code but could confuse future readers about the warning flow.

Verdict

PASS — The lazy loading pattern is mechanically correct. All registry accesses are properly guarded by ensureLoaded() calls, the promise caching handles concurrent callers correctly, and the sync-to-async function signature changes are consistently propagated to all callers. The medium finding about cached rejected promises is a defensive concern that doesn't bite in the current codebase due to the broad try/catch in all loaders.