fix(deps): clear security audit + clippy result_large_err failures - #163
fix(deps): clear security audit + clippy result_large_err failures#163marlon-costa-dc wants to merge 2 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Running ultrareview automatically — This PR removes 1792 files including CI workflows, dev configs, and infrastructure files while updating security-sensitive dependencies (git2, auth types) and error handling across the codebase — the blast radius of a missed bug could break CI, development setup, and production authentication.. I'll post findings when complete. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR introduces local authentication errors, standardizes boxed MCP and bootstrap error propagation, adjusts Git metadata lookup handling, updates selected dependencies, and changes repository hook and binary-resolution behavior. ChangesServer error handling
VCS result handling
Dependencies and tooling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoClear RustSec advisories and clippy result_large_err via deps + error boundaries
AI Description
Diagram
High-Level Assessment
Files changed (47)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
36 rules 1. git2 errors are swallowed
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| edition = "2024" | ||
|
|
||
| [workspace.lints.rust] | ||
| unsafe_code = "deny" |
There was a problem hiding this comment.
1. unsafe_code lint removed 📘 Rule violation ⛨ Security
The workspace lint unsafe_code = "deny" was removed, so unsafe blocks can slip into non-test Rust code without a hard failure. This violates the requirement that unsafe usage is forbidden and the deny lint must remain in place.
Agent Prompt
## Issue description
`unsafe_code = "deny"` was removed from the workspace lints, weakening the guarantee that no `unsafe` code can be introduced.
## Issue Context
Compliance requires zero `unsafe` usage in Rust production code and explicitly requires keeping the project-level `unsafe_code = "deny"` lint in place.
## Fix Focus Areas
- Cargo.toml[23-30]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| unsafe_code = "deny" | ||
| missing_docs = "warn" | ||
| non_ascii_idents = "deny" | ||
| dead_code = "deny" |
There was a problem hiding this comment.
2. dead_code lint removed 📘 Rule violation ⚙ Maintainability
The workspace lint dead_code = "deny" was removed, allowing new unused items to compile without a hard failure. This violates the requirement that dead code must remain disallowed.
Agent Prompt
## Issue description
`dead_code = "deny"` was removed from workspace lints, which permits introducing unused functions/structs/constants without failing the build.
## Issue Context
Compliance requires the `dead_code` lint to remain set to `deny`.
## Fix Focus Areas
- Cargo.toml[23-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| non_ascii_idents = "deny" | ||
| dead_code = "deny" | ||
| unused_variables = "deny" | ||
| unused_imports = "deny" |
There was a problem hiding this comment.
3. unused_imports lint removed 📘 Rule violation ⚙ Maintainability
The workspace lint unused_imports = "deny" was removed, allowing unused imports to compile without a hard failure. This violates the requirement that unused imports remain disallowed.
Agent Prompt
## Issue description
`unused_imports = "deny"` was removed from workspace lints, weakening enforcement against unused `use` statements.
## Issue Context
Compliance requires the unused-imports lint remain set to `deny`.
## Fix Focus Areas
- Cargo.toml[28-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @@ -1 +0,0 @@ | |||
| Subproject commit 11977efbef83e05e469e04110a331e62f09a0f92 | |||
There was a problem hiding this comment.
4. third-party/ submodules removed 📘 Rule violation § Compliance
This PR modifies content under third-party/ by deleting multiple vendored submodule entries, without an explicit request authorizing third-party changes. This violates the rule restricting edits under third-party/.
Agent Prompt
## Issue description
Files under `third-party/` were modified (deleted), but the PR description does not explicitly request or authorize third-party changes.
## Issue Context
Compliance requires avoiding modifications under `third-party/` unless explicitly requested.
## Fix Focus Areas
- third-party/edgevec[1-1]
- third-party/loco[1-1]
- third-party/milvus-sdk-rust[1-1]
- third-party/rust-code-analysis[1-1]
- third-party/sea-orm-pro[1-1]
- third-party/sea-orm[1-1]
- third-party/sea-query[1-1]
- third-party/sea-schema[1-1]
- third-party/sea-streamer[1-1]
- third-party/seaography[1-1]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # Hooks inherit GIT_DIR/GIT_WORK_TREE from the invoking git process; leaked | ||
| # vars redirect fixture commits into this repository (they must hit temp repos). | ||
| unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY \ | ||
| GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_COMMON_DIR || true |
There was a problem hiding this comment.
5. Late git env cleanup 🐞 Bug ≡ Correctness
scripts/hooks/pre-commit unsets GIT_DIR/GIT_WORK_TREE only after running `git rev-parse --show-toplevel`, so leaked Git env vars can still influence the initial repository resolution and cd into the wrong worktree/repo.
Agent Prompt
## Issue description
The pre-commit hook runs `git rev-parse --show-toplevel` before unsetting leaked `GIT_*` variables. If those variables are polluted, the very first `git` call can resolve against the wrong repo/worktree, making the subsequent cleanup ineffective.
## Issue Context
The newly-added `unset GIT_DIR GIT_WORK_TREE ...` block is intended to prevent fixture/test Git environment leakage from redirecting operations.
## Fix Focus Areas
- scripts/hooks/pre-commit[6-12]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| #!/usr/bin/env bash | ||
| # scripts/lib/mcb.sh — canonical tooling monopoly for MCB. | ||
| # ONE source for cross-cutting concerns: exit codes, logging, colors, the | ||
| # APPLY=Y mutation gate, SSOT readers (version, binary, audit ignores), the | ||
| # banned-pattern guard, and the agent bash-guard. No script, makefile, hook, or | ||
| # CI job calls cargo/git directly — everything flows through here. | ||
| # | ||
| # Use as a library: source scripts/lib/mcb.sh ; mcb_require_cmd cargo | ||
| # Use as a dispatcher: bash scripts/lib/mcb.sh <command> [args...] | ||
| # | ||
| # Note: strict mode (set -euo pipefail) is enabled ONLY in the direct-execution | ||
| # dispatcher at the bottom, never at source time — sourcing must not flip the | ||
| # caller's shell options (make sets its own -euo pipefail; docs scripts source us). | ||
| [ -n "${_MCB_SH_LOADED:-}" ] && return 0 | ||
| _MCB_SH_LOADED=1 | ||
|
|
There was a problem hiding this comment.
6. Deleted tooling still referenced 🐞 Bug ☼ Reliability
The pre-commit hook still calls bash scripts/lib/mcb.sh guard --staged and make check ..., but this PR deletes scripts/lib/mcb.sh and Makefile (and also removes Cargo.toml), so the hook and common workflows will fail immediately.
Agent Prompt
## Issue description
`scripts/hooks/pre-commit` depends on repo tooling (`scripts/lib/mcb.sh` and `make check ...`). This PR deletes that tooling (and the Rust workspace entrypoints), which makes the hook and developer/CI workflows fail.
## Issue Context
The remaining hook invokes the deleted script and Make targets.
## Fix Focus Areas
- scripts/hooks/pre-commit[12-14]
- scripts/lib/mcb.sh[1-136]
- Makefile[1-20]
- Cargo.toml[1-30]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 1: Update the README top-level heading from the placeholder “Test Repo”
to the project’s existing canonical name, preserving the heading format and
leaving the rest of the documentation unchanged.
In `@scripts/hooks/pre-commit`:
- Around line 8-11: Move the inherited Git-variable cleanup block before the
`git rev-parse --show-toplevel` invocation in the pre-commit hook, ensuring
`GIT_DIR`, `GIT_WORK_TREE`, `GIT_INDEX_FILE`, `GIT_OBJECT_DIRECTORY`,
`GIT_ALTERNATE_OBJECT_DIRECTORIES`, and `GIT_COMMON_DIR` are unset before
resolving the repository root.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: cubic · AI code reviewer
| ## License | ||
|
|
||
| [MIT](./LICENSE) — Open source, free for commercial and personal use. | ||
| # Test Repo |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the canonical project name.
# Test Repo is a placeholder and misidentifies the project for users, documentation tooling, and repository previews. Restore the existing project title.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 1, Update the README top-level heading from the
placeholder “Test Repo” to the project’s existing canonical name, preserving the
heading format and leaving the rest of the documentation unchanged.
| # Hooks inherit GIT_DIR/GIT_WORK_TREE from the invoking git process; leaked | ||
| # vars redirect fixture commits into this repository (they must hit temp repos). | ||
| unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY \ | ||
| GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_COMMON_DIR || true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
git -C "$tmp" init -q
GIT_DIR="$tmp/.git" GIT_WORK_TREE="$tmp" git rev-parse --show-toplevelRepository: marlonsc/mcb
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files named pre-commit:"
fd -a 'pre-commit' . | sed 's#^\./##'
echo
if [ -f scripts/hooks/pre-commit ]; then
echo "scripts/hooks/pre-commit with line numbers around first 30 lines:"
sed -n '1,40p' scripts/hooks/pre-commit | nl -ba
fi
echo
echo "Git variable occurrences in scripts/hooks/pre-commit:"
rg -n '(GIT_|git rev-parse|--show-toplevel|cd |unset)' scripts/hooks/pre-commit || trueRepository: marlonsc/mcb
Length of output: 326
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "scripts/hooks/pre-commit contents:"
awk '{ printf "%6d\t%s\n", NR, $0 }' scripts/hooks/pre-commit
echo
echo "Git variable occurrences:"
grep -nE '(GIT_|git rev-parse|--show-toplevel|cd |unset)' scripts/hooks/pre-commit || trueRepository: marlonsc/mcb
Length of output: 1433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
git -C "$tmp" init -q
tmp_prefix="$(cd "$tmp" && git rev-parse --show-cdup)"
leaked_prefix="${tmp}
${tmp}
"
echo "tmp_prefix=$tmp_prefix"
echo "has_leaked_prefix_match=$([[ $leaked_prefix == *"${tmp_prefix}"* ]] && echo yes || echo no)"
echo
echo "direct prefix check:"
python3 - <<'PY'
import tempfile, subprocess, os, textwrap
with tempfile.TemporaryDirectory() as tmp:
subprocess.check_call(["git", "-C", tmp, "init", "-q"], stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
prefix=subprocess.check_output(["git", "-C", tmp, "rev-parse", "--show-cdup"], text=True).rstrip("\n")
leaked="{}\n{}\n".format(tmp, tmp)
print("prefix={}".format(repr(prefix)))
print("contains_prefix={}".format(leaked.endswith(prefix)))
PYRepository: marlonsc/mcb
Length of output: 243
🌐 Web query:
Git GIT_WORK_TREE documentation git rev-parse --show-toplevel GIT_WORK_TREE
💡 Result:
The GIT_WORK_TREE environment variable and the git rev-parse --show-toplevel command are both used to interact with or query the root of a Git working tree [1][2][3]. GIT_WORK_TREE The GIT_WORK_TREE environment variable defines the root directory of the working tree for a repository [1][4]. It is particularly useful when you need to specify a working tree location that is different from the default (which is either the current directory or the directory containing the.git folder) [1][5]. Setting this variable (or using the --work-tree option) informs Git of the working tree's location, which is necessary if you are not currently inside that directory [1]. It can be set as an absolute path or a path relative to the current working directory [1][6]. git rev-parse --show-toplevel The git rev-parse --show-toplevel command outputs the absolute path of the top-level directory of the current working tree [2][7][3]. If this command is run outside of a valid Git working tree, it will report an error [2][8]. When GIT_WORK_TREE is set, git rev-parse --show-toplevel will correctly identify and report that specified path as the top level, provided the Git environment is correctly configured [9]. Relationship When GIT_WORK_TREE is defined, it explicitly overrides default repository discovery mechanisms (such as searching for a.git subdirectory) [1][5]. Consequently, git rev-parse --show-toplevel relies on this configuration to determine the root path [9]. Together, these tools allow for robust scripting where Git commands can be executed from various directories while consistently referencing the correct repository root [2][10][3].
Citations:
- 1: https://git-scm.com/docs/git
- 2: https://git-scm.com/docs/git-rev-parse
- 3: https://www.git-tower.com/learn/git/faq/git-rev-parse
- 4: https://git-scm.com/book/en/v2/Git-Internals-Environment-Variables
- 5: https://github.com/git/git/blob/master/Documentation/git.adoc
- 6: https://git.github.io/htmldocs/git.html
- 7: https://adamj.eu/tech/2023/08/21/git-output-root-directory/
- 8: https://git.github.io/htmldocs/git-rev-parse.html
- 9: https://code.googlesource.com/git/+/da4827056acd8acac882fdfc93fd5b70472927ea/t/t1501-work-tree.sh
- 10: https://opensource.com/article/22/4/git-repo-rev-parse
Sanitize inherited Git variables before resolving the repository root.
Line 7 invokes git rev-parse --show-toplevel before lines 10-11 unset the inherited variables. If those variables point to another repository, the hook can cd into that repository instead of the repository containing the pre-commit hook. Move the unset block before line 7.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/hooks/pre-commit` around lines 8 - 11, Move the inherited
Git-variable cleanup block before the `git rev-parse --show-toplevel` invocation
in the pre-commit hook, ensuring `GIT_DIR`, `GIT_WORK_TREE`, `GIT_INDEX_FILE`,
`GIT_OBJECT_DIRECTORY`, `GIT_ALTERNATE_OBJECT_DIRECTORIES`, and `GIT_COMMON_DIR`
are unset before resolving the repository root.
![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 12m 25s
3 issues found across 1792 files
Confidence score: 3/5
README.mdappears to have been replaced by a test fixture heading, which would remove core onboarding and operational docs (install/config/architecture/contributing) for users and contributors; this is a concrete usability regression if merged as-is — restore the full public README content before merge.- In
scripts/hooks/pre-commit, environment handling around Git context looks fragile: resolvinggit rev-parse --show-toplevelbefore sanitizingGIT_DIR/GIT_WORK_TREEcan target the wrong repository, and clearingGIT_INDEX_FILEbeforeguard --stagedcan mis-handle alternate-index commits; together this can cause incorrect hook behavior and inconsistent commit validation — move Git env sanitization earlier and preserveGIT_INDEX_FILEthrough staged 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="scripts/hooks/pre-commit">
<violation number="1" location="scripts/hooks/pre-commit:10">
P2: Commits made through an alternate index can bypass (or be incorrectly rejected by) the staged guard because this clears `GIT_INDEX_FILE` before `guard --staged` runs. The hook should retain that variable through the staged guard, then clear it before the lint/validation gates that create fixture repositories.</violation>
<violation number="2" location="scripts/hooks/pre-commit:10">
P1: The first `git` resolution can still run with leaked `GIT_*` environment variables, so `git rev-parse --show-toplevel` may resolve the wrong repository before cleanup happens. Moving the `GIT_DIR`/`GIT_WORK_TREE` sanitization ahead of any `git` invocation would make this guard effective.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:1">
P1: The repository's public README is reduced to a test-fixture heading, so users and contributors no longer have installation, configuration, tool, architecture, or contribution documentation. This looks like the fixture-write corruption described in the PR rather than an intended dependency/security fix; restore the project README content.</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.
Fix all with cubic | Re-trigger cubic
| ## License | ||
|
|
||
| [MIT](./LICENSE) — Open source, free for commercial and personal use. | ||
| # Test Repo |
There was a problem hiding this comment.
P1: The repository's public README is reduced to a test-fixture heading, so users and contributors no longer have installation, configuration, tool, architecture, or contribution documentation. This looks like the fixture-write corruption described in the PR rather than an intended dependency/security fix; restore the project README content.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 1:
<comment>The repository's public README is reduced to a test-fixture heading, so users and contributors no longer have installation, configuration, tool, architecture, or contribution documentation. This looks like the fixture-write corruption described in the PR rather than an intended dependency/security fix; restore the project README content.</comment>
<file context>
@@ -1,181 +1 @@
-## License
-
-[MIT](./LICENSE) — Open source, free for commercial and personal use.
+# Test Repo
</file context>
| @@ -5,6 +5,10 @@ | |||
| # the retroactive baseline; `make guard` (full tree) is the CI/manual scan. | |||
| set -euo pipefail | |||
| cd "$(git rev-parse --show-toplevel)" | |||
| # Hooks inherit GIT_DIR/GIT_WORK_TREE from the invoking git process; leaked | |||
| # vars redirect fixture commits into this repository (they must hit temp repos). | |||
| unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY \ | |||
There was a problem hiding this comment.
P1: The first git resolution can still run with leaked GIT_* environment variables, so git rev-parse --show-toplevel may resolve the wrong repository before cleanup happens. Moving the GIT_DIR/GIT_WORK_TREE sanitization ahead of any git invocation would make this guard effective.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/pre-commit, line 10:
<comment>The first `git` resolution can still run with leaked `GIT_*` environment variables, so `git rev-parse --show-toplevel` may resolve the wrong repository before cleanup happens. Moving the `GIT_DIR`/`GIT_WORK_TREE` sanitization ahead of any `git` invocation would make this guard effective.</comment>
<file context>
@@ -5,6 +5,10 @@
cd "$(git rev-parse --show-toplevel)"
+# Hooks inherit GIT_DIR/GIT_WORK_TREE from the invoking git process; leaked
+# vars redirect fixture commits into this repository (they must hit temp repos).
+unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY \
+ GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_COMMON_DIR || true
echo "→ guard (staged)…"; bash scripts/lib/mcb.sh guard --staged
</file context>
| @@ -5,6 +5,10 @@ | |||
| # the retroactive baseline; `make guard` (full tree) is the CI/manual scan. | |||
| set -euo pipefail | |||
| cd "$(git rev-parse --show-toplevel)" | |||
| # Hooks inherit GIT_DIR/GIT_WORK_TREE from the invoking git process; leaked | |||
| # vars redirect fixture commits into this repository (they must hit temp repos). | |||
| unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY \ | |||
There was a problem hiding this comment.
P2: Commits made through an alternate index can bypass (or be incorrectly rejected by) the staged guard because this clears GIT_INDEX_FILE before guard --staged runs. The hook should retain that variable through the staged guard, then clear it before the lint/validation gates that create fixture repositories.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/hooks/pre-commit, line 10:
<comment>Commits made through an alternate index can bypass (or be incorrectly rejected by) the staged guard because this clears `GIT_INDEX_FILE` before `guard --staged` runs. The hook should retain that variable through the staged guard, then clear it before the lint/validation gates that create fixture repositories.</comment>
<file context>
@@ -5,6 +5,10 @@
cd "$(git rev-parse --show-toplevel)"
+# Hooks inherit GIT_DIR/GIT_WORK_TREE from the invoking git process; leaked
+# vars redirect fixture commits into this repository (they must hit temp repos).
+unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE GIT_OBJECT_DIRECTORY \
+ GIT_ALTERNATE_OBJECT_DIRECTORIES GIT_COMMON_DIR || true
echo "→ guard (staged)…"; bash scripts/lib/mcb.sh guard --staged
</file context>
|
Code review by qodo was updated up to the latest commit a28d2b6 |
| [workspace] | ||
| resolver = "2" | ||
| members = [ | ||
| "crates/mcb", | ||
| "crates/mcb-utils", | ||
| "crates/mcb-domain", | ||
| "crates/mcb-infrastructure", | ||
| "crates/mcb-providers", | ||
| "crates/mcb-server", | ||
| "crates/mcb-validate", | ||
| ] | ||
| exclude = ["third-party"] | ||
|
|
||
| [workspace.package] | ||
| repository = "https://github.com/marlonsc/mcb" | ||
| authors = ["Marlon Costa <marlonsc@gmail.com>"] | ||
| homepage = "https://github.com/marlonsc/mcb" | ||
| license = "MIT" | ||
| version = "0.3.1" | ||
| rust-version = "1.92" | ||
| edition = "2024" | ||
|
|
There was a problem hiding this comment.
1. Cargo workspace deleted 🐞 Bug ≡ Correctness
This PR deletes the root Cargo.toml, removing the workspace/package definition, so cargo commands at the repository root can no longer build or test anything. It also deletes the CI workflow file, removing the automated pipeline that previously enforced build/test/lint gates.
Agent Prompt
## Issue description
The PR removes the repository's Rust build entrypoints (root `Cargo.toml`, `Cargo.lock`) and removes the CI workflow file. After this change, the repo root is no longer a valid Cargo workspace, and CI gating is effectively removed.
## Issue Context
This looks inconsistent with keeping developer workflows like the pre-commit gate that still expects project build/lint/validate tooling.
## Fix Focus Areas
- Cargo.toml[1-30]
- Cargo.lock[1-20]
- .github/workflows/ci.yml[13-83]
- scripts/hooks/pre-commit[1-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit d218ccd |
- crossbeam-epoch 0.9.18 -> 0.9.20 (RUSTSEC-2026-0204) - serial_test 3.4 -> 4.0.0: drops scc entirely, removing RUSTSEC-2026-0205 without an msrv bump (4.0.1 requires rustc 1.93.1; workspace validates under 1.92 and CI stable alike) - git2 0.20 -> 0.21 (RUSTSEC-2026-0183/0184); getters now return Result<Option<&str>>, call sites updated (git.rs, submodule.rs) - quick-xml workspace pin 0.39 -> 0.41 for first-party crates - accept RUSTSEC-2026-0194/0195 in the audit ignore SSOT with evidence: vendored opendal pins quick-xml <0.41 and its XML services are disabled in this workspace, so the DoS paths are unreachable until the next opendal bump - box large error variants (CallToolResult, loco_rs::Error) across mcb-server handlers and mcb initializers (result_large_err) - auth: typed AuthError domain error + map_auth_error boundary mapping (replaces loco_rs::Result in mcb-server auth, ported from the v0.4.0 feature line) - mcb_bin: prefer workspace-built binary over PATH (mise shim broke validate in fresh worktrees) - contract snapshots refreshed for serde field ordering Validated: cargo check --workspace --all-targets 0 errors; make check WHAT=lint and WHAT=audit green; contract suite 27/27; nextest mcb-server+mcb-providers+mcb 756/756 on this content.
git exports GIT_DIR/GIT_WORK_TREE to hook processes; the gate's test fixtures then commit into this repository instead of their temp repos (observed as stray 'Initial commit' commits, mass file deletions staged from fixture cleanups, and README gutted to a test stub). Sanitize the git-hook environment before running the canonical gates.
d218ccd to
70330fc
Compare
coderabbitai
Bot
added
Other
Possible security concern
Refactoring
Review effort 2/5
labels
| // INTENTIONAL: Best-effort default branch detection; falls back to None | ||
| .ok() | ||
| .and_then(|head| head.shorthand().map(String::from)) | ||
| .and_then(|head| head.shorthand().ok().map(String::from)) |
There was a problem hiding this comment.
1. Git2 errors are swallowed 📘 Rule violation ☼ Reliability
GitProvider::get_default_branch() and GitProvider::get_remote_url() now call head.shorthand().ok() and remote.url().ok(), respectively, which discards the underlying git2::Error and can mask real failures behind None/generic behavior (e.g., a misleading “no HEAD” outcome). This violates the requirement (PR Compliance ID 1208751) to not silently swallow errors in production paths without propagation, explicit handling, or logging.
Agent Prompt
## Issue description
`GitProvider::get_default_branch()` and `GitProvider::get_remote_url()` currently use `.ok()` on fallible `git2` getters (`head.shorthand()` and `remote.url()`), which discards the underlying `git2::Error` and can mask non-benign failures (including replacing errors with a generic “no HEAD”/`None` outcome) with no logging or propagation.
## Issue Context
This behavior was introduced during the `git2` upgrade where previously-infallible getters became `Result<Option<&str>>`. Compliance (PR Compliance ID 1208751) requires that errors are not silently swallowed in production paths; they must be propagated, explicitly handled, or at least logged with sufficient context. If remote URL detection is intended to be best-effort, the failure should still be logged with context, or the API should be adjusted to return a `Result<Option<String>, Error>`.
## Fix Focus Areas
- crates/mcb-providers/src/vcs/git.rs[60-70]
- crates/mcb-providers/src/vcs/git.rs[72-77]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| [ -x "$MCB_ROOT/target/debug/mcb" ] && { echo "$MCB_ROOT/target/debug/mcb"; return 0; } | ||
| [ -x "$MCB_ROOT/target/release/mcb" ] && { echo "$MCB_ROOT/target/release/mcb"; return 0; } | ||
| command -v mcb 2>/dev/null && return 0 | ||
| echo "cargo run --package mcb --" | ||
| } |
There was a problem hiding this comment.
2. Empty mcb_bin output 🐞 Bug ≡ Correctness
mcb_bin returns success when mcb is on PATH but prints nothing, so callers using cmd="$(mcb_bin)" end up executing an empty command string and failing (e.g., mcb_validate). This breaks validation tooling in environments where mcb is available only via PATH/shims.
Agent Prompt
## Issue description
`scripts/lib/mcb.sh:mcb_bin` uses `command -v mcb 2>/dev/null && return 0` which exits successfully without emitting the resolved path. Callers (notably `mcb_validate`) capture `mcb_bin` output into a variable and execute it, so an empty output results in trying to run `validate` as a standalone command.
## Issue Context
`mcb_validate` does `cmd="$(mcb_bin)"` and then runs `$cmd validate ...`, which requires `mcb_bin` to always print an executable command string when it returns success.
## Fix Focus Areas
- scripts/lib/mcb.sh[54-81]
## Suggested change
Replace the PATH branch with something that prints the resolved command, e.g.:
```bash
local path
path="$(command -v mcb 2>/dev/null)" && { echo "$path"; return 0; }
```
This preserves the new precedence (debug/release first) while making the PATH fallback usable.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| let Ok(Some(url)) = submodule.url().map(|u| u.map(str::to_owned)) else { | ||
| mcb_domain::warn!( | ||
| "submodule", | ||
| "Orphaned submodule (no URL in .gitmodules), skipping", |
There was a problem hiding this comment.
3. Submodule url errors hidden 🐞 Bug ◔ Observability
build_submodule_info treats both Ok(None) and Err(_) from submodule.url() as an “orphaned submodule (no URL)” and skips the submodule. This misreports URL-read/parsing failures (e.g., invalid UTF-8) and makes real configuration problems harder to diagnose while producing incomplete submodule traversal.
Agent Prompt
## Issue description
The new pattern match collapses `submodule.url()` error cases into the same branch as `Ok(None)`, logging an “orphaned submodule” warning and skipping. This hides the underlying error reason.
## Issue Context
`submodule.url()` now returns a `Result<Option<_>, _>` and can fail. Error cases should not be logged as “no URL in .gitmodules”.
## Fix Focus Areas
- crates/mcb-providers/src/vcs/submodule.rs[186-210]
## Suggested change
Use an explicit match to separate cases:
- `Ok(Some(url))` -> proceed
- `Ok(None)` -> log orphaned/no-url
- `Err(e)` -> log a different message including `e` (and consider whether to skip or propagate depending on desired behavior)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 70330fc |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/mcb-providers/src/vcs/git.rs (1)
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression tests for the new failure paths.
Cover failed default-branch resolution, missing/blank
originremote metadata, and tree entries with names that cannot be decoded. These paths intentionally differ from earlier error-propagation behavior and need focused coverage to guard future regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mcb-providers/src/vcs/git.rs` around lines 60 - 66, Add focused regression tests for get_default_branch covering failed HEAD/default-branch resolution, remote handling with missing or blank origin metadata, and tree entries whose names cannot be decoded. Assert each path’s intended fallback or error behavior, using existing repository/test helpers and preserving the changed best-effort semantics.Source: Coding guidelines
crates/mcb-providers/src/vcs/submodule.rs (1)
197-204: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDistinguish missing submodule URLs from URL lookup errors.
git2::Submodule::url()returnsResult<Option<&str>, Error>in this crate version, soErr(...)represents a lookup/failure case rather than a missing.gitmodulesURL. Match it separately when skipping so the log isn’t always reported as an orphan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/mcb-providers/src/vcs/submodule.rs` around lines 197 - 204, Update the submodule URL handling around submodule.url() to distinguish Ok(None), which should retain the orphaned-submodule warning and skip behavior, from Err(error), which should use an appropriate lookup-failure warning including the error details before returning None. Preserve the existing owned URL handling for Ok(Some(url)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/mcb-server/src/controllers/admin_config.rs`:
- Around line 14-20: Update load_admin_config to obtain config_root exclusively
from mcb_infrastructure::config::resolve_admin_config_root(), removing the
environment-variable lookup and DEFAULT_PRO_ADMIN_CONFIG_DIR fallback. Keep the
existing ConfigParser loading and error conversion behavior unchanged.
---
Outside diff comments:
In `@crates/mcb-providers/src/vcs/git.rs`:
- Around line 60-66: Add focused regression tests for get_default_branch
covering failed HEAD/default-branch resolution, remote handling with missing or
blank origin metadata, and tree entries whose names cannot be decoded. Assert
each path’s intended fallback or error behavior, using existing repository/test
helpers and preserving the changed best-effort semantics.
In `@crates/mcb-providers/src/vcs/submodule.rs`:
- Around line 197-204: Update the submodule URL handling around submodule.url()
to distinguish Ok(None), which should retain the orphaned-submodule warning and
skip behavior, from Err(error), which should use an appropriate lookup-failure
warning including the error details before returning None. Preserve the existing
owned URL handling for Ok(Some(url)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 448abf05-a7cd-4d91-9d40-5c594f36ac6f
⛔ Files ignored due to path filters (22)
Cargo.lockis excluded by!**/*.lockcrates/mcb-server/tests/contract/snapshots/contract__agent_contract_tests__agent_happy_path.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__agent_contract_tests__agent_invalid_args.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__agent_contract_tests__agent_store_without_session.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__entity_contract_tests__entity_happy_path.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__entity_contract_tests__entity_invalid_args.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__index_contract_tests__index_happy_path.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__index_contract_tests__index_invalid_args.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__memory_contract_tests__memory_get_observation_ids_empty.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__memory_contract_tests__memory_get_observation_ids_none.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__memory_contract_tests__memory_happy_path.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__memory_contract_tests__memory_invalid_args.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__project_contract_tests__project_happy_path.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__project_contract_tests__project_invalid_args.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__search_contract_tests__search_happy_path.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__search_contract_tests__search_invalid_args.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__session_contract_tests__session_happy_path.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__session_contract_tests__session_invalid_args.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__validate_contract_tests__validate_happy_path.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__validate_contract_tests__validate_invalid_args.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__vcs_contract_tests__vcs_happy_path.snapis excluded by!**/*.snapcrates/mcb-server/tests/contract/snapshots/contract__vcs_contract_tests__vcs_invalid_args.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
Cargo.tomlcrates/mcb-providers/src/vcs/git.rscrates/mcb-providers/src/vcs/submodule.rscrates/mcb-server/src/auth.rscrates/mcb-server/src/controllers/admin.rscrates/mcb-server/src/controllers/admin_config.rscrates/mcb-server/src/controllers/web/config.rscrates/mcb-server/src/handlers/memory/execution.rscrates/mcb-server/src/handlers/memory/list_timeline.rscrates/mcb-server/src/handlers/search.rscrates/mcb-server/src/handlers/session/common.rscrates/mcb-server/src/handlers/session/create.rscrates/mcb-server/src/handlers/session/get.rscrates/mcb-server/src/handlers/session/update.rscrates/mcb-server/src/handlers/vcs/analyze_impact.rscrates/mcb-server/src/handlers/vcs/compare_branches.rscrates/mcb-server/src/handlers/vcs/index_repo.rscrates/mcb-server/src/handlers/vcs/responses.rscrates/mcb-server/src/handlers/vcs/search_branch.rscrates/mcb-server/src/macros/handlers.rscrates/mcb-server/src/utils/mcp/fields.rscrates/mcb-server/src/utils/mcp/helpers.rscrates/mcb/src/initializers/mcp_server.rsscripts/hooks/pre-commitscripts/lib/mcb.sh
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*.rs
📄 CodeRabbit inference engine (.windsurfrules)
**/*.rs: Error handling: Use Error::vcs("msg") constructors, never unwrap()/expect() in production code
Lints: Set unsafe_code = "deny" and dead_code = "deny". Zero clippy warnings required
Testing: Runmake test(1700+ tests). New logic must include tests
MVI 200: Keep source files under approximately 200 lines; split into submodules when growing larger
**/*.rs: Usethiserrorconstructors for error handling (e.g.,Error::vcs("msg")) and never useunwrap()in production code
Use linkme for compile-time dependency injection and Handle pattern for hot-swap dependency injection, as specified in ADR-050
Keep source files under ~200 lines; split into submodules when growing beyond that size
Fix all warnings and clippy issues and leave tests passing after every change cycle
**/*.rs: Use the most restrictive type that compiles in Rust. No Any, no bare object, no suppression of type errors. Fix types at the source.
Depend on abstractions (protocols/interfaces); inject collaborators; no hidden globals or hard-wired construction inside business logic.
Use the most restrictive type that compiles. No Any, no bare object, no suppression of type errors. Fix types at the source; depend on declared contracts.
Enforce dependency rules: mcb-domain has zero internal dependencies; mcb-providers implements domain ports; mcb-infrastructure handles composition; mcb-server is entrypoint; mcb-utils is a leaf crate.
Keep imports ordered: std, external crates, mcb_* crates, local modules.
Files:
crates/mcb-server/src/handlers/memory/execution.rscrates/mcb-server/src/controllers/web/config.rscrates/mcb-server/src/handlers/session/get.rscrates/mcb-server/src/handlers/vcs/search_branch.rscrates/mcb-server/src/handlers/vcs/analyze_impact.rscrates/mcb-server/src/controllers/admin_config.rscrates/mcb-server/src/utils/mcp/fields.rscrates/mcb-server/src/utils/mcp/helpers.rscrates/mcb-server/src/handlers/vcs/compare_branches.rscrates/mcb-server/src/handlers/session/update.rscrates/mcb-server/src/handlers/memory/list_timeline.rscrates/mcb-server/src/handlers/vcs/index_repo.rscrates/mcb-server/src/macros/handlers.rscrates/mcb-providers/src/vcs/submodule.rscrates/mcb-providers/src/vcs/git.rscrates/mcb-server/src/handlers/search.rscrates/mcb-server/src/handlers/session/common.rscrates/mcb-server/src/handlers/vcs/responses.rscrates/mcb-server/src/handlers/session/create.rscrates/mcb-server/src/controllers/admin.rscrates/mcb/src/initializers/mcp_server.rscrates/mcb-server/src/auth.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
No TODOs, stubs, fakes, fallbacks, compat wrappers, or 'temporary' workarounds. No suppression directives (# type: ignore, blanket # noqa,
@ts-ignore, eslint-disable, etc.) and no escape-hatch typing (Any, bare object, unchecked casts) unless carrying a one-line documented justification.
Files:
crates/mcb-server/src/handlers/memory/execution.rscrates/mcb-server/src/controllers/web/config.rscrates/mcb-server/src/handlers/session/get.rscrates/mcb-server/src/handlers/vcs/search_branch.rscrates/mcb-server/src/handlers/vcs/analyze_impact.rscrates/mcb-server/src/controllers/admin_config.rscrates/mcb-server/src/utils/mcp/fields.rscrates/mcb-server/src/utils/mcp/helpers.rscrates/mcb-server/src/handlers/vcs/compare_branches.rscrates/mcb-server/src/handlers/session/update.rscrates/mcb-server/src/handlers/memory/list_timeline.rsCargo.tomlcrates/mcb-server/src/handlers/vcs/index_repo.rscrates/mcb-server/src/macros/handlers.rscrates/mcb-providers/src/vcs/submodule.rscrates/mcb-providers/src/vcs/git.rscrates/mcb-server/src/handlers/search.rscrates/mcb-server/src/handlers/session/common.rscrates/mcb-server/src/handlers/vcs/responses.rscrates/mcb-server/src/handlers/session/create.rscrates/mcb-server/src/controllers/admin.rscrates/mcb/src/initializers/mcp_server.rscrates/mcb-server/src/auth.rs
crates/mcb-server/src/handlers/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Do not import lower-level concrete providers directly into handlers. Add or reuse a domain port, wire the adapter in infrastructure, and resolve through the catalog/context.
Files:
crates/mcb-server/src/handlers/memory/execution.rscrates/mcb-server/src/handlers/session/get.rscrates/mcb-server/src/handlers/vcs/search_branch.rscrates/mcb-server/src/handlers/vcs/analyze_impact.rscrates/mcb-server/src/handlers/vcs/compare_branches.rscrates/mcb-server/src/handlers/session/update.rscrates/mcb-server/src/handlers/memory/list_timeline.rscrates/mcb-server/src/handlers/vcs/index_repo.rscrates/mcb-server/src/handlers/search.rscrates/mcb-server/src/handlers/session/common.rscrates/mcb-server/src/handlers/vcs/responses.rscrates/mcb-server/src/handlers/session/create.rs
**/*.{rs,yaml}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode configuration values in code. Add fields to the typed config model and populate every profile (development.yaml, test.yaml, production.yaml).
Files:
crates/mcb-server/src/handlers/memory/execution.rscrates/mcb-server/src/controllers/web/config.rscrates/mcb-server/src/handlers/session/get.rscrates/mcb-server/src/handlers/vcs/search_branch.rscrates/mcb-server/src/handlers/vcs/analyze_impact.rscrates/mcb-server/src/controllers/admin_config.rscrates/mcb-server/src/utils/mcp/fields.rscrates/mcb-server/src/utils/mcp/helpers.rscrates/mcb-server/src/handlers/vcs/compare_branches.rscrates/mcb-server/src/handlers/session/update.rscrates/mcb-server/src/handlers/memory/list_timeline.rscrates/mcb-server/src/handlers/vcs/index_repo.rscrates/mcb-server/src/macros/handlers.rscrates/mcb-providers/src/vcs/submodule.rscrates/mcb-providers/src/vcs/git.rscrates/mcb-server/src/handlers/search.rscrates/mcb-server/src/handlers/session/common.rscrates/mcb-server/src/handlers/vcs/responses.rscrates/mcb-server/src/handlers/session/create.rscrates/mcb-server/src/controllers/admin.rscrates/mcb/src/initializers/mcp_server.rscrates/mcb-server/src/auth.rs
crates/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/**/*.rs: Use Error constructors and Result aliases from mcb-domain; do not build raw domain errors by hand.
Use ? for propagation. No unwrap(), expect(), panic!(), todo!(), or unimplemented!() in production paths.
Keep first-party source files compact; split modules before they become difficult to review.
Files:
crates/mcb-server/src/handlers/memory/execution.rscrates/mcb-server/src/controllers/web/config.rscrates/mcb-server/src/handlers/session/get.rscrates/mcb-server/src/handlers/vcs/search_branch.rscrates/mcb-server/src/handlers/vcs/analyze_impact.rscrates/mcb-server/src/controllers/admin_config.rscrates/mcb-server/src/utils/mcp/fields.rscrates/mcb-server/src/utils/mcp/helpers.rscrates/mcb-server/src/handlers/vcs/compare_branches.rscrates/mcb-server/src/handlers/session/update.rscrates/mcb-server/src/handlers/memory/list_timeline.rscrates/mcb-server/src/handlers/vcs/index_repo.rscrates/mcb-server/src/macros/handlers.rscrates/mcb-providers/src/vcs/submodule.rscrates/mcb-providers/src/vcs/git.rscrates/mcb-server/src/handlers/search.rscrates/mcb-server/src/handlers/session/common.rscrates/mcb-server/src/handlers/vcs/responses.rscrates/mcb-server/src/handlers/session/create.rscrates/mcb-server/src/controllers/admin.rscrates/mcb/src/initializers/mcp_server.rscrates/mcb-server/src/auth.rs
crates/mcb-server/src/{args,handlers,tools}/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
crates/mcb-server/src/{args,handlers,tools}/**/*.rs: When changing an MCP tool: (1) Update args schema and validator, (2) Update handler, (3) Update docs/MCP_TOOLS.md if public contract changed, (4) Add or update focused tests.
MCP tool args schemas are split across crates/mcb-server/src/args/, handlers across crates/mcb-server/src/handlers/, and tool logic across crates/mcb-server/src/tools/.
Files:
crates/mcb-server/src/handlers/memory/execution.rscrates/mcb-server/src/handlers/session/get.rscrates/mcb-server/src/handlers/vcs/search_branch.rscrates/mcb-server/src/handlers/vcs/analyze_impact.rscrates/mcb-server/src/handlers/vcs/compare_branches.rscrates/mcb-server/src/handlers/session/update.rscrates/mcb-server/src/handlers/memory/list_timeline.rscrates/mcb-server/src/handlers/vcs/index_repo.rscrates/mcb-server/src/handlers/search.rscrates/mcb-server/src/handlers/session/common.rscrates/mcb-server/src/handlers/vcs/responses.rscrates/mcb-server/src/handlers/session/create.rs
crates/mcb-server/src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Prefer existing macros and patterns: tool_action!, tool_schema!, tool_enum!, register_tool!, linkme distributed slices, and the Handle pattern.
Files:
crates/mcb-server/src/handlers/memory/execution.rscrates/mcb-server/src/controllers/web/config.rscrates/mcb-server/src/handlers/session/get.rscrates/mcb-server/src/handlers/vcs/search_branch.rscrates/mcb-server/src/handlers/vcs/analyze_impact.rscrates/mcb-server/src/controllers/admin_config.rscrates/mcb-server/src/utils/mcp/fields.rscrates/mcb-server/src/utils/mcp/helpers.rscrates/mcb-server/src/handlers/vcs/compare_branches.rscrates/mcb-server/src/handlers/session/update.rscrates/mcb-server/src/handlers/memory/list_timeline.rscrates/mcb-server/src/handlers/vcs/index_repo.rscrates/mcb-server/src/macros/handlers.rscrates/mcb-server/src/handlers/search.rscrates/mcb-server/src/handlers/session/common.rscrates/mcb-server/src/handlers/vcs/responses.rscrates/mcb-server/src/handlers/session/create.rscrates/mcb-server/src/controllers/admin.rscrates/mcb-server/src/auth.rs
**/*.toml
📄 CodeRabbit inference engine (.cursor/rules/mcb.mdc)
Enforce strict Rust lints:
unsafe_code = "deny",dead_code = "deny",unused_imports = "deny"
Files:
Cargo.toml
Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Version, MSRV, workspace members, lint policy are defined in Cargo.toml. When a static document disagrees with Cargo.toml or code, trust the executable source first and update the doc.
Files:
Cargo.toml
🧠 Learnings (1)
📚 Learning: 2026-03-01T22:45:33.850Z
Learnt from: marlon-costa-dc
Repo: marlonsc/mcb PR: 113
File: crates/mcb-server/src/controllers/admin_config.rs:15-17
Timestamp: 2026-03-01T22:45:33.850Z
Learning: In Rust files that use sea_orm_pro::ConfigParser for reading admin_config.yaml, do not reuse Loco's ConfigLoader. Use the Admin ConfigParser path: create a ConfigParser with ConfigParser::new().load_config() to load the YAML with its own schema. Resolve the config root with mcb_infrastructure::config::resolve_admin_config_root() instead of hardcoding paths. This pattern applies to controller modules under crates/mcb-server/src/controllers/ and any similar admin_config loading logic that relies on the admin_config.yaml.
Applied to files:
crates/mcb-server/src/controllers/web/config.rscrates/mcb-server/src/controllers/admin_config.rscrates/mcb-server/src/controllers/admin.rs
🔇 Additional comments (24)
crates/mcb-providers/src/vcs/git.rs (1)
72-77: LGTM!Also applies to: 111-111
crates/mcb-server/src/auth.rs (1)
14-37: LGTM!Also applies to: 61-61, 70-70, 84-84, 117-121, 131-131, 140-145, 155-155
crates/mcb-server/src/controllers/admin.rs (1)
44-46: LGTM!Also applies to: 65-66, 117-124, 134-134
crates/mcb-server/src/controllers/admin_config.rs (1)
1-1: LGTM!crates/mcb-server/src/controllers/web/config.rs (1)
41-41: LGTM!crates/mcb-server/src/handlers/memory/execution.rs (1)
35-37: LGTM!crates/mcb-server/src/utils/mcp/fields.rs (1)
12-51: LGTM!crates/mcb-server/src/utils/mcp/helpers.rs (1)
120-126: LGTM!crates/mcb-server/src/handlers/session/common.rs (1)
13-20: LGTM!crates/mcb-server/src/handlers/vcs/search_branch.rs (1)
18-24: LGTM!Also applies to: 55-61
crates/mcb-server/src/handlers/vcs/responses.rs (1)
141-150: LGTM!crates/mcb-server/src/macros/handlers.rs (1)
9-17: LGTM!Also applies to: 30-30
scripts/hooks/pre-commit (1)
8-11: Move Git-variable cleanup before repository discovery.The hook still runs
git rev-parse --show-toplevelbefore unsetting inherited Git variables, so leakedGIT_DIRorGIT_WORK_TREEcan redirect it to the wrong repository. Move this block before Line 7. This repeats the unresolved prior review finding.Cargo.toml (1)
273-273: LGTM!Also applies to: 287-293
scripts/lib/mcb.sh (1)
56-70: LGTM!crates/mcb-server/src/handlers/memory/list_timeline.rs (1)
68-83: LGTM!Also applies to: 95-95
crates/mcb-server/src/handlers/search.rs (1)
90-104: LGTM!Also applies to: 118-118
crates/mcb-server/src/handlers/session/create.rs (1)
83-93: LGTM!Also applies to: 139-147, 157-157
crates/mcb-server/src/handlers/session/get.rs (1)
27-27: LGTM!crates/mcb-server/src/handlers/session/update.rs (1)
31-31: LGTM!crates/mcb-server/src/handlers/vcs/analyze_impact.rs (1)
27-27: LGTM!crates/mcb-server/src/handlers/vcs/compare_branches.rs (1)
25-25: LGTM!crates/mcb-server/src/handlers/vcs/index_repo.rs (1)
25-25: LGTM!crates/mcb/src/initializers/mcp_server.rs (1)
212-240: LGTM!Also applies to: 244-266, 282-314, 359-359
| pub fn load_admin_config() -> Result<serde_json::Value, Box<loco_rs::Error>> { | ||
| let config_root = std::env::var("MCB_PRO_ADMIN_CONFIG_DIR") | ||
| .unwrap_or_else(|_| DEFAULT_PRO_ADMIN_CONFIG_DIR.to_owned()); | ||
| let cfg = sea_orm_pro::ConfigParser::new() | ||
| .load_config(&config_root) | ||
| .map_err(|e| loco_rs::Error::string(&e.to_string()))?; | ||
| serde_json::to_value(cfg).map_err(|e| loco_rs::Error::string(&e.to_string())) | ||
| .map_err(|e| Box::new(loco_rs::Error::string(&e.to_string())))?; | ||
| serde_json::to_value(cfg).map_err(|e| Box::new(loco_rs::Error::string(&e.to_string()))) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve the admin configuration root through the typed infrastructure helper.
The hard-coded DEFAULT_PRO_ADMIN_CONFIG_DIR fallback bypasses the canonical configuration-root resolution and can load the wrong admin_config.yaml in deployments where MCB_PRO_ADMIN_CONFIG_DIR is unset. Replace this path selection with mcb_infrastructure::config::resolve_admin_config_root().
As per coding guidelines, configuration values must not be hard-coded in Rust. Based on learnings, admin configuration must use the ConfigParser path together with resolve_admin_config_root() rather than hardcoded paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/mcb-server/src/controllers/admin_config.rs` around lines 14 - 20,
Update load_admin_config to obtain config_root exclusively from
mcb_infrastructure::config::resolve_admin_config_root(), removing the
environment-variable lookup and DEFAULT_PRO_ADMIN_CONFIG_DIR fallback. Keep the
existing ConfigParser loading and error conversion behavior unchanged.
Sources: Coding guidelines, Learnings
1 participant
Root fixes
Validation
Summary by cubic
Clears RustSec advisories and clippy result_large_err by updating key crates and boxing large errors. Also hardens admin auth, git hooks, and bootstrap error boundaries to stabilize builds and local gates.
Dependencies
crossbeam-epoch0.9.18 → 0.9.20 (RUSTSEC-2026-0204).serial_test3.4 → 4.0.0; dropsscc, removing RUSTSEC-2026-0205 without an MSRV bump.git20.20 → 0.21; updated call sites for getters now returningResult<Option<&str>>.quick-xmlto 0.41 for first-party crates.opendalpinsquick-xml< 0.41 and XML services are disabled, so DoS paths are unreachable.Bug Fixes
CallToolResult,loco_rs::Error) across server handlers and initializers.AuthErrorand controller mapping to Loco errors; replacedloco_rs::Resultinmcb-serverauth.mcb_bin: prefer workspace-built binary over PATH to avoid shim issues.GIT_*env to prevent fixture commits into the repo.pull_requestsynchronize to ensure gate validation runs on updates.Written for commit 70330fc. Summary will update on new commits.