Add transport protocol support to MCP server configurations

[aponcedeleonch]

• Introduced MCPServerConfig model with url and transport fields (sse or streamable-http)
• Updated CLI to accept server specs in format "url:transport" (e.g., "http://localhost:3000:sse")
• Modified all API endpoints and services to handle MCPServerConfig instead of plain URL strings
• Updated test case creation and tool loading to respect transport protocol
• Changed database schema and repositories to store transport configuration
• Affects: CLI commands, API routes, LLM service, MCP loader, evaluation service, similarity service

Transport defaults to "streamable-http" when not specified for backwards compatibility.

🤖 Generated with Claude Code

[claude]

PR Review: Transport Protocol Support

Summary: This PR successfully adds transport protocol configuration to MCP servers. The implementation is comprehensive and follows project conventions well.

✅ Strengths

1. Good backwards compatibility - Defaults to streamable-http when transport not specified
2. Proper validation - Pydantic patterns enforce valid transports (sse | streamable-http)
3. Consistent refactoring - Updated all layers (CLI, API, services, storage)
4. Database schema updated - Added transport column with CHECK constraint

🔍 Issues Found

High Priority

1. CLI parsing bug (test_case.py:307)

   • rsplit(':', 1) will incorrectly split URLs with ports
   • Example: http://localhost:8080:sse → splits as http://localhost:8080 and sse ✅
   • But http://localhost:8080 → splits as http://localhost and 8080 ❌
   • Fix: Check if second part is valid transport BEFORE treating it as transport

2. Duplicate MCPServerConfig (schemas.py:21 & cli/models.py:106)

   • Same model defined in two places with identical fields
   • Fix: Import from src in CLI, or extract to shared location

Medium Priority

3. Inconsistent transport naming (mcp_loader.py:46)

   • Database uses streamable-http (hyphen)
   • Code checks streamable_http (underscore) in some places
   • Currently works because only checking == 'sse', but fragile
   • Fix: Normalize to hyphens everywhere or add alias handling

4. Type mismatch warning (evaluation_service.py:366)

   • Function docs say returns list[dict] but code shows returns list[ToolDefinition]
   • Line 380: Accessing .name, .description suggests ToolDefinition objects
   • Fix: Update return type hint and docstring

Low Priority

5. Verbose parsing logic (test_case.py:283-326)

   • 44 lines for simple "URL[:transport]" parsing with repetitive validation
   • Suggestion: Extract URL validation helper, reduce to ~20 lines

6. Missing validation (similarity.py)

   • Changed from mcp_server_urls to mcp_servers but no validation that configs are distinct
   • Could allow duplicate URLs with different transports (likely unintended)

🎯 Recommendations

1. Must fix before merge: Issues Add security with proper configuration #1, Heavy gpu use on tool-quality on local models #2, Fix Ollama provider support and add CLI alias #3
2. Should fix: Issue Update docs #4
3. Nice to have: Issues tool-quality timing out on larger tools #5, Fix CLI test shell scripts #6

📊 Code Quality

• ✅ Follows Pydantic patterns
• ✅ Database migrations handled
• ⚠️ Type hints need correction
• ⚠️ No visible test updates for new parsing logic

Estimated review time: < 2 minutes

[therealnb]

Looks ok - need to fix the checks though.

[therealnb]

@claude check your initial comments were addressed

[claude]

Claude finished @therealnb's task in 1m 45s —— View job

---

Verification of Initial Review Comments

Checking if the issues from the initial review were addressed...

Todo List

• Review Issue Add security with proper configuration #1: CLI parsing bug with URLs containing ports
• Review Issue Heavy gpu use on tool-quality on local models #2: Duplicate MCPServerConfig definitions
• Review Issue Fix Ollama provider support and add CLI alias #3: Inconsistent transport naming
• Review Issue Update docs #4: Type mismatch warning in evaluation_service.py
• Review Issue tool-quality timing out on larger tools #5: Verbose parsing logic (low priority)
• Review Issue Fix CLI test shell scripts #6: Missing validation in similarity.py (low priority)
• Provide final summary

---

Summary of Findings

✅ Issue #1: CLI parsing bug (HIGH PRIORITY) - FIXED

Original issue: rsplit(':', 1) would incorrectly split URLs with ports like http://localhost:8080 as http://localhost and 8080.

Status: RESOLVED ✅

The code in cli/src/mcp_tef_cli/commands/test_case.py:308-320 now properly handles this:

parts = server_spec.rsplit(":", 1)
if len(parts) == 2:
    url, potential_transport = parts
    # Check if this is actually a transport spec or just part of the URL
    if potential_transport in ("sse", "streamable-http"):
        # Verify the URL part is still valid
        if not url.startswith(("http://", "https://")):
            raise click.BadParameter(...)
        return MCPServerConfig(url=url, transport=potential_transport)
