Feature/v0 4 0 multitenant weaviate - #156
Open
marlon-costa-dc wants to merge 147 commits into
Open
Conversation
…ine-tables debug, linker opt-in) for faster local builds
…matched_files) + workflow_dispatch recovery — fixes the v0.3.0 failure mode where one failed build leg blocked the whole GitHub Release
…oss (was 100% cold), isolated ci-coverage key for tarpaulin (no thrash), cache-on-failure:true on all rust-cache jobs
…on save-if:false reusers and only matters where the cache is written (lint, test-cross, coverage), keeping warm deps across failed iterations without poisoning correctness (cargo fingerprinting handles partial caches)
…extest and typos-cli
… + cargo-nextest runner with cargo test fallback + sccache opt-in + install both hooks; archive broken .pre-commit-config.yaml
…test-cross/golden jobs — make test auto-detects and uses nextest for faster parallel test runs
…t for the bilingual codebase (Portuguese terms, crate names, fixtures excluded) — gate catches new typos going forward
…NTS.md; add orchestrate skill encoding the coordinator/executor + safety-doctrine loop
…rated export, not source of truth); all bead changes via bd CLI only
…d to beads (v0.3.1 milestone epic mcb-i7o4, v0.3.2 epic mcb-v5an); non-canonical task tracking replaced by bd
…lds/deploys); always monitor or pick up independent non-blocking work
…n only via pull_request — stops duplicate push+PR runs and run cascading on branches with an open PR
…step; one self-paced 5-min loop per session; lane separation + delegate via subagents per sub-bead with quality gates
…— nextest runs a process per test so the shared process-wide OnceLock context inits concurrently and fails ('shared test context init failed'); max-threads=1 restores cargo-test semantics
…rical Decision') so the ADR validator passes — superseded-by-ADR-034 status stays in the header + body
…ans) and allowlist 'flext' — unblocks the lint typos gate on PR #135 (the words came from archived legacy docs, not active source)
…st context instead of swallowing via .ok()? — golden job (cargo test THREADS=2) reported only generic 'shared test context init failed' (mcb-v5an.16); create_shared_test_context now returns Result<_,String> with the true FastEmbed/EdgeVec cause (no hidden failures)
…utes 60->90 — measured: the cold cache-seeding run hit the 60-min cap and was cancelled mid-run before rust-cache could save, creating a deadlock (perpetually cold -> always times out); 90 lets one run complete and seed the cache, after which runs finish well under it (mcb-lcia)
…(.dolt/, .beads-credential-key, .beads/proxieddb/) + opt out the intentional AGENTS.md<->CLAUDE.md doc-divergence (CLAUDE.md is a thin pointer to the AGENTS.md SSOT); reduces bd doctor 11->3 warnings (remaining: shared-server phantom-db restart + optional plugin, tracked in mcb-okxs)
…h into single shared_app_context (AGENTS.md §3.5 no-wrapper, YAGNI/SSOT) — after the Result refactor the two fns became byte-identical aliases; keep one, update real_providers callers; net -9 LOC
…al context - Changed implementation_status to "Historical snapshot; see bd for live work" in ADRs 008, 009, 010, 012, 014, 015, 016, 017, 018, 019, 021, 022, 023, 027, 028, 031, 032, 033, 035, 038, 039, 040, 041, 042, 043, 044, 045, 047, 049, 051, and 053. - Updated README.md to clarify that ADR status reflects historical snapshots and not current implementation states. - Adjusted phase-9/README.md to indicate that original execution notes are historical design context. - Revised ROADMAP.md to emphasize that current work is tracked in beads and not in the roadmap. - Enhanced CHANGELOG.md to document changes related to the v0.3.2 release. - Updated INTEGRATION_TESTS.md to clarify tracking of service availability reporting and conditional test groups in beads.
…te (mcb-xku4) — cargo-tarpaulin's default ptrace engine intermittently aborts with 'Test failed during run' on tokio/multi-threaded test binaries even when every test passes (observed run 27105549947: all tests green, tarpaulin still errored). Coverage is a metric, not a correctness gate; LINT/TEST_LINUX/TEST_CROSS/VALIDATE/AUDIT/GOLDEN/RELEASE_BUILD stay required. Root-cause --engine llvm fix tracked in mcb-xgji
…ng validators to fix 180s timeout
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Important Review skippedToo many files! This PR contains 461 files, which is 361 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (24)
📒 Files selected for processing (491)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
PR Summary by Qodov0.4.0: Weaviate vector store + org-scoped multitenancy + route/bootstrap refactor
AI Description
Diagram
High-Level Assessment
Files changed (28)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
36 rules 1. Weaviate tenants never applied
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| @@ -1 +0,0 @@ | |||
| Subproject commit c1b8409a45c5dc20de91e331ee0cbb86fb9a72d0 | |||
Contributor
There was a problem hiding this comment.
1. Edits under third-party/ 📘 Rule violation § Compliance
The PR removes vendored submodules under third-party/ without any explicit request authorizing changes in that directory. This violates the rule prohibiting modifications under third-party/ unless explicitly requested.
Agent Prompt
## Issue description
Files under `third-party/` were modified (submodules removed) without an explicit request authorizing changes there.
## Issue Context
The compliance rule forbids any modifications under `third-party/` unless the PR/request explicitly calls for it.
## Fix Focus Areas
- .gitmodules[1-2]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
29
to
+33
| pub fn load_app_config() -> Result<AppConfig> { | ||
| let env_name = std::env::var("LOCO_ENV").unwrap_or_else(|_| "test".to_owned()); | ||
| let env_name = std::env::var("LOCO_ENV").unwrap_or_else(|_| { | ||
| tracing::warn!("LOCO_ENV not set; defaulting to 'test'"); | ||
| "test".to_owned() | ||
| }); |
Contributor
There was a problem hiding this comment.
2. loco_env defaulted to test 📘 Rule violation ☼ Reliability
load_app_config() continues startup/config initialization when LOCO_ENV is missing by defaulting to test, rather than failing fast. This can lead to running with the wrong environment configuration.
Agent Prompt
## Issue description
`load_app_config()` treats missing `LOCO_ENV` as non-fatal and falls back to `test`.
## Issue Context
Per the compliance rule, required external configuration must cause an explicit startup failure when missing.
## Fix Focus Areas
- crates/mcb-infrastructure/src/config/loader.rs[29-33]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+47
to
+54
| /// Router handle, kept to ensure the service remains valid. | ||
| #[allow(dead_code)] | ||
| router: AxumRouter, | ||
| /// Abort handle for the background serve task. | ||
| abort_handle: tokio::task::AbortHandle, | ||
| /// Temp directory with the isolated database. | ||
| #[allow(dead_code)] | ||
| temp_dir: TempDir, |
Contributor
There was a problem hiding this comment.
3. #[allow(dead_code)] in testserver 📘 Rule violation ⚙ Maintainability
The new TestServer struct adds #[allow(dead_code)], bypassing dead-code enforcement instead of removing/using the unused items. This violates the requirement to avoid dead code and not suppress dead_code.
Agent Prompt
## Issue description
`#[allow(dead_code)]` was introduced on fields in `TestServer`, which bypasses dead-code enforcement.
## Issue Context
The compliance rule requires no dead code and disallows adding `#[allow(dead_code)]` to bypass the lint.
## Fix Focus Areas
- crates/mcb-server/tests/utils/test_server.rs[47-54]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+126
to
+128
| /// Print the service availability summary to stdout. | ||
| #[allow(clippy::print_stdout)] | ||
| pub fn print_service_availability_summary() { |
Contributor
There was a problem hiding this comment.
4. Unjustified #[allow(clippy::print_stdout)] 📘 Rule violation ⚙ Maintainability
A clippy suppression directive is used without an inline justification comment. This reduces auditability of why the suppression is necessary.
Agent Prompt
## Issue description
A static-analysis suppression (`#[allow(...)]`) is missing an inline justification comment.
## Issue Context
Suppressions must have a specific, non-empty justification on the same line or immediately following line.
## Fix Focus Areas
- crates/mcb-domain/src/utils/tests/service_detection.rs[126-128]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+9
to
+22
| use std::net::SocketAddr; | ||
| use std::sync::Arc; | ||
|
|
||
| use anyhow::Result; | ||
| use axum::Router as AxumRouter; | ||
| use mcb_domain::entities::ApiKey; | ||
| use mcb_domain::utils::tests::utils::{create_test_admin_user, create_test_organization}; | ||
| use mcb_server::axum_routes; | ||
| use mcb_server::state::McbState; | ||
| use mcb_utils::constants::auth::API_KEY_HEADER; | ||
| use reqwest::Client; | ||
| use tempfile::TempDir; | ||
|
|
||
| use crate::utils::domain_services::create_real_domain_services; |
Contributor
There was a problem hiding this comment.
5. Rust imports mis-grouped 📘 Rule violation ⚙ Maintainability
The use statements are not grouped/ordered as required: an external crate import (reqwest, tempfile) appears after mcb_* imports. This violates the required std → external → mcb_* → local ordering.
Agent Prompt
## Issue description
Rust `use` imports are not grouped/ordered per the required std, external, mcb_*, local sequence.
## Issue Context
Mixed grouping increases diff churn and reduces readability.
## Fix Focus Areas
- crates/mcb-server/tests/utils/test_server.rs[9-22]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+240
to
+274
| fn drop(&mut self) { | ||
| self.abort_handle.abort(); | ||
| } | ||
| } | ||
|
|
||
| /// Test helper: assert that a response body contains the expected substring. | ||
| /// | ||
| /// # Errors | ||
| /// Returns the underlying `reqwest` error if reading the response body fails. | ||
| /// | ||
| /// # Panics | ||
| /// Panics if the body does not contain `expected`. | ||
| pub async fn assert_body_contains(response: reqwest::Response, expected: &str) -> Result<()> { | ||
| let text = response.text().await?; | ||
| assert!( | ||
| text.contains(expected), | ||
| "expected body to contain {expected:?}; got: {text}" | ||
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[tokio::test] | ||
| async fn test_server_starts_and_serves_public_asset() -> Result<()> { | ||
| let Some(server) = TestServer::new().await else { | ||
| return Ok(()); | ||
| }; | ||
| let response = server.get("/ui/theme.css").await?; | ||
| assert_eq!(response.status(), reqwest::StatusCode::OK); | ||
| Ok(()) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
6. test_server.rs exceeds 200 lines 📘 Rule violation ⚙ Maintainability
The newly added source file is ~274 lines long, exceeding the ~200 line guideline. This should be split into smaller modules/helpers to keep files maintainable.
Agent Prompt
## Issue description
A new/modified source file exceeds the ~200 line limit.
## Issue Context
Large files become harder to review and maintain; the rule expects refactoring into submodules.
## Fix Focus Areas
- crates/mcb-server/tests/utils/test_server.rs[240-274]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+83
to
+93
| /// Start the Axum application on an ephemeral loopback port. | ||
| async fn start(router: AxumRouter, state: McbState, temp_dir: TempDir) -> Option<Self> { | ||
| let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.ok()?; | ||
| let addr: SocketAddr = listener.local_addr().ok()?; | ||
| let base_url = format!("http://{addr}"); | ||
|
|
||
| let client = Client::builder() | ||
| .timeout(std::time::Duration::from_secs(30)) | ||
| .build() | ||
| .ok()?; | ||
|
|
Contributor
There was a problem hiding this comment.
7. testserver::start swallows errors 📘 Rule violation ☼ Reliability
TestServer::start() converts multiple fallible operations to Option via .ok()?, discarding the underlying error details without logging/propagating them. This can hide real failures and make test failures/CI flakes harder to diagnose.
Agent Prompt
## Issue description
Fallible calls in `TestServer::start()` are converted with `.ok()?`, which silently discards error information.
## Issue Context
The compliance rule requires errors to be propagated, logged, or explicitly handled with justification.
## Fix Focus Areas
- crates/mcb-server/tests/utils/test_server.rs[83-93]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+20
to
+37
| let collections = response | ||
| .get("classes") | ||
| .and_then(Value::as_array) | ||
| .map(|arr| { | ||
| arr.iter() | ||
| .filter_map(|c| c.get("class").and_then(Value::as_str)) | ||
| .map(|name| CollectionInfo::new(name, 0, 0, None, self.provider_name())) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default(); | ||
| Ok(collections) | ||
| } | ||
|
|
||
| async fn list_file_paths( | ||
| &self, | ||
| collection: &CollectionId, | ||
| limit: usize, | ||
| ) -> Result<Vec<FileInfo>> { |
Contributor
There was a problem hiding this comment.
8. Weaviate collection id mismatch 🐞 Bug ≡ Correctness
WeaviateVectorStoreProvider::list_collections returns Weaviate schema class names and passes them into CollectionInfo::new, which derives a CollectionId from that string; this does not round-trip back to the CollectionId used by class_name(). Server code later reconstructs CollectionId from collection.name to call list_vectors, causing it to query a different Weaviate class and return empty/incorrect results.
Agent Prompt
## Issue description
`list_collections()` currently emits Weaviate *class* names as `CollectionInfo.name`, which the server later feeds into `CollectionId::from_string(...)` and then back into provider methods. Because `class_name(CollectionId)` is not invertible via `CollectionId::from_string(class_name)`, the browse/list-all code will call `list_vectors()` with the wrong CollectionId and therefore query the wrong Weaviate class.
## Issue Context
Weaviate class names are derived from `CollectionId` by prefixing and sanitizing the UUID string. `CollectionInfo::new()` derives `id` from the provided name, and server browse APIs reconstruct `CollectionId` from `collection.name`.
## Fix Focus Areas
- crates/mcb-providers/src/vector_store/weaviate/browser.rs[15-31]
- crates/mcb-providers/src/vector_store/weaviate/client.rs[61-72]
- crates/mcb-domain/src/value_objects/browse/collection.rs[25-42]
- crates/mcb-server/src/controllers/web/browse.rs[38-52]
- crates/mcb-server/src/controllers/collections_api.rs[59-73]
## Suggested fix
In `WeaviateVectorStoreProvider::list_collections`, only include classes that match the provider’s naming scheme, and convert them back to the *underlying* collection identifier string (UUID) used elsewhere.
Example approach:
1. For each returned class name:
- Verify it starts with `WEAVIATE_CLASS_PREFIX`.
- Strip the prefix.
- Replace `_` back to `-` (since the source is a UUID string).
- Validate it parses as a UUID.
2. Construct `CollectionInfo` such that:
- `CollectionInfo.name` is that UUID string (so `CollectionId::from_string(&name)` round-trips).
- `CollectionInfo.id` is `CollectionId::from_string(&uuid_str)` (construct the struct directly rather than using `CollectionInfo::new` if needed).
This aligns Weaviate behavior with providers like Qdrant that use the UUID string as the backend collection name.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+20
to
+30
| async fn collection_exists(&self, name: &CollectionId) -> Result<bool> { | ||
| let class = Self::class_name(name); | ||
| match self | ||
| .request(reqwest::Method::GET, &format!("/v1/schema/{class}"), None) | ||
| .await | ||
| { | ||
| Ok(_) => Ok(true), | ||
| // A missing class is a legitimate "does not exist" answer, not a | ||
| // transport failure: Weaviate returns 404 for unknown classes. | ||
| Err(_) => Ok(false), | ||
| } |
Contributor
There was a problem hiding this comment.
9. Collection_exists masks outages 🐞 Bug ☼ Reliability
WeaviateVectorStoreProvider::collection_exists returns Ok(false) for any request error, not just 404, so timeouts/5xx/auth failures are misreported as “collection missing.” This can trigger incorrect create attempts and makes operational problems appear as empty state.
Agent Prompt
## Issue description
`collection_exists()` currently maps *all* request failures to `Ok(false)`. This conflates “collection does not exist” (expected 404) with connectivity errors, authentication errors, and server-side failures.
## Issue Context
`request()` uses shared HTTP helpers that return an error for any non-2xx response. As a result, 401/500/timeouts all become `Err(...)` and are treated as “does not exist”.
## Fix Focus Areas
- crates/mcb-providers/src/vector_store/weaviate/admin.rs[20-30]
- crates/mcb-providers/src/vector_store/weaviate/client.rs[79-107]
- crates/mcb-providers/src/utils/http_response.rs[31-58]
## Suggested fix
Implement `collection_exists` in a way that can distinguish 404 from other failures:
- Option A (preferred): perform the GET using `reqwest` directly (reusing auth headers logic) and inspect `status()`:
- `200..299` => `Ok(true)`
- `404` => `Ok(false)`
- otherwise => return `Err(Error::vector_db(...))`
- Option B: extend the shared HTTP utility to surface HTTP status codes in the error type for vector-db requests, then branch on 404 here.
Do not swallow transport/timeouts/5xx; those should propagate so callers can retry/alert appropriately.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+32
to
+36
| impl StdioServerShutdown { | ||
| /// Wait for the stdio server to terminate. | ||
| pub async fn wait(mut self) { | ||
| let _ = self.0.changed().await; | ||
| } |
Contributor
There was a problem hiding this comment.
1. stdioservershutdown swallows errors 📘 Rule violation ☼ Reliability
StdioServerShutdown::wait() discards the watch::Receiver::changed() result and spawn_stdio_server() discards the shutdown_tx.send(true) result, hiding shutdown-channel failures. This can make shutdown logic silently stop working and be hard to diagnose.
Agent Prompt
## Issue description
New shutdown signaling code ignores errors via `let _ = ...`, which silently hides failures.
## Issue Context
Compliance requires errors not be swallowed; they must be propagated, logged, or explicitly handled.
## Fix Focus Areas
- crates/mcb/src/initializers/mcp_server.rs[32-36]
- crates/mcb/src/initializers/mcp_server.rs[203-211]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+7
to
+15
| use std::sync::Mutex; | ||
|
|
||
| use mcb_domain::ports::HighlightError; | ||
| use mcb_domain::value_objects::browse::{HighlightSpan, HighlightedCode}; | ||
| use tree_sitter::Language; | ||
| use tree_sitter_highlight::{Highlight, HighlightConfiguration, HighlightEvent, Highlighter}; | ||
|
|
||
| use mcb_domain::value_objects::browse::{HIGHLIGHT_NAMES, map_highlight_to_category}; | ||
|
|
Contributor
There was a problem hiding this comment.
2. highlight_sync_service imports misordered 📘 Rule violation ⚙ Maintainability
use statements are not grouped/ordered as required: mcb_* imports appear before external crates and then mcb_* imports reappear after externals. This violates the mandated std → external → mcb_* → local grouping.
Agent Prompt
## Issue description
Import groups are out of order / non-contiguous, violating the enforced Rust import grouping convention.
## Issue Context
Imports must be contiguous by group and ordered: std, external crates, mcb_* crates, then local modules.
## Fix Focus Areas
- crates/mcb-infrastructure/src/services/highlight_sync_service.rs[7-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+83
to
+90
| if objects.len() >= WEAVIATE_BATCH_SIZE || i == vectors.len() - 1 { | ||
| self.request( | ||
| reqwest::Method::POST, | ||
| "/v1/batch/objects", | ||
| Some(serde_json::json!({ "objects": objects })), | ||
| ) | ||
| .await?; | ||
| objects.clear(); |
Contributor
There was a problem hiding this comment.
3. Moved batch vector reused 🐞 Bug ≡ Correctness
WeaviateVectorStoreProvider::insert_vectors moves objects into serde_json::json! and then calls objects.clear(), which is a use-after-move compile error that prevents the crate from building.
Agent Prompt
## Issue description
In `insert_vectors`, the code constructs the batch JSON payload with `serde_json::json!({ "objects": objects })`, which moves `objects` into the JSON value. Immediately after the request, it calls `objects.clear()`, which is a compile-time use-after-move error.
## Issue Context
This is inside the batching loop for `/v1/batch/objects` inserts.
## Fix Focus Areas
- crates/mcb-providers/src/vector_store/weaviate/provider.rs[73-91]
### Suggested fix
Use a temporary batch value and leave `objects` usable, e.g.:
- `let batch = std::mem::take(&mut objects);` then `json!({"objects": batch})` (no need to `clear()`), or
- `json!({"objects": objects.clone()})` (less efficient), or
- build the JSON from an iterator without moving the original vec.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+21
to
+55
| /// Public routes — no auth required (static assets + redirect). | ||
| pub fn build_public_routes() -> AxumRouter { | ||
| axum::Router::new() | ||
| .route( | ||
| "/", | ||
| axum::routing::get(|| async { axum::response::Redirect::temporary("/ui/") }), | ||
| ) | ||
| .route( | ||
| "/favicon.ico", | ||
| axum::routing::get(|| async { | ||
| ( | ||
| [(axum::http::header::CONTENT_TYPE, "image/svg+xml")], | ||
| include_str!("../../../../assets/admin/favicon.svg"), | ||
| ) | ||
| }), | ||
| ) | ||
| .route( | ||
| "/ui/theme.css", | ||
| axum::routing::get(|| async { | ||
| ( | ||
| [(axum::http::header::CONTENT_TYPE, "text/css")], | ||
| include_str!("../../../../assets/admin/ui/theme.css"), | ||
| ) | ||
| }), | ||
| ) | ||
| .route( | ||
| "/ui/shared.js", | ||
| axum::routing::get(|| async { | ||
| ( | ||
| [(axum::http::header::CONTENT_TYPE, "application/javascript")], | ||
| include_str!("../../../../assets/admin/ui/shared.js"), | ||
| ) | ||
| }), | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
4. Liveness route not registered 🐞 Bug ☼ Reliability
The new Axum router builder does not register any route for the existing health_api::alive handler, so /alive is not served by the router that the app merges into the top-level route table.
Agent Prompt
## Issue description
`health_api::alive()` exists, but the new Axum router builder never routes to it. As a result, `/alive` requests will not match and will return 404 through the assembled router.
## Issue Context
- `health_api::alive()` is intended as a lightweight liveness probe.
- `build_public_routes()` currently only serves `/` and static UI assets.
- The Loco initializer merges `build_router_without_fallback()` directly into the app router, so whatever is missing here is missing in production.
## Fix Focus Areas
- crates/mcb-server/src/controllers/routes.rs[21-55]
- crates/mcb-server/src/controllers/health_api.rs[38-53]
- crates/mcb/src/initializers/mcp_server.rs[238-247]
### Suggested fix
In `build_public_routes()`, add a public GET route for liveness, e.g.:
- `.route("/alive", axum::routing::get(super::health_api::alive))`
Optionally also add an alias for environments that hit `/api/alive` without rewrite:
- `.route("/api/alive", axum::routing::get(super::health_api::alive))`
ⓘ 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 2b32f7c |
| @@ -0,0 +1 @@ | |||
| ../../flext/.agents/provider.toml No newline at end of file | |||
Contributor
There was a problem hiding this comment.
1. External .agents symlinks 🐞 Bug ☼ Reliability
The PR adds new .agents/* and .agents/skills/* entries that resolve to ../../flext/... (outside this repository), so in any checkout without that sibling flext directory these links will be dangling and the referenced agent/workspace configuration won’t load. Any consumer that dereferences these paths will also traverse outside the workspace, which is commonly blocked in CI/sandbox environments and can cause unexpected failures.
Agent Prompt
## Issue description
New `.agents/*` entries are being introduced as external links (e.g. `../../flext/...`) that depend on a sibling `flext` checkout existing outside this repository. This makes the `.agents` surface non-portable and brittle across environments.
## Issue Context
`AGENTS.md` directs workflows to start from `.agents/skills/...`, so if these linked configs/skills aren’t resolvable, agent/tooling setup becomes inconsistent across machines/CI.
## Fix Focus Areas
- .agents/provider.toml[1-1]
- .agents/INSTRUCTION_SURFACE.md[1-1]
- .agents/skills/flext-context-routing[1-1]
## Suggested fix
- Prefer committing the needed `.agents` files directly into this repo (copy/vendor), OR add `flext` as an in-repo submodule/subtree and point links within the repo (no `..` traversal outside the repo).
- If external links are intentional for local-only workflows, gate them behind a documented bootstrap step (that creates links locally) and keep repo-committed `.agents` entries as valid defaults.
ⓘ 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 d5dcf1c |
marlon-costa-dc
enabled auto-merge (squash)
July 29, 2026 03:03
Comment on lines
+7
to
+9
| use mcb_domain::error::{Error, Result}; | ||
| use mcb_domain::value_objects::{CollectionId, SearchResult}; | ||
| use reqwest::Client; |
Contributor
There was a problem hiding this comment.
1. client.rs imports misordered 📘 Rule violation ⚙ Maintainability
The new Weaviate client places mcb_domain imports between external-crate imports, splitting the external group and violating the required std → external → mcb_* → local order.
Agent Prompt
## Issue description
The Weaviate client imports do not follow the required Rust import-group order.
## Issue Context
All external crates, including `dashmap`, `reqwest`, and `serde_json`, must appear before imports from `mcb_*` crates.
## Fix Focus Areas
- crates/mcb-providers/src/vector_store/weaviate/client.rs[3-23]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+59
to
+63
| let collections = state | ||
| .vector_store | ||
| .list_collections() | ||
| .await | ||
| .unwrap_or_default(); |
Contributor
There was a problem hiding this comment.
2. chunks hides provider failures 📘 Rule violation ☼ Reliability
The all-collections path converts a list_collections() failure into an empty collection list with unwrap_or_default(). Clients consequently receive a successful empty response during vector-store outages instead of an error or observable handling.
Agent Prompt
## Issue description
The `chunks` handler silently turns a vector-store listing error into an empty successful response.
## Issue Context
The handler already returns `Result<Response>`, so the provider error can be mapped and propagated; alternatively, explicitly log and justify degraded behavior.
## Fix Focus Areas
- crates/mcb-server/src/controllers/collections_api.rs[59-71]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+19
to
+20
| let Ok(entries) = std::fs::read_dir(&dir) else { | ||
| continue; |
Contributor
There was a problem hiding this comment.
3. Dependency scan hides i/o failures 📘 Rule violation ☼ Reliability
The new dependency scanner silently skips directories when read_dir fails, treating an incomplete scan as if no forbidden dependency was found. Permission and filesystem failures can therefore produce false-negative compliance results.
Agent Prompt
## Issue description
The Cargo dependency scanner silently continues after directory-read failures.
## Issue Context
A failed directory read means the scanner cannot reliably conclude that forbidden dependencies are absent. Return a typed result and propagate or explicitly report the I/O error.
## Fix Focus Areas
- crates/mcb-validate/src/engines/rusty_cargo_deps.rs[12-43]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Verify sccache is installed; if not, warn and attempt install. | ||
| ifeq ($(shell command -v sccache 2>/dev/null),) | ||
| $(warning sccache not found in PATH. Attempting install...) | ||
| $(shell cargo install sccache --locked 2>/dev/null || true) |
Contributor
There was a problem hiding this comment.
4. sccache install failure suppressed 📘 Rule violation ☼ Reliability
The Makefile suppresses every cargo install sccache failure with redirected stderr and || true, despite configuring sccache as mandatory. Setup proceeds without explaining why the required build wrapper is unavailable.
Agent Prompt
## Issue description
The automatic `sccache` installation suppresses command failures and diagnostic output.
## Issue Context
Because `RUSTC_WRAPPER` is unconditionally set to `sccache`, installation failure should stop setup with an actionable error or be explicitly handled without hiding its cause.
## Fix Focus Areas
- Makefile[44-58]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| .map(|distance| 1.0 - distance) | ||
| }) | ||
| .unwrap_or(0.0) | ||
| } |
Contributor
There was a problem hiding this comment.
5. client.rs exceeds size limit 📘 Rule violation ⚙ Maintainability
The newly introduced Weaviate client is 253 lines, exceeding the approximately 200-line source-file limit. Its request handling, GraphQL construction, response validation, and result conversion should be separated into focused modules.
Agent Prompt
## Issue description
The new Weaviate client source file contains 253 lines and exceeds the project size guideline.
## Issue Context
GraphQL query construction and response conversion are natural candidates for extraction while retaining the provider client in `client.rs`.
## Fix Focus Areas
- crates/mcb-providers/src/vector_store/weaviate/client.rs[1-253]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| None, | ||
| None, | ||
| ); | ||
| let org_id = resolve_org_id(args.org_id.as_deref()); |
Contributor
There was a problem hiding this comment.
7. Caller controls tenant scope 🐞 Bug ⛨ Security
The new isolation path takes org_id directly from caller-controlled MCP arguments and uses it for scoped memory reads and writes. An authenticated client can therefore select another organization and access or modify that tenant's memory instead of using an organization bound to its principal.
Agent Prompt
## Issue description
Memory handlers trust the caller-provided `org_id`, allowing clients to select another tenant's storage scope. Resolve the organization exclusively from an authenticated request principal and reject conflicting tenant arguments.
## Issue Context
`MemoryArgs.org_id` is untrusted tool input, and `resolve_org_id` returns explicit input unchanged. The architecture requires handlers to use the principal's organization rather than request arguments.
## Fix Focus Areas
- crates/mcb-server/src/handlers/memory/observation.rs[52-94]
- crates/mcb-server/src/utils/mcp/helpers.rs[32-39]
- crates/mcb-server/src/args/memory.rs[79-101]
- crates/mcb-server/src/handlers/search.rs[232-256]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+26
to
+29
| let body = serde_json::json!({ | ||
| "class": class, | ||
| "vectorizer": "none", | ||
| "vectorIndexConfig": { "distance": WEAVIATE_DISTANCE_METRIC }, |
Contributor
There was a problem hiding this comment.
8. Weaviate tenants never applied 🐞 Bug ⛨ Security
The new Weaviate provider creates ordinary shared classes and performs inserts, searches, and deletes without any tenant identifier. Organizations using the same collection therefore share vectors, enabling cross-tenant code search and mutation despite the provider advertising native multi-tenancy.
Agent Prompt
## Issue description
The Weaviate implementation does not configure multi-tenant classes or provide an organization tenant on vector operations. Add authenticated tenant context throughout the vector-store API and apply it to every Weaviate request.
## Issue Context
Code search currently ignores `SearchArgs.org_id`, while the provider API accepts only a collection identifier. Tenant identity must originate from the authenticated principal, not caller input.
## Fix Focus Areas
- crates/mcb-domain/src/ports/providers/vector_store.rs[10-98]
- crates/mcb-providers/src/vector_store/weaviate/provider.rs[18-173]
- crates/mcb-providers/src/vector_store/weaviate/browser.rs[14-57]
- crates/mcb-server/src/handlers/search.rs[114-130]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+403
to
+410
| services | ||
| .agent_session | ||
| .create_session(session) | ||
| .await | ||
| .map_err(|e| { | ||
| tracing::warn!("Auto-session creation failed (non-fatal): {e}"); | ||
| McpError::internal_error(format!("Auto-session creation failed: {e}"), None) | ||
| })?; |
Contributor
There was a problem hiding this comment.
9. Auto-init failures block tools 🐞 Bug ☼ Reliability
Session and project repository failures are now converted to McpError and propagated through handle_call_tool, even though auto-initialization is documented as non-fatal. A transient database failure consequently rejects the requested tool operation instead of only skipping automatic initialization.
Agent Prompt
## Issue description
Automatic session and project creation now propagates repository failures into tool dispatch. Preserve validation errors where required, but log and swallow repository failures as the documented non-fatal policy specifies.
## Issue Context
`handle_call_tool` uses `?` on the combined auto-initialization routine. Both creation helpers currently map database failures into `McpError`, causing unrelated tools to become unavailable during persistence failures.
## Fix Focus Areas
- crates/mcb-server/src/mcp_server.rs[290-295]
- crates/mcb-server/src/mcp_server.rs[367-410]
- crates/mcb-server/src/mcp_server.rs[454-503]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+22
to
+27
| workflow_dispatch: | ||
| inputs: | ||
| tag: | ||
| description: Existing tag to (re)publish a GitHub Release for (e.g. v0.3.2) | ||
| required: true | ||
| type: string |
Contributor
There was a problem hiding this comment.
10. Manual releases build wrong ref 🐞 Bug ≡ Correctness
The new dispatch input determines the published release tag, but release-build checks out the workflow dispatch SHA rather than that tag. Re-running a historical release can therefore publish binaries built from the current branch under the historical version.
Agent Prompt
## Issue description
Manual release runs publish under the requested tag while building the dispatch branch revision. Make the requested tag the single source of truth for both checkout and release metadata.
## Issue Context
For `workflow_dispatch`, the checkout action defaults to `github.sha`. The input tag is only consumed later by release creation, so artifacts and their release tag can represent different commits.
## Fix Focus Areas
- .github/workflows/release.yml[20-27]
- .github/workflows/release.yml[37-77]
- .github/workflows/release.yml[132-189]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| .unwrap_or(mcb_utils::constants::ide::IDE_MCB_STDIO); | ||
| let session = AgentSession { | ||
| id: session_id.clone(), | ||
| let session = build_auto_session(ctx, session_id, ide_label)?; |
Contributor
There was a problem hiding this comment.
11. Failed validation poisons guard 🐞 Bug ☼ Reliability
auto_create_session records the session ID in init_sessions before the newly fallible model validation and session construction. If validation fails, later calls with that ID skip initialization and validation, leaving the session absent or invalid for the process lifetime.
Agent Prompt
## Issue description
The session initialization guard is committed before fallible validation and persistence complete. Insert the completed guard only after successful creation, or remove it on every failure while preserving concurrency safety.
## Issue Context
A missing `model_id`, clock failure, or repository failure can occur after the `DashSet` insertion. Subsequent calls then take the early-return path and never retry initialization.
## Fix Focus Areas
- crates/mcb-server/src/mcp_server.rs[387-410]
- crates/mcb-server/src/mcp_server.rs[415-450]
ⓘ 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 0e535ae |
![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 479 files
Confidence score: 1/5
crates/mcb-server/src/handlers/search.rsuses caller-suppliedorg_idas the tenant boundary, which can enable cross-tenant memory reads and data exposure—derive tenant scope only from authenticated server context and ignore/validate client-provided org fields.crates/mcb-server/src/hooks/processor.rs,crates/mcb-domain/src/ports/services/memory.rs, and the session summary handlers (crates/mcb-server/src/handlers/session/summarize.rs,crates/mcb-server/src/handlers/memory/session.rs) are inconsistent on org scoping, so data can be written to the wrong tenant, become unreadable, or be deleted across tenants—threadorg_idthrough create/read/delete paths and standardize default-org resolution.crates/mcb-providers/src/vector_store/weaviate/admin.rscollapses auth/network/malformed-response failures into “collection absent,” allowinghealth_checkto pass while Weaviate is actually unavailable—treat only true 404 as absent and surface other failures as unhealthy..github/setup-ci.shcan fail to installsccachebecauseRUSTC_WRAPPERremains set to a missing executable, making CI bootstrap fragile—unsetRUSTC_WRAPPERfor the install fallback and re-export it after successful installation.
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-server/src/handlers/search.rs">
<violation number="1" location="crates/mcb-server/src/handlers/search.rs:250">
P0: Memory search can be switched to another tenant by supplying an `org_id` argument, because this line treats the caller-controlled hidden field as the tenant boundary. Tenant scope should come from authenticated server context and overwrite or reject any request-provided `org_id` before dispatch.</violation>
</file>
<file name="crates/mcb-providers/src/vector_store/weaviate/admin.rs">
<violation number="1" location="crates/mcb-providers/src/vector_store/weaviate/admin.rs:29">
P1: Authentication failures, outages, and malformed responses are reported as “collection absent,” and the default `health_check` consequently succeeds while Weaviate is unavailable. Only the documented 404 case should become `Ok(false)`; other errors should propagate.</violation>
<violation number="2" location="crates/mcb-providers/src/vector_store/weaviate/admin.rs:56">
P2: GraphQL errors are exposed as an active collection with zero vectors because any HTTP-success payload selects `STATUS_ACTIVE`. Treat a non-empty top-level `errors` array as `STATUS_UNKNOWN` before parsing the aggregate result.</violation>
</file>
<file name="crates/mcb-server/src/hooks/processor.rs">
<violation number="1" location="crates/mcb-server/src/hooks/processor.rs:80">
P1: Non-default tenants' tool observations are stored under the bootstrap organization, so their own org-scoped searches cannot retrieve them and multiple tenants' hook data is co-located. `ToolExecutionContext` already carries `org_id`; propagate it into `PostToolUseContext` and require/use that value here instead of `DEFAULT_ORG_ID`.</violation>
<violation number="2" location="crates/mcb-server/src/hooks/processor.rs:128">
P2: Session starts for any non-default tenant search the bootstrap org, so tenant-specific prior context is never found. Carry the resolved organization in `SessionStartContext` (or a request principal) and pass it to `memory_search` rather than silently falling back.</violation>
</file>
<file name=".github/setup-ci.sh">
<violation number="1" location=".github/setup-ci.sh:151">
P1: CI cannot bootstrap sccache on a runner where it is absent: the workflow already sets `RUSTC_WRAPPER=sccache`, so the `cargo install` fallback attempts to compile through the missing executable. Clear `RUSTC_WRAPPER` for this installation.</violation>
</file>
<file name="crates/mcb-validate/src/engines/rusty_cargo_deps.rs">
<violation number="1" location="crates/mcb-validate/src/engines/rusty_cargo_deps.rs:16">
P1: Crate-scoped dependency rules can report violations caused by unrelated manifests because this recursively scans every `Cargo.toml` under `workspace_root` and ignores the rule's `target`. Scope lookup to the target package/workspace member (and avoid excluded/generated trees) before testing its dependencies.</violation>
<violation number="2" location="crates/mcb-validate/src/engines/rusty_cargo_deps.rs:57">
P2: Platform-specific forbidden dependencies bypass this rule because only the top-level `[dependencies]` table is inspected. Include each `[target.<cfg>.dependencies]` table when evaluating architecture boundaries.</violation>
</file>
<file name="docs/MCP_TOOLS.md">
<violation number="1" location="docs/MCP_TOOLS.md:216">
P1: Clients can still receive unresolved provenance, and failures occur when an applicable tool is called rather than at server boot; `model_id` also silently defaults to `UNKNOWN`. This overview should describe boot-time best-effort defaults plus per-request validation instead of guaranteeing complete discovery and fast-fail initialization.</violation>
<violation number="2" location="docs/MCP_TOOLS.md:220">
P2: The documented default `session_id` format/source does not match runtime behavior: boot discovery always generates a UUID and does not promote `CURSOR_TRACE_ID` or `CLAUDE_SESSION_ID` into this field. Document the UUID default so consumers do not rely on IDE IDs or parse the advertised format.</violation>
<violation number="3" location="docs/MCP_TOOLS.md:221">
P1: A server launched from a plain filesystem workspace does not get the promised CWD `repo_path`; discovery returns `None` and provenance validation rejects affected tools. Remove the filesystem guarantee or implement it before advertising non-VCS workspaces as supported.</violation>
<violation number="4" location="docs/MCP_TOOLS.md:222">
P1: The table marks `repo_id` as conditionally required, but every index/search/memory call requires it even when `repo_path` is present. This contract mismatch can make requests documented as valid fail with `Missing execution provenance`.</violation>
<violation number="5" location="docs/MCP_TOOLS.md:224">
P2: `worktree_id` is not auto-discovered or always resolved; it remains `None` unless metadata supplies it. Document the actual override-only behavior rather than a nonexistent Git fallback cascade.</violation>
<violation number="6" location="docs/MCP_TOOLS.md:228">
P1: Setting any listed model environment variable has no effect, while an unset model silently becomes `UNKNOWN` and passes validation instead of producing the documented fast-fail error. This row should reflect metadata override plus the current placeholder, or the implementation must add the advertised discovery and failure behavior.</violation>
<violation number="7" location="docs/MCP_TOOLS.md:237">
P2: The server does not try the listed backend stubs or fall back to the filesystem; it only asks the configured `VcsProvider` while walking CWD ancestors. Replace this sequence with the implemented behavior so users do not expect unsupported workspace types to initialize successfully.</violation>
<violation number="8" location="docs/MCP_TOOLS.md:242">
P2: Not every provenance field can be overridden through `meta`: `timestamp` is always regenerated from the server clock. Exclude it from this guarantee so callers do not assume their supplied event timestamp is preserved.</violation>
</file>
<file name="crates/mcb-server/src/handlers/session/summarize.rs">
<violation number="1" location="crates/mcb-server/src/handlers/session/summarize.rs:26">
P1: Create and fetch can use different default org IDs in this handler, so a summary created without `org_id` can be immediately unreadable (`Session summary not found`). The fetch path now uses `resolve_org_id`, but `create_summary` still persists with `DEFAULT_ORG_ID` fallback.</violation>
</file>
<file name="crates/mcb-server/src/handlers/memory/session.rs">
<violation number="1" location="crates/mcb-server/src/handlers/memory/session.rs:91">
P1: Calls that omit `org_id` now store/query summaries under a derived UUID instead of the repository's literal `DEFAULT_ORG_ID`, so existing default-tenant summaries become invisible and new rows land in a different tenant. Consider making `resolve_org_id` return `DEFAULT_ORG_ID` or otherwise using the same canonical default across both paths.</violation>
</file>
<file name="crates/mcb-domain/src/ports/services/memory.rs">
<violation number="1" location="crates/mcb-domain/src/ports/services/memory.rs:39">
P1: Observation deletion remains cross-tenant: after observations gain `org_id`, `delete_observation` still accepts only an ID and the SeaORM implementation deletes solely by primary key. Thread `org_id` through both delete ports and filter the delete by organization and ID.</violation>
<violation number="2" location="crates/mcb-domain/src/ports/services/memory.rs:117">
P1: Semantic search can miss valid tenant memories when another org's vectors occupy the shared top-N candidate set. `search_memories_impl` queries `MEMORY_COLLECTION_NAME` with no filter and vector metadata omits `org_id`; scope vector insertion/search by org before applying the limit.</violation>
</file>
<file name=".serena/memories/architecture.md">
<violation number="1" location=".serena/memories/architecture.md:1">
P1: Current architecture guidance added under `.serena/memories/` will be treated as historical rather than authoritative, so contributors and agents can miss or distrust these rules. This content belongs in the canonical `docs/`/configuration source instead of a Serena memory projection.</violation>
<violation number="2" location=".serena/memories/architecture.md:17">
P2: The documented server dependency rule rejects an allowed and currently used `mcb-domain` dependency. Keeping this projection aligned with `config/mcb-validate-internal.toml` avoids sending contributors toward an incorrect boundary.</violation>
<violation number="3" location=".serena/memories/architecture.md:37">
P2: Following this validation instruction fails with “No rule to make target 'validate'” instead of running architecture checks. Use the canonical `check` dispatcher invocation documented by the Makefile.</violation>
</file>
<file name=".github/actions/native-deps/action.yml">
<violation number="1" location=".github/actions/native-deps/action.yml:43">
P1: The downloaded ONNX Runtime DLL is loaded by later build/test steps without any integrity verification. Pinning and checking the Windows archive's SHA-256 before `Expand-Archive` would match the Unix installer and prevent a compromised download from becoming executable code.</violation>
</file>
<file name="crates/mcb/src/loco_app.rs">
<violation number="1" location="crates/mcb/src/loco_app.rs:170">
P1: Config-driven `stdio_only` processes remain alive after the MCP client closes stdin because this branch ignores `StdioServerShutdown` and waits only for Ctrl+C/SIGTERM. Waiting on either the stored shutdown receiver or an OS signal would make this mode terminate like the `--stdio` path.</violation>
</file>
<file name="crates/mcb-utils/src/constants/vector_store.rs">
<violation number="1" location="crates/mcb-utils/src/constants/vector_store.rs:122">
P1: `get_chunks_by_file` returns at most 100 chunks because this “pagination” size is used as a one-shot GraphQL limit without any pagination loop. This should either paginate until all matching objects are fetched or avoid presenting the value as a page size.</violation>
<violation number="2" location="crates/mcb-utils/src/constants/vector_store.rs:125">
P1: Listed collection names are not usable in subsequent provider operations: this prefix is added on lookup, while `list_collections` returns prefixed schema names without removing it. The listing should filter to managed classes and map them back to their original `CollectionId` values (with a reversible naming scheme).</violation>
</file>
<file name="crates/mcb-server/src/handlers/memory/handler.rs">
<violation number="1" location="crates/mcb-server/src/handlers/memory/handler.rs:176">
P1: Error-pattern search is not tenant-scoped at the vector candidate stage despite passing `org_id` here. Other organizations' nearest vectors can crowd out this tenant's matches, so tenant scope should be propagated through vector insertion and search, including the Weaviate provider.</violation>
</file>
<file name=".waza.yaml">
<violation number="1" location=".waza.yaml:6">
P2: These limits bypass the repository's canonical-source workflow, so regenerating `.waza.yaml` can discard the entire configuration. Please define them in the owning config/template and generate this projection through the canonical Make target.</violation>
</file>
<file name=".continue/rules/mcb.md">
<violation number="1" location=".continue/rules/mcb.md:9">
P2: Continue agents now lose project-specific architecture and development rules because this pointer claims `AGENTS.md` contains them, while the supplied `AGENTS.md` does not. Move the omitted rules into `AGENTS.md` (or retain a pointer to their actual authority) before regenerating this file.</violation>
</file>
<file name="crates/mcb/src/cli/serve.rs">
<violation number="1" location="crates/mcb/src/cli/serve.rs:52">
P2: Stdio startup or service failures now terminate `mcb serve --stdio` with a successful exit status because this notification carries no result and the spawned task only logs errors. Consider propagating the stdio task's `Result` through the shutdown handle so `execute` returns the failure while preserving normal EOF as success.</violation>
</file>
<file name="crates/mcb-validate/src/validators/naming/checks/ca.rs">
<violation number="1" location="crates/mcb-validate/src/validators/naming/checks/ca.rs:47">
P2: Bare role files such as `handler.rs` outside `handlers/` and `module.rs` outside `di/` now produce no CA naming violation because `file_name != role` rejects them before directory validation. Consider scoping the exact-name exemption to the `repository.rs` entity case instead of applying it to every `RoleWord` matcher.</violation>
</file>
<file name="crates/mcb-validate/rules/testing/TEST001_organization.yml">
<violation number="1" location="crates/mcb-validate/rules/testing/TEST001_organization.yml:14">
P2: Valid inline test modules are missed when the attribute/module shares its line with the item, a trailing comment, or module contents because both patterns now require the declaration to occupy the whole line. Keeping the new start anchors but dropping the end anchors preserves protection from embedded matches without excluding valid Rust formatting.</violation>
</file>
<file name=".config/nextest.toml">
<violation number="1" location=".config/nextest.toml:22">
P2: Tests running longer than 180 seconds will be terminated, so this is not the warning-only slow-test reporting described above it and can turn legitimate long integration tests into failures. Removing `terminate-after` from both profiles keeps the intended warnings without killing tests.</violation>
</file>
<file name=".superpowers/sdd/progress.md">
<violation number="1" location=".superpowers/sdd/progress.md:9">
P2: The progress tracker now asserts task and commit state without the required verification record, so readers cannot distinguish current Beads state from aspiration. Add the exact canonical command, working directory, decisive output/exit code, and bounded scope supporting these claims.</violation>
</file>
<file name="crates/mcb-server/src/handlers/memory/inject.rs">
<violation number="1" location="crates/mcb-server/src/handlers/memory/inject.rs:42">
P2: Truncated observations are reported as injected because their IDs are appended before the token-budget check. Append the ID only after the corresponding entry is accepted into `context` so count, IDs, and context remain consistent.</violation>
</file>
<file name=".serena/memories/coding_standards.md">
<violation number="1" location=".serena/memories/coding_standards.md:26">
P2: Agents reading this as the Cargo lint policy will miss many checks that fail or warn in the workspace. The generated projection should include the full lists or clearly label them as non-exhaustive.</violation>
</file>
<file name="CLAUDE.md">
<violation number="1" location="CLAUDE.md:14">
P2: Validation guidance is weaker than the repository’s required gate and may let changes pass without full checks. Pointing to `make check` as primary keeps contributor behavior aligned with enforced policy.</violation>
</file>
<file name="crates/mcb-providers/src/vector_store/weaviate/registry.rs">
<violation number="1" location="crates/mcb-providers/src/vector_store/weaviate/registry.rs:22">
P2: An empty or whitespace-only `uri` passes factory validation, then produces invalid request URLs for every Weaviate operation. Treat blank values as missing (and normalize surrounding whitespace) before constructing the provider.</violation>
</file>
<file name=".agents/skills/rules-docker">
<violation number="1" location=".agents/skills/rules-docker:1">
P2: The Docker rules cannot be loaded from a standalone clone because this symlink depends on an undeclared sibling `flext` checkout. Consider tracking the skill in this repository or adding a reproducible dependency/bootstrap mechanism that creates the target.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 200 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
Shadow auto-approve: would not auto-approve because issues were found.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| ..Default::default() | ||
| }; | ||
| let limit = args.limit.unwrap_or(DEFAULT_SEARCH_LIMIT as u32) as usize; | ||
| let org_id = resolve_org_id(args.org_id.as_deref()); |
There was a problem hiding this comment.
P0: Memory search can be switched to another tenant by supplying an org_id argument, because this line treats the caller-controlled hidden field as the tenant boundary. Tenant scope should come from authenticated server context and overwrite or reject any request-provided org_id before dispatch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/src/handlers/search.rs, line 250:
<comment>Memory search can be switched to another tenant by supplying an `org_id` argument, because this line treats the caller-controlled hidden field as the tenant boundary. Tenant scope should come from authenticated server context and overwrite or reject any request-provided `org_id` before dispatch.</comment>
<file context>
@@ -244,11 +247,12 @@ impl SearchHandler {
..Default::default()
};
- let limit = args.limit.unwrap_or(DEFAULT_SEARCH_LIMIT as u32) as usize;
+ let org_id = resolve_org_id(args.org_id.as_deref());
+ let limit = resolve_limit(args.limit, DEFAULT_SEARCH_LIMIT as u32);
</file context>
| Ok(_) => Ok(true), | ||
| // A missing class is a legitimate "does not exist" answer, not a | ||
| // transport failure: Weaviate returns 404 for unknown classes. | ||
| Err(_) => Ok(false), |
There was a problem hiding this comment.
P1: Authentication failures, outages, and malformed responses are reported as “collection absent,” and the default health_check consequently succeeds while Weaviate is unavailable. Only the documented 404 case should become Ok(false); other errors should propagate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-providers/src/vector_store/weaviate/admin.rs, line 29:
<comment>Authentication failures, outages, and malformed responses are reported as “collection absent,” and the default `health_check` consequently succeeds while Weaviate is unavailable. Only the documented 404 case should become `Ok(false)`; other errors should propagate.</comment>
<file context>
@@ -0,0 +1,84 @@
+ Ok(_) => Ok(true),
+ // A missing class is a legitimate "does not exist" answer, not a
+ // transport failure: Weaviate returns 404 for unknown classes.
+ Err(_) => Ok(false),
+ }
+ }
</file context>
| .store_observation(StoreObservationInput { | ||
| project_id, | ||
| // ADR-056 (bead mcb-6pjx.1.2): org_id deferred; hooks do not yet carry org context. | ||
| org_id: mcb_utils::constants::values::DEFAULT_ORG_ID.to_owned(), |
There was a problem hiding this comment.
P1: Non-default tenants' tool observations are stored under the bootstrap organization, so their own org-scoped searches cannot retrieve them and multiple tenants' hook data is co-located. ToolExecutionContext already carries org_id; propagate it into PostToolUseContext and require/use that value here instead of DEFAULT_ORG_ID.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/src/hooks/processor.rs, line 80:
<comment>Non-default tenants' tool observations are stored under the bootstrap organization, so their own org-scoped searches cannot retrieve them and multiple tenants' hook data is co-located. `ToolExecutionContext` already carries `org_id`; propagate it into `PostToolUseContext` and require/use that value here instead of `DEFAULT_ORG_ID`.</comment>
<file context>
@@ -76,6 +76,8 @@ impl HookProcessor {
.store_observation(StoreObservationInput {
project_id,
+ // ADR-056 (bead mcb-6pjx.1.2): org_id deferred; hooks do not yet carry org context.
+ org_id: mcb_utils::constants::values::DEFAULT_ORG_ID.to_owned(),
content,
r#type: ObservationType::Execution,
</file context>
| # Ensure sccache is available (mandatory compilation cache) | ||
| if ! command -v sccache &>/dev/null; then | ||
| echo "Installing sccache (mandatory compilation cache)..." >&2 | ||
| _mcb_install_crate sccache |
There was a problem hiding this comment.
P1: CI cannot bootstrap sccache on a runner where it is absent: the workflow already sets RUSTC_WRAPPER=sccache, so the cargo install fallback attempts to compile through the missing executable. Clear RUSTC_WRAPPER for this installation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/setup-ci.sh, line 151:
<comment>CI cannot bootstrap sccache on a runner where it is absent: the workflow already sets `RUSTC_WRAPPER=sccache`, so the `cargo install` fallback attempts to compile through the missing executable. Clear `RUSTC_WRAPPER` for this installation.</comment>
<file context>
@@ -133,18 +133,35 @@ Darwin)
+# Ensure sccache is available (mandatory compilation cache)
+if ! command -v sccache &>/dev/null; then
+ echo "Installing sccache (mandatory compilation cache)..." >&2
+ _mcb_install_crate sccache
+fi
+
</file context>
Suggested change
| _mcb_install_crate sccache | |
| RUSTC_WRAPPER= _mcb_install_crate sccache |
| workspace_root: &std::path::Path, | ||
| pattern_prefix: &str, | ||
| ) -> bool { | ||
| let mut stack = vec![workspace_root.to_path_buf()]; |
There was a problem hiding this comment.
P1: Crate-scoped dependency rules can report violations caused by unrelated manifests because this recursively scans every Cargo.toml under workspace_root and ignores the rule's target. Scope lookup to the target package/workspace member (and avoid excluded/generated trees) before testing its dependencies.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-validate/src/engines/rusty_cargo_deps.rs, line 16:
<comment>Crate-scoped dependency rules can report violations caused by unrelated manifests because this recursively scans every `Cargo.toml` under `workspace_root` and ignores the rule's `target`. Scope lookup to the target package/workspace member (and avoid excluded/generated trees) before testing its dependencies.</comment>
<file context>
@@ -0,0 +1,98 @@
+ workspace_root: &std::path::Path,
+ pattern_prefix: &str,
+) -> bool {
+ let mut stack = vec![workspace_root.to_path_buf()];
+
+ while let Some(dir) = stack.pop() {
</file context>
| | `repo_path` | string | **yes** | Plugin-based workspace discovery: Git → Mercury → CVS → SVN → … → **Filesystem (CWD canonical)** | — (CWD is the ultimate happy path) | | ||
| | `repo_id` | string | if `repo_path` absent | Git remote `origin` URL hash; absent for plain filesystem workspaces | — | | ||
| | `project_id` | string | no | Git remote `origin` (`owner/repo`); absent for plain filesystem workspaces | — | | ||
| | `worktree_id` | string | no | `git rev-parse --git-dir` → `git worktree list` → `.git` dir → `"main"` → **CWD path** | — (always resolved) | |
There was a problem hiding this comment.
P2: worktree_id is not auto-discovered or always resolved; it remains None unless metadata supplies it. Document the actual override-only behavior rather than a nonexistent Git fallback cascade.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/MCP_TOOLS.md, line 224:
<comment>`worktree_id` is not auto-discovered or always resolved; it remains `None` unless metadata supplies it. Document the actual override-only behavior rather than a nonexistent Git fallback cascade.</comment>
<file context>
@@ -213,21 +213,33 @@ Unified entity CRUD (vcs/plan/issue/org resources).
+| `repo_path` | string | **yes** | Plugin-based workspace discovery: Git → Mercury → CVS → SVN → … → **Filesystem (CWD canonical)** | — (CWD is the ultimate happy path) |
+| `repo_id` | string | if `repo_path` absent | Git remote `origin` URL hash; absent for plain filesystem workspaces | — |
+| `project_id` | string | no | Git remote `origin` (`owner/repo`); absent for plain filesystem workspaces | — |
+| `worktree_id` | string | no | `git rev-parse --git-dir` → `git worktree list` → `.git` dir → `"main"` → **CWD path** | — (always resolved) |
+| `operator_id` | string | **yes** | `$USER` env var | — |
+| `machine_id` | string | **yes** | Hostname (`hostname::get()` or `$HOSTNAME`) | — |
</file context>
Suggested change
| | `worktree_id` | string | no | `git rev-parse --git-dir` → `git worktree list` → `.git` dir → `"main"` → **CWD path** | — (always resolved) | | |
| | `worktree_id` | string | no | Explicit request metadata override; no boot-time default | — | |
|
|
||
| | Field | Type | Required | Auto-filled source | Fast-fail message | | ||
| |-------|------|----------|-------------------|-------------------| | ||
| | `session_id` | string | **yes** | IDE session ID (`CURSOR_TRACE_ID`, `CLAUDE_SESSION_ID`, …) or traceable ID `<agent>-<host>-<pid>-<timestamp>` | — (always traceable) | |
There was a problem hiding this comment.
P2: The documented default session_id format/source does not match runtime behavior: boot discovery always generates a UUID and does not promote CURSOR_TRACE_ID or CLAUDE_SESSION_ID into this field. Document the UUID default so consumers do not rely on IDE IDs or parse the advertised format.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/MCP_TOOLS.md, line 220:
<comment>The documented default `session_id` format/source does not match runtime behavior: boot discovery always generates a UUID and does not promote `CURSOR_TRACE_ID` or `CLAUDE_SESSION_ID` into this field. Document the UUID default so consumers do not rely on IDE IDs or parse the advertised format.</comment>
<file context>
@@ -213,21 +213,33 @@ Unified entity CRUD (vcs/plan/issue/org resources).
+
+| Field | Type | Required | Auto-filled source | Fast-fail message |
+|-------|------|----------|-------------------|-------------------|
+| `session_id` | string | **yes** | IDE session ID (`CURSOR_TRACE_ID`, `CLAUDE_SESSION_ID`, …) or traceable ID `<agent>-<host>-<pid>-<timestamp>` | — (always traceable) |
+| `repo_path` | string | **yes** | Plugin-based workspace discovery: Git → Mercury → CVS → SVN → … → **Filesystem (CWD canonical)** | — (CWD is the ultimate happy path) |
+| `repo_id` | string | if `repo_path` absent | Git remote `origin` URL hash; absent for plain filesystem workspaces | — |
</file context>
Suggested change
| | `session_id` | string | **yes** | IDE session ID (`CURSOR_TRACE_ID`, `CLAUDE_SESSION_ID`, …) or traceable ID `<agent>-<host>-<pid>-<timestamp>` | — (always traceable) | | |
| | `session_id` | string | **yes** | UUID generated once at server boot, unless explicitly overridden | — | |
| keep only project-specific notes below. | ||
|
|
||
| - **Task tracking:** `bd` (beads). Run `bd prime`. | ||
| - **Validation:** prefer `make` targets (`make lint` / `make typecheck` / `make test`). |
There was a problem hiding this comment.
P2: Validation guidance is weaker than the repository’s required gate and may let changes pass without full checks. Pointing to make check as primary keeps contributor behavior aligned with enforced policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At CLAUDE.md, line 14:
<comment>Validation guidance is weaker than the repository’s required gate and may let changes pass without full checks. Pointing to `make check` as primary keeps contributor behavior aligned with enforced policy.</comment>
<file context>
@@ -1,3 +1,17 @@
+keep only project-specific notes below.
+
+- **Task tracking:** `bd` (beads). Run `bd prime`.
+- **Validation:** prefer `make` targets (`make lint` / `make typecheck` / `make test`).
+- **Tools:** `ast-grep` (`sg`) for structural search; never `rm` / `sed -i` (use the Edit tool or `trash-put`).
+
</file context>
Suggested change
| - **Validation:** prefer `make` targets (`make lint` / `make typecheck` / `make test`). | |
| - **Validation:** run `make check` (or the corresponding `make` subtargets only when intentionally scoping work). |
Comment on lines
+22
to
+25
| .uri | ||
| .clone() | ||
| .ok_or_else(|| Error::configuration("Weaviate requires uri (http://host:8080)"))?; | ||
| let http_client = create_default_client()?; |
There was a problem hiding this comment.
P2: An empty or whitespace-only uri passes factory validation, then produces invalid request URLs for every Weaviate operation. Treat blank values as missing (and normalize surrounding whitespace) before constructing the provider.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-providers/src/vector_store/weaviate/registry.rs, line 22:
<comment>An empty or whitespace-only `uri` passes factory validation, then produces invalid request URLs for every Weaviate operation. Treat blank values as missing (and normalize surrounding whitespace) before constructing the provider.</comment>
<file context>
@@ -0,0 +1,39 @@
+ use crate::utils::http::{DEFAULT_HTTP_TIMEOUT, create_default_client};
+
+ let uri = config
+ .uri
+ .clone()
+ .ok_or_else(|| Error::configuration("Weaviate requires uri (http://host:8080)"))?;
</file context>
Suggested change
| .uri | |
| .clone() | |
| .ok_or_else(|| Error::configuration("Weaviate requires uri (http://host:8080)"))?; | |
| let http_client = create_default_client()?; | |
| .uri | |
| .as_deref() | |
| .map(str::trim) | |
| .filter(|uri| !uri.is_empty()) | |
| .map(str::to_owned) | |
| .ok_or_else(|| Error::configuration("Weaviate requires uri (http://host:8080)"))?; |
| @@ -0,0 +1 @@ | |||
| ../../../flext/.agents/skills/rules-docker No newline at end of file | |||
There was a problem hiding this comment.
P2: The Docker rules cannot be loaded from a standalone clone because this symlink depends on an undeclared sibling flext checkout. Consider tracking the skill in this repository or adding a reproducible dependency/bootstrap mechanism that creates the target.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .agents/skills/rules-docker, line 1:
<comment>The Docker rules cannot be loaded from a standalone clone because this symlink depends on an undeclared sibling `flext` checkout. Consider tracking the skill in this repository or adding a reproducible dependency/bootstrap mechanism that creates the target.</comment>
<file context>
@@ -0,0 +1 @@
+../../../flext/.agents/skills/rules-docker
\ No newline at end of file
</file context>
added 7 commits
July 30, 2026 00:15
- Box CallToolResult in handler Result types to reduce Err variant size - Box loco_rs::Error in config and MCP server initializers - Unbox errors at call sites where protocol expects concrete type - Add kubernetes-validate dependency for Python test suite
…Result - pyproject: flext-core/flext-cli now come from the GitHub 0.12.0-dev branch (operator directive: the venv always carries the pushed branch, never stale local snapshots); uv.lock regenerated. - annotate analyze_issues/analyze/summarize/generate and the register_result_command handler contract as p.Result[T]: the canonical flext factory protocol now that FlextResultConstruction.ok/fail declare p.Result[V]. Concrete FlextResult return annotations rejected the protocol values (mypy). - remove the lib.agent_pointers/lib.gitops mypy ignore_errors override: the root cause is fixed, suppression no longer needed. Validated: make check WHAT=python (ruff + mypy + 64 pytest + guard) green with flext-core git@acae76c72.
Same serde field-ordering refresh as the earlier snapshot updates, now covering every remaining domain (index, memory, project, search, session, validate, vcs, entity invalid_args). Verified: full mcb-server contract suite 28/28 green.
Contributor
|
Code review by qodo was updated up to the latest commit 04cb8ba |
There was a problem hiding this comment.
8 issues found across 58 files (changes from recent commits).
Confidence score: 2/5
- In
scripts/qlty/parser.py, SARIF root parsing is validating the document as a single run instead of reading therunsarray, so valid qlty reports can be treated as empty and downstream findings are effectively dropped — parse from the SARIF rootrunscollection and add a regression test with multi-run input. - In
scripts/lib/settings.py,BaseMcbSettingsno longer honorsMCB_ENV_FILEbecause copiedmodel_configkeeps flext-core’s resolved.env, which can load the wrong environment and cause configuration drift at runtime — ensureresolve_env_file()is actually applied when building settings config. - In
pyproject.toml, using0.12.0-devbranch refs for both FLEXT dependencies leaves resolution mutable versusuv.lock, so builds can change underneath the same PR and introduce non-reproducible behavior — pin immutable commit SHAs/tags and refresh lockfiles in CI. - In
pyproject.toml, stricter annotation checks are configured but not in the canonical gates, so type-annotation regressions may merge unnoticed — add thepyrefly check(and configured Pyright gate) to required CI checks.
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="pyproject.toml">
<violation number="1" location="pyproject.toml:11">
P2: Dependency resolution is not actually pinned here because `0.12.0-dev` is a mutable branch in both FLEXT repositories. This is already observable for `flext-core`: the branch now points at `e089981…`, while `uv.lock` resolved the same ref to `acae76c…`. Direct installs and future lock refreshes can therefore pick up unreviewed/incompatible changes. Using the tested commit SHAs (or immutable release tags) in `pyproject.toml` would make the dependency reproducible.</violation>
<violation number="2" location="pyproject.toml:135">
P2: This stricter `invalid-annotation` setting is never enforced by the canonical checks, so annotation failures can still merge unnoticed. Consider adding `uv run --no-sync pyrefly check` (and the configured Pyright gate) to `MCB_PYTHON_CHECK` rather than leaving this configuration effective only for manual runs.</violation>
</file>
<file name="scripts/lib/logger.py">
<violation number="1" location="scripts/lib/logger.py:20">
P3: An explicitly empty logger name is silently relabeled as `__main__`, bypassing `FlextUtilitiesLogging`'s non-empty-name validation and misattributing those logs. Distinguishing `None` from `""` preserves the default without masking invalid input.</violation>
<violation number="2" location="scripts/lib/logger.py:21">
P3: Each fetch constructs two `McbLogger` wrappers even though `super().fetch_logger()` already instantiates the dynamic `cls`. Returning that instance with the narrower cast avoids duplicate work and reliance on the wrapped `.logger` detail.</violation>
</file>
<file name="crates/mcb-server/src/handlers/vcs/responses.rs">
<violation number="1" location="crates/mcb-server/src/handlers/vcs/responses.rs:141">
P3: The public `repo_path` documentation now describes the wrong error type: it still promises `Err(CallToolResult)`, while the signature returns `Err(Box<CallToolResult>)`. Updating the rustdoc alongside this boxing change would keep generated API documentation accurate.</violation>
</file>
<file name="scripts/lib/settings.py">
<violation number="1" location="scripts/lib/settings.py:28">
P2: `MCB_ENV_FILE` no longer controls the env file loaded by `BaseMcbSettings`: copying `FlextSettings.model_config` retains flext-core's already-resolved `.env` value, while this class's `resolve_env_file()` is never wired into Pydantic settings. As a result, commands silently use defaults/`.env` even when an operator supplies a custom MCB env file. Consider assigning `model_config["env_file"]` from the MCB resolver (ideally via a module-level resolver usable while constructing the class config) and covering the custom-file path with a settings test.</violation>
</file>
<file name="scripts/qlty/parser.py">
<violation number="1" location="scripts/qlty/parser.py:59">
P0: Valid qlty SARIF reports now parse as empty because the document root is being validated as a single `SarifRun`. The actual results are inside the root `runs` array, while `SarifRun.results` defaults to an empty list and the root `runs` field is ignored. Iterating every root run before converting its results would preserve all findings.</violation>
</file>
<file name="crates/mcb-server/src/controllers/admin_config.rs">
<violation number="1" location="crates/mcb-server/src/controllers/admin_config.rs:1">
P3: This module still uses only `Result` from the Loco prelude, and Loco's `Result<T, E = Error>` alias accepts the newly added boxed error parameter. Keeping the import explicit would avoid bringing the prelude's many controller, extractor, routing, and model names into this small config module.</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
Comment on lines
+59
to
+67
| run = SarifRun.model_validate(data) | ||
|
|
||
| issues: list[SarifIssue] = [] | ||
| for result in run.results: | ||
| issue = _result_to_issue(result) | ||
| if issue is not None: | ||
| issues.append(issue) | ||
|
|
||
| return r[list[SarifIssue]].ok(issues) |
There was a problem hiding this comment.
P0: Valid qlty SARIF reports now parse as empty because the document root is being validated as a single SarifRun. The actual results are inside the root runs array, while SarifRun.results defaults to an empty list and the root runs field is ignored. Iterating every root run before converting its results would preserve all findings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/qlty/parser.py, line 59:
<comment>Valid qlty SARIF reports now parse as empty because the document root is being validated as a single `SarifRun`. The actual results are inside the root `runs` array, while `SarifRun.results` defaults to an empty list and the root `runs` field is ignored. Iterating every root run before converting its results would preserve all findings.</comment>
<file context>
@@ -28,48 +56,12 @@ def parse_sarif_file(path: Path) -> r[list[SarifIssue]]:
except OSError as exc:
return r[list[SarifIssue]].fail(f"cannot read {path}: {exc}")
+ run = SarifRun.model_validate(data)
+
issues: list[SarifIssue] = []
</file context>
Suggested change
| run = SarifRun.model_validate(data) | |
| issues: list[SarifIssue] = [] | |
| for result in run.results: | |
| issue = _result_to_issue(result) | |
| if issue is not None: | |
| issues.append(issue) | |
| return r[list[SarifIssue]].ok(issues) | |
| runs = [SarifRun.model_validate(value) for value in data.get("runs", [])] | |
| issues: list[SarifIssue] = [] | |
| for run in runs: | |
| for result in run.results: | |
| issue = _result_to_issue(result) | |
| if issue is not None: | |
| issues.append(issue) | |
| return r[list[SarifIssue]].ok(issues) |
| use-ignore-files = false | ||
|
|
||
| [tool.pyrefly.errors] | ||
| invalid-annotation = "error" |
There was a problem hiding this comment.
P2: This stricter invalid-annotation setting is never enforced by the canonical checks, so annotation failures can still merge unnoticed. Consider adding uv run --no-sync pyrefly check (and the configured Pyright gate) to MCB_PYTHON_CHECK rather than leaving this configuration effective only for manual runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pyproject.toml, line 135:
<comment>This stricter `invalid-annotation` setting is never enforced by the canonical checks, so annotation failures can still merge unnoticed. Consider adding `uv run --no-sync pyrefly check` (and the configured Pyright gate) to `MCB_PYTHON_CHECK` rather than leaving this configuration effective only for manual runs.</comment>
<file context>
@@ -134,11 +128,11 @@ project-excludes = [
[tool.pyrefly.errors]
-annotation-mismatch = "error"
+invalid-annotation = "error"
bad-argument-type = "error"
bad-assignment = "error"
</file context>
| """ | ||
|
|
||
| model_config: ClassVar[SettingsConfigDict] = ( | ||
| FlextSettings.model_config.copy() |
There was a problem hiding this comment.
P2: MCB_ENV_FILE no longer controls the env file loaded by BaseMcbSettings: copying FlextSettings.model_config retains flext-core's already-resolved .env value, while this class's resolve_env_file() is never wired into Pydantic settings. As a result, commands silently use defaults/.env even when an operator supplies a custom MCB env file. Consider assigning model_config["env_file"] from the MCB resolver (ideally via a module-level resolver usable while constructing the class config) and covering the custom-file path with a settings test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/settings.py, line 28:
<comment>`MCB_ENV_FILE` no longer controls the env file loaded by `BaseMcbSettings`: copying `FlextSettings.model_config` retains flext-core's already-resolved `.env` value, while this class's `resolve_env_file()` is never wired into Pydantic settings. As a result, commands silently use defaults/`.env` even when an operator supplies a custom MCB env file. Consider assigning `model_config["env_file"]` from the MCB resolver (ideally via a module-level resolver usable while constructing the class config) and covering the custom-file path with a settings test.</comment>
<file context>
@@ -10,22 +10,22 @@
model_config: ClassVar[SettingsConfigDict] = (
- FlextSettingsBase.model_config.copy()
+ FlextSettings.model_config.copy()
)
model_config["env_prefix"] = c.ENV_PREFIX
</file context>
Comment on lines
+11
to
+12
| "flext-core @ git+https://github.com/flext-sh/flext-core.git@0.12.0-dev", | ||
| "flext-cli @ git+https://github.com/flext-sh/flext-cli.git@0.12.0-dev", |
There was a problem hiding this comment.
P2: Dependency resolution is not actually pinned here because 0.12.0-dev is a mutable branch in both FLEXT repositories. This is already observable for flext-core: the branch now points at e089981…, while uv.lock resolved the same ref to acae76c…. Direct installs and future lock refreshes can therefore pick up unreviewed/incompatible changes. Using the tested commit SHAs (or immutable release tags) in pyproject.toml would make the dependency reproducible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pyproject.toml, line 11:
<comment>Dependency resolution is not actually pinned here because `0.12.0-dev` is a mutable branch in both FLEXT repositories. This is already observable for `flext-core`: the branch now points at `e089981…`, while `uv.lock` resolved the same ref to `acae76c…`. Direct installs and future lock refreshes can therefore pick up unreviewed/incompatible changes. Using the tested commit SHAs (or immutable release tags) in `pyproject.toml` would make the dependency reproducible.</comment>
<file context>
@@ -8,8 +8,8 @@ version = "0.4.0"
dependencies = [
- "flext-core @ file:///home/marlonsc/flext/flext-core",
- "flext-cli @ file:///home/marlonsc/flext/flext-cli",
+ "flext-core @ git+https://github.com/flext-sh/flext-core.git@0.12.0-dev",
+ "flext-cli @ git+https://github.com/flext-sh/flext-cli.git@0.12.0-dev",
"pydantic>=2.13.4",
</file context>
Suggested change
| "flext-core @ git+https://github.com/flext-sh/flext-core.git@0.12.0-dev", | |
| "flext-cli @ git+https://github.com/flext-sh/flext-cli.git@0.12.0-dev", | |
| "flext-core @ git+https://github.com/flext-sh/flext-core.git@acae76c72cc7396efc681d54cde9222f4292c5bb", | |
| "flext-cli @ git+https://github.com/flext-sh/flext-cli.git@9c834db4c25755f6b17281f08fc734af849af7fa", |
Comment on lines
+21
to
+22
| base = cast(FlextUtilitiesLogging, super().fetch_logger(resolved_name)) | ||
| return cls(resolved_name, _bound_logger=base.logger) |
There was a problem hiding this comment.
P3: Each fetch constructs two McbLogger wrappers even though super().fetch_logger() already instantiates the dynamic cls. Returning that instance with the narrower cast avoids duplicate work and reliance on the wrapped .logger detail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/logger.py, line 21:
<comment>Each fetch constructs two `McbLogger` wrappers even though `super().fetch_logger()` already instantiates the dynamic `cls`. Returning that instance with the narrower cast avoids duplicate work and reliance on the wrapped `.logger` detail.</comment>
<file context>
@@ -8,17 +8,18 @@
- base = cast(FlextLogger, super().fetch_logger(name))
- return cls(name, _bound_logger=base.logger)
+ resolved_name = name or "__main__"
+ base = cast(FlextUtilitiesLogging, super().fetch_logger(resolved_name))
+ return cls(resolved_name, _bound_logger=base.logger)
</file context>
Suggested change
| base = cast(FlextUtilitiesLogging, super().fetch_logger(resolved_name)) | |
| return cls(resolved_name, _bound_logger=base.logger) | |
| return cast(McbLogger, super().fetch_logger(resolved_name)) |
| @classmethod | ||
| def fetch_logger(cls, name: str | None = None) -> McbLogger: | ||
| """Fetch the canonical logger for a module as an ``McbLogger``.""" | ||
| resolved_name = name or "__main__" |
There was a problem hiding this comment.
P3: An explicitly empty logger name is silently relabeled as __main__, bypassing FlextUtilitiesLogging's non-empty-name validation and misattributing those logs. Distinguishing None from "" preserves the default without masking invalid input.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/logger.py, line 20:
<comment>An explicitly empty logger name is silently relabeled as `__main__`, bypassing `FlextUtilitiesLogging`'s non-empty-name validation and misattributing those logs. Distinguishing `None` from `""` preserves the default without masking invalid input.</comment>
<file context>
@@ -8,17 +8,18 @@
"""Fetch the canonical logger for a module as an ``McbLogger``."""
- base = cast(FlextLogger, super().fetch_logger(name))
- return cls(name, _bound_logger=base.logger)
+ resolved_name = name or "__main__"
+ base = cast(FlextUtilitiesLogging, super().fetch_logger(resolved_name))
+ return cls(resolved_name, _bound_logger=base.logger)
</file context>
Suggested change
| resolved_name = name or "__main__" | |
| resolved_name = "__main__" if name is None else name |
| /// * `Ok(PathBuf)` - The resolved filesystem path to the repository. | ||
| /// * `Err(CallToolResult)` - Error if the repository cannot be found or arguments are missing. | ||
| pub fn repo_path(args: &VcsArgs) -> Result<PathBuf, CallToolResult> { | ||
| pub fn repo_path(args: &VcsArgs) -> Result<PathBuf, Box<CallToolResult>> { |
There was a problem hiding this comment.
P3: The public repo_path documentation now describes the wrong error type: it still promises Err(CallToolResult), while the signature returns Err(Box<CallToolResult>). Updating the rustdoc alongside this boxing change would keep generated API documentation accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/src/handlers/vcs/responses.rs, line 141:
<comment>The public `repo_path` documentation now describes the wrong error type: it still promises `Err(CallToolResult)`, while the signature returns `Err(Box<CallToolResult>)`. Updating the rustdoc alongside this boxing change would keep generated API documentation accurate.</comment>
<file context>
@@ -138,14 +138,14 @@ pub struct ImpactResponse {
/// * `Ok(PathBuf)` - The resolved filesystem path to the repository.
/// * `Err(CallToolResult)` - Error if the repository cannot be found or arguments are missing.
-pub fn repo_path(args: &VcsArgs) -> Result<PathBuf, CallToolResult> {
+pub fn repo_path(args: &VcsArgs) -> Result<PathBuf, Box<CallToolResult>> {
if let Some(path) = args.repo_path.as_ref() {
return Ok(PathBuf::from(path));
</file context>
| @@ -1,4 +1,4 @@ | |||
| use loco_rs::prelude::Result; | |||
| use loco_rs::prelude::*; | |||
There was a problem hiding this comment.
P3: This module still uses only Result from the Loco prelude, and Loco's Result<T, E = Error> alias accepts the newly added boxed error parameter. Keeping the import explicit would avoid bringing the prelude's many controller, extractor, routing, and model names into this small config module.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/src/controllers/admin_config.rs, line 1:
<comment>This module still uses only `Result` from the Loco prelude, and Loco's `Result<T, E = Error>` alias accepts the newly added boxed error parameter. Keeping the import explicit would avoid bringing the prelude's many controller, extractor, routing, and model names into this small config module.</comment>
<file context>
@@ -1,4 +1,4 @@
-use loco_rs::prelude::Result;
+use loco_rs::prelude::*;
/// Default config directory when `MCB_PRO_ADMIN_CONFIG_DIR` is not set.
</file context>
Suggested change
| use loco_rs::prelude::*; | |
| use loco_rs::prelude::Result; |
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.