CLI UX Review

Blocking

None.

Suggestions

1. extension_source_list.ts:62-65 — When a glob source expands to multiple paths (e.g. ~/code/aws/* → 5 roots), the log-mode output shows 5 extension roots but not the actual resolved paths. A --verbose expansion or a one-per-line indented list would help users confirm which directories were matched. Not blocking — the JSON output already includes expandedPaths for scripting use.

Verdict

PASS — New swamp extension source add/rm/list commands are well-structured, consistent with the extension trust pattern, both log and JSON output modes are implemented, error messages are clear and actionable, and flag naming (--only, --repo-dir) is consistent with the rest of the CLI.

Adversarial Review

Critical / High

No critical or high severity issues found.

Medium

1. src/cli/mod.ts:605 — Silent swallowing of .swamp-sources.yaml parse errors hides user mistakes.
   The outer catch {} at line 605 catches everything, including UserError from parseSwampSources() when the user writes invalid YAML (e.g., only: [invalid_kind]). The user gets no feedback — their sources file is silently ignored. Contrast with readSwampSources() which correctly re-throws non-NotFound errors. The outer try/catch defeats that.

   Breaking scenario: User adds only: [model] (missing the s) in .swamp-sources.yaml. The Zod validation throws a UserError, but the catch swallows it. Sources silently don't load, and the user has no idea why.

   Suggested fix: Catch Deno.errors.NotFound specifically, or at least log the error:

   } catch (err) {
     if (!(err instanceof Deno.errors.NotFound)) {
       logger.warn`Failed to load extension sources: ${err}`;
     }
   }

2. src/cli/repo_context.ts:82-91 — getSourceWorkflowDirs re-reads and re-resolves .swamp-sources.yaml on every call, including glob expansion and filesystem stat calls.
   This function is called from requireInitializedRepoReadOnly, requireInitializedRepo, and requireInitializedRepoUnlocked. Meanwhile, runCli already resolves sources once at line 600-604. For commands that go through runCli AND one of these repo context functions, the sources file is read, globs expanded, and directories stat'd twice. Same silent-catch issue as #1.

   Impact: Performance waste on repos with many glob sources (the PR description mentions 251+ AWS extensions). Not a correctness bug, but worth noting since it was explicitly designed to be "resolved once and shared across all loaders" per the comment at line 596.

3. src/infrastructure/persistence/swamp_sources_repository.ts:141-144 — Unsafe as cast of YAML content to RepoMarkerData.
   parseYaml(content) as RepoMarkerData trusts arbitrary YAML from the source's .swamp.yaml without validation. If the source repo has a malformed marker (e.g., modelsDir: 42 or modelsDir: ["an", "array"]), this becomes a non-string value that flows into resolve() at line 167. The resolve() call would likely coerce to string, but the behavior is undefined.

   Suggested fix: Validate the marker through the same schema the main repo marker uses, or at least guard resolveKindDir to return the default when the field isn't a string.

Low

1. src/domain/models/user_model_loader.ts:551 — migrateOldFlatBundles uses String.includes("pulled-extensions") to find the pulled dir, which is fragile. If someone names their source directory something containing "pulled-extensions", old bundles could migrate to the wrong namespace. Using the known constant SWAMP_SUBDIRS.pulledModels path suffix would be more precise.

2. src/infrastructure/persistence/paths.ts:146-154 — FNV-1a hash collision risk. 32-bit hash has a ~50% collision probability at ~77k inputs (birthday paradox). With only 8 hex chars, two different source directories could theoretically hash to the same namespace. In practice, a repo with hundreds of source dirs is unlikely to hit this, but worth acknowledging.

3. src/domain/models/user_model_loader.ts:1009-1029 — findNearestDenoConfig uses sync I/O (Deno.statSync) in an otherwise async code path. Not a bug, but sync filesystem calls block the event loop. With many source extensions, this could add up.

Verdict

PASS — The code is well-structured, follows existing patterns consistently, has good test coverage (24+ new tests), and the bundle namespace migration is carefully handled. The medium findings are quality-of-life issues (silent error swallowing, duplicate work) rather than correctness bugs. The core design — namespace by hash, migrate on version change, source > pulled priority — is sound.

Code Review

Well-structured PR that adds .swamp-sources.yaml for loading extensions from external filesystem paths and fixes bundle cache collisions (#1065) via namespace hashing. The architecture follows established patterns cleanly.

Blocking Issues

None.

Suggestions

1. Redundant source resolution in repo_context.ts: getSourceWorkflowDirs() is called independently in three places (requireInitializedRepoReadOnly, requireInitializedRepo, requireInitializedRepoUnlocked), each re-reading and re-parsing .swamp-sources.yaml. Meanwhile, mod.ts already resolves sources once for all other extension kinds. Consider passing the already-resolved workflow dirs through or caching them to avoid redundant I/O on every command invocation.

2. migrateOldFlatBundles uses sync I/O: The method uses Deno.readDirSync, Deno.mkdirSync, and Deno.renameSync on the hot path. This is fine for migration (runs once), but worth noting it blocks the event loop during migration of repos with many flat bundles.

3. DDD observation: The resolveKindDir, setKindDir, and getKindDir functions in swamp_sources_repository.ts use switch statements over ExtensionKind. If new kinds are added, all three switches need updating. A map-based approach would be more maintainable, but the current approach matches existing patterns in the codebase.

What looks good

• Import boundaries respected: CLI commands and presentation renderers import from libswamp/mod.ts; libswamp imports from infrastructure directly — both consistent with established patterns.
• Clean DDD layering: Domain types (swamp_sources.ts) contain only Zod schemas and types; infrastructure (swamp_sources_repository.ts) handles file I/O and glob expansion; application services (add.ts, remove.ts, list.ts) orchestrate operations; CLI commands are thin wiring.
• Dependency injection: SourceAddDeps, SourceRemoveDeps, SourceListDeps enable clean testing without filesystem access.
• Both output modes: All new commands support log and json output modes as required.
• Comprehensive test coverage: 24+ new tests covering domain parsing, infrastructure CRUD, libswamp operations, catalog store versioning, bundle namespace hashing, and integration.
• Safe migration: Old flat bundles are moved (not deleted) to preserve pre-built bundles from pulled extensions. Layout version check forces rescan only when needed.
• Bundle namespace design: Using relative(repoDir, baseDir) as hash input avoids macOS /var vs /private/var symlink issues. FNV-1a hash is appropriate for this use case.
• License headers: All new files include the AGPLv3 copyright header.
• No fire-and-forget promises: All async operations are properly awaited.