Review Summary

This is a well-implemented performance optimization that fixes the ~42 second startup time for swamp --help and other commands that don't need user extensions. The changes are clean, safe, and well-tested.

No Blocking Issues Found

Code Quality ✓

• TypeScript strict mode: No any types introduced, proper typing throughout
• Named exports: commandNeedsExtensions correctly exported for testing
• License headers: All files have AGPLv3 headers

DDD Compliance ✓

• EmbeddedDenoRuntime is appropriately in the infrastructure layer
• Loader functions remain application-level orchestration
• No domain logic violations - purely CLI/infrastructure optimization

Test Coverage ✓

• Comprehensive tests added for commandNeedsExtensions():
  • All 9 skip commands tested
  • Commands that need extensions tested (model, workflow, data)
  • Global flags before commands tested
• Existing EmbeddedDenoRuntime tests verify caching behavior

Correctness ✓

1. SKIP_EXTENSION_COMMANDS: Conservative list of commands that definitely don't need extensions
2. Parallel Promise.all(): Safe because each loader writes to its own registry and has internal try-catch (never throws)
3. Shared EmbeddedDenoRuntime: Safe with new extractionPromise caching to prevent concurrent extractions
4. Shared marker: Good optimization - reads once instead of 4 times

Security ✓

No vulnerabilities introduced - purely a performance optimization with no new input handling.

Suggestions (Non-blocking)

1. Minor: Consider adding a test case for commandNeedsExtensions(["--help"]) to explicitly verify this edge case (though it's implicitly covered since extractCommandInfo(["--help"]) returns {command: ""} which matches the empty string in SKIP_EXTENSION_COMMANDS).

CI passing (lint, test, format, dependency audit). Good to merge!

Adversarial Review

I systematically attempted to break this code across logic, error handling, concurrency, and data integrity dimensions.

Critical / High

None found.

Medium

1. Two separate EmbeddedDenoRuntime instances are created (src/cli/mod.ts:510 and src/cli/mod.ts:538)

When both commandNeedsExtensions(args) is true AND trustedCollectives.length > 0 && marker is true (the common case), two separate EmbeddedDenoRuntime instances are created:

// Line 510 - for extension loading
if (commandNeedsExtensions(args)) {
  const denoRuntime = new EmbeddedDenoRuntime();  // instance 1
  await Promise.all([...]);
}

// Line 538 - for auto-resolver
if (trustedCollectives.length > 0 && marker) {
  const denoRuntime = new EmbeddedDenoRuntime();  // instance 2
  ...
}

While functionally correct (the file-level version marker prevents double extraction), this means:

• Redundant object instantiation
• Each instance maintains its own cachedPath and extractionPromise state
• The second instance will re-check the disk even though extraction is already complete

Suggested fix: Move EmbeddedDenoRuntime creation before the if (commandNeedsExtensions(args)) block and share the single instance across both usages:

const denoRuntime = new EmbeddedDenoRuntime();

if (commandNeedsExtensions(args)) {
  await Promise.all([
    loadUserModels(repoDir, marker, denoRuntime),
    ...
  ]);
}
// ... later ...
if (trustedCollectives.length > 0 && marker) {
  setAutoResolver(new ExtensionAutoResolver({
    extensionInstaller: createAutoResolveInstallerAdapter({
      denoRuntime,  // reuse same instance
      ...
    }),
  }));
}

Low

1. Test coverage gap for --option=value syntax (src/cli/mod_test.ts)

The tests verify --json (a boolean flag in GLOBAL_OPTIONS) works correctly, but don't test value-taking options with = syntax like --log-level=debug. The underlying extractCommandInfo() function has a pre-existing parsing quirk where --option=value style args incorrectly skip the next positional argument.

Example: ["--log-level=debug", "model"] would parse command as "" because model is skipped as a presumed option value.

This doesn't affect extension loading correctness (empty command still skips extensions, which is correct behavior for that edge case), but a test documenting this behavior would be valuable.

2. TOCTOU in file-level extraction check (src/infrastructure/runtime/embedded_deno_runtime.ts:74-85)

The version marker check followed by extraction has a theoretical race:

const existingVersion = (await Deno.readTextFile(versionMarker)).trim();
if (existingVersion === embeddedVersion.value) {
  await Deno.stat(targetBinary);
  return targetBinary;  // early return
}
// ... proceeds to extract

If two separate EmbeddedDenoRuntime instances (from the Medium issue above) both check simultaneously before either completes extraction, both could proceed to extract. This is harmless (identical content written twice) but wasteful. The promise-sharing fix within a single instance is correct; the issue only manifests with multiple instances.

Verdict

PASS — This is a well-implemented performance optimization. The parallel loading via Promise.all() is safe (separate directories and registries), the extraction promise sharing correctly prevents redundant subprocess spawns within an instance, and error handling is robust (all loaders catch errors internally). The Medium issue is a minor efficiency concern, not a correctness bug.