Feature/mcb metrics removal and test cleanup - #153
Conversation
… (-913 lines) Dead test code removed (compiler-proven unused): - test_utils.rs: 11 dead functions (create_test_crate_with_extra_file, create_test_crate_with_tests, create_file_at_path, ensure_workspace_structure, create_constants_file, create_null_provider_file, assert_violation_count, assert_min_violations, assert_violation_at, load_fixture, setup_fixture_crate) - test_constants.rs: 22 dead constants (APPLICATION_CRATE, INFRASTRUCTURE_CRATE, PROVIDERS_CRATE, VALIDATE_CRATE, ERROR_RS, CONSTANTS_RS, NULL_RS, and 15 more) Unused cargo dependencies stripped (cargo-machete verified): - mcb-infrastructure: r2d2, r2d2_postgres, tree-sitter-c-sharp, tree-sitter-kotlin-ng, criterion, flate2, tar, seahash, pulldown-cmark, html-escape, hostname, nix - mcb-server: async-nats, flate2, tar, fs2 - mcb-validate: criterion, garde, pest, pest_derive, jsonpath-rust, once_cell, rusty-rules - mcb-ast-utils: tokio, tracing - mcb-language-support: tracing Also applied crud_adapter.rs macro consolidation from previous session. Part of epic mcb-b9qd (v0.2.2 modernization plan).
…in mcb-providers - Add #[allow(unsafe_code)] before #[linkme::distributed_slice(PROJECT_DETECTORS)] in: - crates/mcb-providers/src/git/project_detection/cargo.rs - crates/mcb-providers/src/git/project_detection/go.rs - crates/mcb-providers/src/git/project_detection/maven.rs - crates/mcb-providers/src/git/project_detection/npm.rs - crates/mcb-providers/src/git/project_detection/python.rs - Add missing doc comments to sqlite repository structs and constructors: - SqliteVcsEntityRepository - SqliteIssueEntityRepository - SqliteOrgEntityRepository - SqlitePlanEntityRepository - Add module-level doc comments in sqlite/mod.rs for public modules Fixes -D unsafe-code lint errors and missing documentation warnings.
- Simplified search and filter conditions in `apply_filter_pipeline` by using `Option::filter` to eliminate unnecessary checks for empty values. - Enhanced readability and maintainability of the code by consolidating logic for sorting and filtering. - Updated environment variable handling in test helpers with `#[allow(unsafe_code)]` to suppress warnings. This refactor improves code clarity and reduces potential errors in filtering logic.
…ate CI/CD workflows - Downgraded versions of several packages (mcb, mcb-application, mcb-ast-utils, mcb-domain, mcb-infrastructure, mcb-language-support, mcb-providers, mcb-server, mcb-validate) from 0.2.2-dev to 0.2.1-dev in Cargo.lock and Cargo.toml. - Updated the Codecov action version in CI workflow from v5.4.3 to v5.5.2 for improved functionality. - Adjusted release workflow to use extracted version and tag for consistency in release naming. These changes ensure compatibility with previous versions and enhance CI/CD reliability.
- Removed the `vcs_indexing_tests` module and its references due to obsolescence. - Updated the `unit.rs` test file to comment out the `vcs_indexing_tests` path, indicating its removal in version 0.2.2. - Enhanced documentation in various modules to clarify the purpose of fields and improve maintainability. - Added module-level documentation for better context in the `test_services_config` and `utils` modules.
…code - Remove prometheus, metrics, opentelemetry deps from 5 crate Cargo.tomls - Simplify crud_adapter.rs (134→80 lines) - Remove 70 dead test constants and 249 lines of unused test utils - Fix issue_entity_repository column reference - Clean sqlite module exports - Regenerate Cargo.lock after dependency removal
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
✨ 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 |
PR Summary by QodoRemove unused dependencies and dead test helpers; refactor CRUD adapters
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
36 rules 1. #[allow(unsafe_code)] lacks justification
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| /// Generates a CRUD adapter struct wrapping a repository trait object. | ||
| macro_rules! impl_crud_adapter { | ||
| ($name:ident($repo_ty:ty) { $($methods:item)* }) => { | ||
| struct $name(Arc<$repo_ty>); | ||
|
|
||
| #[async_trait] | ||
| impl EntityCrudAdapter for $name { | ||
| $($methods)* | ||
| } | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
1. crud_adapter.rs exceeds 200 lines 📘 Rule violation ⚙ Maintainability
The modified file crates/mcb-server/src/admin/crud_adapter.rs is well over the ~200 line limit, increasing maintenance risk and making future changes harder to review. This violates the guideline to split large source files into submodules when they grow beyond ~200 lines.
Agent Prompt
## Issue description
`crates/mcb-server/src/admin/crud_adapter.rs` is a modified source file that exceeds the ~200 line limit and should be split to improve maintainability.
## Issue Context
The PR adds code to this already-large file, but the repository guideline requires splitting files that exceed ~200 lines.
## Fix Focus Areas
- crates/mcb-server/src/admin/crud_adapter.rs[141-152]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| //! - **Assertions**: `assert_no_violations`, | ||
| //! `assert_has_violation_matching`, `assert_no_violation_from_file` | ||
| #![allow(dead_code)] | ||
|
|
There was a problem hiding this comment.
2. test_utils.rs exceeds 200 lines 📘 Rule violation ⚙ Maintainability
The modified file crates/mcb-validate/tests/test_utils.rs exceeds the ~200 line limit, making it harder to navigate and maintain as it grows. It should be split into smaller focused modules/files.
Agent Prompt
## Issue description
`crates/mcb-validate/tests/test_utils.rs` is a modified source file that exceeds the ~200 line limit and should be split into smaller files/modules.
## Issue Context
Even though this is test code, it is still a source file and the guideline applies to modified files to prevent large monolithic modules.
## Fix Focus Areas
- crates/mcb-validate/tests/test_utils.rs[9-20]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /// Application layer test crate name. | ||
| pub const APPLICATION_CRATE: &str = "my-application"; | ||
|
|
||
| /// Infrastructure layer test crate name. | ||
| pub const INFRASTRUCTURE_CRATE: &str = "my-infrastructure"; | ||
|
|
||
| /// Providers layer test crate name. | ||
| pub const PROVIDERS_CRATE: &str = "my-providers"; | ||
|
|
||
| /// Validate crate name (self-referential). | ||
| pub const VALIDATE_CRATE: &str = "my-validate"; | ||
|
|
There was a problem hiding this comment.
3. test_constants.rs exceeds 200 lines 📘 Rule violation ⚙ Maintainability
The modified file crates/mcb-validate/tests/test_constants.rs still exceeds the ~200 line limit after the cleanup, which increases maintenance burden for test infrastructure. It should be split into smaller modules grouped by purpose (crate names, paths, rule IDs, etc.).
Agent Prompt
## Issue description
`crates/mcb-validate/tests/test_constants.rs` is a modified source file that exceeds the ~200 line limit and should be split into smaller modules.
## Issue Context
The PR removes a large block of constants but the file is still >200 lines; splitting will keep future test refactors simpler.
## Fix Focus Areas
- crates/mcb-validate/tests/test_constants.rs[21-32]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| /// Helper to set env var safely. | ||
| #[allow(unsafe_code)] | ||
| /// Helper to set env var safely | ||
| fn set_env(key: &str, value: &str) { |
There was a problem hiding this comment.
1. #[allow(unsafe_code)] lacks justification 📘 Rule violation ⚙ Maintainability
#[allow(unsafe_code)] is used without an inline justification comment on the same line or immediately following line. This violates the requirement to document why a static-analysis suppression is necessary, making future audits and maintenance harder.
Agent Prompt
## Issue description
`#[allow(unsafe_code)]` is a suppression directive but it has no inline justification comment on the same line or the next line, as required.
## Issue Context
This appears to be needed due to Rust 2024 requiring `unsafe` for `env::set_var`/`env::remove_var` in tests, but that rationale must be recorded inline next to the suppression.
## Fix Focus Areas
- crates/mcb-infrastructure/tests/unit/config_figment_tests.rs[20-22]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 15c4c6b |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 12 files
Confidence score: 4/5
- In
crates/mcb-infrastructure/tests/unit/config_figment_tests.rs, the unsafe environment-variable access is still unsound even with#[serial], so concurrent test execution can trigger undefined behavior or flaky results that hide real regressions — remove the unsafe env mutation pattern or isolate those cases in a single-threaded subprocess-based test path. - In
crates/mcb-providers/src/database/sqlite/mod.rs, making implementation modules public duplicates already re-exported repository APIs and expands the crate’s semver surface unnecessarily, which can lock in internal layout and complicate future refactors — keep the modules private and expose only the intended re-exports. - In
crates/mcb-server/src/admin/crud_adapter.rs, theas_ref()plusSome(ref field)combination creates an avoidable&&String, which can obscure intent and rely on coercions that may break with later signature changes — simplify the binding to a single-reference form.
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-infrastructure/tests/unit/config_figment_tests.rs">
<violation number="1" location="crates/mcb-infrastructure/tests/unit/config_figment_tests.rs:23">
P2: These unsafe calls are not made sound merely by serializing the tagged tests: `#[serial]` does not make the shared `unit` test binary single-threaded, and the Rust safety contract also forbids concurrent environment reads outside `std::env`, not only concurrent mutations. Since this binary includes many ordinary and Tokio tests, this new safety rationale does not establish the required invariant on non-Windows targets. Consider running these cases in an actually single-threaded process (and enforcing that in the test command), or isolating environment mutation in subprocesses; the safety comment should state the full invariant that the implementation enforces.</violation>
</file>
<file name="crates/mcb-providers/src/database/sqlite/mod.rs">
<violation number="1" location="crates/mcb-providers/src/database/sqlite/mod.rs:14">
P3: The repository types are already available through the public re-exports, so making these two implementation modules public adds duplicate API paths and unnecessarily makes the module layout part of the crate's compatibility surface. This is also inconsistent with the other SQLite repository modules in this file. Keeping the modules private while retaining the existing `pub use` exports would preserve the intended API.</violation>
</file>
<file name="crates/mcb-server/src/admin/crud_adapter.rs">
<violation number="1" location="crates/mcb-server/src/admin/crud_adapter.rs:229">
P3: The sort-field binding now adds an unnecessary second reference: `as_ref()` already yields `Option<&String>`, while `Some(ref field)` makes `field` an `&&String`. Deref coercions/blanket implementations may hide this in the current calls, but it makes the type and intent harder to follow; matching with `Some(field)` keeps the field as `&String`.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
| /// Helper to set env var safely | ||
| fn set_env(key: &str, value: &str) { | ||
| // SAFETY: Tests must run with --test-threads=1 | ||
| // SAFETY: Tests run serially (#[serial]) — no concurrent env mutation. |
There was a problem hiding this comment.
P2: These unsafe calls are not made sound merely by serializing the tagged tests: #[serial] does not make the shared unit test binary single-threaded, and the Rust safety contract also forbids concurrent environment reads outside std::env, not only concurrent mutations. Since this binary includes many ordinary and Tokio tests, this new safety rationale does not establish the required invariant on non-Windows targets. Consider running these cases in an actually single-threaded process (and enforcing that in the test command), or isolating environment mutation in subprocesses; the safety comment should state the full invariant that the implementation enforces.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-infrastructure/tests/unit/config_figment_tests.rs, line 23:
<comment>These unsafe calls are not made sound merely by serializing the tagged tests: `#[serial]` does not make the shared `unit` test binary single-threaded, and the Rust safety contract also forbids concurrent environment reads outside `std::env`, not only concurrent mutations. Since this binary includes many ordinary and Tokio tests, this new safety rationale does not establish the required invariant on non-Windows targets. Consider running these cases in an actually single-threaded process (and enforcing that in the test command), or isolating environment mutation in subprocesses; the safety comment should state the full invariant that the implementation enforces.</comment>
<file context>
@@ -17,19 +17,19 @@ use std::env;
-/// Helper to set env var safely
fn set_env(key: &str, value: &str) {
- // SAFETY: Tests must run with --test-threads=1
+ // SAFETY: Tests run serially (#[serial]) — no concurrent env mutation.
unsafe {
env::set_var(key, value);
</file context>
| @@ -10,9 +10,11 @@ mod agent_repository; | |||
| mod ddl; | |||
There was a problem hiding this comment.
P3: The repository types are already available through the public re-exports, so making these two implementation modules public adds duplicate API paths and unnecessarily makes the module layout part of the crate's compatibility surface. This is also inconsistent with the other SQLite repository modules in this file. Keeping the modules private while retaining the existing pub use exports would preserve the intended API.
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/mod.rs, line 14:
<comment>The repository types are already available through the public re-exports, so making these two implementation modules public adds duplicate API paths and unnecessarily makes the module layout part of the crate's compatibility surface. This is also inconsistent with the other SQLite repository modules in this file. Keeping the modules private while retaining the existing `pub use` exports would preserve the intended API.</comment>
<file context>
@@ -10,9 +10,11 @@ mod agent_repository;
pub mod executor;
-mod issue_entity_repository;
+/// SQLite-backed issue entity repository implementation.
+pub mod issue_entity_repository;
mod memory_repository;
-mod org_entity_repository;
</file context>
| // Sort | ||
| if let Some(ref field) = params.sort_field | ||
| && valid_sort_fields.contains(field.as_str()) | ||
| if let Some(ref field) = params |
There was a problem hiding this comment.
P3: The sort-field binding now adds an unnecessary second reference: as_ref() already yields Option<&String>, while Some(ref field) makes field an &&String. Deref coercions/blanket implementations may hide this in the current calls, but it makes the type and intent harder to follow; matching with Some(field) keeps the field as &String.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/src/admin/crud_adapter.rs, line 229:
<comment>The sort-field binding now adds an unnecessary second reference: `as_ref()` already yields `Option<&String>`, while `Some(ref field)` makes `field` an `&&String`. Deref coercions/blanket implementations may hide this in the current calls, but it makes the type and intent harder to follow; matching with `Some(field)` keeps the field as `&String`.</comment>
<file context>
@@ -216,8 +226,10 @@ fn apply_filter_pipeline(
// Sort
- if let Some(ref field) = params.sort_field
- && valid_sort_fields.contains(field.as_str())
+ if let Some(ref field) = params
+ .sort_field
+ .as_ref()
</file context>
| if let Some(ref field) = params | |
| if let Some(field) = params |
2 participants
No description provided.