Feature/v0 3 2 ci gates - #150
Open
marlon-costa-dc wants to merge 70 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
…ag for changelog fallback
…all when available
…ffline mcb-v5an.16
…gineering principles mcb-8079
…net-negative LOC mcb-8079
…out (mcb-v5an.11)
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 148 files, which is 48 over the limit of 100. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (157)
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 QodoHarden v0.3.2 CI, release, and local development gates
AI Description
Diagram
High-Level Assessment
Files changed (149)
|
Contributor
Code Review by Qodo
Context used✅ Compliance rules (platform):
36 rules 1. Conflicting package duplicated in lockfile
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| /// Securely erase a string by overwriting its buffer | ||
| #[allow(unsafe_code)] | ||
| #[allow(unsafe_code)] // Why: zeroizing String bytes requires unsafe as_mut_bytes(); exclusive access guarantees safety. |
Contributor
There was a problem hiding this comment.
1. erase_string uses unsafe block 📘 Rule violation ⛨ Security
SecureErasure::erase_string contains an unsafe block (and explicitly allows unsafe_code), which violates the project requirement to avoid unsafe Rust in production code. This can reintroduce memory-safety risk and bypasses the intended unsafe_code denial policy.
Agent Prompt
## Issue description
`SecureErasure::erase_string` currently uses an `unsafe` block (and `#[allow(unsafe_code)]`), which violates the repo policy forbidding unsafe Rust in non-test code.
## Issue Context
The PR modifies the `#[allow(unsafe_code)]` line for `erase_string`, but the function still performs unsafe byte access on a `String`.
## Fix Focus Areas
- crates/mcb-utils/src/utils/crypto.rs[57-66]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+30
to
+33
| 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. load_app_config defaults loco_env 📘 Rule violation ☼ Reliability
load_app_config() defaults LOCO_ENV to test when missing, instead of failing fast at startup. This can start the application under an unintended environment configuration.
Agent Prompt
## Issue description
`load_app_config()` uses a hard-coded fallback (`"test"`) when `LOCO_ENV` is unset, rather than failing fast.
## Issue Context
The compliance rule requires required external configuration to cause an explicit startup failure when missing.
## Fix Focus Areas
- crates/mcb-infrastructure/src/config/loader.rs[29-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+201
to
+205
| "go", | ||
| tree_sitter_go::LANGUAGE.into(), | ||
| tree_sitter_go::HIGHLIGHTS_QUERY, | ||
| ), | ||
| L::Java => ( |
Contributor
There was a problem hiding this comment.
3. New file exceeds 200 lines 📘 Rule violation ⚙ Maintainability
highlight_sync_service.rs is introduced at >200 lines (line numbers exceed 200), violating the guideline to split large source files into submodules. This increases maintenance burden and review difficulty.
Agent Prompt
## Issue description
A newly added production Rust source file exceeds ~200 lines and should be split into smaller modules.
## Issue Context
The file `highlight_sync_service.rs` is introduced at ~292 lines, which exceeds the rule threshold.
## Fix Focus Areas
- crates/mcb-infrastructure/src/services/highlight_sync_service.rs[1-292]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+148
to
+152
| # 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 |
Contributor
There was a problem hiding this comment.
4. Setup-ci requires cargo pre-rust 🐞 Bug ☼ Reliability
The CI setup now assumes sccache is available by installing it via cargo in .github/setup-ci.sh and by exporting RUSTC_WRAPPER=sccache in the Makefile, but several release/publish workflows run the native-deps/setup step before the Rust toolchain is installed and the Makefile’s best-effort install can be ignored. If cargo is not present or the install fails, subsequent builds/releases can abort with cargo: command not found or because the sccache wrapper binary cannot be executed.
Agent Prompt
## Issue description
CI/release/publish and local builds can fail because `sccache` is treated as mandatory in two places: `.github/setup-ci.sh` installs it using `cargo` when missing, but some workflows call the script before installing the Rust toolchain (so `cargo` may not exist), and the Makefile exports `RUSTC_WRAPPER=sccache` unconditionally while only best-effort attempting to install `sccache`, leaving Cargo configured to use a missing wrapper.
## Issue Context
- `native-deps` composite action runs `bash .github/setup-ci.sh ...` on Unix.
- `.github/setup-ci.sh` installs `sccache` via `_mcb_install_crate` / `cargo binstall/install` if it isn’t present.
- In `release.yml`, `native-deps` runs before `dtolnay/rust-toolchain` in `release-build`, and also in `publish-crates` when a registry token exists, so `cargo` may be unavailable when `setup-ci.sh` runs.
- The Makefile sets `RUSTC_WRAPPER := sccache` globally, and the installation attempt happens at Makefile parse-time with errors suppressed (`|| true`), so even if installation fails (or `cargo` is missing) subsequent `cargo` builds still attempt to invoke `sccache` and fail.
## Fix Focus Areas
- .github/setup-ci.sh[136-152]
- .github/workflows/release.yml[64-77]
- .github/workflows/release.yml[260-265]
- .github/actions/native-deps/action.yml[16-19]
- Makefile[35-44]
## Suggested fix
- Reorder `release.yml` steps so `dtolnay/rust-toolchain` runs before `./.github/actions/native-deps` (preferred), and/or
- Update `setup-ci.sh` to only attempt cargo-based installation when `cargo` exists (otherwise skip with a clear message or fail with instructions to install Rust first), and/or
- Install `sccache` via `mozilla-actions/sccache-action` in release/publish jobs before calling `native-deps`.
- In the Makefile, only export `RUSTC_WRAPPER` when `command -v sccache` succeeds, and consider moving installation to an explicit setup target (or making it opt-in via `SCCACHE=1`) so failures are explicit and there is a clean fallback when `sccache` is unavailable.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+419
to
+426
| 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.
5. Auto-create blocks tool calls 🐞 Bug ☼ Reliability
McpServer::handle_request now propagates errors from auto_create_session_and_project, and auto_create_session returns an error on any DB insert failure; if the session id already exists (e.g., persisted client session id after a server restart), the first tool call can fail and the DashSet guard prevents retrying creation.
Agent Prompt
### Issue description
`auto_create_session_and_project(...).await?` makes tool execution fail when auto-session/project creation fails. `auto_create_session` also inserts the session id into `init_sessions` **before** attempting DB creation; if the DB insert fails (including due to duplicate key), the tool call fails and the process will not retry creating that session.
### Issue Context
- The SeaORM `create_session` path uses a plain insert (`sea_repo_insert!`) which is not idempotent.
- The HTTP bridge can persist a session id via a session file; that makes duplicate session ids plausible across server restarts.
### Fix Focus Areas
- crates/mcb-server/src/mcp_server.rs[270-280]
- crates/mcb-server/src/mcp_server.rs[371-428]
- crates/mcb-providers/src/database/seaorm/repos/agent.rs[99-107]
- crates/mcb-providers/src/database/seaorm/repos/common.rs[95-103]
- crates/mcb-server/src/transport/http_client.rs[105-134]
### Suggested fix
- Do not propagate auto-create DB failures to tool callers: catch DB errors and log them, returning `Ok(())` from `auto_create_*`.
- Make session auto-create idempotent:
- Check `get_session(session_id)` first, or
- Treat unique-constraint violations as success, or
- Remove the id from `init_sessions` on failure so a later request can retry.
- If keeping error propagation for validation failures, split error handling: only propagate deterministic input validation errors, not repository/DB errors.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+380
to
389
| if let Some(machine_id) = hostname::get() | ||
| .ok() | ||
| .and_then(|h| h.into_string().ok()) | ||
| .or_else(|| std::env::var("HOSTNAME").ok()) | ||
| .unwrap_or_else(|| FALLBACK_UNKNOWN.to_owned()); | ||
| builder = builder.header(HEADER_MACHINE_ID, machine_id); | ||
| { | ||
| builder = builder.header(HEADER_MACHINE_ID, machine_id); | ||
| } | ||
|
|
||
| builder = builder.header(HEADER_AGENT_PROGRAM, IDE_MCB_CLIENT); | ||
| builder = builder.header(HEADER_MODEL_ID, FALLBACK_UNKNOWN); | ||
| builder = builder.header(HEADER_DELEGATED, "false"); |
Contributor
There was a problem hiding this comment.
6. Http client loses provenance 🐞 Bug ◔ Observability
The HTTP bridge no longer sends model_id and only sends machine_id when hostname resolution succeeds, so provenance can silently fall back to server-side defaults (wrong machine/model attribution) or be missing depending on environment.
Agent Prompt
### Issue description
`post_mcp_request` no longer emits `model_id` and may omit `machine_id`. When headers are absent, `ToolExecutionContext` will use runtime defaults discovered on the server, which can misattribute provenance to the server host/model instead of the client.
### Issue Context
- Server-side runtime defaults derive `machine_id` from the server hostname and set `model_id` to `FALLBACK_UNKNOWN`.
- Provenance validation treats both fields as required for many tools.
### Fix Focus Areas
- crates/mcb-server/src/transport/http_client.rs[357-391]
- crates/mcb-server/src/tools/defaults.rs[136-159]
- crates/mcb-server/src/tools/validation.rs[64-75]
### Suggested fix
- Always send `HEADER_MACHINE_ID` (use a sentinel like `unknown` if it can’t be resolved).
- Restore sending `HEADER_MODEL_ID` (either from config/env or a sentinel).
- Optionally expand operator id detection to be cross-platform (e.g., `USERNAME` on Windows) to keep provenance complete.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
8
to
+18
| use axum::Router as AxumRouter; | ||
| use loco_rs::Result; | ||
| use loco_rs::app::{AppContext as LocoAppContext, Hooks, Initializer}; | ||
| use loco_rs::boot::{BootResult, StartMode, create_app}; | ||
| use loco_rs::boot::{BootResult, ServeParams, StartMode, create_app}; | ||
| use loco_rs::config::Config as LocoConfig; | ||
| use loco_rs::controller::AppRoutes; | ||
| use loco_rs::environment::Environment; | ||
| use mcb_infrastructure::infrastructure::DynamicMigrator; | ||
| use std::net::SocketAddr; | ||
| use std::path::{Path, PathBuf}; | ||
| use tokio::signal; |
Contributor
There was a problem hiding this comment.
1. Misordered imports in loco_app.rs 📘 Rule violation ⚙ Maintainability
crates/mcb/src/loco_app.rs mixes std imports after external and mcb_* imports, violating the required grouped import ordering. This reduces consistency and can cause style/lint gate failures.
Agent Prompt
## Issue description
`crates/mcb/src/loco_app.rs` has `use std::...` imports placed after external crates and `mcb_*` imports, violating the required grouped/ordered Rust import convention.
## Issue Context
Rule requires grouping imports in this order: (1) `std`, (2) external crates, (3) `mcb_*` crates, (4) local (`crate::/self::/super::`).
## Fix Focus Areas
- crates/mcb/src/loco_app.rs[7-18]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| RES=$$(echo '{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"install-validate","version":"1.0"}}}' | timeout 15 $(INSTALL_DIR)/$(BINARY_NAME) serve --stdio 2>/dev/null); \ | ||
| echo "$$RES" | grep -q '"serverInfo"' && echo " MCP stdio: OK" || { echo " FAIL: MCP stdio no response" >&2; exit 1; }; \ | ||
| [ $$R -eq 8 ] && { echo " FAIL: service not active"; exit 1; } || true; \ | ||
| H=0; while [ $$H -lt 10 ]; do curl -sf http://127.0.0.1:8080/ >/dev/null 2>&1 && { echo " HTTP server: OK"; break; }; H=$$((H+1)); [ $$H -lt 10 ] && sleep 1; done; [ $$H -eq 10 ] && { echo " FAIL: HTTP server not responding" >&2; exit 1; }; \ |
Contributor
There was a problem hiding this comment.
2. Hardcoded 8080 install check 📘 Rule violation ≡ Correctness
makefiles/dispatch.mk hardcodes http://127.0.0.1:8080/ during install-validate, instead of deriving the port from configuration or a Make variable. This can break validation in environments where the HTTP bind/port is configured differently.
Agent Prompt
## Issue description
`install-validate` uses a hardcoded URL/port (`http://127.0.0.1:8080/`) which can diverge from the actual configured HTTP port.
## Issue Context
Ports are environment-dependent configuration values; validation logic should read them from a single source of truth (config file, systemd unit env, or a Make variable).
## Fix Focus Areas
- makefiles/dispatch.mk[241-249]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+76
to
77
| - name: Build | ||
| run: make build RELEASE=1 |
Contributor
There was a problem hiding this comment.
3. Release target ignored 🐞 Bug ≡ Correctness
The release workflows install matrix.target but build via make build RELEASE=1, which runs cargo build --release without --target, so the produced binary is for the runner’s host target while being packaged/named as matrix.target. This can publish mislabeled or incorrect-architecture release artifacts (and effectively skips building the intended non-host target).
Agent Prompt
### Issue description
`.github/workflows/release.yml` (and similarly `ci.yml`’s release-build job) sets `targets: ${{ matrix.target }}` and names artifacts by that target, but the build command is `make build RELEASE=1`. The Makefile’s build dispatcher runs `cargo build --release` without `--target`, which means Cargo builds for the host triple, not the matrix target.
### Issue Context
- Workflow expects per-matrix target artifacts (e.g. `x86_64-apple-darwin`).
- Current Makefile build target does not accept or forward a target.
### Fix Focus Areas
- .github/workflows/release.yml[71-99]
- makefiles/dispatch.mk[59-63]
- .github/workflows/ci.yml[563-585]
### Suggested fix
Choose one:
1) **Workflow-level**: replace `make build RELEASE=1` with `cargo build --release --target "${{ matrix.target }}"` and copy from `target/${{ matrix.target }}/release/...`.
2) **Makefile-level**: add a `TARGET` variable and have `DISPATCH_BUILD` do `cargo build --release --target "$(TARGET)"` when `TARGET` is set; then pass `TARGET=${{ matrix.target }}` from the workflow(s) and adjust copy paths accordingly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+42
to
+46
| /// ``` | ||
| /// use mcb_infrastructure::services::highlight_sync_service::HighlightSyncPort; | ||
| /// fn example(port: &dyn HighlightSyncPort) { | ||
| /// let _ = port.highlight("fn main() {}", "rust"); | ||
| /// } |
Contributor
There was a problem hiding this comment.
4. Doctest hits private module 🐞 Bug ≡ Correctness
highlight_sync_service.rs includes a doctest that imports mcb_infrastructure::services::highlight_sync_service::HighlightSyncPort, but the module is declared private (mod highlight_sync_service;). Doctests compile as an external crate, so `cargo test --doc` will fail to compile this example.
Agent Prompt
### Issue description
The doctest in `highlight_sync_service.rs` imports a private module path (`services::highlight_sync_service`), but `services/mod.rs` declares it as a private module. Rust doctests are compiled as an external consumer, so private module paths are not accessible.
### Issue Context
This will break `cargo test --doc` / any CI gate running doctests.
### Fix Focus Areas
- crates/mcb-infrastructure/src/services/highlight_sync_service.rs[39-51]
- crates/mcb-infrastructure/src/services/mod.rs[19-23]
### Suggested fix
Pick one:
1) Make the module public: change `mod highlight_sync_service;` to `pub mod highlight_sync_service;` (or re-export specific items via `pub use highlight_sync_service::HighlightSyncPort;`).
2) If you *don’t* want it public, change the doctest block to ` ```ignore ` (or remove the external-path import and describe usage without compiling code).
Ensure doctests pass after the change.
ⓘ 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 1d253e6 |
marlon-costa-dc
enabled auto-merge (squash)
July 29, 2026 02:51
Comment on lines
+9
to
+14
| 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.
1. Highlight imports are misordered 📘 Rule violation ⚙ Maintainability
The new module places mcb_domain imports before external crates and then repeats the mcb_domain group. Rust imports must be grouped as std, external crates, mcb_*, then local modules.
Agent Prompt
## Issue description
The imports in `highlight_sync_service.rs` do not follow the required Rust import group order.
## Issue Context
External `tree_sitter` imports must precede all `mcb_*` imports, and the two `mcb_domain` blocks should be contiguous.
## Fix Focus Areas
- crates/mcb-infrastructure/src/services/highlight_sync_service.rs[7-14]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+86
to
+89
| let mut state = self | ||
| .state | ||
| .lock() | ||
| .map_err(|e| HighlightError::HighlightingFailed(format!("Lock poisoned: {e}")))?; |
Contributor
There was a problem hiding this comment.
2. highlighterror bypasses constructors 📘 Rule violation ≡ Correctness
The new highlighting implementation directly constructs the thiserror enum variant HighlightError::HighlightingFailed. Domain errors must be created through constructor methods so their formatting and creation remain centralized.
Agent Prompt
## Issue description
The new infrastructure implementation directly instantiates `HighlightError` variants instead of using domain-provided constructor methods.
## Issue Context
Add appropriate constructor methods alongside the domain error definition if they do not exist, then replace all direct `HighlightError` variant construction in the new highlighting implementation.
## Fix Focus Areas
- crates/mcb-infrastructure/src/services/highlight_sync_service.rs[86-136]
- crates/mcb-infrastructure/src/services/highlight_sync_service.rs[235-279]
- crates/mcb-domain/src/ports/services/browse.rs[27-40]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Add-Content -Path $env:GITHUB_PATH -Value $ortLib | ||
| Add-Content -Path $env:GITHUB_ENV -Value "ORT_DYLIB_PATH=$ortLib\onnxruntime.dll" | ||
| Add-Content -Path $env:GITHUB_ENV -Value "PATH=$ortLib;$env:PATH" | ||
| git clone --depth 1 https://github.com/${{ github.repository }}.git . |
Contributor
There was a problem hiding this comment.
3. Windows jobs clone twice 🐞 Bug ☼ Reliability
The Windows test-cross and release-build legs run actions/checkout and then clone into the already populated workspace. git clone ... . therefore fails before either job reaches tests or compilation.
Agent Prompt
## Issue description
Windows CI jobs clone into a workspace already populated by `actions/checkout`, causing deterministic failures.
## Issue Context
Both `test-cross` and `release-build` contain the duplicated checkout sequence. Preserve the existing checkout and use the native dependency action where dependencies are needed.
## Fix Focus Areas
- .github/workflows/ci.yml[320-338]
- .github/workflows/ci.yml[530-548]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| push: | ||
| tags: | ||
| - "v*" | ||
| # Recovery path: manually (re)publish a GitHub Release for an existing tag without |
Contributor
There was a problem hiding this comment.
4. Manual release builds wrong revision 🐞 Bug ≡ Correctness
The new manual release path accepts an arbitrary existing tag, but release-build checks out the dispatch ref rather than that tag. It can consequently publish binaries built from a branch revision under the requested tag.
Agent Prompt
## Issue description
Manual release artifacts are built from the workflow dispatch ref instead of the requested release tag.
## Issue Context
The input tag controls release naming and publication, but the build checkout has no matching `ref`. Ensure all tag-specific build and documentation jobs operate on the validated input tag.
## Fix Focus Areas
- .github/workflows/release.yml[20-27]
- .github/workflows/release.yml[64-67]
- .github/workflows/release.yml[132-191]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| "stdio-only mode: HTTP server disabled (port {})", | ||
| serve_params.port | ||
| ); | ||
| wait_for_shutdown_signal().await; |
Contributor
There was a problem hiding this comment.
5. Stdio completion cannot exit 🐞 Bug ☼ Reliability
Stdio-only serving now waits exclusively for Ctrl+C or SIGTERM, while the stdio transport runs in a detached task whose completion is ignored. When that transport shuts down, the main process remains alive waiting for an unrelated OS signal.
Agent Prompt
## Issue description
Completion of the stdio transport does not terminate stdio-only serving, leaving the process running.
## Issue Context
The initializer drops the stdio task handle. Introduce a shared shutdown notification or retain and await the handle alongside OS signals, while preserving hybrid HTTP behavior.
## Fix Focus Areas
- crates/mcb/src/loco_app.rs[151-173]
- crates/mcb/src/initializers/mcp_server.rs[325-353]
- crates/mcb-server/src/transport/stdio.rs[34-53]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| kill_duplicate_rust_analyzers() { | ||
| printf "\n--- rust-analyzer duplicate cleanup -----------------------------------------\n" | ||
| local pids | ||
| pids=$(pgrep -a -f 'rust.analyzer|rust-analyzer' 2>/dev/null | grep -v 'grep' | awk '{print $1}' || true) |
Contributor
There was a problem hiding this comment.
6. Optimizer kills unrelated analyzers 🐞 Bug ☼ Reliability
Despite describing KEEP_RA as a per-project limit, the optimizer collects every matching rust-analyzer process and retains only one globally. Running it with --apply can terminate active analyzers serving unrelated repositories.
Agent Prompt
## Issue description
The destructive optimizer applies its rust-analyzer retention limit globally rather than per project.
## Issue Context
Group processes by a verified workspace or working directory before applying limits. Do not terminate a process unless its project ownership can be established safely.
## Fix Focus Areas
- scripts/dev-env-optimize.sh[18-20]
- scripts/dev-env-optimize.sh[107-150]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @case "$(WHAT)" in \ | ||
| hooks) cp scripts/hooks/pre-commit .git/hooks/pre-commit; chmod +x .git/hooks/pre-commit; echo "✓ pre-commit hook installed" ;; \ | ||
| tools) cargo install cargo-udeps cargo-audit cargo-tarpaulin 2>/dev/null || true; echo "✓ tools installed" ;; \ | ||
| hooks) cp scripts/hooks/pre-commit scripts/hooks/pre-push .git/hooks/; chmod +x .git/hooks/pre-commit .git/hooks/pre-push; echo "✓ pre-commit + pre-push hooks installed" ;; \ |
Contributor
There was a problem hiding this comment.
7. Setup overwrites beads hooks 🐞 Bug ≡ Correctness
The setup target directly replaces .git/hooks/pre-commit and pre-push, but only the replacement pre-push delegates to Beads. Running setup after bd hooks install --chain therefore removes the required Beads pre-commit integration and can leave hook policy noncompliant.
Agent Prompt
## Issue description
Project hook installation overwrites chained Beads hooks and drops Beads pre-commit execution.
## Issue Context
Make hook setup idempotently preserve or install the Beads chain for both hooks. Avoid plain copies over already installed hook entry points.
## Fix Focus Areas
- makefiles/dispatch.mk[303-308]
- scripts/hooks/pre-commit[1-7]
- scripts/hooks/pre-push[1-10]
- scripts/context/validate-beads-policy.sh[22-45]
ⓘ 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 392a451 |
Comment on lines
+5337
to
+5341
| [[package]] | ||
| name = "protoc-bin-vendored-macos-aarch_64" | ||
| version = "3.2.0" | ||
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "343d3bd7056eda839b03204e68deff7d1b13aba7af2b2fd16890697274262ee7" |
Contributor
There was a problem hiding this comment.
1. Conflicting package duplicated in lockfile 📘 Rule violation ✧ Quality
The added protoc-bin-vendored 3.2.0 records duplicate existing package identities, including a conflicting protoc-bin-vendored-macos-aarch_64 record with a different checksum and unrelated dependencies copied from prost-build. This lockfile corruption may cause Cargo to reject or fail to verify the lockfile, blocking locked builds, Clippy, tests, and CI.
Agent Prompt
## Issue description
`Cargo.lock` contains duplicated `protoc-bin-vendored` 3.2.0 package identities and a corrupted `protoc-bin-vendored-macos-aarch_64` record with a conflicting checksum and unrelated dependencies. This can cause Cargo to reject or fail to validate the committed lockfile.
## Issue Context
The added block repeats existing `protoc-bin-vendored` platform records, while the macOS ARM entry contains checksum and dependency data that do not belong to that platform-binary package. Regenerate the lockfile from the workspace manifests rather than manually retaining or editing the duplicated block, then verify the result with locked Cargo build, Clippy, and test commands.
## Fix Focus Areas
- Cargo.lock[5337-5415]
ⓘ 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 a5c3d9f |
![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 158 files
Confidence score: 2/5
- In
.github/workflows/ci.yml, the Windows matrix currently fails before compilation due to cloning into an already checked-out workspace and dropping required protoc/ONNX setup, which blocks test signal and Windows artifacts entirely — remove the extra clone step and restore the dependency install sequence. - In
.github/workflows/release.yml(also reflected indocs/operations/CHANGELOG.md), manual recovery can publish binaries from the wrong commit/tag and still appear successful even when a platform build fails, creating misleading releases with missing artifacts — consistently check outworkflow_dispatch.inputs.tagin all release jobs and fail/mark the release when any artifact is missing. - In
crates/mcb-server/src/mcp_server.rs, the new persistence.await?path can abort beforeroute_tool_call, so transient session/project save failures now block otherwise valid tool calls — make persistence non-blocking for routing (or degrade gracefully with logging/retry) to preserve tool availability. - In
scripts/dev-env-optimize.sh, global PID cleanup can terminate rust-analyzer/Serena processes from other repositories, causing cross-project disruption — scope process discovery/termination to the current repo before applying cleanup.
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=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:147">
P1: The new tool-install and sccache steps execute third-party action code from mutable tags, creating an avoidable CI supply-chain and reproducibility risk. Pin every new `taiki-e/install-action` and `mozilla-actions/sccache-action` use to a reviewed full commit SHA, matching the rest of the workflow.</violation>
<violation number="2" location=".github/workflows/ci.yml:334">
P0: Every Windows test/release matrix leg fails before compilation because this clones into the workspace already populated by `actions/checkout`; the step also no longer installs the protoc/ONNX dependencies named by the step. Replace these duplicated Git blocks with `./.github/actions/native-deps` (and rely on the existing recursive checkout).</violation>
<violation number="3" location=".github/workflows/ci.yml:362">
P2: Windows regressions outside library unit tests and the single startup smoke test are no longer exercised, even though this leg now has a 240-minute timeout intended for the full suite. Keep the same full-suite command across matrix platforms rather than reducing the required Windows gate.</violation>
</file>
<file name=".github/workflows/release.yml">
<violation number="1" location=".github/workflows/release.yml:22">
P0: A manual recovery for an existing tag can publish binaries built from the wrong commit. `workflow_dispatch.tag` is only used when naming/verifying the release; both checkout steps still check out the dispatch ref. Consider setting the checkout `ref` to the input tag for manual runs (and the pushed tag ref otherwise) in both `release-build` and `create-release`.</violation>
<violation number="2" location=".github/workflows/release.yml:137">
P2: A branch, SHA, or non-version tag passes this “tag exists” check, so the recovery path proceeds without identifying the promised existing release tag. Validate the exact `refs/tags/$INPUT_TAG` ref and the required `v*` format via the canonical Make interface.</violation>
<violation number="3" location=".github/workflows/release.yml:191">
P1: A failed platform build now produces an official, successful-looking release with missing binaries, and the summary still claims all three artifacts exist. This turns a build failure into a partially published release rather than preserving release integrity. Consider requiring all expected assets before publishing (the manual dispatch path can still be used to recover after the failed leg is fixed).</violation>
</file>
<file name="docs/MCP_TOOLS.md">
<violation number="1" location="docs/MCP_TOOLS.md:216">
P2: Clients are now told that missing provenance aborts server startup and that `model_id` is discovered from model environment variables, but the runtime actually boots with `model_id = "UNKNOWN"` and validates provenance only when a relevant tool is called. This makes startup/runbook expectations and the documented `MCB_MODEL_ID` remediation unreliable; the section should describe the implemented defaults and call-time validation, or the runtime behavior needs to be implemented as documented.</violation>
<violation number="2" location="docs/MCP_TOOLS.md:222">
P1: Calls with `repo_path` but no `repo_id` are rejected, contrary to this conditional requirement; the documented ID source is also incorrect. Please mark `repo_id` required and describe its root-commit-derived value.</violation>
<violation number="3" location="docs/MCP_TOOLS.md:237">
P2: Plain filesystem workspaces do not currently have the documented “ultimate happy path”: discovery only accepts a path that the Git `VcsProvider` can open, after which index/search/memory calls reject missing `repo_path` and `repo_id`. The listed non-Git detectors and always-resolved `worktree_id` are also not implemented. This should be documented as Git-only until those fallbacks exist, otherwise users in non-Git directories will expect calls to work and receive missing-provenance errors.</violation>
</file>
<file name="scripts/dev-env-optimize.sh">
<violation number="1" location="scripts/dev-env-optimize.sh:110">
P1: Running `make dev-env-optimize APPLY=Y` can terminate active rust-analyzer and Serena processes belonging to other repositories. The PID lists are global even though the script promises per-project cleanup and computes `PROJECT_ROOT`. Consider identifying each process's project/cwd and filtering to this repository before applying the keep count or sending signals.</violation>
<violation number="2" location="scripts/dev-env-optimize.sh:137">
P1: Once cargo matching works, `--apply` terminates every cargo command older than 30 minutes, including legitimate builds, while ignoring `KEEP_CARGO`. Verify state/activity and retain the configured count before sending `TERM`.</violation>
<violation number="3" location="scripts/dev-env-optimize.sh:208">
P2: Stale cargo checking never finds a process because each `ps` row starts with its PID, not `cargo`. Match the command field instead.</violation>
</file>
<file name="crates/mcb-server/src/mcp_server.rs">
<violation number="1" location="crates/mcb-server/src/mcp_server.rs:280">
P1: Tool calls are now blocked whenever automatic session or project persistence fails. The new `.await?` propagates the `McpError` produced by both create paths before `route_tool_call` runs, despite this helper’s contract and logs describing DB failures as non-fatal. Consider swallowing repository failures after logging them, while propagating only the intended input-validation errors.</violation>
<violation number="2" location="crates/mcb-server/src/mcp_server.rs:391">
P1: A failed auto-initialization permanently marks the session/project as initialized. Because the `DashSet` insertion happens before the newly propagated validation, clock, and repository errors, the first call fails but later calls skip initialization and run without the corresponding record. Consider committing the guard only after successful creation/existence checks, or removing/reserving it with explicit rollback on every failure path so retries remain consistent.</violation>
<violation number="3" location="crates/mcb-server/src/mcp_server.rs:394">
P2: Missing model provenance still creates sessions with `"unknown"` rather than failing as this validation intends. `ToolExecutionContext::resolve` always receives the nonblank `RuntimeDefaults.model_id = FALLBACK_UNKNOWN`, and the HTTP bridge now sends no model header, so this `ok_or_else` cannot distinguish an absent model from a real one. Consider removing the fallback at context discovery/resolution and validating the actual request/transport source before creating the session.</violation>
</file>
<file name="docs/operations/CHANGELOG.md">
<violation number="1" location="docs/operations/CHANGELOG.md:21">
P1: Manual recovery can publish binaries from the workflow branch under a different existing tag because `release-build` never checks out `workflow_dispatch.inputs.tag`. The release jobs should consistently check out and build the requested tag before this is documented as recovery.</violation>
</file>
<file name="Makefile">
<violation number="1" location="Makefile:43">
P1: Fresh environments cannot bootstrap this dependency reliably: parsing even `make help` starts an unguarded install through the already-missing `sccache` wrapper, then suppresses failure and leaves Rust targets broken. An explicit `mcb.sh`/setup phase should install and verify `sccache` before exporting `RUSTC_WRAPPER`, without swallowing errors.</violation>
</file>
<file name="docs/archive/k8s/legacy-manifests.bak/README.md">
<violation number="1" location="docs/archive/k8s/legacy-manifests.bak/README.md:3">
P1: Readers entering this directory can mistake superseded manifests for a supported deployment path because this README explicitly instructs them to deploy it, contradicting the parent archive notice. Add a prominent warning here that these artifacts are historical only and link to the current deployment documentation.</violation>
</file>
<file name="scripts/docs/validate.sh">
<violation number="1" location="scripts/docs/validate.sh:363">
P1: The docs validation gate still passes when `check_outdated.py` crashes or returns a failure because `|| true` suppresses every nonzero status. Let the command propagate its exit code so scanner failures remain visible to CI.</violation>
</file>
<file name=".github/actions/native-deps/action.yml">
<violation number="1" location=".github/actions/native-deps/action.yml:48">
P1: Windows CI extracts and loads an ONNX Runtime DLL without verifying the archive, leaving this executable dependency vulnerable to corrupted or replaced release content. Add a pinned SHA-256 and reject a mismatch before `Expand-Archive`, matching the protoc and Unix ONNX paths.</violation>
</file>
<file name="crates/mcb-infrastructure/src/services/highlight_sync_service.rs">
<violation number="1" location="crates/mcb-infrastructure/src/services/highlight_sync_service.rs:162">
P1: TSX input never reaches this arm: `LanguageId::from_name("tsx")` currently resolves to `TypeScript`, so JSX is highlighted with `LANGUAGE_TYPESCRIPT` instead of `LANGUAGE_TSX`. Correct the duplicate `tsx` alias ordering/mapping in `LanguageId::NAME_EQUIVALENTS` and add a TSX syntax test.</violation>
<violation number="2" location="crates/mcb-infrastructure/src/services/highlight_sync_service.rs:210">
P2: C input never reaches this arm: `LanguageId::from_name("c")` resolves to `Cpp`, so the service silently uses the C++ grammar. Correct the duplicate `c` alias mapping in `LanguageId::NAME_EQUIVALENTS` and cover the C grammar explicitly.</violation>
</file>
<file name="docs/testing/INTEGRATION_TESTS.md">
<violation number="1" location="docs/testing/INTEGRATION_TESTS.md:81">
P2: Starting Docker alone does not make service-gated tests run: the availability helpers require URLs in `config/tests.toml`, whose entries are all commented out. Document the required host mappings (for example Milvus on `29530`, Ollama on `21434`, Redis on `26379`, and Postgres on `25432`) before this test command.</violation>
<violation number="2" location="docs/testing/INTEGRATION_TESTS.md:96">
P1: The documented containerized test command currently fails because `test-runner` invokes the nonexistent `make test-rust` target. Either fix the Compose runner to use a supported test verb or avoid advertising this command until it works.</violation>
</file>
<file name="scripts/context/validate-beads-policy.sh">
<violation number="1" location="scripts/context/validate-beads-policy.sh:42">
P2: The gate can report a valid `prepare-commit-msg` hook even when agent trailers run without `BD_ALLOW_AGENT_COMMIT_TRAILERS=1`, because it checks only string presence and not guard/delegation structure. Validate the installed hook's exact guarded block or its behavior with the variable unset, `0`, and `1`.</violation>
<violation number="2" location="scripts/context/validate-beads-policy.sh:48">
P2: Legacy beads guidance added to active onboarding or user documentation is not detected because those paths are omitted from `scan_paths`. Include all active documentation locations while explicitly excluding archival content.</violation>
<violation number="3" location="scripts/context/validate-beads-policy.sh:58">
P1: Legacy instructions such as `Run bd sync manually` pass this gate because `manual` anywhere on the line exempts the entire match. Restrict exemptions to an explicit negation/deprecation of the matched command rather than unrelated keywords.</violation>
</file>
<file name="scripts/lib/mcb.sh">
<violation number="1" location="scripts/lib/mcb.sh:99">
P1: Production violations in the excluded directories are now invisible to all three gates, including executable `unwrap()` calls and unjustified `#[allow]` attributes. Narrow exclusions to known citation/string matches rather than exempting whole source trees.</violation>
<violation number="2" location="scripts/lib/mcb.sh:104">
P2: The claimed string/declaration exclusions both allow real banned calls through and still flag harmless literal text because they classify entire lines rather than matched tokens. A quote-aware or syntax-aware scan should remove literal/comment spans before searching executable code.</violation>
<violation number="3" location="scripts/lib/mcb.sh:113">
P2: Staging one validator file defeats its new path and doc-comment exclusions, so harmless cited patterns can block the commit. The first two grep invocations should also force a filename with `-H` before applying filename-aware filters.</violation>
<violation number="4" location="scripts/lib/mcb.sh:125">
P1: A commit containing one Rust file can fail the `#[allow]` gate even when the next line has the required justification. `grep -rnE` omits the filename for a single operand, so this loop misparses the line number; forcing filenames with `-H` keeps the parser valid.</violation>
</file>
<file name="scripts/hooks/pre-push">
<violation number="1" location="scripts/hooks/pre-push:8">
P1: Issue-graph synchronization is silently skipped whenever `bd` is missing from `PATH`, despite this being documented as a no-bypass hook. Delegating unconditionally lets command-not-found fail the push instead of reporting success without the Beads gate.</violation>
</file>
<file name="crates/mcb/src/loco_app.rs">
<violation number="1" location="crates/mcb/src/loco_app.rs:170">
P1: Stdio-only processes remain alive after the MCP client closes stdin or the stdio server fails, because this branch can finish only on Ctrl+C/SIGTERM while the stdio task is detached. Retaining the stdio task completion signal and selecting it alongside OS shutdown would let MCP child processes exit normally.</violation>
</file>
<file name="makefiles/dispatch.mk">
<violation number="1" location="makefiles/dispatch.mk:120">
P1: `make hook WHAT=pre-commit` can fail before unit tests run because `$(MCB_TEST_UNIT)` depends on shell variable `T`, but this hook branch never initializes it. Initializing/sanitizing `T` in this branch (as done in `DISPATCH_TEST`) keeps the hook gate runnable.</violation>
<violation number="2" location="makefiles/dispatch.mk:233">
P1: Installation persists the JWT signing secret in the systemd unit, which is copied with ordinary unit-file permissions, even though the canonical secret file is restricted to `0600`. This exposes a credential in configuration/backups and duplicates its source of truth. Consider keeping the secret only in a restricted environment file referenced with `EnvironmentFile=` (with the file containing `JWT_SECRET=...`).</violation>
</file>
<file name="docs/operations/DEPLOYMENT.md">
<violation number="1" location="docs/operations/DEPLOYMENT.md:14">
P2: The documented development command exits with Clap's missing-subcommand error rather than starting a server. `make dev WHAT=run` invokes bare `cargo run`, while the binary requires the `serve` subcommand; the Make dispatch should pass `serve` before this is presented as a working runtime mode.</violation>
</file>
<file name="AGENTS.md">
<violation number="1" location="AGENTS.md:159">
P2: Kubernetes beads cannot satisfy this completion gate because `dese` and the `dese/prod/control` order have no repository-defined meaning. Reference a canonical deployment-environment document or omit this environment-specific interlock until that source exists.</violation>
<violation number="2" location="AGENTS.md:362">
P2: The documented `contributor` repair cannot make this repository healthy: the same section and `scripts/context/validate-beads-policy.sh` require `beads.role` to equal `maintainer`. The `maintainer|contributor` notation is also unsafe to paste because the shell treats `|` as a pipe. This repository-specific repair should name only the accepted `git config beads.role maintainer` command.</violation>
</file>
<file name="crates/mcb-server/src/tools/validation.rs">
<violation number="1" location="crates/mcb-server/src/tools/validation.rs:136">
P2: Servers constructed through the public `McpServer::new(..., None)` path now reject every tool call with `execution_flow is required`. Since the constructor and runtime defaults still explicitly support an absent flow, preserving the `StdioOnly` fallback (or making the constructor parameter non-optional throughout) would avoid this runtime regression.</violation>
</file>
<file name=".config/nextest.toml">
<violation number="1" location=".config/nextest.toml:17">
P2: All `mcb-server` test binaries are now serialized, including hundreds of unit, e2e, and contract tests unrelated to `SharedTestContext`. Narrowing this override to the tests that initialize the shared context would retain nextest parallelism for the rest of the package.</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
| @@ -17,9 +17,11 @@ permissions: | |||
|
|
|||
There was a problem hiding this comment.
P0: Every Windows test/release matrix leg fails before compilation because this clones into the workspace already populated by actions/checkout; the step also no longer installs the protoc/ONNX dependencies named by the step. Replace these duplicated Git blocks with ./.github/actions/native-deps (and rely on the existing recursive checkout).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 334:
<comment>Every Windows test/release matrix leg fails before compilation because this clones into the workspace already populated by `actions/checkout`; the step also no longer installs the protoc/ONNX dependencies named by the step. Replace these duplicated Git blocks with `./.github/actions/native-deps` (and rely on the existing recursive checkout).</comment>
<file context>
@@ -330,69 +331,43 @@ jobs:
- Add-Content -Path $env:GITHUB_PATH -Value $ortLib
- Add-Content -Path $env:GITHUB_ENV -Value "ORT_DYLIB_PATH=$ortLib\onnxruntime.dll"
- Add-Content -Path $env:GITHUB_ENV -Value "PATH=$ortLib;$env:PATH"
+ git clone --depth 1 https://github.com/${{ github.repository }}.git .
+ git fetch --no-tags --prune --no-recurse-submodules --depth=1 origin +${{ github.sha }}:refs/remotes/origin/temp
+ git checkout ${{ github.sha }}
</file context>
| - "v*" | ||
| # Recovery path: manually (re)publish a GitHub Release for an existing tag without | ||
| # re-tagging — e.g. when a build leg failed or a release needs re-running. | ||
| workflow_dispatch: |
There was a problem hiding this comment.
P0: A manual recovery for an existing tag can publish binaries built from the wrong commit. workflow_dispatch.tag is only used when naming/verifying the release; both checkout steps still check out the dispatch ref. Consider setting the checkout ref to the input tag for manual runs (and the pushed tag ref otherwise) in both release-build and create-release.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/release.yml, line 22:
<comment>A manual recovery for an existing tag can publish binaries built from the wrong commit. `workflow_dispatch.tag` is only used when naming/verifying the release; both checkout steps still check out the dispatch ref. Consider setting the checkout `ref` to the input tag for manual runs (and the pushed tag ref otherwise) in both `release-build` and `create-release`.</comment>
<file context>
@@ -17,6 +17,14 @@ on: # yamllint disable-line rule:truthy
- "v*"
+ # Recovery path: manually (re)publish a GitHub Release for an existing tag without
+ # re-tagging — e.g. when a build leg failed or a release needs re-running.
+ workflow_dispatch:
+ inputs:
+ tag:
</file context>
| |-------|------|----------|-------------------|-------------------| | ||
| | `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 | — | |
There was a problem hiding this comment.
P1: Calls with repo_path but no repo_id are rejected, contrary to this conditional requirement; the documented ID source is also incorrect. Please mark repo_id required and describe its root-commit-derived value.
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 222:
<comment>Calls with `repo_path` but no `repo_id` are rejected, contrary to this conditional requirement; the documented ID source is also incorrect. Please mark `repo_id` required and describe its root-commit-derived value.</comment>
<file context>
@@ -213,21 +213,33 @@ Unified entity CRUD (vcs/plan/issue/org resources).
+|-------|------|----------|-------------------|-------------------|
+| `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 | — |
+| `worktree_id` | string | no | `git rev-parse --git-dir` → `git worktree list` → `.git` dir → `"main"` → **CWD path** | — (always resolved) |
</file context>
Suggested change
| | `repo_id` | string | if `repo_path` absent | Git remote `origin` URL hash; absent for plain filesystem workspaces | — | | |
| | `repo_id` | string | **yes** | Identifier derived from the repository root commit, or an explicit request metadata override | `Missing execution provenance for '<tool>': repo_id` | |
| kill_duplicate_rust_analyzers() { | ||
| printf "\n--- rust-analyzer duplicate cleanup -----------------------------------------\n" | ||
| local pids | ||
| pids=$(pgrep -a -f 'rust.analyzer|rust-analyzer' 2>/dev/null | grep -v 'grep' | awk '{print $1}' || true) |
There was a problem hiding this comment.
P1: Running make dev-env-optimize APPLY=Y can terminate active rust-analyzer and Serena processes belonging to other repositories. The PID lists are global even though the script promises per-project cleanup and computes PROJECT_ROOT. Consider identifying each process's project/cwd and filtering to this repository before applying the keep count or sending signals.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/dev-env-optimize.sh, line 110:
<comment>Running `make dev-env-optimize APPLY=Y` can terminate active rust-analyzer and Serena processes belonging to other repositories. The PID lists are global even though the script promises per-project cleanup and computes `PROJECT_ROOT`. Consider identifying each process's project/cwd and filtering to this repository before applying the keep count or sending signals.</comment>
<file context>
@@ -0,0 +1,288 @@
+kill_duplicate_rust_analyzers() {
+ printf "\n--- rust-analyzer duplicate cleanup -----------------------------------------\n"
+ local pids
+ pids=$(pgrep -a -f 'rust.analyzer|rust-analyzer' 2>/dev/null | grep -v 'grep' | awk '{print $1}' || true)
+
+ if [ -z "${pids}" ]; then
</file context>
| &self.auto_init_projects, | ||
| ) | ||
| .await; | ||
| .await?; |
There was a problem hiding this comment.
P1: Tool calls are now blocked whenever automatic session or project persistence fails. The new .await? propagates the McpError produced by both create paths before route_tool_call runs, despite this helper’s contract and logs describing DB failures as non-fatal. Consider swallowing repository failures after logging them, while propagating only the intended input-validation errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/mcb-server/src/mcp_server.rs, line 280:
<comment>Tool calls are now blocked whenever automatic session or project persistence fails. The new `.await?` propagates the `McpError` produced by both create paths before `route_tool_call` runs, despite this helper’s contract and logs describing DB failures as non-fatal. Consider swallowing repository failures after logging them, while propagating only the intended input-validation errors.</comment>
<file context>
@@ -277,7 +277,7 @@ tools:
&self.auto_init_projects,
)
- .await;
+ .await?;
route_tool_call(request, &self.handlers, execution_context).await
</file context>
| serial-shared-context = { max-threads = 1 } | ||
|
|
||
| [[profile.default.overrides]] | ||
| filter = 'package(mcb-server) and kind(test)' |
There was a problem hiding this comment.
P2: All mcb-server test binaries are now serialized, including hundreds of unit, e2e, and contract tests unrelated to SharedTestContext. Narrowing this override to the tests that initialize the shared context would retain nextest parallelism for the rest of the package.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .config/nextest.toml, line 17:
<comment>All `mcb-server` test binaries are now serialized, including hundreds of unit, e2e, and contract tests unrelated to `SharedTestContext`. Narrowing this override to the tests that initialize the shared context would retain nextest parallelism for the rest of the package.</comment>
<file context>
@@ -0,0 +1,23 @@
+serial-shared-context = { max-threads = 1 }
+
+[[profile.default.overrides]]
+filter = 'package(mcb-server) and kind(test)'
+test-group = 'serial-shared-context'
+# Defense-in-depth: the CI warm-up step (`make test SCOPE=warmup`) populates the
</file context>
| - Publication interlock: when Git is explicitly authorized for the lane, the coordinator stages only the bead's | ||
| scoped paths, commits with no agent attribution, pushes, records commit/push/CI evidence in `bd`, and keeps | ||
| the bead open until remote checks finish. | ||
| - GitOps interlock: for Kubernetes/GitOps changes, completion requires dese-first validation from ArgoCD/read-only |
There was a problem hiding this comment.
P2: Kubernetes beads cannot satisfy this completion gate because dese and the dese/prod/control order have no repository-defined meaning. Reference a canonical deployment-environment document or omit this environment-specific interlock until that source exists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At AGENTS.md, line 159:
<comment>Kubernetes beads cannot satisfy this completion gate because `dese` and the `dese/prod/control` order have no repository-defined meaning. Reference a canonical deployment-environment document or omit this environment-specific interlock until that source exists.</comment>
<file context>
@@ -75,26 +84,103 @@ types at the source; depend on declared contracts, not loosely-typed escape hatc
+- Publication interlock: when Git is explicitly authorized for the lane, the coordinator stages only the bead's
+ scoped paths, commits with no agent attribution, pushes, records commit/push/CI evidence in `bd`, and keeps
+ the bead open until remote checks finish.
+- GitOps interlock: for Kubernetes/GitOps changes, completion requires dese-first validation from ArgoCD/read-only
+ cluster evidence, then prod and control sync/soak in the documented dependency order after dese is green. The
+ bead cannot close while dese/prod/control validation is missing, red, skipped without justification, or only
</file context>
|
|
||
| prepare_commit_msg="$(git -C "$MCB_ROOT" rev-parse --git-path hooks/prepare-commit-msg)" | ||
| [ -f "$prepare_commit_msg" ] || fail "prepare-commit-msg hook is missing" | ||
| rg -q 'BD_ALLOW_AGENT_COMMIT_TRAILERS' "$prepare_commit_msg" \ |
There was a problem hiding this comment.
P2: The gate can report a valid prepare-commit-msg hook even when agent trailers run without BD_ALLOW_AGENT_COMMIT_TRAILERS=1, because it checks only string presence and not guard/delegation structure. Validate the installed hook's exact guarded block or its behavior with the variable unset, 0, and 1.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/context/validate-beads-policy.sh, line 42:
<comment>The gate can report a valid `prepare-commit-msg` hook even when agent trailers run without `BD_ALLOW_AGENT_COMMIT_TRAILERS=1`, because it checks only string presence and not guard/delegation structure. Validate the installed hook's exact guarded block or its behavior with the variable unset, `0`, and `1`.</comment>
<file context>
@@ -0,0 +1,63 @@
+
+prepare_commit_msg="$(git -C "$MCB_ROOT" rev-parse --git-path hooks/prepare-commit-msg)"
+[ -f "$prepare_commit_msg" ] || fail "prepare-commit-msg hook is missing"
+rg -q 'BD_ALLOW_AGENT_COMMIT_TRAILERS' "$prepare_commit_msg" \
+ || fail "prepare-commit-msg must guard agent trailers with BD_ALLOW_AGENT_COMMIT_TRAILERS"
+rg -q 'bd hooks run prepare-commit-msg' "$prepare_commit_msg" \
</file context>
| || fail "prepare-commit-msg must still delegate to bd when explicitly enabled" | ||
|
|
||
| scan_paths=() | ||
| for path in AGENTS.md makefiles docs/modules docs/developer .beads/config.yaml; do |
There was a problem hiding this comment.
P2: Legacy beads guidance added to active onboarding or user documentation is not detected because those paths are omitted from scan_paths. Include all active documentation locations while explicitly excluding archival content.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/context/validate-beads-policy.sh, line 48:
<comment>Legacy beads guidance added to active onboarding or user documentation is not detected because those paths are omitted from `scan_paths`. Include all active documentation locations while explicitly excluding archival content.</comment>
<file context>
@@ -0,0 +1,63 @@
+ || fail "prepare-commit-msg must still delegate to bd when explicitly enabled"
+
+scan_paths=()
+for path in AGENTS.md makefiles docs/modules docs/developer .beads/config.yaml; do
+ [ -e "$path" ] && scan_paths+=("$path")
+done
</file context>
| @@ -93,15 +93,45 @@ mcb_guard() { | |||
| src=$(find "$MCB_ROOT/crates" -name '*.rs' -not -path '*/tests/*' -not -path '*/benches/*' -not -path '*/target/*' 2>/dev/null || true) | |||
There was a problem hiding this comment.
P2: The claimed string/declaration exclusions both allow real banned calls through and still flag harmless literal text because they classify entire lines rather than matched tokens. A quote-aware or syntax-aware scan should remove literal/comment spans before searching executable code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/lib/mcb.sh, line 104:
<comment>The claimed string/declaration exclusions both allow real banned calls through and still flag harmless literal text because they classify entire lines rather than matched tokens. A quote-aware or syntax-aware scan should remove literal/comment spans before searching executable code.</comment>
<file context>
@@ -93,15 +93,45 @@ mcb_guard() {
+ local guard_excludes='mcb-validate/src/|mcb-utils/src/constants/validate/'
+
# 1. unwrap/expect/panic/todo/unimplemented in non-test .rs
+ # Exclude: doc comments (///, //!), const/static declarations, string literals.
hits=$(grep -rnE '\b(unwrap|expect)\(|\bpanic!|\btodo!|\bunimplemented!' $src 2>/dev/null \
- | grep -vE '//.*(unwrap|expect)|#\[cfg\(test\)\]' || true)
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.