feat: add vault migrate command

[stack72]

Summary

• Add swamp vault migrate <vault-name> --to-type <type> command that migrates a vault's backend in-place, preserving the vault name so all existing vault reference expressions keep working
• Extract provider instantiation into shared vault_provider_factory.ts used by both VaultService and the migrate operation
• Support --dry-run preview, --config for backend-specific settings, and per-secret progress output in both log and JSON modes

Fixes swamp-club#37

Test Plan

• Unit tests for createVaultProvider factory (built-in types, unsupported type error, case insensitivity)
• Unit tests for vaultMigrate generator (secret copying, config swap, vault-not-found error, unknown-type error, empty vault, delete failure tolerance)
• Unit tests for vaultMigratePreview (preview data, not-found, same-type rejection, unknown-type rejection)
• Existing VaultService tests still pass after registerVault() refactor
• deno check — no type errors
• deno lint — clean
• deno fmt — formatted
• Full test suite — 4249 tests pass
• deno run compile — binary compiles

🤖 Generated with Claude Code

[github-actions]

CLI UX Review

Blocking

1. Missing --force/-f flag (src/cli/commands/vault_migrate.ts): The command prompts for confirmation in log mode but provides no way to skip it non-interactively. Both data gc and vault put offer -f, --force for this exact purpose. Without it, any log-mode automation (CI pipelines, shell scripts) will hang on the [y/N] prompt. JSON mode bypasses the prompt, but that's not a documented or obvious workaround — users familiar with --force from other commands will expect it here.

   Fix: Add .option("-f, --force", "Skip confirmation prompt") and wrap the promptConfirmation call with !options.force, matching the pattern in data_gc.ts:73:

   if (cliCtx.outputMode === "log" && !options.force) {
     const confirmed = await promptConfirmation(...);
     ...
   }

Suggestions

1. --config error message wording (src/cli/commands/vault_migrate.ts:102): Says Invalid JSON for --config — vault_create uses Invalid JSON in --config. Trivial but inconsistent.

2. Dry-run JSON omits type display names (src/cli/commands/vault_migrate.ts:138-149): The preview object has currentTypeName and targetTypeName, but the --dry-run JSON output only includes currentType and targetType. Scripts that want to display human-readable names have to resolve them separately. Consider including the name fields.

3. Asymmetric preview log lines (src/cli/commands/vault_migrate.ts:122-125): Source shows "my-vault" (Mock) (name only in parens) but target shows Local Encryption (local_encryption) (name + identifier). Either show both identifiers, or neither — just pick one format and apply it consistently.

Verdict

NEEDS CHANGES — missing --force flag breaks log-mode scripting, inconsistent with the established data gc and vault put patterns.

[github-actions]

Code Review

Blocking Issues

None.

Suggestions

1. Duplicate RENAMED_VAULT_TYPES constant — src/domain/vaults/vault_provider_factory.ts:32 has a private copy of RENAMED_VAULT_TYPES that is identical to the one exported from src/domain/vaults/vault_service.ts:237. The factory's suggestVaultType helper uses its own copy while migrate.ts and create.ts import the service's export. These could diverge silently. Consider having the factory import from vault_service.ts, or extracting the constant into a shared module (e.g. vault_types.ts).

2. "Empty vault" test doesn't test zero secrets — The test at src/libswamp/vaults/migrate_test.ts:211 ("handles empty vault") registers a MockVaultProvider that pre-populates 3 default secrets, so it never exercises the zero-secret path. A custom provider override returning an empty list() would make the test more precise. The existing test comment acknowledges this, so it's a minor point.

---

Overall this is a clean, well-structured PR:

• Good DDD layering: factory in the domain, migration generator in libswamp (application layer), CLI in commands, renderer in presentation
• Libswamp import boundary correctly respected — CLI and renderer import only from mod.ts
• Comprehensive test coverage across factory, preview, and migration generator (happy paths, error cases, delete-failure tolerance)
• Follows all established project patterns (AnyOptions, consumeStream/EventHandlers, dual log/json output)
• License headers on all new files, design doc updated, skill SKILL.md updated
• Config swap ordering (save-new → delete-old) is correctly implemented — different vault-type directories mean these are distinct file paths, so no risk of self-deletion

