fix(telemetry): propagate remote flag to per-tool events#486
Conversation
The remote flag from _meta.remote was only stamped onto the central server_call_tool event, not the detailed per-tool events emitted inside tool handlers (server_start_process, server_read_file, server_write_file, server_edit_block). As a result, those events were always recorded as remote=not_set in BigQuery even when the call originated from a remote device — making it impossible to see file types or command types for remote tool usage. Root cause: clientInfo from _meta was persisted to the module-level currentClient (read by buildEventProperties for every event), but the remote flag was only written to the local telemetryData object for the single server_call_tool capture. Fix: mirror the currentClient pattern with a module-level currentCallIsRemote flag, set once per call by the CallTool handler (reset to false on every call so it never leaks from a remote call to a subsequent local one), and stamped onto all events in both capture paths (legacy GA4 captureBase and buildEventProperties for the proxy). An explicit remote value passed by the caller (e.g. captureRemote) still takes precedence.
📝 WalkthroughWalkthroughThe PR adds remote call tracking to the server layer and propagates this information to telemetry capture. A new ChangesRemote Call Attribution in Telemetry
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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/server.ts`:
- Around line 1199-1203: The module-level flag currentCallIsRemote (and setter
setCurrentCallIsRemote) causes race conditions; instead carry isRemoteCall in
request-local context: stop calling setCurrentCallIsRemote here, attach
isRemoteCall to the request/metadata object or store it in an AsyncLocalStorage
context created per capture()/CallTool invocation, then update consumers (the
GA4 path and proxy path that currently read currentCallIsRemote) to read the
request-local value (e.g., metadata.isRemoteCall or
asyncLocal.getStore().isRemoteCall) so each tool invocation has its own isolated
remote flag.
🪄 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
Run ID: 1b06352f-8a6d-42f8-b95e-5b7e000ad0db
📒 Files selected for processing (2)
src/server.tssrc/utils/capture.ts
| // Reset remote attribution for every call so a prior remote call never | ||
| // leaks its flag onto a subsequent local call. Set to true only when | ||
| // this call carries the remote marker in _meta. | ||
| const isRemoteCall = !!(metadata && typeof metadata === 'object' && metadata.remote); | ||
| setCurrentCallIsRemote(isRemoteCall); |
There was a problem hiding this comment.
Don't use a shared module flag for request-scoped telemetry state.
currentCallIsRemote can be overwritten by another CallTool while this handler is still awaiting config/tool work, so nested capture() calls will sometimes stamp the wrong remote value. Because the GA4 and proxy paths read this flag later and independently, the two events can even disagree for the same tool call. Please carry isRemoteCall in request-local context instead of module state.
🤖 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/server.ts` around lines 1199 - 1203, The module-level flag
currentCallIsRemote (and setter setCurrentCallIsRemote) causes race conditions;
instead carry isRemoteCall in request-local context: stop calling
setCurrentCallIsRemote here, attach isRemoteCall to the request/metadata object
or store it in an AsyncLocalStorage context created per capture()/CallTool
invocation, then update consumers (the GA4 path and proxy path that currently
read currentCallIsRemote) to read the request-local value (e.g.,
metadata.isRemoteCall or asyncLocal.getStore().isRemoteCall) so each tool
invocation has its own isolated remote flag.
Problem
Remote tool calls are a telemetry blind spot for content detail. We can see that a remote user called
start_process(viaserver_call_toolwithremote=true), but not what file types they read/wrote or what commands they ran.Investigating BigQuery, every detailed per-tool event —
server_start_process(carriescommand/commands),server_read_file/server_write_file/server_edit_block(carryfileExtension/fileSize) — is recorded asremote=not_set, even for calls that clearly came from a remote device. Confirmed: 0 of these events across late May carryremote=true.Root cause
In the
CallToolhandler (server.ts),_meta.clientInfoand_meta.remotewere handled asymmetrically:clientInfo→ persisted to the module-levelcurrentClientviaupdateCurrentClient(), whichbuildEventProperties(incapture.ts) reads for every event. So client attribution "leaks" correctly to all downstream events.remote→ written only to the localtelemetryDataobject passed to the singlecapture_call_tool('server_call_tool', ...)call. It was never persisted anywhere the tool handlers' owncapture()calls could see it.So when
improved-process-tools.ts,filesystem.ts,edit.tsetc. fire their detailedcapture('server_start_process', { command, commands })events, there's no remote context available →remote=not_set.Fix
Mirror the existing
currentClientpattern:currentCallIsRemoteflag +setCurrentCallIsRemote()setter inserver.ts, exported alongsidecurrentClient.CallToolhandler sets it on every call —truewhen_meta.remoteis present,falseotherwise. The explicit reset-to-false guard prevents a remote call from leaking its flag onto a subsequent local call.buildEventProperties(proxy/BigQuery path) andcaptureBase(legacy GA4 path) both stampremote: "true"onto every event whencurrentCallIsRemoteis set. Placed before the caller'ssanitizedPropertiesspread so an explicitremote(e.g. fromcaptureRemote) still wins.After this lands,
server_start_process/server_read_file/ etc. from remote clients will carryremote=true, giving us file-type and command visibility for remote usage at parity with local.Notes / caveat for reviewers
currentClientalready uses (it's reassigned inupdateCurrentClientand read live bycapture.ts). The circular importcapture.ts↔server.tsalready exists forcurrentClient; this adds nothing new.currentClient, this is module-level mutable state, so it assumes one logical call in flight at a time. If local and remote calls interleave concurrently, attribution could be momentarily wrong. The fully-correct solution is request-scoped context (AsyncLocalStorage), but that's a larger refactor; this change reaches parity with the existing accepted behavior and fixes the vast majority of real cases (users typically run one client at a time).Testing
npx tsc --noEmit— clean (exit 0)npm run build— succeeds; verifiedcurrentCallIsRemotepresent in compileddist/server.jsanddist/utils/capture.jsSummary by CodeRabbit