Skip to content

fix(cerebro): harden production readiness#687

Merged
yacosta738 merged 6 commits into
mainfrom
cerebro
Apr 26, 2026
Merged

fix(cerebro): harden production readiness#687
yacosta738 merged 6 commits into
mainfrom
cerebro

Conversation

@yacosta738
Copy link
Copy Markdown
Contributor

Related Issues

fix #669
fix #670
fix #671
fix #672
fix #673
fix #674
fix #675
fix #676


Summary

Harden Cerebro for production readiness by fixing TOML config loading, enforcing startup validation, adding health/readiness endpoints, making request/body and auth behavior explicit, and improving structured observability.
Also tighten delivery safeguards with container default hardening, explicit CI checks for the crate, and binary smoke verification.


Tested Information

Verified from the repository root with targeted Cerebro checks:

  • cargo check --manifest-path clients/cerebro/Cargo.toml
  • cargo test --manifest-path clients/cerebro/Cargo.toml --test config_load_test --test storage_config_test --test health_endpoints_test --test http_limits_test --test mcp_auth_policy --test mcp_tools_contract --test tui_event_emission_tests
  • cargo test --manifest-path clients/cerebro/Cargo.toml --test mcp_tools_contract -- --nocapture

Coverage of the changes includes:

  • TOML config regression loading
  • startup validation for loopback vs non-loopback auth requirements
  • /healthz and /readyz
  • oversized request rejection
  • auth bearer/audit token policy
  • event emission metadata for request lifecycle

Documentation Impact

  • Docs updated in:
    • clients/cerebro/README.md
  • No docs update required because:
  • I verified the documentation matches the current behavior.

Breaking Changes

No intentional breaking API changes, but startup is now stricter for non-loopback deployments without an auth_token, and production operators should use GET /healthz and GET /readyz instead of probing POST /mcp.


Checklist

  • I have checked that there isn’t already a PR solving the same problem.
  • I have read the Contributing Guidelines.
  • I ensured my code follows the project's style guidelines.
  • I have added or updated tests that prove my fix is effective or that my feature works.
  • I have updated the documentation, or I explained above why no documentation update is needed.
  • I verified the documentation matches the current behavior.
  • I have documented any breaking changes in the Breaking Changes section.
  • I have linked the related issue (if any).

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 26, 2026

Warning

Rate limit exceeded

@yacosta738 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 48 minutes and 59 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 59 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 15b7570a-49d9-43b8-90fd-05b2a2b82328

📥 Commits

Reviewing files that changed from the base of the PR and between 47f0e36 and 916d33c.

📒 Files selected for processing (7)
  • .github/workflows/pull-request-check.yml
  • clients/cerebro/README.md
  • clients/cerebro/src/config.rs
  • clients/cerebro/src/server.rs
  • clients/cerebro/src/storage/surreal.rs
  • clients/cerebro/tests/health_endpoints_test.rs
  • clients/cerebro/tests/storage_config_test.rs
📝 Walkthrough

Walkthrough

Adds CI smoke and Rust quality gates, enables TOML serde feature, hardens container defaults and docs, introduces startup validation that requires auth for non-loopback bindings, adds /healthz and /readyz with storage readiness checks, enforces a 1 MiB request body limit, hardens token parsing/comparison, and adds related tests.

Changes

