feat: Telemetry Enhancements: Centralized Host Client Identification#175
feat: Telemetry Enhancements: Centralized Host Client Identification#175waroca wants to merge 2 commits into
Conversation
This commit addresses a critical bug found while using Desktop Commander in
Docker environments, specifically related to command execution. It also
enhances the overall stability of the test suite.
Key changes include:
- **Dynamic Shell Determination**: Implemented a hybrid approach for `start_process`
to intelligently select the appropriate shell. This prioritizes:
1. Shell explicitly provided in the tool call.
2. `defaultShell` configured in `config.json`.
3. Environment variables (`SHELL` or `COMSPEC`).
4. A safe hardcoded fallback (`/bin/sh` for Unix-like, `cmd.exe` for Windows).
This resolves the "spawn bash ENOENT" error in minimal Docker containers,
which was causing server crashes.
- **Test Suite Enhancements**:
- Updated `test-enhanced-repl.js` to dynamically detect `python` or `python3`
commands, making the test more robust across different environments.
- Disabled the obsolete `test-repl-tools.js` by renaming it to prevent
failures due to missing `repl-manager.js`.
- Corrected hardcoded absolute paths in `test_improved_search_truncation.js`
and `test_search_truncation.js` to relative paths, eliminating `ripgrep`
errors and improving test portability.
This commit centralizes the identification of the host client (e.g., Cursor, Claude, Docker) by integrating it directly into the `getConfig` tool. This ensures a single, consistent source for determining the client environment.
Key changes include:
- **`getConfig` Integration**: The `hostClient` is now determined and provided via the `getConfig` tool, streamlining its access across the application.
- **Dockerfile Update**: Added `ENV MCP_CLIENT_DOCKER=true` to the Dockerfile to enable explicit detection of Docker environments, which is then used by the `getConfig` tool.
- **Telemetry Refinement**: Updated `src/utils/capture.ts` to leverage the `hostClient` value obtained from `configManager.getValue('hostClient')`, removing the previous, redundant logic for identifying the client within the telemetry module itself. This ensures that all telemetry events accurately reflect the host client.
- **Code Cleanup**: Removed an unused `process` import from `src/utils/capture.ts`.
These changes improve the accuracy and consistency of client identification for telemetry purposes, providing better insights into the application's usage across different environments.
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DockerContainer
participant App
participant ConfigManager
participant Telemetry
User->>DockerContainer: Starts container (with MCP_CLIENT_DOCKER=true)
DockerContainer->>App: Launches application
App->>ConfigManager: getConfig()
ConfigManager->>App: Returns config with hostClient ('docker')
App->>Telemetry: captureBase() with host_client='docker'
sequenceDiagram
participant User
participant App
participant ConfigManager
User->>App: startProcess(command)
App->>ConfigManager: getConfig()
ConfigManager->>App: Returns config with defaultShell
App->>App: Selects shell based on config/env/platform
App->>App: Executes command with resolved shell
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/tools/config.ts (1)
17-17: Fix type annotation for hostClientIdentifier.The variable is declared as
string | undefinedbut is always assigned a string value, making theundefinedtype unnecessary.- let hostClientIdentifier: string | undefined = 'unknown'; + let hostClientIdentifier: string = 'unknown';src/tools/improved-process-tools.ts (1)
45-61: Enhanced shell determination logic looks good, but consider error handling.The centralized shell determination logic is a significant improvement that properly handles configuration hierarchy and platform differences. However, consider adding error handling for the async
configManager.getConfig()call.Consider wrapping the config retrieval in a try-catch block:
if (!shellUsed) { - const config = await configManager.getConfig(); - if (config.defaultShell) { - shellUsed = config.defaultShell; - } else { + try { + const config = await configManager.getConfig(); + if (config.defaultShell) { + shellUsed = config.defaultShell; + } else { + const isWindows = os.platform() === 'win32'; + if (isWindows && process.env.COMSPEC) { + shellUsed = process.env.COMSPEC; + } else if (!isWindows && process.env.SHELL) { + shellUsed = process.env.SHELL; + } else { + shellUsed = isWindows ? 'cmd.exe' : '/bin/sh'; + } + } + } catch (error) { const isWindows = os.platform() === 'win32'; if (isWindows && process.env.COMSPEC) { shellUsed = process.env.COMSPEC; } else if (!isWindows && process.env.SHELL) { shellUsed = process.env.SHELL; } else { shellUsed = isWindows ? 'cmd.exe' : '/bin/sh'; } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Dockerfile(1 hunks)package.json(1 hunks)src/config-manager.ts(1 hunks)src/tools/config.ts(2 hunks)src/tools/improved-process-tools.ts(2 hunks)src/utils/capture.ts(2 hunks)test/test-enhanced-repl.js(1 hunks)test/test_improved_search_truncation.js(1 hunks)test/test_output/node_repl_debug.txt(2 hunks)test/test_output/repl_test_output.txt(1 hunks)test/test_search_truncation.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/utils/capture.ts (2)
src/version.ts (1)
VERSION(1-1)src/config-manager.ts (1)
configManager(212-212)
src/tools/improved-process-tools.ts (2)
src/config-manager.ts (1)
configManager(212-212)src/terminal-manager.ts (1)
terminalManager(255-255)
🪛 LanguageTool
test/test_output/node_repl_debug.txt
[grammar] ~3-~3: Use proper spacing conventions.
Context: ...32m[STDOUT] Welcome to Node.js v22.15.0. Type ".help" for more information.�[0m �...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~6-~6: Use proper spacing conventions.
Context: ...put buffer: Welcome to Node.js v22.15.0. Type ".help" for more information. > �[0...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~7-~7: Use proper spacing conventions.
Context: ...15.0. Type ".help" for more information. > �[0m �[34mSending simple command...�[0m ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~13-~13: Use proper spacing conventions.
Context: ...st command: Welcome to Node.js v22.15.0. Type ".help" for more information. > Hel...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~43-~43: Use proper spacing conventions.
Context: ...put buffer: Welcome to Node.js v22.15.0. Type ".help" for more information. > Hel...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
test/test_output/repl_test_output.txt
[grammar] ~2-~2: Use proper spacing conventions.
Context: ...on REPL output: Python 3.13.3 (main, Apr 8 2025, 13:54:08) [Clang 16.0.0 (clang-...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~3-~3: There might be a problem here.
Context: ...0.0 (clang-1600.0.26.6)] on darwin Type "help", "copyright", "credits" or "license" for more information. >>>...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~3-~3: Use proper spacing conventions.
Context: ...dits" or "license" for more information. >>> STARTING PYTHON TEST >>> REPL_TEST_VALUE...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~5-~5: Use proper spacing conventions.
Context: ...TING PYTHON TEST >>> REPL_TEST_VALUE: 44 >>> Node.js REPL output: Welcome to Node.js ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~9-~9: Use proper spacing conventions.
Context: ...EPL output: Welcome to Node.js v22.15.0. Type ".help" for more information. > STA...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~10-~10: Use proper spacing conventions.
Context: ...15.0. Type ".help" for more information. > STARTING NODE TEST undefined > NODE_REPL...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
🔇 Additional comments (15)
Dockerfile (1)
4-5: LGTM! Environment variable correctly positioned for Docker detection.The addition of
MCP_CLIENT_DOCKER=trueenvironment variable is well-placed and supports the PR's objective of centralizing host client identification for telemetry purposes.package.json (1)
35-35: LGTM! Useful utility script addition.The clean script provides a convenient way to remove build artifacts using the cross-platform
shxutility.test/test_improved_search_truncation.js (1)
10-10: LGTM! Test path made more portable.Changing from an absolute path to the current directory (
.) makes the test more portable and removes the hardcoded user-specific path.test/test_search_truncation.js (1)
10-10: LGTM! Consistent test path improvement.Good to see consistent application of relative paths across test files, improving portability and removing hardcoded absolute paths.
test/test_output/node_repl_debug.txt (2)
3-3: Test output reflects Node.js version downgrade from v23.8.0 to v22.15.0.The version change is consistent across all output references and aligns with the test environment updates.
Also applies to: 6-6, 13-13, 43-43
38-39: Improved output formatting for multi-line greetings.The greeting outputs are now properly separated into individual lines, improving readability.
test/test_output/repl_test_output.txt (2)
2-2: Test output reflects Python version upgrade from 3.11.0 to 3.13.3.The version change and corresponding test value adjustment (52 → 44) are consistent with the runtime environment update.
Also applies to: 5-5
9-9: Test output reflects Node.js version downgrade from v23.8.0 to v22.15.0.The version change and corresponding test value adjustment (81 → 54) are consistent with the runtime environment update.
Also applies to: 13-13
src/tools/config.ts (2)
18-24: Well-implemented host client detection logic.The environment variable checking follows a logical precedence order (Cursor → Claude → Docker → unknown) and aligns with the PR objectives for centralized host client identification.
36-36: Properly integrated hostClient into system info.The hostClient identifier is correctly added to the system information object, making it available for telemetry and other consumers.
test/test-enhanced-repl.js (2)
9-23: Well-implemented Python command detection.The function correctly prioritizes
python3overpythonand provides clear error messaging when neither is available. The use ofexecSyncwithstdio: 'ignore'is appropriate for this detection logic.
32-34: Good integration of dynamic Python command detection.The detected Python command is properly used in the
startProcesscall, and the explicit shell specification (/bin/bash) ensures consistent behavior across environments.Also applies to: 38-41
src/utils/capture.ts (1)
128-135: Good addition of type safety with BaseProperties interface.The interface properly defines the structure of base properties used in telemetry capture, improving code maintainability and type safety.
src/tools/improved-process-tools.ts (2)
8-9: LGTM: Import additions support enhanced shell determination.The added imports for
osandconfigManagerare necessary and correctly implemented to support the new shell determination logic.
66-66: LGTM: Proper usage of resolved shell variable.The usage of the resolved
shellUsedvariable correctly implements the enhanced shell determination logic.
| "takeown" // Take ownership of files | ||
| ], | ||
| defaultShell: os.platform() === 'win32' ? 'powershell.exe' : 'bash', | ||
| defaultShell: os.platform() === 'win32' ? 'powershell.exe' : '/bin/sh', |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify impact of shell change on existing users.
Changing the default shell from bash to /bin/sh for non-Windows platforms could be a breaking change for users who rely on bash-specific features. While /bin/sh is more universally available, it has fewer features than bash.
Please verify:
- That existing user workflows won't break with this change
- Consider documenting this change in release notes
- Ensure the shell selection logic in other parts of the codebase (mentioned in AI summary) properly handles this change
🏁 Script executed:
#!/bin/bash
# Search for any bash-specific usage that might be affected
echo "Searching for potential bash-specific features in the codebase..."
rg -i "bash|#!/bin/bash" --type js --type ts -A 3 -B 1Length of output: 7093
Document the change to /bin/sh as the new default shell
We’ve confirmed that the default in config-manager.ts is now /bin/sh on Unix, and test/test-default-shell.js has been updated to expect /bin/sh as 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 bash to /bin/sh.”
• Update user documentation:
– README (or any user-facing docs) section describing default shell.
– Code comments in src/server.ts that list “start_process('bash')”.
– Any mention in src/utils/system-info.ts of defaultShell = 'bash'.
🤖 Prompt for AI Agents
In src/config-manager.ts at line 125, the default shell was changed to '/bin/sh'
for Unix systems. To complete this update, add a changelog or release notes
entry stating "Default shell on macOS/Linux changed from `bash` to `/bin/sh`."
Update all user-facing documentation such as the README to reflect this new
default shell. Also, revise code comments in src/server.ts that mention
"start_process('bash')" and update any references to defaultShell = 'bash' in
src/utils/system-info.ts to align with the new default shell setting.
| const hostClient = await configManager.getValue('hostClient'); | ||
| if (hostClient) { | ||
| (baseProperties as any).host_client = hostClient; | ||
| } |
There was a problem hiding this comment.
🛠️ 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'
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/utils/capture.ts around lines 143 to 146, avoid using type casting to
assign the host_client property on baseProperties as it bypasses TypeScript's
type checking. Instead, update the type definition of baseProperties to include
host_client or initialize baseProperties with host_client as an optional
property. Then assign hostClient directly without casting, ensuring type safety
and cleaner code.
| let hostClientIdentifier: string | undefined = 'unknown'; | ||
| if (process.env.CURSOR_TRACE_ID) { | ||
| hostClientIdentifier = 'cursor'; | ||
| } else if (process.env.CLAUDE_MCP_TOKEN) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
i did npm run setup on this repository, also built conteiner from this repo.
in docker i saw "docker" as a client.
Claude desktop: Claude 0.11.6 (0aa9ce) 2025-06-29T21:59:05.000Z
There was a problem hiding this comment.
I think it will make more sense to put docker as a separate flag: is_docker or something like this, this is more about env, than client
There was a problem hiding this comment.
@waroca what do you think if we remove this identification, and keep only is it from docker or not?
There was a problem hiding this comment.
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?
|
As we merged other way to detect closing this one |
4 participants
Telemetry Enhancements: Centralized Host Client Identification
Description
This pull request centralizes the identification of the host client (e.g., Cursor, Claude, Docker) by integrating it directly into the
getConfigtool. This ensures a single, consistent source for determining the client environment across the application. The primary goal is to improve the accuracy and consistency of client identification for telemetry purposes, providing better insights into the application's usage across different environments.Changes Made
getConfigIntegration: ThehostClientis now determined and provided via thegetConfigtool, streamlining its access across the application. This removes redundant logic for identifying the client within other modules.ENV MCP_CLIENT_DOCKER=trueto the Dockerfile. This environment variable enables explicit detection of Docker environments, which is then utilized by thegetConfigtool to correctly identify Docker as the host client.src/utils/capture.ts): Updated the telemetry capture logic to leverage thehostClientvalue obtained fromconfigManager.getValue('hostClient'). This ensures that all telemetry events accurately reflect the host client, improving the granularity and reliability of collected data.How to Test
To test these changes, please follow these steps:
npm install && npm run buildhost_clientproperty is correctly populated with the expected value (e.g.,docker,cursor,claude, orunknown).Checklist
Summary by CodeRabbit