fix(telemetry): record tool usage once centrally, sanitize raw tool names - #1073
fix(telemetry): record tool usage once centrally, sanitize raw tool names#1073edelauna wants to merge 1 commit into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Related GitHub Issue
Refs: #830 (tool-usage telemetry sanitization and single-point attribution half — split from #835; independent of #1069, #1070, #1071)
Description
presentAssistantMessagealready recorded most tool-usage attempts at one central point, but two gaps remained:EditTool,EditFileTool,SearchReplaceTool,GenerateImageTool,ApplyPatchTool) each calledtask.recordToolUsage(...)locally on success, on top of the central call inpresentAssistantMessage. Every successful call through these tools counted as two attempts intask.toolUsage.recordToolUsage/recordToolErrorcall sites keyed offblock.namedirectly: the missing-tool_use.idpath, the malformed-nativeArgspath, and the final "unknown tool" branch. A malicious or malformed tool name from the model could become an arbitrarytoolsUsedproperty key.This PR:
recordToolUsagecalls. Grepped everyrecordToolUsage/recordToolErrorcall site first to confirm the central point inpresentAssistantMessagecovers each removed call before deleting it.recordToolErrorcalls in these handlers are untouched — they're legitimate failure counts with no equivalent central coverage.toTelemetryToolName()inpresentAssistantMessage.ts, mapping any tool name to a safe analytics key before it can reachrecordToolUsage/recordToolErroror PostHog:custom_toolmcp_-prefixed dynamic tool names (valid or malformed) →use_mcp_toolinvalid_tool_callidand unknown-tool-name paths.recordToolUsage/captureToolUsagecall to fire only aftervalidateToolUsesucceeds, so a tool that fails validation records a failure (via the mapper), never a spurious success.invalid_tool_callas a new static member of theToolNameunion inpackages/types/src/tool.ts(with a matchingTOOL_DISPLAY_NAMESentry insrc/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:
Task.toolUsagesummarization intoTask Completed) — in feat(telemetry): aggregate task completion telemetry with delta insta… #1071, not assumed present here.Test Procedure
toTelemetryToolName.spec.ts: static tool keeps its name, custom tool →custom_tool, valid and malformedmcp_names →use_mcp_tool, arbitrary unknown name →invalid_tool_call, and an explicit assertion that the raw name is never echoed back.presentAssistantMessage-tool-usage-attribution.spec.ts: a normal static tool records exactly one attempt; valid/malformedmcp_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 recordsinvalid_tool_calland never the raw name.presentAssistantMessage-unknown-tool.spec.ts: both the missing-id and unknown-tool-name paths now assertrecordToolErroris called withinvalid_tool_call, not the raw model-supplied name.editTool.spec.ts,editFileTool.spec.ts,searchReplaceTool.spec.ts: assert the handler itself no longer callsrecordToolUsageon success, proving removal of the duplicate didn't lose the attempt (it's covered centrally instead).pnpm check-typesclean repo-wide.pnpm lintclean onsrcand@roo-code/types,eslint-suppressions.jsononly decreases (7→3 forpresentAssistantMessage.ts, from removingany-casts on the deleted raw-name paths) — no suppression count increased and no new suppression entries added.core/assistant-message+core/toolsinsrc(591 tests) and@roo-code/types(268 tests), all passing.Pre-Submission Checklist
Documentation Updates