Add local MCP server for public REST reads - #1013
Open
nolanefe wants to merge 2 commits into
Open
Conversation
nolanefe
requested review from
Pa04rth,
northdpole,
paoga87 and
robvanderveer
as code owners
August 12, 2026 17:05
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
Summary by CodeRabbit
WalkthroughAdded a local stdio MCP server for nine allowlisted OpenCRE public REST GET operations. The implementation derives schemas from OpenAPI, validates REST requests, exposes MCP dispatch, adds tests, and documents setup and scope. ChangesPublic MCP server
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
application/mcp/rest_client.py (1)
296-318: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider narrowing the JSON decode fallback.
Line 302 catches every exception to fall back to raw text. Ruff flags this as
BLE001.ValueError(whichjson.JSONDecodeErrorsubclasses) covers bothrequestsand the Flask test response wrapper. A narrower catch keeps genuine adapter faults visible.♻️ Proposed change
try: data = response.json() - except Exception: + except ValueError: data = text🤖 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 `@application/mcp/rest_client.py` around lines 296 - 318, Update _parse_response so the response.json() fallback catches only ValueError instead of every exception, preserving raw-text fallback for JSON decoding failures while allowing genuine adapter errors to propagate.Source: Linters/SAST tools
application/mcp/server.py (1)
61-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the precomputed schemas and remove the duplicated lookups.
Lines 68-72 perform four redundant steps per call:
list_tool_names()rebuilds a list for a membership test,get_tool(name)runs twice, andoperation_input_schemare-walks the OpenAPI parameters even thoughbuild_serveralready resolved every schema intoschemas.client.call_toolperforms the same catalog lookup and schema resolution again. A single membership test againstschemaskeeps the same behavior with less work.♻️ Proposed simplification
try: - if name not in list_tool_names(): + if name not in schemas: raise RestRequestError(f"Unknown MCP tool: {name}") - # Ensure allowlist entry still matches OpenAPI before calling REST. - get_tool(name) - operation_input_schema(get_tool(name)) result = client.call_tool(name, arguments)Drop the now-unused
get_tool,list_tool_names, andoperation_input_schemaimports if nothing else uses them.🤖 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 `@application/mcp/server.py` around lines 61 - 79, Update on_call_tool to validate tool existence with a direct membership check against the precomputed schemas mapping, then call client.call_tool without the redundant list_tool_names, get_tool, or operation_input_schema lookups. Remove those imports if no other code uses them, while preserving the existing unknown-tool error behavior.application/mcp/openapi_loader.py (1)
37-59: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider a recursion guard for
$refcycles.
_resolve_reffollows references recursively. If the committed OpenAPI document ever contains a cyclic$ref, this raisesRecursionErrorat server startup instead of a clearOpenAPILookupError. A visited-ref set keeps the failure diagnosable.♻️ Proposed guard
-def _resolve_ref(spec: Dict[str, Any], node: Any) -> Any: +def _resolve_ref( + spec: Dict[str, Any], node: Any, _seen: Optional[Set[str]] = None +) -> Any: """Resolve local `#/components/`... refs one level deep as needed.""" + seen = set(_seen or ()) if not isinstance(node, dict): return node if "$ref" not in node: - return {key: _resolve_ref(spec, value) for key, value in node.items()} + return {key: _resolve_ref(spec, value, seen) for key, value in node.items()} ref = node["$ref"] if not isinstance(ref, str) or not ref.startswith("`#/`"): raise OpenAPILookupError(f"Unsupported OpenAPI $ref: {ref}") + if ref in seen: + raise OpenAPILookupError(f"Circular OpenAPI $ref: {ref}") + seen.add(ref)Pass
seenthrough the remaining recursive call on the merged result.🤖 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 `@application/mcp/openapi_loader.py` around lines 37 - 59, Update _resolve_ref to track the currently visited $ref values through recursive resolution, including the recursive call after merging sibling keywords. Before following a reference, detect if it is already in the set and raise OpenAPILookupError with the reference identifier; otherwise add it and propagate the set through subsequent _resolve_ref calls.
🤖 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 `@application/mcp/rest_client.py`:
- Around line 172-178: Update the cookie cleanup block in the REST client method
containing _parse_response so failures from self._session.cookies.clear() are
logged instead of silently ignored. Add a module-level logging import and logger
initialized with __name__, then use it in the exception handler while preserving
the existing response parsing and cookie-clearing behavior.
In `@application/tests/mcp_public_tools_test.py`:
- Around line 51-65: Scope the INSECURE_REQUESTS environment change in setUp
using the imported patch.dict mechanism, or otherwise restore its original value
during tearDown. Update setUp and tearDown so later tests do not inherit "True",
while preserving the existing application and database cleanup flow.
---
Nitpick comments:
In `@application/mcp/openapi_loader.py`:
- Around line 37-59: Update _resolve_ref to track the currently visited $ref
values through recursive resolution, including the recursive call after merging
sibling keywords. Before following a reference, detect if it is already in the
set and raise OpenAPILookupError with the reference identifier; otherwise add it
and propagate the set through subsequent _resolve_ref calls.
In `@application/mcp/rest_client.py`:
- Around line 296-318: Update _parse_response so the response.json() fallback
catches only ValueError instead of every exception, preserving raw-text fallback
for JSON decoding failures while allowing genuine adapter errors to propagate.
In `@application/mcp/server.py`:
- Around line 61-79: Update on_call_tool to validate tool existence with a
direct membership check against the precomputed schemas mapping, then call
client.call_tool without the redundant list_tool_names, get_tool, or
operation_input_schema lookups. Remove those imports if no other code uses them,
while preserving the existing unknown-tool error behavior.
🪄 Autofix
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.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 93aff805-2ca6-4c8e-99fe-2b82d7d93e46
📒 Files selected for processing (11)
.gitignoreREADME.mdapplication/mcp/__init__.pyapplication/mcp/__main__.pyapplication/mcp/catalog.pyapplication/mcp/openapi_loader.pyapplication/mcp/rest_client.pyapplication/mcp/server.pyapplication/tests/mcp_public_tools_test.pydocs/api/mcp.mdrequirements-dev.txt
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.
Summary
Implements the maintainer-approved first phase of #1003: a local stdio MCP server exposing a bounded set of public OpenCRE REST reads.
The MCP surface contains nine tools:
get_cre_by_idget_cre_by_nameget_nodeget_documents_by_tagtext_searchlist_root_creslist_all_creslist_standardslist_ga_standardsEach tool is backed by its corresponding
/rest/v1endpoint. Tool exposure comes from an explicit public-read allowlist, while argument names/types/requiredness are derived from the committed OpenAPI specification.Design
OPENCRE_BASE_URLis server-side configuration and cannot be supplied by tool callersget_noderestrictsntypeto CRE document types to prevent collisions with routes such as/rest/v1/user/resourcesScope
Per the direction in #1003, this PR intentionally does not add authentication.
Deferred to follow-up work:
/rest/v1/user/resources/rest/v1/completiondocs/api/mcp.mddocuments local startup, Cursor configuration, the tool-to-REST mapping, security boundary, and these deliberate parity gaps.Validation
make lint: passedjsonschema,networkx), request-handler context typing in MCP tests, and errors reached through existing imported modulesmake mypyalso remains nonzero because of existing repository typing debt; this PR does not attempt unrelated typing cleanupmake test: 767 tests passed, 3 skippedgit diff --check: cleanNo jobs were runbecause the upstream.github/workflows/e2e.ymlis currently fully commented out and contains no executable jobs; this is existing repository configuration, not a failure introduced by this PR.Part of #1003