Code Review

Clean, well-tested fix. The transitive dependency detection and error classification are sound, and the changes are consistent with existing patterns.

Blocking Issues

None.

Suggestions

1. Duplicated bundleWithCache error-handling across 4 loaders — The isExpectedBundleFailure + log-level branching block is now copy-pasted identically in user_model_loader.ts, user_datastore_loader.ts, user_driver_loader.ts, and user_vault_loader.ts. Consider extracting a shared helper (e.g., handleBundleFallback(bundlePath, absolutePath, relativePath, repoDir, error)) in bundle.ts to reduce the surface area for future divergence. Not blocking since the duplication is pre-existing and this PR just modified the existing pattern consistently.

2. isExpectedBundleFailure uses sync I/O — Deno.statSync and Deno.readTextFileSync are called from async bundleWithCache contexts. This is consistent with findNearestDenoConfig which does the same, so not a concern for this PR, but an async version would avoid blocking the event loop on slow filesystems.

3. Transitive dep checking is model-only — The findStaleFiles transitive dependency detection was added to UserModelLoader but not to the datastore, driver, or vault loaders (they already import resolveLocalImports). If those loaders also have catalog-based incremental rebuild, they could benefit from the same check. If they don't, this is fine as-is.

Adversarial Review

Critical / High

None found.

Medium

1. Off-by-one in isExpectedBundleFailure directory traversal — src/domain/models/bundle.ts:220-221

   The repoDir boundary check (break) executes before the deno.json search for that directory:

   while (dir !== root) {
     if (repoDir && resolve(dir) === resolve(repoDir)) break; // breaks BEFORE checking repoDir
     for (const name of ["deno.json", "deno.jsonc"]) { ... }

   If deno.json exists at the repo root (a common layout), the traversal stops without finding it. The function then checks sourceHasBareSpecifiers() and may return true (expected failure), when the failure is actually unexpected because the project config does exist.

   Breaking example: Extension at <repoDir>/extensions/myext/model.ts imports "zod" (bare specifier). deno.json at <repoDir>/deno.json maps "zod" to npm:zod@4. Bundling fails for some other reason (network, syntax error). isExpectedBundleFailure walks up to <repoDir>, hits the break, never sees deno.json, returns true. The cache mtime is touched, suppressing retries on subsequent runs — the user never sees the warning that would have helped them debug.

   Fix: Swap the order — check for deno.json before the boundary break:

   while (dir !== root) {
     for (const name of ["deno.json", "deno.jsonc"]) { ... }
     if (repoDir && resolve(dir) === resolve(repoDir)) break;
     // ...
   }

2. resolveLocalImports includes the entry point in resolvedFiles — src/domain/models/user_model_loader.ts:898-916

   resolveLocalImports([absolutePath], dir) returns the entry point and its transitive deps in resolvedFiles. The entry point's mtime is then included in the newestDepMtime calculation and compared against bundleStat.mtime. This is functionally harmless (entry point was already verified fresh against the catalog, and its mtime should be older than the bundle's), but it adds unnecessary stat calls and makes the intent less clear. Consider filtering the entry point out of resolvedFiles before the dep check, or just documenting that it's intentionally included.

Low

1. Sync I/O in async context — src/domain/models/bundle.ts:223,237

   isExpectedBundleFailure uses Deno.statSync and Deno.readTextFileSync. It's called from the async bundleWithCache error path. Since this is a cold error-handling path (bundling already failed), the blocking impact is negligible in practice. Noting for awareness only.

2. Identical error-handling blocks across 4 loaders — user_model_loader.ts, user_datastore_loader.ts, user_driver_loader.ts, user_vault_loader.ts

   The isExpectedBundleFailure conditional block is copy-pasted across all 4 loader files. A future change to the error-handling logic would need to be applied in 4 places. Not a correctness issue, but increases the maintenance surface. The PR author clearly recognizes this by extracting the shared isExpectedBundleFailure helper, so this is just the remaining duplication.

Verdict

PASS — The logic is sound and well-tested. The off-by-one in the directory traversal boundary (Medium #1) is a real bug that should be addressed, but it only affects failure classification (not data correctness or crashes), so it doesn't block the merge. The transitive dependency detection in findStaleFiles and the expected-vs-unexpected failure distinction in bundleWithCache are both correctly implemented.