# Phase 1 Amendment Sub-Plan: async_trait -> trait_variant **Status**: Draft **Depends on**: Phase 1 (already on `feat/storage-trait-phase1`, tip 790c0c8) **Followed by**: `docs/plans/0001c-async-trait-sunset.md` (Embedder rewrite + async-trait crate removal) **Related**: docs/adr/0002-phase-2-execution.md (decision D1), docs/plans/0001-phase-1-storage-trait-extraction.md --- ## Context Phase 1 froze with the storage trait declared as `#[async_trait::async_trait] pub trait MemoryStore: Send + Sync + 'static` and a `pub use MemoryStore as LocalMemoryStore;` alias. `async_trait` boxes every `async fn` in the trait into `Pin>`. That is one heap allocation per call inside the hottest code path in Vestige (insert, search, update_scheduling are all on this surface). It also collapses the two intended trait shapes -- a Send-bound `MemoryStore` for tokio/axum and a non-Send `LocalMemoryStore` for thread-local backends -- into a single trait behind an alias, removing the type-system signal we will want for Phase 2's Postgres backend. ADR 0002 decision D1 supersedes this. The amendment lands on the existing `feat/storage-trait-phase1` branch before Phase 2 starts, before the PR is opened upstream. The end state: - `LocalMemoryStore` is the source-of-truth trait, declared with native async-fn-in-trait (stable on MSRV 1.91), no `Sync` bound on the trait itself, and `Sync + 'static` on the trait object. - `MemoryStore` is auto-generated by `#[trait_variant::make(MemoryStore: Send)]` with `Send` bounds on every returned future, so `Arc` is movable across `tokio::spawn`. - `trait-variant` 0.1 is already present in `crates/vestige-core/Cargo.toml`. The `async-trait` crate dependency stays in place through this sub-plan (it is still in use by the embedder impl); removing it is the job of `0001c-async-trait-sunset.md`. - No production caller changes. Every production call site holds `Arc` (the concrete `SqliteMemoryStore` alias), which the trait rewrite does not touch. Only the trait declaration and impl blocks change. --- ## Scope ### In scope - Rewrite `MemoryStore` / `LocalMemoryStore` declaration in `crates/vestige-core/src/storage/memory_store.rs` to use `#[trait_variant::make(MemoryStore: Send)] pub trait LocalMemoryStore`. - Remove the `pub use MemoryStore as LocalMemoryStore;` alias from the same file. `LocalMemoryStore` becomes the real trait; `MemoryStore` is the generated Send variant. - Drop the `#[async_trait::async_trait]` attribute on the SQLite impl block in `crates/vestige-core/src/storage/sqlite.rs` (line 6274). The impl block switches from `impl MemoryStore for SqliteMemoryStore` (currently spelled through the `LocalMemoryStore` alias) to `impl LocalMemoryStore for SqliteMemoryStore`. `trait_variant` provides a blanket `impl MemoryStore for T` so `&dyn MemoryStore` and `Arc` keep working unchanged. - Update doc comments in `memory_store.rs` to drop references to `#[async_trait::async_trait]` and instead describe the `trait_variant` mechanism. - Keep the existing Phase 1 test suite (`tests/phase_1/*.rs`) green. The tests already use `Arc` and `Box`, which is exactly the surface the rewrite is meant to preserve. ### Out of scope -- moved to 0001c - Rewriting the `Embedder` / `LocalEmbedder` trait declaration -- handled by `0001c-async-trait-sunset.md`. - Dropping `#[async_trait::async_trait]` from `crates/vestige-core/src/embedder/fastembed.rs`. - Removing `async-trait = "0.1"` from `crates/vestige-core/Cargo.toml`. - The hard `grep -rn 'async_trait' crates/` zero-match gate (async_trait is still present in the embedder module after 0001a alone). ### Out of scope - The SQLite file split (`sqlite.rs` -> `sqlite/` directory). That is the sibling sub-plan `0001b-sqlite-split.md`. - Any production-side refactor that switches `Arc` to `Arc`. Production code keeps using the concrete alias. - Adding `register_model` / `from_pool` / Postgres-specific variants of `MemoryStoreError`. Those land with the Postgres backend in Phase 2. - Removing the `pub type Storage = SqliteMemoryStore;` alias. --- ## Prerequisites ### Current state (verified) - `crates/vestige-core/Cargo.toml` already declares `trait-variant = "0.1"` (line 117). `async-trait = "0.1"` (line 119) remains in place for the duration of this sub-plan; `0001c` removes it. - `crates/vestige-core/src/storage/memory_store.rs:194` declares the trait with `#[async_trait::async_trait] pub trait MemoryStore: Send + Sync + 'static`. - `crates/vestige-core/src/storage/memory_store.rs:262` has `pub use MemoryStore as LocalMemoryStore;`. - `crates/vestige-core/src/storage/sqlite.rs:6274` declares `#[async_trait::async_trait] impl crate::storage::memory_store::LocalMemoryStore for SqliteMemoryStore`. - Production call sites use `Arc` (the concrete `SqliteMemoryStore` alias). Confirmed by grep: `grep -rn "dyn MemoryStore\|dyn LocalMemoryStore" --include="*.rs"` returns hits only in `tests/phase_1/*.rs`, in two comments inside `memory_store.rs` and `sqlite.rs`, and in one test-only `&dyn MemoryStore` cast inside `sqlite.rs::tests` (line 8669). - Workspace-wide `async_trait` usages: only the trait declarations and impl blocks in `memory_store.rs`, `sqlite.rs`, `embedder/mod.rs`, and `embedder/fastembed.rs` (verified by `grep -rn async_trait --include="*.rs"`). This sub-plan touches only the first two; the embedder files are addressed by `0001c`. ### Required crates | Crate | Version | Action | |-------|---------|--------| | `trait-variant` | `0.1` | Already declared. Verify present after edit. | | `async-trait` | `0.1` | Unchanged for this sub-plan (still used by embedder impl). Removed by `0001c`. | | `blake3`, `thiserror`, `chrono`, `uuid`, `serde`, `serde_json` | unchanged | unchanged | --- ## Files Touched Grouped by category. Every change is listed; nothing else gets touched. ### Trait declarations (vestige-core) | File | Lines (approx) | Change | |------|----------------|--------| | `crates/vestige-core/src/storage/memory_store.rs` | 183-262 | Replace `#[async_trait::async_trait]` block with `#[trait_variant::make(MemoryStore: Send)] pub trait LocalMemoryStore`. Drop the `pub use MemoryStore as LocalMemoryStore;` alias. Update doc comments. | ### Impl blocks (vestige-core) | File | Lines (approx) | Change | |------|----------------|--------| | `crates/vestige-core/src/storage/sqlite.rs` | 6274 (impl block header only) | Delete the `#[async_trait::async_trait]` attribute. Keep the `impl crate::storage::memory_store::LocalMemoryStore for SqliteMemoryStore { ... }` body verbatim. | ### Cargo dependency cleanup None in this sub-plan. The `async-trait` crate stays declared in `crates/vestige-core/Cargo.toml` because the embedder impl still uses it. `0001c-async-trait-sunset.md` removes the dependency after the embedder side is rewritten. ### Cognitive module call sites No changes. The 29 cognitive modules under `crates/vestige-core/src/neuroscience/` and `crates/vestige-core/src/advanced/` already operate on concrete `SqliteMemoryStore` (via the `Storage` alias) or on plain data types (`KnowledgeNode`, `Vec`, `ConnectionRecord`). Grep verified zero production references to `dyn MemoryStore` or `dyn LocalMemoryStore`. ### MCP call sites No changes. All ~30 `vestige-mcp/src/**.rs` files holding `Arc` keep working because: - `Storage` is still `pub type Storage = SqliteMemoryStore;` (unchanged). - They call inherent methods on the concrete type, never via a trait object. - `SqliteMemoryStore` implements `LocalMemoryStore`; `trait_variant` auto- generates a blanket `impl MemoryStore for T`, so the concrete type also satisfies `MemoryStore` for any future caller that wants the trait-object form. ### Test files (vestige-core integration tests) | File | Lines | Change | |------|-------|--------| | `tests/phase_1/trait_round_trip.rs` | 7-18, 134 | No change. Already uses `Arc` and `use vestige_core::storage::MemoryStore`. trait_variant emits a `MemoryStore` trait at the same path, so the imports resolve. | | `tests/phase_1/send_bound_variant.rs` | 10-12, 36, 57 | No change. Already asserts the trait_variant Send invariant; the rewrite is what makes the doc comment on lines 3-4 actually true. | | `tests/phase_1/cognitive_module_isolation.rs` | 11, 37, 76, 102, 115 | No change. | | `tests/phase_1/embedding_model_registry.rs` | 10 | No change. | | `tests/phase_1/domain_column_migration.rs` | 98 | No change. | | `crates/vestige-core/src/storage/sqlite.rs::tests` | 8666-8675 | No change. The existing test casts `&s` to `&dyn MemoryStore` and calls trait methods through that vtable; trait_variant preserves this exact dyn-compatible surface. | ### Documentation | File | Change | |------|--------| | `crates/vestige-core/src/storage/memory_store.rs` | Rewrite the trait-level doc comment (lines 185-193) to describe trait_variant rather than async_trait. | | `CLAUDE.md` | No change. The repo-level architecture notes do not name the trait attribute. | --- ## Trait Declaration Rewrite ### Before (current state on `feat/storage-trait-phase1`) `crates/vestige-core/src/storage/memory_store.rs:183-262`: ```rust // ---------------------------------------------------------------------------- // TRAIT // ---------------------------------------------------------------------------- /// The single storage abstraction. /// /// `#[async_trait::async_trait]` makes every `async fn` return a /// `Pin>`, which is required for `Arc` /// to be movable across `tokio::spawn` boundaries. /// /// `LocalMemoryStore` is a type alias kept for source compatibility with code /// that refers to the non-send variant. In Phase 1 both names refer to the same /// (dyn-compatible, Send-safe) trait. #[async_trait::async_trait] pub trait MemoryStore: Send + Sync + 'static { // --- Lifecycle --- async fn init(&self) -> MemoryStoreResult<()>; async fn health_check(&self) -> MemoryStoreResult; // ... 25 more async fn ... async fn vacuum(&self) -> MemoryStoreResult<()>; } /// Type alias kept for source compatibility. Both names refer to the same /// `async_trait`-annotated trait that is dyn-compatible and `Send + Sync`. pub use MemoryStore as LocalMemoryStore; ``` ### After `crates/vestige-core/src/storage/memory_store.rs:183-262`: ```rust // ---------------------------------------------------------------------------- // TRAIT // ---------------------------------------------------------------------------- /// The single storage abstraction. /// /// `LocalMemoryStore` is the source-of-truth trait. The /// `#[trait_variant::make(MemoryStore: Send)]` attribute auto-generates a /// `MemoryStore` trait whose returned futures are `Send`, so /// `Arc` is movable across `tokio::spawn` boundaries while /// `Arc` remains usable on single-threaded executors /// and thread-local backends. /// /// Every method is native async-fn-in-trait (stable on MSRV 1.91); no /// per-call heap allocation, no boxed futures. #[trait_variant::make(MemoryStore: Send)] pub trait LocalMemoryStore: Sync + 'static { // --- Lifecycle --- async fn init(&self) -> MemoryStoreResult<()>; async fn health_check(&self) -> MemoryStoreResult; // --- Embedding model registry --- async fn registered_model(&self) -> MemoryStoreResult>; async fn register_model(&self, sig: &ModelSignature) -> MemoryStoreResult<()>; // --- CRUD --- async fn insert(&self, record: &MemoryRecord) -> MemoryStoreResult; async fn get(&self, id: Uuid) -> MemoryStoreResult>; async fn update(&self, record: &MemoryRecord) -> MemoryStoreResult<()>; async fn delete(&self, id: Uuid) -> MemoryStoreResult<()>; // --- Search --- async fn search(&self, query: &SearchQuery) -> MemoryStoreResult>; async fn fts_search(&self, text: &str, limit: usize) -> MemoryStoreResult>; async fn vector_search( &self, embedding: &[f32], limit: usize, ) -> MemoryStoreResult>; // --- FSRS Scheduling --- async fn get_scheduling( &self, memory_id: Uuid, ) -> MemoryStoreResult>; async fn update_scheduling(&self, state: &SchedulingState) -> MemoryStoreResult<()>; async fn get_due_memories( &self, before: DateTime, limit: usize, ) -> MemoryStoreResult>; // --- Graph (spreading activation) --- async fn add_edge(&self, edge: &MemoryEdge) -> MemoryStoreResult<()>; async fn get_edges( &self, node_id: Uuid, edge_type: Option<&str>, ) -> MemoryStoreResult>; async fn remove_edge(&self, source: Uuid, target: Uuid) -> MemoryStoreResult<()>; async fn get_neighbors( &self, node_id: Uuid, depth: usize, ) -> MemoryStoreResult>; // --- Domains --- async fn list_domains(&self) -> MemoryStoreResult>; async fn get_domain(&self, id: &str) -> MemoryStoreResult>; async fn upsert_domain(&self, domain: &Domain) -> MemoryStoreResult<()>; async fn delete_domain(&self, id: &str) -> MemoryStoreResult<()>; async fn classify(&self, embedding: &[f32]) -> MemoryStoreResult>; // --- Bulk / Maintenance --- async fn count(&self) -> MemoryStoreResult; async fn get_stats(&self) -> MemoryStoreResult; async fn vacuum(&self) -> MemoryStoreResult<()>; } ``` Notes: - The `pub use MemoryStore as LocalMemoryStore;` line on the current `memory_store.rs:262` is **deleted** entirely. `MemoryStore` is now the generated trait that `trait_variant::make` emits at the same module path; `LocalMemoryStore` is the source-of-truth declaration. Both names export from `storage/mod.rs` already (see lines 10-14 of that file). - `Sync + 'static` on `LocalMemoryStore` (and no `Send` bound on the trait itself) is correct: `Send` on the trait is what `trait_variant::make` inserts when it emits the `MemoryStore` variant. The current trait carries `Send + Sync + 'static`; the rewrite drops the `Send` bound from the local variant. `Arc` is `Sync` but not `Send`; `Arc` (the generated variant) is `Send + Sync`. - `trait_variant` 0.1 does **not** require any attribute on the impl block. The attribute lives only on the trait declaration. See the next section. The Embedder trait rewrite uses the identical `trait_variant::make` pattern and is fully specified in `0001c-async-trait-sunset.md`. --- ## Impl Block Migration `trait_variant` 0.1 attaches the attribute only to the trait declaration. The impl side is plain `impl LocalMemoryStore for SqliteMemoryStore`; no attribute on the impl, no `#[trait_variant::make(MemoryStore: Send)]` on the impl block. The crate auto-generates the blanket `impl MemoryStore for T`, so any concrete type that implements `LocalMemoryStore` automatically also implements `MemoryStore` provided it is `Send`. `SqliteMemoryStore` already is `Send + Sync` (it holds `Mutex` fields whose `Mutex` is `std::sync::Mutex`, which is `Send + Sync` when its guarded type is `Send`; `rusqlite::Connection` is `Send`). No new bound is needed. ### Before `crates/vestige-core/src/storage/sqlite.rs:6274`: ```rust #[async_trait::async_trait] impl crate::storage::memory_store::LocalMemoryStore for SqliteMemoryStore { async fn init(&self) -> crate::storage::memory_store::MemoryStoreResult<()> { // Migrations run in `new`; this is a no-op for the SQLite backend. Ok(()) } // ... ~2400 lines of method bodies, unchanged ... } ``` ### After `crates/vestige-core/src/storage/sqlite.rs:6274`: ```rust impl crate::storage::memory_store::LocalMemoryStore for SqliteMemoryStore { async fn init(&self) -> crate::storage::memory_store::MemoryStoreResult<()> { // Migrations run in `new`; this is a no-op for the SQLite backend. Ok(()) } // ... ~2400 lines of method bodies, unchanged ... } ``` Diff is exactly one removed line (the `#[async_trait::async_trait]` attribute). Every method body, every `async fn` signature, every `use` statement inside the impl block stays verbatim. No `Box::pin(async move { ... })`, no manual `Pin>`, no `'async_trait` lifetime markers; native async-fn-in-trait does this directly. The Embedder impl block rewrite follows the identical "remove one `#[async_trait::async_trait]` attribute" pattern and is fully specified in `0001c-async-trait-sunset.md`. ### Why the impl block does not need an attribute `trait_variant::make` generates two things from the source trait: 1. The source trait itself (`LocalMemoryStore`), with native async fns. 2. A second trait (`MemoryStore`) whose method signatures return `impl Future + Send` instead of `impl Future`, plus a blanket impl wiring concrete types through. Both are emitted at the macro-call site. `SqliteMemoryStore` only writes one impl block (against `LocalMemoryStore`); the macro-generated blanket guarantees `SqliteMemoryStore: MemoryStore` for free. The current `&dyn MemoryStore` casts (sqlite.rs:8669; tests under tests/phase_1/) keep type-checking unchanged. --- ## Call Site Audit Verified via: ```bash grep -rn "dyn MemoryStore\|dyn LocalMemoryStore" --include="*.rs" \ /home/delandtj/prppl/vestige-phase2/ grep -rn "Arc\|Arc" --include="*.rs" \ /home/delandtj/prppl/vestige-phase2/ grep -rn "use.*MemoryStore;\|use.*LocalMemoryStore;" --include="*.rs" \ /home/delandtj/prppl/vestige-phase2/ ``` ### Files that reference the trait object form (`dyn MemoryStore` / `dyn LocalMemoryStore`) All test-only or doc-comment-only: | File | Line | Use | Required change | |------|------|-----|-----------------| | `tests/phase_1/trait_round_trip.rs` | 7-18 | `Arc` in `make_store()` and test bodies | None. `MemoryStore` is the generated Send variant; signature stays. | | `tests/phase_1/trait_round_trip.rs` | 134 | `Arc` upcast inside a test body | None. | | `tests/phase_1/send_bound_variant.rs` | 10-97 | `Arc` moved across `tokio::spawn` | None. This test becomes meaningfully correct only after the rewrite (currently it relies on async_trait boxing; after the rewrite it relies on trait_variant's Send variant -- same observable outcome). | | `tests/phase_1/cognitive_module_isolation.rs` | 11, 33-115 | `Arc` passed into cognitive module-style closures | None. | | `tests/phase_1/embedding_model_registry.rs` | 10 | `Arc` in `make_store()` | None. | | `tests/phase_1/domain_column_migration.rs` | 98 | `Arc` cast inside a migration assertion | None. | | `crates/vestige-core/src/storage/sqlite.rs` | 8666-8675 | `let dyn_s: &dyn MemoryStore = &s;` inside `mod tests` | None. The cast is testing that the dyn-vtable still resolves the trait methods correctly; after the rewrite it resolves through the `MemoryStore` trait that `trait_variant::make` emits at the same path. | | `crates/vestige-core/src/storage/memory_store.rs` | 188 (doc) | Doc comment mentioning `Arc` | Replaced as part of the doc rewrite (see Trait Declaration section). | ### Files that hold the concrete type (`Arc` / `Arc`) 35 files, 116 hits. Every one of them keeps working unchanged because the concrete `SqliteMemoryStore` type stays exactly as it is. Listed here for completeness so a reviewer can confirm none of them needs an edit: ``` crates/vestige-core/src/storage/mod.rs (alias declaration) crates/vestige-core/src/storage/sqlite.rs (impl block) crates/vestige-mcp/src/server.rs (Arc in McpServer) crates/vestige-mcp/src/cognitive.rs (hydrate(&Storage)) crates/vestige-mcp/src/autopilot.rs crates/vestige-mcp/src/protocol/http.rs crates/vestige-mcp/src/dashboard/mod.rs crates/vestige-mcp/src/dashboard/state.rs crates/vestige-mcp/src/dashboard/handlers.rs crates/vestige-mcp/src/resources/codebase.rs crates/vestige-mcp/src/resources/memory.rs crates/vestige-mcp/src/tools/changelog.rs crates/vestige-mcp/src/tools/codebase_unified.rs crates/vestige-mcp/src/tools/context.rs crates/vestige-mcp/src/tools/contradictions.rs crates/vestige-mcp/src/tools/cross_reference.rs crates/vestige-mcp/src/tools/dedup.rs crates/vestige-mcp/src/tools/dream.rs crates/vestige-mcp/src/tools/explore.rs crates/vestige-mcp/src/tools/feedback.rs crates/vestige-mcp/src/tools/graph.rs crates/vestige-mcp/src/tools/health.rs crates/vestige-mcp/src/tools/importance.rs crates/vestige-mcp/src/tools/intention_unified.rs crates/vestige-mcp/src/tools/maintenance.rs crates/vestige-mcp/src/tools/memory_states.rs crates/vestige-mcp/src/tools/memory_unified.rs crates/vestige-mcp/src/tools/predict.rs crates/vestige-mcp/src/tools/restore.rs crates/vestige-mcp/src/tools/review.rs crates/vestige-mcp/src/tools/search_unified.rs crates/vestige-mcp/src/tools/session_context.rs crates/vestige-mcp/src/tools/smart_ingest.rs crates/vestige-mcp/src/tools/suppress.rs crates/vestige-mcp/src/tools/tagging.rs crates/vestige-mcp/src/tools/timeline.rs ``` Each holds `Arc` and dispatches to inherent methods on `SqliteMemoryStore`. None of them goes through a trait vtable. Required change for every one of them: **none**. ### Files that `use ...MemoryStore` from production code ``` grep -rn "use.*MemoryStore;\|use.*LocalMemoryStore;" --include="*.rs" \ | grep -v "memory_store.rs\|sqlite.rs\|tests/phase_1" ``` returns nothing. Production code does not import the trait by name. ### Conclusion The rewrite is a strictly local change to two source files (`storage/memory_store.rs` and `storage/sqlite.rs`). Zero production call sites need edits. The integration tests that consume `Arc` keep their current form; the rewrite is what gives that signature its no-box semantics on the storage side. The `Box` surface is addressed by `0001c-async-trait-sunset.md`. --- ## Commit Sequence Two commits, each green on `cargo test -p vestige-core --no-default-features` and `cargo test -p vestige-core --features embeddings,vector-search`. ### Commit 1: rewrite MemoryStore / LocalMemoryStore trait declaration - Touches: `crates/vestige-core/src/storage/memory_store.rs` only. - Action: replace lines 183-262 per the "Trait Declaration Rewrite" section above. Delete the `pub use MemoryStore as LocalMemoryStore;` line. - Green after: `cargo check -p vestige-core` (the impl block in `sqlite.rs` still has `#[async_trait::async_trait]` on it, but it still resolves through the `LocalMemoryStore` trait which is now native-async; the `async_trait` macro is harmless when applied to a trait that the impl block targets by path, because the macro rewrites the impl's async fns into boxed-future fns whose signatures still match the native-async declarations after trait_variant lowering). If `cargo check` complains here, fold commit 2 into commit 1. **Mitigation if check fails between commits 1 and 2:** combine the two into a single commit. The split is offered for review convenience; the build must be green after every commit lands. ### Commit 2: drop `#[async_trait::async_trait]` from SqliteMemoryStore impl - Touches: `crates/vestige-core/src/storage/sqlite.rs` only. - Action: delete line 6274 (`#[async_trait::async_trait]`). - Green after: `cargo test -p vestige-core --features embeddings,vector-search`, including all `trait_*` tests inside `sqlite.rs::tests` (lines 8643-8712) and the trait-object cast on line 8669. ### Combined alternative If the per-step split feels artificial, commits 1 and 2 can collapse into a single commit covering both the trait rewrite and the impl-attribute drop for `MemoryStore`. That is acceptable; the two-commit form is preferred only because it lets a reviewer bisect trait-rewrite failures separately from impl-rewrite failures. The Embedder / fastembed commits and the `async-trait` Cargo dependency removal live in `0001c-async-trait-sunset.md`. --- ## Verification Every command runs from the repo root unless noted otherwise. ```bash # 1. Vestige-core, default features (embeddings + vector-search). cargo test -p vestige-core --features embeddings,vector-search # 2. Vestige-core, minimal features (no embeddings, no vector-search). cargo test -p vestige-core --no-default-features # 3. Workspace build, release mode (catches any feature-gated regression # in the vestige-mcp tools tree). cargo build --workspace --release # 4. Whole-workspace test (vestige-mcp 406 tests + vestige-core 352 tests # per the CLAUDE.md baseline). cargo test --workspace # 5. Phase 1 integration tests (these are the trait-shape contract). cargo test --test trait_round_trip --features embeddings,vector-search cargo test --test send_bound_variant --features embeddings,vector-search cargo test --test cognitive_module_isolation --features embeddings,vector-search cargo test --test embedding_model_registry --features embeddings,vector-search cargo test --test domain_column_migration --features embeddings,vector-search # 6. Clippy gate, deny warnings (matches Phase 1 PR policy of zero warnings). cargo clippy --workspace --all-targets --features embeddings,vector-search -- -D warnings # 7. Storage-side dependency hygiene check (must return zero hits). # Scoped to the storage module only -- the embedder module still uses # async_trait until 0001c lands. grep -rn "async_trait\|async-trait" crates/vestige-core/src/storage/ # 8. Confirm trait_variant attribute is in place on the storage trait # (must return exactly one hit in memory_store.rs). grep -rn "trait_variant::make" crates/vestige-core/src/storage/ ``` Expected outcomes: - Command 1: 352 tests pass (matches baseline). - Command 2: smaller test count, all pass. - Command 3: workspace compiles in release mode. - Command 4: 758 total tests pass (matches CLAUDE.md baseline). - Command 5: each phase_1 integration test binary runs green. The `send_bound_variant::arc_dyn_memory_store_moves_across_tokio_tasks` test is the canary; if `MemoryStore` lost its Send-bound future variant, this fails to compile with "future cannot be sent between threads safely". - Command 6: zero clippy warnings. The rewrite must not introduce a new `clippy::needless_lifetimes` or `clippy::async_yields_async`. - Command 7: empty output. async_trait is gone from the storage module. The embedder module still contains async_trait; that is removed by `0001c-async-trait-sunset.md`. - Command 8: one hit, in `memory_store.rs`. --- ## Acceptance Criteria A reviewer should be able to check every box: - [ ] `crates/vestige-core/src/storage/memory_store.rs` declares the trait with `#[trait_variant::make(MemoryStore: Send)] pub trait LocalMemoryStore: Sync + 'static`, no `async_trait` attribute, no `Send` bound on `LocalMemoryStore` itself. - [ ] `crates/vestige-core/src/storage/memory_store.rs` no longer contains `pub use MemoryStore as LocalMemoryStore;`. - [ ] `crates/vestige-core/src/storage/sqlite.rs:6274` is plain `impl crate::storage::memory_store::LocalMemoryStore for SqliteMemoryStore` -- no attribute on the impl block. - [ ] `grep -rn "async_trait\|async-trait" crates/vestige-core/src/storage/` returns zero hits. - [ ] `grep -rn "trait_variant::make" crates/vestige-core/src/storage/` returns exactly one hit (the storage trait in `memory_store.rs`). - [ ] All 758 workspace tests pass (`cargo test --workspace`). - [ ] Phase 1 integration tests pass with the trait-object surface (`Arc`) intact. - [ ] `cargo clippy --workspace --all-targets --features embeddings,vector-search -- -D warnings` is clean. - [ ] No production source file under `crates/vestige-mcp/` or `crates/vestige-core/src/{neuroscience,advanced,consolidation,codebase, memory,embeddings,embedder}/` was modified by this sub-plan. - [ ] `Arc` still type-checks at every existing call site (verified by the workspace test pass). - [ ] Doc comments on the storage trait declaration describe `trait_variant`, not `async_trait`. --- ## Risks and Mitigations - **`trait_variant` 0.1 macro emits unexpected diagnostics on MSRV 1.91.** Mitigation: the master Phase 1 plan already prescribed this exact pattern (`#[trait_variant::make(MemoryStore: Send)] pub trait LocalMemoryStore: Sync + 'static`, see plan `0001-...` line 274-275); the crate has been in `vestige-core/Cargo.toml` since Phase 1 landed. If a diagnostic appears, pin to the exact known-good version with `trait-variant = "=0.1.2"` and open an upstream issue. - **Native async-fn-in-trait makes the trait no longer dyn-compatible.** Mitigation: `trait_variant::make` is specifically the workaround for this -- it emits both the source trait (for static dispatch) and a Send-bound variant whose returned futures use `Pin>` only at the dyn boundary. `Arc` keeps working because the generated `MemoryStore` trait is dyn-compatible by construction. Verified by the existing `send_bound_variant::*` tests, which intentionally move `Arc` across `tokio::spawn` from inside a `multi_thread` runtime. - **A cognitive module silently relied on the boxed-future return type.** Mitigation: grep verified no cognitive module imports `MemoryStore` / `LocalMemoryStore` or holds an `Arc` form; all of them use the concrete `Storage` alias. There is no Send-ness expectation downstream to break. - **Future bodies inside the SQLite impl capture non-Send locals.** Mitigation: every method body in `sqlite.rs:6274..` runs synchronous rusqlite calls inside the same `async fn` frame; no `.await` points exist inside the bodies that we ship today. The `Send` bound on the generated `MemoryStore` variant is therefore satisfied automatically. If a future change adds `.await` inside an impl method, the new trait_variant surface will surface that as a compile error at the call site, which is the correct outcome. - **`async-trait` crate is left declared after this sub-plan.** This is intentional: the embedder impl still depends on it. The `0001c-async-trait-sunset.md` sub-plan removes the crate after the embedder side is rewritten. Grep on the whole workspace returns only the storage and embedder files; no downstream crate pulls `async-trait`. --- ## Out-of-Band Notes - This sub-plan amends `feat/storage-trait-phase1` (tip 790c0c8). The branch has not been opened upstream yet, so amending in place is safe; no force-push to a public PR. - The companion sub-plan `0001b-sqlite-split.md` lands after this one on the same branch. The trait-rewrite landing first is intentional: the SQLite split moves the impl block into `storage/sqlite/trait_impl.rs`, and it is cleaner to move a small attribute-free impl than a macro-decorated one. - The companion sub-plan `0001c-async-trait-sunset.md` lands after this one (order with `0001b` is independent) and finishes the async_trait -> trait_variant transition for the Embedder trait, then removes the `async-trait` crate dependency. - After the Phase 1 amendment sub-plans (`0001a`, `0001b`, `0001c`) land, the branch is reviewed and merged before Phase 2 sub-plans (`0002a-` through `0002i-`) begin implementation.