Modernization safe wins: stdio-safe bootstrap + dev tooling

[wshobson]

Summary:

• Make server STDIO-safe by eliminating stdout prints and ensuring SSE patching is explicit/conditional.
• Centralize dotenv loading in server bootstrap; remove import-time side effects (dotenv + logging.basicConfig) from library modules.
• Update dev defaults to streamable-http on localhost; keep STDIO target for Claude Desktop.
• Fix dev tooling so 'make check' reliably runs ruff and pyright via uv dev extra.

Checks:

• make lint
• make typecheck (warnings only: missing type stubs for pandas/msgpack)
• make check

Notes:

• Pyright warnings about missing stubs remain; can be addressed later with types packages or config changes.

[claude]

PR Review: Modernization Safe Wins - STDIO-safe Bootstrap + Dev Tooling

Summary

This PR makes excellent improvements to the codebase by addressing critical infrastructure concerns around STDIO safety, eliminating import-time side effects, and improving dev tooling. The changes are well-scoped, follow TDD principles, and align with the project's modernization goals.

---

✅ Strengths

1. Excellent Problem Identification & Solution

• STDIO Safety: Converting print() statements to logging calls is critical for STDIO transport. STDIO uses stdout for JSON-RPC protocol, so any prints would corrupt the stream.
• Side Effects Removal: Removing load_dotenv() and logging.basicConfig() from library modules follows best practices and prevents surprising behavior.
• SSE Patch Laziness: Making the SSE trailing-slash patch conditional/explicit is smart - it avoids import-time monkey-patching that could slow tests and cause unexpected behavior.

2. Strong Test Coverage

The new regression tests are comprehensive and well-designed:

• test_module_import_side_effects.py: Uses subprocess isolation and mocking to verify no side effects occur on import
• test_server_bootstrap_regressions.py: Tests three critical behaviors:
  • No stdout pollution in STDIO mode
  • SSE patch not applied at import time
  • SSE patch can be explicitly applied when needed

3. Documentation Updates

Excellent documentation in the conductor/ directory tracking the modernization effort, including:

• Clear task breakdown and phase planning
• Rationale for golden path decisions
• Follow-on implementation track planning

4. Code Quality Improvements

• Makefile: Now uses uv run --extra dev for lint/format/typecheck to ensure dev dependencies are available
• pyproject.toml: Added pyright>=1.1.400 to dev dependencies for better type checking
• scripts/dev.sh: Improved with better defaults, clearer messaging, and localhost binding by default

---

🔍 Issues & Concerns

1. Breaking Change: Default Transport Changed ⚠️

Location: Makefile:47-59, scripts/dev.sh:1216

The default transport was changed from SSE to streamable-http, but the CLAUDE.md documentation still states:

✅ RECOMMENDED: Claude Desktop works best with the SSE endpoint via mcp-remote bridge. This configuration has been tested and prevents tools from disappearing after initial connection.

Impact: This creates a documentation/implementation mismatch. Users following CLAUDE.md will expect make dev to start with SSE, but it now starts with streamable-http.

Recommendation:

• Either update CLAUDE.md to reflect the new golden path (streamable-http for local dev, STDIO for Claude Desktop)
• OR revert the default back to SSE and make streamable-http opt-in
• The PR description mentions "Update dev defaults to streamable-http on localhost" which suggests the new default is intentional, so updating CLAUDE.md is likely the right path

2. Potential Issue: load_dotenv() Now in Server Module 🤔

Location: maverick_mcp/api/server.py:108-109

The PR adds load_dotenv() directly in server.py at module level:

from dotenv import load_dotenv
# ...
load_dotenv()

Concern: This is still an import-time side effect, just moved to a different module. While it's better than having it scattered across library modules, it would be cleaner to call it in the actual bootstrap function (e.g., in main() or before calling .run()).

Why it matters:

• Importing server.py for testing purposes will load .env even if not intended
• Makes it harder to control environment loading in tests
• The regression test doesn't catch this because it only checks library modules

Recommendation: Move load_dotenv() to the if __name__ == "__main__": block or into a dedicated bootstrap function

3. SSE Patch Function Design 💡

Location: maverick_mcp/api/server.py:632-683

The new apply_sse_trailing_slash_patch() function is well-designed with idempotency checking, but has a minor issue:

Code:

if (
    getattr(fastmcp_http.create_sse_app, "__name__", "")
    == "_patched_create_sse_app"
):
    return

Issue: This check relies on function name string matching, which is brittle. If the function is wrapped by another decorator or the name changes, the check fails.

Better approach:

if hasattr(fastmcp_http.create_sse_app, '_maverick_patched'):
    return
