refactor(core): relocate Embedder trait from weaver-embedding to weaver-core (PR-0.5.B)#275
Merged
Merged
Conversation
…e (PR-0.5.B)
Per docs/specs/weaver-spu-Spec.md §10 PR-0.5.B. Moves the
backend-agnostic `Embedder` trait + associated types to
`weaver-core::embedder` so trait-only consumers can depend on
`weaver-core` alone, without pulling a backend-implementation crate
into their dep graph.
What moves:
- `Embedder` trait + `EmbedderInfo` (from weaver-embedding/src/embedder.rs)
- `EmbedResult`, `LateChunkedResult`, `LateChunkResult`,
`EmbeddingError` (from weaver-embedding/src/grpc_client.rs)
- 5 trait-level tests (object-safety, embed_one default impl,
health_check default, the two strict-1:1 error checks)
All move to `crates/weaver-core/src/embedder.rs`.
Error-type abstraction:
- Original `EmbeddingError` had tonic-typed variants
(`Connection(tonic::transport::Error)`, `Status(tonic::Status)`).
New `EmbeddingError::Transport(Box<dyn Error + Send + Sync>)`
variant abstracts over transport so weaver-core stays clean of
gRPC specifics. `From<tonic::transport::Error>` and
`From<tonic::Status>` impls live in weaver-core (orphan rule
forces them to the trait's crate); the Display preserves the
upstream message via the boxed error.
- `weaver-core` picks up a tonic dep solely for these From impls.
Removed in PR-3.A along with the gRPC client. Tonic was already
transitively present via weaver-database.
- `EmbeddingClient::ensure_ready` updated to downcast the boxed
error rather than match concrete variant names. Behavior
preserved.
Trait dependency direction reversed:
- BEFORE: weaver-core depends on weaver-embedding (for
`EmbeddingClient` direct use in tools/memory/codebase_search.rs).
- AFTER: weaver-embedding depends on weaver-core (for the
relocated trait); weaver-core no longer depends on
weaver-embedding.
Refactor that breaks the prior cycle:
- Added `embedder: Option<Arc<dyn weaver_core::embedder::Embedder>>`
field to `ExecutionContext`. Production daemon
(`weaver-interface::serve`) populates it at boot when the
Phase 1 wiring lands (PR-1.H). Tests construct with `None`.
- `tools/memory/codebase_search` now resolves the embedder via
`ctx.embedder.as_ref().ok_or_else(...)` instead of
constructing `EmbeddingClient` directly. Tools are no longer
responsible for embedder transport setup.
- ExecutionContext loses derive(Debug), gains a manual Debug impl
that prints the embedder slot as a presence flag (dyn Embedder
has no Debug bound).
Backward compatibility:
- `weaver_embedding::embedder::{Embedder, EmbedderInfo}` is
preserved as a deprecated `pub use` re-export from
`weaver_core::embedder`. Existing consumers compile unchanged
during the migration window. Removed in PR-0.5.E.
- `weaver_embedding::grpc_client::{EmbedResult, LateChunkedResult,
LateChunkResult, EmbeddingError}` re-exported from
weaver-core. Same removal.
Acceptance criteria (PR-0.5.B):
- All consumers compile against the new trait location ✓
- All existing trait tests pass at the new location (5/5) ✓
- No `weaver-database` direct use of `weaver-embedding` for the
trait ✓ (weaver-database doesn't import the trait at all today)
- `cargo build --workspace` clean ✓
- `cargo test -p weaver-core --lib` clean (588 passed including
5 embedder tests) ✓
- `cargo clippy -p weaver-core --lib --tests -- -D warnings` clean ✓
- `cargo clippy -p weaver-embedding --lib -- -D warnings` clean ✓
- `cargo clippy -p weaver-spu --lib -- -D warnings` clean ✓
Pre-existing clippy errors in weaver-inference/src/server/handler.rs
(redundant_locals) are not touched by this PR — they exist on main
and are out of scope.
Dependencies: PR-0.5.A (weaver-spu skeleton).
Companion upstream PR: none.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMoves embedder API/types into ChangesEmbedder Migration & Runtime Wiring
Sequence Diagram(s)sequenceDiagram
participant Tool as CodebaseSearchTool
participant ExecCtx as ExecutionContext
participant Embedder as Embedder (dyn backend)
participant DB as Database
Tool->>ExecCtx: execute(query)
ExecCtx->>Embedder: embed_one(query)
Embedder-->>ExecCtx: vector
ExecCtx->>DB: run embedding-aware query(vector)
DB-->>Tool: results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/weaver-interface/src/main.rs (1)
451-458: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueInconsistent indentation on
embedderfield.The
embedder: Nonefield has extra indentation compared to other struct fields. This appears to be a formatting artifact.🧹 Suggested fix for consistent formatting
let tool_ctx = ExecutionContext { working_directory: work_dir.clone(), hades: hades_config, db_pool: None, codebase_pool: None, memory_bypass: false, - embedder: None, + embedder: None, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/weaver-interface/src/main.rs` around lines 451 - 458, The struct literal for ExecutionContext has inconsistent indentation on the embedder field; update the embedder: None line to match the alignment of the other fields (e.g., same indentation as hades: hades_config and memory_bypass: false) so the block is uniformly formatted. Locate the ExecutionContext initialization (symbol: ExecutionContext) and adjust the embedder field indentation to be consistent with the surrounding fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/weaver-core/src/engine/runtime.rs`:
- Around line 956-961: AgentRuntime currently constructs agents with embedder:
None causing ExecutionContext.embedder to be absent and CodebaseSearch to error;
fix by allowing the embedder to be injected either at spawn-time or after
construction: update the spawn APIs (spawn_with_working_dir, spawn_with_extras)
to accept an embedder parameter and pass it into the new
AgentHandle/ExecutionContext, or add a public setter on AgentHandle (e.g.,
set_embedder(&mut self, embedder: Arc<dyn Embedder>) ) that updates the private
exec_ctx.embedder field; ensure ExecutionContext.embedder is set to Some(...)
before the agent is used so CodebaseSearch no longer returns ToolError.
---
Outside diff comments:
In `@crates/weaver-interface/src/main.rs`:
- Around line 451-458: The struct literal for ExecutionContext has inconsistent
indentation on the embedder field; update the embedder: None line to match the
alignment of the other fields (e.g., same indentation as hades: hades_config and
memory_bypass: false) so the block is uniformly formatted. Locate the
ExecutionContext initialization (symbol: ExecutionContext) and adjust the
embedder field indentation to be consistent with the surrounding fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f6743b5-d1f7-4c7e-942d-01423d954ea9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
crates/weaver-core/Cargo.tomlcrates/weaver-core/src/embedder.rscrates/weaver-core/src/engine/runtime.rscrates/weaver-core/src/lib.rscrates/weaver-core/src/tool.rscrates/weaver-core/src/tools/memory/codebase_search.rscrates/weaver-core/tests/engine_tests.rscrates/weaver-core/tests/multi_agent_tests.rscrates/weaver-core/tests/tool_tests.rscrates/weaver-demo/src/hanoi/mod.rscrates/weaver-demo/src/herobench/tools.rscrates/weaver-embedding/Cargo.tomlcrates/weaver-embedding/src/embedder.rscrates/weaver-embedding/src/grpc_client.rscrates/weaver-embedding/src/lib.rscrates/weaver-interface/src/main.rs
Two CR findings on PR #275 commit 9d38cb6: 1. AgentRuntime::spawn_* constructed every agent with exec_ctx.embedder = None and there was no public way to inject one post-spawn. CodebaseSearch would error in production. Fix: added pub fn AgentHandle::set_embedder(Arc<dyn Embedder>) — the daemon (`weaver-interface::serve` per spec PR-1.H) calls this post-spawn to wire the gRPC client (pre-Phase-1) or the in-process candle backend (post-Phase-1) before the agent processes any turn. Idempotent; replaces a prior backend without re-spawning. 2. Indentation was off on `embedder: None` in `weaver-interface/src/main.rs` (artifact from the bulk-add sed). Ran `cargo fmt --all` which fixed the indentation across weaver-interface/main.rs, weaver-core/tests/{engine,multi_agent,tool}_tests, and the embedder module re-export. Unrelated fmt-only changes in other crates excluded from this commit. Validated: - cargo check -p weaver-core clean - cargo test -p weaver-core --lib embedder: 5/5 pass - cargo clippy -p weaver-core --lib --tests -- -D warnings clean
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/weaver-embedding/src/grpc_client.rs`:
- Around line 67-78: The `pub use weaver_core::embedder::{EmbedResult,
EmbedderInfo, EmbeddingError, LateChunkResult, LateChunkedResult}` re-exports
won't emit deprecation warnings; replace these re-exports with deprecated type
aliases (e.g. create `#[deprecated(since = "0.5.D", note = "...")] pub type
EmbedResult = weaver_core::embedder::EmbedResult;` and the same pattern for
`EmbedderInfo`, `EmbeddingError`, `LateChunkResult`, and `LateChunkedResult`) so
downstream consumers get compiler warnings and a clear migration path before
PR-0.5.E removal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9440ffc-7003-4ebf-8445-f240710eab69
📒 Files selected for processing (7)
crates/weaver-core/src/engine/runtime.rscrates/weaver-core/tests/engine_tests.rscrates/weaver-core/tests/multi_agent_tests.rscrates/weaver-core/tests/tool_tests.rscrates/weaver-embedding/src/embedder.rscrates/weaver-embedding/src/grpc_client.rscrates/weaver-interface/src/main.rs
Per CR finding on PR #275: `#[deprecated]` on `pub use` re-exports does not reliably emit warnings at consumer call sites. Switched the data types in `weaver-embedding` to deprecated type aliases so external consumers see the migration warning when they reference the legacy path. Changed: - `grpc_client.rs`: 5 type aliases (`EmbedResult`, `EmbedderInfo`, `EmbeddingError`, `LateChunkResult`, `LateChunkedResult`) replace the prior `pub use` block. - `embedder.rs`: `EmbedderInfo` becomes a deprecated type alias. `Embedder` stays as `pub use` (Rust has no stable trait-alias mechanism; documented inline). Verified the warnings now fire on external call sites (the integration tests in `weaver-embedding/tests/grpc_client.rs` produce the expected `use of deprecated type alias` warnings on their `use` lines, which the previous `pub use` form did not). Internal uses inside `weaver-embedding` itself trigger the same warnings — added `#![allow(deprecated)]` at the crate level. The whole crate is being retired in PR-0.5.E so suppressing internal self-warnings is benign; external consumers are unaffected (they get warnings on their own `use` lines). Validated: - cargo check -p weaver-embedding: clean (0 warnings) - cargo clippy -p weaver-embedding --lib -- -D warnings: clean - cargo test -p weaver-core --lib embedder: 5/5 pass
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/weaver-embedding/src/lib.rs`:
- Around line 37-46: The crate-level allow(deprecated) is too broad—remove the
top-level #![allow(deprecated)] and instead apply narrower allow(deprecated)
attributes directly to the deprecated aliases and re-exports in the grpc_client
and embedder modules: add #[allow(deprecated)] on the five deprecated type
aliases (EmbedResult, EmbeddingError, LateChunkResult, LateChunkedResult,
Embedder) in grpc_client and on the two re-exported items (Embedder,
EmbedderInfo) in embedder (or mark each item individually with
#[allow(deprecated)]), so the rest of the crate continues to be checked under -D
warnings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7537518a-0947-42dd-b652-b6d15b8cbe75
📒 Files selected for processing (3)
crates/weaver-embedding/src/embedder.rscrates/weaver-embedding/src/grpc_client.rscrates/weaver-embedding/src/lib.rs
Comment on lines
+37
to
+46
| // Internal uses of the crate's own deprecated type aliases | ||
| // (`grpc_client::EmbedResult`, `EmbeddingError`, etc., which point at | ||
| // `weaver_core::embedder::*` per PR-0.5.B) trigger `deprecated` | ||
| // warnings under `-D warnings`. The deprecations are *for external | ||
| // consumers* — this crate is being retired in PR-0.5.E anyway, so | ||
| // internal references to its own aliases are benign. The whole-crate | ||
| // allow keeps the build green during the migration window without | ||
| // hiding deprecation drift in callers (callers see the warnings on | ||
| // their own `use` lines). | ||
| #![allow(deprecated)] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -type f -name "lib.rs" | grep weaver-embeddingRepository: toddwbucy/WeaverTools
Length of output: 102
🏁 Script executed:
# Show the file to understand the context around lines 37-46 and 86-87
cat -n crates/weaver-embedding/src/lib.rs | head -120Repository: toddwbucy/WeaverTools
Length of output: 5706
🏁 Script executed:
# Search for other deprecated usages in the crate to see if crate-wide allow suppresses unrelated items
rg "deprecated" crates/weaver-embedding/src/ -A 2 -B 2Repository: toddwbucy/WeaverTools
Length of output: 5421
🏁 Script executed:
# Check if there are clippy configuration files that enforce -D warnings
find . -name "clippy.toml" -o -name ".clippy.toml" -o -name "Cargo.toml" | head -10 | xargs -I {} grep -l "warn" {} 2>/dev/null || trueRepository: toddwbucy/WeaverTools
Length of output: 47
🏁 Script executed:
# Check the Cargo.toml of the weaver-embedding crate for any lint config
cat crates/weaver-embedding/Cargo.tomlRepository: toddwbucy/WeaverTools
Length of output: 6110
🏁 Script executed:
# Check the grpc_client and embedder modules to understand which deprecations exist
head -50 crates/weaver-embedding/src/grpc_client.rsRepository: toddwbucy/WeaverTools
Length of output: 2067
🏁 Script executed:
# Check the embedder module structure
head -50 crates/weaver-embedding/src/embedder.rsRepository: toddwbucy/WeaverTools
Length of output: 1559
🏁 Script executed:
# Search for any other deprecated attributes or API usage that might be unrelated
rg "#\[deprecated" crates/weaver-embedding/src/ --no-headingRepository: toddwbucy/WeaverTools
Length of output: 800
🏁 Script executed:
# Check if there are any deprecation warnings triggered from dependencies within the crate logic
rg "deprecated" crates/weaver-embedding/src/ -c | head -20Repository: toddwbucy/WeaverTools
Length of output: 189
🏁 Script executed:
# Let's check the actual full content of grpc_client.rs to see all the deprecated type aliases
rg "^#\[deprecated" crates/weaver-embedding/src/grpc_client.rs -A 5 | head -100Repository: toddwbucy/WeaverTools
Length of output: 1052
🏁 Script executed:
# Check if there's any actual usage of deprecated items from weaver_core or other crates within the lib.rs and related files
rg "weaver_core::" crates/weaver-embedding/src/lib.rs -A 1 -B 1Repository: toddwbucy/WeaverTools
Length of output: 494
🏁 Script executed:
# Verify whether there are any other usages of deprecated APIs from external crates in this crate
rg "deprecated" crates/weaver-embedding/ --type rust -B 3 -A 3 | grep -E "(use |from |import|::)" | head -20Repository: toddwbucy/WeaverTools
Length of output: 2275
🏁 Script executed:
# Check if workspace has clippy configuration that enforces -D warnings
find . -maxdepth 2 -name "Cargo.toml" -o -name ".cargo" -type d | head -5Repository: toddwbucy/WeaverTools
Length of output: 78
🏁 Script executed:
# Look for workspace-level clippy settings
rg "^\[lints" . --type toml -A 10Repository: toddwbucy/WeaverTools
Length of output: 47
🏁 Script executed:
# Check the actual function implementations in grpc_client.rs that use the deprecated type aliases
grep -n "EmbedResult\|EmbeddingError\|LateChunkResult" crates/weaver-embedding/src/grpc_client.rs | head -30Repository: toddwbucy/WeaverTools
Length of output: 2241
🏁 Script executed:
# Look at the full context of where these deprecated types are actually used in function signatures
rg "fn.*EmbedResult|fn.*EmbeddingError|fn.*LateChunk" crates/weaver-embedding/src/ -A 2Repository: toddwbucy/WeaverTools
Length of output: 2292
🏁 Script executed:
# Check what the current state is: if we removed the crate-wide allow, would the build fail?
# Let's look at all the deprecations to understand scope
cat crates/weaver-embedding/src/grpc_client.rs | sed -n '1,150p'Repository: toddwbucy/WeaverTools
Length of output: 5818
🏁 Script executed:
# Let's verify if there are any other deprecated APIs from dependencies being used in this crate
rg "deprecated" crates/weaver-embedding/src/ -B 5 | grep -E "(use |fn |impl |struct |enum )" | head -20Repository: toddwbucy/WeaverTools
Length of output: 47
🏁 Script executed:
# Check if the grpc_client.rs and embedder.rs modules could have their own module-level allows
head -5 crates/weaver-embedding/src/grpc_client.rs
head -5 crates/weaver-embedding/src/embedder.rsRepository: toddwbucy/WeaverTools
Length of output: 634
🏁 Script executed:
# Verify the coding guidelines requirement about -D warnings
cat Cargo.toml | grep -A 10 "\[lints\|warn\|deny"Repository: toddwbucy/WeaverTools
Length of output: 47
🏁 Script executed:
# Comprehensive check: list all items with #[deprecated] in the crate
rg "#\[deprecated" crates/weaver-embedding/src/ --no-heading -B 2 | grep -E "(pub type |pub use |pub struct |pub fn |pub enum)" | wc -lRepository: toddwbucy/WeaverTools
Length of output: 67
🏁 Script executed:
# Count all deprecated items
rg "#\[deprecated" crates/weaver-embedding/src/ | wc -lRepository: toddwbucy/WeaverTools
Length of output: 68
🏁 Script executed:
# Check if there are any other uses of deprecated APIs from external crates
rg "weaver_core::" crates/weaver-embedding/src/ | grep -v "weaver_core::embedder" | head -10Repository: toddwbucy/WeaverTools
Length of output: 47
🏁 Script executed:
# Look at the module structure to understand if module-level allows would work
tree -I "target" -L 2 crates/weaver-embedding/src/ 2>/dev/null || find crates/weaver-embedding/src/ -type f -name "*.rs"Repository: toddwbucy/WeaverTools
Length of output: 211
🏁 Script executed:
# Let's verify: are all deprecated items defined in grpc_client.rs and embedder.rs?
rg "#\[deprecated" crates/weaver-embedding/src/ --filename -B 1 | grep "^crates"Repository: toddwbucy/WeaverTools
Length of output: 500
🏁 Script executed:
# Check if there are any deprecated items used in other modules (lib.rs root, late_chunk.rs, pin.rs, gguf_backend.rs)
rg "EmbedResult|EmbeddingError|LateChunk|EmbedderInfo|Embedder" crates/weaver-embedding/src/late_chunk.rs
rg "EmbedResult|EmbeddingError|LateChunk|EmbedderInfo|Embedder" crates/weaver-embedding/src/pin.rs
rg "EmbedResult|EmbeddingError|LateChunk|EmbedderInfo|Embedder" crates/weaver-embedding/src/gguf_backend.rsRepository: toddwbucy/WeaverTools
Length of output: 1464
🏁 Script executed:
# Check if any of these modules import from grpc_client or embedder
rg "use.*grpc_client|use.*embedder" crates/weaver-embedding/src/Repository: toddwbucy/WeaverTools
Length of output: 225
Narrow the deprecation lint suppression to grpc_client and embedder modules.
#![allow(deprecated)] suppresses every deprecation warning in the crate, not just the transitional type aliases (EmbedResult, EmbeddingError, LateChunkResult, LateChunkedResult, Embedder, EmbedderInfo). While no unrelated deprecations currently exist, this scope is unnecessary and violates the workspace's -D warnings contract by allowing future API drift to slip through silently. Move the allow to module-level or item-level attributes on the deprecated type definitions in grpc_client.rs (5 aliases) and embedder.rs (2 re-exports), keeping the crate root clean for the -D warnings gate to work as intended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/weaver-embedding/src/lib.rs` around lines 37 - 46, The crate-level
allow(deprecated) is too broad—remove the top-level #![allow(deprecated)] and
instead apply narrower allow(deprecated) attributes directly to the deprecated
aliases and re-exports in the grpc_client and embedder modules: add
#[allow(deprecated)] on the five deprecated type aliases (EmbedResult,
EmbeddingError, LateChunkResult, LateChunkedResult, Embedder) in grpc_client and
on the two re-exported items (Embedder, EmbedderInfo) in embedder (or mark each
item individually with #[allow(deprecated)]), so the rest of the crate continues
to be checked under -D warnings.
Per CR finding on 60b6142: crate-level `#![allow(deprecated)]` suppresses too broadly. Replaced with module-scoped allows on the specific files that contain internal uses of the legacy aliases: - `crates/weaver-embedding/src/grpc_client.rs` — module-scoped `#![allow(deprecated)]` at the file head. This file defines AND internally uses the 5 deprecated type aliases (EmbedResult, EmbeddingError, LateChunkResult, LateChunkedResult, EmbedderInfo); scoping the allow to this single retiring module covers all the internal self-uses without affecting other modules. - `crates/weaver-embedding/tests/grpc_client.rs` — same pattern. The integration tests stay on the legacy alias path because they're testing the legacy gRPC client itself. - `crates/weaver-embedding/src/lib.rs` — `#[allow(deprecated)]` scoped to the single `pub use embedder::{Embedder, EmbedderInfo}` re-export line. The crate-level allow has been removed. Validated: - cargo clippy -p weaver-embedding --lib -- -D warnings clean - cargo test -p weaver-embedding --no-run clean (no deprecation warnings in our crate; the lone deprecation that surfaces is in weaver-inference's vram::query_gpu_vram which is pre-existing and out of scope) External consumers (anywhere outside grpc_client.rs and tests/grpc_client.rs) that reference the legacy aliases still see deprecation warnings on their own use sites — the migration signal to downstream consumers is preserved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/weaver-embedding/src/grpc_client.rs`:
- Around line 346-378: Add regression tests for ensure_ready() that exercise the
Transport downcast path: create one test where the mock gRPC server responds to
info() with a tonic::Status having code Unavailable (or
DeadlineExceeded/FailedPrecondition) and assert ensure_ready() returns
EmbeddingError::NotAvailable; create another test simulating a raw transport
failure (a tonic::transport::Error or a connection-level failure causing
EmbeddingError::Transport) and assert ensure_ready() returns
EmbeddingError::NotAvailable. Target the call flow that invokes self.info() and
the match arm handling EmbeddingError::Transport so the downcast to
tonic::Status and tonic::transport::Error is executed and verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cf63eb1-e9d4-4133-86af-464ebed9f245
📒 Files selected for processing (3)
crates/weaver-embedding/src/grpc_client.rscrates/weaver-embedding/src/lib.rscrates/weaver-embedding/tests/grpc_client.rs
…st (PR-0.5.B CR round 4) Per CR finding on commit 0a457d8: PR-0.5.B's refactor changed `ensure_ready()` from matching concrete tonic-typed EmbeddingError variants (Connection/Status) to downcasting from the abstract `EmbeddingError::Transport(Box<dyn Error>)`. That downcast logic had no test coverage. Added 5 regression tests pinning the contract: - test_ensure_ready_maps_unavailable_status_to_not_available - test_ensure_ready_maps_deadline_exceeded_status_to_not_available - test_ensure_ready_maps_failed_precondition_status_to_not_available These pin the UNAVAILABLE-class Status downcast → NotAvailable remap (the path callers pattern-match on for "embedder briefly unavailable"). Each forces info() to return the named tonic Code via a new `info_error: Option<Status>` field on the existing MockService. - test_ensure_ready_propagates_other_status_codes_as_transport Negative case: a non-UNAVAILABLE-class code (INTERNAL) must NOT get remapped to NotAvailable — it stays as Transport. Pins the discrimination logic so backend errors aren't masked. - test_ensure_ready_maps_dropped_server_to_not_available Raw `tonic::transport::Error` path: spin up the server, connect, drop the server, then call ensure_ready. Either downcast arm (Status with UNAVAILABLE-class code, OR transport::Error) must map to NotAvailable per the contract. Mock infrastructure: added `info_error: Option<Status>` field to MockService (default None for all 7 existing constructions). Validated: - cargo test -p weaver-embedding --test grpc_client test_ensure_ready → 7/7 pass (5 new + 2 existing) - cargo check -p weaver-embedding --tests clean
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced May 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Per
docs/specs/weaver-spu-Spec.md§10 PR-0.5.B. Moves the backend-agnosticEmbeddertrait + associated types toweaver-core::embedderso trait-only consumers (e.g.,weaver-database,weaver-core::surfacing) can depend onweaver-corealone, without pulling a backend-implementation crate into their dep graph.What moves
Embeddertrait +EmbedderInfo(fromweaver-embedding/src/embedder.rs)EmbedResult,LateChunkedResult,LateChunkResult,EmbeddingError(fromweaver-embedding/src/grpc_client.rs)embed_onedefault impl,health_checkdefault impl, two strict-1:1 error checks)All move to
crates/weaver-core/src/embedder.rs.Error-type abstraction
The original
EmbeddingErrorhad tonic-typed variants (Connection(tonic::transport::Error),Status(tonic::Status)). The newEmbeddingError::Transport(Box<dyn Error + Send + Sync>)variant abstracts over transport soweaver-corestays clean of gRPC specifics. TheFrom<tonic::transport::Error>andFrom<tonic::Status>impls live inweaver-core(orphan rule forces them to the trait's crate); theDisplaypreserves the upstream message via the boxed error.weaver-corepicks up atonicdep solely for theseFromimpls. Removed in PR-3.A along with the gRPC client. Tonic was already transitively present viaweaver-database.EmbeddingClient::ensure_readyupdated todowncast_refthe boxed error rather than match concrete variant names. Behavior preserved on the wire.Trait dependency direction reversed
weaver-core→weaver-embedding(forEmbeddingClientdirect use intools/memory/codebase_search.rs).weaver-embedding→weaver-core(for the relocated trait);weaver-coreno longer depends onweaver-embedding.This breaks the cycle that would otherwise form once
weaver-embeddingadds itsweaver-coredep.Refactor that breaks the prior cycle
embedder: Option<Arc<dyn weaver_core::embedder::Embedder>>field toExecutionContext. Production daemon (weaver-interface::serve) populates it at boot when the Phase 1 wiring lands (PR-1.H). Tests construct withNone.tools/memory/codebase_searchnow resolves the embedder viactx.embedder.as_ref().ok_or_else(...)instead of constructingEmbeddingClientdirectly. Tools are no longer responsible for embedder transport setup.ExecutionContextlosesderive(Debug), gains a manualDebugimpl that prints the embedder slot as a presence flag (dyn Embedderhas noDebugbound).Backward compatibility
weaver_embedding::embedder::{Embedder, EmbedderInfo}is preserved as a deprecatedpub usere-export fromweaver_core::embedder. Existing consumers compile unchanged during the migration window. Removed in PR-0.5.E.weaver_embedding::grpc_client::{EmbedResult, LateChunkedResult, LateChunkResult, EmbeddingError}re-exported from weaver-core. Same removal.Test plan
cargo build --workspacecleancargo test -p weaver-core --lib(588 passed including 5 embedder tests at new location)cargo clippy -p weaver-core --lib --tests -- -D warningscleancargo clippy -p weaver-embedding --lib -- -D warningscleancargo clippy -p weaver-spu --lib -- -D warningscleanweaver-databasedirect use ofweaver-embeddingfor the trait (it never had one — verified via grep)Pre-existing clippy errors in
weaver-inference/src/server/handler.rs(redundant_locals) are not touched by this PR — they exist on main and are out of scope for the spec's PR-0.5.B sequencing.Sequencing
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests