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 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (20)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughThe PR adds ChangesTool telemetry attribution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant presentAssistantMessage
participant validateToolUse
participant UseMcpToolTool
participant TelemetryService
presentAssistantMessage->>validateToolUse: Validate tool call
presentAssistantMessage->>UseMcpToolTool: Provide onValidated callback
UseMcpToolTool->>TelemetryService: Invoke callback after validation
presentAssistantMessage->>TelemetryService: Record normalized usage or error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/types/src/tool.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/assistant-message/__tests__/presentAssistantMessage-custom-tool.spec.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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
github-actions
Bot
added
awaiting-review
taltas
left a comment
taltas
left a comment
There was a problem hiding this comment.
One small test improvement; the implementation looks good.
| getMcpHub: () => ({ | ||
| findServerNameBySanitizedName: () => "my_server", | ||
| }), |
There was a problem hiding this comment.
This test currently passes through the validation error fallback because getAllServers is missing. Add the server and tool so it tests the real success path.
| getMcpHub: () => ({ | |
| findServerNameBySanitizedName: () => "my_server", | |
| }), | |
| getMcpHub: () => ({ | |
| findServerNameBySanitizedName: () => "my_server", | |
| getAllServers: () => [ | |
| { | |
| name: "my_server", | |
| tools: [{ name: "do_thing", enabledForPrompt: true }], | |
| }, | |
| ], | |
| }), |
There was a problem hiding this comment.
Fixed in 18172e6. Added getAllServers to the mock hub in the success-path test so validateToolExists runs the real path and confirms the tool exists, instead of falling through to the error fallback. The disallowed-server test below keeps getAllServers absent intentionally — that branch exits before the tool-existence check runs.
github-actions
Bot
added
awaiting-review
87fb4bd to
4d62660
Compare
github-actions
Bot
removed
the
awaiting-review
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/tools/__tests__/applyPatchTool.execute.spec.ts (1)
57-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the double assertions with precise controller test doubles.
Lines 57-62 use
as unknown asto force incomplete mocks intoTaskcontroller types. This bypasses type checking for the controller methods used byApplyPatchTool.Define a purpose-built task double with narrow controller interfaces. If one boundary cast remains necessary, use a single cast and explain it nearby.
As per coding guidelines, “Avoid
as any; use typed APIs, bracket notation for private members when necessary, or precise test doubles and type guards. Use double assertions only as a last resort and explain them with a nearby comment.”🤖 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/tools/__tests__/applyPatchTool.execute.spec.ts` around lines 57 - 62, Replace the double assertions on rooIgnoreController and rooProtectedController in the ApplyPatchTool test setup with typed, purpose-built controller doubles exposing the methods used by ApplyPatchTool. Use a single narrowly scoped cast only if required to satisfy Task, and add a nearby explanation for that boundary cast; do not use broad unknown-based assertions.Source: Coding guidelines
🤖 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/tools/__tests__/applyPatchTool.execute.spec.ts`:
- Around line 57-62: Replace the double assertions on rooIgnoreController and
rooProtectedController in the ApplyPatchTool test setup with typed,
purpose-built controller doubles exposing the methods used by ApplyPatchTool.
Use a single narrowly scoped cast only if required to satisfy Task, and add a
nearby explanation for that boundary cast; do not use broad unknown-based
assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d207c9a9-7091-43c2-b312-ee0d16d01e91
📒 Files selected for processing (6)
src/core/assistant-message/__tests__/presentAssistantMessage-tool-usage-attribution.spec.tssrc/core/assistant-message/__tests__/toTelemetryToolName.spec.tssrc/core/tools/BaseTool.tssrc/core/tools/UseMcpToolTool.tssrc/core/tools/__tests__/applyPatchTool.execute.spec.tssrc/core/tools/__tests__/generateImageTool.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/assistant-message/tests/toTelemetryToolName.spec.ts
- src/core/assistant-message/tests/presentAssistantMessage-tool-usage-attribution.spec.ts
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
4d62660 to
18172e6
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
github-actions
Bot
added
the
awaiting-review
2 participants
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