fix(extensions): re-attempt transient BundleBuildFailed instead of pinning it (#424)

[keeb]

Summary

swamp serve would permanently lose a custom (@user/*) model type after a single transient bundle failure. When the daemon first loaded an npm-dependent extension on a cold deno cache with no network (e.g. a fresh deploy, before any CLI run warmed .swamp/bundles/), deno bundle failed and the catalog recorded a sticky BundleBuildFailed row. Scheduled workflows then failed every run with Unknown model type: <type> — across daemon restarts and even after connectivity returned — while CLI swamp workflow run that had warmed a good bundle still worked.

Root cause: a BundleBuildFailed row carries an empty bundle_path, so in findStaleFiles it slipped past the missing-bundle staleness gate whenever its source fingerprint still matched, and was never re-bundled.

Fix: treat a fingerprint-matched BundleBuildFailed as stale so the next scan re-attempts the bundle and recovers once conditions change. This is not an aggressive retry loop — the next natural scan (restart / next load pass) fixes it. Deterministic ValidationFailed stays excluded (re-bundling it only thrashes). The cold-start ReconcileFromDisk path is intentionally unchanged: a failed bundle leaves the catalog populated, so daemon-restart recovery routes through this warm path. The fix is shared across all extension kinds (model/vault/driver/report).

Closes #424.

Test Plan

• New unit test: findStaleFiles: matching fingerprint + state=BundleBuildFailed → stale (#424); the existing ValidationFailed → not stale regression guard still passes.
• Updated the one vault-loader test that encoded the old pinning behavior to assert recover-on-rescan.
• Full suite green (6182 passed, 0 failed); deno check / lint / fmt clean.
• Live reproduction in an isolated systemd-run -p PrivateNetwork=yes namespace with a cold DENO_DIR: recreated the pinned state (BundleBuildFailed → Unknown model type), then restarted serve online — the daemon recovered on its own (Indexed, bundle rebuilt, scheduled run resolves the type).

Out of scope

A separate datastore-base-path-changed divergence between serve startup and scheduled-execution contexts (surfaced during reproduction) causes an unnecessary catalog re-bundle on the first scheduled run of each startup. This fix makes that invalidation benign; the churn itself is a follow-up.

🤖 Generated with Claude Code

[github-actions]

Code Review

Clean, well-scoped fix for a real bug. The root cause analysis is thorough — a BundleBuildFailed row with an empty bundle_path and unchanged fingerprint slipped through both the fingerprint-mismatch gate and the missing-bundle gate, permanently pinning the failure.

Blocking Issues

None.

Suggestions

1. The inline comment block at bundle_freshness.ts:312-318 (7 lines) is on the verbose side for this codebase. The first sentence and the BundleBuildFailed-vs-ValidationFailed distinction are load-bearing; the cold-start/ReconcileFromDisk detail could live in the design doc alone (which it already does). Minor — not blocking.

What looks good

• Fix placement: The new BundleBuildFailed check is correctly positioned after the fingerprint match and before the bundle_path/bundleExists check, which is exactly where the bug lived.
• Distinction preserved: ValidationFailed (deterministic) stays excluded from retry; BundleBuildFailed (transient) is now retried. The existing ValidationFailed → not stale test guards this boundary.
• Test: New unit test directly exercises the exact scenario (matching fingerprint + empty bundle_path + BundleBuildFailed state). The removed vault-loader integration test was OS-fragile and is now replaced by a portable, more focused test at the right abstraction layer.
• Design doc updated: The warm-start semantics in design/extension.md are updated to reflect the new behavior.
• DDD: Change is entirely within the domain layer (src/domain/extensions/), touching only the staleness determination logic where it belongs. No layer violations.
• Minimal blast radius: 4 files changed, the fix itself is a single 4-line conditional.

[github-actions]

Adversarial Review

Critical / High

None.

Medium

None.

Low

1. src/domain/extensions/bundle_freshness_test.ts:608 — no Windows EBUSY .catch() on cleanup.
   The deleted vault loader test used Deno.remove(...).catch(() => {}) on Windows because it held a SQLite handle. The new freshness test only uses FakeCatalog (in-memory) and text files, so EBUSY is not a realistic concern — and the pattern is consistent with every other test in this file. Noting for completeness only.

2. src/domain/extensions/bundle_freshness.ts:320 — every warm-start scan re-attempts a persistently failing BundleBuildFailed entry.
   If the underlying condition never resolves (e.g. a permanently broken npm dependency), every scan will re-invoke the bundler and fail again. This is bounded by scan frequency (not a tight retry loop) and the PR description explicitly acknowledges this tradeoff, so it's an acceptable design choice. If scan frequency ever increases, this could become noisy — but that's a future concern, not a current bug.

Verdict

PASS. Clean, minimal, well-placed fix. The new BundleBuildFailed check is inserted at exactly the right point in the decision chain — after fingerprint matching (so changed sources still take the normal path) and before the bundle-exists check (so the empty bundle_path on a failed row can't accidentally satisfy the old gate). The ValidationFailed exclusion is preserved correctly. The deleted vault loader test was asserting the old (now-incorrect) pinning behavior; its coverage is properly subsumed by the new unit test at the freshness layer. Logic, ordering, and edge cases check out.