# ...
fastmcp_http.create_sse_app = _patched_create_sse_app
fastmcp_http.create_sse_app._maverick_patched = True

4. Missing Type Annotations 📝

Location: maverick_mcp/api/server.py:632

The new apply_sse_trailing_slash_patch() function lacks a complete docstring and type annotations for the inner function parameters.

Current:

def apply_sse_trailing_slash_patch() -> None:
    """..."""
    # ...
    def _patched_create_sse_app(
        server: Any,
        message_path: str,
        sse_path: str,
        auth: Any | None = None,
        debug: bool = False,
        routes: list[BaseRoute] | None = None,
        middleware: list[Middleware] | None = None,
    ) -> Any:

The return type Any and parameter type Any for server could be more specific. Looking at the FastMCPProtocol, it should likely be FastAPI.

5. Default Host Binding Security 🔒

Location: scripts/dev.sh:1207, old default was 0.0.0.0

Good change: The new default is 127.0.0.1 (localhost), which is more secure.

However, the PR description and CLAUDE.md mention this aligns with MCP security guidance:

Security guidance matters for local, unauthenticated servers:

• Servers SHOULD bind to localhost (127.0.0.1) rather than 0.0.0.0.

Verification needed: Confirm that all documentation examples and Claude Desktop configs use localhost binding, not 0.0.0.0.

---

🎯 Minor Issues

6. Inconsistent Logging Levels

Location: maverick_mcp/api/server.py:778-800

Some status messages use logger.info() with emoji prefixes (✅, 🎯, etc.) while others don't. Consider either:

• Removing emojis from log messages (emojis in logs can cause encoding issues in some environments)
• OR being consistent with emoji usage

Example:

logger.info("✅ Backtesting metrics system initialized successfully")  # Has emoji
logger.info("Enhanced Backtesting Metrics System Enabled")  # No emoji

7. Duplicate Dependency Group

Location: pyproject.toml:1164-1169

The old [dependency-groups] section was removed, which is good. Verify that testcontainers[postgres] is still available through the [project.optional-dependencies] section if needed.

---

🧪 Testing Recommendations

1. Test the New Regression Tests

Run the new tests to ensure they actually pass:

uv run pytest maverick_mcp/tests/test_module_import_side_effects.py -v
uv run pytest maverick_mcp/tests/test_server_bootstrap_regressions.py -v

2. Verify Default Transport Change

Test that the new default (streamable-http) works correctly:

make dev
curl -X POST http://127.0.0.1:8003/mcp

3. Test STDIO Mode

Verify STDIO mode still works without stdout pollution:

make dev-stdio 2>stderr.log 1>stdout.log &
# stdout.log should only contain JSON-RPC messages, no prints

---

📋 Action Items

Must Fix:

1. ✅ Update CLAUDE.md to reflect the new default transport (streamable-http for dev, STDIO for Claude Desktop)
2. ✅ Move load_dotenv() from module-level to bootstrap function in server.py

Should Fix:

3. 🔧 Improve SSE patch idempotency check using a custom attribute instead of name matching
4. 🔧 Add more specific type annotations to apply_sse_trailing_slash_patch()

Nice to Have:

5. 📝 Verify all documentation uses 127.0.0.1 instead of 0.0.0.0 for security
6. 🎨 Consider removing emojis from log messages or standardizing their usage

---

📊 Overall Assessment

Score: 8.5/10

This is a high-quality PR that tackles important technical debt and improves the project's architecture. The changes are well-tested, follow TDD principles, and demonstrate deep understanding of the codebase.

Strengths:

• ✅ Excellent regression test coverage
• ✅ Proper separation of concerns
• ✅ Clear documentation and planning
• ✅ Security improvements (localhost binding)

Weaknesses:

• ⚠️ Documentation/implementation mismatch (default transport)
• ⚠️ load_dotenv() still at import time (just moved)
• 🔧 Minor type annotation and idempotency check improvements needed

Recommendation: Approve with minor revisions. The two "Must Fix" items should be addressed before merging, but they're straightforward fixes that don't require major rework.

---

Great work on this modernization effort! The structured approach via the conductor/ tracking is excellent and should serve as a model for future refactoring work. 🎉

[claude]

Code Review: Modernization Safe Wins - STDIO-Safe Bootstrap + Dev Tooling

Overall Assessment

✅ Excellent refactoring that addresses critical STDIO transport issues and improves code quality significantly. This PR makes the codebase more maintainable and production-ready.

---

Strengths

1. STDIO Transport Safety 🎯

