feat(mcp): standalone MCP server on the hexagonal stack#433
Conversation
Reimplement PR 273's MCP server (search / catalog / indexation tools) on the hexagonal architecture. All tool logic lives in services.orchestrators.MCPService, which composes the Phase-8 orchestrators plus the VectorStore port and stays Ray-free; the api/mcp server is a thin FastMCP entrypoint that resolves the bearer principal in middleware and forwards the partition ACL explicitly. Hardening over the original: - index_url now requires editor access and validates the URL through an SSRF guard with per-redirect-hop re-validation, plus size and timeout ceilings on the download - url_safety also rejects hostnames that resolve to private / loopback / reserved / cloud-metadata addresses (DNS-based SSRF) - caller-supplied metadata is stripped of server-managed keys on writes - get_task_logs caps max_lines; get_file_info reports an exact chunk count and no longer leaks the embedding vector - search file_id is validated and inlined as a quoted Milvus literal Config lives in core/config/mcp.py (OPENRAG_MCP_* env overrides); the service is wired as a lazy container property. log_tail is extracted to core/utils so the admin task-logs route and the MCP tool share it.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR introduces a complete standalone MCP (Model Context Protocol) HTTP server for OpenRAG, exposing document search, catalog browsing, task introspection, and write operations through FastMCP. The implementation includes request-scoped authentication via bearer tokens (with dev bypass), per-request partition ACL enforcement, SSRF-protected URL indexing with strict download guards, and refactored shared log-tail utilities. A companion Milvus vector store fix corrects primary-key field extraction. ChangesMCP Standalone Server
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.env.example (1)
87-95: 💤 Low valueOptionally document the download-bound env vars here too.
For parity with
MCPServerConfig, consider adding commentedOPENRAG_MCP_DOWNLOAD_TIMEOUTandOPENRAG_MCP_MAX_DOWNLOAD_BYTESplaceholders alongside the other MCP vars.📝 Proposed addition
# OPENRAG_MCP_SIMILARITY_THRESHOLD=0.8 +# OPENRAG_MCP_DOWNLOAD_TIMEOUT=30.0 +# OPENRAG_MCP_MAX_DOWNLOAD_BYTES=104857600 # 100 MiB🤖 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 @.env.example around lines 87 - 95, Add commented placeholders for the download-related environment variables to match MCPServerConfig by adding OPENRAG_MCP_DOWNLOAD_TIMEOUT and OPENRAG_MCP_MAX_DOWNLOAD_BYTES alongside the existing MCP vars; update the .env.example block where the MCP settings are defined so it includes commented lines for OPENRAG_MCP_DOWNLOAD_TIMEOUT (e.g., seconds) and OPENRAG_MCP_MAX_DOWNLOAD_BYTES (e.g., bytes) with brief inline hints, ensuring the names exactly match the config keys referenced in MCPServerConfig.conf/config.yaml (1)
324-335: 💤 Low valueSurface the SSRF/DoS download bounds in the config section.
mcp.download_timeoutandmcp.max_download_bytesare overridable viaOPENRAG_MCP_DOWNLOAD_TIMEOUT/OPENRAG_MCP_MAX_DOWNLOAD_BYTES(loader.py Lines 180-181), but neither the keys nor their env vars appear here. Since these are security/DoS hardening knobs, documenting them improves operator visibility.📝 Proposed addition
# Env: OPENRAG_MCP_SERVER_NAME, OPENRAG_MCP_HOST, OPENRAG_MCP_PORT, # OPENRAG_MCP_PATH, OPENRAG_MCP_DEFAULT_TOP_K, OPENRAG_MCP_MAX_TOP_K, -# OPENRAG_MCP_SIMILARITY_THRESHOLD +# OPENRAG_MCP_SIMILARITY_THRESHOLD, OPENRAG_MCP_DOWNLOAD_TIMEOUT, +# OPENRAG_MCP_MAX_DOWNLOAD_BYTES mcp: server_name: OpenRAG MCP host: 0.0.0.0 port: 8081 path: /mcp default_top_k: 5 max_top_k: 50 similarity_threshold: 0.8 + download_timeout: 30.0 + max_download_bytes: 104857600 # 100 MiB🤖 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 `@conf/config.yaml` around lines 324 - 335, The mcp config section is missing the SSRF/DoS hardening keys; add mcp.download_timeout and mcp.max_download_bytes (and document their corresponding env vars OPENRAG_MCP_DOWNLOAD_TIMEOUT and OPENRAG_MCP_MAX_DOWNLOAD_BYTES) to conf/config.yaml so operators can override the downloader limits referenced in loader.py (mcp.download_timeout, mcp.max_download_bytes). Include sensible default values and brief descriptions next to each key so the config reflects the runtime behavior in loader.py and makes the security knobs discoverable.
🤖 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 `@pyproject.toml`:
- Line 60: Update the declared mcp minimum in pyproject.toml from "mcp>=1.11.0"
to "mcp>=1.23.0" (or a later safe version) to include the DNS-rebinding fix,
then regenerate the repository lockfile (uv.lock) so the pinned mcp entry is
updated; ensure you run the project’s dependency lock/update command (e.g.,
poetry lock / pip-compile / whatever tooling the repo uses) and commit the
updated lockfile, then run the test suite to verify nothing else breaks (refer
to pyproject.toml and uv.lock and the FastMCP usage in openrag/api/mcp/server.py
and config in openrag/core/config/mcp.py while making the change).
---
Nitpick comments:
In @.env.example:
- Around line 87-95: Add commented placeholders for the download-related
environment variables to match MCPServerConfig by adding
OPENRAG_MCP_DOWNLOAD_TIMEOUT and OPENRAG_MCP_MAX_DOWNLOAD_BYTES alongside the
existing MCP vars; update the .env.example block where the MCP settings are
defined so it includes commented lines for OPENRAG_MCP_DOWNLOAD_TIMEOUT (e.g.,
seconds) and OPENRAG_MCP_MAX_DOWNLOAD_BYTES (e.g., bytes) with brief inline
hints, ensuring the names exactly match the config keys referenced in
MCPServerConfig.
In `@conf/config.yaml`:
- Around line 324-335: The mcp config section is missing the SSRF/DoS hardening
keys; add mcp.download_timeout and mcp.max_download_bytes (and document their
corresponding env vars OPENRAG_MCP_DOWNLOAD_TIMEOUT and
OPENRAG_MCP_MAX_DOWNLOAD_BYTES) to conf/config.yaml so operators can override
the downloader limits referenced in loader.py (mcp.download_timeout,
mcp.max_download_bytes). Include sensible default values and brief descriptions
next to each key so the config reflects the runtime behavior in loader.py and
makes the security knobs discoverable.
🪄 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: CHILL
Plan: Pro
Run ID: e17fdb2a-1407-475c-871f-68bed7442c80
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.env.exampleconf/config.yamlopenrag/api/mcp/__init__.pyopenrag/api/mcp/auth_context.pyopenrag/api/mcp/server.pyopenrag/api/mcp/test_server.pyopenrag/api/routers/admin/task_logs.pyopenrag/core/config/loader.pyopenrag/core/config/mcp.pyopenrag/core/config/root.pyopenrag/core/utils/log_tail.pyopenrag/core/utils/test_log_tail.pyopenrag/core/utils/test_url_safety.pyopenrag/core/utils/url_safety.pyopenrag/di/container.pyopenrag/di/providers.pyopenrag/di/test_container.pyopenrag/services/orchestrators/mcp_service.pyopenrag/services/orchestrators/test_mcp_service.pypyproject.toml
| "cryptography>=42", | ||
| "tenacity>=8.2.0", | ||
| "aiobreaker>=1.2.0", | ||
| "mcp>=1.11.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Latest mcp release on PyPI
curl -s https://pypi.org/pypi/mcp/json | jq '.info.version'
# Security advisories for the mcp PIP package
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: PIP, package: "mcp") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}'Repository: linagora/openrag
Length of output: 888
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the MCP server implementation
fd -a "server.py" api/mcp 2>/dev/null || true
ls -la api/mcp 2>/dev/null || true
# If api/mcp/server.py exists, print relevant parts
if [ -f "api/mcp/server.py" ]; then
echo "=== api/mcp/server.py (head) ==="
sed -n '1,200p' api/mcp/server.py
echo "=== api/mcp/server.py (tail) ==="
sed -n '200,400p' api/mcp/server.py
fi
# Confirm how mcp/FastMCP is declared in pyproject.toml
rg -n "mcp\s*[\>\=<!~]" pyproject.toml || true
sed -n '1,120p' pyproject.toml || trueRepository: linagora/openrag
Length of output: 2752
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# List likely MCP directories
echo "== Top-level dirs containing 'mcp' =="
fd -t d -a "mcp" . | head -n 50 || true
# Find actual server implementation path(s)
echo "== Files named server.py under api/ =="
fd -a "server.py" api | head -n 50 || true
# Search for FastMCP / streamable HTTP usage
echo "== References to FastMCP =="
rg -n --hidden --glob '!.venv/**' "FastMCP" . || true
echo "== References to streamable HTTP transport =="
rg -n --hidden --glob '!.venv/**' -i "streamable.*http|streamable.*transport|StreamableHTTP|streamable_http" . || true
echo "== References to MCP SDK =="
rg -n --hidden --glob '!.venv/**' -i "mcp\." . || true
# Check for lockfiles that might pin mcp
echo "== Potential lockfiles =="
fd -a -t f -g "*lock*" . | head -n 200 || true
fd -a -t f -g "*requirements*.txt" . | head -n 50 || true
fd -a -t f -g "*constraints*.txt" . | head -n 50 || true
# If any lock file exists, show lines mentioning mcp
for f in $(fd -a -t f -g "*lock*" . | head -n 50); do
if rg -q "mcp\s*[<>=]" "$f" 2>/dev/null; then
echo "== mcp pins in $f =="
rg -n "mcp\s*[<>=]" "$f" || true
fi
doneRepository: linagora/openrag
Length of output: 2916
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect FastMCP usage in server.py around initialization and streamable_http_app
if [ -f "openrag/api/mcp/server.py" ]; then
rg -n "FastMCP\(|stateless_http|streamable_http_app|streamable_http_path|json_response" openrag/api/mcp/server.py
echo "=== server.py: lines 1-120 ==="
sed -n '1,120p' openrag/api/mcp/server.py
echo "=== server.py: lines 420-520 ==="
sed -n '420,520p' openrag/api/mcp/server.py
fi
# Search for how mcp is pinned in lockfiles/requirements
for f in uv.lock **/*uv.lock 2>/dev/null; do
if [ -f "$f" ]; then
echo "== Searching for mcp in $f =="
rg -n "mcp" "$f" || true
fi
done
# Also check other lock/constraints files listed earlier
for f in uv.lock package-lock.json charts/openrag-stack/Chart.lock benchmarks/prompt_eval/uv.lock benchmarks/workspace/requirements.txt utility/requirements.txt; do
if [ -f "$f" ]; then
echo "== Searching for mcp in $f =="
rg -n "mcp" "$f" || true
fi
doneRepository: linagora/openrag
Length of output: 6269
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate uv lockfiles =="
mapfile -t locks < <(fd -t f -a "uv.lock" . || true)
printf '%s\n' "${locks[@]}" || true
if [ "${`#locks`[@]}" -eq 0 ]; then
echo "No uv.lock files found."
exit 0
fi
for f in "${locks[@]}"; do
echo "== Search mcp in $f =="
# Try to find pinned version entries for mcp
rg -n --hidden --glob '!.venv/**' -i "mcp" "$f" | head -n 80 || true
doneRepository: linagora/openrag
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate lock files (by name) =="
fd -t f -a 'uv.lock' . || true
fd -t f -a 'requirements.txt' . || true
fd -t f -a 'constraints.txt' . || true
fd -t f -a '*lock*' . || true
echo "== mcp pins in uv.lock (if present) =="
for f in $(fd -t f -a 'uv.lock' .); do
echo "-- $f --"
rg -n --hidden --glob '!.venv/**' -i 'mcp' "$f" | head -n 80 || true
done
echo "== mcp pins in common requirements files =="
for f in $(fd -t f -a 'requirements*.txt' .); do
if rg -q -i 'mcp' "$f" 2>/dev/null; then
echo "-- $f --"
rg -n -i 'mcp' "$f" || true
fi
doneRepository: linagora/openrag
Length of output: 2206
🌐 Web query:
mcp 1.23.0 DNS rebinding protection not enabled by default FastMCP
💡 Result:
Prior to version 1.23.0, the Model Context Protocol (MCP) Python SDK did not enable DNS rebinding protection by default for HTTP-based servers [1][2][3]. This vulnerability (CVE-2025-66416) meant that HTTP-based MCP servers using FastMCP with streamable HTTP or SSE transport were susceptible to DNS rebinding attacks if run on localhost without authentication and without specific transport security settings configured [1][2][4]. In version 1.23.0 and later, this behavior was corrected. FastMCP servers now automatically enable DNS rebinding protection by default when the host parameter is set to 127.0.0.1 or localhost [1][5][3]. This protection validates the Host header of incoming requests to ensure they originate from trusted sources, effectively mitigating the risk of malicious websites bypassing same-origin policy restrictions [1][4][3]. Users who require specific configurations, such as binding to all interfaces (0.0.0.0) or using custom hostnames, may find this protection is still applied based on the server's initialization parameters [6]. If automatic protection interferes with legitimate use cases, it can be managed by explicitly configuring TransportSecuritySettings or, in some cases, adjusting the host parameter during server instantiation to ensure the SDK correctly identifies the deployment environment [7][6].
Citations:
- 1: GHSA-9h52-p55h-vw2f
- 2: https://nvd.nist.gov/vuln/detail/CVE-2025-66416
- 3: https://cve.imfht.com/detail/CVE-2025-66416
- 4: https://advisories.gitlab.com/pypi/mcp/CVE-2025-66416/
- 5: https://test.osv.dev/vulnerability/GHSA-9h52-p55h-vw2f
- 6: FastMCP DNS rebinding protection always enabled due to late host assignment getzep/graphiti#1205
- 7: https://dev.to/jonathanmeltonfusional/i-gave-gemma-4-150-tools-on-windows-heres-what-actually-happened-1njg
🌐 Web query:
mcp 1.11.0 FastMCP stateless_http streamable_http_app streamable_http_path json_response
💡 Result:
In the Model Context Protocol (MCP) Python SDK (including version 1.11.0 and associated FastMCP implementations), these parameters are central to configuring Streamable HTTP transport for production and scalable deployments [1][2]. streamable_http_app This method returns a mountable ASGI application (compatible with frameworks like Starlette or FastAPI) that enables a FastMCP server to operate over Streamable HTTP [3][4][2]. It allows developers to integrate MCP functionality into existing web applications by mounting it to a specific path [2]. streamable_http_path This parameter defines the specific URL path (e.g., "/mcp") where the Streamable HTTP transport will listen for client connections [3][4][2]. Accurate path configuration is essential, as clients must connect to this exact endpoint to successfully perform the initial handshake [5]. stateless_http When set to True, this mode enables horizontal scalability by removing the need for persistent session affinity on the server [6][7]. In stateless mode, each HTTP request independently manages its own transport context [6]. This is highly recommended for production environments—such as cloud-hosted API Gateways, Lambda functions, or load-balanced clusters—because it prevents issues like "session not found" errors when traffic is distributed across multiple server replicas [8][6][5]. Note that certain bidirectional features, such as LLM sampling or elicitation, may not be supported or may experience issues when stateless mode is enabled because they rely on long-lived, stateful connections [9]. json_response When set to True, this parameter forces the server to use a JSON response format for its communications [3][4]. This is often used alongside stateless_http to optimize the transport for modern web clients and infrastructure that prefer standard JSON-RPC data structures over alternative formats [2]. Implementation Note: In recent versions of FastMCP, configuring stateless_http directly in the FastMCP constructor has been deprecated to favor setting it at the application or transport level (e.g., in http_app or via environment variables like FASTMCP_STATELESS_HTTP) [10][11]. Always verify your specific SDK version's documentation, as the API surface for these configurations has evolved to separate server lifecycle management from transport configuration [10].
Citations:
- 1: https://dev.to/monuminu/model-context-protocol-mcp-the-complete-developer-guide-to-building-production-grade-ai-agents-ah3
- 2: https://github.com/modelcontextprotocol/python-sdk
- 3: https://github.com/jlowin/fastmcp/blob/main/src/fastmcp/server/http.py
- 4: https://gofastmcp.com/python-sdk/fastmcp-server-http
- 5: https://heeki.medium.com/building-an-mcp-server-as-an-api-developer-cfc162d06a83
- 6: https://gofastmcp.com/deployment/http
- 7: https://punkpeye-fastmcp.mintlify.app/guides/stateless-mode
- 8: https://docs.ray.io/en/latest/ray-overview/examples/mcp-ray-serve/01%20Deploy_custom_mcp_in_streamable_http_with_ray_serve.html
- 9: MCP server sampling call hangs when using stateless_http==True PrefectHQ/fastmcp#678
- 10: Error in documentation: need update PrefectHQ/fastmcp#3618
- 11: How to set log_level/stateless_http when mounting FastMCP (DeprecationWarning workaround)? PrefectHQ/fastmcp#996
🌐 Web query:
mcp 1.11.0 FastMCP streamable_http_app streamable_http_path stateless_http
💡 Result:
FastMCP + Streamable HTTP + Stateless (and related paths) in MCP Python SDK: 1) Where these settings live - The official MCP Python SDK (FastMCP is built on top of it) documents FastMCP’s Streamable HTTP transport configuration options including: - streamable_http_path: “Transport paths” - stateless_http: “Whether the server operates in stateless mode” [1] - The FastMCP HTTP server implementation exposes a Streamable HTTP ASGI app factory that takes (among other args) streamable_http_path, stateless_http, and json_response: “stateless_http: Whether to use stateless mode (new transport per request)” [2]. 2) streamable_http_app - The SDK/docs state you can “mount the StreamableHTTP server to an existing ASGI server using the streamable_http_app method” [1]. - Example mounting into a Starlette app: - Mount("/", app=mcp.streamable_http_app) [3] 3) streamable_http_path - The SDK docs show configuring streamable_http_path during FastMCP initialization, and that it controls where the transport endpoint is mounted relative to wherever you mount the returned ASGI app: - mcp_at_root = FastMCP("My Server", json_response=True, streamable_http_path="/") [3] - Mount at /process then results in the Streamable HTTP endpoint being under /process instead of /process/mcp [3] 4) stateless_http - The SDK docs explicitly recommend the production setup: - “Use stateless_http=True and json_response=True for optimal scalability.” [1] - The docs also list stateless server examples (SSE and JSON) and describe that Streamable HTTP supports both stateful and stateless operation modes [1]. - There is a merged PR adding “stateless server support” for StreamableHttp transport where “Each request is independent and does not require initialization.” [4] 5) Putting it together (typical FastMCP usage) - Create a FastMCP server configured for stateless Streamable HTTP with JSON responses: - mcp = FastMCP("StatelessServer", stateless_http=True, json_response=True) [1] - Mount it into an existing ASGI app using streamable_http_app: - Mount("/", app=mcp.streamable_http_app) [3] - Optionally set streamable_http_path on initialization to change the transport endpoint path behavior [3][1]. Answer to the query terms (summary) - mcp 1.11.0: Protocol SDK version exists; MCP Python SDK docs show relevant transport options in the FastMCP/Streamable HTTP documentation (the exact “1.11.0” protocol language is not uniquely tied to the HTTP option names in the sources we found, but these options are documented in the SDK’s Streamable HTTP configuration) [1][2]. - FastMCP: FastMCP provides the Streamable HTTP ASGI mounting via streamable_http_app and supports stateless_http + json_response for production scalability [1][3][2]. - streamable_http_app: Method to integrate the Streamable HTTP server with an existing ASGI app via mounting [1]. - streamable_http_path: Initialization parameter controlling transport endpoint paths [1][3]. - stateless_http: Initialization parameter enabling stateless mode; recommended with json_response=True for scalability [1][2].
Citations:
- 1: http://github.com/modelcontextprotocol/python-sdk?tab=readme-ov-file
- 2: https://github.com/jlowin/fastmcp/blob/main/src/fastmcp/server/http.py
- 3: https://github.com/modelcontextprotocol/python-sdk
- 4: StreamableHttp - stateless server support modelcontextprotocol/python-sdk#554
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find MCP config defaults (host/path) and how they’re loaded
rg -n "OPENRAG_MCP_HOST|mcp\.host|mcp\.path|OPENRAG_MCP_PATH|class .*MCP|default.*mcp" openrag || true
# Look for any transport/security settings that might mitigate DNS rebinding
rg -n "TransportSecuritySettings|dns rebinding|rebind|Host header|host header|security.*settings" openrag || true
# Inspect the config model for mcp (if present)
fd -a "mcp.py" openrag/core/config openrag -d 4 2>/dev/null || true
for f in $(fd -a "mcp*.py" openrag/core/config -t f 2>/dev/null); do
echo "== $f =="
sed -n '1,220p' "$f" | head -n 220
doneRepository: linagora/openrag
Length of output: 1601
Bump mcp minimum version to pick up the DNS-rebinding fix (>=1.23.0)
- This repo declares
mcp>=1.11.0(anduv.lockpinsmcp-1.11.0), but PyPI latest is1.27.1. - Security advisory GHSA-9h52-p55h-vw2f / CVE-2025-66416: DNS rebinding protection is not enabled by default in
mcp< 1.23.0; fixed in 1.23.0. openrag/api/mcp/server.pyuses FastMCP Streamable-HTTP (FastMCP(..., stateless_http=True, json_response=True),streamable_http_app(),streamable_http_path=...), andopenrag/core/config/mcp.pydefaultshostto0.0.0.0, so the “localhost/127.0.0.1” auto-protection path won’t apply.
Change pyproject.toml to mcp>=1.23.0 (or higher) and update the lockfile accordingly.
🤖 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 `@pyproject.toml` at line 60, Update the declared mcp minimum in pyproject.toml
from "mcp>=1.11.0" to "mcp>=1.23.0" (or a later safe version) to include the
DNS-rebinding fix, then regenerate the repository lockfile (uv.lock) so the
pinned mcp entry is updated; ensure you run the project’s dependency lock/update
command (e.g., poetry lock / pip-compile / whatever tooling the repo uses) and
commit the updated lockfile, then run the test suite to verify nothing else
breaks (refer to pyproject.toml and uv.lock and the FastMCP usage in
openrag/api/mcp/server.py and config in openrag/core/config/mcp.py while making
the change).
Collapse the copy-pasted log handling the MCP work had introduced or inherited into one source of truth in core/utils/log_tail.py: - collect_task_logs + MAX_TASK_LOG_LINES move to core; the admin task-logs route re-exports them and MCPService calls the same function, dropping its private _read_task_logs/_MAX_LOG_LINES copy. The MCP tool now enforces the same 1..5000 bound as the admin route. - app_log_file() is the single resolver for the app.json path; the logger sink, admin indexing router and MCP server all use it instead of rebuilding "log_dir / app.json". Removed the dead LOG_FILE in api/dependencies/files.py. - conf/config.yaml mcp section now lists download_timeout and max_download_bytes (and their env vars), matching the all-fields convention every other section follows.
- Add the download_timeout / max_download_bytes env vars to the .env.example MCP block so it matches config.yaml and the loader. - Test that update_file_metadata enforces editor on the move destination for a non-wildcard caller (the existing test used the admin wildcard, which short-circuited the check). - Test that _safe_download rejects a public host that 302-redirects to a loopback target, covering the per-hop SSRF re-validation.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openrag/services/orchestrators/test_mcp_service.py (1)
13-13: ⚡ Quick winDeclare
httpxin test/dev dependency declarations (to avoid relying on transitive installs).
openrag/services/orchestrators/test_mcp_service.pyimportshttpxat line 13, butpyproject.toml’s[dependency-groups]doesn’t list it. Current CI is likely okay becauseapi_tests.ymlexplicitlypip installshttpxanduv.lockalready containshttpx, but making it explicit prevents future breakage if it stops being pulled transitively.🤖 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 `@openrag/services/orchestrators/test_mcp_service.py` at line 13, The test imports the httpx package (see import httpx in openrag/services/orchestrators/test_mcp_service.py) but httpx is not declared in the project's dev/test dependency groups; add httpx to the appropriate test/dev dependency declaration in pyproject.toml (e.g., under [project.optional-dependencies] or the [dependency-groups] test/dev group used by CI) so tests don't rely on transitive installs, and then update lockfile or run poetry/pip-tools to regenerate the lock so CI picks up the explicit dependency.
🤖 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.
Nitpick comments:
In `@openrag/services/orchestrators/test_mcp_service.py`:
- Line 13: The test imports the httpx package (see import httpx in
openrag/services/orchestrators/test_mcp_service.py) but httpx is not declared in
the project's dev/test dependency groups; add httpx to the appropriate test/dev
dependency declaration in pyproject.toml (e.g., under
[project.optional-dependencies] or the [dependency-groups] test/dev group used
by CI) so tests don't rely on transitive installs, and then update lockfile or
run poetry/pip-tools to regenerate the lock so CI picks up the explicit
dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e873f6e-3706-45e0-aaff-83e01c9d5a6d
📒 Files selected for processing (2)
.env.exampleopenrag/services/orchestrators/test_mcp_service.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .env.example
Milvus 2.6 exposes the auto-id INT64 primary key (field name _id) on each
search hit as hit["_id"] and inside the entity — never under the generic
"id" key. _parse_search_response read hit.get("id"), which was always None
and got stringified to the literal "None", so every search result carried
chunk_id "None" and could not be fed back into get_chunk_by_id. Read _id
(with an entity fallback) and emit null rather than "None" when absent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@openrag/services/storage/milvus_store.py`:
- Around line 686-693: In _parse_search_response, avoid calling hit.get("_id")
on non-dict hits by first checking isinstance(hit, dict) (same pattern used when
deriving entity); change the pk extraction to something like: set pk =
hit.get("_id") only if isinstance(hit, dict) else None, then fall back to
entity.get("_id") if entity is a dict, and finally set record["id"] =
self._milvus_id_to_str(pk) if pk is not None else None; update references to
hit, entity, pk, _parse_search_response, and _milvus_id_to_str accordingly so
non-dict hits no longer raise AttributeError and remain handled by the
surrounding _search_errors logic.
🪄 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: CHILL
Plan: Pro
Run ID: fde76cf6-734c-4832-96b4-0ad2f636944a
📒 Files selected for processing (2)
openrag/services/storage/milvus_store.pyopenrag/services/storage/test_milvus_store.py
| # The primary key field is named ``_id`` (auto_id INT64), so Milvus | ||
| # exposes it on the hit as ``_id`` and also inside ``entity`` — NOT | ||
| # under the generic ``id`` key. Reading ``id`` yielded a literal | ||
| # "None" string for every search result. | ||
| pk = hit.get("_id") | ||
| if pk is None: | ||
| pk = entity.get("_id") | ||
| record["id"] = self._milvus_id_to_str(pk) if pk is not None else None |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
pymilvus 2.6 AsyncMilvusClient search response hit format dict id entity primary key
💡 Result:
In PyMilvus 2.6, the AsyncMilvusClient uses the same search method signature as the synchronous MilvusClient, and it returns a list of result sets (one per query vector) [1][2][3]. The search response structure is a list-like object containing search results. When you inspect an individual hit within these results, it is formatted as a dictionary containing the primary key (often named 'id' or your defined primary key field), the distance (or score), and any requested output fields [4][5]. Key structural details for search hits: 1. Result Set: The search method returns a list of hits. Each item in this top-level list corresponds to one of the input query vectors [6][2]. 2. Hit Format: When iterating through the results, each hit is represented as a dictionary [7][4][5]. 3. Primary Key: The primary key of the matched entity is accessible under its defined field name (commonly "id" if that is your primary key field) [4][5]. 4. Entity Fields: Any additional fields requested via the output_fields parameter are nested within the result dictionary [4][5]. If you previously used the ORM-style SDK, note that in the MilvusClient/AsyncMilvusClient interface, these fields are directly accessible in the result dictionary, rather than requiring a.get method call [7][4][5]. Example of accessing the data: async def search_example: res = await async_client.search( collection_name="my_collection", data=[query_vector], limit=5, output_fields=["text", "id"] # "id" is your primary key field) # res is a list of result sets (one for each query vector) for hits in res: for hit in hits: # hit is a dictionary print(f"Primary Key: {hit['id']}") print(f"Distance: {hit['distance']}") print(f"Output Field 'text': {hit['entity'].get('text')}") Note: In older versions or specific configurations, output fields might sometimes be wrapped inside an 'entity' key within the dictionary, but the modern MilvusClient interface typically provides a flattened dictionary structure for the results [4][5]. Always verify your specific output schema if you are migrating from older SDK patterns [6].
Citations:
- 1: https://milvus.io/blog/introducing-milvus-sdk-v2-native-async-support-unified-apis-and-superior-performance.md
- 2: https://milvus.io/docs/use-async-milvus-client-with-asyncio.md
- 3: https://milvusio.medium.com/introducing-milvus-sdk-v2-native-async-support-unified-apis-and-superior-performance-388c3eb6fa2d
- 4: https://milvus.io/blog/data-in-and-data-out-in-milvus-2-6.md
- 5: https://milvus.io/blog/get-started-with-hybrid-semantic-full-text-search-with-milvus-2-5.md
- 6: https://milvus.io/api-reference/pymilvus/v2.6.x/ORM/Partition/search.md
- 7: https://dev.to/sridhar_s_dfc5fa7b6b295f9/master-rag-systems-build-an-end-to-end-langchain-pipeline-with-milvus-reranking-azure-openai-118c
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="openrag/services/storage/milvus_store.py"
# Show relevant sections around the reported lines
sed -n '640,730p' "$FILE" | cat -n
echo "----"
sed -n '740,880p' "$FILE" | cat -n
echo "----"
rg -n "_parse_search_response|_search_errors" "$FILE"Repository: linagora/openrag
Length of output: 11740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="openrag/services/storage/milvus_store.py"
# Find where the collection schema / primary key field is defined.
rg -n "auto_id|primary|INT64|_id\b|milvus.*schema|create_collection|FieldSchema|CollectionSchema" "$FILE"
echo "----"
# Also search other files for schema creation if needed.
rg -n "create_collection|FieldSchema|CollectionSchema|auto_id" openrag/services/storage -SRepository: linagora/openrag
Length of output: 3109
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="openrag/services/storage/test_milvus_store.py"
# Find tests mentioning _parse_search_response, search response keys, or '_id'
rg -n "_parse_search_response|_id\b|'id'|distance|entity|hybrid_search|search\(" "$FILE"
echo "----"
# Show around any matching tests
# (print small windows around first few matches)
matches=$(rg -n "_parse_search_response|_id\b|'id'|distance|entity|hybrid_search|search\(" "$FILE" | cut -d: -f1 | head -n 20)
for ln in $matches; do
start=$((ln-15)); end=$((ln+25));
echo "### around $ln"
sed -n "${start},${end}p" "$FILE" | cat -n
echo
doneRepository: linagora/openrag
Length of output: 6297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="openrag/services/storage/test_milvus_store.py"
sed -n '430,510p' "$FILE" | cat -nRepository: linagora/openrag
Length of output: 2752
Guard primary-key extraction (hit.get("_id")) consistently in _parse_search_response
entity is derived with isinstance(hit, dict) but pk = hit.get("_id") is not; a non-dict hit would raise AttributeError and escape the _search_errors(...) mapping since _parse_search_response(response) runs outside that context. Existing unit tests cover dict-shaped hits (including _id on the hit and fallback to hit["entity"]["_id"]), so this is a consistency hardening fix.
🛡️ Proposed fix
- pk = hit.get("_id")
- if pk is None:
- pk = entity.get("_id")
+ pk = hit.get("_id") if isinstance(hit, dict) else None
+ if pk is None:
+ pk = entity.get("_id")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # The primary key field is named ``_id`` (auto_id INT64), so Milvus | |
| # exposes it on the hit as ``_id`` and also inside ``entity`` — NOT | |
| # under the generic ``id`` key. Reading ``id`` yielded a literal | |
| # "None" string for every search result. | |
| pk = hit.get("_id") | |
| if pk is None: | |
| pk = entity.get("_id") | |
| record["id"] = self._milvus_id_to_str(pk) if pk is not None else None | |
| # The primary key field is named ``_id`` (auto_id INT64), so Milvus | |
| # exposes it on the hit as ``_id`` and also inside ``entity`` — NOT | |
| # under the generic ``id`` key. Reading ``id`` yielded a literal | |
| # "None" string for every search result. | |
| pk = hit.get("_id") if isinstance(hit, dict) else None | |
| if pk is None: | |
| pk = entity.get("_id") | |
| record["id"] = self._milvus_id_to_str(pk) if pk is not None else None |
🤖 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 `@openrag/services/storage/milvus_store.py` around lines 686 - 693, In
_parse_search_response, avoid calling hit.get("_id") on non-dict hits by first
checking isinstance(hit, dict) (same pattern used when deriving entity); change
the pk extraction to something like: set pk = hit.get("_id") only if
isinstance(hit, dict) else None, then fall back to entity.get("_id") if entity
is a dict, and finally set record["id"] = self._milvus_id_to_str(pk) if pk is
not None else None; update references to hit, entity, pk,
_parse_search_response, and _milvus_id_to_str accordingly so non-dict hits no
longer raise AttributeError and remain handled by the surrounding _search_errors
logic.
…p-server-hexagonal # Conflicts: # openrag/api/dependencies/files.py # openrag/api/routers/admin/indexing.py # openrag/di/providers.py
1 participant
Summary
Reimplements PR #273's MCP (Model Context Protocol) server on the hexagonal architecture, plus the function/security/bug audit fixes from reviewing it against the original.
services.orchestrators.MCPService, which composes the Phase-8 orchestrators (RetrievalService,PartitionService,IndexingService,JobService,ConversionService) + theVectorStoreport and stays Ray-free (services → coreonly).api/mcp/server.pyis a thin FastMCP streamable-HTTP entrypoint: it owns aServiceContainerfor the process, resolves the bearer principal inMCPAuthContextMiddleware, and forwards the partition ACL explicitly to the service. Layer guard passes (api → di → services → core).core/config/mcp.py(OPENRAG_MCP_*env overrides); the service is a lazyServiceContainerproperty.iter_file_lines_reversedis extracted tocore/utils/log_tail.pyso the admin task-logs route and the MCP tool share one implementation.Run it with
uv run -m api.mcp.server(defaults0.0.0.0:8081/mcp).Audit findings addressed
Security:
url_safetynow also resolves hostnames and blocks DNS-to-private (the original literal-only check let a public name pointing at an internal IP through). Residual TOCTOU DNS-rebinding is documented.index_urlnow requires editor access (the original allowed any viewer to index); caller-supplied metadata is stripped of server-managed keys on all write tools.Bugs:
get_task_logsnow capsmax_lines(the_MAX_LOG_LINESbound was previously dead).get_file_inforeports an exact chunk count (was silently capped at 2000 and inconsistent withget_file_chunks) and no longer leaks the raw embedding vector in metadata.Test plan
uv run pytest openrag/services/orchestrators/test_mcp_service.py openrag/api/mcp/test_server.py openrag/core/utils/test_url_safety.py openrag/core/utils/test_log_tail.py— 79 passeduv run pytest openrag/di/test_container.py openrag/core/config/— 89 passeduv run python scripts/check_layer_imports.py— OKuv run ruff checkon changed files — cleanuv run -m api.mcp.server+ an MCP client)Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests