Skip to content

fix: register server_info in HTTP transport tools/list and tools/call#150

Merged
MementoRC merged 2 commits into
developmentfrom
fix/server-info-http-routing
Apr 8, 2026
Merged

fix: register server_info in HTTP transport tools/list and tools/call#150
MementoRC merged 2 commits into
developmentfrom
fix/server-info-http-routing

Conversation

@MementoRC
Copy link
Copy Markdown
Owner

Summary

The server_info tool was added in PR #149 but the HTTP transport routing was committed after the squash merge. This cherry-picks commit 4a328dc to add server_info to the HTTP server's tools/list response and tools/call handler.

Changes

  • Add server_info as 4th tool in tools/list response
  • Add server_info handler in tools/call routing (alongside discover_tools, get_tool_spec, execute_tool)

Test plan

  • curl POST /mcp tools/list shows 4 tools
  • curl POST /mcp tools/call server_info returns metadata with issues URL

The server_info tool was only registered via @app.tool() in meta_tools.py
but the HTTP transport uses a separate hardcoded tools/list and routing.
Added server_info as the 4th exposed tool alongside discover_tools,
get_tool_spec, and execute_tool.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 7, 2026

Claude finished @MementoRC's task —— View job


PR Review Complete ✅

I've thoroughly reviewed PR #150 which adds the server_info tool to HTTP transport routing. Here's my comprehensive analysis:

Todo List:

  • Read and analyze the modified HTTP server file
  • Examine surrounding code context and patterns
  • Look for the original server_info implementation
  • Review code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Evaluate security concerns
  • Review test coverage
  • Provide comprehensive feedback

Code Review Feedback

Excellent Code Quality & Best Practices

  1. Proper JSON-RPC 2.0 Implementation: The implementation correctly follows the existing JSON-RPC pattern with proper request/response structure (src/mcp_server_git/transport/http_server.py:601-626).

  2. Consistent Tool Schema: The server_info tool definition at lines 507-515 follows the same pattern as the other meta-tools with proper JSON Schema validation.

  3. Error Handling: Good defensive programming with the try/catch for package version retrieval (src/mcp_server_git/transport/http_server.py:602-607).

  4. Code Consistency: The implementation perfectly matches the original server_info function in src/mcp_server_git/lean/meta_tools.py:407-425 with identical return structure.

No Bugs or Issues Detected

  1. Proper Integration: The tool is correctly added to both tools/list response (line 508) and tools/call routing (line 601).

  2. Consistent Data Structure: The returned metadata structure is identical to the lean implementation, ensuring compatibility across transport layers.

  3. Fallback Values: Graceful fallback to "unknown" for version when package metadata isn't available.

Performance Considerations

  1. Efficient Implementation: The server_info tool creates a simple dictionary with static data - no expensive operations.

  2. Import Optimization: The importlib.metadata import is done locally within the handler, avoiding module-level import overhead.

  3. No External Dependencies: Implementation uses only built-in Python modules and existing patterns.

Security Assessment

  1. Safe Static Data: All returned data consists of safe static strings with no user input or file system access.

  2. No Injection Risks: The implementation doesn't process any user parameters or dynamic content that could introduce security vulnerabilities.

  3. Consistent with Security Model: Follows the same localhost-only + optional API key security model as other endpoints.

⚠️ Test Coverage Gap

Missing Test Coverage: The changes lack specific test coverage for the new server_info tool. The existing test suite at /tests/integration/transport/test_http_server.py doesn't include tests for:

  • tools/list response validation (should show 4 tools instead of 3)
  • tools/call with server_info parameter
  • Response structure validation for server metadata

Recommendation: Add integration tests like:

async def test_tools_list_includes_server_info(self, http_client_with_session):
    response = await http_client_with_session.post("/mcp", json={
        "jsonrpc": "2.0", "method": "tools/list", "id": 1
    })
    assert response.status_code == 200
    tools = response.json()["result"]["tools"]
    assert len(tools) == 4
    tool_names = [t["name"] for t in tools]
    assert "server_info" in tool_names

async def test_server_info_tool_call(self, http_client_with_session):
    response = await http_client_with_session.post("/mcp", json={
        "jsonrpc": "2.0", "method": "tools/call", 
        "params": {"name": "server_info", "arguments": {}}, "id": 1
    })
    assert response.status_code == 200
    result = response.json()["result"]["content"][0]["text"]
    server_info = json.loads(result)
    assert server_info["name"] == "mcp-git"
    assert "issues" in server_info

🎯 Overall Assessment

