fix(model_tools): log warning when enabled/disabled toolset is unknown - #24095
briandevans wants to merge 2 commits into
Conversation
|
CI audit — all 3 failures ( The new |
There was a problem hiding this comment.
Pull request overview
This PR improves observability for toolset configuration by emitting logger.warning when unknown entries appear in enabled_toolsets / disabled_toolsets, ensuring misconfigurations are visible in production logs even when quiet_mode=True.
Changes:
- Add
logger.warning(...)on unknown toolset rejection paths in_compute_tool_definitions, while preserving the interactiveprint(...)output. - Add regression tests validating that unknown toolsets log warnings in
quiet_mode=True, and that known toolsets do not.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| model_tools.py | Logs warnings when toolsets are unknown so production (quiet) paths don’t silently drop misconfigured toolsets. |
| tests/test_model_tools.py | Adds caplog-based regression tests for warning emission on unknown toolsets in quiet mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # silently drop misnamed toolsets. The cache key includes | ||
| # registry._generation, so this only fires once per unique | ||
| # (config, registry-state) pair — not on every turn. (#23997) |
alt-glitch
added
type/bug
16fc166 to
01af6ca
Compare
|
@copilot Addressed in commit 3a4964f6bc: clarified the comment to distinguish the cached (quiet_mode=True) vs uncached (interactive) paths. In quiet_mode the cache in |
3a4964f to
b3f6d48
Compare
b3f6d48 to
959201a
Compare
9a1097b to
a5b3752
Compare
a5b3752 to
7bd331c
Compare
7bd331c to
5bb94cc
Compare
5bb94cc to
2943864
Compare
Every production code path that calls `get_tool_definitions` —
cron, gateway, oneshot, TUI, delegate, ACP — passes `quiet_mode=True`,
which suppressed the user-facing `print("⚠️ Unknown toolset")` line in
`_compute_tool_definitions`. The result: misnamed toolsets in
`enabled_toolsets` / `disabled_toolsets` were silently dropped with no
log evidence anywhere. In NousResearch#23997 this hid an MCP server-name misconfig
across 283 cron sessions on a 13-profile fleet for ~90 min until the
reporter read the source.
Surface the rejection through `logger.warning` so it lands in
`gateway.log` / cron logs regardless of `quiet_mode`. The
existing `print()` for interactive sessions is unchanged. The warning
fires once per unique `(config, registry-generation)` pair via the
existing memoization in `get_tool_definitions`, so a stable misconfig
yields one log line at boot, not one per turn — and when MCP discovery
later bumps `registry._generation` and resolves the alias, the cache
invalidates and the next compute simply succeeds.
Tests assert both additive and subtractive paths emit a WARNING-level
record in `quiet_mode=True`, and that recognized toolsets do NOT
trigger the warning.
Fixes NousResearch#23997 (visibility piece — option (c) in the reporter's
suggested fixes; option (a)/(b) live deeper in the cron tool-resolution
path and are intentionally left out of this PR's scope).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…set warning The original comment claimed the unknown-toolset logger.warning "only fires once per unique (config, registry-state) pair" without noting that the underlying cache only applies when quiet_mode=True. In interactive mode the cache is bypassed and the warning may repeat. Comment now explicitly distinguishes the two paths. Per Copilot review on NousResearch#24095. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2943864 to
592b890
Compare
|
Housekeeping: closing to keep my open-PR set focused on actively-reviewed work. This has been open ~20d without maintainer review and the surrounding code has continued to move, so it's unlikely to land as-is. The underlying fix still stands — happy to reopen and rebase if it would be useful. Thanks! |
What does this PR do?
Every production code path that calls
get_tool_definitionspassesquiet_mode=True(cron, gateway, oneshot, TUI, delegate, ACP, curator). The⚠️ Unknown toolsetline in_compute_tool_definitionsis a bareprint()gated onif not quiet_mode, so in real runtime an unrecognized entry inenabled_toolsets/disabled_toolsetsis silently dropped with no record anywhere.This PR promotes the conditional
printto an unconditionallogger.warning(...)plus the originalprintpreserved for interactive sessions, applied symmetrically to bothenabled_toolsetsanddisabled_toolsets.get_tool_definitionsmemoizes on(enabled, disabled, registry._generation, config-mtime), so a stable misconfig logs exactly one WARNING at startup — not one per turn. When MCP discovery later registers the alias,registry._generationbumps, the cache invalidates, and the next compute resolves cleanly. This is option (c) only from the reporter's three suggestions — no change to resolution rules.Related Issue
Fixes #23997 (visibility piece)
Type of Change
Changes Made
model_tools.py— in_compute_tool_definitions, theelsebranch now emitslogger.warning("Unknown toolset '%s' in {enabled,disabled}_toolsets — dropping. Check spelling againsthermes tools list…", toolset_name)unconditionally, then falls through to the existingprintfor interactive sessions. Applied symmetrically to enabled and disabled.tests/test_model_tools.py—TestUnknownToolsetWarnsInQuietMode(3 cases: unknown enabled, unknown disabled, known toolset doesn't warn) usingcaplogagainst themodel_toolslogger.How to Test
uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest tests/test_model_tools.py tests/test_toolsets.py tests/test_get_tool_definitions_cache_isolation.py tests/hermes_cli/test_tools_disable_enable.py tests/tools/test_delegate_composite_toolsets.py -vmodel_tools.pyonly — 2 of the 3 new tests fail withassert []. Restore — all 3 pass.Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
docs/, docstrings) — N/Acli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/ARelated / Positioning
validate_toolset()to accept<server>:*) and option (b) (havehermes tools enable <server>:* --platform cronwrite the resolved alias) are deliberately out of scope; this PR is the visibility piece.mcp-<name>alias, but didn't add observability when that alias misses.