refactor: restructure MCP server, upgrade to v2 search, add 37 tests (OPE-94, OPE-91)#263
Merged
Merged
Conversation
…E-94, OPE-91) Split 389-line server.py into focused modules: - config.py (16 lines) -- env config, no decorative headers - api_client.py (59 lines) -- persistent httpx client, reused across calls - tools.py (146 lines) -- tool schema definitions - formatters.py (175 lines) -- response -> markdown, independently testable - handlers.py (104 lines) -- dispatch + API calls + safe error messages - server.py (54 lines) -- bootstrap only Security fixes: - Remove hardcoded 'dev-secret-key' default (empty string, fails loud) - Remove 'dev-secret-key' from .env.example - Add local .gitignore for venv/, __pycache__/, .env - Safe error messages: no httpx internals leaked, includes tool name + repo_id CLAUDE.md compliance: - Remove emoji from cached result indicators (was lightning bolt) - All files under 200 lines Note: search endpoint is already correct (/api/v1/search uses query expansion + reranking). The 'uses v1 search' finding in OPE-91 is stale -- there is only one search endpoint and it already uses the latest engine.
- handlers: switch from /search to /search/v2, map max_results to top_k - formatters: support v2 fields (qualified_name, signature, match_reason) - formatters: maintain backward compat with v1 response shape - tests: 37 tests covering config, formatters, handlers, tool schemas - tests: verify no emoji, error message sanitization, dispatch logic - config: bump version 0.3.0 -> 0.4.0 - requirements: add pytest, pytest-asyncio as test dependencies
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors MCP server into modular components: environment-driven config, a persistent async API client, tool schemas, formatters, a handler dispatcher, updated server orchestration, and extensive tests and test config; adds .gitignore and test dependencies. No external behavior beyond these internal reorganizations is introduced. Changes
Sequence Diagram(s)sequenceDiagram
actor LLM as LLM/Client
participant Server as MCP Server
participant Tools as Tools Module
participant Handlers as Handlers Module
participant APIClient as API Client
participant Backend as Backend API
LLM->>Server: request tool list / call_tool(name, args)
Server->>Tools: get_tool_schemas()
Tools-->>Server: tool schemas
Server->>Handlers: call_tool(name, arguments)
Handlers->>Handlers: resolve handler
Handlers->>APIClient: api_get/post(path, payload)
APIClient->>APIClient: ensure singleton client (get_client) with auth header
APIClient->>Backend: HTTP request (with Bearer token)
Backend-->>APIClient: JSON response
APIClient-->>Handlers: parsed JSON
Handlers->>Handlers: format_* -> Markdown
Handlers-->>Server: TextContent result
Server-->>LLM: return result
Note over Server,APIClient: main() finally -> close_client()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
mcp-server/.env.example (1)
2-3: Consider reordering keys to satisfy dotenv-linter.This is only lint hygiene, but reordering
API_KEYbeforeBACKEND_API_URLwill clear the reportedUnorderedKeywarning.♻️ Suggested reorder
-# Backend API Configuration -BACKEND_API_URL=http://localhost:8000 -API_KEY=your-api-key-here +# Backend API Configuration +API_KEY=your-api-key-here +BACKEND_API_URL=http://localhost:8000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/.env.example` around lines 2 - 3, The keys in the .env example are out of alphabetical order causing a dotenv-linter UnorderedKey warning; reorder the environment variables so API_KEY comes before BACKEND_API_URL (i.e., place the "API_KEY" entry above the "BACKEND_API_URL" entry) and save the updated .env.example to satisfy the linter.mcp-server/tests/test_tools.py (1)
48-53: Add a contract test formax_resultsconstraints.Given
search_codeexposesmax_results, add assertions for type/range so schema regressions are caught early.✅ Suggested test extension
def test_search_requires_query_and_repo(self): schemas = {t.name: t for t in get_tool_schemas()} search = schemas["search_code"] assert "query" in search.inputSchema["required"] assert "repo_id" in search.inputSchema["required"] + max_results = search.inputSchema["properties"]["max_results"] + assert max_results["type"] == "integer" + assert max_results["minimum"] == 1 + assert max_results["maximum"] == 100🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/tests/test_tools.py` around lines 48 - 53, Extend test_search_requires_query_and_repo to also validate the search_code max_results schema: fetch the property at search.inputSchema["properties"]["max_results"] and assert it declares a numeric/integer type (e.g., "integer" or "number"), assert it has a "minimum" constraint and that minimum >= 1, and assert it has a "maximum" constraint and that maximum > minimum so the range is explicit and sane; use the existing variables schemas and search to locate the schema and perform these assertions to guard against regressions.mcp-server/api_client.py (1)
26-35: Lazy singleton is fine for single-event-loop usage, but note the potential double-init race.If two coroutines concurrently call
get_client()when_client is None, both could pass theNonecheck and create separate clients. In a stdio-based MCP server this is practically impossible (sequential dispatch), but if this module is ever reused in a concurrent context, anasyncio.Lockwould be needed.No action required for now — just flagging for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/api_client.py` around lines 26 - 35, get_client currently does an unsafe lazy singleton check on the module-level _client which can race if two coroutines call get_client concurrently; to fix, add an asyncio.Lock (e.g., _client_lock) and acquire it inside get_client before checking/creating _client so only one coroutine can create the httpx.AsyncClient (reference get_client, _client, and introduce _client_lock). Ensure the lock is created at module import time and held only for the create/check section to avoid blocking other awaits.mcp-server/tests/test_formatters.py (2)
10-11: Consider using aconftest.pyto managesys.pathinstead of repeating in every test file.All three test files duplicate the same
sys.path.insert(...)pattern. A singleconftest.pyin thetests/directory (or apyproject.toml/pytest.inipythonpathsetting) would eliminate this repetition and be the idiomatic pytest approach.Example
tests/conftest.py:import sys, os sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))Or in
pyproject.toml:[tool.pytest.ini_options] pythonpath = ["mcp-server"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/tests/test_formatters.py` around lines 10 - 11, Move the repeated sys.path manipulation out of individual tests and into a single tests/conftest.py: remove the sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) line from files like test_formatters.py and create tests/conftest.py that performs that same sys.path.insert call so pytest applies it globally; alternatively document adding a pythonpath entry in pyproject.toml/pytest.ini (e.g., pythonpath = ["mcp-server"]) if you prefer configuration over a conftest.py.
72-80: Emoji check only covers the lightning bolt; consider a broader assertion if enforcing a no-emoji policy.The test only asserts
\u26a1is absent, which is the specific emoji from the OPE-91 audit. If the no-emoji policy is meant to be general, a regex-based check against Unicode emoji ranges would be more resilient against future regressions.That said, a targeted check for the known offender is a reasonable pragmatic choice for now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/tests/test_formatters.py` around lines 72 - 80, The test test_no_emoji_in_output currently only checks for the single lightning bolt ("\u26a1") after calling format_search_results, so either broaden the assertion to detect any emoji or make intent explicit; update the test_no_emoji_in_output to use a regex that matches Unicode emoji ranges (or use a utility like the `emoji` package) to assert no emojis appear in output, or add a clarifying comment that the check is intentionally scoped to "\u26a1" if you want to keep it targeted.mcp-server/config.py (1)
11-12: Env var naming is semantically inconsistent but well-documented.The environment variable
BACKEND_API_URLis named to suggest a full API URL, but is consistently documented and used as a base URL (without the/api/v1prefix). The code then appends the prefix to create the Python constantBACKEND_API_URL. While this naming creates semantic confusion, all documentation (README, deployment docs, docker-compose examples) consistently shows the correct pattern—setting the env var to a base URL likehttp://localhost:8000.Consider renaming the env var to
BACKEND_BASE_URLfor clarity, but this is not urgent since the current naming is consistently documented across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/config.py` around lines 11 - 12, The env var naming is confusing: change the os.getenv key so the module-level constant BACKEND_BASE_URL is derived from "BACKEND_BASE_URL" (not "BACKEND_API_URL"), and keep BACKEND_API_URL defined as f"{BACKEND_BASE_URL}{API_PREFIX}"; for compatibility, read the new "BACKEND_BASE_URL" first and fall back to the old "BACKEND_API_URL" env var if the new one is unset, so both names work during rollout (update the os.getenv call that currently sets BACKEND_BASE_URL and ensure the relationship between BACKEND_BASE_URL, BACKEND_API_URL, and API_PREFIX remains intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp-server/api_client.py`:
- Around line 17-23: The docstring for _get_headers() is incorrect: it says
"Warns if no API key is configured" but the function raises ValueError when
API_KEY is missing; update the docstring to accurately describe behavior (e.g.,
"Raise ValueError if no API_KEY is configured") and mention that it returns the
Authorization header {"Authorization": f"Bearer {API_KEY}"} on success,
referencing the API_KEY symbol and the _get_headers function so callers know the
function raises rather than warns.
In `@mcp-server/formatters.py`:
- Around line 99-100: The formatter currently uses direct dict indexing (e.g.,
f['id'], f['imports']) inside the sorted/loop blocks (see the "for f in
sorted(high_import, ...)" loop) which can raise KeyError on partial API
payloads; update these accesses to use .get with sensible defaults (e.g.,
f.get('id', '<unknown>'), f.get('imports', 0)) and coerce values for formatting
as needed so the loop still produces degraded output instead of crashing; apply
the same defensive change to the other similar loops/uses referenced in the diff
(the other occurrences that index fields on the loop variable, such as the
accesses at the later blocks around lines noted in the review).
- Line 23: Guard against None/non-numeric score before multiplying: replace the
direct expression res.get("score", 0) * 100 with parsing logic that reads
score_raw = res.get("score"), then try to coerce to float (e.g. score =
float(score_raw) * 100) inside a try/except catching TypeError/ValueError and
falling back to score = 0; ensure you keep the variable name score for
downstream code and apply this change where res.get("score", 0) is used in
formatters.py so non-numeric or None values won't raise.
In `@mcp-server/handlers.py`:
- Around line 91-93: The current branch returns raw str(error) for ValueError
which can leak internals; in the function handling tool errors replace returning
str(error) with a sanitized message (e.g., a generic "Invalid input" or "Tool
input error" including tool_name and repo_id) and ensure the original exception
is recorded to internal logs (use the module's logger or logger.exception with
the error and context) so only safe text is returned to callers while full
details remain in server logs; update the conditional that checks
isinstance(error, ValueError) to return the sanitized string and add a logging
call that records error, tool_name, and repo_id.
- Around line 24-33: In _handle_search validate args.get("max_results") before
assigning to payload["top_k"]: ensure the value (from args["max_results"]) is
present or defaulted, is an integer, and is within an acceptable range (e.g.,
clamp to min 1 and a sensible max like 100) or raise a clear error if invalid;
then set "top_k" to the sanitized value when building the payload for
api_post("/search/v2") and keep use_reranking and format_search_results(result)
unchanged.
In `@mcp-server/tests/test_handlers.py`:
- Around line 70-75: The test test_none_arguments_handled is making a real
network call because api_get isn't mocked; instead mock api_get (or the
higher-level function that performs HTTP) when invoking
call_tool("list_repositories", None) so the test only verifies that call_tool
handles None args without crashing. Locate the test test_none_arguments_handled
and patch/make a fixture to stub api_get to return a predictable value (e.g., a
dummy response or empty list) or raise the same ValueError path, ensuring
call_tool and _get_headers behavior is exercised in isolation and no real HTTP
request occurs.
In `@mcp-server/tools.py`:
- Around line 35-39: The schema entry for "max_results" is currently unbounded
and can accept 0, negatives, or huge values; update the JSON/schema definition
for "max_results" (the property named "max_results" used when mapping to top_k)
to include "minimum": 1 and a reasonable "maximum" (e.g., 100) to prevent
invalid or expensive searches, keep "type": "integer" and "default": 10, and add
any runtime validation in the code path that maps max_results -> top_k to clamp
or reject values outside [1,100].
---
Nitpick comments:
In `@mcp-server/.env.example`:
- Around line 2-3: The keys in the .env example are out of alphabetical order
causing a dotenv-linter UnorderedKey warning; reorder the environment variables
so API_KEY comes before BACKEND_API_URL (i.e., place the "API_KEY" entry above
the "BACKEND_API_URL" entry) and save the updated .env.example to satisfy the
linter.
In `@mcp-server/api_client.py`:
- Around line 26-35: get_client currently does an unsafe lazy singleton check on
the module-level _client which can race if two coroutines call get_client
concurrently; to fix, add an asyncio.Lock (e.g., _client_lock) and acquire it
inside get_client before checking/creating _client so only one coroutine can
create the httpx.AsyncClient (reference get_client, _client, and introduce
_client_lock). Ensure the lock is created at module import time and held only
for the create/check section to avoid blocking other awaits.
In `@mcp-server/config.py`:
- Around line 11-12: The env var naming is confusing: change the os.getenv key
so the module-level constant BACKEND_BASE_URL is derived from "BACKEND_BASE_URL"
(not "BACKEND_API_URL"), and keep BACKEND_API_URL defined as
f"{BACKEND_BASE_URL}{API_PREFIX}"; for compatibility, read the new
"BACKEND_BASE_URL" first and fall back to the old "BACKEND_API_URL" env var if
the new one is unset, so both names work during rollout (update the os.getenv
call that currently sets BACKEND_BASE_URL and ensure the relationship between
BACKEND_BASE_URL, BACKEND_API_URL, and API_PREFIX remains intact).
In `@mcp-server/tests/test_formatters.py`:
- Around line 10-11: Move the repeated sys.path manipulation out of individual
tests and into a single tests/conftest.py: remove the sys.path.insert(0,
os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) line from files
like test_formatters.py and create tests/conftest.py that performs that same
sys.path.insert call so pytest applies it globally; alternatively document
adding a pythonpath entry in pyproject.toml/pytest.ini (e.g., pythonpath =
["mcp-server"]) if you prefer configuration over a conftest.py.
- Around line 72-80: The test test_no_emoji_in_output currently only checks for
the single lightning bolt ("\u26a1") after calling format_search_results, so
either broaden the assertion to detect any emoji or make intent explicit; update
the test_no_emoji_in_output to use a regex that matches Unicode emoji ranges (or
use a utility like the `emoji` package) to assert no emojis appear in output, or
add a clarifying comment that the check is intentionally scoped to "\u26a1" if
you want to keep it targeted.
In `@mcp-server/tests/test_tools.py`:
- Around line 48-53: Extend test_search_requires_query_and_repo to also validate
the search_code max_results schema: fetch the property at
search.inputSchema["properties"]["max_results"] and assert it declares a
numeric/integer type (e.g., "integer" or "number"), assert it has a "minimum"
constraint and that minimum >= 1, and assert it has a "maximum" constraint and
that maximum > minimum so the range is explicit and sane; use the existing
variables schemas and search to locate the schema and perform these assertions
to guard against regressions.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
mcp-server/.env.examplemcp-server/.gitignoremcp-server/api_client.pymcp-server/config.pymcp-server/formatters.pymcp-server/handlers.pymcp-server/pyproject.tomlmcp-server/requirements.txtmcp-server/server.pymcp-server/tests/__init__.pymcp-server/tests/test_config.pymcp-server/tests/test_formatters.pymcp-server/tests/test_handlers.pymcp-server/tests/test_tools.pymcp-server/tools.py
…or sanitization - api_client: fix docstring (warns -> raises), add asyncio.Lock for safe lazy init - formatters: guard against None/non-numeric score, use .get() for all dict access - handlers: sanitize ValueError (no longer leaks internals), log errors server-side - handlers: add _clamp_max_results to validate and bound top_k to [1, 100] - tools: add minimum/maximum constraints to max_results schema - tests: mock api_get in test_none_arguments_handled (no real HTTP) - tests: add 8 new tests (score=None, clamp bounds, ValueError sanitized, schema bounds) - tests: extract sys.path to conftest.py, remove from all test files - .env.example: alphabetical key ordering (dotenv-linter compliance) Skipped: N3 (env var rename -- deployment concern), N5 (emoji regex -- overkill) 45 tests pass.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
mcp-server/tests/test_formatters.py (1)
76-85: Tighten the no-emoji assertions to match the stated policy.These tests currently block only one code point (
⚡). If the requirement is truly “no emojis anywhere,” consider a shared helper that checks a broader emoji range so regressions don’t slip through.Also applies to: 197-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp-server/tests/test_formatters.py` around lines 76 - 85, The test test_no_emoji_in_output currently only checks for the single code point U+26A1; update it to assert no emojis anywhere by using a shared helper (e.g., assert_no_emojis_in_string) that scans the output of format_search_results for emoji code point ranges or via a robust emoji regex and fails if any emoji is found; add/update this helper in the test module and replace the lone assert "\u26a1" not in output with a call to that helper, and make the same change for the other test(s) referenced at lines 197-199.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcp-server/api_client.py`:
- Around line 61-66: get_client() guards _client with _client_lock but
close_client() manipulates _client without that lock, creating a race; fix by
acquiring the same _client_lock in close_client(), atomically removing/nulling
_client (e.g. copy _client to a local var, set module _client = None) while
holding the lock, then release the lock and await local.aclose() if the local
var exists (so closing happens outside the lock to avoid blocking). Ensure you
reference and use the existing _client_lock, _client, get_client(), and
close_client() symbols.
---
Nitpick comments:
In `@mcp-server/tests/test_formatters.py`:
- Around line 76-85: The test test_no_emoji_in_output currently only checks for
the single code point U+26A1; update it to assert no emojis anywhere by using a
shared helper (e.g., assert_no_emojis_in_string) that scans the output of
format_search_results for emoji code point ranges or via a robust emoji regex
and fails if any emoji is found; add/update this helper in the test module and
replace the lone assert "\u26a1" not in output with a call to that helper, and
make the same change for the other test(s) referenced at lines 197-199.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
mcp-server/.env.examplemcp-server/api_client.pymcp-server/formatters.pymcp-server/handlers.pymcp-server/tests/conftest.pymcp-server/tests/test_config.pymcp-server/tests/test_formatters.pymcp-server/tests/test_handlers.pymcp-server/tests/test_tools.pymcp-server/tools.py
🚧 Files skipped from review as they are similar to previous changes (5)
- mcp-server/handlers.py
- mcp-server/formatters.py
- mcp-server/tests/test_config.py
- mcp-server/tests/test_handlers.py
- mcp-server/tests/test_tools.py
Collaborator
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
MCP server was a 389-line monolith (
server.py) with tool definitions, API calls, response formatting, config, and error handling all in one file. OPE-91 audit flagged: v1 search API (deprecated), zero tests, error messages leaking internal details, emoji in output.Fix
Module restructuring (OPE-94)
server.py: 389 -> 54 lines (entry point only)config.py: Environment config (16 lines)api_client.py: Persistent HTTP client with lazy inittools.py: MCP tool schema definitionshandlers.py: Tool dispatch with centralized error handlingformatters.py: Pure functions converting API responses to markdownSearch v2 upgrade (OPE-91)
/searchto/search/v2max_results(tool schema) totop_k(v2 API)use_reranking: truefor better result qualityqualified_name,signature,match_reasonSecurity hardening (OPE-91)
_safe_error_message()prevents leaking internal URLs, stack tracesTest coverage -- 37 tests
test_config.py(4): Config format validationtest_formatters.py(19): All 7 formatters, v1/v2 compat, emoji checkstest_handlers.py(11): v2 dispatch, max_results->top_k mapping, error sanitizationtest_tools.py(7): Schema validation, required fields, MCP protocol complianceCleanup
OPE-91 Audit Status
Test
Closes OPE-94, OPE-91
Summary by CodeRabbit
New Features
Tests
Chores