Cohort / File(s) Summary
CI / Release Smoke
.github/workflows/_build-cerebro-binaries.yml, .github/workflows/pull-request-check.yml, .github/workflows/dependency-review.yml
Add platform-specific post-build smoke test for the compiled cerebro binary; add cargo fmt/clippy/check/test steps for clients/cerebro; change checkout fetch-depth to full history.
Crate Manifest & Dev deps
clients/cerebro/Cargo.toml
Enable toml serde feature; add subtle dependency and tower dev-dependency (util feature) for tests.
Startup Validation & Config
clients/cerebro/src/config.rs, clients/cerebro/src/main.rs, clients/cerebro/src/bin/cerebro.rs
Normalize/trim secrets, add demo-credential detection, introduce validate_startup_requirements() enforcing auth presence for non-loopback binds and rejecting demo SurrealDB creds; invoke validation early in startup.
HTTP Server, Auth & Observability
clients/cerebro/src/server.rs
Add GET /healthz and GET /readyz (calls storage.ready()); enforce 1 MiB JSON body limit via DefaultBodyLimit; factor parse_bearer_token; use constant-time token comparison; add per-request tracing span and structured logs around auth and tool execution.
Storage readiness
clients/cerebro/src/storage/mod.rs, clients/cerebro/src/storage/surreal.rs
Add Storage::ready() trait method (default Ok); implement SurrealDB readiness check (runs RETURN 1;, maps failures to readiness error).
Container defaults & docs
clients/cerebro/Dockerfile.release-prebuilt, clients/cerebro/README.md
Tighten generated config (bind to 127.0.0.1, demo password local-dev-only), update probe guidance to use /healthz and /readyz, and add deployment/CI/observability guidance.
Tests
clients/cerebro/tests/*
clients/cerebro/tests/config_load_test.rs, .../health_endpoints_test.rs, .../http_limits_test.rs, .../mcp_auth_policy.rs, .../storage_config_test.rs, .../tui_event_emission_tests.rs
Add/expand tests: TOML config parsing, health/readiness behavior, request-body size rejection, auth edge-cases (empty/trailing whitespace, prefix casing, audit token), startup validation for non-loopback auth requirement, and stronger TUI event assertions.

Sequence Diagram(s)

sequenceDiagram
participant Client as Client
participant Router as Axum Router
participant Server as Cerebro Service
participant Storage as Storage Backend
Client->>Router: GET /healthz
Router->>Server: handle_healthz()
Server-->>Client: 200 {status:"ok"}

Client->>Router: GET /readyz
Router->>Server: handle_readyz()
Server->>Storage: storage.ready()
alt storage healthy
    Storage-->>Server: Ok
    Server-->>Client: 200 {status:"ready"}
else storage unhealthy
    Storage-->>Server: Err(error)
    Server-->>Client: 503 {status:"not_ready", error: "<msg>"}
end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 32.61% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(cerebro): harden production readiness' follows Conventional Commit style with the 'fix' prefix, has clear imperative language, and is 41 characters (well under 72-char limit).
Description check ✅ Passed The PR description comprehensively covers all template sections: related issues (#669#676), detailed summary, tested information with specific commands, documentation impact, breaking changes, and all checklist items marked complete.
Linked Issues check ✅ Passed The PR directly addresses all eight linked objectives: #669 (TOML serde fix with tests), #670 (health/readiness endpoints), #671 (startup validation), #672 (container hardening), #673 (request correlation/structured logging), #674 (CI quality gates), #675 (request body limits), and #676 (auth hardening).
Out of Scope Changes check ✅ Passed All changes are scoped to the eight linked issues; workflow updates, Dockerfile adjustments, dependency additions, and config/auth/observability changes are all justified by the PR objectives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cerebro

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added the size/m Denotes a medium change size label Apr 26, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 26, 2026

✅ Contributor Report

User: @yacosta738
Status: Passed (12/13 metrics passed)

Metric Description Value Threshold Status
PR Merge Rate PRs merged vs closed 92% >= 30%
Repo Quality Repos with ≥100 stars 0 >= 0
Positive Reactions Positive reactions received 11 >= 1
Negative Reactions Negative reactions received 0 <= 5
Account Age GitHub account age 3102 days >= 30 days
Activity Consistency Regular activity over time 108% >= 0%
Issue Engagement Issues with community engagement 0 >= 0
Code Reviews Code reviews given to others 604 >= 0
Merger Diversity Unique maintainers who merged PRs 2 >= 0
Repo History Merge Rate Merge rate in this repo 93% >= 0%
Repo History Min PRs Previous PRs in this repo 282 >= 0
Profile Completeness Profile richness (bio, followers) 90 >= 0
Suspicious Patterns Spam-like activity detection 1 N/A

Contributor Report evaluates based on public GitHub activity. Analysis period: 2025-04-26 to 2026-04-26

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Apr 26, 2026

Deploying corvus with  Cloudflare Pages  Cloudflare Pages

Latest commit: 916d33c
Status: ✅  Deploy successful!
Preview URL: https://2b3570bf.corvus-42x.pages.dev
Branch Preview URL: https://cerebro.corvus-42x.pages.dev

View logs

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
clients/cerebro/src/server.rs (1)

226-255: ⚠️ Potential issue | 🔴 Critical

Critical: token comparisons are not constant-time—timing-attack risk that this PR was supposed to fix.

Lines 246 and 250 use plain == to compare bearer tokens against audit_token and expected. str equality short-circuits on the first differing byte, leaking byte-by-byte token information via response timing. PR objective #676 explicitly calls for "secure comparison semantics."

Replace both comparisons with constant-time comparison using subtle::ConstantTimeEq:

Fix using subtle crate

Add subtle = "2" to Cargo.toml, then:

+use subtle::ConstantTimeEq;

     fn authorize(&self, auth_header: Option<&str>) -> Result<AuthContext, CerebroError> {
         // ...
         let token = parse_bearer_token(auth_header)?;

-        if audit_token.is_some_and(|audit| token == audit) {
+        if audit_token.is_some_and(|audit| token.as_bytes().ct_eq(audit.as_bytes()).into()) {
             return Ok(AuthContext { is_audit: true });
         }

-        if token == expected {
+        if token.as_bytes().ct_eq(expected.as_bytes()).into() {
             return Ok(AuthContext { is_audit: false });
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clients/cerebro/src/server.rs` around lines 226 - 255, In authorize(),
replace the unsafe short-circuiting == comparisons (token == audit and token ==
expected) with constant-time comparisons: add subtle = "2" to Cargo.toml, use
subtle::ConstantTimeEq, and compare byte slices via
token.as_bytes().ct_eq(audit.as_bytes()) and
token.as_bytes().ct_eq(expected.as_bytes()), converting the Choice result to a
boolean (e.g., .unwrap_u8() == 1) to decide the branch; keep the rest of
authorize (parse_bearer_token, audit_token handling, returning
AuthContext/CerebroError::Unauthorized) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/pull-request-check.yml:
- Around line 135-139: Update the Cerebro workflow steps to enforce the PR gate:
add a formatting check step that runs cargo fmt --all -- --check against the
Cerebro manifest, add a linting step that runs cargo clippy --all-targets -- -D
warnings against the Cerebro manifest, and make the existing "🦀 Check Cerebro
crate" step include --locked to match "🧪 Test Cerebro crate"; locate and update
the steps named "🦀 Check Cerebro crate" and "🧪 Test Cerebro crate" and insert
new steps (e.g., "🧹 Fmt Cerebro crate" and "🧭 Clippy Cerebro crate") that
reference clients/cerebro/Cargo.toml so format and clippy failures block the PR
as intended.

In `@clients/cerebro/Dockerfile.release-prebuilt`:
- Around line 18-30: The bundled demo config binds the service to loopback and
ships a static password, which causes two problems: (1) document clearly in the
Dockerfile/comment/README that the default host="127.0.0.1" inside the container
prevents access via docker -p and that operators must override host (e.g., to
"0.0.0.0") and supply auth_token to use port mapping; and (2) harden the startup
validator (the function validate_embedded_surreal and the startup auth_token
gate) so when the configured host is non-loopback it rejects known demo
credentials (add a denylist like "local-dev-only",
"CHANGE_ME_BEFORE_PRODUCTION", "root" instead of just checking non-empty) and
fail startup with a clear error message instructing operators to set a secure
password or mount real credentials.

In `@clients/cerebro/README.md`:
- Around line 37-78: Update the docs to (1) replace "a conservative HTTP request
body limit" with the explicit value "1 MiB (1,048,576 bytes)" (this is asserted
in http_limits_test.rs); (2) state explicitly that the service will refuse to
start if you bind to a non-loopback address without an auth_token (reflecting
the startup check in main that requires auth_token for non-loopback bindings);
and (3) reword the POST /mcp line to read that it is "not a substitute for the
probe endpoints above" instead of implying probe usage—adjust the lines
mentioning request limits, production deployment auth_token, and the POST /mcp
probe note accordingly.

In `@clients/cerebro/src/config.rs`:
- Around line 285-290: The validation trims auth_token before checking emptiness
which mismatches tokens loaded verbatim from config files; change the code so
the token is canonicalized at storage time (trim once on deserialization or in
the config's constructor) rather than only on validation—update the
deserialization logic (or the method apply_env_overrides if you prefer a single
normalization point) to trim and store the token into auth_token, ensuring
auth_token, apply_env_overrides, and any places that read auth_token (e.g., the
code computing auth_is_present) all operate on the same canonical trimmed value.

In `@clients/cerebro/src/server.rs`:
- Around line 79-86: Extract the literal body limit into a named constant and
use it in router(): define const MAX_REQUEST_BODY_BYTES: usize = 1024 * 1024 and
replace DefaultBodyLimit::max(1024 * 1024) with
DefaultBodyLimit::max(MAX_REQUEST_BODY_BYTES) in the Arc<Self>::router method so
tests/docs can grep the value; keep the rest of router wiring (routes
handle_health, handle_ready, handle_mcp and with_state(self)) unchanged.
- Around line 295-306: The readiness endpoint currently calls storage().count()
inside handle_ready which triggers an O(rows) table scan; replace that with a
cheap connectivity-ready check by adding an async trait method
Storage::ready(&self) -> Result<(), CerebroError> (with a cheap default no-op),
implement ready for the SurrealDB backend in surreal.rs to perform a bounded
probe (e.g., SELECT 1 / INFO FOR DB or a single-row LIMIT 1 query) and have
in-memory/disk backends return Ok immediately; finally update handle_ready to
call service.storage().ready().await and return not_ready only on error
(optionally add a short TTL cache around ready if you want to avoid frequent
backend probes).
- Around line 128-130: request_id is being set to request.id.to_string(), which
serializes JSON strings with embedded quotes; change the normalization so string
IDs are unwrapped and non-string IDs are stringified. Replace the current
assignment of request_id with logic that checks request.id: if it's a JSON
string use the inner &str (e.g., Value::String or as_str()), otherwise call
to_string() on the Value; update the test in tui_event_emission_tests.rs to
expect "1" instead of "\"1\"". Ensure you modify the symbol request_id
initialization (near where request.params.name and start are set) so tracing and
ToolCallEvent use the unquoted ID.
- Around line 258-270: The parse_bearer_token function currently uses
header.strip_prefix("Bearer ") which enforces a case-sensitive scheme and
rejects RFC 7235-compliant variants; update parse_bearer_token to accept the
"Bearer" scheme case-insensitively and allow HTTP linear whitespace
(tabs/multiple spaces) between scheme and token, while still returning
CerebroError::Unauthorized for missing/empty tokens and preserving strict token
comparison elsewhere; locate and modify the parse_bearer_token implementation to
perform a case-insensitive prefix check for "bearer" (or normalize the scheme to
ASCII lowercase) then trim only the LWS before validating token presence.
- Around line 128-150: The span guard created with let _enter = span.enter()
(variable span and the enter call) is held across the await to
self.tools.handle(...).await and must be removed; instead, avoid keeping an
EnteredSpan alive across await boundaries by using tracing::instrumentation
(e.g., wrap the future with .instrument(span) or annotate the async
block/function with #[tracing::instrument]) so the span is entered/exited on
each poll; specifically, remove or stop using span.enter() where authorize(...)
and the subsequent self.tools.handle(...).await execute and apply
.instrument(span) to the async call that awaits self.tools.handle(...) (or
annotate the surrounding async function) to ensure correct span lifetime without
holding the EnteredSpan across await points.

In `@clients/cerebro/tests/config_load_test.rs`:
- Around line 25-31: Update the test to assert actual secret values instead of
only presence: after calling CerebroConfig::load(Some(&path)), compare
config.auth_token (and config.surreal.password) against the expected literal
test secrets used in the fixture to ensure secret_string_opt::deserialize
preserved content; keep the existing checks for host/port/username but replace
assert!(config.auth_token.is_some()) and
assert!(config.surreal.password.is_some()) with exact equality assertions that
the tokens/passwords match the known test values.

In `@clients/cerebro/tests/health_endpoints_test.rs`:
- Around line 29-49: Add a failing readiness-path test that ensures handle_ready
returns 503 when storage.count() errors: create a small fake Storage
implementation whose count() returns Err(...) (or a test double implementing the
same trait), construct a CerebroService with that fake storage instead of
InMemoryStorage, call the /readyz endpoint via
service.router().oneshot(Request::builder().uri("/readyz")...), and assert the
response.status() is StatusCode::SERVICE_UNAVAILABLE; this locks both branches
of handle_ready and references Storage::count, CerebroService, handle_ready, and
the /readyz endpoint.

In `@clients/cerebro/tests/mcp_auth_policy.rs`:
- Around line 102-150: The test rejects_bearer_token_with_wrong_case_prefix
enforces a non-RFC behavior (Bearer scheme is case-insensitive); either rename
the test to reflect a deliberate strict policy (e.g.,
rejects_lowercase_prefix_per_strict_policy) so intent is clear, or update the
auth parsing in server.rs (the code path used by
CerebroService::handle_json_rpc) to compare the auth-scheme case-insensitively
(use eq_ignore_ascii_case("Bearer") or normalize to_lowercase before comparing)
and accept "bearer" as valid; pick one of these fixes and apply it consistently
to the test and the parser.

In `@clients/cerebro/tests/storage_config_test.rs`:
- Around line 127-144: Add two unit tests to fully exercise
validate_startup_requirements: one that sets config.host to a non-loopback
(e.g., "0.0.0.0"), assigns a non-empty auth_token (e.g., "secrettoken") and
asserts validate_startup_requirements() returns Ok (to verify a real token
unblocks startup), and another that sets host to non-loopback and auth_token to
a whitespace-only string (e.g., "  \t\n") and asserts
validate_startup_requirements() returns an Err containing the "auth token is
required" message (to verify the trim+is_empty rejection). Use the existing
base_config() helper and the same assert patterns as the other tests so they
integrate consistently with the tests named
startup_validation_requires_auth_token_for_non_loopback_host and
startup_validation_allows_loopback_without_auth_token_for_local_dev.

---

Outside diff comments:
In `@clients/cerebro/src/server.rs`:
- Around line 226-255: In authorize(), replace the unsafe short-circuiting ==
comparisons (token == audit and token == expected) with constant-time
comparisons: add subtle = "2" to Cargo.toml, use subtle::ConstantTimeEq, and
compare byte slices via token.as_bytes().ct_eq(audit.as_bytes()) and
token.as_bytes().ct_eq(expected.as_bytes()), converting the Choice result to a
boolean (e.g., .unwrap_u8() == 1) to decide the branch; keep the rest of
authorize (parse_bearer_token, audit_token handling, returning
AuthContext/CerebroError::Unauthorized) unchanged.
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 29c19720-654e-40d4-a480-34577f385279

📥 Commits

Reviewing files that changed from the base of the PR and between 7b2c679 and 810f5ae.

⛔ Files ignored due to path filters (1)
  • clients/cerebro/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (15)
  • .github/workflows/_build-cerebro-binaries.yml
  • .github/workflows/pull-request-check.yml
  • clients/cerebro/Cargo.toml
  • clients/cerebro/Dockerfile.release-prebuilt
  • clients/cerebro/README.md
  • clients/cerebro/src/bin/cerebro.rs
  • clients/cerebro/src/config.rs
  • clients/cerebro/src/main.rs
  • clients/cerebro/src/server.rs
  • clients/cerebro/tests/config_load_test.rs
  • clients/cerebro/tests/health_endpoints_test.rs
  • clients/cerebro/tests/http_limits_test.rs
  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/tests/storage_config_test.rs
  • clients/cerebro/tests/tui_event_emission_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: sonar
  • GitHub Check: pr-checks
  • GitHub Check: dependency-review
  • GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs

⚙️ CodeRabbit configuration file

**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.

Files:

  • clients/cerebro/src/main.rs
  • clients/cerebro/src/bin/cerebro.rs
  • clients/cerebro/tests/config_load_test.rs
  • clients/cerebro/tests/health_endpoints_test.rs
  • clients/cerebro/src/config.rs
  • clients/cerebro/tests/tui_event_emission_tests.rs
  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/tests/http_limits_test.rs
  • clients/cerebro/tests/storage_config_test.rs
  • clients/cerebro/src/server.rs
**/*

