[NOTICKET] refactor(tools): single source of truth for data-access tool names + clarify input_schema is presence-only
Browse filesR11 (TAB slice): the data-access tool-name set was duplicated between invoker.py and planner/registry.py, synced only by a keep-in-lockstep comment. A 5th/renamed tool added in one place but not the other would silently mis-route (registry knows it, CompositeToolInvoker rejects it). Define canonical DATA_ACCESS_TOOLS once in tools/data_access.py; invoker.py imports it. (planner/registry.py + _PLACEHOLDER_RE dedup are the agent-side remainder.)
T1: ToolSpec.input_schema comment said "validates ToolCall.args" but TaskRunner._validate_args is presence-only; properties types are documentation, not enforced. Clarified so nobody assumes type-safety.
Doc/comment + a constant move; no behavior change. 41 tool tests pass, ruff clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- src/tools/contracts.py +7 -1
- src/tools/data_access.py +10 -0
- src/tools/invoker.py +2 -10
|
@@ -36,7 +36,13 @@ from pydantic import BaseModel, Field
|
|
| 36 |
class ToolSpec(BaseModel):
|
| 37 |
name: str
|
| 38 |
category: str # analytics.query | .aggregation | .timeseries | ...
|
| 39 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 40 |
output_kind: str # the ToolOutput.kind it returns
|
| 41 |
description: str # prompt-style: what it does, edge cases, what NOT to use it for
|
| 42 |
phase: Literal["P0", "P1", "P2"] = "P0"
|
|
|
|
| 36 |
class ToolSpec(BaseModel):
|
| 37 |
name: str
|
| 38 |
category: str # analytics.query | .aggregation | .timeseries | ...
|
| 39 |
+
# JSON-schema-ish dict: {"required": [...], "properties": {arg: {"type": ...}}}.
|
| 40 |
+
# VALIDATION CONTRACT — presence only: TaskRunner._validate_args enforces just
|
| 41 |
+
# `required` (each must resolve to a non-None arg). The `properties` types are
|
| 42 |
+
# DOCUMENTATION for the planner prompt, NOT checked at runtime — a wrong-typed
|
| 43 |
+
# arg passes validation and only surfaces (if at all) inside the compute fn.
|
| 44 |
+
# Do not assume type-safety here.
|
| 45 |
+
input_schema: dict[str, Any]
|
| 46 |
output_kind: str # the ToolOutput.kind it returns
|
| 47 |
description: str # prompt-style: what it does, edge cases, what NOT to use it for
|
| 48 |
phase: Literal["P0", "P1", "P2"] = "P0"
|
|
@@ -40,6 +40,16 @@ from src.tools.contracts import ToolOutput
|
|
| 40 |
|
| 41 |
DispatcherFactory = Callable[[Catalog], ExecutorDispatcher]
|
| 42 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 43 |
|
| 44 |
class Retriever(Protocol):
|
| 45 |
"""Minimal interface this invoker needs from the retrieval layer."""
|
|
|
|
| 40 |
|
| 41 |
DispatcherFactory = Callable[[Catalog], ExecutorDispatcher]
|
| 42 |
|
| 43 |
+
# Canonical set of data-access tool names — the single source of truth for which
|
| 44 |
+
# tools this invoker serves. `CompositeToolInvoker` imports it to route by name;
|
| 45 |
+
# the planner registry should derive its data-access spec names from it (agent ->
|
| 46 |
+
# tool is the correct dependency direction). Defining it once here means
|
| 47 |
+
# adding/renaming a data-access tool can't silently drift the router out of sync
|
| 48 |
+
# from the registry (R11). Must match the names in `DataAccessToolInvoker.invoke`.
|
| 49 |
+
DATA_ACCESS_TOOLS: frozenset[str] = frozenset(
|
| 50 |
+
{"list_sources", "describe_source", "query_structured", "retrieve_documents"}
|
| 51 |
+
)
|
| 52 |
+
|
| 53 |
|
| 54 |
class Retriever(Protocol):
|
| 55 |
"""Minimal interface this invoker needs from the retrieval layer."""
|
|
@@ -37,7 +37,7 @@ from src.tools.analytics import (
|
|
| 37 |
temporal,
|
| 38 |
)
|
| 39 |
from src.tools.contracts import ToolOutput
|
| 40 |
-
from src.tools.data_access import DataAccessToolInvoker
|
| 41 |
|
| 42 |
# tool name -> (compute callable, ToolOutput.kind it produces). Kept in lockstep
|
| 43 |
# with src/tools/registry.py output_kind values.
|
|
@@ -81,14 +81,6 @@ class AnalyticsToolInvoker:
|
|
| 81 |
return ToolOutput(tool=tool_name, kind=kind, value=result)
|
| 82 |
|
| 83 |
|
| 84 |
-
# Tool names served by the stateful data-access invoker (catalog + query +
|
| 85 |
-
# retrieval). Everything else is an analyze_* tool and goes to the analytics
|
| 86 |
-
# invoker. Kept in lockstep with _DATA_ACCESS_TOOLS in planner/registry.py.
|
| 87 |
-
_DATA_ACCESS_TOOLS: frozenset[str] = frozenset(
|
| 88 |
-
{"query_structured", "retrieve_documents", "list_sources", "describe_source"}
|
| 89 |
-
)
|
| 90 |
-
|
| 91 |
-
|
| 92 |
class CompositeToolInvoker:
|
| 93 |
"""One `invoke()` for the whole tool surface (KM-465 #4).
|
| 94 |
|
|
@@ -116,7 +108,7 @@ class CompositeToolInvoker:
|
|
| 116 |
self._analytics = analytics or AnalyticsToolInvoker()
|
| 117 |
|
| 118 |
async def invoke(self, tool_name: str, args: dict[str, Any]) -> ToolOutput:
|
| 119 |
-
if tool_name in
|
| 120 |
return await self._data_access.invoke(tool_name, args)
|
| 121 |
return await self._analytics.invoke(tool_name, args)
|
| 122 |
|
|
|
|
| 37 |
temporal,
|
| 38 |
)
|
| 39 |
from src.tools.contracts import ToolOutput
|
| 40 |
+
from src.tools.data_access import DATA_ACCESS_TOOLS, DataAccessToolInvoker
|
| 41 |
|
| 42 |
# tool name -> (compute callable, ToolOutput.kind it produces). Kept in lockstep
|
| 43 |
# with src/tools/registry.py output_kind values.
|
|
|
|
| 81 |
return ToolOutput(tool=tool_name, kind=kind, value=result)
|
| 82 |
|
| 83 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 84 |
class CompositeToolInvoker:
|
| 85 |
"""One `invoke()` for the whole tool surface (KM-465 #4).
|
| 86 |
|
|
|
|
| 108 |
self._analytics = analytics or AnalyticsToolInvoker()
|
| 109 |
|
| 110 |
async def invoke(self, tool_name: str, args: dict[str, Any]) -> ToolOutput:
|
| 111 |
+
if tool_name in DATA_ACCESS_TOOLS:
|
| 112 |
return await self._data_access.invoke(tool_name, args)
|
| 113 |
return await self._analytics.invoke(tool_name, args)
|
| 114 |
|