audit: bead/mcb-o96i-19-sccache-bootstrap-integration -> main - #171
Draft
marlon-costa-dc wants to merge 141 commits into
Draft
audit: bead/mcb-o96i-19-sccache-bootstrap-integration -> main#171marlon-costa-dc wants to merge 141 commits into
marlon-costa-dc wants to merge 141 commits into
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
- Add scripts/lib/tests/conftest.py with reset_settings, settings_factory, temp_dir, temp_file, and capture_logs_fixture. - Add scripts/lib/tests/_utilities/matchers.py with tm.ok/tm.fail/tm.that DSL. - Convert test_agent_pointers, test_cli, test_core, test_gitops, test_make_surface, and test_mcb_sh from unittest to pytest functions using shared fixtures and matchers. Four Python gates pass.
Raise test database max_connections from 1 to 5 and timeouts from 500ms to 5000ms. The previous settings caused flaky integration tests under cargo nextest with PoolTimedOut and auto-session DB errors when async DB operations contended for the single connection. Verified: - cargo nextest run -p mcb --test integration --profile ci: 47 passed - make check WHAT=lint: clean - make check WHAT=validate QUICK=1: 0 violations
…d external test scope
…red arg helpers, typed execution-flow extension
…axum Parts no core MCP
…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 skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
|
Running ultrareview automatically — This 480-file, 30k-line sccache bootstrap integration touches build infrastructure and CI configuration at scale, where a subtle caching or wiring mistake could break builds across the team, so a slower multi-pass review is warranted.. I'll post findings when complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 17m 45s
40 issues found across 480 files
Confidence score: 1/5
- The highest risk is cross-tenant data exposure in
crates/mcb-domain/src/ports/services/memory.rs,crates/mcb-server/src/handlers/search.rs, andcrates/mcb-infrastructure/src/services/memory_service/observation.rs: caller-suppliedorg_idis used as authority, so a client can read or act on another organization’s memory. Bind org scope to the authenticated principal server-side and reject/ignore unverified tenant IDs from request inputs. crates/mcb-domain/src/ports/services/memory.rsalso leavesdelete_observationunscoped by tenant, which means a known foreignObservationIdcan be deleted across org boundaries. Add org-aware repository filters (ID + org) and enforce authorization checks before destructive operations.- Tenant isolation looks incomplete for the new provider path in
crates/mcb-providers/src/vector_store/mod.rs, where Weaviate usage keeps all orgs in one class without tenant-aware read/write selection. This can mix code-context data between customers—introduce tenant-specific class/namespace or explicit tenant parameters on every query/write path. - There are additional correctness regressions in
crates/mcb-server/src/handlers/memory/session.rs,crates/mcb-server/src/hooks/processor.rs,crates/mcb-validate/src/validators/declarative_validator/metrics_rules.rs, andcrates/mcb-validate/src/engines/rusty_cargo_deps.rs(default-org sessions becoming unreachable, observations written to bootstrap org, metric-type mapping bug, and crate-target rules applied too broadly), plus false-health reporting incrates/mcb-providers/src/vector_store/weaviate/admin.rs. Align org ID sources, fix the cyclomatic mapping/target filtering logic, and distinguish transport/auth failures from true “missing collection” 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="crates/mcb-domain/src/ports/services/memory.rs">
<violation number="1" location="crates/mcb-domain/src/ports/services/memory.rs:39">
P1: Observation deletion bypasses the tenant boundary: a caller with another organization's `ObservationId` can delete it because `delete_observation` still takes only the ID and the repository executes an unfiltered `delete_by_id`. Thread `org_id` through this service and repository operation and delete with an `OrgId` predicate, matching the newly scoped reads.</violation>
<violation number="2" location="crates/mcb-domain/src/ports/services/memory.rs:65">
P0: Memory reads remain cross-tenant accessible to a caller that supplies another organization's ID. The new `org_id` parameter only scopes the query to the caller-selected string: `MemoryArgs` accepts it and `resolve_org_id` returns it unchanged before the handler calls this interface. Deriving the scope from an authenticated request principal, rather than from `args.org_id`, would make the new tenant filters enforce an actual boundary.</violation>
<violation number="3" location="crates/mcb-domain/src/ports/services/memory.rs:117">
P1: Semantic memory search is not actually tenant-scoped at the vector layer. All tenants write to the same `memories` collection, vectors carry no `org_id`, and the search call passes `None` as its filter, so other tenants can exhaust the fixed candidate window and hide a tenant's own semantic matches. Store and query with tenant-specific vector namespaces/collections (or a filter supported by every enabled provider) before applying the relational post-filter.</violation>
</file>
<file name="crates/mcb-server/src/handlers/search.rs">
<violation number="1" location="crates/mcb-server/src/handlers/search.rs:250">
P0: A client can now read another tenant's memory by supplying that tenant's `org_id` (or `x-org-id` metadata): this value is accepted as the search scope without being bound to a verified principal. The newly scoped `search_memories` call therefore turns a caller-controlled identifier into cross-tenant data access. Deriving the organization from an authenticated `RequestPrincipal` at the transport boundary, and rejecting any conflicting argument/metadata value, would preserve the tenant boundary.</violation>
<violation number="2" location="crates/mcb-server/src/handlers/search.rs:250">
P1: Memory search with no `org_id` will stop returning PostToolUse execution observations. The new scope is the UUID-v5 `OrgContext` default, while the hook writer persists those observations under the raw bootstrap `DEFAULT_ORG_ID`; tenant-scoped lookup treats them as different organizations. Standardizing the hook/session paths and `resolve_org_id(None)` on one canonical bootstrap ID before applying this scope would keep generated memory searchable.</violation>
</file>
<file name="crates/mcb-providers/src/vector_store/mod.rs">
<violation number="1" location="crates/mcb-providers/src/vector_store/mod.rs:30">
P0: Selecting the newly enabled provider leaves every organization in the same Weaviate class with no tenant selection or tenant-aware query/write path. As a result, code-context reads and writes are not isolated by org, despite the ADR requiring native Weaviate tenants for this boundary. It would be safer to keep this provider unavailable until `org_id` is threaded through the vector-store API and every schema/object/query operation configures and supplies the corresponding Weaviate tenant.</violation>
</file>
<file name="crates/mcb-validate/src/validators/declarative_validator/metrics_rules.rs">
<violation number="1" location="crates/mcb-validate/src/validators/declarative_validator/metrics_rules.rs:26">
P1: Cyclomatic-complexity rules are evaluated as cognitive-complexity rules in this new path. `MetricThresholds::from_metrics_config` currently stores a `cyclomatic_complexity` setting under `MetricType::CognitiveComplexity`, so the built-in METRIC004 rule can emit METRIC001 findings and never test cyclomatic values. Correct that conversion to populate `MetricType::CyclomaticComplexity` before using it here.</violation>
<violation number="2" location="crates/mcb-validate/src/validators/declarative_validator/metrics_rules.rs:26">
P1: METRIC005 and METRIC006 silently become no-op rules here. Their `halstead_volume` and `maintainability_index` YAML keys are not represented by `MetricsConfig` or evaluated by `RcaAnalyzer`, so this conversion accepts them but creates no threshold or violation. Please implement those metric types end-to-end, or reject unsupported metric keys during rule loading so the configured rules do not appear enforced.</violation>
<violation number="3" location="crates/mcb-validate/src/validators/declarative_validator/metrics_rules.rs:62">
P2: Custom metric rule IDs are lost in the reported violations. The rule ID is only logged here, while `MetricViolation` derives its ID from the metric type, so findings from a rule such as `TEAM001` are reported as `METRIC001`. Carry the `ValidatedRule.id` into the metric violation (or use a declarative-rule violation wrapper) so configured rule IDs remain actionable downstream.</violation>
<violation number="4" location="crates/mcb-validate/src/validators/declarative_validator/metrics_rules.rs:62">
P2: Metric-rule applicability filters are ignored, so a custom metric rule intended to skip generated paths or target only selected files still evaluates every collected Rust file. Pass the existing filter executor/dependency context into this slice and gate each rule/file pair with `should_execute_on_file`, as the AST and regex slices do.</violation>
<violation number="5" location="crates/mcb-validate/src/validators/declarative_validator/metrics_rules.rs:69">
P1: The nesting-depth rule is loaded but can never report a violation. `find_violations_in_functions` does not calculate or test `MetricType::NestingDepth`, so METRIC003 is silently skipped when this call runs. Add nesting-depth extraction and a corresponding threshold check before treating that YAML rule as supported.</violation>
</file>
<file name="crates/mcb-server/src/handlers/memory/session.rs">
<violation number="1" location="crates/mcb-server/src/handlers/memory/session.rs:132">
P1: Default-org session summaries become unavailable after this change: an omitted `org_id` now queries the derived `OrgContext` UUID, while existing rows (and the still-active `session::summarize` create path) use the raw `DEFAULT_ORG_ID`. Because the repository filters `OrgId` exactly, `memory get` reports not found despite a matching session summary. Consider migrating the legacy default-org rows and updating every session-summary writer to the same org-ID representation before enabling this scoped lookup (or add a narrowly scoped compatibility path during migration).</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: An unreachable, unauthorized, or rate-limited Weaviate instance is reported as both a missing collection and a healthy vector store, because this arm converts every request failure to `false`. Only the documented missing-class 404 should become `Ok(false)`; propagating other errors preserves the health-check signal and prevents initialization from attempting to create a collection after a connectivity failure.</violation>
<violation number="2" location="crates/mcb-providers/src/vector_store/weaviate/admin.rs:56">
P2: A GraphQL error response can be reported as an active collection with zero vectors here: `request` only rejects HTTP failures, but this path does not apply the Weaviate client's existing `errors` check before defaulting a missing count to zero. Consider treating nonempty top-level GraphQL errors as `unknown` so failed aggregate queries do not look like valid empty collections.</violation>
</file>
<file name="crates/mcb-server/src/hooks/processor.rs">
<violation number="1" location="crates/mcb-server/src/hooks/processor.rs:80">
P1: Post-tool observations from a non-default organization are being written into the bootstrap organization. The router already supplies `ToolExecutionContext.org_id` to the hook trigger, but this hard-coded value discards it before storage, so the observation is absent from the caller's tenant and its tool/status metadata is attributed to the default tenant. Thread the execution-context org ID through `PostToolUseContext`/the hook trigger and use it here (with the default only as the no-context fallback).</violation>
<violation number="2" location="crates/mcb-server/src/hooks/processor.rs:128">
P2: A session-start hook cannot retrieve context for any non-bootstrap organization because this new required search scope is always the default org. Since `SessionStartContext` has no org field, every non-default session will get an empty/wrong-tenant search regardless of its session ID. Carry the resolved organization into the session-start context and pass that value to `memory_search`.</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:25">
P2: Cargo dependency validation follows directory symlinks, so this checkout descends through `.agents/skills/*` into the external `flext` tree. That makes results depend on manifests outside `workspace_root` and can substantially expand the scan (or repeatedly traverse a symlink cycle). Use the `DirEntry` file type without following links, consistent with the existing inventory walker.</violation>
<violation number="2" location="crates/mcb-validate/src/engines/rusty_cargo_deps.rs:27">
P1: A rule targeted at a specific crate is evaluated against every manifest in the workspace, so a dependency in an unrelated crate can make the target crate fail validation. The rule format already provides `target`/`crate_name`; consider selecting that manifest (or passing the target through this helper) before testing the dependency prefix.</violation>
</file>
<file name="crates/mcb-infrastructure/src/services/memory_service/observation.rs">
<violation number="1" location="crates/mcb-infrastructure/src/services/memory_service/observation.rs:107">
P1: Memory tenant selection is still controlled by the MCP request. An explicit `args.org_id` is forwarded unchanged into this new persisted field, and the repository auto-creates the chosen organization/project instead of checking membership, so a caller can write into (and, through the similarly parameterized memory read paths, access) another tenant by supplying its ID. Deriving this value from an authenticated `RequestPrincipal` and rejecting mismatched request fields would make the new tenant scope enforceable.</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:29">
P1: Authenticated Weaviate deployments cannot be used through the normal application configuration: `config.api_key` is always `None` in the production `build_vector_store_config` path, and `VectorStoreConfigContainer` rejects an `api_key` setting. As a result this registered provider never emits its Authorization header and all requests to an auth-enabled endpoint fail. Consider adding a vector-store API-key field and plumbing it into `VectorStoreProviderConfig` (or otherwise sourcing the credential through the supported configuration path).</violation>
</file>
<file name="crates/mcb-providers/src/vector_store/weaviate/browser.rs">
<violation number="1" location="crates/mcb-providers/src/vector_store/weaviate/browser.rs:26">
P1: Collections returned by the Weaviate browser cannot be browsed again: the schema exposes physical names such as `Mcbmy_collection`, but later operations pass that name through `class_name` and query `McbMcbmy_collection`. Convert the physical class name back to the logical collection name here (and skip classes without this provider's prefix) before constructing `CollectionInfo`.</violation>
<violation number="2" location="crates/mcb-providers/src/vector_store/weaviate/browser.rs:54">
P2: `get_chunks_by_file` will return normally indexed chunks with `start_line` set to `0`, so the new browser view loses line locations and its following sort is ineffective. The standard writer stores `start_line` as a JSON string while the result mapper only accepts numeric JSON; make the mapper also parse string line numbers (or store this metadata as a number) before relying on `SearchResult::start_line`.</violation>
</file>
<file name=".github/actions/native-deps/action.yml">
<violation number="1" location=".github/actions/native-deps/action.yml:30">
P1: The Windows dependency bootstrap now downloads and unzips ONNX Runtime without validating the archive hash first. That leaves this CI path vulnerable to tampered or replaced release artifacts and is inconsistent with the checksum verification already used in `.github/setup-ci.sh` for ONNX Runtime on other platforms. Adding a pinned SHA-256 check before `Expand-Archive` would keep the Windows path at the same integrity level as the Unix installers.</violation>
</file>
<file name="crates/mcb-server/src/utils/args.rs">
<violation number="1" location="crates/mcb-server/src/utils/args.rs:13">
P1: MCP clients can request an effectively unbounded result set because this helper accepts every `u32` unchanged. For example, a memory request with `limit: 4294967295` is expanded by the handler and hybrid-memory search before being sent as the vector-store result limit, which can cause an enormous backend query/response (or provider failure) on the unauthenticated `/mcp` route. Consider giving `resolve_limit` an explicit per-operation maximum (and rejecting or clamping values above it) before any over-fetch multipliers are applied.</violation>
</file>
<file name="CLAUDE.md">
<violation number="1" location="CLAUDE.md:1">
P2: Valid operator-directed work can be incorrectly blocked because this mandatory prelude says its rules are “not negotiable,” while the governing `AGENTS.md` explicitly makes operator instructions supersede all other authority, including injected instructions. Keeping a second, conflicting authority block in `CLAUDE.md` also contradicts the file's new instruction not to duplicate rules. Consider removing the prelude and relying on `AGENTS.md` as the canonical governance source.</violation>
</file>
<file name="crates/mcb-validate/src/validators/naming/validator.rs">
<violation number="1" location="crates/mcb-validate/src/validators/naming/validator.rs:91">
P2: Restricted-visibility constants and statics are now skipped by the naming check. `^\s*` makes the optional `pub\s+` prefix responsible for the whole visibility, but it does not accept `pub(crate)`, `pub(super)`, or `pub(in ...)`; a declaration such as `pub(crate) static bad_name: T = ...` therefore produces no violation. Extending the visibility portion to cover `pub(...)` preserves the new start-of-line filtering without losing these declarations.</violation>
</file>
<file name="crates/mcb-validate/src/rules/rule_types.rs">
<violation number="1" location="crates/mcb-validate/src/rules/rule_types.rs:74">
P2: Metric language scopes are currently ignored. A rule such as `METRIC001`, which declares only Rust/Python/JavaScript/TypeScript, is still evaluated for every RCA-supported file, including Java or C++. Filter the rule/threshold before analysis using this field (or move the scope into the existing rule filters) so configured language restrictions are honored.</violation>
</file>
<file name="crates/mcb/src/cli/serve.rs">
<violation number="1" location="crates/mcb/src/cli/serve.rs:52">
P2: A failed MCP stdio transport now exits `mcb serve --stdio` successfully. `serve_stdio()` can return an error for startup or `waiting()` failures, but the shutdown watch only carries `true` and `wait()` discards it, so this branch reaches `return Ok(())` in both the success and failure cases. This prevents MCP clients or a supervising process from detecting and restarting a failed server; consider carrying the task result through the shutdown notification and propagating it from `execute`.</violation>
</file>
<file name="crates/mcb-validate/src/validators/declarative_validator/ast_rules.rs">
<violation number="1" location="crates/mcb-validate/src/validators/declarative_validator/ast_rules.rs:36">
P2: Non-Rust AST selector and `ast_query` rules silently never run because this iterator receives a file list filtered to `LanguageId::Rust` upstream. The AST engines already support Python, JavaScript/TypeScript, Go, Java, and C++, so the declarative file collection should include the languages that AST rules can target (while retaining the existing Rust-only scope where needed for the other slices).</violation>
</file>
<file name=".serena/memories/coding_standards.md">
<violation number="1" location=".serena/memories/coding_standards.md:5">
P2: Non-VCS failures will be recorded and displayed as VCS errors if this example is followed. The workspace has category-specific factories (and `mcb-utils` has its own `UtilsError`), so this guidance should point contributors to the constructor appropriate to the crate and failure category rather than presenting `Error::vcs` as the standard constructor.</violation>
<violation number="2" location=".serena/memories/coding_standards.md:17">
P2: This blanket ban conflicts with the established opaque-DI boundary: `mcb-domain` intentionally uses `Any` to carry infrastructure-owned configuration and database types without reversing the dependency flow. Narrow the rule to prohibit unchecked casts while explicitly preserving the existing opaque-DI exception, so work on registries and service resolution is not driven toward an architectural violation.</violation>
</file>
<file name=".agents/skills/scripts-maintenance">
<violation number="1" location=".agents/skills/scripts-maintenance:1">
P2: A normal clone exposes this skill as a dangling symlink: its relative target requires a sibling `flext` checkout, but no tracked submodule or setup step provides one. Agents following this newly added skill will be unable to load it outside the author's workspace. Consider committing the skill (or a self-contained pointer) here, or provisioning and validating the external source as part of repository setup.</violation>
</file>
<file name="crates/mcb-validate/src/validators/hygiene/quality.rs">
<violation number="1" location="crates/mcb-validate/src/validators/hygiene/quality.rs:193">
P2: Configured Tokio tests, such as `#[tokio::test(flavor = "multi_thread")]`, are no longer scanned by the hygiene-quality checks because this equality test only accepts the argument-free spelling. Recognizing the documented parenthesized form preserves coverage for those tests.</violation>
</file>
<file name=".github/setup-ci.sh">
<violation number="1" location=".github/setup-ci.sh:151">
P2: Setup now installs `sccache` via Cargo before the workflow pins Rust with `dtolnay/rust-toolchain`, so CI depends on whatever Rust toolchain is preinstalled on the runner. In current workflows, `.github/setup-ci.sh` runs first, which means an image change (missing/older ambient Rust) can fail setup before the intended toolchain is installed. It would be safer to install/pin Rust before this step, or make this install path resilient when `cargo`/`rustc` are not yet ready.</violation>
</file>
<file name="crates/mcb/src/loco_app.rs">
<violation number="1" location="crates/mcb/src/loco_app.rs:170">
P2: When `mcp.stdio_only` is enabled, this `serve` path waits only for OS signals, so the process can stay alive even after the stdio MCP transport has already terminated (for example, client disconnect/EOF). The initializer already publishes `StdioServerShutdown` for stdio lifecycle completion, so using that (or selecting between it and OS signals) here would keep stdio-only shutdown behavior consistent and prevent hangs.</violation>
</file>
<file name="crates/mcb-server/src/handlers/memory/inject.rs">
<violation number="1" location="crates/mcb-server/src/handlers/memory/inject.rs:33">
P2: An oversized user-supplied `max_tokens` can overflow while converting the token budget to characters. Since the MCP argument is an unbounded `usize`, values at or above `usize::MAX / CHARS_PER_TOKEN_ESTIMATE + 1` make this multiplication panic with overflow checks or wrap the budget otherwise. Use checked/saturating arithmetic here and preferably bound `max_tokens` in the argument schema so an invalid budget returns a parameter error.</violation>
</file>
<file name="docs/MCP_TOOLS.md">
<violation number="1" location="docs/MCP_TOOLS.md:221">
P2: The new `repo_path`/`repo_id` contract advertises a plain-filesystem fallback and conditional `repo_id` requirement, but runtime provenance enforcement still depends on repository discovery and treats both fields as required for gated tools. This can cause users in non-repo workspaces to follow the docs and still hit unexpected `Missing execution provenance` errors. Consider aligning the table to the current enforcement (or implementing the documented fallback semantics).</violation>
</file>
<file name=".scope/config.toml">
<violation number="1" location=".scope/config.toml:1">
P2: Scope indexing will now be driven by a second, hand-maintained set of language, filtering, and embedding defaults, so later changes to the canonical settings can leave agent context stale or inconsistent. The repository rules explicitly reserve configurable facts for `config/*.yaml`/settings and generated projections; consider adding these values to that owner and generating `.scope/config.toml` from it (with a regeneration check) rather than committing independent literals here.</violation>
</file>
<file name="crates/mcb-validate/src/validators/hygiene/function_naming.rs">
<violation number="1" location="crates/mcb-validate/src/validators/hygiene/function_naming.rs:85">
P2: A comment or doc comment between a test attribute and its function now makes the validator abandon that test, allowing missing assertions to evade the hygiene check. Skipping line comments along with blank lines and attributes keeps the new nearby-function boundary without rejecting documented tests.</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: A `handler.rs` moved outside an allowed handler directory will no longer be reported, because the new exact-name exclusion makes it fail `RoleWord` matching before the directory rule runs. The same blanket exemption also applies to plain provider, adapter, and module files. The false-positive exception for the domain `entities/repository.rs` would be safer when scoped to that ambiguous case (or otherwise made role/context-specific), while exact role filenames in the other CA rules remain subject to placement validation.</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
| /// `org_id` enforces tenant isolation: never returns another org's observation. | ||
| async fn get_observation( | ||
| &self, | ||
| org_id: &str, |
There was a problem hiding this comment.
P0: Memory reads remain cross-tenant accessible to a caller that supplies another organization's ID. The new org_id parameter only scopes the query to the caller-selected string: MemoryArgs accepts it and resolve_org_id returns it unchanged before the handler calls this interface. Deriving the scope from an authenticated request principal, rather than from args.org_id, would make the new tenant filters enforce an actual boundary.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-domain/src/ports/services/memory.rs, line 65:
<comment>Memory reads remain cross-tenant accessible to a caller that supplies another organization's ID. The new `org_id` parameter only scopes the query to the caller-selected string: `MemoryArgs` accepts it and `resolve_org_id` returns it unchanged before the handler calls this interface. Deriving the scope from an authenticated request principal, rather than from `args.org_id`, would make the new tenant filters enforce an actual boundary.</comment>
<file context>
@@ -54,25 +57,38 @@ pub trait ObservationManager: Send + Sync {
+ /// `org_id` enforces tenant isolation: never returns another org's observation.
+ async fn get_observation(
+ &self,
+ org_id: &str,
+ id: &ObservationId,
+ ) -> Result<Option<Observation>>;
</file context>
| ..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: A client can now read another tenant's memory by supplying that tenant's org_id (or x-org-id metadata): this value is accepted as the search scope without being bound to a verified principal. The newly scoped search_memories call therefore turns a caller-controlled identifier into cross-tenant data access. Deriving the organization from an authenticated RequestPrincipal at the transport boundary, and rejecting any conflicting argument/metadata value, would preserve the tenant boundary.
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>A client can now read another tenant's memory by supplying that tenant's `org_id` (or `x-org-id` metadata): this value is accepted as the search scope without being bound to a verified principal. The newly scoped `search_memories` call therefore turns a caller-controlled identifier into cross-tenant data access. Deriving the organization from an authenticated `RequestPrincipal` at the transport boundary, and rejecting any conflicting argument/metadata value, would preserve the tenant boundary.</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>
| pub mod pinecone; | ||
| /// Qdrant vector search engine provider. | ||
| pub mod qdrant; | ||
| pub mod weaviate; |
There was a problem hiding this comment.
P0: Selecting the newly enabled provider leaves every organization in the same Weaviate class with no tenant selection or tenant-aware query/write path. As a result, code-context reads and writes are not isolated by org, despite the ADR requiring native Weaviate tenants for this boundary. It would be safer to keep this provider unavailable until org_id is threaded through the vector-store API and every schema/object/query operation configures and supplies the corresponding Weaviate tenant.
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/mod.rs, line 30:
<comment>Selecting the newly enabled provider leaves every organization in the same Weaviate class with no tenant selection or tenant-aware query/write path. As a result, code-context reads and writes are not isolated by org, despite the ADR requiring native Weaviate tenants for this boundary. It would be safer to keep this provider unavailable until `org_id` is threaded through the vector-store API and every schema/object/query operation configures and supplies the corresponding Weaviate tenant.</comment>
<file context>
@@ -26,6 +27,7 @@ pub mod milvus;
pub mod pinecone;
/// Qdrant vector search engine provider.
pub mod qdrant;
+pub mod weaviate;
// Re-export for convenience
</file context>
| .filter_map(|rule| { | ||
| rule.metrics | ||
| .as_ref() | ||
| .map(|cfg| (rule, MetricThresholds::from_metrics_config(cfg))) |
There was a problem hiding this comment.
P1: Cyclomatic-complexity rules are evaluated as cognitive-complexity rules in this new path. MetricThresholds::from_metrics_config currently stores a cyclomatic_complexity setting under MetricType::CognitiveComplexity, so the built-in METRIC004 rule can emit METRIC001 findings and never test cyclomatic values. Correct that conversion to populate MetricType::CyclomaticComplexity before using it here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-validate/src/validators/declarative_validator/metrics_rules.rs, line 26:
<comment>Cyclomatic-complexity rules are evaluated as cognitive-complexity rules in this new path. `MetricThresholds::from_metrics_config` currently stores a `cyclomatic_complexity` setting under `MetricType::CognitiveComplexity`, so the built-in METRIC004 rule can emit METRIC001 findings and never test cyclomatic values. Correct that conversion to populate `MetricType::CyclomaticComplexity` before using it here.</comment>
<file context>
@@ -0,0 +1,78 @@
+ .filter_map(|rule| {
+ rule.metrics
+ .as_ref()
+ .map(|cfg| (rule, MetricThresholds::from_metrics_config(cfg)))
+ })
+ .collect();
</file context>
| match memory_service.get_session_summary(session_id).await { | ||
| let org_id = resolve_org_id(args.org_id.as_deref()); | ||
| match memory_service | ||
| .get_session_summary(&org_id, session_id) |
There was a problem hiding this comment.
P1: Default-org session summaries become unavailable after this change: an omitted org_id now queries the derived OrgContext UUID, while existing rows (and the still-active session::summarize create path) use the raw DEFAULT_ORG_ID. Because the repository filters OrgId exactly, memory get reports not found despite a matching session summary. Consider migrating the legacy default-org rows and updating every session-summary writer to the same org-ID representation before enabling this scoped lookup (or add a narrowly scoped compatibility path during migration).
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/memory/session.rs, line 132:
<comment>Default-org session summaries become unavailable after this change: an omitted `org_id` now queries the derived `OrgContext` UUID, while existing rows (and the still-active `session::summarize` create path) use the raw `DEFAULT_ORG_ID`. Because the repository filters `OrgId` exactly, `memory get` reports not found despite a matching session summary. Consider migrating the legacy default-org rows and updating every session-summary writer to the same org-ID representation before enabling this scoped lookup (or add a narrowly scoped compatibility path during migration).</comment>
<file context>
@@ -129,7 +127,11 @@ pub async fn get_session(
- match memory_service.get_session_summary(session_id).await {
+ let org_id = resolve_org_id(args.org_id.as_deref());
+ match memory_service
+ .get_session_summary(&org_id, session_id)
+ .await
+ {
</file context>
| ) -> InjectedMemoryContext { | ||
| let mut context = String::new(); | ||
| let mut observation_ids = Vec::new(); | ||
| let max_chars = max_tokens * CHARS_PER_TOKEN_ESTIMATE; |
There was a problem hiding this comment.
P2: An oversized user-supplied max_tokens can overflow while converting the token budget to characters. Since the MCP argument is an unbounded usize, values at or above usize::MAX / CHARS_PER_TOKEN_ESTIMATE + 1 make this multiplication panic with overflow checks or wrap the budget otherwise. Use checked/saturating arithmetic here and preferably bound max_tokens in the argument schema so an invalid budget returns a parameter error.
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/memory/inject.rs, line 33:
<comment>An oversized user-supplied `max_tokens` can overflow while converting the token budget to characters. Since the MCP argument is an unbounded `usize`, values at or above `usize::MAX / CHARS_PER_TOKEN_ESTIMATE + 1` make this multiplication panic with overflow checks or wrap the budget otherwise. Use checked/saturating arithmetic here and preferably bound `max_tokens` in the argument schema so an invalid budget returns a parameter error.</comment>
<file context>
@@ -12,48 +13,67 @@ use rmcp::model::CallToolResult;
+) -> InjectedMemoryContext {
+ let mut context = String::new();
+ let mut observation_ids = Vec::new();
+ let max_chars = max_tokens * CHARS_PER_TOKEN_ESTIMATE;
+ for result in results {
+ let obs = result.observation;
</file context>
Suggested change
| let max_chars = max_tokens * CHARS_PER_TOKEN_ESTIMATE; | |
| let max_chars = max_tokens.saturating_mul(CHARS_PER_TOKEN_ESTIMATE); |
Comment on lines
+221
to
+222
| | `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 | — | |
There was a problem hiding this comment.
P2: The new repo_path/repo_id contract advertises a plain-filesystem fallback and conditional repo_id requirement, but runtime provenance enforcement still depends on repository discovery and treats both fields as required for gated tools. This can cause users in non-repo workspaces to follow the docs and still hit unexpected Missing execution provenance errors. Consider aligning the table to the current enforcement (or implementing the documented fallback semantics).
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 221:
<comment>The new `repo_path`/`repo_id` contract advertises a plain-filesystem fallback and conditional `repo_id` requirement, but runtime provenance enforcement still depends on repository discovery and treats both fields as required for gated tools. This can cause users in non-repo workspaces to follow the docs and still hit unexpected `Missing execution provenance` errors. Consider aligning the table to the current enforcement (or implementing the documented fallback semantics).</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 | — |
+| `project_id` | string | no | Git remote `origin` (`owner/repo`); absent for plain filesystem workspaces | — |
</file context>
Suggested change
| | `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 | — | | |
| | `repo_path` | string | **yes** | Discovered by walking CWD ancestors and opening a repository via the configured VCS provider (current default: Git) | `Missing execution provenance ... repo_path` | | |
| | `repo_id` | string | **yes** | Derived from the opened repository via `vcs.repository_id(&repo)` | `Missing execution provenance ... repo_id` | |
| @@ -0,0 +1,33 @@ | |||
| [project] | |||
There was a problem hiding this comment.
P2: Scope indexing will now be driven by a second, hand-maintained set of language, filtering, and embedding defaults, so later changes to the canonical settings can leave agent context stale or inconsistent. The repository rules explicitly reserve configurable facts for config/*.yaml/settings and generated projections; consider adding these values to that owner and generating .scope/config.toml from it (with a regeneration check) rather than committing independent literals here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .scope/config.toml, line 1:
<comment>Scope indexing will now be driven by a second, hand-maintained set of language, filtering, and embedding defaults, so later changes to the canonical settings can leave agent context stale or inconsistent. The repository rules explicitly reserve configurable facts for `config/*.yaml`/settings and generated projections; consider adding these values to that owner and generating `.scope/config.toml` from it (with a regeneration check) rather than committing independent literals here.</comment>
<file context>
@@ -0,0 +1,33 @@
+[project]
+name = "mcb"
+languages = [
</file context>
| }) | ||
| for (line_idx, potential_fn) in lines.iter().enumerate().skip(start_idx) { | ||
| let trimmed = potential_fn.trim(); | ||
| if trimmed.is_empty() || trimmed.starts_with("#[") { |
There was a problem hiding this comment.
P2: A comment or doc comment between a test attribute and its function now makes the validator abandon that test, allowing missing assertions to evade the hygiene check. Skipping line comments along with blank lines and attributes keeps the new nearby-function boundary without rejecting documented tests.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-validate/src/validators/hygiene/function_naming.rs, line 85:
<comment>A comment or doc comment between a test attribute and its function now makes the validator abandon that test, allowing missing assertions to evade the hygiene check. Skipping line comments along with blank lines and attributes keeps the new nearby-function boundary without rejecting documented tests.</comment>
<file context>
@@ -75,28 +70,26 @@ pub fn validate_test_function_naming(config: &ValidationConfig) -> Result<Vec<Hy
- })
+ for (line_idx, potential_fn) in lines.iter().enumerate().skip(start_idx) {
+ let trimmed = potential_fn.trim();
+ if trimmed.is_empty() || trimmed.starts_with("#[") {
+ continue;
+ }
</file context>
Suggested change
| if trimmed.is_empty() || trimmed.starts_with("#[") { | |
| if trimmed.is_empty() | |
| || trimmed.starts_with(COMMENT_PREFIX) | |
| || trimmed.starts_with("#[") | |
| { |
| } | ||
|
|
||
| fn role_word_matches(file_name: &str, role: &str) -> bool { | ||
| file_name != role && file_name.split('_').any(|part| part == role) |
There was a problem hiding this comment.
P2: A handler.rs moved outside an allowed handler directory will no longer be reported, because the new exact-name exclusion makes it fail RoleWord matching before the directory rule runs. The same blanket exemption also applies to plain provider, adapter, and module files. The false-positive exception for the domain entities/repository.rs would be safer when scoped to that ambiguous case (or otherwise made role/context-specific), while exact role filenames in the other CA rules remain subject to placement validation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-validate/src/validators/naming/checks/ca.rs, line 47:
<comment>A `handler.rs` moved outside an allowed handler directory will no longer be reported, because the new exact-name exclusion makes it fail `RoleWord` matching before the directory rule runs. The same blanket exemption also applies to plain provider, adapter, and module files. The false-positive exception for the domain `entities/repository.rs` would be safer when scoped to that ambiguous case (or otherwise made role/context-specific), while exact role filenames in the other CA rules remain subject to placement validation.</comment>
<file context>
@@ -32,17 +32,21 @@ fn ca_violation(
}
+fn role_word_matches(file_name: &str, role: &str) -> bool {
+ file_name != role && file_name.split('_').any(|part| part == role)
+}
+
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitFlow audit verdict (automated 2026-08-02)
main: 11 commits | behind: 0origin/main: clean/home/marlonsc/mcb/.worktrees/mcb-o96i-19-sccache-bootstrap-integrationVerdict: KEEP-OPEN for review — pushed by gitflow audit; not auto-merged per policy.
Summary by cubic
Integrated
sccacheandcargo-nextest, and consolidated CI with reusable actions to speed up builds and stabilize tests across platforms. Also standardized agent rule pointers toAGENTS.mdand added an automated sync check.Refactors
.github/actions/native-depsand.github/actions/warm-fastembed.main; added agent pointer sync job; enabledtyposin lint.workflow_dispatchtags and resilient publish on partial builds.Dependencies
sccachemandatory; CI setsRUSTC_WRAPPERand enablesSCCACHE_GHA_ENABLED.cargo-nextestadded with.config/nextest.tomlto serialize shared-context tests.typos-cliandcargo-auditinstall paths via.github/setup-ci.sh(withcargo-binstallfallback)..mise.tomltoolchain pins.Written for commit c73f74e. Summary will update on new commits.