diff --git a/src/config-manager.ts b/src/config-manager.ts index 19b7bd8f..1c88700c 100644 --- a/src/config-manager.ts +++ b/src/config-manager.ts @@ -23,6 +23,27 @@ export interface ClientInfo { version: string; } +export function normalizeTelemetryEnabledValue(value: unknown): unknown { + if (typeof value !== 'string') { + return value; + } + + const normalized = value.trim().toLowerCase(); + if (normalized === 'true') { + return true; + } + + if (normalized === 'false') { + return false; + } + + return value; +} + +export function isTelemetryDisabledValue(value: unknown): boolean { + return normalizeTelemetryEnabledValue(value) === false; +} + /** * Singleton config manager for the server */ @@ -185,14 +206,19 @@ class ConfigManager { */ async setValue(key: string, value: any): Promise { await this.init(); + + if (key === 'telemetryEnabled') { + value = normalizeTelemetryEnabledValue(value); + } // Special handling for telemetry opt-out - if (key === 'telemetryEnabled' && value === false) { + if (key === 'telemetryEnabled' && isTelemetryDisabledValue(value)) { // Get the current value before changing it - const currentValue = this.config[key]; + const currentValue: unknown = this.config[key]; + const telemetryAlreadyDisabled = isTelemetryDisabledValue(currentValue); // Only capture the opt-out event if telemetry was previously enabled - if (currentValue !== false) { + if (!telemetryAlreadyDisabled) { // Import the capture function dynamically to avoid circular dependencies const { capture } = await import('./utils/capture.js'); @@ -251,4 +277,4 @@ class ConfigManager { } // Export singleton instance -export const configManager = new ConfigManager(); \ No newline at end of file +export const configManager = new ConfigManager(); diff --git a/src/server.ts b/src/server.ts index f962bef7..975574b2 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1189,6 +1189,7 @@ server.setRequestHandler(CallToolRequestSchema, async (request: CallToolRequest) if (name === 'set_config_value' && args && typeof args === 'object' && 'key' in args) { telemetryData.set_config_value_key_name = (args as any).key; + telemetryData.call_origin = (args as any).origin === 'ui' ? 'ui' : 'llm'; } if (name === 'get_prompts' && args && typeof args === 'object') { const promptArgs = args as any; diff --git a/src/tools/config.ts b/src/tools/config.ts index f4882e13..6d681da5 100644 --- a/src/tools/config.ts +++ b/src/tools/config.ts @@ -222,6 +222,28 @@ export async function setConfigValue(args: unknown) { } } + // Harden boolean fields against stringly-typed inputs like "false". + if (fieldDefinition.valueType === 'boolean') { + if (typeof valueToStore === 'string') { + const normalized = valueToStore.trim().toLowerCase(); + if (normalized === 'true') { + valueToStore = true; + } else if (normalized === 'false') { + valueToStore = false; + } + } + + if (typeof valueToStore !== 'boolean') { + return { + content: [{ + type: "text", + text: `Value for ${parsed.data.key} must be boolean true/false.` + }], + isError: true + }; + } + } + await configManager.setValue(parsed.data.key, valueToStore); // Get the updated configuration to show the user const updatedConfig = await configManager.getConfig(); diff --git a/src/tools/schemas.ts b/src/tools/schemas.ts index a2cdb39e..774fb99e 100644 --- a/src/tools/schemas.ts +++ b/src/tools/schemas.ts @@ -12,6 +12,7 @@ export const SetConfigValueArgsSchema = z.object({ z.array(z.string()), z.null(), ]), + origin: z.enum(['ui', 'llm']).optional(), }); // Empty schemas diff --git a/src/ui/config-editor/src/app.ts b/src/ui/config-editor/src/app.ts index f72d097b..8aacd518 100644 --- a/src/ui/config-editor/src/app.ts +++ b/src/ui/config-editor/src/app.ts @@ -5,6 +5,7 @@ import { renderCompactRow } from '../../shared/compact-row.js'; import { escapeHtml } from '../../shared/escape-html.js'; import { createWidgetStateStorage } from '../../shared/widget-state.js'; import { connectWithSharedHostContext, isObjectRecord, type UiChromeState } from '../../shared/host-context.js'; +import { createUiEventTracker, type UiEventParams } from '../../shared/ui-event-tracker.js'; import { createArrayModalController, renderArrayModalMarkup } from './array-modal.js'; import { CONFIG_FIELD_DEFINITIONS, isConfigFieldKey } from '../../../config-field-definitions.js'; @@ -46,12 +47,57 @@ export interface ApplyConfigResult { interface RenderHooks { onConfigChanged?: (change: { key: string; value: unknown }) => void; onTooltip?: (tooltip: TooltipMessage) => void; + onExpandedChanged?: (expanded: boolean) => void; } type ToolCall = (name: string, args?: Record) => Promise; +type TrackConfigUiEvent = (event: string, params?: Record) => void; let shellController: ToolShellController | undefined; +const CONFIG_EDITOR_COMPONENT = 'config_editor'; +const GET_CONFIG_TOOL_NAME = 'get_config'; +const MAX_TELEMETRY_MESSAGE_LENGTH = 180; + +function sanitizeTelemetryErrorMessage(message: string): string { + // Keep error signal useful while removing path-like data and bounding payload size. + const collapsed = message.replace(/\s+/g, ' ').trim(); + const withoutPaths = collapsed + .replace(/(?:\/|\\)[\w\d_.\-/\\]+/g, '[PATH]') + .replace(/[A-Za-z]:\\[\w\d_.\-/\\]+/g, '[PATH]'); + + if (withoutPaths.length === 0) { + return 'Unknown error'; + } + + if (withoutPaths.length <= MAX_TELEMETRY_MESSAGE_LENGTH) { + return withoutPaths; + } + + return `${withoutPaths.slice(0, MAX_TELEMETRY_MESSAGE_LENGTH - 3)}...`; +} + +function buildConfigUpdateTelemetryParams(args: { + configKey: string; + valueType: string; + errorMessage?: string; + errorStage?: 'set_config_value' | 'transport'; +}): UiEventParams { + const base: UiEventParams = { + config_key: args.configKey, + value_type: args.valueType, + }; + + if (args.errorStage) { + base.error_stage = args.errorStage; + } + if (args.errorMessage) { + base.error_message = sanitizeTelemetryErrorMessage(args.errorMessage); + } + + return base; +} + function isConfigEditorPayload(value: unknown): value is ConfigEditorPayload { return isObjectRecord(value) && Array.isArray((value as Record).entries); } @@ -310,7 +356,7 @@ function getShellOptions(payload: ConfigEditorPayload | null, currentShell: stri return [...options]; } -export function createConfigEditorController(callTool: ToolCall) { +export function createConfigEditorController(callTool: ToolCall, trackConfigUiEvent?: TrackConfigUiEvent) { const state: ConfigEditorState = { payload: null, selectedKey: null, @@ -392,23 +438,45 @@ export function createConfigEditorController(callTool: ToolCall) { const setResult = await callTool('set_config_value', { key: selected.key, value: parsed.value, + origin: 'ui', }); + if (isToolErrorResult(setResult)) { + const errorMessage = extractToolText(setResult) ?? `Failed to update ${selected.key}.`; + trackConfigUiEvent?.('config_update_failed', { + tool_name: 'set_config_value', + ...buildConfigUpdateTelemetryParams({ + configKey: selected.key, + valueType: selected.valueType, + errorMessage, + errorStage: 'set_config_value', + }), + }); + return { ok: false, tooltip: { - message: extractToolText(setResult) ?? `Failed to update ${selected.key}.`, + message: errorMessage, tone: 'error', }, }; } + trackConfigUiEvent?.('config_update_success', { + tool_name: 'set_config_value', + ...buildConfigUpdateTelemetryParams({ + configKey: selected.key, + valueType: selected.valueType, + }), + }); + const refreshed = await callTool('get_config', {}); if (isToolErrorResult(refreshed)) { + const errorMessage = extractToolText(refreshed) ?? 'Value was updated but config refresh failed.'; return { ok: false, tooltip: { - message: extractToolText(refreshed) ?? 'Value was updated but config refresh failed.', + message: errorMessage, tone: 'error', }, }; @@ -425,12 +493,25 @@ export function createConfigEditorController(callTool: ToolCall) { } } - return { ok: true }; + return { + ok: true, + }; } catch (error) { + const errorMessage = `Failed to apply value: ${error instanceof Error ? error.message : String(error)}`; + trackConfigUiEvent?.('config_update_failed', { + tool_name: 'set_config_value', + ...buildConfigUpdateTelemetryParams({ + configKey: selected.key, + valueType: selected.valueType, + errorMessage, + errorStage: 'transport', + }), + }); + return { ok: false, tooltip: { - message: `Failed to apply value: ${error instanceof Error ? error.message : String(error)}`, + message: errorMessage, tone: 'error', }, }; @@ -712,6 +793,7 @@ function render(container: HTMLElement, controller: ReturnType { chrome.expanded = expanded; + hooks.onExpandedChanged?.(expanded); }, }); } @@ -727,7 +809,17 @@ export function bootstrapConfigEditorApp(): void { } const bridge = createToolBridge(); - const controller = createConfigEditorController((name, args) => bridge.callTool(name, args)); + const trackConfigUiEvent = createUiEventTracker( + (name, args) => bridge.callTool(name, args), + { + component: CONFIG_EDITOR_COMPONENT, + baseParams: { origin: 'ui' }, + } + ); + const controller = createConfigEditorController( + (name, args) => bridge.callTool(name, args), + trackConfigUiEvent + ); const widgetState = createWidgetStateStorage(isConfigEditorPayload); const chrome: UiChromeState = { hideSummaryRow: false, @@ -735,6 +827,8 @@ export function bootstrapConfigEditorApp(): void { expanded: true, }; + let configEditorShownEventSent = false; + let quietContextSupported = true; let tooltipHideTimer: number | null = null; @@ -765,28 +859,16 @@ export function bootstrapConfigEditorApp(): void { }; const syncModelContext = (reason: string, change?: { key: string; value: unknown }): void => { - const payload = controller.state.payload; - if (!payload || !quietContextSupported) { + if (!quietContextSupported || !change) { return; } - const values = payload.entries - .map((entry) => `${entry.key}=${JSON.stringify(entry.value)}`) - .join(', '); - const changeText = change - ? `Updated ${change.key} to ${JSON.stringify(change.value)}.` - : 'Configuration updated.'; + app.updateModelContext({ - content: [{ type: 'text', text: `${changeText} Snapshot (${reason}): ${values}` }], + content: [{ type: 'text', text: `Updated ${change.key} to ${JSON.stringify(change.value)} (${reason}).` }], structuredContent: { reason, - changedKey: change?.key, - changedValue: change?.value, - entries: payload.entries.map((entry) => ({ - key: entry.key, - label: entry.label, - value: entry.value, - valueType: entry.valueType, - })), + changedKey: change.key, + changedValue: change.value, }, }).catch(() => { // Host may not support updateModelContext; avoid repeated failed calls. @@ -809,6 +891,12 @@ export function bootstrapConfigEditorApp(): void { syncModelContext('widget-edit', change); }, onTooltip: showTooltip, + onExpandedChanged: (expanded) => { + trackConfigUiEvent(expanded ? 'expand' : 'collapse', { + tool_name: GET_CONFIG_TOOL_NAME, + expanded, + }); + }, }); markReady(); }); @@ -836,6 +924,15 @@ export function bootstrapConfigEditorApp(): void { controller.setPayload(payload); widgetState.write(payload); scheduleRender(); + + if (!configEditorShownEventSent) { + configEditorShownEventSent = true; + // One-shot impression event for get_config UI card visibility. + trackConfigUiEvent('config_editor_shown', { + tool_name: GET_CONFIG_TOOL_NAME, + entry_count: payload.entries.length, + }); + } }; const refreshConfigFromServer = async (): Promise => { diff --git a/src/ui/file-preview/src/app.ts b/src/ui/file-preview/src/app.ts index f44c2df1..c66c1fa3 100644 --- a/src/ui/file-preview/src/app.ts +++ b/src/ui/file-preview/src/app.ts @@ -12,6 +12,7 @@ import { createCompactRowShellController, type ToolShellController } from '../.. import { createWidgetStateStorage } from '../../shared/widget-state.js'; import { renderCompactRow } from '../../shared/compact-row.js'; import { connectWithSharedHostContext, isObjectRecord, type UiChromeState } from '../../shared/host-context.js'; +import { createUiEventTracker } from '../../shared/ui-event-tracker.js'; import { App } from '@modelcontextprotocol/ext-apps'; let isExpanded = false; @@ -761,13 +762,13 @@ export function bootstrapApp(): void { }); }; - trackUiEvent = (event: string, params: Record = {}): void => { - void rpcCallTool?.('track_ui_event', { - event, + trackUiEvent = createUiEventTracker( + (name, args) => app.callServerTool({ name, arguments: args }), + { component: 'file_preview', - params: { tool_name: 'read_file', ...params } - }).catch(() => {}); - }; + baseParams: { tool_name: 'read_file' }, + } + ); // Register ALL handlers BEFORE connect app.onteardown = async () => { diff --git a/src/ui/shared/ui-event-tracker.ts b/src/ui/shared/ui-event-tracker.ts new file mode 100644 index 00000000..9ddde38b --- /dev/null +++ b/src/ui/shared/ui-event-tracker.ts @@ -0,0 +1,43 @@ +type UiEventParamValue = string | number | boolean | null; + +export type UiEventParams = Record; + +type ToolCaller = (name: string, args: Record) => Promise; + +export interface UiEventTrackerOptions { + component: string; + baseParams?: UiEventParams; +} + +function normalizeUiEventParams(params: Record | undefined): UiEventParams { + const normalized: UiEventParams = {}; + + if (!params) { + return normalized; + } + + for (const [key, value] of Object.entries(params)) { + if (typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean' || value === null) { + normalized[key] = value; + } + } + + return normalized; +} + +export function createUiEventTracker(callTool: ToolCaller, options: UiEventTrackerOptions) { + const baseParams = options.baseParams ?? {}; + + return (event: string, params: Record = {}): void => { + void callTool('track_ui_event', { + event, + component: options.component, + params: { + ...baseParams, + ...normalizeUiEventParams(params), + }, + }).catch(() => { + // UI analytics should never block UI interactions. + }); + }; +} diff --git a/src/utils/capture.ts b/src/utils/capture.ts index 8f261e11..364264a2 100644 --- a/src/utils/capture.ts +++ b/src/utils/capture.ts @@ -1,6 +1,6 @@ import { platform } from 'os'; import * as https from 'https'; -import { configManager } from '../config-manager.js'; +import { configManager, isTelemetryDisabledValue } from '../config-manager.js'; import { currentClient } from '../server.js'; let VERSION = 'unknown'; @@ -53,7 +53,6 @@ export function sanitizeError(error: any): { message: string, code?: string } { }; } - /** * Send an event to Google Analytics * @param event Event name @@ -65,7 +64,7 @@ export const captureBase = async (captureURL: string, event: string, properties? const telemetryEnabled = await configManager.getValue('telemetryEnabled'); // If telemetry is explicitly disabled or GA credentials are missing, don't send - if (telemetryEnabled === false || !captureURL) { + if (isTelemetryDisabledValue(telemetryEnabled) || !captureURL) { return; } @@ -368,7 +367,7 @@ const buildEventProperties = async (properties?: any) => { const sendToTelemetryProxy = async (event: string, eventProperties: any) => { try { const telemetryEnabled = await configManager.getValue('telemetryEnabled'); - if (telemetryEnabled === false) return; + if (isTelemetryDisabledValue(telemetryEnabled)) return; const payload = JSON.stringify({ client_id: uniqueUserId, @@ -470,4 +469,4 @@ export const captureRemote = async (event: string, properties?: any) => { ...sanitizedProps, remote: String(true) }); -} \ No newline at end of file +} diff --git a/test/test-telemetry-handling.js b/test/test-telemetry-handling.js new file mode 100644 index 00000000..a32f9fd4 --- /dev/null +++ b/test/test-telemetry-handling.js @@ -0,0 +1,160 @@ +import assert from 'assert'; + +import { + configManager, + isTelemetryDisabledValue, + normalizeTelemetryEnabledValue, +} from '../dist/config-manager.js'; +import { setConfigValue } from '../dist/tools/config.js'; + +function testTelemetryHelpers() { + console.log('\n--- Test: telemetry helper behavior ---'); + + assert.strictEqual(normalizeTelemetryEnabledValue('false'), false); + assert.strictEqual(normalizeTelemetryEnabledValue(' true '), true); + assert.strictEqual(normalizeTelemetryEnabledValue('disabled'), 'disabled'); + + assert.strictEqual(isTelemetryDisabledValue(false), true); + assert.strictEqual(isTelemetryDisabledValue('false'), true); + assert.strictEqual(isTelemetryDisabledValue('FALSE'), true); + assert.strictEqual(isTelemetryDisabledValue(true), false); + assert.strictEqual(isTelemetryDisabledValue('true'), false); + + console.log('ok: telemetry helpers'); +} + +async function testConfigManagerCoercion() { + console.log('\n--- Test: configManager telemetry coercion ---'); + + await configManager.updateConfig({ telemetryEnabled: false }); + await configManager.setValue('telemetryEnabled', 'false'); + + const telemetryEnabled = await configManager.getValue('telemetryEnabled'); + assert.strictEqual(telemetryEnabled, false); + assert.strictEqual(typeof telemetryEnabled, 'boolean'); + + console.log('ok: configManager coercion'); +} + +async function testSetConfigValueCoercion() { + console.log('\n--- Test: set_config_value telemetry coercion ---'); + + await configManager.updateConfig({ telemetryEnabled: false }); + const response = await setConfigValue({ key: 'telemetryEnabled', value: 'false' }); + + assert.ok(response); + assert.notStrictEqual(response.isError, true); + + const telemetryEnabled = await configManager.getValue('telemetryEnabled'); + assert.strictEqual(telemetryEnabled, false); + assert.strictEqual(typeof telemetryEnabled, 'boolean'); + + console.log('ok: set_config_value coercion'); +} + +/** + * Regression test for issue #368: + * When telemetryEnabled is stored as the string 'false' (which happens when + * set via the MCP API), the capture path in captureBase must still treat it + * as disabled. This test reproduces the exact codepath: write string 'false' + * directly to config (bypassing setValue coercion), then read it back via + * getValue and verify isTelemetryDisabledValue gates it. + */ +async function testCapturePathRespectsStringFalse() { + console.log('\n--- Test: capture path respects string "false" (issue #368) ---'); + + // Simulate the pre-fix bug scenario: telemetryEnabled stored as string 'false' + // by writing directly through updateConfig (bypassing setValue normalization). + await configManager.updateConfig({ telemetryEnabled: 'false' }); + + // This is the exact check captureBase and sendToTelemetryProxy perform: + const telemetryEnabled = await configManager.getValue('telemetryEnabled'); + assert.strictEqual( + isTelemetryDisabledValue(telemetryEnabled), + true, + `isTelemetryDisabledValue should return true for stored value ${JSON.stringify(telemetryEnabled)} (type: ${typeof telemetryEnabled})` + ); + + console.log('ok: capture path respects string "false"'); +} + +async function testCapturePathRespectsStringFALSE() { + console.log('\n--- Test: capture path respects string "FALSE" ---'); + + await configManager.updateConfig({ telemetryEnabled: 'FALSE' }); + + const telemetryEnabled = await configManager.getValue('telemetryEnabled'); + assert.strictEqual( + isTelemetryDisabledValue(telemetryEnabled), + true, + `isTelemetryDisabledValue should return true for stored value ${JSON.stringify(telemetryEnabled)}` + ); + + console.log('ok: capture path respects string "FALSE"'); +} + +async function testCapturePathRespectsBooleanFalse() { + console.log('\n--- Test: capture path respects boolean false ---'); + + await configManager.updateConfig({ telemetryEnabled: false }); + + const telemetryEnabled = await configManager.getValue('telemetryEnabled'); + assert.strictEqual( + isTelemetryDisabledValue(telemetryEnabled), + true, + `isTelemetryDisabledValue should return true for stored value ${JSON.stringify(telemetryEnabled)}` + ); + + console.log('ok: capture path respects boolean false'); +} + +async function testCapturePathAllowsTrueValues() { + console.log('\n--- Test: capture path allows true values ---'); + + await configManager.updateConfig({ telemetryEnabled: true }); + let telemetryEnabled = await configManager.getValue('telemetryEnabled'); + assert.strictEqual(isTelemetryDisabledValue(telemetryEnabled), false, 'boolean true should not be disabled'); + + await configManager.updateConfig({ telemetryEnabled: 'true' }); + telemetryEnabled = await configManager.getValue('telemetryEnabled'); + assert.strictEqual(isTelemetryDisabledValue(telemetryEnabled), false, 'string "true" should not be disabled'); + + console.log('ok: capture path allows true values'); +} + +export default async function runTests() { + const originalConfig = await configManager.getConfig(); + + try { + testTelemetryHelpers(); + await testConfigManagerCoercion(); + await testSetConfigValueCoercion(); + await testCapturePathRespectsStringFalse(); + await testCapturePathRespectsStringFALSE(); + await testCapturePathRespectsBooleanFalse(); + await testCapturePathAllowsTrueValues(); + + console.log('\nTelemetry handling tests passed.'); + return true; + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + console.error('Telemetry handling test failed:', message); + if (error instanceof Error && error.stack) { + console.error(error.stack); + } + return false; + } finally { + await configManager.updateConfig(originalConfig); + } +} + +if (import.meta.url === `file://${process.argv[1]}`) { + runTests() + .then((success) => { + process.exit(success ? 0 : 1); + }) + .catch((error) => { + console.error('Unhandled error:', error); + process.exit(1); + }); +}