⚙️ CodeRabbit configuration file

**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.

Files:

  • clients/cerebro/src/main.rs
  • clients/cerebro/src/bin/cerebro.rs
  • clients/cerebro/tests/config_load_test.rs
  • clients/cerebro/tests/health_endpoints_test.rs
  • clients/cerebro/Cargo.toml
  • clients/cerebro/src/config.rs
  • clients/cerebro/tests/tui_event_emission_tests.rs
  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/README.md
  • clients/cerebro/Dockerfile.release-prebuilt
  • clients/cerebro/tests/http_limits_test.rs
  • clients/cerebro/tests/storage_config_test.rs
  • clients/cerebro/src/server.rs
**/*.{md,mdx}

⚙️ CodeRabbit configuration file

**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.

Files:

  • clients/cerebro/README.md
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Keep startup path lean and avoid heavy initialization in command parsing flow

Applied to files:

  • clients/cerebro/src/main.rs
  • clients/cerebro/src/bin/cerebro.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths

Applied to files:

  • clients/cerebro/src/main.rs
  • clients/cerebro/src/bin/cerebro.rs
  • clients/cerebro/src/server.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why

Applied to files:

  • clients/cerebro/src/main.rs
  • clients/cerebro/src/bin/cerebro.rs
  • .github/workflows/pull-request-check.yml
  • .github/workflows/_build-cerebro-binaries.yml
  • clients/cerebro/tests/health_endpoints_test.rs
  • clients/cerebro/Cargo.toml
  • clients/cerebro/src/config.rs
  • clients/cerebro/README.md
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/.github/**/*.{yml,yaml} : For workflow/template-only changes, ensure YAML/template syntax validity

