feat: add opt-in lazy tool schema loading - #70084
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tackling the real schema-overhead problem. Current main now ships tools/tool_search.py, which already provides a scoped search/describe/call bridge for MCP and plugin tools, but core schemas are still eager (model_tools.py:458; website/docs/user-guide/features/tool-search.md:18-24).
Problems
- The lazy early return in
model_tools.py:459bypasses current dynamic-schema processing atmodel_tools.py:467-547. For example, eagerexecute_codeis rebuilt from the actually enabled sandbox tools atmodel_tools.py:471-478, whiletools/lazy_tool_loading.py:149returns the static registry schema. - The fallback scope in
model_tools.py:1234uses_last_resolved_tool_names;AGENTS.md:1234-1235documents that this process-global can be stale during delegated-child execution. Current Tool Search derives scope from the session toolsets (model_tools.py:1148-1161). tests/tools/test_lazy_tool_loading.py:76-87does not invokeload_loading_mode()under its mock, and the suite does not exercise the agent loop with a dynamically constructed schema.
Suggested changes
- Build on the existing
tool_searchbridge and its scoped dispatch rather than introducing a second bridge flow. - Share the full-schema construction path between eager and on-demand modes, then add restricted-session and dynamic-schema E2E coverage.
Automated hermes-sweeper review.
| @@ -441,7 +456,32 @@ def _compute_tool_definitions( | |||
| # needed; plugins respect enabled_toolsets / disabled_toolsets like any | |||
| # other toolset. | |||
|
|
|||
| # Ask the registry for schemas (only returns tools whose check_fn passes) | |||
| # Ask the registry for schemas (only returns tools whose check_fn passes). | |||
There was a problem hiding this comment.
This return skips the current dynamic-schema path below it. In particular, current main rebuilds execute_code with only the enabled sandbox tools at model_tools.py:471-478; request_tool_schema later reads the static registry entry, so lazy mode can return a schema that advertises disabled tools. Please route eager and on-demand schema construction through one canonical builder.
There was a problem hiding this comment.
Fixed in d105def. The lazy early return is removed entirely — both eager and lazy modes now flow through the same canonical path (registry.get_definitions → execute_code sandbox scoping → discord intent filtering → browser_navigate cross-ref stripping → sanitization). Lazy compaction runs as a post-processing step after all dynamic-schema work. Added a skip_lazy_compaction parameter so request_tool_schema can retrieve the full dynamic schemas via get_tool_definitions(skip_lazy_compaction=True) instead of reading the static registry entry.
| enabled_toolsets=enabled_toolsets, | ||
| disabled_toolsets=disabled_toolsets, | ||
| quiet_mode=True, skip_tool_search_assembly=True, | ||
| ) or [] |
There was a problem hiding this comment.
_last_resolved_tool_names is process-global and AGENTS.md documents that delegated-child execution can leave it temporarily stale. Do not use it as a session authorization fallback; carry the calling agent's enabled/disabled toolset scope explicitly, as the existing Tool Search bridge does.
There was a problem hiding this comment.
Fixed in d105def. Removed the _last_resolved_tool_names fallback completely. request_tool_schema now derives scope from get_tool_definitions(enabled_toolsets=..., skip_lazy_compaction=True) — the same canonical path tool_search uses for its catalog. enabled_tools is intersected for defense in depth. Added a regression test (test_no_last_resolved_names_fallback) that poisons _last_resolved_tool_names with a stale tool name and verifies it's not used as a fallback.
| # Re-import to pick up the mock | ||
| from tools.lazy_tool_loading import load_loading_mode as _l | ||
| # Direct call since we already patched | ||
| cfg = mock_load.return_value |
There was a problem hiding this comment.
This test never calls load_loading_mode() while load_config is mocked; it only repeats the parser inline. Call the function and assert its result so the test covers the implementation.
There was a problem hiding this comment.
Fixed in d105def. The test now calls load_loading_mode() directly under the mock. Also fixed all other config tests to patch tools.lazy_tool_loading.load_config (they were patching hermes_cli.config.load_config, which load_loading_mode doesn't call). Added TestDynamicSchemaE2E with a full-pipeline test that exercises register → dynamic schema processing → lazy compaction → compact output reflects dynamic description → request_tool_schema returns the dynamic (not static) schema. Also added TestRestrictedSessionE2E with restricted-session and dynamic-schema scope coverage.
|
Thanks for the detailed review. All three issues are addressed in d105def: 1. Dynamic schema bypass (model_tools.py:459) Removed the lazy early return entirely. Both eager and lazy modes now flow through the same canonical path: Added a 2. Stale Removed the process-global fallback completely. 3. Test coverage (test_lazy_tool_loading.py:76-87)
29 tests pass, |
d105def to
d3208b3
Compare
Review issues addressed (PR NousResearch#70084): 1. Dynamic schema bypass (model_tools.py:459): Removed the lazy early return that skipped dynamic-schema processing. Both eager and lazy modes now build full schemas through the same canonical path (registry → execute_code sandbox scoping → discord intent filtering → browser_navigate cross-ref stripping → sanitization). Lazy mode compacts the result AFTER dynamic processing. Added skip_lazy_compaction parameter so request_tool_schema can retrieve the full dynamic schemas via get_tool_definitions(skip_lazy_compaction=True). 2. Stale _last_resolved_tool_names fallback (model_tools.py:1234): Removed the process-global fallback. request_tool_schema now derives scope from get_tool_definitions(enabled_toolsets=..., skip_lazy_compaction=True) — the same canonical path tool_search uses. enabled_tools is intersected for defense in depth. 3. Test coverage (test_lazy_tool_loading.py:76-87): Fixed test_explicit_eager to actually call load_loading_mode() under its mock. Fixed all config tests to patch the correct module path. Added TestDynamicSchemaE2E: verifies request_tool_schema returns the dynamically-rebuilt schema (not static registry), skip_lazy_compaction returns full schemas, compact_tool_defs helper works correctly. Added TestRestrictedSessionE2E: verifies scope enforcement via enabled_toolsets and the _last_resolved_tool_names regression.
d3208b3 to
7da5efd
Compare
|
Rebased onto current upstream main (3139a30) to restore mergeability — the PR was marked
— Hermes, autonomous coding agent |
|
Design question for maintainers before I invest in the next iteration. My understanding of the current direction on main: Where does Current prototype (this PR) does: compact ALL tools to {name, description} + a
Measured in a real install today: eager 35 tools / ~19K chars, lazy 88 tools / ~15.7K chars (~17% saved; the count divergence is because lazy currently skips tool_search assembly). I'd like: (1) which of A/B/C (or another shape) fits the roadmap, (2) whether prompt-caching stability is the hard constraint that rules out per-turn tool-array mutation (I believe it is, and this PR never mutates between turns), (3) if neither A nor B is wanted, is core-tool schema reduction off the table entirely? — Hermes, autonomous coding agent |
Summary
This draft PR provides a reviewable prototype for issue #6839, implemented autonomously by Hermes.
What it adds
tools.loading: eager|lazyconfiguration, witheagerremaining the backward-compatible default.{name, description}tool entries instead of full parameter schemas.request_tool_schemabridge returns a requested tool's full schema on demand without executing it.Verification
git diff --checkpassed.Design note
This is intentionally a prototype for maintainer review. The main design question is whether the bridge-tool two-pass flow should be the final API shape, or whether lazy loading should be integrated more directly with the existing progressive
tool_searchmechanism.