Bugfix/stabilize offline ci test suites - #154
Open
marlon-costa-dc wants to merge 2 commits into
Open
Conversation
…eview feedback Merge conflicts: - Resolved 12 conflicts in test files after release/v0.2.1 test reorganization Review comment fixes: - Remove hardcoded api_key from config/smoke-test.toml (gemini + qodo) - Replace eprintln! with tracing::warn! in instrumented_embedding_tests (gemini) - Fix provider assertion: expect 'fastembed' not 'openai' in arch validation (qodo) - Replace .expect() with Result propagation in test_fixtures setup (gemini) - Replace hardcoded fallback paths with std::env::temp_dir() (qodo)
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
PR Summary by QodoStabilize offline CI by making embedding-dependent tests deterministic/optional
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
36 rules 1. Dimension mismatch config
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
9
to
24
| use std::path::{Path, PathBuf}; | ||
| use std::sync::Arc; | ||
|
|
||
| use async_trait::async_trait; | ||
|
|
||
| use mcb_domain::error::Result; | ||
| use mcb_domain::ports::EmbeddingProvider; | ||
| use mcb_domain::ports::IndexingResult; | ||
| use mcb_domain::registry::database::{DatabaseProviderConfig, resolve_database_provider}; | ||
| use mcb_domain::value_objects::Embedding; | ||
| use mcb_infrastructure::config::{ConfigLoader, DatabaseConfig}; | ||
| use mcb_infrastructure::di::bootstrap::{AppContext, init_app}; | ||
| use mcb_infrastructure::di::modules::domain_services::DomainServicesFactory; | ||
| use mcb_server::McpServerBuilder; | ||
| use mcb_server::mcp_server::McpServer; | ||
| use tempfile::TempDir; |
Contributor
There was a problem hiding this comment.
1. tempfile import order wrong 📘 Rule violation ⚙ Maintainability
crates/mcb-server/tests/utils/test_fixtures.rs places the external crate import `use tempfile::TempDir; after mcb_*` imports, breaking the required grouped import ordering. This can cause inconsistent style and churn in diffs across the codebase.
Agent Prompt
## Issue description
`use` imports are not grouped/ordered as required: `tempfile` (external crate) appears after `mcb_*` imports.
## Issue Context
Compliance requires grouped and ordered Rust imports: `std`, then external crates, then `mcb_*`, then local modules.
## Fix Focus Areas
- crates/mcb-server/tests/utils/test_fixtures.rs[9-24]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+300
to
308
| pub fn shared_app_context() -> &'static AppContext { | ||
| try_shared_app_context().unwrap_or_else(|| { | ||
| tracing::error!("shared AppContext init failed"); | ||
| panic!("shared AppContext init failed"); | ||
| }) | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // create_test_mcp_server |
Contributor
There was a problem hiding this comment.
2. test_fixtures.rs too long 📘 Rule violation ⚙ Maintainability
crates/mcb-server/tests/utils/test_fixtures.rs is at least 308 lines long after this change (line numbers reach 308+), exceeding the ~200 line guideline. This makes the test fixture module harder to navigate and maintain.
Agent Prompt
## Issue description
The modified Rust source file exceeds the ~200 line limit and should be split.
## Issue Context
Large fixture modules become hard to maintain; splitting into focused submodules keeps responsibilities clearer.
## Fix Focus Areas
- crates/mcb-server/tests/utils/test_fixtures.rs[159-308]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+245
to
+276
| match init_app(config).await { | ||
| Ok(ctx) => Ok(ctx), | ||
| Err(err) => { | ||
| let msg = err.to_string(); | ||
| if msg.contains("model.onnx") | ||
| || msg.contains("Failed to initialize FastEmbed") | ||
| { | ||
| let mut fallback = ConfigLoader::new().load()?; | ||
| let fallback_db_path = std::env::temp_dir() | ||
| .join(format!("mcb-fixtures-fallback-{}.db", std::process::id())); | ||
| fallback.providers.database.configs.insert( | ||
| "default".to_owned(), | ||
| DatabaseConfig { | ||
| provider: "sqlite".to_owned(), | ||
| path: Some(fallback_db_path), | ||
| }, | ||
| ); | ||
| fallback.providers.embedding.provider = Some("openai".to_owned()); | ||
| fallback.providers.embedding.api_key = Some("test-key".to_owned()); | ||
| if let Some(cfg) = | ||
| fallback.providers.embedding.configs.get_mut("default") | ||
| { | ||
| cfg.provider = "openai".to_owned(); | ||
| cfg.model = "text-embedding-3-small".to_owned(); | ||
| cfg.api_key = Some("test-key".to_owned()); | ||
| } | ||
|
|
||
| let ctx = init_app(fallback).await?; | ||
| ctx.embedding_handle() | ||
| .set(Arc::new(DeterministicEmbeddingProvider)); | ||
| Ok(ctx) | ||
| } else { |
Contributor
There was a problem hiding this comment.
3. try_shared_app_context uses fallback config 📘 Rule violation ☼ Reliability
When init_app(config) fails (e.g., missing FastEmbed model), try_shared_app_context() substitutes a fallback configuration and overrides the embedding provider with a deterministic implementation instead of failing initialization. This violates the requirement to fail fast when required external configuration/resources are missing at startup.
Agent Prompt
## Issue description
Startup/config initialization falls back to an alternate configuration when required resources are missing, instead of failing fast.
## Issue Context
Compliance requires explicit startup failure when required external configuration or resources are unavailable; fallback behavior should be avoided or explicitly gated by a documented test-only flag.
## Fix Focus Areas
- crates/mcb-server/tests/utils/test_fixtures.rs[245-276]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+14
to
+21
| fn configure_offline_embedding(config: &mut mcb_infrastructure::config::AppConfig) { | ||
| config.providers.embedding.provider = Some("openai".to_owned()); | ||
| config.providers.embedding.api_key = Some("test-key".to_owned()); | ||
| if let Some(default_cfg) = config.providers.embedding.configs.get_mut("default") { | ||
| default_cfg.provider = "openai".to_owned(); | ||
| default_cfg.model = "text-embedding-3-small".to_owned(); | ||
| default_cfg.api_key = Some("test-key".to_owned()); | ||
| } |
Contributor
There was a problem hiding this comment.
4. Duplicated embedding config literals 📘 Rule violation ⚙ Maintainability
The PR repeats configuration-like literals (e.g., openai, text-embedding-3-small, test-key) directly in multiple files instead of defining a shared constant/helper/config source. This increases drift risk when the offline test embedding configuration changes again.
Agent Prompt
## Issue description
Embedding-provider configuration literals are duplicated across multiple changed files.
## Issue Context
Compliance requires centralizing configuration-like values and reusing shared definitions instead of duplicating literals.
## Fix Focus Areas
- crates/mcb-infrastructure/tests/integration/di/catalog_tests.rs[14-21]
- crates/mcb-server/tests/utils/test_fixtures.rs[262-270]
- config/smoke-test.toml[14-20]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+283
to
+288
| match result { | ||
| Ok(ctx) => Some(ctx), | ||
| Err(err) => { | ||
| tracing::warn!("shared init_app failed: {err}"); | ||
| None | ||
| } |
Contributor
There was a problem hiding this comment.
5. Init errors silently skipped 🐞 Bug ☼ Reliability
In crates/mcb-server/tests/utils/test_fixtures.rs, try_shared_app_context turns any init_app error into None, so tests treating None as “skip” can pass even when bootstrap is genuinely broken (bad config/DB/provider resolution). This can make CI green while core initialization regresses.
Agent Prompt
### Issue description
`try_shared_app_context()` currently logs and returns `None` for *any* `init_app` failure. Callers then return early and the test passes, masking genuine regressions.
### Issue Context
Other test suites use `define_shared_test_context!`, which only returns `None` for the known “FastEmbed model missing” case and otherwise panics.
### Fix Focus Areas
- Update error handling to only return `None` for explicitly recognized “FastEmbed unavailable” (or other intentional skip) errors; for all other errors, fail fast (panic or propagate an error so tests fail).
- crates/mcb-server/tests/utils/test_fixtures.rs[283-289]
- crates/mcb-infrastructure/src/macros.rs[132-143]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+219
to
+290
| CTX.get_or_init(|| { | ||
| std::thread::spawn(|| -> Option<AppContext> { | ||
| let rt = match tokio::runtime::Runtime::new() { | ||
| Ok(rt) => rt, | ||
| Err(err) => { | ||
| tracing::warn!("failed to create runtime for shared app context: {err}"); | ||
| return None; | ||
| } | ||
| }; | ||
| let result = rt.block_on(async { | ||
| let temp_dir = tempfile::tempdir().map_err(|err| { | ||
| mcb_domain::error::Error::config(format!("create temp dir: {err}")) | ||
| })?; | ||
| let temp_root = temp_dir.keep(); | ||
| let temp_path = temp_root.join("mcb-fixtures-shared.db"); | ||
|
|
||
| let mut config = ConfigLoader::new().load()?; | ||
| config.providers.database.configs.insert( | ||
| "default".to_owned(), | ||
| DatabaseConfig { | ||
| provider: "sqlite".to_owned(), | ||
| path: Some(temp_path), | ||
| }, | ||
| ); | ||
| config.providers.embedding.cache_dir = Some(shared_fastembed_test_cache_dir()); | ||
|
|
||
| match init_app(config).await { | ||
| Ok(ctx) => Ok(ctx), | ||
| Err(err) => { | ||
| let msg = err.to_string(); | ||
| if msg.contains("model.onnx") | ||
| || msg.contains("Failed to initialize FastEmbed") | ||
| { | ||
| let mut fallback = ConfigLoader::new().load()?; | ||
| let fallback_db_path = std::env::temp_dir() | ||
| .join(format!("mcb-fixtures-fallback-{}.db", std::process::id())); | ||
| fallback.providers.database.configs.insert( | ||
| "default".to_owned(), | ||
| DatabaseConfig { | ||
| provider: "sqlite".to_owned(), | ||
| path: Some(fallback_db_path), | ||
| }, | ||
| ); | ||
| fallback.providers.embedding.provider = Some("openai".to_owned()); | ||
| fallback.providers.embedding.api_key = Some("test-key".to_owned()); | ||
| if let Some(cfg) = | ||
| fallback.providers.embedding.configs.get_mut("default") | ||
| { | ||
| cfg.provider = "openai".to_owned(); | ||
| cfg.model = "text-embedding-3-small".to_owned(); | ||
| cfg.api_key = Some("test-key".to_owned()); | ||
| } | ||
|
|
||
| let ctx = init_app(fallback).await?; | ||
| ctx.embedding_handle() | ||
| .set(Arc::new(DeterministicEmbeddingProvider)); | ||
| Ok(ctx) | ||
| } else { | ||
| Err(err) | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| match result { | ||
| Ok(ctx) => Some(ctx), | ||
| Err(err) => { | ||
| tracing::warn!("shared init_app failed: {err}"); | ||
| None | ||
| } | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
6. Provider actors lose runtime 🐞 Bug ☼ Reliability
try_shared_app_context initializes AppContext inside a temporary Tokio runtime and drops that runtime when the init thread exits; FastEmbed and EdgeVec providers spawn long-lived actor tasks via tokio::spawn during construction, so dropping the runtime aborts those tasks and can break provider behavior later in the test process.
Agent Prompt
### Issue description
`try_shared_app_context()` creates providers on a dedicated Tokio runtime and then drops that runtime at the end of initialization. Providers like FastEmbed and EdgeVec spawn actor tasks with `tokio::spawn` during construction; those tasks are tied to the runtime and are aborted when the runtime is dropped.
### Issue Context
The shared test-context macro intentionally prevents dropping the init runtime (via `ManuallyDrop`) to keep spawned tasks alive for the process lifetime.
### Fix Focus Areas
- Retain the initialization runtime for the lifetime of the static `AppContext` (e.g., leak it via `ManuallyDrop` like the macro, or store it in a static alongside the context).
- Prefer reusing `mcb_infrastructure::define_shared_test_context!` + a deterministic-fallback layer rather than re-implementing the shared-context logic.
- crates/mcb-server/tests/utils/test_fixtures.rs[219-290]
- crates/mcb-providers/src/embedding/fastembed.rs[66-80]
- crates/mcb-providers/src/vector_store/edgevec.rs[219-233]
- crates/mcb-infrastructure/src/macros.rs[113-131]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+21
to
+25
| let Some(ctx) = try_shared_app_context() else { | ||
| return Err(Error::embedding( | ||
| "Shared AppContext unavailable — FastEmbed model may be missing", | ||
| )); | ||
| }; |
Contributor
There was a problem hiding this comment.
1. Wrong error variant 🐞 Bug ≡ Correctness
create_real_vector_store() returns Error::embedding when the shared AppContext is unavailable, even though the failing component is vector-store access; this misclassifies failures and makes diagnostics misleading. The error message also incorrectly attributes the failure to FastEmbed.
Agent Prompt
### Issue description
`create_real_vector_store()` reports `Error::embedding(...)` when `try_shared_app_context()` is `None`, even though the function is retrieving a `VectorStoreProvider`. This mislabels the failure and produces misleading messages.
### Issue Context
This is a test utility; callers may log or branch on error variants.
### Fix Focus Areas
- crates/mcb-server/tests/utils/real_providers.rs[19-27]
- crates/mcb-domain/src/error/types.rs[72-85]
### Suggested change
- Replace `Error::embedding("Shared AppContext unavailable — FastEmbed model may be missing")` with a more accurate variant/message, e.g. `Error::vector_db("Shared AppContext unavailable; cannot create vector store")` (or `Error::config(...)` / `Error::internal(...)` depending on intended semantics).
- Update the message to not mention FastEmbed for the vector-store path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+14
to
22
| [providers.embedding] | ||
| provider = "openai" | ||
|
|
||
| [providers.embedding.configs.default] | ||
| provider = "fastembed" | ||
| model = "AllMiniLML6V2" | ||
| provider = "openai" | ||
| model = "text-embedding-3-small" | ||
|
|
||
| [providers.vector_store.configs.default] | ||
| provider = "edgevec" |
Contributor
There was a problem hiding this comment.
2. Dimension mismatch config 🐞 Bug ≡ Correctness
config/smoke-test.toml switches embeddings to OpenAI text-embedding-3-small (1536 dimensions) but leaves EdgeVec dimensions unspecified, which defaults to 384; any vector insertion/indexing will then fail at runtime. This makes the smoke-test configuration internally inconsistent and fragile for any end-to-end flow that actually embeds and stores vectors.
Agent Prompt
### Issue description
Smoke-test/offline config selects OpenAI `text-embedding-3-small` embeddings (1536-d), but the EdgeVec vector store config does not set `dimensions`, and the provider factory defaults omitted dimensions to `384`. This creates a guaranteed dimension mismatch once embeddings are inserted into EdgeVec.
### Issue Context
- OpenAI `text-embedding-3-small` is 1536 dimensions.
- EdgeVec index is constructed with its configured `dimensions` and inserts raw embedding vectors.
### Fix Focus Areas
- config/smoke-test.toml[14-23]
- crates/mcb-providers/src/vector_store/edgevec.rs[832-843]
### Suggested change
- In `config/smoke-test.toml`, set:
- `[providers.vector_store.configs.default] dimensions = 1536` (to match `text-embedding-3-small`).
- Also audit the other test-only OpenAI configurations introduced in this PR (e.g., `configure_offline_embedding(...)` and dispatch tests) and ensure their vector store dimensions are updated to 1536 when OpenAI is selected, or switch those scenarios to a 384-d embedding provider if EdgeVec is kept at 384.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
Code review by qodo was updated up to the latest commit a1aeead |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 16 files
Confidence score: 2/5
Cargo.tomlswitches FastEmbed toort-load-dynamic, which can panic on clean CI and standalone installs when ONNX Runtime is missing, creating a concrete runtime break for embedding initialization—either package the shared library with releases or revert to a loading mode that works out of the box.crates/mcb-infrastructure/tests/integration/di/dispatch_tests.rsandcrates/mcb-server/tests/utils/test_fixtures.rsnow make provider assertions brittle and can treat real initialization failures as skips, so regressions in config/DI/provider wiring may pass unnoticed—tighten the assertions and only skip on explicit, narrow offline conditions.crates/mcb-server/tests/utils/test_fixtures.rsreports the fallback as FastEmbed, so tests intended to validate the real ONNX path can accidentally exercise the stub and still appear correct—give fallback and real providers distinct identities and assert on them directly.crates/mcb-infrastructure/tests/unit/util_tests/file_hash_tests.rs,crates/mcb-infrastructure/tests/integration/di/catalog_tests.rs, andconfig/smoke-test.tomlreduce coverage signal (unnecessary offline skips, duplicated catalog behavior, ignored model setting), which increases the chance of config regressions slipping through—decouple non-embedding tests from AppContext embedding init and restore a true custom-config path in smoke/integration tests.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="config/smoke-test.toml">
<violation number="1" location="config/smoke-test.toml:15">
P3: The configured `text-embedding-3-small` value is ignored because setting the direct `providers.embedding.provider` makes the resolver bypass `configs.default`. Keeping `model` in the direct section would make this smoke configuration effective rather than relying on OpenAI's coincidentally matching factory default.</violation>
</file>
<file name="Cargo.toml">
<violation number="1" location="Cargo.toml:253">
P0: FastEmbed initialization now panics on clean CI hosts and standalone release installs because `ort-load-dynamic` requires an external ONNX Runtime shared library that is neither installed nor packaged. Keeping the prior download-binaries backend, or adding explicit cross-platform dylib installation/packaging and `ORT_DYLIB_PATH`, preserves the default local embedding provider.</violation>
</file>
<file name="crates/mcb-infrastructure/tests/integration/di/catalog_tests.rs">
<violation number="1" location="crates/mcb-infrastructure/tests/integration/di/catalog_tests.rs:29">
P3: `test_catalog_builds_with_custom_embedding_config` is now behaviorally identical to `test_catalog_builds_with_default_config`, so the custom-config coverage has been lost while the suite still appears to test it. Consider giving this case a distinct override (and asserting the selected provider/config), or removing/renaming the duplicate test.</violation>
</file>
<file name="crates/mcb-infrastructure/tests/integration/di/dispatch_tests.rs">
<violation number="1" location="crates/mcb-infrastructure/tests/integration/di/dispatch_tests.rs:68">
P1: This test still resolves FastEmbed, so the new `"openai"` assertion fails whenever initialization succeeds (and offline initialization may fail before the assertion). `test_config()` retains the direct `providers.embedding.provider = "fastembed"` value from `config/default.toml`, and the resolver gives that field precedence over `configs["default"]`. The test setup should also set the direct provider and API key to OpenAI, as the catalog tests now do, or clear the direct provider so the named config is exercised.</violation>
</file>
<file name="crates/mcb-infrastructure/tests/unit/util_tests/file_hash_tests.rs">
<violation number="1" location="crates/mcb-infrastructure/tests/unit/util_tests/file_hash_tests.rs:26">
P2: Offline CI now silently skips the file-hash repository tests even though these cases do not need embeddings. Their fixture couples `FileHashRepository` to an AppContext whose creation can return `None` solely because the ONNX model is unavailable, and each test then returns as a passing test. A SQLite-backed file-hash fixture initialized independently of FastEmbed would retain this coverage in the offline suite; only the final full indexing test needs the complete context.</violation>
</file>
<file name="crates/mcb-server/tests/utils/test_fixtures.rs">
<violation number="1" location="crates/mcb-server/tests/utils/test_fixtures.rs:193">
P2: The fallback identifies itself as FastEmbed, so `create_real_embedding_provider` and the real-provider tests cannot distinguish the length-based stub from the actual ONNX provider. In an offline run, tests named and documented as exercising real FastEmbed therefore report success without loading or invoking FastEmbed. Giving the fallback its own provider name (and having real-provider fixtures reject/skip that name) would keep generic server tests offline-capable without producing a false real-provider signal.</violation>
<violation number="2" location="crates/mcb-server/tests/utils/test_fixtures.rs:223">
P1: Unexpected AppContext initialization regressions are now converted into a successful skip in the real-provider tests. The `result` can contain config, SQLite, DI, fallback-provider, and other failures, but this branch collapses all of them to `None`, which `real_providers.rs` treats as “provider unavailable.” Since the known missing-model case is already handled by the deterministic fallback above, unexpected errors here should remain test failures rather than being silently cached in the `OnceLock`.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| "default", | ||
| "hf-hub-native-tls", | ||
| "image-models", | ||
| "ort-load-dynamic", |
There was a problem hiding this comment.
P0: FastEmbed initialization now panics on clean CI hosts and standalone release installs because ort-load-dynamic requires an external ONNX Runtime shared library that is neither installed nor packaged. Keeping the prior download-binaries backend, or adding explicit cross-platform dylib installation/packaging and ORT_DYLIB_PATH, preserves the default local embedding provider.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Cargo.toml, line 253:
<comment>FastEmbed initialization now panics on clean CI hosts and standalone release installs because `ort-load-dynamic` requires an external ONNX Runtime shared library that is neither installed nor packaged. Keeping the prior download-binaries backend, or adding explicit cross-platform dylib installation/packaging and `ORT_DYLIB_PATH`, preserves the default local embedding provider.</comment>
<file context>
@@ -248,7 +248,9 @@ async-nats = "0.46"
- "default",
+ "hf-hub-native-tls",
+ "image-models",
+ "ort-load-dynamic",
] }
</file context>
Suggested change
| "ort-load-dynamic", | |
| "ort-download-binaries-native-tls", |
| provider: "fastembed".to_owned(), | ||
| model: "test".to_owned(), | ||
| api_key: None, | ||
| provider: "openai".to_owned(), |
There was a problem hiding this comment.
P1: This test still resolves FastEmbed, so the new "openai" assertion fails whenever initialization succeeds (and offline initialization may fail before the assertion). test_config() retains the direct providers.embedding.provider = "fastembed" value from config/default.toml, and the resolver gives that field precedence over configs["default"]. The test setup should also set the direct provider and API key to OpenAI, as the catalog tests now do, or clear the direct provider so the named config is exercised.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-infrastructure/tests/integration/di/dispatch_tests.rs, line 68:
<comment>This test still resolves FastEmbed, so the new `"openai"` assertion fails whenever initialization succeeds (and offline initialization may fail before the assertion). `test_config()` retains the direct `providers.embedding.provider = "fastembed"` value from `config/default.toml`, and the resolver gives that field precedence over `configs["default"]`. The test setup should also set the direct provider and API key to OpenAI, as the catalog tests now do, or clear the direct provider so the named config is exercised.</comment>
<file context>
@@ -65,9 +65,9 @@ async fn test_provider_selection_from_config() -> Result<(), Box<dyn std::error:
- provider: "fastembed".to_owned(),
- model: "test".to_owned(),
- api_key: None,
+ provider: "openai".to_owned(),
+ model: "text-embedding-3-small".to_owned(),
+ api_key: Some("test-key".to_owned()),
</file context>
| std::thread::spawn(|| -> Option<AppContext> { | ||
| let rt = match tokio::runtime::Runtime::new() { | ||
| Ok(rt) => rt, | ||
| Err(err) => { |
There was a problem hiding this comment.
P1: Unexpected AppContext initialization regressions are now converted into a successful skip in the real-provider tests. The result can contain config, SQLite, DI, fallback-provider, and other failures, but this branch collapses all of them to None, which real_providers.rs treats as “provider unavailable.” Since the known missing-model case is already handled by the deterministic fallback above, unexpected errors here should remain test failures rather than being silently cached in the OnceLock.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/tests/utils/test_fixtures.rs, line 223:
<comment>Unexpected AppContext initialization regressions are now converted into a successful skip in the real-provider tests. The `result` can contain config, SQLite, DI, fallback-provider, and other failures, but this branch collapses all of them to `None`, which `real_providers.rs` treats as “provider unavailable.” Since the known missing-model case is already handled by the deterministic fallback above, unexpected errors here should remain test failures rather than being silently cached in the `OnceLock`.</comment>
<file context>
@@ -150,10 +157,152 @@ pub fn create_test_indexing_result(
+ std::thread::spawn(|| -> Option<AppContext> {
+ let rt = match tokio::runtime::Runtime::new() {
+ Ok(rt) => rt,
+ Err(err) => {
+ tracing::warn!("failed to create runtime for shared app context: {err}");
+ return None;
</file context>
| async fn test_has_changed(file_hash_repo: Arc<dyn FileHashRepository>) { | ||
| let repo = file_hash_repo; | ||
| async fn test_has_changed(file_hash_repo: Option<Arc<dyn FileHashRepository>>) { | ||
| let Some(repo) = file_hash_repo else { |
There was a problem hiding this comment.
P2: Offline CI now silently skips the file-hash repository tests even though these cases do not need embeddings. Their fixture couples FileHashRepository to an AppContext whose creation can return None solely because the ONNX model is unavailable, and each test then returns as a passing test. A SQLite-backed file-hash fixture initialized independently of FastEmbed would retain this coverage in the offline suite; only the final full indexing test needs the complete context.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-infrastructure/tests/unit/util_tests/file_hash_tests.rs, line 26:
<comment>Offline CI now silently skips the file-hash repository tests even though these cases do not need embeddings. Their fixture couples `FileHashRepository` to an AppContext whose creation can return `None` solely because the ONNX model is unavailable, and each test then returns as a passing test. A SQLite-backed file-hash fixture initialized independently of FastEmbed would retain this coverage in the offline suite; only the final full indexing test needs the complete context.</comment>
<file context>
@@ -9,21 +9,24 @@ use std::time::Duration;
-async fn test_has_changed(file_hash_repo: Arc<dyn FileHashRepository>) {
- let repo = file_hash_repo;
+async fn test_has_changed(file_hash_repo: Option<Arc<dyn FileHashRepository>>) {
+ let Some(repo) = file_hash_repo else {
+ eprintln!("skipping: shared AppContext unavailable (FastEmbed model missing)");
+ return;
</file context>
| } | ||
|
|
||
| fn provider_name(&self) -> &str { | ||
| "fastembed" |
There was a problem hiding this comment.
P2: The fallback identifies itself as FastEmbed, so create_real_embedding_provider and the real-provider tests cannot distinguish the length-based stub from the actual ONNX provider. In an offline run, tests named and documented as exercising real FastEmbed therefore report success without loading or invoking FastEmbed. Giving the fallback its own provider name (and having real-provider fixtures reject/skip that name) would keep generic server tests offline-capable without producing a false real-provider signal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/tests/utils/test_fixtures.rs, line 193:
<comment>The fallback identifies itself as FastEmbed, so `create_real_embedding_provider` and the real-provider tests cannot distinguish the length-based stub from the actual ONNX provider. In an offline run, tests named and documented as exercising real FastEmbed therefore report success without loading or invoking FastEmbed. Giving the fallback its own provider name (and having real-provider fixtures reject/skip that name) would keep generic server tests offline-capable without producing a false real-provider signal.</comment>
<file context>
@@ -150,10 +157,152 @@ pub fn create_test_indexing_result(
+ }
+
+ fn provider_name(&self) -> &str {
+ "fastembed"
+ }
+}
</file context>
| port = 18080 | ||
|
|
||
| [providers.embedding] | ||
| provider = "openai" |
There was a problem hiding this comment.
P3: The configured text-embedding-3-small value is ignored because setting the direct providers.embedding.provider makes the resolver bypass configs.default. Keeping model in the direct section would make this smoke configuration effective rather than relying on OpenAI's coincidentally matching factory default.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/smoke-test.toml, line 15:
<comment>The configured `text-embedding-3-small` value is ignored because setting the direct `providers.embedding.provider` makes the resolver bypass `configs.default`. Keeping `model` in the direct section would make this smoke configuration effective rather than relying on OpenAI's coincidentally matching factory default.</comment>
<file context>
@@ -11,9 +11,12 @@ transport_mode = "stdio"
port = 18080
+[providers.embedding]
+provider = "openai"
+
[providers.embedding.configs.default]
</file context>
Suggested change
| provider = "openai" | |
| provider = "openai" | |
| model = "text-embedding-3-small" |
| async fn test_catalog_builds_with_default_config() { | ||
| let config = ConfigLoader::new().load().expect("load config"); | ||
| let mut config = ConfigLoader::new().load().expect("load config"); | ||
| configure_offline_embedding(&mut config); |
There was a problem hiding this comment.
P3: test_catalog_builds_with_custom_embedding_config is now behaviorally identical to test_catalog_builds_with_default_config, so the custom-config coverage has been lost while the suite still appears to test it. Consider giving this case a distinct override (and asserting the selected provider/config), or removing/renaming the duplicate test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-infrastructure/tests/integration/di/catalog_tests.rs, line 29:
<comment>`test_catalog_builds_with_custom_embedding_config` is now behaviorally identical to `test_catalog_builds_with_default_config`, so the custom-config coverage has been lost while the suite still appears to test it. Consider giving this case a distinct override (and asserting the selected provider/config), or removing/renaming the duplicate test.</comment>
<file context>
@@ -11,11 +11,22 @@ use serial_test::serial;
async fn test_catalog_builds_with_default_config() {
- let config = ConfigLoader::new().load().expect("load config");
+ let mut config = ConfigLoader::new().load().expect("load config");
+ configure_offline_embedding(&mut config);
let result = build_catalog(config).await;
</file context>
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.
No description provided.