Applied to files:

  • .github/workflows/pull-request-check.yml
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests

Applied to files:

  • clients/cerebro/tests/health_endpoints_test.rs
  • clients/cerebro/src/server.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Do not add heavy dependencies for minor convenience; justify new crate additions

Applied to files:

  • clients/cerebro/Cargo.toml
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Preserve release-size profile assumptions in `Cargo.toml` and avoid adding heavy dependencies unless clearly justified

Applied to files:

  • clients/cerebro/Cargo.toml
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable

Applied to files:

  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/src/server.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks

Applied to files:

  • clients/cerebro/src/server.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path

Applied to files:

  • clients/cerebro/src/server.rs
🔇 Additional comments (7)
clients/cerebro/Cargo.toml (2)

51-51: LGTM — tower/util is the right scope for the new tests.

ServiceExt::oneshot from tower::util is what the new health_endpoints_test.rs and http_limits_test.rs rely on. Restricted to dev-deps, no impact on release size.


34-34: LGTM — serde + parse features correctly enable toml::from_str.

In toml 1.x, toml::from_str requires both parse and serde features when default-features are disabled. This matches the requirements of CerebroConfig::load and introduces no unnecessary surface.

.github/workflows/_build-cerebro-binaries.yml (1)

87-96: LGTM — smoke probe wired correctly per OS.

