CLI UX Review

Blocking

None.

Suggestions

1. JSON field names expose internal phase jargon (repo_service.ts, ExtensionLayoutMigrationSummary): phase1MovedCount and phase2DeletedPerExtension are now stable public JSON API. Users scripting against swamp repo upgrade --json will see these names with no context for what "phase1" or "phase2" mean. Consider renamedFileCount / deletedPerExtension for clearer, more stable naming.

2. "flat-layout file(s)" is internal jargon (repo_init.ts:132): The log line Removed Y flat-layout file(s) across Z extension(s) uses the term "flat-layout" which has no meaning to users who haven't read the design doc. "Removed Y outdated file(s) across Z extension(s)" communicates the same intent without leaking the internal layout taxonomy.

3. Next-step hint appends implementation detail (repo_init.ts:139): "The lockfile's stored checksum is verified on each re-pull." is useful for security-conscious users but is noise for the typical migration case. The actionable part ends at Run 'swamp extension install' to restore these extensions into the per-extension layout.

Verdict

PASS — the warn-not-block migration, per-extension output in repo upgrade, and JSON extensionMigration field all land cleanly from a UX perspective. No blocking issues.

Code Review

Blocking Issues

1. libswamp import boundary violation (src/cli/mod.ts:25): Imports enumeratePulledExtensionDirs from the internal path "../libswamp/extensions/enumerate_pulled.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 function is already exported from mod.ts (line 646), so this is a one-line fix.

2. libswamp import boundary violation (src/cli/repo_context.ts:49): Same issue — imports enumeratePulledExtensionDirs from "../libswamp/extensions/enumerate_pulled.ts" instead of "../libswamp/mod.ts".

Suggestions

1. DDD alignment looks good: ExtensionLayoutGeneration is a clean union-type value object, LegacyFileEntry/LegacyLayoutSummary/ExtensionLayoutMigrationSummary are properly immutable summary types, migration logic correctly lives in RepoService, and enumeratePulledExtensionDirs is well-placed as a libswamp query helper.

2. The migration phase separation (phase 1: rename gen-1→gen-2, phase 2: delete gen-2 for re-install) is well-designed. The lockfile-as-coordination-primitive pattern — preserving it on fatal errors, tolerating NotFound on retry — makes partial migrations safely resumable.

3. Test coverage is solid: 5 migration tests covering happy path, partial retry tolerance, idempotency, fatal IO error abort, and mixed-generation state; 6 enumerate-helper tests; checksum-mismatch test; auto-resolver adapter regression tests. Good edge case coverage.

4. The integrity anchor design (lockfile-anchored SHA-256 verification on restore, opt-in on explicit pull, graceful skip for pre-checksum entries) is a nice security improvement.

Adversarial Review

Critical / High

No critical or high severity issues found. The code is well-structured for a large migration change. Key safety mechanisms are solid: the archive path traversal checks (.. and / prefix rejection at pull.ts:743-747, symlink escape validation at pull.ts:770-776), the checksum integrity anchor (pull.ts:700-721), the advisory lock around lockfile writes (pull.ts:217-297), and the atomicWriteTextFile for crash-safe lockfile updates all look correct.

Medium

1. Phase 1 lockfile write bypasses advisory lock — repo_service.ts:1751-1754: migrateExtensionLayoutPhase1 rewrites upstream_extensions.json via atomicWriteTextFile directly, without acquiring the .lock file that updateUpstreamExtensions (pull.ts:260-297) uses for concurrency safety. If a user concurrently runs swamp extension pull during swamp repo upgrade, the two writers can clobber each other. Low probability since repo upgrade is typically run standalone, but the advisory lock exists for exactly this reason.

2. Stale advisory lock after crash — pull.ts:217-240, 291-297: The advisory lock file contains no PID or timestamp. If the process is killed between acquireLock and the finally block's Deno.remove, the .lock file remains permanently. All subsequent operations fail after 10 retries (1 second) with "Another pull may be in progress." The only recovery is manual deletion. Consider writing the PID and checking liveness on conflict, or at minimum adding a stale-lock age check.

3. Broad catch {} in fileExists — pull.ts:341-348: Returns false for permission errors, not just NotFound. If a file exists but is unreadable, detectConflicts skips it and the install silently overwrites it. Same pattern appears in validateSourceCompleteness (pull.ts:603) and collectTsFiles (pull.ts:635).

