CLI UX Review

Blocking

None.

Suggestions

1. Log mode: quoted file list looks odd for ≤5 files (extension_auto_resolve.ts:181)
   The list variable is interpolated as a single LogTape placeholder, so it renders with quotes: missing 3 file(s): ".swamp/pulled-extensions/@swamp/aws/manifest.yaml, ...". The comment explains the tradeoff (avoid double-quoting), but a bulleted or newline-separated list of paths would be more scannable. Not blocking — the JSON path has all files unquoted.

2. Log mode doesn't hint at --json for large truncations (extension_auto_resolve.ts:177–178)
   When >5 files are missing, the log line says ... and 63 more. A trailing hint like (run with --json for the full list) on line 3 would be helpful. Optional improvement.

3. JSON path field in the truncated event is the extension root, not a file path — the field name is clear enough in context (path is used the same way in already_installed), but scripting consumers may need to know these are sibling concepts. Design doc already documents the shape, so this is fine.

Verdict

PASS. The new truncated error variant is actionable (names every missing file + the exact recovery command), structurally consistent with existing auto_resolve events, and the JSON output includes the missing array that scripts need. This is a clean UX improvement over the prior Unknown <kind> type fallback.

Adversarial Review

Critical / High

None found.

Medium

1. Sequential stat calls for large file lists — latency concern (src/cli/auto_resolver_adapters.ts:128-133)

   The inspectInstallation method issues one Deno.stat per file in the lockfile's files array, sequentially:

   for (const relPath of files) {
     try {
       await Deno.stat(join(repoDir, relPath));
     } catch {
       missing.push(relPath);
     }
   }

   An extension with hundreds of files (the PR body mentions 68 files for @swamp/digitalocean) will issue 68 sequential stat syscalls. On NFS, high-latency Docker volumes, or Codespaces mounts, each stat can take 5-20ms, adding 0.3-1.4s of wall time per extension per auto-resolve call. This runs on the hot path of every command that triggers auto-resolve.

   Not blocking because local fs is the typical target and the existing code already had per-directory stat costs, but worth noting as a potential papercut. A Promise.all batch (with concurrency limit) would parallelize the stats.

2. TOCTOU between inspectInstallation and install (src/domain/extensions/extension_auto_resolver.ts:361-383)

   The inspection reads filesystem state, then the code branches to install. Between the two, another process could create/delete files, changing the state. However, this is explicitly documented as defence-in-depth (the ConflictError catch on install covers the race), so this is a known design trade-off, not a bug. Calling it out for completeness.

3. Concurrency: two types from the same extension resolve simultaneously (src/domain/extensions/extension_auto_resolver.ts:160,345-375)

   The re-entrancy guard is per type (this.resolving is a Set<string> keyed on normalizedType). If two types from the same extension (e.g., @swamp/aws/ec2/instance and @swamp/aws/s3/bucket) resolve concurrently, both will call inspectInstallation("@swamp/aws") and both may proceed to install("@swamp/aws"). The defence-in-depth ConflictError catch on the second install prevents data corruption, so this is safe, but the second caller will get a silent null return from install and report failure even though the first caller succeeded. Pre-existing behavior, not introduced by this PR.

Low

1. btoa(String.fromCharCode(...bytes)) in integration test may blow the stack for large files (integration/auto_resolver_truncated_test.ts:136)

   String.fromCharCode(...bytes) spreads all bytes as arguments. V8's argument limit is ~65,536; a file larger than 64KB would throw RangeError: Maximum call stack size exceeded. Not a concern here since the fixture files are tiny hand-written strings, but the helper isn't safe for general use.

2. Empty missing array edge case in renderer (src/presentation/renderers/extension_auto_resolve.ts:155-186)

   If renderAutoResolveTruncated is ever called with missing: [], it would produce missing 0 file(s): with an empty list — grammatically odd but technically harmless. The domain layer guarantees missing.length > 0 before reaching the truncated branch, so this can't happen in practice.

3. entry.files type safety (src/cli/auto_resolver_adapters.ts:126)

   entry.files ?? [] assumes files is either string[] or absent. The UpstreamExtensionEntry interface declares files?: string[], so this is correct. However, a malformed lockfile with files: "not-an-array" would silently iterate characters. This would produce nonsensical missing-file entries but wouldn't crash or corrupt state. The lockfile is trusted internal state, so this is theoretical.

Verdict

PASS. This is a clean, well-structured fix. The tri-state inspection is a sound design choice — it correctly distinguishes the three cases that the old binary check conflated, the read-only invariant is well-enforced, and the test coverage is thorough (unit, domain, and integration layers all exercised). The new code paths are all properly guarded and the existing regression tests have been correctly ported to the new API shape. No blocking issues.

Code Review

Clean, well-structured fix. The tri-state InstallationInspection discriminated union is the right DDD building block — it's a value object (no identity, immutable, equality by structure) that lives in the domain layer and keeps the domain service free of infrastructure concerns. The port boundary is properly respected: inspectInstallation is defined on ExtensionInstallerPort in the domain, the adapter in the CLI layer implements it against the filesystem and lockfile, and the renderer handles both log and json output modes. The read-only approach (detect and report, don't auto-repair) is the right call for shared-repo / multi-worktree safety.

Blocking Issues

None.

Suggestions

1. Sequential stat calls in inspectInstallation (src/cli/auto_resolver_adapters.ts:128–133): The for loop does sequential Deno.stat per file. For extensions with many files (68 in the DigitalOcean example), Promise.allSettled would be faster. Not a concern for correctness — this isn't a hot path — but worth noting if perf profiling ever flags it.

2. Renderer unit test: renderAutoResolveTruncated in src/presentation/renderers/extension_auto_resolve.ts has no dedicated unit test. This is consistent with the existing pattern (other renderAutoResolve* functions in the same file also lack unit tests), and the integration test covers the JSON path indirectly, so not blocking. A follow-up adding renderer-level tests for the whole file would be a nice improvement.

Both items are non-blocking — the test coverage across unit (adapter + domain service) and integration layers is solid, the libswamp import boundary is respected (auto_resolver_adapters.ts imports from ../libswamp/mod.ts), and the design doc update accurately describes the new tri-state taxonomy.