Feature/phase2 di centralization - #149
Open
marlon-costa-dc wants to merge 3 commits into
Open
Conversation
- Remove reqwest dependency from Cargo.lock and related files. - Implement FileSystemProvider trait for file operations. - Add TaskRunnerProvider trait for background task management. - Update IndexingServiceImpl to utilize new providers for file system access and task execution. - Refactor indexing logic to improve directory traversal and error handling. - Introduce new event bus and file system provider configurations in the DI layer.
- Replace `println!` with `writeln!` to ensure proper output flushing. - Update the `print_json` and `print_summary` methods for consistent output formatting. - Enhance readability by restructuring output statements in the validation report.
- Removed mcb-providers from Cargo.lock and related files to streamline dependencies. - Introduced new bypass rules in mcb-validate-internal.toml for handling provider imports across various crates. - Updated tests and code to ensure compliance with the new dependency structure and validation rules.
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: 59 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 (70)
✨ 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 QodoCentralize DI via domain provider ports (FS, task runner, event bus, DB, VCS)
AI Description
Diagram
High-Level Assessment
Files changed (70)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
36 rules 1. Wrong VCS repository_id
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| @@ -0,0 +1,15 @@ | |||
| #![allow(missing_docs)] | |||
| #![allow(unsafe_code)] | |||
Contributor
There was a problem hiding this comment.
1. #![allow(unsafe_code)] added 📘 Rule violation ⛨ Security
The PR introduces crate-level unsafe_code suppression, weakening enforcement of the no-unsafe policy and potentially permitting unsafe expansions without review. This violates the requirement to disallow unsafe code and keep unsafe_code denied.
Agent Prompt
## Issue description
`#![allow(unsafe_code)]` was added, which disables the project’s no-unsafe enforcement for that module.
## Issue Context
This rule requires that `unsafe` is not permitted and that the `unsafe_code` deny policy remains effective.
## Fix Focus Areas
- crates/mcb-domain/src/registry/project_detection.rs[1-3]
- crates/mcb-infrastructure/tests/unit/config/config_figment_tests.rs[1-3]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } | ||
|
|
||
| /// `IndexingServiceDeps` struct. | ||
| #[allow(missing_docs)] |
Contributor
There was a problem hiding this comment.
2. Unjustified #[allow(...)] suppressions 📘 Rule violation ⚙ Maintainability
Multiple #[allow(...)] directives were added without an inline justification comment on the same line or immediately following line. This reduces auditability and makes it unclear why suppressions are safe/necessary.
Agent Prompt
## Issue description
New static-analysis suppression directives were added without required inline justification.
## Issue Context
The compliance rule requires a specific justification comment on the same line or the immediately following line for each suppression.
## Fix Focus Areas
- crates/mcb-application/src/use_cases/indexing_service.rs[111-114]
- crates/mcb-infrastructure/src/di/test_factory.rs[34-41]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+170
to
181
| let _ = writeln!( | ||
| std::io::stdout(), | ||
| "[{}] {}: {} ({}:{})", | ||
| violation.severity, violation.id, violation.message, file_display, line | ||
| violation.severity, | ||
| violation.id, | ||
| violation.message, | ||
| file_display, | ||
| line | ||
| ); | ||
| if let Some(ref suggestion) = violation.suggestion { | ||
| println!(" → {suggestion}"); | ||
| let _ = writeln!(std::io::stdout(), " → {suggestion}"); | ||
| } |
Contributor
There was a problem hiding this comment.
3. Ignored writeln! results 📘 Rule violation ☼ Reliability
The PR adds let _ = writeln!(...) calls that discard I/O errors when printing validation output. This can silently hide failures (e.g., broken pipe) and violates the requirement to propagate, log, or explicitly justify ignoring errors.
Agent Prompt
## Issue description
Several `writeln!` results are discarded via `let _ = ...`, silently swallowing potential output errors.
## Issue Context
The compliance rule requires errors to be propagated, logged, or explicitly handled with justification.
## Fix Focus Areas
- crates/mcb/src/cli/validate.rs[140-199]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+199
to
+220
| let mut dirs = vec![path.to_path_buf()]; | ||
|
|
||
| while let Some(dir) = dirs.pop() { | ||
| match self.file_system_provider.read_dir_entries(&dir).await { | ||
| Ok(entries) => { | ||
| for entry in entries { | ||
| if entry.is_dir { | ||
| let should_skip = entry | ||
| .path | ||
| .file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .is_some_and(|name| SKIP_DIRS.contains(&name)); | ||
|
|
||
| if !should_skip { | ||
| dirs.push(entry.path); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if entry.is_file && self.is_supported_file(&entry.path) { | ||
| files.push(entry.path); | ||
| } |
Contributor
There was a problem hiding this comment.
4. Ignore rules not applied 🐞 Bug ≡ Correctness
IndexingServiceImpl::discover_files no longer uses an ignore-aware walker and now only skips a hard-coded SKIP_DIRS list, so files excluded by .gitignore/.ignore may be indexed (changed results + potentially large perf regression). This contradicts the module’s stated “respecting ignore patterns” behavior.
Agent Prompt
### Issue description
`IndexingServiceImpl::discover_files()` switched from an ignore-aware walker to a raw directory traversal, but the new implementation does not apply `.gitignore`/`.ignore` (or similar) rules. This can cause the indexer to ingest files that were previously ignored, changing indexing results and potentially dramatically increasing the amount of work.
### Issue Context
The module documentation claims file discovery respects ignore patterns, but the current implementation only filters directory names via `SKIP_DIRS`.
### Fix Focus Areas
- crates/mcb-application/src/use_cases/indexing_service.rs[11-16]
- crates/mcb-application/src/use_cases/indexing_service.rs[192-230]
- crates/mcb-application/src/constants.rs[22-24]
### Suggested fix approach
- Re-introduce ignore-aware traversal semantics (equivalent to the prior `ignore::WalkBuilder` behavior) while keeping DI goals:
- Option A (minimal): use `ignore::WalkBuilder` again inside `discover_files()` and keep `FileSystemProvider` only for file reads.
- Option B (more architectural): extend the filesystem port to provide an ignore-aware walk/list API (or ignore-matching helper) and implement it in the local provider.
- Add a unit/integration test that asserts files under an ignored path (e.g. a temp dir with `.gitignore` ignoring `target/` or `dist/`) are not discovered.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+301
to
+307
| let task = Box::pin(async move { | ||
| Self::run_indexing_task(service, files, workspace_root, collection_id, op_id).await; | ||
| }); | ||
|
|
||
| if let Err(e) = self.task_runner_provider.spawn(task) { | ||
| warn!("Failed to spawn indexing background task: {}", e); | ||
| } |
Contributor
There was a problem hiding this comment.
5. Spawn failure stalls indexing 🐞 Bug ☼ Reliability
IndexingServiceImpl::index_codebase returns STARTED even when TaskRunnerProvider::spawn fails; in that case the background task never runs, so the operation is never completed and polling clients can wait forever. The only spawn failure handling is a warn log.
Agent Prompt
### Issue description
`index_codebase()` creates an operation and returns `INDEXING_STATUS_STARTED` even if spawning the background task fails. Since completion happens only inside `run_indexing_task()`, a spawn failure leaves the operation permanently active.
### Issue Context
This regression is introduced by replacing infallible `tokio::spawn` with a fallible `TaskRunnerProvider::spawn` and not translating spawn failure into a terminal operation state.
### Fix Focus Areas
- crates/mcb-application/src/use_cases/indexing_service.rs[277-318]
- crates/mcb-application/src/use_cases/indexing_service.rs[345-395]
### Suggested fix approach
- If `spawn()` returns an error:
- Immediately finalize/cleanup the operation (e.g., call `indexing_ops.complete_operation(&operation_id)` or a new `fail_operation(...)` API if you have one).
- Return an error from `index_codebase()` so callers don’t receive an operation_id that will never progress.
- Optionally publish a failure/completed-with-error event (if your domain events model supports it).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
81
to
84
| pub fn default_event_bus() -> Arc<dyn EventBusProvider> { | ||
| Arc::new(mcb_providers::events::TokioEventBusProvider::new()) | ||
| resolve_event_bus_provider(&EventBusProviderConfig::new("tokio")) | ||
| .unwrap_or_else(|_| Arc::new(NullEventBusProvider)) | ||
| } |
Contributor
There was a problem hiding this comment.
6. Null event bus fallback 🐞 Bug ☼ Reliability
default_event_bus() silently falls back to a no-op NullEventBusProvider when registry resolution fails, so admin/server flows can drop all events without any hard failure. This is particularly risky now that mcb-server no longer depends on mcb-providers and default admin state uses default_event_bus without calling init_app().
Agent Prompt
### Issue description
`default_event_bus()` converts provider-resolution failure into a successful no-op event bus. This can hide missing linkme registration / provider-linking problems and causes events to be dropped silently.
### Issue Context
- `init_app()` explicitly calls `provider_linker::ensure_linked()`, but callers that bypass `init_app()` (e.g., server admin default state) still use `default_event_bus()`.
- `mcb-server` no longer has a direct dependency on `mcb-providers`, increasing the likelihood that linkme registrations aren’t present unless the linker hook is referenced.
### Fix Focus Areas
- crates/mcb-infrastructure/src/infrastructure/factory.rs[75-85]
- crates/mcb-server/src/admin/web/router.rs[25-36]
- crates/mcb-infrastructure/src/di/bootstrap.rs[209-217]
- crates/mcb-infrastructure/src/provider_linker.rs[1-4]
- crates/mcb-server/Cargo.toml[118-131]
### Suggested fix approach
- Prefer fail-fast over silent fallback:
- Change `default_event_bus()` to return `Result<Arc<dyn EventBusProvider>>` and make callers handle failure explicitly, OR
- At minimum, emit an `error!`/`warn!` that clearly indicates events will be dropped.
- Ensure providers are linked for this path:
- Call `crate::provider_linker::ensure_linked()` inside `default_event_bus()` before resolving.
- Only use a NullEventBusProvider when explicitly requested (e.g., config setting), not as an error fallback.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+23
to
+32
| fn resolve_default_database_provider() -> Result<Arc<dyn DatabaseProvider>> { | ||
| let provider_name = list_database_providers() | ||
| .first() | ||
| .map(|(name, _)| *name) | ||
| .ok_or_else(|| { | ||
| Error::configuration("Database: no providers registered in linkme registry") | ||
| })?; | ||
|
|
||
| resolve_database_provider(&DatabaseProviderConfig::new(provider_name)) | ||
| } |
Contributor
There was a problem hiding this comment.
7. Unstable default db provider 🐞 Bug ☼ Reliability
Infrastructure helper resolve_default_database_provider() selects the first provider returned by list_database_providers(), but the registry list preserves linkme slice iteration order and does not define a default selection policy. Adding another database provider can unpredictably change which backend these helpers use.
Agent Prompt
### Issue description
`resolve_default_database_provider()` chooses `.first()` from the registered database providers list, which is not an explicit default policy and can change based on registration/link order.
### Issue Context
This affects the infrastructure helper constructors in `di/repositories.rs` (not the main `init_app()` path, which resolves from configured provider).
### Fix Focus Areas
- crates/mcb-infrastructure/src/di/repositories.rs[23-37]
- crates/mcb-domain/src/macros/registry.rs[64-67]
### Suggested fix approach
- Define a deterministic selection policy:
- Prefer an explicit known name (e.g. try `"sqlite"` first, else error), OR
- Accept a provider name (or config) parameter in these helper functions and resolve that, OR
- Fail with a clear configuration error when multiple providers are registered and no explicit provider is specified.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+68
to
+91
| async fn list_branches(&self, repo: &VcsRepository) -> Result<Vec<VcsBranch>> { | ||
| let (provider, concrete_repo) = self.provider_and_repo_for_path(repo.path()).await?; | ||
| provider.list_branches(&concrete_repo).await | ||
| } | ||
|
|
||
| async fn commit_history( | ||
| &self, | ||
| repo: &VcsRepository, | ||
| branch: &str, | ||
| limit: Option<usize>, | ||
| ) -> Result<Vec<VcsCommit>> { | ||
| let (provider, concrete_repo) = self.provider_and_repo_for_path(repo.path()).await?; | ||
| provider.commit_history(&concrete_repo, branch, limit).await | ||
| } | ||
|
|
||
| async fn list_files(&self, repo: &VcsRepository, branch: &str) -> Result<Vec<PathBuf>> { | ||
| let (provider, concrete_repo) = self.provider_and_repo_for_path(repo.path()).await?; | ||
| provider.list_files(&concrete_repo, branch).await | ||
| } | ||
|
|
||
| async fn read_file(&self, repo: &VcsRepository, branch: &str, path: &Path) -> Result<String> { | ||
| let (provider, concrete_repo) = self.provider_and_repo_for_path(repo.path()).await?; | ||
| provider.read_file(&concrete_repo, branch, path).await | ||
| } |
Contributor
There was a problem hiding this comment.
8. Vcs repo reopened per call 🐞 Bug ➹ Performance
DynamicVcsProvider re-runs provider detection and re-opens the repository for operations like list_files/read_file/diff_refs, even when given an already-opened VcsRepository. For Git2Provider, open_repository computes the root commit and branch list, so repeated calls add avoidable overhead.
Agent Prompt
### Issue description
`DynamicVcsProvider` calls `provider_and_repo_for_path(repo.path())` inside most operations, which repeatedly calls `open_repository()` (and tries multiple providers). This discards any benefit of opening once and can be expensive for providers that scan repository history/branches during open.
### Issue Context
`VcsRepository` is a pure-data entity; to avoid reopening, the dynamic provider needs an internal cache that maps a repo identity (e.g., `RepositoryId` or canonicalized path) to the chosen concrete provider.
### Fix Focus Areas
- crates/mcb-infrastructure/src/di/vcs.rs[33-105]
- crates/mcb-providers/src/vcs/git2_provider.rs[57-107]
- crates/mcb-providers/src/vcs/git2_provider.rs[117-135]
### Suggested fix approach
- In `DynamicVcsProvider::open_repository()`, after resolving a provider, cache the chosen provider keyed by `repo.id()` (or canonical path).
- In subsequent methods (list_branches, list_files, read_file, diff_refs, etc.), pick the provider from the cache instead of re-probing all providers.
- Keep a fallback path (probe providers) if the cache misses, but avoid doing it on every call.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+49
to
+56
| fn create_nats_event_bus_provider( | ||
| config: &EventBusProviderConfig, | ||
| ) -> std::result::Result<Arc<dyn EventBusProvider>, String> { | ||
| let url = config | ||
| .extra | ||
| .get("url") | ||
| .cloned() | ||
| .unwrap_or_else(|| "nats://127.0.0.1:4222".to_owned()); |
Contributor
There was a problem hiding this comment.
1. Hardcoded nats url default 📘 Rule violation ☼ Reliability
create_nats_event_bus_provider silently falls back to a hardcoded nats://127.0.0.1:4222 when config.extra.url is missing, masking misconfiguration and potentially connecting to the wrong broker in non-local environments. Required external connection settings should be provided via configuration and missing values should cause a clear failure instead of using an in-code default.
Agent Prompt
## Issue description
`create_nats_event_bus_provider()` uses a hardcoded default NATS URL when `config.extra["url"]` is missing. This prevents fail-fast behavior for required external configuration and hardcodes an environment-dependent value.
## Issue Context
The NATS URL determines which external broker the provider connects to; defaulting to localhost can hide configuration mistakes and cause unexpected behavior outside local development.
## Fix Focus Areas
- crates/mcb-providers/src/events/nats.rs[49-71]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+62
to
+66
| fn repository_id(&self, repo: &VcsRepository) -> RepositoryId { | ||
| self.providers | ||
| .first() | ||
| .map_or_else(|| *repo.id(), |provider| provider.repository_id(repo)) | ||
| } |
Contributor
There was a problem hiding this comment.
2. Wrong vcs repository_id 🐞 Bug ≡ Correctness
DynamicVcsProvider::repository_id() delegates to the first registered provider rather than the provider that produced the VcsRepository, so repository identity can change depending on registry ordering. This can cause inconsistent IDs vs VcsRepository::id() and mis-associate repository-scoped data.
Agent Prompt
### Issue description
`DynamicVcsProvider::repository_id()` currently calls `repository_id()` on `self.providers.first()`, which is unrelated to the provider that created the `VcsRepository`. That can return a different `RepositoryId` than the one embedded in the `VcsRepository`, breaking identity stability.
### Issue Context
`VcsRepository` already stores an `id` computed at open time by the concrete provider. The dynamic wrapper should not recompute identity using an arbitrary provider.
### Fix Focus Areas
- crates/mcb-infrastructure/src/di/vcs.rs[55-66]
- crates/mcb-domain/src/entities/vcs/vcs_repo.rs[10-18]
### Suggested fix
- Change `DynamicVcsProvider::repository_id()` to return `*repo.id()`.
- (Optional, if you truly need recomputation) store the selected provider alongside the repo (e.g., wrap `VcsRepository` in a provider-tagged struct) so subsequent calls use the same provider.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+49
to
+70
| fn create_nats_event_bus_provider( | ||
| config: &EventBusProviderConfig, | ||
| ) -> std::result::Result<Arc<dyn EventBusProvider>, String> { | ||
| let url = config | ||
| .extra | ||
| .get("url") | ||
| .cloned() | ||
| .unwrap_or_else(|| "nats://127.0.0.1:4222".to_owned()); | ||
| let subject = config | ||
| .extra | ||
| .get("subject") | ||
| .cloned() | ||
| .unwrap_or_else(|| NATS_DEFAULT_SUBJECT.to_owned()); | ||
| let client_name = config.extra.get("client_name").cloned(); | ||
|
|
||
| let rt = tokio::runtime::Handle::try_current().map_err(|e| e.to_string())?; | ||
| rt.block_on(async move { | ||
| NatsEventBusProvider::with_options(&url, &subject, client_name.as_deref()) | ||
| .await | ||
| .map(|provider| Arc::new(provider) as Arc<dyn EventBusProvider>) | ||
| .map_err(|e| e.to_string()) | ||
| }) |
Contributor
There was a problem hiding this comment.
3. Nats factory blocks runtime 🐞 Bug ☼ Reliability
The NATS event bus registry factory uses Handle::block_on to perform async connection during provider resolution, which can deadlock or panic depending on the Tokio runtime context (e.g., current-thread) and will block a runtime thread during network connect. This can hang or crash app startup when the NATS provider is selected.
Agent Prompt
### Issue description
The linkme registry `build` function for the NATS event bus performs async initialization by calling `tokio::runtime::Handle::block_on(...)`. This is risky inside an already-running Tokio runtime: it blocks a runtime thread and may deadlock/panic in some runtime flavors/contexts.
### Issue Context
Provider resolution happens during `init_app()` (async), so registry factories can execute while running on Tokio. The registry factory signature is sync (`fn(&Config) -> Result<Arc<...>, String>`), so it should not do async I/O.
### Fix Focus Areas
- crates/mcb-providers/src/events/nats.rs[49-70]
- crates/mcb-providers/src/events/nats.rs[122-130]
- crates/mcb-infrastructure/src/di/bootstrap.rs[303-309]
### Suggested fix
- Make the registry factory return a provider that connects lazily in async methods:
- Store `url/subject/client_name` in the provider.
- Hold a `tokio::sync::OnceCell<Arc<Client>>` (or similar) and `get_or_try_init().await` inside `publish/subscribe`.
- Alternatively, redesign the registry/DI path to support async factories (larger change), and await provider creation during bootstrap instead of blocking.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+58
to
+72
| #[async_trait] | ||
| impl FileSystemProvider for LocalFileSystemProvider { | ||
| async fn read_to_string(&self, path: &Path) -> Result<String, Error> { | ||
| std::fs::read_to_string(path) | ||
| .map_err(|e| Error::internal(format!("Failed to read file {}: {e}", path.display()))) | ||
| } | ||
|
|
||
| async fn read_dir_entries(&self, path: &Path) -> Result<Vec<DirEntry>, Error> { | ||
| Self::read_dir_sync(path) | ||
| } | ||
|
|
||
| async fn canonicalize_path(&self, path: &Path) -> Result<PathBuf, Error> { | ||
| std::fs::canonicalize(path) | ||
| .map_err(|e| Error::internal(format!("Failed to canonicalize {}: {e}", path.display()))) | ||
| } |
Contributor
There was a problem hiding this comment.
4. Async fs blocks tokio 🐞 Bug ➹ Performance
LocalFileSystemProvider implements async FileSystemProvider methods using blocking std::fs operations, so indexing can block Tokio worker threads while scanning directories and reading files. This can cause runtime starvation and latency spikes during indexing, especially on large workspaces or slow disks.
Agent Prompt
### Issue description
`LocalFileSystemProvider` exposes async APIs but performs synchronous filesystem I/O (`std::fs::*`) inside those async fns. Awaiting these methods does not make the I/O non-blocking; it still runs on the async executor thread.
### Issue Context
Indexing uses `file_system_provider.read_dir_entries().await` during discovery and `read_to_string().await` per file, so this blocking behavior can materially impact end-to-end indexing.
### Fix Focus Areas
- crates/mcb-providers/src/fs/mod.rs[34-72]
- crates/mcb-application/src/use_cases/indexing_service.rs[192-227]
- crates/mcb-application/src/use_cases/indexing_service.rs[427-444]
### Suggested fix
- Switch to `tokio::fs::read_to_string`.
- Implement `read_dir_entries` using `tokio::fs::read_dir` and iterate via `next_entry().await`.
- For operations without async equivalents (or for simplicity), wrap blocking calls in `tokio::task::spawn_blocking` (or `block_in_place` where appropriate) and `await` the join handle.
- Apply the same approach to `canonicalize_path`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+52
to
+55
| for entry in PROJECT_DETECTORS { | ||
| let has_marker = entry.marker_files.iter().any(|f| path.join(f).exists()); | ||
| if !has_marker { | ||
| continue; |
Contributor
There was a problem hiding this comment.
5. Blocking marker scan 🐞 Bug ☼ Reliability
ProjectService::detect_all() does synchronous Path::exists checks inside an async method for each detector marker file, which can block the async runtime and also collapses I/O errors into "not found". This can hide permission/IO problems and silently skip project detection.
Agent Prompt
### Issue description
`detect_all()` is async but calls `path.join(f).exists()` (blocking metadata) per marker file; `exists()` also returns `false` on many errors, so detection silently ignores I/O failures.
### Issue Context
Project detection may run during request handling or other concurrent async workloads; blocking calls reduce throughput and can make failures non-obvious.
### Fix Focus Areas
- crates/mcb-infrastructure/src/project/service.rs[43-63]
### Suggested fix
- Use an async check (e.g., `tokio::fs::try_exists` if available in your Tokio version) or `tokio::fs::metadata(...).await`.
- If you keep a sync check, run it via `tokio::task::spawn_blocking`.
- Distinguish between "missing" and "error" so permission/IO problems can be logged or surfaced appropriately.
ⓘ 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 b931063 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
40 issues found across 71 files
Confidence score: 2/5
crates/mcb-providers/src/events/nats.rsandcrates/mcb-infrastructure/src/di/bootstrap.rscan panic during normal startup whenevent_bus.provider = "nats"because provider resolution callsHandle::block_onfrom inside an active Tokio runtime, turning a valid config into a hard crash—make NATS provider construction async end-to-end (or move blocking work off-runtime) so bootstrap returns an error instead of panicking.crates/mcb-providers/src/events/tokio.rsandcrates/mcb-infrastructure/src/di/bootstrap.rsdo not reliably validatecapacity, so non-numeric values silently fall back and0can panic inbroadcast::channel(0)during resolution—parse and enforce a positive capacity before provider creation and surface a configuration error.crates/mcb-application/src/use_cases/indexing_service.rsregresses indexing behavior by dropping ignore-file matching, which can ingest git-ignored/generated/vendor content, and it can also reportstartedafter spawn failure, leaving status stuck as active—restoreWalkBuilderignore semantics and roll back operation state when spawn fails.crates/mcb-providers/src/task/mod.rsandcrates/mcb-providers/src/fs/mod.rscan fail abruptly under common runtime conditions: task spawning panics without an entered Tokio runtime and async FS APIs block worker threads viastd::fscalls—useHandle::try_current()to returnErrand move file I/O totokio::fs/spawn_blockingto avoid runtime instability.
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="crates/mcb-providers/src/events/nats.rs">
<violation number="1" location="crates/mcb-providers/src/events/nats.rs:65">
P0: Selecting the NATS event bus during normal startup will panic: `init_app` is async and resolves this provider from inside the Tokio runtime, while this synchronous factory calls `Handle::block_on` on that same runtime thread. The NATS connection needs an async-aware construction path (for example, an async registry constructor/resolver) rather than nesting `block_on`; otherwise the advertised NATS provider cannot be initialized through DI.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/bootstrap.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/bootstrap.rs:308">
P0: Starting the app with `event_bus.provider = "nats"` panics during bootstrap. `resolve_from_config()` invokes the synchronous registry builder, while the NATS builder calls `Handle::block_on`; this call occurs inside async `init_app`, where Tokio forbids nested `block_on`. NATS resolution needs an async construction path (or a provider that connects lazily) so startup can await it and propagate connection errors normally.</violation>
<violation number="2" location="crates/mcb-infrastructure/src/di/bootstrap.rs:308">
P1: A configured event-bus `capacity = 0` now panics during startup instead of returning a configuration error. Validate positive capacity before resolving the provider.</violation>
<violation number="3" location="crates/mcb-infrastructure/src/di/bootstrap.rs:339">
P2: Database provider construction now runs twice, and repositories are created by a different provider instance from the one that connected the executor. Reusing `db_provider.connect(...)` avoids duplicate initialization and future state mismatches for stateful providers.</violation>
</file>
<file name="crates/mcb-application/src/use_cases/indexing_service.rs">
<violation number="1" location="crates/mcb-application/src/use_cases/indexing_service.rs:199">
P1: Git-ignored files and directories are now indexed because the manual DFS applies only `SKIP_DIRS`, dropping `WalkBuilder`'s ignore-file matching. This can ingest generated/vendor code or intentionally excluded source; preserve ignore matching in the filesystem abstraction or retain an ignore-aware walker.</violation>
<violation number="2" location="crates/mcb-application/src/use_cases/indexing_service.rs:202">
P2: One unreadable or concurrently removed entry now prevents every other file and subdirectory in that directory from being indexed. Consider exposing per-entry results or partial entries so discovery can record the bad entry and continue with its siblings.</violation>
<violation number="3" location="crates/mcb-application/src/use_cases/indexing_service.rs:305">
P2: A spawn failure is returned as a successful `started` result and leaves `get_status()` reporting an active operation forever. Roll back the operation and propagate the spawn error instead of continuing with the success response.</violation>
</file>
<file name="crates/mcb-providers/src/task/mod.rs">
<violation number="1" location="crates/mcb-providers/src/task/mod.rs:35">
P1: Calling this provider from a thread without an entered Tokio runtime panics instead of returning the `Err` promised by `TaskRunnerProvider::spawn`. Using `Handle::try_current()` preserves the provider boundary and lets callers handle the spawn failure.</violation>
</file>
<file name="crates/mcb-providers/src/events/tokio.rs">
<violation number="1" location="crates/mcb-providers/src/events/tokio.rs:52">
P1: Invalid `capacity` configuration is not reported: non-numeric values silently select the default, while `0` reaches `broadcast::channel(0)` and panics during provider resolution. Parsing and validating the value through the constructor's `Result` would turn both cases into configuration errors.</violation>
</file>
<file name="config/mcb-validate-internal.toml">
<violation number="1" location="config/mcb-validate-internal.toml:169">
P2: DEP006 can still be bypassed from independently compiled Rust targets, notably the existing `crates/mcb/tests` directory and examples/benches omitted from these scan roots. Consider covering all non-provider crate targets (or scanning each crate root with an explicit exclusion for `mcb-providers`) so the boundary applies consistently.</violation>
<violation number="2" location="config/mcb-validate-internal.toml:170">
P1: DEP006 does not enforce the stated ban on provider imports: it only searches for the exact text `mcb_providers::`. Rust imports such as the existing `crates/mcb-infrastructure/src/provider_linker.rs` line `use mcb_providers as _;` (and any aliased or `extern crate` import) pass this boundary despite the infrastructure scan having no allowed files. Parsing `use`/`extern crate` items, with an explicit composition-root exception if `provider_linker.rs` is intentional, would prevent these bypasses and avoid matching fixture strings as imports.</violation>
</file>
<file name="crates/mcb/src/cli/validate.rs">
<violation number="1" location="crates/mcb/src/cli/validate.rs:153">
P2: Text-mode validation can report success after stdout fails, leaving callers with missing or truncated output; every new text `writeln!` result is discarded while JSON output propagates the error. Consider returning `Result` from the text-printing helpers and propagating each write failure through `execute`.</violation>
</file>
<file name="crates/mcb-providers/src/fs/mod.rs">
<violation number="1" location="crates/mcb-providers/src/fs/mod.rs:36">
P2: Filesystem failures are reported as internal application faults and lose their source chain, preventing callers and diagnostics from recognizing I/O errors. Mapping each failure with `Error::io_with_source(context, e)` would preserve the intended error category and source.</violation>
<violation number="2" location="crates/mcb-providers/src/fs/mod.rs:61">
P2: Codebase discovery and file processing can block Tokio worker threads because every async filesystem method performs synchronous `std::fs` I/O without yielding. Using `tokio::fs` (or `spawn_blocking`) for directory iteration, reads, metadata, and canonicalization would keep indexing from stalling unrelated async work.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/vcs.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/vcs.rs:21">
P2: A construction failure in any registered VCS backend now prevents the entire application VCS service from starting, even if another backend is usable. Consider retaining successfully constructed providers, reporting skipped failures, and failing only when none can be initialized.</violation>
<violation number="2" location="crates/mcb-infrastructure/src/di/vcs.rs:40">
P2: Branch search now performs a full repository open, root-history walk, and branch enumeration for every file read, which can make large repositories prohibitively slow. Preserve the provider selected by the initial open or add a cheap provider probe/cache instead of using `open_repository` for every delegated call.</violation>
<violation number="3" location="crates/mcb-infrastructure/src/di/vcs.rs:63">
P2: Repositories opened by any provider except the first can receive an ID computed by the wrong backend, risking incorrect request/context attribution. Return the ID embedded in `VcsRepository`, or retain and dispatch to the provider that created it.</violation>
<violation number="4" location="crates/mcb-infrastructure/src/di/vcs.rs:112">
P2: Repository discovery becomes unavailable if any one registered backend fails, even when other backends already found valid repositories. Consider isolating per-provider scan failures and returning an error only when no provider can complete, while retaining successful results.</violation>
</file>
<file name="crates/mcb-infrastructure/src/infrastructure/factory.rs">
<violation number="1" location="crates/mcb-infrastructure/src/infrastructure/factory.rs:83">
P2: When Tokio provider resolution fails, callers receive a successful-looking bus that silently drops every event, so admin event workflows can appear healthy while doing nothing. Preserve the documented default-bus contract by surfacing the configuration/linking failure instead of falling back to `NullEventBusProvider`.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/test_factory.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/test_factory.rs:41">
P2: Test repositories can be created by a non-SQLite provider while still receiving the caller’s SQLite executor because the factory selects the first registry entry. Selecting the `sqlite` entry explicitly (or passing the provider alongside the executor) keeps repository and executor implementations paired.</violation>
</file>
<file name="crates/mcb-providers/src/lib.rs">
<violation number="1" location="crates/mcb-providers/src/lib.rs:92">
P2: Indexing now performs blocking `std::fs` operations inside async provider methods, potentially stalling Tokio workers throughout directory discovery and per-file processing. Consider implementing this provider with `tokio::fs` or isolating synchronous filesystem work with `spawn_blocking`.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/repositories.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/repositories.rs:25">
P2: Standalone repository factories become registration-order-dependent as soon as another database backend is linked, because the first registry entry is treated as the default without any default semantics. Consider accepting a provider/configuration or explicitly preserving the intended SQLite fallback rather than selecting `.first()`.</violation>
<violation number="2" location="crates/mcb-infrastructure/src/di/repositories.rs:35">
P2: Memory repositories can be created by a different provider instance from the one that opened their executor, since `connect_executor()` resolves the provider again. Passing the already-resolved provider into the connection helper (or calling `provider.connect(...)` directly) keeps connection and repository construction within one provider instance.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/provider_resolvers.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/provider_resolvers.rs:367">
P2: Switching only the provider to NATS passes the default config's empty `nats_url` to the client instead of using the builder's `nats://127.0.0.1:4222` fallback, so startup cannot connect. Treat a blank optional URL as absent or reject it during config validation.</violation>
<violation number="2" location="crates/mcb-infrastructure/src/di/provider_resolvers.rs:374">
P2: Configured NATS connection timeout and reconnect-attempt values are silently ignored by the new resolver. Users can set `connection_timeout_ms` and `max_reconnect_attempts`, but neither reaches the provider, so runtime behavior remains at library defaults. The registry config and NATS constructor should accept and apply both settings, with this resolver forwarding them.</violation>
</file>
<file name="crates/mcb-infrastructure/src/project/service.rs">
<violation number="1" location="crates/mcb-infrastructure/src/project/service.rs:52">
P3: Project detection now has two orchestration implementations, while `detect_all_projects` becomes an unused public implementation and has already diverged in logging behavior. Consider retaining one shared implementation in a dependency-neutral layer or removing the obsolete provider facade in this migration.</violation>
<violation number="2" location="crates/mcb-infrastructure/src/project/service.rs:58">
P2: Detector construction and execution failures now disappear silently, so `detect_all` returns an incomplete result with no diagnostic identifying the failed detector. Preserve the prior warning paths while centralizing this orchestration.</violation>
</file>
<file name="crates/mcb-domain/src/ports/providers/fs.rs">
<violation number="1" location="crates/mcb-domain/src/ports/providers/fs.rs:12">
P2: Indexing cannot distinguish symlinks through this port, so `follow_symlinks = true` cannot be honored and the local adapter silently drops every symlink. A file-kind enum including symlinks would preserve the needed state and prevent contradictory boolean combinations.</violation>
</file>
<file name="crates/mcb-domain/src/registry/project_detection.rs">
<violation number="1" location="crates/mcb-domain/src/registry/project_detection.rs:2">
P2: This module disables the workspace's `unsafe_code = "deny"` policy for all code added here, even though the module only declares and iterates a `linkme` slice and the other registries do not need this exception. Keeping the deny lint active avoids creating an unreviewed pocket where future unsafe code can compile; this allowance can be removed.</violation>
</file>
<file name="crates/mcb-infrastructure/src/config/types/system.rs">
<violation number="1" location="crates/mcb-infrastructure/src/config/types/system.rs:123">
P2: Fallback-created Tokio event buses now use capacity 1000, while `TokioEventBusProvider::new()` and `config/default.toml` still use 1024. Centralizing this default would preserve one shared value (or update all remaining defaults together) to avoid construction-path-dependent behavior.</violation>
</file>
<file name="crates/mcb-domain/src/ports/infrastructure/database.rs">
<violation number="1" location="crates/mcb-domain/src/ports/infrastructure/database.rs:87">
P2: Once another database backend is registered, a repository can be created by one provider around another provider's executor, producing runtime SQL-dialect failures. Encoding provider/executor affinity—such as having the provider own its connection or returning a provider-specific repository bundle from `connect`—would make invalid composition impossible.</violation>
</file>
<file name="crates/mcb-validate/src/validators/dependency/bypass.rs">
<violation number="1" location="crates/mcb-validate/src/validators/dependency/bypass.rs:45">
P3: DEP006 dispatch can regress back to the `_` fallback without any focused test failing. A bypass-boundary test mirroring the DEP004/DEP005 cases and asserting `DependencyViolation::ProviderBypassImport` would protect this new behavior.</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:210">
P2: This integration test now requires the external `git` executable even though the resolved provider uses libgit2. On a test runner without the CLI, `Command::new("git").output()?` fails before `open_repository` is exercised. Building the repository fixture through a library/helper (or explicitly detecting and skipping when the CLI is unavailable) would keep this DI test from adding an unrelated environment dependency.</violation>
</file>
<file name="crates/mcb-server/tests/integration/browse_api_integration.rs">
<violation number="1" location="crates/mcb-server/tests/integration/browse_api_integration.rs:394">
P3: These browse tests now duplicate the infrastructure layer's `VectorStoreBrowserAdapter`, including all three forwarding methods and repeated wrapping at every setup. `mcb_infrastructure::infrastructure::factory::create_test_vector_store_for_e2e` already returns the provider/browser pair backed by one store specifically for this workflow; reusing that factory would keep the adapter behavior in one place and avoid the local copy drifting when `VectorStoreBrowser` changes.</violation>
</file>
<file name="crates/mcb-validate/src/validators/macros/violations.rs">
<violation number="1" location="crates/mcb-validate/src/validators/macros/violations.rs:183">
P3: The new blanket `#[allow(missing_docs)]` hides missing documentation across every enum generated by `define_violations!`, not just the new DEP006 variant. The macro already supports variant doc comments, so documenting `ProviderBypassImport` and removing this suppression would preserve the workspace lint for all 46 macro call sites instead of disabling that quality check globally.</violation>
</file>
<file name="crates/mcb-validate/src/validators/dependency/violation.rs">
<violation number="1" location="crates/mcb-validate/src/validators/dependency/violation.rs:97">
P3: DEP006's ID, message, severity, and validator mapping have no behavioral regression test, so this new rule can stop emitting the intended variant unnoticed. Consider adding a dependency-validator test with a forbidden `mcb_providers::` import and asserting `ProviderBypassImport` plus its metadata.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/modules/domain_services.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/modules/domain_services.rs:100">
P3: Public `ServiceDependencies` documentation is weakened by exempting the entire struct from the workspace `missing_docs` lint for two new undocumented fields. Prefer documenting `file_system_provider` and `task_runner_provider` (including the existing `# Fields` list) and leaving the lint active for this API.</violation>
</file>
<file name="crates/mcb-infrastructure/src/lib.rs">
<violation number="1" location="crates/mcb-infrastructure/src/lib.rs:56">
P3: The new public `provider_linker` module bypasses the workspace documentation lint instead of documenting its purpose. A module doc comment would preserve API documentation and keep the lint effective.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/mod.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/mod.rs:50">
P3: The public `di::vcs` module remains undocumented, and the module-wide allowance also hides missing documentation on future exports. A short module doc comment would preserve the workspace lint instead of suppressing it.</violation>
</file>
<file name="crates/mcb-domain/src/macros/di.rs">
<violation number="1" location="crates/mcb-domain/src/macros/di.rs:6">
P3: The macro accepts `= ImplementationType` annotations but silently ignores them, so an incorrect or stale implementation type still compiles and misrepresents the DI declaration. Consider removing this unused grammar or using it in generated/validation code.</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
| let client_name = config.extra.get("client_name").cloned(); | ||
|
|
||
| let rt = tokio::runtime::Handle::try_current().map_err(|e| e.to_string())?; | ||
| rt.block_on(async move { |
There was a problem hiding this comment.
P0: Selecting the NATS event bus during normal startup will panic: init_app is async and resolves this provider from inside the Tokio runtime, while this synchronous factory calls Handle::block_on on that same runtime thread. The NATS connection needs an async-aware construction path (for example, an async registry constructor/resolver) rather than nesting block_on; otherwise the advertised NATS provider cannot be initialized through DI.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-providers/src/events/nats.rs, line 65:
<comment>Selecting the NATS event bus during normal startup will panic: `init_app` is async and resolves this provider from inside the Tokio runtime, while this synchronous factory calls `Handle::block_on` on that same runtime thread. The NATS connection needs an async-aware construction path (for example, an async registry constructor/resolver) rather than nesting `block_on`; otherwise the advertised NATS provider cannot be initialized through DI.</comment>
<file context>
@@ -37,12 +37,46 @@ use futures::{StreamExt, stream};
+ let client_name = config.extra.get("client_name").cloned();
+
+ let rt = tokio::runtime::Handle::try_current().map_err(|e| e.to_string())?;
+ rt.block_on(async move {
+ NatsEventBusProvider::with_options(&url, &subject, client_name.as_deref())
+ .await
</file context>
|
|
||
| let event_bus: Arc<dyn EventBusProvider> = Arc::new(TokioEventBusProvider::new()); | ||
| let event_bus_resolver = EventBusProviderResolver::new(Arc::clone(&config)); | ||
| let event_bus: Arc<dyn EventBusProvider> = event_bus_resolver.resolve_from_config()?; |
There was a problem hiding this comment.
P0: Starting the app with event_bus.provider = "nats" panics during bootstrap. resolve_from_config() invokes the synchronous registry builder, while the NATS builder calls Handle::block_on; this call occurs inside async init_app, where Tokio forbids nested block_on. NATS resolution needs an async construction path (or a provider that connects lazily) so startup can await it and propagate connection errors normally.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-infrastructure/src/di/bootstrap.rs, line 308:
<comment>Starting the app with `event_bus.provider = "nats"` panics during bootstrap. `resolve_from_config()` invokes the synchronous registry builder, while the NATS builder calls `Handle::block_on`; this call occurs inside async `init_app`, where Tokio forbids nested `block_on`. NATS resolution needs an async construction path (or a provider that connects lazily) so startup can await it and propagate connection errors normally.</comment>
<file context>
@@ -435,13 +304,19 @@ pub async fn init_app(config: AppConfig) -> Result<AppContext> {
- let event_bus: Arc<dyn EventBusProvider> = Arc::new(TokioEventBusProvider::new());
+ let event_bus_resolver = EventBusProviderResolver::new(Arc::clone(&config));
+ let event_bus: Arc<dyn EventBusProvider> = event_bus_resolver.resolve_from_config()?;
let shutdown_coordinator: Arc<dyn ShutdownCoordinator> =
Arc::new(DefaultShutdownCoordinator::new());
</file context>
| && self.is_supported_file(entry.path()) | ||
| { | ||
| files.push(entry.path().to_path_buf()); | ||
| let mut dirs = vec![path.to_path_buf()]; |
There was a problem hiding this comment.
P1: Git-ignored files and directories are now indexed because the manual DFS applies only SKIP_DIRS, dropping WalkBuilder's ignore-file matching. This can ingest generated/vendor code or intentionally excluded source; preserve ignore matching in the filesystem abstraction or retain an ignore-aware walker.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-application/src/use_cases/indexing_service.rs, line 199:
<comment>Git-ignored files and directories are now indexed because the manual DFS applies only `SKIP_DIRS`, dropping `WalkBuilder`'s ignore-file matching. This can ingest generated/vendor code or intentionally excluded source; preserve ignore matching in the filesystem abstraction or retain an ignore-aware walker.</comment>
<file context>
@@ -185,31 +196,32 @@ impl IndexingServiceImpl {
- && self.is_supported_file(entry.path())
- {
- files.push(entry.path().to_path_buf());
+ let mut dirs = vec![path.to_path_buf()];
+
+ while let Some(dir) = dirs.pop() {
</file context>
Comment on lines
+35
to
+36
| tokio::spawn(task); | ||
| Ok(()) |
There was a problem hiding this comment.
P1: Calling this provider from a thread without an entered Tokio runtime panics instead of returning the Err promised by TaskRunnerProvider::spawn. Using Handle::try_current() preserves the provider boundary and lets callers handle the spawn failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-providers/src/task/mod.rs, line 35:
<comment>Calling this provider from a thread without an entered Tokio runtime panics instead of returning the `Err` promised by `TaskRunnerProvider::spawn`. Using `Handle::try_current()` preserves the provider boundary and lets callers handle the spawn failure.</comment>
<file context>
@@ -0,0 +1,38 @@
+
+impl TaskRunnerProvider for TokioTaskRunnerProvider {
+ fn spawn(&self, task: BoxFuture<'static, ()>) -> Result<()> {
+ tokio::spawn(task);
+ Ok(())
+ }
</file context>
Suggested change
| tokio::spawn(task); | |
| Ok(()) | |
| let runtime = tokio::runtime::Handle::try_current().map_err(|error| { | |
| mcb_domain::error::Error::internal(format!( | |
| "Failed to access Tokio runtime: {error}" | |
| )) | |
| })?; | |
| drop(runtime.spawn(task)); | |
| Ok(()) |
Comment on lines
+52
to
+56
| let capacity = config | ||
| .extra | ||
| .get("capacity") | ||
| .and_then(|v| v.parse::<usize>().ok()) | ||
| .unwrap_or(EVENTS_TOKIO_DEFAULT_CAPACITY); |
There was a problem hiding this comment.
P1: Invalid capacity configuration is not reported: non-numeric values silently select the default, while 0 reaches broadcast::channel(0) and panics during provider resolution. Parsing and validating the value through the constructor's Result would turn both cases into configuration errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-providers/src/events/tokio.rs, line 52:
<comment>Invalid `capacity` configuration is not reported: non-numeric values silently select the default, while `0` reaches `broadcast::channel(0)` and panics during provider resolution. Parsing and validating the value through the constructor's `Result` would turn both cases into configuration errors.</comment>
<file context>
@@ -37,12 +37,34 @@ use futures::stream;
+fn create_tokio_event_bus_provider(
+ config: &EventBusProviderConfig,
+) -> std::result::Result<Arc<dyn EventBusProvider>, String> {
+ let capacity = config
+ .extra
+ .get("capacity")
</file context>
Suggested change
| let capacity = config | |
| .extra | |
| .get("capacity") | |
| .and_then(|v| v.parse::<usize>().ok()) | |
| .unwrap_or(EVENTS_TOKIO_DEFAULT_CAPACITY); | |
| let capacity = match config.extra.get("capacity") { | |
| Some(value) => { | |
| let capacity = value | |
| .parse::<usize>() | |
| .map_err(|e| format!("Invalid Tokio event bus capacity '{value}': {e}"))?; | |
| if capacity == 0 { | |
| return Err("Tokio event bus capacity must be greater than zero".to_owned()); | |
| } | |
| capacity | |
| } | |
| None => EVENTS_TOKIO_DEFAULT_CAPACITY, | |
| }; |
| message = "Forbidden provider import outside mcb-providers: {file}:{line} - {context}", | ||
| suggestion = "Resolve providers via DI/registry and import only domain ports outside mcb-providers" | ||
| )] | ||
| ProviderBypassImport { |
There was a problem hiding this comment.
P3: DEP006's ID, message, severity, and validator mapping have no behavioral regression test, so this new rule can stop emitting the intended variant unnoticed. Consider adding a dependency-validator test with a forbidden mcb_providers:: import and asserting ProviderBypassImport plus its metadata.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-validate/src/validators/dependency/violation.rs, line 97:
<comment>DEP006's ID, message, severity, and validator mapping have no behavioral regression test, so this new rule can stop emitting the intended variant unnoticed. Consider adding a dependency-validator test with a forbidden `mcb_providers::` import and asserting `ProviderBypassImport` plus its metadata.</comment>
<file context>
@@ -88,5 +88,17 @@ define_violations! {
+ message = "Forbidden provider import outside mcb-providers: {file}:{line} - {context}",
+ suggestion = "Resolve providers via DI/registry and import only domain ports outside mcb-providers"
+ )]
+ ProviderBypassImport {
+ file: PathBuf,
+ line: usize,
</file context>
| /// * `agent_repository` - Repository for agent session data | ||
| /// * `vcs_provider` - Version control system provider | ||
| /// * `project_service` - Service for project detection and management | ||
| #[allow(missing_docs)] |
There was a problem hiding this comment.
P3: Public ServiceDependencies documentation is weakened by exempting the entire struct from the workspace missing_docs lint for two new undocumented fields. Prefer documenting file_system_provider and task_runner_provider (including the existing # Fields list) and leaving the lint active for this API.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-infrastructure/src/di/modules/domain_services.rs, line 100:
<comment>Public `ServiceDependencies` documentation is weakened by exempting the entire struct from the workspace `missing_docs` lint for two new undocumented fields. Prefer documenting `file_system_provider` and `task_runner_provider` (including the existing `# Fields` list) and leaving the lint active for this API.</comment>
<file context>
@@ -96,6 +97,7 @@ pub struct DomainServicesContainer {
/// * `agent_repository` - Repository for agent session data
/// * `vcs_provider` - Version control system provider
/// * `project_service` - Service for project detection and management
+#[allow(missing_docs)]
pub struct ServiceDependencies {
/// Unique identifier for the current project
</file context>
Comment on lines
+56
to
+57
| #[allow(missing_docs)] | ||
| pub mod provider_linker; |
There was a problem hiding this comment.
P3: The new public provider_linker module bypasses the workspace documentation lint instead of documenting its purpose. A module doc comment would preserve API documentation and keep the lint effective.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-infrastructure/src/lib.rs, line 56:
<comment>The new public `provider_linker` module bypasses the workspace documentation lint instead of documenting its purpose. A module doc comment would preserve API documentation and keep the lint effective.</comment>
<file context>
@@ -53,6 +53,8 @@ pub mod error_ext;
pub mod health;
pub mod logging;
pub mod project;
+#[allow(missing_docs)]
+pub mod provider_linker;
pub mod routing;
</file context>
Suggested change
| #[allow(missing_docs)] | |
| pub mod provider_linker; | |
| /// Forces the provider crate to be linked so distributed registrations are retained. | |
| pub mod provider_linker; |
| pub mod resolver; | ||
| #[cfg(feature = "test-utils")] | ||
| pub mod test_factory; | ||
| #[allow(missing_docs)] |
There was a problem hiding this comment.
P3: The public di::vcs module remains undocumented, and the module-wide allowance also hides missing documentation on future exports. A short module doc comment would preserve the workspace lint instead of suppressing it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-infrastructure/src/di/mod.rs, line 50:
<comment>The public `di::vcs` module remains undocumented, and the module-wide allowance also hides missing documentation on future exports. A short module doc comment would preserve the workspace lint instead of suppressing it.</comment>
<file context>
@@ -45,7 +45,9 @@ pub mod modules;
pub mod resolver;
+#[cfg(feature = "test-utils")]
pub mod test_factory;
+#[allow(missing_docs)]
pub mod vcs;
</file context>
Suggested change
| #[allow(missing_docs)] | |
| /// VCS provider resolution and dynamic provider dispatch. |
| macro_rules! arc_getters { | ||
| () => {}; | ||
|
|
||
| ($name:ident : $ty:ty => $field:ident $(= $impl:ty)? , $($rest:tt)*) => { |
There was a problem hiding this comment.
P3: The macro accepts = ImplementationType annotations but silently ignores them, so an incorrect or stale implementation type still compiles and misrepresents the DI declaration. Consider removing this unused grammar or using it in generated/validation code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-domain/src/macros/di.rs, line 6:
<comment>The macro accepts `= ImplementationType` annotations but silently ignores them, so an incorrect or stale implementation type still compiles and misrepresents the DI declaration. Consider removing this unused grammar or using it in generated/validation code.</comment>
<file context>
@@ -0,0 +1,41 @@
+macro_rules! arc_getters {
+ () => {};
+
+ ($name:ident : $ty:ty => $field:ident $(= $impl:ty)? , $($rest:tt)*) => {
+ #[doc = concat!("Get `", stringify!($name), "`.")]
+ #[must_use]
</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.