Paths line up with the prior build steps (clients/cerebro working dir on Unix, repo-root prefix on Windows), --help is a cheap liveness check, and exit codes will propagate from both bash and pwsh. Nice incremental gate against shipping a binary that fails to even initialize CLI parsing.

clients/cerebro/src/config.rs (1)

282-299: LGTM — secure-by-default startup gate.

Order is right (storage validation first, then network-exposure rule), ExposeSecret is held only long enough to trim and check emptiness, and the loopback predicate treats unrecognized hostnames as non-loopback (fail closed). Aligns with the deny-by-default policy for the security/gateway/config surface.

clients/cerebro/src/bin/cerebro.rs (1)

79-83: LGTM — validation lands before any side effects.

Calling validate_startup_requirements() after apply_env_overrides() and the tui.enabled finalization, but before from_config(...).await and TcpListener::bind, gives operators a clean fast-fail with no half-initialized state. Matches the parallel call in src/main.rs.

clients/cerebro/src/main.rs (1)

19-22: LGTM — fail-fast wired correctly.

Validation runs after env overrides and before any network or storage side effects, matching bin/cerebro.rs. The two startup paths stay in sync, which is what the new tests in storage_config_test.rs rely on.

clients/cerebro/tests/http_limits_test.rs (1)

1-38: LGTM — clean over-limit assertion for the new DefaultBodyLimit.

The padding sized to 2 * 1024 * 1024 cleanly exceeds the 1 MiB router cap and the bearer/content-type headers ensure you're testing the body-size path rather than tripping on auth or parser preconditions first. Nothing more to do here.

Comment thread .github/workflows/pull-request-check.yml
Comment thread clients/cerebro/Dockerfile.release-prebuilt
Comment thread clients/cerebro/README.md
Comment thread clients/cerebro/src/config.rs Outdated
Comment thread clients/cerebro/src/server.rs
Comment thread clients/cerebro/src/server.rs
Comment thread clients/cerebro/tests/config_load_test.rs Outdated
Comment thread clients/cerebro/tests/health_endpoints_test.rs
Comment thread clients/cerebro/tests/mcp_auth_policy.rs
Comment thread clients/cerebro/tests/storage_config_test.rs
@github-actions github-actions Bot added size/l Denotes a large change size and removed size/m Denotes a medium change size labels Apr 26, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
clients/cerebro/src/server.rs (1)

53-90: 🧹 Nitpick | 🔵 Trivial

Body limit + healthz/readyz wiring LGTM; consider rate-limiting the probes.

MAX_REQUEST_BODY_BYTES constant is greppable from tests/docs (addresses the prior nit), and applying DefaultBodyLimit::max(...) at the router level uniformly caps /mcp while leaving the GET probes unaffected (they have no body to read).

Operationally: /healthz and /readyz are unauthenticated. That's correct for kube-style probes inside the cluster, but if Cerebro is exposed to the internet (the same hardening this PR targets), they're DoS-amplification candidates — /readyz in particular still calls storage().ready(), which on the SurrealDB backend issues RETURN 1; over the connection. Not a blocker, but document the assumption (probes are network-policy-restricted) in the operator docs, or layer a tower::limit::ConcurrencyLimitLayer / per-route rate limiter on /readyz before exposing publicly.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clients/cerebro/src/server.rs` around lines 53 - 90, The router exposes
unauthenticated probes (handle_health, handle_ready) and while
DefaultBodyLimit::max(MAX_REQUEST_BODY_BYTES) is fine for /mcp, you should
mitigate DoS amplification by adding a per-route concurrency or rate limit for
the probes and/or documenting the network-policy assumption; update router() to
apply a tower::limit::ConcurrencyLimitLayer or a per-route rate limiter to the
GET("/readyz") (and optionally GET("/healthz")) and ensure the operator docs
mention probes must be network-policy-restricted because handle_ready calls
storage().ready().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@clients/cerebro/src/config.rs`:
- Around line 179-184: The is_demo_credential function currently treats "root"
as a demo credential which overlaps with the explicit username == "root" &&
password == "root" check elsewhere; either remove "root" from is_demo_credential
to limit it to placeholder values ("local-dev-only",
"CHANGE_ME_BEFORE_PRODUCTION") or update non_demo_secret consumers to include a
clearer diagnostic when a password matches a known default. Modify
is_demo_credential (and any callers) to only match placeholder strings or
augment non_demo_secret/validate_startup_requirements to append a hint like
"password matches known demo/default value" when rejecting credentials so
operators see why their "root" password was flagged.
- Around line 304-333: The block in validate_startup_requirements that re-checks
EmbeddedSurreal credentials is redundant because validate_storage (via
validate_embedded_surreal) already enforces non-demo credentials and rejects
"root", so remove the entire if !is_loopback_host(&self.host) &&
self.storage_mode == StorageMode::EmbeddedSurreal { ... } block from
validate_startup_requirements to keep a single source of truth; ensure existing
tests cover validate_embedded_surreal and validate_startup_requirements
semantics and, if you prefer the alternative behavior (allow demo creds on
loopback but not on non-loopback), instead relax validate_embedded_surreal and
add a clarifying comment in validate_startup_requirements explaining the
asymmetry and add tests for both loopback and non-loopback cases.
- Around line 168-177: normalize_secret currently always allocates a new
SecretString even when trimming doesn't change the value; change it to borrow
the exposed secret (via secret.expose_secret()), compute trimmed, return None if
trimmed.is_empty(), return the original SecretString (the incoming secret) when
trimmed == exposed to avoid reallocating, and only construct a new SecretString
when trimmed differs; ensure you drop the borrow from expose_secret() before
moving/returning the original secret (explicitly drop the borrowed &str or scope
it) so the move is legal; update normalize_secret and its callers (e.g.,
apply_env_overrides) accordingly.

