-
-
Notifications
You must be signed in to change notification settings - Fork 735
feat: Telemetry Enhancements: Centralized Host Client Identification #175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,17 @@ export async function getConfig() { | |
|
|
||
| // Add system information to the config response | ||
| const systemInfo = getSystemInfo(); | ||
|
|
||
| // Determine host client | ||
| let hostClientIdentifier: string | undefined = 'unknown'; | ||
| if (process.env.CURSOR_TRACE_ID) { | ||
| hostClientIdentifier = 'cursor'; | ||
| } else if (process.env.CLAUDE_MCP_TOKEN) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you explain how its working? i tried it on macos, and CLAUDE_MCP_TOKEN is empty and i have unknown client most of the time. Where did you found this variable?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am also using macos and I didn't do anything special. I found they were adding that Env variable to my tool call. What version of Claude Desktop are you using? How do you have DC installed? Maybe that influences things?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i did npm run setup on this repository, also built conteiner from this repo.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it will make more sense to put docker as a separate flag:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @waroca what do you think if we remove this identification, and keep only is it from docker or not?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah that that makes sense so to not confuse the underlying client vs the environment its running in. Since we also now have a better way to detect clients, how about we don't merge this at all and I create a separate one with the Dockerfile update and the new flag?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good |
||
| hostClientIdentifier = 'claude'; | ||
| } else if (process.env.MCP_CLIENT_DOCKER === 'true') { | ||
| hostClientIdentifier = 'docker'; | ||
| } | ||
|
|
||
| const configWithSystemInfo = { | ||
| ...config, | ||
| systemInfo: { | ||
|
|
@@ -22,6 +33,7 @@ export async function getConfig() { | |
| isWindows: systemInfo.isWindows, | ||
| isMacOS: systemInfo.isMacOS, | ||
| isLinux: systemInfo.isLinux, | ||
| hostClient: hostClientIdentifier, | ||
| examplePaths: systemInfo.examplePaths | ||
| } | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import {platform} from 'os'; | ||
|
|
||
| import {randomUUID} from 'crypto'; | ||
| import * as https from 'https'; | ||
| import {configManager} from '../config-manager.js'; | ||
|
|
@@ -124,13 +125,26 @@ export const captureBase = async (captureURL: string, event: string, properties? | |
| } | ||
|
|
||
| // Prepare standard properties | ||
| const baseProperties = { | ||
| interface BaseProperties { | ||
| timestamp: string; | ||
| platform: NodeJS.Platform; | ||
| app_version: string; | ||
| engagement_time_msec: string; | ||
| host_client?: string; | ||
| } | ||
|
|
||
| const baseProperties: BaseProperties = { | ||
| timestamp: new Date().toISOString(), | ||
| platform: platform(), | ||
| app_version: VERSION, | ||
| engagement_time_msec: "100" | ||
| }; | ||
|
|
||
| const hostClient = await configManager.getValue('hostClient'); | ||
| if (hostClient) { | ||
| (baseProperties as any).host_client = hostClient; | ||
| } | ||
|
Comment on lines
+143
to
+146
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Improve host_client assignment to avoid type casting. The current implementation uses type casting to assign the host_client property, which bypasses TypeScript's type checking. Consider this cleaner approach: - const hostClient = await configManager.getValue('hostClient');
- if (hostClient) {
- (baseProperties as any).host_client = hostClient;
- }
+ const hostClient = await configManager.getValue('hostClient');
+ if (hostClient) {
+ baseProperties.host_client = hostClient;
+ }Alternatively, if you want to always include the property, modify the interface and initialization: interface BaseProperties {
timestamp: string;
platform: NodeJS.Platform;
app_version: string;
engagement_time_msec: string;
- host_client?: string;
+ host_client: string;
}
const baseProperties: BaseProperties = {
timestamp: new Date().toISOString(),
platform: platform(),
app_version: VERSION,
- engagement_time_msec: "100"
+ engagement_time_msec: "100",
+ host_client: await configManager.getValue('hostClient') || 'unknown'
};
🤖 Prompt for AI Agents |
||
|
|
||
| // Combine with sanitized properties | ||
| const eventProperties = { | ||
| ...baseProperties, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,15 @@ | ||
| Python REPL output: | ||
| Python 3.11.0 (main, Jan 19 2024, 19:53:39) [Clang 15.0.0 (clang-1500.0.40.1)] on darwin | ||
| Python 3.13.3 (main, Apr 8 2025, 13:54:08) [Clang 16.0.0 (clang-1600.0.26.6)] on darwin | ||
| Type "help", "copyright", "credits" or "license" for more information. | ||
| >>> STARTING PYTHON TEST | ||
| >>> REPL_TEST_VALUE: 52 | ||
| >>> REPL_TEST_VALUE: 44 | ||
| >>> | ||
|
|
||
| Node.js REPL output: | ||
| Welcome to Node.js v23.8.0. | ||
| Welcome to Node.js v22.15.0. | ||
| Type ".help" for more information. | ||
| > STARTING NODE TEST | ||
| undefined | ||
| > NODE_REPL_TEST_VALUE: 81 | ||
| > NODE_REPL_TEST_VALUE: 54 | ||
| undefined | ||
| > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify impact of shell change on existing users.
Changing the default shell from
bashto/bin/shfor non-Windows platforms could be a breaking change for users who rely on bash-specific features. While/bin/shis more universally available, it has fewer features than bash.Please verify:
🏁 Script executed:
Length of output: 7093
Document the change to
/bin/shas the new default shellWe’ve confirmed that the default in
config-manager.tsis now/bin/shon Unix, andtest/test-default-shell.jshas been updated to expect/bin/shas the first‐choice shell. A ripgrep search for “bash”/“#!/bin/bash” across JS/TS files shows no remaining runtime dependencies on Bash-only features.Please add the following before merging:
• Changelog/release notes entry: “Default shell on macOS/Linux changed from
bashto/bin/sh.”• Update user documentation:
– README (or any user-facing docs) section describing default shell.
– Code comments in
src/server.tsthat list “start_process('bash')”.– Any mention in
src/utils/system-info.tsofdefaultShell = 'bash'.🤖 Prompt for AI Agents