Private
Public Access
refactor(mcp_tool_specs): delete redundant AGENT_TOOL_NAMES; use tool_names() at consumer sites
AGENT_TOOL_NAMES was a hardcoded snapshot of mcp_tool_specs.tool_names()
in src/models.py. The pre-existing test
test_tool_names_subset_of_models_agent_tool_names literally asserted
'tool_names() ⊆ AGENT_TOOL_NAMES' (proving the redundancy), and
AGENT_TOOL_NAMES was not maintained in lockstep with the registry
(it would silently drift if a new tool was added).
This commit:
1. Deletes AGENT_TOOL_NAMES from src/models.py (replaced by an
explanatory comment in the Constants section).
2. Updates 3 consumer sites in src/app_controller.py:
- 'for t in models.AGENT_TOOL_NAMES' -> 'for t in mcp_tool_specs.tool_names()'
- (in 2 methods: __init__ + a setter)
3. Updates 2 test sites in tests/test_arch_boundary_phase2.py:
- 'from src.models import AGENT_TOOL_NAMES' -> 'from src import mcp_tool_specs'
- 'AGENT_TOOL_NAMES' references -> 'mcp_tool_specs.tool_names()'
4. Removes the tautology test
test_tool_names_subset_of_models_agent_tool_names from
tests/test_mcp_tool_specs.py (it asserted 'AGENT_TOOL_NAMES
superset of tool_names()' which becomes meaningless after
AGENT_TOOL_NAMES is deleted). Also removes the now-unused
'from src import models' import from that test file.
Verification: VC9
git grep 'AGENT_TOOL_NAMES' -- 'src/*.py' 'tests/*.py' # 0 hits
from src import mcp_tool_specs
mcp_tool_specs.tool_names() # returns the canonical 45 tools
from src.app_controller import AppController # uses the new path
Tests verified (15/16 PASS; 1 pre-existing failure unrelated to this
commit):
tests/test_arch_boundary_phase2.py (6 tests; 1 pre-existing
failure: test_rejection_prevents_dispatch
is a dialog-mock issue that
predates Phase 4)
tests/test_mcp_tool_specs.py (10 tests; the tautology test was removed;
the remaining 10 pass)
This commit is contained in:
@@ -13,24 +13,25 @@ class TestArchBoundaryPhase2(unittest.TestCase):
|
||||
|
||||
def test_toml_exposes_all_dispatch_tools(self) -> None:
|
||||
"""manual_slop.toml [agent.tools] must list every tool in mcp_client.dispatch()."""
|
||||
from src import models
|
||||
|
||||
from src import mcp_tool_specs
|
||||
|
||||
# We check the tool names in the source of mcp_client.dispatch
|
||||
import inspect
|
||||
import src.mcp_client as mcp
|
||||
source = inspect.getsource(mcp.dispatch)
|
||||
# This is a bit dynamic, but we can check if it covers our core tool names
|
||||
for tool in models.AGENT_TOOL_NAMES:
|
||||
for tool in mcp_tool_specs.tool_names():
|
||||
if tool not in ("set_file_slice", "py_update_definition", "py_set_signature", "py_set_var_declaration"):
|
||||
# Non-mutating tools should definitely be handled
|
||||
pass
|
||||
def test_toml_mutating_tools_disabled_by_default(self) -> None:
|
||||
"""Verify that the core set of read-only tools is present."""
|
||||
from src.models import AGENT_TOOL_NAMES
|
||||
from src import mcp_tool_specs
|
||||
tool_names = mcp_tool_specs.tool_names()
|
||||
# Our architecture now uses a fixed set of high-signal tools
|
||||
self.assertIn("read_file", AGENT_TOOL_NAMES)
|
||||
self.assertIn("list_directory", AGENT_TOOL_NAMES)
|
||||
self.assertIn("py_get_skeleton", AGENT_TOOL_NAMES)
|
||||
self.assertIn("read_file", tool_names)
|
||||
self.assertIn("list_directory", tool_names)
|
||||
self.assertIn("py_get_skeleton", tool_names)
|
||||
|
||||
def test_mcp_client_dispatch_completeness(self) -> None:
|
||||
"""Verify that all tools in tool_schemas are handled by dispatch()."""
|
||||
|
||||
@@ -7,7 +7,6 @@ Phase 1 of any_type_componentization_20260621. Verifies:
|
||||
- get_tool_schemas() returns the expected list
|
||||
- ToolParameter / ToolSpec dataclasses have correct frozen=True semantics
|
||||
- to_dict() round-trip preserves the legacy dict shape
|
||||
- Cross-module invariant: tool_names() == models.AGENT_TOOL_NAMES subset
|
||||
|
||||
CONVENTION: 1-space indentation. NO COMMENTS.
|
||||
"""
|
||||
@@ -15,7 +14,6 @@ from __future__ import annotations
|
||||
|
||||
import pytest
|
||||
from src import mcp_tool_specs
|
||||
from src import models
|
||||
|
||||
|
||||
EXPECTED_TOOLS: set[str] = {
|
||||
@@ -107,14 +105,6 @@ def test_tool_parameter_to_dict_includes_enum() -> None:
|
||||
assert 'before' in d['enum']
|
||||
|
||||
|
||||
def test_tool_names_subset_of_models_agent_tool_names() -> None:
|
||||
"""Cross-module invariant: every MCP tool is also an agent tool."""
|
||||
native_names = mcp_tool_specs.tool_names()
|
||||
agent_names = set(models.AGENT_TOOL_NAMES)
|
||||
missing_in_agent = native_names - agent_names
|
||||
assert not missing_in_agent, f"Native tools not in AGENT_TOOL_NAMES: {missing_in_agent}"
|
||||
|
||||
|
||||
def test_register_idempotent_replaces_existing() -> None:
|
||||
"""register() should overwrite (idempotent for hot-reload scenarios)."""
|
||||
from src.mcp_tool_specs import ToolSpec, ToolParameter, register
|
||||
|
||||
Reference in New Issue
Block a user