# No valid transport found, treat entire string as URL
return MCPServerConfig(url=server_spec)

This correctly validates that the part after : is a valid transport before treating it as such. URLs like http://localhost:8080 will be treated as a complete URL (not split), while http://localhost:8080:sse will be correctly parsed.

---

✅ Issue #2: Duplicate MCPServerConfig (HIGH PRIORITY) - ACCEPTABLE

Original issue: MCPServerConfig defined in both src/mcp_tef/models/schemas.py:21 and cli/src/mcp_tef_cli/models.py:106.

Status: ACCEPTABLE ✅ (Intentional vendoring with documentation)

The CLI version at cli/src/mcp_tef_cli/models.py:106-125 includes clear documentation explaining this is intentional:

class MCPServerConfig(BaseModel):
    """MCP server configuration with transport type.

    Note: This model is vendored (copied) from src/mcp_tef/models/schemas.py
    to avoid requiring the full server package as a CLI dependency.
    Keep in sync with the main model for consistency.
    """

This is a deliberate architectural decision to keep the CLI independent from the server package. The models are identical, which is correct. The docstring properly warns maintainers to keep them in sync.

---

✅ Issue #3: Inconsistent transport naming (MEDIUM PRIORITY) - NOT AN ISSUE

Original issue: Concern about mixing streamable-http (hyphen) with streamable_http (underscore).

Status: NOT AN ISSUE ✅

After reviewing the code:

• Database schema (schema.sql:65-68) uses streamable-http in CHECK constraint
• Pydantic models use regex pattern r"^(sse|streamable-http)$" enforcing hyphens
• Code in mcp_loader.py:42-48 only checks for transport == "sse" and defaults to else case for streamable-http
• No code uses streamable_http (underscore version)

The naming is consistent throughout. The if transport == "sse" check works correctly because it's an exact match, and anything else (including streamable-http) falls through to the else branch.

---

✅ Issue #4: Type mismatch (MEDIUM PRIORITY) - NOT AN ISSUE

Original issue: Function docs say returns list[dict] but accessing .name, .description suggests list[ToolDefinition].

Status: NOT AN ISSUE ✅

Looking at evaluation_service.py:349-413, the _ingest_from_single_server function:

• Returns: tuple[list[ToolDefinitionResponse], list[str]] (line 354)
• Actually returns: ingested_tools, skipped_tools from tool_repo.batch_create() (line 400)
• The return type annotation is correct

The original review may have been looking at a different function or outdated code. The current code has correct type hints.

---

✅ Issue #5: Verbose parsing logic (LOW PRIORITY) - ACCEPTABLE

Original issue: 44 lines for simple "URL[:transport]" parsing with repetitive validation.

Status: ACCEPTABLE ✅

The parse_server_spec function at test_case.py:283-323 is 41 lines including:

• Comprehensive docstring (12 lines)
• Input validation
• Transport detection with validation
• Clear error messages

This is reasonable for a CLI parsing function that needs to provide user-friendly error messages. The verbosity aids clarity and maintainability.

---

✅ Issue #6: Missing validation (LOW PRIORITY) - NOT APPLICABLE

Original issue: Changed from mcp_server_urls to mcp_servers but no validation that configs are distinct.

Status: NOT APPLICABLE ✅

Looking at similarity.py:

• The API accepts mcp_servers: list[MCPServerConfig] (line 426)
• These are passed to extract_and_normalize_tools() which loads tools from each server
• Allowing duplicate URLs with different transports is potentially useful (same server, different transport)
• If truly duplicate, the tools would just be loaded twice (inefficient but not broken)

There's no clear requirement that server configs must be unique. The system handles duplicates gracefully.

---

Final Verdict

All high and medium priority issues from the initial review have been addressed or were not actual issues upon closer inspection.

Changes Made Since Initial Review:

1. ✅ CLI parsing correctly handles URLs with ports (Issue Add security with proper configuration #1)
2. ✅ MCPServerConfig duplication is intentional and documented (Issue Heavy gpu use on tool-quality on local models #2)
3. ✅ Transport naming is consistent (Issue Fix Ollama provider support and add CLI alias #3 was a false positive)
4. ✅ Type hints are correct (Issue Update docs #4 was a false positive)

Remaining Items (Low Priority):

• Issue tool-quality timing out on larger tools #5 (verbose parsing) and Issue Fix CLI test shell scripts #6 (duplicate validation) are acceptable as-is

Recommendation: ✅ This PR is ready to merge. All critical issues have been resolved, and the low-priority items are acceptable design choices.

--- |