• Critical Fix: Eliminated all print() statements that would corrupt STDIO's JSON-RPC protocol
• Smart Design: Converted prints to proper logger.info() calls for stderr output
• Regression Tests: Added test_server_import_with_stdio_emits_no_stdout() to prevent future issues
• Impact: Claude Desktop and other STDIO clients will now work reliably

2. Import-Time Side Effect Removal 🔧

• Clean Architecture: Removed load_dotenv() and logging.basicConfig() from library modules
• Centralized Bootstrap: Moved env loading to server.py entry point only
• Modules Fixed:
  • config/settings.py
  • data/cache.py
  • providers/stock_data.py
  • providers/market_data.py
  • providers/macro_data.py
  • tools/portfolio_manager.py
  • core/technical_analysis.py
  • core/visualization.py
  • backtesting/visualization.py
• Regression Test: test_module_import_side_effects.py prevents future regressions

3. SSE Patch Improvements ✨

• Lazy Application: Patch now applied explicitly when needed, not at import-time
• Idempotent: apply_sse_trailing_slash_patch() can be called multiple times safely
• Better Logging: Uses proper logger instead of stderr prints
• Testable: Added tests to verify patch behavior

4. Development Experience 🚀

• Updated Defaults: Streamable-HTTP is now default (better for local dev)
• Fixed Tooling: make check now uses --extra dev for reliable ruff/pyright execution
• Better Scripts: dev.sh sources .env early for configuration
• Type Checking: Added pyright to dev dependencies

5. Code Quality 📊

• Modern Enums: Converted Enum to StrEnum where appropriate
• Resource Registration: Fixed decorator pattern to avoid replacing callables
• Comprehensive Tests: Added 2 new test files with 5+ regression tests

---

Issues & Suggestions

Critical Issues ❌

None identified - The PR is production-ready.

Minor Issues ⚠️

1. Type Checker Mismatch (Documentation vs Implementation)

# pyproject.toml now includes pyright
+ "pyright>=1.1.400",

# But Makefile still uses 'ty' (Astral's type checker)
typecheck:
-	@uv run ty check .
+	@uv run --extra dev pyright .

Recommendation: Either:

• Use pyright everywhere and update docs
• Keep ty and update pyproject.toml
• Support both and document the choice

2. Resource Decorator Pattern

# Old (replaced callable)
@mcp.resource("health://")
def health_resource() -> dict[str, Any]:
    pass

# New (preserves callable)
def health_resource() -> dict[str, Any]:
    pass
mcp.resource("health://")(health_resource)

Observation: This is good for avoiding unintended side effects, but inconsistent:

• health_resource and status_dashboard_resource use new pattern
• performance_dashboard_resource still uses decorator pattern

Recommendation: Apply consistently to all resources for clarity.

3. Dependency Group Removal

-[dependency-groups]
-dev = [
-    "testcontainers[postgres]>=4.10.0",
-]

Question: Was testcontainers moved to main dev dependencies or intentionally removed? If integration tests need it, ensure it's still available.

4. Protocol Definition Addition

class FastMCPProtocol(Protocol):
    # ... existing methods ...
+    def add_middleware(self, middleware: Middleware) -> None: ...

Observation: Added but not used in the visible diff. If this was added for future use, consider adding a comment explaining why.

---

Performance Considerations

Positive Impact ✅

1. Faster Test Imports: No more side effects means faster test suite startup
2. Lazy SSE Patch: Only applied when SSE transport is used
3. Cleaner Module Loading: More predictable import behavior

No Concerns

• No performance regressions identified
• Logging changes are negligible overhead

---

Security Considerations

No Issues Found ✅

• Dotenv loading still happens (just centralized)
• No new security vulnerabilities introduced
• Logging changes don't expose sensitive data

---

Test Coverage

Excellent Addition ✅

• test_module_import_side_effects.py: 38 lines of protection against regression
• test_server_bootstrap_regressions.py: 64 lines testing STDIO safety and SSE patch behavior
• Uses subprocess isolation for accurate testing
• Mock-based validation ensures no actual side effects

Recommendation: After merge, run make test-cov to verify overall coverage stays high.

---

Documentation Updates Needed

CLAUDE.md Updates Required 📝

1. Default Transport Changed:

# Old
make dev              # SSE transport (default, recommended)

# New (should be)
make dev              # Streamable-HTTP transport (default, localhost dev)
make dev-stdio        # STDIO transport (recommended for Claude Desktop)

2. Type Checker Clarification:
   Document whether the project uses pyright or ty consistently.

3. Dev Script Environment Variables:
   Document that MAVERICK_TRANSPORT, MAVERICK_HOST, and MAVERICK_PORT can be set in .env.

---

Recommendations

Before Merge 🔴

