Follow-up from #218 (merged in cf196ea). Code review found documentation and type-safety gaps in the new _built_collection_ids cache pattern:
-
search() docstring inconsistency (line 77-85): Docstring claims "Self-heals a missing/emptied index" but _ensure() only rebuilds if collection is empty and its id doesn't match the cached built id. An intentionally-emptied-then-marked-built collection is now trusted and won't rebuild, contradicting the docstring claim.
-
Type-safety gap (line 84): _built_collection_ids[self._collection_name()] = self._collection.id dereferences self._collection.id without a guard. self._collection is typed Any | None, so a future reorder could pass None. packages/ is excluded from mypy, so no checker catches this. Return the collection from _reset_collection() or use self._open().id to guarantee it's not None.
-
Behavior change: empty vault stale display (line 178): An empty vault marked as built stays empty-looking until the auto-sweep runs (up to refresh_min_interval_s, default 60s). Previously _ensure's rebuild caught the first note immediately. This is correct by design but worth documenting as an intended latency trade-off.
-
Module-global not reset between tests (line 28): _built_collection_ids is module-global and never pruned/reset. Safe today only because each test uses a distinct tmp_path (distinct name hash), but fragile if a future test reuses vault paths. Consider a test fixture to clear the cache per test or document the assumption in the code.
Follow-up from #218 (merged in cf196ea). Code review found documentation and type-safety gaps in the new _built_collection_ids cache pattern:
search() docstring inconsistency (line 77-85): Docstring claims "Self-heals a missing/emptied index" but _ensure() only rebuilds if collection is empty and its id doesn't match the cached built id. An intentionally-emptied-then-marked-built collection is now trusted and won't rebuild, contradicting the docstring claim.
Type-safety gap (line 84):
_built_collection_ids[self._collection_name()] = self._collection.iddereferences self._collection.id without a guard. self._collection is typedAny | None, so a future reorder could pass None. packages/ is excluded from mypy, so no checker catches this. Return the collection from _reset_collection() or use self._open().id to guarantee it's not None.Behavior change: empty vault stale display (line 178): An empty vault marked as built stays empty-looking until the auto-sweep runs (up to refresh_min_interval_s, default 60s). Previously _ensure's rebuild caught the first note immediately. This is correct by design but worth documenting as an intended latency trade-off.
Module-global not reset between tests (line 28): _built_collection_ids is module-global and never pruned/reset. Safe today only because each test uses a distinct tmp_path (distinct name hash), but fragile if a future test reuses vault paths. Consider a test fixture to clear the cache per test or document the assumption in the code.