|
| 1 | +# MCP-CLI Architecture Principles |
| 2 | + |
| 3 | +These principles govern all new code in mcp-cli. Existing code should be migrated toward these standards as it is touched. |
| 4 | + |
| 5 | +## 1. Pydantic Native |
| 6 | + |
| 7 | +Data structures are `BaseModel` subclasses, not raw dicts. Use `Field()` for defaults and documentation. Use `model_dump()` only at serialization boundaries (API calls, storage). |
| 8 | + |
| 9 | +```python |
| 10 | +# Yes |
| 11 | +class ToolResult(BaseModel): |
| 12 | + name: str |
| 13 | + content: str |
| 14 | + success: bool = True |
| 15 | + |
| 16 | +# No |
| 17 | +result = {"name": "foo", "content": "bar", "success": True} |
| 18 | +``` |
| 19 | + |
| 20 | +## 2. Async Native |
| 21 | + |
| 22 | +Public APIs are `async def`. Synchronous code is only for pure computation with no I/O. Never block the event loop with synchronous I/O inside async functions. |
| 23 | + |
| 24 | +```python |
| 25 | +# Yes |
| 26 | +async def execute_tool(self, name: str, args: dict) -> ToolResult: ... |
| 27 | + |
| 28 | +# No |
| 29 | +def execute_tool(self, name: str, args: dict) -> ToolResult: |
| 30 | + return asyncio.run(self._execute(name, args)) |
| 31 | +``` |
| 32 | + |
| 33 | +## 3. No Dictionary Goop |
| 34 | + |
| 35 | +Typed models at every boundary. When receiving external data (API responses, config files), parse into models immediately. Pass models between functions, not dicts. |
| 36 | + |
| 37 | +```python |
| 38 | +# Yes |
| 39 | +msg = Message.from_dict(raw_data) |
| 40 | +process(msg) |
| 41 | + |
| 42 | +# No |
| 43 | +process(raw_data) # passing a dict through the stack |
| 44 | +``` |
| 45 | + |
| 46 | +## 4. No Magic Strings |
| 47 | + |
| 48 | +Use `Enum`, `StrEnum`, or named constants from `config/defaults.py`. Never hardcode string literals that represent categories, field names, or configuration values. |
| 49 | + |
| 50 | +```python |
| 51 | +# Yes |
| 52 | +class MessageRole(str, Enum): |
| 53 | + USER = "user" |
| 54 | + ASSISTANT = "assistant" |
| 55 | + TOOL = "tool" |
| 56 | + |
| 57 | +# No |
| 58 | +if msg["role"] == "assistant": ... |
| 59 | +``` |
| 60 | + |
| 61 | +## 5. Core/UI Separation |
| 62 | + |
| 63 | +Logic that is UI-independent must not import from `display/`, `interactive/`, or `commands/`. Core modules handle: tool execution, conversation management, session state, configuration. |
| 64 | + |
| 65 | +UI modules handle: rendering, user input, theming, display formatting. |
| 66 | + |
| 67 | +**Future goal:** core modules extractable into a standalone `mcp-cli-core` package. |
| 68 | + |
| 69 | +``` |
| 70 | +src/mcp_cli/ |
| 71 | + chat/ # Core: conversation, tool processing, context |
| 72 | + config/ # Core: defaults, configuration loading |
| 73 | + tools/ # Core: tool management, execution |
| 74 | + model_management/ # Core: provider/model resolution |
| 75 | + display/ # UI: rendering, formatting, streaming display |
| 76 | + interactive/ # UI: terminal interaction |
| 77 | + commands/ # UI: CLI command handlers |
| 78 | +``` |
| 79 | + |
| 80 | +## 6. Single Source of Truth |
| 81 | + |
| 82 | +All default values live in `config/defaults.py`. Business logic imports constants from there, never hardcodes values. Configuration flows: `defaults.py` -> CLI flags -> runtime config -> component init. |
| 83 | + |
| 84 | +```python |
| 85 | +# Yes |
| 86 | +from mcp_cli.config.defaults import DEFAULT_MAX_TOOL_RESULT_CHARS |
| 87 | +content = truncate(content, DEFAULT_MAX_TOOL_RESULT_CHARS) |
| 88 | + |
| 89 | +# No |
| 90 | +content = truncate(content, 100_000) # magic number |
| 91 | +``` |
| 92 | + |
| 93 | +## 7. Explicit Dependencies |
| 94 | + |
| 95 | +Constructor injection over global singletons. When a component needs a dependency, accept it as a parameter. Global state is a last resort, not a first choice. |
| 96 | + |
| 97 | +## 8. Fail Loudly at Boundaries, Recover Gracefully Inside |
| 98 | + |
| 99 | +Validate inputs at system boundaries (CLI args, API responses, config files). Inside the core, trust the type system. Log errors with context, don't silently swallow exceptions with bare `except Exception`. |
| 100 | + |
| 101 | +## 9. Linting and Type Checking |
| 102 | + |
| 103 | +All code must pass `make check` (linting + type checking). No exceptions. Fix issues before merging, not after. |
| 104 | + |
| 105 | +## 10. Test Coverage |
| 106 | + |
| 107 | +Minimum **90% coverage per file**. New code ships with tests. Use `uv run pytest --cov=src/mcp_cli --cov-fail-under=90` to verify. Tests live alongside the code they test: `tests/chat/`, `tests/display/`, etc. |
| 108 | + |
| 109 | +## 11. Working Examples |
| 110 | + |
| 111 | +Every user-facing feature must have a working example in the `examples/` directory that demonstrates the functionality end-to-end. Examples serve as both documentation and integration tests. |
| 112 | + |
| 113 | +--- |
| 114 | + |
| 115 | +## Known Violations (Tier 4 Backlog) |
| 116 | + |
| 117 | +Architecture review performed after Tier 2. These are tracked for remediation in Tier 4 (Code Quality). |
| 118 | + |
| 119 | +### Core/UI Separation (#5) |
| 120 | + |
| 121 | +| File | Issue | Severity | |
| 122 | +|------|-------|----------| |
| 123 | +| `chat/ui_manager.py` | Imports `prompt_toolkit`, `display/`, `commands/` | HIGH — move to `interactive/` | |
| 124 | +| `chat/command_completer.py` | Imports `prompt_toolkit`, `commands/` | HIGH — move to `interactive/` | |
| 125 | +| `chat/streaming_handler.py:20` | Imports `StreamingDisplayManager` from `display/` | MEDIUM — use protocol | |
| 126 | +| `chat/__main__.py:15` | Imports `register_commands` from `commands/` | MEDIUM — entry point, acceptable | |
| 127 | + |
| 128 | +### Pydantic Native (#1, #3) |
| 129 | + |
| 130 | +| File | Issue | Severity | |
| 131 | +|------|-------|----------| |
| 132 | +| `chat/chat_context.py` | `openai_tools: list[dict]` instead of typed model | MEDIUM | |
| 133 | +| `chat/models.py` | `Message.tool_calls: list[dict]` instead of `list[ToolCallData]` | MEDIUM | |
| 134 | +| `chat/conversation.py` | `_validate_tool_messages()` works on raw dicts | MEDIUM — by design at serialization boundary | |
| 135 | + |
| 136 | +### Explicit Dependencies (#7) |
| 137 | + |
| 138 | +| File | Issue | Severity | |
| 139 | +|------|-------|----------| |
| 140 | +| `chat/tool_processor.py` | Uses `get_tool_state()`, `get_search_engine()` globals | MEDIUM | |
| 141 | +| `chat/conversation.py` | Uses `get_tool_state()` global | MEDIUM | |
| 142 | +| `chat/tool_processor.py` | Uses `get_preference_manager()` global | LOW | |
| 143 | + |
| 144 | +These are deferred to **Tier 4.1 (Replace Global Singletons)** and **Tier 4.2 (Consolidate Message Classes)**. |
0 commit comments