Code Review Summary

This is a well-implemented performance optimization that defers self-contained bundle creation from startup time to first out-of-process execution (Docker). The change delivers a 16x speedup for everyday CLI commands while maintaining correctness.

No Blocking Issues

Code Quality & TypeScript Strict Mode:

• No any types introduced
• All exports are named exports (per CLAUDE.md requirements)
• Proper typing with bundleSourceFactory?: () => Promise<string>

Domain-Driven Design:

• bundleSourceFactory is appropriately placed on ModelDefinition as model metadata
• Factory invocation happens in MethodExecutionService (Domain Service) - correct orchestration boundary
• No persistence concerns leaked into the domain model

Implementation Quality:

• Promise-based memoization (bundlePromise ??=) correctly handles concurrent calls, avoiding duplicate bundling work
• Error handling properly resets bundlePromise = undefined on failure, allowing retries
• Logging preserved via logger.warn on bundle failure
• Bundle population moved to only non-raw drivers (method_execution_service.ts:582-586), avoiding unnecessary work for in-process execution

Security: No new security concerns - bundling logic unchanged, just deferred.

Test Coverage: PR description confirms 3175 tests pass. The existing comprehensive tests for user_model_loader_test.ts and method_execution_service_test.ts cover the behavioral contract, which hasn't changed - only the timing of when bundling occurs.

Suggestions (non-blocking)

1. Documentation update opportunity: design/execution-drivers.md:171 still references bundleSource rather than bundleSourceFactory. This is a minor documentation artifact that could be updated in a follow-up.

LGTM - clean implementation of a valuable performance optimization.

Adversarial Review

I systematically attempted to break this code across logic, error handling, concurrency, and API contract dimensions. The implementation is solid.

Critical / High

None found.

Medium

1. Interface change on public type (model.ts:606): ModelDefinition.bundleSource?: string was replaced with bundleSourceFactory?: () => Promise<string>. While ModelDefinition is exported, this field was only ever set by UserModelLoader internally and consumed by MethodExecutionService. External code directly inspecting bundleSource would now find it undefined. The risk is low since this was an implementation detail, but it's technically a breaking change to the interface shape.

Low

1. Model file deletion timing (user_model_loader.ts:357-369): The closure captures absolutePath at load time but bundling happens at first Docker execution. If a user deletes the model file between these events, bundling will fail. This is a minor behavioral change from eager bundling (which would have failed at load time instead). In practice this is rare and the error message is clear.

2. Non-null assertion in Docker driver (docker_execution_driver.ts:273): The driver uses request.bundle! without null check. If bundleSourceFactory is undefined (e.g., built-in models) and someone attempts Docker execution, they'll get a runtime crash. However, this is unchanged from the prior behavior—built-in models never had bundleSource set either. The error timing is just different now.

Verification of Memoization Pattern

The promise-based memoization is correct:

let bundlePromise: Promise<string> | undefined;
modelDef.bundleSourceFactory = () => {
  bundlePromise ??= bundleExtension(...).catch((error) => {
    bundlePromise = undefined;  // Allow retry on failure
    throw error;
  });
  return bundlePromise;
};

• Concurrent calls share the same promise (no duplicate bundling)
• On failure, bundlePromise resets to undefined, allowing retry
• Each model's closure has its own bundlePromise (loop creates new scope each iteration)

Error Semantics Change

Old: Bundle failure at load time → warning logged → bundleSource undefined → Docker execution later fails at driver

New: Bundle failure at execution time → error thrown immediately with clear message

This is better behavior—you get an actionable error message rather than a mysterious driver failure when bundle is missing.

Verdict

PASS — Clean implementation with correct memoization, proper error propagation, and no regressions. The performance improvement (46s → 2.8s startup) is significant with no observable downside for users.