CLI UX Review

Blocking

1. swamp repo upgrade --json emits two JSON objects — src/presentation/renderers/repo_init.ts

   JsonRepoUpgradeRenderer delegates extensions events to installHandlers, which is the JSON extension-install renderer. That renderer unconditionally calls console.log(JSON.stringify(e.data, null, 2)) when it sees completed. Then the upgrade completed handler prints a second JSON object for the upgrade result.

   On every swamp repo upgrade --json run the output is now two separate objects:

   {
     "entries": [...],
     "installed": 0,
     "migrated": 0,
     "upToDate": 2,
     "failed": 0
   }
   {
     "path": "...",
     "previousVersion": "...",
     ...
   }

   This is invalid JSON and breaks any script that was parsing the output (e.g. swamp repo upgrade --json | jq .path now errors). Before this PR swamp repo upgrade --json emitted exactly one object.

   Suggested fix: In JsonRepoUpgradeRenderer, suppress the install-completed output by not forwarding the inner completed event to the install renderer — or forward it only when there were actual installs/migrations, and fold the counts into the upgrade JSON object instead. The simplest fix is to skip the completed dispatch in dispatchInstallEvent when called from the upgrade JSON renderer (e.g. pass a { skip: ['completed'] } option, or just omit the completed case from the JSON renderer's dispatchInstallEvent call).

Suggestions

1. Log mode: "All extensions up to date." appears before the upgrade summary — src/presentation/renderers/repo_init.ts

   On a healthy repo with no migrations needed, swamp repo upgrade now prints:

   All extensions up to date.
   Repository upgraded: /path/to/repo
     Version: 1.0.0 → 2.0.0
     ...
   

   The extension-install summary fires before the upgrade summary and lacks any contextual framing (no "Extension layout:" prefix or similar). Users who don't know that upgrade now runs an install pass will be confused by the extra leading line. Similarly, repos with no extensions print "No extensions in lockfile." before the upgrade summary, which is surprising.

   Consider suppressing the no-op install completion message (when installed === 0 && migrated === 0 && failed === 0) within the upgrade log renderer, or prefixing it with something like Extensions: so it reads as a sub-step of the upgrade.

Verdict

NEEDS CHANGES — swamp repo upgrade --json now emits two JSON objects, breaking machine-readable consumers. The log-mode output is cosmetically noisy but functional.

Code Review

Blocking Issues

1. Libswamp import boundary violation in src/cli/create_extension_install_deps.ts:27: imports ExtensionInstallDeps from the internal path ../libswamp/extensions/install.ts instead of ../libswamp/mod.ts. Per CLAUDE.md: "CLI commands and presentation renderers must import libswamp types and functions from src/libswamp/mod.ts — never from internal module paths." The type is already re-exported from mod.ts (line 689), so this is a one-line fix:

   -import type { ExtensionInstallDeps } from "../libswamp/extensions/install.ts";
   +import type { ExtensionInstallDeps } from "../libswamp/mod.ts";

Suggestions

1. JSON mode outputs two documents for swamp repo upgrade --json: The extensions event dispatches to the install JSON renderer, which calls console.log(JSON.stringify(e.data)) on the completed event. Then the upgrade's own completed handler emits a second JSON document. Pre-PR, the extension migration data was embedded in RepoUpgradeData, so JSON consumers got one document. Now they get two (extension install data, then upgrade data). Consider either (a) suppressing the install completed output in the JSON upgrade renderer, or (b) documenting this as an intentional change.

2. dispatchInstallEvent lacks an exhaustiveness check (src/presentation/renderers/repo_init.ts:152): The switch on event.kind has no default case with a never assertion. If a new event kind is added to ExtensionInstallEvent, the compiler won't flag the missing branch. Adding default: { const _: never = event; } is a one-liner that prevents silent drops.

What looks good

• Collapsing the split between migration and install into a single extensionInstall call is a clean DDD improvement — "bring extension state into alignment with the lockfile" is one domain concept, not two. The removal of ~280 LOC from repo_service.ts and the corresponding test migration to install_test.ts keeps responsibility where it belongs (the application/libswamp layer orchestrates, not the domain service).
• The sweepLegacyPaths ↔ needsInstallOrMigration separation gives each function a single job with clean test coverage (8 new tests covering install, migration, failure-preserves-legacy, NotFound tolerance, mixed lockfiles, and empty-dir pruning).
• The resolve(repoDir) absolutization fix in createExtensionInstallDeps addresses a real latent bug with relative-path startsWith comparisons, and the regression test for nested parent dir pruning locks it down.
• Test seam via installExtensionFn is well-designed — avoids needing a real registry/tar in unit tests while still exercising the full migrate-then-sweep flow.
• Renderer delegation via dispatchInstallEvent keeps upgrade output consistent with standalone swamp extension install output.

Adversarial Review

Critical / High

1. src/presentation/renderers/repo_init.ts:131-136 — repo upgrade --json emits two JSON objects, breaking the --json contract

   The JsonRepoUpgradeRenderer now dispatches extension install events to JsonExtensionInstallRenderer, whose completed handler does console.log(JSON.stringify(e.data, null, 2)). Then the upgrade's own completed handler also does console.log(JSON.stringify(e.data, null, 2)). This produces two top-level JSON objects on stdout:

   { "entries": [...], "installed": 0, "migrated": 0, ... }
   { "path": "...", "previousVersion": "...", "newVersion": "...", ... }

   Any downstream tool doing swamp repo upgrade --json | jq . or parsing the output as a single JSON value will break. The old renderer only ever emitted one object (the upgrade data, with extensionMigration embedded in it).

   Breaking example: swamp repo upgrade --json on any repo with a lockfile (even if all extensions are up-to-date), then piping to jq or any single-document JSON parser.

   Suggested fix: Either (a) suppress the extension install JSON output during upgrade by passing a no-op renderer, or (b) accumulate the extension install data and embed it in the RepoUpgradeData object so --json still emits a single document, or (c) for the JSON upgrade renderer, replace the extensions handler with a no-op (only emit the consolidated result in completed).

Medium

2. src/libswamp/extensions/install.ts:152-175 — sweepLegacyPaths failure after successful installExtension creates unrecoverable orphaned files

   If installExtension succeeds (new files written, lockfile rewritten to current-layout paths) but sweepLegacyPaths throws on a non-NotFound error (e.g., permission denied), the catch block marks the entry as "failed". In repoUpgrade (init.ts:275-289), this triggers the error path with message "Legacy files have been preserved. Re-run 'swamp repo upgrade'."

   But the lockfile was already rewritten by installExtension — its files[] now points at the new per-extension subtree. On re-run, needsInstallOrMigration reads the updated lockfile, finds all files present at current-layout paths, returns "up_to_date", and the orphaned legacy files are never swept. The error message's advice to re-run is a dead end.

   Breaking example: Extension @org/ext has a legacy file at extensions/models/thing.ts owned by root. installExtension succeeds (writes .swamp/pulled-extensions/@org/ext/models/thing.ts, rewrites lockfile). Deno.remove("extensions/models/thing.ts") fails with permission denied. Entry is "failed", user told to re-run. Re-run skips it because lockfile says current layout, all files present.

   Suggested fix: Either (a) snapshot the lockfile before calling installExtension and restore it on sweep failure so the next run retries the migration, or (b) catch sweep failures separately from install failures and report them as a non-fatal warning (the migration succeeded, just the cleanup didn't).

3. src/cli/commands/repo_init.ts:146-149 — Extension install deps built from pre-upgrade marker, stale if --tool changes

   createExtensionInstallDeps(repoDir, ...) is called before repoUpgrade() runs. It reads the current marker to derive skillsDir via resolveSkillsDir(repoDir, marker.tool). If the user runs swamp repo upgrade --tool cursor on a repo that was previously tool: "claude", the upgrade changes the marker to cursor, but the install deps still have skillsDir computed from "claude". Extension skills get installed to the wrong tool directory.

   Breaking example: swamp repo upgrade --tool cursor on a claude-initialized repo with extensions that provide skills.

   Suggested fix: Build extensionInstallDeps lazily inside repoUpgrade after deps.upgrade() completes, or re-read the marker post-upgrade to compute the correct skillsDir.

Low

4. src/libswamp/extensions/install.ts:199-219 — needsInstallOrMigration does not short-circuit on first legacy classification

   The function continues iterating and stat-ing all remaining files after finding the first legacy file, when it could return "migrate" early (similar to how it short-circuits on the first missing file). Not a correctness issue — just unnecessary I/O on large extension file lists with legacy paths.

Verdict

FAIL — The --json output regression (#1) is a contract-breaking change that will silently corrupt machine-readable output for all repo upgrade --json consumers. Fix that, and the remaining findings are addressable or acceptable.

Code Review

Well-structured PR that correctly consolidates the two-phase legacy migration into the existing extensionInstall primitive. The approach — detect legacy layout, re-pull via the normal install path, then sweep orphaned files — is cleaner than the deleted migrateExtensionLayout approach and eliminates the "silent delete + manual follow-up" UX problem.

Blocking Issues

None.

Suggestions

1. Stale doc comment on PULLED_TYPE_DIRS (src/libswamp/extensions/layout.ts:42-46): The JSDoc still references "the phase-two migration in RepoService" which was deleted in this PR. Worth updating the comment to reference sweepLegacyPaths / classifyExtensionFile as the remaining consumers.

2. Shared alreadyPulled set: Each createInstallContext call in createExtensionInstallDeps creates a fresh new Set(). If two lockfile entries share a transitive dependency, it will be pulled twice. This is pre-existing behavior (the old extension_install.ts command had the same pattern), so not a regression — but worth noting as a potential optimization for a follow-up.

3. dispatchInstallEvent exhaustiveness: The switch statement handles all current event kinds but lacks a default: { const _: never = event; } guard. TypeScript's narrowing would catch a new unhandled variant at deno check time anyway, so this is minor — an explicit exhaustiveness check would make the intent clearer.

What looks good

• DDD alignment: The composition root (createExtensionInstallDeps) lives correctly in the CLI layer, wiring infrastructure (API client, marker repo) into a domain-facing deps interface. The domain service (RepoService) is properly slimmed — migration logic moved to the application service (extensionInstall) where it belongs as cross-aggregate orchestration.
• Import boundary: All CLI and presentation imports go through src/libswamp/mod.ts. The libswamp/repo/init.ts → ../extensions/install.ts import is internal to libswamp, which is correct.
• Test coverage: 8 new tests covering needsInstallOrMigration, sweepLegacyPaths, the full extensionInstall migration flow (happy path, mixed lockfile, failure preservation), and the JSON renderer's single-object invariant. The test seam (InstallExtensionFn) avoids coupling tests to a real registry.
• Error handling: Legacy files are preserved on install failure, the error surfaces extension names and a recovery command, and NotFound is tolerated for interrupted-retry scenarios.
• Relative path bug fix: Absolutizing repoDir in the factory prevents the startsWith comparison failure in cleanupEmptyParentDirs. The regression test for nested _lib/ directories covers this.
• JSON output invariant: The JsonRepoUpgradeRenderer correctly captures the install summary and folds it into the single completed payload instead of delegating to JsonExtensionInstallRenderer (which would emit a second JSON object mid-stream).
• License headers: Present on both new files.
• Net -170 LOC with better behavior — good cleanup.

CLI UX Review

Blocking

None.

Suggestions

1. Extra log noise on every repo upgrade — src/presentation/renderers/repo_init.ts delegates to the extension install renderer unconditionally, so every upgrade now prints Reading lockfile... and either All extensions up to date. or No extensions in lockfile. even when no migration is needed. These lines are already appropriate for swamp extension install, but they read oddly as part of swamp repo upgrade on a clean repo. Consider suppressing resolving/completed events in LogRepoUpgradeRenderer when migrated === 0 and failed === 0, or adding a guard in dispatchInstallEvent to skip no-op results.

2. Upgrade success is silent when extension migration fails — in src/libswamp/repo/init.ts the completed event (which prints Upgraded swamp repository: 0.1.0 → 0.1.1) is never yielded when any extension fails to migrate. The base upgrade (marker, skills, settings) DID succeed, but the user only sees the extension error. They have no log confirmation that the repo itself was upgraded. A line like Upgraded swamp repository: X → Y; extension migration incomplete — re-run to finish. before the error would close that gap.

3. swamp extension install failure summary omits a recovery command — src/presentation/renderers/extension_install.ts:78 warns {count} extension(s) failed to install. with names and errors, but gives no next-step command. swamp repo upgrade error message correctly says Re-run 'swamp repo upgrade'; parity here would be Re-run 'swamp extension install' once the issue is resolved. (Pre-existing gap, surfaced by this PR's unification — low priority.)

Verdict

PASS — JSON output is correct (single top-level object, extensionInstall folded in cleanly), error messages are actionable and name the affected extensions with a recovery command, log-mode output is consistent with existing patterns. Suggestions above are all cosmetic.

Adversarial Review

Critical / High

None found. The core install→sweep sequence is correct: legacy paths are snapshotted before installExtension rewrites the lockfile, the sweep only runs after a successful install (preserving legacy files on failure), and classifyExtensionFile prevents current-layout paths from being deleted. Error handling is clean — per-entry failures are captured without aborting the loop, and infrastructure errors bubble up through the generator's try/catch.

Medium

1. cleanupEmptyParentDirs silently fails on relative repoDir — patched at callsite only (src/cli/create_extension_install_deps.ts:49, src/infrastructure/persistence/directory_cleanup.ts:37). The factory calls resolve(repoDir) to absolutize, which fixes the immediate bug (empty _lib/ dirs stranded). But cleanupEmptyParentDirs's guard currentDir.startsWith(stopAtDir) still silently no-ops for relative paths. Any future caller of sweepLegacyPaths that passes a non-absolute repoDir (e.g. "." from resolveRepoDir's default) would reintroduce the stranded-directory bug. Consider adding resolve() inside sweepLegacyPaths or cleanupEmptyParentDirs itself as a follow-up.

Low

1. sweepLegacyPaths calls cleanupEmptyParentDirs unconditionally after Deno.remove NotFound (src/libswamp/extensions/install.ts:248). When the file was already gone, the cleanup walks parent dirs that may themselves not exist. cleanupEmptyParentDirs handles this gracefully (its own NotFound catch), so this is just unnecessary I/O, not a bug.

2. dispatchInstallEvent routes error events to install renderer which throws UserError (src/presentation/renderers/repo_init.ts:183). Currently harmless because extensionInstall never yields error events (per-entry failures are captured in completed). But if a future refactor adds error events to extensionInstall, the throw inside the extensions handler would bypass the upgrade renderer's own error handler, producing an uncontrolled exception path.

3. createExtensionInstallDeps reads marker before upgrade runs (src/cli/commands/repo_init.ts:111-114). The factory snapshots modelsDir and tool from the pre-upgrade marker. If upgrade() modifies these fields, the extension install pass uses stale values. In practice modelsDir doesn't change during upgrade, making this theoretical only.

Verdict

PASS. The migration logic is sound: install-before-sweep ordering prevents data loss, the failure path preserves legacy files, and the JSON single-object invariant is tested. The relative-path fragility in cleanupEmptyParentDirs is worth a defensive follow-up but doesn't block — the production callsite is correctly absolutized. Net code reduction with better behavior; well tested.