Feature/mcb loc reduction - #148
Open
marlon-costa-dc wants to merge 7 commits into
Open
Conversation
- Add Error::not_found_or() helper to consolidate ok_or_else(|| Error::not_found(...)) pattern - Replace 18 instances across 5 SQLite repository files with the new helper - Improves error handling consistency and reduces boilerplate - All production unwrap/expect remain in test code only (8 instances, acceptable) - Zero clippy warnings, all tests pass
- 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.
- Create comprehensive sync tests in tests/unit/schema_entity_sync_tests.rs
- Detect schema-only fields: Repository.origin_context, Worktree.{org_id, project_id, origin_context}, Branch.origin_context
- Report full schema structure for manual review
- All 7 tests pass, documents divergence mitigation strategy
- Foundation for derive(TableSchema) migration in Phase 1
…schema - Introduced `impl_table_schema!` macro for defining table schemas directly in entity files. - Migrated existing entities (ApiKey, Organization, Team, TeamMember, User) to use the new macro. - Removed legacy schema modules for these entities, consolidating schema definitions. - Added SQLite backend integration with SeaORM for simplified CRUD operations. - Updated repository implementations to utilize SeaORM where applicable. - Added tests to ensure schema synchronization between entities and database definitions.
- Simplified the `tables` function in `definition.rs` by removing unnecessary line breaks. - Enhanced readability of column name assertions in `schema_entity_sync_tests.rs` by formatting them into multi-line vectors. - Cleaned up struct initialization in `backend.rs` for a more concise representation. - Standardized SQL statement formatting in `org_entity_repository.rs`. - Improved import formatting in `row_convert.rs` for better clarity. - Adjusted column type import in `mod.rs` to maintain consistent style.
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: 29 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 (63)
✨ 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 |
coderabbitai
Bot
added
enhancement
Contributor
PR Summary by QodoRefactor DI providers (FS/Task/VCS/EventBus), SeaORM org repo, error helper
AI Description
Diagram
High-Level Assessment
Files changed (64)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
36 rules 1. Org update overwrites created_at
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+30
to
+40
| #[allow(dead_code)] | ||
| #[derive(Clone, Debug, PartialEq, Eq, sea_orm::DeriveEntityModel)] | ||
| #[sea_orm(table_name = $table)] | ||
| pub struct Model { | ||
| #[sea_orm(primary_key)] | ||
| pub $first: $first_ty, | ||
| $( pub $f: $ty ),* | ||
| } | ||
|
|
||
| #[allow(dead_code)] | ||
| #[derive(Copy, Clone, Debug, sea_orm::EnumIter, sea_orm::DeriveRelation)] |
Contributor
There was a problem hiding this comment.
1. #[allow(dead_code)] suppresses dead code 📘 Rule violation ⚙ Maintainability
New #[allow(dead_code)] directives were added in production code, bypassing the project’s dead-code policy and making it easier for unused items to accumulate unnoticed.
Agent Prompt
## Issue description
Production code introduces `#[allow(dead_code)]`, which violates the repo policy against dead code and lint-bypassing.
## Issue Context
These suppressions appear to have been added to silence warnings for generated SeaORM entity items/modules.
## Fix Focus Areas
- crates/mcb-providers/src/database/sqlite/macros.rs[30-41]
- crates/mcb-providers/src/database/sqlite/sea_entities/mod.rs[6-7]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -0,0 +1,20 @@ | |||
| #![allow(missing_docs)] | |||
Contributor
There was a problem hiding this comment.
2. missing_docs allow lacks justification 📘 Rule violation ⚙ Maintainability
Static-analysis suppression directives for missing_docs were added without an inline justification comment, reducing auditability of why the lint is being suppressed.
Agent Prompt
## Issue description
`#![allow(missing_docs)]` / `#[allow(missing_docs)]` suppressions were added without a specific justification comment on the same line (or immediately next line).
## Issue Context
The compliance rule requires each suppression to carry a concrete reason (e.g., "generated registry boilerplate; public API documented in X").
## Fix Focus Areas
- crates/mcb-domain/src/registry/vcs.rs[1-1]
- crates/mcb-infrastructure/src/di/vcs.rs[12-15]
- crates/mcb-infrastructure/src/di/mod.rs[49-50]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| error::{Error, Result}, | ||
| ports::VcsProvider, | ||
| }; | ||
| use std::sync::Arc; |
Contributor
There was a problem hiding this comment.
3. git2_provider.rs import order broken 📘 Rule violation ⚙ Maintainability
A std import (use std::sync::Arc;) is placed after non-std imports, violating the required grouped/ordered Rust import convention.
Agent Prompt
## Issue description
Rust `use` statements are not grouped/ordered as required: `std` imports must come before external crates, then `mcb_*`, then local modules.
## Issue Context
`use std::sync::Arc;` appears after `mcb_domain` imports.
## Fix Focus Areas
- crates/mcb-providers/src/vcs/git2_provider.rs[6-21]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| if has_violations { | ||
| println!(); | ||
| let _ = writeln!(std::io::stdout()); |
Contributor
There was a problem hiding this comment.
4. validate.rs ignores stdout errors 📘 Rule violation ☼ Reliability
The CLI validation output uses let _ = writeln!(...), silently discarding I/O errors instead of propagating or reporting them.
Agent Prompt
## Issue description
`writeln!` results are ignored via `let _ = ...`, which can hide output failures (broken pipe, closed stdout, etc.).
## Issue Context
This code is part of the validate CLI output path; errors should be propagated (`?`) or explicitly handled/logged.
## 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
+126
to
+127
| pub fn default_vcs_provider() -> Result<Arc<dyn VcsProvider>> { | ||
| Ok(Arc::new(DynamicVcsProvider::from_registry()?)) |
Contributor
There was a problem hiding this comment.
5. Old vcs factory left 📘 Rule violation ⚙ Maintainability
A new registry-based VCS provider selection mechanism was added, but the older mcb-providers default_vcs_provider() factory remains present as a parallel implementation, increasing the risk of divergence and unused legacy paths.
Agent Prompt
## Issue description
The PR introduces a new registry-driven VCS provider mechanism in infrastructure DI, but the older provider factory in `mcb-providers` remains, leaving two competing ways to construct the default VCS provider.
## Issue Context
This violates the expectation that superseded implementations are removed in the same change that introduces their replacement (unless both paths remain intentionally reachable and documented).
## Fix Focus Areas
- crates/mcb-infrastructure/src/di/vcs.rs[17-31]
- crates/mcb-providers/src/vcs/factory.rs[10-14]
- crates/mcb-providers/src/vcs/mod.rs[1-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+64
to
+70
| 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.
6. Nats build blocks runtime 🐞 Bug ☼ Reliability
create_nats_event_bus_provider uses tokio::runtime::Handle::block_on to synchronously await an async connect during provider construction, which can panic or block progress when called from within the Tokio runtime. init_app resolves the event bus provider inside async bootstrap, so selecting the NATS provider can prevent the application from starting cleanly.
Agent Prompt
### Issue description
`create_nats_event_bus_provider` blocks on async initialization using `Handle::block_on`, but the provider is resolved during async bootstrap (`init_app`). This can panic (Tokio blocking-in-runtime constraint) or stall startup while the runtime thread is blocked.
### Issue Context
The event bus registry build API is synchronous (`build: fn(&Config) -> Result<Arc<dyn ...>, String>`), but NATS initialization (`with_options`) is async.
### Fix Focus Areas
- crates/mcb-providers/src/events/nats.rs[49-70]
- crates/mcb-infrastructure/src/di/bootstrap.rs[305-307]
### Suggested fix approaches (pick one)
1) **Lazy-initialize NATS connection**: return a provider that stores `url/subject/client_name` and uses an async `OnceCell` to connect on first `publish/subscribe`.
2) **Make resolution async**: change the registry/resolver API for event bus providers to support async builders (e.g., returning a `BoxFuture`), then `await` in `init_app`.
3) If you must block, do it safely: wrap in `tokio::task::block_in_place` and document the startup cost (still not ideal).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+69
to
+72
| 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 | ||
| } |
Contributor
There was a problem hiding this comment.
7. Vcs repo reopening overhead 🐞 Bug ➹ Performance
DynamicVcsProvider calls open_repository again for each VCS operation via provider_and_repo_for_path(repo.path()), re-discovering the provider and re-opening the repo repeatedly. This adds avoidable per-call overhead and scales poorly when multiple VCS providers are registered.
Agent Prompt
### Issue description
`DynamicVcsProvider` reopens repositories on every operation (`list_branches`, `commit_history`, `list_files`, `read_file`, `diff_refs`) by re-running provider discovery + `open_repository`. For git2 this recomputes root hash / branches / default branch and reopens libgit2 state each time.
### Issue Context
The wrapper currently has no stable mapping from `VcsRepository` to the concrete provider that opened it.
### Fix Focus Areas
- crates/mcb-infrastructure/src/di/vcs.rs[33-106]
- crates/mcb-providers/src/vcs/git2_provider.rs[119-135]
### Suggested fix approaches
- **Add provider identity to `VcsRepository`** (e.g., `provider_name: String`) and dispatch directly without reopening.
- **Cache resolution** inside `DynamicVcsProvider` keyed by canonicalized path (e.g., `RwLock<HashMap<PathBuf, Arc<dyn VcsProvider>>>`), so subsequent calls avoid probing and avoid calling `open_repository`.
- If reopening is unavoidable, at least avoid probing multiple providers by caching the first successful provider per path.
ⓘ 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.
8. Indexing loses ignore filtering 🐞 Bug ➹ Performance
discover_files now performs a manual directory traversal and only skips directories listed in SKIP_DIRS, which removes ignore-aware walking behavior from the previous approach. This can cause indexing to traverse and embed many additional files (generated/vendor/etc.), increasing runtime and index noise.
Agent Prompt
### Issue description
`IndexingServiceImpl::discover_files` now walks the filesystem via `read_dir_entries` and filters only by `SKIP_DIRS` + extension. Any previous ignore-aware behavior (e.g., ignore files / repo ignore rules) is no longer applied during traversal.
### Issue Context
`SKIP_DIRS` is a fixed list and cannot cover repo-specific ignore needs.
### Fix Focus Areas
- crates/mcb-application/src/use_cases/indexing_service.rs[192-226]
- crates/mcb-application/src/constants.rs[22-24]
### Suggested fix approaches
- Reintroduce an ignore-aware walker (e.g., `ignore` crate) for traversal, while still using `FileSystemProvider` for file reads.
- Alternatively, extend `FileSystemProvider` with an ignore-aware walking API (provider-specific implementation can use `ignore::WalkBuilder` for local FS).
- Ensure traversal continues to honor `SKIP_DIRS` in addition to ignore rules.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+52
to
+61
| 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()); |
Contributor
There was a problem hiding this comment.
1. Nats config defaults to localhost 📘 Rule violation ☼ Reliability
The NATS provider silently falls back to hardcoded defaults (nats://127.0.0.1:4222 and a default subject) when configuration is missing, instead of failing fast. This can mask misconfiguration and accidentally connect to unintended infrastructure.
Agent Prompt
## Issue description
The NATS event bus provider uses hardcoded defaults when `url`/`subject` are missing, rather than failing fast.
## Issue Context
When a provider is selected, required connection details should be explicitly configured; hardcoded fallbacks can hide deployment mistakes.
## Fix Focus Areas
- crates/mcb-providers/src/events/nats.rs[52-61]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+60
to
+71
| 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.
2. Blocking sync fs calls 🐞 Bug ➹ Performance
LocalFileSystemProvider implements async FileSystemProvider methods using synchronous std::fs operations, which can block Tokio worker threads during directory traversal and file reads. This can significantly slow indexing and degrade runtime responsiveness under load or on slow filesystems.
Agent Prompt
### Issue description
`LocalFileSystemProvider` exposes `async` APIs but performs synchronous filesystem calls (`std::fs::*`) directly inside them. When awaited from Tokio tasks (e.g., indexing), these calls block the runtime worker threads.
### Issue Context
This provider is used by `IndexingServiceImpl` for recursive discovery (`read_dir_entries`) and per-file reads (`read_to_string`), so the blocking behavior can be amplified across many files.
### Fix Focus Areas
- crates/mcb-providers/src/fs/mod.rs[58-73]
- crates/mcb-application/src/use_cases/indexing_service.rs[192-230]
- crates/mcb-application/src/use_cases/indexing_service.rs[427-444]
### How to fix
- Replace synchronous calls with `tokio::fs` equivalents where available (`tokio::fs::read_to_string`, `tokio::fs::read_dir`, `tokio::fs::canonicalize`).
- For operations that don’t have good async equivalents (or if you want to keep std::fs), wrap them in `tokio::task::spawn_blocking` and await the result.
- Ensure directory iteration + `file_type()` is also done in the blocking section (or with Tokio’s async directory entries) to avoid partial blocking.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+143
to
+147
| if let Some(ref db) = self.sea_conn { | ||
| let am = org_to_model(org); | ||
| am.update(db) | ||
| .await | ||
| .map_err(|e| Error::memory_with_source("SeaORM update organization", e))?; |
Contributor
There was a problem hiding this comment.
3. Org update overwrites created_at 🐞 Bug ≡ Correctness
The SeaORM update path in SqliteOrgEntityRepository builds an ActiveModel with `created_at: Set(org.created_at) and uses it for update_org, allowing created_at` to be overwritten from the passed Organization value. In the server handler, Organization for update is deserialized from request payload, so a client can accidentally or maliciously change the persisted creation timestamp.
Agent Prompt
### Issue description
`SqliteOrgEntityRepository::update_org` uses SeaORM with an ActiveModel that sets `created_at` from the input entity. This makes `created_at` writable during updates, diverging from the legacy SQL path (which only updates mutable columns) and enabling persisted timestamp corruption.
### Issue Context
- `org_to_model()` sets `created_at: Set(org.created_at)`.
- `update_org()` uses `am.update(db)` when `sea_conn` is present.
- In production wiring, SQLite connects via `SqliteBackend` and `create_org_entity_repository()` selects `new_with_sea`, so the SeaORM path is the default.
- The server update handler deserializes `Organization` from request data without stripping `created_at`.
### Fix Focus Areas
- crates/mcb-providers/src/database/sqlite/org_entity_repository.rs[43-52]
- crates/mcb-providers/src/database/sqlite/org_entity_repository.rs[141-162]
- crates/mcb-providers/src/database/sqlite/provider.rs[69-79]
- crates/mcb-providers/src/database/sqlite/provider.rs[135-146]
- crates/mcb-server/src/handlers/entities/org.rs[69-72]
### How to fix
- When performing the SeaORM update, do not set `created_at` to `Set(...)`.
- Option A: build a separate ActiveModel for updates where `created_at` is `NotSet`/`Unchanged` and only mutable fields are `Set`.
- Option B: fetch the existing row first and preserve the DB value explicitly.
- Add a regression test covering the SeaORM path: create org, then call `update_org` with a different `created_at`, and assert `created_at` in DB is unchanged.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+34
to
+36
| fn spawn(&self, task: BoxFuture<'static, ()>) -> Result<()> { | ||
| tokio::spawn(task); | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
4. Spawn panics without runtime 🐞 Bug ☼ Reliability
TokioTaskRunnerProvider::spawn calls tokio::spawn and always returns Ok(()), so if called on a thread without an entered Tokio runtime it will panic instead of returning an error. Callers (e.g., IndexingServiceImpl) attempt to handle Err from spawn, but this implementation cannot report that failure mode.
Agent Prompt
### Issue description
`TokioTaskRunnerProvider::spawn` can panic when no Tokio runtime is active on the calling thread, but the port contract is fallible (`Result<()>`). This makes error handling ineffective and can crash the process in alternate composition roots/tests/future call sites.
### Issue Context
The port explicitly documents returning an error if the task cannot be spawned, and `IndexingServiceImpl` checks for `Err` from `spawn()`.
### Fix Focus Areas
- crates/mcb-providers/src/task/mod.rs[33-38]
- crates/mcb-domain/src/ports/providers/task.rs[5-11]
- crates/mcb-application/src/use_cases/indexing_service.rs[299-307]
### How to fix
- Use `tokio::runtime::Handle::try_current()` inside `spawn()`.
- If it fails, return a domain `Error` (or map to the existing `Result` type) instead of panicking.
- If it succeeds, call `handle.spawn(task)`.
- (Optional) Consider capturing/logging the JoinHandle if you want to observe panics in the background task.
ⓘ 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 248006a |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/mcb-infrastructure/src/di/bootstrap.rs (1)
226-249: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInconsistent error context on newly-added resolver calls.
embedding_resolver/vector_store_resolver/cache_resolver/language_resolverwrap resolution failures withError::configuration(format!("X: {e}"))for a labeled, diagnosable error, but the newly addedevent_bus_resolver.resolve_from_config()?,fs_resolver.resolve_from_config()?, andtask_runner_resolver.resolve_from_config()?(lines 306, 314, 317) propagate bare errors without labeling which provider failed. This makes bootstrap failures harder to diagnose in production logs.♻️ Proposed fix: add labeled context to the new resolver calls
- let event_bus: Arc<dyn EventBusProvider> = event_bus_resolver.resolve_from_config()?; + let event_bus: Arc<dyn EventBusProvider> = event_bus_resolver + .resolve_from_config() + .map_err(|e| mcb_domain::error::Error::configuration(format!("EventBus: {e}")))?; ... - let file_system_provider: Arc<dyn FileSystemProvider> = fs_resolver.resolve_from_config()?; + let file_system_provider: Arc<dyn FileSystemProvider> = fs_resolver + .resolve_from_config() + .map_err(|e| mcb_domain::error::Error::configuration(format!("FileSystem: {e}")))?; let task_runner_resolver = TaskRunnerProviderResolver::new(Arc::clone(&config)); - let task_runner_provider: Arc<dyn TaskRunnerProvider> = - task_runner_resolver.resolve_from_config()?; + let task_runner_provider: Arc<dyn TaskRunnerProvider> = task_runner_resolver + .resolve_from_config() + .map_err(|e| mcb_domain::error::Error::configuration(format!("TaskRunner: {e}")))?;Also applies to: 305-317
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mcb-infrastructure/src/di/bootstrap.rs` around lines 226 - 249, Update the newly added event_bus_resolver, fs_resolver, and task_runner_resolver resolve_from_config calls to map failures through mcb_domain::error::Error::configuration with distinct “EventBus:”, “Filesystem:”, and “TaskRunner:” labels, matching the existing embedding_provider, vector_store_provider, cache_provider, and language_provider error-context pattern.crates/mcb-application/src/use_cases/indexing_service.rs (2)
277-317: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winTask spawn failure silently leaves the operation stuck "in progress" while reporting success.
start_operationregistersoperation_idat line 278 before the spawn attempt. Iftask_runner_provider.spawn(task)fails (line 305), only awarn!is emitted — the function still returnsOk(IndexingResult{ status: INDEXING_STATUS_STARTED, operation_id: Some(operation_id), .. }). Sincecomplete_operationis only invoked from insiderun_indexing_task(which never runs),get_status()will reportis_indexing: trueindefinitely and the caller has no way to know indexing never started.🐛 Proposed fix: fail the operation and surface the error when spawn fails
if let Err(e) = self.task_runner_provider.spawn(task) { warn!("Failed to spawn indexing background task: {}", e); + self.indexing_ops.complete_operation(&operation_id); + return Ok(IndexingResult { + files_processed: 0, + chunks_created: 0, + files_skipped: 0, + errors: vec![format!("Failed to spawn indexing task: {e}")], + operation_id: Some(operation_id), + status: "failed".to_owned(), + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mcb-application/src/use_cases/indexing_service.rs` around lines 277 - 317, Update the task spawn failure branch in the indexing method around indexing_ops.start_operation and run_indexing_task so it marks operation_id as failed or otherwise completes/removes the in-progress operation, then returns an error instead of logging a warning and returning INDEXING_STATUS_STARTED. Preserve the immediate success response only when task_runner_provider.spawn succeeds.
192-230: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore safe filesystem traversal semantics.
The stack visit does not deduplicate paths and only skips directories by name, so any symlink loop is an unbounded traversal. The entry type also has no
is_symlinkflag, andLocalFileSystemProvideronly reportsis_file/is_dir, so callers cannot avoid following symlink directories. Add a visited-set or depth cap and either preserve symlink awareness inDirEntry/read_dir_entriesor explicitly skip directory symlinks if ignored links are the intended contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mcb-application/src/use_cases/indexing_service.rs` around lines 192 - 230, Update discover_files to prevent unbounded traversal by tracking visited directory paths or enforcing a traversal depth limit, while preserving recursive discovery. Extend DirEntry and LocalFileSystemProvider::read_dir_entries to expose symlink status, then explicitly skip directory symlinks (or otherwise enforce the intended ignored-link contract) before pushing entries onto dirs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mcb-domain/src/entities/api_key.rs`:
- Around line 30-52: Add schema-sync test coverage for the ApiKey table
definition, asserting the expected table columns, indexes, and both user_id and
org_id foreign keys through ApiKey::table_def() or Schema::definition(). Follow
the existing schema assertion pattern and ensure the test detects drift in all
declared ApiKey schema elements.
In `@crates/mcb-domain/src/entities/team.rs`:
- Around line 80-98: Update schema_entity_sync_tests.rs to explicitly exempt
computed or non-persisted entity fields from schema synchronization checks,
including TeamMember::id derived from team_id and user_id. Preserve validation
for all persisted fields while ensuring this intentional field is not reported
as schema drift.
In `@crates/mcb-domain/src/ports/infrastructure/database.rs`:
- Line 82: Remove the missing_docs suppression in database.rs and add
documentation to every public factory method in the database infrastructure
port. In macros.rs, stop generating allow(dead_code) from sea_entity! and make
the generated items legitimately reachable through the module’s public API or
usages. Then remove the module-level dead_code suppression in
sea_entities/mod.rs and resolve all resulting warnings without adding new
suppression directives.
In `@crates/mcb-domain/src/registry/event_bus.rs`:
- Line 1: Remove the blanket missing-docs suppression and document each public
provider configuration type and its provider and extra fields. Apply this in
crates/mcb-domain/src/registry/event_bus.rs:1,
crates/mcb-domain/src/registry/fs.rs:1,
crates/mcb-domain/src/registry/task_runner.rs:1, and
crates/mcb-domain/src/registry/vcs.rs:1 for EventBusProviderConfig,
FileSystemProviderConfig, TaskRunnerProviderConfig, and VcsProviderConfig
respectively; alternatively, retain each directive only with a one-line
guideline-compliant justification.
- Around line 1-20: Introduce a shared define_provider_config! macro that
generates the standard provider config struct with provider and extra fields,
then replace the duplicated definitions in
crates/mcb-domain/src/registry/event_bus.rs:1-20,
crates/mcb-domain/src/registry/fs.rs:1-20,
crates/mcb-domain/src/registry/task_runner.rs:1-20, and
crates/mcb-domain/src/registry/vcs.rs:1-20 with macro invocations for
EventBusProviderConfig, FileSystemProviderConfig, TaskRunnerProviderConfig, and
VcsProviderConfig respectively; preserve each file’s existing
impl_config_builder! and impl_registry! usage.
In `@crates/mcb-domain/src/schema/definition.rs`:
- Around line 49-79: Extend schema_entity_sync_tests.rs with FK, index, and
unique-constraint synchronization assertions for User, Team, TeamMember, and
ApiKey, matching the existing Organization coverage. Ensure each migrated
entity’s expected metadata is compared against its legacy schema output so
missing or dropped definitions fail CI.
In `@crates/mcb-domain/tests/unit/schema_entity_sync_tests.rs`:
- Around line 221-232: Remove the #[test] annotation from
report_divergence_mitigation and relocate its printed guidance into appropriate
static documentation, such as a doc comment or README. Do not retain a test that
only emits instructional text and performs no schema interaction or assertions.
- Around line 11-79: Convert the diagnostic-only tests print_full_schema_report
and detect_schema_only_fields into regression checks that fail when
schema/entity divergences are detected. In detect_schema_only_fields, assert
that the known schema-only fields such as repositories.origin_context,
worktrees.org_id/project_id/origin_context, and branches.origin_context are
absent or otherwise match the module’s expected entity mapping, while retaining
useful diagnostics in failure messages. Replace or supplement
print_full_schema_report’s unconditional reporting with assertions for the
schema invariants it is intended to validate.
In `@crates/mcb-infrastructure/src/di/provider_resolvers.rs`:
- Around line 406-409: Update VcsProviderResolver::resolve_from_config,
FileSystemProviderResolver::resolve_from_config, and
TaskRunnerProviderResolver::resolve_from_config to read their provider names
from self.config.system.infrastructure, matching
EventBusProviderResolver::resolve_from_config, instead of discarding config or
calling hardcoded defaults. Add any missing fields to the typed configuration
model and populate them in development.yaml, test.yaml, and production.yaml,
preserving the existing provider-resolution behavior for configured values.
In `@crates/mcb-infrastructure/src/di/vcs.rs`:
- Around line 62-67: Update VcsRepositoryProvider::repository_id to resolve the
provider that owns or manages the supplied repo instead of unconditionally using
self.providers.first(). Preserve the repo.id() fallback when no matching
provider exists, and delegate repository_id only to the identified provider.
- Around line 12-15: Add a one-line documented justification to the missing_docs
suppression on DynamicVcsProvider and the corresponding default_vcs_provider
declaration, preserving both suppressions while explaining why documentation is
intentionally omitted.
- Around line 33-123: Cache the provider selected by provider_and_repo_for_path
per repository path, using synchronized path-to-Arc<dyn VcsProvider> state on
DynamicVcsProvider. Update provider_and_repo_for_path to consult the cache
before iterating providers and store successful resolutions, then ensure
list_branches, commit_history, list_files, read_file, and diff_refs reuse the
cached provider without reopening the repository solely to rediscover it.
In `@crates/mcb-providers/src/database/sqlite/org_entity_repository.rs`:
- Around line 21-28: The SQLite repository and row-conversion modules exceed the
source-size guideline and should be split into focused submodules. In
crates/mcb-providers/src/database/sqlite/org_entity_repository.rs#L21-L28,
extract organization, user, team, and API-key functionality into per-entity
modules, keeping SeaORM-versus-executor dispatch localized to the organization
module; update the shared repository exports and preserve existing behavior. In
crates/mcb-providers/src/database/sqlite/row_convert.rs#L244-L409, move the
FromRow implementations into domain modules such as org, plan, and vcs, then
re-export them through row_convert/mod.rs without changing conversion behavior.
- Around line 66-85: Add tests that construct SqliteOrgEntityRepository via
new_with_sea (or an equivalent provider-backed setup) and exercise create_org,
get_org, list_orgs, update_org, and delete_org through the SeaORM connection
path. Keep existing new(...) coverage intact and assert the operations’ expected
results.
- Around line 43-64: Update org_to_model so created_at is not marked Set when
constructing the ActiveModel used by update_org; preserve creation-time
assignment through the insert-specific path while ensuring SeaORM updates leave
the immutable timestamp untouched.
In `@crates/mcb-providers/src/database/sqlite/provider.rs`:
- Around line 139-145: The organization repository construction in provider.rs
must use an explicit typed backend/provider capability instead of downcasting
through Any or silently falling back to SqliteOrgEntityRepository::new; update
the repository factory contract so the SeaORM-backed path is selected
declaratively and invalid backend combinations are not accepted. In backend.rs,
remove the Any-based escape hatch and its related implementation once the typed
factory contract owns this capability.
- Around line 73-78: The connect flow currently opens a separate SeaORM
connection that may not share the schema initialized by connect_and_init. Update
connect to reuse the initialized SQLx pool for the SeaORM backend, or
consistently configure both connections with a shared-cache in-memory URI,
ensuring SqliteOrgEntityRepository::create_org sees the initialized
organizations table.
In `@crates/mcb-providers/src/events/nats.rs`:
- Around line 52-62: The NATS provider factory currently hardcodes the fallback
URL in its configuration parsing. Add a typed NATS URL field to the
configuration model, populate it in every profile configuration, and update the
URL selection in the NATS provider factory to use that field instead of the
literal fallback; keep the existing override behavior for explicitly supplied
values.
- Around line 49-71: Update create_nats_event_bus_provider and the provider
resolution flow used by EventBusProviderResolver::resolve_from_config() and
async init_app() so NATS construction does not call Tokio Handle::block_on while
a runtime is active. Propagate the async factory through the registry, or return
a lazily connecting provider, while preserving the existing URL, subject,
client_name, and error behavior.
In `@crates/mcb-providers/src/fs/mod.rs`:
- Around line 34-73: Update LocalFileSystemProvider::read_dir_entries and its
read_dir_sync helper so blocking directory I/O does not run directly on Tokio
worker threads. Either implement directory iteration with tokio::fs or execute
the existing read_dir_sync work through tokio::task::spawn_blocking, preserving
the current DirEntry construction and Error conversion behavior.
In `@crates/mcb-providers/src/lib.rs`:
- Around line 91-96: Remove the unjustified missing-documentation suppressions
by documenting the affected provider modules and symbols. In
crates/mcb-providers/src/lib.rs lines 91-96, document the fs and task modules;
in crates/mcb-providers/src/fs/mod.rs lines 23-32, document
LocalFileSystemProvider and its impl; and in
crates/mcb-providers/src/task/mod.rs lines 21-31, document
TokioTaskRunnerProvider and its impl. If any suppression remains, add the
required one-line justification directly with that directive.
In `@crates/mcb-providers/src/task/mod.rs`:
- Around line 33-38: Update TokioTaskRunnerProvider::spawn to retain and observe
the JoinHandle returned by tokio::spawn, ensuring task panics or join failures
are surfaced through the existing error-reporting mechanism instead of being
silently discarded. Preserve the current Result<()> API and background-task
behavior for successful tasks.
In `@crates/mcb/src/cli/validate.rs`:
- Line 153: Update the text-printing helpers used by validate execution to
return io::Result, including the writeln! calls around the affected output
paths, and propagate each stdout write failure with ?. Do not discard errors; if
broken pipes are intentionally tolerated, explicitly handle only that error
while propagating all others.
---
Outside diff comments:
In `@crates/mcb-application/src/use_cases/indexing_service.rs`:
- Around line 277-317: Update the task spawn failure branch in the indexing
method around indexing_ops.start_operation and run_indexing_task so it marks
operation_id as failed or otherwise completes/removes the in-progress operation,
then returns an error instead of logging a warning and returning
INDEXING_STATUS_STARTED. Preserve the immediate success response only when
task_runner_provider.spawn succeeds.
- Around line 192-230: Update discover_files to prevent unbounded traversal by
tracking visited directory paths or enforcing a traversal depth limit, while
preserving recursive discovery. Extend DirEntry and
LocalFileSystemProvider::read_dir_entries to expose symlink status, then
explicitly skip directory symlinks (or otherwise enforce the intended
ignored-link contract) before pushing entries onto dirs.
In `@crates/mcb-infrastructure/src/di/bootstrap.rs`:
- Around line 226-249: Update the newly added event_bus_resolver, fs_resolver,
and task_runner_resolver resolve_from_config calls to map failures through
mcb_domain::error::Error::configuration with distinct “EventBus:”,
“Filesystem:”, and “TaskRunner:” labels, matching the existing
embedding_provider, vector_store_provider, cache_provider, and language_provider
error-context pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11952fe0-0ef5-4a92-87f1-f8375059f0a7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (63)
Cargo.tomlcrates/mcb-application/src/use_cases/indexing_service.rscrates/mcb-domain/Cargo.tomlcrates/mcb-domain/src/entities/api_key.rscrates/mcb-domain/src/entities/organization.rscrates/mcb-domain/src/entities/team.rscrates/mcb-domain/src/entities/user.rscrates/mcb-domain/src/macros/di.rscrates/mcb-domain/src/macros/mod.rscrates/mcb-domain/src/macros/schema.rscrates/mcb-domain/src/ports/infrastructure/database.rscrates/mcb-domain/src/ports/mod.rscrates/mcb-domain/src/ports/providers/fs.rscrates/mcb-domain/src/ports/providers/http.rscrates/mcb-domain/src/ports/providers/mod.rscrates/mcb-domain/src/ports/providers/task.rscrates/mcb-domain/src/registry/event_bus.rscrates/mcb-domain/src/registry/fs.rscrates/mcb-domain/src/registry/mod.rscrates/mcb-domain/src/registry/task_runner.rscrates/mcb-domain/src/registry/vcs.rscrates/mcb-domain/src/schema/api_keys.rscrates/mcb-domain/src/schema/definition.rscrates/mcb-domain/src/schema/mod.rscrates/mcb-domain/src/schema/organizations.rscrates/mcb-domain/src/schema/team_members.rscrates/mcb-domain/src/schema/teams.rscrates/mcb-domain/src/schema/types.rscrates/mcb-domain/src/schema/users.rscrates/mcb-domain/tests/unit/mod.rscrates/mcb-domain/tests/unit/schema_entity_sync_tests.rscrates/mcb-infrastructure/src/config/types/system.rscrates/mcb-infrastructure/src/di/bootstrap.rscrates/mcb-infrastructure/src/di/database_resolver.rscrates/mcb-infrastructure/src/di/mod.rscrates/mcb-infrastructure/src/di/modules/domain_services.rscrates/mcb-infrastructure/src/di/provider_resolvers.rscrates/mcb-infrastructure/src/di/test_factory.rscrates/mcb-infrastructure/src/di/vcs.rscrates/mcb-infrastructure/tests/integration/di/dispatch_tests.rscrates/mcb-infrastructure/tests/unit/error/error_ext_tests.rscrates/mcb-infrastructure/tests/unit/routing/router_tests.rscrates/mcb-infrastructure/tests/utils/fs_guards.rscrates/mcb-infrastructure/tests/utils/workspace.rscrates/mcb-providers/Cargo.tomlcrates/mcb-providers/src/database/sqlite/backend.rscrates/mcb-providers/src/database/sqlite/macros.rscrates/mcb-providers/src/database/sqlite/mod.rscrates/mcb-providers/src/database/sqlite/org_entity_repository.rscrates/mcb-providers/src/database/sqlite/plan_entity_repository.rscrates/mcb-providers/src/database/sqlite/provider.rscrates/mcb-providers/src/database/sqlite/row_convert.rscrates/mcb-providers/src/database/sqlite/sea_entities/mod.rscrates/mcb-providers/src/database/sqlite/sea_entities/organization.rscrates/mcb-providers/src/database/sqlite/vcs_entity_repository.rscrates/mcb-providers/src/events/nats.rscrates/mcb-providers/src/events/tokio.rscrates/mcb-providers/src/fs/mod.rscrates/mcb-providers/src/lib.rscrates/mcb-providers/src/task/mod.rscrates/mcb-providers/src/vcs/git2_provider.rscrates/mcb-providers/src/vector_store/milvus.rscrates/mcb/src/cli/validate.rs
💤 Files with no reviewable changes (7)
- crates/mcb-domain/src/schema/api_keys.rs
- crates/mcb-domain/src/schema/organizations.rs
- crates/mcb-domain/src/ports/providers/http.rs
- crates/mcb-domain/src/schema/team_members.rs
- crates/mcb-domain/src/schema/teams.rs
- crates/mcb-domain/src/schema/users.rs
- crates/mcb-domain/Cargo.toml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.toml
📄 CodeRabbit inference engine (.cursor/rules/mcb.mdc)
Enforce strict Rust lints:
unsafe_code = "deny",dead_code = "deny",unused_imports = "deny"
Files:
crates/mcb-providers/Cargo.tomlCargo.toml
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
No TODOs, stubs, fakes, fallbacks, compat wrappers, or 'temporary' workarounds. No suppression directives (# type: ignore, blanket # noqa,
@ts-ignore, eslint-disable, etc.) and no escape-hatch typing (Any, bare object, unchecked casts) unless carrying a one-line documented justification.
Files:
crates/mcb-providers/Cargo.tomlCargo.tomlcrates/mcb-domain/src/registry/event_bus.rscrates/mcb-infrastructure/tests/utils/workspace.rscrates/mcb-providers/src/database/sqlite/sea_entities/mod.rscrates/mcb-domain/src/entities/organization.rscrates/mcb-infrastructure/src/di/test_factory.rscrates/mcb-domain/src/ports/providers/task.rscrates/mcb-domain/src/macros/di.rscrates/mcb-domain/src/entities/user.rscrates/mcb-providers/src/lib.rscrates/mcb-domain/src/registry/vcs.rscrates/mcb-providers/src/database/sqlite/sea_entities/organization.rscrates/mcb-infrastructure/tests/utils/fs_guards.rscrates/mcb-domain/src/registry/mod.rscrates/mcb-domain/src/registry/fs.rscrates/mcb-providers/src/database/sqlite/backend.rscrates/mcb-domain/src/entities/api_key.rscrates/mcb-providers/src/vector_store/milvus.rscrates/mcb-infrastructure/tests/unit/routing/router_tests.rscrates/mcb-infrastructure/tests/integration/di/dispatch_tests.rscrates/mcb-domain/tests/unit/mod.rscrates/mcb-providers/src/events/nats.rscrates/mcb-domain/src/macros/mod.rscrates/mcb-domain/src/registry/task_runner.rscrates/mcb-providers/src/database/sqlite/macros.rscrates/mcb-domain/src/entities/team.rscrates/mcb-infrastructure/tests/unit/error/error_ext_tests.rscrates/mcb-providers/src/vcs/git2_provider.rscrates/mcb-providers/src/task/mod.rscrates/mcb-providers/src/fs/mod.rscrates/mcb-domain/src/macros/schema.rscrates/mcb-providers/src/database/sqlite/mod.rscrates/mcb-infrastructure/src/config/types/system.rscrates/mcb-domain/src/schema/types.rscrates/mcb-providers/src/events/tokio.rscrates/mcb/src/cli/validate.rscrates/mcb-domain/src/ports/mod.rscrates/mcb-infrastructure/src/di/mod.rscrates/mcb-domain/src/schema/mod.rscrates/mcb-infrastructure/src/di/vcs.rscrates/mcb-domain/src/ports/infrastructure/database.rscrates/mcb-domain/tests/unit/schema_entity_sync_tests.rscrates/mcb-infrastructure/src/di/database_resolver.rscrates/mcb-infrastructure/src/di/provider_resolvers.rscrates/mcb-domain/src/ports/providers/mod.rscrates/mcb-providers/src/database/sqlite/plan_entity_repository.rscrates/mcb-domain/src/schema/definition.rscrates/mcb-providers/src/database/sqlite/provider.rscrates/mcb-providers/src/database/sqlite/row_convert.rscrates/mcb-infrastructure/src/di/modules/domain_services.rscrates/mcb-providers/src/database/sqlite/vcs_entity_repository.rscrates/mcb-application/src/use_cases/indexing_service.rscrates/mcb-domain/src/ports/providers/fs.rscrates/mcb-infrastructure/src/di/bootstrap.rscrates/mcb-providers/src/database/sqlite/org_entity_repository.rs
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Version, MSRV, workspace members, lint policy are defined in Cargo.toml. When a static document disagrees with Cargo.toml or code, trust the executable source first and update the doc.
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit inference engine (.windsurfrules)
**/*.rs: Error handling: Use Error::vcs("msg") constructors, never unwrap()/expect() in production code
Lints: Set unsafe_code = "deny" and dead_code = "deny". Zero clippy warnings required
Testing: Runmake test(1700+ tests). New logic must include tests
MVI 200: Keep source files under approximately 200 lines; split into submodules when growing larger
**/*.rs: Usethiserrorconstructors for error handling (e.g.,Error::vcs("msg")) and never useunwrap()in production code
Use linkme for compile-time dependency injection and Handle pattern for hot-swap dependency injection, as specified in ADR-050
Keep source files under ~200 lines; split into submodules when growing beyond that size
Fix all warnings and clippy issues and leave tests passing after every change cycle
**/*.rs: Use the most restrictive type that compiles in Rust. No Any, no bare object, no suppression of type errors. Fix types at the source.
Depend on abstractions (protocols/interfaces); inject collaborators; no hidden globals or hard-wired construction inside business logic.
Use the most restrictive type that compiles. No Any, no bare object, no suppression of type errors. Fix types at the source; depend on declared contracts.
Enforce dependency rules: mcb-domain has zero internal dependencies; mcb-providers implements domain ports; mcb-infrastructure handles composition; mcb-server is entrypoint; mcb-utils is a leaf crate.
Keep imports ordered: std, external crates, mcb_* crates, local modules.
Files:
crates/mcb-domain/src/registry/event_bus.rscrates/mcb-infrastructure/tests/utils/workspace.rscrates/mcb-providers/src/database/sqlite/sea_entities/mod.rscrates/mcb-domain/src/entities/organization.rscrates/mcb-infrastructure/src/di/test_factory.rscrates/mcb-domain/src/ports/providers/task.rscrates/mcb-domain/src/macros/di.rscrates/mcb-domain/src/entities/user.rscrates/mcb-providers/src/lib.rscrates/mcb-domain/src/registry/vcs.rscrates/mcb-providers/src/database/sqlite/sea_entities/organization.rscrates/mcb-infrastructure/tests/utils/fs_guards.rscrates/mcb-domain/src/registry/mod.rscrates/mcb-domain/src/registry/fs.rscrates/mcb-providers/src/database/sqlite/backend.rscrates/mcb-domain/src/entities/api_key.rscrates/mcb-providers/src/vector_store/milvus.rscrates/mcb-infrastructure/tests/unit/routing/router_tests.rscrates/mcb-infrastructure/tests/integration/di/dispatch_tests.rscrates/mcb-domain/tests/unit/mod.rscrates/mcb-providers/src/events/nats.rscrates/mcb-domain/src/macros/mod.rscrates/mcb-domain/src/registry/task_runner.rscrates/mcb-providers/src/database/sqlite/macros.rscrates/mcb-domain/src/entities/team.rscrates/mcb-infrastructure/tests/unit/error/error_ext_tests.rscrates/mcb-providers/src/vcs/git2_provider.rscrates/mcb-providers/src/task/mod.rscrates/mcb-providers/src/fs/mod.rscrates/mcb-domain/src/macros/schema.rscrates/mcb-providers/src/database/sqlite/mod.rscrates/mcb-infrastructure/src/config/types/system.rscrates/mcb-domain/src/schema/types.rscrates/mcb-providers/src/events/tokio.rscrates/mcb/src/cli/validate.rscrates/mcb-domain/src/ports/mod.rscrates/mcb-infrastructure/src/di/mod.rscrates/mcb-domain/src/schema/mod.rscrates/mcb-infrastructure/src/di/vcs.rscrates/mcb-domain/src/ports/infrastructure/database.rscrates/mcb-domain/tests/unit/schema_entity_sync_tests.rscrates/mcb-infrastructure/src/di/database_resolver.rscrates/mcb-infrastructure/src/di/provider_resolvers.rscrates/mcb-domain/src/ports/providers/mod.rscrates/mcb-providers/src/database/sqlite/plan_entity_repository.rscrates/mcb-domain/src/schema/definition.rscrates/mcb-providers/src/database/sqlite/provider.rscrates/mcb-providers/src/database/sqlite/row_convert.rscrates/mcb-infrastructure/src/di/modules/domain_services.rscrates/mcb-providers/src/database/sqlite/vcs_entity_repository.rscrates/mcb-application/src/use_cases/indexing_service.rscrates/mcb-domain/src/ports/providers/fs.rscrates/mcb-infrastructure/src/di/bootstrap.rscrates/mcb-providers/src/database/sqlite/org_entity_repository.rs
**/*.{rs,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode configuration values in code. Add fields to the typed config model and populate every profile (development.yaml, test.yaml, production.yaml).
Files:
crates/mcb-domain/src/registry/event_bus.rscrates/mcb-infrastructure/tests/utils/workspace.rscrates/mcb-providers/src/database/sqlite/sea_entities/mod.rscrates/mcb-domain/src/entities/organization.rscrates/mcb-infrastructure/src/di/test_factory.rscrates/mcb-domain/src/ports/providers/task.rscrates/mcb-domain/src/macros/di.rscrates/mcb-domain/src/entities/user.rscrates/mcb-providers/src/lib.rscrates/mcb-domain/src/registry/vcs.rscrates/mcb-providers/src/database/sqlite/sea_entities/organization.rscrates/mcb-infrastructure/tests/utils/fs_guards.rscrates/mcb-domain/src/registry/mod.rscrates/mcb-domain/src/registry/fs.rscrates/mcb-providers/src/database/sqlite/backend.rscrates/mcb-domain/src/entities/api_key.rscrates/mcb-providers/src/vector_store/milvus.rscrates/mcb-infrastructure/tests/unit/routing/router_tests.rscrates/mcb-infrastructure/tests/integration/di/dispatch_tests.rscrates/mcb-domain/tests/unit/mod.rscrates/mcb-providers/src/events/nats.rscrates/mcb-domain/src/macros/mod.rscrates/mcb-domain/src/registry/task_runner.rscrates/mcb-providers/src/database/sqlite/macros.rscrates/mcb-domain/src/entities/team.rscrates/mcb-infrastructure/tests/unit/error/error_ext_tests.rscrates/mcb-providers/src/vcs/git2_provider.rscrates/mcb-providers/src/task/mod.rscrates/mcb-providers/src/fs/mod.rscrates/mcb-domain/src/macros/schema.rscrates/mcb-providers/src/database/sqlite/mod.rscrates/mcb-infrastructure/src/config/types/system.rscrates/mcb-domain/src/schema/types.rscrates/mcb-providers/src/events/tokio.rscrates/mcb/src/cli/validate.rscrates/mcb-domain/src/ports/mod.rscrates/mcb-infrastructure/src/di/mod.rscrates/mcb-domain/src/schema/mod.rscrates/mcb-infrastructure/src/di/vcs.rscrates/mcb-domain/src/ports/infrastructure/database.rscrates/mcb-domain/tests/unit/schema_entity_sync_tests.rscrates/mcb-infrastructure/src/di/database_resolver.rscrates/mcb-infrastructure/src/di/provider_resolvers.rscrates/mcb-domain/src/ports/providers/mod.rscrates/mcb-providers/src/database/sqlite/plan_entity_repository.rscrates/mcb-domain/src/schema/definition.rscrates/mcb-providers/src/database/sqlite/provider.rscrates/mcb-providers/src/database/sqlite/row_convert.rscrates/mcb-infrastructure/src/di/modules/domain_services.rscrates/mcb-providers/src/database/sqlite/vcs_entity_repository.rscrates/mcb-application/src/use_cases/indexing_service.rscrates/mcb-domain/src/ports/providers/fs.rscrates/mcb-infrastructure/src/di/bootstrap.rscrates/mcb-providers/src/database/sqlite/org_entity_repository.rs
crates/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/**/*.rs: Use Error constructors and Result aliases from mcb-domain; do not build raw domain errors by hand.
Use ? for propagation. No unwrap(), expect(), panic!(), todo!(), or unimplemented!() in production paths.
Keep first-party source files compact; split modules before they become difficult to review.
Files:
crates/mcb-domain/src/registry/event_bus.rscrates/mcb-infrastructure/tests/utils/workspace.rscrates/mcb-providers/src/database/sqlite/sea_entities/mod.rscrates/mcb-domain/src/entities/organization.rscrates/mcb-infrastructure/src/di/test_factory.rscrates/mcb-domain/src/ports/providers/task.rscrates/mcb-domain/src/macros/di.rscrates/mcb-domain/src/entities/user.rscrates/mcb-providers/src/lib.rscrates/mcb-domain/src/registry/vcs.rscrates/mcb-providers/src/database/sqlite/sea_entities/organization.rscrates/mcb-infrastructure/tests/utils/fs_guards.rscrates/mcb-domain/src/registry/mod.rscrates/mcb-domain/src/registry/fs.rscrates/mcb-providers/src/database/sqlite/backend.rscrates/mcb-domain/src/entities/api_key.rscrates/mcb-providers/src/vector_store/milvus.rscrates/mcb-infrastructure/tests/unit/routing/router_tests.rscrates/mcb-infrastructure/tests/integration/di/dispatch_tests.rscrates/mcb-domain/tests/unit/mod.rscrates/mcb-providers/src/events/nats.rscrates/mcb-domain/src/macros/mod.rscrates/mcb-domain/src/registry/task_runner.rscrates/mcb-providers/src/database/sqlite/macros.rscrates/mcb-domain/src/entities/team.rscrates/mcb-infrastructure/tests/unit/error/error_ext_tests.rscrates/mcb-providers/src/vcs/git2_provider.rscrates/mcb-providers/src/task/mod.rscrates/mcb-providers/src/fs/mod.rscrates/mcb-domain/src/macros/schema.rscrates/mcb-providers/src/database/sqlite/mod.rscrates/mcb-infrastructure/src/config/types/system.rscrates/mcb-domain/src/schema/types.rscrates/mcb-providers/src/events/tokio.rscrates/mcb/src/cli/validate.rscrates/mcb-domain/src/ports/mod.rscrates/mcb-infrastructure/src/di/mod.rscrates/mcb-domain/src/schema/mod.rscrates/mcb-infrastructure/src/di/vcs.rscrates/mcb-domain/src/ports/infrastructure/database.rscrates/mcb-domain/tests/unit/schema_entity_sync_tests.rscrates/mcb-infrastructure/src/di/database_resolver.rscrates/mcb-infrastructure/src/di/provider_resolvers.rscrates/mcb-domain/src/ports/providers/mod.rscrates/mcb-providers/src/database/sqlite/plan_entity_repository.rscrates/mcb-domain/src/schema/definition.rscrates/mcb-providers/src/database/sqlite/provider.rscrates/mcb-providers/src/database/sqlite/row_convert.rscrates/mcb-infrastructure/src/di/modules/domain_services.rscrates/mcb-providers/src/database/sqlite/vcs_entity_repository.rscrates/mcb-application/src/use_cases/indexing_service.rscrates/mcb-domain/src/ports/providers/fs.rscrates/mcb-infrastructure/src/di/bootstrap.rscrates/mcb-providers/src/database/sqlite/org_entity_repository.rs
🔇 Additional comments (40)
crates/mcb-infrastructure/tests/unit/error/error_ext_tests.rs (1)
28-42: LGTM!crates/mcb-infrastructure/tests/unit/routing/router_tests.rs (1)
102-102: LGTM!crates/mcb-infrastructure/tests/utils/fs_guards.rs (1)
10-13: LGTM!Also applies to: 33-36
crates/mcb-infrastructure/tests/utils/workspace.rs (1)
3-6: LGTM!crates/mcb-providers/src/vector_store/milvus.rs (1)
430-432: LGTM!crates/mcb/src/cli/validate.rs (1)
1-3: LGTM!Also applies to: 115-115
crates/mcb-infrastructure/src/di/vcs.rs (1)
125-128: LGTM!crates/mcb-infrastructure/tests/integration/di/dispatch_tests.rs (1)
9-9: LGTM!Also applies to: 199-252
crates/mcb-providers/src/vcs/git2_provider.rs (1)
10-10: LGTM!Also applies to: 20-33
crates/mcb-providers/src/events/tokio.rs (1)
49-67: LGTM!crates/mcb-domain/tests/unit/mod.rs (1)
11-11: LGTM!crates/mcb-providers/src/database/sqlite/mod.rs (1)
11-14: LGTM!Also applies to: 25-29
crates/mcb-providers/src/database/sqlite/plan_entity_repository.rs (1)
7-12: LGTM!Also applies to: 59-75, 139-152, 185-198
crates/mcb-providers/src/database/sqlite/vcs_entity_repository.rs (1)
9-16: LGTM!Also applies to: 75-90, 173-185, 265-277, 354-369
crates/mcb-providers/src/database/sqlite/sea_entities/organization.rs (1)
5-16: 🎯 Functional CorrectnessKeep the SeaORM prelude import.
sea_entity!expands attributes like#[sea_orm(table_name = $table)]inside this module, so thesea_ormpath comes from this import. Removing it would make the generated macro output fail to resolve under the workspaceunused_imports = "deny".> Likely an incorrect or invalid review comment.crates/mcb-domain/src/entities/organization.rs (1)
47-60: LGTM!crates/mcb-domain/src/entities/team.rs (1)
61-78: LGTM!crates/mcb-domain/src/entities/user.rs (1)
53-76: LGTM!Cargo.toml (1)
322-324: LGTM!crates/mcb-providers/Cargo.toml (1)
130-132: LGTM!crates/mcb-providers/src/database/sqlite/org_entity_repository.rs (1)
89-140: LGTM!Also applies to: 166-442
crates/mcb-domain/src/macros/di.rs (1)
6-40: 🎯 Functional CorrectnessNo change needed.
crates/mcb-domain/src/macros/mod.rs (1)
21-23: LGTM!crates/mcb-domain/src/registry/mod.rs (1)
59-64: LGTM!crates/mcb-domain/src/schema/mod.rs (1)
4-30: LGTM!crates/mcb-domain/src/schema/types.rs (1)
119-140: LGTM!crates/mcb-domain/src/macros/schema.rs (1)
204-217: 🎯 Functional CorrectnessNo action needed.
Current
impl_table_schema!entity invocations use the full form or existing shorthand arms; no current schema call usescolumns+indexes+foreign_keyswithoutunique_constraints.crates/mcb-domain/src/ports/mod.rs (1)
62-66: See consolidated comment on HTTP client port removal (anchored atcrates/mcb-domain/src/ports/providers/mod.rs).crates/mcb-application/src/use_cases/indexing_service.rs (2)
111-126: See consolidated comment on undocumented new fields under#[allow(missing_docs)](anchored here).
447-452: 🎯 Functional CorrectnessNo change needed.
The workspace declares Rust 1.92 / edition 2024, so the let-chained
if let Some(repo) = ... && !repo.has_changed(...).await?syntax is supported.crates/mcb-infrastructure/src/di/bootstrap.rs (1)
1-413: See consolidated comment on MVI-200 file-length violations (anchored here).crates/mcb-infrastructure/src/di/provider_resolvers.rs (1)
1-496: See consolidated comment on MVI-200 file-length violations.crates/mcb-domain/src/ports/providers/fs.rs (1)
1-24: LGTM!crates/mcb-domain/src/ports/providers/task.rs (1)
1-13: LGTM!crates/mcb-infrastructure/src/config/types/system.rs (1)
14-15: LGTM!Also applies to: 123-123, 148-148
crates/mcb-infrastructure/src/di/database_resolver.rs (1)
37-53: LGTM!crates/mcb-infrastructure/src/di/mod.rs (1)
49-49: LGTM!Also applies to: 64-66
crates/mcb-infrastructure/src/di/modules/domain_services.rs (1)
100-142: Dependency wiring forfile_system_provider/task_runner_providerthroughServiceDependencies→IndexingServiceInputs→build_indexing_service/create_indexing_serviceis correct and matches downstream usage inAppContext/test_factory.rs. See consolidated comments for the undocumented-fields and file-length concerns.Also applies to: 171-224, 261-283
crates/mcb-infrastructure/src/di/test_factory.rs (1)
64-65: LGTM!crates/mcb-domain/src/ports/providers/mod.rs (1)
33-33: 🎯 Functional CorrectnessNo remaining consumers of the removed HTTP client port.
Comment on lines
+30
to
+52
| crate::impl_table_schema!(ApiKey, "api_keys", | ||
| columns: [ | ||
| ("id", Text, pk), | ||
| ("user_id", Text), | ||
| ("org_id", Text), | ||
| ("key_hash", Text), | ||
| ("name", Text), | ||
| ("scopes_json", Text), | ||
| ("expires_at", Integer, nullable), | ||
| ("created_at", Integer), | ||
| ("revoked_at", Integer, nullable), | ||
| ], | ||
| indexes: [ | ||
| "idx_api_keys_user" => ["user_id"], | ||
| "idx_api_keys_org" => ["org_id"], | ||
| "idx_api_keys_key_hash" => ["key_hash"], | ||
| ], | ||
| foreign_keys: [ | ||
| ("user_id", "users", "id"), | ||
| ("org_id", "organizations", "id"), | ||
| ], | ||
| unique_constraints: [], | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add schema-sync coverage for ApiKey.
This adds columns, indexes, and foreign keys without a corresponding ApiKey::table_def()/Schema::definition() assertion. Add a test covering the table, indexes, and both foreign keys.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb-domain/src/entities/api_key.rs` around lines 30 - 52, Add
schema-sync test coverage for the ApiKey table definition, asserting the
expected table columns, indexes, and both user_id and org_id foreign keys
through ApiKey::table_def() or Schema::definition(). Follow the existing schema
assertion pattern and ensure the test detects drift in all declared ApiKey
schema elements.
Source: Coding guidelines
Comment on lines
+80
to
+98
| crate::impl_table_schema!(TeamMember, "team_members", | ||
| columns: [ | ||
| ("team_id", Text, pk), | ||
| ("user_id", Text, pk), | ||
| ("role", Text), | ||
| ("joined_at", Integer), | ||
| ], | ||
| indexes: [ | ||
| "idx_team_members_team" => ["team_id"], | ||
| "idx_team_members_user" => ["user_id"], | ||
| ], | ||
| foreign_keys: [ | ||
| ("team_id", "teams", "id"), | ||
| ("user_id", "users", "id"), | ||
| ], | ||
| unique_constraints: [ | ||
| ["team_id", "user_id"], | ||
| ], | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd schema.rs crates/mcb-domain/src/macros
fd schema_entity_sync_tests.rs crates/mcb-domain
cat -n crates/mcb-domain/src/macros/schema.rs 2>/dev/null | sed -n '1,200p'
cat -n crates/mcb-domain/tests/unit/schema_entity_sync_tests.rs 2>/dev/null | sed -n '1,200p'Repository: marlonsc/mcb
Length of output: 16450
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'crates/mcb-domain/src/entities/team.rs' 'crates/mcb-domain/src/entities/*.rs' 'crates/mcb-domain/src/schema/**' 'crates/mcb-domain/src/schema.rs' 'crates/mcb-domain/tests/unit/schema_entity_sync_tests.rs' 'crates/mcb-domain/src/macros/schema.rs'
echo "== team.rs outline =="
ast-grep outline crates/mcb-domain/src/entities/team.rs || true
echo "== team.rs relevant =="
cat -n crates/mcb-domain/src/entities/team.rs | sed -n '1,220p'
echo "== schema entity sync tests =="
cat -n crates/mcb-domain/tests/unit/schema_entity_sync_tests.rs | sed -n '1,360p'
echo "== Schema and HasTableSchema usages =="
rg -n "struct Schema|impl Schema|fn definition|HasTableSchema|impl_table_schema|team_members|TeamMember" crates/mcb-domain/src crates/mcb-domain/tests/unit/schema_entity_sync_tests.rsRepository: marlonsc/mcb
Length of output: 21238
Add an explicit test allowance for computed schema fields.
TeamMember::id is derived from team_id:user_id and intentionally has no team_members column. Since entity-owned schema metadata is now the source of truth, add/expand schema_entity_sync_tests.rs with an explicit exemption for computed/non-persisted fields so future schema/entity sync runs don’t treat this as a drift failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb-domain/src/entities/team.rs` around lines 80 - 98, Update
schema_entity_sync_tests.rs to explicitly exempt computed or non-persisted
entity fields from schema synchronization checks, including TeamMember::id
derived from team_id and user_id. Preserve validation for all persisted fields
while ensuring this intentional field is not reported as schema drift.
|
|
||
| /// Provider factory for database connections with schema initialization. | ||
| #[async_trait] | ||
| #[allow(missing_docs)] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove lint suppressions and satisfy the underlying lints.
crates/mcb-domain/src/ports/infrastructure/database.rs#L82-L82: document each public factory method instead of suppressingmissing_docs.crates/mcb-providers/src/database/sqlite/macros.rs#L30-L40: stop emittingallow(dead_code)fromsea_entity!; make generated items legitimately reachable.crates/mcb-providers/src/database/sqlite/sea_entities/mod.rs#L6-L6: remove the module-levelallow(dead_code)after fixing reachability.
As per coding guidelines, “No suppression directives” and “Fix all warnings and clippy issues.”
📍 Affects 3 files
crates/mcb-domain/src/ports/infrastructure/database.rs#L82-L82(this comment)crates/mcb-providers/src/database/sqlite/macros.rs#L30-L40crates/mcb-providers/src/database/sqlite/sea_entities/mod.rs#L6-L6
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb-domain/src/ports/infrastructure/database.rs` at line 82, Remove
the missing_docs suppression in database.rs and add documentation to every
public factory method in the database infrastructure port. In macros.rs, stop
generating allow(dead_code) from sea_entity! and make the generated items
legitimately reachable through the module’s public API or usages. Then remove
the module-level dead_code suppression in sea_entities/mod.rs and resolve all
resulting warnings without adding new suppression directives.
Source: Coding guidelines
| @@ -0,0 +1,20 @@ | |||
| #![allow(missing_docs)] | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
#![allow(missing_docs)] added without justification in 4 new files. Each new registry file suppresses the missing-docs lint on its public ProviderConfig struct instead of documenting the fields or justifying the suppression inline.
crates/mcb-domain/src/registry/event_bus.rs#L1-L1: remove the blanket allow and add doc comments forEventBusProviderConfig/provider/extra, or add a one-line justification per guideline.crates/mcb-domain/src/registry/fs.rs#L1-L1: same forFileSystemProviderConfig.crates/mcb-domain/src/registry/task_runner.rs#L1-L1: same forTaskRunnerProviderConfig.crates/mcb-domain/src/registry/vcs.rs#L1-L1: same forVcsProviderConfig.
As per coding guidelines, **/*.{rs,toml}: "No suppression directives ... unless carrying a one-line documented justification."
📍 Affects 4 files
crates/mcb-domain/src/registry/event_bus.rs#L1-L1(this comment)crates/mcb-domain/src/registry/fs.rs#L1-L1crates/mcb-domain/src/registry/task_runner.rs#L1-L1crates/mcb-domain/src/registry/vcs.rs#L1-L1
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb-domain/src/registry/event_bus.rs` at line 1, Remove the blanket
missing-docs suppression and document each public provider configuration type
and its provider and extra fields. Apply this in
crates/mcb-domain/src/registry/event_bus.rs:1,
crates/mcb-domain/src/registry/fs.rs:1,
crates/mcb-domain/src/registry/task_runner.rs:1, and
crates/mcb-domain/src/registry/vcs.rs:1 for EventBusProviderConfig,
FileSystemProviderConfig, TaskRunnerProviderConfig, and VcsProviderConfig
respectively; alternatively, retain each directive only with a one-line
guideline-compliant justification.
Source: Coding guidelines
Comment on lines
+1
to
+20
| #![allow(missing_docs)] | ||
|
|
||
| use std::collections::HashMap; | ||
|
|
||
| #[derive(Debug, Clone, Default)] | ||
| pub struct EventBusProviderConfig { | ||
| pub provider: String, | ||
| pub extra: HashMap<String, String>, | ||
| } | ||
|
|
||
| crate::impl_config_builder!(EventBusProviderConfig {}); | ||
|
|
||
| crate::impl_registry!( | ||
| provider_trait: crate::ports::EventBusProvider, | ||
| config_type: EventBusProviderConfig, | ||
| entry_type: EventBusProviderEntry, | ||
| slice_name: EVENT_BUS_PROVIDERS, | ||
| resolve_fn: resolve_event_bus_provider, | ||
| list_fn: list_event_bus_providers | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Four new registry files are byte-for-byte identical boilerplate aside from type names. Each defines a {X}ProviderConfig { provider: String, extra: HashMap<String, String> } struct and the same impl_config_builder!/impl_registry! invocation shape; a single macro generating the struct too would remove the remaining duplication that impl_config_builder!/impl_registry! don't already cover.
crates/mcb-domain/src/registry/event_bus.rs#L1-L20: fold theEventBusProviderConfigstruct definition into a shareddefine_provider_config!(EventBusProviderConfig)-style macro (mirroringimpl_config_builder!/impl_registry!).crates/mcb-domain/src/registry/fs.rs#L1-L20: same forFileSystemProviderConfig.crates/mcb-domain/src/registry/task_runner.rs#L1-L20: same forTaskRunnerProviderConfig.crates/mcb-domain/src/registry/vcs.rs#L1-L20: same forVcsProviderConfig.
📍 Affects 4 files
crates/mcb-domain/src/registry/event_bus.rs#L1-L20(this comment)crates/mcb-domain/src/registry/fs.rs#L1-L20crates/mcb-domain/src/registry/task_runner.rs#L1-L20crates/mcb-domain/src/registry/vcs.rs#L1-L20
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb-domain/src/registry/event_bus.rs` around lines 1 - 20, Introduce a
shared define_provider_config! macro that generates the standard provider config
struct with provider and extra fields, then replace the duplicated definitions
in crates/mcb-domain/src/registry/event_bus.rs:1-20,
crates/mcb-domain/src/registry/fs.rs:1-20,
crates/mcb-domain/src/registry/task_runner.rs:1-20, and
crates/mcb-domain/src/registry/vcs.rs:1-20 with macro invocations for
EventBusProviderConfig, FileSystemProviderConfig, TaskRunnerProviderConfig, and
VcsProviderConfig respectively; preserve each file’s existing
impl_config_builder! and impl_registry! usage.
Comment on lines
+52
to
+62
| 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(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Hardcoded config defaults embedded in provider code.
The NATS URL ("nats://127.0.0.1:4222") is a literal fallback baked into the factory rather than sourced from the typed config model / profile files.
As per coding guidelines, "Do not hardcode configuration values in code. Add fields to the typed config model and populate every profile (development.yaml, test.yaml, production.yaml)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb-providers/src/events/nats.rs` around lines 52 - 62, The NATS
provider factory currently hardcodes the fallback URL in its configuration
parsing. Add a typed NATS URL field to the configuration model, populate it in
every profile configuration, and update the URL selection in the NATS provider
factory to use that field instead of the literal fallback; keep the existing
override behavior for explicitly supplied values.
Source: Coding guidelines
Comment on lines
+34
to
+73
| fn read_dir_sync(path: &Path) -> Result<Vec<DirEntry>, Error> { | ||
| let entries = std::fs::read_dir(path).map_err(|e| { | ||
| Error::internal(format!("Failed to read directory {}: {e}", path.display())) | ||
| })?; | ||
|
|
||
| let mut out = Vec::new(); | ||
| for entry in entries { | ||
| let entry = entry | ||
| .map_err(|e| Error::internal(format!("Failed to read directory entry: {e}")))?; | ||
| let file_type = entry | ||
| .file_type() | ||
| .map_err(|e| Error::internal(format!("Failed to read file type: {e}")))?; | ||
|
|
||
| out.push(DirEntry { | ||
| path: entry.path(), | ||
| is_file: file_type.is_file(), | ||
| is_dir: file_type.is_dir(), | ||
| }); | ||
| } | ||
|
|
||
| Ok(out) | ||
| } | ||
| } | ||
|
|
||
| #[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()))) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -a 'mod\.rs$' . | sed 's#^\./##' | rg 'crates/mcb-providers/src/fs/mod.rs|indexing_service|fs' || true
echo "== fs provider outline =="
ast-grep outline crates/mcb-providers/src/fs/mod.rs --view expanded || true
echo "== fs provider relevant sections =="
cat -n crates/mcb-providers/src/fs/mod.rs | sed -n '1,120p'
echo "== searches =="
rg -n "FileSystemProvider|read_dir_entries|read_to_string|canonicalize_path|indexing_service|Tokio|tokio" crates -S || trueRepository: marlonsc/mcb
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== mcb-providers package =="
cat -n crates/mcb-providers/Cargo.toml | sed -n '1,120p'
echo "== domain ports filesystem trait =="
cat -n crates/mcb-domain/src/ports/providers/fs.rs | sed -n '1,120p'
echo "== indexing service file candidates =="
fd -i 'indexing_service\.rs$' crates || true
rg -n "struct .*IndexingService|impl .*IndexingService|read_dir_entries|read_to_string|canonicalize_path|file_system_provider|FileSystemProvider" crates/mcb-infrastructure crates/mcb-server crates/mcb-domain crates/mcb-providers -S --glob '*.rs' | head -200Repository: marlonsc/mcb
Length of output: 15581
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== indexing service outline =="
ast-grep outline crates/mcb-application/src/use_cases/indexing_service.rs --view expanded || true
echo "== relevant indexing service sections =="
wc -l crates/mcb-application/src/use_cases/indexing_service.rs
cat -n crates/mcb-application/src/use_cases/indexing_service.rs | sed -n '1,320p'
echo "== workspace tokio features =="
rg -n '^\[workspace\.dependencies\]|^\ttokio\s*=|features\s*=' Cargo.toml crates/mcb-infrastructure/Cargo.toml crates/mcb-server/Cargo.toml -SRepository: marlonsc/mcb
Length of output: 18617
🌐 Web query:
tokio async runtime documentation blocking IO std fs read_to_string worker threads
💡 Result:
In the Tokio asynchronous runtime, tokio::fs::read_to_string is the asynchronous equivalent of std::fs::read_to_string [1][2]. Because most operating systems lack native asynchronous filesystem APIs, Tokio implements tokio::fs::read_to_string by internally dispatching the blocking operation to a dedicated thread pool using spawn_blocking [3][4][2]. Key technical considerations include: 1. Blocking the Worker Threads: If you use the synchronous std::fs::read_to_string directly inside an async function, you will block the specific Tokio worker thread currently executing that task [5][6]. This prevents the executor from driving other pending futures on that same thread, significantly degrading concurrency and performance [7][5][8][6]. 2. Role of spawn_blocking: By using tokio::fs::read_to_string().await, the operation offloads the blocking I/O work to Tokio's dedicated blocking thread pool [3][4][2]. This allows the async worker thread to yield and continue processing other tasks while waiting for the filesystem operation to complete [6]. 3. Performance Impact: Although tokio::fs avoids blocking the executor's worker threads, it still incurs the overhead of dispatching work to the blocking thread pool [3][4]. For large numbers of small I/O operations, this overhead can be significant [3][6]. To optimize, it is recommended to batch operations into fewer spawn_blocking calls when possible, rather than executing many small, individual blocking operations [3][4]. 4. When to use std::fs: You should generally only use std::fs (synchronous) outside of an asynchronous context, such as during application startup or within code specifically designated to run on a non-async thread [6]. Inside an async function, always prefer tokio::fs or manually wrap blocking code in tokio::task::spawn_blocking [7][6].
Citations:
- 1: https://docs.rs/tokio/latest/tokio/fs/fn.read_to_string.html
- 2: https://github.com/tokio-rs/tokio/blob/master/tokio/src/fs/read_to_string.rs
- 3: https://docs.rs/tokio/latest/tokio/fs/
- 4: https://docs.rs/tokio/latest/tokio/fs/index.html
- 5: ⚡ Bolt: [performance improvement] Replace synchronous
std::fs::read_to_stringwithtokio::fs::read_to_stringin async context opendev-to/opendev#186 - 6: https://github.com/leonardomso/rust-skills/blob/HEAD/rules/async-tokio-fs.md
- 7: https://docs.rs/tokio/latest/tokio/task/index.html
- 8: ⚡ Optimize file_read performance using asynchronous I/O eikarna/hermes-rs#42
Move filesystem I/O out of async worker threads.
read_dir_entries uses blocking std::fs::read_dir and file_type() inside async index_codebase/discover_files, so indexing can block a Tokio worker while other async tasks await. Use tokio::fs/asynchronous directory reads, or wrap the blocking directory reads in tokio::task::spawn_blocking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb-providers/src/fs/mod.rs` around lines 34 - 73, Update
LocalFileSystemProvider::read_dir_entries and its read_dir_sync helper so
blocking directory I/O does not run directly on Tokio worker threads. Either
implement directory iteration with tokio::fs or execute the existing
read_dir_sync work through tokio::task::spawn_blocking, preserving the current
DirEntry construction and Error conversion behavior.
Comment on lines
+91
to
+96
| #[allow(missing_docs)] | ||
| pub mod fs; | ||
|
|
||
| #[allow(missing_docs)] | ||
| pub mod task; | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Unjustified #[allow(missing_docs)] suppressions across new provider code. All three sites add #[allow(missing_docs)] without the one-line justification the coding guidelines require for suppression directives.
crates/mcb-providers/src/lib.rs#L91-L96: either document thefs/taskmodules or add a one-line justification for the suppression.crates/mcb-providers/src/fs/mod.rs#L23-L32: add doc comments toLocalFileSystemProviderand its impl block, or justify the suppression inline.crates/mcb-providers/src/task/mod.rs#L21-L31: add doc comments toTokioTaskRunnerProviderand its impl block, or justify the suppression inline.
As per coding guidelines, "No suppression directives ... unless carrying a one-line documented justification."
📍 Affects 3 files
crates/mcb-providers/src/lib.rs#L91-L96(this comment)crates/mcb-providers/src/fs/mod.rs#L23-L32crates/mcb-providers/src/task/mod.rs#L21-L31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb-providers/src/lib.rs` around lines 91 - 96, Remove the unjustified
missing-documentation suppressions by documenting the affected provider modules
and symbols. In crates/mcb-providers/src/lib.rs lines 91-96, document the fs and
task modules; in crates/mcb-providers/src/fs/mod.rs lines 23-32, document
LocalFileSystemProvider and its impl; and in
crates/mcb-providers/src/task/mod.rs lines 21-31, document
TokioTaskRunnerProvider and its impl. If any suppression remains, add the
required one-line justification directly with that directive.
Source: Coding guidelines
Comment on lines
+33
to
+38
| impl TaskRunnerProvider for TokioTaskRunnerProvider { | ||
| fn spawn(&self, task: BoxFuture<'static, ()>) -> Result<()> { | ||
| tokio::spawn(task); | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Spawned task panics are silently discarded.
tokio::spawn(task)'s JoinHandle is dropped, so a panic inside task is never observed by anything — no log, no propagated error. Since this is the generic task-runner port used for background work (e.g., indexing), a failing task disappears silently.
🔍 Proposed fix to surface join errors
fn spawn(&self, task: BoxFuture<'static, ()>) -> Result<()> {
- tokio::spawn(task);
- Ok(())
+ let handle = tokio::spawn(task);
+ tokio::spawn(async move {
+ if let Err(e) = handle.await {
+ tracing::error!("spawned task panicked or was cancelled: {e}");
+ }
+ });
+ Ok(())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| impl TaskRunnerProvider for TokioTaskRunnerProvider { | |
| fn spawn(&self, task: BoxFuture<'static, ()>) -> Result<()> { | |
| tokio::spawn(task); | |
| Ok(()) | |
| } | |
| } | |
| impl TaskRunnerProvider for TokioTaskRunnerProvider { | |
| fn spawn(&self, task: BoxFuture<'static, ()>) -> Result<()> { | |
| let handle = tokio::spawn(task); | |
| tokio::spawn(async move { | |
| if let Err(e) = handle.await { | |
| tracing::error!("spawned task panicked or was cancelled: {e}"); | |
| } | |
| }); | |
| Ok(()) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb-providers/src/task/mod.rs` around lines 33 - 38, Update
TokioTaskRunnerProvider::spawn to retain and observe the JoinHandle returned by
tokio::spawn, ensuring task panics or join failures are surfaced through the
existing error-reporting mechanism instead of being silently discarded. Preserve
the current Result<()> API and background-task behavior for successful tasks.
|
|
||
| if has_violations { | ||
| println!(); | ||
| let _ = writeln!(std::io::stdout()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate stdout write errors instead of discarding them.
These let _ = writeln!(...) calls can silently truncate validation output while execute still succeeds. Return io::Result from the text-printing helpers and propagate it with ?; if broken pipes are intentionally tolerated, handle only that specific error. As per coding guidelines: Use ? for propagation.
Also applies to: 170-197
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb/src/cli/validate.rs` at line 153, Update the text-printing helpers
used by validate execution to return io::Result, including the writeln! calls
around the affected output paths, and propagate each stdout write failure with
?. Do not discard errors; if broken pipes are intentionally tolerated,
explicitly handle only that error while propagating all others.
Source: Coding guidelines
marlon-costa-dc
enabled auto-merge (squash)
July 29, 2026 03:08
![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 64 files
Confidence score: 2/5
- The highest-risk path is runtime/provider initialization in
crates/mcb-providers/src/events/nats.rs,crates/mcb-providers/src/events/tokio.rs, andcrates/mcb-providers/src/task/mod.rs: selecting NATS can panic viaHandle::block_on, zero-capacity broadcast config can panic, and task spawning can panic outside Tokio, turning recoverable startup errors into crashes—keep these paths fully async, validate capacity (>0), and useHandle::try_current()to return errors instead of panicking. - Database write behavior in
crates/mcb-providers/src/database/sqlite/macros.rsandcrates/mcb-providers/src/database/sqlite/provider.rscan fail in normal flows:organizations.idis treated as auto-increment despite being canonical non-autoincrement, and in-memory mode can route SeaORM to a different empty DB (no such table: organizations)—set the PK metadata to non-autoincrement and reuse the initialized pool across both access paths. - Data integrity can drift in
crates/mcb-domain/src/entities/user.rsandcrates/mcb-domain/src/entities/api_key.rs: upgrades may keep duplicate/orphaned user rows and API keys can reference users from another org, which can leak or misattribute data—add/verify migrations for unique+FK constraints and enforce the(user_id, org_id)relationship at schema level. - Indexing reliability is at risk in
crates/mcb-application/src/use_cases/indexing_service.rsandcrates/mcb-providers/src/fs/mod.rs: spawn failures can still reportstartedand leave operations stuck, blockingstd::fscalls can stall Tokio workers, and ignore rules are bypassed so excluded/generated files get indexed—propagate spawn errors with cleanup, move file I/O totokio::fs/spawn_blocking, and restore.gitignore/.ignorefiltering.
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 async startup panics because this factory calls `Handle::block_on` while `init_app` is already running on the Tokio runtime. The NATS construction path should stay async (for example, via an async registry factory) or defer connection initialization; wrapping it in the current runtime's synchronous `block_on` cannot safely bridge this factory contract.</violation>
</file>
<file name="crates/mcb-providers/src/database/sqlite/macros.rs">
<violation number="1" location="crates/mcb-providers/src/database/sqlite/macros.rs:29">
P2: Generated entities cannot represent canonical composite primary keys because this pattern only designates `$first`; `team_members` and `issue_label_assignments` each have two PK columns. Per-column PK metadata would prevent SeaORM identity operations from becoming incorrect as this LOC-reduction helper is applied to the remaining tables.</violation>
<violation number="2" location="crates/mcb-providers/src/database/sqlite/macros.rs:34">
P1: Organization inserts through the normal `SqliteBackend`/SeaORM path use a generated String PK that SeaORM treats as auto-incrementing, although canonical `organizations.id` is not auto-incremented. Mark this PK `auto_increment = false` (or parameterize it) so ActiveModel insert semantics match the schema.</violation>
<violation number="3" location="crates/mcb-providers/src/database/sqlite/macros.rs:46">
P2: The advertised schema-parity metadata omits PK, nullability, uniqueness, and auto-increment attributes, so entity/schema tests pass despite materially different SeaORM behavior. Including these attributes in the generated metadata would make the synchronization check cover the schema semantics it is meant to protect.</violation>
</file>
<file name="crates/mcb-domain/src/entities/user.rs">
<violation number="1" location="crates/mcb-domain/src/entities/user.rs:74">
P1: Upgraded databases still permit duplicate `(org_id, email)` rows and orphaned `org_id`s because these declarations only affect newly created tables, while `migrate_and_verify_schema` validates columns only. Consider migrating/verifying constraints or forcing a controlled rebuild before accepting the existing schema.</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:202">
P1: Files excluded by `.gitignore`, `.ignore`, or Git exclude rules are now indexed because this raw directory traversal only checks the four names in `SKIP_DIRS`. That can ingest generated/vendor files and content users deliberately excluded from the repository (including ignored secrets), increasing index size and potentially exposing unintended content. The provider-based discovery should preserve the prior `WalkBuilder` ignore semantics, either through an ignore-aware filesystem API or equivalent filtering here.</violation>
<violation number="2" location="crates/mcb-application/src/use_cases/indexing_service.rs:202">
P2: A single unreadable or concurrently removed entry now causes every sibling in that directory to be omitted from indexing. Because `read_dir_entries` collapses entry-level failures into one directory-level error, this loop cannot continue past the bad entry as the previous walker did. Consider returning per-entry results or making the provider retain valid entries while reporting individual failures.</violation>
<violation number="3" location="crates/mcb-application/src/use_cases/indexing_service.rs:305">
P1: When `TaskRunnerProvider::spawn` returns `Err`, the caller still receives a successful `started` result while the operation remains active forever and no indexing occurs. Cleaning up the operation and propagating the spawn error keeps polling state and the API result truthful.</violation>
</file>
<file name="crates/mcb-providers/src/fs/mod.rs">
<violation number="1" location="crates/mcb-providers/src/fs/mod.rs:17">
P3: The new local provider and its distributed-slice registration have no automated coverage, leaving success/error behavior and `"local"` resolution vulnerable to silent regressions. Temp-directory tests for all three methods plus one registry-resolution test would cover this integration boundary.</violation>
<violation number="2" location="crates/mcb-providers/src/fs/mod.rs:61">
P1: Indexing can stall a Tokio executor worker because every async provider method performs blocking `std::fs` I/O directly. Using `tokio::fs` or moving the complete synchronous operations into `spawn_blocking` would keep directory traversal and per-file reads off executor workers.</violation>
<violation number="3" location="crates/mcb-providers/src/fs/mod.rs:62">
P2: Filesystem failures are mislabeled as internal errors and flattened into strings, so callers and telemetry cannot identify I/O failures or inspect their source chain. Wrapping each failure with `Error::io_with_source(..., e)` would preserve the intended error semantics and context.</violation>
</file>
<file name="crates/mcb-providers/src/events/tokio.rs">
<violation number="1" location="crates/mcb-providers/src/events/tokio.rs:52">
P1: A configured event-bus capacity of `0` now panics during provider resolution when `broadcast::channel(0)` is constructed. Since capacity is externally configurable and represented as an unrestricted `usize`, the factory should reject zero with its `Result` rather than allowing startup to abort; parsing errors should likewise be returned instead of silently falling back.</violation>
</file>
<file name="crates/mcb-domain/src/entities/api_key.rs">
<violation number="1" location="crates/mcb-domain/src/entities/api_key.rs:48">
P1: An API key can be persisted under one organization while referencing a user from another, because these independent foreign keys validate existence but not the user/org relationship. Consider enforcing `(user_id, org_id)` against the user's organization (or deriving `org_id` through `users`) to preserve the tenant boundary.</violation>
</file>
<file name="crates/mcb-providers/src/task/mod.rs">
<violation number="1" location="crates/mcb-providers/src/task/mod.rs:35">
P2: Calling this provider outside an entered Tokio runtime panics instead of returning the `Err` promised by `TaskRunnerProvider::spawn`, bypassing the caller's failure handling. Using `Handle::try_current()` preserves the trait contract and lets `indexing_service` log the spawn failure.</violation>
</file>
<file name="crates/mcb-providers/src/database/sqlite/provider.rs">
<violation number="1" location="crates/mcb-providers/src/database/sqlite/provider.rs:73">
P2: With an in-memory SQLite path, organization CRUD now targets a second empty database and fails with `no such table: organizations`. Reusing the initialized SQLx pool for SeaORM also keeps both repository paths on the same connection configuration.</violation>
<violation number="2" location="crates/mcb-providers/src/database/sqlite/provider.rs:144">
P3: Production composition now switches organization CRUD to the SeaORM branch, but existing org and DI tests still exercise only the executor branch. A provider-level CRUD test using `connect` and `create_org_entity_repository` would cover this newly activated path and its interaction with the shared executor.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/provider_resolvers.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/provider_resolvers.rs:351">
P2: NATS startup silently ignores the configured `connection_timeout_ms` and `max_reconnect_attempts`: this translation drops both fields, and the NATS registry builder consequently uses default connect options. The resolver/provider configuration contract should carry and apply both documented settings (or remove/reject them) so operational configuration is not accepted as a no-op.</violation>
<violation number="2" location="crates/mcb-infrastructure/src/di/provider_resolvers.rs:358">
P2: A configured `capacity = 0` now crashes startup when this value reaches `broadcast::channel(0)`. Since the loader currently accepts zero, validating it here (or in config validation) and returning `Error::configuration` would keep invalid event-bus settings on the normal error path.</violation>
</file>
<file name="crates/mcb-domain/tests/unit/schema_entity_sync_tests.rs">
<violation number="1" location="crates/mcb-domain/tests/unit/schema_entity_sync_tests.rs:12">
P3: Three tests have no assertions and will always pass: print_full_schema_report, detect_schema_only_fields, and report_divergence_mitigation. They print diagnostic info but never fail, which undermines test reliability and gives false confidence. Either add assertions, convert to debug helpers (e.g. #[cfg(debug_assertions)] with a non-test name), or remove them.</violation>
<violation number="2" location="crates/mcb-domain/tests/unit/schema_entity_sync_tests.rs:52">
P2: Schema/entity divergences do not fail this test: each detected mismatch is only printed, so CI still reports success (and normally hides the captured output). If these fields are intentionally outstanding, this should be a non-test report; otherwise the collected mismatches should be asserted empty so future drift is actually detected.</violation>
<violation number="3" location="crates/mcb-domain/tests/unit/schema_entity_sync_tests.rs:167">
P3: Misleading assertion message: the message claims the labels column should be Text (JSON) but the assertion only checks whether a column named "labels" exists in the schema — it never verifies the type. Update the message or add a type assertion.</violation>
<violation number="4" location="crates/mcb-domain/tests/unit/schema_entity_sync_tests.rs:180">
P2: This “sync” check compares `Organization::table_def()` with another call to that exact same method: `Schema::definition()` registers `from_entity!(Organization)`. It therefore passes even if the manually listed schema diverges from the `Organization` fields. An independent expectation (or compile-time/generated field metadata) is needed for this to test entity/schema synchronization; otherwise this test only verifies the aggregator wiring.</violation>
<violation number="5" location="crates/mcb-domain/tests/unit/schema_entity_sync_tests.rs:217">
P2: Index comparison only checks count equality (assert_eq!(trait_indexes.len(), def_indexes.len())), not name or column content. Two different indexes with the same count would pass undetected. Compare each index's name and columns for a thorough sync check.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/vcs.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/vcs.rs:21">
P2: One registered provider that cannot build from an empty/default config prevents startup even when another VCS provider is usable. The composite could retain successfully constructed providers and fail only when none can initialize, while preserving initialization errors for diagnostics.</violation>
<violation number="2" location="crates/mcb-infrastructure/src/di/vcs.rs:40">
P2: Every branch/history/file/diff request now performs a full metadata-producing repository open solely to rediscover the provider, then the concrete operation opens it again. Retaining the provider selected during the initial open/discovery, or adding a cheap capability probe, would avoid repeated history and branch scans.</violation>
<violation number="3" location="crates/mcb-infrastructure/src/di/vcs.rs:42">
P2: When no provider opens a path, callers receive only the last registry provider's error, potentially hiding the actionable failure from the provider that recognized the repository. Preserve/aggregate probe errors, or continue only for an explicit “not this provider” result.</violation>
<violation number="4" location="crates/mcb-infrastructure/src/di/vcs.rs:63">
P2: With multiple registered VCS implementations, `repository_id` is dispatched to the first registry provider rather than the provider that opened this repository. That ordering is unrelated to the repository’s VCS type and can produce an invalid ID or provider-specific failure. Since `VcsRepository` already carries the ID assigned by its opening provider, returning that stored ID avoids cross-provider dispatch here.</violation>
<violation number="5" location="crates/mcb-infrastructure/src/di/vcs.rs:95">
P3: The default provider now reports `dynamic` instead of the actual VCS type (`git` today), so callers can no longer use `vcs_name()` as specified. Forward the concrete name when unambiguous, or evolve the API to report a type per repository for a multi-VCS composite.</violation>
</file>
<file name="crates/mcb-providers/src/database/sqlite/backend.rs">
<violation number="1" location="crates/mcb-providers/src/database/sqlite/backend.rs:28">
P2: A caller can pair handles for different SQLite databases, causing organization CRUD to use `sea_conn` while the rest of the repository uses `executor`. Since the type cannot validate this invariant, consider deriving both handles from one configuration or making `new` crate-private.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/bootstrap.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/bootstrap.rs:306">
P2: Switching the supplied configuration to `provider = "nats"` passes the default empty `nats_url` to the client instead of using the provider's fallback, causing event-bus initialization to fail. Empty optional URLs should be normalized to `None` or rejected with a targeted configuration error before resolution.</violation>
</file>
<file name="crates/mcb/src/cli/validate.rs">
<violation number="1" location="crates/mcb/src/cli/validate.rs:180">
P2: Text-mode validation can now return success even when stdout fails and the report is only partially or not at all emitted, because all of these `writeln!` results are discarded. JSON mode already propagates the same I/O failure. Consider returning `io::Result<()>` from the text printing helpers and propagating it through `execute` (or explicitly handling only `BrokenPipe` if that is the intended exception).</violation>
</file>
<file name="crates/mcb-providers/src/database/sqlite/org_entity_repository.rs">
<violation number="1" location="crates/mcb-providers/src/database/sqlite/org_entity_repository.rs:76">
P3: Production organization CRUD now follows the SeaORM branches, but the repository tests still construct only `new` and leave every new branch unexecuted. A `new_with_sea` CRUD test should cover updates and the existing foreign-key delete behavior against the production connection setup.</violation>
<violation number="2" location="crates/mcb-providers/src/database/sqlite/org_entity_repository.rs:93">
P2: Organization updates through `new_with_sea` can overwrite the immutable `created_at` value, whereas the executor implementation deliberately leaves it unchanged. `org_to_model` marks every field, including `created_at`, as `Set` before `ActiveModel::update`; consider leaving `created_at` as `NotSet` for updates (or using a separate update model) so both repository paths have the same semantics.</violation>
</file>
<file name="crates/mcb-infrastructure/src/di/database_resolver.rs">
<violation number="1" location="crates/mcb-infrastructure/src/di/database_resolver.rs:38">
P2: Bootstrap now constructs the configured database provider twice, and connects with a different instance from the one used to create repositories. Consider passing the already-resolved provider into the connection path or returning the provider and executor together, avoiding duplicate construction and preserving provider instance state.</violation>
</file>
<file name="crates/mcb-providers/src/database/sqlite/row_convert.rs">
<violation number="1" location="crates/mcb-providers/src/database/sqlite/row_convert.rs:276">
P2: Distinct memberships can receive the same `TeamMemberId` when either source ID contains `:`, because the two strings are concatenated with an ambiguous delimiter. Consider encoding the pair unambiguously (for example, length-prefixing each value or serializing a tuple), while accounting for compatibility with IDs already exposed to clients.</violation>
</file>
<file name="Cargo.toml">
<violation number="1" location="Cargo.toml:323">
P3: This SQLite-only integration pulls SeaORM's default datatype features, adding several unused dependency trees and avoidable compile cost. Disabling defaults and explicitly retaining `macros` keeps the derive-based entities working with a smaller dependency surface.</violation>
</file>
<file name="crates/mcb-domain/src/ports/providers/fs.rs">
<violation number="1" location="crates/mcb-domain/src/ports/providers/fs.rs:11">
P3: A `DirEntry` can represent the impossible state “file and directory,” which makes provider mistakes silently alter discovery because `discover_files` prioritizes `is_dir`. A single file-kind enum (`File`, `Directory`, `Other`) would encode the valid states and make consumers exhaustive.</violation>
<violation number="2" location="crates/mcb-domain/src/ports/providers/fs.rs:22">
P3: `canonicalize_path` is currently dead API, but every filesystem provider or test double must still implement it. Removing it from the port and local adapter would keep the new abstraction limited to operations the application uses.</violation>
</file>
<file name="crates/mcb-domain/src/macros/di.rs">
<violation number="1" location="crates/mcb-domain/src/macros/di.rs:16">
P3: Getter generation is duplicated across all four parsing arms, so future signature, documentation, or body changes can easily diverge between aliases and list endings. A private `@getter` helper arm plus one comma-separated entry parser would centralize the generated method.</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 now omits both new providers, and the struct-wide allow also hides future missing field docs. Prefer documenting `file_system_provider` and `task_runner_provider` (including the `# Fields` list) instead of suppressing `missing_docs` for the whole struct.</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 async startup panics because this factory calls Handle::block_on while init_app is already running on the Tokio runtime. The NATS construction path should stay async (for example, via an async registry factory) or defer connection initialization; wrapping it in the current runtime's synchronous block_on cannot safely bridge this factory contract.
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 async startup panics because this factory calls `Handle::block_on` while `init_app` is already running on the Tokio runtime. The NATS construction path should stay async (for example, via an async registry factory) or defer connection initialization; wrapping it in the current runtime's synchronous `block_on` cannot safely bridge this factory contract.</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>
| #[derive(Clone, Debug, PartialEq, Eq, sea_orm::DeriveEntityModel)] | ||
| #[sea_orm(table_name = $table)] | ||
| pub struct Model { | ||
| #[sea_orm(primary_key)] |
There was a problem hiding this comment.
P1: Organization inserts through the normal SqliteBackend/SeaORM path use a generated String PK that SeaORM treats as auto-incrementing, although canonical organizations.id is not auto-incremented. Mark this PK auto_increment = false (or parameterize it) so ActiveModel insert semantics match the schema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-providers/src/database/sqlite/macros.rs, line 34:
<comment>Organization inserts through the normal `SqliteBackend`/SeaORM path use a generated String PK that SeaORM treats as auto-incrementing, although canonical `organizations.id` is not auto-incremented. Mark this PK `auto_increment = false` (or parameterize it) so ActiveModel insert semantics match the schema.</comment>
<file context>
@@ -0,0 +1,51 @@
+ #[derive(Clone, Debug, PartialEq, Eq, sea_orm::DeriveEntityModel)]
+ #[sea_orm(table_name = $table)]
+ pub struct Model {
+ #[sea_orm(primary_key)]
+ pub $first: $first_ty,
+ $( pub $f: $ty ),*
</file context>
Suggested change
| #[sea_orm(primary_key)] | |
| #[sea_orm(primary_key, auto_increment = false)] |
| ("org_id", "organizations", "id"), | ||
| ], | ||
| unique_constraints: [ | ||
| ["org_id", "email"], |
There was a problem hiding this comment.
P1: Upgraded databases still permit duplicate (org_id, email) rows and orphaned org_ids because these declarations only affect newly created tables, while migrate_and_verify_schema validates columns only. Consider migrating/verifying constraints or forcing a controlled rebuild before accepting the existing schema.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-domain/src/entities/user.rs, line 74:
<comment>Upgraded databases still permit duplicate `(org_id, email)` rows and orphaned `org_id`s because these declarations only affect newly created tables, while `migrate_and_verify_schema` validates columns only. Consider migrating/verifying constraints or forcing a controlled rebuild before accepting the existing schema.</comment>
<file context>
@@ -50,3 +50,27 @@ pub enum UserRole {
+ ("org_id", "organizations", "id"),
+ ],
+ unique_constraints: [
+ ["org_id", "email"],
+ ],
+);
</file context>
Comment on lines
+305
to
+307
| if let Err(e) = self.task_runner_provider.spawn(task) { | ||
| warn!("Failed to spawn indexing background task: {}", e); | ||
| } |
There was a problem hiding this comment.
P1: When TaskRunnerProvider::spawn returns Err, the caller still receives a successful started result while the operation remains active forever and no indexing occurs. Cleaning up the operation and propagating the spawn error keeps polling state and the API result truthful.
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 305:
<comment>When `TaskRunnerProvider::spawn` returns `Err`, the caller still receives a successful `started` result while the operation remains active forever and no indexing occurs. Cleaning up the operation and propagating the spawn error keeps polling state and the API result truthful.</comment>
<file context>
@@ -286,10 +298,14 @@ impl IndexingServiceInterface for IndexingServiceImpl {
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);
+ }
</file context>
Suggested change
| if let Err(e) = self.task_runner_provider.spawn(task) { | |
| warn!("Failed to spawn indexing background task: {}", e); | |
| } | |
| if let Err(e) = self.task_runner_provider.spawn(task) { | |
| self.indexing_ops.complete_operation(&operation_id); | |
| return Err(e); | |
| } |
| @@ -0,0 +1,73 @@ | |||
| use std::path::{Path, PathBuf}; | |||
There was a problem hiding this comment.
P1: Indexing can stall a Tokio executor worker because every async provider method performs blocking std::fs I/O directly. Using tokio::fs or moving the complete synchronous operations into spawn_blocking would keep directory traversal and per-file reads off executor workers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-providers/src/fs/mod.rs, line 61:
<comment>Indexing can stall a Tokio executor worker because every async provider method performs blocking `std::fs` I/O directly. Using `tokio::fs` or moving the complete synchronous operations into `spawn_blocking` would keep directory traversal and per-file reads off executor workers.</comment>
<file context>
@@ -0,0 +1,73 @@
+#[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())))
+ }
</file context>
| @@ -0,0 +1,73 @@ | |||
| use std::path::{Path, PathBuf}; | |||
There was a problem hiding this comment.
P3: The new local provider and its distributed-slice registration have no automated coverage, leaving success/error behavior and "local" resolution vulnerable to silent regressions. Temp-directory tests for all three methods plus one registry-resolution test would cover this integration boundary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-providers/src/fs/mod.rs, line 17:
<comment>The new local provider and its distributed-slice registration have no automated coverage, leaving success/error behavior and `"local"` resolution vulnerable to silent regressions. Temp-directory tests for all three methods plus one registry-resolution test would cover this integration boundary.</comment>
<file context>
@@ -0,0 +1,73 @@
+}
+
+#[linkme::distributed_slice(FILE_SYSTEM_PROVIDERS)]
+static LOCAL_FILE_SYSTEM_PROVIDER: FileSystemProviderEntry = FileSystemProviderEntry {
+ name: "local",
+ description: "Local file-system provider",
</file context>
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct DirEntry { | ||
| pub path: PathBuf, | ||
| pub is_file: bool, |
There was a problem hiding this comment.
P3: A DirEntry can represent the impossible state “file and directory,” which makes provider mistakes silently alter discovery because discover_files prioritizes is_dir. A single file-kind enum (File, Directory, Other) would encode the valid states and make consumers exhaustive.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-domain/src/ports/providers/fs.rs, line 11:
<comment>A `DirEntry` can represent the impossible state “file and directory,” which makes provider mistakes silently alter discovery because `discover_files` prioritizes `is_dir`. A single file-kind enum (`File`, `Directory`, `Other`) would encode the valid states and make consumers exhaustive.</comment>
<file context>
@@ -0,0 +1,23 @@
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub struct DirEntry {
+ pub path: PathBuf,
+ pub is_file: bool,
+ pub is_dir: bool,
+}
</file context>
| @@ -0,0 +1,41 @@ | |||
| /// Generates `Arc`-cloning getter methods for DI container fields. | |||
There was a problem hiding this comment.
P3: Getter generation is duplicated across all four parsing arms, so future signature, documentation, or body changes can easily diverge between aliases and list endings. A private @getter helper arm plus one comma-separated entry parser would centralize the generated method.
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 16:
<comment>Getter generation is duplicated across all four parsing arms, so future signature, documentation, or body changes can easily diverge between aliases and list endings. A private `@getter` helper arm plus one comma-separated entry parser would centralize the generated method.</comment>
<file context>
@@ -0,0 +1,41 @@
+ $crate::arc_getters! { $($rest)* }
+ };
+
+ ($name:ident : $ty:ty => $field:ident $(= $impl:ty)? $(,)?) => {
+ #[doc = concat!("Get `", stringify!($name), "`.")]
+ #[must_use]
</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 now omits both new providers, and the struct-wide allow also hides future missing field docs. Prefer documenting file_system_provider and task_runner_provider (including the # Fields list) instead of suppressing missing_docs for the whole struct.
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 now omits both new providers, and the struct-wide allow also hides future missing field docs. Prefer documenting `file_system_provider` and `task_runner_provider` (including the `# Fields` list) instead of suppressing `missing_docs` for the whole struct.</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>
|
|
||
| async fn read_dir_entries(&self, path: &Path) -> std::result::Result<Vec<DirEntry>, Error>; | ||
|
|
||
| async fn canonicalize_path(&self, path: &Path) -> std::result::Result<PathBuf, Error>; |
There was a problem hiding this comment.
P3: canonicalize_path is currently dead API, but every filesystem provider or test double must still implement it. Removing it from the port and local adapter would keep the new abstraction limited to operations the application uses.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-domain/src/ports/providers/fs.rs, line 22:
<comment>`canonicalize_path` is currently dead API, but every filesystem provider or test double must still implement it. Removing it from the port and local adapter would keep the new abstraction limited to operations the application uses.</comment>
<file context>
@@ -0,0 +1,23 @@
+
+ async fn read_dir_entries(&self, path: &Path) -> std::result::Result<Vec<DirEntry>, Error>;
+
+ async fn canonicalize_path(&self, path: &Path) -> std::result::Result<PathBuf, Error>;
+}
</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.