In `@clients/cerebro/src/server.rs`:
- Around line 267-285: The parse_bearer_token function performs redundant
trimming and uses a Unicode whitespace split; replace the dead
trim_start_matches call with a single rest.trim() and change
split_once(char::is_whitespace) to split_once(char::is_ascii_whitespace) so the
split and trimming are RFC-7230 compliant; update references in
parse_bearer_token (auth_header, scheme, rest, token) accordingly and keep the
same Unauthorized error handling for malformed headers.
- Around line 132-149: The current request_kind value records which tokens are
accepted (self.config.audit_token) rather than which token was actually used;
change this by leaving auth_mode unset when creating the tracing span and
instead record the per-request value after authorize() completes: create the
span with auth_mode empty (or a placeholder), call authorize() to obtain
auth_context, then call span.record("auth_mode", if auth_context.is_audit {
"audit" } else { "operator" }) before exiting the request handler so logs show
the actual token type used; reference the existing span, request_id, tool_name,
authorize(), and auth_context.is_audit symbols when implementing this.
- Around line 144-184: The span with request_id/tool_name/auth_mode is only
applied to self.tools.handle(...) so early logs (authorize failure,
event_bus.publish, final success/failure logs) lack those fields; instead attach
the span to the entire async block assigned to response (i.e., instrument the
async { ... } future that contains authorize, event_bus.publish, and the match)
so every tracing! call inside (tracing::warn! in authorize error path,
event_bus.publish-related logs, and the final tracing::info!/tracing::warn!)
inherit the span; update the use/import to the appropriate Instrument trait
(tracing_futures::Instrument or tracing::Instrument) if required.

In `@clients/cerebro/src/storage/surreal.rs`:
- Around line 477-481: The readiness probe in ready() currently only awaits
self.db.query("RETURN 1;") which misses per-statement failures; change the call
to await the query, then call .check() on the returned Response and propagate
any error into CerebroError::Storage (e.g., await self.db.query("RETURN 1;"),
then .map(|resp| resp.check()).map_err(|err| CerebroError::Storage(...)) or
explicitly match the response and convert resp.check() failure into the same
error format) so that execution errors on the statement cause ready() to fail.

In `@clients/cerebro/tests/health_endpoints_test.rs`:
- Around line 108-128: Update the test
readyz_returns_service_unavailable_when_storage_readiness_fails to also parse
and assert the JSON response body returned by handle_ready: after receiving the
response from service.router().oneshot(...), read and deserialize the body to
ensure it contains {"status":"not_ready","error":<string>} (assert that status
== "not_ready" and that error is a non-empty string); locate the test function
name readyz_returns_service_unavailable_when_storage_readiness_fails and the
handler handle_ready in server.rs to mirror the expected payload shape, and use
the same request setup (CerebroService::new with FailingReadyStorage and
router()) when adding these assertions.

In `@clients/cerebro/tests/storage_config_test.rs`:
- Around line 161-173: The test
startup_validation_rejects_whitespace_only_auth_token_for_non_loopback can be
polluted by ambient CEREBRO_AUTH_TOKEN/CEREBRO_AUDIT_TOKEN because it calls
apply_env_overrides(); fix it by acquiring ENV_LOCK and using EnvVarGuard to
clear or unset both "CEREBRO_AUTH_TOKEN" and "CEREBRO_AUDIT_TOKEN" for the
duration of the test before calling apply_env_overrides(), restoring them on
drop; if EnvVarGuard lacks an unset helper, add EnvVarGuard::unset(key) that
records the previous value and removes the var so the test reliably verifies
whitespace-only tokens in validate_startup_requirements().

---

Outside diff comments:
In `@clients/cerebro/src/server.rs`:
- Around line 53-90: The router exposes unauthenticated probes (handle_health,
handle_ready) and while DefaultBodyLimit::max(MAX_REQUEST_BODY_BYTES) is fine
for /mcp, you should mitigate DoS amplification by adding a per-route
concurrency or rate limit for the probes and/or documenting the network-policy
assumption; update router() to apply a tower::limit::ConcurrencyLimitLayer or a
per-route rate limiter to the GET("/readyz") (and optionally GET("/healthz"))
and ensure the operator docs mention probes must be network-policy-restricted
because handle_ready calls storage().ready().
🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f46a22d4-b595-4cbb-8173-c3bfb994d4c3

📥 Commits

Reviewing files that changed from the base of the PR and between 810f5ae and 47f0e36.

⛔ Files ignored due to path filters (2)
  • clients/agent-runtime/Cargo.lock is excluded by !**/*.lock
  • clients/cerebro/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (14)
  • .github/workflows/dependency-review.yml
  • .github/workflows/pull-request-check.yml
  • clients/cerebro/Cargo.toml
  • clients/cerebro/Dockerfile.release-prebuilt
  • clients/cerebro/README.md
  • clients/cerebro/src/config.rs
  • clients/cerebro/src/server.rs
  • clients/cerebro/src/storage/mod.rs
  • clients/cerebro/src/storage/surreal.rs
  • clients/cerebro/tests/config_load_test.rs
  • clients/cerebro/tests/health_endpoints_test.rs
  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/tests/storage_config_test.rs
  • clients/cerebro/tests/tui_event_emission_tests.rs
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: sonar
  • GitHub Check: pr-checks
  • GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs

⚙️ CodeRabbit configuration file

**/*.rs: Focus on Rust idioms, memory safety, and ownership/borrowing correctness.
Flag unnecessary clones, unchecked panics in production paths, and weak error context.
Prioritize unsafe blocks, FFI boundaries, concurrency races, and secret handling.