[github-actions]

Adversarial Review

Critical / High

1. Missing error handling in copy loop — unhandled exceptions from provider operations
   src/libswamp/vaults/migrate.ts:216-224 (lines in the new file)

   The secret copy loop does not wrap vaultService.get() or targetProvider.put() in a try/catch:

   for (let i = 0; i < keys.length; i++) {
     yield { kind: "copying_secret", ... };
     const value = await vaultService.get(input.vaultName, keys[i]);
     await targetProvider.put(keys[i], value);
   }

   If either call throws (network timeout from AWS SM, permission denied from Azure KV, decryption failure from local_encryption), the exception propagates out of the async generator, through consumeStream, and into the CLI action handler — which has no try/catch around the consumeStream call (src/cli/commands/vault_migrate.ts:153-161). The renderer's error handler is never invoked.

   Breaking example: User migrates from local_encryption to @swamp/aws-sm. On the 5th of 10 secrets, AWS returns a throttling error. The user sees a raw stack trace instead of a clean error, and has no idea that 4 secrets were already copied to the target.

   Suggested fix: Wrap the copy loop body in try/catch and yield an error event:

   try {
     const value = await vaultService.get(input.vaultName, keys[i]);
     await targetProvider.put(keys[i], value);
   } catch (err) {
     yield {
       kind: "error",
       error: operationFailed(
         `Failed to copy secret '${keys[i]}': ${err instanceof Error ? err.message : String(err)}`
       ),
     };
     return;
   }

   This is consistent with how every other generator in src/libswamp/vaults/ handles failures — yield error events, don't throw.

Medium

2. vaultMigrate generator is a public API but lacks same-type validation
   src/libswamp/vaults/migrate.ts:162-260 (the generator function) and src/libswamp/mod.ts (export)

   vaultMigratePreview correctly rejects same-type migrations, but vaultMigrate itself does not check. Since both are exported as public API from libswamp/mod.ts, a library consumer calling vaultMigrate directly (without preview) would silently copy all secrets back onto the same backend, swap config files, and report success — doing pointless and potentially expensive work against external providers.

   Suggested fix: Add the same-type check at the top of vaultMigrate, before the copy loop, yielding a validation_failed error event if sourceConfig.type.toLowerCase() === input.targetType.toLowerCase().

3. Duplicate vault configs after delete failure cause non-deterministic provider loading
   src/libswamp/vaults/migrate.ts:236-244

   The config swap logic is save-new-then-delete-old. If deleteConfig fails (caught and logged as warning), two config files exist with the same vault name:

   • vaults/{oldType}/{id}.yaml
   • vaults/{newType}/{id}.yaml

   On next repo load, VaultService.fromRepository() calls findAll() which scans all type directories and registers both. Since registerVault does this.providers.set(config.name, provider), the last one wins — but the scan order is non-deterministic (depends on Deno.readDir ordering). The user could non-deterministically get the old or new backend.

   Suggested fix: After catching the delete failure, emit a warning event to the user stream (not just a debug log) so the user knows to manually clean up the orphaned config. The warning message should include the exact file path to delete.

4. RENAMED_VAULT_TYPES is duplicated between vault_provider_factory.ts and vault_service.ts
src/domain/vaults/vault_provider_factory.ts:26-32 and src/domain/vaults/vault_service.ts:237-243

   The factory defines its own copy of RENAMED_VAULT_TYPES for suggestVaultType, while vault_service.ts still exports the original. migrate.ts imports from vault_service.ts. If these diverge, users would get inconsistent error messages depending on which code path surfaced the error.

   Suggested fix: Keep one canonical definition and import it everywhere, or move it to a shared constants file since the factory already owns suggestVaultType.

Low

