feat(extensions): W3 — ReconcileFromDisk + freshness-as-aggregate-query (swamp-club#252)

[stack72]

Summary

Implements W3 of the extension catalog rearchitecture — the third workstream after W1a/W1b/W2:

• ReconcileFromDiskService (src/libswamp/extensions/reconcile_from_disk_service.ts): new application service that walks on-disk source trees across all three origin types (locals, pulled, source-mounted), diffs against persisted aggregate state, and emits RowState transitions. Delegates to per-loader bundleAndIndexOne (not InstallExtensionService). Features: dryRun seam with structured ReconcileTransition return type, transition-count guardrail (>50% of rows → abort), cold-start trigger via anyKindNeedsInvalidation().

• enforceI2 deterministic-winner transform: replaces the IntraExtensionDuplicateType throw with a lexicographic-winner + tombstone-loser transform within the Extension aggregate. Cross-aggregate uniqueness (I-Repo-1) still throws DuplicateTypeError at the repository layer.

• Two-layer freshness model: type resolution is now a trivial aggregate query (isFresh = state.tag === "Indexed"). State maintenance is split between cold-start reconcile (ReconcileFromDisk) and warm-start incremental detection (findStaleFiles with fingerprint comparison, preserved for the hot-path development workflow).

• UNREADABLE_DEP_SENTINEL removal: renamed to UNREADABLE_PLACEHOLDER (internal to computeSourceFingerprint). Zero remaining references in production code.

Deliberate scope change from plan v3

findStaleFiles retains fingerprint comparison for the warm-start path. The plan's "~20 LOC shim" target was wrong — warm-start incremental detection is load-bearing (12 loader tests verified this). The architecture agent confirmed the revert is correct and permanent. The design doc documents the resulting two-layer model.

Regression tests

Three regression tests proving the rebundle-loop bug class is structurally fixed:

• feat: Open model type extensions with nested discovery #208: broken transitive dep → stable state, no rebundle loop
• fix: Provide default dataOutputSpecs for user models #209: schema-invalid extension → stable state, no rebundle loop
• refactor: Replace Ink/React output layer with LogTape structured logging #212: cached bundle missing → rebundles once, not in a loop

Pulled reconcile matrix

Three tests covering the pulled-extension reconcile paths:

• New source + lockfile entry → Indexed
• Orphan (no lockfile entry) → Tombstoned
• Missing source + lockfile present → EntryPointUnreadable

Performance benchmark

reconcile_from_disk_bench.ts: 50 local models cold-start = 1.2s, warm-start no-op = 7ms (Apple M2 Max). Pre-committed thresholds documented: ≤1.2x ship, 1.2-2x optimize, >2x redesign.

Note: The 1.2s cold-start number is the W3 baseline. A pre-W3 comparison measurement on main should be captured before merge to verify the ≤1.2x threshold.

Test plan

• 13 dedicated ReconcileFromDisk tests (7 generic contract + 3 regression + 3 pulled matrix)
• 3 new enforceI2 transform tests (two-way, three-way, idempotent)
• Updated 1 repository test for W3 transform behavior
• All 5542 tests pass (deno run test)
• deno check — clean
• deno lint — clean
• deno fmt — clean
• deno run compile — compiled
• UNREADABLE_DEP_SENTINEL grep — zero occurrences
• Author smoke: cold-start, warm-start, catalog-deleted recovery on scratch repo + parent repo with real extensions
• Pre-W3 baseline comparison (~20 min, before merge)
• Diversity-matrix soak: Linux + macOS, multiple repo shapes, 24-48h

🤖 Generated with Claude Code

[github-actions]

CLI UX Review

Blocking

1. logger.error + exit 0 on guardrail abort — reconcile_from_disk_service.ts:159-163 / cli/mod.ts:288

   When the 50% guardrail fires, the service logs at ERROR level:

   ERROR [swamp:extensions:reconcile] Reconcile aborted: 45 transitions out of 60 rows (75%) exceeds 50% guardrail
   

   …but mod.ts:288 discards the return value (await reconciler.execute() — result ignored), and the command continues to exit 0. This is the worst combination: the highest severity log level paired with a silent success exit.

   Existing logger.error calls in the codebase (push blocked, quality checks failed, update not installed) all correspond to operations that actually stopped. A user seeing an ERROR log at the start of a command will reasonably conclude the command failed — then be confused when it exits 0 and produces output.

   Suggested fix (pick one):

   • Downgrade to logger.warn with an actionable note: "Skipped catalog repair: too many rows would change (${n}/${total}). Run \swamp doctor extensions` to inspect."` This is the right level — the system is being cautious, not broken.
   • Or check result.applied in mod.ts and emit a visible warning through the normal output path when applied: false.

   The message also uses internal jargon ("50% guardrail", "rows") that means nothing to a user. Even at warn level this should say what the user should do.

Suggestions

1. "Reconcile complete: N transition(s) applied" is internal jargon — reconcile_from_disk_service.ts:171

   This logger.info message fires during every cold start. "Transition(s)" and "reconcile" are implementation concepts. Something like "Extension catalog updated: ${n} entr${n === 1 ? 'y' : 'ies'} repaired" would be more legible to a user who just ran their first command.

2. Successful reconcile during cold start shows up at INFO by default — reconcile_from_disk_service.ts:171

   Cold-start reconcile runs on first use and after catalog invalidation. The logger.info message will appear on stderr for most users during normal operation. Consider logger.debug for the no-changes paths and reserving logger.info for when transitions were actually applied (current behavior), but pairing it with the terminology fix above.

Verdict

NEEDS CHANGES — the guardrail abort path emits logger.error but exits 0, which is inconsistent with how every other logger.error call in the codebase behaves and will confuse users who see the error log but observe no command failure.

[github-actions]

Code Review

Blocking Issues

1. Import boundary violation in src/cli/mod.ts:80: ReconcileFromDiskService is imported from the internal path ../libswamp/extensions/reconcile_from_disk_service.ts. CLAUDE.md requires CLI commands to import libswamp types from src/libswamp/mod.ts — never from internal module paths. The type is already re-exported from src/libswamp/mod.ts (line 677), so the fix is:

   import { ReconcileFromDiskService } from "../libswamp/mod.ts";

Suggestions

1. Stale @throws doc on observeFreshSource (extension.ts:248): The JSDoc still says @throws IntraExtensionDuplicateType if I2 is violated, but I2 violations are now resolved by the deterministic-winner transform, not thrown. The doc should be updated to reflect the new behavior.

2. Unused _name/_version parameters in enforceI2 (extension.ts:464-465): These params are prefixed with _ to suppress unused-var warnings, but since enforceI2 is a private function (not implementing any interface), the parameters can be removed entirely along with the corresponding arguments at the three call sites. CLAUDE.md discourages the _ prefix pattern for genuinely unused code.

3. isFresh is exported but has no callers or tests (bundle_freshness.ts:240): The function is part of the W3 freshness model rewrite but isn't consumed by any code in this PR. Consider adding a minimal test to document the contract (particularly the undefined → true default) or deferring the export until a consumer exists.

4. IntraExtensionDuplicateType naming: The class is now documented as serving cross-aggregate violations ("retained for cross-aggregate violations surfaced by the repository"), but the name says "IntraExtension". No production code currently throws it. If it's forward-looking infrastructure for I-Repo-1, a rename to CrossExtensionDuplicateType (or similar) would reduce confusion — but this can be deferred.

5. kindDirToKind identity function (reconcile_from_disk_service.ts:621): This function returns its argument unchanged. Consider inlining the call or removing it if the type coercion isn't needed.

What looks good

• DDD alignment: ReconcileFromDiskService is correctly placed as an Application Service in the libswamp layer. Domain invariant enforcement (I2 transform) stays in the Extension aggregate. Repository additions are appropriate.
• Test coverage: 13 dedicated reconcile tests (7 generic + 3 regression + 3 pulled matrix) plus 3 enforceI2 transform tests provide strong coverage. Regression tests for #208/#209/#212 are well-structured.
• Transition-count guardrail: The >50% abort threshold is a good defensive measure against mass-tombstone bugs.
• dryRun seam: Clean structured return type enables future swamp doctor rendering without coupling.
• Design doc updates: The two-layer freshness model and reconcile matrix are well-documented.

[github-actions]

Adversarial Review

Critical / High

1. HIGH: reconcileAll produces duplicate @local/* extensions when .swamp-sources.yaml exists — causes data loss (reconcile_from_disk_service.ts:183-211)

   reconcileLocals (line 190) pushes a reconciled @local/foo into the reconcileAll result array. Then reconcileSourceMounted (line 204) runs and looks for @local/foo in its own local result array and in existingExtensions — but NOT in the reconcileAll result. On cold start (existingExtensions is empty), it creates a fresh @local/foo with only source-mounted sources. On warm start, it starts from the stale persisted snapshot.

   Phase 2 cross-tombstone bug: On warm start, the persisted @local/foo has both local AND source-mounted sources. reconcileLocals calls reconcileExtension with onDiskSources containing only extensions/<kind>/* files. Phase 2 (line 477) iterates all aggregate sources and tombstones anything not in onDiskSources — i.e., it tombstones the source-mounted sources. Meanwhile reconcileSourceMounted does the reverse: Phase 2 tombstones the local sources. saveAll receives two conflicting @local/foo extensions. The second one's diff overwrites the first, losing one set of sources.

   Breaking example: Repo has extensions/models/a.ts + .swamp-sources.yaml pointing at /other/models/b.ts. First cold-start reconcile: reconcileLocals produces @local/foo with a.ts indexed. reconcileSourceMounted produces another @local/foo with b.ts indexed (no a.ts). saveAll's applyDiffForExtension for the second extension deletes a.ts rows. a.ts model is lost from catalog.

   Suggested fix: Pass the already-reconciled local extension from reconcileLocals into reconcileSourceMounted (or accumulate into a shared mutable reference) so source-mounted reconciliation extends the same aggregate instead of creating a duplicate. Alternatively, gather ALL on-disk sources (local + source-mounted) into a single onDiskSources map before calling reconcileExtension once for the local aggregate.

Medium

2. MEDIUM: Guardrail creates a permanent livelock on every CLI invocation (reconcile_from_disk_service.ts:153-165)

   When the >50% guardrail triggers, the early return at line 163 skips markAllKindsPopulated(). Since anyKindNeedsInvalidation() remains true, the next CLI command re-triggers the full reconcile: disk walk, fingerprint computation, bundle attempts — only to hit the guardrail again. This repeats indefinitely, adding significant latency to every swamp command.

   Breaking example: User switches branches (or runs git clean), deleting 15 of 20 source files. First command: reconcile finds 15 tombstone transitions out of 20 rows (75%). Guardrail fires, doesn't save, doesn't mark populated. Every subsequent swamp command pays the full reconcile cost before doing anything useful. Only escape: manually delete _extension_catalog.db.

   Suggested fix: Either (a) call markAllKindsPopulated() even when the guardrail fires (so reconcile only runs once, and the user sees the error in logs without being stuck), or (b) persist a "guardrail tripped" flag that converts subsequent attempts into a no-op with a user-facing diagnostic message, or (c) provide an explicit escape hatch like swamp doctor extensions --force-reconcile.

3. MEDIUM: isFresh(undefined) defaults to fresh — inverts fail-safe (bundle_freshness.ts:240-242)

   export function isFresh(state: string | undefined): boolean {
     return (state ?? "Indexed") === "Indexed";
   }

   An undefined state (no catalog entry) returns true (fresh). If a future caller uses this for a source that was never indexed or whose catalog row was accidentally deleted, the source silently appears fresh and is never re-indexed. The fail-safe default should be false (not fresh → trigger re-indexing). Currently no code imports this function, but it's exported from the public API and from libswamp/mod.ts, so callers will arrive.

Low

4. LOW: Redundant OR clause (reconcile_from_disk_service.ts:439)

   if (fromState !== "Indexed" || fromState === null) — when fromState is null, null !== "Indexed" is already true, making || fromState === null dead code. Could be simplified to if (fromState !== "Indexed").

Verdict

FAIL — The duplicate local extension bug (#1) causes data loss for any repo using .swamp-sources.yaml with local extensions. The guardrail livelock (#2) permanently degrades CLI performance when it triggers. Both need fixes before merge.

[github-actions]

CLI UX Review

Blocking

None.

Suggestions

1. Silent cold-start stall — src/cli/mod.ts:292. When anyKindNeedsInvalidation() is true, the reconcile fires silently and can take ~1.2 s (50 models on M2 Max per the PR benchmark). There's no "Warming up extension catalog…" message before the work starts — users just see the CLI stall. A logger.info at the top of execute() before the disk walk would give users something to anchor to on first run.

2. Guardrail warning points to a command that can't show the transitions yet — reconcile_from_disk_service.ts (the logger.warn guardrail path). "Run swamp doctor extensions to inspect" implies the command will show which entries would have changed, but transition-list rendering is deferred to W6. A user who hits the guardrail and runs doctor extensions gets a registry pass/fail report, not the specific transitions. Softening the message to "Run swamp doctor extensions to check extension health" (or adding a parenthetical like "transition details available in W6") avoids the false implication.

3. Removed warm-path diagnostic — bundle_freshness.ts. The logger.warn for "Rebundling ${relativePath}: cached bundle missing from disk" is gone. That was a useful diagnostic when users were investigating why a rebundle fired. The new reconcile service covers the cold-start case; the warm-path (findStaleFiles) now silently adds the file to the stale list with no log. A logger.debug here would preserve the signal for users with verbose logging enabled without making it noisy for everyone else.

Verdict

PASS — no blocking issues. The PR is an internal service with minimal CLI surface: one silent cold-start hook and a handful of log messages. All new log messages are at appropriate levels and the only actionable one (swamp doctor extensions) refers to a real, existing command.

[github-actions]

Code Review

Well-structured PR that implements the W3 reconcile service, the enforceI2 deterministic-winner transform, and the two-layer freshness model. The code follows DDD principles correctly — ReconcileFromDiskService is properly placed as an application service in src/libswamp/, the Extension aggregate owns its I2 invariant enforcement, and the repository abstraction is respected at the CLI boundary (import via src/libswamp/mod.ts).

Test coverage is comprehensive: 13 dedicated reconcile tests (generic contract, regression, pulled matrix), 3 enforceI2 transform tests, updated repository test, and a performance benchmark.

Blocking Issues

None.

Suggestions

1. extractBundlePath type safety (reconcile_from_disk_service.ts:571-577): The function accepts a loose structural type { tag: string; bundle?: ... } and uses an as cast to return ReturnType<typeof makeBundleLocation>. Importing the RowState union and pattern matching on the tag would be safer than the structural check + cast.

2. Phase 2 path-matching fragility on Windows (reconcile_from_disk_service.ts:438): onDiskSources is keyed by raw absolutePath from collectTsFiles, but Phase 2 checks onDiskSources.has(loc.canonicalPath). On POSIX canonicalizePath is identity so these match, but on Windows it lowercases + replaces backslashes, so the keys would diverge. The multi-run tests are already skipped on Windows, but keying onDiskSources by canonicalizePath(absolutePath) would close this latent gap.

3. makeLoaderForKind instantiation (reconcile_from_disk_service.ts:366): Called inside the per-source loop, creating a new loader for every file. Creating one loader per kind outside the inner loop would avoid repeated construction (minor perf, not correctness).

4. isFresh parameter type (bundle_freshness.ts:238): Accepts string | undefined — using RowStateTag | undefined would catch misspellings at compile time. (May be intentional if callers pass raw catalog strings.)

5. findSourceByPath inline import (reconcile_from_disk_service.ts:554): Uses import("../../domain/extensions/source.ts").Source inline. A regular import at the top of the file would be more conventional.

6. markAllKindsPopulated reaches through to legacyStore (reconcile_from_disk_service.ts:535-548): Breaks the repository abstraction by accessing this.repository.legacyStore directly. Understood this is the legacy path documented for W4 removal — just flagging it for tracking.

[github-actions]

Adversarial Review

Critical / High

None found.

Medium

1. Windows path mismatch in Phase 2 of reconcileExtension (src/libswamp/extensions/reconcile_from_disk_service.ts:438)

   What's wrong: onDiskSources is populated with raw absolute paths from collectTsFiles as Map keys (lines 229-231, 244-245, 289-293). In Phase 2 (line 438), the code checks onDiskSources.has(loc.canonicalPath), but loc.canonicalPath is the output of canonicalizePath() — which lowercases and normalizes backslashes on Windows. On Windows, a raw path like C:\Users\foo\models\test.ts won't match the canonicalized key c:/users/foo/models/test.ts, causing every aggregate source to appear "missing from disk" and be falsely tombstoned or marked EntryPointUnreadable.

   Breaking example: On Windows, after Phase 1 indexes models\a.ts, Phase 2 iterates the updated ext.sources, finds the entry with canonicalized path c:/tmp/repo/extensions/models/a.ts, checks onDiskSources.has("c:/tmp/repo/extensions/models/a.ts") — but the Map key is C:\tmp\repo\extensions\models\a.ts. Miss. Source gets tombstoned.

   Mitigation: Multi-run reconcile tests are already ignore: Deno.build.os === "windows" and Windows isn't a merge gate per W-series precedent, so this doesn't block. However, the fix is straightforward — canonicalize the key when populating onDiskSources:

   onDiskSources.set(canonicalizePath(absolutePath), { kind: kindDir, baseDir: dir });

   or canonicalize in collectTsFiles. Worth fixing before W4 removes the Windows skip.

Low

1. Transition reporting inaccuracy under I2 collisions (src/libswamp/extensions/reconcile_from_disk_service.ts:384-407)

   When two on-disk sources declare the same (kind, type), Phase 1 processes both sequentially. The first is indexed normally. For the second, observeFreshSource adds it, then enforceI2 tombstones it as the loser. Then recordBundled un-tombstones it momentarily before enforceI2 (inside updateSourceState) tombstones it again. The catalog state is correct (loser is tombstoned), but the transition at line 399 records toState: "Indexed" for a source that's actually Tombstoned. No functional impact today — the CLI caller ignores transitions, and swamp doctor rendering (W6) hasn't shipped — but the phantom transition inflates the count that feeds the guardrail ratio.

2. collectTsFiles silently skips symlinked .ts files (src/libswamp/extensions/reconcile_from_disk_service.ts:619-622)

   entry.isFile is false for symlinks, so symlinked .ts files are skipped. entry.isDirectory is also false for symlinks, so symlinked subdirectories aren't traversed. Source-mounted extensions using symlinks would be invisible to reconcile. The warm-start path (findStaleFiles) may have the same limitation, so this is consistent behavior, but worth noting if symlink-based extension development is ever supported.

3. dryRun still executes bundleAndIndexOne side effects (src/libswamp/extensions/reconcile_from_disk_service.ts:369-374)

   dryRun only gates repository.saveAll(). The reconcileAll() call still invokes bundleAndIndexOne, which spawns deno bundle subprocesses and writes bundle files to disk. This is by design — you can't determine transitions without bundling — but a user expecting zero side effects from a dry run gets file system writes. The written bundles are orphaned until the next non-dry reconcile overwrites them.

Verdict

PASS — The core reconcile logic is sound. The two-layer freshness model, enforceI2 deterministic-winner transform, guardrail safety net, and cold-start trigger are all correctly implemented. The Windows path mismatch is a real bug but is mitigated by existing test skips and the W-series Windows policy. No data loss or corruption scenarios on the target POSIX platforms.