feat(mcp): add get_space_by_slug method#2022
Open
plitzenberger wants to merge 20 commits intomainfrom
Open
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds an HTTP MCP endpoint and server wiring, a new mcp-tools package (multiple MCP tools and schemas), MCP-builder docs and evaluation tooling, connection utilities (stdio/SSE/HTTP), Cursor MCP config, workspace/package updates, middleware exclusion for /mcp, and agent/role docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client / Cursor
participant Route as Next.js /mcp Route
participant Transport as WebStandardStreamableHTTPServerTransport
participant Server as McpServer Instance
participant Tool as get_space_by_slug Handler
participant DB as Space Service / DB
Client->>Route: POST /mcp (JSON‑RPC request)
Route->>Transport: create transport & handleRequest(request)
Transport->>Server: dispatch JSON‑RPC -> tool_call
Server->>Tool: invoke get_space_by_slug(slug)
Tool->>DB: query space by slug
DB-->>Tool: return space data / not found
Tool-->>Server: return structuredContent + text
Server->>Transport: JSON‑RPC response
Transport->>Client: HTTP 200 (JSON‑RPC response)
Note over Route,Transport: cleanup
Route->>Transport: close (rgba(0,128,0,0.5))
Route->>Server: close (rgba(0,0,128,0.5))
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
.agents/skills/mcp-builder/scripts/requirements.txt (1)
1-2: Add upper bounds to lock dependency versions for reproducibility.Lines 1–2 allow unbounded upgrades that can introduce breaking changes.
anthropicis stable in 0.x; capping at <1.0.0 is safe. Formcp, the 1.x line has documented breaking changes (v1.10 protocol update, v1.23 auth revision, v1.25 signals v2 branching); capping at <2.0.0 prevents major-version disruption.Proposed change
-anthropic>=0.39.0 -mcp>=1.1.0 +anthropic>=0.39.0,<1.0.0 +mcp>=1.1.0,<2.0.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/scripts/requirements.txt around lines 1 - 2, Update the dependency bounds to prevent unbounded major upgrades: modify the entries for anthropic and mcp so anthropic reads with an upper bound of <1.0.0 and mcp reads with an upper bound of <2.0.0 (e.g., change "anthropic>=0.39.0" to "anthropic>=0.39.0,<1.0.0" and "mcp>=1.1.0" to "mcp>=1.1.0,<2.0.0"); after changing the requirements ensure any lockfile or environment is regenerated to reflect the tightened constraints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/mcp-builder/reference/evaluation.md:
- Around line 59-61: The phrase "each step should benefit from information found
in previous questions" conflicts with the independence rule; update the second
bullet under "Questions must require deep exploration" to clarify it means
"previous steps/sub-questions within the same multi-step question," not earlier
independent questions, so replace "previous questions" with "previous
steps/sub-questions within the same question" and ensure wording aligns with the
existing "Questions must be independent" rule.
In @.agents/skills/mcp-builder/reference/node_mcp_server.md:
- Around line 81-94: The markdown code fence showing the project tree is missing
a language label which triggers MD040; edit the unlabeled fenced block in
.agents/skills/mcp-builder/reference/node_mcp_server.md (the block that starts
with ``` and contains the {service}-mcp-server/ tree) and change the opening
fence from ``` to ```text so the block becomes a labeled text code fence (no
other content changes needed).
- Around line 547-550: The package example pins "zod" to "^3.23.8", which is
incompatible with the MCP SDK requirement for zod >= 3.25.0 (needed for subpath
imports such as "zod/v3"); update the dependency entry that currently reads
"zod": "^3.23.8" to a compatible range (e.g., "zod": "^3.25.0" or ">=3.25.0") so
the example aligns with the MCP SDK and the SKILL.md requirement.
In @.agents/skills/mcp-builder/reference/python_mcp_server.md:
- Around line 532-541: The get_document resource is vulnerable to path traversal
by interpolating name into "./docs/{name}"; fix by resolving the requested file
against a fixed base directory (e.g., base = Path("./docs").resolve()) and
constructing the target Path from name, then calling .resolve() and verifying
the resolved path is inside the base (using Path.is_relative_to or comparing
parents) before opening; if the check fails, raise an exception or return a
400/404. Update get_document and the document_path handling to perform these
checks and refuse escapes like "../".
In @.agents/skills/mcp-builder/scripts/connections.py:
- Around line 67-70: The call_tool method currently returns result.content which
may contain Pydantic model objects (e.g., TextContent, ImageContent) that aren't
JSON-serializable; update call_tool to normalize result.content into pure
JSON-serializable primitives before returning: after calling
self.session.call_tool(tool_name, arguments=arguments) inspect result.content
and recursively convert any Pydantic BaseModel instances (or lists/dicts
containing them) to plain dicts/values (using the model's dict()/model_dump() or
an equivalent recursive encoder) so that the final returned value contains only
primitives (str, int, float, bool, None, list, dict) and is safe for json.dumps;
keep this logic inside call_tool so callers like evaluation.py always receive
serializable data.
In @.agents/skills/mcp-builder/scripts/evaluation.py:
- Line 13: The script currently imports xml.etree.ElementTree (import
xml.etree.ElementTree as ET) which is vulnerable to XXE/entity expansion;
replace that import with defusedxml.ElementTree (e.g., import
defusedxml.ElementTree as ET) so the parser blocks these attack vectors, and add
defusedxml to requirements.txt so the dependency is installed; update any
references to ET in the file to use the defusedxml-backed module (no other code
changes required).
- Around line 109-145: The loop extracts only the first tool_use via next() and
re-prompts, dropping parallel tool calls and allowing an unbounded loop; change
it to collect all tool_use blocks in response.content (e.g., [b for b in
response.content if b.type == "tool_use"]) and iterate over that list to execute
each tool (using connection.call_tool), update tool_metrics for each tool, and
build a single user message whose content is a list of multiple
{"type":"tool_result","tool_use_id":..., "content":...} entries appended once to
messages before calling client.messages.create; also add a turn counter /
MAX_TURNS guard around the while response.stop_reason == "tool_use" loop and
break with a clear error/timeout if the ceiling is reached to prevent runaway
execution.
In @.agents/skills/mcp-builder/scripts/example_evaluation.xml:
- Around line 2-20: The QA pairs in example_evaluation.xml currently contain
only closed-form math problems (each within <qa_pair> tags) that the base model
can solve without MCP tool calls; update the file so the example evaluation
actually exercises the server by replacing or augmenting these <qa_pair> entries
with multi-hop questions that require explicit tool usage (e.g., retrieval +
calculation, API-backed data lookup, sequential tool-chained steps, or
simulation requests) or, if the intent is only simple smoke tests, move/rename
this file to a smoke-test fixture; ensure each revised <qa_pair> clearly
requires a tool call and references the expected tool outputs in the answer
field.
---
Nitpick comments:
In @.agents/skills/mcp-builder/scripts/requirements.txt:
- Around line 1-2: Update the dependency bounds to prevent unbounded major
upgrades: modify the entries for anthropic and mcp so anthropic reads with an
upper bound of <1.0.0 and mcp reads with an upper bound of <2.0.0 (e.g., change
"anthropic>=0.39.0" to "anthropic>=0.39.0,<1.0.0" and "mcp>=1.1.0" to
"mcp>=1.1.0,<2.0.0"); after changing the requirements ensure any lockfile or
environment is regenerated to reflect the tightened constraints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8742cf91-c090-4976-8366-7876bd49a7e4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.agents/AGENTS.md.agents/_library/collaboration/cross-functional-teams.md.agents/_library/competencies/agile-delivery.md.agents/_library/competencies/requirements-engineering.md.agents/_library/output-standards/actionable-recommendations.md.agents/roles/senior-lead-fullstack-nextjs-engineer.base.md.agents/skills/hypha-web-mcp/SKILL.md.agents/skills/mcp-builder/LICENSE.txt.agents/skills/mcp-builder/SKILL.md.agents/skills/mcp-builder/reference/evaluation.md.agents/skills/mcp-builder/reference/mcp_best_practices.md.agents/skills/mcp-builder/reference/node_mcp_server.md.agents/skills/mcp-builder/reference/python_mcp_server.md.agents/skills/mcp-builder/scripts/connections.py.agents/skills/mcp-builder/scripts/evaluation.py.agents/skills/mcp-builder/scripts/example_evaluation.xml.agents/skills/mcp-builder/scripts/requirements.txt.cursor/mcp.jsonapps/web/package.jsonapps/web/src/app/mcp/route.tsapps/web/src/middleware.tspackage.jsonskills-lock.json
Comment on lines
+59
to
+61
| 4. **Questions must require deep exploration** | ||
| - Consider multi-hop questions requiring multiple sub-questions and sequential tool calls | ||
| - Each step should benefit from information found in previous questions |
Contributor
There was a problem hiding this comment.
"Previous questions" contradicts the independence rule.
Line 47 says questions must be independent, but Line 61 says each step should benefit from information found in previous questions. I think this should say previous steps/sub-questions within the same question; otherwise the guidance tells authors to couple tasks that are supposed to stand alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/reference/evaluation.md around lines 59 - 61, The
phrase "each step should benefit from information found in previous questions"
conflicts with the independence rule; update the second bullet under "Questions
must require deep exploration" to clarify it means "previous steps/sub-questions
within the same multi-step question," not earlier independent questions, so
replace "previous questions" with "previous steps/sub-questions within the same
question" and ensure wording aligns with the existing "Questions must be
independent" rule.
Comment on lines
+81
to
+94
| ``` | ||
| {service}-mcp-server/ | ||
| ├── package.json | ||
| ├── tsconfig.json | ||
| ├── README.md | ||
| ├── src/ | ||
| │ ├── index.ts # Main entry point with McpServer initialization | ||
| │ ├── types.ts # TypeScript type definitions and interfaces | ||
| │ ├── tools/ # Tool implementations (one file per domain) | ||
| │ ├── services/ # API clients and shared utilities | ||
| │ ├── schemas/ # Zod validation schemas | ||
| │ └── constants.ts # Shared constants (API_URL, CHARACTER_LIMIT, etc.) | ||
| └── dist/ # Built JavaScript files (entry point: dist/index.js) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add a language to the fenced project-structure block.
Line 81 opens an unlabeled fenced block, which triggers markdownlint MD040.
Proposed doc-lint fix
-```
+```text
{service}-mcp-server/
├── package.json
...
└── dist/ # Built JavaScript files (entry point: dist/index.js)</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/reference/node_mcp_server.md around lines 81 -
94, The markdown code fence showing the project tree is missing a language label
which triggers MD040; edit the unlabeled fenced block in
.agents/skills/mcp-builder/reference/node_mcp_server.md (the block that starts
with ``` and contains the {service}-mcp-server/ tree) and change the opening
fence from ``` to ```text so the block becomes a labeled text code fence (no
other content changes needed).
Comment on lines
+547
to
+550
| "@modelcontextprotocol/sdk": "^1.6.1", | ||
| "axios": "^1.7.9", | ||
| "zod": "^3.23.8" | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the minimum Zod version required by @modelcontextprotocol/sdk for compatibility with imports from "zod/v3", and is zod ^3.23.8 compatible?
💡 Result:
@modelcontextprotocol/sdk requires Zod ≥ 3.25.0 to be compatible with subpath imports like zod/v3 (the SDK uses zod/v4 internally but is intended to stay compatible with projects that have Zod v3.25+ installed). [1][2]
So, zod@^3.23.8 is not compatible with import ... from "zod/v3" because that subpath support starts at 3.25.0. [2]
Sources: [1] [2]
🏁 Script executed:
# Check if the first file exists and read the relevant lines
if [ -f ".agents/skills/mcp-builder/reference/node_mcp_server.md" ]; then
echo "=== File exists: node_mcp_server.md ==="
wc -l ".agents/skills/mcp-builder/reference/node_mcp_server.md"
echo ""
echo "=== Lines 540-555 ==="
sed -n '540,555p' ".agents/skills/mcp-builder/reference/node_mcp_server.md"
else
echo "File not found: .agents/skills/mcp-builder/reference/node_mcp_server.md"
fiRepository: hypha-dao/hypha-web
Length of output: 495
🏁 Script executed:
# Check if the second file exists and read lines 104-107
if [ -f ".agents/skills/hypha-web-mcp/SKILL.md" ]; then
echo "=== File exists: SKILL.md ==="
wc -l ".agents/skills/hypha-web-mcp/SKILL.md"
echo ""
echo "=== Lines 100-110 ==="
sed -n '100,110p' ".agents/skills/hypha-web-mcp/SKILL.md"
else
echo "File not found: .agents/skills/hypha-web-mcp/SKILL.md"
fiRepository: hypha-dao/hypha-web
Length of output: 458
Update the Zod version in the package example to avoid MCP setup breakage.
Line 549 pins zod to ^3.23.8, which is incompatible with MCP SDK's requirement for zod >= 3.25.0 (needed for subpath imports like zod/v3). This also conflicts with .agents/skills/hypha-web-mcp/SKILL.md, which documents requiring zod >= 3.25.
Proposed doc fix
"dependencies": {
"@modelcontextprotocol/sdk": "^1.6.1",
"axios": "^1.7.9",
- "zod": "^3.23.8"
+ "zod": "^3.25.0"
},📝 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.
Suggested change
| "@modelcontextprotocol/sdk": "^1.6.1", | |
| "axios": "^1.7.9", | |
| "zod": "^3.23.8" | |
| }, | |
| "@modelcontextprotocol/sdk": "^1.6.1", | |
| "axios": "^1.7.9", | |
| "zod": "^3.25.0" | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/reference/node_mcp_server.md around lines 547 -
550, The package example pins "zod" to "^3.23.8", which is incompatible with the
MCP SDK requirement for zod >= 3.25.0 (needed for subpath imports such as
"zod/v3"); update the dependency entry that currently reads "zod": "^3.23.8" to
a compatible range (e.g., "zod": "^3.25.0" or ">=3.25.0") so the example aligns
with the MCP SDK and the SKILL.md requirement.
Comment on lines
+532
to
+541
| @mcp.resource("file://documents/{name}") | ||
| async def get_document(name: str) -> str: | ||
| '''Expose documents as MCP resources. | ||
|
|
||
| Resources are useful for static or semi-static data that doesn't | ||
| require complex parameters. They use URI templates for flexible access. | ||
| ''' | ||
| document_path = f"./docs/{name}" | ||
| with open(document_path, "r") as f: | ||
| return f.read() |
Contributor
There was a problem hiding this comment.
The resource example is path-traversal prone.
Interpolating {name} directly into ./docs/{name} lets copied code read ../../... outside the intended docs directory. Since this is sample code in the guide, it should resolve against a fixed base path and reject escapes.
🔒 Proposed fix
`@mcp.resource`("file://documents/{name}")
async def get_document(name: str) -> str:
'''Expose documents as MCP resources.
Resources are useful for static or semi-static data that doesn't
require complex parameters. They use URI templates for flexible access.
'''
- document_path = f"./docs/{name}"
- with open(document_path, "r") as f:
- return f.read()
+ from pathlib import Path
+
+ docs_dir = Path("./docs").resolve()
+ document_path = (docs_dir / name).resolve()
+ if docs_dir not in document_path.parents:
+ raise ValueError("Invalid document name")
+ return document_path.read_text(encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/reference/python_mcp_server.md around lines 532 -
541, The get_document resource is vulnerable to path traversal by interpolating
name into "./docs/{name}"; fix by resolving the requested file against a fixed
base directory (e.g., base = Path("./docs").resolve()) and constructing the
target Path from name, then calling .resolve() and verifying the resolved path
is inside the base (using Path.is_relative_to or comparing parents) before
opening; if the check fails, raise an exception or return a 400/404. Update
get_document and the document_path handling to perform these checks and refuse
escapes like "../".
Comment on lines
+67
to
+70
| async def call_tool(self, tool_name: str, arguments: dict[str, Any]) -> Any: | ||
| """Call a tool on the MCP server with provided arguments.""" | ||
| result = await self.session.call_tool(tool_name, arguments=arguments) | ||
| return result.content |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | grep -E "(connections|evaluation)" | head -20Repository: hypha-dao/hypha-web
Length of output: 165
🏁 Script executed:
fd -t f "connections.py"Repository: hypha-dao/hypha-web
Length of output: 45
🏁 Script executed:
fd -t f "evaluation.py"Repository: hypha-dao/hypha-web
Length of output: 45
🏁 Script executed:
cat -n ".agents/skills/mcp-builder/scripts/connections.py"Repository: hypha-dao/hypha-web
Length of output: 5994
🏁 Script executed:
cat -n ".agents/skills/mcp-builder/scripts/evaluation.py"Repository: hypha-dao/hypha-web
Length of output: 15236
🏁 Script executed:
python3 << 'EOF'
# Let's try to understand what the MCP SDK's CallToolResult looks like
import subprocess
result = subprocess.run(['pip', 'show', 'mcp'], capture_output=True, text=True)
print(result.stdout)
EOFRepository: hypha-dao/hypha-web
Length of output: 433
🏁 Script executed:
# Search for any test files or additional context about call_tool
fd -t f "test" -o "spec" | head -10Repository: hypha-dao/hypha-web
Length of output: 190
🏁 Script executed:
# Check if there are any imports or documentation about CallToolResult
rg "CallToolResult" --type pyRepository: hypha-dao/hypha-web
Length of output: 211
🏁 Script executed:
# Look for any example usage or error handling in the codebase
rg "call_tool" --type py -B 2 -A 5Repository: hypha-dao/hypha-web
Length of output: 211
🏁 Script executed:
grep -r "call_tool" . --include="*.py"Repository: hypha-dao/hypha-web
Length of output: 452
🏁 Script executed:
grep -r "CallToolResult" . --include="*.py"Repository: hypha-dao/hypha-web
Length of output: 45
🏁 Script executed:
# Look for any existing normalization of SDK results
grep -r "model_dump\|json.dumps" . --include="*.py" -B 2 -A 2Repository: hypha-dao/hypha-web
Length of output: 1167
🏁 Script executed:
python3 << 'EOF'
# Let's inspect the MCP SDK to understand the CallToolResult structure
import sys
sys.path.insert(0, '/usr/local/lib/python3.11/dist-packages')
try:
from mcp.types import CallToolResult
import inspect
print("CallToolResult signature:")
print(inspect.signature(CallToolResult))
print("\nCallToolResult class definition:")
print(inspect.getsource(CallToolResult))
except Exception as e:
print(f"Error: {e}")
# Alternative: try to find the class definition
try:
from mcp import types
print("\nAvailable types in mcp.types:")
for name in dir(types):
if 'Content' in name or 'Result' in name:
print(f" {name}")
except Exception as e:
print(f"Error: {e}")
EOFRepository: hypha-dao/hypha-web
Length of output: 1420
🏁 Script executed:
# Try to find MCP SDK source in site-packages
find /usr/local/lib/python3.11/dist-packages/mcp -name "*.py" | xargs grep -l "CallToolResult\|class.*Result" | head -5Repository: hypha-dao/hypha-web
Length of output: 429
🏁 Script executed:
# Check what content blocks look like in MCP SDK
python3 << 'EOF'
import sys
sys.path.insert(0, '/usr/local/lib/python3.11/dist-packages')
try:
from mcp.types import TextContent, CallToolResult
print("TextContent fields:")
print(TextContent.__annotations__)
# Try to see if model_dump is available
tc = TextContent(type="text", text="test")
if hasattr(tc, 'model_dump'):
print("\nmodel_dump available: True")
print("Sample output:", tc.model_dump())
else:
print("\nmodel_dump available: False")
except Exception as e:
print(f"Error: {e}")
import traceback
traceback.print_exc()
EOFRepository: hypha-dao/hypha-web
Length of output: 332
🏁 Script executed:
python3 << 'EOF'
import json
import sys
sys.path.insert(0, '/usr/local/lib/python3.11/dist-packages')
from mcp.types import TextContent
# Try to json.dumps a list of Pydantic models
content = [TextContent(type="text", text="Hello world")]
print("Content type:", type(content))
print("First item type:", type(content[0]))
try:
result = json.dumps(content)
print("json.dumps succeeded:", result)
except TypeError as e:
print("json.dumps FAILED with TypeError:")
print(f" {e}")
# Now try with model_dump()
print("\nWith model_dump():")
try:
dumped = [block.model_dump(mode="json") if hasattr(block, "model_dump") else block for block in content]
result = json.dumps(dumped)
print("json.dumps succeeded:", result)
except TypeError as e:
print("json.dumps FAILED with TypeError:")
print(f" {e}")
EOFRepository: hypha-dao/hypha-web
Length of output: 347
🏁 Script executed:
# Also check what the actual exception would be in the evaluation harness
python3 << 'EOF'
import json
import sys
sys.path.insert(0, '/usr/local/lib/python3.11/dist-packages')
from mcp.types import TextContent, CallToolResult
# Simulate what happens in evaluation.py line 117
tool_result = [TextContent(type="text", text="Result")]
print("Simulating evaluation.py line 117:")
print(f"tool_result = {tool_result}")
print(f"isinstance(tool_result, (dict, list)) = {isinstance(tool_result, (dict, list))}")
if isinstance(tool_result, (dict, list)):
try:
tool_response = json.dumps(tool_result)
print(f"SUCCESS: {tool_response}")
except Exception as e:
print(f"FAILED: {type(e).__name__}: {e}")
else:
tool_response = str(tool_result)
print(f"Fell back to str(): {tool_response}")
EOFRepository: hypha-dao/hypha-web
Length of output: 298
Normalize tool results to JSON-serializable data before returning from the abstraction layer.
The Python SDK's CallToolResult.content contains Pydantic model objects (e.g., TextContent, ImageContent) that are not directly JSON-serializable. When evaluation.py attempts json.dumps() on this list, it raises TypeError: Object of type TextContent is not JSON serializable, causing normal tool calls to be caught as execution errors. Normalize the result to primitives at the source.
🔧 Proposed normalization
async def call_tool(self, tool_name: str, arguments: dict[str, Any]) -> Any:
"""Call a tool on the MCP server with provided arguments."""
result = await self.session.call_tool(tool_name, arguments=arguments)
- return result.content
+ return {
+ "is_error": result.isError,
+ "structured_content": result.structuredContent,
+ "content": [
+ block.model_dump(mode="json") if hasattr(block, "model_dump") else block
+ for block in result.content
+ ],
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/scripts/connections.py around lines 67 - 70, The
call_tool method currently returns result.content which may contain Pydantic
model objects (e.g., TextContent, ImageContent) that aren't JSON-serializable;
update call_tool to normalize result.content into pure JSON-serializable
primitives before returning: after calling self.session.call_tool(tool_name,
arguments=arguments) inspect result.content and recursively convert any Pydantic
BaseModel instances (or lists/dicts containing them) to plain dicts/values
(using the model's dict()/model_dump() or an equivalent recursive encoder) so
that the final returned value contains only primitives (str, int, float, bool,
None, list, dict) and is safe for json.dumps; keep this logic inside call_tool
so callers like evaluation.py always receive serializable data.
| import sys | ||
| import time | ||
| import traceback | ||
| import xml.etree.ElementTree as ET |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .agents/skills/mcp-builder/scripts/evaluation.py | head -80Repository: hypha-dao/hypha-web
Length of output: 3498
🏁 Script executed:
# Check for dependency files
fd -e requirements.txt -e setup.py -e pyproject.toml -e setup.cfg -e Pipfile | head -20Repository: hypha-dao/hypha-web
Length of output: 45
🏁 Script executed:
# Search for dependency files more broadly
find . -maxdepth 3 -type f \( -name "requirements*.txt" -o -name "setup.py" -o -name "pyproject.toml" -o -name "setup.cfg" -o -name "Pipfile" \) 2>/dev/null | head -20Repository: hypha-dao/hypha-web
Length of output: 45
🏁 Script executed:
# Check what's in the mcp-builder directory
ls -la .agents/skills/mcp-builder/
# Search for defusedxml usage in the codebase
rg "defusedxml" .
# Search for how parse_evaluation_file is called
rg "parse_evaluation_file" .Repository: hypha-dao/hypha-web
Length of output: 421
🏁 Script executed:
# Get more of the evaluation.py file to see how parse_evaluation_file is called
wc -l .agents/skills/mcp-builder/scripts/evaluation.pyRepository: hypha-dao/hypha-web
Length of output: 115
🏁 Script executed:
# Read the rest of the file to find how parse_evaluation_file is called
sed -n '80,200p' .agents/skills/mcp-builder/scripts/evaluation.pyRepository: hypha-dao/hypha-web
Length of output: 3855
🏁 Script executed:
# Continue reading to find parse_evaluation_file call
sed -n '200,373p' .agents/skills/mcp-builder/scripts/evaluation.pyRepository: hypha-dao/hypha-web
Length of output: 6017
🏁 Script executed:
# Check if there's a requirements file in the scripts directory or parent
ls -la .agents/skills/mcp-builder/scripts/
# Check for any Python dependency files in the project root
find . -maxdepth 1 -type f -name "*.txt" -o -name "*.toml" -o -name "*.cfg" | head -20
# Search for any other ET usages in the evaluation.py file
rg "ET\." .agents/skills/mcp-builder/scripts/evaluation.pyRepository: hypha-dao/hypha-web
Length of output: 492
🏁 Script executed:
# Check the requirements.txt file
cat .agents/skills/mcp-builder/scripts/requirements.txtRepository: hypha-dao/hypha-web
Length of output: 91
🏁 Script executed:
# Verify defusedxml is a real and maintained package
web_search "defusedxml python package XXE protection"Repository: hypha-dao/hypha-web
Length of output: 113
🌐 Web query:
defusedxml python package XXE protection ElementTree
💡 Result:
defusedxml is a drop-in set of “hardened” XML parser modules for Python that actively blocks common XML attacks (including DTD/entity-based XML bombs like “billion laughs”, and external entity / XXE-style resolution attempts) by rejecting dangerous constructs and raising defusedxml exceptions. (pypi.org)
Using defusedxml with ElementTree (recommended)
Use the defused implementation instead of the stdlib one:
import defusedxml.ElementTree as ET
tree = ET.parse("untrusted.xml") # or ET.fromstring(xml_bytes)
root = tree.getroot()(pypi.org)
Stdlib-wide patching option (use with care)
defusedxml also provides defuse_stdlib() to monkey-patch the standard library XML modules (so imports like xml.etree.ElementTree get redirected to the defused versions):
import defusedxml
defusedxml.defuse_stdlib()
import xml.etree.ElementTree as ET # now uses defusedxml's safe implementation(pypi.org)
Note on XXE + stdlib xml.etree.ElementTree
Recent Python 3 stdlib XML parsers generally don’t expand external entities by default, but security tooling still commonly recommends defusedxml as a robust, explicit defense (and for protection against other XML parser attacks, especially entity-expansion DoS). (codeql.github.com)
Citations:
- 1: https://pypi.org/project/defusedxml/?utm_source=openai
- 2: https://pypi.org/project/defusedxml/?utm_source=openai
- 3: https://pypi.org/project/defusedxml/?utm_source=openai
- 4: https://codeql.github.com/codeql-query-help/python/py-xxe/?utm_source=openai
Use a hardened XML parser for the user-supplied evaluation file.
This script parses arbitrary XML files provided via the --eval_file CLI argument. Using xml.etree.ElementTree exposes this to XXE and entity-expansion DoS attacks. Replace with defusedxml.ElementTree, which actively blocks these attack vectors.
Proposed fix
-import xml.etree.ElementTree as ET
+from defusedxml import ElementTree as ETAlso add defusedxml to requirements.txt.
📝 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.
Suggested change
| import xml.etree.ElementTree as ET | |
| from defusedxml import ElementTree as ET |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/scripts/evaluation.py at line 13, The script
currently imports xml.etree.ElementTree (import xml.etree.ElementTree as ET)
which is vulnerable to XXE/entity expansion; replace that import with
defusedxml.ElementTree (e.g., import defusedxml.ElementTree as ET) so the parser
blocks these attack vectors, and add defusedxml to requirements.txt so the
dependency is installed; update any references to ET in the file to use the
defusedxml-backed module (no other code changes required).
Comment on lines
+109
to
+145
| while response.stop_reason == "tool_use": | ||
| tool_use = next(block for block in response.content if block.type == "tool_use") | ||
| tool_name = tool_use.name | ||
| tool_input = tool_use.input | ||
|
|
||
| tool_start_ts = time.time() | ||
| try: | ||
| tool_result = await connection.call_tool(tool_name, tool_input) | ||
| tool_response = json.dumps(tool_result) if isinstance(tool_result, (dict, list)) else str(tool_result) | ||
| except Exception as e: | ||
| tool_response = f"Error executing tool {tool_name}: {str(e)}\n" | ||
| tool_response += traceback.format_exc() | ||
| tool_duration = time.time() - tool_start_ts | ||
|
|
||
| if tool_name not in tool_metrics: | ||
| tool_metrics[tool_name] = {"count": 0, "durations": []} | ||
| tool_metrics[tool_name]["count"] += 1 | ||
| tool_metrics[tool_name]["durations"].append(tool_duration) | ||
|
|
||
| messages.append({ | ||
| "role": "user", | ||
| "content": [{ | ||
| "type": "tool_result", | ||
| "tool_use_id": tool_use.id, | ||
| "content": tool_response, | ||
| }] | ||
| }) | ||
|
|
||
| response = await asyncio.to_thread( | ||
| client.messages.create, | ||
| model=model, | ||
| max_tokens=4096, | ||
| system=EVALUATION_PROMPT, | ||
| messages=messages, | ||
| tools=tools, | ||
| ) | ||
| messages.append({"role": "assistant", "content": response.content}) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "evaluation.py" -path "*mcp-builder*" | head -20Repository: hypha-dao/hypha-web
Length of output: 113
🏁 Script executed:
wc -l ./.agents/skills/mcp-builder/scripts/evaluation.pyRepository: hypha-dao/hypha-web
Length of output: 117
🏁 Script executed:
sed -n '100,150p' ./.agents/skills/mcp-builder/scripts/evaluation.pyRepository: hypha-dao/hypha-web
Length of output: 1800
🏁 Script executed:
sed -n '70,160p' ./.agents/skills/mcp-builder/scripts/evaluation.pyRepository: hypha-dao/hypha-web
Length of output: 2870
🌐 Web query:
Claude API multiple tool_use blocks in single message response
💡 Result:
Yes—Claude can return multiple tool_use content blocks in a single assistant message (often to run tools in parallel). The API will set stop_reason: "tool_use" and include one or more tool_use blocks in message.content. [1]
How to handle it (client tools):
- Iterate over
response.contentand collect every block wheretype: "tool_use"(each has its ownid,name, andinput). [1] - Execute each tool call in your code.
- Send a new
usermessage whosecontentcontains atool_resultblock for eachtool_use, using the matchingtool_use_id(= the tool useid). [1] - Formatting rules: tool results must come immediately after the assistant’s tool-use message in the history, and within that
usermessage, alltool_resultblocks must come before any text (otherwise you can get a 400 error). [1]
Server tools (e.g., web search) can also involve multiple tool calls, but they’re executed in a server-side loop and results are incorporated automatically (you may see pause_turn if the loop needs continuation). [2]
Sources:
[1] Anthropic docs – Implement tool use (docs.anthropic.com)
[2] Anthropic docs – Tool use overview (platform.claude.com)
Citations:
- 1: https://docs.anthropic.com/en/docs/agents-and-tools/tool-use/implement-tool-use
- 2: https://platform.claude.com/docs/en/agents-and-tools/tool-use/overview
🏁 Script executed:
grep -n "max_turn\|turn.*limit\|exceeded.*turn" ./.agents/skills/mcp-builder/scripts/evaluation.pyRepository: hypha-dao/hypha-web
Length of output: 45
🏁 Script executed:
grep -n "agent_loop" ./.agents/skills/mcp-builder/scripts/evaluation.pyRepository: hypha-dao/hypha-web
Length of output: 192
Handle every tool_use block in the turn and cap the loop.
The current code uses next() to extract only the first tool_use block from response.content and immediately re-prompts, which drops valid parallel tool calls. According to Anthropic's API design, Claude can return multiple tool_use blocks in a single assistant message, and all resulting tool executions must be collected and sent back in a single user message with multiple tool_result blocks—not one at a time.
Additionally, the loop has no maximum turn guard, allowing unbounded execution if a tool continues returning stop_reason == "tool_use".
Collect all tool_use blocks per turn into one tool_result message and add a turn ceiling to prevent runaway loops.
🔁 Proposed fix
- tool_metrics = {}
+ tool_metrics = {}
+ max_turns = 50
+ turns = 0
while response.stop_reason == "tool_use":
- tool_use = next(block for block in response.content if block.type == "tool_use")
- tool_name = tool_use.name
- tool_input = tool_use.input
-
- tool_start_ts = time.time()
- try:
- tool_result = await connection.call_tool(tool_name, tool_input)
- tool_response = json.dumps(tool_result) if isinstance(tool_result, (dict, list)) else str(tool_result)
- except Exception as e:
- tool_response = f"Error executing tool {tool_name}: {str(e)}\n"
- tool_response += traceback.format_exc()
- tool_duration = time.time() - tool_start_ts
-
- if tool_name not in tool_metrics:
- tool_metrics[tool_name] = {"count": 0, "durations": []}
- tool_metrics[tool_name]["count"] += 1
- tool_metrics[tool_name]["durations"].append(tool_duration)
-
- messages.append({
- "role": "user",
- "content": [{
- "type": "tool_result",
- "tool_use_id": tool_use.id,
- "content": tool_response,
- }]
- })
+ turns += 1
+ if turns > max_turns:
+ raise RuntimeError(f"Exceeded {max_turns} tool rounds for question: {question}")
+
+ tool_results = []
+ for tool_use in [block for block in response.content if block.type == "tool_use"]:
+ tool_name = tool_use.name
+ tool_input = tool_use.input
+
+ tool_start_ts = time.time()
+ try:
+ tool_result = await connection.call_tool(tool_name, tool_input)
+ tool_response = (
+ json.dumps(tool_result)
+ if isinstance(tool_result, (dict, list))
+ else str(tool_result)
+ )
+ except Exception as e:
+ tool_response = f"Error executing tool {tool_name}: {e!s}\n"
+ tool_response += traceback.format_exc()
+ tool_duration = time.time() - tool_start_ts
+
+ if tool_name not in tool_metrics:
+ tool_metrics[tool_name] = {"count": 0, "durations": []}
+ tool_metrics[tool_name]["count"] += 1
+ tool_metrics[tool_name]["durations"].append(tool_duration)
+
+ tool_results.append({
+ "type": "tool_result",
+ "tool_use_id": tool_use.id,
+ "content": tool_response,
+ })
+
+ messages.append({"role": "user", "content": tool_results})🧰 Tools
🪛 Ruff (0.15.6)
[warning] 118-118: Do not catch blind exception: Exception
(BLE001)
[warning] 119-119: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/scripts/evaluation.py around lines 109 - 145, The
loop extracts only the first tool_use via next() and re-prompts, dropping
parallel tool calls and allowing an unbounded loop; change it to collect all
tool_use blocks in response.content (e.g., [b for b in response.content if
b.type == "tool_use"]) and iterate over that list to execute each tool (using
connection.call_tool), update tool_metrics for each tool, and build a single
user message whose content is a list of multiple
{"type":"tool_result","tool_use_id":..., "content":...} entries appended once to
messages before calling client.messages.create; also add a turn counter /
MAX_TURNS guard around the while response.stop_reason == "tool_use" loop and
break with a clear error/timeout if the ceiling is reached to prevent runaway
execution.
Comment on lines
+2
to
+20
| <qa_pair> | ||
| <question>Calculate the compound interest on $10,000 invested at 5% annual interest rate, compounded monthly for 3 years. What is the final amount in dollars (rounded to 2 decimal places)?</question> | ||
| <answer>11614.72</answer> | ||
| </qa_pair> | ||
| <qa_pair> | ||
| <question>A projectile is launched at a 45-degree angle with an initial velocity of 50 m/s. Calculate the total distance (in meters) it has traveled from the launch point after 2 seconds, assuming g=9.8 m/s². Round to 2 decimal places.</question> | ||
| <answer>87.25</answer> | ||
| </qa_pair> | ||
| <qa_pair> | ||
| <question>A sphere has a volume of 500 cubic meters. Calculate its surface area in square meters. Round to 2 decimal places.</question> | ||
| <answer>304.65</answer> | ||
| </qa_pair> | ||
| <qa_pair> | ||
| <question>Calculate the population standard deviation of this dataset: [12, 15, 18, 22, 25, 30, 35]. Round to 2 decimal places.</question> | ||
| <answer>7.61</answer> | ||
| </qa_pair> | ||
| <qa_pair> | ||
| <question>Calculate the pH of a solution with a hydrogen ion concentration of 3.5 × 10^-5 M. Round to 2 decimal places.</question> | ||
| <answer>4.46</answer> |
Contributor
There was a problem hiding this comment.
This sample never proves the server's tools are useful.
Every prompt here is a closed-form math/physics calculation, so Claude can answer them from the base model without touching the MCP server. If this file is meant as the example evaluation, it should use multi-hop questions that require tool calls; otherwise rename it as a smoke-test fixture.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/scripts/example_evaluation.xml around lines 2 -
20, The QA pairs in example_evaluation.xml currently contain only closed-form
math problems (each within <qa_pair> tags) that the base model can solve without
MCP tool calls; update the file so the example evaluation actually exercises the
server by replacing or augmenting these <qa_pair> entries with multi-hop
questions that require explicit tool usage (e.g., retrieval + calculation,
API-backed data lookup, sequential tool-chained steps, or simulation requests)
or, if the intent is only simple smoke tests, move/rename this file to a
smoke-test fixture; ensure each revised <qa_pair> clearly requires a tool call
and references the expected tool outputs in the answer field.
plitzenberger
force-pushed
the
litzi/feat/mcp-server
branch
from
58dbc6b to
f3c229f
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
.agents/skills/hypha-web-mcp/SKILL.md (1)
51-53: Optional readability tweak in response validation bullets.Three consecutive bullets start with “If”; consider varying phrasing to reduce repetition and improve scanability.
✍️ Suggested wording tweak
-- If `structuredContent.found` is `true`, use `structuredContent.space`. -- If `false`, report clearly that slug was not found. -- If `isError` is `true`, diagnose with troubleshooting section. +- When `structuredContent.found` is `true`, use `structuredContent.space`. +- Otherwise, report clearly that slug was not found. +- For `isError: true`, diagnose using the troubleshooting section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/hypha-web-mcp/SKILL.md around lines 51 - 53, Revise the three bullets in SKILL.md that start with "If" to improve readability and reduce repetition: keep the same checks for structuredContent.found, structuredContent.space, and isError but change the phrasing so each bullet begins differently (e.g., "When structuredContent.found is true, use structuredContent.space", "When the slug is not found, report this clearly", and "For errors (isError is true), diagnose using the troubleshooting section"), ensuring the logic and intent remain identical to the original lines..agents/skills/mcp-builder/reference/evaluation.md (1)
476-499: Add language specifier to fenced code block.The command-line options block lacks a language identifier, triggering MD040.
🔧 Proposed fix
-``` +```text usage: evaluation.py [-h] [-t {stdio,sse,http}] [-m MODEL] [-c COMMAND]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/reference/evaluation.md around lines 476 - 499, The fenced code block showing the CLI usage lacks a language specifier (causing MD040); update the opening triple-backtick for the usage block in evaluation.md to include a language tag (e.g., ```text) so the block becomes "```text" before the usage lines (the CLI usage block that begins with "usage: evaluation.py ..." and lists the -t, -m, -c, -a, -e, -u, -H, -o options)..agents/skills/mcp-builder/scripts/evaluation.py (1)
269-269: Addstrict=Truetozip()call.Using
zip()without explicitstrict=can silently drop items if lists have different lengths. Sinceqa_pairsandresultsshould always match, usestrict=Trueto catch mismatches.🔧 Proposed fix
- for i, (qa_pair, result) in enumerate(zip(qa_pairs, results)) + for i, (qa_pair, result) in enumerate(zip(qa_pairs, results, strict=True))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/scripts/evaluation.py at line 269, In the loop that iterates with "for i, (qa_pair, result) in enumerate(zip(qa_pairs, results))" update the zip call to pass strict=True so mismatched lengths raise an error; locate that for-loop in evaluation.py and change the zip invocation to use strict=True (ensuring qa_pairs and results must be the same length) and run tests to confirm no regressions..agents/skills/mcp-builder/scripts/connections.py (1)
76-76: Use explicit| Nonefor optional parameters.Ruff flags implicit
Optionalusage (PEP 484). Parameters with= Nonedefaults should use explicit union types.🔧 Proposed fix
- def __init__(self, command: str, args: list[str] = None, env: dict[str, str] = None): + def __init__(self, command: str, args: list[str] | None = None, env: dict[str, str] | None = None):Apply similar changes to
MCPConnectionSSE.__init__(line 91),MCPConnectionHTTP.__init__(line 103), andcreate_connectionparameters (lines 114-118).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/scripts/connections.py at line 76, Update the optional parameter type annotations to use explicit union with None: in MCPConnection.__init__ change args: list[str] and env: dict[str, str] to args: list[str] | None and env: dict[str, str] | None; apply the same change in MCPConnectionSSE.__init__ and MCPConnectionHTTP.__init__; also update the create_connection function signature so any parameters that default to None (e.g., args, env, or other optional args) use the | None union form to satisfy PEP 484/ruff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/skills/hypha-web-mcp/SKILL.md:
- Around line 51-53: Revise the three bullets in SKILL.md that start with "If"
to improve readability and reduce repetition: keep the same checks for
structuredContent.found, structuredContent.space, and isError but change the
phrasing so each bullet begins differently (e.g., "When structuredContent.found
is true, use structuredContent.space", "When the slug is not found, report this
clearly", and "For errors (isError is true), diagnose using the troubleshooting
section"), ensuring the logic and intent remain identical to the original lines.
In @.agents/skills/mcp-builder/reference/evaluation.md:
- Around line 476-499: The fenced code block showing the CLI usage lacks a
language specifier (causing MD040); update the opening triple-backtick for the
usage block in evaluation.md to include a language tag (e.g., ```text) so the
block becomes "```text" before the usage lines (the CLI usage block that begins
with "usage: evaluation.py ..." and lists the -t, -m, -c, -a, -e, -u, -H, -o
options).
In @.agents/skills/mcp-builder/scripts/connections.py:
- Line 76: Update the optional parameter type annotations to use explicit union
with None: in MCPConnection.__init__ change args: list[str] and env: dict[str,
str] to args: list[str] | None and env: dict[str, str] | None; apply the same
change in MCPConnectionSSE.__init__ and MCPConnectionHTTP.__init__; also update
the create_connection function signature so any parameters that default to None
(e.g., args, env, or other optional args) use the | None union form to satisfy
PEP 484/ruff.
In @.agents/skills/mcp-builder/scripts/evaluation.py:
- Line 269: In the loop that iterates with "for i, (qa_pair, result) in
enumerate(zip(qa_pairs, results))" update the zip call to pass strict=True so
mismatched lengths raise an error; locate that for-loop in evaluation.py and
change the zip invocation to use strict=True (ensuring qa_pairs and results must
be the same length) and run tests to confirm no regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddccf83b-82f9-4e2b-a3b6-52e06ca643c1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.agents/skills/hypha-web-mcp/SKILL.md.agents/skills/mcp-builder/LICENSE.txt.agents/skills/mcp-builder/SKILL.md.agents/skills/mcp-builder/reference/evaluation.md.agents/skills/mcp-builder/reference/mcp_best_practices.md.agents/skills/mcp-builder/reference/node_mcp_server.md.agents/skills/mcp-builder/reference/python_mcp_server.md.agents/skills/mcp-builder/scripts/connections.py.agents/skills/mcp-builder/scripts/evaluation.py.agents/skills/mcp-builder/scripts/example_evaluation.xml.agents/skills/mcp-builder/scripts/requirements.txt.cursor/mcp.jsonapps/web/package.jsonapps/web/src/app/mcp/route.tsapps/web/src/middleware.tspackage.jsonskills-lock.json
✅ Files skipped from review due to trivial changes (6)
- .cursor/mcp.json
- .agents/skills/mcp-builder/scripts/requirements.txt
- .agents/skills/mcp-builder/scripts/example_evaluation.xml
- package.json
- skills-lock.json
- apps/web/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/app/mcp/route.ts
- apps/web/src/middleware.ts
plitzenberger
force-pushed
the
litzi/feat/mcp-server
branch
from
19d5530 to
5d5f086
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/app/mcp/route.ts`:
- Around line 12-15: The current logic in apps/web/src/app/mcp/route.ts uses
configuredToken (derived from HYPHA_MCP_AUTH_TOKEN) to "fail open" by returning
true when unset; change this to "fail closed" by refusing requests when
configuredToken is missing — update the branch that checks configuredToken to
deny access (e.g., return false or throw an HTTP 401/403) and log a clear error,
and ensure any downstream auth checks (in the same route handler or middleware
that uses configuredToken) treat an unset token as unauthorized rather than
allowing access.
In `@packages/mcp-tools/src/space/get-space-by-slug.ts`:
- Around line 39-44: The helper safeDateToISOString currently masks parse
failures by returning the epoch string; change it to surface invalid timestamps
instead: update safeDateToISOString to return a nullable string (string | null)
or throw on invalid input rather than returning new Date(0). Specifically, in
safeDateToISOString detect invalid dates with candidate.getTime() and return
null (or throw a descriptive Error) so callers of get-space-by-slug.ts (and any
callers of safeDateToISOString) can handle/propagate validation errors; update
the function signature and all usages accordingly (e.g., where
safeDateToISOString was used to populate timestamps) to accept/handle nulls or
handle the thrown error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 03eeb4fe-8981-403b-9edc-48648adf4a50
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.gitignoreapps/web/package.jsonapps/web/src/app/mcp/route.tspackages/mcp-tools/eslint.config.mjspackages/mcp-tools/package.jsonpackages/mcp-tools/src/index.tspackages/mcp-tools/src/space/get-space-by-slug.tspackages/mcp-tools/tsconfig.json
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- packages/mcp-tools/eslint.config.mjs
- packages/mcp-tools/package.json
- apps/web/package.json
- packages/mcp-tools/tsconfig.json
Comment on lines
+12
to
+15
| // Auth is optional by default to preserve local/dev velocity. | ||
| if (!configuredToken) { | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Auth is fail-open when token is missing.
Allowing all requests when HYPHA_MCP_AUTH_TOKEN is unset is risky for production exposure. Safer default is deny in production (or always deny when unset).
🔒 Suggested hardening
function isAuthorizedRequest(request: Request): boolean {
const configuredToken = process.env.HYPHA_MCP_AUTH_TOKEN;
- // Auth is optional by default to preserve local/dev velocity.
- if (!configuredToken) {
- return true;
- }
+ // Keep local/dev convenience, but fail closed in production.
+ if (!configuredToken) {
+ return process.env.NODE_ENV !== 'production';
+ }
const providedToken = request.headers.get(MCP_AUTH_HEADER);
return providedToken === configuredToken;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/app/mcp/route.ts` around lines 12 - 15, The current logic in
apps/web/src/app/mcp/route.ts uses configuredToken (derived from
HYPHA_MCP_AUTH_TOKEN) to "fail open" by returning true when unset; change this
to "fail closed" by refusing requests when configuredToken is missing — update
the branch that checks configuredToken to deny access (e.g., return false or
throw an HTTP 401/403) and log a clear error, and ensure any downstream auth
checks (in the same route handler or middleware that uses configuredToken) treat
an unset token as unauthorized rather than allowing access.
Comment on lines
+39
to
+44
| function safeDateToISOString(value: unknown): string { | ||
| const candidate = new Date(value as string | number | Date); | ||
| if (Number.isNaN(candidate.getTime())) { | ||
| return new Date(0).toISOString(); | ||
| } | ||
| return candidate.toISOString(); |
Contributor
There was a problem hiding this comment.
Avoid masking invalid timestamps as 1970-01-01T00:00:00.000Z.
Returning epoch on parse failure hides upstream data issues and emits misleading “valid” timestamps to clients. Prefer nullable timestamps (or fail fast) instead of silent coercion.
💡 Suggested fix
export const getSpaceBySlugOutputSchema = z
.object({
@@
- createdAt: z.string(),
- updatedAt: z.string(),
+ createdAt: z.string().nullable(),
+ updatedAt: z.string().nullable(),
@@
-function safeDateToISOString(value: unknown): string {
+function safeDateToISOString(value: unknown): string | null {
const candidate = new Date(value as string | number | Date);
if (Number.isNaN(candidate.getTime())) {
- return new Date(0).toISOString();
+ return null;
}
return candidate.toISOString();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mcp-tools/src/space/get-space-by-slug.ts` around lines 39 - 44, The
helper safeDateToISOString currently masks parse failures by returning the epoch
string; change it to surface invalid timestamps instead: update
safeDateToISOString to return a nullable string (string | null) or throw on
invalid input rather than returning new Date(0). Specifically, in
safeDateToISOString detect invalid dates with candidate.getTime() and return
null (or throw a descriptive Error) so callers of get-space-by-slug.ts (and any
callers of safeDateToISOString) can handle/propagate validation errors; update
the function signature and all usages accordingly (e.g., where
safeDateToISOString was used to populate timestamps) to accept/handle nulls or
handle the thrown error.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
.agents/skills/mcp-builder/reference/python_mcp_server.md (1)
532-541:⚠️ Potential issue | 🟠 MajorHarden the resource example against path traversal.
Line 539 interpolates untrusted
nameinto a filesystem path directly, allowing../escapes in copied implementations.🔒 Proposed doc-safe example
`@mcp.resource`("file://documents/{name}") async def get_document(name: str) -> str: @@ - document_path = f"./docs/{name}" - with open(document_path, "r") as f: - return f.read() + from pathlib import Path + + docs_dir = Path("./docs").resolve() + document_path = (docs_dir / name).resolve() + if docs_dir not in document_path.parents: + raise ValueError("Invalid document name") + return document_path.read_text(encoding="utf-8")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/reference/python_mcp_server.md around lines 532 - 541, The get_document resource currently builds document_path from untrusted name, enabling path traversal; update the get_document function to validate/sanitize name before opening files: reject or sanitize any input containing path separators or '..', or use pathlib.Path to join the base docs directory, call resolve() and assert the resolved path is inside the base docs directory (e.g., base_dir = Path("./docs").resolve(), target = (base_dir / name).resolve(), ensure target.is_file() and str(target).startswith(str(base_dir)) ) then open target; keep the function signature and returned string unchanged..agents/skills/mcp-builder/reference/node_mcp_server.md (2)
81-94:⚠️ Potential issue | 🟡 MinorAdd a language label to the project-structure code fence.
Line 81 opens an unlabeled fence; this commonly fails Markdown linting (MD040).
🧹 Suggested fix
-``` +```text {service}-mcp-server/ ├── package.json ... └── dist/ # Built JavaScript files (entry point: dist/index.js)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/reference/node_mcp_server.md around lines 81 -
94, The Markdown code fence that documents the project tree beginning with
"{service}-mcp-server/" is unlabeled and triggers MD040; update the opening
fence to include a language label (e.g., changetotext) for the block
that starts with "{service}-mcp-server/" so the snippet is lint-friendly and
preserves plain-text formatting.</details> --- `547-550`: _⚠️ Potential issue_ | _🟠 Major_ **Use a Zod version compatible with MCP SDK examples.** Line 549 pins `zod` to `^3.23.8`, which is inconsistent with the repo’s own MCP guidance that requires `zod >= 3.25` for MCP SDK compatibility patterns. <details> <summary>📦 Suggested dependency update</summary> ```diff "dependencies": { "@modelcontextprotocol/sdk": "^1.6.1", "axios": "^1.7.9", - "zod": "^3.23.8" + "zod": "^3.25.0" } ``` </details> ```web What is the minimum Zod version required for compatibility with `@modelcontextprotocol/sdk` imports/patterns involving zod/v3, and is zod ^3.23.8 compatible? ``` <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/reference/node_mcp_server.md around lines 547 - 550, The package lists zod as "^3.23.8", but MCP SDK patterns require zod >= 3.25; ^3.23.8 is therefore not compatible. Update the "zod" dependency in the shown dependency block to a minimum of "^3.25.0" (or a compatible newer v3.x range), so it aligns with `@modelcontextprotocol/sdk` expectations, then reinstall/update the lockfile (npm/yarn) to ensure the new version is used. ``` </details> </blockquote></details> <details> <summary>.agents/skills/mcp-builder/scripts/evaluation.py (2)</summary><blockquote> `13-13`: _⚠️ Potential issue_ | _🟠 Major_ **Use `defusedxml` to prevent XXE attacks.** The script parses user-supplied XML files via `xml.etree.ElementTree`, which is vulnerable to XML External Entity (XXE) and entity-expansion attacks. Replace with `defusedxml.ElementTree`. <details> <summary>🛡️ Proposed fix</summary> ```diff -import xml.etree.ElementTree as ET +from defusedxml import ElementTree as ET ``` Also ensure `defusedxml` is added to `requirements.txt`. </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/scripts/evaluation.py at line 13, Replace the vulnerable import xml.etree.ElementTree with defusedxml.ElementTree (i.e., change the import line in .agents/skills/mcp-builder/scripts/evaluation.py to use defusedxml.ElementTree as ET) so all subsequent uses of ET.parse/ET.fromstring/etc. use the safe parser; also add defusedxml to requirements.txt and run a quick scan of functions in this file that call ET.parse, ET.fromstring, or ET.ElementTree to confirm no API mismatches. ``` </details> --- `109-145`: _⚠️ Potential issue_ | _🟠 Major_ **Handle all `tool_use` blocks per turn and add a loop ceiling.** The current code uses `next()` to extract only the first `tool_use` block, dropping any parallel tool calls Claude may return. Additionally, there's no maximum turn guard, risking unbounded execution. <details> <summary>🔁 Proposed fix</summary> ```diff tool_metrics = {} + max_turns = 50 + turns = 0 while response.stop_reason == "tool_use": - tool_use = next(block for block in response.content if block.type == "tool_use") - tool_name = tool_use.name - tool_input = tool_use.input - - tool_start_ts = time.time() - try: - tool_result = await connection.call_tool(tool_name, tool_input) - tool_response = json.dumps(tool_result) if isinstance(tool_result, (dict, list)) else str(tool_result) - except Exception as e: - tool_response = f"Error executing tool {tool_name}: {str(e)}\n" - tool_response += traceback.format_exc() - tool_duration = time.time() - tool_start_ts - ... - messages.append({ - "role": "user", - "content": [{ - "type": "tool_result", - "tool_use_id": tool_use.id, - "content": tool_response, - }] - }) + turns += 1 + if turns > max_turns: + raise RuntimeError(f"Exceeded {max_turns} tool rounds for question: {question}") + + tool_results = [] + for tool_use in [b for b in response.content if b.type == "tool_use"]: + tool_name = tool_use.name + tool_input = tool_use.input + + tool_start_ts = time.time() + try: + tool_result = await connection.call_tool(tool_name, tool_input) + tool_response = ( + json.dumps(tool_result) + if isinstance(tool_result, (dict, list)) + else str(tool_result) + ) + except Exception as e: + tool_response = f"Error executing tool {tool_name}: {e!s}\n" + tool_response += traceback.format_exc() + tool_duration = time.time() - tool_start_ts + + if tool_name not in tool_metrics: + tool_metrics[tool_name] = {"count": 0, "durations": []} + tool_metrics[tool_name]["count"] += 1 + tool_metrics[tool_name]["durations"].append(tool_duration) + + tool_results.append({ + "type": "tool_result", + "tool_use_id": tool_use.id, + "content": tool_response, + }) + + messages.append({"role": "user", "content": tool_results}) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/scripts/evaluation.py around lines 109 - 145, Replace the single-use next(...) extraction with iterating over all blocks in response.content where block.type == "tool_use" so you handle multiple parallel tool calls per turn (process each tool_use: call connection.call_tool, record tool_metrics entry for tool_name, append the tool_result tool_result block to messages). Also add a ceiling counter (e.g., max_tool_turns or max_iterations) around the outer while response.stop_reason == "tool_use" loop to prevent unbounded turns; increment the counter each loop and break/raise when exceeded. Ensure you still rebuild response by calling client.messages.create with model, EVALUATION_PROMPT, messages, tools and append the assistant response to messages as before. ``` </details> </blockquote></details> <details> <summary>.agents/skills/mcp-builder/scripts/connections.py (1)</summary><blockquote> `67-70`: _⚠️ Potential issue_ | _🔴 Critical_ **Normalize tool results to JSON-serializable primitives.** `result.content` contains Pydantic model objects (e.g., `TextContent`) that are not directly JSON-serializable. When `evaluation.py` attempts `json.dumps()` on this list, it raises `TypeError`. Normalize the result at the source. <details> <summary>🔧 Proposed fix</summary> ```diff async def call_tool(self, tool_name: str, arguments: dict[str, Any]) -> Any: """Call a tool on the MCP server with provided arguments.""" result = await self.session.call_tool(tool_name, arguments=arguments) - return result.content + return { + "is_error": getattr(result, "isError", False), + "content": [ + block.model_dump(mode="json") if hasattr(block, "model_dump") else block + for block in result.content + ], + } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/scripts/connections.py around lines 67 - 70, The call_tool method returns result.content which may include Pydantic model instances (e.g., TextContent) that are not JSON-serializable; update call_tool to normalize/serialize the tool result to JSON-serializable primitives before returning (e.g., convert Pydantic models to dict via .dict() or use their .json()/jsonable conversion and recursively transform nested models/lists), so that callers like evaluation.py can safely call json.dumps on the returned value; modify the code around session.call_tool and result.content in call_tool to perform this normalization and return plain primitives. ``` </details> </blockquote></details> <details> <summary>.agents/skills/mcp-builder/reference/evaluation.md (1)</summary><blockquote> `59-62`: _⚠️ Potential issue_ | _🟡 Minor_ **Clarify "previous questions" to avoid contradiction with independence rule.** Line 47-48 states questions must be independent, but line 61 says "Each step should benefit from information found in previous questions." This appears to mean previous steps/sub-questions within the same multi-step question, not earlier independent questions. <details> <summary>📝 Suggested clarification</summary> ```diff - - Each step should benefit from information found in previous questions + - Each step should benefit from information found in previous steps within the same question ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/reference/evaluation.md around lines 59 - 62, Update the evaluation text to clarify that "previous questions" in the section titled "4. **Questions must require deep exploration**" refers to prior sub-steps within the same multi-step question, not earlier independent questions referenced by the "independence" rule; edit the sentence "Each step should benefit from information found in previous questions" to explicitly say "previous steps/sub-questions within the same multi-step question" and add a short parenthetical linking this to the independence requirement to avoid contradiction with the earlier rule about independent questions. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (4)</summary><blockquote> <details> <summary>.agents/skills/mcp-builder/scripts/evaluation.py (1)</summary><blockquote> `269-269`: **Add `strict=True` to `zip()` for defensive programming.** While `qa_pairs` and `results` should have matching lengths given the loop structure, adding `strict=True` catches mismatches early and prevents silent truncation. <details> <summary>♻️ Proposed fix</summary> ```diff - for i, (qa_pair, result) in enumerate(zip(qa_pairs, results)) + for i, (qa_pair, result) in enumerate(zip(qa_pairs, results, strict=True)) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/scripts/evaluation.py at line 269, In the loop that iterates with for i, (qa_pair, result) in enumerate(zip(qa_pairs, results)), add strict=True to the zip call (i.e., zip(qa_pairs, results, strict=True)) to ensure mismatched lengths raise an error instead of silently truncating; update the call site where qa_pairs and results are zipped and ensure the runtime uses Python 3.10+ (or adjust tests) so the strict parameter is supported. ``` </details> </blockquote></details> <details> <summary>.agents/skills/mcp-builder/reference/evaluation.md (1)</summary><blockquote> `476-499`: **Consider adding a language specifier to the fenced code block.** The CLI help output code block at line 476 lacks a language specifier. Adding `text` or `plaintext` would satisfy the markdownlint MD040 rule and improve rendering consistency. <details> <summary>📝 Suggested fix</summary> ```diff -``` +```text usage: evaluation.py [-h] [-t {stdio,sse,http}] [-m MODEL] [-c COMMAND] ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/reference/evaluation.md around lines 476 - 499,
The fenced code block showing the CLI help for evaluation.py is missing a
language specifier; update the opening fence for that block (the triple
backticks before the "usage: evaluation.py..." line) to include a language tag
such as text or plaintext (e.g., ```text) so Markdown lint rule MD040 is
satisfied and the help output renders consistently.</details> </blockquote></details> <details> <summary>.agents/skills/mcp-builder/scripts/connections.py (1)</summary><blockquote> `76-76`: **Use explicit `| None` type hints instead of implicit `Optional`.** PEP 484 prohibits implicit `Optional` (using `= None` without declaring the type as nullable). Use explicit union types for clarity and type checker compatibility. <details> <summary>♻️ Proposed fix for constructor signatures</summary> ```diff class MCPConnectionStdio(MCPConnection): - def __init__(self, command: str, args: list[str] = None, env: dict[str, str] = None): + def __init__(self, command: str, args: list[str] | None = None, env: dict[str, str] | None = None): class MCPConnectionSSE(MCPConnection): - def __init__(self, url: str, headers: dict[str, str] = None): + def __init__(self, url: str, headers: dict[str, str] | None = None): class MCPConnectionHTTP(MCPConnection): - def __init__(self, url: str, headers: dict[str, str] = None): + def __init__(self, url: str, headers: dict[str, str] | None = None): def create_connection( transport: str, - command: str = None, - args: list[str] = None, - env: dict[str, str] = None, - url: str = None, - headers: dict[str, str] = None, + command: str | None = None, + args: list[str] | None = None, + env: dict[str, str] | None = None, + url: str | None = None, + headers: dict[str, str] | None = None, ) -> MCPConnection: ``` </details> Also applies to: 91-91, 103-103, 112-118 <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In @.agents/skills/mcp-builder/scripts/connections.py at line 76, Update the constructor and any function signatures that use a default None without an explicit nullable type (e.g., in __init__ of the connections-related class where you have "command: str, args: list[str] = None, env: dict[str, str] = None") so that nullable parameters use explicit union types (e.g., "args: list[str] | None = None" and "env: dict[str, str] | None = None"); apply the same change to the other affected signatures referenced in the review (lines around the other constructors/functions at the symbols near lines noted: the other __init__/factory signatures at the same module) so all parameters with a default of None are declared as Type | None for PEP 484 compliance. ``` </details> </blockquote></details> <details> <summary>packages/mcp-tools/src/space/get-space-proposals-by-space-slug.ts (1)</summary><blockquote> `171-178`: **Clarify `totalOnchainTracked` semantics.** `totalOnchainTracked` sums `acceptedIds` + `rejectedIds` (from on-chain) + `onVotingIds` (derived from indexed documents not in accepted/rejected sets). The name implies purely on-chain data, but `onVotingIds` are inferred from indexed documents rather than fetched from on-chain. Consider renaming to `totalTracked` or adding a comment explaining this semantic. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@packages/mcp-tools/src/space/get-space-proposals-by-space-slug.ts` around lines 171 - 178, The field counts.totalOnchainTracked is misleading because it sums acceptedIds + rejectedIds + onVotingIds even though onVotingIds are inferred from indexed proposals (variables: acceptedIds, rejectedIds, onVotingIds, proposals); change the name to counts.totalTracked (or similar) and update all usages of totalOnchainTracked to the new name, or alternatively add a concise inline comment next to the counts object explaining that totalOnchainTracked includes inferred on-voting IDs derived from indexed documents rather than only on-chain data so callers understand the semantics. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/mcp-builder/scripts/evaluation.py:
- Around line 147-151: response_text is set to None when no content block has a
text attribute which later breaks extract_xml_content(response, "response");
update the logic around response_text in evaluation.py to handle the
missing-text case by either (a) providing a safe fallback string (e.g., empty
string or a clearly marked placeholder) before returning so extract_xml_content
always receives a string, or (b) raising a descriptive error (ValueError) that
mentions the missing text when response.content contains no text blocks; locate
the assignment to response_text and the subsequent return near the end of the
function and make the fallback or explicit error there, referencing
response_text, response.content, and extract_xml_content to guide the change.
Duplicate comments:
In @.agents/skills/mcp-builder/reference/evaluation.md:
- Around line 59-62: Update the evaluation text to clarify that "previous
questions" in the section titled "4. Questions must require deep
exploration" refers to prior sub-steps within the same multi-step question,
not earlier independent questions referenced by the "independence" rule; edit
the sentence "Each step should benefit from information found in previous
questions" to explicitly say "previous steps/sub-questions within the same
multi-step question" and add a short parenthetical linking this to the
independence requirement to avoid contradiction with the earlier rule about
independent questions.In @.agents/skills/mcp-builder/reference/node_mcp_server.md:
- Around line 81-94: The Markdown code fence that documents the project tree
beginning with "{service}-mcp-server/" is unlabeled and triggers MD040; update
the opening fence to include a language label (e.g., changetotext) for
the block that starts with "{service}-mcp-server/" so the snippet is
lint-friendly and preserves plain-text formatting.- Around line 547-550: The package lists zod as "^3.23.8", but MCP SDK patterns
require zod >= 3.25; ^3.23.8 is therefore not compatible. Update the "zod"
dependency in the shown dependency block to a minimum of "^3.25.0" (or a
compatible newer v3.x range), so it aligns with@modelcontextprotocol/sdk
expectations, then reinstall/update the lockfile (npm/yarn) to ensure the new
version is used.In @.agents/skills/mcp-builder/reference/python_mcp_server.md:
- Around line 532-541: The get_document resource currently builds document_path
from untrusted name, enabling path traversal; update the get_document function
to validate/sanitize name before opening files: reject or sanitize any input
containing path separators or '..', or use pathlib.Path to join the base docs
directory, call resolve() and assert the resolved path is inside the base docs
directory (e.g., base_dir = Path("./docs").resolve(), target = (base_dir /
name).resolve(), ensure target.is_file() and
str(target).startswith(str(base_dir)) ) then open target; keep the function
signature and returned string unchanged.In @.agents/skills/mcp-builder/scripts/connections.py:
- Around line 67-70: The call_tool method returns result.content which may
include Pydantic model instances (e.g., TextContent) that are not
JSON-serializable; update call_tool to normalize/serialize the tool result to
JSON-serializable primitives before returning (e.g., convert Pydantic models to
dict via .dict() or use their .json()/jsonable conversion and recursively
transform nested models/lists), so that callers like evaluation.py can safely
call json.dumps on the returned value; modify the code around session.call_tool
and result.content in call_tool to perform this normalization and return plain
primitives.In @.agents/skills/mcp-builder/scripts/evaluation.py:
- Line 13: Replace the vulnerable import xml.etree.ElementTree with
defusedxml.ElementTree (i.e., change the import line in
.agents/skills/mcp-builder/scripts/evaluation.py to use defusedxml.ElementTree
as ET) so all subsequent uses of ET.parse/ET.fromstring/etc. use the safe
parser; also add defusedxml to requirements.txt and run a quick scan of
functions in this file that call ET.parse, ET.fromstring, or ET.ElementTree to
confirm no API mismatches.- Around line 109-145: Replace the single-use next(...) extraction with
iterating over all blocks in response.content where block.type == "tool_use" so
you handle multiple parallel tool calls per turn (process each tool_use: call
connection.call_tool, record tool_metrics entry for tool_name, append the
tool_result tool_result block to messages). Also add a ceiling counter (e.g.,
max_tool_turns or max_iterations) around the outer while response.stop_reason ==
"tool_use" loop to prevent unbounded turns; increment the counter each loop and
break/raise when exceeded. Ensure you still rebuild response by calling
client.messages.create with model, EVALUATION_PROMPT, messages, tools and append
the assistant response to messages as before.
Nitpick comments:
In @.agents/skills/mcp-builder/reference/evaluation.md:
- Around line 476-499: The fenced code block showing the CLI help for
evaluation.py is missing a language specifier; update the opening fence for that
block (the triple backticks before the "usage: evaluation.py..." line) to
include a language tag such as text or plaintext (e.g., ```text) so Markdown
lint rule MD040 is satisfied and the help output renders consistently.In @.agents/skills/mcp-builder/scripts/connections.py:
- Line 76: Update the constructor and any function signatures that use a default
None without an explicit nullable type (e.g., in init of the
connections-related class where you have "command: str, args: list[str] = None,
env: dict[str, str] = None") so that nullable parameters use explicit union
types (e.g., "args: list[str] | None = None" and "env: dict[str, str] | None =
None"); apply the same change to the other affected signatures referenced in the
review (lines around the other constructors/functions at the symbols near lines
noted: the other init/factory signatures at the same module) so all
parameters with a default of None are declared as Type | None for PEP 484
compliance.In @.agents/skills/mcp-builder/scripts/evaluation.py:
- Line 269: In the loop that iterates with for i, (qa_pair, result) in
enumerate(zip(qa_pairs, results)), add strict=True to the zip call (i.e.,
zip(qa_pairs, results, strict=True)) to ensure mismatched lengths raise an error
instead of silently truncating; update the call site where qa_pairs and results
are zipped and ensure the runtime uses Python 3.10+ (or adjust tests) so the
strict parameter is supported.In
@packages/mcp-tools/src/space/get-space-proposals-by-space-slug.ts:
- Around line 171-178: The field counts.totalOnchainTracked is misleading
because it sums acceptedIds + rejectedIds + onVotingIds even though onVotingIds
are inferred from indexed proposals (variables: acceptedIds, rejectedIds,
onVotingIds, proposals); change the name to counts.totalTracked (or similar) and
update all usages of totalOnchainTracked to the new name, or alternatively add a
concise inline comment next to the counts object explaining that
totalOnchainTracked includes inferred on-voting IDs derived from indexed
documents rather than only on-chain data so callers understand the semantics.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `4eea4b39-416a-4fc8-9fe5-72d3ebfe2a4b` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 19d5530de9ebd87adcbe5595e62433bcd20732a6 and 5d5f0866f80f7811d9a1a6da67920924cbe21a6e. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (27)</summary> * `.agents/AGENTS.md` * `.agents/references/competencies/mcp-engineering.md` * `.agents/roles/senior-mcp-engineer.base.md` * `.agents/skills/hypha-web-mcp/SKILL.md` * `.agents/skills/mcp-builder/LICENSE.txt` * `.agents/skills/mcp-builder/SKILL.md` * `.agents/skills/mcp-builder/reference/evaluation.md` * `.agents/skills/mcp-builder/reference/mcp_best_practices.md` * `.agents/skills/mcp-builder/reference/node_mcp_server.md` * `.agents/skills/mcp-builder/reference/python_mcp_server.md` * `.agents/skills/mcp-builder/scripts/connections.py` * `.agents/skills/mcp-builder/scripts/evaluation.py` * `.agents/skills/mcp-builder/scripts/example_evaluation.xml` * `.agents/skills/mcp-builder/scripts/requirements.txt` * `.cursor/mcp.json` * `.gitignore` * `apps/web/package.json` * `apps/web/src/app/mcp/route.ts` * `apps/web/src/middleware.ts` * `package.json` * `packages/mcp-tools/eslint.config.mjs` * `packages/mcp-tools/package.json` * `packages/mcp-tools/src/index.ts` * `packages/mcp-tools/src/space/get-space-by-slug.ts` * `packages/mcp-tools/src/space/get-space-proposals-by-space-slug.ts` * `packages/mcp-tools/tsconfig.json` * `skills-lock.json` </details> <details> <summary>✅ Files skipped from review due to trivial changes (11)</summary> * .gitignore * .cursor/mcp.json * apps/web/package.json * packages/mcp-tools/eslint.config.mjs * .agents/skills/mcp-builder/scripts/requirements.txt * .agents/skills/mcp-builder/scripts/example_evaluation.xml * packages/mcp-tools/tsconfig.json * packages/mcp-tools/package.json * skills-lock.json * .agents/references/competencies/mcp-engineering.md * package.json </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * packages/mcp-tools/src/index.ts * packages/mcp-tools/src/space/get-space-by-slug.ts * apps/web/src/app/mcp/route.ts </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Comment on lines
+147
to
+151
| response_text = next( | ||
| (block.text for block in response.content if hasattr(block, "text")), | ||
| None, | ||
| ) | ||
| return response_text, tool_metrics |
Contributor
There was a problem hiding this comment.
Handle case where no text block exists in the response.
If response.content contains no blocks with a text attribute, response_text becomes None, which then causes extract_xml_content(response, "response") at line 168 to fail since it expects a string. Consider providing a fallback or raising a descriptive error.
🛡️ Proposed fix
response_text = next(
(block.text for block in response.content if hasattr(block, "text")),
- None,
+ "",
)
+ if not response_text:
+ return "", tool_metrics
return response_text, tool_metrics📝 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.
Suggested change
| response_text = next( | |
| (block.text for block in response.content if hasattr(block, "text")), | |
| None, | |
| ) | |
| return response_text, tool_metrics | |
| response_text = next( | |
| (block.text for block in response.content if hasattr(block, "text")), | |
| "", | |
| ) | |
| if not response_text: | |
| return "", tool_metrics | |
| return response_text, tool_metrics |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/mcp-builder/scripts/evaluation.py around lines 147 - 151,
response_text is set to None when no content block has a text attribute which
later breaks extract_xml_content(response, "response"); update the logic around
response_text in evaluation.py to handle the missing-text case by either (a)
providing a safe fallback string (e.g., empty string or a clearly marked
placeholder) before returning so extract_xml_content always receives a string,
or (b) raising a descriptive error (ValueError) that mentions the missing text
when response.content contains no text blocks; locate the assignment to
response_text and the subsequent return near the end of the function and make
the fallback or explicit error there, referencing response_text,
response.content, and extract_xml_content to guide the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/mcp-tools/src/space/get-people-by-space-slug.ts (1)
67-73: Consider extractingsafeDateToISOStringto a shared utility.This helper is duplicated verbatim in
get-spaces.tsandget-documents-by-space-slug.ts. Extracting it to a shared module (e.g.,packages/mcp-tools/src/utils/date.ts) would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mcp-tools/src/space/get-people-by-space-slug.ts` around lines 67 - 73, The helper safeDateToISOString is duplicated across get-people-by-space-slug.ts, get-spaces.ts and get-documents-by-space-slug.ts; extract it into a shared module (e.g., create packages/mcp-tools/src/utils/date.ts) that exports function safeDateToISOString, update each file to import { safeDateToISOString } from that module, and remove the inline implementations so all three modules use the single shared utility.packages/mcp-tools/src/space/get-spaces.ts (1)
68-74: Same duplication note as other files.
safeDateToISOStringis duplicated here. Consider consolidating into a shared utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mcp-tools/src/space/get-spaces.ts` around lines 68 - 74, The function safeDateToISOString is duplicated here; remove this local implementation and import the canonical shared utility instead (replace the local safeDateToISOString definition in get-spaces.ts with an import from the shared util module), update any call sites in getSpaces (and related functions in this file) to use the imported safeDateToISOString, and ensure the shared utility export covers the same signature so behavior remains identical.packages/mcp-tools/src/space/get-documents-by-space-slug.ts (1)
83-89: Same duplication note.
safeDateToISOStringis duplicated. This is the third occurrence across the MCP tools files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mcp-tools/src/space/get-documents-by-space-slug.ts` around lines 83 - 89, safeDateToISOString is duplicated across MCP tools; extract it to a single shared utility and replace the duplicates with imports. Create a single exported function (e.g., safeDateToISOString) in a shared utils module (date-utils or utils/dates) with the current logic, then remove the local copies in get-documents-by-space-slug.ts and the other files and import the shared function where used (update any references to call safeDateToISOString unchanged). Ensure the new module is exported from the package index if needed and run the build/tests to catch import path issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/mcp-tools/src/space/get-tokens.ts`:
- Around line 84-90: The handler allows bypassing schema validation so sanitize
and clamp inputs inside handleGetTokens: validate that the incoming limit is a
finite positive integer (e.g., coerce/parse to Number, fallback to DEFAULT_LIMIT
if non-finite or <=0), then clamp with Math.min(..., MAX_LIMIT) into
appliedLimit; also trim the search string and only pass it to findAllTokens if
trim(search) is non-empty (otherwise pass undefined). Update the appliedLimit
and search usage around the existing appliedLimit/findAllTokens call to use
these sanitized values.
---
Nitpick comments:
In `@packages/mcp-tools/src/space/get-documents-by-space-slug.ts`:
- Around line 83-89: safeDateToISOString is duplicated across MCP tools; extract
it to a single shared utility and replace the duplicates with imports. Create a
single exported function (e.g., safeDateToISOString) in a shared utils module
(date-utils or utils/dates) with the current logic, then remove the local copies
in get-documents-by-space-slug.ts and the other files and import the shared
function where used (update any references to call safeDateToISOString
unchanged). Ensure the new module is exported from the package index if needed
and run the build/tests to catch import path issues.
In `@packages/mcp-tools/src/space/get-people-by-space-slug.ts`:
- Around line 67-73: The helper safeDateToISOString is duplicated across
get-people-by-space-slug.ts, get-spaces.ts and get-documents-by-space-slug.ts;
extract it into a shared module (e.g., create
packages/mcp-tools/src/utils/date.ts) that exports function safeDateToISOString,
update each file to import { safeDateToISOString } from that module, and remove
the inline implementations so all three modules use the single shared utility.
In `@packages/mcp-tools/src/space/get-spaces.ts`:
- Around line 68-74: The function safeDateToISOString is duplicated here; remove
this local implementation and import the canonical shared utility instead
(replace the local safeDateToISOString definition in get-spaces.ts with an
import from the shared util module), update any call sites in getSpaces (and
related functions in this file) to use the imported safeDateToISOString, and
ensure the shared utility export covers the same signature so behavior remains
identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7e00d0a-96b6-401b-a72f-93400c8bfee0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/web/src/app/mcp/route.tspackages/core/src/assets/server/queries.tspackages/core/src/space/server/queries.tspackages/mcp-tools/package.jsonpackages/mcp-tools/src/index.tspackages/mcp-tools/src/space/get-documents-by-space-slug.tspackages/mcp-tools/src/space/get-people-by-space-slug.tspackages/mcp-tools/src/space/get-spaces.tspackages/mcp-tools/src/space/get-tokens.ts
✅ Files skipped from review due to trivial changes (2)
- packages/mcp-tools/package.json
- apps/web/src/app/mcp/route.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/mcp-tools/src/index.ts
Comment on lines
+84
to
+90
| const appliedLimit = Math.min(limit ?? DEFAULT_LIMIT, MAX_LIMIT) as number; | ||
|
|
||
| const rows = await findAllTokens( | ||
| { db }, | ||
| { | ||
| search: search?.length ? search : undefined, | ||
| limit: appliedLimit, |
Contributor
There was a problem hiding this comment.
Clamp and sanitize inputs inside the handler (not only at schema boundary).
At Line 84 and Line 89, direct calls to exported handleGetTokens can bypass schema trimming/limits. Non-positive or non-finite limit can remove row capping downstream, and whitespace-only search can still be forwarded.
🔧 Proposed fix
- const appliedLimit = Math.min(limit ?? DEFAULT_LIMIT, MAX_LIMIT) as number;
+ const normalizedLimit =
+ typeof limit === 'number' && Number.isFinite(limit)
+ ? Math.trunc(limit)
+ : DEFAULT_LIMIT;
+ const appliedLimit = Math.max(1, Math.min(normalizedLimit, MAX_LIMIT));
+ const normalizedSearch = search?.trim();
const rows = await findAllTokens(
{ db },
{
- search: search?.length ? search : undefined,
+ search: normalizedSearch ? normalizedSearch : undefined,
limit: appliedLimit,
},
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/mcp-tools/src/space/get-tokens.ts` around lines 84 - 90, The handler
allows bypassing schema validation so sanitize and clamp inputs inside
handleGetTokens: validate that the incoming limit is a finite positive integer
(e.g., coerce/parse to Number, fallback to DEFAULT_LIMIT if non-finite or <=0),
then clamp with Math.min(..., MAX_LIMIT) into appliedLimit; also trim the search
string and only pass it to findAllTokens if trim(search) is non-empty (otherwise
pass undefined). Update the appliedLimit and search usage around the existing
appliedLimit/findAllTokens call to use these sanitized values.
Add a reusable MCP engineering competency reference and a new senior-mcp-engineer role with documentation-first protocol, then register it in the AGENTS router for role-based activation. Made-with: Cursor
Improve prompt reliability by adding conditional docs checks, strict scope control, and explicit breaking-change and verification protocols. Made-with: Cursor
Extract the MCP response shaping into local helper functions to keep route registration focused and prepare for package-level extraction in later migration phases without changing behavior. Made-with: Cursor
Create a minimal mcp-tools package that encapsulates the get_space_by_slug schemas, handler, and registration helper to prepare for route wiring migration in the next phase. Made-with: Cursor
Switch the web MCP route to register get_space_by_slug from @hypha-platform/mcp-tools and remove the inline tool implementation. Update web workspace dependencies and lockfile to include mcp-tools. Made-with: Cursor
Add optional token-based authorization and structured request timing logs to the MCP route, and harden get_space_by_slug date serialization with safe ISO normalization for invalid date values. Made-with: Cursor
Expose `get_space_proposals_by_space_slug` through the shared mcp-tools package and MCP route so clients can fetch proposal status buckets and indexed proposal metadata using a stable space slug contract. Made-with: Cursor
Wire list-spaces read path through findAllSpaces with an optional DB limit, register the tool on the MCP route, and depend on storage-postgres for db. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Expose paginated space members via findPeopleBySpaceSlug with a clear spaceFound flag when the slug does not resolve. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Expose paginated documents with optional state filter and search, ordered by createdAt descending, plus a spaceFound flag for unknown slugs. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Support an optional limit on findAllTokens for bounded reads and expose token rows with normalized numeric and date fields on the MCP route. Co-authored-by: webguru-hypha <webguru-hypha@users.noreply.github.com>
Rename the MCP token tool to get_tokens_by_space_slug and require a slug input so token discovery is explicitly space-scoped and predictable. BREAKING CHANGE: replace get_tokens with get_tokens_by_space_slug and require slug input. Made-with: Cursor
Rename handler and schema identifiers to include by-space-slug naming for consistency with the get_tokens_by_space_slug MCP contract. Made-with: Cursor
plitzenberger
force-pushed
the
litzi/feat/mcp-server
branch
from
3b9e9af to
57bfdfc
Compare
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
2 participants
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 by CodeRabbit
New Features
Documentation
Chores