5. TOCTOU between preview and execute phases
   src/cli/commands/vault_migrate.ts:97-161

   Preview fetches vault state, the user is prompted, then migrate re-fetches. Between these calls the vault could be deleted, secrets could change, or another process could have already migrated it. The migrate generator handles vault-not-found (yields error), but doesn't re-check renamed types or same-type. Unlikely in practice since vault operations are typically single-user.

Verdict

FAIL — The copy loop error handling gap (finding #1) is a real issue for the primary use case of this feature (migrating to external backends where network failures are expected). The fix is straightforward and would bring the generator in line with the established pattern in the codebase.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Confirmation prompt uses internal type strings instead of friendly names — vault_migrate.ts:157: The prompt reads Migrate vault backend from local_encryption to @swamp/aws-sm? but the preview already has preview.currentTypeName and preview.targetTypeName. Prefer the human-readable names: Migrate vault backend from Local Encryption to AWS Secrets Manager?

2. Asymmetry in JSON output between dry-run and completed — Dry-run output (vault_migrate.ts:137-149) includes currentTypeName, but the completed event data (migrate.ts:49-56) has newTypeName but no previousTypeName. A script that needs the human-readable name of the old backend has to pull it from the dry-run step. Consider adding previousTypeName to VaultMigrateData.

3. --force is a no-op in JSON mode — The flag description says "Skip confirmation prompt" but JSON mode never prompts (see vault_migrate.ts:156). This is consistent with workflow_delete so it's not new, but worth noting for documentation clarity.

4. Per-secret progress silently dropped in JSON mode — vault_migrate.ts JSON renderer ignores copying_secret events. For large vaults this means no output until completion, which can look like a hang. A --verbose flag or streaming progress events in JSON mode would help, but this is optional.

Verdict

PASS — well-structured command with clear error messages, consistent --force/--dry-run patterns, and both output modes covered. No blocking issues.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Confirmation prompt uses internal type strings instead of friendly names — vault_migrate.ts:157: The prompt reads Migrate vault backend from local_encryption to @swamp/aws-sm? but the preview already has preview.currentTypeName and preview.targetTypeName. Prefer the human-readable names: Migrate vault backend from Local Encryption to AWS Secrets Manager?

2. Asymmetry in JSON output between dry-run and completed — Dry-run output (vault_migrate.ts:137-149) includes currentTypeName, but the completed event data (migrate.ts:49-56) has newTypeName but no previousTypeName. A script that needs the human-readable name of the old backend has to pull it from the dry-run step. Consider adding previousTypeName to VaultMigrateData.

3. --force is a no-op in JSON mode — The flag description says "Skip confirmation prompt" but JSON mode never prompts (see vault_migrate.ts:156). This is consistent with workflow_delete so it is not new, but worth noting for documentation clarity.

4. Per-secret progress silently dropped in JSON mode — vault_migrate.ts JSON renderer ignores copying_secret events. For large vaults this means no output until completion, which can look like a hang. A --verbose flag or streaming progress events in JSON mode would help, but this is optional.

Verdict

PASS — well-structured command with clear error messages, consistent --force/--dry-run patterns, and both output modes covered. No blocking issues.

[github-actions]

Code Review

Blocking Issues

None.

Suggestions

1. VaultConfig.create() resets createdAt during migration (src/libswamp/vaults/migrate.ts:296): VaultConfig.create() always sets createdAt to new Date(), so the migrated vault loses its original creation timestamp. If audit trails matter, consider adding a VaultConfig.withType() or similar method that preserves the original createdAt. Not blocking since the migration is intentionally creating a "new" config entry.

2. Same-type guard only in preview, not in the generator (src/libswamp/vaults/migrate.ts:229): vaultMigratePreview rejects same-type migrations, but vaultMigrate does not duplicate that check. The CLI always calls preview first so this is safe in practice, but if the generator is ever called independently (e.g., from a workflow), it would allow a no-op migration. Consider adding the guard to the generator as well for defense in depth.

Overall: clean, well-structured PR. Good DDD layering (factory in domain, generators in libswamp, renderers in presentation). Comprehensive test coverage for the new code. The save-then-delete config swap and delete-failure tolerance are solid safety choices. All CLAUDE.md conventions followed.

[github-actions]

Adversarial Review

Critical / High

1. HIGH: Case-sensitivity mismatch in resolveBuiltInProviderConfig silently produces wrong encryption config — src/libswamp/vaults/migrate.ts:114-127

   resolveBuiltInProviderConfig uses strict case matching (case "local_encryption"), but the rest of the system — vaultTypeRegistry.get(), createVaultProvider's switch — normalizes to lowercase. A user passing --to-type Local_Encryption (or any non-lowercase variant) bypasses the config case and gets {} instead of {auto_generate: true, base_dir: repoDir}.

   Breaking example: swamp vault migrate my-vault --to-type Local_Encryption

   • resolveBuiltInProviderConfig("Local_Encryption", ...) falls through to default: return {}
   • LocalEncryptionVaultProvider gets empty config, auto_generate is undefined
   • On put(), it tries ~/.ssh/id_rsa (the default fallback) — fails with confusing SSH key error
   • Worse: if an SSH key does exist, migration silently succeeds using SSH key material instead of auto-generated key. Secrets are encrypted with the wrong key source. User discovers this only when the SSH key changes and secrets become inaccessible.

   Fix: Normalize the type before switching: switch (vaultType.toLowerCase()) at line 118.

Medium

2. MEDIUM: vaultMigrate generator lacks same-type guard — config file deletion possible — src/libswamp/vaults/migrate.ts:229-326

   vaultMigratePreview rejects same-type migrations (line 146), but vaultMigrate does not. Since both are exported from mod.ts as independent functions, a library consumer could call vaultMigrate directly with targetType === sourceConfig.type. The config file path is {type}/{id}.yaml — when types match, saveConfig(newConfig) overwrites the file at the same path, then deleteConfig(sourceConfig) deletes it. The vault config is gone.

   Breaking example: Library code calls vaultMigrate(ctx, deps, { vaultName: "x", targetType: "mock", ... }) on a vault already of type mock. After execution, mock/{id}.yaml is deleted. The vault becomes invisible to the system.

   Fix: Add a same-type guard at the top of vaultMigrate, or document that vaultMigratePreview must be called first, or make vaultMigrate accept the preview result instead of re-querying.

3. MEDIUM: Unhandled exceptions during secret copy propagate without cleanup context — src/libswamp/vaults/migrate.ts:282-291

   If vaultService.get() or targetProvider.put() throws mid-loop (e.g., corrupted secret, network error on remote backend), the exception propagates as an unhandled error through consumeStream, not as a stream error event. The user gets a raw error with no indication of:

   • Which secret failed (index/total)
   • That the source vault is still intact
   • That partial writes may exist in the target backend

   The safety model is preserved (source untouched, config not swapped), but the user experience is poor.

   Breaking example: Secret 3 of 10 has corrupted encryption — get() throws. User sees a raw crypto error and doesn't know the first 2 secrets were already written to the target.

   Fix: Wrap the copy loop body in try/catch and yield a structured error event with the failing key and progress info.

Low

4. LOW: JSON mode skips confirmation even without --force — src/cli/commands/vault_migrate.ts:156

   The confirmation prompt only fires when outputMode === "log". In JSON mode, migration proceeds unconditionally regardless of --force. This is likely intentional for programmatic use but the --force flag description ("Skip confirmation prompt") implies there's always a prompt to skip.

5. LOW: JSON.parse result not validated as object — src/cli/commands/vault_migrate.ts:98

   JSON.parse(options.config) could return a non-object (--config '42', --config '"hello"', --config '[]'). The result is cast to Record<string, unknown> without runtime validation. Downstream Zod schemas catch this for extension types, but built-in types would get unexpected config shapes. Unlikely in practice since the option description shows JSON object examples.

Verdict

FAIL — The case-sensitivity mismatch in resolveBuiltInProviderConfig (#1) is a real bug that causes either confusing errors or silent use of wrong encryption key material, depending on the user's SSH key setup. Fix is a one-line change (vaultType.toLowerCase()).

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Missing --force example in help text (src/cli/commands/vault_migrate.ts): Other destructive commands that have -f, --force also include a force example (e.g. vault_put.ts has "Overwrite existing secret ... --force", extension_rm.ts has "Force remove ... --force"). Consider adding a third example like:

   .example("Skip confirmation prompt", "swamp vault migrate my-vault --to-type local_encryption --force")
   

2. Asymmetric JSON output fields (src/libswamp/vaults/migrate.ts, VaultMigrateData): The completed event has newTypeName (the friendly display name) but no previousTypeName. A script consuming the JSON output that wants to display a human-readable summary has to look up the old type name separately. Suggest adding previousTypeName: string to VaultMigrateData for symmetry.

3. Dry-run log output is sparse: After showing the preview (vault name, secret count, type change), log mode prints only Dry run — no changes made. with no visual emphasis. Minor, but a separator or slightly more prominent message (e.g. No changes made.) would be clearer at a glance.

Verdict

PASS — both log and JSON modes are fully supported, flag names are consistent with existing commands (-f, --force, --dry-run, --repo-dir), error messages are clear and actionable, and the confirmation-prompt-in-log-mode / auto-run-in-json-mode pattern matches the rest of the CLI.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Missing --force example in help text (src/cli/commands/vault_migrate.ts): Other destructive commands with -f, --force include a force example (e.g. vault_put.ts has "Overwrite existing secret ... --force", extension_rm.ts has "Force remove ... --force"). Consider adding a third example like swamp vault migrate my-vault --to-type local_encryption --force.

2. Asymmetric JSON output fields (src/libswamp/vaults/migrate.ts, VaultMigrateData): The completed event has newTypeName but no previousTypeName. A script consuming the JSON output that wants a human-readable summary of the old type has no way to get it without a separate lookup. Suggest adding previousTypeName: string for symmetry.

3. Dry-run log output is sparse: After showing the vault preview, log mode prints only Dry run — no changes made. with no visual emphasis. Minor — slightly more prominent phrasing would make the "nothing changed" outcome clearer at a glance.

Verdict

PASS — both log and JSON modes are fully supported, flag names are consistent with existing commands (-f, --force, --dry-run, --repo-dir), error messages are clear and actionable, and the confirmation-prompt-in-log/auto-run-in-json pattern matches the rest of the CLI.

[github-actions]

Code Review

Clean, well-structured PR. The factory extraction is a good refactor, the migration logic follows established patterns (dependency injection, generator-based event streaming, dual output modes), and test coverage is thorough.

Blocking Issues

None.

Suggestions

1. vaultMigrate generator skips same-type validation — vaultMigratePreview rejects same-type migrations, but the vaultMigrate generator itself does not. If someone calls the generator directly (bypassing preview), a same-type migration would proceed. The CLI always calls preview first so this isn't a practical issue, but adding the check in the generator would make it defensively correct.

2. VaultConfig.create resets createdAt — The migration calls VaultConfig.create(sourceConfig.id, ...) which sets createdAt to new Date(). This means the migrated vault config loses its original creation timestamp. Consider whether preserving the original createdAt (via VaultConfig.fromData) would be more appropriate to reflect the vault's true creation date vs. the migration date.

3. Secret key logging — The log renderer outputs Copying secret 1/N: <key> with the actual key name. For vaults where key names themselves are sensitive (e.g., contain account identifiers), consider whether this should be behind a verbose/debug flag rather than info-level output.

[github-actions]

Adversarial Review

Critical / High

1. src/libswamp/vaults/migrate.ts:229-311 — vaultMigrate generator missing same-type guard leads to config deletion

   vaultMigratePreview rejects same-type migrations (line 146), but the vaultMigrate generator has no such guard. Both functions are independently exported from libswamp/mod.ts. If a consumer calls vaultMigrate directly without calling preview first, and sourceType === targetType:

   • saveConfig(newConfig) writes to vaults/<targetType>/<id>.yaml
   • deleteConfig(sourceConfig) deletes vaults/<sourceType>/<id>.yaml
   • Since targetType === sourceType, these are the same file path (getPath = join(getTypeDir(type), id + ".yaml"))
   • Result: the config file is saved then immediately deleted. Vault config is lost.

   Breaking example:

   // Consumer bypasses preview, calls vaultMigrate directly
   await consumeStream(
     vaultMigrate(ctx, deps, {
       vaultName: "my-vault",
       targetType: "mock", // same as source type
       repoDir: "/repo",
     }),
     handlers,
   );
   // Result: vaults/mock/vault-1.yaml is deleted

   Suggested fix: Add the same-type check at the top of vaultMigrate, before any mutations:

   if (sourceConfig.type.toLowerCase() === input.targetType.toLowerCase()) {
     yield { kind: "error", error: validationFailed("Cannot migrate to the same type.") };
     return;
   }

Medium

2. src/libswamp/vaults/migrate.ts:282-291 — unhandled exception during secret copy breaks the event stream contract

   If targetProvider.put(key, value) throws mid-loop (e.g., target backend is unavailable, permission denied, quota exceeded), the exception propagates out of the generator unhandled. consumeStream uses for await...of, so the error bubbles up as a raw exception rather than a structured { kind: "error" } event. The source vault is intact (no data loss), but the error path is inconsistent with the vault-not-found and unknown-type cases that yield proper error events.

   Breaking example: Target is AWS Secrets Manager, network drops after secret 3 of 5. User sees an unhandled error with no structured indication of partial progress.

   Suggested fix: Wrap the copy loop + config swap in a try/catch that yields a kind: "error" event.

Low

3. src/domain/vaults/local_encryption_vault_provider.ts:284-289 — retry loop could read a partial key if the writer is mid-flush

   The loser-process retry reads via Deno.readTextFile, checking only winnerKey.length > 0. In theory, the file could contain partial content if the winner hasn't finished flushing. In practice, the key is 72 bytes and a single write() syscall for such small data is atomic on all major filesystems/OSes, so this is not a real risk. The retry logic and terminal error at line 290-294 are well-designed.

Verdict

FAIL — Finding #1 is a data-loss scenario where the vault config file is deleted when vaultMigrate is called without preview for a same-type migration. The fix is a one-line guard. The rest of the PR is solid: clean dependency injection, good test coverage, proper config-swap ordering, and sensible error handling.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. JSON field naming inconsistency between dry-run and completed output (vault_migrate.ts:141-145 vs vault_migrate.ts renderer / VaultMigrateData): --dry-run --json emits currentType/targetType/secretCount, but the completed migration JSON emits previousType/newType/secretsMigrated. A script that does a dry-run preview and then executes can't reuse the same field names to compare output. Consider aligning to a single convention (e.g., sourceType/targetType/secretCount throughout).

2. Confirmation prompt uses raw type IDs (vault_migrate.ts:158): the prompt reads Migrate vault backend from local_encryption to @swamp/aws-sm? but the preview lines just above already have the friendly name (Target: AWS Secrets Manager (@swamp/aws-sm)). Using the friendly name in the confirmation too (from Local Encryption to AWS Secrets Manager) would be more readable.

Verdict

PASS — the command is well-structured, both log and JSON modes are supported, error messages are clear and actionable, the confirmation/--force/--dry-run flow is consistent with other vault commands. The two suggestions above are polish items and do not block merge.

[github-actions]

Code Review

Well-structured PR that adds vault migration with a clean separation of concerns. The factory extraction, dependency injection for testability, and copy-then-swap safety model are all solid.

Blocking Issues

None.

Suggestions

1. Partial target cleanup on copy failure (src/libswamp/vaults/migrate.ts:244-254): When a secret copy fails mid-migration, the target provider retains partial state. The source vault is intact (good), but if the user retries, secrets already copied to the target may be duplicated or stale. Consider documenting this in the design doc's safety model section, or adding a note that retries are safe because put is idempotent.

2. Secret key logging (src/presentation/renderers/vault_migrate.ts:31): The log renderer prints each secret key name during migration (Copying secret 1/3: database_password). Key names aren't secret values, but in some environments they can leak information about what's stored. A --quiet flag or masking option could be a future consideration — not needed now.

DDD Assessment

• Factory pattern (vault_provider_factory.ts): Correctly extracted as a shared domain factory — single source of truth for provider instantiation used by both VaultService.registerVault() and the migrate operation.
• Application service (migrate.ts in libswamp): Properly orchestrates domain objects (VaultConfig, VaultProvider, VaultService) without leaking domain logic into the CLI layer.
• Dependency injection (VaultMigrateDeps): Clean seam for testing — all infrastructure dependencies are injectable.
• Domain knowledge (RENAMED_VAULT_TYPES → vault_types.ts): Correct relocation to the domain layer where it belongs, avoiding circular dependency on vault_service.ts.

Compliance

• ✅ AGPLv3 headers on all new .ts files
• ✅ Named exports throughout
• ✅ libswamp import boundary respected (CLI and renderer import from mod.ts)
• ✅ Both log and json output modes supported
• ✅ Unit tests co-located with source files
• ✅ Comprehensive test coverage (factory, preview, migrate, edge cases)
• ✅ AnyOptions pattern matches all 80 existing CLI commands
• ✅ Config swap ordering is safe (type-based directory structure means save/delete target different files)

[github-actions]

Adversarial Review

I traced every code path in this PR across the migration lifecycle (preview → confirm → copy → config-swap → cleanup). The architecture is sound: dependency injection allows thorough testing, the same-type guard prevents the critical config-deletion bug, and the save-new-then-delete-old ordering ensures no data loss on the happy path.

Critical / High

None found.

Medium

1. Crash between saveConfig and deleteConfig leaves nondeterministic state — src/libswamp/vaults/migrate.ts:317-325. If the process is killed after saveConfig(newConfig) succeeds but before deleteConfig(sourceConfig) completes, two YAML files exist for the same vault name: vaults/{oldType}/{id}.yaml and vaults/{newType}/{id}.yaml. On next load, VaultService.fromRepository walks all YAML files and calls registerVault for each; the last one loaded into the providers map wins, and walk order is filesystem-dependent. The user could end up on either the old backend (fully intact) or the new backend (potentially with only partial secrets if the crash happened mid-copy on a previous attempt that also crashed post-save).

   The design doc acknowledges this ("a duplicate entry that the user can clean up"), which is reasonable. No code change needed, but a log warning on startup when duplicate vault names are detected across type directories would make this recoverable without manual YAML archaeology. Not a blocker.

2. --config JSON not validated as an object — src/cli/commands/vault_migrate.ts:97-103. JSON.parse(options.config) can return a non-object (e.g., --config '42', --config '"hello"', --config 'null'). The result is assigned to targetConfig: Record<string, unknown> | undefined but JSON.parse returns any, so there's no runtime guard. A non-object value flows into resolveTargetConfig → createProvider and produces a confusing downstream error rather than a clear validation message. Example: swamp vault migrate my-vault --to-type local_encryption --config '42'. This is the same pattern as vault_create, so it's pre-existing, but worth noting since this is a new command surface.

Low

1. Preview instantiates target provider and discards it — src/libswamp/vaults/migrate.ts:181. deps.createProvider(input.targetType, input.vaultName, targetConfig) is called for validation purposes but the result is discarded. For extension providers whose constructors have side effects (e.g., establishing connections, writing temp files), this creates orphaned resources. Unlikely in practice since constructors should be lightweight, but the contract doesn't enforce it.

2. VaultConfig.create resets createdAt — src/libswamp/vaults/migrate.ts:311-316. Migration creates the new config via VaultConfig.create(sourceConfig.id, ...), which sets createdAt to new Date(). The vault's original creation timestamp is lost. Cosmetic only — no functional impact.

Verdict

PASS — Well-structured migration with good separation of concerns, comprehensive test coverage (including same-type rejection, empty vault, delete failure tolerance, and partial copy failure), and a deliberately conservative safety model. The medium findings are worth considering for follow-up but don't block this PR.