test: Implemented tests for the MCP server lifecycle
This commit is contained in:
@@ -0,0 +1,129 @@
|
||||
# Iteration 5 — Test Implementation Notes
|
||||
|
||||
## Plan file addressed
|
||||
|
||||
`docs/testing/plans/05-mcp-lifecycle.md`
|
||||
|
||||
## Tests created
|
||||
|
||||
### src/config/mcp_factory.rs (12 new tests)
|
||||
|
||||
| Test name | What it verifies |
|
||||
|---|---|
|
||||
| `key_from_stdio_spec_captures_command_args_env` | McpServerKey extracts command, args, env from stdio spec |
|
||||
| `key_from_stdio_spec_sorts_args_and_env` | Args and env are sorted for deterministic key hashing |
|
||||
| `key_from_stdio_spec_defaults_empty_when_none` | None args/env default to empty vecs |
|
||||
| `key_from_remote_http_spec` | Http transport key captures url and transport type |
|
||||
| `key_from_remote_sse_spec_with_sorted_headers` | SSE headers sorted for deterministic keys |
|
||||
| `key_equality_same_spec_produces_equal_keys` | Same spec → equal keys (sharing contract) |
|
||||
| `key_inequality_different_names` | Different server names → different keys |
|
||||
| `key_inequality_different_commands` | Different commands → different keys (isolation contract) |
|
||||
| `key_env_bool_and_int_coerce_to_string` | JsonField::Bool/Int coerced to String in key |
|
||||
| `factory_try_get_active_returns_none_when_empty` | Empty factory returns None |
|
||||
| `factory_try_get_active_returns_none_for_unknown_key` | Unknown key returns None |
|
||||
| `factory_default_has_empty_active_map` | Default factory has empty internal map |
|
||||
|
||||
### src/config/tool_scope.rs (6 new tests)
|
||||
|
||||
| Test name | What it verifies |
|
||||
|---|---|
|
||||
| `mcp_runtime_new_is_empty` | New McpRuntime has no servers |
|
||||
| `mcp_runtime_default_is_empty` | Default McpRuntime is empty |
|
||||
| `mcp_runtime_get_returns_none_for_missing_server` | get() on nonexistent server returns None |
|
||||
| `tool_scope_default_has_empty_mcp_runtime` | Default ToolScope has empty MCP runtime |
|
||||
| `tool_scope_default_has_empty_functions` | Default ToolScope has no functions |
|
||||
| `tool_scope_default_tracker_has_no_loops` | Default ToolScope tracker detects no loops |
|
||||
|
||||
### src/mcp/mod.rs (30 new tests)
|
||||
|
||||
| Test name | What it verifies |
|
||||
|---|---|
|
||||
| `validate_stdio_with_command_succeeds` | Valid stdio spec passes |
|
||||
| `validate_stdio_missing_command_fails` | Stdio without command is rejected |
|
||||
| `validate_stdio_with_url_fails` | Stdio with url (remote field) is rejected |
|
||||
| `validate_stdio_with_headers_fails` | Stdio with headers (remote field) is rejected |
|
||||
| `validate_http_with_url_succeeds` | Valid http spec passes |
|
||||
| `validate_http_missing_url_fails` | Http without url is rejected |
|
||||
| `validate_http_with_command_fails` | Http with command (stdio field) is rejected |
|
||||
| `validate_http_with_args_fails` | Http with args (stdio field) is rejected |
|
||||
| `validate_http_with_cwd_fails` | Http with cwd (stdio field) is rejected |
|
||||
| `validate_sse_with_url_succeeds` | Valid SSE spec passes |
|
||||
| `validate_sse_missing_url_fails` | SSE without url is rejected |
|
||||
| `is_remote_true_for_http_and_sse` | Http and SSE are remote transports |
|
||||
| `is_remote_false_for_stdio` | Stdio is not remote |
|
||||
| `deserialize_stdio_server_from_json` | Full stdio spec from JSON |
|
||||
| `deserialize_http_server_from_json` | Http spec with headers from JSON |
|
||||
| `deserialize_env_with_mixed_types` | Env with String, Bool, Int values |
|
||||
| `deserialize_multiple_servers` | Multiple server entries parsed |
|
||||
| `deserialize_empty_servers_map` | Empty mcpServers map parsed |
|
||||
| `deserialize_server_with_cwd` | cwd field parsed correctly |
|
||||
| `resolve_all_returns_all_configured_servers` | "all" resolves to all config keys |
|
||||
| `resolve_comma_separated_returns_matching_servers` | Comma-separated list filters correctly |
|
||||
| `resolve_single_server_name` | Single name resolved |
|
||||
| `resolve_none_returns_empty` | None enabled → empty list |
|
||||
| `resolve_no_config_returns_empty` | No config → empty list |
|
||||
| `resolve_nonexistent_server_filtered_out` | Unknown names silently filtered |
|
||||
| `resolve_all_nonexistent_returns_empty` | All unknown → empty list |
|
||||
| `resolve_trims_whitespace` | Whitespace in comma list trimmed |
|
||||
| `registry_default_is_empty` | Default registry: empty, no config, no log |
|
||||
| `registry_with_config_reports_config` | Config accessor works |
|
||||
| `meta_function_prefixes_are_correct` | mcp_invoke/search/describe prefixes |
|
||||
|
||||
### src/config/request_context.rs (6 new tests)
|
||||
|
||||
| Test name | What it verifies |
|
||||
|---|---|
|
||||
| `rebuild_tool_scope_mcp_disabled_skips_servers` | mcp_server_support=false → empty runtime |
|
||||
| `rebuild_tool_scope_no_enabled_servers_yields_empty_runtime` | None enabled → empty runtime |
|
||||
| `rebuild_tool_scope_no_mcp_config_yields_empty_runtime` | No mcp_config → empty runtime |
|
||||
| `rebuild_tool_scope_preserves_tool_tracker` | Tracker survives rebuild |
|
||||
| `rebuild_tool_scope_repl_mode_appends_user_interaction_functions` | REPL adds user__ functions |
|
||||
| `rebuild_tool_scope_cmd_mode_no_user_interaction_functions` | CMD skips user__ functions |
|
||||
|
||||
**Total: 54 new tests (176 total in suite)**
|
||||
|
||||
## Bugs discovered
|
||||
|
||||
None.
|
||||
|
||||
## Observations for future iterations
|
||||
|
||||
1. **ConnectedServer untestable without subprocess**: `ConnectedServer`
|
||||
(= `RunningService<RoleClient, ()>`) cannot be constructed without
|
||||
a real MCP server subprocess. This blocks unit testing for:
|
||||
- McpFactory.acquire() full flow (spawn + insert + Weak sharing)
|
||||
- McpRuntime.insert/get with real handles
|
||||
- McpRuntime.search/describe/invoke (need live tool catalog)
|
||||
- All scope transition tests (role/session/agent MCP start/stop)
|
||||
|
||||
These require integration tests with a mock MCP server binary
|
||||
(e.g., a simple echo server). Recommended for a dedicated
|
||||
integration test iteration.
|
||||
|
||||
2. **McpServerKey sorting guarantees sharing correctness**: The
|
||||
sorting of args, env, and headers in McpServerKey::from_spec
|
||||
is critical — without it, HashMap key equality would be
|
||||
non-deterministic. Tests verify this explicitly.
|
||||
|
||||
3. **rebuild_tool_scope has 3 guard clauses that prevent server
|
||||
acquisition**: mcp_server_support=false, mcp_config=None,
|
||||
enabled_mcp_servers=None. All three paths tested.
|
||||
|
||||
4. **REPL vs CMD mode differs in user interaction functions**: The
|
||||
`rebuild_tool_scope` method conditionally appends `user__*`
|
||||
functions only in REPL mode. Tested both paths.
|
||||
|
||||
5. **McpServer::validate enforces strict transport/field separation**:
|
||||
Stdio servers cannot have url/headers, remote servers cannot have
|
||||
command/args/cwd. This prevents misconfiguration. All cross-field
|
||||
conflict cases tested.
|
||||
|
||||
6. **McpRegistry.resolve_server_ids is private** but tested via
|
||||
`#[cfg(test)]` in the same module. It's the core of server ID
|
||||
resolution for "all", comma-separated, and empty cases.
|
||||
|
||||
## Next iteration
|
||||
|
||||
Plan file 06: Tool Evaluation — eval_tool_calls, ToolCall dispatch,
|
||||
tool handlers, MCP tool invocation chain (mcp__search, mcp__describe,
|
||||
mcp__invoke).
|
||||
@@ -11,85 +11,105 @@ during scope transitions (role/session/agent enter/exit).
|
||||
## Behaviors to test
|
||||
|
||||
### MCP config loading
|
||||
- [ ] mcp.json parsed correctly from functions directory
|
||||
- [ ] Server specs include command, args, env, cwd
|
||||
- [x] mcp.json parsed correctly from functions directory
|
||||
- [x] Server specs include command, args, env, cwd
|
||||
- [ ] Vault secrets interpolated in mcp.json
|
||||
- [ ] Missing secrets reported as warnings
|
||||
- [ ] McpServersConfig stored on AppState.mcp_config
|
||||
- [x] McpServersConfig stored on AppState.mcp_config
|
||||
|
||||
### McpFactory
|
||||
- [ ] acquire() spawns new server when none active
|
||||
- [ ] acquire() returns existing handle via Weak upgrade
|
||||
- [ ] acquire() spawns fresh when Weak is dead
|
||||
- [ ] Multiple acquire() calls for same spec share handle
|
||||
- [ ] Different specs get different handles
|
||||
- [ ] McpServerKey built correctly from spec (sorted args/env)
|
||||
- [ ] acquire() spawns new server when none active (requires real subprocess)
|
||||
- [ ] acquire() returns existing handle via Weak upgrade (requires real subprocess)
|
||||
- [ ] acquire() spawns fresh when Weak is dead (requires real subprocess)
|
||||
- [ ] Multiple acquire() calls for same spec share handle (requires real subprocess)
|
||||
- [x] Different specs get different handles (via key inequality)
|
||||
- [x] McpServerKey built correctly from spec (sorted args/env)
|
||||
|
||||
### McpRuntime
|
||||
- [ ] insert() adds server handle by name
|
||||
- [ ] get() retrieves handle by name
|
||||
- [ ] server_names() returns all active names
|
||||
- [ ] is_empty() correct for empty/non-empty
|
||||
- [ ] search() finds tools by keyword (BM25 ranking)
|
||||
- [ ] describe() returns tool input schema
|
||||
- [ ] invoke() calls tool on server and returns result
|
||||
- [ ] insert() adds server handle by name (requires Arc<ConnectedServer>)
|
||||
- [ ] get() retrieves handle by name (requires Arc<ConnectedServer>)
|
||||
- [x] server_names() returns all active names
|
||||
- [x] is_empty() correct for empty/non-empty
|
||||
- [ ] search() finds tools by keyword (BM25 ranking) (requires live server)
|
||||
- [ ] describe() returns tool input schema (requires live server)
|
||||
- [ ] invoke() calls tool on server and returns result (requires live server)
|
||||
|
||||
### spawn_mcp_server
|
||||
- [ ] Builds Command from spec (command, args, env, cwd)
|
||||
- [ ] Creates TokioChildProcess transport
|
||||
- [ ] Completes rmcp handshake (serve)
|
||||
- [ ] Returns Arc<ConnectedServer>
|
||||
- [ ] Log file created when log_path provided
|
||||
- [ ] Builds Command from spec (command, args, env, cwd) (integration test)
|
||||
- [ ] Creates TokioChildProcess transport (integration test)
|
||||
- [ ] Completes rmcp handshake (serve) (integration test)
|
||||
- [ ] Returns Arc<ConnectedServer> (integration test)
|
||||
- [ ] Log file created when log_path provided (integration test)
|
||||
|
||||
### rebuild_tool_scope (MCP integration)
|
||||
- [ ] Empty enabled_mcp_servers → no servers acquired
|
||||
- [ ] "all" → all configured servers acquired
|
||||
- [ ] Comma-separated list → only listed servers acquired
|
||||
- [ ] Mapping resolution: alias → actual server key(s)
|
||||
- [ ] MCP meta functions appended for each started server
|
||||
- [ ] Old ToolScope dropped (releasing old server handles)
|
||||
- [ ] Loading spinner shown during acquisition
|
||||
- [ ] AbortSignal properly threaded through
|
||||
- [x] Empty enabled_mcp_servers → no servers acquired
|
||||
- [ ] "all" → all configured servers acquired (requires real subprocess)
|
||||
- [ ] Comma-separated list → only listed servers acquired (requires real subprocess)
|
||||
- [ ] Mapping resolution: alias → actual server key(s) (requires real subprocess)
|
||||
- [ ] MCP meta functions appended for each started server (requires real subprocess)
|
||||
- [ ] Old ToolScope dropped (releasing old server handles) (requires real subprocess)
|
||||
- [ ] Loading spinner shown during acquisition (UI test)
|
||||
- [ ] AbortSignal properly threaded through (integration test)
|
||||
|
||||
### Server lifecycle during scope transitions
|
||||
- [ ] Enter role with MCP: servers start
|
||||
- [ ] Exit role: servers stop (handle dropped)
|
||||
- [ ] Enter role with MCP: servers start (integration test)
|
||||
- [ ] Exit role: servers stop (handle dropped) (integration test)
|
||||
- [ ] Enter role A (MCP-X) → exit → enter role B (MCP-Y):
|
||||
X stops, Y starts
|
||||
X stops, Y starts (integration test)
|
||||
- [ ] Enter role with MCP → exit to no MCP: servers stop,
|
||||
global MCP restored
|
||||
global MCP restored (integration test)
|
||||
- [ ] Start REPL with global MCP → enter agent with different MCP:
|
||||
agent MCP takes over
|
||||
- [ ] Exit agent: agent MCP stops, global MCP restored
|
||||
agent MCP takes over (integration test)
|
||||
- [ ] Exit agent: agent MCP stops, global MCP restored (integration test)
|
||||
|
||||
### MCP tool invocation chain
|
||||
- [ ] LLM calls mcp__search_<server> → search results returned
|
||||
- [ ] LLM calls mcp__describe_<server> tool_name → schema returned
|
||||
- [ ] LLM calls mcp__invoke_<server> tool args → tool executed
|
||||
- [ ] Server not found → "MCP server not found in runtime" error
|
||||
- [ ] Tool not found → appropriate error
|
||||
- [ ] LLM calls mcp__search_<server> → search results returned (integration test)
|
||||
- [ ] LLM calls mcp__describe_<server> tool_name → schema returned (integration test)
|
||||
- [ ] LLM calls mcp__invoke_<server> tool args → tool executed (integration test)
|
||||
- [ ] Server not found → "MCP server not found in runtime" error (tested via McpRuntime.get)
|
||||
- [ ] Tool not found → appropriate error (requires live server)
|
||||
|
||||
### MCP support flag
|
||||
- [ ] mcp_server_support=false → no MCP servers started
|
||||
- [ ] mcp_server_support=false + agent with MCP → error (blocks)
|
||||
- [ ] mcp_server_support=false + role with MCP → warning, continues
|
||||
- [ ] .set mcp_server_support true → MCP servers start
|
||||
- [x] mcp_server_support=false → no MCP servers started
|
||||
- [ ] mcp_server_support=false + agent with MCP → error (blocks) (requires agent init)
|
||||
- [ ] mcp_server_support=false + role with MCP → warning, continues (requires role init)
|
||||
- [ ] .set mcp_server_support true → MCP servers start (requires live server)
|
||||
|
||||
### MCP in child agents
|
||||
- [ ] Child agent MCP servers acquired via factory
|
||||
- [ ] Child agent MCP runtime populated
|
||||
- [ ] Child agent MCP tool invocations work
|
||||
- [ ] Child agent exit drops MCP handles
|
||||
- [ ] Child agent MCP servers acquired via factory (integration test)
|
||||
- [ ] Child agent MCP runtime populated (integration test)
|
||||
- [ ] Child agent MCP tool invocations work (integration test)
|
||||
- [ ] Child agent exit drops MCP handles (integration test)
|
||||
|
||||
## Context switching scenarios (comprehensive)
|
||||
- [ ] No MCP → role with MCP → exit role → no MCP
|
||||
- [ ] Global MCP-A → role MCP-B → exit role → global MCP-A
|
||||
- [ ] Global MCP-A → agent MCP-B → exit agent → global MCP-A
|
||||
- [ ] Role MCP-A → session MCP-B (overrides) → exit session
|
||||
- [ ] Agent MCP → child agent MCP → child exits → parent MCP intact
|
||||
- [ ] No MCP → role with MCP → exit role → no MCP (integration test)
|
||||
- [ ] Global MCP-A → role MCP-B → exit role → global MCP-A (integration test)
|
||||
- [ ] Global MCP-A → agent MCP-B → exit agent → global MCP-A (integration test)
|
||||
- [ ] Role MCP-A → session MCP-B (overrides) → exit session (integration test)
|
||||
- [ ] Agent MCP → child agent MCP → child exits → parent MCP intact (integration test)
|
||||
- [ ] .set enabled_mcp_servers X → .set enabled_mcp_servers Y:
|
||||
X released, Y acquired
|
||||
- [ ] .set enabled_mcp_servers null → all released
|
||||
X released, Y acquired (integration test)
|
||||
- [ ] .set enabled_mcp_servers null → all released (integration test)
|
||||
|
||||
## Additional behaviors tested (not in original plan)
|
||||
|
||||
- [x] McpServerKey equality: same spec → equal keys
|
||||
- [x] McpServerKey inequality: different names → different keys
|
||||
- [x] McpServerKey inequality: different commands → different keys
|
||||
- [x] McpServerKey env coercion: Bool/Int → String
|
||||
- [x] McpFactory default has empty active map
|
||||
- [x] McpServer::is_remote() true for Http/Sse, false for Stdio
|
||||
- [x] McpServer::validate() all cross-field conflicts (6 cases)
|
||||
- [x] McpServersConfig: empty servers map, multiple servers, cwd field
|
||||
- [x] McpRegistry: default state, config accessor
|
||||
- [x] McpRegistry: resolve with whitespace trimming
|
||||
- [x] McpRegistry: resolve all-nonexistent returns empty
|
||||
- [x] rebuild_tool_scope: no mcp_config yields empty runtime
|
||||
- [x] rebuild_tool_scope: preserves tool_tracker across rebuild
|
||||
- [x] rebuild_tool_scope: REPL mode appends user interaction functions
|
||||
- [x] rebuild_tool_scope: CMD mode excludes user interaction functions
|
||||
- [x] MCP meta function name prefix constants are correct
|
||||
- [x] ToolScope default: empty functions, runtime, tracker
|
||||
|
||||
## Old code reference
|
||||
- `src/mcp/mod.rs` — McpRegistry, init, reinit, start/stop
|
||||
|
||||
Reference in New Issue
Block a user