Code Review

Clean implementation that mirrors the existing vault auto-resolution pattern for datastores. The approach is sound: domain-level resolveDatastoreType() helper, port extension on ExtensionInstallerPort, adapter wiring, and a backwards-compatible rename+fallback strategy in the CLI layer.

Blocking Issues

None.

Suggestions

1. Test naming convention in extension_auto_resolver_test.ts: The three new resolveDatastoreType tests use - as a separator (e.g., "resolveDatastoreType - returns true..."), while CLAUDE.md convention is : (e.g., "resolveDatastoreType: returns true..."). The resolve_datastore_test.ts file was correctly updated to use : — worth aligning the auto-resolver tests too.

2. Duplicated S3 remapping logic: The S3-specific config construction + schema validation + provider creation + fallback flow is largely duplicated between parseDatastoreEnvVar (~lines 102-145) and resolveDatastoreConfig (~lines 244-291). Both paths build a config object from S3 fields, validate it, create a provider, resolve paths, and fall back to built-in S3. Consider extracting a shared helper like tryResolveRenamedS3(bucket, prefix, opts) to reduce the duplication surface — this would also make it easier to add future renamed types.

3. Minor: dsType = ds.type on line 290 (the fallback after extension is unavailable) resets to the original type, which is correct but reads oddly since dsType was already set to ds.type at declaration. A comment like // Extension unavailable, continue with original type would clarify intent.

Adversarial Review

Critical / High

None found. The async conversion is complete across all callers, the fallback logic is sound, and the auto-resolution plumbing mirrors the established vault pattern correctly.

Medium

1. src/cli/resolve_datastore.ts:106,144 — Empty-string prefix inconsistency between extension and built-in paths. When input is s3:my-bucket/ (trailing slash, no prefix text), value.slice(slashIdx + 1) produces "". On the extension path (line 114), if (prefix) is falsy so prefix is omitted from the config object. On the built-in fallback path (line 144), prefix is passed as "" directly into the return value. This means the same input produces { config: { bucket: "my-bucket" } } via extension vs { prefix: "" } via built-in fallback — a semantic divergence. Breaking example: Extension provider treats missing prefix as "use root", built-in consumer treats "" as a set-but-empty prefix with subtly different behavior (e.g., concatenating "/" + "" producing a trailing slash in S3 keys). Suggested fix: Normalize empty prefix to undefined before the branch: const prefix = slashIdx === -1 || slashIdx === value.length - 1 ? undefined : value.slice(slashIdx + 1);

2. src/cli/resolve_datastore.ts:147 — Future renamed non-s3 types will attempt to parse value as JSON with the raw shorthand syntax. If a second entry is added to RENAMED_DATASTORE_TYPES (e.g., "gcs": "@swamp/gcs-datastore"), and users pass gcs:my-bucket, the code skips the if (type === "s3") block, sets type = renamedTo, and falls through to the JSON parsing path (line 175) where JSON.parse("my-bucket") throws a confusing Invalid JSON config error rather than a helpful migration message. This is currently inert since only "s3" is mapped, but the structure invites the bug. Suggested fix: Add a comment noting that any future entry in RENAMED_DATASTORE_TYPES must also add a shorthand-parsing branch analogous to the if (type === "s3") block, or refactor the shorthand parsing to be data-driven.

Low

1. src/domain/extensions/extension_auto_resolver.ts:318-320 — hotLoadDatastores() errors are unhandled. If hotLoadDatastores() throws (e.g., corrupt extension bundle, permission error on the datastores dir), the error propagates up through installAndLoad → doResolve where it's caught by the generic catch block (line 197). For network errors, it shows a "network error" message; for others, it shows "not found" — neither accurately describes a datastore loading failure. This matches the existing behavior for hotLoadVaults() (same pattern), so it's not a regression, but worth noting.

2. src/cli/resolve_datastore_test.ts:113,121,129 — assertRejects with a non-async callback. The callbacks () => parseDatastoreEnvVar(...) return a Promise but are not marked async. assertRejects handles this correctly (it awaits the return value), so this works, but it's inconsistent with the other test patterns that use async () => await .... Purely cosmetic.

Verdict

PASS — The PR is a clean, well-structured implementation that correctly mirrors the vault auto-resolution pattern for datastores. The sync-to-async migration is complete across all call sites, fallback logic is sound, and tests cover the key paths. The medium findings are edge cases that don't affect current functionality.