1. Resolve type checker inconsistency (pyright vs ty)
2. Update CLAUDE.md to reflect new defaults
3. Verify testcontainers dependency is intentionally removed or moved

After Merge 🟢

1. Run full test suite including integration tests
2. Test all transport modes (STDIO, HTTP, SSE) manually
3. Update any client configuration docs if defaults changed
4. Consider: Apply consistent resource registration pattern to performance_dashboard_resource

---

Final Verdict

✅ APPROVE with minor documentation updates

This is a high-quality refactoring that:

• Fixes critical STDIO transport bugs
• Improves code maintainability
• Adds strong regression protection
• Follows best practices for side effect management

The minor issues are documentation/consistency improvements that don't block the merge.

Great work on this modernization effort! 🎉

---

Checklist for Merge

• Resolve type checker tool choice (pyright vs ty)
• Update CLAUDE.md with new defaults
• Verify testcontainers dependency status
• Run make check and make test successfully
• Manual test: STDIO transport with Claude Desktop
• Manual test: Streamable-HTTP with curl
• Manual test: SSE transport with mcp-remote

[wshobson]

Post-review fixes: full dependency upgrade + test suite repair

What changed

Dependency upgrade — All libraries upgraded via uv lock --upgrade:

• fastmcp 2.14.5 → 3.1.0
• fastapi 0.128.2 → 0.135.1
• pandas 3.0.0 → 3.0.1
• langchain family, 80+ other packages updated

Test suite fixed (was 636 errors + 34 failures → 681 passed, 0 failed, 0 errors):

• tests/conftest.py: Docker/testcontainers fixtures now gracefully skip when Docker isn't running (resolved 636 errors)
• maverick_mcp/tests/test_mcp_tools.py, test_in_memory_server.py, test_in_memory_routers.py: Updated for FastMCP 3.x API (Client init, mount() signature, resource return types, SQLite StaticPool for worker threads)
• test_stock_data_enhanced.py, test_stock_data_provider.py: Updated mocks for YFinancePool-based provider architecture
• test_macro_data_provider.py: Fixed pandas frequency deprecations (freq="Q"→"QE", freq="M"→"ME")
• test_mcp_tool_fixes_pytest.py: Fixed LLM factory env var guard (OpenRouter, not OpenAI)
• test_models_functional.py: Fixed SQLite timezone/decimal assertions
• Deleted orphaned test_mailgun_email.py (imported non-existent module)

Production bug fix:

• maverick_mcp/api/routers/portfolio.py: DataFrame.applymap() → DataFrame.map() (removed in pandas 3.x)

Documentation:

• CLAUDE.md updated to align with new transport defaults

Verification

• uv run pytest: 681 passed, 4 skipped, 0 failed, 0 errors
• uv run ruff check .: All checks passed
• uv run ruff format --check .: All files formatted

[wshobson]

Update: Rebased onto main and resolved all conflicts. Branch is now clean and mergeable.

• Resolved conflicts in 11 files (server.py, rate_limiting, 7 test files, conftest, uv.lock)
• Fixed 2 additional post-rebase failures (event loop integrity tests needed json.loads() for resource return type change)
• Regenerated uv.lock fresh
• Final result: 681 passed, 0 failed, 0 errors, all lint/format clean

[claude]

test comment

[claude]

Code Review: 1 bug, 2 warnings. See full review below.

[claude]

Bugs - test_in_memory_server.py:52-65: _serialize_health double-encodes. health_resource() returns a json.dumps string already. _serialize_health passes it through _convert which returns strings as-is, then re-wraps with json.dumps again. json.loads(result[0].text) yields a str not dict, so health_data[status] in test_server_health raises TypeError. Fix: return health_resource() directly from health_resource_str().

[claude]

Warnings - (1) test files: _disable_output_schemas accesses server.providers[0] and lp._components, private FastMCP internals. IndexError if providers is empty or FastMCP changes structure. Guard with: if not server.providers: return. (2) rate_limiting_enhanced.py:229-242: _await_if_needed wraps pipeline method calls that return the pipeline object, not coroutines. pipeline.execute() still runs all commands correctly so no runtime error, but pattern is misleading. Remove _await_if_needed from pipeline calls.

[claude]

Notes - (1) tool_estimation.py:134: switching to model_fields_set check correctly fixes edge case where tool_estimates={} was passed explicitly but overwritten. Good fix. (2) data/performance.py:96,150: execute_command(PING) over ping() sidesteps a strict redis-py type-checker false positive; functionally equivalent. (3) scripts/dev.sh: source .env is now before TRANSPORT defaults so .env-supplied MAVERICK_TRANSPORT feeds into the streamable-http default. Correct ordering.