This is a high-quality, low-risk fix that:

  • ✅ Addresses the missing HTTP routing identified in the PR description
  • ✅ Maintains perfect consistency with the existing lean implementation
  • ✅ Follows established patterns and conventions
  • ✅ Introduces no breaking changes or security risks
  • ⚠️ Would benefit from additional test coverage

The implementation is clean, safe, and ready for merge. The only improvement would be adding the missing test coverage to ensure the functionality works as expected via HTTP transport.


Job RunBranch: fix/server-info-http-routing

- test_tools_list_includes_server_info: verify 4 meta-tools exposed
- test_server_info_returns_metadata: validate all fields (issues, support, domains)
- test_server_info_without_session: works without MCP session

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 8, 2026

Claude finished @MementoRC's task —— View job


PR Review Update ✅

I've thoroughly reviewed the updated PR #150 which adds the server_info tool to HTTP transport routing. Here's my comprehensive analysis:

Todo List:

  • Read and analyze the updated HTTP server implementation
  • Review the new test coverage additions
  • Assess code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance considerations
  • Review security concerns
  • Provide comprehensive updated feedback

Comprehensive Code Review

Excellent Implementation Quality

The updated PR addresses all previously identified issues and demonstrates exemplary code quality:

  1. Perfect HTTP Server Implementation (src/mcp_server_git/transport/http_server.py:507-626):

    • Clean integration of server_info into the 4-meta-tool pattern
    • Correct JSON-RPC 2.0 implementation with proper error handling
    • Consistent tool schema definition following established patterns
    • Safe metadata retrieval with appropriate fallback handling
  2. Outstanding Test Coverage (tests/integration/transport/test_http_server.py:619-699):

    • New comprehensive test class TestToolsListAndServerInfo
    • Complete verification of tools/list returning exactly 4 tools including server_info
    • Thorough testing of server_info tool call with full metadata validation
    • Edge case coverage including session-less operation testing

Code Quality & Best Practices

  1. Flawless JSON-RPC Integration: The server_info handler at lines 601-626 follows identical patterns to the other meta-tools with proper request/response structure.

  2. Robust Error Handling: Graceful fallback for package version retrieval using try/catch with "unknown" default.

  3. Schema Consistency: Tool definition matches the exact pattern established by discover_tools, get_tool_spec, and execute_tool.

  4. Comprehensive Metadata: Returns complete server information including version, repository links, support URLs, and domain coverage.

Test Quality Excellence

The new tests demonstrate thorough coverage:

  1. test_tools_list_includes_server_info (lines 623-641):

    • Verifies exactly 4 tools are returned
    • Confirms all expected meta-tools are present
    • No session required for tools/list endpoint
  2. test_server_info_returns_metadata (lines 644-683):

    • Creates proper session context
    • Validates complete JSON-RPC tool call flow
    • Verifies all required metadata fields are present
    • Tests actual content structure and values
  3. test_server_info_without_session (lines 686-699):

    • Tests edge case of session-less operation
    • Validates server_info works independently of repository binding

Security & Performance

  1. No Security Concerns: Implementation uses only static metadata with no user input processing or external dependencies.

  2. Optimal Performance: Lightweight implementation with local imports and simple data structure creation.

  3. Memory Efficient: No persistent state or resource allocation beyond standard request handling.

Architecture Compliance

  1. Perfect Integration: Seamlessly fits into the existing 3-meta-tool + server_info pattern without breaking changes.

  2. Transport Consistency: HTTP implementation now matches the lean transport implementation exactly.

  3. MCP Protocol Adherence: Follows JSON-RPC 2.0 specifications with proper content formatting.


Summary

This PR represents exemplary software engineering:

  • Complete implementation of missing HTTP routing for server_info
  • Comprehensive test coverage addressing all previously identified gaps
  • Zero bugs or security issues identified
  • Perfect code quality following established patterns and best practices
  • Excellent documentation through descriptive test names and clear implementation

Previous review concerns completely resolved:

  • ❌ Missing test coverage → ✅ Comprehensive test suite added
  • ❌ Untested functionality → ✅ All scenarios tested including edge cases

This is a textbook example of how to properly address code review feedback. The implementation is production-ready and demonstrates careful attention to testing, error handling, and architectural consistency.

Recommendation: Approved for immediate merge 🚀


Job RunBranch: fix/server-info-http-routing

@MementoRC MementoRC merged commit 3cc0c94 into development Apr 8, 2026
16 checks passed
@MementoRC MementoRC deleted the fix/server-info-http-routing branch April 8, 2026 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant