Skip to content

fix(telemetry): record tool usage once centrally, sanitize raw tool names - #1073

Draft
edelauna wants to merge 1 commit into
mainfrom
fix/tool-usage-telemetry-attribution
Draft

fix(telemetry): record tool usage once centrally, sanitize raw tool names#1073
edelauna wants to merge 1 commit into
mainfrom
fix/tool-usage-telemetry-attribution

Conversation

@edelauna

@edelauna edelauna commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Related GitHub Issue

Refs: #830 (tool-usage telemetry sanitization and single-point attribution half — split from #835; independent of #1069, #1070, #1071)

Description

presentAssistantMessage already recorded most tool-usage attempts at one central point, but two gaps remained:

  1. Double-counted attempts. Five tool handlers (EditTool, EditFileTool, SearchReplaceTool, GenerateImageTool, ApplyPatchTool) each called task.recordToolUsage(...) locally on success, on top of the central call in presentAssistantMessage. Every successful call through these tools counted as two attempts in task.toolUsage.
  2. Raw model-controlled tool names reaching telemetry. Several recordToolUsage/recordToolError call sites keyed off block.name directly: the missing-tool_use.id path, the malformed-nativeArgs path, and the final "unknown tool" branch. A malicious or malformed tool name from the model could become an arbitrary toolsUsed property key.

This PR:

  • Removes the five duplicate local recordToolUsage calls. Grepped every recordToolUsage/recordToolError call site first to confirm the central point in presentAssistantMessage covers each removed call before deleting it. recordToolError calls in these handlers are untouched — they're legitimate failure counts with no equivalent central coverage.
  • Adds toTelemetryToolName() in presentAssistantMessage.ts, mapping any tool name to a safe analytics key before it can reach recordToolUsage/recordToolError or PostHog:
    • known static tools → their own name
    • registered custom tools → custom_tool
    • mcp_-prefixed dynamic tool names (valid or malformed) → use_mcp_tool
    • anything else (malformed calls, arbitrary/unknown names) → invalid_tool_call
  • Applies the mapper at every remaining call site that previously used the raw name, including the missing-id and unknown-tool-name paths.
  • Moves the central recordToolUsage/captureToolUsage call to fire only after validateToolUse succeeds, so a tool that fails validation records a failure (via the mapper), never a spurious success.
  • Adds invalid_tool_call as a new static member of the ToolName union in packages/types/src/tool.ts (with a matching TOOL_DISPLAY_NAMES entry in src/shared/tools.ts) — the smallest self-contained type needed here, kept independent of feat(telemetry): aggregate task completion telemetry with delta insta… #1071.

Out of scope

Kept narrow to tool-usage attribution, per the split from #835:

Test Procedure

  • New toTelemetryToolName.spec.ts: static tool keeps its name, custom tool → custom_tool, valid and malformed mcp_ names → use_mcp_tool, arbitrary unknown name → invalid_tool_call, and an explicit assertion that the raw name is never echoed back.
  • New presentAssistantMessage-tool-usage-attribution.spec.ts: a normal static tool records exactly one attempt; valid/malformed mcp_ names map correctly; a validation failure on a known tool records a failure under its own name (no success recorded); a validation failure on an arbitrary unknown name records invalid_tool_call and never the raw name.
  • Updated presentAssistantMessage-unknown-tool.spec.ts: both the missing-id and unknown-tool-name paths now assert recordToolError is called with invalid_tool_call, not the raw model-supplied name.
  • Updated editTool.spec.ts, editFileTool.spec.ts, searchReplaceTool.spec.ts: assert the handler itself no longer calls recordToolUsage on success, proving removal of the duplicate didn't lose the attempt (it's covered centrally instead).
  • pnpm check-types clean repo-wide. pnpm lint clean on src and @roo-code/types, eslint-suppressions.json only decreases (7→3 for presentAssistantMessage.ts, from removing any-casts on the deleted raw-name paths) — no suppression count increased and no new suppression entries added.
  • Narrowest suites: core/assistant-message + core/tools in src (591 tests) and @roo-code/types (268 tests), all passing.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Testing: New and/or updated tests have been added to cover my changes.
  • Visual Snapshot (UI changes only): N/A — no UI changes.
  • Documentation Impact: No documentation updates required (no user-facing behavior change).
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Documentation Updates

  • No documentation updates are required.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 64608054-b6c4-4425-8286-f32026301a0f

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • ✅ Review completed - (🔄 Check again to review again)

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@edelauna edelauna changed the title fix(telemetry): record tool usage once centrally, sanitize raw tool n… fix(telemetry): record tool usage once centrally, sanitize raw tool names Jul 31, 2026
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
.../core/assistant-message/presentAssistantMessage.ts 88.23% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant