Skip to content

fix: sum tool usage from dual sources instead of taking max - #9990

Closed
nightq wants to merge 1 commit into
NousResearch:mainfrom
nightq:fix/issue-9814-insights-undercount
Closed

nightq wants to merge 1 commit into
NousResearch:mainfrom
nightq:fix/issue-9814-insights-undercount

Conversation

@nightq

@nightq nightq commented Apr 15, 2026

Copy link
Copy Markdown

Summary

The Insights tool was undercounting tool usage when tool_name and tool_calls sources coexisted across different sessions.

Root Cause

_get_tool_usage() in agent/insights.py used max() to merge the two tool-count sources, assuming they were duplicate views of the same calls. However, across mixed session sources (gateway / CLI / ACP), one session may record via tool_name while another records only via tool_calls, making them disjoint. When the same tool appeared once in each source, the count was max(1, 1) = 1 instead of the correct 2.

Fix

Changed the merge logic from max() to sum (+), since tool_name (from tool execution results) and tool_calls (from assistant-side call records) can capture disjoint sets of calls across different session sources.

Test Plan

  • Generate insights with mixed gateway + CLI sessions
  • Verify tool counts reflect total invocations across all sources
  • Single-source sessions (tool_name only or tool_calls only) unchanged

Closes #9814

Fixes NousResearch#9814

Root cause: _get_tool_usage() used max() to merge tool_name and tool_calls
sources, assuming they recorded the same calls. But across mixed session
sources (gateway/CLI/ACP), they capture disjoint sets of calls.
Fix: Sum counts from both sources to properly count all tool invocations.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint labels Apr 26, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying the real mixed-session undercount: current main still applies a global per-tool max() in agent/insights.py:281.

Problems

  • The proposed addition at agent/insights.py:280 double-counts normal completed calls. agent/tool_executor.py:938 appends a tool-result message, and run_agent.py:1882-1895 persists both assistant tool_calls and tool-result tool_name. The existing fixture records both representations and expects two terminal calls at tests/agent/test_insights.py:348-350; this change would report four.
  • No regression test accompanies the behavior change. The linked related PR #9896 uses (session_id, tool_name) aggregation before the per-source max, which preserves paired calls while adding disjoint sessions.

Suggested changes

  • Merge per (session_id, tool_name), then total by tool name.
  • Add paired-source and disjoint-session regression cases in tests/agent/test_insights.py.

Automated hermes-sweeper review.

Comment thread agent/insights.py
# disjoint tool calls across different session sources.
all_tools = set(tool_counts) | set(tool_calls_counts)
merged = Counter()
for tool in all_tools:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sums globally aggregated counters, so it doubles ordinary completed calls that have both assistant tool_calls and a tool-result tool_name row. Merge by (session_id, tool_name) first, then aggregate by tool name.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/usage-cost Token accounting, usage reporting, billing, cost tracking labels Jul 12, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Salvaged onto current main as #109132 with your commit authorship preserved where the diff still applied (the file moved/was restructured since April, so some of it is a hand-port with credit in the commit message). Once #109132 merges this PR will be closed with a link to the landed SHA. Thanks for the fix.

@teknium1

Copy link
Copy Markdown
Collaborator

Landed via #109132 (merge b5b697e) with your commit cherry-picked so authorship is preserved — thank you @nightq. Closing this PR as superseded by the merged salvage; the fix is on main now.

@teknium1 teknium1 closed this Sep 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/usage-cost Token accounting, usage reporting, billing, cost tracking comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Insights undercount tool usage when tool_name and tool_calls sources coexist

3 participants