4. hasAnyMissingFiles returns false for empty files array — install.ts:137-153: An extension whose lockfile entry has files: [] (e.g., from a crash after lockfile write but before file extraction, or a corrupt lockfile edit) is treated as "up to date" and never re-installed. The checkForMissingPulledExtensions guard in mod.ts:676-681 also skips the extension because it filters to source files and finds none. This is a narrow edge case but could cause confusion.

5. PULLED_TYPE_DIRS duplicated between layout.ts and repo_service.ts — layout.ts:43-52 defines PULLED_TYPE_DIRS, repo_service.ts:1783-1792 independently defines pulledTypeDirs with the same entries. If one is updated without the other, classifyExtensionFile and migrateExtensionLayoutPhase2 would disagree on what constitutes a gen-2 file vs. a current-layout file. Should be a shared constant.

Low

1. Redundant lockfile reads in dependency resolution — pull.ts:1096: Each dependency iteration re-reads and re-parses the lockfile to check if a dependency is already installed. With MAX_DEPENDENCY_DEPTH = 10 and multiple deps, this is wasteful. The lockfile was just written at line 1075. A cached read would be cleaner.

2. resolveSwampBinaryPath uses synchronous outputSync() — repo_service.ts:1349: Blocks the event loop for which swamp. Minor since this runs once during upgrade, but inconsistent with the project's async conventions.

3. Lockfile path resolution copy-pasted three times — repo_context.ts:252-260, 386-395, 493-501: Identical lockfile path construction logic (with double-call to resolveModelsDir) is repeated verbatim. Maintenance hazard but not a correctness issue.

4. checkForMissingPulledExtensions ignores .js files — mod.ts:678-681: Missing bundle .js files won't trigger the warning. Intentional per the comment, but could mask real breakage if an extension's only artifact is a bundled JS file.

Verdict

PASS — The migration design is sound: phase 1 renames (reversible), phase 2 deletes with lockfile-anchored re-install, and the integrity checksum prevents silent registry drift. The archive extraction has proper path traversal guards. No data loss or security issues in production paths. The medium items are worth addressing in follow-ups but none block this PR.

CLI UX Review

Blocking

None.

Suggestions

1. Warn message omits extension names — warnLegacyExtensionLayout emits "3 extension(s) pending migration. Run 'swamp repo upgrade' to complete." The count is there but the names aren't. summariseLegacyLayout already returns extensionNames; surfacing them (e.g. @swamp/aws/ec2, @swamp/aws/eks pending migration...) would let users confirm which ones need upgrading without a separate lookup.

2. Awkward pluralization in repo upgrade output — "1 file(s)", "1 extension(s)" is a minor roughness. Simple ternary (file(s) → file / files) would polish the log output.

Verdict

PASS — The migration summary added to repo upgrade is clear and includes an explicit CTA (swamp extension install). The checksum-mismatch and migration-failure error messages are actionable and tell users exactly what to run. Flipping warn-instead-of-block is a net UX improvement. JSON mode inherits extensionMigration automatically via JSON.stringify(e.data). No blocking issues.

Code Review

Well-structured PR that solves a real data-integrity problem (silent cross-extension file overwrites) with a clean per-extension layout, integrity anchoring, and a resumable migration path. Comprehensive test coverage with 23+ new tests covering happy paths, edge cases (NotFound tolerance, IO abort), and regressions. Design doc updated, API surface cleaned up.

Blocking Issues

None.

Suggestions

1. Domain→libswamp internal import — src/domain/repo/repo_service.ts:27 imports PULLED_TYPE_DIRS directly from ../../libswamp/extensions/layout.ts (internal path) rather than from ../../libswamp/mod.ts. While the CLAUDE.md import boundary rule literally targets CLI commands and presentation renderers, this creates a domain→application-layer dependency that inverts the DDD dependency direction. Consider importing through mod.ts, or better, promoting PULLED_TYPE_DIRS into a shared domain/infrastructure location since it's fundamentally a filesystem-convention constant used by both layers.

2. Repeated lockfile-path computation in repo_context.ts — the pattern isAbsolute(resolveModelsDir(marker)) ? resolveModelsDir(marker) : resolve(repoPath.value, resolveModelsDir(marker)) appears three times (lines ~252, ~385, ~493), calling resolveModelsDir(marker) twice per occurrence. A small helper (e.g. absoluteModelsDir(marker, repoPath)) would reduce duplication and the redundant calls.

