Severity: Low · Area: Tests / quality · Category: test-coverage
Location: services/agent/selene_agent/utils/mcp_client_manager.py:1
What's wrong
Every test in the suite replaces mcp_manager with MagicMock; there are no tests for MCPClientManager itself (stdio subprocess spawn/restart, execute_tool routing/name resolution, error propagation, shutdown). Since all tool calls from the orchestrator and every autonomy handler funnel through execute_tool, a regression here breaks all 11 modules at once with zero test signal.
How it fails
A change to execute_tool's result decoding (string vs dict envelopes — a shape the reminder handler explicitly special-cases) regresses; every consumer test passes because they mock the manager, and the failure only surfaces as live tool calls returning garbage to the LLM.
Suggested fix
Add unit tests with a fake in-process MCP session covering: tool-name routing to the right module, result envelope normalization, unknown-tool error, and a subprocess-death/restart path (can stub the transport).
Adversarial verification — both skeptics confirmed
Skeptic 1 (confirmed) — Confirmed. No test references MCPClientManager (grep across services/agent/tests/ returns nothing); every consumer test injects mcp_manager=MagicMock() and stubs execute_tool with AsyncMock. All tool execution funnels through execute_tool (orchestrator.py:1005 and autonomy handlers), whose routing, envelope flattening (mcp_client_manager…
Skeptic 2 (confirmed) — Confirmed. Repo-wide grep shows no test references MCPClientManager or mcp_client_manager (services/agent/selene_agent/utils/mcp_client_manager.py, 397 lines) — only production consumers import it. Every test touching tool execution injects mcp_manager=MagicMock()/AsyncMock (test_act_handler.py, test_reminder_handler.py, test_companion_v…
Filed from a multi-agent audit of 855f5cc: 16 reviewers over ~42k lines produced 170 raw findings; each was handed to 2 independent agents prompted to refute it, and only findings both confirmed were kept (19 refuted, 10 split-verdict, 127 unique confirmed). Line numbers are 1-indexed against 855f5cc and will drift as the code changes.
Severity: Low · Area: Tests / quality · Category:
test-coverageLocation:
services/agent/selene_agent/utils/mcp_client_manager.py:1What's wrong
Every test in the suite replaces mcp_manager with MagicMock; there are no tests for MCPClientManager itself (stdio subprocess spawn/restart, execute_tool routing/name resolution, error propagation, shutdown). Since all tool calls from the orchestrator and every autonomy handler funnel through execute_tool, a regression here breaks all 11 modules at once with zero test signal.
How it fails
A change to execute_tool's result decoding (string vs dict envelopes — a shape the reminder handler explicitly special-cases) regresses; every consumer test passes because they mock the manager, and the failure only surfaces as live tool calls returning garbage to the LLM.
Suggested fix
Add unit tests with a fake in-process MCP session covering: tool-name routing to the right module, result envelope normalization, unknown-tool error, and a subprocess-death/restart path (can stub the transport).
Adversarial verification — both skeptics confirmed
Filed from a multi-agent audit of
855f5cc: 16 reviewers over ~42k lines produced 170 raw findings; each was handed to 2 independent agents prompted to refute it, and only findings both confirmed were kept (19 refuted, 10 split-verdict, 127 unique confirmed). Line numbers are 1-indexed against855f5ccand will drift as the code changes.