feat(extensions): W7 — unify extension failure surfaces (swamp-club#342)

[stack72]

Summary

Removes the legacy registries.<kind>.failures[] dual failure surface and makes aggregateState.sourceDetails[] the single authoritative source of extension failure state in swamp doctor extensions. Closes the W1-W6 rearchitecture story — W7 removes the last vestige of the old failure-reporting model.

What changed

• Shared failure recorder (src/domain/extensions/source_failure_recorder.ts) — extracted from reconcile's catch block. Both reconcile and buildIndex call the same helper, eliminating the consistency gap where buildIndex bypassed the aggregate.
• buildIndex failure routing — warm-start catch routes failures through recordSourceFailure → repository.saveAll(). Cold-start failures also routed, with originalError preserving ValidationError type for correct state-tag discrimination.
• markCatalogValidationFailed removed — validation failures now throw ValidationError, caught by the shared helper. No more direct catalog upserts that bypass the aggregate.
• registries.failures[] removed — per-registry status derived from sourceDetails[] failure-state tags (ValidationFailed, BundleBuildFailed, EntryPointUnreadable).
• extension_load_warnings.ts split — kept emitTypeExtractionFailure for stderr regex-mismatch warnings; removed recordLoadFailures, getExtensionLoadWarnings, and the warnings array accumulation.
• last_error catalog column — schema migration adds last_error TEXT to bundle_types. Error messages persist across process invocations. Clears on recovery to Indexed.
• SQL state-preservation fix — upsert ON CONFLICT UPDATE now conditionally excludes state and last_error when callers don't provide them, preventing populateCatalogFromDir from overwriting reconcile's failure states with Indexed.

JSON schema changes (doctor extensions --json)

1. Removed: registries.<kind>.failures[] — the field no longer exists on DoctorRegistryResult
2. Added: sourceDetails[].lastError?: string — populated for failure states (ValidationFailed, BundleBuildFailed, EntryPointUnreadable), absent for healthy states

Migration note

Pre-migration catalog rows in failure states get empty lastError until the next reconcile pass. Self-heals on first run after upgrade — not a bug, just a transient state.

Known behavior changes (follow-ups filed)

• Type-extraction warnings (fix: Defer env expressions to runtime, add globalArguments validation #351): models with non-literal type fields (e.g. const TYPE = "...") produce stderr warnings but no longer flip doctor exit code. They don't register and don't appear in sourceDetails[]. Previously surfaced via the removed registries.failures[] pipeline.
• Streaming protocol (fix: GC scanning now handles nested/namespaced model types #352): kind-completed events carry preliminary status: "pass" during the scan phase; final status is computed from aggregate state in the completed event. Internal-only (renderer is the sole consumer).

Test plan

• 5908 unit/integration tests pass (0 failures)
• deno check / deno lint / deno fmt clean
• 24-point binary verification matrix against compiled ./swamp:
  • Healthy model → Indexed, exit 0
  • Schema-invalid model → failure state, exit 1, lastError populated
  • Missing version → ValidationFailed, exit 1
  • Recovery: broken → fixed → Indexed, lastError clears
  • Rebundle loop guard: stable broken source converges (3 runs)
  • Mixed healthy + broken: healthy registers, broken surfaces
  • JSON contract: no failures field, lastError present
  • Vault failure surfaces in sourceDetails with correct registry status
  • lastError persists across separate process invocations
  • lastError absent from sourceDetails after recovery to Indexed
• SQL regression tests: upsert without state preserves both state and last_error; upsert with explicit state overwrites both
• Integration test: lastError lifecycle (populates on failure, clears on recovery)

🤖 Generated with Claude Code

[github-actions]

Code Review

Well-structured PR that successfully consolidates extension failure surfaces into a single authoritative source (sourceDetails[]). The shared recordSourceFailure helper eliminates the consistency gap between reconcile and buildIndex, and the SQL state-preservation fix is a clean solution.

Blocking Issues

1. Missing unit tests for source_failure_recorder.ts — This is a new 208-line domain file with non-trivial branching logic (ValidationError vs. other error types, existing vs. new sources, state-transition detection, kindDirToExtensionKind/extensionKindToKindDir mapping). CLAUDE.md requires "Comprehensive unit test coverage" with "Unit tests live next to source files: foo.ts → foo_test.ts". No source_failure_recorder_test.ts exists. The recordSourceFailure function alone has 4+ distinct code paths that should be directly tested.

Suggestions

1. Dead _quiet parameter (src/cli/mod.ts) — The quiet parameter in loadUserModels, loadUserVaults, loadUserDrivers, loadUserDatastores, and loadUserReports is now unused (renamed to _quiet). Callers at lines 316-359 still pass the value. Could clean up the parameter from both the function signatures and their call sites in a follow-up.

2. kind-completed always emits status: "pass" (src/libswamp/extensions/doctor.ts) — This is noted in the PR body as a known behavior change, but worth calling out: the kind-completed events are now semantically misleading until the completed event fires. Since the renderer is the sole consumer and only uses the final completed report, this is acceptable — but if other consumers are added, the preliminary status could cause confusion. Consider naming it preliminary or omitting the field from kind-completed events entirely.

3. DDD layer violation tracked — source_failure_recorder.ts (domain) imports ExtensionKind from extension_catalog_store.ts (infrastructure). The ratchet bump from 24→25 is correct. A follow-up to lift ExtensionKind into the domain layer would bring this back down.

[github-actions]

CLI UX Review

Blocking

1. Per-registry scan icons always show ✓ even for failing registries

File: src/presentation/renderers/doctor_extensions.ts:233–235 and src/libswamp/extensions/doctor.ts:325–328

The kind-completed event is now always yielded with status: "pass" (hardcoded), because aggregateState isn't built until after all registries finish loading. The log renderer uses iconFor(r.status) from this event to render the per-registry row, so every registry prints ✓ registry-name regardless of whether it will ultimately fail.

The user sees this sequence:

✓ model        ← misleading: model will fail
✓ vault
✓ driver
✓ datastore
✓ report

    • /path/bad.ts ValidationFailed: missing version

1 passed, 4 failed — OVERALL: FAIL   ← contradicts all the ✓ above

The summary count (1 passed, 4 failed) correctly uses the final report.registries statuses, but the per-registry rows — the most prominent output during the scan phase — all show green checkmarks. A user reading the scan output sees nothing wrong, then gets a FAIL summary with no clear mapping of which registries failed.

The PR description acknowledges this is an intentional architectural decision (follow-up #352), but the renderer hasn't adapted to it. A simple fix: don't render an icon at kind-completed time at all (just • registry-name as an in-progress indicator), and re-render each row with its final ✓/✗ icon inside the completed handler using e.report.registries[name].status.

---

2. Loader errors silently dropped from log-mode output

File: src/libswamp/extensions/doctor.ts:318–323 and src/presentation/renderers/doctor_extensions.ts:237–259

When ensureLoaded() throws, the error message is stored in loaderErrors but never put into the DoctorExtensionsReport. The completed log renderer only shows failure bullets from aggregateState.sourceDetails (catalog-driven). Since a loader crash may not have touched the catalog at all, sourceDetails will have zero failure entries for that registry.

Result: registry shows ✓ model during scan, shows no failure details at completion, and the user only learns the registry failed from the N failed summary count — with no error message explaining why.

The old code created a synthetic <model loader>: error message bullet for exactly this scenario. The new code doesn't surface the loader error in log output at all.

Fix: either include loaderErrors in the DoctorExtensionsReport so the renderer can surface them, or render them before the completed yield.

Suggestions

1. The failure bullets in the completed handler appear without a header (just a blank line then bullets). Previously they appeared under the failing registry row. Consider adding a small label like Failures: or using the same ⚠ warning style used for orphan files so users can orient themselves in the output.

2. In the completed handler, the failure bullets show sourcePath stateTag: lastError but don't indicate which registry the source belongs to. When a user has models and vaults both failing, it may not be obvious which registry owns a given source path. The kind field is already present on DoctorSourceDetail — including it in the bullet (model /path/bad.ts ValidationFailed: …) would help.

Verdict

NEEDS CHANGES — the per-registry scan icons always show ✓ even for failing registries, creating a direct visual contradiction with the summary line; and loader-level errors are silently dropped from log-mode output, a regression from the previous behavior.

[github-actions]

Adversarial Review

Critical

1. dropValidationFailedColumn() recreate-table omits last_error column — guaranteed crash on upgrade from pre-W1b databases

   File: src/infrastructure/persistence/extension_catalog_store.ts:405-431

   The PR adds last_error TEXT NOT NULL DEFAULT '' to createSchema() (line 210), addNewColumnsIfMissing() (lines 304-308), upsert(), upsertWithIdentity(), and mapRow(). However, dropValidationFailedColumn() (lines 405-431) was not updated — its CREATE TABLE bundle_types_new and INSERT INTO...SELECT FROM statements do not include last_error.

   Breaking scenario: A user with a pre-W1b database (has validation_failed column, no drop-migration marker) upgrades to a version containing this PR:

   1. addNewColumnsIfMissing() adds last_error via ALTER TABLE.
   2. dropValidationFailedColumn() recreates the table without last_error.
   3. The last_error column is silently dropped from the table.
   4. Any subsequent upsert() or upsertWithIdentity() call executes INSERT INTO bundle_types (..., last_error) — SQLite throws "table bundle_types has no column named last_error".
   5. The process crashes.

   Self-heals on restart (addNewColumnsIfMissing() re-adds the column, drop-migration marker is already set so it skips), but the first run after upgrade is a hard crash.

   Fix: Add last_error TEXT NOT NULL DEFAULT '' to the bundle_types_new CREATE TABLE in dropValidationFailedColumn(), and add last_error to both the INSERT INTO and SELECT column lists. Add a test that exercises the happy path of dropValidationFailedColumn on a table that already has a last_error column and verifies the column survives.

Medium

2. Cold-start buildIndex failure routing resolves paths against wrong base directory

   File: src/domain/extensions/extension_loader.ts:396

   In the cold-start failure routing (lines 383-438), resolve(dir, failure.file) uses dir (the primary directory) to reconstruct the absolute path. But failure.file is relative to the baseDir it was discovered in — which could be one of additionalDirs rather than dir.

   Breaking scenario: When additionalDirs is passed to buildIndex, and a file from an additional directory fails validation, the reconstructed absolutePath points to a non-existent location under dir. makeSourceLocation computes a wrong canonical path, coldIndex.get() misses, and the failure silently goes unrecorded in the aggregate state.

   Fix: Either propagate baseDir in the failure record from load(), or skip aggregate-state recording when the source can't be located in the index (as a defensive fallback).

3. kind-completed events always emit status: "pass" even when the loader threw

   File: src/libswamp/extensions/doctor.ts:325-328

   The yield at line 326 unconditionally emits status: "pass". When ensureLoaded() throws (caught at line 318), the error is stashed in loaderErrors but the event still says pass. Any streaming consumer that renders per-registry status from kind-completed events will show green checkmarks for failing registries.

   The PR description acknowledges this as a known behavior change (#352). The log renderer happens to work because it re-computes final status in the completed handler. But the JSON streaming protocol's per-event contract is violated — a kind-completed with status: "pass" is an affirmative claim that the registry passed, not a "pending" signal.

Low

4. Repeated same-state failures suppress transition events

   File: src/domain/extensions/source_failure_recorder.ts:123

   When a source already in ValidationFailed fails again (e.g., with a different error message), fromState === toState evaluates true and no transition is emitted. The updated lastError is persisted but the transition is invisible to swamp doctor extensions output. This means a changed error message (e.g., a different validation error after editing) produces no visible diagnostic output.

5. Cold-start makeLocalExtension uses adapter kind as basename

   File: src/domain/extensions/extension_loader.ts:422-425

   When a cold-start failure can't be matched to an existing extension, a new extension is created with basename: this.adapter.kind (e.g., "model"), producing @local/model instead of @local/<repo-name>. This is a minor identity inconsistency that the reconcile pass would correct, but the transient state could confuse diagnostic output.

Verdict

FAIL — Finding #1 is a guaranteed crash on the upgrade path from pre-W1b databases. The dropValidationFailedColumn() recreate-table must include the last_error column.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. --verbose per-source detail omits lastError (src/presentation/renderers/doctor_extensions.ts:133-146)
   In renderAggregateStateLog, the "Per-Source Detail" rows render sourcePath + stateTag + fingerprint + bundlePath but skip lastError. The failure bullets in the completed handler above DO surface it, so users aren't totally blind — but someone reaching for --verbose to dig into a broken source won't see the error message inline next to the path. Consider appending lastError to the per-source rows the same way the failure bullets do: : ${detail.lastError}.

2. loaderErrors is absent-when-empty; its peers are always-present (src/presentation/renderers/doctor_extensions.ts:388-390)
   orphanFiles and recentTransitions are always in the JSON output (empty array when nothing to report). loaderErrors is conditionally omitted when empty. Scripts have to guard with if (output.loaderErrors) rather than iterating directly, which is a subtle inconsistency. Emitting {} unconditionally (or {} only when aggregateState is present) would match the shape of the other always-present fields.

3. Breaking JSON schema change communicated only in the PR body (registries.<kind>.failures[] removed)
   Documented in the PR summary, but nothing in-band warns existing scripts. Scripts that key on result.failures.length > 0 (rather than result.status === "fail") will silently stop detecting failures — undefined.length in JS throws; null | length in jq returns 0. result.status still works, so the impact is narrow, but a note in the release changelog or a prominent migration note would help operators auditing their CI pipelines.

Verdict

PASS — the core failure-surface unification is solid, error messages are actionable, both output modes are consistent, and exit codes are correct. The three items above are quality-of-life tweaks that don't block merge.

[github-actions]

Code Review

Well-structured refactoring that unifies extension failure surfaces through a single sourceDetails[]-based model, eliminating the dual registries.<kind>.failures[] reporting path. The extraction of source_failure_recorder.ts as a shared domain service is clean DDD — both reconcile and buildIndex now route failures through the same pure function.

Blocking Issues

None found.

Suggestions

1. _quiet parameter cleanup (src/cli/mod.ts): Five loadUser* functions now prefix the quiet parameter with _ since recordLoadFailures was removed. This is correct for lint suppression, but if the parameter is truly dead (no consumer inside the function body), consider removing it from both the function signatures and call sites in a follow-up to reduce confusion.

2. kind-completed always emits "pass" (src/libswamp/extensions/doctor.ts:332): The kind-completed event now always carries status: "pass" during the scan phase — final status is only determined in the completed event. The PR description documents this as a known behavior change and the log renderer handles it correctly (just shows registry name during scan, re-renders with correct icons at completion). Worth noting that any external kind-completed consumer would see misleading status — the JSDoc on the event type could mention this caveat.

3. loadAllWithPruning() is only called in tests (src/infrastructure/persistence/extension_repository.ts:171): The new loadAllWithPruning() method distinguishes read-only vs. side-effecting loads, which is a good semantic improvement. Currently only tests exercise it directly. If the doctor aggregate builder or other paths should eventually use it, that's fine as a follow-up.

4. Duplicate failure-routing logic in extension_loader.ts: The warm-start path (lines ~336-369) and cold-start path (lines ~391-444) both build sourcePathIndex/coldIndex maps and call recordSourceFailure with nearly identical setup (fingerprint computation, stat, error wrapping). If these patterns diverge it's fine, but if they stay identical, extracting a small helper could reduce the surface area for inconsistencies.

[github-actions]

Adversarial Review

Critical / High

1. HIGH — Cold-start multi-failure data loss silently overwrites earlier failure states (src/domain/extensions/extension_loader.ts:391-444)

   In the cold-start branch of buildIndex, coldExtensions is loaded once (line 392) and never updated within the failure-recording loop. When two failures target the same extension, the second iteration reads the original extension from coldExtensions[extIdx] (line 406), builds a new modified extension from that stale base, and calls repository.saveAll() — which overwrites the first failure's persisted state via applyDiffForExtension.

   Concrete breaking scenario: A repo with extensions/models/a.ts and extensions/models/b.ts, both belonging to @local/myrepo. On first run (cold-start), both fail validation.

   • Iteration 1: coldExtensions[0] has sources A(Indexed) + B(Indexed). recordSourceFailure produces A(ValidationFailed) + B(Indexed). saveAll writes this.
   • Iteration 2: coldExtensions[0] is still A(Indexed) + B(Indexed) — the in-memory array was never updated. recordSourceFailure produces A(Indexed) + B(BundleBuildFailed). saveAll writes this, overwriting A's ValidationFailed back to Indexed.

   The warm-start path (line 365) correctly does extensions[extIdx] = failResult.extension; — this line is missing from the cold-start path.

   Similarly, when ext is undefined (no existing catalog extension), the fallback creates a throwaway makeLocalExtension({ basename: this.adapter.kind }) on each iteration. Each creates @local/model (or @local/vault, etc.) with only one failure source. The second saveAll call's applyDiffForExtension sees the first failure's row under @local/model, finds it's not in the new extension's sources, and deletes it.

   Suggested fix: Add coldExtensions[extIdx] = failResult.extension; after line 443, and for the ext === undefined case, accumulate failures per-extension before a single saveAll.

Medium

1. MEDIUM — kind-completed events always emit status: "pass" during scan (src/libswamp/extensions/doctor.ts:330-333)

   Every kind-completed event unconditionally reports status: "pass". The real pass/fail is only computed at the completed event (line 360-366). The PR documents this as a known behavior change (#352) and notes the renderer is the sole consumer — the renderer correctly ignores the preliminary status and re-renders from e.report.registries. However, any third-party streaming consumer watching kind-completed events (e.g., a CI integration or custom TUI) would receive wrong intermediate results.

2. MEDIUM — loadAllWithPruning() is dead code in production (src/infrastructure/persistence/extension_repository.ts:171)

   loadAllWithPruning() is defined, tested, and the test was modified in this PR — but it has zero production callers. All production loadAll() calls pass pruneOrphans: false, meaning orphan rows (pulled extensions removed from the lockfile) are never cleaned up except via the reconcile service's explicit tombstoning. If loadAllWithPruning was introduced to close a pruning gap, it needs a production call site. If it's infrastructure for a future feature, that's fine but the test change in this PR may give a false impression of coverage.

Low

1. LOW — Cold-start fallback extension name mismatch (src/domain/extensions/extension_loader.ts:429-432)

   When ext is undefined in the cold-start failure path, makeLocalExtension({ basename: this.adapter.kind }) creates an extension named @local/model (or @local/vault, etc.). The reconcile service creates the "real" local extension as @local/<repo-directory-name>. These never collide (different names), but the cold-start-created @local/model rows become permanent orphans since no subsequent code path adopts or tombstones them. The next reconcile pass creates the proper @local/<repo> extension independently.

Verdict

FAIL — The cold-start multi-failure data loss (finding #1) silently drops failure states when multiple sources in the same extension fail during the first run. The warm-start path has the fix pattern already (line 365) — it just needs to be applied to the cold-start path as well.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Verbose mode: failed sources appear twice. In --verbose mode, a failed source is shown in the failure-bullet block (lines 262–281 of src/presentation/renderers/doctor_extensions.ts, rendered immediately after the registry status list) and again in the "Per-Source Detail" section inside renderAggregateStateLog. Both entries include lastError, so the information is redundant. One option: skip failure-state entries from Per-Source Detail in verbose mode; another is to drop the pre-aggregate failure bullets when verbose is on (the Per-Source Detail is more complete anyway). Minor, but worth a follow-up.

2. Two-pass registry listing in log mode. The new rendering emits each registry name twice: first as • model (dim, during scan) and then as ✓/✗ model (bold, at completion). The code comment explains why (loader errors are only available in the completed event), so the approach is architecturally sound. For slow scans the dim-bullet phase provides useful progress feedback; for fast scans both sets of lines appear in rapid succession and the double-listing can look like noise. Consider suppressing the scan-phase bullets when stdout is not a TTY (piped/CI output), mirroring the pattern other swamp commands use for progress indicators.

3. Breaking JSON schema change — no in-band notice. registries.<kind>.failures[] is removed from the --json output; scripts using parsed.registries.model.failures will silently get undefined. The PR description documents the migration path (aggregateState.sourceDetails[] filtered by failure state tags, plus the new loaderErrors object), but CLI consumers won't see that context. A one-time deprecation cycle (emit the field as [] for a release, add a _deprecated note) or a mention in the command's --help examples would reduce surprise for existing automation. Not blocking given the explicit schema-change notice in the PR, but worth filing a follow-up.

Verdict

PASS — no blocking issues. The unified failure surface is a genuine UX improvement: lastError in failure bullets gives users actionable context without having to dig into logs, and the single sourceDetails[] source of truth is easier to script against than the dual-surface the prior design required.

[github-actions]

Code Review

Clean, well-structured PR that successfully unifies the extension failure surface from dual registries.failures[] + sourceDetails[] into a single source of truth. The source_failure_recorder.ts extraction is well-designed, tests are thorough (374-line unit test file, integration lifecycle test for lastError), and the SQL migration is backward-compatible.

Blocking Issues

None.

Suggestions

1. Stale docstring in extension_catalog_store.ts:660-666 — the upsert JSDoc still references markCatalogValidationFailed (removed in this PR) and describes state as always being in the SET clause. The behavior is now conditional (state is only in the SET list when row.state !== undefined), which is correctly documented in the ExtensionTypeRow.state JSDoc (lines 99-108) but not in the upsert method's comment.

2. loadAllWithPruning() is defined and tested but never called in production — loadAll() was changed from pruning to read-only, and loadAllWithPruning() was added as the explicit cleanup path. However, no production code calls it yet, so orphan rows that were previously cleaned up on every loadAll() will now accumulate. Consider wiring it into the reconcile path or filing a follow-up.

3. Dead _quiet parameters in src/cli/mod.ts — five loadUser* functions now have _quiet = false that's never read after removing recordLoadFailures. The _ prefix satisfies the linter, but the parameter itself is dead code. Minor cleanup for a follow-up.

Overall: well-executed. The domain model is clean, the aggregate-state-driven registry status derivation is more principled than the old warning-partition approach, and the last_error persistence across process restarts is a solid UX improvement.

[github-actions]

Adversarial Review

Critical / High

No critical or high-severity findings.

Medium

1. Warm-start saveAll inside catch block is not try-caught — src/domain/extensions/extension_loader.ts:366

   In the warm-start path, repository.saveAll([failResult.extension]) is called inside the catch block of the stale-files loop (line 366). This call is not wrapped in its own try-catch. If it throws (SQLite lock timeout exhausting busy_timeout, DuplicateTypeError from a concurrent process modifying the same catalog, or disk-full), the error propagates out of the for-loop's catch block, out of buildIndex, and to the caller. The function never returns its result object, even though types from successful earlier iterations are already registered in-memory via this.adapter.register. The caller receives a confusing SQLite or constraint error instead of the expected ExtensionLoadResult.

   Breaking scenario: Two swamp processes running simultaneously. Process A's warm-start loop hits a failure, calls saveAll, and the SQLite busy_timeout expires because process B holds a long write lock. buildIndex throws; the CLI command fails with a raw SQLite error.

   Compare: The cold-start handler (lines 452-459) correctly accumulates failures into coldExtensions/localFallback and does a single saveAll after the loop. The warm-start should either (a) wrap the saveAll in try-catch with a warning log, or (b) accumulate and save once at the end like the cold-start does.

   Suggested fix: Wrap lines 339-367 in an inner try-catch that logs a warning and continues:

   try {
     // ... the existing failure-recording + saveAll logic
   } catch (persistError) {
     this.logger.warn`Failed to persist failure state for ${relativePath}: ${persistError}`;
   }

2. Per-failure saveAll in warm-start is O(N) transactions — src/domain/extensions/extension_loader.ts:366

   Each failed stale file triggers its own saveAll, which opens a SQLite transaction, computes the diff, runs I-Repo-1 over the entire catalog, and commits. With N failures, this is N full-catalog scans + N transactions. The cold-start path (lines 452-459) batches into a single saveAll. The warm-start should batch similarly.

   Breaking scenario: A repo with 50 stale files that all fail (e.g., after a Deno version bump that breaks bundling). Each one triggers a full-catalog I-Repo-1 scan. On a catalog with 1000+ rows, this multiplies startup latency significantly.

Low

1. _quiet parameters are dead but still accepted — src/cli/mod.ts:454,538,601,663

   The quiet parameter is renamed to _quiet (unused) in loadUserModels, loadUserVaults, loadUserDrivers, loadUserDatastores. Callers still pass quiet: true (verified in the CLI bootstrap), but the value has no effect since recordLoadFailures was removed. Warnings now go through LogTape only. Not a regression — LogTape has its own verbosity controls — but the dead parameter is confusing for future maintainers. Consider removing the parameter from the signature and callers in a follow-up.

2. kind-completed event status may differ from final status — src/libswamp/extensions/doctor.ts:330-338

   During the scan phase, kind-completed events carry a status derived only from loaderErrors, not from registryHasFailures (which requires the aggregate state built later). A registry where ensureLoaded() succeeds but sourceDetails contains failure-state entries gets status: "pass" in the stream event but status: "fail" in the final completed event. The log renderer ignores the stream-event status (renders a plain bullet), so users don't see the inconsistency. But external stream consumers (e.g., a future TUI or CI integration reading events) would see a misleading interim status. This is documented as known behavior (#352).

3. removeBySourcePath / removeBySourcePrefix do not canonicalize input paths — src/infrastructure/persistence/extension_catalog_store.ts:724-742

   findBySourcePath canonicalizes its input (line 715), but removeBySourcePath and removeBySourcePrefix do not. The PR's fifth commit deliberately scoped canonicalization to findBySourcePath only, noting that remove callers always pass already-canonical paths. This is correct today — applyDiffForExtension passes source.id.canonicalPath, and deleteBySourcePaths receives canonical paths from DoctorCatalogOrphan. But the asymmetric contract is fragile: a future caller passing a raw Windows path to removeBySourcePath would silently delete nothing. Pre-existing design issue, not introduced by this PR.

Verdict

PASS — The changes are well-structured: failure recording is extracted into a shared pure function with good test coverage, the SQL conditional-update fix correctly preserves failure states, and the cold-start multi-failure fix accumulates correctly. The warm-start per-failure saveAll without try-catch is a robustness gap worth addressing, but it's a non-common failure mode (SQLite lock contention) and doesn't block the merge. The lastError lifecycle (populate on failure, clear on recovery) is correct end-to-end. The registryHasFailures derivation from sourceDetails is a clean replacement for the removed failures[] field.