Files:

  • clients/cerebro/src/storage/mod.rs
  • clients/cerebro/tests/tui_event_emission_tests.rs
  • clients/cerebro/tests/config_load_test.rs
  • clients/cerebro/src/storage/surreal.rs
  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/tests/storage_config_test.rs
  • clients/cerebro/tests/health_endpoints_test.rs
  • clients/cerebro/src/config.rs
  • clients/cerebro/src/server.rs
**/*

⚙️ CodeRabbit configuration file

**/*: Security first, performance second.
Validate input boundaries, auth/authz implications, and secret management.
Look for behavioral regressions, missing tests, and contract breaks across modules.

Files:

  • clients/cerebro/src/storage/mod.rs
  • clients/cerebro/tests/tui_event_emission_tests.rs
  • clients/cerebro/tests/config_load_test.rs
  • clients/cerebro/Cargo.toml
  • clients/cerebro/src/storage/surreal.rs
  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/tests/storage_config_test.rs
  • clients/cerebro/Dockerfile.release-prebuilt
  • clients/cerebro/tests/health_endpoints_test.rs
  • clients/cerebro/src/config.rs
  • clients/cerebro/README.md
  • clients/cerebro/src/server.rs
**/*.{md,mdx}

⚙️ CodeRabbit configuration file

**/*.{md,mdx}: Verify technical accuracy and that docs stay aligned with code changes.
For user-facing docs, check EN/ES parity or explicitly note pending translation gaps.

Files:

  • clients/cerebro/README.md
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/*.rs : Run `cargo fmt --all -- --check`, `cargo clippy --all-targets -- -D warnings`, and `cargo test` for code validation, or document which checks were skipped and why

Applied to files:

  • .github/workflows/pull-request-check.yml
  • clients/cerebro/tests/config_load_test.rs
  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/tests/storage_config_test.rs
  • clients/cerebro/tests/health_endpoints_test.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/.github/**/*.{yml,yaml} : For workflow/template-only changes, ensure YAML/template syntax validity

Applied to files:

  • .github/workflows/pull-request-check.yml
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Never log secrets, tokens, raw credentials, or sensitive payloads in any logging statements

Applied to files:

  • clients/cerebro/tests/config_load_test.rs
  • clients/cerebro/src/config.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools,config}/**/*.rs : Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable

Applied to files:

  • clients/cerebro/tests/config_load_test.rs
  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/tests/storage_config_test.rs
  • clients/cerebro/Dockerfile.release-prebuilt
  • clients/cerebro/README.md
  • clients/cerebro/src/server.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/main.rs : Preserve CLI contract unless change is intentional and documented; prefer explicit errors over silent fallback for unsupported critical paths

Applied to files:

  • clients/cerebro/tests/config_load_test.rs
  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/README.md
  • clients/cerebro/src/server.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Do not add heavy dependencies for minor convenience; justify new crate additions

Applied to files:

  • clients/cerebro/Cargo.toml
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/**/Cargo.toml : Preserve release-size profile assumptions in `Cargo.toml` and avoid adding heavy dependencies unless clearly justified

Applied to files:

  • clients/cerebro/Cargo.toml
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/channels/**/*.rs : Implement `Channel` trait in `src/channels/` with consistent `send`, `listen`, and `health_check` semantics and cover auth/allowlist/health behavior with tests

Applied to files:

  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/tests/storage_config_test.rs
  • clients/cerebro/tests/health_endpoints_test.rs
  • clients/cerebro/src/server.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/{security,gateway,tools}/**/*.rs : Treat `src/security/`, `src/gateway/`, `src/tools/` as high-risk surfaces and never broaden filesystem/network execution scope without explicit policy checks

Applied to files:

  • clients/cerebro/tests/mcp_auth_policy.rs
  • clients/cerebro/src/server.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/**/*.rs : Avoid unnecessary allocations, clones, and blocking operations to maintain performance and efficiency

Applied to files:

  • clients/cerebro/src/server.rs
📚 Learning: 2026-02-17T12:31:17.076Z
Learnt from: CR
Repo: dallay/corvus PR: 0
File: clients/agent-runtime/AGENTS.md:0-0
Timestamp: 2026-02-17T12:31:17.076Z
Learning: Applies to clients/agent-runtime/src/tools/**/*.rs : Implement `Tool` trait in `src/tools/` with strict parameter schema, validate and sanitize all inputs, and return structured `ToolResult` without panics in runtime path

Applied to files:

  • clients/cerebro/src/server.rs
🔇 Additional comments (14)
.github/workflows/dependency-review.yml (1)

24-24: LGTM, though fetch-depth: 0 is overkill here.

actions/dependency-review-action only needs base/head refs on pull_request events, so full history isn't required — but it's harmless aside from a slight checkout cost. Happy to leave as-is.

.github/workflows/pull-request-check.yml (1)

135-145: Quality gates now complete for Cerebro — nicely done. 🎯

fmt + clippy (-D warnings) + check + test (both --locked) line up with #674 and the agent-runtime conventions.

One tiny nit (entirely optional): cargo clippy --all-targets already type-checks everything cargo check would, so step at line 141-142 is largely redundant on green builds. You could drop it to shave ~CI seconds, or keep it as a fast-fail pre-clippy guard — your call.

clients/cerebro/src/storage/mod.rs (1)

83-85: LGTM — sensible default for Storage::ready.