3. Deprecated requireCurrentExtensionLayout uses dynamic import — the backwards-compat shim at layout.ts:183 lazily imports UserError via await import(...) to avoid circular deps. If the function is truly deprecated and all call sites have migrated to warnLegacyExtensionLayout, consider removing it entirely rather than maintaining it with the dynamic-import workaround. If it must stay for one more release cycle, that's fine, but note the removal in a follow-up issue.

Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. src/cli/repo_context.ts:252-259, 383-392, 475-484 — Repeated inline lockfile-path computation is fragile and calls resolveModelsDir(marker) twice per site.

   Three nearly identical 8-line blocks compute the lockfile path inline. Each calls resolveModelsDir(marker) twice (once for the isAbsolute check, once for the resolve). While resolveModelsDir is pure (no side effects) and this is functionally correct, the triplication makes it easy for a future editor to fix one site and miss the other two. A local lockfilePath helper or a shared utility would be more robust.

   Breaking example: If resolveModelsDir were ever made stateful or expensive (unlikely but possible), this would double the cost. More practically, a future change to the path convention only updated in one of three places would produce incorrect workflow enumeration in the other two.

   Suggested fix: Extract a buildLockfilePath(repoDir, marker) function used by all three call sites, or at minimum bind const modelsDir = resolveModelsDir(marker) once before the isAbsolute check.

2. src/libswamp/extensions/layout.ts:74-86 — classifyExtensionFile assumes extension names always start with @ to distinguish from type dirs, but PULLED_TYPE_DIRS could collide with a hypothetical future non-scoped name.

   Currently safe because validateExtensionName enforces @scope/name format, but classifyExtensionFile has no direct dependency on that invariant. If an extension named e.g. models/evil were ever allowed (it wouldn't pass the current regex, but the layout classifier doesn't know that), it would be misclassified as gen-2.

   Suggested fix: No code change needed today — the scoped-name constraint is enforced. This is a documentation-level concern: a comment noting the invariant dependency would prevent future drift.

3. src/libswamp/extensions/pull.ts:700-701 — Lockfile-anchored checksum comparison uses raw string equality between two hex checksums. If the lockfile were ever hand-edited or a future serialisation prepended sha256- (as the design doc's example "sha256-…" suggests), the comparison would always fail.

   The design doc at line 32 of the diff ("checksum": "sha256-…") uses a sha256- prefix in its illustrative example, while computeChecksum returns raw hex. If someone reads the design doc and manually constructs or patches a lockfile entry with a sha256- prefix, every restore would trigger a false-positive mismatch and tell the user their registry has drifted.

   Suggested fix: Either normalise the comparison (strip any sha256- prefix before comparing), or update the design doc example to use raw hex to match the actual stored format.

Low

1. src/libswamp/extensions/enumerate_pulled.ts:54 — readUpstreamExtensions is called on every type enumeration (models, workflows, vaults, drivers, datastores, reports). In src/cli/mod.ts, this means the lockfile is read and parsed 5 times during startup.

   Each of loadUserModels, loadUserVaults, loadUserDrivers, loadUserDatastores, and loadUserReports independently calls enumeratePulledExtensionDirs, which reads and parses the lockfile. The lockfile is small and this is I/O-cached by the OS, so the practical cost is negligible, but a single parse shared across all loaders would be cleaner.

2. src/domain/repo/repo_service.ts:1749 — Phase 1 migration assigns newPath even when the rename was skipped (NotFound). The lockfile entry is still rewritten to newPath for a file that doesn't exist on disk.

   When Deno.rename throws NotFound (source file missing), the code catches it silently but still pushes newPath into updatedFiles. This means the lockfile path is rewritten to the new location even though the file was never actually moved there. Subsequent tooling will look for the file at the new path and not find it — which is the same outcome as if it were at the old path. Not harmful because extension install handles missing files by re-pulling, but it's semantically sloppy.

3. src/libswamp/extensions/pull.ts:975 — Deno.mkdir(absoluteFilesDir, { recursive: true }) is called unconditionally before copyDir, even when the archive has no files/ directory. This creates an empty files/ dir in every extension's subtree.

   Harmless but produces filesystem noise. The models, workflows, etc. dirs are only created by copyDir when content exists; files is the exception.

Verdict

PASS — The code is well-structured, thoroughly tested, and handles the hard parts (migration idempotency, selective delete with abort semantics, integrity verification, shared-scope-root preservation) correctly. The gen-1 → gen-2 → current classification logic is sound, the lockfile coordination invariants hold, and the error paths are well-tested. Medium findings are about future robustness, not current correctness.