refactor(extensions): replace allExtensionMethodsAttached with fingerprint-aware tracking set (swamp-club#318)

[stack72]

Summary

• Replaced the expensive allExtensionMethodsAttached idempotency guard (which re-imported extension bundles and re-parsed schemas on every call) with a cheap fingerprint-aware Map<type, Map<sourcePath, fingerprint>> tracking set — O(1) lookup instead of O(import+parse)
• Fixed a pre-existing gap where buildIndex didn't trigger extension attachment for new extension files unless the base model file also changed — rebundleAndUpdateCatalog now returns extensionTarget for stale secondary exports
• importAndExtendBundle treats "already exists on model type" as an idempotent no-op, covering the load() → attachPendingExtensionsForType cross-path in hotLoadModels
• Added 4 regression tests: Issue 8: Update CLI commands for new architecture #123 cross-path idempotency, hotLoadModels load-then-attach scenario, extension A/B selective attachment, and resetLoadedFlag invariant (ADV-2)

Audit findings (from triage)

Six model registration paths were audited. Full path consolidation was ruled out — extensions and base models are discovered from different sources (local, pulled, catalog), making attachment inherently a separate step. The tracking set approach delivers the same end state (no expensive guard, idempotent attachment) via a structurally honest path.

Adversarial review dispositions

• ADV-1 (low/scope): Step 1 analysis folded into PR description — no separate implementation step
• ADV-2 (medium/risk): resetLoadedFlag stale-tracking — tested empirically, not just documented. resetLoadedFlag doesn't un-attach extensions, so the tracker's claim remains correct
• ADV-3 (low/complexity): Used Map<string, Map<string, string>> instead of null-byte separator — more readable and greppable
• ADV-4 (low/correctness): eagerlyRegisteredTypes narrowing — superseded by the extensionTarget approach which is strictly better
• ADV-5 (low/documentation): No design docs or skills reference the guard — no updates needed
• ADV-6 (low/testing): Purely internal change — no UAT or docs gaps

Test Plan

• All 5821 unit + integration tests pass (1 pre-existing vault loader timing flake)
• E2E verified with compiled binary: base model + extension A loaded, extension B hot-attached without base model change, both methods execute, idempotent on repeat access
• Regression test: Issue 8: Update CLI commands for new architecture #123 cross-path scenario (buildIndex + explicit attach)
• Regression test: hotLoadModels load-then-attach cross-path (no failures/warnings)
• Regression test: extension A/B selective attachment (new B attaches, existing A skipped)
• Regression test: resetLoadedFlag invariant (ADV-2 verification)
• Genuine method-name conflicts still fail properly (existing test passes)

Closes swamp-club#318

🤖 Generated with Claude Code

[github-actions]

Code Review

Clean, well-tested refactor that replaces an expensive O(import+parse) idempotency guard with an O(1) fingerprint-aware tracking set, and fixes a real bug where buildIndex didn't trigger extension attachment when only an extension file (not the base model) was stale.

Blocking Issues

None.

Suggestions

1. String-based error matching is fragile (model_kind_adapter.ts:631): The String(failure.error).includes("already exists on model type") check in importAndExtendBundle couples this code to the exact error message in modelRegistry.extend() (model.ts:933,943). If that message ever changes, this silently breaks. Consider introducing a typed error class (e.g., ExtensionAlreadyAttachedError) or an error code so the check is structural rather than string-based. Not blocking since it's defense-in-depth behind the tracking set, and the error string is specific enough to avoid false positives.

2. Module-level mutable state: The attachedExtensions map is module-level state with no production reset path. Fine for a CLI where process lifetime is short, but worth noting — if this adapter is ever used in a long-running process (e.g., watch mode, daemon), the map grows unbounded. The clearAttachedExtensions export exists for tests; if a production reset path is ever needed, it's already wired up.

What looks good

• The extensionTarget return from rebundleAndUpdateCatalog correctly closes the gap where stale extension files didn't trigger attachPendingExtensionsForType
• Defense-in-depth: tracking set prevents most duplicate work, and importAndExtendBundle gracefully handles "already exists" for cross-path scenarios (load() → attachPendingExtensionsForType)
• Four regression tests cover the key scenarios: cross-path idempotency (#123), hot-load, selective A/B attachment, and resetLoadedFlag invariant (ADV-2)
• Tests properly handle Windows cleanup with .catch(() => {}) and use unique timestamped type IDs for isolation
• DDD structure is sound: tracking state lives in the adapter (domain service), catalog mutations stay in the infrastructure layer

[github-actions]

Adversarial Review

Medium

1. loadSingleType bypasses tracking map (extension_loader.ts:359-372): loadSingleType iterates findExtensionsForType and calls importAndExtendBundle directly — it never consults isExtensionAttached and never calls markExtensionAttached. If attachPendingExtensionsForType runs later for the same type (as happens in the hotLoadModels flow at auto_resolver_adapters.ts:268), the tracking map says "not attached" → the bundle is re-imported and extend() throws "already exists" → caught by the string-matching fallback. Functionally correct thanks to the idempotent fallback, but the claimed O(1) optimization does not apply to this cross-path. The unnecessary re-import is the same cost as the old allExtensionMethodsAttached approach in this specific scenario.

2. clearAttachedExtensions not wired into production reload paths (open.ts:74-90, doctor.ts:309-311): reloadExtensionRegistries() calls resetLoadedFlag() and resetExtensionLoadWarnings() but not clearAttachedExtensions(). Currently safe because resetLoadedFlag doesn't clear registered models — the tracking map truthfully reflects that those extensions are already on the model. However, this creates an undocumented invariant: attachedExtensions and modelRegistry.models must stay in sync for the process lifetime. If any future change introduces model unregistration or registry clearing, extensions for re-registered models would be silently skipped. Consider either (a) calling clearAttachedExtensions() alongside resetLoadedFlag() in production, or (b) documenting this invariant in a code comment near the attachedExtensions declaration.

3. String matching as idempotency mechanism (model_kind_adapter.ts:631): String(failure.error).includes("already exists on model type") is the load-bearing safety net for cross-path idempotency (tested in test 2 and test 4). If the error message format in model.ts:932-934 changes, this silently breaks — importAndExtendBundle would start reporting genuine failures for idempotent re-attachment. This coupling crosses file boundaries with no compile-time guarantee. Consider exporting a sentinel string or error subclass from model.ts to make the coupling explicit.

Low

1. Fingerprint tracking after idempotent catch records stale state (model_kind_adapter.ts:671-677): When an extension with a changed fingerprint is re-attached, extend() throws "already exists" (old method stays), but markExtensionAttached records the new fingerprint. The tracking map now claims the new code is active when the old execute function is still on the model. This is the same behavior as the old approach (which checked method names, not code), so not a regression — but the fingerprint-aware tracking map gives false confidence that code updates are reflected. A comment noting this limitation would prevent future confusion.

2. load() path doesn't mark extensions (extension_loader.ts:220-232): Extensions attached via processSecondaryExport in the load() path are not tracked in attachedExtensions, same gap as finding #1. The hotLoadModels scenario test covers this explicitly, confirming the string-matching fallback works, but the pattern means every load() → attachPendingExtensionsForType transition pays the full import cost once.

Verdict

PASS — No critical or high severity findings. The refactoring is functionally correct: the fingerprint-aware tracking set eliminates the expensive allExtensionMethodsAttached guard for the primary buildIndex → attachPendingExtensionsForType path, and the "already exists" string-matching fallback correctly handles the three cross-path scenarios (load() → attach, loadSingleType → attach, hotLoadModels → attach). The four regression tests provide solid coverage of the key invariants. The medium findings are coupling/maintainability concerns, not correctness bugs.