A trait-level Ok(()) default keeps the existing InMemoryStorage and DiskBackedStorage behaviorally always-ready, while letting SurrealStorage opt into a real probe. Symmetry with count() etc. is fine.

clients/cerebro/README.md (1)

37-83: LGTM — README accurately mirrors the new startup/probe/limits behavior.

The explicit 1 MiB body limit, the fail-fast statement for non-loopback bindings without auth_token, the demo-credential denylist (local-dev-only, CHANGE_ME_BEFORE_PRODUCTION, root), and the /healthz//readyz probe semantics all match the implementation in server.rs / config.rs and the tests. Prior feedback on this section is resolved.

As per coding guidelines: "Verify technical accuracy and that docs stay aligned with code changes."

clients/cerebro/Cargo.toml (1)

33-35: LGTM — dependency additions are tightly scoped and justified.

subtle underpins constant-time token comparison in server.rs, the serde feature on toml is required for typed config loading (issue #669), and tower util is dev-only for ServiceExt in health endpoint tests. No heavy footprint added.

Also applies to: 52-52

clients/cerebro/tests/tui_event_emission_tests.rs (1)

28-33: LGTM — payload-level assertions match the publishing flow.

Asserted fields line up with handle_json_rpc Started (L164-178) and Finished (L195-205) in server.rs. Coverage is asymmetric (Started's status: "started" isn't asserted), but that's a stylistic gap, not a correctness one.

clients/cerebro/tests/config_load_test.rs (1)

1-43: LGTM — exact-value secret assertions close the prior gap.

ExposeSecret-based comparisons exercise secret_string_opt::deserialize end-to-end and would catch silent value drops or sentinel substitutions. Past feedback addressed.

clients/cerebro/Dockerfile.release-prebuilt (1)

18-31: LGTM — secure-by-default posture is now both documented and enforced.

The loopback-bind caveat is called out inline (L18-21), and per the README the startup validator now rejects the bundled local-dev-only / root creds on non-loopback binds, closing the prior soft-footgun. Defaults are demo-only as intended.

Based on learnings: "Do not silently weaken security policy or access constraints; keep default behavior secure-by-default with deny-by-default where applicable."

clients/cerebro/tests/mcp_auth_policy.rs (1)

97-151: LGTM — auth-policy coverage now matches RFC 7235 and #676.

accepts_bearer_token_with_lowercase_prefix resolves the earlier RFC-compliance concern by accepting case-insensitive Bearer, and the new audit-token success test plus the trailing-whitespace rejection case round out the valid/invalid/malformed matrix. Prior feedback addressed.

clients/cerebro/tests/storage_config_test.rs (1)

127-159: LGTM — both new pass-paths exercise the validator end-to-end.

startup_validation_allows_non_loopback_with_real_auth_token correctly upgrades the SurrealDB credentials to non-demo values (operator / secure-password), which is necessary because validate_startup_requirements also gates on non_demo_secret for non-loopback embedded startup — without that, the test would have been masking the auth-token check behind a credential failure. Loopback default-pass test at 140–144 is the right negative control.

clients/cerebro/tests/health_endpoints_test.rs (1)

12-62: Lock down count() to also be unreachable — nice belt-and-braces against regression.

Wiring count() to unreachable!() is exactly what makes this test useful: if someone reverts handle_ready to the previous O(rows) storage.count() implementation (the prior critical concern), this test panics instead of silently passing. Good defensive contract test.

clients/cerebro/src/config.rs (1)

429-442: LGTM — secret trimming is now applied at deserialization time, closing the file-vs-env divergence.

This addresses the prior concern that file-loaded tokens were stored verbatim while env-loaded tokens were trimmed by apply_env_overridesAuthorization: Bearer " secret " from a TOML file would have passed startup validation but failed (or matched the wrong canonical form against) the request-time bearer comparison. Now both ingress paths produce the same canonical secret. Good fix.

clients/cerebro/src/server.rs (2)

237-264: Constant-time comparison + always-parse: looks good; one timing nit on the audit-vs-auth ordering.

Both ct_eq calls compare bytes in constant time, and parse_bearer_token runs unconditionally so that "no token configured" and "wrong scheme" don't diverge in timing. One subtle observation: when audit_token is configured, a winning operator token does one extra ct_eq versus a winning audit token. An attacker who already holds a valid token could in principle distinguish "I'm authenticated as audit" vs "I'm authenticated as operator" via timing — but they already know that from the response/permissions, so this is essentially non-exploitable. Calling it out only so it doesn't surprise a future auditor.

Also: per subtle::ConstantTimeEq for [u8], slices of unequal length short-circuit to Choice(0) — that's the intended behavior (length is not secret in HTTP), but worth noting if you ever want hardened-equal-length comparison.


306-321: LGTM — readiness now delegates to a cheap backend probe and locks the contract.

Switching /readyz from storage().count() to storage().ready() resolves the prior critical concern about probe-induced full table scans on SurrealDB. Pairing this with FailingReadyStorage in the integration tests covers both branches. Returning a structured not_ready body with the underlying error string is operator-friendly without leaking secrets (the error variants here are storage/IO messages, not token material).

Comment thread clients/cerebro/src/config.rs
Comment thread clients/cerebro/src/config.rs
Comment thread clients/cerebro/src/config.rs
Comment thread clients/cerebro/src/server.rs
Comment thread clients/cerebro/src/server.rs Outdated
Comment thread clients/cerebro/src/server.rs
Comment thread clients/cerebro/src/storage/surreal.rs
Comment thread clients/cerebro/tests/health_endpoints_test.rs
Comment thread clients/cerebro/tests/storage_config_test.rs
@sonarqubecloud
Copy link
Copy Markdown

@yacosta738 yacosta738 merged commit 725b3ea into main Apr 26, 2026
19 checks passed
@yacosta738 yacosta738 deleted the cerebro branch April 26, 2026 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment