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 3 commits into
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds ChangesTool telemetry attribution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
edelauna
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/assistant-message/presentAssistantMessage.ts`:
- Around line 638-643: Route native mcp_tool_use handling through the same
validation boundary used by tool_use, removing its pre-validation
recordToolUsage and captureToolUsage calls near the native MCP branch. Ensure
validated native MCP calls use the centralized attribution symbols to record
exactly one attempt, while invalid or disallowed calls record none, and add a
regression test covering native MCP validation failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 78e54642-f04e-4195-9608-19f089b266fd
📒 Files selected for processing (15)
packages/types/src/tool.tssrc/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.tssrc/core/assistant-message/__tests__/toTelemetryToolName.spec.tssrc/core/assistant-message/presentAssistantMessage.tssrc/core/tools/ApplyPatchTool.tssrc/core/tools/EditFileTool.tssrc/core/tools/EditTool.tssrc/core/tools/GenerateImageTool.tssrc/core/tools/SearchReplaceTool.tssrc/core/tools/__tests__/editFileTool.spec.tssrc/core/tools/__tests__/editTool.spec.tssrc/core/tools/__tests__/searchReplaceTool.spec.tssrc/eslint-suppressions.jsonsrc/shared/tools.ts
💤 Files with no reviewable changes (5)
- src/core/tools/SearchReplaceTool.ts
- src/core/tools/ApplyPatchTool.ts
- src/core/tools/EditTool.ts
- src/core/tools/EditFileTool.ts
- src/core/tools/GenerateImageTool.ts
github-actions
Bot
added
the
awaiting-review
a2d3157 to
13a9e95
Compare
github-actions
Bot
removed
the
awaiting-review
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts (1)
236-267: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMocked
getMcpHubskips the real MCP tool-existence check.The mocked
getMcpHub(line 243-245) only implementsfindServerNameBySanitizedName.UseMcpToolTool.validateToolExistscallsmcpHub.getAllServers(), which is missing here. That call throws, and the surroundingtry/catchinvalidateToolExistsreturns{ isValid: true }on any error. The test passes because the error is silently swallowed, not because tool existence and enablement validation genuinely succeeded. The test name and comment claim the MCP tool's own validation passes.Add
getAllServersto the mock so the test exercises the real validation path.🧪 Proposed fix to exercise genuine tool-existence validation
getMcpHub: () => ({ findServerNameBySanitizedName: () => "my_server", + getAllServers: () => [ + { + name: "my_server", + tools: [{ name: "do_thing", enabledForPrompt: true }], + }, + ], }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts` around lines 236 - 267, Update the getMcpHub mock in the test around presentAssistantMessage to implement getAllServers with a server/tool definition that matches my_server and do_thing and is enabled, so UseMcpToolTool.validateToolExists follows genuine MCP validation rather than its error fallback. Preserve the existing findServerNameBySanitizedName behavior and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.ts`:
- Around line 236-267: Update the getMcpHub mock in the test around
presentAssistantMessage to implement getAllServers with a server/tool definition
that matches my_server and do_thing and is enabled, so
UseMcpToolTool.validateToolExists follows genuine MCP validation rather than its
error fallback. Preserve the existing findServerNameBySanitizedName behavior and
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cf965c2a-39a9-4e09-b30a-b7baa0de6737
📒 Files selected for processing (17)
packages/types/src/tool.tssrc/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.tssrc/core/assistant-message/__tests__/presentAssistantMessage-unknown-tool.spec.tssrc/core/assistant-message/__tests__/toTelemetryToolName.spec.tssrc/core/assistant-message/presentAssistantMessage.tssrc/core/tools/ApplyPatchTool.tssrc/core/tools/BaseTool.tssrc/core/tools/EditFileTool.tssrc/core/tools/EditTool.tssrc/core/tools/GenerateImageTool.tssrc/core/tools/SearchReplaceTool.tssrc/core/tools/UseMcpToolTool.tssrc/core/tools/__tests__/editFileTool.spec.tssrc/core/tools/__tests__/editTool.spec.tssrc/core/tools/__tests__/searchReplaceTool.spec.tssrc/eslint-suppressions.jsonsrc/shared/tools.ts
💤 Files with no reviewable changes (5)
- src/core/tools/GenerateImageTool.ts
- src/core/tools/EditTool.ts
- src/core/tools/EditFileTool.ts
- src/core/tools/SearchReplaceTool.ts
- src/core/tools/ApplyPatchTool.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/core/tools/tests/editFileTool.spec.ts
- src/core/assistant-message/tests/presentAssistantMessage-unknown-tool.spec.ts
- src/core/assistant-message/tests/toTelemetryToolName.spec.ts
- packages/types/src/tool.ts
- src/core/tools/tests/searchReplaceTool.spec.ts
- src/eslint-suppressions.json
- src/shared/tools.ts
- src/core/assistant-message/presentAssistantMessage.ts
- src/core/tools/tests/editTool.spec.ts
…ose coverage gaps
github-actions
Bot
added
the
awaiting-review
1 participant
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 installments #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 installments #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
Summary by CodeRabbit
New Features
Bug Fixes
Tests