Repository navigation
feat(*): fold a large tool catalog by default, with the meta-pair reserved #616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
3e6067d
9dff66d
0a1adc3
99722c3
1bd5d68
e7ffd8e
f339b7c
b15a353
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,12 +82,14 @@ def _report_reserved_disabled_tools(self) -> None: | |
| preference expressed by destroying its subject is one that cannot be | ||
| reversed: nothing remembered what to put back. | ||
|
|
||
| The MCP meta-tools are the one exemption, and it is theirs by ownership | ||
| rather than by policy: :meth:`_sync_mcp_meta_tools` registers them while | ||
| some connected server serves resources or prompts and withdraws them when | ||
| none does, so it owns those five names for the life of the loop. An entry | ||
| naming one is a preference nothing can act on -- reported here once, | ||
| ignored where the array is built. | ||
| Two groups are exempt, and both are theirs by ownership rather than by | ||
| policy. :meth:`_sync_mcp_meta_tools` registers the five MCP meta-tools | ||
| while some connected server serves resources or prompts and withdraws them | ||
| when none does, so it owns those names for the life of the loop. | ||
| ``tool_search`` and ``tool_call`` are owned the same way by | ||
| :meth:`_register_default_tools`, and ``tools.tool_search.enabled`` is the | ||
| switch that speaks for them. An entry naming either group is a preference | ||
| nothing can act on -- reported here once, ignored where the array is built. | ||
|
|
||
| Run after :meth:`_register_default_tools` and after MCP connect, so an | ||
| entry naming a tool from either group is resolvable by the time it is | ||
|
|
@@ -98,42 +100,73 @@ def _report_reserved_disabled_tools(self) -> None: | |
| is about the entry the operator can actually see -- most of them are in | ||
| the config file, which is no longer copied into ``_disabled_tools``. | ||
| """ | ||
| from raven.agent.tools.tool_search import META_TOOL_NAMES, TOOL_SEARCH_NAME | ||
| from raven.config.live import disabled_tool_names | ||
| from raven.mcp.prompts import PROMPT_TOOL_NAMES | ||
| from raven.mcp.resources import RESOURCE_TOOL_NAMES | ||
|
|
||
| entries = set(disabled_tool_names(self._live_config)) | self._disabled_tools | ||
| if not entries: | ||
| return | ||
| reserved = RESOURCE_TOOL_NAMES | PROMPT_TOOL_NAMES | ||
| mcp_reserved = RESOURCE_TOOL_NAMES | PROMPT_TOOL_NAMES | ||
| for entry in sorted(entries): | ||
| if entry in self._disabled_tools_reserved_warned: | ||
| continue | ||
| if any(name in reserved for name in self.tools.resolve_configured(entry)): | ||
| resolved = self.tools.resolve_configured(entry) | ||
| if any(name in mcp_reserved for name in resolved): | ||
| self._disabled_tools_reserved_warned.add(entry) | ||
| logger.warning( | ||
| "tools.disabled_tools names '{}', which raven registers and withdraws on its " | ||
| "own as MCP servers serving resources or prompts come and go. The entry has " | ||
| "no effect; remove it to keep the config honest.", | ||
| entry, | ||
| ) | ||
| elif any(name in META_TOOL_NAMES for name in resolved): | ||
| self._disabled_tools_reserved_warned.add(entry) | ||
| # One remedy per name, because the two are not owned the same | ||
| # way: tools.tool_search.enabled decides whether tool_search is | ||
| # registered at all, while tool_call is registered whatever that | ||
| # switch says -- it is the only route to a schema-hidden tool. | ||
| # A shared "turn the fold off" line sent an operator who wrote | ||
| # tool_call here to a setting that leaves it exactly where it was. | ||
| remedy = ( | ||
| "turn the fold off with tools.tool_search.enabled, which is what registers it" | ||
| if TOOL_SEARCH_NAME in resolved | ||
| else "this deploy has no switch for it: it is the only route to a tool whose " | ||
| "schema is not in the array, so it is registered whatever the fold is doing" | ||
| ) | ||
| logger.warning( | ||
| "tools.disabled_tools names '{}', which raven owns for the life of the loop. " | ||
| "The entry has no effect; remove it, or {}.", | ||
| entry, | ||
| remedy, | ||
| ) | ||
|
|
||
| def _withheld_tool_names(self) -> frozenset[str]: | ||
| """Which tools are not on offer right now, read live. | ||
|
|
||
| The registry asks this when it assembles a tool array. Reserved names are | ||
| removed here rather than at the switch: ``_sync_mcp_meta_tools`` owns those | ||
| five for the life of the loop (it registers them while some connected | ||
| server serves resources or prompts and withdraws them when none does), so | ||
| an entry naming one is a preference the loop cannot honour -- reported | ||
| above, ignored here. | ||
| removed here rather than at the switch: ``_sync_mcp_meta_tools`` owns the | ||
| five MCP meta-tools for the life of the loop (it registers them while some | ||
| connected server serves resources or prompts and withdraws them when none | ||
| does), so an entry naming one is a preference the loop cannot honour -- | ||
| reported above, ignored here. | ||
|
|
||
| ``tool_search`` and ``tool_call`` are reserved for that reason and one | ||
| more. Their absence from the array is how ``ToolSearchStrategy`` reads | ||
| "this request has no search route", and it answers by shipping every | ||
| schema instead -- so an off switch here would not slim the array, it would | ||
| unfold it, and mid-turn at that, because this source is read once per | ||
| assembly. The switch that speaks for the fold is | ||
| ``tools.tool_search.enabled``. | ||
|
|
||
| Constructor-supplied names are unioned in because an eval harness passes | ||
| them directly rather than through a config file; a file-less run would | ||
| otherwise lose its blacklist entirely. Nothing copies the config's own | ||
| list in there -- see the note in ``__init__`` on why that made the switch | ||
| one-way. | ||
| """ | ||
| from raven.agent.tools.tool_search import META_TOOL_NAMES | ||
| from raven.config.live import disabled_tool_names | ||
| from raven.mcp.prompts import PROMPT_TOOL_NAMES | ||
| from raven.mcp.resources import RESOURCE_TOOL_NAMES | ||
|
|
@@ -143,7 +176,7 @@ def _withheld_tool_names(self) -> frozenset[str]: | |
| for entry in configured: | ||
| withheld.update(self.tools.resolve_configured(entry)) | ||
| withheld.update(self._unconfigured_tool_names()) | ||
| return frozenset(withheld - (RESOURCE_TOOL_NAMES | PROMPT_TOOL_NAMES)) | ||
| return frozenset(withheld - (RESOURCE_TOOL_NAMES | PROMPT_TOOL_NAMES | META_TOOL_NAMES)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question - is this reservation meant to be one-sided, or does the charter path need it too? This subtraction closes the
I want to be straight that I did not reproduce this and that the resulting behaviour looks correct rather than broken. A charter-narrowed turn hits the So this is a question about intent rather than a claim of defect. If a charter is expected to be able to take the pair away, the docstring's "an off switch here would not slim the array, it would unfold it" is worth qualifying, because that is precisely what the one remaining route does. |
||
|
|
||
| def _unconfigured_tool_names(self) -> set[str]: | ||
| """Registered tools whose config asks for nothing right now, read live. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -262,15 +262,18 @@ def resolve_target(self, name: Any) -> _Target: | |
| return _Target(None, f"Error: '{name}' cannot be invoked via tool_call.") | ||
| tool = self._registry.get(name) | ||
| if tool is None or not self._registry.offers(tool): | ||
| tail = " -- tool_search lists what is currently loaded" if self._search_visible() else "" | ||
| tail = " -- tool_search lists what is currently loaded" if self.search_visible() else "" | ||
| return _Target(None, absent_tool_error(name, tail=tail)) | ||
| return _Target(tool, None) | ||
|
|
||
| def _search_visible(self) -> bool: | ||
| def search_visible(self) -> bool: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocker - route the host's other two claims about the fold through this gate, or derive them from the post-fold array. This method was made public for exactly this job. Its own docstring says it is "the honest gate on anything the host says about the fold", and
Reproduced. I built a main-agent loop with The model is not stranded: The mechanism is pre-existing and this PR does not touch it: render.py:125, identity.py:85 and active_skills.py:63 are byte-identical at d50a13c. What changed is One line to correct either way: No test pairs the fold with either surface. No test file referencing Two directions close it and they are not equivalent. Gate both surfaces on the fold state the way the MCP notice now is, or add |
||
| """Whether ``tool_search`` reaches the model's tool list this turn. | ||
|
|
||
| The old shape of :meth:`tool_call_available`, kept but moved to the | ||
| meta-tool it is actually true of. ``tool_search`` is registered only | ||
| Public because it is also the honest gate on anything the host says | ||
| about the fold: ``_mcp_tool_notices`` tells the model a connected | ||
| server's schemas are withheld, and that sentence is true only while | ||
| this is. The old shape of :meth:`tool_call_available`, kept but moved to | ||
| the meta-tool it is actually true of. ``tool_search`` is registered only | ||
| where the fold is configured on, withheld by the same off switch as | ||
| anything else, and dropped from the request again below the threshold -- | ||
| so a pointer at it is dishonest in three separate states, not one. | ||
|
|
@@ -435,6 +438,23 @@ async def execute(self, name: str, arguments: dict[str, Any] | None = None) -> s | |
| return await self._ctrl.call(name, arguments) | ||
|
|
||
|
|
||
| def _schema_name(entry: Any) -> str | None: | ||
| """The name a tool definition carries, or ``None`` for a shape this strategy | ||
| cannot read. | ||
|
|
||
| The arrays raven assembles are always well formed; replay is what is not. | ||
| It feeds a recorded array back through this same chain, so a corrupt | ||
| recording arrives here as an entry whose ``function`` is a string -- and | ||
| that has to end at replay's own backstop as an unreplayable recording, | ||
| not as a TypeError thrown from a strategy that assumed the shape. An entry | ||
| this cannot read is counted as a tool and passed through untouched: the | ||
| fold may only withhold a schema it recognised. | ||
| """ | ||
| function = entry.get("function") if isinstance(entry, dict) else None | ||
| name = function.get("name") if isinstance(function, dict) else None | ||
| return name if isinstance(name, str) else None | ||
|
|
||
|
|
||
| class ToolSearchStrategy(TokenStrategy): | ||
| """``before_llm_call`` hook that compacts the tool list for large catalogs. | ||
|
|
||
|
|
@@ -470,16 +490,16 @@ async def before_llm_call( | |
| if not tools: | ||
| return messages, tools, model | ||
| self._ctrl.refresh() | ||
| catalog_size = sum(1 for t in tools if t["function"]["name"] not in META_TOOL_NAMES) | ||
| catalog_size = sum(1 for t in tools if _schema_name(t) not in META_TOOL_NAMES) | ||
| if catalog_size <= self._compaction_threshold: | ||
| out = [t for t in tools if t["function"]["name"] != TOOL_SEARCH_NAME] | ||
| out = [t for t in tools if _schema_name(t) != TOOL_SEARCH_NAME] | ||
| return messages, out, model | ||
| present = {t["function"]["name"] for t in tools} | ||
| present = {name for t in tools if (name := _schema_name(t)) is not None} | ||
| if not META_TOOL_NAMES <= present: | ||
| # Meta-tools unavailable (e.g. removed via disabled_tools): expose | ||
| # everything rather than strand the cataloged tools behind a search | ||
| # the model cannot invoke. | ||
| return messages, tools, model | ||
| visible = self._ctrl.visible_names() | ||
| out = [t for t in tools if t["function"]["name"] in visible] | ||
| out = [t for t in tools if (name := _schema_name(t)) is None or name in visible] | ||
| return messages, out, model | ||
|
gloryfromca marked this conversation as resolved.
|
Uh oh!
There was an error while loading. Please reload this page.